Add cairn review CLI verb: list/show/merge for shared-tier review branches#36
Closed
quad341 wants to merge 4 commits into
Closed
Add cairn review CLI verb: list/show/merge for shared-tier review branches#36quad341 wants to merge 4 commits into
quad341 wants to merge 4 commits into
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).
4 tasks
Owner
Author
|
Superseded by #40. This PR is stale (pinned to pre-reconciliation commit 596de13; CI red on build-test + lint after an out-of-band merge exposed symbol collisions between review.go and entry.go/branches.go). Builder reconciled the collisions on feat/crn-ffm-review-cli at commit d8c2436, which was independently re-reviewed and PASSED (see cairn bead crn-j1uh). #40 opens a fresh isolated deploy branch from that reconciled commit. Do not reuse or rebase this PR. |
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 changed
Adds a
cairn reviewcommand group with three subcommands for working with shared-tier review branches:list,show, andmerge.listderives the review tier for each branch and lists them from the CLI.showrenders a diff plus the relevant entry fields for a given review branch, so a reviewer can see exactly what changed before deciding anything.mergeapplies a surgical patch of only the specified fields (not a full rewrite of the entry), produces a--no-ffmerge commit, deletes the source branch afterward, and re-runs the store's reindex so the change is immediately visible.Why built this way
The merge path is split into a curate step and a merge step specifically so that if anything fails partway through, the store is left exactly as it was before the merge started — no partially-applied edits.
Every field that flows into the merged frontmatter (topic key, scope, and anchor type) is validated through the same path-segment validator before any mutation happens, so a value that could corrupt the TOML frontmatter — for example, one containing an embedded newline — is rejected up front rather than allowed to land in committed content and break later reads.
mergealso refuses to merge a file that matches a secret-looking pattern unless the caller explicitly opts in for that invocation, since this is often the first time a reviewer sees the real file content and shouldn't reject it sight unseen.What to review
The anchor-type validation path is the newest piece here: it reuses the same validator already applied to topic key and scope, on a field that otherwise carries free-form text straight into the merged frontmatter. Worth checking that there's no path into the merge that can still reach a write with an unvalidated anchor type.
Also worth a look: the ordering between validation and mutation in the merge path — that ordering is what gives the "no partial state on failure" guarantee described above.
How do we know it works
Tests cover: tier derivation across all three review tiers; the diff/field rendering in
show; thatmergepatches only the fields it's told to rather than the whole entry; that invalid topic key, scope, or anchor type values are rejected before any file is touched (including a regression test pinning a newline-injection payload against anchor type specifically); the exact shape of the merge commit plus post-merge branch deletion and reindex; that a failed merge leaves no partial state; and that the secret-pattern guard blocks by default and only relents on an explicit flag.gofmt,go vet, the full test suite with the race detector, andgolangci-lintall pass clean.