Skip to content

feat(brief): generate the since-last-review marker contract #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ecloin merged 2 commits into from
Aug 11, 2026
Merged

feat(brief): generate the since-last-review marker contract #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
34d80ba
feat(brief): generate the since-last-review marker contract
ecloin Aug 10, 2026
384297f
fix(brief): advance the review marker only on a pass verdict
ecloin Aug 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions bin/fm-brief.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,19 @@
# the configured merge authority approves, firstmate merges to local main
# no-mistakes-prod-only is a registry policy, not a task mode; resolve it to one of
# the three concrete modes at intake before calling this script.
# Only the local-only definition of done carries the review-marker contract, because
# direct-PR and no-mistakes work is reviewed on its forge instead: the worker keeps a
# pinned git tag <task-id>-reviewed as the since-last-review base, and ends every
# ready report with the exact single-base command:
# cd <worktree> && hunk diff <task-id>-reviewed
# Only a review-pass verdict advances that marker - an approval, or a batch of change
# requests handed back as the pass's outcome - and only ever to a tip the reviewer has
# already seen. Inline comments and questions arriving during a live pass are answered
# in place with `hunk session comment add` and never move it, because a mid-pass move
# would drop the rest of that pass out of the next diff. The worker also re-points the
# marker after any rebase or history rewrite (to the current merge-base when the mapping
# is ambiguous, so already-reviewed changes reappear rather than unreviewed ones hiding).
# bin/fm-teardown.sh deletes that one tag when the task is cleaned up.
# The generated ship brief records the chosen mode as a fixed machine-readable
# "Delivery contract: mode=<mode>" line. bin/fm-spawn.sh reads that line and refuses
# to launch a ship task whose explicit --mode disagrees, so an adjusted brief and the
Expand Down Expand Up @@ -375,6 +388,27 @@ The task is complete only when committed on your branch \`fm/$ID\`. Do NOT push,
Keep your branch a clean fast-forward onto the current default branch - if \`main\` has advanced, rebase onto it so the eventual merge stays a fast-forward.
When it is implemented and committed, append \`done: ready in branch fm/$ID\` to the status file and stop.
The configured merge authority approves the ready branch, then firstmate merges it into local \`main\` through the guarded fast-forward path.

# Review marker contract
This branch is reviewed as a since-last-review diff, so the review marker below must always point at the last content the reviewer has already seen.
Before you first report ready, make sure the git tag \`$ID-reviewed\` exists: if \`git rev-parse -q --verify refs/tags/$ID-reviewed\` finds nothing, create it at the pinned merge-base commit with \`git tag $ID-reviewed "\$(git merge-base main HEAD)"\` (substitute the repo's real default branch when it is not \`main\`).
The marker must always name that resolved commit, never a moving ref such as \`main\` or a branch name: a moving ref catches up with your branch tip and the review then shows an empty diff.
Your ready report is the message you end on when you stop, and its final line must be exactly this runnable command, with \`<worktree>\` replaced by this worktree's real absolute path from \`pwd -P\`:
\`cd <worktree> && hunk diff $ID-reviewed\`
That command takes ONE base ref and diffs it against the working tree, so a two-ref form is invalid; never write one.
Record your branch tip (\`git rev-parse HEAD\`) every time you report ready, because that recorded sha is the content the review pass you just opened covers.
When the reviewer's hunk session is live, read the inline notes with \`hunk session comment list --repo . --type user\`.
Only a review-pass VERDICT moves the marker: the reviewer's approval of that pass, or a batch of change requests handed back to you as that pass's outcome.
On such a verdict, FIRST advance the marker to that pass's recorded sha with \`git tag -f $ID-reviewed <recorded-sha>\`, THEN address the feedback, then re-report ready with the same command.
Individual inline comments and questions arriving while the review pass is still live are NOT a verdict, so leave the marker exactly where it is.
Answer each of those in place with \`hunk session comment add --repo . --file <path> --new-line <n> --summary <your answer>\` (use \`--old-line <n>\` for a line your change removes) and keep going in the same pass.
Advancing the marker mid-pass would drop the rest of that pass's still-unreviewed changes out of the reviewer's next diff.
Never advance the marker to a tip the reviewer has not seen, because that hides unreviewed changes from the next review.
After ANY rebase or history rewrite the marker points at commits your branch no longer contains, so re-point it as part of that same rewrite with \`git tag -f $ID-reviewed <rebased commit holding the last reviewed content>\`.
When that mapping is ambiguous, re-point the marker to the current merge-base instead.
Always fail toward showing already-reviewed changes again, never toward hiding unreviewed ones.
Never run the interactive hunk commands (\`hunk diff\`, \`hunk show\`) yourself; they are the reviewer's surface, not a worker tool.
The tag lives in the shared project clone, so create or move ONLY the exact \`$ID-reviewed\` tag and never create, move, or delete any other tag.
EOF
;;
*) # no-mistakes
Expand Down
27 changes: 27 additions & 0 deletions bin/fm-teardown.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@
# device. It refuses and preserves task state when that proof fails; otherwise
# it removes the task's check, trust record, PR sidecar, publication record, and
# quarantine entries with the rest of the volatile state.
# A ship task's review marker - the `<task-id>-reviewed` tag bin/fm-brief.sh's
# local-only contract has the worker pin in the shared project clone - is removed
# with the rest of the task's durable records, after every landed-work refusal has
# passed. Only that exact tag is deleted; a task that never created one is a silent
# no-op, and no other tag in the clone is touched.
# Orca tasks use the same safety checks, then close the recorded terminal and
# remove the recorded worktree through `orca worktree rm`; teardown never guesses
# an Orca target from ambient CLI state.
Expand Down Expand Up @@ -675,6 +680,25 @@ retire_busy_state() {
fi
}

# A local-only ship task keeps a `<task-id>-reviewed` git tag in the shared
# project clone as the reviewer's pinned since-last-review base (generated by
# bin/fm-brief.sh's review-marker contract). Once the work has landed nothing
# consumes it, so retire exactly that one ref and leave every other tag - the
# project's releases and the captain's own markers - untouched. A missing tag is
# the normal case for other delivery modes and is not an error. Called only
# after every landed-work refusal has passed, alongside the other durable
# per-task record removals, so an aborted teardown leaves the marker intact for
# a rerun.
remove_review_marker_tag() { # <project> <task-id>
local proj=$1 id=$2 tag
tag="$id-reviewed"
[ -n "$proj" ] && [ -d "$proj" ] || return 0
git -C "$proj" rev-parse --verify --quiet "refs/tags/$tag" >/dev/null 2>&1 || return 0
git -C "$proj" tag -d "$tag" >/dev/null 2>&1 \
|| echo "warning: could not remove review marker tag $tag in $proj" >&2
return 0
}

validate_pr_poll_cleanup() {
local state_dir=$1 id=$2 quarantine state_device artifact has_artifact=0
fm_task_id_path_safe "$id" || return 0
Expand Down Expand Up @@ -2531,6 +2555,9 @@ fm_backend_clear_transition "$BACKEND" "$STATE" "$T" || true
# Remove the per-task temp root (/tmp/fm-<id>/, incl. its gotmp/) recorded by spawn.
# Read before the state-file rm below; empty (pre-fix tasks without tasktmp=) is a no-op.
[ -n "$TASK_TMP" ] && rm -rf "$TASK_TMP"
# Ship tasks only: a scout worktree is scratch and a secondmate home has no task
# branch, so neither ever carries a review marker.
[ "$KIND" != ship ] || remove_review_marker_tag "$PROJ" "$ID"
remove_pr_poll_artifacts "$STATE" "$ID" || exit 1
retire_busy_state "$STATE" "$ID" "$BUSY_GEN" || exit 1
rm -f "$STATE/$ID.status" "$STATE/$ID.turn-ended" "$STATE/$ID.meta" \
Expand Down
82 changes: 82 additions & 0 deletions tests/fm-brief.test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,87 @@ test_no_mistakes_dod_wording() {
pass "fm-brief.sh: no-mistakes DOD keeps its apostrophe prose, now parse-safe"
}

# Local-only work is reviewed as a since-last-review diff against a pinned tag,
# so that whole contract must be generated rather than hand-built per task: the
# four ways a hand-built command broke (full-changeset diffs, moving compare refs
# that rot into an empty diff, invalid two-ref forms, and a marker stranded by a
# rebase) each have a line here. So does the fifth way a marker loses review
# coverage: only a review-pass verdict may advance it, because moving it on an
# inline comment mid-pass silently drops the rest of that pass out of the next
# diff. direct-PR and no-mistakes work is reviewed on its forge instead, so those
# scaffolds - and scout/secondmate - must stay clear of the whole contract.
test_local_only_review_marker_contract() {
local home id brief other
home="$TMP_ROOT/review-marker-home"
mkdir -p "$home/data"
id="brief-review-marker-e1"
FM_HOME="$home" "$ROOT/bin/fm-brief.sh" "$id" some-proj --mode local-only >/dev/null 2>&1 \
|| fail "local-only brief with the review marker contract should scaffold"
brief="$home/data/$id/brief.md"
assert_grep "# Review marker contract" "$brief" \
"local-only brief lost the review marker contract section"
assert_grep "git rev-parse -q --verify refs/tags/$id-reviewed" "$brief" \
"review marker contract did not have the worker check for an existing marker first"
# shellcheck disable=SC2016 # the literal $(...) must reach the worker unexpanded
assert_grep "git tag $id-reviewed \"\$(git merge-base main HEAD)\"" "$brief" \
"review marker contract did not pin a missing marker at the merge-base commit"
assert_grep "never a moving ref such as \`main\` or a branch name" "$brief" \
"review marker contract did not forbid a moving compare ref"
assert_grep "a moving ref catches up with your branch tip and the review then shows an empty diff" "$brief" \
"review marker contract did not explain how a moving ref rots"
assert_grep "\`cd <worktree> && hunk diff $id-reviewed\`" "$brief" \
"review marker contract did not render the exact runnable review command"
assert_grep "replaced by this worktree's real absolute path from \`pwd -P\`" "$brief" \
"review marker contract did not require a real absolute worktree path"
assert_grep "takes ONE base ref and diffs it against the working tree, so a two-ref form is invalid" "$brief" \
"review marker contract did not rule out the invalid two-ref command form"
assert_grep "Record your branch tip (\`git rev-parse HEAD\`) every time you report ready" "$brief" \
"review marker contract did not have the worker record the reported tip"
assert_grep "Only a review-pass VERDICT moves the marker" "$brief" \
"review marker contract did not gate the marker on a review-pass verdict"
assert_grep "the reviewer's approval of that pass, or a batch of change requests handed back to you as that pass's outcome" "$brief" \
"review marker contract did not define what counts as a verdict"
assert_grep "On such a verdict, FIRST advance the marker to that pass's recorded sha with \`git tag -f $id-reviewed <recorded-sha>\`" "$brief" \
"review marker contract did not advance the marker on a verdict before addressing feedback"
assert_grep "Individual inline comments and questions arriving while the review pass is still live are NOT a verdict, so leave the marker exactly where it is." "$brief" \
"review marker contract did not keep the marker still for mid-pass inline notes"
assert_grep "hunk session comment add --repo . --file <path> --new-line <n> --summary <your answer>" "$brief" \
"review marker contract did not have the worker answer mid-pass notes inline"
assert_grep "Advancing the marker mid-pass would drop the rest of that pass's still-unreviewed changes out of the reviewer's next diff." "$brief" \
"review marker contract did not explain why a mid-pass move loses review coverage"
assert_grep "Never advance the marker to a tip the reviewer has not seen" "$brief" \
"review marker contract did not forbid advancing past unreviewed content"
assert_grep "After ANY rebase or history rewrite" "$brief" \
"review marker contract did not handle a rewritten history stranding the marker"
assert_grep "re-point the marker to the current merge-base instead" "$brief" \
"review marker contract did not resolve an ambiguous rebase mapping safely"
assert_grep "Always fail toward showing already-reviewed changes again, never toward hiding unreviewed ones." "$brief" \
"review marker contract lost its direction-of-safety rule"
assert_grep "hunk session comment list --repo . --type user" "$brief" \
"review marker contract did not have the worker read live inline review notes"
assert_grep "Never run the interactive hunk commands (\`hunk diff\`, \`hunk show\`) yourself" "$brief" \
"review marker contract did not keep interactive review commands out of worker hands"
assert_grep "create or move ONLY the exact \`$id-reviewed\` tag and never create, move, or delete any other tag" "$brief" \
"review marker contract did not confine tag writes to this task's own marker"

# Forge-reviewed ship modes and the non-ship scaffolds carry no marker contract.
for other in no-mistakes direct-PR; do
FM_HOME="$home" "$ROOT/bin/fm-brief.sh" "brief-review-marker-$other" some-proj --mode "$other" >/dev/null 2>&1
assert_no_grep "# Review marker contract" "$home/data/brief-review-marker-$other/brief.md" \
"$other brief must not carry the local-only review marker contract"
assert_no_grep "-reviewed" "$home/data/brief-review-marker-$other/brief.md" \
"$other brief must not mention a review marker tag"
done
FM_HOME="$home" "$ROOT/bin/fm-brief.sh" brief-review-marker-scout some-proj --scout >/dev/null 2>&1
assert_no_grep "# Review marker contract" "$home/data/brief-review-marker-scout/brief.md" \
"scout brief must not carry the review marker contract"
FM_HOME="$home" FM_SECONDMATE_CHARTER='domain work' \
"$ROOT/bin/fm-brief.sh" brief-review-marker-sm --secondmate --no-projects >/dev/null 2>&1
assert_no_grep "# Review marker contract" "$home/data/brief-review-marker-sm/brief.md" \
"secondmate charter must not carry the review marker contract"
pass "fm-brief.sh: local-only briefs carry the complete since-last-review marker contract"
}

test_ship_project_memory_wording() {
local home id brief
home="$TMP_ROOT/project-memory-home"
Expand Down Expand Up @@ -719,6 +800,7 @@ test_ship_mode_is_explicit_not_registry
test_delivery_flags_are_refused_where_they_do_not_apply
test_faster_paths_use_configured_authority_without_stacked_review
test_no_mistakes_dod_wording
test_local_only_review_marker_contract
test_ship_project_memory_wording
test_herdr_lab_contract_is_explicit_and_complete
test_herdr_lab_contract_quotes_foreign_firstmate_path
Expand Down
87 changes: 87 additions & 0 deletions tests/fm-teardown.test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,12 @@
# (w) index.lock mtime read failure -> lock kept, REFUSE
# (x) transient lock cleared after first failed return -> retry ALLOW
# (y) persistent lock (never clears, not provably stale) -> REFUSE loudly
#
# Also covers review-marker cleanup for the since-last-review contract that
# bin/fm-brief.sh generates into local-only briefs:
# (z1) marker tag present, work landed -> only that tag removed, others kept
# (z2) no marker tag at all -> silent, no diagnostic, teardown completes
# (z3) refused teardown -> marker kept (nothing retires before safety)
set -u

# shellcheck source=tests/lib.sh disable=SC1091
Expand Down Expand Up @@ -631,6 +637,84 @@ test_local_only_truly_unpushed_refuses() {
pass "local-only worktree with truly unpushed work is refused (safety preserved)"
}

# The local-only review-marker contract (bin/fm-brief.sh) has the worker pin a
# `<task-id>-reviewed` tag in the shared project clone as the reviewer's
# since-last-review base. Cleanup retires exactly that ref once the work has
# landed; every other tag in the clone, including another task's marker, is none
# of teardown's business.
test_review_marker_tag_is_removed_when_present() {
local case_dir rc
case_dir=$(make_case review-marker-present)
write_meta "$case_dir" local-only ship
wt_commit "$case_dir" "reviewed work"
add_fork_with_pushed_branch "$case_dir"
git -C "$case_dir/project" tag task-x1-reviewed main
git -C "$case_dir/project" tag v1.0.0 main
git -C "$case_dir/project" tag other-task-reviewed main

set +e
run_teardown "$case_dir" > "$case_dir/stdout" 2> "$case_dir/stderr"
rc=$?
set -e

expect_code 0 "$rc" "review-marker-present: teardown should succeed for landed local-only work"
if git -C "$case_dir/project" rev-parse -q --verify refs/tags/task-x1-reviewed >/dev/null 2>&1; then
fail "review-marker-present: the task's review marker survived teardown"
fi
git -C "$case_dir/project" rev-parse -q --verify refs/tags/v1.0.0 >/dev/null 2>&1 \
|| fail "review-marker-present: teardown deleted an unrelated project tag"
git -C "$case_dir/project" rev-parse -q --verify refs/tags/other-task-reviewed >/dev/null 2>&1 \
|| fail "review-marker-present: teardown deleted another task's review marker"
pass "teardown removes exactly the task's review marker tag and leaves other tags alone"
}

# A task that never created a marker - any forge-reviewed mode, or local-only work
# torn down before a first review - must tear down exactly as before: no error, no
# diagnostic, no mention of a marker.
test_review_marker_absent_is_silent() {
local case_dir rc
case_dir=$(make_case review-marker-absent)
write_meta "$case_dir" local-only ship
wt_commit "$case_dir" "reviewed work"
add_fork_with_pushed_branch "$case_dir"

set +e
run_teardown "$case_dir" > "$case_dir/stdout" 2> "$case_dir/stderr"
rc=$?
set -e

expect_code 0 "$rc" "review-marker-absent: a missing review marker must not fail teardown"
assert_grep "teardown task-x1 complete" "$case_dir/stdout" \
"review-marker-absent: teardown did not complete"
assert_no_grep "-reviewed" "$case_dir/stderr" \
"review-marker-absent: teardown complained about a marker that never existed"
assert_no_grep "-reviewed" "$case_dir/stdout" \
"review-marker-absent: teardown mentioned a marker that never existed"
pass "teardown is silent when the task never pinned a review marker"
}

# Ordering: the marker is a durable record of the task, so nothing may retire it
# before the landed-work checks pass. A refused teardown must leave it exactly
# where it was, ready for the rerun after the work actually lands.
test_review_marker_tag_survives_refused_teardown() {
local case_dir rc
case_dir=$(make_case review-marker-refused)
write_meta "$case_dir" local-only ship
wt_commit "$case_dir" "unpushed work"
git -C "$case_dir/project" tag task-x1-reviewed main

set +e
run_teardown "$case_dir" > "$case_dir/stdout" 2> "$case_dir/stderr"
rc=$?
set -e

expect_code 1 "$rc" "review-marker-refused: unlanded work must still refuse"
grep -q REFUSED "$case_dir/stderr" || fail "review-marker-refused: no REFUSED line in stderr"
git -C "$case_dir/project" rev-parse -q --verify refs/tags/task-x1-reviewed >/dev/null 2>&1 \
|| fail "review-marker-refused: a refused teardown removed the review marker anyway"
pass "a refused teardown leaves the review marker in place (safety ordering preserved)"
}

test_local_only_merged_to_local_main_allows() {
local case_dir rc
case_dir=$(make_case merged-main)
Expand Down Expand Up @@ -2599,6 +2683,9 @@ test_local_only_merged_to_local_main_allows
test_no_mistakes_origin_remote_allows
test_no_mistakes_truly_unpushed_refuses
test_local_only_force_overrides_unpushed
test_review_marker_tag_is_removed_when_present
test_review_marker_absent_is_silent
test_review_marker_tag_survives_refused_teardown
test_teardown_missing_busy_sidecar_completes
test_herdr_teardown_clears_escalation_marker
test_herdr_flat_teardown_refuses_orphaning_records_then_retry_completes
Expand Down
Loading