Skip to content

Add shell-based predicates for plugin/hook/skill/mcp use#227

Open
jackh726 wants to merge 2 commits into
mainfrom
shell-predicates
Open

Add shell-based predicates for plugin/hook/skill/mcp use#227
jackh726 wants to merge 2 commits into
mainfrom
shell-predicates

Conversation

@jackh726
Copy link
Copy Markdown
Contributor

What does this PR do?

Adds an optional shell_predicates field — a list of shell commands that must all exit 0 for the enclosing item to be active.

Disclosure questions

AI disclosure.

  • The AI tool authored large parts of the code

Copy link
Copy Markdown
Member

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't read the code yet -- some comments on the docs.

I have two big concerns: (1) the interactions with Windows and (2) the way that these predicates rely on random commands being available on PATH. It seems like asking for trouble. On the flip side, maybe this is a just a matter of enforcing quality criteria on what goes into the recommendations repo. It seems handy for people adding custom plugins and what not.

Another thing I'm wondering: I don't love the idea of having an ever-growing number of fields (crates, shell-predicates, the-next-kind-of-predicate-we-add). I guess right now the semantics is that all must match at every level, but I think at some point we're going to want to go for "where clauses" that have a little grammar and are more readily extensible. I am wary of doing the thing where we add ever-growing bits of declarative syntax that never actually give people the full expressive power they want (e.g., the ability to express "any" predicates, or saysomething like "either this crate OR that crate and this shell predicate").

|-------|------|----------|-------------|
| `name` | string | yes | Plugin name. Used in logs and CLI output. |
| `crates` | string or array | no | Which crates this plugin applies to. Use `["*"]` for all crates. See [Plugin-level filtering](#plugin-level-filtering). |
| `shell_predicates` | array of strings | no | Shell commands that must all exit 0 for the plugin to apply. See [Shell predicates](./shell-predicates.md). |
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| `shell_predicates` | array of strings | no | Shell commands that must all exit 0 for the plugin to apply. See [Shell predicates](./shell-predicates.md). |
| `shell-predicates` | array of strings | no | Shell commands that must all exit 0 for the plugin to apply. See [Shell predicates](./shell-predicates.md). |

I feel like this is more "TOML-idiomatic"?

|-------|-----------|
| Plugin `shell_predicates` | At sync (gates skills & MCP) and at every hook dispatch |
| Skill group `shell_predicates` | At sync, before any git/crates source is fetched |
| Skill frontmatter `shell_predicates` | At sync, after the skill loads |
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what "after the skill loads" means. My expectation is that these predicates would be used to decide whether to "install" the skill and make it available to the agent. After the skill loads sounds to me like it would be used when the agent is activating the skill or something like that.

This suggests to me that we should make a 'lifecycle' page in the reference that gives standard terms we can reference.

name: my-skill
description: Skill that depends on ripgrep
crates: serde
shell_predicates: command -v rg, test -f Cargo.toml
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: If front-matter is YAML, should we make this a YAML array? (I suppose the same could apply to crates) It seems more natural to me. Perhaps the answer is that you should have an option.

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.

2 participants