feat(omp): bridge hook-guard denials and context through native hooks - #11
Open
andrebrait wants to merge 120 commits into
Open
andrebrait wants to merge 120 commits into
andrebrait wants to merge 120 commits into
Conversation
(cherry picked from commit ba49e71)
An unresolved bare specifier fell through to a ref namespaced id built from the whole raw string, so a package subpath import like next/image minted a different id than a bare import of next, even though both refer to the same package. package.json's own dependency node for that package is keyed off the bare name alone, so a subpath import could never land on it and produced no edge at all while the bare import of the same package resolved fine. The fallback now strips the specifier down to its package root first: the first segment for an ordinary package, the first two for a scoped one, matching how the dependency node itself is keyed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit be0eb7f)
A plain package subpath now lands on the same resolver target as its bare import. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit b703d31)
A scoped package subpath lands on the scope plus package only, never on a different package that happens to share the same scope. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 0218333)
A file that only imports a package subpath must resolve just like one that imports the bare package, using the exact package.json plus two file shape from the issue, instead of coming out fully disconnected from the framework it depends on. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 5fb7fc3)
Toward issue 2386: this hoists the reconstruction from graph.json's per node community attribute out of the "sidecar is missing" branch so it runs every time, with no behavior change yet since it is still only used when the sidecar produced nothing. The next commit compares it against the sidecar to also catch a sidecar that exists but is stale. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh (cherry picked from commit 0932abc)
Fixes issue 2386. The existing fallback only reconstructed communities from graph.json when the sidecar was missing entirely; a sidecar that exists but was written by an earlier clustering pass looked identical from the outside and kept winning, since update and watch never regenerate .graphify_analysis.json. This compares the node id set each side covers, cheap and already in memory, rather than the community ids themselves, since those can renumber run to run even for the same partition. A mismatch means the sidecar is stale: prefer the fresh reconstruction, recompute cohesion with score_all so wiki articles and the HTML export do not swap one silent degradation for a smaller one, reset gods_data so the existing self heal at the wiki export site recomputes it, and print a warning naming the exact command that refreshes the sidecar, since the whole point of this issue is that nothing currently signals the divergence. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh (cherry picked from commit d4eb7af)
Corrupts the analysis sidecar community node id set (the exact staleness signature the issue describes) and confirms export html still succeeds, still renders graph.html, and prints the new warning instead of silently exporting against the fossil. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh (cherry picked from commit 4518bae)
Confirms the wiki export path specifically, since it is the one that prints cohesion into article metadata: a stale, impossibly large cohesion value in the sidecar must not survive into the export once staleness is detected, proving score_all actually ran on the fresh reconstruction rather than the fossil value merely being ignored by coincidence. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh (cherry picked from commit 12c50b9)
Guards against a false positive: an up to date sidecar (the normal case immediately after extract or the clustering command) must not trip the new staleness warning or take the reconstruction path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh (cherry picked from commit 21342bd)
The node id set comparison missed a whole class of staleness: a merge, a split, or a single node moving from one community to another can leave the overall node set exactly unchanged while still describing a different clustering, so a stale sidecar that happened to still cover the same nodes kept winning. Compare each side's set of community blocks instead, which catches a partition change even when the flat node set stays identical, while still ignoring a pure id renumbering of the same partition per the existing Graphify-Labs#1667 guard. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 30a5955)
god_nodes ranks purely by graph degree, independent of the community partition, so clearing it to an empty list on the stale path and relying on the wiki subcommand's own recompute-if-empty fallback further down worked today only because wiki happens to be the only current reader of this value. Recompute it directly here instead, so a future reader that does not carry the same fallback cannot silently lose real data. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit b071d6e)
Prep for resolving a scoped call whose callee exists only as a qualified labeled fallback node: builds a label to node id index for every node whose label looks like a qualified method (Class colon colon method with parens), restricted to nodes with a real source file. Nothing reads it yet, so no behavior change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 1c1f133)
Only the Foo colon colon bar branch tags a fallback label, since it is the only one with a class name in source to build one from. The lookup only runs when the primary method index lookup already came back empty, and only accepts an exactly one candidate match, the same guard every other resolver in this module already applies. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit b5e066c)
The issue's own repro, an Unreal style UCLASS whose method declaration never survives header parsing, must still get a calls edge from a site that invokes it through the qualified name. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 3d4dec6)
Two different classes that both fail header parsing and happen to share a class name and a method name must not let a call resolve to either one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 9e26952)
Toward issue 1698: a call like MessageHelper.isRoomClosed() has exactly two navigation segments, so the existing fully qualified prefix capture (which only fires at three or more segments) never sees it, and the code deliberately never stamps member_receiver for this shape either, since that would defer the in file bare name lookup that already resolves the same file case correctly. This adds the new variable with no behavior change yet; capturing into it and threading it through to raw_calls lands in the next commits. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 91550dd)
Still toward issue 1698, still no behavior change: the variable is computed but nothing reads it yet. A capitalized two segment receiver (MessageHelper in MessageHelper.isRoomClosed()) names an object singleton or a class or companion member, statically unambiguous in source. Captured separately from member_receiver so the existing in file bare name lookup and its capitalized receiver deferral guard are untouched; threading this into raw_calls for a dedicated cross file resolver lands next. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 7728d99)
Still toward issue 1698. Still no observable behavior change on its own: nothing reads the new kotlin_object_receiver field yet, so existing edges are unaffected. The resolver that consumes it lands in the next commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit aed98d6)
Fixes issue 1698. Mirrors _resolve_kotlin_qualified_calls just above it and the same type_by_name plus methods_by_type shape _resolve_swift_member_calls uses for its own type qualified case: index every Kotlin sourced class or object by its bare name, resolve the receiver against that index, then look up the callee among that type's methods. Both steps bail on anything but exactly one candidate, the same god node guard every cross file resolver in this module already uses. Verified end to end: the issue's own cross file repro now produces an EXTRACTED calls edge, the same file control case is unaffected, and two objects sharing the same name in different packages correctly produce zero edges instead of an ambiguous guess. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit a6c89df)
Covers three cases: the new cross file resolution (Config.load() called from another file resolves to object Config), a same file control to prove the existing in file resolution is unaffected, and an ambiguous receiver defined in two packages that must yield no edge under the exactly one candidate guard. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit f22f17b)
…hify-Labs#1698 follow-up) Graphify-Labs#3598 resolves Kotlin object/class-qualified member calls across files. Adds two regression tests the PR omitted: - a `companion object` member call (`Service.create()`), pinning the Graphify-Labs#2565 enclosing-class attribution the resolver relies on; - an incremental-rebuild test routed through the real watch._rebuild_code, so it actually exercises the context builder's allow-list carrying _callable_class + method edges (a hand-fed resolution_context_nodes list would bypass exactly the path that silently regressed in sibling PRs). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e distinct (Graphify-Labs#3590 follow-up) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
(cherry picked from commit 4da2243)
(cherry picked from commit 3a9cd7b)
The PHP tree sitter grammar treats a span that is not PHP, such as an inline script block, as opaque markup, so any JavaScript declared there was entirely absent from the graph: functions, calls, and everything else in that region. extract_php now runs a second pass after the PHP extraction. Every part of the source that is not a script body is blanked to spaces, preserving newlines so line numbers stay accurate, and the masked source is parsed with the JS grammar the same way the Vue extractor already masks everything outside its own script block. The two node sets are merged onto the one file node, so a symbol declared in the script block looks like any other symbol in the file. A script tag with only a src attribute and no body masks to an empty region and contributes nothing, and a file with no script tag at all skips the second pass entirely. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 2114766)
Covers the issue's exact reproduction (two functions plus the call edge between them), that line numbers survive the masking pass, that a script tag with only a src attribute contributes nothing without crashing, and that a plain PHP file with no script tag is unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 2d408de)
A reviewer on this PR pointed out that when a JS symbol's id collides with an existing PHP node (same name, one file, resolved in favor of the PHP node), the merge still appended every JS edge unconditionally. An edge meant for the discarded JS symbol silently attached to the unrelated PHP node sharing its id instead of vanishing with it. The file node's own always on collision (both passes deliberately reuse its id) is excluded from this so an unrelated JS symbol's own edges sourced from the file node are unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit c96169b)
Covers a same name PHP/JS function pair in one file: exactly one surviving node, the JS call to the colliding name absent as a calls edge entirely rather than misattached, and the unrelated JS symbol's own contains edge from the shared file node still present. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit f59ce06)
(cherry picked from commit d9f00b0)
(cherry picked from commit ceb961a)
(cherry picked from commit 9d592e6)
…efixes (cherry picked from commit b8e2033)
…tests on extractor error (cherry picked from commit 29ed19f)
…raphify-Labs#3409/Graphify-Labs#3461 follow-up) Graphify-Labs#3461 fixes Graphify-Labs#3409 (PHP closures produce no nodes / inner calls lost). Adds the coverage its tests omitted: - the reporter's exact repro — a FILE-SCOPE argument-position closure — asserting the closure node exists AND its inner call resolves to the closure (not the file); - a no-duplicate-node assertion guarding the walk partition against double-walk; - a method-scope closure asserting its inner call attributes to the closure, not the enclosing method (pins the function_boundary_types behavior). Verified end-to-end: closure->handler() edges attribute to {closure#N} nodes with no duplicates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…te Python import resolution, package/module phantom-cycle fix, terraform list-secret redaction, idempotent wiki/Obsidian writes Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
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.
andrebrait
force-pushed
the
feat/omp-native-integration
branch
from
September 23, 2026 20:28
22caf5e to
ed70c8b
Compare
Claude Code wraps PreToolUse additionalContext in a system reminder that names the hook; the OMP bridge appended the bare guidance after the tool output, where it read as text the tool printed and agents discounted it as untrusted. Lead each qualifying result with one <system-reminder source="graphify"> block instead, the shape OMP's own per-tool TTSR reminders use, and keep the tool output unchanged after it.
OMP reads its agent skills from ~/.omp/agent/skills, not the legacy ~/.pi path the pi platform writes, so the installer had no OMP target even though 'omp' was already reserved in the CLI command set. Adds the omp platform reusing pi's skill bundle (OMP mirrors pi's agent layout), the pi-twin 'graphify omp' subcommand, and the project-scope groups; tests lock destinations, bundle reuse, install/uninstall, and uninstall_all pi parity.
…staller The merged omp-platform branch added a pi-twin 'graphify omp [install|uninstall]' dispatch branch, unreachable behind the bridge PR's 'graphify omp [path|install]' plugin branch. The subcommand surface belongs to the plugin installer: drop the duplicate, and carry the user-scope skill removal in uninstall_all (like amp/agents) since omp has no subcommand twin.
OMP 18.3.1 made path-utils' isInternalUrlPath module-private, so the
bridge failed to load ("Export named 'isInternalUrlPath' not found")
and no guard ran. Use a local scheme test instead: any scheme of two or
more characters followed by a slash, which keeps the single-slash alias
(local:/x) and leaves Windows drive paths alone.
… 18.3.1" This reverts commit bb628ac.
This branch has not been deployed
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.