Repository navigation
Conversation
|
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: Repository: lidge-jun/opencodex/.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 |
|
✅ Deterministic PR hygiene checks passed. |
![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: a2af3afb90
ℹ️ 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 (names.length === 0) return false; | ||
| const callIds = blockedSkillCallIds(body.messages, names); | ||
| for (const msg of body.messages) { | ||
| if (!isRec(msg) || msg.role !== "user" || !Array.isArray(msg.content)) continue; |
There was a problem hiding this comment.
When a user message carries its content as a string rather than a block array, this condition skips it even though userMessageToItems applies maybeElideSkillText to that valid string form. With managedMessagesNative enabled, a large blocked-skill payload beginning with the skill marker is therefore judged eligible and forwarded intact, bypassing claudeCode.blockedSkills; check string content with the same elision predicate before continuing.
Useful? React with 👍 / 👎.
| const relayed = tapAnthropicSseForLog(source, logCtx, (status, meta) => { | ||
| try { | ||
| cleanupAbort(); | ||
| bindUsage(logCtx.usage); | ||
| finishLog(status, undefined, meta.closeReason); | ||
| if (meta.closeReason !== "terminal") upstream.abort(); |
There was a problem hiding this comment.
When Anthropic accepts the request with HTTP 200 and later terminates the SSE stream with event: error, tapAnthropicSseForLog does not recognize that frame and invokes this callback with status 200 at EOF. The managed native request and its active attempt are consequently recorded as successful even though the caller received an error; use an error-aware stream tap or extend the existing tap to propagate the terminal error status.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
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. |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Reviewed exact head a2af3afb90241c00d7c89df4b6478125e7607189. Blocking SSE framing bug: echoRequestedModel() recognizes frames only by two adjacent LF bytes. Valid SSE may use CRLF line endings and therefore separates events with \r\n\r\n; those bytes never match frameEnd, so the scanner eventually relays the stream unchanged and the client sees the provider wire model instead of its requested selector. The stated model-echo contract must hold for both legal line-ending forms. Please parse framing with the existing SSE decoder or support LF and CRLF across chunk boundaries, and add a CRLF/chunk-split regression. The five focused builder, eligibility, bridge-policy, native integration, and decline-trace suites passed 28/28 under CPUQuota=200%, MemoryMax=4G, MemorySwapMax=0, TasksMax=128. Exact-head CI still fails in test shards and GUI gates, and lower PF layers #5814/#5815 remain blocked.
…ders The adapter's endpoint, pinned anthropic-version and key placement move into exported helpers so a second Messages sender can reuse them instead of restating them.
…rce body Allowlisted source fields, the wire model and the provider's own key; no caller header is read, so the managed lane cannot inherit caller-forward authority.
One pure rule for the ingress, count_tokens and the planner: switch, anthropic adapter, key auth, no combo or policy, no synthetic effort or fast row, no vision preprocessing.
Modelled on native Chat: shared attempt, final log, spend tracker, proactive key pick, 401/429 key failover and replay; SSE relays through the existing Anthropic log tap.
Decided after route settlement and the managed-client steps; the caller-forward branch keeps its own earlier decision. The reject guard judges the native path when it applies.
With the switch on and an eligible route, count_tokens estimates the allowlisted body the native lane would send; otherwise the existing estimate is unchanged.
With the switch on, a Messages candidate is judged by the same decline rule the ingress uses; with it off the preview is unchanged.
The builder keeps only allowlisted fields and the adapter's headers; each decline reason is named; the planner reports native only with the switch on and a managed key.
Allowlisted body and provider key on the wire, 401/429 key failover, stream relay and usage, JSON callers, switch-off bridge, caller-forward precedence and count_tokens.
Protocol paths owns the rule, builder and authority split; the Responses transport doc lists the lane among the native senders; the devlog inventory records what stays bridged.
A native lane that would skip a pinned effort, skill elision or a sidecar declines with bridge-only-policy. New reason code, so the contract version moves to 2026-09-25.1.
A pure predicate over the same inputs the translator uses, so a caller can keep a request on the path that applies claudeCode.blockedSkills.
…lies A pinned route effort, a blocked-skill bundle the translator would elide, or a web-search tool the sidecar could serve now declines the native lane with bridge-only-policy.
…aude settings The ingress, count_tokens and the key-reselection recheck pass the translated model id and the ingress's Claude view, so the effort pin and blocked skills read what the bridge reads.
With the switch on, the bridge entry mark carries the decline reason, so the trace explains the bridge; features are scanned once so both marks report the caller's own settings.
The planner passes the resolved selector, so a pinned route effort reports bridge-only-policy; body-dependent policy (skills, web search) is not predictable from features.
The translated lane answers with the client's selector; the native lane now rewrites message_start (stream) or the message (JSON) the same way instead of the wire id.
JSON and stream answers now carry the client's selector, matching the translated lane.
Pinned effort, blocked-skill elision and the web-search sidecar decline the native lane; the planner reports the pin; a declined route's bridge trace names the reason only when on.
…odel echo States which operator policy keeps Messages on the bridge, that stabilizePromptCache is a recorded gap rather than a decline rule, and that native answers echo the client's selector.
031af07 to
6dc5f8c
Compare
a2af3af to
53e4783
Compare
리뷰 · 우선순위 62 / 80이 PR은 스위치 거절 이유는 한곳으로 모았습니다. 스위치가 꺼짐, Anthropic이 아님, 키가 아님, 콤보나 정책 경로, effort 줄, fast 줄, 이미지를 미리 가공해야 함, 다리에서만 하는 운영 규칙입니다. 운영 규칙은 고정된 effort, 막힌 스킬 문서를 빼는 경우, 웹 검색 도구가 붙을 수 있는 경우입니다. 거절되면 다리로 가고, 스위치가 켜져 있으면 그 이유가 흔적에 남습니다. 답의 모델 칸에는 손님이 보낸 이름을 다시 적습니다. 베이스 브랜치는
메인테이너의 판단이 필요한 지점 모델 이름 다시 쓰기를 LF만 보장할지, CRLF도 고칠지입니다. 이 레포의 기존 Anthropic 로그 탭도 작성자는 이 헤드에서 새 테스트를 돌리지 않았고, CI가 첫 실행이라고 적었습니다. 보내는 파일 이 PR의 베이스는 #5815 브랜치입니다. 그 PR이 바뀌거나 닫히면 이 diff도 같이 흔들립니다. 너의 추천 스위치 기본값은 꺼져 있습니다. 머지 전에 이 댓글은 grok-bot이 작성했습니다 |
devin-ai-integration
Bot
added
the
priority: P3
|
Maintainer triage: Criteria (P3): Low: new provider/client integration, large or experimental feature (>2000 LOC or >50 files), RFC/roadmap, or long-stale branch. Related / overlapping PRs:
|
2 participants
Summary
PF-08 of the protocol-first-class unit (
devlog/_plan/260924_protocol_first_class/020_engine_and_codecs.md#pf-08-managed-native-messages). Stacked on #5815 (PF-07).Behind
protocols.rollout.managedMessagesNative(default off): a Messages request whose settled route is a direct, key-authanthropicprovider is sent to/v1/messagesnatively with the proxy-managed key, instead of replaying through the internal Responses body.src/adapters/anthropic.ts: URL resolution,anthropic-versionpin and key-auth header code move unchanged into exported helpers so the native builder uses exactly what the adapter uses.src/adapters/anthropic/passthrough.ts:buildAnthropicMessagesPassthroughRequest— provider key only (reads no caller header; refuses OAuth and forward auth), operatorprovider.headersas the adapter applies them, and the source body with the wire model and a field allowlist.src/server/messages-native-eligibility.ts:nativeMessagesDeclineReasonnames the first rule that keeps a route off the lane (rollout-disabled,cross-wire-ir,auth-mode-not-native,combo-or-policy-route,effort-row/fast-row,vision-preprocessing, and newbridge-only-policy). The ingress,count_tokensand the planner all ask it.bridge-only-policy(new reason code;PROTOCOL_CONTRACT_VERSION→2026-09-25.1). With the switch on, a declined route records its reason on the bridge trace.src/server/messages-native.ts:handleNativeMessageson the PF-05 primitives — one attempt, finish-once final row, spend tracker per physical send, 401/429 key rotation rebuilding the request from the builder each time, same-target 429 replay, SSE relay with the Anthropic log tap, JSON for non-stream callers. The responsemodelechoes the selector the client sent, as the bridge does.count_tokensestimates over the body the native lane would send when the route is eligible.Recorded gaps (devlog 040):
stabilizePromptCacheis a Claude-app cache optimization and does not run on the native lane; calleranthropic-betais not forwarded (allowlist is PF-10).Verification
bun x tsc --noEmit: exit 0 on this head.bun run structure:check,bun run privacy:scan: passed.Tests (
tests/adapters/anthropic/anthropic-messages-passthrough.test.ts,tests/responses/messages-native-eligibility.test.ts,tests/responses/messages-native-bridge-policy.test.ts,tests/claude-integration/messages-native.test.ts,tests/claude-integration/messages-native-decline-trace.test.ts) were written and registered but run in the full local suite below.Full local run on the stack head (feat(protocols): protocol paths as a first-class concern — PF-01..PF-12 #5820, which contains this change):
bun run test— the only failures are Lab CL-03/CL-07/CL-08/SEC-02 andrelease helpertimeouts, which fail identically on a checkout without this stack (local environment), plus service/toggle cases that pass when run alone;cd gui && bun test --isolate tests— 2398 pass, 0 fail.CI on this head: all required checks pass.
Checklist