refactor(prompts): remove the inert omit_skills_catalog flag (#5699) - #5815
refactor(prompts): remove the inert omit_skills_catalog flag (#5699)#5815ntdatt812 wants to merge 2 commits into
Conversation
…ansai#5699) `AgentDefinition::omit_skills_catalog` was threaded through both sub-agent prompt paths and read by neither. This removes it rather than wiring it up, because the repo already settled on the other design. Why removal is the resolution that matches the code: - `render_helpers.rs` records that the `render_skills` helper was deliberately deleted and that the catalogue "lives in the individual agent's prompt.rs". Honouring the flag would resurrect the layering that comment describes removing. - The one definition that opted *in* — `orchestrator/agent.toml`, with `omit_skills_catalog = false` — already renders its own catalogue: `orchestrator/prompt.rs::render_installed_skills` emits `## Installed Skills`. So no definition in the tree loses anything. - `integrations_agent/prompt.rs` documents dropping its own `## Available Skills` block in the skills->workflows unification, and pins the absence with a test. Also corrects a comment this flag had accreted. `config/schema/context.rs` credited `omit_skills_catalog = true` with fixing recursive dispatch by "preventing [the summarizer] from seeing `spawn_subagent`", and used that to justify re-enabling a 4000-token production default. A flag with no reader cannot gate a tool. The actual guard is the summarizer's empty tool allowlist (`[tools] named = []` in `registry/agents/summarizer/agent.toml`); the comment now says so. Backward compatibility: `AgentDefinition` does not set `serde(deny_unknown_fields)`, so custom TOML definitions still carrying the retired key keep loading — the key is ignored, not rejected. Added `retired_omit_skills_catalog_key_still_deserializes` to pin that, so a future `deny_unknown_fields` cannot break those users silently. Note for API consumers: `agent_cli`'s JSON dump no longer emits an `omit_skills_catalog` entry, and `for_subagent` / `from_definition_flags` each take one fewer argument. `cargo test --lib prompts::` 68 passed; `cargo check --lib --tests` clean.
|
Baseline done — all 9 local failures are pre-existing on this host, not from this PR. I ran the same 9 test names on a clean Identical set, identical names:
They all look like Windows path/env assumptions (absolute-path shapes, I checked the last one specifically before concluding, since its name mentions the summarizer and this PR touches a summarizer comment: it fails on clean |
tinysweeper
Bot
added
the
priority: p3
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
|
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 selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThis change removes the inert ChangesSkills catalog flag removal
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change removes an unused configuration flag and updates its call sites and documentation without introducing a supported correctness or production risk; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Out of Scope Changes checkExplanation Most changes are in scope, but context_scout and planner include unrelated iteration, result-size, tier, model, delegate, and tool-surface changes. The vision agent also changes omit_safety_preamble behavior instead of only removing the retired flag.
Warning Your free Security trial is over. An organization admin can activate billing to continue. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Three positional call sites in `tests/raw_coverage/` still passed the removed
`omit_skills_catalog` argument, so `raw_coverage_all` failed to compile with
E0061 in the Rust Core Coverage lane.
I missed them because `raw_coverage_all` declares
`required-features = ["voice", "inference"]`, and my local `cargo check --lib
--tests` runs the default feature set, which skips the target entirely. Checked
now with the same feature list CI uses:
cargo check --tests --features channels,media,inference,voice,web3,\
documents,modules,flows,skills,mcp,crash-reporting,http-server,\
scheduler-gate,file-logging,contacts,runtime-node,memory-git,hosting
Finished clean. `cargo fmt --all` clean.
|
The compile break is fixed and the lane now gets past it. What is failing now is a different test, in a domain this PR does not touch: Why this PR is what surfaced it. Why it looks pre-existing rather than caused. The assertion is on process-global state: it requires the memory client to be uninitialised, and I am not asserting that from reading alone. I have the same command CI runs going against a clean I will post the result either way. If it fails on Everything else on the run is green, including |
|
Baseline result, as promised — and it does not support the conclusion I was leaning toward, so here is what I actually measured. Like-for-like on my host, same command CI runs:
Identical — same three tests, same three assertions, including the one CI flagged at Two caveats I am not going to paper over.
I was also wrong about the mechanism. I suggested this looked like an ordering/global-state dependency inside the merged What stands: nothing in this diff initialises a memory client or touches composio; the diff removes a prompt flag from I would rather you make the call than have me guess: happy to re-run the lane, to open a separate issue for the composio assertion, or to dig further if you can point me at what |
1 participant
Closes #5699.
AgentDefinition::omit_skills_catalogwas threaded through both sub-agent prompt paths and read by neither. The issue offers two resolutions — honour it or remove it — and says either is fine. This removes it, because the repo has already settled on the design that makes it redundant.Why removal, not wiring
render_helpers.rsrecords that the catalogue renderer was deliberately deleted: "render_skillsandrender_connected_integrationshelpers are gone —## Available Skillslives inintegrations_agent/prompt.rs". Honouring the flag would rebuild the layering that comment describes tearing down.true; that is not quite right —orchestrator/agent.toml:26setsomit_skills_catalog = false. It loses nothing, becauseorchestrator/prompt.rs::render_installed_skillsemits## Installed Skillsitself. So no definition in the tree changes behaviour.integrations_agent/prompt.rsdocuments dropping its own## Available Skillsblock in the skills→workflows unification, and pins the absence with a test.The flag is a fossil of a design the repo has already moved past.
A stale comment that had grown around it
config/schema/context.rscredited the flag with fixing recursive dispatch:A flag with no reader cannot gate a tool. The real guard is the summarizer's empty tool allowlist —
[tools] named = []inregistry/agents/summarizer/agent.toml. That comment justifies a production default of 4000 tokens, so leaving it pointing at the wrong mechanism was the part of this issue worth fixing carefully: anyone auditing that default would have concluded the recursion guard was being removed by this PR. It now names the allowlist.Backward compatibility
AgentDefinitiondoes not set#[serde(deny_unknown_fields)], so custom TOML definitions still carryingomit_skills_catalogkeep loading — the key is ignored, not rejected.retired_omit_skills_catalog_key_still_deserializespins that, so a futuredeny_unknown_fieldscannot break those users silently.Two changes API consumers should know about:
agent_cli's JSON dump no longer emits anomit_skills_catalogentry.SystemPromptBuilder::for_subagentandSubagentRenderOptions::from_definition_flagseach take one fewer argument, andSubagentRenderOptions::include_skills_catalogis gone.Verification
cargo check --lib --tests— clean.cargo test --lib prompts::— 68 passed.cargo test --lib— full core suite run; see the note below.cargo fmt --all— clean.Disclosure: the full
cargo test --librun on this Windows host has 9 failures, all in path/environment tests (resolve_action_dir_*,upsert_materializes_home_*,imports_*_jsonl,require_managed_worktree_path_*,absolute_leaves_absolute_paths_alone, and two others). I am baselining them against a cleanmainon the same host and will post the comparison; none touch prompts or definitions. Please read CI as the authority over my local run.Summary by CodeRabbit
New Features
Bug Fixes
Documentation