perf: compact common test output - #846
Conversation
WalkthroughCommand output reduction now uses a registry for Go, Cargo, pytest, and JavaScript test commands. Shared logic removes repeated lines while preserving decisive summaries and raw output artifacts. Tests cover parsing, failures, shell syntax, stderr merging, metadata, tool integration, and token metrics. ChangesCommand Output Reduction
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ShellCommand
participant CommandReducerRegistry
participant OutputReducer
participant RawSpillArtifact
ShellCommand->>CommandReducerRegistry: classify command
CommandReducerRegistry->>OutputReducer: dispatch format reducer
OutputReducer->>RawSpillArtifact: preserve complete output
OutputReducer-->>ShellCommand: return reduced output and omission summary
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tools/shell_output_reducer_test.go`:
- Around line 155-178: Update
TestReduceCommandOutputCompactsRecognizedTestAndBuildNoise in
internal/tools/shell_output_reducer_test.go:155-178 to iterate over the complete
commandReducerCorpus(), including go_test, and continue requiring
command_output_reduced == "true". Update the aggregate metrics assertion in
internal/tools/shell_output_reducer_metrics_test.go:68-104 to fail unless
aggregate model-visible tokens are lower than aggregate raw tokens.
- Around line 205-207: Extend the reduced failure-output test after the
command_output_reduced metadata check to read the raw artifact referenced by the
reducer result, then compare its contents with original. Keep the existing
metadata assertion and ensure the test fails if the artifact path is missing,
unreadable, or contains altered output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4fd3bfb8-ad29-415d-b5b1-b754a838414f
📒 Files selected for processing (3)
internal/tools/shell_output_reducer.gointernal/tools/shell_output_reducer_metrics_test.gointernal/tools/shell_output_reducer_test.go
|
@coderabbitai Both requested test gaps are addressed in the latest commit. Please review the latest commit and approve if everything looks good. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Latest commit ==='
git show -s --format='%H%n%s%n%b' HEAD
echo '=== Changed files in latest commit ==='
git diff-tree --no-commit-id --name-status -r HEAD
echo '=== Stat for PR changes against merge-base ==='
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
git diff --stat "$base"...HEAD
echo '=== Go file outlines ==='
for file in internal/tools/shell_output_reducer.go internal/tools/shell_output_reducer_test.go internal/tools/shell_output_reducer_metrics_test.go; do
if [ -f "$file" ]; then
echo "--- $file ---"
ast-grep outline "$file" --lang go --items all || true
fi
done
echo '=== Latest commit patch ==='
git show --format=fuller --find-renames --find-copies --stat --patch HEAD -- \
internal/tools/shell_output_reducer.go \
internal/tools/shell_output_reducer_test.go \
internal/tools/shell_output_reducer_metrics_test.goLength of output: 7413 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Reducer implementation ==='
nl -ba internal/tools/shell_output_reducer.go | sed -n '1,280p'
echo '=== Main reducer tests ==='
nl -ba internal/tools/shell_output_reducer_test.go | sed -n '1,360p'
echo '=== Metrics corpus test ==='
nl -ba internal/tools/shell_output_reducer_metrics_test.go | sed -n '1,220p'
echo '=== Reducer call sites and spill metadata handling ==='
rg -n -C 4 '\breduceCommandOutput\s*\(|\bspill_path\b|command_output_reduced' \
internal/tools --glob '*.go'
echo '=== Complete PR diff for reducer files ==='
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
git diff --check "$base"...HEAD
git diff --format= --unified=80 "$base"...HEAD -- \
internal/tools/shell_output_reducer.go \
internal/tools/shell_output_reducer_test.go \
internal/tools/shell_output_reducer_metrics_test.goLength of output: 1262 🏁 Script executed: #!/bin/bash
set -euo pipefail
show_numbered() {
local file="$1"
echo "=== ${file} ==="
awk '{printf "%6d %s\n", NR, $0}' "$file"
}
show_numbered internal/tools/shell_output_reducer.go
show_numbered internal/tools/shell_output_reducer_test.go
show_numbered internal/tools/shell_output_reducer_metrics_test.go
echo '=== Reducer call sites and spill metadata handling ==='
rg -n -C 4 '\breduceCommandOutput\s*\(|\bspill_path\b|command_output_reduced' \
internal/tools --glob '*.go'
echo '=== Whitespace errors in complete PR diff ==='
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
git diff --check "$base"...HEAD
echo '=== Complete PR diff for reducer files ==='
git diff --format= --unified=80 "$base"...HEAD -- \
internal/tools/shell_output_reducer.go \
internal/tools/shell_output_reducer_test.go \
internal/tools/shell_output_reducer_metrics_test.goLength of output: 77407
The latest tests cover the full reducer corpus, including I found no blocking issue in the reducer or its test coverage. The sandbox cannot execute repository tests, so this approval is based on static review. Approved. ✅ [approve] If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
The reducer logic here is good — I tried hard to break it and couldn't. Fifteen adversarial shell forms all reject correctly (cargo test 2>&1 | tee log, cargo test >log 2>&1, cargo test 2>&1 >log, cargo test 2>&1 2>&1, $(...), backticks, ;, &&, embedded newline); realistic pytest / jest / vitest / cargo test / cargo check output with failures loses no failure line, no coverage table, no [no test files], no exit status; CRLF is handled; and I confirmed spill recoverability end-to-end through the branch the real tools actually take (reduce, then applySelfManagedOutputBudget with the real exec_command at max_output_tokens: 200 and the real bash) — spill_path survives the second budget, is readable, and is byte-identical to the pre-reduction output.
What I'd like fixed before this merges is the test side, because the PR body's safety claims currently rest on tests that can't fail.
1. The compound-command pass-through test is vacuous. shell_output_reducer_test.go:221 — the fixture is 14 × PASS src/example.test.ts with no runner summary in it, so all six commands bail out on the summary guard inside reduceJavaScriptTestPassingLines and never reach the metachar guard at all. I replaced strings.ContainsAny(command, "|;&<>\n\r") (shell_output_reducer.go:73) with strings.ContainsAny(command, "\n\r") — deleting pipe/compound/redirect rejection outright — and the entire reducer suite stayed green. Give the fixture a real summary line (Test Suites: 14 passed, 14 total, or switch it to the cargo shape with test result: ok. ...) so the assertion exercises the guard it's named for. For what it's worth the guard does work today: with a real test result:line,cargo test --workspace | tee log.txt, ... && echo doneand... > log.txt` all come back unreduced.
2. The "authoritative runner summary" markers aren't authoritative. Each is a substring or line-prefix match anywhere in the output, and I reproduced a false positive for three of them:
shell_output_reducer.go:185—outputHasTrimmedPrefix(output, "Test Files ").npm testrunning a bespoke script that printsTest Files discovered: 14followed by 14✓ contract check … satisfiedlines: reduced=true, omitted=14, and the model sees onlyTest Files discovered: 14 / all checks done / [zero] 14 passing JavaScript test-file lines omitted. That's the whole payload gone. Sincenpm testruns whatever the package says, this marker is doing all the work. Vitest actually printsTest Files N passed (N)— requiringpassed (or a digit on the same line would close it.shell_output_reducer.go:153—outputHasTrimmedPrefix(output, "Finished "). Acargo buildwhose build script logsFinished generating bindingsarmscargo_buildon a failed build (error[E0308], exit 101) and drops all 14Compilinglines. Cargo's line isFinished \dev` profile [unoptimized + debuginfo] target(s) in 2.41s— gating ontarget(s) inorprofile ` would fix it.shell_output_reducer.go:163—strings.Contains(output, " passed")anywhere. A failing pytest run where the app under test printsloading fixtures .......... [ 50%]14× reduces those away. The=== N passed in Xs ===/short test summary infoline is the real marker.
None of these loses anything permanently — the spill has it — but "reduction requires an authoritative runner summary" isn't what the code does yet.
3. Test coverage gaps, all of them. Mutations that left the suite green:
- Recognition — 5 of the 11 accepted invocation forms are untested:
python -m pytest/python3 -m pytest(:111),npx jest|vitest(:118), barejest/vitest(:116),npm|pnpm|yarn run test(:123),npm|pnpm|yarn exec jest|vitest(:126). Onlynpm test,pnpm test,yarn test,pytest -q,cargo test,cargo check,go testare in the corpus — i.e. exactly the subset the "invoked directly or through npm, pnpm, and yarn" claim doesn't need. strings.Trim(marks, ".") == ""at:181→marks != "": green. That's the guard that keeps a failing file's..F.pytest progress line visible.- The JS prefixes at
:190loosened from"PASS "/"✓ "to"PASS"/"✓": green. omitted < commandReducerMinPassingLinesat:34→omitted < 1: green — the 12-line floor isn't pinned.result.Truncated = trueat:46deleted: green.if spillPath == "" { return result }at:40deleted: green. That one matters most — it's the "never reduce unless the artifact was written" invariant, and without it the notice readsexact output saved toand the dropped lines are simply gone. Worth a test that forces the spill to fail.TestReduceCommandOutputPreservesFailureEvidence(:181) coverscargo_testonly; pytest, javascript_test and go_test have no failure-preservation test.
4. The registry test doesn't hit the real branch. TestRegistryAppliesCommandReducerToExecAndBashTools (:266) registers commandReducerFixtureTool under the names exec_command/bash, but that fixture doesn't implement selfBudgeting while both real tools do (I checked: fixture false, exec_command true, bash true). So it runs applyRegistryOutputBudget + enforceOutputCeiling, not applySelfManagedOutputBudget. I verified the real branch by hand and it's fine, but the test as written doesn't prove it — the go test at :30 already does this correctly with NewExecCommandTool; copying that shape for one of the new reducers would cover it.
Smaller things, take or leave:
- vitest's
✓ src/skips.test.ts (8 tests | 3 skipped) 12msgets dropped, so per-file skip attribution disappears (the aggregate survives in theTestsline). That's the same category as the[no test files]carve-out on the go path — consider keeping ✓ lines that mention skipped/todo. Jest's slow-suite timingPASS src/a.test.ts (12.5 s)goes the same way. - The cargo/pytest/JS predicates match on
TrimSpace(line)while the go one deliberately requires column 0 (TestReduceGoTestPassingPackagesPreservesIndentedTestLogs). Cargo'stest x ... oklines are at column 0, socargo_testcould use the strict form and stop eating an indented lookalike from a test's own stdout under-- --nocapture.cargo_buildgenuinely needs the trim. - Not recognized, in case any are worth adding:
yarn jest/yarn vitest(yarn's usual way to run a local binary —yarn testandpnpm execare handled butyarn <bin>isn't),npm run test:unit,poetry run pytest,uv run pytest,cargo nextest run,cargo +nightly test,./node_modules/.bin/jest,bun test.
go build ./..., go vet ./internal/tools/ and go test ./internal/tools/ are all clean on my machine. Happy to approve once (1) and (2) are addressed — (3) and (4) I'd like too but won't hold the PR over them.
2 participants
Summary
2>&1stderr merge while conservatively bypassing pipes, compound commands, file redirection, unsupported commands, and unrecognized output shapesWhy
Zero previously applied semantic command reduction only to
go test. Other common test and build commands could place dozens of repetitive successful lines into model context even though their decisive summaries were already present. Generic truncation can hide useful failures, so this change adds runner-aware reduction guarded by authoritative summary markers and retains the complete redacted output as an artifact.Measured impact
Deterministic 24-package/test-file corpus processed through the same command-output boundary used by
exec_commandandbash:This is a focused reducer microbenchmark, not a claim about total end-to-end session cost.
Safety
exec_commandandbashuse the same registry boundaryVerification
make fmt-checkgo vet ./...go test ./...go test -race ./internal/toolscoveragego run ./cmd/zero-release buildgo run ./cmd/zero-release smokemake lint-staticmake vulncheckgit diff HEAD --checkcargo test 2>&1result, displayed the exact-output artifact path, and retained the correct test summarySummary by CodeRabbit