Repository navigation
test(bulma-ui): close the cwd-relative family instead of naming two of it - #725
Conversation
…f it Two paths in this case were still spelled out while everything around them was derived, and both were the last of their kind. The nested-manifest read asked `PKG_DIR/dist/types-cjs/package.json` — the one line the whole fix rests on, checked at a path the export map no longer decides. It now reads through `cjs`, so it asks the tree consumers are actually served. No mutant drove this one and I could not build a reachable green-but-wrong state from it: a manifest-only repoint needs a second byte-identical tree and none exists on a built `dist`. It removes the last assumed path from the case, which is worth it on its own. The outside-write watch is the one that mattered. Two named directories assert "nothing landed in THESE two places", not "nothing landed outside the tree" — a default spelled `'types-cjs'` or `'../types-cjs'` lands where neither looks, and only the sibling check notices, which is the assertion this case exists so as not to lean on. Enumerating more literals would be the same mistake at greater length. Every spelling that goes wrong here goes wrong the same way: it is cwd-relative. So run the guard from an empty sandbox and they all land inside it, whatever they are spelled, and the net becomes "the sandbox is still empty" — which nothing has to anticipate. The two real trees stay watched by name; they are absolute, a chdir does not move them, and they are the ones that have actually been hit. Driven, not assumed: with the mirror default mutated to the literal `'dist/types-cjs'`, the case fails on `the guard resolved a path against the working directory` — the new assertion, by name, ahead of the sibling. Watching the fixture's parent was the other candidate and it is unusable: `tree()` mkdtemps straight into the system tmpdir, so the parent churns from every other process on the machine.
|
Warning Review limit reachedNext included review available in 11 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: allxsmith/bestax/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
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://11e6e237.bestax.pages.dev |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Nothing cwd-relative escaped, whatever it was spelled. | ||
| assert.deepEqual( | ||
| readdirSync(sandbox), | ||
| [], | ||
| 'the guard resolved a path against the working directory' | ||
| ); |
There was a problem hiding this comment.
The sandbox misses the .. spelling the comment above names — 🟡 Minor · Coverage
What: process.chdir(sandbox) puts the cwd at the root of the watched tree, so only spellings that resolve downward land inside it. 'types-cjs' → <sandbox>/types-cjs ✅ and 'dist/types-cjs' → <sandbox>/dist/types-cjs ✅ (the mutant the commit message drove), but '../types-cjs' → join(tmpdir(), 'types-cjs') — a sibling of the sandbox, not a child. readdirSync(sandbox) is still [], this assertion passes, and the case falls through to the existsSync(${root}-cjs) sibling check on line 1013 for its failure.
Why it matters: '../types-cjs' is named two paragraphs up (line 959) as a spelling that "lands where neither looks, and only the sibling check below would notice, which is the assertion this one exists to not lean on" — and after this change it still does exactly that. The claim on lines 964‑966 ("all of them land inside it, whatever they are spelled") is not true of the .. member, so the family this case set out to close is closed for two of its three named spellings. It reverses my own round‑5 advisory on #715, which offered the chdir'd sandbox as the general form without noting that a sandbox you stand in only nets downward paths.
Fix: stand one level down, so a single .. still lands inside the watched tree. dirname(root) stays out of it — the watched root here is a mkdtemp this case owns, not the churning system tmpdir that made the other approach unusable.
- const sandbox = mkdtempSync(join(tmpdir(), 'bestax-decl-cwd-'));
+ const sandbox = mkdtempSync(join(tmpdir(), 'bestax-decl-cwd-'));
+ // One level DOWN, so a `..` spelling lands inside the watched tree rather
+ // than beside it in the system tmpdir, where nothing would see it.
+ const sandboxCwd = join(sandbox, 'cwd');
+ mkdirSync(sandboxCwd);
@@
- process.chdir(sandbox);
+ process.chdir(sandboxCwd);
@@
assert.deepEqual(
- readdirSync(sandbox),
- [],
+ readdirSync(sandbox, { recursive: true }).sort(),
+ ['cwd'],
'the guard resolved a path against the working directory'
);mkdirSync and readdirSync are already imported; recursive on readdirSync is Node ≥20.1, and the repo is on 22/24.
Traced against mirrorAsCommonjs
bulma-ui/rollup.config.js:653 calls mirrorAsCommonjs(root, mirror), whose body is await cp(from, to, { recursive: true }) — to relative resolves against process.cwd() at call time.
mutated mirror default |
resolves to (cwd = sandbox) | inside sandbox? | which assertion fires |
|---|---|---|---|
'types-cjs' |
<sandbox>/types-cjs |
yes | this one ✅ |
'dist/types-cjs' |
<sandbox>/dist/types-cjs |
yes | this one ✅ |
'../types-cjs' |
<tmpdir>/types-cjs |
no | the sibling existsSync, line 1013 ❌ |
With the fix (cwd = <sandbox>/cwd), the third row becomes <sandbox>/types-cjs and fires here.
flowchart LR
A["mirror = '../types-cjs'"] --> B["cp() resolves vs cwd"]
B -->|"cwd = sandbox"| C["tmpdir/types-cjs<br/>OUTSIDE the watch"]
C --> D["readdirSync(sandbox) == []<br/>assertion passes"]
D --> E["caught only by the sibling check"]
B -->|"cwd = sandbox/cwd"| F["sandbox/types-cjs<br/>inside the watch"]
F --> G["assertion fires by name"]
There was a problem hiding this comment.
Deep review — 1 blocking · 3 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | Coverage | The sandbox is stood in, not above, so it nets only downward spellings. '../types-cjs' — named on line 959 as the motivating case — resolves to <tmpdir>/types-cjs, leaves readdirSync(sandbox) empty, and is still caught only by the sibling existsSync this assertion exists so as not to lean on |
scripts/constants-subpath.test.mjs:996 |
| 2 | 🔵 Advisory | Coverage | The two-literal outside loop is now unfalsifiable by the mutant class: both entries are absolutes captured before the chdir, and no cwd-relative mirror can reach either once the guard runs from the sandbox. Keeping them as defence-in-depth is fine; the comment on 945-951 ("the leak lands in <repo>/dist/types-cjs … both are watched so the case fails from either") describes the route the chdir closes. Deletion candidate: that paragraph's second and third sentences |
scripts/constants-subpath.test.mjs:945-951 |
| 3 | 🔵 Advisory | Robustness | The nested-manifest read now sits below esm/cjs but above assert.notEqual(esm, cjs). A require.types repointed to a different spec inside dist/types (reachable only on a dirty dist where that file exists) now throws ENOENT from readFileSync instead of the clean "both conditions resolve into one directory" message. The case fails either way — message quality only, and moving the read below the notEqual restores it |
scripts/constants-subpath.test.mjs:411 |
| 4 | 🔵 Advisory | Robustness | join(cjs, 'package.json') reads the manifest in the types target's own directory, where TypeScript walks up to the nearest one. Identical today and strictly better than the literal it replaces; it is a same-directory assumption rather than the resolution rule, so a mirror whose entry ever nests one level deeper reads as absent rather than as CommonJS |
scripts/constants-subpath.test.mjs:411 |
Overall: The change is sound in shape and the diagnosis behind it is exactly right — "every spelling that goes wrong here goes wrong the same way: it is cwd-relative" is the correct generalisation, and rejecting dirname(root) for system-tmpdir churn is a correct refutation of the alternative (including of my own round-5 advisory, which offered both forms without separating them). The one gap is that the sandbox is entered at its root, so the net catches 'types-cjs' and 'dist/types-cjs' but not the '../types-cjs' spelling the new comment itself names; standing one level down inside the same mkdtemp closes it without reintroducing the churn problem. Part 1 is behaviour-preserving today (exports['.'].require.types is ./dist/types-cjs/index.d.ts, so cjs resolves to the exact path it replaces) and worth taking for the reason stated. One caveat on this pass: node and pnpm execution was not available to me in this session, so the mutant table in finding 1 is traced from mirrorAsCommonjs's source (bulma-ui/rollup.config.js:679 — cp(from, to, {recursive:true}), to resolved against process.cwd()) rather than observed; the PR's own driven mutant ('dist/types-cjs') is consistent with that trace and I have no reason to doubt it.
Residual risk:
'../types-cjs'— open, posted as finding 1. The assertion passes; only the siblingexistsSyncfails the case, which is the dependency this case exists to remove.'../../types-cjs'and deeper — open and accepted, and it stays open under the suggested fix too. Diminishing: no plausible default is spelled two levels up, and the sibling check still reddens the case. One level is where the named member lives.process.chdirracing another case — refuted. Noconcurrencyoption appears anywhere in the file, and node:test's default subtest concurrency is 1, so cases in thisdescriberun sequentially;node --testisolates across files by process, so no otherscripts/*.test.mjsshares this cwd. The restore is correctly placed:run()returns the awaitedcloseBundle()and the wholeawait run(root)sits inside thetry, so cwd is restored only after the copy has landed — an unawaited hook would have restored it mid-flight and the sandbox would read empty for the wrong reason.rollup.config.jsevaluated under the sandbox cwd — refuted. With--test-name-patternfiltering to this case alone, its dynamicimport()is the module's first evaluation and it happens inside thechdir. Its module scope is bare-specifier imports (resolved from the module's own URL, not cwd) plus object literals;scssBase.includePathsandwatchare unevaluated strings and nothing at top level reads a cwd-relative path.- Another process reddening the new assertion — refuted, and this is the property that makes the sandbox viable where
dirname(root)was not: the watched directory is amkdtempthis case owns and nothing else writes into, rather than the shared system tmpdir everytree()and every other process mkdtemps into. - Part 1 changing what is asserted today — refuted.
bulma-ui/package.json:141gives"./dist/types-cjs/index.d.ts", sodirname(target(...))isPKG_DIR/dist/types-cjs, identical to the removed literal. The two shapes it newly catches (require.typesrepointed at another tree carrying its own manifest) remain unreachable on a builtdistfor the reason the PR body gives, and the PR is straight about that. - A leak into the real
dist/types-cjstrees going unnoticed — refuted for the mutant class by finding 2's own observation: they cannot be reached from the sandbox cwd at all, so the sandbox assertion is now the whole net for cwd-relative writes, which is why its coverage gap is the finding rather than a nicety.
🏄 Killer read on the break, dude — instead of naming two waves you went for the whole set, and that's the right call. Only thing is you're standing at the top of the sandbox instead of one step down in it, so the little
..backwash slips out the side and only the old leash notices. Drop one level, and this one's glassy all the way in. 🤙
… it too The sandbox was entered at its root, which nets only downward spellings. The comment beside it names `'../types-cjs'` as a member of the family, and that one resolves ABOVE the sandbox into the shared tmpdir — out of the net, and back to being caught only by the sibling check this assertion exists so as not to lean on. Naming the case and then not covering it is worse than not naming it. Standing in `sandbox/cwd` instead fixes it without giving back what the sandbox bought: `..` now lands beside the working directory, still inside a `mkdtemp` this case owns, so nothing else on the machine writes there. Two assertions instead of one, each naming its direction. Driven, all three spellings, each caught by the assertion that names it: mirror='dist/types-cjs' -> resolved a path against the working directory mirror='types-cjs' -> resolved a path against the working directory mirror='../types-cjs' -> resolved a path above the working directory Two levels up and beyond stays open and accepted: no plausible default is spelled that way, and the sibling check still reddens the case. Also moved the nested-manifest read below `notEqual(esm, cjs)`. It reads through `cjs`, so with the two collapsed onto one directory it threw ENOENT and lost the message saying what had actually gone wrong. The guard belongs above the thing it guards. And deleted the two sentences that described the route the chdir closes — they explained why both real trees had to be watched from either cwd, which stopped being the reason the moment the guard ran from a sandbox. They stay as defence in depth, which is what the comment says now.
|
deep-review: fresh — review the current head. Settled, do not re-report: everything from the prior round is either fixed in the commit since or explicitly accepted below. Please raise only findings about the NEW commit, and hold advisories to ones that are actionable in this diff. Accepted and closed, by agreement:
|
Preview DeploymentPreview URL: https://5623f49d.bestax.pages.dev |
|
deep-review: fresh — review the current head. Settled, do not re-report: the prior round's findings are fixed in the commit since, or accepted below. Raise only what is new in this diff, and only what is actionable in it. Accepted and closed by agreement:
|
There was a problem hiding this comment.
Deep review — 0 blocking · 2 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Robustness | The sandbox mkdtemp is never removed, so every run of this case leaves a <tmpdir>/bestax-decl-cwd-*/cwd pair behind. Consistent with tree()'s existing behaviour in this file — on the record, not a blocker |
scripts/constants-subpath.test.mjs:979-986 |
| 2 | 🔵 Advisory | Coverage | Execution was unavailable to me this session — node and pnpm were both refused — so the mutant table below is traced from source rather than observed. Noting it so the trace is not read as a run |
— |
Overall: The head commit does what round 6 asked and nothing more: standing in sandbox/cwd rather than at sandbox puts '../types-cjs' back inside a directory this case owns, and splitting the net into two assertions — one per direction — gives each spelling a message that names it. Part 1 is confirmed behaviour-preserving: bulma-ui/package.json:141 is "./dist/types-cjs/index.d.ts", so dirname(target(root.require.types)) is byte-for-byte the literal it replaces, and moving the read below assert.notEqual(esm, cjs) means a collapsed pair now reports the collapse instead of an ENOENT. Nothing here needs a human's attention before merge; if you want one thing to eyeball, it is that const outside is evaluated above the try — that placement is what makes "they are absolute, so the chdir does not move them" true, and a future edit that slides it down silently repoints the first entry into the sandbox.
Mutant table, traced through mirrorAsCommonjs (cwd = <sandbox>/cwd)
bulma-ui/rollup.config.js:653 calls mirrorAsCommonjs(root, mirror); its body is a recursive cp(from, to), so a relative to resolves against process.cwd() at call time. run() returns the awaited closeBundle() and the whole await run(root) sits inside the try, so the copy lands before the cwd is restored.
mutated mirror default |
resolves to | assertion that fires |
|---|---|---|
'dist/types-cjs' |
<sandbox>/cwd/dist/types-cjs |
readdirSync(sandboxCwd) — "resolved a path against the working directory" |
'types-cjs' |
<sandbox>/cwd/types-cjs |
same |
'../types-cjs' |
<sandbox>/types-cjs |
readdirSync(sandbox) — "resolved a path above the working directory" |
Matches the three spellings the commit message reports driving.
Residual risk:
'../../types-cjs'and deeper — open and accepted by agreement; the siblingexistsSyncstill reddens the case.- A mirror default that is absolute but elsewhere — module-relative, a
..offroot, a literal under$HOME— refuted as a silent pass: none is reached from the sandbox, but each leaves the-cjssibling ofrootabsent, so the assertion atscripts/constants-subpath.test.mjs:1035fails the case. Not the assertion this case prefers, and the same accepted trade as the row above. - A leak into the two real trees — refuted for the cwd-relative class, which cannot reach them from the sandbox, and still live for the package-relative class:
outside[1]isPKG_DIR/dist/types-cjs, which a default spelled off the module's own URL still hits, and the snapshot is identity plus size plus mtime rather than a count, so a repeat leak writing the same names is still visible. The pair is not vacuous. process.chdirracing another case in this file — refuted. Two top-leveldescribes, noconcurrencyoption anywhere in the file, and node:test's default subtest concurrency is 1;node --test "scripts/*.test.mjs"isolates across files by process, so no sibling script test shares this cwd. Every other case addresses the filesystem absolutely —PKG_DIR,mkdtemproots,tsc -p <abs>— so even under concurrency none of them reads the moved cwd.rollup.config.jsevaluated under the sandbox cwd — the path where--test-name-patternselects this case alone, making the dynamicimport()inside thetrythe module's first evaluation — refuted.bulma-ui/rollup.config.js:831exports an arrow function as its default, so the config is built on call rather than at module scope; the only top-level values are bare-specifier imports, resolved from the module's own URL and not from cwd, plus object literals whoseincludePathsandwatchare unevaluated strings.- A false failure from outside the process — refuted. Both watched directories are inside a
mkdtempthis case owns; the rejecteddirname(root)approach watched the shared system tmpdir, which is the whole reason it churned. - Symlink escape from the recursive
cp— refuted. The threesymlinkSynccalls in this file are all in the export-map consumer fixtures, lines 321, 491 and 568; the guard'stree()writes plain files only. - Part 1 opening a green-but-wrong state — refuted, and the PR says so itself.
join(cjs, 'package.json')resolves to the removed literal today, and the shapes it newly catches need a second byte-identical declaration tree that a builtdistdoes not contain.
🏄 Dropped in one step lower and now the whole set is yours, dude — three spellings, three call-outs, each one naming its own direction. The manifest read finally asks the map instead of its own memory, and the guard sits above the thing it guards. Clean line, no sections, paddle it in. 🤙
|
🎉 This PR is included in version 5.16.8 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.11 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.3.9 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Follow-up to #715, which merged while these were in flight. Round 5's two advisories, both about the guard suite rather than the shipped fix.
1 — the nested-manifest read was the last hardcoded path
The line the whole #698 fix rests on was checked at
PKG_DIR/dist/types-cjs/package.jsonwhileesm/cjsbeside it were derived fromexports['.']. It now readsjoin(cjs, 'package.json'), so it asks the tree the export map actually serves.No mutant drove this one, and I want to be straight about that: there's no reachable green-but-wrong state from it today. A manifest-only repoint needs a second byte-identical tree and none exists on a built
dist. It removes the last assumed path from the case, which is the whole reason to take it.2 — the outside-write watch asserted less than it read as
This is the one that mattered. Two named directories assert "nothing landed in these two places", not "nothing landed outside the tree". A mirror default spelled
'types-cjs'or'../types-cjs'lands where neither looks, and only the siblingexistsSyncwould notice — which is precisely the assertion the case exists so as not to lean on.Adding a third literal is the same mistake at greater length. Every spelling that goes wrong here goes wrong the same way: it is cwd-relative. So the guard now runs from an empty
mkdtempsandbox and the net is that the sandbox is still empty afterward. No literal has to be anticipated. The two real trees stay watched by name — they're absolute, a chdir doesn't move them, and they're the ones that have actually been hit.Driven, by name
Mutate the mirror default to the literal
'dist/types-cjs':That's the new assertion, firing ahead of the sibling check. Re-verified against
mainafter the rebase, not just against the pre-merge branch.The approach I tried first, which doesn't work
Snapshotting
dirname(root)looks like the general form and was the obvious move. It's unusable:tree()mkdtemps straight into the system tmpdir, so the parent is/var/folders/.../Tand churns from every other process on the machine. The first run failed with a 40KB diff of unrelated entries. Recording it so the next person doesn't spend the same hour.Scope
One test file. No shipped code changes —
bulma-ui/rollup.config.jsandbulma-ui/package.jsonare untouched.Gate: format, lint, typecheck, conformance clean. The
loads the plugin's own published entryfailure in a fresh worktree iseslint-plugin/dist is absent; run its build first, which says what it is.