Skip to content

A job's status carries the facts behind its exit code - #457

Merged
tobert merged 2 commits into
mainfrom
fix/job-status-truth
Sep 21, 2026
Merged

tobert merged 2 commits into
mainfrom
fix/job-status-truth

Conversation

@tobert

@tobert tobert commented Sep 20, 2026

Copy link
Copy Markdown
Owner

A spilled background job and one that genuinely exited 3 were the same report. Job::to_info copied result.code and nothing else, so did_spill and original_code never reached JobInfo, and failed:3 was the whole answer either way. An embedder reading jobs --json had no way to ask whether the 3 was the command's own.

failed:3 stays as the status string — it is the loud signal that GH #212 installed, and replacing it would trade one ambiguity for a compatibility break. The two facts ride alongside it instead. JobInfo is #[non_exhaustive] with a builder, so the fields are additive, and jobs --json serializes JobInfo directly, so they appear with no rendering change:

{"id":1,"command":"seq 1 5000","status":"failed","exit_code":3,"did_spill":true,"original_code":0}

The test that proves it goes through Job::to_info rather than building a JobInfo by hand — which is how the gap stayed invisible, since every existing JobInfo test constructs the struct itself and so cannot miss a field the converter forgets. It runs two jobs under one output limit and separates them on the new fields alone.

Second: the test-only BackendDispatcher drained an external's stdout into its own ring with no tee into the background job's stream, so no test driven through that dispatcher could observe stream routing at all — a hole directly under the streaming work in #446/#448/#449, in the twin that exists so tests can reach exactly that. It now tees the way the production spawn site does, under the same only-or-last-stage rule, with a test that fails when the tee is removed.

Gates: cargo test --all clean, cargo clippy --all --all-targets zero warnings.

🤖 Generated with Claude Code

A spilled background job and one that genuinely exited 3 were the same
report. `Job::to_info` copied `result.code` and nothing else, so
`did_spill` and `original_code` never reached `JobInfo`, and `failed:3`
was the whole answer either way. An embedder reading `jobs --json` could
not ask whether the 3 was the command's own.

`failed:3` stays. It is the loud signal GH #212 installed, and changing
it would trade one ambiguity for a compatibility break. The two facts
ride alongside it instead: `JobInfo` is `#[non_exhaustive]` with a
builder, so the fields are additive, and `jobs --json` serializes
`JobInfo` directly so they appear with no rendering change.

The test that proves it goes through `Job::to_info` rather than building
a `JobInfo` by hand, which is how the gap stayed invisible — every
existing `JobInfo` test constructs the struct itself, so a field the
converter forgets is a field no test misses. It runs two jobs under one
output limit, `seq 1 5000` (spills, remapped from 0) and a function
returning 3 (its own code), and separates them on the new fields alone.

Second: the test-only `BackendDispatcher` drained an external's stdout
into its own ring with no tee into the background job's stream, so no
test driven through that dispatcher could observe stream routing — a
hole directly under the #446/#448/#449 streaming work, in the twin that
exists so tests can reach exactly this. It now tees the way
`spawn::spawn_process` does, under the same
first-or-only-stage rule.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@tobert

tobert commented Sep 20, 2026

Copy link
Copy Markdown
Owner Author

Review (kaibo, cast deepseek)

Reviewed against an idle worktree with this branch checked out. No defect in either change. Confirmed by the review:

  • Serde is wire-compatible. No snapshot pins job JSON, no jobs --json shape assertion covers a spilled job, nothing calls schema_for! on JobInfo, and there is no deny_unknown_fields anywhere in the tree. Both fields are default + skip_serializing_if, so a non-spilling job serializes byte-identically to before.
  • Job::to_info is the only converter. list, reap_finished, and get all route through it, and JobInfo::new is called nowhere else outside tests.
  • The dispatch tee matches production line for line: the same three-key stream lookup, the same Only | Last gate, the same drain_to_stream_teed. It cannot double-write — the only other writer of a job's stdout is publish_job_stdout, which is not on the external path in either the production spawn site or the twin.
  • No existing test changes behavior: the tee needs all three of job_manager, background_job, and background_stream_output, and only the new test sets them.

Findings addressed in 12b6bf3: docs/EMBEDDING.md and JobInfo::new's doc both enumerated the fields and stopped short of the new two. The field docs point at jobs --json as where an embedder reads them and nothing asserted they arrive there, so there is a test now with the contrast case, plus an assertion that both keys are absent when unset. And the tee test only covered PipelinePosition::Only, so a tee ignoring position entirely would have passed — a first-stage case now asserts an upstream stage's bytes stay out of the job's node.

Raised, not taken on here. Two things worth their own work:

  1. execute_user_tool rebuilds a function's result from scratch and never copies did_spill/original_code, so a spill inside a function body loses both before to_info can see them. Same bug class, one layer up, and pre-existing — every other compound form folds through accumulate_result, which carries them. JobInfo.did_spill is therefore complete for a direct command and not for a function call.
  2. JobInfo exposes one with_spill(did_spill, original_code) setter so the pair cannot be set half-way, but ToolResult's builders are separate, so an embedder backend tool can produce original_code: Some(n) with did_spill: false — a state the field's own doc says cannot happen.

Gates after the fixes: cargo test --all exit 0, no failures. cargo clippy --all --all-targets exit 0, zero warning or error lines.

🤖

Review found three gaps, all in what the change promised rather than in
what it did.

docs/EMBEDDING.md enumerates JobInfo's fields for embedders and stopped
short of the two new ones, as did JobInfo::new's own doc. Both name them
now.

The field docs lean on `jobs --json` as the place an embedder reads
them, and nothing asserted they arrive there. A test does now, with the
contrast case: a job that genuinely exited 3 carries neither key, so a
reader separates the two on presence alone. The omit-when-unset test
asserts their absence too — a reader that has not been updated for them
still sees exactly the document it saw before.

The tee test used `PipelinePosition::Only`, so the gate's other half
went untested: a tee that ignored position entirely would have passed.
A first-stage case now asserts an upstream stage's bytes stay out of the
job's node, where they would otherwise sit beside its real output.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@tobert
tobert merged commit d2eb259 into main Sep 21, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant