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 + ) + }) +})