fix(core): forward stderr from stdout-only filters on every exit (adopt upstream #3772)

src/core/runner.rs

@@ -2,6 +2,7 @@
 
 use anyhow::{Context, Result};
 use regex::Regex;
+use std::borrow::Cow;
 use std::process::Command;
 use std::sync::LazyLock;
 
@@ -125,15 +126,6 @@ where
         return Ok(exit_code);
     }
 
-    // `filter_stdout_only` keeps stderr out of the filter input, so nothing
-    // downstream can surface it — a failing child's diagnostics would be dropped
-    // and the exit code left as the only signal (#3026). Emit them here. Only on
-    // failure: on a successful run stderr is the incidental noise RTK exists to
-    // suppress, and the filtered stdout already carries the result.
-    if opts.filter_stdout_only && exit_code != 0 && !result.raw_stderr.trim().is_empty() {
-        eprint!("{}", result.raw_stderr);
-    }
-
     let text_to_filter = if opts.filter_stdout_only {
         raw_stdout
     } else {
@@ -147,6 +139,8 @@ where
         raw
     };
 
+    // stderr is forwarded unchanged below, so it costs the same whether or not rtk is
+    // in the way. What the guard must not inflate is the stdout it replaces.
     let shown = if let Some(label) = opts.tee_label {
         print_with_hint(&filtered, raw, raw_for_tracking, label, exit_code)
     } else {
@@ -159,12 +153,21 @@ where
         guarded
     };
 
-    timer.track(
-        cmd_label,
-        &format!("rtk {}", cmd_label),
-        raw_for_tracking,
-        &shown,
-    );
+    // Stdout-only filters parse structured stdout; stderr still carries diagnostics
+    // (config errors, missing linters) that the user needs.
+    let forwarded_stderr = opts.filter_stdout_only && !result.raw_stderr.trim().is_empty();
+    if forwarded_stderr {
+        eprint!("{}", result.raw_stderr);
+    }
+
+    // Forwarded stderr reaches the user just as much as stdout does, so counting only
+    // stdout would book a passed-through stream as if the filter had removed it.
+    let emitted = if forwarded_stderr {
+        Cow::Owned(format!("{}{}", shown, result.raw_stderr))
+    } else {
+        Cow::Borrowed(shown.as_str())
+    };
+    timer.track(cmd_label, &format!("rtk {}", cmd_label), raw, &emitted);
     Ok(exit_code)
 }
 

tests/stderr_only_failure_test.rs

@@ -0,0 +1,185 @@
+//! Stdout-only filters parse structured stdout, but a tool's diagnostics often go to
+//! stderr. Dropping that stream leaves a failing command looking silent; inventing a
+//! stdout message in its place is just as wrong. These pin both halves.
+
+#![cfg(unix)]
+
+use std::fs;
+use std::os::unix::fs::PermissionsExt;
+use std::path::Path;
+use std::process::{Command, Output};
+
+/// Writes an executable `name` in `dir` that runs `body`.
+fn fake_tool(dir: &Path, name: &str, body: &str) {
+    let path = dir.join(name);
+    fs::write(&path, format!("#!/bin/sh\n{}\n", body)).expect("write fake tool");
+    fs::set_permissions(&path, fs::Permissions::from_mode(0o755)).expect("chmod fake tool");
+}
+
+/// Runs rtk with `dir` first on PATH, so the fake tool shadows any real one.
+fn rtk_with(dir: &Path, args: &[&str]) -> Output {
+    let path = format!(
+        "{}:{}",
+        dir.display(),
+        std::env::var("PATH").unwrap_or_default()
+    );
+    Command::new(env!("CARGO_BIN_EXE_rtk"))
+        .args(args)
+        .env("PATH", path)
+        // Keep tracking off the developer's real database.
+        .env("RTK_DB_PATH", dir.join("rtk-test.db"))
+        .output()
+        .expect("run rtk")
+}
+
+/// A golangci-lint that answers `--version`, then fails the way a config or build
+/// error does: everything on stderr, stdout empty, non-zero exit.
+fn fake_golangci(dir: &Path, failure: &str) {
+    fake_tool(
+        dir,
+        "golangci-lint",
+        &format!(
+            r#"if [ "$1" = "--version" ]; then
+  echo "golangci-lint has version 1.64.8"
+  exit 0
+fi
+{failure}"#
+        ),
+    );
+}
+
+#[test]
+fn golangci_lint_stderr_only_failure_reaches_the_user() {
+    let dir = tempfile::tempdir().expect("tempdir");
+    fake_golangci(
+        dir.path(),
+        r#"echo 'level=error msg="context loading failed: failed to load packages"' >&2
+exit 3"#,
+    );
+
+    let out = rtk_with(dir.path(), &["golangci-lint", "run"]);
+    let stderr = String::from_utf8_lossy(&out.stderr);
+
+    assert!(
+        stderr.contains("context loading failed"),
+        "the tool's own error must reach the user; got stderr: {stderr}"
+    );
+    assert_eq!(out.status.code(), Some(3), "exit code must propagate");
+}
+
+/// The command said nothing on stdout, so rtk must not claim a clean run there. A
+/// filter's "I got nothing" placeholder is not a diagnostic — the real one is on
+/// stderr. This fork's prettier filter reads that stderr report itself (fork PR
+/// #117, upstream issue #2878) and summarises it on stdout, so the pin here is that the
+/// report reaches the user and no success is invented, not which stream carries it.
+#[test]
+fn no_invented_success_when_the_tool_reported_on_stderr() {
+    let dir = tempfile::tempdir().expect("tempdir");
+    fake_tool(
+        dir.path(),
+        "prettier",
+        r#"echo "Checking formatting..." >&2
+echo "[warn] src/a.js" >&2
+echo "Code style issues found in the above file(s)." >&2
+exit 1"#,
+    );
+
+    let out = rtk_with(dir.path(), &["prettier", "--check", "."]);
+    let stdout = String::from_utf8_lossy(&out.stdout);
+    let stderr = String::from_utf8_lossy(&out.stderr);
+
+    let combined = format!("{stdout}{stderr}");
+    assert!(
+        combined.contains("src/a.js"),
+        "prettier's report must reach the user; got stdout: {stdout} stderr: {stderr}"
+    );
+    assert!(
+        !stdout.contains("No errors") && !stdout.contains("formatted"),
+        "a failing check must not be reported as success; got stdout: {stdout}"
+    );
+    assert_eq!(out.status.code(), Some(1), "exit code must propagate");
+}
+
+/// stderr goes out on its own stream, so it must not also be replayed on stdout.
+#[test]
+fn stderr_is_not_duplicated_onto_stdout() {
+    let dir = tempfile::tempdir().expect("tempdir");
+    fake_tool(
+        dir.path(),
+        "ruff",
+        r#"echo "stdout line"
+echo "stderr diagnostic" >&2
+exit 1"#,
+    );
+
+    let out = rtk_with(dir.path(), &["ruff", "check", "."]);
+    let stdout = String::from_utf8_lossy(&out.stdout);
+    let stderr = String::from_utf8_lossy(&out.stderr);
+
+    assert!(
+        !stdout.contains("stderr diagnostic"),
+        "stderr must not be replayed on stdout; got stdout: {stdout}"
+    );
+    assert!(
+        stderr.contains("stderr diagnostic"),
+        "stderr must still reach the user; got stderr: {stderr}"
+    );
+}
+
+/// A command that genuinely says nothing must stay silent.
+#[test]
+fn silent_command_stays_silent() {
+    let dir = tempfile::tempdir().expect("tempdir");
+    fake_tool(dir.path(), "ruff", "exit 0");
+
+    let out = rtk_with(dir.path(), &["ruff", "check", "."]);
+
+    assert!(
+        String::from_utf8_lossy(&out.stdout).trim().is_empty(),
+        "a silent command must not gain output"
+    );
+    assert!(String::from_utf8_lossy(&out.stderr).trim().is_empty());
+    assert_eq!(out.status.code(), Some(0));
+}
+
+/// Not a golangci-lint quirk: every stdout-only filter forwards stderr.
+#[test]
+fn stderr_forwarding_is_not_golangci_specific() {
+    let dir = tempfile::tempdir().expect("tempdir");
+    fake_tool(
+        dir.path(),
+        "ruff",
+        r#"echo "ruff failed hard" >&2
+exit 2"#,
+    );
+
+    let out = rtk_with(dir.path(), &["ruff", "check", "."]);
+
+    assert!(
+        String::from_utf8_lossy(&out.stderr).contains("ruff failed hard"),
+        "stderr must reach the user for any stdout-only filter"
+    );
+    assert_eq!(out.status.code(), Some(2), "exit code must propagate");
+}
+
+/// golangci-lint exits 1 when it merely found lint issues. RTK reports them and
+/// returns 0, so the linter running successfully does not fail a build.
+#[test]
+fn lint_issues_are_summarised_and_exit_zero() {
+    let dir = tempfile::tempdir().expect("tempdir");
+    let issues = r#"{"Issues":[{"FromLinter":"errcheck","Text":"unchecked","Pos":{"Filename":"main.go","Line":1,"Column":1},"SourceLines":["x()"],"Severity":""}]}"#;
+    fake_golangci(dir.path(), &format!("echo '{issues}'\nexit 1"));
+
+    let out = rtk_with(dir.path(), &["golangci-lint", "run"]);
+    let stdout = String::from_utf8_lossy(&out.stdout);
+
+    assert!(
+        stdout.contains("1 issues in 1 files") && stdout.contains("errcheck"),
+        "expected an issue summary; got: {stdout}"
+    );
+    assert_eq!(
+        out.status.code(),
+        Some(0),
+        "exit 1 means issues found, which RTK reports without failing"
+    );
+}

tests/stderr_passthrough_test.rs

@@ -1,8 +1,9 @@
 #![cfg(unix)]
-//! A failing tool's stderr must reach the user (#3026). Filters that compress
-//! stdout run with `RunOptions::stdout_only`, which keeps stderr out of the filter
-//! input — so unless the runner emits it, the diagnostics are dropped and the exit
-//! code becomes the only surviving signal.
+//! A tool's stderr must reach the user whatever the exit code. Filters that
+//! compress stdout run with `RunOptions::stdout_only`, which keeps stderr out of
+//! the filter input — so unless the runner emits it, the diagnostics are dropped
+//! and the exit code becomes the only surviving signal (upstream #3029), or, on
+//! a success exit with empty stdout, nothing at all survives (upstream #3772).
 
 use std::fs;
 use std::os::unix::fs::PermissionsExt;
@@ -55,15 +56,22 @@ fn failing_tool_stderr_reaches_the_user() {
 }
 
 #[test]
-fn successful_tool_stderr_stays_suppressed() {
-    // On success stderr is the incidental noise RTK exists to compress away, and
-    // the filtered stdout already carries the result.
+fn successful_tool_stderr_reaches_the_user() {
+    // A success exit carries diagnostics too — a deprecation notice, a missing
+    // config, prettier's whole report — and with stdout empty the never-worse
+    // guard used to collapse the run to silence on both streams.
     let db = tempfile::tempdir().expect("create db tempdir");
     let out = rtk_ruff(&shim(0), &db);
 
     assert_eq!(out.status.code(), Some(0));
     assert!(
-        !String::from_utf8_lossy(&out.stderr).contains(DIAGNOSTIC),
-        "successful run must not leak tool stderr"
+        String::from_utf8_lossy(&out.stderr).contains(DIAGNOSTIC),
+        "diagnostic swallowed on a success exit — stderr was {:?}, stdout was {:?}",
+        String::from_utf8_lossy(&out.stderr),
+        String::from_utf8_lossy(&out.stdout)
+    );
+    assert!(
+        String::from_utf8_lossy(&out.stdout).trim().is_empty(),
+        "the tool said nothing on stdout, so neither may rtk"
     );
 }