feat: add PTY tools for background terminal sessions#6
Merged
Conversation
- Add RingBuffer for output capture with search support - Add PTYManager for session lifecycle management - Add pty_spawn, pty_write, pty_read, pty_list, pty_kill tools - Add comprehensive test coverage for all components
- Commander: terminal tool selection guidance - Implementer: bash vs pty decision rules - Executor: PTY tools for background processes - Reviewer: PTY cleanup verification
There was a problem hiding this comment.
5 issues found across 23 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/tools/pty/tools/write.ts">
<violation number="1" location="src/tools/pty/tools/write.ts:92">
P2: The return message reports `args.data.length` bytes but actually sends `parsedData.length` bytes. After escape sequence parsing, `\x03` (4 chars) becomes a single byte, making the reported count inaccurate. Use `parsedData.length` for the correct byte count.</violation>
</file>
<file name="src/index.ts">
<violation number="1" location="src/index.ts:242">
P2: Duplicated `session.deleted` check. This new block is identical in structure to the existing one above it. Consider merging the PTY cleanup into the existing `session.deleted` handler to reduce code duplication and improve maintainability.</violation>
</file>
<file name="tests/tools/pty/read.test.ts">
<violation number="1" location="tests/tools/pty/read.test.ts:74">
P2: Weak assertion - this only verifies the output format includes `pattern=`, not that filtering actually works. Consider asserting that the result contains the matched line ("error: bad") and/or doesn't contain non-matching lines ("line1", "line3").</violation>
</file>
<file name="src/tools/pty/tools/spawn.ts">
<violation number="1" location="src/tools/pty/tools/spawn.ts:50">
P2: The `description` parameter is required but never used. It's not passed to `manager.spawn()`, not stored in the session, and not included in the output. Either pass the description through the manager to store it (requires updating `SpawnOptions`, `PTYSession`, and `PTYSessionInfo` types), or mark it as `.optional()` if it's not needed.</violation>
</file>
<file name="tests/tools/pty/spawn.test.ts">
<violation number="1" location="tests/tools/pty/spawn.test.ts:49">
P2: Potential flaky test: `echo hello` completes almost instantly, so asserting `Status: running` may fail intermittently due to a race condition. Consider using a longer-running command like `sleep 1` or `cat`, or assert on a status that accommodates both running and completed states.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- buffer.test.ts: negative offset, empty string, unicode handling - manager.test.ts: read() with offset/limit, search(), write to killed session - integration.test.ts: spawn → write → read → kill lifecycle tests
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="tests/tools/pty/manager.test.ts">
<violation number="1" location="tests/tools/pty/manager.test.ts:146">
P2: Weak assertion: `toBeGreaterThanOrEqual(0)` will always pass since array length can never be negative. This test named "should find matching lines" doesn't actually verify that matches were found. Use `toBeGreaterThanOrEqual(1)` or `toBeGreaterThan(0)` to validate the search functionality.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- write.ts: Report parsedData.length instead of args.data.length for accurate byte count - spawn.ts: Make description parameter optional (was required but unused) - read.test.ts: Strengthen pattern filtering assertion to verify actual filtering
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/tools/pty/tools/spawn.ts">
<violation number="1" location="src/tools/pty/tools/spawn.ts:51">
P2: Documentation/schema mismatch: The `DESCRIPTION` constant states the `description` parameter is required, but the schema now marks it as optional. Either update the documentation to reflect that description is optional, or remove `.optional()` if it should remain required.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- spawn.test.ts: Use sleep instead of echo to avoid race condition on status check - manager.test.ts: Use toBeGreaterThan(0) instead of toBeGreaterThanOrEqual(0) for meaningful assertion - spawn.ts: Fix documentation to reflect that description is optional
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="README.md">
<violation number="1" location="README.md:13">
P2: JSON does not support comments. Users copying this config block directly will get a parse error. Consider moving the path comment outside the code block or using a JSON5 syntax hint.</violation>
<violation number="2" location="README.md:94">
P2: Incorrect threshold value. The `auto-clear-ledger` hook uses 80% threshold (see `src/hooks/auto-clear-ledger.ts`), not 60%. The 60% threshold is for `preemptive-compaction`, which is a different hook.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Remove JSON comment (not valid JSON) - Clarify threshold: preemptive compaction at 60%, auto-clear at 80%
- Update auto-clear-ledger from 80% to 60% - Update test expectations - Update README to reflect consistent 60% threshold
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
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
New Tools
pty_spawnpty_writepty_readpty_listpty_killKey Distinction
background_taskpty_spawnChanges
New Files (17)
src/tools/pty/- 9 files (types, buffer, manager, 5 tools, index)tests/tools/pty/- 7 test files (46 tests)Modified Files
package.json- add bun-pty dependencysrc/index.ts- register PTY tools with session cleanupTesting
Design Document
See
thoughts/shared/designs/2026-01-01-pty-integration-design.mdSummary by cubic
Adds PTY tools to manage background terminal sessions so long-running and interactive processes don’t block the agent. Prompts now choose between bash and PTY, and sessions auto-clean on session deletion.
New Features
Bug Fixes
Written for commit aa4b550. Summary will update on new commits.