From 52b0d1295eb5b1062fd080d06cded034830f2f32 Mon Sep 17 00:00:00 2001 From: m4air Date: Thu, 10 Sep 2026 22:38:30 -0700 Subject: [PATCH] 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. --- ...e-control-ai-absent-custom-command.test.ts | 75 ++++++++ ...settings-undefined-custom-command.test.tsx | 69 ++++++++ ...source-control-ai-legacy-reconciliation.ts | 14 +- src/shared/source-control-ai-settings.ts | 39 +++-- ...l-ai-undefined-field-normalization.test.ts | 163 ++++++++++++++++++ 5 files changed, 339 insertions(+), 21 deletions(-) create mode 100644 src/main/persistence-source-control-ai-absent-custom-command.test.ts create mode 100644 src/renderer/src/components/settings/source-control-ai-settings-undefined-custom-command.test.tsx create mode 100644 src/shared/source-control-ai-undefined-field-normalization.test.ts diff --git a/src/main/persistence-source-control-ai-absent-custom-command.test.ts b/src/main/persistence-source-control-ai-absent-custom-command.test.ts new file mode 100644 index 00000000000..e07fe709451 --- /dev/null +++ b/src/main/persistence-source-control-ai-absent-custom-command.test.ts @@ -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}') + }) +}) diff --git a/src/renderer/src/components/settings/source-control-ai-settings-undefined-custom-command.test.tsx b/src/renderer/src/components/settings/source-control-ai-settings-undefined-custom-command.test.tsx new file mode 100644 index 00000000000..d13536fd085 --- /dev/null +++ b/src/renderer/src/components/settings/source-control-ai-settings-undefined-custom-command.test.tsx @@ -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( + {}} 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() + }) +}) diff --git a/src/shared/source-control-ai-legacy-reconciliation.ts b/src/shared/source-control-ai-legacy-reconciliation.ts index 1da4fd43619..63166794f37 100644 --- a/src/shared/source-control-ai-legacy-reconciliation.ts +++ b/src/shared/source-control-ai-legacy-reconciliation.ts @@ -24,15 +24,21 @@ function hasEntries(value: Record | 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(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) } } diff --git a/src/shared/source-control-ai-settings.ts b/src/shared/source-control-ai-settings.ts index 319468a2a91..3e818898c00 100644 --- a/src/shared/source-control-ai-settings.ts +++ b/src/shared/source-control-ai-settings.ts @@ -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 - } + }) } diff --git a/src/shared/source-control-ai-undefined-field-normalization.test.ts b/src/shared/source-control-ai-undefined-field-normalization.test.ts new file mode 100644 index 00000000000..4d522286001 --- /dev/null +++ b/src/shared/source-control-ai-undefined-field-normalization.test.ts @@ -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): 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 + 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 + ) + }) +})