Skip to content

fix(core): refresh MCP tools before session snapshots - #54942

Open
Dante-dan wants to merge 1 commit into
anomalyco:devfrom
Dante-dan:mcp-resume-catalog
Open

Dante-dan wants to merge 1 commit into
anomalyco:devfrom
Dante-dan:mcp-resume-catalog

Conversation

@Dante-dan

@Dante-dan Dante-dan commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Issue for this PR

Closes #54794

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

Refresh the connected MCP tool inventory before a session takes its tool snapshot. Previously, flush only waited for initial registration; an ACP-resumed turn could miss an already-connected tool while its catalog update waited in the 100ms debounce and produce a false removal notice. The refresh shares the reconciliation lock and skips unchanged inventories.

How did you verify your code works?

  • The new registry regression fails with the previous initial-only flush and passes with this change, without advancing the debounce timer.
  • packages/core: bun run test test/mcp.test.ts --timeout 30000 — 78 passed, 1 failed. The failure was terminates MCP descendants after the wrapper exits successfully hitting its internal 3s timeout.
  • With Node 24 and dependencies hydrated from the existing lockfile, the targeted rerun of the new catalog regression, notification coalescing and descendant cleanup test passed 3/3.
  • packages/core: bun run typecheck passed; affected-file formatting and git diff --check passed.
  • Root lint passed with Node 24. The full monorepo typecheck encountered sandboxed Astro telemetry writes and then a missing aws4fetch dependency in the console app; affected-core typechecking passed.
  • The author's native ACP/real-model reproduction was not run; this validates the stale connected-catalog boundary.

Screenshots / recordings

No UI changes.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

Added by the opencode bot (AI-generated).

Related: #39902

@github-actions

Copy link
Copy Markdown
Contributor

The following comment was made by an LLM, it may be inaccurate:

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks sound to me, and I have no blocking concerns.

How I checked it: the new regression test fails on the base and passes with this PR. test/mcp.test.ts has one failure, the descendant-cleanup timeout, and the base has the same one. Session tests and the packages/core type check pass. I could not run the ACP resume reproduction from #54794, because it needs the free hosted model, which rejects local builds, and a mock provider does not trigger the race. So I have only verified the fix at the registry level, not end to end over ACP.

On cost: flush runs before every step, and it now takes the reconcile lock and calls MCP.tools(). That is an in-memory read, so the extra work per step is small. The unchanged-list check by reference (===) is correct, because entries are only replaced with new objects when the server re-lists its tools.

Public API: unchanged.

Overlap: #52214 (open) adds a separate refresh that the MCP add/remove/connect/disconnect routes await, and it keeps flush limited to waiting for the initial registration. Folding the refresh into flush, as this PR does, also covers those routes for the next session snapshot, so the two PRs should be reconciled before both land.

Related: #39902

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ACP resume sometimes marks a reattached MCP tool as removed

1 participant