Skip to content

Fix edit_file corruption and matching, shell timeout process groups, grep/glob read boundary - #24

Merged
enowdev merged 10 commits into
enowdev:mainfrom
nvdorman:fix/edit-file-vps-tools
Aug 12, 2026
Merged

Fix edit_file corruption and matching, shell timeout process groups, grep/glob read boundary#24
enowdev merged 10 commits into
enowdev:mainfrom
nvdorman:fix/edit-file-vps-tools

Conversation

@nvdorman

Copy link
Copy Markdown
Contributor

Summary

  • edit_file: the adjacent-insertion recovery replaced the first substring occurrence of the matched line anywhere in the file, corrupting an unrelated line mid-text while reporting success. It now splices by the matched line's byte range and never combines with replace_all.
  • edit_file matching: the verbatim old_string is tried before any EOL normalization, fileEOL picks the majority flavor (one stray CR/CRLF no longer poisons every multi-line edit on a file), and NUMBER| prefix stripping requires consecutive numbers so pipe-delimited data is never mangled. Adds stale-edit diagnostics: ambiguous matches list their line numbers, near-miss lines are surfaced, mixed pastes are called out.
  • read_file: no longer misreads a rune split at the 400 KB cap as binary; lone-CR files display per line.
  • shell: timed-out/cancelled foreground commands are killed with their whole process group (descendants held the shell's pipes and wedged the session); configureProcessGroup preserves existing SysProcAttr instead of clobbering sandbox flags.
  • grep/glob: follow the same read boundary as read_file (project sessions may search outside the workspace); grep reports lines exceeding the scanner buffer instead of silently presenting the rest of the file as match-free.

Merged latest main (PowerShell terminal protocol, MCP background reader, fast model switching) with conflicts resolved to keep both sides' fixes: killLocked keeps main's reap-wait but upgrades to a process-group kill; mcp/client.go takes main's reviewed implementation wholesale.

Test plan

  • New regression tests written first and observed failing: wrong-line splice, replace_all multiplication, mixed-EOL exact match, stray-CR exact match, non-sequential prefix strip, rune-split truncation, lone-CR display, project-session grep/glob boundary, overlong-line report
  • go build ./..., go vet, full go test ./... — green except pre-existing internal/llm live tests requiring a valid OpenAI key; TestStdioRoundTrip / TestModelSetConcurrentWithConfigReads verified flaky-under-load only (pass isolated and on retry, identical code to main)
  • make install-cli build with dashboard embedded; daemon restarted and healthy on the new binary

Made with Cursor

nvdorman and others added 10 commits August 8, 2026 09:23
MCP stdio held a lock for the full RPC duration, so Close/Refresh blocked when
IDA or another backend hung, leaving zombie children and freezing the web UI.
Chat also re-mapped the entire transcript every frame and grew unbounded
reasoning strings during high-effort turns.

Release the send lock while waiting for responses, kill+Wait on Close, batch
stream patches by message only, and make live reasoning display configurable
via display.show_reasoning and display.max_live_reasoning_chars.
Reasoning traces are long decompiler-style text. Rendering them through the
chat Markdown pipeline on expand created hundreds of React nodes and locked
the main thread (Chrome "Page Unresponsive"). Show reasoning as plain
pre-wrap text in a height-capped scroller, and defer the body to the next
frame so the toggle stays responsive.
…-misses

Most edit_file failures are model-side: short old_string hits many sites, or
stale/wrong identifiers (entity vs attachEntity) never exist in the file.
Surface occurrence line numbers when the match is ambiguous, and near-miss
file lines when nothing matches, so the agent re-reads instead of inventing.
Also steer the prompt to require unique context and prefer edit_file over sed.
A foreground command that outlived its timeout kept running behind the
persistent shell, holding its pipes and corrupting every later call in the
session. Put the shell in its own process group, kill the whole group on
timeout or cancellation, and hand the next call a fresh shell.
configureProcessGroup now preserves existing SysProcAttr settings instead of
clobbering sandbox flags.

Co-authored-by: Cursor <cursoragent@cursor.com>
The adjacent-insertion recovery replaced the first substring occurrence of
the matched line anywhere in the file, corrupting an unrelated line mid-text
while reporting success; it now splices by the matched line's byte range and
never combines with replace_all. Matching tries the verbatim old_string
before any EOL normalization, fileEOL picks the majority flavor so one stray
CR/CRLF cannot poison every multi-line edit, and NUMBER| prefix stripping
requires consecutive numbers so pipe-delimited data is never mangled.
read_file no longer misreads a rune split at the 400 KB cap as binary and
displays lone-CR files per line.

Co-authored-by: Cursor <cursoragent@cursor.com>
grep and glob refused paths outside the workspace even in project sessions
where read_file and list_files may read anywhere, which broke searches over
the very files the agent could read. Both now resolve through the same read
boundary. grep also reports when a line exceeds the scanner buffer instead
of silently presenting the rest of the file as match-free.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

# Conflicts:
#	internal/config/config.go
#	internal/mcp/client.go
#	internal/tools/shell.go
#	web/src/pages/ChatPage.tsx
enowdev pushed a commit that referenced this pull request Aug 12, 2026
…savers

The test asserted a guarantee the handler never made. handleModelSet saves
in a detached goroutine, so twenty overlapping switches land on disk in any
order; demanding that "model-19" specifically won the race failed whenever
the scheduler finished the savers out of order. It reproduced reliably at
GOMAXPROCS=2 and was the sole red check on PRs #23 and #24.

Two changes:

  - Assert what IS ordered (the in-memory pointer swap must show the last
    switch) and only that SOME save landed, not which one.
  - Use a manual temp dir and drain the savers before removing it. The
    detached goroutines outlive the test body, so t.TempDir's RemoveAll was
    racing them into "directory not empty" — the failure the deleted
    mid-test os.Remove was papering over.

Stress: 40x at GOMAXPROCS=2 green (was ~5 failures in 30), and 10x under
-race. Runtime drops from 48s to 3s.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@enowdev
enowdev merged commit 8ccfe98 into enowdev:main Aug 12, 2026
1 of 2 checks passed
enowdev pushed a commit that referenced this pull request Aug 12, 2026
Review follow-ups to PR #24. The adjacent-insertion recovery reports
success, so every case where it guesses wrong is a silent file corruption
the model never learns about. Two of these were reproduced against the
merged code before fixing.

1. A NUMBER| paste reached the fuzzy path. The prefix stayed in the
   anchor's token set, so a stale "12|- **pool39v2** | 14 hand |" still
   scored high against the real line, and the new row was written to the
   file carrying its literal "13|" prefix — reported as "1 replacement(s)".
   Prefixed pastes now never reach the guess; exact matching already
   handles them properly via stripReadFileLinePrefixes.

2. A deleted anchor spliced onto a surviving sibling row. Rows of one
   table share nearly every token by construction, so a removed row
   cleared both the 0.78 similarity floor and the 0.12 margin against its
   neighbour. Edit distance does not separate these either — "2026-02-01"
   vs "2026-03-01" is one character, exactly like a typo. What separates
   them is WHICH characters differ: digits are a line's identifying detail
   (dates, ids, versions, counts) and a rewording keeps them while a
   different row does not. Requiring identical digits admits the
   abbreviation case this recovery exists for and rejects the sibling-row
   confusion. A bounded Levenshtein check backs it up.

3. read_file and edit_file disagreed on a lone CR. read_file split on
   every CR while the file's own regression test treats an embedded CR as
   data, so read_file handed the model line numbers that do not exist in
   the file — manufacturing the stale anchors this PR set out to remove.
   Both sides now split on a lone CR only in a genuinely CR-terminated
   file.

4. kill(-pid) had no fallback. It addresses "the group led by pid", so a
   shell that never became a group leader would take the antares daemon
   down with it. Every path calls configureProcessGroup today and nothing
   enforces that; the PGID is now verified against our own before the
   negative kill, falling back to signalling the single process.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@enowdev

enowdev commented Aug 12, 2026

Copy link
Copy Markdown
Owner

Merged as 8ccfe98. Reviewed the byte-range splice, the majority fileEOL detection, the verbatim-first matching, and the process-group kill; the core fixes hold up, and the grep/glob change matches read_file's existing resolveRead rather than widening anything.

The red go check was not this PR — TestModelSetConcurrentWithConfigReads asserted that model-19 specifically won a race between twenty detached savers. It reproduces on main at GOMAXPROCS=2 and is fixed in 9f2cd57.

Four defects found in review are fixed on top in 9e3710f, two of them reproduced against your merged code first:

  1. NUMBER| paste reached the fuzzy path. The prefix stayed in the anchor's token set, so a stale 12|- **pool39v2** | 14 hand | still scored high, and the new row was written to the file carrying a literal 13| prefix — reported as 1 replacement(s). Prefixed pastes now never reach the guess.
  2. A deleted anchor spliced onto its surviving sibling row. Table rows share nearly every token, so a removed row cleared both the 0.78 floor and the 0.12 margin. Edit distance doesn't separate these either — 2026-02-01 vs 2026-03-01 is one character, same as a typo. The guard now keys on digits, which a rewording preserves and a different row doesn't.
  3. read_file and edit_file disagreed on a lone CR. read_file split on every CR while your own stray-CR test treats an embedded CR as data, so the model got line numbers the file doesn't have — manufacturing the stale anchors this PR set out to remove. Both now split on a lone CR only in a genuinely CR-terminated file.
  4. kill(-pid) had no fallback. Every path calls configureProcessGroup today, but nothing enforces it; if a shell ever isn't a group leader, the negative kill takes the daemon down. The PGID is now checked against our own first.

Regression tests for all four are in internal/tools/file_edit_recovery_test.go. Thanks — the recovery path is genuinely useful, it just needed tighter gates given that it reports success.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants