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
| export const kickstartKill = new Command() | ||
| .command('kickstart:kill') | ||
| .description('Runs docker compose down in current directory') | ||
| .option('--yes', 'Skip confirmation prompt', false) | ||
| .action(action) |
| export async function confirmOrExit(message: string, yes: boolean): Promise<void> { | ||
| if (yes) return; | ||
|
|
||
| console.warn(chalk.yellow(message)); | ||
|
|
||
| if (!process.stdin.isTTY || !process.stdout.isTTY) { | ||
| errorAndExit('Pass --yes to confirm this operation non-interactively.'); | ||
| return; | ||
| } |
--yestokickstart:killkickstart:installimport:generateoptions to use kebab case with ongoing but deprecated camelCase support