Andrewpai/yes flag

[andrewpai]

• Added support for --yes to kickstart:kill
  • If the CLI is running on a TTY (human at a terminal) they get the same confirmation prompt
  • If not, the CLI returns a message indicating that --yes is required an no action taken
• Added command line params for the interactive items in kickstart:install
• Changed import:generate options to use kebab case with ongoing but deprecated camelCase support

[Copilot]

🟡 Changes recommended

The new confirmation/test logic has a few correctness and reliability issues (TTY detection can hang, setTimeout-driven install steps bypass try/catch, and persistent nock interceptors can leak across tests).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR improves CLI ergonomics and safety by standardizing confirmation behavior for destructive operations (via a shared confirmOrExit() helper + --yes flag), while also aligning other commands with documented option-naming conventions and adding/adjusting tests and contributor guidance.

Changes:

• Added utils.confirmOrExit() and wired kickstart:kill to support --yes and non-interactive confirmation gating.
• Enhanced kickstart:install to accept non-interactive inputs via CLI options (including env-var indirection for the admin password) and added unit tests for the new validation/answer-resolution logic.
• Updated import:generate option names to kebab-case while retaining hidden deprecated aliases with deprecation warnings; bumped package version and added contributing guidance.

File summaries

File	Description
src/utils.ts	Adds confirmOrExit() helper and adjusts dotenv config verbosity.
src/commands/kickstart-kill.ts	Adds --yes option and uses confirmOrExit() before destructive Docker teardown.
src/commands/kickstart-install.ts	Adds CLI options + extracted validation/answer-resolution for unattended installs.
src/commands/import-generate.ts	Migrates flags to kebab-case and keeps deprecated aliases with warnings.
package.json	Version bump and test script updates to include new test file.
package-lock.json	Updates lockfile version metadata to match the package version bump.
CONTRIBUTING.md	Documents command/option conventions, risky-ops policy, and test-running guidance.
AGENTS.md	Documents --yes confirmation expectations for risky operations.
tests/telemetry/telemetry.test.js	Adds nock stubs for PostHog calls in full-command telemetry tests.
tests/commands/kickstart-install.test.js	Adds unit tests for new kickstart-install validation and option resolution logic.

Review details

Suppressed comments (1)

tests/telemetry/telemetry.test.js:88

• This test uses nock.persist() but doesn't clean up the interceptor, which can leak into subsequent tests and make failures order-dependent. Prefer cleaning nock in the finally block (or avoid persist if a single call is expected).

      nock('https://us.i.posthog.com')
        .persist()
        .post('/batch/')
        .reply(200)

• Files reviewed: 9/10 changed files
• Comments generated: 5
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

There are confirmed functional issues (email normalization/validation and kickstart-kill success reporting) plus missing tests for newly introduced risky-operation gating behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (6)

Previously missed (4) — in code that hasn't changed since the last review.

src/commands/kickstart-install.ts:27

• validateEmail() currently tests the raw input against an un-anchored regex. This will accept values with leading/trailing whitespace (or other surrounding text) and then those untrimmed values are written into kickstart.json as the admin email, which can break login/config.

This issue also appears in the following locations of the same file:

• line 81
• line 142
  src/commands/kickstart-kill.ts:37

• The close handler always prints a success message even when docker compose down -v fails (non-zero exit code). This can lead users/automation to believe the container and volumes were destroyed when they were not.
  src/commands/kickstart-install.ts:60
• Typo in the new JSDoc: "intial" should be "initial".
  src/commands/kickstart-kill.ts:33
• spawn(..., { stdio: 'inherit' }) will not provide a readable starting.stdout stream (it will be null), so the subsequent for await (const data of starting.stdout) block is dead code. This is misleading and makes it look like output is being processed when it isn't.

src/commands/kickstart-install.ts:85

• When --admin-email is provided, the code validates it but then stores the original (potentially whitespace-padded) string in email. If validateEmail() starts trimming/anchoring (as suggested), the resolved value should also be normalized before being persisted to kickstart.json.

  if (options.adminEmail !== undefined) {
    const result = validateEmail(options.adminEmail);
    if (result !== true) {
      throw new Error(`--admin-email: ${result}`);
    }

src/commands/kickstart-install.ts:142

• Prompted email input is assigned verbatim; if the user pastes an email with trailing whitespace it will be accepted (regex matches a substring) and then written with the whitespace into kickstart.json. Trimming here keeps stored values consistent.

    if (email === undefined)    email    = prompted.email as string;

• Files reviewed: 9/10 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

There are a couple of user-facing typos and one confirmation-path Promise that can hang in test/mocked-exit environments and should be fixed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/commands/kickstart-install.ts:60

• Typo in JSDoc: "intial" should be "initial".
  src/commands/kickstart-install.ts:216
• User-facing error message has a grammatical mistake ("does not exists"). This should be "does not exist".

• Files reviewed: 11/12 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

It introduces a few fixable issues in the changed code (unused import, a typo, improved error throwing/message, and missing test coverage for new import:generate deprecation behavior).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

src/commands/kickstart-install.ts:216

• Throwing a colored string loses stack trace information and the error message has a grammar issue ("does not exists"). Prefer throwing an Error with a correctly worded message.
  src/commands/import-generate.ts:4
• readFile is imported from fs/promises but never used in this file; this will fail linting/tsc noUnusedLocals in stricter setups and adds noise for maintainers.
  src/commands/kickstart-install.ts:60
• Typo in JSDoc: "intial" should be "initial".

• Files reviewed: 11/12 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

There are a couple of correctness/safety gaps (explicit crypto UUID generation and non-interactive confirm flow when process.exit is mocked) that should be addressed before merging.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/commands/kickstart-install.ts:220

• kickstart:install generates secrets using crypto.randomUUID() without importing node:crypto. This relies on a global WebCrypto implementation being present, which may not be true across supported Node runtimes and is inconsistent with other files that import randomUUID from node:crypto. Use node:crypto's randomUUID explicitly.
  src/utils.ts:224
• In the non-interactive path, confirmOrExit() calls errorAndExit() (which calls process.exit) and then returns. If process.exit is mocked/deferred (common in unit tests or programmatic usage), the Promise resolves and the caller can continue with the risky operation. Consider throwing/rejecting after errorAndExit (similar to handleConfirmationAnswer) so execution cannot proceed when exit is not terminal.

• Files reviewed: 12/13 changed files
• Comments generated: 0 new
• Review effort level: Lite

[andrewpai]

Addressed both items flagged in the latest review (5160436306) in 4c3ccc7:

1. crypto.randomUUID() used without importing node:crypto (kickstart-install.ts) — now explicitly imports randomUUID from node:crypto, matching the convention already used in src/utils.ts and src/utilities/kickstart/variable-substitution.ts.

2. confirmOrExit() non-interactive path can silently succeed if process.exit is mocked/deferred (utils.ts) — this was the same bug class as the handleConfirmationAnswer fix from a couple of reviews ago, just in the sibling (non-interactive) branch. confirmOrExit now throws after errorAndExit() rather than returning normally, so the promise can't silently resolve and let the caller proceed with the risky operation when exit doesn't actually halt execution. Updated the three existing non-interactive tests in tests/utils.test.js to use assert.rejects, since they previously wouldn't have caught this regression.

[Copilot]

🟢 Approval recommended

The changes align with the stated behavior, introduce appropriate safety gating for destructive operations, and include targeted unit coverage for the new/updated logic.

Review details

• Files reviewed: 12/13 changed files
• Comments generated: 0 new
• Review effort level: Lite

[mark-robustelli]

@andrewpai shouldn't this merge to the 'next' branch?

[mark-robustelli]

A few things to think about.

[Copilot]

🟡 Changes recommended

Critical and moderate findings remain in CI configuration and kickstart-install validation, error handling, and parser coverage.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

src/commands/import-generate.ts:30

• The warning says the old flag "will be deprecated", but these options are already registered and documented as deprecated in this change. That message gives users the wrong removal status; say that the flag is already deprecated and will be removed in a future release.

      `${old} will be deprecated in a future release.`

src/commands/kickstart-install.ts:241

• These new Commander options are not exercised by the added tests: the tests call resolveInstallAnswers() directly, so a typo in any flag declaration or its mapping into InstallOptions would go undetected. Add a parser-level test for kickstartInstall that passes each option spelling and verifies the action receives the expected fields, as required by the repository's new testing policy.

  .option('--admin-email <email>', 'Admin user email address (skips prompt)')
  .option('--admin-password-env <ENV_VAR>', 'Name of environment variable containing the admin password (skips prompt)')
  .option('--application-name <name>', 'Application name (skips prompt)')

src/commands/kickstart-install.ts:217

• The new error message has incorrect grammar: does not exists should be does not exist.

    if (!fs.existsSync(directory)) throw (chalk.red(`Something went wrong. ${directory} does not exists.`))

• Files reviewed: 12/12 changed files
• Comments generated: 5
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved moderate findings remain in installation error handling, validation, command wiring, test isolation, and contributor guidance.

Review details

Suppressed comments (12)

Previously missed (1) — in code that hasn't changed since the last review.

src/commands/import-generate.ts:30

• This warning is emitted only when the camelCase option is already being treated as deprecated, but it says the flag “will be deprecated,” which gives users the opposite status. Please state that the old flag is deprecated and will be removed in a future release.

AGENTS.md:31

• This newly added policy has the same mismatch with existing commands: lambda:delete permanently deletes a lambda without a confirmation or --yes option. Either update the existing destructive commands or narrow the policy wording so agents are not instructed to assume a gate that is not present.

- Commands that perform irreversible or potentially disruptive operations require `--yes` to proceed non-interactively

CONTRIBUTING.md:26

• npm run test:unit and npm run test:integration are not defined in package.json, so contributors following these instructions get npm missing-script errors. The repository exposes npm test as its test command; document that command here or add the missing scripts.

# Unit tests (run these before every commit)
npm run test:unit

# Integration tests (requires a live FusionAuth instance)
npm run test:integration

CONTRIBUTING.md:15

• This policy is broader than the current implementation: lambda:delete permanently deletes a lambda at src/commands/lambda-delete.ts:13-18, but that command has no confirmation or --yes gate. Either apply the policy to existing destructive commands or scope this statement so the contributor guidance is accurate.

Commands that perform risky operations must gate execution behind user confirmation using `confirmOrExit()` from `src/utils.ts`. All such commands must expose a `--yes` flag.

tests/commands/kickstart-kill.test.js:76

• This test invokes the real action, which calls the un-awaited logEvent() after the precondition checks. Unlike the telemetry tests, it neither disables telemetry nor intercepts PostHog, so a clean run can create src/.fa/config.json and issue a background network request. Isolate telemetry for these action tests (and restore the environment afterward).

      await action(
        { yes: true },
        {
          isDockerInstalled: () => true,
          confirmOrExit: async (...args) => { confirmCalls.push(args) },
          spawn: (...args) => { spawnCalls.push(args); return fakeChildProcess() },

src/commands/kickstart-install.ts:61

• The new JSDoc contains a spelling error in intial; please correct it to initial so the public helper documentation is accurate.

 * We need the intial admin's credentials (email and password) and a name for a

src/commands/kickstart-install.ts:242

• The new install options are tested only through resolveInstallAnswers; no test invokes kickstartInstall.parseAsync() or the command action. As a result, the Commander option-to-action wiring and the actual generation path can regress while all current tests still pass. Add a command-level test covering the new options and generated output.

  .option('--admin-email <email>', 'Admin user email address (skips prompt)')
  .option('--admin-password-env <ENV_VAR>', 'Name of environment variable containing the admin password (skips prompt)')
  .option('--application-name <name>', 'Application name (skips prompt)')
  .action((dir, options) => action(dir, options))

src/commands/kickstart-install.ts:20

• This validator is not anchored, so values such as prefix admin@example.com pass validateEmail(). The new --admin-email path can therefore write a non-email string into the generated kickstart; anchor the expression and cover a trailing/leading-text case.

export const EMAIL_REGEX = /(([^<>()[\]\\.,;:\s@"]+(\.[^<>()[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))/;

src/commands/kickstart-install.ts:217

• The new error message uses does not exists; the grammatically correct form is does not exist.

    if (!fs.existsSync(directory)) throw (chalk.red(`Something went wrong. ${directory} does not exists.`))

src/commands/kickstart-install.ts:231

• The new sequential file-generation steps now run inside this catch, but the handler only logs the error and then resolves. A failed copy, kickstart write, or .env update will therefore return exit code 0 to unattended callers; rethrow or call errorAndExit() so installation failures are reported as failures.

  } catch (e) {
    console.error(e)

src/commands/kickstart-install.ts:208

• Once the spinner is started, any failure in the new sequential file-generation steps reaches the catch at line 230, which only logs the error. The spinner is never stopped, so a failed install can leave its timer active and keep the CLI alive; stop or fail the spinner in the error/finally path as the success path does.

    const spinner = yoctoSpinner({ text: "Building..." }).start()

src/commands/kickstart-kill.ts:63

• The tests call the exported action directly and supply yes: true, but never parse kickstartKill with --yes. A regression in this new option's Commander registration or its handoff to action would therefore go undetected; add a command-level parse test for the flag.

  .option('--yes', 'Skip confirmation prompt', false)
  .action((options) => action(options))

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved validation, failure-reporting, policy, documentation, and test-isolation issues remain.

Review details

Suppressed comments (11)

Previously missed (1) — in code that hasn't changed since the last review.

tests/commands/kickstart-kill.test.js:17

• The action calls logEvent() without awaiting it, and these tests do not disable or intercept telemetry. With the default environment, the success and rejection cases can send real PostHog requests and leave asynchronous network work running, making the suite depend on external services. Disable telemetry or inject a mocked logger for this test suite.

AGENTS.md:34

• This rule covers every potentially disruptive command, but the existing kickstart:stop still runs docker compose stop without confirmOrExit() or --yes (src/commands/kickstart-stop.ts:17-41). Either bring that command under the new policy or narrow the wording to irreversible/destructive operations; otherwise this guidance is already violated by the repository.

- Commands that perform irreversible or potentially disruptive operations require `--yes` to proceed non-interactively
- Without `--yes`, these commands exit with an error in non-TTY contexts (agents, pipes, scripts)
- Always obtain user confirmation before passing `--yes`; never pass it autonomously for destructive operations
- Where available, prefer running with `--dry-run` first to preview changes before committing

CONTRIBUTING.md:26

• The contributor guide instructs users to run npm run test:unit and npm run test:integration, but neither script exists in package.json; the only configured test command is npm test. These instructions fail immediately for contributors unless the missing scripts are added.

# Unit tests (run these before every commit)
npm run test:unit

# Integration tests (requires a live FusionAuth instance)
npm run test:integration

CONTRIBUTING.md:15

• This universal policy is already violated by src/commands/lambda-delete.ts:13-18, which performs an irreversible delete without confirmOrExit() and exposes no --yes option. Either update existing destructive commands as part of this policy or scope the statement to commands covered by this change.

Commands that perform risky operations must gate execution behind user confirmation using `confirmOrExit()` from `src/utils.ts`. All such commands must expose a `--yes` flag.

CONTRIBUTING.md:15

• This policy also classifies the existing kickstart:stop command as requiring confirmation because it is potentially disruptive, but that command still runs docker compose stop without confirmOrExit() or --yes (src/commands/kickstart-stop.ts:17-41). Either update that command or narrow the policy to irreversible/destructive operations so the new contributor guidance is internally consistent.

Commands that perform risky operations must gate execution behind user confirmation using `confirmOrExit()` from `src/utils.ts`. All such commands must expose a `--yes` flag.

src/commands/import-generate.ts:30

• These aliases are already labeled deprecated and hidden from help, so saying they “will be deprecated in a future release” is misleading. Tell users that the flag is deprecated and will be removed in a future release.

      `${old} will be deprecated in a future release.`

src/commands/kickstart-install.ts:20

• Because this expression is not anchored, a value such as not-an-email admin@example.com passes validateEmail() and is written to the generated kickstart file. The new --admin-email path therefore accepts malformed addresses; require the entire input to match the address pattern (for example, add ^ and $).

export const EMAIL_REGEX = /(([^<>()[\]\\.,;:\s@"]+(\.[^<>()[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))/;

src/commands/kickstart-install.ts:232

• Failures in the file-copy, kickstart-generation, or environment-file steps are caught here and only logged, so the command resolves with exit code 0 even though the installation is incomplete. Report the failure through errorAndExit (or otherwise exit nonzero) so unattended callers can detect it.

  } catch (e) {
    console.error(e)
  }

src/commands/kickstart-install.ts:61

• “intial” is misspelled; please change it to “initial” in this user-facing function documentation.

 * We need the intial admin's credentials (email and password) and a name for a

src/commands/kickstart-install.ts:217

• The error message uses the plural verb exists with the singular subject directory; change it to does not exist.

    if (!fs.existsSync(directory)) throw (chalk.red(`Something went wrong. ${directory} does not exists.`))

src/commands/kickstart-install.ts:242

• The new Commander options are not exercised through kickstartInstall.parseAsync(); the tests call resolveInstallAnswers() directly. A wiring error in these .option() declarations or in passing options to action() would therefore leave the documented non-interactive install path broken while all current tests pass.

  .option('--admin-email <email>', 'Admin user email address (skips prompt)')
  .option('--admin-password-env <ENV_VAR>', 'Name of environment variable containing the admin password (skips prompt)')
  .option('--application-name <name>', 'Application name (skips prompt)')
  .action((dir, options) => action(dir, options))

• Files reviewed: 12/12 changed files
• Comments generated: 0 new
• Review effort level: Lite

[andrewpai]

@andrewpai shouldn't this merge to the 'next' branch?

Done.

[Copilot]

🔵 Needs a closer look

Unresolved moderate findings affect contributor commands, install error handling, documentation, and CLI wiring coverage.

Review details

Suppressed comments (7)

Previously missed (3) — in code that hasn't changed since the last review.

src/commands/kickstart-install.ts:230

• If any post-prompt operation fails (for example cpSync, createKickstart, or renameSync), this catch only logs the error; the spinner started at line 207 is never stopped. That can leave the terminal animation running, and potentially keep the CLI alive, instead of returning a clean failure. Keep the spinner in scope and stop or fail it in this error path.
  src/commands/kickstart-install.ts:241
• The new public flags are only exercised by calling resolveInstallAnswers with hand-built option objects; no test parses kickstartInstall with --admin-email, --admin-password-env, or --application-name. A Commander option/handler wiring regression could therefore leave these documented flags ineffective. Add a command-level parse test (or an injectable action seam) for the CLI spellings.
  src/commands/import-generate.ts:30
• This warning calls the option deprecated but then says it “will be deprecated,” which is contradictory and leaves the removal status unclear. Since the aliases are already deprecated, say they are deprecated and will be removed in a future release.

CONTRIBUTING.md:26

• These commands are not defined in package.json: contributors following this section will get Missing script errors for both test:unit and test:integration. The repository and CI expose npm test as the test entry point, so update this block or add the missing scripts before documenting them.

npm run test:unit

# Integration tests (requires a live FusionAuth instance)
npm run test:integration

src/commands/kickstart-install.ts:60

• intial is misspelled in this new documentation comment; please correct it to initial.

 * We need the intial admin's credentials (email and password) and a name for a

src/commands/kickstart-install.ts:216

• The error message has a subject/verb agreement error: does not exists should be does not exist.

    if (!fs.existsSync(directory)) throw (chalk.red(`Something went wrong. ${directory} does not exists.`))

src/commands/kickstart-kill.ts:62

• The new --yes option registration is not exercised: all tests call the exported action({ yes: ... }) directly. A Commander parsing regression could make the real CLI ignore --yes while these tests remain green; add a kickstartKill.parseAsync(['--yes'], { from: 'user' }) coverage case.

  .option('--yes', 'Skip confirmation prompt', false)

• Files reviewed: 13/14 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Address spinner cleanup and add end-to-end CLI parsing coverage; correct the import flag deprecation wording.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

src/commands/kickstart-install.ts:233

• If any filesystem step after yoctoSpinner(...).start() throws, this catch only sets process.exitCode; it never stops the active spinner. The failed install can therefore leave the animation running/terminal state unclean (and may keep the CLI alive), so retain the spinner reference and stop or fail it in this error path before exiting.
  src/commands/kickstart-install.ts:243
• The tests call resolveInstallAnswers directly, so they never parse kickstart:install with Commander. A regression in any of these new kebab-case flags or Commander’s camel-casing would still pass while the real CLI receives undefined options and prompts; add a parseAsync test supplying all three flags (and an environment variable for the password) and assert the mapped fields reach the action.
  src/commands/import-generate.ts:30
• The aliases are already marked deprecated in DEPRECATED_FLAGS and their option descriptions, but this warning says they “will be deprecated in a future release.” That reverses their current status and can mislead users; state that the flag is deprecated and may be removed in a future release.

• Files reviewed: 12/13 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved telemetry cleanup races and an install-failure spinner leak remain; the deprecation warning also needs correction.

Review details

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

src/commands/import-generate.ts:30

• This warning says the camelCase flag "will be deprecated in a future release", but these aliases are already marked deprecated in DEPRECATED_FLAGS and in their hidden option descriptions. That gives users the wrong lifecycle status; say that the flag is already deprecated and will be removed in a future release (or provide the removal version).

tests/telemetry/telemetry.test.js:91

• telemetryEnable.parse() has the same un-awaited logEvent() behavior, so this cleanup can run before PostHog sends the request and make the test intermittently perform a real network call. Synchronize the command's telemetry flush before removing the interceptor.

        nock.cleanAll()

tests/telemetry/telemetry.test.js:77

• telemetryDisable.parse() invokes an async action that calls logEvent() without awaiting it, so its PostHog flush can still be pending when this finally runs. Cleaning the persistent interceptor here can remove it before that request is issued, causing the test to fall through to a real network request and become flaky; await the telemetry action/flush before cleaning the interceptor (or otherwise synchronize cleanup).

        nock.cleanAll()

src/commands/kickstart-install.ts:233

• If any install step after spinner.start() throws, this catch only sets process.exitCode and leaves the spinner running. The failure path can therefore keep the CLI's spinner/timer active or leave terminal output in progress instead of cleanly terminating; keep the spinner in scope and stop it (or mark it failed) before returning the error.

  } catch (e) {
    console.error(e)
    // Ensure a failed install (copy, kickstart write, rename, env update, etc.)
    // is reflected in the process exit code rather than silently exiting 0.
    process.exitCode = 1

• Files reviewed: 18/19 changed files
• Comments generated: 0 new
• Review effort level: Lite

[mark-robustelli]

Looks good to me.

[brob]

LGTM