feat(bin): add head-keyed PR review ledger with opt-in reviewed-head merge handoff - #16
Merged
Merged
Conversation
) * Fix dispatch resolver model and receipts * no-mistakes(review): Drop model-drift branch, harden receipt lock and brief join * no-mistakes(review): Scope receipt recording to clear, report failed joins, measure latency * no-mistakes(review): Narrow dispatch clause and concurrency test, shrink lock budget * no-mistakes(review): Accept --project on the join, assert drop-or-append concurrency * no-mistakes(review): Split lock budgets by path, drop receipt size bound * no-mistakes(review): Record brief_path as spelled, drop abs_path normalization * no-mistakes(review): Pin model in contract, bound receipt latency, record reason * no-mistakes(review): Report dropped resolution receipts, project profile agreement, drop dispatch_id * no-mistakes(review): Enforce append-only cmp, complete join example, govern latency bound * no-mistakes(review): Keep no-rules exit 0 without jq, dedupe error default * no-mistakes(review): Refuse symlinked receipts path, drop dead no_rules jq argument * no-mistakes(document): Document receipt identity, symlink refusal, jq exit narrowing
* fix(bin): refuse an unrecognised fm-send flag instead of sending it as text fm-send's option loop ended in an unconditional `*) break ;;`, so any token it did not recognise - including one obviously shaped as a flag - fell out of the loop and became the positional message body. A steer invoked with a flag that does not exist was durably written into a live worker's steering inbox as the literal flag string while fm-send exited 0, so the worker was mis-steered and the caller got a success code and no diagnostic. The accepted set is now an allowlist rather than a pattern. --key is a real, supported flag parsed after this loop and must keep falling through it untouched, so a blanket "starts with -- and matched no case arm, therefore refuse" rule would have broken it. A bare -- ends flag parsing, which is how a message whose text starts with -- is sent. That separator is threaded to the two --key dispatch points so text after it is text everywhere rather than being re-parsed as a flag. A single-dash word was never a flag here and still needs no separator. The refusal exits before anything is marked, recorded, rung, or typed, the same discipline the header already applies to an empty message. * no-mistakes(review): drop -- end-of-flags separator, keep pure flag allowlist * no-mistakes(document): document fm-send's flag allowlist and leading-`--` message limit * docs(bin): drop the flag-allowlist commentary from fm-send's source The header block in bin/fm-send.sh is that script's documented contract. Recording the no-end-of-flags-separator limitation there amends that contract and turns a deliberate, narrow behaviour change into a documented guarantee the project would then owe. The rationale comment above the option loop goes for the same reason: the limitation describes a decision, which belongs in the pull request, not in the source, where it reads as a promise. Removes only those thirteen comment lines. The refusal itself is unchanged: the option loop remains a pure allowlist, --key still falls through to its own plane untouched, there is no end-of-flags handling, the usage line is unmodified, and the tests are untouched. * fix(bin): refuse trailing arguments after fm-send's --key The option loop breaks at --key without consuming what follows it, and the key path reads only the key itself, so every remaining argument was discarded in silence while the key was still delivered and the command still exited 0. `fm-send.sh lane --key Enter --not-a-real-flag` sent Enter and reported success. That is the same silent-delivery shape the unknown-flag refusal in this change exists to remove, so the key path contradicted the contract on that one path. The same ordering bypassed the --fire-and-forget incompatibility: FIRE_AND_FORGET_ID is only set when the flag precedes --key, so `--key Enter --fire-and-forget x` passed both existing guards. The key path now refuses any trailing argument before delivering the key, naming the offending token in the wording already used for an unknown flag in flag position, and names --fire-and-forget specifically so that incompatibility holds on either ordering. Adds regression coverage for both orderings and for a trailing plain word; both new tests fail before this commit and pass after it.
* Add head-keyed PR review policy ledger * Add post-merge browser QA gate * Fix PR review and post-merge gates * Close remaining PR review gate gaps * Harden migration risk and QA evidence parsing * Close PR review guard bypasses * Tighten review evidence boundaries * Bind final review authorization * Invalidate stale review dispositions * Harden review evidence validation
* Add keyed decision defer mode * no-mistakes(review): Fix defer date identity, hold age, parent channel, reporting * no-mistakes(review): Derive board defer from the option's until alone * no-mistakes(review): Show the defer date on the board card * Fix deferred decision lifecycle edges * no-mistakes(review): Drop fabricated defer hold reason fallback * no-mistakes(document): Correct stale captain-defer docs for the recorded answer path * Fix defer intake failure edges * Require future dates for decision defers * no-mistakes(review): Narrow UTC day parsing; fix elapsed-defer recovery guidance * no-mistakes(review): Refuse duplicate board option values; fix defer recovery wording * Stabilize chat defer hold assertion * Keep chat defer date stable across midnight * Refactor defer validation for bounded lint
* fix(brief): forbid validation auto-accept * no-mistakes(review): restore fleet-wide --yes ban, add ask-user routing sentence * no-mistakes(ci): Fixed a flaky test that failed the "Behavior portable serial 4" shard. Failure: tests/fm-pi-branch-extension.test.sh -> test_captain_outcome_processing_turn_is_sequence_keyed_and_re_presented, with "Error: supervision branch prompt settled but produced no durable outcome for its claimed wake rows" (thrown at .pi/extensions/fm-branch-supervision.ts:1548). Nothing in this PR's diff (the --yes DoD line, the harness-adapters sentence, three brief assertions) touches that extension or test; the other two check runs on the same head commit (99a0187) passed. It is a pre-existing race that surfaces on a slow/loaded runner. Root cause: in fm-branch-supervision.ts a wake builds the branch session (ensureBranch), then runs several awaited subprocesses (flushMirror, actingAsOwner, scopeForUnreadWake, writeEligibleRowsSnapshot, away-posture read-back) and only then snapshots reportRevisionBeforePrompt immediately before session.prompt(...); after the prompt settles it requires that revision to have advanced. The test synchronized on the wrong point: `settle(() => __fmSessions.length === 2, "replacement branch session")`. Session creation precedes that snapshot, so when the extension's pre-prompt work is slower than the test's report append, report2's durable append lands before the snapshot and the wake rejects its own settled prompt as outcome-less. The routine wake earlier in the same test already waits on __fmPrompts.length === 1 and is unaffected. Fix (tests/fm-pi-branch-extension.test.sh:1377, 9 insertions / 1 deletion): wait for the wake prompt as well as the replacement session, matching the routine wake's own idiom, with a comment naming why the built session is not the synchronization point. No production code changed; no new machinery. Verification: reproduced the exact CI error deterministically by temporarily injecting a delay ahead of reportRevisionBeforePrompt (delays 100/200/300/400/500/700 ms all failed with the identical message); that injection was reverted (git status shows only the test file modified). With the fix the test passes under injected delays of 100, 400 and 1500 ms. Full file run: exit 0, 45 tests passing. 24 parallel runs of the target test: 24/24 pass. shellcheck -x on the changed file is clean, and this PR's own tests (tests/fm-brief.test.sh, tests/fm-ask-user-authority.test.sh) still pass. The change is left uncommitted in the worktree, since prior rounds' commits on this branch were made by the executor rather than this phase
* docs: audit AGENTS.md size and ownership * docs: slim always-loaded Firstmate contract * no-mistakes(review): drop audit doc, dedupe skill triggers, fix stale pointers * no-mistakes(review): fix yolo brief split, state guard, and stale pointers * no-mistakes(review): restore backstop wake duty, dedupe trigger, repoint pointers * no-mistakes(document): Repoint stale brief guidance comment
… avoid jq argv overflow (#10) * fix: bound startup reconciliation and large fleet input * no-mistakes(review): Drop redundant contribution-input EXIT trap in fleet snapshot * no-mistakes(test): Widen cleanup deadline test budget to avoid load flakes * no-mistakes(document): Document startup summary deferral and herdr cleanup deadline * no-mistakes(ci): Lint 1 failed because ShellCheck SC2329 ("function never invoked") fired at tests/fm-herdr-session-cleanup.test.sh:356. That line is a subshell copy of fixture_workspaces that replaces the file's main version. The fake herdr command calls fixture_workspaces indirectly when it answers `workspace list` and `api snapshot`, and ShellCheck can't see that call. The fix is one comment line above the replacement: `# shellcheck disable=SC2329 # invoked indirectly by the fake herdr workspace list.` The same file already does this for its other indirectly-called replacements (lines 43 and 49), as do tests/fm-daemon.test.sh and tests/fm-bootstrap.test.sh. No behavior changed. Checked locally: `bin/fm-lint.sh tests/fm-herdr-session-cleanup.test.sh` passes with pinned ShellCheck 0.11.0 and full extended analysis, and `bash tests/fm-herdr-session-cleanup.test.sh` passes every test, including the journal-read-count, deadline, lock and identity tests. The change is not committed
…ger fixtures (cherry picked from commit 5118fbce1f5ba294d74ec0862913a5c4bce7129d)
…e path (cherry picked from commit 53740853205c45ae4c8b835656224a60708998d6)
… pending remeasurement
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @twilwa, your pull request is larger than the review limit of 150,000 diff characters
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbaec58be7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| --slurpfile rollup "$TMP/rollup.json" \ | ||
| --slurpfile policy "$POLICY" ' | ||
| def pages($x): ($x[0] | add // []); | ||
| def external: select((.user.login // .author.login // "") != $author); |
There was a problem hiding this comment.
When the authenticated collector is also the pull-request author—the normal case when one GitHub account opens the PR and posts the final disposition—external removes that disposition comment from the snapshot. The later checkpoint therefore can never set observed_in_latest_snapshot to true, so every ready and merge attempt remains permanently blocked; retain collector-authored comments so the exact bound disposition URL can be recognized and excluded afterward.
AGENTS.md reference: AGENTS.md:L125-L128
Useful? React with 👍 / 👎.
Comment on lines
+76
to
+78
| jq -cn --argjson files "$COUNT" --argjson lines "$LINES" \ | ||
| '{level:"low",reason:("bounded reversible implementation: " + ($files|tostring) + " files and " + ($lines|tostring) + " changed lines, with no high-stakes surface detected")}' | ||
| fi |
There was a problem hiding this comment.
Any change below the file and line thresholds whose filenames miss the preceding regex is classified low unconditionally. A small public-interface, authorization, production, or data-loss change in a generic path such as src/router.ts or src/middleware.ts therefore skips the configured high-stakes Fable and independent-review requirements; unmatched implementation code should remain high unless an explicit low-risk rule proves otherwise.
AGENTS.md reference: AGENTS.md:L125-L128
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…a function only ever called by a trap must carry `# shellcheck disable=SC2329`, or the full-analysis lint fails. This PR added `reap_zombie_owner` in tests/fm-herdr-session-cleanup.test.sh, called only by `trap reap_zombie_owner EXIT`, without that directive. A local run of `bin/fm-lint.sh --partition 2of2` with the pinned ShellCheck 0.11.0 exited 1 with that single SC2329 finding (line 454). In CI the job was stopped (exit 143) at about 10.5 minutes, before it printed the finding; main's partition 2 took 441 s. Fix: added the directive, worded like the file's existing ones (lines 43, 49, 365). No other sites: that was the only partition-2 finding, and partition 1 passed in CI. Verified: `shellcheck --norc --external-sources -- tests/fm-herdr-session-cleanup.test.sh` exits 0. Not rerun: the full 24-minute partition after the fix, and the test itself (Test stays skipped). The fix is uncommitted in the worktree. ci-1 (Behavior portable serial 3), not caused by this PR, flaky, no change. The only failure is tests/fm-watch-checkpoint.test.sh, "watch lock pid survived quiet checkpoint timeout". bin/fm-watch.sh takes its singleton lock at line 2327 but only sets up its cleanup-on-exit trap at 2456; a timeout in between leaves .watch.lock/pid behind. Reproduced locally: `timeout 0.6`–`1.0` leaves the pid file, 0.2/0.4/1.5/2 s do not. fm-watch.sh, fm-watch-checkpoint.sh and the test are unchanged from base 040b337. The only changed file the watcher uses (fm-captain-hold.sh) runs at wake time, not during startup. The same code passed on main. Closing the gap means changing upstream watcher code, beyond this carry-forward; worth fixing separately. ci-3 (PR must be raised via no-mistakes), not caused by the code, no change. It fails with "Required no-mistakes pipeline steps are not completed: test (status=skipped)", which is expected because the user intent keeps Test skipped. ci-4 (Review changed files (advisory)), external, no change. It fails with "No OpenRouter API key configured": a missing repository secret, not a code defect
twilwa
changed the title
This was referenced Sep 25, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Supersedes #11
Finish PR 16's fork-local review-policy fixes so GitHub reviewed-head handoff is required only when the repo policy opts in, use the fork's configured Claude Opus 5.5 model with zero mandatory independent reviews, and retain collector-authored feedback. Leave fm-pr-risk.sh P1 #2 as a separate follow-up and note it in the PR body. Keep Test skipped under the captain's fork-local rule; run Review, Document, and Lint, push through this isolated gate, and wait for fork CI. Do not merge, force-push, push main, or publish the prohibited commits.
What Changed
Adds a pull request review ledger for GitHub PRs. New scripts
bin/fm-pr-review.sh,bin/fm-pr-review-snapshot.shandbin/fm-pr-risk.shstore review state per PR head: checkpoints, dispositions, attestations, holds and post-merge QA. Their settings live in the new.github/firstmate-review-policy.json. In this fork it setsrequire_reviewed_head_handoff: false, namesclaude-opus-5-5as the high-stakes no-mistakes model, and requires 0 independent agent reviews. The snapshot now keeps inline review-thread comments written by the collecting account instead of dropping them.bin/fm-pr-merge.shreads that policy on GitHub merges:FM_PR_REVIEW_EXPECTED_HEADonly whenrequire_reviewed_head_handoffistrue.release-holdas the way to clear it.The docs and prompts that say which script merges GitHub PRs now describe this opt-in split (
AGENTS.md,bin/fm-branch-prompt.sh,bin/fm-merge-local.sh, the skills,docs/).Brings in the fork's other
bin/changes:bin/fm-dispatch-resolve.shpins the resolver model, ties requests to a fixed snapshot of the brief, and writes dispatch receipts under a lock, skipping partly written lines.bin/fm-captain-hold.sh, newbin/fm-calendar-lib.sh).bin/fm-send.shrejects unknown flags and stray--keyarguments.bin/fm-herdr-session-cleanup.sh,bin/fm-startup-network.sh).AGENTS.mdinto new skills:task-intake,task-delivery,backlog-managementandpr-review-policy.Tests were added or extended for each of these areas.
Follow-up, not in this PR:
bin/fm-pr-risk.shfinding P1 #2 is left for a separate change.🤖 Generated with Claude Code
Risk Assessment
✅ Low: The change does what the intent asks: the reviewed-head handoff is required only when the policy opts in, a direct GitHub merge still refuses a PR with an unreleased ledger hold, the configured Opus 5.5 model and zero required independent reviews are applied consistently, inline comments from the collector are kept, and the agent-facing docs agree with the scripts; tests that run the scripts cover each path.
Testing
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 3 issues found → auto-fixed (2) ✅
bin/fm-pr-review-snapshot.sh:116- The intent requires keeping feedback written by the collector. The commit changed the sharedexternalfilter (bin/fm-pr-review-snapshot.sh:89-91) so that top-level comments (:108) and submitted reviews (:112) keep the collector's own comments when the collector is also the PR author. The inline-thread path at :116 still filters withselect((.author.login // "") != $author), so it drops every inline review comment from the collector whenever the collector wrote the PR. That is the fork's normal case: the maintainer both authors the PR and collects its reviews. Trace with the updated fixture in tests/fm-pr-review.test.sh:287-303 (actor=maintainer, PR author=maintainer, thread THREAD_12 has comments from sentry and maintainer): the thread keeps only the sentry comment, with author "sentry" and body "inline". The check at tests/fm-pr-review.test.sh:327 expects 2 maintainer items containing "final disposition evidence" but gets 1, sotest_live_collector_includes_submitted_reviews_and_inline_threadsfails after this commit. Test is skipped in this run, which is why nothing caught it. The header comment (:8-10) also now describes behaviour that the inline path does not have. Fix: at :116, use the same rule asexternal:select((.author.login // "") != $author or (.author.login // "") == $actor). The ledger's exact-post exclusion (bin/fm-pr-review.sh:204-207) already handles collector-authored entries.bin/fm-pr-merge.sh:384- Withrequire_reviewed_head_handoff: false(.github/firstmate-review-policy.json:3),bin/fm-pr-merge.shnow merges GitHub PRs directly and never reads the review ledger. Before this change, every GitHub merge went throughfm-pr-review.sh merge, which refuses on an unreleased ledgerhold(bin/fm-pr-review.sh:240), missing dispositions, or missing high-stakes attestations. Now, if a PR has a ledgerholdfor a product, rights, spend, or security gate and an agent callsbin/fm-pr-merge.sh <task> <url>, the merge goes through and the human gate is skipped. The only thing stopping this is the prose rule in AGENTS.md:128 and .agents/skills/pr-review-policy/SKILL.md:53 ("or a human review gate is active"). The intent does make the handoff opt-in, so this is not a contradiction. A narrower guard would close the gap: for GitHub, require the handoff (or refuse) whenever a ledger for this PR URL has an unreleased hold on its current generation. That adds a ledger read to fm-pr-merge.sh, which goes beyond the stated scope, so the remedy needs the user's approval.bin/fm-branch-prompt.sh:112- Several agent-facing instructions still sayfm-pr-review.sh mergeis the only or mandatory GitHub merge entrypoint. This contradicts the new policy-gated rule in AGENTS.md:128, pr-review-policy SKILL.md:53-54, docs/configuration.md:269 and docs/scripts.md:137. Sites: bin/fm-branch-prompt.sh:112 (the away-posture prompt: "the only GitHub merge entrypoint"), .agents/skills/task-delivery/SKILL.md:37 ("for every GitHub task PR merge"), .agents/skills/bearings/SKILL.md:147 ("merge GitHub only throughbin/fm-pr-review.sh merge"), and docs/architecture.md:388 ("GitHub task merges go throughbin/fm-pr-review.sh merge"). Following these is still safe because the wrapper still works, but agents now get conflicting rules. Reword them to match AGENTS.md:128.🔧 Fix applied.
2 issues (1 warning, 1 info) still open:
.agents/skills/pr-review-policy/SKILL.md:55- The sentence "It takes a fresh complete snapshot, starts a new generation if the head moved, refuses pending reviews, stale checks, missing dispositions, or missing high-stakes attestations, records the merge decision ... then hands the same URL to the guarded merge command" used to describebin/fm-pr-review.sh merge. Commit e6673c2 inserted abin/fm-pr-merge.shsentence just before it, and the round-1 fix round then extended that sentence (line 54). "It" now reads asbin/fm-pr-merge.sh, the route for the fork's defaultrequire_reviewed_head_handoff: false. Butfm-pr-merge.shtakes no review snapshot and runs none of those checks: its only new ledger check is the hold refusal at bin/fm-pr-merge.sh:404-419.ready_check(bin/fm-pr-review.sh:232-260) runs only underfm-pr-review.sh merge. Concrete path: an agent loads this skill for a high-stakes PR, reads that the direct merge refuses missing attestations, and relies on the script instead of checking readiness. The PR then merges without the configured no-mistakes attestation or a bound final disposition, and nothing reports it. docs/architecture.md:386 got the same fix in the round-1 fix round (it now namesbin/fm-pr-review.sh merge), but this skill sentence was left behind. Fix: start line 55 with "bin/fm-pr-review.sh mergetakes a fresh complete snapshot…" (or "The review-ledger merge…"), so the checks are attributed only to that entrypoint.docs/architecture.md:381- This says the tracked reviewer configuration fixes the "exact Fable 5.1 high-stakes model". Commit e6673c2 changed .github/firstmate-review-policy.json tono_mistakes_model: "claude-opus-5-5"andindependent_agent_reviews: 0, so the architecture doc now names the wrong required model. The intent explicitly requires the fork's configured Claude Opus 5.5 model. Reword it to refer to the configured high-stakes model (for example "the configured high-stakes no-mistakes model", as docs/configuration.md:268 does) instead of naming a model.🔧 Fix applied.
✅ Re-checked - no issues remain.
⏭️ **Test** - skipped
Step was skipped.
✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.