Conversation
coreldh
force-pushed
the
fm/c0815-fm-codex-appserver-client
branch
from
August 16, 2026 07:07
e0f7bac to
50d8bd3
Compare
Contributor
Author
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
Make Firstmate the owning client of codex app-server so Codex worker turn state comes from the protocol rather than pane guessing. Own one foreground child process group with bidirectional protocol pipes and bounded stderr; drive initialize, thread, and turn lifecycle; require terminal protocol status and child process result to agree before success; use an explicit deadline as the only hang detector; signal only the exact owned process group; publish concrete fail-closed evidence through the single busy-state resolver; and clean up through teardown. Incorporate the independent adversarial gate finding bound to e0f7bac: observe the child exit/reap event, never signal a reaped or recycled numeric process-group id, and resolve without waiting for inherited pipe closure. Add a regression that safely audits and fails on any signal attempt after reap. Also make the version gate inspect FM_CODEX_BIN, call Codex's identifier a thread id rather than session id, and raise the suite default deadline above one second. Preserve maintainer merge authority, never broadly kill processes, and do not add or drive Herdr lifecycle behavior.
What Changed
Risk Assessment
Testing
Verified the exact target checkout, portable protocol/process lifecycle and post-reap signal audit, busy-state/version/wiring/control integrations, and real Codex 0.147.0 success, failed-terminal, interrupt, and deadline/reap paths; a manual turn produced matching streamed output, terminal receipt, clean child exit, thread terminology, and idle resolver state. The directly relevant teardown cleanup passed, while one unrelated pre-existing Herdr fixture case failed for the isolated test-harness reason reported above.
Evidence: Live app-server E2E guard
Real Codex 0.147.0 success, failed-terminal, interrupt, and timeout/reap controls.Evidence: Manual real-Codex success transcript
APPSERVER_PROTOCOL_EVIDENCE; Codex turn success: turn-completed; Codex thread id printed.Evidence: Persisted joint protocol/process receipt
outcome=success, terminal=completed, child_exit=0, child_signal=none, with concrete thread and turn ids.Evidence: Persisted busy-state resolver result
state=idle source=codex-appserver event=turn-completed deadline=nonePipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed (3) ✅
bin/fm-codex-appserver-client.mjs:627- The required criterion says “use an explicit deadline as the only hang detector,” but this adds an independent 30-secondstartupTimerthat terminates a child stalled during initialize/thread setup, while the explicit turn deadline is not created until after that setup. Captain, decide whether this separate startup timeout is authorized; otherwise establish the authoritative absolute deadline when the child is spawned and use its remaining budget across initialize, thread, and turn lifecycle.🔧 Fix: Honor captain-authorized bounded startup timeout
1 error still open:
bin/fm-codex-appserver-client.mjs:441-turnIdis set by an unsolicitedturn/startednotification, so it is not proof thatturn/startreturned successfully. If that notification arrives and the response is withheld, this guard disables the startup timeout while no turn deadline has been installed, leaving the client hung indefinitely; if terminal notifications and a clean exit follow, the current success checks can also accept the turn without the required successful handshake. Track an explicit startup-complete state only after validating theturn/startresponse, gate busy publication/success on it, and extend the startup regression with an early notification plus withheld response.🔧 Fix: Require validated handshake before turn activation
1 error still open:
bin/fm-codex-appserver-client.mjs:444-startupTimeout()unconditionally replaces an earlier concrete failure. If malformed protocol input triggersprotocolFailure()shortly before the startup deadline and the child remains alive through the grace period, this timer changesprotocol-invalid-jsonintostartup-timeout, so the receipt no longer reports the actual give-up path. Preserve the first recorded failure while still running startup escalation.🔧 Fix: Preserve initial startup failure through timeout
✅ Re-checked - no issues remain.
tests/fm-teardown.test.sh:1565- The broader teardown script has a pre-existing unrelated fixture defect: its missing-adapter case executes a copied teardown while setting FM_ROOT_OVERRIDE to the original repository, making the supposedly removed Herdr adapter available. The target only changes Codex receipt removal in teardown, and that directly relevant case passed. This unrelated Herdr test was left unchanged to preserve the explicit no-Herdr scope.git rev-parse HEAD,git status --porcelain=v1, and target diff inspectionbash tests/fm-codex-appserver-client.test.shbash tests/fm-busy-state.test.shbash tests/fm-busy-adapter-wiring.test.shbash tests/fm-control.test.shbash tests/fm-teardown.test.shFM_CODEX_LIVENESS_LIVE_E2E=1 bash tests/fm-codex-liveness-live-e2e.test.shusing installedcodex-cli 0.147.0Manual real-Codex one-shot invocation ofbin/fm-codex-appserver-client.mjs, followed by inspection of its streamed output, terminal receipt, busy-state record, and child exit resultVerified/tmp/fm-codex-appserver-evidence-successwas removed andgit status --porcelain=v1remained empty✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.