Repository navigation
perf+fix: gate the never-called stage_rowset_write, and clamp a declared subagent's fs_permissions - #656
Merged
Conversation
`stage_rowset_write` was resident on every model call of every run and had
never once been invoked. It now ships withheld behind its own hyperparameter
row, `tool_surface.rowset_staging_tool`, defaulting to "off".
WHY A NEW KNOB RATHER THAN THE EXISTING ONE
`ArtifactToolFamilyExposure` is all-or-nothing over publish_artifact,
revise_artifact and stage_rowset_write. On the evidence the three do not behave
like a family at all, so flipping it would have withheld the third most-used
tool in the product to save the one nobody uses. The new knob COMPOSES with it
rather than replacing it: family "off" still withholds all three, and the
row-set row withholds exactly one.
WHAT I MEASURED
Corpus, every run store on this machine (1,441 tool_invocations rows over 323
run ids; 43,551 run-event lines over 272 sessions; 883 context_occupancy
model-call rows over 475 runs):
tool invocations run events resident on
publish_artifact 142 18,866 883/883 model calls
revise_artifact 36 3,930 883/883 model calls
stage_rowset_write 0 0 883/883 model calls
The non-zero rows are positive controls, not decoration. My first scan returned
zero for EVERY tool because it read the wrong key off the JSONL rows (they are
{"op":...,"record":{...}}, so a top-level read yields nothing and raises
nothing). That false zero read exactly like a real one. It was re-run against
the correct column with three controls before any zero was believed.
Token cost, measured through the production chain rather than estimated --
RuntimeRunHandler._stage_rowset_write_tool -> _model_visible_tools ->
wrap_tools_with_display, counted with the occupancy ledger's own
ContextTokenCounter at claude-sonnet-5:
before: 6 tools, 3,486 tok; stage_rowset_write 3,704 B / 900 tok
after : 5 tools, 2,586 tok; stage_rowset_write absent
delta : -900 estimated tokens
3,704 B / 900 tok is byte-for-byte the live context_occupancy.jsonl record.
Without the display wrap the same tool is 3,963 B / 978 tok, so the match pins
the exact object rather than landing there by rounding. publish_artifact (805)
and revise_artifact (372) are bit-identical across both surfaces, asserted as
equality rather than presence.
WHY THE ZERO IS EXPLANATION (a), NOT (b)
The tool is genuinely composed and genuinely offered -- it is not stranded by
the MCP staging-UI removal, which explicitly kept rowset staging.
tool_invocations rows are written at TOOL_CALL_STARTED, i.e. when the model
emits the call and before any gate, policy or refusal, so a call attempted and
then rejected would still have left a row. Zero rows means the model never
chose it.
THE OTHER FINDING, WHICH THIS DOES NOT FIX
REVIEWED_ROWSET_TARGETS admits exactly one connector op, ("linear",
"update_issue"), and _stage refuses every other target outright -- while the
900-token model-facing description advertises generic bulk writes and offers
"update 12 contacts" as its own example. We were paying 900 tokens per call to
advertise a general capability whose reviewed surface is a single connector op.
Gating the schema stops the rent; it does not close that gap. Recorded in
PRD-D3 and cross-referenced to docs/audit/generative-ui/FINDINGS.md, and named
as the review moment: re-open the knob alongside a widened whitelist or an
honest description, not on its own.
WHAT IS DELIBERATELY UNCHANGED
Only the model-facing schema stops being sent. The repository,
RuntimeStageLedger, WriteStager, RowsetPolicyResolver, the commit queue,
BuiltinRowSetEffectExecutor and every approval/resume path key on effect KIND,
never on tool availability, so an already-staged row set still executes.
SurfaceWriteBackCoordinator still stages row sets from user-driven write-backs
with no model involvement. Each is asserted by constructing the object, not by
importing the module.
The product consequence, stated rather than buried: this removes the lane's
only AGENT-INITIATED producer. Staged tables now arise from the user's own
write-back, never from the model proposing one. That is a decision, not a side
effect.
PROOF IT REACHED PRODUCTION, NOT JUST THE TEST SUITE
Every pre-existing test that touches this tool INJECTS it, so all of them stay
green whether or not the gate is wired to anything. Two things guard against
that "landed, not wired" shape:
- four pinned assertions went RED after the handler edit and were then
updated deliberately (test_artifact_family_tool_surface.py,
test_document.py);
- I mutated the seam back to the old family knob and re-ran: 3 of the new
tests failed, and the failure output printed 'stage_rowset_write': (3704,
900). Restored, and they pass.
TESTS
new tests/unit/runtime_worker/test_rowset_staging_tool_gate.py: 18 passed
full ai-backend suite: 10,080 passed, 3 failed, 14 skipped, 2 deselected
All 3 failures are pre-existing and reproduce identically on the pristine main
checkout (9,518 passed, same 3 failed): the two documented borrowed-venv
version pins, plus test_model_catalog's TestModelCatalogRealLitellm, which is a
full-suite-only LiteLLM cost-map collection-order issue and passes in
isolation.
ruff check + ruff format clean; tools/check_dark_capabilities.py and
tools/check_dark_wiring.py both OK.
WHAT I COULD NOT ESTABLISH
The -900 is the schema-text delta in the ledger's tokenizer units, not the
provider's own accounting of the tool block -- the sample record carries an
unattributed_delta of 6,356 between segment sum and provider_input_tokens. Per
FINDINGS section 4/7 the saving lands on COLD calls at list price, so it is
"-900 per cold prompt", never "-900 x every model call". The predicted
cold-prompt row (20,547 -> 19,647) is recorded in FINDINGS section 6 as a
PREDICTION to be scored against a real run, not as a result. And "never
invoked" is a fact about one machine's corpus, dominated by journey-harness
boots rather than human sessions; it is stated that way in every docstring
rather than as a claim about all users.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n's own boundary
`SubagentDefinition.fs_permissions` was the one capability axis with no
narrowing. Tools, skills, scopes and approval posture all intersect with the
parent through `SubagentAuthorityPolicy`; filesystem rules went straight from a
user-authored definition to the deepagents middleware untouched.
Two deepagents facts turn that into a widening device rather than a ceiling.
A child's rules are resolved as `spec.get("permissions", parent)` — a
REPLACEMENT, not an intersection — and `_check_fs_permission` answers "allow"
when no rule matches. So a definition that names one path does not add to the
boundary, it DELETES the rest of it, and the deletion is invisible because the
matcher's default is permissive.
Measured before this change, on a run with one granted folder: a definition
carrying `allow read+write+execute /**` — which `FilesystemPermissionSpec`
validates, since its only checks are "starts with /" and "no .. or ~" — arrived
at the builder as exactly that one rule, having replaced all six the supervisor
runs under, including rule 4's `interrupt` on every other read and rule 5's
`deny` on every other write.
The fix is an intersection, in the module that already owns intersections.
`SubagentAuthorityPolicy.narrow_fs_permissions` keeps a definition-owned `allow`
only for the (path, operation) pairs the parent's own ordered rule list already
allows, keeps every `deny` (a deny only tightens), and appends the parent's list
behind the survivors — which is what converts deepagents' replacement back into
an intersection, because everything the child did not name now meets the
parent's verdict instead of the matcher's permissive default. Containment is
conservative: overlap with an earlier restrictive rule refuses, only containment
in an allow rule permits, and anything it cannot reason about (a fixed-depth
`*`, an operation no parent rule mentions) is refused rather than guessed.
`parent_rules` is a required keyword on `_subagents_with_fs_permissions`. An
empty list is a real answer — the hosted images compose no host rules and the
supervisor is likewise unrestricted over a virtual-only backend — and it must
never be reachable by a caller forgetting to measure one.
BEHAVIOUR CHANGES
* A definition-owned `allow` over ground the run does not grant no longer
survives. Nothing in the repo ships such a definition: `fs_permissions` has no
producer in any service, app, fixture or seed — the only other reference is
the `packages/api-types` mirror — so this breaks no shipped flow. It can break
a hand-written `subagent_defs/<name>.json` on a desktop install, which is the
intended effect.
* A definition that names one path no longer implicitly holds everything else.
Before, `allow read+write /drafts/` also meant "and every other path is
unmatched, therefore permitted". Now `/drafts/` is granted and the rest of the
disk gets the parent's `interrupt`/`deny`. Strictly narrower, and the reason
the first change is not enough on its own.
* A write inside a granted root, under the default Manual bypass posture, now
ASKS instead of proceeding: the parent's write rule for a granted root is
`interrupt` under Manual, so a child's `allow write` there is refused and
falls through to that interrupt. Under Bypass the parent's rule is `allow` and
the child keeps it. The run's sealed bypass decision, not a definition, keeps
deciding whether a write pauses.
* `execute` never survives. Deep Agents 0.7.x types `FilesystemOperation` as
`Literal["read", "write"]` and never issues an execute check, so this is inert
today; it is refused rather than passed through because fail-closed is the
right default for an operation no parent rule can speak about.
WHAT THIS DOES NOT ESTABLISH
The clamp holds at the translation seam, which is where a definition's reach is
decided. It is NOT live enforcement today, and the tests say so rather than
implying otherwise: deepagents reads a subagent spec with dict access
(`graph.py` 648/657/664) and `SubagentDefinition` is a Pydantic model, so
`create_deep_agent` raises `AttributeError: 'SubagentDefinition' object has no
attribute 'get'` at line 657 — seven lines before `permissions` is read. Any
declared subagent, hostile or benign, with or without `fs_permissions`, fails
the build. That is a separate availability defect and is deliberately not fixed
here; fixing it without this clamp would convert the crash into a live
arbitrary-host-read. `TestDeepAgentsSpecShape` is the tripwire that fires when
someone makes definitions dict-shaped.
Also not established: I did not boot the packaged desktop, and I did not
construct a two-tenant case for the store — `FileSubagentDefinitionStore` writes
to one per-installation `subagent_defs/` directory and the declare route
discards the scoped identity it computes, which is unchanged by this commit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
FINDINGS.md conflicted where both sides edited §6's resident-tools paragraph. Took the branch's version (the tool left the list) and replaced the sentence naming JSON-schema title-slimming as the next lever with a dated correction: that lever was refuted at scoping — titles never reach the provider. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two agent-built changes that each survived adversarial review, banked together. Both sat unmerged for ~25 days.
1.
stage_rowset_writeleaves the model surface by defaultThe tool cost 900–1,223 estimated tokens of schema on every model call (900 where the
/tools/guidance route mounts; 1,223 on 601 of the 883 occupancy rows on disk) and was invoked 0 times across 323 runs. Its family siblings are heavily used over the same corpus —publish_artifact142 invocations (3rd most-used tool in the product),revise_artifact36 — so this is one dead tool inside a live lane, not a dead lane.ArtifactToolFamilyExposureis all-or-nothing over all three, and flipping it would have withheld the 3rd most-used tool. So this adds a per-tool row,tool_surface.rowset_staging_tool(default"off"), that composes with the family switch rather than replacing it. Same mechanism and same evidence shape as therun_tool_programgate in #632.Withheld, not deleted: the repository, ledger,
WriteStager, commit queue,BuiltinRowSetEffectExecutorand every approval/resume path are keyed on effect kind, never on tool availability, and the lane keeps its user-driven producer inSurfaceWriteBackCoordinator.Stated plainly: this removes the lane's only agent-initiated producer. And the verifier's caveat stands — "never invoked" is solid, "offered a usable opportunity and declined" is not: the tool's only reviewed target is
("linear", "update_issue"), and the corpus holds 4list_issuescalls out of 1,441. We were paying up to 1,223 tokens a call to advertise generic bulk writes whose reviewed surface is one connector op. Gating the schema stops the rent; it does not close that gap.Measured with the occupancy ledger's own counter, by driving the production composition chain: 6 tools / 3,486 tok → 5 tools / 2,586 tok. Siblings asserted bit-identical across both surfaces, not merely present.
2. A declared subagent's
fs_permissionsis clamped to the run's grantsfs_permissionswas translated straight into deepagentsFilesystemPermissionrules with no clamp, and deepagents replaces a subagent's permissions rather than intersecting them — somode='allow', paths=['/**'], operations=['read','write','execute']validated and survived to the graph build.Severity, honestly: not reachable by the model or by prompt injection (no first-party HTTP tool; a direct write is refused by rule 5 plus
permits_write), and not exploitable today for a less flattering reason — any declared subagent currently crashes the run build, because deepagents reads specs with dict access andSubagentDefinitionis a Pydantic model.That is exactly why this lands first. Fixing that crash without this clamp would convert a crash into a live arbitrary-host-read. The crash fix is in flight separately and is based on this branch.
Also in here
FINDINGS.md§6 named JSON-schema title-slimming as "the next lever". It is replaced with a dated correction: that lever was refuted at scoping — titles never reach the provider (bind_tools→langchain_core.utils.function_callingalready strips them). A saving measured at an observation seam is not a saving.Verification
10,903 passed. The 2 failures are the borrowed venv's version pins (deepagents 0.7.1 vs 0.7.4, langgraph 1.2.9 vs 1.2.10), which CI pins correctly. Dark-wiring 313 baselined / 0 new; dark-capabilities clean; 58 harness gates.
🤖 Generated with Claude Code