Codex review: needs maintainer review before merge. Reviewed August 1, 2026, 8:15 PM ET / August 2, 2026, 00:15 UTC.

ClawSweeper review

What this changes

The PR stops Windows comparison normalization from trimming surrounding whitespace, adds containment regression coverage, and enables existing trailing-whitespace deny-policy tests on Windows.

Merge readiness

⚠️ Ready for maintainer review - 4 items remain

This PR is a narrowly scoped, source-supported repair for a Windows comparison bug and includes credible real Windows before/after proof. It should remain open for a maintainer to explicitly confirm the security-boundary contract: padded Windows paths must compare as distinct lexical paths, even though some external callers may newly fail closed.

Priority: P1
Reviewed head: bb5b29bb99d127e450d7a33b0aaf25f3fbbc0f33
Owner decision: Required. See Decision needed.

Review scores

Verification

How this fits together

isPathInside is the package’s lexical containment predicate for Windows paths and feeds root confinement plus mutation-deny checks. Roots, targets, and deny-policy entries enter the comparison layer; its result permits or blocks filesystem operations.

flowchart LR
  A[Roots, targets, and policy entries] --> B[Windows path normalization]
  B --> C[Lexical containment comparison]
  C --> D[Root confinement]
  C --> E[Mutation deny policy]
  D --> F[Filesystem operation]
  E --> F

Decision needed

Why: The implementation is mechanically narrow and follows the repository’s fail-closed guidance, but it intentionally changes behavior for external callers that previously depended on an undocumented whitespace-trimming comparison.

Before merge

• Resolve merge risk (P1) - Merging changes an externally observable Windows compatibility behavior: a padded root or deny entry no longer matches its unpadded sibling and will fail closed instead.
• Resolve merge risk (P1) - Fork workflow approval has not yet produced hosted Windows CI results; the contributor supplied detailed local Windows proof, but an approved windows-latest run would independently enforce the new unskipped deny-policy cases.
• Complete next step (P1) - No mechanical repair remains: a maintainer must confirm the intended compatibility and fail-closed semantics for padded Windows paths before merge.

Answering the maintainer decision the review flagged, since it is the only thing left on this PR.

I intended option 1 — confirm fail-closed root semantics, and the patch is written for that reading: normalizeWindowsPathForComparison produces a comparison key for containment math, so the lexical spelling of the root has to survive into the key. A space is a legal path component character on Windows, so C:\root and C:\root are two different directories and merging them into one key is what produced the false true.

On the compatibility risk, to scope what actually changes:

• Callers whose root has no surrounding whitespace: unchanged.
• Callers who pass a padded root and padded candidates: unchanged — both sides keep the same spelling and still compare equal.
• Callers who pass a padded root against unpadded candidates: this is the case that flips, and it flips from "reported inside" to "reported outside". That direction is fail-closed, so the failure mode is a denied operation rather than an escape.

Option 2 (normalize root input at the configuration boundary) is not mutually exclusive with this and I think it is the better long-term shape — but it belongs where roots are constructed, not inside a containment predicate, and it does not fix the predicate for callers who build paths themselves. Happy to open that as a follow-up if you want it.

Also flagged in the PR body and worth repeating here: toLowerCase() is not injective, so this key can still collapse distinct NTFS names on non-ASCII input (Turkish İ being the obvious one). That is pre-existing, deliberately out of scope here, and a separate follow-up if you want comparison moved to an ASCII-only or locale-invariant fold.

No further changes planned from my side — the branch is at c0696e5 and I'll hold it there unless you'd like something reshaped.

Both remaining items are about the intended contract across boundary layers, so I measured it instead of arguing it. bb5b29b adds what the measurement produced; the PR body carries the full write-up.

The contract is already yours, and it is pinned by committed tests. test/deny-mutations.test.ts has preserves trailing whitespace in denied paths and … in denied prefixes. Both carry it.skipIf(skipOnWindows) — the skip exists because the trimming normalizer made Windows disagree with them.

On Windows the deny list is currently applied to the wrong directory. Driving those two scenarios through the public root() API on Windows 11 / Node 24.15.0, swapping only src/path.ts between runs, with both private and private present on disk:

denied prefixes entry: "private "

  main ab93382   write("private /file.txt") -> ALLOWED     <- the denied directory is writable
                 write("private/file.txt")  -> BLOCKED     <- an undenied sibling is blocked

  this branch    write("private /file.txt") -> BLOCKED
                 write("private/file.txt")  -> ALLOWED

That is stronger than the isPathInside example already in the body: it is not an approximation, it is an inversion, and it affects the mutation guard rather than only the containment predicate.

Given that, I dropped the Windows skip on those two cases. They are load-bearing there:

test/deny-mutations.test.ts on Windows
  this branch    Tests  10 passed | 1 skipped (11)
  main path.ts   Tests   2 failed | 8 passed | 1 skipped (11)

Full Windows suite: 447 passed | 185 skipped → 449 passed | 183 skipped. Nothing else moved. ci.yml already runs windows-latest on Node 22 and 24, so this is now enforced in CI rather than only on my machine. The third skipIf in that file (symlink ancestors) is unrelated and untouched.

On the compatibility item: every other in-package caller passes a realpath result (archive-staging, file-store*, install-path), where the on-disk spelling is authoritative and exactness is the fail-closed direction. deny-mutations was the only caller reading free-text configuration, and it is the one the change repairs. So the blast radius for "a caller that supplied a padded root and relied on the trim" is limited to callers outside this package; if you would rather keep that leniency for policy input specifically, the coercion belongs at the policy boundary (policyPathEntries, next to safeDirName's existing trim() in install-path.ts:7) rather than in the comparison primitive — happy to add that instead if you prefer it.

CHANGELOG updated with the deny-mutations effect under the existing Unreleased entry.

One correction to the note above, so the claim is not read as stronger than it is.

ci.yml does define windows-latest on Node 22 and 24, but no workflow run has executed on this PR: ci, coverage, benchmarks, and CodeQL are all sitting at action_required on both this head and the previous one, so they are waiting on a maintainer to approve workflows for the fork. Only ClawSweeper Dispatch and the Socket checks have run.

So the Windows numbers above are from my machine (Windows 11, Node 24.15.0), and the unskipped cases will only be enforced once you approve the run. If you would rather not spend a CI approval on it yet, the local before/after is reproducible with:

git checkout ab93382 -- src/path.ts && pnpm exec vitest run test/deny-mutations.test.ts   # 2 failed
git checkout HEAD -- src/path.ts     && pnpm exec vitest run test/deny-mutations.test.ts   # 10 passed

Maintainer-side reproduction on the exact head bb5b29bb99d127e450d7a33b0aaf25f3fbbc0f33 supports LAND.

Verified locally with Node 24.18.0 and pnpm 10.34.5:

• pnpm exec vitest run test/windows-path.test.ts test/deny-mutations.test.ts — 14 passed, 2 platform skips.
• pnpm check — lint and build passed; full suite 607 passed, 25 skipped. The final temporary consumer install was interrupted after npm registry requests stalled for five minutes, with no package assertion failure emitted.
• Built-library proof after pnpm build: normalizeWindowsPathForComparison("C:\\root ") returned "c:\\root ", while the unpadded path returned "c:\\root"; the comparison keys were distinct.
• git diff --check — clean.

Source review agrees with the patch shape: path comparison must preserve legal path characters, and any input convenience trimming belongs at a specific configuration boundary rather than inside the shared confinement predicate. This also makes the existing trailing-whitespace deny-policy contract enforceable on Windows in the fail-closed direction.

Hosted Windows CI is still the remaining independent gate; the contributor's real Windows before/after proof covers the platform behavior meanwhile.