[companion] feat(skills): list skills through the omp binary (skills --json) - #30
andrebrait wants to merge 2 commits into
Conversation
Install persists OMP_WEB_OMP_BIN when set. The LAN-settings test copied process.env and leaked the host value into the written service env file.
The skills page must list skills with no running session, so a session-keyed RPC source could not be relied on and the pure-Node replica had to stay correct by hand. Ask the omp binary instead: 'omp skills --json' runs the same discovery sessions use, is fresh from disk on every load, and needs no session. The pure-Node scan stays only as a fallback for installs whose binary predates the command.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
There was a problem hiding this comment.
🟡 Changes recommended
The authoritative discovery path is unreachable and production selection behavior is insufficiently tested.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Attempts to use omp-native skill discovery with a local-scan fallback.
Changes:
- Adds
skills_listpayload mapping and RPC-based discovery. - Adds mapping and malformed-input tests.
- Isolates
OMP_WEB_OMP_BINin the systemd test environment.
File summaries
| File | Summary |
|---|---|
lib/skills-service.ts |
Adds RPC discovery and fallback scanning. Critical (3 votes): skills_list is not registered for passthrough, so the authoritative path is unreachable. |
lib/skills-service.test.mjs |
Tests payload mapping and malformed input. Moderate (1 vote): production selection and fallback behavior lack discovery-level coverage. |
bin/omp-web-systemd.test.mjs |
Prevents inherited binary configuration from affecting the systemd test. |
Review details
Suppressed comments (2)
lib/skills-service.ts:303
- The new production selection logic is not exercised by the added tests: they cover only
skillsFromRpcPayload, notdiscoverSkillswith a running candidate, command failure, or the no-session path. That allows the feature to pass its 15 mapping tests while the live branch is unreachable and the documented fallback behavior is unverified; add a discovery-level test with the process/RPC boundary mocked to assert authoritative selection and fallback.
export async function discoverSkills(cwd: string): Promise<SkillsWithDiagnostics> {
const viaRpc = await discoverSkillsViaRpc(cwd);
if (viaRpc) return viaRpc;
lib/skills-service.ts:303
- The PR description requires
omp skills --json, but this path never resolves or executes the omp binary: it only probes already-running RPC wrappers and otherwise invokes the replica scan. Consequently the pre-session skills page still misses omp-only namespace/collision resolution and does not get per-request disk freshness. Please make the binary command the primary source (with this cwd), retaining the scan only when the command is unsupported or unavailable.
const viaRpc = await discoverSkillsViaRpc(cwd);
if (viaRpc) return viaRpc;
- Files reviewed: 2/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const response = (await session.send({ type: "skills_list" })) as unknown; | ||
| if (!isRecord(response) || response.success !== true) continue; | ||
| return skillsFromRpcPayload(response.data); |
Review companion for kahme247/ompweb#110 — identical diff (head
feat/skills-list-clivsupstream/main).What
The skills page now asks the omp binary for its listing:
omp skills --jsonruns the same discovery sessions use and is parsed into the app's SkillInfo shape. The pure-Node scan stays only as a fallback for installs whose binary predates the command.Why
The page is a pre-session surface: it must list skills with no running session, so a session-keyed source could not be relied on, and the replicated scan rules drift from omp's actual discovery (name-collision namespaces being the concrete case). Keying the listing on binary capability instead of session liveness keeps the source stable per install; every exec re-reads disk, so installs, uninstalls and toggles show immediately. Depends on the
skillsCLI landing in oh-my-pi.Testing
npx tsc --noEmitcleannode --experimental-strip-types --test lib/skills-service.test.mjs: 15 pass, including newskillsFromCliPayloadmapping tests