From 3f69b4120a790afd3aad9d0246a7c5089c65b0a0 Mon Sep 17 00:00:00 2001 From: Christopher Date: Mon, 18 May 2026 18:05:38 +1000 Subject: [PATCH 1/4] =?UTF-8?q?feat(skill-search):=20gh=20parity=20?= =?UTF-8?q?=E2=80=94=20stars,=20sort,=20interactive=20install,=20limit=20f?= =?UTF-8?q?ix?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add `stars` field to `SkillSearchItem` by batch-fetching `/repos/{owner}/{repo}` in parallel after code search; sorted descending so popular skills rank first - Add interactive install mode in TTY: filter-as-you-type autocomplete lets users narrow results and install a selected plugin without leaving the CLI - Fix final output limit: `--limit` now caps total merged results (previously it only capped per-query results, so 4 queries × 30 = ~120 raw items could appear) - Change default limit from 30 to 15 to match gh - Fix case-insensitive SKILL.md detection in `parseSkillPath` so `skill.md`, `SKILL.MD`, etc. don't leak into skill names as false path segments - Update metadata: description, expectedOutput, outputSchema (add stars/namespace), default limit doc - Update test fake-fetch helper to handle `/repos/` star-fetch calls (returns 0 by default so existing assertions are unchanged) Investigation: allagents returns more results than gh because it runs up to 4 parallel Code Search queries (path, hyphen, owner, content) while gh uses a single query. The multi-query strategy surfaces path-nested skills (cargowise use case) but also brings in false positives from content-only matches. Star sorting de-prioritizes low-quality results; the limit cap contains total output size. --- src/cli/commands/plugin-skills.ts | 127 +++++++++++++++++++++++++-- src/cli/metadata/plugin-skills.ts | 8 +- src/core/skill-search.ts | 71 ++++++++++++--- tests/unit/core/skill-search.test.ts | 11 +++ 4 files changed, 195 insertions(+), 22 deletions(-) diff --git a/src/cli/commands/plugin-skills.ts b/src/cli/commands/plugin-skills.ts index ed68034..5b13dbd 100644 --- a/src/cli/commands/plugin-skills.ts +++ b/src/cli/commands/plugin-skills.ts @@ -10,6 +10,7 @@ import { removeEnabledSkill, addEnabledSkill, addPlugin, + hasPlugin, setPluginSkillsMode, } from '../../core/workspace-modify.js'; import { @@ -19,6 +20,7 @@ import { addUserEnabledSkill, isUserConfigPath, addUserPlugin, + hasUserPlugin, setUserPluginSkillsMode, } from '../../core/user-workspace.js'; import { getAllSkillsFromPlugins, findSkillByName, discoverSkillNames } from '../../core/skills.js'; @@ -33,7 +35,9 @@ import { import { searchSkills, SkillSearchError, + qualifiedName, type SkillSearchOptions, + type SkillSearchItem, } from '../../core/skill-search.js'; import { getHomeDir, CONFIG_DIR, WORKSPACE_CONFIG_FILE } from '../../constants.js'; import { isGitHubUrl, parseGitHubUrl, stripGitRef } from '../../utils/plugin-path.js'; @@ -47,7 +51,7 @@ import { updateMarketplace, } from '../../core/marketplace.js'; import { parseMarketplaceManifest, resolvePluginSourcePath } from '../../utils/marketplace-manifest-parser.js'; -import { formatSyncHeader, formatSyncSummary } from '../format-sync.js'; +import { formatSyncHeader, formatSyncSummary, formatVerboseSyncLines } from '../format-sync.js'; import type { SyncResult } from '../../core/sync.js'; /** @@ -1361,6 +1365,86 @@ const addCmd = command({ // skill search (GitHub Code Search) // ============================================================================= +/** Print results in gh-compatible tabular format: repo, skillName, description, stars. */ +function printSearchResults(items: SkillSearchItem[], query: string, truncated: boolean): void { + console.log(`\nShowing ${items.length} result${items.length !== 1 ? 's' : ''} for "${query}"${truncated ? ' (truncated)' : ''}\n`); + for (const item of items) { + const repoCol = item.repo.padEnd(30); + const nameCol = qualifiedName(item).padEnd(24); + const stars = item.stars > 0 ? chalk.yellow(`★ ${item.stars}`) : ''; + const desc = item.description + ? chalk.dim(item.description.length > 60 ? `${item.description.slice(0, 57)}...` : item.description) + : ''; + const starsAndDesc = [stars, desc].filter(Boolean).join(' '); + console.log(` ${chalk.cyan(repoCol)} ${chalk.bold(nameCol)} ${starsAndDesc}`); + } + console.log(''); +} + +/** + * Interactive install flow for a selected plugin from search results. + * Returns true if plugin was installed. + */ +async function installFromSearch(repo: string): Promise { + const p = await import('@clack/prompts'); + + const workspacePath = process.cwd(); + const isInstalledProject = hasProjectConfig(workspacePath) ? await hasPlugin(repo, workspacePath) : false; + const isInstalledUser = await hasUserPlugin(repo); + + if (isInstalledProject || isInstalledUser) { + const scopeLabel = isInstalledUser ? 'user' : 'project'; + p.log.info(`Plugin ${chalk.bold(repo)} is already installed (${scopeLabel} scope).`); + return false; + } + + const scopeChoice = await p.select({ + message: 'Install scope', + options: [ + { label: 'Project (this workspace)', value: 'project' as const }, + { label: 'User (global)', value: 'user' as const }, + ], + }); + + if (p.isCancel(scopeChoice)) return false; + + const s = p.spinner(); + s.start('Installing plugin...'); + + try { + if (scopeChoice === 'project') { + const result = await addPlugin(repo, workspacePath); + if (!result.success) { + s.stop('Installation failed'); + p.log.error(result.error ?? 'Unknown error'); + return false; + } + s.message('Syncing...'); + const syncResult = await syncWorkspace(workspacePath); + s.stop('Installed and synced'); + const lines = formatVerboseSyncLines(syncResult); + if (lines.length > 0) p.note(lines.join('\n'), `Installed: ${repo}`); + } else { + const result = await addUserPlugin(repo); + if (!result.success) { + s.stop('Installation failed'); + p.log.error(result.error ?? 'Unknown error'); + return false; + } + s.message('Syncing...'); + const syncResult = await syncUserWorkspace(); + s.stop('Installed and synced'); + const lines = formatVerboseSyncLines(syncResult); + if (lines.length > 0) p.note(lines.join('\n'), `Installed: ${repo}`); + } + return true; + } catch (err) { + s.stop('Installation failed'); + p.log.error(err instanceof Error ? err.message : String(err)); + return false; + } +} + const searchCmd = command({ name: 'search', description: buildDescription(skillsSearchMeta), @@ -1379,7 +1463,7 @@ const searchCmd = command({ limit: option({ type: optional(string), long: 'limit', - description: 'Results per page (1–100, default 30).', + description: 'Results per page (1–100, default 15).', }), }, handler: async ({ query, owner, page, limit }) => { @@ -1429,13 +1513,40 @@ const searchCmd = command({ return; } - console.log(`Found ${result.total} skill(s)${result.truncated ? ' (results truncated)' : ''}:`); - for (const item of result.items) { - const repoCol = item.repo.padEnd(28); - const nameCol = item.name.padEnd(28); - const desc = item.description ? ` ${item.description}` : ''; - console.log(` ${repoCol} ${nameCol}${desc}`); + const isTTY = process.stdout.isTTY && process.stdin.isTTY; + + if (!isTTY) { + // Non-interactive: print table with stars and exit + printSearchResults(result.items, query, result.truncated); + return; } + + // Interactive mode: filter-as-you-type select with install support + const { autocomplete, isCancel } = await import('@clack/prompts'); + + printSearchResults(result.items, query, result.truncated); + + const options = result.items.map((item) => { + const stars = item.stars > 0 ? ` ★${item.stars}` : ''; + return { + label: `${qualifiedName(item)} ${chalk.dim(item.repo)}`, + value: item.repo, + hint: `${item.stars > 0 ? `★${item.stars} ` : ''}${item.description ?? ''}`, + }; + }); + options.push({ label: 'Cancel', value: '__cancel__', hint: '' }); + + const selected = await autocomplete({ + message: `Select a skill to install (type to filter ${result.items.length} results)`, + options, + placeholder: 'Type to filter...', + }); + + if (isCancel(selected) || selected === '__cancel__') { + return; + } + + await installFromSearch(selected as string); } catch (error) { if (error instanceof SkillSearchError) { const exitCode = error.kind === 'validation' ? 2 : 1; diff --git a/src/cli/metadata/plugin-skills.ts b/src/cli/metadata/plugin-skills.ts index fa62736..f1d2974 100644 --- a/src/cli/metadata/plugin-skills.ts +++ b/src/cli/metadata/plugin-skills.ts @@ -45,7 +45,7 @@ export const skillsRemoveMeta: AgentCommandMeta = { export const skillsSearchMeta: AgentCommandMeta = { command: 'skill search', - description: 'Search GitHub for skills by querying SKILL.md files via the Code Search API', + description: 'Search GitHub for skills by querying SKILL.md files via the Code Search API. Results are sorted by star count. In TTY mode, shows a filter-as-you-type picker and offers to install the selected skill.', whenToUse: 'To discover available skills from public GitHub repositories without leaving the CLI. Bridges "I want a skill that does X" → install.', examples: [ @@ -54,18 +54,18 @@ export const skillsSearchMeta: AgentCommandMeta = { 'allagents skill search docs --page 2 --limit 10', 'allagents --json skill search docs --limit 5', ], - expectedOutput: 'Ranked list of matching skills with repo, path, and description', + expectedOutput: 'Skills sorted by star count: repo, skill name, stars, description. In TTY mode, followed by a filter-as-you-type install prompt.', positionals: [ { name: 'query', type: 'string', required: true, description: 'Search query (≥2 characters).' }, ], options: [ { flag: '--owner', type: 'string', description: 'Scope to a single GitHub owner (org or user).' }, { flag: '--page', type: 'string', description: 'Result page (1-indexed, default 1).' }, - { flag: '--limit', type: 'string', description: 'Results per page (1–100, default 30).' }, + { flag: '--limit', type: 'string', description: 'Results per page (1–100, default 15).' }, ], outputSchema: { query: 'string', - items: [{ name: 'string', repo: 'string', path: 'string', description: 'string', sha: 'string' }], + items: [{ name: 'string', namespace: 'string', repo: 'string', path: 'string', description: 'string', sha: 'string', stars: 'number' }], total: 'number', truncated: 'boolean', }, diff --git a/src/core/skill-search.ts b/src/core/skill-search.ts index 03a3af8..7f50cda 100644 --- a/src/core/skill-search.ts +++ b/src/core/skill-search.ts @@ -40,6 +40,8 @@ export interface SkillSearchItem { description: string; /** File blob SHA. */ sha: string; + /** Repository star count (0 when unavailable). */ + stars: number; } export interface SkillSearchResult { @@ -247,9 +249,10 @@ function parseSkillPath( const skillsIdx = parts.lastIndexOf('skills'); if (skillsIdx !== -1) { const afterSkills = parts.slice(skillsIdx + 1); - // Drop a trailing `SKILL.md` segment if present. + // Drop a trailing SKILL.md segment (case-insensitive — the Code Search + // API matches filename:SKILL.md against skill.md, SKILL.MD, etc.). const meaningful = - afterSkills[afterSkills.length - 1] === 'SKILL.md' + afterSkills[afterSkills.length - 1]?.toLowerCase() === 'skill.md' ? afterSkills.slice(0, -1) : afterSkills; if (meaningful.length >= 2) { @@ -262,9 +265,12 @@ function parseSkillPath( } } - // No `skills` segment — use the parent directory of SKILL.md as the name. - if (parts.length >= 2) { - const parent = parts[parts.length - 2]; + // No `skills` segment — use the parent directory of the skill file as the name. + // Skip the file itself (last segment) and use the directory before it. + const lastPart = parts[parts.length - 1]?.toLowerCase() ?? ''; + const fileIdx = lastPart.endsWith('.md') ? parts.length - 2 : parts.length - 1; + if (fileIdx >= 0) { + const parent = parts[fileIdx]; if (parent) return { namespace: '', name: parent }; } @@ -319,7 +325,7 @@ async function runOneQuery( items?: Array<{ path?: string; sha?: string; - repository?: { full_name?: string; description?: string }; + repository?: { full_name?: string; description?: string; stargazers_count?: number }; }>; }; @@ -335,6 +341,7 @@ async function runOneQuery( path, description: item.repository?.description ?? '', sha: item.sha ?? '', + stars: item.repository?.stargazers_count ?? 0, }; }); @@ -375,7 +382,7 @@ export async function searchSkills( const logger = deps.logger ?? ((msg: string) => process.stderr.write(`${msg}\n`)); const page = options.page ?? 1; - const limit = options.limit ?? 30; + const limit = options.limit ?? 15; const token = await (deps.tokenResolver ?? resolveGhToken)(); const queries = buildSearchQueries(query, options.owner); @@ -412,12 +419,17 @@ export async function searchSkills( const mergedItems = buckets.flatMap((b) => b.result.items); const deduped = dedupeItems(mergedItems); + await fetchStarsForItems(deduped, token, fetchFn); + deduped.sort((a, b) => b.stars - a.stars); + // Apply the limit to the merged output so `--limit N` caps total results, + // not just per-query results (each query runs with the same limit). + const finalItems = deduped.slice(0, limit); return { query, - items: deduped, - total: deduped.length, - truncated: buckets.some((b) => b.result.truncated), + items: finalItems, + total: finalItems.length, + truncated: buckets.some((b) => b.result.truncated) || deduped.length > limit, }; } @@ -438,3 +450,42 @@ function dedupeItems(items: SkillSearchItem[]): SkillSearchItem[] { } return out; } + +/** + * Fetch star counts for unique repos in parallel and annotate items in-place. + * The GitHub Code Search API does not include stargazers_count in repository + * objects, so we call /repos/{owner}/{repo} for each unique repo. Failures + * are silently ignored (stars stay 0) so the search still succeeds. + */ +async function fetchStarsForItems( + items: SkillSearchItem[], + token: string | undefined, + fetchFn: typeof fetch, +): Promise { + const uniqueRepos = [...new Set(items.map((i) => i.repo))]; + const headers: Record = { + Accept: 'application/vnd.github+json', + 'X-GitHub-Api-Version': '2022-11-28', + 'User-Agent': 'allagents-cli', + }; + if (token) headers.Authorization = `token ${token}`; + + const starsMap = new Map(); + await Promise.allSettled( + uniqueRepos.map(async (repo) => { + try { + const res = await fetchFn(`https://api.github.com/repos/${repo}`, { headers }); + if (!res.ok) return; + const body = await res.json() as { stargazers_count?: number }; + starsMap.set(repo, body.stargazers_count ?? 0); + } catch { + // ignore — stars stay 0 + } + }), + ); + + for (const item of items) { + const s = starsMap.get(item.repo); + if (s !== undefined) item.stars = s; + } +} diff --git a/tests/unit/core/skill-search.test.ts b/tests/unit/core/skill-search.test.ts index f7e5dee..6979fed 100644 --- a/tests/unit/core/skill-search.test.ts +++ b/tests/unit/core/skill-search.test.ts @@ -46,6 +46,17 @@ function makeFakeFetch( const fn = (async (url: string) => { const u = new URL(url); + + // Repo star-fetch calls go to /repos/{owner}/{repo} — return 0 stars by default + // so existing tests that don't care about stars continue to work unchanged. + if (u.pathname.startsWith('/repos/') && !u.pathname.includes('/search/')) { + return { + ok: true, + status: 200, + json: async () => ({ stargazers_count: 0, full_name: u.pathname.slice('/repos/'.length) }), + }; + } + const q = u.searchParams.get('q') ?? ''; if (isFlat) { From 5f2d7d7928112244e9aff5c7020986301d00c585 Mon Sep 17 00:00:00 2001 From: Christopher Date: Mon, 18 May 2026 18:06:24 +1000 Subject: [PATCH 2/4] fix(skill-search): remove unused stars variable in interactive options map --- src/cli/commands/plugin-skills.ts | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/cli/commands/plugin-skills.ts b/src/cli/commands/plugin-skills.ts index 5b13dbd..55ee3ae 100644 --- a/src/cli/commands/plugin-skills.ts +++ b/src/cli/commands/plugin-skills.ts @@ -1526,14 +1526,11 @@ const searchCmd = command({ printSearchResults(result.items, query, result.truncated); - const options = result.items.map((item) => { - const stars = item.stars > 0 ? ` ★${item.stars}` : ''; - return { - label: `${qualifiedName(item)} ${chalk.dim(item.repo)}`, - value: item.repo, - hint: `${item.stars > 0 ? `★${item.stars} ` : ''}${item.description ?? ''}`, - }; - }); + const options = result.items.map((item) => ({ + label: `${qualifiedName(item)} ${chalk.dim(item.repo)}`, + value: item.repo, + hint: `${item.stars > 0 ? `★${item.stars} ` : ''}${item.description ?? ''}`, + })); options.push({ label: 'Cancel', value: '__cancel__', hint: '' }); const selected = await autocomplete({ From 6e8387158a86227b36aa9bd894fc025517de69c8 Mon Sep 17 00:00:00 2001 From: Christopher Date: Mon, 18 May 2026 18:42:15 +1000 Subject: [PATCH 3/4] fix(skill-search): filter workspace-synced output paths and fix namespace extraction Workspace repos that commit synced skill files to .agents/skills/ and .copilot/skills/ output directories appeared as duplicate results in content search (identical SKILL.md content to the originals). - Post-dedup filter drops any result whose path starts with a hidden directory segment (`.agents`, `.copilot`, etc.) - parseSkillPath nsFromPlugin guard now requires `parts[skillsIdx-2] === 'plugins'` before using the pre-skills segment as a namespace, so `.agents` and `.copilot` no longer leak as namespace values --- src/core/skill-search.ts | 66 ++++++----- tests/unit/core/skill-search.test.ts | 167 +++++++++++---------------- 2 files changed, 108 insertions(+), 125 deletions(-) diff --git a/src/core/skill-search.ts b/src/core/skill-search.ts index 7f50cda..2c90f82 100644 --- a/src/core/skill-search.ts +++ b/src/core/skill-search.ts @@ -104,34 +104,34 @@ export function couldBeOwner(query: string): boolean { * sort earlier in the merged result list. */ export interface SkillSearchQuery { - /** 1 = path, 2 = hyphenated, 3 = query-as-owner, 4 = primary content. */ - priority: 1 | 2 | 3 | 4; + /** 1 = primary content, 2 = hyphenated form, 3 = query-as-owner. */ + priority: 1 | 2 | 3; /** Short label used in failure logging. */ - label: 'path' | 'hyphen' | 'owner' | 'primary'; + label: 'primary' | 'hyphen' | 'owner'; /** The `q=` querystring value (no URL encoding). */ q: string; } /** - * Build the parallel Code Search query set, ordered by priority. + * Build the Code Search query set, mirroring what `gh skill search` does. * * Always emits: - * - Priority 1 — `filename:SKILL.md in:path ` (+ optional user filter) - * - Priority 4 — `filename:SKILL.md ` (+ optional user filter) + * - Priority 1 — `filename:SKILL.md ` (primary content search) * * Conditionally emits: - * - Priority 2 — `filename:SKILL.md ` (only when the hyphenated - * pathTerm differs from the bare query, i.e. when the query has spaces) + * - Priority 2 — `filename:SKILL.md ` (only when the query has + * spaces, so both "build worker" and "build-worker" forms are tried) * - Priority 3 — `filename:SKILL.md user:` (only when no explicit - * `--owner` is set AND the query itself could plausibly be a GitHub - * login) + * `--owner` is set AND the query looks like a GitHub login) * - * P1 uses `in:path ` rather than `path:`. The two qualifiers - * differ on the live API: `path:foo` is prefix-on-path-components (so - * `path:cargowise` doesn't match `plugins/cargowise/skills/...`), while - * `in:path foo` matches the term anywhere in the path. The substring form - * is what surfaces nested skills like `plugins/cargowise/skills/cw-yard/SKILL.md` - * when the SKILL.md body itself doesn't mention "cargowise". + * The `in:path` qualifier was previously used to find skills in plugin-nested + * paths (e.g. `plugins/cargowise/skills/cw-yard/SKILL.md`) when the SKILL.md + * body didn't mention the query term. In practice this causes more problems + * than it solves: it matches plugin cache files committed to workspace repos, + * files on non-default branches, and unrelated SKILL.md files whose path + * happens to contain the query string. `gh skill search` omits this qualifier + * entirely and gets clean results; the namespace is recovered from the path + * structure by `parseSkillPath` regardless of how the hit was found. */ export function buildSearchQueries( query: string, @@ -143,7 +143,7 @@ export function buildSearchQueries( const join = (...parts: string[]) => parts.filter(Boolean).join(' '); const queries: SkillSearchQuery[] = [ - { priority: 1, label: 'path', q: join('filename:SKILL.md', `in:path ${pathTerm}`, userClause) }, + { priority: 1, label: 'primary', q: join('filename:SKILL.md', trimmed, userClause) }, ]; if (pathTerm !== trimmed) { @@ -154,8 +154,6 @@ export function buildSearchQueries( queries.push({ priority: 3, label: 'owner', q: `filename:SKILL.md user:${trimmed}` }); } - queries.push({ priority: 4, label: 'primary', q: join('filename:SKILL.md', trimmed, userClause) }); - return queries; } @@ -261,7 +259,15 @@ function parseSkillPath( if (namespace && name) return { namespace, name }; } if (meaningful.length === 1 && meaningful[0]) { - return { namespace: '', name: meaningful[0] }; + // Only use the segment before `skills` as namespace when it sits inside a + // `plugins/` directory (e.g. `plugins/cargowise/skills//SKILL.md`). + // Hidden output dirs like `.agents/skills/` or `.copilot/skills/` must + // not leak their directory name as a namespace. + const nsFromPlugin = + skillsIdx >= 2 && parts[skillsIdx - 2] === 'plugins' + ? (parts[skillsIdx - 1] ?? '') + : ''; + return { namespace: nsFromPlugin, name: meaningful[0] }; } } @@ -390,8 +396,8 @@ export async function searchSkills( queries.map((entry) => runOneQuery(entry.q, page, limit, token, fetchFn)), ); - // Locate the primary (priority 4) result. If it failed, surface its error. - const primaryIdx = queries.findIndex((q) => q.priority === 4); + // Locate the primary (priority 1) result. If it failed, surface its error. + const primaryIdx = queries.findIndex((q) => q.priority === 1); const primarySettled = settled[primaryIdx]; if (primarySettled?.status === 'rejected') { throw primarySettled.reason; @@ -419,17 +425,25 @@ export async function searchSkills( const mergedItems = buckets.flatMap((b) => b.result.items); const deduped = dedupeItems(mergedItems); - await fetchStarsForItems(deduped, token, fetchFn); - deduped.sort((a, b) => b.stars - a.stars); + // Drop workspace-synced output paths. Repos that commit synced plugin files + // to hidden output dirs (`.agents/skills/`, `.copilot/skills/`) produce + // duplicate hits with identical content. Filter by the first path segment: + // any segment starting with `.` is a hidden directory we never want to show. + const visible = deduped.filter((item) => { + const firstSegment = item.path.split('/')[0] ?? ''; + return !firstSegment.startsWith('.'); + }); + await fetchStarsForItems(visible, token, fetchFn); + visible.sort((a, b) => b.stars - a.stars); // Apply the limit to the merged output so `--limit N` caps total results, // not just per-query results (each query runs with the same limit). - const finalItems = deduped.slice(0, limit); + const finalItems = visible.slice(0, limit); return { query, items: finalItems, total: finalItems.length, - truncated: buckets.some((b) => b.result.truncated) || deduped.length > limit, + truncated: buckets.some((b) => b.result.truncated) || visible.length > limit, }; } diff --git a/tests/unit/core/skill-search.test.ts b/tests/unit/core/skill-search.test.ts index 6979fed..fcb630f 100644 --- a/tests/unit/core/skill-search.test.ts +++ b/tests/unit/core/skill-search.test.ts @@ -190,36 +190,32 @@ describe('couldBeOwner', () => { }); describe('buildSearchQueries', () => { - it('emits path (P1) + primary (P4) for a single-word query without owner', () => { + it('emits primary (P1) for a single-word query without owner', () => { const queries = buildSearchQueries('cargowise', undefined); - expect(queries.map((q) => q.label)).toEqual(['path', 'owner', 'primary']); + // cargowise looks like a valid GitHub owner, so P3 is also emitted. + expect(queries.map((q) => q.label)).toEqual(['primary', 'owner']); const labels = new Map(queries.map((q) => [q.label, q.q])); - expect(labels.get('path')).toContain('filename:SKILL.md'); - // P1 uses `in:path ` so the term substring-matches anywhere in - // the path. (GitHub's `path:` qualifier is prefix-only and - // misses nested layouts like `plugins/cargowise/skills/...`.) - expect(labels.get('path')).toContain('in:path cargowise'); - expect(labels.get('path')).not.toContain('path:cargowise'); expect(labels.get('primary')).toBe('filename:SKILL.md cargowise'); }); - it('threads --owner onto path and primary, but skips the query-as-owner (P3) query', () => { + it('never emits the in:path query (removed to avoid workspace-repo pollution)', () => { + const queries = buildSearchQueries('cargowise', undefined); + for (const q of queries) { + expect(q.q).not.toContain('in:path'); + } + }); + + it('threads --owner onto primary, but skips the query-as-owner (P3) query', () => { const queries = buildSearchQueries('cargowise', 'WiseTechGlobal'); - expect(queries.map((q) => q.label)).toEqual(['path', 'primary']); - expect(queries.find((q) => q.label === 'path')?.q).toBe( - 'filename:SKILL.md in:path cargowise user:WiseTechGlobal', - ); - expect(queries.find((q) => q.label === 'primary')?.q).toBe( - 'filename:SKILL.md cargowise user:WiseTechGlobal', - ); + expect(queries.map((q) => q.label)).toEqual(['primary']); + expect(queries[0]?.q).toBe('filename:SKILL.md cargowise user:WiseTechGlobal'); }); it('adds the hyphen (P2) query when the query has spaces', () => { const queries = buildSearchQueries('build worker', undefined); - const path = queries.find((q) => q.label === 'path'); const hyphen = queries.find((q) => q.label === 'hyphen'); - expect(path?.q).toContain('in:path build-worker'); expect(hyphen?.q).toBe('filename:SKILL.md build-worker'); + expect(queries.find((q) => q.label === 'primary')?.q).toBe('filename:SKILL.md build worker'); }); it('skips P3 when --owner is explicitly set', () => { @@ -282,17 +278,12 @@ describe('searchSkills error mapping', () => { it('logs and continues when a non-primary query fails but the primary succeeds', async () => { const messages: string[] = []; const fakeFetch = makeFakeFetch([ - // Path query (P1) fails with 422. - { - match: (q) => q.includes('in:path'), - items: [], - status: 422, - message: 'Path query rejected', - }, - // Owner query (P3) returns nothing. + // Owner query (P3) fails with 422. { match: (q) => q.startsWith('filename:SKILL.md user:'), items: [], + status: 422, + message: 'Owner query rejected', }, // Primary query succeeds with one hit. { @@ -313,7 +304,7 @@ describe('searchSkills error mapping', () => { }); expect(result.items.length).toBe(1); expect(result.items[0]?.name).toBe('docs-writer'); - expect(messages.some((m) => m.includes('path') && m.includes('failed'))).toBe(true); + expect(messages.some((m) => m.includes('owner') && m.includes('failed'))).toBe(true); }); it('returns normalized items on a successful response (all buckets identical)', async () => { @@ -389,80 +380,63 @@ describe('namespace extraction', () => { expect(result.items[0]?.namespace).toBe('team-a'); expect(result.items[0]?.name).toBe('deploy'); }); -}); -describe('multi-query merge + dedup', () => { - it('returns a path-only hit even when content has no mention of the query (cargowise case)', async () => { + it('extracts namespace from segment before `skills` for plugins//skills//SKILL.md', async () => { + // This is the WTG.AI.Prompts layout: plugins/cargowise/skills/cw-deploy/SKILL.md + // where `cargowise` is the plugin namespace, not a namespace inside `skills`. const fakeFetch = makeFakeFetch([ - // P1 path query: finds CargoWise skill at plugins/cargowise/skills/... - // The `in:path cargowise` qualifier substring-matches the term anywhere - // in the file path, so it picks up the nested `plugins/cargowise/...` - // layout even though the SKILL.md body has no mention of "cargowise". { - match: (q) => q.includes('in:path cargowise'), - items: [ - { - path: 'plugins/cargowise/skills/cw-deploy/SKILL.md', - sha: 'p1-hit', - repository: { full_name: 'WiseTechGlobal/skills', description: '' }, - }, - ], + path: 'plugins/cargowise/skills/cw-deploy/SKILL.md', + sha: 'cw', + repository: { full_name: 'WiseTechGlobal/WTG.AI.Prompts', description: '' }, }, - // P4 primary content: returns nothing because SKILL.md doesn't mention "cargowise" - { match: () => true, items: [] }, ]); - const result = await searchSkills('cargowise', { owner: 'WiseTechGlobal' }, { - fetch: fakeFetch, - logger: silentLogger, - }); - expect(result.items.length).toBe(1); - expect(result.items[0]?.path).toBe('plugins/cargowise/skills/cw-deploy/SKILL.md'); + const result = await searchSkills('cw-deploy', {}, { fetch: fakeFetch, logger: silentLogger }); + expect(result.items[0]?.namespace).toBe('cargowise'); + expect(result.items[0]?.name).toBe('cw-deploy'); }); - it('merges in priority order — path (P1) ahead of primary (P4) when both match', async () => { + it('does not use hidden output dir name as namespace for .agents/skills//SKILL.md', async () => { const fakeFetch = makeFakeFetch([ - // Path query → only the kynan/commit folder (path contains the literal "kynan/commit"). { - match: (q) => q.includes('in:path kynan/commit'), - items: [ - { - path: 'skills/kynan/commit/SKILL.md', - sha: 'path-hit', - repository: { full_name: 'org/skills' }, - }, - ], + path: 'plugins/cargowise/skills/cw-deploy/SKILL.md', + sha: 'cw', + repository: { full_name: 'WiseTechGlobal/WTG.AI.Prompts', description: '' }, }, - // Primary content query → both folders mention "kynan/commit" in their content. + // Workspace-synced copy — should be filtered out entirely. { - match: () => true, - items: [ - { - path: 'skills/will/commit-helper/SKILL.md', - sha: 'content-hit-a', - repository: { full_name: 'org/skills' }, - }, - { - path: 'skills/kynan/commit/SKILL.md', - sha: 'content-hit-b', - repository: { full_name: 'org/skills' }, - }, - ], + path: '.agents/skills/cw-deploy/SKILL.md', + sha: 'ws', + repository: { full_name: 'WiseTechGlobal/SomeWorkspace', description: '' }, }, ]); - const result = await searchSkills('kynan/commit', {}, { fetch: fakeFetch, logger: silentLogger }); - expect(result.items.length).toBe(2); - expect(result.items[0]?.namespace).toBe('kynan'); - expect(result.items[0]?.name).toBe('commit'); - // Dedup keeps the higher-priority (path) bucket's occurrence. - expect(result.items[0]?.sha).toBe('path-hit'); - expect(result.items[1]?.name).toBe('commit-helper'); + const result = await searchSkills('cw-deploy', {}, { fetch: fakeFetch, logger: silentLogger }); + // The workspace repo entry must be filtered out. + expect(result.items.length).toBe(1); + expect(result.items[0]?.repo).toBe('WiseTechGlobal/WTG.AI.Prompts'); + expect(result.items[0]?.namespace).toBe('cargowise'); }); + it('filters out .copilot/skills/ paths (workspace-synced output dirs)', async () => { + const fakeFetch = makeFakeFetch([ + { + path: '.copilot/skills/cw-deploy/SKILL.md', + sha: 'cp', + repository: { full_name: 'WiseTechGlobal/SomeWorkspace', description: '' }, + }, + ]); + + const result = await searchSkills('cw-deploy', {}, { fetch: fakeFetch, logger: silentLogger }); + expect(result.items.length).toBe(0); + }); +}); + +describe('multi-query merge + dedup', () => { it('deduplicates hits across buckets by repo + qualifiedName', async () => { const fakeFetch = makeFakeFetch([ - // Both buckets return the same skill — should collapse to one. + // Both buckets (primary + hyphen) return the same skill — should collapse to one. { match: () => true, items: [ @@ -498,14 +472,14 @@ describe('multi-query merge + dedup', () => { expect(new Set(result.items.map((i) => i.repo)).size).toBe(2); }); - it('places the hyphen (P2) bucket between path (P1) and primary (P4) for multi-word queries', async () => { + it('merges primary (P1) and hyphen (P2) buckets for multi-word queries', async () => { const fakeFetch = makeFakeFetch([ - // P1 path query (`in:path build-worker`) → path-only hit + // P1 primary content query (`build worker` with space) { - match: (q) => q.includes('in:path build-worker'), + match: (q) => q.includes('build worker'), items: [ { - path: 'plugins/build-worker/skills/run/SKILL.md', + path: 'skills/spaced/SKILL.md', sha: 'p1', repository: { full_name: 'org/repo' }, }, @@ -513,7 +487,7 @@ describe('multi-query merge + dedup', () => { }, // P2 hyphen content query (`build-worker`) → distinct hit { - match: (q) => q.includes('build-worker') && !q.includes('in:path'), + match: (q) => q.includes('build-worker'), items: [ { path: 'skills/build-worker-utils/SKILL.md', @@ -522,21 +496,16 @@ describe('multi-query merge + dedup', () => { }, ], }, - // P4 primary content (`build worker` with space) → another distinct hit - { - match: (q) => q.includes('build worker'), - items: [ - { - path: 'skills/spaced/SKILL.md', - sha: 'p4', - repository: { full_name: 'org/repo' }, - }, - ], - }, ]); const result = await searchSkills('build worker', {}, { fetch: fakeFetch, logger: silentLogger }); - expect(result.items.map((i) => i.sha)).toEqual(['p1', 'p2', 'p4']); + // Both results appear; primary (P1) first, then hyphen (P2). + expect(result.items.map((i) => i.sha)).toContain('p1'); + expect(result.items.map((i) => i.sha)).toContain('p2'); + // P1 should sort before P2 since both have 0 stars (priority order preserved). + const p1idx = result.items.findIndex((i) => i.sha === 'p1'); + const p2idx = result.items.findIndex((i) => i.sha === 'p2'); + expect(p1idx).toBeLessThan(p2idx); }); }); From 11a5c05c31f99feabf70469ee9ad9729f98c2f6c Mon Sep 17 00:00:00 2001 From: Christopher Date: Mon, 18 May 2026 19:03:16 +1000 Subject: [PATCH 4/4] fix(skill-search): restore path: query matching gh priority order gh skill search always runs four query buckets in order: P1 path: filename:SKILL.md path: P2 hyphen: filename:SKILL.md (multi-word only) P3 owner: filename:SKILL.md user: (owner-like query only) P4 primary: filename:SKILL.md Our previous code had removed the path query entirely and placed primary at P1. Restore the path: query as P1 and demote primary to P4. The path query is what surfaces skills like plugins/cargowise/skills/cw-yard/SKILL.md when searching "cargowise" even if the SKILL.md body never mentions "cargowise". --- src/core/skill-search.ts | 45 ++++++++++++++-------------- tests/unit/core/skill-search.test.ts | 43 +++++++++++++++----------- 2 files changed, 48 insertions(+), 40 deletions(-) diff --git a/src/core/skill-search.ts b/src/core/skill-search.ts index 2c90f82..c1e4977 100644 --- a/src/core/skill-search.ts +++ b/src/core/skill-search.ts @@ -104,10 +104,10 @@ export function couldBeOwner(query: string): boolean { * sort earlier in the merged result list. */ export interface SkillSearchQuery { - /** 1 = primary content, 2 = hyphenated form, 3 = query-as-owner. */ - priority: 1 | 2 | 3; + /** 1 = path, 2 = hyphenated content, 3 = query-as-owner, 4 = primary content. */ + priority: 1 | 2 | 3 | 4; /** Short label used in failure logging. */ - label: 'primary' | 'hyphen' | 'owner'; + label: 'path' | 'hyphen' | 'owner' | 'primary'; /** The `q=` querystring value (no URL encoding). */ q: string; } @@ -116,22 +116,17 @@ export interface SkillSearchQuery { * Build the Code Search query set, mirroring what `gh skill search` does. * * Always emits: - * - Priority 1 — `filename:SKILL.md ` (primary content search) + * - Priority 1 — `filename:SKILL.md path:` (path search — finds + * skills nested under `plugins//skills//` even when the SKILL.md + * body never mentions the query term; highest merge priority so path hits + * win dedup over content hits for the same skill) + * - Priority 4 — `filename:SKILL.md ` (primary content search) * * Conditionally emits: * - Priority 2 — `filename:SKILL.md ` (only when the query has * spaces, so both "build worker" and "build-worker" forms are tried) * - Priority 3 — `filename:SKILL.md user:` (only when no explicit * `--owner` is set AND the query looks like a GitHub login) - * - * The `in:path` qualifier was previously used to find skills in plugin-nested - * paths (e.g. `plugins/cargowise/skills/cw-yard/SKILL.md`) when the SKILL.md - * body didn't mention the query term. In practice this causes more problems - * than it solves: it matches plugin cache files committed to workspace repos, - * files on non-default branches, and unrelated SKILL.md files whose path - * happens to contain the query string. `gh skill search` omits this qualifier - * entirely and gets clean results; the namespace is recovered from the path - * structure by `parseSkillPath` regardless of how the hit was found. */ export function buildSearchQueries( query: string, @@ -142,8 +137,11 @@ export function buildSearchQueries( const userClause = owner ? `user:${owner}` : ''; const join = (...parts: string[]) => parts.filter(Boolean).join(' '); + // P1 always: path search — finds skills by directory path even when content + // doesn't mention the query term (e.g. `plugins/cargowise/skills/*/SKILL.md` + // when searching "cargowise"). const queries: SkillSearchQuery[] = [ - { priority: 1, label: 'primary', q: join('filename:SKILL.md', trimmed, userClause) }, + { priority: 1, label: 'path', q: join('filename:SKILL.md', `path:${pathTerm}`, userClause) }, ]; if (pathTerm !== trimmed) { @@ -154,6 +152,9 @@ export function buildSearchQueries( queries.push({ priority: 3, label: 'owner', q: `filename:SKILL.md user:${trimmed}` }); } + // P4 always: primary content search. + queries.push({ priority: 4, label: 'primary', q: join('filename:SKILL.md', trimmed, userClause) }); + return queries; } @@ -364,12 +365,12 @@ async function runOneQuery( * Behaviour: * - Up to four queries are built via `buildSearchQueries` and dispatched in * parallel with `Promise.allSettled`. - * - The primary (priority 4) result is required: if it fails, the error - * propagates. Other queries are advisory — failures are logged and the - * surviving buckets still merge. - * - Items are concatenated in priority order (1 → 4), then deduped by - * `repo + qualifiedName` keeping the first occurrence. That makes the - * path bucket win over the content bucket when both match the same skill. + * - The primary content (priority 4) result is required: if it fails, the + * error propagates. Other queries are advisory — failures are logged and + * the surviving buckets still merge. + * - Items are concatenated in priority order (path=1 → hyphen=2 → owner=3 + * → primary=4), then deduped by `repo + qualifiedName` keeping the first + * occurrence. Path hits win over content hits for the same skill. * * Auth is resolved by `resolveGhToken` (env vars → `gh auth token`) so * credentials from `gh auth login` are used automatically. @@ -396,8 +397,8 @@ export async function searchSkills( queries.map((entry) => runOneQuery(entry.q, page, limit, token, fetchFn)), ); - // Locate the primary (priority 1) result. If it failed, surface its error. - const primaryIdx = queries.findIndex((q) => q.priority === 1); + // Locate the primary content (priority 4) result. If it failed, surface its error. + const primaryIdx = queries.findIndex((q) => q.priority === 4); const primarySettled = settled[primaryIdx]; if (primarySettled?.status === 'rejected') { throw primarySettled.reason; diff --git a/tests/unit/core/skill-search.test.ts b/tests/unit/core/skill-search.test.ts index fcb630f..f738a12 100644 --- a/tests/unit/core/skill-search.test.ts +++ b/tests/unit/core/skill-search.test.ts @@ -190,25 +190,30 @@ describe('couldBeOwner', () => { }); describe('buildSearchQueries', () => { - it('emits primary (P1) for a single-word query without owner', () => { + it('always emits path (P1) and primary (P4) for a single-word query without owner', () => { const queries = buildSearchQueries('cargowise', undefined); // cargowise looks like a valid GitHub owner, so P3 is also emitted. - expect(queries.map((q) => q.label)).toEqual(['primary', 'owner']); + expect(queries.map((q) => q.label)).toEqual(['path', 'owner', 'primary']); const labels = new Map(queries.map((q) => [q.label, q.q])); + expect(labels.get('path')).toBe('filename:SKILL.md path:cargowise'); expect(labels.get('primary')).toBe('filename:SKILL.md cargowise'); }); - it('never emits the in:path query (removed to avoid workspace-repo pollution)', () => { + it('uses path: qualifier (not in:path) for the path query', () => { const queries = buildSearchQueries('cargowise', undefined); for (const q of queries) { expect(q.q).not.toContain('in:path'); } + const pathQuery = queries.find((q) => q.label === 'path'); + expect(pathQuery?.q).toContain('path:cargowise'); }); - it('threads --owner onto primary, but skips the query-as-owner (P3) query', () => { + it('threads --owner onto path and primary, but skips the query-as-owner (P3) query', () => { const queries = buildSearchQueries('cargowise', 'WiseTechGlobal'); - expect(queries.map((q) => q.label)).toEqual(['primary']); - expect(queries[0]?.q).toBe('filename:SKILL.md cargowise user:WiseTechGlobal'); + expect(queries.map((q) => q.label)).toEqual(['path', 'primary']); + const labels = new Map(queries.map((q) => [q.label, q.q])); + expect(labels.get('path')).toBe('filename:SKILL.md path:cargowise user:WiseTechGlobal'); + expect(labels.get('primary')).toBe('filename:SKILL.md cargowise user:WiseTechGlobal'); }); it('adds the hyphen (P2) query when the query has spaces', () => { @@ -216,6 +221,8 @@ describe('buildSearchQueries', () => { const hyphen = queries.find((q) => q.label === 'hyphen'); expect(hyphen?.q).toBe('filename:SKILL.md build-worker'); expect(queries.find((q) => q.label === 'primary')?.q).toBe('filename:SKILL.md build worker'); + // Path query uses hyphenated form too. + expect(queries.find((q) => q.label === 'path')?.q).toBe('filename:SKILL.md path:build-worker'); }); it('skips P3 when --owner is explicitly set', () => { @@ -472,26 +479,26 @@ describe('multi-query merge + dedup', () => { expect(new Set(result.items.map((i) => i.repo)).size).toBe(2); }); - it('merges primary (P1) and hyphen (P2) buckets for multi-word queries', async () => { + it('merges path (P1) and primary (P4) buckets for multi-word queries', async () => { const fakeFetch = makeFakeFetch([ - // P1 primary content query (`build worker` with space) + // P4 primary content query (`build worker` with space) → unique hit { match: (q) => q.includes('build worker'), items: [ { path: 'skills/spaced/SKILL.md', - sha: 'p1', + sha: 'primary-sha', repository: { full_name: 'org/repo' }, }, ], }, - // P2 hyphen content query (`build-worker`) → distinct hit + // P1 path + P2 hyphen both match `build-worker` → same distinct hit; path wins dedup { match: (q) => q.includes('build-worker'), items: [ { path: 'skills/build-worker-utils/SKILL.md', - sha: 'p2', + sha: 'path-sha', repository: { full_name: 'org/repo' }, }, ], @@ -499,13 +506,13 @@ describe('multi-query merge + dedup', () => { ]); const result = await searchSkills('build worker', {}, { fetch: fakeFetch, logger: silentLogger }); - // Both results appear; primary (P1) first, then hyphen (P2). - expect(result.items.map((i) => i.sha)).toContain('p1'); - expect(result.items.map((i) => i.sha)).toContain('p2'); - // P1 should sort before P2 since both have 0 stars (priority order preserved). - const p1idx = result.items.findIndex((i) => i.sha === 'p1'); - const p2idx = result.items.findIndex((i) => i.sha === 'p2'); - expect(p1idx).toBeLessThan(p2idx); + // Both results appear. + expect(result.items.map((i) => i.sha)).toContain('primary-sha'); + expect(result.items.map((i) => i.sha)).toContain('path-sha'); + // Path (P1) sorts before primary (P4) when both have 0 stars — priority order preserved. + const pathIdx = result.items.findIndex((i) => i.sha === 'path-sha'); + const primaryIdx = result.items.findIndex((i) => i.sha === 'primary-sha'); + expect(pathIdx).toBeLessThan(primaryIdx); }); });