fix(acp): keep agent-native tool calls out of dispatch correlation

[dosenr]

Related issue

Closes #4594. Extracted from review of #2211 (#2211 (comment)), which depends on this change and remains parked as a conflicting draft.

Summary

ACP agents report both agent-native tools and Omnigent MCP bridge tools through the same tool_call update. Treating every update as bridge dispatch state leaves a native call in the correlation queue, so the next bridge completion can attach to the wrong card. Native cards also disappear after reload because only their initial, in-progress item was emitted.

This change snapshots the exact bridge aliases advertised at session/new. Only calls matching that immutable set enter dispatch correlation. Other calls are marked self_executed, retain their arguments through completion, and emit a durable completed card.

ELI5: ACP has two tool lanes. This keeps native tools in the display lane and MCP tools in the dispatch lane.

ACP tool update
      |
      +-- advertised bridge alias --> dispatch correlation
      |
      +-- anything else -----------> self-executed card

Unknown shapes fail closed as self-executed. They may render a duplicate card, but cannot poison a later bridge dispatch.

Test Plan

• uv run pytest tests/inner/test_acp_executor.py tests/runtime/harnesses/test_executor_adapter.py -q: 83 passed.
• Stacked with feat(hermes): add hermes-acp streaming harness (ACP transport) #2211, the selected ACP, Hermes ACP, adapter, spawn-env, and readiness regression suite passes: 128 total tests, including existing adjacent coverage.
• uv run pre-commit run --all-files: passed on current main 40fa33f7.
• tests/inner/ and tests/runtime/, excluding two unavailable optional-dependency collection modules: 548 passed, 26 skipped, 6 failed. The same six tests and pytest plugin error reproduce on unmodified 40fa33f7.
• Real Goose 1.41 ACP turn on ee1bf0d6: the bridge tool dispatched once; its humanized request and completion shared the same call ID; no event was marked self-executed.
• Real Hermes ACP turn on stacked feat(hermes): add hermes-acp streaming harness (ACP transport) #2211: a native terminal pair and an MCP bridge pair completed under separate stable IDs.

Seven focused test functions cover native and bridge wire shapes, the nested Goose _meta.goose.toolCall.toolName shape, foreign-server collisions, no-advertised-bridge behavior, immutable session snapshots, session cleanup, durable native completion, and a mixed native-to-bridge dispatch sequence. This replaces the previous roughly 20-function, 680-line proof package with 221 added test lines.

Demo

Real Goose 1.41 bridge turn on this commit:

ToolCallRequest  | omnigent: validation marker | call_3BQVnaIZznhpzmv7DTeHcYLr
dispatch         | validation_marker           | GOOSE_BRIDGE_FINAL_EE1BF0D6
ToolCallComplete | omnigent: validation marker | call_3BQVnaIZznhpzmv7DTeHcYLr
TurnComplete     | GOOSE_BRIDGE_FINAL_EE1BF0D6

The screenshot is a genuine Hermes native-plus-bridge turn from the earlier stacked branch. The final commits were revalidated by direct ACP turns; the UI event shape is unchanged.

Type of change

• Bug fix
• Feature
• UI / frontend change
• Refactor / chore
• Docs
• Test / CI
• Breaking change

Test coverage

• Unit tests added / updated
• Integration tests added / updated
• E2E tests added / updated
• Manual verification completed
• Existing tests cover this change
• Not applicable

Coverage notes

Manual verification covers real Goose bridge dispatch and a stacked Hermes native-plus-bridge turn. Goose ACP did not expose an agent-native tool in this configuration, so native classification is covered live by Hermes and deterministically by the generic tests.

Changelog

Agent-native ACP tool cards persist without mis-pairing the next MCP tool dispatch.

[github-actions]

@dosenr This PR is a Bug fix, Feature, or UI / frontend change but the Demo section is missing or only contains a placeholder.

These change types require a screenshot or screen recording so reviewers can see the new behaviour without checking out the branch. Please update the Demo section with:

• A screenshot or screen recording of the change, or
• A link to a hosted video or GIF showing the new behaviour.

Use N/A only when the change has no user-visible effect whatsoever (e.g. a pure refactor or test-only change). If that's the case, uncheck the relevant type box and check Refactor / chore or Test / CI instead.

[dosenr]

Pushed 19f61f24: classification now anchors on a session-level snapshot of the tool names advertised at session/new (_bridge_tool_names, beside the existing _bridge_server_names snapshot) instead of the mutable per-turn _omnigent_tools. The MCP server and its tools are sent once at session creation, so a later turn with a different tool set could reclassify calls against tools the agent never received for this session. A restarted session re-snapshots. Regression test added; the existing bridge tests now set the session snapshot.

[dosenr]

Status update:

• Applied the repository formatter to the new session-snapshot regression test; the change is mechanical only.
• Pre-commit checks now pass on head b6558343.
• The substantive behavior remains unchanged: bridge/native classification uses the immutable tool set advertised at session/new, and a restarted session snapshots its new set.

[fanzeyi]

@dosenr — this is great, thanks.

[fanzeyi]

@dosenr — this is great, thanks.

[omnigent-ci]

Polly AI Review

Summary of review

This is a well-scoped, well-tested fix. It correctly separates ACP agent-native tool calls from Omnigent MCP-bridge calls so that only advertised bridge aliases enter the adapter's _pending_mcp_call_ids correlation queue, preventing a stray native call from mis-pairing the next real bridge dispatch. The self-executed completion path also emits a durable completed function_call so native cards survive reload. I found no blocking correctness or security issues.

Blocking issues

None.

I verified the core claims against the source:

• Native calls are stamped self_executed, and the adapter's queue push is now gated by not (event.metadata or {}).get("self_executed") (line ~919), so they no longer enter correlation. Bridge tools still round-trip _stable_tool_executor → _pending_mcp_call_ids.popleft() unchanged.
• The self-executed completion block is correctly placed after the if not call_id or call_id in self._dispatched_call_ids: return guard, so it only fires for non-dispatched, real-call-id events — it emits the completed function_call (same call_id) followed by the function_call_output, matching the scaffold's durable-card shape.
• No subclasses of AcpExecutor currently exist, and Goose/Qwen are independent Executor subclasses, so the new _session_mcp_servers helper is a safe internal refactor with no drift risk today.
• _reset_session_state() now also clears _tool_names/_self_executed_args/_bridge_tool_aliases on the error and "Session not found" paths (previously only _session_id/_system_prompt_sent were reset) — a genuine improvement that prevents stale tool state leaking across a respawned session.
• Only 4 files changed; no lockfile or dependency/extras changes.

Security vulnerabilities

None. The classification fails closed (unknown shapes → self_executed), which cannot poison bridge dispatch. No new external input is trusted for control flow, no secrets touched.

Non-blocking notes

• Alias derivation must stay in sync with harness naming. _session_mcp_servers builds bridge aliases from the fixed conventions mcp_{server}_{tool}, mcp__{server}__{tool}, {server}__{tool}, plus the bare {tool} name. If a future ACP harness advertises MCP tools under a different naming shape, its legitimate bridge calls would be misclassified as self_executed. Per the fail-closed design that only risks a duplicate/orphan render (not dispatch corruption), but it's an implicit coupling worth a comment or a follow-up if new ACP agents are onboarded.
• Bare-name collision. When any MCP server is present, aliases includes the bare tool names, so a genuine agent-native tool that happens to share a name with an Omnigent tool would be treated as a bridge call. This is the inherent ambiguity of Goose's humanized titles and is acceptable, but it's the one case where the classifier can be wrong in the "treat native as bridge" direction.
• Minor agent field inconsistency. The observed request emits "agent": ctx.response_id while the self-executed completion emits "agent": self._current_agent or ctx.response_id. Harmless (the fields are separate items deduped by call_id), just slightly asymmetric.

Overall: a clean, correctly-layered fix with focused deterministic tests plus live Goose/Hermes validation. Recommend merge once any human review of the alias-convention coupling is satisfied.

---

_{Automated review by Polly · workflow run}

[dosenr]

@fanzeyi - rebased onto current main (b06722c)

same change, one commit, 4 files. I double checked the original issue still reproduces on that base, so the fix is still needed, and have been running it daily in my deployment for weeks. Executor and adapter suites are green on the new head, including a new interleaving test that pins the exact mis-correlation this fixes.

[dosenr]

Rebased onto current main. Clean rebase, nothing hand-resolved, and the commit is unchanged from the previous head. Same fix you approved in July, just carried forward.

Still needed on today's main: without the fix, the PR's own tests fail 9 of 99. I also re-ran a live Hermes ACP turn on the new head, one native call plus one bridge call in the same turn, and each keeps its own call ID.

---

Numbers, all from commands on the rebased head 05e7ed2d (base: main 4309ef98):

range-diff previous head vs rebased   =  (commit diff identical)
executor + adapter suites             99 passed
tests/inner + tests/runtime           3 failed, 3518 passed, 48 skipped
  (same 3 failures on unmodified main, unrelated modules)
failing-first on plain main           9 failed, 90 passed
pre-commit                            all pass except vscode-tsc, fails identically on plain main

[dhruv0811]

Maintainer takeover. I rebased this onto current main (d3a278098) and reduced the diff, because main absorbed most of it while the PR sat.

The bug is still real on today's main: a native ACP tool_call emits metadata={"call_id": ...} with no gate flag, so its id enters _pending_mcp_call_ids and the next bridge dispatch pops it.

What landed on main since your last rebase:

This PR's piece	On main now
adapter: gate the correlation-queue push	already there, as internally_executed (#5200)
adapter: re-emit a completed function_call so the card survives reload	already there, generically via _observed_tool_calls (#5583)
executor: _self_executed_args	already there, as _tool_inputs
executor: session/new alias snapshot + classification	missing, and the part that matters

Resolving the conflict by keeping both sides would have shipped two metadata keys for one concept, two argument caches, and two durable-card emitters, which means a duplicate card for every observed call. So I dropped the adapter changes entirely and re-pointed the classification at main's existing internally_executed key. Your _reset_session_state() survives as you wrote it.

Net: 4 files / +350 −19 becomes 2 files / +82 −17 plus tests. The design, the wire shapes (including the nested Goose _meta.goose.toolCall.toolName case), and the live Goose/Hermes validation are yours, so you are still the commit author; the reduction is mine.

Tests: the classification coverage now lives in tests/inner/test_acp_executor.py and includes an end-to-end regression that drives the real ACP executor into the adapter and asserts the bridge dispatch keeps its own call id (the bridge-1 receiving native-1 failure from #4594). 7 of 104 fail with the fix reverted. tests/runtime/harnesses/test_executor_adapter.py is 50/50.

Two notes for reviewers:

• The description above still describes the original four-file version. I left it as the author wrote it rather than rewriting someone else's PR text.
• I have not re-run a live Goose or Hermes turn against this reduced commit. The deterministic executor-to-adapter test covers the contract that changed; the durable-card half is now main's code, covered by fix(harness): persist observed tool-call cards so they survive reload #5583.

[omnigent-ci]

Polly AI Review

Blocking issues

None. The core mechanism is sound and matches the existing framework contract:

• internally_executed is the pre-existing metadata key that _executor_adapter.py:732 already honors (also emitted by codex_executor.py). This PR correctly reuses it rather than inventing a parallel convention, so native ACP calls are kept out of _pending_mcp_call_ids and can no longer steal the next bridge dispatch's call_id.
• The dispatch/classification coupling in _session_mcp_servers() is a good structural choice: aliases are snapshotted from the same session_new_servers() payload that creates the relay, so the two cannot drift. The immutable-per-session snapshot is also correct given ACP sends mcpServers only at session/new and the agent keeps that tool set for the session's life; _ensure_session()'s short-circuit on a reused session_id is consistent with this, and _reset_session_state() clears the snapshot on every reset/reconnect path (Session not found, process error, close).
• The durable native card works: a native ToolCallRequest is cached in _observed_tool_calls, is not added to _dispatched_call_ids, and is re-emitted as a completed function_call on ToolCallComplete — so it survives reload, while bridge calls remain suppressed because their id lands in _dispatched_call_ids.
• Fail-closed direction is the safe one: an unrecognized shape classifies as native, which at worst renders a duplicate card but cannot poison correlation.

Security vulnerabilities

None. No new external input handling, deserialization, or trust-boundary changes. The alias set is built purely from Omnigent's own advertised tool/server names.

Non-blocking notes

• Bare-name alias collision. aliases = set(tools) if servers else set() treats the unprefixed Omnigent tool name as a bridge alias. If an agent-native tool happens to share a bare name with an Omnigent MCP tool (e.g. shell, web_search), a native call would be misclassified as a bridge call, and its completion would not produce a durable card via the observed path. This seems rare and is arguably preferable to the reverse failure, but worth a comment noting the assumption.
• Reverse misclassification cost. If a genuine bridge call were ever not matched by any alias (false negative), _stable_tool_executor would allocate a fresh call_id and the executor's ToolCallComplete would not be suppressed, yielding a duplicate/ghost card. The alias set covers the shapes seen so far; if new agents surface other machine-name shapes this is the failure mode to watch.
• Description vs. code term. The description repeatedly says self_executed, but the code uses internally_executed. Purely a wording mismatch (no code impact) — worth aligning the prose to avoid confusion for future readers grepping the key.

Summary

A focused, well-scoped bug fix that correctly separates ACP agent-native tool calls from MCP-bridge dispatch by snapshotting the advertised bridge aliases at session/new and reusing the existing internally_executed metadata contract. The change is internally consistent, resets its state on every session-invalidation path, and is backed by targeted tests covering the wire shapes, no-relay behavior, snapshot immutability, and the original native-before-bridge defect. A real Goose bridge turn and a Hermes native-plus-bridge screenshot are included, satisfying the visual-demonstration expectation for this user-visible card change. No blocking or security concerns; ready to merge pending the minor notes above.

---

_{Automated review by Polly · workflow run}

[github-actions]

🏷️ Doc impact: no-doc-update

Internal fix to ACP executor tool-call classification (agent-native vs MCP-bridge dispatch correlation) with tests; no user-facing surface, integration, or documented behavior changed.

_{Auto-classified on merge. Set the label manually before merging to override. · run}