Surface a sub-agent blocked on an approval - #125
Merged
Conversation
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe replay harness now creates approval-blocked sub-agent threads. The client resolves hidden child metadata, hoists approval activity, updates notifications and sub-agent status indicators, and supports direct approval navigation. UI, specification, documentation, and Playwright coverage validate the flow. ChangesApproval-blocked sub-agent flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ParentThread
participant ReplayHarness
participant ChildThread
participant Browser
participant NotificationAPI
ParentThread->>ReplayHarness: trigger approval sub-agent
ReplayHarness->>ChildThread: create delayed child turn
ChildThread->>Browser: emit approval activity
Browser->>Browser: resolve metadata and hoist activity
Browser->>NotificationAPI: show child approval notification
NotificationAPI->>Browser: focus child thread
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/giskard-server/src/bin/giskard-server-replay.rs (1)
131-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThird copy of the same "wait for WebSocket receiver" poll loop.
spawn_subagent_turn,spawn_nested_subagent_turn, and nowspawn_approval_subagent_turneach duplicate the identical 2s/10ms receiver-count poll (lines 136-143 here). Worth extracting once.♻️ Proposed helper
async fn wait_for_receiver(sender: &broadcast::Sender<AgentEvent>) -> bool { let deadline = tokio::time::Instant::now() + tokio::time::Duration::from_secs(2); while sender.receiver_count() == 0 && tokio::time::Instant::now() < deadline { tokio::time::sleep(tokio::time::Duration::from_millis(10)).await; } sender.receiver_count() > 0 }Each spawn function then starts with
if !Self::wait_for_receiver(&sender).await { return; }.🤖 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 `@crates/giskard-server/src/bin/giskard-server-replay.rs` around lines 131 - 172, The receiver-count polling logic is duplicated across spawn_subagent_turn, spawn_nested_subagent_turn, and spawn_approval_subagent_turn. Extract it into a shared async wait_for_receiver helper using the existing 2-second deadline and 10ms polling interval, then have each spawn function return early when the helper reports no receiver.
🤖 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 `@crates/giskard-server/src/bin/giskard-server-replay.rs`:
- Around line 80-82: Replace the singleton active_approval state with a shared
map keyed by ApprovalId, updating the relevant replay state and initialization.
Have both spawn_approval_subagent_turn and start_turn’s approval path insert
their ThreadId/TurnId under the correct approval ID, and update respond_approval
to use its request ID for lookup and acknowledgement instead of discarding it.
Preserve independent pending approvals so concurrent approvals cannot overwrite
or misattribute confirmations.
In `@crates/giskard-server/static/app.css`:
- Around line 112-114: Update the text-shadow declaration in
.thread.activity-subagent .thread-status to use the lowercase CSS keyword
currentcolor instead of currentColor, satisfying the value-keyword-case style
rule while preserving the existing styling.
---
Nitpick comments:
In `@crates/giskard-server/src/bin/giskard-server-replay.rs`:
- Around line 131-172: The receiver-count polling logic is duplicated across
spawn_subagent_turn, spawn_nested_subagent_turn, and
spawn_approval_subagent_turn. Extract it into a shared async wait_for_receiver
helper using the existing 2-second deadline and 10ms polling interval, then have
each spawn function return early when the helper reports no receiver.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 00001c34-e901-4a98-a856-e08619a8f023
📒 Files selected for processing (9)
crates/giskard-server/src/bin/giskard-server-replay.rscrates/giskard-server/static/app.csscrates/giskard-server/static/app.jscrates/giskard-server/tests/ui.rsdocs/subagents.mdspecs/giskard-specification.mdtests/e2e/README.mdtests/e2e/tests/helpers.tstests/e2e/tests/subagent-approvals.spec.ts
marmeladema
force-pushed
the
claude/sub-agent-approval-requests-vffexn
branch
from
July 25, 2026 10:30
9fb9dd8 to
33c4974
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/giskard-server/static/app.js (2)
4518-4534: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueCard state and summary can disagree.
stateLabelcomes from the whole subtree (subagentNeedsApproval), butsummarystill usessubagentActivityForThread(thread.id)— this thread's own activity. A card reading "Needs approval" because a grandchild is blocked will show the parent label or an unrelated summary. Sourcing the summary from the winning subtree activity would keep the two consistent.🤖 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 `@crates/giskard-server/static/app.js` around lines 4518 - 4534, Update renderSubagentCard so the summary is sourced from the same winning subtree activity used by subagentNeedsApproval and stateLabel, rather than only subagentActivityForThread(thread.id). Preserve the existing parentLabel fallback, and ensure approval, running, error, and idle state labels display a summary from the corresponding subtree activity when available.
877-892: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRepeated linear cache scans make indicator repaints super-linear.
knownThreadMetascans every cached project's thread array.effectiveThreadActivitycallsactivityHostThreadIdfor each activity entry, and each of those walks the ancestor chain with one full scan per hop;renderAllThreadActivityIndicatorsthen repeats this per row. A single id→threadMaprebuilt inrememberProjectThreadswould collapse this to O(1) lookups. Fine at current scale, worth doing before thread counts grow.Also applies to: 1029-1041
🤖 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 `@crates/giskard-server/static/app.js` around lines 877 - 892, Replace repeated project-thread array scans with a single id-to-thread Map rebuilt by rememberProjectThreads. Update knownThreadMeta and activityHostThreadId (used by effectiveThreadActivity and renderAllThreadActivityIndicators) to resolve thread IDs through that Map while preserving existing metadata and ancestor behavior. Ensure the Map is refreshed whenever cached project threads are remembered.
🤖 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 `@crates/giskard-server/static/app.js`:
- Around line 937-955: Prevent repeated stale-list refreshes for unresolved
thread IDs by adding a set of attempted IDs near staleThreadListRefreshTimer,
gating the handleThreadActivity scheduling path with that set, and recording
each missing tid before calling scheduleStaleThreadListRefresh. Keep the
existing burst debounce and refreshKnownThreadLists behavior unchanged.
- Around line 2212-2224: Reserve the approval deduplication key in the
notification handler before the conditional refresh and other awaits, then
remove the later state.notifiedApprovals.set call. Ensure every failure path
that returns without successfully notifying clears the reserved key, while
preserving the existing deduplication behavior for successful notifications.
- Around line 1064-1068: Regenerate the IDE screenshots to reflect the new
activity-approval and activity-subagent row states, state-approval badge, and
needs-approval card styling, then update docs/screenshots/ide-desktop.png and
docs/screenshots/ide-mobile.png with the resulting images.
---
Nitpick comments:
In `@crates/giskard-server/static/app.js`:
- Around line 4518-4534: Update renderSubagentCard so the summary is sourced
from the same winning subtree activity used by subagentNeedsApproval and
stateLabel, rather than only subagentActivityForThread(thread.id). Preserve the
existing parentLabel fallback, and ensure approval, running, error, and idle
state labels display a summary from the corresponding subtree activity when
available.
- Around line 877-892: Replace repeated project-thread array scans with a single
id-to-thread Map rebuilt by rememberProjectThreads. Update knownThreadMeta and
activityHostThreadId (used by effectiveThreadActivity and
renderAllThreadActivityIndicators) to resolve thread IDs through that Map while
preserving existing metadata and ancestor behavior. Ensure the Map is refreshed
whenever cached project threads are remembered.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 2736ce74-ecd1-43fb-a54d-c4b28244a792
📒 Files selected for processing (9)
crates/giskard-server/src/bin/giskard-server-replay.rscrates/giskard-server/static/app.csscrates/giskard-server/static/app.jscrates/giskard-server/tests/ui.rsdocs/subagents.mdspecs/giskard-specification.mdtests/e2e/README.mdtests/e2e/tests/helpers.tstests/e2e/tests/subagent-approvals.spec.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/e2e/README.md
- crates/giskard-server/static/app.css
- crates/giskard-server/tests/ui.rs
- specs/giskard-specification.md
- tests/e2e/tests/helpers.ts
- crates/giskard-server/src/bin/giskard-server-replay.rs
marmeladema
force-pushed
the
claude/sub-agent-approval-requests-vffexn
branch
from
July 25, 2026 10:49
33c4974 to
b1f71c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/giskard-server/static/app.js (2)
4497-4519: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueEach card scans the activity map three times.
subagentIsRunning,subagentNeedsApproval, andsubagentSubtreeActivityindependently callsubagentSubtreeActivities, which walks the ownership chain for every activity entry. Computing the subtree list once per thread and deriving all three from it would cut the work ~3× insubagentCountsandrenderSubagentCard.🤖 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 `@crates/giskard-server/static/app.js` around lines 4497 - 4519, Refactor subagentCounts and renderSubagentCard to compute subagentSubtreeActivities(thread.id) once per thread, then derive running, approval, and ranked activity state from that shared list. Update subagentIsRunning, subagentNeedsApproval, and subagentSubtreeActivity or their callers to accept and reuse the precomputed activities without rescanning the activity map, preserving the existing ranking and origin behavior.
1063-1075: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider hoisting host resolution out of the per-row loop.
effectiveThreadActivityrescans all ofstate.threadActivityand re-walks the ownership chain viaactivityHostThreadIdfor every sidebar row, so a full repaint is O(rows × activities × depth). A single pass buildinghost → best activitybefore rendering all rows would collapse this to one walk per activity entry.🤖 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 `@crates/giskard-server/static/app.js` around lines 1063 - 1075, The effectiveThreadActivity path currently rescans state.threadActivity and resolves ownership for every sidebar row; hoist this work into a single precomputed host-to-best-activity map before the row-rendering loop. Update effectiveThreadActivity and its callers to reuse that map while preserving the existing ranking and origin-selection behavior.
🤖 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 `@tests/e2e/tests/subagent-approvals.spec.ts`:
- Around line 268-297: Add an explicit expect(child).toBeTruthy() immediately
after childThreadId resolves in the “notifies once when two events describe the
same approval” test, before recording notifications or invoking
handleThreadActivity. Keep the existing deduplication assertions unchanged so a
failed child-thread lookup cannot make the test pass without exercising the
dedup path.
---
Nitpick comments:
In `@crates/giskard-server/static/app.js`:
- Around line 4497-4519: Refactor subagentCounts and renderSubagentCard to
compute subagentSubtreeActivities(thread.id) once per thread, then derive
running, approval, and ranked activity state from that shared list. Update
subagentIsRunning, subagentNeedsApproval, and subagentSubtreeActivity or their
callers to accept and reuse the precomputed activities without rescanning the
activity map, preserving the existing ranking and origin behavior.
- Around line 1063-1075: The effectiveThreadActivity path currently rescans
state.threadActivity and resolves ownership for every sidebar row; hoist this
work into a single precomputed host-to-best-activity map before the
row-rendering loop. Update effectiveThreadActivity and its callers to reuse that
map while preserving the existing ranking and origin-selection 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: CHILL
Plan: Pro Plus
Run ID: 12b15c39-c66f-46f9-afdc-0bb92f2317cf
📒 Files selected for processing (9)
crates/giskard-server/src/bin/giskard-server-replay.rscrates/giskard-server/static/app.csscrates/giskard-server/static/app.jscrates/giskard-server/tests/ui.rsdocs/subagents.mdspecs/giskard-specification.mdtests/e2e/README.mdtests/e2e/tests/helpers.tstests/e2e/tests/subagent-approvals.spec.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/giskard-server/static/app.css
- tests/e2e/README.md
- tests/e2e/tests/helpers.ts
- crates/giskard-server/tests/ui.rs
- specs/giskard-specification.md
- crates/giskard-server/src/bin/giskard-server-replay.rs
Approvals already routed correctly into a managed sub-agent thread and were answerable once you opened it, but nothing told you it was blocked. A managed child is deliberately absent from the sidebar, so every browser affordance keyed to a thread id quietly no-opped for it: no status symbol on any row, a notification titled with a bare id prefix, and a click that dead-ended on "Approval thread is not in the current project list." The child just looked hung. Three fixes: - Resolve thread identity from the cached per-project thread summaries (which include managed sub-agents) instead of from the rendered sidebar row. This alone restores notification naming and click-to-navigate. Lookups go through an id index, since hoisting resolves ids once per activity entry, then per ancestor hop, then again per sidebar row. - Hoist a hidden child's activity onto the nearest ancestor that does have a row, showing the most urgent state among the row and its hidden descendants (approval > error > active turn), naming the originating child in the tooltip and marking the state as hoisted. Ancestor and descendant walks are bounded so cyclic ownership terminates. - Give the header sub-agent monitor and its cards a distinct "needs approval" state: an approval also marks the turn active, so a blocked child previously rendered identically to a busy one. A card covers its whole owned subtree, since nested descendants are not listed separately, and takes its summary from the same subtree activity that sets its state. Because the server materializes a child on its own, activity can arrive for a thread the browser has never listed; it now refreshes its cached lists before naming or navigating to such a thread, so the first approval from a brand-new child is still attributable. That refresh is bounded per id — some ids never resolve, such as trailing activity from a deleted thread, and an ungated retry would re-fetch every project list for the rest of the turn. It also exposed a latent bug: rebuilding the sidebar rows discards the selection highlight with the old DOM, and every existing caller happened to follow the reload with openThread, which re-derives it. loadThreads now restores it itself. Since the refresh sits between the notification dedup check and the record, the dedup key is claimed before awaiting it, so two events describing one approval cannot both notify. Covered by a new Playwright spec driving a scripted sub-agent that blocks on an approval: the ancestor badge, the monitor state, the notification contents, click-through navigation and answering, single-notification dedup, the bounded refresh, plus direct assertions on the hoisting priority and the cycle guard. The scripted child waits before raising its approval — thread activity is broadcast live and never replayed on connect, so firing immediately raced the browser's WebSocket. The replay harness tracks pending approvals by id rather than in a single slot, so a parent and a child blocked at the same time cannot overwrite each other's confirmation target. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhKuYcqLiesiQEDJJ1gBCY
marmeladema
force-pushed
the
claude/sub-agent-approval-requests-vffexn
branch
from
July 25, 2026 11:08
b1f71c1 to
70611b6
Compare
This was referenced Jul 25, 2026
marmeladema
pushed a commit
that referenced
this pull request
Jul 25, 2026
An approval and a server request both stop a turn until the user answers. The browser only ever treated the first as blocking: a thread waiting on a server request fell through to the generic active-turn branch, rendered as `o` — indistinguishable from merely busy — ranked below an approval when a row had to represent several descendants, never reached "Needs approval" on a sub-agent card, and fired no notification at all. A sub-agent blocked on a question was exactly as invisible as the case fixed in #125, on the path next to it. Codex itself already blurs the split: MCP tool approvals arrive as `requestUserInput` server requests and get promoted to approval cards by a marker. The distinction is a protocol artifact, not something worth surfacing, so activity ranking, the sidebar glyph, the sub-agent monitor and notifications now key off "waits on the user" rather than "is an approval". Card rendering deliberately stays per-kind — a decision has fixed choices, a server request has a per-method form — because that part was never duplicated. Names follow the behaviour. State that now holds both kinds is renamed away from "approval" (`activity-waiting`, `waitingNotificationKey`, `notifiedRequests`, `waitingRequestRowById`, and so on) so it stops claiming to be approval-only; `pendingApprovals` and `answeredApprovals` keep their names, because they still hold only approvals. The card label becomes "Waiting on you", which is true of both. Writing the test for it surfaced a second gap: the waiting state has to clear when the user acts, not when the harness confirms. Answering an approval broadcasts `ApprovalResolved`, but a server request's resolved event comes from the harness on its own schedule and may never come, so the sidebar kept demanding attention for something already answered. The browser now clears its own waiting state on send. The activity summary also stops being the raw JSON-RPC method name, which was reaching the notification body. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhKuYcqLiesiQEDJJ1gBCY
marmeladema
pushed a commit
that referenced
this pull request
Jul 25, 2026
An approval and a server request both stop a turn until the user answers. The browser only ever treated the first as blocking: a thread waiting on a server request fell through to the generic active-turn branch, rendered as `o` — indistinguishable from merely busy — ranked below an approval when a row had to represent several descendants, never reached "Needs approval" on a sub-agent card, and fired no notification at all. A sub-agent blocked on a question was exactly as invisible as the case fixed in #125, on the path next to it. Codex itself already blurs the split: MCP tool approvals arrive as `requestUserInput` server requests and get promoted to approval cards by a marker. The distinction is a protocol artifact, not something worth surfacing, so activity ranking, the sidebar glyph, the sub-agent monitor and notifications now key off "waits on the user" rather than "is an approval". Card rendering deliberately stays per-kind — a decision has fixed choices, a server request has a per-method form — because that part was never duplicated. Names follow the behaviour. State that now holds both kinds is renamed away from "approval" (`activity-waiting`, `waitingNotificationKey`, `notifiedRequests`, `waitingRequestRowById`, and so on) so it stops claiming to be approval-only; `pendingApprovals` and `answeredApprovals` keep their names, because they still hold only approvals. The card label becomes "Waiting on you", which is true of both. Writing the test for it surfaced a second gap: the waiting state has to clear when the user acts, not when the harness confirms. Answering an approval broadcasts `ApprovalResolved`, but a server request's resolved event comes from the harness on its own schedule and may never come, so the sidebar kept demanding attention for something already answered. The browser now clears its own waiting state on send. The activity summary also stops being the raw JSON-RPC method name, which was reaching the notification body. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhKuYcqLiesiQEDJJ1gBCY
marmeladema
pushed a commit
that referenced
this pull request
Jul 25, 2026
An approval and a server request both stop a turn until the user answers. The browser only ever treated the first as blocking: a thread waiting on a server request fell through to the generic active-turn branch, rendered as `o` — indistinguishable from merely busy — ranked below an approval when a row had to represent several descendants, never reached "Needs approval" on a sub-agent card, and fired no notification at all. A sub-agent blocked on a question was exactly as invisible as the case fixed in #125, on the path next to it. Codex itself already blurs the split: MCP tool approvals arrive as `requestUserInput` server requests and get promoted to approval cards by a marker. The distinction is a protocol artifact, not something worth surfacing, so activity ranking, the sidebar glyph, the sub-agent monitor and notifications now key off "waits on the user" rather than "is an approval". Card rendering deliberately stays per-kind — a decision has fixed choices, a server request has a per-method form — because that part was never duplicated. Names follow the behaviour. State that now holds both kinds is renamed away from "approval" (`activity-waiting`, `waitingNotificationKey`, `notifiedRequests`, `waitingRequestRowById`, and so on) so it stops claiming to be approval-only; `pendingApprovals` and `answeredApprovals` keep their names, because they still hold only approvals. The card label becomes "Waiting on you", which is true of both. Writing the test for it surfaced a second gap: the waiting state has to clear when the user acts, not when the harness confirms. Answering an approval broadcasts `ApprovalResolved`, but a server request's resolved event comes from the harness on its own schedule and may never come, so the sidebar kept demanding attention for something already answered. The browser now clears its own waiting state on send. The activity summary also stops being the raw JSON-RPC method name, which was reaching the notification body. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhKuYcqLiesiQEDJJ1gBCY
marmeladema
pushed a commit
that referenced
this pull request
Jul 25, 2026
An approval and a server request both stop a turn until the user answers. The browser only ever treated the first as blocking: a thread waiting on a server request fell through to the generic active-turn branch, rendered as `o` — indistinguishable from merely busy — ranked below an approval when a row had to represent several descendants, never reached "Needs approval" on a sub-agent card, and fired no notification at all. A sub-agent blocked on a question was exactly as invisible as the case fixed in #125, on the path next to it. Codex itself already blurs the split: MCP tool approvals arrive as `requestUserInput` server requests and get promoted to approval cards by a marker. The distinction is a protocol artifact, not something worth surfacing, so activity ranking, the sidebar glyph, the sub-agent monitor and notifications now key off "waits on the user" rather than "is an approval". Card rendering deliberately stays per-kind — a decision has fixed choices, a server request has a per-method form — because that part was never duplicated. Names follow the behaviour. State that now holds both kinds is renamed away from "approval" (`activity-waiting`, `waitingNotificationKey`, `notifiedRequests`, `waitingRequestRowById`, and so on) so it stops claiming to be approval-only; `pendingApprovals` and `answeredApprovals` keep their names, because they still hold only approvals. The card label becomes "Waiting on you", which is true of both. Writing the test for it surfaced a second gap: the waiting state has to clear when the user acts, not when the harness confirms. Answering an approval broadcasts `ApprovalResolved`, but a server request's resolved event comes from the harness on its own schedule and may never come, so the sidebar kept demanding attention for something already answered. The browser now clears its own waiting state on send. The activity summary also stops being the raw JSON-RPC method name, which was reaching the notification body. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhKuYcqLiesiQEDJJ1gBCY
marmeladema
pushed a commit
that referenced
this pull request
Jul 25, 2026
An approval and a server request both stop a turn until the user answers. The browser only ever treated the first as blocking: a thread waiting on a server request fell through to the generic active-turn branch, rendered as `o` — indistinguishable from merely busy — ranked below an approval when a row had to represent several descendants, never reached "Needs approval" on a sub-agent card, and fired no notification at all. A sub-agent blocked on a question was exactly as invisible as the case fixed in #125, on the path next to it. Codex itself already blurs the split: MCP tool approvals arrive as `requestUserInput` server requests and get promoted to approval cards by a marker. The distinction is a protocol artifact, not something worth surfacing, so activity ranking, the sidebar glyph, the sub-agent monitor and notifications now key off "waits on the user" rather than "is an approval". Card rendering deliberately stays per-kind — a decision has fixed choices, a server request has a per-method form — because that part was never duplicated. Names follow the behaviour. State that now holds both kinds is renamed away from "approval" (`activity-waiting`, `waitingNotificationKey`, `notifiedRequests`, `waitingRequestRowById`, and so on) so it stops claiming to be approval-only; `pendingApprovals` and `answeredApprovals` keep their names, because they still hold only approvals. The card label becomes "Waiting on you", which is true of both. Writing the test for it surfaced a second gap: the waiting state has to clear when the user acts, not when the harness confirms. Answering an approval broadcasts `ApprovalResolved`, but a server request's resolved event comes from the harness on its own schedule and may never come, so the sidebar kept demanding attention for something already answered. The browser now clears its own waiting state on send. The activity summary also stops being the raw JSON-RPC method name, which was reaching the notification body. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhKuYcqLiesiQEDJJ1gBCY
marmeladema
pushed a commit
that referenced
this pull request
Jul 25, 2026
An approval and a server request both stop a turn until the user answers. The browser only ever treated the first as blocking: a thread waiting on a server request fell through to the generic active-turn branch, rendered as `o` — indistinguishable from merely busy — ranked below an approval when a row had to represent several descendants, never reached "Needs approval" on a sub-agent card, and fired no notification at all. A sub-agent blocked on a question was exactly as invisible as the case fixed in #125, on the path next to it. Codex itself already blurs the split: MCP tool approvals arrive as `requestUserInput` server requests and get promoted to approval cards by a marker. The distinction is a protocol artifact, not something worth surfacing, so activity ranking, the sidebar glyph, the sub-agent monitor and notifications now key off "waits on the user" rather than "is an approval". Card rendering deliberately stays per-kind — a decision has fixed choices, a server request has a per-method form — because that part was never duplicated. Names follow the behaviour. State that now holds both kinds is renamed away from "approval" (`activity-waiting`, `waitingNotificationKey`, `notifiedRequests`, `waitingRequestRowById`, and so on) so it stops claiming to be approval-only; `pendingApprovals` and `answeredApprovals` keep their names, because they still hold only approvals. The card label becomes "Waiting on you", which is true of both. Writing the test for it surfaced a second gap: the waiting state has to clear when the user acts, not when the harness confirms. Answering an approval broadcasts `ApprovalResolved`, but a server request's resolved event comes from the harness on its own schedule and may never come, so the sidebar kept demanding attention for something already answered. The browser now clears its own waiting state on send. The activity summary also stops being the raw JSON-RPC method name, which was reaching the notification body. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhKuYcqLiesiQEDJJ1gBCY
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
2 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.
Approvals already routed correctly into a managed sub-agent thread and
were answerable once you opened it, but nothing told you it was blocked.
A managed child is deliberately absent from the sidebar, so every browser
affordance keyed to a thread id quietly no-opped for it: no status symbol
on any row, a notification titled with a bare id prefix, and a click that
dead-ended on "Approval thread is not in the current project list." The
child just looked hung.
Three fixes:
(which include managed sub-agents) instead of from the rendered sidebar
row. This alone restores notification naming and click-to-navigate.
a row, showing the most urgent state among the row and its hidden
descendants (approval > error > active turn), naming the originating
child in the tooltip and marking the state as hoisted. Ancestor and
descendant walks are bounded so cyclic ownership terminates.
"needs approval" state: an approval also marks the turn active, so a
blocked child previously rendered identically to a busy one. A card now
represents its whole owned subtree, since nested descendants are not
listed separately.
Because the server materializes a child on its own, activity can arrive
for a thread the browser has never listed; it now refreshes its cached
lists before naming or navigating to such a thread, so the first approval
from a brand-new child is still attributable.
Covered by a new Playwright spec driving a scripted sub-agent that blocks
on an approval: the ancestor badge, the monitor state, the notification
contents, click-through navigation and answering, plus direct assertions
on the hoisting priority and the cycle guard. The scripted child waits
before raising its approval — thread activity is broadcast live and never
replayed on connect, so firing immediately raced the browser's WebSocket.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01NhKuYcqLiesiQEDJJ1gBCY
Summary by CodeRabbit