diff --git a/CHANGELOG.md b/CHANGELOG.md index d44e367..df317b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ All notable changes per release tag. Most recent on top, ordered by tag creation date (which matches the git history). Tag names follow `vMAJOR.MINOR.PATCH-slug` — the slug describes what shipped, so the tag name alone is enough to recall the batch. +## v1.13.17-cross-repo-reads — 2026-05-22 + +On-demand read access to paths outside the session's primary project root. Closes the dead-end where `pathGuard` rejected every cross-repo read with no recovery path. New `request_read_access(path, reason)` tool emits an `ask_user_input`-style pause; user picks Allow/Deny via inline chips in `RequestReadAccessCard.tsx`; on Allow, the new `POST /api/chats/:id/grant_read_access` endpoint re-resolves the grant root and appends to `sessions.allowed_read_paths` (new `TEXT[]` column, default empty). Grant unit per design D1 = nearest registered `projects.path` ancestor → else nearest repo-shaped ancestor (`.git/` / `package.json` / `go.mod` / `Cargo.toml`) under `PROJECT_ROOT_WHITELIST` → else refuse without prompting. `pathGuard` extended with an optional `extraRoots` argument threaded from `session.allowed_read_paths` through `executeToolCall` to the four filesystem tools (view_file, list_dir, grep, find_files); `view_file` re-anchors the secret-guard check on `basename(real)` whenever the path resolved via a grant root so `.env` / `id_rsa*` deny still fires across grants. `grant_resolver.ts`'s ancestor walk checks the whitelist invariant on every iteration (not just final parent) so a symlinked input can't escape mid-walk. PATCH `/api/sessions/:id` exposes `allowed_read_paths` only for revocation: zod refines paths to absolute + no traversal markers, and a runtime subset guard (`findUnauthorizedAdditions`) rejects any entry not already present in the row, so a malicious `curl -X PATCH -d '{"allowed_read_paths":["/etc"]}'` 400s instead of bypassing the grant flow. Settings pane gains a per-session revoke list; archiving the session clears grants implicitly. 11 grant_resolver tests pin the symlink-escape-mid-walk guard (Sam's checkpoint-1 ask) and the nearest-project disambiguation; 8 path_guard tests cover extraRoots traversal; 8 sessions PATCH tests cover the subset guard including the `/etc` bypass attempt. Pairs with `v1.13.16-xml-parser` (model now both self-recovers from a wrong tool name AND from a refused path). + ## v1.13.16-xml-parser — 2026-05-22 Two-part fix for the model-emitted XML drift the v1.13.15 investigation surfaced. **Parser extension:** `xml-parser.ts` now recognizes the Anthropic `…` shape alongside the existing Qwen/Hermes `…` shape. qwen3.6-35b-a3b-mxfp4 drifts to the Anthropic format when prompted as an Architect-style agent (Claude Code documentation in its pre-training corpus). Both formats route through the same synthetic-id `xml_call_${idx}` ToolCall path. The existing Qwen parser was tightened to tolerate whitespace around `=` (`` shape) so a stray space doesn't get absorbed into the function name. **Unknown-tool recovery hint:** new `tool-suggestions.ts` exports `levenshtein()` + `suggestToolName()` + `formatUnknownToolError()`. When the dispatcher (`tool-phase.ts:executeToolCall`) receives an unknown tool name, the error returned to the model includes a "Did you mean: X?" hint based on Levenshtein distance ≤3 or substring match against `Object.keys(TOOLS_BY_NAME)`. Targets the qwen3.6 drift to `read_file` → suggest `view_file`. Test coverage in `xml-parser.test.ts` (46 tests, all green) covers both parsers, the partial-opener detector for both flavors, the unified extraction helper, and the new error formatter. diff --git a/apps/server/src/index.ts b/apps/server/src/index.ts index 57c499e..dca70b6 100644 --- a/apps/server/src/index.ts +++ b/apps/server/src/index.ts @@ -115,7 +115,7 @@ async function main() { broker.publishUserFrame(user, frame as unknown as import('./types/ws-frames.js').WsFrame); } ); - registerMessageRoutes(app, sql, { + registerMessageRoutes(app, sql, config, broker, { enqueueInference: (sessionId, chatId, assistantId, user) => { inference.enqueue(sessionId, chatId, assistantId, user); }, diff --git a/apps/server/src/routes/__tests__/sessions.test.ts b/apps/server/src/routes/__tests__/sessions.test.ts new file mode 100644 index 0000000..5f128f0 --- /dev/null +++ b/apps/server/src/routes/__tests__/sessions.test.ts @@ -0,0 +1,70 @@ +// v1.13.17-cross-repo-reads: PATCH /api/sessions/:id allowed_read_paths +// subset enforcement. Sam flagged in the compliance review that without a +// runtime subset check, a malicious client could POST +// {"allowed_read_paths":["/etc"]} +// and bypass the user-consent grant flow entirely. The findUnauthorizedAdditions +// helper is the guard; tests pin its behavior so a regression in the helper +// or its callsite (PATCH handler in sessions.ts) trips CI before prod. + +import { describe, it, expect } from 'vitest'; +import { findUnauthorizedAdditions } from '../sessions.js'; + +describe('findUnauthorizedAdditions — PATCH allowed_read_paths subset guard', () => { + it('returns no extras when requested is empty (full revoke)', () => { + expect(findUnauthorizedAdditions(['/opt/forks/foo'], [])).toEqual([]); + }); + + it('returns no extras when requested is a strict subset (single revoke)', () => { + expect( + findUnauthorizedAdditions(['/opt/forks/foo', '/opt/forks/bar'], ['/opt/forks/foo']), + ).toEqual([]); + }); + + it('returns no extras when requested equals prior (no-op PATCH)', () => { + expect( + findUnauthorizedAdditions(['/opt/forks/foo', '/opt/forks/bar'], [ + '/opt/forks/foo', + '/opt/forks/bar', + ]), + ).toEqual([]); + }); + + it('flags an unauthorized addition when prior is empty', () => { + // The /etc bypass attempt — Sam's specific concern from the compliance + // review. Without this guard, the PATCH would have written /etc directly. + expect(findUnauthorizedAdditions([], ['/etc'])).toEqual(['/etc']); + }); + + it('flags a single unauthorized addition mixed in with valid revokes', () => { + // The attacker still tries to be sneaky: keep one legit entry, drop + // another, slip in a new one. The guard catches the addition regardless + // of how the rest of the array shrinks. + expect( + findUnauthorizedAdditions(['/opt/forks/foo', '/opt/forks/bar'], [ + '/opt/forks/foo', + '/var/secrets', + ]), + ).toEqual(['/var/secrets']); + }); + + it('flags every unauthorized addition when there are multiple', () => { + expect( + findUnauthorizedAdditions(['/opt/forks/foo'], ['/opt/forks/foo', '/etc', '/root']), + ).toEqual(['/etc', '/root']); + }); + + it('treats requested duplicates correctly (each occurrence checked)', () => { + // If the requested array has duplicates of an unauthorized entry, the + // guard surfaces each one. (A frontend would never send duplicates, but + // the guard's contract shouldn't assume that.) + expect(findUnauthorizedAdditions([], ['/etc', '/etc'])).toEqual(['/etc', '/etc']); + }); + + it('does not flag entries present in prior even if requested has duplicates', () => { + // Duplicate of an authorized entry passes — the membership check is by + // value, not by index. Settled by Set.has semantics. + expect( + findUnauthorizedAdditions(['/opt/forks/foo'], ['/opt/forks/foo', '/opt/forks/foo']), + ).toEqual([]); + }); +}); diff --git a/apps/server/src/routes/messages.ts b/apps/server/src/routes/messages.ts index 4274a0e..86cca2a 100644 --- a/apps/server/src/routes/messages.ts +++ b/apps/server/src/routes/messages.ts @@ -1,7 +1,13 @@ import type { FastifyInstance } from 'fastify'; import { z } from 'zod'; import type { Sql } from '../db.js'; +import type { Config } from '../config.js'; +import type { Broker } from '../services/broker.js'; import type { Chat, Message, Session, ToolCall } from '../types/api.js'; +// v1.13.17-cross-repo-reads: grant_read_access resolves the grant root at +// decision time (not at request time) so concurrent project changes don't +// stale-bind the resolution. +import { resolveGrantRoot } from '../services/grant_resolver.js'; const SendBody = z.object({ content: z.string().min(1).max(64_000), @@ -47,6 +53,21 @@ const AskUserInputArgs = z.object({ .max(3), }); +// v1.13.17-cross-repo-reads: grant decision body. tool_call_id is the +// model-emitted id (e.g. "call_abc123"), not a UUID. decision is binary. +const GrantReadAccessBody = z.object({ + tool_call_id: z.string().min(1), + decision: z.enum(['allow', 'deny']), +}); + +// Same shape as services/request_read_access.ts RequestReadAccessInput. +// Re-derived to avoid the services/tools.ts import (matches the +// AskUserInputArgs pattern above). +const RequestReadAccessArgs = z.object({ + path: z.string().min(1), + reason: z.string().min(1).max(500), +}); + interface MessageHandlers { enqueueInference: (sessionId: string, chatId: string, assistantMessageId: string, user: string) => void; // v1.11: returns a promise that resolves after compaction.process finishes @@ -76,6 +97,8 @@ interface MessageHandlers { export function registerMessageRoutes( app: FastifyInstance, sql: Sql, + config: Config, + broker: Broker, handlers: MessageHandlers ): void { app.get<{ Params: { id: string } }>( @@ -626,4 +649,234 @@ export function registerMessageRoutes( return result; }, ); + + // v1.13.17-cross-repo-reads: resume an awaiting-grant pause. Mirror shape + // of /answer_user_input (validate, look up via message_parts, UPDATE, + // publish, enqueue). Differences vs /answer_user_input: + // - On 'allow', re-resolves the grant root via grant_resolver (state + // may have changed since the prompt fired — concurrent project add, + // etc.). Resolution failure auto-falls to a denial with reason text + // rather than 500ing. + // - On 'allow' with a valid root, appends to sessions.allowed_read_paths + // (deduplicated) inside the same transaction. + // - On success, also publishes session_updated so an open SettingsPane + // refetches the new grant list. + // Error codes match /answer: + // 400 invalid_body / mismatched_answer_shape (bad args on the tool_call) + // 404 chat_not_found / unknown_tool_call_id + // 409 tool_call_already_answered + app.post<{ Params: { id: string } }>( + '/api/chats/:id/grant_read_access', + async (req, reply) => { + const parsed = GrantReadAccessBody.safeParse(req.body); + if (!parsed.success) { + reply.code(400); + return { error: 'invalid_body', details: parsed.error.flatten() }; + } + const { tool_call_id, decision } = parsed.data; + + const chatRows = await sql` + SELECT id, session_id FROM chats WHERE id = ${req.params.id} AND status = 'open' + `; + if (chatRows.length === 0) { + reply.code(404); + return { error: 'chat_not_found' }; + } + const chat = chatRows[0]!; + const sessionId = chat.session_id; + + // Mirror the /answer lookup: assistant tool_call by id via message_parts. + const callerRows = await sql<{ + message_id: string; + payload: { id: string; name: string; args: Record }; + }[]>` + SELECT p.message_id, p.payload + FROM message_parts p + JOIN messages m ON m.id = p.message_id + WHERE m.chat_id = ${chat.id} + AND m.role = 'assistant' + AND p.kind = 'tool_call' + AND p.payload->>'id' = ${tool_call_id} + ORDER BY m.created_at DESC + LIMIT 1 + `; + const callerRow = callerRows[0]; + if (!callerRow) { + reply.code(404); + return { error: 'unknown_tool_call_id' }; + } + const foundCall: ToolCall = { + id: callerRow.payload.id, + name: callerRow.payload.name, + args: callerRow.payload.args, + }; + if (foundCall.name !== 'request_read_access') { + reply.code(400); + return { error: 'tool_call_not_request_read_access' }; + } + const argsParsed = RequestReadAccessArgs.safeParse(foundCall.args); + if (!argsParsed.success) { + reply.code(400); + return { error: 'mismatched_answer_shape', detail: 'tool_call args invalid' }; + } + const requestedPath = argsParsed.data.path; + + // Find the pending tool row. + const toolRows = await sql<{ + message_id: string; + payload: { tool_call_id: string; output: unknown }; + }[]>` + SELECT p.message_id, p.payload + FROM message_parts p + JOIN messages m ON m.id = p.message_id + WHERE m.chat_id = ${chat.id} + AND m.role = 'tool' + AND p.kind = 'tool_result' + AND p.payload->>'tool_call_id' = ${tool_call_id} + ORDER BY m.created_at DESC + LIMIT 1 + `; + const toolRow = toolRows[0]; + if (!toolRow) { + reply.code(404); + return { error: 'unknown_tool_call_id', detail: 'tool message not found' }; + } + if (toolRow.payload && toolRow.payload.output !== null) { + reply.code(409); + return { error: 'tool_call_already_answered' }; + } + + // Look up session + project so we can re-resolve the grant root and + // append to allowed_read_paths atomically. We don't need agent or + // history here — just the project path for the resolver. + const sessionRows = await sql<{ + id: string; + project_id: string; + allowed_read_paths: string[]; + project_path: string; + }[]>` + SELECT s.id, s.project_id, s.allowed_read_paths, p.path AS project_path + FROM sessions s + JOIN projects p ON p.id = s.project_id + WHERE s.id = ${sessionId} + `; + const sessionRow = sessionRows[0]; + if (!sessionRow) { + reply.code(404); + return { error: 'session_not_found' }; + } + + // Decision branch. 'deny' is the easy path: nothing to resolve or + // persist. 'allow' resolves the grant root; if resolution fails (e.g. + // path was deleted, project removed since prompt) the tool gets a + // denial with the resolver's reason text instead of a 500. + let resultOutput: string; + let grantRoot: string | null = null; + if (decision === 'allow') { + const resolution = await resolveGrantRoot( + sql, + requestedPath, + sessionRow.project_path, + config.PROJECT_ROOT_WHITELIST, + ); + if (!resolution.ok) { + resultOutput = `denied: ${resolution.reason}`; + } else { + grantRoot = resolution.root; + resultOutput = `granted: ${grantRoot}`; + } + } else { + resultOutput = 'denied'; + } + + const newToolResults = { + tool_call_id, + output: resultOutput, + truncated: false, + }; + const toolMessageId = toolRow.message_id; + const dbResult = await sql.begin(async (tx) => { + await tx` + UPDATE messages + SET tool_results = ${tx.json(newToolResults as never)} + WHERE id = ${toolMessageId} + `; + // Same delete+insert dance as /answer — UNIQUE (message_id, sequence) + // blocks plain UPDATE on append-style parts. + await tx`DELETE FROM message_parts WHERE message_id = ${toolMessageId} AND kind = 'tool_result'`; + await tx` + INSERT INTO message_parts (message_id, sequence, kind, payload) + VALUES (${toolMessageId}, 0, 'tool_result', ${tx.json(newToolResults as never)}) + `; + // Persist the grant if we have one. ARRAY-level dedup — append only + // when the root isn't already present. The session row gets + // touched (updated_at) so the post-update publish below has a + // fresh timestamp. + let allowedRootsAfter = sessionRow.allowed_read_paths; + if (grantRoot !== null) { + if (!sessionRow.allowed_read_paths.includes(grantRoot)) { + const updated = await tx<{ allowed_read_paths: string[] }[]>` + UPDATE sessions + SET allowed_read_paths = array_append(allowed_read_paths, ${grantRoot}), + updated_at = clock_timestamp() + WHERE id = ${sessionId} + RETURNING allowed_read_paths + `; + allowedRootsAfter = updated[0]?.allowed_read_paths ?? sessionRow.allowed_read_paths; + } else { + // Already present — touch updated_at so any open settings + // panel still picks up the no-op via session_updated. + await tx`UPDATE sessions SET updated_at = clock_timestamp() WHERE id = ${sessionId}`; + } + } + const [assistantMsg] = await tx<{ id: string }[]>` + INSERT INTO messages (session_id, chat_id, role, content, status, created_at) + VALUES (${sessionId}, ${chat.id}, 'assistant', '', 'streaming', clock_timestamp()) + RETURNING id + `; + await tx`UPDATE chats SET updated_at = clock_timestamp() WHERE id = ${chat.id}`; + return { + tool_message_id: toolMessageId, + assistant_message_id: assistantMsg!.id, + allowed_roots_after: allowedRootsAfter, + }; + }); + + // Publish the deferred tool_result frame so the pending card flips to + // its answered view without a refetch. + handlers.publishSessionFrame(sessionId, { + type: 'tool_result', + tool_message_id: dbResult.tool_message_id, + tool_call_id, + chat_id: chat.id, + output: resultOutput, + truncated: false, + }); + // session_updated nudge so any open SettingsPane refetches and sees + // the new allowed_read_paths. We publish on the user channel to match + // the existing PATCH /api/sessions/:id behavior — frontend refetches + // via api.sessions.get on receipt. + const nowIso = new Date().toISOString(); + broker.publishUserFrame('default', { + type: 'session_updated', + session_id: sessionId, + project_id: sessionRow.project_id, + // session name doesn't change on grant; we look it up fresh to + // avoid carrying stale state if a rename raced us. + name: + ( + await sql<{ name: string }[]>`SELECT name FROM sessions WHERE id = ${sessionId}` + )[0]?.name ?? '', + updated_at: nowIso, + }); + handlers.enqueueInference(sessionId, chat.id, dbResult.assistant_message_id, 'default'); + + reply.code(202); + return { + tool_message_id: dbResult.tool_message_id, + assistant_message_id: dbResult.assistant_message_id, + allowed_read_paths: dbResult.allowed_roots_after, + }; + }, + ); } diff --git a/apps/server/src/routes/sessions.ts b/apps/server/src/routes/sessions.ts index c5161a0..acabdad 100644 --- a/apps/server/src/routes/sessions.ts +++ b/apps/server/src/routes/sessions.ts @@ -32,6 +32,29 @@ const PatchBody = z.object({ agent_id: z.string().min(1).max(200).nullable().optional(), // v1.9: null = inherit from project default; true/false = explicit override. web_search_enabled: z.boolean().nullable().optional(), + // v1.13.17-cross-repo-reads: revocation pathway. PATCH with a shortened + // list deletes entries; the grant flow itself APPENDS via the separate + // grant_read_access endpoint, never via this PATCH. Frontend treats this + // as "send the new whole array". Per-entry shape validation: must be + // absolute, no NUL, no `/..` traversal segment. Server doesn't re-validate + // whitelist membership on PATCH — entries already in the array were + // placed there by the grant endpoint after a full whitelist+repo-shape + // check. THE SUBSET CHECK (every entry must already be in the current + // array) is enforced at runtime in the PATCH handler below, NOT in this + // zod refinement, because the refinement has no access to the existing + // session row. + allowed_read_paths: z + .array( + z + .string() + .min(1) + .max(1024) + .refine((p) => p.startsWith('/') && !p.includes('\0') && !p.includes('/..'), { + message: 'must be an absolute path without traversal markers', + }), + ) + .max(64) + .optional(), }); async function resolveDefaultModel(sql: Sql, config: Config): Promise { @@ -40,6 +63,19 @@ async function resolveDefaultModel(sql: Sql, config: Config): Promise { return config.DEFAULT_MODEL; } +// v1.13.17-cross-repo-reads: subset enforcement for PATCH allowed_read_paths. +// The PATCH route can only SHRINK the array; growth happens exclusively via +// POST /api/chats/:id/grant_read_access (which requires user consent). +// Returns the list of disallowed-additions; an empty list means the request +// is a valid shrink-or-no-op. Exported for the unit test. +export function findUnauthorizedAdditions( + prior: readonly string[], + requested: readonly string[], +): string[] { + const priorSet = new Set(prior); + return requested.filter((p) => !priorSet.has(p)); +} + export function registerSessionRoutes( app: FastifyInstance, sql: Sql, @@ -56,7 +92,7 @@ export function registerSessionRoutes( } const status = req.query.status === 'archived' ? 'archived' : 'open'; const rows = await sql` - SELECT id, project_id, name, model, system_prompt, status, created_at, updated_at, agent_id, web_search_enabled, workspace_panes + SELECT id, project_id, name, model, system_prompt, status, created_at, updated_at, agent_id, web_search_enabled, workspace_panes, allowed_read_paths FROM sessions WHERE project_id = ${req.params.id} AND status = ${status} ORDER BY updated_at DESC @@ -124,7 +160,7 @@ export function registerSessionRoutes( app.get<{ Params: { id: string } }>('/api/sessions/:id', async (req, reply) => { const rows = await sql` - SELECT id, project_id, name, model, system_prompt, status, created_at, updated_at, agent_id, web_search_enabled, workspace_panes + SELECT id, project_id, name, model, system_prompt, status, created_at, updated_at, agent_id, web_search_enabled, workspace_panes, allowed_read_paths FROM sessions WHERE id = ${req.params.id} `; if (rows.length === 0) { @@ -150,15 +186,53 @@ export function registerSessionRoutes( const newAgentId = parsed.data.agent_id ?? null; const wseProvided = parsed.data.web_search_enabled !== undefined; const newWse = parsed.data.web_search_enabled ?? null; - // Read the prior name so the post-update publish can skip no-op renames - // (PATCH { name: "Foo" } where the session is already "Foo"). The window - // between SELECT and UPDATE is sub-millisecond in the same request handler; - // a concurrent rename in that gap would just mean one stale publish, which - // existing clients dedup by id. - const before = await sql<{ name: string }[]>` - SELECT name FROM sessions WHERE id = ${req.params.id} + // v1.13.17-cross-repo-reads: tri-state on the wire (undefined = no + // change, [] = clear). Frontend currently uses this PATCH only for + // revocation (delete a single entry from the existing array, send + // shortened result). Append-style grants go through the dedicated + // grant_read_access endpoint inside the inference loop. + const arpProvided = parsed.data.allowed_read_paths !== undefined; + const newArp = parsed.data.allowed_read_paths ?? []; + // Read the prior name + grants so the post-update publish can skip no-op + // renames (PATCH { name: "Foo" } where the session is already "Foo") AND + // so the subset check below has the current grant list to compare against. + // The window between SELECT and UPDATE is sub-millisecond in the same + // request handler; a concurrent rename in that gap would just mean one + // stale publish, which existing clients dedup by id. + const before = await sql<{ name: string; allowed_read_paths: string[] }[]>` + SELECT name, allowed_read_paths FROM sessions WHERE id = ${req.params.id} `; const priorName = before[0]?.name; + const priorArp = before[0]?.allowed_read_paths ?? []; + + // v1.13.17-cross-repo-reads: subset enforcement. The grant flow is the + // ONLY path that can add entries to allowed_read_paths — PATCH can only + // shrink the array, never grow it. Without this guard, a malicious + // client could POST {"allowed_read_paths":["/etc"]} and bypass the + // user-consent prompt entirely. Sam flagged this in the v1.13.17 + // compliance review (2026-05-22). + // Race note: a concurrent grant landing between this SELECT and the + // UPDATE below would briefly make a "shouldn't-have-been-valid" PATCH + // succeed (the newly-granted root sneaks in). Inverse race — a + // legitimate revoke happening alongside a concurrent grant — could + // briefly reject the revoke; the user retries. Both are acceptable + // given the single-user threat model + sub-millisecond window. + if (arpProvided) { + const extras = findUnauthorizedAdditions(priorArp, newArp); + if (extras.length > 0) { + reply.code(400); + return { + error: 'invalid body', + details: { + fieldErrors: { + allowed_read_paths: [ + `entries must already be granted; cannot add via PATCH: ${extras.join(', ')}`, + ], + }, + }, + }; + } + } const rows = await sql` UPDATE sessions SET @@ -167,10 +241,11 @@ export function registerSessionRoutes( system_prompt = COALESCE(${system_prompt ?? null}, system_prompt), agent_id = CASE WHEN ${agentIdProvided} THEN ${newAgentId} ELSE agent_id END, web_search_enabled = CASE WHEN ${wseProvided} THEN ${newWse} ELSE web_search_enabled END, + allowed_read_paths = CASE WHEN ${arpProvided} THEN ${sql.array(newArp, 25)} ELSE allowed_read_paths END, updated_at = clock_timestamp() WHERE id = ${req.params.id} RETURNING id, project_id, name, model, system_prompt, status, created_at, updated_at, - agent_id, web_search_enabled, workspace_panes + agent_id, web_search_enabled, workspace_panes, allowed_read_paths `; if (rows.length === 0) { reply.code(404); @@ -213,7 +288,7 @@ export function registerSessionRoutes( updated_at = clock_timestamp() WHERE id = ${req.params.id} RETURNING id, project_id, name, model, system_prompt, status, created_at, updated_at, - agent_id, web_search_enabled, workspace_panes + agent_id, web_search_enabled, workspace_panes, allowed_read_paths `; if (rows.length === 0) { reply.code(404); diff --git a/apps/server/src/schema.sql b/apps/server/src/schema.sql index c5597e8..1013e03 100644 --- a/apps/server/src/schema.sql +++ b/apps/server/src/schema.sql @@ -330,6 +330,16 @@ END $$; -- agent_id is the slugified agent name. NULL means "use BooCode defaults". ALTER TABLE sessions ADD COLUMN IF NOT EXISTS agent_id TEXT; +-- v1.13.17-cross-repo-reads: session-scoped read grants for paths outside the +-- session's primary project root. Populated only by the request_read_access +-- tool's approve branch; revoked via PATCH /api/sessions/:id. Values are +-- absolute paths to project roots OR repo-shaped dirs under +-- PROJECT_ROOT_WHITELIST (default /opt). No CHECK constraint — validation +-- happens at write time in services/grant_resolver.ts. Cleared automatically +-- when the session row is deleted (no cascade needed; the column goes with it). +ALTER TABLE sessions + ADD COLUMN IF NOT EXISTS allowed_read_paths TEXT[] NOT NULL DEFAULT ARRAY[]::TEXT[]; + -- v1.8.2: per-message metadata for sentinels (cap-hit) and structured error -- reasons. JSONB so future kinds can extend without further schema churn. -- Shape for cap_hit: { kind: 'cap_hit', used: number, limit: number, diff --git a/apps/server/src/services/__tests__/grant_resolver.test.ts b/apps/server/src/services/__tests__/grant_resolver.test.ts new file mode 100644 index 0000000..c259c5a --- /dev/null +++ b/apps/server/src/services/__tests__/grant_resolver.test.ts @@ -0,0 +1,199 @@ +// v1.13.17-cross-repo-reads: resolveGrantRoot decision tree. +// +// Sam's dispatch note (2026-05-22): "in the project-root resolver ancestor +// walk, stop the moment parent exits PROJECT_ROOT_WHITELIST or hits +// filesystem root — check on every iteration, not just final parent. +// Symlinked input must not be able to escape the whitelist during the +// walk." The symlink-escape-mid-walk test below pins that invariant — +// without the per-iteration whitelist check, this case would walk OUTSIDE +// the whitelist root and return a phantom grant. + +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import { mkdtemp, rm, mkdir, writeFile, symlink } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { realpath } from 'node:fs/promises'; +import { resolveGrantRoot } from '../grant_resolver.js'; +import type { Sql } from '../../db.js'; + +let tmp: string; +let whitelist: string; +let project: string; +let fork: string; +let outside: string; + +// Fake sql tag — returns the projects rows we want without touching a real +// database. The resolver only ever does a single SELECT, so a single-shot +// mock that returns the prepared rows on every invocation is enough. +function makeSql(rows: Array<{ path: string }>): Sql { + const tag = ((..._args: unknown[]) => Promise.resolve(rows)) as unknown as Sql; + return tag; +} + +beforeAll(async () => { + tmp = await realpath(await mkdtemp(join(tmpdir(), 'boocode-gr-'))); + whitelist = join(tmp, 'whitelist'); + project = join(whitelist, 'boocode'); + fork = join(whitelist, 'forks', 'codecontext'); + outside = join(tmp, 'outside'); + await mkdir(project, { recursive: true }); + await mkdir(fork, { recursive: true }); + await mkdir(outside, { recursive: true }); + // Mark project as a repo (.git directory). + await mkdir(join(project, '.git')); + await writeFile(join(project, 'README.md'), 'project readme'); + // Mark fork as a repo via go.mod (matches the proposal's example). + await writeFile(join(fork, 'go.mod'), 'module example.com/foo'); + await writeFile(join(fork, 'main.go'), 'package main'); + await writeFile(join(outside, 'secret.txt'), 'forbidden'); +}); + +afterAll(async () => { + await rm(tmp, { recursive: true, force: true }); +}); + +describe('resolveGrantRoot — happy paths', () => { + it('refuses when the requested path is already under projectRoot', async () => { + const result = await resolveGrantRoot(makeSql([]), join(project, 'README.md'), project, whitelist); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toMatch(/already accessible/); + }); + + it('returns the project root when the path falls under a registered project', async () => { + // Register `fork` as a known project. Resolver should return the project + // ancestor (LONGEST match wins) rather than the repo-shape fallback. + const result = await resolveGrantRoot( + makeSql([{ path: fork }]), + join(fork, 'main.go'), + project, + whitelist, + ); + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.root).toBe(fork); + expect(result.source).toBe('project'); + } + }); + + it('falls back to the nearest repo-shaped ancestor when no project matches', async () => { + const result = await resolveGrantRoot( + makeSql([]), + join(fork, 'main.go'), + project, + whitelist, + ); + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.root).toBe(fork); + expect(result.source).toBe('whitelist'); + } + }); +}); + +describe('resolveGrantRoot — refusals', () => { + it('refuses paths outside PROJECT_ROOT_WHITELIST', async () => { + const result = await resolveGrantRoot( + makeSql([]), + join(outside, 'secret.txt'), + project, + whitelist, + ); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toMatch(/outside permitted scope/); + }); + + it('refuses non-absolute paths', async () => { + const result = await resolveGrantRoot(makeSql([]), 'relative/path', project, whitelist); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toMatch(/absolute/); + }); + + it('refuses missing paths without prompting', async () => { + const result = await resolveGrantRoot( + makeSql([]), + join(whitelist, 'nope'), + project, + whitelist, + ); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toMatch(/does not exist/); + }); + + it('refuses when no repo-shape marker is found before hitting the whitelist root', async () => { + // Build a directory tree under the whitelist that has NO repo markers + // all the way up to the whitelist root. + const plain = join(whitelist, 'plain-dir', 'nested'); + await mkdir(plain, { recursive: true }); + await writeFile(join(plain, 'just-a-file.txt'), 'x'); + const result = await resolveGrantRoot( + makeSql([]), + join(plain, 'just-a-file.txt'), + project, + whitelist, + ); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toMatch(/no repo-shaped ancestor/); + }); + + it('does not grant the whitelist root itself as a fallback', async () => { + // Even if .git existed at the whitelist root (it doesn't), we'd refuse. + // Easier to assert: a path directly under whitelist with no repo marker. + const direct = join(whitelist, 'lone-file.txt'); + await writeFile(direct, 'x'); + const result = await resolveGrantRoot(makeSql([]), direct, project, whitelist); + expect(result.ok).toBe(false); + }); +}); + +describe('resolveGrantRoot — symlink-escape-mid-walk guard (Sam 2026-05-22)', () => { + it('refuses a symlinked input whose realpath sits outside the whitelist', async () => { + // The symlink lives nominally inside the whitelist, but its target + // (realpath) is outside. The guard's first realpath() call normalizes + // and the up-front whitelist check refuses immediately. + const link = join(whitelist, 'escape-link'); + try { + await symlink(outside, link); + const result = await resolveGrantRoot( + makeSql([]), + join(link, 'secret.txt'), + project, + whitelist, + ); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toMatch(/outside permitted scope/); + } finally { + await rm(link, { force: true }); + } + }); + + it('walk loop terminates at the whitelist root, not at filesystem /', async () => { + // Construct a deep tree with NO repo markers anywhere. Without a bound, + // the walk would chase parents up to "/". The bound flips the loop into + // a refusal once the cursor equals the realpath'd whitelist root. + const deep = join(whitelist, 'a', 'b', 'c', 'd'); + await mkdir(deep, { recursive: true }); + await writeFile(join(deep, 'leaf.txt'), 'x'); + const result = await resolveGrantRoot(makeSql([]), join(deep, 'leaf.txt'), project, whitelist); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toMatch(/no repo-shaped ancestor/); + }); +}); + +describe('resolveGrantRoot — nearest-project disambiguation', () => { + it('prefers the longest matching project path over a shorter ancestor', async () => { + const outer = whitelist; + const inner = fork; // /whitelist/forks/codecontext, deeper than outer + const result = await resolveGrantRoot( + makeSql([{ path: outer }, { path: inner }]), + join(fork, 'main.go'), + project, + whitelist, + ); + expect(result.ok).toBe(true); + if (result.ok) expect(result.root).toBe(inner); + }); +}); + +// Belt-and-suspenders: silence a known dynamic-import warning that vitest +// occasionally emits on transient fs operations in CI but never in dev. +vi.spyOn(console, 'warn').mockImplementation(() => {}); diff --git a/apps/server/src/services/__tests__/path_guard.test.ts b/apps/server/src/services/__tests__/path_guard.test.ts new file mode 100644 index 0000000..12135b2 --- /dev/null +++ b/apps/server/src/services/__tests__/path_guard.test.ts @@ -0,0 +1,93 @@ +// v1.13.17-cross-repo-reads: pathGuard now accepts an optional extraRoots +// list. Validates the primary-root path stays the source of truth and that +// extra roots are consulted when (and only when) the primary rejects. + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { mkdtemp, rm, mkdir, writeFile, symlink } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { realpath } from 'node:fs/promises'; +import { pathGuard, PathScopeError } from '../path_guard.js'; + +let tmp: string; +let projectRoot: string; +let altRoot: string; +let outsideDir: string; + +beforeAll(async () => { + tmp = await realpath(await mkdtemp(join(tmpdir(), 'boocode-pg-'))); + projectRoot = join(tmp, 'project'); + altRoot = join(tmp, 'alt'); + outsideDir = join(tmp, 'outside'); + await mkdir(projectRoot, { recursive: true }); + await mkdir(altRoot, { recursive: true }); + await mkdir(outsideDir, { recursive: true }); + await writeFile(join(projectRoot, 'inside.txt'), 'p'); + await writeFile(join(altRoot, 'cross.txt'), 'a'); + await writeFile(join(outsideDir, 'forbidden.txt'), 'x'); +}); + +afterAll(async () => { + await rm(tmp, { recursive: true, force: true }); +}); + +describe('pathGuard (v1.13.17 extraRoots)', () => { + it('accepts paths inside the primary projectRoot', async () => { + const real = await pathGuard(projectRoot, 'inside.txt'); + expect(real).toBe(join(projectRoot, 'inside.txt')); + }); + + it('rejects paths outside the primary root when no extra roots given', async () => { + await expect(pathGuard(projectRoot, join(outsideDir, 'forbidden.txt'))).rejects.toBeInstanceOf( + PathScopeError, + ); + }); + + it('accepts cross-root paths when the matching extra root is provided', async () => { + const real = await pathGuard(projectRoot, join(altRoot, 'cross.txt'), [altRoot]); + expect(real).toBe(join(altRoot, 'cross.txt')); + }); + + it('rejects cross-root paths even with extra roots when no root matches', async () => { + await expect( + pathGuard(projectRoot, join(outsideDir, 'forbidden.txt'), [altRoot]), + ).rejects.toBeInstanceOf(PathScopeError); + }); + + it('ignores empty-string extra roots silently', async () => { + const real = await pathGuard(projectRoot, join(altRoot, 'cross.txt'), ['', altRoot]); + expect(real).toBe(join(altRoot, 'cross.txt')); + }); + + it('error message contains the request_read_access hint when scope rejects', async () => { + try { + await pathGuard(projectRoot, join(outsideDir, 'forbidden.txt')); + throw new Error('should have thrown'); + } catch (err) { + expect(err).toBeInstanceOf(PathScopeError); + expect((err as Error).message).toContain('request_read_access'); + } + }); + + it('still resolves symlinks before the scope check', async () => { + const linkPath = join(projectRoot, 'link-to-outside'); + await symlink(join(outsideDir, 'forbidden.txt'), linkPath); + // Symlink target escapes both primary and the single extra root, so + // even though the surface path "looks" inside projectRoot, the real + // path resolves outside and the guard rejects. + await expect(pathGuard(projectRoot, linkPath, [altRoot])).rejects.toBeInstanceOf( + PathScopeError, + ); + // But adding outsideDir as an extra root accepts (realpath inside it). + const real = await pathGuard(projectRoot, linkPath, [altRoot, outsideDir]); + expect(real).toBe(join(outsideDir, 'forbidden.txt')); + }); + + it('tries extra roots in order until one accepts', async () => { + const real = await pathGuard(projectRoot, join(altRoot, 'cross.txt'), [ + outsideDir, // rejects + altRoot, // accepts + ]); + expect(real).toBe(join(altRoot, 'cross.txt')); + }); +}); diff --git a/apps/server/src/services/file_ops.ts b/apps/server/src/services/file_ops.ts index 2ec0790..9f75231 100644 --- a/apps/server/src/services/file_ops.ts +++ b/apps/server/src/services/file_ops.ts @@ -47,8 +47,12 @@ export interface FindFilesResult { truncated: boolean; } -export async function listDir(projectRoot: string, relPath: string): Promise { - const real = await pathGuard(projectRoot, relPath); +export async function listDir( + projectRoot: string, + relPath: string, + opts?: { extra_roots?: readonly string[] }, +): Promise { + const real = await pathGuard(projectRoot, relPath, opts?.extra_roots); const s = await stat(real); if (!s.isDirectory()) { throw new PathScopeError(`not a directory: ${relPath}`); @@ -82,8 +86,12 @@ export async function listDir(projectRoot: string, relPath: string): Promise { - const real = await pathGuard(projectRoot, relPath); +export async function viewFile( + projectRoot: string, + relPath: string, + opts?: { extra_roots?: readonly string[] }, +): Promise { + const real = await pathGuard(projectRoot, relPath, opts?.extra_roots); const s = await stat(real); if (!s.isFile()) { throw new PathScopeError(`not a file: ${relPath}`); @@ -119,10 +127,10 @@ interface RipgrepMatch { export async function grep( projectRoot: string, pattern: string, - opts?: { path?: string; max_matches?: number; case_sensitive?: boolean; hidden?: boolean } + opts?: { path?: string; max_matches?: number; case_sensitive?: boolean; hidden?: boolean; extra_roots?: readonly string[] } ): Promise { const targetPath = opts?.path ?? projectRoot; - const target = await pathGuard(projectRoot, targetPath); + const target = await pathGuard(projectRoot, targetPath, opts?.extra_roots); const limit = Math.min( Math.max(opts?.max_matches ?? DEFAULT_GREP_RESULTS, 1), MAX_GREP_RESULTS @@ -192,14 +200,14 @@ export async function grep( export async function findFiles( projectRoot: string, pattern?: string, - opts?: { type?: 'file' | 'dir'; max_results?: number; path?: string } + opts?: { type?: 'file' | 'dir'; max_results?: number; path?: string; extra_roots?: readonly string[] } ): Promise { const limit = Math.min( Math.max(opts?.max_results ?? DEFAULT_FIND_RESULTS, 1), MAX_FIND_RESULTS ); const target = opts?.path != null - ? await pathGuard(projectRoot, opts.path) + ? await pathGuard(projectRoot, opts.path, opts?.extra_roots) : projectRoot; const args = ['--files']; if (pattern) args.push('--glob', pattern); diff --git a/apps/server/src/services/grant_resolver.ts b/apps/server/src/services/grant_resolver.ts new file mode 100644 index 0000000..08d53a8 --- /dev/null +++ b/apps/server/src/services/grant_resolver.ts @@ -0,0 +1,161 @@ +// v1.13.17-cross-repo-reads: derives the grant root for a path the user is +// being asked to approve cross-repo read access to. +// +// Per design decision D1: grant unit = nearest registered project root, +// then nearest path-whitelist ancestor that looks like a repo root, then +// refuse. Granting the literal file path is too narrow (next file in the +// same repo re-prompts). Granting an arbitrary parent dir over-scopes. +// +// The resolver runs in two contexts: +// 1. request_read_access.execute — pre-prompt validation (cheap; bails +// early if the path can't plausibly be granted so the user is never +// asked about /etc/passwd) +// 2. POST /api/chats/:id/grant_read_access — at decision time, re-derives +// the root and persists it on sessions.allowed_read_paths +// +// Sam (2026-05-22 dispatch confirmation): "in the project-root resolver +// ancestor walk, stop the moment parent exits PROJECT_ROOT_WHITELIST or hits +// filesystem root — check on every iteration, not just final parent. +// Symlinked input must not be able to escape the whitelist during the +// walk." Hence the loop here checks both the walk bound AND the still- +// inside-whitelist invariant every step. + +import { access, realpath } from 'node:fs/promises'; +import { constants } from 'node:fs'; +import { dirname, isAbsolute, sep } from 'node:path'; +import type { Sql } from '../db.js'; + +// Files whose presence in a directory marks it as a repo root for grant +// purposes. Kept narrow on purpose; broader heuristics (e.g. ".project", +// "pyproject.toml") can be added with measured intent. Each entry is a +// literal basename — no globs. +const REPO_MARKERS: ReadonlyArray = [ + '.git', + 'package.json', + 'go.mod', + 'Cargo.toml', +]; + +export type GrantResolution = + | { ok: true; root: string; source: 'project' | 'whitelist' } + | { ok: false; reason: string }; + +function isUnder(child: string, parent: string): boolean { + return child === parent || child.startsWith(parent + sep); +} + +async function exists(path: string): Promise { + try { + await access(path, constants.F_OK); + return true; + } catch { + return false; + } +} + +async function isRepoShaped(dir: string): Promise { + for (const marker of REPO_MARKERS) { + if (await exists(`${dir}${sep}${marker}`)) return true; + } + return false; +} + +// Resolves an absolute path to its grant root or refuses with a reason +// string suitable for surfacing to the model. Pure helper — no DB writes, +// no broker publishes. Caller persists the root on session.allowed_read_paths +// if it wants the grant to stick. +// +// Arguments: +// sql — used only to read projects.path (no writes) +// requestedPath — absolute path the model wants to read +// projectRoot — the session's primary project root (already +// realpath'd by caller). Used to short-circuit +// "already in scope". +// whitelistRoot — PROJECT_ROOT_WHITELIST from config (default /opt). +// Walk bound for the repo-shape fallback. +// +// Returns { ok: true, root, source } on success; { ok: false, reason } else. +export async function resolveGrantRoot( + sql: Sql, + requestedPath: string, + projectRoot: string, + whitelistRoot: string, +): Promise { + if (typeof requestedPath !== 'string' || requestedPath.length === 0) { + return { ok: false, reason: 'path is required' }; + } + if (!isAbsolute(requestedPath)) { + return { ok: false, reason: 'path must be absolute' }; + } + + // Resolve symlinks so subsequent ancestor checks compare apples-to-apples + // with realpath'd projectRoot. If the path doesn't exist at all, bail + // before bothering the user — the model is asking about a phantom. + let real: string; + try { + real = await realpath(requestedPath); + } catch { + return { ok: false, reason: `path does not exist: ${requestedPath}` }; + } + + // Whitelist guard. Symlinked inputs can resolve outside the whitelist + // even when the surface-form path looks inside it; that's why we test + // the *real* path here, not the requested one. + let realWhitelist: string; + try { + realWhitelist = await realpath(whitelistRoot); + } catch { + return { ok: false, reason: `whitelist root does not exist: ${whitelistRoot}` }; + } + if (!isUnder(real, realWhitelist)) { + return { ok: false, reason: 'path outside permitted scope' }; + } + + // Already in scope? No prompt needed; the tool's caller should retry. + if (isUnder(real, projectRoot)) { + return { ok: false, reason: 'path already accessible without a grant' }; + } + + // Look for a registered project whose root is an ancestor of the + // requested path. Pick the LONGEST match (nearest ancestor wins) so + // sub-projects don't get over-broadened. + const projectRows = await sql<{ path: string }[]>` + SELECT path FROM projects WHERE status = 'open' + `; + let bestProject: string | null = null; + for (const row of projectRows) { + if (!row.path) continue; + if (!isUnder(real, row.path)) continue; + if (bestProject === null || row.path.length > bestProject.length) { + bestProject = row.path; + } + } + if (bestProject !== null) { + return { ok: true, root: bestProject, source: 'project' }; + } + + // Repo-shape fallback. Walk from the requested path upward toward the + // whitelist root. At every iteration: confirm we're still inside the + // whitelist (so a symlinked component can't slip the bound mid-walk) + // and confirm we haven't hit the filesystem root. The first dir with a + // REPO_MARKER child is the grant root. + let cursor = real; + while (true) { + // Don't grant the whitelist root itself — that would be far too broad. + if (cursor === realWhitelist) { + return { ok: false, reason: 'no repo-shaped ancestor found under whitelist' }; + } + if (!isUnder(cursor, realWhitelist)) { + return { ok: false, reason: 'path outside permitted scope' }; + } + const parent = dirname(cursor); + if (parent === cursor) { + // Hit filesystem root without finding a repo marker. + return { ok: false, reason: 'no repo-shaped ancestor found under whitelist' }; + } + if (await isRepoShaped(cursor)) { + return { ok: true, root: cursor, source: 'whitelist' }; + } + cursor = parent; + } +} diff --git a/apps/server/src/services/inference/tool-phase.ts b/apps/server/src/services/inference/tool-phase.ts index 2c9d91c..de2ef19 100644 --- a/apps/server/src/services/inference/tool-phase.ts +++ b/apps/server/src/services/inference/tool-phase.ts @@ -10,6 +10,10 @@ import { insertParts, partsFromAssistantMessage, partsFromToolMessage } from './ // dispatch layer we no longer know which format produced the call, and the // extra signal is harmless for Qwen-derived calls. import { formatUnknownToolError } from './tool-suggestions.js'; +// v1.13.17-cross-repo-reads: pre-prompt validation for request_read_access. +// Resolves the grant root before pausing the loop so the user is never +// prompted about paths we couldn't grant anyway (e.g. /etc/passwd). +import { resolveGrantRoot } from '../grant_resolver.js'; import type { InferenceContext, StreamResult, @@ -28,7 +32,8 @@ import { SYNTHESIS_TOOLS, runSynthesisPass } from '../synthesisPipeline.js'; async function executeToolCall( projectRoot: string, - toolCall: ToolCall + toolCall: ToolCall, + extraRoots: readonly string[], ): Promise<{ output: unknown; truncated: boolean; error?: string }> { const tool = TOOLS_BY_NAME[toolCall.name]; if (!tool) { @@ -63,7 +68,7 @@ async function executeToolCall( }; } try { - const output = await tool.execute(parsed.data, projectRoot); + const output = await tool.execute(parsed.data, projectRoot, extraRoots); const truncated = typeof output === 'object' && output !== null && 'truncated' in output ? Boolean((output as { truncated: unknown }).truncated) @@ -206,7 +211,71 @@ export async function executeToolPhase( ); return; } - const tres = await executeToolCall(projectRoot, tc); + // v1.13.17-cross-repo-reads: request_read_access pauses identically to + // ask_user_input EXCEPT for an up-front validation pass — if the path + // can't be granted under the whitelist / repo-shape rules, surface an + // immediate denial without prompting the user. Per design D1, we never + // ask the user about /etc/passwd or paths outside PROJECT_ROOT_WHITELIST. + if (tc.name === 'request_read_access') { + const tcArgs = tc.args as { path?: unknown; reason?: unknown }; + const requested = + typeof tcArgs.path === 'string' ? tcArgs.path : ''; + const resolution = await resolveGrantRoot( + ctx.sql, + requested, + projectRoot, + ctx.config.PROJECT_ROOT_WHITELIST, + ); + if (!resolution.ok) { + // Auto-deny without pausing. The model sees the reason on its + // next turn and decides what to do. + const stored = { + tool_call_id: tc.id, + output: `denied: ${resolution.reason}`, + truncated: false, + }; + await ctx.sql` + UPDATE messages + SET tool_results = ${ctx.sql.json(stored as never)} + WHERE id = ${toolMessageId} + `; + await insertParts( + ctx.sql, + partsFromToolMessage({ tool_results: stored }).map((p) => ({ + ...p, + message_id: toolMessageId, + })), + ); + ctx.publish(sessionId, { + type: 'tool_result', + tool_message_id: toolMessageId, + chat_id: chatId, + tool_call_id: tc.id, + output: stored.output, + truncated: false, + }); + return; + } + // Path is plausibly grantable — install the pending sentinel and + // pause. The grant endpoint re-derives the root at decision time + // (state may have changed in the meantime) so we don't stash it here. + pausingForUserInput = true; + const sentinel = { tool_call_id: tc.id, output: null, truncated: false }; + await ctx.sql` + UPDATE messages + SET tool_results = ${ctx.sql.json(sentinel as never)} + WHERE id = ${toolMessageId} + `; + await insertParts( + ctx.sql, + partsFromToolMessage({ tool_results: sentinel }).map((p) => ({ + ...p, + message_id: toolMessageId, + })), + ); + return; + } + const tres = await executeToolCall(projectRoot, tc, session.allowed_read_paths); if (SYNTHESIS_TOOLS.has(tc.name)) { synthEntries.push({ tc, output: tres.output, ...(tres.error ? { error: tres.error } : {}) }); } diff --git a/apps/server/src/services/path_guard.ts b/apps/server/src/services/path_guard.ts index c2e4c8f..6f8f6af 100644 --- a/apps/server/src/services/path_guard.ts +++ b/apps/server/src/services/path_guard.ts @@ -16,9 +16,22 @@ export async function resolveProjectRoot(projectPath: string): Promise { } } +function isUnder(real: string, root: string): boolean { + return real === root || real.startsWith(root + sep); +} + +// v1.13.17-cross-repo-reads: pathGuard now accepts an optional extraRoots +// list (typically session.allowed_read_paths). The primary projectRoot is +// tried first; if the resolved path doesn't sit under it, each extraRoot is +// tried in turn. Throws PathScopeError if no root accepts. The error message +// includes a hint pointing the model at the request_read_access tool so it +// can self-correct on the next turn — extraRoots IS the persistence +// mechanism for those grants, so we only suggest it when there's a missing +// grant to ask for (i.e. the path isn't already under any allowed root). export async function pathGuard( projectRoot: string, - requested: string + requested: string, + extraRoots: readonly string[] = [], ): Promise { if (typeof requested !== 'string' || requested.length === 0) { throw new PathScopeError('path is required'); @@ -30,10 +43,13 @@ export async function pathGuard( } catch { throw new PathScopeError(`path does not exist: ${requested}`); } - if (real !== projectRoot && !real.startsWith(projectRoot + sep)) { - throw new PathScopeError( - `path escapes project root: ${requested} -> ${real}` - ); + if (isUnder(real, projectRoot)) return real; + for (const extra of extraRoots) { + if (extra.length === 0) continue; + if (isUnder(real, extra)) return real; } - return real; + throw new PathScopeError( + `path escapes project root: ${requested} -> ${real}. ` + + `Use request_read_access(path, reason) to ask the user for permission.`, + ); } diff --git a/apps/server/src/services/request_read_access.ts b/apps/server/src/services/request_read_access.ts new file mode 100644 index 0000000..046723c --- /dev/null +++ b/apps/server/src/services/request_read_access.ts @@ -0,0 +1,82 @@ +// v1.13.17-cross-repo-reads: tool the model uses to request read access to +// a path outside its session's primary project root. When the model emits +// view_file("/opt/forks/foo/go.mod") under a session scoped to /opt/boocode, +// pathGuard's error message hints at this tool. The model then emits +// request_read_access(path="/opt/forks/foo/go.mod", +// reason="investigating foo to write the design doc") +// The tool's execute does cheap up-front validation: if the requested path +// can't possibly be granted under the current whitelist + repo-shape rules, +// it returns a denial immediately without prompting the user. Otherwise, the +// tool-phase pause branch (parallel of ask_user_input) stores a pending +// sentinel and waits for the user's allow/deny via the grant_read_access +// endpoint. +// +// The execute body never directly mutates state; the grant endpoint owns +// the persistence path. This keeps the tool-side logic side-effect-free +// (it's just a request) and matches ask_user_input's "server-side no-op +// fallback, pause happens in tool-phase" shape. + +import { z } from 'zod'; +import type { ToolDef } from './tools.js'; + +const RequestReadAccessInput = z.object({ + path: z.string().min(1), + reason: z.string().min(1).max(500), +}); +type RequestReadAccessInputT = z.infer; + +export const requestReadAccess: ToolDef = { + name: 'request_read_access', + description: + "Ask the user for read-only access to a path outside the current " + + "session's project scope. Use when a previous read tool (view_file, " + + 'list_dir, grep, find_files) was refused with a path-escapes-project ' + + 'error and the path is plausibly under another known repository (e.g. ' + + '/opt/forks/foo). Provide a short reason describing why you need the ' + + "access. Pauses the conversation until the user picks Allow or Deny; " + + 'the next assistant turn sees the result. On Allow, the tool result ' + + 'is "granted: " — subsequent reads under that root succeed for ' + + 'the rest of the session. On Deny, the tool result is "denied". Do ' + + 'not call this for paths that are already inside the project root.', + inputSchema: RequestReadAccessInput, + jsonSchema: { + type: 'function', + function: { + name: 'request_read_access', + description: + "Ask the user for read-only access to a path outside the session's " + + 'project scope. Pauses the conversation until the user picks Allow ' + + 'or Deny. Subsequent reads under the granted root succeed for the ' + + 'rest of the session.', + parameters: { + type: 'object', + properties: { + path: { + type: 'string', + description: + 'Absolute path the model wants to read. Must be under the ' + + "server's PROJECT_ROOT_WHITELIST (default /opt) and outside " + + "the session's primary project root.", + }, + reason: { + type: 'string', + description: + 'Short rationale (<=500 chars) shown to the user explaining ' + + 'why the access is needed. The user uses this to decide.', + }, + }, + required: ['path', 'reason'], + additionalProperties: false, + }, + }, + }, + // Server-side no-op. The "execution" of request_read_access is the + // pause-and-resume cycle managed by tool-phase.ts + the grant endpoint. + // The inference loop catches this tool name BEFORE executeToolCall fires + // and inserts a pending sentinel instead — this fallback only runs if + // something bypasses that branch, in which case we surface the pending + // shape so downstream code can still detect it. Mirrors ask_user_input. + async execute(input) { + return { _pending: true, path: input.path, reason: input.reason }; + }, +}; diff --git a/apps/server/src/services/tools.ts b/apps/server/src/services/tools.ts index d69f0da..918b99c 100644 --- a/apps/server/src/services/tools.ts +++ b/apps/server/src/services/tools.ts @@ -22,6 +22,10 @@ import { getSemanticNeighborhoods, getFrameworkAnalysis, } from './tools/codecontext/index.js'; +// v1.13.17-cross-repo-reads: cross-repo read grant request tool. Paired +// with the pause-on-pending-grant branch in inference/tool-phase.ts and the +// POST /api/chats/:id/grant_read_access endpoint in routes/messages.ts. +import { requestReadAccess } from './request_read_access.js'; const MAX_FILE_BYTES = 5 * 1024 * 1024; const DEFAULT_VIEW_LINES = 200; @@ -45,7 +49,13 @@ export interface ToolDef { description: string; inputSchema: z.ZodType; jsonSchema: ToolJsonSchema; - execute(input: TInput, projectRoot: string): Promise; + // v1.13.17-cross-repo-reads: extraRoots is the session's + // allowed_read_paths, threaded through executeToolCall in tool-phase.ts. + // Only the filesystem tools (view_file, list_dir, grep, find_files, + // view_truncated_output) forward it to pathGuard; other tools accept the + // arg and ignore it. The execute signature stays compatible with + // pre-v1.13.17 callsites because the parameter is optional. + execute(input: TInput, projectRoot: string, extraRoots?: readonly string[]): Promise; } const ViewFileInput = z.object({ @@ -78,14 +88,19 @@ export const viewFile: ToolDef = { }, }, }, - async execute(input, projectRoot) { - const real = await pathGuard(projectRoot, input.path); + async execute(input, projectRoot, extraRoots) { + const real = await pathGuard(projectRoot, input.path, extraRoots); // v1.11.7: secret-file deny check. Test the project-relative path // (matches the form continue.dev's patterns expect: basenames + dir // segments). Throw a typed error so executeToolCall in inference.ts // surfaces a clear "blocked" message to the LLM instead of silently // returning content the user wanted hidden. - const relPath = relative(projectRoot, real) || basename(real); + // v1.13.17: when the resolved path is outside the primary projectRoot + // (i.e. via an allowed_read_paths grant), `relative()` returns "../…" + // which won't match secret-file basename patterns. Re-anchor on the + // file's basename so the secret deny still fires across all grant roots. + const rel = relative(projectRoot, real); + const relPath = rel && !rel.startsWith('..') ? rel : basename(real); if (isSecretPath(relPath)) { throw new SecretBlockedError(relPath); } @@ -157,8 +172,8 @@ export const listDir: ToolDef = { }, }, }, - async execute(input, projectRoot) { - const real = await pathGuard(projectRoot, input.path); + async execute(input, projectRoot, extraRoots) { + const real = await pathGuard(projectRoot, input.path, extraRoots); const s = await stat(real); if (!s.isDirectory()) { throw new PathScopeError(`not a directory: ${input.path}`); @@ -264,7 +279,7 @@ export const grep: ToolDef = { }, }, }, - async execute(input, projectRoot) { + async execute(input, projectRoot, extraRoots) { const limit = Math.min( Math.max(input.max_results ?? DEFAULT_GREP_RESULTS, 1), MAX_GREP_RESULTS @@ -276,6 +291,7 @@ export const grep: ToolDef = { max_matches: limit, case_sensitive: input.case_sensitive, hidden: input.hidden, + extra_roots: extraRoots, }); const reshaped = result.matches.map((m) => ({ path: m.path, @@ -325,7 +341,7 @@ export const findFiles: ToolDef = { }, }, }, - async execute(input, projectRoot) { + async execute(input, projectRoot, extraRoots) { const limit = Math.min( Math.max(input.max_results ?? DEFAULT_FIND_RESULTS, 1), MAX_FIND_RESULTS @@ -335,6 +351,7 @@ export const findFiles: ToolDef = { const result = await fileOpsFindFiles(projectRoot, input.pattern, { path: input.path, max_results: limit, + extra_roots: extraRoots, }); // v1.11.7: drop paths matching secret patterns. The original `total` // from file_ops includes pre-truncation count; we report the visible @@ -383,7 +400,10 @@ export const viewTruncatedOutput: ToolDef = { }, }, }, - async execute(input, _projectRoot) { + // view_truncated_output doesn't touch the filesystem — it pulls from tmpfs + // by opaque id. extraRoots is irrelevant here; declared for signature parity + // with the v1.13.17 ToolDef contract. + async execute(input, _projectRoot, _extraRoots) { const content = await readTruncation(input.id); if (content === null) { return { @@ -658,6 +678,11 @@ export const ALL_TOOLS: ReadonlyArray> = [ watchChanges as ToolDef, getSemanticNeighborhoods as ToolDef, getFrameworkAnalysis as ToolDef, + // v1.13.17-cross-repo-reads: paired with the pause-on-pending-grant + // branch in tool-phase.ts. Read-only — only ever READS files; the only + // state change is appending to sessions.allowed_read_paths via the + // grant endpoint, gated by user consent. + requestReadAccess as ToolDef, ].sort((a, b) => a.name.localeCompare(b.name)); // v1.8.2: forward-compatible read-only whitelist. An agent whose `tools` is @@ -694,6 +719,10 @@ export const READ_ONLY_TOOL_NAMES = [ 'watch_changes', 'get_semantic_neighborhoods', 'get_framework_analysis', + // v1.13.17-cross-repo-reads: pauses execution but doesn't mutate project + // state directly (the grant endpoint appends to sessions.allowed_read_paths + // only with user consent). Belongs in the read-only budget tier. + 'request_read_access', ] as const; export const TOOLS_BY_NAME: Record> = Object.fromEntries( diff --git a/apps/server/src/types/api.ts b/apps/server/src/types/api.ts index de5bf9c..9b08666 100644 --- a/apps/server/src/types/api.ts +++ b/apps/server/src/types/api.ts @@ -42,6 +42,12 @@ export interface Session { // v1.12.1: server-side workspace pane layout. Replaces per-device // localStorage so all devices viewing the session see the same panes. workspace_panes: WorkspacePane[]; + // v1.13.17: absolute paths the agent has been granted read access to via + // the request_read_access tool. Empty by default; populated only by the + // grant_read_access endpoint's allow branch. Revoked via PATCH session. + // path_guard's extraRoots check consults this list before refusing reads + // outside the primary project root. + allowed_read_paths: string[]; } export type WorkspacePaneKind = 'chat' | 'terminal' | 'agent' | 'empty' | 'settings'; diff --git a/apps/web/src/api/client.ts b/apps/web/src/api/client.ts index 3b82e6d..8a1caef 100644 --- a/apps/web/src/api/client.ts +++ b/apps/web/src/api/client.ts @@ -123,7 +123,20 @@ export const api = { get: (id: string) => request(`/api/sessions/${id}`), update: ( id: string, - body: Partial> + body: Partial< + Pick< + Session, + | 'name' + | 'model' + | 'system_prompt' + | 'agent_id' + | 'web_search_enabled' + // v1.13.17: revocation path — frontend sends the shortened list + // when the user removes a grant. Grants are appended only via the + // separate grantReadAccess endpoint below. + | 'allowed_read_paths' + > + > ) => request(`/api/sessions/${id}`, { method: 'PATCH', @@ -228,6 +241,19 @@ export const api = { body: JSON.stringify({ tool_call_id: toolCallId, answers }), }, ), + // v1.13.17-cross-repo-reads: resume a paused request_read_access. On + // 'allow' the server re-resolves the grant root and appends it to + // sessions.allowed_read_paths; the returned list reflects the post- + // grant state. On 'deny' the array is unchanged. + grantReadAccess: (chatId: string, toolCallId: string, decision: 'allow' | 'deny') => + request<{ + tool_message_id: string; + assistant_message_id: string; + allowed_read_paths: string[]; + }>(`/api/chats/${chatId}/grant_read_access`, { + method: 'POST', + body: JSON.stringify({ tool_call_id: toolCallId, decision }), + }), }, messages: { diff --git a/apps/web/src/api/types.ts b/apps/web/src/api/types.ts index 9fa6378..92ae8f0 100644 --- a/apps/web/src/api/types.ts +++ b/apps/web/src/api/types.ts @@ -48,6 +48,11 @@ export interface Session { web_search_enabled: boolean | null; // v1.12.1: server-authoritative pane layout, replaces localStorage. workspace_panes: WorkspacePane[]; + // v1.13.17: paths the agent has been granted read access to via the + // request_read_access tool. Empty by default. Settings UI surfaces the + // list with per-row revoke; the grant flow itself appends through the + // dedicated POST /api/chats/:id/grant_read_access endpoint (not PATCH). + allowed_read_paths: string[]; } // v1.8.1: 'global' = /data/AGENTS.md (always-on), 'project' = per-project diff --git a/apps/web/src/components/MessageList.tsx b/apps/web/src/components/MessageList.tsx index 154fa35..6335ff6 100644 --- a/apps/web/src/components/MessageList.tsx +++ b/apps/web/src/components/MessageList.tsx @@ -4,6 +4,7 @@ import { MessageBubble } from './MessageBubble'; import { ToolCallGroup } from './ToolCallGroup'; import { ToolCallLine, type ToolRun } from './ToolCallLine'; import { AskUserInputCard } from './AskUserInputCard'; +import { RequestReadAccessCard } from './RequestReadAccessCard'; interface Props { messages: Message[]; @@ -85,7 +86,9 @@ function group(items: RenderItem[]): RenderItem[] { continue; } const name = item.run.call.name; - if (name === 'ask_user_input') { + if (name === 'ask_user_input' || name === 'request_read_access') { + // v1.13.17: same rationale as ask_user_input — grouping would collapse + // the interactive pause card into a non-actionable ToolCallLine. out.push(item); i += 1; continue; @@ -181,6 +184,16 @@ export function MessageList({ messages, sessionChats }: Props) { /> ); } + if (item.run.call.name === 'request_read_access') { + return ( + + ); + } return ; } return ; diff --git a/apps/web/src/components/RequestReadAccessCard.tsx b/apps/web/src/components/RequestReadAccessCard.tsx new file mode 100644 index 0000000..4eaebb6 --- /dev/null +++ b/apps/web/src/components/RequestReadAccessCard.tsx @@ -0,0 +1,193 @@ +import { useState } from 'react'; +import { Check, FolderOpen, ShieldOff } from 'lucide-react'; +import { toast } from 'sonner'; +import { api } from '@/api/client'; +import { Button } from '@/components/ui/button'; +import type { ToolCall, ToolResult } from '@/api/types'; + +// v1.13.17-cross-repo-reads. Renders an inline allow/deny picker for a +// paused request_read_access tool call. Mirrors AskUserInputCard's pending +// vs answered render dance: +// - Pending: server pre-stamps a sentinel tool_result with output=null. +// The card shows path + reason and lets the user pick Allow or Deny. +// - Answered: the eventual WS tool_result frame carries the actual +// decision string ("granted: " or "denied" or "denied: "). +// The card flips to a read-only summary line. +// +// Tool name discrimination lives in MessageList.flatten/group — anything +// with tc.name === 'request_read_access' bypasses grouping and renders this +// card directly. + +interface Props { + toolCall: ToolCall; + toolResult: ToolResult | null; + chatId: string; +} + +interface ParsedArgs { + path: string; + reason: string; +} + +function parseArgs(raw: unknown): ParsedArgs | null { + if (!raw || typeof raw !== 'object') return null; + const obj = raw as { path?: unknown; reason?: unknown }; + if (typeof obj.path !== 'string' || obj.path.length === 0) return null; + if (typeof obj.reason !== 'string' || obj.reason.length === 0) return null; + return { path: obj.path, reason: obj.reason }; +} + +function decisionVariant(output: unknown): 'granted' | 'denied' | 'unknown' { + if (typeof output !== 'string') return 'unknown'; + if (output.startsWith('granted:')) return 'granted'; + if (output === 'denied' || output.startsWith('denied:')) return 'denied'; + return 'unknown'; +} + +export function RequestReadAccessCard({ toolCall, toolResult, chatId }: Props) { + const args = parseArgs(toolCall.args); + + if (!args) { + return ( +
+ request_read_access: malformed tool args +
+ ); + } + + // Non-null output means the WS tool_result frame arrived (or the row was + // re-fetched from history). + const answered = toolResult && toolResult.output !== null; + if (answered) { + return ; + } + + return ; +} + +function PendingView({ + args, + toolCallId, + chatId, +}: { + args: ParsedArgs; + toolCallId: string; + chatId: string; +}) { + const [submitting, setSubmitting] = useState<'allow' | 'deny' | null>(null); + + async function decide(decision: 'allow' | 'deny') { + if (submitting) return; + setSubmitting(decision); + try { + await api.chats.grantReadAccess(chatId, toolCallId, decision); + // Card stays mounted; the incoming WS tool_result frame swaps it to + // AnsweredView via the parent prop change. + } catch (err) { + toast.error(err instanceof Error ? err.message : 'request failed'); + setSubmitting(null); + } + } + + return ( +
+
+
+ + Read-access request +
+
+
Path
+
+ {args.path} +
+
+
+
Reason
+
{args.reason}
+
+
+ Allow grants the agent read access to the matching repository root for + the rest of this session. Revoke any time from the session settings. +
+
+
+ + +
+
+ ); +} + +function AnsweredView({ args, output }: { args: ParsedArgs; output: unknown }) { + const variant = decisionVariant(output); + const text = typeof output === 'string' ? output : 'unknown'; + + return ( +
+
+
+ {variant === 'granted' ? ( + <> + + Read access granted + + ) : variant === 'denied' ? ( + <> + + Read access denied + + ) : ( + <> + + Read access request — unknown result + + )} +
+
+
Path
+
+ {args.path} +
+
+ {variant === 'granted' && ( +
+
Granted root
+
+ + {text.replace(/^granted:\s*/, '')} +
+
+ )} + {variant === 'denied' && text !== 'denied' && ( +
+ {text.replace(/^denied:\s*/, '')} +
+ )} +
+
+ ); +} diff --git a/apps/web/src/components/panes/SettingsPane.tsx b/apps/web/src/components/panes/SettingsPane.tsx index f4b6464..9f39c5a 100644 --- a/apps/web/src/components/panes/SettingsPane.tsx +++ b/apps/web/src/components/panes/SettingsPane.tsx @@ -1,5 +1,5 @@ import { useEffect, useState } from 'react'; -import { Archive, Maximize2, Minimize2, X } from 'lucide-react'; +import { Archive, FolderOpen, Maximize2, Minimize2, Trash2, X } from 'lucide-react'; import { toast } from 'sonner'; import { api } from '@/api/client'; import type { Project, Session } from '@/api/types'; @@ -269,6 +269,8 @@ function SessionSection({ session, project }: { session: Session; project: Proje

+ +