Skip to content
Open
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
79 changes: 78 additions & 1 deletion apps/mcp/src/__tests__/tools/repo-tools.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,21 @@
import { describe, expect, it, vi } from "vitest";
import { beforeEach, describe, expect, it, vi } from "vitest";

// clone_repository calls node:fs/promises' rm(targetDir, {recursive:true,
// force:true}) directly (no injection seam like gitExec has). Mocked here
// so a test — especially the protected-directory regression tests below,
// which deliberately exercise targetDir values like "/etc" and "/home"
// that really exist on whatever machine runs this suite — can never touch
// the real filesystem, regardless of whether the guard being tested is
// actually correct.
const rmMock = vi.fn().mockResolvedValue(undefined);
vi.mock("node:fs/promises", () => ({
rm: (...args: unknown[]) => rmMock(...args),
}));

beforeEach(() => {
rmMock.mockClear();
});

import {
getRepoTools,
handleRepoTool,
Expand Down Expand Up @@ -271,6 +288,66 @@ describe("handleRepoTool – clone_repository", () => {
expect(result.content[0].text).toContain("git clone failed");
});

// Regression tests: clone_repository force-deletes targetDir before
// cloning into it (`rm(targetDir, { recursive: true, force: true })`),
// and targetDir was previously taken as-is from agent-supplied input
// with no validation — a task/prompt that got the agent to pass a
// catastrophic targetDir (e.g. "/", "/home") could wipe it out with no
// confirmation. None of these should ever reach gitExec or the delete.
it.each([
["/", "/"],
["/home", "/home"],
["/home/goose", "/home/goose"],
["/home/goose/", "/home/goose"],
["/etc", "/etc"],
["/etc/", "/etc"],
["/tmp", "/tmp"],
["/../../etc", "/etc"],
["/home/goose/../../etc", "/etc"],
["/home/goose/../", "/home"],
])("refuses to clone into the protected system directory %s", async (targetDir, _resolved) => {
const client = makeClient({
getRepositoryCloneInfo: vi.fn().mockResolvedValue(cloneInfo),
});
const gitExec = vi.fn();
const result = await handleRepoTool(
"clone_repository",
{ projectId: "p1", pluginId: "com.paca.github", repoId: "r1", targetDir },
client,
[],
gitExec,
);
expect(rmMock).not.toHaveBeenCalled();
expect(gitExec).not.toHaveBeenCalled();
expect(result.content[0].text).toContain("Failed to clone repository");
expect(result.content[0].text).toMatch(/protected system directory/i);
});

it("still allows cloning into a subdirectory of a protected top-level directory", async () => {
const client = makeClient({
getRepositoryCloneInfo: vi.fn().mockResolvedValue(cloneInfo),
});
const gitExec = vi
.fn()
.mockResolvedValueOnce({ stdout: "", stderr: "" })
.mockResolvedValueOnce({ stdout: "main\n", stderr: "" });

const result = await handleRepoTool(
"clone_repository",
{
projectId: "p1",
pluginId: "com.paca.github",
repoId: "r1",
targetDir: "/home/goose/repo",
},
client,
[],
gitExec,
);
expect(gitExec).toHaveBeenCalled();
expect(result.content[0].text).toContain("cloned successfully");
});

it("throws a ZodError when repoId is missing", async () => {
await expect(
handleRepoTool(
Expand Down
58 changes: 58 additions & 0 deletions apps/mcp/src/tools/repo-tools.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { execFile as execFileCb } from "node:child_process";
import { rm } from "node:fs/promises";
import { resolve as resolvePath } from "node:path";
import { promisify } from "node:util";
import type { Tool } from "@modelcontextprotocol/sdk/types.js";
import { z } from "zod";
Expand Down Expand Up @@ -62,6 +63,61 @@ function authenticatedCloneURL(cloneURL: string, token: string): string {
return `https://x-access-token:${encodeURIComponent(token)}@${host}${parsed.pathname}`;
}

/**
* The small set of top-level directories a container image actually needs
* intact to keep running — `clone_repository` must never recursively
* delete any of these, however it got there (an agent-chosen targetDir, a
* confused task instruction, or plain prompt injection from cloned repo
* content). Deliberately not a fixed-prefix jail: targetDir is documented
* as any absolute path (see CloneRepositorySchema), so a specific
* subdirectory of any of these — e.g. /home/goose/repo, the actual
* default — is still allowed; only the bare top-level directory itself is
* refused. Ported forward as a new safety check, not a regression fix:
* the Python this replaces (repo_tools.py) ran the equivalent `rm -rf`
* with no such guard either.
*
* /home/goose is listed explicitly alongside /home itself: it's the
* container user's actual home directory (DEFAULT_REPO_DIR's own parent),
* so it's at least as likely a target for an accidental or confused
* targetDir as any of the OS-level directories below it, even though the
* OS itself would keep running without it.
*/
const FORBIDDEN_DELETE_TARGETS = new Set([
"/",
"/bin",
"/boot",
"/dev",
"/etc",
"/home",
"/home/goose",
"/lib",
"/lib64",
"/opt",
"/proc",
"/root",
"/run",
"/sbin",
"/srv",
"/sys",
"/tmp",
"/usr",
"/var",
]);

/**
* Throws if targetDir resolves (after normalizing `..`/`.` segments) to one
* of FORBIDDEN_DELETE_TARGETS — called before every recursive delete this
* file performs on an agent-supplied path.
*/
function assertSafeDeleteTarget(targetDir: string): void {
const resolved = resolvePath("/", targetDir);
if (FORBIDDEN_DELETE_TARGETS.has(resolved)) {
throw new Error(
`refusing to delete ${resolved} — it's a protected system directory, not a valid clone target`,
);
}
}

function errorMessage(error: unknown): string {
if (error && typeof error === "object") {
const e = error as {
Expand Down Expand Up @@ -312,6 +368,8 @@ export async function handleRepoTool(
const targetDir: string = args.targetDir || DEFAULT_REPO_DIR;

try {
assertSafeDeleteTarget(targetDir);

const info = await client.getRepositoryCloneInfo(
pluginId,
projectId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,67 @@ describe("useConversationEventWindow", () => {
expect(key[0]).not.toBe("global-chat");
}
});
it("prunes the tail cache once a real fetch catches up to what it buffered", async () => {
// Regression test: applyRealtimeAgentEvent only ever appends to the
// tail cache's `events` array, and its own doc comment claims pruning
// "happens there [in useConversationEventWindow], not here" — this
// confirms that promise is actually kept, otherwise the tail grows
// unboundedly for the life of the tab.
const stream = fakeStream(200);
const { result, signal, queryClient } = open();
await waitFor(() => expect(result.current.events).toHaveLength(200));

await signal(stream.grow(3));
await waitFor(() => expect(lastIndex(result.current.events)).toBe(202));
expect(
queryClient.getQueryData<ConversationEventsTail>(
conversationEventsTailKey(CONVERSATION_ID),
)?.events,
).toHaveLength(3);

// A real fetch reconciling the window (mirrors useProjectRealtime
// invalidating conversationEventWindowKey on a status transition) now
// covers the same 3 events the tail was buffering.
await act(() =>
queryClient.invalidateQueries({
queryKey: conversationEventWindowKey(CONVERSATION_ID),
}),
);
await waitFor(() =>
expect(
queryClient.getQueryData<ConversationEventsTail>(
conversationEventsTailKey(CONVERSATION_ID),
)?.events,
).toHaveLength(0),
);

// The window still shows the newest events — now sourced from the
// refetched page (capped at pageSize=200, so it holds indices 3..202)
// instead of the tail buffer, which is exactly why the buffer's own
// 3 events are now safe to have pruned away.
expect(result.current.events).toHaveLength(200);
expect(result.current.events[0].event_index).toBe(3);
expect(lastIndex(result.current.events)).toBe(202);
});

it("does not prune tail events a real fetch hasn't reached yet", async () => {
// The inverse of the above: a live event past what's currently paged
// in must survive pruning — only events already redundant with
// pagedEvents get dropped.
const stream = fakeStream(200);
const { result, signal, queryClient } = open();
await waitFor(() => expect(result.current.events).toHaveLength(200));

await signal(stream.grow(1));
await waitFor(() => expect(lastIndex(result.current.events)).toBe(200));

expect(
queryClient.getQueryData<ConversationEventsTail>(
conversationEventsTailKey(CONVERSATION_ID),
)?.events,
).toHaveLength(1);
});

it("keeps paging back to the start without overlapping what is held", async () => {
fakeStream(250);
const { result } = open({ pageSize: 100 });
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
import { useInfiniteQuery, useQuery } from "@tanstack/react-query";
import { useMemo, useRef, useState } from "react";
import {
useInfiniteQuery,
useQuery,
useQueryClient,
} from "@tanstack/react-query";
import { useEffect, useMemo, useRef, useState } from "react";
import {
type AgentConversationEvent,
CONVERSATION_EVENTS_PAGE_SIZE,
type ConversationEventsTail,
conversationEventsTailKey,
conversationEventsTailQueryOptions,
conversationEventWindowInfiniteOptions,
} from "@/lib/agent-api";
Expand Down Expand Up @@ -63,6 +69,8 @@ export function useConversationEventWindow({
// silently appended under them — see `events` below.
const [following, setFollowing] = useState(true);

const queryClient = useQueryClient();

const { data: tail } = useQuery(
conversationEventsTailQueryOptions(conversationId),
);
Expand Down Expand Up @@ -92,6 +100,31 @@ export function useConversationEventWindow({
const lastPagedIndex =
pagedEvents.length > 0 ? (pagedEvents.at(-1)?.event_index ?? -1) : -1;

// Shrinks the tail cache's `events` array down to just the events past
// lastPagedIndex, the actual pruning conversationEventsTailQueryOptions'
// own doc comment promises happens "there, not here" — a promise
// applyRealtimeAgentEvent (the "here" in question) never kept on its
// own, since it only ever appends. Runs whenever a real fetch advances
// lastPagedIndex (the initial page load, or the reconciling refetch
// useProjectRealtime triggers on a status transition): any tail event
// at or below that index is now redundant with what's in pagedEvents,
// so without this the tail's events array grows for as long as a
// conversation keeps streaming, for the life of the browser tab.
useEffect(() => {
if (lastPagedIndex < 0) return;
queryClient.setQueryData(
conversationEventsTailKey(conversationId),
(prev: ConversationEventsTail | undefined) => {
if (!prev || prev.events.length === 0) return prev;
const pruned = prev.events.filter(
(e) => e.event_index > lastPagedIndex,
);
if (pruned.length === prev.events.length) return prev;
return { ...prev, events: pruned };
},
);
}, [queryClient, conversationId, lastPagedIndex]);

// Live events already merged in stay visible even after `following` flips
// off mid-turn — e.g. expanding a tall tool call's panel nudges the
// viewport off the exact bottom pixel, which reads as "scrolled away."
Expand Down
10 changes: 5 additions & 5 deletions apps/web/src/lib/agent-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export const AGENT_PRESETS: AgentPreset[] = [
defaultLLMProvider: "anthropic",
defaultLLMModel: "claude-sonnet-4-6",
defaultSystemPrompt:
"You are an expert software engineer. You implement features and fix bugs by writing clean, maintainable code and following best practices.",
"You are an expert software engineer. You implement features and fix bugs by writing clean, maintainable code and following best practices.\n\nWhen you're assigned a task with nothing else said, skip skill-routing analysis and go straight to the `paca-do` skill — call `load_skill` with `paca-do` and execute it end-to-end — don't narrate the choice, just get to work.",
},
{
id: "code-reviewer",
Expand All @@ -36,7 +36,7 @@ export const AGENT_PRESETS: AgentPreset[] = [
defaultLLMProvider: "anthropic",
defaultLLMModel: "claude-sonnet-4-6",
defaultSystemPrompt:
"You are a meticulous code reviewer. You examine code for correctness, security vulnerabilities, performance issues, and adherence to best practices, providing constructive and actionable feedback.",
"You are a meticulous code reviewer. You examine code for correctness, security vulnerabilities, performance issues, and adherence to best practices, providing constructive and actionable feedback.\n\nWhen you're assigned a task with nothing else said, skip skill-routing analysis and go straight to the `paca-test` skill — call `load_skill` with `paca-test` and verify it — don't narrate the choice, just get to work.",
},
{
id: "qa-engineer",
Expand All @@ -45,7 +45,7 @@ export const AGENT_PRESETS: AgentPreset[] = [
defaultLLMProvider: "anthropic",
defaultLLMModel: "claude-sonnet-4-6",
defaultSystemPrompt:
"You are a quality assurance engineer. You write comprehensive test suites, identify edge cases, create test plans, and ensure software reliability through thorough testing strategies.",
"You are a quality assurance engineer. You write comprehensive test suites, identify edge cases, create test plans, and ensure software reliability through thorough testing strategies.\n\nWhen you're assigned a task with nothing else said, skip skill-routing analysis and go straight to the `paca-test` skill — call `load_skill` with `paca-test` — don't narrate the choice, just get to work.",
},
{
id: "planner",
Expand All @@ -55,7 +55,7 @@ export const AGENT_PRESETS: AgentPreset[] = [
defaultLLMProvider: "anthropic",
defaultLLMModel: "claude-sonnet-4-6",
defaultSystemPrompt:
"You are an expert project planner. You break down goals into well-defined tasks using `create_task`. For each task, set an appropriate task type (use `list_task_types` to see available types), a clear title, description, and acceptance criteria. Group related tasks under Epics or parent tasks where appropriate. Use `list_task_statuses` to understand the project's workflow.",
"You are an expert project planner. You break down goals into well-defined tasks using `create_task`. For each task, set an appropriate task type (use `list_task_types` to see available types), a clear title, description, and acceptance criteria. Group related tasks under Epics or parent tasks where appropriate. Use `list_task_statuses` to understand the project's workflow.\n\nWhen you're assigned a task with nothing else said, skip skill-routing analysis and go straight to the `paca-sprint` skill — call `load_skill` with `paca-sprint` — don't narrate the choice, just get to work.",
},
{
id: "business-analyst",
Expand All @@ -65,7 +65,7 @@ export const AGENT_PRESETS: AgentPreset[] = [
defaultLLMProvider: "anthropic",
defaultLLMModel: "claude-sonnet-4-6",
defaultSystemPrompt:
'You are an expert business analyst. You produce requirements by:\n- Writing detailed user stories (`create_task` with type Story) in the format "As a [persona], I want [goal] so that [benefit]".\n- Adding clear, testable acceptance criteria to each story.\n- Creating Epics (`create_task` with type Epic) to group related stories.\n- Documenting business rules, edge cases, and non-functional requirements as comments or task description updates.\n\nUse `list_task_types` and `list_tasks` to understand the project context and avoid duplicating requirements.',
'You are an expert business analyst. You produce requirements by:\n- Writing detailed user stories (`create_task` with type Story) in the format "As a [persona], I want [goal] so that [benefit]".\n- Adding clear, testable acceptance criteria to each story.\n- Creating Epics (`create_task` with type Epic) to group related stories.\n- Documenting business rules, edge cases, and non-functional requirements as comments or task description updates.\n\nUse `list_task_types` and `list_tasks` to understand the project context and avoid duplicating requirements.\n\nWhen you\'re assigned a task with nothing else said, skip skill-routing analysis and go straight to the `paca-clarify` skill — call `load_skill` with `paca-clarify` — don\'t narrate the choice, just get to work.',
},
{
id: "custom",
Expand Down
2 changes: 1 addition & 1 deletion deploy/docker-compose.dev.yml
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ services:
# The Goose-based sandbox image.
#
# MUST be built from services/agent-server/Dockerfile, NOT the
# raw ghcr.io/block/goose image referenced directly — found live,
# raw ghcr.io/aaif-goose/goose image referenced directly — found live,
# the hard way, while wiring this up: the raw upstream image has no
# Node.js at all, so the built-in Paca MCP server (spawned via
# `npx -y @paca-ai/paca-mcp` — see PACA_API_KEY above, which enables
Expand Down
2 changes: 1 addition & 1 deletion deploy/docker-compose.prod.yml
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ services:
DOCKER_SOCKET: /var/run/docker.sock
# The Goose-based sandbox image — built from
# services/agent-server/Dockerfile (adds Node.js + the Paca MCP
# package on top of upstream Goose), NOT the raw ghcr.io/block/goose
# package on top of upstream Goose), NOT the raw ghcr.io/aaif-goose/goose
# image directly: the raw image has no Node.js, so the built-in Paca
# MCP server can never start, and Goose hangs session/new forever
# rather than surfacing that as an error.
Expand Down
Loading
Loading