feat(omp): bridge hook-guard denials and context through native hooks - #11
Open
andrebrait wants to merge 7 commits into
Open
andrebrait wants to merge 7 commits into
andrebrait wants to merge 7 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.
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.