fix(chat): prevent ask_user question from shadowing sandbox access prompt#3662
Merged
Kitenite merged 12 commits intoApr 25, 2026
Merged
Conversation
Pull the inline QuestionStatusDescription component and its QUESTION_STATUS_CONFIG lookup into a shared ToolStatusBadge with a variant prop (default / success / danger) so other tool call rows can reuse consistent badge styling without duplicating the pattern.
…g sandbox access prompt The harness sets displayState.pendingQuestion when ask_user fires but only clears it on agent_end — never when the user answers. If an agent asks a question and then immediately calls request_access, the already- answered question stays in displayState.pendingQuestion and shadows the new sandbox prompt. The agent then waits for the sandbox prompt to be answered, but the UI never shows it, causing a deadlock. Fix: track answeredQuestionIds per session. In getDisplayState, filter out any harness pendingQuestion whose ID has already been answered this turn so the sandbox prompt can surface correctly. Clear the set on agent_start/agent_end. Also threads the sandbox request reason through as a top-level description on the pending question object so UIs can display it as context beneath the question header.
Replace the generic INPUT/OUTPUT block for request_access with a dedicated RequestSandboxAccessToolCall component that matches the question tool's visual language: - FolderLock icon with "Request Access" title - Status badge in the hint slot: AWAITING RESPONSE / ACCESS GRANTED / ACCESS DENIED / CANCELLED / ERROR (red only on error) - Not expandable/clickable while awaiting a response - After resolution: path and reason as muted context lines, then the access decision as the primary answer line Also surfaces the request reason as context below the question text in QuestionInputOverlay, using a new optional description field on the pending question shape. Alias request_sandbox_access → request_access so either tool name routes to the same component.
Contributor
📝 WalkthroughWalkthroughPer-turn answered-question tracking was added to runtime/session state; pending question models now allow an optional description; new ToolStatusBadge and RequestSandboxAccessToolCall UI components were added and ToolCallBlock routing/normalization updated; server and host runtime flows were adjusted to record answered IDs and avoid showing already-answered pending questions. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as Chat UI
participant Service
participant Harness
participant Runtime as RuntimeSession
User->>UI: submit answer to ask_user (questionId)
UI->>Service: question.respond(questionId, payload)
Service->>Runtime: mark questionId in answeredQuestionIds
Service->>Harness: await respondToQuestion(...)
alt harness success
Harness-->>Service: success/result
Service->>Runtime: clear pendingSandboxQuestion if matched
else harness error
Harness-->>Service: throws
Service->>Runtime: rollback answeredQuestionIds and restore pendingSandboxQuestion (if cleared)
end
Service->>Runtime: compute display state (filter pendingQuestion if answered)
Service-->>UI: return updated display state (may include sandbox access request with description)
UI-->>User: render updated UI (ToolStatusBadge / RequestSandboxAccess)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 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 |
Greptile SummaryThis PR fixes two related deadlock/regression bugs in the chat UI where an answered Key changes:
Confidence Score: 4/5Safe to merge; the core deadlock fix is correct and consistently applied to both runtimes. One minor logic bug and one style note are the only remaining items. The
|
| Filename | Overview |
|---|---|
| packages/chat/src/server/trpc/utils/runtime/runtime.ts | Adds answeredQuestionIds: Set<string> to RuntimeSession; clears on agent_start / agent_end. Core fix is correct and symmetric. |
| packages/chat/src/server/trpc/service.ts | Initialises answeredQuestionIds, populates it on question respond, and filters pendingQuestion in getDisplayState. Logic is sound and mirrors the host-service implementation. |
| packages/host-service/src/runtime/chat/chat.ts | Electron runtime: same answeredQuestionIds fix applied; also adds description field for sandbox reason and makes ChatPendingQuestionOption.description optional. Consistent with the web runtime. |
| apps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/RequestSandboxAccessToolCall/RequestSandboxAccessToolCall.tsx | New dedicated UI component for request_access tool call. Minor: hasContext uses ?? instead of ` |
| apps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/ToolStatusBadge/ToolStatusBadge.tsx | New shared badge component extracted from AskUserQuestionToolCall. Clean refactor with variant support. |
| apps/desktop/src/renderer/components/Chat/ChatInterface/utils/tool-helpers.ts | Adds request_access → request_access canonical alias and keeps request_sandbox_access → request_access for backwards compatibility. Correct. |
| apps/desktop/src/renderer/components/Chat/ChatInterface/components/ChatInputFooter/components/QuestionInputOverlay/QuestionInputOverlay.tsx | Adds optional description field rendering beneath the question text. Used to surface the sandbox access reason in the approval overlay. |
| packages/chat/src/server/trpc/utils/runtime/runtime.test.ts | Test helpers updated to include answeredQuestionIds: new Set() in the default runtime fixture. No logic change. |
Sequence Diagram
sequenceDiagram
participant Agent
participant Harness
participant Runtime as RuntimeSession
participant UI as getDisplayState
Agent->>Harness: ask_user_question(id=Q1)
Harness->>UI: pendingQuestion = {id: Q1}
UI->>UI: show Q1 overlay
UI->>Runtime: respondToQuestion(Q1)
Runtime->>Runtime: answeredQuestionIds.add(Q1)
Runtime->>Harness: respondToQuestion(Q1)
Note over Harness: harness still holds pendingQuestion=Q1 (cleared only on agent_end)
Agent->>Harness: request_access(path, reason)
Harness->>Runtime: sandbox_access_request event
Runtime->>Runtime: pendingSandboxQuestion = {id: Q2, path, reason}
UI->>Runtime: getDisplayState()
Runtime->>Runtime: Q1 in answeredQuestionIds → harnessPendingQuestion = null
Runtime->>UI: pendingQuestion = sandboxPendingQuestion (Q2)
UI->>UI: show sandbox approval overlay
UI->>Runtime: respondToQuestion(Q2, Yes)
Runtime->>Runtime: answeredQuestionIds.add(Q2)
Runtime->>Runtime: pendingSandboxQuestion = null
Runtime->>Harness: respondToQuestion(Q2)
Harness->>Agent: access granted / denied
Harness->>Runtime: agent_end event
Runtime->>Runtime: answeredQuestionIds.clear()
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/RequestSandboxAccessToolCall/RequestSandboxAccessToolCall.tsx
Line: 79
Comment:
**`??` instead of `||` for `hasContext` check**
`requestedPath ?? reason` uses nullish coalescing, which only falls through to `reason` when `requestedPath` is `null` or `undefined`. If `requestedPath` happens to be an empty string `""` (e.g. the tool was called with `args.path = ""`), the expression short-circuits to `Boolean("")` → `false`, silently hiding `reason` even when it has content.
The intent is "either path or reason is present", which requires logical OR:
```suggestion
const hasContext = Boolean(requestedPath || reason);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/RequestSandboxAccessToolCall/RequestSandboxAccessToolCall.tsx
Line: 82-88
Comment:
**`isPending={false}` hardcoded suppresses spinner for the pending state**
`ToolCallRow` renders a `BrailleSpinner` in the icon slot when `isPending={true}` and the row is not hovered, giving users a visual cue that work is in progress. By hardcoding `isPending={false}`, the icon always stays as `FolderLockIcon` — even while the sandbox request is waiting for a response — forfeiting the built-in loading indicator.
The component's own `ToolStatusBadge` ("Awaiting Response" + clock icon) does convey the pending state, so this isn't a correctness bug, but it diverges from the rest of the tool call rows that use `isPending` to drive the spinner. Consider passing `isPending={isPending}` to stay consistent with the pattern:
```suggestion
<ToolCallRow
icon={FolderLockIcon}
isPending={isPending}
isError={false}
title="Request Access"
description={statusBadge}
>
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(desktop): custom UI for request_acc..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| const isPending = status === "pending"; | ||
| const isCancelledOrError = status === "cancelled" || status === "error"; | ||
| const hasContext = Boolean(requestedPath ?? reason); |
There was a problem hiding this comment.
?? instead of || for hasContext check
requestedPath ?? reason uses nullish coalescing, which only falls through to reason when requestedPath is null or undefined. If requestedPath happens to be an empty string "" (e.g. the tool was called with args.path = ""), the expression short-circuits to Boolean("") → false, silently hiding reason even when it has content.
The intent is "either path or reason is present", which requires logical OR:
Suggested change
| const hasContext = Boolean(requestedPath ?? reason); | |
| const hasContext = Boolean(requestedPath || reason); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/RequestSandboxAccessToolCall/RequestSandboxAccessToolCall.tsx
Line: 79
Comment:
**`??` instead of `||` for `hasContext` check**
`requestedPath ?? reason` uses nullish coalescing, which only falls through to `reason` when `requestedPath` is `null` or `undefined`. If `requestedPath` happens to be an empty string `""` (e.g. the tool was called with `args.path = ""`), the expression short-circuits to `Boolean("")` → `false`, silently hiding `reason` even when it has content.
The intent is "either path or reason is present", which requires logical OR:
```suggestion
const hasContext = Boolean(requestedPath || reason);
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+82
to
+88
| <ToolCallRow | ||
| icon={FolderLockIcon} | ||
| isPending={false} | ||
| isError={false} | ||
| title="Request Access" | ||
| description={statusBadge} | ||
| > |
There was a problem hiding this comment.
isPending={false} hardcoded suppresses spinner for the pending state
ToolCallRow renders a BrailleSpinner in the icon slot when isPending={true} and the row is not hovered, giving users a visual cue that work is in progress. By hardcoding isPending={false}, the icon always stays as FolderLockIcon — even while the sandbox request is waiting for a response — forfeiting the built-in loading indicator.
The component's own ToolStatusBadge ("Awaiting Response" + clock icon) does convey the pending state, so this isn't a correctness bug, but it diverges from the rest of the tool call rows that use isPending to drive the spinner. Consider passing isPending={isPending} to stay consistent with the pattern:
Suggested change
| <ToolCallRow | |
| icon={FolderLockIcon} | |
| isPending={false} | |
| isError={false} | |
| title="Request Access" | |
| description={statusBadge} | |
| > | |
| <ToolCallRow | |
| icon={FolderLockIcon} | |
| isPending={isPending} | |
| isError={false} | |
| title="Request Access" | |
| description={statusBadge} | |
| > |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/RequestSandboxAccessToolCall/RequestSandboxAccessToolCall.tsx
Line: 82-88
Comment:
**`isPending={false}` hardcoded suppresses spinner for the pending state**
`ToolCallRow` renders a `BrailleSpinner` in the icon slot when `isPending={true}` and the row is not hovered, giving users a visual cue that work is in progress. By hardcoding `isPending={false}`, the icon always stays as `FolderLockIcon` — even while the sandbox request is waiting for a response — forfeiting the built-in loading indicator.
The component's own `ToolStatusBadge` ("Awaiting Response" + clock icon) does convey the pending state, so this isn't a correctness bug, but it diverges from the rest of the tool call rows that use `isPending` to drive the spinner. Consider passing `isPending={isPending}` to stay consistent with the pattern:
```suggestion
<ToolCallRow
icon={FolderLockIcon}
isPending={isPending}
isError={false}
title="Request Access"
description={statusBadge}
>
```
How can I resolve this? If you propose a fix, please make it concise.![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/host-service/src/runtime/chat/chat.ts (1)
584-591:⚠️ Potential issue | 🟠 MajorRoll back optimistic question state if the harness response fails.
Line 584 marks the question as answered before
respondToQuestionsucceeds. If the harness rejects the response, subsequent display-state reads will hide the still-unanswered prompt; sandbox prompts are also cleared on Line 588.Proposed fix
- runtime.answeredQuestionIds.add(input.payload.questionId); - if ( - runtime.pendingSandboxQuestion?.questionId === input.payload.questionId - ) { - runtime.pendingSandboxQuestion = null; - } - - return runtime.harness.respondToQuestion(input.payload); + const questionId = input.payload.questionId; + const wasAlreadyAnswered = runtime.answeredQuestionIds.has(questionId); + const previousSandboxQuestion = runtime.pendingSandboxQuestion; + const clearsSandboxQuestion = + previousSandboxQuestion?.questionId === questionId; + + runtime.answeredQuestionIds.add(questionId); + if (clearsSandboxQuestion) { + runtime.pendingSandboxQuestion = null; + } + + try { + return await runtime.harness.respondToQuestion(input.payload); + } catch (error) { + if (!wasAlreadyAnswered) { + runtime.answeredQuestionIds.delete(questionId); + } + if (clearsSandboxQuestion && runtime.pendingSandboxQuestion === null) { + runtime.pendingSandboxQuestion = previousSandboxQuestion; + } + throw error; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/runtime/chat/chat.ts` around lines 584 - 591, The code optimistically marks a question answered and clears pendingSandboxQuestion before calling runtime.harness.respondToQuestion, so if respondToQuestion rejects the UI state becomes inconsistent; wrap the respondToQuestion call in a try/catch and only add to runtime.answeredQuestionIds and clear runtime.pendingSandboxQuestion after a successful await, or if you must keep the optimistic update, revert those changes in the catch block and rethrow the error. Target the block that manipulates runtime.answeredQuestionIds, runtime.pendingSandboxQuestion and the call to runtime.harness.respondToQuestion to implement the rollback-on-failure behavior.packages/chat/src/server/trpc/service.ts (1)
360-367:⚠️ Potential issue | 🟠 MajorRoll back answered-question state when
respondToQuestionrejects.Line 360 hides the question optimistically, but if
runtime.harness.respondToQuestionfails on Line 367, the UI can no longer show the unanswered prompt; matching sandbox prompts are also cleared before success.Proposed fix
- runtime.answeredQuestionIds.add(input.payload.questionId); - if ( - runtime.pendingSandboxQuestion?.questionId === - input.payload.questionId - ) { - runtime.pendingSandboxQuestion = null; - } - return runtime.harness.respondToQuestion(input.payload); + const questionId = input.payload.questionId; + const wasAlreadyAnswered = + runtime.answeredQuestionIds.has(questionId); + const previousSandboxQuestion = runtime.pendingSandboxQuestion; + const clearsSandboxQuestion = + previousSandboxQuestion?.questionId === questionId; + + runtime.answeredQuestionIds.add(questionId); + if (clearsSandboxQuestion) { + runtime.pendingSandboxQuestion = null; + } + + try { + return await runtime.harness.respondToQuestion(input.payload); + } catch (error) { + if (!wasAlreadyAnswered) { + runtime.answeredQuestionIds.delete(questionId); + } + if ( + clearsSandboxQuestion && + runtime.pendingSandboxQuestion === null + ) { + runtime.pendingSandboxQuestion = previousSandboxQuestion; + } + throw error; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chat/src/server/trpc/service.ts` around lines 360 - 367, The optimistic hide of the question must be reverted if respondToQuestion fails: wrap the call to runtime.harness.respondToQuestion(input.payload) in a try/catch, and in the catch remove input.payload.questionId from runtime.answeredQuestionIds and restore runtime.pendingSandboxQuestion to the previous value if it was cleared (compare by questionId) before re-throwing the error; ensure you capture the previous pendingSandboxQuestion value into a local variable before clearing it so it can be restored on failure.packages/chat/src/server/trpc/utils/runtime/runtime.test.ts (1)
319-345:⚠️ Potential issue | 🔴 CriticalAdd the required
answeredQuestionIdsfield to the restart fixture.The
RuntimeSessionfixture at lines 319-345 is missing theansweredQuestionIds: Set<string>field defined in the interface.Proposed fix
mcpManualStatuses: new Map(), lastErrorMessage: "stale error", pendingSandboxQuestion: null, + answeredQuestionIds: new Set(), cwd: "/tmp", };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chat/src/server/trpc/utils/runtime/runtime.test.ts` around lines 319 - 345, The RuntimeSession test fixture named runtime is missing the required answeredQuestionIds Set; update the runtime object (in runtime.test.ts) to include the answeredQuestionIds property initialized as a Set<string> (e.g., new Set()) so it satisfies the RuntimeSession interface; ensure the property name is exactly answeredQuestionIds and add it alongside the existing fields like sessionId, harness, mcpManager, hookManager, mcpManualStatuses, lastErrorMessage, pendingSandboxQuestion, and cwd.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/AskUserQuestionToolCall/AskUserQuestionToolCall.tsx (1)
250-255: Use semantic variants for answered/cancelled badges.Answered and cancelled currently render with the same default color as pending, even though
ToolStatusBadgesupports state-specific variants.🎨 Proposed visual consistency tweak
isPending ? ( <ToolStatusBadge icon={ClockIcon} label="Awaiting Response" /> ) : isAnswered ? ( - <ToolStatusBadge icon={CheckIcon} label="Answered" /> + <ToolStatusBadge icon={CheckIcon} label="Answered" variant="success" /> ) : isCancelled || isCancelledByError || isCancelledByStop ? ( - <ToolStatusBadge icon={XIcon} label="Cancelled" /> + <ToolStatusBadge icon={XIcon} label="Cancelled" variant="danger" /> ) : undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/AskUserQuestionToolCall/AskUserQuestionToolCall.tsx` around lines 250 - 255, The status badges currently all use the default styling; update the ToolStatusBadge usages in AskUserQuestionToolCall so answered and cancelled states pass semantic variant props (e.g., variant="success" for the CheckIcon/Answered case and variant="danger" or "error" for the XIcon/Cancelled case) while keeping the pending case as-is; specifically modify the branches that check isPending, isAnswered, and the isCancelled || isCancelledByError || isCancelledByStop condition to supply the appropriate variant prop to ToolStatusBadge so visual state maps to semantic meaning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/RequestSandboxAccessToolCall/RequestSandboxAccessToolCall.tsx`:
- Around line 69-79: The code treats a trimmed-empty args.path as a valid value,
causing requestedPath ?? reason to pick an empty string and hide the reason;
change the requestedPath initialization so trimmed empty strings become null
(e.g., set requestedPath = typeof args.path === "string" ? args.path.trim() ||
null : null) and keep hasContext as Boolean(requestedPath ?? reason) (or
equivalently use (requestedPath || reason) in hasContext) so a blank path no
longer masks a provided reason; update references to requestedPath and
hasContext accordingly in RequestSandboxAccessToolCall.
- Around line 59-60: The code only inspects result.content and returns
toAccessDecision(content) or "error", so outputs normalized by getResult(part)
into result.text (plain string responses) are ignored; update the logic in
RequestSandboxAccessToolCall (where getResult(part) is used) to fallback to
result.text when result.content is falsy (e.g., const content = typeof
result.content === "string" ? result.content.trim() : (typeof result.text ===
"string" ? result.text.trim() : "")) before calling toAccessDecision(content) so
plain string outputs are handled correctly.
In
`@apps/desktop/src/renderer/components/Chat/ChatInterface/utils/tool-helpers.ts`:
- Around line 32-33: Update the expectation in the normalization unit test to
reflect the new canonical alias: replace the asserted string
"request_sandbox_access" with "request_access" in the test for the normalization
logic in tool-helpers.test.ts so it matches the mapping in tool-helpers.ts where
request_sandbox_access is normalized to request_access.
---
Outside diff comments:
In `@packages/chat/src/server/trpc/service.ts`:
- Around line 360-367: The optimistic hide of the question must be reverted if
respondToQuestion fails: wrap the call to
runtime.harness.respondToQuestion(input.payload) in a try/catch, and in the
catch remove input.payload.questionId from runtime.answeredQuestionIds and
restore runtime.pendingSandboxQuestion to the previous value if it was cleared
(compare by questionId) before re-throwing the error; ensure you capture the
previous pendingSandboxQuestion value into a local variable before clearing it
so it can be restored on failure.
In `@packages/chat/src/server/trpc/utils/runtime/runtime.test.ts`:
- Around line 319-345: The RuntimeSession test fixture named runtime is missing
the required answeredQuestionIds Set; update the runtime object (in
runtime.test.ts) to include the answeredQuestionIds property initialized as a
Set<string> (e.g., new Set()) so it satisfies the RuntimeSession interface;
ensure the property name is exactly answeredQuestionIds and add it alongside the
existing fields like sessionId, harness, mcpManager, hookManager,
mcpManualStatuses, lastErrorMessage, pendingSandboxQuestion, and cwd.
In `@packages/host-service/src/runtime/chat/chat.ts`:
- Around line 584-591: The code optimistically marks a question answered and
clears pendingSandboxQuestion before calling runtime.harness.respondToQuestion,
so if respondToQuestion rejects the UI state becomes inconsistent; wrap the
respondToQuestion call in a try/catch and only add to
runtime.answeredQuestionIds and clear runtime.pendingSandboxQuestion after a
successful await, or if you must keep the optimistic update, revert those
changes in the catch block and rethrow the error. Target the block that
manipulates runtime.answeredQuestionIds, runtime.pendingSandboxQuestion and the
call to runtime.harness.respondToQuestion to implement the rollback-on-failure
behavior.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/AskUserQuestionToolCall/AskUserQuestionToolCall.tsx`:
- Around line 250-255: The status badges currently all use the default styling;
update the ToolStatusBadge usages in AskUserQuestionToolCall so answered and
cancelled states pass semantic variant props (e.g., variant="success" for the
CheckIcon/Answered case and variant="danger" or "error" for the XIcon/Cancelled
case) while keeping the pending case as-is; specifically modify the branches
that check isPending, isAnswered, and the isCancelled || isCancelledByError ||
isCancelledByStop condition to supply the appropriate variant prop to
ToolStatusBadge so visual state maps to semantic meaning.
🪄 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: dc633df5-6f26-46d3-a376-c323735e72ae
📒 Files selected for processing (13)
apps/desktop/src/renderer/components/Chat/ChatInterface/components/ChatInputFooter/ChatInputFooter.tsxapps/desktop/src/renderer/components/Chat/ChatInterface/components/ChatInputFooter/components/QuestionInputOverlay/QuestionInputOverlay.tsxapps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/ToolCallBlock.tsxapps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/AskUserQuestionToolCall/AskUserQuestionToolCall.tsxapps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/RequestSandboxAccessToolCall/RequestSandboxAccessToolCall.tsxapps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/RequestSandboxAccessToolCall/index.tsapps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/ToolStatusBadge/ToolStatusBadge.tsxapps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/ToolStatusBadge/index.tsapps/desktop/src/renderer/components/Chat/ChatInterface/utils/tool-helpers.tspackages/chat/src/server/trpc/service.tspackages/chat/src/server/trpc/utils/runtime/runtime.test.tspackages/chat/src/server/trpc/utils/runtime/runtime.tspackages/host-service/src/runtime/chat/chat.ts
Comment on lines
+32
to
+33
| request_access: "request_access", | ||
| request_sandbox_access: "request_access", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify stale expectations for request_sandbox_access normalization.
# Expected after the fix: any normalizeToolName("request_sandbox_access") expectation should be "request_access".
rg -n -C3 'request_sandbox_access' --iglob '*tool-helpers.test.ts'Repository: superset-sh/superset
Length of output: 1168
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1. Verify the implementation shows the mapping
echo "=== Implementation in tool-helpers.ts (lines 32-33) ==="
sed -n '30,35p' apps/desktop/src/renderer/components/Chat/ChatInterface/utils/tool-helpers.ts
# 2. Check for all references to this mapping in tests
echo ""
echo "=== All test expectations for request_access in tool-helpers.test.ts ==="
rg -n 'request_access' --iglob '*tool-helpers.test.ts'Repository: superset-sh/superset
Length of output: 419
Update the normalization test for the new canonical alias.
The implementation now maps request_sandbox_access to request_access (line 33), but the test at lines 20–22 of tool-helpers.test.ts still expects "request_sandbox_access". Update the test to expect "request_access" instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/components/Chat/ChatInterface/utils/tool-helpers.ts`
around lines 32 - 33, Update the expectation in the normalization unit test to
reflect the new canonical alias: replace the asserted string
"request_sandbox_access" with "request_access" in the test for the normalization
logic in tool-helpers.test.ts so it matches the mapping in tool-helpers.ts where
request_sandbox_access is normalized to request_access.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 13 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="packages/chat/src/server/trpc/service.ts">
<violation number="1" location="packages/chat/src/server/trpc/service.ts:360">
P2: Marking the question as answered before `respondToQuestion` succeeds can hide a still-pending question after a failed response.</violation>
</file>
<file name="packages/host-service/src/runtime/chat/chat.ts">
<violation number="1" location="packages/host-service/src/runtime/chat/chat.ts:584">
P2: Question state is marked answered before `respondToQuestion` succeeds, so a failed response can hide an actually pending prompt and block recovery.</violation>
</file>
<file name="apps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/RequestSandboxAccessToolCall/RequestSandboxAccessToolCall.tsx">
<violation number="1" location="apps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/RequestSandboxAccessToolCall/RequestSandboxAccessToolCall.tsx:59">
P2: Only `result.content` is checked here, but tool results that are plain (non-JSON) strings get normalized into `{ text: output }` by `getResult`. When the sandbox access response comes back as `result.text` instead of `result.content`, this evaluates to `""` and `toAccessDecision` returns `null`, causing the status to incorrectly fall through to `"error"`. Consider also checking `result.text` as a fallback.</violation>
<violation number="2" location="apps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/RequestSandboxAccessToolCall/RequestSandboxAccessToolCall.tsx:79">
P2: Using nullish coalescing here hides `reason` when `requestedPath` is an empty string.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…uest_access UI
- Use || for hasContext so empty string args.path doesn't hide reason
- Fall back to result.text when result.content is absent, since getResult wraps
non-JSON string outputs as { text }
- Update normalizeToolName test to match the request_sandbox_access → request_access alias
…at-request-access
…at-request-access
Contributor
There was a problem hiding this comment.
1 issue found across 5 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="packages/host-service/src/runtime/chat/chat.ts">
<violation number="1" location="packages/host-service/src/runtime/chat/chat.ts:584">
P2: `answeredQuestionIds` is updated only after awaiting `respondToQuestion`, creating a race window where stale harness pending questions can still shadow sandbox prompts.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/chat/src/server/trpc/service.ts (2)
225-238: Reason now shown twice in the approval overlay.
descriptionat the top level renders the sandbox reason (per the PR's "show reason beneath the question" change), but the "Yes" option'sdescriptionstill embeds the same reason asAllow access. Reason: ${reason}. The user will see the reason both above and inside the Yes button. Consider dropping it from the option description.♻️ Proposed refactor
const sandboxPendingQuestion = runtime.pendingSandboxQuestion ? { questionId: runtime.pendingSandboxQuestion.questionId, question: `Grant sandbox access to "${runtime.pendingSandboxQuestion.path}"?`, description: runtime.pendingSandboxQuestion.reason, options: [ - { - label: "Yes", - description: `Allow access. Reason: ${runtime.pendingSandboxQuestion.reason}`, - }, + { label: "Yes", description: "Allow access." }, { label: "No", description: "Deny access." }, ], } : null;The same duplication exists in
packages/host-service/src/runtime/chat/chat.tslines 488–501 and should be addressed there too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chat/src/server/trpc/service.ts` around lines 225 - 238, The overlay shows the sandbox reason twice: once in the top-level sandboxPendingQuestion.description and again inside the "Yes" option's description. Update the sandboxPendingQuestion construction (the runtime.pendingSandboxQuestion branch) to remove the embedded reason from the "Yes" option's description (change `Allow access. Reason: ${runtime.pendingSandboxQuestion.reason}` to a shorter label like `Allow access.` or similar) so the reason only appears in sandboxPendingQuestion.description; apply the same change to the equivalent sandboxPendingQuestion construction in the host-service chat runtime where pendingSandboxQuestion is built.
360-385: Rollback logic is correct.The capture-before-mutate pattern handles all the relevant cases:
wasAlreadyAnsweredguard prevents removing a pre-existing entry on failure.pendingSandboxQuestion === nullguard avoids clobbering a newer sandbox event that may have arrived during theawait.- Adding to
answeredQuestionIdsbefore the await is intentional and desirable: it letsgetDisplayStatefilter the answered question while the harness response is in flight, so the UI can surface the sandbox prompt immediately.This logic is duplicated near-verbatim in
packages/host-service/src/runtime/chat/chat.ts(lines 584–605). Acceptable for two independent runtime managers, but a shared helper (e.g.markAnsweredWithRollback(runtime, questionId, fn)) would reduce drift if the logic evolves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chat/src/server/trpc/service.ts` around lines 360 - 385, Extract the duplicated capture-before-mutate rollback logic into a shared helper (e.g., markAnsweredWithRollback(runtime, questionId, fn)) and replace the inlined block in packages/chat/src/server/trpc/service.ts with a call to that helper passing () => runtime.harness.respondToQuestion(input.payload); the helper should: record wasAlreadyAnswered = runtime.answeredQuestionIds.has(questionId) and previousSandboxQuestion = runtime.pendingSandboxQuestion, add questionId to runtime.answeredQuestionIds, clear runtime.pendingSandboxQuestion if it matches questionId, await fn(), and on error if !wasAlreadyAnswered remove the questionId from runtime.answeredQuestionIds and if the clear occurred and runtime.pendingSandboxQuestion is still null restore previousSandboxQuestion, then rethrow the error; add the same call-site replacement in the other duplicate (packages/host-service/src/runtime/chat/chat.ts) so both runtimes share the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/chat/src/server/trpc/service.ts`:
- Around line 225-238: The overlay shows the sandbox reason twice: once in the
top-level sandboxPendingQuestion.description and again inside the "Yes" option's
description. Update the sandboxPendingQuestion construction (the
runtime.pendingSandboxQuestion branch) to remove the embedded reason from the
"Yes" option's description (change `Allow access. Reason:
${runtime.pendingSandboxQuestion.reason}` to a shorter label like `Allow
access.` or similar) so the reason only appears in
sandboxPendingQuestion.description; apply the same change to the equivalent
sandboxPendingQuestion construction in the host-service chat runtime where
pendingSandboxQuestion is built.
- Around line 360-385: Extract the duplicated capture-before-mutate rollback
logic into a shared helper (e.g., markAnsweredWithRollback(runtime, questionId,
fn)) and replace the inlined block in packages/chat/src/server/trpc/service.ts
with a call to that helper passing () =>
runtime.harness.respondToQuestion(input.payload); the helper should: record
wasAlreadyAnswered = runtime.answeredQuestionIds.has(questionId) and
previousSandboxQuestion = runtime.pendingSandboxQuestion, add questionId to
runtime.answeredQuestionIds, clear runtime.pendingSandboxQuestion if it matches
questionId, await fn(), and on error if !wasAlreadyAnswered remove the
questionId from runtime.answeredQuestionIds and if the clear occurred and
runtime.pendingSandboxQuestion is still null restore previousSandboxQuestion,
then rethrow the error; add the same call-site replacement in the other
duplicate (packages/host-service/src/runtime/chat/chat.ts) so both runtimes
share the helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df6c8eb3-b70a-4c34-a4d4-d9abf3b6b1c8
📒 Files selected for processing (2)
packages/chat/src/server/trpc/service.tspackages/host-service/src/runtime/chat/chat.ts
Contributor
There was a problem hiding this comment.
2 issues 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="packages/chat/src/server/trpc/service.ts">
<violation number="1" location="packages/chat/src/server/trpc/service.ts:375">
P2: Optimistic rollback in `question.respond` is race-prone: a failing concurrent call can revert state after another call already succeeded for the same question.</violation>
</file>
<file name="packages/host-service/src/runtime/chat/chat.ts">
<violation number="1" location="packages/host-service/src/runtime/chat/chat.ts:590">
P2: Concurrent `respondToQuestion` calls can race with optimistic rollback, leaving answered-question tracking inconsistent after mixed success/failure.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/chat/src/server/trpc/service.ts (1)
39-86: Solid optimistic-state design; consider de-duplicating with the host-service copy.The rollback logic is well thought out: deduping via
pendingQuestionResponses, deferring throughPromise.resolve().then(...)so theseton line 84 lands before any.catch/.finallyfires, and gating both rollback branches onpendingQuestionResponses.get(questionId) === responsePromisecorrectly prevents rolling back whenagent_start/agent_endhas cleared state mid-flight. ThepreviousSandboxQuestion === nullguard also avoids clobbering a newer sandbox question that arrived during the in-flight respond.That said, this function is duplicated almost verbatim in
packages/host-service/src/runtime/chat/chat.ts(lines 110‑153). Since both files own nearly identicalRuntimeSessionshapes with the same fields used here (harness.respondToQuestion,pendingSandboxQuestion,answeredQuestionIds,pendingQuestionResponses), this would be a good candidate for a small generic helper parameterized over the payload/result types so future fixes only need to land in one place.♻️ Sketch: shared generic helper
Create something like
packages/chat/src/server/trpc/utils/runtime/respond-to-question.ts(or a shared util both packages can import) and parameterize over the types:interface OptimisticQuestionRuntime<TResult> { harness: { respondToQuestion: (payload: { questionId: string }) => Promise<TResult> | TResult }; pendingSandboxQuestion: { questionId: string } | null; answeredQuestionIds: Set<string>; pendingQuestionResponses: Map<string, Promise<TResult>>; } export function respondToQuestionWithOptimisticState< TPayload extends { questionId: string }, TResult, >( runtime: OptimisticQuestionRuntime<TResult>, payload: TPayload, ): Promise<TResult> { // ...same body, using runtime.harness.respondToQuestion(payload) }Then both
service.tsandhost-service/.../chat.tsimport the same implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chat/src/server/trpc/service.ts` around lines 39 - 86, The respondToQuestionWithOptimisticState implementation is duplicated; extract it into a shared generic helper (e.g., respondToQuestionWithOptimisticState) that accepts a runtime shaped like { harness: { respondToQuestion(...) }, pendingSandboxQuestion, answeredQuestionIds, pendingQuestionResponses } and is parameterized over payload/result types, then replace the local function in service.ts and the near-identical function in host-service's chat.ts to both import and call the shared helper (keep the exact rollback/dedup logic: use Promise.resolve().then(...), check pendingQuestionResponses.get(questionId) === responsePromise before rolling back, and restore previousSandboxQuestion when appropriate).packages/host-service/src/runtime/chat/chat.ts (1)
110-153: Duplicated implementation — see suggestion inpackages/chat/src/server/trpc/service.ts.This
respondToQuestionWithOptimisticStateis essentially the same as the one inpackages/chat/src/server/trpc/service.ts(lines 43‑86), just typed againstChatQuestionPayload/RuntimeQuestionResultinstead of the tRPC variants. Keeping two copies in lock-step is fragile — a future tweak (e.g., adding telemetry, changing rollback rules) has to land in both. Consider extracting a shared generic helper, as suggested in the comment onservice.ts.The implementation itself looks correct: rollback gating on
pendingQuestionResponses.get(questionId) === responsePromiseprevents stomping overagent_start/agent_endclears, and theruntime.pendingSandboxQuestion === nullguard avoids restoring over a newer sandbox prompt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/runtime/chat/chat.ts` around lines 110 - 153, respondToQuestionWithOptimisticState is a duplicate of the same logic in packages/chat/src/server/trpc/service.ts; extract the shared optimistic-state helper into a common module and have both implementations call it to avoid drift. Create a generic function (e.g., respondWithOptimisticState<TPayload, TResult>) that encapsulates the optimistic-add, rollback on error (checking pendingResponses.get(questionId) === responsePromise), sandbox restore logic (checking previous pendingSandboxQuestion and pendingSandboxQuestion === null), and cleanup of pendingQuestionResponses; replace the local respondToQuestionWithOptimisticState and the tRPC variant to delegate to that exported helper while preserving the exact checks and types (use adapter thin wrappers to convert ChatQuestionPayload/RuntimeQuestionResult and the tRPC types if necessary).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/chat/src/server/trpc/service.ts`:
- Around line 39-86: The respondToQuestionWithOptimisticState implementation is
duplicated; extract it into a shared generic helper (e.g.,
respondToQuestionWithOptimisticState) that accepts a runtime shaped like {
harness: { respondToQuestion(...) }, pendingSandboxQuestion,
answeredQuestionIds, pendingQuestionResponses } and is parameterized over
payload/result types, then replace the local function in service.ts and the
near-identical function in host-service's chat.ts to both import and call the
shared helper (keep the exact rollback/dedup logic: use
Promise.resolve().then(...), check pendingQuestionResponses.get(questionId) ===
responsePromise before rolling back, and restore previousSandboxQuestion when
appropriate).
In `@packages/host-service/src/runtime/chat/chat.ts`:
- Around line 110-153: respondToQuestionWithOptimisticState is a duplicate of
the same logic in packages/chat/src/server/trpc/service.ts; extract the shared
optimistic-state helper into a common module and have both implementations call
it to avoid drift. Create a generic function (e.g.,
respondWithOptimisticState<TPayload, TResult>) that encapsulates the
optimistic-add, rollback on error (checking pendingResponses.get(questionId) ===
responsePromise), sandbox restore logic (checking previous
pendingSandboxQuestion and pendingSandboxQuestion === null), and cleanup of
pendingQuestionResponses; replace the local respondToQuestionWithOptimisticState
and the tRPC variant to delegate to that exported helper while preserving the
exact checks and types (use adapter thin wrappers to convert
ChatQuestionPayload/RuntimeQuestionResult and the tRPC types if necessary).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 414df429-ebef-4ede-abe4-10aeab35bf20
📒 Files selected for processing (8)
apps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/AskUserQuestionToolCall/AskUserQuestionToolCall.tsxapps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/ToolStatusBadge/index.tspackages/chat/src/server/trpc/service.test.tspackages/chat/src/server/trpc/service.tspackages/chat/src/server/trpc/utils/runtime/index.tspackages/chat/src/server/trpc/utils/runtime/runtime.test.tspackages/chat/src/server/trpc/utils/runtime/runtime.tspackages/host-service/src/runtime/chat/chat.ts
✅ Files skipped from review due to trivial changes (3)
- packages/chat/src/server/trpc/utils/runtime/index.ts
- apps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/ToolStatusBadge/index.ts
- packages/chat/src/server/trpc/utils/runtime/runtime.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/chat/src/server/trpc/utils/runtime/runtime.ts
- apps/desktop/src/renderer/components/Chat/ChatInterface/components/ToolCallBlock/components/AskUserQuestionToolCall/AskUserQuestionToolCall.tsx
- packages/chat/src/server/trpc/service.test.ts
12 tasks
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
Apr 25, 2026
…ompt (superset-sh#3662) * refactor(desktop): extract ToolStatusBadge from AskUserQuestionToolCall Pull the inline QuestionStatusDescription component and its QUESTION_STATUS_CONFIG lookup into a shared ToolStatusBadge with a variant prop (default / success / danger) so other tool call rows can reuse consistent badge styling without duplicating the pattern. * fix(chat,host-service): prevent stale ask_user question from shadowing sandbox access prompt The harness sets displayState.pendingQuestion when ask_user fires but only clears it on agent_end — never when the user answers. If an agent asks a question and then immediately calls request_access, the already- answered question stays in displayState.pendingQuestion and shadows the new sandbox prompt. The agent then waits for the sandbox prompt to be answered, but the UI never shows it, causing a deadlock. Fix: track answeredQuestionIds per session. In getDisplayState, filter out any harness pendingQuestion whose ID has already been answered this turn so the sandbox prompt can surface correctly. Clear the set on agent_start/agent_end. Also threads the sandbox request reason through as a top-level description on the pending question object so UIs can display it as context beneath the question header. * feat(desktop): custom UI for request_access tool call Replace the generic INPUT/OUTPUT block for request_access with a dedicated RequestSandboxAccessToolCall component that matches the question tool's visual language: - FolderLock icon with "Request Access" title - Status badge in the hint slot: AWAITING RESPONSE / ACCESS GRANTED / ACCESS DENIED / CANCELLED / ERROR (red only on error) - Not expandable/clickable while awaiting a response - After resolution: path and reason as muted context lines, then the access decision as the primary answer line Also surfaces the request reason as context below the question text in QuestionInputOverlay, using a new optional description field on the pending question shape. Alias request_sandbox_access → request_access so either tool name routes to the same component. * fix(desktop): handle empty path/reason and plain-string result in request_access UI - Use || for hasContext so empty string args.path doesn't hide reason - Fall back to result.text when result.content is absent, since getResult wraps non-JSON string outputs as { text } - Update normalizeToolName test to match the request_sandbox_access → request_access alias * fix(chat): preserve question prompt on response failure * fix(chat): rollback question state on response failure * lint * fix(chat): dedupe concurrent question responses --------- Co-authored-by: Kiet Ho <hoakiet98@gmail.com>
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 25, 2026
upstream の 10 commits は #426 と #427 で fork 固有の差分を保ちながら個別に cherry-pick 取り込み済み。本 merge は ours strategy で **記録だけ** マージ済みに することで behind=0 を達成し、git 履歴上の追跡を正しくする。 Cherry-pick 取り込み済 (PR #426): - 5aab22a fix closed picker filters (superset-sh#3702) → cdb52f9 - 99db5be [codex] simplify workspace controls (superset-sh#3714) → f079606 - 186078a fix(chat): prevent ask_user question from shadowing sandbox access prompt (superset-sh#3662) → 09d6b57 - 47893c2 fix desktop workspace creation title clamp (superset-sh#3718) → 6a8c4ae - 09323ff Add diff pane file viewer action (superset-sh#3715) → 817ed8d - a5891c6 remove pending launch log (superset-sh#3721) → 0764d03 - c83de0c Add Tiptap table support (superset-sh#3719) → e67a885 - 486b621 [codex] Fix v2 terminal lifecycle after sleep (superset-sh#3711) → b71fbbb (+ #426 内 review fixups) Cherry-pick 取り込み済 (PR #427): - e07aef6 feat(desktop): play v2 notification hooks client-side (superset-sh#3675) → 27ac18a - eae6008 [codex] Port v2 terminal hotkeys to v1 (superset-sh#3724) → 05a77b8 (+ #427 内 Windows .ps1 v2 化) Fork 固有領域は変更ゼロで保持: 19 tRPC procedures (workspaces.githubExtended)、 AudioScheduler / Aivis TTS / notification-manager、terminal suggestion handler (新 terminalKeyboardHandler.ts に移植)、TERMINAL_OPTIONS、SUPERSET_WORKSPACE_NAME、 MainWindowEffects、INCEPTION_AUTH_PROVIDER_ID、v1MigrationState、TiptapPromptEditor、 electron-builder.ts (dmg.size="4g", fileAssociations)、Service Status Dashboard、 Linux daemon systemd、Worktree auto-sync、Windows support、DnD scratch route 他。
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.
Issue
Two related bugs with
request_accessin the chat UI:Deadlock: when an agent calls
ask_user_questionfollowed byrequest_accessin the same turn, the already-answered ask_user question stays indisplayState.pendingQuestion(the harness only clears it onagent_end). This shadows the new sandbox prompt — the agent waits for approval that the UI never shows, hanging forever.Answered question reappears: leaving and re-entering a workspace re-renders the component tree, causing the stale
pendingQuestionto surface again as if the user never responded.Root cause
displayState.pendingQuestionis set onask_questionevents but only cleared onagent_end. Since the sandbox prompt blocks the agent from completing,agent_endnever fires, and the stale question never clears — a deadlock.Fix
Track
answeredQuestionIdsper session. IngetDisplayState, filter out any harnesspendingQuestionwhose ID has already been answered this turn, so the sandbox prompt can surface correctly. Clear the set onagent_start/agent_end. Applied to both thepackages/chat(web/CLI) andpackages/host-service(Electron) runtime implementations.Changes
Commits (in order):
ToolStatusBadgefromAskUserQuestionToolCall— shared badge component withvariantprop used by both question and sandbox tool call rowsansweredQuestionIdstracking in both runtime implementationsRequestSandboxAccessToolCallUI matching the question tool's visual languageUI improvements for
request_access:FolderLockicon, "Request Access" titlerequest_sandbox_accessaliased →request_access(canonical name matches the actual tool name)Testing
ask_user_question(e.g. "ask me a yes/no question") — answer itrequest_accesson a path (e.g. "now request access to ~/Downloads")request_accesstool call row showsFolderLockicon, "Request Access" title, and AWAITING RESPONSE badge while pending (not expandable)Summary by cubic
Stops answered
ask_user_questionprompts from blocking therequest_accessoverlay and adds a focused UI for sandbox access requests with clear status and context.Bug Fixes
answeredQuestionIdsper session inpackages/chatandpackages/host-service; filter answered harnesspendingQuestionsorequest_accessshows; clear onagent_start/agent_end.pendingQuestionResponses; keep answered state and sandbox overlay consistent on resolve/reject.respondToQuestionfailure: do not mark the question answered and restorependingSandboxQuestionso the overlay stays visible.apps/desktop, handle emptypath/reasonand plain-string results forrequest_access.New Features
RequestSandboxAccessToolCallwithFolderLockicon and status badge (Awaiting Response / Access Granted / Access Denied / Cancelled / Error); not expandable while pending; after resolution, shows path/reason and the decision. Show the request reason under the approval overlay question; extract reusableToolStatusBadge; aliasrequest_sandbox_access→request_access.Written for commit f8148e5. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests