Skip to content

kernel: follow-ups from the #196 lens set (vocabulary owner, render bound, temporal contract) #197

Description

@kjgbot

Non-blocking concerns raised by all three lenses against #196 at 3924cf3. Every lens returned REVIEW_PASSED; these are the items they flagged as worth doing at the next natural pass rather than in a diagnostic fix. Filed so they are not lost in PR comments.

1. reason_label is a second owner of the journal vocabulary

machine.rs hand-writes a wildcard-free match from CompletionReason to its journal spelling. That buys compile-time exhaustiveness at a fail-closed boundary — a new variant is a build error until labelled — but it duplicates what rename_all = "snake_case" already derives. every_reason_label_matches_its_serialized_form pins the duplication, and pinning a copy is weaker than not having one.

Both the structure and maintainability lenses landed here independently. Suggested: move the label onto CompletionReason itself, or derive it, so the journal has one spelling owner.

2. The drift test's variant array is hand-maintained

reason_label's match makes a new variant a compile error; that same variant merely missing from the test's array is caught by nothing. The docstring says so honestly, which does not remove the risk. strum::IntoEnumIterator would eliminate the "add it in two places" rule in one line.

Deliberately not done in #196: adding a dependency is not a decision a diagnostic fix should smuggle in. It is a decision worth making on its own.

3. worker_failure_detail renders before it truncates

let rendered = match output {
    Value::String(text) => text.clone(),
    other => other.to_string(),
};

A worker shipping a multi-megabyte JSON object gets fully rendered on the completion path before anything is measured or cut — the exact "bloat the journal" case the docstring claims to guard. The journal write is bounded; the allocation is not.

The docstring overstates the guarantee, which is the part I want fixed regardless of whether the render is capped. Either tighten it to say "bounded in the journal, not in memory", or cap the render. Not a security issue while workers are inside the trust boundary, but the comment should not promise more than the code does.

4. The capture's ordering constraint is implicit

let mut failure_detail = failure_reason.is_some().then(|| worker_failure_detail(...)).flatten();

This must run before any reject() call. Today it does, by construction. If a future edit moved the initialization below the closure, reject would find a non-None detail and format "rejected: …; worker reported: …" for a completion whose original reason was Success. A plain if let Some(_) = failure_reason { … } else { None } reads identically and makes the ordering constraint self-evident.

5. machine.rs is at 502 lines

AGENTS.md rule 1 calls 500 a design smell. reason_label tipped it over. Not a defect, but it is the nudge — and relocating that function per item 1 also resolves this.

6. Two accounts packed into one detail string

VerificationRecord has only detail, so the rejection and the worker's report are joined into one string. Mild schema flattening. Noted by the structure lens as worth a passing thought, not a change.


Related: #195 (the defect #196 fixes), #189 (where it surfaced).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions