diff --git a/.changeset/fn-8466-skill-path-read-boundary.md b/.changeset/fn-8466-skill-path-read-boundary.md new file mode 100644 index 0000000000..1b205d2d92 --- /dev/null +++ b/.changeset/fn-8466-skill-path-read-boundary.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Allow session Read tool to open host-advertised plugin skill body paths under worktree boundary. +category: fix +dev: Worktree-bound pi sessions treat one normalized AgentOptions.additionalSkillPaths list as a read-only boundary exception for read/glob/grep and as DefaultResourceLoader skill roots (GitHub #2384 / FN-8466); skill-root write/edit remain blocked. diff --git a/docs/agents.md b/docs/agents.md index 60b6ed0b1c..81a2cc3a8a 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -932,6 +932,10 @@ Dashboard Chat, Chat Room responders, and task-detail Planner Chat run at the in Task-detail Planner Chat is included because it is a `task-planner:` ChatManager session. This does not change the readonly planning/mission interview lanes or WhatsApp plugin chat. Chat verification remains limited to its existing allowlisted profiles rather than accepting arbitrary shell commands. +### Worktree session file boundary + +Pi sessions started in an isolated task worktree reject filesystem paths outside that worktree. The established project-memory and task-attachment exceptions remain unchanged. Separately, when Fusion advertises skill bodies through `AgentOptions.additionalSkillPaths` (including enabled plugin skill roots), it allows only `read`, `glob`, and `grep` to access those exact normalized roots. `write`, `edit`, and Bash working directories remain worktree-bound for skill roots; this is not a general `~/.fusion/plugins` exception. + ```bash fn message inbox fn message outbox diff --git a/packages/engine/src/__tests__/pi-create-fn-agent.test.ts b/packages/engine/src/__tests__/pi-create-fn-agent.test.ts index 5049568a46..415f9d3e20 100644 --- a/packages/engine/src/__tests__/pi-create-fn-agent.test.ts +++ b/packages/engine/src/__tests__/pi-create-fn-agent.test.ts @@ -496,6 +496,114 @@ describe("worktree path boundary helpers", () => { expect(result).toEqual({ ok: true, content: [{ type: "text", text: "attachment content" }] }); }); + it("allows only read/glob/grep under host-advertised skill roots", async () => { + const makeTool = (name: string) => ({ + name, + label: name, + description: `${name} a skill file`, + parameters: {}, + execute: vi.fn().mockResolvedValue({ ok: true, content: [] }), + }); + const skillRoot = "/Users/agent/.fusion/plugins/de-sloppify/skills"; + const skillPath = `${skillRoot}/de-sloppify/references/style.md`; + const [readTool, globTool, grepTool, writeTool, editTool, bashTool] = [ + makeTool("read"), + makeTool("glob"), + makeTool("grep"), + makeTool("write"), + makeTool("edit"), + makeTool("bash"), + ]; + const { wrapToolsWithBoundary } = await import("../pi.js"); + const wrapped = wrapToolsWithBoundary( + [readTool, globTool, grepTool, writeTool, editTool, bashTool] as any, + "/project/.worktrees/fn-8466", + "/project", + [skillRoot], + ); + + for (const tool of wrapped.slice(0, 3) as any[]) { + await tool.execute(`call-${tool.name}`, { path: skillPath }); + } + expect(readTool.execute).toHaveBeenCalledOnce(); + expect(globTool.execute).toHaveBeenCalledOnce(); + expect(grepTool.execute).toHaveBeenCalledOnce(); + + for (const tool of wrapped.slice(3, 5) as any[]) { + const result = await tool.execute(`call-${tool.name}`, { path: skillPath }); + expect(result).toMatchObject({ ok: false, error: expect.stringContaining("outside the worktree boundary") }); + } + expect(writeTool.execute).not.toHaveBeenCalled(); + expect(editTool.execute).not.toHaveBeenCalled(); + + const bashResult = await (wrapped[5] as any).execute("call-bash", { command: "pwd", cwd: skillRoot }); + expect(bashResult).toMatchObject({ ok: false, error: expect.stringContaining("outside the worktree boundary") }); + expect(bashTool.execute).not.toHaveBeenCalled(); + + const outsideResult = await (wrapped[0] as any).execute("call-outside", { path: "/other/project/secret" }); + expect(outsideResult).toMatchObject({ ok: false, error: expect.stringContaining("outside the worktree boundary") }); + expect(readTool.execute).toHaveBeenCalledOnce(); + }); + + it("rejects host skill paths when no read-only extra roots are provided", async () => { + const mockReadTool = { + name: "read", + label: "Read", + description: "Read a file", + parameters: {}, + execute: vi.fn().mockResolvedValue({ ok: true, content: [] }), + }; + const { wrapToolsWithBoundary } = await import("../pi.js"); + const wrapped = wrapToolsWithBoundary([mockReadTool as any], "/project/.worktrees/fn-8466", "/project"); + + const result = await (wrapped[0] as any).execute("call-1", { + path: "/Users/agent/.fusion/plugins/de-sloppify/skills/de-sloppify/SKILL.md", + }); + expect(result).toMatchObject({ ok: false, error: expect.stringContaining("outside the worktree boundary") }); + expect(mockReadTool.execute).not.toHaveBeenCalled(); + }); + + it("canonicalizes macOS-style skill roots before allowing reads", async () => { + const skillRoot = "/var/folders/fn-8466/plugin/skills"; + const canonicalSkillPath = "/private/var/folders/fn-8466/plugin/skills/de-sloppify/SKILL.md"; + const mockReadTool = { + name: "read", + label: "Read", + description: "Read a file", + parameters: {}, + execute: vi.fn().mockResolvedValue({ ok: true, content: [] }), + }; + realpathSyncNativeMock.mockImplementation((path: PathLike) => { + const text = String(path); + return text.startsWith("/var/") ? `/private${text}` : text; + }); + const { wrapToolsWithBoundary } = await import("../pi.js"); + const wrapped = wrapToolsWithBoundary( + [mockReadTool as any], + "/project/.worktrees/fn-8466", + "/project", + [skillRoot], + ); + + const result = await (wrapped[0] as any).execute("call-1", { path: canonicalSkillPath }); + expect(result).toEqual({ ok: true, content: [] }); + expect(mockReadTool.execute).toHaveBeenCalledOnce(); + }); + + it("normalizes one stable skill-root list for resource loading and boundary wiring", async () => { + const { normalizeAdditionalSkillPaths } = await import("../pi.js"); + expect(normalizeAdditionalSkillPaths(["/skills/plugin", "", "/skills/plugin/", "/skills/ce"])).toEqual([ + "/skills/plugin", + "/skills/ce", + ]); + + const fs = await vi.importActual("node:fs"); + const source = fs.readFileSync(`${process.cwd()}/src/pi.ts`, "utf8"); + expect(source).toContain("const normalizedAdditionalSkillPaths = normalizeAdditionalSkillPaths(options.additionalSkillPaths);"); + expect(source).toContain("additionalSkillPaths: normalizedAdditionalSkillPaths"); + expect(source).toMatch(/wrapToolsWithBoundary\(\s*toolsWithActionGate,\s*boundaryContext\.worktreePath,\s*boundaryContext\.worktreeProjectRoot,\s*normalizedAdditionalSkillPaths,/); + }); + it("does not wrap tools when cwd is not a worktree", async () => { const mockTool = { name: "read", diff --git a/packages/engine/src/pi.ts b/packages/engine/src/pi.ts index 17165ac237..efbf38feef 100644 --- a/packages/engine/src/pi.ts +++ b/packages/engine/src/pi.ts @@ -1616,6 +1616,29 @@ function normalizeExistingPathForGitComparison(path: string): string { } } +/** + * FNXC:SkillReadBoundary 2026-07-21-12:00: + * GitHub #2384 / FN-8466 requires the exact host-advertised additional skill + * roots to drive both pi discovery and the worktree Read boundary. Resolve and + * deduplicate once so the manifest cannot advertise a skill body the boundary + * rejects through a divergent raw-path list. + */ +export function normalizeAdditionalSkillPaths(paths?: readonly string[]): string[] { + const seen = new Set(); + const normalizedPaths: string[] = []; + + for (const path of paths ?? []) { + if (!path?.trim()) continue; + const normalizedPath = normalizeExistingPathForGitComparison(resolve(path)); + if (!seen.has(normalizedPath)) { + seen.add(normalizedPath); + normalizedPaths.push(normalizedPath); + } + } + + return normalizedPaths; +} + function isSameOrInsidePath(parentPath: string, childPath: string): boolean { const rel = relative(parentPath, childPath); return rel === "" || (!rel.startsWith("..") && !isAbsolute(rel)); @@ -1653,12 +1676,14 @@ async function assertValidWorktreeSession(cwd: string, projectRoot: string): Pro * - Task attachments under .fusion/tasks/N/attachments/ are allowed (for reading context files) * - Sibling task specs (.fusion/tasks/N/PROMPT.md and task.json) are allowed for * read-only tools (read/glob/grep) so agents can consult dependency specs. + * - Host-advertised additional skill roots are allowed for read-only tools only. * - All other paths outside the worktree are rejected * * @param worktreePath - Absolute path to the worktree directory * @param projectRoot - Absolute path to the project root (derived from worktree) * @param requestedPath - The path being accessed * @param toolName - Tool making the request (controls read-only exceptions) + * @param readOnlyExtraRoots - Host-advertised roots readable by read-only tools * @returns true if allowed, false if rejected */ function isWorktreeAllowedPath( @@ -1666,6 +1691,7 @@ function isWorktreeAllowedPath( projectRoot: string, requestedPath: string, toolName?: string, + readOnlyExtraRoots: readonly string[] = [], ): boolean { // Normalize paths const worktreeResolved = resolve(worktreePath); @@ -1707,12 +1733,25 @@ function isWorktreeAllowedPath( // into the worktree. `glob`/`grep` are narrow enough to allow as well so // the agent can discover them; writes and bash remain restricted. const readOnlyTools = new Set(["read", "glob", "grep"]); - if ( - toolName && - readOnlyTools.has(toolName) && - projectRelativePaths.some((relPath) => /^\.fusion\/tasks\/[^/]+\/(PROMPT\.md|task\.json)$/.test(relPath)) - ) { - return true; + if (toolName && readOnlyTools.has(toolName)) { + if (projectRelativePaths.some((relPath) => /^\.fusion\/tasks\/[^/]+\/(PROMPT\.md|task\.json)$/.test(relPath))) { + return true; + } + + /* + FNXC:SkillReadBoundary 2026-07-21-12:00: + GitHub #2384 / FN-8466 lets agents Read only the specific additional skill + roots advertised by this session. Do not extend this exception to write, + edit, or bash: plugin skill bodies remain host-owned read-only context. + */ + if (readOnlyExtraRoots.some((root) => { + const rootResolved = resolve(root); + const rootCanonical = normalizeExistingPathForGitComparison(rootResolved); + return isSameOrInsidePath(rootResolved, requestedResolved) + || isSameOrInsidePath(rootCanonical, requestedCanonical); + })) { + return true; + } } // All other paths outside the worktree are rejected @@ -1726,6 +1765,7 @@ function isWorktreeAllowedPath( * @param tools - Array of tool definitions to wrap * @param worktreePath - Absolute path to the worktree directory (if applicable) * @param projectRoot - Absolute path to the project root (if applicable) + * @param readOnlyExtraRoots - Host-advertised roots readable by read/glob/grep only * @returns Wrapped tools with boundary validation */ /** @@ -1778,11 +1818,14 @@ export function wrapToolsWithBoundary( tools: ToolDefinition[], worktreePath: string | null, projectRoot: string | null, + readOnlyExtraRoots: readonly string[] = [], ): ToolDefinition[] { if (!worktreePath || !projectRoot) { return tools; // Not a worktree session, no wrapping needed } + const normalizedReadOnlyExtraRoots = normalizeAdditionalSkillPaths(readOnlyExtraRoots); + return tools.map((tool) => { // Only wrap tools that access the filesystem const fileToolNames = new Set(["read", "write", "edit", "glob", "grep", "bash"]); @@ -1804,13 +1847,13 @@ export function wrapToolsWithBoundary( // Check path argument for file operations const pathArg = params.path as string | undefined; - if (pathArg && !isWorktreeAllowedPath(worktreePath, projectRoot, pathArg, tool.name)) { + if (pathArg && !isWorktreeAllowedPath(worktreePath, projectRoot, pathArg, tool.name, normalizedReadOnlyExtraRoots)) { const relToProject = relative(projectRoot, pathArg); return boundaryRejection( `Path "${relToProject}" is outside the worktree boundary. ` + `Coding agents can only modify files inside the current worktree. ` + - `Exceptions (read-only): .fusion/memory/, .fusion/tasks/*/attachments/, ` + - `and .fusion/tasks/*/{PROMPT.md,task.json} for dependency context.`, + `Existing exceptions include .fusion/memory/ and task attachments; ` + + `read-only tools may also access sibling task specs and host-advertised skill roots.`, ); } @@ -2314,6 +2357,15 @@ export async function createFnAgent(options: AgentOptions): Promise }); } + /* + FNXC:SkillReadBoundary 2026-07-21-12:00: + GitHub #2384 / FN-8466 requires one canonical additional-skill list for both + DefaultResourceLoader's manifest and the worktree boundary. A skill location + the loader advertises must be readable, while the boundary grants it only to + read/glob/grep rather than write/edit/bash. + */ + const normalizedAdditionalSkillPaths = normalizeAdditionalSkillPaths(options.additionalSkillPaths); + // `tools: "readonly"` MUST mean a hermetically sealed read-only session with // respect to host extension injection. Host extensions (`@runfusion/fusion`) // can register write tools like `fn_task_create`, so they are deliberately @@ -2348,8 +2400,8 @@ export async function createFnAgent(options: AgentOptions): Promise ? [options.systemPromptLayers.dynamic] : [], ...(effectiveExtensionPaths.length > 0 ? { additionalExtensionPaths: [...effectiveExtensionPaths] } : {}), - ...(options.additionalSkillPaths && options.additionalSkillPaths.length > 0 - ? { additionalSkillPaths: [...options.additionalSkillPaths] } + ...(normalizedAdditionalSkillPaths.length > 0 + ? { additionalSkillPaths: normalizedAdditionalSkillPaths } : {}), ...(skillsOverrideFn ? { skillsOverride: skillsOverrideFn } : {}), }); @@ -2454,6 +2506,7 @@ export async function createFnAgent(options: AgentOptions): Promise toolsWithActionGate, boundaryContext.worktreePath, boundaryContext.worktreeProjectRoot, + normalizedAdditionalSkillPaths, ); // Sort tools alphabetically by name for deterministic ordering. // Prompt caching requires the tool list to be byte-identical across