Capture agent stderr and surface it as a failure diagnostic#65
Open
Tim-Pohlmann wants to merge 8 commits into
Open
Capture agent stderr and surface it as a failure diagnostic#65Tim-Pohlmann wants to merge 8 commits into
Tim-Pohlmann wants to merge 8 commits into
Conversation
The agent-plumbing CI assertions can only check "status: failure" - every agent CLI failure (auth, bad model, bad flag) collapses into the same "exited with code N" reason, since stderr was discarded entirely (RedirectStandardError: false) and even stdout's last line was dropped on the failure path. Capture the last non-empty line of each stream (stderr preferred, falling back to stdout) as ProcessFailure.Diagnostic, and fold it into JobFailure's error message, so callers can tell auth failures apart from other agent-exit failures instead of just observing that something went wrong.
Tim-Pohlmann
force-pushed
the
feat/capture-agent-stderr
branch
from
July 9, 2026 17:42
26630c7 to
fdec34b
Compare
Tim-Pohlmann
changed the base branch from
feat/multi-provider-model-passthrough
to
infra/job-yml-composite-actions
July 9, 2026 17:43
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves failure diagnostics from subprocess executions by capturing stderr (and falling back to stdout) and threading the last meaningful line into job failure errors, enabling CI/tests to distinguish common agent failure modes (auth/usage/bad flags) instead of only seeing a generic exit code.
Changes:
- Capture and drain stderr in
ProcessWrapper.RunAsync, producingProcessFailure.Diagnosticfrom the last non-empty stderr line (or stdout fallback). - Thread process diagnostics into
JobFailure.Erroras an error suffix when present. - Add unit tests covering diagnostic capture behavior and error-message formatting.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/Rix.Tests/ProcessWrapperTests.cs | Adds tests verifying stderr-preferred diagnostics, stdout fallback, and null diagnostic when no output. |
| tests/Rix.Tests/JobRunnerTests.cs | Adds tests verifying JobFailure.Error includes/omits diagnostic suffix appropriately. |
| src/Rix/Process/ProcessWrapper.cs | Redirects and drains stderr concurrently; adds ProcessFailure.Diagnostic and sets it on non-zero exit. |
| src/Rix/Job/JobRunner.cs | Appends diagnostic detail to agent failure error text when available. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- ProcessWrapper.RunAsync now awaits stderrTask (best-effort) before stdoutTask's exception propagates, so a callback exception doesn't leave stderr's read loop running unobserved against a disposed process's stream. - EchoToStderr now shell/PowerShell-quotes its arg instead of raw interpolation, so a future test passing metacharacters won't break or expand unexpectedly.
stdoutTask and stderrTask are now drained symmetrically on fault (either one faulting first now kills the process and best-effort-drains both), and stderr lines are forwarded through the existing onStdoutLine callback with a "[stderr] " prefix so redirecting/capturing stderr doesn't silently drop it from logs.
onStdoutLine is null at several call sites (git, npm install) that used to rely on stderr being inherited straight from the console before this method redirected/captured it. Those calls now got zero stderr visibility instead of the "prefixed but still visible" behavior other callers kept. Fall back to Console.Error.WriteLine so it's restored for them too.
The Console.Error fallback (used when onStdoutLine is null) was still prefixing with "[stderr] ", contradicting its own comment and needlessly altering the emitted line - that fallback already writes to a stream that never carries stdout, so there's nothing to disambiguate.
Sonar flagged RunAsync's cognitive complexity (16 > 15) and a suspicious always-true condition in the switch pattern. Extracting the forwarder selection into its own method and using a plain null-check resolves both.
- ProcessFailure.Diagnostic: clarify it's null unless the process actually ran and exited non-zero - the Win32Exception and timeout paths construct it without one, which the prior wording didn't account for. - BuildStderrForwarder: stdout/stderr are separate streams: what's actually shared (and needs the prefix to stay distinguishable) is the callback.
Draining stdout and stderr concurrently (added to fix the pipe-deadlock issue) let a caller-supplied onStdoutLine be invoked from both reader tasks at once - a real hazard for callers that assume serialized calls (test spies appending to a List<string>, non-thread-safe loggers). Wrap the callback once in a lock-based gate and share that synchronized delegate across both streams so invocations are always serialized, regardless of which stream a line arrived from. Synchronize is internal rather than private so the guarantee can be pinned down with a deterministic test: real OS pipe delivery can't be forced to overlap on demand (child stdout is block-buffered and arrives in one end-of-process burst on this platform, while stderr trickles line by line, so an end-to-end timing test can run for a long time without ever actually racing). Driving Synchronize directly with two threads under our own control proves the mutual exclusion instead.
|
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.
Summary
ProcessWrapper.RunAsyncdiscarded stderr entirely (RedirectStandardError: false) and dropped stdout's last line on the failure path, so every agent-CLI failure (auth, bad model, bad flag) collapsed into the same generic"exited with code N"reason.ProcessFailure.Diagnostic, folded intoJobFailure.Erroras"agent failed: exited with code N: <diagnostic>".status: failure.Test plan
dotnet test— all 178 tests pass, including newProcessWrapperTests/JobRunnerTestscoverage for stderr capture and diagnostic threading.