Skip to content

Conversation

@0xMink
Copy link
Contributor

@0xMink 0xMink commented Feb 9, 2026

Closes #11343

Summary

  • initShadowGit() compared core.worktree to this.workspaceDir using raw string equality, which fails on Windows when git stores the path with forward slashes but VS Code resolves it with backslashes, or when drive-letter casing differs
  • Reused the existing arePathsEqual() utility from src/utils/path.ts which already handles path normalization and case-insensitive comparison on Windows
  • Added .trim() on core.worktree output to handle trailing whitespace/newlines from git config reads
  • Added explicit fail-closed guard: missing core.worktree now throws a clear error instead of producing a misleading mismatch message

Test plan

  • New: trailing-newline in core.worktree does not cause false mismatch (stub-based integration test)
  • New: missing core.worktree throws explicit error (integration test with config unset)
  • Existing: all 31 prior tests continue to pass
  • Total: 33 tests passing

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working labels Feb 9, 2026
@roomote
Copy link
Contributor

roomote bot commented Feb 9, 2026

Rooviewer Clock   See task

Both issues from the previous review are resolved. No new issues found.

  • normalizePathForCompare duplicates existing arePathsEqual() -- consider reusing it with .trim() on the git output instead of introducing a new function
  • Test "treats forward and backslash paths as equal on Windows" fails on Linux CI because path.resolve/path.normalize don't convert backslashes to forward slashes on POSIX
Previous reviews

Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues.

Comment on lines 18 to 36
/**
* Normalizes a file path for comparison by resolving relative segments,
* canonicalizing separators, and stripping trailing separators.
* On Windows, also lowercases for case-insensitive comparison.
*/
export function normalizePathForCompare(p: string, isWindows = process.platform === "win32"): string {
let normalized = path.resolve(path.normalize(p.trim()))

const root = path.parse(normalized).root
if (normalized.length > root.length && normalized.endsWith(path.sep)) {
normalized = normalized.slice(0, -1)
}

if (isWindows) {
normalized = normalized.toLowerCase()
}

return normalized
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The codebase already has arePathsEqual() which handles path normalization, trailing separator stripping, and case-insensitive comparison on win32. Using it here would avoid the duplication and the cross-platform test issues described in the next comment. The only gap is .trim() for git config output, which can be applied to the worktree value before passing it to arePathsEqual:

Suggested change
/**
* Normalizes a file path for comparison by resolving relative segments,
* canonicalizing separators, and stripping trailing separators.
* On Windows, also lowercases for case-insensitive comparison.
*/
export function normalizePathForCompare(p: string, isWindows = process.platform === "win32"): string {
let normalized = path.resolve(path.normalize(p.trim()))
const root = path.parse(normalized).root
if (normalized.length > root.length && normalized.endsWith(path.sep)) {
normalized = normalized.slice(0, -1)
}
if (isWindows) {
normalized = normalized.toLowerCase()
}
return normalized
}
import { arePathsEqual } from "../../utils/path"

Then at the comparison site: if (!arePathsEqual(worktree?.trim(), this.workspaceDir)). This was also the approach suggested by the issue investigator in #11343 (comment).

Fix it with Roo Code or mention @roomote and request a fix.

Comment on lines 921 to 925
it("treats forward and backslash paths as equal on Windows", () => {
expect(normalizePathForCompare("C:\\Users\\Dennis\\Repo", true)).toBe(
normalizePathForCompare("C:/Users/Dennis/Repo", true),
)
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails on non-Windows CI (confirmed locally: 1 failed | 6 passed). Node.js path.resolve/path.normalize use the host OS's separator conventions, so on Linux backslashes are treated as literal filename characters, not separators. The isWindows parameter only controls case-folding, not separator behavior. The result on Linux is:

Expected: ".../c:/users/dennis/repo"
Received: ".../c:\users\dennis\repo"

If normalizePathForCompare is kept, this test needs to either be skipped on non-Windows platforms (describe.skipIf(process.platform !== "win32")) or restructured to not depend on cross-platform path module behavior.

Fix it with Roo Code or mention @roomote and request a fix.

@0xMink 0xMink force-pushed the fix/checkpoint-worktree-path-comparison branch from c30f78a to 88c3654 Compare February 9, 2026 21:23
@0xMink
Copy link
Contributor Author

0xMink commented Feb 9, 2026

Thanks, good catches. Updated the PR:

  • Removed normalizePathForCompare() and switched to the existing arePathsEqual() from src/utils/path.ts
  • Added .trim() on core.worktree output before comparison to handle trailing whitespace from git
  • Replaced the unit tests with two platform-stable integration tests: one verifying trailing-newline tolerance via a stub on getShadowGitConfigWorktree, and one verifying the explicit throw when core.worktree is missing
  • 33/33 tests passing on Windows

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Checkpoint worktree path comparison fails on Windows due to path identity mismatch

1 participant