Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
33 changes: 18 additions & 15 deletions src/services/tools/fileCommon.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,19 +147,20 @@ describe("fileCommon", () => {
expect(validateNoRedundantPrefix("file.ts", cwd, runtime)).toBeNull();
});

it("should reject absolute paths that contain the cwd prefix", () => {
it("should auto-correct absolute paths that contain the cwd prefix", () => {
const result = validateNoRedundantPrefix("/workspace/project/src/file.ts", cwd, runtime);
expect(result).not.toBeNull();
expect(result?.error).toContain("Redundant path prefix detected");
expect(result?.error).toContain("Please use relative paths to save tokens");
expect(result?.error).toContain("src/file.ts"); // Should suggest the relative path
expect(result?.correctedPath).toBe("src/file.ts");
expect(result?.warning).toContain("Using relative paths");
expect(result?.warning).toContain("saves tokens");
expect(result?.warning).toContain("auto-corrected");
});

it("should reject absolute paths at the cwd root", () => {
it("should auto-correct absolute paths at the cwd root", () => {
const result = validateNoRedundantPrefix("/workspace/project/file.ts", cwd, runtime);
expect(result).not.toBeNull();
expect(result?.error).toContain("Redundant path prefix detected");
expect(result?.error).toContain("file.ts"); // Should suggest the relative path
expect(result?.correctedPath).toBe("file.ts");
expect(result?.warning).toContain("auto-corrected");
});

it("should allow absolute paths outside cwd (they will be caught by validatePathInCwd)", () => {
Expand All @@ -182,7 +183,8 @@ describe("fileCommon", () => {
runtime
);
expect(result).not.toBeNull();
expect(result?.error).toContain("src/file.ts");
expect(result?.correctedPath).toBe("src/file.ts");
expect(result?.warning).toContain("auto-corrected");
});

it("should handle nested paths correctly", () => {
Expand All @@ -192,14 +194,15 @@ describe("fileCommon", () => {
runtime
);
expect(result).not.toBeNull();
expect(result?.error).toContain("src/components/Button/index.ts");
expect(result?.correctedPath).toBe("src/components/Button/index.ts");
expect(result?.warning).toContain("auto-corrected");
});

it("should reject path that equals cwd exactly", () => {
it("should auto-correct path that equals cwd exactly", () => {
const result = validateNoRedundantPrefix("/workspace/project", cwd, runtime);
expect(result).not.toBeNull();
expect(result?.error).toContain("Redundant path prefix detected");
expect(result?.error).toContain("."); // Should suggest current directory
expect(result?.correctedPath).toBe(".");
expect(result?.warning).toContain("auto-corrected");
});

it("should not match partial directory names", () => {
Expand All @@ -217,15 +220,15 @@ describe("fileCommon", () => {
});
const sshCwd = "/home/user/cmux/project/branch";

// Should reject absolute paths with redundant prefix on SSH too
// Should auto-correct absolute paths with redundant prefix on SSH too
const result = validateNoRedundantPrefix(
"/home/user/cmux/project/branch/src/file.ts",
sshCwd,
sshRuntime
);
expect(result).not.toBeNull();
expect(result?.error).toContain("Redundant path prefix detected");
expect(result?.error).toContain("src/file.ts");
expect(result?.correctedPath).toBe("src/file.ts");
expect(result?.warning).toContain("auto-corrected");

// Should allow relative paths on SSH
expect(validateNoRedundantPrefix("src/file.ts", sshCwd, sshRuntime)).toBeNull();
Expand Down
36 changes: 32 additions & 4 deletions src/services/tools/fileCommon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export function validateFileSize(stats: FileStat): { error: string } | null {

/**
* Validates that a file path doesn't contain redundant workspace prefix.
* Returns an error object if the path contains the cwd prefix, null if valid.
* If the path contains the cwd prefix, returns the corrected relative path and a warning.
* This helps save tokens by encouraging relative paths.
*
* Works for both local and SSH runtimes by using runtime.normalizePath()
Expand All @@ -57,13 +57,13 @@ export function validateFileSize(stats: FileStat): { error: string } | null {
* @param filePath - The file path to validate
* @param cwd - The working directory
* @param runtime - The runtime to use for path normalization
* @returns Error object if redundant prefix found, null if valid
* @returns Object with corrected path and warning if redundant prefix found, null if valid
*/
export function validateNoRedundantPrefix(
filePath: string,
cwd: string,
runtime: Runtime
): { error: string } | null {
): { correctedPath: string; warning: string } | null {
// Only check absolute paths (start with /) - relative paths are fine
// This works for both local and SSH since both use Unix-style paths
if (!filePath.startsWith("/")) {
Expand All @@ -87,7 +87,8 @@ export function validateNoRedundantPrefix(
const relativePath =
normalizedPath === cleanCwd ? "." : normalizedPath.substring(cleanCwd.length + 1);
return {
error: `Redundant path prefix detected. The path '${filePath}' contains the workspace directory. Please use relative paths to save tokens: '${relativePath}'`,
correctedPath: relativePath,
warning: `Note: Using relative paths like '${relativePath}' instead of '${filePath}' saves tokens. The path has been auto-corrected for you.`,
};
}

Expand Down Expand Up @@ -135,3 +136,30 @@ export function validatePathInCwd(

return null;
}

/**
* Validates and auto-corrects redundant path prefixes in file paths.
* Returns the corrected path and an optional warning message.
*
* This is a convenience wrapper around validateNoRedundantPrefix that handles
* the common pattern of auto-correcting paths and returning warnings.
*
* @param filePath - The file path to validate (may be modified if redundant prefix found)
* @param cwd - The working directory
* @param runtime - The runtime to use for path normalization
* @returns Object with correctedPath and optional warning
*/
export function validateAndCorrectPath(
filePath: string,
cwd: string,
runtime: Runtime
): { correctedPath: string; warning?: string } {
const redundantPrefixValidation = validateNoRedundantPrefix(filePath, cwd, runtime);
if (redundantPrefixValidation) {
return {
correctedPath: redundantPrefixValidation.correctedPath,
warning: redundantPrefixValidation.warning,
};
}
return { correctedPath: filePath };
}
24 changes: 14 additions & 10 deletions src/services/tools/file_edit_insert.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { tool } from "ai";
import type { FileEditInsertToolResult } from "@/types/tools";
import type { ToolConfiguration, ToolFactory } from "@/utils/tools/tools";
import { TOOL_DEFINITIONS } from "@/utils/tools/toolDefinitions";
import { validatePathInCwd, validateNoRedundantPrefix } from "./fileCommon";
import { validatePathInCwd, validateAndCorrectPath } from "./fileCommon";
import { EDIT_FAILED_NOTE_PREFIX, NOTE_READ_FILE_RETRY } from "@/types/tools";
import { executeFileEditOperation } from "./file_edit_operation";
import { RuntimeError } from "@/runtime/Runtime";
Expand All @@ -23,18 +23,13 @@ export const createFileEditInsertTool: ToolFactory = (config: ToolConfiguration)
{ abortSignal }
): Promise<FileEditInsertToolResult> => {
try {
// Validate no redundant path prefix (must come first to catch absolute paths)
const redundantPrefixValidation = validateNoRedundantPrefix(
// Validate and auto-correct redundant path prefix
const { correctedPath: validatedPath, warning: pathWarning } = validateAndCorrectPath(
file_path,
config.cwd,
config.runtime
);
if (redundantPrefixValidation) {
return {
success: false,
error: redundantPrefixValidation.error,
};
}
file_path = validatedPath;

const pathValidation = validatePathInCwd(file_path, config.cwd, config.runtime);
if (pathValidation) {
Expand Down Expand Up @@ -81,7 +76,7 @@ export const createFileEditInsertTool: ToolFactory = (config: ToolConfiguration)
}
}

return executeFileEditOperation({
const result = await executeFileEditOperation({
config,
filePath: file_path,
abortSignal,
Expand Down Expand Up @@ -119,6 +114,15 @@ export const createFileEditInsertTool: ToolFactory = (config: ToolConfiguration)
};
},
});

// Add path warning if present
if (pathWarning && result.success) {
return {
...result,
warning: pathWarning,
};
}
return result;
} catch (error) {
if (error && typeof error === "object" && "code" in error && error.code === "EACCES") {
return {
Expand Down
14 changes: 5 additions & 9 deletions src/services/tools/file_edit_operation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {
generateDiff,
validateFileSize,
validatePathInCwd,
validateNoRedundantPrefix,
validateAndCorrectPath,
} from "./fileCommon";
import { RuntimeError } from "@/runtime/Runtime";
import { readFileString, writeFileString } from "@/utils/runtime/helpers";
Expand Down Expand Up @@ -43,18 +43,13 @@ export async function executeFileEditOperation<TMetadata>({
FileEditErrorResult | (FileEditDiffSuccessBase & TMetadata)
> {
try {
// Validate no redundant path prefix (must come first to catch absolute paths)
const redundantPrefixValidation = validateNoRedundantPrefix(
// Validate and auto-correct redundant path prefix
const { correctedPath: validatedPath, warning: pathWarning } = validateAndCorrectPath(
filePath,
config.cwd,
config.runtime
);
if (redundantPrefixValidation) {
return {
success: false,
error: redundantPrefixValidation.error,
};
}
filePath = validatedPath;

const pathValidation = validatePathInCwd(filePath, config.cwd, config.runtime);
if (pathValidation) {
Expand Down Expand Up @@ -139,6 +134,7 @@ export async function executeFileEditOperation<TMetadata>({
success: true,
diff,
...operationResult.metadata,
...(pathWarning && { warning: pathWarning }),
};
} catch (error) {
if (error && typeof error === "object" && "code" in error) {
Expand Down
14 changes: 7 additions & 7 deletions src/services/tools/file_read.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ describe("file_read tool", () => {
}
});

it("should reject absolute paths containing the workspace directory", async () => {
it("should auto-correct absolute paths containing the workspace directory", async () => {
// Setup
const content = "test content";
await fs.writeFile(testFilePath, content);
Expand All @@ -332,12 +332,12 @@ describe("file_read tool", () => {
// Execute
const result = (await tool.execute!(args, mockToolCallOptions)) as FileReadToolResult;

// Assert
expect(result.success).toBe(false);
if (!result.success) {
expect(result.error).toContain("Redundant path prefix detected");
expect(result.error).toContain("Please use relative paths to save tokens");
expect(result.error).toContain("test.txt"); // Should suggest the relative path
// Assert - Should succeed with auto-correction warning
expect(result.success).toBe(true);
if (result.success) {
expect(result.warning).toContain("auto-corrected");
expect(result.warning).toContain("test.txt"); // Should mention the relative path
expect(result.content).toContain("test content");
}
});

Expand Down
14 changes: 5 additions & 9 deletions src/services/tools/file_read.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { tool } from "ai";
import type { FileReadToolResult } from "@/types/tools";
import type { ToolConfiguration, ToolFactory } from "@/utils/tools/tools";
import { TOOL_DEFINITIONS } from "@/utils/tools/toolDefinitions";
import { validatePathInCwd, validateFileSize, validateNoRedundantPrefix } from "./fileCommon";
import { validatePathInCwd, validateFileSize, validateAndCorrectPath } from "./fileCommon";
import { RuntimeError } from "@/runtime/Runtime";
import { readFileString } from "@/utils/runtime/helpers";

Expand All @@ -22,18 +22,13 @@ export const createFileReadTool: ToolFactory = (config: ToolConfiguration) => {
// Note: abortSignal available but not used - file reads are fast and complete quickly

try {
// Validate no redundant path prefix (must come first to catch absolute paths)
const redundantPrefixValidation = validateNoRedundantPrefix(
// Validate and auto-correct redundant path prefix
const { correctedPath: validatedPath, warning: pathWarning } = validateAndCorrectPath(
filePath,
config.cwd,
config.runtime
);
if (redundantPrefixValidation) {
return {
success: false,
error: redundantPrefixValidation.error,
};
}
filePath = validatedPath;

// Validate that the path is within the working directory
const pathValidation = validatePathInCwd(filePath, config.cwd, config.runtime);
Expand Down Expand Up @@ -172,6 +167,7 @@ export const createFileReadTool: ToolFactory = (config: ToolConfiguration) => {
modifiedTime: fileStat.modifiedTime.toISOString(),
lines_read: numberedLines.length,
content,
...(pathWarning && { warning: pathWarning }),
};
} catch (error) {
// Handle specific errors
Expand Down
3 changes: 1 addition & 2 deletions src/telemetry/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,8 @@ import { VERSION } from "../version";
* Get base telemetry properties included with all events
*/
export function getBaseTelemetryProperties(): BaseTelemetryProperties {
const gitDescribe: string = VERSION.git_describe;
return {
version: gitDescribe,
version: String(VERSION.git_describe),
platform: window.api?.platform || "unknown",
electronVersion: window.api?.versions?.electron || "unknown",
};
Expand Down
2 changes: 2 additions & 0 deletions src/types/tools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ export type FileReadToolResult =
modifiedTime: string;
lines_read: number;
content: string;
warning?: string;
}
| {
success: false;
Expand All @@ -61,6 +62,7 @@ export type FileReadToolResult =
export interface FileEditDiffSuccessBase {
success: true;
diff: string;
warning?: string;
}

export interface FileEditErrorResult {
Expand Down