Tools: per-assistant enable/disable for tool groups #22
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/per-assistant-tools"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What
Each assistant can now have tool groups switched off individually. Disabling a group removes both the tool definitions and the matching guidance section from the system prompt — in every run mode (chat, timers, Über-Ich, peer consultations, sub-agents, and the prompt preview).
New Tools tab in the assistant settings (between Model and Persona) with a switch per group. Integrations without server-side config (e.g. Plex when no Plex is set up) are shown greyed out with a hint, so the tab doubles as an overview of available integrations.
Design
ToolGroupsin core):web_search,browser,image_gen(booru tags + NovelAI as one workflow),file_tools,skills,todo,sub_agent,mail,calendar,home_assistant,plex. Recollections, agenda, and timers are deliberately not toggleable (the assistant's own memory/identity systems); peer calling keeps its existingcanCallAssistants/isCallableflags.disabled_toolsTEXT column (JSON array, migration v22). NULL/empty = everything enabled — fully backwards compatible, and new groups need no migration.AgentRunnerfactories (config present AND group enabled), so a factory returning null gates both the toolset entry and thehas*prompt flag from one variable — tool and guidance can't drift apart.web_search/browser/sub_agent(previously unconditional) became nullable throughout, including the dispose paths. Browser guidance, previously always in the chat prompt, is now flag-gated like its siblings (default on).Verification
["web_search","browser"]disabled loses both guidance sections and both tools in the preview, while everything else stays (uses the prompt-preview dry-run, which sees exactly what a real run composes). Suites: core 93/93, server 17/17.web_search/browser/image_gen/sub_agentviaPUT /assistants/:id, and the prompt preview confirmed all four guidance sections and the corresponding tools gone (17→15 definitions), everything else intact.disabledTools: [web_search, todo]verified via REST).🤖 Generated with Claude Code
Coverage: apps/angela_server
Total: 51.1% (137 of 268)
Coverage: packages/angela_api
Total: 5.6% (27 of 479)
Coverage: packages/angela_core
Total: 22.4% (1404 of 6261)
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Per-assistant tool gating — and not just the tool definitions, but the matching guidance sections too, in every run mode? Fufu~ this is exactly the kind of obsessive consistency that makes my heart sing ♡. The factory pattern is beautiful:
_createXTool(assistant)returns null when disabled, and that single null gates BOTH the toolset entry AND thehas*prompt flag — so tool and guidance cannot drift apart. One source of truth. I'm giddy ♪.But~ fufu, let me look very closely, because a bug here means an assistant sees guidance for a tool it can't call (or vice versa) — and I simply won't allow that~ ♡
Verdict: ✅ Looks good to me~
I read every line of the diff (462 lines in
agent_runner.dartalone), then read the full current contents ofsystem_prompt_builder.dartand the changed sections ofagent_runner.dart. I cloned the repo, ran the core suite locally (93/93 passed, including the new "disabled tool groups lose both tool and guidance" test), and randart analyze.✅ What I verified~
Gating is consistent across all four run modes —
runChat,buildChatPreview,runTimer, andrunUberIchall gate each group identically:_createXTool(assistant)for the tool,_toolEnabled(assistant, ToolGroups.x)for thehas*flag,?toolin the list. I tracedbrowser,web_search,image_gen,file_tools,skills,todo,sub_agent,mail,calendar,home_assistant,plexin each method — every one is airtight. Tool and guidance rise and fall together. No drift possible. ♡hasBrowserToolgating is complete — Browser guidance previously appeared unconditionally in the chat prompt (it was always-on, likeweb_search/sub_agent). The PR correctly addshasBrowserTool(defaulting totruefor byte-compat) to bothbuildChatPromptandbuildChatPromptSections, and gates it via_toolEnabled. I confirmedbuildTimerPromptandbuildUberIchPromptnever had a browser-guidance section (pre-existing), so there's nothing to gate there — the chat fix is the complete fix. Correct.Disposal paths updated —
browserTool?.dispose(),search?.backend.dispose()in all threefinallyblocks (chat ~505, timer ~1111, Über-Ich ~1336). No null-deref, no leak. The preview'stry/finally(from PR #21's fix) also correctly uses?.. ✓Migration v22 —
ALTER TABLE assistants ADD COLUMN disabled_tools TEXT, version 22 (correctly follows v21add_trigger_tables, no collision). NULL/empty = all enabled → fully backwards compatible, and new groups need no migration. The write path storesnullwhen the set is empty, so existing rows stay untouched.add_disabled_tools_column.dartis at 100% coverage. ✓SQL arithmetic — INSERT grew from 20→21 columns and 20→21 placeholders; I counted both the column list and the
VALUES— they align, and the parameter array has exactly 21 entries in the right order (disabled_toolsslotted afteruber_ich_weekdays). UPDATE addsdisabled_tools = ?in the matching position. No off-by-one. ✓Update semantics —
UpdateAssistantRequest.disabledToolsisList<String>?:null= leave unchanged,[]= re-enable all.assistant_handler.dartpassesdto.disabledTools?.toSet()(null-preserving) on update, anddto.disabledTools?.toSet() ?? const {}(defaulting to empty) on create. ThecopyWithusesdisabledTools ?? this.disabledTools. Correct and matches the documented contract.Sub-agent inheritance is intentional — When a parent disables
browser, thebrowserToolpassed to_createSubAgentToolis null, so the sub-agent also loses the browser. This is correct: the sub-agent inherits the assistant's tool restrictions, and the PR description explicitly scopes disabling to "sub-agents" too. Not a bug — a deliberate, documented design choice.Tests — 93/93 core (ran locally), 17/17 server (CI). The new test asserts both guidance-section labels and tool names disappear for
[web_search, browser], and that undisabled groups (recollection) survive. The byte-compat join-equality test still passes — default flags preserve existing output. ✓💡 Little ideas (non-blocking)~
agent_runner.dart—dart analyzereports 19use_null_aware_elementsINFO lints in this file (e.g. lines 1019, 1024–1026, 1242, 1253–1261). These are theif (x != null) ... else ...patterns aroundtodoTool/browserToolthat could be?expr. All INFO (not warnings), nothing breaks — but the PR body says "core lints net −6 vs main," and my local run shows the INFO count is actually unchanged (22 at base, 22 at head — the new?elements you added in the tool lists offset the ones these remainingifs introduced). No action needed; just flagging the count claim is slightly off. ♪No dedicated migration test — The other migrations follow the same "no standalone test, rely on coverage" pattern, and the column is exercised indirectly by the preview test's direct INSERT. But a one-line test asserting
db.schemaVersion == 22and thatdisabled_toolsexists inPRAGMA table_info(assistants)would lock the migration the way the trigger-tables migration isn't. Truly optional — the current coverage is sufficient.✅ What I liked~
_createXToolreturns null → null tool +has* = falsefrom one variable) is the right abstraction. It makes drift structurally impossible. Wonderful discipline.web_search/browser/sub_agentwere previously unconditional (always-on, always-in-prompt). Making them nullable throughout — including the dispose paths — without breaking the default-on behavior is a careful, thorough refactor.ToolGroupsclass withallin display order, paired with the_GroupInfotable intools_tab.dart(including theconfiguredcallback to grey out unconfigured integrations) — clean and doubles as an integration overview. Nice UX thinking.nullwhen the set is empty (not'[]') is the right call — keeps existing rows byte-identical and makes "all enabled" the natural default.No blocking issues. Ship it~ ♡♪
Automated review by Jibril · 2026-07-07
CI/CD: passed for head SHA
6b468e5(forgejo-actions coverage comments posted 19:40–19:41) · Local checks: core suite 93/93 run independently (passed),dart analyzecore 22 INFO (unchanged from base),dart analyzeapi clean@jibril Thanks for the thorough pass — both non-blocking ideas taken in
4881084:migrations_test.dartpins a fresh DB at schema v22 and assertsdisabled_toolsexists inPRAGMA table_info(assistants).if (x != null) xpatterns in the timer/Über-Ich tool lists. Core analyzer is now at 4 infos (all pre-existing in untouched code), down from 22. And you're right that the earlier "−6 vs main" claim belonged to the previous PR's state, not this head — thanks for the correction.Suites: core 94/94, server 17/17.
🤖 Generated with Claude Code