control: nested diagnostic value enums carry a retaining decode fallback (closes #79)

Merged
IC
2mo ago
+220-24

Closes #79. Implements the op-enum/value-enum ruling, plus two boundary cases the implementation surfaced that the ruling does not reach.

The fix

RunningImageUnavailableReason and TerminalDisposition become open string enums with a retaining fallback:

_ => Self::Unknown(value)

Both go through one open_string_enum! macro rather than hand-rolled impl pairs. Two users justify it over two copies, and TerminalExitKind on #80 will be a third — one implementation is also the only way the wire-name mappings cannot drift apart in a later edit.

The arm that actually closes #79:

fn unknown_provenance_reason_does_not_discard_healthy_siblings() {
    // 3 modules: one carrying "future_reason", two reporting clean match
    assert_eq!(modules.len(), 3);

That is the measured defect from the issue — a three-module response where one unknown reason took the whole thing down — now asserted to survive intact.

Retention, not discard, and the arm that proves it

#[serde(other)] maps unknowns to a unit variant and discards the string; Unknown(String) keeps it. That retention is what places this inside line 275's intent rather than merely exempt from it — the rule exists so an unknown value and a malformed body do not decode alike, and:

assert!(serde_json::from_str::<RunningImageUnavailableReason>("42").is_err());
assert!(serde_json::from_str::<TerminalDisposition>("{\"value\":\"failed\"}").is_err());

Wrong-typed bodies still fail. Without that arm the fallback would be indistinguishable from the catch-all the rule forbids, so it is tested on both enums rather than one — a macro bug would otherwise surface in only the tested instance.

Classification sweep

Every pub enum in subc-control, classified on receiver-side dispatch — does a consumer change behaviour on the decoded variant (retry, escalate, refuse, branch)? Rendering is not behaviour; the daemon matching on a value it is about to send is producer-side and irrelevant, since the daemon always knows its own variants.

CLOSED (op / dispatched-on)     ClientControlRequest · ClientControlResponse
                                ClientControlPush · RouteCloseReason · PollKind
FALLBACK APPLIED                RunningImageUnavailableReason · TerminalDisposition
OUT OF SCOPE — tagged           ModuleDeclaredProvenance · RunningImageAgreement
                                RunningImageEvidence · SupervisorRouteConsumer
                                StderrCaptureState · StderrTailEntry
OUT OF SCOPE — name collision   SupervisorHealthStatus

A first pass had TerminalDisposition and SupervisorHealthStatus closed on producer-side reasoning; the receiver-side re-check moved both, and the compile errors from adding a variant served as the census — every broken match arm was read rather than suppressed, and all of them render or construct.

Two boundaries the ruling does not reach — both now in the doc

Tagged enums. Six of the qualifying enums are internally-tagged objects, and #[serde(other)] does not apply to them at all. "Add a fallback" does not say what an unknown tag retains: tag only (discards the payload — the exact thing retention exists to prevent), tag plus a serde_json::Value (faithful, but an untyped blob inside a typed wire crate), or tag plus guessed common fields. Three different contracts. Excluded pending a ruling; happy to take it as a follow-up.

An enum that already has a semantic unknown. SupervisorHealthStatus has a real unknown state meaning "health not determined". Adding Unknown(String) forced renaming that variant to UnknownStatus, producing two near-identically-named variants with materially different meanings — an asserted state versus a decoder's limit — plus a source-breaking rename for Rust consumers and loss of Copy. I built it, saw the result, and removed it: it is byte-identical to master in this diff. The doc now carries the rule.

That case is worth the doc line on its own: the fallback pattern collides with any enum that already has an unknown/other/unspecified member, and the collision is invisible at the wire level — both render adjacently, so a future mix-up would not show up in output.

Notes

TerminalDisposition loses Copy (unavoidable once a variant owns a String). Compile census found no dependent call site needing a change — all references construct, record, or render.

ck's render path for Unknown(...) goes through the existing control-character escaping from #59. That was checked deliberately, not assumed: the string is module-influenced, and an unescaped attestation render is how a hostile module forges a verdict with ANSI/OSC sequences.

No TS decoder mirrors these enums, so an open Rust enum does not leave a closed TS twin behind. No golden fixtures changed — existing wire names serialize identically.

The diff is ordered so the ruled change (RunningImageUnavailableReason + doc) is separable from the sweep (TerminalDisposition), if you want one without the other.

Verification

cargo test --workspace                              780 passed  0 failed  1 ignored   (baseline 775, +5)
cargo clippy --workspace --all-targets -D warnings  clean
cargo fmt --all --check                             clean
cargo build --workspace --locked                    clean
check-wire-crate-versions.sh origin/master          6 crates, none unbumped

RED evidence per arm: retention (mutated to Unknown(String::new()) → retention test failed) · wire names (mutated not_running mapping → round-trip failed) · malformed body (widened to a Value deserializer → non-string test failed) · mixed response (rejecting unknown → sibling-preservation failed) · ck escaping (bypassing the escape helper → escaping test failed).


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Fixes #79: unknown diagnostic string values now decode to a retained Unknown(String) fallback, so a supervisor.provenance response with one unrecognized reason no longer fails the whole decode. RunningImageUnavailableReason and TerminalDisposition both get the fallback via one shared open_string_enum! macro.

Scope and side effects

  • TerminalDisposition loses Copy; the compile census found no dependent call sites needing changes.
  • SupervisorHealthStatus stays closed because its semantic unknown variant would collide with the fallback; tagged diagnostic enums stay closed pending a payload-retention ruling.
  • ck escapes unknown reason strings when rendering so a hostile module's value cannot inject control sequences.
  • The protocol doc records the retention rule, its op-enum boundary, and the two excluded cases.
  • Version bumps: subc-control 0.9.0, subc-core 0.11.0; subc-client-rs now depends on subc-control 0.9.