From ebb9b1ae0f2febca580ab49038d077203312c077 Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Thu, 23 Apr 2026 11:15:05 -0700 Subject: [PATCH] fix(ui): gate direct file opens by artifact policy Signed-off-by: Taylor Ho --- .../src/features/chat/ui/ToolCallAdapter.tsx | 102 +++++++----- .../ToolCallAdapter.header-file.test.tsx | 146 ++++++++++++++++++ .../ui/__tests__/ToolCallAdapter.test.tsx | 107 ++++++------- 3 files changed, 263 insertions(+), 92 deletions(-) create mode 100644 ui/goose2/src/features/chat/ui/__tests__/ToolCallAdapter.header-file.test.tsx diff --git a/ui/goose2/src/features/chat/ui/ToolCallAdapter.tsx b/ui/goose2/src/features/chat/ui/ToolCallAdapter.tsx index 3a8edfcc7a..a6ece6783b 100644 --- a/ui/goose2/src/features/chat/ui/ToolCallAdapter.tsx +++ b/ui/goose2/src/features/chat/ui/ToolCallAdapter.tsx @@ -125,18 +125,25 @@ function InputSummary({ function ToolLocations({ locations }: { locations: ToolCallLocation[] }) { const { t } = useTranslation("chat"); - const { pathExists, openResolvedPath } = useArtifactPolicyContext(); + const { pathExists, openResolvedPath, resolveMarkdownHref } = + useArtifactPolicyContext(); const [openError, setOpenError] = useState(null); const openLocation = async (path: string) => { try { setOpenError(null); - const exists = await pathExists(path); - if (!exists) { - setOpenError(t("tools.fileNotFound", { path })); + const candidate = resolveMarkdownHref(path); + if (!candidate?.allowed) { + setOpenError(candidate?.blockedReason || t("tools.pathOutsideRoots")); return; } - await openResolvedPath(path); + + const exists = await pathExists(candidate.resolvedPath); + if (!exists) { + setOpenError(t("tools.fileNotFound", { path: candidate.resolvedPath })); + return; + } + await openResolvedPath(candidate.resolvedPath); } catch (error) { setOpenError(error instanceof Error ? error.message : String(error)); } @@ -145,19 +152,24 @@ function ToolLocations({ locations }: { locations: ToolCallLocation[] }) { return (
- {locations.map((location) => ( - - ))} + {locations.map((location) => { + const candidate = resolveMarkdownHref(location.path); + + return ( + + ); + })}
{openError ? (

{openError}

@@ -371,7 +383,7 @@ export function ToolCallAdapter({ locations: visibleLocations, arguments: args, }); - const { openResolvedPath } = useArtifactPolicyContext(); + const { openResolvedPath, resolveMarkdownHref } = useArtifactPolicyContext(); const rawResult = result ?? (typeof rawOutput === "string" @@ -389,7 +401,13 @@ export function ToolCallAdapter({ headerFileLabel && headerFilePath ? splitHeaderTitle(name, headerFileLabel) : null; - const canOpenHeaderFile = Boolean(headerFilePath && headerTitleParts); + const headerFileCandidate = useMemo( + () => (headerFilePath ? resolveMarkdownHref(headerFilePath) : null), + [headerFilePath, resolveMarkdownHref], + ); + const canOpenHeaderFile = Boolean( + headerTitleParts && headerFileCandidate?.allowed, + ); return (
@@ -405,25 +423,31 @@ export function ToolCallAdapter({ headerTitleParts ? ( <> {headerTitleParts.prefix} - + {canOpenHeaderFile ? ( + + ) : ( + {headerTitleParts.fileLabel} + )} {headerTitleParts.suffix} ) : ( diff --git a/ui/goose2/src/features/chat/ui/__tests__/ToolCallAdapter.header-file.test.tsx b/ui/goose2/src/features/chat/ui/__tests__/ToolCallAdapter.header-file.test.tsx new file mode 100644 index 0000000000..aeed075b54 --- /dev/null +++ b/ui/goose2/src/features/chat/ui/__tests__/ToolCallAdapter.header-file.test.tsx @@ -0,0 +1,146 @@ +import { render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { ToolCardDisplay } from "@/features/chat/hooks/ArtifactPolicyContext"; +import type { ArtifactPathCandidate } from "@/features/chat/lib/artifactPathPolicy"; +import { ToolCallAdapter } from "../ToolCallAdapter"; + +const mockResolveToolCardDisplay = + vi.fn< + ( + args: Record, + name: string, + result?: string, + ) => ToolCardDisplay + >(); +const mockResolveMarkdownHref = + vi.fn<(href: string) => ArtifactPathCandidate | null>(); +const mockPathExists = vi.fn<(path: string) => Promise>(); +const mockOpenResolvedPath = vi.fn<(path: string) => Promise>(); + +vi.mock("@/features/chat/hooks/ArtifactPolicyContext", () => ({ + useArtifactPolicyContext: () => ({ + resolveToolCardDisplay: mockResolveToolCardDisplay, + resolveMarkdownHref: mockResolveMarkdownHref, + pathExists: mockPathExists, + openResolvedPath: mockOpenResolvedPath, + }), +})); + +const EMPTY_DISPLAY: ToolCardDisplay = { + role: "none", + primaryCandidate: null, + secondaryCandidates: [], +}; + +function makeCandidate( + overrides: Partial = {}, +): ArtifactPathCandidate { + return { + id: "c-1", + rawPath: "/project/Sources/main.swift", + resolvedPath: "/project/Sources/main.swift", + source: "arg_key", + confidence: "high", + kind: "file", + allowed: true, + blockedReason: null, + toolCallId: "tool-1", + toolName: "edit_file", + toolCallIndex: 0, + appearanceIndex: 0, + ...overrides, + }; +} + +function renderAdapter( + overrides: Partial[0]> = {}, +) { + return render( + , + ); +} + +beforeEach(() => { + mockResolveToolCardDisplay.mockReset(); + mockResolveToolCardDisplay.mockReturnValue(EMPTY_DISPLAY); + mockResolveMarkdownHref.mockReset(); + mockResolveMarkdownHref.mockImplementation((href) => + makeCandidate({ + rawPath: href, + resolvedPath: href, + }), + ); + mockPathExists.mockReset(); + mockPathExists.mockResolvedValue(true); + mockOpenResolvedPath.mockReset(); + mockOpenResolvedPath.mockResolvedValue(undefined); +}); + +describe("ToolCallAdapter header file link", () => { + it("opens the file from the header filename without expanding the accordion", async () => { + const user = userEvent.setup(); + + renderAdapter(); + + await user.click(screen.getByRole("button", { name: /open main\.swift/i })); + + expect(mockOpenResolvedPath).toHaveBeenCalledWith( + "/project/Sources/main.swift", + ); + expect(screen.queryByText("Path")).not.toBeInTheDocument(); + }); + + it("does not expose a clickable header filename outside allowed roots", async () => { + const user = userEvent.setup(); + mockResolveMarkdownHref.mockReturnValue( + makeCandidate({ + rawPath: "/etc/passwd", + resolvedPath: "/etc/passwd", + allowed: false, + blockedReason: "Path is outside allowed project/artifacts roots.", + }), + ); + + renderAdapter({ + name: "Edit passwd", + arguments: { path: "/etc/passwd", line: 1 }, + result: "Updated /etc/passwd", + }); + + expect( + screen.queryByRole("button", { name: /open passwd/i }), + ).not.toBeInTheDocument(); + + await user.click(screen.getByText("passwd")); + + expect(mockOpenResolvedPath).not.toHaveBeenCalled(); + expect(screen.getByText("Path")).toBeInTheDocument(); + }); + + it("opens the accordion when clicking the non-file title text", async () => { + const user = userEvent.setup(); + + const { container } = renderAdapter(); + + const titlePrefix = container.querySelector("[data-tool-title-prefix]"); + expect(titlePrefix).toBeTruthy(); + + if (!titlePrefix) { + throw new Error("Expected non-file title text"); + } + + await user.click(titlePrefix); + + expect(mockOpenResolvedPath).not.toHaveBeenCalled(); + expect(screen.getByText("Path")).toBeInTheDocument(); + }); +}); diff --git a/ui/goose2/src/features/chat/ui/__tests__/ToolCallAdapter.test.tsx b/ui/goose2/src/features/chat/ui/__tests__/ToolCallAdapter.test.tsx index 58d684243d..7565ea58e3 100644 --- a/ui/goose2/src/features/chat/ui/__tests__/ToolCallAdapter.test.tsx +++ b/ui/goose2/src/features/chat/ui/__tests__/ToolCallAdapter.test.tsx @@ -1,6 +1,6 @@ import { render, screen, within } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; -import { describe, expect, it, vi } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import type { ToolCardDisplay } from "@/features/chat/hooks/ArtifactPolicyContext"; import type { ArtifactPathCandidate } from "@/features/chat/lib/artifactPathPolicy"; import { ToolCallAdapter } from "../ToolCallAdapter"; @@ -15,13 +15,15 @@ const mockResolveToolCardDisplay = result?: string, ) => ToolCardDisplay >(); +const mockResolveMarkdownHref = + vi.fn<(href: string) => ArtifactPathCandidate | null>(); const mockPathExists = vi.fn<(path: string) => Promise>(); const mockOpenResolvedPath = vi.fn<(path: string) => Promise>(); vi.mock("@/features/chat/hooks/ArtifactPolicyContext", () => ({ useArtifactPolicyContext: () => ({ resolveToolCardDisplay: mockResolveToolCardDisplay, - resolveMarkdownHref: () => null, + resolveMarkdownHref: mockResolveMarkdownHref, pathExists: mockPathExists, openResolvedPath: mockOpenResolvedPath, }), @@ -55,6 +57,22 @@ function makeCandidate( }; } +beforeEach(() => { + mockResolveToolCardDisplay.mockReset(); + mockResolveToolCardDisplay.mockReturnValue(EMPTY_DISPLAY); + mockResolveMarkdownHref.mockReset(); + mockResolveMarkdownHref.mockImplementation((href) => + makeCandidate({ + rawPath: href, + resolvedPath: href, + }), + ); + mockPathExists.mockReset(); + mockPathExists.mockResolvedValue(true); + mockOpenResolvedPath.mockReset(); + mockOpenResolvedPath.mockResolvedValue(undefined); +}); + function renderAdapter( overrides: Partial[0]> = {}, ) { @@ -113,54 +131,6 @@ describe("ToolCallAdapter — ArtifactActions", () => { expect(rawInputPanel).toHaveTextContent('"path": "/project/src/main.ts"'); }); - it("opens the file from the header filename without expanding the accordion", async () => { - const user = userEvent.setup(); - mockResolveToolCardDisplay.mockReturnValue(EMPTY_DISPLAY); - mockOpenResolvedPath.mockResolvedValue(undefined); - - renderAdapter({ - name: "Edit main.swift", - kind: "edit", - arguments: { path: "/project/Sources/main.swift", line: 1 }, - result: "Updated /project/Sources/main.swift", - open: undefined, - }); - - await user.click(screen.getByRole("button", { name: /open main\.swift/i })); - - expect(mockOpenResolvedPath).toHaveBeenCalledWith( - "/project/Sources/main.swift", - ); - expect(screen.queryByText("Path")).not.toBeInTheDocument(); - }); - - it("opens the accordion when clicking the non-file title text", async () => { - const user = userEvent.setup(); - mockResolveToolCardDisplay.mockReturnValue(EMPTY_DISPLAY); - mockOpenResolvedPath.mockReset(); - mockOpenResolvedPath.mockResolvedValue(undefined); - - const { container } = renderAdapter({ - name: "Edit main.swift", - kind: "edit", - arguments: { path: "/project/Sources/main.swift", line: 1 }, - result: "Updated /project/Sources/main.swift", - open: undefined, - }); - - const titlePrefix = container.querySelector("[data-tool-title-prefix]"); - expect(titlePrefix).toBeTruthy(); - - if (!titlePrefix) { - throw new Error("Expected non-file title text"); - } - - await user.click(titlePrefix); - - expect(mockOpenResolvedPath).not.toHaveBeenCalled(); - expect(screen.getByText("Path")).toBeInTheDocument(); - }); - it("renders command input summaries with a clamped preview and expanded bash highlighting", async () => { const user = userEvent.setup(); mockResolveToolCardDisplay.mockReturnValue(EMPTY_DISPLAY); @@ -227,9 +197,6 @@ describe("ToolCallAdapter — ArtifactActions", () => { it("renders multi-location fallbacks as inline file pills", async () => { const user = userEvent.setup(); - mockResolveToolCardDisplay.mockReturnValue(EMPTY_DISPLAY); - mockPathExists.mockResolvedValue(true); - mockOpenResolvedPath.mockResolvedValue(undefined); renderAdapter({ name: "read_file", @@ -258,6 +225,40 @@ describe("ToolCallAdapter — ArtifactActions", () => { expect(mockOpenResolvedPath).toHaveBeenCalledWith("/project/src/index.js"); }); + it("disables multi-location pills outside allowed roots", () => { + mockResolveMarkdownHref.mockImplementation((href) => + href === "/project/src/index.js" + ? makeCandidate({ + rawPath: href, + resolvedPath: href, + allowed: false, + blockedReason: "Path is outside allowed project/artifacts roots.", + }) + : makeCandidate({ + rawPath: href, + resolvedPath: href, + }), + ); + + renderAdapter({ + name: "read_file", + kind: "read", + arguments: { path: "/project/src/renderer.js", line: 42 }, + locations: [ + { path: "/project/src/renderer.js", line: 42 }, + { path: "/project/src/index.js", line: 12 }, + ], + result: "flattened result", + }); + + expect( + screen.getByRole("button", { name: /renderer\.js:42/i }), + ).toBeEnabled(); + expect( + screen.getByRole("button", { name: /index\.js:12/i }), + ).toBeDisabled(); + }); + it('renders "Open file" button when primary candidate exists', () => { const primary = makeCandidate(); mockResolveToolCardDisplay.mockReturnValue({