feat!: finish the data layer — blessed ModelTurn conversion, counter/newtype ids, owned dispatch, serde across the protocol surface - #2419
Merged
Conversation
…n conversion The only CompletionResponse -> ModelTurn conversion lived as ~15 inlined lines in rig-agent's runner; a second driver copying them can silently drift (the tool-name sets must come from the prepared request, and the finish reason must go through the normalized accessor). Move the assembly into rig-run as ModelTurn::from_response(resp, &PreparedRequest) plus from_response_parts for drivers that carry the name sets by value, and rewrite the runner to use it. Also drops the stale 'Arc clone' comment: CompletionResponse::raw is a plain serde_json::Value.
internal_call_id becomes a Copy + Hash + Ord + serde-transparent NonZeroU64 counter id (via the existing counter_id! macro) everywhere it flows: rig-core's streaming assembler and StreamedAssistantContent / StreamedUserContent items, rig-run's PendingToolCall / TurnState / policy context / StreamedTurnAssembler, rig-agent's hook events, rewrite frames and stream feed. It was a 21-char nanoid String cloned at every boundary; an ECS driver keys per-frame correlation maps by it. Persisted runs re-emit saved ids after resume, so the counter_id! macro gains advance_past (an AtomicU64 fetch_max) and PendingToolCall's deserialize advances the mint counter past any persisted id — a resumed process cannot mint an id its consumers already saw. ConversationId is a string-backed Hash + Eq + serde(transparent) newtype in rig-core id.rs; ConversationMemory (and the rig-memory adapters), AgentConfig, the builder/runner/prompt-request conversation setters and InMemoryConversationMemory now use it instead of bare strings. Serialized stream items and persisted runs carry internal_call_id as a number now (documented break; run-state format has no cross-version stability contract).
ToolCatalog::execute (async fn over &self + &mut ToolContext) and the
ConversationMemory methods (&'a self + &'a ConversationId) return
borrowed futures, so an executor-agnostic host spawning tasks had to do
the clone-into-async-move dance at every spawn site. Add:
- ToolCatalog::execute_owned(self, name, args, context) -> impl Future
+ Send + 'static yielding (ToolResult, ToolContext) — the mutated
context comes back by value, preserving execute's publish contract.
- ConversationMemoryExt::{load_owned, append_owned, clear_owned} over
Arc<Self> + owned ConversationId, blanket-implemented for every
ConversationMemory including trait objects.
Both are wrappers over the borrowed methods, which remain the
implementation surface. Send + 'static pinned by tests.
Derive Serialize + Deserialize on AgentRunStep, PreparedRequest, RequestPatch, InvalidToolCallContext, RetryRequest, InvalidToolCallAction and StreamedTurnEvent, and add Clone (+ serde) to ModelTurnOutcome. AgentRun could already re-derive its step from persisted state, but a component caching an in-flight step, prepared request or patch could not round-trip through a saved world — now the whole protocol surface is data. Round-trip tests for each.
A mid-stream *streamed* turn was the one piece of run state that could not be persisted, breaking the save/load parity AgentRun promises. Derive Clone + serde through the assembler and its internals (ToolCallDeltaState, ReasoningPart(State), PendingInvalid, ExclusionCount as a transparent count). Clone copies the exclusion count — each lineage warns for its own drops — and deserialize restores it, keeping the loudness contract across a resume. Dropping an assembler mid-turn (an ECS despawn) stays silent unless content was actually excluded. Midway serialize/resume test proves the resumed assembler produces the identical turn.
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
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.
Interstitial PR between #2405 and the
rig-bevyplugin (bevy-prep series, one-protocol-two-drivers). An audit ofmainagainst the ECS-driver requirements found the protocol extraction complete but left data-layer gaps that would force a second driver to duplicate runner logic or fight the borrow checker at every task-spawn site. This PR closes all of them so the plugin PR is pure assembly. Ordered commits, each green.1.
ModelTurn::from_response— the conformance anchorThe only
CompletionResponse → ModelTurnconversion was ~15 inlined lines in rig-agent's runner, with two inputs hand-assembly gets wrong: the tool-name sets must come from the prepared request (never re-derived from the spec), and the finish reason must go through the normalizedfinish_reason()accessor. A second driver copying those lines can silently drift — which defeats "one protocol, two drivers, conformance by construction".rig_run::ModelTurn::from_response(resp, &PreparedRequest)(plusfrom_response_partsfor drivers carrying the name sets by value) is now the one blessed conversion; the runner is rewritten over it. Also drops the stale "Arcclone" comment (CompletionResponse::rawis a plainserde_json::Value).2.
InternalCallIdcounter id +ConversationIdnewtype (breaking)internal_call_idwas a 21-char nanoidStringcloned at every boundary. It is now aCopy + Hash + Ord, serde-transparentNonZeroU64counter id (the existingcounter_id!macro, same shape asRunId), threaded through rig-core's streaming assembler and stream items, rig-run's pending-call/policy types, and rig-agent's hook events (which drop their&'a strborrow for a by-value id).advance_past(anAtomicU64::fetch_max) andPendingToolCall's deserialize advances the mint counter past any persisted id — a resumed process cannot mint an id its consumers already saw (regression-tested).ConversationIdis a string-backed, serde-transparent newtype inrig_core::id; theConversationMemorytrait, rig-memory adapters, and theconversation(..)setters (nowimpl Into<ConversationId>, so string call sites compile unchanged) use it.internal_call_idas a number now — documented in MIGRATING; run state has no cross-version stability contract.3. Owned-future entry points
ToolCatalog::execute_owned(self, name, args, context) -> (ToolResult, ToolContext)andConversationMemoryExt::{load_owned, append_owned, clear_owned}(blanket impl overArc<Self>+ ownedConversationId) give hosts that spawn tasksSend + 'staticfutures without per-site clone-into-async moveceremony. The mutatedToolContextreturns by value, preservingexecute's publish contract (the runner's dispatch observes context metadata the same way). The borrowed methods stay the implementation surface.4. Serde across the step/policy types
AgentRunStep,PreparedRequest,RequestPatch,InvalidToolCallContext,RetryRequest,InvalidToolCallAction,StreamedTurnEventderiveSerialize + Deserialize;ModelTurnOutcomegainsClone(+ serde).AgentRuncould re-derive its step, but a component caching an in-flight step/prepared request could not round-trip through a saved world. Round-trip tests for each.5.
StreamedTurnAssembler: Clone + Serialize + DeserializeA mid-stream streamed turn was the one piece of run state that could not persist. The assembler and its internals now derive Clone + serde; the
ExclusionCountdrop-guard serializes as a transparent count (loudness contract survives a resume) andClonecopies it so each lineage warns for its own drops. Dropping an assembler mid-turn (an ECS despawn) stays silent unless replayed assistant content was actually excluded. A midway serialize/resume test proves the restored assembler produces the identical turn.(
ModelRef: Deserialize, flagged by the audit, turned out to already exist — no change.)Verification
cargo nextest run --workspace --all-features: 6239 passed, 0 failed;git diff --stat main -- tests/cassettesempty (commit 1 is byte-neutral on requests — full provider suites replay clean).-D warnings) clean;cargo check --target wasm32-unknown-unknowngreen for rig-core (incl.--all-features), rig-run, rig-agent, rig.from_responsefield-for-field equality, counter-id/ConversationIdround-trips +advance_past, persisted-id counter advance on deserialize, owned-futureSend + 'staticpins with behavior parity, step/policy serde round-trips, assembler midway resume.grep ModelTurn::newin rig-agent: only test-fixture construction remains.