fix(cli): validate init flags before creating the project directory#1207
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
A small case, but instructive in its discipline. Read at head 4b2762e1, base main, single author hand throughout — and a clean parallel-PR field (#1206, #1208, #1404 each branched independently off main from the same author; no Graphite stack, no stale-base hazard to map).
What the diff actually does
packages/cli/src/commands/init.ts:686-715 reorders the non-interactive block so the three pre-write checks — --video/--audio flag conflict, --video <path> existence, --audio <path> existence — fire before the mkdirSync(destDir, { recursive: true }) at line 715. The pre-existing duplicate-existence checks inside the per-flag branches at init.ts:721-737 are correctly retired in favour of the now-resolved videoPath / audioPath locals, so handleVideoFile(videoPath, destDir, false) at line 724 and copyFileSync(audioPath, ...) at line 735 consume the same already-validated values. No dispatch-chain orphan, no resurrected re-resolve, no silent change of error messages or exit codes.
Why the framing convinces
The PR body's stated invariant — every other early validation already rejects before mkdirSync — survives a walk of the file:
--template/-V/--resolutionallprocess.exit(1)atinit.ts:64,:125,:139and:671-680, well above the non-interactivedestDirresolution.- The
existsSync(destDir) && readdirSync(destDir).length > 0not-empty guard atinit.ts:691-694is correctly the first check inside the non-interactive block and stays first. - The interactive twin at
init.ts:862-884resolves andexistsSync-checks bothvideoFlagandaudioFlagbefore itsmkdirSync(destDir)calls — the PR body's parallel claim verified. (ThevideoFlagvsaudioFlagprecedence asymmetry there —if … else if …rather than a hard conflict — is deliberately scoped out in the body and is, I agree, a UX question rather than a defect of this fix.)
Tests earn their keep
init.test.ts:147 strengthens the pre-existing -v missing.mp4 case with the existsSync(target) === false assertion the sibling tests already use; without the reorder, that assertion fails. Two new cases — --audio missing.mp3 and --video + --audio together — cover the remaining two failure paths the body enumerates, each ending with the same existsSync(target) === false invariant. Three pass with the fix, three would fail without it; the test suite locks the door against future regressions of this exact category. Full CLI suite reported green at 690/690, and every CI check on the rollup is SUCCESS.
Scope honesty, and what is deliberately not fixed
I noticed two further process.exit(1) paths downstream of the mkdirSync that also leave an orphan directory: the scaffold-failure case at init.ts:778 (template fetch / copy failure) and an implicit one if handleVideoFile at :724 throws on probe/transcode. Both are different failure families from "flag validation in non-interactive mode," and the PR body's enumeration of "three failure paths" is correct for the category it claims to fix. Bundling them in would either widen the diff into a structural cleanup (e.g. a try/catch around the whole non-interactive scaffold with cleanup of destDir on failure) or quietly conflate two bug shapes. Leaving them as separate work, with a clear in-PR scope statement, is the right call. Worth mentioning in case the author wants to file a sibling ticket — but not a blocker on this one.
Verdict
Clean — ready for stamp at 4b2762e1.
The reorder is minimal, the dispatch chain reads correctly under the new shape, no defensive-guard downgrades or contradictory rules were introduced, and the tests pin the precise invariant the body claims. Re-verify if HEAD moves before stamp.
Review by Via
Problem
In non-interactive mode,
hyperframes initcreates the project directory before validating the source-file flags. Three failure paths exit after themkdirSync:--videoand--audiopassed together--video <path>where the file does not exist--audio <path>where the file does not existEach one leaves an empty orphan directory behind in the caller's CWD. For CI and agent loops (the audience of
--non-interactive) a failed scaffold that still mutates the filesystem is surprising: a retry with a corrected flag passes the "directory already exists and is not empty" check only because the orphan happens to be empty, and an aborted run pollutes the workspace.The other early validations already get this right:
--template,-V, and--resolutionall reject before any filesystem write, and their tests assertexistsSync(target) === false. The interactive path also validates the source file before itsmkdirSync. The non-interactive video/audio path was the only one ordered the other way.Fix
Reorder the non-interactive block: flag-conflict check and source-file existence checks first,
mkdirSync(destDir)after. The existence checks reuse the already-resolved paths, so the video/audio handling below switches from re-resolvingvideoFlag/audioFlagto usingvideoPath/audioPathdirectly. No messages or exit codes changed.Not touched: the interactive path resolves
--video/--audiowith else-if precedence (video wins) rather than rejecting the combination. That asymmetry predates this change and is a UX question rather than a bug, so it stays as is.Tests
-v missing.mp4case with theexistsSync(target) === falseassertion the sibling tests already use (it would have failed before this fix).--audio missing.mp3exits 1 with no directory created.--video+--audiotogether exits 1 with no directory created.All three fail on main and pass with the reorder. Full cli suite: 690/690.