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:
📝 WalkthroughWalkthroughAdds a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 5556dde
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tests/unit/commands/deploy/deploy.test.ts (1)
5-19: Consider expanding test coverage forisNonInteractive().The test correctly verifies the
process.env.CIpath with proper cleanup. However,isNonInteractive()has multiple detection paths (!process.stdin.isTTY,!process.stdout.isTTY,isCI). Consider adding tests for these conditions if comprehensive coverage is desired.Also, the test file is named
deploy.test.tsbut tests a utility fromscripted-commands.ts. Moving this totests/unit/utils/scripted-commands.test.tswould better reflect what's being tested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/commands/deploy/deploy.test.ts` around lines 5 - 19, The current test only covers the CI env branch for isNonInteractive; add unit tests that simulate the other detection paths by stubbing/mocking process.stdin.isTTY to false, process.stdout.isTTY to false, and the isCI helper to return true/false so each branch is exercised, and move the test file from deploy.test.ts into tests/unit/utils/scripted-commands.test.ts (or update the test suite path) so the file name matches the tested symbol isNonInteractive and its module scripted-commands.ts.tests/integration/commands/deploy/deploy-non-interactive.test.ts (1)
124-173: Assert the advertised behaviors, not just success/failure.The 422 case never checks that the retried name is actually suffixed, and the CI failure case accepts any thrown error. Both features can regress while this suite stays green. Tighten the assertions against
mockApi.requestsand the error text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/commands/deploy/deploy-non-interactive.test.ts` around lines 124 - 173, Update the two tests to assert specific behaviors instead of just success/failure: in the "should auto-resolve name collision with suffix" test (the one using createRoutesForSiteCreation, startDeployMockApi, callCli and parseDeploy) inspect mockApi.requests to verify that after the initial 422 create-site request for "taken-name" a subsequent POST used a suffixed name (e.g., "taken-name-1") and assert parseDeploy.site_id remains correct; in the "should fail fast..." test (the one calling callCli with CI: 'true') assert the thrown error message contains the expected helpful text (use rejects.toThrow(/expected error text/)) and also verify mockApi.requests reflect the accounts GET was called and no site-creation POST was attempted. Ensure you reference the existing helpers (startDeployMockApi, callCli, getCLIOptions, mockApi.requests, parseDeploy) when adding these assertions.
🤖 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/deploy/deploy.ts`:
- Around line 1113-1134: The non-interactive branch duplicates team guidance
instead of reusing validateTeamForSiteCreation(), causing inconsistent messages
and missing slug details; replace the logic after resolveTeam(accounts) so that
if no team is returned you call validateTeamForSiteCreation(accounts, options)
(or reuse its returned message/throw behavior) to produce the proper
multi-team/no-default messaging (including slugs) and only fall back to
generateDeployCommand/createSiteWithFlags when validateTeamForSiteCreation
permits; ensure you still set options.createSite/options.team and call
createSiteWithFlags(options, command, site) when resolveTeam returns a team and
use logAndThrowError only for the validated, shared error message.
- Around line 297-307: The fallback branch currently pushes '--site-name
<SITE_NAME>' when neither options.createSite nor options.site is set, which
makes recovery suggest creating a new project; change that to keep recovery
pointed at the current site by pushing the site-flag placeholder instead (use
parts.push('--site <SITE>') or similar) so prepareProductionDeploy() and the
non-interactive locked-site retry will target the existing linked site; update
the code around options.createSite/options.site handling (the parts array logic)
accordingly and keep the availableTeams check as-is.
In `@src/commands/deploy/index.ts`:
- Around line 86-93: Update the help text for the '--create-site [name]' option
(the string passed where '--create-site [name]' is defined in
src/commands/deploy/index.ts) to remove the incorrect "Requires --team flag if
you have multiple teams" clause; instead either drop that sentence entirely or
replace it with a correct note such as "If multiple teams exist, a default team
will be auto-selected; use --team to choose a different team." Also ensure the
'--site-name <name>' help remains consistent with this behavior.
- Around line 131-140: The code sets options.createSite from options.siteName
but doesn't block when a site is already selected; update the branch that
handles options.siteName/options.createSite to first detect an already-resolved
target site (e.g. check options.site (the --site flag),
process.env.NETLIFY_SITE_ID, or the linked project/site resolution that
ensureSiteExists() would prefer) and fail fast with logAndThrowError if any of
those exist, so a conflicting --site/NETLIFY_SITE_ID/linked project can't be
overridden by --site-name; keep the existing checks for mismatched --create-site
values and only set options.createSite = options.siteName when no resolved site
exists.
In `@src/commands/teams/teams-list.ts`:
- Around line 6-7: In teamsList, forward the CLI auth override into the
authenticate call so scripted/CI runs skip the interactive login: change the
authenticate invocation in the teamsList function from command.authenticate() to
pass the token from the parsed options (e.g. command.authenticate(options.auth)
or command.authenticate(options?.auth)) so the CLI `--auth` value is used when
present.
---
Nitpick comments:
In `@tests/integration/commands/deploy/deploy-non-interactive.test.ts`:
- Around line 124-173: Update the two tests to assert specific behaviors instead
of just success/failure: in the "should auto-resolve name collision with suffix"
test (the one using createRoutesForSiteCreation, startDeployMockApi, callCli and
parseDeploy) inspect mockApi.requests to verify that after the initial 422
create-site request for "taken-name" a subsequent POST used a suffixed name
(e.g., "taken-name-1") and assert parseDeploy.site_id remains correct; in the
"should fail fast..." test (the one calling callCli with CI: 'true') assert the
thrown error message contains the expected helpful text (use
rejects.toThrow(/expected error text/)) and also verify mockApi.requests reflect
the accounts GET was called and no site-creation POST was attempted. Ensure you
reference the existing helpers (startDeployMockApi, callCli, getCLIOptions,
mockApi.requests, parseDeploy) when adding these assertions.
In `@tests/unit/commands/deploy/deploy.test.ts`:
- Around line 5-19: The current test only covers the CI env branch for
isNonInteractive; add unit tests that simulate the other detection paths by
stubbing/mocking process.stdin.isTTY to false, process.stdout.isTTY to false,
and the isCI helper to return true/false so each branch is exercised, and move
the test file from deploy.test.ts into
tests/unit/utils/scripted-commands.test.ts (or update the test suite path) so
the file name matches the tested symbol isNonInteractive and its module
scripted-commands.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1faab391-53c7-471f-b3b1-501a06e1a942
📒 Files selected for processing (14)
docs/commands/deploy.mddocs/commands/teams.mdsrc/commands/base-command.tssrc/commands/deploy/deploy.tssrc/commands/deploy/index.tssrc/commands/deploy/option_values.tssrc/commands/main.tssrc/commands/teams/index.tssrc/commands/teams/teams-list.tssrc/commands/teams/teams.tssrc/utils/scripted-commands.tstests/integration/commands/deploy/deploy-non-interactive.test.tstests/integration/commands/teams/teams.test.tstests/unit/commands/deploy/deploy.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/utils/scripted-commands.test.ts (1)
3-50: Consider adding a test for the interactive (false) case.The test suite only covers scenarios where
isNonInteractive()returnstrue. Adding a negative test case that verifies the function returnsfalsewhen in an interactive environment (both TTYs defined and no CI indicators) would strengthen coverage and guard against regressions where the function might incorrectly always returntrue.💡 Suggested additional test case
+ test('should return false when in interactive environment', async () => { + const originalCI = process.env.CI + const originalStdinTTY = process.stdin.isTTY + const originalStdoutTTY = process.stdout.isTTY + + delete process.env.CI + Object.defineProperty(process.stdin, 'isTTY', { value: true, configurable: true }) + Object.defineProperty(process.stdout, 'isTTY', { value: true, configurable: true }) + + try { + const isNonInteractive = await loadModule() + expect(isNonInteractive()).toBe(false) + } finally { + if (originalCI === undefined) { + delete process.env.CI + } else { + process.env.CI = originalCI + } + Object.defineProperty(process.stdin, 'isTTY', { value: originalStdinTTY, configurable: true }) + Object.defineProperty(process.stdout, 'isTTY', { value: originalStdoutTTY, configurable: true }) + } + })Note: This test would also require mocking
ci-info'sisCIexport tofalseto work correctly in CI environments (see related comment).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/utils/scripted-commands.test.ts` around lines 3 - 50, Add a negative test that asserts isNonInteractive() returns false when the environment is interactive: mock ci-info's isCI to false, ensure process.env.CI is unset, and set process.stdin.isTTY and process.stdout.isTTY to true (or their original truthy values) before importing isNonInteractive via the existing loadModule helper; restore the original values in a finally block similar to the other tests to avoid test pollution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/utils/scripted-commands.test.ts`:
- Around line 3-50: Add a negative test that asserts isNonInteractive() returns
false when the environment is interactive: mock ci-info's isCI to false, ensure
process.env.CI is unset, and set process.stdin.isTTY and process.stdout.isTTY to
true (or their original truthy values) before importing isNonInteractive via the
existing loadModule helper; restore the original values in a finally block
similar to the other tests to avoid test pollution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f16ad9a-6311-4331-b29e-f9b190659398
📒 Files selected for processing (6)
docs/commands/deploy.mdsrc/commands/deploy/deploy.tssrc/commands/deploy/index.tssrc/commands/teams/teams-list.tstests/integration/commands/deploy/deploy-non-interactive.test.tstests/unit/utils/scripted-commands.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/commands/teams/teams-list.ts
- src/commands/deploy/index.ts
- tests/integration/commands/deploy/deploy-non-interactive.test.ts
- src/commands/deploy/deploy.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/commands/deploy/deploy-non-interactive.test.ts`:
- Around line 168-178: The test's failure assertion is too narrow: instead of
only expecting /--team/ update the assertion around callCli (used with deploy
and getCLIOptions) to verify the thrown error message also includes the
available team slugs and the guidance to run `teams:list`; in other words,
replace the single toThrow(/--team/) check with an assertion that the error text
contains the team slugs returned by the mock API and the `teams:list`
instruction (e.g., via a combined regex or separate contains checks) so the test
validates the full documented UX contract for the deploy command in
non-interactive mode.
- Around line 100-118: The test in deploy-non-interactive.test.ts calls the CLI
with '--site-name my-test-site' but only asserts IDs; update the test that
invokes callCli (and parses with parseDeploy) to also assert the site name was
propagated by adding an expectation like
expect(deploy.site_name).toBe('my-test-site') (or the appropriate response
property your parseDeploy returns) immediately after the existing ID checks;
locate this near the callCli/parseDeploy usage and ensure getCLIOptions/mockApi
returns a response that includes site_name so the assertion can pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10f67b5f-5be8-47b2-9993-419e4a4440c2
📒 Files selected for processing (1)
tests/integration/commands/deploy/deploy-non-interactive.test.ts
tests/integration/commands/deploy/deploy-non-interactive.test.ts
Outdated
Show resolved
Hide resolved
eduardoboucas
left a comment
eduardoboucas
left a comment
There was a problem hiding this comment.
Great changes. I left some comments, nothing major.
| `Multiple teams available. Please specify which team to use with --team flag.\n` + | ||
| `Available teams: ${availableTeams}\n\n` + | ||
| `Example: netlify deploy --create-site${siteName ? ` ${siteName}` : ''} --team <TEAM_SLUG>`, | ||
| `Example: netlify deploy --create-site${siteName ? ` ${siteName}` : ''} --team <TEAM_SLUG>\n\n` + |
There was a problem hiding this comment.
Should this use the new flag?
| `Example: netlify deploy --create-site${siteName ? ` ${siteName}` : ''} --team <TEAM_SLUG>\n\n` + | |
| `Example: netlify deploy --site-name${siteName ? ` ${siteName}` : ''} --team <TEAM_SLUG>\n\n` + |
| @@ -84,9 +84,13 @@ For detailed configuration options, see the Netlify documentation.`, | |||
| .addOption(new Option('--upload-source-zip', 'Upload source code as a zip file').default(false).hideHelp(true)) | |||
| .option( | |||
There was a problem hiding this comment.
I think I would set hideHelp(true) so that we keep supporting the old flag but we only advertise the new one.
|
|
||
| ### [teams](/commands/teams) | ||
|
|
||
| Handle various team operations |
There was a problem hiding this comment.
Do you have any plans for what other operations would go here?
| return Boolean(scriptedCommand && testingPrompts && noForceFlag) | ||
| } | ||
|
|
||
| export const isNonInteractive = (): boolean => |
There was a problem hiding this comment.
We already have other places in the app where we try to assert this. It would be great if we could centralise this into one place with a consistent implementation, especially given how crucial it is that we handle non-interactive mode correctly across the board.
Also, may I suggest that we flip the logic here and call this isInteractive? This will avoid double negative scenarios like if (!isNonInteractive()), which add unnecessary confusion.
2 participants
[disclaimer - I definitely used claude to help me articulate the needs and changes but I've independently reviewed the changes]
Summary
AI agents are one of the fastest-growing categories of CLI users, but the current deploy flow assumes a human at a terminal. When an agent runs
netlify deploy --prod --dir .on an unlinked folder, it takes 5 attempts to get a successful deploy — the CLI hangs on interactive prompts that agents can't answer, flag names don't match what agents guess, name collisions are a dead end with no recovery path, and there's no programmatic way to discover teams.This PR makes
netlify deploy --dir . --proda one-command operation for agents, CI pipelines, and scripts — while keeping the interactive experience completely unchanged for humans.The problem
Here's what actually happened when an AI agent tried to deploy a site:
netlify deploy --prod --dir .netlify deploy --prod --dir . --site-name ...--site-namedoesn't existnetlify deploy --helpnetlify deploy --prod --dir . --create-site hello-worldnetlify deploy --prod --dir . --create-site hello-world-$(date +%s)5 attempts for what should be a 1-command operation. Each failure mode is independently fixable, and fixing all of them turns the experience from "frustrating trial and error" into "just works".
What changed
Non-interactive auto-create
The biggest issue: when running without a TTY (piped output, CI, agents), the CLI drops into an
inquirerprompt that hangs forever. Now,ensureSiteExists()detects non-interactive environments and takes action instead of blocking:netlify teams:listThe same guard was added to
prepareProductionDeploy(), which has its own interactive prompt about locked deploys that also hangs in non-interactive mode.--site-name <name>flagUsers (and agents) instinctively reach for
--site-namebecause that's the standard CLI pattern — the verb is the command, the noun is a flag. The existing--create-site [name]is an unusual boolean-ish flag with an optional argument that reads ambiguously in help text.--site-name my-sitenow works and maps to--create-site my-siteinternally. The old flag still works identically.Name collision auto-resolution
When a requested site name is taken (API returns 422), the CLI now automatically retries with a random hex suffix appended (e.g.
my-site-a3f1b2c4) and logs what happened. Previously this was a hard error with no recovery — agents had to invent their own uniqueness strategy.Default team resolution
If the user has a default team, it's now used automatically when
--teamis omitted — even with multiple teams. Previously only the single-team case was auto-resolved. This means most users never need to pass--teamat all.teams:listcommandThere was no non-interactive way to discover team slugs. Agents could only find them by parsing the interactive prompt output from a failed command.
netlify teams:listandnetlify teams:list --jsonnow provide programmatic team discovery. All non-interactive error messages point to this command.Before and after
Before (5 attempts)
After (1 attempt)
Backwards compatibility
--create-site [name]continues to work exactly as before--site-nameis purely additive--siteorNETLIFY_SITE_IDare unaffected