Repository navigation
feat(protocols): protocol paths as a first-class concern — PF-01..PF-12 - #5820
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (188)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. |
![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: 6f3c0290db
ℹ️ 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".
| const exact = settled.provider && settled.model | ||
| ? plan.candidates.find(candidate => candidate.provider === settled.provider && candidate.model === settled.model) | ||
| : undefined; | ||
| return exact ?? plan.candidates.find(candidate => candidate.eligible); |
There was a problem hiding this comment.
When shadowPlan is enabled and a combo selects or fails over to a non-first candidate with a different protocol path, this fallback compares the observed trace against the wrong candidate. The combo runtime deliberately replaces the final context with provider: "combo" and the requested selector in src/server/responses/core-combo.ts:676-680, so exact cannot match the plan's physical provider/model candidates and the first eligible candidate is always used, producing a false planMismatch. Derive the settled candidate from the final attempt's provider/model or otherwise preserve the answering child's identity.
Useful? React with 👍 / 👎.
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 64 / 80이 PR은 "미리 계산한 경로"와 "요청이 실제로 간 경로"가 같은지 확인하는 기능을 넣는다. 스위치 이름은 명령도 세 개가 늘었다. src/protocols/trace.ts 메인테이너의 판단이 필요한 지점 기준 브랜치가 섀도 비교는 Chat이랑 Messages만 한다. Responses 요청은 일부러 빼 두었고, PR 본문이랑 가이드에도 그렇게 적혀 있다. Responses가 실제 트래픽의 대부분이면, 스위치를 켜도 그 요청은 검사되지 않는다. 본문은 타입 검사와 구조 검사가 통과했다고 적고, 전체 테스트 결과는 나중에 붙인다고 해 두었다. 너의 추천 콤보가 끝난 뒤에도 실제로 답한 자식의 provider와 model이 로그에 남게 하고, 이 댓글은 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:
|
Records the stacked work packets (PF-01..PF-12), the invariants every packet keeps, and the per-packet design so Chat Completions and Messages can reach the shared execution policy without the public Responses wire as a mandatory detour.
One leaf module names protocols, upstream wires, path hops, delivery modes and a closed reason-code list, and maps the older spellings (InboundWire "anthropic", adapter ids, Lab identities) explicitly instead of renaming them in place.
Which request features survive each cross-wire hop, in the compatibility-manifest vocabulary, so a path's losses (Chat n and logprobs through Responses, Messages top_k) are computed from the path instead of discovered by users.
Current and target paths for 3 ingresses x 3 upstreams x stream, so every later packet changes a named cell on purpose rather than drifting the contract.
ProtocolPlanV1 and ProtocolTraceV1 with bounded validators shared by the server and the dashboard, fixed before the parallel GUI and runtime packets depend on them.
The dashboard imports these modules directly; the boundary test fails on any import that would drag server, router or Lab code into the GUI.
apiSurfaces stays raw in the schema so a mistyped enabled can never degrade to "inherit" and reopen a surface; protocols degrades to absence because every default is the conservative one. No request path reads either key yet.
One reader for apiSurfaces/protocols: a malformed Messages value closes the surface, an absent one inherits claudeCode.enabled, and every rollout switch defaults off.
The new area needs an owner doc; inbound-compat and config link to it instead of restating the vocabulary.
…rivacy scan A literal bearer header and a userinfo URL read as secrets to privacy:scan; constructing them from parts keeps the same assertions without tripping the scanner.
…race A pure leaf decides disagreement on mode, upstream and request path for the settled candidate; finalize sets planMismatch only when an input was recorded, never on throw.
…s entry marks Behind protocols.rollout.shadowPlan (off): the preview snapshot with basis dispatch is kept beside the marks; nothing is read, sent or stored with the switch off.
…w safety Covers settled-candidate selection, the uncompared response path and compatibility rejects, and that old trace rows without planMismatch stay valid.
Thin clients of the protocol management routes; policy reads when bare and writes only when a setting flag is given, leaving value validation to the server.
…-verb exemptions The mutation-consistency check now reads the registry's mutates flag, so the read-only plan POST is not forced to claim a write; the skill surface is regenerated.
…rity Each verb's method, path and body, --json passthrough, exit 2 before any send on bad argv, and that every protocol route is verbed rather than exempt.
Delivery modes, preview, trace, the unrepresentable policy, every rollout switch off by default, the shadow plan, and what still travels the internal Responses bridge.
…ults Operators editing config.json need the unrepresentable policy and each rollout switch, and that a malformed block falls back to the defaults.
Routes, which invocation writes, server-side validation of switch names, and the management reference now points at the CLI verbs and the guide.
…ated inventory Each scenario states whether it is implemented, behind which switch, which tests name it and that none has live evidence; the inventory is rechecked against this branch.
…verbs protocol-paths owns where the input is recorded, what is compared and what is not, and the CLI client; the management doc drops the deferred-verb note.
…0 ships them The guide and configuration reference were written before native Messages over OAuth and the anthropic-beta allowlist landed below this change.
The verbs exist now, so the headless parity map names them instead of the owed-verb placeholder.
The shared runtime request helper sends its JSON content type on every call, so the GET case should not pin a missing header.
A long path such as Responses → Messages · Translated overflowed the narrow model column and was cut off, hiding the mode. The badge now wraps inside the cell.
e076aab to
02832f8
Compare
lidge-jun
changed the title
⏳ DRAFT
What to do
Automatic draft conversion failed (token cannot change draft status). Please convert this pull request to a draft manually. The required |
…nking The Responses passthrough already drops replayed opaque reasoning when the serving identity changed or the blob was rejected; the Anthropic adapter now does the same for signed thinking and redacted_thinking blocks.
02832f8 to
3df0d8f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3df0d8f4d2
ℹ️ 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".
| const persist = ctx.deps.saveConfigPreservingClaudeCode | ||
| ?? (await import("../../config")).saveConfigPreservingClaudeCode; |
There was a problem hiding this comment.
When production leaves the persistence seam unset, this await import() runs after applyProtocolSettingsPatch has already mutated the shared live config. A concurrent management request can therefore modify and save the same object during this suspension; the eventual save may unintentionally include that request's changes, and a failed save restores the old snapshot over the concurrent in-memory update. Resolve the persister before taking the snapshot and applying the patch, or perform mutation and persistence under the same serialized config transaction.
Useful? React with 👍 / 👎.
| const nativeBody = (target: ComboTarget, route: RouteResult | undefined): Rec | undefined => { | ||
| if (!route || route.provider.adapter !== "openai-chat") return undefined; | ||
| const body = envelope.freshBody(); |
There was a problem hiding this comment.
With nativeChatCombos enabled, judge() invokes this helper for every openai-chat candidate only to retain a boolean verdict, but freshBody() charges the entire clone as retained request_copies and provides no release handle. Those discarded eligibility clones remain charged for the request, and the selected candidate is cloned again during dispatch, so a sufficiently large request with several candidates can hit the 32 MiB translator limit and return a local 413 before sending anything even though only one candidate body is needed. Cache the eligible body for later dispatch or inspect eligibility without creating a charged clone.
Useful? React with 👍 / 👎.
1 participant
Summary
Protocol-first-class unit (
devlog/_plan/260924_protocol_first_class/), landed as one squash of the reviewed stack #5808 → #5809 → #5810 → #5811 → #5812 → #5813 → #5814 → #5815 → #5816 → #5817 → #5819 → this PR, rebased onto the currentdev. Chat Completions and Anthropic Messages reach the shared execution owner without the public Responses wire as a mandatory detour; every new lane is behind aprotocols.rollout.*switch that defaults off.apiSurfaces/protocolsconfig (feat(protocols): shared protocol contract, feature dispositions and baseline (PF-01) #5808).GET /api/protocols,POST /api/protocols/plan, API page request-path preview (feat(protocols): preview the request path without sending (PF-03) #5810).PATCH /api/protocols/settings, API cards (feat(protocols): separate Messages API exposure from the Claude toggle (PF-04) #5812).directEncoders(feat(protocols): encode Chat and Messages responses directly from adapter events (PF-09) #5814).nativeChatCombos(feat(chat): send eligible Chat combo candidates on the native lane (PF-07) #5815).managedMessagesNative, bridge-only policy declines (feat(claude): send eligible managed-key Messages natively (PF-08) #5816).anthropic-betaallowlist, unpooled OAuth native Messages, opaque-state guard on the native lane (feat(messages): OAuth, beta allowlist and opaque-state guard on the native lane (PF-10) #5819).ocx api protocols|explain|policy, Protocol paths guide and references.tests/adapters/anthropic/anthropic-opaque-strip.test.ts).Verification
3df0d8f4d28e(rebased onto currentdev):bun x tsc --noEmitexit 0; guibun x tsc -bexit 0;bun run structure:check,bun run skill:surface:check,bun run privacy:scanpass; file-size ratchet, core–Lab boundary, repo hygiene, skill-ocx and headless CLI parity tests pass locally (137 + 9 pass, 0 fail).dev: every stacked PR's required CI passed on its exact head; full localbun run teston the stack head showed only failures that reproduce identically on a checkout without this stack (Lab CL-03/07/08, SEC-02,release helpertimeouts);cd gui && bun test --isolate tests2398 pass, 0 fail.devis recorded in a comment on this PR.Maintainer integration
Integrated into
devby a maintainer under the MAINTAINERS.md maintainer-integration exception, as a single squash, at the maintainer's explicit direction and without waiting for CI on this rebased head. Exact-head evidence above is local; CI on this head runs after merge and is checked in the post-merge comment.Screenshots
See #5809, #5810, #5812 and #5817 (captured from the stack head in an isolated home).
Checklist