Repository navigation
fix(core): cap the stderr a stdout-only filter forwards on a clean run - #4120
Conversation
Forwarding stderr verbatim keeps a golangci-lint config error from vanishing, but it also hands back every `go: downloading …` line a cold module cache produces: a passing `go test` went from 3220 raw bytes to 3216 through rtk, where the stdout filter alone returns 24. Stderr still goes through whole whenever it is the report -- the command failed, or the filter had nothing to show for stdout -- and now also whenever capping would not pay: below the recovery store's floor, when there is no store to point at, and whenever the note and the hint would cost more than the lines they replace. Otherwise the last lines are kept and the rest goes behind the `[full output: …]` handle, which puts that same `go test` at 354 bytes with its warnings intact. The end, not the head: a tool resolves and downloads before it builds, so a cap on the head keeps the chatter and drops the two lines worth forwarding. The kept lines are sliced out of the original rather than re-joined, so CRLF endings survive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📊 Automated PR Analysis
SummaryFixes a regression in rtk's stdout-only filter mode where stderr was forwarded verbatim on every run, letting chatty-but-harmless stderr (e.g. Review Checklist
Analyzed automatically by wshm · This is an automated analysis, not a human review. |
pszymkowiak
left a comment
There was a problem hiding this comment.
Reviewed by building the branch and driving src/core/runner.rs with stub go / golangci-lint binaries on PATH (150 go: downloading … lines on stderr, one ok line on stdout), isolated HOME and DB.
Verified
- develop:
rtk go test ./...→ 6816 B out of 6820 B native (0.06 % reduction), stderr forwarded verbatim. - this PR: 565 B (92 % reduction):
... (+140 earlier stderr lines not shown), the last 10 lines kept,[full output: rtk recall <hash>];rtk recall <hash>returns all 150 lines. Off-by-one checked (150 = 140 + 10, kept lines are 141–150). - Whole-forward cases all hold: exit ≠ 0 (6792 B, no note, exit 1 propagated); empty stdout (golangci config-error shape, 938 B whole, exit 7 propagated);
RTK_TEE=0andRTK_RECALL=0(whole, zeroearlier stderr/full outputstrings); 11 lines = 486 B below the 500 B floor → whole and nothing stored; 12 lines = 540 B → capped and stored. ANSI in the tail survives. - Store side effects only after the floor and line-count checks, matching the
curl_cmdconvention.
Follow-ups, none blocking
- Every clean chatty run now writes a recovery entry (one sqlite row, or one of
tee_max_files= 20 in legacy tee mode). 20 cleango testruns in tee mode evict every earlier failure log. Same trade-offcurl_cmd/tsc_cmdalready make, so not new policy, but the success path could require a minimum saving (a 12-line stderr is stored for an 18 B gain). - Legacy tee mode with
tee_on_successwrites two files per clean run (<epoch>_go_test.logand<epoch>_go_test-stderr.log) for content the first already holds. Non-default config, bounded. - The CRLF-preservation rationale in the doc comment is unreachable:
stream.rs::read_lines_lossystrips\rbefore the runner sees stderr (verified: 60 CRLF lines in → 0 CR out). The byte-offset slice is still the cheaper implementation; only the comment overstates why.
Approving. CI green on all three OSes.
Fixes one regression from the release review on #3979, introduced by #3772.
Part of a set of six, one per originating PR: #3681, #3552, #3265, #3772 (this), #3857, #3941.
The regression
RunOptions::stdout_only()is documented as "stdout-only to filter, stderr passthrough", and #3772 implemented the passthrough half, which had never existed — before it, a tool reporting on stderr with an empty stdout (golangci-lint with a config or build error) produced no output at all. That part was right and stays.But it forwarded stderr verbatim on every stdout-only path, so a chatty-but-harmless stderr swamped the filtered stdout. With a stub
goemitting 150go: downloading …lines and oneok …line:go test ./...developThe fix
Stderr still goes through whole whenever it is the report:
exit_code != 0), orThose are the cases the forwarding exists for, and they cover golangci-lint.
Otherwise it is capped — and three things make it give up and forward the lot anyway:
RTK_TEE=0, disabled, unwritable)curl_cmddeclines the same wayThat last one matters: a line-count cap with no byte floor made rtk emit +82 % on an 11-line stderr.
The cap keeps the end, not the head
A tool resolves and downloads before it builds, so its chatter comes first and anything it has to say comes last. A cap on the head kept
go: downloading module-1 … module-10and dropped exactly the two lines worth forwarding:Both now survive. The kept lines are sliced out of the original at a byte offset rather than re-joined, so CRLF endings are preserved —
str::lines()drops the\rand nothing puts it back.What the cap holds back goes behind the
[full output: …]handle, assrc/cmds/README.mdrequires of any "+N more".Result
Verification
11 unit tests in a new
forwarded_stderr_testsmodule: the tail cap and its note, the failing-run and empty-stdout whole-forward cases, the recovery-floor and no-store and would-not-shrink refusals, CRLF preservation, a non-ASCII stderr sliced without panicking, singular/plural, andlast_lines_offset's boundaries (unterminated last line, nothing to skip, empty).cargo fmt --all,cargo clippy --all-targetsandcargo test --allare green, rebased on currentdevelop.🤖 Generated with Claude Code