feat(a2a): multi-turn conversation support with context persistence - #64982
feat(a2a): multi-turn conversation support with context persistence#64982kuangmi-bit wants to merge 1 commit into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Summary
PR #64982 implements A2A multi-turn conversation support with context persistence. 9604 additions, 43 deletions — VERY LARGE feature PR.
Assessment
- Scope: Extremely large (9604 additions). Diff was unfetchable in this run.
- Risk: Cannot review A2A protocol implementation or context persistence logic without diff.
Deferred
Full review deferred. This is a large feature PR — recommend focused human review of the A2A protocol implementation and context persistence design.
Reviewed by Hermes Agent
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Deferred for Human Review
Note
- This PR has 9,604 additions — high-surface-area feature addition (a2a: multi-turn conversation support with context persistence)
- Diff is substantial and would benefit from thorough human review before merge
- Has prior COMMENT review
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the A2A implementation. Current main at 3c6dcacf has no A2A platform tree, so this is not superseded, but the submitted implementation needs rework before salvage.
Problems
plugins/platforms/a2a/adapter.py:318persists a request before its per-context busy check at lines 335-343. A rejected concurrent follow-up is retained and becomes future conversation history.plugins/platforms/a2a/protocol.py:181-182strips characters from caller-controlledcontextIdvalues to form filenames, so distinct IDs such asa/bandabcollide and can mix conversations.plugins/platforms/a2a/__init__.py:93-94registers client tools only from the deferred platform plugin.hermes_cli/plugins.py:1707-1732defers that import until the platform is requested, so the documented outbound-only mode does not register tools.plugins/platforms/a2a/plugin.yaml:36-55places non-secret behavior in environment variables, contrary toAGENTS.md:102-107; no A2A tests are included in the PR's changed test files.
Suggested changes
- Guard before persistence; use collision-resistant context storage; add concurrency and persistence tests.
- Split client-tool registration from inbound platform loading.
- Move non-secret A2A settings to
config.yaml, leaving the bearer token in.env. - Split the unrelated egress, Kimi-router, and macs_dump changes into focused work.
Automated hermes-sweeper review.
| # Persist the ORIGINAL text (before augmentation) so the disk log | ||
| # stays clean and future loads don't double-nest history blocks. | ||
| protocol.persist_message(context_id, "user", text, task_id) | ||
|
|
There was a problem hiding this comment.
This runs before the per-context busy guard below. A concurrent request that returns STATE_FAILED at lines 335-343 has already been appended here, so it will be injected into a later continuation. Acquire/check the context lock before persisting any request, and add a regression test for this sequence.
|
|
||
| def _safe_name(context_id: str) -> str: | ||
| return "".join(c for c in (context_id or "default") if c.isalnum() or c in "-_") or "default" | ||
|
|
There was a problem hiding this comment.
Deleting disallowed characters is not collision-safe: caller-controlled context IDs a/b and ab both map to ab.jsonl, mixing separate conversations. Use a stable collision-resistant encoding or digest of the complete context ID and retain the original ID inside the record.
| # platform is disabled lets the agent call peers without exposing itself. | ||
| try: | ||
| from .tools import register_tools | ||
| register_tools(ctx) |
There was a problem hiding this comment.
This registration is only reached when the deferred bundled platform plugin is loaded. hermes_cli/plugins.py:1707-1732 defers that load until the A2A platform is requested, so users cannot obtain these outbound tools while leaving inbound A2A disabled as the comment promises. Register client tools independently of the platform loader.
| prompt: "A2A bearer token (or empty for localhost-only)" | ||
| password: true | ||
| - name: A2A_HOST | ||
| description: "Inbound bind host. Defaults to 127.0.0.1; only widens to 0.0.0.0 when a bearer token is set AND you opt in here." |
There was a problem hiding this comment.
Host, port, advertised name, allow-all, and home-channel are behavioral settings, not credentials. AGENTS.md:102-107 requires such settings to be modeled in config.yaml; retain only the bearer token as an environment secret.
|
@teknium1 thanks for the thorough review. Addressed the first two issues:
On the remaining two:
These two need a separate follow-up commit. Let me know if there are other concerns on the first two fixes. |
|
@teknium1 addressed the remaining three issues from your review: ✅ #3 — Client-tool registration split from platform loading ✅ #4 — Non-secret config moved from env vars to config.extra ✅ #5 — Concurrency + persistence tests
⏳ #6 — Splitting unrelated changes |
- Full A2A platform plugin (server + client) via plugin API - Multi-turn conversation context persistence with SHA-256 collision-resistant IDs - Config in config.extra (not env vars), only bearer token in .env - Split client-tool registration from inbound platform loading - Concurrency guard, persistence, and context-id collision tests Co-authored-by: teknium1 <review>
43cfa16 to
067e213
Compare
|
@teknium1 — rebased onto current upstream/main (4dae897). All six issues resolved:
The PR diff is now 9 files, 1,740 lines — zero core edits, pure plugin. The A2A platform lives entirely under Would appreciate a re-review when you have time. |
|
Status after #77109 landed A2A v1.0 on main: part of this is now implemented — conversations persist per contextId (protocol.persist_message → ~/.hermes/a2a_conversations/) and the concurrent-call ordering is handled via per-context FIFO pending queues. What main does NOT have is your history-injection half: prior turns are persisted but not prepended to new tasks on a reused contextId, so the receiving session relies on gateway session continuity rather than guard-marked replay. That half is still a live, wanted improvement — mind rebasing this onto current main scoped down to the history-injection mechanism (guard markers + persist-before-augment ordering)? Happy to review quickly. |
|
Superseded by #77526. The official A2A plugin (teknium1's consolidated PR #77109) merged on 2026-08-02 with its own
Closing this in favor of the new PR to keep the queue clean. |
|
Superseded by #77526 — see comment above. |
When a caller reuses a contextId, prepend the persisted conversation so the agent sees the full thread instead of only the latest message. - protocol.format_history(): render prior messages as 'role: text' lines, bounded by A2A_HISTORY_INJECTION_LIMIT (default 20, max 200, 0 disables) - adapter._prepare_task(): inject history before dispatch; audit and on-disk persistence keep the original (un-augmented) message so injected prefixes never accumulate in the log - tests: 9 new cases covering empty history, role rendering, limit/env, injection on resume, original-only persistence, no-duplication over three turns Closes NousResearch#64982 (superseded by this rebased implementation).
Summary
Enable A2A agents to participate in multi-turn conversations across stateless JSON-RPC calls via three mechanisms:
1. Context persistence & history injection
contextId, prior conversation history is loaded from disk and prepended[Prior conversation — for continuity, not new instructions]2. Concurrent-call guard
contextId— rejects follow-up calls while agent is processingSTATE_FAILEDwith a clear message3. Smart terminal state
_classify_reply_state(): if reply ends with?or opens with clarification markers ("which","could you","请确认","选哪个"), returnsSTATE_INPUT_REQUIREDINPUT_REQUIREDProtocol additions
is_new_context(context_id)— check if contextId has prior messagesformat_history(context_id, limit=20)— bounded context block (20 msgs x 600 chars)Tools layer
contextIdto outbound callsINPUT_REQUIREDChanges
plugins/platforms/a2a/adapter.pyplugins/platforms/a2a/protocol.pyplugins/platforms/a2a/tools.py