cairn review list: don't abort the whole listing on one malformed branch

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
 			}

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")

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})
 	}

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

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 `"<name>\tmalformed: <reason>"`
+(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 "<sha>^{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-<agent>-<hex>`).
+
+3. **SHA integrity.** The deploy commit was independently re-resolved via
+   `git rev-parse --verify --quiet "<sha>^{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).