-
Notifications
You must be signed in to change notification settings - Fork 826
fix(adapters): decide code mode from tool semantics, not from the name exec #1951
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
bc22943
8a40403
b296b0a
e2720f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,8 +17,27 @@ import { | |
| // included it either. | ||
| const NEIGHBOR_AGENT_TOOL_NAMES = ["Read", "Grep", "Glob", "Bash", "LS"] as const; | ||
|
|
||
| /** | ||
| * The two halves of the code-mode shape, kept provider-neutral here. | ||
| * | ||
| * `./cursor/tool-definitions.ts` owns the Cursor-scoped versions of these | ||
| * (`isCursorCodeModeExecTool` / `isBareCodexShellBridgeTool`), but those additionally require | ||
| * the Cursor Responses namespace. This nudge is shared by Anthropic, Google, Kiro, | ||
| * OpenAI-chat and command-code, so it needs the same semantics without that provider gate. | ||
| */ | ||
| const CODEX_UNIFIED_EXEC_TOOL_NAME = "exec"; | ||
| const CODEX_SHELL_BRIDGE_TOOL_NAMES = ["exec_command", "shell_command"] as const; | ||
|
|
||
| function isCodexCodeModeExecTool(tool: Pick<OcxTool, "name" | "freeform">): boolean { | ||
| return tool.name === CODEX_UNIFIED_EXEC_TOOL_NAME && tool.freeform === true; | ||
| } | ||
|
|
||
| function isBareShellBridgeTool(tool: Pick<OcxTool, "name">): boolean { | ||
| return (CODEX_SHELL_BRIDGE_TOOL_NAMES as readonly string[]).includes(tool.name); | ||
| } | ||
|
|
||
| function quoteNames(names: readonly string[]): string { | ||
| return names.map(name => `\`${name}\``).join(", "); | ||
| return names.map(name => "`" + name + "`").join(", "); | ||
| } | ||
|
|
||
| function uniqueNames(names: readonly string[]): string[] { | ||
|
|
@@ -40,48 +59,83 @@ export function shouldInjectNonOpenAIToolCatalogNudge(provider: Pick<OcxProvider | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Codex code mode is a SEMANTIC property, not a name. | ||
| * | ||
| * The tool that carries it is a `freeform` `exec` whose body is JavaScript evaluated in a V8 | ||
| * isolate, advertised alongside no bare shell bridge. A provider is free to advertise an | ||
| * ordinary structured tool called `exec` that runs a shell string — and a catalog can list | ||
| * `exec` next to `exec_command`/`shell_command`, which is the flat-bridge shape, not code mode. | ||
| * | ||
| * Classifying on the name alone would tell those turns that `exec` takes JavaScript and that | ||
| * shell is only reachable as a nested `tools.*` helper. Both are false there, and a model that | ||
| * believes them sends the wrong arguments or avoids a legitimate execution tool entirely. | ||
| * | ||
| * So callers that HAVE the tool objects decide with the semantic predicate and pass the verified | ||
| * wire name in; the name-only entry point cannot decide it and does not try. | ||
| */ | ||
| function codeModeExecWireName( | ||
| advertised: ReadonlySet<string>, | ||
| verifiedName: string | undefined, | ||
| ): string | undefined { | ||
| if (!verifiedName) return undefined; | ||
| return advertised.has(verifiedName) ? verifiedName : undefined; | ||
| } | ||
|
|
||
| export function buildNonOpenAIToolCatalogNudgeFromNames( | ||
| wireNames: readonly string[] | undefined, | ||
| toWireName: (name: string) => string = name => name, | ||
| codeModeExecName?: string, | ||
| ): string | undefined { | ||
| const names = uniqueNames(wireNames ?? []); | ||
| if (names.length === 0) return undefined; | ||
|
|
||
| const advertised = new Set(names); | ||
| // Compare in the catalog's own coordinate system. `advertised` holds WIRE names, so a | ||
| // provider that rewrites them (Claude OAuth `custom_`, Anthropic compat `cx_`) would never | ||
| // match a bare neighbor name and would forbid tools the turn actually advertises — the | ||
| // match a bare neighbor name and would forbid tools the turn actually advertises -- the | ||
| // catalog would list `custom_apply_patch` while the same sentence banned `apply_patch`. | ||
| const unavailableNeighborNames = NEIGHBOR_AGENT_TOOL_NAMES.filter( | ||
| name => !advertised.has(name) && !advertised.has(toWireName(name)), | ||
| ); | ||
| const verifiedCodeModeExecName = codeModeExecWireName(advertised, codeModeExecName); | ||
|
|
||
| return [ | ||
| "Tool contract: use the current tool catalog as ground truth.", | ||
| `Valid tool names for this turn are exactly ${quoteNames(names)}.`, | ||
| "Valid tool names for this turn are exactly " + quoteNames(names) + ".", | ||
| "These listed names are the complete top-level tool-call surface for this turn.", | ||
| "Call only listed names with their listed argument keys; do not invent, translate, or rename tools.", | ||
| "Names mentioned only in instructions, tool descriptions, argument descriptions, or nested helper APIs are not additional top-level tools.", | ||
| "If a listed tool exposes nested helpers such as a tools.* API, call the listed parent tool and use those helpers only inside that tool's input.", | ||
| verifiedCodeModeExecName | ||
| ? "`" + verifiedCodeModeExecName + "` is Codex code mode: its body is JavaScript evaluated in a V8 isolate. Nested helpers are called INSIDE that body as `await tools.<name>(...)`, for example `await tools.exec_command({cmd: \"ls\"})` or `await tools.codex_app__list_threads({})`. Absence from the top-level catalog or from `" + verifiedCodeModeExecName + "`'s description is not absence: deferred helpers stay callable on `tools.<name>`. Discover them from the isolate global `ALL_TOOLS`, not `tools.ALL_TOOLS`. Do not skip an available nested helper because it is omitted from the listed top-level names." | ||
| : "If a listed tool exposes nested helpers such as a tools.* API, call the listed parent tool and use those helpers only inside that tool's input.", | ||
| unavailableNeighborNames.length > 0 | ||
| ? `Do not use neighboring-agent tool names ${quoteNames(unavailableNeighborNames)} unless this turn's catalog lists those exact names.` | ||
| ? "Do not use neighboring-agent tool names " + quoteNames(unavailableNeighborNames) + " unless this turn's catalog lists those exact names." | ||
| : undefined, | ||
| "If you need shell, file search, file read, edit, or discovery behavior, choose the listed tool that provides that capability.", | ||
| "Count a tool call only after its tool result returns; batch independent read-only calls when the runtime supports it.", | ||
| ].filter((line): line is string => typeof line === "string").join(" "); | ||
| } | ||
|
|
||
| export function buildNonOpenAIToolCatalogNudgeForTools( | ||
| tools: readonly Pick<OcxTool, "namespace" | "name">[] | undefined, | ||
| tools: readonly Pick<OcxTool, "namespace" | "name" | "freeform">[] | undefined, | ||
| toolChoice?: OcxRequestOptions["toolChoice"], | ||
| toWireName: (tool: Pick<OcxTool, "namespace" | "name">) => string = tool => namespacedToolName(tool.namespace, tool.name), | ||
| ): string | undefined { | ||
| const visibleNames = tools | ||
| ?.filter(toolChoiceToolPredicate(toolChoice)) | ||
| .map(toWireName); | ||
| const visible = tools?.filter(toolChoiceToolPredicate(toolChoice)); | ||
| const visibleNames = visible?.map(toWireName); | ||
| // Decide code mode from the tool OBJECTS, while the `freeform` flag still exists — reducing | ||
| // to wire names first throws away the only thing that distinguishes Codex's JavaScript | ||
| // `exec` from an ordinary structured tool that happens to share the name. | ||
| const codeModeExecTool = visible?.find(isCodexCodeModeExecTool); | ||
| const codeModeExecName = codeModeExecTool | ||
| && !visible?.some(isBareShellBridgeTool) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In AGENTS.md reference: src/AGENTS.md:L19-L19 Useful? React with 👍 / 👎. |
||
| ? toWireName(codeModeExecTool) | ||
| : undefined; | ||
| // Neighbor names are bare and un-namespaced, so probe the same transform with a bare tool. | ||
| return buildNonOpenAIToolCatalogNudgeFromNames( | ||
| visibleNames, | ||
| name => toWireName({ name }), | ||
| codeModeExecName, | ||
| ); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Check
namespacebefore classifying a shell bridge as bare.Line 35 checks only
tool.name. A namespaced tool such asmcp__other__exec_commandis not a bare shell bridge, but Line 132 will suppress Codex code-mode guidance when it is present. The freeformexectool then receives generic guidance and the model does not receive the nested-helper discovery contract.Include
namespacein the predicate and require it to be absent. Add a regression test with a freeform bareexecplus a namespacedexec_command.Proposed fix
As per path instructions,
src/**changes must not introduce provider or adapter contract drift. The PR objective also requires exclusion only for a visible bare shell bridge.📝 Committable suggestion
🤖 Prompt for AI Agents
Source: Path instructions