fix(core): warn when tools.eager entries match no discovered tool - #12451

Critical-only review at ea5d4262, base a64d8dec.

Verdict: APPROVE — no review or thread has ever been filed against this PR, so there is no historical blocker to re-check, and my own scan found no Critical. The triage pass reported no blockers either; I did not rely on that and verified the load-bearing claims in code.

The diagnostic cannot break discovery

This was the first thing to establish, because all three call sites sit on completion paths where an exception would be attributed to discovery rather than to a warning. The whole body of warnOnUnmatchedEagerToolEntries is wrapped in try { … } catch {} and documented as never throwing. The three insertions are one import and three one-line calls, each after the registry has settled: the per-session bulk pass after its finally has decremented bulkPassDepth and flushed any refused batch; the pooled pass after setMCPDiscoveryState(COMPLETED) and its mcp-client-update emit; and the incremental pass after the emit that the AppContainer batch-flush subscriber depends on for observing the terminal state. Nothing existing is reordered, and the diff is purely additive at +510/-0.

The matcher is the same oracle as the gate it warns about

A warning that fires on correctly spelled entries would be worse than no warning, so I checked the matching rather than trusting the description. toolMatchesRuleToolName(ruleToolName, contextToolName) is the signature in rule-parser.ts:415, and the new code calls it as toolMatchesRuleToolName(entry, name) with entry the parsed rule name and name a registered tool name — the same order permission-manager.ts:949 uses for (eagerName, canonicalName). keptEagerEntries also mirrors the manager: it skips rule.invalid and pushes rule.toolName, which is what permission-manager.ts:604 and 1333 do. Reversed arguments here would have produced one spurious warning per entry per session, and they are not reversed.

Candidates come from registry.getAllToolNames() rather than from instantiated-tool membership, which is the right choice and the one the module documents: set membership would false-positive on meta-categories and aliases and would miss built-ins that are still unwarmed lazy factories. The test at line 200 pins exactly that case.

The two suppression classes are sound, and asymmetric in the right direction

isOnUnconnectedServer suppresses only when the server name is a configured key and its status is not CONNECTED. It tests that with Object.prototype.hasOwnProperty.call(servers, serverName) rather than in, so a server literally named constructor or toString cannot false-match off the prototype and silently suppress a real typo. An entry whose server is not configured at all stays reportable, which is correct: that is a spelling mistake, not an outage. Empty server names and non-mcp__ entries both return false early.

Suppression is deliberately not written to the warn-once ledger, so a later pass — once the server is up — still reports the entry if the name really is wrong. computer_use__* entries are skipped by the same continue that precedes the ledger write, so they are re-evaluated each pass rather than permanently silenced. Both asymmetries are the safe direction: a suppressed entry can still be reported later, and nothing is recorded as warned unless a warning actually went out.

Warn-once is per session and cannot leak or repeat

The ledger is a WeakMap<Config, Set<string>>, so it is keyed per session, dies with it, and never touches global scope. A /mcp reconnect, a hot-reload reconcile or a second startup pass therefore does not repeat a warning the operator has already seen, and the retained set is bounded by the number of eager entries. Cost is only incurred when it can matter: three early returns (no permission manager or the allowlist is inactive, no registry, no entries) precede any work, and the loop is small in both dimensions.

Two smaller things I checked rather than assumed

console.warn is the established operator-facing channel in this package, not a new convention — there are 26 non-test uses under packages/core/src, including the message display dispatcher and the stream guards. And Config.getEagerTools() returns readonly string[] | undefined, which is what the ?? [] and the typeof raw !== 'string' guard in keptEagerEntries are written against.

Test coverage

The suite pins every branch in the implementation rather than a happy path: warns on an unmatched dynamic entry, on the legacy blocking pass, and on a misspelt built-in; stays quiet when every entry resolves, for meta-category and alias entries, for a tool that is only an unwarmed lazy factory, and when no allowlist is active; warns each entry once across re-discovery; does not re-report entries initialize() already dropped. The unconnected-server group covers a correctly spelled tool on a down server, a client-budget refusal, the case where the server is connected and simply has no such tool, an unknown server name as a typo, and the re-check-after-suppression behaviour. computer_use__* has its own case. I found no assertion that could pass against a broken implementation.

CI

Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox) and both Desktop Shell lanes pass at this head. web-shell E2E Smoke is still pending and I did not wait on it; nothing about this change touches the web shell. The red Test lane the author reported on the previous commit was in scripts/tests/package-scripts.test.js, which none of these three files can influence, and the lane is green after the base merge — that is the only part of that account I needed to confirm, and it holds.

APPROVE at ea5d426.

• Head re-verified immediately before submitting: ea5d426, OPEN and mergeable. No formal review and no inline thread existed on this PR before mine, so there was no historical blocker to clear.
• Required CI green on this head, with review-pr excluded as reviewer-bot infrastructure per my standing disclosure; Test (macos/windows) and Integration Tests (CLI, No Sandbox) are skipped repo-wide.
• No new Critical. The load-bearing property of this change is that the existence check cannot disagree with the mechanism it describes, and it holds. The deferral decision at permission-manager.ts:945-952 is eagerToolAllowList.some(eagerName => toolMatchesRuleToolName(eagerName, canonicalName)), and the allowlist is built by the same shape filter that keptEagerEntries re-derives verbatim: drop non-strings and empties, drop what parseRule marks invalid, otherwise keep rule.toolName. Same matcher, same argument order, same candidate names. An entry that really does keep a tool eager therefore cannot be reported. The corollary settles the wildcard question, which is the first thing I went looking for: toolMatchesRuleToolName has no glob branch, so an mcp__* style entry keeps nothing eager today either, and reporting it is correct rather than a false positive.
• The other two false-positive risks are handled and each is pinned by its own test: meta-category and alias entries match through the same READ_TOOLS, EDIT_TOOLS and shell-covers-monitor branches the mechanism uses, and a built-in that is still an unwarmed lazy factory still matches because the candidate set is getAllToolNames rather than the set of instantiated tools.
• The suppression logic points the right way. A correctly spelled tool on a configured server that is not connected is an outage rather than a typo and stays quiet, and that suppression is deliberately not written into the warn-once ledger, so a later pass still reports the entry if it proves wrong once the server is up; an entry whose server is not configured at all stays reportable. The computer_use__ exclusion states its reason instead of hiding it.
• It cannot damage what it runs inside. The whole body is wrapped so a diagnostic can never fail a discovery pass, it returns early unless the allowlist is active, and the warn-once ledger is a WeakMap keyed by Config, so a reconnect or a hot-reload reconcile neither repeats a warning nor leaks past the session. Emitting through console.warn with that eslint-disable reasoning matches the sibling allowlist warning already at permission-manager.ts:344, so it follows the established pattern for this feature rather than introducing one.
• All three call sites are settled-registry points (the per-session bulk pass, the legacy blocking pass, and the startup plus hot-reload reconcile path), which is where this has to run because MCP tools are discovered after initialize().