fix(#2070): accept adaptive model_profile in validate health; warn on invalid models tiers (W022) (#2336)
* test(#2070): fail-first tests for adaptive model_profile and models tier validation Encodes the three acceptance criteria from #2070 plus the boundary cases the resolver silently ignores today (non-string values, empty string, mistyped phase-type key), and pins VALID_TIERS to a catalog-derived set. Red phase: these fail against current src/ by design. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SLufH5sDuqA1AiEGu45cuA * fix(#2070): accept adaptive model_profile in validate health; warn on invalid models tiers (W022) W004 sourced its profile list from a hand-maintained literal that predated the adaptive profile, so `"model_profile": "adaptive"` was false-flagged. It now reads VALID_PROFILES, which model-catalog.cts derives from model-catalog.json. models.<phase_type> was validated nowhere: the resolver's tier gate silently drops unknown values, so a typo like `"planning": "opuss"` was an undiagnosable no-op. A new W022 flags unknown phase-type keys and invalid tier values (including non-string values, which the same gate also drops). VALID_TIERS moves from a function-local literal in model-resolver.cts to a catalog-derived export, so health and the resolver cannot disagree by construction rather than by parity test. Object.values(adaptiveTierMap) is ['opus','sonnet','haiku'] plus 'inherit' — identical to the previous literal, so resolution behavior is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SLufH5sDuqA1AiEGu45cuA * docs(#2070): changeset for validate health adaptive profile + W022 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SLufH5sDuqA1AiEGu45cuA * fix(#2070): close review findings — malformed models, tier-list duplication, changeset gate Review of the initial fix surfaced three real defects, folded in per the no-defer rule: 1. verify.cts: the W022 guard skipped a top-level `models` that is present but not a plain object (`[]`, `"opus"`, `5`, `true`). The resolver ignores those identically, so they were the same undiagnosable no-op #2070 targets — just one level up. They now warn; absent/null/{} stay silent. 2. config-loader.cts: RUNTIME_OVERRIDE_TIERS was a second hardcoded copy of the tier vocabulary this change had just de-hardcoded elsewhere. It now derives from the catalog via ADAPTIVE_TIER_VALUES (no 'inherit' — runtime overrides resolve to a concrete tier). Byte-equivalent to the old literal. 3. scripts/changeset/lint.cjs: USER_FACING_PREFIXES omitted `src/`. Post-ADR-457 the product source is src/*.cts compiled to a gitignored gsd-core/bin/lib, so the `gsd-core/` prefix is dead coverage for library code and a src/-only PR could merge with no release note — including this one. Adding `src/` closes the gate; tests/ stays non-user-facing. Also corrects a false docstring in the VALID_TIERS test: value-equality cannot detect a re-hardcoded literal, so the test no longer claims it does. Two review findings were rejected with evidence rather than actioned: - W021 double-allocation is governed by ADR-612 ("W021 renumber -> void ... kept, message-disambiguated"), not a defect. - Global-defaults validation would be a false-positive generator: config-loader reads ~/.gsd/defaults.json only on the "no .planning/" branch, and health early-returns E001 without .planning/, so those values provably never affect resolution in any context health can run. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SLufH5sDuqA1AiEGu45cuA * test(#2070): regenerate install goldens for the changeset-lint change scripts/ ships as an installed artifact, so scripts/changeset/lint.cjs's content hash is pinned in all 18 runtime golden fixtures. Adding 'src/' to USER_FACING_PREFIXES changed that hash and tripped every golden parity check. Regenerated via `npm run gen:golden`; the only delta is the lint.cjs hash. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SLufH5sDuqA1AiEGu45cuA * docs(#2070): backfill PR number 2336 into changeset Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SLufH5sDuqA1AiEGu45cuA --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -17,7 +17,7 @@
|
||||
* - ./planning-workspace.cjs (planningDir, planningRoot)
|
||||
* - ./shell-command-projection.cjs (execGit, platformWriteSync, platformReadSync)
|
||||
* - ./core-utils.cjs (detectSubRepos)
|
||||
* - ./model-catalog.cjs (KNOWN_RUNTIMES, KNOWN_PROVIDERS)
|
||||
* - ./model-catalog.cjs (KNOWN_RUNTIMES, KNOWN_PROVIDERS, ADAPTIVE_TIER_VALUES)
|
||||
*/
|
||||
|
||||
import fs from 'node:fs';
|
||||
@@ -35,7 +35,7 @@ import { CONFIG_DEFAULTS as CANONICAL_CONFIG_DEFAULTS, normalizeLegacyKeys } fro
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports
|
||||
import configSchema = require('./config-schema.cjs');
|
||||
const { VALID_CONFIG_KEYS, DYNAMIC_KEY_PATTERNS, isCentralConfigKey: _isCentralConfigKeyFn } = configSchema;
|
||||
import { KNOWN_RUNTIMES, KNOWN_PROVIDERS } from './model-catalog.cjs';
|
||||
import { KNOWN_RUNTIMES, KNOWN_PROVIDERS, ADAPTIVE_TIER_VALUES } from './model-catalog.cjs';
|
||||
// ─── Federated Config (ADR-857 phase 3b) ─────────────────────────────────────
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports
|
||||
import federatedConfigModule = require('./federated-config.cjs');
|
||||
@@ -190,7 +190,11 @@ function isGitIgnored(cwd: string, targetPath: string): boolean {
|
||||
|
||||
// ─── Model alias resolution ───────────────────────────────────────────────────
|
||||
|
||||
const RUNTIME_OVERRIDE_TIERS = new Set(['opus', 'sonnet', 'haiku']);
|
||||
// Catalog-derived (model-catalog.cts) so this vocabulary can never drift from
|
||||
// VALID_TIERS in verify.cts — see #2070 "Generative Fix Divergence". Excludes
|
||||
// 'inherit' (unlike VALID_TIERS): runtime overrides always resolve to a
|
||||
// concrete tier, never the adaptive sentinel.
|
||||
const RUNTIME_OVERRIDE_TIERS = ADAPTIVE_TIER_VALUES;
|
||||
const _warnedConfigKeys = new Set<string>();
|
||||
|
||||
function _warnUnknownProfileOverrides(parsed: Record<string, unknown>, configLabel: string): void {
|
||||
|
||||
@@ -77,6 +77,13 @@ export { _catalog as catalog };
|
||||
export const VALID_PROFILES: string[] = [..._catalog.profiles];
|
||||
export const VALID_PHASE_TYPES: Set<string> = new Set(_catalog.phaseTypes);
|
||||
export const VALID_AGENT_TIERS: Set<string> = new Set(Object.keys(_catalog.adaptiveTierMap));
|
||||
// Catalog-derived so this can never drift from the resolver's tier gate:
|
||||
// Object.values(adaptiveTierMap) === ['opus', 'sonnet', 'haiku'] today, plus 'inherit'.
|
||||
export const VALID_TIERS: Set<string> = new Set([...Object.values(_catalog.adaptiveTierMap), 'inherit']);
|
||||
// Same catalog-derived tier values as VALID_TIERS but WITHOUT 'inherit' — used
|
||||
// by config-loader's runtime-override validation (model_profile_overrides /
|
||||
// model_policy.runtime_tiers), which does not accept 'inherit' as a tier.
|
||||
export const ADAPTIVE_TIER_VALUES: Set<string> = new Set(Object.values(_catalog.adaptiveTierMap));
|
||||
|
||||
/** Per-profile model slots for each agent. */
|
||||
export interface AgentModelProfiles {
|
||||
|
||||
@@ -16,7 +16,7 @@
|
||||
* - ./config-loader.cjs (loadConfig)
|
||||
* - ./configuration.cjs (CONFIG_DEFAULTS as CANONICAL_CONFIG_DEFAULTS)
|
||||
* - ./model-profiles.cjs (MODEL_PROFILES, AGENT_TO_PHASE_TYPE, AGENT_DEFAULT_TIERS, VALID_AGENT_TIERS, nextTier)
|
||||
* - ./model-catalog.cjs (MODEL_ALIAS_MAP, RUNTIME_PROFILE_MAP, PROVIDER_PRESETS)
|
||||
* - ./model-catalog.cjs (MODEL_ALIAS_MAP, RUNTIME_PROFILE_MAP, PROVIDER_PRESETS, VALID_TIERS)
|
||||
*/
|
||||
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports
|
||||
@@ -30,7 +30,7 @@ import { CONFIG_DEFAULTS as CANONICAL_CONFIG_DEFAULTS } from './configuration.cj
|
||||
import modelProfiles = require('./model-profiles.cjs');
|
||||
const { MODEL_PROFILES, AGENT_TO_PHASE_TYPE, AGENT_DEFAULT_TIERS, VALID_AGENT_TIERS, nextTier } = modelProfiles;
|
||||
|
||||
import { MODEL_ALIAS_MAP, RUNTIME_PROFILE_MAP, PROVIDER_PRESETS } from './model-catalog.cjs';
|
||||
import { MODEL_ALIAS_MAP, RUNTIME_PROFILE_MAP, PROVIDER_PRESETS, VALID_TIERS } from './model-catalog.cjs';
|
||||
|
||||
import fs from 'node:fs';
|
||||
import path from 'node:path';
|
||||
@@ -333,7 +333,6 @@ function resolveModelInternal(cwd: string, agentType: string): string {
|
||||
const phaseTypeTier = (phaseType && configModels && typeof configModels === 'object')
|
||||
? configModels[phaseType]
|
||||
: undefined;
|
||||
const VALID_TIERS = new Set(['opus', 'sonnet', 'haiku', 'inherit']);
|
||||
const tier = (phaseTypeTier && VALID_TIERS.has(phaseTypeTier))
|
||||
? phaseTypeTier
|
||||
: (profile === 'inherit'
|
||||
|
||||
@@ -28,6 +28,7 @@ import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs';
|
||||
import { detectSchemaFiles, checkSchemaDrift } from './schema-detect.cjs';
|
||||
import { isCanonicalPlanningFile } from './artifacts.cjs';
|
||||
import { extractTaggedBlocks } from './markdown-sectionizer.cjs';
|
||||
import { VALID_PROFILES, VALID_TIERS, VALID_PHASE_TYPES } from './model-catalog.cjs';
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports -- agent-install-check.cjs is an export= CommonJS module
|
||||
import agentInstallCheck = require('./agent-install-check.cjs');
|
||||
const { checkAgentsInstalled } = agentInstallCheck;
|
||||
@@ -1354,13 +1355,39 @@ function cmdValidateHealth(
|
||||
try {
|
||||
const rawCfg = fs.readFileSync(configPath, 'utf-8');
|
||||
const parsed = JSON.parse(rawCfg) as Record<string, unknown>;
|
||||
const validProfiles = ['quality', 'balanced', 'budget', 'inherit'];
|
||||
if (parsed['model_profile'] && !validProfiles.includes(parsed['model_profile'] as string)) {
|
||||
if (parsed['model_profile'] && !VALID_PROFILES.includes(parsed['model_profile'] as string)) {
|
||||
addIssue(
|
||||
'warning',
|
||||
'W004',
|
||||
`config.json: invalid model_profile "${parsed['model_profile'] as string}"`,
|
||||
`Valid values: ${validProfiles.join(', ')}`,
|
||||
`Valid values: ${VALID_PROFILES.join(', ')}`,
|
||||
);
|
||||
}
|
||||
const configModels = parsed['models'];
|
||||
if (configModels && typeof configModels === 'object' && !Array.isArray(configModels)) {
|
||||
for (const [phaseType, tierValue] of Object.entries(configModels as Record<string, unknown>)) {
|
||||
if (!VALID_PHASE_TYPES.has(phaseType)) {
|
||||
addIssue(
|
||||
'warning',
|
||||
'W022',
|
||||
`config.json: models has an unknown phase type "${phaseType}" which will be ignored`,
|
||||
`Valid phase types: ${[...VALID_PHASE_TYPES].join(', ')}`,
|
||||
);
|
||||
} else if (typeof tierValue !== 'string' || !VALID_TIERS.has(tierValue)) {
|
||||
addIssue(
|
||||
'warning',
|
||||
'W022',
|
||||
`config.json: models.${phaseType} has an invalid tier value ${JSON.stringify(tierValue)} which will be ignored`,
|
||||
`Valid tiers: ${[...VALID_TIERS].join(', ')}`,
|
||||
);
|
||||
}
|
||||
}
|
||||
} else if (configModels !== undefined && configModels !== null) {
|
||||
addIssue(
|
||||
'warning',
|
||||
'W022',
|
||||
`config.json: models is set to ${JSON.stringify(configModels)}, but must be an object mapping phase types to tiers — this value will be ignored`,
|
||||
`Set models to an object like {"planning": "sonnet"}, or remove the key to use profile defaults`,
|
||||
);
|
||||
}
|
||||
} catch (err) {
|
||||
|
||||
Reference in New Issue
Block a user