Add cairn review CLI verb: list/show/merge for shared-tier review branches#40
Merged
Conversation
…nches Implements AF1/AF2 of crn-xw3's design: the review half of the propose/ review flow crn-419's remember command already writes one side of. review list derives tier from the changed entry file's path (never the branch name); review show renders the diff plus a parsed Entry so a reviewer can inspect real content before any merge; review merge surgically patches only reviewer-specified frontmatter fields, blocks on an obvious secret pattern unless overridden, --no-ff merges with the "librarian: merge <branch> — <title> (<bead>)" convention, deletes the branch, and reindexes — aborting cleanly with no partial state on any failure up through the merge itself. Refs crn-ffm.
opts.AnchorType flowed straight into tomlQuote unvalidated, unlike TopicKey/Scope. An embedded newline produced invalid TOML that merged successfully before Reindex choked on it, corrupting the default branch and breaking every store read path (IterEntries) until hand-fixed. Validate AnchorType via ValidatePathSegment in MergeReviewBranch, matching the existing TopicKey/Scope pattern, with a regression test pinning the newline-injection case reported in review. Addresses the blocking finding from cairn/reviewer's crn-8emp review (mail gm-wisp-9h17fbl).
…nches.go (crn-j1uh) origin/main gained two independent features since this branch forked: entry.go's own splitFrontmatter/tomlQuote (PR #24) and branches.go's own ReviewBranch/ListReviewBranches for stale-branch tracking (PR #39, crn-0yv.1). Both pairs collide with review.go's pre-existing identically-named declarations once merged. - tomlQuote: consolidated. entry.go's version is a strict, behavior-compatible superset for review.go's validated-input call sites (TopicKey/AnchorType/ Scope, already passed through ValidatePathSegment); deleted review.go's duplicate, zero call-site changes needed. - splitFrontmatter: renamed to splitFrontmatterForPatch in review.go. Not substitutable with entry.go's version -- different input type (string vs []byte), different return contract (bool-flag+body vs sentinel-error+ closeAndBody), and review.go's closing-fence retention is required by patchFrontmatterFields for byte-exact surgical reassembly. - ReviewBranch/ListReviewBranches: renamed to ReviewMergeBranch/ ListReviewMergeBranches in review.go. branches.go's versions serve the librarian sweep's age/SHA-tracked notify-escalate bookkeeping; review.go's serve the interactive list/show/merge review flow. Different shapes for different concerns, developed independently -- not a merge candidate. Verified: gofmt, go vet, go build, go test ./... -race -count=1, and golangci-lint (cache-cleaned) all clean across every package, run from the module root. Specifically confirmed both ListReviewBranches test families (branches_test.go's, unchanged, and review_test.go's, renamed) pass side by side with no cross-talk, and TestMergeReviewBranchRejectsInvalidAnchorType / TestTomlQuoteEscapesBackslashAndDoubleQuote still pass against the shared entry.go implementation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Closed
4 tasks
quad341
added a commit
that referenced
this pull request
Jul 23, 2026
PR #40 (review CLI verb) merged into origin/main after this branch's merge base, adding review_test.go/cmd/review_test.go calls to Find(store, id). This branch's Find takes a leading context.Context (index-backed reads reconciliation). Update the 4 stale call sites to pass t.Context(), matching sibling calls already in these files.
quad341
added a commit
that referenced
this pull request
Jul 23, 2026
…ead of scanning entries (#42) * fix(cairn): index-backed reads + entry_tags DDL race fix, squashed for rebase (crn-2xpm) Squashes builder/crn-2xpm-reconcile's commits since merge-base f6970f5 (net diff only, unchanged) into one commit re-applied cleanly on current origin/main, so a linear rebase no longer replays this branch's internal merge commits' pre-resolution states as independent patches. Deploy gate's attempt_bounded_self_rebase found a real conflict (rc=12) rebasing this branch's history onto origin/main, even though a 3-way git merge-tree of the same two trees was clean. Root cause: this branch contains merge commits (9168234, 3479d32, cc6ceb2, 8d913c5) that already 3-way-resolved conflicts between parallel feature branches and against origin/main. A linear rebase instead replays each original commit individually -- including pre-resolution commits like 9724ae9, whose lone-patch content still conflicts with what origin/main looks like now, even though that conflict was already resolved once, downstream, via those merge commits. Merging origin/main again (a third time) would not fix this: it resolves the conflict at the merge point but leaves the same problematic commit replayable-and-conflicting on any subsequent rebase. Flattening the branch's own history is the only durable fix. Net change (unchanged from e9cdcd2, verified identical via `git diff --cached --stat` vs `git diff f6970f5 e9cdcd2 --stat`): the index-backed reads stack (Status/Visible/Prime/Find all index-backed, ctx-threaded, zero full-body TOML decode or hit_count writes on read paths), the entry_tags DROP+CREATE DDL race fix + txlock=immediate (crn-j3k4), the busy_timeout/WAL locking fix (crn-t250), crn-ln1's scopeMismatchWarnings diagnostic reconciled onto the index-backed Status read, and the 4 review_test.go/cmd/review_test.go call sites updated to the new Find(ctx, store, id) signature after PR #40 landed with the old one. New SHA per crn-fie5 SHA-pinning mandate -- requires fresh reviewer verdict; e9cdcd2's PASS does not carry over. Old branch builder/crn-2xpm-reconcile (tip e9cdcd2, already-open PR #41) left untouched as historical reference, same treatment as the PR it superseded. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: release gate PASS for cairn-index-backed-reads-squashed --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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
Adds
cairn review {list,show,merge}— a CLI workflow for a librarian totriage pending contributions on shared-tier
remember/*branches before theyland on the default branch.
cairn review list [--tier <tier>]discovers pendingremember/*branchesand derives each entry's tier from its changed file path, so a librarian
can see what's waiting without checking out anything.
cairn review show <branch>renders the diff plus the parsedEntryfields for one branch, so the librarian can read the actual content before
deciding — this is the real gate, everything else is bookkeeping.
cairn review merge <branch> [--topic-key ...] [--scope ...] [--anchor-type ...] [--bead ...] [--allow-secret-pattern]curates theentry's frontmatter (patching only the fields the librarian specifies, not
a full rewrite), merges with
--no-ff, deletes the source branch, andreindexes. An obvious-secret-pattern check blocks the merge by default
and requires an explicit flag to override.
Why one PR
This originally shipped as PR #36, but that PR is stale: main advanced twice
after #36 opened (once for unrelated helper functions, once for stale-branch
detection) and both times independently added a same-named helper that
collided with one already defined in this feature's own
review.go. Bothcollisions are resolved by renaming this feature's local helpers (not by
touching the other side), so this PR carries only this feature's 4 files, no
changes to the code it previously collided with. #36 is closed as
superseded by this PR.
Review notes
mergereuses the existingValidatePathSegmentguard ontopic_key,scope, andanchor_type— the last one closes a newline-injectiongap (an unvalidated
--anchor-typevalue could corrupt the defaultbranch's TOML frontmatter and break every read path store-wide) found and
fixed during review; see
TestMergeReviewBranchRejectsInvalidAnchorType.mergerunsgit merge --abortand leaves thedefault branch exactly as it was — no partial state — even though a
frontmatter curation commit may already have landed on the source branch
itself (that commit is inert until a successful merge; a retry is
idempotent).
AWS/GitHub/Slack/Google/Stripe/private-key) on the single changed file —
by design, not a substitute for
cairn review show.Test plan
go test ./... -race -count=1— all 4 packages, zero regressionsgofmt -l .,go vet ./...,golangci-lint run ./...— all clean→ fix → re-review PASS round trip on the anchor-type validation gap
release-gates/cairn-review-cli-verb-gate.md🤖 Deployed by actual-factory