feat(omp): bridge hook-guard denials and context through native hooks - #11
Open
andrebrait wants to merge 14 commits into
Open
andrebrait wants to merge 14 commits into
andrebrait wants to merge 14 commits into
Conversation
Claude PreToolUse additionalContext parity: run the guard in tool_call as before (strict deny path unchanged), but deliver the nudge by appending it to that call's tool_result content instead of accumulating a deduped context-transform message. Every qualifying call now carries its own guidance inline, persisted across compaction like any tool output; the before_agent_start reset now only clears pending deliveries.
Member
Author
|
@coderabbitai review |
There was a problem hiding this comment.
🟡 Changes recommended
The new OMP documentation claims guidance is “deduplicated,” but the implementation can append/concatenate per-call guidance without deduplication, so the docs should be corrected to match behavior.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds an “Oh My Pi (OMP)” native extension bridge so OMP can call Graphify’s existing hook-guard policy via an installed graphify CLI, translating denials into OMP blocks and attaching guidance to tool results. This expands Graphify’s installer surface area (new graphify omp … subcommand), ships a wheel-contained OMP package, and adds both Python + Bun-based tests plus CI coverage for the integration.
Changes:
- Ship a new OMP extension package under
graphify/omp/(TypeScript entrypoint +package.json) and include it in Python package data. - Add
graphify omp {install,path}CLI support (delegates installation toomp plugin install …) and suppress startup tip noise forompinvocations. - Add Python and Bun tests plus a dedicated CI job to validate the native hook bridging behavior end-to-end.
File summaries
| File | Description |
|---|---|
tests/test_omp_install.py |
Validates graphify omp path/install behavior and propagation of host install failures. |
tests/omp.test.ts |
End-to-end Bun tests exercising the OMP extension’s tool-call/tool-result bridge against a real installed graphify CLI. |
README.md |
Documents OMP install workflow, guard behavior, safety constraints, and uninstall steps. |
pyproject.toml |
Includes OMP extension files (omp/package.json, omp/index.ts) in wheel/sdist package data. |
graphify/omp/package.json |
Declares the OMP extension manifest and extension entrypoint. |
graphify/omp/index.ts |
Implements native OMP hook bridge: guarded subprocess invocation, cancellation, size/time limits, and tool-result context injection. |
graphify/omp/bun.lock |
Pins Bun dependency tree for the OMP extension tests and CI install reproducibility. |
graphify/install.py |
Adds omp to install command dispatch and implements graphify omp {install,path} delegation to omp plugin install. |
graphify/__main__.py |
Adds omp to silent commands and exposes omp help text in CLI usage. |
CHANGELOG.md |
Notes the OMP native guard feature in the changelog. |
.gitignore |
Ignores graphify/omp/node_modules/ and a temporary root /node_modules symlink used by Bun tests. |
.github/workflows/ci.yml |
Adds an omp-bridge job installing both uv + Bun and running tests/omp.test.ts. |
Review details
- Files reviewed: 10/12 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.
Comment on lines
+348
to
+350
| Denials become OMP `block`/`reason`; guidance becomes one deduplicated context | ||
| message, cleared for each new user run and session navigation. In-flight hooks | ||
| are cancelled on these boundaries. No graph is created or updated automatically. |
Copilot review finding: the README still described the replaced deduplicated context message. Describe the actual behavior: each qualifying call's guidance is appended to that call's persisted tool result.
Upstream OMP moved isReadableUrlPath from
@oh-my-pi/pi-coding-agent/tools/path-utils to @oh-my-pi/pi-tui/tools/read.
At the 18.2.2 devDependency floor path-utils still re-exported it, so CI
stayed green, but every OMP install past that point (18.2.6 is current)
throws SyntaxError: Export named 'isReadableUrlPath' not found before the
extension's api.on("tool_call", ...) ever registers, silently disabling
the hook-guard nudge.
Import it from @oh-my-pi/pi-tui/tools/read directly, raise the
@oh-my-pi/pi-coding-agent floor to ^18.2.6 (the first version pinned by
this fix where the export lives at its new home), and declare
@oh-my-pi/pi-tui ^18.2.6 as its own devDependency since the bridge now
imports from it directly rather than relying on it as an undeclared
transitive dependency of pi-coding-agent.
bun install --frozen-lockfile in the omp-bridge job pins an exact known-good OMP version, so CI can never observe a future upstream symbol move (this is exactly how the pi-tui export move shipped unnoticed: the lockfile pinned 18.2.2, where isReadableUrlPath still lived in pi-coding-agent). Add a step that installs whatever OMP publishes today and re-runs the suite against it, continue-on-error like the security-scan job's advisory checks, so an unrelated upstream release cannot block a merge while still surfacing a real break.
…oject guard _is_cwd_relative answered "no root and no drive" to decide whether a file_path/path value is cwd-anchored. A URL is rootless and driveless by that exact same test (https://x, myscheme://x, and a bare www.host/path all have no root/drive), so it short-circuited straight to "in project" and the containment check below never ran. - _normalize_hook_path: trims/de-quotes input and strips a leading file:// scheme to the local path it names (OMP's own pipeline resolves file:// to a local path, not an external URL -- it must still nudge). - _is_foreign_url_scheme: rejects any other whole-value scheme:// prefix. Deliberately does not enumerate a harness's internal-scheme vocabulary (local://, artifact://, ...) -- an unrecognized scheme is simply treated as not-a-local-source-file. - _has_embedded_url_scheme_segment: catches the same URL after an upstream host has already glued it onto an absolute prefix (<root>/local:/x), which looks exactly like a real file to the (unchanged) containment check. - _is_external_www_target: a bare www.host/path carries no :// at all, so it is handled separately, existence-gated to mirror OMP's own "an existing local path wins over URL" precedence. Containment, the extension allow-list, and staleness/strict-deny logic are untouched.
- isRemote gets a file:// carve-out: OMP's own resolveReadPathAsync
already reduces file:// to the real local path (its
strictExternalUrlRe deliberately excludes file), so routing it
through yields the correct absolute path instead of silently
dropping every file:// read. isInternalUrlPath/isReadableUrlPath
keep earning their place -- resolveReadPathAsync still pre-resolves
a rootless *foreign*-scheme value onto cwd into something that reads
as a real in-project file, so the bridge still must not hand it
those.
- resolveReadPath -> resolveReadPathAsync: same variant order/winner
semantics, non-blocking probes instead of statSync per candidate on
the tool_call hot path (read.ts itself already uses the async
variant).
- Drop the explicit { splitter: parseSearchPath } argument to
expandDelimitedPathEntries -- already its default.
Adds one bridge-level regression test: an in-project file:// target
still nudges, an out-of-project one still stays silent.
The OMP host loads graphify/omp/index.ts from its installed site-packages location, which ships no node_modules (see package.json's "files" list). The host provides @oh-my-pi/pi-coding-agent to legacy extensions but never @oh-my-pi/pi-tui, so the bridge's `isReadableUrlPath` import from @oh-my-pi/pi-tui/tools/read failed at runtime with `Cannot find package '@oh-my-pi/pi-tui'`, disabling the extension entirely. Vendor the two-line isReadableUrlPath predicate locally instead of importing it, drop the now-unused @oh-my-pi/pi-tui devDependency and regenerate bun.lock, and update the CI advisory step that used to bump both packages. Add a regression test asserting index.ts only imports @oh-my-pi/pi-coding-agent and node: builtins, so a future reintroduction of an unresolvable package fails the suite instead of only the host.
…er host copies Two @oh-my-pi/pi-coding-agent copies are reachable on this machine (18.1.17 and 18.2.6) and we cannot determine from outside the host which one it binds. Auditing every bridge import against the older 18.1.17 copy found resolveReadPathAsync missing -- it was only added in a later release as a non-blocking-probe performance optimization (per its own upstream docstring: "identical variant order and winner semantics, but non-blocking probes"), not a correctness fix. Revert to the synchronous resolveReadPath, present in both copies, so the extension loads regardless of which copy the host binds. A marginally faster extension that fails to load is worth nothing. Strengthen the import allow-list test with a version-floor check: every named import the bridge takes from @oh-my-pi/pi-coding-agent/tools/path-utils must be exported by the oldest host-provided copy. Parses the real import list out of index.ts rather than hardcoding it, so a future added symbol is checked automatically. Skips cleanly when that copy is not present on the machine running the suite.
…ct file _normalize_hook_path unconditionally reduced any file:// value to its path component, discarding the authority. file://evil.com/<in-project path> then classified identically to file:///<in-project path>: the same absolute path, silently treated as local. RFC 8089 / Node's url.fileURLToPath (ERR_INVALID_FILE_URL_HOST) say a file:// URL is local only when its authority is empty or localhost. Reduce only those two forms; leave any other authority intact so _is_foreign_url_scheme classifies it as the remote reference it is.
isRemote's file:// carve-out matched the scheme alone: any authority was treated as local, so file://evil.com/<in-project path> reached resolveReadPath, resolved to the real local path, and reached the guard as if it named a file in the project. Mirror Node's own url.fileURLToPath (ERR_INVALID_FILE_URL_HOST): only an empty or localhost authority is local. Any other authority now falls through to the generic scheme:// remote check, same as it would for https:// or any other foreign scheme.
The advisory job installed @oh-my-pi/pi-coding-agent@latest and ran the bridge suite against it on every CI run -- mutable, unpinned third-party code executing unconditionally. Replace the install-and- run with a download-only check: fetch the package with install scripts disabled, then statically compare index.ts's imported path-utils symbols against that file's declared exports as plain text. The downloaded package is never imported or executed, but the same upstream symbol move (isReadableUrlPath, pi-coding-agent -> pi-tui) that motivated this step still surfaces.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Companion PR (review-only)
Companion of Graphify-Labs#3506 (same head, same upstream base
v8=26b02b5, identical diff), opened on our fork so our review bots can access it — the upstream repository is read-only for us and its check runs sit inaction_required.Current state carries the parity delivery:
tool_callruns the guard andtool_resultappends the nudge to the call's persisted tool result (ClaudePreToolUseadditionalContext parity) — see the parity commit and the upstream thread for the review round and rationale. Review-only refs: do not merge here; landing happens upstream and viaintegration.