fix(codemode): recover tool names that miss by namespace convention - #33
PierrotAWB wants to merge 1 commit into
Conversation
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
The following comment was made by an LLM, it may be inaccurate: |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/codemode/src/tool-runtime.ts">
<violation number="1" location="packages/codemode/src/tool-runtime.ts:710">
P2: Every failed call scans all tool-tree nodes through depth four, then traverses each candidate again; large tool registries make `UnknownTool` handling scale with registry size. Build a leaf index once when creating the runtime, or stop collecting once enough candidates establish ambiguity.</violation>
<violation number="2" location="packages/codemode/src/tool-runtime.ts:776">
P2: When an ambiguous candidate contains a non-identifier segment such as `resolve-library-id`, this message emits invalid CodeMode syntax. Format candidates with the existing bracket-notation path formatter so the recovery hint remains actionable.</violation>
<violation number="3" location="packages/codemode/src/tool-runtime.ts:854">
P3: Because recovery is silent and telemetry now records only the resolved path, recovered calls are indistinguishable from direct calls in `calls`, `toolCalls`, and the `onToolCallStart`/`onToolCallEnd` hooks. The PR's rollout note says to monitor the host-naming fixes to avoid reintroducing the problem, but with no marker for recovery the frequency of these naming misses cannot be measured, and the original mis-written name is dropped everywhere. Surface the written name (or a `recovered` flag) alongside the resolved name so the stopgap's effectiveness is observable.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const suggestions = | ||
| candidates.length > 1 | ||
| ? [ | ||
| `Did you mean ${candidates.map((candidate) => `tools.${candidate.join(".")}`).join(" or ")}?`, |
There was a problem hiding this comment.
P2: When an ambiguous candidate contains a non-identifier segment such as resolve-library-id, this message emits invalid CodeMode syntax. Format candidates with the existing bracket-notation path formatter so the recovery hint remains actionable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/codemode/src/tool-runtime.ts, line 776:
<comment>When an ambiguous candidate contains a non-identifier segment such as `resolve-library-id`, this message emits invalid CodeMode syntax. Format candidates with the existing bracket-notation path formatter so the recovery hint remains actionable.</comment>
<file context>
@@ -673,28 +673,115 @@ const namespaceKeys = <R>(tools: HostTools<R>, path: ReadonlyArray<string>): Rea
+ const suggestions =
+ candidates.length > 1
+ ? [
+ `Did you mean ${candidates.map((candidate) => `tools.${candidate.join(".")}`).join(" or ")}?`,
+ "Use tools.$codemode.search({ query }) to find available described tools.",
+ ]
</file context>
| `Did you mean ${candidates.map((candidate) => `tools.${candidate.join(".")}`).join(" or ")}?`, | |
| `Did you mean ${candidates.map((candidate) => toolExpression(candidate.join("."))).join(" or ")}?`, |
| const found: Array<ReadonlyArray<string>> = [] | ||
| const walk = (node: HostTools<R>, prefix: ReadonlyArray<string>): void => { | ||
| if (prefix.length >= MAX_RECOVERY_DEPTH) return | ||
| for (const key of Object.keys(node)) { |
There was a problem hiding this comment.
P2: Every failed call scans all tool-tree nodes through depth four, then traverses each candidate again; large tool registries make UnknownTool handling scale with registry size. Build a leaf index once when creating the runtime, or stop collecting once enough candidates establish ambiguity.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/codemode/src/tool-runtime.ts, line 710:
<comment>Every failed call scans all tool-tree nodes through depth four, then traverses each candidate again; large tool registries make `UnknownTool` handling scale with registry size. Build a leaf index once when creating the runtime, or stop collecting once enough candidates establish ambiguity.</comment>
<file context>
@@ -673,28 +673,115 @@ const namespaceKeys = <R>(tools: HostTools<R>, path: ReadonlyArray<string>): Rea
+ const found: Array<ReadonlyArray<string>> = []
+ const walk = (node: HostTools<R>, prefix: ReadonlyArray<string>): void => {
+ if (prefix.length >= MAX_RECOVERY_DEPTH) return
+ for (const key of Object.keys(node)) {
+ if (isBlockedMember(key)) continue
+ const value = node[key]
</file context>
| // tool that actually ran, not the name the program wrote. | ||
| const resolved = resolve(callableTools, path) | ||
| const tool = resolved.tool | ||
| const name = resolved.path.join(".") |
There was a problem hiding this comment.
P3: Because recovery is silent and telemetry now records only the resolved path, recovered calls are indistinguishable from direct calls in calls, toolCalls, and the onToolCallStart/onToolCallEnd hooks. The PR's rollout note says to monitor the host-naming fixes to avoid reintroducing the problem, but with no marker for recovery the frequency of these naming misses cannot be measured, and the original mis-written name is dropped everywhere. Surface the written name (or a recovered flag) alongside the resolved name so the stopgap's effectiveness is observable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/codemode/src/tool-runtime.ts, line 854:
<comment>Because recovery is silent and telemetry now records only the resolved path, recovered calls are indistinguishable from direct calls in `calls`, `toolCalls`, and the `onToolCallStart`/`onToolCallEnd` hooks. The PR's rollout note says to monitor the host-naming fixes to avoid reintroducing the problem, but with no marker for recovery the frequency of these naming misses cannot be measured, and the original mis-written name is dropped everywhere. Surface the written name (or a `recovered` flag) alongside the resolved name so the stopgap's effectiveness is observable.</comment>
<file context>
@@ -758,15 +845,19 @@ export const make = <R>(
+ // tool that actually ran, not the name the program wrote.
+ const resolved = resolve(callableTools, path)
+ const tool = resolved.tool
+ const name = resolved.path.join(".")
const call = { name }
const recordAndObserve = (input: unknown) =>
</file context>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
0ab4eb1 to
6c2ccab
Compare
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/codemode/src/tool-runtime.ts">
<violation number="1" location="packages/codemode/src/tool-runtime.ts:701">
P2: When a tool is nested below multiple namespaces, a wrong root namespace still returns `UnknownTool` even when the leaf has one unique match elsewhere. The `path.length === 2` guard limits recovery to root namespace/leaf calls; recursively search the callable tree while preserving the remaining path.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (leaf === undefined || parent === undefined) return [] | ||
| const guesses: Array<ReadonlyArray<string>> = [] | ||
| if (leaf.startsWith(`${parent}_`)) guesses.push([...path.slice(0, -1), leaf.slice(parent.length + 1)]) | ||
| if (path.length === 2) { |
There was a problem hiding this comment.
P2: When a tool is nested below multiple namespaces, a wrong root namespace still returns UnknownTool even when the leaf has one unique match elsewhere. The path.length === 2 guard limits recovery to root namespace/leaf calls; recursively search the callable tree while preserving the remaining path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/codemode/src/tool-runtime.ts, line 701:
<comment>When a tool is nested below multiple namespaces, a wrong root namespace still returns `UnknownTool` even when the leaf has one unique match elsewhere. The `path.length === 2` guard limits recovery to root namespace/leaf calls; recursively search the callable tree while preserving the remaining path.</comment>
<file context>
@@ -673,7 +673,42 @@ const namespaceKeys = <R>(tools: HostTools<R>, path: ReadonlyArray<string>): Rea
+ if (leaf === undefined || parent === undefined) return []
+ const guesses: Array<ReadonlyArray<string>> = []
+ if (leaf.startsWith(`${parent}_`)) guesses.push([...path.slice(0, -1), leaf.slice(parent.length + 1)])
+ if (path.length === 2) {
+ for (const key of Object.keys(tools)) if (key !== parent) guesses.push([key, leaf])
+ }
</file context>
|
This is a bandaid. Better to just name the tools consistently. |
1 participant
When the agent calls a tool by a name that is one naming convention away from the real one, the call now finds the tool instead of failing. Today it fails, the user loses the turn, and the mistake was not really the model's — our own tool names are not self-consistent, so there is no rule for it to follow.
Context
Found while investigating the
executetool's error rate on Replo's/agent-toolsdashboard. Over 30 days of production traffic (external users only),Unknown toolaccounts for 132 failed calls, 16% of allexecutefailures — the second-largest class after JSON-string args (fixed separately in #32).These read like hallucinations. They are not: every failing name has a real tool sitting one convention away from it.
bedrock.bedrock_generate_design_directions(29 fails) →bedrock.generate_design_directions, which served 294 real callsbedrock.bedrock_find_assets(24) →bedrock.find_assets(582 calls)bedrock.bedrock_generate_image(22) →bedrock.generate_image(1,037 calls)shopify.shopify_products_search(9) →bedrock.shopify_products_search(1,273 calls)The cause is on our side. Replo's bedrock MCP server registers 64 tools; 11 carry a redundant
bedrock_prefix (all thebedrock_database_*ones) and 53 do not. Because the CodeMode namespace is alsobedrock, all of these are things a model legitimately observes:There is no rule to learn, so it generalizes from the prefixed ones and loses a turn roughly one time in ten on
generate_design_directions. The second shape is a namespace guess: everything Shopify is proxied underbedrock.shopify_*, andshopifyis the obvious namespace to reach for.Fixing the host naming is the real cure and should still happen, but it is a rename across live tools, it only helps that one host, and the same trap reappears the next time anyone registers a prefixed tool. This makes the runtime tolerant of the mistake in the meantime.
What changed
resolvepreviously walked the path and threw on the first missing segment. It now retries an unresolved path two structural ways — a namespace repeated inside the tool name, and the same tool name found anywhere else in the tree — and proceeds only when exactly one candidate resolves.Two properties worth calling out:
onToolCallStart/onToolCallEndand everything downstream stay honest. Without this the fix would quietly corrupt exactly the dashboard that found the bug.resolvenow returns the tool and the path that reached it. The walk is shared with a non-throwingnodeAt, so there is one traversal rather than three, and the two existing error messages (Unknown toolvsnot callable) are preserved exactly.What to scrutinize
The scope of the candidate search is the judgment call. "Same leaf name anywhere in the tree" is broader than the two observed mistakes and could in principle retarget a call across namespaces in a way the author didn't intend — bounded by the uniqueness requirement, which is what makes it safe, but worth your eyes. The narrower alternative is prefix-stripping only, which fixes ~79 of the 132 and leaves the
shopify.*class failing.Also worth deciding: whether silent recovery is right at all versus an error that names the correct tool. I chose recovery because the error still costs the turn, and these paths have exactly one possible meaning. The suggestion machinery is already in place if you'd rather flip it.
Testing Done
bun testinpackages/codemode(268 pass),bun turbo typecheckacross all 30 packages, andoxlintat the same 8 pre-existing warnings asdev— none in new code.New
test/recovery.test.ts, written to fail first:bedrock_database_query_records) is left aloneSummary by cubic
Makes the CodeMode runtime recover tool calls whose names are one naming convention away from the real tool, instead of failing the whole turn. When a path misses,
resolveretries it two structural ways and proceeds only when exactly one candidate resolves; ambiguous cases still fail, and the error now names the candidates. Telemetry records the resolved path, soonToolCallStart/onToolCallEndreport the tool that actually ran.Written for commit 6c2ccab. Summary will update on new commits.