From 57db590bdda4378cab50b08b2f7dbc2eaeaecf28 Mon Sep 17 00:00:00 2001 From: "mux-bot[bot]" <264182336+mux-bot[bot]@users.noreply.github.com> Date: Thu, 30 Apr 2026 20:26:07 +0000 Subject: [PATCH 01/22] refactor: route ThemeContext color-scheme through isLightThemeMode Replace the inline `theme === "light" || theme === "flexoki-light"` check in `getColorScheme` with the shared `isLightThemeMode` helper from `shiki-shared.ts`, so the `-light` suffix convention has a single source of truth and stays consistent with the syntax-highlighting call sites that already use it. --- src/browser/contexts/ThemeContext.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/browser/contexts/ThemeContext.tsx b/src/browser/contexts/ThemeContext.tsx index 5e83061b65..fc20eaa609 100644 --- a/src/browser/contexts/ThemeContext.tsx +++ b/src/browser/contexts/ThemeContext.tsx @@ -10,6 +10,7 @@ import React, { } from "react"; import { readPersistedString, usePersistedState } from "@/browser/hooks/usePersistedState"; import { UI_THEME_KEY } from "@/common/constants/storage"; +import { isLightThemeMode } from "@/browser/utils/highlighting/shiki-shared"; export type ThemeMode = "light" | "dark" | "flexoki-light" | "flexoki-dark"; export type ThemePreference = ThemeMode | "auto"; @@ -74,7 +75,8 @@ const FAVICON_BY_SCHEME: Record<"light" | "dark", string> = { /** Map theme mode to CSS color-scheme value */ function getColorScheme(theme: ThemeMode): "light" | "dark" { - return theme === "light" || theme === "flexoki-light" ? "light" : "dark"; + // Reuse the shared `-light` suffix convention so we have one source of truth for the light/dark mapping. + return isLightThemeMode(theme) ? "light" : "dark"; } function applyThemeFavicon(theme: ThemeMode) { From 5f595db41bba1e04f867d343548751cea9db01bb Mon Sep 17 00:00:00 2001 From: mux-bot Date: Fri, 1 May 2026 12:22:54 +0000 Subject: [PATCH 02/22] refactor: drop unused appendSpace literals on skill/model alias suggestions The skill and model alias build callbacks in getSlashCommandSuggestions hardcode the trailing space, so the appendSpace: true property on those SuggestionDefinition literals is dead. Remove it and add a brief comment explaining why we don't propagate appendSpace through these paths so a future reader doesn't assume the field is consulted there. --- src/browser/utils/slashCommands/suggestions.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/browser/utils/slashCommands/suggestions.ts b/src/browser/utils/slashCommands/suggestions.ts index 35d6751886..d43b896c43 100644 --- a/src/browser/utils/slashCommands/suggestions.ts +++ b/src/browser/utils/slashCommands/suggestions.ts @@ -82,12 +82,14 @@ function buildTopLevelSuggestions( return scope; }; + // The skill build callback below hardcodes the trailing space, so we omit + // `appendSpace` here — leaving it set would be a no-op and falsely suggest + // the build path consults it. const skillDefinitions: SuggestionDefinition[] = (context.agentSkills ?? []) .filter((skill) => !SLASH_COMMAND_DEFINITION_MAP.has(skill.name)) .map((skill) => ({ key: skill.name, description: `${skill.description} (${formatScopeLabel(skill.scope)})`, - appendSpace: true, })); const skillSuggestions = filterAndMapSuggestions(skillDefinitions, partial, (definition) => { @@ -100,12 +102,13 @@ function buildTopLevelSuggestions( }; }); - // Model alias one-shot suggestions (e.g., /haiku, /sonnet, /opus+high) + // Model alias one-shot suggestions (e.g., /haiku, /sonnet, /opus+high). + // The build callback below hardcodes the trailing space, so `appendSpace` + // is intentionally omitted here. const modelAliasDefinitions: SuggestionDefinition[] = Object.entries(MODEL_ABBREVIATIONS).map( ([alias, modelId]) => ({ key: alias, description: `Send with ${formatModelDisplayName(modelId.split(":")[1] ?? modelId)} (one message, +level for thinking)`, - appendSpace: true, }) ); From 0714309232523bff4d2e7ef2991ab888d0f9acf0 Mon Sep 17 00:00:00 2001 From: mux Date: Fri, 1 May 2026 16:23:22 +0000 Subject: [PATCH 03/22] refactor: extract shared ServiceTier type from ServiceTierSchema The OpenAI service-tier literal union ('auto' | 'default' | 'flex' | 'priority') was duplicated in three places: the CLI options interface (src/cli/run.ts), the providerModelFactory cast at the providers.jsonc boundary, and a local OpenAIServiceTier alias in ProvidersSection.tsx. ServiceTierSchema (src/common/config/schemas/providersConfig.ts) already defines this enum as the runtime source of truth, so derive a TypeScript ServiceTier alias via z.infer once and import it at each site. Pure type-only refactor; the emitted JS and the schema remain unchanged. --- src/browser/features/Settings/Sections/ProvidersSection.tsx | 3 ++- src/cli/run.ts | 3 ++- src/common/config/schemas/providersConfig.ts | 1 + src/node/services/providerModelFactory.ts | 3 ++- 4 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/browser/features/Settings/Sections/ProvidersSection.tsx b/src/browser/features/Settings/Sections/ProvidersSection.tsx index f5ccc4b671..4bf8560d72 100644 --- a/src/browser/features/Settings/Sections/ProvidersSection.tsx +++ b/src/browser/features/Settings/Sections/ProvidersSection.tsx @@ -78,6 +78,7 @@ import type { AddCustomOpenAICompatibleProviderInput, ProviderConfigInfo, } from "@/common/orpc/types"; +import type { ServiceTier } from "@/common/config/schemas/providersConfig"; type MuxGatewayLoginStatus = "idle" | "starting" | "waiting" | "success" | "error"; type CodexOauthFlowStatus = "idle" | "starting" | "waiting" | "error"; @@ -85,7 +86,7 @@ type CopilotLoginStatus = "idle" | "starting" | "waiting" | "success" | "error"; const OPENAI_SERVICE_TIER_UNSET = "unset"; -type OpenAIServiceTier = "auto" | "default" | "flex" | "priority"; +type OpenAIServiceTier = ServiceTier; type OpenAIServiceTierSelectValue = typeof OPENAI_SERVICE_TIER_UNSET | OpenAIServiceTier; function isOpenAIServiceTier(value: string): value is OpenAIServiceTier { diff --git a/src/cli/run.ts b/src/cli/run.ts index 8037588351..233c9f0283 100644 --- a/src/cli/run.ts +++ b/src/cli/run.ts @@ -35,6 +35,7 @@ import { type SendMessageOptions, type WorkspaceChatMessage, } from "../common/orpc/types"; +import type { ServiceTier } from "../common/config/schemas/providersConfig"; import { createDisplayUsage } from "../common/utils/tokens/displayUsage"; import { getTotalCost, @@ -298,7 +299,7 @@ interface CLIOptions { mcpConfig: boolean; experiment: string[]; budget?: number; - serviceTier?: "auto" | "default" | "flex" | "priority"; + serviceTier?: ServiceTier; use1m?: boolean; keepBackgroundProcesses?: boolean; } diff --git a/src/common/config/schemas/providersConfig.ts b/src/common/config/schemas/providersConfig.ts index 957e4331a4..beed8dfb43 100644 --- a/src/common/config/schemas/providersConfig.ts +++ b/src/common/config/schemas/providersConfig.ts @@ -5,6 +5,7 @@ import { ProviderModelEntrySchema } from "./providerModelEntry"; export const CacheTtlSchema = z.enum(["5m", "1h"]); export const ServiceTierSchema = z.enum(["auto", "default", "flex", "priority"]); +export type ServiceTier = z.infer; export const CodexOauthDefaultAuthSchema = z.enum(["oauth", "apiKey"]); export const BaseProviderConfigSchema = z diff --git a/src/node/services/providerModelFactory.ts b/src/node/services/providerModelFactory.ts index 573a1f3c7f..84f02a0972 100644 --- a/src/node/services/providerModelFactory.ts +++ b/src/node/services/providerModelFactory.ts @@ -20,6 +20,7 @@ import { import { parseCodexOauthAuth } from "@/node/utils/codexOauthAuth"; import type { Config, ProviderConfig, ProvidersConfig } from "@/node/config"; import type { MuxProviderOptions } from "@/common/types/providerOptions"; +import type { ServiceTier } from "@/common/config/schemas/providersConfig"; import type { ExternalSecretResolver } from "@/common/types/secrets"; import { isOpReference } from "@/common/utils/opRef"; import { isProviderDisabledInConfig } from "@/common/utils/providers/isProviderDisabled"; @@ -1286,7 +1287,7 @@ export class ProviderModelFactory { if (configServiceTier && muxProviderOptions.openai?.serviceTier == null) { muxProviderOptions.openai = { ...muxProviderOptions.openai, - serviceTier: configServiceTier as "auto" | "default" | "flex" | "priority", + serviceTier: configServiceTier as ServiceTier, }; } if (configWireFormat === "responses" || configWireFormat === "chatCompletions") { From 8ac69864d92f8ffdb720edeb94f3938a44772f04 Mon Sep 17 00:00:00 2001 From: "mux-bot[bot]" <264182336+mux-bot[bot]@users.noreply.github.com> Date: Sat, 2 May 2026 00:28:07 +0000 Subject: [PATCH 04/22] refactor: extract ResolvedWorkspaceAiSettings type alias in taskService MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shape `{ model: string; thinkingLevel?: ThinkingLevel }` (used internally to read per-agent AI settings off partial workspace metadata) was duplicated 7 times across the parameter and return types of `resolveWorkspaceAISettings`, `resolveTaskAISettings`, and `resolveParentAutoResumeOptions`. The new sub-agent defaults split (#3215) made this duplication especially visible because it added a third method copying the same inline shape. Introduce a private `ResolvedWorkspaceAiSettings` interface at module scope and use it everywhere. Pure type-level cleanup — emitted JS, runtime behavior, and the schema-derived `WorkspaceAISettings` type (where `thinkingLevel` is required) are all unchanged. 🤖 _Generated with `mux` • Model: `anthropic:claude-opus-4-7` • Thinking: `xhigh`_ --- src/node/services/taskService.ts | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index c139ce3c79..0f964476b0 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -91,6 +91,17 @@ export type TaskKind = "agent"; export type AgentTaskStatus = NonNullable; +/** + * Resolved per-agent AI settings (canonical model + optional thinking level). + * + * `thinkingLevel` is optional because internal callers read these settings off of + * partial workspace metadata where the field may be missing on older entries. + */ +interface ResolvedWorkspaceAiSettings { + model: string; + thinkingLevel?: ThinkingLevel; +} + export interface AgentTaskStatusLookup { exists: boolean; taskStatus: AgentTaskStatus | null; @@ -388,11 +399,11 @@ export class TaskService { // fall back to legacy workspace settings for older configs. private resolveWorkspaceAISettings( workspace: { - aiSettingsByAgent?: Record; - aiSettings?: { model: string; thinkingLevel?: ThinkingLevel }; + aiSettingsByAgent?: Record; + aiSettings?: ResolvedWorkspaceAiSettings; }, agentId: string | undefined - ): { model: string; thinkingLevel?: ThinkingLevel } | undefined { + ): ResolvedWorkspaceAiSettings | undefined { const normalizedAgentId = typeof agentId === "string" && agentId.trim().length > 0 ? normalizeAgentId(agentId, "") @@ -406,8 +417,8 @@ export class TaskService { private resolveTaskAISettings(params: { cfg: ReturnType; parentMeta: { - aiSettingsByAgent?: Record; - aiSettings?: { model: string; thinkingLevel?: ThinkingLevel }; + aiSettingsByAgent?: Record; + aiSettings?: ResolvedWorkspaceAiSettings; }; agentId: string; modelString?: string; @@ -456,8 +467,8 @@ export class TaskService { parentWorkspaceId: string, parentEntry: { workspace: { - aiSettingsByAgent?: Record; - aiSettings?: { model: string; thinkingLevel?: ThinkingLevel }; + aiSettingsByAgent?: Record; + aiSettings?: ResolvedWorkspaceAiSettings; }; }, fallbackModel: string, From e81909e3ef5daf7ecb8c141f48179a2a559e6c10 Mon Sep 17 00:00:00 2001 From: "mux-bot[bot]" <264182336+mux-bot[bot]@users.noreply.github.com> Date: Sat, 2 May 2026 05:04:35 +0000 Subject: [PATCH 05/22] refactor: drop redundant GuardAnchors type alias in file_edit_insert MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In `src/node/services/tools/file_edit_insert.ts`, `GuardAnchors` was defined as `Pick`, but `InsertContentOptions` itself is already `Pick` after the `.nullish()` strict-mode refactor in #2250 stripped the `InsertContentOptions` interface down to those same two fields. The two aliases are now structurally identical, so `GuardAnchors` is dead. Drop the alias and use `InsertContentOptions` for the two callers (`insertWithGuards`, `resolveGuardAnchor`). Both names were file-local; no exports change. The function names (`insertWithGuards`, `resolveGuardAnchor`) already convey "guard" context, so the parameter type doesn't need to repeat it. Pure type-level cleanup — emitted JS, runtime behavior, and the public tool surface are all unchanged. --- src/node/services/tools/file_edit_insert.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/node/services/tools/file_edit_insert.ts b/src/node/services/tools/file_edit_insert.ts index fd0354b2b2..12a7f6a6f6 100644 --- a/src/node/services/tools/file_edit_insert.ts +++ b/src/node/services/tools/file_edit_insert.ts @@ -44,8 +44,6 @@ function guardFailure(error: string): InsertOperationFailure { }; } -type GuardAnchors = Pick; - export const createFileEditInsertTool: ToolFactory = (config: ToolConfiguration) => { return tool({ description: TOOL_DEFINITIONS.file_edit_insert.description, @@ -175,7 +173,7 @@ function insertContent( function insertWithGuards( originalContent: string, contentToInsert: string, - anchors: GuardAnchors + anchors: InsertContentOptions ): InsertOperationSuccess | InsertOperationFailure { const anchorResult = resolveGuardAnchor(originalContent, anchors); if (!anchorResult.success) { @@ -216,7 +214,7 @@ function findUniqueSubstringIndex( function resolveGuardAnchor( originalContent: string, - { insert_before, insert_after }: GuardAnchors + { insert_before, insert_after }: InsertContentOptions ): GuardResolutionSuccess | InsertOperationFailure { const fileEol = detectFileEol(originalContent); From b513ce920614138b567a13fa345904071c986f0f Mon Sep 17 00:00:00 2001 From: ammar-agent Date: Sat, 2 May 2026 20:18:11 +0000 Subject: [PATCH 06/22] refactor: extract pushStreamErrorRow helper in StreamingMessageAggregator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both stream-error rows in `buildDisplayedMessagesForMessage` (the existing `message.metadata?.error` branch and the new `finishReason === "length"` branch added in #3223) push structurally identical objects, differing only in `id` suffix, `error` string, and `errorType`. The shared parent-message-derived fields (`historyId`, `historySequence`, `model`, `routedThroughGateway`, `timestamp`) were duplicated across both pushes. Extract a local `pushStreamErrorRow` closure that captures the shared fields once. Each branch now reduces to a single call passing the three differing values. Pure refactor — emitted DisplayedMessage objects are identical. --- .../messages/StreamingMessageAggregator.ts | 49 ++++++++++++------- 1 file changed, 30 insertions(+), 19 deletions(-) diff --git a/src/browser/utils/messages/StreamingMessageAggregator.ts b/src/browser/utils/messages/StreamingMessageAggregator.ts index 86d3ac78ee..3cd986b594 100644 --- a/src/browser/utils/messages/StreamingMessageAggregator.ts +++ b/src/browser/utils/messages/StreamingMessageAggregator.ts @@ -32,6 +32,7 @@ import { } from "@/common/types/stream"; import { GOAL_BUDGET_LIMIT_KIND, GOAL_CONTINUATION_KIND } from "@/constants/goals"; import type { LanguageModelV2Usage } from "@ai-sdk/provider"; +import type { StreamErrorType } from "@/common/types/errors"; import type { TodoItem, StatusSetToolResult, NotifyToolResult } from "@/common/types/tools"; import { completeInProgressTodoItems } from "@/common/utils/todoList"; import { getToolOutputUiOnly } from "@/common/utils/tools/toolOutputUiOnly"; @@ -3118,20 +3119,37 @@ export class StreamingMessageAggregator { } }); - // Create stream-error DisplayedMessage if message has error metadata - // This happens after all parts are displayed, so error appears at the end - if (message.metadata?.error) { + // Both stream-error rows (real error metadata + synthesized + // max_tokens truncation) share the same parent-message-derived + // fields. Capture them in one place so adding a new branch later + // can't accidentally drift on `model` / `routedThroughGateway` / + // `historySequence` / `timestamp`. + const pushStreamErrorRow = ( + idSuffix: string, + error: string, + errorType: StreamErrorType + ): void => { displayedMessages.push({ type: "stream-error", - id: `${message.id}-error`, + id: `${message.id}-${idSuffix}`, historyId: message.id, - error: message.metadata.error, - errorType: message.metadata.errorType ?? "unknown", + error, + errorType, historySequence, - model: message.metadata.model, + model: message.metadata?.model, routedThroughGateway: message.metadata?.routedThroughGateway, timestamp: baseTimestamp, }); + }; + + // Create stream-error DisplayedMessage if message has error metadata + // This happens after all parts are displayed, so error appears at the end + if (message.metadata?.error) { + pushStreamErrorRow( + "error", + message.metadata.error, + message.metadata.errorType ?? "unknown" + ); } else if ( // Stream ended cleanly *but* the provider truncated us at max_tokens. // The backend's stream-end path treats this as a successful completion @@ -3144,19 +3162,12 @@ export class StreamingMessageAggregator { !hasActiveStream && message.metadata?.finishReason === "length" ) { - displayedMessages.push({ - type: "stream-error", - id: `${message.id}-length`, - historyId: message.id, - error: - "The model hit its max output token limit before finishing this response. " + + pushStreamErrorRow( + "length", + "The model hit its max output token limit before finishing this response. " + "Lower the thinking level (or split the turn into smaller steps) to give it more headroom.", - errorType: "max_output_tokens", - historySequence, - model: message.metadata.model, - routedThroughGateway: message.metadata?.routedThroughGateway, - timestamp: baseTimestamp, - }); + "max_output_tokens" + ); } } From 5ba447d951b40252708b58ab9a260d082e48ed3c Mon Sep 17 00:00:00 2001 From: ammar-agent Date: Sun, 3 May 2026 05:10:00 +0000 Subject: [PATCH 07/22] refactor: extract seedScrollDirectionBaseline helper in useAutoScroll --- src/browser/hooks/useAutoScroll.ts | 38 +++++++++++++++++------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/src/browser/hooks/useAutoScroll.ts b/src/browser/hooks/useAutoScroll.ts index dfe455c3c7..ac990558ca 100644 --- a/src/browser/hooks/useAutoScroll.ts +++ b/src/browser/hooks/useAutoScroll.ts @@ -83,6 +83,19 @@ export function useAutoScroll() { setAutoScroll(enabled); }, []); + // Seed the baseline read by handleScroll's released-branch direction check + // (`currentScrollTop > previousScrollTop`). Call this from any code path that + // flips autoScrollRef / programmaticDisableRef without a guaranteed follow-up + // scroll event — e.g. jumpToBottom skips the write when scrollTop is already + // max, and disableAutoScroll never fires a scroll event itself. Without a + // fresh baseline, the next user-driven scroll event could compare against a + // stale value (carried across workspace switches or the prior session) and + // misread a small wheel-up notch as "moving toward bottom", spuriously + // relocking the lock that was just released. + const seedScrollDirectionBaseline = useCallback(() => { + lastScrollTopRef.current = contentRef.current?.scrollTop ?? 0; + }, []); + const stickToBottom = useCallback(() => { const scrollContainer = contentRef.current; if (!scrollContainer) return; @@ -142,29 +155,22 @@ export function useAutoScroll() { programmaticDisableRef.current = false; setAutoScrollEnabled(true); stickToBottom(); - // Seed the direction baseline used by handleScroll's released-branch - // user-intent path. stickToBottom doesn't always emit a scroll event - // (it skips the write when scrollTop is already max), so without this - // seed the next user-driven scroll event could compare against a stale - // value carried across workspace switches or earlier sessions. - lastScrollTopRef.current = contentRef.current?.scrollTop ?? 0; + // stickToBottom skips the write when scrollTop is already max, so we may + // not get a follow-up scroll event to refresh lastScrollTopRef. + seedScrollDirectionBaseline(); startBottomLockFrameLoop(); - }, [setAutoScrollEnabled, startBottomLockFrameLoop, stickToBottom]); + }, [seedScrollDirectionBaseline, setAutoScrollEnabled, startBottomLockFrameLoop, stickToBottom]); const disableAutoScroll = useCallback(() => { userScrollIntentUntilRef.current = 0; programmaticDisableRef.current = true; setAutoScrollEnabled(false); - // Seed the direction baseline. The released-branch user-intent path in - // handleScroll compares the next scroll event's scrollTop against - // lastScrollTopRef. disableAutoScroll never fires a scroll event itself, - // so without this seed a small wheel-up notch following a programmatic - // disable would be misread as "moving toward bottom" (because - // previousScrollTop was 0 or some unrelated earlier value), spuriously - // relocking the lock that was just disabled. - lastScrollTopRef.current = contentRef.current?.scrollTop ?? 0; + // disableAutoScroll never fires a scroll event itself, so seed the + // baseline now to keep the next user-driven scroll event's direction + // check honest. + seedScrollDirectionBaseline(); stopBottomLockFrameLoop(); - }, [setAutoScrollEnabled, stopBottomLockFrameLoop]); + }, [seedScrollDirectionBaseline, setAutoScrollEnabled, stopBottomLockFrameLoop]); const markUserScrollIntent = useCallback(() => { programmaticDisableRef.current = false; From fc03e704710d81e52ae423dc78318c58f0a07b38 Mon Sep 17 00:00:00 2001 From: "mux-bot[bot]" <264182336+mux-bot[bot]@users.noreply.github.com> Date: Sun, 3 May 2026 16:20:39 +0000 Subject: [PATCH 08/22] refactor: drop redundant isPlanHandoffAgent boolean in streamContextBuilder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `isPlanHandoffAgent` boolean in `buildPlanInstructions` was extracted when the gate was `effectiveAgentId === "exec" || effectiveAgentId === "orchestrator"`. After #3224 ripped out the Orchestrator agent, the boolean collapsed to a single equality check (`effectiveAgentId === "exec"`), and the trailing `else if (isPlanHandoffAgent && chatHasStartHerePlanSummary)` redundantly re-evaluated the same gate just to log a debug line. Replace the flat `if/else if` with a nested `if (effectiveAgentId === "exec") { … }` that tests the Start Here summary inside, removing the duplicate gate re-check and the now-meaningless boolean. A short comment captures the rationale so a future reader doesn't reintroduce the alias. Pure refactor — emitted control flow, the debug log, and `planContentForTransition` assignments are identical, and the existing 8-test `streamContextBuilder.test.ts` suite passes unchanged (257 tests across the related taskService / modelMessageTransform / aiService suites also pass). --- src/node/services/streamContextBuilder.ts | 78 ++++++++++++----------- 1 file changed, 41 insertions(+), 37 deletions(-) diff --git a/src/node/services/streamContextBuilder.ts b/src/node/services/streamContextBuilder.ts index 7185bc8c75..22a422f355 100644 --- a/src/node/services/streamContextBuilder.ts +++ b/src/node/services/streamContextBuilder.ts @@ -167,48 +167,52 @@ export async function buildPlanInstructions( // Read plan content for agent transition (plan-like → exec). // Only read if switching to the built-in exec agent and last assistant was plan-like. + // The `effectiveAgentId === "exec"` gate used to also include "orchestrator" + // (extracted to an `isPlanHandoffAgent` boolean) before #3224 ripped that agent + // out; the boolean is now redundant with a single equality check, so inline it. let planContentForTransition: string | undefined; - const isPlanHandoffAgent = effectiveAgentId === "exec"; - if (isPlanHandoffAgent && !chatHasStartHerePlanSummary) { - const lastAssistantMessage = [...requestPayloadMessages] - .reverse() - .find((m) => m.role === "assistant"); - const lastAgentId = lastAssistantMessage?.metadata?.agentId; - if (lastAgentId && planResult.content.trim()) { - let lastAgentIsPlanLike = false; - if (lastAgentId === effectiveAgentId) { - lastAgentIsPlanLike = agentIsPlanLike; - } else { - try { - const lastDefinition = await readAgentDefinition( - runtime, - agentDiscoveryPath, - lastAgentId - ); - const lastChain = await resolveAgentInheritanceChain({ - runtime, - workspacePath: agentDiscoveryPath, - agentId: lastAgentId, - agentDefinition: lastDefinition, - workspaceId, - }); - lastAgentIsPlanLike = isPlanLikeInResolvedChain(lastChain); - } catch (error) { - workspaceLog.warn("Failed to resolve last agent definition for plan handoff", { - lastAgentId, - error: getErrorMessage(error), - }); + if (effectiveAgentId === "exec") { + if (chatHasStartHerePlanSummary) { + workspaceLog.debug( + "Skipping plan content injection for plan handoff transition: Start Here already includes the plan in chat history." + ); + } else { + const lastAssistantMessage = [...requestPayloadMessages] + .reverse() + .find((m) => m.role === "assistant"); + const lastAgentId = lastAssistantMessage?.metadata?.agentId; + if (lastAgentId && planResult.content.trim()) { + let lastAgentIsPlanLike = false; + if (lastAgentId === effectiveAgentId) { + lastAgentIsPlanLike = agentIsPlanLike; + } else { + try { + const lastDefinition = await readAgentDefinition( + runtime, + agentDiscoveryPath, + lastAgentId + ); + const lastChain = await resolveAgentInheritanceChain({ + runtime, + workspacePath: agentDiscoveryPath, + agentId: lastAgentId, + agentDefinition: lastDefinition, + workspaceId, + }); + lastAgentIsPlanLike = isPlanLikeInResolvedChain(lastChain); + } catch (error) { + workspaceLog.warn("Failed to resolve last agent definition for plan handoff", { + lastAgentId, + error: getErrorMessage(error), + }); + } } - } - if (lastAgentIsPlanLike) { - planContentForTransition = planResult.content; + if (lastAgentIsPlanLike) { + planContentForTransition = planResult.content; + } } } - } else if (isPlanHandoffAgent && chatHasStartHerePlanSummary) { - workspaceLog.debug( - "Skipping plan content injection for plan handoff transition: Start Here already includes the plan in chat history." - ); } return { effectiveAdditionalInstructions, planFilePath, planContentForTransition }; From 27e6a10965ea65905ed79166fe8ac05f55f3e309 Mon Sep 17 00:00:00 2001 From: "mux-bot[bot]" <264182336+mux-bot[bot]@users.noreply.github.com> Date: Sun, 3 May 2026 20:17:34 +0000 Subject: [PATCH 09/22] refactor: drop unused workspaceName param from parseRuntimeString MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The second argument was named `_workspaceName` (underscore-prefixed = unused) since it was introduced in the original SSH runtime PR and has never been referenced by the function body. The /new simplification (#3230) made the noise especially visible: the new `createNewWorkspace` call site had to pass `options.workspaceName ?? "(auto-generated)"` purely to satisfy the signature, and added a comment claiming `parseRuntimeString only uses the name for error reporting context` — but the function never reads it. Drop the parameter from the signature, the placeholder + misleading comment at the only non-test caller, and the boilerplate `workspaceName` constant + 23 second-arguments in `chatCommands.test.ts`. Pure dead-parameter cleanup — emitted JS, error messages, and runtime behavior are all identical. Mobile already had the cleaner shape (`parseRuntimeStringForMobile(runtime?: string)`) so the desktop signature now matches it. --- src/browser/utils/chatCommands.test.ts | 55 +++++++++++--------------- src/browser/utils/chatCommands.ts | 14 ++----- 2 files changed, 26 insertions(+), 43 deletions(-) diff --git a/src/browser/utils/chatCommands.test.ts b/src/browser/utils/chatCommands.test.ts index 64e257552c..84b0af5cac 100644 --- a/src/browser/utils/chatCommands.test.ts +++ b/src/browser/utils/chatCommands.test.ts @@ -53,27 +53,25 @@ beforeEach(() => { }); describe("parseRuntimeString", () => { - const workspaceName = "test-workspace"; - test("returns undefined for undefined runtime (default to worktree)", () => { - expect(parseRuntimeString(undefined, workspaceName)).toBeUndefined(); + expect(parseRuntimeString(undefined)).toBeUndefined(); }); test("returns undefined for explicit 'worktree' runtime", () => { - expect(parseRuntimeString("worktree", workspaceName)).toBeUndefined(); - expect(parseRuntimeString("WORKTREE", workspaceName)).toBeUndefined(); - expect(parseRuntimeString(" worktree ", workspaceName)).toBeUndefined(); + expect(parseRuntimeString("worktree")).toBeUndefined(); + expect(parseRuntimeString("WORKTREE")).toBeUndefined(); + expect(parseRuntimeString(" worktree ")).toBeUndefined(); }); test("returns local config for explicit 'local' runtime", () => { // "local" now returns project-dir runtime config (no srcBaseDir) - expect(parseRuntimeString("local", workspaceName)).toEqual({ type: "local" }); - expect(parseRuntimeString("LOCAL", workspaceName)).toEqual({ type: "local" }); - expect(parseRuntimeString(" local ", workspaceName)).toEqual({ type: "local" }); + expect(parseRuntimeString("local")).toEqual({ type: "local" }); + expect(parseRuntimeString("LOCAL")).toEqual({ type: "local" }); + expect(parseRuntimeString(" local ")).toEqual({ type: "local" }); }); test("parses valid SSH runtime", () => { - const result = parseRuntimeString("ssh user@host", workspaceName); + const result = parseRuntimeString("ssh user@host"); expect(result).toEqual({ type: "ssh", host: "user@host", @@ -82,7 +80,7 @@ describe("parseRuntimeString", () => { }); test("preserves case in SSH host", () => { - const result = parseRuntimeString("ssh User@Host.Example.Com", workspaceName); + const result = parseRuntimeString("ssh User@Host.Example.Com"); expect(result).toEqual({ type: "ssh", host: "User@Host.Example.Com", @@ -91,7 +89,7 @@ describe("parseRuntimeString", () => { }); test("handles extra whitespace", () => { - const result = parseRuntimeString(" ssh user@host ", workspaceName); + const result = parseRuntimeString(" ssh user@host "); expect(result).toEqual({ type: "ssh", host: "user@host", @@ -100,12 +98,12 @@ describe("parseRuntimeString", () => { }); test("throws error for SSH without host", () => { - expect(() => parseRuntimeString("ssh", workspaceName)).toThrow("SSH runtime requires host"); - expect(() => parseRuntimeString("ssh ", workspaceName)).toThrow("SSH runtime requires host"); + expect(() => parseRuntimeString("ssh")).toThrow("SSH runtime requires host"); + expect(() => parseRuntimeString("ssh ")).toThrow("SSH runtime requires host"); }); test("accepts SSH with hostname only (user will be inferred)", () => { - const result = parseRuntimeString("ssh hostname", workspaceName); + const result = parseRuntimeString("ssh hostname"); // Uses tilde path - backend will resolve it via runtime.resolvePath() expect(result).toEqual({ type: "ssh", @@ -115,7 +113,7 @@ describe("parseRuntimeString", () => { }); test("accepts SSH with hostname.domain only", () => { - const result = parseRuntimeString("ssh dev.example.com", workspaceName); + const result = parseRuntimeString("ssh dev.example.com"); // Uses tilde path - backend will resolve it via runtime.resolvePath() expect(result).toEqual({ type: "ssh", @@ -125,7 +123,7 @@ describe("parseRuntimeString", () => { }); test("uses tilde path for root user too", () => { - const result = parseRuntimeString("ssh root@hostname", workspaceName); + const result = parseRuntimeString("ssh root@hostname"); // Backend will resolve ~ to /root for root user expect(result).toEqual({ type: "ssh", @@ -135,7 +133,7 @@ describe("parseRuntimeString", () => { }); test("parses docker runtime with image", () => { - const result = parseRuntimeString("docker ubuntu:22.04", workspaceName); + const result = parseRuntimeString("docker ubuntu:22.04"); expect(result).toEqual({ type: "docker", image: "ubuntu:22.04", @@ -143,10 +141,7 @@ describe("parseRuntimeString", () => { }); test("parses devcontainer runtime with config path", () => { - const result = parseRuntimeString( - "devcontainer .devcontainer/devcontainer.json", - workspaceName - ); + const result = parseRuntimeString("devcontainer .devcontainer/devcontainer.json"); expect(result).toEqual({ type: "devcontainer", configPath: ".devcontainer/devcontainer.json", @@ -154,13 +149,13 @@ describe("parseRuntimeString", () => { }); test("throws error for devcontainer without config path", () => { - expect(() => parseRuntimeString("devcontainer", workspaceName)).toThrow( + expect(() => parseRuntimeString("devcontainer")).toThrow( "Dev container runtime requires a config path" ); }); test("parses docker with registry image", () => { - const result = parseRuntimeString("docker ghcr.io/myorg/dev:latest", workspaceName); + const result = parseRuntimeString("docker ghcr.io/myorg/dev:latest"); expect(result).toEqual({ type: "docker", image: "ghcr.io/myorg/dev:latest", @@ -168,19 +163,15 @@ describe("parseRuntimeString", () => { }); test("throws error for docker without image", () => { - expect(() => parseRuntimeString("docker", workspaceName)).toThrow( - "Docker runtime requires image" - ); - expect(() => parseRuntimeString("docker ", workspaceName)).toThrow( - "Docker runtime requires image" - ); + expect(() => parseRuntimeString("docker")).toThrow("Docker runtime requires image"); + expect(() => parseRuntimeString("docker ")).toThrow("Docker runtime requires image"); }); test("throws error for unknown runtime type", () => { - expect(() => parseRuntimeString("remote", workspaceName)).toThrow( + expect(() => parseRuntimeString("remote")).toThrow( "Unknown runtime type: 'remote'. Use 'ssh ', 'docker ', 'devcontainer ', 'worktree', or 'local'" ); - expect(() => parseRuntimeString("kubernetes", workspaceName)).toThrow( + expect(() => parseRuntimeString("kubernetes")).toThrow( "Unknown runtime type: 'kubernetes'. Use 'ssh ', 'docker ', 'devcontainer ', 'worktree', or 'local'" ); }); diff --git a/src/browser/utils/chatCommands.ts b/src/browser/utils/chatCommands.ts index fe758eb354..5ce359d384 100644 --- a/src/browser/utils/chatCommands.ts +++ b/src/browser/utils/chatCommands.ts @@ -1012,10 +1012,7 @@ async function handleForkCommand( * - "devcontainer " -> Dev container runtime * - undefined -> Worktree runtime (default) */ -export function parseRuntimeString( - runtime: string | undefined, - _workspaceName: string -): RuntimeConfig | undefined { +export function parseRuntimeString(runtime: string | undefined): RuntimeConfig | undefined { // Use shared parser from common/types/runtime const parsed = parseRuntimeModeAndHost(runtime); @@ -1128,13 +1125,8 @@ export async function createNewWorkspace( } } - // Parse runtime config if provided. Use a placeholder when no caller-provided - // workspace name is available (auto-name path); parseRuntimeString only uses - // the name for error reporting context. - const runtimeConfig = parseRuntimeString( - effectiveRuntime, - options.workspaceName ?? "(auto-generated)" - ); + // Parse runtime config if provided. + const runtimeConfig = parseRuntimeString(effectiveRuntime); const result = await options.client.workspace.create({ projectPath: options.projectPath, From f98d160b6f638bfb9060b3ce14dfc38e866eef63 Mon Sep 17 00:00:00 2001 From: "mux-bot[bot]" <264182336+mux-bot[bot]@users.noreply.github.com> Date: Tue, 5 May 2026 20:28:19 +0000 Subject: [PATCH 10/22] refactor: drop dead length guard in parseBedrockModelName secondPart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The early `dotParts.length < 2` return at the top of `parseBedrockModelName` already guarantees `dotParts.length >= 2` by the time we compute `secondPart`, so the `dotParts.length > 1 ? dotParts[1].toLowerCase() : ""` ternary's empty-string branch is unreachable. The Deepseek V4 commit (#3237) extended the surrounding logic but didn't introduce this asymmetry — `firstPart` on the line above already accesses `dotParts[0].toLowerCase()` without a guard, so the redundant ternary on `secondPart` was the odd one out. Inline to a direct `dotParts[1].toLowerCase()` access (matching `firstPart`'s shape) and capture the rationale in a one-line comment so a future reader doesn't reintroduce the guard. Pure dead-code cleanup — emitted JS, runtime behavior, and the existing 18-test `modelDisplay` suite (including the new DeepSeek + Bedrock cases) are all unchanged. --- src/common/utils/ai/modelDisplay.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/common/utils/ai/modelDisplay.ts b/src/common/utils/ai/modelDisplay.ts index 216176ec5f..431d89e917 100644 --- a/src/common/utils/ai/modelDisplay.ts +++ b/src/common/utils/ai/modelDisplay.ts @@ -237,8 +237,10 @@ function parseBedrockModelName(modelId: string): string | null { const knownVendors = ["anthropic", "amazon", "meta", "cohere", "mistral", "ai21"]; const knownRegionPrefixes = ["global", "us", "eu", "ap", "sa"]; + // The early `dotParts.length < 2` return above guarantees both indices exist here, + // so neither access needs a length guard. const firstPart = dotParts[0].toLowerCase(); - const secondPart = dotParts.length > 1 ? dotParts[1].toLowerCase() : ""; + const secondPart = dotParts[1].toLowerCase(); // Format is either: vendor.model or region.vendor.model const isVendor = knownVendors.includes(firstPart); From 9b1afa7c8cf22d08783b3b8641f2a6954559ee9f Mon Sep 17 00:00:00 2001 From: mux-auto-cleanup Date: Wed, 6 May 2026 05:09:15 +0000 Subject: [PATCH 11/22] refactor: extract isAgentTaskActiveStatus predicate in task_await MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dedupe the three call sites in task_await.ts that gated on the active agent-task subset (queued | running | awaiting_report). The duplication became especially visible in #3234, which extended both the timeout=0 and "timed out" branches symmetrically with `getAgentTaskElapsedField`, making the two structurally identical `{ status, taskId, ...elapsed }` returns sit a few lines apart from a third copy in the ForegroundWaitBackgroundedError branch that picked between the same three values. Add a local `isAgentTaskActiveStatus` type predicate (with the `AgentTaskActiveStatus` subset alias) at the top of the file alongside the existing `coerceTimeoutMs` / `parseTimestampMs` helpers and replace all three inline checks with a single call. The predicate narrows the nullable `AgentTaskStatus | null` return of `getAgentTaskStatus` to the active subset so the existing returns keep their narrowed `status` field without `as const` gymnastics. Pure refactor — emitted return shapes (including `elapsed_ms`), narrowed `status` literals, and the existing 17-test `task_await` suite all pass unchanged. --- src/node/services/tools/task_await.ts | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/node/services/tools/task_await.ts b/src/node/services/tools/task_await.ts index b8c6680039..5d05b28d69 100644 --- a/src/node/services/tools/task_await.ts +++ b/src/node/services/tools/task_await.ts @@ -15,10 +15,22 @@ import { import { getErrorMessage } from "@/common/utils/errors"; import { ForegroundWaitBackgroundedError, + type AgentTaskStatus, type AgentTaskStatusLookup, type AgentTaskTimestamps, } from "@/node/services/taskService"; +// Status values for which task_await still treats an agent task as live and +// should surface the live status (plus an `elapsed_ms` field) instead of +// awaiting a report. Centralised here so the timeout=0 and "timed out" error +// branches below stay in lockstep when shared fields are added — see #3234, +// which extended both branches symmetrically with `getAgentTaskElapsedField`. +type AgentTaskActiveStatus = "queued" | "running" | "awaiting_report"; + +function isAgentTaskActiveStatus(status: AgentTaskStatus | null): status is AgentTaskActiveStatus { + return status === "queued" || status === "running" || status === "awaiting_report"; +} + function coerceTimeoutMs(timeoutSecs: unknown): number | undefined { if (typeof timeoutSecs !== "number" || !Number.isFinite(timeoutSecs)) return undefined; if (timeoutSecs < 0) return undefined; @@ -247,7 +259,7 @@ export const createTaskAwaitTool: ToolFactory = (config: ToolConfiguration) => { // current task status instead of awaiting. if (timeoutMs === 0) { const status = taskService.getAgentTaskStatus(taskId); - if (status === "queued" || status === "running" || status === "awaiting_report") { + if (isAgentTaskActiveStatus(status)) { return { status, taskId, ...getAgentTaskElapsedField(taskId) }; } @@ -299,12 +311,9 @@ export const createTaskAwaitTool: ToolFactory = (config: ToolConfiguration) => { } catch (error: unknown) { if (error instanceof ForegroundWaitBackgroundedError) { const currentStatus = taskService.getAgentTaskStatus(taskId); - const normalizedStatus = - currentStatus === "queued" || - currentStatus === "running" || - currentStatus === "awaiting_report" - ? currentStatus - : ("running" as const); + const normalizedStatus = isAgentTaskActiveStatus(currentStatus) + ? currentStatus + : ("running" as const); return { status: normalizedStatus, taskId, @@ -323,7 +332,7 @@ export const createTaskAwaitTool: ToolFactory = (config: ToolConfiguration) => { } if (/timed out/i.test(message)) { const status = taskService.getAgentTaskStatus(taskId); - if (status === "queued" || status === "running" || status === "awaiting_report") { + if (isAgentTaskActiveStatus(status)) { return { status, taskId, ...getAgentTaskElapsedField(taskId) }; } if (!status) { From 0da96805e6c609a379c3fea502088b623d81c011 Mon Sep 17 00:00:00 2001 From: "mux-bot[bot]" <264182336+mux-bot[bot]@users.noreply.github.com> Date: Wed, 6 May 2026 20:34:05 +0000 Subject: [PATCH 12/22] refactor: extract detachLanguageModelCleanup helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit moveLanguageModelCleanup and runLanguageModelCleanup both implemented the same single-shot "read attached cleanup, delete the slot, return it" sequence inline (the same as LanguageModelWithCleanup cast + symbol read + delete). The new file from #3241 introduced both call sites with the duplication baked in. Extract a private detachLanguageModelCleanup() helper so the two surviving public functions read as their intent (move = re-attach to target, run = invoke + swallow) instead of repeating the slot-management plumbing. The behaviour is identical: detach is the only path that mutates the slot, callers take the same return-on-undefined branch they did before, and runLanguageModelCleanup keeps its existing try/catch around the invocation. Pure refactor — emitted JS, the symbol's single-shot semantics, and the existing 6-test languageModelCleanup suite are all unchanged. --- src/node/services/languageModelCleanup.ts | 24 ++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/src/node/services/languageModelCleanup.ts b/src/node/services/languageModelCleanup.ts index 1b96c9138e..b1fde61913 100644 --- a/src/node/services/languageModelCleanup.ts +++ b/src/node/services/languageModelCleanup.ts @@ -23,14 +23,24 @@ export function attachLanguageModelCleanup( modelWithCleanup[languageModelCleanupSymbol] = cleanup; } +// Single-shot pop: read the attached cleanup (if any) and clear the slot in one +// step so move/run callers can't accidentally leave a stale cleanup behind that +// would re-fire on a later run. +function detachLanguageModelCleanup(model: LanguageModel): LanguageModelCleanup | undefined { + const modelWithCleanup = model as LanguageModelWithCleanup; + const cleanup = modelWithCleanup[languageModelCleanupSymbol]; + if (cleanup === undefined) { + return undefined; + } + delete modelWithCleanup[languageModelCleanupSymbol]; + return cleanup; +} + export function moveLanguageModelCleanup(source: LanguageModel, target: LanguageModel): void { - const sourceWithCleanup = source as LanguageModelWithCleanup; - const cleanup = sourceWithCleanup[languageModelCleanupSymbol]; + const cleanup = detachLanguageModelCleanup(source); if (cleanup === undefined) { return; } - - delete sourceWithCleanup[languageModelCleanupSymbol]; attachLanguageModelCleanup(target, cleanup); } @@ -43,15 +53,11 @@ export function runLanguageModelCleanup(model: LanguageModel | undefined): void if (model === undefined) { return; } - - const modelWithCleanup = model as LanguageModelWithCleanup; - const cleanup = modelWithCleanup[languageModelCleanupSymbol]; + const cleanup = detachLanguageModelCleanup(model); if (cleanup === undefined) { return; } - delete modelWithCleanup[languageModelCleanupSymbol]; - try { cleanup(); } catch (error) { From a110cf0d5d3bb6e166feb40e64a1983d49950128 Mon Sep 17 00:00:00 2001 From: "mux-bot[bot]" <264182336+mux-bot[bot]@users.noreply.github.com> Date: Thu, 7 May 2026 12:36:40 +0000 Subject: [PATCH 13/22] refactor: derive TokenRecord from BrowserBridgeTokenPayload --- .../browser/BrowserBridgeTokenManager.ts | 27 +++++++++---------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/src/node/services/browser/BrowserBridgeTokenManager.ts b/src/node/services/browser/BrowserBridgeTokenManager.ts index 6678be4880..fb91d9ae3e 100644 --- a/src/node/services/browser/BrowserBridgeTokenManager.ts +++ b/src/node/services/browser/BrowserBridgeTokenManager.ts @@ -2,11 +2,18 @@ import { randomBytes } from "node:crypto"; import { assert } from "@/common/utils/assert"; import { log } from "@/node/services/log"; -interface TokenRecord { +export interface BrowserBridgeTokenPayload { workspaceId: string; sessionName: string; streamPort: number; allowOtherWorkspaceSession: boolean; +} + +// TokenRecord = the validated payload plus the TTL deadline; extending the +// payload type keeps the field list in one place so a future payload addition +// (e.g. a new scoping flag) cannot drift between the stored record, the mint +// input, and the validate-time rebuild below. +interface TokenRecord extends BrowserBridgeTokenPayload { expiresAtMs: number; } @@ -14,13 +21,6 @@ interface BrowserBridgeTokenMintOptions { allowOtherWorkspaceSession?: boolean; } -export interface BrowserBridgeTokenPayload { - workspaceId: string; - sessionName: string; - streamPort: number; - allowOtherWorkspaceSession: boolean; -} - const BROWSER_BRIDGE_TOKEN_TTL_MS = 30_000; const CLEANUP_INTERVAL_MS = 60_000; @@ -76,12 +76,11 @@ export class BrowserBridgeTokenManager { return null; } - return { - workspaceId: record.workspaceId, - sessionName: record.sessionName, - streamPort: record.streamPort, - allowOtherWorkspaceSession: record.allowOtherWorkspaceSession, - }; + // Strip the TTL deadline; rest-spread keeps the payload field list driven + // by BrowserBridgeTokenPayload so adding a payload field doesn't require a + // matching edit here. + const { expiresAtMs, ...payload } = record; + return payload; } private cleanupExpired(): void { From ed6ec3343425101a88ae6412f593687cbd588eff Mon Sep 17 00:00:00 2001 From: "mux-bot[bot]" <264182336+mux-bot[bot]@users.noreply.github.com> Date: Thu, 7 May 2026 16:38:04 +0000 Subject: [PATCH 14/22] refactor: extract renameAliasField helper for bash tool preprocess MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bash tool's z.preprocess shim normalizes quirky model emissions to canonical field names. After the DeepSeek v4 fix in #3247 added a second 'description' → 'display_name' rename block (mirroring the existing 'command' → 'script' rename), the two blocks were structurally identical: skip when canonical is already a string, drop the alias via destructure, re-spread with the canonical name. Each alias still required its own 'as Record & { : string }' cast plus the same three-line destructure/spread. Extract a private renameAliasField(obj, alias, canonical) helper that captures the rename pattern in one place, with the rationale for why aliases exist (and why they stay undocumented) noted on the helper. The call site collapses to two named lines that read as the intent ('rename command to script', 'rename description to display_name') instead of fifteen lines of mostly-identical destructure/spread. Pure refactor — emitted JS, the canonical-field-wins precedence, the 'no-op when neither alias nor canonical is a string' branches, and the existing 36-test toolDefinitions suite (including the four new command/description alias precedence cases) are all unchanged. --- src/common/utils/tools/toolDefinitions.ts | 44 ++++++++++++++--------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/src/common/utils/tools/toolDefinitions.ts b/src/common/utils/tools/toolDefinitions.ts index fb92354969..3af50a6776 100644 --- a/src/common/utils/tools/toolDefinitions.ts +++ b/src/common/utils/tools/toolDefinitions.ts @@ -854,6 +854,26 @@ export const ProposeStatusToolArgsSchema = z.object({ const MuxConfigFileSchema = z.enum(["providers", "config"]); +/** + * Rename a string-typed alias field to its canonical name on a plain object, + * dropping the alias to keep downstream tool args canonical. No-op if the + * canonical field is already a string or the alias is missing/non-string. + * + * Used by the bash tool's `preprocess` to normalize quirky model emissions + * (e.g. `command` → `script`, `description` → `display_name`) without + * duplicating the same destructure/spread shape per alias. + */ +function renameAliasField( + obj: Record, + alias: string, + canonical: string +): Record { + if (typeof obj[canonical] === "string") return obj; + if (typeof obj[alias] !== "string") return obj; + const { [alias]: aliasValue, ...rest } = obj; + return { ...rest, [canonical]: aliasValue }; +} + /** * Tool definitions: single source of truth * Key = tool name, Value = { description, schema } @@ -869,25 +889,17 @@ export const TOOL_DEFINITIONS = { "On Windows this runs in Git Bash; to discard output use `>/dev/null` (not `>nul`).", schema: z.preprocess( (value) => { - // Compatibility: some models emit { command: "..." } instead of { script: "..." }. - // Normalize to `script` so downstream code (tool runner + UI) stays consistent. + // Compatibility shims for models that emit alias fields: + // - some models emit `command` instead of `script` + // - DeepSeek v4 emits `description` instead of `display_name` + // Normalize both so downstream code (tool runner + UI) sees canonical args. + // Aliases are intentionally undocumented in the public schema; we don't + // want to invite other models to use the wrong field. if (typeof value !== "object" || value === null || Array.isArray(value)) return value; let obj = value as Record; - - if (typeof obj.script !== "string" && typeof obj.command === "string") { - // Drop the legacy field to keep tool args canonical (and avoid confusing downstream consumers). - const { command, ...rest } = obj as Record & { command: string }; - obj = { ...rest, script: command }; - } - - // Compatibility: DeepSeek v4 emits `description` instead of `display_name`. - // Treat `description` as an undocumented alias so the call still validates. - if (typeof obj.display_name !== "string" && typeof obj.description === "string") { - const { description, ...rest } = obj as Record & { description: string }; - obj = { ...rest, display_name: description }; - } - + obj = renameAliasField(obj, "command", "script"); + obj = renameAliasField(obj, "description", "display_name"); return obj; }, z.object({ From 492d45f21a7e31e931346eb64b67d42e7a27d3af Mon Sep 17 00:00:00 2001 From: "mux-bot[bot]" <264182336+mux-bot[bot]@users.noreply.github.com> Date: Thu, 7 May 2026 20:31:02 +0000 Subject: [PATCH 15/22] refactor: collapse ReviewPanel selection-validity branches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both arms of the immersive vs non-immersive selection-validity check in ReviewPanel did the same shape: `selectedHunkId && X.some(...)`, then `setSelectedHunkId(filteredHunks[0].id)` if false. Only `X` differed (`hunks` for immersive, `filteredHunks` otherwise). Pick the validity list up front and run a single check so both modes stay in lockstep, preserving the immersive-aware behavior added in #3249. Pure refactor — emitted JS, the early-return on `filteredHunks.length === 0`, the effect's dependency list, and the existing 5-test ImmersiveReviewView suite (including the two #3249 regression tests) are all unchanged. --- .../RightSidebar/CodeReview/ReviewPanel.tsx | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/browser/features/RightSidebar/CodeReview/ReviewPanel.tsx b/src/browser/features/RightSidebar/CodeReview/ReviewPanel.tsx index 202b9802a9..9ca187e0d6 100644 --- a/src/browser/features/RightSidebar/CodeReview/ReviewPanel.tsx +++ b/src/browser/features/RightSidebar/CodeReview/ReviewPanel.tsx @@ -1259,7 +1259,7 @@ export const ReviewPanel: React.FC = ({ filteredHunksRef.current = filteredHunks; // Ensure selectedHunkId is valid after filtering/sorting: - // - If no selection or selection not in filtered list, select first visible hunk + // - If no selection or selection not in the validity list, select first visible hunk // - This runs after sorting, so we always select the top-most hunk in current order // // Immersive review can intentionally navigate to a hunk that is hidden by @@ -1272,15 +1272,11 @@ export const ReviewPanel: React.FC = ({ useEffect(() => { if (filteredHunks.length === 0) return; - if (isImmersive) { - const selectionExists = selectedHunkId && hunks.some((h) => h.id === selectedHunkId); - if (!selectionExists) { - setSelectedHunkId(filteredHunks[0].id); - } - return; - } - - const selectionValid = selectedHunkId && filteredHunks.some((h) => h.id === selectedHunkId); + // Picking the validity list up front keeps the immersive and non-immersive + // behavior in lockstep — the only difference is which list we accept the + // current selection against. + const validityList = isImmersive ? hunks : filteredHunks; + const selectionValid = selectedHunkId && validityList.some((h) => h.id === selectedHunkId); if (!selectionValid) { setSelectedHunkId(filteredHunks[0].id); } From 828f815c7bf19c718f95c1159a74d28c4b347c1e Mon Sep 17 00:00:00 2001 From: ammar-agent Date: Sat, 9 May 2026 20:17:08 +0000 Subject: [PATCH 16/22] refactor: collapse FileEditToolCall preview gating into single nullable Replace the `shouldShowLargeDiffPreview` boolean + repeated `&& largeDiffPreview` checks with a single nullable `activeDiffPreview` handle so JSX truthiness checks narrow the type directly. Behavior is unchanged; this just removes the redundant `Boolean()` cast and the duplicated truthiness guards that TypeScript's narrowing couldn't flow through. --- src/browser/features/Tools/FileEditToolCall.tsx | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/browser/features/Tools/FileEditToolCall.tsx b/src/browser/features/Tools/FileEditToolCall.tsx index 915d94424a..0d34f82343 100644 --- a/src/browser/features/Tools/FileEditToolCall.tsx +++ b/src/browser/features/Tools/FileEditToolCall.tsx @@ -168,7 +168,9 @@ export const FileEditToolCall: React.FC = ({ const diff = result && result.success ? (uiOnlyDiff ?? result.diff) : undefined; const filePath = extractToolFilePath(args); const largeDiffPreview = diff ? buildLargeDiffPreview(diff) : null; - const shouldShowLargeDiffPreview = Boolean(largeDiffPreview && !showRaw && !showFullDiff); + // Single nullable handle for the active preview so JSX truthiness checks narrow the type + // directly (no separate boolean + repeated `&& largeDiffPreview` guards). + const activeDiffPreview = largeDiffPreview && !showRaw && !showFullDiff ? largeDiffPreview : null; // Copy to clipboard with feedback const { copied, copyToClipboard } = useCopyToClipboard(); @@ -242,12 +244,12 @@ export const FileEditToolCall: React.FC = ({ {result.success && diff && ( <> - {shouldShowLargeDiffPreview && largeDiffPreview && ( + {activeDiffPreview && (
Large diff preview: showing{" "} - {largeDiffPreview.displayedLines.toLocaleString()} of{" "} - {largeDiffPreview.totalLines.toLocaleString()} lines. Full patch is still + {activeDiffPreview.displayedLines.toLocaleString()} of{" "} + {activeDiffPreview.totalLines.toLocaleString()} lines. Full patch is still available from the menu.