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..44d3f19d --- /dev/null +++ b/internal/agent/identity.go @@ -0,0 +1,120 @@ +// 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()() + + 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) + } + a.diffs = a.filterDiffs(a.diffs) + a.diffs = a.filterLargeDiffs(a.diffs) + 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 +// 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..60f1d15b --- /dev/null +++ b/internal/agent/identity_test.go @@ -0,0 +1,199 @@ +// 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") + } + }) +} + +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") + } + }) + } +} 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