Skip to content

Fix orchestration skill coverage for provider-home skill roots (#8256) - #8510

Merged
nwparker merged 2 commits into
mainfrom
nwparker/bugbash-8256
Jul 14, 2026
Merged

nwparker merged 2 commits into
mainfrom
nwparker/bugbash-8256

Conversation

@nwparker

@nwparker nwparker commented Jul 13, 2026 •

Copy link
Copy Markdown
Contributor

Summary

The Orchestration → Agent coverage panel reported the orchestration skill as Missing for agents that actually have it installed. For a user who installed the skill for every agent, the panel showed only "2 of 8 detected agents have the skill" (Claude + Codex ready; Claude Agent Teams, Grok, OpenCode, Pi, Antigravity, Cursor all Missing).

This teaches skill discovery + coverage about each agent's own global skills directory, so an install performed with npx skills add … --global is credited to the right agent.

Root cause

The reporter's symlink framing is a red herring — symlink following in src/main/skills/discovery.ts (findSkillFiles, lines ~101-127) is already correct and tested. The real cause is two gaps:

  1. src/main/skills/skill-discovery-sources.ts:32-45 — buildSkillDiscoverySources only scanned ~/.codex/skills, ~/.agents/skills, ~/.claude/skills, and ~/.codex/plugins/cache. It never scanned the per-provider home skill roots that npx skills add --global writes to, so those installs were never discovered.
  2. src/renderer/src/lib/orchestration-skill-coverage.ts:24-57, :103 — the coverage layer had no location definitions for those provider roots and no agent→location mappings, so every unmapped agent fell back to ['agents-home']. claude-agent-teams was also unmapped despite reading the same ~/.claude/skills as Claude Code.

Issue #8503 independently confirmed the exact symptom (~/.pi/agent/skills/orchestration present, Pi shown Missing).

The fix

Added the provider home skill roots (provider agent-skills) to discovery, matching coverage location definitions, and per-agent mappings. Each path was verified against the repo's own config-path helpers (not blindly trusted):

Agent Skill root Codebase evidence
grok ~/.grok/skills src/main/grok/hook-service.ts (${home}/.grok)
opencode ~/.config/opencode/skills OPENCODE_CONFIG_DIR="$HOME/.config/opencode"
pi ~/.pi/agent/skills PI_CODING_AGENT_DIR = ~/.pi/agent; #8503 example
antigravity ~/.gemini/antigravity/skills src/main/antigravity/hook-service.ts (~/.gemini/…)
cursor ~/.cursor/skills src/main/cursor/hook-service.ts (~/.cursor)

Each agent maps to ['<provider>-home', 'agents-home'], and claude-agent-teams maps to ['claude-home', 'agents-home'] (it runs Claude Code). Coverage matching reuses the file's existing pathContainsSegments contiguous-segment matcher (case-insensitive, backslash-normalized) so Windows/SSH paths work, and the sourceKind !== 'repo' guard keeps repo-scoped installs from counting as global coverage.

Because the discovery fix scans these roots (with symlink following intact), this also resolves the sibling report #8503 (Pi/OpenCode).

Evidence

UI screenshot isn't feasible in headless CI — the co-located unit tests are authoritative.

Before (new tests fail against current main):

 FAIL  src/renderer/src/lib/orchestration-skill-coverage.test.ts > marks each provider-home agent from its own global skills location
   AssertionError: expected false to be true
 FAIL  src/renderer/src/lib/orchestration-skill-coverage.test.ts > marks Claude Agent Teams from ~/.claude/skills like Claude Code
   AssertionError: expected false to be true
 FAIL  src/main/skills/discovery.test.ts > scans each provider home skill root that npx skills --global writes to
   AssertionError: arrayContaining([...grok/opencode/pi/antigravity/cursor...])

 Test Files  2 failed (2)
      Tests  3 failed | 13 passed (16)

After (fix applied):

 Test Files  2 passed (2)
      Tests  18 passed (18)

Also green: pnpm run typecheck:tsc:node, pnpm run typecheck:tsc:web, oxlint on all changed files, and the nearby skill suites (src/main/skills, src/main/ipc/skills.test.ts, useInstalledAgentSkills.test.ts, agent-skill-cli-prerequisite.test.ts — 37 passed).

ELI5

Orca has a checklist that says which of your AI helpers already learned the "orchestration" trick. But Orca was only peeking into two of the drawers where the trick can be stored. If you put the trick in Grok's drawer or Pi's drawer, Orca didn't look there, so it said "missing" even though the helper had it. This makes Orca open every helper's own drawer before deciding.

Trade-offs

None for the default layout. Known limitation (pre-existing, matches how the .codex/.claude/.agents roots already work): the roots are the default locations, so relocation env vars (XDG_CONFIG_HOME/OPENCODE_CONFIG_DIR, GROK_HOME, PI_CODING_AGENT_DIR) aren't honored for skill discovery yet — a reasonable follow-up.

Relationship to existing PR

Connected PR #8329 by @sdhilip200 targets the same bug. I implemented this independently first, then compared: our changes converge on the exact same five provider paths, the same pathContainsSegments segment lists, and the same agent mappings (including claude-agent-teams). That independent convergence is strong validation of the path list. Decision: keep this PR independent. It is functionally equivalent to #8329 but adds: a negative-leak assertion (a provider-home install must not mark unrelated agents), an end-to-end symlinked-discovery test tying #8256 + #8503 together, a Windows-backslash case for a multi-segment provider path, and "why" comments. Full credit to @sdhilip200 for the parallel diagnosis — either could land; if #8329 is preferred, the extra tests here are worth cherry-picking.

Review loops

1 (a fresh independent reviewer returned CLEAN with no must-fix items; one non-blocking suggestion — a Windows-path case for the new multi-segment locations — was adopted).

⚠️ Bug-bash PR — please review; do not merge yet.

Fixes #8256

Made with Orca 🐋

Add the per-provider global skill directories that npx skills --global
writes to (grok, opencode, pi, antigravity, cursor) to skill discovery,
plus matching coverage location definitions and per-agent mappings, and
map claude-agent-teams to the same ~/.claude roots as Claude Code.

Fixes #8256

Co-authored-by: Orca <help@stably.ai>
@coderabbitai

coderabbitai Bot commented Jul 13, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: ff358acf-09ea-434d-8d24-0126a6fd073d

📥 Commits

Reviewing files that changed from the base of the PR and between cc59286 and 821849c.

📒 Files selected for processing (4)
  • src/main/skills/discovery.test.ts
  • src/main/skills/skill-discovery-sources.ts
  • src/renderer/src/lib/orchestration-skill-coverage.test.ts
  • src/renderer/src/lib/orchestration-skill-coverage.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/main/skills/skill-discovery-sources.ts
  • src/main/skills/discovery.test.ts
  • src/renderer/src/lib/orchestration-skill-coverage.test.ts

📝 Walkthrough

Walkthrough

Expanded skill discovery to scan Grok, OpenCode, Pi, Gemini, Antigravity, and Cursor home directories, including symlinked skills under provider roots. Extended orchestration coverage identifiers and agent mappings for these providers and Claude Agent Teams. Added tests for provider-specific coverage, Windows-style OpenCode paths, distinct Gemini and Antigravity handling, and provider-home skill discovery.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is detailed but missing required template sections for Screenshots, Testing, AI Review Report, Security Audit, and Notes. Add the missing template sections, including 'No visual change' if applicable, the testing checklist, AI review report, security audit, and notes.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: fixing orchestration skill coverage for provider-home skill roots.
Linked Issues check ✅ Passed The changes address #8256 by adding provider-specific skill roots, symlink-aware discovery, and coverage mappings for the affected agents.
Out of Scope Changes check ✅ Passed The added tests and mappings stay focused on the skill-discovery and coverage fix, with no unrelated feature work apparent.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

…skills)

Co-authored-by: Orca <help@stably.ai>
@nwparker

Copy link
Copy Markdown
Contributor Author

Adversarial hardening review — 3 rounds

Ran 3 hostile rounds (fresh reviewer subagent each). Baseline before/after each change: focused vitest (discovery.test.ts + orchestration-skill-coverage.test.ts), typecheck:tsc:node, typecheck:tsc:web, oxlint, and switch-exhaustiveness — all green (19 tests pass).

Change applied (1 should-fix)

Added the Gemini CLI home skill root — ~/.gemini/skills (home-gemini discovery source + gemini-home coverage location + gemini: ['gemini-home','agents-home'] mapping), symmetric with the other provider roots.

Why: the authoritative skills CLI supported-agents table writes Gemini CLI's global skills to ~/.gemini/skills/, and gemini is a first-class detectable/auto-pick agent. The PR already covered its sibling antigravity (~/.gemini/antigravity/skills) but left plain gemini falling through to the agents-home default — the exact false-negative class #8256/#8503 fix. Segment matching keeps .gemini/skills and .gemini/antigravity/skills strictly distinct (added a dedicated sibling non-collision test asserting all four directions). Additive-safe (false-negative-only, no regression).

Provider paths — verified against the skills CLI (authoritative) + repo helpers

All 5 original paths CONFIRMED exactly: ~/.config/opencode/skills, ~/.pi/agent/skills, ~/.gemini/antigravity/skills, ~/.cursor/skills, and ~/.grok/skills (grok is a Grok-native skills dir per xAI docs; the skills CLI table doesn't list grok, but ~/.grok/skills matches the repo's resolveGrokHomeDir and is additive-safe). The task's suspicion that antigravity/opencode were weakest was refuted — all check out.

Dismissed findings (with reasons)

  • opencode XDG_CONFIG_HOME / GROK_HOME / PI_CODING_AGENT_DIR / OPENCODE_CONFIG_DIR not honored (should-fix + nit) — DISMISSED as correct-as-is. buildSkillDiscoverySources takes homeDir as a parameter precisely because it must be host-portable: WSL passes a Windows UNC home and discovery runs in the Windows main process, while SSH/relay runs the RPC on the remote host. Reading process.env.XDG_CONFIG_HOME there would apply the Windows main-process env to a Linux WSL-target path — a cross-host regression. It also matches the pre-existing ~/.codex/~/.claude roots (which likewise ignore CODEX_HOME). Residual: a local user who relocates a provider config via env var gets a bounded false-negative — pre-existing, additive-safe, not worth a host-unsafe change.
  • claude + claude-agent-teams both flip from one ~/.claude/skills install (nit) — DISMISSED; that's correct (both genuinely read ~/.claude/skills), and the chip pre-dates this PR.
  • ['agent-skills'] provider tag vs provider-specific tag; a slightly-loose test comment (nits) — DISMISSED; provider tag doesn't affect coverage (matching keys on rootPath only) and is internally consistent across all new roots.

Per-dimension verdicts

  • Correctness/regressions: PASS — segment (not substring) matching; no cross-contamination across any of the 9 location matchers; isGlobalOrchestrationSkill still gates sourceKind !== 'repo'; all mappings additive (every agent keeps agents-home), so nothing that was "Ready" becomes "Missing".
  • Perf: PASS — discovery is async IPC (not a sync/hook hot path), cached + in-flight-deduped per target; the +6 roots are stat-gated (non-existent roots return immediately), walks bounded (depth 4, MAX_SKILL_FILES=200, realpath cycle-guard).
  • Windows: PASS — all path.join, no POSIX/string concat; matchers backslash-normalize; Windows-path tests included.
  • Linux: PASS — ~/.config XDG-default semantics; segment matching case-insensitive.
  • macOS: PASS.
  • SSH/relay + WSL: PASS — roots resolve on the correct host (RPC runs on remote host; WSL injects a UNC home via getWslHome), identical to the existing .claude/.codex roots.
  • Scope: PASS — 4 files only (2 product + 2 test).
  • Test efficacy: PASS — all new tests fail on un-fixed code and exercise real code paths (verified factory defaults reach pathContainsSegments).

Verdict: ship it. 3 rounds, 0 must-fix, 1 should-fix applied (Gemini root), remaining items dismissed with reasons.

@nwparker
nwparker merged commit 5886ffa into main Jul 14, 2026
1 check passed
@nwparker
nwparker deleted the nwparker/bugbash-8256 branch July 14, 2026 01:13
brennanb2025 pushed a commit that referenced this pull request Jul 29, 2026
Settings → Orchestration → Agent coverage decided whether each detected agent had the orchestration skill by matching a skill's single `rootPath` against a hand-maintained table of path segments. Skill discovery dedups by canonical file path, so when one provider home is a symlink onto another agent's skills directory — which `npx skills add --global` does — the two collapse into one row and the absorbed roots survive only in `rootPaths`, which the old matcher ignored. With `~/.grok/skills` symlinked onto `~/.claude/skills`, Claude read Ready and Grok read Missing even though Grok loads the same files.

Now classifies every entry in `rootPaths` and resolves each root through the `SkillDiscoverySource` the scanner already returns, accepting it when `sourceKind` is not `repo` and `owner` is either the agent's owner or `null`. Deletes `ORCHESTRATION_SKILL_LOCATIONS` and `ORCHESTRATION_SKILL_LOCATION_IDS_BY_AGENT` — a renderer-side mirror of `buildSkillDiscoverySources` that had to be hand-edited for every new provider root, which is what caused this class of bug.

No discovery roots change here; those landed in #8510.

Fixed during review:
- Restored the required `{ kind: 'local' }` argument to `useDetectedAgents`. Dropping it failed typecheck (TS2554) and pinned `detectedIds` to null, leaving the widget on "Checking installed agents and skill paths…" forever. It also reverted the remote-host scoping from #9790.
- Stopped a duplicate repo root from shadowing an owning home root. `SkillDiscoverySource.path` is not unique: when the scan cwd is the home directory, `~/.claude/skills` is scanned as both a home root and a repo root, and a path-keyed Map is last-write-wins — so every agent read Missing while the panel above said Installed. Guaranteed in Settings on a WSL project.
- Carried OMP coverage in the owner-based shape so #6422's coverage-table hunks could be dropped on merge.

Behavior note: an orchestration skill inside an enabled Claude plugin now counts for Claude. That is a deliberate consequence of trusting the scanner's ownership data, matches how the Codex plugin cache was already treated, and is pinned by a test.

Refs #8256

Co-authored-by: sdhilip200 <49802211+sdhilip200@users.noreply.github.com>
enrick-dev pushed a commit to enrick-dev/orca that referenced this pull request Oct 15, 2026
enrick-dev pushed a commit to enrick-dev/orca that referenced this pull request Oct 15, 2026
Settings → Orchestration → Agent coverage decided whether each detected agent had the orchestration skill by matching a skill's single `rootPath` against a hand-maintained table of path segments. Skill discovery dedups by canonical file path, so when one provider home is a symlink onto another agent's skills directory — which `npx skills add --global` does — the two collapse into one row and the absorbed roots survive only in `rootPaths`, which the old matcher ignored. With `~/.grok/skills` symlinked onto `~/.claude/skills`, Claude read Ready and Grok read Missing even though Grok loads the same files.

Now classifies every entry in `rootPaths` and resolves each root through the `SkillDiscoverySource` the scanner already returns, accepting it when `sourceKind` is not `repo` and `owner` is either the agent's owner or `null`. Deletes `ORCHESTRATION_SKILL_LOCATIONS` and `ORCHESTRATION_SKILL_LOCATION_IDS_BY_AGENT` — a renderer-side mirror of `buildSkillDiscoverySources` that had to be hand-edited for every new provider root, which is what caused this class of bug.

No discovery roots change here; those landed in stablyai#8510.

Fixed during review:
- Restored the required `{ kind: 'local' }` argument to `useDetectedAgents`. Dropping it failed typecheck (TS2554) and pinned `detectedIds` to null, leaving the widget on "Checking installed agents and skill paths…" forever. It also reverted the remote-host scoping from stablyai#9790.
- Stopped a duplicate repo root from shadowing an owning home root. `SkillDiscoverySource.path` is not unique: when the scan cwd is the home directory, `~/.claude/skills` is scanned as both a home root and a repo root, and a path-keyed Map is last-write-wins — so every agent read Missing while the panel above said Installed. Guaranteed in Settings on a WSL project.
- Carried OMP coverage in the owner-based shape so stablyai#6422's coverage-table hunks could be dropped on merge.

Behavior note: an orchestration skill inside an enabled Claude plugin now counts for Claude. That is a deliberate consequence of trusting the scanner's ownership data, matches how the Codex plugin cache was already treated, and is pinned by a test.

Refs stablyai#8256

Co-authored-by: sdhilip200 <49802211+sdhilip200@users.noreply.github.com>
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.

[Bug]: Skill detection fails for symlinked skills in most supported agents

1 participant