fix(source-control-ai): an absent legacy customAgentCommand no longer crashes the settings pane

A settings.json whose legacy `commitMessageAi` block has no `customAgentCommand`
key made the reconciliation copy `undefined` in as an OWN property, which then
clobbered the `customAgentCommand: ''` default in the normalizer's
`{ ...defaults, ...base }` merge. Main loads that on every start and hands it to
the renderer over structured clone (which, unlike JSON, preserves an own
undefined key), so the state was sticky and self-perpetuating and the settings
pane threw "Cannot read properties of undefined (reading 'trim')" at the
page.settings boundary (crash report 21699b66, v1.4.199).

Fixed at the normalizer contract rather than the one call site: the normalizer
now reads an own key holding undefined as absent - on input, on the nested
per-operation instruction strings, and on its own output - so no consumer of
SourceControlAiSettings sees an off-type value. That also unblocks remote work:
the `SourceControlAiSettings` wire schema in `rpc-contract/git-params.ts` types
`customAgentCommand` as a required string, so the same profile failed param
validation for remote/SSH commit-message generation, not only the settings pane.

Behavior change, deliberate: a legacy core field (`enabled`, `agentId`,
`customPrompt`, `customAgentCommand`) that is *absent* from the legacy block now
reads as absence rather than as a rollback-build edit. Previously an absent field
compared unequal to the projected value, so it counted as "changed" and was
written back - which, once the normalizer stops adopting undefined, would reset
the field to its default. A saved `customAgentCommand` is now preserved and
`enabled: false` stays off instead of flipping on. All four fields are required
in `CommitMessageAiSettings`, so a well-formed legacy block written by any
rollback build is unaffected; a differential probe over 288 well-formed profiles
produces byte-identical output against the previous code.

Tests cover the pure normalizer contract, the reconciliation, and the real
Store load path that made the state sticky.
This commit is contained in:
m4air
2026-09-10 22:54:40 -07:00
parent 2ecde717b4
commit 52b0d1295e
5 changed files with 339 additions and 21 deletions
@@ -0,0 +1,75 @@
import { mkdtempSync, rmSync } from 'node:fs'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import { createStore, testState, writeDataFile } from './persistence-test-harness'
import { getDefaultSourceControlAiSettings } from '../shared/source-control-ai'
vi.mock('electron', () => ({
app: { getPath: () => testState.dir },
safeStorage: { isEncryptionAvailable: () => false }
}))
vi.mock('./telemetry/client', () => ({ track: vi.fn() }))
vi.mock('./telemetry/cohort-classifier', () => ({ getCohortAtEmit: () => ({}) }))
// Crash report 21699b66 (v1.4.199): JSON.stringify drops a key holding undefined, so a profile
// that hit the bug once reloads with `customAgentCommand` absent from BOTH blocks. The load path
// must hand the renderer a string again instead of re-deriving undefined.
const { customAgentCommand: _dropped, ...sourceControlAiWithoutCustomCommand } =
getDefaultSourceControlAiSettings()
describe('loading a profile whose persisted customAgentCommand is absent', () => {
beforeEach(() => {
testState.dir = mkdtempSync(join(tmpdir(), 'orca-test-'))
})
afterEach(() => {
rmSync(testState.dir, { recursive: true, force: true })
})
it('restores the string in both the new and the legacy block', () => {
writeDataFile({
schemaVersion: 1,
repos: [],
worktreeMeta: {},
settings: {
commitMessageAi: {
enabled: true,
agentId: null,
selectedModelByAgent: {},
selectedThinkingByModel: {},
customPrompt: ''
},
sourceControlAi: sourceControlAiWithoutCustomCommand
}
})
const settings = createStore().getSettings()
expect(settings.sourceControlAi?.customAgentCommand).toBe('')
expect(settings.commitMessageAi?.customAgentCommand).toBe('')
})
it('keeps a saved custom command that the legacy block predates', () => {
writeDataFile({
schemaVersion: 1,
repos: [],
worktreeMeta: {},
settings: {
commitMessageAi: {
enabled: true,
agentId: 'custom',
selectedModelByAgent: {},
selectedThinkingByModel: {},
customPrompt: ''
},
sourceControlAi: {
...getDefaultSourceControlAiSettings(),
agentId: 'custom',
customAgentCommand: 'my-generator {prompt}'
}
}
})
const settings = createStore().getSettings()
expect(settings.sourceControlAi?.customAgentCommand).toBe('my-generator {prompt}')
})
})
@@ -0,0 +1,69 @@
// @vitest-environment happy-dom
import { renderToStaticMarkup } from 'react-dom/server'
import { describe, expect, it } from 'vitest'
import { getDefaultSettings } from '../../../../shared/constants'
import type { CommitMessageAiSettings } from '../../../../shared/commit-message-ai-types'
import {
getDefaultSourceControlAiSettings,
mergeLegacyCommitMessageAiIntoSourceControlAi
} from '../../../../shared/source-control-ai'
import type { SourceControlAiSettings } from '../../../../shared/source-control-ai-types'
import { CommitMessageAiPane } from './CommitMessageAiPane'
// Crash report 21699b66 (v1.4.199, boundary page.settings): a settings.json whose legacy
// `commitMessageAi` block has no `customAgentCommand` key — the shape JSON.stringify leaves
// behind once the field has been undefined once.
const legacyWithoutCustomAgentCommand = {
enabled: true,
agentId: null,
selectedModelByAgent: {},
selectedThinkingByModel: {},
customPrompt: ''
} as unknown as CommitMessageAiSettings
function reloadFromDisk(sourceControlAi: SourceControlAiSettings): SourceControlAiSettings {
return mergeLegacyCommitMessageAiIntoSourceControlAi(
JSON.parse(JSON.stringify(sourceControlAi)) as SourceControlAiSettings,
legacyWithoutCustomAgentCommand
)
}
function renderPane(sourceControlAi: SourceControlAiSettings): () => string {
const settings = {
...getDefaultSettings('/tmp'),
commitMessageAi: legacyWithoutCustomAgentCommand,
sourceControlAi
}
return () =>
renderToStaticMarkup(
<CommitMessageAiPane settings={settings} updateSettings={() => {}} settingsSearchQuery="" />
)
}
describe('Source Control AI pane with no persisted customAgentCommand', () => {
it('keeps customAgentCommand a string when the legacy block omits it', () => {
const migrated = mergeLegacyCommitMessageAiIntoSourceControlAi(
undefined,
legacyWithoutCustomAgentCommand
)
const loaded = reloadFromDisk(migrated)
expect(Object.hasOwn(loaded, 'customAgentCommand')).toBe(true)
expect(typeof loaded.customAgentCommand).toBe('string')
})
it('renders the pane instead of throwing the page.settings boundary TypeError', () => {
const migrated = mergeLegacyCommitMessageAiIntoSourceControlAi(
undefined,
legacyWithoutCustomAgentCommand
)
expect(renderPane(reloadFromDisk(migrated))).not.toThrow()
})
// Structured-clone IPC, unlike JSON, hands the renderer an own key holding undefined, so the
// pane must survive that shape even when it never goes back through the legacy reconciliation.
it('renders the pane when main sends an own customAgentCommand key holding undefined', () => {
const cloned = { ...getDefaultSourceControlAiSettings(), customAgentCommand: undefined }
expect(renderPane(cloned as unknown as SourceControlAiSettings)).not.toThrow()
})
})
@@ -24,15 +24,21 @@ function hasEntries(value: Record<string, unknown> | null | undefined): boolean
return Object.keys(value ?? {}).length > 0
}
// An absent legacy field reads back as undefined: that is absence, not a rollback-build edit, so
// it must not overwrite the new-format value or land as an own undefined key (crash 21699b66).
function legacyFieldChanged<T>(legacy: T | undefined, projected: T | undefined): boolean {
return legacy !== undefined && legacy !== projected
}
function legacyCoreChanges(
legacy: CommitMessageAiSettings,
projected: CommitMessageAiSettings
): LegacyCoreChanges {
return {
enabled: legacy.enabled !== projected.enabled,
agentId: legacy.agentId !== projected.agentId,
customPrompt: legacy.customPrompt !== projected.customPrompt,
customAgentCommand: legacy.customAgentCommand !== projected.customAgentCommand
enabled: legacyFieldChanged(legacy.enabled, projected.enabled),
agentId: legacyFieldChanged(legacy.agentId, projected.agentId),
customPrompt: legacyFieldChanged(legacy.customPrompt, projected.customPrompt),
customAgentCommand: legacyFieldChanged(legacy.customAgentCommand, projected.customAgentCommand)
}
}
+22 -17
View File
@@ -1,5 +1,6 @@
import { isCustomAgentId } from './commit-message-agent-spec'
import type { CommitMessageAiSettings } from './commit-message-ai-types'
import { omitUndefinedValues } from './rpc-contract/ui-update-value-tolerance-params'
import {
DEFAULT_SOURCE_CONTROL_ACTION_COMMAND_TEMPLATES,
SOURCE_CONTROL_ACTION_IDS,
@@ -61,19 +62,21 @@ export function sourceControlAiSettingsFromLegacy(
const legacyActionRecipe = actionRecipeFromLegacyCommitMessageAi(legacy)
return {
...defaults,
enabled: legacy.enabled,
agentId: legacy.agentId,
selectedModelByAgent: { ...legacy.selectedModelByAgent },
selectedModelByAgentByHost: copyRecord(legacy.selectedModelByAgentByHost) ?? {},
discoveredModelsByAgent: copyRecord(legacy.discoveredModelsByAgent) ?? {},
discoveredModelsByAgentByHost: copyRecord(legacy.discoveredModelsByAgentByHost) ?? {},
selectedThinkingByModel: { ...legacy.selectedThinkingByModel },
customAgentCommand: legacy.customAgentCommand,
instructionsByOperation: {
commitMessage: legacy.customPrompt ?? '',
pullRequest: '',
branchName: legacy.customPrompt ?? ''
},
...omitUndefinedValues({
enabled: legacy.enabled,
agentId: legacy.agentId,
selectedModelByAgent: { ...legacy.selectedModelByAgent },
selectedModelByAgentByHost: copyRecord(legacy.selectedModelByAgentByHost) ?? {},
discoveredModelsByAgent: copyRecord(legacy.discoveredModelsByAgent) ?? {},
discoveredModelsByAgentByHost: copyRecord(legacy.discoveredModelsByAgentByHost) ?? {},
selectedThinkingByModel: { ...legacy.selectedThinkingByModel },
customAgentCommand: legacy.customAgentCommand,
instructionsByOperation: {
commitMessage: legacy.customPrompt ?? '',
pullRequest: '',
branchName: legacy.customPrompt ?? ''
}
}),
actions: {
...defaults.actions,
commitMessage: legacyActionRecipe,
@@ -92,7 +95,9 @@ export function normalizeSourceControlAiSettings(
value: SourceControlAiSettings | null | undefined,
legacy?: CommitMessageAiSettings | null
): SourceControlAiSettings {
const base = value ?? sourceControlAiSettingsFromLegacy(legacy)
// Persisted settings reach main over structured clone, which preserves an own key whose
// value is undefined; spread over the defaults it would clobber them (crash 21699b66).
const base = omitUndefinedValues(value ?? sourceControlAiSettingsFromLegacy(legacy))
const defaults = getDefaultSourceControlAiSettings()
const normalizedLaunchActionDefaults = normalizeSourceControlAiActionDefaults(
base.launchActionDefaults
@@ -132,7 +137,7 @@ export function normalizeSourceControlAiSettings(
]
})
) as SourceControlAiSettings['actions']
return {
return omitUndefinedValues({
...defaults,
...base,
selectedModelByAgent: { ...defaults.selectedModelByAgent, ...base.selectedModelByAgent },
@@ -148,11 +153,11 @@ export function normalizeSourceControlAiSettings(
},
instructionsByOperation: {
...defaults.instructionsByOperation,
...base.instructionsByOperation
...omitUndefinedValues(base.instructionsByOperation ?? {})
},
modelOverridesByOperation: copyRecord(base.modelOverridesByOperation),
prCreationDefaults: { ...defaults.prCreationDefaults, ...base.prCreationDefaults },
actions: { ...defaults.actions, ...normalizedActions, ...migratedTextActions },
launchActionDefaults: normalizedLaunchActionDefaults ?? defaults.launchActionDefaults
}
})
}
@@ -0,0 +1,163 @@
import { describe, expect, it } from 'vitest'
import type { CommitMessageAiSettings } from './commit-message-ai-types'
import {
getDefaultSourceControlAiSettings,
mergeLegacyCommitMessageAiIntoSourceControlAi,
normalizeSourceControlAiSettings,
sourceControlAiSettingsFromLegacy
} from './source-control-ai'
import type { SourceControlAiSettings } from './source-control-ai-types'
// Crash report 21699b66 (v1.4.199): structured-clone IPC preserves an own key whose value is
// undefined, so such a key used to clobber the default it was spread over and the settings pane
// crashed on `customAgentCommand.trim()`.
function withExplicitUndefined(overrides: Record<string, unknown>): SourceControlAiSettings {
return { ...getDefaultSourceControlAiSettings(), ...overrides } as SourceControlAiSettings
}
const legacyWithoutOptionalStrings = {
enabled: true,
agentId: null,
selectedModelByAgent: {},
selectedThinkingByModel: {}
} as unknown as CommitMessageAiSettings
describe('normalizeSourceControlAiSettings with explicit-undefined own keys', () => {
it('keeps the string defaults instead of adopting undefined', () => {
const normalized = normalizeSourceControlAiSettings(
withExplicitUndefined({ customAgentCommand: undefined })
)
expect(normalized.customAgentCommand).toBe('')
})
it('keeps the other required defaults too', () => {
const normalized = normalizeSourceControlAiSettings(
withExplicitUndefined({
enabled: undefined,
agentId: undefined,
selectedModelByAgent: undefined,
selectedThinkingByModel: undefined,
instructionsByOperation: undefined,
actions: undefined,
prCreationDefaults: undefined
})
)
expect(normalized.enabled).toBe(true)
expect(normalized.agentId).toBeNull()
expect(normalized.selectedModelByAgent).toEqual({})
expect(normalized.selectedThinkingByModel).toEqual({})
expect(normalized.instructionsByOperation).toEqual({
commitMessage: '',
pullRequest: '',
branchName: ''
})
expect(normalized.actions?.commitMessage?.commandInputTemplate).toBeTruthy()
expect(normalized.prCreationDefaults?.draft).toBe(false)
})
it('keeps per-operation instruction strings when one is explicitly undefined', () => {
const normalized = normalizeSourceControlAiSettings(
withExplicitUndefined({
instructionsByOperation: { commitMessage: undefined, pullRequest: 'PR style' }
})
)
expect(normalized.instructionsByOperation.commitMessage).toBe('')
expect(normalized.instructionsByOperation.pullRequest).toBe('PR style')
})
it('never emits an own key holding undefined', () => {
const normalized = normalizeSourceControlAiSettings(
withExplicitUndefined({ customAgentCommand: undefined, modelOverridesByOperation: undefined })
)
expect(Object.entries(normalized).filter(([, value]) => value === undefined)).toEqual([])
})
})
describe('legacy commitMessageAi blocks missing keys', () => {
it('defaults the strings when converting a legacy-only profile', () => {
const converted = sourceControlAiSettingsFromLegacy(legacyWithoutOptionalStrings)
expect(converted.customAgentCommand).toBe('')
expect(converted.instructionsByOperation.commitMessage).toBe('')
})
it('reconciles without re-injecting undefined, and stays stable across reloads', () => {
const first = mergeLegacyCommitMessageAiIntoSourceControlAi(
undefined,
legacyWithoutOptionalStrings
)
const reloaded = mergeLegacyCommitMessageAiIntoSourceControlAi(
JSON.parse(JSON.stringify(first)) as SourceControlAiSettings,
legacyWithoutOptionalStrings
)
const reloadedAgain = mergeLegacyCommitMessageAiIntoSourceControlAi(
JSON.parse(JSON.stringify(reloaded)) as SourceControlAiSettings,
legacyWithoutOptionalStrings
)
expect(first.customAgentCommand).toBe('')
expect(reloaded.customAgentCommand).toBe('')
expect(reloadedAgain).toEqual(reloaded)
})
// An absent legacy key is absence, not a rollback-build edit: reconciling it must not
// overwrite what the new-format settings already hold.
it('keeps a saved customAgentCommand the legacy block never learned about', () => {
const saved = normalizeSourceControlAiSettings({
...getDefaultSourceControlAiSettings(),
agentId: 'custom',
customAgentCommand: 'my-generator {prompt}'
})
const merged = mergeLegacyCommitMessageAiIntoSourceControlAi(saved, {
...legacyWithoutOptionalStrings,
agentId: 'custom'
})
expect(merged.customAgentCommand).toBe('my-generator {prompt}')
})
it('keeps the saved agentId and its action recipe when the legacy block omits agentId', () => {
const saved = normalizeSourceControlAiSettings({
...getDefaultSourceControlAiSettings(),
agentId: 'codex'
})
const legacyWithoutAgentId = { ...legacyWithoutOptionalStrings } as Record<string, unknown>
delete legacyWithoutAgentId.agentId
const merged = mergeLegacyCommitMessageAiIntoSourceControlAi(
saved,
legacyWithoutAgentId as unknown as CommitMessageAiSettings
)
expect(merged.agentId).toBe('codex')
expect(merged.actions?.commitMessage?.agentId).toBe('codex')
})
it('keeps saved commit-message instructions when the legacy block omits customPrompt', () => {
const saved = normalizeSourceControlAiSettings({
...getDefaultSourceControlAiSettings(),
instructionsByOperation: {
commitMessage: 'always conventional commits',
pullRequest: '',
branchName: ''
}
})
const merged = mergeLegacyCommitMessageAiIntoSourceControlAi(
saved,
legacyWithoutOptionalStrings
)
expect(merged.instructionsByOperation.commitMessage).toBe('always conventional commits')
})
it('keeps Source Control AI switched off when the legacy block omits enabled', () => {
const saved = normalizeSourceControlAiSettings({
...getDefaultSourceControlAiSettings(),
enabled: false
})
const legacyWithoutEnabled = {
agentId: null,
selectedModelByAgent: {},
selectedThinkingByModel: {},
customPrompt: '',
customAgentCommand: ''
} as unknown as CommitMessageAiSettings
expect(mergeLegacyCommitMessageAiIntoSourceControlAi(saved, legacyWithoutEnabled).enabled).toBe(
false
)
})
})