fix(helpers): accept Windows absolute paths in normalizeDirectory#6
Open
samuelgudi wants to merge 1 commit intoAlaeddineMessadi:mainfrom
Open
fix(helpers): accept Windows absolute paths in normalizeDirectory#6samuelgudi wants to merge 1 commit intoAlaeddineMessadi:mainfrom
samuelgudi wants to merge 1 commit intoAlaeddineMessadi:mainfrom
Conversation
normalizeDirectory previously required the resolved path to start with
"/", which is only correct on POSIX platforms. On Windows, path.resolve
produces drive-letter paths like "C:\Users\me\project" which failed
the startsWith("/") check and were incorrectly rejected as "not an
absolute path", making every tool that accepts the `directory` parameter
unusable for Windows clients.
Switch the check to path.isAbsolute from node:path, which is
platform-aware and recognizes both POSIX ("/home/user/project") and
Windows ("C:\Users\me\project", "\\server\share") absolute paths.
The check is performed on the result of path.resolve, so relative input
paths are still normalized against process.cwd() as before — behavior on
POSIX platforms is unchanged.
Also update the error message for invalid directories to include both a
POSIX and a Windows example, and add two regression tests: one that
accepts process.cwd() on any platform, and one (skipped on non-Windows
runners) that confirms a Windows drive-letter path is accepted.
On a Windows test run the upstream suite went from 20 failing tests to
13 — the 13 remaining failures are unrelated Linux-only assumptions in
other tests (hardcoded /tmp, /home paths) and are out of scope here.
📝 WalkthroughWalkthroughThe changes update Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/helpers.test.ts (2)
931-936: Strengthen the regression assertion.
toBeDefined()can pass even with a wrong normalization result. Assert the exact normalized value to make this test more diagnostic.✅ Stronger assertion
+import { resolve } from "node:path"; ... it("accepts the process working directory on any platform", () => { // Platform-agnostic regression: process.cwd() is always absolute for the // current platform, so normalizeDirectory must accept it regardless of OS. - const result = normalizeDirectory(process.cwd()); - expect(result).toBeDefined(); + const cwd = process.cwd(); + const result = normalizeDirectory(cwd); + expect(result).toBe(resolve(cwd)); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers.test.ts` around lines 931 - 936, The test currently only checks normalizeDirectory(process.cwd()) is defined; replace the weak toBeDefined() assertion with a deterministic equality check using Node's path utilities: compute the expected value with path.resolve(process.cwd()) (or path.normalize/process-specific normalization consistent with how normalizeDirectory implements it) and assert expect(result).toBe(expected). Also ensure the test file imports Node's path (e.g., const path = require("path") or import path from "path") so you can calculate expected via path.resolve(process.cwd()) to make the regression assertion exact.
943-951: Make the Windows test less environment-sensitive.This currently assumes
process.cwd()is always drive-letter formatted. Some Windows environments can use UNC/prefixed cwd paths, which makes this flaky.🧪 More robust drive-letter test setup
it.runIf(process.platform === "win32")( "accepts Windows drive-letter absolute paths", () => { - const cwd = process.cwd(); // e.g. "C:\\Users\\runner\\work\\..." - expect(cwd).toMatch(/^[A-Za-z]:\\/); - const result = normalizeDirectory(cwd); + const cwd = process.cwd(); + const drivePath = /^[A-Za-z]:\\/.test(cwd) + ? cwd + : `${process.env.SystemDrive ?? "C:"}\\`; + const result = normalizeDirectory(drivePath); expect(result).toBeDefined(); expect(result).toMatch(/^[A-Za-z]:\\/); }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers.test.ts` around lines 943 - 951, The test "accepts Windows drive-letter absolute paths" is flaky because it uses process.cwd() which may be a UNC or prefixed path; instead make the test environment-independent by supplying a deterministic drive-letter path or by mocking process.cwd() to return one. Update the test around normalizeDirectory to either set const cwd = "C:\\temp\\some\\path" (or build one from a single-letter drive) or spy on process.cwd() to return that drive-letter path, then call normalizeDirectory(cwd) and assert the drive-letter regex; ensure the test name and use of normalizeDirectory remain unchanged.src/helpers.ts (1)
99-112:isAbsolute(normalized)is effectively unreachable afterresolve().Because
resolve()already returns an absolute path, this branch is mostly dead code and the “not an absolute path” error path won’t realistically execute. Consider either validating the raw input before resolving (if absolute-only is intended) or dropping this guard to avoid misleading logic.♻️ Optional refactor (if absolute-only input is intended)
- const normalized = resolve(directory); - - // Defensive check: `resolve` guarantees an absolute path on every - // supported platform, but we verify via the platform-aware `isAbsolute` - // so callers get a clear error if that assumption is ever violated. - if (!isAbsolute(normalized)) { + if (!isAbsolute(directory)) { throw new Error( `Invalid directory: "${directory}" is not an absolute path. ` + `Provide a full path like "/home/user/my-project" (POSIX) or ` + `"C:\\\\Users\\\\me\\\\my-project" (Windows).`, ); } + const normalized = resolve(directory);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/helpers.ts` around lines 99 - 112, The isAbsolute check after calling resolve() is redundant because resolve(directory) always returns an absolute path; update the code by either moving the absolute-path validation to before calling resolve (validate directory with isAbsolute(directory) and throw there if you require callers to supply an absolute path) or remove the post-resolve guard entirely so you don't have unreachable logic; look for the symbols resolve, isAbsolute, normalized, and directory to make the change and keep error messaging consistent with whichever approach you pick.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/helpers.ts`:
- Around line 99-112: The isAbsolute check after calling resolve() is redundant
because resolve(directory) always returns an absolute path; update the code by
either moving the absolute-path validation to before calling resolve (validate
directory with isAbsolute(directory) and throw there if you require callers to
supply an absolute path) or remove the post-resolve guard entirely so you don't
have unreachable logic; look for the symbols resolve, isAbsolute, normalized,
and directory to make the change and keep error messaging consistent with
whichever approach you pick.
In `@tests/helpers.test.ts`:
- Around line 931-936: The test currently only checks
normalizeDirectory(process.cwd()) is defined; replace the weak toBeDefined()
assertion with a deterministic equality check using Node's path utilities:
compute the expected value with path.resolve(process.cwd()) (or
path.normalize/process-specific normalization consistent with how
normalizeDirectory implements it) and assert expect(result).toBe(expected). Also
ensure the test file imports Node's path (e.g., const path = require("path") or
import path from "path") so you can calculate expected via
path.resolve(process.cwd()) to make the regression assertion exact.
- Around line 943-951: The test "accepts Windows drive-letter absolute paths" is
flaky because it uses process.cwd() which may be a UNC or prefixed path; instead
make the test environment-independent by supplying a deterministic drive-letter
path or by mocking process.cwd() to return one. Update the test around
normalizeDirectory to either set const cwd = "C:\\temp\\some\\path" (or build
one from a single-letter drive) or spy on process.cwd() to return that
drive-letter path, then call normalizeDirectory(cwd) and assert the drive-letter
regex; ensure the test name and use of normalizeDirectory remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ba3631b-3176-4ae9-8e43-5fa309af2837
📒 Files selected for processing (2)
src/helpers.tstests/helpers.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #5.
Summary
normalizeDirectorychecked the resolved path withnormalized.startsWith(\"/\"), which is only correct on POSIX. On Windows,path.resolvereturns drive-letter paths likeC:\Users\me\project, so every Windows absolute path was rejected and thedirectoryparameter was effectively unusable for Windows MCP clients (all 79 tools affected).This PR switches the check to the platform-aware
path.isAbsolutefromnode:path.path.resolvealready guarantees an absolute path on every supported platform, so the new check is equivalent on POSIX (behavior unchanged) and correct on Windows.Changes
src/helpers.tsisAbsolutefromnode:path.normalized.startsWith(\"/\")with!isAbsolute(normalized).tests/helpers.test.tsaccepts the process working directory on any platform— a platform-agnostic regression that passesprocess.cwd()throughnormalizeDirectory. With the old code this test fails on Windows (cwd isC:\...) and passes on Linux; with the new code it passes on both.it.runIf):accepts Windows drive-letter absolute paths— asserts that a real Windows drive-letter path is accepted and round-trips through the function.No runtime dependencies added. No public API change. POSIX semantics unchanged.
Test results
Run on Windows 11 with Node 22,
npm test:upstream/main): 20 failing / 300 passing / 320 totalThe fix itself turns 7 previously-failing Windows tests green (the ones exercising
normalizeDirectoryvia the tool handlers with Linux-style paths that resolve to the current drive but then hit the absolute-path check incorrectly). The remaining 13 failures on Windows are pre-existing and unrelated to this fix — they hardcode Linux paths like/tmpinto string comparisons (e.g.expect(result).toBe(\"/tmp\")), which can't succeed on Windows whereresolve(\"/tmp\")returnsX:\tmp. Happy to file a follow-up PR making those tests platform-agnostic if you want, but I kept this PR scoped to the actual bug.On Linux, the full suite is unchanged by these changes — the new platform-agnostic test passes (cwd is absolute on Linux too), and the Windows-only test is skipped via
it.runIf(process.platform === \"win32\").Workaround that this replaces
Before this fix, the only way to make opencode-mcp usable on Windows was to omit the
directoryparameter entirely and rely on the cwd of the process that spawnedopencode serve. That works for single-project setups but defeats the point of the per-requestdirectoryheader in multi-project setups.Conventional Commits
fix(helpers): accept Windows absolute paths in normalizeDirectorySummary by CodeRabbit