diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 3a1a2974..09f5ef3c 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -29,6 +29,7 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/llmlifecycle" "github.com/open-cli-collective/codereview-cli/internal/marker" "github.com/open-cli-collective/codereview-cli/internal/plannedactions" + "github.com/open-cli-collective/codereview-cli/internal/prref" "github.com/open-cli-collective/codereview-cli/internal/reporoot" "github.com/open-cli-collective/codereview-cli/internal/review" "github.com/open-cli-collective/codereview-cli/internal/reviewplan" @@ -1158,8 +1159,8 @@ func TestDryRunWithPinnedReviewSHAsUsesCompareDiffAndPinnedFileRefs(t *testing.T result.ReviewBaseSHA != reviewBaseSHA || result.ReviewHeadSHA != reviewHeadSHA { t.Fatalf("result SHAs = current %s/%s review %s/%s", result.CurrentBaseSHA, result.CurrentHeadSHA, result.ReviewBaseSHA, result.ReviewHeadSHA) } - if !strings.Contains(result.Artifacts.Dir, reviewHeadSHA) || !strings.Contains(result.Artifacts.Dir, reviewBaseSHA) { - t.Fatalf("artifact dir = %s, want pinned head/base SHAs", result.Artifacts.Dir) + if !strings.Contains(result.Artifacts.Dir, prref.ShortSHA(reviewHeadSHA)) || !strings.Contains(result.Artifacts.Dir, prref.ShortSHA(reviewBaseSHA)) { + t.Fatalf("artifact dir = %s, want pinned short head/base SHAs", result.Artifacts.Dir) } for _, call := range provider.fileCalls { if call.path == "main.go" && (call.gitRef == reviewBaseSHA || call.gitRef == reviewHeadSHA) { diff --git a/internal/runartifact/runartifact.go b/internal/runartifact/runartifact.go index 5e24ad72..0de8de63 100644 --- a/internal/runartifact/runartifact.go +++ b/internal/runartifact/runartifact.go @@ -12,6 +12,7 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/fsatomic" "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + "github.com/open-cli-collective/codereview-cli/internal/prref" "github.com/open-cli-collective/codereview-cli/internal/statepaths" ) @@ -56,11 +57,11 @@ func ForRun(layout statepaths.Layout, ref gitprovider.PRRef, pr gitprovider.PR, if err != nil { return Paths{}, err } - scope, err := statepaths.ResumeScope(profile, postingIdentity) - if err != nil { + if _, err := statepaths.ResumeScope(profile, postingIdentity); err != nil { return Paths{}, err } - dir := filepath.Join(layout.DataRoot, "runs", prKey, pr.Head.SHA, pr.Base.SHA, scope, "run-"+statepaths.Encode(runID)) + scopeHash := statepaths.KeyHash(prKey, pr.Head.SHA, pr.Base.SHA, profile, postingIdentity) + dir := filepath.Join(layout.DataRoot, "runs", prKey, prref.ShortSHA(pr.Head.SHA), prref.ShortSHA(pr.Base.SHA), scopeHash, "run-"+statepaths.Encode(runID)) return FromDir(dir), nil } diff --git a/internal/runartifact/runartifact_test.go b/internal/runartifact/runartifact_test.go index 176d6fa6..84172fe0 100644 --- a/internal/runartifact/runartifact_test.go +++ b/internal/runartifact/runartifact_test.go @@ -4,9 +4,110 @@ import ( "errors" "os" "path/filepath" + "strings" "testing" + + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + "github.com/open-cli-collective/codereview-cli/internal/prref" + "github.com/open-cli-collective/codereview-cli/internal/statepaths" ) +func TestForRunUsesCompactPath(t *testing.T) { + const ( + headSHA = "0123456789abcdef0123456789abcdef01234567" + baseSHA = "fedcba9876543210fedcba9876543210fedcba98" + profile = "signalft-reviewer-profile" + postingIdentity = "signalft-reviewer-bot" + agentID = "frontend:react-correctness" + runID = "123e4567-e89b-12d3-a456-426614174000" + ) + ref := gitprovider.PRRef{Host: "github.com", Owner: "SignalFT", Repo: "signal-adminapp-frontend", Number: 123} + pr := gitprovider.PR{ + Ref: ref, + Head: gitprovider.PRBranchRef{SHA: headSHA}, + Base: gitprovider.PRBranchRef{SHA: baseSHA}, + } + layout := statepaths.NewLayout(filepath.FromSlash("C:/Users/Konstantin/AppData/Local/cr/data"), "") + + paths, err := ForRun(layout, ref, pr, profile, postingIdentity, runID) + if err != nil { + t.Fatalf("ForRun: %v", err) + } + prKey, err := statepaths.PRKey(ref.Host, ref.Owner, ref.Repo, ref.Number) + if err != nil { + t.Fatalf("PRKey: %v", err) + } + scope, err := statepaths.ResumeScope(profile, postingIdentity) + if err != nil { + t.Fatalf("ResumeScope: %v", err) + } + wantScopeHash := statepaths.KeyHash(prKey, headSHA, baseSHA, profile, postingIdentity) + runsRoot := filepath.Join(layout.DataRoot, "runs") + components := func(dir string) []string { + rel, err := filepath.Rel(runsRoot, dir) + if err != nil { + t.Fatalf("Rel(%q): %v", dir, err) + } + return strings.Split(rel, string(filepath.Separator)) + } + gotComponents := components(paths.Dir) + if len(gotComponents) != 5 { + t.Fatalf("path components below runs = %d, want 5: %q", len(gotComponents), paths.Dir) + } + if gotComponents[0] != prKey { + t.Fatalf("PR key component = %q, want readable key %q", gotComponents[0], prKey) + } + if gotComponents[1] != prref.ShortSHA(headSHA) || len(gotComponents[1]) != 12 { + t.Fatalf("head component = %q, want 12-char %q", gotComponents[1], prref.ShortSHA(headSHA)) + } + if gotComponents[2] != prref.ShortSHA(baseSHA) || len(gotComponents[2]) != 12 { + t.Fatalf("base component = %q, want 12-char %q", gotComponents[2], prref.ShortSHA(baseSHA)) + } + if gotComponents[3] != wantScopeHash || len(gotComponents[3]) != 12 { + t.Fatalf("scope component = %q, want 12-char tuple hash %q", gotComponents[3], wantScopeHash) + } + if gotComponents[4] != "run-"+runID { + t.Fatalf("run component = %q, want full UUID %q", gotComponents[4], "run-"+runID) + } + + legacyDir := filepath.Join(layout.DataRoot, "runs", prKey, headSHA, baseSHA, scope, "run-"+statepaths.Encode(runID)) + legacyReviewerRepo := filepath.Join(legacyDir, "workbench", "reviewers", statepaths.Encode(agentID), "repo") + compactReviewerRepo := filepath.Join(paths.WorkbenchDir, "reviewers", statepaths.Encode(agentID), "repo") + if len(legacyReviewerRepo) <= 260 || len(compactReviewerRepo) >= 260 { + t.Fatalf("reviewer repo path lengths = legacy %d, compact %d; want legacy > 260 and compact < 260", len(legacyReviewerRepo), len(compactReviewerRepo)) + } + + otherProfile, err := ForRun(layout, ref, pr, "other-profile", postingIdentity, runID) + if err != nil { + t.Fatalf("ForRun with other profile: %v", err) + } + if components(otherProfile.Dir)[3] == gotComponents[3] { + t.Fatal("scope component did not change when profile changed") + } + otherIdentity, err := ForRun(layout, ref, pr, profile, "other-posting-identity", runID) + if err != nil { + t.Fatalf("ForRun with other posting identity: %v", err) + } + if components(otherIdentity.Dir)[3] == gotComponents[3] { + t.Fatal("scope component did not change when posting identity changed") + } + + for _, test := range []struct { + name string + profile string + postingIdentity string + }{ + {name: "blank profile", profile: "", postingIdentity: postingIdentity}, + {name: "blank posting identity", profile: profile, postingIdentity: ""}, + } { + t.Run(test.name, func(t *testing.T) { + if _, err := ForRun(layout, ref, pr, test.profile, test.postingIdentity, runID); err == nil { + t.Fatal("ForRun accepted blank resume scope value") + } + }) + } +} + func TestMarkerMatchesRequiresValidKindAndRunID(t *testing.T) { dir := t.TempDir() if err := WriteMarker(dir, KindReview, "run-1"); err != nil {