From b3a0b62879dbd4e649e4a38cf191a1a818768a93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=89=BA=E4=B8=B4?= Date: Mon, 10 Aug 2026 18:28:51 +0800 Subject: [PATCH 1/3] feat(resume): verify input identity before reusing checkpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ocr review --resume` admitted a session whenever the ref text matched, which is neither sufficient nor necessary evidence about the input: `abc1234` and `abc1234def` name the same commit, while a branch name that did not change can name a new one. Resuming then mixed comments computed from one input with comments computed from another, and nothing in the report distinguished them. Compare the resolved input identity instead. agent.ResolveIdentity replays the run's own selection — the same diff load and the same two filter passes — and returns the identity a real run would record, so the parent manifest and the child candidate are directly comparable. Any mismatch on mode, repository, source artifact or rule config rejects the whole resume rather than degrading to partial reuse. A provider or model change must be asked for with --provider or --model; one that arrived through config or the environment is rejected. Two behaviour changes fall out of this. Ref text no longer decides admission, so ValidateOptions only checks the review mode. A parent that completed zero items is now admitted: its manifest is verifiable, so its selected set is simply re-dispatched, which is the case resume exists for. Reuse is then gated on the parent manifest rather than on the checkpoint lines: only a fingerprint the manifest claims as completed or reused is reused, which keeps the manifest the single source of coverage truth. That gate is also what makes an unreadable checkpoint survivable, so review loads through LoadReviewResumeState, which drops lines it cannot parse — the file such a line described is simply reviewed again, instead of one truncated write costing every other file its checkpoint. Scan keeps the strict LoadResumeState, because with no manifest to arbitrate, a dropped line cannot be told apart from a checkpoint that was never written. Rejection happens strictly before agent.New, because session.New writes session_start the moment it is called — validating any later would leave an orphan session behind every rejection. Keeping it there needs the run to review exactly what was admitted, so the pre-flight hands back the commit endpoints it resolved that identity from, and a resumed run loads its diff from those instead of from the refs the user typed; file_read reads at the same sealed head. Both loads then see the same immutable objects, so a ref moving after admission can no longer change what the run reviews, and no mid-run re-check is needed to discover that it did. An accepted resume records one resume_lineage event naming the parent run and the provider/model endpoints, surfaced by `ocr session show`; it carries non-secret labels only. Interrupted runs become unresumable, since session_end is the sole carrier of run_manifest. That is deliberate: an unverifiable input is exactly what this change refuses to build on, and the error says so rather than reporting the parent as unproductive. Refs #786 --- cmd/opencodereview/review_cmd.go | 89 +++- cmd/opencodereview/review_resume_more_test.go | 278 ++++++++++++- cmd/opencodereview/session_cmd.go | 22 + internal/agent/agent.go | 47 ++- internal/agent/agent_test.go | 6 + internal/agent/identity.go | 74 ++++ internal/agent/identity_test.go | 166 ++++++++ internal/agent/manifest_integration_test.go | 191 ++++++++- internal/agent/sealed_input_test.go | 110 +++++ internal/session/history.go | 11 + internal/session/list.go | 27 ++ internal/session/persist.go | 34 ++ internal/session/persist_test.go | 11 +- internal/session/resume.go | 107 ++++- internal/session/resume_identity.go | 191 +++++++++ internal/session/resume_identity_test.go | 382 ++++++++++++++++++ internal/session/resume_test.go | 177 ++++++-- pages/src/content/docs/en/cli-reference.md | 23 +- pages/src/content/docs/ja/cli-reference.md | 24 +- pages/src/content/docs/ru/cli-reference.md | 25 +- pages/src/content/docs/zh/cli-reference.md | 18 +- 21 files changed, 1925 insertions(+), 88 deletions(-) create mode 100644 internal/agent/identity.go create mode 100644 internal/agent/identity_test.go create mode 100644 internal/agent/sealed_input_test.go create mode 100644 internal/session/resume_identity.go create mode 100644 internal/session/resume_identity_test.go diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index e10844e4..77aa7a29 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -14,6 +14,7 @@ import ( "time" "github.com/alibaba/open-code-review/internal/agent" + "github.com/alibaba/open-code-review/internal/diff" "github.com/alibaba/open-code-review/internal/llm" "github.com/alibaba/open-code-review/internal/mcp" "github.com/alibaba/open-code-review/internal/session" @@ -157,17 +158,30 @@ func executeReview(opts reviewOptions) error { return err } cc.Template.MaxTokens = maxTokens + + // Strictly before agent.New, so a rejected resume persists nothing. The sealed + // input it returns pins the run to the very commits this check passed on, so + // the decision cannot be undone by a ref moving afterwards. + sealed, err := validateResumeIdentity(context.Background(), cc, opts, rt, resumeState) + if err != nil { + return err + } + llmIdentity := &jsonLLMIdentity{ Provider: rt.Provider, Model: rt.Model, } + var sealedInput *diff.InputResolution + if sealed != nil { + sealedInput = &sealed.Resolution + } + mode := tool.ParseReviewMode(opts.from, opts.to, opts.commit) - ref, _ := mode.RefValue(opts.to, opts.commit) fileReader := &tool.FileReader{ RepoDir: cc.RepoDir, Mode: mode, - Ref: ref, + Ref: fileReadRef(mode, opts, sealedInput), Runner: cc.GitRunner, } tools := buildToolRegistry(rt.Collector, fileReader) @@ -207,6 +221,7 @@ func executeReview(opts reviewOptions) error { Background: opts.background, GitRunner: cc.GitRunner, Resume: resumeState, + SealedInput: sealedInput, MaxTokensBudget: int64(opts.maxTokensBudget), SkipFilter: opts.noFilter, RuntimeConfig: rt.RuntimeConfig, @@ -327,19 +342,81 @@ func loadReviewResumeState(repoDir string, opts reviewOptions) (*session.ResumeS if current.ReviewMode == session.ReviewModeWorkspace { return nil, fmt.Errorf("resume requires --from/--to or --commit; workspace resume is not supported") } - state, err := session.LoadResumeState(repoDir, opts.resume) + state, err := session.LoadReviewResumeState(repoDir, opts.resume) if err != nil { return nil, fmt.Errorf("load resume session: %w (run 'ocr session list' to see available sessions)", err) } if err := state.ValidateOptions(current); err != nil { return nil, fmt.Errorf("%w (run 'ocr session list' to see available sessions)", err) } - if state.CompletedCount() == 0 { - return nil, fmt.Errorf("resume session %q has no completed review items (run 'ocr session list' to see available sessions)", opts.resume) - } + // A parent whose every item failed is deliberately allowed through: it has a + // verifiable manifest, so its whole selected set can simply be re-dispatched. + // Whether the checkpoints may be reused at all is decided later, by + // validateResumeIdentity, once the input identity is known. return state, nil } +// validateResumeIdentity rejects a resume whose input, rules, provider or model +// no longer match the parent run. +// +// It must run before agent.New: agent.New creates the session, and session.New +// writes session_start immediately, so validating any later would leave an orphan +// session on disk behind every rejection. It must also run after max-tokens is +// resolved, because the per-file token ceiling decides which large diffs are +// dropped and therefore which files the input identity covers. +// +// provider and model are explicit exactly when their flag was passed on this +// command line: both default to the empty string and nothing else can set them, +// so a provider that changed via config file or environment stays implicit — +// which is the transition this check exists to reject. +func validateResumeIdentity(ctx context.Context, cc *commonContext, opts reviewOptions, rt *llmRuntime, state *session.ResumeState) (*agent.SealedInput, error) { + if state == nil { + return nil, nil + } + sealed, err := agent.ResolveIdentity(ctx, agent.Args{ + RepoDir: cc.RepoDir, + From: opts.from, + To: opts.to, + Commit: opts.commit, + ReviewMode: reviewModeFromOptions(opts), + Template: *cc.Template, + SystemRule: cc.Resolver, + FileFilter: cc.FileFilter, + GitRunner: cc.GitRunner, + }) + if err != nil { + return nil, fmt.Errorf("resolve current input identity: %w", err) + } + if err := state.ValidateResume(session.ResumeRequest{ + Identity: sealed.Identity, + Provider: rt.Provider, + Model: rt.Model, + ProviderExplicit: opts.provider != "", + ModelExplicit: opts.model != "", + }); err != nil { + return nil, err + } + return sealed, nil +} + +// fileReadRef picks the ref file_read resolves paths against. +// +// A sealed input replaces the ref the user typed with the commit that ref +// resolved to at admission. The diff under review is pinned to that same commit, +// so leaving the reader on a moving ref would let the model read one version of a +// file while reviewing the diff of another. Workspace mode has no ref at all, and +// keeps none: its content is the working tree, which is what the diff describes. +func fileReadRef(mode tool.ReviewMode, opts reviewOptions, sealed *diff.InputResolution) string { + ref, ok := mode.RefValue(opts.to, opts.commit) + if !ok { + return "" + } + if sealed != nil && sealed.ResolvedHead != "" { + return sealed.ResolvedHead + } + return ref +} + func reviewModeFromOptions(opts reviewOptions) string { if opts.commit != "" { return session.ReviewModeCommit diff --git a/cmd/opencodereview/review_resume_more_test.go b/cmd/opencodereview/review_resume_more_test.go index edeb59db..d278c194 100644 --- a/cmd/opencodereview/review_resume_more_test.go +++ b/cmd/opencodereview/review_resume_more_test.go @@ -4,10 +4,19 @@ package main import ( + "context" + "os" + "os/exec" + "path/filepath" "strings" "testing" + "time" + "github.com/alibaba/open-code-review/internal/agent" + "github.com/alibaba/open-code-review/internal/config/template" + "github.com/alibaba/open-code-review/internal/diff" "github.com/alibaba/open-code-review/internal/session" + "github.com/alibaba/open-code-review/internal/tool" ) // writeRangeResumeSession persists a range-mode session with the given completed @@ -58,14 +67,275 @@ func TestLoadReviewResumeState_WithSession(t *testing.T) { } }) - t.Run("no completed items errors", func(t *testing.T) { + // A parent run where every item failed used to be blocked here, which made + // the one case resume exists for unrecoverable. It is now admitted: the + // manifest is verifiable, so the whole selected set is simply re-dispatched. + t.Run("no completed items is admitted", func(t *testing.T) { t.Setenv("HOME", t.TempDir()) repoDir := t.TempDir() id := writeRangeResumeSession(t, repoDir) // no items recorded - _, err := loadReviewResumeState(repoDir, reviewOptions{resume: id, from: "main", to: "feature"}) - if err == nil || !strings.Contains(err.Error(), "no completed review items") { - t.Fatalf("got %v, want no-completed-items error", err) + state, err := loadReviewResumeState(repoDir, reviewOptions{resume: id, from: "main", to: "feature"}) + if err != nil { + t.Fatalf("loadReviewResumeState: %v", err) } + if state == nil || state.CompletedCount() != 0 { + t.Fatalf("got %v, want an admitted state with 0 completed items", state) + } + }) +} + +// gitIn runs git in dir with a fixed identity so commits are reproducible. +func gitIn(t *testing.T, dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", append([]string{"-C", dir}, args...)...) + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@t", + "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@t") + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v: %s", args, err, out) + } +} + +// commitFile writes content to name and commits it. +func commitFile(t *testing.T, dir, name, content, message string) { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, name), []byte(content), 0o644); err != nil { + t.Fatalf("write %s: %v", name, err) + } + gitIn(t, dir, "add", ".") + gitIn(t, dir, "commit", "-m", message) +} + +// revParse resolves a ref to its full SHA, so a test can spell the same commit a +// different way. +func revParse(t *testing.T, dir, ref string) string { + t.Helper() + out, err := exec.Command("git", "-C", dir, "rev-parse", ref).Output() + if err != nil { + t.Fatalf("git rev-parse %s: %v", ref, err) + } + return strings.TrimSpace(string(out)) +} + +// initResumeRepo adds a second commit to the shared single-commit fixture, so +// HEAD~1..HEAD is a real range with a reviewable Go file in it. +func initResumeRepo(t *testing.T) string { + t.Helper() + dir := initTestGitRepo(t) + commitFile(t, dir, "main.go", "package main\n\nfunc main() {}\n", "add main") + return dir +} + +func resumeTestContext(repoDir string) *commonContext { + return &commonContext{ + RepoDir: repoDir, + Template: &template.Template{MaxTokens: 4000}, + IsGitRepo: true, + } +} + +// writeVerifiableParent persists a review session whose manifest records identity +// with the given provider and model, as a completed parent run would. +func writeVerifiableParent(t *testing.T, repoDir string, id session.RunIdentity, provider, model string) string { + t.Helper() + sh := session.New(repoDir, "feature", model, session.SessionOptions{ + ReviewMode: session.ReviewModeRange, + DiffFrom: "HEAD~1", + DiffTo: "HEAD", + Operation: session.OperationReview, }) + b := sh.Manifest() + b.SetRepository(session.ManifestRepository{IdentitySHA256: id.RepositorySHA256}) + b.SetInput(session.ManifestInput{Mode: id.Mode, SourceArtifactSHA256: id.SourceArtifactSHA256}) + b.SetExecution(session.ManifestExecution{ + Provider: provider, Model: model, RuleConfigSHA256: id.RuleConfigSHA256, + }) + if err := b.RegisterSelected(session.CoverageItem{ItemID: "item-1", Path: "main.go"}); err != nil { + t.Fatalf("register selected: %v", err) + } + if err := b.MarkCompleted("item-1"); err != nil { + t.Fatalf("mark completed: %v", err) + } + m, err := b.Finalize(time.Second) + if err != nil { + t.Fatalf("finalize manifest: %v", err) + } + sh.SetFinalManifest(&m) + if err := sh.Finalize(); err != nil { + t.Fatalf("finalize session: %v", err) + } + return sh.SessionID +} + +func countSessionFiles(t *testing.T, repoDir string) int { + t.Helper() + dir, err := session.SessionsDir(repoDir) + if err != nil { + t.Fatalf("SessionsDir: %v", err) + } + entries, err := os.ReadDir(dir) + if err != nil { + if os.IsNotExist(err) { + return 0 + } + t.Fatalf("read sessions dir: %v", err) + } + n := 0 + for _, e := range entries { + if strings.HasSuffix(e.Name(), ".jsonl") { + n++ + } + } + return n +} + +// TestValidateResumeIdentity drives the command-layer check against a parent that +// really exists on disk, with the identity resolved from real git input rather +// than a fixture digest. Its most important assertion is the last one: a rejected +// resume must not leave a session behind, which is only true while the check runs +// before agent.New. +func TestValidateResumeIdentity(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + repoDir := initResumeRepo(t) + cc := resumeTestContext(repoDir) + // A real range: HEAD~1..HEAD contains the commit that added main.go, so the + // resolved identity is derived from actual reviewable content rather than from + // the digest of an empty selected set. + opts := reviewOptions{from: "HEAD~1", to: "HEAD"} + + sealed, err := agent.ResolveIdentity(context.Background(), agent.Args{ + RepoDir: repoDir, + From: opts.from, + To: opts.to, + ReviewMode: reviewModeFromOptions(opts), + Template: *cc.Template, + }) + if err != nil { + t.Fatalf("ResolveIdentity: %v", err) + } + + parentID := writeVerifiableParent(t, repoDir, sealed.Identity, "anthropic", "claude") + state, err := loadReviewResumeState(repoDir, reviewOptions{resume: parentID, from: opts.from, to: opts.to}) + if err != nil { + t.Fatalf("loadReviewResumeState: %v", err) + } + if state.Manifest == nil { + t.Fatal("parent manifest did not survive the round trip through session_end") + } + + t.Run("same provider and model is accepted", func(t *testing.T) { + rt := &llmRuntime{Provider: "anthropic", Model: "claude"} + if _, err := validateResumeIdentity(context.Background(), cc, opts, rt, state); err != nil { + t.Errorf("want accepted, got: %v", err) + } + }) + + t.Run("nil state is a non-resume run", func(t *testing.T) { + rt := &llmRuntime{Provider: "anthropic", Model: "claude"} + if _, err := validateResumeIdentity(context.Background(), cc, opts, rt, nil); err != nil { + t.Errorf("a run with no --resume must not be checked, got: %v", err) + } + }) + + t.Run("differently spelled but equivalent refs are accepted", func(t *testing.T) { + // The false-rejection half of the contract. The parent was reviewed as + // HEAD~1..HEAD; naming the very same two commits by full SHA is the same + // input, and a check that compared ref text would kill this resume. + equivalent := opts + equivalent.from = revParse(t, repoDir, "HEAD~1") + equivalent.to = revParse(t, repoDir, "HEAD") + if equivalent.from == opts.from || equivalent.to == opts.to { + t.Fatal("fixture must use a different spelling than the parent did") + } + rt := &llmRuntime{Provider: "anthropic", Model: "claude"} + if _, err := validateResumeIdentity(context.Background(), cc, equivalent, rt, state); err != nil { + t.Errorf("ref spelling must not decide admission, got: %v", err) + } + }) + + t.Run("implicit provider change is rejected and persists nothing", func(t *testing.T) { + before := countSessionFiles(t, repoDir) + + rt := &llmRuntime{Provider: "openai", Model: "gpt-5"} + _, err := validateResumeIdentity(context.Background(), cc, opts, rt, state) + if err == nil { + t.Fatal("want rejection for a provider change with no --provider flag") + } + if !strings.Contains(err.Error(), "provider changed") { + t.Errorf("error must name the provider as the cause, got: %v", err) + } + if after := countSessionFiles(t, repoDir); after != before { + t.Errorf("rejection created %d session file(s); it must persist nothing", after-before) + } + }) + + t.Run("explicit provider change is accepted", func(t *testing.T) { + // opts.provider being non-empty is exactly what "--provider was passed" + // means: the flag defaults to empty and nothing else assigns it. + explicit := opts + explicit.provider = "openai" + rt := &llmRuntime{Provider: "openai", Model: "gpt-5"} + if _, err := validateResumeIdentity(context.Background(), cc, explicit, rt, state); err != nil { + t.Errorf("want accepted with --provider, got: %v", err) + } + }) + + // Last, because it moves the repo's HEAD for good: every case above needs the + // input to still match. + t.Run("changed input is rejected and persists nothing", func(t *testing.T) { + before := countSessionFiles(t, repoDir) + + // This is the motivating case. The user re-runs the identical command — + // `--from HEAD~1 --to HEAD` is unchanged — but a new commit means those refs + // now name a different diff, so the checkpoints describe work on content + // that is no longer under review. + commitFile(t, repoDir, "main.go", + "package main\n\nfunc main() { println(\"changed\") }\n", "change main") + + rt := &llmRuntime{Provider: "anthropic", Model: "claude"} + _, err := validateResumeIdentity(context.Background(), cc, opts, rt, state) + if err == nil { + t.Fatal("want rejection after the same refs resolved to a different diff") + } + if !strings.Contains(err.Error(), "reviewed input changed") { + t.Errorf("error must name the input as the cause, got: %v", err) + } + if after := countSessionFiles(t, repoDir); after != before { + t.Errorf("rejection created %d session file(s); it must persist nothing", after-before) + } + }) +} + +// TestFileReadRef pins the other half of sealing the input. The diff is pinned to +// the commit admission resolved, so file_read has to read at that same commit: +// leaving it on the ref the user typed would let the model read one version of a +// file while reviewing the diff of another. +func TestFileReadRef(t *testing.T) { + sealed := &diff.InputResolution{ResolvedBase: "aaa", ResolvedHead: "bbb"} + cases := []struct { + name string + mode tool.ReviewMode + opts reviewOptions + sealed *diff.InputResolution + want string + }{ + {"range without a seal keeps the typed ref", tool.ModeRange, + reviewOptions{from: "main", to: "feature"}, nil, "feature"}, + {"range with a seal reads at the sealed head", tool.ModeRange, + reviewOptions{from: "main", to: "feature"}, sealed, "bbb"}, + {"commit with a seal reads at the sealed head", tool.ModeCommit, + reviewOptions{commit: "HEAD"}, sealed, "bbb"}, + // Workspace content is the working tree, which is exactly what its diff + // describes, so there is no ref to pin and none to carry over. + {"workspace has no ref even with a seal", tool.ModeWorkspace, + reviewOptions{}, sealed, ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := fileReadRef(tc.mode, tc.opts, tc.sealed); got != tc.want { + t.Errorf("fileReadRef = %q, want %q", got, tc.want) + } + }) + } } diff --git a/cmd/opencodereview/session_cmd.go b/cmd/opencodereview/session_cmd.go index ac52d1bc..75fa3e65 100644 --- a/cmd/opencodereview/session_cmd.go +++ b/cmd/opencodereview/session_cmd.go @@ -284,6 +284,14 @@ func printSessionDetail(w io.Writer, s *session.Summary, items []session.ItemDet if s.ResumedFrom != "" { fmt.Fprintf(w, " Resumed: from session %s\n", s.ResumedFrom) } + if l := s.ResumeLineage; l != nil { + fmt.Fprintf(w, " Parent: run %s\n", l.ParentRunID) + if l.IsTransition() { + fmt.Fprintf(w, " Transition: %s → %s\n", + describeTarget(l.SourceProvider, l.SourceModel), + describeTarget(l.TargetProvider, l.TargetModel)) + } + } fmt.Fprintf(w, " Started: %s\n", describeStart(*s)) if !s.EndTime.IsZero() { fmt.Fprintf(w, " Ended: %s\n", s.EndTime.Local().Format("2006-01-02 15:04:05")) @@ -324,6 +332,20 @@ func printSessionDetail(w io.Writer, s *session.Summary, items []session.ItemDet tw.Flush() } +// describeTarget renders a provider/model pair for the transition line. Either +// side may be empty (a non-provider endpoint records no provider name). +func describeTarget(provider, model string) string { + switch { + case provider == "" && model == "": + return "-" + case provider == "": + return model + case model == "": + return provider + } + return provider + "/" + model +} + func displayMode(m string) string { if m == "" { return "-" diff --git a/internal/agent/agent.go b/internal/agent/agent.go index f7402a9f..b384f75c 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -136,6 +136,14 @@ type Args struct { // Resume is an optional read-only checkpoint index from a previous review session. Resume *session.ResumeState + // SealedInput pins this run to commit endpoints a pre-flight resolve already + // froze, instead of resolving From/To/Commit again. Set only on the resume + // path, where admission compared an identity derived from those endpoints: + // re-resolving a raw ref here could read a commit the admitted identity never + // covered, and the mismatch would surface only after the child session and + // manifest existed. Nil means resolve normally, which is every non-resume run. + SealedInput *diff.InputResolution + // MaxTokensBudget caps the aggregate token usage (input+output) across the // whole run; dispatch stops once the running total + a per-file look-ahead // would exceed it. 0 = unlimited. Mirrors scan.Args.MaxTokensBudget. @@ -349,6 +357,13 @@ func (a *Agent) Run(ctx context.Context) ([]model.LlmComment, error) { } } + // Record which run this one continued, before any item is dispatched, so the + // lineage is on disk even if the review then fails outright. It is written + // once per run and only for an accepted resume — admission was already + // decided by the command layer, which rejects before a session exists. + a.session.RecordResumeLineage(session.NewResumeLineage( + a.args.Resume, a.session.SessionID, a.args.Provider, a.args.Model)) + // Step 2: Dispatch per-file subtasks concurrently comments, err := a.dispatchSubtasks(ctx) if len(comments) > 0 { @@ -473,11 +488,30 @@ func (a *Agent) recordWarning(warningType, file, message string) { func (a *Agent) loadDiffs(ctx context.Context) error { var provider *diff.Provider + // A sealed input substitutes the commit SHAs a pre-flight resolve already froze + // for the refs the user typed. Both loads then read the same immutable objects, + // which is what makes this run's input provably the admitted one: a ref moving + // after admission can no longer change what gets reviewed. Neither mode's + // semantics shift under the substitution — range keeps its merge-base, because + // the sealed base already is that merge-base and merge-base(base, head) is base + // whenever base is an ancestor of head; commit mode keeps its first-parent + // comparison, which is derived from the commit rather than from its spelling. + // Workspace mode seals no head and is left alone. + from, to, commit := a.args.From, a.args.To, a.args.Commit + if s := a.args.SealedInput; s != nil && s.ResolvedHead != "" { + switch { + case commit != "": + commit = s.ResolvedHead + case s.ResolvedBase != "": + from, to = s.ResolvedBase, s.ResolvedHead + } + } + switch { - case a.args.Commit != "": - provider = diff.NewCommitProvider(a.args.RepoDir, a.args.Commit, a.args.GitRunner) - case a.args.From != "" && a.args.To != "": - provider = diff.NewProvider(a.args.RepoDir, a.args.From, a.args.To, a.args.GitRunner) + case commit != "": + provider = diff.NewCommitProvider(a.args.RepoDir, commit, a.args.GitRunner) + case from != "" && to != "": + provider = diff.NewProvider(a.args.RepoDir, from, to, a.args.GitRunner) default: provider = diff.NewWorkspaceProvider(a.args.RepoDir, a.args.GitRunner) } @@ -732,7 +766,10 @@ func (a *Agent) applyResume(diffs []model.Diff) []model.Diff { continue } fingerprint := reviewItemFingerprint(mode, d) - item, ok := resume.Item(fingerprint) + // ReusableItem, not Item: coverage lives in the parent manifest, so a + // checkpoint line the parent's manifest did not settle as completed or + // reused is not evidence of anything and its file is reviewed again. + item, ok := resume.ReusableItem(fingerprint) if !ok { toDispatch = append(toDispatch, d) continue diff --git a/internal/agent/agent_test.go b/internal/agent/agent_test.go index c0692693..ce2cd21a 100644 --- a/internal/agent/agent_test.go +++ b/internal/agent/agent_test.go @@ -583,6 +583,12 @@ func TestApplyResumeReusesCompletedItemsAcrossModels(t *testing.T) { }}, }, }, + // The checkpoint line alone earns no reuse: coverage is the parent + // manifest's to state, so a.go is only eligible because the parent settled + // it as completed. + Manifest: &session.RunManifest{ + Coverage: session.Coverage{Completed: []session.CoverageItem{{ItemID: fp, Fingerprint: fp}}}, + }, } collector := tool.NewCommentCollector() sess := session.New(t.TempDir(), "feature", "openai-model", session.SessionOptions{ diff --git a/internal/agent/identity.go b/internal/agent/identity.go new file mode 100644 index 00000000..2c125ccd --- /dev/null +++ b/internal/agent/identity.go @@ -0,0 +1,74 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package agent + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "fmt" + + "github.com/alibaba/open-code-review/internal/diff" + "github.com/alibaba/open-code-review/internal/session" + "github.com/alibaba/open-code-review/internal/stdout" +) + +// SealedInput is what a pre-flight resolve froze: the identity it computed, and +// the commit endpoints it computed that identity from. +// +// Resolution is the point of this type. Handing it back to the run pins the run +// to the same immutable commits, so the second diff load cannot disagree with +// the first — which is what lets the resume decision stay entirely before the +// run exists. Without it, a ref that moved after admission would be discovered +// only mid-run, once a child session and manifest were already on disk. +type SealedInput struct { + Identity session.RunIdentity + Resolution diff.InputResolution +} + +// ResolveIdentity computes the identity a review with these args would record, +// without creating anything: no session, manifest, runner or LLM call. Resume +// needs it before it may reuse a checkpoint, and a rejected resume must leave no +// trace on disk, so the identity has to be available strictly before +// session.New — which writes session_start the moment it is called. +// +// It reproduces the run's selection exactly — the same diff load followed by the +// same two filter passes — because the identity is derived from the filtered, +// sealed selected set, not from every parsed diff. See runIdentity for what +// skipping a pass would cost. +// +// Those filter passes are chatty, and the review that follows in the same +// command prints them again, so this stays silent. stdout.Quiet is safe here for +// the reason it documents: this is pre-flight work on the main goroutine, before +// any concurrent output exists. +func ResolveIdentity(ctx context.Context, args Args) (*SealedInput, error) { + defer stdout.Quiet()() + + a := &Agent{args: args} + if err := a.loadDiffs(ctx); err != nil { + return nil, fmt.Errorf("load diffs: %w", err) + } + a.diffs = a.filterDiffs(a.diffs) + a.diffs = a.filterLargeDiffs(a.diffs) + return &SealedInput{Identity: a.runIdentity(), Resolution: a.inputResolution}, nil +} + +// runIdentity reads the identity off the agent's current selection. +// +// It is only meaningful once the selection is final: sourceArtifactSHA256 hashes +// whatever a.diffs holds, so calling it before both filter passes yields a digest +// no run ever records, and a resume comparing that digest against a parent +// manifest would reject work it should have reused. +func (a *Agent) runIdentity() session.RunIdentity { + id := session.RunIdentity{ + Mode: a.manifestMode(), + SourceArtifactSHA256: a.sourceArtifactSHA256(), + RuleConfigSHA256: a.ruleConfigSHA256(), + } + if raw := a.repoRemoteIdentity; raw != "" { + sum := sha256.Sum256([]byte(raw)) + id.RepositorySHA256 = hex.EncodeToString(sum[:]) + } + return id +} diff --git a/internal/agent/identity_test.go b/internal/agent/identity_test.go new file mode 100644 index 00000000..a6cec8fd --- /dev/null +++ b/internal/agent/identity_test.go @@ -0,0 +1,166 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package agent + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/alibaba/open-code-review/internal/config/rules" + "github.com/alibaba/open-code-review/internal/config/template" +) + +// initIdentityRepo builds a workspace with two reviewable files, one the +// extension rules drop, and one large enough for the token filter to drop. +func initIdentityRepo(t *testing.T) string { + t.Helper() + dir := initPreviewRepo(t) + for name, content := range map[string]string{ + "main.go": "package main\n\nfunc main() {}\n", + "keep.go": "package main\n\nfunc keep() {}\n", + "notes.xyz": "not a reviewable extension\n", + "huge.go": "package main\n\n// " + strings.Repeat("filler ", 4000) + "\n", + } { + if err := os.WriteFile(filepath.Join(dir, name), []byte(content), 0o644); err != nil { + t.Fatalf("write %s: %v", name, err) + } + } + return dir +} + +func identityArgs(dir string, maxTokens int) Args { + return Args{RepoDir: dir, Template: template.Template{MaxTokens: maxTokens}} +} + +// TestResolveIdentityMatchesRunPath is the assertion the whole resume check rests +// on: the digest resolved before a run must equal the digest that run records. +// +// source_artifact_sha256 is computed from the sealed selected set, which is what +// survives filterDiffs and filterLargeDiffs — not every parsed diff. A pre-flight +// that skipped either pass would produce a value no run ever writes, and every +// resume comparison would then fail against a parent manifest for a reason that +// has nothing to do with the input having changed. The subtests below prove both +// filters really do move the digest, so this equality is load-bearing rather than +// coincidental. +func TestResolveIdentityMatchesRunPath(t *testing.T) { + dir := initIdentityRepo(t) + args := identityArgs(dir, 4000) + + // Replay the review path in Run's order and take the digest + // applyInputIdentity would hand the manifest builder. + run := &Agent{args: args} + if err := run.loadDiffs(context.Background()); err != nil { + t.Fatalf("loadDiffs: %v", err) + } + rawDigest := run.sourceArtifactSHA256() + rawCount := len(run.diffs) + + run.diffs = run.filterDiffs(run.diffs) + afterExtDigest := run.sourceArtifactSHA256() + afterExtCount := len(run.diffs) + + run.diffs = run.filterLargeDiffs(run.diffs) + want := run.sourceArtifactSHA256() + + if afterExtCount >= rawCount || len(run.diffs) >= afterExtCount { + t.Fatalf("fixture must exercise both filters, got raw=%d ext=%d large=%d", + rawCount, afterExtCount, len(run.diffs)) + } + if rawDigest == afterExtDigest || afterExtDigest == want { + t.Fatal("both filters must move the digest, otherwise this test proves nothing") + } + + sealed, err := ResolveIdentity(context.Background(), args) + if err != nil { + t.Fatalf("ResolveIdentity: %v", err) + } + got := sealed.Identity + if got.SourceArtifactSHA256 != want { + t.Errorf("source_artifact_sha256 drifted between the two entry points:\n pre-flight = %s\n run path = %s", + got.SourceArtifactSHA256, want) + } + if got.Mode == "" { + t.Error("mode is mandatory in the manifest and must never resolve empty") + } + if len(got.SourceArtifactSHA256) != 64 || len(got.RuleConfigSHA256) != 64 { + t.Errorf("digests must be 64 hex chars, got %d and %d", + len(got.SourceArtifactSHA256), len(got.RuleConfigSHA256)) + } +} + +func TestResolveIdentityTracksConfigChanges(t *testing.T) { + dir := initIdentityRepo(t) + + baseSealed, err := ResolveIdentity(context.Background(), identityArgs(dir, 4000)) + if err != nil { + t.Fatalf("ResolveIdentity: %v", err) + } + base := baseSealed.Identity + + t.Run("is deterministic", func(t *testing.T) { + again, err := ResolveIdentity(context.Background(), identityArgs(dir, 4000)) + if err != nil { + t.Fatalf("ResolveIdentity: %v", err) + } + if again.Identity != base { + t.Errorf("identity is not deterministic:\n%+v\n%+v", again.Identity, base) + } + }) + + t.Run("an exclude that drops a file moves both digests", func(t *testing.T) { + args := identityArgs(dir, 4000) + args.FileFilter = &rules.FileFilter{Exclude: []string{"keep.go"}} + sealed, err := ResolveIdentity(context.Background(), args) + if err != nil { + t.Fatalf("ResolveIdentity: %v", err) + } + got := sealed.Identity + if got.RuleConfigSHA256 == base.RuleConfigSHA256 { + t.Error("rule_config_sha256 must move when the file filter changes") + } + if got.SourceArtifactSHA256 == base.SourceArtifactSHA256 { + t.Error("source_artifact_sha256 must move when a selected file disappears") + } + }) + + t.Run("an exclude that matches nothing moves only the rule digest", func(t *testing.T) { + // This is why the resume check compares source_artifact before + // rule_config: a filter change that really dropped files is reported as + // the input change it caused, and only a filter change with no effect on + // the selected set falls through to the unattributable rule digest. + args := identityArgs(dir, 4000) + args.FileFilter = &rules.FileFilter{Exclude: []string{"no/such/path/**"}} + sealed, err := ResolveIdentity(context.Background(), args) + if err != nil { + t.Fatalf("ResolveIdentity: %v", err) + } + got := sealed.Identity + if got.SourceArtifactSHA256 != base.SourceArtifactSHA256 { + t.Error("the selected set did not change, so source_artifact_sha256 must not either") + } + if got.RuleConfigSHA256 == base.RuleConfigSHA256 { + t.Error("rule_config_sha256 must still move") + } + }) + + t.Run("max tokens moves the input identity", func(t *testing.T) { + // max_tokens feeds filterLargeDiffs, so raising it admits files the parent + // run had dropped. The input identity changes even though no file did, and + // the resume is rejected as an input change. + sealed, err := ResolveIdentity(context.Background(), identityArgs(dir, 4_000_000)) + if err != nil { + t.Fatalf("ResolveIdentity: %v", err) + } + got := sealed.Identity + if got.SourceArtifactSHA256 == base.SourceArtifactSHA256 { + t.Error("a higher token ceiling admits huge.go and must move source_artifact_sha256") + } + if got.RuleConfigSHA256 != base.RuleConfigSHA256 { + t.Error("max_tokens is not part of the rule configuration") + } + }) +} diff --git a/internal/agent/manifest_integration_test.go b/internal/agent/manifest_integration_test.go index 8952bef8..5a823e35 100644 --- a/internal/agent/manifest_integration_test.go +++ b/internal/agent/manifest_integration_test.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "strings" + "sync" "testing" "time" @@ -42,6 +43,11 @@ func (manifestFlowClient) CompletionsWithCtx(_ context.Context, req llm.ChatRequ } func newManifestFlowAgent(t *testing.T, diffs []model.Diff, resume *session.ResumeState) *Agent { + t.Helper() + return newManifestFlowAgentWithClient(t, diffs, resume, manifestFlowClient{}) +} + +func newManifestFlowAgentWithClient(t *testing.T, diffs []model.Diff, resume *session.ResumeState, client llm.LLMClient) *Agent { t.Helper() t.Setenv("HOME", t.TempDir()) repoDir := t.TempDir() @@ -57,7 +63,7 @@ func newManifestFlowAgent(t *testing.T, diffs []model.Diff, resume *session.Resu From: "main", To: "feature", ReviewMode: session.ReviewModeRange, - LLMClient: manifestFlowClient{}, + LLMClient: client, Model: "fake", Session: sh, Resume: resume, @@ -81,6 +87,35 @@ func newManifestFlowAgent(t *testing.T, diffs []model.Diff, resume *session.Resu return a } +// seedParentManifest gives a hand-built ResumeState the parent manifest a real +// resume always has, matching the identity a already resolved. Without one the +// agent reuses nothing at all: dispatch re-verifies the input against the parent +// manifest, and only fingerprints the parent's own coverage settled are eligible. +// +// It must be called once a.diffs is final — the identity hashes exactly that set. +func seedParentManifest(t *testing.T, a *Agent, fingerprints ...string) { + t.Helper() + resume := a.args.Resume + if resume == nil { + t.Fatal("seedParentManifest needs an agent built with a resume state") + } + id := a.runIdentity() + items := make([]session.CoverageItem, 0, len(fingerprints)) + for _, fp := range fingerprints { + items = append(items, session.CoverageItem{ItemID: fp, Fingerprint: fp}) + } + resume.Closed = true + resume.Manifest = &session.RunManifest{ + SchemaVersion: session.ManifestSchemaVersion, + RunID: resume.SessionID, + Operation: session.OperationReview, + Repository: session.ManifestRepository{IdentitySHA256: id.RepositorySHA256}, + Input: session.ManifestInput{Mode: id.Mode, SourceArtifactSHA256: id.SourceArtifactSHA256}, + Execution: session.ManifestExecution{RuleConfigSHA256: id.RuleConfigSHA256}, + Coverage: session.Coverage{Selected: items, Completed: items}, + } +} + func finishManifestFlow(t *testing.T, a *Agent) *session.RunManifest { t.Helper() if err := a.finalizeManifest(); err != nil { @@ -312,6 +347,7 @@ func TestManifestFlowResumeRecordsParentAndReusedItem(t *testing.T) { }, } a := newManifestFlowAgent(t, diffs, resume) + seedParentManifest(t, a, fingerprint) if _, err := a.dispatchSubtasks(context.Background()); err != nil { t.Fatalf("dispatch: %v", err) } @@ -324,6 +360,157 @@ func TestManifestFlowResumeRecordsParentAndReusedItem(t *testing.T) { } } +// promptSpy answers every request the same way and keeps the prompts, so a test +// can assert what never reached the model. +type promptSpy struct { + mu sync.Mutex + prompts []string +} + +func (p *promptSpy) CompletionsWithCtx(_ context.Context, req llm.ChatRequest) (*llm.ChatResponse, error) { + var prompt string + for _, message := range req.Messages { + if text, ok := message.Content.(string); ok { + prompt += text + } + } + p.mu.Lock() + p.prompts = append(p.prompts, prompt) + p.mu.Unlock() + return agentTaskDoneResponse(), nil +} + +// TestManifestFlowReusedCommentsStayOutOfPrompts pins the isolation half of +// reuse: a reused finding is merged into the output and nothing else. Feeding it +// back into a prompt would let the parent's conclusions steer the child's review +// of a different file, and the finding would also be paid for twice. +func TestManifestFlowReusedCommentsStayOutOfPrompts(t *testing.T) { + diffs := []model.Diff{ + {OldPath: "cached.go", NewPath: "cached.go", Diff: "+cached", Insertions: 1}, + {OldPath: "fresh.go", NewPath: "fresh.go", Diff: "+fresh", Insertions: 1}, + } + fingerprint := reviewItemFingerprint(session.ReviewModeRange, diffs[0]) + const reusedFinding = "PARENT_FINDING_ABOUT_CACHED_GO" + resume := &session.ResumeState{ + SessionID: "parent-run", + ReviewMode: session.ReviewModeRange, + DiffFrom: "main", + DiffTo: "feature", + Items: map[string]session.ResumeItem{ + fingerprint: { + FilePath: "cached.go", + OldPath: "cached.go", + NewPath: "cached.go", + Fingerprint: fingerprint, + Comments: []model.LlmComment{{Path: "cached.go", Content: reusedFinding}}, + }, + }, + } + spy := &promptSpy{} + a := newManifestFlowAgentWithClient(t, diffs, resume, spy) + seedParentManifest(t, a, fingerprint) + + comments, err := a.dispatchSubtasks(context.Background()) + if err != nil { + t.Fatalf("dispatch: %v", err) + } + + var merged bool + for _, c := range comments { + if c.Content == reusedFinding { + merged = true + } + } + if !merged { + t.Fatalf("reused finding must be merged into the output, got %+v", comments) + } + // One dispatch, for fresh.go only — cached.go was reused, so it is neither + // re-reviewed nor mentioned to the model. + spy.mu.Lock() + defer spy.mu.Unlock() + if len(spy.prompts) != 1 { + t.Fatalf("prompts = %d, want 1 (fresh.go only)", len(spy.prompts)) + } + for _, prompt := range spy.prompts { + if strings.Contains(prompt, reusedFinding) { + t.Error("a reused finding must not enter a new LLM context") + } + if strings.Contains(prompt, "cached.go") { + t.Error("a reused file must not be sent to the model again") + } + } +} + +// TestManifestFlowResumeIgnoresCheckpointTheParentManifestDoesNotVouchFor covers +// a checkpoint line whose fingerprint the parent's coverage never settled. The +// manifest is the single source of truth for coverage, so the line is not +// evidence of anything and its file is reviewed again. +func TestManifestFlowResumeIgnoresCheckpointTheParentManifestDoesNotVouchFor(t *testing.T) { + diffs := []model.Diff{{OldPath: "cached.go", NewPath: "cached.go", Diff: "+cached", Insertions: 1}} + fingerprint := reviewItemFingerprint(session.ReviewModeRange, diffs[0]) + resume := &session.ResumeState{ + SessionID: "parent-run", + ReviewMode: session.ReviewModeRange, + DiffFrom: "main", + DiffTo: "feature", + Items: map[string]session.ResumeItem{ + fingerprint: {FilePath: "cached.go", NewPath: "cached.go", Fingerprint: fingerprint}, + }, + } + a := newManifestFlowAgent(t, diffs, resume) + // A manifest that settled some *other* item: identity still matches, so the + // resume is admitted, but this fingerprint is not in completed or reused. + seedParentManifest(t, a, "fp-some-other-item") + + if _, err := a.dispatchSubtasks(context.Background()); err != nil { + t.Fatalf("dispatch: %v", err) + } + manifest := finishManifestFlow(t, a) + if len(manifest.Coverage.Reused) != 0 || len(manifest.Coverage.Completed) != 1 { + t.Fatalf("coverage reused=%d completed=%d, want 0/1 — the unvouched checkpoint must be re-reviewed", + len(manifest.Coverage.Reused), len(manifest.Coverage.Completed)) + } +} + +// TestManifestFlowResumeOfFullyFailedParentRedispatchesEverything pins the case +// resume exists for. The parent's every item failed, so replay left no +// checkpoint behind and its coverage vouches for nothing: each selected item is +// reviewed again, and the child settles them itself. +func TestManifestFlowResumeOfFullyFailedParentRedispatchesEverything(t *testing.T) { + diffs := []model.Diff{ + {OldPath: "one.go", NewPath: "one.go", Diff: "+one", Insertions: 1}, + {OldPath: "two.go", NewPath: "two.go", Diff: "+two", Insertions: 1}, + } + // No Items: every review_item_done the parent wrote was retracted by the + // review_item_failed that followed it. + resume := &session.ResumeState{ + SessionID: "parent-run", + ReviewMode: session.ReviewModeRange, + DiffFrom: "main", + DiffTo: "feature", + Items: map[string]session.ResumeItem{}, + } + + a := newManifestFlowAgent(t, diffs, resume) + fingerprints := make([]string, 0, len(diffs)) + for _, d := range diffs { + fingerprints = append(fingerprints, reviewItemFingerprint(session.ReviewModeRange, d)) + } + seedParentManifest(t, a, fingerprints...) + // Same selection, but the parent completed none of it. + cov := &a.args.Resume.Manifest.Coverage + cov.Failed, cov.Completed = cov.Completed, nil + + if _, err := a.dispatchSubtasks(context.Background()); err != nil { + t.Fatalf("dispatch: %v", err) + } + manifest := finishManifestFlow(t, a) + if len(manifest.Coverage.Reused) != 0 || len(manifest.Coverage.Completed) != len(diffs) { + t.Fatalf("coverage reused=%d completed=%d, want 0/%d — a fully failed parent must re-dispatch everything", + len(manifest.Coverage.Reused), len(manifest.Coverage.Completed), len(diffs)) + } +} + func TestManifestFlowResumeWithReusedAndAllRerunsFailedIsPartial(t *testing.T) { diffs := []model.Diff{ {OldPath: "cached.go", NewPath: "cached.go", Diff: "+cached", Insertions: 1}, @@ -346,6 +533,7 @@ func TestManifestFlowResumeWithReusedAndAllRerunsFailedIsPartial(t *testing.T) { } a := newManifestFlowAgent(t, diffs, resume) + seedParentManifest(t, a, fingerprint) if _, err := a.dispatchSubtasks(context.Background()); err != nil { t.Fatalf("partial resumed dispatch must not return an error: %v", err) } @@ -382,6 +570,7 @@ func TestManifestFlowResumeWithProviderTransition(t *testing.T) { } a := newManifestFlowAgent(t, diffs, resume) + seedParentManifest(t, a, fingerprint) // Simulate a provider/model transition on the child run. New() already // called initManifest with the default "fake" model; re-seed execution so // the frozen manifest reflects the child's actual provider/model. diff --git a/internal/agent/sealed_input_test.go b/internal/agent/sealed_input_test.go new file mode 100644 index 00000000..f93aa976 --- /dev/null +++ b/internal/agent/sealed_input_test.go @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package agent + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "testing" + + "github.com/alibaba/open-code-review/internal/config/template" + "github.com/alibaba/open-code-review/internal/session" +) + +// sealRepo builds a two-commit repo on a branch, so `main..feature` is a real +// range whose head a later commit can move. +func sealRepo(t *testing.T) string { + t.Helper() + dir := initPreviewRepo(t) + gitIn(t, dir, "branch", "-M", "main") + gitIn(t, dir, "checkout", "-b", "feature") + commitIn(t, dir, "main.go", "package main\n\nfunc main() {}\n", "add main") + return dir +} + +func gitIn(t *testing.T, dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", append([]string{"-C", dir}, args...)...) + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@t", + "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@t") + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v: %s", args, err, out) + } +} + +func commitIn(t *testing.T, dir, name, content, msg string) { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, name), []byte(content), 0o644); err != nil { + t.Fatalf("write %s: %v", name, err) + } + gitIn(t, dir, "add", ".") + gitIn(t, dir, "commit", "-m", msg) +} + +// TestSealedInputPinsRunToAdmittedCommits is the guarantee that lets the resume +// decision live entirely before the run exists. +// +// The command resolves the input once to compare identities, and the run resolves +// it again to review it. If the second resolve could see a different commit, a +// ref moving in between would admit one input and review another — and the +// mismatch would only surface once a child session and manifest were on disk. +// Handing the run the sealed endpoints removes the window: the second resolve +// reads the same immutable commits, so it can only ever agree. +// +// The control half matters as much as the pinned half: without the seal the same +// moved ref really does change the identity, so this test proves the seal is what +// holds it still rather than the fixture being inert. +func TestSealedInputPinsRunToAdmittedCommits(t *testing.T) { + dir := sealRepo(t) + args := Args{ + RepoDir: dir, + From: "main", + To: "feature", + Template: template.Template{MaxTokens: 4000}, + } + + sealed, err := ResolveIdentity(context.Background(), args) + if err != nil { + t.Fatalf("ResolveIdentity: %v", err) + } + if sealed.Resolution.ResolvedBase == "" || sealed.Resolution.ResolvedHead == "" { + t.Fatalf("a range resolve must yield both endpoints, got %+v", sealed.Resolution) + } + + // The ref moves after admission: `feature` now names a commit the admitted + // identity never covered. + commitIn(t, dir, "late.go", "package main\n\nfunc late() {}\n", "move the ref") + + t.Run("the sealed run still reviews the admitted input", func(t *testing.T) { + pinned := args + pinned.SealedInput = &sealed.Resolution + if got := runPathIdentity(t, pinned); got != sealed.Identity { + t.Errorf("a sealed run must review exactly the admitted input:\n admitted = %+v\n ran = %+v", + sealed.Identity, got) + } + }) + + t.Run("without the seal the same move changes the input", func(t *testing.T) { + if got := runPathIdentity(t, args); got == sealed.Identity { + t.Fatal("fixture proves nothing: the moved ref must change an unsealed identity") + } + }) +} + +// runPathIdentity replays what the run itself selects — the same diff load +// followed by the same two filter passes — and reads the identity off that +// selection. Going through the Agent rather than through ResolveIdentity is the +// point: it is the run's own load that the seal has to steer. +func runPathIdentity(t *testing.T, args Args) session.RunIdentity { + t.Helper() + a := &Agent{args: args} + if err := a.loadDiffs(context.Background()); err != nil { + t.Fatalf("loadDiffs: %v", err) + } + a.diffs = a.filterLargeDiffs(a.filterDiffs(a.diffs)) + return a.runIdentity() +} diff --git a/internal/session/history.go b/internal/session/history.go index 074b9c28..f36f4392 100644 --- a/internal/session/history.go +++ b/internal/session/history.go @@ -280,6 +280,17 @@ func (sh *SessionHistory) RecordReviewItemReused(filePath, oldPath, newPath, fin } } +// RecordResumeLineage persists the run's single resume_lineage event. A nil +// lineage is a non-resumed run and writes nothing. +func (sh *SessionHistory) RecordResumeLineage(l *ResumeLineage) { + if sh == nil || l == nil { + return + } + if p := sh.persist; p != nil { + p.WriteResumeLineage(l) + } +} + // RecordReviewItemFailed persists an incomplete file-level checkpoint. func (sh *SessionHistory) RecordReviewItemFailed(filePath, oldPath, newPath, fingerprint, errorMsg string) { if sh == nil { diff --git a/internal/session/list.go b/internal/session/list.go index 284663f6..0fd424b5 100644 --- a/internal/session/list.go +++ b/internal/session/list.go @@ -41,6 +41,10 @@ type Summary struct { Aborted bool `json:"aborted"` Legacy bool `json:"legacy"` RunManifest *RunManifest `json:"run_manifest,omitempty"` + + // ResumeLineage is present only for a run that resumed another, and records + // which run it continued and across which provider and model. + ResumeLineage *ResumeLineage `json:"resume_lineage,omitempty"` } // ItemDetail describes one file-level record within a session, used by `ocr session show`. @@ -80,6 +84,13 @@ type summaryRecord struct { DurationSeconds float64 `json:"duration_seconds"` LLMFailures int64 `json:"llm_failures"` RunManifest *RunManifest `json:"run_manifest"` + SchemaVersion string `json:"schema_version"` + RunID string `json:"run_id"` + ParentRunID string `json:"parent_run_id"` + SourceProvider string `json:"source_provider"` + SourceModel string `json:"source_model"` + TargetProvider string `json:"target_provider"` + TargetModel string `json:"target_model"` } // SessionsDir returns the on-disk directory that holds JSONL session files @@ -227,6 +238,22 @@ func applyRecordToSummary(s *Summary, rec summaryRecord) { if !ts.IsZero() { s.StartTime = ts } + case "resume_lineage": + // Unknown lineage schemas are ignored rather than half-read: a version + // this build does not understand may not mean these fields at all. + if rec.SchemaVersion != ResumeLineageSchemaVersion { + return + } + s.ResumeLineage = &ResumeLineage{ + Type: rec.Type, + SchemaVersion: rec.SchemaVersion, + RunID: rec.RunID, + ParentRunID: rec.ParentRunID, + SourceProvider: rec.SourceProvider, + SourceModel: rec.SourceModel, + TargetProvider: rec.TargetProvider, + TargetModel: rec.TargetModel, + } case "review_item_done": s.CompletedFiles++ s.TotalComments += countCommentsRaw(rec.Comments) diff --git a/internal/session/persist.go b/internal/session/persist.go index ffd46e6e..9bf502c4 100644 --- a/internal/session/persist.go +++ b/internal/session/persist.go @@ -320,6 +320,40 @@ func (jw *jsonlWriter) WriteToolCall(filePath string, taskType TaskType, toolNam return uuid } +// WriteResumeLineage writes the one resume_lineage record of a resumed run. +// Readers that do not know this event type ignore it, so it costs older tooling +// nothing. +func (jw *jsonlWriter) WriteResumeLineage(l *ResumeLineage) string { + uuid := generateUUID() + + jw.mu.Lock() + defer jw.mu.Unlock() + rec := map[string]any{ + "uuid": uuid, + "parentUuid": jw.lastUUID, + "type": l.Type, + "sessionId": jw.sessionID, + "timestamp": time.Now().UTC().Format(time.RFC3339), + "schema_version": l.SchemaVersion, + "run_id": l.RunID, + "parent_run_id": l.ParentRunID, + "source_provider": l.SourceProvider, + "source_model": l.SourceModel, + "target_provider": l.TargetProvider, + "target_model": l.TargetModel, + } + jw.writeRecordLocked(rec) + // Flushed like the checkpoint records are, and for the same reason: the point + // of lineage is to survive a run that dies. Left buffered it would only reach + // disk when the first item completes, which is exactly the window where a run + // is most likely to die instead. + if jw.writer != nil { + jw.writer.Flush() + } + jw.lastUUID = uuid + return uuid +} + // WriteSessionEnd writes the final session_end summary record and closes the // file. When manifest is non-nil it is embedded under "run_manifest"; session_end // is the last physical record of the stream and no separate run_manifest record diff --git a/internal/session/persist_test.go b/internal/session/persist_test.go index 44506dd5..604518d9 100644 --- a/internal/session/persist_test.go +++ b/internal/session/persist_test.go @@ -445,16 +445,19 @@ func TestReviewItemResumeRoundTrip(t *testing.T) { } } -func TestResumeStateValidateOptionsRejectsMismatchedRange(t *testing.T) { +func TestResumeStateValidateOptionsRejectsMismatchedMode(t *testing.T) { state := &ResumeState{ SessionID: "s1", ReviewMode: ReviewModeRange, DiffFrom: "main", DiffTo: "feature", } - err := state.ValidateOptions(SessionOptions{ReviewMode: ReviewModeRange, DiffFrom: "main", DiffTo: "other"}) - if err == nil { - t.Fatal("expected mismatch error") + if err := state.ValidateOptions(SessionOptions{ReviewMode: ReviewModeCommit, DiffCommit: "abc123"}); err == nil { + t.Fatal("expected mode mismatch error") + } + // Differing ref text under the same mode is not a rejection reason. + if err := state.ValidateOptions(SessionOptions{ReviewMode: ReviewModeRange, DiffFrom: "main", DiffTo: "other"}); err != nil { + t.Errorf("ref text must not decide admission, got: %v", err) } } diff --git a/internal/session/resume.go b/internal/session/resume.go index fb741dff..c9d70a9e 100644 --- a/internal/session/resume.go +++ b/internal/session/resume.go @@ -29,6 +29,22 @@ type ResumeState struct { ScanPaths []string HasScanPathScope bool Items map[string]ResumeItem + + // Manifest is the parent run's coverage snapshot, carried only by the + // session_end record. A nil Manifest means the parent's input identity cannot + // be verified, not that the parent did no work. + Manifest *RunManifest + + // Closed reports whether a session_end record was replayed. A session can + // close without a manifest — legacy sessions predate manifests, and a run that + // never froze one still writes session_end — so Closed is what tells an + // interrupted parent apart from one that closed with nothing to verify. + Closed bool + + // reusable caches the parent manifest's completed and reused fingerprints, + // built on first use by ReusableItem. Reuse is decided on one goroutine + // before any dispatch begins, so this needs no lock. + reusable map[string]bool } // ResumeItem is a completed file-level checkpoint, keyed by diff fingerprint. @@ -58,6 +74,7 @@ type resumeRecord struct { SourceSessionID string `json:"sourceSessionId"` Error string `json:"error"` Comments []model.LlmComment `json:"comments"` + RunManifest *RunManifest `json:"run_manifest"` } // SessionFilePath returns the JSONL path for a persisted session. @@ -72,8 +89,26 @@ func SessionFilePath(repoDir, sessionID string) (string, error) { return filepath.Join(home, ".opencodereview", sessionSubDir, encodeRepoPath(repoDir), sessionID+".jsonl"), nil } -// LoadResumeState replays a previous session JSONL into a fingerprint index. +// LoadResumeState replays a previous session JSONL into a fingerprint index. A +// record that cannot be parsed fails the load: with nothing to arbitrate coverage, +// a dropped line is indistinguishable from a checkpoint that was never written, +// and the pair it may have belonged to — a review_item_failed retracting an +// earlier done record — cannot be reconstructed from the rest of the file. func LoadResumeState(repoDir, sessionID string) (*ResumeState, error) { + return loadResumeState(repoDir, sessionID, false) +} + +// LoadReviewResumeState replays a review session, dropping records it cannot +// parse. Review reuse is gated on the parent manifest rather than on these lines +// (see ReusableItem), so an unreadable checkpoint just means its file is reviewed +// again — which is what a corrupted checkpoint is supposed to do. Failing the +// whole load instead would turn one bad line into the loss of every other file's +// checkpoint. +func LoadReviewResumeState(repoDir, sessionID string) (*ResumeState, error) { + return loadResumeState(repoDir, sessionID, true) +} + +func loadResumeState(repoDir, sessionID string, skipUnparseable bool) (*ResumeState, error) { path, err := SessionFilePath(repoDir, sessionID) if err != nil { return nil, err @@ -93,7 +128,7 @@ func LoadResumeState(repoDir, sessionID string) (*ResumeState, error) { for { line, readErr := reader.ReadBytes('\n') if len(line) > 0 { - if err := state.applyResumeLine(line); err != nil { + if err := state.applyResumeLine(line); err != nil && !skipUnparseable { return nil, err } } @@ -110,6 +145,8 @@ func LoadResumeState(repoDir, sessionID string) (*ResumeState, error) { return state, nil } +// applyResumeLine folds one record into the index. It reports an unparseable +// line to the caller, which decides whether that is fatal. func (s *ResumeState) applyResumeLine(line []byte) error { var rec resumeRecord if err := json.Unmarshal(line, &rec); err != nil { @@ -138,6 +175,13 @@ func (s *ResumeState) applyResumeLine(line []byte) error { if rec.Fingerprint != "" { delete(s.Items, rec.Fingerprint) } + case "session_end": + s.Closed = true + // Last one wins: a session file holds at most one session_end, but + // replaying a truncated write should not clear an earlier good one. + if rec.RunManifest != nil { + s.Manifest = rec.RunManifest + } } return nil } @@ -161,7 +205,9 @@ func (s *ResumeState) applySessionStart(rec resumeRecord) { } } -// CompletedCount returns the number of reusable file-level checkpoints. +// CompletedCount returns the number of file-level checkpoints replay recovered. +// Review reuse is narrower — see ReusableItem — while scan, having no manifest +// to consult, reuses exactly these. func (s *ResumeState) CompletedCount() int { if s == nil { return 0 @@ -182,7 +228,49 @@ func (s *ResumeState) Item(fingerprint string) (ResumeItem, bool) { return item, true } -// ValidateOptions verifies that the requested review range matches the prior session. +// ReusableItem returns the checkpoint for fingerprint only if the parent +// manifest also recorded that fingerprint as completed or reused. +// +// The manifest is the single source of truth for coverage, so a checkpoint line +// alone is not enough: the parent froze its verdict per item, and a replayed +// record that the manifest does not vouch for is a record whose outcome the +// parent did not stand behind. This is also what makes a dropped +// review_item_failed line harmless here — the failure is recorded in the +// manifest too, and coverage never lies about it. +func (s *ResumeState) ReusableItem(fingerprint string) (ResumeItem, bool) { + if s == nil || s.Manifest == nil { + return ResumeItem{}, false + } + if s.reusable == nil { + s.reusable = manifestReusableFingerprints(s.Manifest) + } + if !s.reusable[fingerprint] { + return ResumeItem{}, false + } + return s.Item(fingerprint) +} + +// manifestReusableFingerprints collects the fingerprints the parent manifest +// settled as completed or reused. Both count: a parent that itself resumed +// carries forward results it did not compute, and those are no less final. +func manifestReusableFingerprints(m *RunManifest) map[string]bool { + out := make(map[string]bool, len(m.Coverage.Completed)+len(m.Coverage.Reused)) + for _, group := range [][]CoverageItem{m.Coverage.Completed, m.Coverage.Reused} { + for _, item := range group { + if item.Fingerprint != "" { + out[item.Fingerprint] = true + } + } + } + return out +} + +// ValidateOptions verifies that this session can be resumed in the requested +// review mode at all. It deliberately does not compare the ref text the user +// typed: `abc1234` and `abc1234def` can name the same commit while a ref whose +// name did not change can name a new one, so ref spellings are neither +// sufficient nor necessary evidence about the input. ValidateResume compares the +// resolved input identity instead. func (s *ResumeState) ValidateOptions(opts SessionOptions) error { if s == nil { return nil @@ -196,16 +284,7 @@ func (s *ResumeState) ValidateOptions(opts SessionOptions) error { if s.ReviewMode != opts.ReviewMode { return fmt.Errorf("resume session review mode %q does not match current mode %q", s.ReviewMode, opts.ReviewMode) } - switch opts.ReviewMode { - case ReviewModeRange: - if s.DiffFrom != opts.DiffFrom || s.DiffTo != opts.DiffTo { - return fmt.Errorf("resume session range %q..%q does not match current range %q..%q", s.DiffFrom, s.DiffTo, opts.DiffFrom, opts.DiffTo) - } - case ReviewModeCommit: - if s.DiffCommit != opts.DiffCommit { - return fmt.Errorf("resume session commit %q does not match current commit %q", s.DiffCommit, opts.DiffCommit) - } - default: + if opts.ReviewMode != ReviewModeRange && opts.ReviewMode != ReviewModeCommit { return fmt.Errorf("resume mode %q is not supported", opts.ReviewMode) } return nil diff --git a/internal/session/resume_identity.go b/internal/session/resume_identity.go new file mode 100644 index 00000000..e318b99d --- /dev/null +++ b/internal/session/resume_identity.go @@ -0,0 +1,191 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package session + +import "fmt" + +// RunIdentity is the resolved input identity of a candidate run. Every field +// mirrors the manifest field a finished run records, so a parent manifest and a +// child candidate are directly comparable. It is produced by agent.ResolveIdentity +// before any session exists. +type RunIdentity struct { + Mode string // manifest input.mode + SourceArtifactSHA256 string // manifest input.source_artifact_sha256 + RuleConfigSHA256 string // manifest execution.rule_config_sha256 + RepositorySHA256 string // manifest repository.identity_sha256; empty when the repo has no remote +} + +// ResumeRequest is what the resuming command is asking to do: run this input +// identity with this provider and model, reusing a parent's checkpoints. +// +// ProviderExplicit and ModelExplicit report whether the value came from a flag +// on this very command line. A provider or model that changed because a config +// file, environment variable or shell RC changed is an implicit transition, and +// an implicit transition is exactly what this check exists to catch. +type ResumeRequest struct { + Identity RunIdentity + Provider string + Model string + ProviderExplicit bool + ModelExplicit bool +} + +const resumeHint = "start a new review instead of resuming" + +// ValidateResume reports whether req may reuse this parent's checkpoints. +// +// A mismatch on any input field rejects the whole resume rather than degrading to +// partial reuse: mixing results computed from one input with results computed +// from another produces a report in which no field distinguishes the two. The +// caller must run this before session.New — session.New writes session_start the +// moment it is called, so a later check would leave an orphan session behind +// every rejection. +// +// Checks run top to bottom and the first mismatch decides. Order matters at +// source_artifact vs rule_config: rule_config_sha256 is a single aggregate over +// the rule-text layers and the file filter and cannot be decomposed, so a filter +// change is reported as the input mismatch it actually caused rather than as an +// unattributable rule change. +func (s *ResumeState) ValidateResume(req ResumeRequest) error { + if s == nil { + return nil + } + if err := s.validateInputIdentity(req.Identity); err != nil { + return err + } + + m := s.Manifest + providerChanged := m.Execution.Provider != req.Provider + if providerChanged && !req.ProviderExplicit { + return fmt.Errorf("resume rejected: provider changed from %q to %q without being asked for; %s to resume across providers on purpose", m.Execution.Provider, req.Provider, explicitFlagHint("--provider", req.Provider)) + } + // Model is only compared within the same provider: switching provider on + // purpose necessarily brings that provider's own model with it. + if !providerChanged && m.Execution.Model != req.Model && !req.ModelExplicit { + return fmt.Errorf("resume rejected: model changed from %q to %q without being asked for; %s to resume across models on purpose", m.Execution.Model, req.Model, explicitFlagHint("--model", req.Model)) + } + return nil +} + +// validateInputIdentity compares only the input half of the resume contract: the +// parent manifest must be verifiable, and every input field must match. Provider +// and model are deliberately left out: those are command-line intent, not +// something derived from the input. +// +// This runs once, at admission, and is never repeated during the run: the caller +// pins the run to the commit endpoints this comparison was made against (see +// agent.SealedInput), so a second comparison could only ever confirm the first. +func (s *ResumeState) validateInputIdentity(id RunIdentity) error { + if s == nil { + return nil + } + + m := s.Manifest + switch { + case m == nil && !s.Closed: + // Distinguishing all of these from "manifest present, zero completed + // items" is the whole point: that one is resumable, none of these are. + return fmt.Errorf("resume session %q was interrupted before it closed, so it never recorded a run manifest and its input identity cannot be verified; %s", s.SessionID, resumeHint) + case m == nil: + // It closed cleanly, so blaming an interruption would send the user + // looking for a crash that never happened. A session_end with no manifest + // is a session older than run manifests, or a run that failed before + // freezing one. + return fmt.Errorf("resume session %q closed without a run manifest, so its input identity cannot be verified — it either predates run manifests or failed before recording one; %s", s.SessionID, resumeHint) + case m.SchemaVersion != ManifestSchemaVersion: + return fmt.Errorf("resume session %q carries manifest schema %q, but this build can only verify %q; %s", s.SessionID, m.SchemaVersion, ManifestSchemaVersion, resumeHint) + case m.Operation != OperationReview: + return fmt.Errorf("resume session %q recorded operation %q, not %q; %s", s.SessionID, m.Operation, OperationReview, resumeHint) + case len(m.Coverage.Selected) == 0: + // Without this, an empty parent and an empty child would both hash to the + // canonical empty digest, pass every comparison, and produce a run that + // reuses nothing and dispatches nothing. + return fmt.Errorf("resume session %q selected no input, so it has nothing to resume; %s", s.SessionID, resumeHint) + } + + if m.Input.Mode != id.Mode { + // Mode feeds item_id derivation, so parent and child items cannot even be + // put side by side. + return fmt.Errorf("resume rejected: input mode changed from %q to %q; %s", m.Input.Mode, id.Mode, resumeHint) + } + // Both sides empty means a repository with no remote, which is unchanged. + if m.Repository.IdentitySHA256 != id.RepositorySHA256 { + return fmt.Errorf("resume rejected: repository identity changed, so this is not the repository the parent run reviewed; %s", resumeHint) + } + if m.Input.SourceArtifactSHA256 != id.SourceArtifactSHA256 { + return fmt.Errorf("resume rejected: the reviewed input changed since session %q — a ref may now point at a different commit, or the selected file set changed; %s", s.SessionID, resumeHint) + } + if m.Execution.RuleConfigSHA256 == "" { + return fmt.Errorf("resume session %q recorded no rule identity, so it cannot be verified against the current rules; %s", s.SessionID, resumeHint) + } + if m.Execution.RuleConfigSHA256 != id.RuleConfigSHA256 { + // The digest is one aggregate, so it can only be attributed to a layer, + // never to a specific rule or pattern. + return fmt.Errorf("resume rejected: review rule identity changed — either a rule text layer (custom, project, global or system) or the include/exclude file filter differs from session %q; %s", s.SessionID, resumeHint) + } + return nil +} + +// explicitFlagHint renders the actionable half of a transition rejection. value +// is empty whenever the endpoint has no provider name — one configured straight +// from environment variables has none — and `pass --provider ` is not a command +// anyone can run, so name the flag rather than echoing the empty value. +func explicitFlagHint(flag, value string) string { + if value == "" { + return "pass " + flag + " explicitly" + } + return "pass " + flag + " " + value +} + +// ResumeLineageSchemaVersion versions the resume_lineage event independently of +// the run manifest: lineage records a transition between runs, not a run's +// coverage, so the two evolve separately. +const ResumeLineageSchemaVersion = "ocr.resume-lineage/v1" + +// ResumeLineage records which run this one continued and which provider and +// model it moved between. Source and target being equal means a same-target +// resume; there is no separate transition-kind field because the kind is exactly +// what comparing them tells you. +// +// Every field is a non-secret label. No API key, authorization header, endpoint, +// absolute path, diff, prompt or model response ever belongs here. +type ResumeLineage struct { + Type string `json:"type"` + SchemaVersion string `json:"schema_version"` + RunID string `json:"run_id"` + ParentRunID string `json:"parent_run_id"` + SourceProvider string `json:"source_provider"` + SourceModel string `json:"source_model"` + TargetProvider string `json:"target_provider"` + TargetModel string `json:"target_model"` +} + +// NewResumeLineage builds the lineage for an accepted resume of parent. It +// returns nil when there is nothing to record — no parent, or a parent with no +// verifiable manifest — so callers can hand the result straight to +// RecordResumeLineage. +func NewResumeLineage(parent *ResumeState, runID, targetProvider, targetModel string) *ResumeLineage { + if parent == nil || parent.Manifest == nil { + return nil + } + return &ResumeLineage{ + Type: "resume_lineage", + SchemaVersion: ResumeLineageSchemaVersion, + RunID: runID, + ParentRunID: parent.Manifest.RunID, + SourceProvider: parent.Manifest.Execution.Provider, + SourceModel: parent.Manifest.Execution.Model, + TargetProvider: targetProvider, + TargetModel: targetModel, + } +} + +// IsTransition reports whether the resume changed provider or model, which is +// the condition that required an explicit flag to be accepted. +func (l *ResumeLineage) IsTransition() bool { + if l == nil { + return false + } + return l.SourceProvider != l.TargetProvider || l.SourceModel != l.TargetModel +} diff --git a/internal/session/resume_identity_test.go b/internal/session/resume_identity_test.go new file mode 100644 index 00000000..3a605202 --- /dev/null +++ b/internal/session/resume_identity_test.go @@ -0,0 +1,382 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package session + +import ( + "os" + "strings" + "testing" +) + +// parentIdentity is the identity every fixture parent below was run with. Each +// test case mutates exactly one field of the child so the assertion pins which +// comparison did the rejecting. +var parentIdentity = RunIdentity{ + Mode: InputModeRange, + SourceArtifactSHA256: "artifact-parent", + RuleConfigSHA256: "rules-parent", + RepositorySHA256: "repo-parent", +} + +// parentState builds a resumable parent: a v1 review manifest whose identity is +// parentIdentity, run on anthropic/claude, with one selected item. mutate may +// adjust the manifest to model a parent that is older, interrupted or empty. +func parentState(mutate func(*RunManifest)) *ResumeState { + m := &RunManifest{ + SchemaVersion: ManifestSchemaVersion, + RunID: "run-parent", + Operation: OperationReview, + Repository: ManifestRepository{IdentitySHA256: parentIdentity.RepositorySHA256}, + Input: ManifestInput{ + Mode: parentIdentity.Mode, + SourceArtifactSHA256: parentIdentity.SourceArtifactSHA256, + }, + Execution: ManifestExecution{ + Provider: "anthropic", + Model: "claude", + RuleConfigSHA256: parentIdentity.RuleConfigSHA256, + }, + Coverage: Coverage{Selected: []CoverageItem{{ItemID: "item-1"}}}, + } + if mutate != nil { + mutate(m) + } + return &ResumeState{SessionID: "parent-session", Manifest: m, Closed: true} +} + +// request is a resume of parentState with the same provider and model, which is +// the accepted baseline every case below deviates from. +func request(mutate func(*ResumeRequest)) ResumeRequest { + req := ResumeRequest{ + Identity: parentIdentity, + Provider: "anthropic", + Model: "claude", + } + if mutate != nil { + mutate(&req) + } + return req +} + +func TestValidateResume(t *testing.T) { + tests := []struct { + name string + // parent mutates the fixture parent manifest; nil leaves it resumable. + parent func(*RunManifest) + // req mutates the matching request; nil leaves it accepted. + req func(*ResumeRequest) + // state mutates the parent state itself, for the two unverifiable-parent + // cases that cannot be expressed by mutating a manifest. + state func(*ResumeState) + // wantErr is a substring of the required error, or "" to require acceptance. + wantErr string + }{ + { + name: "identical input, provider and model is accepted", + }, + { + name: "parent that completed nothing is still resumable", + // The parent's own coverage is irrelevant here: admission depends on + // whether the input is verifiable, not on how much work got done. A + // fully-failed parent is the case resume exists for. + parent: func(m *RunManifest) { + m.Coverage.Failed = []CoverageItem{{ItemID: "item-1"}} + }, + }, + { + name: "differing ref text with identical resolved input is accepted", + // Nothing in the request carries ref text any more; this pins that a + // parent recording one spelling accepts a child that resolved the same + // input from another. + parent: func(m *RunManifest) { + m.Input.RequestedFrom = "main" + m.Input.RequestedHead = "abc1234" + }, + }, + { + name: "interrupted parent with no manifest is rejected as unverifiable", + state: func(s *ResumeState) { s.Manifest = nil; s.Closed = false }, + wantErr: "was interrupted before it closed", + }, + { + // Same rejection, different cause: this one did close, so blaming an + // interruption would send the user hunting a crash that never happened. + name: "parent that closed without a manifest is rejected as unverifiable", + state: func(s *ResumeState) { s.Manifest = nil }, + wantErr: "closed without a run manifest", + }, + { + name: "unknown manifest schema is rejected", + parent: func(m *RunManifest) { m.SchemaVersion = "ocr.run-manifest/v99" }, + wantErr: "manifest schema", + }, + { + name: "non-review parent is rejected", + parent: func(m *RunManifest) { m.Operation = "scan" }, + wantErr: "operation", + }, + { + name: "parent that selected nothing is rejected", + parent: func(m *RunManifest) { m.Coverage.Selected = nil }, + wantErr: "selected no input", + }, + { + name: "changed input mode is rejected", + req: func(r *ResumeRequest) { r.Identity.Mode = InputModeCommit }, + wantErr: "input mode changed", + }, + { + name: "changed repository identity is rejected", + req: func(r *ResumeRequest) { r.Identity.RepositorySHA256 = "repo-other" }, + wantErr: "repository identity changed", + }, + { + name: "repository with no remote on both sides is unchanged", + parent: func(m *RunManifest) { + m.Repository.IdentitySHA256 = "" + }, + req: func(r *ResumeRequest) { r.Identity.RepositorySHA256 = "" }, + }, + { + name: "changed source artifact is rejected", + req: func(r *ResumeRequest) { r.Identity.SourceArtifactSHA256 = "artifact-moved" }, + wantErr: "reviewed input changed", + }, + { + name: "parent without rule identity is rejected as unverifiable", + parent: func(m *RunManifest) { m.Execution.RuleConfigSHA256 = "" }, + wantErr: "no rule identity", + }, + { + name: "changed rule config is rejected", + req: func(r *ResumeRequest) { r.Identity.RuleConfigSHA256 = "rules-other" }, + wantErr: "rule identity changed", + }, + { + // A filter change moves both digests, and source artifact is compared + // first so the user is told what actually changed about the input + // rather than being pointed at an unattributable rule digest. + name: "filter change is reported as an input change, not a rule change", + req: func(r *ResumeRequest) { + r.Identity.SourceArtifactSHA256 = "artifact-fewer-files" + r.Identity.RuleConfigSHA256 = "rules-with-exclude" + }, + wantErr: "reviewed input changed", + }, + { + name: "implicit provider change is rejected", + req: func(r *ResumeRequest) { r.Provider = "openai" }, + wantErr: "provider changed", + }, + { + name: "explicit provider change is accepted", + req: func(r *ResumeRequest) { + r.Provider = "openai" + r.Model = "gpt-5" + r.ProviderExplicit = true + }, + }, + { + name: "implicit model change is rejected", + req: func(r *ResumeRequest) { r.Model = "claude-next" }, + wantErr: "model changed", + }, + { + name: "explicit model change is accepted", + req: func(r *ResumeRequest) { + r.Model = "claude-next" + r.ModelExplicit = true + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + state := parentState(tc.parent) + if tc.state != nil { + tc.state(state) + } + + err := state.ValidateResume(request(tc.req)) + + switch { + case tc.wantErr == "" && err != nil: + t.Fatalf("want accepted, got error: %v", err) + case tc.wantErr != "" && err == nil: + t.Fatalf("want rejection mentioning %q, got accepted", tc.wantErr) + case tc.wantErr != "" && !strings.Contains(err.Error(), tc.wantErr): + t.Fatalf("error %q does not mention %q", err, tc.wantErr) + } + }) + } +} + +// A nil parent is a non-resume run and must not be second-guessed here. +func TestValidateResumeNilStateAccepts(t *testing.T) { + var s *ResumeState + if err := s.ValidateResume(request(nil)); err != nil { + t.Errorf("nil state must accept, got: %v", err) + } +} + +// An endpoint configured straight from environment variables resolves no +// provider name, so the rejection must not tell the user to run `--provider ` +// with nothing after it. The same applies to a model that resolved empty. +func TestTransitionRejectionStaysActionableWithNoName(t *testing.T) { + for _, tc := range []struct { + name string + req func(*ResumeRequest) + wantSub string + }{ + { + name: "provider resolved to no name", + req: func(r *ResumeRequest) { r.Provider = "" }, + wantSub: "pass --provider explicitly", + }, + { + name: "model resolved to no name", + req: func(r *ResumeRequest) { r.Model = "" }, + wantSub: "pass --model explicitly", + }, + } { + t.Run(tc.name, func(t *testing.T) { + err := parentState(nil).ValidateResume(request(tc.req)) + if err == nil { + t.Fatal("an unasked-for transition must still be rejected") + } + if !strings.Contains(err.Error(), tc.wantSub) { + t.Errorf("message must name the flag rather than echo an empty value, got: %v", err) + } + }) + } +} + +func TestNewResumeLineage(t *testing.T) { + t.Run("nil parent records nothing", func(t *testing.T) { + if l := NewResumeLineage(nil, "run-child", "anthropic", "claude"); l != nil { + t.Errorf("want nil for a non-resume run, got %+v", l) + } + }) + + t.Run("parent without manifest records nothing", func(t *testing.T) { + s := parentState(nil) + s.Manifest = nil + if l := NewResumeLineage(s, "run-child", "anthropic", "claude"); l != nil { + t.Errorf("want nil when there is no parent run id, got %+v", l) + } + }) + + t.Run("transition carries both endpoints", func(t *testing.T) { + l := NewResumeLineage(parentState(nil), "run-child", "openai", "gpt-5") + if l == nil { + t.Fatal("want a lineage") + } + want := ResumeLineage{ + Type: "resume_lineage", + SchemaVersion: ResumeLineageSchemaVersion, + RunID: "run-child", + ParentRunID: "run-parent", + SourceProvider: "anthropic", + SourceModel: "claude", + TargetProvider: "openai", + TargetModel: "gpt-5", + } + if *l != want { + t.Errorf("lineage mismatch:\n got %+v\nwant %+v", *l, want) + } + if !l.IsTransition() { + t.Error("a provider and model change is a transition") + } + }) + + t.Run("same target is a lineage but not a transition", func(t *testing.T) { + l := NewResumeLineage(parentState(nil), "run-child", "anthropic", "claude") + if l == nil { + t.Fatal("a same-target resume still records its parent") + } + if l.IsTransition() { + t.Error("identical source and target is not a transition") + } + }) + + t.Run("nil lineage is not a transition", func(t *testing.T) { + var l *ResumeLineage + if l.IsTransition() { + t.Error("nil must not report a transition") + } + }) +} + +// End-to-end for the event itself: a lineage written by a live session must come +// back out of the session file through the same reader `ocr session show` uses. +func TestResumeLineageRoundTripsThroughSessionFile(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + repoDir := t.TempDir() + + sh := New(repoDir, "feature", "gpt-5", SessionOptions{ + ReviewMode: ReviewModeRange, + DiffFrom: "main", + DiffTo: "feature", + ResumedFrom: "parent-session", + Operation: OperationReview, + }) + want := NewResumeLineage(parentState(nil), sh.SessionID, "openai", "gpt-5") + sh.RecordResumeLineage(want) + if err := sh.Finalize(); err != nil { + t.Fatalf("finalize session: %v", err) + } + + summary, err := LoadSummary(repoDir, sh.SessionID) + if err != nil { + t.Fatalf("LoadSummary: %v", err) + } + got := summary.ResumeLineage + if got == nil { + t.Fatal("session file carries no resume_lineage") + } + if *got != *want { + t.Errorf("lineage did not round-trip:\n got %+v\nwant %+v", *got, *want) + } +} + +// The round trip above finalizes the session, which flushes on its way out and +// so cannot tell a buffered record from a persisted one. Lineage exists to +// survive a run that dies before it finalizes, so assert it is on disk while the +// session is still open. +func TestResumeLineageReachesDiskBeforeFinalize(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + repoDir := t.TempDir() + + sh := New(repoDir, "feature", "gpt-5", SessionOptions{ + ReviewMode: ReviewModeRange, + Operation: OperationReview, + }) + sh.RecordResumeLineage(NewResumeLineage(parentState(nil), sh.SessionID, "openai", "gpt-5")) + + path, err := SessionFilePath(repoDir, sh.SessionID) + if err != nil { + t.Fatalf("SessionFilePath: %v", err) + } + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read session file: %v", err) + } + if !strings.Contains(string(raw), ResumeLineageSchemaVersion) { + t.Error("resume_lineage is still buffered; a run that dies here loses its lineage") + } +} + +// An unknown lineage schema must be ignored rather than half-read: its fields may +// not mean what this build assumes. +func TestUnknownLineageSchemaIsIgnored(t *testing.T) { + var s Summary + applyRecordToSummary(&s, summaryRecord{ + Type: "resume_lineage", + SchemaVersion: "ocr.resume-lineage/v99", + ParentRunID: "run-parent", + }) + if s.ResumeLineage != nil { + t.Errorf("want an unknown schema ignored, got %+v", s.ResumeLineage) + } +} diff --git a/internal/session/resume_test.go b/internal/session/resume_test.go index bf39eb03..ab28f2f4 100644 --- a/internal/session/resume_test.go +++ b/internal/session/resume_test.go @@ -113,6 +113,26 @@ func TestItem_ReturnsCopy(t *testing.T) { } } +// --- ResumeState.ReusableItem --- + +// The manifest settles coverage per item, and only completed and reused are +// settled as results worth carrying forward. A failed item is settled too — as +// work that must happen again — so the checkpoint index agreeing is not enough. +func TestReusableItem_ManifestFailureIsNotReusable(t *testing.T) { + s := &ResumeState{ + Items: map[string]ResumeItem{"fp-failed": {FilePath: "a.go", Fingerprint: "fp-failed"}}, + Manifest: &RunManifest{ + Coverage: Coverage{ + Selected: []CoverageItem{{ItemID: "fp-failed", Fingerprint: "fp-failed"}}, + Failed: []CoverageItem{{ItemID: "fp-failed", Fingerprint: "fp-failed"}}, + }, + }, + } + if _, ok := s.ReusableItem("fp-failed"); ok { + t.Error("an item the parent manifest settled as failed must be reviewed again") + } +} + // --- ValidateOptions --- func TestValidateOptions_NilState(t *testing.T) { @@ -171,7 +191,11 @@ func TestValidateOptions_RangeMatches(t *testing.T) { } } -func TestValidateOptions_RangeMismatch(t *testing.T) { +// Ref text is no longer evidence about the input: two spellings can name the +// same commit and one spelling can name two different commits over time. +// ValidateResume compares the resolved input identity instead, so ValidateOptions +// must not reject on the typed refs alone. +func TestValidateOptions_IgnoresRangeText(t *testing.T) { s := &ResumeState{ ReviewMode: ReviewModeRange, DiffFrom: "main", @@ -182,8 +206,8 @@ func TestValidateOptions_RangeMismatch(t *testing.T) { DiffFrom: "main", DiffTo: "feature-b", }) - if err == nil { - t.Fatal("expected error for range mismatch") + if err != nil { + t.Errorf("ref text must not decide admission, got: %v", err) } } @@ -201,7 +225,7 @@ func TestValidateOptions_CommitMatches(t *testing.T) { } } -func TestValidateOptions_CommitMismatch(t *testing.T) { +func TestValidateOptions_IgnoresCommitText(t *testing.T) { s := &ResumeState{ ReviewMode: ReviewModeCommit, DiffCommit: "abc123", @@ -210,8 +234,8 @@ func TestValidateOptions_CommitMismatch(t *testing.T) { ReviewMode: ReviewModeCommit, DiffCommit: "def456", }) - if err == nil { - t.Fatal("expected error for commit mismatch") + if err != nil { + t.Errorf("ref text must not decide admission, got: %v", err) } } @@ -237,9 +261,7 @@ func TestApplyResumeLine_SessionStart(t *testing.T) { DiffFrom: "main", DiffTo: "feature", }) - if err := s.applyResumeLine(line); err != nil { - t.Fatalf("unexpected error: %v", err) - } + s.applyResumeLine(line) if s.SessionID != "sess-1" { t.Errorf("SessionID = %q", s.SessionID) } @@ -261,9 +283,7 @@ func TestApplyResumeLine_ReviewItemDone(t *testing.T) { Fingerprint: "fp-handler", Comments: []model.LlmComment{{Content: "potential nil deref"}}, }) - if err := s.applyResumeLine(line); err != nil { - t.Fatalf("unexpected error: %v", err) - } + s.applyResumeLine(line) if s.CompletedCount() != 1 { t.Fatalf("CompletedCount = %d, want 1", s.CompletedCount()) } @@ -283,9 +303,7 @@ func TestApplyResumeLine_ReviewItemDone_FallbackToNewPath(t *testing.T) { NewPath: "renamed.go", Fingerprint: "fp-renamed", }) - if err := s.applyResumeLine(line); err != nil { - t.Fatalf("unexpected error: %v", err) - } + s.applyResumeLine(line) item, ok := s.Item("fp-renamed") if !ok { t.Fatal("missing fp-renamed") @@ -301,9 +319,7 @@ func TestApplyResumeLine_ReviewItemDone_EmptyFingerprint(t *testing.T) { Type: "review_item_done", FilePath: "skip.go", }) - if err := s.applyResumeLine(line); err != nil { - t.Fatalf("unexpected error: %v", err) - } + s.applyResumeLine(line) if s.CompletedCount() != 0 { t.Error("items with empty fingerprint should be skipped") } @@ -316,9 +332,7 @@ func TestApplyResumeLine_ReviewItemReused(t *testing.T) { FilePath: "reused.go", Fingerprint: "fp-reused", }) - if err := s.applyResumeLine(line); err != nil { - t.Fatalf("unexpected error: %v", err) - } + s.applyResumeLine(line) if _, ok := s.Item("fp-reused"); !ok { t.Error("review_item_reused should be tracked in Items") } @@ -332,9 +346,7 @@ func TestApplyResumeLine_ReviewItemFailed(t *testing.T) { Type: "review_item_failed", Fingerprint: "fp-fail", }) - if err := s.applyResumeLine(line); err != nil { - t.Fatalf("unexpected error: %v", err) - } + s.applyResumeLine(line) if _, ok := s.Item("fp-fail"); ok { t.Error("failed item should be removed from Items") } @@ -347,9 +359,7 @@ func TestApplyResumeLine_ReviewItemFailed_EmptyFingerprint(t *testing.T) { line := mustJSON(t, resumeRecord{ Type: "review_item_failed", }) - if err := s.applyResumeLine(line); err != nil { - t.Fatalf("unexpected error: %v", err) - } + s.applyResumeLine(line) // Should not affect existing items when fingerprint is empty. if s.CompletedCount() != 1 { t.Error("empty fingerprint failure should not affect existing items") @@ -359,9 +369,7 @@ func TestApplyResumeLine_ReviewItemFailed_EmptyFingerprint(t *testing.T) { func TestApplyResumeLine_UnknownType(t *testing.T) { s := &ResumeState{Items: make(map[string]ResumeItem)} line := mustJSON(t, resumeRecord{Type: "session_end"}) - if err := s.applyResumeLine(line); err != nil { - t.Fatalf("unknown record types should be silently ignored, got: %v", err) - } + s.applyResumeLine(line) if s.CompletedCount() != 0 { t.Error("unknown type should not add items") } @@ -369,9 +377,11 @@ func TestApplyResumeLine_UnknownType(t *testing.T) { func TestApplyResumeLine_InvalidJSON(t *testing.T) { s := &ResumeState{Items: make(map[string]ResumeItem)} - err := s.applyResumeLine([]byte(`{invalid json}`)) - if err == nil { - t.Fatal("expected error for invalid JSON") + if err := s.applyResumeLine([]byte(`{invalid json}`)); err == nil { + t.Fatal("an unreadable line must be reported; the caller decides whether it is fatal") + } + if s.CompletedCount() != 0 { + t.Error("an unreadable line must not add items") } } @@ -570,12 +580,16 @@ func TestLoadResumeState_MultipleRecords(t *testing.T) { } } -func TestLoadResumeState_InvalidJSON(t *testing.T) { - tmpHome := t.TempDir() - t.Setenv("HOME", tmpHome) +// TestLoadReviewResumeState_CorruptLineDoesNotAbortLoad pins the cost of one +// unreadable line in a review session: the file it described is reviewed again, +// and nothing else is affected. Failing the load instead would let a single +// truncated write cost every other file its checkpoint — the opposite of what a +// checkpoint is for. +func TestLoadReviewResumeState_CorruptLineDoesNotAbortLoad(t *testing.T) { + t.Setenv("HOME", t.TempDir()) - repoDir := "/test/invalid" - sessionID := "bad-json" + repoDir := "/test/corrupt" + sessionID := "corrupt-session" path, err := SessionFilePath(repoDir, sessionID) if err != nil { t.Fatal(err) @@ -583,13 +597,92 @@ func TestLoadResumeState_InvalidJSON(t *testing.T) { if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil { t.Fatal(err) } - if err := os.WriteFile(path, []byte("{bad json}\n"), 0600); err != nil { + lines := [][]byte{ + mustJSON(t, resumeRecord{Type: "session_start", SessionID: sessionID, ReviewMode: ReviewModeRange}), + mustJSON(t, resumeRecord{Type: "review_item_done", FilePath: "good.go", Fingerprint: "fp-good"}), + []byte(`{"type":"review_item_done","fingerprint":`), + mustJSON(t, resumeRecord{Type: "review_item_done", FilePath: "later.go", Fingerprint: "fp-later"}), + mustJSON(t, resumeRecord{Type: "session_end", RunManifest: &RunManifest{ + Coverage: Coverage{Completed: []CoverageItem{ + {ItemID: "fp-good", Fingerprint: "fp-good"}, + {ItemID: "fp-later", Fingerprint: "fp-later"}, + }}, + }}), + } + var buf []byte + for _, line := range lines { + buf = append(append(buf, line...), '\n') + } + if err := os.WriteFile(path, buf, 0600); err != nil { t.Fatal(err) } - _, err = LoadResumeState(repoDir, sessionID) - if err == nil { - t.Fatal("expected error for invalid JSON content") + state, err := LoadReviewResumeState(repoDir, sessionID) + if err != nil { + t.Fatalf("one bad line must not fail a review load: %v", err) + } + // Records on both sides of the bad line survive — replay does not stop at it. + for _, fp := range []string{"fp-good", "fp-later"} { + if _, ok := state.ReusableItem(fp); !ok { + t.Errorf("%s should still be reusable", fp) + } + } +} + +// TestLoadResumeState_CorruptLineIsFatal pins the strict entry point, which scan +// resume uses. Scan has no manifest to arbitrate coverage, so an unreadable line +// cannot be told apart from a checkpoint that was never written: reporting the +// damage is the only honest answer, and silently reusing — or silently discarding +// — every other checkpoint is not. +func TestLoadResumeState_CorruptLineIsFatal(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + repoDir := "/test/corrupt-strict" + sessionID := "strict-session" + path, err := SessionFilePath(repoDir, sessionID) + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil { + t.Fatal(err) + } + buf := append(mustJSON(t, resumeRecord{Type: "review_item_done", FilePath: "good.go", Fingerprint: "fp-good"}), '\n') + buf = append(buf, []byte("{bad json}\n")...) + if err := os.WriteFile(path, buf, 0600); err != nil { + t.Fatal(err) + } + + if _, err := LoadResumeState(repoDir, sessionID); err == nil { + t.Fatal("the strict loader must report an unreadable line rather than drop it") + } +} + +// TestLoadResumeState_IntactSessionStillReusable guards the scan path against the +// regression this split exists to prevent: an undamaged session must keep every +// checkpoint it recorded, with no manifest involved. +func TestLoadResumeState_IntactSessionStillReusable(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + repoDir := "/test/intact-scan" + sessionID := "intact-session" + path, err := SessionFilePath(repoDir, sessionID) + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil { + t.Fatal(err) + } + buf := append(mustJSON(t, resumeRecord{Type: "review_item_done", FilePath: "good.go", Fingerprint: "fp-good"}), '\n') + if err := os.WriteFile(path, buf, 0600); err != nil { + t.Fatal(err) + } + + state, err := LoadResumeState(repoDir, sessionID) + if err != nil { + t.Fatal(err) + } + if _, ok := state.Item("fp-good"); !ok { + t.Error("an intact checkpoint must stay reusable without a manifest") } } diff --git a/pages/src/content/docs/en/cli-reference.md b/pages/src/content/docs/en/cli-reference.md index 726c6397..53fbd2b3 100644 --- a/pages/src/content/docs/en/cli-reference.md +++ b/pages/src/content/docs/en/cli-reference.md @@ -183,13 +183,27 @@ ocr review --from main --to feature-branch --resume ocr review --commit abc123 --resume ``` -Resume is strict by design: +Resume is strict by design. Checkpoints are only reused when the resumed run +would review the same thing the parent did: - workspace reviews cannot be resumed -- range reviews must use the same `--from` and `--to` -- commit reviews must use the same `--commit` +- the review mode must match: a range session cannot be resumed as a commit one +- the resolved input must match. Ref *spellings* are not compared — `abc1234` + and `abc1234def` name the same commit — but if the same refs now resolve to a + different diff, or the rules or filters changed the selected file set, the + whole resume is rejected rather than partially reused +- a provider or model change must be asked for explicitly with `--provider` / + `--model`. A change that arrived through config or the environment is rejected +- the parent must carry a run manifest, which is what its input is verified + against. A run killed with Ctrl-C never wrote one, and sessions older than run + manifests never had one +- only files the parent's manifest settled are reused. A checkpoint the manifest + does not account for, or one that is unreadable, costs that file its + checkpoint and nothing more — it is simply reviewed again - `--preview` and `--resume` cannot be used together +A rejected resume writes nothing: no session, no manifest, no LLM call. + ### Output #### Text (default, `--audience human`) @@ -359,6 +373,9 @@ ocr session list --json ### `ocr session show` +A resumed run also prints the run it continued, and the provider/model +transition when the resume crossed one. + ```bash ocr session show ocr session show --json diff --git a/pages/src/content/docs/ja/cli-reference.md b/pages/src/content/docs/ja/cli-reference.md index cc600fcb..8bae6fb5 100644 --- a/pages/src/content/docs/ja/cli-reference.md +++ b/pages/src/content/docs/ja/cli-reference.md @@ -169,13 +169,28 @@ ocr review --from main --to feature-branch --resume ocr review --commit abc123 --resume ``` -再開は意図的に厳密です: +再開は意図的に厳密です。今回の実行が親と同じ対象をレビューする場合にのみ、 +チェックポイントが再利用されます: - ワークスペースレビューは再開できません -- 範囲レビューは同じ `--from` と `--to` が必要です -- 単一 commit レビューは同じ `--commit` が必要です +- レビューモードが一致する必要があります: 範囲セッションを単一 commit として + 再開することはできません +- 解決後の入力が一致する必要があります。ref の*表記*は比較しません + (`abc1234` と `abc1234def` は同じ commit を指します) が、同じ ref が別の + diff に解決される場合、あるいはルールやフィルタが選択ファイル集合を変えた + 場合は、部分的に再利用するのではなく再開全体を拒否します +- provider や model の変更は `--provider` / `--model` で明示的に指定する必要が + あります。設定ファイルや環境変数経由の変更は拒否されます +- 親の実行が run manifest を持っている必要があります。入力はこれと照合して + 検証されます。Ctrl-C で中断された実行は書き出しておらず、run manifest より + 古いセッションはそもそも持っていません +- 再利用されるのは、親の manifest が結果を確定したファイルだけです。manifest が + 裏付けないチェックポイントや読み取れないチェックポイントは、そのファイルが + もう一度レビューされるだけで、他のファイルには影響しません - `--preview` と `--resume` は併用できません +拒否された再開は何も残しません: セッションも manifest も作らず、LLM も呼びません。 + ### 出力 #### Text（デフォルト、`--audience human`） @@ -335,6 +350,9 @@ ocr session list --json ### `ocr session show` +再開した実行では、継続元の実行も表示されます。provider や model をまたいだ +再開の場合は、その切り替えも表示されます。 + ```bash ocr session show ocr session show --json diff --git a/pages/src/content/docs/ru/cli-reference.md b/pages/src/content/docs/ru/cli-reference.md index 0052b0fe..a0b8bbae 100644 --- a/pages/src/content/docs/ru/cli-reference.md +++ b/pages/src/content/docs/ru/cli-reference.md @@ -165,13 +165,29 @@ ocr review --from main --to feature-branch --resume ocr review --commit abc123 --resume ``` -Возобновление намеренно выполняется строго: +Возобновление намеренно выполняется строго. Контрольные точки переиспользуются +только тогда, когда новый запуск проверяет ровно то же, что и родительский: - ревью рабочей области нельзя возобновить; -- для ревью диапазона нужно использовать те же `--from` и `--to`; -- для ревью коммита нужно использовать тот же `--commit`; +- режим ревью должен совпадать: сессию диапазона нельзя возобновить как ревью + коммита; +- разрешённый вход должен совпадать. *Написание* ref не сравнивается (`abc1234` + и `abc1234def` указывают на один коммит), но если те же ref теперь дают другой + diff, либо правила или фильтры изменили набор выбранных файлов, возобновление + отклоняется целиком, а не переиспользуется частично; +- смену provider или model нужно запросить явно через `--provider` / `--model`; + изменение, пришедшее из конфигурации или окружения, отклоняется; +- у родительского запуска должен быть run manifest — именно по нему проверяется + вход. Запуск, прерванный Ctrl-C, его не записал, а сессии старше run manifest + его никогда и не имели; +- переиспользуются только файлы, судьбу которых зафиксировал manifest родителя. + Контрольная точка, которую manifest не подтверждает или которую не удалось + прочитать, стоит этому файлу его контрольной точки и не более — он просто + проверяется заново; - `--preview` и `--resume` нельзя использовать вместе. +Отклонённое возобновление не оставляет ничего: ни сессии, ни manifest, ни вызова LLM. + ### Вывод #### Текст (по умолчанию, `--audience human`) @@ -334,6 +350,9 @@ ocr session list --json ### `ocr session show` +Возобновлённый запуск также печатает запуск, который он продолжил, и переход +provider/model, если возобновление его пересекло. + ```bash ocr session show ocr session show --json diff --git a/pages/src/content/docs/zh/cli-reference.md b/pages/src/content/docs/zh/cli-reference.md index 0f679fc0..5b3e7aac 100644 --- a/pages/src/content/docs/zh/cli-reference.md +++ b/pages/src/content/docs/zh/cli-reference.md @@ -171,13 +171,23 @@ ocr review --from main --to feature-branch --resume ocr review --commit abc123 --resume ``` -恢复逻辑是严格的： +恢复逻辑是严格的。只有当本次运行评审的对象与父运行完全一致时，checkpoint 才会被复用： - 工作区评审不能恢复 -- 区间评审必须使用相同的 `--from` 和 `--to` -- 单 commit 评审必须使用相同的 `--commit` +- 评审模式必须一致：区间会话不能以单 commit 模式恢复 +- 解析后的输入必须一致。ref 的*写法*不参与比较（`abc1234` 与 `abc1234def` + 指向同一个 commit），但如果相同的 ref 现在解析到不同的 diff，或规则、过滤器 + 改变了选中的文件集合，整次恢复会被拒绝，而不是部分复用 +- 切换 provider 或 model 必须通过 `--provider` / `--model` 显式声明；经由配置 + 文件或环境变量发生的变化一律拒绝 +- 父运行必须带有 run manifest，输入正是拿它来校验的。被 Ctrl-C 终止的运行没写出 + manifest，早于 run manifest 的老 session 则从来就没有 +- 只有父 manifest 认领过的文件才会复用。manifest 未认领或已损坏的 checkpoint 只 + 影响它自己那个文件——该文件重新评审一次，其余不受影响 - `--preview` 和 `--resume` 不能同时使用 +被拒绝的恢复不会留下任何产物：不创建 session、不写 manifest、不调用 LLM。 + ### 输出 #### Text（默认，`--audience human`） @@ -343,6 +353,8 @@ ocr session list --json ### `ocr session show` +恢复的运行还会打印它所继续的父运行，若这次恢复跨了 provider 或 model，也会打印这次切换。 + ```bash ocr session show ocr session show --json From 7d5d0207b8a8ec914e306b41ab14553d8da0d9e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=89=BA=E4=B8=B4?= Date: Tue, 11 Aug 2026 19:29:32 +0800 Subject: [PATCH 2/3] fix(resume): freeze refs before loading identity diff --- internal/agent/identity.go | 46 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/internal/agent/identity.go b/internal/agent/identity.go index 2c125ccd..44d3f19d 100644 --- a/internal/agent/identity.go +++ b/internal/agent/identity.go @@ -45,6 +45,14 @@ type SealedInput struct { func ResolveIdentity(ctx context.Context, args Args) (*SealedInput, error) { defer stdout.Quiet()() + resolution, err := resolveInputBeforeDiff(ctx, args) + if err != nil { + return nil, err + } + if resolution != nil { + args.SealedInput = resolution + } + a := &Agent{args: args} if err := a.loadDiffs(ctx); err != nil { return nil, fmt.Errorf("load diffs: %w", err) @@ -54,6 +62,44 @@ func ResolveIdentity(ctx context.Context, args Args) (*SealedInput, error) { return &SealedInput{Identity: a.runIdentity(), Resolution: a.inputResolution}, nil } +// resolveInputBeforeDiff turns every moving head ref into an immutable commit +// before the diff used for admission is loaded. Range mode then computes its +// merge-base against that frozen head; commit mode needs only the frozen head. +func resolveInputBeforeDiff(ctx context.Context, args Args) (*diff.InputResolution, error) { + switch { + case args.Commit != "": + head, err := resolveCommitHead(ctx, args, args.Commit) + if err != nil { + return nil, err + } + return &diff.InputResolution{ResolvedHead: head}, nil + case args.From != "" && args.To != "": + from, err := resolveCommitHead(ctx, args, args.From) + if err != nil { + return nil, err + } + head, err := resolveCommitHead(ctx, args, args.To) + if err != nil { + return nil, err + } + resolved := diff.NewProvider(args.RepoDir, from, head, args.GitRunner).ResolveInput(ctx) + if resolved.ResolvedBase == "" { + return nil, fmt.Errorf("resolve merge-base between %q and %q", args.From, args.To) + } + return &resolved, nil + default: + return nil, nil + } +} + +func resolveCommitHead(ctx context.Context, args Args, ref string) (string, error) { + head := diff.NewCommitProvider(args.RepoDir, ref, args.GitRunner).ResolveInput(ctx).ResolvedHead + if head == "" { + return "", fmt.Errorf("resolve commit %q", ref) + } + return head, nil +} + // runIdentity reads the identity off the agent's current selection. // // It is only meaningful once the selection is final: sourceArtifactSHA256 hashes From 0413e7bd36a6688cdfda6b83ba8abede57d9321a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=89=BA=E4=B8=B4?= Date: Tue, 11 Aug 2026 20:33:58 +0800 Subject: [PATCH 3/3] test(resume): cover sealed input resolution --- internal/agent/identity_test.go | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/internal/agent/identity_test.go b/internal/agent/identity_test.go index a6cec8fd..60f1d15b 100644 --- a/internal/agent/identity_test.go +++ b/internal/agent/identity_test.go @@ -164,3 +164,36 @@ func TestResolveIdentityTracksConfigChanges(t *testing.T) { } }) } + +func TestResolveInputBeforeDiffCommit(t *testing.T) { + dir := sealRepo(t) + gitIn(t, dir, "remote", "add", "origin", "https://example.com/org/repo.git") + sealed, err := ResolveIdentity(context.Background(), Args{RepoDir: dir, Commit: "HEAD"}) + if err != nil { + t.Fatalf("ResolveIdentity: %v", err) + } + if sealed.Resolution.ResolvedHead == "" { + t.Fatalf("commit resolve must freeze its head, got %+v", sealed.Resolution) + } + if sealed.Identity.RepositorySHA256 == "" { + t.Fatal("resolved identity must include the repository identity") + } +} + +func TestResolveInputBeforeDiffRejectsInvalidRefs(t *testing.T) { + dir := sealRepo(t) + for _, tc := range []struct { + name string + args Args + }{ + {"commit", Args{RepoDir: dir, Commit: "missing"}}, + {"range from", Args{RepoDir: dir, From: "missing", To: "feature"}}, + {"range to", Args{RepoDir: dir, From: "main", To: "missing"}}, + } { + t.Run(tc.name, func(t *testing.T) { + if _, err := ResolveIdentity(context.Background(), tc.args); err == nil { + t.Fatal("unresolvable ref must fail before loading the diff") + } + }) + } +}