From e6e7c8b7cde3623fbe25d8822014e2a2547ea5bb Mon Sep 17 00:00:00 2001 From: tulsi Date: Thu, 2 Apr 2026 10:45:30 -0700 Subject: [PATCH] Allow opening explicit outputs outside artifact roots (#82) * Allow opening explicit outputs outside artifact roots * fix: tighten artifact path opening rules --- ui/goose2/src-tauri/src/commands/system.rs | 6 + ui/goose2/src-tauri/src/lib.rs | 1 + .../chat/hooks/ArtifactPolicyContext.tsx | 41 +++-- .../lib/__tests__/artifactPathPolicy.test.ts | 90 +++++++++- .../chat/lib/artifactPathCommandExtraction.ts | 139 +++++++++++++++ .../chat/lib/artifactPathPolicyCore.ts | 63 ++++--- .../src/features/chat/ui/ToolCallCard.tsx | 62 +++++-- .../ui/__tests__/MarkdownContent.test.tsx | 9 +- .../chat/ui/__tests__/ToolCallCard.test.tsx | 167 +++++++++++++++++- ui/goose2/src/shared/api/system.ts | 4 + 10 files changed, 529 insertions(+), 53 deletions(-) create mode 100644 ui/goose2/src/features/chat/lib/artifactPathCommandExtraction.ts diff --git a/ui/goose2/src-tauri/src/commands/system.rs b/ui/goose2/src-tauri/src/commands/system.rs index 04213d23ad..d1bda0cefc 100644 --- a/ui/goose2/src-tauri/src/commands/system.rs +++ b/ui/goose2/src-tauri/src/commands/system.rs @@ -3,3 +3,9 @@ pub fn get_home_dir() -> Result { let home_dir = dirs::home_dir().ok_or("Could not determine home directory")?; Ok(home_dir.to_string_lossy().into_owned()) } + +#[tauri::command] +#[allow(dead_code)] +pub fn path_exists(path: String) -> bool { + std::path::Path::new(&path).exists() +} diff --git a/ui/goose2/src-tauri/src/lib.rs b/ui/goose2/src-tauri/src/lib.rs index 6837f5eb84..894a03a211 100644 --- a/ui/goose2/src-tauri/src/lib.rs +++ b/ui/goose2/src-tauri/src/lib.rs @@ -70,6 +70,7 @@ pub fn run() { commands::doctor::run_doctor, commands::doctor::run_doctor_fix, commands::system::get_home_dir, + commands::system::path_exists, ]) .build(tauri::generate_context!()) .expect("error while building tauri application") diff --git a/ui/goose2/src/features/chat/hooks/ArtifactPolicyContext.tsx b/ui/goose2/src/features/chat/hooks/ArtifactPolicyContext.tsx index b2f8a98469..990beccefa 100644 --- a/ui/goose2/src/features/chat/hooks/ArtifactPolicyContext.tsx +++ b/ui/goose2/src/features/chat/hooks/ArtifactPolicyContext.tsx @@ -8,6 +8,7 @@ import { type ReactNode, } from "react"; import type { Message } from "@/shared/types/messages"; +import { pathExists } from "@/shared/api/system"; import { buildArtifactsIndexForMessages, resolveMarkdownLocalHref, @@ -27,6 +28,7 @@ interface ArtifactPolicyContextValue { result?: string, ) => ToolCardDisplay; resolveMarkdownHref: (href: string) => ArtifactPathCandidate | null; + pathExists: (path: string) => Promise; openResolvedPath: (path: string) => Promise; } @@ -39,6 +41,7 @@ const EMPTY_DISPLAY: ToolCardDisplay = { const DEFAULT_CONTEXT_VALUE: ArtifactPolicyContextValue = { resolveToolCardDisplay: () => EMPTY_DISPLAY, resolveMarkdownHref: () => null, + pathExists: async () => false, openResolvedPath: async () => {}, }; @@ -94,24 +97,40 @@ export function ArtifactPolicyProvider({ [normalizedRoots], ); - const openResolvedPath = useCallback(async (path: string) => { - const key = path.trim().toLowerCase(); - const now = Date.now(); - const lastOpenAt = lastOpenAtByPathRef.current.get(key) ?? 0; - if (now - lastOpenAt < 1200) { - return; - } - lastOpenAtByPathRef.current.set(key, now); - await openPath(path); - }, []); + const checkPathExists = useCallback((path: string) => pathExists(path), []); + + const openResolvedPath = useCallback( + async (path: string) => { + const exists = await checkPathExists(path); + if (!exists) { + throw new Error(`File not found: ${path}`); + } + + const key = path.trim().toLowerCase(); + const now = Date.now(); + const lastOpenAt = lastOpenAtByPathRef.current.get(key) ?? 0; + if (now - lastOpenAt < 1200) { + return; + } + lastOpenAtByPathRef.current.set(key, now); + await openPath(path); + }, + [checkPathExists], + ); const contextValue = useMemo( () => ({ resolveToolCardDisplay, resolveMarkdownHref, + pathExists: checkPathExists, openResolvedPath, }), - [openResolvedPath, resolveMarkdownHref, resolveToolCardDisplay], + [ + checkPathExists, + openResolvedPath, + resolveMarkdownHref, + resolveToolCardDisplay, + ], ); return ( diff --git a/ui/goose2/src/features/chat/lib/__tests__/artifactPathPolicy.test.ts b/ui/goose2/src/features/chat/lib/__tests__/artifactPathPolicy.test.ts index 495e5be133..20d8196b08 100644 --- a/ui/goose2/src/features/chat/lib/__tests__/artifactPathPolicy.test.ts +++ b/ui/goose2/src/features/chat/lib/__tests__/artifactPathPolicy.test.ts @@ -116,6 +116,46 @@ describe("artifactPathPolicy", () => { expect(blocked.blockedReason).toContain("outside allowed"); }); + it("allows explicit write outputs outside default roots", () => { + const ranking = rankMessageToolArtifacts( + [ + { + toolCallId: "write-1", + toolName: "Write coffee_shop_inventory.csv", + args: {}, + result: "/Users/test/Desktop/coffee_shop_inventory.csv (new)", + toolCallIndex: 0, + }, + ], + ["/Users/test/.goose/artifacts"], + ); + + expect(ranking.primaryCandidate?.resolvedPath).toBe( + "/Users/test/Desktop/coffee_shop_inventory.csv", + ); + expect(ranking.primaryCandidate?.allowed).toBe(true); + expect(ranking.primaryCandidate?.blockedReason).toBeNull(); + }); + + it("keeps write arg-key paths outside default roots blocked until a result confirms them", () => { + const candidates = extractToolCallCandidates( + { + toolCallId: "write-1", + toolName: "write_file", + args: { path: "/Users/test/Desktop/coffee_shop_inventory.csv" }, + toolCallIndex: 0, + }, + ["/Users/test/.goose/artifacts"], + ); + + expect(candidates).toHaveLength(1); + expect(candidates[0].resolvedPath).toBe( + "/Users/test/Desktop/coffee_shop_inventory.csv", + ); + expect(candidates[0].allowed).toBe(false); + expect(candidates[0].blockedReason).toContain("outside allowed"); + }); + it("keeps write-origin candidates and drops noisy non-write regex candidates", () => { const ranking = rankMessageToolArtifacts( [ @@ -148,6 +188,47 @@ describe("artifactPathPolicy", () => { ).toBe(false); }); + it("extracts command-style output paths from tool titles", () => { + const ranking = rankMessageToolArtifacts( + [ + { + toolCallId: "cmd-1", + toolName: + "python3 scripts/build_dashboard.py --output output/table.csv", + args: {}, + toolCallIndex: 0, + }, + ], + roots, + ); + + expect(ranking.primaryCandidate?.resolvedPath).toBe( + "/Users/test/project-a/output/table.csv", + ); + expect(ranking.primaryCandidate?.allowed).toBe(true); + }); + + it("extracts command-style output paths from command args", () => { + const ranking = rankMessageToolArtifacts( + [ + { + toolCallId: "cmd-1", + toolName: "functions.exec_command", + args: { + cmd: "python3 scripts/export.py > output/weather-dashboard.html", + }, + toolCallIndex: 0, + }, + ], + roots, + ); + + expect(ranking.primaryCandidate?.resolvedPath).toBe( + "/Users/test/project-a/output/weather-dashboard.html", + ); + expect(ranking.primaryCandidate?.allowed).toBe(true); + }); + it("does not treat html tags as local paths", () => { const candidates = extractToolCallCandidates( { @@ -211,15 +292,8 @@ describe("artifactPathPolicy", () => { expect(ranking.primaryCandidate?.allowed).toBe(true); expect(ranking.primaryCandidate?.resolvedPath).toBe( - "/Users/test/.goose/artifacts/button-interactions.html", + "/Users/test/button-interactions.html", ); - expect( - ranking.secondaryCandidates.some( - (candidate) => - candidate.resolvedPath === "/Users/test/button-interactions.html" && - candidate.allowed === false, - ), - ).toBe(true); }); it("treats unix absolute paths outside the previous whitelist as absolute", () => { diff --git a/ui/goose2/src/features/chat/lib/artifactPathCommandExtraction.ts b/ui/goose2/src/features/chat/lib/artifactPathCommandExtraction.ts new file mode 100644 index 0000000000..f380ebae51 --- /dev/null +++ b/ui/goose2/src/features/chat/lib/artifactPathCommandExtraction.ts @@ -0,0 +1,139 @@ +type ToolNamePathCandidate = { + rawPath: string; + source: "arg_key" | "result_regex"; + confidence: "high" | "low"; + fromResultText: boolean; +}; + +const COMMAND_ARG_KEYS = ["cmd", "command"] as const; + +function stripEnclosingQuotes(token: string): string { + const trimmed = token.trim(); + if (!trimmed) return ""; + if ( + (trimmed.startsWith('"') && trimmed.endsWith('"')) || + (trimmed.startsWith("'") && trimmed.endsWith("'")) || + (trimmed.startsWith("`") && trimmed.endsWith("`")) + ) { + return trimmed.slice(1, -1); + } + return trimmed; +} + +function splitCommandTokens(command: string): string[] { + // Preserve quoted segments so paths with spaces stay intact. + const tokens = command.match(/"[^"]*"|'[^']*'|`[^`]*`|\S+/g); + return tokens ?? []; +} + +function extractCommandPathCandidates( + command: string, + isLikelyLocalPath: (candidate: string) => boolean, + stripTokenPunctuation: (token: string) => string, +): string[] { + const tokens = splitCommandTokens(command); + if (tokens.length === 0) return []; + + const candidates: string[] = []; + const pushIfLocalPath = (rawToken?: string) => { + if (!rawToken) return; + const normalized = stripTokenPunctuation(stripEnclosingQuotes(rawToken)); + if (!isLikelyLocalPath(normalized)) return; + candidates.push(normalized); + }; + + for (let index = 0; index < tokens.length; index += 1) { + const token = tokens[index]; + const normalizedToken = token.toLowerCase(); + + if (/^(?:\d?>|>>|\d?>>)$/.test(token)) { + pushIfLocalPath(tokens[index + 1]); + continue; + } + + if ( + normalizedToken === "-o" || + normalizedToken === "--output" || + normalizedToken === "--out" || + normalizedToken === "--save" || + normalizedToken === "--file" || + normalizedToken === "--path" || + normalizedToken === "--output-file" + ) { + pushIfLocalPath(tokens[index + 1]); + continue; + } + + const equalsMatch = token.match( + /^--(?:output|out|save|file|path|output-file)=(.+)$/i, + ); + if (equalsMatch) { + pushIfLocalPath(equalsMatch[1]); + } + } + + return candidates; +} + +export function extractToolNamePathCandidates( + toolName: string, + isLikelyLocalPath: (candidate: string) => boolean, + stripTokenPunctuation: (token: string) => string, +): ToolNamePathCandidate[] { + const matches: ToolNamePathCandidate[] = []; + const trimmed = toolName.trim(); + if (!trimmed) return matches; + + const match = trimmed.match(/^(write|create|save)\s+(.+)$/i); + if (match) { + const remainder = match[2].trim(); + if (remainder) { + const quoted = remainder.match(/^["'`](.+?)["'`]$/); + const token = quoted ? quoted[1] : remainder.split(/\s+/)[0]; + const cleaned = stripTokenPunctuation(token); + if (isLikelyLocalPath(cleaned)) { + matches.push({ + rawPath: cleaned, + source: "arg_key", + confidence: "high", + fromResultText: false, + }); + } + } + } + + for (const rawPath of extractCommandPathCandidates( + trimmed, + isLikelyLocalPath, + stripTokenPunctuation, + )) { + matches.push({ + rawPath, + source: "result_regex", + confidence: "low", + fromResultText: false, + }); + } + + return matches; +} + +export function collectCommandArgPathCandidates( + args: Record, + isLikelyLocalPath: (candidate: string) => boolean, + stripTokenPunctuation: (token: string) => string, +): string[] { + const values: string[] = []; + for (const key of COMMAND_ARG_KEYS) { + const value = args[key]; + if (typeof value !== "string" || !value.trim()) continue; + values.push( + ...extractCommandPathCandidates( + value, + isLikelyLocalPath, + stripTokenPunctuation, + ), + ); + } + return values; +} diff --git a/ui/goose2/src/features/chat/lib/artifactPathPolicyCore.ts b/ui/goose2/src/features/chat/lib/artifactPathPolicyCore.ts index 7619b1731d..b5f1644074 100644 --- a/ui/goose2/src/features/chat/lib/artifactPathPolicyCore.ts +++ b/ui/goose2/src/features/chat/lib/artifactPathPolicyCore.ts @@ -1,3 +1,8 @@ +import { + collectCommandArgPathCandidates, + extractToolNamePathCandidates, +} from "@/features/chat/lib/artifactPathCommandExtraction"; + export type ArtifactCandidateSource = | "arg_key" | "result_regex" @@ -205,23 +210,6 @@ function extractResultPathCandidates(result: string): string[] { return matches; } -function extractToolNamePathCandidates(toolName: string): string[] { - const trimmed = toolName.trim(); - if (!trimmed) return []; - - const match = trimmed.match(/^(write|create|save)\s+(.+)$/i); - if (!match) return []; - - const remainder = match[2].trim(); - if (!remainder) return []; - - const quoted = remainder.match(/^["'`](.+?)["'`]$/); - const token = quoted ? quoted[1] : remainder.split(/\s+/)[0]; - const cleaned = stripTokenPunctuation(token); - if (!isLikelyLocalPath(cleaned)) return []; - return [cleaned]; -} - function inferHomeDirFromRoots(allowedRoots: string[]): string | null { for (const root of allowedRoots) { const normalized = normalizePath(root); @@ -290,7 +278,12 @@ export function resolvePathCandidate( export function evaluatePathScope( resolvedPath: string, allowedRoots: string[], + options?: { allowOutsideRoots?: boolean }, ): { allowed: boolean; blockedReason: string | null } { + if (options?.allowOutsideRoots) { + return { allowed: true, blockedReason: null }; + } + const normalizedPath = normalizeComparablePath(resolvedPath); const roots = allowedRoots .map((root) => normalizeComparablePath(root)) @@ -306,6 +299,15 @@ export function evaluatePathScope( }; } +function shouldAllowOutsideRootsForToolCandidate( + input: ToolCallArtifactInput, + candidate: { + fromResultText: boolean; + }, +): boolean { + return isWriteOrientedTool(input.toolName) && candidate.fromResultText; +} + function collectArgPathCandidates(args: Record): string[] { const values: string[] = []; for (const key of PATH_ARG_KEYS) { @@ -361,6 +363,7 @@ export function extractToolCallCandidates( rawPath: string; source: ArtifactCandidateSource; confidence: ArtifactCandidateConfidence; + fromResultText: boolean; }> = []; for (const rawPath of collectArgPathCandidates(input.args)) { @@ -368,14 +371,28 @@ export function extractToolCallCandidates( rawPath, source: "arg_key", confidence: "high", + fromResultText: false, }); } - for (const rawPath of extractToolNamePathCandidates(input.toolName)) { + for (const candidate of extractToolNamePathCandidates( + input.toolName, + isLikelyLocalPath, + stripTokenPunctuation, + )) { + rawCandidates.push(candidate); + } + + for (const rawPath of collectCommandArgPathCandidates( + input.args, + isLikelyLocalPath, + stripTokenPunctuation, + )) { rawCandidates.push({ rawPath, - source: "arg_key", - confidence: "high", + source: "result_regex", + confidence: "low", + fromResultText: false, }); } @@ -389,15 +406,21 @@ export function extractToolCallCandidates( rawPath, source: "result_regex", confidence: "low", + fromResultText: true, }); } } return rawCandidates.map((raw, idx) => { const resolvedPath = resolvePathCandidate(raw.rawPath, allowedRoots); + const allowOutsideRoots = shouldAllowOutsideRootsForToolCandidate( + input, + raw, + ); const { allowed, blockedReason } = evaluatePathScope( resolvedPath, allowedRoots, + { allowOutsideRoots }, ); const appearanceIndex = startingAppearanceIndex + idx; return { diff --git a/ui/goose2/src/features/chat/ui/ToolCallCard.tsx b/ui/goose2/src/features/chat/ui/ToolCallCard.tsx index f7e9ed229e..9b43c9ec4a 100644 --- a/ui/goose2/src/features/chat/ui/ToolCallCard.tsx +++ b/ui/goose2/src/features/chat/ui/ToolCallCard.tsx @@ -94,7 +94,7 @@ export function ToolCallCard({ const [openError, setOpenError] = useState(null); const elapsed = useElapsedTime(status); const displayName = formatToolName(name); - const { resolveToolCardDisplay, openResolvedPath } = + const { resolveToolCardDisplay, pathExists, openResolvedPath } = useArtifactPolicyContext(); const hasContent = Object.keys(args).length > 0 || result != null; @@ -118,17 +118,53 @@ export function ToolCallCard({ const displayPath = (candidate: ArtifactPathCandidate): string => candidate.rawPath || candidate.resolvedPath; - const openCandidate = async (candidate: ArtifactPathCandidate) => { - if (!candidate.allowed) { + const openCandidate = async ( + candidate: ArtifactPathCandidate, + options?: { allowFallback?: boolean }, + ) => { + const orderedCandidates = options?.allowFallback + ? [ + candidate, + ...display.secondaryCandidates.filter( + (secondaryCandidate) => secondaryCandidate.id !== candidate.id, + ), + ] + : [candidate]; + + try { + setOpenError(null); + + for (const orderedCandidate of orderedCandidates) { + const exists = await pathExists(orderedCandidate.resolvedPath); + if (orderedCandidate.allowed && exists) { + await openResolvedPath(orderedCandidate.resolvedPath); + return; + } + } + + for (const orderedCandidate of orderedCandidates) { + const exists = await pathExists(orderedCandidate.resolvedPath); + if (exists && !orderedCandidate.allowed) { + setOpenError( + orderedCandidate.blockedReason || + "Path is outside allowed project/artifacts roots.", + ); + return; + } + } + + const firstAllowedCandidate = orderedCandidates.find( + (orderedCandidate) => orderedCandidate.allowed, + ); + if (firstAllowedCandidate) { + setOpenError(`File not found: ${firstAllowedCandidate.resolvedPath}`); + return; + } + setOpenError( candidate.blockedReason || "Path is outside allowed project/artifacts roots.", ); - return; - } - try { - setOpenError(null); - await openResolvedPath(candidate.resolvedPath); } catch (error) { setOpenError(error instanceof Error ? error.message : String(error)); } @@ -174,7 +210,9 @@ export function ToolCallCard({ type="button" onClick={() => { if (!display.primaryCandidate) return; - void openCandidate(display.primaryCandidate); + void openCandidate(display.primaryCandidate, { + allowFallback: true, + }); }} className={cn( "inline-flex max-w-full items-center gap-1.5 rounded-md border px-2.5 py-1 text-xs transition-colors", @@ -227,7 +265,11 @@ export function ToolCallCard({