Repository navigation
Conversation
Greptile SummaryThe PR adds
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/cli/bounded-stdin-text.ts | Extracts a reusable stream reader that stops once cumulative input exceeds the supplied byte limit. |
| src/cli/handlers/artifacts.ts | Replaces the local stdin implementation with the shared bounded reader without changing its error contract. |
| src/cli/handlers/terminal.ts | Adds file and stdin text sourcing with an early file-size check, bounded streaming, exclusivity validation, and unchanged RPC semantics. |
| src/cli/handlers/terminal.test.ts | Covers file and stdin input, validation errors, bounded oversized input, help output, and agent-prompt classification. |
| src/cli/specs/core.ts | Registers and documents the new mutually exclusive `--text-file <path |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[terminal send] --> B{Input source}
B -->|--text| C[Inline text]
B -->|--text-file path| D[Stat file]
B -->|--text-file -| E[Read stdin]
D --> F[Bounded stream reader]
E --> F
C --> G[Validate terminal input limit]
F --> G
G --> H[terminal.send RPC]
Reviews (2): Last reviewed commit: "fix(cli): bound terminal text file reads" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 85237e4109c78e726422b6ded7d66737f3b65f89 and 22f58c0. 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughAdds bounded UTF-8 text reading for async streams and stdin. Adds Merge Risk: ⚪ Minimal · up to Terminal send now supports bounded UTF-8 text from files or stdin while retaining existing prompt and retry behavior. The previously incomplete validation guidance has been corrected, leaving no current merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5013e96a-64f4-44c3-b417-7ddc09eb5cce
📥 Commits
Reviewing files that changed from the base of the PR and between d607a63 and 32bdf27f8912709655d4e3f73a329e9d936b9696.
📒 Files selected for processing (5)
src/cli/bounded-stdin-text.tssrc/cli/handlers/artifacts.tssrc/cli/handlers/terminal.test.tssrc/cli/handlers/terminal.tssrc/cli/specs/core.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The feature works and is well-tested, but the file path reads the whole file into memory before enforcing the 16 MiB limit (the stdin path is streaming-bounded), and the file too-large error is a plain Error while the stdin one is a RuntimeClientError — so --json consumers get different error codes for the same condition. Both are anchored inline.
Reviewed changes
terminal sendgains--text-file <path|->(UTF-8 file, or stdin via-), mutually exclusive with--text, with explicit rejection of empty input and the existing 16 MiB terminal-input limit enforced.- Extracted the bounded stdin reader used by
artifacts createintosrc/cli/bounded-stdin-text.tsand reused it in both handlers (artifact behavior unchanged). - Agent-prompt classification (
text + enter, notinterrupt) is preserved for file/stdin input. - Spec/usage line updated, plus tests for file, stdin, exclusivity, missing/empty file, size limit, agent-prompt semantics, and help text.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
32bdf27 to
7af6d50
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 45732c9a-c376-4ed0-9b75-ab1254e1b5e1
📥 Commits
Reviewing files that changed from the base of the PR and between 212c0e4 and 7af6d50c3ba5bcb90e0883d1088f767768defcbc.
📒 Files selected for processing (5)
src/cli/bounded-stdin-text.tssrc/cli/handlers/artifacts.tssrc/cli/handlers/terminal.test.tssrc/cli/handlers/terminal.tssrc/cli/specs/core.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/cli/handlers/artifacts.ts
- src/cli/specs/core.ts
- src/cli/handlers/terminal.ts
- src/cli/handlers/terminal.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The delta against the prior review resolves both open findings: the file read path is now bounded and the too-large error surface is unified.
- Bounded the
--text-filefile read —readTerminalSendTextFilenow stats the path first (size pre-check) and streams viacreateReadStreamthroughreadTextStreamWithinLimit, so oversized files are rejected before a read stream is even opened. - Generalized the bounded stdin reader — extracted
readTextStreamWithinLimit(input, maxBytes, createTooLargeError)with areadStdinTextWithinLimitwrapper;artifacts.tsreuses it with unchanged behavior. - Unified the too-large error code — file and stdin paths both throw
RuntimeClientError('invalid_argument', TERMINAL_INPUT_TOO_LARGE_ERROR), so--jsonconsumers no longer see divergent codes. - Added regression tests — oversized-file short-circuit (
createReadStreamnever called) and oversized-stdin JSON code parity, via avi.hoistedmock ofnode:fscreateReadStream.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
7af6d50 to
85237e4
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Rebased onto What moved: the flag lives on Ordering note: Verified on Windows 11, Node 24.19.0, pnpm 12 (corepack): |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/handlers/terminal-send.ts (1)
26-26: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the prompt-option validation message.
--text-filenow satisfies this requirement. The message still says that--retry-requestand--wait-submitrequire--text.State that either
--textor--text-fileis required, with--enterand without--interrupt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 335eba78-e889-478e-a68e-255456aa5000
📥 Commits
Reviewing files that changed from the base of the PR and between 8cd0abf and 85237e4109c78e726422b6ded7d66737f3b65f89.
📒 Files selected for processing (6)
src/cli/bounded-stdin-text.tssrc/cli/handlers/artifacts.tssrc/cli/handlers/terminal-send-text-file.test.tssrc/cli/handlers/terminal-send.tssrc/cli/specs/terminal-send.tssrc/cli/terminal-send-text-source.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/cli/handlers/artifacts.ts
- src/cli/bounded-stdin-text.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run reviewed the delta since the prior pullfrog review at 7af6d50: a rebase onto current main plus an extraction of the file/stdin resolution into its own module.
- Extracted the input resolution —
readTerminalSendTextFileandgetTerminalSendTextmoved out of the terminal handler intosrc/cli/terminal-send-text-source.ts; the logic is byte-identical to the previously-reviewed version. - Integrated into the richer handler —
src/cli/handlers/terminal-send.tsnow resolves input viaawait getTerminalSendText(flags, cwd), so file/stdin input flows through the existing prompt-candidacy,--retry-request/--wait-submitpreflight, and legacy-host paths exactly like--text. - Relocated spec and coverage — the usage/flag changes now live in
src/cli/specs/terminal-send.ts, and the tests insrc/cli/handlers/terminal-send-text-file.test.ts(12 tests passing;tc:cliclean).
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Review follow-up: the bounded reader now keeps Uint8Array chunks as bytes instead of stringifying them, terminal input is decoded with a fatal UTF-8 decoder so a malformed file or stdin is refused rather than sent with U+FFFD replacements, and the prompt-only flag error names --text-file.
85237e4 to
22f58c0
Compare
|
Follow-up on the CodeRabbit outside-diff comment for
A regression test in |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run reviewed the delta against the prior pullfrog review at 85237e4: a fix to the shared bounded reader's byte handling plus strict UTF-8 validation on the file/stdin input path.
- Fixed
Uint8Arraychunk handling —readStreamBytesWithinLimitnow wraps plainUint8Arraychunks withBuffer.frominstead of stringifying them, so stdin/file bytes decode correctly rather than into decimal lists like"226,130,172". - Split the shared reader into bytes/text layers —
readStreamBytesWithinLimitreturns aBuffer;readTextStreamWithinLimit(lossy, unchanged forartifacts) andreadStdinBytesWithinLimitbuild on it. - Rejected invalid UTF-8 at the source —
decodeTerminalSendTextusesTextDecoder('utf-8', { fatal: true }), so a non-UTF-8 file or stdin is refused instead of silently sent with U+FFFD replacement characters, including split multi-byte sequences. - Corrected the prompt-option message — the
--retry-request/--wait-submitpreflight now names--text or --text-file. - Extended coverage — new
bounded-stdin-text.test.tsplus tests for invalid-UTF-8 file/stdin refusal, split multi-byte characters, and the updated flag-requirement message.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
Closing. On v1.4.200 a three-line |
1 participant
ELI5
Coordinators keep losing multi-line briefings when sending them to agent terminals: shell quoting mangles or truncates what
--textcarries. Now the message can come from a file (--text-file <path>) or from stdin (--text-file -), byte-identical to what you wrote.What Changed
terminal sendgains--text-file <path|->: UTF-8 file input,-reads stdin. Mutually exclusive with--text(explicit error), and an explicitly passed empty file/stdin is rejected with the path named — an explicit flag must not degrade into a bare Enter send.new TextDecoder('utf-8', { fatal: true }), so a Latin-1 or UTF-16 file (easy to produce on Windows) fails withinvalid_argumentand the path named instead of reaching the agent with U+FFFD replacement characters. Sending a prompt that silently differs from the file is worse than refusing it.--retry-request/--wait-submitnow report that they require--textor--text-file; the old message named only--texteven though a file-sourced prompt satisfies it.src/cli/terminal-send-text-source.tsresolves the send text from--textor--text-file.terminalSendHandlercalls it as its first step, so the payload is fully materialized before prompt candidacy, the prompt-delivery capability preflight, and the--retry-requestcheck. That ordering matters: the retry ID is bound to the prompt payload, and--retry-request/--wait-submitare only legal for a prompt candidate — a file-sourced prompt has to count as one, and an unreadable file must fail before any host contact.--enter(and not--interrupt) is classified exactly like--text.assertTerminalInputWithinLimit/TERMINAL_INPUT_MAX_BYTES(no new limit invented). An oversized file is refused from itsstatsize, before a read stream is opened.artifacts createwas extracted tosrc/cli/bounded-stdin-text.tsand reused; behavior and error message there are unchanged. It now exposes a byte-levelreadStreamBytesWithinLimit(so terminal input can be decoded strictly) and keeps a plainUint8Arraychunk as bytes rather than stringifying it into"226,130,172".src/cli/specs/terminal-send.ts: usage becomes[--text <text> | --text-file <path|->],text-filejoinsallowedFlags, and a note documents the file/stdin form and its refusals, including the non-UTF-8 one.Why
Multi-line dispatch briefings through
--textare quoting-hostile on every shell (worst on Windows), and a truncated briefing is one of the feeders of the paste-without-submit failures coordinators then have to untangle. Reading from a file/stdin removes the quoting layer entirely. Stdin precedent already existed in-tree for artifacts; this promotes it to a shared, concretely-named module.Rebased onto current
mainafter #16904 moved the send handler tosrc/cli/handlers/terminal-send.tsand its spec tosrc/cli/specs/terminal-send.ts. The new coverage lives in its ownsrc/cli/handlers/terminal-send-text-file.test.tsrather than growingterminal.test.tspast the 800-line test budget.Linked Issue
Fixes #14915
Visual Proof
N/A — CLI flag with no UI surface; behavior is covered by the tests listed below (file, stdin, exclusivity, missing file, empty file, size limit, agent-prompt classification, retry-request binding, help/usage).
Testing
Windows 11, Node 24.19.0, pnpm 12 (corepack). No live end-to-end run against a packaged build, so the manual box stays unchecked.
pnpm tc:cli— exit 0pnpm tc:node— exit 0vitest run src/cli/bounded-stdin-text.test.ts src/cli/handlers/terminal-send-text-file.test.ts— 20 passed. 16 cover the CLI (multi-line UTF-8 file, stdin,--text+--text-fileconflict, missing file, empty file, empty stdin, oversized file refused beforecreateReadStream, oversized stdin, non-UTF-8 file and non-UTF-8 stdin both refused withterminal.sendnever called, a multi-byte character split across two stdin chunks still joining, agent-prompt classification through the capability preflight,--retry-requestbound to the file payload, unreadable file failing before any host contact, the--text or --text-fileflag message, help usage); 4 cover the bounded reader (Uint8Arraychunk bytes, byte-length limit, subarray view, string chunks)vitest run src/cli/handlers/terminal.test.ts src/cli/handlers/artifacts.test.ts— 36 passed (unchanged upstream send coverage plus artifacts, green after the stdin-reader extraction)vitest run src/cli— 1153 passed / 13 failed. The same 13 fail on a clean checkout of the base commit (9d29e6878e) on this machine:orchestration-mutation-recovery(3),orchestration-gate-cli(1),orchestration-timeout-cli(1),orchestration-worker-cli(2) andruntime/client-recovery(6), all asserting shell quoting of recovery commands. None touch this diff.ORCA_CODE_QUALITY_BASE=upstream/main pnpm run check:code-quality:changed—0 new finding(s) across 7 changed file(s)for native, type-aware and React Doctoroxlinton the 7 changed files — 0 findings;pnpm run check:max-lines-ratchet— OK, no new bypassesFull
pnpm lint/pnpm test/pnpm buildwere not run locally; CI covers them.AI Disclosure
Implemented by a Codex lane under my orchestration (strict TDD); rebased, re-scoped to the new file layout, and re-verified locally with Claude Code assistance.
Review
resolve(cwd, path)for the file path, never a hand-built separator; stdin is read as a stream with no tty assumption; no shell is involved, which is the point of the flag on Windows.textfield and the sameagentPrompt/waitSubmitMsshape; only where the CLI sources the string changed. The file is read on the client, which is correct — the path the user types is a client path.--text-fileis additive. Existing--textcallers take an identical code path, and the old-host and legacy-prompt branches are untouched.statsize without streaming it into memory.artifacts create.Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)Author
X: @EnricoPin