mirror of
https://github.com/civitai/civitai.git
synced 2026-09-20 22:08:18 +08:00
fix(test-cache): normalise the leading slash POSIX fileURLToPath adds to a Windows file URL (#4981)
* fix(test-cache): normalise the leading slash POSIX fileURLToPath adds to a Windows file URL
`scripts/__tests__/test-cache-core.test.ts` has been red on every pull request
opened since 07:13Z today, failing one assertion of 32:
× gives the same path for the same file in two worktrees, URL or path
AssertionError: expected null to be 'src/a.ts'
Root cause, established by executing the function rather than reading it:
`fileURLToPath` is platform-dependent. Given `file:///C:/Dev/wt/two/src/a.ts`
it returns `C:\Dev\wt\two\src\a.ts` on Windows but `/C:/Dev/wt/two/src/a.ts`
on POSIX — with a leading slash. Neither drive-letter comparison in `toRel`
can see past that slash, so the path stops matching `root` and `isAbsolute()`
returns null instead of the relative path. The test asserts the Windows
result; CI runs Linux.
The fix normalises the leading slash away immediately after the conversion, so
the drive-letter forms below it read the same shape whichever platform
resolved the URL. It is one line in the shared prefix of every path reaching
this function, which is why the diff carries more comment than code.
Verified:
- red at the pre-fix commit, green at HEAD. With the fix reverted and the test
file unchanged: 2 failed | 32 passed. With the fix: 34 passed.
- Six control cases executed before and after — both POSIX spellings, a POSIX
path outside the root, an already-relative path, the Windows non-URL path,
and a Windows path outside the root. All unchanged by the fix. Only the
failing case moves.
Two tests added, and they are labelled from what they were MEASURED to do at
the pre-fix commit rather than from what they were written to do. The first
draft called both "invariant"; the run showed the second one fails at base, so
it is regression coverage and is now named that. The POSIX one does pass at
base and stays labelled an invariant guard, so nobody counts it as regression
coverage it does not provide.
Scope note: this is a fix for a defect on main, deliberately kept out of the
unrelated PR that surfaced it. #4971, which introduced the test, merged at
07:13:43Z with this same shard already failing on its own head commit.
One judgement worth flagging for review: a POSIX path whose first segment is
literally a single letter and a colon (`/C:/…`) would now be rewritten. That
spelling is pathological on POSIX and cannot be produced by `fileURLToPath`
from a non-Windows URL, but it is the one input whose handling this changes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(test-cache): scope the normalisation to the file:// branch, and drop a subsumed test
Round 0 of the pre-merge audit found two things, both acted on here. Neither
is a correctness defect in the shipped fix — CI was fully green at 20282b3e77,
19 entries, 18 success, 1 skipped, shard 2 passing.
1. NARROWED (R1). The normalisation ran on every input, so it also rewrote a
genuinely POSIX path whose first segment is a letter and a colon. Measured:
toRel('/C:/notes/x.md', '/C:') returned 'notes/x.md' before and null after.
That was the one behaviour change the PR body had to flag for review.
Only a file:// id can carry the platform artefact, so only a file:// id
needs the repair. Moving it inside that branch satisfies the requirement
exactly and leaves every non-URL input byte-for-byte as it was. The flagged
behaviour change is gone rather than documented.
Measured across all three variants on 8 cases: base fails only the Windows
file:// URL case; the unscoped draft fixes that but breaks the POSIX
pseudo-drive case; the narrowed version is correct on all 8.
2. DELETED a test I added (the "regression" equality at :93-103). The audit
ran a mutation table I had not. Against four mutants — normalisation
deleted, inverted, prefix-compare broken, and "strip any leading slash" —
that test killed only the first, which the pre-existing assertion at :71-72
already kills, and it PASSED both the inverted and broken-prefix mutants.
An equality with no anchor is satisfied when both sides return null, which
is the property I had described as its strength. It is subsumed, and
strictly weaker than the assertion that surfaced the bug.
A comment now records why it was removed, so it is not re-added as an
apparent improvement.
The invariant guard survives and is unchanged: it is the only thing in the
file that kills the "strip any leading slash" mutant, and every other toRel
assertion here is Windows-shaped while CI and every Linux/macOS dev run POSIX.
A new invariant guard pins the POSIX pseudo-drive case the narrowing protects.
Re-verified after the change, because an audit fix resets the verification
gate: 34 passed at HEAD; at the pre-fix commit 1 failed | 33 passed, the single
failure being the genuine regression guard at :71-72. That is a cleaner control
than the previous revision, where two failed because the subsumed test failed
alongside it.
Round 0 reports and does not move the ladder; its advisory verdict was safe to
merge, and the requirement survived questioning — the Windows fixture is this
repo's only Windows coverage for toRel, since no workflow runs vitest on a
Windows runner, so platform-gating or deleting it would zero that coverage.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -72,6 +72,39 @@ describe('keys are portable between worktrees', () => {
|
||||
expect(toRel('file:///C:/Dev/wt/two/src/a.ts', 'C:/Dev/wt/two/')).toBe('src/a.ts');
|
||||
});
|
||||
|
||||
// The assertion above was red on POSIX from the moment it landed, because `fileURLToPath`
|
||||
// resolves a Windows file URL to `/C:/...` here and to `C:\...` on Windows. It is the
|
||||
// regression guard for that defect, and a mutation table showed it is also the STRONGEST
|
||||
// one available: of four mutants — normalisation deleted, inverted, prefix-compare broken,
|
||||
// and "strip any leading slash" — it kills the first three.
|
||||
//
|
||||
// 🔴 An earlier draft of this fix added a second regression test asserting that the URL and
|
||||
// path spellings return the SAME key. It was removed rather than kept: it is subsumed. It
|
||||
// kills the deleted-normalisation mutant, which the assertion above already kills, and it
|
||||
// PASSES both the inverted and broken-prefix mutants, because an equality with no anchor is
|
||||
// satisfied when both sides return null. Do not re-add it — an unanchored equality reads as
|
||||
// robustness and is the weaker check.
|
||||
//
|
||||
// What follows is an INVARIANT guard, labelled from what it was MEASURED to do at the
|
||||
// pre-fix commit rather than from what it was written to do: it passes before the fix and
|
||||
// after, so it is NOT regression coverage and must not be counted as any. It earns its
|
||||
// place on a different axis — every other `toRel` assertion in this file is Windows-shaped
|
||||
// while CI and every Linux/macOS dev run POSIX, and it is the only thing that kills the
|
||||
// "strip any leading slash" mutant.
|
||||
it('invariant: POSIX ids are unaffected by the drive-letter normalisation', () => {
|
||||
expect(toRel('/home/u/repo/src/b.ts', '/home/u/repo')).toBe('src/b.ts');
|
||||
expect(toRel('file:///home/u/repo/src/b.ts', '/home/u/repo')).toBe('src/b.ts');
|
||||
expect(toRel('/var/tmp/x.json', '/home/u/repo')).toBeNull();
|
||||
expect(toRel('src/c.ts', '/home/u/repo')).toBe('src/c.ts');
|
||||
});
|
||||
|
||||
// The narrowing above (normalising inside the `file://` branch) is what keeps this true:
|
||||
// a POSIX path whose first segment is a letter and a colon is NOT a platform artefact and
|
||||
// must not be repaired. Measured against the unscoped draft, which returned null here.
|
||||
it('invariant: a POSIX path that merely looks like a drive letter is left alone', () => {
|
||||
expect(toRel('/C:/notes/x.md', '/C:')).toBe('notes/x.md');
|
||||
});
|
||||
|
||||
// A read outside the repo (a temp file the test wrote itself) is not an input anyone else shares.
|
||||
it('drops absolute paths outside the repo', () => {
|
||||
expect(toRel('D:/elsewhere/x.json', 'C:/Dev/wt/one')).toBeNull();
|
||||
|
||||
@@ -82,7 +82,22 @@ export const sha = (data) => createHash('sha256').update(data).digest('hex');
|
||||
|
||||
export function toRel(id, root) {
|
||||
let p = String(id);
|
||||
if (p.startsWith('file://')) p = fileURLToPath(p);
|
||||
if (p.startsWith('file://')) {
|
||||
// 🔴 `fileURLToPath` is PLATFORM-DEPENDENT, and that is what made this function's own
|
||||
// test red on every PR: on Windows `file:///C:/x` yields `C:\x`, but on POSIX it yields
|
||||
// `/C:/x` — a leading slash the drive-letter comparisons below cannot see past, so the
|
||||
// path stopped matching `root` and `isAbsolute` returned null instead of the relative
|
||||
// path. Normalising restores the drive-letter shape those comparisons expect —
|
||||
// the `r.toLowerCase()` prefix test and the `/^[A-Za-z]:\//` guard — whichever
|
||||
// platform resolved the URL.
|
||||
//
|
||||
// 🔴 Scoped to THIS branch deliberately. Applied to every id it would also rewrite a
|
||||
// genuinely POSIX path whose first segment is a letter and a colon: measured,
|
||||
// `toRel('/C:/notes/x.md', '/C:')` returned 'notes/x.md' before and null after. Only a
|
||||
// `file://` id can carry the platform artefact, so only a `file://` id needs the repair,
|
||||
// and confining it here leaves every non-URL input byte-for-byte as it was.
|
||||
p = fileURLToPath(p).replace(/^\/([A-Za-z]:)/, '$1');
|
||||
}
|
||||
p = p.split('?')[0].replace(/\\/g, '/');
|
||||
const r = root.replace(/\\/g, '/').replace(/\/$/, '');
|
||||
if (p.toLowerCase().startsWith(r.toLowerCase() + '/')) return p.slice(r.length + 1);
|
||||
|
||||
Reference in New Issue
Block a user