feat: add durable identifiers to messages#2836
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
83fae70 to
fe16a56
Compare
|
|
||
| # Add the response message to the conversation | ||
| _ensure_message_id(message) | ||
| agent.messages.append(message) |
There was a problem hiding this comment.
Issue: The PR description says ids are assigned "at the append chokepoint," but in practice _ensure_message_id is called at five separate Python call sites (here ×3 in event_loop.py, plus agent._append_messages, the bidi agent, and two inline _generate_message_id() calls in interventions/registry.py). Any future code path that appends a message without remembering to call the helper will silently produce an id-less message, quietly breaking the durability invariant this PR exists to guarantee.
Suggestion: Consider funneling all of these through a single private append helper (e.g. have the three event_loop.py sites and the interventions sites go through agent._append_messages, which already assigns the id and fires MessageAddedEvent). If a single chokepoint genuinely isn't reachable for some of these paths, a short comment explaining why each site assigns the id independently would help the next person preserve the invariant. The TS side has the same shape (two ensureMessageId sites in agent.ts + an inline generateMessageId() in interventions/registry.ts).
| missing, ``None``, or empty-string id is treated as absent and replaced, since an empty id | ||
| cannot serve as a durable key. | ||
| """ | ||
| if not message.get("id"): |
There was a problem hiding this comment.
Issue: _ensure_message_id mutates the caller-supplied Message in place, injecting an id into an object the caller still owns and may reuse. For input messages the user constructed, this is an observable side effect on their object that isn't obvious from the call site. (Same applies to the TS ensureMessageId.)
Suggestion: This is likely intentional — it's what makes a caller-supplied id stable — but a one-line note on the function that it intentionally mutates the passed message would prevent a future reader from "fixing" it into a copy and breaking stability. Worth confirming there's no path where the same Message dict is shared across two agents/turns where an injected id could leak unexpectedly.
| return True | ||
| elif isinstance(action, Guide): | ||
| event.agent.messages.append({"role": "user", "content": [{"text": action.feedback}]}) | ||
| event.agent.messages.append( |
There was a problem hiding this comment.
Issue (minor): These two intervention sites call _generate_message_id() inline and build the id into the dict literal, rather than appending the message and letting _ensure_message_id assign it. This is a third id-assignment pattern (distinct from _ensure_message_id and from going through _append_messages), which adds to the scatter noted in event_loop.py.
Suggestion: If these direct messages.append(...) calls can't route through _append_messages (the comment in the TS twin says they intentionally bypass the hook pipeline / conversation manager), that's fine — but a brief inline note here explaining the bypass and that the id is assigned manually as a result would make the divergence intentional rather than looking like an inconsistency.
| """Assign a durable id to the message in place if it does not already have a usable one. | ||
|
|
||
| A message that already carries a non-empty id (e.g. restored from a session or supplied by a | ||
| caller) keeps it, so the same message has a stable identifier everywhere it is observed. A |
There was a problem hiding this comment.
Issue: Python exposes a public, null-safe accessor get_message_id(message) (mirroring the existing get_message_metadata), but the TypeScript side exposes only the raw optional message.id property with no accessor. For a change whose stated goal is keeping the Message shape "behaviorally identical" across both SDKs, the public surface is asymmetric — a consumer reading durable ids writes get_message_id(m) in Python but m.id in TS.
Suggestion: Either add a matching getMessageId helper in TS, or note in the PR why the asymmetry is acceptable (e.g. TS optional-property + ?. access already covers the null-safe case idiomatically). Not blocking, but worth a deliberate decision since this is the public read path memory stores will use.
|
Assessment: Comment (no blocking issues) Solid, well-scoped change. The durable Review themes
Nicely done overall — the strip-before-provider regression test and the redaction-preservation tests are exactly the right things to lock down for this feature. |
|
My concern is scope and layer. As a DTO that memory stores read for dedup, this is fine. But the PR already persists it inside SessionMessage and snapshots, so it's effectively entering the session layer. If the natural next step is for it to become the session manager's record key. I'd want us to explicitly not do that, and ideally keep durable identity on the persistence wrapper rather than on the provider-facing Message. It also overlaps with the planned transcript work, which models messages as a DAG. A flat message.id that's preserved across redaction/truncation can't express edits or branches, so shipping it as "the" durable identity now will collide with transcript node identity later. Finally, on correctness as a unique key: it's preserved across deep copies (as_tool, graph, swarm), assigned only at append chokepoints (the summarizer already needed a manual fix to keep the invariant), and never validated for uniqueness or format with caller-supplied ids trusted as-is. Before we lean on (session_id, message_id) anywhere, I'd like the uniqueness scope defined (per session_id), copy/sub-agent semantics decided, and assignment enforced at the persistence boundary rather than by convention. In short: can we scope this PR to "opaque durable id for memory dedup," document it as non-authoritative and not a session/transcript identity, and defer the canonical-identity decision to the transcript design? |
Description
Messages do not carry a durable identity. The only per-message tracking available today is ephemeral: the memory
ExtractionCoordinator's in-session high-water-mark sequence number, andSessionMessage.message_id— an ordinal index that is not stable across conversation-manager truncation or session restore. Neither survives as a durable key, so a memory store has no way to build a(session_id, message_id)tuple to deduplicate extracted messages across sessions.This change gives every message a durable, stable
idin both SDKs. The id is assigned once, when a message is added to the conversation, and is preserved everywhere that message is later observed —MessageAddedEventsubscribers, session persistence, and snapshots. It is never sent to model providers (the existing role/content whitelist already strips everything else). Memory stores can now combine this id with a session id to identify a message uniquely across restarts.Assignment happens at append time rather than at construction. This keeps it idempotent — a message that already has an id (restored from a session, supplied by a caller, or re-appended) keeps it — which is what makes the id stable through a save/restore cycle. It also means messages that were persisted before this change are left without an id rather than being silently backfilled with a fresh one on each load.
Both SDKs generate a canonical (hyphenated) UUID v4: Python via
str(uuid.uuid4()), TypeScript viacrypto.randomUUID(). The shapes match deliberately, so a message id means the same thing regardless of which SDK produced it.Wiring the id into the memory extraction pipeline (so stores receive it for deduplication) is intentionally out of scope here; this PR only establishes the durable id on the
Messagetype so that work can build on it.This is a coordinated change across both SDKs to keep the
Messageshape consistent. Python and TypeScript are kept behaviorally identical: assign at the append chokepoint, preserve on redaction, exclude from provider payloads, and no backfill of legacy messages.Public API Changes
Messagegains an optional, durableid.Python — new
idfield on theMessageTypedDict, plus a null-safe accessor:TypeScript — new optional
idonMessageDataand theMessageclass; it round-trips throughtoJSON/fromJSON/clone:Both fields are optional and backward compatible: existing code that constructs messages without an id is unaffected, and messages persisted before this change deserialize with no id. The id is assigned by the agent when a message is appended, so a caller-supplied id (e.g. on input messages) is always preserved.
Related Issues
Resolves: #2805
Documentation PR
No documentation changes. The id is assigned automatically and is primarily a building block for upcoming memory-store deduplication; there is no new user-facing workflow to document yet.
Type of Change
New feature
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce new warnings.
hatch run prepareBeyond the unit suites (both SDKs green), I exercised the change end to end: ran a real agent turn and confirmed every recorded message has a unique id, then persisted through a
FileSessionManagerand restored into a fresh agent, confirming the restored ids match the persisted ones. Regression tests assert the id never reaches the model-provider payload — in Python at thestream_messageswhitelist, and in TypeScript at the Anthropic adapter's request formatting (where TS does its stripping). Note:hatch run prepare's static-analysis step couldn't bootstrap locally due to an unrelated native build failure in the optional[cedar]extra (rustc version); ruff/mypy on the changed files and the full test suites pass, and CI runs the gate cleanly.Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.