feat(skills): add auto-trigger control for bundled skills - #333
Merged
Conversation
Users reported that the bundled skills fire on their own whenever a prompt happens to match their description, and asked to be able to require naming them explicitly. No two agents spell that policy the same way, so the effective value is written into every published copy as both vendor markers, always explicit and always inverse of each other: `disable-model-invocation` in the SKILL.md frontmatter for Claude Code, and `policy.allow_implicit_invocation` in `agents/openai.yaml` for Codex. Both are written for every agent rather than gated on a per-agent capability table: the Agent Skills standard requires runtimes to ignore markers they do not recognize, and the behavior of the eight Claude Code derivatives could not be verified either way. Only directories oo already generates are touched. Codex's `config.toml` and Claude's `skillOverrides` would work too, but both are hand-edited and concurrently written by those tools' own UIs. See ADR 0002. The policy is an input to publication rather than a runtime switch, so `off`/`on` republish immediately and startup synchronization keeps its cheap short-circuit; `oo skills repair` applies a hand-edited value. `--all` is a standing policy that keeps covering bundled skills added by later releases, and `--out-dir` exports keep the shipped default so a vendored copy never carries one machine's preference. Signed-off-by: Kevin Cui <bh@bugs.cc>
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
Summary by CodeRabbit
WalkthroughAdds persisted auto-trigger settings for bundled skills, including global and per-skill disablement. New Sequence Diagram(s)sequenceDiagram
participant User
participant AutoTriggerCLI
participant SettingsStore
participant BundledSkillPublisher
participant ManagedAgent
User->>AutoTriggerCLI: Run auto-trigger off or on
AutoTriggerCLI->>SettingsStore: Persist policy
AutoTriggerCLI->>SettingsStore: Read resolved policy
AutoTriggerCLI->>BundledSkillPublisher: Publish bundled skills
BundledSkillPublisher->>ManagedAgent: Write rendered skill artifacts
ManagedAgent-->>AutoTriggerCLI: Return publication status
AutoTriggerCLI-->>User: Print state and publication results
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/application/commands/skills/auto-trigger-policy.test.ts (1)
29-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract repeated policy fixtures into a local factory.
Use a
createPolicy()helper at the bottom ofsrc/application/commands/skills/auto-trigger-policy.test.tsfor the repeated{ disabled, disabledAll }setup objects.
src/application/commands/skills/auto-trigger-policy.test.ts#L29-L30: create the standing-policy fixture through the factory.src/application/commands/skills/auto-trigger-policy.test.ts#L38-L39: create the named-skill fixture through the factory.src/application/commands/skills/auto-trigger-policy.test.ts#L92-L94: reuse the standing-policy fixture factory.src/application/commands/skills/auto-trigger-policy.test.ts#L112-L113: reuse the named-skill fixture factory.As per coding guidelines, “In test files, extract repeated setup (mock, stub, or setup objects) into a local factory function at the bottom of the file.”
🤖 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/application/commands/skills/auto-trigger-policy.test.ts` around lines 29 - 30, Extract the repeated { disabled, disabledAll } test setup into a local createPolicy() factory at the bottom of src/application/commands/skills/auto-trigger-policy.test.ts, then update the fixtures at lines 29-30, 38-39, 92-94, and 112-113 to use it while preserving their respective standing-policy and named-skill values.Source: Coding guidelines
docs/commands.md (1)
1961-1964: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the command reference at the CLI-contract level.
Remove the explanation of agents hiding skill descriptions from their context; retain only the observable result: disabled skills are not implicitly invoked, including the bundled
ooskill’s unprompted suggestions.
docs/commands.md#L1961-L1964: replace host-context mechanics with observable behavior.docs/commands.zh-CN.md#L1645-L1647: make the equivalent Chinese revision.As per coding guidelines,
docs/commands*.mdmust describe only the user-facing CLI contract, not internal implementation details.🤖 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 `@docs/commands.md` around lines 1961 - 1964, Replace the internal host-context explanation in docs/commands.md lines 1961-1964 with only the observable CLI behavior: disabled skills are not implicitly invoked, including the bundled `oo` skill’s unprompted end-of-session suggestions. Apply the equivalent Chinese revision in docs/commands.zh-CN.md lines 1645-1647, preserving the meaning while removing implementation details.Source: Coding guidelines
src/application/commands/skills/auto-trigger/index.cli.test.ts (1)
227-231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated
resolveStorePathssetup into a local helper.The same
resolveStorePaths({ appName: APP_NAME, env: sandbox.env, platform: process.platform })block appears five times; a single factory at the bottom of the file (returning store paths or the settings file path directly) removes the duplication.♻️ Suggested helper
function resolveSandboxStorePaths(sandbox: CliSandbox) { return resolveStorePaths({ appName: APP_NAME, env: sandbox.env, platform: process.platform, }); }As per coding guidelines: "In test files, extract repeated setup (mock, stub, or setup objects) into a local factory function at the bottom of the file."
Also applies to: 256-260, 344-352, 402-406, 479-483
🤖 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/application/commands/skills/auto-trigger/index.cli.test.ts` around lines 227 - 231, Extract the repeated resolveStorePaths configuration into a local resolveSandboxStorePaths helper at the bottom of the test file, accepting the sandbox and returning the resolved store paths. Replace all five duplicated setup blocks in the affected test cases with this helper, accessing settingsFilePath where needed while preserving existing behavior.Source: Coding guidelines
src/application/commands/skills/auto-trigger/on.ts (1)
12-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass the settings helper directly instead of wrapping it.
removeAutoTriggerDisabledSkillsalready matches theapplySkillssignature, so the arrow adds no logic.♻️ Proposed simplification
- applySkills: (settings, skillNames) => - removeAutoTriggerDisabledSkills(settings, skillNames), + applySkills: removeAutoTriggerDisabledSkills,As per coding guidelines: "Never create single-line functions that merely delegate to another function without adding logic, validation, or semantic value."
🤖 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/application/commands/skills/auto-trigger/on.ts` around lines 12 - 13, Update the applySkills property to reference removeAutoTriggerDisabledSkills directly instead of wrapping it in a single-line arrow function, preserving the existing settings and skillNames behavior.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 `@src/application/commands/skills/auto-sync.ts`:
- Around line 59-64: Update the Promise.all construction in the auto-sync
command so readSkillAutoTriggerPolicy starts without being awaited during
operand creation, allowing synchronizeRegistrySkills to run even if policy
loading fails. Preserve policy usage for synchronizeBundledSkills while ensuring
both synchronization paths are started independently.
In `@src/application/commands/skills/auto-trigger-policy.test.ts`:
- Around line 79-127: The tests in “renders both markers…”, “flips both
markers…”, and “flips the markers only for the named skill” hardcode the POSIX
path “agents/openai.yaml”. Import or reuse the path join utility and construct
this expected relative path with join("agents", "openai.yaml") in each affected
readMarkerContent call.
In `@src/application/commands/skills/embedded-assets.test.ts`:
- Line 210: Update the expected asset list in the embedded-assets alignment test
to retain the existing skill-authoring.md, existing-workflow.md, and
oo-powered.md reference entries alongside agents/openai.yaml, matching the
outputs emitted by embedded-assets.ts.
In `@src/application/commands/skills/install.cli.test.ts`:
- Around line 508-511: Capture the result of the `skills auto-trigger off --all`
call in the test and assert that its exitCode is 0 before setting up the export
and running `skills add`. Keep the existing environment setup and export
assertions unchanged.
---
Nitpick comments:
In `@docs/commands.md`:
- Around line 1961-1964: Replace the internal host-context explanation in
docs/commands.md lines 1961-1964 with only the observable CLI behavior: disabled
skills are not implicitly invoked, including the bundled `oo` skill’s unprompted
end-of-session suggestions. Apply the equivalent Chinese revision in
docs/commands.zh-CN.md lines 1645-1647, preserving the meaning while removing
implementation details.
In `@src/application/commands/skills/auto-trigger-policy.test.ts`:
- Around line 29-30: Extract the repeated { disabled, disabledAll } test setup
into a local createPolicy() factory at the bottom of
src/application/commands/skills/auto-trigger-policy.test.ts, then update the
fixtures at lines 29-30, 38-39, 92-94, and 112-113 to use it while preserving
their respective standing-policy and named-skill values.
In `@src/application/commands/skills/auto-trigger/index.cli.test.ts`:
- Around line 227-231: Extract the repeated resolveStorePaths configuration into
a local resolveSandboxStorePaths helper at the bottom of the test file,
accepting the sandbox and returning the resolved store paths. Replace all five
duplicated setup blocks in the affected test cases with this helper, accessing
settingsFilePath where needed while preserving existing behavior.
In `@src/application/commands/skills/auto-trigger/on.ts`:
- Around line 12-13: Update the applySkills property to reference
removeAutoTriggerDisabledSkills directly instead of wrapping it in a single-line
arrow function, preserving the existing settings and skillNames behavior.
🪄 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: a33030b3-4de6-4788-91bc-5883b56564b1
📒 Files selected for processing (34)
README-ZH_CN.mdREADME.mdcontrib/skills/shared/oo-create-skill/SKILL.mdcontrib/skills/shared/oo-create-skill/agents/openai.yamlcontrib/skills/shared/oo-find-skills/SKILL.mdcontrib/skills/shared/oo-find-skills/agents/openai.yamlcontrib/skills/shared/oo-publish-skill/SKILL.mdcontrib/skills/shared/oo-publish-skill/agents/openai.yamlcontrib/skills/shared/oo/SKILL.mdcontrib/skills/shared/oo/agents/openai.yamldocs/commands.mddocs/commands.zh-CN.mdsrc/application/commands/skills/auto-sync.tssrc/application/commands/skills/auto-trigger-policy.test.tssrc/application/commands/skills/auto-trigger-policy.tssrc/application/commands/skills/auto-trigger/index.cli.test.tssrc/application/commands/skills/auto-trigger/index.tssrc/application/commands/skills/auto-trigger/off.tssrc/application/commands/skills/auto-trigger/on.tssrc/application/commands/skills/auto-trigger/publish.tssrc/application/commands/skills/auto-trigger/report.tssrc/application/commands/skills/auto-trigger/status.tssrc/application/commands/skills/auto-trigger/toggle-command.tssrc/application/commands/skills/embedded-assets.test.tssrc/application/commands/skills/embedded-assets.tssrc/application/commands/skills/index.test.tssrc/application/commands/skills/index.tssrc/application/commands/skills/install.cli.test.tssrc/application/commands/skills/repair.tssrc/application/commands/skills/shared.tssrc/application/commands/telemetry-decisions.test.tssrc/application/schemas/settings.test.tssrc/application/schemas/settings.tssrc/i18n/catalog.ts
Awaiting `readSkillAutoTriggerPolicy()` while the caller built its `Promise.all` array meant a settings file the run could not parse stopped `synchronizeRegistrySkills()` from ever being called. `FileSettingsStore.read()` throws `errors.store.readFailed` on a corrupt file, and registry synchronization has nothing to do with the auto-trigger policy, so one unreadable file took down both halves of startup synchronization instead of one. The read moves into `synchronizeBundledSkills()`, which is the only half that needs it. Also assert the exit code of the `auto-trigger off` setup runs in two tests whose expectations *are* the resulting state. Both would have stayed green if setup had silently failed, proving nothing. Signed-off-by: Kevin Cui <bh@bugs.cc>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Users told us the bundled skills act on their own too eagerly — a prompt that merely resembles a description is enough for an agent to route through
oo.oo skills auto-trigger off --all(or naming individual skills) makes them manual-only: still installed, still invocable as/ooor$oo, just no longer volunteered.The awkward part is that no two agents spell this the same way. Claude Code reads
disable-model-invocationfrom the SKILL.md frontmatter; Codex readspolicy.allow_implicit_invocationfromagents/openai.yaml. Both markers are now written into every published copy, always explicitly and always inverse of each other, derived from one boolean inauto-trigger-policy.ts. Writing both everywhere instead of gating on a per-agent capability table is deliberate: the Agent Skills standard requires runtimes to ignore markers they don't recognize, and the behavior of the eight Claude Code derivatives couldn't be verified either way — a table would have been guesswork.agents/openai.yamlstays in the static file list and always states a value, so switching the policy never adds or removes a file from a published skill directory.Both vendors also offer a config-file route (
~/.codex/config.toml, Claude'sskillOverrides). Neither is used: those files are hand-edited and concurrently written by those tools' own UIs, sooowould be merging into a foreign format and losing races it can't detect. ADR 0002 records that trade-off.The main thing to have an opinion about while reviewing is that the policy is an input to publication, not a runtime switch.
off/onrepublish immediately, and every other republication path — upgrade, newly detected agent host,oo skills add,oo skills repair— picks it up because the renderer reads it. The deliberate gap is a hand-edited[skills.auto_trigger]: detecting that would mean parsing every published SKILL.md on everyooinvocation, sooo skills repairis the documented way to apply it. Relatedly,--allis a standing policy rather than a snapshot so bundled skills added by later releases stay covered, and--out-direxports keep the shipped default — a vendored copy shouldn't carry one machine's preference to everyone who consumes it.Worth knowing before merging: with auto-trigger off, Claude Code drops the skill's description from its context entirely, so the bundled
ooskill can no longer ask the agent to runoo skills recommend planand end-of-session suggestions stop until it's turned back on. That's inherent to the mechanism and documented alongside the command. Nothing changes by default — the feature is entirely opt-in. The nine flipped assertions inindex.test.tsare a leftover guard from the Codex removal in #254 that assertedagents/openai.yamlwas absent; they now assert it's present carrying the default policy.