refactor: simplify getPhaseModelWithOverrides calls per code review

Address code review feedback on PR #629:
- Make settingsService parameter optional in getPhaseModelWithOverrides
- Function now handles undefined settingsService gracefully by returning defaults
- Remove redundant ternary checks in 4 call sites:
  - apps/server/src/routes/context/routes/describe-file.ts
  - apps/server/src/routes/context/routes/describe-image.ts
  - apps/server/src/routes/worktree/routes/generate-commit-message.ts
  - apps/server/src/services/auto-mode-service.ts
- Remove unused DEFAULT_PHASE_MODELS imports where applicable
This commit is contained in:
Stefan de Vogelaere
2026-01-20 20:16:25 +01:00
parent 8efd14c580
commit f007ca2c80
5 changed files with 40 additions and 53 deletions

View File

@@ -15,6 +15,7 @@ import type {
PhaseModelEntry,
Credentials,
} from '@automaker/types';
import { DEFAULT_PHASE_MODELS } from '@automaker/types';
import {
mergeAutoModePrompts,
mergeAgentPrompts,
@@ -502,17 +503,28 @@ export interface PhaseModelWithOverridesResult {
* Also resolves the provider if the phase model has a providerId.
*
* @param phase - The phase key (e.g., 'enhancementModel', 'specGenerationModel')
* @param settingsService - Settings service instance
* @param settingsService - Optional settings service instance (returns defaults if undefined)
* @param projectPath - Optional project path for checking overrides
* @param logPrefix - Prefix for log messages
* @returns Promise resolving to phase model with provider info
*/
export async function getPhaseModelWithOverrides(
phase: PhaseModelKey,
settingsService: SettingsService,
settingsService?: SettingsService | null,
projectPath?: string,
logPrefix = '[SettingsHelper]'
): Promise<PhaseModelWithOverridesResult> {
// Handle undefined settingsService gracefully
if (!settingsService) {
logger.info(`${logPrefix} SettingsService not available, using default for ${phase}`);
return {
phaseModel: DEFAULT_PHASE_MODELS[phase] || { model: 'sonnet' },
isProjectOverride: false,
provider: undefined,
credentials: undefined,
};
}
try {
const globalSettings = await settingsService.getGlobalSettings();
const credentials = await settingsService.getCredentials();

View File

@@ -12,7 +12,6 @@
import type { Request, Response } from 'express';
import { createLogger } from '@automaker/utils';
import { DEFAULT_PHASE_MODELS } from '@automaker/types';
import { PathNotAllowedError } from '@automaker/platform';
import { resolvePhaseModel } from '@automaker/model-resolver';
import { simpleQuery } from '../../../providers/simple-query-service.js';
@@ -161,18 +160,12 @@ ${contentToAnalyze}`;
phaseModel: phaseModelEntry,
provider,
credentials,
} = settingsService
? await getPhaseModelWithOverrides(
'fileDescriptionModel',
settingsService,
cwd,
'[DescribeFile]'
)
: {
phaseModel: DEFAULT_PHASE_MODELS.fileDescriptionModel,
provider: undefined,
credentials: undefined,
};
} = await getPhaseModelWithOverrides(
'fileDescriptionModel',
settingsService,
cwd,
'[DescribeFile]'
);
const { model, thinkingLevel } = resolvePhaseModel(phaseModelEntry);
logger.info(

View File

@@ -13,7 +13,7 @@
import type { Request, Response } from 'express';
import { createLogger, readImageAsBase64 } from '@automaker/utils';
import { DEFAULT_PHASE_MODELS, isCursorModel } from '@automaker/types';
import { isCursorModel } from '@automaker/types';
import { resolvePhaseModel } from '@automaker/model-resolver';
import { simpleQuery } from '../../../providers/simple-query-service.js';
import * as secureFs from '../../../lib/secure-fs.js';
@@ -279,18 +279,12 @@ export function createDescribeImageHandler(
phaseModel: phaseModelEntry,
provider,
credentials,
} = settingsService
? await getPhaseModelWithOverrides(
'imageDescriptionModel',
settingsService,
cwd,
'[DescribeImage]'
)
: {
phaseModel: DEFAULT_PHASE_MODELS.imageDescriptionModel,
provider: undefined,
credentials: undefined,
};
} = await getPhaseModelWithOverrides(
'imageDescriptionModel',
settingsService,
cwd,
'[DescribeImage]'
);
const { model, thinkingLevel } = resolvePhaseModel(phaseModelEntry);
logger.info(

View File

@@ -11,7 +11,7 @@ import { promisify } from 'util';
import { existsSync } from 'fs';
import { join } from 'path';
import { createLogger } from '@automaker/utils';
import { DEFAULT_PHASE_MODELS, isCursorModel, stripProviderPrefix } from '@automaker/types';
import { isCursorModel, stripProviderPrefix } from '@automaker/types';
import { resolvePhaseModel } from '@automaker/model-resolver';
import { mergeCommitMessagePrompts } from '@automaker/prompts';
import { ProviderFactory } from '../../../providers/provider-factory.js';
@@ -162,18 +162,12 @@ export function createGenerateCommitMessageHandler(
phaseModel: phaseModelEntry,
provider: claudeCompatibleProvider,
credentials,
} = settingsService
? await getPhaseModelWithOverrides(
'commitMessageModel',
settingsService,
worktreePath,
'[GenerateCommitMessage]'
)
: {
phaseModel: DEFAULT_PHASE_MODELS.commitMessageModel,
provider: undefined,
credentials: undefined,
};
} = await getPhaseModelWithOverrides(
'commitMessageModel',
settingsService,
worktreePath,
'[GenerateCommitMessage]'
);
const { model } = resolvePhaseModel(phaseModelEntry);
logger.info(

View File

@@ -2318,18 +2318,12 @@ Format your response as a structured markdown document.`;
phaseModel: phaseModelEntry,
provider: analysisClaudeProvider,
credentials,
} = this.settingsService
? await getPhaseModelWithOverrides(
'projectAnalysisModel',
this.settingsService,
projectPath,
'[AutoMode]'
)
: {
phaseModel: DEFAULT_PHASE_MODELS.projectAnalysisModel,
provider: undefined,
credentials: undefined,
};
} = await getPhaseModelWithOverrides(
'projectAnalysisModel',
this.settingsService,
projectPath,
'[AutoMode]'
);
const { model: analysisModel, thinkingLevel: analysisThinkingLevel } =
resolvePhaseModel(phaseModelEntry);
logger.info(