feat(session): versioned run manifest + partial-coverage contract#379
feat(session): versioned run manifest + partial-coverage contract#379chethanuk 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.
|
🔍 OpenCodeReview found 8 issue(s) in this PR.
📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| switch set { | ||
| case "completed": | ||
| sh.completed[path] = struct{}{} | ||
| case "reused": | ||
| sh.reused[path] = struct{}{} | ||
| case "waived": | ||
| sh.waived[path] = struct{}{} | ||
| } |
There was a problem hiding this comment.
The trackCoverage switch handles "completed", "reused", and "waived" but silently ignores any other value, including "failed". Meanwhile, RecordReviewItemFailed bypasses trackCoverage entirely and manually manages sh.failed + sh.failures under lock. This asymmetry is fragile: if a future refactor attempts to consolidate by calling trackCoverage("failed", path), failures would be silently dropped because there is no case "failed" branch.
Either add a case "failed" branch here (and have RecordReviewItemFailed use trackCoverage plus a separate locked append for sh.failures), or add a default case that panics/logs to make unsupported set names fail loudly rather than silently.
Suggestion:
| switch set { | |
| case "completed": | |
| sh.completed[path] = struct{}{} | |
| case "reused": | |
| sh.reused[path] = struct{}{} | |
| case "waived": | |
| sh.waived[path] = struct{}{} | |
| } | |
| switch set { | |
| case "completed": | |
| sh.completed[path] = struct{}{} | |
| case "reused": | |
| sh.reused[path] = struct{}{} | |
| case "failed": | |
| sh.failed[path] = struct{}{} | |
| case "waived": | |
| sh.waived[path] = struct{}{} | |
| default: | |
| panic(fmt.Sprintf("trackCoverage: unknown set %q", set)) | |
| } |
| func revParse(repoDir, ref string) string { | ||
| if ref == "" { | ||
| return "" | ||
| } |
There was a problem hiding this comment.
When repoDir is empty (e.g., scan mode on a non-git directory) but ref is non-empty, this will call runGitCmdStdout("", "rev-parse", ...), which executes git -C "" rev-parse .... While currently the scan path passes an empty resolvedRange{} directly (avoiding this), the revParse function itself lacks a guard on repoDir. If future callers pass a non-empty ref with an empty repoDir, git will likely fail with a confusing error rather than returning empty gracefully as documented.
Consider adding a repoDir guard for defensive consistency with repoIdentity:
Suggestion:
| func revParse(repoDir, ref string) string { | |
| if ref == "" { | |
| return "" | |
| } | |
| func revParse(repoDir, ref string) string { | |
| if ref == "" || repoDir == "" { | |
| return "" | |
| } |
| if ctx.Err() != nil || errors.Is(err, context.Canceled) { | ||
| a.session.MarkCancelled() | ||
| } |
There was a problem hiding this comment.
Bug: ctx.Err() != nil is true for both context.Canceled and context.DeadlineExceeded. This means a deadline-exceeded run (timeout) will also call MarkCancelled(), causing ComputeTerminalState to report the run as "partial" or "failed" due to cancellation rather than reflecting the actual timeout semantics. Individual items that timed out are already correctly classified as FailureTimeout via ClassifyFailure, but the overall run state will misleadingly indicate cancellation.
Consider checking specifically for cancellation:
if errors.Is(ctx.Err(), context.Canceled) || errors.Is(err, context.Canceled) {Or if you intentionally want deadline-exceeded runs to also be treated as partial/failed at the manifest level, consider renaming MarkCancelled to something more general like MarkIncomplete and updating the documentation to clarify this behavior.
| func ClassifyFailure(err error) string { | ||
| switch { | ||
| case err == nil: | ||
| return FailureProviderError |
There was a problem hiding this comment.
When err == nil, this function returns FailureProviderError, which is counterintuitive and potentially dangerous. A nil error conventionally means success, not a provider error. While current callers guard against passing nil (both agent.go and scan/agent.go only call ClassifyFailure(err) inside if err != nil blocks), this is an exported function and any future caller that accidentally passes nil will silently get a wrong failure class recorded in the manifest, corrupting failure statistics and terminal state computation.
Consider returning an empty string or panicking for nil to make misuse immediately visible, or at minimum document this behavior prominently.
Suggestion:
| func ClassifyFailure(err error) string { | |
| switch { | |
| case err == nil: | |
| return FailureProviderError | |
| func ClassifyFailure(err error) string { | |
| switch { | |
| case err == nil: | |
| // nil error should never reach here; callers must guard. | |
| // Return empty to avoid misclassifying success as failure. | |
| return "" |
| return uuid | ||
| } | ||
|
|
||
| // WriteSessionEnd writes the final session_end summary record and closes the file. |
There was a problem hiding this comment.
The doc comment says "and closes the file" but this function no longer closes the file (that responsibility was moved to flushAndClose called from Finalize). This stale comment will mislead future maintainers who rely on the godoc.
Suggestion:
| // WriteSessionEnd writes the final session_end summary record and closes the file. | |
| // WriteSessionEnd writes the session_end summary record. The file is NOT closed | |
| // here; Finalize writes the run_manifest as the last record and then closes via | |
| // flushAndClose. |
| data, err := json.Marshal(m) | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "[ocr session] failed to marshal run manifest: %v\n", err) | ||
| return "" | ||
| } | ||
| rec := make(map[string]any) | ||
| if err := json.Unmarshal(data, &rec); err != nil { | ||
| fmt.Fprintf(os.Stderr, "[ocr session] failed to encode run manifest: %v\n", err) | ||
| return "" | ||
| } |
There was a problem hiding this comment.
The marshal→unmarshal round-trip through map[string]any is unnecessary overhead and introduces a subtle type fidelity risk: integer fields (SchemaVersion, Concurrency, DurationMS) become float64 in the map, which could produce unexpected JSON output for edge-case values (e.g., very large int64 losing precision). A cleaner approach is to build the envelope map directly and embed the manifest as a nested struct field, or use a wrapper struct with both envelope and manifest fields.
| return jw.writeReviewItemRecord("review_item_waived", filePath, oldPath, newPath, fingerprint, "", "", "", nil) | ||
| } | ||
|
|
||
| func (jw *jsonlWriter) writeReviewItemRecord(recordType, filePath, oldPath, newPath, fingerprint, sourceSessionID, failureClass, errorMsg string, comments []model.LlmComment) string { |
There was a problem hiding this comment.
The writeReviewItemRecord function now takes 9 string/slice parameters, several of which are the same type (string). This high arity makes it easy to accidentally swap arguments (e.g., failureClass and errorMsg). Consider introducing a small options struct or record-builder pattern to reduce the risk of parameter misordering at call sites.
… 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.
95b009e to
dbe57fa
Compare
1 participant
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 #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.