diff --git a/internal/gateio/gateio.go b/internal/gateio/gateio.go new file mode 100644 index 0000000..10d1672 --- /dev/null +++ b/internal/gateio/gateio.go @@ -0,0 +1,768 @@ +// Package gateio maps provider/ledger state into the pure gate kernel. +package gateio + +import ( + "context" + "errors" + "fmt" + "io" + "strings" + "time" + + "github.com/open-cli-collective/codereview-cli/internal/gate" + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + "github.com/open-cli-collective/codereview-cli/internal/ledger" + "github.com/open-cli-collective/codereview-cli/internal/marker" + "github.com/open-cli-collective/codereview-cli/internal/runlock" + "github.com/open-cli-collective/codereview-cli/internal/statepaths" +) + +// Status identifies how gate IO resolved the request. +type Status string + +// Result status values. +const ( + StatusContinue Status = "continue" + StatusEarlyExit Status = "early_exit" + StatusDryRunFresh Status = "dry_run_fresh" + StatusRepairUnsupported Status = "repair_unsupported" + StatusRetryPostsUnsupported Status = "retry_posts_unsupported" + StatusError Status = "error" + StatusBaseMovedAbort Status = "base_moved_abort" +) + +// Store is the ledger behavior required by gate IO. +type Store interface { + ListRunsForHeadScope(context.Context, ledger.ListRunsForHeadScopeParams) ([]ledger.Run, error) + GetRun(context.Context, string) (ledger.Run, error) + ListPlannedActions(context.Context, string) ([]ledger.PlannedAction, error) + AllocateRun(context.Context, ledger.AllocateRunParams) (ledger.Run, error) + CompleteRun(context.Context, string, ledger.Outcome, time.Time) error +} + +// Lock is a held run lock. +type Lock interface { + Release() error +} + +// AcquireFunc acquires one non-blocking run lock. +type AcquireFunc func(string) (Lock, error) + +// Options contains gate IO dependencies. +type Options struct { + Store Store + Provider gitprovider.GitProvider + Layout statepaths.Layout + Acquire AcquireFunc + Now func() time.Time + StaleHeartbeatThreshold time.Duration + Warnings io.Writer +} + +// Request identifies one gate evaluation. +type Request struct { + PRRef gitprovider.PRRef + PR gitprovider.PR + PRKey string + Profile string + PostingIdentity gitprovider.Identity + PostingIdentityKey string + Flags gate.Flags + ArtifactPath string +} + +// Result is the outcome of one gate IO evaluation. +type Result struct { + Status Status + Decision gate.Decision + Run ledger.Run + Lock Lock + Warnings []string +} + +type staleProbe struct { + lock Lock +} + +type markerRecord struct { + marker marker.ActionMarker + when time.Time + order int +} + +type staleRun struct { + run ledger.Run +} + +// Evaluate acquires live gate state, calls the pure kernel, and executes +// non-repair decisions. +func Evaluate(ctx context.Context, opts Options, req Request) (Result, error) { + if err := validateOptions(&opts); err != nil { + return Result{}, err + } + if err := validateRequest(req); err != nil { + return Result{}, err + } + if req.Flags.DryRun { + decision := gate.Decide(gate.Request{ + Flags: req.Flags, + PR: gate.PRSummary{State: gate.PRStateFresh}, + }) + if decision.Kind == gate.DecisionError { + return Result{Status: StatusError, Decision: decision}, nil + } + return Result{Status: StatusDryRunFresh, Decision: decision}, nil + } + + lockPath, err := currentLockPath(opts.Layout, req) + if err != nil { + return Result{}, err + } + currentLock, err := opts.acquire(lockPath) + if err != nil { + return Result{}, err + } + releaseCurrent := true + defer func() { + if releaseCurrent { + _ = currentLock.Release() + } + }() + + for attempts := 0; ; attempts++ { + if err := ctx.Err(); err != nil { + return Result{}, err + } + state, err := buildLocalState(ctx, opts, req) + if err != nil { + return Result{}, err + } + if attempts > len(state.staleRuns)+2 { + return Result{}, fmt.Errorf("gateio: stale-base cleanup exceeded retry limit") + } + + decision := gate.Decide(state.kernel) + if !localDecisionApplies(req.Flags, decision) { + state, err = attachExternalState(ctx, opts, req, state) + if err != nil { + return Result{}, err + } + decision = gate.Decide(state.kernel) + } + + result, retry, err := executeDecision(ctx, opts, req, state, decision, currentLock, &releaseCurrent) + if err != nil { + return Result{}, err + } + if retry { + continue + } + return result, nil + } +} + +// AbortIfBaseMoved aborts run when the PR base ref no longer matches run.BaseSHA. +func AbortIfBaseMoved(ctx context.Context, opts Options, req Request, run ledger.Run) (Result, error) { + if err := validateAbortOptions(&opts); err != nil { + return Result{}, err + } + if err := validateRequest(req); err != nil { + return Result{}, err + } + decision, moved, err := baseMovedDecision(ctx, opts, req, run.BaseSHA) + if err != nil { + return Result{}, err + } + if !moved { + return Result{Status: StatusContinue, Run: run}, nil + } + now := opts.now() + if err := opts.Store.CompleteRun(ctx, run.RunID, ledger.OutcomeAborted, now); err != nil { + return Result{}, err + } + return Result{ + Status: StatusBaseMovedAbort, + Run: run, + Decision: decision, + }, nil +} + +func baseMovedDecision(ctx context.Context, opts Options, req Request, baseSHA string) (gate.Decision, bool, error) { + pr, err := opts.Provider.GetPR(ctx, req.PRRef) + if err != nil { + return gate.Decision{}, false, err + } + if pr.Base.SHA == baseSHA { + return gate.Decision{}, false, nil + } + return gate.Decision{ + Kind: gate.DecisionError, + Message: fmt.Sprintf("base moved from %s to %s", baseSHA, pr.Base.SHA), + }, true, nil +} + +type gateState struct { + kernel gate.Request + runByID map[string]ledger.Run + staleRuns []staleRun + staleLocks map[string]staleProbe +} + +func (s gateState) releaseStaleLocks() { + for _, probe := range s.staleLocks { + if probe.lock != nil { + _ = probe.lock.Release() + } + } +} + +func buildLocalState(ctx context.Context, opts Options, req Request) (gateState, error) { + runs, err := opts.Store.ListRunsForHeadScope(ctx, ledger.ListRunsForHeadScopeParams{ + PRKey: req.PRKey, + SHA: req.PR.Head.SHA, + Profile: req.Profile, + PostingIdentity: req.postingKey(), + }) + if err != nil { + return gateState{}, err + } + + state := gateState{ + kernel: gate.Request{Flags: req.Flags, PR: gate.PRSummary{State: gate.PRStateFresh}}, + runByID: make(map[string]ledger.Run, len(runs)), + staleLocks: make(map[string]staleProbe), + } + for _, run := range runs { + state.runByID[run.RunID] = run + if run.BaseSHA != req.PR.Base.SHA { + state.staleRuns = append(state.staleRuns, staleRun{run: run}) + continue + } + summary, err := summarizeRun(ctx, opts.Store, run) + if err != nil { + return gateState{}, err + } + state.kernel.ExactRuns = append(state.kernel.ExactRuns, summary) + } + return state, nil +} + +func attachExternalState(ctx context.Context, opts Options, req Request, state gateState) (gateState, error) { + for _, stale := range state.staleRuns { + summary, err := summarizeRun(ctx, opts.Store, stale.run) + if err != nil { + state.releaseStaleLocks() + return gateState{}, err + } + candidate, probe, err := summarizeStaleCandidate(opts, req, stale.run, summary) + if err != nil { + state.releaseStaleLocks() + return gateState{}, err + } + state.kernel.StaleBaseCandidates = append(state.kernel.StaleBaseCandidates, candidate) + if probe.lock != nil { + state.staleLocks[stale.run.RunID] = probe + } + } + + prSummary, err := summarizePR(ctx, opts.Provider, req) + if err != nil { + state.releaseStaleLocks() + return gateState{}, err + } + state.kernel.PR = prSummary + if prSummary.State == gate.PRStatePartial { + partialRun, err := lookupScopedPartialRun(ctx, opts, req, prSummary.RunID) + if err != nil { + state.releaseStaleLocks() + return gateState{}, err + } + if partialRun != nil { + state.kernel.PartialRun = partialRun + } + } + return state, nil +} + +func localDecisionApplies(flags gate.Flags, decision gate.Decision) bool { + switch decision.Kind { + case gate.DecisionResume, gate.DecisionRetryPosts, gate.DecisionError: + return true + case gate.DecisionFresh: + return flags.Rerun + case gate.DecisionEarlyExit, gate.DecisionRepair, gate.DecisionAbortStale: + return false + default: + return false + } +} + +func executeDecision(ctx context.Context, opts Options, req Request, state gateState, decision gate.Decision, currentLock Lock, releaseCurrent *bool) (Result, bool, error) { + result := Result{Decision: decision, Warnings: append([]string(nil), decision.Warnings...)} + + switch decision.Kind { + case gate.DecisionResume: + state.releaseStaleLocks() + run, ok := state.runByID[decision.RunID] + if !ok { + return Result{}, false, fmt.Errorf("gateio: resume run %q was not loaded", decision.RunID) + } + if baseResult, err := AbortIfBaseMoved(ctx, opts, req, run); err != nil { + return Result{}, false, err + } else if baseResult.Status == StatusBaseMovedAbort { + return baseResult, false, nil + } + *releaseCurrent = false + result.Status = StatusContinue + result.Run = run + result.Lock = currentLock + emitWarnings(opts.Warnings, result.Warnings) + return result, false, nil + case gate.DecisionFresh: + state.releaseStaleLocks() + if baseDecision, moved, err := baseMovedDecision(ctx, opts, req, req.PR.Base.SHA); err != nil { + return Result{}, false, err + } else if moved { + result.Status = StatusBaseMovedAbort + result.Decision = baseDecision + return result, false, nil + } + if err := abortRuns(ctx, opts, decision.SupersedeRunIDs); err != nil { + return Result{}, false, err + } + run, err := allocateFresh(ctx, opts, req) + if err != nil { + return Result{}, false, err + } + *releaseCurrent = false + result.Status = StatusContinue + result.Run = run + result.Lock = currentLock + emitWarnings(opts.Warnings, result.Warnings) + return result, false, nil + case gate.DecisionEarlyExit: + state.releaseStaleLocks() + result.Status = StatusEarlyExit + emitWarnings(opts.Warnings, result.Warnings) + return result, false, nil + case gate.DecisionAbortStale: + if err := abortStaleRuns(ctx, opts, state, decision.AbortStaleRunIDs); err != nil { + state.releaseStaleLocks() + return Result{}, false, err + } + state.releaseStaleLocks() + return Result{}, true, nil + case gate.DecisionRepair: + state.releaseStaleLocks() + result.Status = StatusRepairUnsupported + emitWarnings(opts.Warnings, result.Warnings) + return result, false, nil + case gate.DecisionRetryPosts: + state.releaseStaleLocks() + result.Status = StatusRetryPostsUnsupported + emitWarnings(opts.Warnings, result.Warnings) + return result, false, nil + case gate.DecisionError: + state.releaseStaleLocks() + result.Status = StatusError + emitWarnings(opts.Warnings, result.Warnings) + return result, false, nil + default: + state.releaseStaleLocks() + return Result{}, false, fmt.Errorf("gateio: unsupported gate decision %q", decision.Kind) + } +} + +func summarizeRun(ctx context.Context, store Store, run ledger.Run) (gate.RunSummary, error) { + mode, err := gatePostMode(run.PostMode) + if err != nil { + return gate.RunSummary{}, err + } + state, err := gateRunState(run.Outcome) + if err != nil { + return gate.RunSummary{}, err + } + actions, err := store.ListPlannedActions(ctx, run.RunID) + if err != nil { + return gate.RunSummary{}, err + } + summary := gate.RunSummary{ + RunID: run.RunID, + Attempt: run.Attempt, + PostMode: mode, + State: state, + } + for _, action := range actions { + if !action.Required { + continue + } + switch action.Status { + case ledger.PlannedActionPending: + summary.RequiredPending++ + case ledger.PlannedActionFailedTerminal: + summary.RequiredFailedTerminal++ + if action.FailureClass != nil && *action.FailureClass == ledger.PlannedActionFailureClassAuth { + summary.FailureClass = gate.FailureClassAuth + } else if summary.FailureClass == gate.FailureClassNone { + summary.FailureClass = gate.FailureClassTerminal + } + case ledger.PlannedActionPosted, ledger.PlannedActionSuperseded, ledger.PlannedActionPlannedOnly: + } + } + return summary, nil +} + +func summarizeStaleCandidate(opts Options, req Request, run ledger.Run, summary gate.RunSummary) (gate.StaleBaseCandidate, staleProbe, error) { + lockPath, err := lockPathForRun(opts.Layout, req.PRRef, run) + if err != nil { + return gate.StaleBaseCandidate{}, staleProbe{}, err + } + lock, err := opts.acquire(lockPath) + if err == nil { + return gate.StaleBaseCandidate{Run: summary, LockState: gate.LockStateFree}, staleProbe{lock: lock}, nil + } + if !errors.Is(err, runlock.ErrHeld) { + return gate.StaleBaseCandidate{}, staleProbe{}, err + } + return gate.StaleBaseCandidate{ + Run: summary, + LockState: gate.LockStateHeld, + HeartbeatStale: heartbeatStale(run, opts.now(), opts.StaleHeartbeatThreshold), + }, staleProbe{}, nil +} + +func summarizePR(ctx context.Context, provider gitprovider.GitProvider, req Request) (gate.PRSummary, error) { + comments, err := provider.ListIssueComments(ctx, req.PRRef) + if err != nil { + return gate.PRSummary{}, err + } + reviews, err := provider.ListReviews(ctx, req.PRRef) + if err != nil { + return gate.PRSummary{}, err + } + + records := make([]markerRecord, 0, len(comments)+len(reviews)) + order := 0 + for _, comment := range comments { + if !sameIdentity(comment.Author, req.PostingIdentity) { + continue + } + for _, found := range marker.FindActions(comment.Body) { + records = append(records, markerRecord{marker: found, when: comment.CreatedAt, order: order}) + order++ + } + } + for _, review := range reviews { + if !sameIdentity(review.Author, req.PostingIdentity) { + continue + } + for _, found := range marker.FindActions(review.Body) { + records = append(records, markerRecord{marker: found, when: review.SubmittedAt, order: order}) + order++ + } + } + return classifyMarkers(records, req.PR.Head.SHA, req.PR.Base.SHA), nil +} + +func classifyMarkers(records []markerRecord, headSHA, baseSHA string) gate.PRSummary { + submits := map[string]markerRecord{} + var currentNoDiff *markerRecord + var currentPartial *markerRecord + var stale *markerRecord + + for i := range records { + record := records[i] + found := record.marker + if found.SHA != headSHA { + continue + } + if found.BaseSHA != baseSHA { + stale = newest(stale, &record) + continue + } + if found.Kind == marker.ActionKindSubmitReview { + key := markerKey(found) + submits[key] = record + } + } + for i := range records { + record := records[i] + found := record.marker + if found.SHA != headSHA || found.BaseSHA != baseSHA || found.Kind != marker.ActionKindRollupComment { + continue + } + if found.Outcome == marker.RollupOutcomeNothingToReview { + currentNoDiff = newest(currentNoDiff, &record) + continue + } + if _, ok := submits[markerKey(found)]; ok { + continue + } + currentPartial = newest(currentPartial, &record) + } + if len(submits) > 0 { + if record, ok := newestSubmit(submits); ok { + return gate.PRSummary{State: gate.PRStateCompleteReview, RunID: record.marker.RunID} + } + } + if currentNoDiff != nil { + return gate.PRSummary{State: gate.PRStateCompleteNoDiff, RunID: currentNoDiff.marker.RunID, Outcome: gate.PROutcomeNothingToReview} + } + if currentPartial != nil { + outcome := gate.PROutcome(currentPartial.marker.Outcome) + return gate.PRSummary{State: gate.PRStatePartial, RunID: currentPartial.marker.RunID, Outcome: outcome} + } + if stale != nil { + return gate.PRSummary{State: gate.PRStateStaleBase, RunID: stale.marker.RunID} + } + return gate.PRSummary{State: gate.PRStateFresh} +} + +func lookupScopedPartialRun(ctx context.Context, opts Options, req Request, runID string) (*gate.RunSummary, error) { + run, err := opts.Store.GetRun(ctx, runID) + if errors.Is(err, ledger.ErrNotFound) { + return nil, nil + } + if err != nil { + return nil, err + } + if run.PRKey != req.PRKey || run.SHA != req.PR.Head.SHA || run.BaseSHA != req.PR.Base.SHA || + run.Profile != req.Profile || run.PostingIdentity != req.postingKey() { + return nil, nil + } + summary, err := summarizeRun(ctx, opts.Store, run) + if err != nil { + return nil, err + } + return &summary, nil +} + +func abortStaleRuns(ctx context.Context, opts Options, state gateState, runIDs []string) error { + for _, runID := range runIDs { + probe, ok := state.staleLocks[runID] + if !ok || probe.lock == nil { + return fmt.Errorf("gateio: stale abort target %q was not locked", runID) + } + if err := opts.Store.CompleteRun(ctx, runID, ledger.OutcomeAborted, opts.now()); err != nil { + return err + } + } + return nil +} + +func abortRuns(ctx context.Context, opts Options, runIDs []string) error { + for _, runID := range runIDs { + if err := opts.Store.CompleteRun(ctx, runID, ledger.OutcomeAborted, opts.now()); err != nil { + return err + } + } + return nil +} + +func allocateFresh(ctx context.Context, opts Options, req Request) (ledger.Run, error) { + return opts.Store.AllocateRun(ctx, ledger.AllocateRunParams{ + PRKey: req.PRKey, + PRURL: req.PR.URL, + SHA: req.PR.Head.SHA, + BaseSHA: req.PR.Base.SHA, + Profile: req.Profile, + PostingIdentity: req.postingKey(), + PostMode: ledger.PostModeLive, + StartedAt: opts.now(), + ArtifactPath: req.ArtifactPath, + }) +} + +func currentLockPath(layout statepaths.Layout, req Request) (string, error) { + return layout.LockFile(statepaths.LockSpec{ + Host: req.PRRef.Host, + Owner: req.PRRef.Owner, + Repo: req.PRRef.Repo, + PRNumber: req.PRRef.Number, + HeadSHA: req.PR.Head.SHA, + BaseSHA: req.PR.Base.SHA, + Profile: req.Profile, + PostingIdentity: req.postingKey(), + }) +} + +func lockPathForRun(layout statepaths.Layout, ref gitprovider.PRRef, run ledger.Run) (string, error) { + return layout.LockFile(statepaths.LockSpec{ + Host: ref.Host, + Owner: ref.Owner, + Repo: ref.Repo, + PRNumber: ref.Number, + HeadSHA: run.SHA, + BaseSHA: run.BaseSHA, + Profile: run.Profile, + PostingIdentity: run.PostingIdentity, + }) +} + +func (o *Options) acquire(path string) (Lock, error) { + acquire := o.Acquire + if acquire == nil { + acquire = func(path string) (Lock, error) { return runlock.Acquire(path) } + } + return acquire(path) +} + +func (o Options) now() time.Time { + if o.Now != nil { + return o.Now().UTC() + } + return time.Now().UTC() +} + +func (r Request) postingKey() string { + if strings.TrimSpace(r.PostingIdentityKey) != "" { + return r.PostingIdentityKey + } + if strings.TrimSpace(r.PostingIdentity.Login) != "" { + return r.PostingIdentity.Login + } + return r.PostingIdentity.ID +} + +func validateOptions(opts *Options) error { + if err := validateAbortOptions(opts); err != nil { + return err + } + if strings.TrimSpace(opts.Layout.DataRoot) == "" { + return fmt.Errorf("gateio: layout data root is required") + } + if opts.StaleHeartbeatThreshold <= 0 { + return fmt.Errorf("gateio: stale heartbeat threshold must be positive") + } + return nil +} + +func validateAbortOptions(opts *Options) error { + if opts.Store == nil { + return fmt.Errorf("gateio: store is required") + } + if opts.Provider == nil { + return fmt.Errorf("gateio: provider is required") + } + return nil +} + +func validateRequest(req Request) error { + if err := req.PRRef.Validate(); err != nil { + return err + } + if req.PR.Ref != req.PRRef { + return fmt.Errorf("gateio: PR snapshot ref %+v does not match request ref %+v", req.PR.Ref, req.PRRef) + } + if strings.TrimSpace(req.PRKey) == "" { + return fmt.Errorf("gateio: pr key is required") + } + expectedPRKey, err := statepaths.PRKey(req.PRRef.Host, req.PRRef.Owner, req.PRRef.Repo, req.PRRef.Number) + if err != nil { + return err + } + if req.PRKey != expectedPRKey { + return fmt.Errorf("gateio: pr key %q does not match request ref %q", req.PRKey, expectedPRKey) + } + if strings.TrimSpace(req.Profile) == "" { + return fmt.Errorf("gateio: profile is required") + } + if strings.TrimSpace(req.postingKey()) == "" { + return fmt.Errorf("gateio: posting identity is required") + } + if strings.TrimSpace(req.PR.Head.SHA) == "" || strings.TrimSpace(req.PR.Base.SHA) == "" { + return fmt.Errorf("gateio: PR head and base SHA are required") + } + if !req.Flags.DryRun && strings.TrimSpace(req.ArtifactPath) == "" { + return fmt.Errorf("gateio: artifact path is required") + } + return nil +} + +func gatePostMode(mode ledger.PostMode) (gate.PostMode, error) { + switch mode { + case ledger.PostModeLive: + return gate.PostModeLive, nil + case ledger.PostModeDryRun: + return gate.PostModeDryRun, nil + default: + return "", fmt.Errorf("gateio: unsupported post mode %q", mode) + } +} + +func gateRunState(outcome *ledger.Outcome) (gate.RunState, error) { + if outcome == nil { + return gate.RunStateRunning, nil + } + switch *outcome { + case ledger.OutcomeIncomplete: + return gate.RunStateIncomplete, nil + case ledger.OutcomeApproved: + return gate.RunStateApproved, nil + case ledger.OutcomeRequestChanges: + return gate.RunStateRequestChanges, nil + case ledger.OutcomeComment: + return gate.RunStateComment, nil + case ledger.OutcomeNothingToReview: + return gate.RunStateNothingToReview, nil + case ledger.OutcomeDryRun: + return gate.RunStateDryRun, nil + case ledger.OutcomeAborted: + return gate.RunStateAborted, nil + case ledger.OutcomeFailed: + return gate.RunStateFailed, nil + default: + return "", fmt.Errorf("gateio: unsupported outcome %q", *outcome) + } +} + +func heartbeatStale(run ledger.Run, now time.Time, threshold time.Duration) bool { + last := run.StartedAt + if run.HeartbeatAt != nil { + last = *run.HeartbeatAt + } + return now.Sub(last) > threshold +} + +func sameIdentity(author gitprovider.Identity, target gitprovider.Identity) bool { + if strings.TrimSpace(author.ID) != "" && strings.TrimSpace(target.ID) != "" { + return author.ID == target.ID + } + return strings.TrimSpace(author.Login) != "" && author.Login == target.Login +} + +func markerKey(found marker.ActionMarker) string { + return found.RunID + "\x00" + found.SHA + "\x00" + found.BaseSHA +} + +func newest(current *markerRecord, candidate *markerRecord) *markerRecord { + if current == nil { + selected := *candidate + return &selected + } + if candidate.when.After(current.when) || (candidate.when.Equal(current.when) && candidate.order > current.order) { + selected := *candidate + return &selected + } + return current +} + +func newestSubmit(submits map[string]markerRecord) (markerRecord, bool) { + var selected *markerRecord + for _, record := range submits { + selected = newest(selected, &record) + } + if selected == nil { + return markerRecord{}, false + } + return *selected, true +} + +func emitWarnings(w io.Writer, warnings []string) { + if w == nil { + return + } + for _, warning := range warnings { + fmt.Fprintln(w, warning) + } +} diff --git a/internal/gateio/gateio_test.go b/internal/gateio/gateio_test.go new file mode 100644 index 0000000..f7361ee --- /dev/null +++ b/internal/gateio/gateio_test.go @@ -0,0 +1,893 @@ +package gateio + +import ( + "bytes" + "context" + "errors" + "path/filepath" + "reflect" + "strings" + "testing" + "time" + + "github.com/open-cli-collective/codereview-cli/internal/gate" + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + "github.com/open-cli-collective/codereview-cli/internal/ledger" + "github.com/open-cli-collective/codereview-cli/internal/marker" + "github.com/open-cli-collective/codereview-cli/internal/runlock" + "github.com/open-cli-collective/codereview-cli/internal/statepaths" +) + +const ( + testHeadSHA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + testBaseSHA = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + testOldBase = "cccccccccccccccccccccccccccccccccccccccc" +) + +var testNow = time.Date(2026, 5, 31, 12, 0, 0, 0, time.UTC) + +func TestEvaluateResumesLocalRunBeforePRState(t *testing.T) { + ctx := context.Background() + fixture := newFixture(t) + run := fixture.allocateRun(t, "run-resume", testBaseSHA, ledger.PostModeLive) + + submit := mustRenderAction(t, marker.ActionMarker{ + RunID: "run-complete", + ActionID: "submit-1", + Kind: marker.ActionKindSubmitReview, + SHA: testHeadSHA, + BaseSHA: testBaseSHA, + }) + if err := fixture.provider.SetReviews(fixture.req.PRRef, []gitprovider.Review{{ + ID: gitprovider.ReviewID("review-1"), + Author: fixture.req.PostingIdentity, + Body: submit, + SubmittedAt: testNow, + }}); err != nil { + t.Fatalf("SetReviews: %v", err) + } + + result, err := Evaluate(ctx, fixture.opts(), fixture.req) + if err != nil { + t.Fatalf("Evaluate: %v", err) + } + defer releaseResultLock(t, result) + if result.Status != StatusContinue || result.Decision.Kind != gate.DecisionResume || result.Run.RunID != run.RunID { + t.Fatalf("Evaluate = %#v, want resume of %s", result, run.RunID) + } + if runs := fixture.listRuns(t); len(runs) != 1 { + t.Fatalf("runs after resume = %d, want no fresh allocation", len(runs)) + } +} + +func TestEvaluateLocalResumeSkipsExternalFailures(t *testing.T) { + fixture := newFixture(t) + run := fixture.allocateRun(t, "run-resume", testBaseSHA, ledger.PostModeLive) + stale := fixture.allocateRun(t, "run-stale", testOldBase, ledger.PostModeLive) + stalePath := fixture.lockPathForRun(t, stale) + opts := fixture.opts() + provider := &countingProvider{GitProvider: fixture.provider} + opts.Provider = provider + opts.Store = plannedActionErrorStore{ + Store: fixture.store, + runID: stale.RunID, + err: errors.New("unexpected stale action lookup"), + } + opts.Acquire = func(path string) (Lock, error) { + if path == stalePath { + return nil, errors.New("unexpected stale lock probe") + } + return fixture.locks.acquire(path) + } + fixture.provider.SetError(gitprovider.OperationListIssueComments, gitprovider.WrapError(gitprovider.ErrRetryable, gitprovider.OperationListIssueComments, errors.New("comments unavailable"))) + fixture.provider.SetError(gitprovider.OperationListReviews, gitprovider.WrapError(gitprovider.ErrRetryable, gitprovider.OperationListReviews, errors.New("reviews unavailable"))) + + result, err := Evaluate(context.Background(), opts, fixture.req) + if err != nil { + t.Fatalf("Evaluate: %v", err) + } + defer releaseResultLock(t, result) + if result.Status != StatusContinue || result.Decision.Kind != gate.DecisionResume || result.Run.RunID != run.RunID { + t.Fatalf("Evaluate = %#v, want local resume of %s", result, run.RunID) + } + if provider.issueComments != 0 || provider.reviews != 0 { + t.Fatalf("marker reads = issueComments:%d reviews:%d, want none", provider.issueComments, provider.reviews) + } +} + +func TestEvaluateIncompleteCompletedRunIsResumable(t *testing.T) { + fixture := newFixture(t) + run := fixture.allocateRun(t, "run-incomplete", testBaseSHA, ledger.PostModeLive) + if err := fixture.store.CompleteRun(context.Background(), run.RunID, ledger.OutcomeIncomplete, testNow.Add(-time.Minute)); err != nil { + t.Fatalf("CompleteRun incomplete: %v", err) + } + + result, err := Evaluate(context.Background(), fixture.opts(), fixture.req) + if err != nil { + t.Fatalf("Evaluate: %v", err) + } + defer releaseResultLock(t, result) + if result.Status != StatusContinue || result.Decision.Kind != gate.DecisionResume || result.Run.RunID != run.RunID { + t.Fatalf("Evaluate = %#v, want incomplete resume", result) + } +} + +func TestEvaluatePRMarkerDecisions(t *testing.T) { + tests := []struct { + name string + seed func(*testing.T, *fixture) + wantStatus Status + wantKind gate.DecisionKind + wantRunID string + wantOut gate.PROutcome + wantRuns int + }{ + { + name: "complete submit review exits early", + seed: func(t *testing.T, f *fixture) { + body := mustRenderAction(t, marker.ActionMarker{RunID: "run-submit", ActionID: "submit-1", Kind: marker.ActionKindSubmitReview, SHA: testHeadSHA, BaseSHA: testBaseSHA}) + setReviews(t, f, []gitprovider.Review{{ID: "review-1", Author: f.req.PostingIdentity, Body: body, SubmittedAt: testNow}}) + }, + wantStatus: StatusEarlyExit, + wantKind: gate.DecisionEarlyExit, + wantRunID: "run-submit", + }, + { + name: "complete no-diff rollup exits early", + seed: func(t *testing.T, f *fixture) { + body := mustRenderAction(t, marker.ActionMarker{RunID: "run-nodiff", ActionID: "rollup-1", Kind: marker.ActionKindRollupComment, SHA: testHeadSHA, BaseSHA: testBaseSHA, Outcome: marker.RollupOutcomeNothingToReview}) + setIssueComments(t, f, []gitprovider.IssueComment{{ID: "issue-1", Author: f.req.PostingIdentity, Body: body, CreatedAt: testNow}}) + }, + wantStatus: StatusEarlyExit, + wantKind: gate.DecisionEarlyExit, + wantRunID: "run-nodiff", + wantOut: gate.PROutcomeNothingToReview, + }, + { + name: "partial rollup is repair unsupported", + seed: func(t *testing.T, f *fixture) { + body := mustRenderAction(t, marker.ActionMarker{RunID: "run-partial", ActionID: "rollup-1", Kind: marker.ActionKindRollupComment, SHA: testHeadSHA, BaseSHA: testBaseSHA, Outcome: marker.RollupOutcomeRequestChanges}) + setIssueComments(t, f, []gitprovider.IssueComment{{ID: "issue-1", Author: f.req.PostingIdentity, Body: body, CreatedAt: testNow}}) + }, + wantStatus: StatusRepairUnsupported, + wantKind: gate.DecisionRepair, + wantRunID: "run-partial", + wantOut: gate.PROutcomeRequestChanges, + }, + { + name: "paired rollup and submit review exits early", + seed: func(t *testing.T, f *fixture) { + rollup := mustRenderAction(t, marker.ActionMarker{RunID: "run-paired", ActionID: "rollup-1", Kind: marker.ActionKindRollupComment, SHA: testHeadSHA, BaseSHA: testBaseSHA, Outcome: marker.RollupOutcomeRequestChanges}) + submit := mustRenderAction(t, marker.ActionMarker{RunID: "run-paired", ActionID: "submit-1", Kind: marker.ActionKindSubmitReview, SHA: testHeadSHA, BaseSHA: testBaseSHA}) + setIssueComments(t, f, []gitprovider.IssueComment{{ID: "issue-1", Author: f.req.PostingIdentity, Body: rollup, CreatedAt: testNow}}) + setReviews(t, f, []gitprovider.Review{{ID: "review-1", Author: f.req.PostingIdentity, Body: submit, SubmittedAt: testNow.Add(time.Minute)}}) + }, + wantStatus: StatusEarlyExit, + wantKind: gate.DecisionEarlyExit, + wantRunID: "run-paired", + }, + { + name: "current complete marker beats stale marker", + seed: func(t *testing.T, f *fixture) { + current := mustRenderAction(t, marker.ActionMarker{RunID: "run-current", ActionID: "submit-1", Kind: marker.ActionKindSubmitReview, SHA: testHeadSHA, BaseSHA: testBaseSHA}) + stale := mustRenderAction(t, marker.ActionMarker{RunID: "run-stale-marker", ActionID: "rollup-1", Kind: marker.ActionKindRollupComment, SHA: testHeadSHA, BaseSHA: testOldBase, Outcome: marker.RollupOutcomeComment}) + setIssueComments(t, f, []gitprovider.IssueComment{ + {ID: "issue-current", Author: f.req.PostingIdentity, Body: current, CreatedAt: testNow}, + {ID: "issue-stale", Author: f.req.PostingIdentity, Body: stale, CreatedAt: testNow.Add(time.Minute)}, + }) + }, + wantStatus: StatusEarlyExit, + wantKind: gate.DecisionEarlyExit, + wantRunID: "run-current", + }, + { + name: "stale-base marker allocates fresh", + seed: func(t *testing.T, f *fixture) { + body := mustRenderAction(t, marker.ActionMarker{RunID: "run-stale-marker", ActionID: "rollup-1", Kind: marker.ActionKindRollupComment, SHA: testHeadSHA, BaseSHA: testOldBase, Outcome: marker.RollupOutcomeComment}) + setIssueComments(t, f, []gitprovider.IssueComment{{ID: "issue-1", Author: f.req.PostingIdentity, Body: body, CreatedAt: testNow}}) + }, + wantStatus: StatusContinue, + wantKind: gate.DecisionFresh, + wantRunID: "run-stale-marker", + wantRuns: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fixture := newFixture(t) + tt.seed(t, fixture) + result, err := Evaluate(context.Background(), fixture.opts(), fixture.req) + if err != nil { + t.Fatalf("Evaluate: %v", err) + } + defer releaseResultLock(t, result) + if result.Status != tt.wantStatus || result.Decision.Kind != tt.wantKind || result.Decision.RunID != tt.wantRunID || result.Decision.Outcome != tt.wantOut { + t.Fatalf("Evaluate = %#v, want status=%s kind=%s run=%q outcome=%q", result, tt.wantStatus, tt.wantKind, tt.wantRunID, tt.wantOut) + } + if tt.wantRuns > 0 && len(fixture.listRuns(t)) != tt.wantRuns { + t.Fatalf("runs = %d, want %d", len(fixture.listRuns(t)), tt.wantRuns) + } + }) + } +} + +func TestEvaluateIgnoresForgedOtherAuthorMarkers(t *testing.T) { + fixture := newFixture(t) + body := mustRenderAction(t, marker.ActionMarker{RunID: "forged", ActionID: "submit-1", Kind: marker.ActionKindSubmitReview, SHA: testHeadSHA, BaseSHA: testBaseSHA}) + setReviews(t, fixture, []gitprovider.Review{{ID: "review-forged", Author: gitprovider.Identity{Login: "other", ID: "other-id"}, Body: body, SubmittedAt: testNow}}) + + result, err := Evaluate(context.Background(), fixture.opts(), fixture.req) + if err != nil { + t.Fatalf("Evaluate: %v", err) + } + defer releaseResultLock(t, result) + if result.Status != StatusContinue || result.Decision.Kind != gate.DecisionFresh { + t.Fatalf("Evaluate = %#v, want fresh allocation", result) + } +} + +func TestEvaluatePartialRunScopeMismatchRepairs(t *testing.T) { + fixture := newFixture(t) + fixture.allocateRun(t, "run-partial", testOldBase, ledger.PostModeLive) + body := mustRenderAction(t, marker.ActionMarker{RunID: "run-partial", ActionID: "rollup-1", Kind: marker.ActionKindRollupComment, SHA: testHeadSHA, BaseSHA: testBaseSHA, Outcome: marker.RollupOutcomeApproved}) + setIssueComments(t, fixture, []gitprovider.IssueComment{{ID: "issue-1", Author: fixture.req.PostingIdentity, Body: body, CreatedAt: testNow}}) + + result, err := Evaluate(context.Background(), fixture.opts(), fixture.req) + if err != nil { + t.Fatalf("Evaluate: %v", err) + } + if result.Status != StatusRepairUnsupported || result.Decision.Kind != gate.DecisionRepair { + t.Fatalf("Evaluate = %#v, want repair unsupported for scoped mismatch", result) + } +} + +func TestEvaluateStaleBaseLockAuthority(t *testing.T) { + tests := []struct { + name string + holdOldLock bool + heartbeat *time.Time + wantAborted bool + wantWarning string + }{ + {name: "held fresh heartbeat", holdOldLock: true, heartbeat: timePtr(testNow.Add(-time.Minute))}, + {name: "held stale heartbeat warns", holdOldLock: true, heartbeat: timePtr(testNow.Add(-time.Hour)), wantWarning: "stale-base run run-stale is locked and has a stale heartbeat"}, + {name: "held nil heartbeat uses started at", holdOldLock: true, wantWarning: "stale-base run run-stale is locked and has a stale heartbeat"}, + {name: "free lock aborts", wantAborted: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fixture := newFixture(t) + run := fixture.allocateRun(t, "run-stale", testOldBase, ledger.PostModeLive) + if tt.heartbeat != nil { + setHeartbeat(t, fixture.store, run.RunID, *tt.heartbeat) + } + if tt.holdOldLock { + fixture.locks.hold(t, fixture.lockPathForRun(t, run)) + } + beforeRuns := fixture.listRuns(t) + var warnings bytes.Buffer + opts := fixture.opts() + opts.Warnings = &warnings + + result, err := Evaluate(context.Background(), opts, fixture.req) + if err != nil { + t.Fatalf("Evaluate: %v", err) + } + defer releaseResultLock(t, result) + if result.Status != StatusContinue || result.Decision.Kind != gate.DecisionFresh || result.Run.RunID == "" { + t.Fatalf("Evaluate = %#v, want fresh continuation after stale-base handling", result) + } + if result.Run.RunID == run.RunID || result.Run.BaseSHA != testBaseSHA { + t.Fatalf("fresh run = %#v, want new current-base run", result.Run) + } + afterRuns := fixture.listRuns(t) + if len(afterRuns) != len(beforeRuns)+1 { + t.Fatalf("runs after Evaluate = %d, want %d", len(afterRuns), len(beforeRuns)+1) + } + gotRun, err := fixture.store.GetRun(context.Background(), run.RunID) + if err != nil { + t.Fatalf("GetRun stale: %v", err) + } + gotAborted := gotRun.Outcome != nil && *gotRun.Outcome == ledger.OutcomeAborted + if gotAborted != tt.wantAborted { + t.Fatalf("stale aborted = %v, want %v", gotAborted, tt.wantAborted) + } + if tt.wantWarning != "" && !strings.Contains(warnings.String(), tt.wantWarning) { + t.Fatalf("warnings = %q, want %q", warnings.String(), tt.wantWarning) + } + }) + } +} + +func TestEvaluateRerunSupersedesResumable(t *testing.T) { + fixture := newFixture(t) + old := fixture.allocateRun(t, "run-old", testBaseSHA, ledger.PostModeLive) + stale := fixture.allocateRun(t, "run-stale", testOldBase, ledger.PostModeLive) + stalePath := fixture.lockPathForRun(t, stale) + fixture.req.Flags.Rerun = true + opts := fixture.opts() + provider := &countingProvider{GitProvider: fixture.provider} + opts.Provider = provider + opts.Acquire = func(path string) (Lock, error) { + if path == stalePath { + return nil, errors.New("unexpected stale lock probe") + } + return fixture.locks.acquire(path) + } + fixture.provider.SetError(gitprovider.OperationListIssueComments, gitprovider.WrapError(gitprovider.ErrRetryable, gitprovider.OperationListIssueComments, errors.New("comments unavailable"))) + + result, err := Evaluate(context.Background(), opts, fixture.req) + if err != nil { + t.Fatalf("Evaluate rerun: %v", err) + } + defer releaseResultLock(t, result) + if result.Decision.Kind != gate.DecisionFresh || result.Run.RunID == old.RunID { + t.Fatalf("Evaluate = %#v, want fresh run after rerun", result) + } + gotOld, err := fixture.store.GetRun(context.Background(), old.RunID) + if err != nil { + t.Fatalf("GetRun old: %v", err) + } + if gotOld.Outcome == nil || *gotOld.Outcome != ledger.OutcomeAborted { + t.Fatalf("old outcome = %v, want aborted", gotOld.Outcome) + } + if provider.issueComments != 0 || provider.reviews != 0 { + t.Fatalf("marker reads = issueComments:%d reviews:%d, want none", provider.issueComments, provider.reviews) + } +} + +func TestEvaluateRerunDoesNotMutateWhenBaseRefetchFails(t *testing.T) { + fixture := newFixture(t) + old := fixture.allocateRun(t, "run-old", testBaseSHA, ledger.PostModeLive) + fixture.req.Flags.Rerun = true + fixture.provider.SetError(gitprovider.OperationGetPR, gitprovider.WrapError(gitprovider.ErrRetryable, gitprovider.OperationGetPR, errors.New("pr unavailable"))) + + if _, err := Evaluate(context.Background(), fixture.opts(), fixture.req); err == nil { + t.Fatal("Evaluate rerun GetPR error = nil, want error") + } + gotOld, err := fixture.store.GetRun(context.Background(), old.RunID) + if err != nil { + t.Fatalf("GetRun old: %v", err) + } + if gotOld.Outcome != nil { + t.Fatalf("old outcome = %v, want unchanged running run", gotOld.Outcome) + } + if runs := fixture.listRuns(t); len(runs) != 1 { + t.Fatalf("runs after failed rerun = %d, want no fresh allocation", len(runs)) + } +} + +func TestEvaluateRetryPostsUnsupportedSkipsExternalState(t *testing.T) { + fixture := newFixture(t) + run := fixture.allocateRun(t, "run-retry", testBaseSHA, ledger.PostModeLive) + insertAction(t, fixture.store, plannedAction(run.RunID, "pending-required", ledger.PlannedActionPending, true, nil)) + if err := fixture.store.CompleteRun(context.Background(), run.RunID, ledger.OutcomeFailed, testNow); err != nil { + t.Fatalf("CompleteRun failed: %v", err) + } + stale := fixture.allocateRun(t, "run-stale", testOldBase, ledger.PostModeLive) + stalePath := fixture.lockPathForRun(t, stale) + fixture.req.Flags.RetryPosts = true + opts := fixture.opts() + opts.Acquire = func(path string) (Lock, error) { + if path == stalePath { + return nil, errors.New("unexpected stale lock probe") + } + return fixture.locks.acquire(path) + } + fixture.provider.SetError(gitprovider.OperationListReviews, gitprovider.WrapError(gitprovider.ErrRetryable, gitprovider.OperationListReviews, errors.New("reviews unavailable"))) + + result, err := Evaluate(context.Background(), opts, fixture.req) + if err != nil { + t.Fatalf("Evaluate retry-posts: %v", err) + } + if result.Status != StatusRetryPostsUnsupported || result.Decision.Kind != gate.DecisionRetryPosts || result.Decision.RunID != run.RunID { + t.Fatalf("Evaluate = %#v, want retry-posts unsupported for %s", result, run.RunID) + } +} + +func TestEvaluateDryRunFreshDoesNotAllocate(t *testing.T) { + fixture := newFixture(t) + fixture.req.Flags.DryRun = true + + result, err := Evaluate(context.Background(), fixture.opts(), fixture.req) + if err != nil { + t.Fatalf("Evaluate dry-run: %v", err) + } + if result.Status != StatusDryRunFresh || result.Decision.Kind != gate.DecisionFresh { + t.Fatalf("Evaluate = %#v, want dry-run fresh", result) + } + if runs := fixture.listRuns(t); len(runs) != 0 { + t.Fatalf("runs after dry-run = %d, want no allocation", len(runs)) + } +} + +func TestEvaluateFreshDoesNotAllocateWhenBaseRefetchFails(t *testing.T) { + fixture := newFixture(t) + fixture.provider.SetError(gitprovider.OperationGetPR, gitprovider.WrapError(gitprovider.ErrRetryable, gitprovider.OperationGetPR, errors.New("pr unavailable"))) + + if _, err := Evaluate(context.Background(), fixture.opts(), fixture.req); err == nil { + t.Fatal("Evaluate fresh GetPR error = nil, want error") + } + if runs := fixture.listRuns(t); len(runs) != 0 { + t.Fatalf("runs after failed fresh = %d, want no allocation", len(runs)) + } +} + +func TestEvaluateDryRunSurfacesKernelErrors(t *testing.T) { + fixture := newFixture(t) + fixture.req.Flags.DryRun = true + fixture.req.Flags.Rerun = true + fixture.req.Flags.RetryPosts = true + + result, err := Evaluate(context.Background(), fixture.opts(), fixture.req) + if err != nil { + t.Fatalf("Evaluate dry-run invalid flags: %v", err) + } + if result.Status != StatusError || result.Decision.Kind != gate.DecisionError || result.Decision.ErrorReason != gate.ErrorMutuallyExclusiveFlags { + t.Fatalf("Evaluate = %#v, want status error for mutually exclusive flags", result) + } + if runs := fixture.listRuns(t); len(runs) != 0 { + t.Fatalf("runs after dry-run error = %d, want no allocation", len(runs)) + } +} + +func TestEvaluateRejectsMismatchedRequestIdentity(t *testing.T) { + tests := []struct { + name string + mutate func(*Request) + wantErr string + }{ + { + name: "pr snapshot ref", + mutate: func(req *Request) { + req.PR.Ref.Number++ + }, + wantErr: "PR snapshot ref", + }, + { + name: "pr key", + mutate: func(req *Request) { + req.PRKey = "github_other_repo_22" + }, + wantErr: "pr key", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fixture := newFixture(t) + tt.mutate(&fixture.req) + _, err := Evaluate(context.Background(), fixture.opts(), fixture.req) + if err == nil { + t.Fatal("Evaluate error = nil, want mismatch error") + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("Evaluate error = %q, want substring %q", err, tt.wantErr) + } + }) + } +} + +func TestAbortIfBaseMoved(t *testing.T) { + fixture := newFixture(t) + run := fixture.allocateRun(t, "run-current", testBaseSHA, ledger.PostModeLive) + moved := fixture.req.PR + moved.Base.SHA = testOldBase + if err := fixture.provider.SetPR(fixture.req.PRRef, moved); err != nil { + t.Fatalf("SetPR moved: %v", err) + } + + result, err := AbortIfBaseMoved(context.Background(), fixture.opts(), fixture.req, run) + if err != nil { + t.Fatalf("AbortIfBaseMoved: %v", err) + } + if result.Status != StatusBaseMovedAbort { + t.Fatalf("AbortIfBaseMoved = %#v, want base moved abort", result) + } + got, err := fixture.store.GetRun(context.Background(), run.RunID) + if err != nil { + t.Fatalf("GetRun: %v", err) + } + if got.Outcome == nil || *got.Outcome != ledger.OutcomeAborted { + t.Fatalf("outcome = %v, want aborted", got.Outcome) + } +} + +func TestEvaluateAbortsIfBaseMoved(t *testing.T) { + fixture := newFixture(t) + moved := fixture.req.PR + moved.Base.SHA = testOldBase + if err := fixture.provider.SetPR(fixture.req.PRRef, moved); err != nil { + t.Fatalf("SetPR moved: %v", err) + } + + result, err := Evaluate(context.Background(), fixture.opts(), fixture.req) + if err != nil { + t.Fatalf("Evaluate: %v", err) + } + if result.Status != StatusBaseMovedAbort || result.Decision.Kind != gate.DecisionError { + t.Fatalf("Evaluate = %#v, want base moved abort", result) + } + runs := fixture.listRuns(t) + if len(runs) != 0 { + t.Fatalf("runs = %d, want no allocation after base moved", len(runs)) + } +} + +func TestEvaluateAbortsResumedRunIfBaseMoved(t *testing.T) { + fixture := newFixture(t) + run := fixture.allocateRun(t, "run-current", testBaseSHA, ledger.PostModeLive) + moved := fixture.req.PR + moved.Base.SHA = testOldBase + if err := fixture.provider.SetPR(fixture.req.PRRef, moved); err != nil { + t.Fatalf("SetPR moved: %v", err) + } + + result, err := Evaluate(context.Background(), fixture.opts(), fixture.req) + if err != nil { + t.Fatalf("Evaluate: %v", err) + } + if result.Status != StatusBaseMovedAbort || result.Decision.Kind != gate.DecisionError { + t.Fatalf("Evaluate = %#v, want base moved abort", result) + } + got, err := fixture.store.GetRun(context.Background(), run.RunID) + if err != nil { + t.Fatalf("GetRun: %v", err) + } + if got.Outcome == nil || *got.Outcome != ledger.OutcomeAborted { + t.Fatalf("resumed outcome = %v, want aborted", got.Outcome) + } +} + +func TestAbortIfBaseMovedDoesNotRequireStaleThreshold(t *testing.T) { + fixture := newFixture(t) + run := fixture.allocateRun(t, "run-current", testBaseSHA, ledger.PostModeLive) + opts := fixture.opts() + opts.StaleHeartbeatThreshold = 0 + + result, err := AbortIfBaseMoved(context.Background(), opts, fixture.req, run) + if err != nil { + t.Fatalf("AbortIfBaseMoved: %v", err) + } + if result.Status != StatusContinue { + t.Fatalf("AbortIfBaseMoved = %#v, want continue", result) + } +} + +func TestGateRunStateMapsLedgerOutcomes(t *testing.T) { + tests := []struct { + name string + outcome *ledger.Outcome + want gate.RunState + }{ + {name: "running", want: gate.RunStateRunning}, + {name: "incomplete", outcome: outcomePtr(ledger.OutcomeIncomplete), want: gate.RunStateIncomplete}, + {name: "approved", outcome: outcomePtr(ledger.OutcomeApproved), want: gate.RunStateApproved}, + {name: "request changes", outcome: outcomePtr(ledger.OutcomeRequestChanges), want: gate.RunStateRequestChanges}, + {name: "comment", outcome: outcomePtr(ledger.OutcomeComment), want: gate.RunStateComment}, + {name: "nothing to review", outcome: outcomePtr(ledger.OutcomeNothingToReview), want: gate.RunStateNothingToReview}, + {name: "dry run", outcome: outcomePtr(ledger.OutcomeDryRun), want: gate.RunStateDryRun}, + {name: "aborted", outcome: outcomePtr(ledger.OutcomeAborted), want: gate.RunStateAborted}, + {name: "failed", outcome: outcomePtr(ledger.OutcomeFailed), want: gate.RunStateFailed}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := gateRunState(tt.outcome) + if err != nil { + t.Fatalf("gateRunState: %v", err) + } + if got != tt.want { + t.Fatalf("gateRunState = %q, want %q", got, tt.want) + } + }) + } +} + +func TestSummarizeRunCountsRequiredActions(t *testing.T) { + fixture := newFixture(t) + run := fixture.allocateRun(t, "run-failed", testBaseSHA, ledger.PostModeLive) + insertAction(t, fixture.store, plannedAction(run.RunID, "pending-required", ledger.PlannedActionPending, true, nil)) + insertAction(t, fixture.store, plannedAction(run.RunID, "pending-optional", ledger.PlannedActionPending, false, nil)) + insertAction(t, fixture.store, plannedAction(run.RunID, "failed-auth", ledger.PlannedActionFailedTerminal, true, strPtr(ledger.PlannedActionFailureClassAuth))) + insertAction(t, fixture.store, plannedAction(run.RunID, "failed-optional", ledger.PlannedActionFailedTerminal, false, strPtr(ledger.PlannedActionFailureClassTerminal))) + insertAction(t, fixture.store, plannedAction(run.RunID, "planned-only", ledger.PlannedActionPlannedOnly, true, nil)) + insertAction(t, fixture.store, plannedAction(run.RunID, "superseded", ledger.PlannedActionSuperseded, true, nil)) + if err := fixture.store.CompleteRun(context.Background(), run.RunID, ledger.OutcomeFailed, testNow); err != nil { + t.Fatalf("CompleteRun failed: %v", err) + } + run, err := fixture.store.GetRun(context.Background(), run.RunID) + if err != nil { + t.Fatalf("GetRun: %v", err) + } + + got, err := summarizeRun(context.Background(), fixture.store, run) + if err != nil { + t.Fatalf("summarizeRun: %v", err) + } + want := gate.RunSummary{ + RunID: run.RunID, + Attempt: run.Attempt, + PostMode: gate.PostModeLive, + State: gate.RunStateFailed, + RequiredPending: 1, + RequiredFailedTerminal: 1, + FailureClass: gate.FailureClassAuth, + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("summarizeRun = %#v, want %#v", got, want) + } +} + +func TestEvaluateRejectsNonPositiveStaleThreshold(t *testing.T) { + fixture := newFixture(t) + opts := fixture.opts() + opts.StaleHeartbeatThreshold = 0 + if _, err := Evaluate(context.Background(), opts, fixture.req); err == nil { + t.Fatal("Evaluate threshold 0 error = nil, want error") + } +} + +func TestEvaluateRejectsMissingLayoutDataRoot(t *testing.T) { + fixture := newFixture(t) + opts := fixture.opts() + opts.Layout.DataRoot = "" + if _, err := Evaluate(context.Background(), opts, fixture.req); err == nil { + t.Fatal("Evaluate missing data root error = nil, want error") + } +} + +func TestAbortStaleRunsRequiresLockedTarget(t *testing.T) { + fixture := newFixture(t) + err := abortStaleRuns(context.Background(), fixture.opts(), gateState{staleLocks: map[string]staleProbe{}}, []string{"missing-run"}) + if err == nil { + t.Fatal("abortStaleRuns missing lock error = nil, want error") + } +} + +type fixture struct { + store *ledger.Store + provider *gitprovider.Fake + layout statepaths.Layout + locks *memoryLocks + req Request +} + +func newFixture(t *testing.T) *fixture { + t.Helper() + store, err := ledger.Open(context.Background(), filepath.Join(t.TempDir(), "ledger.db")) + if err != nil { + t.Fatalf("Open ledger: %v", err) + } + t.Cleanup(func() { + if err := store.Close(); err != nil { + t.Fatalf("Close ledger: %v", err) + } + }) + layout := statepaths.NewLayout(filepath.Join(t.TempDir(), "data", statepaths.AppDir), filepath.Join(t.TempDir(), "cache", statepaths.AppDir)) + ref := gitprovider.PRRef{Host: "github", Owner: "open-cli", Repo: "codereview-cli", Number: 22} + prKey, err := statepaths.PRKey(ref.Host, ref.Owner, ref.Repo, ref.Number) + if err != nil { + t.Fatalf("PRKey: %v", err) + } + bot := gitprovider.Identity{Login: "review-bot", ID: "bot-id"} + pr := gitprovider.PR{ + Ref: ref, + URL: "https://example.test/pr/22", + State: gitprovider.PRStateOpen, + Head: gitprovider.PRBranchRef{SHA: testHeadSHA}, + Base: gitprovider.PRBranchRef{SHA: testBaseSHA}, + } + provider := &gitprovider.Fake{} + if err := provider.SetPR(ref, pr); err != nil { + t.Fatalf("SetPR: %v", err) + } + return &fixture{ + store: store, + provider: provider, + layout: layout, + locks: newMemoryLocks(), + req: Request{ + PRRef: ref, + PR: pr, + PRKey: prKey, + Profile: "default", + PostingIdentity: bot, + PostingIdentityKey: bot.Login, + ArtifactPath: "/tmp/fresh-run", + }, + } +} + +func (f *fixture) opts() Options { + return Options{ + Store: f.store, + Provider: f.provider, + Layout: f.layout, + Acquire: f.locks.acquire, + Now: func() time.Time { return testNow }, + StaleHeartbeatThreshold: 10 * time.Minute, + } +} + +func (f *fixture) allocateRun(t *testing.T, runID, baseSHA string, mode ledger.PostMode) ledger.Run { + t.Helper() + run, err := f.store.AllocateRun(context.Background(), ledger.AllocateRunParams{ + PRKey: f.req.PRKey, + PRURL: f.req.PR.URL, + RunID: runID, + SHA: f.req.PR.Head.SHA, + BaseSHA: baseSHA, + Profile: f.req.Profile, + PostingIdentity: f.req.postingKey(), + PostMode: mode, + StartedAt: testNow.Add(-time.Hour), + ArtifactPath: "/tmp/" + runID, + }) + if err != nil { + t.Fatalf("AllocateRun(%s): %v", runID, err) + } + return run +} + +func (f *fixture) listRuns(t *testing.T) []ledger.Run { + t.Helper() + runs, err := f.store.ListRunsForHeadScope(context.Background(), ledger.ListRunsForHeadScopeParams{ + PRKey: f.req.PRKey, + SHA: f.req.PR.Head.SHA, + Profile: f.req.Profile, + PostingIdentity: f.req.postingKey(), + }) + if err != nil { + t.Fatalf("ListRunsForHeadScope: %v", err) + } + return runs +} + +func (f *fixture) lockPathForRun(t *testing.T, run ledger.Run) string { + t.Helper() + path, err := lockPathForRun(f.layout, f.req.PRRef, run) + if err != nil { + t.Fatalf("lockPathForRun: %v", err) + } + return path +} + +type memoryLocks struct { + held map[string]bool +} + +type memoryLock struct { + path string + locks *memoryLocks +} + +type countingProvider struct { + gitprovider.GitProvider + issueComments int + reviews int +} + +type plannedActionErrorStore struct { + Store + runID string + err error +} + +func (s plannedActionErrorStore) ListPlannedActions(ctx context.Context, runID string) ([]ledger.PlannedAction, error) { + if runID == s.runID { + return nil, s.err + } + return s.Store.ListPlannedActions(ctx, runID) +} + +func (p *countingProvider) ListIssueComments(ctx context.Context, ref gitprovider.PRRef) ([]gitprovider.IssueComment, error) { + p.issueComments++ + return p.GitProvider.ListIssueComments(ctx, ref) +} + +func (p *countingProvider) ListReviews(ctx context.Context, ref gitprovider.PRRef) ([]gitprovider.Review, error) { + p.reviews++ + return p.GitProvider.ListReviews(ctx, ref) +} + +func newMemoryLocks() *memoryLocks { + return &memoryLocks{held: map[string]bool{}} +} + +func (m *memoryLocks) acquire(path string) (Lock, error) { + if m.held[path] { + return nil, runlock.ErrHeld + } + m.held[path] = true + return memoryLock{path: path, locks: m}, nil +} + +func (m *memoryLocks) hold(t *testing.T, path string) { + t.Helper() + lock, err := m.acquire(path) + if err != nil { + t.Fatalf("hold lock: %v", err) + } + t.Cleanup(func() { + _ = lock.Release() + }) +} + +func (l memoryLock) Release() error { + if !l.locks.held[l.path] { + return nil + } + delete(l.locks.held, l.path) + return nil +} + +func setReviews(t *testing.T, f *fixture, reviews []gitprovider.Review) { + t.Helper() + if err := f.provider.SetReviews(f.req.PRRef, reviews); err != nil { + t.Fatalf("SetReviews: %v", err) + } +} + +func setIssueComments(t *testing.T, f *fixture, comments []gitprovider.IssueComment) { + t.Helper() + if err := f.provider.SetIssueComments(f.req.PRRef, comments); err != nil { + t.Fatalf("SetIssueComments: %v", err) + } +} + +func mustRenderAction(t *testing.T, action marker.ActionMarker) string { + t.Helper() + body, err := marker.RenderAction(action) + if err != nil { + t.Fatalf("RenderAction: %v", err) + } + return body +} + +func releaseResultLock(t *testing.T, result Result) { + t.Helper() + if result.Lock != nil { + if err := result.Lock.Release(); err != nil { + t.Fatalf("Release result lock: %v", err) + } + } +} + +func setHeartbeat(t *testing.T, store *ledger.Store, runID string, heartbeat time.Time) { + t.Helper() + if err := store.UpdateHeartbeat(context.Background(), runID, heartbeat); err != nil { + t.Fatalf("UpdateHeartbeat: %v", err) + } +} + +func insertAction(t *testing.T, store *ledger.Store, action ledger.PlannedAction) { + t.Helper() + if err := store.InsertPlannedAction(context.Background(), action); err != nil { + t.Fatalf("InsertPlannedAction: %v", err) + } +} + +func plannedAction(runID, actionID string, status ledger.PlannedActionStatus, required bool, failureClass *string) ledger.PlannedAction { + return ledger.PlannedAction{ + ActionID: actionID, + RunID: runID, + Kind: ledger.PlannedActionRollupComment, + PlannedAt: testNow, + PayloadJSON: "{}", + Status: status, + Required: required, + FailureClass: failureClass, + } +} + +func timePtr(value time.Time) *time.Time { + return &value +} + +func outcomePtr(value ledger.Outcome) *ledger.Outcome { + return &value +} + +func strPtr(value string) *string { + return &value +} diff --git a/internal/ledger/ledger.go b/internal/ledger/ledger.go index 24bf8c6..2efe1ce 100644 --- a/internal/ledger/ledger.go +++ b/internal/ledger/ledger.go @@ -264,6 +264,14 @@ type AllocateRunParams struct { ArtifactPath string } +// ListRunsForHeadScopeParams identifies local rows for one PR/head/profile/identity scope. +type ListRunsForHeadScopeParams struct { + PRKey string + SHA string + Profile string + PostingIdentity string +} + // Session records LLM session usage for a run. type Session struct { SessionRowID string @@ -647,6 +655,42 @@ FROM runs WHERE run_id = ?`, runID) return run, nil } +// ListRunsForHeadScope lists runs for one PR/head/profile/identity across bases. +func (s *Store) ListRunsForHeadScope(ctx context.Context, params ListRunsForHeadScopeParams) ([]Run, error) { + if err := validateListRunsForHeadScopeParams(params); err != nil { + return nil, err + } + if err := s.checkOpen(); err != nil { + return nil, err + } + rows, err := s.db.QueryContext(ctx, ` +SELECT run_id, pr_key, sha, base_sha, attempt, profile, posting_identity, post_mode, + started_at, heartbeat_at, completed_at, outcome, artifact_path, + blocking_count, major_count, minor_count, nits_count +FROM runs +WHERE pr_key = ? AND sha = ? AND profile = ? AND posting_identity = ? +ORDER BY base_sha, attempt DESC, started_at DESC, run_id`, + params.PRKey, params.SHA, params.Profile, params.PostingIdentity, + ) + if err != nil { + return nil, fmt.Errorf("ledger: list runs for head scope: %w", err) + } + defer rows.Close() + + var runs []Run + for rows.Next() { + run, err := scanRun(rows) + if err != nil { + return nil, err + } + runs = append(runs, run) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("ledger: list runs for head scope rows: %w", err) + } + return runs, nil +} + // DeleteRun deletes a run and lets SQLite cascade child rows. func (s *Store) DeleteRun(ctx context.Context, runID string) error { if strings.TrimSpace(runID) == "" { @@ -878,6 +922,35 @@ WHERE run_id = ?`, }) } +// UpdateHeartbeat records the latest liveness timestamp for a run. +func (s *Store) UpdateHeartbeat(ctx context.Context, runID string, heartbeatAt time.Time) error { + if strings.TrimSpace(runID) == "" { + return invalidInput("run_id", runID) + } + if heartbeatAt.IsZero() { + return invalidInput("heartbeat_at", "") + } + return s.write(ctx, func(ctx context.Context, db *sql.DB) error { + result, err := db.ExecContext(ctx, ` +UPDATE runs +SET heartbeat_at = ? +WHERE run_id = ?`, + encodeTime(heartbeatAt), runID, + ) + if err != nil { + return fmt.Errorf("ledger: update heartbeat: %w", err) + } + affected, err := result.RowsAffected() + if err != nil { + return fmt.Errorf("ledger: update heartbeat rows affected: %w", err) + } + if affected == 0 { + return ErrNotFound + } + return nil + }) +} + // UpsertNamedSession inserts or updates a named provider session row. func (s *Store) UpsertNamedSession(ctx context.Context, session NamedSession) error { if err := validateNamedSession(session); err != nil { @@ -927,6 +1000,21 @@ FROM named_sessions WHERE name = ?`, name) return session, nil } +func validateListRunsForHeadScopeParams(params ListRunsForHeadScopeParams) error { + required := map[string]string{ + "pr_key": params.PRKey, + "sha": params.SHA, + "profile": params.Profile, + "posting_identity": params.PostingIdentity, + } + for name, value := range required { + if strings.TrimSpace(value) == "" { + return invalidInput(name, value) + } + } + return nil +} + func validateAllocateRunParams(params AllocateRunParams) error { for field, value := range map[string]string{ "pr_key": params.PRKey, diff --git a/internal/ledger/ledger_test.go b/internal/ledger/ledger_test.go index 0d79571..6d3904c 100644 --- a/internal/ledger/ledger_test.go +++ b/internal/ledger/ledger_test.go @@ -217,6 +217,97 @@ func TestAllocateRunSeparatesAttemptSequencesByResumeKey(t *testing.T) { } } +func TestListRunsForHeadScope(t *testing.T) { + store := openStore(t) + base := validAllocateRunParams() + first := allocateRun(t, store, base) + + secondParams := base + secondParams.RunID = "" + secondParams.StartedAt = secondParams.StartedAt.Add(time.Minute) + secondParams.ArtifactPath = "/tmp/run-2" + second := allocateRun(t, store, secondParams) + + staleParams := base + staleParams.RunID = "run-stale" + staleParams.BaseSHA = "cccccccccccccccccccccccccccccccccccccccc" + staleParams.ArtifactPath = "/tmp/run-stale" + stale := allocateRun(t, store, staleParams) + + for _, mutate := range []func(*AllocateRunParams){ + func(p *AllocateRunParams) { + p.RunID = "run-other-head" + p.SHA = "dddddddddddddddddddddddddddddddddddddddd" + }, + func(p *AllocateRunParams) { p.RunID = "run-other-profile"; p.Profile = "other" }, + func(p *AllocateRunParams) { p.RunID = "run-other-identity"; p.PostingIdentity = "other@example.com" }, + } { + params := base + params.ArtifactPath = "/tmp/" + params.RunID + mutate(¶ms) + allocateRun(t, store, params) + } + + got, err := store.ListRunsForHeadScope(context.Background(), ListRunsForHeadScopeParams{ + PRKey: base.PRKey, + SHA: base.SHA, + Profile: base.Profile, + PostingIdentity: base.PostingIdentity, + }) + if err != nil { + t.Fatalf("ListRunsForHeadScope: %v", err) + } + wantIDs := []string{second.RunID, first.RunID, stale.RunID} + if gotIDs := runIDs(got); !reflect.DeepEqual(gotIDs, wantIDs) { + t.Fatalf("ListRunsForHeadScope IDs = %#v, want %#v", gotIDs, wantIDs) + } +} + +func TestListRunsForHeadScopeValidation(t *testing.T) { + store := openStore(t) + valid := ListRunsForHeadScopeParams{ + PRKey: "github_open-cli_codereview-cli_36", + SHA: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + Profile: "default", + PostingIdentity: "reviewer@example.com", + } + tests := []struct { + name string + mutate func(*ListRunsForHeadScopeParams) + }{ + {name: "pr key", mutate: func(p *ListRunsForHeadScopeParams) { p.PRKey = "" }}, + {name: "sha", mutate: func(p *ListRunsForHeadScopeParams) { p.SHA = "" }}, + {name: "profile", mutate: func(p *ListRunsForHeadScopeParams) { p.Profile = "" }}, + {name: "posting identity", mutate: func(p *ListRunsForHeadScopeParams) { p.PostingIdentity = "" }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + params := valid + tt.mutate(¶ms) + if _, err := store.ListRunsForHeadScope(context.Background(), params); !errors.Is(err, ErrInvalidInput) { + t.Fatalf("ListRunsForHeadScope error = %v, want ErrInvalidInput", err) + } + }) + } +} + +func TestUpdateHeartbeat(t *testing.T) { + store := openStore(t) + run := allocateRun(t, store, validAllocateRunParams()) + heartbeat := time.Date(2026, 5, 30, 12, 2, 0, 0, time.UTC) + + if err := store.UpdateHeartbeat(context.Background(), run.RunID, heartbeat); err != nil { + t.Fatalf("UpdateHeartbeat: %v", err) + } + got, err := store.GetRun(context.Background(), run.RunID) + if err != nil { + t.Fatalf("GetRun: %v", err) + } + if got.HeartbeatAt == nil || !got.HeartbeatAt.Equal(heartbeat) { + t.Fatalf("HeartbeatAt = %v, want %v", got.HeartbeatAt, heartbeat) + } +} + func TestAllocateRunConcurrentSameKey(t *testing.T) { store := openStore(t) ctx := context.Background() @@ -1108,6 +1199,14 @@ func allocateRun(t *testing.T, store *Store, params AllocateRunParams) Run { return run } +func runIDs(runs []Run) []string { + ids := make([]string, 0, len(runs)) + for _, run := range runs { + ids = append(ids, run.RunID) + } + return ids +} + func validSession(runID string) Session { completed := time.Date(2026, 5, 30, 12, 3, 0, 0, time.UTC) duration := int64(1200) diff --git a/internal/statepaths/statepaths.go b/internal/statepaths/statepaths.go index 05425e4..876eb26 100644 --- a/internal/statepaths/statepaths.go +++ b/internal/statepaths/statepaths.go @@ -42,6 +42,18 @@ type RunSpec struct { Attempt int } +// LockSpec identifies one review resume key's lock file. +type LockSpec struct { + Host string + Owner string + Repo string + PRNumber int + HeadSHA string + BaseSHA string + Profile string + PostingIdentity string +} + // RunPaths contains all per-run artifact paths. type RunPaths struct { Dir string @@ -116,10 +128,21 @@ func (l Layout) Run(spec RunSpec) (RunPaths, error) { if err != nil { return RunPaths{}, err } + lockFile, err := l.LockFile(LockSpec{ + Host: spec.Host, + Owner: spec.Owner, + Repo: spec.Repo, + PRNumber: spec.PRNumber, + HeadSHA: spec.HeadSHA, + BaseSHA: spec.BaseSHA, + Profile: spec.Profile, + PostingIdentity: spec.PostingIdentity, + }) + if err != nil { + return RunPaths{}, err + } runDir := filepath.Join(l.DataRoot, "runs", prKey, spec.HeadSHA, spec.BaseSHA, scope, attempt) - keyHash := KeyHash(prKey, spec.HeadSHA, spec.BaseSHA, spec.Profile, spec.PostingIdentity) - lockFile := filepath.Join(l.DataRoot, "locks", prKey+"__"+spec.HeadSHA[:7]+"__"+keyHash+".lock") return RunPaths{ Dir: runDir, @@ -132,6 +155,25 @@ func (l Layout) Run(spec RunSpec) (RunPaths, error) { }, nil } +// LockFile returns the advisory run-lock path for spec's resume key. +func (l Layout) LockFile(spec LockSpec) (string, error) { + prKey, err := PRKey(spec.Host, spec.Owner, spec.Repo, spec.PRNumber) + if err != nil { + return "", err + } + if err := validateSHA("head SHA", spec.HeadSHA); err != nil { + return "", err + } + if err := validateSHA("base SHA", spec.BaseSHA); err != nil { + return "", err + } + if _, err := ResumeScope(spec.Profile, spec.PostingIdentity); err != nil { + return "", err + } + keyHash := KeyHash(prKey, spec.HeadSHA, spec.BaseSHA, spec.Profile, spec.PostingIdentity) + return filepath.Join(l.DataRoot, "locks", prKey+"__"+spec.HeadSHA[:7]+"__"+keyHash+".lock"), nil +} + // SlicePatch returns the artifact path for an agent/file diff slice. func (p RunPaths) SlicePatch(agentID, filePath string) (string, error) { if err := requireNonEmpty("agent ID", agentID); err != nil { diff --git a/internal/statepaths/statepaths_test.go b/internal/statepaths/statepaths_test.go index 5500dfc..3820a65 100644 --- a/internal/statepaths/statepaths_test.go +++ b/internal/statepaths/statepaths_test.go @@ -220,6 +220,22 @@ func TestRunPaths(t *testing.T) { if paths.LockFile != filepath.Join("data", AppDir, "locks", prKey+"__aaaaaaa__62574572babb.lock") { t.Fatalf("LockFile = %q", paths.LockFile) } + lockFile, err := layout.LockFile(LockSpec{ + Host: spec.Host, + Owner: spec.Owner, + Repo: spec.Repo, + PRNumber: spec.PRNumber, + HeadSHA: spec.HeadSHA, + BaseSHA: spec.BaseSHA, + Profile: spec.Profile, + PostingIdentity: spec.PostingIdentity, + }) + if err != nil { + t.Fatalf("LockFile: %v", err) + } + if lockFile != paths.LockFile { + t.Fatalf("LockFile() = %q, want Run().LockFile %q", lockFile, paths.LockFile) + } if got := layout.HTTPCacheDir(); got != filepath.Join("cache", AppDir, "http") { t.Fatalf("HTTPCacheDir = %q", got) } @@ -287,6 +303,49 @@ func TestRunSpecValidation(t *testing.T) { } } +func TestLockSpecValidation(t *testing.T) { + layout := NewLayout("data", "cache") + valid := LockSpec{ + Host: "github", + Owner: "open-cli", + Repo: "codereview-cli", + PRNumber: 34, + HeadSHA: headSHA, + BaseSHA: baseSHA, + Profile: "work", + PostingIdentity: "reviewer", + } + + tests := []struct { + name string + mutate func(*LockSpec) + wantErr string + }{ + {name: "empty host", mutate: func(s *LockSpec) { s.Host = "" }, wantErr: "host"}, + {name: "empty owner", mutate: func(s *LockSpec) { s.Owner = "" }, wantErr: "owner"}, + {name: "empty repo", mutate: func(s *LockSpec) { s.Repo = "" }, wantErr: "repo"}, + {name: "empty profile", mutate: func(s *LockSpec) { s.Profile = "" }, wantErr: "profile"}, + {name: "empty identity", mutate: func(s *LockSpec) { s.PostingIdentity = "" }, wantErr: "posting identity"}, + {name: "bad pr number", mutate: func(s *LockSpec) { s.PRNumber = 0 }, wantErr: "PR number"}, + {name: "short head sha", mutate: func(s *LockSpec) { s.HeadSHA = strings.Repeat("a", 39) }, wantErr: "head SHA"}, + {name: "uppercase base sha", mutate: func(s *LockSpec) { s.BaseSHA = strings.Repeat("B", 40) }, wantErr: "base SHA"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + spec := valid + tt.mutate(&spec) + _, err := layout.LockFile(spec) + if err == nil { + t.Fatalf("LockFile() error = nil, want substring %q", tt.wantErr) + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("LockFile() error = %q, want substring %q", err, tt.wantErr) + } + }) + } +} + func TestRootsAreHermeticAndEnsured(t *testing.T) { root := statedirtest.Hermetic(t)