Repository navigation
test(shared): stop two suites asserting POSIX separators on Windows - #16511
Conversation
Both files describe paths with POSIX literals while their subjects compose paths through `node:path`, so the assertions only hold where the separator happens to be `/`. `node-markdown-document-discovery` keys its fake tree at `/repo/docs` and `/repo/one`, but `discoverMarkdownRelativePaths` descends with `join(absoluteDirectoryPath, entry.name)` — `\repo\docs` on win32. The child lookup misses, `readDirectory` yields nothing, and the walk stops at the root: `docs/guide.mdx` disappears and the depth-limit case never reaches its limit, so it resolves `[]` instead of rejecting. Keying the children with `join` walks the tree the subject actually walks. `git-fetch-head-lock` expects `cwd: '/tmp/repo'` from a subject that returns `path.resolve(cwd, 'repo')`, which is `C:\tmp\repo` on win32. Asserting through `path.resolve` pins the behaviour — that `-C` and `--git-dir` are resolved against the cwd — rather than the separator of whichever machine runs the suite. Verified on Windows 11: the two files go from 3 failed / 12 passed to 14 passed / 1 skipped, and the wider `src/shared` run shows no regression. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughUpdated tests to construct expected Git paths with Merge Risk: ⚪ Minimal · up to This test-only change makes two path assertions portable across operating systems without changing production behavior; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description follows the required template and provides a clear explanation, change summary, rationale, linked issue, visual-proof rationale, platform-specific testing, AI disclosure, review notes, and checklist status. It also explains why full lint and test runs were blocked by pre-existing Windows issues. Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. 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 |
…tablyai#16511) Both files describe paths with POSIX literals while their subjects compose paths through `node:path`, so the assertions only hold where the separator happens to be `/`. `node-markdown-document-discovery` keys its fake tree at `/repo/docs` and `/repo/one`, but `discoverMarkdownRelativePaths` descends with `join(absoluteDirectoryPath, entry.name)` — `\repo\docs` on win32. The child lookup misses, `readDirectory` yields nothing, and the walk stops at the root: `docs/guide.mdx` disappears and the depth-limit case never reaches its limit, so it resolves `[]` instead of rejecting. Keying the children with `join` walks the tree the subject actually walks. `git-fetch-head-lock` expects `cwd: '/tmp/repo'` from a subject that returns `path.resolve(cwd, 'repo')`, which is `C:\tmp\repo` on win32. Asserting through `path.resolve` pins the behaviour — that `-C` and `--git-dir` are resolved against the cwd — rather than the separator of whichever machine runs the suite. Verified on Windows 11: the two files go from 3 failed / 12 passed to 14 passed / 1 skipped, and the wider `src/shared` run shows no regression. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
ELI5
Two test files write file paths the Mac/Linux way, with forward slashes. The code they test builds paths the way each operating system wants them, so on Windows the two never line up. One test then compares mismatched strings and fails loudly; the other quietly explores an empty folder and concludes there is nothing there — which is worse, because that is also what a broken traversal looks like. This makes both tests describe paths the way the code does.
What Changed
node-markdown-document-discovery.test.tskeys its fake directory tree withjoininstead of'/repo/docs'and'/repo/one'literals.git-fetch-head-lock.test.tsassertspath.resolve(...)instead of the literal'/tmp/repo'.Test-only. No production file changes.
Why
Both subjects compose paths through
node:path; both suites described them with POSIX literals.Discovery walked nothing.
discoverMarkdownRelativePathsdescends withso on win32 it asks for
\repo\docswhile the fake tree only answers to/repo/docs.readDirectoryyields nothing, recursion ends at the root, and two cases fail for one reason:The second is the one worth pausing on. A depth limit that is never reached resolves
[], which is indistinguishable from a correct walk over an empty tree — on this platform the suite could not tell a broken traversal from a working one.The lock test compared a resolved path to a literal.
resolveGitFetchHeadCommandreturnspath.resolve(cwd, args[index + 1]):Asserting through
path.resolvepins what the function promises — that-Cand--git-dirresolve against the cwd — rather than the separator of whichever machine runs the suite.Neither is visible to CI:
joinandresolveemit/on Linux and macOS.Linked Issue
Fixes #16509
Visual Proof
N/A— test-only change, no user-facing surface.Testing
Windows 11 Pro (26200), Node 24.15.0, on
mainatcac538854:src/shared(551 files)pnpm typecheckexits 0 andoxlinton both files exits 0.The remaining
src/sharedfailures on Windows aresetup-agent-sequencinglosing a prompt with special characters through a realcmd.exespawn — a different mechanism, noted at the bottom of #16509 and deliberately not touched here.Full
pnpm lintandpnpm teststill cannot complete on a stock Windows checkout for reasons that predate this change: #14479, #15200, andnode-ptynow requiring a from-source rebuild sincebuild(windows): refuse unpatched node-pty prebuilds.No new test. The change is the test correction — three cases go from red to green on Windows and keep passing elsewhere, which is the regression signal.
AI Disclosure
Claude Opus 5 via Claude Code was used to find the failures, isolate the cause, and write the fix.
Review
join/resolvereturn the same values on macOS and Linux as the literals did, so those platforms are unaffected; Windows stops disagreeing.Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance — covered under Review.
#15994 is a much larger Windows-suite sweep and touches neither file, so these do not overlap.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred) —typecheckand the affected suites locally;lint/testblocked on Windows by the pre-existing issues named under Testing, so CI covers them