fix(core): make the file contract reach where it claimed to - #254
Conversation
598be52 to
7fa4611
Compare
Review — approve, with one bug fixedBoth gaps are real and the reasoning around them is sound. Some verification I did rather than took on trust: The The
Fixed: the round-trip is not byte-identical
Not for a single-file search. Give ripgrep one file as the search path and it prints no filename at all — there is nothing to disambiguate — so its output carries no NUL:
A stray colon in front of every line. The root cause is that While there: a row with no path came from a single-file search, so its file is the search root. Attributing it there rather than skipping it means the result filter no longer depends on the pre-call gate having adjudicated that call correctly. Same defence-in-depth argument the PR makes for filtering results at all. Reverting the fix fails both new tests with |
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
3cae1f2 to
4aa9a23
Compare
Closes the Grep/Glob gap
FLOATBOAT_ADOPTION_PLAN.md§4.1 recorded as 仍未做, plus two more found while doing it.A contract
denyis deliberately not waivable — not even bybypassPermissions— which makes it the one verdict that has to hold everywhere. It did not.1. A sub-agent did not inherit the contract
packages/core/src/agent.tsforwardsmode,permissions,hooks,sandboxConfigandautoModeinto theTasksub-agent'srunAgent.contractwas missed when it was added in #239.So
read: denyonsecrets/**bound the main agent and said nothing at all to the sub-agent it spawned to do the reading. Delegation was a way around the one gate that is not supposed to have one.Proven, not assumed — reverting the one-line fix:
The secret's value reaches the provider payload.
2.
GrepandGlobreturned denied pathsBoth take a search root, so the pre-call verdict only ever covered where the search started. A search rooted at the workspace is allowed — and then hands back matches from denied paths, in content mode with the matched line attached.
Results now run 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.read: denyread: askaskis not filtered. It means "stop and ask before reading this file" and mid-search there is nobody to ask; oneGrepbecoming two hundred prompts is how a contract gets deleted, and a hit is not yet a read. Reading the file still fires the ordinary approval.Output ends with
[2 results withheld by the file contract]— the count, never the paths. Silence is worse: an agent that searches and finds nothing goes looking throughBash, which the contract does not reach at all.3. The plugin capability bridge
apps/server/src/runtime-composition.tsgated the call throughdispatchToolCalland then built aToolContextwithout the contract, so a plugin'sGrepskipped the filter it had just been gated by.Parsing ripgrep
The default
:separator is not reversible —src/od:d.ts:1:hithas three readings, and picking wrong withholds the wrong file. Hence--null.--nullis not uniform, which nearly shipped a filter that did nothing:content,count\npath\0restfiles_with_matches\0Splitting
files_with_matchesoutput on\nyields a single row whose path is the first file and whose text carries every other path along for the ride — the filter drops nothing and looks like it worked. Verified against ripgrep 14.1.1 in all four modes.parseRipgrepRowsis a pure function with captured fixtures, so it is tested whether or notrgis installed. Output is rejoined to be byte-identical to today's.CI actually runs the Grep suite now
hasRipgrep()self-skips whenrgis absent — so the Grep tests may never have run in CI, while being what guards an output format we parse byte for byte. CI installs ripgrep and setsDC_REQUIRE_RIPGREP=1, which turns the skip into a failure.Local run confirms it: skips drop from 16 to 12 once
rgis on PATH.Verification
Notes
.envnever reaches this filter — the fixtures usesecrets/prod.key, which does. A.envfixture would have passed vacuously.limit/head_limit, so denied entries cannot eat result slots and make a search look empty.deepcode mcp serveexecutes tools with no dispatcher at all — no mode, permissions, contract or hooks. Out of scope here; tracked separately.🤖 Generated with Claude Code