fix(build): refuse to clear an output directory no build produced - #3601
Merged
Merged
Conversation
`veryfront build` emptied its output directory before writing, so a project that already kept something in `dist/` — its own bundle, a checked-in artifact, anything — lost it. The CLI printed nothing about the deletion: the developer saw `✓ Built in 1.35s` and discovered the loss later, from the absence. The same happened to whatever `build.outDir` pointed at, so moving the framework out of the project's `dist/` only moved the target. A directory that holds `_veryfront/` is a previous build's output: the setup step creates that entry before anything else is written, so every directory the build has ever owned carries it, including one left by an older release or by a build that failed halfway. Deleting anything else is not a step a developer can be assumed to have consented to, and it cannot be undone, so the build now stops and says what it found and where to send its output instead. Empty and absent directories are unaffected, so first builds and rebuilds over our own output behave exactly as before — and an absent directory is no longer "removed", which also drops the `! Operation failed, using fallback err=NotFound ... remove '.../dist'` line every first build printed. Constraint: Existing projects must keep rebuilding without an opt-in flag, so ownership has to be inferred from output an older release already wrote. Rejected: Warn and delete anyway | The files are still gone; a warning after an irreversible deletion is a report, not a choice. Rejected: Delete only entries a build wrote | Requires a manifest no previous release left behind, and still guesses at the rest. Confidence: high Scope-risk: narrow Reversibility: clean Tested: build-setup unit tests (foreign dir refused and intact, empty dir cleared, absent dir not removed, our own dir still cleared of stale files); src/build + cli/commands/build suites; tests/integration/server/build; the finding's own repro against a published-0.1.1229 scaffold — seeded dist/index.js, dist/IMPORTANT.txt, dist/nested/deep.txt survive with EXIT=1, and the same for a configured build.outDir Not-tested: hosted/cloud builds, which stage into a fresh per-build directory and so never reach the refusal
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughBuild setup now checks output-directory ownership before deletion. It rejects unreadable or non-empty foreign directories, while allowing absent, empty, and previously marked Veryfront directories. Tests cover these cases and message path redaction. ChangesOutput Directory Safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 805720ae44
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review of the refusal found three ways it still authorized the deletion it
exists to stop.
An output directory that could not be listed — no permission, a transient
filesystem error, a path that is a regular file rather than a directory — was
treated as ours and removed recursively. A failed inspection proves nothing
about what is there, so it now refuses and says so, the same way a foreign
directory does.
Ownership was claimed by any entry named `_veryfront`. The marker is the
directory this step creates, so a project that keeps a file or a symlink of
that name in its own output had everything beside it deleted. Only a directory
of that name counts now.
The refusal interpolated the output path, which the CLI has already resolved to
an absolute path, putting the machine's filesystem layout into human and
`--json` output alike (AGENTS.md, "Secret and internal-detail safety"). It now
names the path the developer configured — `dist`, `custom-out` — falling back
to the last segment for an output outside the project rather than printing a
chain of `../`.
Confidence: high
Scope-risk: narrow
Reversibility: clean
Tested: build-setup unit tests (uninspectable output refused and intact,
`_veryfront` file refused, refusal free of machine paths) plus the existing
cases; src/build/production-build + cli/commands/build suites; against a
published-0.1.1229 scaffold with a local build — foreign dist/ and a foreign
`build: { outDir }` refused with their seeded files intact, dist/ as a regular
file refused, first build into an absent dist/ and a rebuild over our own
output still green with stale artifacts cleared
Not-tested: a directory readable by stat but not by readdir, which needs a
permission setup the suite cannot make portably; covered by injecting the
listing failure instead
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/build/production-build/build/build-setup.test.ts`:
- Around line 38-42: Replace all direct Deno.* filesystem calls in the test
file, including the fs.remove and fs.readDir implementations and the additional
affected test paths, with the existing runtime-neutral filesystem helpers. Keep
the ownership-safety tests’ behavior unchanged while ensuring no \bDeno\.
references remain anywhere in the file.
In `@src/build/production-build/build/build-setup.ts`:
- Around line 113-123: Sanitize each arbitrary filesystem name before
constructing the refusal message in the build-setup flow: escape control and
terminal characters, redact or otherwise safely handle sensitive identifiers,
and enforce a maximum displayed length per name. Update the names mapping used
by preview and add coverage for an entry name containing control characters,
while preserving the existing preview limit and refusal behavior.
- Around line 35-40: Update the output-directory cleanup flow around
outputDirectoryNeedsClearing and handleErrorWithFallback so ownership validation
and recursive removal use the same directory identity atomically. Replace the
separate validation-then-remove sequence with an adapter operation that binds
the check to deletion, or detect and fail if outputDir is replaced between those
steps; preserve the existing error fallback behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fec81f3e-3343-46ee-8242-2c84b8716ae6
📒 Files selected for processing (2)
src/build/production-build/build/build-setup.test.tssrc/build/production-build/build/build-setup.ts
The refusal lists what it found in the output directory, and those names come off the filesystem: a file can be called anything a filesystem allows, control characters and ANSI escapes included. Printed straight, a name like `\x1b[2Kfake.txt` does not appear in the message — it rewrites the line, and the sentence it erases is the one that tells the developer what to do about the refusal. Control characters now render as `?`, the way `ls` shows them, and a long name is cut so one entry cannot push the rest off screen. The output path goes through the same treatment, since a project directory can be named the same way. Confidence: high Scope-risk: narrow Reversibility: clean Tested: build-setup unit tests, including a new case that writes a file whose name carries an ANSI erase sequence and asserts the message carries no control character at all; the refusal, first-build and rebuild paths re-driven against a published-0.1.1229 scaffold
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What broke
veryfront buildempties its output directory before writing. It never checkedwhose directory it was, so a project that already kept anything in
dist/lostit — silently. Reproduced against published 0.1.1229 (Deno-installed CLI,
sandbox outside the platform checkout):
Not a word about clearing, emptying or removing anything, and exit 0. The same
happened to a configured
build.outDir, so the fix that madebuild.outDirbehonoured only moved the target.
What changed
setupBuildDirectoriesnow decides whether the output directory is the build'sto delete, and deletes only what it can show is its own:
_veryfront/directory → a previous build's output, cleared asbefore (the setup step creates that entry before anything else is written, so
every directory the build has ever owned carries it, including from older
releases and from builds that failed halfway — no new marker file, no upgrade
cliff). A file or symlink of that name is not that directory and does not
count;
! Operation failed, using fallback err=NotFound ... remove '.../dist'linethat every first build printed);
is a regular file → refused, because a failed inspection proves nothing about
what is there;
[build-failed]naming the directory,the entries it found, and the two ways out.
The message names the path the developer configured. The CLI resolves the
output to an absolute path before the build sees it, and printing that would
put the machine's filesystem layout into human and
--jsonoutput alike; anoutput outside the project falls back to its last segment rather than a chain
of
../.The entry names go through the same filter. They come off the filesystem, so a
file can be called anything a filesystem allows: printed straight, a name like
\x1b[2Kfake.txtwould not appear in the message but rewrite the line, erasingthe sentence that says what to do about the refusal. Control characters render
as
?, and long names are cut.Warning and deleting anyway was rejected: the files are still gone, and a
warning after an irreversible deletion is a report, not a choice.
Verification
Regression tests in
src/build/production-build/build/build-setup.test.ts(written first; the refusal cases failed with "Expected function to reject"
and the seeded files deleted before the fix):
without naming the machine's paths in the message;
removewas never called;_veryfrontis a file rather than a directory;left in the message;
_veryfront/.Against a build of this branch, driving the finding's own repro in the same
published-0.1.1229 scaffold:
dist/{index.js,IMPORTANT.txt,nested/deep.txt}build.outDir: "custom-out"holdingKEEP.txtKEEP.txtintactdist/_veryfrontas a file, beside a sentineldistas a regular filedist/removed, first builddist/with a stale file--dry-runover a foreigndist/Suites:
src/build/production-build,cli/commands/build, and the fullpre-push gate — all green.
Notes
No documentation change is needed for this one; the behaviour is now
self-describing at the point of failure. Hosted builds stage into a fresh
per-build directory and never reach the refusal.