fix(responses): fence helper spellings from every manufactured bare alias - #4819
Conversation
…lias
The bare echo alias was fenced; the bare `tool_choice` selector path was not.
A request that declared `mcp__remote.exec` and selected it with a bare selector
-- `{name: "exec"}` or `allowedTools: ["exec"]` -- still put bare `exec` into
`declaredToolNames` from the second loop, and `normalizeDeclaredToolName`
(src/types/tools.ts) then rewrote an undeclared `apply_patch`, `exec_command` or
`write_stdin` onto it. Bare logical selection is a supported request shape
(`toolAllowedByChoice`), so this was constructible, not hypothetical.
Sweeping the rest of the ingress found a third copy of the same rule in a
different file. `collectDeclaredWireToolNames` builds the passthrough declared
catalog and fenced exactly one name, `exec`, so a namespaced `exec_command`,
`shell_command`, `write_stdin`, `apply_patch` or `view_image` still donated its
bare spelling to a set that reaches `normalizeDeclaredToolName`,
`resolveCodeModeHelperName` and `declaresCodeModeExec`. Bare
`exec_command`/`shell_command` there switch nested-helper normalization OFF for
a catalog that genuinely declared the shell; the rest are accepted as declared
under a name the caller only authorized inside a namespace.
Both copies drifted because each site kept its own list, so there is now one:
`NAMESPACED_BARE_ALIAS_EXCLUDED_NAMES` in src/types/tools.ts, derived from
`CODE_MODE_EXEC_TOOL_NAME` and `CODE_MODE_HELPER_TOOL_NAMES` so it cannot fall
out of step with the names it protects. All three sites read it.
What is preserved: selection resolves the bare shorthand against the request
catalog rather than this map, so a bare selector still selects and still forces
the tool; canonical `ns__name` and dotted `ns.name` restoration are untouched;
non-helper names keep both their #4679 echo alias and their selector alias; and a
genuine namespace-free declaration of `apply_patch` keeps its bare name, since
that is the caller declaring a tool rather than a namespace being discarded.
A full inventory of `buildToolBridgeMaps` confirms no remaining path gives a
namespaced helper a bare spelling, and that `freeformToolNames`,
`bareCustomToolNames`, `bareFunctionToolNames` and `toolSearchToolNames` never
reach a declared-name or code-mode argument. The invariant is recorded in
structure/transports/responses.md.
The `mcp__functions.exec` selector case in responses-parser.test.ts moves to a
non-helper name so it still pins the bare-selector contract, and gains an
explicit assertion that a helper spelling stops at canonical and dotted.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe change centralizes six excluded helper names and applies the exclusion to namespaced alias, selector, echo, and passthrough declaration paths. Tests cover declaration filtering and identity restoration. A planning document records a separate deferred preview-fence test. ChangesNamespaced helper alias fencing
Preview fence test planning
Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
…claration CI on #4819 refused a call it should have allowed: `responses-undeclared-tool-guard.test.ts:2047` posts a catalog declaring `mcp__functions.exec` with `tool_choice: {name: "exec"}`, the provider echoes bare `exec`, and the turn answered 502 instead of 200. That test encodes a legitimate shape, and it already proved the boundary holds: the same case refuses `apply_patch`, `exec_command`, `shell_command`, `write_stdin` and `view_image` from the identical request. It could do both because passthrough separates the two jobs a bare alias does. Identity RESTORATION runs first -- `authorizedBareNamespaceToolAliases` is built from `toolNsMap`, rewrites the echoed bare name to the namespaced identity, and the guard then authorizes `ns__name` -- while DECLARATION is withheld, because `declaredWireToolNames` admits bare `exec` from the bridge map only when the client's own catalog declared it. The selector fence withdrew both, so restoration stopped firing and a declared, explicitly selected tool became undeclared. Only the declaration is unsafe: a declared-name set carrying bare `exec` is what makes `normalizeDeclaredToolName` rewrite an undeclared helper name onto the selected tool. So the selector path now writes `toolNsMap` and `toolParameterSchemas` and skips `declaredToolNames` for the six spellings. The caller nominated exactly one tool by name, `bareNameCounts` proves nothing else answers to it, and restoring that identity authorizes nothing the request had not already declared. The echo path keeps withholding both. A bare echo is a guess rather than a nomination, and `tests/responses/bare-echo-alias.test.ts` has pinned the `toolNsMap` exclusion there since #4679. The bridges check the declared set before consulting `toolNsMap`, so a bare helper echo is refused there either way; that is the safe side and it is unchanged. Fixture updated to assert the split rather than a blanket absence, and the `mcp__functions.exec` case in responses-parser.test.ts now pins both halves. structure/transports/responses.md states the invariant on declaration rather than on aliasing.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
리뷰 · 우선순위 75 / 80이 PR은 이미 #4813이 막아 준 것은 이름공간 도구가 bare echo로 헬퍼 철자를 더 위험한 부분은 세 번째 복사본이었습니다. passthrough의 두 번째 커밋이 중요합니다. 첫 펜스가 selector에서 선언과 identity alias를 둘 다 빼 버리자 CI의 라인 223 - echo 경로: 이름공간 헬퍼 여섯 철자는 bare alias를 만들지 않음 (공유 집합 사용). 메인테이너의 판단이 필요한 지점
너의 추천
이 댓글은 grok-bot이 작성했습니다 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b6afa7705
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!NAMESPACED_BARE_ALIAS_EXCLUDED_NAMES.has(t.name)) { | ||
| budget?.chargeRetained(new TextEncoder().encode(t.name).byteLength, { kind: "retained_collectors" }); | ||
| declaredToolNames.add(t.name); | ||
| } |
There was a problem hiding this comment.
When a streaming provider echoes an explicitly selected namespaced helper by its bare name, withholding it from declaredToolNames makes response.function_call_arguments.done fail with undeclared_tool_call: restoreRoutedNamespaceCalls only rewrites function_call and custom_tool_call objects, so the bare name in this standard follow-up event remains unchanged before the guard checks it. Extend namespace restoration to this event shape (and its namespace field), otherwise a stream can accept the added item and then abort at arguments completion.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| isAllowedToolChoice, | ||
| toolChoiceToolPredicate, | ||
| declaresCodeModeExec, | ||
| NAMESPACED_BARE_ALIAS_EXCLUDED_NAMES, |
There was a problem hiding this comment.
This change updates src/server/ and adds a public export in src/types.ts, but only structure/transports/responses.md is updated. structure/INDEX.md maps src/server/ to additional documents and maps src/types.ts to both runtime.md and config.md; the repository rule requires every listed document for each changed area to be updated in the same change, so synchronize those owners before landing.
AGENTS.md reference: src/AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
…lias (lidge-jun#4819) Release-blocker fix for 2.57.0 from the third regression audit. Exact head has a green aggregate ci check. The helper-name fence that landed in lidge-jun#4813 covered the echo-alias path only; the tool_choice compatibility path still manufactured a bare alias, so a request declaring a namespaced exec and selecting it with a bare selector put bare exec into the declared set and an undeclared helper was rewritten onto it. Sweeping for the rule found a third live copy in the passthrough declared catalog, which fenced exactly one name, so a namespaced exec_command or shell_command could also switch nested-helper normalization off for a catalog that genuinely declared the shell. The rule now lives in one place. The follow-up commit corrects an over-strict first attempt: a bare alias does two jobs, identity restoration and declaration, and only the declaration was ever unsafe, so restoration is preserved and a caller-declared, explicitly selected tool keeps working. Host-owned merge decision; no local suite, typecheck, build, or install was run.
1 participant
Summary
The helper-name fence that landed in #4813 covered the echo-alias path only. A third regression audit found the same widening one path deeper, and sweeping for it turned up a third copy of the rule that was also live.
The
tool_choicecompatibility path manufactured a bare alias without the helper exclusion, so a request declaring a namespacedexecand selecting it with a bare selector still put bareexecinto the declared set, and an undeclaredapply_patch,exec_commandorwrite_stdinwas then rewritten onto it. Selection itself never depended on that map, so the tool is still selected, still forced by a required mode, and still restores under its flattened and dotted names; a helper spelling loses only the bare restore alias, exactly as on the echo path.The passthrough declared catalog built its own list and fenced exactly one name, so a namespaced
exec_command,shell_command,write_stdin,apply_patchorview_imagestill donated its bare spelling to a set that reaches helper normalization. Two of those fail in the opposite direction from the reported one: a bareexec_commandorshell_commandarriving from a namespace switches nested-helper normalization off for a catalog that genuinely declared the shell. Rather than widen a third private list, the rule now lives in one place, derived from the code-mode helper names it is meant to protect.Verification
tool_choiceselector case, which is what the previous fixture could not detect.Checklist
Summary by CodeRabbit
exec,exec_command,shell_command,write_stdin,apply_patch, andview_image.