Repository navigation
fix(native-chat): list OMP skills in the / menu as /skill:<name> - #31999
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linked Issues check |  |
Issue #31996 requires OMP skills in the Skills group from scanned and session-reported sources. In the reviewed head, NATIVE_CHAT_AGENT_PROFILES has no omp entry in `src/shared/native-chat-agent… |
Add the OMP profile with skillPrefix: '/', skillSourceOwner: 'omp', and skillNamePrefix: 'skill:'. Add skillCommandPrefix?: string to AcpDialect, set it to 'skill:' in the OMP dialect, and use it in `AcpStructuredOptions.adoptCo… |
| Docstring Coverage | |
Docstring coverage is 29.41% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 31 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: displaying OMP skills in the native-chat slash menu. Description check The description includes all required sections, explains the user-visible behavior and implementation, links issue #31996, documents testing, and addresses cross-platform and compatibility considerati…Out of Scope Changes check The reported command discovery, ACP lifecycle, shared autocomplete, mobile integration, and related tests support OMP command and skill discovery or exercise shared behavior required by issue #31996. …Full details: Description check
-
Explanation
The description includes all required sections, explains the user-visible behavior and implementation, links issue
#31996, documents testing, and addresses cross-platform and compatibility considerations.
Full details: Out of Scope Changes check
-
Explanation
The reported command discovery, ACP lifecycle, shared autocomplete, mobile integration, and related tests support OMP command and skill discovery or exercise shared behavior required by issue
#31996. The available evidence does not demonstrate a concrete unrelated product change.
-
Full details: Linked Issues check
-
Explanation
Issue
#31996requires OMP skills in the Skills group from scanned and session-reported sources. In the reviewedhead,NATIVE_CHAT_AGENT_PROFILEShas noompentry insrc/shared/native-chat-agent-profiles.ts.AcpDialecthas noskillCommandPrefix, andAcpStructuredOptions.adoptCommandsassignskind: 'command'to every adopted command. The required OMP profile and ACP classification are therefore not present. The available diff could not be read because changed blobs are unavailable, but the head file evidence shows the unmet requirements.Resolution
Add the OMP profile with
skillPrefix: '/',skillSourceOwner: 'omp', andskillNamePrefix: 'skill:'. AddskillCommandPrefix?: stringtoAcpDialect, set it to'skill:'in the OMP dialect, and use it inAcpStructuredOptions.adoptCommandsto classify matching entries askind: 'skill'. Retain tests for scanning, session reports, Skills grouping, and selection.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
76579684-6678-49d9-b65a-f90ea08ad194
-
mobile/src/session/mobile-native-chat-composer-menu.tsmobile/src/session/use-mobile-structured-agent-session-send.test.tsxsrc/main/acp/acp-structured-acquire.tssrc/main/acp/acp-structured-command-publication.test.tssrc/renderer/src/components/native-chat/native-chat-composer-state.test.tssrc/renderer/src/components/native-chat/native-chat-composer-state.tssrc/renderer/src/components/native-chat/use-native-chat-composer-catalog.test.tsxsrc/renderer/src/components/native-chat/use-native-chat-composer-catalog.tssrc/shared/__fixtures__/omp-native-commands.jsonsrc/shared/native-chat-picker-items.scaling.test.tssrc/shared/native-chat-picker-triggers.tssrc/shared/native-chat-slash-commands.omp.test.tssrc/shared/native-chat-slash-commands.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
-
There was a problem hiding this comment.
Actionable comments posted: 2
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
6b296de3-a394-4644-b916-1574c62f02b1
-
mobile/src/session/mobile-native-chat-picker-scaling.test.tsmobile/src/session/use-mobile-structured-session-commands.test.tsxsrc/main/acp/acp-at-rest-commands.test.tssrc/main/acp/acp-at-rest-commands.tssrc/main/acp/acp-command-discovery.test-fixture.tssrc/main/acp/acp-session-discovery.tssrc/main/acp/acp-structured-command-publication.test.tssrc/main/acp/acp-structured-session-adapter-deps.tssrc/main/acp/acp-structured-session-adapter.tssrc/main/native-chat/agent-session-wire/at-rest-command-catalog.tssrc/main/native-chat/agent-session-wire/structured-agent-session-at-rest-commands.tssrc/main/runtime/structured-agent-launch-parity.test.tssrc/main/runtime/structured-agent-runtime-registrations.tssrc/renderer/src/components/native-chat/use-native-chat-composer-catalog.test.tsx
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.
-
|
OMP skills were missing from a fresh native chat's slash menu because the empty chat had not started a provider session. The change obtains the configured native runtime's command list through the existing discovery catalog before the first message. Enabled The full review fixed older-client compatibility, disabled-skill suggestions from disk, late native command publication, quadratic description lookup, first-open discovery and managed-root admission cleanup. Three ordinary review rounds, the convergence decision, architecture panels, focused verification, performance/readiness checks and the final whole-mechanism comparison are complete. No open source finding remains. The in-scope source, compatibility and ownership differences are intended adaptations. Internal worker demand remains inherited at this pinned head; its separate fix is already merged as #34342. The retained UI catalog notification path is unchanged. Discovery uses the configured executable/account/environment and physical workspace cwd. It owns a disposable native ACP session independently of the conversation, admits two active/closing managed roots and 32 waiters, and separates the nine-second answer deadline from teardown. It never starts a user turn or gates a manual send. Missing/failed reports retry; explicit native empty lists and later live reports retain authority. Wire rows remain command-kind for older clients; shared presentation classifies OMP skill tokens. No persisted format changed. Verification:
Current reviewed head is cbfa417. Its reviewed merge is 4a577d5, combining reviewed 74d8e2b with pinned main 964c64b after a conflict blocked merging. Main rewrote the shared catalog callback and lifecycle. The resolution retains its single current attempt, generation/admission/disposal and complete shared Codex owners while preserving OMP's nine-second answer, ten-second failed-attempt recovery and retained native rows. The existing signal waiter bounds the source answer independently of process cleanup; the common folder check treats OMP absence as retryable unavailability. Main's existing-source defaults remain intact. Four internal source policies express those existing obligations; no parallel cache, retry service, saved format or send gate was added. The integration changes 33 production lines in two files, with targeted protocol/queue/recovery/performance tests and honest filesystem fixtures. Seven root files pass 63 tests, including the final strengthened 24-test ACP file; three mobile files pass 23. Tests cover three failed reports then recovery, eight healthy queued contexts, missing/restored folders, retained idle rows and repeated failed demand. Main's pool code is unchanged; performance instrumentation counts pool removals and removable-waiter work separately at 16/160 folders. Initial clock/fixture and aggregate-counter failures remain retained alongside corrected final passes. The required direct merge hook passes, the tree is clean, and the lockfile is unchanged. One fresh independent reviewer agrees that the selected source contracts, main defaults and complete process-pool repair are preserved; the final whole-mechanism reference comparison is complete. No broken flow was found. Under the main-merge rule, including conflicts, the prior QA evidence carries; no new runtime QA pass is claimed. The first fresh CI failure is a test-only AST ratchet finding in the new eight-context fixture; no shipped-flow failure was demonstrated. Test-only 0c931a4 replaces the new fixture timer wait with explicit close gates. Test-only 1e99b10 observes actual acquisition/lease completion before beginning the next failed-demand wave: filesystem disappearance alone preceded real removal settlement and permit release. The complete 24-test file passes through tc-gate on the final tree, with every original bound and assertion retained; both ordinary hooks pass and exact-file AST findings are zero. Initial failed fixture/barrier attempts remain retained. No production code, AST rule, baseline or timeout was changed by either repair. One continued independent reviewer agrees that all original assertions, real close behavior and completion obligations are preserved. These non-shipped commits preserve QA evidence. Fresh CI at 1e99b10 actually compiled all four projects and passed static checks, mobile, four unit shards, wire/relay integration, packaging, three WSL distros and persistence. Its remaining shard failed two within-wave child-count assertions in this same fixture: independently timed cancellations could interleave with real owner completion. A fresh panel confirmed the repeat and selected test-only cbfa417: hold the existing failed-child close gates until every wave answer/cancellation settles, then perform original close, removal, real release and the idle barrier. Failure cleanup releases held gates before disposal/drain. All 106 original assertion tokens, values and order remain; two whitespace-only formatter changes are recorded. The entire named file passes 24/24 in the actual local Bun 1.4.3 project through tc-gate, and its normal hook passes. Fresh CI confirms the repaired ACP file passes all 24 tests without skips in the actual Bun 1.4.4 project, and all four compiler projects execute without type errors. The final-head CI run is green: static analysis/typecheck, all five unit shards, relay integration, cross-version wire compatibility, mobile bundle, Linux/Windows packaging and final selection evidence pass. Mobile verification, all three WSL distros, and persistence pass. The one watch on this head exited successfully, with no retry. All 43 final checks are successful or skipped (26/17). No production policy, owner, timer, threshold or assertion was changed, and no new runtime pass or observed CI callback trace is claimed. Earlier reviewed merge 74d8e2b combined reviewed fa82a6a with pinned main e310a63 after a conflict blocked merging. Four conflicts were resolved narrowly: main's shared/mobile composer extraction and optional current-account resolver coexist with native reported skill availability, exact token insertion, cancellation and managed-root ownership. Two mobile test harnesses now exercise the extracted shipping APIs, preserving first-menu, exact-send and scaling assertions. Eight named root files pass 120/120 tests and four mobile files pass 35/35 through tc-gate. The required direct merge hook passes after a test-only brace correction; the legitimate incoming lockfile exactly matches pinned main, with no setup churn. One independent reviewer agrees both parent intents and closed fixes are preserved. Shipped code was integrated, but no new PR flow or proven broken flow was found. Under the main-merge rule, including conflicts, existing QA and reference evidence carry; no new runtime pass is claimed. Earlier reviewed merge fa82a6a combined reviewed 818f585 with pinned main ffed50d after the merge was blocked. Git found no textual conflict, but six test imports were retargeted to main's relocated shared input modules. Every assertion is unchanged. The overlapping production implementations retain the same behavior after import relocation and removal of intermediary reexports; the first-description index and native skill authority are preserved. The six root files pass 110/110 tests and three mobile files pass 21/21 through tc-gate. The required direct merge hook passes, with no setup lockfile churn. One independent reviewer agrees that both parent intents, assertions and moved picker behavior are preserved. Manual resolution changes only test imports and introduces no shipped-flow change or proven broken flow. Under the main-merge staleness rule, including conflicts, prior QA carries. Earlier merge 818f585 combines reviewed f3b4740 with pinned main c23ab55, including merged #34256's terminal-send line-limit repair. There are no conflicts, resolutions or overlapping PR files; all 26 PR paths are byte-identical to the prior reviewed head. The direct required merge hook passed with an unchanged tree and lockfile, and independent narrow verification agrees that both intents are preserved. Earlier merge f3b4740 combines reviewed c348e5d with pinned main 75bcd32. It includes merged #34202's repairs for all three inherited compiler/ratchet failures. The sole overlapping PR file retains main's extracted option-writer import. A redundant function-type annotation was removed to keep the adapter within its existing line limit; parameter/return signature, emitted runtime body, field order and teardown semantics are unchanged. All 41 named ACP tests and the required direct merge hook pass, with independent verification AGREE and no lockfile change. QA and reference evidence carry. Earlier merge c89d62d additively merges reviewed a86bafd with pinned main 7f846fd after main repaired the inherited CI limits through #34076 and subsequent consolidation. There were no conflicts or manual edits. Twenty-five PR paths are byte-identical to the reviewed parent; the sole overlap preserves main's launch-parity fixture additions alongside the unchanged OMP discovery contract. The required direct merge hook passed without changing the tree. Independent narrow verification agrees that both parent intents, all closed fixes and reference evidence are preserved. Earlier merge a86bafd brought pinned main 33a86f3 into 7365cf and aligned five mobile test import/mock paths after main's composer relocation; all 21 affected tests passed, with assertions unchanged. Both additive main merges preserve passed QA flows, with refs=done. Test-only c348e5d moves an existing filesystem module load before test execution to meet main's new test-import rule; all cases/assertions and actual cleanup semantics are unchanged. Its 18 named tests and normal hook passed, with independent verification AGREE. This commit preserves runtime QA evidence. Windows ownership QA built 6d98736. The later b644 readonly owner alias changes no flow, independently proven by full-file byte identity after expansion; the main merge preserves the passed flows. Earlier first-open/phone evidence at 4b615cc and Claude/Codex controls at 895f14b carry for unchanged flows. Main comparison used d4cab5c. This PR retains the existing host-owned catalog notification path: a changed catalog visits registered client subscribers and sends command updates when the list reference changes; phone menus keep their one-shot read. The separate worker-demand fix is merged as #34342 and does not change UI catalog fanout; it is outside the pinned head reviewed here. No future UI-fanout follow-up is claimed. Cleanup follows existing managed-root completion: after managed close settles and root exit is authoritative, discovery awaits removal of that probe's scratch before releasing admission, irrespective of historical descendant diagnostics. Live or unverifiable roots retain ownership; actual later exit runs the same tracked removal-before-release cleanup. Unresolved shutdown rejects. Failed tree teardown can leave descendants beyond the two roots and scratch removal can race survivors. The real Windows run observed root exited/tree null; it did not manufacture failed OS tree teardown. Deterministic contracts cover those outcomes. No universal descendant-containment claim is made. Evidence limits: the phone's raw first RPC response/timing was not captured; first-menu UI/native chronology was. First-open creation→native-report upper bounds 3.058s desktop/0.986s phone/0.845s disabled are not RPC latencies. No unadvertised namespace was exercised. Reload produced acknowledgment/end-turn; isolated late publication is proven by the host/subscriber contract. Existing global catalog publication still visits subscribers. The new Node-side mobile integration is runtime-tested, excluded from the RN compiler through the existing convention; ordinary mobile hooks and desktop production retain compiler gates. Earlier Mac Codex config and Windows For Brennan's review:
Tracked in STA-11735 and #31996. Contributor's original description predates the additive review fixes; the mechanism and evidence above describe the final change. Windows native A/B/C contexts and retained chat across graceful restart:
Graceful restart: same C empty chat before → after. Visual selected-tab identity matches; raw retained-identity and literal graceful-stop proof are in the audit artifacts.
Pending probe retirement: existing B chat after native Command restoration. Exact pending timing/abort-reason proof is in the raw chronology; this still image alone does not prove cancellation.
The ordinary native Command setting restored during the proven pending retirement:
The marker retains its carried QA/review head 7365cf and actual qa_head=6d987369; additive merges a86bafd, c89d62, f3b474, 818f585 and fa82a6a plus test-only c348e5d are described above. The runtime-equivalent annotation cleanup was independently verified and changes no QA flow. |
ELI5
OMP tells Orca which skills a session loaded, naming them
skill:<name>and expanding/skill:<name>when they arrive as text. Orca had no entry for OMP in its per-agent profile table, so it treated OMP as an agent with no skills grammar and hid every skill from the/menu. It also labelled all of OMP's ACP commands as commands, so even a session report could not put them in the Skills group. This adds the missing profile and teaches the ACP option layer to recognise theskill:prefix.What Changed
Before: typing
/in an OMP chat offered only/model,/effort,/clear,/compact,/resume./skill:plannever completed and no Skills group appeared, even thoughomp acpsendsavailable_commands_updateentries like{"name":"skill:plan", …}.After: OMP skills appear as
/skill:<name>rows in the picker's Skills group, both from thehome-ompskill scan and from what the live session reports. Picking one inserts/skill:<name>.Two parts:
src/shared/native-chat-agent-profiles.ts— anompprofile shaped like Pi's:skillPrefix: '/',skillSourceOwner: 'omp',skillNamePrefix: 'skill:'.deriveSlashAutocompletegates onprofile !== null, so this alone re-enables scanned and reported skills for OMP.src/main/acp/acp-dialects/acp-dialect.ts+omp-dialect.ts+src/main/acp/acp-structured-options.ts—new
skillCommandPrefix?: stringdialect field, set to'skill:'for OMP, andadoptCommandstags matching entries
kind: 'skill'instead of'command'. ACP commands carry no source field,so the name prefix is the only signal available.
Tests:
acp-structured-options.test.ts:reports OMP skill: commands as skills and the rest as commands.native-chat-composer-state.test.ts:lists OMP skills as /skill:<name>, and only the ones its session reports once it does.native-chat-slash-commands.omp.test.ts: the existing assertion that OMP has no profile is replaced by an assertion on the profile's shape — the previoustoBeNull()would otherwise fail on this change.Why
The alternative was to special-case OMP inside
deriveSlashAutocompleteor to injectkind: 'skill'from the ACP command name in the renderer. Both spread agent knowledge into shared derivation code. The profile table already exists for exactly this (Codex$sigil, Piskill:prefix), and the dialect is already the place where per-agent wire quirks live, so the fix stays declarative.Linked Issue
Fixes #31996
Visual Proof
N/A — no new element is added to the UI; the existing Skills group simply populates for OMP the way it already does for Pi. I can attach before/after captures of the
/menu if a reviewer wants them, but I did not want to pad the PR with screenshots of a headless session.Testing
Manual UI verification was not performed in a desktop client; the composer derivation and the ACP command tagging are both covered by the tests above.
AI Disclosure
Patches and tests were drafted with an AI coding agent (OMP, running an OmniRoute-hosted chat model), then reviewed, rebased, and verified on macOS by a human (the reporter). Test/typecheck/lint output above comes from that human-run verification.
Review
Agent skill upstream boundary
Notes
Related: #30739 (Pi) is the same missing-profile shape. OMP's dialect already handles its own quirks, so no other agent's behaviour changes: the profile entry and the dialect field are opt-in, and
skillCommandPrefixis unset for every other dialect.Cross-platform: pure shared/main TypeScript; no platform-specific paths or APIs.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred) — typecheck plus targeted vitest/oxlint/oxfmt were run locally; fullpnpm lint/pnpm test/pnpm buildare left to CI.