Skip to content

Normalize a parameter name the way a tool name is normalized - #25

Merged
CNSeniorious000 merged 2 commits into
mainfrom
param-names
Aug 31, 2026
Merged

CNSeniorious000 merged 2 commits into
mainfrom
param-names

Conversation

@CNSeniorious000

@CNSeniorious000 CNSeniorious000 commented Aug 30, 2026

Copy link
Copy Markdown
Owner

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-path cost the tool its whole signature, every sibling included, replaced by async def notion_patch(**kwargs: Any) and a pointer to notion_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

# A parameter name travels to the tool as a JSON key, so a renamed one has to travel back: the block spells `file_path` and `from_`, the tool still expects `file-path` and `from`. A raw key passed straight through is left alone, which is what a cell written before the rename does.
renames = {p["name"]: p["raw"] for p in spec.get("params") or [] if p.get("raw")}
async def call(**kwargs):
return await bridge.call(name, {renames.get(key, key): value for key, value in kwargs.items()} if renames else kwargs)

Three shapes, three answers

dsh-py-codeact/lib/index.js

Lines 257 to 290 in 285f0ad

/** The 36 names a parameter genuinely cannot take. The other four in {@link PY_KEYWORDS} — `_`, `case`, `match`, `type` — are SOFT keywords: `def f(*, type: str)` compiles, and rejecting them cost a tool its whole signature over a parameter name as ordinary as `type`. They stay refused as TOOL names, where the block imports them and `type` would shadow the builtin for the rest of the session. */
const PY_HARD_KEYWORDS = new Set([...PY_KEYWORDS].filter((word) => !['_', 'case', 'match', 'type'].includes(word)))
/**
* The name a parameter can be CALLED by, or `null` when Python has none to offer.
*
* A parameter name travels to the tool as a JSON key, so unlike a tool name this cannot simply be renamed: the kernel maps the spelling back before dispatch, and `raw` below is what it maps to. Two normalisations, mirroring the two shapes that occur: `-` is legal in an MCP name and in no identifier, and a hard keyword takes the trailing underscore Python programmers already write for it (PEP 8's `class_`).
*/
const paramName = (raw) => {
if (/^[A-Za-z_][A-Za-z0-9_]*$/.test(raw)) return PY_HARD_KEYWORDS.has(raw) ? `${raw}_` : raw
const folded = fold(raw)
return /^[A-Za-z_][A-Za-z0-9_]*$/.test(folded) && !PY_HARD_KEYWORDS.has(folded) ? folded : null
}
/**
* Spell every parameter of one tool, refusing any normalisation that would displace a sibling.
*
* The lesson from folding MCP names one level up: an alias must never take a name that something real answers to. A tool declaring both `file-path` and `file_path` keeps `file_path` meaning `file_path`, and the hyphenated one falls back to `**kwargs` rather than quietly stealing it.
*/
function spellParams(params) {
const taken = new Set(params.map((p) => p.name))
const claimed = new Set()
return params.map((p) => {
const name = paramName(p.name)
if (name === null || name === p.name) return p
// `raw` is the wire key, present only where the two differ — which is also how both halves tell a renamed parameter from one that simply cannot be spelled.
if (taken.has(name) || claimed.has(name)) return p
claimed.add(name)
return { ...p, name, raw: p.name }
})
}
/** Whether the block can put this parameter in a signature: it was renamed, or it needed no renaming. */
const isSpelled = (p) => p.raw !== undefined || paramName(p.name) === p.name

async def odd(
    *,
    file_path: str,          # ← "file-path", folded the way a tool name is
    from_: str = ...,        # ← "from", a HARD keyword: the trailing underscore Python programmers already write
    type: str = ...,         # ← a SOFT keyword, which needed nothing at all
    **kwargs: Any  # spell as dict keys: "a b"
) -> Any:

Soft keywords were being refused for no reason. PY_KEYWORDS carries all 40 names, but Python 3.14 has 36 hard keywords and 4 soft ones (_, case, match, type) — and def f(*, type: str, match: int, case: bool, _: str) compiles. A parameter named type is ordinary, and it was costing its tool the entire signature. They stay refused as tool names, where the block imports them and type would 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-path and file_path keeps file_path meaning file_path, and the hyphenated one stays a dict key rather than quietly stealing it.

async def clash(
    *,
    file_path: int = ...,
    **kwargs: Any  # spell as dict keys: "file-path"
) -> Any:

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:

mutation fails
kernel sends the spelling instead of the raw key the spelling the block shows maps back to the key the tool declared
drop the collision guard a fold never displaces the sibling that already owns the name
stop suffixing hard keywords renders valid Python for parameter names that are not valid Python + 2 more
keep refusing soft keywords a parameter name is normalised, and only the rest costs the signature

The 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 a SyntaxError that takes the whole fenced block with it.

Note

One assertion was replaced rather than fixed: an unnameable parameter costs the signature, not the tool asserted the old fallback, where file-path collapsed 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.

@coderabbitai

coderabbitai Bot commented Aug 30, 2026

Copy link
Copy Markdown

Important

Approval pending

CodeRabbit 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.

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

嘿——我发现了 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


Sourcery 对开源项目免费——如果您喜欢我们的审查,请考虑分享 ✨
请帮助我变得更有用!请在每条评论上点击 👍 或 👎,我会利用反馈来改进审查结果。
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


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread lib/index.js

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sourcery assessment

Approved.

@CNSeniorious000
CNSeniorious000 merged commit e553d3c into main Aug 31, 2026
15 checks passed
@CNSeniorious000
CNSeniorious000 deleted the param-names branch August 31, 2026 00:40
Repository owner deleted a comment from chatgpt-codex-connector Bot Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant