fix(fx-dev): make a finished wait detectable and forbid double-backgrounding - #27
Merged
Conversation
… runner The runner's stdout gave the caller nothing to branch on: a log that stopped growing mid-review was indistinguishable from a finished one, so a truncated capture was read as a completed review. Every exit path now ends in a STATUS= sentinel — COMPLETED, DRY_RUN, or ERROR — written last, after the EXIT trap terminates and confirms the death of every descendant that inherited the log's file descriptor. Without that reap, a codex child outliving a signalled runner appends review prose after the sentinel and the log has no STATUS= tail at all. STATUS is decoupled from the exit code: COMPLETED passes codex's own status through unchanged and reports it separately as CODEX_EXIT=, because a non-zero codex exit does not distinguish a failed reviewer from an opinionated one. The pipeline runs under set +e with PIPESTATUS captured by the very next statement; any command in between resets the array.
`pgrep -f 'codex review'` matches the watching command's own command line, so the check never reports the process gone. One stood here and spun for 13 minutes after the reviewed process had already finished. background-waits.md already owns this check and forbids the pattern outright; point at it rather than keeping a second, broken copy that can diverge again.
…wait looks like An `&` added to a launch run_in_background: true had already backgrounded made the wrapper return in milliseconds; the instant return was read as the review completing and a truncated log was reported as a finished review. Forbid `&`, `nohup` and `disown` on an already-backgrounded launch, each with the distinct way it breaks — `&` returns the wrapper's status, `nohup` buys nothing, `disown` makes `wait` report status 0 for a live job — and state the completion test the failure actually needed: judge a wait by its log's tail, not by how fast the call came back.
There was a problem hiding this comment.
🔵 Needs a closer look
The Bash signal/trap and process-reaping changes are intricate enough that they warrant a final human verification pass in a real runtime environment.
Pull request overview
This PR ports upstream fixes into this repo’s fx-dev skills to make codex-review background waits reliably detectable and to prevent “instant return” false-completion caused by double-backgrounding, while also removing a known self-matching pgrep -f diagnosis footgun.
Changes:
- Adds a strict stdout completion sentinel (
STATUS=+CODEX_EXIT=) torun-codex-review.sh, including signal-path handling to prevent post-sentinel log writes. - Updates
codex-review/SKILL.mdto remove the self-matching process check and to document the newSTATUS=contract as the authoritative branching mechanism. - Documents the “never double-background” rule in
background-waits.mdand bumps thefx-devplugin + marketplace metadata versions.
File summaries
| File | Description |
|---|---|
| plugins/fx-dev/skills/dev/references/background-waits.md | Adds an explicit rule forbidding &/nohup/disown when run_in_background: true is already used, and clarifies “instant return ≠ completion”. |
| plugins/fx-dev/skills/codex-review/SKILL.md | Removes the self-matching pgrep -f stalled-review check and adds guidance to branch on the log’s final STATUS= line. |
| plugins/fx-dev/skills/codex-review/scripts/run-codex-review.sh | Implements the STATUS= completion contract and ensures the sentinel remains the log tail (including descendant reaping on abnormal exits). |
| plugins/fx-dev/.claude-plugin/plugin.json | Bumps fx-dev version to 5.2.2. |
| .claude-plugin/marketplace.json | Bumps marketplace metadata version to 0.27.2. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Why
Port of fx/skills#6 into this repo's copy of the
fx-devskills. Same two production failures, both wait-shaped:while pgrep -f "codex review"matched its own command line, so it waited on itself and spun for 13 minutes after the reviewed process had already finished.&was added inside a call the host had already backgrounded. The wrapper returned in milliseconds, that instant return was read as the review completing, and a truncated log was read as a finished review.The
pgreprule already existed inbackground-waits.md, andcodex-review/SKILL.mdalready pointed at it. It was violated anyway — so more prose was not the fix.The root cause was structural.
run-codex-review.shwas the only script any skill launches as a background wait with no completion contract. Its three siblings (wait-for-ci-checks.sh,wait-for-copilot-review.sh,wait-for-coderabbit-review.sh) all emitSTATUS=<state>as their last stdout line under an EXIT trap. Without that sentinel a truncated log is indistinguishable from a finished one (failure 2), and with no completion signal to wait on, an agent invents one (failure 1).codex-review/SKILL.mdwas also handing readers the loaded gun: its Diagnosing a stalled review section — the one you open precisely when a wait seems hung — containedpgrep -f 'codex review', the exact self-matching patternbackground-waits.mdforbids.What changed
1.
run-codex-review.shgains aSTATUS=completion contract —COMPLETED,DRY_RUN,ERROR. Not the siblings' five states:PENDINGcannot occur (no budget, nothing times out),NOT_CONFIGUREDcannot occur (no configuration question), andTERMINAL_PASSmust not exist — it would be read as "no findings", the exact misread the header was written to prevent.STATUSis decoupled from the exit code: onCOMPLETEDthe code is codex's own, reported asCODEX_EXIT=<n>and given no meaning here. Documented exit-3 semantics unchanged.2. The self-matching
pgrepis gone from the stalled-review diagnosis, replaced by a pointer to the log-mtime checkbackground-waits.mdalready owns. The genuinely diagnostic rollout step is kept.3. The double-background rule lands once, in
background-waits.md§ The rule, adjacent to the launch template it is a mutation of.Port notes
Written against this repo's conventions, not the source's:
[SKILL_BASE_DIR]/skills/...rather than[SKILLS_DIR]/..., literal.claude/team/waitspaths, andfx-dev:-prefixed skill names. No[SKILL_BASE_DIR]definition notes were added, since this repo does not use them.This repo's
background-waits.mdis a narrower variant than the source's — nohost-adapters.mdto reference, and different anchor wording ("Never background a wait without the redirect"). The rule was rewritten in this file's own idiom, namingrun_in_background: truedirectly instead of the source's per-host phrasing, and both§cross-references resolve inside this file. Reconciling the two variants is deliberately out of scope.Before this change
run-codex-review.shdiffered from the source on exactly three lines, all skill-name spellings. After it, that is still the only divergence — verified by diff.Version bumps are in the first commit because
.githooks/pre-commitcompares againstmerge-base HEAD mainand rejects any commit touchingplugins/<name>/without the bump already present. Every commit ran the hook unbypassed.Design notes for review
&,nohupanddisownare forbidden for three different reasons, not one. Verified:&forks and returns at once;nohupruns in the foreground and passes the exit status through unchanged, adding only SIGHUP immunity plus anohup.outredirect that engages solely when stdout is a terminal — which the mandatory> log 2>&1already precludes;disownbackgrounds nothing, but drops the job from the shell's table, after whichwait <pid>returns immediately with status 0 (measured: 0 ms and rc 0 while the child was still running, versus 2000 ms and rc 7 for the same tracked job), fabricating a clean finish.The reap's bounded
kill -0poll is not a hand-rolled wait.background-waits.md§ What this does NOT govern blesses exactly this: a bounded teardown wait inside a single command with no external completion to wait for.pgrep -P <pid>filters on parent PID, so unlike the forbiddenpgrep -f <pattern>it cannot self-match — verified.pgrepis a soft dependency. Without it the reap no-ops and the late-write hazard returns, degrading quietly: theSTATUS=line and the 128+N exit are unaffected.Upstream review
The source PR went through four Codex iterations. Nine findings: seven fixed, two classified immaterial (a PID-reuse race requiring the kernel to cycle the whole PID space in microseconds, and a residual single-assignment gap that is not closable in bash — trap dispatch happens between commands, and
trap ''discards signals rather than queueing them). One Copilot thread disputed thedisownclaim; it was tested, the claim held, and the thread was resolved with the measurements. Three of the nine findings were false shell-behaviour claims in the new text, which is why every such claim now carries its verifying test.Test plan
Run against this repo's copy with a
codexshim; no real review was invoked.bash -non every bundled script parsesSTATUS=ERROR, exit 3STATUS=ERROR, exit 4 via the trap (exited 1 before this change — inside codex's reserved range)CODEX_REVIEW_DRY_RUN=1→STATUS=DRY_RUN, exit 0tee→CODEX_EXIT=1,STATUS=COMPLETED, exit 1 (not inverted to ERROR)CODEX_REVIEW_OUT→WARNINGnaming the path,STATUS=COMPLETED— ateefailure is attributed totee, not to codextoml_keyquoting intact (mcp_servers."acme.review".enabled=false)STATUS=ERROR, zero review lines after the sentinelcodex-review/SKILL.md(only the prohibition prose reference)jq emptyvalid on both version filesNot done:
shellcheckis not installed on this machine, so validation isbash -nplus the behavioural suite.