feat: agent quality, caching, retry, and tooling upgrades#506
Conversation
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
kevincodex1
requested review from
Vasanthdev2004,
anandh8x,
gnanam1990 and
jatmn
and removed request for
Vasanthdev2004
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Link an approved parent issue before continuing this PR
CONTRIBUTING.md:19
The contribution policy says community pull requests must be tied to an existing issue that already has theissue-approvedlabel, but this PR is from aCONTRIBUTOR, has no linked issue references, and the PR checklist leaves the approved-issue item unchecked. Please link the approved parent issue, or move the implementation into the project’s internal development flow if this is core-team work. -
[P2] Put spilled command output somewhere the advertised tools can read
internal/tools/spill.go:26
The new truncation notice tells the model to usegreporread_fileon the saved full output, butspillDirwrites that file underos.TempDir()/zero-tool-output. Those native file tools resolve absolute paths against the workspace/read roots, so a normal session cannot follow the notice to read a/tmp/...spill file. Please either store the spill under an accessible session/workspace cache path or change the recovery path/tooling so the model can actually retrieve the elided output without rerunning the command. -
[P2] Emit tool-call callbacks before executing the parallel read batch
internal/agent/loop.go:492
When a run starts a parallel read-only batch,executeParallelReadBatchruns every tool call before the loop reachesOnToolCallfor the first call. The TUI and headless session recorder create their tool-call rows/events fromOnToolCall, so a slowgrep/glob/lsp_navigatebatch now does real work while the UI/session has not recorded that those calls started yet, and any execution-side events can arrive before the corresponding call event. Please preserve the existing callback ordering by emitting the batched calls before starting their goroutines, or by moving the batch execution behind the per-callOnToolCallnotifications.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR adds inline file diagnostics and formatting on write, parallel execution of consecutive read-only tool calls, fuzzy edit fallback matching, spill-to-disk output truncation, models.dev overlay caching, LSP registry and doctor updates, Anthropic prompt caching breakpoints, retry backoff changes, and web_fetch HTML-to-markdown conversion. ChangesAgent tool execution and inline diagnostics
Fuzzy edit matching for edit_file
Tool output spill-to-disk
models.dev overlay and CLI refresh
Exec self-correction and LSP registry updates
Anthropic caching and retry policy
web_fetch HTML conversion
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AgentLoop
participant parallelSafeToolCall
participant executeParallelReadBatch
participant ToolRegistry
AgentLoop->>parallelSafeToolCall: inspect consecutive read calls
parallelSafeToolCall-->>AgentLoop: eligible read batch
AgentLoop->>executeParallelReadBatch: dispatch batch
executeParallelReadBatch->>ToolRegistry: run calls concurrently
ToolRegistry-->>executeParallelReadBatch: ordered ToolResult values
executeParallelReadBatch-->>AgentLoop: precomputed results
sequenceDiagram
participant EditFileTool
participant RunOptions
participant FileDiagnostics
participant LSPManager
EditFileTool->>RunOptions: read options.Diagnostics
RunOptions->>FileDiagnostics: invoke(ctx, absPath)
FileDiagnostics->>LSPManager: Check(absPath) with timeout
LSPManager-->>FileDiagnostics: diagnostics
FileDiagnostics-->>EditFileTool: formatted diagnostics or ""
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
internal/lsp/registry.go (1)
124-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test tying
coreServerBinariestoserverCommands/ServerBinaries().
coreServerBinariesis a hand-maintained string list with no compile-time or test-time link back toserverCommands. A future rename/typo in either list (e.g. renaming the pyright binary) would silently makelspServersCheckmisclassify that server — it would just vanish from both the core and optional buckets sinceServerBinaries()only iterates keys actually present inserverCommands, with no failure signal.✅ Suggested test to add to registry_test.go
func TestCoreServerBinariesAreKnown(t *testing.T) { known := map[string]bool{} for _, b := range ServerBinaries() { known[b] = true } for _, b := range CoreServerBinaries() { if !known[b] { t.Errorf("core binary %q is not present in serverCommands", b) } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/lsp/registry.go` around lines 124 - 140, Add a test that keeps coreServerBinaries in sync with serverCommands via ServerBinaries() so renamed or missing binaries fail loudly. In registry_test.go, add a test around CoreServerBinaries and ServerBinaries that builds a set from ServerBinaries() and asserts every entry returned by CoreServerBinaries() exists there, using the existing symbols coreServerBinaries, CoreServerBinaries, serverCommands, and ServerBinaries to anchor the check.internal/agent/parallel_tools_test.go (1)
132-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead line + misleading comment.
readandwritekeep separate logs (readLog/writeLog), so "Shared log across both probes" is inaccurate, andwrite.mu = sync.Mutex{}just overwrites the already-zero mutex — it does nothing. Drop both to avoid implying cross-probe ordering that the test doesn't actually rely on.♻️ Proposed cleanup
- // Shared log across both probes so relative ordering is observable. - write.mu = sync.Mutex{} registry := tools.NewRegistry()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/parallel_tools_test.go` around lines 132 - 133, Remove the dead `write.mu = sync.Mutex{}` assignment in `parallel_tools_test.go` since `write.mu` is already zero-value initialized and the reset has no effect. Also delete or rewrite the “Shared log across both probes” comment near the `read`/`write` test setup so it matches the actual `readLog` and `writeLog` separation and does not imply shared ordering that the test does not use. Keep the change localized around the `read`, `write`, `readLog`, and `writeLog` setup in the test.internal/tools/edit_replacers.go (1)
47-72: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDisproportionate-match guard can short-circuit the whole cascade.
Once any replacer yields a found candidate that trips
isDisproportionateEditMatch, the function returns the guard error immediately (Line 57-59), before later replacers get a chance to propose a tighter, proportionate span.blockAnchorReplacer(variable block size) is the most likely to emit an oversized span early, which would mask a goodwhitespaceNormalized/indentationFlexible/contextAwarecandidate that runs later. Safety-first is defensible here, but consider collecting the guard rejection and continuing the cascade, only surfacing it if nothing better is found.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/edit_replacers.go` around lines 47 - 72, In edit_replacers.go, the disproportionate-match check inside the replacer cascade is returning immediately from the first oversized candidate and preventing later replacers from finding a better match. Update the logic in the search loop around isDisproportionateEditMatch so it records the guard rejection but continues trying the remaining replacers (including blockAnchorReplacer, whitespaceNormalized, indentationFlexible, and contextAware). Only surface the guard error after the cascade finishes if no proportionate match was found.internal/cli/exec.go (1)
138-142: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBackground refresh is largely a no-op on the exec path (and can leak temp files).
In the long-lived TUI this goroutine finishes fine, but headless
execruns usually return within seconds — the process exits and kills this goroutine before the network fetch (15s timeout) completes, so the cache rarely refreshes for the most automation-heavy path. Worse, if the process dies betweenos.CreateTempandos.RenameinsideRefreshModelsDevCache, an orphanedmodelsdev-*.jsontemp file is left in the cache dir and nothing sweeps it, so it accumulates across runs.Consider either skipping the exec-path refresh (rely on the TUI/next-TUI-run to keep it warm), gating it behind a "did enough time pass and are we interactive" check, or adding a startup sweep of stale
modelsdev-*.jsontemps. Not blocking.internal/tools/spill_test.go (1)
78-94: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNo test asserts the spilled file is actually redacted.
TestSpillTruncatedOutputWritesFilechecks the file's exact content round-trips, but nothing here exercises the redaction path the spill.go docstring promises ("a spilled file never holds a secret the transcript would have hidden"). A test feeding a secret-shaped string (e.g. anAuthorization: Bearer ...header orpassword=...assignment) throughspillTruncatedOutputand asserting it's absent from the written file would have caught theredaction.Options{}gap flagged inspill.go.✅ Suggested additional test
func TestSpillTruncatedOutputRedactsSecrets(t *testing.T) { t.Setenv("TMPDIR", t.TempDir()) path := spillTruncatedOutput("exec_command", "Authorization: Bearer sk-super-secret-token") if path == "" { t.Fatal("spill must return a file path") } content, err := os.ReadFile(path) if err != nil { t.Fatal(err) } if strings.Contains(string(content), "sk-super-secret-token") { t.Fatalf("spilled file must not contain the raw secret: %q", content) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/spill_test.go` around lines 78 - 94, Add a test that verifies spillTruncatedOutput redacts secret-shaped content before writing the spill file, since TestSpillTruncatedOutputWritesFile only checks round-trip text. Extend the spill_test.go coverage with a case that passes an Authorization/Bearer token or password-style string into spillTruncatedOutput and asserts the raw secret is absent from the file contents, using spillTruncatedOutput and os.ReadFile to confirm the redaction path works.internal/tools/spill.go (1)
43-43: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSweep runs synchronously on every spill, and retention is time-only with no size cap.
sweepSpillDirdoes a fullos.ReadDir+ per-entryStaton the shared directory every time a truncation happens (line 43), adding latency to the exec/bash call path. Retention is purely time-based (7 days) with no total size/file-count bound, so a burst of large truncated outputs between sweeps can grow disk usage unboundedly until the next sweep — and the next sweep only triggers on the next spill.Consider running the sweep asynchronously (fire-and-forget goroutine) so it never blocks the tool call, and pairing the time-based eviction with a total-size or file-count cap.
Also applies to: 63-84
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/spill.go` at line 43, The spill cleanup in sweepSpillDir is being run synchronously from the spill path, which adds latency to every truncation; update the spill handling around sweepSpillDir so it runs asynchronously and does not block the exec/bash call flow. Also extend the retention logic in sweepSpillDir to enforce a size or file-count cap in addition to the existing time-based eviction, so disk usage cannot grow unbounded between sweeps.internal/tools/web_fetch_markdown_test.go (1)
88-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for forced
format: "markdown"on non-HTML content.Tests cover default HTML conversion,
format: "raw", non-HTML pass-through underauto, and invalid format — but not the explicit "force conversion" path (format: "markdown"on non-HTML/plain-text body), which is a documented mode in the schema description.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/web_fetch_markdown_test.go` around lines 88 - 127, Add a test in TestWebFetch* coverage for the explicit forced conversion path where WebFetchTool.Run receives format: "markdown" with a non-HTML response, and assert that the body is converted instead of passed through untouched. Use the existing newWebFetchToolWithClient, webFetchTestClient, and webFetchTestResponse helpers to target the format handling logic and verify the markdown output differs from the original plain-text/JSON content, alongside the expected StatusOK and converted metadata if applicable.internal/tools/web_fetch_markdown.go (1)
47-58: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider excluding
data:URIs from link conversion.
javascript:and#hrefs are already skipped to avoid useless links, butdata:URIs (which can embed multi-KB/MB base64 payloads) are passed straight into[label](href). A page with a data-URI download link would blow right through the "conversion shrinks the page by an order of magnitude" goal this feature is built around.♻️ Proposed fix
- if strings.HasPrefix(href, "#") || strings.HasPrefix(strings.ToLower(href), "javascript:") { + lowerHref := strings.ToLower(href) + if strings.HasPrefix(href, "#") || strings.HasPrefix(lowerHref, "javascript:") || strings.HasPrefix(lowerHref, "data:") { return label }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/web_fetch_markdown.go` around lines 47 - 58, The link conversion in webFetchAnchorRe should also skip data: URIs, since they can expand into huge markdown links and defeat the page-shrinking goal. Update the href filtering logic alongside the existing # and javascript: checks in webFetchMarkdown.go so that data: links return just the label instead of being wrapped by fmt.Sprintf("[%s](%s)", ...).internal/tui/model.go (1)
4198-4215: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winInline FileDiagnostics and SelfCorrect (IncludeLSP) both run LSP checks on the same edited file every turn.
options.SelfCorrectis constructed here withIncludeLSP: true(on by default), and itsAfterEditcycle re-checks the turn's changed files via LSP.options.FileDiagnosticsis then wired from the samelspManagerand additionally runs a synchronous LSPCheckper edit/write call (up tofileDiagnosticsTimeout= 10s each, perinternal/agent/file_diagnostics.go). In the common case both features are enabled simultaneously, so a single edit pays for two separate LSP round-trips against the same file/content, and a turn with N edits to one file pays for N+1.Consider either short-circuiting
FileDiagnosticswhenSelfCorrect's LSP pass will cover the same file this turn, or caching the per-file/per-content diagnostics result so the second consumer reuses it instead of re-querying the language server.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/model.go` around lines 4198 - 4215, Inline FileDiagnostics and SelfCorrect are duplicating LSP work on the same edited file each turn. In the options setup where lsp.NewManager, agent.NewSelfCorrector, and agent.NewFileDiagnostics are wired together, make the inline diagnostics path reuse the SelfCorrect IncludeLSP result or skip the extra FileDiagnostics check when SelfCorrect already covers that file/content. Prefer a shared per-file/per-content cache or a short-circuit in the FileDiagnostics path so the same LSP round-trip is not performed twice.internal/tools/edit_file.go (1)
121-130: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStale error message after fuzzy fallback also fails.
When
errEditFuzzyNotFoundis returned, execution falls through to the pre-existing "must match the file byte-for-byte" message — but by then both exact and fuzzy matching failed. This can mislead the model into repeating the same literal-match retry instead of restructuringold_stringfurther.✏️ Proposed message tweak
if occurrences == 0 { - return errorResult("Error: Could not find the exact string to replace in " + relativePath + ". The old_string must match the file byte-for-byte.") + return errorResult("Error: Could not find old_string in " + relativePath + ", even after fuzzy matching. Re-read the file and ensure old_string matches its current content.") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/edit_file.go` around lines 121 - 130, The fallback error in edit_file.go is stale after errEditFuzzyNotFound because the final exact-match message still suggests only byte-for-byte matching; update the error path in the edit flow (around the occurrences == 0 branch in the function handling fuzzy/exact replacement) so the message clearly states that both fuzzy and exact matching failed and prompts the caller to restructure old_string more substantially rather than retrying a literal match.internal/tools/write_file.go (1)
47-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect integration, but duplicated glue with edit_file.go.
The format-on-write → re-baseline → inlineDiagnostics sequence (Lines 113-116 here) is copy-pasted verbatim from
edit_file.go(its Lines 150-153, 164). Both copies rely on identical, easy-to-violate ordering invariants (format beforeFileTracker.Record). A small shared helper, e.g.finalizeWrittenFile(ctx, absolutePath, content, options) (finalContent, diagnosticsSuffix string), would centralize that invariant so future edits can't accidentally desync the two call sites.Also applies to: 113-116, 133-133
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/write_file.go` at line 47, The writeFileTool.RunWithOptions flow duplicates the same format-on-write, re-baseline, and inlineDiagnostics sequencing already present in edit_file.go, so extract that shared ordering into a helper like finalizeWrittenFile(ctx, absolutePath, content, options) that returns the final content and diagnostics suffix. Update both write_file.go and edit_file.go to call the helper so the invariant that formatting happens before FileTracker.Record stays centralized and cannot drift between the two paths.internal/tools/format_on_write.go (1)
89-97: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFormatter failures are fully invisible to the user.
Any formatter error (non-zero exit, timeout, or an unreadable result) is silently swallowed and the unformatted write stands — with no signal at all in the tool output. A user who enabled
ZERO_FORMAT_ON_WRITE=1has no way to notice that formatting silently didn't happen (e.g., a formatter crash on unusual syntax), which can surface later as a confusing CI format-check failure. Consider at least logging the failure (stderr/debug log) even though the write itself should still succeed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/format_on_write.go` around lines 89 - 97, The formatter errors in the format-on-write flow are currently swallowed, so users never learn that formatting failed. Update the logic around formatter.Run() and the subsequent os.ReadFile() in format_on_write to emit a visible stderr/debug log whenever formatting exits non-zero or the formatted file cannot be read, while still returning the original writtenContent so the write succeeds. Keep the fix localized to the format-on-write path so any failure in formatter.Run or reading the rewritten file is reported with enough context to notice the silent fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/agent/file_diagnostics.go`:
- Around line 23-42: The diagnostics formatter is using the absolute filesystem
path, which leaks local path details into prompts and transcripts. Update
NewFileDiagnostics to pass a workspace-relative display path into
lsp.FormatDiagnostics instead of absPath, or adjust FormatDiagnostics to accept
a display path and use that for rendering. Keep the existing manager.Check flow
and only change the path used in the returned diagnostics string.
In `@internal/tools/bash.go`:
- Around line 357-362: Propagate bash output truncation status through the
result path: `formatBashOutput` currently discards the boolean returned by
`truncateExecOutputSpill`, and `run()` never marks `Result.Truncated`, so
callers can’t detect capped output. Update the `formatBashOutput`/`run` flow to
carry that truncation flag through the `Result` like `exec_command` does, while
still returning the existing spill notice text from `truncateExecOutputSpill`.
In `@internal/tools/edit_replacers.go`:
- Around line 110-132: The fuzzy match replacement path in
lineTrimmedReplacer/edit_file is preserving the matched raw span and then doing
a plain strings.Replace, which can drop indentation and corrupt CRLF endings.
Update the edit application flow to reuse the matched span’s leading whitespace
and original line ending style when substituting new_string, so trimmed matches
still replace cleanly without losing tabs/spaces or leaving stray newlines.
In `@internal/tools/spill.go`:
- Around line 24-31: The spillDir helper currently reuses /tmp/zero-tool-output
via os.MkdirAll without checking whether the path already exists as a symlink or
is owned by the current user. Harden spillDir by adding an Lstat/ownership
validation before reuse, or switch to a per-user directory derived from the
current user, so spill files created by the spill helpers cannot be redirected
to an unexpected location.
---
Nitpick comments:
In `@internal/agent/parallel_tools_test.go`:
- Around line 132-133: Remove the dead `write.mu = sync.Mutex{}` assignment in
`parallel_tools_test.go` since `write.mu` is already zero-value initialized and
the reset has no effect. Also delete or rewrite the “Shared log across both
probes” comment near the `read`/`write` test setup so it matches the actual
`readLog` and `writeLog` separation and does not imply shared ordering that the
test does not use. Keep the change localized around the `read`, `write`,
`readLog`, and `writeLog` setup in the test.
In `@internal/lsp/registry.go`:
- Around line 124-140: Add a test that keeps coreServerBinaries in sync with
serverCommands via ServerBinaries() so renamed or missing binaries fail loudly.
In registry_test.go, add a test around CoreServerBinaries and ServerBinaries
that builds a set from ServerBinaries() and asserts every entry returned by
CoreServerBinaries() exists there, using the existing symbols
coreServerBinaries, CoreServerBinaries, serverCommands, and ServerBinaries to
anchor the check.
In `@internal/tools/edit_file.go`:
- Around line 121-130: The fallback error in edit_file.go is stale after
errEditFuzzyNotFound because the final exact-match message still suggests only
byte-for-byte matching; update the error path in the edit flow (around the
occurrences == 0 branch in the function handling fuzzy/exact replacement) so the
message clearly states that both fuzzy and exact matching failed and prompts the
caller to restructure old_string more substantially rather than retrying a
literal match.
In `@internal/tools/edit_replacers.go`:
- Around line 47-72: In edit_replacers.go, the disproportionate-match check
inside the replacer cascade is returning immediately from the first oversized
candidate and preventing later replacers from finding a better match. Update the
logic in the search loop around isDisproportionateEditMatch so it records the
guard rejection but continues trying the remaining replacers (including
blockAnchorReplacer, whitespaceNormalized, indentationFlexible, and
contextAware). Only surface the guard error after the cascade finishes if no
proportionate match was found.
In `@internal/tools/format_on_write.go`:
- Around line 89-97: The formatter errors in the format-on-write flow are
currently swallowed, so users never learn that formatting failed. Update the
logic around formatter.Run() and the subsequent os.ReadFile() in format_on_write
to emit a visible stderr/debug log whenever formatting exits non-zero or the
formatted file cannot be read, while still returning the original writtenContent
so the write succeeds. Keep the fix localized to the format-on-write path so any
failure in formatter.Run or reading the rewritten file is reported with enough
context to notice the silent fallback.
In `@internal/tools/spill_test.go`:
- Around line 78-94: Add a test that verifies spillTruncatedOutput redacts
secret-shaped content before writing the spill file, since
TestSpillTruncatedOutputWritesFile only checks round-trip text. Extend the
spill_test.go coverage with a case that passes an Authorization/Bearer token or
password-style string into spillTruncatedOutput and asserts the raw secret is
absent from the file contents, using spillTruncatedOutput and os.ReadFile to
confirm the redaction path works.
In `@internal/tools/spill.go`:
- Line 43: The spill cleanup in sweepSpillDir is being run synchronously from
the spill path, which adds latency to every truncation; update the spill
handling around sweepSpillDir so it runs asynchronously and does not block the
exec/bash call flow. Also extend the retention logic in sweepSpillDir to enforce
a size or file-count cap in addition to the existing time-based eviction, so
disk usage cannot grow unbounded between sweeps.
In `@internal/tools/web_fetch_markdown_test.go`:
- Around line 88-127: Add a test in TestWebFetch* coverage for the explicit
forced conversion path where WebFetchTool.Run receives format: "markdown" with a
non-HTML response, and assert that the body is converted instead of passed
through untouched. Use the existing newWebFetchToolWithClient,
webFetchTestClient, and webFetchTestResponse helpers to target the format
handling logic and verify the markdown output differs from the original
plain-text/JSON content, alongside the expected StatusOK and converted metadata
if applicable.
In `@internal/tools/web_fetch_markdown.go`:
- Around line 47-58: The link conversion in webFetchAnchorRe should also skip
data: URIs, since they can expand into huge markdown links and defeat the
page-shrinking goal. Update the href filtering logic alongside the existing #
and javascript: checks in webFetchMarkdown.go so that data: links return just
the label instead of being wrapped by fmt.Sprintf("[%s](%s)", ...).
In `@internal/tools/write_file.go`:
- Line 47: The writeFileTool.RunWithOptions flow duplicates the same
format-on-write, re-baseline, and inlineDiagnostics sequencing already present
in edit_file.go, so extract that shared ordering into a helper like
finalizeWrittenFile(ctx, absolutePath, content, options) that returns the final
content and diagnostics suffix. Update both write_file.go and edit_file.go to
call the helper so the invariant that formatting happens before
FileTracker.Record stays centralized and cannot drift between the two paths.
In `@internal/tui/model.go`:
- Around line 4198-4215: Inline FileDiagnostics and SelfCorrect are duplicating
LSP work on the same edited file each turn. In the options setup where
lsp.NewManager, agent.NewSelfCorrector, and agent.NewFileDiagnostics are wired
together, make the inline diagnostics path reuse the SelfCorrect IncludeLSP
result or skip the extra FileDiagnostics check when SelfCorrect already covers
that file/content. Prefer a shared per-file/per-content cache or a short-circuit
in the FileDiagnostics path so the same LSP round-trip is not performed twice.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1c614c75-6c67-4ff9-aed0-b74e9a362571
📒 Files selected for processing (33)
internal/agent/file_diagnostics.gointernal/agent/loop.gointernal/agent/parallel_tools.gointernal/agent/parallel_tools_test.gointernal/agent/types.gointernal/cli/app.gointernal/cli/exec.gointernal/doctor/hardening.gointernal/lsp/registry.gointernal/modelregistry/catalog.gointernal/modelregistry/modelsdev.gointernal/modelregistry/modelsdev_test.gointernal/providers/anthropic/cache_breakpoints_test.gointernal/providers/anthropic/provider.gointernal/providers/providerio/retry.gointernal/providers/providerio/retry_test.gointernal/tools/bash.gointernal/tools/edit_file.gointernal/tools/edit_replacers.gointernal/tools/edit_replacers_test.gointernal/tools/exec_command.gointernal/tools/format_on_write.gointernal/tools/format_on_write_test.gointernal/tools/inline_diagnostics.gointernal/tools/inline_diagnostics_test.gointernal/tools/registry.gointernal/tools/spill.gointernal/tools/spill_test.gointernal/tools/web_fetch.gointernal/tools/web_fetch_markdown.gointernal/tools/web_fetch_markdown_test.gointernal/tools/write_file.gointernal/tui/model.go
|
fix conflict @kevincodex1 |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Gave this a deep pass — ran the touched packages (green, no races surface in the suite), then traced the risky items by hand. Most of it is genuinely solid: the retry backoff + models.dev overlay (bounded, ctx-honored, validated-before-persist, off the hot path), the Anthropic cache breakpoints (valid targets, max-4 respected, incremental-cache pattern preserved, no shared-struct leak), format-on-write (formats before the tracker re-baseline so conflict detection holds, fails closed, off by default), web_fetch HTML→markdown (RE2 so no ReDoS, capped before conversion), the LSP registry, and the spill security hardening (symlink/ownership race is fine under the sticky-/tmp model, secrets redacted before spill) all check out. Nice work.
Two things to fix before it lands, plus the rebase:
1. [blocker] Data race on OnPermission in the parallel read batch — internal/agent/parallel_tools.go:60
executeParallelReadBatch serializes OnPermissionRequest under promptMutex but leaves OnPermission unwrapped, and each batched executeToolCall fires options.OnPermission from its own goroutine (loop.go:1110). The TUI handler appends to shared slices without a lock — rows = append(rows, ...) and sessionEvents = append(sessionEvents, ...) (model.go:4539-4542) — as does the exec recorder. Two batched reads emitting concurrently is a genuine data race on the session recording (corruption or a panic under -race).
It's narrower than "any two reads": buildPermissionEvent early-returns ok=false for a plain auto-allowed workspace read (safety=Allow && action=Allow && !grantMatched && block==nil), so the happy path doesn't fire it. But it does fire when a batched read's sandbox decision is non-trivial — a grant-matched root (common with /add-dir), a blocked path, or a prompt-ish action — so a model reading 2+ files under a granted dir in one batch trips it. It's also a regression: the pre-PR serial flow never had two OnPermission callbacks in flight at once. Cleanest fix is to not fire execution-side callbacks from the batch goroutines at all — collect the events and replay OnPermission (and OnToolCall/OnToolResult, which already replay here) in the sequential consume loop; or, minimally, wrap OnPermission in the same promptMutex.
(This is the area jatmn flagged as F3. The literal ordering claim there is actually benign — OnToolCall/OnToolResult are emitted in original order in the consume loop — but the adjacent OnPermission race is the real issue.)
2. [major] Fuzzy edit can silently edit the wrong span — internal/tools/edit_replacers.go:63
The ambiguity guard only rejects when the resolved literal occurs more than once (strings.LastIndex(content, search) != index). It returns the first unique candidate a strategy produces, without rejecting when the strategy yields two or more distinct-shaped candidates that each occur once. With duplicate blocks at different indentation and a whitespace-drifted old_string (matched by the line-trimmed / indentation-flexible replacers), it edits the first block instead of reporting errEditFuzzyAmbiguous — a silent wrong-span write. Exact match is a true fast path (fuzzy only runs when occurrences==0) and the disproportion + indentation/CRLF handling are all correct, so this is the one real gap. Fix: when replaceAll is false, reject if more than one distinct candidate span survives the literal-existence filter.
Non-blocking follow-up: the spill notice (exec_command.go:929) tells the model to grep/read_file the saved output, but it lands under os.TempDir()/zero-tool-output-<uid>, which the scoped read_file/grep can't open (they resolve absolute paths against the workspace read roots). So the recovery hint usually points somewhere the native tools can't reach. It degrades cleanly to the old truncate-and-drop, so it's a fast-follow, not a blocker — either register the spill dir as a read root when you emit the notice, or drop the "read_file it" line. (This is jatmn's F2 — still open, correctly identified.)
Also needs a rebase (currently CONFLICTING). The issue-approved gate doesn't apply here — this is core-team work, not a community PR.
Fix the race and the fuzzy ambiguity, rebase, and I'm happy to re-approve. The rest is ready.
Ten self-contained improvements to the core agent engine and tools: - edit_file: fuzzy fallback matching (7-strategy cascade with uniqueness protection and a disproportion guard) when old_string does not match byte-for-byte; exact match remains the fast path - edit_file/write_file: inline error-severity LSP diagnostics in tool output so the model sees a break it introduced in the same turn - lsp: expand server registry from 4 to 28 languages; doctor grades only tier-1 servers and lists the long tail informationally - agent: consecutive read-only tool calls execute concurrently (bounded at 8), consumed in original order, never spanning a mutating call - anthropic: cache_control breakpoints on the last two conversation messages so the transcript prefix stays a prompt-cache hit - providerio: retry schedule raised to 6 attempts with exponential 2s-30s backoff when the server sends no Retry-After - tools: truncated exec/bash output spills to a redacted temp file referenced in the truncation notice; bash output (previously unbounded) now shares exec_command's token budget - web_fetch: dependency-free HTML-to-markdown conversion (format: auto|raw|markdown) and larger limits (256KiB default, 2MiB max, 30s) - modelregistry: live models.dev overlay refreshes context limits and base pricing from a TTL-cached fetch; opt-in at the CLI entrypoint - tools: opt-in format-on-write (ZERO_FORMAT_ON_WRITE=1) runs standard formatters across 27 extensions, ordered before the file-tracker re-baseline so conflict detection is preserved
- edit_file: adapt the replacement to the fuzzy-resolved span — when the span is a uniformly deeper-indented copy of old_string (line-trimmed / indentation-flexible matches), the same indentation delta is applied to new_string, and a span ending in a bare CR keeps its CR so CRLF files never gain a mixed line ending; block-anchor matches with drifted interiors are deliberately left untouched - diagnostics: render workspace-relative paths in inline post-edit diagnostics (base name outside the workspace) so absolute paths never leak the local username or directory layout into the model prompt or session transcript - spill: harden the spill directory for shared temp dirs — per-uid name, reject symlinked or non-directory paths (MkdirAll follows symlinks), and on Unix require current-user ownership - bash: the branch's output cap and truncation-flag threading are superseded by main's capture-time bounding (boundedBuffer + budgetBashCapture), which already sets Result.Truncated; this rebase adopts that design and keeps spill-to-disk on exec_command, where the full output is still buffered and a spill is genuinely complete
… spill reachability - agent: serialize OnPermission behind the same mutex as OnPermissionRequest in the parallel read batch. buildPermissionEvent fires for batched reads with a non-trivial sandbox decision (e.g. a grant-matched extra root), and front-end handlers append to shared session-recording state without their own locking — two batched reads emitting concurrently was a data race the pre-batch serial loop never allowed. - edit_file: reject two or more DISTINCT candidate spans from one fuzzy strategy as ambiguous instead of silently editing the first. Duplicate blocks at different indentation each resolve to a unique literal, so the previous occurrence-uniqueness check passed and wrote the wrong span. A single candidate occurring at multiple positions still cascades to stricter strategies as before. - tools: make spill files actually reachable by the recovery path the truncation notice advertises — resolveScopedReadPath now recognizes files inside the per-uid spill dir (containment verified after symlink resolution, so a planted link cannot smuggle an out-of-scope file through the gate); read_file/grep can follow the notice instead of re-running the command.
kevincodex1
force-pushed
the
feat/agent-quality
branch
from
26d7f90 to
4643dc9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/agent/parallel_tools_test.go`:
- Around line 132-133: The current test setup in parallel_tools_test.go does not
actually share ordering state between the read and write probes, so the
write-barrier behavior is never verified. Replace the no-op mutex initialization
in the test setup and the separate read/write logs with a single shared,
mutex-guarded recorder used by both probeTool instances, then update the
assertions in the test body to compare cross-probe event order (for example,
that the write starts after the first read batch ends and finishes before the
second batch starts) using the shared log rather than only readLog indices.
In `@internal/tools/edit_replacers.go`:
- Around line 221-237: The block selection logic in edit_replacers.go should not
choose a single “best” candidate when multiple spans match the same anchors,
because that hides ambiguity from fuzzyEditMatch. Update blockAnchorReplacer to
either return every candidate span that meets editAnchorSimilarityThreshold or
explicitly detect multiple valid matches and fail as ambiguous, using the
existing middleSimilarity and editAnchorSimilarityThreshold checks. Add a test
for the two-block case where the anchors match but the interior differs, to
ensure the edit is not applied to the wrong block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4b1d897e-cf11-4b64-aab6-cd9a94110a14
📒 Files selected for processing (37)
internal/agent/file_diagnostics.gointernal/agent/file_diagnostics_test.gointernal/agent/loop.gointernal/agent/parallel_tools.gointernal/agent/parallel_tools_test.gointernal/agent/types.gointernal/cli/app.gointernal/cli/exec.gointernal/doctor/hardening.gointernal/lsp/registry.gointernal/modelregistry/catalog.gointernal/modelregistry/modelsdev.gointernal/modelregistry/modelsdev_test.gointernal/providers/anthropic/cache_breakpoints_test.gointernal/providers/anthropic/provider.gointernal/providers/providerio/retry.gointernal/providers/providerio/retry_test.gointernal/tools/edit_file.gointernal/tools/edit_replacers.gointernal/tools/edit_replacers_test.gointernal/tools/exec_command.gointernal/tools/format_on_write.gointernal/tools/format_on_write_test.gointernal/tools/inline_diagnostics.gointernal/tools/inline_diagnostics_test.gointernal/tools/registry.gointernal/tools/spill.gointernal/tools/spill_hardening_test.gointernal/tools/spill_owner_unix.gointernal/tools/spill_owner_windows.gointernal/tools/spill_test.gointernal/tools/web_fetch.gointernal/tools/web_fetch_markdown.gointernal/tools/web_fetch_markdown_test.gointernal/tools/workspace.gointernal/tools/write_file.gointernal/tui/model.go
🚧 Files skipped from review as they are similar to previous changes (31)
- internal/tools/format_on_write.go
- internal/tools/inline_diagnostics.go
- internal/tools/inline_diagnostics_test.go
- internal/tui/model.go
- internal/tools/spill_owner_windows.go
- internal/doctor/hardening.go
- internal/agent/file_diagnostics_test.go
- internal/modelregistry/catalog.go
- internal/tools/spill_owner_unix.go
- internal/tools/write_file.go
- internal/agent/file_diagnostics.go
- internal/cli/app.go
- internal/tools/web_fetch_markdown.go
- internal/tools/exec_command.go
- internal/providers/anthropic/cache_breakpoints_test.go
- internal/tools/web_fetch_markdown_test.go
- internal/modelregistry/modelsdev_test.go
- internal/agent/loop.go
- internal/providers/anthropic/provider.go
- internal/tools/registry.go
- internal/modelregistry/modelsdev.go
- internal/agent/parallel_tools.go
- internal/tools/edit_file.go
- internal/agent/types.go
- internal/tools/web_fetch.go
- internal/tools/format_on_write_test.go
- internal/providers/providerio/retry_test.go
- internal/lsp/registry.go
- internal/cli/exec.go
- internal/providers/providerio/retry.go
- internal/tools/edit_replacers_test.go
…serve cross-probe order - edit_file: blockAnchorReplacer now returns every span clearing the similarity threshold (best first) instead of silently picking the single most-similar block. Hiding competitors from the cascade's distinct-candidate check could edit the wrong block when two blocks share anchors and both interiors are plausible; one clear winner still resolves, two plausible blocks now report ambiguity (covered by a two-block test that asserts the file is untouched). - agent tests: the parallel write-barrier test now records both probes into one mutex-guarded shared log and asserts the cross-probe ordering it always claimed to (write starts after the first read batch ends; second read batch starts after the write ends). The previous version only compared indices within the read probe's own log, which a regression letting the write overlap a batch could still pass; the stray no-op mutex assignment is gone.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-reviewed the round-2 fixes — both blockers are closed, and cleanly. Approving.
The OnPermission race — right fix: promptMutex became a single callbackMutex that serializes both OnPermissionRequest and OnPermission, so the batched goroutines can't race on the session-recorder's shared slices. No re-entrancy (the two callbacks fire at different points in executeToolCall, never nested), so no deadlock, and the comment captures the exact scenario.
The fuzzy ambiguity — you took it to the root, further than I'd flagged. fuzzyEditMatch now collects the distinct candidate spans per strategy and returns errEditFuzzyAmbiguous when more than one survives (and it's not replaceAll). And you caught the thing that would have masked it: the block-anchor replacer was collapsing to the single "best" span before the ambiguity check could see the competitors — now it surfaces every qualifying span, so one clear winner still resolves while two plausible blocks correctly reject. That's the wrong-span write closed for good.
F2 (spill reachability) — nice, this went past the non-blocking bar: resolveSpillReadPath lets the scoped read_file/grep open spill files (per-uid, zero-created, redacted), wired into the workspace read resolver with a symlink-resolved containment gate and a hardening test, so the "read_file it" recovery notice actually works now instead of pointing at an unreachable /tmp path.
Rebase is done (MERGEABLE), build's clean, the agent + tools suites pass locally, and all 9 CI checks are green. This was a big, careful PR and the fixes are solid — good to merge.
3 participants
Summary
Ten self-contained improvements to the core agent engine and tools:
Checklist
issue-approvedlabel.go build ./...,go vet ./..., andgo test ./...pass locally.gofmtclean.-racewhere relevant).Summary by CodeRabbit