Update built-in Pi hook integration - #7008
Merged
austinywang merged 19 commits intoJun 29, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a managed Pi extension installer and uninstaller, splits the embedded Pi hook source into new Swift files, updates stop-notification deduping, refreshes Pi docs and localization strings, and expands Pi integration test coverage. ChangesPi hook extension parity
Estimated review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (21 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…lt-in-pi-hook-integration-for-curre # Conflicts: # .github/swift-file-length-budget.tsv
Contributor
Author
|
@codex review |
Rate Limit Exceeded
|
@austinywang I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,260 of the 240,000 allowed lines of code this month. Reviews resume on 1 July 2026 (in 4 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
To use Codex here, create a Codex account and connect to github. |
Contributor
Greptile SummaryThis PR upgrades the built-in Pi hook integration from the old
Confidence Score: 4/5Mergeable after the lifecycle concerns flagged in earlier threads are confirmed addressed; the new env allowlist and error-message cleanups look correct in the current diff. The env filter is now a genuine allowlist (ends with CLI/CMUXCLI+PiExtensionSourcePart1.swift and CLI/CMUXCLI+PiExtensionSourcePart2.swift contain the generated TypeScript that drives Pi session state, resume bindings, and notification routing — the areas where prior threads flagged the most significant concerns. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Pi as Pi Agent
participant Ext as cmux-session.ts (Generated Extension)
participant Cmux as cmux CLI
participant SW as cmux.swift (Stop Notification)
Pi->>Ext: session_start
Ext->>Cmux: hooks pi session-start (sync)
Ext->>Cmux: surface resume set --checkpoint-id sessionId
Ext->>Cmux: surface resume get (verify)
Pi->>Ext: before_agent_start
Ext->>Cmux: hooks pi prompt-submit + turn_id (sync)
Pi->>Ext: tool_execution_start
Ext->>Cmux: hooks feed PreToolUse (detached, non-blocking)
Pi->>Ext: tool_execution_end
Ext->>Cmux: hooks feed PostToolUse (detached, non-blocking)
Pi->>Ext: agent_end
Ext->>Cmux: hooks pi notification (sync)
alt "notificationRouted = true"
Ext->>Cmux: "hooks pi stop + cmux_notification_routed=true"
Cmux->>SW: "stopNotificationAlreadyRouted = true"
Note over SW: Native macOS notification suppressed
else "notificationRouted = false"
Ext->>Cmux: hooks pi stop (no flag)
Note over SW: Native macOS notification fires as fallback
end
Pi->>Ext: session_shutdown
alt "state.stopped = false (ungraceful exit)"
Ext->>Cmux: hooks pi stop + terminationReason (no notification flag)
Note over SW: Native macOS notification fires
end
Ext->>Cmux: surface resume clear --checkpoint-id sessionId
Note over Ext: sessionStates.delete only if clear succeeds
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Pi as Pi Agent
participant Ext as cmux-session.ts (Generated Extension)
participant Cmux as cmux CLI
participant SW as cmux.swift (Stop Notification)
Pi->>Ext: session_start
Ext->>Cmux: hooks pi session-start (sync)
Ext->>Cmux: surface resume set --checkpoint-id sessionId
Ext->>Cmux: surface resume get (verify)
Pi->>Ext: before_agent_start
Ext->>Cmux: hooks pi prompt-submit + turn_id (sync)
Pi->>Ext: tool_execution_start
Ext->>Cmux: hooks feed PreToolUse (detached, non-blocking)
Pi->>Ext: tool_execution_end
Ext->>Cmux: hooks feed PostToolUse (detached, non-blocking)
Pi->>Ext: agent_end
Ext->>Cmux: hooks pi notification (sync)
alt "notificationRouted = true"
Ext->>Cmux: "hooks pi stop + cmux_notification_routed=true"
Cmux->>SW: "stopNotificationAlreadyRouted = true"
Note over SW: Native macOS notification suppressed
else "notificationRouted = false"
Ext->>Cmux: hooks pi stop (no flag)
Note over SW: Native macOS notification fires as fallback
end
Pi->>Ext: session_shutdown
alt "state.stopped = false (ungraceful exit)"
Ext->>Cmux: hooks pi stop + terminationReason (no notification flag)
Note over SW: Native macOS notification fires
end
Ext->>Cmux: surface resume clear --checkpoint-id sessionId
Note over Ext: sessionStates.delete only if clear succeeds
Reviews (15): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Contributor
Greptile SummaryThis PR refactors the built-in Pi hook integration by extracting the generated TypeScript extension into its own Swift file (
Confidence Score: 3/5The env sanitizer and Feed turn-ID wiring in the new Pi extension file have correctness gaps that need resolution before this ships to users. The env-filter fallthrough is a real, demonstrable gap: any env var whose name doesn't match the deny-list regex (e.g. CLI/CMUXCLI+PiExtension.swift — specifically the
|
| Filename | Overview |
|---|---|
| CLI/CMUXCLI+PiExtension.swift | New 700-line file housing the generated Pi extension (TypeScript embedded in a Swift raw string literal) and the Swift install/uninstall methods. Contains env-filter fallthrough that leaks non-pattern-matched secrets, a generic id-field lookup that can corrupt turn IDs in Feed, and a heuristic-only launch-argv normalizer that silently drops script paths for non-standard Pi installations. |
| CLI/cmux.swift | Removes the old inline Pi extension (~220 lines) and adds the cmux_notification_routed deduplication guard to the stop-notification path; the guard logic looks correct. |
| Resources/Localizable.xcstrings | Adds 7 new CLI string keys for Pi hook install/uninstall messages with full translations across all supported locales (ar, bs, da, de, en, es, fr, it, ja, km, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, zh-Hant). |
| tests/test_pi_extension_install.py | Expands regression harness to cover resume binding flow, Feed telemetry events, notification routing, env sanitization, and turn-ID consistency. Uses time.sleep polling via wait_for_text for the detached Feed spawn — acceptable as test-only scaffolding. |
| cmux.xcodeproj/project.pbxproj | Adds CMUXCLI+PiExtension.swift to the Xcode project under PBXBuildFile and PBXFileReference sections; wiring looks correct. |
| docs/agent-hooks.md | Updates Pi's telemetry column from 'none' to 'tool_execution_start / tool_execution_end telemetry' and clarifies the Feed troubleshooting note. |
| docs/feed.md | Updates Pi row in the Feed integration table to reflect tool-execution telemetry instead of 'lifecycle only'. |
| .github/swift-file-length-budget.tsv | Reduces CLI/cmux.swift budget by ~210 lines (matching the removed Pi extension block) and adds CLI/CMUXCLI+PiExtension.swift at 700 lines. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Pi as Pi Agent
participant Ext as cmux-session.ts (Extension)
participant cmux as cmux CLI
Pi->>Ext: session_start
Ext->>cmux: hooks pi session-start
Ext->>cmux: surface resume get
cmux-->>Ext: "{resume_binding}"
Ext->>cmux: surface resume set (if stale)
cmux-->>Ext: "{ok}"
Ext->>cmux: surface resume get (verify)
Pi->>Ext: before_agent_start
Ext->>cmux: hooks pi prompt-submit (turn_id)
Pi->>Ext: tool_execution_start
Ext->>cmux: hooks feed --source pi --event PreToolUse (async)
Pi->>Ext: tool_execution_end
Ext->>cmux: hooks feed --source pi --event PostToolUse (async)
Pi->>Ext: agent_end
Ext->>cmux: "hooks pi stop (cmux_notification_routed=true)"
Ext->>cmux: hooks pi notification
Pi->>Ext: session_shutdown
alt not already stopped
Ext->>cmux: hooks pi stop
end
Ext->>cmux: surface resume clear
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Pi as Pi Agent
participant Ext as cmux-session.ts (Extension)
participant cmux as cmux CLI
Pi->>Ext: session_start
Ext->>cmux: hooks pi session-start
Ext->>cmux: surface resume get
cmux-->>Ext: "{resume_binding}"
Ext->>cmux: surface resume set (if stale)
cmux-->>Ext: "{ok}"
Ext->>cmux: surface resume get (verify)
Pi->>Ext: before_agent_start
Ext->>cmux: hooks pi prompt-submit (turn_id)
Pi->>Ext: tool_execution_start
Ext->>cmux: hooks feed --source pi --event PreToolUse (async)
Pi->>Ext: tool_execution_end
Ext->>cmux: hooks feed --source pi --event PostToolUse (async)
Pi->>Ext: agent_end
Ext->>cmux: "hooks pi stop (cmux_notification_routed=true)"
Ext->>cmux: hooks pi notification
Pi->>Ext: session_shutdown
alt not already stopped
Ext->>cmux: hooks pi stop
end
Ext->>cmux: surface resume clear
Reviews (3): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/CMUXCLI`+PiExtension.swift:
- Around line 345-350: The resume binding check in resumeBindingMatches() is too
permissive because it only compares kind and checkpoint_id, which allows
session_start to reuse a stale persisted Pi binding even when the command
context has changed. Update the session_start flow that decides whether to call
surface resume set so it also compares the stored command context fields (such
as cwd and sanitized argv) against the current session, or always rewrite the
resume_binding when a session starts. Keep resumeBindingMatches() and the
surrounding resume-state update logic as the key places to adjust so the
persisted binding is refreshed whenever the launch context differs.
- Around line 600-610: The read failure handling in CMUXCLI+PiExtension’s
file-loading path drops the underlying error, so update the catch block to
preserve diagnostics when throwing CLIError. In the String(contentsOf:encoding:)
error path, keep the existing localized “Failed to read %@” message but append
String(describing: error) to the wrapped CLIError rather than using
error.localizedDescription. Make this change in the same helper/method that
reads the URL so permission, encoding, and missing-file failures retain their
original details.
In `@docs/feed.md`:
- Line 87: The Feed docs prose is inconsistent with the updated Pi row: the
later paragraph still describes Pi as only providing lifecycle/session-restore
hooks. Update that paragraph to match the new Pi Feed support and reference the
same Pi extension behavior shown in the table so the documentation stays
consistent throughout.
In `@tests/test_pi_extension_install.py`:
- Around line 253-266: The resume verification in this test is too loose because
it only checks that `surface resume get/set/clear` appear somewhere in the logs.
Tighten the assertions around the `session_start` path by validating the exact
read-after-write sequence in the resume flow, or by asserting the exact number
of `surface resume get` calls so the post-`set` verification read is required.
Use the existing `wait_for_text` checks and the hook log expectations in
`test_pi_extension_install.py` to pin the ordered `get -> set -> get -> clear`
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0c9298ec-ce3b-4dc4-81bc-edf85e41046d
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (7)
CLI/CMUXCLI+PiExtension.swiftCLI/cmux.swiftResources/Localizable.xcstringscmux.xcodeproj/project.pbxprojdocs/agent-hooks.mddocs/feed.mdtests/test_pi_extension_install.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/CMUXCLI`+PiExtensionSourcePart2.swift:
- Around line 136-143: The pre-write `runCmux(["--json", "surface", "resume",
"get", ...target], cwd)` call in `CMUXCLI+PiExtensionSourcePart2` is redundant
because its `stdout` is never used and the subsequent `set` plus post-write
verification already handle success/failure. Remove this initial `get`/warning
block from the `session_start` path, and keep the existing post-write check
after the `set` so failures are still surfaced via `runCmux` and `warn`.
- Around line 132-134: The resume-binding paths ignore the
CMUX_PI_HOOKS_DISABLED kill switch, so both ensureResumeBinding and
clearResumeBinding should short-circuit the same way sendHook and sendFeed do.
Add the environment check near the start of ensureResumeBinding and
clearResumeBinding so session_start and session_shutdown stop spawning cmux
surface resume subprocesses and stop mutating surface bindings when the flag is
set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d5794112-bf9a-41ab-abcd-b20475d72818
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (5)
CLI/CMUXCLI+PiExtension.swiftCLI/CMUXCLI+PiExtensionSource.swiftCLI/CMUXCLI+PiExtensionSourcePart1.swiftCLI/CMUXCLI+PiExtensionSourcePart2.swiftcmux.xcodeproj/project.pbxproj
💤 Files with no reviewable changes (1)
- CLI/CMUXCLI+PiExtension.swift
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_pi_extension_install.py (1)
141-143: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the fake resume binding reflect the requested checkpoint.
The test now exercises both
pi-session-testandpi-session-interrupted, but the fakesurface resume setalways persistscheckpoint_id: "pi-session-test". That means the interrupted session’s read-after-write verification is testing a fake mismatch, and regressions that pass the wrong--checkpoint-idfor non-primary sessions can slip through. Parse the--checkpoint-idargument from$@and write that value intofake-surface-binding.json.As per path instructions, correctness-critical session/resume identity should come from explicit IDs, not a fallback or constant.
Proposed test harness tightening
*"surface resume set"*) - printf '{"resume_binding":{"kind":"pi","checkpoint_id":"pi-session-test","source":"agent-hook","command":"pi --session pi-session-test"}}\n' > "$CMUX_TEST_PI_BINDING_FILE" + checkpoint_id="" + previous="" + for token in "$@"; do + if [ "$previous" = "--checkpoint-id" ]; then + checkpoint_id="$token" + break + fi + previous="$token" + done + printf '{"resume_binding":{"kind":"pi","checkpoint_id":"%s","source":"agent-hook","command":"pi --session %s"}}\n' "$checkpoint_id" "$checkpoint_id" > "$CMUX_TEST_PI_BINDING_FILE" printf '{"ok":true}\n' ;;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_pi_extension_install.py` around lines 141 - 143, The fake surface resume binding in the test harness is hardcoded to a single checkpoint ID, which can hide incorrect checkpoint handling for non-primary sessions. Update the `surface resume set` case in the test setup to read the `--checkpoint-id` value from the command arguments (`$@`) and persist that exact value into `fake-surface-binding.json`, so the binding produced by the `surface resume set` path matches the requested session in the test.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/test_pi_extension_install.py`:
- Around line 141-143: The fake surface resume binding in the test harness is
hardcoded to a single checkpoint ID, which can hide incorrect checkpoint
handling for non-primary sessions. Update the `surface resume set` case in the
test setup to read the `--checkpoint-id` value from the command arguments (`$@`)
and persist that exact value into `fake-surface-binding.json`, so the binding
produced by the `surface resume set` path matches the requested session in the
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e200f358-78a6-446e-975d-44d4e73a846c
📒 Files selected for processing (2)
CLI/CMUXCLI+PiExtensionSourcePart2.swifttests/test_pi_extension_install.py
added 2 commits
June 26, 2026 19:06
…lt-in-pi-hook-integration-for-curre
added 2 commits
June 28, 2026 20:20
…lt-in-pi-hook-integration-for-curre # Conflicts: # .github/swift-file-length-budget.tsv # Resources/Localizable.xcstrings
…lt-in-pi-hook-integration-for-curre # Conflicts: # .github/swift-file-length-budget.tsv
…lt-in-pi-hook-integration-for-curre # Conflicts: # .github/swift-file-length-budget.tsv
…lt-in-pi-hook-integration-for-curre # Conflicts: # .github/swift-file-length-budget.tsv
austinywang
added a commit
that referenced
this pull request
Jun 29, 2026
…gents-don Resolve conflicts from main's #7008 (Pi hook integration rewrite): - CLI/cmux.swift: take main's side; the Pi extension now lives in CMUXCLI+PiExtension*.swift. Branch's OpenCode no-surface gate is preserved. - CLI/CMUXCLI+PiExtensionSourcePart2.swift: re-port the branch's gate change onto main's new Pi extension. sendHook/sendFeed now route when CMUX_SOCKET_PATH plus one scoped identity (surface/panel/workspace) is present; surface-scoped resume bindings stay gated on CMUX_SURFACE_ID. - tests/test_pi_extension_install.py: keep main's comprehensive surface-based coverage intact and add a self-contained no-surface phase (workspace-only) asserting hooks register with a socket, stay silent without one, and skip surface-scoped resume bindings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This branch was successfully 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
1 participant
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.
Fixes #5555
Summary
Validation
No local dev build was run per task instructions.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Updates the built-in Pi hook to the current
@earendil-works/pi-coding-agentfor better reliability, telemetry, and safety. Adds non-blocking tool telemetry, routed notifications with fallback, localized CLI output, and a disable flag; completes #5555.New Features
~/.pi/agent/extensions/cmux-session.tswith session lifecycle, per-turn IDs, and a non-blocking Feed bridge fortool_execution_start/tool_execution_end.argv/cwd, and clears the binding on shutdown.hooks pi notification, guards against duplicates, and preserves native fallback on failures.CMUX_PI_HOOKS_DISABLEDto fully disable the integration.Refactors
CLI/CMUXCLI+PiExtension*.swift, removes the old inline v1 fromcmux.swift, and localizes new CLI strings; updates docs./tmppaths, timeouts), and expands tests for Feed payloads, turn-id reuse, resume binding flows, env sanitization,argvnormalization, notification routing/fallback, the disable flag, and install safety checks.Written for commit 303f8c6. Summary will update on new commits.
Summary by CodeRabbit
tool_execution_start/tool_execution_end).--yes/-ybypass, and refusal to modify unexpected non-Pi content.