diff --git a/cmd/review.go b/cmd/review.go index 181ddc4..54d67ae 100644 --- a/cmd/review.go +++ b/cmd/review.go @@ -40,6 +40,14 @@ var reviewListCmd = &cobra.Command{ return err } for _, b := range branches { + // A branch ListReviewMergeBranches couldn't resolve (ga-1jk7la) + // is reported so its owner can find and fix it, but it never + // gets to hide every other pending branch -- always shown, + // regardless of --tier, since its own tier is unknown. + if b.Error != "" { + fmt.Printf("%s\tmalformed: %s\n", b.Name, b.Error) + continue + } if tier != "" && b.Tier != tier { continue } diff --git a/cmd/review_test.go b/cmd/review_test.go index e86c901..5e56633 100644 --- a/cmd/review_test.go +++ b/cmd/review_test.go @@ -2,6 +2,8 @@ package cmd import ( "bytes" + "os" + "path/filepath" "strings" "testing" @@ -118,6 +120,35 @@ func TestReviewListFiltersByTier(t *testing.T) { assert.NotContains(t, stdout, roleBranch) } +// TestReviewListReportsMalformedBranchAndStillListsValidOnes covers +// ga-1jk7la at the CLI layer: a remember/* branch whose changed file falls +// outside every recognized scope dir must not make `cairn review list` +// fail outright -- it must report that one branch as malformed and still +// print every other pending branch normally. +func TestReviewListReportsMalformedBranchAndStillListsValidOnes(t *testing.T) { + store := reviewCLIStore(t) + validBranch, e := seedReviewBranch(t, store, "topic-valid", []string{"rig:web"}, "body") + + def := strings.TrimSpace(gitOutput(t, store, "symbolic-ref", "--short", "HEAD")) + const malformedBranch = "remember/bad-a1b2c3d4" + gitOutput(t, store, "checkout", "-q", "-b", malformedBranch) + require.NoError(t, os.MkdirAll(filepath.Join(store, "cmd", "gc"), 0o750)) + require.NoError(t, os.WriteFile(filepath.Join(store, "cmd", "gc", "dispatch_control_ready_test.go"), []byte("not an entry"), 0o600)) + gitCommitAll(t, store, "malformed entry outside any scope dir") + gitOutput(t, store, "checkout", "-q", def) + + var err error + stdout := captureStdout(t, func() { + err = runReviewCmd(t, "review", "list", "--store", store) + }) + require.NoError(t, err, "one malformed branch must not abort 'cairn review list'") + assert.Contains(t, stdout, validBranch) + assert.Contains(t, stdout, "tier=rig") + assert.Contains(t, stdout, e.ID) + assert.Contains(t, stdout, malformedBranch+"\tmalformed: ") + assert.Contains(t, stdout, "unrecognized top-level scope dir") +} + func TestReviewShowPrintsDiffAndEntryFields(t *testing.T) { store := reviewCLIStore(t) branch, e := seedReviewBranch(t, store, "topic-a", []string{"rig:web"}, "the body text") diff --git a/internal/cairn/review.go b/internal/cairn/review.go index 0af05d4..7e2dc95 100644 --- a/internal/cairn/review.go +++ b/internal/cairn/review.go @@ -26,6 +26,14 @@ type ReviewMergeBranch struct { Tier string // "global" | "rig" | "role" -- never "agent" (crn-xw3 FR-8: agent/ is private and never reaches a review branch) TierValue string // e.g. "web" for a rig:web entry; "" for global EntryPath string // the changed entry file's path, relative to store + + // Error is set, and Tier/TierValue/EntryPath left zero, when this + // branch's own merge status or changed-file tier could not be resolved + // -- one malformed or unusual branch must not blank out visibility into + // every other one (ga-1jk7la), mirroring branches.go's + // ReviewBranch.Error: the same reporting-not-erroring stance for the + // same remember/* ref namespace. + Error string } // DefaultBranch resolves the store's currently checked-out branch -- the @@ -43,6 +51,14 @@ func DefaultBranch(ctx context.Context, store string) (string, error) { // derives its tier from the single entry file it changes -- never from the // branch name -- per crn-xw3 AF1: the branch is only ever named after the // entry's random id (reviewBranchName), which carries no tier information. +// +// A branch whose own merge status or tier cannot be resolved is reported +// via its ReviewMergeBranch.Error field, not a returned error: one +// malformed or unusual branch (e.g. one whose changed file falls outside +// every recognized scope dir) must not blank out every other pending +// branch for every reviewer/tier/identity (ga-1jk7la). The returned error +// stays reserved for a failure that makes the whole listing impossible -- +// resolving the default branch, or the for-each-ref call itself. func ListReviewMergeBranches(ctx context.Context, store string) ([]ReviewMergeBranch, error) { def, err := DefaultBranch(ctx, store) if err != nil { @@ -61,18 +77,21 @@ func ListReviewMergeBranches(ctx context.Context, store string) ([]ReviewMergeBr } merged, err := isMergedInto(ctx, store, name, def) if err != nil { - return nil, fmt.Errorf("check merge status of %q: %w", name, err) + branches = append(branches, ReviewMergeBranch{Name: name, Error: fmt.Sprintf("check merge status of %q: %v", name, err)}) + continue } if merged { continue // already merged into def -- not awaiting review, regardless of whether the branch was deleted } relPath, err := changedEntryFile(ctx, store, def, name) if err != nil { - return nil, err + branches = append(branches, ReviewMergeBranch{Name: name, Error: err.Error()}) + continue } tier, value, err := tierFromEntryPath(relPath) if err != nil { - return nil, fmt.Errorf("branch %q: %w", name, err) + branches = append(branches, ReviewMergeBranch{Name: name, Error: fmt.Sprintf("branch %q: %v", name, err)}) + continue } branches = append(branches, ReviewMergeBranch{Name: name, Tier: tier, TierValue: value, EntryPath: relPath}) } diff --git a/internal/cairn/review_test.go b/internal/cairn/review_test.go index ac26dfe..6b98368 100644 --- a/internal/cairn/review_test.go +++ b/internal/cairn/review_test.go @@ -98,20 +98,24 @@ func TestListReviewBranchesDerivesTierFromEntryPath(t *testing.T) { assert.True(t, strings.HasPrefix(role.EntryPath, "role/reviewer/")) } -// TestListReviewBranchesErrorsOnPrivateTierBranch documents the other side -// of tierFromEntryPath's contract: ListReviewMergeBranches has no tier +// TestListReviewBranchesReportsPrivateTierBranchAsError documents the other +// side of tierFromEntryPath's contract: ListReviewMergeBranches has no tier // filter of its own, so a remember/* branch that (unusually) changes a file -// under agent/ surfaces as an explicit error rather than being silently -// skipped or reported as a fourth tier. In production cmd/remember.go never -// routes a private-tier entry through CommitToReviewBranch, but the -// guarantee lives here, not there. -func TestListReviewBranchesErrorsOnPrivateTierBranch(t *testing.T) { +// under agent/ is reported back via its own Error field rather than being +// silently skipped or reported as a fourth tier -- and, per ga-1jk7la, that +// one branch's problem does not abort the whole call or hide any other +// pending branch. In production cmd/remember.go never routes a +// private-tier entry through CommitToReviewBranch, but the guarantee lives +// here, not there. +func TestListReviewBranchesReportsPrivateTierBranchAsError(t *testing.T) { store := reviewStore(t) - fixtureBranch(t, store, "private-topic", []string{"agent:bot"}, "a private note") + privateBranch, _ := fixtureBranch(t, store, "private-topic", []string{"agent:bot"}, "a private note") - _, err := ListReviewMergeBranches(t.Context(), store) - require.Error(t, err) - assert.Contains(t, err.Error(), "private agent/ tier") + branches, err := ListReviewMergeBranches(t.Context(), store) + require.NoError(t, err) + require.Len(t, branches, 1) + assert.Equal(t, privateBranch, branches[0].Name) + assert.Contains(t, branches[0].Error, "private agent/ tier") } // TestListReviewMergeBranchesExcludesAlreadyMergedBranch covers crn-12ubp: @@ -156,6 +160,61 @@ func TestListReviewMergeBranchesExcludesAlreadyMergedBranch(t *testing.T) { assert.Contains(t, names, pendingBranch, "an unmerged branch must still be listed as pending") } +// TestListReviewMergeBranchesToleratesMalformedBranch covers ga-1jk7la: a +// single remember/* branch whose changed file sits outside every +// recognized top-level scope dir -- a shape NewEntry/Create can never +// produce (Create only ever writes under one of scopeDirs), but one that +// has turned up in a real store as an out-of-band or historical branch -- +// must not blank out visibility into every other pending branch. Before +// this fix, tierFromEntryPath's error on that one branch propagated +// straight out of ListReviewMergeBranches as a hard top-level error, so a +// reviewer got nothing back for the whole store, not just the malformed +// entry. +func TestListReviewMergeBranchesToleratesMalformedBranch(t *testing.T) { + store := reviewStore(t) + validBranch, _ := fixtureBranch(t, store, "valid-topic", []string{"rig:web"}, "a valid note") + + def, err := DefaultBranch(t.Context(), store) + require.NoError(t, err) + + // Construct a remember/* branch that changes a file outside every + // recognized top-level scope dir -- mirroring the real-world repro's + // own "cmd/..." path -- via the same raw-git recipe + // TestChangedEntryFileErrorsWhenBranchTouchesMoreThanOneFile uses, + // since NewEntry/Create validates paths and could never produce this + // shape through the normal remember flow. + const malformedBranch = "remember/bad-a1b2c3d4" + _, err = gitRun(t.Context(), store, "checkout", "-q", "-b", malformedBranch) + require.NoError(t, err) + require.NoError(t, os.MkdirAll(filepath.Join(store, "cmd", "gc"), 0o750)) + require.NoError(t, os.WriteFile(filepath.Join(store, "cmd", "gc", "dispatch_control_ready_test.go"), []byte("not an entry"), 0o600)) + gitCommitAll(t, store, "malformed entry outside any scope dir") + _, err = gitRun(t.Context(), store, "checkout", "-q", def) + require.NoError(t, err) + + branches, err := ListReviewMergeBranches(t.Context(), store) + require.NoError(t, err, "one malformed branch must not abort the whole list") + require.Len(t, branches, 2) + + byName := make(map[string]ReviewMergeBranch, len(branches)) + for _, b := range branches { + byName[b.Name] = b + } + + valid := byName[validBranch] + assert.Equal(t, "rig", valid.Tier) + assert.Equal(t, "web", valid.TierValue) + assert.True(t, strings.HasPrefix(valid.EntryPath, "rig/web/")) + assert.Empty(t, valid.Error, "a valid branch must carry no error") + + malformed, ok := byName[malformedBranch] + require.True(t, ok, "the malformed branch must still be reported, not silently dropped") + assert.Empty(t, malformed.Tier) + assert.Empty(t, malformed.TierValue) + assert.Empty(t, malformed.EntryPath) + assert.Contains(t, malformed.Error, "unrecognized top-level scope dir") +} + func TestTierFromEntryPath(t *testing.T) { cases := []struct { name string diff --git a/release-gates/crn-g2mm3-gate.md b/release-gates/crn-g2mm3-gate.md new file mode 100644 index 0000000..e1eda24 --- /dev/null +++ b/release-gates/crn-g2mm3-gate.md @@ -0,0 +1,70 @@ +# Release Gate: crn-g2mm3 + +**Bead:** crn-g2mm3 (deploy) — source: crn-gx2v0 (review), origin: ga-1jk7la (gascity store, cross-rig handback) +**Commit:** `bdab0daab443f56e81db0eb294063add6383fd62`, cut onto `deploy/crn-g2mm3-gate` +**Date:** 2026-08-21 + +## Background + +`internal/cairn/review.go`'s `ListReviewMergeBranches` had three early +`return nil, err` points in its per-branch loop (`isMergedInto`, +`changedEntryFile`, `tierFromEntryPath`). Any single malformed `remember/*` +branch (real trigger: `remember/bd-ready-json-pagination-f249525c`) aborted +the entire listing for every tier/identity/reviewer — `cairn review list` +went down fleet-wide on one bad branch. + +The fix mirrors the existing sibling pattern in `branches.go`'s +`ListReviewBranches`/`ReviewBranch.Error` (report-don't-abort): adds +`ReviewMergeBranch.Error`; the three failure points append +`{Name, Error}` and `continue` instead of returning early. +`cmd/review.go` prints malformed entries as `"\tmalformed: "` +(unconditionally, before the `--tier` filter) then continues the normal +listing. `DefaultBranch`/for-each-ref failures (genuinely +whole-listing-impossible) are untouched and still return a real error. + +## Criteria + +| # | Criterion | Result | Evidence | +|---|-----------|--------|----------| +| 1 | Review PASS for exact deployed commit | PASS | crn-gx2v0 (closed, reviewer verdict PASS) notes cite `bdab0daab443f56e81db0eb294063add6383fd62` as the GREEN/fix commit; crn-g2mm3's own `**Commit:**` field and `gc.deploy_commit` metadata cite the identical SHA (D = R, byte-for-byte). Both independently re-resolved via `git rev-parse --verify --quiet "^{commit}"` (not trusted as transcribed mail/bead text) — real commit object, matches itself exactly, so not a short-prefix collision. | +| 2 | Acceptance criteria met | PASS | Read the actual diff directly (`git diff origin/main bdab0da -- internal/cairn/review.go cmd/review.go`), not just reviewer prose: confirmed all three early-return sites (`isMergedInto`, `changedEntryFile`, `tierFromEntryPath` error paths) replaced with `branches = append(branches, ReviewMergeBranch{Name: name, Error: ...}); continue`; new `Error` field added with doc comment; `DefaultBranch`/for-each-ref error returns untouched (correct scope — those really are whole-listing-impossible); `cmd/review.go` prints malformed branches unconditionally before the `--tier` filter check. Matches the claimed mirror of `branches.go:41`'s `ReviewBranch.Error` pattern. | +| 3 | Tests pass | PASS | Independently re-run in a fresh `mktemp` scratch worktree (detached at `bdab0da`, removed after) — not reused from the reviewer's report. `go build ./...` clean; `go vet ./...` clean; `gofmt -l .` empty; `golangci-lint run ./...` 0 issues; `go test -race ./...` — all 7 packages `ok` (root, cmd, formulas, internal/cairn, internal/critic, internal/obslog, scripts), zero FAIL, zero SKIP. Diff-owned tests resolved by exact name (`-run` + `-v`), all PASS individually: `TestReviewListReportsMalformedBranchAndStillListsValidOnes` (cmd), `TestListReviewBranchesReportsPrivateTierBranchAsError` + `TestListReviewMergeBranchesToleratesMalformedBranch` (internal/cairn). No go.mod/go.sum changes — no dependency surface. | +| 4 | No open HIGH findings | PASS | crn-gx2v0: "No blockers found. PASSing to deploy." One non-blocking nit noted (a `--tier`-while-malformed interaction not separately tested) — explicitly called out as trivial/non-blocking, not a HIGH finding. | +| 5 | Clean tree | PASS | `deploy/crn-g2mm3-gate` cut directly from `bdab0daab443f56e81db0eb294063add6383fd62^{commit}` via `resolve_deploy_branch_target`; `git status --porcelain` empty before the gate-file commit. | +| 6 | Clean divergence from main | PASS | `git merge-base --is-ancestor origin/main bdab0daab443f56e81db0eb294063add6383fd62` → true: `origin/main` (`afc97ffe5c16aaccd450b3c790e48966207387b1`) is the exact, direct parent-chain ancestor of the deploy commit (2 commits ahead: RED `ebbcdde3`, GREEN `bdab0da`). This is also the exact base the reviewer recorded. Trivial fast-forward — **no self-rebase needed at all**. | +| 7 | Single feature theme | PASS | Single bead, no `rollup-ship` label. All 4 changed files (`cmd/review.go`, `cmd/review_test.go`, `internal/cairn/review.go`, `internal/cairn/review_test.go`) sit under one subsystem: review-branch listing. Removing this commit pair from main would cleanly revert to the old abort-on-malformed behavior with nothing else affected. | + +## Verdict: PASS — proceeding to PR. + +## Process notes + +1. **Merge authority.** Per this session's role instructions: arm + `gh pr merge --auto` on the PR opened below (no strategy flag), then + verify by re-reading PR state. A declined arm (expected once the PR is + already MERGEABLE/CLEAN with nothing left pending to defer) routes to + mayor as a `MERGE-REQUEST` — the deployer never self-merges on + quad341/cairn. This matches both the current role prompt and the standing + memory `cairn-auto-merge-requires-explicit-strategy`, whose top-of-file + banner records self-merge as PAUSED (2026-08-20) pending an operator + ruling, with the operator-authored `deployer.md.tmpl` guardrail + controlling until then. The most recent prior deploy (`crn-tk8qd`, PR + #134) already reached this same conclusion and flagged the underlying + mayor-ruling/operator-guardrail contradiction separately rather than + acting on the old "standing authorization" — not re-litigated again here. + +2. **Branch target.** `builder/ga-1jk7la` (bead prose) and + `gc.deploy_branch` metadata are provenance-only, local-only (not on + origin) per the deploy bead's own instruction. `deploy/crn-g2mm3-gate` + was cut fresh from the exact reviewed SHA via `resolve_deploy_branch_target`; + `assert_safe_push_target` confirmed the derived name does not match the + shared-worktree-branch signature (`gc--`). + +3. **SHA integrity.** The deploy commit was independently re-resolved via + `git rev-parse --verify --quiet "^{commit}"` on both the D + (deploy-bead metadata) and R (review-bead notes) sides before comparison, + before any gate step ran — never trusted as an eyeballed or transcribed + string from the routing mail. + +4. **Ancestry scope.** No `.claude/**` paths in the diff; all commits in the + deploy range cite bead ids belonging to this deploy chain + (crn-gx2v0 / ga-1jk7la).