fix(bin): enforce steer length, brief path, and red-PR safety rules - #1432
fix(bin): enforce steer length, brief path, and red-PR safety rules#1432Boxyboxy wants to merge 9 commits into
Conversation
Each rule already cost the fleet real turns, and each existed only as prose. fm-send: refuse text over 400 bytes before any send or pending-reply record. Two ~1,400-byte steers to one busy pane both reported an unconfirmed send and never arrived, while a ~230-byte pointer to the same pane landed first try. The refusal names the working pattern (append to the brief, send a pointer) and --allow-long is the explicit opt-out. The --key path is untouched. fm-brief: bake the absolute firstmate path into both the ship and the scout scaffold for any report or evidence output, and say plainly that a path inside the worktree does not survive cleanup. Three crewmates wrote evidence to a bare relative data/<id>/ path and lost it when the worktree was returned. fm-pr-merge: read the PR's check state and refuse a red PR, naming the failing checks. No configured checks and pending checks are deliberately not red; a check state that cannot be read is a refusal, because "not red" must be established rather than assumed. --allow-red-checks is the captain-authorized exception and records merge_checks_override= in the task's metadata, above the canonical pr= line so the metadata identity parse still accepts the file. Tests are colocated in the existing suites for the three scripts.
…ef refusal and write region
|
Automated reminder: thanks for the PR! This branch currently has a merge conflict with the base branch. When you get a chance, please rebase onto (or merge) the latest base branch, resolve the conflict, and push. After that, checks will re-run and the PR will get looked at again. Noted for firstmate#1432 at |
|
Closing deliberately, not abandoning the work. These changes have been merged into The work itself is unchanged and fully reviewed - seven review rounds through the no-mistakes pipeline. |
2 participants
Intent
Mechanize three firstmate safety rules that were prose-only with no enforcement, each of which already cost the fleet real turns. (1) bin/fm-send.sh must refuse an over-long steer instead of losing it silently: two ~1,400-character steers to a busy worker pane both reported an unconfirmed send and genuinely never arrived, while a ~230-character pointer to the same pane landed first try. The requested threshold was exactly 400 bytes, defined as a named constant with a comment citing that failure, with an explicit opt-out flag (--allow-long) named in the refusal text, and the refusal must name the working pattern (append the content to data//brief.md, then send a short pointer such as 're-read your brief, section X amended'). No other send path was to change: --key sends, key sequences, and the submit-verification logic are deliberately untouched. (2) bin/fm-brief.sh must bake the ABSOLUTE firstmate path into both the ship and the --scout scaffolds for any report or evidence output, and say plainly that a path inside the worktree does not survive cleanup: three separate workers were told to write evidence to a bare relative data// path, wrote it inside their disposable worktree, and lost it at teardown. Wording was deliberately kept short. (3) bin/fm-pr-merge.sh must refuse a red PR, since AGENTS.md states 'Never merge a red PR' as an absolute rule yet the blessed merge path performed no check on the PR's checks. It reads check state via the gh-axi CLI already used in this repo. The pending/missing/unreadable behaviour was a deliberate decision documented in the script header: 'no checks configured at all' is treated as NOT red (several fleet repos have no required checks and blocking them would break ordinary merges), pending is NOT red (the merge proceeds with a note on stderr), but a check state that cannot be read IS a refusal because 'not red' must be a positive finding rather than the absence of evidence. --allow-red-checks is the captain-authorized override and records merge_checks_override= in the task's metadata so the decision is durable; that record is deliberately written ABOVE the canonical pr= line because bin/fm-pr-lib.sh's metadata identity parse rejects unknown keys after pr=, and placing it after would break the watcher's merge poll. Scope was deliberately limited to exactly these three scripts plus their tests, because a live fleet of ~19 workers is being supervised by this repo's scripts right now; the supervision path (fm-watch.sh, fm-watch-arm.sh, fm-wake-*) was explicitly not to be touched. Tests were deliberately added to the existing colocated suites (tests/fm-send-strict.test.sh, tests/fm-brief.test.sh, tests/fm-pr-merge.test.sh) rather than as new files, per the repo convention of extending an existing script; tests/fm-pr-check-security.test.sh's gh-axi mock also needed a 'pr checks' answer because it drives fm-pr-merge. Two related omp-specific fixes from the same audit were explicitly out of scope: they live on a local-only branch and are blocked on a captain decision. The branch is cut from origin/main and targets main.
What Changed
bin/fm-send.shnow refuses a steer longer than a 400-byte constant before any send or pending-reply record is created, since over-long steers were reported as unconfirmed and never arrived at a busy pane. The refusal names the sanctioned pattern (append the content to the task'sdata/<id>/brief.md, then steer a short absolute-path pointer), and a per-call--allow-longflag is the only opt-out - deliberately not anFM_SEND_*environment tunable.--keysends, key sequences, and submit verification are untouched.bin/fm-brief.shresolves the firstmate root, data, and state directories to real absolute paths and refuses to scaffold rather than bake in one it could not resolve. Both the ship and--scoutscaffolds now state the absolute report/evidence path and say plainly that a relativedata/...path resolves inside the disposable worktree and is destroyed at cleanup; the ship scaffold permits that one outside write only when the task section explicitly asks for a report or evidence file, and never overbrief.md.bin/fm-pr-merge.shreads the PR check rollup as structured data throughgh pr view --json statusCheckRollupand classifies each entry itself, consulting both check-run conclusions and commit-status states so a failing external status is no longer misread as pending. Failing checks refuse to merge, and so does a check state that cannot be read; no configured checks, pending, and an all-skipped rollup are deliberately not red, with the latter two merging under a note on stderr.--allow-red-checksis the captain-authorized exception and recordsmerge_checks_override=<reason>above the canonicalpr=line via the same guarded atomic meta writefm-pr-check.shuses.AGENTS.md,docs/architecture.md,docs/configuration.md,docs/scripts.md) record the new gate-decision-via-brief contract and the three refusals. Tests extend the existing colocated suites rather than adding new files, driven by recordedghrollup payloads undertests/fixtures/gh-status-check-rollup/, andbin/fm-test-run.shmaps fixture paths to their consuming suite.Risk Assessment
✅ Low: The change is well-bounded to three scripts plus their tests, fails closed in the safety-relevant direction, has been hardened across seven prior review rounds, and now carries fixture-backed coverage of its only forge-facing logic; the two remaining findings are message-accuracy issues that do not affect the merge or send decisions.
Testing
I ran the five directly-affected suites (all pass) and then the repo's full changed-file selection, 46/46 suites with zero failures and one unrelated environmental skip because the Pi harness package is not installed on this machine. Beyond the suites I drove each guard the way an operator would and captured CLI transcripts: fm-send.sh against a real tmux pane running
cat -uon a private socket, so a refused 1.6KB steer provably typed nothing while the short absolute-path pointer echoed back as submitted, --allow-long sent with the flag consumed, and --key untouched - the same transcript also reproduces the unconfirmed-delivery failure that motivated the cap; fm-brief.sh generated into one home with both the base-commit and branch scripts and diffed, showing the new absolute-path wording in the ship rules and scout definition-of-done plus the refusal when a firstmate dir cannot be resolved; and fm-pr-merge.sh run against live GitHub pull requests with real gh auth and only the merge command stubbed, refusing a genuinely red PR by check name with no metadata written and no poll armed, merging the no-checks and green PRs, refusing an unreadable state with GitHub's own diagnostic attached, recording the captain-authorized override durably above the canonical pr= line, and covering the pending/all-skipped/external-commit-status/unreadable states from the recorded payload fixtures through the script's own filter. I also confirmed the override's placement is load-bearing by showing the shared metadata identity gate accepts it as written and rejects it moved below pr=. No pull request was merged - PR 1404 and 1420 are still open. There is no UI, HTML, or rendered surface in this change, so CLI transcripts and the generated-brief diff are the end-user artifacts; no screenshot applies. The worktree is clean and all scratch directories were removed.Evidence: fm-send over-long steer refusal - real tmux pane transcript
$ fm-send.sh af-demo "<1624-byte gate-decision text>" error: message is 1624 bytes, over the 400-byte limit; a steer this long is lost by a busy pane instead of landing. Append the content to /tmp/fm-send-ev/home/data/af-demo/brief.md, then send a short pointer such as 'READ /tmp/fm-send-ev/home/data/af-demo/brief.md, section X amended' - the absolute path, because a worker cannot find a gitignored brief by searching from its own worktree. Pass --allow-long right after the target for a deliberate long send. exit status: 1 --- pane contents after the refusal --- [pane is empty: nothing was typed, so nothing could be half-delivered] $ fm-send.sh af-demo "READ /tmp/fm-send-ev/home/data/af-demo/brief.md, section 3 amended" exit status: 0 --- pane contents: typed once, then echoed back by cat, i.e. it submitted --- READ /tmp/fm-send-ev/home/data/af-demo/brief.md, section 3 amended READ /tmp/fm-send-ev/home/data/af-demo/brief.md, section 3 amended $ fm-send.sh af-demo --allow-long "<1624-byte text>" error: text not submitted to fmdemo:fm-af-demo (delivery unconfirmed; verdict=pending; ...) --- that is the exact outcome the cap prevents ---Evidence: Generated crewmate brief - absolute report/evidence paths, before vs after
--- ship brief --- # Rules 1. Never push to the default branch. Never merge a PR. -2. Stay inside this worktree; modify nothing outside it. +2. Stay inside this worktree; the status file below is the only file you write outside it. + The single exception: if the# Tasksection above explicitly asks you for a report or evidence file, write it at the absolute path/tmp/fm-evidence-home/data/af-ship1/report.md, or at the absolute path of an evidence file that section names in that same directory - never/tmp/fm-evidence-home/data/af-ship1/brief.md, which is these instructions and may be amended with a decision you must read - and nowhere else outside this worktree. + Never use a relativedata/...path for it: a relative path lands inside this worktree, and the worktree is destroyed at cleanup. --- scout brief --- # Definition of done -Write your findings to/tmp/fm-evidence-home/data/af-scout1/report.md. +Write your findings to the absolute path/tmp/fm-evidence-home/data/af-scout1/report.md. +Use that absolute path, never a relativedata/...one: a relative path lands inside this worktree, and the worktree is destroyed at cleanup. $ FM_DATA_OVERRIDE=relative/does-not-exist fm-brief.sh af-x firstmate --scout error: firstmate data dir 'relative/does-not-exist' is relative and does not exist, so a brief cannot name a path the crewmate can reach from its own worktree; set FM_HOME or FM_DATA_OVERRIDE to an absolute path exit status: 1Evidence: fm-pr-merge red-PR refusal - live GitHub check states, merge stubbed
# CASE 1 - a genuinely red PR (real checks read from GitHub) FAILURE PR must be raised via no-mistakes FAILURE Stock macOS Bash snapshot compatibility SUCCESS (8 others) $ fm-pr-merge.sh task-red #1404: PR #1404 has failing checks, refusing to merge: PR must be raised via no-mistakes Stock macOS Bash snapshot compatibility Fix the checks, or pass --allow-red-checks for a captain-authorized exception. exit status: 1 --- merge commands actually issued --- [none: gh-axi pr merge was never invoked] # CASE 4 - a check state that CANNOT be read (real gh failure) error: could not read the check state of PR ... (gh could not return the PR's check rollup: GraphQL: Could not resolve to a Repository with the name 'example/no-such-repo-fm'. (repository)), refusing to merge; "not red" must be established, not assumed. # CASE 5 - captain-authorized override on the SAME real red PR 5:mode=no-mistakes 6:merge_checks_override=failing checks: PR must be raised via no-mistakes; Stock macOS Bash snapshot compatibility 7:pr=https://github.com/kunchenguid/firstmate/pull/1404 8:pr_head=13193229bd27b62ea11603cec0b112d6da457dc7 # CASE 6 - recorded forge payloads through the script's own filter note: 1 check(s) are still pending; pending is not failing, so the merge proceeds note: all 4 check(s) are skipped or cancelled, so no check actually passed; skipped is not failing, so the merge proceeds error: PR ... has failing checks, refusing to merge: vercel/preview # CASE 7 - override placement override ABOVE pr= -> identity parse: ACCEPTED override BELOW pr= -> identity parse: REJECTEDEvidence: Full changed-file regression summary
Pipeline
Updates from git push no-mistakes
... (4 earlier update rounds omitted to keep the PR body within GitHub's 65536-char limit; full history is in the run log.)
🔧 Fix: read PR check rollup as structured data; resolve brief paths
6 issues (3 warnings, 3 infos) still open:
bin/fm-pr-merge.sh:197- record_checks_override rebuilds the task's meta with two greps and|| true, which is weaker than the while-read reconstruction bin/fm-pr-check.sh:95-101 already uses for the identical job on the identical file.|| truecannot distinguish grep's exit 1 (no matching line, expected) from exit >=2 (a real read error), and GNU grep that decides the file is binary prints "Binary file ... matches" on stdout instead of the lines. Either way the tmp is built from partial content, and the validation that follows does not catch it: fm_pr_metadata_identity_parse only rejects unknown keys that appear AFTER pr=, so a tmp holding one junk line plusmerge_checks_override=plus the pr block parses clean, gets moved over the meta, and silently drops window=, worktree=, project=, harness=, and mode= - which is what teardown's landed-work check and the watcher read. Reuse fm-pr-check.sh'swhile IFS= read -r lineloop over $META instead; it is immune to both failure modes and removes the second grep pass.bin/fm-brief.sh:98- DATA is now resolved to a real absolute path (or refused), but STATE on the next line is still${FM_STATE_OVERRIDE:-$FM_HOME/state}with no resolution, and STATE is what gets baked into every scaffold as the status file at line 134/389. Run the change's own scenario -cd <home> && FM_HOME=. bin/fm-brief.sh <id> proj, which tests/fm-brief.test.sh:test_relative_home_is_resolved_or_refused exercises - and the emitted brief names an absolute/abs/home/data/<id>/report.mdfor evidence but a relative./state/<id>.statusfor status, directly under new rule 2's claim that "the status file below is the only file you write outside it". The crewmate's status appends then land inside its disposable worktree and vanish at cleanup, which is a worse instance of the exact loss this change exists to prevent: a lost report costs the findings, a lost status file costs every wake firstmate supervises the task by. Resolve STATE through the samecd -- && pwd/ refuse-if-relative block as DATA.bin/fm-pr-merge.sh:118- read_check_state sends gh's stderr to /dev/null, so the hard refusal it feeds reports only "gh could not return the PR's check rollup" with no cause. This gate now blocks every task PR merge, and the distinct causes need distinct operator actions: an expired token needsgh auth login, a 403 needs a scope, a rate limit needs a wait, a jq filter error needs a code fix, and a missing gh needs an install - the message advises "retry once gh can reach the PR" for all five. fm-pr-check.sh can afford2>/dev/nullthere because its pr_head lookup degrades to not recording a field; here the same suppression drops the only evidence for a stop-the-line refusal. Capture the CLI's stderr and append it to the refusal text.bin/fm-pr-merge.sh:146- The commit-status branch maps EXPECTED to skip, but GitHub's EXPECTED means a required status context that has been declared and has not reported yet - semantically pending, not skipped. The merge decision is the same either way, but the evidence is not: with a rollup of one SUCCESS check run plus one EXPECTED required status, CHECK_PENDING stays 0 and CHECK_SKIP != CHECK_TOTAL, so neither note at lines 276-286 fires and the merge proceeds with no output at all. That is the one outcome the header's own design rules out ("it merges with its own note on stderr rather than silently"). Move EXPECTED into the pending class so the note fires. Note also that NEUTRAL in that same branch is dead: NEUTRAL is a CheckConclusionState, never a StatusState.bin/fm-pr-merge.sh:195- record_checks_override mirrors fm-pr-check.sh's meta-write idiom but omits three of its guards: there is no EXIT trap, so an interrupt between mktemp and mv leaves a mode-0600.fm-pr-merge-meta.XXXXXXorphaned in state/ (fm-pr-check.sh:80-86 traps exactly this); there is no[ "$META_DEVICE" = "$STATE_DEVICE" ]assertion, so if the meta ever sits on a different device from state/ themvdegrades from an atomic rename to a copy, which is the window this whole idiom exists to close; and there is no post-mv re-validation of $META (fm-pr-check.sh:112-113 re-runs fm_pr_private_file_valid and the identity parse on the destination). Practically these are covered because fm-pr-check.sh asserted device parity moments earlier, so this is a parity gap in a security-hardened file rather than a live hole.docs/architecture.md:195- Both new refusals are absent from the docs that describe these scripts. docs/architecture.md:195 still says fm-pr-merge.sh "records pr= and any available pr_head= through bin/fm-pr-check.sh before calling gh-axi pr merge" with no mention of the red-check gate or --allow-red-checks, docs/scripts.md:85 summarizes it the same way, and docs/architecture.md:86 describes fm-send.sh's settle behavior with no mention of the length cap or --allow-long. An agent that reads architecture.md to learn the merge path will not learn that merging a red PR is now mechanically blocked, which is the one fact most worth discovering there.🔧 Fix: adopt sibling meta-write guards; resolve every brief path
3 issues (1 warning, 2 infos) still open:
tests/fm-brief.test.sh:578- The two negative guards in test_relative_home_is_resolved_or_refused can never match, so they prove nothing. assert_no_grep is a fixed-string grep (tests/lib.sh:206,grep -F --), but the patterns are written as regexes: line 578 is '\./data/' and line 580 is "'\./state/" - both carry a literal backslash (in double quotes bash keeps.intact, since.is not one of $" \ newline). A brief that regressed to a relative home would contain./data/ and './state/, never./data/ or './state/, so both assertions pass whether or not the defect is present. The positive assertions two lines above ($abs/data/$id/report.md and $abs/state/$id.status) are what actually cover the property today; these two read as extra safety and deliver none. Drop the backslashes so the fixed strings are '`./data/' and "'./state/".bin/fm-brief.sh:78- FM_ROOT is the third firstmate path the scaffolds bake in, and it is the one that did not get the new treatment. The header now claims "Every firstmate path a scaffold bakes in - the report or evidence dir and the status file alike - is resolved to a real absolute path first", and DATA/STATE go through resolve_home_dir at lines 104-105, but line 78 leaves FM_ROOT as ${FM_ROOT_OVERRIDE:-<absolute>} unresolved. That value is baked into the scaffolds at lines 231, 303, 417 and 420 as paths the crewmate must read or execute from inside its worktree ($FM_ROOT/.agents/skills/decision-hold-lifecycle/SKILL.md, $FM_ROOT/bin/fm-herdr-lab.sh, $FM_ROOT/bin/fm-ensure-agents-md.sh). With a relative FM_ROOT_OVERRIDE the scout brief tells the worker to read a relative skill path that resolves inside its disposable worktree and is not there - the same failure class this change exists to close, on the path that gates scout completion. Every production setter (fm-fleet-snapshot.sh, fm-bootstrap.sh, fm-config-push.sh) passes an absolute $FM_ROOT today, so this is a consistency gap rather than a live break, but the same was true of FM_HOME before this change. Route FM_ROOT through the same resolve_home_dir call.bin/fm-pr-merge.sh:122- CHECK_ROLLUP_FILTER is the only forge-facing logic this change adds, and no test exercises it. Every case in tests/fm-pr-merge.test.sh mocks gh at the wrong layer: GH_ROLLUP_MOCK_BODY emits already-formatted "<conclusion>|<state>|<name>" rows, so the jq expression that produces them - the.state // ""arm that fixes the original commit-status bug, the.name // .contextfallback, theerror(...)branch that makes a missing rollup unreadable - is never evaluated. I ran the filter against real gh 2.92.0 and it is correct today (a red PR yieldsrollup|10plus one row per check; a repo with no workflows yieldsstatusCheckRollup: []and sorollup|0, confirming the documented no-checks path). But the defect this change was written to fix was precisely a consumed representation drifting from the forge's data, and the current suite would not notice that happening again. A case that pipes a recordedgh pr view --json statusCheckRollupJSON fixture through the filter would close it; note that gh evaluates-qwith its embedded jq, so a system-jq test is close but not identical.🔧 Fix: cover rollup filter with recorded payloads; resolve brief root
3 issues (1 error, 2 infos) still open:
tests/fixtures/gh-status-check-rollup/README.md:1- The new fixture README is a tracked markdown file inside the maintained-prose scope, but it is not classified in docs/documentation-audiences.json, so bin/fm-doc-audience-check.sh fails. That checker's scope is the fixed pathspec*.md*.mdx*.rst*.txtdocs/examples/*(bin/fm-doc-audience-check.sh:145 and REQUIRED_TRACKED_PATTERNS), resolved throughgit ls-files -- <patterns>(line 40-50), and a git pathspec of*.mdmatches at any depth. Reproducing its own set logic (missing = tracked - classified, line 175-186) against this worktree yields exactly one entry:tests/fixtures/gh-status-check-rollup/README.md. The check therefore exits withunclassified: tests/fixtures/gh-status-check-rollup/README.md, and tests/fm-documentation-audiences.test.sh:56 (test_repository_inventory_passes) runs that check against the real repo root, so the suite fails. This will not surface in a --changed run either: the new tests/fixtures/* branch in bin/fm-test-run.sh:717 resolves the fixture only to tests/fm-pr-merge.test.sh, so the doc-audience suite is never selected and the break lands in the full suite and CI instead. Note the sibling fixture dir tests/fixtures/quota-array-dispatch/ carries only .json files, which is why no prior fixture ever needed a classification. Fix: add {"path": "tests/fixtures/gh-status-check-rollup/README.md", "audience": "maintainer-verification"} to docs/documentation-audiences.json - that class is defined as recording repeatable evidence for an active guarantee, which is exactly what the file does.bin/fm-pr-merge.sh:303- The rollup is read at line 303, then bin/fm-pr-check.sh runs (line 337 - guard, non-executing legacy migration, poll prepare/publish, several forge and filesystem round trips), and only then does gh-axi pr merge fire at line 369. Nothing binds the merge to the commit whose checks were read, so a push that lands in that window merges a head this gate never inspected - the one outcome the rule exists to prevent. The window is seconds and needs a concurrent push, so it is narrow rather than likely. The material is already at hand to close it: fm-pr-check.sh records the forge's exact head as pr_head= in the task's meta, and gh pr merge accepts --match-head-commit <sha>, which refuses server-side when the head moved. Reading pr_head= back after line 338 and passing it through (when recorded; GitLab tasks and gh's degraded head lookup record none) would make the merge apply to the commit the gate actually cleared. Worth a captain call rather than a mechanical edit, since it adds a flag to the blessed merge path.bin/fm-pr-merge.sh:251- record_checks_override (lines 251-283) is now a near-line-for-line copy of bin/fm-pr-check.sh:92-118: same device-parity assertion, same mktemp under $STATE, same while-read reconstruction, same chmod 0600, same fm_pr_private_file_valid + fm_pr_metadata_identity_parse on the temp, same fm_pr_regular_destination_on_device_or_absent before the mv, same post-mv revalidation. Two independent copies of a security-hardened atomic write on the identical file mean the next guard added to one has to be remembered for the other - which is exactly how this copy came to be missing three guards in round 3 and needed a follow-up round to regain them. bin/fm-pr-lib.sh already owns every primitive both callers use and would be the natural home for one fm_pr_meta_rewrite helper taking a filter callback or a key/value pair. Flagging as info rather than proposing the edit: the script header at lines 244-250 states the duplication was deliberate ("that writer's reconstruction and its whole guard set rather than a lookalike"), and consolidating touches a security-sensitive shared library, so it is the captain's call whether the coupling is worth it.🔧 Fix: fold fixture README provenance into consuming test header
3 infos still open:
bin/fm-pr-merge.sh:313- When the rollup is partly readable (CHECK_SEEN != CHECK_TOTAL at line 306) AND the rows that were read include failures, the UNREADABLE branch is tested first, so the refusal prints only "could not read the check state ... Retry once gh can reach the PR" and discards the failing check names tally_check_rows already extracted into FAILING. The operator is told to retry the CLI when the actual problem is a red PR. The same precedence costs the durable record: under --allow-red-checks, OVERRIDE_REASON becomes "check state unreadable: ..." instead of naming the checks that were overridden, so the captain-authorized exception is recorded without the evidence that motivated it. Both paths still refuse, so there is no safety hole. Suggest appending the extracted failing names to the unreadable message (and to OVERRIDE_REASON) when CHECK_FAIL > 0, so a partly-read red rollup reports both facts.bin/fm-brief.sh:98- resolve_home_dir's refusal text is shared by all three call sites and ends with "set FM_HOME or $override to an absolute path". That is correct for the data and state dirs, which default to $FM_HOME/data andbin/fm-brief.sh:395- The ship scaffold's permitted outside-worktree write is "under$DATA/$ID/- at the absolute path$DATA/$ID/report.md, or a named file beside it in that directory". That directory also holds$DATA/$ID/brief.md, and this same branch changes AGENTS.md section 7 to deliver every gate decision by appending it to that file and steering the worker to re-read it. A worker asked for an evidence file, writing "a named file beside it", can therefore overwrite the one channel firstmate now uses to hand it a captain decision, with no error and no trace. The scout scaffold does not have this exposure: its rule 2 names "the report and the status file" specifically rather than a directory. Probability is low (a worker would have to pick that exact name), but the collision is new to this branch and one clause closes it - e.g. name the permitted files as report.md or an evidence file, never brief.md. Flagging rather than editing because this sentence has been deliberately retuned across several rounds.🔧 Fix: report red checks in unreadable rollups; fix brief refusal and write region
3 issues (1 warning, 2 infos) still open:
bin/fm-send.sh:262- The length refusal teaches a pointer pattern that the same branch's AGENTS.md says structurally does not work. AGENTS.md:299-300 (changed in this branch) now requires the steer to give "that brief's ABSOLUTE path and tell the worker to READ it, never to find or look for it", with the stated reason that "data/is gitignored, so a worker searching from its own worktree structurally cannot find the file". The refusal's suggested pointer is "re-read your brief, section X amended" - no path, and "re-read" rather than READ. That is exactly the shape AGENTS.md rules out. It matters because the brief reaches most workers as piped CONTENT, not a path: bin/fm-spawn.sh:429-452 launches claude/codex/opencode/pi/grok with"$(... encode launch-brief < __BRIEF__)", so only the kimi adapter (fm-spawn.sh:1525) ever hands over a path. A scout worker following the refusal's advice has no absolute path anywhere in its brief (the scout scaffold names only report.md) and cannot find a gitignored file outside its worktree. The refusal already computes the absolute path in BRIEF_HINT one line above, so it has the value it omits from the example. I am flagging rather than editing because this exact wording was your round-1 instruction; the conflict is that AGENTS.md §7 was rewritten afterwards. Suggest aligning the example with the contract, e.g. "...then send a short pointer such as 'READ <that path>, section 3 amended'".tests/fm-brief.test.sh:636- The state-dir refusal case runs without FM_HOME, so it does not test what its guard claims. With FM_HOME unset, fm-brief.sh derives FM_HOME from FM_ROOT (bin/fm-brief.sh:111-112), making DATA resolve to the firstmate repo's own$ROOT/data. Today the STATE refusal fires first so nothing is written, but if that refusal ever regressed the brief would be scaffolded into$ROOT/data/brief-relative-state-refused/brief.md- inside the checkout, in a directory.gitignore:3hides, so the stray write leaves no trace - while the test'sassert_absent "$home/data/brief-relative-state-refused/brief.md"watches a path that is never created under either behaviour and passes regardless. Theexpect_code 1above it still catches the regression, so this is a blind guard plus a checkout-pollution hazard rather than a hole. The FM_DATA_OVERRIDE case at line 624 has the same missing FM_HOME (its own assert_absent happens to point at the right relative path). Fix: pass FM_HOME="$home" to both cases, as the root-refusal case at line 615 already does, and point the state case's assert_absent at "$home/data/...".bin/fm-test-run.sh:717- The new fixture mapping resolves families frombasename $(dirname $path), which is only correct for a fixture exactly one directory deep. A fixture placed directly attests/fixtures/<file>.jsonyields the reference string "fixtures", and families_for_test_reference (line 586-597) then greps every test script for that word - matching tests/fm-test-run.test.sh:6 ("exercise the runner with fixtures") and any other suite whose prose mentions fixtures, while quite possibly missing the suite that actually reads the file. A fixture nested deeper (tests/fixtures/a/b/c.json) resolves to "b" with the same failure mode. Both existing fixture dirs happen to be one level deep, so nothing is broken today; the mapping just quietly stops meaning what its comment says outside that shape. Suggest matchingtests/fixtures/*/*for the dirname rule and leaving a direct child of tests/fixtures/ as__unmapped__, so a misplaced fixture fails loudly at selection time instead of selecting an arbitrary family set.🔧 Fix: align send refusal pointer with contract; fix brief test guards
2 infos still open:
bin/fm-pr-merge.sh:190-NEUTRALis grouped withSKIPPED|CANCELLEDin classify_check, so a rollup whose entries are all NEUTRAL trips the all-skip branch and prints "all N check(s) on <url> are skipped or cancelled, so no check actually passed". Nothing was skipped or cancelled, and GitHub treats a NEUTRAL conclusion as a passing, non-blocking outcome for branch protection - so the note tells the captain no check passed when in fact every check completed acceptably. The merge decision is unaffected (NEUTRAL is correctly not red either way); only the evidence line is wrong. The adjacent comment already reasons carefully about why EXPECTED belongs in pending rather than skip; NEUTRAL deserves the same separation - either its ownpass-adjacent class, or wording for the note that covers "skipped, cancelled, or neutral".bin/fm-send.sh:240- The new MAX_TEXT_BYTES=400 cap is a hard refusal on a path that has an automated internal caller: bin/fm-pending-reply-lib.sh:685 resendsfm_pending_reply_recovery_messagethrough fm-send.sh when a marked secondmate request goes unanswered. I measured that message's worst case at 324 bytes (160-byte fixed prefix + the 120-char request_summary bound from fm_pending_reply_summarize + the 22-byte FM_FROMFIRST_MARK and correlation token added by fm_pending_reply_embed_corr), so it fits today with 76 bytes of headroom. Nothing ties the two constants together, though, and the failure mode if that headroom is ever consumed - a longer prefix, or raising the 120-char summary bound - is silent and misleading: fm-send refuses with "message is N bytes, over the 400-byte limit", fm_pending_reply_send_recovery reads only a non-zero status and recordsrecovery_failed, and the escalation reports a delivery failure rather than a message that was never allowed to be sent. Pin the relationship: assert the recovery message's bound against MAX_TEXT_BYTES in tests/fm-send-strict.test.sh or the pending-reply suite, or have the recovery resend pass --allow-long since it is a firstmate-generated message of known bounded size, not an operator steer.✅ **Test** - passed
✅ No issues found.
bin/fm-test-run.sh tests/fm-send-strict.test.sh tests/fm-brief.test.sh tests/fm-pr-merge.test.sh tests/fm-pr-check-security.test.sh tests/fm-secondmate-safety.test.sh- 5/5 suites passbin/fm-test-run.sh --changed --base 99533c5d- 46/46 suites pass, 0 failures, 1 unrelated environmental gate skip (Pi harness package absent)Manual E2E:bin/fm-send.sh <task> "<1624-byte text>"against a REAL tmux pane runningcat -uon a private tmux socket - refused, pane verified emptyManual E2E:bin/fm-send.sh <task> "READ <abs>/brief.md, section 3 amended"- typed once and echoed back by the pane (submitted)Manual E2E:bin/fm-send.sh <task> --allow-long "<520-byte text>"- exit 0, text typed, flag consumed not typed; and with the 1624-byte text, reproduced the unconfirmed-delivery outcome the cap exists to preventManual E2E:bin/fm-send.sh <task> --key Enter- unaffected by the length guardManual E2E:bin/fm-brief.sh af-ship1 firstmateandbin/fm-brief.sh af-scout1 firstmate --scoutunder one FM_HOME, generated with both the base-commit and branch scripts and diffedManual E2E:FM_DATA_OVERRIDE=relative/does-not-exist bin/fm-brief.sh ... --scoutandFM_ROOT_OVERRIDE=relative/does-not-exist bin/fm-brief.sh ...- both refuse with the settable variable namedManual E2E against LIVE GitHub (realghauth, merge command stubbed):bin/fm-pr-merge.sh task-red https://github.com/kunchenguid/firstmate/pull/1404- refused, both failing checks named, no pr= recorded, no poll armedManual E2E against LIVE GitHub: PR 1425 (no configured checks) and PR 1420 (fully green) both proceed to the merge call with pr=/pr_head= recorded and no override lineManual E2E:bin/fm-pr-merge.sh task-un https://github.com/example/no-such-repo-fm/pull/7- real gh failure, refusal carries GitHub's own GraphQL diagnosticManual E2E:bin/fm-pr-merge.sh task-ov https://github.com/kunchenguid/firstmate/pull/1404 --allow-red-checks- merges and writes merge_checks_override=<failing check names> above the canonical pr= lineManual E2E: recorded payloadstests/fixtures/gh-status-check-rollup/{expected-required-status,all-skipped,commit-status-failure,unreadable}.jsonreplayed through fm-pr-merge.sh's own -q rollup filter - pending note, all-skipped note, external-commit-status refusal, unreadable refusalManual check:fm_pr_metadata_identity_parse(bin/fm-pr-lib.sh) ACCEPTS the meta as written and REJECTS the same content with the override line moved below pr=Post-run verification viagh pr viewthat PR 1404 and PR 1420 remain OPEN and unmerged✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.