Merged
Conversation
📝 WalkthroughWalkthroughReplaces hardcoded CLI suggestions with environment-aware commands (detecting npm exec/npx), adds utilities/tests for invocation formatting, switches installer credential checks to OAuth token presence, enhances device-auth polling with per-request timeouts and richer logging, and standardizes README examples to use Changes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Member
Author
nicknisi
changed the title
nicknisi
changed the title
Greptile SummaryThis PR addresses three installer auth reliability issues: (1) switches Confidence Score: 5/5This PR is safe to merge; all changes are well-scoped bug fixes and reliability improvements with matching tests. No P0 or P1 issues found. The AbortController timeout is properly cleaned up in a finally block, the getAccessToken() pivot is intentional and correct, the refresh token persistence fix is straightforward, and the command-invocation utility is cleanly designed and tested. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[workos install] --> B{WORKOS_API_KEY\nor --api-key?}
B -- yes --> Z[Enter installer core]
B -- no --> C{activeEnv\nhas apiKey?}
C -- yes --> D{isUnclaimed\nEnvironment?}
D -- yes --> Z
D -- no --> E{getAccessToken\nreturns value?}
E -- yes\nvalid token --> Z
E -- no\nexpired or missing --> F[authenticate]
F --> G{ensureAuthenticated}
G -- has refresh token --> H[refreshAccessToken]
H -- success --> Z
H -- fail / expired --> I[Device Auth Flow]
G -- no refresh token --> I
I --> J[pollForToken loop\nper-poll 30s AbortController]
J -- token received --> K[updateTokens\nincl. refreshToken NEW]
K --> Z
J -- timeout 5 min --> L[DeviceAuthError\nwith last poll summary]
Reviews (3): Last reviewed commit: "fix: tighten auth recovery guidance" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 8-13: The README contains inconsistent npx usage: replace all
occurrences of the older form "npx workos install" with the explicit "npx
workos@latest install" (and any other npm-exec examples that omit `@latest`) to
ensure users run the latest installer; search for the literal string "npx workos
install" (and variants) and update them to "npx workos@latest install", keeping
the existing npm global install examples unchanged except to append `@latest`
where applicable.
In `@src/lib/device-auth.ts`:
- Around line 149-154: The catch in the token-poll loop currently swallows every
exception and continues (using logInfo + continue), which hides hard failures;
change the logic inside the catch so only transient/network errors are retried
and everything else is rethrown or returned to surface the root cause.
Specifically, inside the polling logic around the token poll (the catch that
logs "[device-auth] Token poll network error, retrying:" and the later analogous
catch), detect retryable errors (e.g., network timeouts, ECONNRESET,
fetch/network-related exceptions or a bounded retry counter) and continue only
for those; for non-retryable errors (invalid_client, malformed response,
persistent auth errors, or other thrown Error types without network codes)
rethrow or return the error so it bubbles up instead of turning into the generic
timeout. Ensure you preserve and include the original error when rethrowing so
callers can see the root cause.
In `@src/lib/run-with-core.ts`:
- Line 244: Add a focused unit test to assert that saveCredentials is called
with the refreshToken obtained from the device-polling flow: mock or stub the
device polling flow (the code path that throws the "Not authenticated..." error
and the later device-poll success path) and verify that saveCredentials receives
the refreshToken returned by that polling before any further use; target the
functions/methods involved (saveCredentials and the device-polling handler/logic
invoked by runWithCore or the function that throws the Not authenticated error)
and add the same assertion for the equivalent device-polling branch referenced
around lines 386-392 to ensure both code paths persist the refresh token.
In `@src/utils/command-invocation.spec.ts`:
- Around line 9-15: Tests currently only cover the npm_command branch; add unit
tests that exercise the other detection branches by calling getWorkOSCommand and
formatWorkOSCommand with objects containing npm_execpath and
npm_config_user_agent values that should trigger the same invocation formatting.
Specifically, add one test calling getWorkOSCommand({ npm_execpath: '<tool>/npm'
}) and another with { npm_config_user_agent: 'npm/' } (or values that match your
detection logic) and corresponding formatWorkOSCommand assertions (e.g.,
formatWorkOSCommand('auth login', {...}) -> '<expected invocation> auth login')
to ensure those branches are covered; reference getWorkOSCommand and
formatWorkOSCommand when adding the tests.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 563d17b3-0ead-454a-9bd3-847e5782b891
📒 Files selected for processing (9)
README.mdsrc/lib/adapters/cli-adapter.spec.tssrc/lib/adapters/cli-adapter.tssrc/lib/device-auth.tssrc/lib/resolve-install-credentials.spec.tssrc/lib/resolve-install-credentials.tssrc/lib/run-with-core.tssrc/utils/command-invocation.spec.tssrc/utils/command-invocation.ts
| // This should rarely happen since bin.ts handles auth first | ||
| // But keep as safety net for programmatic usage | ||
| throw new Error('Not authenticated. Run `workos auth login` first.'); | ||
| throw new Error(`Not authenticated. Run \`${formatWorkOSCommand('auth login')}\` first.`); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Great recovery-path improvements (command hint + refresh-token persistence).
Both changes are high-value for auth resilience. Consider adding a focused test that asserts saveCredentials receives refreshToken from device polling, to lock this behavior in.
Also applies to: 386-392
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/run-with-core.ts` at line 244, Add a focused unit test to assert that
saveCredentials is called with the refreshToken obtained from the device-polling
flow: mock or stub the device polling flow (the code path that throws the "Not
authenticated..." error and the later device-poll success path) and verify that
saveCredentials receives the refreshToken returned by that polling before any
further use; target the functions/methods involved (saveCredentials and the
device-polling handler/logic invoked by runWithCore or the function that throws
the Not authenticated error) and add the same assertion for the equivalent
device-polling branch referenced around lines 386-392 to ensure both code paths
persist the refresh token.
Comment on lines
+9
to
+15
| it('uses npx workos@latest when launched by npm exec', () => { | ||
| expect(getWorkOSCommand({ npm_command: 'exec' })).toBe('npx workos@latest'); | ||
| }); | ||
|
|
||
| it('formats commands with the detected invocation', () => { | ||
| expect(formatWorkOSCommand('auth login', { npm_command: 'exec' })).toBe('npx workos@latest auth login'); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add tests for remaining detection branches.
getWorkOSCommand() also branches on npm_execpath and npm_config_user_agent; covering those would protect against regressions in non-npm_command environments.
✅ Additional branch coverage
describe('command invocation helpers', () => {
@@
it('uses npx workos@latest when launched by npm exec', () => {
expect(getWorkOSCommand({ npm_command: 'exec' })).toBe('npx workos@latest');
});
+
+ it('uses npx workos@latest when npm_execpath indicates npx', () => {
+ expect(getWorkOSCommand({ npm_execpath: '/usr/local/lib/node_modules/npm/bin/npx-cli.js' })).toBe(
+ 'npx workos@latest',
+ );
+ });
+
+ it('uses npx workos@latest when npm user agent indicates npx', () => {
+ expect(getWorkOSCommand({ npm_config_user_agent: 'npx/10.9.0 node/v20.0.0' })).toBe('npx workos@latest');
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/command-invocation.spec.ts` around lines 9 - 15, Tests currently
only cover the npm_command branch; add unit tests that exercise the other
detection branches by calling getWorkOSCommand and formatWorkOSCommand with
objects containing npm_execpath and npm_config_user_agent values that should
trigger the same invocation formatting. Specifically, add one test calling
getWorkOSCommand({ npm_execpath: '<tool>/npm' }) and another with {
npm_config_user_agent: 'npm/' } (or values that match your detection logic) and
corresponding formatWorkOSCommand assertions (e.g., formatWorkOSCommand('auth
login', {...}) -> '<expected invocation> auth login') to ensure those branches
are covered; reference getWorkOSCommand and formatWorkOSCommand when adding the
tests.
Member
Author
|
Addressed the PR review follow-up in
Validation after the follow-up:
The group-to-role mapping / dashboard-path guidance concern remains intentionally out of scope for this PR and will be handled next. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/commands/login.ts (2)
176-190:⚠️ Potential issue | 🔴 CriticalDevice-code polling can still block indefinitely on a hung token request.
Line 180 performs
fetchwithout a per-request abort/timeout. If that single call hangs, the loop never reaches the overall timeout check and the CLI can remain stuck at “Waiting for authentication...”.🔧 Suggested fix
const POLL_TIMEOUT_MS = 5 * 60 * 1000; // 5 minutes +const POLL_REQUEST_TIMEOUT_MS = 15_000; // 15 seconds per poll request @@ while (Date.now() - startTime < POLL_TIMEOUT_MS) { await sleep(currentInterval); try { - const tokenResponse = await fetch(endpoints.token, { - method: 'POST', - headers: { - 'Content-Type': 'application/x-www-form-urlencoded', - }, - body: new URLSearchParams({ - grant_type: 'urn:ietf:params:oauth:grant-type:device_code', - device_code: deviceAuth.device_code, - client_id: clientId, - }), - }); + const pollController = new AbortController(); + const pollTimeout = setTimeout(() => pollController.abort(), POLL_REQUEST_TIMEOUT_MS); + let tokenResponse: Response; + try { + tokenResponse = await fetch(endpoints.token, { + method: 'POST', + headers: { + 'Content-Type': 'application/x-www-form-urlencoded', + }, + body: new URLSearchParams({ + grant_type: 'urn:ietf:params:oauth:grant-type:device_code', + device_code: deviceAuth.device_code, + client_id: clientId, + }), + signal: pollController.signal, + }); + } finally { + clearTimeout(pollTimeout); + } const data = await tokenResponse.json(); @@ - } catch { + } catch (error) { + if (error instanceof Error && error.name === 'AbortError') { + logInfo(`[login] Poll request timed out after ${POLL_REQUEST_TIMEOUT_MS}ms; retrying`); + } continue; } }Also applies to: 241-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/login.ts` around lines 176 - 190, The polling loop can hang if a single fetch call to endpoints.token never returns; add a per-request timeout using AbortController so each POST to endpoints.token (inside the while loop that checks POLL_TIMEOUT_MS and uses sleep/currentInterval) is aborted after a bounded time (e.g., min(remainingTime, REQUEST_TIMEOUT_MS)), and handle the abort error path so the loop continues to the overall timeout; apply the same per-request abort/timeout fix to the other token request occurrence referenced (the fetch around lines 241-243) and ensure you reference deviceAuth.device_code and clientId when building the request so behavior is unchanged except for the timeout/abort handling.
226-226:⚠️ Potential issue | 🟡 MinorOne recovery hint still bypasses invocation-aware formatting.
Line 226 still hardcodes
workos env add. For npm-exec/npx sessions this can point users to a non-existent global binary; useformatWorkOSCommand('env add')here for consistency.🔧 Suggested fix
- clack.log.info(chalk.dim('Run `workos env add` to configure an environment manually')); + clack.log.info(chalk.dim(`Run \`${formatWorkOSCommand('env add')}\` to configure an environment manually`));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/login.ts` at line 226, The log message currently hardcodes "workos env add" which bypasses invocation-aware formatting; update the clack.log.info call that prints the recovery hint to use formatWorkOSCommand('env add') instead of the literal string so npm-exec/npx sessions get the correct executable form (keep the chalk.dim wrapper and existing message formatting when replacing the hardcoded command).src/lib/token-refresh-client.ts (1)
54-68:⚠️ Potential issue | 🟠 MajorRefresh timeout handle is not cleared on thrown fetch paths.
If
fetchthrows before Line 68, the timer created at Line 55 is left active. Clear it in afinallyblock around the fetch call.🔧 Suggested fix
try { const controller = new AbortController(); const timeout = setTimeout(() => controller.abort(), REFRESH_TIMEOUT_MS); - - const response = await fetch(tokenUrl, { - method: 'POST', - headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, - body: new URLSearchParams({ - grant_type: 'refresh_token', - refresh_token: creds.refreshToken, - client_id: clientId, - }), - signal: controller.signal, - }); - - clearTimeout(timeout); + let response: Response; + try { + response = await fetch(tokenUrl, { + method: 'POST', + headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, + body: new URLSearchParams({ + grant_type: 'refresh_token', + refresh_token: creds.refreshToken, + client_id: clientId, + }), + signal: controller.signal, + }); + } finally { + clearTimeout(timeout); + }Also applies to: 100-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/token-refresh-client.ts` around lines 54 - 68, The timeout created with AbortController in token-refresh-client.ts is not guaranteed to be cleared if fetch throws; wrap the fetch call that uses controller and timeout (the block creating controller = new AbortController(), setTimeout(...), and await fetch(...)) in a try/finally so clearTimeout(timeout) is called in the finally block and controller.signal is still used for the request; apply the same change to the other refresh block around lines handling the second fetch (the block affecting lines 100-108) so both request paths always clear their timers regardless of exceptions.src/commands/claim.ts (1)
118-119:⚠️ Potential issue | 🟡 MinorOne remaining hardcoded command still breaks npx/global consistency.
Line 119 still suggests
workos env listdirectly. This can reintroduce the same confusion this PR is fixing for invocation-specific guidance.Suggested patch
- clack.log.info('Complete the claim in your browser, then run `workos env list` to verify.'); + clack.log.info( + `Complete the claim in your browser, then run \`${formatWorkOSCommand('env list')}\` to verify.`, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/claim.ts` around lines 118 - 119, The message logged after spinner.stop currently hardcodes "workos env list", breaking invocation-specific guidance; update the clack.log.info call (near spinner.stop and clack.log.info) to build the command string using the project's invocation helper (the same utility used elsewhere in the codebase, e.g. getInvocation()/getInvocationCommand() or equivalent) instead of the literal "workos env list", and interpolate that generated command into the message "Complete the claim in your browser, then run `<invocation> env list` to verify." Ensure you use the existing helper function name used across the repo so the message matches npx/global invocation.
♻️ Duplicate comments (1)
src/lib/device-auth.ts (1)
153-158:⚠️ Potential issue | 🟠 MajorOnly retry transient poll failures; surface hard failures immediately.
Line 153 currently retries on every exception. This can hide non-retryable failures and degrade into a generic timeout at Line 199, delaying recovery and obscuring root cause.
🔧 Proposed fix
- } catch (error) { - logInfo( - '[device-auth] Token poll network error, retrying:', - error instanceof Error ? error.message : String(error), - ); - continue; + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + const isAbort = error instanceof Error && error.name === 'AbortError'; + + if (isAbort) { + logInfo('[device-auth] Token poll request timed out, retrying'); + lastPollSummary = `request_timeout after ${POLL_REQUEST_TIMEOUT_MS}ms`; + continue; + } + + logError('[device-auth] Token poll failed:', message); + throw new DeviceAuthError(`Token polling failed: ${message}`); } finally { clearTimeout(timeout); }Also applies to: 198-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/device-auth.ts` around lines 153 - 158, The catch in the token-polling routine is currently swallowing all exceptions (logging and continue) which hides non-retryable failures; update the catch blocks in the polling function (look for the token poll logic around the logInfo call and the continue statements in pollForDeviceToken / whatever function contains '[device-auth] Token poll network error, retrying:') to distinguish transient vs permanent errors: detect transient/network conditions (e.g., network/fetch errors, timeouts, HTTP 5xx/429) and only continue/retry for those, but for non-transient errors (HTTP 4xx client errors, malformed responses, authentication failures) rethrow or return the error immediately so it surfaces instead of looping; apply the same change to the second catch at the later polling spot (the block referenced at lines 198-201) so both handlers follow the same retry-vs-fail logic and use logInfo/logError appropriately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/commands/claim.ts`:
- Around line 118-119: The message logged after spinner.stop currently hardcodes
"workos env list", breaking invocation-specific guidance; update the
clack.log.info call (near spinner.stop and clack.log.info) to build the command
string using the project's invocation helper (the same utility used elsewhere in
the codebase, e.g. getInvocation()/getInvocationCommand() or equivalent) instead
of the literal "workos env list", and interpolate that generated command into
the message "Complete the claim in your browser, then run `<invocation> env
list` to verify." Ensure you use the existing helper function name used across
the repo so the message matches npx/global invocation.
In `@src/commands/login.ts`:
- Around line 176-190: The polling loop can hang if a single fetch call to
endpoints.token never returns; add a per-request timeout using AbortController
so each POST to endpoints.token (inside the while loop that checks
POLL_TIMEOUT_MS and uses sleep/currentInterval) is aborted after a bounded time
(e.g., min(remainingTime, REQUEST_TIMEOUT_MS)), and handle the abort error path
so the loop continues to the overall timeout; apply the same per-request
abort/timeout fix to the other token request occurrence referenced (the fetch
around lines 241-243) and ensure you reference deviceAuth.device_code and
clientId when building the request so behavior is unchanged except for the
timeout/abort handling.
- Line 226: The log message currently hardcodes "workos env add" which bypasses
invocation-aware formatting; update the clack.log.info call that prints the
recovery hint to use formatWorkOSCommand('env add') instead of the literal
string so npm-exec/npx sessions get the correct executable form (keep the
chalk.dim wrapper and existing message formatting when replacing the hardcoded
command).
In `@src/lib/token-refresh-client.ts`:
- Around line 54-68: The timeout created with AbortController in
token-refresh-client.ts is not guaranteed to be cleared if fetch throws; wrap
the fetch call that uses controller and timeout (the block creating controller =
new AbortController(), setTimeout(...), and await fetch(...)) in a try/finally
so clearTimeout(timeout) is called in the finally block and controller.signal is
still used for the request; apply the same change to the other refresh block
around lines handling the second fetch (the block affecting lines 100-108) so
both request paths always clear their timers regardless of exceptions.
---
Duplicate comments:
In `@src/lib/device-auth.ts`:
- Around line 153-158: The catch in the token-polling routine is currently
swallowing all exceptions (logging and continue) which hides non-retryable
failures; update the catch blocks in the polling function (look for the token
poll logic around the logInfo call and the continue statements in
pollForDeviceToken / whatever function contains '[device-auth] Token poll
network error, retrying:') to distinguish transient vs permanent errors: detect
transient/network conditions (e.g., network/fetch errors, timeouts, HTTP
5xx/429) and only continue/retry for those, but for non-transient errors (HTTP
4xx client errors, malformed responses, authentication failures) rethrow or
return the error immediately so it surfaces instead of looping; apply the same
change to the second catch at the later polling spot (the block referenced at
lines 198-201) so both handlers follow the same retry-vs-fail logic and use
logInfo/logError appropriately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7042bd57-c7ed-4190-931e-b0f3c09a52f0
📒 Files selected for processing (12)
README.mdsrc/commands/auth-status.tssrc/commands/claim.tssrc/commands/login.tssrc/doctor/checks/ai-analysis.tssrc/lib/agent-interface.tssrc/lib/credential-proxy.tssrc/lib/device-auth.tssrc/lib/ensure-auth.tssrc/lib/token-refresh-client.tssrc/lib/token-refresh.tssrc/utils/exit-codes.ts
nicknisi
force-pushed
the
nicknisi/cus-feedback
branch
from
2a9adff to
d15d8a3
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/utils/command-invocation.spec.ts (1)
9-15: 🧹 Nitpick | 🔵 TrivialAdd branch tests for
npm_execpathandnpm_config_user_agentdetection paths.Current coverage validates only the
npm_commandbranch; please add explicit tests for the other invocation-detection inputs to prevent regressions.✅ Suggested test additions
describe('command invocation helpers', () => { @@ it('uses npx workos@latest when launched by npm exec', () => { expect(getWorkOSCommand({ npm_command: 'exec' })).toBe('npx workos@latest'); }); + + it('uses npx workos@latest when npm_execpath indicates npx', () => { + const env = { npm_execpath: '/usr/local/lib/node_modules/npm/bin/npx-cli.js' }; + expect(getWorkOSCommand(env)).toBe('npx workos@latest'); + expect(formatWorkOSCommand('auth login', env)).toBe('npx workos@latest auth login'); + }); + + it('uses npx workos@latest when npm_config_user_agent indicates npx', () => { + const env = { npm_config_user_agent: 'npx/10.9.0 node/v20.0.0' }; + expect(getWorkOSCommand(env)).toBe('npx workos@latest'); + expect(formatWorkOSCommand('auth login', env)).toBe('npx workos@latest auth login'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/command-invocation.spec.ts` around lines 9 - 15, Tests only cover the npm_command branch; add tests that assert getWorkOSCommand and formatWorkOSCommand behave correctly when invocation is detected via npm_execpath and npm_config_user_agent (e.g., simulate process info objects containing npm_execpath pointing to a node_modules/.bin/npx path and npm_config_user_agent containing an npm exec token), creating expectations analogous to the existing 'npm exec' tests so both getWorkOSCommand(...) and formatWorkOSCommand('auth login', ...) return 'npx workos@latest' and 'npx workos@latest auth login' respectively for those detection inputs.src/lib/device-auth.ts (1)
153-161:⚠️ Potential issue | 🟠 MajorCatch block still retries on all errors, not just timeouts.
The previous review suggested only continuing on
AbortError(per-request timeout) and throwing on other errors. The current code still catches all errors and continues, which means persistent failures (invalid config, server errors, etc.) will be silently retried until the 5-minute overall timeout, obscuring the root cause.🐛 Proposed fix: Distinguish AbortError from other failures
} catch (error) { + const isAbort = error instanceof Error && error.name === 'AbortError'; + if (!isAbort) { + logError( + '[device-auth] Token poll failed:', + error instanceof Error ? error.message : String(error), + ); + throw new DeviceAuthError( + `Token polling failed: ${error instanceof Error ? error.message : String(error)}`, + ); + } logInfo( - '[device-auth] Token poll network error, retrying:', + '[device-auth] Token poll request timed out, retrying:', error instanceof Error ? error.message : String(error), ); continue; } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/device-auth.ts` around lines 153 - 161, The catch in the token-poll loop is currently swallowing all errors; change it so only per-request timeouts (AbortError) are treated as retryable: inside the catch, detect timeout errors (e.g., error.name === 'AbortError' or instanceof AbortError) and call logInfo + continue for those, but rethrow non-timeout errors so they surface immediately; keep the existing clearTimeout(timeout) in the finally block. Target the token-polling catch block that uses logInfo and the timeout variable in src/lib/device-auth.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/device-auth.ts`:
- Around line 153-161: The catch in the token-poll loop is currently swallowing
all errors; change it so only per-request timeouts (AbortError) are treated as
retryable: inside the catch, detect timeout errors (e.g., error.name ===
'AbortError' or instanceof AbortError) and call logInfo + continue for those,
but rethrow non-timeout errors so they surface immediately; keep the existing
clearTimeout(timeout) in the finally block. Target the token-polling catch block
that uses logInfo and the timeout variable in src/lib/device-auth.ts.
In `@src/utils/command-invocation.spec.ts`:
- Around line 9-15: Tests only cover the npm_command branch; add tests that
assert getWorkOSCommand and formatWorkOSCommand behave correctly when invocation
is detected via npm_execpath and npm_config_user_agent (e.g., simulate process
info objects containing npm_execpath pointing to a node_modules/.bin/npx path
and npm_config_user_agent containing an npm exec token), creating expectations
analogous to the existing 'npm exec' tests so both getWorkOSCommand(...) and
formatWorkOSCommand('auth login', ...) return 'npx workos@latest' and 'npx
workos@latest auth login' respectively for those detection inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5e9d9257-776e-4e13-ba88-23daf7d5d378
📒 Files selected for processing (19)
README.mdsrc/commands/auth-status.tssrc/commands/claim.tssrc/commands/login.tssrc/doctor/checks/ai-analysis.tssrc/lib/adapters/cli-adapter.spec.tssrc/lib/adapters/cli-adapter.tssrc/lib/agent-interface.tssrc/lib/credential-proxy.tssrc/lib/device-auth.tssrc/lib/ensure-auth.tssrc/lib/resolve-install-credentials.spec.tssrc/lib/resolve-install-credentials.tssrc/lib/run-with-core.tssrc/lib/token-refresh-client.tssrc/lib/token-refresh.tssrc/utils/command-invocation.spec.tssrc/utils/command-invocation.tssrc/utils/exit-codes.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.
Summary
Addresses installer-auth failure modes surfaced in customer feedback:
Waiting for authentication....npx workos@latest ...when launched via npm exec/npx.npx workos@latest install.Why
The Slack thread showed three related pain points: docs/global-binary confusion, browser auth succeeding while the CLI continued waiting, and unclear recovery commands that could hit an older global binary. This PR fixes the CLI-side reliability and messaging issues. The group-to-role mapping concern remains a product/discoverability gap rather than a CLI auth bug.
Validation
pnpm test src/lib/resolve-install-credentials.spec.ts src/lib/adapters/cli-adapter.spec.ts src/utils/command-invocation.spec.tspnpm typecheckSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests