Repository navigation
Fix Windows provider descendant cleanup - #25
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: f72672979a
ℹ️ 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".
jmborden1112-beep
left a comment
jmborden1112-beep
left a comment
There was a problem hiding this comment.
LGTM
Needs Fixing
None. The Windows job cleanup paths fail closed and retain descendant cleanup on assignment, resume, thread-start and read failures.
Requires Human Review
None beyond the repository approval gate fulfilled here under the owner's second-account authorization. This is agent source review, not independent human sign-off.
Recommended Optional
None.
Create Follow-up Issue
None.
Workflow Trace: exact head3ab154be0dd72a6a1ef7967afbdb4b6b39297b00; source review by /root/fable_stack_review plus root verification. Actual worker fallback was gpt-6-luna medium because the skill-preferred worker was unavailable. Local npm test:116passed,3platformskips,0failed; Python selector suite77tests with1skip; package validators and diff check passed. All eight final-head hosted checks passed. No live models, installation into the user's Codex configuration, publication, or activation. Older #22/#23 snapshots reproduce the descendant-cleanup failure, while this parent fixes it; they will be refreshed after this merge.
Windows provider timeouts could leave a descendant alive after the deadline because cleanup depended on taskkill. The parent now owns a kill-on-close Job Object, starts the supervisor suspended, assigns it before resuming, and terminates the job directly on timeout. Startup interruption and thread-start failure also close the job. Processes remain hidden.
Pipe-read failures now report provider_failed even when termination returns reserved supervisor codes 124/125; genuine supervisor startup failures still report provider_unavailable. Regression coverage checks both classifications and cleanup, alongside the existing descendant timeout and successful-background-child checks. The PowerShell wrapper assertion tolerates line wrapping while preserving version/error requirements.
Final local validation at 3ab154b on Windows: 119 Node tests, 116 passed and three skipped; 77 Laya Python tests, 76 passed and one unavailable Windows symlink test skipped; package validation passed. The new regression failed for both reserved codes before the fix and passed afterward. Independent automated source review found no remaining issue. All eight hosted checks passed on this exact head, including Ubuntu/macOS/Windows on Node 18/24, packed artifact and the release gate.
This is local subprocess cleanup, not a sandbox or OpenShell containment claim. No model, paid provider, installed plugin, or live runtime was changed.