Preserve stderr so wrapped command failures are visible#14
Draft
hsoerensen wants to merge 1 commit into
Draft
Conversation
runProxy captured stderr from the executor and then never wrote it anywhere. Any command whose useful output lives on stderr was silently truncated to whatever the stdout filter produced.
Concrete case: a repo with a lefthook pre-push hook that enforces coverage. When the hook fails, git push exits 1 and writes the hook error plus 'failed to push some refs' to stderr. Through ztk the agent saw:
exit=1
output: ok
filterGitPush returned 'ok' for the empty stdout, the real failure on stderr was thrown away, and the agent had no idea why the push failed. In practice this causes real churn: agents loop, retry, or give up because the wrapper hides the actual error.
Fix: forward result.stderr to the user's stderr via compat.writeStderr. Stdout filtering is unchanged, so secret-masking filters like the env filter still run on failed commands. Tests cover stderr passthrough on both zero and nonzero exit, plus a regression test that env-style sensitive values (API_KEY etc.) stay masked when the underlying command exits nonzero.
Note on stderr filtering: ztk has no stderr filters today. Secrets that show up on stderr (URL-embedded credentials in git auth errors, tokens in verbose curl/ssh output) already reach the agent's environment when those commands run outside ztk, so this PR does not introduce new exposure; it removes an accidental side effect of dropping the stream. A scrubber for URL-embedded credentials, GitHub/AWS tokens, and Bearer headers is a follow-up, but the current behavior silently turns real errors into 'ok', and that's the bigger bug.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors proxy output handling by centralizing stdout post-processing and introducing explicit stderr forwarding, with new unit tests validating the updated behavior.
Changes:
- Replace inline stdout processing with a new
processOutput()helper that returns processed stdout plus original stderr. - Update
runProxy()to emit processed stdout via existing path and additionally writestderrto stderr. - Add tests covering stderr forwarding and stdout masking behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, | ||
| log_path, | ||
| ) catch {}; | ||
| if (processed.stderr.len > 0) compat.writeStderr(processed.stderr) catch {}; |
Comment on lines
+101
to
+108
| fn processOutput(cmd: []const u8, result: executor.ExecResult, allocator: std.mem.Allocator) ProcessedOutput { | ||
| const filtered = applyFilters(cmd, result.stdout, allocator); | ||
| const final_bytes = maybeApplySession(cmd, filtered, allocator); | ||
| return .{ | ||
| .stdout = final_bytes, | ||
| .stderr = result.stderr, | ||
| }; | ||
| } |
| }, | ||
| log_path, | ||
| ) catch {}; | ||
| if (processed.stderr.len > 0) compat.writeStderr(processed.stderr) catch {}; |
| const result = try executor.exec(cmd_args, allocator, .filter_stdout_only); | ||
| const filtered = applyFilters(cmd_str, result.stdout, allocator); | ||
| const final_bytes = maybeApplySession(cmd_str, filtered, allocator); | ||
| const processed = processOutput(cmd_str, result, allocator); |
| stderr: []const u8, | ||
| }; | ||
|
|
||
| fn processOutput(cmd: []const u8, result: executor.ExecResult, allocator: std.mem.Allocator) ProcessedOutput { |
| }; | ||
| const processed = processOutput("git push", exec_result, arena.allocator()); | ||
| try std.testing.expectEqualStrings(exec_result.stderr, processed.stderr); | ||
| try std.testing.expect(std.mem.indexOf(u8, processed.stdout, "ok") != null); |
Contributor
Author
|
Closing and reopening as draft to investigate Copilot flagged issues. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
runProxy captured stderr from the executor and then never wrote it anywhere. Any command whose useful output lives on stderr was silently truncated to whatever the stdout filter produced.
Concrete case: a repo with a lefthook pre-push hook that enforces code coverage percentage. When the hook fails, git push exits 1 and writes the hook error plus 'failed to push some refs' to stderr. Through ztk the agent saw:
filterGitPush returned 'ok' for the empty stdout, the real failure on stderr was thrown away, and the agent had no idea why the push failed. In practice this causes real churn: agents loop, retry, or give up because the wrapper hides the actual error.
Fix
forward result.stderr to the user's stderr via compat.writeStderr. Stdout filtering is unchanged, so secret-masking filters like the env filter still run on failed commands. Tests cover stderr passthrough on both zero and nonzero exit, plus a regression test that env-style sensitive values (API_KEY etc.) stay masked when the underlying command exits nonzero.
Additional
ztk has no stderr filters today. Secrets that show up on stderr (URL-embedded credentials in git auth errors, tokens in verbose curl/ssh output) already reach the agent's environment when those commands run outside ztk.
I'm thinking it may be questionable for ztk to do any filtering of secrets at all, as that could well be the responsibility of another tool.
Note
Preserve stderr output from wrapped commands in
runProxyPreviously, stderr from executed commands was silently discarded. Now
runProxyforwards it verbatim to the process stderr viacompat.writeStderrin non-test builds.processOutputhelper in proxy.zig that returns both processed stdout and the original stderr as aProcessedOutputstruct.Macroscope summarized dfed8b3.