From 9dfb17cd64086719d4a1ac10fff3dfadb061f65a Mon Sep 17 00:00:00 2001 From: kjgbot Date: Sun, 6 Sep 2026 02:27:34 +0200 Subject: [PATCH 1/6] fix(kernel): a worker-reported failure must journal why, not just its label (#195) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `failure_detail` is populated only for kernel-side rejections, so `completion_actions` mapping over it dropped the verification record entirely whenever a WORKER reported the failure. `output` is nulled for every non-success, so the reason then survived only as the `completionReason` taxonomy label — the exact outcome both that branch's comment and remote.rs's comment say they exist to prevent. Two halves, because a fallback alone would only make the record non-null without restoring any diagnostic: - `machine.rs` always emits a record for a failure, falling back to naming the reported reason when no detail accompanied it. - `remote.rs` captures the worker's own output as the detail. It is the only account of what went wrong that exists — `OutOfBandCompletion` carries no error field — and it is precisely what gets nulled. Bounded to 2000 chars on a char boundary, since output is arbitrary worker-supplied data. The regression test covers the arm that had no coverage: every existing row in machine/tests.rs sets `failure_detail: Some(..)`, so the suite only ever exercised the arm that worked. Verified by reverting the machine.rs change and confirming the new test fails with its own assertion message. kernel: cargo test --workspace — 159 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- kernel/relayflowd-core/src/machine.rs | 23 ++++--- kernel/relayflowd-core/src/machine/tests.rs | 68 +++++++++++++++++++++ kernel/relayflowd/src/engine/remote.rs | 33 +++++++++- 3 files changed, 115 insertions(+), 9 deletions(-) diff --git a/kernel/relayflowd-core/src/machine.rs b/kernel/relayflowd-core/src/machine.rs index c070cb58..985745ce 100644 --- a/kernel/relayflowd-core/src/machine.rs +++ b/kernel/relayflowd-core/src/machine.rs @@ -320,14 +320,21 @@ pub fn completion_actions( // taxonomy label and the diagnostic is gone. let verification = match &result.failure_reason { None => Some(verify(step, &result.output)), - Some(_) => result - .failure_detail - .as_ref() - .map(|detail| crate::entry::VerificationRecord { - gate: "execution".to_owned(), - verdict: crate::entry::VerificationVerdict::Fail, - detail: detail.clone(), - }), + // `failure_detail` is populated only for kernel-side rejections, so + // mapping over it dropped the record entirely whenever a WORKER + // reported the failure — leaving the reason in the taxonomy label + // alone, which is the outcome this branch exists to prevent. The + // record is now unconditional: a failure always names itself, and the + // fallback marks that no detail accompanied the report rather than + // implying one was given. + Some(reason) => Some(crate::entry::VerificationRecord { + gate: "execution".to_owned(), + verdict: crate::entry::VerificationVerdict::Fail, + detail: result + .failure_detail + .clone() + .unwrap_or_else(|| format!("worker reported {reason:?} without detail")), + }), }; let verified = verification .as_ref() diff --git a/kernel/relayflowd-core/src/machine/tests.rs b/kernel/relayflowd-core/src/machine/tests.rs index 31aa0e3b..f4fc9c33 100644 --- a/kernel/relayflowd-core/src/machine/tests.rs +++ b/kernel/relayflowd-core/src/machine/tests.rs @@ -489,3 +489,71 @@ impl AppendAction for Action { } } } + +/// Regression, 2026-09-06 (#195): a WORKER-reported failure carries no +/// `failure_detail` — that field is set only for kernel-side rejections — and +/// the completion used to map over it, journaling `verification: null`. The +/// reason then survived only as the taxonomy label, which is precisely what the +/// branch was written to prevent, and `output` is nulled for every non-success +/// so nothing else carried it either. +/// +/// Every other row in this file sets `failure_detail: Some(..)`, so the whole +/// suite exercised the arm that worked and none of it touched the arm that did +/// not. This asserts the arm that did not. +#[test] +fn worker_reported_failure_without_detail_still_records_a_verification() { + let spec: crate::RunSpec = serde_json::from_value(json!({ + "version": "0.1.0", + "steps": [ + { + "id": "only", + "type": "deterministic", + "command": "false", + "max_iterations": 1 + } + ] + })) + .unwrap(); + let result = AttemptResult { + output: Value::Null, + budget: Budget::default(), + completed_by: "worker".to_owned(), + end_pins: None, + effects: Vec::new(), + trajectory_tail: None, + failure_reason: Some(CompletionReason::WorkerError), + // The point of the case: the worker reported a failure and sent no + // detail with it. + failure_detail: None, + }; + let entries: Vec<_> = completion_actions("run", &spec.steps[0], 1, 0, result, 1_000) + .into_iter() + .filter_map(|action| match action { + Action::Append(entry) => Some(entry), + _ => None, + }) + .collect(); + + let completed = entries + .iter() + .find(|entry| entry.entry_type == EntryType::StepCompleted) + .expect("a step.completed entry"); + let payload: StepCompletedPayload = + serde_json::from_value(serde_json::to_value(&completed.payload).unwrap()).unwrap(); + + let record = payload + .verification + .expect("a worker-reported failure must journal WHY, not just its taxonomy label"); + assert_eq!(record.verdict, crate::VerificationVerdict::Fail); + assert_eq!(record.gate, "execution"); + // The reason itself has to appear, or the record is present but empty of + // information and the diagnostic is still gone. + assert!( + record.detail.contains("WorkerError"), + "detail should name the reported reason, got {:?}", + record.detail + ); + // And the taxonomy label must still be the reason the worker gave, not + // overwritten by the verification bookkeeping. + assert_eq!(payload.completion_reason, CompletionReason::WorkerError); +} diff --git a/kernel/relayflowd/src/engine/remote.rs b/kernel/relayflowd/src/engine/remote.rs index de1b4a8b..cec80b8e 100644 --- a/kernel/relayflowd/src/engine/remote.rs +++ b/kernel/relayflowd/src/engine/remote.rs @@ -65,7 +65,13 @@ impl Engine { // A rejected completion names the mistake in the journal. `output` is // nulled for every non-success, so the detail rides the completion's // verification record — the same channel a failed gate uses. - let mut failure_detail = None; + // + // A WORKER-reported failure gets that treatment too. `OutOfBandCompletion` + // carries no error field, so the only account of what went wrong is the + // output the worker sent with its failing completion — and that is + // exactly what gets nulled. Capture it here, bounded, or the run records + // that the step failed and discards every trace of why. + let mut failure_detail = failure_reason.and_then(|_| worker_failure_detail(&completion.output)); let mut rejected_completion = false; let mut reject = |error: anyhow::Error| { rejected_completion = true; @@ -337,3 +343,28 @@ fn next_stream_offset(journal: &SqliteJournal, stream: &str) -> Result { } Ok(next) } + +/// The worker's own account of a failure, bounded so a large or hostile output +/// cannot bloat the journal. `None` when the worker sent nothing useful, which +/// keeps the caller's fallback ("reported X without detail") honest rather than +/// recording an empty string as though it were a diagnostic. +fn worker_failure_detail(output: &Value) -> Option { + const MAX: usize = 2000; + if output.is_null() { + return None; + } + let rendered = match output { + Value::String(text) => text.clone(), + other => other.to_string(), + }; + let trimmed = rendered.trim(); + if trimmed.is_empty() { + return None; + } + // Truncate on a char boundary; `output` is arbitrary worker-supplied data + // and slicing it by byte index would panic on multi-byte input. + Some(match trimmed.char_indices().nth(MAX) { + None => trimmed.to_owned(), + Some((cut, _)) => format!("{}… ({} bytes truncated)", &trimmed[..cut], trimmed.len() - cut), + }) +} From 8ff925dde2a7a1d4dab20d76e9e98e4e95a60197 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Sun, 6 Sep 2026 02:45:15 +0200 Subject: [PATCH 2/6] test(kernel): cover worker_failure_detail, and name its units (#195) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses all three concerns from the maintainability lens on this branch. The helper had four distinct behaviors and no coverage — including the multi-byte truncation whose panic mode the comment explicitly names. A future simplification back to `&trimmed[..MAX_CHARS]` would have hit that in production; it now fails a test instead. Verified by reintroducing the byte slice, which panics in `truncation_does_not_split_a_multi_byte_char`. Also: `MAX` -> `MAX_CHARS` with a note on why both chars and bytes appear in one function, and the call site no longer reads as though the failure reason is consumed when it is only tested for presence. kernel: cargo test --workspace — 164 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- kernel/relayflowd/src/engine/remote.rs | 69 ++++++++++++++++++++++++-- 1 file changed, 66 insertions(+), 3 deletions(-) diff --git a/kernel/relayflowd/src/engine/remote.rs b/kernel/relayflowd/src/engine/remote.rs index cec80b8e..7fd5c636 100644 --- a/kernel/relayflowd/src/engine/remote.rs +++ b/kernel/relayflowd/src/engine/remote.rs @@ -71,7 +71,10 @@ impl Engine { // output the worker sent with its failing completion — and that is // exactly what gets nulled. Capture it here, bounded, or the run records // that the step failed and discards every trace of why. - let mut failure_detail = failure_reason.and_then(|_| worker_failure_detail(&completion.output)); + let mut failure_detail = failure_reason + .is_some() + .then(|| worker_failure_detail(&completion.output)) + .flatten(); let mut rejected_completion = false; let mut reject = |error: anyhow::Error| { rejected_completion = true; @@ -349,7 +352,9 @@ fn next_stream_offset(journal: &SqliteJournal, stream: &str) -> Result { /// keeps the caller's fallback ("reported X without detail") honest rather than /// recording an empty string as though it were a diagnostic. fn worker_failure_detail(output: &Value) -> Option { - const MAX: usize = 2000; + // Chars, not bytes: the cut below is by char index. The suffix reports the + // remainder in bytes, which is why both units appear in one function. + const MAX_CHARS: usize = 2000; if output.is_null() { return None; } @@ -363,8 +368,66 @@ fn worker_failure_detail(output: &Value) -> Option { } // Truncate on a char boundary; `output` is arbitrary worker-supplied data // and slicing it by byte index would panic on multi-byte input. - Some(match trimmed.char_indices().nth(MAX) { + Some(match trimmed.char_indices().nth(MAX_CHARS) { None => trimmed.to_owned(), Some((cut, _)) => format!("{}… ({} bytes truncated)", &trimmed[..cut], trimmed.len() - cut), }) } + +#[cfg(test)] +mod worker_failure_detail_tests { + use super::worker_failure_detail; + use serde_json::{Value, json}; + + #[test] + fn a_null_or_blank_output_yields_no_detail() { + // The caller's fallback ("reported X without detail") is only honest if + // this returns None rather than an empty string dressed as a diagnostic. + assert_eq!(worker_failure_detail(&Value::Null), None); + assert_eq!(worker_failure_detail(&json!("")), None); + assert_eq!(worker_failure_detail(&json!(" \n\t ")), None); + } + + #[test] + fn a_string_output_is_carried_verbatim_and_trimmed() { + assert_eq!( + worker_failure_detail(&json!(" analyzer exited 1: no such model ")), + Some("analyzer exited 1: no such model".to_owned()) + ); + } + + #[test] + fn a_non_string_output_is_rendered_rather_than_dropped() { + // A worker that reports structured failure data must not have it + // discarded just because it is not a bare string. + assert_eq!( + worker_failure_detail(&json!({"code": 2})), + Some(r#"{"code":2}"#.to_owned()) + ); + } + + /// The comment on the truncation names a panic mode — byte slicing on + /// multi-byte input — and nothing tested it. A future "simplification" back + /// to `&trimmed[..MAX_CHARS]` panics here instead of in production. + #[test] + fn truncation_does_not_split_a_multi_byte_char() { + // 3000 two-byte chars: every candidate byte index near the cut lands + // mid-char, so a byte slice would panic. + let output = json!("é".repeat(3000)); + let detail = worker_failure_detail(&output).expect("detail for a long output"); + assert!(detail.contains('…'), "expected a truncation marker, got {detail:?}"); + assert!(detail.contains("bytes truncated")); + // Cut at 2000 chars, so 1000 chars * 2 bytes remain. + assert!( + detail.contains("2000 bytes truncated"), + "expected the byte remainder, got {detail:?}" + ); + assert_eq!(detail.chars().take_while(|c| *c == 'é').count(), 2000); + } + + #[test] + fn an_output_at_the_boundary_is_not_truncated() { + let exact = "a".repeat(2000); + assert_eq!(worker_failure_detail(&json!(exact.clone())), Some(exact)); + } +} From 92a25e18483af0633b5459f37e1f8865551add64 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Sun, 6 Sep 2026 02:58:26 +0200 Subject: [PATCH 3/6] fix(kernel): name the failure reason in the journal's vocabulary, not Rust's (#195) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the structure lens on this branch: `format!("{reason:?}")` emitted `WorkerError` — Rust Debug, an engine-internal representation — into text a human reads out of the journal. `CompletionReason` serializes `rename_all = "snake_case"`, so the `completionReason` field beside it already says `worker_error`. One thing had two spellings depending on which field you read. `reason_label` uses the serde representation, so the fallback detail and the taxonomy label now agree. The regression test pins the string rather than leaving the contract implicit — it asserts `worker_error`, so a silent return to Debug formatting fails it. kernel: cargo test --workspace — 164 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- kernel/relayflowd-core/src/machine.rs | 15 ++++++++++++++- kernel/relayflowd-core/src/machine/tests.rs | 7 +++++-- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/kernel/relayflowd-core/src/machine.rs b/kernel/relayflowd-core/src/machine.rs index 985745ce..4b9943ef 100644 --- a/kernel/relayflowd-core/src/machine.rs +++ b/kernel/relayflowd-core/src/machine.rs @@ -302,6 +302,19 @@ fn start_actions(state: &RunState, step: &StepSpec, attempt: u32, now_ms: i64) - vec![Action::Append(started), execute] } +/// A completion reason in the journal's own vocabulary rather than Rust's. +/// `CompletionReason` serializes `rename_all = "snake_case"`, so this yields +/// the same spelling the `completionReason` field carries (`worker_error`), and +/// a reader is never shown two names for one thing. `Debug` would emit +/// `WorkerError` — an engine-internal representation crossing into text the +/// author reads. +fn reason_label(reason: &CompletionReason) -> String { + serde_json::to_value(reason) + .ok() + .and_then(|value| value.as_str().map(str::to_owned)) + .unwrap_or_else(|| format!("{reason:?}")) +} + /// `semantic_executions` is the number of *completed* semantic executions /// before this attempt (`StepRuntime::semantic_executions`). The attempt being /// completed here ran to a result, so it is the `semantic_executions + 1`-th @@ -333,7 +346,7 @@ pub fn completion_actions( detail: result .failure_detail .clone() - .unwrap_or_else(|| format!("worker reported {reason:?} without detail")), + .unwrap_or_else(|| format!("worker reported {} without detail", reason_label(reason))), }), }; let verified = verification diff --git a/kernel/relayflowd-core/src/machine/tests.rs b/kernel/relayflowd-core/src/machine/tests.rs index f4fc9c33..525dc243 100644 --- a/kernel/relayflowd-core/src/machine/tests.rs +++ b/kernel/relayflowd-core/src/machine/tests.rs @@ -548,9 +548,12 @@ fn worker_reported_failure_without_detail_still_records_a_verification() { assert_eq!(record.gate, "execution"); // The reason itself has to appear, or the record is present but empty of // information and the diagnostic is still gone. + // The journal's own vocabulary, not Rust's Debug spelling: the same + // `worker_error` a reader sees in `completionReason`. Pinning the string + // keeps the two from drifting apart. assert!( - record.detail.contains("WorkerError"), - "detail should name the reported reason, got {:?}", + record.detail.contains("worker_error"), + "detail should name the reported reason in journal vocabulary, got {:?}", record.detail ); // And the taxonomy label must still be the reason the worker gave, not From 606896c953435c6d1a9d6c91f2d8c231e2581020 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Sun, 6 Sep 2026 03:11:52 +0200 Subject: [PATCH 4/6] test(kernel): make the multi-byte truncation test actually exercise the panic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The history lens caught a false verification claim in 8ff925d, and it was right. That 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 test did fail, but on a length assertion, which is a far weaker signal than the panic it advertised. 8ff925d's message said the mutation "panics in truncation_does_not_split_a_multi_byte_char". A failed `assert!` is technically a panic, so the sentence was defensible and still misleading: it implied the UTF-8 boundary panic the test claims to pin, and that is not what was observed. Switched to `€` (THREE bytes), so byte index 2000 falls at 666 chars + 2 bytes, mid-character. The mutation now panics at the slice itself: 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) With the fix restored: 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 A test whose stated rationale is false is worse than no test, because the next reader trusts it. The comment now records what was wrong with the old one. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- kernel/relayflowd/src/engine/remote.rs | 27 ++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/kernel/relayflowd/src/engine/remote.rs b/kernel/relayflowd/src/engine/remote.rs index 7fd5c636..ea476b41 100644 --- a/kernel/relayflowd/src/engine/remote.rs +++ b/kernel/relayflowd/src/engine/remote.rs @@ -406,23 +406,30 @@ mod worker_failure_detail_tests { ); } - /// The comment on the truncation names a panic mode — byte slicing on - /// multi-byte input — and nothing tested it. A future "simplification" back - /// to `&trimmed[..MAX_CHARS]` panics here instead of in production. + /// The truncation comment names a panic mode — byte slicing on multi-byte + /// input — and this pins it. The character matters: `€` is THREE bytes, so + /// byte index `MAX_CHARS` (2000) falls at 666 chars + 2 bytes, mid-character, + /// and `&trimmed[..MAX_CHARS]` panics on it. + /// + /// An earlier version of this test used `é` and claimed the same thing. That + /// was wrong: `é` is two bytes, so byte 2000 is a valid boundary and the + /// byte-slice mutation does NOT panic there — it silently returns half the + /// intended characters. The test still failed, but on a length assertion, + /// which is a much weaker signal than the panic it advertised. A test whose + /// stated rationale is false is worse than no test, because the next reader + /// trusts it. #[test] fn truncation_does_not_split_a_multi_byte_char() { - // 3000 two-byte chars: every candidate byte index near the cut lands - // mid-char, so a byte slice would panic. - let output = json!("é".repeat(3000)); + // 3000 three-byte chars = 9000 bytes. + let output = json!("€".repeat(3000)); let detail = worker_failure_detail(&output).expect("detail for a long output"); assert!(detail.contains('…'), "expected a truncation marker, got {detail:?}"); - assert!(detail.contains("bytes truncated")); - // Cut at 2000 chars, so 1000 chars * 2 bytes remain. + // Cut at 2000 CHARS = 6000 bytes, so 3000 bytes remain. assert!( - detail.contains("2000 bytes truncated"), + detail.contains("3000 bytes truncated"), "expected the byte remainder, got {detail:?}" ); - assert_eq!(detail.chars().take_while(|c| *c == 'é').count(), 2000); + assert_eq!(detail.chars().take_while(|c| *c == '€').count(), 2000); } #[test] From 39c779cb19b6fd50719b907ff8e06fde43fe8538 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Sun, 6 Sep 2026 03:18:14 +0200 Subject: [PATCH 5/6] fix(kernel): make the journal label exhaustive so Debug cannot leak (#195) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The history lens blocked 92a25e1, correctly. `reason_label` serialized and fell back to `format!("{reason:?}")`, so the fallback path could journal `WorkerError` beside `completionReason: worker_error` — the same engine-internal spelling leak DRIVE-LOG records being removed from `RunSnapshot`. That also made 92a25e1's message false as written: it said "the fallback detail and the taxonomy label now agree" and the doc comment said a reader is "never shown two names", when the fallback did exactly that. Replaced with an exhaustive match returning `&'static str`. There is no wildcard, so a new `CompletionReason` variant is a compile error until it is given a journal label: the boundary now fails closed at build time rather than at runtime. Hand-written spellings can drift from serde, so `every_reason_label_matches_its_serialized_form` 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 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- kernel/relayflowd-core/src/machine.rs | 34 +++++++++++++++------ kernel/relayflowd-core/src/machine/tests.rs | 27 ++++++++++++++++ 2 files changed, 51 insertions(+), 10 deletions(-) diff --git a/kernel/relayflowd-core/src/machine.rs b/kernel/relayflowd-core/src/machine.rs index 4b9943ef..339e9166 100644 --- a/kernel/relayflowd-core/src/machine.rs +++ b/kernel/relayflowd-core/src/machine.rs @@ -303,16 +303,30 @@ fn start_actions(state: &RunState, step: &StepSpec, attempt: u32, now_ms: i64) - } /// A completion reason in the journal's own vocabulary rather than Rust's. -/// `CompletionReason` serializes `rename_all = "snake_case"`, so this yields -/// the same spelling the `completionReason` field carries (`worker_error`), and -/// a reader is never shown two names for one thing. `Debug` would emit -/// `WorkerError` — an engine-internal representation crossing into text the -/// author reads. -fn reason_label(reason: &CompletionReason) -> String { - serde_json::to_value(reason) - .ok() - .and_then(|value| value.as_str().map(str::to_owned)) - .unwrap_or_else(|| format!("{reason:?}")) +/// +/// Exhaustive on purpose. An earlier version serialized and fell back to +/// `format!("{reason:?}")`, which meant the fallback path could journal +/// `WorkerError` beside `completionReason: worker_error` — the same +/// engine-internal spelling leak DRIVE-LOG records being removed from +/// `RunSnapshot`. A match with no wildcard cannot leak: adding a variant is a +/// compile error here until it is given its journal label, so the boundary +/// fails closed at build time rather than at runtime. +/// +/// These strings must stay identical to the `rename_all = "snake_case"` +/// spellings `CompletionReason` serializes with, which +/// `every_reason_label_matches_its_serialized_form` pins. +fn reason_label(reason: &CompletionReason) -> &'static str { + match reason { + CompletionReason::Success => "success", + CompletionReason::VerificationFailed => "verification_failed", + CompletionReason::RetriesExhausted => "retries_exhausted", + CompletionReason::LeaseExpired => "lease_expired", + CompletionReason::Crashed => "crashed", + CompletionReason::Timeout => "timeout", + CompletionReason::WorkerError => "worker_error", + CompletionReason::BudgetExceeded => "budget_exceeded", + CompletionReason::Canceled => "canceled", + } } /// `semantic_executions` is the number of *completed* semantic executions diff --git a/kernel/relayflowd-core/src/machine/tests.rs b/kernel/relayflowd-core/src/machine/tests.rs index 525dc243..feb0dddd 100644 --- a/kernel/relayflowd-core/src/machine/tests.rs +++ b/kernel/relayflowd-core/src/machine/tests.rs @@ -560,3 +560,30 @@ fn worker_reported_failure_without_detail_still_records_a_verification() { // overwritten by the verification bookkeeping. assert_eq!(payload.completion_reason, CompletionReason::WorkerError); } + +/// `reason_label` hand-writes the journal spellings, so nothing but a test stops +/// it drifting from what `CompletionReason` actually serializes. Pins every +/// variant against serde rather than spot-checking one, so a rename in either +/// place fails here instead of silently showing a reader two names for one +/// completion. +#[test] +fn every_reason_label_matches_its_serialized_form() { + for reason in [ + CompletionReason::Success, + CompletionReason::VerificationFailed, + CompletionReason::RetriesExhausted, + CompletionReason::LeaseExpired, + CompletionReason::Crashed, + CompletionReason::Timeout, + CompletionReason::WorkerError, + CompletionReason::BudgetExceeded, + CompletionReason::Canceled, + ] { + let serialized = serde_json::to_value(reason).unwrap(); + assert_eq!( + serialized.as_str().expect("a string spelling"), + super::reason_label(&reason), + "journal label drifted from the serialized form for {reason:?}" + ); + } +} From 3924cf32d1f1090d63310b639b7c9bb410c84561 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Sun, 6 Sep 2026 03:31:15 +0200 Subject: [PATCH 6/6] fix(kernel): keep both accounts when a rejected completion also reported failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the maintainability lens at 39c779c. `reject` overwrote `failure_detail` unconditionally, so a worker that reported its own failure AND then tripped `validate_agent_completion` lost its account entirely — the rejection replaced it. `validate_agent_completion` runs for every agent completion, not only successful ones, so that path is reachable. That is the loss this branch exists to stop, reintroduced one layer up: the completions that lose the most information are exactly the ones where the most has gone wrong. Both are kept now — "rejected: {error}; worker reported: {detail}" — because they answer different questions. The rejection says why the kernel refused the completion; the worker's output says what went wrong upstream of that. Also records, on the drift test, that its variant list is hand-maintained: the wildcard-free match in `reason_label` makes a NEW variant a compile error, but a variant merely missing from the test array is caught by nothing. Noted rather than solved, since removing the second list means an iterable-enum dependency, which is not a decision a diagnostic fix should smuggle in. cargo test --workspace: 165 passed, 0 failed Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- kernel/relayflowd-core/src/machine/tests.rs | 6 ++++++ kernel/relayflowd/src/engine/remote.rs | 12 +++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/kernel/relayflowd-core/src/machine/tests.rs b/kernel/relayflowd-core/src/machine/tests.rs index feb0dddd..8d27e322 100644 --- a/kernel/relayflowd-core/src/machine/tests.rs +++ b/kernel/relayflowd-core/src/machine/tests.rs @@ -566,6 +566,12 @@ fn worker_reported_failure_without_detail_still_records_a_verification() { /// variant against serde rather than spot-checking one, so a rename in either /// place fails here instead of silently showing a reader two names for one /// completion. +/// +/// The list below is itself hand-maintained: `reason_label`'s wildcard-free +/// match makes a NEW variant a compile error there, but a new variant simply +/// missing from this array is not caught by anything. Add variants in both +/// places. (An iterable-enum derive would remove the second list; that is a +/// dependency decision, not one to smuggle into a diagnostic fix.) #[test] fn every_reason_label_matches_its_serialized_form() { for reason in [ diff --git a/kernel/relayflowd/src/engine/remote.rs b/kernel/relayflowd/src/engine/remote.rs index ea476b41..89051670 100644 --- a/kernel/relayflowd/src/engine/remote.rs +++ b/kernel/relayflowd/src/engine/remote.rs @@ -79,7 +79,17 @@ impl Engine { let mut reject = |error: anyhow::Error| { rejected_completion = true; failure_reason = Some(CompletionReason::WorkerError); - failure_detail = Some(format!("{error:#}")); + // Keep BOTH accounts when a worker reports its own failure and then + // trips validation. The rejection says why the kernel refused the + // completion; the worker's output says what went wrong upstream of + // that, and the two are rarely the same story. Overwriting here + // would discard the worker's account for exactly the completions + // that have the most gone wrong — the loss this whole change exists + // to stop, reintroduced one layer up. + failure_detail = Some(match failure_detail.take() { + Some(reported) => format!("rejected: {error:#}; worker reported: {reported}"), + None => format!("{error:#}"), + }); }; let effects = if matches!(step.kind, StepKind::Agent { .. }) { let recorded = recorded_effects(&journal, &step, completion.attempt)?;