fix(tools): detect Windows drive-letter paths as absolute on POSIX hosts (#54039)#54066
fix(tools): detect Windows drive-letter paths as absolute on POSIX hosts (#54039)#54066eliot-onbox wants to merge 1 commit intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR adds a
Confidence Score: 5/5
Reviews (1): Last reviewed commit: "fix(tools): detect Windows drive-letter ..." | Re-trigger Greptile |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46f114b871
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (path.isAbsolute(expanded)) { | ||
| return expanded; | ||
| if (path.isAbsolute(expanded) || WIN_DRIVE_LETTER_RE.test(expanded)) { | ||
| return path.resolve(expanded); |
There was a problem hiding this comment.
On POSIX hosts, path.resolve("C:\\...") is not treated as a Windows absolute path; it gets prefixed with process.cwd(). Because this branch now classifies drive-letter inputs as absolute and then calls path.resolve(expanded), Windows-style paths are still rewritten to host-local paths (often still under the workspace when process.cwd() matches it), so the doubled-path behavior from #54039 can still occur in sandbox path resolution. This should preserve the original drive-letter string (or normalize with path.win32) instead of resolving with POSIX path.resolve.
Useful? React with 👍 / 👎.
| const candidate = mapped.startsWith("@") ? mapped.slice(1) : mapped; | ||
| return path.isAbsolute(candidate) | ||
| return path.isAbsolute(candidate) || WIN_DRIVE_LETTER_RE.test(candidate) | ||
| ? path.resolve(candidate) |
There was a problem hiding this comment.
This change has the same cross-platform issue in tool path guarding: on POSIX, path.resolve(candidate) for C:\\... or D:/... produces <process.cwd>/C:\\... rather than preserving the Windows absolute path. That means the new "absolute" handling still rewrites the input, and now depends on global process cwd instead of the provided workspace root, so the fix can misclassify or mismatch paths in memory-flush path checks.
Useful? React with 👍 / 👎.
…sts (openclaw#54039) On POSIX, `path.isAbsolute('C:\\...')` returns false, causing the read/write/edit tools to join a Windows absolute path with the workspace root — producing doubled paths like `C:\workspace\C:\workspace\file.md`. Add a WIN_DRIVE_LETTER_RE (/^[A-Za-z]:[/\\]/) fallback to the three path resolution entry points: - sandbox-paths.ts: resolveToCwd, resolveAllowedTmpMediaPath - pi-tools.read.ts: resolveToolPathAgainstWorkspaceRoot On native Windows this is redundant (path.isAbsolute already handles it), but it protects against cross-platform edge cases. Includes regression tests for both files.
eliot-onbox
force-pushed
the
fix/windows-read-path-54039
branch
from
46f114b to
4916a71
Compare
openclaw-barnacle
bot
added
docker
|
Addressed the review feedback from @chatgpt-codex-connector: Problem: On POSIX, Fix: When Tests now assert exact equality ( |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4916a712a9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (WIN_DRIVE_LETTER_RE.test(candidate)) { | ||
| // On POSIX, path.resolve would prepend cwd to a Windows drive-letter path. | ||
| // Return the candidate unchanged — it is already absolute on the remote OS. | ||
| return candidate; |
There was a problem hiding this comment.
The new early return for WIN_DRIVE_LETTER_RE skips path.resolve on Windows too, so resolveToolPathAgainstWorkspaceRoot can now return non-canonical equivalents like C:/... or C:\dir\..\file. In wrapToolMemoryFlushAppendOnlyWrite, that value is compared by exact string against allowedAbsolutePath (built with path.resolve), so memory-flush writes can be rejected even when the agent targets the same file. This regression only appears on Windows-style absolute inputs, but it breaks a valid write path in memory-flush runs.
Useful? React with 👍 / 👎.
1 participant
Problem
On POSIX hosts,
path.isAbsolute("C:\\...")returnsfalse, so when an agent passes a Windows absolute path to the read/write/edit tools, the workspace root gets prepended — producing doubled paths like:Reported in #54039.
Fix
Add a
WIN_DRIVE_LETTER_RE(/^[A-Za-z]:[/\\]/) fallback alongsidepath.isAbsolute()in three path resolution entry points:resolveToCwd,resolveAllowedTmpMediaPathresolveToolPathAgainstWorkspaceRootOn native Windows this is redundant, but it protects against cross-platform edge cases.
Tests
6 new regression tests across both files (backslash paths, forward-slash paths, relative path passthrough).