approvals: name every gated tool for the operator, and close the seam that hid it - #728
approvals: name every gated tool for the operator, and close the seam that hid it#728M3gA-Mind wants to merge 1 commit into
Conversation
Eleven tools the runtime classifies as `Reach::Consequence` had no entry in the console's label maps, so an approval card for any of them read the generic "Use one of its tools" and an operator was asked to approve something unnamed. `publish_artifact` and `run_workflow` are among the most consequential things an agent can do; `curl` and `http_request` are unbounded outward reach. The labels are the visible half. The lasting half is that the drift was silent by construction: the declarations are Rust, the labels are TypeScript, and no build step compared them -- `workspace_create` sat mislabelled from tinyhumansai#551 until now with every lane green. `consequence_tools()` generates `frontend/src/lib/gated-tools.generated.ts` from the typed table, a Rust test fails if the committed copy drifts, and a frontend test asserts every name in it resolves to real words. Both halves fail closed: each asserts its own input is non-empty and names anchors, so a broken extraction is loud rather than green. Reading the typed table rather than parsing its source text is deliberate. A script over `DECLARED` would rot on the first formatting change and fail open -- and it undercounts today: the reproduction in the issue misses every `d_grantable(` and multi-line entry, which is why tinyhumansai#706 reports six missing tools where there are eleven.
|
@coderabbitai review |
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR generates the frontend gated-tool list from Rust consequence declarations, adds labels for previously unlabeled tools, exports the generic fallback string, and adds Rust and frontend validation for coverage, population, and label uniqueness. ChangesGated tool label coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
The best thing here is the paragraph about the issue's own reproduction script — it matched 38 of the table's 50 entries, missing all five d_grantable( declarations and seven rustfmt-wrapped ones, then reported the smaller answer without a word. "That is the fail-open defect this issue is about, committed by the script written to demonstrate it." A regex that matches nothing and one that matches everything produce the same shape of output, so a source-text checker cannot report its own blind spots. Finding that while fixing the thing it under-reported, and correcting six to eleven, is the whole argument for generating from the typed table.
And the loop is genuinely closed at both ends — I checked rather than assumed:
gated_tool_snapshot_matchesis a non-ignored test thatinclude_str!s the checked-ingated-tools.generated.tsand compares it againstgenerated_gated_tools_ts(). Adding aConsequencetool to the Rust table without regenerating fails theRustlane. That is the drift seam shut.the_gated_tool_list_is_never_silently_emptyguards the generator against returning nothing through a formatting change — the repro script's failure mode, applied to the replacement. That is the right lesson to draw from it.gated-tool-labels.test.tscloses the other half, so a generated name with no label fails the frontend lane.
The wording discipline is also right: web_fetch staying "a web page" while curl and http_request say "any address" matters precisely because the Standing permissions list renders these with no payload underneath, so two labels that read alike are two permissions an operator cannot tell apart. Enforcing distinctness with a test rather than in review is the correct call for copy that is load-bearing.
0 major. 1 question, and it is the same shape as the bug. Approving.
Question — consequence_tools() enumerates DECLARED; declared_tools() enumerates DECLARED plus COMPOSIO_EXECUTE
pub fn consequence_tools() -> Vec<&'static str> {
let mut tools: Vec<&'static str> = DECLARED
.iter()
.filter(|d| d.reach == Reach::Consequence)
...composio_execute is not in DECLARED — it is chained on separately by declared_tools(), because its consequence is a property of the action in its arguments rather than of its name. So the generated list does not contain it, and the crate now holds two enumerations of "every tool" that disagree by exactly one entry, with the new one narrower.
This may well be correct: a Composio approval card presumably names the action rather than the tool, so composio_execute may never need a toolAction label. If so, the exclusion deserves a sentence at consequence_tools() saying why it does not chain what its sibling chains — because the failure mode you have just spent this PR closing is precisely "an entry that lives outside the array nobody thought to look past".
If it is not correct, then a Composio send — one of the more consequential things in the product — is the twelfth tool falling through to "Use one of its tools", and the generator would be reproducing the structural blind spot rather than the textual one. Worth confirming which, and pinning it either way: a test asserting the two enumerations differ only by COMPOSIO_EXECUTE, with the reason, would make the divergence deliberate instead of incidental.
Before merging: base is current and the branch is CLEAN.
|
Closing this as superseded by #721, which shipped the same fix first and better. #721 ("name the eleven gated tools on the approval card", issue #701) landed Three specific concessions, because "duplicate" undersells it — two of its
The design objection in One thing here is not covered by #721 and may be worth a small follow-up: its Verified independently against Branch kept for now; no code from this PR should land. |
2 participants
Summary
Closes #706.
Eleven tools the runtime classifies as
Reach::Consequencehad no entry in theconsole's label maps.
Consequenceis the reach that parks under the defaultsupervisedmode, so every one of them reaches an operator on an approval card— and
toolActionfell through to the literal "Use one of its tools", whichnames nothing.
publish_artifactandrun_workfloware among the mostconsequential things an agent can do;
curlandhttp_requestare unboundedoutward reach.
file_writeeditapply_patchcsv_exportread_workspace_statehttp_requestcurlgit_operationsmcp_call_toolrun_workflowpublish_artifactThe wordings are deliberately distinct from each other and from the existing
entries. The Standing permissions list (#374) renders these labels with no
payload block underneath, so two tools that read alike are two permissions an
operator cannot tell apart — which is why
web_fetchstays "a web page" whilecurlandhttp_requestsay "any address". A test enforces that distinctnessrather than leaving it to review.
It is eleven, not six — and the undercount is the bug in miniature
Corrected figures, read from the typed table rather than from its source text:
ConsequencetoolsEFFECT_LABELScomposio_authorize,mcp_registry_tool_call,media_generate_image,media_generate_videoTOOL_LABELSThe issue reported six of twelve. The reproduction script filed with it matched
38 of the table's 50 entries: it missed all 5
d_grantable(declarations and 7 entries
rustfmthad wrapped across lines — thenreported the smaller answer without a word. The five extra misses it hid —
file_write,edit,apply_patch,csv_export,read_workspace_state— areprecisely the ones a line-oriented scan cannot see.
That is the fail-open defect this issue is about, committed by the script
written to demonstrate it. A checker that parses the table's source text
cannot be trusted to report its own blind spots, because a regex that matches
nothing and a regex that matches everything produce the same shape of output: a
list, printed confidently. It is the single strongest argument for the design
below, which never looks at source text at all.
The fix that lasts: close the seam, do not patch the drift
The declarations are Rust, the labels are TypeScript, and nothing in either
build compared them.
workspace_createsat rendering the generic fallback fromissue #551 until now with every lane green.
Two halves, both reading typed data rather than text:
consequence_tools()(src/policy/consequence.rs) filtersDECLAREDonreach == Reach::Consequenceand generatesfrontend/src/lib/gated-tools.generated.ts. A Rust test(
gated_tool_snapshot_matches) fails if the committed copy drifts, mirroringthe existing
sdl_snapshot_matchespattern forschema.graphql. Regeneratewith
cargo test -- --ignored regenerate_gated_tools.gated-tool-labels.test.tsimports that generated list and asserts everyname resolves through the real
toolActionresolver to something other thanthe fallback.
They compose: Rust proves the list is current, TypeScript proves the list is
named. Adding a
Reach::Consequencetool without a label fails the second;changing a
Reachwithout regenerating fails the first.Asserting through
toolActionrather than against the label maps is deliberate.The maps are module-private, and the property that matters is not "a key exists"
but "an operator sees words" — routing through the real resolver also covers the
EFFECT_LABELS-first ordering, which is howcomposio_authorizeandmcp_registry_tool_callare legitimately named without appearing inTOOL_LABELSat all.Both halves fail closed
A check whose own input silently arrives empty reports success having compared
nothing, which is the same shape as the defect being fixed. So each half asserts
its own input is populated (
> 10) and contains named anchors(
publish_artifact,run_workflow,shell), rather than trusting that anon-empty list arrived.
the_gated_tool_list_is_never_silently_emptyandis not vacuous: the generated list is populatedexist only to fail when theextraction breaks.
composio_executeis deliberately outside all of this, as it is outsideDECLAREDitself: its reach is read from the action slug in its arguments, sothere is no single answer to snapshot.
EFFECT_LABELSnames it.i18n: reported rather than changed
These labels are raw English string literals, and this PR keeps them that
way. There is no i18n infrastructure in this repository — no
useT, noen.ts, no locale directory anywhere underfrontend/, and no em-dash lane inci.yml.TOOL_LABELSandEFFECT_LABELShave always been plain literals inlanguage.ts. Introducing a translation layer for eleven strings would be aseparate change with its own argument, so the existing pattern is followed and
flagged here instead of being altered silently. No label value contains U+2014.
API Or Behavior Changes
instead of reading "Use one of its tools". No gating behaviour changes — this
is presentation only; nothing about what parks, or when, is touched.
policy::consequence::consequence_tools()andgenerated_gated_tools_ts().frontend/src/lib/gated-tools.generated.ts, committedand drift-checked. Regenerate with
cargo test -- --ignored regenerate_gated_tools.UNNAMED_TOOL_ACTIONfromlanguage.ts, so the test comparesagainst the same string the console renders rather than restating it.
Tests
cargo fmt --all -- --check— passed.cargo clippy --locked --no-deps --features openhuman,tinycortex --all-targets -- -D warnings— passed, zero lints.cargo build --all-targets --features openhuman,tinycortex— passed. Alsocargo check --locked --all-features --all-targets.cargo test --features openhuman,tinycortex— 3130 passed, 0 failed, 4 ignored (lib), plus 10 / 1 / 11 / 2 / 0 across the other targets.Frontend, on Node 22 to match the
Consolejob:npm ci, thennpm run typecheck,typecheck:e2e,typecheck:unitall clean, andnpm test40 files / 444 tests passed.
Every added test was checked by reverting its fix
resolves each declared Consequence tool to a real label(+ distinctness, as the removals collapse onto one fallback)publish_artifactlabel alone removedresolves each declared Consequence tool to a real labelGATED_TOOLSemptiedis not vacuous: the generated list is populatedshell's label duplicated ontorun_workflowgives no two gated tools the same sentenceReachflipped without regeneratinggated_tool_snapshot_matchesconsequence_toolsextraction returns zero rowsthe_gated_tool_list_is_never_silently_emptyBaselines on the unmodified tree ran green first (frontend 3 passed; Rust 11
passed) so that each red above is attributable to its own sabotage rather than
to a broken tree.
Documentation
The rationale lives next to the code it constrains:
consequence_tools()andgated_tool_snapshot_matchescarry the seam argument in Rust, the generatedfile states it cannot be hand-edited, and
gated-tool-labels.test.tsexplainswhy it asserts through the resolver. No
docs/spec/change — this adds a checkover an existing contract rather than changing one.
Summary by CodeRabbit
New Features
Bug Fixes