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
…ayload Published docs described a product that does not exist: 1,213 tests (actually 1,631), 18 hooks in the ko/zh READMEs and across the docs site (actually 21), 25-27 skills (actually 28), and a cxc-ultraresearch skill that has not shipped since the protocol was absorbed into cxc-search Tier 3. - tests badge 1,213 -> 1,631 in all three READMEs and the docs site - hook count 18 -> 21 everywhere, with the correct event split 5/3/6/2/1/1/3 - add the three managed-worktree hooks to the hooks reference, the manifest reference (now generated from the real manifest array) and how-it-works - skills 25/27 -> 28; add dev-diagram-viewer, kwrite, remote, worktree-guardian - drop cxc-ultraresearch from docs site, structure/INDEX.md and the skill-hub catalog; point the parallel-lane rows at cxc-search Tier 3 - repair dead structure/INDEX.md paths (_plan/mvp_* moved to _fin/) and add the missing skill-search component section - list all eight component test dirs in build-test - add CHANGELOG.md recording the PR #1 ancestry answer (squashed as dac77cc, merged 2026-08-09, never released) Verified: npm test 1631 pass / 0 fail (exit 0); gate.mjs exit 0; the INDEX path check reports 0 misses (7 before); all three worktree hooks present in all three reference surfaces. Refs #24
…the changelog Release prep runs immediately before promotion, after every test-adding phase has landed: the suite is measured (1659 pass / 0 fail), inventory.mjs --write --tests republishes every registered surface from that measurement, and --published then reports 1659. Doing it earlier would leave the badge behind the phases that added tests, and the release gate's publishedCounts rule would correctly refuse. - package.json and the eight component packages -> 0.2.0-beta.1 - plugin.json -> 0.2.0-beta.1+codex.20260815063523 (build metadata regenerated) - CHANGELOG: the 0.2.0-beta.1 section, with PR #1's ancestry answer recorded Verified: npm test 1659/0; inventory --check exit 0; gate exit 0; build OK. Refs #28
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