feat: add full Oh My Pi (OMP) support - #256
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
|
Addressed the multi-root session review in f979800. Dashboard discovery now canonicalizes paths, deduplicates by session ID, sorts duplicates by modified time descending with a stable path tie-break, and therefore keeps the newest continued copy rather than root-order priority. Added regression coverage for a newer duplicate in the second root; the production module now passes 9 isolated Rust tests. |
There was a problem hiding this comment.
All reported issues were addressed across 42 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
|
This is on our merge path — could you rebase onto current master? The v0.33.1 release wave landed conflicting changes in the areas this touches. Once it's green we'll do the full review pass. |
f979800 to
5754598
Compare
|
Rebased I also resolved the valid review findings while re-verifying the v0.33.1 contracts: Pi setup now rolls back a newly-added package entry on later failure; OMP doctor can repair shared config without an OMP binary; OMP subprocess errors preserve timeout details and use a 10 MiB buffer; dashboard Pi login-shell discovery precedes OMP fallback; OMP profile names are validated; multi-root dedup uses explicit root priority and canonicalizes roots rather than every session; the homepage support matrix now includes OMP. Post-rebase verification:
|
There was a problem hiding this comment.
Pull request overview
This PR adds first-class Oh My Pi (OMP) support across Magic Context’s CLI, Pi-compatible extension/runtime, dashboard session/model discovery, and documentation—while keeping Pi-compatible session semantics (harness='pi') and shared project memory/database behavior.
Changes:
- CLI: introduce an
ompharness with setup/doctor flows, OMP model discovery, OMP path resolution, and OpenCode → Pi/OMP migration targeting. - Dashboard: expand Pi-compatible session-root discovery to include OMP profile/XDG layouts and add OMP model selector parsing.
- Extension + docs: update Pi extension to support OMP env semantics (agent dir allowlist resolution) and document installation, conflicts, migration, and dashboard behavior.
Reviewed changes
Copilot reviewed 42 out of 43 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates top-level product messaging and setup/doctor guidance to include OMP. |
| packages/plugin/scripts/build-config-docs.ts | Updates generated config-doc preface to reflect shared config across OpenCode/Pi/OMP. |
| packages/pi-plugin/src/subagent-runner.ts | Resolves relative allowlisted extensions from PI_CODING_AGENT_DIR for Pi/OMP parity. |
| packages/pi-plugin/src/subagent-runner.test.ts | Adds test coverage for host agent-dir based allowlist resolution. |
| packages/pi-plugin/README.md | Updates extension docs for shared config paths and adds OMP installation/troubleshooting guidance. |
| packages/pi-plugin/package.json | Publishes explicit OMP extension manifest metadata and keywords/description updates. |
| packages/docs/src/content/docs/reference/dashboard.md | Documents dashboard behavior for Pi-compatible sessions including OMP. |
| packages/docs/src/content/docs/reference/configuration.md | Updates configuration reference header to include OMP. |
| packages/docs/src/content/docs/index.mdx | Updates docs landing page to include OMP and “three harnesses” messaging. |
| packages/docs/src/content/docs/help/troubleshooting.md | Adds OMP-specific restart guidance (/reload-plugins) and wording updates. |
| packages/docs/src/content/docs/help/faq.md | Updates FAQ for shared DB across OpenCode/Pi/OMP and migration to Pi/OMP. |
| packages/docs/src/content/docs/help/compatibility.md | Adds OMP compaction/memory conflict documentation and resolution steps. |
| packages/docs/src/content/docs/getting-started/migrating-between-harnesses.md | Extends migration guide to cover OpenCode → Pi/OMP. |
| packages/docs/src/content/docs/getting-started/introduction.md | Updates intro narrative to include OMP as a supported harness. |
| packages/docs/src/content/docs/getting-started/installation.md | Adds OMP as a first-class install target and documents OMP-specific setup actions. |
| packages/docs/src/content/docs/concepts/mural.md | Clarifies mural support across OpenCode and Pi-compatible hosts (Pi/OMP). |
| packages/docs/src/content/docs/concepts/memory.md | Updates memory-sharing explanation to include OMP. |
| packages/dashboard/src-tauri/src/pi_sessions.rs | Adds OMP profile/XDG root discovery and deduplication across multiple Pi-compatible roots. |
| packages/dashboard/src-tauri/src/commands.rs | Adds omp models --json parsing and OMP model discovery fallback for dashboard. |
| packages/cli/src/lib/v22-backfill-commands.ts | Updates schema-fence restart warning to include OMP. |
| packages/cli/src/lib/paths.ts | Adds OMP path resolution helpers (profiles, overrides, XDG data layout). |
| packages/cli/src/lib/paths-omp.test.ts | Adds tests for OMP path resolution behavior. |
| packages/cli/src/lib/omp-helpers.ts | Adds OMP binary detection, command helpers, settings access, and model/plugin parsing. |
| packages/cli/src/lib/omp-helpers.test.ts | Adds tests for OMP model parsing and command timeout/buffer behavior. |
| packages/cli/src/lib/harness-select.ts | Extends harness selection/validation to include --harness omp. |
| packages/cli/src/index.ts | Updates CLI help text to reflect OMP support and Pi/OMP migration. |
| packages/cli/src/commands/setup.ts | Dispatches setup to OMP setup flow and updates next-steps messaging. |
| packages/cli/src/commands/setup-pi.ts | Refactors setup into a Pi-compatible host abstraction and adds rollback support. |
| packages/cli/src/commands/setup-pi.test.ts | Adds rollback tests for Pi settings package registration. |
| packages/cli/src/commands/setup-omp.ts | Implements OMP setup via Pi-compatible setup host + transactional OMP conflict disabling. |
| packages/cli/src/commands/setup-omp.test.ts | Adds tests for OMP transactional rollback behavior. |
| packages/cli/src/commands/migrate.ts | Extends migration target to --to omp and routes output to OMP sessions root. |
| packages/cli/src/commands/migrate.test.ts | Adds CLI parsing test coverage for --to omp. |
| packages/cli/src/commands/doctor.ts | Dispatches doctor to OMP doctor flow. |
| packages/cli/src/commands/doctor-omp.ts | Adds OMP-specific doctor/repair/issue-report logic. |
| packages/cli/src/commands/doctor-omp.test.ts | Adds tests for healthy OMP doctor flow and config creation when OMP is missing. |
| packages/cli/src/adapters/types.ts | Extends adapter types to include `HarnessKind = ... |
| packages/cli/src/adapters/omp.ts | Adds an OMP adapter covering detection, plugin enablement, cache/log paths, and install hint. |
| packages/cli/src/adapters/omp.test.ts | Adds adapter tests for detecting an enabled plugin via omp plugin list --json. |
| packages/cli/src/adapters/index.ts | Registers OMP adapter in the global adapter list and exports it. |
| packages/cli/package.json | Updates CLI package metadata/keywords to include OMP. |
| CONFIGURATION.md | Updates shared-config + doctor documentation to include OMP behavior. |
| .gitignore | Ignores agent log directories (AgentLogs/, .agent-logs/). |
Suppressed comments (2)
packages/dashboard/src-tauri/src/pi_sessions.rs:154
PI_CONFIG_DIRisn’t trimmed before converting toPathBuf. If the env var contains leading/trailing whitespace it will resolve to a different directory than the CLI uses (which trims), causing session-root discovery to diverge across components.
let config_dir = std::env::var_os("PI_CONFIG_DIR")
.filter(|v| !v.is_empty())
.map(PathBuf::from)
.unwrap_or_else(|| PathBuf::from(".omp"));
packages/dashboard/src-tauri/src/pi_sessions.rs:169
XDG_DATA_HOMEis also consumed without trimming. Trimming avoids treating whitespace-only values as valid and keeps behavior aligned with the CLI path resolver (which trims).
if let Some(xdg_data) = std::env::var_os("XDG_DATA_HOME").filter(|v| !v.is_empty()) {
let app_root = PathBuf::from(xdg_data).join("omp");
append_omp_profile_roots(&mut roots, &app_root.join("profiles"), true);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const reportedAgentDir = options.deps.runOmpCommand(omp.path, ["config", "path"], 10_000); | ||
| if (!reportedAgentDir.ok) | ||
| add(results, "warn", "Could not verify OMP active agent directory"); | ||
| else if (reportedAgentDir.stdout === getOmpAgentDir()) { | ||
| add(results, "pass", `OMP agent directory resolved to ${getOmpAgentDir()}`); | ||
| } else { | ||
| add( | ||
| results, | ||
| "fail", | ||
| `OMP reports agent directory ${reportedAgentDir.stdout}, but Magic Context resolved ${getOmpAgentDir()}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fixed in 9730e23. Doctor now resolves both the reported and expected agent directories before comparison, and the healthy-install regression test returns an equivalent path with a trailing /./ segment.
| if let Some(agent_dir) = std::env::var_os("PI_CODING_AGENT_DIR").filter(|v| !v.is_empty()) { | ||
| roots.push(PathBuf::from(agent_dir).join("sessions")); | ||
| } |
There was a problem hiding this comment.
Fixed in c683ddb. PI_CODING_AGENT_DIR, PI_CONFIG_DIR, and XDG_DATA_HOME now share one trimming helper; blank values are ignored. Added direct regression coverage, and the production session module passes 11 isolated Rust tests.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Thanks for the rebase — we did the full review. The direction is right and several pieces are exactly how we'd want them (probing the distinct
The remaining three: complete the OMP path/config contract ( Happy to re-review as soon as these land — the containment shape you already have (own adapter, own commands) makes them all tractable. |
There was a problem hiding this comment.
2 issues found across 43 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/cli/src/commands/doctor-omp.ts">
<violation number="1" location="packages/cli/src/commands/doctor-omp.ts:92">
P3: The new OMP doctor reimplements the same semver triple-parse/compare that `comparePiVersion` in `commands/setup-pi.ts` already provides for the same Pi-compatible flow. The two functions parse `X.Y.Z` with the identical regex and compare component-by-component, so a future change to how Pi/OMP versions are treated must be made in two places and can drift. Consider extracting a shared semver helper (e.g. in `lib`) and having both the Pi setup and the OMP doctor use it, matching the existing repo convention of shared CLI helpers.</violation>
<violation number="2" location="packages/cli/src/commands/doctor-omp.ts:221">
P2: The OMP path check compares `omp config path` with the agent directory instead of the OMP config path, which can make every healthy installation fail its doctor check. Compare against `getOmpConfigPath()` (and label the result as a config path) or invoke the command that actually reports the agent directory.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| if (!reportedAgentDir.ok) { | ||
| add(results, "warn", "Could not verify OMP active agent directory"); | ||
| } else { | ||
| const reportedPath = resolve(reportedAgentDir.stdout); |
There was a problem hiding this comment.
P2: The OMP path check compares omp config path with the agent directory instead of the OMP config path, which can make every healthy installation fail its doctor check. Compare against getOmpConfigPath() (and label the result as a config path) or invoke the command that actually reports the agent directory.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/commands/doctor-omp.ts, line 221:
<comment>The OMP path check compares `omp config path` with the agent directory instead of the OMP config path, which can make every healthy installation fail its doctor check. Compare against `getOmpConfigPath()` (and label the result as a config path) or invoke the command that actually reports the agent directory.</comment>
<file context>
@@ -0,0 +1,425 @@
+ if (!reportedAgentDir.ok) {
+ add(results, "warn", "Could not verify OMP active agent directory");
+ } else {
+ const reportedPath = resolve(reportedAgentDir.stdout);
+ const expectedPath = resolve(getOmpAgentDir());
+ if (reportedPath === expectedPath) {
</file context>
| return match ? [Number(match[1]), Number(match[2]), Number(match[3])] : null; | ||
| } | ||
|
|
||
| function isOlderThan(value: string | null, minimum: string): boolean { |
There was a problem hiding this comment.
P3: The new OMP doctor reimplements the same semver triple-parse/compare that comparePiVersion in commands/setup-pi.ts already provides for the same Pi-compatible flow. The two functions parse X.Y.Z with the identical regex and compare component-by-component, so a future change to how Pi/OMP versions are treated must be made in two places and can drift. Consider extracting a shared semver helper (e.g. in lib) and having both the Pi setup and the OMP doctor use it, matching the existing repo convention of shared CLI helpers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/commands/doctor-omp.ts, line 92:
<comment>The new OMP doctor reimplements the same semver triple-parse/compare that `comparePiVersion` in `commands/setup-pi.ts` already provides for the same Pi-compatible flow. The two functions parse `X.Y.Z` with the identical regex and compare component-by-component, so a future change to how Pi/OMP versions are treated must be made in two places and can drift. Consider extracting a shared semver helper (e.g. in `lib`) and having both the Pi setup and the OMP doctor use it, matching the existing repo convention of shared CLI helpers.</comment>
<file context>
@@ -0,0 +1,425 @@
+ return match ? [Number(match[1]), Number(match[2]), Number(match[3])] : null;
+}
+
+function isOlderThan(value: string | null, minimum: string): boolean {
+ const left = parseSemver(value);
+ const right = parseSemver(minimum);
</file context>
|
Hey @Lynricsy, please address cubic findings also. Thanks! |
|
@waltino Thanks for the detailed containment review. I addressed all five merge blockers and pushed three atomic commits:
Verification completed:
Two unrelated current-master gates remain visible but were not changed/suppressed here: Dashboard ConfigEditor schema parity has four pre-existing unclassified fields (36 pass / 2 fail), and root/Pi lint reports pre-existing unused-symbol diagnostics outside these touched paths. Rust 1.77 source checking is additionally blocked before project compilation by the current lockfile's The branch is pushed at |
|
Follow-up documentation consistency fix pushed as |
There was a problem hiding this comment.
1 issue found across 20 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/docker/Dockerfile.omp">
<violation number="1" location="tests/docker/Dockerfile.omp:69">
P3: The new OMP E2E image isn't wired into the local runner: `tests/docker/run-e2e.sh` only accepts `opencode|pi|all`, so `run-e2e.sh omp` fails with "Unknown target" and `all` never runs it. Add an `omp` arm to `run_target` so developers can reproduce the CI e2e-omp job locally, matching the existing pi/opencode targets.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| RUN chmod +x /test/test-omp-e2e.sh | ||
|
|
||
| WORKDIR /test/project | ||
| ENTRYPOINT ["/test/test-omp-e2e.sh"] |
There was a problem hiding this comment.
P3: The new OMP E2E image isn't wired into the local runner: tests/docker/run-e2e.sh only accepts opencode|pi|all, so run-e2e.sh omp fails with "Unknown target" and all never runs it. Add an omp arm to run_target so developers can reproduce the CI e2e-omp job locally, matching the existing pi/opencode targets.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/docker/Dockerfile.omp, line 69:
<comment>The new OMP E2E image isn't wired into the local runner: `tests/docker/run-e2e.sh` only accepts `opencode|pi|all`, so `run-e2e.sh omp` fails with "Unknown target" and `all` never runs it. Add an `omp` arm to `run_target` so developers can reproduce the CI e2e-omp job locally, matching the existing pi/opencode targets.</comment>
<file context>
@@ -0,0 +1,69 @@
+RUN chmod +x /test/test-omp-e2e.sh
+
+WORKDIR /test/project
+ENTRYPOINT ["/test/test-omp-e2e.sh"]
</file context>
|
All nine findings from the latest cubic review were valid and are fixed in P1 — package-only OMP discovery produced an unusable binary P1 — active-profile sessions vanished under the XDG guard P2 — non-array P2 — prototype-member provider ids were rewritten P2 — non-executable file counted as an OMP install P2 — P3 — P3 — version floor not asserted P3 — whitespace-sensitive Verification after these changes:
|
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Full re-review done on the current branch head (1f6d417). Four of the five required changes are verified addressed with real regression coverage — the plain-Pi child-extension resolution (with the negative test proving pre-PR argv when OMP is absent), the path/config contract with the global write-scope guard, the explicit bidirectional provider mapping with round-trip tests, and the real-OMP CI job. Good work, genuinely. Two things remain before merge:
With those two, this merges. Thanks for the persistence through the review rounds. |
|
Thorough revision round — thank you. Re-reviewed against current master: 4 of the 5 requested changes are verified addressed, with real regression tests (the plain-Pi child-extension resolution fix in particular is exactly right, and its negative test fails on the pre-fix code as it should). Two remaining asks before merge:
Merge on green after those two. |
Co-authored-by: Wine Fox <fox@ling.plus>
Co-authored-by: Wine Fox <fox@ling.plus>
Co-authored-by: Wine Fox <fox@ling.plus>
Co-authored-by: Wine Fox <fox@ling.plus>
Co-authored-by: Wine Fox <fox@ling.plus>
Co-authored-by: Wine Fox <fox@ling.plus>
Co-authored-by: Wine Fox <fox@ling.plus>
Co-authored-by: Wine Fox <fox@ling.plus>
Co-authored-by: Wine Fox <fox@ling.plus>
Co-authored-by: Wine Fox <fox@ling.plus>
Co-authored-by: Wine Fox <fox@ling.plus>
Co-authored-by: Wine Fox <fox@ling.plus>
Co-authored-by: Wine Fox <fox@ling.plus>
Co-authored-by: Wine Fox <fox@ling.plus>
Co-authored-by: Wine Fox <fox@ling.plus>
Co-authored-by: Wine Fox <fox@ling.plus>
Co-authored-by: Wine Fox <fox@ling.plus>
Co-authored-by: Wine Fox <fox@ling.plus>
Co-authored-by: Wine Fox <fox@ling.plus>
Co-authored-by: Wine Fox <fox@ling.plus>
1f6d417 to
403265c
Compare
Co-authored-by: Wine Fox <fox@ling.plus>
|
Addressed the two remaining merge asks and the outstanding named-profile XDG finding.
New commits:
Verification on rebased head:
Branch force-pushed with lease and is mergeable; hosted checks are running on |
Co-authored-by: Wine Fox <fox@ling.plus>
Co-authored-by: Wine Fox <fox@ling.plus>
Co-authored-by: Wine Fox <fox@ling.plus>
|
Compared this branch directly with #297 and ported the parts that materially improve runtime behavior. Credit to @randomvariable / Naadir Jeewa for identifying and validating the strict OMP child-argv contract. The important gap was real: #256 still emitted Pi-only child flags ( Improvements now on #256:
I also fixed all seven valid findings currently reported against #297 rather than importing them:
Commits:
Verification on
|
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Co-authored-by: Wine Fox <fox@ling.plus>
Summary
Compatibility decisions
harness='pi'discriminator for session-scoped rowsomp models --jsonselectors so scoped and nested IDs are preservedVerification
omp plugin, launched OMP, and/ctx-statusreported Magic Context v0.33.0 with historian idleEnvironment note
The full Tauri Cargo suite could not link on this workstation because
pkg-config/DBus development metadata is absent; the failure occurs inlibdbus-sysbefore project code compiles.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Adds full Oh My Pi (OMP) support across the CLI, Pi-compatible extension, and dashboard, including setup, doctor, and OpenCode→OMP migration. Discovery is robust (profiles, XDG, executable checks), and CI now runs a real OMP Docker E2E that executes the production subagent argv.
New Features
ompharness adapter and end-to-end support forsetup --harness omp,doctor --harness omp, and--harness opencode|pi|omp.doctor migrate --from opencode --to ompwrites Pi/OMP JSONL into the active OMP sessions root.ompcanonicalization/resolve so IDs round‑trip with the shared config.omp models --json, scans OMP sessions (including profiles), and deduplicates across aliased roots.@cortexkit/pi-magic-context.Bug Fixes
Written for commit 18a5108. Summary will update on new commits.
Greptile Summary
Adds first-class Oh My Pi support across the CLI, Pi-compatible extension, dashboard discovery, migration, CI, and documentation.
Confidence Score: 5/5
The PR appears safe to merge, with only a non-blocking cache-list/detail mismatch remaining from the earlier multi-root discovery thread.
Multi-root deduplication now removes duplicate logical rows, but cache listing filters eventless copies before selection while detail lookup selects from all copies, so unusual divergent copies can still resolve to different files.
Files Needing Attention: packages/dashboard/src-tauri/src/pi_sessions.rs
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR CLI[Magic Context CLI] --> Setup[OMP setup and doctor] Setup --> OMP[Oh My Pi] OMP --> Extension[Pi-compatible extension] Extension --> Store[(Shared Magic Context database)] OMP --> Sessions[OMP JSONL sessions] Sessions --> Dashboard[Dashboard discovery] Store --> Dashboard OpenCode[OpenCode sessions] --> Migration[Session migration] Migration --> SessionsReviews (10): Last reviewed commit: "test(omp): 🧪 execute production subagen..." | Re-trigger Greptile