fix: contain stdio process trees during aggregate teardown - #263
Merged
Conversation
📝 WalkthroughWalkthroughThe PR adds deterministic macOS OAuth lock fallbacks and replaces upstream stdio handling with a platform-aware contained transport. Upstream startup, shutdown, descendant cleanup, Windows launching, and related tests are updated. ChangesmacOS OAuth lock coordination
Contained upstream stdio lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant UpstreamProcessManager
participant ContainedStdioClientTransport
participant ChildProcess
participant ContainmentBoundary
UpstreamProcessManager->>ContainedStdioClientTransport: close()
ContainedStdioClientTransport->>ChildProcess: end stdin
ContainedStdioClientTransport->>ContainmentBoundary: verify containment
ContainedStdioClientTransport->>ChildProcess: force terminate after timeout
ContainmentBoundary-->>ContainedStdioClientTransport: containment complete
ContainedStdioClientTransport-->>UpstreamProcessManager: onclose
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/oauth-local-lock.test.ts (1)
223-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting a
deferred()helper for the repeated hold/entered promise pattern.The
let releaseX!: () => void; const holdX = new Promise<void>((resolve) => { releaseX = resolve; });pattern (and itsmarkXEntered/xEnteredcounterpart) is repeated ~4 times across these two tests. A small shared helper would reduce duplication and make the tests easier to scan.♻️ Proposed helper
+function deferred<T = void>(): { promise: Promise<T>; resolve: (value: T) => void } { + let resolve!: (value: T) => void; + const promise = new Promise<T>((res) => { + resolve = res; + }); + return { promise, resolve }; +}Usage example:
- let releaseFirst!: () => void; - const holdFirst = new Promise<void>((resolve) => { - releaseFirst = resolve; - }); - let markFirstEntered!: () => void; - const firstEntered = new Promise<void>((resolve) => { - markFirstEntered = resolve; - }); + const { promise: holdFirst, resolve: releaseFirst } = deferred(); + const { promise: firstEntered, resolve: markFirstEntered } = deferred();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/oauth-local-lock.test.ts` around lines 223 - 333, Extract a shared deferred-promise helper for the repeated hold/release and entered/marker pairs in the macOS lock tests. Update the affected tests around withOAuthLocalLock to use the helper for both waiting and resolving, while preserving the existing synchronization and cleanup behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/upstream/contained-stdio-transport.ts`:
- Around line 187-191: Bound the second await of childClose in the close flow
after forceTerminate(), using the existing gracefulShutdownDelayMs or an
equivalent timeout helper. Keep verifyContainment() as the authoritative step
that detects and throws for unconfirmed containment, while preserving the normal
graceful-close path.
- Around line 266-278: Replace the single post-SIGKILL probe in the termination
flow with bounded polling until containmentVerificationTimeoutMs (1,000 ms)
expires, rechecking isPosixProcessGroupRunning after short delays. Record and
throw the containment failure only if the process group remains running at the
deadline or verification throws; preserve the existing forceTerminate and
containmentFailure checks.
In `@tests/contained-stdio-transport.test.ts`:
- Around line 347-355: Guard the “rejects a second start rather than spawning a
second contained child” test with the same runIf condition used by the other
real-process tests, excluding win32. Keep the existing transport setup and
double-start assertion unchanged for supported platforms.
---
Outside diff comments:
In `@tests/oauth-local-lock.test.ts`:
- Around line 223-333: Extract a shared deferred-promise helper for the repeated
hold/release and entered/marker pairs in the macOS lock tests. Update the
affected tests around withOAuthLocalLock to use the helper for both waiting and
resolving, while preserving the existing synchronization and cleanup behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 16327af8-3ec9-4903-9b90-22bdf072ee89
📒 Files selected for processing (8)
src/oauth/local-lock.tssrc/secrets/windows-secret-command.tssrc/upstream/contained-stdio-transport.tssrc/upstream/upstream-process-manager.tstests/contained-stdio-transport.test.tstests/oauth-local-lock.test.tstests/upstream-manager.test.tsvitest.config.ts
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
Review exception documented for the final head
I manually re-reviewed the final containment diff and test coverage: no shell execution was introduced; Windows remains fail-closed on missing Job Object helper-close proof; POSIX only emits close after a verified-empty process group. Dismissing the stale automated request under the maintainer rate-limit exception. |
mohanagy
dismissed
coderabbitai[bot]’s stale review
July 27, 2026 06:56
All inline findings are resolved. CodeRabbit incremental re-review is rate-limited and documented in the PR; final CI is green.
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
1 participant
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.
Summary
Security impact
shell: false.Root cause
A direct stdio child could exit while a descendant retained inherited pipes. Its public close never arrived, leaving teardown/capacity state behind for later aggregate tests. Windows also needed a launch-time Job Object boundary rather than post-exit tree cleanup.
Validation
npm test(1,716 passed, 27 skipped)npm run test:core(415 passed, 22 skipped)npm run test:coverage(1,716 passed, 27 skipped; containment transport 86.8% branches)npm run lint,npm run typecheck,npm run build,npm run smoke:cli,npm run check:pack,npm run test:packageFixes #255