feat(bin): reconcile gated checks-green PRs into the Cipher pr-ready hook - #23
Merged
morris2spears merged 7 commits intoSep 1, 2026
Merged
Conversation
…hook A gated PR that reached checks-green only after registration - via rebase or sync, repair or recovery, or a manual coordinator reconciliation - never re-invoked the pr-ready trigger, so a healthy bridge could miss the transition entirely (iinvy-control-plane#65), and iinvy-control-plane was not gated at all. A second live case (iinvy#294) showed local run-step state can also under-report while the pipeline's CI monitor is wedged even though GitHub already reports the PR open and CLEAN. - gate morris2spears/iinvy-control-plane as a third Cipher repository and make every consumer handle an arbitrary gated set - add a watcher-cadence 'reconcile' sweep that re-registers every recorded gated checks-green PR through bin/fm-pr-check.sh, the one canonical trigger, refreshing the exact head and staying silent unless a new event is acknowledged - accept GitHub's own open-and-CLEAN answer as checks-green truth in the pr-ready preflight, the retry sweep, and the reconcile sweep, so a wedged local CI monitor cannot suppress the event - extract shared live-head and forge-green readers into fm-pr-lib.sh - regression coverage: behind/red -> sync -> green, duplicate reconciliation, head advance after acknowledgement, watcher recovery end to end, and the wedged-monitor forge-green case Closes #19 Claude-Session: https://claude.ai/code/session_01S9g25zaupxf13Rdx55HsLa
…arden reconcile sweep
morris2spears
force-pushed
the
fm/firstmate-issue19-checks-green-reconciliation
branch
from
August 31, 2026 23:36
abdd68d to
f8fbb2d
Compare
morris2spears
deleted the
fm/firstmate-issue19-checks-green-reconciliation
branch
September 1, 2026 00:36
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
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
The developer (acting as firstmate's captain/supervisor) wanted firstmate issue #19 fixed: every gated repository PR that reaches exact-head checks-green must reliably invoke the idempotent Cipher
pr-readyhook, not just at initialfm-pr-check.shregistration, so real transitions (like iinvy-control-plane#65) can no longer be missed and agent prose is never the only notification path. They explicitly scoped in expanding the gate to a third repository,morris2spears/iinvy-control-plane, and required all code, docs, and tests that assumed exactly two gated repos to handle an arbitrary N, while preserving exact-head binding, request dedupe, fail-closed holds, and regression coverage for behind/conflicted to sync/rebase to green plus watcher repair/recovery. They then insisted the change ship through the no-mistakes pipeline itself: drive every gate to completion rather than hand-fixing findings, escalate ask-user findings instead of answering them, and reportdone: PR <url> checks green. One explicit follow-up decision was to fix review-1 by clearing the$ID.pendingmarker on every early-exit path out of the sweep (held delivery, missing RID, no ack file, migration-deferred exit), not only on the announce success path. After PR #22 merged and left PR #23 conflicting, they directed reconciling PR #23 through the no-mistakes branch-custody/sync path preserving both changes, rerunning the full gate, and reporting the new exact head - with a standing constraint that firstmate must never merge or deploy, since Cipher and the captain own that.What Changed
bin/fm-cipher-hook.sh reconcilesweep, run by the watcher on the slow check cadence, that re-registers every recorded gated pull request now at exact-head checks-green throughbin/fm-pr-check.sh, so a green transition reached after registration (rebase, sync, repair, or manual coordinator run) still emits its idempotentpr-readyevent instead of relying on agent prose. The sweep spends at mostFM_CIPHER_RECONCILE_BUDGET(default 8) forge round trips per cadence and resumes at the next task viastate/.cipher-reconcile-cursor, clears its.pendingmarker on every early-exit path, and wakes Firstmate only when a new event is acknowledged.bin/fm-pr-lib.shso a wedged or stale local CI monitor cannot suppress an event; the forge query is deliberately strict (open, CLEAN, and a rollup carrying at least one passed check with nothing running or unsuccessful), since GitHub reports CLEAN for a PR with no checks at all.bin/fm-pr-check.shnow preserves an already recordedpr_headfor the same PR when the forge cannot answer, rather than erasing it.morris2spears/iinvy-control-planeto the gated set and reworks the code, docs, and tests that assumed exactly two gated repositories to handle an arbitrary N; extendstests/fm-cipher-hook.test.shto 21 cases covering post-registration green delivery, hold cleanup, watcher reconciliation, forge-green override and rollup classification, the reconcile budget rotation, and recorded-head survival across a silent forge.Risk Assessment
✅ Low: All findings from both prior rounds are now closed at their shared boundaries with source-traced verification, the remaining delta is one narrowing condition plus documentation and tests, and every failure direction in the change stays fail-closed (no path can produce a false checks-green or an incorrect merge head).
Testing
I exercised the change at the level Cipher actually sees it. Beyond the targeted automated suites (the full Cipher-hook bridge suite and the two documentation-contract suites, all green), I drove a real CLI transcript through a synthetic authenticated Hermes gateway: amorris2spears/iinvy-control-planePR registers while CI is still running and spends no event, a reconcile sweep stays silent while it is not green, and once a coordinator sync advances the head and checks pass, the sweep printsdelivered <request-id> iinvy-pr-ready <task>, refreshespr_headin task metadata, and the gateway receives exactly one HMAC-V2-validiinvy-pr-readypayload bound to the new exact head. A repeat sweep is silent with no second delivery, a wedged local CI monitor still delivers on GitHub's own open+CLEAN-with-a-passed-check answer, and a PR that is CLEAN but has no passed check delivers nothing (fail-closed). The third gated repository is recognized alongside the other two while a non-gated repo is refused, confirming the arbitrary-N list. This is a shell/CLI and webhook surface with no rendered UI, so the reviewer-visible evidence is the command transcript and the received gateway payload rather than screenshots. No findings; the worktree is clean and no source or test files were modified.Evidence: End-to-end reconcile transcript (gated control-plane PR: red -> synced green -> delivered once)
=== 0. gated repositories known to the hook (arbitrary N, third repo added) === morris2spears/iinvy morris2spears/iinvy-storefront morris2spears/iinvy-control-plane repo-gated morris2spears/iinvy gated repo-gated morris2spears/iinvy-storefront gated repo-gated morris2spears/iinvy-control-plane gated repo-gated example/project NOT gated === 1. registration while CI is still running (head A) === armed: state/cp-task.check.sh recorded pr_head : aaaaaaaa... cipher events : none - nothing spent === 2. reconcile sweep while still not green === sweep output : <silent> gateway requests : 0 === 3. coordinator syncs/rebases; checks turn green AFTER registration (head B) === sweep output : delivered fmch-v1-0a6cfc57... iinvy-pr-ready cp-task refreshed pr_head: bbbbbbbb... { "v2_valid": true, "event": "iinvy-pr-ready", "repo": "https://github.com/morris2spears/iinvy-control-plane/pull/19", "head": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", "task": "cp-task" } === 4. a second sweep on the same green head === sweep output : <silent> gateway requests : 1 (still exactly one) === 5a. GitHub says CLEAN but NO check has passed (fail-closed) === sweep output : <silent> gateway requests : 0 (mergeable is not verified) === 5b. once a real check has passed, local monitor still wedged === sweep output : delivered fmch-v1-978c15e5... iinvy-pr-ready wedged-taskEvidence: Exact payload received by the Cipher/Hermes gateway
{"request_id":"fmch-v1-0a6cfc57ce6c70ef1a3d3c3aa9fdcdb7ba0e679df084235f52fc904c396ace20","v2_valid":true,"e":"iinvy-pr-ready","pr":"https://github.com/morris2spears/iinvy-control-plane/pull/19","head":"bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"}Evidence: Targeted Cipher-hook suite output
ok - reconcile delivers a post-registration checks-green transition once with the fresh exact head ok - a held reconcile delivery owes no announcement and is never re-announced after retry-held ok - normal supervision reconciles a post-registration checks-green transition and wakes firstmate once ok - GitHub-side green truth delivers the PR-ready event despite a wedged local CI monitor ok - a budgeted reconcile sweep reaches every gated task in turn across cadences ok - a silent forge preserves the recorded exact head only for the same pull request ok - the forge snapshot query calls only a genuinely passed check rollup green ok - only iinvy repositories emit at checks-green and every route failure holds themEvidence: Evidence demo script and harness (reuses the suite's own fixtures)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 5 issues found → auto-fixed (2) ✅
bin/fm-pr-lib.sh:289- fm_pr_github_live_head still requires an existing worktree ([ -n "$wt" ] && [ -d "$wt" ]), while the sibling fm_pr_github_snapshot added in the same commit falls back to a worktree-lessgh pr view <url>. fm-pr-check.sh:74 uses the worktree-bound one, and the meta rewrite at fm-pr-check.sh:92-98 strips bothpr=andpr_head=and only re-addspr_headwhen PR_HEAD is non-empty. Failure sequence: teardown removes a gated ship worktree, the watcher's reconcile sweep runs before meta is removed; fm_pr_github_snapshot (no worktree needed) returns a live head, so LIVE_HEAD != RECORDED_HEAD is false only if they match - either way the sweep reaches fm-cipher-hook.sh:341 and re-registers. fm-pr-check.sh then resolves no head, erasespr_head=from meta, andrun_python request-idraises missing-pr-head so RID is empty. On every subsequent cadence RECORDED_HEAD is empty and LIVE_HEAD is set, so the head-mismatch branch re-registers again forever, one gh call plus a full meta rewrite and poll republish per cadence, and the PR-ready event can never be delivered. A transient gh failure reproduces the same pr_head erasure with self-healing on a later cadence. Fix at the shared boundary: have fm_pr_github_live_head accept a missing worktree the way fm_pr_github_snapshot does, and/or have fm-pr-check.sh preserve the existing recorded pr_head when the live head cannot be resolved rather than dropping it.bin/fm-cipher-hook.sh:312- The reconcile sweep performs one unconditionalgh pr viewround trip per gated recorded PR (plus a crew-state subprocess and a pythonrequest-idcall) for every task in"$STATE"/*.meta, with no per-cadence bound, and fm-watch.sh:826 runs it under run_check_process's 30s CHECK_TIMEOUT (fm-watch.sh:105). The glob is deterministically sorted, so once the accumulated per-task cost exceeds 30s the sweep is killed at the same point every cadence and the alphabetically-later gated tasks are never reached - the exact class of missed checks-green transition this change exists to fix. An acknowledged PR awaiting Cipher's merge also keeps paying its round trip on every cadence indefinitely (fm-cipher-hook.sh:327-333 only short-circuits after the snapshot). Bound the work per cadence with a rotating start cursor (or skip the snapshot for identities already acked and announced unless the recorded head is stale) so every gated task is reached across cadences.bin/fm-pr-lib.sh:364- FM_PR_GITHUB_SNAPSHOT_QUERY is the predicate that decides whether an unverified PR may emit a checks-green merge-boundary event, and it is never evaluated by any test. The gh fake at tests/fm-cipher-hook.test.sh:147-154 ignores the-qargument entirely and prints a canned<state> <mergeStateStatus> <head> <passed>line, so the jq program's central safety property - that GitHub's CLEAN answer for a PR with an empty or still-running statusCheckRollup is NOT green - is asserted nowhere. A regression in the$passed | any/$settled | alllogic would let a PR whose CI never ran deliver an iinvy-pr-ready event, and every existing test would still pass. Add a direct test that pipes representative statusCheckRollup fixtures (empty rollup, QUEUED check, FAILURE check, all-SKIPPED, one SUCCESS) through the real query with jq and asserts the verdict column.bin/fm-pr-lib.sh:373- The query defaults headRefOid to "-" to keep the joined columns aligned, but defaults.stateand.mergeStateStatusto "". If either is empty the joined line has an empty field andread -r state merge head greencollapses it under IFS word splitting, shifting every later column left - the head SHA lands inmergeand the green verdict lands inhead. The result fails safe (FM_PR_GITHUB_GREEN stays 0 becausestatewould then hold a mergeStateStatus value, never OPEN, and the head fails fm_pr_head_valid), but the head is silently reported unknown for a reason the caller cannot distinguish from a forge error. Use the same "-" placeholder for the state and mergeStateStatus fields.bin/fm-cipher-hook.sh:251- state/cipher-hooks/announced/ accumulates one marker per request id (i.e. per gated PR head) and one per interrupted task, and nothing ever prunes it - not teardown, not the python supersede path, which cleans holds and diagnostics (bin/fm-cipher-hook.py:631) but knows nothing about this directory. Every rebase of a gated PR leaves another permanently orphaned marker. It is also the one cipher-hooks subdirectory created by the shell rather than by record_dirs() in bin/fm-cipher-hook.py:388, which otherwise owns that layout. Prune markers whose acks/<rid>.json is gone, or move the directory under record_dirs() so the existing record lifecycle covers it.🔧 Fix: bound reconcile sweep, keep pr_head, test forge-green query
4 issues (1 warning, 3 infos) still open:
bin/fm-cipher-hook.sh:412- The rotating cursor and per-cadence budget added at fm-cipher-hook.sh:390-419 are the mechanism that guarantees every gated task is eventually reached, and no test exercises them. No test sets FM_CIPHER_RECONCILE_BUDGET and no fixture builds more than one gated task, so TOTAL is always 1 and theTAKEN < TOTALbound short-circuits before BUDGET, the modular(START + TAKEN) % TOTALwrap, the cursor lookup loop, and the cursor-not-found reset are ever evaluated. An off-by-one in START (e.g.START=$INDEXinstead ofINDEX + 1) would silently reintroduce the exact alphabetical-head starvation this commit exists to close, and every current test would still pass. The same commit's other behavioral fix - preserving an already recordedpr_headwhen the forge cannot answer (bin/fm-pr-check.sh:81-84) - is likewise uncovered; tests/fm-pr-check-security.test.sh:593 only proves the fresh-task case where no prior head exists. Add a case with FM_CIPHER_RECONCILE_BUDGET=1 and two or three gated tasks asserting that successive sweeps reach each one, and a case that re-registers a task with the gh fake failing and asserts the recorded pr_head survives.bin/fm-cipher-hook.sh:315- prune_markers claims to stop the announced directory growing "one permanent entry per gated head" (comment at fm-cipher-hook.sh:299-303), but its request-marker branch prunes only markers whoseacks/<rid>.jsonis absent, and nothing in the codebase ever deletes an ack: bin/fm-cipher-hook.py:630-635 unlinks only holds and diagnostics, and no teardown or watcher path touches state/cipher-hooks at all (bin/fm-watch.sh:812 only globs holds). Since a request marker is written exclusively while its ack exists, the*)branch is unreachable in normal operation and every rebased head still leaves a permanent marker - now mirrored by a permanent ack. The.pendingbranch does work. Either scope the prune to the task lifecycle (drop request markers whose task metadata is gone, the way the pending branch does) or correct the comment to say the announced set is intentionally bounded by the durable ack set rather than pruned.bin/fm-pr-check.sh:82- The head-preservation fallback readspr_head=out of the metadata without checking that the metadata's recordedpr=is the same pull request being registered. If a task's PR URL is replaced (fm-pr-check.sh accepts any well-formed URL for an existing task) during a window where gh cannot answer, the newpr=is written alongside the previous pull request's head, so the metadata asserts an exact head that never belonged to that PR. The consequences are contained -identity_for_eventwould derive a request id over the wrong pair andgh pr merge --match-head-commit(bin/fm-pr-merge.sh:121) refuses at the forge, and the next reconcile sweep sees LIVE_HEAD != RECORDED_HEAD and re-registers - but the fix is one condition: only fall back whengrep -qxF "pr=$URL" "$META"holds.bin/fm-cipher-hook.sh:395-state/.cipher-reconcile-cursoris a new durable state artifact, but AGENTS.md's state inventory (AGENTS.md:95-125) documents every other entry down to private single-purpose markers like.decision-nudge-pendingand.pr-check-migration-scan-v1, and the catch-all watcher-internals line covers only.hash-* .count-* .stale-* .stale-since-* .paused-* .wedge-escalations-* .seen-* .hb-surfaced-* .last-* .heartbeat-streak- none of which match this name. docs/configuration.md:93 describes the budget and resume behavior but not the file. Add one line to the AGENTS.md state inventory so the artifact has a documented owner and a "never touch" classification like its peers.🔧 Fix: test reconcile budget and head preservation, bind fallback
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-cipher-hook.test.sh- full targeted suite for the changed bridge (21 cases, includestest_reconcile_delivers_post_registration_green,test_reconcile_hold_leaves_no_stale_announcement,test_watcher_reconciles_post_registration_green,test_forge_green_overrides_wedged_local_monitor,test_forge_green_query_classifies_check_rollup,test_reconcile_budget_reaches_every_gated_task_in_turn,test_recorded_head_survives_a_silent_forge)bash tests/fm-instruction-owners.test.sh- doc/instruction ownership contract for the edited AGENTS.md and cipher-hook SKILL.mdbash tests/fm-documentation-audiences.test.sh- documentation audience/link contract for the edited docs/configuration.md, docs/architecture.md, docs/scripts.mdManual end-to-end transcript against a synthetic authenticated Cipher/Hermes gateway (evidencedemo.sh, reusing the suite's own harness):fm-cipher-hook.sh repo-gatedfor all three gated repos plus a non-gated control;fm-pr-check.shregistration of amorris2spears/iinvy-control-planePR while CI is red (no event spent);fm-cipher-hook.sh reconcilewhile red (silent); a coordinator sync advancing the head to a green state andreconciledelivering one exact-headiinvy-pr-ready; a repeat sweep proving idempotence; a wedged-local-monitor case where GitHub's own open+CLEAN+passed-check answer delivers; and a CLEAN-but-no-passed-check case proving fail-closed✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.