Skip to content

feat(omp): add extension-based rewrite integration for Oh My Pi - #1365

Closed
makoMakoGo wants to merge 5 commits into
rtk-ai:developfrom
makoMakoGo:feat/omp-extension-rewrite
Closed

feat(omp): add extension-based rewrite integration for Oh My Pi#1365
makoMakoGo wants to merge 5 commits into
rtk-ai:developfrom
makoMakoGo:feat/omp-extension-rewrite

Conversation

@makoMakoGo

@makoMakoGo makoMakoGo commented Apr 17, 2026

Copy link
Copy Markdown

Summary

Adds Oh My Pi (OMP) as a first-class RTK CLI integration via a TypeScript extension that intercepts tool_call events for the bash tool and delegates command rewrite decisions to rtk rewrite.

  • New init flag: rtk init --omp / rtk init -g --omp
  • Installs ./.omp/extensions/rtk.ts for project scope or ~/.omp/agent/extensions/rtk.ts for global scope
  • Keeps rewrite logic in the Rust registry; the OMP extension is a thin runtime adapter
  • Supports --show, --uninstall, --dry-run, project-scoped install, and global install
  • Refuses to overwrite pre-existing non-stock OMP extension files

Closes #591.

Review cleanup

This version was rebased onto current develop to keep the PR focused on OMP only:

  • Removed the previous docs noise and broad wording changes
  • Kept Hermes and existing agent documentation intact
  • Switched OMP from --agent omp to --omp because OMP is a CLI/runtime integration, closer to OpenCode/Gemini/Codex than IDE rules-file integrations

Test plan

  • cargo fmt --all -- --check
  • cargo clippy --all-targets
  • cargo test
  • bun --eval 'await import("./hooks/omp/rtk.ts")'
  • cargo run -- init --help
  • cargo run -- init --omp --dry-run
  • cargo run -- init --omp --show

@CLAassistant

CLAassistant commented Apr 17, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@makoMakoGo
makoMakoGo force-pushed the feat/omp-extension-rewrite branch from abc616a to 915ef53 Compare April 17, 2026 14:46
@makoMakoGo

Copy link
Copy Markdown
Author

I have used this pr for about 2 days on my omp.

@makoMakoGo

Copy link
Copy Markdown
Author

@aeppling @KuSh @FlorianBruniaux this is now rebased onto current master, the PR description matches the current fail-closed OMP behavior, and the targeted checks were rerun:

  • cargo test test_omp
  • cargo test test_validate_init_target_selection
  • cargo test test_npx_unknown_tool_passthrough
  • bun --eval await import("./hooks/omp/rtk.ts")

This PR has been open since 2026-04-17. Could one of you review it when you have time? Thanks.

@makoMakoGo

Copy link
Copy Markdown
Author

@pszymkowiak also pinging you here since you merge/review a lot of RTK work and this touches init/hook integration paths.

@tsubus

tsubus commented May 6, 2026

Copy link
Copy Markdown

this looks like a vibe coded mess, isn't this other one looking much better? https://github.com/beeemT/rtk/pull/2

@makoMakoGo

makoMakoGo commented May 6, 2026

Copy link
Copy Markdown
Author

this looks like a vibe coded mess, isn't this other one looking much better? beeemT#2

PR #2 is prompt-only, while OMP supports a cleaner command-rewrite path via tool_call. My personal opinion is that using event.input.command before the native BashTool runs, so RTK keeps transparent behavior without overriding the bash tool or relying on the model to remember rtk.

PR #2 describes Capability A(which I mentioned above), but the actual patch does not implement it; it ends up as Capability B (prompt-injection) only.

@KuSh KuSh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @makoMakoGo, thanks for your PR.

There's a lot of documentation changes that feels like nitpicks around terms (extensions/hooks/...). That seems uneeded and unwanted. Could you try to stay closer to what we already have?

Also OMP do have pre hooks, why not just use that? Why did you choose to use an extension?

@makoMakoGo

makoMakoGo commented May 14, 2026

Copy link
Copy Markdown
Author

Thanks for the review, @KuSh.

On the docs: Agreed — I'll strip the broad terminology changes and keep the diff to OMP-specific additions
only. The hook/plugin/extension wording updates aren't needed for this integration.

On extension vs pre hook: The goal is the same as a pre-execution rewrite: intercept the bash tool call before it runs and replace the command with the rtk rewrite result.

I chose the extension path because it's the actively maintained surface for tool interception in current OMP. Specifically:

  • --hook is treated as an alias for an extension path in current OMP
  • Extension modules are auto-discovered from .omp/extensions/ and ~/.omp/agent/extensions/
  • OMP's authoring docs recommend ExtensionAPI for new integrations
  • The hook subsystem's tool_call event can only block/allow — it cannot mutate tool input in-place, which is needed for command rewriting. In other words, the legacy hook result contract is block/allow-oriented and does not expose an explicit updated-input return shape for command rewriting.

This also aligns with RTK's existing transparent-rewrite integrations (OpenCode plugin, OpenClaw plugin) rather than the rules-file approach or hooks.

That said — if you'd prefer a pre-hook implementation, or would rather wait for another contributor's take, I'm happy to adjust or close. Just let me know which direction you'd prefer. 🤗

@KuSh

KuSh commented May 15, 2026

Copy link
Copy Markdown
Collaborator

On extension vs pre hook: The goal is the same as a pre-execution rewrite: intercept the bash tool call before it runs and replace the command with the rtk rewrite result.

I chose the extension path because it's the actively maintained surface for tool interception in current OMP. Specifically:

* `--hook` is treated as an alias for an extension path in current OMP

* Extension modules are auto-discovered from .omp/extensions/ and ~/.omp/agent/extensions/

* OMP's authoring docs recommend ExtensionAPI for new integrations

* The hook subsystem's tool_call event can only block/allow — it cannot mutate tool input in-place, which is needed for command rewriting. In other words, the legacy hook result contract is block/allow-oriented and does not expose an explicit updated-input return shape for command rewriting.

This also aligns with RTK's existing transparent-rewrite integrations (OpenCode plugin, OpenClaw plugin) rather than the rules-file approach or hooks.

That said — if you'd prefer a pre-hook implementation, or would rather wait for another contributor's take, I'm happy to adjust or close. Just let me know which direction you'd prefer. 🤗

No, I was just questioning the choice since, after a quick review of the OMP repo, I didn't find anything that prioritizes extensions over hooks. But after your explanation and a closer review of the documentation, extensions seem like the right choice.

So rebase, handle conflicts, and reduce documentation noise—the principles remain the same even if OMP chooses different wording.

@makoMakoGo
makoMakoGo force-pushed the feat/omp-extension-rewrite branch from 32043fc to f22686a Compare May 15, 2026 11:22
@rtk-release-bot rtk-release-bot Bot added the wrong-base PR targets master instead of develop label May 15, 2026
@rtk-release-bot

Copy link
Copy Markdown
Contributor

Automatic message from CI checks : It seems like this branch is targeting the wrong branch, any contribution should target develop branch.

See CONTRIBUTING.md for details.

@makoMakoGo
makoMakoGo force-pushed the feat/omp-extension-rewrite branch 4 times, most recently from 9a2fba8 to 2e4b84c Compare May 15, 2026 12:44
@makoMakoGo
makoMakoGo changed the base branch from master to develop May 15, 2026 12:44
@makoMakoGo
makoMakoGo marked this pull request as draft May 15, 2026 13:01
@makoMakoGo
makoMakoGo force-pushed the feat/omp-extension-rewrite branch 2 times, most recently from c13a98a to 6f39049 Compare May 15, 2026 13:28
@makoMakoGo

Copy link
Copy Markdown
Author

@KuSh updated.

Changed:

  • Kept docs to OMP-additive entries only; no broad terminology changes
  • Aligned OMP extension with RTK's fail-open convention used by Hermes/OpenCode etc: missing rtk or rewrite
    failures leave the original command unchanged

Verified:

  • cargo fmt --all -- --check
  • cargo clippy --all-targets
  • cargo test
  • bun --eval 'await import("./hooks/omp/rtk.ts")'
  • Local OMP simulation: tool_call rewrites supported bash commands through rtk
  • Manual fail-open check: hid the rtk binary; command passed through unchanged, no block

Ready for re-review when you have time.

@makoMakoGo
makoMakoGo marked this pull request as ready for review May 15, 2026 14:03
Comment thread hooks/omp/rtk.ts Outdated
Comment thread hooks/omp/rtk.ts Outdated
Comment thread hooks/omp/rtk.ts

const hasRtk = Boolean(Bun.which("rtk"));

pi.on("tool_call", async (event) => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick, why not just skip registering the tool_call hook if RTK isn't available?
How could we alert the user about that?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I moved the RTK availability check to extension setup: when rtk is missing, the extension now registers only a session_start warning via OMP's UI and skips the
tool_call handler; when rtk is available, it registers the rewrite hook as before.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially tried ctx.ui.notify(..., "warning"), but session_start notifications did not appear reliably during OMP startup, likely because the initial chat container is rebuilt afterward.

So I chose the persistent hook status line instead, as shown in the screenshots below.

image 屏幕截图 2026-05-19 052602

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to align this with the OpenCode plugin behavior: when rtk is missing, don't register the tool hook and surface a warning.

For OMP, it's a little werid.

 mode.init()
    -> UI start
    -> initHooksAndCustomTools()
    -> emit session_start
    -> extension: ctx.ui.notify(...)
    -> showWarning -->chatContainer

  main.ts 
    -> mode.renderInitialMessages()
    -> chatContainer.clear()
    -> warning clear (not show)

As a result, I cant see any warning expected.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does persistent hook status works flawlessly? It seems enough. I'm not sure to understand your latest message.

Comment thread src/hooks/init.rs Outdated
Comment thread src/hooks/init.rs
Comment thread src/main.rs Outdated
hooks::init::uninstall,
)?;
if omp {
hooks::init::uninstall_omp(global, ctx)?;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this should go into uninstall_init_dispatch as user could want to uninstall multiple things in one go

@KuSh

KuSh commented May 23, 2026

Copy link
Copy Markdown
Collaborator

@makoMakoGo in the meantime Pi support has been merged. Could you rebase your work on what has been done here. Perhaps some mutualization could be used.

@aeppling aeppling removed the wrong-base PR targets master instead of develop label May 23, 2026
@makoMakoGo

Copy link
Copy Markdown
Author

@makoMakoGo in the meantime Pi support has been merged. Could you rebase your work on what has been done here. Perhaps some mutualization could be used.

Thanks for the heads-up. I’ll take a look at the merged Pi support and coordinate with @thedavemarshall on the best path forward. 🫡

@makoMakoGo

makoMakoGo commented May 23, 2026

Copy link
Copy Markdown
Author

@KuSh

I rebased this on top of the merged Pi support (#1741) and tested the OMP path locally.

I confirmed that OMP v15.2.4 can load the merged Pi extension unchanged when the file is installed under OMP's native extension directories (.omp/extensions/rtk.ts or ~/.omp/agent/extensions/rtk.ts). The core runtime mechanism is the same: handle tool_call, narrow to bash, call rtk rewrite, and mutate event.input.command.

OMP provides legacy-pi-compat: it uniformly remaps the scopes of historical Pi internal packages — @mariozechner/, @earendil-works/, and @oh-my-pi/* — to OMP's own bundled @oh-my-pi/*.

Given that, I think this PR should not maintain a separate OMP runtime extension. 🤔 I'll update it to reuse the merged Pi extension implementation and keep only the OMP-specific install/init paths, docs, and tests.

The intended final shape is:

  • no separate OMP rewrite logic
  • install the Pi-compatible RTK extension into .omp/extensions/rtk.ts / ~/.omp/agent/extensions/rtk.ts
  • keep OMP-specific rtk init --omp, uninstall/show behavior, and coverage for those paths

Any other suggestions on the intended shape before I push the update ? I'm happy to do that.

@makoMakoGo

Copy link
Copy Markdown
Author

wait

I tested pure reuse of the merged Pi extension under OMP, including the missing-rtk path.

The normal rewrite mechanism is compatible and should stay aligned with Pi: listen to tool_call, handle only bash, delegate to rtk rewrite, and mutate event.input.command.

related to #1365 (comment)

However, reusing the Pi extension file unchanged has a bad OMP startup UX when rtk is missing from PATH.
In OMP, pi.exec("rtk", ["--version"]) fails during extension loading, and the error is not visibly surfaced in the UI. I also confirmed that ctx.ui.notify(...) during session_start is cleared by OMP's initial render, while ctx.ui.setStatus(...) remains visible.

I want do something like this:
屏幕截图 2026-05-24 015240

to do this, I need to use the ctx.ui.notify(...) approach. But it did not work reliably because the notification was emitted during startup and then disappeared when OMP rendered/cleared the initial chat container.

but I can only do this to alert the user about that:

image

the persistent status worked in my manual OMP testing and stayed visible after startup.

@KuSh

KuSh commented May 23, 2026

Copy link
Copy Markdown
Collaborator

Any other suggestions on the intended shape before I push the update ? I'm happy to do that.

Try to keep the spirit of the pi extension, even for install/uninstall, follow what have been documented. Rebase and resolve conflicts. If the pi extension doesn't work as is, we can duplicate it, but asking OMP maintainer about that usage which seems legit to me, could be a better option. Keep up the good work!

@makoMakoGo

Copy link
Copy Markdown
Author

Opened the issue for that notify bug, and it's fixed in oh-my-pi. Will move on, need some time though.

@makoMakoGo
makoMakoGo marked this pull request as draft May 27, 2026 11:38
Add Oh My Pi (OMP) as a CLI integration. OMP loads TypeScript
extensions that intercept tool_call events; RTK installs a dedicated
rtk.ts extension that delegates bash command rewrites to rtk rewrite.

- hooks/omp/rtk.ts: extension source (Bun runtime, tool_call handler)
- hooks/omp/README.md: hook-specific documentation
- src/hooks/constants.rs: OMP extension path constants
- src/hooks/init.rs: run_omp_mode, uninstall_omp, show_omp_config
- src/main.rs: --omp init flag and dispatch
- Docs: additive OMP entries in README, supported-agents, hooks/README

Supports both project-scoped (.omp/extensions/rtk.ts) and global
(~/.omp/agent/extensions/rtk.ts) installation. Fails closed when
rtk binary is not on PATH.
@makoMakoGo
makoMakoGo force-pushed the feat/omp-extension-rewrite branch from bb67262 to 32dd7e6 Compare May 27, 2026 11:46
@Crandel

Crandel commented Jun 30, 2026

Copy link
Copy Markdown

any news for this PR?

DavidHLP added a commit to DavidHLP/rtk that referenced this pull request Jul 22, 2026
Add two features that competing PRs (rtk-ai#1365, rtk-ai#2166) include and that
maintainer KuSh specifically requested in rtk-ai#1365 review:

- setLabel('RTK') — set extension label in OMP UI
- session_start + ctx.ui.setStatus — show persistent warning in OMP UI
  when rtk binary is missing or too old

Also extends ExtensionAPI interface with setLabel() and session_start
event support (optional ui field for backward compatibility).

Signed-off-by: DavidHLP <lysf15520112973@163.com>
@alvins82

alvins82 commented Aug 25, 2026

Copy link
Copy Markdown

Yes also looking for this @makoMakoGo @thedavemarshall

@alvins82

Copy link
Copy Markdown

PR reply draft (comment on #1365, voice: alvins82)

@makoMakoGo @KuSh — I picked this up and reworked it against current develop (29f9bb7). Everything below is verifiable on the branch; details of the OMP discovery findings are in the PR description.

Shape

Your 05-23 message ("the intended final shape is: no separate OMP rewrite logic; install the Pi-compatible RTK extension into .omp/extensions/rtk.ts / ~/.omp/agent/extensions/rtk.ts; keep OMP-specific init/uninstall/show behavior, docs, and tests") is now exactly the state of the PR:

  • The hooks/omp/ directory is gone. There is no OMP-specific runtime code anywhere.
  • rtk init --agent omp installs the same hooks/pi/rtk.ts that Pi gets (local: <project>/.omp/extensions/rtk.ts; -g: ~/.omp/agent/extensions/rtk.ts). OMP loads it unchanged through its legacy-pi-compat layer (import remap of @earendil-works/pi-coding-agent to the bundled @oh-my-pi/pi-coding-agent).
  • OMP-specific surface is Rust-side only: the --agent omp target, install/uninstall/--show wiring, additive docs, tests.

Your 05-23 "wait" finding is addressed

The missing-rtk startup UX you hit — silent failure of pi.exec("rtk", ["--version"]) during extension load, ctx.ui.notify wiped by OMP's initial render — is fixed inside the shared file, so both runtimes carry it. Four small guarded additions to hooks/pi/rtk.ts, everything else byte-identical to the merged Pi extension:

  1. Header note that the file is shared with OMP.
  2. On a failed version probe, the extension registers a persistent ctx.ui.setStatus("rtk", "RTK disabled: …") on session_start (in addition to the existing console.warn). This is the exact primitive from your second screenshot — verified persistent in OMP 18.0.4. It is wrapped in optional-chaining + try/catch, so on Pi the callback never fires and the code is inert.
  3. Guarded setLabel("RTK") — OMP surfaces extension labels in its session UI; Pi has no such method, so it's a no-op there.
  4. The session_start-based alert means OMP users see why rewrites are off (missing binary vs. too old), instead of the silent startup path you described.

ctx.ui.notify is deliberately not used: in the 18.0.4 sources (current release) the notify toast is still cleared on the initial render, so the code doesn't depend on that upstream fix at all.

Your 05-14 questions

Docs: agreed — the rework is strictly additive. One row per agent table, one OMP section in supported-agents.md (between Pi and OpenClaw), one row in the install-modes table. No terminology rewrites; +25 lines in that file total.

Extensions vs. pre-hooks: with mutualization this question resolves itself — OMP has no new surface to maintain at all. The OMP compat layer executes the Pi extension natively (tool_call → bash-only → rtk rewrite → mutate event.input.command), identical behavior to Pi, and --hook being an alias for an extension path in current OMP (per your 05-14 reply) means the extension is the pre-hook mechanism there.

05-22 review items — all five

  1. hooks/omp/rtk.ts:57 default-branch simplification — file is gone; the simplification lives on in the shared extension (default: skip).
  2. hooks/omp/rtk.ts:51 skip-when-unmodified — same: rtk rewrite no-op (empty stdout or unchanged command) → skip.
  3. hooks/omp/rtk.ts:80 "skip registering tool_call if RTK isn't available, how to alert?" — the availability check happens at load; when the probe fails the tool_call handler does nothing and the user gets the persistent session status + console warning (the session_start-alert approach you suggested, generalized to work on Pi too).
  4. init.rs:1808 split condition — gone: omp_extension_path_for_scope(global) computes the full path (local .omp/extensions/rtk.ts, global ~/.omp/agent/extensions/rtk.ts).
  5. init.rs:1824 write_if_changed — the OMP install path uses it: write_if_changed(path, PI_PLUGIN, "OMP extension", ctx), same as the Pi path.

OMP 18.0.4 discovery findings (corrected, source-verified)

I went through the OMP 18.0.4 sources (can1357/oh-my-pi, tag v18.0.4) and ran a full discovery matrix against the real binary, because a couple of our earlier readings of OMP's loading model were wrong. The facts that matter for this design:

  • Both .omp/extensions/ (project) and ~/.omp/agent/extensions/ (user) are auto-discovered and executed, unconditionally, per session. Your original local path was correct. v15.2.4 has the identical structure (builtin.ts:444), so the 05-23 local test holds for both versions.
  • There is no enable/trust gate in the extension load chain; per-file import errors are silently swallowed. This is why the missing-rtk alert matters — a broken extension file would otherwise fail silently.
  • OMP 18.0.4 ships no built-in rtk extension (full binary fingerprint grep; the only rtk strings are a third-party crate copyright notice) — nothing to conflict with or shadow.

Verification

  • cargo fmt --all -- --check, cargo clippy --all-targets: clean.
  • Full suite: 2645 lib tests (2637 passed / 8 ignored / 0 failed) + 8 integration suites, 0 failed. 9 new OMP unit tests + 3 in main.rs.
  • Extension harness (Pi-shaped fake API): 5/5 scenarios — normal rewrite, no-op passthrough, rtk missing, rtk too old, non-bash passthrough.
  • Dry runs of all four OMP install/uninstall paths (outputs in the PR description), including the real-world 3-way uninstall bail on a pre-existing global install.
  • Live OMP 18.0.4: extension executes through the compat layer (explicit -e); user-scope file auto-discovered and probed every session; project-scope .omp/extensions/rtk.ts auto-loaded and executed with no flags, no config, no prompt (marker binary fired). Full model-run e2e: a real omp -p session (qwen3.8-27b) shows the bash call as rtk git status in the session JSONL after the extension rewrite, and the session completed.

Two design questions for you, @KuSh

  1. Uninstall asymmetry. OMP uninstall is 3-way safe (missing → no-op; exact stock match → remove; modified RTK content → refuse with "remove the file manually"), while --agent pi is unconditional. OMP's file lives in a user-global dir, so I made it conservative there. Should we make Pi 3-way too, or keep the asymmetry?
  2. Guarded additions to the shared file. The four guarded deltas are inert on Pi and exist for OMP. I'd keep one shared file (the whole point of 05-23) rather than forking a Pi-pristine variant — but your call, since it touches the file a merged PR owns.

Where it stands

Rebased onto develop 29f9bb7 (current head at time of writing). 8 files, +551/−17. I've opened a new PR (#3707) from my fork with this shape — a fresh branch rather than a force-push onto yours, so you're welcome to close #1365 or take over either one. You're credited as co-author on both commits.

@KuSh KuSh self-assigned this Aug 26, 2026
@makoMakoGo

Copy link
Copy Markdown
Author

@alvins82 Thanks! I'm currently busy working toward my Ph.D. I'm glad you can continue supporting OMP RTK support and let me know. I'll close the PR. Best of luck!

@makoMakoGo makoMakoGo closed this Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for Oh-My-Pi

7 participants