Conversation
github-actions
bot
added
the
maintainer:e2e:ok
Contributor
LLxprt PR Review – PR #1403Issue AlignmentDirectly addresses all 5 root causes in #1401: (1) Side EffectsMinimal. Code QualityStrong. Centralized cleanup function eliminates duplication. Proper error handling with try/catch around all cleanup operations. Closure issue fixed by moving Tests and CoverageCoverage impact: increase. Added 9 meaningful tests: 3 CLI cleanup tests (auto-invocation, duplicate tolerance, error safety), 5 VerdictReady |
Contributor
Summary by CodeRabbit
WalkthroughAdds a static ShellExecutionService.destroyAllPtys() and invokes it from the CLI exit cleanup flow (errors ignored). Core ShellExecutionService now tracks per-PTY resources, centralizes cleanup, and expands tests for PTY destruction, listener disposal, and abort/exit race handling. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Process
participant Cleanup as runExitCleanup
participant SES as ShellExecutionService
participant PTY as Active PTYs
CLI->>Cleanup: process exit
Cleanup->>SES: destroyAllPtys()
activate SES
SES->>PTY: iterate activePtys
loop per PTY
SES->>PTY: safePtyDestroy (prefer destroy(), fallback kill())
PTY-->>SES: destroyed
SES->>SES: dispose listeners & headless terminal
end
SES->>SES: reset lastActivePtyId
SES-->>Cleanup: complete (errors ignored)
deactivate SES
Cleanup->>Cleanup: continue sync cleanup
Cleanup-->>CLI: exit cleanup finished
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
Author
|
Addressed both requested CodeRabbit nitpicks in commit c31d52e:
Re-ran full verification after these updates:
Also noting CI bot feedback: PR remains linked via title (Fixes #1401), and PR body will be updated to include an explicit linked-issue section as well. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/core/src/services/shellExecutionService.ts`:
- Around line 1099-1110: finalizeResult can access a disposed `@xterm/headless`
terminal (calling render(true) and getFullBufferText(headlessTerminal)) after
destroyAllPtys/cleanupPtyEntryResources disposes headlessTerminal; add a guard
at the start of finalizeResult to skip buffer access when the PTY was removed or
disposed (e.g., check this.activePtys.has(pid) or an entry.disposed boolean on
the PtyEntry) and only call render(true)/getFullBufferText when the entry and
its headlessTerminal are still valid, or set and check entry.disposed when
cleanupPtyEntryResources disposes the terminal to short-circuit finalizeResult.
🧹 Nitpick comments (2)
packages/core/src/services/shellExecutionService.ts (1)
828-830:processingChain.catch(() => {})is re-attached on every data chunk.This works but creates a new no-op
.catchhandler on everyhandleOutputcall. SinceprocessingChainis reassigned each time (line 779), the previous.catchis already attached to the old tail. This is functionally correct — just slightly wasteful. Not blocking, but you could attach the catch once outsidehandleOutputand keep re-pointing a single variable.packages/core/src/services/shellExecutionService.test.ts (1)
596-708: ThoroughdestroyAllPtystest suite.Covers multi-PTY cleanup, no-op on empty map,
destroy()preference, andkill()fallback. The tests properly resolve pending promises by firingonExitafterdestroyAllPtys, which mirrors the real-world sequence.One minor hardening suggestion: consider calling
ShellExecutionService.destroyAllPtys()in the top-levelbeforeEach(line 74) to guard against state leakage from a prior test that might fail mid-execution before its own cleanup runs.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/services/shellExecutionService.test.tspackages/core/src/services/shellExecutionService.ts
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-03T00:20:08.053Z
Learning: ShellExecutionService applies a hard cap only in the child_process path (appendAndTruncate, “16MB” message) and currently has no truncation in the PTY path, which can lead to unbounded memory before tool-level token limiting runs.
📚 Learning: 2026-01-03T00:20:08.053Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-03T00:20:08.053Z
Learning: ShellExecutionService applies a hard cap only in the child_process path (appendAndTruncate, “16MB” message) and currently has no truncation in the PTY path, which can lead to unbounded memory before tool-level token limiting runs.
Applied to files:
packages/core/src/services/shellExecutionService.test.tspackages/core/src/services/shellExecutionService.ts
📚 Learning: 2026-02-06T15:52:42.315Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1305
File: scripts/generate-keybindings-doc.ts:1-5
Timestamp: 2026-02-06T15:52:42.315Z
Learning: In reviews of vybestack/llxprt-code, do not suggest changing existing copyright headers from 'Google LLC' to 'Vybestack LLC' for files that originated from upstream. Preserve upstream copyrights in files that came from upstream, and only apply 'Vybestack LLC' copyright on newly created, original LLxprt files. If a file is clearly LLxprt-original, it may carry the Vybestack header; if it is upstream-originated, keep the original sponsor header.
Applied to files:
packages/core/src/services/shellExecutionService.test.tspackages/core/src/services/shellExecutionService.ts
📚 Learning: 2026-01-03T00:20:08.053Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-03T00:20:08.053Z
Learning: Shell tool (packages/core/src/tools/shell.ts) always applies tool-output-max-tokens via limitOutputTokens to the model-facing llmContent at the end of execute(), formatting a warning when truncated.
Applied to files:
packages/core/src/services/shellExecutionService.ts
📚 Learning: 2026-02-11T15:12:42.133Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1260
File: packages/core/src/tools/task.ts:645-894
Timestamp: 2026-02-11T15:12:42.133Z
Learning: In packages/core/src/tools/task.ts, async tasks intentionally pass `undefined` (not the foreground signal) to orchestrator.launch() so background tasks run independently of foreground agent lifecycle (Ctrl+C, turn completion). They use their own asyncAbortController for manual cancellation via /task end and setTimeout for timeout handling. This is documented at lines 748-754 and was fixed in commit c00f12740.
Applied to files:
packages/core/src/services/shellExecutionService.ts
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Test (Linux) - sandbox:docker
🔇 Additional comments (6)
packages/core/src/services/shellExecutionService.ts (4)
141-156: Clean helper for safe PTY teardown.Good pattern — preferring
destroy()overkill()with a blanket catch for already-exited PTYs.
158-190: Centralized resource cleanup looks solid.Each disposal is individually wrapped in try/catch, which is the right approach for teardown code where any single step can fail independently. Timeouts are cleared and nulled correctly.
849-877: Race-listener cleanup pattern looks correct.Storing
raceAbortListenerby reference and removing it in both.thenand.catchpaths ensures no leaked listeners. ThecleanupRaceListeneridempotency (null-check + null-assign) is a nice touch.
575-581: Early registration ofactivePtyEntrybefore disposables are assigned is fine.
cleanupPtyEntryResourcesuses optional chaining (?.dispose()) so partially-initialized entries are handled safely.packages/core/src/services/shellExecutionService.test.ts (2)
567-593: Good regression tests for PTY kill-on-cleanup and resilience.These two tests validate that
safePtyDestroyis invoked during normal exit cleanup and that a throwingkill()doesn't propagate. Solid coverage for the new cleanup path.
710-798: Excellent listener-identity and catch-path coverage.Verifying that every
addEventListener('abort', cb)has a matchingremoveEventListener('abort', cb)with the exact same reference is a strong way to prove no listener leaks. The catch-path test (poisoningisBinary) is a creative way to exercise the.catchbranch in thePromise.race.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
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.
TLDR
Fixes PTY resource leaks by making PTY teardown deterministic in shell execution cleanup paths and in CLI exit cleanup, with regression tests around abort-race listener cleanup and global PTY teardown.
Dive Deeper
Reviewer Test Plan
Run:
Testing Matrix
Linked issues
Fixes #1401