Andrewpai/yes flag - #55
Conversation
There was a problem hiding this comment.
🟡 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 wiredkickstart:killto support--yesand non-interactive confirmation gating. - Enhanced
kickstart:installto 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:generateoption 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.
- import-generate: detect deprecated flags in --flag=value form, not just bare --flag - kickstart-install: replace setTimeout-chained install steps with sequential awaited steps so errors propagate through try/catch and ordering is deterministic; also await createKickstart (was previously fire-and-forget) - utils: confirmOrExit now requires both stdin and stdout to be TTYs before treating the session as interactive, and normalizes confirmation input (trims whitespace, accepts y/yes case-insensitively)
There was a problem hiding this comment.
🟡 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 -vfails (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 readablestarting.stdoutstream (it will be null), so the subsequentfor 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
- utils.ts: extract isConfirmationAccepted() as a pure, exported function so the accept/reject decision logic can be unit tested directly without simulating a real TTY - kickstart-kill.ts: export action() and add an injectable deps parameter (isDockerInstalled, confirmOrExit, spawn) so tests can exercise the confirmation gating without touching real docker or exiting the process - add __tests__/utils.test.js covering isConfirmationAccepted and the yes-bypass / non-interactive TTY-detection paths of confirmOrExit - add __tests__/commands/kickstart-kill.test.js covering docker-not-installed, CLI_DIR mismatch, --yes bypass, and confirm-rejected gating paths - wire both new test files into the test and test:unit npm scripts
There was a problem hiding this comment.
🟡 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
…erred Previously, the rl.question callback called process.exit(0) on decline but had no return statement, so resolve() ran unconditionally afterward. In production this was masked because process.exit halts execution synchronously, but in any environment where exit is mocked or deferred (e.g. tests), a declined confirmation would be silently treated as accepted, letting the caller proceed with the risky operation. - extract handleConfirmationAnswer(answer, resolve, reject): resolves on accept, exits + rejects on decline, so the promise can never silently resolve when exit doesn't actually happen - confirmOrExit now passes both resolve and reject into handleConfirmationAnswer - add 3 tests in __tests__/utils.test.js covering accept, decline, and the decline-with-mocked-exit case that reproduces the original bug
There was a problem hiding this comment.
🟡 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 readFileis imported fromfs/promisesbut 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
- extract getDeprecatedFlagUsage(argv) as a pure, exported function so the deprecation-detection logic is testable without mocking process.argv or console.warn - export DEPRECATED_FLAGS for use in tests - add __tests__/commands/import-generate.test.js covering: no deprecated flags used, bare --flag and --flag=value forms detected, multiple deprecated flags detected together, new kebab-case form not flagged, and that both the deprecated and current flag spellings populate the same underlying Commander option property - wire the new test file into the test and test:unit npm scripts
There was a problem hiding this comment.
🔵 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
- kickstart-install.ts: import randomUUID from node:crypto explicitly instead of relying on the global WebCrypto object, matching the convention already used elsewhere in the codebase - utils.ts: confirmOrExit() now throws after errorAndExit() in the non-interactive path, mirroring the fix already applied to handleConfirmationAnswer in the interactive path. In production this is a no-op since process.exit(1) halts synchronously first, but in any environment where exit is mocked/deferred, the promise now rejects instead of silently resolving and letting the caller proceed with the risky operation - update the three non-interactive tests in __tests__/utils.test.js to assert.rejects, which now actually exercises the fixed behavior
|
Addressed both items flagged in the latest review (5160436306) in 4c3ccc7:
|
There was a problem hiding this comment.
🟢 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
--yestokickstart:killkickstart:installimport:generateoptions to use kebab case with ongoing but deprecated camelCase support