Repository navigation
cairn review list: don't abort the whole listing on one malformed branch - #135
Merged
Merged
Conversation
added 3 commits
August 21, 2026 15:22
…rmed branch
ga-1jk7la: a single remember/* branch whose changed file falls outside
every recognized top-level scope dir hard-errors the whole
ListReviewMergeBranches call, so `cairn review list` returns nothing for
any tier/identity the moment it hits one such branch -- not just the
malformed entry.
Red: TestListReviewMergeBranchesToleratesMalformedBranch fails today with
exactly the bug's own repro error ("... has unrecognized top-level scope
dir \"cmd\"") even though a second, fully valid pending branch also
exists in the store.
ga-1jk7la: ListReviewMergeBranches (internal/cairn/review.go) returned nil plus a hard error the moment any single remember/* branch failed isMergedInto, changedEntryFile, or tierFromEntryPath -- e.g. a branch whose changed file sits outside every recognized scope dir (global/rig/role/agent). That took down `cairn review list` entirely, for every tier/identity, over one unrelated branch. Mirror the same-domain sibling ListReviewBranches/ReviewBranch.Error pattern already established in branches.go: add ReviewMergeBranch.Error, and change the loop to append the branch with Error set and continue instead of returning early, at all three failure points. The function's own signature is unchanged -- ([]ReviewMergeBranch, error) -- error stays reserved for a failure that makes the whole listing impossible (DefaultBranch, for-each-ref). cmd/review.go's `review list` now prints a malformed branch as its own "<name>\tmalformed: <reason>" line (always shown, regardless of --tier, since its tier is unknown) and continues listing every other pending branch normally. Updates TestListReviewBranchesErrorsOnPrivateTierBranch (renamed TestListReviewBranchesReportsPrivateTierBranchAsError) to the new tolerant contract, and adds two regression tests: one at the internal/cairn unit level (TestListReviewMergeBranchesToleratesMalformedBranch) and one at the cmd CLI level (TestReviewListReportsMalformedBranchAndStillListsValidOnes), both constructing a malformed branch via the same raw-git recipe TestChangedEntryFileErrorsWhenBranchTouchesMoreThanOneFile already uses (NewEntry/Create can't produce this shape through the normal remember path).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
cairn review list's merge-branch listing used to go down fleet-wide the moment it hit a single malformedremember/*branch — one branch it couldn't resolve (bad merge-base check, unreadable changed file, or an unrecognized tier) aborted the entire listing, hiding every other pending review branch for every tier/identity/reviewer.Now a branch that can't be resolved is reported inline (
<branch> malformed: <reason>) and skipped, and the rest of the listing proceeds normally. This mirrors the report-don't-abort behaviorcairn review listalready used for the sibling branch-listing path — merge-branch listing was the one place still aborting on error.Why built this way
The three previously-fatal error paths (merge-status check, reading the changed file, deriving its tier) only ever fail on a single misbehaving branch — none of them mean the whole listing is unusable. Errors that genuinely do mean that (resolving the default branch, or the underlying
for-each-refcall itself) are untouched and still return a real error, so a genuinely broken repo state isn't silently swallowed.Review notes
ReviewMergeBranch.Errorfield — set (withTier/TierValue/EntryPathleft zero) exactly when a branch can't be resolved.--tierfilter, so a bad branch can't be hidden by filtering on a tier it doesn't have.Test plan
go build/go vet/gofmt/golangci-lintall cleango test -race ./...— all 7 packages green, zero failures/skipsrelease-gates/crn-g2mm3-gate.md🤖 Deployed by actual-factory