diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 312617a..4e79d60 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -52,6 +52,20 @@ jobs: sudo sysctl -w kernel.apparmor_restrict_unprivileged_userns=0 || true sudo sysctl -w net.ipv4.ip_unprivileged_port_start=53 || true + # The Grep tool shells out to ripgrep and parses its `--null` output, and + # its tests self-skip when `rg` is absent. A skipped suite reads as a + # passing one, so install it rather than hope the runner image has it — + # `DC_REQUIRE_RIPGREP` then turns the skip into a failure. + - name: Install ripgrep + run: | + if command -v rg >/dev/null 2>&1; then + rg --version + elif [ "$RUNNER_OS" = "Linux" ]; then + sudo apt-get install -y ripgrep + else + brew install ripgrep + fi + - name: Typecheck run: pnpm typecheck @@ -66,6 +80,7 @@ jobs: # it self-skips on non-Linux / when bwrap/slirp4netns are absent. env: DC_SANDBOX_NET_TEST: '1' + DC_REQUIRE_RIPGREP: '1' run: pnpm test - name: Build + app-server release gate diff --git a/CHANGELOG.md b/CHANGELOG.md index 2342a33..58a60c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,39 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### 🔒 Security + +- **A sub-agent did not inherit the file contract.** The `Task` delegation + forwarded mode, permission rules, hooks, sandbox config and auto-mode — every + gate except the contract. So "never read `secrets/**`" bound the main agent + and said nothing to the sub-agent it spawned to do the reading, and since a + contract `deny` is deliberately not waivable, this was the one gate that was + supposed to hold no matter what. A regression test asserts the secret never + reaches the provider. +- **`Grep` and `Glob` returned results the contract denies reading.** Both take + a search _root_, so the pre-call verdict only ever covered where the search + started; a search rooted at the workspace was allowed and then handed back + matches from denied paths, with the matched line attached. Results are now + filtered through the same `evaluatePath` the gate uses — no second glob + dialect to drift — and the output ends with a count of what was withheld, + never with the paths. `ask` is not filtered: mid-search there is nobody to + ask, and a hit is not yet a read. +- The plugin capability bridge passed no contract into the tools it executed, so + a plugin's `Grep` skipped the same filter. + ### 🐛 Fixed +- **`Grep` over a single file no longer prefixes every line with a colon.** + ripgrep omits the filename when the search path is one _file_ — there is + nothing to disambiguate — so its `--null` output carries no NUL, and rejoining + the record as `path:text` with an absent path emitted `:1:hit`. Parsing now + distinguishes "rg printed no path" from "rg printed an empty field", and the + separator is written back only where rg wrote one. Such a row is attributed to + the search root for contract filtering, so the result filter does not depend on + the pre-call gate having adjudicated that call correctly. +- CI installs ripgrep and sets `DC_REQUIRE_RIPGREP=1`. The `Grep` suite + self-skips when `rg` is absent, so it may never have run in CI — and it now + covers ripgrep's `--null` output format, which the tool parses byte for byte. - **The test suite could re-initialise your own repository.** `git` reads `GIT_DIR` from the environment and a git hook sets it, so a fixture calling `git init` on a temp directory from inside the pre-commit gate did not diff --git a/apps/server/src/runtime-composition.ts b/apps/server/src/runtime-composition.ts index a5ffdb8..0cc65f0 100644 --- a/apps/server/src/runtime-composition.ts +++ b/apps/server/src/runtime-composition.ts @@ -359,6 +359,10 @@ export function buildPluginCapabilityBridge(options: PluginBridgeOptions): Plugi cwd: options.cwd, signal: options.signal, sandboxConfig: options.sandboxConfig, + // Grep and Glob filter their own results against the contract, so the + // bridge has to hand it over — the pre-call verdict above only covers + // the search root. + contract: options.contract, }); await options.hooks.dispatch({ event: 'PostToolUse', diff --git a/docs/file-contract.md b/docs/file-contract.md index 3ceb5ca..2c9a303 100644 --- a/docs/file-contract.md +++ b/docs/file-contract.md @@ -132,6 +132,41 @@ to skip prompts has no business clearing it — otherwise the contract's stronge sentence would also be its easiest to disable. A contract `ask` is an ordinary approval and follows the mode and hook chain like any other. +A sub-agent runs under its parent's contract. Delegation is not a way around it. + +### Search results + +`Grep` and `Glob` take a search **root**, so deciding before the call only +answers where the search starts. A search rooted at the workspace is allowed and +then returns whatever it finds — including, before this was closed, the matched +line out of a file the contract said must never be read. + +So their results are filtered afterwards, through the same rules: + +| Contract says | Grep | Glob | +| ------------- | ---------------------------------------- | ------------ | +| `read: deny` | hit removed, along with its matched line | path removed | +| `read: ask` | hit kept | path kept | +| `read: allow` | hit kept | path kept | + +`ask` is not filtered. It means "stop and ask before reading this file", and +mid-search there is nobody to ask — a single `Grep` turning into two hundred +prompts is how a contract gets deleted. A path appearing in a result listing is +not yet a read, and reading it still goes through the ordinary approval. + +When anything is withheld, the output ends with a count: + +``` +[2 results withheld by the file contract] +``` + +The count, never the paths. Staying silent would be worse than the count leaks: +an agent that searches and finds nothing goes looking through `Bash`, which the +contract does not reach at all. + +Note that ripgrep already skips hidden and `.gitignore`d files by default, so +`.env` never reaches this filter — but `secrets/prod.key` does. + ## Interaction with `settings.json` The two rule sets compose by **most-restrictive-wins**: diff --git a/packages/core/src/agent.test.ts b/packages/core/src/agent.test.ts index bfc8272..9384302 100644 --- a/packages/core/src/agent.test.ts +++ b/packages/core/src/agent.test.ts @@ -4,6 +4,7 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { runAgent as runAgentCore, type RunAgentOptions } from './agent.js'; +import { parseFileContract } from './config/file-contract.js'; import type { LedgerKind, LedgerSink, NewLedgerRecord } from './ledger/index.js'; import { HookDispatcher } from './hooks/index.js'; import { SessionManager } from './sessions/index.js'; @@ -511,6 +512,48 @@ describe('runAgent', () => { expect(provider.received).toHaveLength(3); }); + it('a sub-agent inherits the file contract', async () => { + // The delegation forwarded mode, permissions, hooks, sandbox and autoMode + // but not the contract, so "never read this path" held for the main agent + // and said nothing to the sub-agent it spawned to do the reading. Note the + // mode here is `bypassPermissions` — a contract deny is not waivable, which + // is precisely why it has to travel. + await fs.writeFile(join(cwd, 'prod.key'), 'KEY=hunter2\n'); + const provider = new MockProvider([ + toolUse('delegating', { + type: 'tool_use', + id: 'task1', + name: 'Task', + input: { prompt: 'read prod.key and tell me the value' }, + }), + toolUse('reading', { + type: 'tool_use', + id: 'r1', + name: 'Read', + input: { file_path: join(cwd, 'prod.key') }, + }), + endTurn('could not read it'), // ← sub-agent, after the block + endTurn('done'), // ← back in the top-level agent + ]); + await runAgent({ + provider, + tools: new ToolRegistry(), + systemPrompt: '', + userMessage: 'what is the key?', + model: 'deepseek-chat', + cwd, + contract: parseFileContract( + ['version: 1', 'rules:', ' - glob: "prod.key"', ' read: deny'].join('\n'), + ), + }); + + // The sub-agent's Read must have been refused, so the secret never reaches + // any message the provider was handed. + const everySentMessage = JSON.stringify(provider.received); + expect(everySentMessage).not.toContain('hunter2'); + expect(everySentMessage).toMatch(/file contract/); + }); + it('a sub-agent cannot spawn further sub-agents (depth guard)', async () => { // At subAgentDepth=1, runSubAgent is not wired, so Task fails gracefully. const provider = new MockProvider([ diff --git a/packages/core/src/agent.ts b/packages/core/src/agent.ts index be7f2e2..6a454f1 100644 --- a/packages/core/src/agent.ts +++ b/packages/core/src/agent.ts @@ -290,6 +290,7 @@ export async function runAgent(opts: RunAgentOptions): Promise { signal: opts.signal, sandboxConfig: opts.sandboxConfig, sandboxDefaultMode: opts.sandboxDefaultMode, + contract: opts.contract, sessionDir: opts.session ? `${opts.session.manager.root}/${opts.session.id}` : undefined, turnId: opts.session?.turnId, askUser: opts.askUser, @@ -377,6 +378,11 @@ export async function runAgent(opts: RunAgentOptions): Promise { signal: signal ?? opts.signal, mode: runtimePolicy.mode, permissions: runtimePolicy.permissions, + // Every gate the parent runs under has to travel with the delegation. + // A contract that stops at the Task boundary is one that says "never + // read .env" to the main agent and nothing at all to the sub-agent it + // spawns to do the reading. + contract: opts.contract, hooks: opts.hooks, sandboxConfig: opts.sandboxConfig, autoMode: opts.autoMode, diff --git a/packages/core/src/config/contract-dispatch.test.ts b/packages/core/src/config/contract-dispatch.test.ts index 6bdc068..82834ed 100644 --- a/packages/core/src/config/contract-dispatch.test.ts +++ b/packages/core/src/config/contract-dispatch.test.ts @@ -4,6 +4,8 @@ import { evaluateContract, fileContractWarnings, mostRestrictive, + withheldNotice, + withholdDeniedReads, } from './contract-dispatch.js'; import { parseFileContract, type FileContract } from './file-contract.js'; import type { PermissionVerdict } from './permissions.js'; @@ -213,3 +215,69 @@ describe('fileContractWarnings', () => { ).toEqual([]); }); }); + +// Grep and Glob take a search *root*, so the pre-call gate can only adjudicate +// where the search starts. These cover what it finds. +describe('withholdDeniedReads', () => { + const id = (p: string) => p; + + it('is the identity when there is no contract', () => { + const paths = ['src/a.ts', '.env']; + expect(withholdDeniedReads(undefined, CWD, paths, id)).toEqual({ + kept: paths, + withheld: 0, + }); + }); + + it('removes paths the contract denies reading', () => { + expect(withholdDeniedReads(secrets, CWD, ['src/a.ts', '.env', '.env.local'], id)).toEqual({ + kept: ['src/a.ts'], + withheld: 2, + }); + }); + + it('keeps `ask` paths', () => { + // There is nobody to prompt mid-search, and a hit is not yet a read. One + // Grep turning into two hundred approvals is how a contract gets deleted. + const asks = contract('rules:\n - glob: "**/*.ts"\n read: ask\n'); + expect(withholdDeniedReads(asks, CWD, ['src/a.ts'], id)).toEqual({ + kept: ['src/a.ts'], + withheld: 0, + }); + }); + + it('keeps paths outside the workspace, matching the pre-call gate', () => { + // A per-project contract has no authority over /etc, and inventing one here + // would make the filter disagree with the gate it is supposed to complete. + expect(withholdDeniedReads(secrets, CWD, ['/etc/hosts', '../sibling/.env'], id)).toEqual({ + kept: ['/etc/hosts', '../sibling/.env'], + withheld: 0, + }); + }); + + it('reads the path out of whatever shape the caller has', () => { + const rows = [ + { path: 'src/a.ts', text: 'hit' }, + { path: '.env', text: 'SECRET=hunter2' }, + ]; + const { kept, withheld } = withholdDeniedReads(secrets, CWD, rows, (r) => r.path); + expect(kept).toEqual([{ path: 'src/a.ts', text: 'hit' }]); + expect(withheld).toBe(1); + expect(JSON.stringify(kept)).not.toContain('hunter2'); + }); + + it('keeps a row with no path — it cannot be attributed, so it is not judged', () => { + expect(withholdDeniedReads(secrets, CWD, [''], id)).toEqual({ kept: [''], withheld: 0 }); + }); +}); + +describe('withheldNotice', () => { + it('says nothing when nothing was withheld', () => { + expect(withheldNotice(0)).toBeUndefined(); + }); + + it('reports the count and never the paths', () => { + expect(withheldNotice(1)).toBe('[1 result withheld by the file contract]'); + expect(withheldNotice(3)).toBe('[3 results withheld by the file contract]'); + }); +}); diff --git a/packages/core/src/config/contract-dispatch.ts b/packages/core/src/config/contract-dispatch.ts index beddae1..ac75263 100644 --- a/packages/core/src/config/contract-dispatch.ts +++ b/packages/core/src/config/contract-dispatch.ts @@ -63,6 +63,62 @@ export function contractGovernedTools(): string[] { return Object.keys(TOOL_AXIS); } +export interface WithheldResults { + kept: T[]; + /** How many entries the contract removed. Reported, never itemised. */ + withheld: number; +} + +/** + * Remove search results whose path the contract denies reading. + * + * Grep and Glob take a *search root*, so the pre-call gate can only adjudicate + * where the search starts — not what it finds. A search rooted at the workspace + * is allowed, and then returns `.env` among its hits, with the matched line + * attached. The pre-call verdict was correct and the outcome still contradicts + * the contract; the gap is that the tool produces paths nobody asked about. + * + * The decision runs through `evaluatePath`, the same function the gate uses, on + * a path normalized the same way. There is deliberately no second glob dialect + * and no translation into ripgrep's exclusion syntax: an approximate copy of the + * rules that diverges from the original is worse than the gap it closes. + * + * Only `deny` withholds. `ask` means "stop and ask before reading this file" and + * there is nobody to ask mid-search — turning one Grep into two hundred prompts + * would get the contract deleted, and a path in a result listing is not yet a + * read. Read the file and the ordinary `ask` still fires. + * + * A path outside the workspace is kept, matching the gate: a per-project + * contract has no authority over `/etc`. + */ +export function withholdDeniedReads( + contract: FileContract | undefined, + cwd: string, + items: T[], + pathOf: (item: T) => string, +): WithheldResults { + if (!contract) return { kept: items, withheld: 0 }; + + const kept: T[] = []; + let withheld = 0; + for (const item of items) { + const raw = pathOf(item); + const path = raw ? normalizeContractPath(cwd, raw) : null; + if (path !== null && evaluatePath(contract, { path, action: 'read' }).verdict === 'deny') { + withheld++; + continue; + } + kept.push(item); + } + return { kept, withheld }; +} + +/** One line naming how much was withheld, without naming any of it. */ +export function withheldNotice(withheld: number): string | undefined { + if (withheld <= 0) return undefined; + return `[${withheld} result${withheld === 1 ? '' : 's'} withheld by the file contract]`; +} + const SEVERITY: Record = { 'no-match': 0, allow: 1, diff --git a/packages/core/src/config/index.ts b/packages/core/src/config/index.ts index abac390..252425f 100644 --- a/packages/core/src/config/index.ts +++ b/packages/core/src/config/index.ts @@ -99,6 +99,8 @@ export { evaluateContract, fileContractWarnings, mostRestrictive, + withheldNotice, + withholdDeniedReads, type ContractDispatchRequest, type ContractWarningInput, } from './contract-dispatch.js'; diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 2df133b..3ac88ff 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -149,6 +149,8 @@ export { evaluateContract, fileContractWarnings, mostRestrictive, + withheldNotice, + withholdDeniedReads, } from './config/index.js'; // Credentials (M2; M3c adds ApiKeyHelperRefresher) diff --git a/packages/core/src/tools/glob.test.ts b/packages/core/src/tools/glob.test.ts index 5f1a5dc..9e3e756 100644 --- a/packages/core/src/tools/glob.test.ts +++ b/packages/core/src/tools/glob.test.ts @@ -3,6 +3,7 @@ import { mkdtemp, rm } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { parseFileContract } from '../config/file-contract.js'; import { GlobTool } from './glob.js'; describe('GlobTool', () => { @@ -48,4 +49,43 @@ describe('GlobTool', () => { const r = await GlobTool.execute({}, { cwd: tmp }); expect(r.isError).toBe(true); }); + + describe('file contract', () => { + const denySecrets = parseFileContract( + ['version: 1', 'rules:', ' - glob: "src/nested/**"', ' read: deny'].join('\n'), + ); + + it('withholds denied paths from the listing', async () => { + const r = await GlobTool.execute( + { pattern: '**/*.ts', path: tmp }, + { cwd: tmp, contract: denySecrets }, + ); + expect(r.content).toMatch(/a\.ts/); + expect(r.content).not.toMatch(/c\.ts/); + expect(r.content).toMatch(/1 result withheld by the file contract/); + expect(r.data?.withheld).toBe(1); + }); + + it('leaves the listing alone when nothing is denied', async () => { + const withContract = await GlobTool.execute( + { pattern: '**/*.ts', path: tmp }, + { cwd: tmp, contract: parseFileContract('version: 1\n') }, + ); + const without = await GlobTool.execute({ pattern: '**/*.ts', path: tmp }, { cwd: tmp }); + expect(withContract.content).toBe(without.content); + expect(withContract.data?.withheld).toBeUndefined(); + }); + + it('does not count denied paths against the limit', async () => { + // Filtering after truncation would let denied entries eat result slots, + // so a search could come back empty while matches existed. + const r = await GlobTool.execute( + { pattern: '**/*.ts', path: tmp, limit: 2 }, + { cwd: tmp, contract: denySecrets }, + ); + const paths = (r.content as string).split('\n').filter((l) => l.endsWith('.ts')); + expect(paths).toHaveLength(2); + expect(paths.every((p) => !p.includes('nested'))).toBe(true); + }); + }); }); diff --git a/packages/core/src/tools/glob.ts b/packages/core/src/tools/glob.ts index 2e9dd07..9b996ef 100644 --- a/packages/core/src/tools/glob.ts +++ b/packages/core/src/tools/glob.ts @@ -3,6 +3,7 @@ import { glob } from 'node:fs/promises'; import { isAbsolute, relative, resolve } from 'node:path'; +import { withheldNotice, withholdDeniedReads } from '../config/contract-dispatch.js'; import type { ToolContext, ToolHandler, ToolResult } from '../types.js'; interface GlobInput { @@ -54,7 +55,14 @@ export const GlobTool: ToolHandler = { } // Convert to absolute, dedupe - const abs = [...new Set(matches.filter(Boolean).map((p) => resolve(searchPath, p)))]; + const found = [...new Set(matches.filter(Boolean).map((p) => resolve(searchPath, p)))]; + + // The pre-call gate adjudicated the search root, not the hits. A listing + // exposes only names, not contents — but a contract that denies reading a + // path and then enumerates it has still told the agent the file is there + // and what it is called, which is most of what a name-based secret gives + // away. Filter before the mtime stat, so a denied path is not even opened. + const { kept: abs, withheld } = withholdDeniedReads(ctx.contract, ctx.cwd, found, (p) => p); // Sort by mtime descending (best-effort; skip stat errors) const { promises: fs } = await import('node:fs'); @@ -75,9 +83,12 @@ export const GlobTool: ToolHandler = { const lines = top.map((s) => relative(ctx.cwd, s.p) || s.p); if (truncated) lines.push(`... [${top.length} of ${stamped.length}]`); + const notice = withheldNotice(withheld); + if (notice) lines.push(notice); + return { content: lines.join('\n') || '(no matches)', - data: { count: top.length, total: stamped.length }, + data: { count: top.length, total: stamped.length, ...(withheld > 0 ? { withheld } : {}) }, }; }, }; diff --git a/packages/core/src/tools/grep.test.ts b/packages/core/src/tools/grep.test.ts index 365d476..ece8710 100644 --- a/packages/core/src/tools/grep.test.ts +++ b/packages/core/src/tools/grep.test.ts @@ -5,7 +5,8 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { promisify } from 'node:util'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; -import { GrepTool } from './grep.js'; +import { parseFileContract } from '../config/file-contract.js'; +import { formatRipgrepRow, GrepTool, parseRipgrepRows } from './grep.js'; const execFileAsync = promisify(execFile); @@ -14,10 +15,78 @@ async function hasRipgrep(): Promise { await execFileAsync('rg', ['--version']); return true; } catch { + // Skipping locally is a convenience; skipping in CI is a green suite that + // tested nothing. The Grep tool parses ripgrep's `--null` output byte for + // byte, so an environment without it must say so rather than pass. + if (process.env.DC_REQUIRE_RIPGREP === '1') { + throw new Error('DC_REQUIRE_RIPGREP=1 but ripgrep (rg) is not on PATH'); + } return false; } } +// Captured from ripgrep 14.1.1. These run without ripgrep installed, which is +// the point: the integration tests below self-skip when `rg` is missing, and a +// parser that only runs when a binary happens to be present is a parser nobody +// is testing. +describe('parseRipgrepRows', () => { + it('reads content mode, where NUL follows the path and newline ends the record', () => { + const out = 'a.ts\x001:hit\nsecrets/prod.key\x001:KEY=hunter2\n'; + expect(parseRipgrepRows(out, 'content')).toEqual([ + { path: 'a.ts', text: '1:hit' }, + { path: 'secrets/prod.key', text: '1:KEY=hunter2' }, + ]); + }); + + it('reads files_with_matches, where NUL *is* the record separator', () => { + // No newlines at all. Splitting this on '\n' yields one row whose path is + // `a.ts` and whose text carries every other path along for the ride — a + // filter would drop nothing and still look like it worked. + const out = 'a.ts\x00secrets/prod.key\x00b.ts\x00'; + expect(parseRipgrepRows(out, 'files_with_matches')).toEqual([ + { path: 'a.ts', text: null }, + { path: 'secrets/prod.key', text: null }, + { path: 'b.ts', text: null }, + ]); + }); + + it('reads count mode', () => { + expect(parseRipgrepRows('a.ts\x003\n', 'count')).toEqual([{ path: 'a.ts', text: '3' }]); + }); + + it('distinguishes an empty match from a path printed alone', () => { + // `rg '^$'` in content mode without -n prints `path\0` — the path, the NUL, + // and nothing. That is not the same record as files_with_matches' `path\0`, + // and rg writes a trailing separator for one and not the other. + expect(formatRipgrepRow(parseRipgrepRows('blank.txt\x00\n', 'content')[0]!)).toBe('blank.txt:'); + expect(formatRipgrepRow(parseRipgrepRows('blank.txt\x00', 'files_with_matches')[0]!)).toBe( + 'blank.txt', + ); + }); + + it('handles a path containing a colon, which is why NUL is needed', () => { + const rows = parseRipgrepRows('src/od:d.ts\x001:hit\n', 'content'); + expect(rows).toEqual([{ path: 'src/od:d.ts', text: '1:hit' }]); + // And round-trips to exactly what rg would have printed without --null. + expect(formatRipgrepRow(rows[0]!)).toBe('src/od:d.ts:1:hit'); + }); + + it('passes through a record with no path', () => { + expect(parseRipgrepRows('--\n', 'content')).toEqual([{ path: null, text: '--' }]); + }); + + it('does not invent a separator rg never printed', () => { + // Give rg one *file* as the search path and it prints no filename at all — + // there is nothing to disambiguate. Rejoining `path + ':' + text` with an + // absent path prepends a colon to every line: `:1:alpha hit`. + const rows = parseRipgrepRows('1:alpha hit\n', 'content'); + expect(rows).toEqual([{ path: null, text: '1:alpha hit' }]); + expect(formatRipgrepRow(rows[0]!)).toBe('1:alpha hit'); + // count mode over a single file is the same shape: a bare number. + expect(formatRipgrepRow(parseRipgrepRows('1\n', 'count')[0]!)).toBe('1'); + }); +}); + describe('GrepTool', async () => { let tmp: string; const skipReason = (await hasRipgrep()) ? null : 'ripgrep (rg) not installed'; @@ -27,6 +96,10 @@ describe('GrepTool', async () => { await fs.writeFile(join(tmp, 'a.ts'), 'function verifyToken() {}\n'); await fs.writeFile(join(tmp, 'b.ts'), 'verifyToken(); // call site\n'); await fs.writeFile(join(tmp, 'c.md'), 'verifyToken is documented here\n'); + // Not a dotfile: ripgrep skips hidden and gitignored paths by default, so a + // `.env` fixture would pass without the filter doing anything at all. + await fs.mkdir(join(tmp, 'secrets'), { recursive: true }); + await fs.writeFile(join(tmp, 'secrets', 'prod.key'), 'KEY=hunter2 verifyToken\n'); }); afterAll(async () => { if (tmp) await rm(tmp, { recursive: true, force: true }); @@ -49,6 +122,18 @@ describe('GrepTool', async () => { expect(r.content).not.toMatch(/c\.md/); }); + it.skipIf(skipReason)('searching one file prints the line, not `:line`', async () => { + // rg omits the filename when the search path is a single file, so there is + // no NUL in its output. Rejoining as `path:text` with an absent path put a + // stray colon in front of every line of an otherwise correct result. + const r = await GrepTool.execute( + { pattern: 'verifyToken', path: join(tmp, 'a.ts'), '-n': true }, + { cwd: tmp }, + ); + expect(r.isError).toBeFalsy(); + expect(r.content).toBe('1:function verifyToken() {}'); + }); + it.skipIf(skipReason)('returns (no matches) on miss', async () => { const r = await GrepTool.execute({ pattern: 'doesNotExist_xyzabc', path: tmp }, { cwd: tmp }); expect(r.isError).toBeFalsy(); @@ -64,6 +149,67 @@ describe('GrepTool', async () => { expect(r.data?.mode).toBe('files_with_matches'); }); + // The pre-call gate adjudicates the *search root*. A search rooted at the + // workspace is allowed, and then hands back the contents of every file it + // matched — including the ones the contract says must never be read. + describe('file contract', () => { + const contract = (decision: string) => + parseFileContract( + ['version: 1', 'rules:', ' - glob: "secrets/**"', ` read: ${decision}`].join('\n'), + ); + + it.skipIf(skipReason)('withholds matches from a denied path, with their content', async () => { + const r = await GrepTool.execute( + { pattern: 'verifyToken', path: tmp }, + { cwd: tmp, contract: contract('deny') }, + ); + expect(r.content).toMatch(/a\.ts/); + expect(r.content).not.toMatch(/prod\.key/); + expect(r.content).not.toMatch(/hunter2/); // the secret itself + expect(r.content).toMatch(/1 result withheld by the file contract/); + expect(r.data?.withheld).toBe(1); + }); + + it.skipIf(skipReason)('withholds in files_with_matches mode too', async () => { + const r = await GrepTool.execute( + { pattern: 'verifyToken', path: tmp, output_mode: 'files_with_matches' }, + { cwd: tmp, contract: contract('deny') }, + ); + expect(r.content).toMatch(/a\.ts/); + expect(r.content).not.toMatch(/prod\.key/); + expect(r.data?.withheld).toBe(1); + }); + + it.skipIf(skipReason)('leaves `ask` results alone', async () => { + const r = await GrepTool.execute( + { pattern: 'verifyToken', path: tmp }, + { cwd: tmp, contract: contract('ask') }, + ); + // Nobody can be prompted mid-search, and a search is not a read of every + // hit. Reading the file still goes through the ordinary approval. + expect(r.content).toMatch(/prod\.key/); + expect(r.data?.withheld).toBeUndefined(); + }); + + it.skipIf(skipReason)('emits the same output as before when nothing is denied', async () => { + const withContract = await GrepTool.execute( + { pattern: 'verifyToken', path: tmp, '-n': true }, + { cwd: tmp, contract: parseFileContract('version: 1\n') }, + ); + const without = await GrepTool.execute( + { pattern: 'verifyToken', path: tmp, '-n': true }, + { cwd: tmp }, + ); + // Compared as sets: ripgrep searches in parallel and does not promise an + // order, so two runs of the same query legitimately differ in sequence. + const lines = (r: typeof without) => (r.content as string).split('\n').sort(); + expect(lines(withContract)).toEqual(lines(without)); + // `--null` is an implementation detail of parsing, never of the output. + expect(without.content).not.toContain('\0'); + expect(without.content).toMatch(/a\.ts:1:function verifyToken/); + }); + }); + if (skipReason) { it('skipped: ripgrep not available', () => { expect(skipReason).toMatch(/ripgrep/); diff --git a/packages/core/src/tools/grep.ts b/packages/core/src/tools/grep.ts index 06f9161..366ce64 100644 --- a/packages/core/src/tools/grep.ts +++ b/packages/core/src/tools/grep.ts @@ -4,6 +4,7 @@ import { execFile } from 'node:child_process'; import { promisify } from 'node:util'; import { isAbsolute, resolve } from 'node:path'; +import { withheldNotice, withholdDeniedReads } from '../config/contract-dispatch.js'; import type { ToolContext, ToolHandler, ToolResult } from '../types.js'; const execFileAsync = promisify(execFile); @@ -19,6 +20,72 @@ interface GrepInput { head_limit?: number; } +/** + * One ripgrep output record: the path it printed, and whatever followed. + * + * Both halves are nullable because rg genuinely omits either one, and `''` is a + * value it can also print. `path: null` means rg printed no filename — it does + * that when the search path is a single *file*, since there is nothing to + * disambiguate. `text: null` means it printed the path alone, as + * `files_with_matches` does. Collapsing either onto `''` loses the distinction + * between "absent" and "empty", and the separator is reinstated from it. + */ +export interface RipgrepRow { + path: string | null; + text: string | null; +} + +/** + * Split `--null` output back into (path, rest) pairs. + * + * `--null` does not mean one thing. In `content` and `count` modes it puts a NUL + * *after the path* and still ends each record with a newline. In + * `files_with_matches` it uses the NUL as the record terminator and emits no + * newlines at all — so splitting that output on '\n' yields a single line with + * every path concatenated, and a filter reading only its first field would drop + * nothing while appearing to work. + * + * The reason for `--null` at all is that the default `:` separator is not + * reversible: `src/od:d.ts:1:hit` has three plausible readings, and picking + * wrong means withholding the wrong file — or failing to withhold the right one. + * + * A record with no NUL is one rg printed without a filename: either the search + * path was a single file, or it is rg's `--` context separator. Neither has an + * attributable path, so both come back as `path: null` and pass through. + */ +export function parseRipgrepRows(stdout: string, mode: GrepInput['output_mode']): RipgrepRow[] { + if (mode === 'files_with_matches') { + // NUL terminates the record and nothing follows the path. + return stdout + .split('\0') + .filter(Boolean) + .map((path) => ({ path, text: null })); + } + return stdout + .split('\n') + .filter(Boolean) + .map((raw) => { + const nul = raw.indexOf('\0'); + return nul === -1 + ? { path: null, text: raw } + : { path: raw.slice(0, nul), text: raw.slice(nul + 1) }; + }); +} + +/** + * Undo `--null`, reproducing rg's default output byte for byte. + * + * The separator is written back only where rg wrote one. Give a single file as + * the search path and rg prints `1:hit` with no filename at all — emitting + * `path + ':' + text` there would prepend a colon to every line of a result set + * that was previously correct. + */ +export function formatRipgrepRow(row: RipgrepRow): string { + if (row.path === null) return row.text ?? ''; + if (row.text === null) return row.path; + return `${row.path}:${row.text}`; +} + export const GrepTool: ToolHandler = { name: 'Grep', definition: { @@ -59,6 +126,14 @@ export const GrepTool: ToolHandler = { const args: string[] = []; args.push('--color=never'); args.push('--max-columns=500'); + // `--null` puts a NUL after every printed path, in every output mode. The + // default `:` separator cannot be parsed back: a path may contain a colon, + // so `a:b:c` is ambiguous and guessing wrong would withhold the wrong line + // — or fail to withhold the right one. `--no-heading` pins the shape rather + // than relying on rg's TTY detection. Both are undone before returning, so + // the visible output is byte-identical to before. + args.push('--null'); + args.push('--no-heading'); if (input['-i']) args.push('-i'); if (input.type) args.push('--type', input.type); if (input.glob) args.push('--glob', input.glob); @@ -102,17 +177,41 @@ export const GrepTool: ToolHandler = { }; } - let lines = stdout.split('\n').filter(Boolean); + const rows = parseRipgrepRows(stdout, mode); + + // The pre-call gate adjudicated the search root. It could not adjudicate + // what the search found — and in content mode a hit carries the matched + // line, so an unfiltered result set hands over the contents of a file the + // contract says must not be read. + // + // A row with no path came from a single-file search, so the file it came + // from is the search root. Attributing it there rather than skipping it + // keeps the filter independent of the gate having got that call right. + const { kept, withheld } = withholdDeniedReads( + ctx.contract, + ctx.cwd, + rows, + (row) => row.path ?? searchPath, + ); + + let lines = kept.map(formatRipgrepRow); + const matched = lines.length; + if (input.head_limit && input.head_limit > 0) { const truncated = lines.length > input.head_limit; lines = lines.slice(0, input.head_limit); - if (truncated) - lines.push(`... [${lines.length} of ${stdout.split('\n').filter(Boolean).length}]`); + if (truncated) lines.push(`... [${lines.length} of ${matched}]`); } + // Say that something was withheld, never what. Silence is worse than the + // count: an agent that finds nothing goes looking through Bash, which the + // contract does not reach at all. + const notice = withheldNotice(withheld); + if (notice) lines.push(notice); + return { content: lines.join('\n') || '(no matches)', - data: { mode, matches: lines.length }, + data: { mode, matches: matched, ...(withheld > 0 ? { withheld } : {}) }, }; }, }; diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index f0c3fc7..40bcee4 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -118,6 +118,12 @@ export interface ToolContext { signal?: AbortSignal; /** Optional platform sandbox config — passed through to Bash tool (M3.5). */ sandboxConfig?: import('./config/types.js').SandboxConfig; + /** + * Path-axis rules, for tools that emit paths the pre-call gate never saw. + * Grep and Glob adjudicate their search *root* before running and their + * *results* after; everything else is decided entirely by the dispatcher. + */ + contract?: import('./config/file-contract.js').FileContract; /** Sandbox mode used when `sandboxConfig` names none (hosts: workspace-write). */ sandboxDefaultMode?: import('./config/types.js').SandboxMode; /**