feat(desktop): agent icon in v2 terminal pane via lifecycle hooks#4232
Merged
Conversation
…cycle hooks Wires the existing v2 agent-lifecycle hook through to a per-terminal binding store so the pane's primary icon swaps to the running agent's logo (Claude, Codex, Gemini, …) the moment the hook fires, falling back to the generic terminal glyph otherwise. Adds the missing SessionStart/SessionEnd registrations across every agent that exposes them and gives Cursor, Copilot, Gemini, OpenCode, and Pi a v2 dispatch path so they all carry agent identity.
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR extends lifecycle hooks to carry an AgentIdentity from wrappers through hook scripts and the host-service, records last-seen agent identity per terminal in a renderer store, and renders the agent's icon in the terminal pane header. ChangesAgent Lifecycle Hooks and Terminal Binding
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
…d getIcon slot Terminal panes set `renderTitle`, which makes DefaultHeaderContent skip the `getIcon` slot entirely (panes/.../DefaultHeaderContent.tsx:24-26). The visible icon is the TerminalSquare inside TerminalSessionDropdown, so route the agent-icon swap there. The pane-registry getIcon stays wired for defensive consistency in case rendering ever falls back.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 32 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/main/lib/agent-setup/templates/gemini-hook.template.sh">
<violation number="1" location="apps/desktop/src/main/lib/agent-setup/templates/gemini-hook.template.sh:20">
P2: `SessionEnd` is passed through as-is, but the v1 fallback mapper does not recognize `SessionEnd`, so stop events can be dropped on legacy/fallback dispatch.</violation>
</file>
<file name="apps/desktop/src/main/lib/agent-setup/templates/opencode-plugin.template.js">
<violation number="1" location="apps/desktop/src/main/lib/agent-setup/templates/opencode-plugin.template.js:168">
P1: `session.deleted` can misclassify deleted child sessions as root sessions and emit a false `SessionEnd` event.</violation>
</file>
<file name="apps/desktop/src/main/lib/agent-setup/templates/copilot-hook.template.sh">
<violation number="1" location="apps/desktop/src/main/lib/agent-setup/templates/copilot-hook.template.sh:19">
P1: v1 fallback now sends `SessionStart`/`SessionEnd` event types, which can break legacy `/hook/complete` handling that expects `Start`/`Stop`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Greptile SummaryThis PR wires
Confidence Score: 3/5The icon feature is backward-compatible and safe to ship in isolation, but there is a concrete regression for Copilot's working indicator caused by a missing case in mapEventType. The packages/host-service/src/events/map-event-type.ts needs
|
| Filename | Overview |
|---|---|
| packages/host-service/src/events/map-event-type.ts | Not modified in this PR but critical: SessionEnd (PascalCase) is absent from the Stop branch, silently dropping every SessionEnd event from every agent. |
| apps/desktop/src/main/lib/agent-setup/templates/copilot-hook.template.sh | Upgrades to v2 payload and switches sessionEnd → SessionEnd; the rename causes a working-indicator regression due to the unmodified mapEventType. |
| apps/desktop/src/renderer/stores/v2-agent-bindings/store.ts | Well-structured Zustand store with correct isolation and out-of-order protection; minor churn on identical-identity events with newer timestamps. |
| packages/shared/src/agent-identity.ts | New shared type file; clean AgentIdentity definition with proper optional fields and good JSDoc. |
| packages/host-service/src/trpc/router/notifications/notifications.ts | Clean schema extension with proper empty-string normalization; normalizeAgentIdentity handles missing agentId and optional fields correctly. |
| apps/desktop/src/main/lib/agent-setup/templates/pi-extension.template.ts | Adds session_start → SessionStart; missing session_end counterpart despite PR description claiming both are registered. |
| packages/workspace-client/src/lib/eventBus.ts | Correctly passes the optional agent field through the event bus and re-exports AgentIdentity type. |
Sequence Diagram
sequenceDiagram
participant Agent as Agent Process
participant Hook as Hook Script
participant HS as host-service notifications.ts
participant EB as EventBus
participant Store as V2AgentBindingStore
participant Icon as TerminalPaneIcon
Agent->>Hook: fires SessionStart/SessionEnd/Stop
Hook->>Hook: build v2 payload with agent identity
Hook->>HS: POST /trpc/notifications.hook
HS->>HS: normalizeAgentIdentity()
HS->>HS: mapEventType(eventType)
alt SessionStart or Start or Stop
HS->>EB: broadcastAgentLifecycle with agent
EB->>Store: setBinding(terminalId, identity)
Store->>Icon: re-render with preset logo
else SessionEnd PascalCase — BUG
HS-->>Hook: ignored:true (200 OK)
Note over Hook,HS: v1 fallback also skipped
end
Note over Store,Icon: clearBinding on terminal:lifecycle exit
Comments Outside Diff (1)
-
packages/host-service/src/events/map-event-type.ts, line 39-49 (link)SessionEnd(PascalCase) missing fromStopmapping — causes silent drop and working-indicator regressionmapEventTypealready handlesSessionStart(PascalCase →Start), but theStopbranch only listssessionEnd(camelCase) andsession_end(snake_case). Every PascalCaseSessionEndsent by Claude, Mastra, Droid, Gemini, Cursor, and Copilot returnsnull, triggering the early-return{ success: true, ignored: true }path — the event is silently dropped and never broadcast. The most concrete regression is Copilot: before this PRsessionEndwas mapped to"Stop"in the hook script and correctly reset the working indicator; after the PR it maps to"SessionEnd"which is now dropped, leaving the working indicator permanently stuck in the "working" state after a Copilot session ends.Prompt To Fix With AI
This is a comment left during a code review. Path: packages/host-service/src/events/map-event-type.ts Line: 39-49 Comment: **`SessionEnd` (PascalCase) missing from `Stop` mapping — causes silent drop and working-indicator regression** `mapEventType` already handles `SessionStart` (PascalCase → `Start`), but the `Stop` branch only lists `sessionEnd` (camelCase) and `session_end` (snake_case). Every PascalCase `SessionEnd` sent by Claude, Mastra, Droid, Gemini, Cursor, and Copilot returns `null`, triggering the early-return `{ success: true, ignored: true }` path — the event is silently dropped and never broadcast. The most concrete regression is Copilot: before this PR `sessionEnd` was mapped to `"Stop"` in the hook script and correctly reset the working indicator; after the PR it maps to `"SessionEnd"` which is now dropped, leaving the working indicator permanently stuck in the "working" state after a Copilot session ends. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/host-service/src/events/map-event-type.ts:39-49
**`SessionEnd` (PascalCase) missing from `Stop` mapping — causes silent drop and working-indicator regression**
`mapEventType` already handles `SessionStart` (PascalCase → `Start`), but the `Stop` branch only lists `sessionEnd` (camelCase) and `session_end` (snake_case). Every PascalCase `SessionEnd` sent by Claude, Mastra, Droid, Gemini, Cursor, and Copilot returns `null`, triggering the early-return `{ success: true, ignored: true }` path — the event is silently dropped and never broadcast. The most concrete regression is Copilot: before this PR `sessionEnd` was mapped to `"Stop"` in the hook script and correctly reset the working indicator; after the PR it maps to `"SessionEnd"` which is now dropped, leaving the working indicator permanently stuck in the "working" state after a Copilot session ends.
### Issue 2 of 3
apps/desktop/src/renderer/stores/v2-agent-bindings/store.ts:37-45
**Store update on every identical-identity event causes unnecessary re-renders**
The guard skips updates only when all identity fields match AND `existing.lastEventAt >= occurredAt`. For the common case — same `agentId`/`sessionId` but a newer `occurredAt` — the guard fails, writing a new `byTerminalId` object reference and re-rendering all `useV2AgentBindingStore` subscribers even though the displayed icon is unchanged. Consider skipping the update entirely when identity fields are identical regardless of timestamp, or updating only `lastEventAt` without replacing the `identity` reference.
### Issue 3 of 3
apps/desktop/src/main/lib/agent-setup/templates/pi-extension.template.ts:79-82
**`session_end` hook not registered for Pi despite PR description claiming both SessionStart and SessionEnd**
Only `session_start` → `SessionStart` was added. Pi's `session_shutdown` still fires `Stop` so the working indicator recovers, but a dedicated `session_end` hook would provide the canonical session-lifetime signal the PR aims to standardize. If Pi's API doesn't expose `session_end`, the PR description should note the omission.
Reviews (1): Last reviewed commit: "fix(desktop): swap agent icon in Termina..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+37
to
+45
| if ( | ||
| existing && | ||
| existing.identity.agentId === identity.agentId && | ||
| existing.identity.sessionId === identity.sessionId && | ||
| existing.identity.definitionId === identity.definitionId && | ||
| existing.lastEventAt >= occurredAt | ||
| ) { | ||
| return state; | ||
| } |
There was a problem hiding this comment.
The guard skips updates only when all identity fields match AND existing.lastEventAt >= occurredAt. For the common case — same agentId/sessionId but a newer occurredAt — the guard fails, writing a new byTerminalId object reference and re-rendering all useV2AgentBindingStore subscribers even though the displayed icon is unchanged. Consider skipping the update entirely when identity fields are identical regardless of timestamp, or updating only lastEventAt without replacing the identity reference.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/stores/v2-agent-bindings/store.ts
Line: 37-45
Comment:
**Store update on every identical-identity event causes unnecessary re-renders**
The guard skips updates only when all identity fields match AND `existing.lastEventAt >= occurredAt`. For the common case — same `agentId`/`sessionId` but a newer `occurredAt` — the guard fails, writing a new `byTerminalId` object reference and re-rendering all `useV2AgentBindingStore` subscribers even though the displayed icon is unchanged. Consider skipping the update entirely when identity fields are identical regardless of timestamp, or updating only `lastEventAt` without replacing the `identity` reference.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+79
to
+82
| pi.on("session_start", (_event, ctx) => { | ||
| if (skip(ctx)) return; | ||
| fire("SessionStart"); | ||
| }); |
There was a problem hiding this comment.
session_end hook not registered for Pi despite PR description claiming both SessionStart and SessionEnd
Only session_start → SessionStart was added. Pi's session_shutdown still fires Stop so the working indicator recovers, but a dedicated session_end hook would provide the canonical session-lifetime signal the PR aims to standardize. If Pi's API doesn't expose session_end, the PR description should note the omission.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/lib/agent-setup/templates/pi-extension.template.ts
Line: 79-82
Comment:
**`session_end` hook not registered for Pi despite PR description claiming both SessionStart and SessionEnd**
Only `session_start` → `SessionStart` was added. Pi's `session_shutdown` still fires `Stop` so the working indicator recovers, but a dedicated `session_end` hook would provide the canonical session-lifetime signal the PR aims to standardize. If Pi's API doesn't expose `session_end`, the PR description should note the omission.
How can I resolve this? If you propose a fix, please make it concise.SessionStart fires when an agent attaches to a terminal — the agent is idle, waiting for the first prompt. Mapping it to `Start` was making the pane show the working spinner before the user typed anything. Split session-lifetime events out of the per-turn cadence: `Attached`/`Detached` drive the pane icon binding only and never touch the working indicator or play any sound. Per-turn `Start`/`Stop` keeps its prior behavior.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/main/lib/agent-setup/templates/cursor-hook.template.sh (1)
57-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
SessionStart/SessionEndfall through to the v1 curl when the v2 POST fails — consider adding a guard.
notify-hook.template.shprotects its v1 path with:[ -z "$SUPERSET_TAB_ID" ] && [ -z "$SESSION_ID" ] && exit 0
cursor-hook.template.shhas no equivalent guard before the v1curl. SinceSessionStart/SessionEndare v2-only events (the v1/hook/completeserver doesn't understand them), when the v2 POST times out or returns non-2xx the script unconditionally fires a v1 request witheventType=SessionStartoreventType=SessionEndand a potentially emptySUPERSET_TAB_ID— producing spurious and semantically incorrect v1 traffic.Adding a one-line guard before the v1 block mirrors the pattern in
notify-hook.template.shand suppresses these calls:🛡️ Proposed fix
+# v1 fallback for terminals running against the legacy electron localhost server. +# Skip if there is no legacy tab context (pure v2 terminal). +[ -z "$SUPERSET_TAB_ID" ] && exit 0 curl -sG "http://127.0.0.1:${SUPERSET_PORT:-{{DEFAULT_PORT}}}/hook/complete" \🤖 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 `@apps/desktop/src/main/lib/agent-setup/templates/cursor-hook.template.sh` around lines 57 - 66, Before the v1 curl block in cursor-hook.template.sh add the same guard used in notify-hook.template.sh so v1 requests are suppressed for v2-only events: check [ -z "$SUPERSET_TAB_ID" ] && [ -z "$SESSION_ID" ] and skip/exit when both are empty so SessionStart/SessionEnd (v2-only) don't fall through to the v1 /hook/complete curl; place this check immediately before the curl that uses SUPERSET_TAB_ID, SUPERSET_TAB_ID/SESSION_ID and EVENT_TYPE to mirror the notify-hook.template.sh pattern.apps/desktop/src/main/lib/agent-setup/agent-wrappers-claude-codex-opencode.ts (1)
411-433:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCodex hooks.json should inline
SUPERSET_AGENT_ID=codexlike Claude does.
getClaudeManagedHookCommand(line 84-86) deliberately prependsSUPERSET_AGENT_ID=claudeso the v2 identity flows through even when the user launches the agent outside the Superset wrapper (system PATH → real binary, no wrapper env vars). The Codex hooks.json registration here uses barenotifyScriptPathforSessionStart/UserPromptSubmit/Stop, so a user invokingcodexdirectly will still trigger the global hooks but the v2 payload'sagent.agentIdwill be empty, the host service will drop theagentfield during normalization, and the pane icon will never bind. This silently regresses the documented goal ("show the agent's icon… complementing… outside the wrapper") for Codex.🛠️ Suggested fix
+ // Mirror getClaudeManagedHookCommand: inline SUPERSET_AGENT_ID so the v2 + // payload carries identity even when codex is launched outside the wrapper. + const codexCommand = `SUPERSET_AGENT_ID=codex ${notifyScriptPath}`; + const managedEvents: Array<{ eventName: "SessionStart" | "UserPromptSubmit" | "Stop"; definition: ClaudeHookDefinition; }> = [ { eventName: "SessionStart", definition: { - hooks: [{ type: "command", command: notifyScriptPath }], + hooks: [{ type: "command", command: codexCommand }], }, }, { eventName: "UserPromptSubmit", definition: { - hooks: [{ type: "command", command: notifyScriptPath }], + hooks: [{ type: "command", command: codexCommand }], }, }, { eventName: "Stop", definition: { - hooks: [{ type: "command", command: notifyScriptPath }], + hooks: [{ type: "command", command: codexCommand }], }, }, ];You'll likely want to also update
isManagedCodexHookCommand/isSupersetManagedHookCommandmatching to recognize the new prefix during reconciliation, otherwise existing entries written by previous installs will be deduped against the new form.🤖 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 `@apps/desktop/src/main/lib/agent-setup/agent-wrappers-claude-codex-opencode.ts` around lines 411 - 433, The Codex managedEvents entries use bare notifyScriptPath so the v2 agent id is not injected; update the three definitions inside managedEvents (SessionStart, UserPromptSubmit, Stop) to use the same env-prefix form as getClaudeManagedHookCommand (i.e. prepend SUPERSET_AGENT_ID=codex to the command string) so agent.agentId is preserved when users run the real codex binary; also update isManagedCodexHookCommand and isSupersetManagedHookCommand matching logic to recognize and dedupe both the old bare notifyScriptPath and the new SUPERSET_AGENT_ID=codex-prefixed form during reconciliation.
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/agent-setup/notify-hook.test.ts (1)
61-82: 💤 Low valueLGTM – parameterized template coverage is a good addition.
One optional note:
expectedV2Payload(Line 62–63) is character-for-character identical to the literal inside thegetNotifyScriptContentdescribe block at Line 17. Hoisting it to a module-level constant would mean a single update point if the payload shape ever changes.🤖 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 `@apps/desktop/src/main/lib/agent-setup/notify-hook.test.ts` around lines 61 - 82, Hoist the duplicate expectedV2Payload string into a module-level constant and reuse it in both the "per-agent hook scripts dispatch to v2" test and the getNotifyScriptContent describe block to avoid divergence; replace the local expectedV2Payload declaration in the inner describe with a reference to the new shared constant and update any imports/usages accordingly so both the template parameterized tests (loop over "cursor-hook.template.sh", "copilot-hook.template.sh", "gemini-hook.template.sh") and the getNotifyScriptContent tests compare against the single source of truth.
🤖 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 `@apps/desktop/src/main/lib/agent-setup/agent-wrappers-mastra.ts`:
- Line 71: The notifyCommand currently built as `const notifyCommand = \`bash
${quoteShellPath(notifyScriptPath)}\`` lacks the inline SUPERSET_AGENT_ID env
var; update notifyCommand to prefix the invocation with
`SUPERSET_AGENT_ID=mastracode` (e.g. `SUPERSET_AGENT_ID=mastracode bash <path>`)
so the v2 hook payload includes agent.agentId; locate where notifyCommand is
defined (uses quoteShellPath and notifyScriptPath and is passed into
buildWrapperScript which receives agentId: "mastracode") and change the string
construction to include the env var inline.
In `@apps/desktop/src/main/lib/agent-setup/templates/opencode-plugin.template.js`:
- Around line 150-172: The session.deleted handler can misidentify child
sessions because isChildSession(sessionID) may fail after the session is
removed; change the logic in the event.type === "session.deleted" branch to
first check event.properties?.info?.parentID (use
Boolean(event.properties?.info?.parentID)) and only fall back to await
isChildSession(sessionID) when parentID is not present; then call await
notify("SessionEnd") only if the combined child-check is false (also handle null
sessionID safely). Update the symbols in that block: event.type,
event.properties?.info?.parentID, sessionID, isChildSession, and notify.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/components/HostNotificationSubscriber/HostNotificationSubscriber.tsx`:
- Around line 43-47: The lifecycle handler in HostNotificationSubscriber
currently only sets a binding when payload.agent exists, leaving stale bindings
when payload.agent is absent; update the else path to clear the terminal's
binding by calling
useV2AgentBindingStore.getState().setBinding(payload.terminalId, null /* or
undefined */ , payload.occurredAt) or the store's clearBinding method if one
exists, so the UI falls back to the generic icon immediately; modify the branch
around payload.agent to set or clear via setBinding(payload.terminalId, ...)
accordingly.
In `@apps/desktop/src/renderer/stores/v2-agent-bindings/store.ts`:
- Around line 34-52: In setBinding, guard against stale out-of-order events by
first checking if an existing binding exists and if existing.lastEventAt >=
occurredAt and returning state immediately (using state.byTerminalId and
lastEventAt), then keep the identity-equality short-circuit (compare
existing.identity.agentId/sessionId/definitionId) to avoid unnecessary writes;
otherwise update state.byTerminalId with the new { identity, lastEventAt:
occurredAt } — this ensures late events for a different identity cannot clobber
a newer binding.
---
Outside diff comments:
In
`@apps/desktop/src/main/lib/agent-setup/agent-wrappers-claude-codex-opencode.ts`:
- Around line 411-433: The Codex managedEvents entries use bare notifyScriptPath
so the v2 agent id is not injected; update the three definitions inside
managedEvents (SessionStart, UserPromptSubmit, Stop) to use the same env-prefix
form as getClaudeManagedHookCommand (i.e. prepend SUPERSET_AGENT_ID=codex to the
command string) so agent.agentId is preserved when users run the real codex
binary; also update isManagedCodexHookCommand and isSupersetManagedHookCommand
matching logic to recognize and dedupe both the old bare notifyScriptPath and
the new SUPERSET_AGENT_ID=codex-prefixed form during reconciliation.
In `@apps/desktop/src/main/lib/agent-setup/templates/cursor-hook.template.sh`:
- Around line 57-66: Before the v1 curl block in cursor-hook.template.sh add the
same guard used in notify-hook.template.sh so v1 requests are suppressed for
v2-only events: check [ -z "$SUPERSET_TAB_ID" ] && [ -z "$SESSION_ID" ] and
skip/exit when both are empty so SessionStart/SessionEnd (v2-only) don't fall
through to the v1 /hook/complete curl; place this check immediately before the
curl that uses SUPERSET_TAB_ID, SUPERSET_TAB_ID/SESSION_ID and EVENT_TYPE to
mirror the notify-hook.template.sh pattern.
---
Nitpick comments:
In `@apps/desktop/src/main/lib/agent-setup/notify-hook.test.ts`:
- Around line 61-82: Hoist the duplicate expectedV2Payload string into a
module-level constant and reuse it in both the "per-agent hook scripts dispatch
to v2" test and the getNotifyScriptContent describe block to avoid divergence;
replace the local expectedV2Payload declaration in the inner describe with a
reference to the new shared constant and update any imports/usages accordingly
so both the template parameterized tests (loop over "cursor-hook.template.sh",
"copilot-hook.template.sh", "gemini-hook.template.sh") and the
getNotifyScriptContent tests compare against the single source of truth.
🪄 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: cf23c9ec-ed39-4d25-9fba-4831cfd2343a
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
apps/desktop/plans/20260507-1659-agent-lifecycle-hooks-terminal-binding.mdapps/desktop/src/main/lib/agent-setup/agent-wrappers-amp.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers-claude-codex-opencode.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers-common.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers-copilot.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers-cursor.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers-droid.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers-gemini.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers-mastra.tsapps/desktop/src/main/lib/agent-setup/notify-hook.test.tsapps/desktop/src/main/lib/agent-setup/templates/copilot-hook.template.shapps/desktop/src/main/lib/agent-setup/templates/cursor-hook.template.shapps/desktop/src/main/lib/agent-setup/templates/gemini-hook.template.shapps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.shapps/desktop/src/main/lib/agent-setup/templates/opencode-plugin.template.jsapps/desktop/src/main/lib/agent-setup/templates/pi-extension.template.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalPaneIcon/TerminalPaneIcon.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalPaneIcon/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxapps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/components/HostNotificationSubscriber/HostNotificationSubscriber.tsxapps/desktop/src/renderer/stores/v2-agent-bindings/index.tsapps/desktop/src/renderer/stores/v2-agent-bindings/store.test.tsapps/desktop/src/renderer/stores/v2-agent-bindings/store.tspackages/host-service/src/events/types.tspackages/host-service/src/trpc/router/notifications/notifications.test.tspackages/host-service/src/trpc/router/notifications/notifications.tspackages/shared/package.jsonpackages/shared/src/agent-identity.tspackages/workspace-client/package.jsonpackages/workspace-client/src/index.tspackages/workspace-client/src/lib/eventBus.ts
Contributor
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
`@apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/statusTransitions.ts`:
- Around line 37-39: Split the current combined check for payload.eventType so
"Attached" remains the noop (return { clearSources: [], setStatus: null }) but
"Detached" explicitly clears transient pane state: in the branch where
payload.eventType === "Detached" return an object that lists the transient
sources to clear (e.g., include "working" and "permission" in clearSources) and
set setStatus to null while not clearing the "review" source; update the code
around payload.eventType, "Attached", "Detached" and the returned {
clearSources: [], setStatus } to implement this behavior.
In `@packages/host-service/src/events/map-event-type.ts`:
- Around line 28-40: The lifecycle mapping currently only checks PascalCase
"Attached"/"Detached" (and mixed-case session variants) and thus misses
lowercase emits; update the conditional checks that examine eventType (the
comparisons around eventType === "Attached" ... and eventType === "Detached"
...) to also accept the lowercase aliases "attached" and "detached" (and ensure
any sessionStart/sessionEnd checks already handling lowercase forms remain
consistent), so the function that maps eventType returns "Attached"/"Detached"
for those lowercase inputs as well.
🪄 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: def83361-2630-4024-b833-1aaac8aa9c14
📒 Files selected for processing (6)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.tsxapps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/lifecycleEvents.tsapps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/statusTransitions.test.tsapps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/statusTransitions.tspackages/host-service/src/events/map-event-type.test.tspackages/host-service/src/events/map-event-type.ts
✅ Files skipped from review due to trivial changes (2)
- packages/host-service/src/events/map-event-type.test.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.tsx
…nal gates In v2 host-service terminals only SUPERSET_TERMINAL_ID is set in the PTY env (packages/host-service/src/terminal/env.ts:183) — SUPERSET_TAB_ID is v1-only. Several wrapper guards only checked SUPERSET_TAB_ID, so: - The codex wrapper's TUI session-log watcher (the per-turn Start/PermissionRequest source for codex) never started in v2, leaving codex panes stuck on idle while generating. Codex's native ~/.codex/hooks.json fires unreliably in interactive TUI mode, which was the original reason for the watcher. - The OpenCode plugin returned early before installing handlers in v2. - The Copilot wrapper skipped its hooks.json injection in v2. All three now gate on either v1 (SUPERSET_TAB_ID) or v2 (SUPERSET_TERMINAL_ID). Also rename `--enable codex_hooks` → `--enable hooks` in the codex wrapper: the older flag name still works via deprecation alias but prints a noisy warning every launch (codex 0.129+).
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/main/lib/agent-setup/templates/codex-wrapper-exec.template.sh">
<violation number="1" location="apps/desktop/src/main/lib/agent-setup/templates/codex-wrapper-exec.template.sh:22">
P1: The rollout watcher is not process-scoped: it can attach to another Codex session’s `rollout-*.jsonl` file and emit lifecycle events for the wrong terminal.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Codex's official hook surface (~/.codex/hooks.json with SessionStart, UserPromptSubmit, Stop) is the right integration point. The session-log watcher was a workaround for older codex builds whose log shape no longer exists in 0.129+; keeping it as an alternative path adds complexity for no real benefit. The wrapper is now a one-liner: forward to the real binary with --enable hooks and the legacy notify=... callback for Stop parity.
Contributor
There was a problem hiding this comment.
2 issues found across 26 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/stores/v2-agent-bindings/store.ts">
<violation number="1" location="apps/desktop/src/renderer/stores/v2-agent-bindings/store.ts:37">
P1: The equality short-circuit drops newer same-identity events, so `lastEventAt` can go stale and allow delayed older events to overwrite the binding.</violation>
</file>
<file name="apps/desktop/src/main/lib/agent-setup/templates/codex-wrapper-exec.template.sh">
<violation number="1" location="apps/desktop/src/main/lib/agent-setup/templates/codex-wrapper-exec.template.sh:70">
P2: Watcher shutdown only targets the subshell PID; terminate child pipeline processes too to avoid orphaned `tail` processes.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
| existing && | ||
| existing.identity.agentId === identity.agentId && | ||
| existing.identity.sessionId === identity.sessionId && | ||
| existing.identity.definitionId === identity.definitionId |
Contributor
There was a problem hiding this comment.
P1: The equality short-circuit drops newer same-identity events, so lastEventAt can go stale and allow delayed older events to overwrite the binding.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/stores/v2-agent-bindings/store.ts, line 37:
<comment>The equality short-circuit drops newer same-identity events, so `lastEventAt` can go stale and allow delayed older events to overwrite the binding.</comment>
<file context>
@@ -27,12 +27,14 @@ export const useV2AgentBindingStore = create<V2AgentBindingState>((set) => ({
existing.identity.sessionId === identity.sessionId &&
- existing.identity.definitionId === identity.definitionId &&
- existing.lastEventAt >= occurredAt
+ existing.identity.definitionId === identity.definitionId
) {
return state;
</file context>
Suggested change
| existing.identity.definitionId === identity.definitionId | |
| existing.identity.definitionId === identity.definitionId && | |
| existing.lastEventAt >= occurredAt |
Tip: Review your code locally with the cubic CLI to iterate faster.
| kill "$SUPERSET_CODEX_START_WATCHER_PID" >/dev/null 2>&1 || true | ||
| wait "$SUPERSET_CODEX_START_WATCHER_PID" 2>/dev/null || true | ||
| if [ -n "$SUPERSET_CODEX_SESSION_WATCHER_PID" ]; then | ||
| kill "$SUPERSET_CODEX_SESSION_WATCHER_PID" >/dev/null 2>&1 || true |
Contributor
There was a problem hiding this comment.
P2: Watcher shutdown only targets the subshell PID; terminate child pipeline processes too to avoid orphaned tail processes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/agent-setup/templates/codex-wrapper-exec.template.sh, line 70:
<comment>Watcher shutdown only targets the subshell PID; terminate child pipeline processes too to avoid orphaned `tail` processes.</comment>
<file context>
@@ -1,5 +1,75 @@
+_superset_debug "codex exited status=$SUPERSET_CODEX_STATUS"
+
+if [ -n "$SUPERSET_CODEX_SESSION_WATCHER_PID" ]; then
+ kill "$SUPERSET_CODEX_SESSION_WATCHER_PID" >/dev/null 2>&1 || true
+ wait "$SUPERSET_CODEX_SESSION_WATCHER_PID" 2>/dev/null || true
+ _superset_debug "session watcher stopped pid=$SUPERSET_CODEX_SESSION_WATCHER_PID"
</file context>
Linked v2 presets now store the host-service agent config id in `agentId` so multiple agents sharing a presetId (e.g., two Claude configs) each get their own pill and resolver path. Older rows that store presetId still resolve via fallback, and V2PresetsSection auto-migrates them when the config id is uniquely determined. Also: useV2PresetExecution refetches the live agent list at launch time so command edits take effect without a remount; AgentDetail's onChanged now hands the updated row back so the cache can be patched optimistically; QuickAddPresets accepts a separate iconId.
Contributor
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
… form Bump CURSOR_HOOK_VERSION and GEMINI_HOOK_VERSION to v2 so existing installs re-detect the managed marker and replace stale wrapper paths (e.g., from prior worktree-style data dirs). Gemini's `isManaged` predicate now also matches definitions whose top-level `command` points at the managed script, in addition to the nested `hooks[].command` form, so both shapes get cleaned up.
Adds an Amp system plugin at ~/.config/amp/plugins/superset-lifecycle.ts that bridges Amp's session.start / agent.start / agent.end events into the existing notify hook. The wrapper now also exports SUPERSET_AGENT_ID so the plugin's spawned notifier inherits Superset env context. Registered as a new amp-plugin setup action that runs alongside the existing amp-wrapper.
The linked-agent branch of PresetEditorDialog stacked an alert banner
on top of a read-only command field with two near-duplicate hints
('Edit the command in Agents settings' + 'Read-only — managed in
Agents settings'). Drops the banner and the second hint; replaces them
with a single inline 'Edit in <agent name>' link beside the Command
label. The muted mono command block carries the read-only signal on
its own. Missing/disabled-agent caption is preserved.
4 tasks
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.
Summary
AgentIdentity(agentId, optionalsessionId/definitionId) end-to-end on the existing v2agent:lifecyclehook so the running agent is identifiable per terminal — defined once in@superset/shared/agent-identityand reused by the hook receiver, the WS event bus, and the renderer.useV2AgentBindingStore) is keyed byterminalId, set byHostNotificationSubscriber, cleared onterminal:lifecycle exit.SessionStart/SessionEndhooks on every agent that natively exposes them (Claude, Mastra, Droid, Gemini, Cursor, OpenCode, Pi). Codex schema has noSessionEndso onlySessionStartwas added there; Copilot already had both.SUPERSET_AGENT_IDso the same payload shape flows through.Test plan
bun run typecheckclean across 29 packagesbun run lintcleannotify-hook.test.ts(6/6) — asserts v2 payload includesagent.agentId/agent.sessionId; new cases assert cursor/copilot/gemini scripts each contain the v2 dispatch + payloadnotifications.test.ts(6/6) — new cases for identity passthrough, empty-string normalization, missingagentIddropagent-wrappers.test.ts(29/29)v2-agent-bindings/store.test.ts(5/5) — set/clear, retain on identical Stop, replace on different sessionId/agentId, isolation\$SUPERSET_HOST_AGENT_HOOK_URLfromcursor-hook.template.shcorrectly fails the new v2-dispatch testclaudein a v2 terminal → Claude logo replaces terminal glyph;/exitthencodex→ switches to Codex glyph; close terminal → glyph clearsNotes
SUPERSET_AGENT_IDcontinue to work and just don't trigger the icon swap.apps/desktop/plans/20260507-1659-agent-lifecycle-hooks-terminal-binding.md.Summary by cubic
Shows the running agent’s icon in v2 terminal panes and links v2 terminal presets to host-service agent configs for live updates. Adds Amp lifecycle via a system plugin and splits session lifecycle from per-turn signals so panes don’t show false “working” states (v2-only via SUPERSET_TERMINAL_ID + SUPERSET_HOST_AGENT_HOOK_URL).
New Features
AgentIdentityend-to-end; defined in@superset/shared/agent-identityand re-exported from@superset/workspace-client. Bound perterminalId, set onagent:lifecycle, cleared onterminal:lifecycle exit.TerminalPaneIconswaps the pane icon to the agent logo (Claude, Codex, Gemini, Cursor, Copilot, OpenCode, Amp, Mastracode, Pi, Droid); used inTerminalSessionDropdownwith pane-registrygetIconas a fallback.Attached/Detached(icon binding only).Start/Stopkeep driving status and the chime.session.start/agent.start/agent.endto our notify hook; the wrapper exportsSUPERSET_AGENT_IDso identity flows into the payload.presetId) for accurate commands and icons; older rows fall back topresetIdand auto-migrate when unambiguous. Launcher refetches configs at run time; Agents settings patches cache on save; QuickAdd accepts a separateiconId.Bug Fixes
SUPERSET_TERMINAL_ID.--enable codex_hooksto--enable hooks; inlinesSUPERSET_AGENT_ID=codex.SUPERSET_AGENT_ID. Gemini also recognizes flatcommandwhen cleaning old managed entries.Attachedfor status and clears transient status onDetached, while still updating the icon binding.Written for commit 94c0f4a. Summary will update on new commits.
Summary by CodeRabbit
New Features
Behavior Changes
Tests