feat(remote): phase 1 — runtime roles, /readyz protocol negotiation, data-plane /v1/catalog - #2772
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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team 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: c10ef21a92
ℹ️ 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".
| pid: process.pid, | ||
| port: boundPort ?? listenPort, | ||
| status, | ||
| ...readyProtocolMetadata(config, req), |
There was a problem hiding this comment.
Update the readiness documentation alongside this response change: docs-site/src/content/docs/reference/cli/lifecycle.md:158-160 still describes the sanitized identity as exactly {service, version, uptime, pid, port, status}, and the translated lifecycle pages repeat that obsolete shape. Operators and API consumers therefore cannot discover the new protocol-negotiation fields from the public docs; document protocol, minimumClientProtocol, and managementUrl in the English source and synchronize the locales.
AGENTS.md reference: src/AGENTS.md:L28-L28
Useful? React with 👍 / 👎.
리뷰 · 우선순위 67 / 80이 PR은 원격 허브 스택의 1단계 코드입니다. 베이스는 #2771 브랜치( 역할 키 자체는 기존 config 패턴을 따릅니다. 잘못된 손수정은 진짜 동작 변화는 역할 키보다 라인 src/remote/protocol.ts readyProtocolMetadata - origin 파싱 실패 때 throw합니다. 라인 src/server/index.ts GET /v1/catalog - 라인 src/server/catalog-download.ts serializePersistedCatalog - 요청 경로에서 라인 src/server/index.ts loopbackRouteAllowed /v1/catalog - 주석이 "remote catalog client needs"로 바뀌었습니다. 루프백 allowlist는 원래 직접 띄운 라인 src/config.ts runtimeRole - 잘못된 값은 경고 후 standalone. 다른 degraded 키와 같은 패턴입니다. Phase 1에서는 이 값으로 경로 src/server/auth-cors.ts AUTH_MATRIX - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes on exact head c10ef21a92d20b38868ce4329f3d0f884443bd83. This phase is now 203 commits behind current dev@ae356a3cf, and its hosted matrix is not green: the privacy scan fails and tests/release-version-line.test.ts:108 fails on both Linux shard 3 and macOS. Because this phase touches shared runtime role, readiness, catalog, and server surfaces, reviewing or merging the old tree would not establish behavior on the current integration line. Please rebuild/rebase the phase on current dev, resolve the privacy and release-line failures without weakening either gate, and request review again only after exact-head CI is green. The remote-runtime direction is not being rejected; this review state records that the current head is stale and failing.
bad1624 to
dfae1da
Compare
c10ef21 to
07d7f10
Compare
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Re-reviewing exact head 07d7f1006a15cacaa99fb7834d28b73efa520eba against its current stack base 36992baa9.
The old stale-tree problem is resolved and the implementation correctly removed the data-plane catalog ETag/304 path: /v1/catalog now returns Cache-Control: no-store, never emits a validator, and ignores If-None-Match. The configured-key id remains bounded and omitted from non-configured admissions. Most of the current matrix is green, with macOS still pending.
Two blockers remain before this phase can be approved:
readyProtocolMetadata()still throws when the request has no valid HTTP(S) management origin (src/remote/protocol.ts:45-55), and/readyzspreads it without a bounded fallback insrc/server/index.ts:1065-1074./readyzis an unauthenticated supervisor endpoint that previously always returned the readiness contract; a malformed/hostile Host can now turn a healthy process into an uncaught 500 instead of a stable 200/503. Make origin derivation non-throwing at the handler boundary: either omit the additive remote metadata for an invalid observed origin or return a deliberate bounded invalid-origin response while preserving ordinary readiness. Add a live-server regression with an invalid Host proving no uncaught request failure.- The PR body and the current parent contract describe a different route from the code. The body still says this phase adds
/v1/catalog, uses a strong ETag + 304 +private, no-cache, rejectsx-api-key, caps at 32 MiB, and returns 503. Currentdevalready shipped/v1/catalogin #2979; this delta reconciles it to no-store/no-validator, acceptsx-api-keylike/v1/models, uses a 256 MiB remote cap, and returns 507. The child tests explicitly pin those current-dev choices. Update this PR description and the parent decade docs before merge; a security/release contract cannot simultaneously instruct both cache/admission policies. In particular,030_phase1_protocol_catalog.md:46-49,54-58must match the already-shipped admission matrix and the chosen cap/status.
The stack base #2771 is also still under CHANGES_REQUESTED, and this PR targets the stack branch rather than an integration branch. Resolve the parent contract, rebase this phase onto the corrected parent/current dev, and rerun the exact-head matrix. Do not merge the child while its authoritative parent remains contradictory.
test 3/4 — tests/cli-transport-honesty.test.ts flagged the sync runner for returning a literal 0 after awaiting a handler. That guard exists because the pattern erases a failure the handler recorded in process.exitCode, and the exemption list requires a verified reason rather than a name. The connected branch has no such reason: handleConnectedSyncCatalogWrite drives app-server restarts, so it returns process.exitCode like every other runner. Node types that as number | string; only a numeric code means anything here. hygiene — gui/src/connect-pairing.ts carried an eslint-disable for react-refresh/only-export-components. The rule was right: one file exported both a transport function and a component. Split into connect-pairing-transport.ts rather than suppressed; the transport is testable without React and the form has no logic beyond calling it. gates — the relayed pairing request went out unauthenticated. submitConnectPairing took `fetchImpl: typeof fetch = fetch`, and a default parameter binds the global as it was when the module was evaluated — the unwrapped original, not the wrapper installApiAuthFetch puts on window.fetch. The relay needs the machine-session headers that wrapper attaches, so the hub refused it. Resolved at call time now. gui suite — tests/connect-pairing.test.ts passed alone and failed in the full run. App calls installApiAuthFetch() at module scope, so it runs on first import only; a later test importing App gets the cached module and no install, leaving the wrapper bound to whichever window imported it first. The test now binds the wrapper to its own window before mounting, and claude-toggle-race.test.tsx clears the install latch in afterEach alongside the window it closes. Both are test isolation, not product behavior. The macos failure on these heads is not from this stack: dev's own HEAD fails the same tests/server-auth.test.ts websocket-refresh assertion on the same runner, with #3139's fix already in dev. #2772 went green on rerun of the identical head.
… /v1/catalog #2979 landed /v1/catalog on dev while this phase was open, so the rebase had to decide, per behavior, which side wins rather than taking one branch wholesale. Taken from dev, because narrowing shipped behavior is a regression, not a hardening: GET+HEAD instead of GET-only; a 256 MiB cap with 507 instead of 32 MiB with 503, since a supported 2,000-model catalog serializes to roughly 92 MB and the smaller cap rejected valid input; a malformed persisted catalog rendering as 404 rather than 500, which keeps "your file is corrupt" indistinguishable from "you have no catalog"; and x-api-key accepted, matching /v1/models and the AUTH_MATRIX row #809 shipped. This route forwards no caller credential upstream, so x-api-key carries no extra authority — rejecting it would only 401 Anthropic-SDK clients holding a valid data credential. Taken from this phase, because dev has no equivalent: the x-opencodex-key-id echo that names which configured key was admitted. dev defined withRemoteCatalogKeyId but never called it, so multi-key attribution was dead code; it is now wired into the route, scoped to configured keys, with an unsafe id omitted rather than sanitized and its warning never repeating the id. Taken from neither, per D2 in the design unit: the route emits Cache-Control: no-store with no ETag and no 304. dev's private,no-cache plus a strong byte-derived ETag is unsafe on a body that varies by key identity — no-cache permits storage and forces revalidation, and the revalidation is what crosses identities. /api/catalog keeps its validator; it is management-authenticated, loopback-scoped, and identity-invariant. The duplicate AUTH_MATRIX row this phase added is dropped: dev already had one for /v1/catalog, and the two disagreed on x-api-key, so the matrix contradicted itself and the live-server check failed against whichever row it read first.
36992ba to
4080bdb
Compare
07d7f10 to
4fdd54d
Compare
2 participants
Summary
Verification
Run on lidge-ai (remote CI host) at c10ef21, per the no-local-suite policy:
Checklist