feat(core)!: make the empty-identifier rule structural on response metadata - #2340
Open
gold-silver-copper wants to merge 8 commits into
Open
feat(core)!: make the empty-identifier rule structural on response metadata#2340gold-silver-copper wants to merge 8 commits into
gold-silver-copper wants to merge 8 commits into
Conversation
gold-silver-copper
force-pushed
the
fix/issue-2336-structural-empty-id
branch
from
August 15, 2026 09:34
109fe8b to
b3f0d7d
Compare
gold-silver-copper
added a commit
that referenced
this pull request
Aug 15, 2026
Four items from review of #2340. The central claims held under checking — serialized JSON really is byte-identical (the serde attributes are unchanged at both ends), read-side churn really is small (every `.map(String::from)` adaptation is in tests, none in production) — so these are the edges. **The load-bearing absence is now a guard, not a comment.** The whole persistence argument rests on `WireId` having no `Deserialize`: that missing impl is what forces every boundary to read `Option<String>` and normalize. Adding the derive would leave every existing test green, because the explicit `deserialize_with` attributes keep working — while letting the *next* `Option<WireId>` field be declared without one. `StreamPartId`, in the same file, pairs the same kind of prose with `serialize_part_id.rs`; `WireId` had only the prose. `identity_leak/deserialize_wire_id.rs` closes that: it fails to compile today, and starts compiling — so fails the cell — if the derive is added. **The macro's own doc block contradicted the hunk below it.** The header still said the fields were `Option<String>` and that "the invariant lives in these generated setters", fifteen lines above the new text saying the rule is now structural. A maintainer applying `response_metadata_setters!` to a new type would have followed the header and hit E0308. **`ProviderResponseError::provider_request_id` kept the hole.** It is the same transport id on the failure path, and since #2335 removed `#[non_exhaustive]` a struct literal could set `Some(String::new())`, which `Display` renders as `(request id: )` — a divergence from the success path that can no longer produce one, of exactly the kind the setter docs warn about. Now `Option<WireId>`. The cost is two lines: the accessor is `.as_deref()`, so it is unchanged, the setter still takes `Option<String>` so provider call sites are unchanged, and the repo builds this type through its constructors everywhere. **The one runtime behavior change now has tests.** Retyping is compile-time everywhere except the streamed `MessageId` arm, where an empty announced id used to become `Some("")` and is now absence. Two cells pin it, including that an empty event does not erase a real terminal id. Mutation-checked: making the empty case mint an id — the plausible "fix", since `new_or_mint` does exactly that elsewhere — fails both. Also pins the absent-identifier wire shape of the two core types, which the round-trip tests cannot see: `null` and a missing key both load as `None`, so a change to these attributes would rewrite every stored record silently. The shape is asymmetric and predates this work — `message_id`/`response_id` are written as explicit nulls, `provider_request_id` is omitted — and the test asserts what is, so the retype is provably shape-neutral. MIGRATING now names the retyped error field and shows the `deserialize_optional_wire_id` attribute a downstream type needs.
gold-silver-copper
added a commit
that referenced
this pull request
Aug 16, 2026
Four items from review of #2340. The central claims held under checking — serialized JSON really is byte-identical (the serde attributes are unchanged at both ends), read-side churn really is small (every `.map(String::from)` adaptation is in tests, none in production) — so these are the edges. **The load-bearing absence is now a guard, not a comment.** The whole persistence argument rests on `WireId` having no `Deserialize`: that missing impl is what forces every boundary to read `Option<String>` and normalize. Adding the derive would leave every existing test green, because the explicit `deserialize_with` attributes keep working — while letting the *next* `Option<WireId>` field be declared without one. `StreamPartId`, in the same file, pairs the same kind of prose with `serialize_part_id.rs`; `WireId` had only the prose. `identity_leak/deserialize_wire_id.rs` closes that: it fails to compile today, and starts compiling — so fails the cell — if the derive is added. **The macro's own doc block contradicted the hunk below it.** The header still said the fields were `Option<String>` and that "the invariant lives in these generated setters", fifteen lines above the new text saying the rule is now structural. A maintainer applying `response_metadata_setters!` to a new type would have followed the header and hit E0308. **`ProviderResponseError::provider_request_id` kept the hole.** It is the same transport id on the failure path, and since #2335 removed `#[non_exhaustive]` a struct literal could set `Some(String::new())`, which `Display` renders as `(request id: )` — a divergence from the success path that can no longer produce one, of exactly the kind the setter docs warn about. Now `Option<WireId>`. The cost is two lines: the accessor is `.as_deref()`, so it is unchanged, the setter still takes `Option<String>` so provider call sites are unchanged, and the repo builds this type through its constructors everywhere. **The one runtime behavior change now has tests.** Retyping is compile-time everywhere except the streamed `MessageId` arm, where an empty announced id used to become `Some("")` and is now absence. Two cells pin it, including that an empty event does not erase a real terminal id. Mutation-checked: making the empty case mint an id — the plausible "fix", since `new_or_mint` does exactly that elsewhere — fails both. Also pins the absent-identifier wire shape of the two core types, which the round-trip tests cannot see: `null` and a missing key both load as `None`, so a change to these attributes would rewrite every stored record silently. The shape is asymmetric and predates this work — `message_id`/`response_id` are written as explicit nulls, `provider_request_id` is omitted — and the test asserts what is, so the retype is provably shape-neutral. MIGRATING now names the retyped error field and shows the `deserialize_optional_wire_id` attribute a downstream type needs.
gold-silver-copper
force-pushed
the
fix/issue-2336-structural-empty-id
branch
from
August 16, 2026 00:09
f93f52a to
0b3cb24
Compare
gold-silver-copper
added a commit
that referenced
this pull request
Aug 16, 2026
…2352) #2340 made the response-metadata identifiers `Option<WireId>` but left `completion::Message::Assistant`'s `id` an `Option<String>`. History is built by copying an id straight off a response, so every hand-off had to convert: Message::Assistant { id: turn.message_id.map(String::from), content } 29 such sites across the workspace, 11 of them production. That repetition was the type system pointing out that the two ends of one hand-off disagreed about a value they both hold. Retyping the field deletes all 29 rather than respelling them: id: turn.message_id, id: self.message_id.clone(), Exactly one conversion survives, at OpenAI's provider-native `Message::Assistant` whose `id` is a raw `String` — the wire boundary, which is where #2340's design says the handle is unwrapped. This is also the id that matters most for the invariant: it is the one *persisted in history and replayed upstream*, where the types #2340 hardened are transient. `Message::Assistant { id: Some(String::new()) }` used to compile and put the sentinel into stored history. Persisted data is unchanged, and four tests pin that rather than assert it: the handle serializes as its bare string, an absent id is still an explicit `null` rather than omitted, a record written without the key still loads, and a stored `""` loads as `None` instead of failing the record. That third test exists because the first attempt got it wrong. A bare `Option<T>` field is implicitly optional to serde, but adding `deserialize_with` drops that special case, so `serde(default)` is what *preserves* the accepted shape rather than what changes it. Omitting it made every id-less stored message unloadable, and the existing `round_trip_diff_recipe_detects_every_dropped_key` caught it. The pairing now matches every other identifier field. Reading is unaffected — `WireId` derefs to `str`. Construction from a raw string goes through `WireId::new`, which yields `None` for `""`. Several test assertions got shorter as a side effect: comparing `Option<&str>` against `Some("id")` beats allocating a `String` to compare with.
…tadata
StreamFinal, completion::CompletionResponse and completion::ResponseIdentity
normalize an empty identifier string to None, but the rule lived only in the
with_*_id setters the response_metadata_setters! macro generates. The fields
are public, so `x.message_id = Some(String::new())` compiled and put the
sentinel back -- making the buffered and streaming results of one request
compare unequal, and letting empty ids reach telemetry and replayed assistant
messages.
The three identifier fields are now Option<WireId>. WireId already existed for
exactly this ("the only constructor rejects the empty string, so an absent
handle is Option::None by construction") and its only constructor still is
WireId::new, so Some("") is unrepresentable however the value is built. The
setters are unchanged for callers -- they still take anything Into<String> and
route it through WireId::new.
rig-agent's identity-carrying types follow (CompletionCall, ModelTurn,
StreamedTurn, PartialStreamedTurn): they round-trip these ids through
ResponseIdentity, so leaving them Option<String> would have made the invariant
structural in core and advisory one layer up.
Serialized data is unaffected. WireId gains a transparent Serialize, so the
JSON is byte-identical, and it deliberately gains NO Deserialize: a fallible
one (the issue's suggested `try_from = "String"`) would turn a stored "" into
a hard load failure, and a transparent one would accept "" and reintroduce the
hole from the JSON side. Instead every deserialization boundary reads
Option<String> and normalizes -- through the existing Repr types, or through
the new streaming::deserialize_optional_wire_id on types that derive
Deserialize directly.
model stays Option<String>: it is a label rather than an identifier, has no
production reader on either type, and never reaches a request or a replayed
message, so its setter remains the only thing normalizing it.
Provider-native wire types keep Option<String> on purpose -- they mirror what
the provider actually sent, including an echoed "", and normalization belongs
at the conversion boundary, which is where WireId::new now runs.
Closes #2336
The first formulation asserted on a diagnostic whose `note: tuple variant defined here` renders the `Option::Some` definition from the standard library's source -- which CI does not have, so the recorded stderr matched locally and truncated there. Binding the sentinel first makes the error the assignment's own type mismatch, which cites nothing outside the test file.
An independent impact analysis of this branch found nine defects. The two that mattered: Persisted agent-run state was changing shape. rig-agent's ModelTurn, StreamedTurn and PartialStreamedTurn ids carried no serde attributes on main, so an absent id serialized as `null`; retyping them added `skip_serializing_if`, which silently omitted the key instead. Every field now carries exactly main's attribute set plus the normalizing `deserialize_with` -- and `default` alongside it, because `deserialize_with` removes the implicit missing-field tolerance a bare `Option` has (measured: bare `Option` yields Ok(None) on a missing key, `deserialize_with` alone yields Err). A new test pins all three properties: null on write, missing loads as absent, stored "" normalizes. `==` against a `&str` did not actually work. `Deref` does not provide it; `ToolCallId` carries explicit `PartialEq<str>`/`PartialEq<&str>` impls and `WireId` now does too, with a test. Also: dropped a dead `deserialize_with` from CompletionResponse (it deserializes through its Repr, so a field hook never runs) and said so in a comment; extended the compile-fail case from one field to all three plus StreamFinal and ResponseIdentity; moved the MIGRATING section into the `0.41 → next` band it belongs to and restored main's heading spacing; added the per-crate CHANGELOG entries; and corrected a now-more-misleading claim that WireId is "constructible solely from PartId::Wire".
Ten recorded cells across anthropic, openai and gemini pinning what #2336 actually moves: every response identifier now travels through Option<WireId> on both the serialize and the deserialize side, and the values come from provider payloads this repo had no round-trip coverage for. Per provider, on the axes that provider actually populates: - blocking response identity survives a JSON round-trip - streamed terminal record survives a JSON round-trip, comparing equal to the value it was written from - an agent run's recorded completion calls survive a round-trip OpenAI is covered on both APIs because they populate different axes: Responses reports a message-scoped msg_ id, Chat Completions a response-scoped chatcmpl- id, and both report x-request-id. Anthropic reports a message id and a request-id header and no response-scoped id. Gemini is the absent case -- it reports no request-id header, so the cells assert None survives as None rather than becoming an empty id, which is the sentinel the whole change exists to make unrepresentable. What these prove, stated honestly: a cassette cannot observe the newtype, which is a compile-time property pinned by the trybuild case. What they catch is the migration having broken extraction or persistence against a real provider payload. They pass identically against origin/main's source -- that is the point, since the change is meant to be behaviour-preserving.
Four items from review of #2340. The central claims held under checking — serialized JSON really is byte-identical (the serde attributes are unchanged at both ends), read-side churn really is small (every `.map(String::from)` adaptation is in tests, none in production) — so these are the edges. **The load-bearing absence is now a guard, not a comment.** The whole persistence argument rests on `WireId` having no `Deserialize`: that missing impl is what forces every boundary to read `Option<String>` and normalize. Adding the derive would leave every existing test green, because the explicit `deserialize_with` attributes keep working — while letting the *next* `Option<WireId>` field be declared without one. `StreamPartId`, in the same file, pairs the same kind of prose with `serialize_part_id.rs`; `WireId` had only the prose. `identity_leak/deserialize_wire_id.rs` closes that: it fails to compile today, and starts compiling — so fails the cell — if the derive is added. **The macro's own doc block contradicted the hunk below it.** The header still said the fields were `Option<String>` and that "the invariant lives in these generated setters", fifteen lines above the new text saying the rule is now structural. A maintainer applying `response_metadata_setters!` to a new type would have followed the header and hit E0308. **`ProviderResponseError::provider_request_id` kept the hole.** It is the same transport id on the failure path, and since #2335 removed `#[non_exhaustive]` a struct literal could set `Some(String::new())`, which `Display` renders as `(request id: )` — a divergence from the success path that can no longer produce one, of exactly the kind the setter docs warn about. Now `Option<WireId>`. The cost is two lines: the accessor is `.as_deref()`, so it is unchanged, the setter still takes `Option<String>` so provider call sites are unchanged, and the repo builds this type through its constructors everywhere. **The one runtime behavior change now has tests.** Retyping is compile-time everywhere except the streamed `MessageId` arm, where an empty announced id used to become `Some("")` and is now absence. Two cells pin it, including that an empty event does not erase a real terminal id. Mutation-checked: making the empty case mint an id — the plausible "fix", since `new_or_mint` does exactly that elsewhere — fails both. Also pins the absent-identifier wire shape of the two core types, which the round-trip tests cannot see: `null` and a missing key both load as `None`, so a change to these attributes would rewrite every stored record silently. The shape is asymmetric and predates this work — `message_id`/`response_id` are written as explicit nulls, `provider_request_id` is omitted — and the test asserts what is, so the retype is provably shape-neutral. MIGRATING now names the retyped error field and shows the `deserialize_optional_wire_id` attribute a downstream type needs.
…2352) #2340 made the response-metadata identifiers `Option<WireId>` but left `completion::Message::Assistant`'s `id` an `Option<String>`. History is built by copying an id straight off a response, so every hand-off had to convert: Message::Assistant { id: turn.message_id.map(String::from), content } 29 such sites across the workspace, 11 of them production. That repetition was the type system pointing out that the two ends of one hand-off disagreed about a value they both hold. Retyping the field deletes all 29 rather than respelling them: id: turn.message_id, id: self.message_id.clone(), Exactly one conversion survives, at OpenAI's provider-native `Message::Assistant` whose `id` is a raw `String` — the wire boundary, which is where #2340's design says the handle is unwrapped. This is also the id that matters most for the invariant: it is the one *persisted in history and replayed upstream*, where the types #2340 hardened are transient. `Message::Assistant { id: Some(String::new()) }` used to compile and put the sentinel into stored history. Persisted data is unchanged, and four tests pin that rather than assert it: the handle serializes as its bare string, an absent id is still an explicit `null` rather than omitted, a record written without the key still loads, and a stored `""` loads as `None` instead of failing the record. That third test exists because the first attempt got it wrong. A bare `Option<T>` field is implicitly optional to serde, but adding `deserialize_with` drops that special case, so `serde(default)` is what *preserves* the accepted shape rather than what changes it. Omitting it made every id-less stored message unloadable, and the existing `round_trip_diff_recipe_detects_every_dropped_key` caught it. The pairing now matches every other identifier field. Reading is unaffected — `WireId` derefs to `str`. Construction from a raw string goes through `WireId::new`, which yields `None` for `""`. Several test assertions got shorter as a side effect: comparing `Option<&str>` against `Some("id")` beats allocating a `String` to compare with.
Three test hooks declared their own positional stand-in for a type that
already exists:
type IdentityTriple = (Option<WireId>, Option<WireId>, Option<WireId>);
type IdentityPair = (Option<String>, Option<String>);
`ResponseIdentity` *is* that triple. Once a tuple stands in for it, the
struct has to be taken apart field by field to fill the tuple, and any
field whose type does not line up needs a conversion on the way — which
is where
event.identity.provider_request_id.as_deref().map(str::to_owned)
came from. Holding the real type removes both the aliases and every
conversion:
.push(event.identity.clone());
The two `StreamRun` test structs get the same treatment: their
`message_id` now holds the handle rather than a `String`, so the
assignment is a plain clone and the existing `as_deref()` reads are
unchanged.
This also tests more than before. `CompletionResponse::message_id` is
documented to mirror `identity.message_id`, and the tuple version used
the former while never checking the two agree; that is now an explicit
assertion in the hook. The comparisons against `completion_calls` also
compare whole handles now instead of `as_deref()`-ed strings.
Net -9 lines, and no identity field is converted to `String` anywhere in
the workspace.
Nine findings. Eight were documentation and test hygiene; one was a real
code gap.
**`Reasoning::id` is the other durable handle** (finding 8). The module
doc claimed `WireId` is "the only value that may populate the replayable
message types", naming `Reasoning::id` — which was still a bare
`Option<String>` with an unfiltered setter. The sentinel was live, not
hypothetical: the OpenRouter converter carried a hand-spelled
`.filter(|id| !id.is_empty())` whose comment said the empty id actually
occurs, and two request builders (`xai::api`,
`openai::responses_api`) gated on `Some` and sent the value straight
into the body, each with a comment asserting an invariant the type did
not enforce. A `Some("")` walked through both.
The field is now `Option<WireId>`, so those comments are true by
construction. The filter is deleted as unreachable and the streaming
hand-off drops its `WireId::into_string` downgrade — the migration
removes a conversion rather than adding one. `with_id` widens to
`impl Into<String>` (source-compatible) and normalizes, so an empty
argument is absence. Note the gap was inherited from `main`, not
introduced by this branch; the changelog says so.
Tests: a compile-fail cell, the `default`/`deserialize_with` trichotomy
(`""`, `null`, missing key all load as absence), absent-serializes-as-
`null`, the setter's clear-on-empty, and a request-body regression for
each of the two gated consumers. The persisted shape is byte-identical
to `main` — checked by serializing on both sides and diffing.
**The changelog said two false things** (findings 1, 2). It claimed no
`serde(default)` was added and that the field had been required; both
are wrong, and the sentence invited the dangerous edit of removing the
"redundant" `default`, which would make every pre-`id` stored message
fail to load. It also credited removing 19 `.map(String::from)` sites,
which is net-zero against `main` (8 before, 8 after) — those
conversions were introduced by this branch and then removed by it. The
honest justification, that the two ends of one hand-off should hold the
same type, is kept.
**`StreamingCompletionResponse::message_id`** (finding 4) is public API
retyped by this branch and was named in none of the three inventories.
Now in all three.
**Three vestigial round-trips** (finding 5): `StreamRun::message_id` is
already an `Option<WireId>`, so `.and_then(WireId::new)` on it was
unwrap-recheck-rewrap. The `gpt_5_6_reasoning.rs` site is left alone —
its source is a provider-native `Option<String>`, so its `WireId::new`
is a real normalization.
**A shipped WARN log** (finding 6) `?`-captured an `Option<WireId>`,
rendering `Some(WireId("msg_1"))` where it had rendered `Some("msg_1")`.
`as_deref()` restores it.
**The identity cells asserted round-trip equality without presence**
(finding 7), which holds vacuously when both sides are `None`. Ten cells
now anchor on the axes each provider genuinely issues, and every anchor
is demonstrated to fail under its own mutation: a no-op
`with_identity` breaks all three agent-run cells, and deleting Gemini's
blocking or streaming `response_id` normalization breaks exactly the
cells fed by that path. The cells still pass against `main`'s source,
which is their stated design.
Nits (finding 9): MIGRATING gains the `Option<String>` case
(`and_then`, since `map` gives `Option<Option<WireId>>`), and `WireId`
regains `PartialOrd`/`Ord`, which `Option<String>` had and the sibling
`ToolCallId` keeps.
One thing deliberately left: `StreamedAssistantContent::ReasoningDelta`
still carries its provider id as a raw `String`, so one
`and_then(WireId::new)` remains where rig-agent rebuilds the handle.
Retyping that public enum reaches ~30 sites, well beyond this scope.
gold-silver-copper
force-pushed
the
fix/issue-2336-structural-empty-id
branch
from
August 16, 2026 05:39
1b35341 to
011257f
Compare
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.
Closes #2336.
StreamFinalandcompletion::CompletionResponsenormalize an empty identifier toNone, but the rule lived only in thewith_*_idsetters. The fields arepub, so this compiled and put the sentinel back:The three identifier fields are now
Option<WireId>, soSome("")is unrepresentable rather than merely discouraged. Serialized JSON is byte-identical and a stored""still loads asNone.The shape of the fix
WireIdalready existed for exactly this — its doc comment states the principle the issue is about ("the only constructor rejects the empty string, so an absent handle isOption::Noneby construction").WireId::newremains the only way in, so the invariant holds however the value is built.The
with_*_idsetters are unchanged for callers: they still take anythingInto<String>and route it throughWireId::new.Three decisions the issue left open
1.
modelstaysOption<String>. The issue flagged this as "worth deciding in review rather than assuming". It is a label, not an identifier: it has no production reader on either type, never reaches a request or a replayed assistant message, and typing it asWireIdwould corrupt that type's documented meaning ("the durable provider handle … the only value that may populate the replayable message types and travel upstream"). Its setter remains the only thing normalizing it, and the macro doc now says so explicitly instead of implying the rule is uniform. A reviewer who weighs the equality-divergence argument more heavily than the type's meaning would reasonably introduce a smallModelNamenewtype instead — that is the disagreement worth having here.2.
WireIdgainsSerializebut deliberately noDeserialize. The issue suggests mirroringToolCallId's#[serde(try_from = "String", into = "String")]. That specific spelling is the one variant that breaks persistence:{"message_id": ""}deserializes toOk(None)— both types routeDeserializethrough aReprofOption<String>and then through the normalizing setters.try_fromDeserializewould turn a stored""— which an older or third-party producer may have written — into a hard load failure for the whole agent-run record.#[serde(transparent)]Deserializewould do the opposite: accept""and reintroduce the hole from the JSON side, defeating the change.So serialization is transparent (bare string, byte-identical output) and every deserialization boundary reads
Option<String>and normalizes — through the existingReprtypes, or through a newstreaming::deserialize_optional_wire_idon types that deriveDeserializedirectly. The rationale is recorded on the type so the next person does not "fix" the missing impl.3. Scope beyond the two types named. The issue names
StreamFinalandCompletionResponse. Two more places carry the same ids and had the same gap:completion::ResponseIdentity— the carrier the other two convert to and from, and it derivesDeserializedirectly, so it uses the new field-level normalizer.rig-agent'sCompletionCall,ModelTurn,StreamedTurnandPartialStreamedTurn— they round-trip these ids throughResponseIdentity, so leaving themOption<String>would have made the invariant structural in core and advisory one layer up.Provider-native wire types keep
Option<String>on purpose.anthropic::StreamingCompletionResponse, openai's, cohere's, bedrock's and friends mirror what the provider actually sent, including an echoed""; normalization belongs at the conversion boundary, which is exactly whereWireId::newnow runs.Read-side churn: much smaller than the issue feared
The issue estimated "roughly 130 in-crate and 230 elsewhere" and predicted the compiler would give the real number. It did: the whole workspace needed 28 files, and almost none of it is read-site churn, because
WireIdderefs tostr(matchingToolCallId, which already does):Derefalone does not giveid == "msg_1", soWireIdalso gains the explicitPartialEq<str>/PartialEq<&str>implsToolCallIdalready carries.Only a site that wants an owned
Stringchanges, to.map(String::from). That is 16 sites workspace-wide, mostly where an id is handed toMessage::Assistant { id }, which is a different type and staysOption<String>.Tests
The invariant is structural — a new compile-fail case in the existing
identity_leaktrybuild suite, so the proof is that the program does not build:Serialization is unchanged —
a_handle_serializes_as_its_bare_stringpins the transparent form, andidentifiers_persist_as_bare_strings_and_empty_loads_as_absent(onCompletionResponse) andstream_final_identifiers_persist_as_bare_strings(onStreamFinal) each round-trip a real value and then re-load the same record with the ids replaced by"", asserting they come backNonerather than erroring.Persisted shape is unchanged, byte for byte —
an_absent_identifier_still_serializes_as_nullpins the subtle one: rig-agent's turn types carry noskip_serializing_ifon these fields, so an absent id must keep serializing asnullrather than being omitted. Each field now carries exactly main's attribute set plus the normalizingdeserialize_with, anddefaultalongside it —deserialize_withremoves the implicit missing-field tolerance a bareOptionhas (measured: a bareOptionyieldsOk(None)on a missing key,deserialize_withalone yieldsErr).Normalization is preserved at every boundary —
deserializing_an_empty_handle_is_absence_not_an_errorcovers"", absent andnull;response_identity_normalizes_a_stored_empty_identifiercovers the direct-Deserializetype.The pre-existing
identity_leaksuite still passes unchanged, confirming that addingSerializeandDereftoWireIddoes not weaken the part-identity contract (all four existing cases targetStreamPartIdor field privacy, notWireId's trait set).Live cassette matrix — 10 recorded cells across three providers
Every identifier this PR retypes comes from a provider payload, and the repo had no round-trip coverage for any of them. Recorded live and replayable key-free:
blocking_identity_survives_a_json_round_tripmsg_…id +request-idheader; no response-scoped idstreaming_identity_survives_a_json_round_tripagent_run_identity_survives_a_json_round_tripcompletion_callssurvive persistenceresponses_identity_survives_a_json_round_tripmsg_…+x-request-idchat_completions_identity_survives_a_json_round_tripchatcmpl-…+x-request-idstreaming_terminal_identity_survives_a_json_round_tripagent_run_identity_survives_a_json_round_tripblocking_identity_survives_a_json_round_triprequest-idheader, soNonemust reload asNone, never as an empty idstreaming_identity_survives_a_json_round_tripagent_run_identity_survives_a_json_round_tripOpenAI is covered on both APIs deliberately: they populate different axes, so a single one would leave the other's extraction path unpinned. Gemini earns its place by being the provider that exercises absence — the sentinel this change makes unrepresentable is exactly what a broken absent-path would produce.
What these prove, stated honestly. A cassette cannot observe the newtype — that is a compile-time property, pinned by the trybuild case above. What the recordings catch is the migration having broken extraction or persistence against a real provider payload, which is the part that would ship silently. They pass identically against
origin/main's source, and that is the point: the change is meant to be behaviour-preserving, and these are the evidence that it is.Fixtures total 44 KB, replay with no API keys, and were audited for secrets (only the harness's
msg_REDACTED_n/chatcmpl-REDACTED_n/resp_REDACTED_nplaceholders appear).Verification
cargo fmt --all --checkRUSTFLAGS="-D warnings" cargo clippy -p rig-core -p rig -p rig-agent --all-targets --all-features— cleancargo test -p rig --all-features— no failurescargo test -p rig-core --lib— 1417 passed;cargo test -p rig-agent --lib— 521 passedcargo test -p rig-core --test identity_leak— passes, including the new casecargo check --workspace --all-targets— clean (including examples)cargo check -p rig-core --target wasm32-unknown-unknown— cleancassette_files_match_registered_scenariospassescargo test -p rig --all-features— 36 binaries, 2,172 tests, 0 failuresCompletionCall::with_identitybreaks all three agent-run cells; deleting Gemini's blocking or streamingresponse_idnormalization breaks exactly the cells fed by that path. The anchored cells still pass againstorigin/main's source, which is the matrix's stated designReasoningonmainand on this branch and diffing: byte-identical ({"id":"rs_1",…}and{"id":null,…})Also migrated, after review
Two fields flagged in earlier drafts as "worth their own PR" turned out to cost almost nothing and are included:
ProviderResponseError::provider_request_idcarries the same transport id on the failure path. Leaving itOption<String>would have let an error render(request id: )for a value the success path can no longer produce — the exact buffered-vs-error divergence the setter docs warn about. The migration is two lines: the accessor is.as_deref(), so it still returnsOption<&str>and no reader changed, the setter still takesOption<String>so no provider call site changed, and the type is built through its constructors everywhere in-tree.message::Reasoning::idis the other durable handle — the module doc already claimedWireIdis "the only value that may populate the replayable message types", naming this field, while it was still a bareOption<String>. The sentinel was live rather than hypothetical: the OpenRouter converter carried a hand-spelled.filter(|id| !id.is_empty())whose comment noted the empty id actually occurs, and two request builders (xai::api,openai::responses_api) gated onSomeand sent the value straight into the body, each with a comment asserting an invariant the type did not enforce. Those comments are now true by construction; the filter is deleted as unreachable and the streaming hand-off drops itsWireId::into_stringdowngrade, so the change removes a conversion rather than adding one. This gap was inherited frommain, not introduced here.Still out of scope
StreamedAssistantContent::ReasoningDelta::provider_idremains anOption<String>, so oneand_then(WireId::new)survives where rig-agent rebuilds the handle from a delta event. Retyping that public enum reaches ~30 sites — flagging it rather than widening this PR to cover it.Breaking
The public field types change, so this needs the breaking window — #2221 (v0.42.0) is still open.
MIGRATING.mdgains a section with the before/after for reads, writes and serialized data, and the #2335 section's "one invariant is widened" note now points at it instead of describing an open gap.