feat(session): versioned run manifest + partial-coverage contract#7
Open
chethanuk wants to merge 7 commits into
Open
feat(session): versioned run manifest + partial-coverage contract#7chethanuk wants to merge 7 commits into
chethanuk wants to merge 7 commits into
Conversation
…age tracking Add an append-only run_manifest JSONL record written once by Finalize as the last line of the session file and retained in memory (Manifest() getter), so persisted and reported coverage are identical by construction. - manifest.go: RunManifest struct, pure ComputeTerminalState (table over skipped/complete/failed/partial incl. cancelled + waive), failure-class constants + ClassifyFailure. - history.go: RunMeta on SessionOptions; coverage sets tracked on SessionHistory (selected/completed/reused/failed/waived + typed failures); SetSelected/SetArtifactChecksum/MarkCancelled/RecordReviewItemWaived; Finalize computes state and writes the manifest as the final record. - persist.go: WriteRunManifest + WriteReviewItemWaived; failureClass added to review_item_failed; session_end no longer closes so the manifest stays last. - list.go: Summary.State populated from the run_manifest record. RecordReviewItemFailed gains a class param; existing agent callers pass panic/skipped_limit/ClassifyFailure(err).
Wire the run-manifest coverage denominator into the diff-review path: - recordSelectionAndArtifact records the non-deleted reviewable set (SetSelected) and an order-independent sha256 over the sorted per-file fingerprints (SetArtifactChecksum) before resume/dispatch, so selected == completed + reused + failed + waived holds. - Run marks the session cancelled when the context is cancelled. Failure classes (panic/skipped_limit/ClassifyFailure) were threaded through the dispatch paths in the prior commit. Adds a stub-LLM table test over success/mixed/timeout/cancel/panic/skip asserting manifest state + classes, plus an artifact-checksum order-independence test.
…ntity, resolved range)
runmeta.go builds session.RunMeta for review and scan:
- config_hash: sha256 over an allowlisted, non-secret field set
{provider_protocol, model, base_url_host (userinfo+query stripped),
language, timeout}. Token/AuthHeader/ExtraHeaders are never in the input, so
the hash is stable across key rotation.
- rules_hash: sha256 over (label \0 raw bytes) of each loaded rule layer
(custom/project/global) plus a label+version marker for the embedded system
layer.
- repo identity: origin remote URL with userinfo stripped + HEAD SHA.
- resolved range SHAs rev-parsed from --from/--to/--commit.
Threaded via agent.Args.RunMeta / scan.Args.RunMeta into session.New.
llmRuntime now retains the resolved endpoint for non-secret hash inputs.
Tests cover config-hash redaction/invariance, userinfo stripping, resolved
range SHAs, and rules-hash sensitivity.
Add jsonOutput.Manifest surfaced verbatim from the run's in-memory manifest (same value persisted as the last JSONL record). Manifest() joins the ResultProvider interface; both agent.Agent and scan.Agent implement it. Legacy status values (success/completed_with_errors/completed_with_warnings/ skipped) are UNCHANGED — manifest.state is the new machine contract. Adds a regression-lock test over the status paths plus manifest-presence tests (including the no-files skipped path).
Record coverage on the scan path so full scans emit the same manifest as diff reviews: - SetSelected + artifact checksum (order-independent sha256 over sorted path-based fingerprints) at dispatch start. - executeSubtask records done / failed(ClassifyFailure) / skipped_limit per item so the coverage invariant holds. - Run marks the session cancelled on context cancellation. Scan has no diff/resume, so the fingerprint is path-based (ponytail note: content-hash if scan grows resume). Adds a parity table test over success/mixed/all-failed/cancel/skip.
Add --waive path/a.go,path/b.go (requires --resume): waived diffs are not dispatched, are recorded as review_item_waived, and count toward coverage so a waive can flip partial -> complete. A later resume treats a waived item as covered (indexed like a completed item, empty comments). No fingerprint-addressed or config-file waives — paths only (ponytail note). Tests: waive flips partial->complete, without waive stays partial, --waive without --resume errors.
codeant-ai
Bot
added
the
size:XXL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
chethanuk
force-pushed
the
feat/issue-367-run-manifest
branch
2 times, most recently
from
July 15, 2026 09:49
51283dd to
b4ba228
Compare
codeant-ai
Bot
added
size:XXL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| --to string target ref to end diff at (e.g., 'feature-branch') | ||
| --tools string path to JSON tools config file (default: embedded)`) | ||
| --tools string path to JSON tools config file (default: embedded) | ||
| --waive string comma-separated paths to waive (skip but count as covered); requires --resume`) |
There was a problem hiding this comment.
Suggestion: The help text for --waive has an extra trailing backtick, so CLI usage output shows a malformed flag description. Remove the stray character to avoid confusing users and breaking copy/paste from help output. [typo]
Severity Level: Minor 🧹
⚠️ Suggestion addresses non-issue; raw string usage intentional.
⚠️ CLI help already displays correct waive flag description.Steps of Reproduction ✅
1. Run the CLI with review help, e.g. `ocr review --help`, which executes `runReview()` in
`cmd/opencodereview/review_cmd.go:21`. This calls `parseReviewFlags()` in
`cmd/opencodereview/flags.go:120`. When `-h`/`--help` is present, `ocrFlagSet.Parse()`
(flags.go:65-76) sets `a.showHelp`, causing `parseReviewFlags()` to return with
`opts.showHelp = true` (flags.go:149-151).
2. `runReview()` sees `opts.showHelp` and calls `printReviewUsage()` in
`cmd/opencodereview/flags.go:204`, which prints a raw multiline string via
`fmt.Println(`...`)` (flags.go:205-259).
3. The help text is defined as a Go raw string literal delimited by backticks: the opening
backtick is after `fmt.Println(` on line 205, and the closing backtick appears at the end
of the `--waive` line on line 259, followed by the closing parenthesis for `fmt.Println`.
At runtime, only the string content is printed; neither the backtick nor the closing `)`
is part of the output.
4. Observing the actual CLI output shows the `--waive` help line rendered correctly as
plain text without any stray backticks or parentheses. The pattern `fmt.Println(`raw
string` )` matches the established usage in this file (e.g., `printConfigUsage()` at
flags.go:299-352), so the suggestion to remove the trailing backtick is addressing a
non-issue and would in fact break the raw string literal.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** cmd/opencodereview/flags.go
**Line:** 259:259
**Comment:**
*Typo: The help text for `--waive` has an extra trailing backtick, so CLI usage output shows a malformed flag description. Remove the stray character to avoid confusing users and breaking copy/paste from help output.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+89
to
+96
| add := func(label, path string) { | ||
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| return | ||
| } | ||
| h.Write([]byte(label)) | ||
| h.Write([]byte{0}) | ||
| h.Write(data) |
There was a problem hiding this comment.
Suggestion: rules_hash currently hashes only the top-level rule JSON files, but rule resolution can load additional referenced rule files (for example markdown files referenced from rule.json). If one of those referenced files changes, the effective rules change while the hash stays the same, producing incorrect manifest identity. Include the fully resolved rule content (or all referenced files) in the hash input so the hash tracks actual runtime rules. [incomplete implementation]
Severity Level: Major ⚠️
⚠️ Run manifest cannot detect changes in external rule files.
⚠️ Rules_hash equality may misrepresent actual review rule versioning.Steps of Reproduction ✅
1. Configure a project rule file `.opencodereview/rule.json` in a repo. Its schema is
`ProjectRule` in `internal/config/rules/system_rules.go:186-191`, with entries of type
`ProjectRuleEntry` (system_rules.go:179-184) where the `Rule` field can be a file path
like `rules/auth.md`. `loadProjectRule()` at system_rules.go:93-107 reads this JSON and
calls `resolveRuleEntries(pr.Rules, repoDir)` (system_rules.go:106), which in turn
resolves any `Rule` values that look like file paths into actual file contents via
`tryReadRuleFile()` and `readRuleFileSafe()` (system_rules.go:213-240, 11-39).
2. Run `ocr review` in that repo so `runReview()` in `cmd/opencodereview/review_cmd.go:21`
executes. It calls `loadCommonContext()` (shared.go, not shown here), which internally
uses `rules.NewResolver(repoDir, opts.rulePath)` (system_rules.go:252-29) to load the
system, custom, project, and global rule layers. For the project layer,
`resolveRuleEntries()` ensures the in-memory rules actually used at runtime include the
contents of referenced files (e.g., `rules/auth.md`) rather than just the JSON strings.
3. In the same `runReview()` call, `buildRunMeta()` in
`cmd/opencodereview/runmeta.go:32-45` is invoked at `review_cmd.go:105-107` with
`repoDir`, `opts.rulePath`, and the resolved range. `buildRunMeta()` computes `RulesHash`
by calling `computeRulesHash(customRulePath, repoDir, ocrVersion)` (runmeta.go:39,
87-108). Inside `computeRulesHash`, the helper `add()` at runmeta.go:89-97 reads only the
top-level JSON files: the custom rule file (if any), the project
`.opencodereview/rule.json` (runmeta.go:101-103), and the global
`~/.opencodereview/rule.json` (runmeta.go:104-106), then hashes their raw bytes along with
labels and the system version (runmeta.go:107-108). No referenced rule files such as
`rules/auth.md` are included in the hash input.
4. Modify a referenced rule file (e.g., `rules/auth.md`) without changing
`.opencodereview/rule.json`. A subsequent `ocr review` run will use the new rule content
because `resolveRuleEntries()` reloads `rules/auth.md` (system_rules.go:213-221). However,
`computeRulesHash()` still hashes the unchanged JSON files only, so the resulting
`RulesHash` in `session.RunManifest` (internal/session/manifest.go:107-127, field
`RulesHash` at line 119) remains identical between runs even though the effective rule
text applied at runtime has changed. This produces a stale or misleading manifest identity
for rules, as downstream consumers cannot distinguish runs with different rule file
contents when only the transitive rule files change.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** cmd/opencodereview/runmeta.go
**Line:** 89:96
**Comment:**
*Incomplete Implementation: `rules_hash` currently hashes only the top-level rule JSON files, but rule resolution can load additional referenced rule files (for example markdown files referenced from `rule.json`). If one of those referenced files changes, the effective rules change while the hash stays the same, producing incorrect manifest identity. Include the fully resolved rule content (or all referenced files) in the hash input so the hash tracks actual runtime rules.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+387
to
+391
| // Establish the coverage denominator and input-identity checksum before | ||
| // resume/dispatch so every dispatched outcome lands in a bucket selected | ||
| // already covers (selected is a superset of completed ∪ reused ∪ failed | ||
| // ∪ waived; equality only for fully-covered runs). | ||
| a.recordSelectionAndArtifact() |
There was a problem hiding this comment.
Suggestion: recordSelectionAndArtifact() is called only after the early return for len(a.diffs) == 0, so when all reviewable diffs are pre-filtered by token size the session finalizes without any selected coverage set and no failed item records, producing a misleading terminal state (skipped) instead of a partial/failed run. Record selection before this return (and/or record per-file skipped-limit failures) so manifest state reflects that work was selected but not completed. [logic error]
Severity Level: Critical 🚨
- ❌ JSON run manifest mislabels token-filtered reviews as skipped.
- ⚠️ Downstream tools see zero selected files despite diffs.
- ⚠️ Session coverage omits failures for oversized token-filtered diffs.Steps of Reproduction ✅
1. Run the diff-review CLI (`ocr review`) so `runReview` in
`cmd/opencodereview/review_cmd.go` constructs an `agent.Agent` via
`agent.New(agent.Args{...})` (around lines 108–142) and then calls `comments, err :=
ag.Run(ctx)` at approximately `cmd/opencodereview/review_cmd.go:153` (verified via
BulkRead).
2. In `internal/agent/agent.go:203-252`, `Agent.Run` loads diffs (`loadDiffs`), counts
reviewable files, applies path/extension filters via `filterDiffs`, and sets `a.diffs` to
the reviewable set; for a commit where all reviewable diffs are extremely large, each
`model.Diff.Diff` string exceeds the token limit used in `filterLargeDiffs`.
3. Inside `dispatchSubtasks` at `internal/agent/agent.go:376-386`, `a.diffs =
a.filterLargeDiffs(a.diffs)` drops every diff because `llm.CountTokens(d.Diff) > limit`
(see `filterLargeDiffs` at `internal/agent/agent.go:848-870`), so `len(a.diffs) == 0` and
the function returns `nil, fmt.Errorf("all diffs filtered out by token size")` at line
385; the subsequent `a.recordSelectionAndArtifact()` call at line 391 is never executed,
so `SessionHistory.SetSelected` and `SetArtifactChecksum` are not called and no per-file
failures are recorded.
4. Back in `Agent.Run` (`internal/agent/agent.go:242-251`), the error from
`dispatchSubtasks` is returned; `a.session.MarkCancelled()` is not invoked because
`ctx.Err() == nil` and `errors.Is(err, context.Canceled)` is false. When the caller
eventually finalizes the session (`SessionHistory.Finalize` at
`internal/session/history.go:329-360`), `buildManifestLocked` at
`internal/session/history.go:364-419` computes `selected` as the union of `sh.selected`,
`completed`, `reused`, `failed`, and `waived`, all empty in this scenario, and calls
`ComputeTerminalState(0, 0, 0, 0, 0, false)` at `internal/session/manifest.go:139-151`,
which returns `StateSkipped`. The manifest (exposed via `Agent.Manifest()` at
`internal/agent/agent.go:259-266` and serialized in JSON output through
`jsonOutput.Manifest` in `cmd/opencodereview/output.go:43-56, 152-155, 279-27`) thus
reports a `skipped` run even though reviewable work existed but was entirely filtered out
by token-size limits, misrepresenting coverage as "nothing selected" instead of
partial/failed.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** internal/agent/agent.go
**Line:** 387:391
**Comment:**
*Logic Error: `recordSelectionAndArtifact()` is called only after the early return for `len(a.diffs) == 0`, so when all reviewable diffs are pre-filtered by token size the session finalizes without any `selected` coverage set and no failed item records, producing a misleading terminal state (`skipped`) instead of a partial/failed run. Record selection before this return (and/or record per-file skipped-limit failures) so manifest state reflects that work was selected but not completed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+633
to
+634
| comments := a.args.CommentCollector.CommentsForPath(it.Path) | ||
| a.session.RecordReviewItemDone(it.Path, it.Path, it.Path, fingerprint, comments) |
There was a problem hiding this comment.
Suggestion: RunPerFile can return before async code_comment post-processing finishes, but this code reads CommentsForPath immediately and persists the item as done. That creates a race where session checkpoints miss comments for files whose comment workers are still running. Ensure comment workers for that file are drained before reading comments, or persist completion first and attach comments after the batch-level await. [race condition]
Severity Level: Major ⚠️
- ⚠️ Session JSONL checkpoints may omit some per-file review comments.
- ⚠️ Future resume features could reuse incomplete per-file comments.Steps of Reproduction ✅
1. Run a full-scan via `runScan` in `cmd/opencodereview/scan_cmd.go:13-35`, which
constructs a `scan.Agent` with a non-nil `CommentWorkerPool`
(`llmloop.NewCommentWorkerPool(opts.concurrency)` at line 23) and a shared
`tool.CommentCollector` (`rt.Collector` at line 22), then calls `ag.Run(ctx)` at
`scan_cmd.go:51`.
2. Inside `Agent.Run` (`internal/scan/agent.go:214-282`), execution reaches
`dispatchSubtasks` (`agent.go:396-64`), which groups items into batches and calls
`dispatchBatch` for each batch (`agent.go:22-36` and `35-36` in the snippet starting at
line 396).
3. In `dispatchBatch` (`internal/scan/agent.go:106-186`), each `model.ScanItem` in the
batch is processed in a goroutine that calls `a.executeSubtask(fileCtx, it)`
(`agent.go:145-181`). `executeSubtask` (`agent.go:198-241`) renders messages, computes
`fingerprint := scanFingerprint(it)` (`agent.go:216`), and calls `a.runner.RunPerFile(ctx,
messages, it.Path)` (`agent.go:233`).
4. Within `RunPerFile` (`internal/llmloop/loop.go:31-145` and `loop.go:260-145`),
`executeToolCall` handles `code_comment` tool calls asynchronously when
`CommentWorkerPool` is non-nil: it schedules `pool.Submit` work that calls
`resolveAndCollect(asyncCtx)` and eventually does `r.deps.CommentCollector.Add(*cm)` for
each comment (`loop.go:83-112, 115-132`). `RunPerFile` returns before these async workers
are guaranteed to finish. Immediately after `RunPerFile` returns, `executeSubtask` calls
`comments := a.args.CommentCollector.CommentsForPath(it.Path)` and
`a.session.RecordReviewItemDone(..., fingerprint, comments)` (`agent.go:238-239). The
async workers may still be running, so `CommentsForPath` can return an incomplete set of
comments for `it.Path`. Later, `dispatchSubtasks` drains the worker pool
(`a.args.CommentWorkerPool.Await()` at `agent.go:43-47` in the 396-64 snippet), but there
is no subsequent update of the already-persisted `RecordReviewItemDone` checkpoint. Thus,
the session JSONL checkpoint for that scan item can miss comments that were added after
`CommentsForPath` ran.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** internal/scan/agent.go
**Line:** 633:634
**Comment:**
*Race Condition: `RunPerFile` can return before async `code_comment` post-processing finishes, but this code reads `CommentsForPath` immediately and persists the item as done. That creates a race where session checkpoints miss comments for files whose comment workers are still running. Ensure comment workers for that file are drained before reading comments, or persist completion first and attach comments after the batch-level await.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
📝 WalkthroughWalkthroughThe PR adds immutable manifests for review and scan runs, records coverage and typed failures, supports resume-only path waiving, fingerprints run inputs, exposes manifests in JSON output, and updates session viewers and CLI documentation. ChangesRun manifest contract and persistence
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant ReviewAgent
participant SessionHistory
participant JSONOutput
CLI->>ReviewAgent: pass RunMeta and WaivePaths
ReviewAgent->>SessionHistory: record selection and outcomes
SessionHistory-->>ReviewAgent: finalize RunManifest
ReviewAgent->>JSONOutput: provide Manifest()
JSONOutput-->>CLI: emit manifest and legacy status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
… docs Tests + docs: - acceptance_test.go: end-to-end matrix over the terminal-state scenarios (complete/partial/failed/skipped/cancelled/waive, plus a cancelled mid-run row where selected is a strict superset of the outcome sets); provider-transition resume case starts from a PARTIAL run and asserts the resume flips it to complete with parent_session_id intact and an unchanged artifact checksum (input identity retained); TestManifestNeverLeaksSecrets is the canonical secret guard. - manifest_test.go: TestManifestSchemaLock pins the literal schema version (1) and the top-level JSON key set. The standalone Finalize test was folded into the acceptance matrix (failure-class + artifact columns). - emit_run_result_test.go: no-files manifest case folded into the legacy-status regression table. - agent/manifest_test.go: newManifestAgent helper replaces three copies of the Args literal. - docs: run-manifest schema, state->exit-code mapping, --waive flag; files invariant stated with the superset wording; commit-mode range fields documented. Hardening: - viewer/store.go: peekSession read only the last JSONL line for session_end; run_manifest is now the last line, so the session-list fast path lost duration/files/failures. Checks the last two lines; regression test fails on the old code. - manifest.go: cancelled run whose only covered items are waives is partial, not failed; ManifestFiles doc corrected (selected is a superset); Finalize made idempotent (written-once guard). - session logging to stderr: the [ocr session] messages went to stdout, which carries the --format json payload; all four sites now use os.Stderr. - redactRemoteURL (nee stripURLUserinfo) also strips query/fragment, so ?access_token=... can never reach the manifest; scp-style remotes still pass through (they fail url.Parse and carry no password). Verified E2E. - scanFingerprint hashes mode\0path\0content (ScanItem.Content is already in memory), matching the diff path's content-sensitive fingerprint. - scan dispatch goroutines now recover from panics, record the item as failed with class "panic", and let Finalize still write the manifest, mirroring the diff-review dispatch. Panic row added to the scan parity table. - session.ArtifactChecksum: the sort/join/sha256 tail duplicated in the agent and scan recordSelectionAndArtifact now lives in one place so the fingerprint-join convention cannot diverge. - computeRulesHash hashes the resolver's resolved rule layers (new rules.UserLayers accessor) instead of re-reading the top-level rule.json bytes: rule entries that reference files (e.g. team.md) are expanded to content by rules.NewResolver, so editing a referenced file now changes rules_hash even though rule.json's bytes do not. Test locks referenced-file edit -> hash change. - scan executeSubtask drains the async CommentWorkerPool before reading CommentsForPath and persisting review_item_done; without the per-file Await the checkpoint could persist an incomplete comment set while async code_comment workers were still running (the diff path already had this Await). - filterLargeDiffs/filterLargeScans record each dropped oversized file as failed with class skipped_limit (plus a warning), so the manifest's selected denominator includes pre-filtered files: a partially filtered run is partial, an all-filtered run is failed with per-file failures rather than an empty "skipped". Two agent table rows + a scan filter test lock this. - persist.go/history.go: corrected stale close-ownership comments (WriteSessionEnd no longer closes the file; flushAndClose does). Full go test ./... green (2037); -race green on session/agent/scan/ llmloop/cmd; make check clean. E2E: failing review persists ... -> session_end -> run_manifest with state=failed and class=provider_error; no-diff review emits valid skipped JSON with the manifest; a remote with userinfo+query credentials is fully redacted.
chethanuk
force-pushed
the
feat/issue-367-run-manifest
branch
from
July 15, 2026 10:33
95b009e to
dbe57fa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/agent.go`:
- Around line 856-858: Update filterLargeDiffs to skip deleted files before
counting tokens or applying the size limit, using the diff’s existing deletion
indicator. Preserve current filtering, warning, and skipped_limit failure
behavior for non-deleted files so deleted files neither emit warnings nor record
failures.
- Around line 489-493: Trim surrounding whitespace from each path while
constructing the waived map in the WaivePaths loop, before using it as the map
key, so comma-separated values such as “a.go, b.go” match effective diff paths.
In `@internal/scan/agent.go`:
- Around line 634-648: Remove the CommentWorkerPool.Await() call from
executeSubtask’s per-file completion path before reading CommentsForPath.
Preserve recording the collected comments and completion status, while relying
on a synchronization point that cannot overlap sibling RunPerFile submissions if
draining is required.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c3d9d25-37f7-4507-a923-05b8454cdded
📒 Files selected for processing (28)
cmd/opencodereview/emit_run_result_test.gocmd/opencodereview/flags.gocmd/opencodereview/flags_test.gocmd/opencodereview/output.gocmd/opencodereview/output_helpers_test.gocmd/opencodereview/review_cmd.gocmd/opencodereview/runmeta.gocmd/opencodereview/runmeta_test.gocmd/opencodereview/scan_cmd.gocmd/opencodereview/session_cmd_test.gocmd/opencodereview/shared.gointernal/agent/agent.gointernal/agent/manifest_test.gointernal/config/rules/system_rules.gointernal/scan/agent.gointernal/scan/manifest_test.gointernal/session/acceptance_test.gointernal/session/history.gointernal/session/list.gointernal/session/list_test.gointernal/session/manifest.gointernal/session/manifest_test.gointernal/session/persist.gointernal/session/persist_test.gointernal/session/resume.gointernal/viewer/store.gointernal/viewer/store_test.gopages/src/content/docs/en/cli-reference.md
Comment on lines
+489
to
+493
| waived := make(map[string]struct{}, len(a.args.WaivePaths)) | ||
| for _, p := range a.args.WaivePaths { | ||
| waived[p] = struct{}{} | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Trim spaces from waived paths.
If the user provides a space after the comma in --waive a.go, b.go, the parsed path " b.go" won't match the diff's effective path. Consider trimming spaces to prevent silent waiver misses.
💡 Proposed fix
waived := make(map[string]struct{}, len(a.args.WaivePaths))
for _, p := range a.args.WaivePaths {
- waived[p] = struct{}{}
+ waived[strings.TrimSpace(p)] = struct{}{}
}📝 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.
Suggested change
| waived := make(map[string]struct{}, len(a.args.WaivePaths)) | |
| for _, p := range a.args.WaivePaths { | |
| waived[p] = struct{}{} | |
| } | |
| waived := make(map[string]struct{}, len(a.args.WaivePaths)) | |
| for _, p := range a.args.WaivePaths { | |
| waived[strings.TrimSpace(p)] = struct{}{} | |
| } |
🤖 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/agent.go` around lines 489 - 493, Trim surrounding whitespace
from each path while constructing the waived map in the WaivePaths loop, before
using it as the map key, so comma-separated values such as “a.go, b.go” match
effective diff paths.
Comment on lines
856
to
858
| for _, d := range diffs { | ||
| tokens := llm.CountTokens(d.Diff) | ||
| if tokens > limit { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Ignore deleted files in filterLargeDiffs.
Deleted files are passed through a.diffs, but filterLargeDiffs does not ignore them. If a deleted file has a large diff, it will be dropped, print a warning, and record a skipped_limit failure.
However, deleted files are skipped entirely during dispatch (line 406) and are explicitly excluded from the selected denominator (line 563). Recording them as failed here breaks the coverage invariant (as the failure is not in selected) and emits noisy warnings for files that wouldn't be reviewed anyway.
🐛 Proposed fix
for _, d := range diffs {
+ if d.IsDeleted {
+ kept = append(kept, d)
+ continue
+ }
tokens := llm.CountTokens(d.Diff)📝 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.
Suggested change
| for _, d := range diffs { | |
| tokens := llm.CountTokens(d.Diff) | |
| if tokens > limit { | |
| for _, d := range diffs { | |
| if d.IsDeleted { | |
| kept = append(kept, d) | |
| continue | |
| } | |
| tokens := llm.CountTokens(d.Diff) | |
| if tokens > limit { |
🤖 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/agent.go` around lines 856 - 858, Update filterLargeDiffs to
skip deleted files before counting tokens or applying the size limit, using the
diff’s existing deletion indicator. Preserve current filtering, warning, and
skipped_limit failure behavior for non-deleted files so deleted files neither
emit warnings nor record failures.
Comment on lines
634
to
+648
| _, err := a.runner.RunPerFile(ctx, messages, it.Path) | ||
| return err | ||
| if err != nil { | ||
| a.session.RecordReviewItemFailed(it.Path, it.Path, it.Path, fingerprint, session.ClassifyFailure(err), err.Error()) | ||
| return err | ||
| } | ||
| // The async CommentWorkerPool may still be resolving code_comment calls | ||
| // for this file when RunPerFile returns; drain it before reading the | ||
| // collector so the persisted checkpoint carries the full comment set | ||
| // (mirrors the diff-review path's per-file Await). | ||
| if a.args.CommentWorkerPool != nil { | ||
| a.args.CommentWorkerPool.Await() | ||
| } | ||
| comments := a.args.CommentCollector.CommentsForPath(it.Path) | ||
| a.session.RecordReviewItemDone(it.Path, it.Path, it.Path, fingerprint, comments) | ||
| return nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --pattern 'func ($_ *CommentWorkerPool) Await() {
$$$
}' --lang goRepository: chethanuk/open-code-review
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the pool definition and all references
fd -a '.*\.go$' . | xargs rg -n --context 2 'type\s+CommentWorkerPool|func\s+\(.*CommentWorkerPool.*\)\s+Await|CommentWorkerPool\.Await\(|Await\(\)'
# Show the file(s) that define the type, if any
fd -a '.*\.go$' . | xargs rg -n --context 3 'type\s+CommentWorkerPool'Repository: chethanuk/open-code-review
Length of output: 6901
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- internal/llmloop/pool.go ---'
sed -n '1,220p' internal/llmloop/pool.go
echo
echo '--- internal/scan/agent.go (dispatch/batch area) ---'
sed -n '360,700p' internal/scan/agent.goRepository: chethanuk/open-code-review
Length of output: 16444
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- CommentWorkerPool.Submit call sites ---'
rg -n --context 2 'CommentWorkerPool\.Submit\(|\.Submit\(' internal
echo
echo '--- internal/llmloop/loop.go relevant area ---'
sed -n '1,260p' internal/llmloop/loop.goRepository: chethanuk/open-code-review
Length of output: 11369
Avoid Await() here; it can race with sibling Submit() calls.
CommentWorkerPool.Await() is documented as unsafe to run concurrently with Submit(), and executeSubtask() reaches it while other file goroutines are still inside RunPerFile() and may still be submitting comment work to the shared pool. That can violate the pool contract and trigger a sync.WaitGroup panic.
🤖 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/scan/agent.go` around lines 634 - 648, Remove the
CommentWorkerPool.Await() call from executeSubtask’s per-file completion path
before reading CommentsForPath. Preserve recording the collected comments and
completion status, while relying on a synchronization point that cannot overlap
sibling RunPerFile submissions if draining is required.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Implements a versioned, immutable run manifest and an explicit partial-coverage contract for review/scan runs, per
alibaba/open-code-review#367. Builds on the merged sessions/checkpoints substrate (PR alibaba#306).The manifest is one new append-only
run_manifestJSONL record, written exactly once bySessionHistory.Finalize()as the last line of the existing session file, held in memory, and surfaced verbatim asjsonOutput.Manifestin--format json— the same Go value serialized in both places, so persisted and reported coverage are identical by construction. No sidecar file. Immutability = append-only + written-once; a resumed run creates a new session file whose manifest carriesparent_session_id.What's in it
internal/session/manifest.go): a pureComputeTerminalState(selected, completed, reused, failed, waived, cancelled)→complete/partial/failed/skipped.selectedis the coverage denominator — a superset ofcompleted ∪ reused ∪ failed ∪ waived, equal for fully-covered runs and strictly larger for a cancelled/budget-truncated partial. A waive satisfies coverage (counts like a completed review).provider_error/timeout/cancelled/panic/skipped_limit, viaClassifyFailure(err)(context-error chain) plus the explicit panic/skip paths. Recorded per item asfailureClass; the legacysubtask_errorwarning is untouched.cmd/opencodereview/runmeta.go):config_hashover an allowlisted, non-secret field set (protocol, model, base-URL host with userinfo+query stripped, language, timeout) — stable across API-key rotation, never carries a credential;rules_hashper rule layer; repo identity (origin remote with userinfo/query stripped, HEAD SHA); resolved range SHAs.--waive path,...(requires--resume): waived diffs are skipped, recorded asreview_item_waived, count toward coverage, and stay covered on later resumes.--format jsongains amanifestobject; the legacystatusvalues are unchanged (regression-locked by test). Exit code stays 0 for partial (main.gountouched) —manifest.stateis the machine contract, no--fail-onflag.panicand the manifest still finalizes).--waiveadded topages/src/content/docs/en/cli-reference.md.Commits (7: 6 feature slices + 1 tests/hardening slice)
c8b7f35feat(session): manifest record, terminal-state, coverage tracking9411ee4feat(agent): selected set, artifact checksum, cancellation markinga6ff4a1feat(cmd): run metadata (version, provider, hashes, repo identity, range SHAs)9ba43fffeat(output): expose manifest in--format json7c9c476feat(scan): manifest parity for full scansff745d4feat(review):--waivefor resumed runsdbe57fachore(tests): acceptance matrix + provider-transition + secret guard; docs + hardeningHardening (in
dbe57fa)peekSession(the list fast path) read only the last JSONL line to findsession_end; now thatrun_manifestis the last line, duration/file-count/failures showed as zero for every manifest-era session. Fixed to check the last two lines;TestPeekSession_RunManifestAfterSessionEndfails on the pre-fix code.partial, notfailed(waive satisfies coverage).Finalizeis idempotent (written-once guard) so immutability is enforced, not incidental.[ocr session]messages go toos.Stderr(all 4 sites, incl. 2 pre-existing), so--format jsonstdout stays pure.redactRemoteURLstrips userinfo and query/fragment:?access_token=…can never reach the manifest. Verified E2E. scp-style remotes (git@host:path) pass through — they failurl.Parseand carry no password.scanFingerprintis content-based (mode\0path\0content), matching the diff path's content-sensitive fingerprint.recover, record classpanic, and still finalize the manifest — mirrors the diff-review dispatch.session.ArtifactChecksumde-dupes the checksum tail shared by the agent/scan paths;TestManifestSchemaLockpins the literal schema version + top-level key set; the resume acceptance test proves a partial run flips to complete on resume with an unchanged artifact checksum.rules_hashtracks resolved rules: rule entries that reference files (e.g. arule.jsonentry pointing atteam.md) are expanded to content byrules.NewResolver; the hash now covers the resolved layers (via a smallUserLayersaccessor) instead of the top-levelrule.jsonbytes, so editing a referenced rule file changes the hash. Test locks it.executeSubtasknowAwaits theCommentWorkerPoolbefore readingCommentsForPathand persistingreview_item_done— mirrors the per-fileAwaitthe diff path already had; without it a checkpoint could persist an incomplete comment set.filterLargeDiffs/filterLargeScansrecord each dropped file as failed with classskipped_limit, so a partially filtered run reportspartial(notcompletewith the file silently missing) and an all-filtered run reportsfailedwith per-file failures (not an emptyskipped). Table rows + a scan filter test lock this.Deviations from the design (all minor, adapted to real code)
session_endno longer closes the file;Finalizewritessession_end→run_manifest→ close, so the manifest stays the last line. All existing readers (list.walkSessionFile,resume.applyResumeLine,viewer/store) tolerate a trailing unknown record — verified.RecordReviewItemFailed's signature required updating callers immediately to keep the tree building between commits).rules_hashhashes the resolver's already-loaded rule layers (one smallUserLayersaccessor on the rules package) rather than re-reading files — the hash covers resolved rule content, including files referenced fromrule.json.skipped_limitfile yieldsstate: failed(sincefailed == selected), per the spec table — noted so it isn't surprising.en/cli-reference.mdonly; ja/zh translations left for a follow-up.Type of Change
How Has This Been Tested?
make testpasses locally (full-racesuite, EXIT=0)make checkpasses (tidy + fmt + vet, EXIT=0)… → session_end → run_manifest(manifest is the last line); a failing review producedstate: failed,files: {selected:1, failed:1},failures[].class: provider_error.authorization,bearer,x-api-key,auth_token,:token@,sk-ant→ none present; a remote configured with userinfo+query credentials persists fully redacted. A test-level secret guard also asserts this on a synthesized manifest.Checklist
go fmt,go vet)Related Issues
Addresses
alibaba/open-code-review#367.Summary by CodeRabbit
--waive <paths>for resumed reviews, allowing selected files to be skipped while remaining counted as covered.