feat(brief): generate the since-last-review marker contract - #1
Merged
Conversation
Local-only ship work is reviewed interactively as a diff against a fixed base, but the generated brief said only "report ready in branch", so the review command was hand-built per task and broke four ways: whole-changeset diffs instead of since-last-review, a moving compare ref that catches up with the branch tip and renders an empty diff, invalid two-ref command forms, and a marker left pointing at pre-rebase commits after a history rewrite. The local-only definition of done now carries the contract instead. The worker pins a `<task-id>-reviewed` tag at the merge-base before first reporting ready, ends every ready report with the exact single-base `cd <worktree> && hunk diff <task-id>-reviewed`, records the reported tip and advances the marker to that recorded sha only when feedback arrives, and re-points the marker after any rebase - falling back to the current merge-base when the mapping is ambiguous, so already-reviewed changes reappear rather than unreviewed ones hiding. Tag writes are confined to that one ref. direct-PR, no-mistakes, scout, and secondmate output is unchanged: that work is reviewed on its forge. fm-teardown.sh retires the marker with the task's other durable records, after every landed-work refusal has passed, so a refused teardown leaves it intact and no other tag in the clone is touched.
The first version told the worker to advance the marker "when review feedback arrives", which reads as any feedback - including a single inline comment or question arriving while the reviewer is still working through the pass. Advancing there silently drops the rest of that pass's unreviewed changes out of the next diff, which is the exact failure the marker exists to prevent. The generated contract now separates the two cases. A review-pass verdict - the reviewer's approval, or a batch of change requests handed back as the pass's outcome - advances the marker to that pass's recorded sha before the feedback is addressed. Inline comments and questions arriving mid-pass are explicitly not a verdict: the marker stays put and the worker answers each one in place with hunk session comment add, then keeps going in the same pass.
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.
Problem
Local-only ship work is reviewed interactively as a diff against a fixed base, but the generated brief said only "report ready in branch
fm/<id>" and defined no review contract. The review command was therefore hand-built per task, and it has broken four ways:hunk difftakes one base ref and diffs it against the working tree)<id>-reviewedtag at pre-rebase commitsChange
bin/fm-brief.sh- the local-only definition of done now generates a "Review marker contract" section.direct-PRandno-mistakeswork is reviewed on its forge, so their output, plus scout and secondmate output, is byte-identical to before. The generated contract has the worker:<task-id>-reviewedatgit merge-base <default-branch> HEADbefore first reporting ready, never at a moving refcd <worktree> && hunk diff <task-id>-reviewed, using the worktree's real absolute path frompwd -Phunk session comment add, since a mid-pass move drops the rest of that pass's unreviewed changes out of the next diffhunk session comment list --repo . --type user, and never run the interactivehunk diff/hunk showthemselvesbin/fm-teardown.sh- retires exactly the<task-id>-reviewedtag from the project clone for ship tasks, with the task's other durable records. An absent tag is a silent no-op, no other tag is touched, and it runs only after every landed-work refusal has passed, so a refused teardown leaves the marker intact for the rerun.Both facts live in the scripts' own headers (and
--help), matching this repo's one-owner rule for mechanics;AGENTS.mdis unchanged.Verification
tests/fm-brief.test.sh- newtest_local_only_review_marker_contractasserts every clause of the generated contract with the task id substituted, and asserts the other four scaffolds carry none of it. 21/21 pass.tests/fm-teardown.test.sh- new cases (z1) marker removed while unrelated tags and another task's marker survive, (z2) absent marker is silent, (z3) a refused teardown keeps the marker (safety ordering). 61/61 pass.no-mistakes,direct-PR, scout and secondmate briefs againstmain: identical.bin/fm-lint.shclean on ShellCheck 0.11.0 (the pin).bin/fm-test-run.sh --changed: 34 suites, 0 failures (2 harness-gated skips).🤖 Generated with Claude Code