fix(desktop): preserve legacy agent hook notifications#3904
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughNotification flow now carries an optional Changes
Sequence Diagram(s)sequenceDiagram
participant AgentScript as Agent Script
participant HostSvc as Host-Service
participant DB as DB (terminalSessions)
participant Broadcaster as EventBus
participant Legacy as Legacy Listener
AgentScript->>HostSvc: POST v2 { eventType, terminalId, workspaceId }
HostSvc->>DB: find terminalSession by terminalId
DB-->>HostSvc: terminalSession (maybe null)
HostSvc->>Broadcaster: broadcastAgentLifecycle { eventType, terminalId, workspaceId (derived) }
AgentScript->>Legacy: POST localhost:/hook/complete (legacy v1)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Review rate limit: 6/8 reviews remaining, refill in 11 minutes and 37 seconds.Comment |
Greptile SummaryThis PR fixes a regression introduced in #3675 where removing the Confidence Score: 5/5Safe to merge; the fix is targeted, well-tested, and the intentional double-dispatch behavior is low-risk in practice. All findings are P2. The logic of the fix is correct, test coverage is thorough (fallback path, shell script assertions, negative assertion for the removed exit), and the change is a clear regression reversal. No files require special attention; the only uncertainty is the runtime behavior of the v1 /hook/complete handler when called from a pure-v2 terminal, which is outside the scope of this PR.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.sh | Removes early exit after successful v2 host-service dispatch; adds workspaceId to the v2 PAYLOAD; legacy v1 /hook/complete is now always called regardless of v2 outcome |
| packages/host-service/src/trpc/router/notifications/notifications.ts | Adds optional workspaceId to hookInput schema and uses it as a fallback when the terminal session row is not yet in the DB |
| packages/host-service/src/trpc/router/notifications/notifications.test.ts | Adds a test case for the workspaceId fallback when the terminal session row is absent; existing test for unknown terminals still passes |
| apps/desktop/src/main/lib/agent-setup/notify-hook.test.ts | Updates PAYLOAD assertion to include workspaceId; renames and extends test to assert that 2*) exit 0 is absent from the script |
Sequence Diagram
sequenceDiagram
participant Agent as CLI Agent (Claude/Codex)
participant Script as notify-hook.sh
participant HS as host-service tRPC (v2)
participant V1 as Electron /hook/complete (v1)
participant EB as EventBus / Renderer
Agent->>Script: lifecycle event (Stop/Start/etc.)
alt SUPERSET_HOST_AGENT_HOOK_URL is set (v2 env)
Script->>HS: POST {terminalId, workspaceId, eventType}
HS->>HS: lookup terminalSession.originWorkspaceId
Note over HS: fallback to input.workspaceId if row absent (NEW)
HS->>EB: broadcastAgentLifecycle(workspaceId, eventType)
Note over Script: No longer exits after v2 success (regression fix)
end
Script->>V1: GET /hook/complete?paneId&tabId&sessionId&eventType
Note over Script,V1: Always dispatched now (was skipped on v2 2xx before)
V1-->>Script: 200 OK
Comments Outside Diff (1)
-
apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.sh, line 104-134 (link)Potential duplicate notifications for v2 users
With the early exit removed, v2 terminals (those that have
SUPERSET_HOST_AGENT_HOOK_URLset) now always reach the legacy/hook/completecall, even when the v2 host-service dispatch succeeds. If the v1 Electron listener is still active and processes the event in a v2 environment, users could receive duplicate ringtones or sidebar indicators — one from the v2 event bus and one from the legacy endpoint.Worth confirming that the
/hook/completehandler silently ignores requests arriving with emptytabId/sessionIdvalues (likely absent in pure v2 terminals), so v2 users are unaffected in practice.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.sh Line: 104-134 Comment: **Potential duplicate notifications for v2 users** With the early exit removed, v2 terminals (those that have `SUPERSET_HOST_AGENT_HOOK_URL` set) now always reach the legacy `/hook/complete` call, even when the v2 host-service dispatch succeeds. If the v1 Electron listener is still active and processes the event in a v2 environment, users could receive duplicate ringtones or sidebar indicators — one from the v2 event bus and one from the legacy endpoint. Worth confirming that the `/hook/complete` handler silently ignores requests arriving with empty `tabId`/`sessionId` values (likely absent in pure v2 terminals), so v2 users are unaffected in practice. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.sh:104-134
**Potential duplicate notifications for v2 users**
With the early exit removed, v2 terminals (those that have `SUPERSET_HOST_AGENT_HOOK_URL` set) now always reach the legacy `/hook/complete` call, even when the v2 host-service dispatch succeeds. If the v1 Electron listener is still active and processes the event in a v2 environment, users could receive duplicate ringtones or sidebar indicators — one from the v2 event bus and one from the legacy endpoint.
Worth confirming that the `/hook/complete` handler silently ignores requests arriving with empty `tabId`/`sessionId` values (likely absent in pure v2 terminals), so v2 users are unaffected in practice.
Reviews (1): Last reviewed commit: "fix(desktop): preserve legacy agent hook..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/agent-setup/notify-hook.test.ts (1)
28-37: ⚡ Quick winStrengthen this test to verify behavior instead of one removed string literal.
expect(script).not.toContain("2*) exit 0")can still pass if a different early-exit pattern is introduced. Prefer asserting that the legacy curl call appears after the host-service curl block.Suggested test hardening
- expect(script).not.toContain("2*) exit 0"); + const hostHookIdx = script.indexOf( + 'curl -sX POST "$SUPERSET_HOST_AGENT_HOOK_URL"', + ); + const legacyHookIdx = script.indexOf( + 'curl -sG "http://127.0.0.1:${SUPERSET_PORT:-{{DEFAULT_PORT}}}/hook/complete"', + ); + expect(hostHookIdx).toBeGreaterThanOrEqual(0); + expect(legacyHookIdx).toBeGreaterThan(hostHookIdx);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/notify-hook.test.ts` around lines 28 - 37, The test in notify-hook.test.ts is brittle because it only checks for absence of a specific string; update the assertion to verify ordering instead: read the generated script (variable script, from notify-hook.template.sh) and locate the host-service curl block (e.g. the "$SUPERSET_HOST_AGENT_HOOK_URL" curl) and the legacy curl call (the "$SUPERSET_HOST_AGENT_HOOK_URL" legacy curl pattern or legacy URL variable), then assert that the index/position of the host-service block is less than the index of the legacy curl call (e.g. expect(script.indexOf(hostServiceSnippet)).toBeLessThan(script.indexOf(legacySnippet))). This ensures the legacy curl appears after the v2 host-service hook and is robust to changes in exact early-exit strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/main/lib/agent-setup/notify-hook.test.ts`:
- Around line 28-37: The test in notify-hook.test.ts is brittle because it only
checks for absence of a specific string; update the assertion to verify ordering
instead: read the generated script (variable script, from
notify-hook.template.sh) and locate the host-service curl block (e.g. the
"$SUPERSET_HOST_AGENT_HOOK_URL" curl) and the legacy curl call (the
"$SUPERSET_HOST_AGENT_HOOK_URL" legacy curl pattern or legacy URL variable),
then assert that the index/position of the host-service block is less than the
index of the legacy curl call (e.g.
expect(script.indexOf(hostServiceSnippet)).toBeLessThan(script.indexOf(legacySnippet))).
This ensures the legacy curl appears after the v2 host-service hook and is
robust to changes in exact early-exit strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f028120-8673-4652-944c-6b2748547146
📒 Files selected for processing (4)
apps/desktop/src/main/lib/agent-setup/notify-hook.test.tsapps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.shpackages/host-service/src/trpc/router/notifications/notifications.test.tspackages/host-service/src/trpc/router/notifications/notifications.ts
janghoosa
force-pushed
the
janghoosa/create-workspace
branch
from
dba3e04 to
76a05f8
Compare
1 participant
Description
While the v2 workspace flow is still rolling out, some users may continue using the v1/legacy desktop flow. In that setup, agent lifecycle hooks should still reach the existing desktop hook listener so terminal status updates and completion signals continue to work.
Previously, when
SUPERSET_HOST_AGENT_HOOK_URLwas present, the notify hook script returned after a successful host-service dispatch. That meant the legacy/hook/completelistener did not receive the same lifecycle event.This PR keeps the host-service notification path intact, but also preserves dispatch to the existing desktop hook listener.
It also includes
workspaceIdin the host-service hook payload so lifecycle events are not dropped if the terminal session row is not visible yet.Related Issues
N/A
Type of Change
Testing
bun test apps/desktop/src/main/lib/agent-setup/notify-hook.test.ts packages/host-service/src/trpc/router/notifications/notifications.test.ts apps/desktop/src/ renderer/routes/_authenticated/components/V2NotificationController/lib/statusTransitions.test.ts apps/desktop/src/renderer/routes/_authenticated/components/ V2NotificationController/lib/resolveV2NotificationTarget.test.ts apps/desktop/src/renderer/stores/v2-notifications/store.test.tsbun run lintScreenshots (if applicable)
N/A
Additional Notes
This appears to be a regression from
e07aef637/ #3675 (feat(desktop): play v2 notification hooks client-side), which added the v2 host-service hook path and exited early after a successful host-service dispatch. That prevented the existing desktop/hook/completelistener from receiving the same lifecycle event.The regression is present from
desktop-v1.6.0onward.v2 is not yet comfortable for my workflow because worktree creation and PR number visibility are still being improved there, so I am currently using the v1 flow and hit this hook regression. Once this PR is merged, I would like to keep using v2 more and contribute follow-up improvements for the remaining rough edges I run into.
Summary by cubic
Preserves legacy v1 desktop agent notifications alongside the v2
host-servicehook so terminal status and completion keep working. Also includesworkspaceIdin the v2 payload and falls back to it to avoid dropped events when the terminal session isn’t loaded yet./hook/completewhen legacy pane/tab ids exist; for pure v2 terminals (no legacy ids), it skips the legacy call to prevent duplicates.host-servicehook acceptsworkspaceIdand falls back to it when the terminal session isn’t visible; broadcasts are no longer ignored.workspaceIdfallback.Written for commit 5f843f7. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Tests
Chores