fix: harden runtime boundaries and resource lifecycles - #1
Conversation
|
✅ Target branch corrected This pull request now targets The |
github-actions
Bot
changed the title
github-actions
Bot
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f2efabfe3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const recursionRequested = !spawnedBySubagent && message.includes(SUBSPAWN_TOKEN); | ||
| const mintedGrant = recursionRequested ? mintRecursionGrant(obj) : null; | ||
| const controlledMessage = stripControlMarkers(message); |
There was a problem hiding this comment.
When a host applies the hook twice to a root coordinator spawn, the first pass has already removed SUBSPAWN_TOKEN, so the second pass sets recursionRequested to false and stripControlMarkers deletes the minted CXC-SUBSPAWN-GRANT. It then prepends a leaf guard, leaving the coordinator unable to perform its authorized child spawn despite the idempotence guarantee below. Re-running runSpawnAttachHook on its own first-pass updatedInput.message reproduces the lost grant.
Useful? React with 👍 / 👎.
| export function terminateChild(child: ChildProcess): void { | ||
| if (child.exitCode !== null || child.signalCode !== null) return; | ||
| child.kill("SIGTERM"); | ||
| // `exit` does not imply the process group is gone: a grandchild can retain an | ||
| // inherited stdout/stderr descriptor and prevent Node's `close` event. Always | ||
| // signal the detached group while the runner still owns this ChildProcess. | ||
| signalProcessTree(child, "SIGTERM"); | ||
| if (process.platform !== "win32") { | ||
| const timer = setTimeout(() => { | ||
| if (child.exitCode === null && child.signalCode === null) child.kill("SIGKILL"); | ||
| signalProcessTree(child, "SIGKILL"); |
There was a problem hiding this comment.
When the direct Codex process exits after spawning a descendant that inherits its output pipe, this group-level SIGTERM does not remove the descendant before runTurn returns; the newly added runTurn: timeout kills a process group after the direct child has exited test consistently finds the PID still reachable, and the test process remains open until the three-second SIGKILL fallback. Timed-out turns can therefore leave helper processes and inherited resources alive during the grace period rather than satisfying the intended process-tree cleanup.
Useful? React with 👍 / 👎.
…MERGED dac77cc) WP2 of loop codex-rs-codexclaw-pr-1-hotl-pabcd-2-developer-c. - 000_pr_investigation: metadata, change map, staleness/overlap analysis, independent audit (MERGE-WITH-FIXES, 4 Medium as follow-up seeds), local merge-tree verification (1507 tests, build, gate), merge record. - 001_merge_verdict: parallel re-audit session's verification doc, reconciled with the executed squash merge.
WP2 of loop codex-rs-codexclaw-pr-1-hotl-pabcd-2-developer-c. - 000_pr_investigation: PR meta, change terrain, staleness/overlap analysis - 001_merge_verdict: direct verification (PR head 1453/0, merge-sim vs latest dev 1507/0, gate+build OK), merge executed (squash dac77cc, 2026-08-09T01:18:41Z), post-merge independent audit GO-WITH-FIXES blockers=0, two non-blocking High follow-ups recorded
2 participants
What
Why / root cause
Several independently grown runtime paths had inconsistent trust rules and unbounded resource lifecycles. Oversized or child-controlled input could bypass policy checks, while slow storage, concurrent downloads, stale jobs, event streams, and descendant processes could retain resources indefinitely.
Compatibility and impact
Normal-size API requests, messages, attachments, subagent dispatch, and recent job history keep their existing behavior. Intentional limits affect only overload or unsafe cases:
Verification