fix(a2a): isolate persisted context histories - #83706
Conversation
|
CI triage: the only failing test slice is |
Store new conversation logs under a versioned SHA-256 namespace, retain original context IDs for listing, and leave ambiguous legacy sanitized logs untouched rather than reopening cross-context history leakage. Adapted from the collision-resistant storage approach in 067e213. Co-authored-by: sg-architect <kuangmi@nudge.com.cn>
2346a31 to
9d56d25
Compare
kuangmi-bit
left a comment
There was a problem hiding this comment.
Reviewed the fix against the local plugin code. Verdict: solid fix — SHA-256 filenames plus a v2 storage namespace eliminates both collision classes, and persisting context_id in each record makes list_conversations round-trip correct.
Bonus — this also fixes a list→recall inconsistency: previously list_conversations() returned sanitized stems (tenanta) while a2a_history expects the raw context_id (tenant/a), so a user recalling from the list could load the wrong (merged) file. With real context_ids persisted in v2 records, the round-trip is now consistent.
Two nits, neither blocking:
-
_safe_name(None)now raises — old code tolerated None via(context_id or "default"); the newcontext_id.encode(...)would raise AttributeError, which the broadexceptinpersist_messageswallows silently (message dropped). Current callers all pass str (extract_context_idguarantees ""), so this is future-proofing:_safe_name(context_id or "default")(or a guard inside) keeps the defensive contract. -
Legacy histories are deliberately orphaned — v2-only reads mean old
a2a_conversations/*.jsonlbecome invisible after upgrade. That is the right security posture given the ambiguity, but worth one line in the PR description (or an upgrade note) so users know old histories are archived, not lost. A one-time migration helper is a nice-to-have, not required.
The triaged CI failure (test_multiplex_busy_input_mode) is unrelated to this change and matches our own observations. Our open PR #77526 (history injection) touches the same module but only reads via load_conversation/format_history, so it is compatible with the v2 layout — no conflict expected.
Summary
contextIdvalues cannot collapse to the same filenamecontext_idsolist_conversations()returns caller-visible IDs instead of hashesLegacy data handling
Legacy sanitized logs are left untouched on disk, but they are not automatically loaded or listed. The old mapping was many-to-one, so even a filename that looks like a safe context ID may contain records written through another ID (for example,
tenant/aandtenanta). Automatic fallback would therefore reopen the cross-context leakage this patch prevents. Manual recovery remains possible from the unchanged legacy files.Tests
python -m pytest tests/plugins/test_a2a_plugin.py -o 'addopts=' -q— 120 passedruff check plugins/platforms/a2a/protocol.py tests/plugins/test_a2a_plugin.py— passedgit diff --check— passedI also ran
tests/plugins: 1,336 passed, 3 skipped, and 15 failed outside this change's focused gate. Those failures were in optional Hindsight/FAL dependencies plus one order-dependent A2A skill assertion that passes whentest_a2a_plugin.pyis run in isolation; this PR does not modify those paths.Prior work and coordination
This adapts the collision-resistant SHA-256 storage approach originally implemented by @kuangmi-bit in #64982 /
067e2137; the commit retains co-author attribution. I asked in #77526 whether they prefer carrying the fix there or as this focused follow-up. This PR is intentionally a draft pending that coordination.Fixes #83701