fix(lint): clear all 109 findings, decided site by site - #27
Conversation
Uncapped count 109 before, 0 after. Every category adjudicated rather than
silenced: no //nolint added except one, and that one carries a deadline.
unused, 28, every one dead scaffolding in test files. Twenty-two were
forwarding shims in compat_adapters_test.go — methods on PrepSubsystem that
call the package-level function of the same name, of which fifty-six are used
and these were not. The rest were a pipelineForgeMetaReader nothing
constructed, three fetch-loop helpers, and boolPtr, assertNotSame and
assertSPDXHeader. Gone, not silenced.
staticcheck, 22 down to 0, of which eight died with the code above.
SA1012 x11 — nil passed as a Context. Every one in tests, all replaced with
context.TODO(); a nil Context is a panic waiting for any callee that reads
from it.
ST1019 — the MCP SDK imported twice in one file, once aliased and once not,
with both spellings in use. Collapsed onto the alias.
S1009 x2 — `x == nil || len(x) == 0`, where len(nil) is already 0.
QF1002 x4 — switch { case r.URL.Path == "..." } turned into a tagged switch.
Checked each block was uniform first; all four were.
SA1019 x2 — httputil Director, deprecated since Go 1.26 and superseded since
1.20, migrated to Rewrite. Not a rename: Rewrite receives both the inbound
and outbound request, so client headers cannot be forwarded upstream by
accident, which is what Director was replaced for. SetURL and
SetXForwarded restore what NewSingleHostReverseProxy's own Director did.
The last SA1019 is session.Log, and it is documented rather than fixed. MCP
protocol 2026-07-28 (SEP-2577) retires the logging feature outright; there is
no successor call, so this cannot be a mechanical swap. The window is at least
twelve months, so it must move before ~2027-07-28, and the comment says so
along with the two candidates — a server notification, or folding the lines
into the status:// resource this file already publishes. Marked with the one
nolint in this change, with the reason and the date.
errcheck, 59 down to 0, and the split is the point. Silence is correct for a
read cursor, a response body, a notification whose listener may be gone, a
`git rebase --abort` where there may be no rebase to abort, and a cleanup of a
directory that may already be deleted. Every one now says so in a comment
rather than being bare.
Reporting is correct where silence loses work, and those get core.Warn, the
idiom already in these files:
writeStatusResult x9 — the status file is what the monitor polls; a silent
failure leaves a workspace looking stuck forever.
performAsyncIfRegistered x6 — a follow-up that fails to dispatch skips a
pipeline stage and nothing downstream notices.
workspaces.Set x7 — Set only fails on a locked or sealed registry, so a
failure means a workspace stops being tracked.
Action.Run x4 — runner.start, stop and kill; a lifecycle transition that
fails silently leaves the runner in an unknown state.
fs.Write x2, CommitToJournal, HTTPPost x3, JSONUnmarshalString, mirror push,
gh pr ready, planCleanup — each loses the artefact or the step it exists
to produce.
Two edits landed on the wrong line while scripting this and are worth naming:
one wrapped a doc comment as if it were a call, and one overwrote a
`if !getResult.OK {` guard. Both were caught by the compiler within the same
minute, and the second was restored from git rather than reconstructed from
memory. Patching by line number needs descending order; ascending shifts every
site after the first insertion.
go build, go vet, go test and gofmt all clean.
Co-Authored-By: Virgil <virgil@lethean.io>
|
Warning Review limit reached
Next review available in: 28 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe changes make ignored operation results explicit, add warnings for selected persistence and integration failures, preserve best-effort behaviour, update reverse-proxy rewriting, and align tests and compatibility adapters with current APIs. ChangesAgentic lifecycle and result handling
Test and compatibility maintenance
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
go/pkg/agentic/handlers.go (2)
66-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse usage-example comments for the new rationale.
The new comments explain why results are checked or discarded. The repository rule requires comments to show how to use the code with real values. Replace the explanatory text with a short usage example, or remove it when the code is self-explanatory. Keep the required
//nolintdirective and move its migration rationale to project documentation if needed.
- go/pkg/agentic/handlers.go#L66-L67: Replace the status-write rationale with a usage example or remove it.
- go/pkg/agentic/handlers.go#L93-L94: Replace the QA dispatch rationale with a usage example or remove it.
- go/pkg/agentic/handlers.go#L112-L113: Replace the auto-PR dispatch rationale with a usage example or remove it.
- go/pkg/agentic/handlers.go#L131-L132: Replace the verification dispatch rationale with a usage example or remove it.
- go/pkg/agentic/handlers.go#L175-L176: Replace the ingest dispatch rationale with a usage example or remove it.
- go/pkg/agentic/handlers.go#L194-L195: Replace the poke dispatch rationale with a usage example or remove it.
- go/pkg/agentic/handlers.go#L216-L217: Replace the harvest auto-PR rationale with a usage example or remove it.
- go/pkg/agentic/review_queue.go#L323-L324: Replace the PR-ready rationale with a usage example or remove it.
- go/pkg/agentic/runner.go#L33-L33: Replace the runner-action rationale with a usage example or remove it.
- go/pkg/agentic/shutdown.go#L74-L74: Replace the start-action rationale with a usage example or remove it.
- go/pkg/agentic/shutdown.go#L87-L87: Replace the stop-action rationale with a usage example or remove it.
- go/pkg/agentic/shutdown.go#L100-L100: Replace the kill-action rationale with a usage example or remove it.
- go/pkg/agentic/message.go#L98-L98: Replace the inbox-listener rationale with a usage example or remove it.
- go/pkg/agentic/message.go#L166-L166: Replace the channel-listener rationale with a usage example or remove it.
- go/pkg/agentic/watch.go#L123-L123: Replace the completed-workspace rationale with a usage example or remove it.
- go/pkg/agentic/watch.go#L145-L145: Replace the merged-workspace rationale with a usage example or remove it.
- go/pkg/agentic/watch.go#L166-L166: Replace the failed-workspace rationale with a usage example or remove it.
- go/pkg/monitor/monitor.go#L499-L499: Replace the inbox-listener rationale with a usage example or remove it.
- go/pkg/monitor/monitor.go#L517-L529: Move the session-log migration rationale to project documentation or replace it with a usage example.
As per coding guidelines, write comments as usage examples showing how to use the code with real values, rather than descriptive explanations.
🤖 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 `@go/pkg/agentic/handlers.go` around lines 66 - 67, Replace the rationale comments with short usage examples using realistic values, or remove them where the surrounding code is self-explanatory, while preserving required //nolint directives. Apply this to go/pkg/agentic/handlers.go lines 66-67, 93-94, 112-113, 131-132, 175-176, 194-195, and 216-217; go/pkg/agentic/review_queue.go lines 323-324; go/pkg/agentic/runner.go line 33; go/pkg/agentic/shutdown.go lines 74, 87, and 100; go/pkg/agentic/message.go lines 98 and 166; go/pkg/agentic/watch.go lines 123, 145, and 166; and go/pkg/monitor/monitor.go lines 499 and 517-529. For the session-log migration rationale in monitor.go, move it to project documentation or replace it with a usage example; do not add explanatory rationale comments.Source: Coding guidelines
93-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn a skipped result for missing follow-up actions.
performAsyncIfRegisteredreturnscore.Result{}whencis nil orc.Action(action).Exists()is false. The callers then emit the same dispatch-failure warning for missingagentic.qa/agentic.auto-pr/agentic.verify/agentic.ingest/agentic.poke, so a completion with an unregistered follow-up produces a false warning. Returncore.Result{OK: true}from the missing-action branch, and keep the dispatch failure only for nil core or registered dispatch failures.Also applies to lines 112-116, 131-135, 175-179, 194-198, and 216-220.
🤖 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 `@go/pkg/agentic/handlers.go` around lines 93 - 97, Update performAsyncIfRegistered so its missing-action branch—when c is nil or c.Action(action).Exists() is false—returns core.Result{OK: true} instead of an empty result. Preserve failure results for registered actions that fail to dispatch, so the existing follow-up callers only warn on actual dispatch failures across agentic.qa, agentic.auto-pr, agentic.verify, agentic.ingest, and agentic.poke.go/pkg/agentic/persist.go (1)
53-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the comment-format rule consistently.
The new comments explain why results are ignored or reported. They do not show how to use the code with real values.
go/pkg/agentic/persist.go#L53-L53: rewrite the cleanup comment as a usage example.go/pkg/agentic/persist.go#L58-L59: rewrite the registry warning rationale as a usage example.go/pkg/agentic/ingest.go#L94-L94: rewrite the HTTP failure rationale as a usage example.go/pkg/agentic/mirror.go#L176-L176: rewrite the mirror failure rationale as a usage example.go/pkg/agentic/plan_retention.go#L85-L85: rewrite the scheduled cleanup rationale as a usage example.go/pkg/agentic/verify.go#L38-L39: rewrite the merged-status rationale as a usage example.go/pkg/agentic/verify.go#L69-L70: rewrite the review-status rationale as a usage example.go/pkg/agentic/verify.go#L123-L123: rewrite the rebase-abort rationale as a usage example.go/pkg/agentic/verify.go#L153-L154: rewrite the label-application rationale as a usage example.go/pkg/agentic/verify.go#L174-L175: rewrite the label-creation rationale as a usage example.go/pkg/agentic/verify.go#L193-L194: rewrite the label-parse rationale as a usage example.go/pkg/agentic/repo_sync.go#L170-L171: rewrite the registration rationale as a usage example.go/pkg/agentic/platform.go#L602-L602: rewrite the response-close rationale as a usage example.go/pkg/agentic/transport.go#L123-L123: rewrite the HTTP response-close rationale as a usage example.go/pkg/agentic/transport.go#L177-L177: rewrite the MCP response-close rationale as a usage example.go/pkg/agentic/transport.go#L201-L201: rewrite the notification response-close rationale as a usage example.go/pkg/agentic/transport.go#L286-L287: rewrite the SSE-drain rationale as a usage example.go/pkg/opencode/proxy.go#L83-L91: replace the API-history explanation with a usage example.As per coding guidelines, write comments as usage examples showing how to use the code with real values, rather than descriptive explanations.
🤖 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 `@go/pkg/agentic/persist.go` at line 53, Rewrite the comments at go/pkg/agentic/persist.go:53-53 and :58-59, ingest.go:94-94, mirror.go:176-176, plan_retention.go:85-85, verify.go:38-39, :69-70, :123-123, :153-154, :174-175, and :193-194, repo_sync.go:170-171, platform.go:602-602, transport.go:123-123, :177-177, :201-201, and :286-287, plus go/pkg/opencode/proxy.go:83-91, replacing each rationale or API-history explanation with a concise usage example containing realistic values while preserving the associated behavior and symbols.Source: Coding guidelines
🤖 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 `@go/pkg/agentic/dispatch.go`:
- Around line 937-939: Handle writeStatusResult failures as blocking errors
rather than warnings at go/pkg/agentic/dispatch.go:937-939, preventing
successful dispatch and running-workspace tracking;
go/pkg/agentic/dispatch_sync.go:112-114, preventing stale-status polling;
go/pkg/agentic/prep.go:470-478, preventing failed registry-state storage;
go/pkg/agentic/queue.go:481-483, preventing successful drainOne completion and
duplicate dispatch; go/pkg/agentic/resume.go:103-105, preventing core.Ok while
resume remains possible; go/pkg/agentic/status.go:220-222, preventing
derived-status success reporting; and go/pkg/agentic/pr.go:153-155, preventing
PR success and cleanup until PRURL is durable. Propagate or return the
persistence failure through each caller’s existing error path.
- Around line 935-936: Replace or remove the descriptive comments at
go/pkg/agentic/dispatch.go:935-936, go/pkg/agentic/dispatch_sync.go:110-111,
go/pkg/agentic/prep.go:443-444, 468-469, 474-475, 491, 493-494, 833-836,
913-914, go/pkg/runner/runner.go:340-341, 415-416, 433, 478-479, 500-501,
go/pkg/agentic/queue.go:479-480, go/pkg/agentic/resume.go:101-102,
go/pkg/agentic/status.go:218-219, and go/pkg/agentic/pr.go:151-152. Any retained
comment must be a concrete usage example with realistic values; otherwise remove
it, without changing the surrounding status, polling, registry, Git, deletion,
or best-effort logic.
In `@go/pkg/agentic/prep.go`:
- Around line 837-841: Guard the remote-branch update logic in the preparation
flow by checking the result of the git checkout invoked before the ls-remote
check. Only run git pull for currentBranch when checkout succeeds; otherwise
skip the pull or return a controlled preparation error, ensuring a failed
checkout cannot update another branch.
In `@go/pkg/agentic/repo_sync_cov_test.go`:
- Line 38: Restore nil-input coverage in
TestRepoSyncCov_RepoSyncContext_Good_NilFallsBackToBackground at
go/pkg/agentic/repo_sync_cov_test.go:38-38 by passing a nil context, or rename
the test if it intentionally covers a non-nil context. At
go/pkg/agentic/review_queue_cov_test.go:312-312, test the nil-context guard
separately, or update the stale comment so it no longer claims nil handling is
covered.
In `@go/pkg/agentic/review_queue.go`:
- Around line 297-299: In the review findings write path, update the handling
around fs.Write so a failed write returns before dispatchFixFromQueue is
invoked. Preserve the existing warning, but ensure dispatchFixFromQueue only
runs after the findings file has been successfully written.
In `@go/pkg/agentic/verify.go`:
- Around line 155-157: Make the label-based review hand-off fail-safe in
go/pkg/agentic/verify.go at lines 155-157 and 193-197: in the label application
flow around HTTPPost, return a failure or schedule a retry when the POST is
unsuccessful instead of only warning; in the label parsing flow, return the
parse failure and prevent HTTPPost until a valid nonzero label ID is available,
ensuring flagForReview never proceeds without a confirmed label.
In `@go/pkg/opencode/proxy.go`:
- Around line 83-96: The authenticated proxy setup around
NewSingleHostReverseProxy must ensure exactly one hook is configured. Clear
rp.Director before assigning the Rewrite callback, while preserving the existing
SetURL, Host, SetXForwarded, and Authorization behavior.
---
Nitpick comments:
In `@go/pkg/agentic/handlers.go`:
- Around line 66-67: Replace the rationale comments with short usage examples
using realistic values, or remove them where the surrounding code is
self-explanatory, while preserving required //nolint directives. Apply this to
go/pkg/agentic/handlers.go lines 66-67, 93-94, 112-113, 131-132, 175-176,
194-195, and 216-217; go/pkg/agentic/review_queue.go lines 323-324;
go/pkg/agentic/runner.go line 33; go/pkg/agentic/shutdown.go lines 74, 87, and
100; go/pkg/agentic/message.go lines 98 and 166; go/pkg/agentic/watch.go lines
123, 145, and 166; and go/pkg/monitor/monitor.go lines 499 and 517-529. For the
session-log migration rationale in monitor.go, move it to project documentation
or replace it with a usage example; do not add explanatory rationale comments.
- Around line 93-97: Update performAsyncIfRegistered so its missing-action
branch—when c is nil or c.Action(action).Exists() is false—returns
core.Result{OK: true} instead of an empty result. Preserve failure results for
registered actions that fail to dispatch, so the existing follow-up callers only
warn on actual dispatch failures across agentic.qa, agentic.auto-pr,
agentic.verify, agentic.ingest, and agentic.poke.
In `@go/pkg/agentic/persist.go`:
- Line 53: Rewrite the comments at go/pkg/agentic/persist.go:53-53 and :58-59,
ingest.go:94-94, mirror.go:176-176, plan_retention.go:85-85, verify.go:38-39,
:69-70, :123-123, :153-154, :174-175, and :193-194, repo_sync.go:170-171,
platform.go:602-602, transport.go:123-123, :177-177, :201-201, and :286-287,
plus go/pkg/opencode/proxy.go:83-91, replacing each rationale or API-history
explanation with a concise usage example containing realistic values while
preserving the associated behavior and symbols.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9826d814-9b15-4bf5-ac10-59e26da03098
📒 Files selected for processing (40)
go/pkg/agentic/commands_test.gogo/pkg/agentic/compat_adapters_test.gogo/pkg/agentic/dispatch.gogo/pkg/agentic/dispatch_sync.gogo/pkg/agentic/dispatch_sync_test.gogo/pkg/agentic/fetch_loop_test.gogo/pkg/agentic/handlers.gogo/pkg/agentic/ingest.gogo/pkg/agentic/message.gogo/pkg/agentic/mirror.gogo/pkg/agentic/persist.gogo/pkg/agentic/pipeline_commands_test.gogo/pkg/agentic/plan_retention.gogo/pkg/agentic/platform.gogo/pkg/agentic/pr.gogo/pkg/agentic/prep.gogo/pkg/agentic/prep_extra_test.gogo/pkg/agentic/qa.gogo/pkg/agentic/queue.gogo/pkg/agentic/repo_sync.gogo/pkg/agentic/repo_sync_cov_test.gogo/pkg/agentic/resume.gogo/pkg/agentic/review_queue.gogo/pkg/agentic/review_queue_cov_test.gogo/pkg/agentic/runner.gogo/pkg/agentic/shutdown.gogo/pkg/agentic/shutdown_example_test.gogo/pkg/agentic/shutdown_test.gogo/pkg/agentic/status.gogo/pkg/agentic/status_test.gogo/pkg/agentic/test_assertions_test.gogo/pkg/agentic/transport.gogo/pkg/agentic/verify.gogo/pkg/agentic/watch.gogo/pkg/agentic/workspace_stats_test.gogo/pkg/lib/lib_test.gogo/pkg/monitor/monitor.gogo/pkg/opencode/imports_test.gogo/pkg/opencode/proxy.gogo/pkg/runner/runner.go
💤 Files with no reviewable changes (5)
- go/pkg/agentic/pipeline_commands_test.go
- go/pkg/agentic/fetch_loop_test.go
- go/pkg/lib/lib_test.go
- go/pkg/agentic/test_assertions_test.go
- go/pkg/agentic/compat_adapters_test.go
| // Reported: the status file is what the monitor polls, so a silent write | ||
| // failure leaves the workspace looking stuck indefinitely. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Apply the comment-format rule to all newly added Go comments.
The listed comments explain internal control flow instead of showing usage with real values. Replace each comment with a usage example or remove it.
- go/pkg/agentic/dispatch.go#L935-L936: Replace the descriptive status-write rationale.
- go/pkg/agentic/dispatch_sync.go#L110-L111: Replace the descriptive polling rationale.
- go/pkg/agentic/prep.go#L443-L444: Replace the descriptive registry rationale.
- go/pkg/agentic/prep.go#L468-L469: Replace the descriptive status-write rationale.
- go/pkg/agentic/prep.go#L474-L475: Replace the descriptive registry rationale.
- go/pkg/agentic/prep.go#L491-L491: Replace the descriptive best-effort rationale.
- go/pkg/agentic/prep.go#L493-L494: Replace the descriptive registry rationale.
- go/pkg/agentic/prep.go#L833-L836: Replace the descriptive Git rationale.
- go/pkg/agentic/prep.go#L913-L914: Replace the descriptive documentation-clone rationale.
- go/pkg/runner/runner.go#L340-L341: Replace the descriptive registry rationale.
- go/pkg/runner/runner.go#L415-L416: Replace the descriptive registry rationale.
- go/pkg/runner/runner.go#L433-L433: Replace the descriptive deletion rationale.
- go/pkg/runner/runner.go#L478-L479: Replace the descriptive best-effort rationale.
- go/pkg/runner/runner.go#L500-L501: Replace the descriptive registry rationale.
- go/pkg/agentic/queue.go#L479-L480: Replace the descriptive status-write rationale.
- go/pkg/agentic/resume.go#L101-L102: Replace the descriptive status-write rationale.
- go/pkg/agentic/status.go#L218-L219: Replace the descriptive status-write rationale.
- go/pkg/agentic/pr.go#L151-L152: Replace the descriptive status-write rationale.
As per coding guidelines, Go comments must be usage examples showing how to use the code with real values, rather than descriptive explanations.
📍 Affects 8 files
go/pkg/agentic/dispatch.go#L935-L936(this comment)go/pkg/agentic/dispatch_sync.go#L110-L111go/pkg/agentic/prep.go#L443-L444go/pkg/agentic/prep.go#L468-L469go/pkg/agentic/prep.go#L474-L475go/pkg/agentic/prep.go#L491-L491go/pkg/agentic/prep.go#L493-L494go/pkg/agentic/prep.go#L833-L836go/pkg/agentic/prep.go#L913-L914go/pkg/runner/runner.go#L340-L341go/pkg/runner/runner.go#L415-L416go/pkg/runner/runner.go#L433-L433go/pkg/runner/runner.go#L478-L479go/pkg/runner/runner.go#L500-L501go/pkg/agentic/queue.go#L479-L480go/pkg/agentic/resume.go#L101-L102go/pkg/agentic/status.go#L218-L219go/pkg/agentic/pr.go#L151-L152
🤖 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 `@go/pkg/agentic/dispatch.go` around lines 935 - 936, Replace or remove the
descriptive comments at go/pkg/agentic/dispatch.go:935-936,
go/pkg/agentic/dispatch_sync.go:110-111, go/pkg/agentic/prep.go:443-444,
468-469, 474-475, 491, 493-494, 833-836, 913-914,
go/pkg/runner/runner.go:340-341, 415-416, 433, 478-479, 500-501,
go/pkg/agentic/queue.go:479-480, go/pkg/agentic/resume.go:101-102,
go/pkg/agentic/status.go:218-219, and go/pkg/agentic/pr.go:151-152. Any retained
comment must be a concrete usage example with realistic values; otherwise remove
it, without changing the surrounding status, polling, registry, Git, deletion,
or best-effort logic.
Source: Coding guidelines
| if r := writeStatusResult(workspaceDir, workspaceStatus); !r.OK { | ||
| core.Warn("agentic: failed to write workspace status", "reason", r.Value) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Handle status persistence failures before completing state-changing work.
All listed callers treat a failed writeStatusResult as a warning while continuing with the new state or success result. This breaks the durable status contract between status.json, monitors, registries, and retries.
- go/pkg/agentic/dispatch.go#L937-L939: Do not report successful dispatch or track the running workspace until the initial status is durable.
- go/pkg/agentic/dispatch_sync.go#L112-L114: Do not poll stale status after the initial write fails.
- go/pkg/agentic/prep.go#L470-L478: Do not store a failed registry state while status.json still contains the previous state.
- go/pkg/agentic/queue.go#L481-L483: Do not return from
drainOneas successful after a failed write; prevent duplicate dispatch. - go/pkg/agentic/resume.go#L103-L105: Do not return
core.Okwhile the status file still permits another resume. - go/pkg/agentic/status.go#L220-L222: Do not report a derived status as durable when persistence failed.
- go/pkg/agentic/pr.go#L153-L155: Do not report PR success or complete cleanup until
PRURLis durably recorded.
📍 Affects 7 files
go/pkg/agentic/dispatch.go#L937-L939(this comment)go/pkg/agentic/dispatch_sync.go#L112-L114go/pkg/agentic/prep.go#L470-L478go/pkg/agentic/queue.go#L481-L483go/pkg/agentic/resume.go#L103-L105go/pkg/agentic/status.go#L220-L222go/pkg/agentic/pr.go#L153-L155
🤖 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 `@go/pkg/agentic/dispatch.go` around lines 937 - 939, Handle writeStatusResult
failures as blocking errors rather than warnings at
go/pkg/agentic/dispatch.go:937-939, preventing successful dispatch and
running-workspace tracking; go/pkg/agentic/dispatch_sync.go:112-114, preventing
stale-status polling; go/pkg/agentic/prep.go:470-478, preventing failed
registry-state storage; go/pkg/agentic/queue.go:481-483, preventing successful
drainOne completion and duplicate dispatch; go/pkg/agentic/resume.go:103-105,
preventing core.Ok while resume remains possible;
go/pkg/agentic/status.go:220-222, preventing derived-status success reporting;
and go/pkg/agentic/pr.go:153-155, preventing PR success and cleanup until PRURL
is durable. Propagate or return the persistence failure through each caller’s
existing error path.
| _ = process.RunIn(ctx, repoDir, "git", "checkout", currentBranch) | ||
| if process.RunIn(ctx, repoDir, "git", "ls-remote", "--exit-code", "--heads", "origin", currentBranch).OK { | ||
| process.RunIn(ctx, repoDir, "git", "pull", "--ff-only", "origin", currentBranch) | ||
| _ = process.RunIn(ctx, repoDir, "git", "pull", "--ff-only", "origin", currentBranch) | ||
| } else if defaultBranch != "" { | ||
| process.RunIn(ctx, repoDir, "git", "fetch", "origin", defaultBranch) | ||
| _ = process.RunIn(ctx, repoDir, "git", "fetch", "origin", defaultBranch) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Guard the pull with the checkout result.
If git checkout fails, repoDir can remain on a different branch. A successful ls-remote then permits git pull ... currentBranch on that different checkout. This can update the wrong branch. Skip the pull after a failed checkout, or return a controlled preparation error.
🤖 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 `@go/pkg/agentic/prep.go` around lines 837 - 841, Guard the remote-branch
update logic in the preparation flow by checking the result of the git checkout
invoked before the ls-remote check. Only run git pull for currentBranch when
checkout succeeds; otherwise skip the pull or return a controlled preparation
error, ensuring a failed checkout cannot update another branch.
| if r := fs.Write(findingsFile, output); !r.OK { | ||
| core.Warn("agentic: failed to write review findings", "path", findingsFile, "reason", r.Value) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Stop dispatching the fix task when the findings file was not written.
The task at Line 302 tells the worker to read .core/coderabbit-findings.txt. When fs.Write fails, this code only warns and still dispatches. The worker can read an old file or no file, then apply incorrect or no fixes. Return the write failure before dispatchFixFromQueue, or pass the in-memory output instead of relying on the file.
Return before dispatching
- if r := fs.Write(findingsFile, output); !r.OK {
- core.Warn("agentic: failed to write review findings", "path", findingsFile, "reason", r.Value)
+ if writeResult := fs.Write(findingsFile, output); !writeResult.OK {
+ core.Warn("agentic: failed to write review findings", "path", findingsFile, "reason", writeResult.Value)
+ result.Action = "findings_write_failed"
+ result.Detail = "review findings could not be persisted"
+ return result
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if r := fs.Write(findingsFile, output); !r.OK { | |
| core.Warn("agentic: failed to write review findings", "path", findingsFile, "reason", r.Value) | |
| } | |
| if writeResult := fs.Write(findingsFile, output); !writeResult.OK { | |
| core.Warn("agentic: failed to write review findings", "path", findingsFile, "reason", writeResult.Value) | |
| result.Action = "findings_write_failed" | |
| result.Detail = "review findings could not be persisted" | |
| return result | |
| } |
🤖 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 `@go/pkg/agentic/review_queue.go` around lines 297 - 299, In the review
findings write path, update the handling around fs.Write so a failed write
returns before dispatchFixFromQueue is invoked. Preserve the existing warning,
but ensure dispatchFixFromQueue only runs after the findings file has been
successfully written.
| if r := HTTPPost(ctx, url, payload, s.forgeToken, "token"); !r.OK { | ||
| core.Warn("agentic: failed to apply PR label", "url", url, "reason", r.Value) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the label-based review hand-off fail-safe.
Both failure paths leave flagForReview without a valid, confirmed label. A warning does not preserve the queue contract: a failed POST leaves the PR unlabelled, while a parse failure produces label ID 0.
go/pkg/agentic/verify.go#L155-L157: return a failure or schedule a retry after label application fails.go/pkg/agentic/verify.go#L193-L197: return the parse failure and prevent label application until a valid label ID exists.
📍 Affects 1 file
go/pkg/agentic/verify.go#L155-L157(this comment)go/pkg/agentic/verify.go#L193-L197
🤖 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 `@go/pkg/agentic/verify.go` around lines 155 - 157, Make the label-based review
hand-off fail-safe in go/pkg/agentic/verify.go at lines 155-157 and 193-197: in
the label application flow around HTTPPost, return a failure or schedule a retry
when the POST is unsuccessful instead of only warning; in the label parsing
flow, return the parse failure and prevent HTTPPost until a valid nonzero label
ID is available, ensuring flagForReview never proceeds without a confirmed
label.
| // Rewrite, not Director: Director is deprecated as of Go 1.26 and | ||
| // superseded since 1.20. Rewrite is also the safer of the two — it | ||
| // hands the hook both the inbound and outbound request, so headers | ||
| // arriving from the client cannot be forwarded upstream by accident, | ||
| // which is the hazard Director was replaced for. | ||
| // | ||
| // SetURL reproduces what NewSingleHostReverseProxy's own Director did | ||
| // (scheme, host, path join), and SetXForwarded restores the | ||
| // X-Forwarded-* handling that came with it. | ||
| rp.Rewrite = func(pr *httputil.ProxyRequest) { | ||
| pr.SetURL(u) | ||
| pr.Out.Host = pr.In.Host | ||
| pr.SetXForwarded() | ||
| pr.Out.Header.Set("Authorization", authHeader) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)proxy\.go$|go/go\.mod$|go/go\.sum$' || true
echo "== target outline =="
ast-grep outline go/pkg/opencode/proxy.go || true
echo "== relevant target lines =="
sed -n '1,140p' go/pkg/opencode/proxy.go
echo "== go version / module hints =="
if [ -f go/go.mod ]; then sed -n '1,20p' go/go.mod; fi
echo "== occurrences of NewSingleHostReverseProxy and Rewrite/Director in repo =="
rg -n "NewSingleHostReverseProxy|Rewrite|Director" go/pkg/opencode/proxy.goRepository: dAppCore/agent
Length of output: 7039
🌐 Web query:
Go net/http/httputil ReverseProxy Director Rewrite requires one of Director Rewrite before ServeHTTP
💡 Result:
In Go's net/http/httputil package, you should use either the Director or the Rewrite field to configure a ReverseProxy, but never both [1][2]. The Rewrite field is the modern, preferred approach [3][4]. The Director field is deprecated [3][5]. Key details regarding these fields: - Mutually Exclusive: At most one of Rewrite or Director may be set [1][5]. If you set both, the ReverseProxy behavior is undefined or will result in an error depending on the implementation version. - Preferred Method: Rewrite should be used for all new code [4]. It was introduced to address fundamental security issues present in the older Director API, such as potential IP spoofing and the insecure handling of hop-by-hop headers [3][2]. - Operation: Both functions are executed before the proxy calls ServeHTTP to forward the request [3][6]. They modify the request object before it is sent to the target server [3][1]. - Migration: If you have existing code using Director, you should migrate to Rewrite. Rewrite provides a ProxyRequest object that grants safer, more explicit control over both inbound and outbound requests, including methods like SetURL and SetXForwarded to handle headers securely [6][2][4]. There is no requirement that you must have one or the other before ServeHTTP is called in a general sense; rather, ServeHTTP is the method that invokes whichever of these hooks you have configured [3][7]. If neither is set, the proxy will not perform any custom request modification.
Citations:
- 1: https://github.com/golang/go/blob/go1.20.12/src/net/http/httputil/reverseproxy.go
- 2: net/http/httputil: replace Director with Rewrite golang/go#53002
- 3: https://pkg.go.dev/net/http/httputil
- 4: https://imti.co/golang-reverse-proxy/
- 5: https://go.dev/src/net/http/httputil/reverseproxy.go
- 6: https://tip.golang.org/src/net/http/httputil/reverseproxy.go
- 7: https://github.com/golang/go/blob/2403e594/src/net/http/httputil/reverseproxy.go
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== go.mod module cache availability =="
go env GOPATH GOMODCACHE
echo "== Go source check for ReverseProxy ServeHTTP Director/Rewrite handling =="
GOROOT="$(go env GOROOT)"
if [ -f "$GOROOT/src/net/http/httputil/reverseproxy.go" ]; then
rg -n "func \\(rp \\*ReverseProxy\\) ServeHTTP|Director != nil|Rewrite != nil|Director \\(rp.director|Rewrite \\(rp" "$GOROOT/src/net/http/httputil/reverseproxy.go" -A 5 -B 5
sed -n '1,220p' "$GOROOT/src/net/http/httputil/reverseproxy.go"
else
echo "GOROOT source not present"
fi
echo "== Go version =="
go versionRepository: dAppCore/agent
Length of output: 9194
Clear Director before setting Rewrite.
NewSingleHostReverseProxy(u) assigns Director; assigning Rewrite again leaves both non-nil, and ReverseProxy.ServeHTTP returns a 502 Bad Gateway with “ReverseProxy must have exactly one of Director or Rewrite set” before sending authenticated requests to the sandbox. Clear rp.Director before rp.Rewrite, or create the authenticated proxy with Rewrite directly.
🤖 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 `@go/pkg/opencode/proxy.go` around lines 83 - 96, The authenticated proxy setup
around NewSingleHostReverseProxy must ensure exactly one hook is configured.
Clear rp.Director before assigning the Rewrite callback, while preserving the
existing SetURL, Host, SetXForwarded, and Authorization behavior.
Two sites from the SA1012 sweep, one of which hung CI for ten minutes.
review_queue_cov_test.go — TestReviewqueue_RunPRManageLoop_Bad_GuardsInvalidArgs
states its contract in its own comment: "Nil context and non-positive
interval both return immediately." runPRManageLoop guards `ctx == nil`,
so passing context.TODO() walked past the guard into
ticker := time.NewTicker(interval) // interval = time.Hour
and parked in the select waiting an hour for a tick, on a 10m test
budget. That is the `panic: test timed out` on PR #27; the goroutine dump
names review_queue.go:133 with interval 0x34630b8a000 = 1h.
repo_sync_cov_test.go — TestRepoSyncCov_RepoSyncContext_Good_NilFallsBack-
ToBackground did NOT fail, which is worse. repoSyncContext's nil branch
is the only reason the function exists; with a real Context the first
assertion merely repeated the passthrough case asserted on the next line,
so the function's whole purpose went untested and the suite stayed green.
Both found by auditing every substitution against its enclosing test name
instead of trusting a green run — the loud one was already known, and
only that method surfaced the silent one. A test named for its input is
the specific case; the linter's advice is the general one, and here they
disagree. Each site now carries the reason inline.
Co-Authored-By: Virgil <virgil@lethean.io>
1 participant
Uncapped: 109 → 0. Every category adjudicated rather than silenced — one
//nolintin the whole change, and it carries a deadline.unused— 28 → 0All dead scaffolding in test files. Twenty-two were forwarding shims in
compat_adapters_test.go(methods onPrepSubsystemcalling the package function of the same name — 56 are used, these weren't). The rest: apipelineForgeMetaReadernothing constructed, three fetch-loop helpers, andboolPtr/assertNotSame/assertSPDXHeader. Gone, not silenced.staticcheck— 22 → 0Eight died with the code above. The rest:
nilpassed as aContext→context.TODO(). A nil Context is a panic waiting for any callee that reads it.x == nil || len(x) == 0—len(nil)is already 0.httputilDirector→Rewrite.Director→Rewriteis not a rename:Rewritereceives both inbound and outbound requests, so client headers cannot be forwarded upstream by accident — which is whatDirectorwas replaced for.SetURL+SetXForwardedrestore whatNewSingleHostReverseProxy's own Director did.The one deferral, with a date
session.Log. MCP protocol 2026-07-28 (SEP-2577) retires the logging feature outright — there is no successor call, so it cannot be a mechanical swap. Window is at least twelve months, so it must move before ~2027-07-28. The comment states that, plus the two candidates: a server notification, or folding the lines into thestatus://resource that file already publishes. That's the single//nolint, with reason and date.errcheck— 59 → 0, and the split is the pointSilence is correct for a read cursor, a response body, a notification whose listener may be gone, a
git rebase --abortwhere there may be no rebase, and a cleanup of a directory that may already be deleted. Every one now says so rather than being bare.Reporting is correct where silence loses work — these get
core.Warn, the idiom already in these files:writeStatusResult×9 — the status file is what the monitor polls; silent failure leaves a workspace looking stuck foreverperformAsyncIfRegistered×6 — a follow-up that fails to dispatch skips a pipeline stage, and nothing downstream noticesworkspaces.Set×7 — only fails on a locked or sealed registry, so failure means a workspace stops being trackedAction.Run×4 —runner.start/stop/kill; a lifecycle transition failing silently leaves the runner in an unknown statefs.Write×2,CommitToJournal,HTTPPost×3,JSONUnmarshalString, mirror push,gh pr ready,planCleanup— each loses the artefact or step it exists to produceTwo scripting mistakes worth naming
One patch wrapped a doc comment as if it were a call; another overwrote an
if !getResult.OK {guard. Both caught by the compiler within the same minute, and the guard was restored from git rather than reconstructed from memory. Patching by line number needs descending order — ascending shifts every site after the first insertion.Verification
go build,go vet,go test ./...andgofmtall clean. Lint run uncapped.🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io
Summary by CodeRabbit