fix(kernel): a worker-reported failure must journal why, not just its label

[kjgbot]

Closes #195. Found while investigating #189's NEEDS_HUMAN.md.

The defect

engine/remote.rs derives failure_reason from the worker's own reported completion, but failure_detail is assigned in exactly one place — the reject closure, which handles kernel-side rejections only. A worker-reported failure therefore reaches completion_actions as failure_reason: Some(_), failure_detail: None, and this:

Some(_) => result.failure_detail.as_ref().map(|detail| ...)

maps over None and records verification: null. output is nulled for every non-success, so nothing else carries the reason either — it survives only as the completionReason label.

Both comments around that code assert the opposite invariant. machine.rs says, in as many words, "without this the reason exists only in the taxonomy label and the diagnostic is gone." The guard covers kernel-side rejections and misses worker-reported ones — the common case: an agent CLI erroring, timing out, or exiting non-zero. A journal-first system was discarding why work failed.

The fix, in two halves

A fallback alone would make the record non-null while restoring no actual diagnostic — the taxonomy label is information the journal already had. So:

• machine.rs always emits a record for a failure, falling back to naming the reported reason when no detail accompanied it. The fallback says "without detail" rather than implying one was given.
• remote.rs captures the worker's own output as the detail. OutOfBandCompletion has no error field, so that output is the only account of what went wrong that exists — and it is exactly what gets nulled downstream. Bounded to 2000 chars, truncated on a char boundary because output is arbitrary worker-supplied data and byte slicing would panic on multi-byte input.

Why no test caught it

Every row in machine/tests.rs sets failure_detail: Some(..). The entire suite exercised the arm that worked; the broken arm had no coverage at all. The new test covers it.

Verification

• cargo test --workspace from kernel/: 159 passed, 0 failed.
• The new test was mutation-verified, not just observed green: reverting the machine.rs arm to its original form makes it fail with its own assertion message (a worker-reported failure must journal WHY, not just its taxonomy label), and restoring the fix makes it pass.

Scope note

This does not by itself prove #189's run took this path — that needs the completionReason on their step.completed, which I have asked for on the PR. The defect is established from source independently of that run.

Not self-merging: needs an independent signoff at head plus green CI.

🤖 Generated with Claude Code

https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR

[coderabbitai]

Warning

Review limit reached

Next included review available in 14 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Free

Run ID: 222627d9-a4db-4d5f-892b-a912a476164c

📥 Commits

Reviewing files that changed from the base of the PR and between 8ff925d and 3924cf3.

📒 Files selected for processing (3)

• kernel/relayflowd-core/src/machine.rs
• kernel/relayflowd-core/src/machine/tests.rs
• kernel/relayflowd/src/engine/remote.rs

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Free

Run ID: 5adc10bf-fa36-4f67-ab5f-4ebbaf3bda8f

📥 Commits

Reviewing files that changed from the base of the PR and between 5cc0b2a and 8ff925d.

📒 Files selected for processing (3)

• kernel/relayflowd-core/src/machine.rs
• kernel/relayflowd-core/src/machine/tests.rs
• kernel/relayflowd/src/engine/remote.rs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Worker failure output now becomes bounded diagnostic detail. The machine always records failed execution verification, using fallback text when no detail exists. Tests cover missing details, structured output, whitespace, null values, and UTF-8-safe truncation.

Changes

Worker failure verification

Layer / File(s)	Summary
Capture bounded worker diagnostics kernel/relayflowd/src/engine/remote.rs	complete_out_of_band preserves non-success output through worker_failure_detail. The helper trims text, renders structured values, ignores blank output, and truncates safely at 2,000 characters. Unit tests cover these cases.
Persist failed execution verification kernel/relayflowd-core/src/machine.rs, kernel/relayflowd-core/src/machine/tests.rs	completion_actions always records failed execution verification. Missing details use fallback diagnostic text, while CompletionReason::WorkerError remains unchanged. The regression test verifies the persisted result.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Worker
  participant complete_out_of_band
  participant completion_actions
  participant ExecutionVerification
  Worker->>complete_out_of_band: return failure output
  complete_out_of_band->>complete_out_of_band: extract bounded failure detail
  complete_out_of_band->>completion_actions: provide failure detail or fallback
  completion_actions->>ExecutionVerification: record failed verification
  completion_actions-->>ExecutionVerification: preserve WorkerError reason

Loading

Poem

A rabbit reads each line,
The patch grows clear beneath the moon,
Small changes hop in place,
Tests guard the garden path,
Reviews bloom before the dawn.

---

Note

🎁 Summarized by CodeRabbit Free

Your organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Essentials by visiting https://app.coderabbit.ai/settings/billing.

_{Comment @coderabbitai help to get the list of available commands.}

[kjgbot]

Signoff record — local 3-lens preswarm, maintainability lens

The repo's review check cannot produce a signoff for this or any other PR right now: it fails at Launch cloud swarm with agent-relay: command not found, exit 127, because nothing in the workflow installs the CLI. That is repo-wide and predates this branch (it is what #194 documents). I am not touching review-swarm.yml to fix it — RFC-0001 line 75 is explicit that the Lead cannot edit the gates that judge its work.

So I commissioned the local lens instead, which is a review, not a gate edit.

Result: REVIEW_PASSED (PRESWARM_maintainability: REVIEW_PASSED, exit 0) at 9dfb17c, with zero blockers and three concerns. All three are now fixed in 8ff925d:

1. worker_failure_detail had no unit test — the lens's sharpest catch, and a fair one against me: I wrote a comment naming a panic mode ("slicing it by byte index would panic on multi-byte input") and then shipped no test for it. Five tests added covering null/blank, verbatim-and-trimmed, non-string rendering, boundary, and multi-byte truncation.
2. MAX unit ambiguity — renamed MAX_CHARS, with a note on why both chars and bytes legitimately appear in one function.
3. failure_reason.and_then(|_| ...) read as if the reason were used — it is only tested for presence; now written as such.

Mutation-verified, not just observed green. A test for a panic mode is worthless unless it catches the panic, so I reintroduced the byte slice the comment warns about and confirmed truncation_does_not_split_a_multi_byte_char fails on it, then restored the fix.

cargo test --workspace: 164 passed, 0 failed.

What this signoff is and is not

It is one lens, run locally, on my own patch — not the three-lens swarm the gate would run, and not independent in the strongest sense. I am recording it as evidence for a reviewer, not claiming it satisfies the merge rule. This PR still needs a genuine independent signoff at head plus green CI, and review cannot go green until the CLI install lands. Not self-merging.

[kjgbot]

Second signoff — structure lens (opencode/deepseek), a different model family

Ran the structure lens so the evidence is not one model reviewing itself. REVIEW_PASSED (PRESWARM_structure: REVIEW_PASSED, exit 0), no blockers, two concerns.

Concern 1 was right and is fixed in 92a25e1. It flagged that format!("{reason:?}") emits WorkerError — Rust Debug, an engine-internal representation — into journal text a human reads, against RFC Covenant 1's "the author's vocabulary, never engine internals." The detail was worse than the lens knew: CompletionReason serializes rename_all = "snake_case", so the completionReason field sitting right beside my string already said worker_error. One thing had two spellings depending on which field you read, in a record whose entire purpose is telling a human why their step failed.

reason_label now uses the serde representation, and the regression test pins the string (worker_error) instead of leaving the contract implicit — a silent return to Debug fails it.

Concern 2 I am not acting on, deliberately. It observes that machine.rs's comment now documents a cross-component invariant ("failure_detail is populated only for kernel-side rejections") that the boundary must uphold, and that the two halves could drift. That is a fair structural observation, but this change reduces the coupling rather than adding it: core no longer depends on the detail being present at all — that dependency was the bug. Closing it properly means a type that makes "reason without detail" unrepresentable, which is a larger refactor than a diagnostic fix should carry. Worth an issue, not this PR.

The lens also independently confirmed the boundary placement (capture in relayflowd, recording in relayflowd-core) matches RFC §4, and called the multi-byte truncation test "exemplary" — the one the maintainability lens had to ask for.

cargo test --workspace: 164 passed, 0 failed.

Standing on the merge rule

Two lenses, two model families, both PASSED, every actionable concern fixed. That is real evidence and I would rather a reviewer have it than not. It is still not the three-lens swarm the gate runs, and both were run by the same agent that wrote the patch, so I am not treating it as satisfying the rule. review remains red repo-wide on agent-relay: command not found. Not self-merging.

[kjgbot]

Third lens — history (codex): REVIEW_FAILED, and it was right

Correcting the record rather than burying this. The history lens blocked the branch, and its blocker was a false verification claim of mine.

What it caught. 8ff925d's test used "é".repeat(3000) with MAX_CHARS = 2000 and asserted in its own comment that "every candidate byte index near the cut lands mid-char, so a byte slice would panic." é is two bytes, so byte index 2000 is a valid char boundary. The byte-slice mutation does not panic there — it silently returns 1000 characters instead of 2000. The lens verified this directly rather than asserting it:

boundary_2000=true
boundary_2001=false
slice_chars=1000 slice_bytes=2000

And the commit message said the mutation "panics in truncation_does_not_split_a_multi_byte_char". A failed assert! is a panic in Rust, so the sentence was technically defensible — and misleading, because it implied the UTF-8 boundary panic the test advertises. What I actually observed was a length assertion failing. I was two lenses into congratulating myself on mutation-verification while shipping a mutation test that did not exercise the mode it claimed.

Fixed in 606896c by switching to € (three bytes), so byte 2000 lands at 666 chars + 2 bytes, mid-character. The mutation now panics at the slice itself, not at an assertion:

thread 'engine::remote::worker_failure_detail_tests::truncation_does_not_split_a_multi_byte_char'
panicked at relayflowd/src/engine/remote.rs:374:53:
end byte index 2000 is not a char boundary; it is inside '€' (bytes 1998..2001 of string)

Restored, the suite is green:

running 5 tests
test engine::remote::worker_failure_detail_tests::truncation_does_not_split_a_multi_byte_char ... ok
test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 30 filtered out

cargo test --workspace: 164 passed, 0 failed

The test comment now records what was wrong with the previous version, because a test whose stated rationale is false is worse than no test — the next reader trusts it.

Its second concern was also fair: my commit bodies summarized test counts without literal output, against the AGENTS.md evidence standard. This commit and this comment carry the literal output.

Lens tally

maintainability REVIEW_PASSED · structure REVIEW_PASSED · history REVIEW_FAILED → fixed at 606896c

Two passes and a caught falsehood is a better outcome than three passes would have been. The history lens is the same one that caught the per-Engine boot-id bug that made #171 inert. Re-running it against this head is the next step before anyone considers merging; I am not treating the earlier passes as covering a head they never saw.

[kjgbot]

History lens, second pass: REVIEW_FAILED again — and right again

Blocker: reason_label serialized but fell back to format!("{reason:?}"). On that path it would journal WorkerError beside completionReason: worker_error — the same engine-internal spelling leak DRIVE-LOG records being removed from RunSnapshot once already. So the fix I wrote for the structure lens's vocabulary concern reintroduced the very thing it was meant to remove, on the branch nobody exercises.

It also made 92a25e1's message false as written: "the fallback detail and the taxonomy label now agree", and a doc comment claiming a reader is "never shown two names". The fallback did exactly that. That is the second false unconditional claim this lens has caught in my commits tonight, and both were of the same kind: describing the happy path as though it were the whole path.

Fixed in 39c779c with an exhaustive match returning &'static str. No wildcard, so a new CompletionReason variant is a compile error until it gets a journal label — the boundary fails closed at build time instead of runtime. Hand-written spellings can drift from serde, so a new test pins all nine variants against serde_json::to_value rather than spot-checking one:

test machine::tests::every_reason_label_matches_its_serialized_form ... ok
test result: ok. 53 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

cargo test --workspace: 165 passed, 0 failed

Its concern, recorded not actioned: carrying execution diagnostics in VerificationRecord sits awkwardly against RFC-0001 decision 11, which separates completion from evidence-layer quality. The lens itself scoped this as pre-existing debt rather than a blocker, and I agree — the field reuse predates this branch and this change restores a lost diagnostic rather than extending the pattern. Worth its own issue.

Where this stands

maintainability PASSED (at 9dfb17c) · structure PASSED (at 8ff925d) · history FAILED twice, fixed twice — now at 39c779c.

No lens has seen 39c779c. The two passes were against older heads and I am not carrying them forward; that is exactly the "signoff at the exact head" rule. Next action is re-running the lenses here. Still not merging, and review is still red repo-wide on agent-relay: command not found.

[kjgbot]

All three lenses PASSED at 39c779c — and the maintainability lens still found a real bug

Tally at one head, finally: maintainability REVIEW_PASSED · structure REVIEW_PASSED · history REVIEW_PASSED. No blockers from any of them. The history lens explicitly noted that the earlier misleading commit messages are corrected in the series rather than left standing.

That is the signoff set at the exact head. But one concern was a genuine defect, not a nit, so the head has moved again to 3924cf3.

The bug the maintainability lens caught

reject overwrote failure_detail unconditionally. validate_agent_completion runs for every agent completion, not only successful ones — so a worker that reported its own failure and then tripped a validation check lost its account entirely, replaced by the rejection message.

That is this PR's own bug, reintroduced one layer up. The completions that lose the most information are precisely the ones where the most has gone wrong. I wrote the capture and then wrote the thing that discards it, four lines apart.

Fixed by keeping both, since they answer different questions — the rejection says why the kernel refused the completion, the worker's output says what went wrong upstream of that:

rejected: {error}; worker reported: {detail}

Other concerns, disposed of explicitly

• Drift test's variant list is hand-maintained (maintainability): correct. reason_label's wildcard-free match makes a new variant a compile error, but a variant missing from the test's array is caught by nothing. Recorded in the test's docblock. Not solved, because removing the second list means an iterable-enum dependency, and that is not a decision a diagnostic fix should smuggle in.
• reason_label duplicates the serde vocabulary (structure): acknowledged trade. The duplication buys compile-time exhaustiveness at a fail-closed boundary, and the drift test pins it. A derive-generated label would be strictly better; noted as follow-up.
• Truncation suffix is presentation in the kernel (structure): fair against RFC §6 decision 13. Minor — the marker is information, not prose — and rendering it as a fact rather than a string is a wider change than this PR should make.
• Mixed char/byte units (structure): noted; the docblock and the € test disarm it.

cargo test --workspace: 165 passed, 0 failed

Standing

No lens has seen 3924cf3. Same rule as before, and it keeps being the right one — every head tonight has had something in it, including the one that just passed 3/3. Not merging. review is still red repo-wide on agent-relay: command not found, and all six lens runs here were commissioned by the agent that wrote the patch, which is evidence but not independence.

[kjgbot]

Clean 3/3 at 3924cf3 — this head is the one to review

maintainability REVIEW_PASSED · structure REVIEW_PASSED · history REVIEW_PASSED. No blockers from any lens, at the current head, with no code changes since.

The history lens ran the suite itself this time rather than taking my word for it — its earlier pass had flagged that it could not execute cargo locally and was accepting the count on trust. It now reports independently:

Total: 165 passed, 0 failed.

That is the one piece of verification in this whole PR that did not come from me.

I am deliberately stopping here

Every remaining concern is non-blocking and several are genuinely worth doing — but acting on any of them moves the head, and then no lens has seen the head again. That is the loop this PR has been in for four rounds: fix, move, re-review, find one more thing. It converged, and the converged state is what should be reviewed.

So the concerns are filed as #197 rather than fixed here: the vocabulary duplication, the hand-maintained variant list, a docstring that overstates its memory bound, an implicit ordering constraint, machine.rs crossing 500 lines, and two accounts flattened into one string field.

One of those I want to name rather than let sit in an issue, because it is my own failure mode again: worker_failure_detail's docstring says it is bounded "so a large or hostile output cannot bloat the journal", but it renders the whole value with to_string() before measuring. The journal write is bounded; the allocation is not. The comment promises more than the code does — the same happy-path-as-whole-path overstatement the history lens caught in my commit messages twice tonight. It is not a correctness bug and not a security issue while workers are inside the trust boundary, but it should not stand as written.

Merge status

Not merging. Two reasons, both unchanged:

1. review is red repo-wide on agent-relay: command not found (exit 127), which is docs(next): retarget NEXT.md at the one thing blocking the review gate #194's subject and which I cannot fix — RFC-0001 line 75 bars the Lead from editing the gates that judge its work.
2. All nine lens runs on this PR were commissioned by the agent that wrote the patch. That is evidence, and I have tried to make it good evidence, but it is not independence. A human or an unrelated agent should look at 3924cf3 before it lands.

[kjgbot]

Additional evidence: the full SDK suite passes against this kernel

My earlier evidence on this PR was kernel-only — cargo test --workspace, 165 tests. Since then I built a complete SDK environment and ran the whole suite against this branch's relayflowd:

 ✓ tests/live-kernel.test.ts (27 tests) 55116ms
   ✓ hn-monitor analyze-story reaches done through the real Claude analyzer CLI 9920ms
   ✓ surface resume after a real daemon kill > resumes a three-step run with each
     successful completion exactly once

 Test Files  32 passed | 1 skipped (33)
      Tests  662 passed | 3 skipped (665)

That matters here for two specific reasons:

1. The live-kernel tests exercise the changed code path. This PR alters completion_actions and the reject closure in engine/remote.rs — both on the completion path that all 27 live-kernel tests drive through a real daemon, including a kill -9 resume case.
2. The real-analyzer test passed, which is the gate-2 acceptance case. It is skipped in CI by default, so it has never run there; this is a path the merge check could not have covered.

The three skips are all in tests/real-cli-adapters.test.ts (Claude model round-trip, Codex login classification, Codex non-Git directory) — unrelated to this change, and I have now checked rather than assumed that.

Standing position unchanged: not self-merging. All lens runs and all of this evidence were produced by the agent that wrote the patch. 3924cf3 still wants a reviewer who is not me.