fix(cargo): surface --message-format=json build errors instead of "compiled"

[luccinmasirika]

Summary

Fixes #2419. rtk cargo build --message-format=json (and cargo check --message-format=json) reported a failed build as cargo build (1 crates compiled) and dropped the compiler error — the filtered output was identical to a successful build except for the (easily-swallowed) exit code, and "compiled" reads as success.

Root cause

The build summary is produced by a line handler that keys on the human error[..] / warning: text. With --message-format=json, cargo emits those diagnostics as NDJSON ({"reason":"compiler-message","message":{"level":"error",...}}), which the handler never matches, so it counted zero errors and fell into the "compiled" branch.

Fix

extract_json_diagnostics() recovers the NDJSON compiler-message records from the raw stream and returns their rendered text for a given level. Both summary paths (the streaming CargoBuildHandler::format_summary and the batch filter_cargo_build) now fold those into the error/warning counts and print the rendered diagnostics. The streaming path also uses the exit code as a backstop, so a non-zero build is never summarized as compiled. Non-json output is unaffected — the helper returns nothing for it (mirrors the existing aborting due to / could not compile skips).

Before / After

$ rtk cargo build --message-format=json     # failing build
# before:
cargo build (1 crates compiled)             # ← looks like success, exit 101

# after:
error[E0308]: mismatched types
 --> src/main.rs:2:18
  |
2 |     let x: i32 = "nope";
  |            ---   ^^^^^^ expected `i32`, found `&str`
cargo build: 1 errors, 0 warnings (1 crates)   # exit 101

A passing json build is unchanged (cargo build (1 crates compiled) + Finished, exit 0).

Tests

• test_cargo_build_stream_json_failure_not_compiled (streaming) and test_filter_cargo_build_json_failure_not_compiled (batch): feed NDJSON error records and assert the error is surfaced, the count is right, and the output is not reported as "compiled".
• cargo fmt --all && cargo clippy --all-targets && cargo test — clippy clean, all cargo tests pass, no regressions.
• Verified end-to-end with the installed-style binary on a scratch crate (broken build → shows error[E0308] + exit 101; passing build → "compiled" + exit 0).

[hgunduzoglu]

Checked this out and tested it locally (macOS arm64) against a real failing build — works exactly as described, and the success/non-json paths are untouched:

Command	before	this PR
rtk cargo build --message-format=json (failing)	cargo build (1 crates compiled), exit 101	error[E0308]… + cargo build: 1 errors, 0 warnings, exit 101 ✓
rtk cargo build --message-format=json (passing)	1 crates compiled	1 crates compiled ✓ (no regression)
rtk cargo build (non-json, passing)	1 crates compiled	1 crates compiled ✓

cargo fmt/clippy/test clean, 51 cargo_cmd tests pass. The refactor into cargo_build_success_line / cargo_build_failure_summary is a nice dedup of the streaming and batch paths, and using the exit code as a backstop in format_summary is the right call — it means a non-zero build can never be summarized as compiled even if the diagnostic shape changes.

One non-blocking observation (token-savings angle): in cargo_build_failure_summary, the JSON diagnostics are emitted uncapped (for d in &json.errors { … }), whereas the human-text path in filter_cargo_build caps blocks at MAX_CHECK_BLOCKS = CAP_ERRORS. A --message-format=json build with many errors would therefore print every rendered diagnostic in full — which can get large, and is exactly what RTK normally bounds. Might be worth applying the same CAP_ERRORS cap (with a … N more style hint) to the JSON error list for consistency. Not a blocker — the correctness fix here is the important part.

Nice work, and good test coverage on both the streaming and batch paths.

[luccinmasirika]

Thanks a lot @hgunduzoglu for taking the time to test this end to end.

And good catch on the uncapped JSON diagnostics. You're right that it should match the human-text path, so I've capped the JSON errors at CAP_ERRORS (and warnings at CAP_WARNINGS) with a … +N more hint when it overflows. The summary line still reports the real totals, so nothing gets lost. Pushing it now.

error[E0308]: mismatched types
  --> src/main.rs:20:21
   |
20 |     let _x18: i32 = "err18";
   |               ---   ^^^^^^^ expected `i32`, found `&str`
   |               |
   |               expected due to this
error[E0308]: mismatched types
  --> src/main.rs:21:21
   |
21 |     let _x19: i32 = "err19";
   |               ---   ^^^^^^^ expected `i32`, found `&str`
   |               |
   |               expected due to this
… +5 more errors
cargo build: 25 errors, 0 warnings (1 crates)

[luccinmasirika]

@hgunduzoglu quick follow-up on your point about capping the JSON diagnostics.

While wiring that cap in, I went digging and found something bigger. The human-text path isn't actually capped on the real command path either. MAX_CHECK_BLOCKS lives in filter_cargo_build, which is only the batch helper. But rtk cargo build / cargo check run through the streaming path (BlockStreamFilter in core/stream.rs), and feed_line emits every error block live with no cap. There's even a blocks_emitted counter in BlockStreamFilter that gets incremented but never read, so it looks like the cap was meant to be wired there and never was.

I reproduced it on a 25-error crate: plain rtk cargo build prints all 25 full diagnostic blocks (178 lines, no +N more hint), while the json path now correctly caps at 20. So the default, most common path is the one still flooding the output.

I'd like to fix it in a follow-up: wire the cap into BlockStreamFilter (reusing that dead blocks_emitted counter) so build, check and test all bound their output at CAP_ERRORS with a … +N more hint, matching the json and batch paths. The summary still reports the real total, and the full output stays in the tee log.

Okay if I pick this up?

[luccinmasirika]

Also added a token-savings test on the json failure path while I was in there, since cargo_cmd didn't have one. It locks in the reduction (~74% on a large failing build, the repo floor is 60%) so the cap can't silently regress later.

That wraps up the json side for me. The streaming cap is the separate follow-up I described above.

[hgunduzoglu]

Re-reviewed the updated branch (through d061c23) — this fully addresses my cap point. Pulled it and tested on a 25-error crate:

$ rtk cargo build --message-format=json     # 25 type errors
… (20 rendered diagnostics)
… +5 more errors
cargo build: 25 errors, 0 warnings (1 crates)    # exit 101

JSON errors cap at CAP_ERRORS, warnings at CAP_WARNINGS, each with a … +N more hint, and the summary still reports the true totals — exactly matching the human/batch path. cargo fmt --check, clippy --all-targets, and cargo test all clean (54 cargo_cmd tests, including the new token-savings assertion on the failure path and the success-path test — nice touch locking the ~74% reduction in so the cap can't silently regress). LGTM from my side.

On your follow-up — yes, please do, and as a separate PR. I reproduced exactly what you describe on the default path:

$ rtk cargo build          # no --message-format=json, 25 errors
# prints all 25 full E0308 blocks, no "+N more" hint

Confirmed blocks_emitted in core/stream.rs is declared, initialized, and incremented (self.blocks_emitted += 1) but never read — so the streaming cap was clearly intended and left unwired. Reusing it to bound build/check/test at CAP_ERRORS with the same … +N more hint (full output still in the tee log) is the right fix, and the default path is the higher-impact one since it's the common case. Keeping it out of this PR is the correct call — this one stays focused on the json regression. Happy to review that follow-up when it's up.

[KuSh]

Code Review — AI-assisted (Claude Sonnet 4.6 via Claude Code)

This review was generated using multi-agent AI analysis (8 finder angles × parallel verification). Findings are ranked by severity; each one survived a dedicated verify pass.

---

Overview

The fix is sound and addresses a real, impactful bug: CargoBuildHandler only matched human-readable error[/warning: prefixes, so --message-format=json builds always reported "compiled" regardless of outcome. The approach of parsing compiler-message NDJSON records at exit via extract_json_diagnostics() and merging into both paths is clean. The six new tests cover the core scenarios well.

Four issues surfaced that a maintainer should address before merge.

---

🔴 Finding 1 — cargo_build_failure_summary omits force_tee_hint (data loss)

When json.errors.len() > CAP_ERRORS, the overflow line "… +N more errors\n" is emitted but the dropped errors are silently discarded — no tee file is written and no recovery hint is shown to the user.

Every other overflow path in the file calls force_tee_hint:

// filter_cargo_build human path (line ~819) — DOES call it:
if let Some(hint) = crate::core::tee::force_tee_hint(&all_blocks, "cargo-check-issues") {
    result.push_str(&format!("  {}\n", hint));
}

// new cargo_build_failure_summary JSON path — MISSING:
if json.errors.len() > CAP_ERRORS {
    out.push_str(&format!("… +{} more errors\n", json.errors.len() - CAP_ERRORS));
    // ← no force_tee_hint here
}

force_tee_hint writes the full content to ~/.local/share/rtk/tee/ and returns a [full output: ~/path/to/file.log] hint so the user can retrieve the rest. Without it, errors beyond the cap are permanently lost for that session.

Fix: pass the overflowed diagnostics to force_tee_hint using a label like "cargo-json-errors", mirroring the human path.

---

🟡 Finding 2 — strip_ansi not called on rendered field

extract_json_diagnostics pushes msg["rendered"] verbatim into the output bucket:

bucket.push(rendered.trim_end().to_string());

The rendered field contains ANSI escape sequences when CARGO_TERM_COLOR=always is set (common in CI) or when --message-format=json-diagnostic-rendered-ansi is used. strip_ansi exists at src/core/utils.rs:48 and is called by every other filter module that handles compiler output.

Fix:

bucket.push(crate::core::utils::strip_ansi(rendered).trim_end().to_string());

---

🟡 Finding 3 — "N warnings generated" summary record not filtered

In JSON mode rustc emits a compiler-message record whose message field is "N warning(s) generated" alongside the actual warning diagnostics. extract_json_diagnostics only skips "aborting due to" and "could not compile" — not the "generated" summary. Result: json.warnings.len() is inflated by 1, the count in the summary line is wrong, and the noise text appears in the rendered output.

Human mode handles this correctly in should_skip:

if line.starts_with("warning:") && line.contains("generated") && line.contains("warning") {
    return true;
}

Fix: add a parallel guard in extract_json_diagnostics:

if text.starts_with("aborting due to")
    || text.starts_with("could not compile")
    || (text.contains("warning") && text.contains("generated"))
{
    continue;
}

---

🟡 Finding 4 — No explicit arg-based routing for JSON mode (altitude)

The fix detects JSON mode opportunistically by examining output content rather than inspecting the --message-format flag at call time. This means:

1. All three cargo JSON variants (json, json-diagnostic-rendered-ansi, json-render-diagnostics) are handled identically with no place to branch on variant-specific behavior (e.g. ANSI stripping for the -rendered-ansi variant).
2. The .max(handler.error_count, json.errors.len()) guard is the only thing preventing double-counting if both human and JSON diagnostics appear — safe today by coincidence, not by construction.
3. Two callers (CargoTestHandler::format_summary line 188, filter_cargo_nextest line 1041) check build_filtered.starts_with("cargo build:") before accepting the result. After this PR, cargo_build_failure_summary puts rendered diagnostics before the summary line, so this check silently falls to the "last 5 meaningful lines" fallback if filter_cargo_build ever receives JSON output. Currently guarded by has_compile_errors staying false in JSON mode — but that's a hidden invariant, not an enforced one.

PR #2750 (open, same issue) solved this with a focused has_json_message_format(args) that explicitly handles all three variants and their space-separated forms. Borrowing that function as a guard at the run_build/run_check level would:

• make JSON mode an explicit routing decision
• close the starts_with fragility above
• give a natural place to apply strip_ansi unconditionally on the JSON path
• replace the .max() defence-in-depth with mutual exclusion by construction

---

🟡 Finding 5 — starts_with("cargo build:") invariant is silently fragile

After this PR, cargo_build_failure_summary builds its output by placing rendered diagnostic text before the "cargo build: N errors..." summary line. Two callers rely on that prefix to accept the result:

// CargoTestHandler::format_summary, line ~188:
let build_filtered = filter_cargo_build(raw);
if build_filtered.starts_with("cargo build:") {         // ← fails if json.errors non-empty
    return Some(format!("{}\n", build_filtered.replacen("cargo build:", "cargo test:", 1)));
}
// silent fallback: last 5 meaningful lines (raw NDJSON in JSON mode)

The same guard appears in filter_cargo_nextest (~line 1041).

Today this is safe: has_compile_errors is only set when a human-readable error[ or error: prefix is detected, which never fires on {-starting NDJSON lines, so filter_cargo_build is never called with JSON output. But that safety is a hidden
invariant — the moment someone extends has_compile_errors to cover JSON errors (a natural next step), both callers silently fall back to dumping raw NDJSON to the LLM instead of a clean "cargo test: N errors" summary.

This is one of the two issues that explicit arg-based routing (Finding 4) would close by construction: with has_json_message_format(args) as a guard, the JSON and human paths are mutually exclusive at the routing level, so filter_cargo_build can never receive JSON output regardless of how has_compile_errors evolves.

---

⚪ Cleanup

Duplicate counting setup — the same 3-line block is copy-pasted into both CargoBuildHandler::format_summary and filter_cargo_build. When the .max() logic changes (e.g. to fix Finding 3), there are two places to update.

Triple allocation in cargo_build_success_line — three chained format! calls each allocate and discard the previous string. A single format! with a conditional covers both cases.

---

Review performed via Claude Code (claude-code, Sonnet 4.6) using 8 parallel finder angles + dedicated verifier per finding. Findings #1–3 are directly actionable; Finding #4 is the architectural note from comparing this PR with #2750.

[luccinmasirika]

Thanks for the thorough pass @KuSh, really helpful. Went through each one, all pushed on this branch:

Finding 1 (force_tee_hint): fixed (cf1caf9). The JSON failure path now writes the full diagnostics to the tee log and surfaces the [full output: ...] hint when the cap trips, matching the human path, so nothing beyond the cap is lost.

Finding 2 (strip_ansi): fixed (b47653a). extract_json_diagnostics runs the rendered field through strip_ansi before buffering, so CARGO_TERM_COLOR=always and the -rendered-ansi variant no longer leak escape codes.

Finding 3 ("N warnings generated"): fixed (9ae0872). Added the same guard the human should_skip uses, so the summary record no longer inflates json.warnings.len() or shows up in the output.

Cleanup: done (ff6cebd, 9f20821). Pulled the duplicated count merge into a shared helper and collapsed the triple format! in cargo_build_success_line into a single allocation.

Findings 4 and 5 (arg-based routing / starts_with fragility): done (5ea03f3). Added explicit has_json_message_format(args) detection; run_build and run_check now branch on it and route --message-format=json (and the json-* variants, inline or space-separated) to the batch filter_cargo_build path instead of inferring json mode from output content. Json routing is now an explicit decision rather than relying on the has_compile_errors invariant you flagged. Added 9 detection unit tests. Kept the .max() guard as belt-and-suspenders defence in depth.

@hgunduzoglu thanks for the two rounds of hands-on testing. The default (non-json) path cap you confirmed via the unwired blocks_emitted in core/stream.rs will go up as its own PR.

[luccinmasirika]

@KuSh Two out-of-scope items I hit while wiring the json routing, both pre-existing (not introduced here), flagging so they don't get lost:

• cargo test --message-format=json: on a compile error the test path (CargoTestHandler) isn't json-aware, so it prints nothing (only the exit code signals failure).
• cargo check --message-format=json: reuses the cargo build: summary wording via filter_cargo_build, so the label reads "build". Pre-existing (the non-json check path already reuses CargoBuildHandler), minor, follow-up.

Both sit outside this regression fix. Happy to open a follow-up for the test one if you agree.

[KuSh]

@KuSh Two out-of-scope items I hit while wiring the json routing, both pre-existing (not introduced here), flagging so they don't get lost
[...]
Both sit outside this regression fix. Happy to open a follow-up for the test one if you agree.

It would be great to extend this PR to handle cargo json format messages globally.

[luccinmasirika]

Thanks @KuSh. I've extended the PR so cargo json output is handled consistently:

• cargo check --message-format=json now reads cargo check: instead of cargo build: (also fixed on the non-json check path).
• cargo test --message-format=json printed nothing on a compile error; it now surfaces the diagnostics with a cargo test: summary.
• cargo clippy --message-format=json reported No issues found on failing runs; it now surfaces json errors and warnings.

[KuSh]

This seems unnecessary now that you check has_json_message_format, did I miss something?

[luccinmasirika]

Hey @KuSh,
I realized I didn't check everything carefully. I was trying to finish it all while having lunch today. Let me check everything again.

[KuSh]

This function, unlike run_cargo_streamed and run_build/run_check, doesn’t handle non-zero exit codes, which seems wrong

[KuSh]

it should take exit code and special case 0 errors, 0 warnings if exit != 0

[KuSh]

it seems to be missing Ice and also perhaps FailureNote, see https://docs.rs/cargo_metadata/latest/cargo_metadata/diagnostic/enum.DiagnosticLevel.html#variants
It could also be missing from the text filtering path and be handled later

[KuSh]

run_test doesn't handle json message formats

[luccinmasirika]

This one is handled by the streamed CargoTestHandler rather than a routing branch: --message-format=json only reformats the build phase, the test harness output stays human, so the handler aggregates the "test result:" lines and parses the json diagnostics in format_summary on a compile error (covered by test_cargo_test_stream_json_compile_error). Routing it through the batch path would change the failing-test output format, so I added a comment on run_test explaining the choice in 1143e6f. Happy to switch to an explicit branch if you'd rather.

[KuSh]

This line was always top before your change, this should stay that way

[KuSh]

Use join_with_overflow from utils

[KuSh]

Please take inspiration from filter_go_test_json for that function

[KuSh]

As there's only two call site for those wrappers, I'd recommend dropping them and directly call with_label("build")/with_label("check") there

[luccinmasirika]

Thanks @KuSh, all addressed. The streamed handler no longer parses json (it only sees non-json), the batch filter takes the exit code now so a failed build with nothing to parse says failed instead of compiled, and an ICE counts as an error. Also switched to typed structs like filter_go_test_json, put the summary line back on top, used join_with_overflow for the caps, and dropped the new()/for_check() wrappers. Replied on the run_test thread separately.

[pszymkowiak]

Reviewed and verified locally: cargo fmt clean, cargo clippy --all-targets zero warnings, full test suite green (2177 passed / 0 failed).

Confirmed the bug independently on current develop HEAD before applying this patch: rtk cargo build --message-format=json on a broken crate prints cargo build (1 crates compiled) — reads as success — while the real exit code is 101. An agent skimming the output (rather than checking the exit code) would wrongly conclude the build succeeded. This fix surfaces the actual error[E0308]... diagnostic and the correct cargo build: N errors, M warnings summary.

Verified end-to-end with a real broken crate (let x: i32 = "nope";):

• Before (unpatched): cargo build (1 crates compiled), exit 101 — misleading.
• After (this PR): error[E0308]: mismatched types + cargo build: 1 errors, 0 warnings (1 crates), exit 101 — correct.
• Passing build unaffected: cargo build (1 crates compiled) + Finished ..., exit 0 — no regression.

Also like the exit_code backstop in cargo_build_failure_summary (cargo build: failed (exit N) when no JSON diagnostics parse, e.g. a build-script failure) — closes the bug class, not just the compiler-message case. Good test coverage (20+ new tests) including that backstop (test_batch_filter_nonzero_exit_without_diagnostics_is_not_compiled).

LGTM 👍