fix(devin): restore namespaced tool identities - #4487
Conversation
(cherry picked from commit e7bfb08)
(cherry picked from commit 9ebbcad)
… [skip ci] Carry of #4438 by Yongzhaooo, with the open CodeRabbit finding on src/adapters/openai-chat.ts folded in. A timeline developer message whose only part is non-text (a video part, for example) serializes to an empty string here. The generic Chat path drops such a message through the existing break, but the new chronological exception turned it into { role: "system", content: "" }, which some upstreams reject. Skip it on the non-native path so the OCG route matches the generic path instead of inventing a content-free system message. Native OpenAI developer behavior is unchanged. The finding also asked for video parts to be mapped to a Chat video_url part. That is declined here: the Chat serializer has never emitted video for any destination or role, including ordinary user messages on current dev, so it is a pre-existing gap across every Chat provider rather than something this change introduces, and no upstream in this repository is known to accept that part type. Landing it inside a destination-scoped ordering fix would change every Chat destination on unvalidated wire format. structure/transports/responses.md is at its 600-line budget on dev with no headroom, so its four-line cross-reference is dropped rather than adding the repository's first grace.oversizeDocs entry for a cross-link. The owning description stays in structure/providers/chat-compat.md and the cross-references in runtime.md, transports/inventory.md, providers/cursor.md and data-planes/inbound-compat.md are unchanged. Co-authored-by: Yongzhao <133014490+Yongzhaooo@users.noreply.github.com>
(cherry picked from commit 324008b)
(cherry picked from commit 4eafb3f)
(cherry picked from commit 930c916)
(cherry picked from commit b7d1330)
(cherry picked from commit 6e16274)
(cherry picked from commit d7f27e3)
A caller abort that lands while the native Chat stall clock is waiting on upstream silence must be the only reported outcome. The stream closes as a cancellation, onCancel fires once, upstream is cancelled once, and no upstream_stall_timeout terminal surfaces even after the deadline it was racing has elapsed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit 176cbbd)
… doc [skip ci] Carry of #4389 by olddonkey onto current dev. structure/transports/responses.md is exactly at its 600-line budget on dev, so the two new owning sections this change needs could not be added there at all: bun run structure:check failed at 630 lines. structure/AGENTS.md says an over-budget doc is split along a topic boundary with its own manifest entry, and that a grace.oversizeDocs entry is only for a split already planned — so this takes the split rather than parking a promise nobody would keep. Request-copy and stream-buffer accounting are one topic and now live in structure/transports/byte-accounting.md with its own manifest entry. responses.md is byte-identical to dev again. Both documents sit in structure/transports/, so the seventeen cross-references this change adds keep their relative prefix and only change file name and are otherwise untouched. structure/gui-and-management-api.md was 602 lines for the same reason. Its cross-reference is dropped instead: the dashboard and management API own neither the request-decompression nor the SSE-rewrite path, which makes it the least load-bearing of the seventeen. The other sixteen are unchanged. Co-authored-by: Olddonkey <22208754+olddonkey@users.noreply.github.com>
Map each unique Cognition bare tool name back to the request-declared Codex identity before bridge validation. Refuse ambiguous local-name collisions instead of selecting by declaration order. (cherry picked from commit edc6db3)
Carry of #4457 by jeongjin0, with the unresolved CodeRabbit finding on src/adapters/devin.ts folded in. The return map tracked only advertised local names, but the adapter accepts canonical names on return too, which the existing catalog test pins. With { namespace: "a", name: "x" } and { namespace: "b", name: "a__x" }, a returned a__x is both the first tool's canonical identity and the second tool's advertised name. The map resolved it to b__a__x, so src/bridge.ts dispatched the call through the second tool's identity — a tool the caller may not have named. Register canonical identities as aliases of themselves and mark a conflicting alias ambiguous, so that name now fails before dispatch like any other ambiguous bare name. The unambiguous local name and the unrelated canonical name still resolve, and every existing case is unchanged: a single namespaced tool, a duplicate identical declaration, a bare/namespaced collision, and an undeclared name left for the shared guard. Co-authored-by: Jeongjin Shin <80797980+jeongjin0@users.noreply.github.com>
# Conflicts: # structure/adapters/registry.md
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
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. |
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 74 / 80설명 고치는 위치는 지금 tip( 베이스는 열린 #4485( 경로/심볼 - 베이스가 #4485라 tip에 단독 머지하면 lane C가 꼬입니다. #4485(그리고 그 아래 #4473) 이후 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
3 participants
Summary
Carry of #4457 by @jeongjin0 onto current
dev. This is the lane C tip, stacked on #4485, so its own diff is just this change.Cognition's request mapper advertises the local tool name, so a namespaced Codex tool such as
mcp__cua_repl__jsis sent upstream asjsand comes back asjs. The Responses bridge then validates that bare name against the request-declared tools, does not find it, and fails the turn as an undeclared client tool — the call is lost even though the tool was advertised.Closes #4456.
Review finding folded in
The unresolved finding on
src/adapters/devin.tsis correct and is fixed here. The return map tracked only advertised local names, but the adapter accepts canonical names on return too — an existing test pins that. With{ namespace: "a", name: "x" }and{ namespace: "b", name: "a__x" }, a returneda__xis both the first tool's canonical identity and the second tool's advertised name. The map resolved it tob__a__x, sosrc/bridge.tsdispatched the call through the second tool's identity, which the caller may not have named.Canonical identities are now registered as aliases of themselves, and a conflicting alias is marked ambiguous, so that name fails before dispatch like any other ambiguous bare name. Every existing case is unchanged, which the test matrix pins: a single namespaced tool, a duplicate identical declaration, a bare/namespaced collision, an undeclared name left to the shared guard, and the unambiguous local and unrelated canonical names that still resolve.
Conflict resolved
devlanded #4484 while this lane was preparing, which mergesdevin-cliintodevinand rewrites the samestructure/adapters/registry.mdparagraph this change extends.src/adapters/devin.tsmerged without conflict — #4484 moves provider identity and login, this change touches the tool-name return map, and they do not overlap. The document takesdev's rewritten paragraph, with the tool-name contract re-applied on top of the new wording rather than the old. Confirmed by running the full Devin test set including #4484's owndevin-provider-merge-migration.test.tsand its renameddevin-login.test.ts.Attribution, per
AGENTS.mdandCREDITS.md— the landing commit must carry this trailer, and the squash message must not drop it:Lane C of
devlog/_plan/260913_contributor_carry_train/is #4473 → #4485 → this. Retarget todevonce #4485 lands.Verification
Source head
edc6db3ffa6b0857e38589e44a02ebc9fda82dfecarried onto #4485's head, thenorigin/devmerged up through the stack so this tip sits ondevatf7d9dbad03.bun teston all seven Devin files — 136 pass, 0 fail:devin-adapter,devin-cli-authmode-migration,devin-hardening,devin-login,devin-prompt-cache,devin-provider-merge-migration,devin-stream-deadline. The last two come fromdev's feat(providers): merge devin-cli into devin (import-first login, deprecated alias) #4484 and are included deliberately, because that is the change this tip had to merge.bun run typecheck— clean.bun run structure:check— passes.bun run privacy:scan— passes.The live
/v1/responsescalls todevin/swe-2in #4457 are the author's evidence and were not reproduced here; no live Cognition request was made from this carry.Checklist
privacy:scanpasses. The folded-in finding is itself a dispatch-correctness fix: it stops a tool call from reaching a tool the caller did not name.