feat: agent-friendly login flow (--request / --check)#7971
feat: agent-friendly login flow (--request / --check)#7971
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces a ticket-based authentication flow for the Netlify CLI login command. It exposes the OAuth CLIENT_ID as a public export, adds a storeToken helper function for persisting user data, and implements two new functions: loginRequest (initiates ticket-based login with an authorization URL) and loginCheck (verifies and exchanges the ticket for credentials). The login command is extended with --request and --check options to route between these new authentication paths while preserving the existing OAuth flow. The CLI help text is updated with login guidance, and comprehensive unit tests are provided for the new functions. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
📊 Benchmark resultsComparing with bd7f95b
|
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/base-command.ts`:
- Around line 178-196: The storeToken function accepts an optional userId which
can lead to writing invalid config keys like "users.undefined"; change the
parameter type for userId to be required (e.g., in the destructured param object
for storeToken make userId: string rather than userId?: string) so callers must
provide a valid string ID, and update any call sites if necessary to ensure they
pass a concrete userId (referencing the storeToken symbol to locate the change).
In `@src/commands/login/login-check.ts`:
- Around line 16-21: The catch currently treats any error from api.showTicket()
as an auth denial; change it to distinguish APIError transport/server failures
by checking the error type (e.g., instanceof APIError) and inspecting
error.status: map only auth-related statuses (401, 404, etc.) to the existing
denied flow (call logJson({status: 'denied'}) and log('Status: denied')), but
for other statuses or non-APIError exceptions rethrow or surface the error (so
network/server failures are not masked). Update the catch around showTicket to
perform this conditional handling while keeping the denied branch for the
specific status codes and preserving existing logging functions (logJson, log).
- Line 1: Remove the unused import "process" from login-check.ts and add
defensive null/undefined checks around the API response fields before
type-asserting or passing them to storeToken: verify response.data.access_token
exists before using it (the variable referenced as access_token) and verify
response.data.user and its properties (user.name, user.email) exist before
accessing or passing them to storeToken; update the code around the
functions/variables named access_token, user, and storeToken to only call
storeToken with the token and user fields when they are non-null, and handle the
error path (e.g., return or throw) when required fields are missing to avoid
unsafe type assertions.
In `@src/commands/login/login-request.ts`:
- Line 9: The loginRequest function currently ignores the required CLI
parameter; update loginRequest(options: OptionValues) to read and validate
options.request (or options['request']) and either include its value in the
login flow (e.g., attach it to the login payload, include it in the
output/logging, or pass it to the downstream request handler) or, if you prefer
the flag to be non-parameterized, change the flag definition instead; locate the
loginRequest function and ensure you reference the unique symbol
options.request, add a presence check and a clear error or usage message when
missing, and wire the value into wherever the function sends/prints the login
request so the provided message is actually consumed.
- Around line 14-23: api.createTicket() returns a ticket whose id is optional,
but the code uses ticket.id directly in string interpolation for url, logJson,
and log calls; add a guard after the await (e.g., if (!ticket?.id) { throw new
Error("Failed to create ticket: missing id"); }) or handle the undefined case by
logging an error and exiting, then use ticket.id safely afterwards when building
url, check_command, and calling logJson/log; update references in this block
around the createTicket call, the url construction, logJson, and subsequent
log() invocations to rely on the validated ticket.id.
In `@src/commands/login/login.ts`:
- Around line 21-31: The CLI currently silently prefers options.request when
both options.request and options.check are passed; add an explicit validation at
the start of the handler to detect if both options.request and options.check are
true and return a clear error (e.g., throw an Error or use the CLI's error
reporting) before importing or calling loginRequest/loginCheck; ensure this
check runs before the existing branching so neither loginRequest nor loginCheck
is executed when both flags are present and include a concise message like
"`--request` and `--check` are mutually exclusive".
In `@tests/unit/commands/login/login-request.test.ts`:
- Around line 16-25: The test declares stdoutSpy with a too-broad type; change
its type to vitest's MockInstance parameterized for process.stdout.write and
import MockInstance from 'vitest' so the chained .mockImplementation is
type-safe: update the stdoutSpy declaration to use
MockInstance<Parameters<typeof process.stdout.write>, ReturnType<typeof
process.stdout.write>> (or equivalent) and add the MockInstance import, leaving
stdoutOutput and the vi.spyOn(process.stdout, 'write') usage unchanged so the
mockImplementation call type-checks.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
src/commands/base-command.tssrc/commands/login/index.tssrc/commands/login/login-check.tssrc/commands/login/login-request.tssrc/commands/login/login.tssrc/commands/main.tstests/unit/commands/login/login-check.test.tstests/unit/commands/login/login-request.test.ts
| try { | ||
| ticket = await api.showTicket({ ticketId }) | ||
| } catch { | ||
| logJson({ status: 'denied' }) | ||
| log('Status: denied') | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/commands/login/login-check.ts | head -40Repository: netlify/cli
Length of output: 1434
🏁 Script executed:
fd -t f "node_modules/@netlify/api" -o "package.json" | head -5Repository: netlify/cli
Length of output: 198
🏁 Script executed:
rg "showTicket" --type ts --type tsx -A 5 -B 5 | head -50Repository: netlify/cli
Length of output: 82
🏁 Script executed:
rg "showTicket" --type ts -A 5 -B 5Repository: netlify/cli
Length of output: 4326
🏁 Script executed:
cat package.json | jq '.dependencies."@netlify/api"' 2>/dev/null || grep '@netlify/api' package.jsonRepository: netlify/cli
Length of output: 64
🏁 Script executed:
cat package.json | grep -A 5 -B 5 '"@netlify/api"'Repository: netlify/cli
Length of output: 371
🏁 Script executed:
cat -n tests/unit/commands/login/login-check.test.ts | head -80Repository: netlify/cli
Length of output: 2909
🏁 Script executed:
rg "showTicket" --type ts -B 3 -A 10 | grep -A 10 "catch"Repository: netlify/cli
Length of output: 583
🏁 Script executed:
rg "NetlifyAPI" --type ts -A 20 | head -100Repository: netlify/cli
Length of output: 6503
🏁 Script executed:
rg "APIError" --type ts -B 3 -A 3Repository: netlify/cli
Length of output: 18132
🏁 Script executed:
cat -n src/utils/command-helpers.ts | grep -A 10 -B 5 "APIError"Repository: netlify/cli
Length of output: 662
Distinguish auth/ticket errors from transport/server failures in error handling.
The catch block currently treats all errors from showTicket() as denied, including network failures and server errors. Use the status property of APIError to map only specific status codes (like 404 for not found, 401 for unauthorized) to denied, and rethrow or handle other errors appropriately. This prevents masking server/transport failures and allows for proper error diagnosis.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/login/login-check.ts` around lines 16 - 21, The catch currently
treats any error from api.showTicket() as an auth denial; change it to
distinguish APIError transport/server failures by checking the error type (e.g.,
instanceof APIError) and inspecting error.status: map only auth-related statuses
(401, 404, etc.) to the existing denied flow (call logJson({status: 'denied'})
and log('Status: denied')), but for other statuses or non-APIError exceptions
rethrow or surface the error (so network/server failures are not masked). Update
the catch around showTicket to perform this conditional handling while keeping
the denied branch for the specific status codes and preserving existing logging
functions (logJson, log).
src/commands/login/login-request.ts
Outdated
| import { log, logJson, USER_AGENT } from '../../utils/command-helpers.js' | ||
| import { CLIENT_ID } from '../base-command.js' | ||
|
|
||
| export const loginRequest = async (options: OptionValues) => { |
There was a problem hiding this comment.
--request <message> is required but currently ignored.
Line 9 accepts a required message, but the flow does not use it. Either consume it (e.g., include it in output) or make the flag non-parameterized.
🧰 Tools
🪛 GitHub Check: Lint
[failure] 9-9:
'options' is defined but never used. Allowed unused args must match /^_/u
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/login/login-request.ts` at line 9, The loginRequest function
currently ignores the required CLI parameter; update loginRequest(options:
OptionValues) to read and validate options.request (or options['request']) and
either include its value in the login flow (e.g., attach it to the login
payload, include it in the output/logging, or pass it to the downstream request
handler) or, if you prefer the flag to be non-parameterized, change the flag
definition instead; locate the loginRequest function and ensure you reference
the unique symbol options.request, add a presence check and a clear error or
usage message when missing, and wire the value into wherever the function
sends/prints the login request so the provided message is actually consumed.
| let stdoutSpy: ReturnType<typeof vi.spyOn> | ||
| let stdoutOutput: string[] | ||
| const originalEnv = { ...process.env } | ||
|
|
||
| beforeEach(() => { | ||
| stdoutOutput = [] | ||
| stdoutSpy = vi.spyOn(process.stdout, 'write').mockImplementation((chunk) => { | ||
| stdoutOutput.push(chunk.toString()) | ||
| return true | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the test file
find . -type f -name "login-request.test.ts" | head -20Repository: netlify/cli
Length of output: 104
🏁 Script executed:
# Check the repository structure
git ls-files | grep -E "tests/unit/commands/login" | head -20Repository: netlify/cli
Length of output: 148
🏁 Script executed:
# List the tests/unit/commands directory
ls -la tests/unit/commands/ 2>/dev/null || echo "Directory not found"Repository: netlify/cli
Length of output: 217
🏁 Script executed:
# Read the test file to verify the type annotation
cat -n ./tests/unit/commands/login/login-request.test.ts | head -40Repository: netlify/cli
Length of output: 1531
🏁 Script executed:
# Check TypeScript configuration
cat -n tsconfig.build.json 2>/dev/null || echo "File not found"Repository: netlify/cli
Length of output: 342
🏁 Script executed:
# Look for other similar spy patterns in test files
rg "vi\.spyOn\(process\.stdout" tests/unit --type ts -A 2 -B 1Repository: netlify/cli
Length of output: 761
🏁 Script executed:
# Check if there's a package.json build script
cat package.json | grep -A 5 '"build"' | head -15Repository: netlify/cli
Length of output: 326
🏁 Script executed:
# Check for vitest configuration
cat vitest.config.ts 2>/dev/null | head -30 || echo "Not found as .ts"Repository: netlify/cli
Length of output: 889
🏁 Script executed:
# Check for vitest config in package.json
cat package.json | grep -A 10 '"vitest"'Repository: netlify/cli
Length of output: 345
🏁 Script executed:
# Check if there's a separate tsconfig for tests
ls -la tsconfig*.jsonRepository: netlify/cli
Length of output: 251
🏁 Script executed:
# Check the base tsconfig to see full configuration
cat -n tsconfig.json | head -50Repository: netlify/cli
Length of output: 115
🏁 Script executed:
# Check the base tsconfig configuration
cat -n tsconfig.base.jsonRepository: netlify/cli
Length of output: 487
🏁 Script executed:
# Check all npm scripts
cat package.json | grep -A 50 '"scripts"' | head -60Repository: netlify/cli
Length of output: 2513
🏁 Script executed:
# Check if there's a type-check or tsc script
grep -E '"(type.?check|tsc)"' package.jsonRepository: netlify/cli
Length of output: 78
🏁 Script executed:
# Look at the actual return type of vi.spyOn in vitest types
# First check what types are available
rg "spyOn.*process\.stdout" --type ts -B 2 -A 5 | head -50Repository: netlify/cli
Length of output: 1205
🏁 Script executed:
# Check node_modules for vitest types
find node_modules/vitest -name "*.d.ts" 2>/dev/null | grep -i spy | head -10Repository: netlify/cli
Length of output: 37
🏁 Script executed:
# Search for how vi.spyOn is typed in vitest
rg "spyOn" node_modules/vitest/dist --type ts -B 2 -A 2 2>/dev/null | head -50Repository: netlify/cli
Length of output: 37
🏁 Script executed:
# Look for other test files that use vi.spyOn to see patterns
rg "vi\.spyOn\(" tests/ --type ts -A 3 -B 1 | head -100Repository: netlify/cli
Length of output: 5225
🏁 Script executed:
# Check if there are type annotations on similar spy variables
rg "let.*Spy.*=.*vi\.spyOn" tests/ --type ts -B 1 | head -50Repository: netlify/cli
Length of output: 37
🏁 Script executed:
# Look for how type annotations are handled in other test files
rg "let.*:\s*" tests/unit/lib/exec-fetcher.test.ts --type ts -B 0 -A 0 | head -20Repository: netlify/cli
Length of output: 182
🏁 Script executed:
# Check the exec-fetcher test file for comparison
cat -n tests/unit/lib/exec-fetcher.test.ts | head -30Repository: netlify/cli
Length of output: 1112
🏁 Script executed:
# Check how MockInstance is used in other test files
rg "MockInstance" tests/unit --type ts -B 1 -A 1 | head -50Repository: netlify/cli
Length of output: 580
🏁 Script executed:
# Look at the registry test file to see the full pattern with mockImplementation
cat -n tests/unit/lib/functions/registry.test.ts | head -50Repository: netlify/cli
Length of output: 1928
🏁 Script executed:
# Check what type MockInstance should be parameterized with for process.stdout.write
rg "process\.stdout" tests/unit/commands/login/login-request.test.ts -B 5 -A 10Repository: netlify/cli
Length of output: 414
🏁 Script executed:
# Look for other uses of MockInstance in the codebase
rg "MockInstance" tests/ --type ts -B 2 -A 2 | head -100Repository: netlify/cli
Length of output: 838
🏁 Script executed:
# Check if there's a .eslintrc or similar that might have type checking rules
ls -la | grep -E "eslint|tsconfig"Repository: netlify/cli
Length of output: 417
🏁 Script executed:
# Try to understand what the proper type should be - look at function registry test more
cat -n tests/unit/lib/functions/registry.test.ts | sed -n '40,90p'Repository: netlify/cli
Length of output: 2377
🏁 Script executed:
# Try to understand the correct type for process.stdout.write spy
# Search for MockInstance usage with write-like methods
rg "MockInstance.*write\|write.*MockInstance" tests/ --type tsRepository: netlify/cli
Length of output: 37
🏁 Script executed:
# Check if there are any type errors in the current test file
# by looking for `@ts-expect-error` patterns in similar test files
rg "@ts-expect-error" tests/unit/commands/login --type tsRepository: netlify/cli
Length of output: 37
🏁 Script executed:
# Look at another test file that uses mockImplementation to understand the pattern
cat -n tests/unit/lib/completion/generate-autocompletion.test.ts | head -40Repository: netlify/cli
Length of output: 1583
🏁 Script executed:
# Verify the correct MockInstance syntax by checking if it's already imported elsewhere
rg "import.*MockInstance.*from.*vitest" tests/ --type tsRepository: netlify/cli
Length of output: 183
🏁 Script executed:
# Check if login-request.test.ts or login-check.test.ts already imports MockInstance
cat -n tests/unit/commands/login/login-request.test.ts | head -15Repository: netlify/cli
Length of output: 590
Use MockInstance type for stdoutSpy instead of generic ReturnType.
Line 16's type annotation is too broad. Import MockInstance from vitest and parameterize it with the correct signature for process.stdout.write:
Suggested fix
-import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'
+import { afterEach, beforeEach, describe, expect, test, vi, type MockInstance } from 'vitest'
describe('loginRequest', () => {
- let stdoutSpy: ReturnType<typeof vi.spyOn>
+ let stdoutSpy: MockInstance<(chunk: any) => boolean>This will fail npm run typecheck (which runs tsc on all source files including tests) due to type incompatibility with the chained .mockImplementation() on line 22.
🧰 Tools
🪛 GitHub Check: typecheck
[failure] 22-22:
Type 'MockInstance<{ (buffer: string | Uint8Array, cb?: ((err?: Error | null | undefined) => void) | undefined): boolean; (str: string | Uint8Array<...>, encoding?: BufferEncoding | undefined, cb?: ((err?: Error | ... 1 more ... | undefined) => void) | undefined): boolean; }>' is not assignable to type 'MockInstance<(this: unknown, ...args: unknown[]) => unknown>'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/commands/login/login-request.test.ts` around lines 16 - 25, The
test declares stdoutSpy with a too-broad type; change its type to vitest's
MockInstance parameterized for process.stdout.write and import MockInstance from
'vitest' so the chained .mockImplementation is type-safe: update the stdoutSpy
declaration to use MockInstance<Parameters<typeof process.stdout.write>,
ReturnType<typeof process.stdout.write>> (or equivalent) and add the
MockInstance import, leaving stdoutOutput and the vi.spyOn(process.stdout,
'write') usage unchanged so the mockImplementation call type-checks.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/unit/commands/login/login-request.test.ts (1)
16-22:⚠️ Potential issue | 🔴 Critical
stdoutSpyis typed too broadly and breaks typecheck here as well.Use a concrete Vitest
MockInstancetype forprocess.stdout.writeto keep the spy assignment type-safe.Proposed fix
-import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, test, vi, type MockInstance } from 'vitest' @@ - let stdoutSpy: ReturnType<typeof vi.spyOn> + let stdoutSpy: MockInstance<typeof process.stdout.write>As per coding guidelines
**/*.ts: “Compiles TypeScript using `tsc --project tsconfig.build.json` with `npm run build`”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/commands/login/login-request.test.ts` around lines 16 - 22, The test currently types stdoutSpy too broadly (ReturnType<typeof vi.spyOn>) causing TypeScript errors; change the declaration of stdoutSpy to use Vitest's concrete MockInstance type for process.stdout.write (e.g., MockInstance<typeof process.stdout.write>) and update the assignment in beforeEach to match that type so the spy/mocked implementation is type-safe; locate the variable named stdoutSpy and the vi.spyOn(process.stdout, 'write') call to apply the type change.src/commands/login/login-request.ts (1)
13-21:⚠️ Potential issue | 🟠 MajorFail fast when ticket creation returns no ID.
Using
''as fallback emits invalidauthorize/--checkoutput instead of surfacing the failure. Guard and throw before building the URL/commands.Proposed fix
- const ticketId = ticket.id ?? '' + if (!ticket.id) { + throw new Error('Could not create login ticket: missing ticket id') + } + const ticketId = ticket.id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/login/login-request.ts` around lines 13 - 21, The code currently falls back to an empty string for ticketId (ticket.id ?? '') which produces invalid URLs/commands; instead, in the function that handles ticket creation, validate that ticket.id exists and if not throw an error (e.g., "Failed to create login ticket") before constructing url or check commands. Replace the fallback logic around ticket.id with an explicit guard that throws when ticket.id is falsy, then use the validated ticketId to build url (`${webUI}/authorize?response_type=ticket&ticket=${ticketId}`), logJson({ ticket_id: ticketId, url, check_command: `netlify login --check ${ticketId}` }) and the subsequent log(...) calls.
🤖 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/commands/login/login-request.ts`:
- Around line 13-21: The code currently falls back to an empty string for
ticketId (ticket.id ?? '') which produces invalid URLs/commands; instead, in the
function that handles ticket creation, validate that ticket.id exists and if not
throw an error (e.g., "Failed to create login ticket") before constructing url
or check commands. Replace the fallback logic around ticket.id with an explicit
guard that throws when ticket.id is falsy, then use the validated ticketId to
build url (`${webUI}/authorize?response_type=ticket&ticket=${ticketId}`),
logJson({ ticket_id: ticketId, url, check_command: `netlify login --check
${ticketId}` }) and the subsequent log(...) calls.
In `@tests/unit/commands/login/login-request.test.ts`:
- Around line 16-22: The test currently types stdoutSpy too broadly
(ReturnType<typeof vi.spyOn>) causing TypeScript errors; change the declaration
of stdoutSpy to use Vitest's concrete MockInstance type for process.stdout.write
(e.g., MockInstance<typeof process.stdout.write>) and update the assignment in
beforeEach to match that type so the spy/mocked implementation is type-safe;
locate the variable named stdoutSpy and the vi.spyOn(process.stdout, 'write')
call to apply the type change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/commands/login/login-check.tssrc/commands/login/login-request.tssrc/commands/login/login.tstests/unit/commands/login/login-check.test.tstests/unit/commands/login/login-request.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/commands/login/login-check.ts
- src/commands/login/login.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/commands/login/login-request.test.ts (1)
33-51: Add one assertion for the JSON contract emitted byloginRequest.These tests cover plain text well, but they don’t explicitly validate the JSON payload shape (
ticket_id,url,check_command), which is a key behavior in this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/commands/login/login-request.test.ts` around lines 33 - 51, Add an assertion that parses the JSON payload emitted by loginRequest and verifies the contract fields { ticket_id, url, check_command } match expected values; specifically, after calling loginRequest() and capturing stdoutOutput, find and JSON.parse the JSON line (or parse the full output if JSON-only), then assert parsed.ticket_id === 'test-ticket-123', parsed.url === 'https://app.netlify.com/authorize?response_type=ticket&ticket=test-ticket-123' (and when NETLIFY_WEB_UI is set, expect the custom host), and parsed.check_command === 'netlify login --check test-ticket-123' to ensure the JSON shape produced by loginRequest is validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/commands/blobs.md`:
- Around line 64-65: Update the `force` flag description to use sentence case
and correct verb agreement: replace “Bypasses prompts & Force the command to
run.” with “Bypasses prompts and forces the command to run.”; ensure this change
is applied to the `force` flag entry in docs/commands/blobs.md so the help text
reads clearly and grammatically.
In `@docs/commands/login.md`:
- Around line 24-27: Update the command description that currently states login
is browser-only to indicate that browser login is the default but not required;
modify the sentence near the top of docs/commands/login.md to note that
--request/--check enable non-browser (agent/HITL) login flows and that the
browser is the default method. Keep references to the existing flags (--request,
--check, --new, --json) intact and ensure the wording clarifies “browser
default” rather than “browser required.”
---
Nitpick comments:
In `@tests/unit/commands/login/login-request.test.ts`:
- Around line 33-51: Add an assertion that parses the JSON payload emitted by
loginRequest and verifies the contract fields { ticket_id, url, check_command }
match expected values; specifically, after calling loginRequest() and capturing
stdoutOutput, find and JSON.parse the JSON line (or parse the full output if
JSON-only), then assert parsed.ticket_id === 'test-ticket-123', parsed.url ===
'https://app.netlify.com/authorize?response_type=ticket&ticket=test-ticket-123'
(and when NETLIFY_WEB_UI is set, expect the custom host), and
parsed.check_command === 'netlify login --check test-ticket-123' to ensure the
JSON shape produced by loginRequest is validated.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (22)
docs/commands/agents.mddocs/commands/api.mddocs/commands/blobs.mddocs/commands/build.mddocs/commands/clone.mddocs/commands/completion.mddocs/commands/db.mddocs/commands/dev.mddocs/commands/env.mddocs/commands/functions.mddocs/commands/init.mddocs/commands/link.mddocs/commands/login.mddocs/commands/logs.mddocs/commands/open.mddocs/commands/recipes.mddocs/commands/sites.mddocs/commands/status.mddocs/commands/unlink.mddocs/commands/watch.mdtests/unit/commands/login/login-check.test.tstests/unit/commands/login/login-request.test.ts
✅ Files skipped from review due to trivial changes (5)
- docs/commands/open.md
- docs/commands/api.md
- docs/commands/sites.md
- docs/commands/unlink.md
- docs/commands/recipes.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/commands/login/login-check.test.ts
| - `filter` (*string*) - For monorepos, specify the name of the application to run the command in | ||
| - `force` (*boolean*) - Bypasses prompts & Force the command to run. |
There was a problem hiding this comment.
Fix flag description grammar for --force.
Line 65 has awkward wording: “Bypasses prompts & Force the command to run.” Please use sentence case and verb agreement (for example, “Bypasses prompts and forces the command to run.”).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/commands/blobs.md` around lines 64 - 65, Update the `force` flag
description to use sentence case and correct verb agreement: replace “Bypasses
prompts & Force the command to run.” with “Bypasses prompts and forces the
command to run.”; ensure this change is applied to the `force` flag entry in
docs/commands/blobs.md so the help text reads clearly and grammatically.
| - `check` (*string*) - Check the status of a login ticket created with --request | ||
| - `json` (*boolean*) - Output as JSON (for use with --request or --check) | ||
| - `new` (*boolean*) - Login to new Netlify account | ||
| - `request` (*string*) - Create a login ticket for agent/human-in-the-loop auth |
There was a problem hiding this comment.
Update the command description to reflect non-browser login support.
With --request/--check now documented, the browser-only statement is outdated. Please adjust the text at Line 12 so it indicates browser login is default, not required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/commands/login.md` around lines 24 - 27, Update the command description
that currently states login is browser-only to indicate that browser login is
the default but not required; modify the sentence near the top of
docs/commands/login.md to note that --request/--check enable non-browser
(agent/HITL) login flows and that the browser is the default method. Keep
references to the existing flags (--request, --check, --new, --json) intact and
ensure the wording clarifies “browser default” rather than “browser required.”
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/commands/login/login-request.ts (1)
6-19:⚠️ Potential issue | 🟠 Major
--request <message>value is still discarded.
loginRequest(Line 6) doesn’t accept/use the message, so the required CLI argument has no effect. Either wire the value through this flow or make--requestnon-parameterized.Proposed direction
// src/commands/login/login.ts - await loginRequest() + await loginRequest(String(options.request)) // src/commands/login/login-request.ts -export const loginRequest = async () => { +export const loginRequest = async (requestMessage: string) => { ... - logJson({ ticket_id: ticketId, url, check_command: `netlify login --check ${ticketId}` }) + logJson({ ticket_id: ticketId, request_message: requestMessage, url, check_command: `netlify login --check ${ticketId}` })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/login/login-request.ts` around lines 6 - 19, The loginRequest function currently ignores the CLI's --request value; modify loginRequest to accept a requestMessage (string | undefined) parameter, pass it into the Netlify API ticket creation call (e.g., api.createTicket({ clientId: CLIENT_ID, message: requestMessage })) and include the requestMessage in the logJson output (e.g., request_message: requestMessage) so the CLI-provided message is preserved through the flow; update references to ticket creation (createTicket) and the logJson call accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/base-command.ts`:
- Around line 182-193: The code uses lodash/merge to build userData which leaves
existing auth.github.user and auth.github.token intact when source values are
undefined; replace the merge-based construction in the userData assignment with
an explicit object creation/assignment that sets auth.github.user and
auth.github.token to null (or empty string) to ensure prior credentials are
cleared—locate the userData variable assignment and the call to
merge(globalConfig.get(`users.${userId}`), ...) and change it to build a plain
object that explicitly overwrites auth and auth.github fields (including setting
github.user and github.token to null/''), then write that object back to
globalConfig as before.
---
Duplicate comments:
In `@src/commands/login/login-request.ts`:
- Around line 6-19: The loginRequest function currently ignores the CLI's
--request value; modify loginRequest to accept a requestMessage (string |
undefined) parameter, pass it into the Netlify API ticket creation call (e.g.,
api.createTicket({ clientId: CLIENT_ID, message: requestMessage })) and include
the requestMessage in the logJson output (e.g., request_message: requestMessage)
so the CLI-provided message is preserved through the flow; update references to
ticket creation (createTicket) and the logJson call accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/commands/base-command.tssrc/commands/login/login-check.tssrc/commands/login/login-request.tssrc/commands/login/login.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/commands/login/login-check.ts
| const userData = merge(globalConfig.get(`users.${userId}`), { | ||
| id: userId, | ||
| name, | ||
| email, | ||
| auth: { | ||
| token: accessToken, | ||
| github: { | ||
| user: undefined, | ||
| token: undefined, | ||
| }, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
lodash merge undefined values overwrite destination property
💡 Result:
_.merge() is designed to skip undefined values from the source when the destination already has a value: “Source properties that resolve to undefined are skipped if a destination value exists.” [1]. So { a: undefined } in the source will not overwrite an existing dest.a.
Also, _.mergeWith() won’t help you force “write undefined”, because if the customizer returns undefined, Lodash treats that as “fall back to normal merge behavior” [1]—which still skips undefined.
Workarounds (pick one):
- Use
null(or another sentinel) instead ofundefinedwhen you want to clear a value, so merge will overwrite. - If you want assignment semantics (including overwriting with
undefined), use a non-merge approach (e.g.,Object.assign/ spread) for those fields (note: shallow) [1]. - Post-process after merge: set keys that are explicitly
undefinedin the source onto the result (works for shallow keys; for deep paths you’d need to walk the object).
Example (shallow keys):
const out = _.merge({}, dest, src);
for (const k of Object.keys(src)) {
if (src[k] === undefined) out[k] = undefined;
}Sources: Lodash docs for _.merge / _.mergeWith behavior [1]; discussion/example of merge() skipping undefined [2].
[1] (lodash.com)
[2] (onlinecode.org)
Citations:
Reset token-reset semantics explicitly instead of relying on merge with undefined.
lodash/merge skips source properties that are undefined when a destination value already exists. This means stale auth.github credentials will not be cleared by the current code, creating a security risk where old tokens can survive token-reset operations.
Replace the merge with explicit property assignment:
Explicit overwrite approach
- const userData = merge(globalConfig.get(`users.${userId}`), {
+ const existingUser = globalConfig.get(`users.${userId}`) ?? {}
+ const userData = {
+ ...existingUser,
id: userId,
name,
email,
auth: {
+ ...(existingUser.auth ?? {}),
token: accessToken,
github: {
user: undefined,
token: undefined,
},
},
- })
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const userData = merge(globalConfig.get(`users.${userId}`), { | |
| id: userId, | |
| name, | |
| email, | |
| auth: { | |
| token: accessToken, | |
| github: { | |
| user: undefined, | |
| token: undefined, | |
| }, | |
| }, | |
| }) | |
| const existingUser = globalConfig.get(`users.${userId}`) ?? {} | |
| const userData = { | |
| ...existingUser, | |
| id: userId, | |
| name, | |
| email, | |
| auth: { | |
| ...(existingUser.auth ?? {}), | |
| token: accessToken, | |
| github: { | |
| user: undefined, | |
| token: undefined, | |
| }, | |
| }, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/base-command.ts` around lines 182 - 193, The code uses
lodash/merge to build userData which leaves existing auth.github.user and
auth.github.token intact when source values are undefined; replace the
merge-based construction in the userData assignment with an explicit object
creation/assignment that sets auth.github.user and auth.github.token to null (or
empty string) to ensure prior credentials are cleared—locate the userData
variable assignment and the call to merge(globalConfig.get(`users.${userId}`),
...) and change it to build a plain object that explicitly overwrites auth and
auth.github fields (including setting github.user and github.token to null/''),
then write that object back to globalConfig as before.
biilmann
force-pushed
the
agent-login-flow
branch
from
651c4d7 to
27ce668
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/commands/login/login-check.ts (1)
14-20:⚠️ Potential issue | 🟠 MajorDon’t classify every
showTicket()failure asdenied.This currently converts network/server failures into a false denial state. Only auth/ticket-not-found statuses should map to
denied; other failures should be surfaced.🔧 Proposed fix
- } catch { - logJson({ status: 'denied' }) - log('Status: denied') - return + } catch (error_) { + const status = + typeof error_ === 'object' && error_ !== null && 'status' in error_ + ? Number((error_ as { status?: number }).status) + : undefined + + if (status === 401 || status === 404) { + logJson({ status: 'denied' }) + log('Status: denied') + return + } + + throw error_ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/login/login-check.ts` around lines 14 - 20, The catch around api.showTicket wrongly treats all failures as denied; update the handler in login-check.ts (around api.showTicket and the ticket/ticketId logic) to distinguish authentication/ticket-not-found errors from other failures: inspect the thrown error (e.g. error.status || error.code || error.response?.status) and if it indicates 401/403/404 then call logJson({ status: 'denied' }) and log('Status: denied'), otherwise rethrow or surface the error (so non-auth/network/server errors are not misreported as denied). Ensure you reference api.showTicket, ticketId, logJson and log when making the change.src/commands/base-command.ts (1)
182-193:⚠️ Potential issue | 🔴 Critical
storeTokenstill may not clear staleauth.githubcredentials.Using
merge()here can keep previousauth.github.user/tokenvalues when source values areundefined, so token-reset semantics are not guaranteed.🔧 Proposed fix
- const userData = merge(globalConfig.get(`users.${userId}`), { + const existingUser = globalConfig.get(`users.${userId}`) ?? {} + const userData = { + ...existingUser, id: userId, name, email, auth: { + ...(existingUser.auth ?? {}), token: accessToken, github: { - user: undefined, - token: undefined, + user: null, + token: null, }, }, - }) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/base-command.ts` around lines 182 - 193, The current use of merge(...) to create userData can leave stale auth.github.user/token values when the source fields are undefined; instead, read the existing user object via globalConfig.get(`users.${userId}`) into a local variable and construct userData explicitly (or use a mergeWith customizer) so that id, name, email and auth are assigned exactly — e.g., build userData = { ...existingUser, id: userId, name, email, auth: { token: accessToken, github: { user: undefined, token: undefined } } } — ensuring auth.github.user and auth.github.token are explicitly cleared; update the code around merge, userData, and the globalConfig.get(...) call accordingly.
🧹 Nitpick comments (1)
tests/unit/commands/login/login-request.test.ts (1)
33-52: Add one negative-path test for missingticket.id.The runtime guard exists in
loginRequest; adding a test for that branch will prevent regressions in failure handling.🧪 Suggested test
+ test('throws when ticket creation returns no id', async () => { + mocks.createTicket.mockResolvedValueOnce({}) + await expect(loginRequest()).rejects.toThrow('Failed to create login ticket') + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/commands/login/login-request.test.ts` around lines 33 - 52, Add a negative-path unit test that mocks the login ticket response to omit ticket.id and calls loginRequest, then assert the function fails gracefully: it should reject/throw or log an error and must not print the authorize URL or the "netlify login" command; locate where loginRequest is invoked in the tests and use the existing stdoutOutput capture to confirm no authorize URL/command appears when ticket.id is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/login/login.ts`:
- Around line 25-34: The login helpers ignore command-scoped API/config; update
the calls to loginRequest and loginCheck to pass the command's network and
config so they use the same API options and global config as the rest of the
CLI. Specifically, when calling loginRequest() and loginCheck(options) pass
through command.netlify.apiOpts and command.netlify.globalConfig (e.g., call
loginRequest({ apiOpts: command.netlify.apiOpts, globalConfig:
command.netlify.globalConfig, ... }) and loginCheck({ options, apiOpts:
command.netlify.apiOpts, globalConfig: command.netlify.globalConfig })) so the
functions use those symbols instead of hardcoded config.
---
Duplicate comments:
In `@src/commands/base-command.ts`:
- Around line 182-193: The current use of merge(...) to create userData can
leave stale auth.github.user/token values when the source fields are undefined;
instead, read the existing user object via globalConfig.get(`users.${userId}`)
into a local variable and construct userData explicitly (or use a mergeWith
customizer) so that id, name, email and auth are assigned exactly — e.g., build
userData = { ...existingUser, id: userId, name, email, auth: { token:
accessToken, github: { user: undefined, token: undefined } } } — ensuring
auth.github.user and auth.github.token are explicitly cleared; update the code
around merge, userData, and the globalConfig.get(...) call accordingly.
In `@src/commands/login/login-check.ts`:
- Around line 14-20: The catch around api.showTicket wrongly treats all failures
as denied; update the handler in login-check.ts (around api.showTicket and the
ticket/ticketId logic) to distinguish authentication/ticket-not-found errors
from other failures: inspect the thrown error (e.g. error.status || error.code
|| error.response?.status) and if it indicates 401/403/404 then call logJson({
status: 'denied' }) and log('Status: denied'), otherwise rethrow or surface the
error (so non-auth/network/server errors are not misreported as denied). Ensure
you reference api.showTicket, ticketId, logJson and log when making the change.
---
Nitpick comments:
In `@tests/unit/commands/login/login-request.test.ts`:
- Around line 33-52: Add a negative-path unit test that mocks the login ticket
response to omit ticket.id and calls loginRequest, then assert the function
fails gracefully: it should reject/throw or log an error and must not print the
authorize URL or the "netlify login" command; locate where loginRequest is
invoked in the tests and use the existing stdoutOutput capture to confirm no
authorize URL/command appears when ticket.id is missing.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
src/commands/base-command.tssrc/commands/login/index.tssrc/commands/login/login-check.tssrc/commands/login/login-request.tssrc/commands/login/login.tssrc/commands/main.tstests/unit/commands/login/login-check.test.tstests/unit/commands/login/login-request.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/unit/commands/login/login-check.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/commands/login/index.ts
| if (options.request) { | ||
| const { loginRequest } = await import('./login-request.js') | ||
| await loginRequest() | ||
| return | ||
| } | ||
|
|
||
| if (options.check) { | ||
| const { loginCheck } = await import('./login-check.js') | ||
| await loginCheck(options) | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /repo && git ls-files | grep -E "(login|command)" | head -20Repository: netlify/cli
Length of output: 110
🏁 Script executed:
cat -n src/commands/login/login.ts | head -50Repository: netlify/cli
Length of output: 1902
🏁 Script executed:
fd -t f "login-request\|login-check" --type fRepository: netlify/cli
Length of output: 37
🏁 Script executed:
find . -name "login-request.js" -o -name "login-request.ts"Repository: netlify/cli
Length of output: 92
🏁 Script executed:
find . -name "login-check.js" -o -name "login-check.ts"Repository: netlify/cli
Length of output: 90
🏁 Script executed:
rg "export.*loginRequest" -t ts -t jsRepository: netlify/cli
Length of output: 132
🏁 Script executed:
rg "export.*loginCheck" -t ts -t jsRepository: netlify/cli
Length of output: 149
🏁 Script executed:
cat -n src/commands/login/login-request.tsRepository: netlify/cli
Length of output: 1077
🏁 Script executed:
cat -n src/commands/login/login-check.tsRepository: netlify/cli
Length of output: 1949
🏁 Script executed:
rg "command\.netlify" -t ts -A 3 -B 1 | head -50Repository: netlify/cli
Length of output: 2979
🏁 Script executed:
cat -n src/commands/base-command.ts | head -100Repository: netlify/cli
Length of output: 4263
🏁 Script executed:
rg "apiOpts|globalConfig" -t ts -B 2 -A 2 | head -60Repository: netlify/cli
Length of output: 4043
🏁 Script executed:
rg "new NetlifyAPI" -t ts -A 2 -B 2 | head -80Repository: netlify/cli
Length of output: 1117
🏁 Script executed:
rg "apiOpts.*=" -t ts -B 2 -A 2 | grep -A 5 "const apiOpts"Repository: netlify/cli
Length of output: 413
🏁 Script executed:
cat -n src/commands/base-command.ts | sed -n '200,300p'Repository: netlify/cli
Length of output: 4587
🏁 Script executed:
rg "this.netlify" -t ts -A 2 | head -80Repository: netlify/cli
Length of output: 1004
🏁 Script executed:
rg "type NetlifyOptions|interface NetlifyOptions" -t ts -A 20Repository: netlify/cli
Length of output: 1134
🏁 Script executed:
cat -n src/commands/types.ts | head -80Repository: netlify/cli
Length of output: 108
🏁 Script executed:
rg "apiOpts" src/commands/base-command.ts -B 5 -A 5Repository: netlify/cli
Length of output: 918
🏁 Script executed:
cat -n src/commands/login/login.ts | sed -n '20,40p'Repository: netlify/cli
Length of output: 770
🏁 Script executed:
rg "loginRequest\|loginCheck" -t ts --type-list | grep -v node_modules | head -40Repository: netlify/cli
Length of output: 444
🏁 Script executed:
cat -n src/commands/base-command.ts | sed -n '360,420p'Repository: netlify/cli
Length of output: 2864
🏁 Script executed:
cat -n src/commands/login/login-check.ts | tail -20Repository: netlify/cli
Length of output: 665
🏁 Script executed:
rg "loginRequest\(" -t ts -B 3 -A 3Repository: netlify/cli
Length of output: 1552
🏁 Script executed:
cat -n src/commands/base-command.ts | sed -n '475,520p'Repository: netlify/cli
Length of output: 1711
🏁 Script executed:
rg "command.netlify.apiOpts" -t ts -B 3 -A 3 | head -40Repository: netlify/cli
Length of output: 37
🏁 Script executed:
rg "NetlifyAPI\(" -t ts -B 2 -A 4 | head -80Repository: netlify/cli
Length of output: 1492
🏁 Script executed:
cat -n src/commands/login/login-check.ts | sed -n '1,20p'Repository: netlify/cli
Length of output: 825
Pass command-scoped API/config into request/check flows.
The loginRequest() and loginCheck() functions create API instances with hardcoded configuration, ignoring custom API URLs, proxies, and global config set on the command. This causes divergent behavior from standard CLI auth paths that use command.netlify.apiOpts and command.netlify.globalConfig.
Pass these settings to the helper functions:
Proposed changes
if (options.request) {
const { loginRequest } = await import('./login-request.js')
- await loginRequest()
+ await loginRequest({ apiOpts: command.netlify.apiOpts })
return
}
if (options.check) {
const { loginCheck } = await import('./login-check.js')
- await loginCheck(options)
+ await loginCheck(options, {
+ apiOpts: command.netlify.apiOpts,
+ globalConfig: command.netlify.globalConfig,
+ })
return
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/login/login.ts` around lines 25 - 34, The login helpers ignore
command-scoped API/config; update the calls to loginRequest and loginCheck to
pass the command's network and config so they use the same API options and
global config as the rest of the CLI. Specifically, when calling loginRequest()
and loginCheck(options) pass through command.netlify.apiOpts and
command.netlify.globalConfig (e.g., call loginRequest({ apiOpts:
command.netlify.apiOpts, globalConfig: command.netlify.globalConfig, ... }) and
loginCheck({ options, apiOpts: command.netlify.apiOpts, globalConfig:
command.netlify.globalConfig })) so the functions use those symbols instead of
hardcoded config.
…ndly auth Agents can't complete the browser-based OAuth flow on their own. This adds a non-blocking two-step flow: create a ticket with --request to get a shareable URL, then check its status with --check to finalize login. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
biilmann
force-pushed
the
agent-login-flow
branch
from
27ce668 to
ae3371e
Compare
sean-roberts
left a comment
sean-roberts
left a comment
There was a problem hiding this comment.
I tested it out, it could be improved to have better direction setting for agents. like "pending" could say "pending (waiting for user to authenticate with the url provided)" but those can be improved later
3 participants
Summary
This aims to improve the AX of the CLI for agents that are not running on the same box as their human.
It adds a login flow:
That returns the url for ticket auth instead of trying to open a browser and gives the agent a command to check the status of a ticket:
I also added some extra help output to the
netlify helpcommand and in thenetlify deploycommand when running without authentication to help agents.Note, we'll need to add support for the "message" in the auth ticket and we'll need to add support for displaying this message from the agent to the user in the ticket flow.
Details
--request <message>flag tonetlify loginthat creates a ticket and returns a shareable authorization URL — no browser needed--check <ticket-id>flag that checks ticket status (pending/authorized/denied) and finalizes login when authorized--jsonnetlify --helpand during the interactive login flow pointing to the new flagsstoreTokenhelper fromexpensivelyAuthenticatefor reuseTest plan
netlify login --request "test"→ outputs ticket ID, URL, and check commandnetlify login --check <id>before authorizing →Status: pendingnetlify login --check <id>after authorizing →Status: authorized+ user info, CLI is logged innetlify login --check <expired-id>→Status: denied--jsonflag produces JSON output for both--requestand--checknetlify login(no flags) still works as beforenetlify --helpshows new login hintsnpm run typecheckpassesnpm exec vitest -- run tests/unit/commands/login/🤖 Generated with Claude Code