fix: handle Windows-style session paths when running on POSIX#50116
fix: handle Windows-style session paths when running on POSIX#50116RIPRODUCTIONS wants to merge 1 commit intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR adds a cross-platform path guard to Issues found:
Confidence Score: 3/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/config/sessions/paths.ts
Line: 28
Comment:
**Unreachable `?? candidate` fallback**
`Array.prototype.split` always returns an array with at least one element (even `"".split(/[/\\]/)` yields `[""]`), so `parts[parts.length - 1]` can never be `undefined`. The `?? candidate` branch is dead code.
```suggestion
return parts[parts.length - 1] || candidate;
```
Switching to `||` also covers the empty-string case — if the path ends with a separator (e.g. `C:\foo\`), `parts.at(-1)` is `""`, and `|| candidate` falls back to the original string, giving the caller a non-empty value to check against.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/config/sessions/paths.ts
Line: 211
Comment:
**Redundant double `path.resolve`**
`path.resolve(a, b)` already resolves `a` internally before joining, so the inner `path.resolve(sessionsDir)` is a no-op and just adds noise.
```suggestion
return path.resolve(sessionsDir, basename);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/config/sessions/paths.ts
Line: 208-213
Comment:
**Silent fallthrough on degenerate Windows paths**
If `isWindowsAbsoluteOnPosix` returns `true` but `basename` ends up empty (e.g. `C:\` or `C:\foo\`, where `split` yields a trailing `""`), the guard block is skipped and execution falls through to the normal POSIX logic. On POSIX `path.isAbsolute("C:\\foo\\")` returns `false`, so the Windows string is treated as a *relative* path and `path.resolve(realBase, "C:\\foo\\")` produces the broken concatenated path the PR is trying to prevent.
While a sessions entry stored without a filename is highly unlikely, the silent fallthrough means the guard's invariant can be violated. Consider throwing explicitly in this case:
```typescript
if (isWindowsAbsoluteOnPosix(trimmed)) {
const basename = extractBasename(trimmed);
if (basename && basename !== "." && basename !== "..") {
return path.resolve(sessionsDir, basename);
}
// Degenerate Windows path with no usable filename — cannot resolve safely.
throw new Error("Session file path must be within sessions directory");
}
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "fix: handle Windows-..." Re-review progress:
|
| function extractBasename(candidate: string): string { | ||
| // Split on both separators to handle Windows paths on POSIX. | ||
| const parts = candidate.split(/[/\\]/); | ||
| return parts[parts.length - 1] ?? candidate; |
There was a problem hiding this comment.
?? candidate fallback
Array.prototype.split always returns an array with at least one element (even "".split(/[/\\]/) yields [""]), so parts[parts.length - 1] can never be undefined. The ?? candidate branch is dead code.
| return parts[parts.length - 1] ?? candidate; | |
| return parts[parts.length - 1] || candidate; |
Switching to || also covers the empty-string case — if the path ends with a separator (e.g. C:\foo\), parts.at(-1) is "", and || candidate falls back to the original string, giving the caller a non-empty value to check against.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/sessions/paths.ts
Line: 28
Comment:
**Unreachable `?? candidate` fallback**
`Array.prototype.split` always returns an array with at least one element (even `"".split(/[/\\]/)` yields `[""]`), so `parts[parts.length - 1]` can never be `undefined`. The `?? candidate` branch is dead code.
```suggestion
return parts[parts.length - 1] || candidate;
```
Switching to `||` also covers the empty-string case — if the path ends with a separator (e.g. `C:\foo\`), `parts.at(-1)` is `""`, and `|| candidate` falls back to the original string, giving the caller a non-empty value to check against.
How can I resolve this? If you propose a fix, please make it concise.| if (isWindowsAbsoluteOnPosix(trimmed)) { | ||
| const basename = extractBasename(trimmed); | ||
| if (basename && basename !== "." && basename !== "..") { | ||
| return path.resolve(path.resolve(sessionsDir), basename); |
There was a problem hiding this comment.
path.resolve
path.resolve(a, b) already resolves a internally before joining, so the inner path.resolve(sessionsDir) is a no-op and just adds noise.
| return path.resolve(path.resolve(sessionsDir), basename); | |
| return path.resolve(sessionsDir, basename); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/sessions/paths.ts
Line: 211
Comment:
**Redundant double `path.resolve`**
`path.resolve(a, b)` already resolves `a` internally before joining, so the inner `path.resolve(sessionsDir)` is a no-op and just adds noise.
```suggestion
return path.resolve(sessionsDir, basename);
```
How can I resolve this? If you propose a fix, please make it concise.| if (isWindowsAbsoluteOnPosix(trimmed)) { | ||
| const basename = extractBasename(trimmed); | ||
| if (basename && basename !== "." && basename !== "..") { | ||
| return path.resolve(path.resolve(sessionsDir), basename); | ||
| } | ||
| } |
There was a problem hiding this comment.
If isWindowsAbsoluteOnPosix returns true but basename ends up empty (e.g. C:\ or C:\foo\, where split yields a trailing ""), the guard block is skipped and execution falls through to the normal POSIX logic. On POSIX path.isAbsolute("C:\\foo\\") returns false, so the Windows string is treated as a relative path and path.resolve(realBase, "C:\\foo\\") produces the broken concatenated path the PR is trying to prevent.
While a sessions entry stored without a filename is highly unlikely, the silent fallthrough means the guard's invariant can be violated. Consider throwing explicitly in this case:
if (isWindowsAbsoluteOnPosix(trimmed)) {
const basename = extractBasename(trimmed);
if (basename && basename !== "." && basename !== "..") {
return path.resolve(sessionsDir, basename);
}
// Degenerate Windows path with no usable filename — cannot resolve safely.
throw new Error("Session file path must be within sessions directory");
}Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/sessions/paths.ts
Line: 208-213
Comment:
**Silent fallthrough on degenerate Windows paths**
If `isWindowsAbsoluteOnPosix` returns `true` but `basename` ends up empty (e.g. `C:\` or `C:\foo\`, where `split` yields a trailing `""`), the guard block is skipped and execution falls through to the normal POSIX logic. On POSIX `path.isAbsolute("C:\\foo\\")` returns `false`, so the Windows string is treated as a *relative* path and `path.resolve(realBase, "C:\\foo\\")` produces the broken concatenated path the PR is trying to prevent.
While a sessions entry stored without a filename is highly unlikely, the silent fallthrough means the guard's invariant can be violated. Consider throwing explicitly in this case:
```typescript
if (isWindowsAbsoluteOnPosix(trimmed)) {
const basename = extractBasename(trimmed);
if (basename && basename !== "." && basename !== "..") {
return path.resolve(sessionsDir, basename);
}
// Degenerate Windows path with no usable filename — cannot resolve safely.
throw new Error("Session file path must be within sessions directory");
}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Pull request overview
This PR improves cross-platform compatibility for session transcript paths by correctly handling Windows-style absolute paths when session metadata is read on POSIX (e.g., Linux containers).
Changes:
- Adds POSIX-only detection for Windows drive-letter absolute paths.
- Extracts the leaf filename from Windows-style paths and resolves it within the sessions directory to prevent malformed concatenations.
| // Split on both separators to handle Windows paths on POSIX. | ||
| const parts = candidate.split(/[/\\]/); | ||
| return parts[parts.length - 1] ?? candidate; | ||
| } | ||
|
|
| if (isWindowsAbsoluteOnPosix(trimmed)) { | ||
| const basename = extractBasename(trimmed); | ||
| if (basename && basename !== "." && basename !== "..") { | ||
| return path.resolve(path.resolve(sessionsDir), basename); | ||
| } | ||
| } |
| // Guard: if running on POSIX but the candidate is a Windows-style absolute | ||
| // path (e.g. stored in sessions.json by a Windows host and read inside a | ||
| // Docker container), extract the leaf filename to avoid creating paths like | ||
| // "/home/node/.openclaw/.../C:\Users\...". | ||
| if (isWindowsAbsoluteOnPosix(trimmed)) { | ||
| const basename = extractBasename(trimmed); | ||
| if (basename && basename !== "." && basename !== "..") { | ||
| return path.resolve(path.resolve(sessionsDir), basename); | ||
| } | ||
| } |
| // Already on Windows — path.isAbsolute handles it natively. | ||
| return false; | ||
| } | ||
| return /^[A-Za-z]:[/\\]/.test(candidate); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6075f5b08
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isWindowsAbsoluteOnPosix(trimmed)) { | ||
| const basename = extractBasename(trimmed); | ||
| if (basename && basename !== "." && basename !== "..") { | ||
| return path.resolve(path.resolve(sessionsDir), basename); |
There was a problem hiding this comment.
Preserve agent isolation when rewriting Windows session paths
When a POSIX host reads a Windows-authored sessionFile whose basename already exists locally (for example C:\...\agents\bot2\sessions\sess-1.jsonl while resolving a bot1 session), this branch now rewrites it straight to <sessionsDir>/sess-1.jsonl. That is a new silent aliasing bug: before, the bad Windows path stayed isolated under a distinct broken name, but after this change it can append to the wrong live transcript and resolveAndPersistSessionFile() will persist that alias back into sessions.json. The Windows-path fix needs to preserve agent/root context instead of collapsing everything to the basename.
Useful? React with 👍 / 👎.
|
Codex review: needs real behavior proof before merge. Summary Reproducibility: yes. for the bug class: a POSIX Node path check shows Windows drive-letter paths are treated as non-absolute, and current main uses that same path.isAbsolute branch before resolving persisted sessionFile values. I did not run the full TypeScript helper to avoid write-producing test/tool caches in this read-only review. Real behavior proof Next step before merge Security Review findings
Review detailsBest possible solution: Revise the resolver to recognize Windows absolute paths on POSIX while preserving agent/root context through the existing fallback flow, reject degenerate path shapes, and add focused regression coverage plus redacted real import proof. Do we have a high-confidence way to reproduce the issue? Yes, for the bug class: a POSIX Node path check shows Windows drive-letter paths are treated as non-absolute, and current main uses that same path.isAbsolute branch before resolving persisted sessionFile values. I did not run the full TypeScript helper to avoid write-producing test/tool caches in this read-only review. Is this the best way to solve the issue? No. The PR fixes the simplest same-agent filename case, but the basename-only early return bypasses existing realpath normalization and cross-agent fallback behavior; the safer solution is to normalize Windows absolute paths into the existing resolver path instead of returning early. Full review comments:
Overall correctness: patch is incorrect Security concerns:
Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against d8d441cd496e. |
When sessions.json is written by a Windows host but read inside a Linux Docker container, paths like "C:\Users\...\abc.jsonl" are treated as relative by path.isAbsolute (which only recognises POSIX absolutes). This causes resolvePathWithinSessionsDir to concatenate the Windows path onto the container base, producing broken paths like "/home/node/.openclaw/.../C:\Users\...\abc.jsonl". Add isWindowsAbsoluteOnPosix() guard that detects drive-letter paths on POSIX and extracts the leaf filename, resolving it within the sessions directory instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
c6075f5 to
7cc30a6
Compare
|
Prepared this one for maintainer review/CI.\n\nChanges made on top of the original patch:\n- Rebased onto current |
Summary
sessions.jsonis written by a Windows host but read inside a Linux Docker container, paths likeC:\Users\...\abc.jsonlare treated as relative bypath.isAbsolute(which only recognises POSIX absolutes)resolvePathWithinSessionsDirto concatenate the Windows path onto the container base, producing broken paths like/home/node/.openclaw/.../C:\Users\...\abc.jsonlisWindowsAbsoluteOnPosix()guard that detects drive-letter paths on POSIX and extracts the leaf filename, resolving it within the sessions directoryTest plan
resolvePathWithinSessionsDircorrectly extracts basename fromC:\Users\foo\sessions\abc.jsonlon LinuxC:/forward/slash/abc.jsonlvariant also handledpath.sep === "\\")🤖 Generated with Claude Code