Skip to content

feat: add shared skill validation modes - #71

Open
tortmayr wants to merge 1 commit into
mainfrom
global-skills
Open

feat: add shared skill validation modes#71
tortmayr wants to merge 1 commit into
mainfrom
global-skills

Conversation

@tortmayr

Copy link
Copy Markdown
Contributor

What it does

Shared skills that are otherwise portable are currently skipped when they contain agent-specific metadata such
as Claude’s disable-model-invocation, even though the selected agent can handle them. This adds a native
validation mode that passes such skills through for the agent to interpret, while retaining strict validation
as the default.

  • add strict and native validation across CLI and config override layers
  • keep the portable subset as the strict default; native mode delegates metadata interpretation to the selected agent
  • apply validation to standalone mounts and composed tool config

How to test

Add a skill

  1. Add a shared skill under ~/.config/enclave/skills/ containing disable-model-invocation.
  2. Start Enclave with the default strict validation and verify that the skill is skipped.
  3. Set "skills_validation": "native" in Enclave’s configuration.
  4. Start Enclave again and verify that the skill is loaded unchanged.

Follow-ups

Breaking changes

  • This PR introduces breaking changes and has been coordinated with maintainers.

Review checklist

@EclipseSourceAI EclipseSourceAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note

Autonomous AI review.

This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.

Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.

To get an updated review after pushing changes, a maintainer may re-request a review from this account.

Running in Eclipse Enclave, submitted via review-guard-mcp

Adds a skills_validation run option (strict default, native) so shared skills carrying agent-specific frontmatter such as disable-model-invocation are passed through instead of skipped. Plumbing follows the existing host_config shape (option def, generated CLI/registry code, Defaults, sources) and the mode is threaded into both skill composition paths, the standalone mount and the composed tool config source. make generate produces no drift, and build plus the touched package tests pass locally.

Main points for a maintainer:

  • The mode is resolved in three places (DefaultOptions, ValidateOptions, and a raw string compare in sharedSkillValidator) and, unlike the sibling enum options, is not case-normalized. A model.SkillsValidationMode resolver like model.WorktreeMetadataMode would collapse that.
  • The use skills_validation=native hint is appended to every strict failure, including plain mistakes like a missing description, which pushes users to disable validation for the wrong reason.
  • Naming: native clashes with the meaning native already has in these docs (the tool's own on-disk config layout). Worth settling before the config value ships.
  • The mode is a session-wide switch, so one non-portable skill takes all shared skills out of validation. Fine if that is the intended trade-off, but it is the main behavioral decision in the PR.
  • Unlike host_config, skills_validation is deliberately allowed in project configs. That looks defensible since project shared skills already live in the host-managed config root and project tool-specific skills bypass validation entirely today, but it is a policy call worth a second look.
  • The branch does not merge into main: the only conflict is internal/model/option_sources_gen.go, which is regenerated content.

Comment thread internal/app/validation.go Outdated
Comment thread internal/runtime/skills_mount.go Outdated
Comment thread internal/runtime/skills_mount.go Outdated
Comment thread internal/runtime/skills_mount.go
Comment thread docs/configuration.md Outdated
Comment thread docs/configuration.md
Comment thread docs/configuration.md Outdated
Comment thread internal/config/options_def.go
@tortmayr

Copy link
Copy Markdown
Contributor Author

@EclipseSourceAI

@EclipseSourceAI EclipseSourceAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note

Autonomous AI review.

This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.

Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.

To get an updated review after pushing changes, a maintainer may re-request a review from this account.

Running in Eclipse Enclave, submitted via review-guard-mcp

Follow-up review. All eight points from the earlier review are addressed in the latest commit:

  • skills_validation is now normalized (trim/lowercase, empty falls back to strict) in normalizeOptions before the enum check in ValidateOptions, matching the other string-enum options.
  • model.SkillsValidationMode was added as the single resolver, used by both sharedSkillValidator and the option layers, mirroring the WorktreeMetadataMode pattern.
  • The "delegate to agent mode" hint is now scoped to the unsupported-frontmatter-fields error only, not appended to every strict failure (missing description, name mismatch, etc. no longer carry it).
  • The unsupported-fields error again lists the offending field names and the allowed set, instead of falling through to the raw YAML decode error.
  • The value was renamed from native to agent, avoiding the clash with the existing "native host config" terminology in the docs.
  • docs/configuration.md now states that overrides key on directory name in both modes and that agent mode can let two directories share a frontmatter name without deduplication.
  • The stray passthrough paragraph that read ambiguously next to the new option was cleaned up.
  • skills_validation was moved next to host_config_paths in OptionDefs, matching enclave config ordering.

Test coverage was extended accordingly (case-insensitivity, unknown values, hint scoping, missing-file vs invalid-frontmatter diagnostics). Build and the touched package tests pass locally. No new issues found in the diff since the last review; nothing further to flag.

These previous comments can be resolved as they are now handled:

I can't resolve them myself as I would need write permission on this repository.

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