Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions npm/src/agent/ProbeAgent.js
Original file line number Diff line number Diff line change
Expand Up @@ -2771,6 +2771,7 @@ Follow these instructions carefully:
* For rewriting entire functions/classes/methods, use the symbol parameter instead (no exact text matching needed).
* For editing specific lines from search/extract output, use start_line (and optionally end_line) with the line numbers shown in the output.${this.hashLines ? ' Line references include content hashes (e.g. "42:ab") for integrity verification.' : ''}
* For editing inside large functions: first use extract with the symbol target (e.g. "file.js#myFunction") to see the function with line numbers${this.hashLines ? ' and hashes' : ''}, then use start_line/end_line to surgically edit specific lines within it.
* IMPORTANT: Keep old_string as small as possible — include only the lines you need to change plus minimal context for uniqueness. For replacing large blocks (10+ lines), prefer line-targeted editing with start_line/end_line to constrain scope.
- Use 'create' for new files or complete file rewrites.
- If an edit fails, read the error message — it tells you exactly how to fix the call and retry.
- The system tracks which files you've seen via search/extract. If you try to edit a file you haven't read, or one that changed since you last read it, the edit will fail with instructions to re-read first. Always use extract before editing to ensure you have current file content.` : ''}
Expand Down
1 change: 1 addition & 0 deletions npm/src/agent/shared/prompts.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
- Avoid implementing special cases when a general approach works
- Never expose secrets, API keys, or credentials in generated code. Never log sensitive information.
- Do not surprise the user with unrequested changes. Do what was asked, including reasonable follow-up actions, but do not refactor surrounding code or add features that were not requested.
- When editing files, keep edits focused and minimal. For changes spanning more than a few lines, prefer line-targeted editing (start_line/end_line) over text replacement (old_string) — it constrains scope and prevents accidental removal of adjacent content. Never include unrelated sections in an edit operation.

Check notice on line 90 in npm/src/agent/shared/prompts.js

View check run for this annotation

probelabs / Visor: architecture

architecture Issue

The guidance about preferring line-targeted editing for large blocks is duplicated across three locations: ProbeAgent.js (system message), prompts.js (engineer prompt), and edit.js (error message). While this reinforcement is intentional, ensure all three stay in sync.
Raw output
Consider defining the guidance text in one place and referencing it, or add a comment in each location noting the other locations that need to stay in sync. This reduces risk of drift where one location gets updated but others don't.
- After every significant change, verify the project still builds and passes linting. Do not wait until the end to discover breakage.

# After Implementation
Expand Down
7 changes: 7 additions & 0 deletions npm/src/tools/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,13 @@
return `Error editing file: Multiple occurrences found - the old_string appears ${occurrences} times in ${file_path}. To fix: (1) Set replace_all=true to replace all occurrences, or (2) Include more surrounding lines in old_string to make the match unique (add the full line or adjacent lines for context).`;
}

// Guard against over-scoped text edits (replacing large blocks with tiny replacements)

Check warning on line 451 in npm/src/tools/edit.js

View check run for this annotation

probelabs / Visor: quality

style Issue

Magic numbers 20 and 0.5 are hardcoded without explanation. These threshold values determine when an edit is considered 'over-scoped' but lack self-documenting names or comments explaining the rationale.
Raw output
Extract to named constants with documentation: const MIN_LINES_FOR_SCOPE_CHECK = 20; const MIN_REPLACEMENT_RATIO = 0.5; // Reject edits replacing 20+ lines with less than 50% of original line count

Check notice on line 451 in npm/src/tools/edit.js

View check run for this annotation

probelabs / Visor: quality

logic Issue

The scope check uses `matchTarget` (which may be fuzzy-matched text) rather than the original `old_string`. If fuzzy matching expands the match significantly, the line count could differ from what the user intended. Consider whether the check should use `old_string` or `matchTarget`.
Raw output
Consider whether the line count should be based on the user's original `old_string` or the fuzzy-matched `matchTarget`. Using `matchTarget` is reasonable since that's what will actually be replaced, but document this decision.

Check notice on line 451 in npm/src/tools/edit.js

View check run for this annotation

probelabs / Visor: architecture

architecture Issue

The guard is placed after the uniqueness check but before the replacement. Consider whether this should be earlier in the validation chain (e.g., right after parsing old_string) to fail fast and provide clearer error ordering.
Raw output
The current placement is acceptable, but consider documenting the validation order: (1) path validation, (2) file existence, (3) read-before-write check, (4) mode routing, (5) content validation (uniqueness, over-scope). This makes the validation flow explicit.
const oldLines = matchTarget.split('\n').length;
const newLines = new_string.split('\n').length;
if (oldLines >= 20 && newLines < oldLines * 0.5) {
return `Error editing file: Edit scope too large — replacing ${oldLines} lines with ${newLines} lines risks accidental content deletion. To fix: (1) Use line-targeted editing (start_line/end_line) instead to constrain scope, or (2) Split into smaller, focused edits that each target only the lines you intend to change.`;
}

Check warning on line 457 in npm/src/tools/edit.js

View check run for this annotation

probelabs / Visor: architecture

architecture Issue

The guard thresholds (20 lines, 50% ratio) are hard-coded magic numbers without justification. These values determine when an edit is considered 'over-scoped' but lack documentation explaining why these specific thresholds were chosen or whether they're configurable.
Raw output
Extract these thresholds to named constants at the top of the file with comments explaining the rationale. Consider making them configurable via options if different projects might need different thresholds. Example:

// Thresholds for detecting potentially accidental large-scale deletions
// 20 lines: large enough to catch significant over-scoping but not small edits
// 50% ratio: catches replacement that dramatically shrinks content
const OVER_SCOPE_LINE_THRESHOLD = 20;
const OVER_SCOPE_RATIO_THRESHOLD = 0.5;
// Perform the replacement

Check notice on line 458 in npm/src/tools/edit.js

View check run for this annotation

probelabs / Visor: architecture

architecture Issue

The over-scope validation logic is embedded inline in the main edit flow. The codebase already has validation functions extracted (isPathAllowed, handleSymbolEdit, handleLineEdit). This guard could follow the same pattern for consistency and testability.
Raw output
Consider extracting the validation into a dedicated function like `validateEditScope(oldLines, newLines)` that returns `{ valid: boolean, reason?: string }`. This would make the validation logic reusable, testable in isolation, and consistent with other validation patterns in the file.
let newContent;
if (replace_all) {
Expand Down
53 changes: 53 additions & 0 deletions npm/tests/unit/edit-create-tools.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,59 @@
expect(newContent).toContain('const message = "Goodbye"');
expect(newContent).not.toContain('const message = "Hello"');
});
test('should reject text edit when old_string is much larger than new_string', async () => {

Check notice on line 187 in npm/tests/unit/edit-create-tools.test.js

View check run for this annotation

probelabs / Visor: quality

logic Issue

Missing edge case test at exactly 20 lines with exactly 10 lines replacement (50% ratio). The condition uses `newLines < oldLines * 0.5`, so 20→10 should pass but 20→9 should fail. Testing the boundary ensures the inequality is correct.
Raw output
Add a test case verifying the boundary: 20 old lines with 10 new lines (exactly 50%) should be allowed, and optionally 20 old lines with 9 new lines should be rejected.
// Create a file with 30+ lines
const lines = Array.from({ length: 35 }, (_, i) => `line ${i + 1}: some content here`);
const originalContent = lines.join('\n');
await fs.writeFile(testFile, originalContent);

const edit = editTool({ allowedFolders: [testDir] });

// Try to replace 25 lines with just 2 lines (over-scoped edit)
const oldString = lines.slice(5, 30).join('\n'); // 25 lines
const newString = 'replacement line 1\nreplacement line 2'; // 2 lines

const result = await edit.execute({
file_path: testFile,
old_string: oldString,
new_string: newString
});

expect(result).toContain('Error editing file: Edit scope too large');
expect(result).toContain('25 lines with 2 lines');
expect(result).toContain('line-targeted editing');

// Verify file was not modified
const content = await fs.readFile(testFile, 'utf-8');
expect(content).toBe(originalContent);
});

test('should allow text edit when replacement is proportional', async () => {
// Create a file with 30+ lines
const lines = Array.from({ length: 35 }, (_, i) => `line ${i + 1}: some content here`);
const originalContent = lines.join('\n');
await fs.writeFile(testFile, originalContent);

const edit = editTool({ allowedFolders: [testDir] });

// Replace 25 lines with 20 lines (proportional, no false positive)
const oldString = lines.slice(5, 30).join('\n'); // 25 lines
const newLines = Array.from({ length: 20 }, (_, i) => `new line ${i + 1}: updated content`);
const newString = newLines.join('\n'); // 20 lines

const result = await edit.execute({
file_path: testFile,
old_string: oldString,
new_string: newString
});

expect(result).toContain('Successfully edited');

// Verify the edit was applied
const content = await fs.readFile(testFile, 'utf-8');
expect(content).toContain('new line 1: updated content');
expect(content).not.toContain('line 10: some content here');
});
});

describe('createTool', () => {
Expand Down
Loading