Skip to content

feat: strengthen supervision lifecycle controls - #1631

Closed
coreldh wants to merge 14 commits into
kunchenguid:mainfrom
coreldh:fm/c0803-o2-fold
Closed

coreldh wants to merge 14 commits into
kunchenguid:mainfrom
coreldh:fm/c0803-o2-fold

Conversation

@coreldh

@coreldh coreldh commented Aug 4, 2026 •

Copy link
Copy Markdown
Contributor

Intent

Fold two previously-split branches back into one self-contained PR, per an explicit ruling from the repository owner, and remove a contract change he rejected.

BACKGROUND: An earlier PR (#1493) bundled a large supervision/record-integrity change with a hardening of the watcher arm guard. A prior lane rebased it onto current main (8 conflicts) and split it into two branches: PR1 (the bulk plus five gate fixes) and PR2 (the arm-guard hardening). The owner then ruled on both.

RULING 1 - SECURITY, fold the hardening IN. His words: 'the watcher-arm bypass must never exist in a merged state. Not the split-and-bet-on-order path.' The bulk commit INTRODUCES the bypass: it added an inferred 'noexec' flag so that legitimate 'bash -n bin/fm-watch.sh' syntax checks could be allowed, but the inference is unsafe because bash options like --rcfile and --init-file consume a value. I verified on main that no such inference exists, and verified on the bulk-only branch that all of these reach the ALLOW path: 'bash --rcfile -n bin/fm-watch.sh' and 'bash --init-file -n bin/fm-watch.sh' (which EXECUTE the watcher, since -n is swallowed as the rcfile argument), and 'bash -n bin/fm-watch.sh > bin/fm-watch.sh' (which TRUNCATES it). So shipping the bulk without the hardening would land a real regression. The hardening replaces the inference with an exact positive allowlist for the only two sole-syntax-check forms, and rejects protected output redirection independently and before every allow path. I re-verified against the folded tree that all attack forms now deny and both legitimate syntax-check forms still allow. This is deliberate and is the whole point of the PR being self-contained.

RULING 2 - CONTRACT, the 'Done' redefinition is REJECTED. His words: 'Done = MERGED, never green-open. Unlanded work does not release dependents.' The bulk added bin/fm-record-reconcile.sh, which transitioned a backlog row from In flight to Done as soon as a worker reported complete with a clean tree - i.e. on a green-but-unmerged PR - which released dependents against work that had not landed. I removed that transition. The smallest compliant alternative the gate itself named, and what I implemented, is to keep the row In flight and record terminal-retention evidence (exact head, clean tree) on it, leaving teardown - which separately refuses unlanded work - as the only path that retires it. The write is idempotent so repeated terminal wakes do not churn the row body. A reviewer seeing a 'tasks-axi done' call deleted should read it as this deliberate rejection, not an oversight.

DELIBERATE MERGE DECISIONS worth knowing: (a) The two lanes both touched the terminal-wake reconcile path. PR1 had built a helper that detected terminal wakes by PARSING ANNOTATION PROSE, then patched it again to survive truncated annotations; PR2 replaced that approach entirely with structural status-key mapping. I resolved in favor of the structural approach and dropped the prose parser, because it supersedes both gate fixes rather than competing with them. (b) That resolution silently broke two test seams which I deliberately restored: the drain now calls the reconcile script through FM_RECORD_RECONCILE_BIN again so the existing 'terminal signals reconcile records even when annotations truncate' test can still stub it, and the test file re-defines RECONCILE (which PR2 had removed while PR1 still used it, an unbound-variable failure under set -u). (c) Reconciliation refuses any row whose worker has resumed - a real safety property PR1 added - so the drain-driven test needs a current-state source; I export FM_CREW_STATE_BIN so it survives into the reconcile child.

VERIFICATION: full suite is 1034 passing with 3 failures, and I confirmed all 3 reproduce unmodified on main (Orca spawn metadata, fm-send --key exit code, teardown tasktmp) - none are from this change. Lint and the doc-audience gate are clean. Do not fix those 3 pre-existing failures here.

Final shape: 44 files, +2909/-104.

What Changed

  • Reconcile terminal wakes from structural status keys, custody-version drained queues and replaced reports, and retain exact-head/clean-tree evidence while completed but unmerged rows remain In flight.
  • Add retained awaiting-captain supervision, identity-bound process-progress sampling, serialized model-capacity holds, receipt-bound decision resolution, and strict spec-forging brief preflights.
  • Harden watcher and steering paths with exact syntax-check allowlisting, protected-redirection denial, verified arm ownership, and affirmative empty-composer checks where backend state is inspectable.

Risk Assessment

⚠️ Medium: Captain, no material source defect remains, but the broad supervision and concurrency changes plus the explicitly retained PID-reuse containment leave moderate residual risk.

Testing

The supplied broad-suite baseline and its three main-reproducing failures were not rerun; focused security, reconciliation, and truncated-notification suites passed, while manual CLI evidence demonstrated real guard decisions, unchanged branch HEAD, retained metadata, no Done transition, and idempotent repeated reconciliation. No standalone lint phase ran, although the watcher test script includes its own final shellcheck assertion.

Evidence: Watcher guard allow/deny transcript
CASE: exact short syntax check is allowed
COMMAND: bash -n bin/fm-watch.sh
EXIT: 0
RESPONSE: <silent allow>

CASE: exact long syntax check is allowed
COMMAND: bash --noexec bin/fm-watch.sh
EXIT: 0
RESPONSE: <silent allow>

CASE: --rcfile consumes -n and is denied
COMMAND: bash --rcfile -n bin/fm-watch.sh
EXIT: 2
RESPONSE: {"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"deny"},"systemMessage":"[watcher-nested] a protected watcher command must not run through a wrapper, substitution, or compound command"}

CASE: --init-file consumes -n and is denied
COMMAND: bash --init-file -n bin/fm-watch.sh
EXIT: 2
RESPONSE: {"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"deny"},"systemMessage":"[watcher-nested] a protected watcher command must not run through a wrapper, substitution, or compound command"}

CASE: protected output redirection is denied
COMMAND: bash -n bin/fm-watch.sh > bin/fm-watch.sh
EXIT: 2
RESPONSE: {"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"deny"},"systemMessage":"[watcher-redirection] a protected watcher command must not use shell redirection"}
Evidence: Terminal retention and idempotency transcript
WAKE-DRAIN OUTPUT
1785807813	1	signal	sample-terminal.status	signal: sample-terminal.status
record reconciliation: /var/folders/cq/xf4qcb9j0qzc2dbh173mflbm0000gn/T/no-mistakes-evidence/01KZ53QC8FMRR1QTJWKEWVSA7R/reconcile-home/data/record-reconciliation/reconcile.20260804T014334Z.3437.0.receipt
wake annotation: latest wake-EVENT observed at drain, not current state: sample-terminal.status: done: producer complete at 21b01e86d4fbe2b3996f0ab32db0e15d2c55fab5

PERSISTED RESULT
head before: 21b01e86d4fbe2b3996f0ab32db0e15d2c55fab5
head after:  21b01e86d4fbe2b3996f0ab32db0e15d2c55fab5
metadata retained: yes
row still under In flight: yes
row occurrences under Done: 0
terminal-retention marker count after two passes: 1
backlog sha256 after first wake:  b3c230476c30f011fb4b3eb12d8b4921030b5c628daa1ee42037ecf480b518e2
backlog sha256 after second pass: b3c230476c30f011fb4b3eb12d8b4921030b5c628daa1ee42037ecf480b518e2
idempotent body: yes

FINAL BACKLOG
## In flight
- [ ] sample-terminal - Completed producer awaiting merge (repo: sample) (kind: ship) (since 2026-08-03)
  terminal-retained: head=21b01e86d4fbe2b3996f0ab32db0e15d2c55fab5 tree=clean; producer reported complete and its worktree is clean. Endpoint metadata and worktree are retained, and this row stays In flight - releasing no dependent - until teardown confirms the work landed.
## Queued

## Done

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-crew-state.sh:632 - The repository contract says running, fixing, and CI states remain working, and this function's comment says an absent worker identity leaves that mapping unchanged. The catch-all instead changes every PID-unbound full-source working run to unknown. Thus a legitimate CI step without a native model PID—or a transiently unreadable step log—is first mapped to working and then overwritten; the test fake masks this by injecting codex started pid=4242 into every log response. Confirm whether all PID-unbound runs should now be indeterminate; otherwise preserve the authoritative run-step state unless a bound worker is positively contradicted.
  • 🚨 bin/fm-crew-state.sh:373 - The worker check does not actually detect PID reuse by another allowed-family process. If the recorded worker dies without an exit marker and its PID is reused by an unrelated Codex, Claude, or other matching agent process, the broad command regex reports the stale run as live. Bind the PID to a process-birth identity at the launcher/logging boundary and compare that exact tuple here.
  • 🚨 bin/fm-model-capacity-hold.sh:97 - Registration publishes an exclusive per-ID receipt but overwrites the shared active marker without serialization. Two different hold IDs can both observe no marker, both create receipts, both return success, and the last mv silently discards the first active reservation; releasing the winner then opens capacity despite the first successful registration. An interruption after receipt publication also makes retry fail permanently. Serialize the home-wide lifecycle and make a matching orphan receipt repairable before reporting success.

🔧 Fix: Fix worker identity and capacity hold races
3 errors still open:

  • 🚨 bin/fm-crew-state.sh:357 - The required fix was to “bind the recorded worker PID to a process-birth identity at the launcher/logging boundary and compare that exact tuple,” but this patch only consumes a new pid-identity-hex token. No production source emits that token; it appears only in this parser and its test helper. The installed no-mistakes v1.41.2 logs confirm the gap: 261 step logs contain plain started pid=... records and zero contain the new identity token, including this review round. Consequently every real worker remains unbound and a dead or reused PID still inherits working. The durable fix requires the no-mistakes launcher to emit the exact identity and a compatible version floor; otherwise retain the authoritative run-step behavior as explicitly documented containment rather than claiming PID-reuse closure.
  • 🚨 bin/fm-wake-lib.sh:13 - Extracting fm_pid_identity makes fm-wake-lib.sh depend on a new sibling, but several supported isolated-copy paths still copy only fm-wake-lib.sh—for example the Claude auto-arm, turn-end guard, session-lock, AFK-return, and historical-backend fixtures. Those copies now source a missing file and lose the shared identity functions, introducing failures beyond the three explicitly allowed pre-existing failures. Update every bundle/copy manifest to carry the new helper, or keep this dependency self-contained.
  • 🚨 bin/fm-model-capacity-hold.sh:119 - The serialized registration path still treats an active marker with the same ID as a successful retry without comparing reason or dispatch_ref. A second caller reusing the slug with different reservation metadata receives success and can later release that ID, retiring the first caller's reservation. Apply the existing registration_matches check to the active marker and its receipt before returning already active.

🔧 Fix: Document PID containment and secure hold retries
✅ Re-checked - no issues remain.

✅ **Test** - passed

✅ No issues found.

  • bash tests/fm-arm-pretool-check.test.sh
  • bash tests/fm-record-reconcile.test.sh
  • bash tests/fm-wake-queue.test.sh
  • /var/folders/cq/xf4qcb9j0qzc2dbh173mflbm0000gn/T/no-mistakes-evidence/01KZ53QC8FMRR1QTJWKEWVSA7R/reproduce-intent-evidence.sh "$PWD"
  • Compared terminal-retention-after-first-wake.md with terminal-retention-after-second-pass.md and verified identical content.
  • Verified git status --short remained empty after testing.
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

Rebuild update - 2026-08-04

BLOCKER-1 is closed in the rebuilt head. Composer preflight capability is static and adapter-owned: tmux, herdr, Orca, and cmux are classified and accept only classified:empty; zellij alone is statically unclassified and falls back to its existing submit-verification policy; unknown classifier capability and every non-empty or unknown classified verdict refuse before mutation.

The independently gated production repair is unchanged by the rebase. Commit 966e211e and its rebased twin aa4103a have the same subject (fix: preserve zellij composer fallback) and the same stable patch ID, 3eea69560f217c8baa543f4aff36a4c24f27bda6.

The three authorized non-blocking follow-ups are closed:

  • tests/fm-send-strict.test.sh now exercises the fm-send.sh capability-unknown refusal arm through the executable path and kills mutant C.
  • docs/architecture.md now documents the statically unclassified fallback and fail-closed unknown states.
  • docs/scripts.md now describes fm-composer-lib.sh as the classifier used by statically classified backends.

fm-watch-triage status: this BLOCKER-1 repair and its follow-ups do not modify bin/fm-watch-triage.sh or tests/fm-watch-triage.test.sh. Any changes to that area visible elsewhere in this PR are inherited from earlier branch commits; their disposition is a separate decision, and this repair takes no position on them.

Validation evidence:

  • Independent gate passed the exact production repair at 966e211e, including 28 crafted classifier outputs, fail-closed classified and unclassified unknown cases, and zellij red-before-green.
  • No-mistakes run 01KZ81N8QN97X17NBBEVM2B6QD passed intent, rebase, review, focused behavioral tests, documentation, and full repository lint with no findings. Its testing independently passed tests/fm-send-strict.test.sh and tests/fm-backend-zellij.test.sh and killed mutant C.
  • The repair full-suite run reported total=125, failed=7, and skipped_gate=25; six failures reproduced on main and the seventh reproduced on the pre-repair feature-branch control.
  • The branch also contains upstream main advance bea3d23 for upstream PR fix(bin): resolve remote entrypoint path through symlinks #1709 in fm-remote-entrypoint.sh; no gate-bound BLOCKER-1 production byte moved.

Published head: 0fddbae64cb4fbd8b0a0b3dbe276e4ff5a3a9c87.

coreldh added 14 commits August 4, 2026 22:29
Done means merged, never green-open. Reconciliation previously transitioned a
terminal producer row from In flight to Done as soon as the worker reported
complete with a clean tree, which released dependents against work that had not
landed.

Record the terminal-retention evidence (exact head, clean tree) on the row
instead and leave it In flight. Teardown, which separately refuses unlanded
work, stays the only path that retires it. The write is idempotent so repeated
terminal wakes do not churn the row body.

Also restores the reconcile-binary and current-state seams the two lanes' tests
depend on, so the resumed-worker refusal is still exercised through the drain
path.
@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate: closing this as stale. It has been waiting on a contributor update for 14+ days with no author push or comment. Reopen if you want to pick it back up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants