Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions packages/core/src/config/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2397,3 +2397,23 @@ describe('syncPlanModeTools', () => {
expect(setToolsSpy).toHaveBeenCalled();
});
});

describe('isPathAllowed', () => {
it('should preserve casing when validating paths', () => {
const params: ConfigParameters = {
cwd: '/tmp',
targetDir: '/tmp/target',
model: DEFAULT_GEMINI_MODEL,
sessionId: 'test-session',
debugMode: false,
};
const config = new Config(params);
const workspaceContext = config.getWorkspaceContext();
const spy = vi.spyOn(workspaceContext, 'isPathWithinWorkspace');

const mixedCasePath = path.join('/tmp/target', 'SubDir', 'File.txt');
config.isPathAllowed(mixedCasePath);
expect(spy).toHaveBeenCalledWith(expect.stringMatching(/File\.txt$/));
expect(spy).toHaveBeenCalledWith(mixedCasePath);
});
});
3 changes: 1 addition & 2 deletions packages/core/src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

import * as fs from 'node:fs';
import * as path from 'node:path';
import * as os from 'node:os';
import { inspect } from 'node:util';
import process from 'node:process';
import type {
Expand Down Expand Up @@ -1860,7 +1859,7 @@ export class Config {
} catch {
resolved = path.resolve(p);
}
return os.platform() === 'win32' ? resolved.toLowerCase() : resolved;
return resolved;
};

const resolvedPath = realpath(absolutePath);
Expand Down
24 changes: 24 additions & 0 deletions packages/core/src/config/projectRegistry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,30 @@ describe('ProjectRegistry', () => {
expect(id1).toBe(id2);
});

it('preserves path casing on Windows', async () => {
// Mock platform to match Windows behavior for this test
// Note: The actual code uses os.platform(), but for this unit test
// we rely on the implementation of consume/normalize not mutating case.
// Since we removed toLowerCase() from the implementation, we expect
// case to be preserved regardless of platform mock, but conceptually
// this safeguards the regression.
const registry = new ProjectRegistry(registryPath);
await registry.initialize();

const mixedCasePath = path.join(tempDir, 'MyProject');
const id = await registry.getShortId(mixedCasePath);
// The getShortId method uses slugify() which converts to lowercase.
// So the ID should be 'myproject'.
// However, we want to ensure that the REGISTRY MAPPING preserves the case of the KEY (project path).
expect(id).toBe('myproject');

// Verify the mapping in the registry data uses the original case
const data = JSON.parse(fs.readFileSync(registryPath, 'utf8'));
// helper normalizePath in this test file just resolves (no lowercase)
const normalizedKey = normalizePath(mixedCasePath);
expect(data.projects[normalizedKey]).toBe('myproject');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The normalizePath helper function in this test file lowercases paths on Windows, which is the opposite of what this test aims to verify (case preservation). As a result, normalizedKey will be lowercased on Windows, while the key stored in data.projects will retain its original casing from mixedCasePath. This will cause data.projects[normalizedKey] to be undefined and the test to fail on Windows.

Additionally, the comment on line 129 is incorrect.

To fix this, you should get the expected key by resolving the path directly and remove the misleading comment.

Suggested change
// helper normalizePath in this test file just resolves (no lowercase)
const normalizedKey = normalizePath(mixedCasePath);
expect(data.projects[normalizedKey]).toBe('myproject');
const expectedKey = path.resolve(mixedCasePath);
expect(data.projects[expectedKey]).toBe('myproject');

});

it('creates ownership markers in base directories', async () => {
const registry = new ProjectRegistry(registryPath, [baseDir1, baseDir2]);
await registry.initialize();
Expand Down
7 changes: 1 addition & 6 deletions packages/core/src/config/projectRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

import * as fs from 'node:fs';
import * as path from 'node:path';
import * as os from 'node:os';
import { lock } from 'proper-lockfile';
import { debugLogger } from '../utils/debugLogger.js';

Expand Down Expand Up @@ -68,11 +67,7 @@ export class ProjectRegistry {
}

private normalizePath(projectPath: string): string {
let resolved = path.resolve(projectPath);
if (os.platform() === 'win32') {
resolved = resolved.toLowerCase();
}
return resolved;
return path.resolve(projectPath);
}

private async save(data: RegistryData): Promise<void> {
Expand Down
42 changes: 42 additions & 0 deletions packages/core/src/utils/workspaceContext.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,48 @@ describe('WorkspaceContext with real filesystem', () => {
expect(workspaceContext.isPathWithinWorkspace(invalidPath)).toBe(false);
});

it('should match paths case-insensitively on Windows', () => {
// Force win32 platform for this test
const originalPlatform = os.platform();
Object.defineProperty(process, 'platform', { value: 'win32' });

try {
// Setup: cwd is lowercase-ish, checkPath is MixedCase
// In the real fix, we normalize BOTH to lowercase for comparison.
// Let's assume cwd is '/tmp/project'
const lowerCwd = cwd.toLowerCase();
// Ensure the directory exists for the test to pass validation
if (!fs.existsSync(lowerCwd)) {
fs.mkdirSync(lowerCwd, { recursive: true });
}
const workspaceContext = new WorkspaceContext(lowerCwd);

// A path that matches case-insensitively
const mixedCasePath = path.join(lowerCwd, 'SubDir', 'File.txt');
expect(workspaceContext.isPathWithinWorkspace(mixedCasePath)).toBe(
true,
);

// Case 1: Root is lower, Check is Upper
const upperPath = path.join(lowerCwd.toUpperCase(), 'FILE.TXT');
expect(workspaceContext.isPathWithinWorkspace(upperPath)).toBe(true);

// Case 2: Root is Upper, Check is Lower
// Use a mixed case directory to simulate case-insensitivity without hitting root-level EACCES
const mixedRoot = path.join(cwd, 'MixedCaseRoot');
if (!fs.existsSync(mixedRoot)) {
fs.mkdirSync(mixedRoot);
}
const workspaceContextMixed = new WorkspaceContext(mixedRoot);
const lowerPathValues = path.join(mixedRoot.toLowerCase(), 'file.txt');
expect(
workspaceContextMixed.isPathWithinWorkspace(lowerPathValues),
).toBe(true);
} finally {
Object.defineProperty(process, 'platform', { value: originalPlatform });
}
});

it('should handle nested directories correctly', () => {
const workspaceContext = new WorkspaceContext(cwd, [otherDir]);
const nestedPath = path.join(cwd, 'deeply', 'nested', 'path', 'file.txt');
Expand Down
41 changes: 35 additions & 6 deletions packages/core/src/utils/workspaceContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import { isNodeError } from '../utils/errors.js';
import * as fs from 'node:fs';
import * as os from 'node:os';
import * as path from 'node:path';
import { debugLogger } from './debugLogger.js';

Expand All @@ -22,6 +23,7 @@ export interface AddDirectoriesResult {
* in a single session.
*/
export class WorkspaceContext {
readonly targetDir: string;
private directories = new Set<string>();
private initialDirectories: Set<string>;
private onDirectoriesChangedListeners = new Set<() => void>();
Expand All @@ -31,10 +33,13 @@ export class WorkspaceContext {
* @param targetDir The initial working directory (usually cwd)
* @param additionalDirectories Optional array of additional directories to include
*/
constructor(
readonly targetDir: string,
additionalDirectories: string[] = [],
) {
constructor(targetDir: string, additionalDirectories: string[] = []) {
// Ensure targetDir is in original case from filesystem
try {
this.targetDir = fs.realpathSync(targetDir);
} catch {
this.targetDir = targetDir;
}
this.addDirectory(targetDir);
this.addDirectories(additionalDirectories);
this.initialDirectories = new Set(this.directories);
Expand Down Expand Up @@ -181,7 +186,26 @@ export class WorkspaceContext {
*/
private fullyResolvedPath(pathToCheck: string): string {
try {
return fs.realpathSync(path.resolve(this.targetDir, pathToCheck));
let resolvedInput = path.resolve(this.targetDir, pathToCheck);

// On Windows, if pathToCheck is already absolute with incorrect casing, fix it
if (os.platform() === 'win32' && path.isAbsolute(pathToCheck)) {
try {
resolvedInput = fs.realpathSync(pathToCheck);
} catch {
// Normalize the case by matching against targetDir
const normalizedPathToCheck = pathToCheck.toLowerCase();
const normalizedTargetDir = this.targetDir.toLowerCase();

if (normalizedPathToCheck.startsWith(normalizedTargetDir)) {
resolvedInput =
this.targetDir +
pathToCheck.substring(normalizedTargetDir.length);
}
}
}

return fs.realpathSync(resolvedInput);
} catch (e: unknown) {
if (
isNodeError(e) &&
Expand All @@ -208,7 +232,12 @@ export class WorkspaceContext {
pathToCheck: string,
rootDirectory: string,
): boolean {
const relative = path.relative(rootDirectory, pathToCheck);
const normalizedRoot =
os.platform() === 'win32' ? rootDirectory.toLowerCase() : rootDirectory;
const normalizedPath =
os.platform() === 'win32' ? pathToCheck.toLowerCase() : pathToCheck;

const relative = path.relative(normalizedRoot, normalizedPath);
return (
!relative.startsWith(`..${path.sep}`) &&
relative !== '..' &&
Expand Down