From e184de7ce742c93ca1f74b3a0b7e1c61b74a9601 Mon Sep 17 00:00:00 2001 From: oratis Date: Sun, 9 Aug 2026 22:11:43 +0800 Subject: [PATCH 1/2] fix(core): make the file contract reach where it claimed to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three places where a contract `deny` did not hold. A deny is deliberately not waivable — not even by bypassPermissions — so it is the one verdict that has to be true everywhere, and it was not. 1. A sub-agent did not inherit the contract. The Task delegation forwarded mode, permissions, hooks, sandbox and autoMode; contract was simply missed when it was added. "Never read secrets/**" bound the main agent and said nothing to the sub-agent it spawned to do the reading. Reverting the one-line fix makes the new test fail with the secret's value in the provider payload. 2. 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 — in content mode, with the matched line attached. 3. The plugin capability bridge built a ToolContext without the contract, so a plugin's Grep skipped the same filter it had just been gated by. The filter runs results through `evaluatePath`, the same function the gate uses, on a path normalized the same way. Deliberately not translated into ripgrep's --glob exclusions: an approximate second copy of the rules that drifts from the original is worse than the gap it closes. Only `deny` withholds. `ask` means "stop and ask before reading this file" and mid-search there is nobody to ask; one Grep turning into two hundred prompts is how a contract gets deleted, and a hit is not yet a read. The output ends with a count of what was withheld and never the paths — silence is worse, because an agent that finds nothing goes looking through Bash, which the contract does not reach at all. Parsing ripgrep needed --null. The default `:` separator is not reversible: `src/od:d.ts:1:hit` has three readings, and picking wrong withholds the wrong file. --null is not uniform either — in content and count modes it follows the path and newline still ends the record, but in files_with_matches it *is* the record separator and there are no newlines at all. Splitting that on '\n' yields one row carrying every path in its text, so the filter would drop nothing and still look like it worked. Verified against ripgrep 14.1.1 in all four modes; the parser is a pure function with captured fixtures so it is tested whether or not rg is installed. CI now 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 at all — and it is now what guards an output format we parse byte for byte. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 15 +++ CHANGELOG.md | 23 ++++ apps/server/src/runtime-composition.ts | 4 + docs/file-contract.md | 35 ++++++ packages/core/src/agent.test.ts | 43 +++++++ packages/core/src/agent.ts | 6 + .../core/src/config/contract-dispatch.test.ts | 68 +++++++++++ packages/core/src/config/contract-dispatch.ts | 56 +++++++++ packages/core/src/config/index.ts | 2 + packages/core/src/index.ts | 2 + packages/core/src/tools/glob.test.ts | 40 ++++++ packages/core/src/tools/glob.ts | 15 ++- packages/core/src/tools/grep.test.ts | 115 +++++++++++++++++- packages/core/src/tools/grep.ts | 78 +++++++++++- packages/core/src/types.ts | 6 + 15 files changed, 501 insertions(+), 7 deletions(-) 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..c1a8314 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,31 @@ 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 +- 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..01c0697 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,57 @@ 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: '' }, + { path: 'secrets/prod.key', text: '' }, + { path: 'b.ts', text: '' }, + ]); + }); + + it('reads count mode', () => { + expect(parseRipgrepRows('a.ts\x003\n', 'count')).toEqual([{ path: 'a.ts', text: '3' }]); + }); + + 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: '', text: '--' }]); + }); +}); + describe('GrepTool', async () => { let tmp: string; const skipReason = (await hasRipgrep()) ? null : 'ripgrep (rg) not installed'; @@ -27,6 +75,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 }); @@ -64,6 +116,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..307a58e 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,52 @@ interface GrepInput { head_limit?: number; } +/** One ripgrep output record: the path it printed, and whatever followed. */ +export interface RipgrepRow { + path: string; + text: string; +} + +/** + * 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 has no attributable path (rg's `--` context separator is + * the only one that occurs in practice) and passes through untouched. + */ +export function parseRipgrepRows(stdout: string, mode: GrepInput['output_mode']): RipgrepRow[] { + if (mode === 'files_with_matches') { + return stdout + .split('\0') + .filter(Boolean) + .map((path) => ({ path, text: '' })); + } + return stdout + .split('\n') + .filter(Boolean) + .map((raw) => { + const nul = raw.indexOf('\0'); + return nul === -1 + ? { path: '', text: raw } + : { path: raw.slice(0, nul), text: raw.slice(nul + 1) }; + }); +} + +/** Undo `--null`, reproducing rg's default output byte for byte. */ +export function formatRipgrepRow(row: RipgrepRow): string { + return row.text === '' ? row.path : `${row.path}:${row.text}`; +} + export const GrepTool: ToolHandler = { name: 'Grep', definition: { @@ -59,6 +106,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 +157,32 @@ 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. + const { kept, withheld } = withholdDeniedReads(ctx.contract, ctx.cwd, rows, (row) => row.path); + + 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; /** From 4aa9a233655a0e906ef7635b2d83c42723a0a1ae Mon Sep 17 00:00:00 2001 From: oratis Date: Sun, 9 Aug 2026 23:27:48 +0800 Subject: [PATCH 2/2] fix(grep): do not invent a separator ripgrep never printed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Give `Grep` a single *file* as its search path and ripgrep omits the filename — there is nothing to disambiguate — so its `--null` output carries no NUL at all. `formatRipgrepRow` rejoined every record as `path + ':' + text`, which for an absent path produced `:1:hit`: a stray colon in front of every line of an otherwise correct result. The round-trip was only ever verified against multi-file output. `RipgrepRow` now distinguishes "rg printed no path" from "rg printed an empty field" — both occur, and `''` is a value rg can genuinely print — so the separator is written back only where rg wrote one. That also fixes the mirror case: a content-mode match on an empty line is `path\0`, which rg prints as `path:`, not `path`. A row with no path came from a single-file search, so its file is the search root; attributing it there keeps the contract filter from depending on the pre-call gate having adjudicated that call correctly. Verified against ripgrep 14.1.1 in all four output shapes. Reverting the fix fails both new tests with `null:1:alpha hit`. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 8 +++++ packages/core/src/tools/grep.test.ts | 41 ++++++++++++++++++++--- packages/core/src/tools/grep.ts | 49 ++++++++++++++++++++++------ 3 files changed, 84 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c1a8314..58a60c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### 🐛 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. diff --git a/packages/core/src/tools/grep.test.ts b/packages/core/src/tools/grep.test.ts index 01c0697..ece8710 100644 --- a/packages/core/src/tools/grep.test.ts +++ b/packages/core/src/tools/grep.test.ts @@ -44,9 +44,9 @@ describe('parseRipgrepRows', () => { // 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: '' }, - { path: 'secrets/prod.key', text: '' }, - { path: 'b.ts', text: '' }, + { path: 'a.ts', text: null }, + { path: 'secrets/prod.key', text: null }, + { path: 'b.ts', text: null }, ]); }); @@ -54,6 +54,16 @@ describe('parseRipgrepRows', () => { 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' }]); @@ -62,7 +72,18 @@ describe('parseRipgrepRows', () => { }); it('passes through a record with no path', () => { - expect(parseRipgrepRows('--\n', 'content')).toEqual([{ path: '', text: '--' }]); + 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'); }); }); @@ -101,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(); diff --git a/packages/core/src/tools/grep.ts b/packages/core/src/tools/grep.ts index 307a58e..366ce64 100644 --- a/packages/core/src/tools/grep.ts +++ b/packages/core/src/tools/grep.ts @@ -20,10 +20,19 @@ interface GrepInput { head_limit?: number; } -/** One ripgrep output record: the path it printed, and whatever followed. */ +/** + * 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; - text: string; + path: string | null; + text: string | null; } /** @@ -40,15 +49,17 @@ export interface RipgrepRow { * 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 has no attributable path (rg's `--` context separator is - * the only one that occurs in practice) and passes through untouched. + * 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: '' })); + .map((path) => ({ path, text: null })); } return stdout .split('\n') @@ -56,14 +67,23 @@ export function parseRipgrepRows(stdout: string, mode: GrepInput['output_mode']) .map((raw) => { const nul = raw.indexOf('\0'); return nul === -1 - ? { path: '', text: raw } + ? { path: null, text: raw } : { path: raw.slice(0, nul), text: raw.slice(nul + 1) }; }); } -/** Undo `--null`, reproducing rg's default output byte for byte. */ +/** + * 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 { - return row.text === '' ? row.path : `${row.path}:${row.text}`; + if (row.path === null) return row.text ?? ''; + if (row.text === null) return row.path; + return `${row.path}:${row.text}`; } export const GrepTool: ToolHandler = { @@ -163,7 +183,16 @@ export const GrepTool: ToolHandler = { // 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. - const { kept, withheld } = withholdDeniedReads(ctx.contract, ctx.cwd, rows, (row) => row.path); + // + // 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;