🤖 refactor: auto-cleanup

[mux-bot]

Summary

Long-lived auto-cleanup PR that accumulates low-risk, behavior-preserving refactors picked from recent main commits.

Changes

Drop dead ?? [] fallback for sanitizeSchema.tagNames

The JSX-tag preservation plugin added in #3256 built an ALLOWED_RAW_HTML_TAG_NAMES Set by lowercasing sanitizeSchema.tagNames, guarded with a (sanitizeSchema.tagNames ?? []).map(...) fallback. sanitizeSchema is constructed locally in the same module via { ...defaultSchema, tagNames: [...] } with a concrete array literal — after the spread/override, tagNames is unconditionally a non-empty string[], so the outer ?? [] fallback can never fire and only serves to obscure that invariant from future readers.

Drop the outer fallback and add a comment capturing the rationale (the inner ...(defaultSchema.tagNames ?? []) spread still guards the library type, which is genuinely optional). A future refactor that touches the schema construction now has to spell out any change that could legitimately make tagNames undefined, instead of inheriting the dead guard by copy-paste.

Pure refactor — emitted JS, the rehypePreserveUnknownRawHtml plugin's behavior (parsed via the same lowercased Set), the unknown-tag-as-text preservation semantics added in #3256, and the existing MarkdownRenderer.raw-html test suite are all unchanged.

Previous cleanups

Extract readInlineHeightPx helper in useAutoResizeTextarea

The chat input auto-resize stabilization in #3263 added a second Number.parseFloat(el.style.height) + Number.isFinite(...) pair to verify that the cached pixel height still reflects the textarea's inline style, sitting a handful of lines from the existing pair in the canOnlyGrow first-render fallback. Both spell out the same intent — read the textarea's inline height style as a finite px number, or treat it as missing — but as two independent expressions they were one stray edit away from drifting on what counts as a usable inline height (e.g. how to handle an empty string, a non-px unit, or the NaN that parseFloat returns for either).

Extract a private readInlineHeightPx(el): number | null helper that captures the parseFloat + isFinite pair in one place, with the rationale captured on the helper itself so a future caller (e.g. another resize-coordination path that also needs to read the inline height) doesn't reintroduce the duplicated check. The two call sites collapse to a single named call, and the surviving branches replace Number.isFinite(appliedHeight) / Number.isFinite(currentInlineHeight) with !== null checks (functionally identical because the helper already filters non-finite values, plus TypeScript narrows number | null cleanly without a separate type guard).

Pure refactor — emitted writes, the canOnlyGrow grow-only semantics, the post-resize "restore the px height after auto writes or external clears" verification added in #3263, and the existing 5-test useAutoResizeTextarea suite (including the regression cases for capped large-text deletion and external inline-height clears added in #3263) are all unchanged.

Extract settleOnTranscript helper in agentStatusService

The new AgentStatusService.runForWorkspace (added by the AI-generated sidebar status loop in #3238) had the same two-line pair — markRecencyObserved() to consume any observed recency bump, then state.lastInputHash = inputHash to advance the dedup hash — repeated across the three branches that produce a definitive outcome for the current transcript: post-provider failure (model refused tool / rate limit / persistent provider error), placeholder rejection (defense-in-depth filter for generic "Awaiting next task" output), and successful persist. The other three branches (empty workspace, idle/frozen dedup hit, pre-provider auth/config failure) intentionally call only markRecencyObserved() because they should still retry against the same transcript when conditions change.

Hoist the duplicated pair into a settleOnTranscript() closure declared next to the existing markRecencyObserved closure inside runForWorkspace, with a comment capturing why the three settlement branches advance the dedup hash and the three retry branches don't, so a future reader doesn't reintroduce the duplicated pair (or accidentally swap one for the other). The three settlement call sites collapse to a single named call, and the three retry call sites keep their bare markRecencyObserved() shape.

Pure refactor — emitted JS, the six branch behaviors (settle vs retry), the dedup-hash semantics documented on the lastInputHash field, and the existing 25-test agentStatusService suite (including the post-provider / placeholder / success / pre-provider / pre-provider-retry / dedup regression cases from #3238) are all unchanged.

Collapse ReviewPanel selection-validity branches

ReviewPanel.tsx's post-filter selection-validity effect (touched by the immersive sidebar fix in #3249) split into two structurally identical arms: an isImmersive branch that validated selectedHunkId against hunks (the unfiltered diff) and a non-immersive branch that validated against filteredHunks. Both arms then ran the same setSelectedHunkId(filteredHunks[0].id) reset on miss, with the immersive arm needing its own early return to avoid double-running the reset. The duplication invited drift between the two modes — e.g. a future hide-read-style filter that adjusts the immersive reset would have to keep both arms in lockstep by hand.

Pick the validity list up front (const validityList = isImmersive ? hunks : filteredHunks;) and run a single selectedHunkId && validityList.some(...) check, with a short comment capturing the rationale so a future reader doesn't reintroduce the split. The early-return on filteredHunks.length === 0, the dependency list ([filteredHunks, hunks, isImmersive, selectedHunkId, setSelectedHunkId]), and the immersive-aware "only reset when the hunk has truly disappeared from the diff" semantics added in #3249 are all preserved.

Pure refactor — emitted JS, the selection-reset trigger conditions, and the existing 5-test ImmersiveReviewView suite (including the two immersive regression tests added in #3249) are all unchanged.

Extract renameAliasField helper for bash tool preprocess

The bash tool's z.preprocess shim (in src/common/utils/tools/toolDefinitions.ts) 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, unknown> & { <alias>: 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. A future alias (e.g. another quirky-model field) becomes a one-line addition.

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 command/description alias precedence cases added in #3247) are all unchanged.

Derive TokenRecord from BrowserBridgeTokenPayload

BrowserBridgeTokenManager (in src/node/services/browser/BrowserBridgeTokenManager.ts, touched by the new other-workspace session picker in #3243) declared two structurally identical interfaces side by side: a private TokenRecord (the stored token state) and an exported BrowserBridgeTokenPayload (the validate() return shape), with TokenRecord differing only by the extra expiresAtMs: number deadline. validate() then rebuilt the payload by listing each of the four shared fields a third time. The #3243 commit made this drift surface especially visible — adding allowOtherWorkspaceSession required parallel edits in TokenRecord, BrowserBridgeTokenPayload, the mint() insert, and the validate() rebuild.

Collapse TokenRecord to extends BrowserBridgeTokenPayload (with the rationale captured in a short comment so a future reader doesn't reintroduce the duplicated field list) and rewrite the validate() rebuild as a rest-spread destructure (const { expiresAtMs, ...payload } = record; return payload;). The eslint config already enables ignoreRestSiblings: true, so the unused expiresAtMs binding doesn't trigger no-unused-vars. A future payload field (e.g. another scoping flag) now lives in exactly one place — the exported BrowserBridgeTokenPayload interface — and automatically flows through both the stored record shape and the validate-time rebuild.

Pure refactor — emitted JS, the TTL semantics, the null-on-expired path, and the existing 5-test BrowserBridgeTokenManager suite (including the new "preserves explicit other-workspace session scope" case) are all unchanged.

Extract detachLanguageModelCleanup helper in languageModelCleanup

moveLanguageModelCleanup and runLanguageModelCleanup (in the new src/node/services/languageModelCleanup.ts from the OpenAI WebSocket transport opt-in PR #3241) both implemented the same "look up the attached cleanup, delete the symbol slot, return it" sequence inline. The duplication is structurally identical — same LanguageModelWithCleanup cast, same symbol read, same delete — so both call sites were one stray edit away from drifting on whether the slot gets cleared before or after the cleanup runs (which matters because attachLanguageModelCleanup asserts the slot is empty).

Extract a private detachLanguageModelCleanup helper that does the single-shot pop in one place and returns the cleanup (or undefined). Both surviving public functions reduce to their intent: move re-attaches the popped cleanup to the target, and run invokes it inside the existing try/catch that swallows + logs failures. A short comment captures the rationale on the helper itself so the next caller (e.g. a future "cancel without invoking" path) can't accidentally leak a slot.

Pure refactor — emitted JS, the symbol's single-shot semantics, and the existing 6-test languageModelCleanup suite are all unchanged.

Extract isAgentTaskActiveStatus predicate in task_await

Three call sites in src/node/services/tools/task_await.ts gated on the active agent-task subset ("queued" | "running" | "awaiting_report") by inlining the three-arm equality check. The new task_await elapsed-timing commit (#3234) extended both the timeoutMs === 0 branch and the timed out catch-branch symmetrically with ...getAgentTaskElapsedField(taskId), making the two structurally identical { status, taskId, ...elapsed } returns sit a handful of lines from a third copy in the ForegroundWaitBackgroundedError branch that picks between the same three values to coalesce against a "running" fallback.

Add a local AgentTaskActiveStatus subset alias plus an isAgentTaskActiveStatus type predicate near the existing coerceTimeoutMs / parseTimestampMs helpers, and collapse all three inline checks to a single call. The predicate narrows the nullable AgentTaskStatus | null return of getAgentTaskStatus to the active subset, so the surviving { status, taskId, ... } returns keep their narrowed status field with no as const gymnastics. The new comment captures the rationale so a future field added to one of the active-status branches won't silently drift away from the others.

Pure refactor — emitted return shapes (including elapsed_ms), narrowed status literals, and the existing 17-test task_await suite are all unchanged.

Drop dead length guard in parseBedrockModelName secondPart

The early dotParts.length < 2 return at the top of parseBedrockModelName (in src/common/utils/ai/modelDisplay.ts) already guarantees dotParts.length >= 2 by the time secondPart is computed, which makes the dotParts.length > 1 ? dotParts[1].toLowerCase() : "" ternary's empty-string branch unreachable. The DeepSeek V4 commit (#3237) extended the surrounding formatter without touching this site, but the new DeepSeek branch made the asymmetry more obvious — the line directly above (firstPart) already accesses dotParts[0].toLowerCase() without a guard, so the 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 + existing Bedrock cases) are all unchanged.

Drop unused workspaceName param from parseRuntimeString

The second argument to parseRuntimeString (in src/browser/utils/chatCommands.ts) was named _workspaceName (underscore-prefixed = unused) when it was introduced in the original SSH runtime PR, and has never been referenced by the function body — error messages are all generic and don't include any workspace-specific context.

The /new-mirrors-/fork 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. Mobile already had the cleaner shape (parseRuntimeStringForMobile(runtime?: string)).

Drop the parameter from the signature, drop the placeholder + misleading comment at the only non-test caller (createNewWorkspace), and drop the workspaceName = "test-workspace" constant + 23 second-arguments in chatCommands.test.ts. Pure dead-parameter cleanup — emitted JS, error messages, and runtime behavior are all identical, and the desktop signature now matches mobile's.

Drop redundant isPlanHandoffAgent boolean in streamContextBuilder

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.

Extract seedScrollDirectionBaseline helper in useAutoScroll

useAutoScroll (touched by #3226) seeds lastScrollTopRef from two call sites — jumpToBottom and disableAutoScroll — to keep handleScroll's released-branch direction check (currentScrollTop > previousScrollTop) honest after a programmatic ownership transfer that may not produce a follow-up scroll event. Both sites repeated the same write (lastScrollTopRef.current = contentRef.current?.scrollTop ?? 0) and a multi-line block comment re-explaining the same shared rationale.

Extract a seedScrollDirectionBaseline useCallback with the shared rationale captured once on the helper itself. Each call site reduces to a single call plus a one-line comment naming the site-specific reason it doesn't get a free scroll-event refresh (stickToBottom skips the write at max; disableAutoScroll never fires a scroll event itself). The dependency arrays for jumpToBottom and disableAutoScroll add the new helper, which has empty deps (useCallback(..., [])) so its identity is stable across renders — no extra re-creations of the surrounding callbacks.

This shrinks the surface area for future drift: a third path that needs to flip autoScrollRef/programmaticDisableRef without a guaranteed scroll-event tail (e.g. the deferred workspace-switch hydration race called out in #3226's "Pains") can call the helper instead of duplicating the rationale a third time. Pure refactor — emitted writes, write order, and ref values are identical, and the existing 25-test useAutoScroll suite (including the 6 new regression tests added in #3226) passes unchanged.

Extract pushStreamErrorRow helper in StreamingMessageAggregator

buildDisplayedMessagesForMessage now has two branches that synthesize a stream-error DisplayedMessage: the existing message.metadata?.error path and the finishReason === "length" path added in #3223. Both pushes were structurally identical, differing only in id suffix, error string, and errorType — the parent-message-derived fields (historyId, historySequence, model, routedThroughGateway, timestamp) were duplicated across both call sites.

Extract a local pushStreamErrorRow closure that captures the shared fields once. Each branch reduces to a single call passing the three differing values. The model access switches from message.metadata.model (which relied on the outer if (message.metadata?.error) narrowing) to message.metadata?.model, which is functionally identical when metadata is defined and falls through to undefined otherwise — same emitted value either way.

This shrinks the surface area for future drift: the next branch added (e.g. a different finish-reason synthesis) can't accidentally pass a stale model or forget routedThroughGateway. Pure refactor — emitted DisplayedMessage objects are identical, and the existing 77-test SMA suite (including the 6 new max-tokens tests) passes unchanged.

Drop redundant GuardAnchors type alias in file_edit_insert

In src/node/services/tools/file_edit_insert.ts, GuardAnchors was defined as Pick<InsertContentOptions, "insert_before" | "insert_after">, but InsertContentOptions itself is already Pick<FileEditInsertToolArgs, "insert_before" | "insert_after"> after the .nullish() strict-mode refactor in #2250 stripped the InsertContentOptions interface down to those same two fields. The two aliases became 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 already convey "guard" context, so the parameter type doesn't need to repeat it. This noise was especially visible to the new guardless-empty-file path added in #3220 since it sits next to insertContent which uses InsertContentOptions.

Validation

• bun test src/browser/hooks/useAutoResizeTextarea.test.tsx — 5/5 pass (covers both call sites the helper extraction touches: the canOnlyGrow first-render fallback path and the post-resize verification path that repairs an externally-cleared inline height, including the two regression cases added in 🤖 fix: stabilize chat input auto-resize height #3263 for capped large-text deletion and external inline-height clears).
• make static-check — eslint, tsgo (×2), prettier all pass; only shfmt fails because the binary is unavailable in this environment, and no shell files are touched.

Risks

Minimal — purely a local helper extraction inside useAutoResizeTextarea.ts. readInlineHeightPx() is a literal one-line hoist of Number.parseFloat(el.style.height) with the existing Number.isFinite(...) filter folded in, and the call sites replace Number.isFinite(x) checks with x !== null checks against the helper's narrowed number | null return. No effect dependencies, ref shapes, or write order change. There's no IPC, persisted-state, or layout-coupling risk.

---

Auto-cleanup checkpoint: bd02fc9

---

Generated with mux • Model: anthropic:claude-opus-4-7 • Thinking: xhigh

[mux-bot]

⚠️ Auto-fixup could not resolve CI failure: flaky tests, not caused by the cleanup commit.

Failed job: Test / Integration (run 25244368462)

Failures (both in tests/ui/compaction/compaction.test.ts):

1. force compaction triggers during streaming — timed out at 120000 ms waiting for "Compaction boundary" transcript text.
2. /compact with Ctrl+Enter (turn-end) does NOT auto-background foreground bash — waitFor in tests/ui/helpers.ts:144 could not locate the workspace in the sidebar.

What I checked:

• The PR diff is type-only refactors (extract ResolvedWorkspaceAiSettings / ServiceTier aliases, drop unused GuardAnchors / appendSpace literals, route ThemeContext color-scheme through isLightThemeMode). None of it touches compaction, streaming, the workspace consumer manager, or the test harness.
• Ran both failing tests locally on 1312f5a68:
  • force compaction triggers during streaming → PASS in 6.7s.
  • /compact with Ctrl+Enter ... → PASS in 4.7s.
• The streaming-compaction test has a 120s budget and clearly hit a slow/contended runner; the workspace-not-found timeout looks like the usual sidebar race in the harness.

No fix pushed. A re-run of Test / Integration should clear it; if it recurs on this PR or others, the test harness itself likely needs a stability pass (separate from this cleanup PR).

[mux-bot]

⚠️ Auto-fixup: CI failure looks like a flaky integration test, not caused by the cleanup commits.

Failed job: Test / Integration (run 25400653486)

Failing test: tests/ui/compaction/compaction.test.ts › force compaction triggers during streaming — timed out at 120s waiting for "Compaction boundary" to appear in the transcript. The next test in the same file cascaded into "Project not found in sidebar" (broken state from the prior timeout).

Why this looks flaky, not regression-caused:

• None of the files touched by this PR are in the compaction code path. The diff is pure type/dead-code cleanup in ThemeContext, useAutoScroll, StreamingMessageAggregator (stream-error row helper extract — no behavioral change), streamContextBuilder (drop unused isPlanHandoffAgent bool), taskService, file_edit_insert, provider config, slash command suggestions, chatCommands, and cli/run.ts.
• This branch's prior PR-workflow runs (May 1, May 2, May 3) all passed with the same nine refactor commits already on the branch. The only commit added since the last green run is 5f8c84bab refactor: drop dead length guard in parseBedrockModelName secondPart, which is unrelated to compaction.
• tests/ui/compaction/compaction.test.ts has a documented history of stabilization patches (88c99f08c, 5dc32b039, 355e0007c).
• The failure mode is a 120s timeout, typical of CI runner contention rather than a logic regression.

No fix pushed — recommend re-running the failed job (gh run rerun 25400653486 --failed). If it reproduces deterministically, it's worth investigating whether one of today's main-branch merges (DeepSeek V4 support, the perf revert, or the Harbor CI change) destabilized this scenario, since the auto-cleanup branch picked those up via rebase.

[mux-bot]

⚠️ Auto-fixup could not resolve CI failure: flaky integration test, not caused by the cleanup commits.

What failed (run 25684672492):

• Test / Integration → tests/ipc/config/modelNotFound.test.ts › should classify OpenAI 400 model_not_found as model_not_found (not retryable) (1 failed / 627 passed / 51 skipped after retries; same suite logged RETRY 1 against multiple live-provider tests).
• The assertion expect(errorEvent).toBeDefined() failed because no stream-error event was received within the 10 s window after hitting the real OpenAI API with a synthetic non-existent model id.

Why it's not from this PR:
None of the 22 files this PR touches are in the model-error classification path. The relevant files were not modified by any of the cleanup commits:

• src/node/services/utils/sendMessageError.ts
• src/node/services/streamManager.ts
• src/common/utils/messages/retryEligibility.ts
• src/common/orpc/schemas/errors.ts

The cleanup diff is pure refactor (extract helpers, inline single-use booleans, drop dead length guards, share a ServiceTier type alias, etc.) with no behavior change on the streaming/error-emission paths.

What I tried:

1. Pulled the failed-run logs via extract_pr_logs.sh 25684672492.
2. Confirmed only Test / Integration + the dependent Required gate failed; Static Checks, unit, E2E (linux + macos), Storybook, server smoke, Windows, and all builds passed.
3. Diffed every file in the PR against main (bd02fc99..421bd9e8) and grepped for any keyword in the failing test's path (model_not_found, errorType, stream-error, retryEligibility). The only matches are inside StreamingMessageAggregator.ts, which is a renderer-side helper extraction (pushStreamErrorRow) and cannot affect IPC stream-error events emitted from the backend.
4. Cross-checked that the test target is gpt-nonexistent-model-xyz123 against the live OpenAI endpoint with a 10 s timeout — a known flaky pattern that the suite already retries.

Recommended action: re-run Test / Integration (or push an empty commit). Manual intervention only needed if the failure reproduces after a re-run.