Skip to content

fix(skills): scope completions by session flavor#667

Merged
tiann merged 1 commit into
tiann:mainfrom
NightWatcher314:fix/flavor-scoped-skills
May 24, 2026
Merged

fix(skills): scope completions by session flavor#667
tiann merged 1 commit into
tiann:mainfrom
NightWatcher314:fix/flavor-scoped-skills

Conversation

@NightWatcher314
Copy link
Copy Markdown
Contributor

Summary

  • pass the session flavor from /sessions/:id/skills through the hub RPC to the CLI
  • resolve skill roots by agent flavor instead of merging .agents, .claude, and .codex for every session
  • include marketplace/plugin cache skill roots for the matching flavor

Fixes #662.

Behavior

  • Claude sessions include shared .agents skills, Claude .claude skills, and Claude plugin cache skills.
  • Codex sessions include shared .agents skills, Codex .codex skills, Codex .system skills, /etc/codex/skills, and Codex plugin cache skills.
  • Other flavors fall back to shared .agents skills plus their own plugin cache namespace if present.

Test plan

  • bun test cli/src/modules/common/skills.test.ts
  • bun run typecheck:cli
  • bun run typecheck:hub

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Findings

  • [Major] Plugin skill discovery scans every cached version instead of installed plugins — listPluginCacheSkillsRoots now adds skills roots for every owner/plugin/version directory in the cache, so stale or uninstalled marketplace plugins can appear in completions and duplicate skill names can resolve to an arbitrary cached version. The existing plugin command path treats installed_plugins.json as the installed-state source and picks the newest installation; skill discovery should do the same instead of treating cache contents as installed state. Evidence: cli/src/modules/common/skills.ts:195.
    Suggested fix:
    type InstalledPluginsFile = {
        plugins?: Record<string, Array<{ installPath?: string; lastUpdated?: string }>>
    }
    
    async function listPluginCacheSkillsRoots(flavor?: string): Promise<string[]> {
        const configDir = getAgentConfigDir(flavor)
        const installedPath = join(configDir, 'plugins', 'installed_plugins.json')
        const installed = JSON.parse(await readFile(installedPath, 'utf-8')) as InstalledPluginsFile
    
        return Object.values(installed.plugins ?? {})
            .map((installations) => [...installations].sort((a, b) => Date.parse(b.lastUpdated ?? '') - Date.parse(a.lastUpdated ?? ''))[0]?.installPath)
            .filter((installPath): installPath is string => typeof installPath === 'string' && installPath.length > 0)
            .map((installPath) => join(installPath, 'skills'))
    }

Questions

  • None.

Summary
Review mode: initial
One issue found: plugin cache traversal uses cache presence as installed-plugin state, which can surface skills that the agent should not offer. Residual risk: I could not run the targeted test command in this environment.

Testing

  • Not run (automation): bun test cli/src/modules/common/skills.test.ts failed because bun is not installed in the runner.

HAPI Bot

Comment thread cli/src/modules/common/skills.ts Outdated
@NightWatcher314 NightWatcher314 force-pushed the fix/flavor-scoped-skills branch from 65be53e to 26336ec Compare May 23, 2026 16:59
@NightWatcher314
Copy link
Copy Markdown
Contributor Author

Addressed the review feedback in 26336ec:

  • listPluginCacheSkillsRoots now reads <agent-config>/plugins/installed_plugins.json instead of walking every cached plugin version.
  • Skill roots are derived from the installed plugin installPath entries.
  • Added coverage that cached-but-uninstalled marketplace skills are not listed.

Re-ran:

  • bun test cli/src/modules/common/skills.test.ts
  • bun run typecheck:cli
  • bun run typecheck:hub

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Findings

  • [Major] Plugin skill discovery ignores configured agent homes — getAgentConfigDir always reads $HOME/.claude / $HOME/.codex, but the existing slash-command/plugin path respects CLAUDE_CONFIG_DIR and CODEX_HOME. Users running Claude or Codex with those env vars will have installed marketplace plugins recorded under the configured directory, so /sessions/:id/skills will silently omit those plugin skills. Evidence: cli/src/modules/common/skills.ts:38.
    Suggested fix:
    function getAgentConfigDir(flavor?: string): string {
        switch (normalizeFlavor(flavor)) {
            case 'claude':
                return process.env.CLAUDE_CONFIG_DIR || join(getHomeDirectory(), '.claude');
            case 'codex':
                return process.env.CODEX_HOME || join(getHomeDirectory(), '.codex');
            default:
                return join(getHomeDirectory(), `.${normalizeFlavor(flavor)}`);
        }
    }

Questions

  • None.

Summary
Review mode: follow-up after new commits
The prior stale-cache finding is addressed by reading installed_plugins.json. One remaining issue: the new plugin skill lookup does not honor configured Claude/Codex homes, so plugin skills can disappear for non-default agent config locations.

Testing

  • Not run (automation): bun test cli/src/modules/common/skills.test.ts could not run because bun is not installed in this runner.

HAPI Bot

Comment thread cli/src/modules/common/skills.ts
@NightWatcher314 NightWatcher314 force-pushed the fix/flavor-scoped-skills branch 3 times, most recently from 4d5409e to 95db4c1 Compare May 23, 2026 17:25
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Findings

  • [Major] Custom CODEX_HOME drops bundled Codex system skills — the user-root scan still decides whether to descend into .system with isCodexSkillsRoot(root), which only matches paths ending in .codex/skills. When Codex runs with CODEX_HOME=/custom/codex, getUserSkillsRoots('codex') returns /custom/codex/skills, so skills/.system/* is skipped even though default ~/.codex/skills/.system is still included. Evidence: cli/src/modules/common/skills.ts:229.
    Suggested fix:
    function shouldIncludeCodexSystem(root: string, flavor: string): boolean {
        return flavor === 'codex' && (
            root.endsWith(join('.codex', 'skills')) ||
            root === join(getAgentConfigDir('codex'), 'skills')
        );
    }
    
    Promise.all(userRoots.map(async (root) => await listTopLevelSkillDirs(root, {
        includeCodexSystem: shouldIncludeCodexSystem(root, flavor)
    }))).then((dirs) => dirs.flat())

Questions

  • None.

Summary
Review mode: follow-up after new commits
The previous configured-home marketplace finding is fixed. One remaining regression: Codex .system skills are still only discovered from the literal ~/.codex/skills-style path, so custom CODEX_HOME users lose bundled system skills.

Testing

  • Not run (automation): bun test cli/src/modules/common/skills.test.ts failed because bun is not installed in this runner.
  • git diff --check passed.

HAPI Bot

Comment thread cli/src/modules/common/skills.ts Outdated
@NightWatcher314 NightWatcher314 force-pushed the fix/flavor-scoped-skills branch from 95db4c1 to 7aff6f5 Compare May 23, 2026 17:32
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Findings

  • No high-confidence issues found in the latest diff.

Questions

  • None.

Summary

  • Review mode: follow-up after new commits
  • Previous bot finding around custom CODEX_HOME system skills appears addressed by the new shouldIncludeCodexSystem() path check and added test coverage. Residual risk: I could not execute the Bun test/typecheck commands in this runner.

Testing

  • Not run (automation): bun is not installed in this runner.
  • git diff --check passed.

HAPI Bot

@tiann tiann merged commit c417330 into tiann:main May 24, 2026
3 checks passed
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.

Scope skill completion to the active agent and include marketplace-installed skills

2 participants