refactor(runtime): make dispatched attempts own request observations - #4300
refactor(runtime): make dispatched attempts own request observations#4300Astro-Han wants to merge 13 commits into
Conversation
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
07565cc to
4c96ffa
Compare
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
jackwener
left a comment
There was a problem hiding this comment.
Reviewed the latest head e3df5b052. No P0 or P1 — approving. One P3 inline. All three checks are terminal green. This is 35 files across 10 commits, so I went after the authority and integrity fixes rather than reading evenly; scope is stated at the end.
Keeping projections subordinate to authority is the right principle and is implemented correctly. A malformed projection row now resolves to a 'malformed' state that callers handle, instead of throwing out of a read that authority could have satisfied. A derived cache must never be able to break a read the source of truth can serve, and the default path stays fail-loud rather than swallowing corruption everywhere.
The repair guard is the strongest part. Conditioning a derived repair on ifLedgerRevision makes it a compare-and-swap against the canonical ledger, so a repair computed from a stale read cannot overwrite state derived from newer authority. The test earns particular credit: it corrupts the projection row directly through raw SQL (SET event_json = '{malformed') and then verifies no cold answer is persisted once canonical authority advances. That drives the actual race rather than asserting the happy path.
Two real correctness catches in the digest work. Widening the telemetry cache key from ${step}:${requestHash} to include providerId and modelId closes a genuine collision: identical request shapes under different providers or models previously shared a key. And deriving requestHash from the actual serializedRequest instead of a separately built JSON.stringify([...]) makes the digest describe what is really sent — a digest computed over a different structure than the request is not a digest of that request.
One note on method. The intermediate commits pass a malformed: 'throw' | 'missing' option, but that is not what the head looks like — it resolved into the inspected.state discriminated result. I reviewed the head state; anyone reading commit-by-commit should know the option does not survive to the final tree.
Scope. I examined the two authority commits, the malformed-projection handling, the ledger-revision repair guard, and the digest and cache-key changes. I did not line-by-line review request-shape.ts (768 lines changed) or the prompt-composition and latest-context-snapshot rewrites. "Approved" here means no P0/P1 in the areas I drove, not that every line of a 2,136-addition change was verified.
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
| replaceProjectionId || ledgerRevision | ||
| ? { | ||
| ...(replaceProjectionId ? { replaceEventId: replaceProjectionId } : {}), | ||
| ...(ledgerRevision ? { ifLedgerRevision: ledgerRevision } : {}), |
There was a problem hiding this comment.
P3 — this guard is silently inert when the optional method is absent.
readEventLedgerRevision is optional on the AgentRunStore type in core/src/agent-run.ts:798, and this call site spreads it conditionally:
...(ledgerRevision ? { ifLedgerRevision: ledgerRevision } : {})So a store that does not implement the method yields undefined, the option is omitted, and the repair proceeds unguarded — which is precisely the race commit b60d9e717 exists to close. "No guard available" and "guard satisfied" become indistinguishable at this call site.
It is not reachable today, and I checked rather than assumed: agent-run-store.ts declares the method non-optionally on its own interface and implements it at line 677, and execution-stores.ts:501 forwards it. Both production stores provide it, so the guard always applies in practice.
The gap is that a safety property depends on an optional member with a silent fallback. If the intent is that repair must always be guarded, making the method required on the store type this path consumes — or failing loudly when it is missing rather than proceeding — would keep the guarantee from quietly evaporating. The same shape applies at history-compact-ledger.ts:134.
Summary
ModelCallAttemptown one bounded, secret-freePreparedRequestObservationlatest_contextfrom canonical attempts and their observations, with malformed/old projections unable to block canonical appendLifecycle and authority
sequenceDiagram participant Runtime participant Seam as AI SDK model-call seam participant Artifact as Private artifact store participant Provider participant Ledger as Canonical ModelCallAttempt ledger participant Projection as Derived projections participant Diagnostics as Context diagnostics Runtime->>Runtime: Normalize one secret-free prepared request Note over Runtime: Derive serialization, digest, bytes, and segments together Runtime->>Artifact: Start optional serialized-request persistence Note over Runtime,Artifact: Best effort and never awaited by dispatch or accounting Runtime->>Runtime: Check abort and accounting readiness alt Aborted before seam invocation Runtime--xSeam: Do not invoke doStream or doGenerate Note over Ledger,Projection: No canonical attempt, sent entry, or projection update else AI SDK seam invoked Runtime->>Seam: doStream or doGenerate with prepared params Note over Runtime,Seam: Seam invocation authorizes the sent attempt Seam->>Provider: Provider-specific transport Note over Seam,Provider: Observation is semantic SDK input, not provider wire Provider-->>Runtime: Settlement and provider-reported usage Runtime->>Ledger: Commit canonical attempt with observation alt Projection is valid or absent Ledger->>Projection: Derive completed-main projection monotonically else Projection is malformed Ledger-->>Projection: Skip derived write and never roll back authority end end Diagnostics->>Projection: Read latest_context alt Current canonical v2 snapshot Projection-->>Diagnostics: Warm O(1) composition else Missing, old, or malformed Diagnostics->>Ledger: Read canonical ledger revision Diagnostics->>Ledger: Rebuild from canonical attempts Note over Ledger: Legacy provider events apply only if no canonical-era record exists Ledger-->>Diagnostics: Authority-derived candidate Diagnostics->>Ledger: CAS-check the same ledger revision alt Revision unchanged Diagnostics->>Projection: Repair derived row else Authority advanced concurrently Diagnostics--xProjection: Drop stale repair and do not write end endThe dependency is deliberately one-way: canonical events may derive or repair projections; projection absence, corruption, schema drift, or repair failure cannot block or rewrite canonical authority. RuntimeEvents and checkpoints remain the authority for conversation and compaction history. The observation only explains the semantic request Maka handed to the AI SDK seam.
Scope
This PR implements Workstream 1 of #4299 only. It does not implement semantic prefix continuity, continuity verdicts, provider-specific wire capture, or Inspector UI changes. Tool Availability remains owned by its existing runtime.
Relates to #4299.
Testing