feat: make skill discovery nested, feat: improve codex installs and platform detection#46
Conversation
Minoo7
changed the title
Minoo7
changed the title
There was a problem hiding this comment.
Pull request overview
Improves support for discovering, converting, and installing skills that live under nested skills/ directories (e.g., config/skills/...), preventing duplicated path prefixes during conversion/install, and adds regression tests to cover these cases.
Changes:
- Expand marker-based skill discovery to find
SKILL.mdunder any nestedskills/ancestor (not just<root>/skills). - Fix recursive glob target mapping + conversion path derivation to avoid duplicated prefixes like
skills/config/skills/.... - Add unit/integration regression tests for nested discovery, conversion mapping, recursive glob anchoring, and end-to-end plugin skill install.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/run-tests.ts | Registers new unit + integration tests in the test runner. |
| tests/integration/nested-plugin-skills.test.ts | Adds end-to-end install coverage for nested config/skills/... in a Claude plugin package. |
| tests/core/platforms/converter.test.ts | Adds a converter regression test ensuring nested skills paths are anchored correctly. |
| tests/core/install/nested-resource-discovery.test.ts | Adds tests for nested skill discovery and convenience filtering. |
| tests/core/flows/recursive-glob.test.ts | Adds regression coverage for recursive glob anchoring with nested skills sources. |
| platforms.jsonc | Updates Claude-plugin import flows to match nested resources (**/skills/**/*, etc.) and adds nested AGENTS.md import mapping. |
| packages/core/src/core/install/strategies/conversion-strategy.ts | Normalizes matchedPattern after conversion to keep resource filtering aligned with canonical paths. |
| packages/core/src/core/install/resource-search.ts | Introduces a generic marker search that scans for marker files under nested resource dir ancestors. |
| packages/core/src/core/install/resource-pattern-normalization.ts | Adds normalization logic to anchor converted matched patterns at canonical installable type dirs. |
| packages/core/src/core/install/resource-discoverer.ts | Switches skill discovery to use the new marker-file search (nested-aware). |
| packages/core/src/core/install/import-flow-converter.ts | Updates path transformation to use glob matching + recursive glob target mapping for ** patterns. |
| packages/core/src/core/glob-target-mapping.ts | Adjusts recursive glob mapping to anchor nested sources to the destination resource dir and avoid duplicated prefixes. |
| packages/core/src/core/flows/platform-converter.ts | Fixes conversion stage execution to derive targets using the original from pattern (avoids skills/config/skills/...). |
| package-lock.json | Lockfile updated (project version bump and dependency resolution changes). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await fs.mkdir(path.join(workspace, '.cursor'), { recursive: true }); | ||
| await fs.mkdir(path.join(workspace, '.claude'), { recursive: true }); | ||
|
|
||
| const pkgDir = path.join(workspace, 'packages', 'langsmith-plugin'); | ||
| await writeFile( | ||
| path.join(pkgDir, '.claude-plugin', 'plugin.json'), | ||
| JSON.stringify({ name: 'langsmith-plugin', version: '0.1.0' }, null, 2) | ||
| ); | ||
| await writeFile( | ||
| path.join(pkgDir, 'config', 'AGENTS.md'), | ||
| '# Plugin Guide\n' | ||
| ); | ||
| await writeFile( | ||
| path.join(pkgDir, 'config', 'skills', 'langsmith-dataset', 'SKILL.md'), | ||
| '---\nname: langsmith-dataset\n---\n# Dataset Skill\n' | ||
| ); | ||
| await writeFile( | ||
| path.join(pkgDir, 'config', 'skills', 'langsmith-dataset', 'helper.ts'), | ||
| 'export const helper = true;\n' | ||
| ); | ||
|
|
||
| const result = runCli( | ||
| ['install', './packages/langsmith-plugin', '--skills', 'langsmith-dataset', '--force', '--conflicts', 'overwrite'], | ||
| workspace, |
There was a problem hiding this comment.
This test creates both .cursor/ and .claude/ in the workspace before running the install. Platform auto-detection will then return both platforms, so openpackage install ... may install into both targets, making the test slower and potentially flaky/unfocused.
To keep the test deterministic and scoped to the behavior being asserted, either avoid creating .claude/ here or pass --platforms cursor in the CLI args.
| function createPackageDiscoveryFilter(basePath: string) { | ||
| return (entryPath: string): boolean => { | ||
| const relativePath = relative(basePath, entryPath).replace(/\\/g, '/'); | ||
| if (!relativePath || relativePath === '.') { | ||
| return true; | ||
| } | ||
|
|
||
| return !isExcludedFromPackage(relativePath); | ||
| }; |
There was a problem hiding this comment.
findMarkerResourceFiles() now walks the entire basePath, but the filter only applies isExcludedFromPackage(), which does not exclude large workspace-local dirs like .git/ and node_modules/. This can make installs/discovery very slow (or effectively hang) when installing from a local repo that has those directories.
Consider extending createPackageDiscoveryFilter to skip common non-package directories (at least .git and node_modules, and likely the existing WORKSPACE_DISCOVERY_EXCLUDES set in packages/core/src/constants/workspace.ts) and use the isDirectory argument from walkFiles’s filter signature to prune entire subtrees early.
Minoo7
changed the title
|
Added a follow-up pass on this PR:
Example that didn’t work before but does work now: |
|
Sorry for the delayed response! Will review and merge as soon as I can. |
- Discover skill markers under nested `*/skills` paths - Anchor recursive glob conversions to the skill directory - Add coverage for converter, discovery, and CLI flows
hyericlee
force-pushed
the
t3code/nested-skill-discovery
branch
from
2abf75d to
185a937
Compare
skills/ancestor instead of only<root>/skillsskills/...paths without duplicated prefixesconfig/skills/...resources install correctly after conversionOtherwise installing from places like langsmith directly, didn’t work.