Fix plugin agent hibernation registration - #6991
Closed
austinywang wants to merge 18 commits into
Closed
austinywang wants to merge 18 commits into
austinywang wants to merge 18 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 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 |
Contributor
Greptile SummaryFixes plugin agent hibernation registration (#5502) by expanding the hook-dispatch guards across OpenCode, Pi, and Amp so that session hooks fire whenever a socket (
Confidence Score: 5/5Safe to merge — all three plugin hook guards are now consistent, the Pi return-value fix is correctly threaded through callers, and the double-layer protection (sendHook gate + surfaceTargetArgs gate) keeps surface resume bindings from firing in no-socket and no-surface scenarios. The guard logic is symmetric across all three plugins and matches the stated invariant (socket ∧ routing-id). The Pi sendHook return-value change from true to false on skip is safe: ensureResumeBinding has its own CMUX_PI_HOOKS_DISABLED and surfaceTargetArgs guards, so the observable behavior for the disabled case is unchanged; the socket/no-surface paths are now guarded at both layers. The new regression tests cover surface-no-socket, workspace-no-socket, and workspace-with-socket scenarios end-to-end. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[sendHook called] --> B{HOOKS_DISABLED=1?}
B -- yes --> SKIP[return false / void\nno cmux subprocess]
B -- no --> C{CMUX_SOCKET_PATH\nor CMUX_SOCKET set?}
C -- no --> SKIP
C -- yes --> D{CMUX_SURFACE_ID\nor CMUX_WORKSPACE_ID set?}
D -- no --> SKIP
D -- yes --> E{sessionId present?}
E -- no --> SKIP
E -- yes --> F[spawn cmux hooks subcommand\nreturn true / void]
F --> G{caller: ok && sessionId?}
G -- Pi session-start --> H{surfaceTargetArgs:\nsocket present?}
H -- no --> NOSURFACE[skip resume binding]
H -- yes --> I{CMUX_SURFACE_ID set?}
I -- no --> NOSURFACE
I -- yes --> J[cmux surface resume set]
subgraph sendFeed [sendFeed — always surface-scoped]
SF1{socket present?} -- no --> SFskip[return]
SF1 -- yes --> SF2{CMUX_SURFACE_ID set?}
SF2 -- no --> SFskip
SF2 -- yes --> SF3[spawn cmux hooks feed]
end
%%{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"}}}%%
flowchart TD
A[sendHook called] --> B{HOOKS_DISABLED=1?}
B -- yes --> SKIP[return false / void\nno cmux subprocess]
B -- no --> C{CMUX_SOCKET_PATH\nor CMUX_SOCKET set?}
C -- no --> SKIP
C -- yes --> D{CMUX_SURFACE_ID\nor CMUX_WORKSPACE_ID set?}
D -- no --> SKIP
D -- yes --> E{sessionId present?}
E -- no --> SKIP
E -- yes --> F[spawn cmux hooks subcommand\nreturn true / void]
F --> G{caller: ok && sessionId?}
G -- Pi session-start --> H{surfaceTargetArgs:\nsocket present?}
H -- no --> NOSURFACE[skip resume binding]
H -- yes --> I{CMUX_SURFACE_ID set?}
I -- no --> NOSURFACE
I -- yes --> J[cmux surface resume set]
subgraph sendFeed [sendFeed — always surface-scoped]
SF1{socket present?} -- no --> SFskip[return]
SF1 -- yes --> SF2{CMUX_SURFACE_ID set?}
SF2 -- no --> SFskip
SF2 -- yes --> SF3[spawn cmux hooks feed]
end
Reviews (16): 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}
…rnation-opencode-plugin-agents-don
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`+AmpExtension.swift:
- Line 117: The no-surface fallback gate in CMUXCLI+AmpExtension currently
allows spawning hooks with only partial CMUX context, which can route sessions
without a reliable target. Update the guard around the fallback so it requires
CMUX_SOCKET_PATH together with a scoped identity such as CMUX_WORKSPACE_ID or
CMUX_PANEL_ID before proceeding, and keep the fail-closed behavior when the
socket is missing. Use the existing CMUXCLI+AmpExtension fallback check to
locate the change.
🪄 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: 6b865cd3-9b63-4799-b3a2-fc34031f2cac
📒 Files selected for processing (6)
.github/workflows/ci.ymlCLI/CMUXCLI+AmpExtension.swiftCLI/cmux.swifttests/test_amp_extension_install.pytests/test_opencode_plugin_install.pytests/test_pi_extension_install.py
…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>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 5-10: The sendHook function currently returns true on skip paths,
which incorrectly signals success even when no hook was sent. Update sendHook in
CMUXCLI+PiExtensionSourcePart2 to return false for the early-exit cases (hooks
disabled, missing socket/routing keys, missing sessionId), so callers can
distinguish dropped hooks from successful sends. This will prevent follow-up
logic like ensureResumeBinding and cmux_notification_routed from running when
nothing was actually routed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e9634ac-b33f-4fe7-9e1f-3797b9708709
📒 Files selected for processing (7)
.github/workflows/ci.ymlCLI/CMUXCLI+AmpExtension.swiftCLI/CMUXCLI+PiExtensionSourcePart2.swiftCLI/cmux.swifttests/test_amp_extension_install.pytests/test_opencode_plugin_install.pytests/test_pi_extension_install.py
💤 Files with no reviewable changes (3)
- tests/test_pi_extension_install.py
- tests/test_opencode_plugin_install.py
- tests/test_amp_extension_install.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
CodeRabbit (PR #6991): sendHook returned true on every early-exit skip, so a session_start skipped for a missing socket still ran ensureResumeBinding and emitted a surface-scoped "surface resume set" without a socket, and a dropped notification could mark cmux_notification_routed. Return false on skip so the ok-gated resume binding and notification-routed flag only apply when the hook was actually routed. Add a Pi regression: with a surface but no socket, assert the extension does not set a surface resume binding and routes no session hook. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
autoreview (PR #6991): session_shutdown's clearResumeBinding only checked for a surface target, so a process with CMUX_SURFACE_ID but no CMUX_SOCKET_PATH still ran `cmux --json surface resume clear ...`, which could route to the default or wrong tagged cmux socket. Gate the shared surfaceTargetArgs helper on CMUX_SOCKET_PATH so both ensureResumeBinding and clearResumeBinding stay inert without a socket — consistent with sendHook/sendFeed. Strengthen the surface-but-no-socket regression to assert the extension stays completely silent (no session hook, no surface resume set or clear). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 6-7: The routing guard in CMUXCLI+PiExtensionSourcePart2 is now
allowing CMUX_PANEL_ID, but the CLI target resolution in cmux.swift still only
falls back for CMUX_SURFACE_ID and CMUX_WORKSPACE_ID, creating a false-success
path for panel-only setups. Update the hook target resolution used by runCmux()
/ hooks fallback so it also recognizes CMUX_PANEL_ID, or remove CMUX_PANEL_ID
from the gating predicates in sendHook()/sendFeed() until that end-to-end
contract exists. Keep the guard and the CLI resolution logic aligned around the
same target identifiers.
In `@tests/test_pi_extension_install.py`:
- Around line 299-303: The “no socket” test envs are still inheriting
CMUX_SOCKET_PATH from check_env, so they may accidentally follow the
socket-present path. In the test setup around the surface_no_socket_env and
workspace_no_socket_env branches, explicitly remove CMUX_SOCKET_PATH from each
copied env before launching Bun, so the intended skip-path behavior is isolated
from ambient process state.
🪄 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: 463d2ac7-62cd-4cc6-bde1-24c221c42a85
📒 Files selected for processing (2)
CLI/CMUXCLI+PiExtensionSourcePart2.swifttests/test_pi_extension_install.py
…no-socket tests CodeRabbit (PR #6991): - The CLI hook routing (cmux.swift `command == "hooks"`) no-ops unless CMUX_SURFACE_ID or CMUX_WORKSPACE_ID resolves a target; it never honors CMUX_PANEL_ID. Drop CMUX_PANEL_ID from the Amp/OpenCode/Pi sendHook/sendFeed guards so a panel-only process does not observe a false-success no-op route. - The Pi no-socket regressions copied os.environ via check_env, so an ambient CMUX_SOCKET_PATH would silently exercise the socket-present path. Explicitly pop CMUX_SOCKET_PATH in both no-socket envs so the skip path is deterministic. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
autoreview (PR #6991): the Amp and OpenCode no-socket regressions build check_env from os.environ and only removed surface/panel ids, so an ambient CMUX_SOCKET_PATH (e.g. running the suite inside a cmux pane) made the no-socket phase exercise the socket-present path and fail the "invoked cmux without CMUX_SOCKET_PATH" assertion. Pop CMUX_SOCKET_PATH before the no-socket run, matching the Pi test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
autoreview (PR #6991): sendFeed was broadened to workspace-only contexts, but runFeedHook in cmux.swift no-ops (`print("{}")`) unless CMUX_SURFACE_ID is set. A no-surface hibernated Pi agent would then spawn a cmux subprocess per tool start/end that records nothing — wasteful on a hot path. Require socket + surface for sendFeed (session hooks stay workspace-routable; only feed needs a surface). Assert in the no-surface regression that feed telemetry does not fire without a surface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ract autoreview (PR #6991): the guards required CMUX_SOCKET_PATH specifically, but CLISocketEnvironment.socketPath resolves CMUX_SOCKET_PATH ?? CMUX_SOCKET and the Pi env allowlist preserves CMUX_SOCKET. An agent in a legacy environment with only CMUX_SOCKET would silently drop all Amp/OpenCode/Pi session hooks and Pi resume bindings. Accept (CMUX_SOCKET_PATH || CMUX_SOCKET) in sendHook, sendFeed, and surfaceTargetArgs so the guard matches what the CLI can actually route. Also pop legacy CMUX_SOCKET alongside CMUX_SOCKET_PATH in the no-socket test phases so the regressions stay deterministic regardless of the caller env. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rnation-opencode-plugin-agents-don
…rnation-opencode-plugin-agents-don
Contributor
Author
|
Addressing CodeRabbit's docstring coverage warning: this PR only changes generated plugin hook snippets, focused CI wiring, and regression tests for hook routing. It does not introduce a new public API/docstring surface, and the CodeRabbit status check is passing, so no docstring changes are needed for this PR. |
…rnation-opencode-plugin-agents-don
…rnation-opencode-plugin-agents-don
…rnation-opencode-plugin-agents-don
lawrencecchen
added
the
stale-revisit
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
3 participants
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 #5502\n\n## Summary\n- add no-surface plugin hook regressions for OpenCode, Pi, and Amp\n- allow plugin session hooks to run with workspace/socket cmux context when CMUX_SURFACE_ID is absent\n- wire OpenCode and Amp plugin install tests into the focused CLI regression lane\n\n## Validation\n- python3 -m py_compile tests/test_opencode_plugin_install.py tests/test_pi_extension_install.py tests/test_amp_extension_install.py\n- git diff --check HEAD~2..HEAD\n- reproduced red state before the fix with existing CLI: OpenCode/Pi no-surface plugin tests failed before production changes; Amp skipped locally because node is unavailable\n\nNo local dev build or xcodebuild run per request.
Summary by cubic
Fixes #5502 by correcting plugin agent hibernation across OpenCode, Pi, and Amp. Hooks now run only with a socket (
CMUX_SOCKET_PATHorCMUX_SOCKET) plus a surface or workspace; Pi feed telemetry stays surface‑scoped.CMUX_SURFACE_IDorCMUX_WORKSPACE_ID(dropCMUX_PANEL_ID). Surface resume bindings still requireCMUX_SURFACE_ID. Pi:sendHookreturns false on skip;surfaceTargetArgsrequires a socket;sendFeedrequires socket + surface. Accept legacyCMUX_SOCKET.CMUX_SOCKET_PATH/CMUX_SOCKETand assert silence. Pi adds a surface‑but‑no‑socket regression (no hooks, no resume set/clear) and a workspace‑only with socket phase (session hooks OK, resume/feed skipped). CI runs OpenCode → Pi → Amp.Written for commit 6844484. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests