[pull] main from danny-avila:main - #213
Merged
Merged
Conversation
* feat: prestart sealed foreground subagent invocations * fix: bind eager dispatch to model attempt configuration * fix: trace early subagents beneath explicit dispatch chains * fix: isolate dispatch trace output and settlement ownership
* 🧲 fix: Match MCP Server Filters Leniently in Tool Search `tool_search`'s `mcp_server` filter compared the requested name to the tool's suffix with `===`, so a near-miss returned nothing and read as an outage. Observed: an agent with 18 ClickHouse tools attached searched with `mcp_server: "mcp-clickhouse"` — the published package name — while the server was keyed `clickhouse`, so every tool was filtered out. It got back "The tool registry is empty or no matching deferred tools are registered", concluded the service was unavailable, and hedged the answer it had been asked for. The tools were in its registry the whole time. Requested names now resolve against the servers that actually have tools: exact first, then case- and separator-insensitive (a host rewrites what it cannot put in a tool name, so `ClickHouse Cloud` becomes `ClickHouse_Cloud` and a model asking for `clickhouse-cloud` means the same server), then with a leading `mcp` token dropped. Trying them in that order keeps a deployment running both `github` and `mcp-github` unambiguous — each resolves to itself. The empty-result message now distinguishes the three cases it used to conflate: nothing registered, the name matched nothing, or the named server has no tools. When a name matches nothing it lists the servers that do exist, which is the line that would have prevented the wrong conclusion above. Tests cover the resolver and the search flow end to end: `mcp-clickhouse` now finds the `clickhouse` tools, and an unknown name reports the available servers instead of claiming an empty registry. The flow tests polyfill `globalThis.crypto`, absent on Node 18, which LangChain's callback manager needs to mint a uuid before the tool body runs. * 🧹 fix: Address Codex Review on Server-Name Resolution Three findings from the review of a8f08f8. P1 — an unfiltered search paid for server bookkeeping it never used. Building `availableServers` ran a map, a filter and a Set over the whole searchable registry on every call, and the result was read only on the server-filtered path. Collection is now one loop that also builds the searchable subset, and the two server indexes exist only when a filter was supplied. P2 — distinct registered names can share a canonical form (`foo-bar` and `foobar` both reduce to `foobar`), and the resolver kept whichever it saw first, so an inexact request was routed to an arbitrary one and the other server's tools disappeared. Every candidate is returned instead. An exact name still wins outright, so this only widens a request that was already ambiguous, and a superset is recoverable by reading the tool names while an arbitrary pick is not. P3 — resolution considered only servers with deferred tools, so a server whose tools are all loaded already was reported as though it did not exist. Filters now resolve against every registered server and the outcome is split: matched with searchable tools, matched with nothing left to search, or genuinely unknown. The empty-result message states which, since telling someone their server does not exist when it does is the same class of misdirection this branch set out to fix. 111 tests pass; tsc and eslint clean on the touched files. * 🩹 fix: Collapse the Unfiltered Search to One Traversal Round two of the Codex review on this branch; four of five findings applied. P1 — the previous "single pass" was worse than what it replaced. It copied the registry with `Array.from`, filled a staging array, then walked that again with `filter` and `map`: four traversals and an extra allocation on the unfiltered hot path. The staging array now exists only when a server filter does, because only then is the set to filter by unknown until the registry has been read. An unfiltered search builds its results during the one traversal it already makes. P2 — the empty-searchable fallback ran before the server diagnosis, so a registry holding only loaded tools answered a request for a real server with "the tool registry is empty or no deferred tools are registered". Both conditions are true there; naming the server is the useful one, so it goes first. P2 — `available_mcp_servers` was emitted at runtime but absent from `ToolSearchArtifact['metadata']`, so a typed consumer could not read what the library returns. Added as an optional field. P3 — a tool named `tool_mcp_` yields an empty server suffix that no filter can name, and it was reaching both indexes; `getAvailableMcpServers` already excludes it. Now skipped, so an unmatched search cannot report an empty server name or emit `['']`. Not applied: the finding that the `node:crypto` import must precede `@jest/globals` for shortest-to-longest ordering. It is the longer line — 57 characters against 53 — and `node scripts/sort-imports.ts` reports the file already sorted, so the current order is what the repository's own tooling wants. 112 tests pass; tsc and eslint clean on the touched files. * 🫧 fix: Refuse Empty Canonical Keys and Report Partial Server Matches Round three of the Codex review; both new findings applied. A registered name with no ASCII alphanumerics — `---`, or a Unicode-only name — canonicalizes to the empty string, and it was indexed under that key. Any request that also canonicalized to nothing then resolved to it, so a search for `!!!` could return an unrelated server's tools. Empty canonical keys are now skipped on both sides: such a server is reachable only by its exact name, and such a request is reported as unmatched. A filter naming several servers can resolve only partly, and the diagnostics lived solely on the empty-result path. Requesting `['github', 'slack']` where GitHub has deferred tools and Slack does not returned GitHub's results while the artifact still echoed both names, which reads as though both were searched. The unresolved and idle halves are now stated on every successful path as well, and carried in `unmatched_mcp_servers` / `idle_mcp_servers`, both added to the exported artifact metadata alongside `available_mcp_servers`. 114 tests pass; tsc clean. * 🧷 fix: Require an MCP Token Boundary and Keep Listings Parseable Round four of the Codex review; all three new findings applied. The `mcp` prefix was stripped from any canonical name beginning with those three letters, so `mcparty` resolved to a server called `arty` — an unknown name quietly returning an unrelated server's tools, the same failure class as the empty canonical key fixed last round. The boundary is now checked on the original string, before canonicalization removes separators: `mcp-clickhouse` and `mcp_clickhouse` still resolve to `clickhouse`, `mcparty` resolves to nothing. The partial-filter notice was prepended to the server listing, whose contract is parseable JSON and whose tests assert it. The diagnostics now travel inside the payload as a `notes` array instead, so `JSON.parse` keeps working. `filterMetadata` reached every nonempty path but not the empty-result return, so a caller could read the diagnosis in the text but not from `unmatched_mcp_servers` / `idle_mcp_servers`. Spread there too. 116 tests pass; tsc clean. * 🔡 fix: Canonicalize Unicode Names and Keep Search Output Parseable Round five of the Codex review; both new findings applied. Canonicalization stripped everything outside `[a-z0-9]`, which folded distinct names together rather than merely dropping separators: `éfoo` reduced to `foo`, so a request for an unregistered `foo` resolved to `éfoo` and returned its tools. Letters and digits of any script are preserved now, and only separators and punctuation are removed. The empty-key guard from the previous round still covers a name that is punctuation alone. The listing fix did not go far enough. `formatSearchResults` also returns JSON, and both the local and code-interpreter success paths still prefixed the partial-filter notice in front of it, so a consumer that had been parsing search output could no longer do so. Those diagnostics now travel inside the payload as a `notes` array, matching the listing. `filterNotice` survives only on the plain-text "no tools matched the pattern" return, which is prose and not parsed. 118 tests pass; tsc clean. * 🔠 fix: Keep Combining Marks and Compose Canonical Server Names Round six of the Codex review; the one new finding applied. Keeping only letters and digits still discarded combining marks, which folds cased Unicode onto ASCII: `İ`.toLowerCase() is `i` followed by a combining dot above, so `İfoo` canonicalized to `ifoo` and a request for a plain `ifoo` resolved to it. Verified before fixing — the old expression returns `ifoo` for both names. Marks are preserved now and the result is composed with NFC, which also settles a second inconsistency in the other direction: a decomposed `e` + acute name and a precomposed `éfoo` previously canonicalized differently, so the same server written two ways failed to match itself. Both now agree. Punctuation-only names still canonicalize to the empty string, so the guard added in round three continues to keep them out of fuzzy matching. 120 tests pass; tsc clean. * 🪶 fix: Canonicalize by Folding, Never by Discarding Round seven raised two more matching errors — `❤️` equalling `☀️` once their symbols were stripped, and `ΟΣ` failing to match `οσ` because `toLowerCase` produces a final sigma. Both are the same defect as the four before them, so this changes the approach rather than adding a sixth guard. Every wrong match this resolver has produced came from deleting characters, because deleting merges two distinct names onto one key: non-ASCII made `éfoo` equal `foo`, combining marks made `İfoo` equal `ifoo`, symbols made `❤️` equal `☀️`, and punctuation-only names all collapsed to the empty string. The request this leniency exists to serve is narrow — a separator written one way when the server uses another — so canonicalization now performs exactly that equivalence and no other. Nothing is discarded. The result folds case, composes with NFC, and collapses runs of `-`, `_`, `.` and space to a single `-`. The uppercase/lowercase round trip approximates the Unicode case folding `toLowerCase` alone does not do, which is what lets `ΟΣ` meet `οσ`. Verified pairwise against the runtime before committing: the six names that previously merged are now distinct, and the two that should agree — the Greek casing pair, and a decomposed name against its precomposed registration — do. 125 tests pass; tsc clean. * 🧊 fix: Fold Case With toLowerCase Alone The uppercase round trip added last round to match a Greek final sigma also equated dotless `ı` with ASCII `i`, which Unicode case folding keeps apart — so a request for an unregistered `ifoo` resolved to a registered `ıfoo` and returned its tools. That is the seventh false match on this branch and, like the others, it came from a rule that merged two identities. Case is folded with `toLowerCase` alone now. Checked pairwise against the runtime across the nine pairs this branch has accumulated: the round trip violates one of them, plain lowercase violates none. The only capability lost is matching `ΟΣ` to `οσ`, and that failure is a missed match rather than a wrong one — it produces the message naming the servers that do exist, which is the outcome this whole branch was written to provide. Also from this round: dropped a comment that restated the empty-suffix check it preceded, and gave the test's `globalThis.crypto` probe the concrete `Crypto` type rather than asserting through `unknown`. 125 tests pass; tsc clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )