Repository navigation
fix(go): respect build failure exit status - #2188
Conversation
Gate go build success output on the child exit code and surface unrecognized non-zero build output instead of reporting success. Closes rtk-ai#2185.
pszymkowiak
left a comment
There was a problem hiding this comment.
Thanks for the thorough fix — the diagnose matches the repro in #2185 exactly, and the two-layer approach (allow-list entry for pattern + exit-aware fallback) is the right call. The runner.rs refactor (RunMode::FilteredWithExit + shared run_captured_filter) is a particularly valuable piece: it lays the plumbing so other ecosystems can opt into exit-aware filtering without duplicating logic. Several other filters (cargo clippy, mypy, ruff, tsc, eslint, golangci-lint, go vet) have the same latent "no errors detected = Success" pattern and would benefit from the same migration.
A second independent review (code-reviewer agent) and empirical sweep across 19 commands on develop vs this branch confirmed byte-identical behavior on every non-go-build path.
A few light suggestions for follow-up, none blocking:
-
ANSI escape codes in the failure fallback:
format_go_build_failureechoes raw stderr lines throughtruncatewithout passing throughstrip_ansi. Realgo builddoesn't emit ANSI, so this is a theoretical concern, but a malicious or unusualgowrapper could inject OSC 8 hyperlinks or alt-screen sequences that survive into the LLM context. Astrip_ansicall per line would close that gap cheaply. -
"pattern "prefix inis_go_build_error_line: matches any line starting withpattern(lowercased). A custom build tool printingpattern ./internal/...: 12 files matchedwould be classified as an error. Tightening to"pattern ./"or requiring": "after the argument would eliminate that risk while still matching every realgo buildpattern-error shape. -
Tests: the new tests use
assert!(result.contains(...))— consistent with the rest ofgo_cmd.rs, so just a style note. Adding edge-case tests forfilter_go_build_with_exit("", 1)and whitespace-only inputs would round out coverage. -
Follow-up opportunity (out of scope for this PR): migrating
cargo clippy,mypy,ruff,tsc,eslint,golangci-lint, andgo vettorun_filtered_with_exitwould close the same class of bug across the rest of the codebase.
Approving — the fix is correct, the refactor is well-bounded, and behavior is preserved across every other command. Thanks for the careful work!
Closes #2185.
Summary
rtk go buildsuccess output on the wrapped process exit code.go buildoutput instead of reportingGo build: Success.pattern ./...: directory prefix ....Test plan
cargo fmt --all -- --checkcargo clippy --all-targets -- -D warningscargo test --all --quiettarget/debug/rtk go build ./...