Skip to content

Add OMP skill discovery source - #6422

Merged
brennanb2025 merged 4 commits into
stablyai:mainfrom
caioribeiroclw-pixel:caio/omp-skill-discovery
Jul 29, 2026
Merged

brennanb2025 merged 4 commits into
stablyai:mainfrom
caioribeiroclw-pixel:caio/omp-skill-discovery

Conversation

@caioribeiroclw-pixel

@caioribeiroclw-pixel caioribeiroclw-pixel commented Jun 26, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds OMP as a first-class skill provider in skill discovery, scanning ~/.omp/agent/skills alongside Codex, Claude, and Agent Skills homes.

This follows the confirmation in #6369 that OMP consumes skills from ~/.omp/agent/skills, so future OMP users/devices do not need a manual symlink for Orca's skill discovery UI to see those skills.

Changes

  • Add omp to SkillProvider.
  • Add a home discovery source: OMP home → ~/.omp/agent/skills.
  • Add the OMP provider label in the skills page.
  • Extend the skill discovery test to cover an OMP skill package and source root.

Screenshots

No visual behavior change beyond adding the OMP provider label for discovered OMP skills in the existing skills list UI.

Testing

  • pnpm lint — not run separately; this PR only touches a typed provider union/source list, UI label map, and a focused discovery test.
  • pnpm exec tsgo --noEmit -p config/tsconfig.node.json
  • pnpm exec tsgo --noEmit -p config/tsconfig.tc.web.json
  • pnpm vitest run --config config/vitest.config.ts src/main/skills/discovery.test.ts
  • pnpm build — not run; targeted typechecks and discovery tests passed.
  • Added or updated high-quality tests that would catch regressions, or explained why tests were not needed
  • git diff --check

Note: this environment uses Node v22.22.2, so pnpm install --frozen-lockfile printed the repo's Node 24 engine warning. The targeted tests/typechecks above still passed.

AI Review Report

I reviewed the change for the main compatibility risks touched by this PR:

  • provider enum/string consistency across shared types, main-process discovery, renderer labels, and tests;
  • path handling for macOS/Linux/Windows, especially os.homedir() plus path joining rather than shell expansion;
  • no changes to shortcuts, shell execution, Electron IPC, or runtime skill loading behavior;
  • no accidental broadening of scans beyond the explicit OMP skill home.

The generated review bot did not produce actionable code comments. It did flag the missing PR-template sections, which this update fills in.

Security Audit

Security-sensitive review points:

  • This PR does not execute skill content, install dependencies, invoke shell commands, or add network access.
  • It only adds one additional local discovery directory: ~/.omp/agent/skills.
  • The directory is constructed from the user's home directory and fixed path segments, not from user input.
  • No credentials, auth state, secrets, or config mutation paths are touched.
  • No Electron IPC boundary changes are introduced.

Follow-up: if Orca later supports configurable skill homes, that path should be validated and represented distinctly from fixed provider homes.

Notes

The provider label is intentionally OMP to match the user-facing provider name in #6369. The source label is OMP home, mirroring the existing Codex home / Claude home pattern.

ELI5

Skill discovery includes OMP’s home folder (~/.omp/agent/skills) as a first-class source. OMP skills show up in Orca without manual symlinks.

@coderabbitai

coderabbitai Bot commented Jun 26, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

SkillProvider now includes omp. Skill discovery adds ~/.omp/agent/skills as home-omp with provider omp. The discovery test covers an OMP-backed Planning skill and adds the local scan-root assertion. The skills page maps omp to the OMP display label.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
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 (4 passed)
Check name Status Explanation
Title check ✅ Passed Title is concise and matches the main change: adding OMP skill discovery support.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description follows the required template and includes summary, screenshots, testing, AI review, security audit, and notes with cross-platform coverage confirmed.

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.

@nwparker nwparker added the enhancement New feature or request label Jul 27, 2026
Rebuild the OMP home source on main's six-argument `source()` signature,
which now carries a pathApi adapter and an owning agent.
…ation

- Drop the `SkillProvider` widening: `~/.omp/agent/skills` is a provider home
  like `~/.pi/agent/skills`, so it carries `agent-skills` and identifies OMP
  through the source `owner`. That keeps every `Record<SkillProvider, ...>`
  (including the Skills page label map) at a zero diff.
- Add the `omp-home` orchestration coverage location and map OMP to it, so the
  Agent coverage panel stops reporting a real OMP install as missing.
Both roots end in agent/skills, so dropping either leading segment let one
agent's install mark the other with a green suite. Also corrects the OMP
owner assertion's rationale: no OMP native-chat profile exists, so owner
matters because a null owner leaks OMP-only skills into other pickers.
@brennanb2025

Copy link
Copy Markdown
Contributor

Thanks for this — the diagnosis in #6369 was right and the OMP home path is correct. I've pushed a few changes on top so it can land.

De-staled the branch. It was branched off v1.4.102-rc.0 and conflicting. source() has since grown a pathApi adapter and an owner argument, so I merged main and rebuilt the OMP entry on the current signature. Using pathApi.join (rather than the imported join) is load-bearing: it's what makes WSL discovery emit a posix path instead of a backslash path the in-distro scanner can't use.

Dropped the SkillProvider widening. ~/.omp/agent/skills is a provider home exactly like ~/.pi/agent/skills, so it now carries providers: ['agent-skills'] and identifies OMP through the newer source-level owner: 'omp' (AgentType already includes omp). That keeps src/shared/skills.ts and the Skills page label map at a zero diff. Worth flagging why this matters beyond churn: the provider filter dropdown in SkillsPage.tsx is hand-written rather than derived from the union, so a dedicated 'omp' provider would have compiled fine but shipped a badge with no matching filter option — OMP skills would have vanished under every non-"All" provider filter. owner: 'omp' is also what keeps OMP-only skills out of the Codex/Claude/Grok native-chat pickers, which owner: null would have leaked.

Added the half that was missing. orchestration-skill-coverage.ts had no omp entry, so OMP fell through to the default ['agents-home'] and the Settings → Orchestration "Agent coverage" chip could still only read Ready via ~/.agents/skills — i.e. the symlink you were trying to retire. Added an omp-home location and mapped omp: ['omp-home', 'agents-home'].

Tests. Extended the discovery test to cover the OMP root and pin owner, and added a Pi/OMP distinctness guard — both roots end in agent/skills, so dropping either leading anchor made one agent's install mark the other with a green suite. Verified both anchor mutations now fail.

Validated in a dev build with a real ~/.omp/agent/skills/orchestration: the Skills page lists it under source "OMP home" with the Agent Skills badge, and the coverage function marks omp covered from that root alone while pi, claude, and unmapped agents stay uncovered.

Two adjacent gaps are deliberately left alone: coverage matches the singular rootPath rather than rootPaths (already being handled in #8329), and remote OMP skill linking over SSH needs a separate relay change.

@greptile-apps

greptile-apps Bot commented Jul 29, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds OMP (~/.omp/agent/skills) as a first-class skill discovery source alongside the existing Codex, Claude, Grok, Pi, Gemini, and other agent home directories. OMP skills are surfaced in the Orca UI without requiring a manual symlink.

  • skill-discovery-sources.ts: adds a new 'home-omp' scan root (~/.omp/agent/skills) with owner: 'omp' and providers: ['agent-skills'], matching the Pi/Grok/Cursor pattern exactly.
  • orchestration-skill-coverage.ts: adds the 'omp-home' location definition (segment match on ['.omp', 'agent', 'skills']) and maps the omp agent to ['omp-home', 'agents-home'], so orchestration-skill install status is tracked correctly.
  • Tests: discovery.test.ts exercises actual filesystem discovery for an OMP skill; orchestration-skill-coverage.test.ts adds a cross-contamination test verifying Pi and OMP do not mark each other despite sharing the agent/skills path tail.

Confidence Score: 5/5

The change is purely additive — one new scan root, one new location definition, and accompanying tests — with no modifications to existing paths, IPC boundaries, or runtime behavior.

Every added piece mirrors the immediately-preceding Pi entry in structure and naming. The cross-contamination test for Pi vs OMP on a shared path tail, plus the owner-is-omp assertion, cover the only subtle correctness concern. No existing paths, providers, or UI mappings were altered.

Files Needing Attention: No files require special attention.

Important Files Changed

Filename Overview
src/main/skills/skill-discovery-sources.ts Adds OMP home source with correct path, owner, and providers — consistent with all prior agent home entries.
src/renderer/src/lib/orchestration-skill-coverage.ts Adds omp-home location definition and agent mapping; segment matching correctly disambiguates from Pi's identical path tail.
src/main/skills/discovery.test.ts Extends filesystem-level discovery test with an OMP skill and verifies the owner assertion; coverage is well-targeted.
src/renderer/src/lib/orchestration-skill-coverage.test.ts New Pi/OMP cross-contamination test (including Windows-style path) fully covers the shared agent/skills suffix concern.

Reviews (1): Last reviewed commit: "review: guard the Pi/OMP matcher anchors" | Re-trigger Greptile

brennanb2025 added a commit to sdhilip200/orca that referenced this pull request Jul 29, 2026
stablyai#6422 adds an omp-home scan root plus an omp entry in the path-segment
table this PR deletes. The root is what matters; owner: 'omp' already
satisfies the new matcher, so re-express the coverage cases here and the
rebase can drop that PR's coverage hunks with nothing lost.

Also pins the Pi/OMP distinction stablyai#6422 guarded: both roots end in
agent/skills, so a mis-mapped owner would cross-cover them.
@brennanb2025
brennanb2025 merged commit 8272077 into stablyai:main Jul 29, 2026
25 checks passed
brennanb2025 added a commit to sdhilip200/orca that referenced this pull request Jul 29, 2026
stablyai#6422 added an omp-home scan root plus an omp entry in the path-segment
table this branch deletes. Kept its skill-discovery-sources.ts root in
full — without it ~/.omp/agent/skills is never scanned. Dropped its
orchestration-skill-coverage.ts and .test.ts hunks: the tables are gone,
and owner: 'omp' already satisfies the owner-based matcher. Both of its
coverage concerns are carried here in the new shape, including the
Pi/OMP anchor guard.
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
* Add OMP skill discovery source

* review: scope OMP discovery to the shared provider and cover orchestration

- Drop the `SkillProvider` widening: `~/.omp/agent/skills` is a provider home
  like `~/.pi/agent/skills`, so it carries `agent-skills` and identifies OMP
  through the source `owner`. That keeps every `Record<SkillProvider, ...>`
  (including the Skills page label map) at a zero diff.
- Add the `omp-home` orchestration coverage location and map OMP to it, so the
  Agent coverage panel stops reporting a real OMP install as missing.

* review: guard the Pi/OMP matcher anchors

Both roots end in agent/skills, so dropping either leading segment let one
agent's install mark the other with a green suite. Also corrects the OMP
owner assertion's rationale: no OMP native-chat profile exists, so owner
matters because a null owner leaks OMP-only skills into other pickers.

---------

Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com>
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

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants