Skip to content

fix(chat): honor [skills] enabled=false at runtime#663

Merged
penso merged 3 commits intomainfrom
precious-tibia
Apr 11, 2026
Merged

fix(chat): honor [skills] enabled=false at runtime#663
penso merged 3 commits intomainfrom
precious-tibia

Conversation

@penso
Copy link
Copy Markdown
Collaborator

@penso penso commented Apr 11, 2026

Summary

Closes #655.

SkillsConfig.enabled is documented and defaulted to true in the schema (crates/config/src/schema.rs:1275), but the runtime never consulted it — FsSkillDiscoverer::default_paths() was invoked unconditionally from every chat.rs callsite, so every discovered SKILL.md landed in the system prompt regardless of [skills] enabled = false. This is the same failure mode as #638 / #639: schema promises behavior the code doesn't deliver.

  • Added discover_skills_if_enabled(&MoltisConfig) helper in crates/chat/src/lib.rs that returns Vec::new() when config.skills.enabled is false, otherwise delegates to FsSkillDiscoverer.
  • Routed all four FsSkillDiscoverer usages inside LiveChatService (send path, model-probe path, and the two load_prompt_persona_for_session paths) through the helper. The first two thread a fresh moltis_config::discover_and_load() (matching surrounding patterns); the other two reuse the already-loaded persona.config.

Non-chat callsites (cli, gateway/services, gateway/server, skills/watcher, tools/skill_tools, httpd/server) are intentionally out of scope — they don't inject skills into the LLM prompt. Can be a follow-up if full coverage is desired.

Validation

Completed

  • cargo check -p moltis-chat
  • cargo +nightly-2025-11-30 fmt --all -- --check
  • cargo +nightly-2025-11-30 clippy -p moltis-chat -p moltis-config -p moltis-skills --all-targets -- -D warnings
  • cargo test -p moltis-chat --lib → 175 passed, 0 failed (includes two new regression tests)

Remaining

  • just lint (full Darwin clippy sweep)
  • just test (full test suite)
  • ./scripts/local-validate.sh <PR>

Manual QA

  1. Add to moltis.toml:
    [skills]
    enabled = false
  2. Place a SKILL.md under <data_dir>/skills/test-skill/SKILL.md.
  3. Start a session and inspect the system prompt / <available_skills> block.
  4. Expected: no skills discovered, no <available_skills> entries. Flip enabled = true and confirm the skill reappears.

Tests

Two new #[tokio::test]s in crates/chat/src/lib.rs:

  • discover_skills_if_enabled_short_circuits_when_disabled — plants a real SKILL.md under a temp data_dir and asserts the helper returns an empty list when enabled = false, proving we don't fall through to the filesystem.
  • discover_skills_if_enabled_runs_discoverer_when_enabled — the complementary arm, asserting the planted skill is discovered when enabled = true.

Tests share a module-local tokio::sync::Mutex so they serialize their mutations to the global moltis_config::set_data_dir override.

Closes #655. SkillsConfig.enabled was documented and defaulted to true in
the schema but never consulted at runtime — skills were always discovered
and injected into the LLM prompt. Add a discover_skills_if_enabled helper
that short-circuits to an empty Vec when the flag is false, and route all
chat.rs discovery callsites through it so operators who set
`[skills] enabled = false` get zero skills in the prompt.

Entire-Checkpoint: f3c67e202c0b
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 11, 2026

Greptile Summary

This PR fixes #655 by adding a discover_skills_if_enabled async helper that returns an empty vec when config.skills.enabled is false, and routes all four FsSkillDiscoverer call sites in LiveChatService (send path, model-probe path, two persona paths) through it. Previous review feedback on Mutex<()> (replaced with Semaphore::const_new(1)) and the redundant discover_and_load() on the model-probe path have both been addressed in follow-up commits.

Confidence Score: 5/5

Safe to merge — the fix is correct, both code arms are tested, and prior review concerns are resolved.

All four callsites correctly route through the new helper; the semaphore replaces the forbidden Mutex<()>; the redundant discover_and_load() in the model-probe path was cleaned up in the final commit. No P0/P1 issues remain.

No files require special attention.

Important Files Changed

Filename Overview
crates/chat/src/lib.rs Adds discover_skills_if_enabled helper that gates skill discovery on config.skills.enabled, routes all four FsSkillDiscoverer call sites through it, and adds two regression tests using Semaphore::const_new(1) for proper test serialization.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[chat request arrives] --> B{send / model-probe / persona path}
    B --> C[call discover_skills_if_enabled\n&config]
    C --> D{config.skills.enabled?}
    D -- false --> E[return Vec::new\nno skills injected into prompt]
    D -- true --> F[FsSkillDiscoverer::default_paths\nmoltis_config::data_dir]
    F --> G[FsSkillDiscoverer::discover]
    G --> H{discover result}
    H -- Ok --> I[return Vec of SkillMetadata]
    H -- Err --> J[warn! + return Vec::new]
    I --> K[inject into system prompt\navailable_skills block]
    E --> L[skip available_skills block]
Loading

Reviews (3): Last reviewed commit: "fix(chat): reuse existing config in cont..." | Re-trigger Greptile

Per CLAUDE.md "mutex must guard actual state", a bare `Mutex<()>` is
forbidden. The tests only need serialization of the global data_dir
override, not guarded state — use `tokio::sync::Semaphore` with a
single permit, which is the idiomatic serialization primitive.

Addresses Greptile P2 feedback on #663.

Entire-Checkpoint: 76d5468681db
@codspeed-hq
Copy link
Copy Markdown
Contributor

codspeed-hq bot commented Apr 11, 2026

Merging this PR will not alter performance

✅ 39 untouched benchmarks
⏩ 5 skipped benchmarks1


Comparing precious-tibia (69a6141) with main (5afb1bc)

Open in CodSpeed

Footnotes

  1. 5 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 11, 2026

Codecov Report

❌ Patch coverage is 82.08955% with 12 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/chat/src/lib.rs 82.08% 12 Missing ⚠️

📢 Thoughts on this report? Let us know!

The model-probe/context path already loads `moltis_config::discover_and_load()`
into a local `config` binding; the skill discovery call was loading it a
second time unnecessarily. Reuse the existing binding.

Addresses Greptile P2 feedback on #663.

Entire-Checkpoint: f63026f15bae
@penso penso merged commit a7b3801 into main Apr 11, 2026
40 checks passed
@penso penso deleted the precious-tibia branch April 11, 2026 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: [skills] enabled config field is documented but never read at runtime

1 participant