From b1de8a8df0f3b0f854f654297b8489e432726c93 Mon Sep 17 00:00:00 2001 From: Kun Chen <3233006+kunchenguid@users.noreply.github.com> Date: Fri, 26 Jun 2026 09:23:06 -0700 Subject: [PATCH 001/167] feat(send): route secondmate replies for firstmate requests (#93) * feat(send): mark firstmate->secondmate requests for status-path reply fm-send now prepends a distinct from-firstmate marker (new bin/fm-marker-lib.sh: the label [fm-from-firstmate] followed by ASCII unit separator 0x1f) when the resolved target is a bare fm- whose meta records kind=secondmate. The marker reuses the afk untypable-separator concept but is deliberately distinct from the daemon's bare leading 0x1f, so it never conflates with a secondmate's own afk escalation marker. A secondmate is itself a firstmate, so a relayed request lands in its own chat, which the main firstmate never reads. The secondmate charter (fm-brief.sh) and AGENTS.md now document the recognize-and-respond-via-status contract: a marked request is answered via the status file (or a doc plus a status pointer), never chat-only; an unmarked message stays conversational captain intervention. Crewmate/scout targets, explicit session:window targets, and the --key path are unchanged. Backbone (watcher, daemon, tmux-lib, afk) untouched. Hermetic tests cover marked vs unmarked sends and the exact marker bytes. * no-mistakes(document): Sync secondmate marker docs --- .../skills/secondmate-provisioning/SKILL.md | 4 +- AGENTS.md | 6 + CONTRIBUTING.md | 1 + README.md | 1 + bin/fm-brief.sh | 15 +- bin/fm-marker-lib.sh | 61 ++++++ bin/fm-send.sh | 27 ++- docs/architecture.md | 2 + docs/scripts.md | 3 +- tests/fm-secondmate-lifecycle-e2e.test.sh | 8 +- tests/fm-send-secondmate-marker.test.sh | 180 ++++++++++++++++++ 11 files changed, 301 insertions(+), 7 deletions(-) create mode 100644 bin/fm-marker-lib.sh create mode 100755 tests/fm-send-secondmate-marker.test.sh diff --git a/.agents/skills/secondmate-provisioning/SKILL.md b/.agents/skills/secondmate-provisioning/SKILL.md index fc0b155459..d92a00ed70 100644 --- a/.agents/skills/secondmate-provisioning/SKILL.md +++ b/.agents/skills/secondmate-provisioning/SKILL.md @@ -32,9 +32,9 @@ bin/fm-brief.sh --secondmate ... The scaffold writes a charter brief instead of a task brief. Set `FM_SECONDMATE_CHARTER=''` to fill the charter text and `FM_SECONDMATE_SCOPE=''` when the routing scope differs. If you scaffold without `FM_SECONDMATE_CHARTER`, replace the `{TASK}` placeholder before seeding. -Keep the charter focused on the persistent responsibility, available project clones, and escalation back to the main firstmate status file. +Keep the charter focused on the persistent responsibility, available project clones, escalation back to the main firstmate status file, and the requests-from-main-firstmate contract. The scaffold's definition of done encodes the idle-by-default contract: on startup the secondmate reconciles only its own in-flight work and then waits for routed tasks, never self-initiating a survey or audit. -Preserve that wording when filling the charter. +Preserve that wording when filling the charter, including the marker rule that marked supervisor requests return through status or a doc pointer while unmarked captain messages stay conversational. Provision the persistent home and registry entry after the charter is filled: diff --git a/AGENTS.md b/AGENTS.md index 80700a2085..257804159a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -291,6 +291,10 @@ A project may appear in several `projects:` clone lists, so choose the secondmat If the resolved project is `local-only`, keep the work with the main firstmate even when a secondmate scope sounds relevant. If a secondmate's scope fits, steer that secondmate with one concise instruction via `bin/fm-send.sh fm- ''` and let it run the normal lifecycle inside its own home. The bare `fm-` target resolves through this home's `state/.meta`; pass `session:window` only when intentionally targeting a window outside this firstmate home. +A secondmate is itself a firstmate, so a request reaches it in its own chat, which you never read - the return channel that wakes you is its status file. +So `fm-send` to a bare `fm-` whose meta is `kind=secondmate` automatically prepends a from-firstmate marker (`bin/fm-marker-lib.sh`); the secondmate recognizes it and returns its answer via its status file, or via a doc under its home plus a status pointer for a detailed response, never only in chat. +Expect and read that response on the status/doc path the same way you read any other status signal; do not peek the secondmate's chat for the answer. +A captain typing directly into the secondmate's window is unmarked and stays a conversational captain intervention, so do not relay captain-destined chat through this path; the marker is applied only by `fm-send` to a `kind=secondmate` target. Do not spawn a direct crewmate for work that belongs to a secondmate scope unless the secondmate is blocked or the captain explicitly redirects it. If no secondmate scope fits, proceed in the main firstmate or create a new secondmate with the captain when that domain should become persistent. When you create a new secondmate, hand its in-scope queued items off from the main backlog into its home with `bin/fm-backlog-handoff.sh` so it owns its domain's queue from day one (section 6). @@ -345,6 +349,7 @@ Covered by section 8. Steer a crewmate only with short single lines via `bin/fm-send.sh`; anything long belongs in a file the crewmate can read. Steer a secondmate the same way. Its charter retargets escalation to the main firstmate's status file, so routine internal churn stays inside the secondmate home and only `done`, `blocked`, `needs-decision`, `failed`, or captain-relevant phase changes wake the main firstmate. +Because `fm-send` to a `kind=secondmate` target marks the request as from-firstmate (section 7 intake), the secondmate's answer comes back on that status/doc path too, not in its chat; read the response there as an ordinary status signal and do not peek its chat for it. ### Delivery modes and yolo @@ -586,6 +591,7 @@ The scaffold writes a charter brief instead of a task brief. Set `FM_SECONDMATE_CHARTER=''` to fill the charter text and `FM_SECONDMATE_SCOPE=''` when the routing scope differs. If you scaffold without `FM_SECONDMATE_CHARTER`, replace the `{TASK}` placeholder before seeding. Keep the charter focused on persistent responsibility, available project clones, escalation back to the main firstmate status file, and the idle-by-default contract: reconcile only its own in-flight work and then wait, never self-initiating a survey or audit. +Preserve the requests-from-main-firstmate contract in the charter: marked requests return via status or a doc pointer, while unmarked direct captain messages stay conversational. Before seeding, loading, handing backlog to, or launching a secondmate home, load `secondmate-provisioning`. The status-reporting protocol is intentionally sparse: crewmates append status only for supervisor-actionable phase changes or `needs-decision`/`blocked`/`done`/`failed`, because every append wakes firstmate. For any generated brief that still contains `{TASK}`, replace it with a clear task description, acceptance criteria, and any constraints or context the crewmate needs before spawning or seeding. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ec99ec0929..17d306c571 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -63,6 +63,7 @@ tests/fm-wake-queue.test.sh # durable wake queue losslessness, cat tests/fm-watcher-lock.test.sh # watcher singleton, lock-race, watch-arm liveness, and guard-warning tests tests/fm-daemon.test.sh # sub-supervisor classifier, /afk presence-gating, max-defer, composer, and fm-send submit tests tests/fm-send-settle.test.sh # fm-send post-submit settle pause, tuning, disable, and --key bypass tests +tests/fm-send-secondmate-marker.test.sh # fm-send from-firstmate marker for kind=secondmate targets: marked vs crewmate/explicit/--key, and the exact marker byte sequence tests/fm-wake-daemon-lifecycle-e2e.test.sh # watcher + daemon lifecycle e2e: restart catch-up, batching, dedupe, stale-pane routing, and digest injection tests/fm-composer-ghost.test.sh # dim-ghost stripping, ghost-only composer detection, and escape-free peek tests tests/fm-afk-inject-e2e.test.sh # private-socket end-to-end test of the afk injection path (partial-input deferral, swallowed-Enter retry) diff --git a/README.md b/README.md index ceb14ff854..71b6d5871a 100644 --- a/README.md +++ b/README.md @@ -109,6 +109,7 @@ Outside tmux, crewmates land in a detached `firstmate` session you can attach to You chat with the first mate. It routes each request to a crewmate in its own tmux window and git worktree, supervises the fleet with a zero-token event-driven watcher, and brings you finished PRs, approved local merges, or investigation reports. Persistent secondmate homes are linked firstmate worktrees; startup syncs live ones and secondmate launch syncs the target home to the primary default-branch commit without fetching from origin when it is safe. +When a routed request goes to a secondmate, firstmate marks it so the answer returns through status or a document pointer; direct typing into that secondmate window stays conversational. A presence-gated sub-supervisor (`/afk`) can self-handle routine events and batch only what matters while you step away. When firstmate works on itself, spawn-time isolation checks and a primary-checkout tangle alarm keep the operating checkout on its default branch and stop a crewmate that did not land in a separate worktree. diff --git a/bin/fm-brief.sh b/bin/fm-brief.sh index 3acc08267c..1193d7ef9d 100755 --- a/bin/fm-brief.sh +++ b/bin/fm-brief.sh @@ -13,7 +13,8 @@ # --secondmate writes a persistent secondmate charter. The project list # is cloned into the secondmate home, while the natural-language scope # tells the main firstmate when to route work there; routine churn stays in its own home; -# only captain-relevant escalations append to this home's status file. +# captain-relevant escalations and marked from-firstmate replies append to this +# home's status file. # Set FM_SECONDMATE_CHARTER='' to fill the charter text. # Set FM_SECONDMATE_SCOPE='' to write a routing scope distinct from the charter text. # For ship tasks, the definition of done is shaped by the project's delivery mode @@ -31,6 +32,8 @@ set -eu SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=bin/fm-marker-lib.sh +. "$SCRIPT_DIR/fm-marker-lib.sh" FM_ROOT="${FM_ROOT_OVERRIDE:-$(cd "$SCRIPT_DIR/.." && pwd)}" FM_HOME="${FM_HOME:-${FM_ROOT_OVERRIDE:-$FM_ROOT}}" DATA="${FM_DATA_OVERRIDE:-$FM_HOME/data}" @@ -90,12 +93,22 @@ You do not generate your own work. Act only on tasks the main firstmate routes to you. Never start a survey, audit, or "find improvements" sweep on your own initiative; that is not your job and it is unwanted. +# Requests from the main firstmate +You are a firstmate in your own home, so an incoming message reaches you in your own chat. +You must distinguish who it is from, because the answer goes to a different place. +A request relayed to you by the main firstmate (your supervisor) is tagged with a leading \`$FM_FROMFIRST_LABEL\` marker followed by an invisible system separator; this marker is untypable, so a human never produces it. +When a message carries that marker, do the work, then respond via the STATUS/ESCALATION path below, never only in this chat: the main firstmate does not read your chat, so a chat-only reply is lost. +For a terse result, a status line is the whole answer. +For a detailed answer (an investigation, a plan, an audit), write it to a doc under your home's \`data/\` and append a status line that points to that doc - the scout-report pattern - so the main firstmate is woken and can read it. +A message with NO marker is the captain typing directly into your pane: treat it as authoritative captain intervention and stay conversational exactly as you would for any captain message; do not force it onto the status path. + # Escalation to main firstmate Handle routine work yourself. Escalate only true captain-relevant outcomes by appending one line: \`echo "{state}: {one short line}" >> $STATUS_FILE\` States: working, needs-decision, blocked, done, failed. Use this only for material phase changes, a captain decision, a real blocker, a failure, or work ready for review. +This is also how you return the answer to a marked from-firstmate request above. Routine internal supervision, heartbeats, retries, and crewmate churn stay inside your own home and must not touch that status file. # Definition of done diff --git a/bin/fm-marker-lib.sh b/bin/fm-marker-lib.sh new file mode 100644 index 0000000000..6cc69cd04f --- /dev/null +++ b/bin/fm-marker-lib.sh @@ -0,0 +1,61 @@ +#!/usr/bin/env bash +# fm-marker-lib.sh - the from-firstmate request marker. +# +# When the MAIN firstmate relays a work request to one of its SECONDMATES, +# bin/fm-send.sh prepends this marker to the message text. A secondmate is itself +# a firstmate running in its own home, so without a marker it treats every +# incoming fm-send/tmux line as if its captain typed it and answers +# CONVERSATIONALLY in its own chat. But the main firstmate never reads a +# secondmate's chat: the only main<-secondmate wakeup channel is the status file +# (charter escalation), optionally pointing to a doc for detail. A detailed +# chat-only reply therefore strands, unseen. +# +# The marker lets the secondmate tell its supervisor's request apart from a +# message the captain typed directly into its pane: +# +# - marked -> a from-firstmate request. Do the work, then respond via the +# STATUS/ESCALATION path (a status line for a terse result, or a +# doc plus a status pointer - the scout-report pattern - for a +# detailed one) so it surfaces to the main firstmate via the +# watcher signal. It MUST NOT respond only in chat. +# - unmarked -> the captain typing directly. Stay conversational, exactly as +# before: authoritative captain intervention. +# +# This contract lives in the generated secondmate charter (bin/fm-brief.sh) so it +# travels with the live secondmate, and is summarized in AGENTS.md. +# +# Distinct from the afk daemon marker, on purpose. +# The away-mode daemon (bin/fm-supervise-daemon.sh) marks its daemon->firstmate +# escalations with a BARE leading unit separator (FM_INJECT_MARK, ASCII 0x1f). +# This from-firstmate marker mirrors that CONCEPT - it reuses the ASCII unit +# separator (0x1f), which is untypable on a normal keyboard, as the "a human can +# never forge this" guarantee - but it is a DISTINCT sequence: a human-readable +# label FOLLOWED by the separator, never a bare leading 0x1f. The afk contract +# keys on a LEADING 0x1f, which this marker never has, so the two cannot +# conflate: a secondmate's own afk machinery never mistakes a from-firstmate +# request for an internal daemon escalation, and vice versa. The visible label is +# also what the secondmate's LLM actually reads in its pane, since the separator +# byte itself is invisible. +# +# Sourced by bin/fm-send.sh, bin/fm-brief.sh, and the tests. No side effects on +# source. set -u / set -e safe. + +# The label field: human-readable, greppable, and distinctive enough that the +# captain would not type it by hand. This is the part the secondmate's LLM reads. +FM_FROMFIRST_LABEL='[fm-from-firstmate]' + +# The full marker fm-send prepends to a from-firstmate request: the label, then +# the ASCII unit separator (0x1f) as the untypable field separator. The request +# text follows the separator. +FM_FROMFIRST_MARK="${FM_FROMFIRST_LABEL}"$'\x1f' + +# fm_message_from_firstmate: 0 (true) if carries the from-firstmate +# marker - it begins with the label immediately followed by the unit separator - +# and 1 otherwise. The unit separator is untypable, so a captain-typed message, +# even one that happens to start with the label text alone, is never matched. +fm_message_from_firstmate() { # + case "$1" in + "$FM_FROMFIRST_MARK"*) return 0 ;; + esac + return 1 +} diff --git a/bin/fm-send.sh b/bin/fm-send.sh index 80c6641e8f..dd8e889cdd 100755 --- a/bin/fm-send.sh +++ b/bin/fm-send.sh @@ -12,6 +12,13 @@ # instead of silently leaving an unsubmitted instruction (incident afk-invx-i5). # The composer/submit logic is shared with the away-mode daemon via # bin/fm-tmux-lib.sh. Tune with FM_SEND_RETRIES (default 3) / FM_SEND_SLEEP (0.4). +# +# From-firstmate marker: when the resolved target is a bare `fm-` whose meta +# records kind=secondmate, the text is prefixed with the from-firstmate marker +# (bin/fm-marker-lib.sh) so the secondmate routes its reply via its status file +# or a status-pointed doc instead of stranding it in chat the main firstmate +# never reads. A crewmate/scout target, an explicit session:window escape-hatch +# target, and the --key path are never marked - their behavior is unchanged. # After a successful text submit fm-send pauses FM_SEND_SETTLE seconds (default 1, # 0 disables) before returning: a cleared composer only proves the text was # submitted, but the harness needs a beat to spin up the turn before its busy @@ -27,6 +34,8 @@ STATE="${FM_STATE_OVERRIDE:-$FM_HOME/state}" # shellcheck source=bin/fm-tmux-lib.sh . "$SCRIPT_DIR/fm-tmux-lib.sh" +# shellcheck source=bin/fm-marker-lib.sh +. "$SCRIPT_DIR/fm-marker-lib.sh" "$SCRIPT_DIR/fm-guard.sh" || true @@ -48,9 +57,25 @@ resolve() { esac } +RAW_TARGET=$1 T=$(resolve "$1") shift +# Mark a from-firstmate -> secondmate request. Only a bare `fm-` target, +# resolved through this home's meta and recording kind=secondmate, is marked: the +# secondmate then routes its reply via the status path (see fm-marker-lib.sh). +# An explicit session:window target (the escape hatch for windows outside this +# home) and any crewmate/scout target are left unmarked, and so is the --key path. +MARK_PREFIX="" +case "$RAW_TARGET" in + fm-*) + meta="$STATE/${RAW_TARGET#fm-}.meta" + if [ -f "$meta" ] && grep -q '^kind=secondmate$' "$meta" 2>/dev/null; then + MARK_PREFIX="$FM_FROMFIRST_MARK" + fi + ;; +esac + if [ "${1:-}" = "--key" ]; then tmux send-keys -t "$T" "$2" else @@ -61,7 +86,7 @@ else sleep_s=${FM_SEND_SLEEP:-0.4} # Type once, submit, verify. Lenient: only a positively-confirmed swallow # (text still in the composer) is an error; an unreadable pane is assumed sent. - verdict=$(fm_tmux_submit_core "$T" "$*" "$retries" "$sleep_s" "$settle") + verdict=$(fm_tmux_submit_core "$T" "$MARK_PREFIX$*" "$retries" "$sleep_s" "$settle") case "$verdict" in pending) echo "error: text not submitted to $T (Enter swallowed; text left in composer)" >&2 diff --git a/docs/architecture.md b/docs/architecture.md index 182bba9bed..0a891f3f4f 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -50,6 +50,8 @@ Seeding is transactional: if validation, cloning, initialization, or registry up `local-only` projects stay with the main first mate because they merge into the main local checkout instead of a remote-backed PR path. The same project may appear in multiple secondmate homes when their scopes differ, such as issue triage versus feature development. Secondmates are idle by default: after startup recovery reconciles only work already in their own home, an empty queue waits silently for routed tasks, and they never self-initiate surveys or audits. +Bare `fm-send.sh fm-` requests to a live `kind=secondmate` are prefixed with the from-firstmate marker from `bin/fm-marker-lib.sh`, so the secondmate returns terse answers through status lines and detailed answers through docs plus status pointers instead of replying only in its own chat. +Explicit `session:window` sends and direct human typing stay unmarked, so captain intervention in a secondmate pane remains conversational. After seeding a secondmate, `fm-backlog-handoff.sh` moves already-judged in-scope queued items from the main backlog into that secondmate home so the domain queue starts in the right place. Idle secondmate panes are healthy; teardown is explicit and refuses while the secondmate home has in-flight work unless the captain has approved discard with `--force`. diff --git a/docs/scripts.md b/docs/scripts.md index 6b00887d33..5aa092a32c 100644 --- a/docs/scripts.md +++ b/docs/scripts.md @@ -17,6 +17,7 @@ Each file also starts with a short header comment. | `fm-project-mode.sh` | Resolve a project's delivery mode and `+yolo` flag from `data/projects.md` | | `fm-merge-local.sh` | Fast-forward a `local-only` project's local default branch after approval | | `fm-review-diff.sh` | Review a crewmate branch against the authoritative base, with optional `--stat` output | +| `fm-marker-lib.sh` | Shared from-firstmate request marker and detector sourced by `fm-send.sh`, `fm-brief.sh`, and tests | | `fm-watch-arm.sh` | Verified per-home watcher re-arm; reports `started`, `healthy`, or `FAILED`; `--restart` relaunches only this home's watcher | | `fm-watch.sh` | Singleton-safe one-shot watcher; blocks until supervision work is due, queues it durably, then exits with one reason line | | `fm-supervise-daemon.sh` | Presence-gated sub-supervisor for walk-away (`/afk`) supervision: wraps `fm-watch.sh`, self-handles routine wakes in bash, and escalates only captain-relevant events as one verified, batched, single-line digest prefixed with a sentinel marker | @@ -25,7 +26,7 @@ Each file also starts with a short header comment. | `fm-tasks-axi-lib.sh` | Shared `tasks-axi` compatibility probe sourced by bootstrap and teardown | | `fm-wake-drain.sh` | Atomically drain queued watcher wakes before handling supervision work | | `fm-wake-lib.sh` | Shared durable wake queue and portable lock helpers sourced by the watcher, drain, arm, guard, and daemon | -| `fm-send.sh` | Send one verified literal line (or `--key Escape`) to a crewmate window; exits non-zero when Enter is positively swallowed; text sends pause `FM_SEND_SETTLE` seconds after success | +| `fm-send.sh` | Send one verified literal line (or `--key Escape`) to a direct-report window; exits non-zero on confirmed swallowed Enter; bare `kind=secondmate` targets are marked as from-firstmate; text sends pause `FM_SEND_SETTLE` seconds after success | | `fm-tmux-lib.sh` | Shared tmux pane primitives for busy detection, dim-ghost-aware and border-aware composer detection, and verified submit retry | | `fm-peek.sh` | Print a bounded tail of a crewmate pane | | `fm-pr-check.sh` | Record a PR-ready task and arm the watcher's merge poll | diff --git a/tests/fm-secondmate-lifecycle-e2e.test.sh b/tests/fm-secondmate-lifecycle-e2e.test.sh index f1d251c283..3d4950e8c3 100755 --- a/tests/fm-secondmate-lifecycle-e2e.test.sh +++ b/tests/fm-secondmate-lifecycle-e2e.test.sh @@ -140,9 +140,13 @@ phase_send() { FM_FAKE_TMUX_LOG="$LOG" FM_FAKE_TMUX_CAPTURE="$PANE" \ "$ROOT/bin/fm-send.sh" fm-design 'route this work' >/dev/null 2>&1 \ || fail "fm-send failed for a bare firstmate window with home metadata" - assert_grep 'send-keys -t firstmate:fm-design -l route this work' "$LOG" "send did not use the window recorded in this home's meta" + # design is a kind=secondmate target, so the request is prefixed with the + # from-firstmate marker (bin/fm-marker-lib.sh): the send targets the meta window + # AND carries the marker label, and the original payload still follows it. + assert_grep 'send-keys -t firstmate:fm-design -l [fm-from-firstmate]' "$LOG" "send did not use the window recorded in this home's meta, or did not mark the secondmate request" + assert_grep 'route this work' "$LOG" "the original request text did not survive the marker" assert_no_grep 'send-keys -t other-session:fm-design' "$LOG" "send targeted a foreign same-named window" - pass "send: a bare fm- routes to the window recorded in this home's meta" + pass "send: a bare fm- secondmate routes to the meta window with the from-firstmate marker" } phase_handoff() { diff --git a/tests/fm-send-secondmate-marker.test.sh b/tests/fm-send-secondmate-marker.test.sh new file mode 100755 index 0000000000..442b6a6024 --- /dev/null +++ b/tests/fm-send-secondmate-marker.test.sh @@ -0,0 +1,180 @@ +#!/usr/bin/env bash +# fm-send from-firstmate marker for secondmate targets. +# +# A secondmate is itself a firstmate, so a request relayed to it lands in its own +# chat - which the main firstmate never reads (the only channel back is the terse +# status file). fm-send therefore prepends a from-firstmate marker +# (bin/fm-marker-lib.sh) when, and only when, the resolved target is a bare +# `fm-` whose meta records kind=secondmate, so the secondmate can recognize +# the request and route its reply via the status path. These tests pin that +# behavior hermetically (stubbed tmux, no real agent): +# 1. A send to a kind=secondmate target prepends the marker to the literal text. +# 2. A send to a crewmate (kind=ship) target sends the bare text, no marker. +# 3. An explicit session:window target (no meta) is never marked. +# 4. The --key path never carries the marker. +# 5. The marker is exactly the label "[fm-from-firstmate]" + ASCII 0x1f, and the +# fm_message_from_firstmate detector keys on that untypable sequence. +set -u + +# shellcheck source=tests/lib.sh +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" +# shellcheck source=bin/fm-marker-lib.sh +. "$ROOT/bin/fm-marker-lib.sh" + +SEND="$ROOT/bin/fm-send.sh" + +TMP_ROOT=$(fm_test_tmproot fm-send-marker) + +# A fake tmux that (a) records the literal text of every `send-keys -l` to +# FM_SEND_LOG and (b) lets fm-send's submit path reach a clean "empty" verdict. +# display-message yields a numeric cursor_y; capture-pane returns an empty +# bordered composer so fm_tmux_composer_state reads "empty" (submit landed) on the +# first Enter. Only the literal (-l) text is logged; Enter retries and --key sends +# are not, so the log holds exactly what was typed into the composer. +make_stubs() { # -> echoes fakebin dir + local dir=$1 fb="$1/fakebin" + mkdir -p "$fb" + cat > "$fb/tmux" <<'SH' +#!/usr/bin/env bash +set -u +case "${1:-}" in + send-keys) + shift + literal=0 + while [ $# -gt 0 ]; do + case "$1" in + -t) shift 2 ;; + -l) literal=1; shift ;; + *) break ;; + esac + done + if [ "$literal" = 1 ]; then + printf '%s' "${1:-}" >> "$FM_SEND_LOG" + fi + exit 0 ;; + display-message) + for a in "$@"; do case "$a" in *cursor_y*) printf '0\n'; exit 0 ;; esac; done + printf 'fakepane\n'; exit 0 ;; + capture-pane) printf '\xe2\x94\x82 \xe2\x94\x82\n'; exit 0 ;; + list-windows) exit 0 ;; +esac +exit 0 +SH + chmod +x "$fb/tmux" + cat > "$fb/sleep" <<'SH' +#!/usr/bin/env bash +exit 0 +SH + chmod +x "$fb/sleep" + printf '%s\n' "$fb" +} + +# run_send -- +# Runs fm-send.sh with the stubs on PATH against the given home (which holds +# state/.meta). FM_ROOT_OVERRIDE points at the same non-repo home so +# fm-guard's tangle check stays silent; guard noise goes to stderr (discarded). +# FM_SEND_SETTLE=0 keeps the run fast. Truncates the log first; returns fm-send's +# exit code. +run_send() { + local fb=$1 home=$2 log=$3; shift 3 + : > "$log" + env PATH="$fb:$PATH" \ + FM_ROOT_OVERRIDE="$home" FM_HOME="$home" FM_SEND_LOG="$log" FM_SEND_SETTLE=0 \ + "$SEND" "$@" 2>/dev/null +} + +# setup_home -> echoes a fresh home dir with an empty state/. +setup_home() { + local home="$TMP_ROOT/$1-$RANDOM" + mkdir -p "$home/state" + printf '%s\n' "$home" +} + +test_secondmate_target_is_marked() { + local dir fb log home rc got + dir="$TMP_ROOT/sm"; mkdir -p "$dir" + fb=$(make_stubs "$dir"); log="$dir/send.log" + home=$(setup_home sm) + fm_write_secondmate_meta "$home/state/domain.meta" "$home" "sess:fm-domain" + run_send "$fb" "$home" "$log" "fm-domain" "audit the build"; rc=$? + expect_code 0 "$rc" "send to a secondmate target should succeed" + got=$(cat "$log") + case "$got" in + "$FM_FROMFIRST_MARK"audit\ the\ build) : ;; + *) fail "secondmate send: literal text should be marker+text"$'\n'"--- bytes ---"$'\n'"$(printf '%s' "$got" | od -An -c)" ;; + esac + pass "fm-send: a kind=secondmate target gets the from-firstmate marker prepended" +} + +test_crewmate_target_is_not_marked() { + local dir fb log home rc got + dir="$TMP_ROOT/crew"; mkdir -p "$dir" + fb=$(make_stubs "$dir"); log="$dir/send.log" + home=$(setup_home crew) + fm_write_meta "$home/state/build.meta" \ + "window=sess:fm-build" "worktree=$home/wt" "project=$home/p" \ + "harness=echo" "kind=ship" "mode=no-mistakes" "yolo=off" + run_send "$fb" "$home" "$log" "fm-build" "fix the test"; rc=$? + expect_code 0 "$rc" "send to a crewmate target should succeed" + got=$(cat "$log") + [ "$got" = "fix the test" ] \ + || fail "crewmate send: expected bare text, got marker or other"$'\n'"--- bytes ---"$'\n'"$(printf '%s' "$got" | od -An -c)" + pass "fm-send: a kind=ship (crewmate) target is sent unmarked" +} + +test_explicit_window_is_not_marked() { + local dir fb log home rc got + dir="$TMP_ROOT/explicit"; mkdir -p "$dir" + fb=$(make_stubs "$dir"); log="$dir/send.log" + home=$(setup_home explicit) + # No meta lookup happens for an explicit session:window target, so even with a + # same-named secondmate meta present it must stay unmarked (escape hatch). + fm_write_secondmate_meta "$home/state/win.meta" "$home" "other:win" + run_send "$fb" "$home" "$log" "other:win" "ping"; rc=$? + expect_code 0 "$rc" "send to an explicit window should succeed" + got=$(cat "$log") + [ "$got" = "ping" ] \ + || fail "explicit session:window send: expected bare text, got marker"$'\n'"--- bytes ---"$'\n'"$(printf '%s' "$got" | od -An -c)" + pass "fm-send: an explicit session:window target is never marked" +} + +test_key_path_is_not_marked() { + local dir fb log home rc + dir="$TMP_ROOT/key"; mkdir -p "$dir" + fb=$(make_stubs "$dir"); log="$dir/send.log" + home=$(setup_home key) + fm_write_secondmate_meta "$home/state/domain.meta" "$home" "sess:fm-domain" + run_send "$fb" "$home" "$log" "fm-domain" --key Escape; rc=$? + expect_code 0 "$rc" "--key send to a secondmate should succeed" + [ ! -s "$log" ] \ + || fail "--key path logged a literal send (marker leaked into a keypress)"$'\n'"--- bytes ---"$'\n'"$(od -An -c "$log")" + pass "fm-send: the --key path carries no marker (no literal text is typed)" +} + +test_marker_is_label_plus_unit_separator() { + local us hex + us=$(printf '\037') + [ "$FM_FROMFIRST_MARK" = "[fm-from-firstmate]$us" ] \ + || fail "marker is not the expected label + 0x1f sequence"$'\n'"--- bytes ---"$'\n'"$(printf '%s' "$FM_FROMFIRST_MARK" | od -An -c)" + # The last byte must be ASCII unit separator 0x1f, the untypable guarantee. + hex=$(printf '%s' "$FM_FROMFIRST_MARK" | od -An -tx1 | tr -d ' \n') + case "$hex" in + *1f) : ;; + *) fail "marker does not end in a 0x1f byte; bytes were: $hex" ;; + esac + # The detector keys on that exact untypable sequence. + fm_message_from_firstmate "${FM_FROMFIRST_MARK}do the work" \ + || fail "detector should recognize a marked message" + fm_message_from_firstmate "do the work" \ + && fail "detector must reject an unmarked message" + # The bare label without the separator (the typable part) is NOT a match. + fm_message_from_firstmate "[fm-from-firstmate]do the work" \ + && fail "detector must reject the label without the 0x1f separator" + pass "fm-send: the marker is exactly '[fm-from-firstmate]' + ASCII 0x1f, detector keys on it" +} + +test_secondmate_target_is_marked +test_crewmate_target_is_not_marked +test_explicit_window_is_not_marked +test_key_path_is_not_marked +test_marker_is_label_plus_unit_separator From 37c694cf4a79b02aadb6a0e851ae702065958dbb Mon Sep 17 00:00:00 2001 From: Kun Chen <3233006+kunchenguid@users.noreply.github.com> Date: Fri, 26 Jun 2026 12:11:12 -0700 Subject: [PATCH 002/167] fix: recognize squash-merged work as landed (#96) * fix(teardown): recognize squash-merged work as landed fm-teardown refused to tear down a worktree whose PR was squash-merged and whose head branch was then deleted - the most common GitHub flow - because its safety check used commit reachability (HEAD --not --remotes) rather than whether the work actually landed. A squash merge writes one new commit on the default branch, so the branch's own commits are on no remote-tracking branch, and auto-delete-on-merge drops the head ref too; reachability was non-empty and teardown false-refused merged work. For a normal ship task whose commits are not remote-reachable, before refusing, treat the work as landed if either its PR is merged (resolved from meta pr= or the branch name via gh-axi; authoritative for squash, rebase, and merge alike, and surviving branch deletion) or its content is already in the up-to-date default branch (a 3-way merge-tree that adds nothing the default branch lacks - robust to the default having advanced past the merge-base). Dirty worktrees and genuinely unlanded work still refuse; a gh lookup error falls back to the content check and, if that is inconclusive, refuses (fail-safe, never silently allows). Fork, local-only, scout, secondmate, and --force paths are unchanged. Tests cover squash-merged+deleted-branch (allow), genuinely unlanded (refuse), dirty (refuse), normally pushed (allow), content-in-default fallback (allow), and gh-error+content-absent (refuse, fail-safe). * no-mistakes(review): Guard teardown against stale PR proof * no-mistakes(review): Verify teardown PR heads * no-mistakes(document): Sync teardown landing docs * no-mistakes: apply CI fixes --- AGENTS.md | 14 +- CONTRIBUTING.md | 2 +- bin/fm-pr-check.sh | 30 +++- bin/fm-promote.sh | 2 +- bin/fm-teardown.sh | 129 ++++++++++++++-- docs/architecture.md | 3 + docs/scripts.md | 4 +- tests/fm-teardown.test.sh | 309 ++++++++++++++++++++++++++++++++++++-- 8 files changed, 453 insertions(+), 40 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 257804159a..f41139ba96 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -32,7 +32,8 @@ Hard rules, in priority order: The one standing, captain-authorized relaxation is a project's `yolo` flag (section 7): with `yolo` on, firstmate makes routine approval decisions itself, but anything destructive, irreversible, or security-sensitive still escalates to the captain. 3. **Never tear down a worktree that holds unlanded work.** `bin/fm-teardown.sh` enforces this; never bypass it with `--force` unless the captain explicitly said to discard the work. - The work is "landed" once `HEAD` is reachable from any remote-tracking branch (a fork counts as a remote - upstream-contribution PRs pushed to a fork satisfy this in any mode); for `local-only` ship tasks with no remote at all, the work may instead be merged into the local default branch. + The work is "landed" once `HEAD` is reachable from any remote-tracking branch (a fork counts as a remote - upstream-contribution PRs pushed to a fork satisfy this in any mode); for a normal ship task whose commits are not so reachable, it is also landed when its PR is merged and GitHub reports the current worktree HEAD as that PR's head (which covers the common squash-merge-then-delete-branch flow, where the branch's commits live nowhere on a remote yet the recorded work merged) or when its content is already present in the up-to-date default branch; for `local-only` ship tasks with no remote at all, the work may instead be merged into the local default branch. + Uncommitted changes are never landed. The scout carve-out: a scout task's worktree is declared scratch from the start - its deliverable is the report, and teardown lets the worktree go once that report exists (section 7). 4. **Crewmates never address the captain.** All crewmate communication flows through you. @@ -81,7 +82,7 @@ projects/ cloned repos; gitignored; READ-ONLY for you state/ volatile runtime signals; gitignored .status appended by crewmates: ": " lines .turn-ended touched by turn-end hooks - .meta written by fm-spawn: window=, worktree=, project=, harness=, kind=, mode=, yolo=; kind=secondmate also records home= and projects= (fm-pr-check appends pr=) + .meta written by fm-spawn: window=, worktree=, project=, harness=, kind=, mode=, yolo=; kind=secondmate also records home= and projects= (fm-pr-check appends pr= and verified pr_head= when available) .check.sh optional slow poll you write per task (e.g. merged-PR check) .wake-queue durable queued wakes: epochseqkindkeypayload .afk durable away-mode flag; present = sub-supervisor may inject escalations (set by /afk, cleared on user return) @@ -356,7 +357,7 @@ Because `fm-send` to a `kind=secondmate` target marks the request as from-firstm A ship task's path from `done` to landed on `main` is set by the project's `mode` (recorded in meta; section 6); `yolo` decides who approves. The Validate / PR ready / Ship teardown stages below are written for the `no-mistakes` path; the other modes diverge: - **no-mistakes** - the stages below as written: no-mistakes validation pipeline -> PR -> captain merge. -- **direct-PR** - no pipeline. The crewmate pushes and opens the PR itself (its brief says so) and reports `done: PR `. Skip the Validate step and go straight to PR ready (run `fm-pr-check`, relay the PR). Teardown uses the normal pushed-branch check. +- **direct-PR** - no pipeline. The crewmate pushes and opens the PR itself (its brief says so) and reports `done: PR `. Skip the Validate step and go straight to PR ready (run `fm-pr-check`, relay the PR). Teardown uses the normal landed-work check. - **local-only** - no remote, no PR. The crewmate stops at `done: ready in branch fm/`. Review the diff with `bin/fm-review-diff.sh `, relay a one-paragraph summary to the captain, and on approval run `bin/fm-merge-local.sh ` to fast-forward local `main` (it refuses anything but a clean fast-forward - if it does, have the crewmate rebase). No `fm-pr-check`. Then teardown, whose safety check requires the branch already merged into local `main`, OR the work pushed to any remote (a fork counts - relevant for upstream-contribution PRs on a local-only-registered project). When reviewing any crewmate branch diff, use `bin/fm-review-diff.sh ` rather than `git diff ...branch` directly. @@ -377,7 +378,7 @@ Use chat for yes/no decisions; use lavish-axi when there are multiple findings o ### PR ready For PR-based ship tasks, the ready signal depends on mode: `no-mistakes` reports `done: PR checks green` after CI is green, while `direct-PR` reports `done: PR ` after opening the PR. -Run `bin/fm-pr-check.sh ` - it records `pr=` in the task's meta and arms the watcher's merge poll. +Run `bin/fm-pr-check.sh ` - it records `pr=` and a verified `pr_head=` when available in the task's meta and arms the watcher's merge poll. Tell the captain: the PR's full URL (always the complete `https://...` link, never a bare `#number` - the captain's terminal makes a full URL clickable), a one-paragraph summary, and, for `no-mistakes`, the risk level it emitted. (The check contract, for any custom `state/.check.sh` you write yourself: print one line only when firstmate should wake, print nothing otherwise, and finish before `FM_CHECK_TIMEOUT`.) @@ -389,7 +390,10 @@ If the captain says "merge it", run `gh-axi pr merge` yourself; that instruction bin/fm-teardown.sh ``` -The script refuses if the worktree holds unpushed work; treat a refusal as a stop-and-investigate, not an obstacle. +The script refuses if the worktree holds uncommitted changes or committed work that has not landed; treat a refusal as a stop-and-investigate, not an obstacle. +"Landed" is broader than remote-reachable: for a normal ship task whose commits are not reachable from any remote-tracking branch, the script also accepts the work when its PR is merged and GitHub reports the current worktree HEAD as that PR's head, or when its content is already present in the up-to-date default branch. +This recognizes the common squash-merge-then-delete-branch flow, where the branch's own commits live nowhere on a remote yet the change is fully in `main`; a merged-and-deleted branch now tears down cleanly instead of false-refusing. +Genuinely unlanded work (no matching merged PR head and content not in the default branch) and dirty worktrees still refuse, and a gh lookup error falls back to the content check rather than silently allowing. Known benign case: after an external-PR task, a squash merge leaves the branch commits reachable only on the contributor's fork; add the fork as a remote and fetch (`git remote add fork && git fetch fork`), then retry - never reach for `--force`. After a successful PR-based teardown, it also runs `bin/fm-fleet-sync.sh` for that project, best-effort, so the clone's local default catches up to the merge and the just-merged branch, now gone on the remote and free of its worktree, is pruned immediately. Then update the backlog using the teardown reminder: run `tasks-axi done` when the compatible tool is available, otherwise move the task to Done in `data/backlog.md` manually with the full `https://...` PR URL or local merge note and date and keep Done to the 10 most recent. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 17d306c571..0d0a72bc51 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -74,7 +74,7 @@ tests/fm-update.test.sh # fast-forward-only self-update, rerea tests/fm-secondmate-sync.test.sh # local-HEAD secondmate sync, no-fetch, bootstrap nudge gating, and spawn hook tests tests/fm-secondmate-lifecycle-e2e.test.sh # persistent secondmate routing, seeding, backlog handoff, spawn, recovery, teardown, and FM_HOME flow tests tests/fm-secondmate-safety.test.sh # secondmate home safety, idle charter, handoff validation, and teardown boundary tests -tests/fm-teardown.test.sh # fm-teardown.sh safety and reminder checks: local-only fork-remote allow, truly-unpushed refuse, merged-to-main allow, no-mistakes regression, tasks-axi reminder, --force override +tests/fm-teardown.test.sh # fm-teardown.sh landed-work safety and reminder checks: fork-remote allow, squash/content landings, dirty and unlanded refusals, PR-head metadata, tasks-axi reminder, --force override [ "$(readlink CLAUDE.md)" = "AGENTS.md" ] [ "$(readlink .claude/skills)" = "../.agents/skills" ] FM_HEARTBEAT=2 FM_POLL=1 bin/fm-watch-arm.sh # watcher re-arm smoke test (prints arm status, then "heartbeat") diff --git a/bin/fm-pr-check.sh b/bin/fm-pr-check.sh index 928226e3b1..4271654f9d 100755 --- a/bin/fm-pr-check.sh +++ b/bin/fm-pr-check.sh @@ -1,8 +1,8 @@ #!/usr/bin/env bash -# Record a PR-ready task: appends pr= to state/.meta and arms the -# watcher's merge poll by writing state/.check.sh, which prints one line iff -# the PR is merged (the watcher's check contract: output = wake firstmate, -# silence = keep sleeping). +# Record a PR-ready task: appends pr= and a verified pr_head= to +# state/.meta when available, then arms the watcher's merge poll by writing +# state/.check.sh, which prints one line iff the PR is merged (the watcher's +# check contract: output = wake firstmate, silence = keep sleeping). # Usage: fm-pr-check.sh set -eu @@ -15,8 +15,26 @@ ID=$1 URL=$2 META="$STATE/$ID.meta" -if [ -f "$META" ] && ! grep -qxF "pr=$URL" "$META"; then - echo "pr=$URL" >> "$META" +if [ -f "$META" ]; then + WT=$(grep '^worktree=' "$META" | tail -1 | cut -d= -f2- || true) + LOCAL_HEAD= + PR_HEAD= + if [ -n "$WT" ] && [ -d "$WT" ]; then + LOCAL_HEAD=$(git -C "$WT" rev-parse --verify HEAD 2>/dev/null || true) + if [ -n "$LOCAL_HEAD" ] && command -v gh >/dev/null 2>&1; then + if REMOTE_HEAD=$(cd "$WT" && gh pr view "$URL" --json headRefOid -q .headRefOid 2>/dev/null); then + if [ "$LOCAL_HEAD" = "$REMOTE_HEAD" ]; then + PR_HEAD=$LOCAL_HEAD + fi + fi + fi + fi + if ! grep -qxF "pr=$URL" "$META"; then + echo "pr=$URL" >> "$META" + fi + if [ -n "$PR_HEAD" ] && ! grep -qxF "pr_head=$PR_HEAD" "$META"; then + echo "pr_head=$PR_HEAD" >> "$META" + fi fi cat > "$STATE/$ID.check.sh" <.meta so fm-teardown.sh applies the full unpushed-work protection +# state/.meta so fm-teardown.sh applies the full ship-task teardown protection # again. After promoting, send the crewmate its ship instructions via fm-send.sh # (inventory scratch state, reset to a clean default-branch base, carry over only # intended fix changes, create branch fm/, implement, then report done diff --git a/bin/fm-teardown.sh b/bin/fm-teardown.sh index ddd0a6de3e..e08e448659 100755 --- a/bin/fm-teardown.sh +++ b/bin/fm-teardown.sh @@ -3,9 +3,18 @@ # secondmate home, kill the tmux window, clear volatile state, refresh/prune # the project's clone for PR-based ship tasks, then print a backlog-refresh # reminder. -# REFUSES if the worktree holds work not on any remote, because treehouse return -# hard-resets the worktree and kills its processes. A fork counts as a remote, -# so upstream-contribution PRs pushed to a fork satisfy this in any mode. +# REFUSES if the worktree holds work that has not LANDED, because treehouse return +# hard-resets the worktree and kills its processes. Work has landed when it is +# reachable from any remote-tracking branch (a fork counts as a remote, so +# upstream-contribution PRs pushed to a fork satisfy this in any mode), OR - for a +# normal ship task whose commits are not so reachable - when its PR is merged and +# GitHub reports the current HEAD as that PR's head, or its content is already +# present in the up-to-date default branch. This recognizes the common +# squash-merge-then-delete-branch flow, where the branch's own commits live nowhere +# on a remote yet the change is fully in main. +# A gh lookup error falls back to the content check; if that is also inconclusive, +# teardown refuses rather than risk discarding unlanded work. +# Uncommitted changes are never landed. # local-only projects additionally accept work merged into the local default # branch (firstmate performs that merge on the captain's approval) as a fallback # for the common case where there is no remote at all. @@ -20,9 +29,9 @@ # never left leased forever. If the treehouse return fails, teardown leaves the # leased home and state in place instead of hiding a still-held lease. # Usage: fm-teardown.sh [--force] -# --force skips the unpushed-work check for ordinary tasks and discards -# secondmate child work for kind=secondmate. Only use it when the captain has -# explicitly said to discard the work. +# --force skips ordinary-task dirty and landed-work checks, skips scout report +# checks, and discards secondmate child work for kind=secondmate. Only use it +# when the captain has explicitly said to discard the work. set -eu SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" @@ -72,6 +81,79 @@ meta_value() { grep "^$key=" "$meta" | cut -d= -f2- || true } +# Resolve the PR number for a worktree branch via gh-axi. Echoes the number on a +# single match and returns 0; returns non-zero on no match or any lookup failure, +# so the caller treats it as "no PR found" (fail-safe). +pr_number_from_branch() { + local branch=$1 out n + [ -n "$branch" ] && [ "$branch" != HEAD ] || return 1 + out=$( cd "$WT" && gh-axi pr list --state all --head "$branch" --limit 1 2>/dev/null ) || return 1 + n=$(printf '%s\n' "$out" | sed -n 's/^[[:space:]]*\([0-9][0-9]*\),.*/\1/p' | head -1) + [ -n "$n" ] || return 1 + printf '%s' "$n" +} + +# Is the worktree's PR merged for this exact HEAD? Resolves the PR from the +# recorded pr= URL first, then from the branch name, and asks GitHub for both the +# PR state and head. Returns non-zero when the PR is not merged, the current HEAD +# is not the PR head, no PR is found, or any gh error occurs - the caller then +# falls back to the content check. +pr_is_merged() { + local branch=$1 target view state head current + if [ -n "$PR_URL" ]; then + target=$PR_URL + else + target=$(pr_number_from_branch "$branch") || return 1 + fi + [ -n "$target" ] || return 1 + view=$(cd "$WT" && gh pr view "$target" --json state,headRefOid -q '.state + "\t" + .headRefOid' 2>/dev/null) || return 1 + state=${view%%$'\t'*} + head=${view#*$'\t'} + [ "$state" != "$view" ] || return 1 + case "$state" in + MERGED|merged) ;; + *) return 1 ;; + esac + [ -n "$head" ] || return 1 + current=$(git -C "$WT" rev-parse --verify HEAD 2>/dev/null) || return 1 + [ "$current" = "$head" ] +} + +# Is the branch's content already present in the up-to-date default branch? Fetches +# first, then 3-way merges the default branch with HEAD: when HEAD introduces nothing +# the default branch does not already contain (e.g. its change landed via squash) the +# merged tree equals the default branch's tree. This isolates branch-only changes, so +# unrelated commits the default branch gained past the merge-base do not count as +# "added". Returns non-zero when inconclusive (no default ref, or a merge conflict), +# so the caller refuses rather than guesses. +content_in_default() { + local name ref default_tree merged_tree + name=$(default_branch) || return 1 + if git -C "$WT" remote get-url origin >/dev/null 2>&1; then + git -C "$WT" fetch --quiet origin "+refs/heads/$name:refs/remotes/origin/$name" >/dev/null 2>&1 || return 1 + ref="refs/remotes/origin/$name" + elif git -C "$WT" rev-parse --quiet --verify "refs/heads/$name" >/dev/null 2>&1; then + ref="refs/heads/$name" + else + return 1 + fi + default_tree=$(git -C "$WT" rev-parse --quiet --verify "$ref^{tree}" 2>/dev/null) || return 1 + [ -n "$default_tree" ] || return 1 + merged_tree=$(git -C "$WT" merge-tree --write-tree "$ref" HEAD 2>/dev/null) || return 1 + merged_tree=$(printf '%s\n' "$merged_tree" | head -1) + [ "$merged_tree" = "$default_tree" ] +} + +# Has the worktree's committed work actually LANDED, though its commits are not +# reachable from any remote-tracking branch? True when a merged PR proves the +# current HEAD, OR the content is already in the default branch (fallback, which +# also covers the no-PR and gh-error paths). False only for genuinely unlanded work. +work_is_landed() { + local branch=$1 + pr_is_merged "$branch" && return 0 + content_in_default +} + backlog_refresh_reminder() { local pr done_cmd report_path if fm_tasks_axi_compatible; then @@ -429,9 +511,14 @@ if [ -d "$WT" ] && [ "$FORCE" != "--force" ]; then else # The fm-spawn hook file is ours, never work product; ignore it in the dirty check. dirty=$(git -C "$WT" status --porcelain 2>/dev/null | grep -vE '^\?\? \.claude/' | head -1 || true) - # A worktree's work is "safely on a remote" once HEAD is reachable from ANY - # remote-tracking branch (empty result here). A fork is a remote too, so - # upstream-contribution PRs pushed to a fork satisfy this regardless of mode. + # Reachability test: is HEAD reachable from ANY remote-tracking branch? Empty + # means the work is already pushed (a fork is a remote too, so upstream- + # contribution PRs pushed to a fork pass here). Non-empty does NOT prove the work + # is unlanded: a squash or rebase merge rewrites the branch into a new commit on + # the default branch, and a repo that auto-deletes the head branch on merge also + # drops its remote-tracking ref - so a merged-and-deleted branch trips this test + # while being fully landed. We therefore treat reachability as a fast accept, not + # the sole verdict, and fall through to a landed-work check before refusing. unpushed=$(git -C "$WT" log --oneline HEAD --not --remotes -- 2>/dev/null | head -5 || true) if [ -n "$unpushed" ] && [ "$MODE" = local-only ]; then # local-only ships have no remote in the common case, so the "on a remote" @@ -447,12 +534,26 @@ if [ -d "$WT" ] && [ "$FORCE" != "--force" ]; then echo "Merge the branch into local $DEFAULT first (bin/fm-merge-local.sh after the captain approves), or push to a fork/remote, or get the captain's explicit OK to discard, then --force." >&2 exit 1 fi - elif [ -n "$dirty" ] || [ -n "$unpushed" ]; then - echo "REFUSED: worktree $WT has work not on any remote." >&2 - [ -n "$dirty" ] && echo "uncommitted changes present" >&2 - [ -n "$unpushed" ] && printf 'unpushed commits:\n%s\n' "$unpushed" >&2 - echo "Push the branch (or get the captain's explicit OK to discard, then --force)." >&2 + elif [ -n "$dirty" ]; then + # Uncommitted changes are never landed and the reset would discard them; always + # refuse, regardless of whether the committed work itself has landed. + echo "REFUSED: worktree $WT has uncommitted changes." >&2 + echo "uncommitted changes present" >&2 + echo "Commit them (or get the captain's explicit OK to discard, then --force)." >&2 exit 1 + elif [ -n "$unpushed" ]; then + # Commits not reachable from any remote. Before refusing, recognize LANDED work: + # a merged PR for the current HEAD or content already in the up-to-date default + # branch. On a gh lookup error work_is_landed falls back to the content check, + # and if that is also inconclusive it returns false - so we never silently allow + # teardown of possibly-unlanded work; only genuinely unlanded work is refused. + branch=$(git -C "$WT" rev-parse --abbrev-ref HEAD 2>/dev/null || echo HEAD) + if ! work_is_landed "$branch"; then + echo "REFUSED: worktree $WT has work not on any remote and not landed." >&2 + printf 'unpushed commits:\n%s\n' "$unpushed" >&2 + echo "Push the branch, land its PR, or get the captain's explicit OK to discard, then --force." >&2 + exit 1 + fi fi fi fi diff --git a/docs/architecture.md b/docs/architecture.md index 0a891f3f4f..82e7d849da 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -68,6 +68,9 @@ The `data/secondmates.md` line schema and the secondmate environment variables a `data/projects.md` records each project's delivery mode and optional `+yolo` autonomy flag. `no-mistakes` projects run the full validation pipeline, `direct-PR` projects open PRs without that pipeline, and `local-only` projects stay local until firstmate performs an approved fast-forward merge. +Teardown is fail-closed for ship worktrees: dirty worktrees refuse, and committed work must be landed before the worktree is returned. +Landed work is accepted when `HEAD` is reachable from any remote-tracking branch, when a PR for the current `HEAD` is merged, or when the worktree content is already present in the freshly fetched default branch. +That content check lets a squash-merged PR whose head branch was deleted tear down cleanly without using `--force`; `local-only` work instead tears down after the approved local default-branch merge or after the branch is pushed to any remote. ## Project memory belongs to projects diff --git a/docs/scripts.md b/docs/scripts.md index 5aa092a32c..6aa0007c7e 100644 --- a/docs/scripts.md +++ b/docs/scripts.md @@ -29,8 +29,8 @@ Each file also starts with a short header comment. | `fm-send.sh` | Send one verified literal line (or `--key Escape`) to a direct-report window; exits non-zero on confirmed swallowed Enter; bare `kind=secondmate` targets are marked as from-firstmate; text sends pause `FM_SEND_SETTLE` seconds after success | | `fm-tmux-lib.sh` | Shared tmux pane primitives for busy detection, dim-ghost-aware and border-aware composer detection, and verified submit retry | | `fm-peek.sh` | Print a bounded tail of a crewmate pane | -| `fm-pr-check.sh` | Record a PR-ready task and arm the watcher's merge poll | +| `fm-pr-check.sh` | Record `pr=` and a verified `pr_head=` when available for a PR-ready task, then arm the watcher's merge poll | | `fm-promote.sh` | Promote a scout task in place so it becomes a protected ship task | -| `fm-teardown.sh` | Return the worktree or retire/release a secondmate home; protects ship work, requires scout reports, checks child work, and prints the backlog reminder | +| `fm-teardown.sh` | Return a clean, landed ship worktree or retire/release a secondmate home; requires scout reports, checks child work, and prints the backlog reminder | | `fm-harness.sh` | Detect the running harness; resolve the effective crewmate harness | | `fm-lock.sh` | Per-home firstmate session lock | diff --git a/tests/fm-teardown.test.sh b/tests/fm-teardown.test.sh index f985b55487..e5cb13551f 100755 --- a/tests/fm-teardown.test.sh +++ b/tests/fm-teardown.test.sh @@ -1,24 +1,42 @@ #!/usr/bin/env bash -# Tests for bin/fm-teardown.sh's unpushed-work safety check. +# Tests for bin/fm-teardown.sh's landed-work safety check. # -# Covers the local-only fork-remote fix: a local-only-registered project whose -# task pushes its work to a fork (upstream-contribution PRs) must be teardown- -# eligible because a fork IS a remote. The pre-fix code short-circuited to a -# strict local-main check and false-refused legitimate fork-pushed work. +# The check refuses to tear down a worktree whose work has not LANDED, because +# treehouse return hard-resets the worktree. "Landed" means reachable from a remote +# OR - for a normal ship task whose commits are not so reachable - its PR is merged +# and GitHub reports the current HEAD as that PR's head, or its content is already +# in the up-to-date default branch. +# +# Covers two fixes: +# - local-only fork-remote: a fork IS a remote, so fork-pushed upstream- +# contribution PRs are teardown-eligible (the pre-fix code false-refused them). +# - squash-merge-then-delete-branch: the branch's own commits live nowhere on a +# remote after a squash merge deletes the head branch, yet the change is fully in +# main. Reachability alone false-refused this common GitHub flow; the check now +# recognizes the matching merged PR head (or the content already in main) as +# landed. # # Matrix: -# (a) local-only + HEAD on a fork remote-tracking branch -> ALLOW (the fix) +# (a) local-only + HEAD on a fork remote-tracking branch -> ALLOW (fork fix) # (b) local-only + truly unpushed work (no remote, not main) -> REFUSE (safety) # (c) local-only + merged into local main, no remote -> ALLOW (no regression) -# (d) no-mistakes + HEAD on origin remote-tracking branch -> ALLOW (no regression) -# (e) no-mistakes + truly unpushed work -> REFUSE (no regression) +# (d) no-mistakes + HEAD on origin remote-tracking branch -> ALLOW (no regression) +# (e) no-mistakes + unpushed, no PR, content not in default -> REFUSE (safety) # (f) local-only + truly unpushed + --force -> ALLOW (escape hatch) +# (g) no-mistakes + squash-merged PR, branch-deleted -> ALLOW (squash fix) +# (h) no-mistakes + no PR but content already in default -> ALLOW (content fallback) +# (i) no-mistakes + dirty worktree, even when work landed -> REFUSE (dirty wins) +# (j) no-mistakes + gh lookup errors + content not in default -> REFUSE (fail-safe) +# (k) no-mistakes + merged PR but HEAD moved afterward -> REFUSE (stale PR) +# (l) no-mistakes + stale origin/main but fetched content -> ALLOW (fresh fetch) +# (m) fm-pr-check rerun after HEAD moved -> no stale pr_head set -u # shellcheck source=tests/lib.sh . "$(dirname "${BASH_SOURCE[0]}")/lib.sh" TEARDOWN="$ROOT/bin/fm-teardown.sh" +PR_CHECK="$ROOT/bin/fm-pr-check.sh" TMP_ROOT=$(fm_test_tmproot fm-teardown-tests) # Build a fresh sandbox for one test case. Sets up: @@ -46,7 +64,26 @@ SH # tmux kill-window etc.: succeed silently. exit 0 SH - chmod +x "$fakebin/treehouse" "$fakebin/tmux" + # Default gh-axi mock: no PR is associated with the branch, and viewing any PR + # number fails. This keeps the landed-work check hermetic (never reaching the real + # gh-axi) and represents the common "no GitHub PR" baseline. Tests that need a + # merged PR or a lookup error override this file with the helpers below. + cat > "$fakebin/gh-axi" <<'SH' +#!/usr/bin/env bash +case "${1:-} ${2:-}" in + "pr list") printf '%s\n' "count: 0 (showing first 0)" "pull_requests[]: []" ; exit 0 ;; + "pr view") echo "error: pull request not found" >&2 ; exit 1 ;; +esac +exit 0 +SH + cat > "$fakebin/gh" <<'SH' +#!/usr/bin/env bash +case "${1:-} ${2:-}" in + "pr view") echo "error: pull request not found" >&2 ; exit 1 ;; +esac +exit 0 +SH + chmod +x "$fakebin/treehouse" "$fakebin/tmux" "$fakebin/gh-axi" "$fakebin/gh" # Bare origin so the clone has an `origin` remote and origin/HEAD. git init -q --bare "$case_dir/origin.git" @@ -112,6 +149,84 @@ add_fork_with_pushed_branch() { git -C "$case_dir/project" fetch -q fork } +# Commit a real file change on the worktree's task branch (unlike wt_commit, which +# makes an empty commit). A non-empty tree is what the content-in-default check +# inspects. Args: case_dir file content [message] +wt_commit_file() { + local case_dir=$1 file=$2 content=$3 msg=${4:-add $2} + printf '%s\n' "$content" > "$case_dir/wt/$file" + git -C "$case_dir/wt" add -- "$file" + git -C "$case_dir/wt" -c user.email=t@t -c user.name=t commit -q -m "$msg" +} + +# Land = as a single commit on origin's default branch, simulating a +# squash merge whose net change matches the task branch but whose commit differs. +# After this, the branch's content is in origin/main even though the branch's own +# commits are not reachable from it. Args: case_dir file content +land_on_origin_main() { + local case_dir=$1 file=$2 content=$3 tmp + tmp="$case_dir/_land" + git clone -q "$case_dir/origin.git" "$tmp" + printf '%s\n' "$content" > "$tmp/$file" + git -C "$tmp" add -- "$file" + git -C "$tmp" -c user.email=t@t -c user.name=t commit -q -m "squash $file" + git -C "$tmp" push -q origin HEAD:main + rm -rf "$tmp" +} + +# Override GitHub lookups to report PR 7 as merged with the supplied head. +add_gh_pr_merged_for_head() { + local case_dir=$1 head=$2 + cat > "$case_dir/fakebin/gh-axi" <<'SH' +#!/usr/bin/env bash +case "${1:-} ${2:-}" in + "pr list") + printf '%s\n' "count: 1 (showing first 1)" "pull_requests[1]{number,state}:" " 7,merged" ; exit 0 ;; + "pr view") + printf '%s\n' "pull_request:" " number: 7" " state: merged" ' merged: "2026-06-26T00:00:00Z"' ; exit 0 ;; +esac +exit 0 +SH + cat > "$case_dir/fakebin/gh" <&2 +exit 1 +SH + chmod +x "$case_dir/fakebin/gh-axi" "$case_dir/fakebin/gh" +} + +append_pr_meta_for_current_head() { + local case_dir=$1 head + head=$(git -C "$case_dir/wt" rev-parse HEAD) + printf '%s\n' \ + 'pr=https://github.com/example/repo/pull/7' \ + "pr_head=$head" >> "$case_dir/state/task-x1.meta" +} + +# Override gh-axi so every call fails, simulating an API/network error. +add_gh_axi_error() { + local case_dir=$1 + cat > "$case_dir/fakebin/gh-axi" <<'SH' +#!/usr/bin/env bash +echo "error: gh-axi unavailable" >&2 +exit 1 +SH + cat > "$case_dir/fakebin/gh" <<'SH' +#!/usr/bin/env bash +echo "error: gh unavailable" >&2 +exit 1 +SH + chmod +x "$case_dir/fakebin/gh-axi" "$case_dir/fakebin/gh" +} + # Run teardown with PATH mocking. Args: case_dir [extra args...] run_teardown() { local case_dir=$1; shift @@ -221,7 +336,9 @@ test_no_mistakes_truly_unpushed_refuses() { local case_dir rc case_dir=$(make_case nm-unpushed) write_meta "$case_dir" no-mistakes ship - wt_commit "$case_dir" "unpushed work" + # Real content that is not pushed, has no PR (default gh-axi mock), and never + # landed on origin/main: genuinely unlanded work that must still refuse. + wt_commit_file "$case_dir" feature.txt hello "unpushed work" set +e run_teardown "$case_dir" > "$case_dir/stdout" 2> "$case_dir/stderr" @@ -230,7 +347,170 @@ test_no_mistakes_truly_unpushed_refuses() { expect_code 1 "$rc" "nm-unpushed: teardown should refuse" grep -q REFUSED "$case_dir/stderr" || fail "nm-unpushed: no REFUSED line in stderr" - pass "no-mistakes worktree with truly unpushed work is refused (no regression)" + pass "no-mistakes worktree with genuinely unlanded work is refused (safety preserved)" +} + +test_squash_merged_branch_deleted_allows() { + local case_dir rc pr_head + case_dir=$(make_case squash-merged) + write_meta "$case_dir" no-mistakes ship + # Real branch content that is NOT pushed and NOT on origin/main: a squash merge + # rewrote it into a different commit on main and auto-deleted the head branch, so + # HEAD is unreachable from every remote-tracking branch. The matching merged PR is + # the only signal that the work landed. + wt_commit_file "$case_dir" feature.txt hello "add feature" + append_pr_meta_for_current_head "$case_dir" + pr_head=$(git -C "$case_dir/wt" rev-parse HEAD) + add_gh_pr_merged_for_head "$case_dir" "$pr_head" + + set +e + run_teardown "$case_dir" > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 0 "$rc" "squash-merged: teardown should succeed when the PR is merged" + ! grep -q REFUSED "$case_dir/stderr" || fail "squash-merged: teardown printed a REFUSED line" + pass "squash-merged + deleted-branch worktree (PR merged) is torn down (the fix)" +} + +test_merged_pr_with_later_local_commit_refuses() { + local case_dir rc pr_head + case_dir=$(make_case stale-pr-head) + write_meta "$case_dir" no-mistakes ship + wt_commit_file "$case_dir" feature.txt hello "add feature" + append_pr_meta_for_current_head "$case_dir" + pr_head=$(git -C "$case_dir/wt" rev-parse HEAD) + wt_commit_file "$case_dir" later.txt local-only "local follow-up" + add_gh_pr_merged_for_head "$case_dir" "$pr_head" + + set +e + run_teardown "$case_dir" > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "stale-pr-head: teardown should refuse when HEAD moved after PR recording" + grep -q REFUSED "$case_dir/stderr" || fail "stale-pr-head: no REFUSED line in stderr" + pass "merged PR does not allow teardown after a later local commit" +} + +test_pr_check_does_not_refresh_stale_pr_head() { + local case_dir rc pr_head new_head count + case_dir=$(make_case pr-check-stale) + write_meta "$case_dir" no-mistakes ship + wt_commit_file "$case_dir" feature.txt hello "add feature" + pr_head=$(git -C "$case_dir/wt" rev-parse HEAD) + add_gh_pr_merged_for_head "$case_dir" "$pr_head" + + FM_ROOT_OVERRIDE="$ROOT" \ + FM_STATE_OVERRIDE="$case_dir/state" \ + PATH="$case_dir/fakebin:$PATH" \ + "$PR_CHECK" task-x1 https://github.com/example/repo/pull/7 >/dev/null + + wt_commit_file "$case_dir" later.txt local-only "local follow-up" + new_head=$(git -C "$case_dir/wt" rev-parse HEAD) + + FM_ROOT_OVERRIDE="$ROOT" \ + FM_STATE_OVERRIDE="$case_dir/state" \ + PATH="$case_dir/fakebin:$PATH" \ + "$PR_CHECK" task-x1 https://github.com/example/repo/pull/7 >/dev/null + + count=$(grep -c '^pr_head=' "$case_dir/state/task-x1.meta" || true) + expect_code 1 "$count" "pr-check-stale: stale rerun should not append a second pr_head" + ! grep -qxF "pr_head=$new_head" "$case_dir/state/task-x1.meta" \ + || fail "pr-check-stale: stale rerun recorded the later local HEAD" + + set +e + run_teardown "$case_dir" > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "pr-check-stale: teardown should refuse after a later local commit" + grep -q REFUSED "$case_dir/stderr" || fail "pr-check-stale: no REFUSED line in stderr" + pass "fm-pr-check does not refresh PR head after HEAD moves" +} + +test_content_in_default_fallback_allows() { + local case_dir rc + case_dir=$(make_case content-landed) + write_meta "$case_dir" no-mistakes ship + # No pr= recorded and the default gh-axi mock reports no PR, so the merged-PR path + # cannot fire and the content check must carry it. The branch adds feature.txt, and + # the same net change has independently landed on origin/main via a squash commit. + wt_commit_file "$case_dir" feature.txt hello "add feature" + land_on_origin_main "$case_dir" feature.txt hello + + set +e + run_teardown "$case_dir" > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 0 "$rc" "content-landed: teardown should succeed when content is already in the default branch" + ! grep -q REFUSED "$case_dir/stderr" || fail "content-landed: teardown printed a REFUSED line" + pass "worktree whose content already landed in the default branch is torn down (content fallback)" +} + +test_content_fallback_refreshes_stale_origin_ref() { + local case_dir rc + case_dir=$(make_case content-stale-ref) + write_meta "$case_dir" no-mistakes ship + wt_commit_file "$case_dir" feature.txt hello "add feature" + git -C "$case_dir/project" config --unset-all remote.origin.fetch + git -C "$case_dir/project" config --add remote.origin.fetch '+refs/heads/not-main:refs/remotes/origin/not-main' + land_on_origin_main "$case_dir" feature.txt hello + + set +e + run_teardown "$case_dir" > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 0 "$rc" "content-stale-ref: teardown should use the freshly fetched default branch" + ! grep -q REFUSED "$case_dir/stderr" || fail "content-stale-ref: teardown printed a REFUSED line" + pass "content fallback refreshes origin default before comparing trees" +} + +test_dirty_worktree_refuses() { + local case_dir rc pr_head + case_dir=$(make_case dirty-wt) + write_meta "$case_dir" no-mistakes ship + printf '%s\n' 'pr=https://github.com/example/repo/pull/7' >> "$case_dir/state/task-x1.meta" + # The committed work has fully landed (merged PR + content in default), but an + # uncommitted edit remains. Dirtiness must refuse regardless: the reset would + # discard those changes. + wt_commit_file "$case_dir" feature.txt hello "add feature" + land_on_origin_main "$case_dir" feature.txt hello + pr_head=$(git -C "$case_dir/wt" rev-parse HEAD) + add_gh_pr_merged_for_head "$case_dir" "$pr_head" + printf '%s\n' "uncommitted edit" > "$case_dir/wt/feature.txt" + + set +e + run_teardown "$case_dir" > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "dirty-wt: teardown should refuse a dirty worktree even when the committed work has landed" + grep -q REFUSED "$case_dir/stderr" || fail "dirty-wt: no REFUSED line in stderr" + grep -q "uncommitted changes" "$case_dir/stderr" || fail "dirty-wt: refusal did not cite uncommitted changes" + pass "dirty worktree is refused even when its committed work has landed (dirty always wins)" +} + +test_gh_error_and_content_absent_refuses() { + local case_dir rc + case_dir=$(make_case gh-error) + write_meta "$case_dir" no-mistakes ship + printf '%s\n' 'pr=https://github.com/example/repo/pull/7' >> "$case_dir/state/task-x1.meta" + # Real content not pushed, the PR lookup errors, and origin/main never gained the + # content. The fail-safe must refuse rather than allow on a transient gh failure. + wt_commit_file "$case_dir" feature.txt hello "add feature" + add_gh_axi_error "$case_dir" + + set +e + run_teardown "$case_dir" > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "gh-error: teardown should refuse when the PR lookup errors and content is not landed" + grep -q REFUSED "$case_dir/stderr" || fail "gh-error: no REFUSED line in stderr" + pass "gh lookup error with content not in default refuses (fail-safe)" } test_local_only_force_overrides_unpushed() { @@ -256,3 +536,10 @@ 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_squash_merged_branch_deleted_allows +test_merged_pr_with_later_local_commit_refuses +test_pr_check_does_not_refresh_stale_pr_head +test_content_in_default_fallback_allows +test_content_fallback_refreshes_stale_origin_ref +test_dirty_worktree_refuses +test_gh_error_and_content_absent_refuses From fe3c867788fe389bbcaddbf9500c27cceb99504a Mon Sep 17 00:00:00 2001 From: Kun Chen <3233006+kunchenguid@users.noreply.github.com> Date: Fri, 26 Jun 2026 14:45:13 -0700 Subject: [PATCH 003/167] fix: harden no-mistakes validation contract (#97) * fix: harden no-mistakes validation contract against gate-park and self-fix duplication Rewrite the no-mistakes validation block scaffolded into every ship brief so a crewmate knows the right move at each gate: the pipeline owns every fix (including the fix for a real bug review finds in the crewmate's own code); respond, never self-implement/abort/re-run; the ask-user loop end to end (feed the decision back via axi respond, do not hand-fix); process every return (backgrounding ok, never idle-wait for auto-advance); avoid --yes (it auto-resolves ask-user with zero escalation); and review findings always gate (review auto-fix is disabled). Add the supervisor heuristic in AGENTS.md: firstmate keys off the no-mistakes run step status - running/fixing/ci means working, awaiting_approval/fix_review means parked (surfaced as awaiting_agent: parked on axi status) - not shell liveness, plus a self-fix-duplication red flag (hand-commits/abort/re-run mid-validation). Resolves the gate-park-deadlock and self-fix-duplication failure modes. * no-mistakes(document): Sync validation docs * no-mistakes(lint): Lint checks clean --- AGENTS.md | 12 ++++++++++-- CONTRIBUTING.md | 5 +++-- bin/fm-brief.sh | 23 ++++++++++++++++++----- 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index f41139ba96..11f3013d2c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -371,10 +371,18 @@ For `no-mistakes`-mode ship tasks, when a crewmate's status says `done`, trigger Load `harness-adapters` for the target harness's skill invocation form; natural language also works if uncertain. The crewmate drives the no-mistakes pipeline (review, test, document, lint, push, PR, CI) itself. -The no-mistakes pipeline fixes auto-fix findings on its own (inside its own worktree); the crewmate advances each gate with `no-mistakes axi respond`, and must never edit or commit code while a run is active. +The no-mistakes pipeline owns every validation fix on the crewmate's branch in its own worktree; the crewmate advances each gate with `no-mistakes axi respond`, and must never hand-edit, commit, reset, checkout, abort, or re-run while a run is active. When it reports `needs-decision` (ask-user findings), relay the findings to the captain unless `yolo=on` permits routine approval on your judgment, then send the decision back as a short instruction (the crewmate responds via `no-mistakes axi respond`). Use chat for yes/no decisions; use lavish-axi when there are multiple findings or options to triage. +Judge a validating crewmate by the run's step status, never by whether its shell is still running; read it cheaply with `no-mistakes axi status`. + +- `running`/`fixing`/`ci` - the pipeline is working (a fix round, a test, or CI monitoring); these run for many minutes and quiet is normal, so leave it alone. +- `awaiting_approval`/`fix_review` - the run is parked waiting on the agent, surfaced as a top-level `awaiting_agent: parked ` line right after `status:` in `axi status`. + The crewmate owes a `respond`; if it is idle-waiting for the run to advance on its own, steer it to drive the gate, because a parked gate never self-resolves. +- Red flag - self-fix duplication: a validating crewmate making fresh hand-commits, aborting the run, or re-running it mid-validation is re-doing work the pipeline already owns. + Steer it back to respond-only: the pipeline applies every fix on the branch from `axi respond`, including the fix for a real bug the review found in the crewmate's own code, and hand-fixing forces a full re-validation. + ### PR ready For PR-based ship tasks, the ready signal depends on mode: `no-mistakes` reports `done: PR checks green` after CI is green, while `direct-PR` reports `done: PR ` after opening the PR. @@ -491,7 +499,7 @@ Watcher liveness is not enough if you are foreground-blocked. Whenever one or more tasks are in flight, do not run long foreground-blocking operations in your own session. This is about firstmate's own session: it includes a no-mistakes pipeline firstmate runs for this repo, long builds, and any other multi-minute command. Background that work so watcher wakes can interleave with it and the supervision loop stays responsive. -A crewmate driving its own `no-mistakes` validation does the opposite: it runs that gate drive in the foreground and drives it synchronously, never backgrounding or idle-waiting on its own validation run. +A crewmate driving its own `no-mistakes` validation does the opposite: it drives that gate loop synchronously and processes every return, never idle-waiting for its own validation run to advance on its own. Token discipline: status files before panes; default peeks to 40 lines; never stream a pane repeatedly through yourself; batch what you tell the captain. The context-% shown in a peek is not actionable as crew health; ignore it and intervene only on real signals (`signal`, `stale`, `needs-decision`, `blocked`), looping or confusion in the pane, or a question the brief already answers. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0d0a72bc51..8dabf200be 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -49,8 +49,9 @@ See the [no-mistakes quick start](https://kunchenguid.github.io/no-mistakes/star Tracked changes to firstmate itself - `AGENTS.md`, `README.md`, `CONTRIBUTING.md`, `.tasks.toml`, `.github/workflows/`, `bin/`, and agent skill files - ship through the `no-mistakes` pipeline on a feature branch and require an explicit merge approval. When supervising live crewmates, keep firstmate's own long validation or build commands in the background so watcher wakes can still be handled. -A crewmate driving its own `no-mistakes` validation does the opposite: it runs the gate in the foreground and lets each synchronous `no-mistakes axi run` or `no-mistakes axi respond` call return. -The pipeline owns auto-fix changes; the crewmate authorizes them with `no-mistakes axi respond --action fix --findings ` instead of editing or committing while the run is active. +Unlike firstmate, a crewmate owns its own validation gate loop: it processes every `no-mistakes axi run` or `no-mistakes axi respond` return, responds to gates, and never waits for a parked gate to self-resolve. +The pipeline owns every validation fix, including auto-fix findings and fixes for real bugs found in the crewmate's own code; the crewmate authorizes or answers with `no-mistakes axi respond` instead of editing, committing, aborting, or re-running while the run is active. +Do not use `--yes` for crewmate validation because it silently resolves `ask-user` findings without escalation. Local `.no-mistakes/` state and test evidence stay out of this repo; `.no-mistakes.yaml` keeps evidence in a temp directory instead. Check and test the toolbelt before pushing: diff --git a/bin/fm-brief.sh b/bin/fm-brief.sh index 1193d7ef9d..b10b7f7b44 100755 --- a/bin/fm-brief.sh +++ b/bin/fm-brief.sh @@ -207,11 +207,24 @@ The task is complete only when committed on your branch. When you believe it is complete, append \`done: {summary}\` to the status file and stop. Firstmate will then instruct you to run /no-mistakes to validate and ship a PR. -During validation you drive the gates while the pipeline owns the fixes. Run it in the foreground and follow this contract: -- Never edit or \`git commit\` code yourself while a run is active; the pipeline applies every fix in its own worktree. -- When a gate shows auto-fix findings, advance it with \`no-mistakes axi respond --action fix --findings \` (the pipeline applies the fix and re-reviews). Escalate ask-user findings per rule 6. -- \`no-mistakes axi run\` and \`axi respond\` block synchronously for many minutes (test and CI especially); the pipeline often fixes findings itself with no gate, so when a call returns no \`gate:\` object that is normal - just let it return. -- Never cancel, abort, re-run, or background the run, and never idle-wait for a background notification: the call is in the foreground and returns on its own. +During validation the pipeline owns every fix; you only drive the gates. +Once a run is active, every fix - both auto-fix findings and the fix for a real bug the review finds in your own code - is applied by the pipeline on your branch, in its own worktree. +Never hand-edit, \`git commit\`, \`git reset\`/\`git checkout\`, abort, or re-run while a run is active: doing so duplicates the pipeline's work and forces a full re-validation. +You advance the work only by responding to gates: +- Process every return; never idle-wait. + \`no-mistakes axi run\` / \`axi respond\` return either at a gate (a \`gate:\` object) or at a terminal or CI-ready outcome (no \`gate:\`). + A run legitimately runs long - test, CI, and each fix round take many minutes - so a quiet call is working, not stalled. + Backgrounding the call is fine; idle-waiting for the run to advance on its own is not, because it never advances past a gate by itself. + Read every return: on a \`gate:\`, respond, and loop until you reach an outcome. +- Auto-fix findings: advance the gate with \`no-mistakes axi respond --action fix --findings \`; the pipeline applies the fix on your branch and re-reviews. + You never apply it yourself. +- Review findings always gate. + Review auto-fix is disabled, so every actionable review finding parks for your response instead of being self-fixed - drive it like any other gate. +- ask-user findings: escalate to firstmate (rule 6) and stop. + When the decision comes back, feed it to the gate with \`no-mistakes axi respond\` (the \`--action\`/answer the decision implies) and let the pipeline apply it. + Even when it is a real bug in your own code, do NOT implement the decided fix yourself and do NOT abort to go fix it - the pipeline applies it from your \`respond\`. +- Avoid \`--yes\`: it silently auto-resolves every finding, including \`ask-user\`, with zero escalation, so a decision the captain should make gets resolved without them. + Drive gates manually and escalate \`ask-user\` findings. After /no-mistakes reports CI green, append \`done: PR {url} checks green\` and stop. You are finished. EOF From 9391e901001e13428a61f7de34e04f65a2bdaae1 Mon Sep 17 00:00:00 2001 From: Kun Chen <3233006+kunchenguid@users.noreply.github.com> Date: Fri, 26 Jun 2026 16:07:15 -0700 Subject: [PATCH 004/167] fix(watch): assert watcher liveness during wake drain (#101) * fix(watch): tighten watcher re-arm discipline and assert liveness on drain The watcher supervision chain could silently lapse: re-arms were bundled at the tail of multi-command calls (where fm-watch-arm no-ops when a cycle is briefly still alive, so no fresh cycle got established), and text-only "holding" turns ran no supervision script, so fm-guard's liveness banner - which only fires when a guarded script runs - never triggered. AGENTS.md section 8: make the re-arm rules unambiguous - keep exactly one live cycle while work is in flight; re-arm after each FIRE (a completed arm task carrying a wake reason) and never churn on a healthy/started no-op; run fm-watch-arm standalone, never bundled; never end a turn blind, holds included. Existing material (singleton lock, beacon, guard banner, afk exception) is preserved. fm-wake-drain.sh: assert watcher liveness after draining by reusing fm-guard.sh's existing graced, beacon-based banner, so a lapse also surfaces on a plain drain-and-handle turn. Called after the queue is emptied so the guard never re-prints its own queued-wakes notice, and best-effort so it never changes the drain's exit status. The grace beacon keeps it silent right after a normal fire and warns only on a stale-beyond-grace lapse with work in flight. Watcher core (fm-watch.sh, lock, beacon-touch, wake-queue) untouched. tests: keep the worktree-tangle check inert across the drain-invoking suites (wake-helpers points FM_ROOT at a non-git dir, the same trick the direct fm-guard.sh tests use), and add a regression test asserting the drain warns on a lapse and stays silent right after a fire. * no-mistakes(document): Document drain-time watcher liveness --- AGENTS.md | 18 ++++++++++++++---- CONTRIBUTING.md | 2 +- bin/fm-guard.sh | 4 ++-- bin/fm-wake-drain.sh | 19 ++++++++++++++++++- bin/fm-watch-arm.sh | 14 ++++++++------ docs/architecture.md | 4 +++- docs/scripts.md | 2 +- tests/fm-wake-queue.test.sh | 31 ++++++++++++++++++++++++++++--- tests/wake-helpers.sh | 13 +++++++++++++ 9 files changed, 88 insertions(+), 19 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 11f3013d2c..cb88b3544f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -438,13 +438,22 @@ It costs zero tokens while running and exits with one reason line when something It also writes each detected wake to the durable queue at `state/.wake-queue` before advancing suppression markers such as `.seen-*`, `.stale-*`, `.last-check`, or `.last-heartbeat`. At the start of every wake-handling turn and every recovery turn, run `bin/fm-wake-drain.sh` before peeking panes, reading status files beyond the reason line, or starting new work. The printed one-shot reason line is still useful, but the drained queue is the lossless backlog. -After handling drained wakes, re-arm the watcher before you end the turn by running `bin/fm-watch-arm.sh` as a background task. -Arm or re-arm the watcher only through the harness's own tracked background mechanism - the one that survives the call and notifies you when the process exits - so the re-arm actually persists and the next wake reaches you. +**Keep exactly one live cycle.** +The arm chain IS the supervision: while any task is in flight, keep exactly one live `bin/fm-watch-arm.sh` background task at all times, because if no cycle is live firstmate is blind. +Each cycle is one harness-tracked background task that blocks until a wake is due, fires with one reason line, and ends, so the chain survives only when firstmate starts the next cycle after each fire. +After handling the drained wakes, re-arm before you end the turn by running `bin/fm-watch-arm.sh` as its own background task. +Arm or re-arm the watcher only through the harness's own tracked background mechanism - the one that survives the call and notifies you when the process exits - so the cycle actually persists and the next wake reaches you. Never fire-and-forget the watcher with a shell `&` inside another call: that backgrounded child is reaped when the call returns, so supervision silently stops, and worse, the dying process reports a false "already running" that hides the gap. +**Standalone, never bundled.** +Run `bin/fm-watch-arm.sh` as its OWN background task with nothing else in that bash, never tacked onto the tail of a multi-command call: bundled, its self-verifying status line is buried in unrelated output and it can silently no-op as a side effect of those other commands, so no fresh cycle gets established and supervision lapses unnoticed. `bin/fm-watch-arm.sh` is self-verifying: it confirms a genuinely live watcher with a fresh beacon and prints exactly one honest status line - `watcher: started ...`, `watcher: healthy ...`, or `watcher: FAILED - no live watcher with a fresh beacon` (which exits non-zero) - so treat that line, not a process count or an unverified "already running", as the source of truth for watcher state. +**Re-arm after each FIRE; do not churn on a no-op.** +Read that line to know whether a cycle is already live: `started` (this arm just launched the live cycle, now blocking for the next wake) and `healthy` (a live cycle already held the lock) both mean a cycle is live, so do NOT start another - re-running it while one is healthy only churns no-op tasks and never establishes a fresh cycle; `FAILED` means no live cycle, so arm one now after draining any queued wakes. +A cycle is down only when its background task completes carrying a WAKE REASON (`signal`/`stale`/`check`/`heartbeat`): that is the watcher firing, and that is the one moment to handle the wake and then start exactly one fresh cycle. The watcher is singleton-safe: acquisition is race-proof, so under any number of concurrent arms at most one watcher ever holds this home's lock, and a duplicate that somehow starts self-evicts within one poll once it sees the lock no longer names it. If one is already alive with a fresh liveness beacon, another invocation exits cleanly instead of creating a duplicate watcher; if the live holder's beacon is stale, the new invocation exits with an actionable failure. -Re-arming is the primary model: just run `bin/fm-watch-arm.sh` and let the singleton lock no-op when a healthy watcher is already alive. +**No turn ends blind, holds included.** +Never end a turn while any task is in flight without a live cycle running: a text-only "holding" or "waiting" reply with crewmates live and no live cycle is a bug, and because such a turn runs no supervision script it is exactly the blind gap the script-only guard (`fm-guard.sh`, below) cannot catch, so this discipline must. If a forced restart is ever genuinely needed, use `bin/fm-watch-arm.sh --restart`, which stops only this home's watcher (the pid recorded in this home's `state/.watch.lock`) and starts a fresh one. Never `pkill -f bin/fm-watch.sh`: that pattern matches every firstmate home's watcher, including secondmate homes that run the same script, so a broad pkill from one home kills sibling homes' watchers. Away-mode supervision is provided by the `/afk` skill and its daemon; while `state/.afk` exists, the daemon owns the watcher. @@ -456,7 +465,7 @@ Empty polls, elapsed waiting time, and "still no change" are tool bookkeeping, n bin/fm-watch-arm.sh # safe verified re-arm; run as harness-tracked background; no-ops if healthy bin/fm-watch-arm.sh --restart # home-scoped forced restart; never a broad pkill bin/fm-watch.sh # the watcher itself; exits with: signal|stale|check|heartbeat -bin/fm-wake-drain.sh # drain queued wake records at turn start +bin/fm-wake-drain.sh # drain queued wake records at turn start; asserts guard after draining ``` On wake, in order of cheapness: @@ -483,6 +492,7 @@ This exception is narrow: ordinary crewmates still trip stale detection when the Arming the watcher is the last action of every wake-handling turn - but the protocol no longer relies on remembering that. While running, `fm-watch.sh` touches `state/.last-watcher-beat` every poll cycle. The supervision scripts (`fm-peek`, `fm-send`, `fm-spawn`, `fm-teardown`, `fm-pr-check`, `fm-promote`, `fm-review-diff`, `fm-fleet-sync`, `fm-update`) call `bin/fm-guard.sh` first, which warns to stderr when any task is in flight (`state/*.meta` exists) but queued wakes are pending, or that beacon is missing or older than `FM_GUARD_GRACE` (default 300s). +`bin/fm-wake-drain.sh` runs the same guard after it drains, so the liveness check also fires on a drain-and-handle turn that runs no other supervision script, narrowing the window in which a lapsed chain can hide; the grace beacon keeps it silent right after a normal fire and it warns only on a genuine stale-beyond-grace lapse. The no-watcher case leads with a prominent, bordered ●-marked banner (in-flight count, beacon age, and the exact one-line re-arm command) so it reads as an alarm rather than a buried stderr line you can skim past. So the next time you touch the fleet with queued wakes or no watcher alive, the tool output itself tells you what to do - a pull-based guard that works on any harness, since it rides the script output you already read rather than a harness-specific hook. The grace window keeps normal handling (watcher briefly down between a wake and its re-arm) silent. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8dabf200be..776d810191 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -60,7 +60,7 @@ Check and test the toolbelt before pushing: bash -n bin/*.sh # syntax-check the toolbelt shellcheck bin/*.sh tests/*.sh # lint the toolbelt and behavior tests; CI enforces this for test_script in tests/*.test.sh; do "$test_script"; done # behavior tests, matching CI -tests/fm-wake-queue.test.sh # durable wake queue losslessness, catch-up, double-drain, and duplicate-collapse tests +tests/fm-wake-queue.test.sh # durable wake queue losslessness, catch-up, double-drain, duplicate-collapse, and drain liveness guard tests tests/fm-watcher-lock.test.sh # watcher singleton, lock-race, watch-arm liveness, and guard-warning tests tests/fm-daemon.test.sh # sub-supervisor classifier, /afk presence-gating, max-defer, composer, and fm-send submit tests tests/fm-send-settle.test.sh # fm-send post-submit settle pause, tuning, disable, and --key bypass tests diff --git a/bin/fm-guard.sh b/bin/fm-guard.sh index 7ebfe7c5f2..9dc8c35de6 100755 --- a/bin/fm-guard.sh +++ b/bin/fm-guard.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash -# Watcher liveness and worktree-tangle guard, called at the top of the -# supervision scripts. +# Watcher liveness and worktree-tangle guard, called by supervision scripts and +# by fm-wake-drain.sh after it empties queued wakes. # First, always warn if the firstmate primary checkout (FM_ROOT) is on a named # non-default branch, because that means firstmate-on-itself work landed in the # primary instead of an isolated worktree. diff --git a/bin/fm-wake-drain.sh b/bin/fm-wake-drain.sh index 8b4f38e7ad..a5ddbcf69c 100755 --- a/bin/fm-wake-drain.sh +++ b/bin/fm-wake-drain.sh @@ -1,5 +1,5 @@ #!/usr/bin/env bash -# Atomically drain durable watcher wake records. +# Atomically drain durable watcher wake records, then assert watcher liveness. set -u SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" @@ -9,6 +9,21 @@ SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" DRAIN_TMP= DRAIN_LOCK_HELD=false +# Defense in depth for the watcher re-arm chain: this script runs at the top of +# every wake-handling and recovery turn, so assert watcher liveness here too. A +# lapsed supervision chain then surfaces on a plain drain-and-handle turn, not +# only when a guarded supervision script (fm-peek/fm-send/...) happens to run. +# Reuse fm-guard.sh's existing graced, beacon-based banner (FM_GUARD_GRACE) - do +# not duplicate the beacon math. Because the watcher touches its beacon every +# poll cycle, a normal fire leaves a recent beacon well inside grace and stays +# silent; only a genuine stale-beyond-grace lapse with work in flight warns. Call +# after the queue is emptied so guard never re-prints its own queued-wakes notice +# for the records this run just drained, and never let a guard hiccup change the +# drain's exit status. +assert_watcher_liveness() { + "$SCRIPT_DIR/fm-guard.sh" || true +} + # shellcheck disable=SC2317,SC2329 # Invoked by trap handlers below. cleanup() { local status=$? @@ -30,6 +45,7 @@ DRAIN_LOCK_HELD=true if [ ! -s "$FM_WAKE_QUEUE" ]; then : > "$FM_WAKE_QUEUE" + assert_watcher_liveness exit 0 fi @@ -41,4 +57,5 @@ mv "$FM_WAKE_QUEUE" "$DRAIN_TMP" || exit 1 fm_wake_print_deduped "$DRAIN_TMP" || exit "$?" rm -f "$DRAIN_TMP" DRAIN_TMP= +assert_watcher_liveness exit 0 diff --git a/bin/fm-watch-arm.sh b/bin/fm-watch-arm.sh index 697edbccd1..3841bf20e7 100755 --- a/bin/fm-watch-arm.sh +++ b/bin/fm-watch-arm.sh @@ -4,11 +4,12 @@ # The watcher (bin/fm-watch.sh) is one-shot: it blocks until a wake is due, prints # one reason line, and exits. Reliability depends on re-arming through a mechanism # that SURVIVES the call and NOTIFIES on exit, so firstmate must run this script as -# the harness's own tracked background task (e.g. run_in_background). NEVER fire it -# and forget with a shell `&` inside another call: that backgrounded child is -# reaped when the call returns, leaving NO watcher running and - worse - a false -# "already running" off the dying process. That exact mistake silently took -# supervision down for ~30 minutes. +# the harness's own tracked background task (e.g. run_in_background). Run it as +# its own standalone background task, never bundled onto the tail of another +# command. NEVER fire it and forget with a shell `&` inside another call: that +# backgrounded child is reaped when the call returns, leaving NO watcher running +# and a false "already running" off the dying process. That exact mistake +# silently took supervision down for ~30 minutes. # # This script forks the watcher as a tracked child, then VERIFIES the outcome # before it settles in. It confirms a watcher process is genuinely alive AND the @@ -22,7 +23,8 @@ # stale-beacon or dead-pid holder either self-heals (the fresh child steals the # dead lock per the singleton self-eviction/steal path and is confirmed) or this # returns the FAILED line. On started/healthy it exits zero; on FAILED it exits -# non-zero so the failure is loud and a caller can react. +# non-zero so the failure is loud and a caller can react. A healthy line means a +# live cycle already exists; do not churn extra no-op arms until that cycle fires. # # --restart: stop ONLY this FM_HOME's watcher (the pid recorded in THIS home's # state/.watch.lock) and start a fresh one. It resolves and signals exactly that diff --git a/docs/architecture.md b/docs/architecture.md index 82e7d849da..6efee0c7af 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -10,11 +10,13 @@ firstmate's full operating manual for the orchestrator agent itself is [`AGENTS. A zero-token bash watcher (`bin/fm-watch.sh`) sleeps on the fleet and wakes the first mate only when a crewmate reports, stalls, a PR merges, or an internal heartbeat review is due. Detected wakes are also written to a durable local queue (`state/.wake-queue`) before detector state advances, so a missed one-shot process exit can be recovered by draining the queue. +After each drain, `fm-wake-drain.sh` runs the same liveness guard as the supervision scripts, so a lapsed watcher chain surfaces even on a turn that only drains and handles queued wakes. Routine watcher polling, re-arm no-ops, elapsed waiting time, and unchanged heartbeat reviews stay silent; an idle crew costs you nothing. Routine re-arms go through `bin/fm-watch-arm.sh`, which forks the watcher as a tracked child, verifies it is genuinely alive with a fresh liveness beacon, and prints exactly one honest status line (`started` / `healthy` / `FAILED`, the last exiting non-zero) - never a false `already running` off a dying process. Its `--restart` mode signals only the watcher recorded in the current home's `state/.watch.lock`, so restarting one home cannot kill sibling secondmate watchers. A pull-based guard (`bin/fm-guard.sh`) warns through supervision tool output if the primary checkout is tangled, or if tasks are in flight and that watcher stops running or queued wakes are waiting to be drained. +The drain script calls that guard after emptying the queue, which avoids repeating the queued-wakes warning for records it just consumed while still warning on stale watcher liveness. It leads with prominent bordered banners for the tangle and no-watcher cases so they cannot be skimmed past. A presence-gated sub-supervisor (`bin/fm-supervise-daemon.sh`) extends this for walk-away supervision: the `/afk` skill activates it, after which it self-handles routine wakes in bash and escalates only captain-relevant events as one batched, single-line digest (prefixed with an in-band sentinel marker so firstmate can tell daemon injections apart from real messages). @@ -96,5 +98,5 @@ Kill the first mate session anytime; the next one reconciles and carries on. ## Development notes -The current watcher reliability work keeps the one-shot process model and adds a durable queue, race-proof singleton lock, duplicate self-eviction, and a self-verifying tracked-child arm wrapper. +The current watcher reliability work keeps the one-shot process model and adds a durable queue, race-proof singleton lock, duplicate self-eviction, drain-time liveness assertion, and a self-verifying tracked-child arm wrapper. The presence-gated sub-supervisor (`bin/fm-supervise-daemon.sh`) provides proactive wake routing for walk-away supervision via the `/afk` skill; a blocking-waiter split remains a deferred follow-up phase. diff --git a/docs/scripts.md b/docs/scripts.md index 6aa0007c7e..27c6693968 100644 --- a/docs/scripts.md +++ b/docs/scripts.md @@ -24,7 +24,7 @@ Each file also starts with a short header comment. | `fm-tangle-lib.sh` | Shared default-branch resolution and primary-checkout tangle classification sourced by bootstrap and guard | | `fm-ff-lib.sh` | Shared guarded fast-forward helper for `/updatefirstmate` origin pulls and no-fetch local secondmate syncs | | `fm-tasks-axi-lib.sh` | Shared `tasks-axi` compatibility probe sourced by bootstrap and teardown | -| `fm-wake-drain.sh` | Atomically drain queued watcher wakes before handling supervision work | +| `fm-wake-drain.sh` | Atomically drain queued watcher wakes before handling supervision work, then run the watcher-liveness guard | | `fm-wake-lib.sh` | Shared durable wake queue and portable lock helpers sourced by the watcher, drain, arm, guard, and daemon | | `fm-send.sh` | Send one verified literal line (or `--key Escape`) to a direct-report window; exits non-zero on confirmed swallowed Enter; bare `kind=secondmate` targets are marked as from-firstmate; text sends pause `FM_SEND_SETTLE` seconds after success | | `fm-tmux-lib.sh` | Shared tmux pane primitives for busy detection, dim-ghost-aware and border-aware composer detection, and verified submit retry | diff --git a/tests/fm-wake-queue.test.sh b/tests/fm-wake-queue.test.sh index 34c5ee842f..894440681d 100755 --- a/tests/fm-wake-queue.test.sh +++ b/tests/fm-wake-queue.test.sh @@ -1,9 +1,11 @@ #!/usr/bin/env bash # tests/fm-wake-queue.test.sh - wake-queue losslessness (the queue safety matrix): # concurrent append/drain, signal catch-up while no watcher runs, stale/check -# enqueue-before-suppressor ordering, atomic double-drain, and duplicate collapse. -# Nothing is lost and nothing is double-consumed. Watcher/lock liveness lives in -# fm-watcher-lock.test.sh; daemon classification/injection in fm-daemon.test.sh. +# enqueue-before-suppressor ordering, atomic double-drain, duplicate collapse, +# and the drain-time watcher-liveness assertion. +# Nothing is lost and nothing is double-consumed. General watcher/lock liveness +# lives in fm-watcher-lock.test.sh; daemon classification/injection in +# fm-daemon.test.sh. set -u # shellcheck source=tests/wake-helpers.sh @@ -155,9 +157,32 @@ test_drain_dedupes_obvious_duplicates() { pass "drain collapses obvious duplicate heartbeat and signal records" } +# The drain runs at the top of every wake-handling turn, so it also asserts +# watcher liveness via fm-guard.sh: a lapsed re-arm chain then surfaces even on a +# plain drain-and-handle turn that runs no other supervision script. It must warn +# when work is in flight with no live watcher, and stay silent right after a +# normal fire (a fresh beacon within grace), so it never false-alarms every wake. +test_drain_asserts_watcher_liveness() { + local dir state err + dir=$(make_case drain-liveness) + state="$dir/state" + err="$dir/drain.err" + printf 'window=test:fm-x\nkind=ship\n' > "$state/x.meta" + FM_STATE_OVERRIDE="$state" "$DRAIN" >/dev/null 2> "$err" || fail "drain failed while asserting liveness" + grep -F 'WATCHER DOWN' "$err" >/dev/null || fail "drain did not surface the watcher-down banner with work in flight and no live watcher" + : > "$err" + touch "$state/.last-watcher-beat" + FM_STATE_OVERRIDE="$state" FM_GUARD_GRACE=300 "$DRAIN" >/dev/null 2> "$err" || fail "drain failed with a fresh beacon" + if grep -F 'WATCHER DOWN' "$err" >/dev/null; then + fail "drain false-alarmed right after a normal fire (fresh beacon within grace)" + fi + pass "drain asserts watcher liveness: warns on a lapse, stays silent right after a fire" +} + test_concurrent_append_and_drain test_signal_catchup_without_running_watcher test_stale_enqueue_before_suppressor test_check_output_is_queued test_atomic_double_drain test_drain_dedupes_obvious_duplicates +test_drain_asserts_watcher_liveness diff --git a/tests/wake-helpers.sh b/tests/wake-helpers.sh index 595671f469..17a4688942 100644 --- a/tests/wake-helpers.sh +++ b/tests/wake-helpers.sh @@ -7,6 +7,19 @@ # shellcheck source=tests/lib.sh . "$(dirname "${BASH_SOURCE[0]}")/lib.sh" +# fm-wake-drain.sh now calls fm-guard.sh to assert watcher liveness on every +# drain. fm-guard.sh's first check warns when the firstmate PRIMARY checkout +# (FM_ROOT) sits on a feature branch; with no override FM_ROOT resolves to the +# test runner's own checkout, which during validation is on a feature branch, so +# each drain would emit a spurious worktree-tangle banner. Point the tangle check +# at a fresh non-git dir to keep it inert across these suites - the same trick the +# direct fm-guard.sh tests use. A per-call FM_ROOT_OVERRIDE still wins where a +# suite sets its own (e.g. the watcher-lock guard-banner cases). +if [ -z "${FM_ROOT_OVERRIDE:-}" ]; then + FM_ROOT_OVERRIDE="$(fm_test_tmproot fm-wake-tangle-root)" + export FM_ROOT_OVERRIDE +fi + # append_wake : append a wake record to the durable # queue in a subshell scoped to , using the production wake library. append_wake() { From 362fb54e02156f02d65f9f19d48269dbeb85ceb6 Mon Sep 17 00:00:00 2001 From: Kun Chen <3233006+kunchenguid@users.noreply.github.com> Date: Fri, 26 Jun 2026 19:03:38 -0700 Subject: [PATCH 005/167] fix(brief): slim no-mistakes contract behind version floor (#102) * fix(brief): slim no-mistakes contract to pointer + firstmate wrapper no-mistakes v1.31.2 self-documents the validation mechanics in its own SKILL.md (refreshed from the binary on init) and live axi help output, including the review-always-gates, no-abort/rerun-mid-run, and never-idle-wait facts. fm-brief.sh's no-mistakes-mode Definition of done was restating all of that, a drift hazard since the brief lives in a different repo from the version-matched SKILL.md. Collapse the duplicated mechanics block to a ~3-line pointer at no-mistakes' own guidance and keep only the firstmate-specific wrapper: the done handshake, ask-user escalation to firstmate via rule 6, the --yes captain stance, and the CI-green reporting line. * no-mistakes(review): Enforce no-mistakes bootstrap version floor * no-mistakes(document): Sync no-mistakes docs * no-mistakes(lint): Fix ShellCheck version parsing --- AGENTS.md | 12 +++++---- CONTRIBUTING.md | 9 +++---- bin/fm-bootstrap.sh | 27 +++++++++++++++++++ bin/fm-brief.sh | 26 ++++++------------ docs/configuration.md | 2 +- docs/scripts.md | 2 +- tests/fm-bootstrap.test.sh | 45 +++++++++++++++++++++++++++++--- tests/fm-secondmate-sync.test.sh | 11 +++++++- 8 files changed, 100 insertions(+), 34 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index cb88b3544f..0f7d255568 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -113,6 +113,7 @@ Otherwise it prints one line per problem or capability fact; handle each: - `MISSING: (install: )` - list the missing tools to the captain with a one-line purpose each plus the printed install commands, wait for consent (one approval may cover the list), then run `bin/fm-bootstrap.sh install `. For `treehouse`, this also covers an installed version whose `treehouse get` lacks `--lease`; treat it as an upgrade request. + For `no-mistakes`, this also covers an installed version older than 1.31.2, because crewmate validation briefs delegate gate mechanics to no-mistakes' version-matched guidance. - `NEEDS_GH_AUTH` - ask the captain to run `! gh auth login` (interactive; you cannot run it for them). - `TANGLE: ` - the firstmate primary checkout (the repo root, `FM_ROOT`) is stranded on a feature branch instead of its default branch: a crewmate working firstmate-on-itself branched/committed in the primary instead of its own isolated worktree (section 8). The work is safe on that branch ref; restore the primary to its default branch with the printed `git -C checkout `, then re-validate that branch in a proper worktree. This is the only sanctioned firstmate-initiated git write to the primary, and it is a non-destructive branch switch that strands nothing. - `CREW_HARNESS_OVERRIDE: ` - record and use the override silently; surface a harness fact only if it actually blocks work or the captain asks. @@ -371,17 +372,17 @@ For `no-mistakes`-mode ship tasks, when a crewmate's status says `done`, trigger Load `harness-adapters` for the target harness's skill invocation form; natural language also works if uncertain. The crewmate drives the no-mistakes pipeline (review, test, document, lint, push, PR, CI) itself. -The no-mistakes pipeline owns every validation fix on the crewmate's branch in its own worktree; the crewmate advances each gate with `no-mistakes axi respond`, and must never hand-edit, commit, reset, checkout, abort, or re-run while a run is active. -When it reports `needs-decision` (ask-user findings), relay the findings to the captain unless `yolo=on` permits routine approval on your judgment, then send the decision back as a short instruction (the crewmate responds via `no-mistakes axi respond`). +The ship brief intentionally does not restate no-mistakes gate mechanics; it points the crewmate to the version-matched SKILL.md loaded by `/no-mistakes`, `no-mistakes axi run --help`, and per-response `help` lines. +Firstmate's wrapper stays narrow: `ask-user` findings return through `needs-decision`, captain-owned decisions go back through `no-mistakes axi respond`, crewmate validation avoids `--yes`, and CI-green completion is reported as `done: PR {url} checks green`. Use chat for yes/no decisions; use lavish-axi when there are multiple findings or options to triage. Judge a validating crewmate by the run's step status, never by whether its shell is still running; read it cheaply with `no-mistakes axi status`. - `running`/`fixing`/`ci` - the pipeline is working (a fix round, a test, or CI monitoring); these run for many minutes and quiet is normal, so leave it alone. - `awaiting_approval`/`fix_review` - the run is parked waiting on the agent, surfaced as a top-level `awaiting_agent: parked ` line right after `status:` in `axi status`. - The crewmate owes a `respond`; if it is idle-waiting for the run to advance on its own, steer it to drive the gate, because a parked gate never self-resolves. + The crewmate owes a response; if it is idle-waiting for the run to advance on its own, steer it to follow no-mistakes' active-gate help. - Red flag - self-fix duplication: a validating crewmate making fresh hand-commits, aborting the run, or re-running it mid-validation is re-doing work the pipeline already owns. - Steer it back to respond-only: the pipeline applies every fix on the branch from `axi respond`, including the fix for a real bug the review found in the crewmate's own code, and hand-fixing forces a full re-validation. + Steer it back to no-mistakes' respond flow; the pipeline, not the crewmate, applies validation fixes. ### PR ready @@ -603,7 +604,8 @@ Map firstmate's real backlog operations to the approved commands: Scaffold with `bin/fm-brief.sh ` - it writes `data//brief.md` with the standard contract (branch setup, status-reporting protocol, push/merge rules, definition of done) and all paths filled in. The ship-brief Setup opens with a worktree-isolation assertion ahead of the branch step: the crewmate confirms it is in its own treehouse worktree, not the primary checkout, and stops with `blocked: launched in primary checkout, not an isolated worktree` if not - the upstream half of the worktree-tangle guard (section 8). -For a ship task the definition of done is shaped by the project's delivery mode (section 6): `no-mistakes` ends in the harness-appropriate no-mistakes validation pipeline, `direct-PR` has the crewmate push and open the PR itself, `local-only` has it stop at "ready in branch" for firstmate to review and merge locally. +For a ship task the definition of done is shaped by the project's delivery mode (section 6): `no-mistakes` stops after the implementation commit, then firstmate triggers the harness-appropriate no-mistakes validation pipeline; `direct-PR` has the crewmate push and open the PR itself, and `local-only` has it stop at "ready in branch" for firstmate to review and merge locally. +The no-mistakes brief points to no-mistakes' version-matched guidance and keeps only firstmate-specific wrapper rules for `ask-user` escalation, `--yes` avoidance, and the CI-green done line. The scaffold reads the mode via `fm-project-mode.sh`, so you do not pass it. Ship briefs also include the project-memory contract: run `bin/fm-ensure-agents-md.sh` when the project already has agent-memory files or when the task produced durable project-intrinsic knowledge, then record proportionate learnings in `AGENTS.md`. For scout tasks add `--scout`: the scaffold swaps the definition of done for the report contract (findings to `data//report.md`, no branch, no push, no PR) and declares the worktree scratch; scout is mode-agnostic. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 776d810191..d33d335146 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -16,7 +16,7 @@ Dependency bots are exempt so their automation keeps working, but regular contri 1. Fork the repo, then clone the parent repo or set your local `origin` back to the parent (`git@github.com:kunchenguid/firstmate.git`). 2. Create a branch and make your changes. -3. Initialize the gate with your fork as the push target: `no-mistakes init --fork-url git@github.com:/firstmate.git` (fork routing requires **no-mistakes v1.30.1+**; without a fork, plain `no-mistakes init` still works for maintainers with push access). +3. Initialize the gate with your fork as the push target: `no-mistakes init --fork-url git@github.com:/firstmate.git` (firstmate expects **no-mistakes v1.31.2+**; without a fork, plain `no-mistakes init` still works for maintainers with push access). 4. Commit your changes. 5. Push through the gate instead of pushing to `origin`: @@ -25,7 +25,7 @@ Dependency bots are exempt so their automation keeps working, but regular contri ``` 6. Run `no-mistakes` to attach to the pipeline, watch findings, authorize auto-fixes, and review ask-user findings as needed. - While a run is active, let the pipeline apply authorized fixes instead of editing or committing them by hand. + Follow the installed no-mistakes version's SKILL.md and live `axi` help for gate mechanics. 7. Once the pipeline passes, it pushes the branch to your fork and opens the PR against the parent repo for you. See the [no-mistakes quick start](https://kunchenguid.github.io/no-mistakes/start-here/quick-start/) for the full first-run walkthrough. @@ -49,9 +49,8 @@ See the [no-mistakes quick start](https://kunchenguid.github.io/no-mistakes/star Tracked changes to firstmate itself - `AGENTS.md`, `README.md`, `CONTRIBUTING.md`, `.tasks.toml`, `.github/workflows/`, `bin/`, and agent skill files - ship through the `no-mistakes` pipeline on a feature branch and require an explicit merge approval. When supervising live crewmates, keep firstmate's own long validation or build commands in the background so watcher wakes can still be handled. -Unlike firstmate, a crewmate owns its own validation gate loop: it processes every `no-mistakes axi run` or `no-mistakes axi respond` return, responds to gates, and never waits for a parked gate to self-resolve. -The pipeline owns every validation fix, including auto-fix findings and fixes for real bugs found in the crewmate's own code; the crewmate authorizes or answers with `no-mistakes axi respond` instead of editing, committing, aborting, or re-running while the run is active. -Do not use `--yes` for crewmate validation because it silently resolves `ask-user` findings without escalation. +Crewmate validation follows the installed no-mistakes version's SKILL.md and live `axi` help instead of duplicating gate mechanics in firstmate docs. +Firstmate's wrapper still matters: `ask-user` findings route to the captain through firstmate, and crewmates avoid `--yes` because it silently resolves captain-owned decisions without escalation. Local `.no-mistakes/` state and test evidence stay out of this repo; `.no-mistakes.yaml` keeps evidence in a temp directory instead. Check and test the toolbelt before pushing: diff --git a/bin/fm-bootstrap.sh b/bin/fm-bootstrap.sh index 36b8696f44..210288ca75 100755 --- a/bin/fm-bootstrap.sh +++ b/bin/fm-bootstrap.sh @@ -20,6 +20,8 @@ # landed in the primary instead of its own worktree; restore it per the line. # treehouse is also MISSING when its installed version lacks # "treehouse get --lease" support. +# no-mistakes is also MISSING when its installed version is older than +# 1.31.2. # tasks-axi is an OPTIONAL backlog-management capability reported only # when tasks-axi --version is 0.1.1 or newer. It is never a MISSING # line and never prompts an install. @@ -128,11 +130,33 @@ install_cmd() { } TOOLS="tmux node gh treehouse no-mistakes gh-axi chrome-devtools-axi lavish-axi" +NO_MISTAKES_MIN_MAJOR=1 +NO_MISTAKES_MIN_MINOR=31 +NO_MISTAKES_MIN_PATCH=2 treehouse_supports_lease() { treehouse get --help 2>&1 | grep -Eq '(^|[^[:alnum:]_-])--lease([^[:alnum:]_-]|$)' } +no_mistakes_version_parts() { + local output + command -v no-mistakes >/dev/null 2>&1 || return 1 + output=$(no-mistakes --version 2>/dev/null) || return 1 + printf '%s\n' "$output" | sed -nE 's/.*[vV]?([0-9]+)\.([0-9]+)\.([0-9]+).*/\1 \2 \3/p' | head -n 1 +} + +no_mistakes_compatible() { + local parts major minor patch extra + parts=$(no_mistakes_version_parts) || return 1 + IFS=' ' read -r major minor patch extra <<< "$parts" + [ -n "$major" ] && [ -n "$minor" ] && [ -n "$patch" ] && [ -z "$extra" ] || return 1 + [ "$major" -gt "$NO_MISTAKES_MIN_MAJOR" ] && return 0 + [ "$major" -eq "$NO_MISTAKES_MIN_MAJOR" ] || return 1 + [ "$minor" -gt "$NO_MISTAKES_MIN_MINOR" ] && return 0 + [ "$minor" -eq "$NO_MISTAKES_MIN_MINOR" ] || return 1 + [ "$patch" -ge "$NO_MISTAKES_MIN_PATCH" ] +} + if [ "${1:-}" = "install" ]; then shift [ $# -gt 0 ] || { echo "usage: fm-bootstrap.sh install ..." >&2; exit 1; } @@ -151,6 +175,9 @@ done if command -v treehouse >/dev/null 2>&1 && ! treehouse_supports_lease; then echo "MISSING: treehouse (install: $(install_cmd treehouse))" fi +if command -v no-mistakes >/dev/null 2>&1 && ! no_mistakes_compatible; then + echo "MISSING: no-mistakes (install: $(install_cmd no-mistakes))" +fi gh auth status >/dev/null 2>&1 || echo "NEEDS_GH_AUTH" # Worktree-tangle check: the firstmate primary checkout (FM_ROOT) must sit on its # default branch, not a feature branch (see fm-tangle-lib.sh). Scoped to the diff --git a/bin/fm-brief.sh b/bin/fm-brief.sh index b10b7f7b44..f5668cee80 100755 --- a/bin/fm-brief.sh +++ b/bin/fm-brief.sh @@ -207,24 +207,14 @@ The task is complete only when committed on your branch. When you believe it is complete, append \`done: {summary}\` to the status file and stop. Firstmate will then instruct you to run /no-mistakes to validate and ship a PR. -During validation the pipeline owns every fix; you only drive the gates. -Once a run is active, every fix - both auto-fix findings and the fix for a real bug the review finds in your own code - is applied by the pipeline on your branch, in its own worktree. -Never hand-edit, \`git commit\`, \`git reset\`/\`git checkout\`, abort, or re-run while a run is active: doing so duplicates the pipeline's work and forces a full re-validation. -You advance the work only by responding to gates: -- Process every return; never idle-wait. - \`no-mistakes axi run\` / \`axi respond\` return either at a gate (a \`gate:\` object) or at a terminal or CI-ready outcome (no \`gate:\`). - A run legitimately runs long - test, CI, and each fix round take many minutes - so a quiet call is working, not stalled. - Backgrounding the call is fine; idle-waiting for the run to advance on its own is not, because it never advances past a gate by itself. - Read every return: on a \`gate:\`, respond, and loop until you reach an outcome. -- Auto-fix findings: advance the gate with \`no-mistakes axi respond --action fix --findings \`; the pipeline applies the fix on your branch and re-reviews. - You never apply it yourself. -- Review findings always gate. - Review auto-fix is disabled, so every actionable review finding parks for your response instead of being self-fixed - drive it like any other gate. -- ask-user findings: escalate to firstmate (rule 6) and stop. - When the decision comes back, feed it to the gate with \`no-mistakes axi respond\` (the \`--action\`/answer the decision implies) and let the pipeline apply it. - Even when it is a real bug in your own code, do NOT implement the decided fix yourself and do NOT abort to go fix it - the pipeline applies it from your \`respond\`. -- Avoid \`--yes\`: it silently auto-resolves every finding, including \`ask-user\`, with zero escalation, so a decision the captain should make gets resolved without them. - Drive gates manually and escalate \`ask-user\` findings. +You drive no-mistakes by responding to its gates, not by implementing fixes. +Follow no-mistakes' own guidance for the mechanics: it loads when you invoke /no-mistakes, and \`no-mistakes axi run --help\` plus the \`help\` lines in each \`axi\` response are authoritative and version-matched to the installed binary. +Do not hand-edit, commit, or fix findings yourself while a run is active - the pipeline applies every fix. + +Two firstmate-specific rules layer on top of that guidance: +- ask-user findings are not yours to answer: escalate to firstmate (rule 6) and stop. + When the decision comes back, feed it to the gate with \`no-mistakes axi respond\` and let the pipeline apply it - do not route the question to "the user" or implement the fix yourself. +- Avoid \`--yes\`: the captain, not you, owns the ask-user decisions it would silently auto-resolve. After /no-mistakes reports CI green, append \`done: PR {url} checks green\` and stop. You are finished. EOF diff --git a/docs/configuration.md b/docs/configuration.md index 51124079de..7110e6c412 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -45,7 +45,7 @@ Launch mechanics, including the verified command templates, live in [`bin/fm-spa ## Toolchain -On first launch the first mate detects what its required toolchain is missing or too old (tmux, node, gh, treehouse with durable lease support, no-mistakes, gh-axi, chrome-devtools-axi, lavish-axi), lists it with the exact install commands, and installs only after you say go. +On first launch the first mate detects what its required toolchain is missing or too old (tmux, node, gh, treehouse with durable lease support, no-mistakes v1.31.2 or newer, gh-axi, chrome-devtools-axi, lavish-axi), lists it with the exact install commands, and installs only after you say go. If compatible `tasks-axi` is already on `PATH`, bootstrap records it as an optional capability fact and firstmate uses its verbs for routine backlog mutations; when it is absent or incompatible, firstmate keeps hand-editing `data/backlog.md` exactly as before. Bootstrap also reports a `TANGLE:` line when `FM_ROOT` is on a named non-default branch; follow the printed checkout remediation rather than treating it as an installable tool problem. Bootstrap also runs the guarded local secondmate sync for recorded live secondmate homes. diff --git a/docs/scripts.md b/docs/scripts.md index 27c6693968..c1785f9125 100644 --- a/docs/scripts.md +++ b/docs/scripts.md @@ -5,7 +5,7 @@ Each file also starts with a short header comment. | Script | Description | | ------------------------ | ------------------------------------------------------------------------------------------------------------------- | -| `fm-bootstrap.sh` | Detect required toolchain problems, optional capability facts, and primary-checkout `TANGLE:` problems; locally sync live secondmate homes; refresh clones best-effort; install tools only after consent | +| `fm-bootstrap.sh` | Detect required toolchain and version problems, optional capability facts, and primary-checkout `TANGLE:` problems; locally sync live secondmate homes; refresh clones best-effort; install tools only after consent | | `fm-fleet-sync.sh` | Fetch clones, clean-fast-forward their checked-out default branches, and safely prune branches whose remote is gone | | `fm-update.sh` | Self-update the running firstmate repo and registered secondmate homes with fast-forward-only pulls from origin | | `fm-backlog-handoff.sh` | Move already-judged in-scope queued backlog items from the main home into a seeded secondmate home | diff --git a/tests/fm-bootstrap.test.sh b/tests/fm-bootstrap.test.sh index 2029433a30..ade86092bf 100755 --- a/tests/fm-bootstrap.test.sh +++ b/tests/fm-bootstrap.test.sh @@ -4,8 +4,9 @@ # Bootstrap prints one line per problem or capability fact and is silent when all # is well. firstmate consumes the exact 'MISSING: treehouse (install: ...)' and # 'TASKS_AXI: available' lines, so those contracts are pinned verbatim. The cases -# are table-driven over the two inputs that vary: whether `treehouse get --help` -# advertises --lease, and which (if any) tasks-axi version is on PATH. +# are table-driven over the inputs that vary: whether `treehouse get --help` +# advertises --lease, which (if any) tasks-axi version is on PATH, and which +# no-mistakes version is on PATH. set -u # shellcheck source=tests/lib.sh @@ -19,7 +20,7 @@ TMP_ROOT=$(fm_test_tmproot fm-bootstrap-tests) make_fake_toolchain() { local dir=$1 fakebin fakebin=$(fm_fakebin "$dir") - fm_fake_exit0 "$fakebin" tmux node no-mistakes gh-axi chrome-devtools-axi lavish-axi + fm_fake_exit0 "$fakebin" tmux node gh-axi chrome-devtools-axi lavish-axi cat > "$fakebin/gh" <<'SH' #!/usr/bin/env bash if [ "${1:-}" = auth ] && [ "${2:-}" = status ]; then @@ -41,6 +42,15 @@ fi exit 0 SH chmod +x "$fakebin/treehouse" + cat > "$fakebin/no-mistakes" <<'SH' +#!/usr/bin/env bash +if [ "${1:-}" = --version ]; then + printf '%s\n' "${FM_FAKE_NO_MISTAKES_VERSION:-no-mistakes version v1.31.2 (fake) 2026-06-27T00:02:18Z}" + exit 0 +fi +exit 0 +SH + chmod +x "$fakebin/no-mistakes" printf '%s\n' "$fakebin" } @@ -97,4 +107,33 @@ ROWS pass "bootstrap reports treehouse lease + tasks-axi compatibility contracts" } +test_no_mistakes_min_version() { + local label version mode case_dir fakebin out missing n + missing='MISSING: no-mistakes (install: curl -fsSL https://raw.githubusercontent.com/kunchenguid/no-mistakes/main/docs/install.sh | sh)' + n=0 + while IFS='^' read -r label version mode; do + [ -n "$label" ] || continue + n=$((n + 1)) + case_dir="$TMP_ROOT/no-mistakes-$n" + mkdir -p "$case_dir/home" + fakebin=$(make_fake_toolchain "$case_dir") + out=$(PATH="$fakebin:$BASE_PATH" FM_HOME="$case_dir/home" FM_ROOT_OVERRIDE="$case_dir/home" \ + FM_FAKE_TREEHOUSE_LEASE_HELP=1 FM_FAKE_NO_MISTAKES_VERSION="$version" "$ROOT/bin/fm-bootstrap.sh") + case "$mode" in + empty) + [ -z "$out" ] || fail "$label: expected silence, got: $out" ;; + missing) + [ "$out" = "$missing" ] || fail "$label: expected '$missing', got: $out" ;; + esac + done <<'ROWS' +minimum no-mistakes version is accepted^no-mistakes version v1.31.2 (fake)^empty +newer no-mistakes minor is accepted^no-mistakes version v1.32.0 (fake)^empty +newer no-mistakes major is accepted^no-mistakes version v2.0.0 (fake)^empty +older no-mistakes patch reports an upgrade^no-mistakes version v1.31.1 (fake)^missing +unparseable no-mistakes version reports an upgrade^no-mistakes development build^missing +ROWS + pass "bootstrap enforces no-mistakes minimum version" +} + test_bootstrap_reporting +test_no_mistakes_min_version diff --git a/tests/fm-secondmate-sync.test.sh b/tests/fm-secondmate-sync.test.sh index e2ada37f09..a6ddc212d1 100755 --- a/tests/fm-secondmate-sync.test.sh +++ b/tests/fm-secondmate-sync.test.sh @@ -264,7 +264,7 @@ make_fake_toolchain() { local dir=$1 fakebin fakebin="$dir/fakebin" mkdir -p "$fakebin" - fm_fake_exit0 "$fakebin" tmux node no-mistakes gh-axi chrome-devtools-axi lavish-axi + fm_fake_exit0 "$fakebin" tmux node gh-axi chrome-devtools-axi lavish-axi cat > "$fakebin/gh" <<'SH' #!/usr/bin/env bash exit 0 @@ -278,6 +278,15 @@ fi exit 0 SH chmod +x "$fakebin/treehouse" + cat > "$fakebin/no-mistakes" <<'SH' +#!/usr/bin/env bash +if [ "${1:-}" = --version ]; then + printf '%s\n' 'no-mistakes version v1.31.2 (fake)' + exit 0 +fi +exit 0 +SH + chmod +x "$fakebin/no-mistakes" printf '%s\n' "$fakebin" } From ba62f03009c1a08f347bedee37ed42c80fa0609a Mon Sep 17 00:00:00 2001 From: Kun Chen <3233006+kunchenguid@users.noreply.github.com> Date: Fri, 26 Jun 2026 21:51:32 -0700 Subject: [PATCH 006/167] fix(send): settle codex skill popups before submit (#103) * fix(send): settle codex $skill popup before submit A `$` invocation (e.g. $no-mistakes) opens codex's $-autocomplete popup; submitting too fast lets it swallow the Enter so the invocation never lands - biting every pipeline trigger to a codex crew/secondmate. Mirror the existing `/` slash-popup handling: give a `$...` message the 1.2s popup-settle before the (retried) Enter, but scope it to harness=codex (read from the target's meta) so `$`-prefixed plain text ("$5/month", "$HOME") to claude/opencode/pi keeps the 0.3s fast path. An explicit session:window target has no meta -> harness unknown -> treated as non-codex. The retried Enter in fm_tmux_submit_core still backs the settle up; the `/` case, --key path, marker, and meta-resolution contract are unchanged. Record the codex $-popup fact in the harness-adapters skill, and add a per-harness settle-selection test (codex $ ->1.2, claude $ ->0.3, explicit $ ->0.3, any / ->1.2, plain ->0.3). * no-mistakes(document): Sync fm-send popup docs --- .agents/skills/harness-adapters/SKILL.md | 6 ++ CONTRIBUTING.md | 1 + bin/fm-send.sh | 32 +++++- docs/architecture.md | 2 +- docs/scripts.md | 2 +- tests/fm-send-popup-settle.test.sh | 121 +++++++++++++++++++++++ 6 files changed, 160 insertions(+), 4 deletions(-) create mode 100755 tests/fm-send-popup-settle.test.sh diff --git a/.agents/skills/harness-adapters/SKILL.md b/.agents/skills/harness-adapters/SKILL.md index 554a37ac8b..8edddb7183 100644 --- a/.agents/skills/harness-adapters/SKILL.md +++ b/.agents/skills/harness-adapters/SKILL.md @@ -71,6 +71,12 @@ That styled capture is internal to the boolean detector only. | Interrupt | single Escape | | Skill invocation | `$` (e.g. `$no-mistakes`); `/` is claude-only and codex rejects it as "Unrecognized command" | +A `$` invocation opens a `$`-autocomplete (skill) popup, the same hazard as the `/` slash popup: submitting too fast lets the popup swallow the Enter, so the invocation never lands. +`fm-send` handles it the same way it handles `/` - it gives the popup a longer settle (1.2s) between typing and the first Enter, with `fm_tmux_submit_core`'s retried Enter as the safety net - but the `$` settle is scoped to `harness=codex`, read from the target's `state/.meta`. +That scope matters because, unlike `/`, a leading `$` commonly starts ordinary text (`$5/month`, `$HOME`), so a universal `$` rule would needlessly slow plain steers to claude/opencode/pi; only a codex target receiving a `$...` message gets the popup-settle. +An explicit `session:window` target has no meta, so its harness is unknown and treated as non-codex (the safe fast-path default). +This is why the validation trigger (`$no-mistakes`) to a codex crew now lands on the first Enter instead of biting the popup. + Directory trust dialog on first run per repo root: "Do you trust the contents of this directory?" Accept with Enter. The decision persists for the repo, so later worktrees of the same project skip it. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d33d335146..38038990b8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -63,6 +63,7 @@ tests/fm-wake-queue.test.sh # durable wake queue losslessness, cat tests/fm-watcher-lock.test.sh # watcher singleton, lock-race, watch-arm liveness, and guard-warning tests tests/fm-daemon.test.sh # sub-supervisor classifier, /afk presence-gating, max-defer, composer, and fm-send submit tests tests/fm-send-settle.test.sh # fm-send post-submit settle pause, tuning, disable, and --key bypass tests +tests/fm-send-popup-settle.test.sh # fm-send pre-Enter popup-settle selection for slash commands and codex $skill invocations tests/fm-send-secondmate-marker.test.sh # fm-send from-firstmate marker for kind=secondmate targets: marked vs crewmate/explicit/--key, and the exact marker byte sequence tests/fm-wake-daemon-lifecycle-e2e.test.sh # watcher + daemon lifecycle e2e: restart catch-up, batching, dedupe, stale-pane routing, and digest injection tests/fm-composer-ghost.test.sh # dim-ghost stripping, ghost-only composer detection, and escape-free peek tests diff --git a/bin/fm-send.sh b/bin/fm-send.sh index dd8e889cdd..489c07ca54 100755 --- a/bin/fm-send.sh +++ b/bin/fm-send.sh @@ -12,6 +12,8 @@ # instead of silently leaving an unsubmitted instruction (incident afk-invx-i5). # The composer/submit logic is shared with the away-mode daemon via # bin/fm-tmux-lib.sh. Tune with FM_SEND_RETRIES (default 3) / FM_SEND_SLEEP (0.4). +# Slash commands, and codex `$...` skill invocations resolved through harness +# meta, get a longer pre-Enter settle so completion popups do not swallow Enter. # # From-firstmate marker: when the resolved target is a bare `fm-` whose meta # records kind=secondmate, the text is prefixed with the from-firstmate marker @@ -76,12 +78,38 @@ case "$RAW_TARGET" in ;; esac +# Resolve the target's harness from its meta (recorded by fm-spawn), used only to +# scope the codex `$` popup-settle below. A bare fm- target carries +# meta; an explicit session:window escape-hatch target has none, so its harness is +# unknown and treated as non-codex (the safe default that keeps the fast path). +TARGET_HARNESS="" +case "$RAW_TARGET" in + fm-*) + meta="$STATE/${RAW_TARGET#fm-}.meta" + if [ -f "$meta" ]; then + TARGET_HARNESS=$(grep '^harness=' "$meta" 2>/dev/null | tail -1 | cut -d= -f2- || true) + fi + ;; +esac + if [ "${1:-}" = "--key" ]; then tmux send-keys -t "$T" "$2" else # Slash commands open a completion popup in some TUIs (verified on codex); - # submitting too fast selects nothing. Give popups time to settle. - case "$*" in /*) settle=1.2 ;; *) settle=0.3 ;; esac + # submitting too fast selects nothing, so give the popup time to settle before + # the (retried) Enter. Codex opens the same kind of popup for a `$` + # invocation, so a `$...` message to a codex target gets the same settle. That + # `$` case is scoped to codex on purpose: unlike `/`, a leading `$` commonly + # starts ordinary text ("$5/month", "$HOME"), so a universal `$` rule would + # needlessly slow plain text to claude/opencode/pi. The retried Enter in + # fm_tmux_submit_core still backs the settle up either way. + case "$*" in + /*) settle=1.2 ;; + \$*) + if [ "$TARGET_HARNESS" = codex ]; then settle=1.2; else settle=0.3; fi + ;; + *) settle=0.3 ;; + esac retries=${FM_SEND_RETRIES:-3} sleep_s=${FM_SEND_SLEEP:-0.4} # Type once, submit, verify. Lenient: only a positively-confirmed swallow diff --git a/docs/architecture.md b/docs/architecture.md index 6efee0c7af..0f16fa010b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -21,7 +21,7 @@ It leads with prominent bordered banners for the tangle and no-watcher cases so A presence-gated sub-supervisor (`bin/fm-supervise-daemon.sh`) extends this for walk-away supervision: the `/afk` skill activates it, after which it self-handles routine wakes in bash and escalates only captain-relevant events as one batched, single-line digest (prefixed with an in-band sentinel marker so firstmate can tell daemon injections apart from real messages). Its injection path shares `bin/fm-tmux-lib.sh` with `fm-send.sh`, so dim-ghost-aware and border-aware composer detection plus verified submit retry stay consistent; stalled escalation delivery raises `state/.subsuper-inject-wedged` after `FM_MAX_DEFER_SECS` instead of silently deferring forever. -`fm-send.sh` adds its own `FM_SEND_SETTLE` pause after successful text sends so immediate peeks catch the receiving turn starting; the sub-supervisor uses only the shared submit core and does not pay that pause. +`fm-send.sh` selects a pre-Enter popup-settle for slash commands and for codex `$...` skill invocations using the target's recorded `harness=` meta, then adds its own `FM_SEND_SETTLE` pause after successful text sends so immediate peeks catch the receiving turn starting; the sub-supervisor uses only the shared submit core and does not pay that post-submit pause. ## Worktrees, not branches in your checkout diff --git a/docs/scripts.md b/docs/scripts.md index c1785f9125..82103fabfa 100644 --- a/docs/scripts.md +++ b/docs/scripts.md @@ -26,7 +26,7 @@ Each file also starts with a short header comment. | `fm-tasks-axi-lib.sh` | Shared `tasks-axi` compatibility probe sourced by bootstrap and teardown | | `fm-wake-drain.sh` | Atomically drain queued watcher wakes before handling supervision work, then run the watcher-liveness guard | | `fm-wake-lib.sh` | Shared durable wake queue and portable lock helpers sourced by the watcher, drain, arm, guard, and daemon | -| `fm-send.sh` | Send one verified literal line (or `--key Escape`) to a direct-report window; exits non-zero on confirmed swallowed Enter; bare `kind=secondmate` targets are marked as from-firstmate; text sends pause `FM_SEND_SETTLE` seconds after success | +| `fm-send.sh` | Send one verified literal line (or `--key Escape`) to a direct-report window; exits non-zero on confirmed swallowed Enter; bare `kind=secondmate` targets are marked as from-firstmate; slash commands and codex `$...` skill invocations get popup-settle before Enter; text sends pause `FM_SEND_SETTLE` seconds after success | | `fm-tmux-lib.sh` | Shared tmux pane primitives for busy detection, dim-ghost-aware and border-aware composer detection, and verified submit retry | | `fm-peek.sh` | Print a bounded tail of a crewmate pane | | `fm-pr-check.sh` | Record `pr=` and a verified `pr_head=` when available for a PR-ready task, then arm the watcher's merge poll | diff --git a/tests/fm-send-popup-settle.test.sh b/tests/fm-send-popup-settle.test.sh new file mode 100755 index 0000000000..fcf0d2b66a --- /dev/null +++ b/tests/fm-send-popup-settle.test.sh @@ -0,0 +1,121 @@ +#!/usr/bin/env bash +# fm-send pre-submit popup-settle selection (the codex `$` fix). +# +# Some TUIs open a completion popup when the composer's first character triggers +# it: codex (and others) for a leading `/` slash command, and codex specifically +# for a leading `$` invocation (e.g. `$no-mistakes`). Submitting before the +# popup settles lets it swallow the Enter, so the line never submits. fm-send +# absorbs this by pausing `settle` seconds AFTER typing and BEFORE the (retried) +# Enter - the first sleep fm_tmux_submit_core makes. These tests pin the +# settle-SELECTION matrix hermetically (stubbed tmux + sleep, no real agent): +# +# /... -> 1.2 (universal; `/` only starts a command, never plain text) +# $... to codex -> 1.2 (scoped: codex opens a `$` popup) +# $... to claude -> 0.3 (NOT codex: `$` commonly starts plain text "$5", "$HOME") +# $... explicit -> 0.3 (session:window target has no meta -> harness unknown +# -> non-codex safe default) +# plain text -> 0.3 (fast path) +# +# The popup-settle is the FIRST sleep recorded: fm_tmux_submit_core types the text, +# then `sleep "$settle"`, then the Enter-retry loop (sleep 0.4 each) and finally +# fm-send's own post-submit FM_SEND_SETTLE pause. So tail-vs-head matters: this +# suite asserts on the HEAD sleep, distinct from fm-send-settle.test.sh which pins +# the TAIL (post-submit) pause. The retried Enter in fm_tmux_submit_core remains the +# real safety net; this settle is only the optimization that lets the popup clear so +# the first Enter lands. +# +# Every case below passes a LITERAL `$` / `$price` message in single quotes +# on purpose - the whole point is to send an unexpanded `$...` line to the agent - +# so SC2016 (which flags single-quoted `$` as a probably-forgotten expansion) is a +# false positive here and is disabled file-wide. +# shellcheck disable=SC2016 +set -u + +# shellcheck source=tests/lib.sh +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" + +SEND="$ROOT/bin/fm-send.sh" + +TMP_ROOT=$(fm_test_tmproot fm-send-popup-settle) + +# Same stub shape as fm-send-settle.test.sh: a fake tmux that drives the submit +# path to a clean "empty" verdict on the first Enter, and a fake sleep that records +# every requested duration (one per line) into FM_SLEEP_LOG instead of sleeping. +make_stubs() { # -> echoes fakebin dir + local dir=$1 fb="$1/fakebin" + mkdir -p "$fb" + cat > "$fb/tmux" <<'SH' +#!/usr/bin/env bash +set -u +case "${1:-}" in + send-keys) exit 0 ;; + display-message) + for a in "$@"; do case "$a" in *cursor_y*) printf '0\n'; exit 0 ;; esac; done + printf 'fakepane\n'; exit 0 ;; + capture-pane) printf '\xe2\x94\x82 \xe2\x94\x82\n'; exit 0 ;; + list-windows) exit 0 ;; +esac +exit 0 +SH + chmod +x "$fb/tmux" + cat > "$fb/sleep" <<'SH' +#!/usr/bin/env bash +printf '%s\n' "${1:-}" >> "$FM_SLEEP_LOG" +exit 0 +SH + chmod +x "$fb/sleep" + printf '%s\n' "$fb" +} + +# first_settle