Always write result.json, even on job failure#66
Open
Tim-Pohlmann wants to merge 11 commits into
Open
Conversation
Tim-Pohlmann
added a commit
that referenced
this pull request
Jul 8, 2026
The matrix's failure-mode legs asserted on the agent CLI's own prose error text (e.g. "Not logged in", "Incorrect API key"), which doesn't scale - every new failure mode meant hardcoding more vendor wording into workflow YAML, and the jq/case-based structural checks needed to make that robust don't belong embedded in a workflow script. agent-plumbing now only proves the plumbing itself works end-to-end (checkout, build, composite actions, output capture) via the one config that needs no credentials: opencode's free default model. Real per-failure-mode assertions move to a dedicated rix-job-e2e test next, where they can do proper JSON parsing instead of workflow-embedded jq. This also lets run-rix-job's "Run rix job" step drop back to a plain invocation - the stdout-capture/jq-parsed status+error outputs it grew existed only to feed the assertions removed here, and are unused now that rix always writes result.json (see #66) rather than only on success.
Callers previously had no reliable place to read a failed job's outcome from disk - result.json was only persisted on success, forcing consumers (e.g. CI) to scrape stdout instead. rix submit already rejects a non-success result.json, so writing it unconditionally is safe.
Tim-Pohlmann
force-pushed
the
feat/always-write-result-json
branch
from
July 9, 2026 17:44
547c6f5 to
55344e4
Compare
Tim-Pohlmann
changed the base branch from
feat/multi-provider-model-passthrough
to
feat/capture-agent-stderr
July 9, 2026 17:44
Tim-Pohlmann
added a commit
that referenced
this pull request
Jul 9, 2026
The matrix's failure-mode legs asserted on the agent CLI's own prose error text (e.g. "Not logged in", "Incorrect API key"), which doesn't scale - every new failure mode meant hardcoding more vendor wording into workflow YAML, and the jq/case-based structural checks needed to make that robust don't belong embedded in a workflow script. agent-plumbing now only proves the plumbing itself works end-to-end (checkout, build, composite actions, output capture) via the one config that needs no credentials: opencode's free default model. Real per-failure-mode assertions move to a dedicated rix-job-e2e test next, where they can do proper JSON parsing instead of workflow-embedded jq. This also lets run-rix-job's "Run rix job" step drop back to a plain invocation - the stdout-capture/jq-parsed status+error outputs it grew existed only to feed the assertions removed here, and are unused now that rix always writes result.json (see #66) rather than only on success.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes rix job persist result.json for both success and failure outcomes so callers (including CI) can read a stable machine-readable result from disk instead of parsing stdout.
Changes:
- Write
result.jsonunconditionally inStartup.ExecuteJobAsync(previously only onJobSuccess). - Update the job runner tests to expect
result.jsonto be written on failure.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/Rix.Tests/JobRunnerTests.cs | Updates the failure-path test to expect result.json to be written. |
| src/Rix/Startup.cs | Writes result.json for all IJobResult outcomes instead of only on success. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…just existence The PR's contract change is that callers can reliably read the job outcome from result.json, so the failure-path test should confirm its content actually says "failure", not just that the file exists.
- Wrap the failure-path JsonDocument.Parse in using var (it's disposable and holding pooled buffers longer than needed is unnecessary). - Use consistent <c> tags instead of markdown backticks, and an em-dash instead of a hyphen, in ExecuteJobAsync's doc comment.
CloneAsync threw InvalidOperationException uncaught, so a clone failure (bad repo, auth, network) skipped ExecuteJobAsync's result.json write entirely - contradicting the "regardless of outcome" guarantee this PR adds. Convert it to a SetupFailure instead, matching how agent-install failures and CreateBundleAsync failures are already handled.
…ror, test SetupFailure - Make the result.json write uncancellable and best-effort (catch IOException), so a cancellation racing right after the job finishes - or a transient disk error - can't stop the correct exit code from being returned. - Drop the redundant "clone failed: " prefix - RunGitAsync's own exception message already says "git clone failed: ...". - Add RunAsync_WritesResultJson_OnSetupFailure: the existing coverage only exercised the JobFailure path, leaving the other non-success outcome (installer failure) unverified.
Startup.ExecuteJobAsync is async; the new best-effort result.json warning used the blocking Console.Error.WriteLine instead of awaiting the async form.
…ailure path File.WriteAllTextAsync throws UnauthorizedAccessException (not IOException) when the target path exists as a directory or lacks write permission - the exact "permission issues" scenario the best-effort write was meant to guard against, so the catch clause needed to include it too. Added a test that forces the write to fail this way and asserts the exit code is still correct, closing the new-code coverage gap SonarCloud's quality gate flagged.
- TestStubs.cs: drop the redundant/incorrect Repository. qualifier from the <see cref> so it resolves (GitHubReadHost is already in scope via the existing using directive). - Startup.cs: wrap the Console.Error warning write in its own try/catch so a closed/broken stderr can't defeat the "must not stop the correct exit code from being returned" guarantee around the result.json write.
<paramref> must name a parameter of the member it's attached to, but these doc comments are on CloneAsync/CreateBundleAsync while naming the class's primary-constructor parameters (clone, createBundle) - a mismatch that's misleading even where no compiler warning fires. Reworded both as plain <c> references to the constructor parameter instead. Fixed createBundle's copy too since it's the same mistake, even though only the clone one was in this PR's diff.
Console.WriteLine(json) could throw on a broken/closed stdout pipe (e.g. the reader end goes away) and prevent ExecuteJobAsync from ever reaching the exit-code switch or the result.json write below it - undermining the same "must not stop the correct exit code from being returned" guarantee already established for the result.json write and its own warning write. Wrapped it in a matching best-effort try/catch. Added a regression test with a TextWriter that fails every write, swapped in via Console.SetOut. Note: Console.SetOut wraps the writer in a synchronized TextWriter whose async methods delegate to the synchronous ones under a lock, so the fake has to override WriteLine, not WriteLineAsync, or the override is silently never reached (verified by temporarily reverting the fix and confirming the test fails only once the writer overrides the right method).
Comment on lines
+105
to
+106
| try { await Console.Out.WriteLineAsync(json); } | ||
| catch (IOException) { /* nothing left to report to */ } |
Comment on lines
+117
to
+118
| try { await Console.Error.WriteLineAsync($"warning: failed to write result.json: {ex.Message}"); } | ||
| catch (IOException) { /* nothing left to report to */ } |
An already-disposed stdout/stderr fails the same way a broken pipe does (IOException), but only the pipe case was caught. Extracted the two identical try/catch blocks into a single WriteBestEffortAsync helper so both call sites pick up the wider catch instead of drifting apart again.
|
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
rix jobnow writesresult.jsonregardless of outcome, not just on success.rix submitalready rejected a non-successresult.json(SubmitRunner.cs), so this is safe with no changes needed there.Test plan
dotnet build Rix.slnx -c Releasedotnet test Rix.slnx -c Release(173 passed)