tools: ship every tool on, switchable from one settings map#710
Conversation
… engine `main` has been red since a69dfd9 (7 commits). Four independent failures were stacked, and only the first was visible: `check-file-size.sh` runs before the Rust toolchain is even installed, so fmt/clippy/doc/test never executed and the three compile/doc breaks behind it went unreported. 1. **file-size ratchet.** 13 grandfathered files grew past their recorded ceilings without the baseline being regenerated. Fixed the way the script itself prescribes — `make file-size-update`. No entry added, none obsolete; still 32 grandfathered files. 2. **`stella-store` did not compile (8 errors).** #704 and #707 independently implemented #616's `store.db` retention. #704 landed `stella-store/src/retention.rs` + `retention_tests.rs` + `stella-cli/src/storage_prune.rs` (`stella storage prune`); #707 landed `stella-store/src/prune.rs` + `stella stats prune`. #707's engine won and swapped `pub mod retention;` for `pub mod prune;` in lib.rs — but left #704's two *consumers* wired up, so `retention.rs` became an orphan outside the module tree while `mod retention_tests;` and `mod storage_prune;` kept importing `crate::retention`. `prune.rs` is unambiguously the survivor: `AGENTS.md` documents `stella stats prune` with exactly its semantics, it has an end-to-end witness in `stella-cli/tests/stats_prune_cli.rs`, and it is cross-referenced from `stella-store/src/lib.rs`, `usage.rs`, and `usage_cmd.rs`. The `retention` path cited no issue and was referenced only by its own files. So this completes the supersession rather than reviving the loser: the three files and their `main.rs` wiring are deleted. 3. **`cargo doc -D warnings` failed in `stella-store`.** prune.rs's module docs intra-doc-linked `dependent_tables_cover_every_execution_keyed_table`, which is `#[cfg(test)]` and so unresolvable when rustdoc builds without test cfg. Named in backticks instead, with a note saying why it is not a link. 4. **`cargo doc -D warnings` failed in `stella-tools`.** A redundant explicit link target in policy.rs (`[`catalog::group_for`](crate::catalog::group_for)` — label and target resolve identically). Collapsed to the plain path form. Two behavioural notes, deliberate in #707 and made permanent by dropping #704's tests, called out because they are easy to miss in a red-main repair: unfinished (in-flight) executions are no longer protected from pruning — prune.rs argues a NULL `finished_at` is exactly the debris retention should reclaim — and a pending enterprise export no longer holds an execution back; the export ledger is instead excluded from the cascade outright, since `executions.id` is AUTOINCREMENT and ids are never reused. Verified locally: check-no-scratch, check-action-pins, check-file-size, `cargo fmt --check`, `cargo clippy --workspace --all-targets -D warnings`, and `RUSTDOCFLAGS=-D warnings cargo doc --workspace --no-deps` all pass.
Retires the per-capability flags. `Availability::Bash` and `::Web` were policy
defaults wearing an availability costume — neither named a prerequisite, only a
default — which is why every "should this be on?" question needed another enum
variant, another `RegistryOptions` field, and another hand-written branch.
`Availability` now means environment prerequisites only (a media key, a search
key, an issue backend); whether a satisfiable tool is *allowed* is
`ToolPolicy`'s business.
- `bash` and the key-free web family are `Always`. `RegistryOptions` loses
`bash`/`web` entirely. `always_on()` 42 → 46.
- `"tools"` in settings.json is an open map: any tool name, group name, or `*`
→ on/off. Same syntax operators already type; it just stops hardcoding two
keys. Covers MCP (`mcp`) and customer-registered tools (`custom`) too.
- `PolicyToolSet` enforces it directly below `DiscoveryToolSet` — above MCP,
custom, and interactive — so `tool_search` cannot advertise a withheld tool.
It filters `schemas()` AND refuses `execute()`: hiding is a prompt-budget
measure, gating is a capability boundary, and the two must not be confused.
- `candidate_ws.rs` gets it too. Best-of-N candidates build their own registry
from the same options, so post-flip they were a bypass.
- `stella tools` names the key that withheld each tool instead of two
hardcoded "disabled (default)" blocks.
The managed ceiling needed more than union-of-denials, which the witnesses
caught: a managed `{"process": "off"}` is a group key and a project
`{"start_process": "on"}` is more specific, so the union left the org's denial
standing while the tool ran anyway. `apply_tool_ceiling` now also drops any
merged grant the ceiling would deny.
Witnesses verified against the old code in a detached worktree — six fail
there, including `bash_is_registered_with_no_options_at_all` (the flip itself)
and four proving the old `ToolsSettings` silently dropped any key that was not
`bash`/`web`. Stated honestly: `{"bash":"off"}` hiding-and-refusing is a
regression pin, not a fail-on-old witness — bash was absent by default before
too. Its group and custom-tool siblings are the discriminating ones.
Refs #615
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
There was a problem hiding this comment.
Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Suggestion:
The process-free branch of run_turn drives the engine on the raw ToolRegistry without wrapping it in PolicyToolSet, so the "tools" policy (operator/managed-org tool switches) is not enforced in the interactive REPL and --no-pipeline one-shot when process-free authority is active.
The settings surface for the tool policy. `views/settings.rs` said "as more
config surfaces move here they become sections of this tab" — this is the
second section.
The deck had no tool list at all, so one now flows from the driver. The catalog
alone would not do: MCP tools and customer-registered custom tools only exist
at runtime, and being able to switch off your own tools is the point. The
driver sources from the live base executor + custom tools + the catalog's
session set, and re-derives the effective policy from disk on every refresh —
`cfg.tool_policy` is frozen at session start, so reading it would have made a
save invisible until restart.
Rows are grouped by catalog group, with `mcp` and `custom` as ordinary sections
so a customer sees their own tools by name. Each row names the key and the
scope that switched it off. Keys are `engine.rs`'s vocabulary verbatim —
`space`/`⏎` toggle, `x` clear, `s`/`S` save user/project, `r` reload, `Esc`
release — with `t` to focus, mirroring `e`.
Org-managed denials render locked and refuse to toggle, and the *writer*
refuses a grant the ceiling denies. Both halves, because a UI that offers a
switch which silently will not work misrepresents the posture.
Two details worth naming:
- `ToolRow` has no `enabled` field. On/off is derived from the switches plus
unsaved edits, so there is one answer rather than two that can drift. The
resolver checks the pending edit at each precedence level before the saved
switch, so a pending *group* grant cannot outrank a saved *exact* denial —
the naive "edits first, then base" version gets that backwards.
- `ToolsSettings::save_to` mirrors `AgentEngineConfig::save_to`: read as a
`Value`, replace only `"tools"`, symlink-reject, 0600 for user scope. An
emptied section removes the key rather than writing `{}`. The panel sends
only changed keys and the driver read-modify-writes, so saving to one scope
cannot copy another scope's switches into it.
Attribution needed more than `disabled_by()`: that yields the key, but the
*scope* is unrecoverable from the merged policy — merging is what destroys it.
Hence `Settings::load_tool_scopes` / `ToolScopePolicies`, which also exposes
`managed_tool_ceiling()` outside the settings module for the first time.
Witnesses are by mutation, since every surface here is new: each named test was
confirmed to flip red when the behaviour it pins is removed (the lock
short-circuit, the exact-vs-group key choice, the key-preserving write, the
writer's ceiling refusal, the tab rendering the panel). That pass caught a weak
witness of its own — `toggling_a_tool_writes_its_exact_name_never_its_group`
originally toggled `bash`, whose group is also `bash`, so it could not tell the
two keys apart; it now toggles `start_process` and asserts `send_stdin` is
untouched.
Rendering was verified by drawing the real deck into a `TestBackend` at 160x30
and 90x30, focused and unfocused, with a pending group edit and a locked row.
That found one defect: a truncated MCP name ran into the state column. The
snapshot test writes its frame to `$CARGO_TARGET_TMPDIR` so it stays
inspectable.
Also moves `mod tool_policy;` to `main.rs`, closing the in-repo FOLLOW-UP the
previous commit left behind.
Refs #615
Merging main into this branch brought in #709's `stella storage prune` without the arm that `run_storage`'s later `match cmd` needs, so the branch did not compile. `run_storage` already returns for `Prune` before the storage map loads — retention operates on `store.db`, not on the map — but exhaustiveness checking is not flow-sensitive, so the arm has to exist even though it is genuinely unreachable. Verified the base commit fails `cargo check -p stella-cli --tests` on its own, so this is the merge's breakage, not the tools editor's.
|
Update: the TUI settings-tab editor landed here too, so this PR now covers the whole decision rather than leaving the UI as a follow-up. (The "Not in scope" note above is stale on that point — per-tool subcommand deny lists remain out of scope, as intended.)
Three things worth a reviewer's attention:
Witnesses here are by mutation (every surface is new): each named test was confirmed to flip red when the behaviour it pins is removed. That pass caught a weak witness of its own — the exact-vs-group test originally toggled Also note the last commit repairs the branch: merging |
…aw `ToolRegistry` without wrapping it in `PolicyToolSet`, so the `"tools"` policy (operator/managed-org tool switches) is not enforced in the interactive REPL and `--no-pipeline` one-shot when process-free authority is active.
This commit fixes the issue reported at stella-cli/src/agent.rs:2058
## Bug
In `stella-cli/src/agent.rs`, `run_turn` branches on `process_free_authority_active()`:
```rust
let outcome = if crate::enterprise_telemetry::process_free_authority_active() {
let engine =
Engine::with_sleeper(provider, registry, engine_config_for(cfg), &TokioSleeper)
.with_calibration(calibration);
engine.run_turn_with_sender(messages, budget, &tx).await
} else {
// ... builds CustomToolSet -> InteractiveToolSet -> PolicyToolSet -> DiscoveryToolSet
let permitted = PolicyToolSet::new(&interactive, session_tool_policy(cfg));
...
};
```
The process-free branch hands the raw `registry` (`&ToolRegistry`) straight to the engine, **never** wrapping it in `PolicyToolSet`. The `else` branch does apply `PolicyToolSet::new(&interactive, session_tool_policy(cfg))`.
### Concrete trigger
`run_turn` is invoked from the interactive REPL loop and from `run_raw_one_shot` (`--no-pipeline`). In process-free mode, `ToolRegistry` still registers many non-host tools (`read_file`, `write_file`, `edit_file`, `apply_edits`, `delete_file`, `save_memory`, task tools, `generate_svg`, etc. — only the host-reaching class is withheld at construction). So with an operator/managed-org policy such as:
```json
{ "tools": { "write_file": "off" } }
```
or
```json
{ "tools": { "*": "off", "read_file": "on" } }
```
the model can **still** call the disabled tools on this path, because no `PolicyToolSet` gates them. Every other driver path (`pipeline` in `agent/goal.rs`, `command_deck.rs`, `subsession.rs`, `fleet_cmd.rs`, `run_pipeline_one_shot`) applies `PolicyToolSet` unconditionally — this was the only path with the gap, breaking the stated invariant that the policy sits above the whole session tool stack on every path.
## Fix
Wrap `registry` in `PolicyToolSet::new(registry, session_tool_policy(cfg))` in the process-free branch before handing it to the engine, mirroring the other paths:
```rust
let permitted = PolicyToolSet::new(registry, session_tool_policy(cfg));
let engine =
Engine::with_sleeper(provider, &permitted, engine_config_for(cfg), &TokioSleeper)
.with_calibration(calibration);
engine.run_turn_with_sender(messages, budget, &tx).await
```
This is type-safe: `&ToolRegistry` already coerces to `&dyn ToolExecutor` (it was passed directly to `Engine::with_sleeper`), which is what `PolicyToolSet::new` accepts, and `&PolicyToolSet` implements `ToolExecutor` (it is wrapped by `DiscoveryToolSet` in the `else` branch). `PolicyToolSet` and `session_tool_policy` are already imported/used in the same function. The process-free layer intentionally strips MCP/custom/interactive/discovery layers, but policy enforcement is preserved.
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: macanderson <mac@macanderson.com>
…dropped (#717) `fmt + clippy + test` failed in 7s on `feat/tool-switches-on-709`. As on main, `check-file-size.sh` runs before the Rust toolchain installs, so the fast fail hid a genuine compile error behind it. Three fixes: 1. **`stella-cli/src/settings.rs` crossed the 1500-line ratchet at 1623 lines.** It is not grandfathered, and the guard hard-blocks that case while explicitly forbidding a baseline entry — "the baseline only covers files that predate the guard, and this is the rule that stops the tree getting worse." So it is split, not exempted: the 868-line `#[cfg(test)] mod tests` moves to `stella-cli/src/settings/tests.rs`. That directory already holds seven sibling modules, and the same split has precedent in e72b02f (deck_ui.rs) and in stella-store's `*_tests.rs` files. settings.rs drops to 756 lines, the new file is 874 — neither needs an entry. Pure move, verified as such: the extracted body is byte-identical to the old inline body modulo one dedent level, all 27 `#[test]`s are still present, and `cargo fmt --check` passes on the result untouched. `use super::*` resolves to `crate::settings` from a file submodule exactly as it did inline, so nothing about name resolution changes. 2. **`StorageCmd::Prune(_)` was missing from `run_storage`'s match** — `error[E0004]: non-exhaustive patterns`. The merge commit 6e61207 kept the variant and its early-return dispatch but dropped the `unreachable!` arm. `origin/main` still has that arm (main.rs:1305) and main is green, so this was introduced by this branch's conflict resolution, not inherited. Restored. 3. **Four grandfathered files grew** — agent_tests.rs +157, agent.rs +32, candidate_ws.rs +24, command_deck.rs +1 — so the baseline is regenerated per the script's own instruction. It also *tightened* five ceilings where this branch shrank files (registry.rs 3052→3014, main.rs, stella-store/lib.rs, media.rs, and one more), so the ratchet moves net-inward. Still 32 entries. Note the ordering: regenerating the baseline before fix (2) records a ceiling that fix (2) then exceeds by the two lines it adds. Baseline last. Gate, full workspace: check-no-scratch, check-action-pins, check-file-size, `cargo fmt --check`, `cargo clippy --workspace --all-targets -- -D warnings`, `RUSTDOCFLAGS=-D warnings cargo doc --workspace --no-deps`, and `cargo test --workspace` — 3,559 tests across 61 suites, 0 failures.
…doc link
runs before the Rust toolchain installs, so its fast fail kept hiding the rest.
1. **The regenerated baseline was recorded against the wrong tree.** It has
`command_deck.rs` at 4466 and `deck_ui.rs` at 3940 — main's values — but
this branch's TUI tools editor puts them at 4592 and 3965. Six grandfathered
files were still over ceiling. Regenerated against the actual tree; the
raises this branch really owes are now visible, per the guard's own rule:
command_deck.rs 4465→4592 (+127) the `tools` section: `handle_tools_input`
and `tool_policy_inbound`, beside their
inline sibling `handle_engine_config_input`
deck_ui.rs 3940→3965 (+25) tools panel state on the deck
agent.rs 2223→2224 (+1) the process-free `PolicyToolSet` wrap
main.rs 1776→1782 (+6) the Prune arm's fuller comment
engine.rs 1650→1652 (+2) `t` alongside `e` in the key row
deck_render.rs 1670→1671 (+1)
command_deck.rs's +127 is the one worth a reviewer's eye. It is left inline
deliberately: `handle_tools_input` is the exact structural peer of
`handle_engine_config_input` directly above it, and hoisting only the newer
of the two into a submodule would split a matched pair to satisfy a counter.
2. **`StorageCmd::Prune(_)` was duplicated, not missing** — `warning:
unreachable pattern`, which `-D warnings` makes fatal. #717 read the merge as
having *dropped* main's last-position arm, but the merge had *moved* it to
the top of the match with a fuller comment, so re-adding it made two. Removed
the re-added copy; the branch's own better-documented arm at the head stands.
3. **`views/settings.rs` intra-doc-links private `SPLIT_MIN_WIDTH`** —
`-D rustdoc::private_intra_doc_links` fails the doc build. The prose only
names the threshold, so it drops to plain code formatting rather than widen
the constant's visibility for a comment's sake.
Gate, full workspace: check-no-scratch, check-action-pins, check-file-size,
`cargo fmt --check`, `cargo clippy --workspace --all-targets -- -D warnings`,
`RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps`, and
`cargo test --workspace` — 3,579 tests across 61 suites, 0 failures.
…-709 # Conflicts: # CHANGELOG.md
|
CI is green — #717 fixed the first failure but four more were stacked behind it.
One thing for your call
Also flagging that the process-free |
#724) `main` is red at 872ff39. `check-file-size.sh` fails before the Rust toolchain installs, so the whole gate is dark behind it: stella-tui/src/deck_render.rs grew to 1677, over its ceiling of 1676 (+1) stella-tui/src/deck_ui.rs grew to 4049, over its ceiling of 4025 (+24) Neither PR was wrong on its own — this is a collision. #710 and #722 both touch the deck, and #722's baseline was computed against a tree that predated #710's merge. #710 landed first and raised `deck_ui.rs` to 4025; #722 then merged on top and added its own growth over a ceiling that had moved underneath it. Two individually-green PRs, red once combined — the exact shape the merge queue exists to catch (see ci.yml's `merge_group` note), which means neither was queued. Baseline regenerated against the actual tip. Only the two entries move; the other 29 are untouched and the count drops 32→31 (#710's split of settings.rs retired an entry). Worth a second opinion: the guard prefers a new module to a raised ceiling for non-irreducible growth, and `deck_ui.rs` at 4049 is the tree's second-worst file. This raise is the minimum that unblocks a red `main` — +24 and +1, both genuine feature lines from #722 — but if the intent is to hold the line on the deck specifically, the extraction is the better follow-up and this can be reverted into it. Gate, full workspace at this commit: check-no-scratch, check-action-pins, check-file-size, `cargo fmt --check`, `cargo clippy --workspace --all-targets -- -D warnings`, `RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps`, and `cargo test --workspace` — 3,605 tests across 61 suites, 0 failures.
1 participant
Stacks on #709 — base it there, or rebase onto main once #709 lands.
mainis currently red on its own (the #704/#707 rival-prune collision); #709 repairs it, and this cherry-picks cleanly on top with the full gate green.Implements the decision: ship with every tool on, and make one switch mechanism work the same way for every tool — built-in, MCP, or customer-registered.
Why the old flags had to go, not just flip
Availability::Bashand::Webwere never availability. Neither named a prerequisite; they named a default. That's why every new "should this be on?" question cost another enum variant, anotherRegistryOptionsboolean, and another hand-written branch in the registry — and why the switch only ever covered built-ins.Split cleanly now:
Availability= what the environment supplies. A media key, a search key, an issue backend.bashand the key-free web tools have no prerequisite, so they areAlways.ToolPolicy= what the operator allows. One open"tools"map insettings.json.The switch
Same syntax operators already type — it just stops hardcoding two keys. One precedence rule: most specific wins, exact name > group >
"*". So{"*": "off", "read_file": "on"}is a read-only agent in two lines.Groups come from
catalog.rs's own section comments, promoted from comments to data. Tools the table has never seen are addressable too:mcp__<server>__<tool>→mcp, anything else →custom, so a customer can switch off their own registered tools without enumerating them.Enforcement
PolicyToolSetsits directly belowDiscoveryToolSet— above MCP, custom, and interactive — the only position that sees the whole session surface. It filtersschemas()and refusesexecute(). That asymmetry is the point: hiding a schema is a prompt-budget measure, but a capability gate has to hold when the name is called anyway, from a stale prompt or a replayed trajectory. (DiscoveryToolSet's lean mode is deliberately the opposite — hides without gating. These must not be confused.) The refusal reads asunknown tool, so a disabled tool is indistinguishable from one that was never built.candidate_ws.rsgets it too — best-of-N candidates build their own registry from the same options, so post-flip they were a bypass.Org control, without new syntax
A deny in the managed scope survives any project grant. The witnesses caught a real hole here:
deny_all_from(union of denials) is not sufficient, because a managed group deny ({"process": "off"}) loses to a more-specific project grant ({"start_process": "on"}).apply_tool_ceilingnow also drops any merged grant the ceiling would deny.Witnesses
Verified in a detached worktree at the base — six fail there:
bash_is_registered_with_no_options_at_allandthe_key_free_web_family_…— the flip itself; both panic with "bash must be advertised with no configuration"ToolsSettingssilently dropped any key that wasn'tbash/webStated plainly:
{"bash":"off"}hiding-and-refusing is a regression pin, not a fail-on-old witness — bash was absent by default before too, so it passes either way. Its group and custom-tool siblings are the discriminating ones.Prose
Every "off by default" claim rewritten:
agent-tools/index.mdx(counts 42→46, new "Switching tools off" section),permissions.mdx,configuration/settings.mdx,configuration/index.mdx,examples/team-settings.mdx, andbash.rs's module docs — which keep the still-true point that switching the tool off removes the shell TOOL, not the shell CAPABILITY (run_tests,run_scriptet al. still reachbash -c; thecommand.startedchain is what covers them uniformly).Not in scope
Per-tool subcommand deny lists (flagging individual verbs of a tool off) — noted as a future direction, deliberately not built here. The TUI settings-tab editor is the remaining piece of the decision;
ToolsSettingsnow derivesSerializespecifically so it can round-trip.Refs #615