feat(kernel): lens follow-ups — vocabulary owner, render bound, ordering (#197) - #376
Conversation
…ing (#197) Three follow-ups filed by #196's lens set: 1. Vocabulary owner: `reason_label` had two owners (the free `fn` in `machine.rs` and serde's `rename_all = "snake_case"`), with a hand- maintained variant array in `every_reason_label_matches_its_serialized_form` as the drift guard. Moved the labeler onto `CompletionReason` itself (`.journal_label()`) alongside a `CompletionReason::ALL` const, so the enum owns its label AND its enumeration. The old free fn is deleted; the machine-side wrapper test stays only to catch a rename at that call site. 2. Render bound: `worker_failure_detail` allocated the full `Value::to_string()` before measuring or truncating — a multi-megabyte JSON object would fully materialize in memory before the guard cut it, contradicting the docstring's "bounded so a large or hostile output cannot bloat the journal" claim. A hand-rolled bounded `Write` now stops `serde_json::to_writer` at `MAX_RENDER_BYTES` (MAX_CHARS * 4 + slack). Large `Value::String` inputs are also cut on a char boundary before cloning. Two new tests hit the render cap with ~500 KB inputs. 3. Ordering explicit: the `failure_reason.is_some().then(|| worker_failure_detail(...)).flatten()` capture had an implicit precondition — must run before the `reject` closure or a success-path completion would inherit a stale detail. Rewrote as an explicit `match` with a NOTE comment naming the precondition so a future edit that moves it downward reads as suspicious. Kernel lib tests: 138 passed / 0 failed. Related: #196 (where these were filed), #195 (the defect #196 fixed), #189 (where it surfaced), #218 (lens drift — separate). Session-Id: efeda5df-9b7c-48d4-b2ce-957f5bef0a82
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c32351c. Configure here.
| } else { | ||
| trimmed.to_owned() | ||
| } | ||
| } |
There was a problem hiding this comment.
Truncation count ignores render cap
Low Severity
The bytes truncated remainder uses the length of the already-capped render. When worker output exceeds MAX_RENDER_BYTES and still has more than MAX_CHARS, the journaled figure is the leftover of the cap, not how much output was actually dropped.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c32351c. Configure here.
There was a problem hiding this comment.
4 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="kernel/relayflowd/src/engine/remote.rs">
<violation number="1" location="kernel/relayflowd/src/engine/remote.rs:416">
P2: When a failure string has more than 8,256 leading whitespace bytes, this prefix cap discards the useful diagnostic and returns `None`. Trim the string before applying the render cap so bounded rendering preserves the diagnostic.</violation>
<violation number="2" location="kernel/relayflowd/src/engine/remote.rs:464">
P2: When the render cap and character cap both apply, this flag is ignored and the detail reports only the remainder of the capped prefix. Mark the detail as render-bounded, or report the byte count as a lower bound, whenever `render_truncated` is true.</violation>
</file>
<file name="kernel/relayflowd-core/src/entry.rs">
<violation number="1" location="kernel/relayflowd-core/src/entry.rs:316">
P2: The test's name and doc claim it proves `ALL` enumerates every variant, but it only checks that the listed labels are unique. Both this test and `every_journal_label_matches_serialized` iterate over `CompletionReason::ALL`, so if a new variant is added to the enum and `journal_label` but not to `ALL`, neither test observes it. The claim '(b) a test failure if you skip ALL' is therefore false — the missing variant is silently undetected and can drift the journal label out of sync with the serialized form. Either add a real coverage guard or correct the misleading name/doc.</violation>
</file>
<file name="kernel/relayflowd-core/src/machine/tests.rs">
<violation number="1" location="kernel/relayflowd-core/src/machine/tests.rs:646">
P3: This test duplicates the owner-side drift test in `entry.rs` without exercising the machine path. Remove the wrapper or make it assert through `completion_actions` so it provides behavior coverage rather than a second identical assertion.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| truncated: false, | ||
| }; | ||
| let _ = serde_json::to_writer(&mut writer, other); | ||
| render_truncated = writer.truncated; |
There was a problem hiding this comment.
P2: When the render cap and character cap both apply, this flag is ignored and the detail reports only the remainder of the capped prefix. Mark the detail as render-bounded, or report the byte count as a lower bound, whenever render_truncated is true.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At kernel/relayflowd/src/engine/remote.rs, line 464:
<comment>When the render cap and character cap both apply, this flag is ignored and the detail reports only the remainder of the capped prefix. Mark the detail as render-bounded, or report the byte count as a lower bound, whenever `render_truncated` is true.</comment>
<file context>
@@ -378,20 +387,90 @@ fn next_stream_offset(journal: &SqliteJournal, stream: &str) -> Result<u64> {
+ truncated: false,
+ };
+ let _ = serde_json::to_writer(&mut writer, other);
+ render_truncated = writer.truncated;
+ // Repair a mid-multi-byte cut so `String::from_utf8` never fails.
+ while !writer.buf.is_empty() && std::str::from_utf8(&writer.buf).is_err() {
</file context>
| Value::String(text) => text.clone(), | ||
| other => other.to_string(), | ||
| Value::String(text) => { | ||
| if text.len() > MAX_RENDER_BYTES { |
There was a problem hiding this comment.
P2: When a failure string has more than 8,256 leading whitespace bytes, this prefix cap discards the useful diagnostic and returns None. Trim the string before applying the render cap so bounded rendering preserves the diagnostic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At kernel/relayflowd/src/engine/remote.rs, line 416:
<comment>When a failure string has more than 8,256 leading whitespace bytes, this prefix cap discards the useful diagnostic and returns `None`. Trim the string before applying the render cap so bounded rendering preserves the diagnostic.</comment>
<file context>
@@ -378,20 +387,90 @@ fn next_stream_offset(journal: &SqliteJournal, stream: &str) -> Result<u64> {
- Value::String(text) => text.clone(),
- other => other.to_string(),
+ Value::String(text) => {
+ if text.len() > MAX_RENDER_BYTES {
+ render_truncated = true;
+ // Char-boundary-safe slice for the pre-cap head.
</file context>
| /// build error if you skip `journal_label`, or (b) a test failure if you | ||
| /// skip `ALL`. | ||
| #[test] | ||
| fn all_covers_every_serialized_label() { |
There was a problem hiding this comment.
P2: The test's name and doc claim it proves ALL enumerates every variant, but it only checks that the listed labels are unique. Both this test and every_journal_label_matches_serialized iterate over CompletionReason::ALL, so if a new variant is added to the enum and journal_label but not to ALL, neither test observes it. The claim '(b) a test failure if you skip ALL' is therefore false — the missing variant is silently undetected and can drift the journal label out of sync with the serialized form. Either add a real coverage guard or correct the misleading name/doc.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At kernel/relayflowd-core/src/entry.rs, line 316:
<comment>The test's name and doc claim it proves `ALL` enumerates every variant, but it only checks that the listed labels are unique. Both this test and `every_journal_label_matches_serialized` iterate over `CompletionReason::ALL`, so if a new variant is added to the enum and `journal_label` but not to `ALL`, neither test observes it. The claim '(b) a test failure if you skip ALL' is therefore false — the missing variant is silently undetected and can drift the journal label out of sync with the serialized form. Either add a real coverage guard or correct the misleading name/doc.</comment>
<file context>
@@ -241,6 +241,89 @@ pub enum CompletionReason {
+ /// build error if you skip `journal_label`, or (b) a test failure if you
+ /// skip `ALL`.
+ #[test]
+ fn all_covers_every_serialized_label() {
+ let labels: std::collections::HashSet<&str> =
+ CompletionReason::ALL.iter().map(|r| r.journal_label()).collect();
</file context>
| CompletionReason::BudgetExceeded, | ||
| CompletionReason::Canceled, | ||
| ] { | ||
| for reason in CompletionReason::ALL { |
There was a problem hiding this comment.
P3: This test duplicates the owner-side drift test in entry.rs without exercising the machine path. Remove the wrapper or make it assert through completion_actions so it provides behavior coverage rather than a second identical assertion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At kernel/relayflowd-core/src/machine/tests.rs, line 646:
<comment>This test duplicates the owner-side drift test in `entry.rs` without exercising the machine path. Remove the wrapper or make it assert through `completion_actions` so it provides behavior coverage rather than a second identical assertion.</comment>
<file context>
@@ -637,29 +637,17 @@ fn worker_reported_failure_without_detail_still_records_a_verification() {
- CompletionReason::BudgetExceeded,
- CompletionReason::Canceled,
- ] {
+ for reason in CompletionReason::ALL {
let serialized = serde_json::to_value(reason).unwrap();
assert_eq!(
</file context>


Closes #197. Builds on #196's lens set. Three follow-ups: (1) moved reason_label onto CompletionReason::journal_label() with ALL const; (2) bounded worker_failure_detail render via a BoundedWriter — no more full allocation of hostile JSON before truncation; (3) explicit-match capture ordering. Kernel lib tests 138/0.
Note
Medium Risk
Changes kernel journaling and out-of-band completion handling for worker-supplied output; risk is mitigated by exhaustive tests and defensive bounds, but mistakes could still mis-record verification detail or labels.
Overview
#197 follow-ups tighten how completion reasons and worker failure text enter the journal, without changing the overall completion state machine.
Completion reason vocabulary now lives on
CompletionReasonviajournal_label()and a hand-maintainedALLlist, with owner-side tests that every label matches serde’ssnake_caseoutput. The duplicatereason_labelmatch inmachine.rsis removed; call sites (e.g. verification fallback detail) usereason.journal_label().Worker failure detail in
complete_out_of_bandcaps rendering before full allocation: large strings are sliced on char boundaries up toMAX_RENDER_BYTES, and non-string JSON uses a budgetedWriteduringserde_json::to_writer, with tests for hostile multi‑MB JSON/strings and a corrected multi-byte char truncation test.Ordering fix:
failure_detailis captured only when the completion is already non-success, via an explicitmatch, and that capture must run before anyreject()so a success completion cannot pick up stale detail and get a bogus “rejected …; worker reported …” string.Reviewed by Cursor Bugbot for commit c32351c. Bugbot is set up for automated code reviews on this repo. Configure here.