Tag all Rig-owned public Serde enums - #2281
Open
gold-silver-copper wants to merge 8 commits into
Open
Conversation
gold-silver-copper
marked this pull request as ready for review
August 11, 2026 19:39
…r implicit tagging Clears the four #2281 review findings (B-1..B-4 in the cassette-suite proposal). ## B-1 — Cohere tool_choice asymmetry, resolved against the live wire The review reported that `Required`/`None` skip the advertised-tools check that `Specific` performs, and predicted a remote 400. Rather than assume, both were probed live. The first probe (command-r-plus-08-2024) returned 200 for both and looked like a false report — but that model ignores tool_choice entirely. On a current model the wire is asymmetric: command-a-03-2025, tool_choice with no `tools`: REQUIRED -> 400 invalid request: tool_choice 'required' can only be specified if 'tools' are specified NONE -> 200 So `Required` is guarded (it would otherwise ship `{"tool_choice":"REQUIRED"}` with no `tools` key, since `tools` carries `skip_serializing_if = "Vec::is_empty"`), and `None` is deliberately NOT guarded — "call nothing" is meaningful without a tool list and the provider honours it. That answers the open question the finding left. A unit test pins both halves and records the wire evidence; the `Required` half cannot be a cassette because the request never leaves. Noted in passing, not fixed here: `command-r-plus` now returns `404 model 'command-r-plus' was removed on September 15, 2025`, and rig still exports it plus five siblings. That is issue #2241 / PR #2263's territory. ## B-3 — a wall that guards the drift this PR fixes `domain_serde_policy` flagged only `#[serde(untagged)]`. Every enum this PR retagged carried no such attribute — implicit external tagging is a *default*, not an attribute, and a blacklist cannot see a default. All of them would have passed the wall unchanged. Adds the complement: `rig_owned_public_data_bearing_enums_declare_a_representation` requires every rig-owned public data-bearing Serde enum to declare `tag`, `tag`+`content`, or an allowlisted `untagged`. Fieldless enums are exempt (they serialize as plain strings). `src/providers/` is excluded structurally as provider-owned wire shape, and `rig-bedrock/src/types/converse_output.rs` — 37 enums mirroring the aws-sdk Converse types one-for-one — is exempt by path with a written justification and a staleness check. An unparsed enum body is now reported rather than skipped: a silent skip reads as a pass, which was one of the two ways the original scanner could go blind. Validated by falsification, per the plan: run against `03643160^` it flags exactly six — MediaType, ToolChoice, FinishReason, Filter, ToolCallDeltaContent, ModelListingError — the five the review named plus one it missed. Green on this tree. ## B-2 / B-4 — MIGRATING.md Drops `"provider":null` from both columns of the `StreamedUserContent` row: `ToolResult::provider` carries `skip_serializing_if = "Option::is_none"`, so rig never emits that key and the documented "exact wire shape" was unmatchable. Extends the adjacency rule to struct variants, which are most of the retagged stream enums. Verified against the derive rather than asserted: the enums use `#[serde(tag = "type", content = "content")]`, and serde's adjacent tagging nests every variant kind under `content`, while the internally-tagged `ToolChoice`/`ModelListingError` keep their fields top-level. rig-core: 1249 lib tests, 4 policy tests, clippy clean.
Review of the wall I added in e9e370a found three ways it could be defeated or made to cry wolf. All three reproduced before fixing; each now has a regression test. 1. Prose defeated it silently. `attribute_block_before` deliberately included doc-comment lines, and the callers probe that block with `contains("tag")`. Any data-bearing enum whose rustdoc contained `tag` as a substring — `stage`, `advantage`, `vintage`, or the word "tagged" — exempted itself. Verified: an enum documented "Two-stage handshake result" with no representation was NOT flagged, which is exactly the regression class the wall exists to catch. The block now collects attribute lines only. 2. A blank line between `#[derive(...)]` and `pub enum` — legal Rust — emptied the block, failed the `Serialize` probe, and skipped the enum entirely. Blank lines and doc comments are now skipped rather than ending the block; only real code ends it. 3. Both brace scanners counted braces inside comments. A variant documented ``/// Rendered as `{`.`` either pinned `depth` above zero for the rest of the enum (every later variant reads as nested, the enum looks fieldless, and it is exempted) or ran `enum_body` past the closing brace and reported a spurious `UNPARSED BODY` failure — a CI break for a contributor whose only sin was a brace in prose. Both now strip line comments first, with a string-literal-aware stripper so `"http://x"` is not truncated. Falsification re-run after the change, since it altered the attribute parsing: against `03643160^` the wall still flags exactly the same six enums (MediaType, ToolChoice, FinishReason, Filter, ToolCallDeltaContent, ModelListingError) and stays green on this tree — so the stricter parsing neither lost coverage nor started hiding anything. rig-core: 1249 lib tests, 5 policy tests, clippy clean.
…text
Both scanners in this file were text heuristics, and both were defeated in
review. Patching them again would only trade one substring hazard for the next:
the previous fix stopped `contains("tag")` seeing doc comments, but
`#[serde(rename = "stage")]` is an *attribute*, so it would still have fooled the
probe. The parser is the only thing that agrees with rustc about what an
attribute is.
`syn::parse_file` now drives both checks, per the stated policy:
1. parse every scanned .rs file
2. visit every ItemEnum, descending inline `mod { … }` blocks — rig declares
whole provider wire surfaces in them (`pub mod gemini_api_types`), and a
top-level-only scan silently exempted every enum inside
3. select `Visibility::Public` only
4. read serde-ness from `#[derive(...)]` entries only, comparing the path's
last segment so `serde::Serialize` counts
5. data-bearing = any variant whose fields are not `Fields::Unit`
6. read the representation from enum-level `#[serde(...)]` metadata *keys*
only — `tag = "type"` yields the key `tag`, while a rename *value* of
"untagged" or "stage" yields nothing. That distinction is exactly what a
substring probe cannot make
7. a parse failure panics; a file the scanner cannot read is a file the policy
is not enforcing and must never read as compliance
Deletes `attribute_block_before`, `strip_line_comment`, `enum_body`,
`has_data_bearing_variant` and `public_untagged_enums` — every text heuristic in
the file, including the ones added one commit ago.
The unit tests now assert on syntax the old scanners got wrong: prose containing
`stage`, a blank line between derive and declaration, `rename_all` (a key that is
not a representation), `rename = "untagged"` (a value that spells one), a brace
in a doc comment, a fieldless enum, a non-serde enum, a private enum, and an enum
inside an inline module. Plus a direct test that an unparsable file fails.
Falsification re-run, since the scanner changed entirely: against `03643160^` it
still flags exactly the same six enums — MediaType, ToolChoice, FinishReason,
Filter, ToolCallDeltaContent, ModelListingError — and stays green here, with the
18-entry untagged allowlist unchanged and no stale rows.
Adds syn (full, parsing) and proc-macro2 (span-locations, for real line numbers
in violation reports) as rig-core dev-dependencies; both were already workspace
deps.
rig-core: 1249 lib tests, 5 policy tests, clippy clean.
…parse errors P2 from review. `serde_keys` and `derives_serde` drove `parse_nested_meta` with a callback that consumed `key = value` but not serde's documented **nested** form, `key(...)`. syn then failed mid-attribute with "expected ',' after bound(...)", and both callers discarded that error with `let _ = …`, so every key *after* the nested entry went unseen. Reproduced before fixing: #[serde(bound(serialize = "T: Serialize"), untagged)] -> untagged = false #[serde(rename(serialize = "wire"), tag = "type")] -> no representation The first is the dangerous one. It is silent, and because provider directories are excluded from the positive representation scan, an untagged enum there could bypass the allowlist check entirely — the exact hole that wall exists to close. The second is merely a spurious violation. Both attribute readers now parse their arguments as a comma-separated `Punctuated<Meta, Token![,]>` (`Punctuated<Path, …>` for derives), which admits all three container-attribute forms serde documents — bare path (`untagged`), name-value (`tag = "type"`), and nested list (`bound(...)`, `rename(...)`) — and propagate every parse error up through `collect_enums` to `parsed_enums`, where it panics. An attribute this scanner cannot read must fail the test, never quietly narrow it. Regression tests cover both nested forms, a nested-only attribute (`bound` alone is neither tag nor untagged, so the enum still owes a representation), and a malformed attribute that must fail rather than pass. Falsification re-run, since the scanner changed again: against `03643160^` the representation wall still flags exactly the same six enums. Running the whole file there also surfaces the untagged wall correctly failing on AssistantContent, StreamedAssistantContent and StreamedUserContent — untagged on that tree and absent from today's allowlist — which earlier filtered runs had not shown. On this tree the corrected parsing reveals no previously hidden untagged enum: the allowlist is unchanged at 18 entries with no stale rows. rig-core: 1249 lib tests, 7 policy tests, clippy clean.
…cape hatch
Four review findings, all low, all confirmed by reproduction first.
## The wall skipped the workspace-root `src/`
`rig_owned_public_serde_enums_are_not_untagged` walks SOURCE_ROOTS
(`["src", "crates"]`); the representation check I added walked only
`workspace.join("crates")`. The published `rig` facade at the workspace-root
`src/` was therefore outside the wall, and its own path filter would have
rejected the file anyway — `relative.contains("/src/")` never matches
`src/lib.rs`, which has no leading slash. A public data-bearing enum added there
would fall back to implicit external tagging and pass silently: precisely the
failure the wall exists to prevent.
Both roots are now scanned, the filter tests `starts_with("src/") ||
contains("/src/")`, and the check gains the floor-file assertion its sibling
already had — the absence of which is exactly why a missing root went unnoticed.
Verified by planting a `pub enum RootFacadeProbe { A(u32) }` in the root `src/`:
now reported as `src/_policy_probe.rs:2 RootFacadeProbe`, previously silent.
## Cohere: local guards rejected requests the provider honours
`CohereCompletionRequest::additional_params` is `#[serde(flatten)]`, so a
`"tools"` key there lands at the top level of the body — and because the typed
`tools` field is `skip_serializing_if = "Vec::is_empty"`, it can be the *only*
`tools` key on the wire. My `Required` guard tested `tools.is_empty()` against
the typed field alone and so failed a well-formed request locally.
`Specific` had the same blind spot, inherited rather than introduced: with the
typed list empty it validates requested names against an empty `available` set
and rejects every one. Both arms now consult the escape hatch via `carries_tools`
and skip validation they cannot perform on a partial view, rather than rejecting
on incomplete information.
## Stale fixture-function references
`decode_matrix_cases` was renamed to `stream_item_matrix_cases`; a comment and,
worse, a runtime assert message still named the old function — the exact text a
contributor reads off a red test.
rig-core 1250 lib tests, 7 policy tests; rig-agent 483; clippy clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MultiTurnStreamItem,StreamedAssistantContent, andStreamedUserContentaround nested adjacenttype/contenttaggingWhy
PR #2277 tagged
AssistantContent, but several other Rig domain enums still depended on Serde's implicit external representation or variant trial order. In particular,StreamedAssistantContent::Unknowncould overlap with known object-shaped variants, allowing provider-native payloads that happened to contain fields such astextto be reclassified by shape.This change makes semantic identity explicit. Known tags with malformed content fail loudly, while explicitly tagged
Unknownpayloads remain opaque and value-equal.Impact
This is an intentional serialization break. Old untagged, externally tagged, and accidental PascalCase forms no longer load.
MIGRATING.mddocuments before/after JSON for every changed public enum and the impact on persisted agent runs, completion responses, stream logs, filters, model-listing errors, output modes, and SQLite metric configuration.Provider request and response JSON is unchanged. Provider-native wire enums retain their upstream schemas, and all 18 public enum-level untagged provider exceptions are documented in a stale-checked allowlist.
Follow-up to #609.
Validation
cargo fmt --all --checkcargo clippy --all-targets --all-features -- -D warningscargo test(full workspace)cargo test -p rig-core --lib— 1,245 passed, 3 ignoredcargo test -p rig-agentcargo test -p rig-sqlitecargo test -p rig-core --test domain_serde_policycargo doc --workspace --no-depsReview
A separate full-diff review covered correctness, serialization boundaries, provider-wire leakage, unsafe edge cases, test completeness, and documentation drift. It found and fixed the policy-wall coverage gap, stale
StreamFinalwording, and the legacyMediaTypecasing example. No confirmed P0/P1 findings remain.