fix(agent): name the callable alias when a bridged tool call misses - #3920
fix(agent): name the callable alias when a bridged tool call misses#3920probepark wants to merge 2 commits into
Conversation
yazzang-homelab
left a comment
There was a problem hiding this comment.
Reviewed as independent architect review (read-only, no push access).
Verified:
parseToolCallNamedegrades safely: a server name containing_simply fails to match and yields no alias, so the worst case is the pre-existing dead end, never a wrong route.- Cross-server refusal and the
customWireNamepath are both covered by red-team cases. Tool <name> not foundprefix stays byte-stable; hint text is appended only.- Head contains current
dev(b621997), all 14 checks green.
Nit (non-blocking): the alias limit of 3 is silent — if more than 3 bridged siblings match, the model sees a truncated set with no marker. Fine for the observed failure shape.
gajae.pr-review-verdict.v1 merge-approved sha256:512c666f0c82cfe264f0908036a10c56296193bc reviewer:architect evidence:https://github.com/Yeachan-Heo/gajae-code/pull/3920/checks
|
이 코멘트를 철회한다. 원문은 틀렸다. 원문에서 "exact-head CI 게이트 때문에 머지 전 현재 확인한 것:
따라서 이 PR은 지금 head 그대로 머지 가능하며, 리베이스도 force-push도 필요 없다. 위쪽 리뷰의 verdict 블록은 그 head sha에 대해 계속 유효하다. 내가 stale한 (참고: 이 오진의 뿌리였던 병합 판정 불일치는 #3929로 따로 정리했다. |
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Batch review — gaebal-gajae review lane
VERDICT: MERGE_READY
gajae.pr-review-verdict.v1 merge-approved sha256:9b631a4a77134022cb624ba938098060349cc1bacbeeed3d389ebd249d923b4c reviewer:architect evidence:31088003291
Reviewed head: 512c666f0c82cfe264f0908036a10c56296193bc (current GitHub head at review time). Note: the briefed head 52706ae87d2a01a2a436217bedb186f613c906e6 had already drifted when the lane fetched; this review targets the true current head. No merge performed by this lane — an independent GJC terminal review is recorded here per repository workflow; merge remains the owner's action.
Maintainer constraint verification (issue #3917 intake)
- Prefix preservation — PASS. The base
Tool <name> not foundstring is byte-unchanged (packages/agent/src/agent-loop.ts:2764); hints append after${base}.only when a live alias or discovery name exists. Red-team tests assert the exact substring (expectBaseNotFound). - Fail-closed aliases / cross-server names — PASS.
findToolCallNameAliasesmatches on the parsed base name and never suggests a different server's tool when the call names a server (parseToolCallNameserver-scope guard); no alias is invented when no active tool shares the base (tests:does not cross servers when suggesting an alias,does not invent an alias when no active tool shares the base name). - No capability/authority bypass — PASS. Tool dispatch is unchanged (exact match on
name/customWireName); the change only enriches the not-found error message. No tool is routed or executed under a different identity, and no permission gate is touched. - Live callable discovery suggestions — PASS. Aliases and the discovery call name are sourced exclusively from the active
toolsregistry;findToolDiscoveryCallNameprefers the exactsearch_tool_bm25and otherwise resolves the bridged alias — it never emits the bare literal when discovery is bridged (tests:points at the bridged discovery call name,prefers the unbridged discovery name when both are callable).
Findings
- CRITICAL: 0 · HIGH: 0 · MEDIUM: 0.
- LOW (advisory, non-blocking):
parseToolCallName's regex assumes the bridge server/instance segments contain no_; a server id containing underscores or a literal non-bridged tool namedmcp__<x>_<y>would misparse. Observed shapes degrade to no-suggestion (fail-closed), never to a wrong routing suggestion.- An unnamespaced call (e.g.
foo) with multiple same-base bridged candidates across servers lists up to 3 aliases joined with "or". This is suggestion-only — nothing is routed on ambiguity.
- Scope complete:
Tool <name> not foundhas exactly one emission site (agent-loop.ts:2764), which this PR covers. CHANGELOG entry under## [Unreleased]/### Fixedfollows repo convention.
Tests / CI
- Local at exact head (isolated worktree
/tmp/review-3920-exact, natives built at 0.12.12):agent-loop-tool-not-found-red-team.test.ts— 14 pass / 0 fail (57 expects);agent-loop.test.ts— 36 pass / 0 fail (162 expects). - CI at current head: 14/14 SUCCESS, 0 FAIL (run 31088003291), including the exact targeted test file
test:packages/agent/test/agent-loop-tool-not-found-red-team.test.tsand thegjc-state-gatesaggregates.
Disposition
- Issue #3917: accepted intake; maintainer-defined scope is exactly the four constraints above; this batch lane is review owner. PR #3920 fully covers the issue; the issue is linked and its disposition is recorded in a separate signed comment.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
MCP bridges expose tools as `mcp__<server>__<instance>_<tool>` and mint the instance segment per session, so a name the model replayed from earlier context resolves to nothing while the tool is active under a sibling name. Dispatch is exact-match, and the not-found error named neither the live alias nor a way to find it -- captured sessions show the same turn burning two tool slots on it. The discovery hint had the mirror-image defect: it was gated on, and hardcoded, the bare `search_tool_bm25` literal, so a bridged discovery tool dropped the hint entirely rather than naming how discovery is actually callable. Lore-id: 8f2b41c9 Constraint: `Tool <name> not found` stays byte-stable -- downstream consumers match on that prefix Rejected: fuzzy/edit-distance matching | silently routes the model at a different tool when it merely typo'd a name Rejected: match on base name across servers | two servers can expose the same tool name, and the wrong server's tool is worse than the dead end Confidence: high Scope-risk: narrow Reversibility: easy Tested: bun test packages/agent/test (689 pass), 7 new red-team cases covering bridged aliases, stale instance segments, customWireName, cross-server refusal, and bridged discovery
512c666 to
376400f
Compare
경고 — 이 PR의 현재 head가 CHANGELOG 전체를 삭제한다머지하면 안 된다. 확인된 사실: 1바이트 — 개행 하나만 남았다. 원인은 내 쪽이다#3932(11:25:32Z 머지)가 그런데 그 결과 리베이스에서 CHANGELOG가 처음으로 진짜 충돌을 내기 시작했고, 그 충돌을 해소하는 과정에서 파일이 비워졌다. 시간대가 명확하다:
전환 비용을 예고하지 못한 건 내 잘못이다. 미안하다. 복구git fetch origin
git checkout origin/dev -- packages/coding-agent/CHANGELOG.md # 해당 패키지 경로로
# 그 다음 ## [Unreleased] 아래에 이 PR의 항목만 다시 추가
git add packages/coding-agent/CHANGELOG.md
git commit --amend --no-edit # 또는 새 커밋앞으로 리베이스에서 CHANGELOG 충돌이 나면 양쪽 항목을 모두 푸시 전에 다음으로 자가 점검할 수 있다: git cat-file -s HEAD:packages/coding-agent/CHANGELOG.md # 30만 바이트 근처여야 정상 |
yazzang-homelab
left a comment
There was a problem hiding this comment.
앞선 내 승인을 철회한다. 이 head는 머지하면 안 된다.
내가 승인한 시점 이후 head가 force-push됐고, 새 head에서 CHANGELOG가 파괴됐다. 승인은 그 이전 커밋에 대한 것이었으므로 현재 head에는 유효하지 않다.
코드 리뷰 내용 자체는 그대로 유효하다 — 로직에 대한 판단은 바뀌지 않았다. 되돌리는 것은 머지 가능 판정뿐이다.
원인은 내가 머지한 #3932다(union 드라이버 제거). 상세와 복구 명령은 이 PR에 이미 남긴 코멘트와 #3942 에 있다. CHANGELOG를 origin/dev에서 복원하고 이 PR의 항목만 다시 넣은 뒤 푸시하면, 그 head에 대해 즉시 재승인하겠다.
#3941(CI 가드)이 머지되면 이 상태는 CI에서 자동으로 걸린다.
|
@Yeachan-Heo 이 PR은 APPROVED 상태인데 현재 head에서 CHANGELOG가 파괴돼 있다. 지금 머지 버튼을 누르면 릴리스 이력이 날아간다.
$ git cat-file -s 376400f80:packages/agent/CHANGELOG.md
1GitHub의 원인은 내가 머지한 #3932다. #3942 에 전말을 정리했고, #3941 로 CI 가드를 올렸다. 12건이 같은 상태이며 그중 6건이 APPROVED 리뷰를 달고 있다. 복구: git checkout origin/dev -- packages/agent/CHANGELOG.md |
Removing `packages/*/CHANGELOG.md merge=union` in #3932 was correct -- union never conflicts, it concatenates both sides of an overlapping hunk, which silently filed entries into versions that had already shipped (35 such entries audited on dev, #3929). What it did not account for is the transition: these files now conflict on rebase for the first time, and a bad resolution drops the whole history with no marker. That is not hypothetical. #3932 merged at 11:25:32Z. Between 11:29:29Z and 11:35:02Z, ten open pull requests across six authors force-pushed heads whose CHANGELOG was a single newline -- every released section gone. #3920 #3697 #3870 #3908 #3887 #3864 #3729 #3869 #3866 #3873. Nothing caught it: the files still parse, no test reads them, and the loss looks like a large deletion inside an otherwise legitimate diff. The guard asserts the one property that matters and nothing more: every `## [X.Y.Z]` heading present at the merge base must still be present at the head. Additions pass, rewording passes, and a release commit that consumes `## [Unreleased]` into a new version passes. Only losing a released section fails, and the message names the recovery command. Runs in `affected-plan`, which already checks out full history and carries the immutable event base sha, so it costs one bun invocation and needs no new job. Constraint: a release bump must still be able to add a version heading Constraint: must not depend on byte-size heuristics -- a legitimately small changelog is not a violation Rejected: threshold on deleted line count | fires on large legitimate edits and misses a small changelog emptied completely Rejected: restore merge=union | reinstates the silent misfiling this replaced, and GitHub ignores the driver anyway Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test scripts/changelog-history-guard.test.ts (11 pass); guard run against the three real broken heads (#3873 #3920 #3869) exits 1 and names the lost sections; clean range exits 0; bun run check:tools exit 0 Not-tested: a real release-bump PR end to end
Removing `packages/*/CHANGELOG.md merge=union` in #3932 was correct -- union never conflicts, it concatenates both sides of an overlapping hunk, which silently filed entries into versions that had already shipped (35 such entries audited on dev, #3929). What it did not account for is the transition: these files now conflict on rebase for the first time, and a bad resolution drops the whole history with no marker. That is not hypothetical. #3932 merged at 11:25:32Z. Between 11:29:29Z and 11:35:02Z, ten open pull requests across six authors force-pushed heads whose CHANGELOG was a single newline -- every released section gone. #3920 #3697 #3870 #3908 #3887 #3864 #3729 #3869 #3866 #3873. Nothing caught it: the files still parse, no test reads them, and the loss looks like a large deletion inside an otherwise legitimate diff. The guard asserts the one property that matters and nothing more: every `## [X.Y.Z]` heading present at the merge base must still be present at the head. Additions pass, rewording passes, and a release commit that consumes `## [Unreleased]` into a new version passes. Only losing a released section fails, and the message names the recovery command. Runs in `affected-plan`, which already checks out full history and carries the immutable event base sha, so it costs one bun invocation and needs no new job. Constraint: a release bump must still be able to add a version heading Constraint: must not depend on byte-size heuristics -- a legitimately small changelog is not a violation Rejected: threshold on deleted line count | fires on large legitimate edits and misses a small changelog emptied completely Rejected: restore merge=union | reinstates the silent misfiling this replaced, and GitHub ignores the driver anyway Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test scripts/changelog-history-guard.test.ts (11 pass); guard run against the three real broken heads (#3873 #3920 #3869) exits 1 and names the lost sections; clean range exits 0; bun run check:tools exit 0 Not-tested: a real release-bump PR end to end
Fixes #3917.
Problem
MCP bridges expose tools as
mcp__<server>_<tool>, and proxied bridges add a per-session instance segment:mcp__<server>__<instance>_<tool>. Dispatch is exact-match onname/customWireName, so a name the model replayed from earlier context resolves to nothing even when the tool is active under a sibling name.Tool <name> not foundnamed neither the live alias nor a way to find it.Captured five times across three sessions today (
019fd580,019fd583,019fd595), including two calls burned in a single turn:mcp__jzi2uzmxd57z__wbg7pcrl46bd_searchsearchmcp__jzi2uzmxd57z__2dl71hzhrahf_findfindmcp__jzi2uzmxd57z__jgspauo3hmi5_subagentmcp__jzi2uzmxd57z__gbbgnmhc3qkt_subagentThe recovery hint had the mirror-image defect:
hasToolDiscoverywas gated on a tool callable as exactlysearch_tool_bm25, and the hint text hardcoded that literal. A bridged discovery tool therefore dropped the hint entirely — and emitting it unconditionally would have been worse, naming a second non-callable tool.Change
packages/agent/src/agent-loop.tsparseToolCallNamesplits the bridge namespace (mcp__<server>__<instance>_) off a call name.findToolCallNameAliasesnames active tools that denote the same tool. A namespaced call is matched only against its own server or against an unnamespaced tool — two servers can expose the same tool name, and routing the model at the wrong server is worse than the dead end.findToolDiscoveryCallNameresolves how discovery is actually callable, preferring the unbridged name.toolMatchesCallNameis replaced bytoolCallNames, which the new helpers share; it had no other caller.Tool <name> not foundremains the byte-stable prefix.Verification
bun test packages/agent/test— 689 pass, 0 fail. Seven new cases inagent-loop-tool-not-found-red-team.test.tscover the bridged alias, a stale instance segment,customWireNamereachability, cross-server refusal, absence of a false suggestion, the bridged discovery hint, and the unbridged-discovery preference.bun --cwd=packages/agent run checkclean.Note
devis currently red independently of this PR:packages/coding-agent/test/agent-session-abort-timeout.test.tsfails since 4e33420 (#3894). Tracked and fixed separately.