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. |
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. |
✅ READY
Hygiene✅ 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: d0077bc541
ℹ️ 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 two PATCH /api/protocols/settings requests finish parsing concurrently, this await yields after the first request has already mutated the shared config but before its locked save. The second request can then mutate the same object, causing the first save to serialize the second request's state and return 200 even though its own change—potentially closing Messages—was lost; if that save fails, restoring the first snapshot can also erase the second request's successful mutation. Resolve/import the persistence function before taking the snapshot and applying the patch so the live mutation and synchronous save remain one uninterrupted operation.
Useful? React with 👍 / 👎.
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Reviewed PF-04 layer at exact head d0077bc541fa0a86f7133240323f44e8724f3e7a against feat/pf05-inference-primitives. The focused bounded suites pass 41/41, but the settings writer has a concurrency blocker.
patchProtocolSettings() snapshots and mutates the shared live config, then awaits the fallback dynamic import that resolves saveConfigPreservingClaudeCode. Two concurrent PATCH requests can interleave during that await: request B mutates the same object before request A saves, so A can persist/return B's state; A's rollback on save failure can also erase B's successful mutation. Resolve the persistence function before snapshot/apply (or move the whole read-modify-save into the existing serialized mutation primitive) so mutation through synchronous locked persistence is uninterrupted. Add a concurrent PATCH regression covering both success and rollback interleavings.
Separately, the PR is still draft-gated for the required UI screenshot and its stack CI inherits failures from lower PF layers; approval must wait for those too.
리뷰 · 우선순위 68 / 80이 PR은 Messages API를 Claude 켜기 스위치에서 떼어 낸다. 대시보드 API 페이지는 Responses, Chat Completions, Messages를 카드로 보여 준다. Messages 카드는 꺼져 있어도 남고, 토글이 있다. 끄면 Claude 설정 화면과 네이티브 토글이 베이스는 #5811의 라인 - 라인 - 라인 -
메인테이너의 판단이 필요한 지점
손으로
너의 추천 방향은 유지해라. 닫을 중복 PR은 없다. 베이스를 이 댓글은 grok-bot이 작성했습니다 |
/v1/messages and count_tokens now share the one surface reader, so an explicit apiSurfaces.messages value wins, a malformed one closes both, and absence still inherits claudeCode.enabled.
… metadata
The keys payload gains surfaces.{responses,chat,messages} from the shared resolver.
claudeCodeEnabled stays for older dashboards and now mirrors the resolved Messages
state, so they never advertise a closed endpoint.
…ping writer PUT /api/claude-code and the native Claude toggle each re-stated the auth-mode migration stamp. The protocol settings route writes the same block next, so the rule lives in commitClaudeCodeBlock rather than in a third copy.
Strict parsing refuses unknown keys and wrong types. Closing Messages also writes claudeCode.enabled=false so an older binary after rollback stays closed; opening writes only the explicit surface value. A snapshot lets the route undo a failed save.
The Messages toggle and protocol switches persist through the locked saveConfigPreservingClaudeCode, undo the in-memory change when the save fails, and answer with the fresh GET /api/protocols shape. Declared as a PF-12 deferred verb.
…adata Strict body, both keys written on close, only the surface on open, rollback on a failed save, 409 on lock contention, and surfaces in the API access metadata.
Absent inherits, dashboard-written false is closed on old and new binaries, explicit true with Claude off is open only on the new one, invalid values close, and count_tokens agrees with /v1/messages in every row.
parseApiSurfaces reads surfaces from the keys payload and answers undefined for an older server, so the page can fall back instead of guessing. patchProtocolSettings validates the fresh GET shape the server returns.
State, source and Messages toggle strings for the three API cards, including the note that closing Messages also turns the Claude integration off.
Each card states whether the API is served, its endpoint, and who decided it. The Messages card keeps showing while closed, carries the toggle, and links to the Claude page. Without surfaces from the server the flat endpoint list stays.
The keys payload's surfaces are validated on read and in the session cache, and a successful toggle reloads the payload from the same apiBase rather than guessing.
Three cards with state and source, a closed Messages card that stays visible, the older-server fallback, and a toggle that PATCHes the target apiBase then reloads.
…r and API cards Protocol paths now names the ingress reader, the PATCH writer and its rollback, and why closing Messages also writes claudeCode.enabled; the dashboard doc covers the three API cards and the older-server fallback.
The server config reference explains inheritance, fail-closed values and the downgrade behavior of the dashboard toggle; the management API table gains the settings route and its errors.
243fd58 to
70c2c84
Compare
d0077bc to
03189b6
Compare
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-04 of the protocol-first-class unit (
devlog/_plan/260924_protocol_first_class/030_gui_and_management_api.md#pf-04-api-surface-settings). Stacked on #5811 (PF-05).Messages API exposure becomes its own setting instead of riding on the Claude integration toggle, without ever reopening a surface an operator closed.
resolveApiSurfaceSettingsis the only reader of Messages exposure:/v1/messagesand/v1/messages/count_tokensshare one gate, so they cannot disagree. ExplicitapiSurfaces.messages.enabledwins; a malformed value closes the surface; absence inheritsclaudeCode.enabled(existing installs unchanged).surfaces: { responses, chat, messages }withenabledandsource.claudeCodeEnabledis kept for older dashboards and now mirrors the resolved Messages surface, so an old dashboard never shows a closed endpoint as open.PATCH /api/protocols/settings(messagesEnabled,unrepresentable,rollout), strictly validated, saved through the locked config path. Disabling Messages writesapiSurfaces.messages.enabled = falseandclaudeCode.enabled = falsein one save, so an older binary after rollback stays closed; enabling writes onlyapiSurfaces.messages.enabled = true. A failed save restores the live config (409 on lock contention, 500 otherwise) without echoing error text.claudeCodeblock inline in two places; that moves into one writer (src/claude/claude-code-block.ts) used by both and by the PATCH, keeping hand-edit protection unchanged.apiSurfacesin the server configuration reference, the PATCH route in the management API reference, structure owner docs.Behavior to note for review: with
apiSurfaces.messages.enabled: trueset explicitly, turning Claude off on the Claude page no longer closes/v1/messageson this binary — the explicit surface setting wins, and the card shows the source. A hand-writtenapiSurfaces.messages.enabled: falsewithclaudeCode.enabled: trueis not rollback-safe; only dashboard-written states are.Verification
bun x tsc --noEmit: exit 0. New root test files additionally typechecked with a temporary tsconfig: exit 0.gui:bun x tsc -b,bun run lint,bun run lint:i18n: exit 0.bun run structure:check: passed.Tests (
tests/server/protocol-settings-route.test.ts,tests/claude-integration/messages-surface-matrix.test.tsrecording the upgrade/rollback matrix,gui/tests/api-surface-cards.test.tsx, extendedtests/server/api-access-endpoints.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.
Screenshots
Captured from the stack head in an isolated home (fake providers, no real credentials).
API page, Responses / Chat Completions / Messages cards
Checklist