Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ interface ChatInputFooterProps {
pendingQuestion?: {
questionId: string;
question: string;
description?: string;
options?: { label: string; description?: string }[];
} | null;
isQuestionSubmitting?: boolean;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ interface QuestionInputOverlayProps {
question: {
questionId: string;
question: string;
description?: string;
options?: QuestionOption[];
};
isSubmitting: boolean;
Expand Down Expand Up @@ -63,9 +64,16 @@ export function QuestionInputOverlay({
<div className="flex max-h-[300px] flex-col overflow-hidden rounded-[13px] border-[0.5px] border-border bg-foreground/[0.02]">
{/* Question — pinned header */}
<div className="flex shrink-0 items-start gap-2 px-3 pt-3 pb-3">
<p className="flex-1 text-sm leading-snug text-foreground">
{question.question}
</p>
<div className="flex-1 space-y-1">
<p className="text-sm leading-snug text-foreground">
{question.question}
</p>
{question.description && (
<p className="text-xs leading-snug text-muted-foreground">
{question.description}
</p>
)}
</div>
<Tooltip>
<TooltipTrigger asChild>
<button
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import { ListTaskStatusesToolCall } from "./components/ListTaskStatusesToolCall"
import { ListTasksToolCall } from "./components/ListTasksToolCall";
import { ListWorkspacesToolCall } from "./components/ListWorkspacesToolCall";
import { LspInspectToolCall } from "./components/LspInspectToolCall";
import { RequestSandboxAccessToolCall } from "./components/RequestSandboxAccessToolCall";
import { SkillToolCall } from "./components/SkillToolCall";
import { StartAgentSessionToolCall } from "./components/StartAgentSessionToolCall";
import { SubagentToolCall } from "./components/SubagentToolCall";
Expand Down Expand Up @@ -614,8 +615,15 @@ export function ToolCallBlock({
);
}

if (toolName === "request_sandbox_access") {
return <SupersetToolCall part={part} toolName="Request sandbox access" />;
if (toolName === "request_access") {
return (
<RequestSandboxAccessToolCall
part={part}
args={args}
result={result}
isInterrupted={isInterrupted}
/>
);
}

if (toolName === "lsp_inspect") {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
} from "lucide-react";
import { useMemo } from "react";
import type { ToolPart } from "../../../../utils/tool-helpers";
import { ToolStatusBadge } from "../ToolStatusBadge";

interface QuestionToolOption {
label: string;
Expand Down Expand Up @@ -124,27 +125,6 @@ function findAnswerForQuestion({
return undefined;
}

type QuestionStatus = "awaiting" | "answered" | "cancelled";

const QUESTION_STATUS_CONFIG: Record<
QuestionStatus,
{ label: string; icon: typeof ClockIcon }
> = {
awaiting: { label: "Awaiting Response", icon: ClockIcon },
answered: { label: "Answered", icon: CheckIcon },
cancelled: { label: "Cancelled", icon: XIcon },
};

function QuestionStatusDescription({ status }: { status: QuestionStatus }) {
const { label, icon: Icon } = QUESTION_STATUS_CONFIG[status];
return (
<span className="ml-2 flex items-center gap-1 font-medium uppercase tracking-wide">
<Icon className="h-3 w-3 shrink-0" />
{label}
</span>
);
}

function toSingleQuestion(
args: Record<string, unknown>,
): QuestionToolQuestion[] {
Expand Down Expand Up @@ -267,11 +247,11 @@ export function AskUserQuestionToolCall({
title="Question"
description={
isPending ? (
<QuestionStatusDescription status="awaiting" />
<ToolStatusBadge icon={ClockIcon} label="Awaiting Response" />
) : isAnswered ? (
<QuestionStatusDescription status="answered" />
<ToolStatusBadge icon={CheckIcon} label="Answered" />
) : isCancelled || isCancelledByError || isCancelledByStop ? (
<QuestionStatusDescription status="cancelled" />
<ToolStatusBadge icon={XIcon} label="Cancelled" />
) : undefined
}
>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
import { ToolCallRow } from "@superset/ui/ai-elements/tool-call-row";
import {
CheckIcon,
CircleXIcon,
ClockIcon,
FolderLockIcon,
XIcon,
} from "lucide-react";
import type { ComponentType } from "react";
import type { ToolPart } from "../../../../utils/tool-helpers";
import type { ToolStatusBadgeVariant } from "../ToolStatusBadge";
import { ToolStatusBadge } from "../ToolStatusBadge";

interface RequestSandboxAccessToolCallProps {
part: ToolPart;
args: Record<string, unknown>;
result: Record<string, unknown>;
isInterrupted?: boolean;
}

type AccessStatus = "pending" | "granted" | "denied" | "cancelled" | "error";

const ACCESS_STATUS_CONFIG: Record<
AccessStatus,
{
icon: ComponentType<{ className?: string }>;
label: string;
variant?: ToolStatusBadgeVariant;
}
> = {
pending: { icon: ClockIcon, label: "Awaiting Response" },
granted: { icon: CheckIcon, label: "Access Granted" },
denied: { icon: XIcon, label: "Access Denied" },
cancelled: { icon: XIcon, label: "Cancelled" },
error: { icon: CircleXIcon, label: "Error", variant: "danger" },
};

function toAccessDecision(content: string): "granted" | "denied" | null {
if (content.startsWith("Access already granted")) return "granted";
if (content.startsWith("Access granted")) return "granted";
if (content.startsWith("Access denied")) return "denied";
return null;
}

function toAccessStatus(
part: ToolPart,
result: Record<string, unknown>,
isInterrupted: boolean,
): AccessStatus {
if (
isInterrupted &&
part.state !== "output-available" &&
part.state !== "output-error"
) {
return "cancelled";
}
if (part.state !== "output-available" && part.state !== "output-error") {
return "pending";
}
if (part.state === "output-error" || result.isError === true) {
return "error";
}
const content =
(typeof result.content === "string" && result.content.trim()) ||
(typeof result.text === "string" && result.text.trim()) ||
"";
return toAccessDecision(content) ?? "error";
}

export function RequestSandboxAccessToolCall({
part,
args,
result,
isInterrupted = false,
}: RequestSandboxAccessToolCallProps) {
const requestedPath = typeof args.path === "string" ? args.path.trim() : null;
const reason = typeof args.reason === "string" ? args.reason.trim() : null;

const status = toAccessStatus(part, result, isInterrupted);
const { icon, label, variant } = ACCESS_STATUS_CONFIG[status];
const statusBadge = (
<ToolStatusBadge icon={icon} label={label} variant={variant} />
);

const isPending = status === "pending";
const isCancelledOrError = status === "cancelled" || status === "error";
const hasContext = Boolean(requestedPath || reason);

return (
<ToolCallRow
icon={FolderLockIcon}
isPending={false}
isError={false}
title="Request Access"
description={statusBadge}
>
Comment on lines +90 to +96
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

{!isPending && hasContext ? (
<div className="space-y-1 px-3 py-2">
{requestedPath ? (
<div className="text-xs text-muted-foreground">
Path: {requestedPath}
</div>
) : null}
{reason ? (
<div className="text-xs text-muted-foreground">
Reason: {reason}
</div>
) : null}
{!isCancelledOrError ? (
<div className="text-sm text-foreground">
{status === "granted" ? "Access granted" : "Access denied"}
</div>
) : (
<div className="flex items-center gap-1 text-sm text-destructive">
<CircleXIcon className="h-3 w-3 shrink-0" />
Aborted
</div>
)}
</div>
) : undefined}
</ToolCallRow>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { RequestSandboxAccessToolCall } from "./RequestSandboxAccessToolCall";
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { cn } from "@superset/ui/lib/utils";
import type { ComponentType } from "react";

const VARIANT_CLASSES = {
default: "",
success: "text-emerald-500",
danger: "text-destructive",
} as const;

export type ToolStatusBadgeVariant = keyof typeof VARIANT_CLASSES;

interface ToolStatusBadgeProps {
icon: ComponentType<{ className?: string }>;
label: string;
variant?: ToolStatusBadgeVariant;
}

export function ToolStatusBadge({
icon: Icon,
label,
variant = "default",
}: ToolStatusBadgeProps) {
return (
<span
className={cn(
"ml-2 flex items-center gap-1 font-medium uppercase tracking-wide",
VARIANT_CLASSES[variant],
)}
>
<Icon className="h-3 w-3 shrink-0" />
{label}
</span>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export type { ToolStatusBadgeVariant } from "./ToolStatusBadge";
export { ToolStatusBadge } from "./ToolStatusBadge";
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@ describe("normalizeToolName", () => {
expect(normalizeToolName("web_extract")).toBe("web_fetch");
expect(normalizeToolName("ask_user")).toBe("ask_user_question");
expect(normalizeToolName("ast_smart_edit")).toBe("ast_smart_edit");
expect(normalizeToolName("request_sandbox_access")).toBe(
"request_sandbox_access",
);
expect(normalizeToolName("request_sandbox_access")).toBe("request_access");
expect(normalizeToolName("task_write")).toBe("task_write");
expect(normalizeToolName("task_check")).toBe("task_check");
expect(normalizeToolName("submit_plan")).toBe("submit_plan");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ const TOOL_NAME_ALIASES: Record<string, string> = {

// Keep explicit passthroughs for newer Mastra tool names
ast_smart_edit: "ast_smart_edit",
request_sandbox_access: "request_sandbox_access",
request_access: "request_access",
request_sandbox_access: "request_access",
Comment on lines +32 to +33
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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.

task_write: "task_write",
task_check: "task_check",
submit_plan: "submit_plan",
Expand Down
70 changes: 66 additions & 4 deletions packages/chat/src/server/trpc/service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,17 @@ mock.module("mastracode", () => ({

const { ChatRuntimeService } = await import("./service");

function createRuntime(): RuntimeSession {
function createRuntime(options?: {
respondToQuestion?: RuntimeSession["harness"]["respondToQuestion"];
}): RuntimeSession {
return {
sessionId: SESSION_ID,
cwd: CWD,
harness: {
abort: mock(() => {}),
respondToToolApproval: mock(async (payload: unknown) => payload),
respondToQuestion: mock(async (payload: unknown) => payload),
respondToQuestion:
options?.respondToQuestion ?? mock(async (payload: unknown) => payload),
respondToPlanApproval: mock(async (payload: unknown) => payload),
} as unknown as RuntimeSession["harness"],
mcpManager: null as RuntimeSession["mcpManager"],
Expand All @@ -39,11 +42,13 @@ function createRuntime(): RuntimeSession {
path: "/tmp/secret",
reason: "Need access",
},
answeredQuestionIds: new Set(),
pendingQuestionResponses: new Map(),
};
}

function createServiceHarness() {
const runtime = createRuntime();
function createServiceHarness(options?: Parameters<typeof createRuntime>[0]) {
const runtime = createRuntime(options);
const service = new ChatRuntimeService({
headers: async () => ({}),
apiUrl: "http://localhost:3000",
Expand Down Expand Up @@ -134,4 +139,61 @@ describe("ChatRuntimeService control mutations", () => {
});
expect(runtime.pendingSandboxQuestion).toBeNull();
});

it("does not clear pending question state when question response fails", async () => {
const respondToQuestion = mock(async () => {
throw new Error("failed to answer");
}) as RuntimeSession["harness"]["respondToQuestion"];
const { caller, runtime } = createServiceHarness({ respondToQuestion });

await expect(
caller.session.question.respond({
sessionId: SESSION_ID,
cwd: CWD,
payload: { questionId: "sandbox-1", answer: "Yes" },
}),
).rejects.toThrow("failed to answer");

expect(runtime.answeredQuestionIds.has("sandbox-1")).toBe(false);
expect(runtime.pendingSandboxQuestion).toEqual({
questionId: "sandbox-1",
path: "/tmp/secret",
reason: "Need access",
});
});

it("deduplicates concurrent responses for the same question", async () => {
let resolveResponse: (value: unknown) => void = () => {};
const respondToQuestion = mock(
() =>
new Promise((resolve) => {
resolveResponse = resolve;
}),
) as RuntimeSession["harness"]["respondToQuestion"];
const { caller, runtime } = createServiceHarness({ respondToQuestion });
const payload = { questionId: "sandbox-1", answer: "Yes" };

const firstResponse = caller.session.question.respond({
sessionId: SESSION_ID,
cwd: CWD,
payload,
});
const secondResponse = caller.session.question.respond({
sessionId: SESSION_ID,
cwd: CWD,
payload,
});

await new Promise((resolve) => setTimeout(resolve, 0));

expect(respondToQuestion).toHaveBeenCalledTimes(1);
expect(runtime.answeredQuestionIds.has("sandbox-1")).toBe(true);
expect(runtime.pendingSandboxQuestion).toBeNull();

resolveResponse({ ok: true });

await expect(firstResponse).resolves.toEqual({ ok: true });
await expect(secondResponse).resolves.toEqual({ ok: true });
expect(runtime.pendingQuestionResponses.size).toBe(0);
});
});
Loading
Loading