Repository navigation
fix: validate pane send-text against the canonical line limit - #119
Conversation
pane.send_text skipped the input preflight that pane.send_input runs. On macOS a canonical-mode line past MAX_CANON is dropped without an error, so a long send-text reported success and delivered nothing. Run the same validation and return pane_send_failed. Add a macOS regression test and a real-pty test that delivers over 1 MiB to a late raw-mode reader. Session-Id: e75a3fb2-2ceb-4fc5-aa83-182e88573cd6 Model: claude-opus-5-5 Harness: claude-code 2.1.289 Machine: cf6e768835c7 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 7 billable files and costs up to $1.75.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 23 minutes for your next included review. View limit details
Comment |
cameronsjo
left a comment
There was a problem hiding this comment.
Spec-compliance verdict: PASS against the stated requirement (pane.send_text runs the same validate_input_submission preflight as pane.send_input and returns pane_send_failed instead of silent success): src/app/api/panes.rs:1935 mirrors src/app/api/panes.rs:1964, placed before try_send_bytes, same error code, same err.to_string() surfacing. Note the PR does not fix the originally reported symptom (truncation in a Claude Code pane); the PR body says that was not reproduced.
Plan-adherence sub-verdict: no governing plan found (no matching file in docs/plans/ for branch fix/send-text-truncation, no OpenSpec change for it). The diff is multi-file and changes API behavior, so it is non-trivial: absent plan graded Important (advisory). No deviations to grade. Fork rule check: PR targets cameronsjo/herdr, adds a docs/fork/CHANGES.md entry (docs/fork/CHANGES.md:233), commit type fix: is valid.
Escalation: no manifest, auth, crypto, secret, or untrusted-input-to-sink surface touched; no security review needed. The change only adds a refusal path.
Checks run: read the full diff, src/platform/macos/input.rs (validator and line counter), src/platform/mod.rs:532-548 (non-macOS guard), src/pty/actor/unix.rs (handle, test imports), src/api/schema/panes.rs:361, src/api.rs:1231. Did not build or run tests (the macOS test is not run by PR CI; a native build here needs the Zig prefetch and writes to the worktree).
Critical
None.
Important
-
Stale user-facing contract:
docs/next/website/src/content/docs/socket-api.mdx:143(confidence: high). It says that on macOS onlypane.send_input(used bypane run) andagent.startrun the canonical-limit check, and that multiple short lines and raw-mode input stay supported. After this PRpane.send_textis also checked, so a CLI user who getspane_send_failedfrompane send-textwill find docs that say it is not covered. Update that paragraph, and mention the limit nearpane send-textindocs/next/website/src/content/docs/cli-reference.mdx:265. Check whether the localized docs trees carry copies of the socket-api paragraph. -
No test pins the non-refusal side, which is exactly the false-refusal risk (
src/platform/macos/input_tests.rs:195-230; confidence: medium). The new macOS test only covers "oversized canonical line is rejected" and "a shortretry\nstill goes through". Nothing asserts that (a) a 3000-byte line sent to a pane with ICANON off (set_canonical(.., false)) is accepted and arrives, or (b) a multi-line canonical payload where every line is under 1024 bytes but the total is over it is accepted. By readinglongest_canonical_line(src/platform/macos/input.rs:62-143) both pass today: the check returns early when ICANON is off (input.rs:41) and the line length resets at each delimiter (input.rs:138-140). So raw-mode panes (Claude Code, readline/ZLE shells at the prompt) and multi-line paste with short lines are not wrongly refused. But that holds only through the existingsend_inputtests; send_text has its own call site and no coverage of these cases. Add both cases to the new test; they are cheap with the existing pty helpers. The test has not run in any CI at this head, so it is unverified. From reading, it compiles (closure takes&mut App,PaneSendTextParamshas onlypane_idandtext,Method::PaneSendTextexists atsrc/api/schema.rs:129) and should pass (a 3000-byte unterminated line gives a 3000 > 1024 refusal whose message contains "canonical mode"; theretry\nread matches the pattern atinput_tests.rs:174-183). Treat that as unconfirmed until run on macOS. -
No governing plan or OpenSpec change for a behavior change to an API method (see plan-adherence line; confidence: low-medium). The fork keeps API specs in OpenSpec (issue #112); the
send_input/agent.startcanonical rejection is the closest existing contract, and this widens it.
Nit
-
Error advice is shaped for a shell (
src/platform/macos/input.rs:54, reached viasrc/app/api/panes.rs:1936; confidence: medium). The text says "wait for the shell prompt and retry". Forsend-textthe target is often a non-shell canonical reader (cat, a program callingread, an agent still starting), where waiting does not change the mode and a retry fails the same way. The message does not say the fix may be to split the text into lines under the limit. A send-text-specific suffix would be clearer to the CLI user. The error itself is surfaced (codepane_send_failed, message intact), so this is wording only. -
Residual silent drop (confidence: medium). The mode is sampled at admission (
input.rs:26-28says so), but the write happens later in the actor queue. A pane that flips from raw to canonical between admission and write (a program starting or exiting) can still drop an over-limit line with no error. Thedocs/fork/CHANGES.md:233heading ("refuses ... the macOS tty would drop") reads as a full guarantee; a one-line caveat matching the socket-api wording would keep it honest. -
Linux test cannot fail fast (
src/pty/actor/unix.rs:1634-1699; confidence: medium). Withcfmakeraw(VMIN=1) the reader thread's blocking read hangs forever if the actor stalls, so a regression shows as a hang until the runner's timeout rather than an assertion. A poll with a deadline, orVMIN=0/VTIMEas the macOS tests use, would turn that into a failure. Also the PR body says this test passes without the fix, yetdocs/fork/CHANGES.md:249lists it under "Regression checks"; it is a characterization test for the unchanged raw-mode path, not a regression pin for this fix. Say so in the entry so an upstream sync does not read it as covering the new check. -
docs/fork/CHANGES.md:233cites "a 622 KB send-text through a throwaway server" as evidence; no script or test in the repo reproduces it. Fine as a record, but not verifiable from the diff.
Update the socket API and CLI reference for the macOS canonical-line check on pane.send_text, extend the macOS regression test with multi-line and raw-mode acceptance cases, let the Linux delivery test time out instead of hanging, and point the error at splitting the text. Record in the fork ledger that the Linux test is a characterization test and that the mode is sampled at admission. Session-Id: e75a3fb2-2ceb-4fc5-aa83-182e88573cd6 Model: claude-opus-5-5 Harness: claude-code 2.1.289 Machine: cf6e768835c7 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cameronsjo
left a comment
There was a problem hiding this comment.
Spec-compliance verdict: PASS against the requirement (pane.send_text runs the same canonical-limit preflight as pane.send_input and returns pane_send_failed instead of silent success): src/app/api/panes.rs:1935-1937 runs validate_input_submission before try_send_bytes. Round 2 touched no handler logic.
Plan-adherence sub-verdict: no governing plan found (no docs/plans/ file for this branch). No deviations to grade. Same absence as round 1; see Important 1.
Escalation: none. No manifest, auth, crypto, secret, or untrusted-input-to-sink surface in the round-2 commit; the change only adds a refusal path and docs.
Round-1 resolution check:
- R1 Important 1 (stale docs): RESOLVED in
docs/next.docs/next/website/src/content/docs/socket-api.mdx:143now listspane.send_text;docs/next/website/src/content/docs/cli-reference.mdx:278adds the limit note. The localized treesdocs/preview/website/...:141anddocs/versions/0.9.4,0.9.5still carry the old paragraph; correct, since the repo rules say preview is CI-owned and published versions describe released behavior only. - R1 Important 2 (missing non-refusal tests): RESOLVED in source.
src/platform/macos/input_tests.rs:230-257adds the multi-line short-line accept and the ICANON-off 3000-byte accept. Read-through below. - R1 Important 3 (no plan/OpenSpec): worker's claim that no existing canonical-rejection requirement exists is TRUE.
openspec/specs/holds agent-registry, agent-sidebar, agent-skill, codex-lifecycle, command-palette, fork-maintenance, labels, layout-moves, server-status, workspace-api; a grep for canonical, MAX_CANON, send_text, send-text, send_input acrossopenspec/matches only "canonical id" and "canonicalization" lines inworkspace-api/spec.md. So there is no spec to widen. The plan-absent grade stands (Important 1). - R1 Nits: error wording (
src/platform/macos/input.rs:54now adds "or split the text into shorter lines"), admission-time caveat (docs/fork/CHANGES.md:239-241), Linux test timeout (src/pty/actor/unix.rs:1661-1663) and "characterization test" labeling (docs/fork/CHANGES.md:249-251) are all addressed.
Checks run: read the full round-2 diff and src/platform/macos/input_tests.rs in full; grepped src, tests, docs, openspec for the old error text. Old error text: the only hits are the reworded line in input.rs:54 and the docs paragraphs; no test asserts the old string (tests assert contains("canonical mode") at input_tests.rs:156-159 and :222-225, which the new text still satisfies). Not run: rustfmt/cargo (not installed in this environment), and the macOS tests (not runnable here or in PR CI).
Compile read-through of input_tests.rs:230-257 (not compiled):
set_canonical(&<tty follower file>, false): signaturefn set_canonical(&std::fs::File, bool)at line 36; the follower handle is aFile. OK. It clears ICANON and ECHO, sets VMIN=0/VTIME=20 (:43-48), the same helper the existing raw-mode case uses at:288.- The
send_textclosure takes(&mut App, &str);&multi(&String) and&longcoerce at the call, same as the existing&"x".repeat(3000).assert_eq!(received, multi.as_bytes())comparesVec<u8>with&[u8], which has aPartialEqimpl.read_line(...)returnsVec<u8>;append(&mut chunk)needschunkmutable and it is. No new imports are needed. - Formatting: the
format!call at:231-236is split one argument per line, which matches rustfmt's width heuristic for that call. Low risk. Runcargo fmtbefore merge anyway, per the repo's rules. - Assertions on Darwin: the ICANON-off case follows the same path as the existing raw test (
input_tests.rs:288-296), so the accept side should hold. The multi-line case passes the preflight by line length (each line is 601 bytes with its newline, under 1024).
Critical
None.
Important
None. (Orchestrator: the one advisory item, no spec for the canonical admission check, is answered in the PR body: the fork ledger in docs/fork/CHANGES.md is the spec for this area, and no OpenSpec requirement exists to extend.)
Nit
-
Possible Darwin flow-control dependence in the multi-line case (
src/platform/macos/input_tests.rs:237-245; confidence: low). The three lines are 1803 bytes in total and are queued before any read. The test passes only if the pty master write path blocks (the actor retries onWouldBlock) once the line discipline holds about 1024 bytes of pending lines, instead of dropping the overflow. The existing multi-line test (:353-362) reads concurrently from a thread, so it does not prove that case. If this macOS test fails on the first master push, that is the first place to look. A reader thread, as incanonical_multiline_submission_preserves_each_line, removes the dependence. -
Misleading failure message in the Linux test (
src/pty/actor/unix.rs:1688; confidence: medium). With VMIN=0/VTIME=30 a 3 s stall now returnscount == 0, and the assertion says "pty closed after N bytes". That reads like a pty hang-up, not a timeout. The test is a hang-to-failure improvement overall, but "read timed out or pty closed" would point at the real cause. The 3 s deadline can also flake on a heavily loaded runner. -
CLI doc advice is odd (
docs/next/website/src/content/docs/cli-reference.mdx:278; confidence: low). "wait for a raw-mode prompt" is not something a CLI user can observe, and waiting does not change the mode for a non-shell canonical reader such ascat. "Split the text into shorter lines" is the actionable half; consider dropping the wait advice here. -
The shared error text now says "split the text" for
agent.starttoo (src/platform/macos/input.rs:54; confidence: low). Agent arguments cannot contain LF (seeinput_tests.rs:130), so splitting is not an option there. Wording only.
…fork-docs docs/fork/CHANGES.md conflict only: this branch and master (#119) each added a new section at the same spot. Kept both whole, this branch's section first because the ledger is newest first. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016MXBYW3meTL95gcAeo2HFm
…on-edit docs/fork/CHANGES.md conflict only: this branch and master (#115, #119) each added a new section at the same spot. Kept all three whole, this branch's section first because the ledger is newest first. AGENTS.md merged without a conflict. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016MXBYW3meTL95gcAeo2HFm
…guard docs/fork/CHANGES.md conflict only: this branch and master (#115, #116, #119) each added a new section at the same spot. Kept all of them whole, this branch's section first because the ledger is newest first. This branch's edit to the #74/#36 entry merged without a conflict. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016MXBYW3meTL95gcAeo2HFm
…autolocalize docs/fork/CHANGES.md conflict only: this branch and master (#115, #116, #117, #119) each added a new section at the same spot. Kept all of them whole, this branch's section first because the ledger is newest first. AGENTS.md merged without a conflict. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016MXBYW3meTL95gcAeo2HFm
…workspace-id docs/fork/CHANGES.md conflict only: this branch and master (#115, #116, #117, #118, #119) each added a new section at the same spot. Kept all of them whole, this branch's section first because the ledger is newest first. The retired note on the #81 entry merged without a conflict. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016MXBYW3meTL95gcAeo2HFm
What broke
herdr pane send-textwas reported to deliver a truncated message to a Claude Code pane. I could not reproduce truncation in the server's write path. In a throwaway server on its own socket, a 622 KB multi-paragraph send-text arrived byte-identical (cmpclean) in a raw-mode reader that started reading 3 seconds late. The actor's partial-write and WouldBlock handling is correct.What I did find:
pane.send_textskipped thevalidate_input_submissionpreflight thatpane.send_inputruns. On macOS the tty drops a canonical-mode line longer thanMAX_CANON(1024 bytes) with no error. In the throwaway server, a 3000-byte line sent tocat(canonical) reported success, andcatreceived 0 bytes even after a newline and EOF. That is a silent drop on a path that can look like a cut-off message.Fix
handle_pane_send_textnow runs the same preflight and returnspane_send_failed("input contains a N-byte line but the terminal is in canonical mode...") instead of reporting success.Verification
cargo fmt --checkpasses locally.send_text_rejects_oversized_canonical_input_instead_of_dropping_it(src/platform/macos/input_tests.rs, macOS only).actor_delivers_text_far_past_the_pty_buffer_to_a_slow_raw_reader(src/pty/actor/unix.rs): over 1 MiB of multibyte multi-paragraph text through a real pty to a reader that starts 300 ms late. It pins current raw-mode delivery; it passes without this fix.scripts/docker-check.shcould not finish: rustc was OOM-killed in the container (SIGKILL). CI on this PR is the proof. The macOS row runs only on pushes to master.Not reproduced
If truncation still shows up in Claude Code panes (raw mode), the cause is outside herdr's PTY write path.
send-textwrites raw bytes with no bracketed-paste wrapping, whilepane runwraps when the pane enabled bracketed paste. A paste-handling difference in the receiver is the next suspect.Session-Id: e75a3fb2-2ceb-4fc5-aa83-182e88573cd6
Model: claude-opus-5-5
Harness: claude-code 2.1.289
Machine: cf6e768835c7
Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com
Spec
No OpenSpec requirement covers the canonical admission check for
pane.send_input,agent.start, orpane.send_text. The fork ledger entry indocs/fork/CHANGES.mdis the spec for this area.🤖 Generated with Claude Code
https://claude.ai/code/session_01Hoi2iouMQo68rvVwKVpuXY