Skip to content

feat(skills): advanced skills system with conditional activation, forked execution, and visibility controls#67

Open
toreleon wants to merge 4 commits into
mainfrom
worktree-feat+advanced-skills-system
Open

feat(skills): advanced skills system with conditional activation, forked execution, and visibility controls#67
toreleon wants to merge 4 commits into
mainfrom
worktree-feat+advanced-skills-system

Conversation

@toreleon

@toreleon toreleon commented Apr 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Enhanced frontmatter parser: FrontmatterValue enum supporting scalars, inline arrays [a, b], block arrays (- item), and boolean parsing
  • Extended SkillMetadata: 7 new fields (paths, context, effort, allowed_tools, disable_model_invocation, user_invocable, hooks) and 3 new enums (SkillContext, SkillEffort, SkillHookDef)
  • Conditional activation via paths globs: Skills with path patterns stay hidden until matching files are touched in the session. Uses globset for efficient multi-pattern matching
  • Forked execution context: Skills with context: fork return metadata for the ReAct loop to spawn isolated sub-agents with effort-based step limits and tool restrictions
  • Token budgeting & visibility controls: Budgeted skill index (1% of context window), 3-tier descriptions (builtin full / non-builtin truncated / overflow name-only), disable-model-invocation hides from model, user-invocable controls slash command access
  • Embedded hooks & runtime variables: Skills can define lifecycle hooks in frontmatter, runtime variable substitution for ${SKILL_DIR}, ${SESSION_ID}, ${WORKING_DIR} in both {{var}} and ${VAR} syntax

Example skill using all features

---
name: typescript-fixer
description: "Use when TypeScript compilation fails"
namespace: typescript
context: fork
effort: high
paths: ["**/*.ts", "**/*.tsx"]
allowed-tools: [Read, Edit, Bash, Grep]
disable-model-invocation: false
user-invocable: true
hooks: ["PostToolUse:Edit:npx tsc --noEmit 2>&1 | head -20"]
---

Test plan

  • 99 skills tests pass in opendev-agents (parsing, metadata, loader, discovery)
  • 31 invoke_skill tests pass in opendev-tools-impl (fork, visibility, hooks)
  • cargo clippy -p opendev-agents -p opendev-tools-impl -- -D warnings passes
  • cargo check -p opendev-tools-impl passes (full downstream)
  • cargo build --release -p opendev-cli succeeds

🤖 Generated with Claude Code

toreleon added 4 commits April 9, 2026 02:32
…ation, forked execution, and visibility controls

Add 6 major features to the skills system to surpass Claude Code's capabilities:

1. Enhanced frontmatter parser: FrontmatterValue enum supporting scalars,
   inline arrays [a, b], and block arrays (- item). Boolean parsing.
2. Extended SkillMetadata: 7 new fields (paths, context, effort,
   allowed_tools, disable_model_invocation, user_invocable, hooks) and
   3 new enums (SkillContext, SkillEffort, SkillHookDef).
3. Conditional activation via paths globs: skills with path patterns stay
   hidden until matching files are touched. Uses globset for efficient
   multi-pattern matching.
4. Forked execution context: skills with context:fork return metadata
   instructing the ReAct loop to spawn an isolated sub-agent with
   effort-based step limits and tool restrictions.
5. Token budgeting and visibility controls: budgeted skill index (1% of
   context window), tiered descriptions (builtin full, non-builtin
   truncated, overflow name-only), disable-model-invocation hides from
   model, user-invocable controls slash command access.
6. Embedded hooks and runtime variables: skills can define lifecycle hooks
   in frontmatter, runtime variable substitution for SKILL_DIR, SESSION_ID,
   WORKING_DIR in both {{var}} and ${VAR} syntax.
Forked skills (context: fork) were setting skill_fork metadata but
nothing in the ReAct loop consumed it, causing the skill content to
be injected inline with no iteration limit — leading to infinite
loops (3000+ iterations observed).

Now tool_dispatch detects skill_fork: true in Skill tool results and
automatically spawns a General subagent with the skill content as the
task, bounded by the agent's max_steps limit (25 by default).
The /skills command was hardcoded to always return "No custom skills
configured". Now it scans .opendev/skills/ (project) and
~/.opendev/skills/ (global), parses frontmatter, and lists skills
with their namespace, description, context type, and source.

@bdqnghi bdqnghi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this — there's a lot of solid work here, and the frontmatter parser upgrade, the extended SkillMetadata, and the visibility filtering are close to mergeable. But as a single PR it can't land yet, for three reasons:

1. Three advertised features ship with no production wiring. notify_file_touched (conditional activation) has no caller; invocation_source (the user-invocation escape hatch) is never set; skill_hooks metadata is parsed but never executed. This is worse than inert: a paths-scoped skill becomes invisible in the Skill tool list and suggestions with no way to activate it, and a disable_model_invocation skill becomes uninvokable by anyone, since the user path that should bypass the filter doesn't exist yet.

2. The forked-execution path drops its own contract. The fork ignores the skill's effort, max_steps, and allowed_tools settings, and it bypasses the dispatch-level fast-abort — cancellation still propagates cooperatively via the child token, but the "Interrupted by user" result path never fires, so a cancelled fork reports as if it completed.

3. Two concrete bugs: expand_dollar_variables matches on substring boundaries (e.g. $FOO also expands inside $FOOBAR), and truncate_desc slices at a byte offset and will panic on multi-byte UTF-8.

Suggested split: (a) parser + SkillMetadata fields + visibility filtering + tests, with the two bugs fixed — that can merge quickly; (b) forked execution and hooks as a follow-up once the activation/invocation wiring exists end-to-end. Happy to review both promptly. Note the branch is also well behind main now and will need a rebase.

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