control: tagged diagnostic enums retain unknown tag and body - #82
control: tagged diagnostic enums retain unknown tag and body#82iceteaSA wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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: reorderedA 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:
- Retain the complete ordered object (store the full
Valuemap including the tag member at its original position;serializeemits the stored object verbatim; the tag accessor reads out of the map). With the workspace'spreserve_orderthis 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. - 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.
64bec9a to
40e1763
Compare
|
Fixed at
Verified independently of the test suiteI 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: The Committed arms: leading/middle/trailing on Doc
GatesRe-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. |
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
cubic's 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
Nothing currently breaks (793 green), but the exposure is forward-looking: someone writes a There is a third option, and it makes the constraint freeYour ruling asked for byte-faithful round-trip; I inferred that required Proved it in a scratch crate depending on serde_json with default features only: Same byte-faithfulness, zero workspace impact, contained entirely inside Your call — three options, my recommendation is 3
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 |
|
Option 3 — rebuild it that way. And take the signature shift with it: the ruling's Two acknowledgments your analysis earns:
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. |
40e1763 to
62fd2dd
Compare
|
Rebuilt as option 3 at The feature is actually removed, not just un-declaredManifest line restored to
Shape
Verified independently of the crate's own testsI re-ran the scratch harness that originally proved the defect. It depends on All four of your vectors are in the committed round-trip test as-is, on One finding from the rebuild worth keepingWith
GatesNo 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. |
There was a problem hiding this comment.
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
62fd2dd to
836f5e4
Compare
|
Both findings were valid. Fixed at P1 — the panic was real, and my first attempt to reproduce it produced a false negativeReproduced against Mechanism: Fixed at A note on how I nearly dismissed this. My first reproduction attempt used P2 — nested order was lost, and it refutes a claim I made explicitlyIn 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: 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. GatesRe-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. |
There was a problem hiding this comment.
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
836f5e4 to
8d810f4
Compare
|
Fixed at The defectByte-faithfulness held when the outer value was Alphabetical output — Fixed by decoding nested evidence directly from Scope check — this path is the only oneRather than fix the reported vector, I enumerated which of the six enums can nest another:
Verified with an external harness against the built crate, not only the committed tests: The pattern this PR has now exhibited three timesEach 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 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. GatesTest count is unchanged because the new vectors were added as assertions inside existing round-trip tests rather than as new Base note: Re-requesting review. |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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>
|
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.
8d810f4 to
ce676f6
Compare
|
Rebased onto Versions resolved by taking master's values as the base and bumping only for my own public-API addition: CI will be red, and not for this PR's reason. Master at What I could verify meanwhile, on the rebased branch: 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. |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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>
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.detailis already a deliberateOption<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:
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:
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:
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
Valuebody 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: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_valueescaping added in #59.Shape
Six hand-written
Serialize/Deserializeimpls 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'sopen_string_enum!remains the right call for its uniform shape.)Verification
subc-control0.10.0,subc-client-rs0.11.0,subc-core0.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.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Makes the six diagnostic tagged enums in
subc-controldeserialize unknown tags intoUnknown { 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.Serialize/Deserializeover private wire enums; the affected types dropEqand stayPartialEq.RunningImageEvidenceinside a knownRunningImageAgreement);serde_json preserve_orderstays off.ck.rsrenders unknown tags with both tag and body escaped, so embedded control characters can't reach the terminal.docs/subc-control-protocol.md.subc-controlto 0.11.0 and updates thesubc-client-rsdependency.Written for commit ce676f6. Summary will update on new commits.