fix(cargo-wdk): update message format to json-render-diagnostics for clearer compiler diagnostics and make tests print the entire error message of child command invocations instead of truncating parts

[krishnakumar4a4]

This pull request updates the way external commands are executed in the build and packaging tasks, ensuring more precise control over output streams and improving the handling of command diagnostics. The main changes involve passing a CaptureStream parameter to all uses of the CommandExec::run method, updating tests accordingly, and switching the cargo build message format for better diagnostic output.

Command execution improvements:

• All invocations of CommandExec::run across build and packaging code now include an explicit CaptureStream argument, allowing callers to specify whether to capture stdout or stderr for each command. This provides finer control over command output handling. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]

• Imports and test mocks have been updated throughout the codebase to support the new CaptureStream parameter in CommandExec::run. This includes changes to test signatures and expectations to match the updated method signature. [1] [2] [3] [4] [5] [6] [7]

Cargo build diagnostics:

• The cargo build command now uses the --message-format=json-render-diagnostics option instead of --message-format=json. This ensures that only compiler diagnostics are emitted to stdout, while errors and warnings are sent to stderr, improving output clarity. [1] [2] [3]

Test and error handling updates:

• Tests for build and packaging tasks have been updated to use the new CaptureStream parameter, including changes to mock expectations and error assertions. Error handling is also improved to check the correct output fields. [1] [2] [3] [4] [5] [6] [7]

These changes collectively improve the reliability and clarity of command execution and diagnostics in the build process.

[Copilot]

Pull request overview

Updates cargo-wdk’s build invocation and test harnesses to improve the clarity and debuggability of compiler/build diagnostics.

Changes:

• Switch BuildTask’s cargo build invocation to --message-format=json-render-diagnostics.
• Update integration test helpers to inherit cargo clean output and to print full stdout/stderr for failed cargo wdk build runs.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
crates/cargo-wdk/src/actions/build/build_task.rs	Changes Cargo message format used by BuildTask::run for richer diagnostics.
crates/cargo-wdk/tests/build_command_test.rs	Adjusts test helpers to improve visibility of subprocess output and failure reporting.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The inline comment is misleading: --message-format=json-render-diagnostics still emits Cargo's machine-readable JSON message stream on stdout; it doesn't generally redirect compiler warnings/errors to stderr. Please reword this comment to reflect the actual behavior (e.g., that JSON messages include a rendered diagnostic field), or adjust stream handling if stderr is intended to be parsed.

Suggested change

	// We use `json-render-diagnostics` message format to
	// ensure only compiler diagnostics are emitted to stdout while still allowing
	// errors and warnings to be sent to stderr
	// Use `json-render-diagnostics` so Cargo emits its machine-readable JSON
	// message stream on stdout, including a pre-rendered `rendered` field in
	// diagnostic messages that we can consume via `cargo_metadata::Message`.

[krishnakumar4a4]

I think the current comment is fine.

[wmmc88]

nit: The "imperative" phrasing is more succint (ie. Use vs. We use)

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

crates/cargo-wdk/src/providers/exec.rs:79

• When capture_stream is StdErr, this sets stderr to Stdio::piped(), which suppresses the child process’ stderr from being shown to the parent process even on success (the bytes are captured but never forwarded). If the intent is only to improve error reporting while preserving normal user-visible stderr (warnings/progress), consider inheriting stderr by default and only capturing it when needed, or forwarding the captured stderr back to the parent on success/failure.

        cmd.stdout(Stdio::piped());

        // Capture this stream only on need basis to avoid unnecessary overhead
        if matches!(capture_stream, CaptureStream::StdErr) {
            cmd.stderr(Stdio::piped());
        }

crates/cargo-wdk/src/providers/exec.rs:77

• The phrase "on need basis" is ungrammatical and a bit unclear; consider rewording to "as needed" (or similar).

        // Capture this stream only on need basis to avoid unnecessary overhead
        if matches!(capture_stream, CaptureStream::StdErr) {

crates/cargo-wdk/src/actions/build/build_task.rs:131

• This comment says errors/warnings are still "sent to stderr", but CommandExec::run(..., CaptureStream::StdErr) pipes stderr, which prevents cargo’s stderr from being emitted to the parent stderr during normal runs. Either clarify the comment to reflect that stderr is captured (not emitted), or adjust the execution strategy so stderr is still forwarded to the user.

        // We use `json-render-diagnostics` message format to
        // ensure only compiler diagnostics are emitted to stdout while still allowing
        // errors and warnings to be sent to stderr
        args.push("--message-format=json-render-diagnostics".to_string());
        args.push("-p".to_string());
        args.push(self.package_name.to_string());
        if let Some(path) = self.manifest_path.to_str() {
            args.push("--manifest-path".to_string());
            args.push(path.to_string());
        } else {
            return Err(BuildTaskError::EmptyManifestPath);
        }
        if let Some(profile) = self.profile {
            args.push("--profile".to_string());
            args.push(profile.to_string());
        }
        if let Some(target_arch) = self.target_arch {
            args.push("--target".to_string());
            args.push(to_target_triple(target_arch));
        }
        if let Some(flag) = trace::get_cargo_verbose_flags(self.verbosity_level) {
            args.push(flag.to_string());
        }
        let args = args
            .iter()
            .map(std::string::String::as_str)
            .collect::<Vec<&str>>();

        // Run cargo build from the provided working directory so that config.toml
        // is respected
        let output = self.command_exec.run(
            "cargo",
            &args,
            None,
            Some(self.working_dir),
            CaptureStream::StdErr,
        )?;

crates/cargo-wdk/tests/new_command_test.rs:223

• The .expect("Failed to execute cargo wdk build command") message is used while invoking the cargo-wdk new command in this helper; consider updating it to reference the new command to make failures less confusing.

    let output = cmd
        .output()
        .expect("Failed to execute cargo wdk build command");

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 96.93878% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.65%. Comparing base (009dcd7) to head (c658933).

Files with missing lines	Patch %	Lines
crates/cargo-wdk/src/actions/build/package_task.rs	92.30%	1 Missing and 1 partial ⚠️
crates/cargo-wdk/src/providers/exec.rs	93.75%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #614      +/-   ##
==========================================
+ Coverage   77.41%   77.65%   +0.23%     
==========================================
  Files          24       24              
  Lines        4853     4909      +56     
  Branches     4853     4909      +56     
==========================================
+ Hits         3757     3812      +55     
- Misses        979      981       +2     
+ Partials      117      116       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gurry]

This seems wrong. It might prevent the cargo build output from streaming on the screen in real time.

[wmmc88]

yeah @gurry is right. this would prevent cargo build from streaming correctly. the existing implementation also has the inverse problem (blocks all the other wdk tool output from streaming correctly).

[wmmc88]

you probably need something like:

       // Take ownership of the stderr handle
       let stderr_handle = child.stderr.take().unwrap();
   
       // Spawn a thread that reads stderr line-by-line,
       // prints to the real stderr (streaming), and accumulates
       let tee_thread = thread::spawn(move || {
           let mut captured = Vec::new();
           let reader = BufReader::new(stderr_handle);
           for line in reader.lines() {
               let line = line.unwrap();
               // Stream to terminal in real time
               eprintln!("{line}");
               captured.push(line);
           }
           captured.join("\n")
       });

[wmmc88]

nit: The "imperative" phrasing is more succint (ie. Use vs. We use)

[wmmc88]

this should be stderr since its a cargo command (ie. human readable output is on stderr)

[wmmc88]

stderr

[wmmc88]

yeah @gurry is right. this would prevent cargo build from streaming correctly. the existing implementation also has the inverse problem (blocks all the other wdk tool output from streaming correctly).

[wmmc88]

you probably need something like:

       // Take ownership of the stderr handle
       let stderr_handle = child.stderr.take().unwrap();
   
       // Spawn a thread that reads stderr line-by-line,
       // prints to the real stderr (streaming), and accumulates
       let tee_thread = thread::spawn(move || {
           let mut captured = Vec::new();
           let reader = BufReader::new(stderr_handle);
           for line in reader.lines() {
               let line = line.unwrap();
               // Stream to terminal in real time
               eprintln!("{line}");
               captured.push(line);
           }
           captured.join("\n")
       });

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

BuildTask parses Cargo JSON messages from output.stdout, but CommandExec::run currently only populates the captured stream (and clears the other). With CaptureStream::StdErr here, output.stdout will be empty, so the message parser won’t receive any JSON. Mandatory fix: capture stdout for cargo message parsing (e.g., use CaptureStream::StdOut for this call), or adjust CommandExec::run to always return stdout/stderr bytes in Output while using CaptureStream only to decide what gets embedded into CommandError (and/or what gets tee’d).

[Copilot]

get_target_arch_from_cargo_rustc reads and parses output.stdout, but CommandExec::run will leave output.stdout empty when CaptureStream::StdErr is used. Mandatory fix: use CaptureStream::StdOut for this call, or change CommandExec::run to preserve stdout bytes even when stderr is the selected captured stream.

Suggested change

	CaptureStream::StdErr,
	CaptureStream::StdOut,

[Copilot]

std::process::Output conventionally contains both stdout and stderr if they were produced; here the non-captured stream is always cleared, even though the child may have produced it (and it may be needed by callers for parsing, debugging, or tests). This is already causing functional breakage in callers that expect stdout (see build code). Mandatory fix: preserve both streams in the returned Output (e.g., pipe + concurrently drain both, or use wait_with_output with both piped and tee both to the console), and use CaptureStream strictly to choose which stream’s content is surfaced in CommandError / error reporting.

Suggested change

	let output = match capture_stream {
	CaptureStream::StdOut => Output {
	status,
	stdout: captured_buf,
	stderr: Vec::new(),
	},
	CaptureStream::StdErr => Output {
	status,
	stdout: Vec::new(),
	stderr: captured_buf,
	},
	};
	// Preserve both stdout and stderr in the returned Output. The selected
	// capture_stream determines which stream is teed and used for error
	// reporting, but both streams are retained if they were piped.
	let (stdout, stderr) = match capture_stream {
	CaptureStream::StdOut => {
	// We already consumed stdout via the tee thread; now drain stderr.
	let mut stderr_buf = Vec::new();
	if let Some(mut stderr_pipe) = child.stderr.take() {
	stderr_pipe
	.read_to_end(&mut stderr_buf)
	.map_err(|e| CommandError::from_io_error(command, args, e))?;
	}
	(captured_buf, stderr_buf)
	}
	CaptureStream::StdErr => {
	// We already consumed stderr via the tee thread; now drain stdout.
	let mut stdout_buf = Vec::new();
	if let Some(mut stdout_pipe) = child.stdout.take() {
	stdout_pipe
	.read_to_end(&mut stdout_buf)
	.map_err(|e| CommandError::from_io_error(command, args, e))?;
	}
	(stdout_buf, captured_buf)
	}
	};
	let output = Output { status, stdout, stderr };

[Copilot]

Calling .expect("tee thread panicked") can crash the entire process on a thread panic, which is risky for a CLI tool (and makes failures harder to diagnose/handle cleanly). Prefer returning a structured error instead (e.g., map a panic to a CommandError variant or convert it into an io::Error with context).

Suggested change

	.expect("tee thread panicked")
	.map_err(|_| {
	CommandError::from_io_error(
	command,
	args,
	std::io::Error::new(
	std::io::ErrorKind::Other,
	"tee thread panicked while capturing command output",
	),
	)
	})?

[Copilot]

BufRead::lines() is line-oriented UTF-8 text processing; it strips newlines, normalizes line endings (notably on Windows), and cannot preserve arbitrary bytes. This can corrupt output for tools that emit non-line-based content (progress updates using \r, partial lines, binary output) and can subtly change machine-readable streams. A more robust approach is to tee raw bytes (read into a buffer and write the exact bytes through), and collect bytes rather than Vec<String>.

Suggested change

	let buf_reader = BufReader::new(reader);
	let mut lines = Vec::new();
	for line in buf_reader.lines() {
	let line = line?;
	writeln!(writer, "{line}")?;
	lines.push(line);
	}
	let mut buf_reader = BufReader::new(reader);
	let mut lines = Vec::new();
	let mut buf = Vec::new();
	loop {
	buf.clear();
	let bytes_read = buf_reader.read_until(b'\n', &mut buf)?;
	if bytes_read == 0 {
	break;
	}
	// Write the raw bytes to the writer to avoid altering the stream.
	writer.write_all(&buf)?;
	// Derive a logical "line" for collection purposes, similar to BufRead::lines():
	// strip trailing '\n' and an optional preceding '\r'.
	let mut end = buf.len();
	if end > 0 && buf[end - 1] == b'\n' {
	end -= 1;
	}
	if end > 0 && buf[end - 1] == b'\r' {
	end -= 1;
	}
	let line = String::from_utf8(buf[..end].to_vec())?;
	lines.push(line);
	}