wedge build: web_search + LSP client/diagnostics (stages 01-03)#175
Conversation
Adds a web_search tool alongside web_fetch: discovers URLs via a pluggable searchBackend (env-configured generic JSON backend; swappable for Exa/You/ Tavily/Brave) and formats ranked results as a numbered list. Registered in CoreNetworkTools(). Deviation from the stage spec: the spec said readOnlySafety, but the repo enforces (TestCoreNetworkToolsExposePromptMetadata) that every core network tool is prompt-gated network egress — so web_search matches web_fetch's posture instead of being auto-allowed. Necessary fallout from adding the tool: knownToolNames (+web_search), the network-tools metadata test (now covers both), and a stale 'unknown tool' fixture that had used web_search.
New internal/lsp package: a direct (self-spawned, stdio) Language Server Protocol client so unattended runs see real compiler diagnostics without an open editor. - client.go: JSON-RPC 2.0 with LSP Content-Length framing; concurrent Call/Notify, id-matched responses, server->client request auto-reply, notification handler, context cancellation, read-loop teardown. - server.go: process lifecycle (spawn via os/exec, initialize/initialized handshake, graceful shutdown+exit with kill fallback). Handshake factored out (performInitialize) for pipe-based testing. - registry.go: extension -> server command (.go/.ts/.tsx/.js/.jsx/.py/.rs), languageId map, PATH availability via exec.LookPath. - types.go: minimal LSP types + PathToURI/URIToPath (Windows-drive aware). Stdlib-only; tested over in-memory pipes with a stub server (no real gopls), including -race for the concurrent id router.
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughAdds an LSP subsystem (types, framed JSON‑RPC client, server orchestration, session sync, manager), a pluggable web_search tool with HTTP backend, and a bounded post-edit SelfCorrector integrated into the agent loop; includes unit and integration tests for these behaviors. ChangesLSP Subsystem
Web Search Tool
Agent Self-Correction
Sequence DiagramsequenceDiagram
participant Manager
participant Session
participant Client
participant LSPProcess
Manager->>Session: Check / sync(text)
Session->>Client: Notify/Call (didOpen/didChange)
Client->>LSPProcess: writeMessage (Content-Length framed)
LSPProcess-->>Client: framed response or notification
Client->>Session: deliver response or invoke NotificationHandler
Session-->>Manager: publishDiagnostics (debounced)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 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/lsp/client.go`:
- Around line 101-140: Close() currently doesn't prevent new Calls/Notifies and
pending entries can leak; fix by making Close() set the closed state (close the
c.closed channel or set a flag) before calling failPending, and have Call()
check that closed state under the same mutex before allocating id/ch and before
calling c.write (if closed return an error immediately). Also ensure the closed
select branch in Call removes c.pending[id] under the mutex before returning
(and similarly ensure Notify() checks closed and returns an error instead of
calling c.write). Use c.mu to serialize checks/registration/cleanup around
pending, closed and nextID; keep existing helpers failPending/readError for
error values.
In `@internal/tools/web_search.go`:
- Around line 164-178: The Search implementation ignores the provider captured
by defaultSearchBackend (backend.provider), so either wire that value into the
outbound HTTP request or remove the config knob; to fix, update
httpSearchBackend.Search to, when backend.provider is non-empty, set a provider
header (e.g. request.Header.Set("X-Zero-Websearch-Provider", backend.provider))
before sending the request so backends that route/auth by provider receive it,
or if you prefer to drop the feature remove the provider field and related env
parsing in defaultSearchBackend.
🪄 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 Plus
Run ID: c52eb571-e931-473d-ba1e-41b5881edbaf
📒 Files selected for processing (13)
internal/lsp/client.gointernal/lsp/client_test.gointernal/lsp/registry.gointernal/lsp/registry_test.gointernal/lsp/server.gointernal/lsp/types.gointernal/lsp/types_test.gointernal/specialist/manifest.gointernal/specialist/manifest_test.gointernal/tools/registry.gointernal/tools/registry_test.gointernal/tools/web_search.gointernal/tools/web_search_test.go
Makes the LSP client produce actionable signal so ZERO can ask 'did my edit compile?' headlessly. - documents.go: per-server session with document lifecycle (didOpen/didChange full-sync/didClose), a URI->diagnostics store fed by publishDiagnostics, and WaitForDiagnostics with a publish-baseline + 300ms quiet debounce (servers never signal 'analysis complete'). - diagnostics.go: FormatDiagnostics (path:line:col: severity: message, 1-based), HasErrors, FilterBySeverity, severity labels. - manager.go: Manager — one lazily-started, reused server per language (keyed by binary), routes a file to the right server, Check(ctx,path,text) -> settled diagnostics, graceful Shutdown; unknown extension -> (nil,nil), not an error. Concurrency-safe; tested under -race over in-memory pipes with a stub server (no real gopls). FormatDiagnostics takes the path explicitly since a Diagnostic carries only a range, not its document.
gnanam1990
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/lsp/diagnostics.go`:
- Around line 57-68: FormatDiagnostics is producing multi-line outputs when
Diagnostic.Message contains newlines/tabs; normalize whitespace so each
diagnostic becomes a single physical line. In FormatDiagnostics (and where
severityLabel/Diagnostic are used), replace trimming with collapsing internal
whitespace (e.g., split on whitespace and join with a single space) or
explicitly replace \n and \t with spaces so the formatted string remains
"path:line:col: severity: message" on one line per diag; ensure you trim
leading/trailing spaces after normalization.
In `@internal/lsp/documents.go`:
- Around line 69-90: The sync function currently increments s.versions[uri] and
sets s.open[uri] before releasing s.mu and calling s.client.Notify, which allows
races and leaves local state advanced on failed Notify; fix by serializing
per-URI work and committing state only after a successful Notify. Introduce a
per-URI lock (e.g. a fileLocks map keyed by URI holding *sync.Mutex or
sync.Mutex) or reuse a per-URI channel so that session.sync (the sync method)
acquires the URI-specific lock, computes/assigns the version only while holding
that lock, performs the Notify call while still serialized for that URI, and
only after Notify returns successfully update s.open[uri] and s.versions[uri]
(or increment the version after a successful notify); ensure Notify failures do
not leave versions advanced and always release the per-URI lock when done. Use
identifiers: session.sync, s.versions, s.open, s.client.Notify, PathToURI,
TextDocumentItem to locate and change the code.
- Around line 52-65: The publishDiagnostics handler currently overwrites caches
regardless of payload version; update the logic in the block that unmarshals
PublishDiagnosticsParams (payload) to check payload.Version against
s.versions[payload.URI] and skip updating s.diagnostics, s.lastPublish, and
s.publishCount when payload.Version is present and less than the stored
s.versions[payload.URI]; only proceed with the existing
write/delete/waiter-close sequence when payload.Version is absent or >=
s.versions[payload.URI], ensuring waiters are not notified for stale payloads.
In `@internal/lsp/manager.go`:
- Around line 67-72: The current flow always returns sess.diagnosticsFor(uri)
even if waitForDiagnostics timed out without seeing a publish newer than the
baseline; change waitForDiagnostics to return a boolean (e.g., sawNewerPublish
bool) indicating whether a publish newer than sess.publishBaseline(uri) was
observed, update its signature and all callers, then in the Check path after
sess.sync call use the boolean return: if waitForDiagnostics(...) returns false,
return nil or an empty diagnostics slice instead of sess.diagnosticsFor(uri),
otherwise return sess.diagnosticsFor(uri); ensure you update any call sites to
handle the new return value.
- Around line 97-105: Manager.Shutdown currently discards errors from each
sess.server.Shutdown(ctx), yielding false success; change it to capture and
propagate failures: iterate sessions from m.sessions as now but collect errors
from sess.server.Shutdown(ctx) (e.g., append non-nil errors or wrap them with
context including session ID), and after the loop return a combined error (first
error or an aggregated multi-error) instead of nil; keep the same locking
behavior around m.sessions and reference Manager.Shutdown, m.sessions, and
sess.server.Shutdown(ctx) when making the change.
🪄 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 Plus
Run ID: bf52674f-98cc-4732-ac34-1d4a05217a97
📒 Files selected for processing (5)
internal/lsp/diagnostics.gointernal/lsp/diagnostics_test.gointernal/lsp/documents.gointernal/lsp/manager.gointernal/lsp/manager_test.go
- lsp/client.go: Close() now disables the client — Call/Notify reject after close instead of leaking a pending entry / writing to a dead conn. - lsp/documents.go: serialize same-document syncs through the Notify write via a per-URI lock and commit version/open only on success; drop delayed publishDiagnostics for a superseded (older positive) version; WaitForDiagnostics now reports whether a fresh publish arrived. - lsp/manager.go: Check returns (nil,nil) when no publish newer than baseline arrives (don't return a stale prior result for new text); Shutdown joins and returns per-server shutdown errors instead of always reporting success. - lsp/diagnostics.go: FormatDiagnostics collapses embedded whitespace so each diagnostic stays one physical line. - tools/web_search.go: forward ZERO_WEBSEARCH_PROVIDER in the request so the config knob isn't inert. Adds tests: close-disables-client, stale-version drop, multi-line message collapse, Shutdown error propagation, httptest backend (provider+parse).
|
Addressed the review. LSP: |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/lsp/manager_test.go (1)
170-201:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRace condition:
t.Errorf()called from goroutines.Line 188 calls
t.Errorf()from within a goroutine. Thetesting.Tmethods are not safe for concurrent use—calling them from multiple goroutines can cause data races and undefined behavior.🔒 Proposed fix to collect errors safely
func TestManagerCheckConcurrentReusesOneServer(t *testing.T) { var mu sync.Mutex started := 0 + var errs []error base := stubStarter([][]Diagnostic{{{Severity: SeverityWarning, Message: "w"}}}, false) m := fastManager(func(ctx context.Context, command []string, root string) (lspServer, error) { mu.Lock() started++ mu.Unlock() return base(ctx, command, root) }) defer m.Shutdown(context.Background()) var wg sync.WaitGroup for i := 0; i < 4; i++ { wg.Add(1) go func() { defer wg.Done() if _, err := m.Check(context.Background(), "main.go", "package main"); err != nil { - t.Errorf("concurrent Check: %v", err) + mu.Lock() + errs = append(errs, err) + mu.Unlock() } }() } wg.Wait() + for _, err := range errs { + t.Errorf("concurrent Check: %v", err) + } // All four calls target the same .go server; only one session is retained. m.mu.Lock() sessions := len(m.sessions) m.mu.Unlock() if sessions != 1 { t.Fatalf("expected a single reused session, got %d", sessions) } }🤖 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/manager_test.go` around lines 170 - 201, The test calls t.Errorf from multiple goroutines inside TestManagerCheckConcurrentReusesOneServer (the goroutine that calls m.Check), which is unsafe; change the goroutine to capture any error and send it to a buffered channel or append to a mutex-protected slice, then after wg.Wait iterate that collection and call t.Errorf/t.Fatalf from the main test goroutine; keep references to the existing wg, the m.Check call, and the final sessions assertion (m.sessions) so only the error-reporting is moved out of the child goroutines.
🧹 Nitpick comments (1)
internal/lsp/manager_test.go (1)
52-52: 💤 Low valueConsider handling unmarshal errors for test clarity.
The ignored unmarshal error means malformed JSON will silently skip the message. While acceptable for a test stub, explicitly checking and logging errors would make test failures clearer if the stub receives unexpected input.
🤖 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/manager_test.go` at line 52, The json.Unmarshal call currently ignores its error (json.Unmarshal(body, &msg)); change it to capture and handle the error and fail the test so malformed JSON surface clearly: assign err := json.Unmarshal(body, &msg) and if err != nil call t.Fatalf or t.Errorf (e.g., t.Fatalf("failed to unmarshal message: %v; body=%q", err, body)) so the test stub logs the unmarshalling error and the raw body for debugging.
🤖 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.
Outside diff comments:
In `@internal/lsp/manager_test.go`:
- Around line 170-201: The test calls t.Errorf from multiple goroutines inside
TestManagerCheckConcurrentReusesOneServer (the goroutine that calls m.Check),
which is unsafe; change the goroutine to capture any error and send it to a
buffered channel or append to a mutex-protected slice, then after wg.Wait
iterate that collection and call t.Errorf/t.Fatalf from the main test goroutine;
keep references to the existing wg, the m.Check call, and the final sessions
assertion (m.sessions) so only the error-reporting is moved out of the child
goroutines.
---
Nitpick comments:
In `@internal/lsp/manager_test.go`:
- Line 52: The json.Unmarshal call currently ignores its error
(json.Unmarshal(body, &msg)); change it to capture and handle the error and fail
the test so malformed JSON surface clearly: assign err := json.Unmarshal(body,
&msg) and if err != nil call t.Fatalf or t.Errorf (e.g., t.Fatalf("failed to
unmarshal message: %v; body=%q", err, body)) so the test stub logs the
unmarshalling error and the raw body for debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 875c9c87-3cf9-46b2-a1d6-4ddf60d779f2
📒 Files selected for processing (9)
internal/lsp/client.gointernal/lsp/client_test.gointernal/lsp/diagnostics.gointernal/lsp/diagnostics_test.gointernal/lsp/documents.gointernal/lsp/manager.gointernal/lsp/manager_test.gointernal/tools/web_search.gointernal/tools/web_search_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/lsp/diagnostics_test.go
- internal/lsp/client_test.go
- internal/lsp/manager.go
- internal/lsp/documents.go
- internal/tools/web_search.go
- internal/lsp/client.go
- internal/lsp/diagnostics.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Requesting changes for one production behavior gap.
internal/lsp/manager.go:55-64:Manager.Checkpromises graceful degradation when no language server is available, but it only checksServerFor(path)and then callssessionFor, which eventually runsStartServer. On a clean machine withoutgopls,pyright-langserver,rust-analyzer, etc., a configured extension returns an executable-not-found error instead of(nil, nil). Since LSP diagnostics are an opportunistic foundation layer, missing local language servers should degrade the same way unsupported extensions do. Please check availability before starting, or translate executable-not-found from the starter into no diagnostics, and add a test for a configured extension whose server binary is unavailable.
Non-blocking note: I saw the CodeRabbit test comments in manager_test.go; the ignored unmarshal is worth cleaning up, but t.Errorf from worker goroutines is not the merge blocker for me here.
Validation I ran on this head:
git diff --check origin/main...HEADpassedgo test ./internal/lsp ./internal/tools ./internal/specialistpassedgo test ./...passedgo run ./cmd/zero-release buildpassed
…review) Manager.Check translated a missing-binary start error (exec.ErrNotFound) into (nil,nil) so a configured extension on a machine without gopls/ pyright/rust-analyzer degrades exactly like an unsupported extension — LSP diagnostics are an opportunistic layer, not a hard dependency. Other start failures still surface. Adds a test injecting an exec.ErrNotFound starter.
|
Addressed @Vasanthdev2004's blocker: |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check the exec.ErrNotFound handling in manager.go
rg -n "ErrNotFound\|errors.Is\|exec.Err" --type=go internal/lsp/manager.go
echo "---"
# Check the test for ErrNotFound starter
rg -n "ErrNotFound\|NotFound\|notFound" --type=go internal/lsp/manager_test.go
echo "---"
# Show the relevant Check method section
ast-grep --pattern 'func (m *Manager) Check($$$) ($$$) { $$$ }'Length of output: 2418 🏁 Script executed: #!/bin/bash
# Find isServerUnavailable definition
rg -n "isServerUnavailable" --type=go
echo "==="
# Find the test for binary-not-found / unavailable
rg -n "unavailable\|notInstalled\|BinaryNotFound\|ErrExec\|exec\.ErrNotFound\|errors\.As\|errors\.Is" --type=go internal/lsp/
echo "==="
# Show full manager.go for context
cat internal/lsp/manager.goLength of output: 5903 🏁 Script executed: #!/bin/bash
cat internal/lsp/manager_test.goLength of output: 7745
Triggering a fresh review now. ✅ Action performedReview finished.
|
* stage 04: self-correct loop After a successful mutating tool call, verify the changed files (LSP diagnostics + project tests) and feed failures back to the model to fix — bounded and autonomy-gated, so an unattended run fixes its own mistakes without looping forever. - internal/agent/selfcorrect.go: SelfCorrector with injectable diagnosticsChecker (lsp.Manager adapter) and projectVerifier (verify.DetectPlan+Run); AfterEdit produces a redacted, model-facing corrective message; per-run attempt ceiling; low autonomy reports without auto-attempting; medium/high auto-correct. - internal/agent/loop.go: invoke AfterEdit after a successful mutating tool call (ChangedFiles non-empty), appending feedback to messages. nil SelfCorrect (the default) is a no-op — read-only tools never trigger it. - internal/agent/types.go: add Options.SelfCorrect *SelfCorrector (the explicit options-struct change). Single-pass verify per call (the agent loop + model edits provide the multi- attempt cycle, not selfverify's same-command re-runs). Tested with fakes (no gopls/shell): fail-then-pass, max-attempts abort, read-only skip, low-autonomy report-only, LSP-only failure, secret redaction; -race clean. CLI activation (a --self-correct flag building the SelfCorrector in exec.go) is intentionally left out to honor this stage's file scope — the engine is exported and ready to wire. * wedge: propagate file-read error in self-correct LSP checker (#176 review) CodeRabbit/golangci-lint (nilerr) flagged lspDiagnosticsChecker.Check returning (nil, nil) after os.ReadFile fails. Propagate the error instead; the sole caller (inspect) already skips files whose Check errors, so a deleted/unreadable file still degrades quietly while future callers can decide. Adds a regression test.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Requesting changes for one new blocker in the stage-04 self-correct integration.
internal/agent/loop.go:334-340: self-correct feedback is appended as aMessageRoleUserimmediately after a successful mutating tool result, while the loop may still have later tool calls from the same assistant message to execute. If a model emits multiple tool calls in one turn and the first mutates, the message sequence becomesassistant(tool_calls: c1,c2) -> tool(c1) -> user(self-correct feedback) -> tool(c2), which violates strict provider replay ordering. Providers like Anthropic require every advertised tool_use to be answered by its tool_result before any new user message. Please queue/aggregate self-correct feedback until after thefor collected.ToolCallsbatch has appended all tool results, or run self-correct once after the batch over the union of changed files. Add a regression test with two tool calls where the first mutates and self-correct emits feedback, asserting all tool messages for the batch precede the self-correct user message.
The previous LSP missing-binary blocker is fixed: Manager.Check now degrades to (nil, nil) for exec.ErrNotFound and keeps other start failures surfaced.
Validation I ran on this head:
git diff --check origin/main...HEADpassedgo test -count=1 ./internal/agent ./internal/lsp ./internal/tools ./internal/specialistpassedgo test ./...passedgo run ./cmd/zero-release buildpassedgo run ./cmd/zero-release smokepassed
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/agent/loop.go (1)
330-342: 💤 Low valueConsider capturing the outcome for observability.
Line 335 discards the
outcomereturn value fromAfterEditwith_. While the feedback content implies the outcome ("stopping auto-correction" for aborted, "reporting only" for reported), capturing the outcome would allow explicit logging or metrics for when the corrector reachesOutcomeAbortedorOutcomeReportedstates without parsing feedback strings.🔍 Optional improvement
- if feedback, _ := options.SelfCorrect.AfterEdit(ctx, toolResult.ChangedFiles); feedback != "" { + if feedback, outcome := options.SelfCorrect.AfterEdit(ctx, toolResult.ChangedFiles); feedback != "" { + // Optional: log or metric the outcome here for observability messages = append(messages, zeroruntime.Message{ Role: zeroruntime.MessageRoleUser, Content: feedback, }) }🤖 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/loop.go` around lines 330 - 342, The AfterEdit call currently ignores the returned outcome; change the call in the block that checks options.SelfCorrect, toolResult.Status and toolResult.ChangedFiles to capture both return values (e.g., outcome, feedback := options.SelfCorrect.AfterEdit(...)) and then use the outcome (compare to SelfCorrectOutcome values like OutcomeAborted and OutcomeReported) to emit a log or metric via the existing logger/metrics path so you can observe aborted vs reported states while still appending non-empty feedback to messages; reference the SelfCorrect interface, its AfterEdit method, the outcome variable, and constants OutcomeAborted/OutcomeReported when implementing the handling.internal/agent/selfcorrect.go (1)
213-216: ⚡ Quick winClarify comment: caller continues rather than "deciding".
The comment states "let the caller decide", but the caller (
inspectat line 115) silently continues rather than deciding how to handle the error. Either update the comment to reflect graceful degradation intent, or changeinspectto distinguish recoverable errors (missing file) from failures (server crash).📝 Suggested clarification
func (c lspDiagnosticsChecker) Check(ctx context.Context, path string) ([]lsp.Diagnostic, error) { text, err := os.ReadFile(path) if err != nil { - return nil, err // unreadable (e.g. deleted) -> let the caller decide + return nil, err // unreadable (e.g. deleted); inspect will skip this file } return c.manager.Check(ctx, path, string(text)) }🤖 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/selfcorrect.go` around lines 213 - 216, The comment in lspDiagnosticsChecker.Check incorrectly says "let the caller decide" even though the caller function inspect silently continues; either update the comment in Check to state it intentionally returns nil,error to allow graceful continuation (e.g., "unreadable (e.g. deleted) -> caller will continue") or change the caller inspect to treat recoverable errors specially: in inspect, detect missing-file or recoverable conditions using os.IsNotExist(err) (or other error classification) and continue, while only bubbling up true failures; reference the lspDiagnosticsChecker.Check function and the inspect function when making this change.
🤖 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/selfcorrect.go`:
- Around line 107-132: SelfCorrector.inspect currently drops non-nil errors from
sc.checker.Check and sc.verifier.Verify; change it to surface those errors in
the returned CorrectionReport instead of silently continuing: for
sc.checker.Check, when err != nil (and not already handled by the checker), set
report.Failed = true and attach a synthetic lsp.Diagnostic for that file (use
lsp.Diagnostic.Message = err.Error() and Severity = lsp.SeverityError) into
report.LSPDiagnostics[file]; for sc.verifier.Verify, when it returns (vr, err)
and err != nil, set report.Failed = true and populate report.VerifyReport with
an error-bearing report (e.g., a VerifyReport/verify.Report instance with
OK=false and an Error/Message field containing err.Error()) so verification
failures are visible to callers; keep the existing logic that treats
vr.OK==false as failure. Ensure you reference SelfCorrector.inspect,
sc.checker.Check, sc.verifier.Verify, report.LSPDiagnostics,
report.VerifyReport, and report.Failed when implementing.
---
Nitpick comments:
In `@internal/agent/loop.go`:
- Around line 330-342: The AfterEdit call currently ignores the returned
outcome; change the call in the block that checks options.SelfCorrect,
toolResult.Status and toolResult.ChangedFiles to capture both return values
(e.g., outcome, feedback := options.SelfCorrect.AfterEdit(...)) and then use the
outcome (compare to SelfCorrectOutcome values like OutcomeAborted and
OutcomeReported) to emit a log or metric via the existing logger/metrics path so
you can observe aborted vs reported states while still appending non-empty
feedback to messages; reference the SelfCorrect interface, its AfterEdit method,
the outcome variable, and constants OutcomeAborted/OutcomeReported when
implementing the handling.
In `@internal/agent/selfcorrect.go`:
- Around line 213-216: The comment in lspDiagnosticsChecker.Check incorrectly
says "let the caller decide" even though the caller function inspect silently
continues; either update the comment in Check to state it intentionally returns
nil,error to allow graceful continuation (e.g., "unreadable (e.g. deleted) ->
caller will continue") or change the caller inspect to treat recoverable errors
specially: in inspect, detect missing-file or recoverable conditions using
os.IsNotExist(err) (or other error classification) and continue, while only
bubbling up true failures; reference the lspDiagnosticsChecker.Check function
and the inspect function when making this change.
🪄 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 Plus
Run ID: 09c2956b-0e8d-4cec-9828-ffb2ddae2a0d
📒 Files selected for processing (4)
internal/agent/loop.gointernal/agent/selfcorrect.gointernal/agent/selfcorrect_test.gointernal/agent/types.go
|
The
Verified by @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify Close/Call/Notify/failPending implementation
cat internal/lsp/client.go
echo "==="
# Check the test
rg -n "TestClientRejectsCallsAfterClose" --type=go -A 30 internal/lsp/client_test.goLength of output: 7949
Triggering the review now. ✅ Action performedReview finished.
|
|
@CodeRabbit approvable ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check open review comments on PR `#175`
gh pr view 175 --repo Gitlawb/zero --json reviewDecision,reviews,state
echo "==="
# List review comments still open/unresolved
gh api repos/Gitlawb/zero/pulls/175/comments --jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: .body[0:200], resolved: .resolved}'
echo "==="
# Get review threads
gh api repos/Gitlawb/zero/pulls/175/reviews --jq '.[] | {id: .id, user: .user.login, state: .state, submitted_at: .submitted_at}'Length of output: 31003
1. Vasanthdev2004 blocker — Self-correct feedback is injected as Anthropic (and strict providers generally) require every 2. My actionable —
Non-blocking (can defer):
|
…iew) SelfCorrector.inspect silently continued on every checker.Check error and ignored verifier errors, so a real LSP startup/sync failure, an unreadable changed file, or a failed plan detection produced report.Failed=false -> OutcomePassed with no signal. Manager.Check already degrades missing servers to (nil,nil) and DetectPlan returns an empty plan (not an error) when nothing is detected, so a non-nil error from either is a genuine failure. Record such errors in CorrectionReport.InspectErrors, fail the pass, and surface them in the model feedback. Adds regression tests for both paths.
|
Verified each open thread against current HEAD (
The one genuinely-open item is fixed in
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check selfcorrect.go inspect function for error handling
rg -n "InspectErrors\|report\.Failed\|checker\.Check\|verifier\.Verify\|CorrectionReport" --type=go internal/agent/selfcorrect.go
echo "=== inspect function ==="
ast-grep --pattern 'func ($_ *SelfCorrector) inspect($$$) ($$$) { $$$ }'
echo "=== CorrectionReport struct ==="
rg -n "CorrectionReport" --type=go internal/agent/selfcorrect.go -A 8 -B 2
echo "=== loop.go self-correct injection point ==="
sed -n '320,360p' internal/agent/loop.go
echo "=== full loop tool-call batch handling ==="
rg -n "AfterEdit\|SelfCorrect\|changedFiles\|ChangedFiles\|tool_result\|toolResult" --type=go internal/agent/loop.go -C 3Length of output: 5028
However, the // loop.go — inside: for index, call := range collected.ToolCalls {
if options.SelfCorrect != nil && toolResult.Status == tools.StatusOK && len(toolResult.ChangedFiles) > 0 {
if feedback, _ := options.SelfCorrect.AfterEdit(ctx, toolResult.ChangedFiles); feedback != "" {
messages = append(messages, zeroruntime.Message{Role: MessageRoleUser, Content: feedback})
}
}
// ← loop continues to tool_result(c2), tool_result(c3)…In a multi-tool-call turn this produces: Anthropic requires all // after the for loop, before the next turn
var batchChangedFiles []string
for _, tr := range toolResults { // or accumulate inside the loop
if tr.Status == tools.StatusOK {
batchChangedFiles = append(batchChangedFiles, tr.ChangedFiles...)
}
}
if options.SelfCorrect != nil && len(batchChangedFiles) > 0 {
if feedback, _ := options.SelfCorrect.AfterEdit(ctx, batchChangedFiles); feedback != "" {
messages = append(messages, zeroruntime.Message{Role: MessageRoleUser, Content: feedback})
}
}Not triggering the review yet — this one needs to land first since it will cause live API errors on any prompt that issues two or more tool calls in one turn. |
Keep code/comments provider-neutral: describe the backend interface as 'any hosted search API' rather than naming specific services.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verdict: Request changes
Finding:
[P1] Preserve tool_result ordering when self-correct feedback is generated - internal/agent/loop.go:334
The self-correct feedback is appended inside the loop that is still answering the assistant turn's tool calls. If a model emits multiple tool calls in one turn and an earlier successful mutating tool triggers self-correct feedback, the transcript becomes: assistant tool_use batch -> tool_result for call 1 -> user self-correct feedback -> tool_result for call 2. Strict provider replay expects all tool_result messages for the assistant's advertised tool calls to be contiguous before the next user message, so this can break the next completion request or later session replay/compaction for common multi-tool batches.
Move self-correct feedback emission until after the current tool-call batch has finished, or collect pending feedback during the loop and append it only after every advertised tool call has a matching tool_result or aborted placeholder. Please add a regression test with two tool calls in one assistant turn where the first mutates and self-correct fails, asserting all tool results are recorded before the feedback message.
Validation run locally:
- go test ./...
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
@CodeRabbit approvable ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check current state of AfterEdit/self-correct injection in loop.go
sed -n '290,380p' internal/agent/loop.go
echo "=== rg for AfterEdit usage ==="
rg -n "AfterEdit\|batchChanged\|ChangedFiles\|changedFiles\|SelfCorrect" --type=go internal/agent/loop.go -C 3Length of output: 5131
Anthropic rejects that ordering with a 400. The fix is straightforward — accumulate // inside the for loop — accumulate, don't call AfterEdit yet
var batchChangedFiles []string
// ...existing for loop body...
if toolResult.Status == tools.StatusOK {
batchChangedFiles = append(batchChangedFiles, toolResult.ChangedFiles...)
}
// remove the current AfterEdit block entirely from the loop
// after the for loop, before the model-escalation block
if options.SelfCorrect != nil && len(batchChangedFiles) > 0 {
if feedback, _ := options.SelfCorrect.AfterEdit(ctx, batchChangedFiles); feedback != "" {
messages = append(messages, zeroruntime.Message{
Role: zeroruntime.MessageRoleUser,
Content: feedback,
})
}
}That one change is the last blocker. Everything else ( |
review) Self-correct feedback was appended as a user message inside the per-tool-call loop, so a multi-tool-call turn whose first (mutating) call failed verification produced: assistant tool_use batch -> tool_result 1 -> user feedback -> tool_result 2. A user message interleaved between tool_results breaks strict provider replay and later session replay/compaction. Accumulate feedback during the batch and append it once after the loop (same after-batch pattern already used for mid-run model escalation), so every advertised tool call keeps its tool_result contiguous. Adds a two-tool-call regression test asserting the feedback lands after both tool_results.
|
Fixed in Self-correct feedback is now accumulated during the tool-call batch and appended once after the loop, so every advertised tool call keeps its Added @coderabbitai review |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/loop.go`:
- Around line 263-268: The SelfCorrect.AfterEdit call is currently invoked per
successful mutating tool inside the loop (e.g., where SelfCorrect.AfterEdit and
selfCorrectFeedback are used), which can consume per-run state multiple times
and produce stale diagnostics; instead accumulate ChangedFiles (and any other
edit metadata) across the loop into a single collection, remove per-tool
AfterEdit calls, and call SelfCorrect.AfterEdit once after the batch completes,
then append its returned feedback to selfCorrectFeedback exactly once so
tool_result ordering and single-instance semantics (Options.SelfCorrect) are
preserved.
🪄 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 Plus
Run ID: 6b315c14-3a3a-44d7-98fc-fb49f9d3d090
📒 Files selected for processing (2)
internal/agent/loop.gointernal/agent/loop_test.go
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check the current state of AfterEdit in loop.go
rg -n "AfterEdit\|selfCorrectFeedback\|batchChanged\|ChangedFiles\|changedFiles\|SelfCorrect" --type=go internal/agent/loop.go -C 3
echo "=== loop structure around tool batch ==="
sed -n '280,400p' internal/agent/loop.go
echo "=== test ==="
rg -n "TestRunDefersSelfCorrectFeedbackUntilAfterToolBatch" --type=go -A 60 internal/agent/loop_test.goLength of output: 9204
All prior blockers are resolved. Approving now. [approve] |
…es (#175 review) Deferring only the feedback emission still ran SelfCorrect.AfterEdit once per mutating tool call, which (a) consumes the per-run attempt budget multiple times in a single turn and (b) verifies an intermediate edit that a later call in the same batch already superseded — AfterEdit is documented as one per-run instance holding attempt state, so this is a correctness issue, not just transcript shape. Aggregate every mutating tool's ChangedFiles during the batch and call AfterEdit once afterward over the deduped union, appending its feedback once (still after all tool_results, preserving contiguity). Adds dedupeStrings and a regression test asserting two writes in one turn consume exactly one attempt and emit one feedback message.
|
Good catch — fixed in
Added @coderabbitai review |
|
@CodeRabbit approve |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== AfterEdit / selfCorrect / changedFiles in loop.go ==="
rg -n "AfterEdit\|selfCorrectFeedback\|batchChanged\|changedFiles\|ChangedFiles\|SelfCorrect\|dedupeStrings" --type=go internal/agent/loop.go -C 3
echo ""
echo "=== loop batch region ==="
sed -n '280,420p' internal/agent/loop.go
echo ""
echo "=== dedupeStrings definition ==="
rg -n "dedupeStrings" --type=go internal/agent/ -C 5
echo ""
echo "=== TestRunBatchesSelfCorrectOncePerTurn ==="
rg -n "TestRunBatchesSelfCorrectOncePerTurn" --type=go -A 80 internal/agent/loop_test.goLength of output: 12621
All blockers are resolved. Approving now. [approve] |
✅ Action performedComments resolved and changes approved. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verdict: Approve
I re-reviewed the latest head 14823b6.
No blockers found.
What I checked:
web_searchis prompt-gated network egress, registered in the core network tool set, and the configured provider is now forwarded to the backend request body.- LSP client/session fixes address the earlier close/pending-call and stale diagnostics concerns.
- Self-correct feedback is now batched after all tool results, preserving strict provider replay ordering, and
AfterEditruns once over the deduped changed-file union.
Validation run locally:
gofmt -l internal/agent internal/lsp internal/tools internal/specialistcleangit diff --check origin/main...HEADcleango test -count=1 ./internal/agent ./internal/lsp ./internal/tools ./internal/specialistgo test ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smoke
Note: I attempted go test -race ./internal/lsp ./internal/agent, but this Windows environment cannot run race builds because gcc is missing for cgo.
2 participants
First three stages of the ZERO wedge build pack — the model-agnostic agent that runs unattended and fixes its own mistakes. Each stage is TDD-gated and tagged (
zero-wedge-01..03). Stdlib-only; clean-room (Go implemented fresh from behavioural reference, no reproduced source). Verified the built binary runs and registers the new tool.Stage 01 —
web_searchtool (zero-wedge-01)Complements
web_fetch(fetch a known URL) by discovering URLs — what an unattended agent needs when it hits unknown errors or new library versions.internal/tools/web_search.go:webSearchToolover a pluggablesearchBackend; env-configured generic JSON backend (ZERO_WEBSEARCH_BASE_URL/_API_KEY/_PROVIDER), swappable for Exa/You/Tavily/Brave.query+ clampedlimit(default 5, cap 10); numbered-list output; output redaction; clear "no search backend configured" error.CoreNetworkTools().Intentional deviation from the stage prompt: the prompt said
readOnlySafety, but the repo's ownTestCoreNetworkToolsExposePromptMetadataenforces that every core network tool is prompt-gated egress. Shipping an auto-allowed network tool (and weakening that test) would be a security regression, soweb_searchmatchesweb_fetch'snetwork/promptposture — confirmed in the built binary (web_search [network/prompt]).Stage 02 — LSP client foundation (
internal/lsp) (zero-wedge-02)A direct (self-spawned, stdio) Language Server Protocol client so an unattended run sees real compiler diagnostics with no open editor.
client.go: JSON-RPC 2.0 + LSPContent-Lengthframing; concurrent id-matchedCall/Notify, server→client request auto-reply, notification handler, context cancellation.server.go: lifecycle (spawn,initialize/initializedhandshake, graceful shutdown+exit with kill fallback).registry.go: extension → server command (.go/.ts/.tsx/.js/.jsx/.py/.rs), languageId, PATH availability.types.go: minimal LSP types +PathToURI/URIToPath(Windows-drive aware).Stage 03 — LSP document sync + diagnostics (
zero-wedge-03)Turns the client into actionable signal: "did my edit compile?", headless.
documents.go: per-server session — document lifecycle (didOpen / full-sync didChange / didClose), URI→diagnostics store fed bypublishDiagnostics,WaitForDiagnosticswith a publish-baseline + 300ms quiet debounce.diagnostics.go:FormatDiagnostics(path:line:col: severity: message, 1-based),HasErrors,FilterBySeverity.manager.go:Manager— one lazily-started, reused server per language;Check(ctx,path,text) → settled diagnostics; unknown extension →(nil,nil).A TDD-caught bug worth noting:
WaitForDiagnosticsinitially returned the previous check's diagnostics (keyed off global last-publish time); fixed with a per-URI publish counter so each check waits for its own publish.Verification
Per-stage gate, all green:
go build ./..., fullgo test ./...,gofmt -lclean. LSP concurrency tested with-raceover in-memory pipes (stub server, no realgopls). Built the binary locally: runs, listsweb_searchas prompt-gated,internal/lspcompiled in.Notes / sequencing
internal/lsphas no CLI surface yet — it's foundation consumed by stage 04 (the self-correct loop), which will wire testrunner/verify/selfverify/LSP intointernal/agent/loop.go. That stage touches the core agent loop (conflict-prone vs in-flight TUI/loop PRs) and is intentionally not in this PR — it should land deliberately after sequencing.Summary by CodeRabbit
New Features
Utilities
Tests