fix(cli): bind hub status credentials to connection snapshots - #4476
Conversation
`ocx status` on a connected client read the connection snapshot and the current `service-api-token` independently, so a reconnect or a key rotation between the two reads could send the new token to the snapshot's hub, or the snapshot's hub the new connection's token. The window is real: `collectRemoteHubStatus` awaits a dynamic import before it reads credentials. It now rereads the persisted connection and passes a token only when that connection still matches the snapshot's `serverUrl`, `apiKeyId` and `connectedAt` AND the token fingerprint matches that connection's `tokenFingerprint`. Otherwise it skips the live request and falls back to the snapshot owner's cache, or reports `unavailable`. A withheld token now carries its own cause. `resolveHubState` reported every null token as "this client has no usable data-plane token", which is false for a client that reconnected and holds a perfectly good token for a different hub - the operator would go re-enroll a credential that is not the problem. The caller supplies the reason through the new `withheldTokenReason`, so a changed connection, a missing token file and a fingerprint mismatch are named separately. That folds the maintainer review finding on #4382; it is the same misdiagnosis class as #4169 in the stop path. Verification: bun test tests/cli/cli-status-hub-state.test.ts (20 pass), tests/clients/client-hub-state.test.ts + tests/server/v1-hub-state.test.ts (35 pass), bun run typecheck, bun run structure:check, bun run privacy:scan, and the docs-site build required by docs-site/AGENTS.md (441 pages) - the one actionable CodeRabbit finding on the source pull request. Carried from #4382 by @luvs01. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
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. |
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (9)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthrough
ChangesRemote Hub status handling
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The status command safely withholds credentials when connection or token ownership changes and falls back to matching cached state or unavailable output. The change is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files. (6 skipped: 6 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
리뷰 · 우선순위 71 / 80설명 수정은 우선순위 71은 원격 허브 상태의 자격 증명 혼선(보안·운영)을 직접 막고, 변경이 status/hub-state로 한정되기 때문이다. tip
심볼 #4382 / #4169 - 같은 결함 가족. 캐리 후 원본 닫기. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
…og-pull Lane L of the contributor carry train: hub status credential binding (#4382 by luvs01) and the authenticated remote catalog pull (#4413 by rrmlima). Tip-only CI by owner authorization for this batch. Run 34744284722 concluded success on 394b96d, the exact head merged here, and it covers both links because the lane is cumulative. #4476 carries no ci check of its own.
|
Merged into dev as part of lane L, under the tip-only CI economy the repository owner authorized for this batch. Three things this record names explicitly, so the deviation stays a recorded decision rather than an inferred one:
Landed via merge commit d865aac, verified as an ancestor of origin/dev. |
Summary
Carries #4382 by @luvs01.
ocx statuson a connected client read the connection snapshot and the currentservice-api-tokenindependently.collectRemoteHubStatusawaits a dynamic import before it reads credentials, so a reconnect or a key rotation inside that window could send the new token to the snapshot's hub, or send the snapshot's hub a token that no longer belongs to it.The collector now rereads the persisted connection and passes a token only when that connection still matches the snapshot's
serverUrl,apiKeyIdandconnectedAt, and the token fingerprint matches that connection'stokenFingerprint. Otherwise it skips the live request and falls back to the snapshot owner's cached hub state, or reportsunavailable.Beyond the source branch, a withheld token now names its own cause.
resolveHubStatereported every null token as "this client has no usable data-plane token". That is false for a client that reconnected and holds a perfectly good token for a different hub, and it sends the operator to re-enroll a credential that is not the problem. The caller supplies the cause through the newwithheldTokenReason, so a changed connection, a missing token file and a fingerprint mismatch read differently:That folds the maintainer review finding on #4382 ("reason 문자열을 한 번 읽어 보면 좋습니다"), and it is the same misdiagnosis class as #4169 in the stop path.
This is the bottom link of lane L in
devlog/_plan/260913_contributor_carry_train/. Its head commit carries[skip ci]; the lane tip runs the matrix for the whole lane.Verification
bun test tests/cli/cli-status-hub-state.test.ts— 20 passbun test tests/clients/client-hub-state.test.ts tests/server/v1-hub-state.test.ts— 35 passbun run typecheck— cleanbun run structure:check— passedbun run privacy:scan— passedcd docs-site && bun install --frozen-lockfile && bun run build— 441 pages built. This is the one actionable CodeRabbit finding on the source pull request, anddocs-site/AGENTS.mdrequires it fordocs-site/**changes.Local full suite was not run, per the lane's instruction. Hosted CI on the lane tip is the suite proof.
Checklist
Summary by CodeRabbit
New Features
ocx statusnow verifies connection and token details before requesting live Remote Hub state.unavailable.Documentation
Tests