Skip to content

fix(storage): keep portable path tests stable on Windows - #2395

Merged
jackwener merged 3 commits into
apache:mainfrom
liugddx:fix/windows-storage-portable-path-tests
Aug 7, 2026
Merged

fix(storage): keep portable path tests stable on Windows#2395
jackwener merged 3 commits into
apache:mainfrom
liugddx:fix/windows-storage-portable-path-tests

Conversation

@liugddx

Copy link
Copy Markdown
Member

Summary

  • preserve both source-native and host-normalized cwd variants in the Codex SQLite coarse filter
  • keep the authoritative two-sided cwd comparison after the SQL LIMIT
  • use a domain-valid POSIX sandbox path in the session-metadata test instead of the Windows host temp path

Part of #2142 Phase 1.

Why

On Windows, path.resolve('/target') becomes a drive-qualified native path before the Codex thread query runs. The SQL pre-filter therefore discarded rows storing Codex's POSIX cwd before the existing JavaScript normalization could compare them. Separately, the sandbox-boundary fixture passed tmpdir() into a contract that intentionally accepts normalized absolute POSIX paths.

Validation

  • npm --workspace @maka/storage run build
  • targeted Codex cwd tests: 3/3 pass on Windows
  • targeted sandbox-boundary settlement test: pass on Windows
  • npx biome check for changed files
  • git diff --check
  • combined local Windows storage baseline with fix(storage): close usage stores after lease revocation #2365: 678 pass / 14 fail / 45 skip, improved from 677 pass / 18 fail / 44 skip

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this — the fixture fix is solid (the tmpdir() path really did violate the POSIX-absolute contract on Windows), and the variant set is a strict superset of the old behavior, so there's no regression risk.

One thing I'd double-check though: coarseCwdVariants only flips the trailing separator, so a filter like C:\\Users\\me\\project (which is what process.cwd() actually looks like on Windows) produces {C:\\..., C:\\.../, C:\\...\\} — no C:/Users/me/project form in there. If Codex does store POSIX-shaped cwds as the PR body says, the pre-filter would still drop those rows before the authoritative normalizePath comparison gets to accept them. In other words the fix covers the POSIX-input shape the tests exercise, but not the native-input shape production actually sees (old and new code both miss that one, so it's not a regression — just an incomplete claim).

Minimal fix if you want it airtight: add the forward-slash-flipped form (candidate.replace(/\\\\/g, '/')) to the set — it's a pure function, so a direct unit test with path.win32 would pin it on every CI, not just the Windows lane (the current 3 cwd tests pass on POSIX under both old and new code, which is how this slipped through). Alternatively, if Codex actually stores native paths on Windows, it's worth softening the PR description to frame this as a test-stability fix rather than a production bug fix.

Non-blocking either way. Minor: the comment at foreign-session-store.ts:573-576 ("the exact normalized path or its trailing-separator variant") no longer describes the six-variant set.

@liugddx

Copy link
Copy Markdown
MemberAuthor

Addressed in b01947775 - good catch.

  • codexCwdSqlVariants() now adds both interior separator forms (C:\\... and C:/...) for the source-native and host-normalized candidates, plus their trailing-separator forms.
  • Added a pure cross-platform test using path.win32.join() that pins C:/Users/me/project and its trailing-slash variant on every CI platform.
  • Updated the stale coarse-filter comment to describe source-native and host-normalized separator forms.

Validation:

  • @maka/core build
  • @maka/storage build
  • Codex scan target: 10/10 pass on Windows
  • Biome and git diff --check

The complete foreign-session test file still has the pre-existing Windows symlink privilege failure when Developer Mode is unavailable; that is separate from this review change and remains in the #2142 capability-skip group.

@liugddx
liugddxforce-pushed the fix/windows-storage-portable-path-tests branch from b019477 to e120277CompareAugust 7, 2026 07:34
@liugddx

Copy link
Copy Markdown
MemberAuthor

The new failure is repository-wide formatting residue from #2406, not this PR's storage changes. The required typecheck job stops in npm run format:check on six packages/headless test files; all #2395 tests, workspace tests, and the aggregate test job pass.

I rebased #2395 onto current main (9a811325b) and confirmed the same six failures locally on a clean main. Opened #2410 as a mechanical, isolated fix rather than adding unrelated headless files here. Once #2410 lands, I will rebase this PR and retrigger the required checks.

The review follow-up itself remains green locally: Core/Storage build, four targeted cwd tests, and git diff --check pass.

@liugddx
liugddxforce-pushed the fix/windows-storage-portable-path-tests branch from e120277 to 144834aCompareAugust 7, 2026 08:04
@liugddx
liugddx requested a review from Astro-HanAugust 7, 2026 09:41

@jackwenerjackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial review — approve & merge

Scope

  • codexCwdSqlVariants(): coarse SQL pre-filter for Codex cwd across separator styles
  • Fixture: sandbox boundary path forced to POSIX /tmp/... (contract requires normalized absolute POSIX paths)

Attack surface checked

SQL injection — variants are bound params (IN (?,?,…)), never interpolated. Safe.

False negatives (still miss rows before JS filter)

  • Separator flip + trailing /\ covered; reviewer gap addressed with path.win32 unit test.
  • Case (C: vs c:), 8.3 short names, UNC-vs-drive still not covered — pre-existing on both SQL and normalizePath() string compare; not a regression.
  • Cross-host resolve() of foreign-style paths is noisy on POSIX but production filter is host-native.

False positives / LIMIT pollution

  • Variant set is same-path shape variants only; should not admit other projects into the LIMIT window.
  • Authoritative two-sided normalizePath() still runs in codexRowsToSummaries().

Empty IN () — function always yields ≥1 variant for any string input.

Sandbox fixtureisNormalizedAbsolutePath rejects \ and non-/ roots; tmpdir() on Windows violates the contract. Hardcoding /tmp/maka-output correctly exercises :slash_tmpalready_applied.

API surface — exporting codexCwdSqlVariants is a minor public export for testability; acceptable.

Validation

  • CI green (incl. windows_baseline non-blocking)
  • Local: 3 Codex cwd tests + sandbox settlement test pass

No blocking issues. LGTM.

@jackwener
jackwener merged commit d3a778e into apache:mainAug 7, 2026
11 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@liugddx@jackwener@Astro-Han