fix(skills): make the skill-pack globs mean what an operator writes - #940
Open
brahyam wants to merge 1 commit into
Open
fix(skills): make the skill-pack globs mean what an operator writes#940brahyam wants to merge 1 commit into
brahyam wants to merge 1 commit into
Conversation
config.skillGlobs was matched against the skill's DIRECTORY, never against the path of the SKILL.md that defines it. So the natural way to write the pattern — skills/*/SKILL.md, the path the glob appears to be filtering — matched nothing, every skill was skipped, and the catalog reported total 0 with no error. The pack read as empty rather than misconfigured. A trailing /SKILL.md is now stripped from the glob and the directory matched. Normalizing the pattern rather than ALSO matching the file path is the point: matching both widens every directory-shaped glob, because skills/*/* — written to select skills/<team>/<skill> — also matches skills/<team>/SKILL.md as a file path. An imported pack skill injects instructions into every session in its target scope, so an allowlist admitting a skill it did not name is not only untidy. exclude gets the same normalization and the same directory comparison, for both halves of that reasoning. It was failing open — collectSharedBundle filters bundle files through isExcludedPath, which walks ancestor prefixes, while this path compared the skill directory exactly, so exclude: ["trusted"] dropped every bundle file under trusted/ and imported the skill sitting in it. But matching the raw SKILL.md path instead would fail the other way: exclude: ["**/*.md"], which an operator writes to strip documentation out of the shared bundle, would drop every skill, and skills/*/* would reach a skill one level up. Neither is a missing import — a skill that never becomes a candidate is absent from kept, so archiveRemoved archives every already-imported skill of the pack on the next sync. An empty skillGlobs now means unset rather than "nothing". asConfig sets it to [] whenever the client sends an array — a cleared field, or an array of non-strings. An empty array is truthy and matches nothing, so every skill was dropped and the same archive cascade followed: a cleared field silently uninstalled the pack. A bare SKILL.md glob selects the repository-root skill, which planIngest supports and which has no directory part to strip. counts.filtered reports how many SKILL.md files either list dropped before any other classification. `total 0, filtered 7` says the filter is the cause; `total 0, filtered 0` says the repository has no skills. Those were indistinguishable, which is what made the first failure hard to find. It reaches the API and recordImport but not the admin, whose catalog renders a fixed pill order — that is a visible change and is left for a follow-up that can carry a demo.
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
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.
What
config.skillGlobson a skill pack was matched against the skill's directory, never against the path of theSKILL.mdthat defines it:So the natural way to write the pattern —
skills/*/SKILL.md, which is the path the glob appears to be filtering — matched nothing, every skill was skipped, and the catalog reportedtotal 0with no error. The pack read as empty rather than as misconfigured.Four changes, three of which are failures found in the same guard.
1. A trailing
/SKILL.mdis stripped from the glob, and the directory is matched. Normalizing the pattern rather than also matching the file path matters: the OR form would widen every directory-shaped glob, becauseskills/*/*— written to selectskills/<team>/<skill>— also matchesskills/<team>/SKILL.mdas a file path. An imported pack skill injects instructions into every session in the target scope, so an allowlist admitting a skill it did not name is not only untidy.2.
excludegets the same leniency, because as a denylist it was failing open. It also meant two different things in one config:collectSharedBundlefilters bundle files throughisExcludedPath, which walks ancestor prefixes, while this path compared the skill directory exactly.exclude: ["trusted"]therefore dropped every bundle file undertrusted/and imported the skill sitting in it, andexclude: ["trusted/*/SKILL.md"]matched nothing at all. Both call sites now go throughisExcludedPath, which is one rule instead of two.3. An empty
skillGlobsmeans unset rather than "nothing".asConfigsetscfg.skillGlobs = []whenever the client sends an array — a cleared field, or an array of non-strings. An empty array is truthy andmatchesAny(x, [])is false, so every skill was dropped; on the next syncimportPackreturned an emptykeptandarchiveRemovedarchived every skill the pack had ever published into each target scope. A cleared field silently uninstalled the pack.4.
counts.filteredreports how manySKILL.mdfiles were dropped by either list before any other classification.total 0, filtered 7says the filter is the cause;total 0, filtered 0says the repository has no skills. Those were indistinguishable, which is what made the first failure hard to find.5.
excludeis normalized the same way, and matched against the directory. Routing it throughisExcludedPathon the rawSKILL.mdpath would widen it exactly as matchingskillGlobsagainst that path would:exclude: ["**/*.md"]— an operator stripping documentation out of the shared bundle — dropped 2 of 3 skills in a test repository, andskills/*/*reached a skill one level up through its manifest. That is not a missing import: a skill that never becomes a candidate is absent fromkept, soarchiveRemovedarchives every already-imported skill of the pack on the next sync.6. A bare
SKILL.mdglob selects the repository-root skill.planIngestsupports a manifest at the root, and the glob an operator would write for it has no directory part to strip.Deliberately not included
counts.filteredreaches the API and not the admin. The pack catalog renders a fixedPILL_ORDERwith nofilteredentry, so an operator still sees an empty candidate list with no explanation. Adding"filtered"toPILL_ORDERand aREASONentry is enough —pillCountalready falls through tocounts[key]andpillShownalready hides a zero — but it should not be clickable, because filtering to it shows an empty table: those skills never became candidates.That is a visible change to the admin and this repository asks for a demo with one. It is left out rather than shipped undemonstrated, and is worth a follow-up — two review passes asked for it, so treat it as wanted rather than optional. The count is not dead in the meantime: it is in the catalog response and is persisted by
recordImport, so a sync's own record says what its filters dropped.Verified
test/ingest.test.tscovers a glob ending in/SKILL.md, a directory glob (unchanged), a glob matching neither, a directory-shaped glob that must not be widened, an emptyskillGlobs,excludeby ancestor and by manifest path, and thefilteredcount separating a filtered pack from an empty one.npm run typecheck, lint,prettier --check, and the ingest, sync-engine, pack-store, bundle and skill-pack-routes suites pass.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.