refactor(prompts): remove the inert omit_skills_catalog flag (#5699)

[ntdatt812]

Closes #5699.

AgentDefinition::omit_skills_catalog was 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.rs records that the catalogue renderer was deliberately deleted: "render_skills and render_connected_integrations helpers are gone — ## Available Skills lives in integrations_agent/prompt.rs". Honouring the flag would rebuild the layering that comment describes tearing down.
• The one definition that opts in already has its catalogue. The issue states every in-tree definition sets true; that is not quite right — orchestrator/agent.toml:26 sets omit_skills_catalog = false. It loses nothing, because orchestrator/prompt.rs::render_installed_skills emits ## Installed Skills itself. So no definition in the tree changes behaviour.
• integrations_agent/prompt.rs documents dropping its own ## Available Skills block 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.rs credited the flag with fixing recursive dispatch:

Re-enabled at 4000 tokens after the recursive-dispatch root cause was fixed by the omit_skills_catalog = true guard on the summarizer archetype (which prevents it from seeing spawn_subagent and thus cannot recurse).

A flag with no reader cannot gate a tool. The real guard is the summarizer's empty tool allowlist — [tools] named = [] in registry/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

AgentDefinition does not set #[serde(deny_unknown_fields)], so custom TOML definitions still carrying omit_skills_catalog keep loading — the key is ignored, not rejected. retired_omit_skills_catalog_key_still_deserializes pins that, so a future deny_unknown_fields cannot break those users silently.

Two changes API consumers should know about:

• agent_cli's JSON dump no longer emits an omit_skills_catalog entry.
• SystemPromptBuilder::for_subagent and SubagentRenderOptions::from_definition_flags each take one fewer argument, and SubagentRenderOptions::include_skills_catalog is 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 --lib run 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 clean main on 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

• Behavior Changes

  • Skills catalogs are now included in sub-agent context by default.
  • Removed the option to suppress skills catalogs from agent configurations and prompt rendering.
  • Updated the vision agent to omit the safety preamble instead of the skills catalog.
  • Agent list JSON output no longer includes the retired setting.

• Bug Fixes

  • Legacy agent definitions containing the retired setting continue to load correctly.

• Tests

  • Updated prompt, agent, and configuration tests to reflect the new default behavior.

[ntdatt812]

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 main (same machine, same toolchain):

test result: FAILED. 0 passed; 9 failed; 0 ignored; 11113 filtered out

Identical set, identical names:

test	on this branch	on clean main
embed::agent::…::absolute_leaves_absolute_paths_alone	FAILED	FAILED
worktree_schemas::…::require_managed_worktree_path_enforces_absolute_and_managed	FAILED	FAILED
spawn_parallel_agents::…::invalid_ownership_paths_are_rejected_with_the_host_message	FAILED	FAILED
config::schema::load::…::resolve_action_dir_blank_env_does_not_pin	FAILED	FAILED
config::schema::load::…::resolve_action_dir_override_beats_default_when_no_env	FAILED	FAILED
profiles::ops::…::upsert_materializes_home_and_list_enriches_paths	FAILED	FAILED
session_import::ops_tests::imports_legacy_date_folder_jsonl	FAILED	FAILED
session_import::ops_tests::imports_flat_native_jsonl_with_parity	FAILED	FAILED
artifact_offload::…::offload_failure_keeps_the_inline_payload_for_the_summarizer_fallback	FAILED	FAILED

They all look like Windows path/env assumptions (absolute-path shapes, $HOME materialisation, action-dir env resolution, JSONL fixture paths). Happy to open a separate issue with the failure output if that is useful — it is unrelated to this PR either way.

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 main too, and it exercises artifact offload, not prompt flags.

[tinysweeper]

tinysweeper found nothing blocking. Approving.

$0.0000 · 0 in / 0 out

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 6ce8d002-d37a-4c44-9e27-776e91229d4a

📥 Commits

Reviewing files that changed from the base of the PR and between b9149f0 and 7d68e04.

📒 Files selected for processing (11)

• tests/raw_coverage/agent_archivist_debug_round21_raw_coverage_e2e.rs
• tests/raw_coverage/agent_harness_leftovers_raw_coverage_e2e.rs
• tests/raw_coverage/agent_harness_raw_coverage_e2e.rs
• tests/raw_coverage/agent_large_round25_raw_coverage_e2e.rs
• tests/raw_coverage/agent_prompts_subagent_raw_coverage_e2e.rs
• tests/raw_coverage/agent_round26_raw_coverage_e2e.rs
• tests/raw_coverage/agent_session_round24_raw_coverage_e2e.rs
• tests/raw_coverage/agent_session_turn_raw_coverage_e2e.rs
• tests/raw_coverage/inference_agent_raw_coverage_e2e.rs
• tests/raw_coverage/tools_agent_credentials_state_raw_coverage_e2e.rs
• tests/raw_coverage/tools_approval_channels_raw_coverage_e2e.rs

💤 Files with no reviewable changes (9)

• tests/raw_coverage/agent_large_round25_raw_coverage_e2e.rs
• tests/raw_coverage/agent_session_turn_raw_coverage_e2e.rs
• tests/raw_coverage/agent_archivist_debug_round21_raw_coverage_e2e.rs
• tests/raw_coverage/agent_session_round24_raw_coverage_e2e.rs
• tests/raw_coverage/tools_agent_credentials_state_raw_coverage_e2e.rs
• tests/raw_coverage/agent_harness_leftovers_raw_coverage_e2e.rs
• tests/raw_coverage/tools_approval_channels_raw_coverage_e2e.rs
• tests/raw_coverage/agent_harness_raw_coverage_e2e.rs
• tests/raw_coverage/agent_round26_raw_coverage_e2e.rs

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.

---

📝 Walkthrough

Walkthrough

The pull request removes the inert omit_skills_catalog option from agent definitions, prompt APIs, prompt wiring, configurations, generated definitions, and tests. Legacy TOML keys remain accepted during deserialization.

Changes

Skills catalog flag removal

Layer / File(s)	Summary
Prompt contracts src/core/agent_cli.rs, src/openhuman/agent/prompts/...	The agent JSON output, SystemPromptBuilder::for_subagent, and SubagentRenderOptions no longer expose the skills catalog option.
Prompt wiring src/openhuman/agent/harness/session/builder/factory.rs, src/openhuman/agent/registry/defaults.rs, src/openhuman/config/schema/context.rs	Prompt construction and synthesized definitions stop forwarding or documenting the removed option.
Agent configurations src/openhuman/agent/registry/agents/*, src/openhuman/flows/agents/*, src/openhuman/memory/agent/*, src/openhuman/skills/runtime/*, scripts/debug/*	Agent and generated audit configurations no longer set omit_skills_catalog.
Tests and fixtures src/openhuman/agent/**, src/openhuman/channels/**, src/openhuman/tools/**, tests/raw_coverage/*	Fixtures, prompt tests, loader assertions, and compatibility coverage match the removed option and legacy TOML behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 7d68e

This PR removes an unused configuration flag and updates its call sites and tests; no actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: senamakel

Poem

A rabbit found a flag asleep,
Buried deep in prompt-land's heap.
The catalog now runs free,
Old TOML keeps its key,
And tests hop neatly into step.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	Most changes are in scope, but vision_agent/agent.toml replaces omit_skills_catalog = true with omit_safety_preamble = true, which introduces an unrelated behavior change not required by issue #…	Remove the unrelated omit_safety_preamble = true change from the vision agent configuration, unless a separate requirement explicitly requires that behavior.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the main change: removal of the inert omit_skills_catalog flag.
Linked Issues check	✅ Passed	The PR satisfies issue #5699 by removing omit_skills_catalog from AgentDefinition, sub-agent prompt APIs, configurations, call sites, and tests. It also preserves legacy TOML deserialization as re…
Docstring Coverage	✅ Passed	Docstring coverage is 92.11% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 38 functions across 11 files.

Full details: Linked Issues check

Explanation

The PR satisfies issue #5699 by removing omit_skills_catalog from AgentDefinition, sub-agent prompt APIs, configurations, call sites, and tests. It also preserves legacy TOML deserialization as required by the selected removal approach.

Full details: Out of Scope Changes check

Explanation

Most changes are in scope, but vision_agent/agent.toml replaces omit_skills_catalog = true with omit_safety_preamble = true, which introduces an unrelated behavior change not required by issue #5699.

• Fix all pre-merge checks with AI

Warning

Your free Security trial is over. An organization admin can upgrade to Advanced for continuous pull request security review or dismiss this notice.

---

_{Comment @coderabbitai help to get the list of available commands.}

[ntdatt812]

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:

---- composio_raw_coverage_e2e::composio_controller_registry_and_scope_handlers_cover_validation_edges ----
assertion failed: memory_missing.contains("memory client not initialised")
test result: FAILED. 23 passed; 1 failed; 448 filtered out

Why this PR is what surfaced it. rust-coverage-changed.sh maps src/<a>/<b>/… to the libtest filter <a>::<b>. This PR touches src/openhuman/tools/orchestrator_tools.rs, so openhuman::tools is in the scope set and drags the composio raw-coverage target into the run. Until my last push, the lane never reached that test — it died at the E0061 compile error first.

Why it looks pre-existing rather than caused. The assertion is on process-global state: it requires the memory client to be uninitialised, and raw_coverage_all merges ~76 former test files into one binary. Nothing in this PR initialises a memory client — the diff removes a prompt flag from AgentDefinition and drops one key from agent_cli's JSON dump.

I am not asserting that from reading alone. I have the same command CI runs going against a clean main on my machine:

cargo test --features "$(bash scripts/ci/product-features.sh)" \
  --test raw_coverage_all -- composio_raw_coverage_e2e:: --test-threads=1

I will post the result either way. If it fails on main too, this is a latent order/state dependency that any PR touching openhuman::tools will now hit, and it deserves its own issue rather than a fix smuggled into this one — tell me if you would rather I file it or fix it here.

Everything else on the run is green, including Rust Quality, and both bots have approved.

[ntdatt812]

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:

cargo test --features "$(bash scripts/ci/product-features.sh)" \
  --test raw_coverage_all -- composio_raw_coverage_e2e:: --test-threads=1

	result
clean main	21 passed; 3 failed
this branch	21 passed; 3 failed

Identical — same three tests, same three assertions, including the one CI flagged at composio_raw_coverage_e2e.rs:877. On the platform I can test, this PR changes nothing about that outcome.

Two caveats I am not going to paper over.

1. My host is Windows, and two of those three failures are plainly Windows artifacts — assertion failed: empty.archive_dir.ends_with("state/triggers") is a path-separator mismatch. So my run is contaminated, and I cannot claim from it that the CI failure is pre-existing on Linux.

2. I also checked whether main's own CI vindicates it, and it does not: the most recent green main run (33019940407) scoped to libtest filter 'openhuman::config' and never compiled the composio target at all. main being green says nothing here either way.

I was also wrong about the mechanism. I suggested this looked like an ordering/global-state dependency inside the merged raw_coverage_all binary. It is not — running that single test with --exact and no siblings fails identically on clean main. So whatever it is, co-running is not the trigger.

What stands: nothing in this diff initialises a memory client or touches composio; the diff removes a prompt flag from AgentDefinition, drops one key from agent_cli's JSON dump, and fixes three positional call sites in tests/raw_coverage/. This PR pulled the composio target into the lane's scope (via src/openhuman/tools/orchestrator_tools.rs → filter openhuman::tools) and, until my last push, the lane died at the compile error before ever reaching it.

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 memory client not initialised depends on in CI.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[tinysweeper]

How this change flows

2 changed behaviours across 1 relationship. The code graph does not know these behaviours yet — normal for newly added code, and a cold index otherwise. 73 further behaviours left out to keep the diagram readable.

flowchart LR
  n0["AgentDefinition<br/>changed"]:::changed
  n1["make_def<br/>changed"]:::changed
  n1 -->|uses| n0
  classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
  classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
  classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
  classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff

Loading

_{Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge.}

[M3gA-Mind]

Maintainer review — this one is clean; the open question is not about your code

I checked this against current main (fa044d38):

• Merges cleanly. git merge-tree --write-tree origin/main <head> returns no conflict, despite the 71-file footprint — the agent.toml deletions and the test edits have not been disturbed by the module splits that landed since.
• CI is green. Every check on run 33489490338 passes, including Rust Core Coverage, Rust Feature-Gate Smoke, Rust Quality and the PR CI Gate.
• No unresolved review threads. CodeRabbit ended APPROVED; tinysweeper APPROVED.

I also verified the central claim rather than taking it on trust. On current main the flag really is inert:

• src/openhuman/agent/prompts/builder.rs:123 takes it as _omit_skills_catalog: bool — underscore-prefixed, i.e. deliberately unread.
• The value it derives, include_skills_catalog (prompts/types.rs:441), is read only from test files. git grep include_skills_catalog origin/main returns no production reader.

So "threaded through both sub-agent prompt paths and read by neither" is accurate as stated.

The one thing blocking this is a maintainer decision, not a change to your PR

#5778 takes the opposite resolution of the same issue. #5699 explicitly offers both — honour the flag or remove it — and #5778 (opened 2026-08-25, two days before this) wires it up by reintroducing a shared SkillsCatalogSection. Only one of the two can merge.

For what it is worth, I think the argument in your description is the stronger one, and it is stronger because it is sourced from the repo rather than from taste: render_helpers.rs records that the catalogue renderer was deliberately deleted, and integrations_agent/prompt.rs pins the absence of its ## Available Skills block with a test. Honouring the flag rebuilds a layering the repo already tore down, and changes the prompt bytes for every definition that currently sets it — which has a prefix-cache cost that #5704 is separately concerned about.

The catch worth naming honestly: your PR deletes the flag from ~50 agent.toml files, so if the maintainers later decide they do want per-definition catalogue control, it comes back as a new feature rather than a one-line flip. That is the real trade-off between the two PRs, and it is theirs to make, not mine.

I am not approving this — flagging it as ready-pending-that-decision so whoever picks between #5815 and #5778 does it deliberately rather than by whichever merges first.

[M3gA-Mind]

Approved after a maintainer-side verification pass.

Verified on the current head: MERGEABLE against main, zero failing and zero pending required checks, and no unresolved, non-outdated review threads.

This is one of two required approvals; a second maintainer review is still needed before merge.