feat(omp): add Oh My Pi (OMP) support - #3707
Conversation
Add `--agent omp` to `rtk init` (with `-g`, `--uninstall`, `--show`) for the Oh My Pi coding agent (https://github.com/can1357/oh-my-pi). OMP loads the same `hooks/pi/rtk.ts` extension via its built-in legacy-pi-compat layer, which remaps the Pi package imports to OMP's bundled equivalent — so no separate OMP implementation is needed and the rewrite behavior stays byte-identical (mutualization). - Local scope: <project>/.omp/extensions/rtk.ts - Global scope: ~/.omp/agent/extensions/rtk.ts - `--uninstall` is three-way safe: missing → no-op, stock content → removed, modified RTK content → bail with manual-removal guidance - `--show` reports both scopes (installed / stock / modified / absent) Co-authored-by: makoMakoGo <makoMakoGo@users.noreply.github.com>
The extension is shared with OMP via its legacy-pi-compat layer. Add
guarded helpers that are strict no-ops on Pi: a persistent
"RTK disabled: <reason>" session status registered on session_start
(OMP wipes one-shot notify toasts on the initial render) and a
setLabel("RTK") UI label set before the version probe, so the
extension is identifiable in the session UI even when rtk is missing
or too old.
Co-authored-by: makoMakoGo <makoMakoGo@users.noreply.github.com>
|
|
There was a problem hiding this comment.
Reviewed at high effort, cross-checked against #1365 (this PR's predecessor thread) and the actual Pi/OMP SDKs upstream rather than just this PR's own claims. Inline comments on specific lines below; general notes here.
Comment coverage from #1365: all five of the original inline review comments are addressed in this PR's current shape.
Design questions raised in #1365, still open here:
- 3-way-safe uninstall not duplicated to
uninstall_pi(src/hooks/init.rs:3421, unchanged by this PR — outside this diff's hunks so I can't anchor a line comment on it).uninstall_ompgot the missing/exact-match/modified split;uninstall_piis still an unconditional delete for what's now the identical shared file. Since both install locations are equally user-editable global dirs, there's no principled reason to leave Pi unconditional — recommend makinguninstall_pi3-way safe too, in this PR or a fast follow-up. - Guarded additions to the shared file: This seems reasonable as long as the additions remain lightweight. We can always split the file later if it becomes too cluttered.
|
Addressed the review feedback in
Verification: Please re-review. |
# Conflicts: # hooks/README.md
|
Resolved the branch conflict by merging the latest GitHub now reports the PR as mergeable. Validation after the merge: Ready for re-review. |
|
Following up on the review-level design note: This is included in |
KuSh
left a comment
KuSh
left a comment
There was a problem hiding this comment.
Re-reviewed at high effort against 12a7cf0. Verified each of the three earlier threads against the new code rather than the commit messages — two are genuinely fixed and I've resolved them:
setLabel— call removed entirely, so the arity/runtime-detection problem can't fire on either runtime. Understood on not having a live Pi host; removing the API instead of guessing its contract makes that check moot. The replacement is correct on the same axis:ctx.ui.setStatus("rtk", text)matches Pi's documented two-argumentsetStatus(key, text).session_startrace —registerRtkUnavailableNotice(pi)now runs beforeawait pi.exec(...), and the retainedsessionContext+applyStatus()covers both orderings. Fail-opentry/catcharoundpi.onis right. Accepted thesetStatus-over-notifydecision given you checked OMP 18.0.4's source directly.
Also confirmed uninstall_pi is now three-way safe, and OMP's global path is corroborated by OMP's own SDK docs (agentDir: ~/.omp/agent).
The stock-hash thread stays open — details in that thread, but the short version is the allowlist is missing the revision that is stock today, and it's shipped: dev-0.46.1-rc.367 (published 2026-08-26, seven minutes after #2938 merged) contains hooks/pi/rtk.ts hashing to 3eb16108f51a29c2a62a453d5c97a6ea2da8aea1061da34c50fdcfaa32dc0ff7, which is not in the list.
New findings from this pass are inline below. One that isn't anchorable because the file isn't in the diff:
hooks/pi/README.md is now stale in two ways. hooks/README.md was updated to say the pi/ entry documents the file as "shared with Oh My Pi (OMP)", but hooks/pi/README.md itself never mentions OMP and still lists only the .pi/-scoped install paths. It also still states at line 36 that "Uninstall is idempotent", which is no longer true given the new hard-fail path (see the inline comment on uninstall_pi).
|
Follow-up fixes are pushed in
Validation: |
KuSh
left a comment
KuSh
left a comment
There was a problem hiding this comment.
Re-reviewed at high effort against d268e8f. Verified all five open threads against the code rather than the commit message — four are genuinely fixed and resolved, and I've reopened one that I closed too eagerly on my side.
I re-verified the hash work independently: the current hooks/pi/rtk.ts trims-and-hashes to 5e80e811…, matching the new first entry, 3eb16108… (the revision shipped in dev-0.46.1-rc.367) and 628308… are both present, and the never-shipped 2cbb2a7a… entry is gone. The new test is the invariant asked for. I also checked the PI_CODING_AGENT_DIR claim upstream rather than taking it on trust — OMP's own packages/utils/src/dirs.ts and docs/extension-loading.md confirm it, and that same doc independently confirms both install paths.
Reopened: the dry-run footer thread. My confirmation was wrong — I checked the two edge branches and not the main one. uninstall_omp's stock-content + dry_run branch prints [dry-run] would remove OMP extension: … and returns without print_dry_run_footer(), while uninstall_pi prints both. Details in that thread.
New findings from this pass are inline below. The two mediums are worth resolving before merge; the rest are judgement calls.
|
Follow-up fixes for the latest re-review are pushed in Addressed:
Validation is clean: Ready for re-review. |
KuSh
left a comment
KuSh
left a comment
There was a problem hiding this comment.
Re-reviewed at high effort against 0022488. All six previous threads verified fixed against the code and resolved — the new tests/omp_init_test.rs end-to-end coverage (driving the built binary and asserting on real stdout/stderr) is a genuine improvement over the unit tests that missed the earlier dry-run gaps.
I also independently re-verified the hash work: every revision of hooks/pi/rtk.ts reachable from develop, plus the current embedded file, is present with a matching digest. That table is complete.
Five new findings inline. One of them is a correction of my own earlier judgement: I previously waved off the line-ending concern as low-risk, reasoning about the global ~/.pi path. That was wrong — project-local is the default scope, which puts the file inside the user's repository. Details in the comment on is_known_stock_pi_plugin; I'd treat it as the blocker of this round.
Design suggestion that ties several of these together. Two of the remaining problems — the hard refusal on non-stock content at install, and the shared-path deletion at uninstall — are both cases where RTK detects something surprising and then either aborts or proceeds anyway, with no way for the user to decide. RTK already has the right mechanism for exactly this: PatchMode::Ask (src/hooks/init.rs:4258), which prompts [y/N], prints [dry-run] would prompt ... instead of blocking under --dry-run, and is skippable via --auto-patch.
Reusing that shape rather than inventing a second confirmation mechanism would resolve both:
- Install, content differs from stock — prompt
Overwrite? [y/N]instead ofbail!. This is the--forceescape hatch that's currently missing, and becauseread_linehits EOF on a non-TTY it fails closed, so scripted and hook-driven installs behave exactly as they do today. It also gives the CRLF case below a supported recovery path. - Uninstall, path shared via
PI_CODING_AGENT_DIR— prompt before deleting. Today it prints aneprintln!warning and deletes anyway, which gives the user the information at the moment it's already too late to act on.
No prompt is warranted for the ordinary upgrade path (older stock → current stock); that's what running rtk init asked for.
|
Addressed the latest re-review in In addition to the four inline fixes:
Validation remains clean: Ready for re-review. |
|
Verified this implementation against a real omp install — supporting merge. Path correctness confirmed. I checked omp's actual extension discovery:
I tested the shared extension end-to-end: the exact The shared-path protection (asking before overwriting/removing the Pi/OMP aliased file when For maintainers: this PR is more complete and correct than the other open omp attempts (#1827, #2166, #3151) — it targets the right directories, protects shared state, and is currently MERGEABLE. Recommend accepting this one. |
KuSh
left a comment
KuSh
left a comment
There was a problem hiding this comment.
Re-reviewed at high effort against 256d4a7. All four previous threads verified fixed against the code and resolved — the CRLF normalisation is applied consistently (comparison, hashing, and the invariant test), the dry-run halves of install and uninstall finally agree, the unreadable-file path degrades instead of erroring, and PiCompatibleAgent makes the branch exhaustive at compile time.
The prompt work is a better implementation than I suggested: extracting prompt_user_confirmation with an explicit IsTerminal check and routing the pre-existing settings-patch prompt through it improves that older call site too, and reusing --auto-patch / --no-patch avoids inventing a second flag vocabulary. The docs, including the CI guidance row, cover it properly.
A correction on my part, which is finding 1 below. I accepted the shared-path warning and prompt without checking the predicate that triggers them. global_extension_is_shared compares resolved paths only, and since resolve_omp_dir returns $PI_CODING_AGENT_DIR verbatim, it is unconditionally true for any Pi user who set that variable — whether or not OMP is installed, or even present. That turns a documented Pi workflow into a no-op in non-interactive use. I should have caught that when I signed off on the mitigation.
Findings 1 and 2 compound: the over-broad predicate decides there's something to confirm, and the non-interactive default then answers no and exits 0. Together they make rtk init -g --agent pi --uninstall silently do nothing in CI for a user who has never touched OMP.
Five findings inline.
|
|
||
| /// Check whether a global Pi-compatible extension path is shared by both | ||
| /// agents through `PI_CODING_AGENT_DIR`. | ||
| fn global_extension_is_shared(global: bool, path: &Path, agent: PiCompatibleAgent) -> Result<bool> { |
There was a problem hiding this comment.
This fires for Pi users who don't have OMP at all, and I should have caught it when I accepted the warn/prompt mitigation rather than only reviewing its wording.
The predicate is a pure path comparison, and resolve_omp_dir returns $PI_CODING_AGENT_DIR verbatim exactly as resolve_pi_dir does. So whenever that variable is set — a documented, long-standing Pi feature with nothing to do with OMP — global_extension_is_shared(true, path, Pi) is unconditionally true. Nothing checks that OMP is installed, configured, or even present on the machine.
Two consequences for a plain Pi user who relocated their agent dir:
rtk init -g --agent piwarns them about an agent they may never have heard of.rtk init -g --agent pi --uninstallnow prompts, and in any non-tty context (CI, provisioning,| tee) the prompt self-answersN— so the documented uninstall flow becomes a silent no-op that still exits 0 and leaves the file in place.
That's a regression in an existing, unrelated workflow introduced by an OMP feature, which makes it the most serious item in this round.
The fix is to require evidence OMP is actually in play before treating the alias as meaningful — the .omp directory existing, an OMP install having been performed, or an explicit opt-in — rather than inferring it from the presence of Pi's own environment variable. Path equality alone can't distinguish "both agents share this file" from "only Pi is here and OMP's default path happens to resolve to the same place".
| false | ||
| } | ||
| PatchMode::Ask => { | ||
| let prompt = format!("Overwrite the non-stock {} at {}?", name, path.display()); |
There was a problem hiding this comment.
The non-interactive default silently converts a requested action into a no-op that still reports success.
With the default PatchMode::Ask and no tty, prompt_user_confirmation returns false, so install prints <name> at <path> was not changed. and returns Ok(()) → exit 0. A provisioning script or CI job that runs rtk init --agent pi has no way to tell "installed" from "declined and did nothing"; both are exit 0, and the distinguishing text goes to stdout among other output.
I want to be fair here, because this behaviour follows from the prompt I asked for: it is documented, --auto-patch is the correct answer for automation, and the docs table explicitly points CI at it. So this isn't an oversight so much as an unresolved design question — but the answer matters, because the previous behaviour was a hard bail! with exit 1, i.e. loud. Silently succeeding while doing nothing is the one outcome automation can't defend against.
Suggest exiting non-zero when a requested install or uninstall did not happen — covering both the Ask-declined and --no-patch (PatchMode::Skip, line 3496) paths. A distinct exit code would be even better, letting callers separate "refused to touch protected content" from a genuine failure. Same reasoning applies to the shared-path decline in confirm_shared_global_uninstall, which is how finding 1 becomes silent.
| == normalize_pi_plugin_line_endings(PI_PLUGIN).trim_end() | ||
| } | ||
|
|
||
| fn looks_like_rtk_pi_plugin(content: &str) -> bool { |
There was a problem hiding this comment.
PI_PLUGIN_REWRITE_MARKER (line 36) is pi.exec("rtk", ["rewrite", but two revisions listed in KNOWN_PI_PLUGIN_HASHES call a locally-imported helper with no pi. prefix. Verified against the repository:
1da5793:100: const result = await exec("rtk", ["rewrite", cmd])
a7cab79:100: const result = await exec("rtk", ["rewrite", cmd])
9bd6e6f:34: const result = await pi.exec("rtk", ["rewrite", cmd], {
So a user who installed at either of the first two revisions and then edited the file is classified as is not RTK content; leaving it alone with exit 0, when it plainly is RTK content and should take the "contains RTK content that does not match the stock extension" path. They're told nothing needs removing while a modified RTK extension stays installed.
Worth being explicit that this is a narrow regression from my own earlier request: the previous contains("rtk rewrite") heuristic did match both revisions (three occurrences each, in their header comments). Moving to a call-site marker was still the right call — it fixed the much larger false-negative where a comment-stripped current extension went unrecognised — but the marker needs to cover the older shape too. Matching on exec("rtk", ["rewrite" (without the pi. prefix) covers both forms in one, since it's a substring of the newer call.
| name, | ||
| path.display() | ||
| ), | ||
| PatchMode::Auto => {} |
There was a problem hiding this comment.
The PatchMode::Auto => {} arm makes the destructive case the only one that previews nothing about the overwrite.
rtk init --agent pi --auto-patch --dry-run against a file with user edits prints:
[dry-run] would update Pi extension: .pi/extensions/rtk.ts
[dry-run] Nothing written.
which is textually identical to a routine stock-version bump. The Ask and Skip arms both explain what will happen; Auto — the mode that will actually destroy the user's content without asking — stays silent, so the preview is least informative precisely where it matters most.
Something like [dry-run] would overwrite non-stock {name}: {path} in that arm would make --auto-patch --dry-run usable as the safety check it's meant to be.
| Ok(()) | ||
| } | ||
|
|
||
| fn print_omp_extension_status(label: &str, path: &Path) -> Result<()> { |
There was a problem hiding this comment.
--show is a read-only status command but aborts on an unreadable or non-UTF-8 file: fs::read_to_string(path)? propagates, so the command exits 1 with Failed to read ... and the remaining scope line is never printed — the user gets a half-rendered report and no status for the other scope.
The uninstall path already solved this in this same PR: read_extension_for_uninstall reports the problem and degrades to leaving the file alone. Applying the same treatment here — an (unreadable) status line, then continuing to the next scope — would make the two consistent, and matters more for --show, whose entire purpose is to report state rather than change it.
5 participants
feat(omp): add Oh My Pi (OMP) support
Closes #591
Supersedes #1365
Adds
--agent omptortk init/--uninstall/--showfor the Oh My Pi coding agent.Per the 05-23 mutualization decision, OMP ships no separate extension file: the install places the exact
hooks/pi/rtk.tsthat Pi gets into OMP's extension directory, which OMP loads unchanged through its built-inlegacy-pi-compatlayer (import remap of@earendil-works/pi-coding-agentto the bundled@oh-my-pi/pi-coding-agent). Rewrite behavior is therefore byte-identical to Pi on both tested OMP versions (15.2.4 and 18.0.4).Usage
Install updates current or known historical stock content without prompting. For modified or unrelated content, the default is a confirmation prompt;
--auto-patchapproves and--no-patchleaves the file unchanged. CRLF and LF stock files are treated identically.Uninstall is 3-way safe (the global file lives in a user-global dir): missing → no-op; exact stock match → removed; modified RTK content → preserved and refused on a normal run (previewed during
--dry-run). Unreadable or unrelated content is left alone. WhenPI_CODING_AGENT_DIRmakes Pi and OMP share one global file, uninstall warns and prompts before removal;--auto-patchapproves it. Last line from a dry run against a pre-existing, hand-modified global install:Why an extension (and not OMP pre-hooks)
OMP's
--hookis an alias for an extension path in current OMP, and the extension is the actively maintained surface for tool interception. With mutualization there is no new runtime surface at all — OMP's compat layer executes the Pi extension directly (tool_call→ bash-only →rtk rewrite→ mutateevent.input.command).Behavior
tool_callis delegated tortk rewrite; empty or unchanged output → command passes through untouched.console.warnis kept; the extension additionally registers a persistent session status (ctx.ui.setStatus("rtk", "RTK disabled: …")onsession_start) where the runtime exposes one. This is OMP's visible-alert channel:ctx.ui.notifytoasts are wiped by OMP's initial render (verified in 18.0.4 sources), sonotifyis deliberately not used. All additions are guarded (optional-chaining + try/catch), so the shared file is behaviorally identical on Pi.Files
11 files, +1745/−143 (no
hooks/omp/):src/hooks/init.rs--showpaths, shared Pi/OMP path detection, stock-content classification, confirmation flow, and lifecycle testssrc/hooks/constants.rsOMP_DIR/OMP_LOCAL_DIRnext to the existing Pi constantssrc/main.rsAgentTarget::Omp+ dispatch, 3 testshooks/pi/rtk.tsREADME.md,docs/guide/getting-started/supported-agents.md,hooks/README.md,src/hooks/README.mdTest plan
cargo fmt --all -- --check,cargo clippy --all-targets: clean.omp): extension executes through the compat layer (-e); user-scope file auto-discovered and probed every session; project-scope.omp/extensions/rtk.tsauto-loaded and executed with no flags, no config, no prompt (marker binary fired). Full model-run e2e: a realomp -psession (qwen3.8-27b) shows the bash call asrtk git statusin the session JSONL after the extension rewrite, and the session completed.Rework of @makoMakoGo's #1365. Attribution: alvins82 +
Co-authored-by: makoMakoGo <makoMakoGo@users.noreply.github.com>(both original commits).