fix(bin): preserve Codex supervision continuity - #2776
Open
mcordovaforus wants to merge 8 commits into
Open
Conversation
Confidence Score: 5/5The PR appears safe to merge from this review because no additional unacknowledged actionable defect was established. The changed Stop-hook path keeps each checkpoint below the hook timeout, preserves durable wake recovery, and applies ownership checks during timeout cleanup; the remaining duration-input defects are already explicitly recorded by the PR. Reviews (1): Last reviewed commit: "no-mistakes(document): Document Stop-own..." | Re-trigger Greptile |
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 wanted the committed change fully validated and delivered through the no-mistakes pipeline, including review, tests, documentation, lint, push, pull request creation, and CI. Direct push access to the upstream repository was unavailable, so delivery needed to proceed through the developer's authorized GitHub fork while preserving the already validated head.
What Changed
Risk Assessment
Testing
Confirmed the target checkout was clean, exercised the focused checkpoint and turn-end guard behavior suites—including the absorbed-working then terminal-blocked recovery path—and captured a live Codex CLI transcript showing the real foreground checkpoint result without using the background arm path.
Evidence: Live Codex foreground checkpoint transcript
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-turnend-guard.sh:182- The new 540-second cap exists only in the Stop-hook path. The documented first cycle invokesfm-watch-checkpoint.shdirectly withFM_CODEX_WATCH_CHECKPOINT, whose parser accepts values above 540 unchanged; e.g.600blocks Codex for 600 seconds before any Stop successor is reached. Normalize and cap the value infm-watch-checkpoint.shso initial and Stop-owned checkpoints share the promised bound.🔧 Fix: Centralize Codex checkpoint duration bounds
2 warnings still open:
bin/fm-watch-checkpoint.sh:47- The normalizer strips leading zeroes after its zero check.FM_CODEX_WATCH_CHECKPOINT=000therefore reachestimeout 0; Codex immediately receives another Stop continuation rather than a watcher checkpoint, creating an unbounded loop. Validate zero after stripping (or strip before validation) and use the 180-second default.bin/fm-watch-checkpoint.sh:45- The generic checkpoint CLI now silently converts an explicit invalid--secondsvalue to a 180-second wait, whereas it previously failed with exit 2. This changes the failure contract for non-Codex callers despite the requested non-Codex compatibility. Keep explicit invalid arguments fail-fast, and apply the Codex environment fallback only on the Codex-owned path.🔧 **Test** - 1 issue found → auto-fixed ✅
bin/fm-watch-checkpoint.sh:52- On this macOS host,fm-watch-checkpoint.shtakes its Perl fallback (notimeout/gtimeout). A quiet 1-second checkpoint exits 124 but retains.watch.lockwith a dead PID, failing the pre-existing cleanup invariant before the complete checkpoint suite can reach its new cases. The fallback is unchanged from the base commit, but the target now relies on it at each Codex Stop; make the fallback wait for watcher cleanup or otherwise release its owned stale lock, then rerun the focused checkpoint suite.bin/fm-test-run.sh tests/fm-watch-checkpoint.test.sh tests/fm-turnend-guard.test.shbin/fm-test-run.sh tests/fm-turnend-guard.test.shExact existing target-added checkpoint cases: duration cap and absorbed-working:→ laterblocked:→ Stop-hook recovery → durable wake drainExact existing Codex guard/hook cases: away-mode loop guard, 540-second cap, process-root hook execution, nested-root exclusionManual Perl-fallback reproduction of a quiet checkpoint and dead retained watcher-lock PIDCompared the failing checkpoint fallback and original quiet-cleanup test against basedc0172c48b36a6303501af25d0915ad6e7d04192🔧 Fix: Checkpoint cleanup trap race fixed
✅ Re-checked - no issues remain.
git status --short && git rev-parse HEADbash tests/fm-watch-checkpoint.test.shbash tests/fm-turnend-guard.test.shFM_HOME=<evidence>/fmhome FM_ROOT_OVERRIDE=$PWD codex exec … 'Run exactly bin/fm-watch-checkpoint.sh --seconds 1 …'🔧 **Document** - 1 issue found → auto-fixed ✅
bin/fm-supervision-instructions.sh:167- Generated Codex session-start guidance still tells ordinary wakes to run the next checkpoint manually, contradicting the new Stop-owned successor contract; update this rendered line in the implementation phase.🔧 Fix: Document Stop-owned Codex checkpoint continuity
✅ Re-checked - no issues remain.
🔧 Fix: Provisioned pinned actionlint and passed lint
1 warning still open:
✅ **Push** - passed
✅ No issues found.