feat: add shared skill validation modes - #71
Conversation
EclipseSourceAI
left a comment
There was a problem hiding this comment.
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 insharedSkillValidator) and, unlike the sibling enum options, is not case-normalized. Amodel.SkillsValidationModeresolver likemodel.WorktreeMetadataModewould collapse that. - The
use skills_validation=nativehint is appended to every strict failure, including plain mistakes like a missing description, which pushes users to disable validation for the wrong reason. - Naming:
nativeclashes with the meaningnativealready 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_validationis 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.
18acde9 to
a978248
Compare
EclipseSourceAI
left a comment
There was a problem hiding this comment.
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_validationis now normalized (trim/lowercase, empty falls back to strict) innormalizeOptionsbefore the enum check inValidateOptions, matching the other string-enum options.model.SkillsValidationModewas added as the single resolver, used by bothsharedSkillValidatorand the option layers, mirroring theWorktreeMetadataModepattern.- 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
nativetoagent, avoiding the clash with the existing "native host config" terminology in the docs. docs/configuration.mdnow states that overrides key on directory name in both modes and thatagentmode can let two directories share a frontmatternamewithout deduplication.- The stray passthrough paragraph that read ambiguously next to the new option was cleaned up.
skills_validationwas moved next tohost_config_pathsinOptionDefs, matchingenclave configordering.
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:
- normalize skills_validation like the other enum options
- use a model.SkillsValidationMode resolver instead of raw string comparisons
- scope the "use native mode" hint to the unsupported-field case
- keep the field list in the unsupported-frontmatter-fields error message
- rename "native" to avoid clashing with the existing native-config-layout meaning
- document that overrides still key on directory name, including the agent-mode duplicate-name case
- drop or move the passthrough paragraph that now reads ambiguously
- reorder skills_validation next to host_config_paths in OptionDefs
I can't resolve them myself as I would need write permission on this repository.
a978248 to
8634827
Compare
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 nativevalidation mode that passes such skills through for the agent to interpret, while retaining strict validation
as the default.
How to test
Add a skill
~/.config/enclave/skills/containingdisable-model-invocation."skills_validation": "native"in Enclave’s configuration.Follow-ups
Breaking changes
Review checklist