🤖 refactor: auto-cleanup#3291
Open
mux-bot[bot] wants to merge 25 commits into
Open
Conversation
mux-bot
Bot
force-pushed
the
auto-cleanup
branch
11 times, most recently
from
2ca21da to
31a3fa8
Compare
Contributor
Author
|
Failure: Why this is a flake, not caused by the cleanup commit:
Action: No fix pushed. Recommend retrying the Windows job; if it persists, the Windows ready-wait may need to be bumped above 900ms or the assertion further hardened (separate from this cleanup PR). |
mux-bot
Bot
force-pushed
the
auto-cleanup
branch
8 times, most recently
from
f4a79eb to
0a996c2
Compare
Contributor
Author
|
Symptom (run Root cause: Why this isn't the cleanup PR's fault:
Existing fixes on other branches (not yet on
Recommendation: cherry-pick |
mux-bot
Bot
force-pushed
the
auto-cleanup
branch
2 times, most recently
from
a936fc2 to
c13b6a1
Compare
Contributor
Author
|
Symptom (run
Why this is not caused by the cleanup commits
Action
|
Both image_generate and image_edit duplicated the same "Adjust Settings > Experiments > Image Tools or request fewer images." setup hint literal when the requested image count exceeded the configured maxImagesPerCall. Hoist it next to the existing IMAGE_TOOL_PROVIDER_SETUP_HINT in imageArtifacts.ts so the guidance has a single source of truth, matching the pattern established by the prior auto-cleanup PR.
Move `urlAttributes`/`blockedSchemes` out of `sanitizeMermaidSvg` and into module-level constants (`SANITIZER_URL_ATTRIBUTES`, `SANITIZER_BLOCKED_URL_SCHEMES`). They are pure lookup data with no per-call dependency, so allocating them on every render of every Mermaid diagram was wasted work — these now allocate once per module load. Pure cleanup: identical contents, identical lookup semantics, no behavior change. The full Mermaid.test.tsx suite (including the recent sanitizer regression tests) still passes. --- _Generated with `mux` • Model: `anthropic:claude-opus-4-7` • Thinking: `xhigh` • Cost: `$`_ <!-- mux-attribution: model=anthropic:claude-opus-4-7 thinking=xhigh costs= -->
`summarizeToolPart` carried the AI SDK v5 `state` → lifecycle phase mapping inline as a nested ternary plus a multi-line comment listing the states. The ternary obscured the actual mapping (output-available and output-redacted both fold to "done") and made adding new states a multi-line edit. Hoist the mapping to a `TOOL_PART_PHASE_BY_STATE` record at module scope, keyed on the SDK state strings and typed as the literal phase union. The summarizer now does a single table lookup. States not in the table (e.g. `input-streaming`) still yield `null` and the bare `[tool <name>]` form, matching prior behavior exactly. Behavior-preserving: same input strings, same output bytes for every `state` value the prior code handled, and the same `null` fallthrough for everything else.
Three slash-command discovery surfaces (suggestions in ChatInput, ghost
hints in ChatInput, and CommandPalette) duplicated the same inline
lambda:
(experimentId) =>
resolveSlashCommandExperimentValue(experimentId, {
goals: goalsExperimentEnabled,
workspaceHeartbeats: workspaceHeartbeatsExperimentEnabled,
})
Add createSlashCommandExperimentResolver(snapshot) next to its sibling
resolver so each callsite only describes the experiment snapshot it
observes. The new helper returns the exact same closure each callsite
previously built inline, so behavior is byte-identical.
The SSH-provisioning commit (#3302) added a second `import type { Runtime } from "../node/runtime/Runtime"` line right below an existing `import type { InitLogger, WorkspaceInitResult }` from the same module. Fold the new symbol into the existing import — byte-identical semantics, one fewer line of boilerplate, no behavior change.
Extract a shared `BUILT_IN_SKILL_NAMES` constant for the two test assertions in `agentSkillsService.test.ts` that previously enumerated the same five built-in skill names inline. The two existing call sites differ only in their project-skill prefix; spreading the shared constant at the end of each list preserves the exact sort order and assertion semantics. Byte-equivalent to the previous inline arrays; the only effect is that the next new built-in skill needs to land in one place instead of two.
The Tooltip + Switch + Reset Button block for the per-agent advisor toggle was duplicated between renderAgentDefaults and renderUnknownAgentDefaults. The two call sites differ only in the agent id variable and which setter/resetter to invoke, so wrap the shared markup in a small AdvisorSwitch component. Same aria-label, same tooltip text via getAdvisorSwitchState, same conditional Reset semantics — pure cleanup with no behavior change.
Hoist the ["text", "delta", "textDelta"] field priority used to extract AI SDK v5 text-delta payloads into a shared TEXT_OUTPUT_DELTA_FIELDS constant in src/common/utils/ai/streamChunks.ts and reuse it from streamManager.ts (fullStream text-delta handling) and tools/advisor.ts (advisor streamText onChunk handling). Byte-equivalent semantics; only the wiring detail (that the same fallback list applies to both callsites) moves into the shared module.
BudgetTile and TurnsTile each render the same dt-label + Edit-button
header row. Lift the duplicated markup into a small file-scoped
`StatTileHeader` helper so the styling lives in one place and the
tile bodies focus on their own numeric content. Behavior, aria
labels, and class names are unchanged — `GoalTab.test.tsx` (23
tests, including the inline budget/turn-cap editors that locate
the openers via `getByLabelText("Edit goal …")`) still passes.
Three callsites (ChatInput/types.ts, messageQueue.ts, agentSession.ts) each privately redeclared the same NonNullable<SendMessageOptions["goalInterventionPolicy"]> alias after #3319 added the field. Centralizing the type next to SendMessageOptions itself removes the duplication and ensures future sub-typing changes only need to land in one place. Pure relocation — identical type semantics at every callsite.
The silent-continuation summary helper added in #3326 was declared as a private method on `AgentSession` but only depends on its input `parts` argument — it never touches `this`. Convert it to a module-level free function alongside the existing free helpers (`stripGoalInterventionPolicy`, `coerceGoalSyntheticMessageKind`, `extractAgentSkillRefs`, etc.) so the no-`this` contract is obvious to readers and the file's helper style stays consistent. The companion `maybeAutoCompleteGoalFromSilentContinuation` method stays on the class because it accesses `this.workspaceGoalService`, `this.workspaceId`, and `log` indirectly through instance context. Byte-for-byte identical logic at the only callsite. Pure cleanup, no behavior change.
The seed loop in applyReviewPaneUpdate built a key→index map for the existing base list by calling base.indexOf(h) on every iteration — O(n²) when the loop already had the index in hand. Switch to `base.entries()` so the position the map stores is unambiguously the slot we just visited. Behavior-identical for unique references (the only realistic case), and now correct-by-construction even if a future refactor ever introduced duplicate references in `base`.
This test-only export was added speculatively in #3262 (Instructions tab) but no test ever imports it; ripgrep across src/ and tests/ finds zero callers. Removing the unused export lets pendingFocusGeneration become `const` since the reset helper was the only reason it was `let`. The Map is still mutated in place via .set(), and getter/setter exports are unchanged, so observable runtime behavior is byte-identical.
Five toolPolicy entries differed only by tool name and all shared
`action: "disable" as const`. Hoist the names into a local
`CLI_DISABLED_TOOL_NAMES` tuple and produce the policy array via
`.map(name => ({ regex_match: name, action: "disable" as const }))`.
Picked from the `ask_user_question` disable that landed in #3336 —
that commit added a fifth identical-shape entry, so the table form is
now strictly tidier than five copy-pasted object literals. Adding the
next disabled CLI tool drops to one string instead of duplicating the
shape. Byte-equivalent toolPolicy contents (same five entries, same
order, same action), so no behavior change.
Generated with `mux`
…lCall The two pill renderers in the ask_user_question executing UI (one per question plus the Summary pill) inlined the same nine-class Tailwind string and the same three-way selected/complete/neutral branch logic. Extract a module-level `getSectionPillClassName(isSelected, isComplete)` helper so both callsites share one declaration; the next visual tweak to those pills now lives in one place instead of two. Identical class strings at both callsites, so the rendered output is byte-equivalent and the AskUserQuestionToolCall.test.tsx suite (4/4 pass) continues to find the question pills by their unchanged role and labels.
The SSH_AUTH_SOCK / SSH_AGENT_PID save+restore pattern was inlined four times (twice in withoutSshAgent's finally block, twice in afterAll) using the same 'if (saved === undefined) delete; else assign' shape. The new module-local restoreEnvVar(key, savedValue) helper mirrors that exact semantics in one place. Each callsite collapses from four lines to one and the per-key delete-vs-assign decision lives in a single function. Picked from #3340 - that PR brought signingService.test.ts back under review when it bumped the Ed25519 capabilities test timeout to 15s. Tests still pass (14/14).
The single-result `buildReviewDiffPathFilter` wrapper was exported from ReviewPanel.tsx solely so two assertions in ReviewPanel.assistedStats.test.ts could grab the first spec's pathFilter. Production callers (ReviewPanel's diff-load effect and ReviewAssistedStatsReporter) already use the multi-spec `buildReviewDiffPathFilterSpecs` form directly so they can handle assisted multi-project pins. Move the wrapper into the test file as a private helper. Pure relocation — identical implementation, same two describe-block assertions, same buildReviewDiffPathFilterSpecs delegation — so the ReviewPanel module's exported surface drops one helper that no production code ever called. Picked from the `fix: surface assisted review hunks reliably` change in #3344, which introduced the test-only wrapper alongside the production multi-spec form.
The two new `parseGoalBudgetFlag` and `parseGoalTurnsFlag` helpers in src/cli/run.ts (added in #3348) built their error strings with `'...' + value + '...'` concatenation, which is the only `throw new Error('...')` pair in the file — every other error (including the adjacent `parseMode` helper) uses backtick template literals. Convert these two messages to template literals for consistency. Pure stylistic alignment; the thrown Error has the same message text.
Hoist the duplicated Tailwind class string used by the Pause / Mark complete / Reopen buttons in GoalTab.tsx into a single module-level GOAL_LIFECYCLE_NEUTRAL_BUTTON_CLASS constant. Each call site previously inlined the same 'border-border-light bg-surface-secondary text-foreground hover:bg-surface-tertiary inline-flex items-center gap-1.5 rounded-md border px-3 py-1.5 text-sm' string; the accent-colored Archive and success-tinted Resume buttons keep their own class strings because they are the only primary / positive variants in the row. Pure DRY relocation: same class string at every callsite, so the existing GoalTab.test.tsx 'active goals expose a de-prominent Clear; completed goals promote Archive as primary' assertion (which checks that Reopen's className does NOT contain 'bg-accent' and that Archive's DOES) still holds, and every Pause / Mark complete / Reopen aria-label lookup in the suite continues to resolve. No behavior change.
…RYABLE_ERRORS #3356 introduced two adjacent constants in SSHRuntime.ts that listed the same four receive-pack thin-pack failure patterns inline: PROJECT_SYNC_RETRYABLE_ERRORS (used by the broad retry classifier) and UNRESOLVED_DELTA_PUSH_PATTERNS (used by isUnresolvedDeltaPushFailure to decide whether the next retry attempt should add --no-thin). Define the four-string sub-class once on UNRESOLVED_DELTA_PUSH_PATTERNS and spread it into PROJECT_SYNC_RETRYABLE_ERRORS so the lists can never drift. .some(pattern => errorMsg.includes(pattern)) sees the same patterns in the same order, so the retry classification is byte-equivalent — pure DRY, no behavior change.
The intent-only summary mode added in #3360 introduced a second normalizeForComparison(value) !== normalizeForComparison(command) call, so the same whitespace- and case-insensitive command-equality check now appears in three spots: filtering a redundant intent, filtering a redundant display-name fallback, and the trailing-using-clause stripper. Pull that predicate into a single matchesCommand helper next to normalizeForComparison so all three callsites read as 'intent (or display name, or candidate) is the same as the command' and any future tweak to the comparison normalization happens in one place. Pure rename/extract: the helper composes the existing normalizer, returns the identical boolean, and no callsite changes its branch direction.
ReviewAssistedStatsReporter and ReviewPanel each reimplemented the same 'filter raw assisted hunks against the user's dismissed key list' logic — one inline (with a per-call `new Set`) and one through an intermediate `dismissedAssistedKeySet` useMemo. Both versions early- return the input array reference when the dismissed list is empty so downstream useMemo consumers keep a stable identity. Moves the logic into a pure `filterDismissedAssistedHunks` helper next to its sibling `formatAssistedFilter` in `src/common/utils/review/assistedReview.ts`, with focused unit tests covering identity preservation, key-based filtering, and the 'dismissed key no longer present' no-op path. Both consumers now share the helper; the now-unused `dismissedAssistedKeySet` memo in `ReviewPanel` drops out. Pure behavior-preserving refactor — no runtime semantics change.
The text/plaintext fast path in highlightDiffChunk inlined the same
{type, lines.map(...), usedFallback} object literal that createFallback-
Chunk already returned; they differed only on the boolean. Replace both
with createPlainTextChunk(chunk, usedFallback), so the line mapping
exists once and each callsite keeps the boolean it cares about as a
named argument (false for the intentional plaintext fast path, true for
all recovery paths). Pure DRY — line numbers, html escaping, and the
usedFallback value at each callsite are unchanged.
`subscribeAdditionalSystemContext` already used a small `getSubscribers` helper to lazily create the per-workspace `Set<() => void>` in `subscribersByWorkspace`. `subscribeAdditionalSystemContextFocus` inlined the same pattern against `focusListenersByWorkspace`, which also forced an extra `const target = listeners` aliasing step to satisfy TypeScript's narrowing inside the unsubscribe closure. Extract a single `getOrCreateListenerSet(map, workspaceId)` helper, route both `getSubscribers` and a new `getFocusListeners` through it, and simplify `subscribeAdditionalSystemContextFocus` to call `.add()` and `.delete()` directly on the returned Set. The Map identities, generation counter, and notify ordering are unchanged — pure dedup with no runtime behavior change. 🤖 Generated with mux
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Rolling, low-risk cleanup batch. Each run adds one extremely small,
behavior-preserving refactor picked from changes that have landed on
mainsince the last checkpoint.
Auto-cleanup checkpoint: 8b90119
Current diff against
main:src/browser/utils/highlighting/highlightDiffChunk.ts— dedupe thetwo plain-text
HighlightedChunkconstruction sites. Thetext/plaintextfast path inlined the same{ type, lines.map(...), usedFallback }object literal thatcreateFallbackChunkalreadyreturned; they differed only on the boolean. Replace both with a
single
createPlainTextChunk(chunk, usedFallback)helper so the linemapping (html-escape, old/new line numbers,
originalIndex = startIndex + i) lives in one place. Each callsite keeps the booleanit cares about as a named argument —
falsefor the intentionalplain-text fast path,
truefor every recovery path (oversized diff,line-count mismatch, empty-extraction guard, Shiki failure). Tests in
highlightDiffChunk.test.tscontinue to assert both flag values; nobehavior change.
src/browser/utils/additionalSystemContextStore.ts— drop theunused
__resetAdditionalSystemContextFocusForTestsexport andpromote
pendingFocusGenerationfromlettoconst. The resethelper was added speculatively alongside the Instructions tab in
🤖 feat: Instructions tab in right sidebar #3262, but ripgrep across
src/andtests/finds zero importers —no test ever calls it. Removing it lets the only other reason the
module needed a mutable binding (the
pendingFocusGeneration = new Map()reassignment inside the helper) go away, so the binding canbe a
constMap that's still mutated in place via.set(). TherequestAdditionalSystemContextFocus/getAdditionalSystemContextFocusGenerationcallers continue to seethe exact same Map identity and counter semantics — pure dead-code
removal with no runtime behavior change.
src/browser/features/Messages/Mermaid.tsx— hoist the URL-attributeset and blocked-scheme list out of
sanitizeMermaidSvgintomodule-level constants (
SANITIZER_URL_ATTRIBUTES,SANITIZER_BLOCKED_URL_SCHEMES). They are pure lookup data with noper-call dependency, so allocating them on every Mermaid render was
wasted work; they now allocate once per module load. Identical
contents and identical lookup semantics — pure cleanup with no
behavior change.
src/node/services/agentStatusService.ts— hoist the AI SDK v5tool-part
state→ lifecycle phase mapping out ofsummarizeToolPartinto a module-level
TOOL_PART_PHASE_BY_STATErecord. The previousnested ternary obscured the actual mapping (
output-availableandoutput-redactedboth fold to"done") and made adding new states amulti-line edit. The summarizer now does a single table lookup; the
union-typed values document the two valid phases inline. States not
in the table (e.g.
input-streaming) still yieldnulland the bare[tool <name>]form, byte-for-byte identical to before.src/browser/utils/slashCommands/experimentVisibility.ts(+ twoconsumers) — add
createSlashCommandExperimentResolver(snapshot)alongside
resolveSlashCommandExperimentValue. Three slash-commanddiscovery surfaces (suggestions in
ChatInput, the ghost-hint call inChatInput, and theCommandPalettepalette query) had each inlinedthe same
(experimentId) => resolveSlashCommandExperimentValue(experimentId, snapshot)lambda. The new helper returns the exact same closure eachcallsite previously built inline, so the
isExperimentEnabledpredicate handed to
getSlashCommandSuggestions/getCommandGhostHintis behaviorally identical; only the wiring detail (that resolution
is parameterized by experiment id) moves into the visibility module.
Rebased through the
goalsGA removal — the snapshot now carriesonly
workspaceHeartbeatsbut the resolver helper itself isunchanged.
src/cli/run.ts— fold theRuntimetype import into the existingimport type { InitLogger, WorkspaceInitResult } from "../node/runtime/Runtime"line. The SSH-provisioning commit (🤖 perf: cut SSH workspace startup ~9× on the warm path via fused single-RTT materialization #3302) added a second
import type { Runtime } from "../node/runtime/Runtime"directly below it; mergingthem is byte-identical semantics with one fewer line of boilerplate.
No behavior change.
src/node/services/agentSkills/agentSkillsService.test.ts— extract ashared
BUILT_IN_SKILL_NAMESconstant for the twotoEqual([...])assertions that previously enumerated the same five built-in skill
names (
imagegen,init,mux-diagram,mux-docs,orchestrate)inline. The two call sites differ only in their project-skill prefix;
spreading the shared constant at the end of each list preserves the
exact sort order and assertion semantics. The next built-in skill
added (e.g. when
/orchestrategraduates to advertised, or a newhidden skill lands) only needs to update one list instead of two.
Byte-equivalent assertion output; no runtime behavior change.
src/browser/features/Settings/Sections/TasksSection.tsx— extract anAdvisorSwitchcomponent for the per-agent advisor toggle. TheTooltip+Switch+ optionalResetbutton block was duplicatedbetween
renderAgentDefaultsandrenderUnknownAgentDefaults(addedin 🤖 fix: enable advisor for default subagents #3309 when the Advisor experiment gained default-on behavior for
Exec and Plan). The two call sites differ only in the agent-id
variable they close over and which setter/resetter to invoke, so the
shared markup now lives in one place. Same
aria-label, sametooltip text via
getAdvisorSwitchState, same conditional Resetsemantics — the TasksSection UI test (
defaults advisor on for Exec and Plan when the experiment is enabled) still finds both switchesby their unchanged role + name.
src/common/utils/ai/streamChunks.ts(+ two consumers) — hoist the["text", "delta", "textDelta"]field priority used to extract AI SDKv5 text-delta payloads into a shared
TEXT_OUTPUT_DELTA_FIELDSconstant. Both
streamManager.ts(thefullStreamtext-deltaparthandler) and
tools/advisor.ts(the advisorstreamTextonChunktext-delta handler, added in 🤖 feat: stream advisor output live #3310) had inlined the same array
literal; centralizing it prevents drift if the AI SDK normalizes
another alias and documents the intent in one place. The other two
extractChunkDeltaTextcallsites inaiService.tsuse deliberatelydifferent field orderings (
["textDelta", "delta", "text"]forparent-step text-delta capture,
["text", "textDelta", "delta"]forreasoning-delta capture) and are intentionally left untouched.
src/common/utils/review/assistedReview.ts(+ twoReviewPanelconsumers) — extract a pure
filterDismissedAssistedHunks(rawAssistedHunks, dismissedKeys)helper alongsideformatAssistedFilter. TheAssisted-Review UX pass (🤖 fix(review): assisted-review UX pass #3358) introduced a per-workspace user-side
dismissed-pin list and both
ReviewAssistedStatsReporter(thealways-mounted Review-tab badge feeder) and the
ReviewPanelfiltered
rawAssistedHunksagainst it — with slightly differentshapes (one inline with a per-call
new Set, the other through aseparate
dismissedAssistedKeySetuseMemo). Both versions early-return the input array reference when the dismissed list is empty so
downstream useMemo consumers keep a stable identity. The new helper
preserves that identity contract, both callers now share it, and the
now-unused
dismissedAssistedKeySetintermediate memo drops out.Pure behavior-preserving refactor with unit-test coverage for the
identity, filtering, and stale-key-no-op paths.
src/browser/hooks/useChatTranscriptFullWidth.ts(+GeneralSectionconsumer) — extract a
persistChatTranscriptFullWidth(enabled)helperthat wraps the
updatePersistedState<boolean | undefined>(CHAT_TRANSCRIPT_FULL_WIDTH_KEY, enabled ? true : undefined)call. The full-width chat transcript setting (🤖 feat: add full-width chat transcript setting #3350) introduced the
same
updatePersistedStateblock in three places — once in the hook'ssetSyncedValue(backend → cache mirror) and twice inGeneralSection(initial config load and the toggle handler) — eachencoding the same on-disk shape contract: store
truewhen enabled,clear the slot otherwise. Centralizing it in the hook module makes
that contract the single source of truth and lets
GeneralSectiondrop its now-unused
updatePersistedStateandCHAT_TRANSCRIPT_FULL_WIDTH_KEYimports. Identical persisted-statewrites, identical call ordering — the existing
useChatTranscriptFullWidth.test.tsxandGeneralSection.test.tsxsuites pass unchanged (15/15).
src/browser/utils/additionalSystemContextStore.ts— dedupe theper-workspace listener-set lazy-init shared by
subscribersByWorkspaceandfocusListenersByWorkspace. ThegetSubscribershelper already handled theexisting ?? newSet()insert dance for the snapshot subscriber map,but
subscribeAdditionalSystemContextFocusopen-coded the samepattern against
focusListenersByWorkspaceand needed an extraconst target = listenersaliasing line so the unsubscribe closureretained narrowing after the
let listenersreassignment. Extract asingle
getOrCreateListenerSet(map, workspaceId)helper, route bothgetSubscribersand a newgetFocusListenersthrough it, and letthe focus subscriber call
.add()/.delete()directly on thereturned Set. Map identities, the
pendingFocusGenerationcounter,and notify ordering are unchanged — pure dedup with no runtime
behavior change.