feat(client): a connected client reports the hub's state, not its own - #4255
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: 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. |
리뷰 · 우선순위 76 / 80이 PR은 #4236 허브 단일 포트 스택의 6번째 조각입니다. 형제인 5번 docs(#4254)와 같은 베이스( 고치는 사고는 본문이 한 줄로 말합니다. 연결된 클라이언트 머신에서 에이전트가 로컬 지금 이 PR이 넣는 답은 세 층입니다.
설계 메모는
경로/심볼 - 경로/심볼 - ko 경로/심볼 - 캐시 TTL 없음 - 의도(나이를 보여 주고 사람이 판단). 허브에서 로그인/로스터가 바뀐 뒤 클라이언트가 오래 오프라인이면 경로/심볼 - 경로/심볼 - CI - 호스티드 Cross-platform이 아직 대부분 pending/UNSTABLE. 머지 증거로 쓰려면 초록을 기다리세요. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
b26eee3 to
2aa572d
Compare
e016704 to
7e18fc9
Compare
2e90005 to
902fc02
Compare
… data plane A connected client had no way to ask the hub what it can actually serve, so `ocx status` on the client reported the client's own empty credential store as if it were the truth: `xai ✗ not logged in` on a machine whose hub has xAI logged in and is serving grok. An agent working on such a client read that output and concluded the hub could not serve grok. `GET|HEAD /v1/hub-state` is the least-privilege read that fixes it, built in the `/v1/catalog` (#809) tradition: the same `resolveApiAuth` admission set, the same origin check, no parameters, `no-store`, no validator, and a fixed bounded body of booleans plus model ids. The alternative operators reach for — handing the client an admin token so it can call `GET /api/providers` — is exactly the trade #809 already refused, and widening `/api/*` to the data plane would be worse. What crosses the boundary is deliberately narrow. `hasCredential` is the same `!!p.apiKey` presence projection `GET /api/providers` ships; `loggedIn` is `oauthLoginSummary`'s boolean with the email and account id dropped rather than masked. Provider names already leak through `/v1/catalog` slugs, so the delta is those two booleans. The projection builds every row field by field for that reason: a spread would silently start exporting whatever field is added to a provider or a login record next. The route 404s with its own `hub_state_not_a_hub` code unless `runtimeRole === "hub"`, so a standalone install gains no surface at all, and that gate runs AFTER admission on purpose — answering an anonymous caller would turn the route into a free "is that host a hub?" probe. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…o local state `fetchHubState` sits beside `downloadClientCatalog` because it is the same kind of call: one bounded, schema-validated, unconditional GET with the per-client data key. There is deliberately no management variant — the client holds no hub management credential, and minting one to read a list of booleans is the trade #809 already refused. `resolveHubState` is where the actual fix lives. Every failure path — a 404 from a hub too old to serve the route, a 401, an unreachable host, a non-JSON or malformed or foreign-schema body, an oversized one — lands on `cache` or `unavailable` with a reason, and none of them reaches back into the client's own providers and logins. That fallback is invisible in output, because local state renders exactly like hub state, which is how a client came to report `xai ✗ not logged in` about a hub that had xAI logged in. The last good response is cached 0600 at `<OPENCODEX_HOME>/hub-state.json`, stamped with the (serverUrl, apiKeyId, connectedAt) triple the rest of the client lifecycle compares on. A stale cache is still the HUB's state; an unstamped one would be a different hub's, which after a disconnect and reconnect is not staleness but a lie. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ls what is local The report was not incomplete, it was misattributed. On a connected client `ocx status` printed `OAuth logins:` from this machine's own credential store — empty by design — plus local providers and a local five-model roster, with one buried `Remote hub: connected (<url>)` line as the only hint that none of it described the machine doing the work. An agent read `xai ✗ not logged in` off such a client and concluded the hub could not serve grok. Now a connected client leads with a banner naming the hub origin, prints the hub's OAuth logins, providers and delegable models above the local block, labels that block "not used for routing while connected", and tags the lines that really are about this machine — proxy, health, dashboard, config, service, shim, Codex runtime/version/source/home — `(local)`. The tag appears only while connected: on a standalone install every line is local and tagging them all would teach the reader to ignore the tag. `--json` gains `runtimeRole` and a `remoteHub` block with the same `stateSource: "hub" | "cache" | "unavailable"` honesty. `schemaVersion` stays 1 (additive, same rule as `versionSkew`), and `connection` is untouched: it describes the LINK, `remoteHub` describes what is on the other end of it. When the hub cannot be read the block is empty with a reason and the banner says so, because the one thing this must never do is answer from local state. Providers carry `authMode`, which is what stops "no API key" from reading as "not configured" for an `oauth` provider — the precise inference that went wrong. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`cmdClaude` gated the roster writer on `typeof route === "number"`, which is false on a connected client — `route` is a `ClaudeRoutingTarget` there. So `~/.claude/agents/ocx-*.md` on a client was whatever a previous standalone run had left, indefinitely. And when it did run, it built the roster from local `config.subagentModels`: the list this machine had before it joined the hub. The operator saw five delegable native models on a client whose hub serves grok, with nothing in the output admitting the list described a different machine. The gate is gone and `buildClaudeAgentDefs` takes an explicit roster argument, defaulting to today's behaviour including "unset means the defaults, an explicit `[]` means none". On a connected client `ocx claude` passes the hub's featured roster; when the hub cannot be read it falls back to the local list and SAYS so, because an unannounced fallback is indistinguishable from a correct answer, which is how this defect stayed invisible. The five-row cap stays. It is a Claude Code picker constraint, not the bug — sourcing the five from the wrong machine was. `entryParts` already kept the raw id when a provider is absent from local config, which is what lets `xai/grok-4.6` produce `ocx-grok-4-6.md` on a credential-less client instead of throwing and aborting the sync for every other model too; there is now a test holding that. `syncClaudeAgentDefsAtProxyStartup` uses the same roster from the on-disk cache only. Startup makes no hub round trip for it: the live read belongs on the `ocx claude` path, and an offline hub must not stand between an operator and a local proxy start. Behaviour change worth naming: the first `ocx claude` after this lands rewrites a previously frozen roster on a client. The `generated-by: opencodex` ownership marker is untouched, so a user-authored `ocx-*.md` is still never overwritten or pruned. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…talog blob
`runtimeRole: "client"` and the `client` block were already printed and were
already missed. An agent read a client's `config.json`, saw `providers: {}` and no
grok provider, and concluded the hub could not serve grok — two floors below the
fact that this machine stores no provider credentials on purpose.
Two changes, both about where a reader's eye lands. `_remoteHub` is now the FIRST
key on a client, naming the hub origin and pointing at `ocx status`, which is the
command that has the facts. And `client.priorCatalog` — the base64 snapshot
connect takes before overwriting the local catalog, up to 64 MB of it — prints as
`<omitted: N bytes>`, mirroring `sanitizeModelCostsForDisplay`, instead of
burying every other field under a wall of base64.
`_remoteHub` is synthetic and never persisted: `clientConnectionSchema` is
`.strict()` so a real `client.note` would not validate, and persisted prose drifts
from the behaviour it describes. `config export` emits the real config untouched —
annotation and omission marker both absent — so round trips still validate.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Includes the en remote-hub guide paragraph on what a connected client shows: the `State from hub <origin>` banner, the hub-sourced OAuth/provider/model lines, the `(local)` tags, `runtimeRole` and the `remoteHub` block with its three-valued `stateSource`, and that the read uses the per-client data key only. The ko copy is left for the docs lane and is named as undone in the devlog. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…s every failure `buildHubState` mapped every `config.providers` row, `disabled: true` included, while `/v1/catalog` and `/v1/models` both filter a disabled provider out — so this route was the only data-plane surface that named one, and three comments plus the devlog claimed the delta over `/v1/catalog` was "only the two booleans". A boundary comment that understates what the code discloses is worse than none: it is what the next reviewer checks against. A disabled provider is now dropped entirely (a client cannot route to it, and `authMode` already explains a keyless row), and the comments state the delta exactly, down to the name of an enabled provider the catalog omits for want of a credential. `disabled` stays in the contract because an older hub still sends `true` and a client must label that row. `truncated` joins the body so a cap is reported rather than silently clipping the lists, and `hubStateFailureReason` grows a sentence for `hub_state_content_type_invalid` and for the `hub_state_http_<status>` family — the `ocx status` banner printed `state unavailable (hub_state_http_507)`, which reads like a client bug when the hub has in fact answered. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…e cache on disconnect `_remoteHub.connected` was hardcoded `true` for any config carrying a `client` block, so a machine whose data key was revoked at the hub, rotated away, or whose token file was gone still printed `connected: true` in `ocx config show`. Configuration is not evidence that a connection works, which is the defect this unit exists to fix, in miniature. `remoteHubConfigNote` now reads `collectClientConnectionStatus()` and requires both halves — a settled connection record and the token file whose fingerprint matches it — and names what is wrong when either fails. The status is passed as a thunk so a standalone or hub install returns before the probe, and `./connect` is imported lazily so `ocx config get/set` does not drag the client lifecycle in. `disconnectClient` also unlinks `<OPENCODEX_HOME>/hub-state.json`. It is owner-stamped, so a reader would reject it, but a disconnected machine should not keep a file naming the former hub's providers and logins. Best effort, after the connection is cleared: the disconnect has already succeeded by then and a stubborn cache file must not fail it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…opback listener The decision not to add the route to `loopbackRouteAllowed` had no test behind it. This one starts a real hub with the loopback listener bound, asks it for `GET /v1/hub-state` with no credential, and asserts a 404 whose code is `not_found` — the listener's refusal, not the route's own `hub_state_not_a_hub`, which would have meant the request reached the handler. The same run then reads the route successfully on the public listener with a data key, so a route that disappeared entirely cannot make the first assertion pass vacuously. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…e ownership Seven findings, all accepted, with the corrected disclosure delta for `/v1/hub-state` written down in the place the wrong one was: "only the two booleans" became the exact list, and the note that a disabled provider is not exported at all. `structure/01_runtime.md` now names `src/remote/hub-state.ts` and `src/client/hub-state.ts` under remote-hub ownership, including the rule that a failed read reports "unavailable" rather than degrading to local state. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
4ccf177 to
af7c114
Compare
1 participant
Summary
An agent running on a connected client machine read that machine's local
~/.opencodex/config.jsonandocx status, sawxai ✗ not logged in, no grok provider and only five delegable models, and concluded the hub could not serve grok — while the hub has xAI logged in and serves grok.Nothing malfunctioned. Every number it read was correct about the client, and a client stores no provider credentials and no featured roster by design. The defect is attribution: three surfaces reported local facts in the voice of the system, and a locally sourced report is byte-indistinguishable from a hub-sourced one.
The hub now has something to ask.
GET|HEAD /v1/hub-stateis a least-privilege data-plane read in the/v1/catalog(#809) tradition: the sameresolveApiAuthadmission set and the same origin check, because nothing here forwards a caller credential upstream; no query parameters,Cache-Control: no-store, no validator, a fixed bounded body. It answers with booleans and model ids —hasCredentialis the!!p.apiKeypresence projectionGET /api/providersalready ships,loggedInisoauthLoginSummary's boolean with the email and account id dropped rather than masked — plus the hub'ssubagentModelsroster, its version andruntimeRole.buildHubStatebuilds every row field by field and never spreads a provider or a login record, which is what makes "no keys, no emails, no account ids" checkable by reading one function; a spread would silently begin exporting whatever field is added to those records next. The route 404s with its ownhub_state_not_a_hubcode unlessruntimeRole === "hub", so a standalone install gains no surface at all, and that gate runs after admission on purpose — answering an anonymous caller would turn the route into a free "is that host a hub?" probe. A provider the operator markeddisabledis not exported at all, so the disclosure delta over/v1/catalogand/v1/modelsis exactly:hasCredential,loggedIn,authMode, the roster, and the name and adapter of an enabled provider the catalog omits for want of a usable credential — which is the point of the route.truncatedreports a cap being hit rather than serving a prefix silently. It is deliberately absent fromloopbackRouteAllowed(pinned by a test on a real listener, see the review round), and per the reviewer note on #4236 nothing was added to/api/*and no admin credential goes anywhere near a client.ocx statuson a connected client reports the hub and labels what is local. The report leads withState from hub <origin>: provider credentials, logins and delegable models below are the HUB's, not this machine's., prints the hub's OAuth logins, providers and delegable models above the local block, labels that blockLocal-only (not used for routing while connected), and tags the lines that really describe this machine — proxy, health, dashboard, config, PID file, runtime, service, shim, Codex runtime/version/source/home —(local). The tag appears only while connected: on a standalone install every line is local and tagging them all would train the reader to skip the tag. Providers carryauthMode, which is what stops "no API key" from reading as "not configured" for an OAuth provider — the precise inference that went wrong.Failure is reported, never substituted.
stateSourceis three-valued:hub(live),cache(the last good read from this connection, still the hub's state),unavailable(nothing true is known). An older hub that 404s the route produces⚠️ Hub <origin>: state unavailable (this hub is too old to report its state; upgrade the hub) — provider and login lines below are LOCAL and do not describe the hub.Every other failure — 401, unreachable, non-JSON, malformed, foreign schema, oversized — lands the same way. A two-valued ok/failed flag would have invited the same silent local fallback at the next call site. The last good response is cached 0600 at<OPENCODEX_HOME>/hub-state.json, stamped with the(serverUrl, apiKeyId, connectedAt)triple and compared withsameClientConnectionOwner: a stale cache is still the hub's state, while an unstamped one would be a different hub's after a disconnect and reconnect, which is not staleness but a lie.The spawn surface.
cmdClaudegated the roster writer ontypeof route === "number", false on a connected client (routeis aClaudeRoutingTarget), so~/.claude/agents/ocx-*.mdstayed whatever a previous standalone run had left — and when it did run it built from localconfig.subagentModels. That is the operator's "only 5 delegable models". The gate is gone;buildClaudeAgentDefsandinjectClaudeAgentDefstake an explicit roster argument defaulting to today's behaviour (including "unset means the defaults, an explicit[]means none"), and on a client the hub's roster is passed in. The five-row cap stays — it is a Claude Code picker constraint, not the bug; sourcing the five from the wrong machine was, and the hub can now change which five without touching the client. An unreadable hub falls back to the local list and prints a warning, because an unannounced fallback is exactly how this stayed invisible.entryPartsalready kept the raw id when a provider is absent locally, which is what letsxai/grok-4.6yieldocx-grok-4-6.mdon a credential-less client instead of throwing and aborting the sync for every other model too; that was latent and untested and now has a test. Thegenerated-by: opencodexownership marker is untouched.syncClaudeAgentDefsAtProxyStartupuses the same roster from the cache only — startup makes no hub round trip, so an offline hub cannot stand between an operator and a local proxy start.Behaviour change worth naming: the first
ocx claudeafter this lands rewrites a previously frozen roster on a client.ocx config showputs a synthetic_remoteHubnote first on a client —{ connected, origin, note: "provider credentials and model availability live on the hub; run ocx status" }— because it has to be read before the emptyprovidersmap, not after it.connectedis observed fromcollectClientConnectionStatus()(a settled connection record and the token file whose fingerprint matches it), never inferred from the presence of aclientblock; when either half fails the note names what is wrong instead.client.priorCatalog, a base64 catalog snapshot up to 64 MB, prints as<omitted: N bytes>, mirroringsanitizeModelCostsForDisplay. Both are display-only:config exportemits the real config untouched so round trips still validate, and a persistedclient.notewas rejected becauseclientConnectionSchemais.strict()and persisted prose drifts.--jsongainsruntimeRoleand an always-presentremoteHubblock.schemaVersionstays 1 (additive, the same rule asversionSkew), andconnectionis untouched: it describes the link,remoteHubdescribes what is on the other end of it.Design and decision record:
devlog/_plan/260911_hub_single_port/060_client_hub_state.md. The en remote-hub guide gains a "What a connected client shows" section; the ko copy is named as undone there, with three other follow-ups (GET /api/machine/hub-statefor the client's own dashboard, aconfiguredboolean to distinguish a never-configured OAuth provider from a logged-out one, andocx doctor, which still reports local state on a client).Verification
bun test tests/server/server-auth.test.tsis 111 pass / 1 fail on this branch and, re-run at the base commit8c277294c, 111 pass / 1 fail there too —native passthrough upstream reset still logs 502 and penalizes the pool, the same pre-existing failure PR4's devlog recorded. Not a regression.scripts/test-layout/layout.json(explicit) andtests/fixtures/test-layout-expected.json. The one that cannot be satisfied by reading the projection istests/server/v1-hub-state.test.ts: it configures a real-looking provider key and a real-looking OAuth credential (access token, refresh token, email) and asserts none of those bytes, and none of the field names that carry them, appear in the serialized response. It also pins 401-before-the-role-is-disclosed, the cross-origin 403, HEAD, the role gate's own 404 onstandaloneand on an absent role, and thecontent-length/size ceiling. The admission-matrix proof — that this 404 is the role gate and not a vanished route, so no accepted cell passes vacuously — is driven against a real request intests/server/api-key-attribution.test.ts, which asserts thehub_state_not_a_hubcode; that is where the standalone 404 is pinned as admission evidence.tests/clients/client-hub-state.test.tsenumerates every failure mode of the fetch and asserts each lands oncacheorunavailablewith a reason — the invariant being defended is that none of them reaches back into local state. It also covers owner mismatch, a rotatedapiKeyId, and malformed and symlinked cache files.tests/cli/cli-status-hub-state.test.tsspawnsocx statusthree times: against a local fake hub (which refuses any request not carrying the client's own data key, so the credential path is proven), against an unreachable hub, and on a standalone machine whose output gains no banner and no(local).ocx service …,ocx start/stop/ensure/sync/restore/connect/disconnectandlaunchctlwere not run, and the real~/.opencodex,~/.codex,~/.claude/agentsand~/Library/LaunchAgentswere not touched. Every test pointsOPENCODEX_HOMEat amkdtempdirectory, andtests/preload.tsarmsOCX_TEST_HOME_GUARD=1for every invocation including a barebun test <file>.Review round
Seven findings, all accepted. The two that mattered are the same mistake this PR is about,
committed by this PR: a boundary comment claiming less disclosure than the code performed, and a
connected: trueinferred from configuration rather than observed./v1/hub-stateexported every provider row,disabled: trueincluded (should-fix) — while/v1/catalogand/v1/modelsboth filter a disabled provider out, making this the only data-plane surface that named one. Three comments and the devlog claimed the delta was "only the two booleans".buildHubStatenow drops a disabled provider entirely (a client cannot route to it, andauthModealready explains a keyless row), and all four places state the delta exactly.HubStateProvider.disabledstays in the contract, alwaysfalsefrom a hub of this version: an older hub still sendstrue, and a client reading one must label that row rather than present it as routable.hub_state_content_type_invalidandhub_state_http_<status>printed as bare codes (should-fix) — the banner could readstate unavailable (hub_state_http_507), which sends an operator hunting for a client bug when the hub has answered (507 is the hub's ownhub_state_too_large). Both render as sentences now: "the hub's state response was not JSON" and "the hub answered HTTP N to the state request", with the numeric suffix validated so a non-numeric code still falls back to the code._remoteHub.connectedwas hardcodedtrue(should-fix) — including on a machine whose key was revoked at the hub, rotated away, or whose token file was deleted. Derived fromcollectClientConnectionStatus()now, requiring both the connection record and the matching token file; the status arrives as a thunk so a standalone or hub install returns before the probe, and./connectis imported lazily soocx config get/setdoes not drag the client lifecycle in..slice(0, MAX_HUB_STATE_PROVIDERS)truncated silently (nit) — a hub with 240 providers served 200 and said nothing, so the client told its reader the other 40 do not exist.truncated: booleanis now in the contract and the parser (absent reads asfalse, for an older hub; a present non-boolean is still refused), andocx statusappends "(the hub truncated this state to fit its response caps; some rows are not listed)".disconnectClientnow unlinks<OPENCODEX_HOME>/hub-state.json, best effort, after the connection is cleared.GET /v1/hub-statewith no credential and must answer 404 with codenot_found(the listener's refusal), nothub_state_not_a_hub(which would mean the handler was reached); the same run then reads the route successfully on the public listener with a data key, so a deleted route cannot pass vacuously. The verification bullet above is also corrected: the standalone 404 is pinned as admission evidence inapi-key-attribution.test.ts, notv1-hub-state.test.ts.structure/01_runtime.md(nit) — "Remote Hub hardening ownership" now namessrc/remote/hub-state.tsandsrc/client/hub-state.ts, including the rule that a failed read reports "unavailable" rather than degrading to local state.Verification (review round)
No new test files, so
layout.jsonandtests/fixtures/test-layout-expected.jsonare unchanged. No repository-wide suite (operator instruction); hosted CI on the pushed head is the proof. Noocx service …,ocx start/stop/ensure/sync/connect/disconnect/statusorlaunchctlwas run on this live hub, and every test keptOPENCODEX_HOMEin amkdtempdirectory.Checklist
Refs #4236