feat(core): compile-time skills feature gate (#4798)#4913
Conversation
Declares the compile-time gate for the skills / skill_runtime / skill_registry domains. Default-ON so the desktop build is unchanged; slim builds opt out via --no-default-features. The dep list is intentionally empty: unlike `voice` (hound/lettre), these domains have no exclusive dependencies — every crate they touch is shared with always-on domains, and runtime_node/runtime_python are used by Agent/Flows/Memory too. The gate's value is tool-surface, prompt bloat and startup cost, not binary size.
…e carve-out (tinyhumansai#4798) Turns skills/mod.rs into an always-compiled facade: behavioural submodules are #[cfg(feature = "skills")], with a stub taking their place when the gate is off. skills is NOT a leaf — it is partly load-bearing infrastructure. tools/traits.rs re-exports the crate's unified ToolResult/ToolContent out of skills::types and ~236 files consume them; mcp_client and runtime_node import that path directly. So skills::types and skills::ops_types stay compiled in BOTH directions (inert serde/std-only definitions, zero coupling to gated siblings) and only behaviour is gated. Gating them wholesale would take down the entire tool trait system, MCP, and the Node runtime. The stub therefore mirrors FUNCTIONS ONLY and re-exports the real types, so there is zero type duplication — less drift surface than the voice stub, which had to re-declare SttResult + SttProvider.
…ature (tinyhumansai#4798) Same facade+stub shape as the skills domain — the three skill domains ship as one unit under one gate. The stub is deliberately tiny: every caller of the run machinery (spawn_workflow_run_background, await_run_outcome, WorkflowRunStarted) lives inside code gated by the same feature — the run_workflow agent tool, the openhuman.skills_* handlers, and this domain's own tests — so they vanish together. It owes only what always-on code reaches: the controller aggregators (src/core/all.rs).
…feature (tinyhumansai#4798) Same facade+stub shape. The stub mirrors only what always-on code reaches: the boot catalog refresh kicked off unconditionally by core::runtime::services, plus the controller aggregators. The catalog store, wire types and skill_setup agent are only referenced from code gated by the same feature.
…humansai#4798) Concrete tool types in a Vec can't be stubbed away, so the registrations are #[cfg]-gated: RunWorkflowTool/AwaitWorkflowTool, the Workflow* metadata tools, the SkillRegistry* tools, and SkillRuntimeResolveRuntimes. Per the DoD the tool list OMITS gated tools rather than degrading them to a disabled-error. agent/tools.rs gates `run_workflow` — it sits in the always-on agent domain but is a pure skill_runtime client. The three tools/mod.rs re-export globs are #[cfg]'d rather than backed by empty stub `tools` modules, mirroring how the voice gate handles audio_toolkit::tools::* — an empty stub module re-exports nothing and trips unused_imports at the glob site. skill_allowlist feeds only the gated registrations, so it is explicitly discarded when the feature is off.
…ls` is off (tinyhumansai#4798) Two sites the stub facade deliberately does not reach: loader.rs — the skill_setup/skill_executor BuiltinAgent entries must be #[cfg], not stubbed: include_str! embeds the agent TOML from disk regardless of module gating, so the entry itself has to disappear. task_dispatcher/executor.rs — the workflow-resolution branch. registry::get_workflow returns Option<WorkflowDefinition>, which flattens in AgentDefinition and is destructured at the call site; stubbing it would mean re-declaring that struct, which is exactly the type duplication the carve-out exists to avoid. With the domain compiled out no handle can ever resolve to a skill, so falling through to the builtin-agent branch is the correct behaviour, not a degradation.
…ai#4798) Mirrors the voice pair: with the feature on all three skill namespaces must be registered; with it off none may reach the registry (the stub-facade correctness gate). Also cfg's the pre-existing group_mapping_smoke assert for `skills` — group_for_namespace is registry-derived, so a compile-time-gated domain has no controller to map. CI cannot catch this class of break: the smoke lane runs cargo check, which never compiles test code.
…ansai#4798) openhuman_core is depended on with default-features = false, so the new default-ON gate must be named explicitly or the desktop app would ship without the skill domains. Multi-line array so sibling gates append cleanly.
…tinyhumansai#4798) Adds the gate-table row and a per-gate note covering the carve-out — the generalizable rule for the remaining gates: put a domain's inert types in a dep-free submodule and leave it ungated; stub only the behaviour. Also records why the two #[cfg] call sites can't be stubbed, and why the dep list is intentionally empty. ci-lite: no functional change. The smoke lane is an allow-list (--no-default-features --features tokenjuice-treesitter), so every new default-ON gate is exercised in its disabled direction automatically. Renames the step off "voice gate" since it now covers all of them.
tinyhumansai#4798) Three tests hard-assert skills-domain behaviour and break only under --no-default-features, which CI never test-runs (the smoke lane is cargo check, which does not compile test code): - definition_tests: the effective-max-iterations table panics with "missing built-in agent definition: skill_executor" once the skill_setup/skill_executor BuiltinAgent entries are gated out. - session::tests refresh_workflows_{picks_up,retracts}_skill_*: both exercise real SKILL.md discovery from disk, which the disabled facade answers with an empty catalog by design. Found by sweeping the full disabled --lib suite rather than the scoped core::all filter; the mcp_agent/mcp_setup rows in the same table belong to tinyhumansai#4799 and are left untouched.
…t line (tinyhumansai#4798) No functional change either way: the smoke lane's `--no-default-features --features tokenjuice-treesitter` is an explicit ALLOW-LIST, so the new default-ON `skills` gate is already exercised in its disabled direction with no workflow edit at all. tinyhumansai#4797 renamed the same step first and logged it in the coordination Conflict Log; dropping our identical-intent rename leaves a conflict-free merge. The allow-list rationale is documented durably in AGENTS.md instead.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe PR adds a default-enabled ChangesSkills feature gate
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
coderabbitai
Bot
added
agent
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb274f28db
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/openhuman/agent/harness/session/tests.rs (1)
488-492: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the disabled refresh contract explicitly.
These gates remove both tests from
--no-default-featuresbuilds. Add a#[cfg(not(feature = "skills"))]regression test (or equivalent) asserting that on-diskSKILL.mdfiles are not discovered and refresh/retraction state remains empty. The suppliedsrc/openhuman/skills/stub.rs:50-80contract should be tested, not only compiled out.As per coding guidelines, tests are required for new behavior; untested code is incomplete.
Also applies to: 558-561
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/agent/harness/session/tests.rs` around lines 488 - 492, Add a complementary #[cfg(not(feature = "skills"))] regression test alongside the existing skill discovery tests, exercising the disabled facade’s load_workflow_metadata/refresh path with an on-disk SKILL.md fixture. Assert that no skills are discovered and refresh/retraction state remains empty, covering the contract implemented by the skills stub rather than compiling the test out.Source: Coding guidelines
src/openhuman/tools/ops.rs (1)
228-231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a feature-state diagnostic for the gated skill-tool registry.
When
skillsis disabled, these registrations silently disappear; when enabled, there is no stable signal describing the resulting tool surface. Add onetracing::debug!summary per build mode (or an equivalent count) around these blocks so boot diagnostics can explain the registry contents.As per coding guidelines,
**/*.rsrequires diagnostic logging for new or changed flows, including branches and state transitions.Also applies to: 474-514
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/tools/ops.rs` around lines 228 - 231, Add a single tracing::debug! diagnostic around the skill-gated tool registration in the registry-building flow, covering both the enabled and disabled cfg(feature = "skills") build modes. Report the resulting skill-tool registration state or count, and preserve the existing RunWorkflowTool and AwaitWorkflowTool registrations unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 236-239: Update the slim-build example referenced by the voice
facade documentation so its description matches the enabled features: either add
skills to the --no-default-features feature list or revise the “everything
except voice” wording to acknowledge that skills are also disabled. Keep the
voice stub guidance consistent with the resulting build command.
In `@app/src-tauri/Cargo.toml`:
- Around line 135-137: Update the openhuman_core dependency feature list in
Cargo.toml to also enable the core crate’s tokenjuice-treesitter and voice
features alongside skills, so the Tauri build matches the intended default
feature set.
In `@src/openhuman/skill_runtime/mod.rs`:
- Around line 8-16: Update the module-level documentation near the compile-time
gate description to remove the cfg-sensitive rustdoc link to stub, replacing it
with plain code formatting or feature-specific documentation while preserving
the existing explanation.
In `@src/openhuman/skills/mod.rs`:
- Around line 5-11: Update the module-level documentation in the skills facade
to remove the claim that the disabled stub exposes skills::tools, while
preserving the accurate list of other always-available exports and the
no-op/empty behavior description.
In `@src/openhuman/skills/stub.rs`:
- Around line 61-68: In src/openhuman/skills/stub.rs lines 61-68, add stable
debug logs to both all_skills_registered_controllers and
all_skills_controller_schemas before returning empty results; in
src/openhuman/skills/stub.rs lines 94-96, log the cleanup subscriber no-op; and
in src/openhuman/skill_registry/stub.rs lines 18-28, add corresponding debug
logs to both empty controller/schema aggregators. Keep the existing no-op return
behavior unchanged.
---
Nitpick comments:
In `@src/openhuman/agent/harness/session/tests.rs`:
- Around line 488-492: Add a complementary #[cfg(not(feature = "skills"))]
regression test alongside the existing skill discovery tests, exercising the
disabled facade’s load_workflow_metadata/refresh path with an on-disk SKILL.md
fixture. Assert that no skills are discovered and refresh/retraction state
remains empty, covering the contract implemented by the skills stub rather than
compiling the test out.
In `@src/openhuman/tools/ops.rs`:
- Around line 228-231: Add a single tracing::debug! diagnostic around the
skill-gated tool registration in the registry-building flow, covering both the
enabled and disabled cfg(feature = "skills") build modes. Report the resulting
skill-tool registration state or count, and preserve the existing
RunWorkflowTool and AwaitWorkflowTool registrations unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0cb23710-9830-45af-ac4b-26efb7fb1f00
📒 Files selected for processing (17)
AGENTS.mdCargo.tomlapp/src-tauri/Cargo.tomlsrc/core/all_tests.rssrc/openhuman/agent/harness/definition_tests.rssrc/openhuman/agent/harness/session/tests.rssrc/openhuman/agent/task_dispatcher/executor.rssrc/openhuman/agent/tools.rssrc/openhuman/agent_registry/agents/loader.rssrc/openhuman/skill_registry/mod.rssrc/openhuman/skill_registry/stub.rssrc/openhuman/skill_runtime/mod.rssrc/openhuman/skill_runtime/stub.rssrc/openhuman/skills/mod.rssrc/openhuman/skills/stub.rssrc/openhuman/tools/mod.rssrc/openhuman/tools/ops.rs
…ature-gate # Conflicts: # AGENTS.md # Cargo.toml # app/src-tauri/Cargo.toml
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85f6db6dc2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ature-gate # Conflicts: # Cargo.toml # app/src-tauri/Cargo.toml # src/core/all_tests.rs
coderabbitai
Bot
removed
the
agent
The skills disabled-facade stub deliberately has no `tools` module (the `skills::tools` glob is `#[cfg(feature = "skills")]` at its re-export site in `tools/mod.rs`, mirroring the voice gate). The module doc listed `tools` as part of the stub surface, which was inaccurate. Addresses CodeRabbit review.
|
Maintainer review changes + CodeRabbit disposition. Change pushed (
CodeRabbit comments — disposition: Fixed
Declined, with reason
|
|
Maintainer sync: merged current |
|
@coderabbitai review The head has moved on since the standing review: every actionable comment has been addressed (fixed, or declined in-thread with reasoning), and the branch has since been re-synced onto current |
|
✅ Action performedReview finished.
|
|
Maintainer conflict resolution: merged current main (now includes #4915, the meet gate #4800) into this branch. All conflicts were the expected mechanical drift from two sibling gates in the #4797–#4800 series editing the same shared registration sites — resolved by union (both gates kept), file by file:
No blanket --ours/--theirs; each hunk read and merged by hand. Semantic check (a textually clean merge can still be broken): The two gates target disjoint domains ( |
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Approving — both conditions genuinely met.
CI: fully green. 13 pass, 5 skipped by changed-area detection (correct for a Rust-only change), zero pending, zero failing. The lanes that matter here all pass: Rust Quality (fmt, clippy), Rust Feature-Gate Smoke (gates off) — the only lane that ever compiles stub.rs files and therefore the sole guard against stub drift — plus Rust Core Coverage, Rust Tauri Coverage, and PR CI Gate.
Review state: no CHANGES_REQUESTED. CodeRabbit's earlier block was stale (raised ~18h and several commits before the current head); it has since re-reviewed and approved.
Verified beyond CI:
- The skills gate follows the established
voice/web3facade+stub pattern; default build is byte-identical. - One real doc defect was found and fixed (
aec00afe7): the facade module doc claimedtoolswas part of the disabled-stub surface, but the stub deliberately has notoolsmodule (the glob is#[cfg(feature="skills")]at its re-export site, mirroringvoice). Wording corrected — zero behaviour change. - The six declined CodeRabbit items were each grounded in the already-merged
voice/web3pathfinder gates or explicit out-of-scope, and answered in-thread rather than silently dropped. - Re-synced onto current
main(includes #4915's meet gate); the resulting conflicts were resolved to the union — both gates' rows, feature lines, and registration entries retained.
Not merging — that is the maintainer's call.
…conflicts Both sibling gates append their feature to the default set and add docs; kept both: default now includes skills (from tinyhumansai#4913, merged) and flows (this PR). AGENTS.md feature table + gate-pattern docs keep both the skills and flows sections.
4 participants
Summary
skillsfeature (default-ON) gating theopenhuman::skills+skill_runtime+skill_registrydomains, composing with the runtimeDomainSet::skillsflag from feat(core): DomainSet on CoreBuilder + DomainRegistration filter seam (feature-gate prerequisite) #4796.cargo build --no-default-features --features "<gates without skills>".skillsis not a leaf — it is partly load-bearing infrastructure.tools/traits.rsre-exports the unifiedToolResult/ToolContentout ofskills::types, and 236 files depend on them. Gating the domain wholesale would take down the entire tool trait system.skills::typesandskills::ops_typesstay unconditionally compiled; only behaviour is gated.skills = []). The value is LOC, RPC surface, tool-belt size and attack surface — not binary size.Problem
#4795 wants one compile-time feature gate per subsystem family; #4796 shipped the runtime
DomainSetaxis. This PR adds the compile-time half for the skills family.The blocking discovery:
skillsis not a self-contained leaf. The unified tool-result types (ToolResult/ToolContent) live inskills::types, andtools/traits.rsre-exports them as the canonical path. 236 files consume that path, andmcp_clientandruntime_nodeimport it directly. A naive#[cfg]overpub mod skillsdeletes those types and fails the build of every tool in the crate — the exact "toggling the gate must not break the core" constraint from the epic.Second, the epic's
## Definition of Donecarries two bullets that do not apply here:DomainSetflag onCoreBuilder" — already landed by feat(core): DomainSet on CoreBuilder + DomainRegistration filter seam (feature-gate prerequisite) #4796 (PR feat(core): runtime DomainSet composition axis — group-tagged registry + ambient filter (#4796) #4808) for every gate. This PR is compile-time only.skills = []is correct and intentional.Solution
Cargo.toml:skills = []added todefault— dep list intentionally empty, with a comment saying so, so nobody "fixes" it later.skills::types+skills::ops_typesstay unconditionally compiled — they are inert serde types with no behavioural deps. Only the behaviour behind them is gated. The stub therefore mirrors functions only, never types, which is strictly less drift surface than mirroring both: a field added toToolResultcannot desync across configs because there is exactly one definition.skills/,skill_runtime/,skill_registry/):pub modstays always-compiled, real submodules go behind#[cfg(feature = "skills")], and eachstub.rsre-exposes the always-on caller surface with disabled-error / empty bodies. Aggregator entry points return empty, socore/all.rsneeds no call-sitecfg.get_workflowcould not be stubbed. It returns a type that#[serde(flatten)]s and is destructured by its caller, so a stub return value cannot satisfy the caller's shape. The dispatch branch is gated instead (agent/task_dispatcher/executor.rs).app/src-tauri/Cargo.toml):openhuman_coreisdefault-features = false, soskillsis added explicitly to keep the domain in the shipped desktop app. Verified viacargo tree -e features -i openhuman.rust-feature-gate-smokelane is a deny-by-default allow-list (--no-default-features --features tokenjuice-treesitter), so a new default-ON gate is excluded automatically and the disabled combination is already covered.AGENTS.md): gate-table row + the generalizable lesson — put inert serde types in a dep-free submodule, leave it ungated, and stub only behaviour.When off: the
skills/skill_runtime/skill_registrynamespaces are absent from/schemaand their methods return a clean JSON-RPC unknown-method (-32000); the 16 skill tools are absent; the tool trait system still compiles and works.Submission Checklist
core/all_tests.rs, plus skills-dependent harness tests cfg'd for the disabled build.#[cfg]attrs, a Cargo feature, stub facades, docs);diff-coverreports no coverable lines. The directional gate tests cover the on/off behaviour. — Diff coverage ≥ 80%## Relatedskills = [].docs/RELEASE-MANUAL-SMOKE.md)Closes #NNNin the## RelatedsectionImpact
skillson and the build is byte-identical.SKILL.mdexecution — a meaningful attack-surface reduction for headless deployments.Related
DomainSetseam (PR feat(core): runtime DomainSet composition axis — group-tagged registry + ambient filter (#4796) #4808); follows the feat(core): feature gate — voice #4803 voice pathfinder and feat(core): feature gate — media generation #4804 media leaf gate (PR feat(core): compile-time media feature gate (media_generation + image) (#4804) #4840). Sibling gates: feat(core): feature gate — flows/workflows #4797 (flows), feat(core): feature gate — MCP #4799 (mcp), feat(core): feature gate — meet #4800 (meet), feat(core): feature gate — web3 #4802 (web3, PR feat(web3): compile-time web3 feature gate for wallet/web3/x402 (#4802) #4855).voiceandtokenjuice-treesitterare currently not forwarded inapp/src-tauri/Cargo.toml(whoseopenhuman_coredep isdefault-features = false), so those two domains are missing from the shipped desktop app today. Pre-existing regression, out of scope here — being fixed epic-level.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
feat/4798-skills-feature-gatecb274f28dba3b0f518366a6a81745bd784ebeb3e(11 commits, 17 files, +430/-8)Validation Run
pnpm --filter openhuman-app format:checkpnpm typecheckcargo fmtclean · enabledcargo check0 errors · disabledcargo check --no-default-features --features tokenjuice-treesitter0 errors · clippy clean in both directions.app/src-tauri/Cargo.toml(one dep line), no Tauri Rust source touched — forwarding verified viacargo tree -e features -i openhuman— Tauri fmt/check (if changed)Boot smoke (both directions, 2 runs for repeatability) — both configs built, linked, booted, and served
/rpc:/schema; probeopenhuman.skills_list→{"result":{"workflows":[]}}.-32000); the process survives — and the tool trait system still works with skills off, which is what proves the type carve-out.Validation Blocked
command:cargo test --no-default-featureserror:2 pre-existing failures onmaintoday —group_mapping_smokeandharness_excludes_gated_namespaces, both asserting thevoicenamespace uncfg'd (fallout from the merged voice gate feat(core): feature gate — voice #4803). CI cannot see them: the feature-gate smoke lane runscargo check, which never compiles test code.impact:Not introduced by this PR and not fixed by it. This PR cfg'd its own asserts and left voice's alone; the voice asserts are a separate, epic-level fix that is pending. Scoped enabled-config tests and both-directioncargo checkare green.Behavior Changes
skillscompile-time gate. Default build unchanged.Parity Contract
/rpc); gate OFF yields absence at every registration site, with stubs returning disabled-errors on the always-on caller surface.skills::types/::ops_typeshave a single definition shared by both configs, so field drift between on/off is structurally impossible. Composes with, and does not replace, the runtimeDomainSet::skillsguard from feat(core): DomainSet on CoreBuilder + DomainRegistration filter seam (feature-gate prerequisite) #4796.Duplicate / Superseded PR Handling
Summary by CodeRabbit