Skip to content
Closed
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
24 changes: 24 additions & 0 deletions apps/web/src/diffFileActions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,30 @@ describe("openDiffFilePrimaryAction", () => {
).toBe("src/index.ts");
});

it("keeps a literal backslash in a POSIX filename", () => {
expect(
resolveDiffPathForWorkspace({
filePath: "src/a\\b.txt",
workspaceRoot: "/repo",
repositoryRoot: "/repo",
}),
).toBe("src/a\\b.txt");
expect(
resolveDiffPathForWorkspace({
filePath: "/repo/src/a\\b.txt",
workspaceRoot: "/repo",
repositoryRoot: "/repo",
}),
).toBe("src/a\\b.txt");
expect(
resolveDiffPathForWorkspace({
filePath: "pkg\\one/src/index.ts",
workspaceRoot: "/repo/pkg\\one",
repositoryRoot: "/repo",
}),
).toBe("src/index.ts");
});

// regression: #6174 opening diff files broke for repositories at a filesystem root
it.each([
{ workspaceRoot: "/frontend", repositoryRoot: "/" },
Expand Down
27 changes: 18 additions & 9 deletions apps/web/src/diffFileActions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,20 @@ interface OpenDiffFilePrimaryActionInput {
readonly openInEditor: (targetPath: string) => void;
}

function normalizedRelativePathSegments(filePath: string): ReadonlyArray<string> | null {
function normalizedRelativePathSegments(
filePath: string,
workspaceRoot: string | undefined,
): ReadonlyArray<string> | null {
if (filePath.startsWith("/") || isWindowsAbsolutePath(filePath) || /^[a-zA-Z]:/.test(filePath)) {
return null;
}

const segments = filePath
.replaceAll("\\", "/")
.split("/")
.filter((segment) => segment.length > 0 && segment !== ".");
// Backslashes are legal filename characters on POSIX hosts.
const path =
workspaceRoot === undefined || isWindowsAbsolutePath(workspaceRoot)
? filePath.replaceAll("\\", "/")
: filePath;
Comment on lines +29 to +32

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Apply the same path-style splitting to workspace prefixes

When a nested POSIX project directory contains a literal backslash, such as workspace /repo/front\end inside repository /repo, a repository-relative diff path front\end/src/index.ts now resolves to null instead of the previous src/index.ts. The new file-segment normalization preserves front\end as one component, but repositoryRelativeWorkspaceSegments still splits it on both separators, so the workspace-membership comparison fails. Consequently, openDiffFilePrimaryAction silently ignores clicks on these files. Make workspace-prefix segmentation use the same path-style rule.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 9a2d737: workspace segments now split on \ only for Windows roots, with a nested POSIX workspace test.

const segments = path.split("/").filter((segment) => segment.length > 0 && segment !== ".");
if (segments.length === 0 || segments.includes("..")) return null;
return segments;
}
Expand All @@ -40,15 +45,16 @@ function repositoryRelativeWorkspaceSegments(
const normalizedRepositoryRoot = normalizeProjectPathForComparison(repositoryRoot);
if (normalizedWorkspaceRoot === normalizedRepositoryRoot) return [];

const separator = normalizedRepositoryRoot.includes("\\") ? "\\" : "/";
const windows = isWindowsAbsolutePath(workspaceRoot);
const separator = windows ? "\\" : "/";
const repositoryPrefix = normalizedRepositoryRoot.endsWith(separator)
? normalizedRepositoryRoot
: `${normalizedRepositoryRoot}${separator}`;
if (!normalizedWorkspaceRoot.startsWith(repositoryPrefix)) return null;

return normalizedWorkspaceRoot
.slice(repositoryPrefix.length)
.split(/[\\/]+/)
.split(windows ? /[\\/]+/ : /\/+/)
.filter(Boolean);
}

Expand All @@ -60,11 +66,14 @@ export function resolveDiffPathForWorkspace(input: {
// Claude and ACP tool-call diffs name files by absolute path.
if (isAbsolutePath(input.filePath)) {
const relativePath = workspaceRelativeFilePath(input.filePath, input.workspaceRoot);
const segments = relativePath === null ? null : normalizedRelativePathSegments(relativePath);
const segments =
relativePath === null
? null
: normalizedRelativePathSegments(relativePath, input.workspaceRoot);
return segments ? segments.join("/") : null;
}

const fileSegments = normalizedRelativePathSegments(input.filePath);
const fileSegments = normalizedRelativePathSegments(input.filePath, input.workspaceRoot);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
if (!fileSegments) return null;

const workspaceSegments = repositoryRelativeWorkspaceSegments(
Expand Down
8 changes: 5 additions & 3 deletions apps/web/src/fileContextMenu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,14 +89,15 @@ function buildFileContextMenuItems(input: {
export function useFileContextMenu(environmentId: EnvironmentId | null) {
const openInEditor = useAtomCommand(shellEnvironment.openInEditor, { reportFailure: false });
const serverConfig = useAtomValue(serverEnvironment.configValueAtom(environmentId));
const canOpenInEditor = useAtomValue(shellEnvironment.openInEditor.permissionAtom(environmentId));

const availableEditors = serverConfig?.availableEditors ?? [];
const availableEditors = canOpenInEditor ? (serverConfig?.availableEditors ?? []) : [];
const capabilities: FileContextMenuCapabilities = {
// WSL can reveal in Windows Explorer despite reporting Linux; use server wording.
revealLabel:
environmentId !== null &&
serverConfig?.shellRevealInFileManager === true &&
serverConfig.availableEditors.includes("file-manager")
availableEditors.includes("file-manager")
? serverConfig.shellRevealInFileManagerKind === undefined
? revealInFileExplorerLabelForOs(serverConfig.environment.platform.os)
: revealInFileExplorerLabelForKind(serverConfig.shellRevealInFileManagerKind)
Expand All @@ -117,7 +118,8 @@ export function useFileContextMenu(environmentId: EnvironmentId | null) {
action === "open" || reveal
? ("file-manager" as const)
: (action.slice("editor:".length) as EditorId);
if (action !== "open" && !reveal && !capabilities.editorIds.includes(editor)) return;
if (!capabilities.editorIds.includes(editor)) return;
if (reveal && capabilities.revealLabel === undefined) return;

const result = await openInEditor({
environmentId,
Expand Down
47 changes: 30 additions & 17 deletions packages/client-runtime/src/state/commandPermissions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
AuthAccessReadScope,
AuthAccessWriteScope,
AuthEnvironmentMaintainScope,
AuthFilesystemReadScope,
AuthOrchestrationOperateScope,
AuthOrchestrationReadScope,
AuthPreviewOperateScope,
Expand Down Expand Up @@ -204,23 +205,35 @@ describe("command permissions", () => {
),
);

it.effect.each([WS_METHODS.scheduledTasksDelete, ORCHESTRATION_V2_WS_METHODS.stopWorkflow])(
"uses the target grant for both availability and execution of %s",
(method) =>
Effect.scoped(
Effect.gen(function* () {
const registry = yield* setup;
const permissions = createCommandPermissions(runtime, method);
registry.set(sessions(env), AsyncResult.success(grant(true)));
expect(registry.get(permissions.permissionAtom(env))).toBe(true);
expect(permissions.permissionAtom(env)).toBe(permissions.permissionAtom(env));
yield* permissions.authorize(registry, env);
registry.set(sessions(other), AsyncResult.success(grant(false)));
expect(registry.get(permissions.permissionAtom(other))).toBe(false);
const denied = yield* permissions.authorize(registry, other).pipe(Effect.flip);
expect(denied._tag).toBe("EnvironmentAuthorizationError");
}),
),
it.effect.each([
WS_METHODS.scheduledTasksDelete,
ORCHESTRATION_V2_WS_METHODS.stopWorkflow,
WS_METHODS.shellOpenInEditor,
])("uses the target grant for both availability and execution of %s", (method) =>
Effect.scoped(
Effect.gen(function* () {
const registry = yield* setup;
const permissions = createCommandPermissions(runtime, method);
registry.set(sessions(env), AsyncResult.success(grant(true)));
expect(registry.get(permissions.permissionAtom(env))).toBe(true);
expect(permissions.permissionAtom(env)).toBe(permissions.permissionAtom(env));
yield* permissions.authorize(registry, env);
registry.set(
sessions(other),
AsyncResult.success({
...grant(false),
scopes: [AuthFilesystemReadScope],
permissions: [AuthFilesystemReadScope],
}),
);
expect(registry.get(permissions.permissionAtom(other))).toBe(false);
const denied = yield* permissions.authorize(registry, other).pipe(Effect.flip);
expect(denied).toMatchObject({
_tag: "EnvironmentAuthorizationError",
requiredPermission: AuthOrchestrationOperateScope,
});
}),
),
);
it.effect("waits for an initial grant", () =>
Effect.scoped(
Expand Down
1 change: 1 addition & 0 deletions packages/contracts/src/clientRpcPermissions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ export const CLIENT_GUARDED_RPC_SCOPES = {

// Starts an agent session on this environment that can read its logs and database.
[WS_METHODS.serverPrepareTriage]: AuthOrchestrationOperateScope,
[WS_METHODS.shellOpenInEditor]: AuthOrchestrationOperateScope,
[WS_METHODS.scheduledTasksUpsert]: AuthOrchestrationOperateScope,
[WS_METHODS.scheduledTasksSetEnabled]: AuthOrchestrationOperateScope,
[WS_METHODS.scheduledTasksDelete]: AuthOrchestrationOperateScope,
Expand Down
Loading