Wire LSP diagnostics into self-correct (exec + TUI)#191
Conversation
newExecSelfCorrector now backs the --self-correct loop with both halves: the workspace test plan AND an lsp.Manager-based diagnostics checker (IncludeLSP=true). The manager is lazy (Manager.Check degrades a missing language server to nil,nil), and a deferred cleanup shuts it down at run end, terminating any spawned sessions. This revives the internal/lsp client (44 -> 3 unreachable funcs); it was dead because the only consumer passed a nil checker with IncludeLSP=false. Add TestManagerCheckRealGopls: drives NewManager -> Check against a real gopls and asserts a real diagnostic; skips when gopls is absent, so it stays CI-safe.
runAgentWithOptions now builds a SelfCorrector for every turn, so post-edit verification (workspace test plan + LSP diagnostics) runs by default in the TUI. The spec-draft planning path is excluded, matching exec. A per-turn lsp.Manager is created and torn down when the run returns. Auto-fix vs report-only follows the active permission mode (unsafe->high, auto->medium, ask/other->low) via selfCorrectAutonomyForMode.
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughLSP diagnostics are now collected during the post-edit self-correct loop in both the CLI exec path and the TUI. Both entry points create a per-run ChangesLSP diagnostics wiring into self-correct loop
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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}
…fcorrect The TUI default now runs only the LSP-diagnostics half of self-correct — cheap and scoped to the changed files — so it never adds the whole-repo test plan's latency to a turn (and a pre-existing failure can't hijack the agent). The project test plan is opt-in per session via a new /selfcorrect [on|off] command (alias /sc); IncludeTests reads m.selfCorrectTests, default false. exec keeps both halves.
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/tui/commands.go`:
- Around line 208-210: Update the usage field for the /selfcorrect command
definition (at the usage line in the command struct around line 208) to include
all accepted arguments. The usage string currently shows only [on|off] but the
command handler also accepts status, list, tests, full, and lsp as valid
arguments. Expand the usage string to reflect all these options so users can
discover the full command capabilities through the help text.
🪄 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: fdd08b2b-dd43-458a-a987-3e5a01689b95
📒 Files selected for processing (3)
internal/tui/commands.gointernal/tui/model.gointernal/tui/session_controls.go
session.didClose sent a textDocument/didClose notification but was never called (the manager only closes documents via Shutdown), so it was unreachable even from tests and flagged by staticcheck U1000. Removing it takes internal/lsp to 0 truly-dead funcs and 0 U1000 findings; the 2 remaining unreachable-from-main funcs (Available, URIToPath) are public, test-covered API.
The usage/description advertised only [on|off] but the handler also accepts status, list, tests, full, and lsp. Surface the primary verbs (on|off|status) and clarify the on/off behavior so command discovery matches the handler (CodeRabbit).
|
Addressed the @coderabbitai review |
|
✅ Action performedReview finished.
|
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verdict: Request changes
The runtime wiring looks good and validation is green, but one user-facing help issue is still present:
internal/tui/commands.go:208documents/selfcorrect [on|off], whilehandleSelfCorrectCommandalso acceptsstatus,list,tests,full, andlsp.internal/tui/session_controls.go:197returns the same incomplete usage text for invalid arguments.
Please update both usage strings to reflect the accepted forms, for example:
/selfcorrect [status|on|off|tests|full|lsp]
Validation I ran locally on the PR head:
git diff --check origin/main...HEADgo test ./internal/cli ./internal/tui ./internal/lsp ./internal/agent -run 'SelfCorrect|LSP|Manager|Command|Exec' -count=1 -timeout 180sgo test ./internal/cli ./internal/tui ./internal/lsp -count=1 -timeout 240sgo test ./... -timeout 300s
Once the usage text is fixed, I do not see another blocker from this pass.
Both the command definition and the invalid-argument error message now list the full accepted set [status|on|off|tests|full|lsp]; the redundant 'list' alias is dropped so docs and handler agree exactly. Addresses Vasanth's review (the error-message usage at session_controls.go was still [on|off]) and CodeRabbit's help-text finding.
|
@Vasanthdev2004 thanks — fixed in e746120. Both usage strings now list the full accepted set and match the handler exactly:
Dropped the redundant @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/tui/commands.go (1)
210-210: ⚡ Quick winConsider clarifying "off" semantics for UX.
The phrase "off/lsp return to LSP-only" may confuse users—"off" could be interpreted as "disable entirely" rather than "disable tests, keep LSP." More explicit phrasing would improve discoverability: e.g.,
"off/lsp disable tests (LSP-only)"or"off/lsp: LSP-only mode".📝 Example revision
- description: "Show or set post-edit self-correction depth (LSP-only default; on/tests/full add the project test plan; off/lsp return to LSP-only).", + description: "Show or set post-edit self-correction depth (LSP-only default; on/tests/full add the project test plan; off/lsp disable tests, LSP-only).",🤖 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/commands.go` at line 210, The description string for the post-edit self-correction depth option in the commands.go file is ambiguous. The phrase "off/lsp return to LSP-only" may confuse users who could interpret "off" as disabling the feature entirely rather than disabling tests while keeping LSP functionality. Replace this phrase with clearer language that explicitly states what the "off" option does, such as "off/lsp disable tests (LSP-only)" or "off/lsp: LSP-only mode" to improve user discoverability and reduce confusion about the actual behavior.
🤖 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/tui/commands.go`:
- Line 210: The description field in the commands.go file contains a comma
splice where two independent clauses are joined by a comma instead of a
semicolon. In the description string that starts with "Show or set post-edit
self-correction depth", replace the comma before "off/lsp return to LSP-only"
with a semicolon to properly separate the independent clauses and maintain
grammatical correctness in the user-facing help text.
---
Nitpick comments:
In `@internal/tui/commands.go`:
- Line 210: The description string for the post-edit self-correction depth
option in the commands.go file is ambiguous. The phrase "off/lsp return to
LSP-only" may confuse users who could interpret "off" as disabling the feature
entirely rather than disabling tests while keeping LSP functionality. Replace
this phrase with clearer language that explicitly states what the "off" option
does, such as "off/lsp disable tests (LSP-only)" or "off/lsp: LSP-only mode" to
improve user discoverability and reduce confusion about the actual behavior.
🪄 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: b0065e33-0a10-4c96-9352-5b18dd7b2233
📒 Files selected for processing (2)
internal/tui/commands.gointernal/tui/session_controls.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/tui/session_controls.go
Reword the description so 'off/lsp' reads as 'disable tests, LSP-only' rather than the ambiguous 'return to LSP-only', which could be misread as disabling the feature entirely (CodeRabbit).
|
Addressed the description nitpick in 0b634f5 — @coderabbitai review |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verdict: Approved
The previous requested change is fixed on the latest head (e746120):
internal/tui/commands.gonow documents/selfcorrect [status|on|off|tests|full|lsp].internal/tui/session_controls.gonow returns the same usage string for invalid args.- The handler accepts exactly those documented args now (
status,on,off,tests,full,lsp).
I rechecked the LSP self-correct wiring and did not find a functional blocker. One note: CodeRabbit has a fresh wording nit on the /selfcorrect description (off/lsp return to LSP-only could be clearer), but I do not consider that a blocker from this review.
Validation run locally on PR head:
git diff --check origin/main...HEADgo test ./internal/cli ./internal/tui ./internal/lsp ./internal/agent -run 'SelfCorrect|LSP|Manager|Command|Exec' -count=1 -timeout 180sgo test ./internal/cli ./internal/tui ./internal/lsp -count=1 -timeout 240sgo test ./... -timeout 300sgo vet ./...go build ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smoke
|
✅ Action performedReview finished.
|
2 participants
Summary
Wires the LSP-diagnostics half of the post-edit self-correct loop. It was built and
tested but never connected — the only consumer passed a
nilchecker withIncludeLSP=false, which left the entireinternal/lspclient unreachable.internal/cli/exec.go):newExecSelfCorrectornow backs--self-correctwithboth halves — the workspace test plan and an
lsp.Manager-based diagnostics checker(
IncludeLSP=true). The manager is lazy (Manager.Checkdegrades a missing/unsupportedlanguage server to
nil, nil) and a deferred cleanup shuts it down at run end.internal/tui/model.go): self-correct now runs by default every turn, butkept fast — only the LSP-diagnostics half, scoped to the changed files. The whole-repo
test plan is opt-in per session via a new
/selfcorrect [on|off]command (alias/sc), so it never addsgo test ./...latency to a turn and a pre-existing failure can'thijack the agent. Spec-draft (planning) is excluded; the per-turn
lsp.Manageris torndown at run end; auto-fix vs report-only follows the active permission mode
(
unsafe→high,auto→medium,ask→low).TestManagerCheckRealGopls: drivesNewManager → Checkagainst a real gopls andasserts a real diagnostic; skips when gopls is absent, so it stays CI-safe.
Effect
Revives
internal/lsp(44 → 3 unreachable funcs); total prod-unreachable 178 → 135.Why fast-by-default in the TUI
The test half (
go test ./...) is whole-project and slow (up to a 120s timeout); the LSPhalf is change-scoped and cheap. Running the full suite after every interactive edit would
add the suite's latency to each turn and re-surface pre-existing failures every time. So the
TUI defaults to LSP-only and gates the test half behind
/selfcorrect on.execkeeps bothhalves (it is headless and already opt-in via
--self-correct).Verification
go build ./...,go vet,gofmtclean; fullgo test ./...green.--self-correctrun surfaces and fixes a seeded compile error thatthe baseline run leaves broken; the
gopls serveprocess spawns mid-run and is cleanedup at the end.
Commits
/selfcorrecttoggleSummary by CodeRabbit
/selfcorrectcommand (alias/sc) to control post-edit self-correction depth, including LSP-only diagnostics or broader test-plan verification.goplsbinary (skips if not available).