test(cli): cover the cloud client 401-refresh-retry decorator#1202
Merged
jrusso1020 merged 1 commit intoJun 4, 2026
Merged
Conversation
createCloudClient wraps the generated client in a Proxy that catches HyperframesApiError(401), force-refreshes credentials, and retries once. That auth recovery path had no tests; a regression would only surface as cloud commands failing outright on server-side token revocation or clock-skew rejections. Covers: passthrough, refresh-and-retry with the new token actually re-resolved (not a stale header replay), refresh failure surfacing the original 401, single-retry on repeated 401, and no refresh on non-401 or transport errors. Zero source changes.
jrusso1020
approved these changes
Jun 4, 2026
Collaborator
jrusso1020
left a comment
There was a problem hiding this comment.
Test-only PR (zero source changes) covering the 401-refresh-retry decorator in packages/cli/src/cloud/index.ts:39 — the auth-recovery path that silently failed every cloud command on token revocation / clock-skew if it regressed. Genuinely untested before this. Reviewed the full test file end-to-end and cross-checked each assertion against the decorator and the generated client.
Strengths
- Goes through
createCloudClient()rather than the unexportedwrapWith401Retry, so the tests also pin the factory wiring the cloud commands actually use (index.test.ts:38). Right altitude. - The load-bearing assertion is the real one:
index.test.ts:78-85checks the retry carriesBearer tok-new, not a replay of the stale header. I verified this isn't vacuous —_gen/client.ts:96re-invokesgetAuthHeaders()insiderequest()per call, so a refresh that wasn't re-resolved would 401 forever. This test would actually catch that. - Refresh-failure case asserts the original 401 surfaces (
index.test.ts:101-105), matching the decorator'scatch { throw err }atindex.ts:60— and correctly notes why (the 401 carries the message/codereportApiErrorneeds), not the refresh error. - "Exactly one retry" (
index.test.ts:115) and the two no-refresh branches (non-401 at:127, transport error at:141) cover the remaining decorator paths. fetchis stubbed inbeforeEachbeforecreateCloudClient()runs in each test body, so theopts.fetchImpl ?? fetchcapture at_gen/client.ts:91resolves to the mock — the PR body's note about this is accurate.
Notes (non-blocking)
- nit:
headersOfreturnsunknownand feedstoMatchObject— fine, but the type narrowing only guards against missingheaders, not its shape.toMatchObjectcarries the real assertion, so this is purely cosmetic.
Verdict: APPROVE
Reasoning: Targeted, correct coverage of a previously-untested auth-recovery path; every assertion maps to a real decorator branch and the key new-token assertion is verified load-bearing. CI green, no source risk.
— Claude (pr-review)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds test coverage for the 401-retry decorator that
createCloudClient()wraps around the generated cloud client (packages/cli/src/cloud/index.ts). Zero source changes; sibling of #1131, which coveredreportApiErrorin the same module.Why
The decorator is the auth recovery path for every cloud command: it catches
HyperframesApiError(status=401), force-refreshes the OAuth token, and retries the call exactly once. None of that was tested. A regression here would only surface for users as cloud commands failing outright on server-side token revocation or clock-skew rejections, exactly the cases the decorator exists to absorb.How
The tests go through
createCloudClient()rather than the unexported decorator, so they also pin the factory wiring the cloud commands actually use.auth.jsis module-mocked to control the token lifecycle, and globalfetchis stubbed (the generated client falls back to it when nofetchImplis injected, andcreateCloudClientdoesn't expose that knob).Six cases:
reportApiErrorneeds)HyperframesApiErrorpass through with no refreshValidation
bunx vitest run(cli package): 679 passed, including the 6 newbunx oxlint/oxfmt --check: cleanbun run build: green