Repository navigation
fix(create-bestax): never empty a non-empty folder under -y, and validate flags first - #954
Conversation
…date flags first -y used to empty an existing folder without asking, before the other flags were checked, so a typo in -t left the folder empty with no scaffold. Every flag is now validated before any prompt runs or anything on disk changes. A non-empty folder under -y, or with no terminal to ask in, stops the run with exit code 1 and lists what is there. The new --overwrite flag, named after create-vite's, is what removes the files. Accepting the interactive "Remove existing files" question now only records the answer, and the folder is emptied after the last prompt, so a later Ctrl+C leaves it alone. This is a change to -y. A script that relied on it to empty a folder now needs --overwrite, and the help text and README say so. Two items from #950 live in the same flow. Ctrl+C at the skills prompt now cancels like every other prompt instead of reading as "no". The package.json name is lower-cased and loses a leading underscore, the way npm init and create-vite derive one, since npm rejects both. Fixes #945 Refs #950
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 44 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (14)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Preview DeploymentPreview URL: https://638c81e0.bestax.pages.dev |
|
deep-review: fresh The previous run was cancelled at the runner limit before it posted anything, so this asks for the same full review again. The behavior change lives in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep review — 1 blocking · 2 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | API | Documented break to -y releases as a patch; no BREAKING CHANGE: footer, though release.config.js already has a breaking → major rule |
create-bestax/README.md:98 |
| 2 | 🔵 Advisory | Robustness | A target path that exists as a file throws a raw ENOTDIR out of create() — pre-existing shape, carried into the new helper |
create-bestax/src/file-system.ts:23 |
| 3 | 🔵 Advisory | Robustness | The interactive "Remove existing files and continue?" prompt still does not list the files, though entries is in hand and the refusal path lists them |
create-bestax/src/project-creator.ts:121 |
Overall: The change is sound and the ordering is right — validate flags → resolve and validate the name → decide about the directory → ask everything → then empty. I built create-bestax/dist from this branch and ran #945’s two repros plus seven adjacent paths against the real binary: demo -y and demo -t vite-typescript -y both leave .gitdir, notes.txt, src untouched and exit 1; -t vite-typescript -y --overwrite also refuses before deleting anything; --overwrite with every flag and no TTY empties then scaffolds; a non-empty directory with no TTY and no --overwrite gets the --overwrite refusal rather than the -y-suggesting NO_TTY text; MyApp→myapp, _under→under, My.App→my.app, and _/___ are rejected. 302 unit tests pass, coverage 98.91% / 92.55% branches, lint and typecheck clean. The riskiest part is not the code — it is the release type (finding 1); look at that first, because it is the one thing neither this review nor a fix agent can set.
Residual risk:
--overwritecan still empty-then-fail.validateOptionschecks the template name, not thatgetTemplatePath(template)exists, so on a partial installcopyDirectorythrowsTEMPLATE_NOT_FOUNDafteremptyDirectory. Open, but low:filesinpackage.jsonshipstemplates, both template directories are in the tarball, and--overwriteis explicit consent to the delete. The un-consented version of this shape (-yalone) is gone.-ycallers inside this repo. Refuted.create-bestax/e2e/utils/scaffold.ts:44-47removesoutputDirbefore scaffolding, so the directory is always missing and the plan is "empty".eval/agent-loop/bin/run-batch.sh:67clears its work dir per run. A lonerun-iteration.shwith a reused work-dir now fails instead of silently emptying the app folder, which is what that script’s ownset -euo pipefailand its "a broken scaffold is INFRA failure" comment ask for.- Names that survive normalization. Refuted for the cases that matter: only strings of
_/.(never leading.) normalize to empty, andvalidateProjectNamenow rejects those._-becomes package name-, whichvalidate-npm-package-nameaccepts (no leading./_, lowercase, URL-safe), and mid-name_/.are legal npm characters — so keeping them, rather than following create-vite’s collapse of every non-alphanumeric to a dash, is a deliberate difference and not a gap. - A partially-emptied directory. Refuted.
emptyDirectoryis now reached from exactly one place (project-creator.ts:608), after the last scaffold prompt, and there is no otheremptyDir/removecall anywhere insrc/. - Listing surfaces. Refuted.
docs/docs/guides/distribution.md’s "What goes stale" has no bullet for create-bestax flags, and no docs page carries a CLI options table — the README and--helpare the only two, and both changed here. - Not a risk, but worth knowing: the telemetry question is asked after the scaffold, so it lands after the emptying. The README’s "nothing is removed until you have answered the last question" sits under a five-item list that ends at the skills prompt, which is accurate for that list.
🏄 Total cleanup on a gnarly wipeout —
-yused to drop in and bulldoze your whole beach, now it just paddles back and tells you what is sitting there. Ran the real binary through the original repros and it holds. Only thing left is whether this rides out as a 4.2.13 or a 5.0.0, and that is the captain’s call.
…stop on a file path The interactive "Remove existing files and continue?" question named only the directory, while the refusal under -y listed what was in it. The question that actually deletes files now lists them the same way. A project name that matched an existing file crashed with a raw ENOTDIR stack, because the directory listing ran on a file. It now stops with a plain message and exit code 1 before anything is read, whatever the flags say. Refs #945
|
deep-review: verify Every open thread has a reply: fixed, with the commit, or refuted with a reason. This settles them on 39b6232. |
Preview DeploymentPreview URL: https://393ca2b3.bestax.pages.dev |
There was a problem hiding this comment.
Deep review (verify) — 3 resolved · 0 open
| # | Severity | Area | Thread | Outcome | Location |
|---|---|---|---|---|---|
| 1 | 🟡 Minor | API | Documented break to -y ships as a patch with no BREAKING CHANGE: footer |
✅ Accepted — reasoned and deliberate, now on the record | create-bestax/README.md:98 |
| 2 | 🔵 Advisory | Robustness | Target path that exists as a file crashed with a raw ENOTDIR stack |
✅ Verified fixed | create-bestax/src/file-system.ts:34 |
| 3 | 🔵 Advisory | Robustness | The one prompt that deletes files did not say which files | ✅ Verified fixed | create-bestax/src/project-creator.ts:121 |
What I checked
- #1 — refutation accepted. The reply is from
allxsmith(live roleadmin) and gives a reason rather than a bare "out of scope": the old-yemptied a folder before the other flags were even checked, which is data loss rather than a contract. That was the second of the two options the original finding offered, and the choice is now explicit in the PR body ("It ships as a patch on purpose…"), increate-bestax/README.md:97-98, and in the-yrefusal itself, which names--overwrite. A reasoned deliberate choice — accepted, not re-raised. - #2 — fixed.
existsAsNonDirectory(file-system.ts:23-29) usesfs.statSync(path, { throwIfNoEntry: false }), andcheckExistingDirectory(project-creator.ts:109-112) calls it beforelistDirectoryEntries, so nothing reads the path andENOTDIRcan no longer be thrown. It also sits ahead of the--overwritebranch, so a file path stops even with that flag —project-creator.test.ts:261-279passes{ yes: true, overwrite: true }and assertslistDirectoryEntriesis never called. The dedicated check the finding recommended was taken over the empty-listing alternative. - #3 — fixed.
promptOverwriteDirectory(targetDir, entries)(project-creator.ts:128) threads the listing throughprompts.ts:40-50intoMESSAGES.DIRECTORY_NOT_EMPTY(constants.ts:43-44), which renders through the same cappedlistEntrieshelper the-yrefusal uses, so the path that deletes files now names them.project-creator.test.ts:195-213asserts the prompt text contains(notes.txt)and thatemptyDirectoryis still not called at answer time;:281-294holds the cap. - Test run:
pnpm --filter create-bestax run test— 9 suites, 306 tests, all passing. Empirical CLI invocations were not possible in this session: the sandbox blocks filesystem writes outside the repository, so the assertions above rest on the unit tests plus the code paths rather than on a freshdistrun.
Overall: This pass settled threads and reviewed no commits. All three previously open threads are now closed — two fixed in 39b6232b and verified against the current code, one accepted on a reasoned refutation from an admin. Nothing is left open, and no new findings were raised, by design; a maintainer wanting the current code reviewed afresh should post a deep-review: fresh steer.
Residual risk: Not assessed — a verify pass settles existing threads and does not hunt for new failure modes. The two fixes verified here were checked against the tests that pin them, so the specific defects they addressed are closed.
🏄 Three threads paddled in clean — two patched up proper, and one where the maintainer called the drop right on patch-versus-major. Board is waxed, no chop left in the lineup. Good to go, dude.
github-actions
Bot
added
the
review-converged
allxsmith
added
the
needs-human-review
📸 Story screenshots at handoff —
|
|
🎉 This PR is included in version 5.27.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.13 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.25.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.14.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
-yemptied an existing folder without asking, and did it before the other flags were checked. So-t vite-typescript -yemptied the folder and then failed on the template, leaving neither the user's files nor a scaffold. Without a terminal, the message for a non-empty folder suggested-yas the way through. In the interactive flow, answering yes to "Remove existing files and continue?" emptied the folder right away, so a Ctrl+C at a later prompt left it empty.Every flag value is now validated before any prompt runs or anything on disk changes.
-yno longer deletes anything. A non-empty target folder under-y, or with no terminal to ask in, stops the run with exit code 1, lists what the folder holds, and points at the new--overwriteflag. The flag takes create-vite's name and description, since create-next-app has no override at all, and it also answers the interactive question. An interactive yes is now only recorded, and the folder is emptied after the last prompt, so a Ctrl+C anywhere before that leaves the files alone.This is a behavior change to
-y, made because it was losing people's files. It ships as a patch on purpose, since the old behavior was a data-loss bug rather than a contract. A script that relied on-yto empty a folder now needs--overwrite. The--helptext, the README and the package's CLAUDE.md say so. The agent-loop eval'srun-iteration.shscaffolds with-yinto a work folder its README expects to be fresh per run; a reused one now stops with the refusal instead of being emptied silently.Two items from #950 live in the same flow and are fixed here. Ctrl+C at the skills prompt now cancels like every other prompt, where before it was read as "no" and the app was scaffolded without skills. The package.json name is now lower-cased the way
npm initdoes it and loses a leading underscore the way create-vite does it, since npm rejects both. The folder keeps the name as typed, and the CLI prints the package name when it differs. A name that would leave nothing, such as one made only of underscores, is rejected. A project name that matches an existing file now stops with a plain message instead of a stack trace, and the interactive overwrite question lists what the folder holds, as the-yrefusal does.The other #950 items aren't in this PR: flags under
bun createand the CLI's runtime dependency on the library.pnpm allpasses locally.Fixes #945
Refs #950