Skip to content

control: tagged diagnostic enums retain unknown tag and body - #82

Open
iceteaSA wants to merge 1 commit into
cortexkit:masterfrom
iceteaSA:feat/tagged-enum-fallback
Open

control: tagged diagnostic enums retain unknown tag and body#82
iceteaSA wants to merge 1 commit into
cortexkit:masterfrom
iceteaSA:feat/tagged-enum-fallback

Conversation

@iceteaSA

@iceteaSA iceteaSA commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Implements the tagged-enum contract ruled on #81, closing the boundary parked when that PR landed the string-enum half.

The contract, as ruled

Unknown { tag: String, body: serde_json::Value } — retain both. Tag-only would discard exactly what retention exists to keep; guessed common fields would be a schema invented by the consumer. The untyped-region objection was mine and the maintainer answered it from the crate's own precedent: ErrorBody.detail is already a deliberate Option<Value> on this wire, so a bounded labeled untyped region is established practice here rather than a new concession.

Six internally-tagged diagnostic enums, all nested in list responses:

ModuleDeclaredProvenance   RunningImageAgreement   RunningImageEvidence
SupervisorRouteConsumer    StderrCaptureState      StderrTailEntry

Classification re-verified by the receiver-side test — does a receiver change behaviour on the decoded variant? Rendering and storing are not behaviour. No receiver dispatches on any of the six; none stays closed. Op enums are untouched.

The two constraints, both load-bearing

Byte-faithful round-trip is an exact text assertion, not a structural one:

let wire = r#"{"kind":"future_consumer","detail":{"z":1}}"#;
let decoded: SupervisorRouteConsumer = serde_json::from_str(wire).unwrap();
assert_eq!(serde_json::to_string(&decoded).unwrap(), wire);

Field order and the nested object survive. RED-proved by omitting the retained body: the assertion fails tag-only against expected payload.

Malformed still fails — this is the arm that keeps the change inside the no-catch-all rule rather than exempt from it. A fallback that swallows a number or an array is the catch-all the rule forbids:

for wire in ["42", r#""future""#, "[]"] {
    assert!(serde_json::from_str::<SupervisorRouteConsumer>(wire).is_err());
    assert!(serde_json::from_str::<StderrCaptureState>(wire).is_err());
}

Two enums, so a per-enum mistake cannot hide. RED-proved by temporarily accepting non-objects.

Security: a wider surface than the string half

A Value body can carry arbitrary strings, so this is a bigger attack surface than #81's string enums. Verified rather than assumed, with real payloads — OSC 52 clipboard-write and screen-clear:

"status": "future_status\u{1b}]52;c;AAAA\u{07}",
"detail": "future detail\u{1b}[2J"
...
assert!(!rendered.bytes().any(|byte| byte < 0x20));

That final assertion is a universal claim over the whole rendered string, not a spot check for the escaped form. Both tag and body route through the existing provenance_value escaping added in #59.

Shape

Six hand-written Serialize/Deserialize impls with private derive-wire enums, not a macro. Tagged variants carry distinct fields, so a generic macro would need real token parsing and could silently drop or reorder them — six auditable impls beat one clever macro. (The string half's open_string_enum! remains the right call for its uniform shape.)

Verification

tests    792 passed, 0 failed, 1 ignored   (baseline 787 on eb49bd5b, +5)
clippy   clean -D warnings
fmt      clean
build    --locked clean
wire     6 crates examined, none changed without a bump

subc-control 0.10.0, subc-client-rs 0.11.0, subc-core 0.12.0, lock updated in the same commit. No golden fixtures changed — known wire shapes are byte-identical. No TypeScript mirror exists for these six.

Deploy note

Not urgent to land. A module restart wave is in progress on this box and hub's constraint set holds wire changes until it settles — this PR is subject to that like anything else. Review at leisure.


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

Makes the six diagnostic tagged enums in subc-control deserialize unknown tags into Unknown { tag, body } instead of erroring, so clients tolerate future discriminator values without discarding the payload. Known tags still round-trip byte-identically; non-object payloads still fail.

  • Replaces derived impls with hand-written Serialize/Deserialize over private wire enums; the affected types drop Eq and stay PartialEq.
  • Retains the body as recursively ordered JSON pairs taken directly from the wire, so byte-faithful round-trip holds even when an unknown value is nested inside a known ancestor (an unknown RunningImageEvidence inside a known RunningImageAgreement); serde_json preserve_order stays off.
  • Rejects duplicate discriminator fields as decode errors instead of reaching an unreachable arm.
  • ck.rs renders unknown tags with both tag and body escaped, so embedded control characters can't reach the terminal.
  • Documents the object-retention fallback rule in docs/subc-control-protocol.md.
  • Bumps subc-control to 0.11.0 and updates the subc-client-rs dependency.

Written for commit ce676f6. Summary will update on new commits.

Review in cubic

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 6 files

Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.

Re-trigger cubic

Comment thread crates/subc-control/src/lib.rs Outdated
Comment thread crates/subc-control/src/lib.rs Outdated

@subc-alfonso subc-alfonso Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by execution. The shape is right (six auditable impls over a clever macro — agreed, and the ruling's constraints are both present with RED evidence), but cubic's P2 is REAL and I confirmed it by execution rather than reading:

let wire = r#"{"detail":{"z":1},"kind":"future_consumer"}"#;
// decode → re-encode yields {"kind":"future_consumer","detail":{"z":1}}
// assertion `left == right` failed: reordered

A producer that emits the discriminator anywhere but first gets its wire text rewritten, so the byte-faithful claim currently holds only for the field order the tests chose — a claim true of the fixtures rather than the property. Two acceptable resolutions, your pick:

  1. Retain the complete ordered object (store the full Value map including the tag member at its original position; serialize emits the stored object verbatim; the tag accessor reads out of the map). With the workspace's preserve_order this makes byte-faithfulness true for ANY member order, and the round-trip test should then include a tag-not-first vector so the property is pinned where it was weakest.
  2. Weaken the claim to match the mechanism: document the round-trip as semantic-faithful with tag-first normalization, and change the exact-text assertion accordingly. Honest, but option 1 is barely more code and makes the stronger contract true, so I'd take 1.

Also taking cubic's P3: docs/subc-control-protocol.md still describes the tagged enums as closed — the doc moved for #81's string half and should move here too, same section.

Everything else verified from my seat: receiver-side classification re-check clean, malformed-still-fails on two enums (the per-enum-mistake argument is right), the OSC-52/screen-clear render test's universal below-0x20 assertion is the correct form, and the version bumps are already collision-safe against master. Re-request review when the revision lands — the twin matrix runs on your push.

@iceteaSA
iceteaSA force-pushed the feat/tagged-enum-fallback branch from 64bec9a to 40e1763 Compare August 28, 2026 20:47
@iceteaSA

Copy link
Copy Markdown
Collaborator Author

Fixed at 40e17631. Took option 1 — retain the complete ordered object.

Unknown.body now stores the full object including the discriminator at its original position, and serialization emits that object directly. tag stays a decoded projection rather than a separately-stored field that gets re-prepended. Byte-faithfulness is now true by construction rather than by a re-insertion rule someone has to keep correct.

Verified independently of the test suite

I proved the defect with a scratch harness against the built crate rather than by reading, so I re-proved the fix the same way — including a vector that is not in the committed tests:

leading   faithful=true     {"kind":"future_consumer","detail":{"z":1}}
trailing  faithful=true     {"detail":{"z":1},"kind":"future_consumer"}
middle    faithful=true     {"a":1,"kind":"future_consumer","b":2}
deep      faithful=true     {"a":{"n":[1,2]},"kind":"future_x","zz":"s","b":null}

malformed still rejected: true

The deep case — nested array, null, mixed key types around a middle discriminator — was invented for this check precisely because the committed vectors are the author's own choice, which is the failure this whole round was about.

Committed arms: leading/middle/trailing on SupervisorRouteConsumer, leading/trailing on StderrCaptureState, with the non-leading case RED-proved against 64bec9ac first.

Doc

docs/subc-control-protocol.md:277 now carries the tagged contract in the same section as #81's string half — Unknown { tag, body }, byte-faithful for any member order, malformed still fails, covers the six nested diagnostic enums, does not reopen op enums. The string-enum text and the semantic-unknown collision sub-case are intact.

Gates

tests    793 passed, 0 failed, 1 ignored   (792 before this fix)
clippy   clean -D warnings
fmt      clean
build    --locked clean
wire     6 crates examined, none changed without a bump

Re-requesting review — the twin matrix runs on this push.

One note on your framing, because it is better than mine and worth keeping: "a claim true of the fixtures rather than the property." My round-trip test was not wrong, it was narrow, and the narrowness was invisible because I chose the example — and the natural example to reach for is the one that works. The general form is that a faithfulness test written by the same person who wrote the serializer inherits that person's mental model of the input space. Worth widening deliberately rather than waiting for a reviewer to supply the case you would not have picked.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 7 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread crates/subc-control/Cargo.toml Outdated
@iceteaSA

Copy link
Copy Markdown
Collaborator Author

cubic's preserve_order P2 is valid, and the reason it got in is my error — I told the implementer the feature was already enabled after reading it in the worktree that already contained their change. I verified a fact about the branch and reported it as a fact about the baseline.

origin/master                serde_json = "1"
this branch                  serde_json = { version = "1", features = ["preserve_order"] }

So the "it's already there, byte-faithfulness is free" premise I handed down was false, and the cost landed unexamined.

The unification is real — confirmed empirically, not from the docs

$ cargo tree -e features -p subc-core
  serde_json feature "indexmap"
  indexmap feature "default"

subc-core declares plain serde_json = "1" and still builds against an IndexMap-backed Value, because Cargo resolves one serde_json with the union of features. Same for subc-protocol, subc-transport, subc-mcp. A PR about enum fallbacks silently changes Value map ordering in four crates that never opted in.

Nothing currently breaks (793 green), but the exposure is forward-looking: someone writes a Value-based golden in subc-core, it passes under insertion order, and it fails the day this feature is removed — with the cause four crates away in an unrelated manifest.

There is a third option, and it makes the constraint free

Your ruling asked for byte-faithful round-trip; I inferred that required preserve_order. That inference is wrong. JSON member order is present in the deserializer streamMapAccess yields entries in document order. It is serde_json::Value's Map that discards it when backed by BTreeMap. A custom Deserialize capturing Vec<(String, Value)> keeps the order without any feature change.

Proved it in a scratch crate depending on serde_json with default features only:

plain Value round-trip:
  in:  {"zeta":1,"alpha":2}
  out: {"alpha":2,"zeta":1}          order lost, as expected

ordered-capture round-trip (no preserve_order):
  faithful=true   {"kind":"future_consumer","detail":{"z":1}}
  faithful=true   {"detail":{"z":1},"kind":"future_consumer"}
  faithful=true   {"a":1,"kind":"future_x","b":2}

Same byte-faithfulness, zero workspace impact, contained entirely inside subc-control's Unknown variant.

Your call — three options, my recommendation is 3

  1. Keep preserve_order — declare it explicitly in each affected crate rather than relying on unification, and audit Value-based goldens across the four. Honest, but it makes an unrelated global change permanent.
  2. Weaken to semantic-faithful (your option 2 from the review) — no feature change, weaker contract.
  3. Ordered capture inside the variant — full byte-faithfulness, no feature change, no crate-wide effect. Costs a hand-written Deserialize for the Unknown arm, which is in keeping with the six hand-written impls already here.

I'd take 3: it gives you the stronger contract you ruled for and removes the cost that made it questionable. Say the word and I'll rebuild it that way; the current push stands if you'd rather take 1 with the audit.

One note on Unknown { tag, body } under option 3: body becomes the ordered pair-list rather than a Value, so the public shape shifts slightly from your ruling. If you want Value preserved in the signature, option 1 with explicit declarations is the honest version and I'll do the audit.

@subc-alfonso

subc-alfonso Bot commented Aug 29, 2026

Copy link
Copy Markdown

Option 3 — rebuild it that way. And take the signature shift with it: the ruling's body: serde_json::Value was a means (retain everything the producer sent), never an end — an ordered pair-list is a strictly better means since it retains the one thing Value structurally discards. The Unknown variant has zero consumers today, so the shape is free to be right before anyone depends on it.

Two acknowledgments your analysis earns:

  1. My review carried the same false premise as your implementer brief — "with the workspace's preserve_order" asserted a baseline fact that was actually a branch fact (the feature I recalled lives in agent-token-vectors, not the workspace). You verified a fact about the branch and reported it as the baseline; I recalled a fact about one crate and reported it as the workspace. Same error, opposite directions, and your empirical cargo tree -e features check is what a review-time falsifier for that claim looks like.

  2. The unification exposure is the sharper finding than the fix: a features-union change in one manifest silently altering Value semantics in four crates that never opted in — passing today, failing the day the feature is removed, with the cause four crates away — is the piggyback class at the Cargo layer. Your option 3 doesn't just avoid the cost, it removes the mechanism.

The scratch-crate proof with the three vectors (including tag-not-first and interleaved members) is exactly the evidence shape the revision needs — bring those vectors into the round-trip test as-is. Everything else from my review stands; re-request when the rebuild lands and the twin matrix will run on it.

@iceteaSA
iceteaSA force-pushed the feat/tagged-enum-fallback branch from 40e1763 to 62fd2dd Compare August 29, 2026 06:46
@iceteaSA

Copy link
Copy Markdown
Collaborator Author

Rebuilt as option 3 at 62fd2dd8. preserve_order is gone; byte-faithfulness now holds without it.

The feature is actually removed, not just un-declared

Manifest line restored to serde_json = "1", and the diff against master on that file is now empty. The load-bearing check is the resolved graph, not the manifest:

$ cargo tree -e features -p subc-core | grep 'serde_json feature "indexmap"'
  0 occurrences

Value map semantics in subc-core, subc-protocol, subc-transport, and subc-mcp are back to what they were before this PR existed.

Shape

OrderedJsonObject(Vec<(String, serde_json::Value)>) with a custom MapAccess deserializer capturing top-level member order, and an ordered Serialize that emits it verbatim. Unknown.body holds the complete object including the discriminator at its original position; tag is its projection. Nested values stay serde_json::Value — nested order never enters the byte-faithfulness question, since nested objects re-serialize from whatever they decoded into.

Verified independently of the crate's own tests

I re-ran the scratch harness that originally proved the defect. It depends on subc-control with default serde_json features, and its own tree confirms indexmap absent — so this is faithfulness demonstrated with the feature genuinely gone, not merely un-requested:

leading   faithful=true
trailing  faithful=true
middle    faithful=true
deep      faithful=true
malformed still rejected: true

All four of your vectors are in the committed round-trip test as-is, on SupervisorRouteConsumer and StderrCaptureState.

One finding from the rebuild worth keeping

With preserve_order removed but before the pair-list landed, the implementer measured which vectors actually went RED:

middle   RED
deep     RED
leading  RED
trailing PASSED

{"detail":…,"kind":…} is already BTreeMap-sorted (d < k), so the trailing vector passes accidentally in the no-feature world. The vector that originally exposed the bug would have been vacuous as its own regression test. That is the same trap one layer down — a test whose fixture happens to satisfy the property by coincidence rather than by mechanism — and it is why all four vectors are pinned rather than the one that first failed.

Gates

tests    795 passed, 0 failed, 1 ignored   (793 before)
clippy   clean -D warnings
fmt      clean
build    --locked clean
wire     6 crates examined, none changed without a bump
tree     serde_json feature "indexmap" ABSENT from subc-core

No golden fixture changed or loosened — I asked specifically whether removing the feature let any order-insensitive test tighten back, and none had been loosened for it in the first place.

Re-requesting review.

On the mirrored premise error: worth noting the asymmetry in how the two of us could have caught it. Yours was a recall claim ("the workspace has preserve_order") — checkable only by going and looking. Mine was a read claim, and I did look — at the wrong tree. Neither of us was careless; both of us were confident about the right kind of fact from the wrong scope. cargo tree -e features is the falsifier for both directions, because it reports the resolved graph rather than any single manifest or memory of one.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 7 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread crates/subc-control/src/lib.rs
Comment thread crates/subc-control/src/lib.rs Outdated
@iceteaSA

Copy link
Copy Markdown
Collaborator Author

Both findings were valid. Fixed at 836f5e4c.

P1 — the panic was real, and my first attempt to reproduce it produced a false negative

Reproduced against 62fd2dd8, then re-verified after the fix with the same external harness:

                                                        before      after
{"status":"reported","status":"unverifiable"}           PANIC       DecodeErr
{"status":"unverifiable","status":"reported"}           DecodeErr   DecodeErr
{"status":"reported","build":{},"status":"unverifiable"} PANIC      DecodeErr
{"status":"unverifiable","build":{},"status":"reported"} PANIC      DecodeErr

controls
{"status":"unverifiable"}                               Ok          Ok
{"status":"future_thing"}                               Ok          Ok

Mechanism: read_tagged scans manually and takes the FIRST occurrence of the discriminator; serde_json::from_value into the derive-tagged wire enum takes the LAST. On a duplicate key the two stages disagree, so the arm documented as impossible executes. RFC 8259 permits duplicate names, so this is well-formed JSON any module can emit — and it unwinds rather than returning an error. The daemon's per-connection task contains it; ck and SDK consumers take an abort mid-read.

Fixed at read_tagged, not at the two arms. Rejecting a duplicate discriminator at the tag-read refuses the malformed frame once for all six decoders and removes the first-wins/last-wins split entirely. Patching the two unreachable!() sites would have cured the symptom while leaving the disagreement live for the next two-stage decode added.

A note on how I nearly dismissed this. My first reproduction attempt used "kind" as the discriminator and returned clean on every vector — which reads as a refutation. It wasn't: the key is "status", and the tell was that my controls also failed. A control that fails means the experiment has not discriminated; it has only shown that nothing worked. I would have reported this finding as unreproducible on the strength of a test that never exercised the path.

P2 — nested order was lost, and it refutes a claim I made explicitly

{"kind":"future_x","detail":{"zeta":1,"alpha":2}}   ->  {"detail":{"alpha":2,"zeta":1}}
{"kind":"future_x","d":{"b":{"zz":1,"aa":2}}}       ->  {"d":{"b":{"aa":2,"zz":1}}}

In the rebuild brief I wrote that only top-level order matters for byte-faithfulness, invited contradiction, and the implementer looked and agreed with me. Two of us confirmed a wrong claim — agreement between an author and the person that author briefed is not independent verification, which is precisely what the review caught.

Took option (a) — recursive ordered retention rather than narrowing the documented claim. The ruling said byte-faithful; this round has been a sequence of claims that held only for the fixtures chosen, and narrowing would have shipped a third, weaker claim after two corrections rather than making the original one true.

Verified with vectors that are not in the committed test set, since the recurring failure here is fixtures chosen by the same person who wrote the code:

faithful=true   {"kind":"future_x","a":[{"zz":1,"aa":2}]}                 unsorted object inside an array
faithful=true   {"kind":"future_x","z":{"y":{"x":{"cc":1,"bb":2,"aa":3}}}} three levels, each unsorted
faithful=true   {"kind":"future_x","l":[{"q":1,"a":2},{"z":3,"b":4}]}      array of two unsorted objects
faithful=true   {"kind":"future_x","m":{"zz":null,"aa":true,"mm":1.5}}     mixed null/bool/float

The guarantee now reads: unknown diagnostic bodies round-trip byte-faithfully at any nesting depth and member order; duplicate, missing, and non-string discriminators and non-object bodies all reject.

Gates

tests     797 passed, 0 failed, 1 ignored   (795 before)
clippy    clean -D warnings
fmt       clean
build     --locked clean
wire      6 crates examined, none unbumped
tree      serde_json feature "indexmap" ABSENT from subc-core

Re-requesting review.

One observation across the four rounds this PR has taken, since the pattern is more useful than any individual fix: every finding has been a claim that was true of the example it was tested against. Tag-leading order. Top-level-only nesting. An arm unreachable given the inputs someone thought to try. In each case the code was correct for the case in mind and the test encoded the same blind spot, because the same person chose both. The external harness has been worth more than the committed tests at catching these — not because it is better written, but because writing it forced me to pick inputs after the implementation rather than alongside it.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 7 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread crates/subc-control/src/lib.rs
@iceteaSA

Copy link
Copy Markdown
Collaborator Author

Fixed at 8d810f43. The finding was valid and reproduced before fixing.

The defect

Byte-faithfulness held when the outer value was Unknown and broke when the outer variant was known:

in:  {"status":"match","evidence":{"method":"future_probe","zz":1,"aa":2}}
out: {"status":"match","evidence":{"aa":2,"method":"future_probe","zz":1}}

Alphabetical output — BTreeMap ordering. A known tag decoded through into_value()serde_json::Value, so a nested Unknown was reconstructed from an already-sorted map and its retained order died before the nested enum's Deserialize ever ran.

Fixed by decoding nested evidence directly from OrderedJsonValue rather than round-tripping through Value; the lossy conversion remains only for order-insensitive leaves.

Scope check — this path is the only one

Rather than fix the reported vector, I enumerated which of the six enums can nest another:

RunningImageAgreement    evidence, running, disk  -> RunningImageEvidence
ModuleDeclaredProvenance none
SupervisorRouteConsumer  none
StderrCaptureState       none
StderrTailEntry          none

RunningImageAgreement is the only one, via three fields. The reported vector exercised evidence alone, so I verified Mismatch's running + disk independently — both faithful, two unknown siblings under one known parent.

Verified with an external harness against the built crate, not only the committed tests:

Match.evidence         faithful=true
Mismatch.running+disk  faithful=true
known/known            faithful=true    control
nested string enum     faithful=true    control

The pattern this PR has now exhibited three times

round 1   byte-faithful   held only for tag-LEADING input
round 2   byte-faithful   held only for TOP-LEVEL members
round 3   byte-faithful   held only when the OUTER value is Unknown

Each round the code was correct for the case in mind and the tests encoded the same blind spot, because the same person chose both. This round is the sharpest instance: every test we wrote put Unknown at the outer level, since that is the variant the feature is about — while a real supervisor.provenance response returns known agreements containing evidence, and the evidence is what carries a future discriminator. The tested position and the production position were different ones, and nothing in the test suite could notice.

I do not have a general fix for that beyond what happened here: an external reviewer choosing inputs the author would not. Worth stating plainly since it is the third occurrence rather than a one-off.

Gates

tests     797 passed, 0 failed, 1 ignored
clippy    clean -D warnings
fmt       clean
build     --locked clean
wire      passes against current origin/master (41cb2be4)
tree      serde_json feature "indexmap" ABSENT

Test count is unchanged because the new vectors were added as assertions inside existing round-trip tests rather than as new #[test] functions.

Base note: origin/master has moved to 41cb2be4 (adding self_signals to ModuleManifest, protocol 0.14.0 / control 0.10.0 / core 0.12.0). This branch is still on the older base and the wire-version gate passes against the new master regardless. I will rebase when you are ready to merge rather than churn the branch mid-review — say the word.

Re-requesting review.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 7 files (changes from recent commits).

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="crates/subc-control/src/lib.rs">

<violation number="1" location="crates/subc-control/src/lib.rs:815">
P2: Unknown bodies containing non-canonical JSON scalar spellings are not byte-faithful: `visit_f64` loses the numeric lexeme and `visit_str` loses escape spelling. Preserve raw scalar representations, or narrow the exact-round-trip contract and test that behavior.</violation>
</file>

Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

where
E: serde::de::Error,
{
serde_json::Number::from_f64(value)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Unknown bodies containing non-canonical JSON scalar spellings are not byte-faithful: visit_f64 loses the numeric lexeme and visit_str loses escape spelling. Preserve raw scalar representations, or narrow the exact-round-trip contract and test that behavior.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/subc-control/src/lib.rs, line 815:

<comment>Unknown bodies containing non-canonical JSON scalar spellings are not byte-faithful: `visit_f64` loses the numeric lexeme and `visit_str` loses escape spelling. Preserve raw scalar representations, or narrow the exact-round-trip contract and test that behavior.</comment>

<file context>
@@ -570,25 +604,690 @@ pub enum StderrCaptureState {
+            where
+                E: serde::de::Error,
+            {
+                serde_json::Number::from_f64(value)
+                    .map(OrderedJsonValue::Number)
+                    .ok_or_else(|| E::custom("non-finite JSON number"))
</file context>

@subc-alfonso

subc-alfonso Bot commented Aug 30, 2026

Copy link
Copy Markdown

Both fixes verified from the diff and the round-trip evidence — the nested-unknown-under-known-variant catch closes the last byte-faithfulness hole, and the panic repro discipline (false-negative first attempt disclosed) is exactly the standard.

One mechanical blocker before merge: the PR is now CONFLICTING against master (today's version bumps landed — subc-control 0.9.0 / subc-core 0.13.0), and a conflicted PR receives no workflow runs at all (no merge ref exists — it looks identical to a CI outage, so this comment is the only signal you get). Rebase over master and the twin CI + merge fire on green from my side.

Unknown internally tagged diagnostic values retain recursively ordered pairs captured directly from the deserializer stream. Known running-image agreements now decode nested evidence from that ordered representation instead of converting through serde_json::Value, so byte-faithfulness holds wherever an unknown value appears relative to a known ancestor. Duplicate discriminators are rejected during tag reading before a known-tag decoder can reach a mismatched unreachable arm; non-object bodies remain decode errors. ErrorBody.detail establishes the crate's deliberate use of an untyped Option<Value> region for wire compatibility. serde_json preserve_order is deliberately not enabled, so workspace-wide Value semantics remain unchanged.

CONSUMER-IMPACT: receivers tolerate future tags in the six diagnostic object enums with recursively ordered body retention through known ancestors, duplicate-discriminator rejection, and unchanged known-tag behavior; subc-control 0.10.0, subc-client-rs 0.11.0, and subc-core 0.12.0.
@iceteaSA
iceteaSA force-pushed the feat/tagged-enum-fallback branch from 8d810f4 to ce676f6 Compare August 30, 2026 22:49
@iceteaSA

Copy link
Copy Markdown
Collaborator Author

Rebased onto 8cad75ecce676f6d. Conflicts were three version-cascade sites (Cargo.lock, subc-client-rs/Cargo.toml, subc-core/Cargo.toml); subc-control/src/lib.rs auto-merged clean, so the change itself is untouched by the rebase.

Versions resolved by taking master's values as the base and bumping only for my own public-API addition:

subc-protocol    0.16.0   master, untouched
subc-control     0.10.0 → 0.11.0   my change (Unknown variants on the tagged enums)
subc-client-rs   0.10.1   master, untouched
subc-core        0.13.0   master, untouched

CI will be red, and not for this PR's reason. Master at 8cad75ec does not compile — provenance.rs:515 passes the new TestTempDir guard to fs::remove_dir_all. Details and the missed-site check are on #85; short version is that I reproduced it on a detached worktree at 8cad75ec with none of my changes present, and my branch's provenance.rs is byte-identical to master's.

What I could verify meanwhile, on the rebased branch:

cargo test -p subc-control                              20 passed, 0 failed
cargo test -p subc-protocol -p subc-transport -p subc-client-rs   105 passed, 0 failed

So everything this PR touches is green; the red target is one my diff does not reach. Once master compiles I will re-run the full workspace gate and post the number rather than assuming it.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 existing issue remains and 2 new issues found across 6 files (changes from recent commits).

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="crates/subc-control/src/lib.rs">

<violation number="1" location="crates/subc-control/src/lib.rs:475">
P2: The new public `Unknown` variants do not implement the documented `body: serde_json::Value` API. Expose `serde_json::Value` for `body` (using an order-preserving serde_json configuration if required) or update the public contract and downstream API accordingly.</violation>

<violation number="2" location="crates/subc-control/src/lib.rs:966">
P2: When a known tagged value repeats a payload field, this parser silently chooses one value instead of rejecting malformed input. Reject duplicate non-discriminator fields before `ordered_field` or `into_value` selects a value.</violation>
</file>

Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

fn ordered_field<'a>(body: &'a OrderedJsonObject, field: &str) -> Option<&'a OrderedJsonValue> {
body.as_entries()
.iter()
.find_map(|(key, value)| (key == field).then_some(value))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: When a known tagged value repeats a payload field, this parser silently chooses one value instead of rejecting malformed input. Reject duplicate non-discriminator fields before ordered_field or into_value selects a value.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/subc-control/src/lib.rs, line 966:

<comment>When a known tagged value repeats a payload field, this parser silently chooses one value instead of rejecting malformed input. Reject duplicate non-discriminator fields before `ordered_field` or `into_value` selects a value.</comment>

<file context>
@@ -570,25 +604,690 @@ pub enum StderrCaptureState {
+fn ordered_field<'a>(body: &'a OrderedJsonObject, field: &str) -> Option<&'a OrderedJsonValue> {
+    body.as_entries()
+        .iter()
+        .find_map(|(key, value)| (key == field).then_some(value))
+}
+
</file context>

/// is its decoded discriminator projection.
Unknown {
tag: String,
body: OrderedJsonObject,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: The new public Unknown variants do not implement the documented body: serde_json::Value API. Expose serde_json::Value for body (using an order-preserving serde_json configuration if required) or update the public contract and downstream API accordingly.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/subc-control/src/lib.rs, line 475:

<comment>The new public `Unknown` variants do not implement the documented `body: serde_json::Value` API. Expose `serde_json::Value` for `body` (using an order-preserving serde_json configuration if required) or update the public contract and downstream API accordingly.</comment>

<file context>
@@ -450,26 +454,33 @@ pub struct SupervisorRoute {
+    /// is its decoded discriminator projection.
+    Unknown {
+        tag: String,
+        body: OrderedJsonObject,
+    },
 }
</file context>

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant