diff --git a/bin/fm-brief.sh b/bin/fm-brief.sh index a873c84051..d1724d9a5b 100755 --- a/bin/fm-brief.sh +++ b/bin/fm-brief.sh @@ -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 -reviewed as the since-last-review base, and ends every +# ready report with the exact single-base command: +# cd && hunk diff -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=" 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 @@ -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 \`\` replaced by this worktree's real absolute path from \`pwd -P\`: +\`cd && 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 \`, 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 --new-line --summary \` (use \`--old-line \` 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 \`. +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 diff --git a/bin/fm-teardown.sh b/bin/fm-teardown.sh index a45f8abe48..e3b56070d6 100755 --- a/bin/fm-teardown.sh +++ b/bin/fm-teardown.sh @@ -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 `-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. @@ -675,6 +680,25 @@ retire_busy_state() { fi } +# A local-only ship task keeps a `-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() { # + 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 @@ -2531,6 +2555,9 @@ fm_backend_clear_transition "$BACKEND" "$STATE" "$T" || true # Remove the per-task temp root (/tmp/fm-/, 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" \ diff --git a/tests/fm-brief.test.sh b/tests/fm-brief.test.sh index a348e2d345..5f4b00e12b 100755 --- a/tests/fm-brief.test.sh +++ b/tests/fm-brief.test.sh @@ -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 && 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 \`" "$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 --new-line --summary " "$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" @@ -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 diff --git a/tests/fm-teardown.test.sh b/tests/fm-teardown.test.sh index a0815a967e..20d1e2c3f2 100755 --- a/tests/fm-teardown.test.sh +++ b/tests/fm-teardown.test.sh @@ -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 @@ -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 +# `-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) @@ -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