diff --git a/.agents/skills/harness-adapters/SKILL.md b/.agents/skills/harness-adapters/SKILL.md index 7215cf28b6a..2086db0ac36 100644 --- a/.agents/skills/harness-adapters/SKILL.md +++ b/.agents/skills/harness-adapters/SKILL.md @@ -92,7 +92,7 @@ This preserves launch success instead of passing a known-bad value. | Busy-pane signature | `esc to interrupt` | | Exit command | `/exit` | | Interrupt | single Escape | -| Skill invocation | `/` (e.g. `/code-review`) | +| Skill invocation | `/` (e.g. `/security-review`). NOT `/code-review` or `/verify`: those two Claude Code built-ins are `disable-model-invocation`, so a crewmate can never self-invoke them (verified by execution, 2.1.220). Whether typing one into a crew's composer with `fm-send` counts as the human-typed turn the gate wants is UNVERIFIED - do not rely on it. This is why `bin/fm-brief.sh` names no review command. | First launch in a fresh worktree, or first ever on a machine, may show a trust or bypass-permissions confirmation. After every spawn, peek the pane within about 20 seconds. @@ -204,7 +204,7 @@ Launch with a positional prompt: `grok --always-approve "$(cat )"`. | Busy-pane signature | `Ctrl+c:cancel` (the mid-turn cancel hint in grok's keybind bar, shown iff a turn is running; the spinner line is a braille glyph + `… N.Ns` + `[stop]`, e.g. `⠹ Thinking… 1.1s … [stop]`). Idle keybind bar shows only `Shift+Tab:mode │ Ctrl+.:shortcuts`. The ASCII `Ctrl+c:cancel` is the busy regex (avoids locale fragility of matching braille). | | Exit command | `Ctrl+Q` double-press within 1000ms (it is a confirmed destructive action). Prints `Resume this session with: grok --resume `. `Ctrl+D` is the quit key in VS Code family terminals. NOT `/exit` and NOT `Ctrl+C`. | | Interrupt | single `Ctrl+C` (cancels the current turn; the footer shows `Ctrl+c:cancel` mid-turn). `Esc` only moves focus to the scrollback, it does NOT interrupt. | -| Skill invocation | `/` (e.g. `/code-review`), same as claude; verified end to end that grok discovers a user-level skill and invokes it. Opens a slash-autocomplete popup, so a too-fast Enter selects the popup entry instead of sending. For an argument-taking command that first Enter does not submit at all - it expands the selection into an argument-hint placeholder in the composer (e.g. `/compact` -> `/compact compaction instructions`, live-verified), leaving real text still sitting there unsubmitted; a genuine second Enter is required. `fm-send`'s retried Enter lands it, because `fm_tmux_composer_state` correctly recognizes that placeholder-filled text as still-pending. | +| Skill invocation | `/` (e.g. `/security-review`), same as claude, and subject to the same caveat about Claude Code's gated built-ins; verified end to end that grok discovers a user-level skill and invokes it. Opens a slash-autocomplete popup, so a too-fast Enter selects the popup entry instead of sending. For an argument-taking command that first Enter does not submit at all - it expands the selection into an argument-hint placeholder in the composer (e.g. `/compact` -> `/compact compaction instructions`, live-verified), leaving real text still sitting there unsubmitted; a genuine second Enter is required. `fm-send`'s retried Enter lands it, because `fm_tmux_composer_state` correctly recognizes that placeholder-filled text as still-pending. | | Autonomy | `--always-approve` (footer shows `· always-approve`); auto-approves every tool execution, verified to run fully unattended. `--permission-mode bypassPermissions` is the stronger equivalent. | | Env marker | `GROK_AGENT=1`, set for child/tool processes. grok does NOT set `CLAUDECODE` despite Claude compatibility, so the marker is unambiguous. | | Resume | `grok --resume ` (id printed on exit) or `grok -c` / `--continue` (most recent for the cwd); `--fork-session` branches a new session id. | diff --git a/AGENTS.md b/AGENTS.md index ad3aa14539a..d74df7e37cb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -145,8 +145,9 @@ After any merge you perform without asking, post a one-line "merged ` (`local-only`). -**Your review is independent of the crew's own `/code-review`, `/verify`, and hook run, not a rubber stamp for them.** +A ship crewmate pushes its branch and reports `review-ready:` (mode `PR`) or `done: ready in branch fm/` (`local-only`), each carrying `reviewed by: - `. +**Your review is independent of the crew's own review, its end-to-end exercise, and the hook run, not a rubber stamp for them.** +A report missing that clause means the crew never got a review: send it back rather than reviewing on top of a gate that did not run. +Judge the outcome half against the diff you are about to read anyway - "no findings" on a diff with obvious defects is how an un-run review gives itself away. 1. Read the diff with `bin/fm-review-diff.sh ` (summary first, then `--full` or `--files `) - never a raw `git diff`, which can be stale. 2. Review it against the project's direction (section 5, gate 4). Mechanical quality is the hooks' and CI's job; you are looking for drift, wrong-shaped solutions, and scope creep. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d34b2ccf3f0..ea442c12a9e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -3,7 +3,8 @@ Thanks for wanting to contribute. This fork ships changes through an ordinary pull request, gated by CI and by review. -There is no separate validation pipeline to install: the quality gate is Claude Code hooks, the built-in `/code-review` skill, and the CI workflow in `.github/workflows/ci.yml`. +There is no separate validation pipeline to install: the quality gate is Claude Code hooks, an independent review of the diff, and the CI workflow in `.github/workflows/ci.yml`. +An agent contributor cannot invoke Claude Code's built-in `/code-review` or `/verify` and must obtain that review another way, such as a review subagent over the diff; `bin/fm-brief.sh`'s header owns why. ## Workflow diff --git a/bin/fm-brief.sh b/bin/fm-brief.sh index 136c1f59e4a..52fc4fb0e37 100755 --- a/bin/fm-brief.sh +++ b/bin/fm-brief.sh @@ -27,16 +27,33 @@ # with needs-decision rather than quietly working against it. # For ship tasks, the definition of done is shaped by the project's delivery mode # (data/projects.md via fm-project-mode.sh; see AGENTS.md task lifecycle): -# PR implement -> /code-review + /verify -> push the branch, open NO PR -> -# report `review-ready: branch fm/ pushed, no PR` and STOP (default). +# PR implement -> independent review + end-to-end exercise -> push the branch, +# open NO PR -> report +# `review-ready: branch fm/ pushed, no PR - reviewed by: - +# ` and STOP (default). # Firstmate reviews the pushed branch against the direction; findings are # fixed in place and re-signalled `review-ready:`, and only an approval opens # the PR (plain gh) with `done: PR `. The review gate sits BEFORE the PR so -# a finding costs one fix, not a re-review plus a re-run of the crew's verify and -# full suite and the PR's CI - the largest source of rework measured in the fleet. +# a finding costs one fix, not a re-review plus a re-run of the crew's end-to-end +# exercise and full suite and the PR's CI - the largest source of rework measured +# in the fleet. # The push still happens, so the work is durable against a box reboot. -# local-only implement on branch, stop and report "ready in branch" (no push/PR); +# local-only implement on branch, stop and report +# `done: ready in branch fm/ - reviewed by: - ` +# (no push/PR); # firstmate reviews, user approves, firstmate merges to local main +# Both modes require an INDEPENDENT review of the diff and name no command to get one. +# Naming one is unsafe in two directions: a harness may refuse to let an agent invoke its +# review command (Claude Code's built-in `/code-review` and `/verify` are both +# disable-model-invocation, i.e. runnable only in a turn a human typed them in, which a +# crewmate never has), and a same-named command from another source may review a different +# artifact (an already-open PR rather than the working diff). The scaffold therefore demands +# the OUTCOME - a fresh reader over the diff - and makes the crew report BOTH the mechanism and +# what the review found, on the status line it already writes. The outcome half is what makes +# the claim checkable: firstmate reads the same diff independently, so "no findings" on a diff +# with obvious defects is the tell that no review ran. A mechanism alone is unfalsifiable. +# The gate is built once by review_gate() and interpolated into both DODs, because two copies +# of one contract drift the moment only one is edited. # Ship briefs begin with a worktree-isolation assertion before the branch step. # Scout tasks ignore mode - their deliverable is a report, not a merge - but they still # carry the direction, because a recommendation that ignores it is worthless. @@ -292,6 +309,25 @@ read -r MODE _ < - \`.** Name the mechanism, then say what came back: the findings you acted on, or \`no findings\` if it genuinely returned none. + Naming a mechanism but no outcome does not count: firstmate reads the same diff independently, and a review that found nothing worth reporting on a diff with obvious defects is how it detects a review that never ran. Reporting nothing at all means the review did not happen and firstmate will send it back. + Fix the real findings yourself. A finding that turns on a human judgment call - a product choice, a destructive or irreversible action, a security trade-off - is NOT yours to decide: escalate it under rule 6 and stop. +3. Exercise the change end-to-end - drive the affected flow in the real app, not just the tests. + Do not assume a named command for this either; use whatever your harness provides. +EOF +} + case "$MODE" in local-only) RULE1="1. Never push to any remote and never open a PR. Work only on your \`fm/$ID\` branch; firstmate handles the merge into local \`main\`." @@ -301,11 +337,10 @@ This project ships **local-only**: no remote, no PR. 1. Implement the change and commit it on your branch \`fm/$ID\`. Do NOT push, do NOT open a PR, do NOT merge. Run the tests your change AFFECTS as you go. Run the project's FULL suite EXACTLY ONCE, at the end, before step 6 - not after every edit. -2. Run \`/code-review\` and address what it finds. Fix the real findings; a finding that is a human judgment call is not yours to decide - escalate it under rule 6. -3. Run \`/verify\` to exercise the change end-to-end - drive the affected flow in the real app, not just the tests. +$(review_gate "\`done:\` line at step 6") 4. Direction check: in one line, state how this change honors the Direction above. If it moves against the direction, stop and escalate under rule 6 instead of shipping it. 5. 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. -6. Append \`done: ready in branch fm/$ID\` to the status file and stop. +6. Append \`done: ready in branch fm/$ID - reviewed by: {mechanism} - {what it found}\` to the status file and stop, filling in both halves from step 2. Firstmate then reviews your branch diff against the project's direction, the user approves, and firstmate merges it into local \`main\`. EOF @@ -320,17 +355,16 @@ Firstmate reviews your pushed branch BEFORE any PR exists, so its findings cost 1. Implement the change and commit it on your branch. Run the tests your change AFFECTS as you go. Run the project's FULL suite EXACTLY ONCE, at the end, before step 6 - not after every edit. Re-running the whole suite per edit was the single biggest time sink measured in this fleet. -2. Run \`/code-review\` and address what it finds. - Fix the real findings yourself. A finding that turns on a human judgment call - a product choice, a destructive or irreversible action, a security trade-off - is NOT yours to decide: escalate it under rule 6 and stop. -3. Run \`/verify\` to exercise the change end-to-end - drive the affected flow in the real app, not just the tests. +$(review_gate "\`review-ready:\` line at step 6, and again in the PR body at step 7") + **State in the PR body how you exercised it.** 4. Satisfy the project's quality hooks. They run automatically on commit and push (secret scan, lint, typecheck, tests). A blocked commit or push means the gate caught something real; fix the cause, never work around the gate. 5. Direction check: in one line, state how this change honors the Direction above. If the task as specified would move AGAINST the direction, do not quietly implement it - escalate under rule 6. 6. **Push your branch. Open NO PR.** The push makes your work durable; the PR would only make firstmate's review expensive to act on. - Append \`review-ready: branch fm/$ID pushed, no PR\` to the status file and STOP. Firstmate now reviews your diff against the direction. + Append \`review-ready: branch fm/$ID pushed, no PR - reviewed by: {mechanism} - {what it found}\` to the status file and STOP, filling in both halves from step 2. Firstmate now reviews your diff against the direction. 7. Firstmate replies with one of two things: - - **Findings.** Fix them IN PLACE on the same branch, push again, and append \`review-ready:\` again. Repeat until firstmate approves. No PR exists yet, so there is nothing to churn. - - **Approval.** Open the PR with plain \`gh\` (\`gh pr create\`), append \`done: PR {url}\` to the status file, and stop. + - **Findings.** Fix them IN PLACE on the same branch, push again, and append \`review-ready:\` again, reporting the review of what you changed this round. Repeat until firstmate approves. No PR exists yet, so there is nothing to churn. + - **Approval.** Open the PR with plain \`gh\` (\`gh pr create\`), stating in the body how you reviewed and exercised the change, append \`done: PR {url}\` to the status file, and stop. Do NOT merge the PR, and do not wait for CI yourself. Firstmate watches CI; the user merges. Once the PR is open your work is on the remote, so firstmate releases your worktree at that point - finish step 7 and stop cleanly. diff --git a/bin/fm-hooks-install.sh b/bin/fm-hooks-install.sh index e649047e7ab..641e924a2bc 100755 --- a/bin/fm-hooks-install.sh +++ b/bin/fm-hooks-install.sh @@ -2,8 +2,9 @@ # Ensure a project worktree has a mechanical quality floor: Claude Code hooks that # enforce secret-scanning, lint, typecheck, and tests without an agent's cooperation. # Hooks are the floor that cannot be talked out of it. The judgment layer on top of -# them is the crewmate's own /code-review pass and firstmate's independent, -# direction-aware review of the diff before it reaches the user. +# them is the crewmate's own independent review of its diff and firstmate's independent, +# direction-aware review of the diff before it reaches the user. The crewmate's review +# names no command by design; bin/fm-brief.sh's header owns why. # # This is a worktree utility for crewmates, not a supervision script, so it does not # call fm-guard.sh, and firstmate never runs it against a project clone itself: diff --git a/bin/fm-promote.sh b/bin/fm-promote.sh index 827c17998f2..4dcc8eac9c0 100755 --- a/bin/fm-promote.sh +++ b/bin/fm-promote.sh @@ -6,6 +6,11 @@ # (inventory scratch state, reset to a clean default-branch base, carry over only # intended fix changes, create branch fm/, implement, then report done # according to the project's delivery mode). +# The scout brief carries no ship review gate and is never regenerated on promotion, +# so those instructions must carry it themselves: an independent review of the diff +# and a `reviewed by: - ` report. Without it a promoted crew +# cannot satisfy the rule in AGENTS.md's "Review and ship" and would be bounced forever. +# bin/fm-brief.sh's header owns why the gate names no command. # Usage: fm-promote.sh set -eu @@ -26,4 +31,4 @@ mv "$TMP" "$META" HOME_Q=$(printf '%q' "$FM_HOME") echo "promoted $ID to ship (teardown protection restored)" -echo "next: FM_HOME=$HOME_Q bin/fm-send.sh fm-$ID ''" +echo "next: FM_HOME=$HOME_Q bin/fm-send.sh fm-$ID ' - \">'" diff --git a/docs/proposals/context-management.md b/docs/proposals/context-management.md index f06e82cfbf4..b3da020c6c6 100644 --- a/docs/proposals/context-management.md +++ b/docs/proposals/context-management.md @@ -8,7 +8,7 @@ Harness posture: full stack on Claude/Agent-SDK, a bounded-tools-plus-reset floo ## 1. What this is A long-running firstmate session accumulates context the same way any agent does: every wake, pane peek, crew-state read, review diff, and fleet snapshot adds tokens, and a supervising session lives for hours across many tasks. -Crews carry even more risk per session, because they do the heavy reads, run `/code-review`, `/verify`, and full test suites, and today get zero context guidance in their brief. +Crews carry even more risk per session, because they do the heavy reads, run their own diff review, end-to-end exercise, and full test suites, and today get zero context guidance in their brief. Left alone, both drift toward the window limit, and the harness falls back to lossy auto-compaction that keeps what it guesses is important rather than what firstmate chose. This proposal makes context reset a routine, lossless, first-class operation rather than a crash-only event. diff --git a/tests/fm-brief.test.sh b/tests/fm-brief.test.sh index ea578734f7c..ceb4bd3ac57 100755 --- a/tests/fm-brief.test.sh +++ b/tests/fm-brief.test.sh @@ -125,8 +125,8 @@ test_pr_dod_carries_the_review_contract() { FM_HOME="$home" "$ROOT/bin/fm-brief.sh" brief-dod-b2 pr-proj >/dev/null 2>&1 brief="$home/data/brief-dod-b2/brief.md" - assert_grep '/code-review' "$brief" "PR DOD lost the self-review step" - assert_grep '/verify' "$brief" "PR DOD lost the end-to-end verify step" + # The review and end-to-end steps are owned by test_review_gate_is_obtainable_and_disclosed, + # which covers both delivery modes; asserting them here too would fail twice for one cause. assert_grep "Do NOT merge the PR" "$brief" "PR DOD lost the never-merge rule" assert_grep "never work around the gate" "$brief" "PR DOD lost the do-not-bypass-hooks rule" assert_grep "# Quality floor" "$brief" "ship brief lost the quality-floor section" @@ -136,6 +136,107 @@ test_pr_dod_carries_the_review_contract() { pass "fm-brief.sh: the PR definition of done carries the full review contract" } +# The review gate must be obtainable on the harness the crew actually runs on, and +# must be disclosed. The scaffold used to name `/code-review` and `/verify`; both are +# Claude Code built-ins marked disable-model-invocation, so a claude crewmate cannot +# invoke either (verified by execution: `Skill(code-review)` and `Skill(verify)` are +# both refused with reason `disable_model_invocation`). Naming a command is unsafe in +# the other direction too: a same-named command from another source can review a +# different artifact (an already-open PR rather than the working diff). +# +# So the scaffold must name NO review command and must instead force the crew to +# report the mechanism it used. The two DOD branches are separate heredocs, so a test +# that only covers PR mode would leave half the defect in place - both are asserted here. +test_review_gate_is_obtainable_and_disclosed() { + local home brief proj id + home="$TMP_ROOT/review-gate-home" + write_registry "$home" + + for id_proj in "brief-gate-pr:pr-proj" "brief-gate-local:local-proj"; do + id=${id_proj%%:*} + proj=${id_proj##*:} + FM_HOME="$home" "$ROOT/bin/fm-brief.sh" "$id" "$proj" >/dev/null 2>&1 + brief="$home/data/$id/brief.md" + + # The gate itself: a real, independent review of the diff, not a re-read. + assert_grep 'INDEPENDENT code review of your diff' "$brief" \ + "$id: DOD lost the independent-review requirement" + assert_grep 'fresh reader over the diff' "$brief" \ + "$id: DOD lost the not-your-own-re-read requirement" + assert_grep 'review subagent' "$brief" \ + "$id: DOD lost the works-on-every-harness fallback mechanism" + assert_grep 'Exercise the change end-to-end' "$brief" \ + "$id: DOD lost the end-to-end exercise step" + + # The disclosure half is load-bearing: without it an un-run review passes as clean. + # It has two halves and BOTH are required. A mechanism alone is unfalsifiable - a crew + # that skipped the review writes the same "review subagent" as one that ran it, and the + # brief itself supplies that answer two lines earlier. The outcome half is what firstmate + # can cross-check, because it reads the same diff independently. + assert_grep 'reviewed by: - ' "$brief" \ + "$id: DOD lost the mechanism-plus-outcome disclosure format" + assert_grep 'no findings' "$brief" \ + "$id: DOD lost the explicit empty-result form, leaving 'found nothing' indistinguishable from 'did not run'" + assert_grep 'reviewed by: {mechanism} - {what it found}' "$brief" \ + "$id: the step 6 report line lost its fill-in-both-halves template" + + # No un-runnable command may be named. `code-review` is asserted bare, not as + # `/code-review`, so swapping to codex's `$code-review` sigil cannot pass either; + # the cost is that the hyphenated English phrase is banned too, which the failure + # message says out loud. `verify` cannot get the same treatment - it is an ordinary + # English word the brief may legitimately use - so only its command form is banned. + assert_no_grep 'code-review' "$brief" \ + "$id: DOD contains the string 'code-review' - the command is un-runnable, and the bare hyphenated word is banned with it" + assert_no_grep '/verify' "$brief" "$id: DOD still names the un-runnable /verify" + done + pass "fm-brief.sh: both DOD modes demand an obtainable, disclosed review and name no command" +} + +# The rationale for naming no command is identical in both modes, so it is built once by +# review_gate() and interpolated twice. This pins that: if someone re-inlines it into the two +# heredocs, the copies drift and one mode silently keeps the old contract. Comparing the two +# generated briefs' step 2/3 blocks catches that without asserting on the wording itself. +test_review_gate_is_identical_in_both_modes() { + local home pr_gate local_gate + home="$TMP_ROOT/gate-shared-home" + write_registry "$home" + FM_HOME="$home" "$ROOT/bin/fm-brief.sh" brief-shared-pr pr-proj >/dev/null 2>&1 + FM_HOME="$home" "$ROOT/bin/fm-brief.sh" brief-shared-local local-proj >/dev/null 2>&1 + + # Steps 2 and 3 inclusive: from the review step up to (not including) the next numbered step. + pr_gate=$(sed -n '/^2\. Obtain an INDEPENDENT/,/^4\./p' "$home/data/brief-shared-pr/brief.md" | sed '$d') + local_gate=$(sed -n '/^2\. Obtain an INDEPENDENT/,/^4\./p' "$home/data/brief-shared-local/brief.md" | sed '$d') + + [ -n "$pr_gate" ] || fail "PR brief has no step 2 review gate to compare" + [ -n "$local_gate" ] || fail "local-only brief has no step 2 review gate to compare" + + # Exactly two lines are allowed to differ, and both are checked here rather than + # skipped: the report-line sentence (review_gate()'s single parameter) and the + # PR-body exercise disclosure (PR mode only - local-only has no PR body to state + # it in). Everything else must be byte-identical, which is what catches a fix + # applied to one heredoc and not the other. + # shellcheck disable=SC2016 # Literal backticks must remain unexpanded. + case "$pr_gate" in + *'**Report the review on your `review-ready:` line at step 6, and again in the PR body at step 7,'*) ;; + *) fail "PR gate lost its report-line parameter" ;; + esac + # shellcheck disable=SC2016 # Literal backticks must remain unexpanded. + case "$local_gate" in + *'**Report the review on your `done:` line at step 6,'*) ;; + *) fail "local-only gate lost its report-line parameter" ;; + esac + case "$pr_gate" in + *'**State in the PR body how you exercised it.**'*) ;; + *) fail "PR gate lost the PR-body exercise disclosure" ;; + esac + + pr_gate=$(printf '%s\n' "$pr_gate" | grep -v '\*\*Report the review on your\|\*\*State in the PR body how you exercised it\.\*\*') + local_gate=$(printf '%s\n' "$local_gate" | grep -v '\*\*Report the review on your') + [ "$pr_gate" = "$local_gate" ] || fail "review gate drifted between modes: +$(diff <(printf '%s\n' "$pr_gate") <(printf '%s\n' "$local_gate") || true)" + pass "fm-brief.sh: the review gate does not drift between the two delivery modes" +} + # The GitHub tooling rule: gh-axi is for reads only, mutations (the PR open # above all) go through plain gh, and a gh-axi error falls back to gh instead # of being debugged. Crews never address the user, so no crew brief may point @@ -479,6 +580,8 @@ test_context_discipline_in_ship_and_scout test_ship_modes_generate_clean_briefs test_legacy_mode_token_maps_to_pr test_pr_dod_carries_the_review_contract +test_review_gate_is_obtainable_and_disclosed +test_review_gate_is_identical_in_both_modes test_briefs_carry_the_github_tooling_split test_direction_is_injected_into_ship_and_scout test_absent_direction_is_explicit_not_silent