Normalize a parameter name the way a tool name is normalized - #25
Merged
Merged
Conversation
|
Important Approval pendingCodeRabbit has no unresolved comments, but it has not reviewed the latest commit. Use the checkbox below to review the latest commit. CodeRabbit will approve the changes if it finds no blocking issues.
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 |
There was a problem hiding this comment.
嘿——我发现了 1 个问题
AI 代理提示词
请处理本次代码审查中的评论:
## 单条评论
### 评论 1
<location path="lib/index.js" line_range="399-400" />
<code_context>
- return { lines: [`async def ${spec.name}(`, ' *,', ...fields, `) -> ${spec.returns}:`, ...body], types }
+ const fields = named.map((p) => ` ${p.name}: ${p.type}${p.required ? '' : ' = ...'},${trailing(p.doc)}`)
+ // Named, because nothing else lists them and a required argument the model never sees is one the host rejects it for. `**kwargs` takes no trailing comma, and a lone `*` before it is a SyntaxError — hence the two shapes below.
+ if (rest.length > 0) fields.push(` **kwargs: Any # spell as dict keys: ${rest.map((p) => JSON.stringify(p.name)).join(', ')}`)
+ const open = named.length === 0 ? [`async def ${spec.name}(`] : [`async def ${spec.name}(`, ' *,']
+ return { lines: [...open, ...fields, `) -> ${spec.returns}:`, ...body], types }
</code_context>
<issue_to_address>
**问题 (bug_risk):** 当工具有一个名为 `kwargs` 的有效参数,同时还有另一个参数需要回退到其余参数映射时,渲染器会在同一个函数签名中同时生成 `kwargs: ...` 和 `**kwargs: Any`。Python 会因重复的 `kwargs` 参数而拒绝生成的代码块,导致整个工具绑定代码块无法编译。
**触发条件:** 当架构包含名为 `kwargs` 的参数,同时还包含无法规范化的、无法拼写成有效标识符的参数,例如 `a b` 或 `file-path`。
**建议修复:** 在选择命名参数时保留 `kwargs`,或者为生成的 `**kwargs` 参数使用另一个能够确保唯一的名称。
```suggestion
const named = spec.params.filter((p) => isSpelled(p) && p.name !== 'kwargs')
const rest = spec.params.filter((p) => !isSpelled(p) || p.name === 'kwargs')
```
</issue_to_address>Sourcery 评估
等待批准。 请先处理 1 个发现的问题。
阻塞性发现:lib/index.js:400
请帮助我变得更有用!请在每条评论上点击 👍 或 👎,我会利用反馈来改进审查结果。
Original comment in English
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="lib/index.js" line_range="399-400" />
<code_context>
- return { lines: [`async def ${spec.name}(`, ' *,', ...fields, `) -> ${spec.returns}:`, ...body], types }
+ const fields = named.map((p) => ` ${p.name}: ${p.type}${p.required ? '' : ' = ...'},${trailing(p.doc)}`)
+ // Named, because nothing else lists them and a required argument the model never sees is one the host rejects it for. `**kwargs` takes no trailing comma, and a lone `*` before it is a SyntaxError — hence the two shapes below.
+ if (rest.length > 0) fields.push(` **kwargs: Any # spell as dict keys: ${rest.map((p) => JSON.stringify(p.name)).join(', ')}`)
+ const open = named.length === 0 ? [`async def ${spec.name}(`] : [`async def ${spec.name}(`, ' *,']
+ return { lines: [...open, ...fields, `) -> ${spec.returns}:`, ...body], types }
</code_context>
<issue_to_address>
**issue (bug_risk):** When a tool has a valid parameter named `kwargs` and another parameter that falls back to the rest mapping, the renderer emits both `kwargs: ...` and `**kwargs: Any` in the same function signature. Python rejects the generated block with a duplicate `kwargs` argument, so the entire tool-binding block fails to compile.
**Triggers:** When a schema contains a parameter named `kwargs` alongside an unspellable parameter such as `a b` or `file-path` that cannot be normalized.
**Suggested fix:** Reserve `kwargs` when selecting named parameters, or use a different guaranteed-unique name for the generated `**kwargs` parameter.
```suggestion
const named = spec.params.filter((p) => isSpelled(p) && p.name !== 'kwargs')
const rest = spec.params.filter((p) => !isSpelled(p) || p.name === 'kwargs')
```
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: lib/index.js:400
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
CNSeniorious000
force-pushed
the
param-names
branch
from
August 30, 2026 22:34
ee073b1 to
285f0ad
Compare
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Follows #24, which owns the parameter rendering this builds on.
A hyphen is routine in an MCP name — #19 measured 12 of 208 tool names carrying one, 4.0% of dispatches — and #21 normalized them, one level up. One level down nothing was normalized: a parameter named
file-pathcost the tool its whole signature, every sibling included, replaced byasync def notion_patch(**kwargs: Any)and a pointer tonotion_patch?.Why a parameter is not a tool name
A tool name is ours to spell — the binding is a function we create, and it closes over the raw name. A parameter name travels to the tool as a JSON key, so renaming it needs a way back, or every call silently sends a key the tool does not know. That failure would look like the tool being broken rather than the binding being wrong, which is why the reverse map is the part this PR tests hardest.
dsh-py-codeact/py/kernel.py
Lines 214 to 218 in 285f0ad
Three shapes, three answers
dsh-py-codeact/lib/index.js
Lines 257 to 290 in 285f0ad
Soft keywords were being refused for no reason.
PY_KEYWORDScarries all 40 names, but Python 3.14 has 36 hard keywords and 4 soft ones (_,case,match,type) — anddef f(*, type: str, match: int, case: bool, _: str)compiles. A parameter namedtypeis ordinary, and it was costing its tool the entire signature. They stay refused as tool names, where the block imports them andtypewould shadow the builtin for the rest of the session.And only the leftover is vague now. One unspellable parameter used to erase every other parameter from the signature; it now takes just its own slot in
**kwargs, with the raw key named rather than left to be guessed. That is what the kernel has always done with it — the two halves finally show the same picture.The collision rule, learned the hard way
#21 shipped three collision bugs before it was right, all the same shape: an alias must never take a name something real answers to. A tool declaring both
file-pathandfile_pathkeepsfile_pathmeaningfile_path, and the hyphenated one stays a dict key rather than quietly stealing it.Verification
On the live 33-tool catalogue the rendered block is byte-identical — 43603 before and after, 0 parameters renamed. Nothing there needed normalizing, which is exactly why this was never noticed.
Six assertions added or rewritten (all pass), each mutation-checked by reverting the exact line it guards:
the spelling the block shows maps back to the key the tool declareda fold never displaces the sibling that already owns the namerenders valid Python for parameter names that are not valid Python+ 2 morea parameter name is normalised, and only the rest costs the signatureThe hard-keyword mutation also trips the pre-existing block-compiles guard, which is the honest check: without the suffix,
async def odd(*, from: str)is aSyntaxErrorthat takes the whole fenced block with it.Note
One assertion was replaced rather than fixed:
an unnameable parameter costs the signature, not the toolasserted the old fallback, wherefile-pathcollapsed everything. It now asserts each of the three normalisations and that only the true leftover reaches**kwargs.Gates:
node test/smoke.js,uvx ruff check py/,TY_UV=scripts uvx ty check py/kernel.py— all clean.