From ebe3315bdd93c2d9fdd773c9555976ce2863b823 Mon Sep 17 00:00:00 2001 From: Jack Amadeo Date: Mon, 4 May 2026 15:00:22 -0400 Subject: [PATCH] replace artifact heuristics/regexes with protocol messages (#8996) --- crates/goose/src/acp/server.rs | 24 +- ui/goose2/AGENTS.md | 1 - ui/goose2/package.json | 3 +- ui/goose2/scripts/check-file-sizes.mjs | 186 ------- .../chat/hooks/ArtifactPolicyContext.tsx | 264 +++++----- .../__tests__/ArtifactPolicyContext.test.tsx | 200 ++------ .../__tests__/useArtifactLinkHandler.test.tsx | 68 +-- .../chat/hooks/useArtifactLinkHandler.ts | 7 - .../chat/hooks/useChatSessionController.ts | 21 +- .../lib/__tests__/artifactPathPolicy.test.ts | 390 -------------- .../chat/lib/artifactPathCommandExtraction.ts | 139 ----- .../features/chat/lib/artifactPathPolicy.ts | 270 ---------- .../chat/lib/artifactPathPolicyCore.ts | 485 ------------------ .../stores/__tests__/chatSessionStore.test.ts | 5 + .../features/chat/stores/chatSessionStore.ts | 4 + ui/goose2/src/features/chat/ui/ChatView.tsx | 30 +- .../src/features/chat/ui/ToolCallAdapter.tsx | 178 +++---- .../src/features/chat/ui/ToolChainCards.tsx | 1 + .../ui/__tests__/ChatView.mcpApp.test.tsx | 2 +- .../ui/__tests__/ToolCallAdapter.test.tsx | 237 ++------- .../chat/ui/widgets/ArtifactsWidget.tsx | 44 +- .../__tests__/acpNotificationHandler.test.ts | 38 ++ ui/goose2/src/shared/api/acpApi.ts | 3 + .../src/shared/api/acpNotificationHandler.ts | 72 ++- ui/goose2/src/shared/types/chat.ts | 1 + ui/goose2/src/shared/types/messages.ts | 19 + 26 files changed, 484 insertions(+), 2208 deletions(-) delete mode 100644 ui/goose2/scripts/check-file-sizes.mjs delete mode 100644 ui/goose2/src/features/chat/lib/__tests__/artifactPathPolicy.test.ts delete mode 100644 ui/goose2/src/features/chat/lib/artifactPathCommandExtraction.ts delete mode 100644 ui/goose2/src/features/chat/lib/artifactPathPolicy.ts delete mode 100644 ui/goose2/src/features/chat/lib/artifactPathPolicyCore.ts diff --git a/crates/goose/src/acp/server.rs b/crates/goose/src/acp/server.rs index 23b96896..da9aba0b 100644 --- a/crates/goose/src/acp/server.rs +++ b/crates/goose/src/acp/server.rs @@ -367,14 +367,6 @@ fn get_requested_line(arguments: Option<&rmcp::model::JsonObject>) -> Option) -> ToolCallLocation { - let mut loc = ToolCallLocation::new(path); - if let Some(l) = line { - loc = loc.line(l); - } - loc -} - fn is_developer_file_tool(tool_name: &str) -> bool { matches!(tool_name, "read" | "write" | "edit") } @@ -391,7 +383,7 @@ fn extract_locations_from_meta( .filter_map(|entry| { let path = entry.get("path")?.as_str()?; let line = entry.get("line").and_then(|v| v.as_u64()).map(|l| l as u32); - Some(create_tool_location(path, line)) + Some(ToolCallLocation::new(path).line(line)) }) .collect::>(); if locations.is_empty() { @@ -422,12 +414,12 @@ fn extract_tool_locations( if let Some(path_str) = path_str { if matches!(tool_name, "read") { let line = get_requested_line(tool_call.arguments.as_ref()); - locations.push(create_tool_location(path_str, line)); + locations.push(ToolCallLocation::new(path_str).line(line)); return locations; } if matches!(tool_name, "write" | "edit") { - locations.push(create_tool_location(path_str, Some(1))); + locations.push(ToolCallLocation::new(path_str).line(1)); return locations; } @@ -447,19 +439,19 @@ fn extract_tool_locations( let line = extract_view_line_range(text) .map(|range| range.0 as u32) .or(Some(1)); - locations.push(create_tool_location(path_str, line)); + locations.push(ToolCallLocation::new(path_str).line(line)); } Some("str_replace") | Some("insert") => { let line = extract_first_line_number(text) .map(|l| l as u32) .or(Some(1)); - locations.push(create_tool_location(path_str, line)); + locations.push(ToolCallLocation::new(path_str).line(line)); } Some("write") => { - locations.push(create_tool_location(path_str, Some(1))); + locations.push(ToolCallLocation::new(path_str).line(1)); } _ => { - locations.push(create_tool_location(path_str, Some(1))); + locations.push(ToolCallLocation::new(path_str).line(1)); } } break; @@ -468,7 +460,7 @@ fn extract_tool_locations( } if locations.is_empty() { - locations.push(create_tool_location(path_str, Some(1))); + locations.push(ToolCallLocation::new(path_str).line(1)); } } } diff --git a/ui/goose2/AGENTS.md b/ui/goose2/AGENTS.md index 39bf3c9b..4f477167 100644 --- a/ui/goose2/AGENTS.md +++ b/ui/goose2/AGENTS.md @@ -219,7 +219,6 @@ Additional tooling notes: - Unit/component tests use Vitest and Testing Library via `just test` or `pnpm test`. - E2E tests use Playwright via `just test-e2e` and `just test-e2e-all`. -- File size enforcement runs through `pnpm check:file-sizes` and is included in `just check`. - Before handing off a change, run the smallest relevant verification step. Use `just ci` when you need the full local gate. - GitHub Actions also runs desktop-oriented checks, including Playwright coverage, that are broader than the local pre-push hook. diff --git a/ui/goose2/package.json b/ui/goose2/package.json index eaa0e62b..1f87b6c2 100644 --- a/ui/goose2/package.json +++ b/ui/goose2/package.json @@ -8,10 +8,9 @@ "dev": "vite", "build": "tsc && vite build", "typecheck": "tsc --noEmit", - "check:file-sizes": "node ./scripts/check-file-sizes.mjs", "check:i18n": "node ./scripts/check-i18n-strings.mjs", "lint": "biome lint .", - "check": "biome check . && pnpm check:file-sizes && pnpm check:i18n", + "check": "biome check . && pnpm check:i18n", "format": "biome format --write .", "preview": "vite preview", "tauri": "tauri", diff --git a/ui/goose2/scripts/check-file-sizes.mjs b/ui/goose2/scripts/check-file-sizes.mjs deleted file mode 100644 index 91cca88b..00000000 --- a/ui/goose2/scripts/check-file-sizes.mjs +++ /dev/null @@ -1,186 +0,0 @@ -import { readFileSync, readdirSync } from "node:fs"; -import { join, relative } from "node:path"; - -const DEFAULT_LIMIT = 500; - -// Add narrowly scoped exceptions here with justification -const EXCEPTIONS = { - "src/features/sidebar/ui/SidebarProjectsSection.tsx": { - limit: 570, - justification: - "Drag-and-drop handlers for session-to-project moves and project reorder, plus activeProjectId highlight.", - }, - "src/features/chat/ui/ChatView.tsx": { - limit: 570, - justification: - "ACP prewarm guards, project-aware working dir selection, working context sync, chat bootstrapping, context-ring compaction wiring, and gated [perf:chatview] logging via perfLog (dev-only by default).", - }, - "src/features/chat/hooks/useChat.ts": { - limit: 510, - justification: - "Session preparation, provider/model handoff, persona-aware sends, cancellation, and compaction replay still live in one chat lifecycle hook.", - }, - "src/shared/api/acpNotificationHandler.ts": { - limit: 570, - justification: - "ACP replay/live update handling, pending session buffering, model/config propagation, MCP structured tool output, and streaming perf tracking still share one notification entrypoint.", - }, - "src/shared/api/__tests__/acpNotificationHandler.test.ts": { - limit: 540, - justification: - "Notification handler regression coverage spans live streaming, replay ordering, MCP app payload attachment, and structured tool output preservation in one integration-style suite.", - }, - "src/features/chat/ui/__tests__/ContextPanel.test.tsx": { - limit: 550, - justification: - "Workspace widget integration tests cover branch switching, worktree creation, dirty-state dialogs, and picker interactions.", - }, - "src/features/sidebar/ui/Sidebar.tsx": { - limit: 580, - justification: - "Search-as-you-type filtering and draft-aware sidebar highlight logic.", - }, - "src/app/AppShell.tsx": { - limit: 780, - justification: - "Shell still coordinates ACP session loading, replay-buffer cleanup on load failure, project reassignment, home-session restoration, app-level chat routing, restored project-draft reuse, and app-level compaction settings deep links. Includes gated [perf:load]/[perf:newtab] logging via perfLog (dev-only by default).", - }, - "src/features/chat/hooks/useChatSessionController.ts": { - limit: 840, - justification: - "Controller now centralizes home-to-chat pending state transfer, workspace/project preparation, provider/model/persona handoff, Goose cross-provider model selection sequencing with rollback, context-usage readiness resets, queued-target compaction gating, and auto-compaction-aware send orchestration pending a later decomposition pass.", - }, - "src/features/chat/hooks/__tests__/useChatSessionController.test.ts": { - limit: 520, - justification: - "Controller regression coverage now spans model/provider rollback, stale usage resets, compact-before-send, and queued-persona auto-compaction support checks in one hook suite.", - }, - "src/features/chat/stores/chatStore.ts": { - limit: 520, - justification: - "Chat runtime state, queued-message persistence, replay loading flags, and usage snapshot tracking still live together in one Zustand store.", - }, - "src/features/chat/ui/AgentModelPicker.tsx": { - limit: 570, - justification: - "Agent-first picker currently keeps the full trigger, recommended-model view, searchable full-model view, and ACP/goose-specific labeling logic in one component pending later extraction.", - }, - "src/features/chat/stores/__tests__/chatSessionStore.test.ts": { - limit: 540, - justification: - "ACP session overlay regressions currently need one broad integration-style store suite.", - }, - "src/features/chat/stores/chatSessionStore.ts": { - limit: 640, - justification: - "ACP-backed session overlay persistence, draft migration, and sidebar-facing session merge logic live together for now.", - }, - "src/features/chat/ui/ChatInput.tsx": { - limit: 510, - justification: - "Voice dictation send/stop guards, attachment handling, and mention/picker coordination still share one chat composer component.", - }, - "src/features/chat/ui/MessageBubble.tsx": { - limit: 580, - justification: - "Bubble rendering still owns assistant identity, grouped tool output, attachments, inline MCP app tool/result wiring, app-initiated message plumbing, full-width inline layout handling, inline auto-scroll callback plumbing, and the inline actions tray pending a later extraction pass.", - }, - "src/features/chat/ui/__tests__/MessageBubble.test.tsx": { - limit: 520, - justification: - "Message bubble regression coverage still keeps copy state, action tray layout, provider/persona identity, tool chains, and shared rendering behavior in one suite while the MCP app-specific assertions live in a companion test file.", - }, - "src/features/skills/ui/SkillsView.tsx": { - limit: 620, - justification: - "SkillsView currently centralizes list/detail state, project-aware skill hydration, category/source filtering, import/export flows, and detail-page action wiring pending a later decomposition.", - }, - "src/features/chat/ui/__tests__/ChatInput.test.tsx": { - limit: 570, - justification: - "Composer regression coverage spans personas, queueing, attachments, voice-input edge cases, and the compaction popover/settings ingress in one interaction-heavy suite.", - }, - "src-tauri/src/commands/projects.rs": { - limit: 520, - justification: - "Project CRUD plus reorder_projects command for sidebar drag-and-drop ordering.", - }, - "src-tauri/src/commands/system.rs": { - limit: 640, - justification: - "Desktop system commands still centralize file mentions, attachment inspection, platform-aware path dedupe, guarded image loading, and export helpers in one Tauri command surface.", - }, -}; - -// Directories excluded from size checks (imported library code) -const EXCLUDED_DIRS = [ - "src/shared/ui", - "src/components/ai-elements", - "src/hooks", -]; - -const DIRS_TO_CHECK = [ - { dir: "src/app", glob: /\.[jt]sx?$/ }, - { dir: "src/features", glob: /\.[jt]sx?$/ }, - { dir: "src/shared", glob: /\.[jt]sx?$/ }, - { dir: "src/components", glob: /\.[jt]sx?$/ }, - { dir: "src/hooks", glob: /\.[jt]sx?$/ }, - { dir: "src-tauri/src", glob: /\.rs$/ }, -]; - -function countLines(filePath) { - const content = readFileSync(filePath, "utf8"); - return content.split("\n").length; -} - -function isExcluded(filePath) { - const rel = relative(".", filePath); - return EXCLUDED_DIRS.some((dir) => rel.startsWith(dir)); -} - -function walkDir(dir, pattern) { - const results = []; - let entries; - try { - entries = readdirSync(dir, { withFileTypes: true }); - } catch { - return results; - } - for (const entry of entries) { - const fullPath = join(dir, entry.name); - if (entry.isDirectory()) { - results.push(...walkDir(fullPath, pattern)); - } else if (pattern.test(entry.name)) { - results.push(fullPath); - } - } - return results; -} - -const violations = []; - -for (const { dir, glob } of DIRS_TO_CHECK) { - const files = walkDir(dir, glob); - for (const file of files) { - if (isExcluded(file)) continue; - const rel = relative(".", file); - const limit = EXCEPTIONS[rel]?.limit ?? DEFAULT_LIMIT; - const lines = countLines(file); - if (lines > limit) { - violations.push({ file: rel, lines, limit }); - } - } -} - -if (violations.length > 0) { - console.error("Desktop file size check failed:"); - for (const v of violations) { - console.error(` - ${v.file}: ${v.lines} lines (limit ${v.limit})`); - } - console.error( - "\nSplit the file or add a narrowly scoped exception in `scripts/check-file-sizes.mjs`.", - ); - process.exit(1); -} else { - console.log("File size check passed."); -} diff --git a/ui/goose2/src/features/chat/hooks/ArtifactPolicyContext.tsx b/ui/goose2/src/features/chat/hooks/ArtifactPolicyContext.tsx index 4271f377..a3e9b6e8 100644 --- a/ui/goose2/src/features/chat/hooks/ArtifactPolicyContext.tsx +++ b/ui/goose2/src/features/chat/hooks/ArtifactPolicyContext.tsx @@ -7,20 +7,17 @@ import { useRef, type ReactNode, } from "react"; -import type { Message } from "@/shared/types/messages"; +import type { + Message, + ToolCallLocation, + ToolKind, +} from "@/shared/types/messages"; import { pathExists } from "@/shared/api/system"; -import { - buildArtifactsIndexForMessages, - inferHomeDirFromRoots, - isWriteOrientedTool, - resolveMarkdownLocalHref, - type ArtifactPathCandidate, -} from "@/features/chat/lib/artifactPathPolicy"; -export interface ToolCardDisplay { - role: "primary_host" | "none"; - primaryCandidate: ArtifactPathCandidate | null; - secondaryCandidates: ArtifactPathCandidate[]; +export interface ArtifactLinkCandidate { + resolvedPath: string; + rawPath: string; + line?: number | null; } export interface SessionArtifact { @@ -33,28 +30,18 @@ export interface SessionArtifact { lastTouchedAt: number; kind: "file" | "folder" | "path"; toolName: string | null; + toolKind?: ToolKind; + line?: number | null; } interface ArtifactPolicyContextValue { - resolveToolCardDisplay: ( - args: Record, - name: string, - result?: string, - ) => ToolCardDisplay; - resolveMarkdownHref: (href: string) => ArtifactPathCandidate | null; + resolveMarkdownHref: (href: string) => ArtifactLinkCandidate | null; pathExists: (path: string) => Promise; openResolvedPath: (path: string) => Promise; getAllSessionArtifacts: () => SessionArtifact[]; } -const EMPTY_DISPLAY: ToolCardDisplay = { - role: "none", - primaryCandidate: null, - secondaryCandidates: [], -}; - const DEFAULT_CONTEXT_VALUE: ArtifactPolicyContextValue = { - resolveToolCardDisplay: () => EMPTY_DISPLAY, resolveMarkdownHref: () => null, pathExists: async () => false, openResolvedPath: async () => {}, @@ -65,11 +52,12 @@ const ArtifactPolicyContext = createContext( DEFAULT_CONTEXT_VALUE, ); -function shortenPath(fullPath: string, homeDir: string | null): string { - if (homeDir && fullPath.startsWith(homeDir)) { - return `~${fullPath.slice(homeDir.length)}`; - } - return fullPath; +function normalizePath(path: string): string { + return path.replace(/\\/g, "/").trim(); +} + +function normalizeComparablePath(path: string): string { + return normalizePath(path).replace(/\/+$/, "").toLowerCase(); } function parentDir(path: string): string { @@ -83,73 +71,118 @@ function basenameOf(path: string): string { return parts[parts.length - 1] ?? path; } +function hasExtension(path: string): boolean { + const name = basenameOf(path); + const dot = name.lastIndexOf("."); + return dot > 0 && dot < name.length - 1; +} + +function inferPathKind(path: string): SessionArtifact["kind"] { + const normalized = normalizePath(path); + if (normalized.endsWith("/")) return "folder"; + if (hasExtension(normalized)) return "file"; + return "path"; +} + +function isExternalHref(href: string): boolean { + return ( + /^[a-zA-Z][a-zA-Z\d+.-]*:/.test(href) && + !href.toLowerCase().startsWith("file://") + ); +} + +function isAbsolutePath(path: string): boolean { + return path.startsWith("/") || /^[a-zA-Z]:[\\/]/.test(path); +} + +function resolveRelativeToBase(base: string, relativePath: string): string { + const normalizedBase = normalizePath(base).replace(/\/+$/, ""); + const normalizedRelative = normalizePath(relativePath).replace(/^\.\/+/, ""); + if (!normalizedRelative || normalizedRelative === ".") return normalizedBase; + + const stack = normalizedBase.split("/").filter(Boolean); + const hasWindowsDriveRoot = /^[a-zA-Z]:$/.test(stack[0] ?? ""); + for (const segment of normalizedRelative.split("/")) { + if (!segment || segment === ".") continue; + if (segment === "..") { + if (stack.length > 0) stack.pop(); + continue; + } + stack.push(segment); + } + + const resolved = stack.join("/"); + if (hasWindowsDriveRoot) return resolved; + return `/${resolved}`; +} + +function resolvePath(path: string, sessionCwd: string | null): string { + const normalized = normalizePath(path); + if (!normalized) return ""; + + if (normalized.toLowerCase().startsWith("file://")) { + return normalized.slice("file://".length); + } + + if (isAbsolutePath(normalized)) { + return normalized; + } + + return sessionCwd + ? resolveRelativeToBase(sessionCwd, normalized) + : normalized; +} + +function isNonEmptyLocation( + location: ToolCallLocation, +): location is ToolCallLocation & { path: string } { + return typeof location.path === "string" && location.path.trim().length > 0; +} + export function ArtifactPolicyProvider({ messages, - allowedRoots, + sessionCwd, children, }: { messages: Message[]; - allowedRoots: string[]; + sessionCwd: string | null; children: ReactNode; }) { - const normalizedRoots = useMemo( - () => [...new Set(allowedRoots.map((root) => root.trim()).filter(Boolean))], - [allowedRoots], + const normalizedSessionCwd = useMemo( + () => sessionCwd?.trim() || null, + [sessionCwd], ); const lastOpenAtByPathRef = useRef(new Map()); - const artifactsIndex = useMemo( - () => buildArtifactsIndexForMessages(messages, normalizedRoots), - [messages, normalizedRoots], - ); - - const { argsToToolCallId, toolCardDisplayByToolCallId } = useMemo(() => { - const displayByToolCallId = new Map(); - - for (const ranking of artifactsIndex.byMessageId.values()) { - if (!ranking.primaryToolCallId || !ranking.primaryCandidate) continue; - if ( - !ranking.primaryCandidate.toolName || - !isWriteOrientedTool(ranking.primaryCandidate.toolName) - ) { - continue; - } - displayByToolCallId.set(ranking.primaryToolCallId, { - role: "primary_host", - primaryCandidate: ranking.primaryCandidate, - secondaryCandidates: ranking.secondaryCandidates, - }); - } - - return { - argsToToolCallId: artifactsIndex.argsToToolCallId, - toolCardDisplayByToolCallId: displayByToolCallId, - }; - }, [artifactsIndex]); - - const resolveToolCardDisplay = useCallback( - (args: Record, _name: string, _result?: string) => { - const toolCallId = argsToToolCallId.get(args); - if (!toolCallId) return EMPTY_DISPLAY; - return toolCardDisplayByToolCallId.get(toolCallId) ?? EMPTY_DISPLAY; - }, - [argsToToolCallId, toolCardDisplayByToolCallId], - ); - const resolveMarkdownHref = useCallback( - (href: string) => resolveMarkdownLocalHref(href, normalizedRoots), - [normalizedRoots], + (href: string): ArtifactLinkCandidate | null => { + const trimmed = href.trim(); + if (!trimmed || trimmed.startsWith("#")) return null; + if (trimmed.toLowerCase().startsWith("javascript:")) return null; + if (isExternalHref(trimmed)) return null; + + const withoutHash = trimmed.split("#")[0]; + const withoutQuery = withoutHash.split("?")[0]; + if (!withoutQuery) return null; + + return { + rawPath: withoutQuery, + resolvedPath: resolvePath(withoutQuery, normalizedSessionCwd), + }; + }, + [normalizedSessionCwd], ); const resolveOpenTarget = useCallback( async (path: string): Promise => { - if (await pathExists(path)) { - return path; + const resolvedPath = resolvePath(path, normalizedSessionCwd); + if (await pathExists(resolvedPath)) { + return resolvedPath; } return null; }, - [], + [normalizedSessionCwd], ); const checkPathExists = useCallback( @@ -161,7 +194,8 @@ export function ArtifactPolicyProvider({ async (path: string) => { const resolvedTarget = await resolveOpenTarget(path); if (!resolvedTarget) { - throw new Error(`File not found: ${path}`); + const cwdMessage = normalizedSessionCwd ?? ""; + throw new Error(`File not found: ${path} (session cwd: ${cwdMessage})`); } const key = resolvedTarget.trim().toLowerCase(); @@ -173,52 +207,50 @@ export function ArtifactPolicyProvider({ lastOpenAtByPathRef.current.set(key, now); await openPath(resolvedTarget); }, - [resolveOpenTarget], + [resolveOpenTarget, normalizedSessionCwd], ); const getAllSessionArtifacts = useCallback((): SessionArtifact[] => { - const homeDir = - normalizedRoots.length > 0 - ? inferHomeDirFromRoots(normalizedRoots) - : null; - const artifactMap = new Map(); - for (const [messageId, ranking] of artifactsIndex.byMessageId.entries()) { - const message = messages.find((m) => m.id === messageId); - const timestamp = message?.created ?? 0; + for (const message of messages) { + if (message.role !== "assistant") continue; + if (message.metadata?.userVisible === false) continue; + + for (const block of message.content) { + if (block.type !== "toolRequest") continue; + const locations = block.locations?.filter(isNonEmptyLocation) ?? []; + + for (const location of locations) { + const resolvedPath = resolvePath(location.path, normalizedSessionCwd); + const key = normalizeComparablePath(resolvedPath); + if (!key) continue; - for (const candidates of ranking.candidatesByToolCallId.values()) { - for (const candidate of candidates) { - if (!candidate.allowed) continue; - if (!candidate.toolName || !isWriteOrientedTool(candidate.toolName)) { - continue; - } - const key = candidate.resolvedPath.trim().toLowerCase(); const existing = artifactMap.get(key); - if (existing) { existing.versionCount += 1; - if (timestamp > existing.lastTouchedAt) { - existing.lastTouchedAt = timestamp; - existing.toolName = candidate.toolName; + if (message.created > existing.lastTouchedAt) { + existing.lastTouchedAt = message.created; + existing.toolName = block.toolName ?? block.name; + existing.toolKind = block.toolKind; + existing.line = location.line; } - } else { - artifactMap.set(key, { - resolvedPath: candidate.resolvedPath, - displayPath: shortenPath(candidate.resolvedPath, homeDir), - filename: basenameOf(candidate.resolvedPath), - directoryPath: shortenPath( - parentDir(candidate.resolvedPath), - homeDir, - ), - resolvedDirectoryPath: parentDir(candidate.resolvedPath), - versionCount: 1, - lastTouchedAt: timestamp, - kind: candidate.kind, - toolName: candidate.toolName, - }); + continue; } + + artifactMap.set(key, { + resolvedPath, + displayPath: resolvedPath, + filename: basenameOf(resolvedPath), + directoryPath: parentDir(resolvedPath), + resolvedDirectoryPath: parentDir(resolvedPath), + versionCount: 1, + lastTouchedAt: message.created, + kind: inferPathKind(resolvedPath), + toolName: block.toolName ?? block.name, + toolKind: block.toolKind, + line: location.line, + }); } } } @@ -226,11 +258,10 @@ export function ArtifactPolicyProvider({ return Array.from(artifactMap.values()).sort( (a, b) => b.lastTouchedAt - a.lastTouchedAt, ); - }, [messages, normalizedRoots, artifactsIndex]); + }, [messages, normalizedSessionCwd]); const contextValue = useMemo( () => ({ - resolveToolCardDisplay, resolveMarkdownHref, pathExists: checkPathExists, openResolvedPath, @@ -241,7 +272,6 @@ export function ArtifactPolicyProvider({ getAllSessionArtifacts, openResolvedPath, resolveMarkdownHref, - resolveToolCardDisplay, ], ); diff --git a/ui/goose2/src/features/chat/hooks/__tests__/ArtifactPolicyContext.test.tsx b/ui/goose2/src/features/chat/hooks/__tests__/ArtifactPolicyContext.test.tsx index e41a7f51..2fa09ac4 100644 --- a/ui/goose2/src/features/chat/hooks/__tests__/ArtifactPolicyContext.test.tsx +++ b/ui/goose2/src/features/chat/hooks/__tests__/ArtifactPolicyContext.test.tsx @@ -6,96 +6,39 @@ import { useArtifactPolicyContext, } from "../ArtifactPolicyContext"; -import { openPath } from "@tauri-apps/plugin-opener"; - const mockPathExists = vi.fn<(path: string) => Promise>(); vi.mock("@/shared/api/system", () => ({ pathExists: (path: string) => mockPathExists(path), })); -function Probe({ - readArgs, - writeArgs, - clonedWriteArgs, -}: { - readArgs: Record; - writeArgs: Record; - clonedWriteArgs: Record; -}) { - const { resolveToolCardDisplay } = useArtifactPolicyContext(); - const readDisplay = resolveToolCardDisplay(readArgs, "read_file"); - const writeDisplay = resolveToolCardDisplay(writeArgs, "write_file"); - const clonedDisplay = resolveToolCardDisplay(clonedWriteArgs, "write_file"); - - return ( -
- {readDisplay.role} - {writeDisplay.role} - - {writeDisplay.primaryCandidate?.resolvedPath ?? ""} - - - {String(writeDisplay.secondaryCandidates.length)} - - {clonedDisplay.role} -
- ); -} - -function TextFollowupProbe({ - writeArgs, -}: { - writeArgs: Record; -}) { - const { resolveToolCardDisplay, getAllSessionArtifacts } = - useArtifactPolicyContext(); - const display = resolveToolCardDisplay( - writeArgs, - "writing markdown file about alphabet history", - ); +function ArtifactsProbe() { + const { getAllSessionArtifacts } = useArtifactPolicyContext(); const artifacts = getAllSessionArtifacts(); return (
- {display.role} - - {display.primaryCandidate?.resolvedPath ?? ""} - - + {artifacts.map((artifact) => artifact.resolvedPath).join(",")} + {String(artifacts.length)}
); } -function ReadOnlyProbe({ readArgs }: { readArgs: Record }) { - const { resolveToolCardDisplay, getAllSessionArtifacts } = - useArtifactPolicyContext(); - const display = resolveToolCardDisplay(readArgs, "read_file"); - const artifacts = getAllSessionArtifacts(); +function LinkProbe({ href }: { href: string }) { + const { resolveMarkdownHref } = useArtifactPolicyContext(); + const candidate = resolveMarkdownHref(href); return (
- {display.role} - - {artifacts.map((artifact) => artifact.resolvedPath).join(",")} - + {candidate?.resolvedPath ?? ""}
); } describe("ArtifactPolicyContext", () => { - it("computes one primary host per message and resolves tool cards by args identity", () => { - mockPathExists.mockReset(); - vi.mocked(openPath).mockReset(); - const readArgs = { path: "/Users/test/project-a/notes.md" }; - const writeArgs = { - paths: [ - "/Users/test/project-a/output/final_report.md", - "/Users/test/project-a/output/notes.md", - ], - }; + it("uses reported ACP tool locations as session artifacts", () => { const messages: Message[] = [ { id: "assistant-1", @@ -106,28 +49,16 @@ describe("ArtifactPolicyContext", () => { type: "toolRequest", id: "tool-1", name: "read_file", - arguments: readArgs, + arguments: {}, status: "completed", + toolKind: "read", + locations: [{ path: "/Users/test/project-a/notes.md" }], }, { type: "toolResponse", id: "tool-1", name: "read_file", - result: "Read /Users/test/project-a/notes.md", - isError: false, - }, - { - type: "toolRequest", - id: "tool-2", - name: "write_file", - arguments: writeArgs, - status: "completed", - }, - { - type: "toolResponse", - id: "tool-2", - name: "write_file", - result: "Created /Users/test/project-a/output/final_report.md", + result: "Read notes", isError: false, }, ], @@ -137,96 +68,33 @@ describe("ArtifactPolicyContext", () => { render( - + , ); - expect(screen.getByTestId("read-role")).toHaveTextContent("none"); - expect(screen.getByTestId("write-role")).toHaveTextContent("primary_host"); - expect(screen.getByTestId("write-primary")).toHaveTextContent( - "/Users/test/project-a/output/final_report.md", + expect(screen.getByTestId("artifact-count")).toHaveTextContent("1"); + expect(screen.getByTestId("artifact-paths")).toHaveTextContent( + "/Users/test/project-a/notes.md", ); - expect( - Number(screen.getByTestId("write-secondary-count").textContent), - ).toBeGreaterThan(0); - expect(screen.getByTestId("cloned-role")).toHaveTextContent("none"); }); - it("does not treat read-only tool paths as session artifacts", () => { - mockPathExists.mockReset(); - vi.mocked(openPath).mockReset(); - const readArgs = { path: "/Users/test/project-a/notes.md" }; - const messages: Message[] = [ - { - id: "assistant-read-only", - role: "assistant", - created: Date.now(), - content: [ - { - type: "toolRequest", - id: "tool-read", - name: "read_file", - arguments: readArgs, - status: "completed", - }, - { - type: "toolResponse", - id: "tool-read", - name: "read_file", - result: "Read /Users/test/project-a/notes.md", - isError: false, - }, - ], - }, - ]; - - render( - - - , - ); - - expect(screen.getByTestId("read-only-role")).toHaveTextContent("none"); - expect(screen.getByTestId("read-only-artifacts")).toHaveTextContent(""); - }); - - it("uses assistant text after a tool call to populate file actions and the Files tab", () => { - mockPathExists.mockReset(); - vi.mocked(openPath).mockReset(); - const writeArgs = {}; + it("does not filter reported locations outside allowed roots", () => { const messages: Message[] = [ { id: "assistant-1", role: "assistant", created: Date.now(), - metadata: { userVisible: true, agentVisible: true }, content: [ { type: "toolRequest", id: "tool-1", - name: "writing markdown file about alphabet history", - arguments: writeArgs, + name: "write_file", + arguments: {}, status: "completed", - }, - { - type: "toolResponse", - id: "tool-1", - name: "writing markdown file about alphabet history", - result: "completed", - isError: false, - }, - { - type: "text", - text: "The file alpha.md has been created at /Users/test/alpha.md.", + toolKind: "edit", + locations: [{ path: "/tmp/outside.md" }], }, ], }, @@ -235,20 +103,26 @@ describe("ArtifactPolicyContext", () => { render( - + , ); - expect(screen.getByTestId("text-followup-role")).toHaveTextContent( - "primary_host", + expect(screen.getByTestId("artifact-paths")).toHaveTextContent( + "/tmp/outside.md", ); - expect(screen.getByTestId("text-followup-path")).toHaveTextContent( - "/Users/test/alpha.md", + }); + + it("resolves local markdown hrefs relative to the session cwd", () => { + render( + + + , ); - expect(screen.getByTestId("text-followup-artifacts")).toHaveTextContent( - "/Users/test/alpha.md", + + expect(screen.getByTestId("link-path")).toHaveTextContent( + "/Users/test/app/output/report.md", ); }); }); diff --git a/ui/goose2/src/features/chat/hooks/__tests__/useArtifactLinkHandler.test.tsx b/ui/goose2/src/features/chat/hooks/__tests__/useArtifactLinkHandler.test.tsx index 59040189..15ded405 100644 --- a/ui/goose2/src/features/chat/hooks/__tests__/useArtifactLinkHandler.test.tsx +++ b/ui/goose2/src/features/chat/hooks/__tests__/useArtifactLinkHandler.test.tsx @@ -1,52 +1,33 @@ import { render, screen } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { describe, expect, it, vi, beforeEach } from "vitest"; -import type { ArtifactPathCandidate } from "@/features/chat/lib/artifactPathPolicy"; - -// ── mocks ──────────────────────────────────────────────────────────── +import type { ArtifactLinkCandidate } from "@/features/chat/hooks/ArtifactPolicyContext"; const mockResolveMarkdownHref = - vi.fn<(href: string) => ArtifactPathCandidate | null>(); + vi.fn<(href: string) => ArtifactLinkCandidate | null>(); const mockOpenResolvedPath = vi.fn<(path: string) => Promise>(); vi.mock("@/features/chat/hooks/ArtifactPolicyContext", () => ({ useArtifactPolicyContext: () => ({ - resolveToolCardDisplay: () => ({ - role: "none", - primaryCandidate: null, - secondaryCandidates: [], - }), resolveMarkdownHref: mockResolveMarkdownHref, pathExists: async () => false, openResolvedPath: mockOpenResolvedPath, + getAllSessionArtifacts: () => [], }), })); import { useArtifactLinkHandler } from "../useArtifactLinkHandler"; -// ── helpers ────────────────────────────────────────────────────────── - function makeCandidate( - overrides: Partial = {}, -): ArtifactPathCandidate { + overrides: Partial = {}, +): ArtifactLinkCandidate { return { - id: "md-1", rawPath: "/project/report.md", resolvedPath: "/Users/test/project/report.md", - source: "arg_key", - confidence: "high", - kind: "file", - allowed: true, - blockedReason: null, - toolCallId: null, - toolName: null, - toolCallIndex: 0, - appearanceIndex: 0, ...overrides, }; } -/** Renders a container with the click handler and an anchor link inside. */ function Harness({ href, label }: { href: string; label: string }) { const { handleContentClick, pathNotice } = useArtifactLinkHandler(); return ( @@ -59,7 +40,6 @@ function Harness({ href, label }: { href: string; label: string }) { ); } -/** Renders a container with a non-link element. */ function HarnessNoLink() { const { handleContentClick, pathNotice } = useArtifactLinkHandler(); return ( @@ -72,15 +52,13 @@ function HarnessNoLink() { ); } -// ── tests ──────────────────────────────────────────────────────────── - describe("useArtifactLinkHandler", () => { beforeEach(() => { mockResolveMarkdownHref.mockReset(); mockOpenResolvedPath.mockReset(); }); - it("calls resolveMarkdownHref and openResolvedPath for allowed local links", async () => { + it("opens resolved local links", async () => { const user = userEvent.setup(); const candidate = makeCandidate(); mockResolveMarkdownHref.mockReturnValue(candidate); @@ -93,24 +71,24 @@ describe("useArtifactLinkHandler", () => { expect(mockOpenResolvedPath).toHaveBeenCalledWith(candidate.resolvedPath); }); - it("shows blocked notice for disallowed paths", async () => { + it("shows opener errors", async () => { const user = userEvent.setup(); - const blocked = makeCandidate({ - allowed: false, - blockedReason: "Path is outside allowed roots.", - }); - mockResolveMarkdownHref.mockReturnValue(blocked); + mockResolveMarkdownHref.mockReturnValue( + makeCandidate({ resolvedPath: "/secret/data.md" }), + ); + mockOpenResolvedPath.mockRejectedValue( + new Error("File not found: /secret/data.md"), + ); render(); await user.click(screen.getByText("Secret")); - expect(mockOpenResolvedPath).not.toHaveBeenCalled(); expect(screen.getByTestId("notice")).toHaveTextContent( - "Path is outside allowed roots.", + "File not found: /secret/data.md", ); }); - it("does not intercept external URLs (defers to MarkdownLink's LinkSafetyModal)", async () => { + it("does not intercept external URLs", async () => { const user = userEvent.setup(); render(); @@ -129,20 +107,4 @@ describe("useArtifactLinkHandler", () => { expect(mockResolveMarkdownHref).not.toHaveBeenCalled(); expect(mockOpenResolvedPath).not.toHaveBeenCalled(); }); - - it("shows default blocked reason when blockedReason is null", async () => { - const user = userEvent.setup(); - const blocked = makeCandidate({ - allowed: false, - blockedReason: null, - }); - mockResolveMarkdownHref.mockReturnValue(blocked); - - render(); - await user.click(screen.getByText("Blocked")); - - expect(screen.getByTestId("notice")).toHaveTextContent( - "Path is outside allowed roots.", - ); - }); }); diff --git a/ui/goose2/src/features/chat/hooks/useArtifactLinkHandler.ts b/ui/goose2/src/features/chat/hooks/useArtifactLinkHandler.ts index 543b535d..b40369b9 100644 --- a/ui/goose2/src/features/chat/hooks/useArtifactLinkHandler.ts +++ b/ui/goose2/src/features/chat/hooks/useArtifactLinkHandler.ts @@ -28,13 +28,6 @@ export function useArtifactLinkHandler() { const resolved = resolveMarkdownHref(href); if (!resolved) return; - if (!resolved.allowed) { - setPathNotice( - resolved.blockedReason || "Path is outside allowed roots.", - ); - return; - } - setPathNotice(null); void openResolvedPath(resolved.resolvedPath).catch((err) => { setPathNotice(err instanceof Error ? err.message : String(err)); diff --git a/ui/goose2/src/features/chat/hooks/useChatSessionController.ts b/ui/goose2/src/features/chat/hooks/useChatSessionController.ts index efd2ff10..4488e9aa 100644 --- a/ui/goose2/src/features/chat/hooks/useChatSessionController.ts +++ b/ui/goose2/src/features/chat/hooks/useChatSessionController.ts @@ -14,7 +14,6 @@ import { resolveAgentProviderCatalogIdStrict } from "@/features/providers/provid import { buildProjectSystemPrompt, composeSystemPrompt, - getProjectArtifactRoots, resolveProjectDefaultArtifactRoot, } from "@/features/projects/lib/chatProjectContext"; import { setStoredModelPreference } from "../lib/modelPreferences"; @@ -107,10 +106,10 @@ export function useChatSessionController({ const selectedPersona = personas.find( (persona) => persona.id === selectedPersonaId, ); - const projectArtifactRoots = useMemo( - () => getProjectArtifactRoots(project), - [project], - ); + const sessionCwd = + activeWorkspace?.path ?? + session?.workingDir ?? + resolveProjectDefaultArtifactRoot(project); const projectDefaultArtifactRoot = useMemo( () => resolveProjectDefaultArtifactRoot(project), [project], @@ -118,13 +117,9 @@ export function useChatSessionController({ const projectMetadataPending = Boolean( effectiveProjectId && !projectDefaultArtifactRoot && projectsLoading, ); - const allowedArtifactRoots = useMemo( - () => [ - ...new Set( - projectArtifactRoots.map((path) => path.trim()).filter(Boolean), - ), - ], - [projectArtifactRoots], + const sessionArtifactCwd = useMemo( + () => sessionCwd?.trim() || null, + [sessionCwd], ); const availableProjects = useMemo( () => @@ -792,7 +787,7 @@ export function useChatSessionController({ return { session, project, - allowedArtifactRoots, + sessionArtifactCwd, messages, chatState, tokenState: resolvedTokenState, diff --git a/ui/goose2/src/features/chat/lib/__tests__/artifactPathPolicy.test.ts b/ui/goose2/src/features/chat/lib/__tests__/artifactPathPolicy.test.ts deleted file mode 100644 index 13079223..00000000 --- a/ui/goose2/src/features/chat/lib/__tests__/artifactPathPolicy.test.ts +++ /dev/null @@ -1,390 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { - buildArtifactsIndexForMessages, - dedupeAndRankCandidates, - evaluatePathScope, - extractToolCallCandidates, - rankMessageToolArtifacts, - resolvePathCandidate, -} from "../artifactPathPolicy"; - -const roots = ["/Users/test/project-a", "/Users/test/project-b", "/Users/test"]; - -describe("artifactPathPolicy", () => { - it("prefers the latest write-oriented tool call over earlier tool calls", () => { - const ranking = rankMessageToolArtifacts( - [ - { - toolCallId: "read-1", - toolName: "read_file", - args: { path: "/Users/test/project-a/notes.md" }, - toolCallIndex: 0, - }, - { - toolCallId: "write-1", - toolName: "write_file", - args: { path: "/Users/test/project-a/result.md" }, - toolCallIndex: 1, - }, - ], - roots, - ); - - expect(ranking.primaryToolCallId).toBe("write-1"); - expect(ranking.primaryCandidate?.resolvedPath).toBe( - "/Users/test/project-a/result.md", - ); - }); - - it("boosts filename and output-directory signals", () => { - const ranking = rankMessageToolArtifacts( - [ - { - toolCallId: "write-1", - toolName: "write_file", - args: { - paths: [ - "/Users/test/project-a/notes.md", - "/Users/test/project-a/output/final_report.md", - ], - }, - toolCallIndex: 0, - }, - ], - roots, - ); - - expect(ranking.primaryCandidate?.resolvedPath).toBe( - "/Users/test/project-a/output/final_report.md", - ); - }); - - it("uses appearance order as tie-breaker when signals are equal", () => { - const ranking = rankMessageToolArtifacts( - [ - { - toolCallId: "write-1", - toolName: "write_file", - args: { - paths: [ - "/Users/test/project-a/a.txt", - "/Users/test/project-a/b.txt", - ], - }, - toolCallIndex: 0, - }, - ], - roots, - ); - - expect(ranking.primaryCandidate?.resolvedPath).toBe( - "/Users/test/project-a/b.txt", - ); - }); - - it("dedupes equivalent resolved paths", () => { - const candidates = extractToolCallCandidates( - { - toolCallId: "write-1", - toolName: "write_file", - args: { path: "/Users/test/project-a/report.md" }, - result: "Wrote /Users/test/project-a/report.md successfully", - toolCallIndex: 0, - }, - roots, - ); - - const deduped = dedupeAndRankCandidates(candidates); - expect(deduped).toHaveLength(1); - expect(deduped[0].resolvedPath).toBe("/Users/test/project-a/report.md"); - }); - - it("allows paths inside any configured root and blocks others", () => { - const resolvedAllowed = resolvePathCandidate( - "/Users/test/project-b/output/summary.md", - roots, - ); - const allowed = evaluatePathScope(resolvedAllowed, roots); - expect(allowed.allowed).toBe(true); - expect(allowed.blockedReason).toBeNull(); - - const blocked = evaluatePathScope("/Users/other/outside/file.md", roots); - expect(blocked.allowed).toBe(false); - 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"], - ); - - 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/project-a"], - ); - - 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( - [ - { - toolCallId: "ls-1", - toolName: "ls /Users/test", - args: {}, - result: "small_business_issues_report.md\nrandom.json", - toolCallIndex: 0, - }, - { - toolCallId: "write-1", - toolName: "Write weather-dashboard.html", - args: {}, - result: "Created weather-dashboard.html", - toolCallIndex: 1, - }, - ], - roots, - ); - - expect(ranking.primaryToolCallId).toBe("write-1"); - expect(ranking.primaryCandidate?.resolvedPath).toContain( - "weather-dashboard.html", - ); - expect( - ranking.secondaryCandidates.some((candidate) => - candidate.resolvedPath.includes("small_business_issues_report.md"), - ), - ).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( - { - toolCallId: "write-1", - toolName: "write_file", - args: {}, - result: - "\n\n