From 6642cd023ec6998a865543945feed8a6dbc6d79c 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/15] 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 c5fdc8b5a5c4e37e14d7daa76db82d9861aa3470 Mon Sep 17 00:00:00 2001 From: mux-bot Date: Fri, 1 May 2026 12:22:54 +0000 Subject: [PATCH 02/15] 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 b675a5e9d5572a9f2d4bf75631e8a6eb0628a2fe Mon Sep 17 00:00:00 2001 From: mux Date: Fri, 1 May 2026 16:23:22 +0000 Subject: [PATCH 03/15] 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 e9fe85052ba08dd628f4b919fa1a79a1cae7695c 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/15] 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 bea5c35487..90ec917d5a 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -88,6 +88,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; @@ -382,11 +393,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, "") @@ -400,8 +411,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; @@ -450,8 +461,8 @@ export class TaskService { parentWorkspaceId: string, parentEntry: { workspace: { - aiSettingsByAgent?: Record; - aiSettings?: { model: string; thinkingLevel?: ThinkingLevel }; + aiSettingsByAgent?: Record; + aiSettings?: ResolvedWorkspaceAiSettings; }; }, fallbackModel: string, From 045531a4301ad1862acc17fcbf1b6e8fa474e1da 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/15] 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 515e5f3a70965928bd2aa24e3a5274cde3758c95 Mon Sep 17 00:00:00 2001 From: ammar-agent Date: Sat, 2 May 2026 20:18:11 +0000 Subject: [PATCH 06/15] 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 76987e5bb6..655e924e4f 100644 --- a/src/browser/utils/messages/StreamingMessageAggregator.ts +++ b/src/browser/utils/messages/StreamingMessageAggregator.ts @@ -27,6 +27,7 @@ import { type StreamLifecycleSnapshot, } from "@/common/types/stream"; 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"; @@ -3064,20 +3065,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 @@ -3090,19 +3108,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 53023f9a502a8c26120fcb1363cc0a1e84e37978 Mon Sep 17 00:00:00 2001 From: ammar-agent Date: Sun, 3 May 2026 05:10:00 +0000 Subject: [PATCH 07/15] 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 1cb708f63fa59f2bc85610cdbc74d964ee0d0559 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/15] 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 3f2ae1265cd9e0ca427c49142b88bae473c19e1d 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/15] 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 5af6130512..7a467f3b8c 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 bb51128e7d..a035b95a32 100644 --- a/src/browser/utils/chatCommands.ts +++ b/src/browser/utils/chatCommands.ts @@ -685,10 +685,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); @@ -801,13 +798,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 ce9416c220ba75347342d6bbdc1d678f51edbdb7 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/15] 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 cace9693bc423297bf10497a5bb805b1a834fbd7 Mon Sep 17 00:00:00 2001 From: mux-auto-cleanup Date: Wed, 6 May 2026 05:09:15 +0000 Subject: [PATCH 11/15] 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 f5aaf9e7722b3274485fa889888d0d02af0a3f22 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/15] 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 c9012e954ddaa1fa3263ece827064d6a61cfaacf 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/15] 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 1b21692b567f9e0694c283691aa452ce1d33196c 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/15] 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 bd5439e5a2..c8f606ddfa 100644 --- a/src/common/utils/tools/toolDefinitions.ts +++ b/src/common/utils/tools/toolDefinitions.ts @@ -833,6 +833,26 @@ export const ProposeNameToolArgsSchema = 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 } @@ -848,25 +868,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 62aefc8c20fce52e1814d0b8a814f7ad81eb5a30 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/15] 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 703c6187e8..ff0eb276d8 100644 --- a/src/browser/features/RightSidebar/CodeReview/ReviewPanel.tsx +++ b/src/browser/features/RightSidebar/CodeReview/ReviewPanel.tsx @@ -1246,7 +1246,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 @@ -1259,15 +1259,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); }