Ask the cleanup question of a directory the demo test owns - #368
Merged
Conversation
CommitLore — record lintTrailers: clean — 1 commit in Active constraints for the paths this PR touchesLimits (75)
Ruled out (169)
Warnings (45)
Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR. |
Two tests in `test/demo.test.ts` decided whether `runDemo` had left its temporary directory behind by listing the process-wide `tmpdir()` and filtering for `commitlore-demo*`. That namespace belongs to every process on the machine, so under `npx vitest run` the answer came back from whichever worker happened to be holding its own demo directory at that moment. Both went red in two separate worktrees, naming a different leftover directory each time, and both passed in isolation -- the shape that teaches a reader to discount red, which is the one signal the commit gate depends on. The first took a before/after delta, which narrows the window without closing it: a directory another worker creates after the `before` snapshot and still holds at the `after` snapshot is absent from `before` and present in `after`, so it reads as leftover. The second took no delta at all and failed if any `commitlore-demo*` directory existed anywhere in `tmpdir()`, whoever made it. The property is worth keeping exactly as stated -- the demo must remove its temporary directory, including when it crashes mid-run -- so the fix gives the assertion a namespace it owns rather than softening it. `runDemo` now takes a `tmpRoot` defaulting to `tmpdir()`, and the suite creates one root per case and asserts that root is *empty*: no prefix filter, so nothing anyone else wrote can either satisfy or break it, which is a stronger claim than the one it replaces. Routing every demo in the file through that root also stops the suite writing `commitlore-demo-*` into the shared tmpdir, where a run in another checkout would have read it -- this suite was itself a plausible source of the failures it suffered. An option rather than an env override, because the production default has to be unmistakable: `runDemo()` with no argument is still `tmpdir()`, decided at the one line that creates the directory, where an ambient `COMMITLORE_DEMO_TMPDIR` could redirect a real `commitlore demo` run with nothing at the call site saying so. Record-Id: r-owntmproot Limit: the demo still defaults to the shared tmpdir, so concurrent `commitlore demo` runs still create sibling directories there -- that is deliberate, and it is safe only because nothing now asserts over that namespace Ruled-out: An env override such as COMMITLORE_DEMO_TMPDIR | it moves the production default off the call site, where an ambient variable can redirect a real run and nothing in the code reads as changed Ruled-out: Keeping the before/after delta and widening it | the delta narrows the window rather than closing it, and the directory that turned this red was created inside the window it leaves open Ruled-out: Deleting the two tests or dropping the prefix filter to make them pass | the property is real and cheap to hold, so that trades a flaky true signal for a permanent blind spot over cleanup after a crash Certainty: firm Blast: local Undo: easy Verified: reproduced deterministically before fixing -- a helper creating a `commitlore-demo-*` directory in the shared tmpdir every 25ms during the run, which is what a second worker does, gave `2 failed | 5 passed` with both cleanup tests printing the foreign directories; the same helper against this change gives 7 passed with 397 foreign directories live in tmpdir at exit; demo, cli and init suites 40 passed, full suite 78 files, 1942 passed with 1 skipped; the built bundle's `commitlore demo` still creates and removes under `tmpdir()` with no argument, counted 0 before and 0 after; typecheck clean and dist rebuilt Unverified: whether the two failures reported in the wild had any cause other than a concurrently held demo directory -- only that shape was reproduced
MongLong0214
force-pushed
the
fix-issue-364
branch
from
August 1, 2026 05:05
3df19e1 to
6543870
Compare
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
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.
Closes #364
The defect
test/demo.test.tsdecided whetherrunDemohad left its temporary directory behind by listing the process-widetmpdir()and filtering forcommitlore-demo*. That namespace belongs to every process on the machine, so undernpx vitest runthe answer came back from whichever worker happened to be holding its own demo directory at that moment.beforesnapshot and still holds at theaftersnapshot is absent frombeforeand present inafter, so it reads as leftover.commitlore-demo*directory existed anywhere intmpdir(), whoever made it.Deterministic reproduction
A helper creating a
commitlore-demo-*directory in the shared tmpdir every 25ms during the run — which is what a second vitest worker does — reproduces it every time.Against the unfixed code:
Against this change, with the same helper running: 7 passed, with 397 foreign
commitlore-demo-*directories live in tmpdir at exit.The change
The property under test is real and is preserved as stated — the demo must remove its temporary directory, including when it crashes mid-run. The fix gives the assertion a namespace it owns rather than softening it.
runDemotakes atmpRootdefaulting totmpdir(). The suite creates one root per case and asserts that root is empty — no prefix filter, so nothing anyone else wrote can either satisfy or break it. That is a stronger claim than the one it replaces. Routing every demo in the file through a suite-owned root also stops the suite writingcommitlore-demo-*into the shared tmpdir, where a run in another checkout would have read it — this suite was itself a plausible source of the failures it suffered.An option, not an env override, because the production default has to be unmistakable:
runDemo()with no argument is stilltmpdir(), decided at the one line that creates the directory. An ambientCOMMITLORE_DEMO_TMPDIRcould redirect a realcommitlore demorun with nothing at the call site saying so.Verification
test/demo.test.tsin isolation: 7 passeddemo+cli+init: 40 passednpm run typecheckclean;npm run buildrun anddist/committedcommitlore demowith no argument still creates and removes undertmpdir()— 0 demo directories before, 0 after