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 👍 / 👎.
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