feat(device): implement OAuth 2.0 Device Authorization Grant#166
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughAdds a new package ChangesDevice Authorization Package Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (9)
packages/device/test/actions/poll.test.ts (1)
135-150: ⚡ Quick winAssert backoff timing, not only final call count.
This test passes even if
slow_downis ignored, sincerunAllTimersAsync()drains everything and you only assert total calls. Please assert delay behavior (e.g., step timers withvi.advanceTimersByTimeAsyncand verify the next token request does not happen before the increased interval).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/device/test/actions/poll.test.ts` around lines 135 - 150, The test currently only asserts total fetch calls so it won't catch ignored slow_down timing; update the test for poll(context) to assert backoff timing by using vi.advanceTimersByTimeAsync instead of vi.runAllTimersAsync: after the first fetch returns { error: "slow_down" } verify that advancing timers by less than the expected increased interval does NOT trigger the next token request (fetchMock call count stays 1), then advance by the remaining time (past the increased interval) and assert fetchMock increments to 2 (and later to 3 after the userProfile fetch). Reference poll(context), fetchMock, and the pollPromise to control and observe timer-based scheduling.packages/device/src/@types/device.ts (2)
3-3: Consider removingparamswrapper if onlyscopeis supported.The
paramsobject inDeviceAuthorizationConfigcurrently contains onlyscope. If no other parameters are anticipated, consider flattening to{ url: string; scope?: string }for simpler access patterns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/device/src/`@types/device.ts at line 3, The DeviceAuthorizationConfig type currently uses a nested params object for scope which adds unnecessary nesting; update the DeviceAuthorizationConfig definition so the object form is flattened to { url: string; scope?: string } instead of { url: string; params?: { scope?: string } }, and update any usages of DeviceAuthorizationConfig (e.g., places that access .params.scope) to read .scope directly to avoid breaking behavior.
28-33: Consider allowing parameters for token and userInfo endpoints.The
accessTokenanduserInfoconfigurations accept{ url: string }but don't support parameters likedeviceAuthorizationdoes. Some OAuth providers may require query parameters or headers for these endpoints. Consider whether the configuration should be more flexible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/device/src/`@types/device.ts around lines 28 - 33, The accessToken and userInfo type definitions currently accept string or { url: string } but lack support for additional parameters; update the type for accessToken and userInfo (in packages/device/src/@types/device.ts) to mirror the more flexible shape used by deviceAuthorization by allowing an object like { url: string; params?: Record<string,string>; headers?: Record<string,string> } (or a named interface) so callers can supply query parameters or headers when calling those endpoints; ensure any code that consumes accessToken and userInfo (search for references to accessToken, userInfo, and deviceAuthorization) is updated to read optional params/headers when building requests.packages/device/src/shared/env.ts (3)
35-39: ⚡ Quick winDocument the precedence order for environment variable lookup.
The
getEnvfunction triesAURA_AUTH_*,AURA_*,AUTH_*, and then the raw key. Document this precedence explicitly so users understand which variable will be used when multiple are set. Consider whether this could lead to confusion if users set multiple variants.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/device/src/shared/env.ts` around lines 35 - 39, The getEnv function currently checks keys in the order defined by the keys array (AURA_AUTH_<KEY>, AURA_<KEY>, AUTH_<KEY>, then <KEY>) but this precedence isn’t documented; add a concise JSDoc comment immediately above the getEnv function that explicitly lists that lookup order and states that the first match wins (AURA_AUTH_* > AURA_* > AUTH_* > raw), and also consider (optional) adding a non-throwing runtime warning/log (inside getEnv) when more than one of the variants is set to alert users to potential confusion so they know which variable is being used.
1-1: Minimize the scope of@ts-nocheckif possible.The
@ts-nocheckdirective disables all type checking for this file, which can hide legitimate type errors. Consider using more targeted@ts-expect-erroror@ts-ignorecomments on specific lines where cross-runtime checks are problematic, or use conditional types to preserve type safety.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/device/src/shared/env.ts` at line 1, The file currently has a top-level "`@ts-nocheck`" which disables all type checking; remove that directive and instead apply narrowly scoped fixes: replace the file-level "`@ts-nocheck`" with explicit type annotations for exported values/functions (e.g., any exported env accessors or parsers) and use targeted "// `@ts-expect-error`" or "// `@ts-ignore`" comments only on the specific lines that trip cross-runtime typing, or refactor those lines to use conditional/union types to model runtime differences; ensure exported symbols retain proper TypeScript types so the module remains type-safe while tolerating the few runtime-specific exceptions.
29-31: Consider logging or exposing environment access errors.The
catchblock silently swallows all errors, which may hide configuration issues or runtime problems. In development, consider logging these errors or providing a way to surface them to the consumer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/device/src/shared/env.ts` around lines 29 - 31, The catch block in packages/device/src/shared/env.ts currently swallows all errors (the anonymous catch that returns undefined); update it to log or surface the error before returning undefined—e.g., use the project's logger (or console.error) to record the caught exception with context (mention the env accessor or function name in the log) so configuration/runtime access problems are visible while preserving the current undefined return behavior.packages/device/src/@types/config.ts (1)
28-28: ⚡ Quick winDocument the unit and epoch for
expiresAt.The
expiresAtfield is anumberbut it's unclear whether this represents Unix milliseconds, Unix seconds, or a relative duration. Add a JSDoc comment to clarify the expected format.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/device/src/`@types/config.ts at line 28, The expiresAt field's numeric unit/epoch is ambiguous; update the declaration for expiresAt to include a JSDoc comment clarifying whether it is a Unix timestamp in milliseconds or seconds (and whether it's UTC), e.g. "Unix epoch milliseconds (ms since 1970-01-01T00:00:00Z)"; place this comment immediately above the expiresAt declaration in the type definition so callers of expiresAt know the expected format and units.packages/device/src/@types/session.ts (2)
9-9: Consider renaming thesessionproperty for clarity.The
Sessioninterface has asessionproperty, creating a redundant naming pattern likesession.session.sub. Consider renaming the property touserordatafor better ergonomics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/device/src/`@types/session.ts at line 9, The Session interface currently defines a property named session (Session.session) which leads to redundant access like session.session.sub; rename the property to a clearer name (e.g., user or data) in the Session interface declaration and update all touched symbols — references, type annotations, constructors, serializers/deserializers, and tests that access Session.session — to the new name (for example, change Session.session -> Session.user and update usages like session.session.sub -> session.user.sub); ensure exported types and any runtime code (parsers, JSON keys, middleware) are adjusted accordingly to avoid breaking callers.
10-10: ⚡ Quick winDocument the format for the
expiresfield.The
expiresfield is typed asstringbut doesn't specify the expected format. Should this be an ISO 8601 date string, a Unix timestamp string, or another format? Add JSDoc clarification.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/device/src/`@types/session.ts at line 10, The expires field lacks a documented format; add a JSDoc comment directly above the expires property (the expires field in the session type/interface) that states the exact expected format (e.g., "ISO 8601 UTC datetime string, e.g. '2026-05-22T15:30:00Z'") and whether offsets are allowed, and update the comment if you instead expect a Unix timestamp string; keep the TypeScript type as string but clarify the canonical representation and provide one example to guide callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/device/package.json`:
- Line 4: Decide whether packages/device is intended to be published or the
README should be corrected: if it should be public, remove the "private": true
field from package.json, ensure package.json includes the correct "name"
("`@aura-stack/device`"), valid "version", "license", "repository" and any needed
"publishConfig" and then run npm/pnpm publish; if it should remain private,
update packages/device/README.md to remove npm/jsr badges and change the install
instruction ("pnpm add `@aura-stack/device`") to reflect that the package is not
published (or provide local install instructions), keeping the package metadata
and README consistent.
In `@packages/device/src/`@types/config.ts:
- Line 7: The providers array currently uses DeviceProviderCredentials<any,
DefaultUser>, which erases the provider Profile type and breaks type-safety for
the profile transformer; update the type to avoid any—either use
DeviceProviderCredentials<unknown, DefaultUser> to safely preserve unknown
profile shapes or make the surrounding config generic so providers is typed as
DeviceProviderCredentials<P, DefaultUser>[] (or a union preserving each
provider's profile) so the profile transformer on DeviceProviderCredentials can
be type-checked; adjust the providers declaration (and the config type if you
choose the generic route) accordingly, keeping references to
BuiltInDeviceProvider, DeviceProviderCredentials and DefaultUser.
- Line 32: The providers registry currently uses DeviceProviderCredentials<any,
any> which erases Profile/DefaultUser type info; make the containing type
generic and propagate those generics into providers so type parameters are
preserved—for example, add generics (Profile, DefaultUser) to the interface/type
that contains providers and change providers:
Record<LiteralUnion<BuiltInDeviceProvider>, DeviceProviderCredentials<any, any>>
to providers: Record<LiteralUnion<BuiltInDeviceProvider>,
DeviceProviderCredentials<Profile, DefaultUser>> (or use unknown instead of any
if you prefer stricter typing), updating usages of that containing type
accordingly; reference: providers, LiteralUnion, BuiltInDeviceProvider,
DeviceProviderCredentials, Profile, DefaultUser.
In `@packages/device/src/actions/authorize.ts`:
- Around line 27-45: Wrap the call to response.json() in a try/catch to handle
non-JSON responses from fetcher: attempt to parse JSON for the success/error
flow used by OAuthDeviceAuthorizationResponse and safeParse, but if parsing
throws, read response.text() (or use a fallback) and throw a DeviceOAuthError
(e.g., error "server_error") with a description that includes the HTTP status
and the raw response body; ensure the existing error branch that checks
response.ok still uses the parsed json when available but falls back to the
caught non-JSON path so all failures surface as DeviceOAuthError instead of raw
parser exceptions.
In `@packages/device/src/actions/poll.ts`:
- Around line 74-99: The loop in poll.ts currently issues the first token POST
immediately; change it to honor the negotiated initial interval by waiting
before the first request. In the function containing the while loop (use the
variables intervalMs and initialIntervalMs and the loop that calls
fetcher(tokenURL,...)), add logic to sleep for intervalMs (or initialIntervalMs)
on the first iteration—e.g. introduce a boolean like firstPoll = true and if
firstPoll and intervalMs > 0 await sleep(intervalMs) then set firstPoll =
false—so subsequent iterations still use the existing slow_down handling
(intervalMs += SLOW_DOWN_INTERVAL_INCREMENT_SECONDS * 1000) and continue to
sleep as implemented.
- Around line 101-132: The pending device code is only cleared on the successful
happy path (context.setPending?.(null) after getUserInfo), so failures
(DeviceOAuthError, DeviceAuthError, parsing/userinfo errors, or timeout) leave
stale state; update the poll logic (the poll function) to always clear the
pending state on any terminal outcome by invoking context.setPending?.(null)
before every throw path or, better, wrap the polling/token-exchange block in a
try/finally and call context.setPending?.(null) in the finally so that errors
thrown from DeviceOAuthError, DeviceAuthError, safeParse failures, getUserInfo
failures, and the timeout case all clear the pending value.
In `@packages/device/src/actions/userinfo.ts`:
- Around line 8-20: The code builds `sub` from multiple profile fields but only
stringifies `profile.id`, so numeric `profile.sub`/`uid`/`user_id` can leak as
numbers; update the `sub` assignment to normalize every candidate to a string
(or undefined) before selecting it (e.g., coerce each of `profile.id`,
`profile.sub`, `profile.uid`, `profile.user_id` to string if present) so `sub`
is always a string, and adjust the error branch in the same function to rely on
that normalized `sub`; locate the `sub` variable and the return object in this
file (references: `sub`, `profile`, returned `email`, `name`, `image`) and apply
the normalization consistently.
In `@packages/device/src/schemas.ts`:
- Around line 15-16: The accessToken and userInfo schema entries currently wrap
a single branch in valibot.union; remove the unnecessary union and use the
direct validators instead (replace valibot.union([valibot.pipe(valibot.string(),
valibot.url())]) with valibot.pipe(valibot.string(), valibot.url()) for both
accessToken and userInfo) so the schema uses the intended single URL-validated
string validators without an extra union layer.
In `@packages/device/src/shared/fetcher.ts`:
- Around line 1-10: The JSDoc for the fetcher function does not state that the
timeout triggers controller.abort(), causing fetch to throw an AbortError (or
DOMException with name "AbortError"); update the comment above fetcher to
explicitly document that a timeout will abort the request and that callers must
catch/handle the AbortError (or DOMException "AbortError") when using fetcher,
mentioning the default timeout behavior and the error type so consumers can
handle timeouts gracefully.
- Around line 15-18: The fetch call currently overwrites any caller-provided
options.signal with controller.signal, silently preventing external aborts;
change this to create a merged AbortSignal (e.g., new AbortController merged =
new AbortController()), wire options.signal and controller.signal to call
merged.abort() (options.signal?.addEventListener('abort', () => merged.abort())
and controller.signal.addEventListener('abort', () => merged.abort())), pass
merged.signal into fetch instead of controller.signal, and ensure you remove
listeners and clearTimeout(timeoutId) in finally to avoid leaks; reference the
existing controller, timeoutId, and the fetch(...) call in fetcher.ts.
In `@packages/device/vitest.config.ts`:
- Around line 19-20: Replace the ESM-unsafe use of __dirname in vitest.config.ts
by deriving a dirname from import.meta.url and using that with path.resolve for
the aliases; specifically, compute a module dirname (via
fileURLToPath(import.meta.url) and path.dirname) at the top of the file and then
update the alias targets referenced as "@" and "`@test`" (currently defined as
path.resolve(__dirname, "./src") and path.resolve(__dirname, "./test")) to use
the derived dirname so the config works under "type":"module".
---
Nitpick comments:
In `@packages/device/src/`@types/config.ts:
- Line 28: The expiresAt field's numeric unit/epoch is ambiguous; update the
declaration for expiresAt to include a JSDoc comment clarifying whether it is a
Unix timestamp in milliseconds or seconds (and whether it's UTC), e.g. "Unix
epoch milliseconds (ms since 1970-01-01T00:00:00Z)"; place this comment
immediately above the expiresAt declaration in the type definition so callers of
expiresAt know the expected format and units.
In `@packages/device/src/`@types/device.ts:
- Line 3: The DeviceAuthorizationConfig type currently uses a nested params
object for scope which adds unnecessary nesting; update the
DeviceAuthorizationConfig definition so the object form is flattened to { url:
string; scope?: string } instead of { url: string; params?: { scope?: string }
}, and update any usages of DeviceAuthorizationConfig (e.g., places that access
.params.scope) to read .scope directly to avoid breaking behavior.
- Around line 28-33: The accessToken and userInfo type definitions currently
accept string or { url: string } but lack support for additional parameters;
update the type for accessToken and userInfo (in
packages/device/src/@types/device.ts) to mirror the more flexible shape used by
deviceAuthorization by allowing an object like { url: string; params?:
Record<string,string>; headers?: Record<string,string> } (or a named interface)
so callers can supply query parameters or headers when calling those endpoints;
ensure any code that consumes accessToken and userInfo (search for references to
accessToken, userInfo, and deviceAuthorization) is updated to read optional
params/headers when building requests.
In `@packages/device/src/`@types/session.ts:
- Line 9: The Session interface currently defines a property named session
(Session.session) which leads to redundant access like session.session.sub;
rename the property to a clearer name (e.g., user or data) in the Session
interface declaration and update all touched symbols — references, type
annotations, constructors, serializers/deserializers, and tests that access
Session.session — to the new name (for example, change Session.session ->
Session.user and update usages like session.session.sub -> session.user.sub);
ensure exported types and any runtime code (parsers, JSON keys, middleware) are
adjusted accordingly to avoid breaking callers.
- Line 10: The expires field lacks a documented format; add a JSDoc comment
directly above the expires property (the expires field in the session
type/interface) that states the exact expected format (e.g., "ISO 8601 UTC
datetime string, e.g. '2026-05-22T15:30:00Z'") and whether offsets are allowed,
and update the comment if you instead expect a Unix timestamp string; keep the
TypeScript type as string but clarify the canonical representation and provide
one example to guide callers.
In `@packages/device/src/shared/env.ts`:
- Around line 35-39: The getEnv function currently checks keys in the order
defined by the keys array (AURA_AUTH_<KEY>, AURA_<KEY>, AUTH_<KEY>, then <KEY>)
but this precedence isn’t documented; add a concise JSDoc comment immediately
above the getEnv function that explicitly lists that lookup order and states
that the first match wins (AURA_AUTH_* > AURA_* > AUTH_* > raw), and also
consider (optional) adding a non-throwing runtime warning/log (inside getEnv)
when more than one of the variants is set to alert users to potential confusion
so they know which variable is being used.
- Line 1: The file currently has a top-level "`@ts-nocheck`" which disables all
type checking; remove that directive and instead apply narrowly scoped fixes:
replace the file-level "`@ts-nocheck`" with explicit type annotations for exported
values/functions (e.g., any exported env accessors or parsers) and use targeted
"// `@ts-expect-error`" or "// `@ts-ignore`" comments only on the specific lines
that trip cross-runtime typing, or refactor those lines to use conditional/union
types to model runtime differences; ensure exported symbols retain proper
TypeScript types so the module remains type-safe while tolerating the few
runtime-specific exceptions.
- Around line 29-31: The catch block in packages/device/src/shared/env.ts
currently swallows all errors (the anonymous catch that returns undefined);
update it to log or surface the error before returning undefined—e.g., use the
project's logger (or console.error) to record the caught exception with context
(mention the env accessor or function name in the log) so configuration/runtime
access problems are visible while preserving the current undefined return
behavior.
In `@packages/device/test/actions/poll.test.ts`:
- Around line 135-150: The test currently only asserts total fetch calls so it
won't catch ignored slow_down timing; update the test for poll(context) to
assert backoff timing by using vi.advanceTimersByTimeAsync instead of
vi.runAllTimersAsync: after the first fetch returns { error: "slow_down" }
verify that advancing timers by less than the expected increased interval does
NOT trigger the next token request (fetchMock call count stays 1), then advance
by the remaining time (past the increased interval) and assert fetchMock
increments to 2 (and later to 3 after the userProfile fetch). Reference
poll(context), fetchMock, and the pollPromise to control and observe timer-based
scheduling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a8e9e22-da70-4897-afe5-f96e5fde2935
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
packages/core/package.jsonpackages/device/CHANGELOG.mdpackages/device/README.mdpackages/device/deno.jsonpackages/device/package.jsonpackages/device/src/@types/config.tspackages/device/src/@types/device.tspackages/device/src/@types/index.tspackages/device/src/@types/session.tspackages/device/src/actions/authorize.tspackages/device/src/actions/poll.tspackages/device/src/actions/userinfo.tspackages/device/src/device-client.tspackages/device/src/index.tspackages/device/src/providers/github.tspackages/device/src/providers/index.tspackages/device/src/schemas.tspackages/device/src/shared/constants.tspackages/device/src/shared/env.tspackages/device/src/shared/errors.tspackages/device/src/shared/fetcher.tspackages/device/src/shared/form.tspackages/device/src/shared/sleep.tspackages/device/src/shared/url.tspackages/device/test/actions/authorize.test.tspackages/device/test/actions/poll.test.tspackages/device/test/providers.test.tspackages/device/tsconfig.jsonpackages/device/tsdown.config.tspackages/device/vitest.config.tspnpm-workspace.yaml
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/device/src/providers/index.ts (1)
37-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t fallback to env when
clientIdis explicitly provided but invalid.The current truthy check treats
clientId: ""as absent, so Line 38 may silently pull from env instead of failing validation for the supplied config.Suggested fix
- const hasCredentials = config.clientId - const envConfig = hasCredentials ? {} : { clientId: defineOAuthEnvironment(config.id) } + const hasExplicitClientId = config.clientId !== undefined && config.clientId !== null + const envConfig = hasExplicitClientId ? {} : { clientId: defineOAuthEnvironment(config.id) }
♻️ Duplicate comments (2)
packages/device/src/actions/poll.ts (1)
110-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear pending state on the non-OK terminal branch too.
This throw path skips
cleanUpPending(), leaving stale pending auth after terminal server errors.🔧 Proposed fix
if (!response.ok) { const message = typeof json === "object" && json !== null && "error_description" in json ? String((json as { error_description: string }).error_description) : `Token request failed (${response.status}).` + cleanUpPending() throw new DeviceOAuthError("server_error", message) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/device/src/actions/poll.ts` around lines 110 - 116, The non-OK branch in the token response handling (inside the function in packages/device/src/actions/poll.ts that currently throws DeviceOAuthError("server_error", ...)) skips calling cleanUpPending(), leaving stale pending auth; modify that branch to call cleanUpPending() (await it if it's async) before throwing the DeviceOAuthError so pending state is always cleared on terminal errors, keeping the existing message construction for the thrown error.packages/device/src/shared/fetcher.ts (1)
16-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRespect pre-aborted caller signals and clean up abort listeners.
If
options.signalis already aborted, this path won’t abort the internal controller, and the request may proceed unexpectedly. Also, the listener should be removed infinallyto avoid leaks.🔧 Proposed fix
export const fetcher = async (url: string | Request, options: RequestInit = {}, timeout: number = 5000) => { const controller = new AbortController() const timeoutId = setTimeout(() => controller.abort(), timeout) + const onExternalAbort = () => controller.abort() if (options.signal) { - options.signal.addEventListener("abort", () => controller.abort()) + if (options.signal.aborted) { + controller.abort() + } else { + options.signal.addEventListener("abort", onExternalAbort, { once: true }) + } } const response = await fetch(url, { ...options, signal: controller.signal, - }).finally(() => clearTimeout(timeoutId)) + }).finally(() => { + clearTimeout(timeoutId) + options.signal?.removeEventListener("abort", onExternalAbort) + }) return response }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/device/src/shared/fetcher.ts` around lines 16 - 23, The code doesn't respect pre-aborted caller signals and leaks the abort listener; before attaching the listener check if options.signal?.aborted and call controller.abort() immediately (so the internal controller reflects a pre-aborted caller), and when you add the listener use a named handler (e.g., const onAbort = () => controller.abort()) so you can remove it in the finally block; also in the finally ensure you remove the options.signal event listener (if one was added) and still clearTimeout(timeoutId) to avoid leaks — update the listener handling around the addEventListener call and the finally after the fetch invocation (references: options.signal, controller, timeoutId, fetch).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/device/src/actions/userinfo.ts`:
- Line 24: Wrap the call to response.json() in a try/catch so JSON parse
failures are caught and re-thrown as a DeviceOAuthError; replace the direct
await response.json() assignment to profile with a guarded block that tries to
parse, and on any exception throws new DeviceOAuthError(...) (including the
original error/message and relevant context such as response.status or the raw
response text if helpful) so the action's typed error surface remains consistent
(referencing the profile variable assignment and response.json() call, and the
DeviceOAuthError class).
In `@packages/device/test/actions/authorize.test.ts`:
- Around line 26-27: Tests in authorize.test.ts call
vi.stubEnv("GITHUB_CLIENT_ID") (and similar stubs later) but teardown only
clears globals; add environment cleanup to avoid leakage by removing those
stubbed env vars after each test (e.g., in an afterEach/afterAll teardown).
Specifically, after tests that call vi.stubEnv("GITHUB_CLIENT_ID") remove the
stubbed value (either via vi.unstubEnv("GITHUB_CLIENT_ID") if your test runner
supports it or by deleting process.env.GITHUB_CLIENT_ID) and do the same for any
other env keys stubbed in the file so subsequent tests run in a clean
environment.
---
Duplicate comments:
In `@packages/device/src/actions/poll.ts`:
- Around line 110-116: The non-OK branch in the token response handling (inside
the function in packages/device/src/actions/poll.ts that currently throws
DeviceOAuthError("server_error", ...)) skips calling cleanUpPending(), leaving
stale pending auth; modify that branch to call cleanUpPending() (await it if
it's async) before throwing the DeviceOAuthError so pending state is always
cleared on terminal errors, keeping the existing message construction for the
thrown error.
In `@packages/device/src/shared/fetcher.ts`:
- Around line 16-23: The code doesn't respect pre-aborted caller signals and
leaks the abort listener; before attaching the listener check if
options.signal?.aborted and call controller.abort() immediately (so the internal
controller reflects a pre-aborted caller), and when you add the listener use a
named handler (e.g., const onAbort = () => controller.abort()) so you can remove
it in the finally block; also in the finally ensure you remove the
options.signal event listener (if one was added) and still
clearTimeout(timeoutId) to avoid leaks — update the listener handling around the
addEventListener call and the finally after the fetch invocation (references:
options.signal, controller, timeoutId, fetch).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44ea4367-647c-4b89-aa81-aad9b23f8d26
📒 Files selected for processing (17)
packages/device/package.jsonpackages/device/src/@types/config.tspackages/device/src/actions/authorize.tspackages/device/src/actions/poll.tspackages/device/src/actions/userinfo.tspackages/device/src/device-client.tspackages/device/src/index.tspackages/device/src/providers/index.tspackages/device/src/schemas.tspackages/device/src/shared/constants.tspackages/device/src/shared/errors.tspackages/device/src/shared/fetcher.tspackages/device/src/shared/url.tspackages/device/test/actions/authorize.test.tspackages/device/test/actions/poll.test.tspackages/device/test/providers.test.tspackages/device/vitest.config.ts
✅ Files skipped from review due to trivial changes (2)
- packages/device/vitest.config.ts
- packages/device/src/index.ts
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
1 participant
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.
Description
This pull request introduces the new
@aura-stack/devicepackage, adding support for the OAuth 2.0 Device Authorization Grant flow defined in RFC 8628.This flow enables OAuth authentication for devices and environments with limited input capabilities or without a traditional browser, such as:
The package provides the
createDeviceClientAPI for creating device authorization clients and includes a built-in GitHub OAuth device provider.Key Changes
@aura-stack/devicepackagecreateDeviceClientAPIUsage
Resources