diff --git a/server/lib/README.md b/server/lib/README.md index bd12fd0cf8..ad89c815f4 100644 --- a/server/lib/README.md +++ b/server/lib/README.md @@ -86,7 +86,7 @@ The barrel `server/lib/index.js` is a machine-checkable enumeration of every pub | `storyboardScenes.js` | Durable ids + id-first addressing for `stages.storyboards.scenes[]` (#3413). `ensureStoryboardIds(scenes)` stamps a deterministic `scene-NN` / `shot-NN` id on any entry lacking one — and re-stamps later copies of a DUPLICATE id (collision-escaped with a `-2` suffix, so peers and concurrent readers derive identical ids); `resolveStoryboardTarget(list, { id, index })` → `{ index, record, matchedBy, stale }` resolves a captured id against a fresh array, falling back to the index only when no id was captured, and reporting `stale: true` (409, never an index retarget) when a captured id is gone. | | `seasonStructure.js` | Season/episode structure recommendation. | | `seriesCharacterArc.js` | Per-character story-arc shapes (`series.characterArcs[]`): want/need, start → end state, transition beats. Sanitizers + `renderCharacterArcsForPrompt` for the `arc.transitions` editorial check. | -| `llmRoutePin.js` | The per-record LLM route pin `{ providerId, model, effort }` — `LLM_ROUTE_PIN_LIMITS` + `llmRoutePinSchema` (door check, `effort` as the shared `EFFORT_LEVELS` enum), `sanitizeLlmRoutePin(raw)` (trim/cap, `null` for an all-empty pin), and `resolveLlmRoutePin(pin, perCall)` → `{ providerId, model, effort, providerMatchesPin }`. Owns the never-cross-providers rule once: a per-call pick beats the pin, and the pin's model/effort are inherited only while the effective provider is still the one they were picked for. Consumed by `seriesLlmOverride.js` and FableLoom's play pin. | +| `llmRoutePin.js` | The per-record LLM route pin `{ providerId, model, effort }` — `LLM_ROUTE_PIN_LIMITS` + `llmRoutePinSchema` (door check, `effort` as the shared `EFFORT_LEVELS` enum), `sanitizeLlmRoutePin(raw)` (trim/cap, `null` for an all-empty pin), and `resolveLlmRoutePin(pin, perCall)` → `{ providerId, model, effort, providerMatchesPin }`. Owns the never-cross-providers rule once, in its two shapes: `resolveLlmRoutePin` merges a record pin with an independent per-call pick per FIELD (guarded by a provider-id comparison), while `pickLlmRoutePinLayer(...layers)` + `llmRoutePinNamesProvider(layer)` take the most specific layer WHOLE for pins whose provider and model were chosen together in one control (falling through to the base layer). Consumed by `seriesLlmOverride.js`, FableLoom's play pin, and the Creative Director stage pins. | | `seriesLlmOverride.js` | Pure `resolveSeriesLlmOverride(series, { overrideProvider, overrideModel })` → `{ provider, model, providerMatchesSeries }` — shared fallback so Pipeline LLM actions honor the series' configured provider/model, only inheriting the series model when the effective provider still matches. | | `bibleExtractor.js` | LLM bible-extraction stage + sanitization. | | `catalogBulkParsers.js` | Dependency-free markdown/CSV/JSON parsers for `POST /api/catalog/bulk-import` and YAML/markdown serializers for `GET /api/catalog/export`. | diff --git a/server/lib/cosValidation.js b/server/lib/cosValidation.js index a6a3649c85..58ad2169ff 100644 --- a/server/lib/cosValidation.js +++ b/server/lib/cosValidation.js @@ -664,6 +664,16 @@ export function reviewerConfigMetadata(config) { * `agy --model --effort ` invocation agy rejects. This is the * one call every site makes instead. * + * Deliberately NOT expressed on `lib/llmRoutePin.js` (#4793). That module owns a + * single `{ providerId, model, effort }` triple; a reviewer pin is two maps keyed + * by reviewer slug, with no provider dimension at all — the slug IS the routing + * key — and its precedence is whole-MAP (an explicitly empty task map overrides + * the defaults), not per-field. Its effort is also validated against each + * reviewer's OWN ladder via `normalizeReviewerEffort`, which is strictly narrower + * than the union `EFFORT_LEVELS` enum the shared schema uses: `agy` really does + * reject `--effort max`. There is no shape the two can meet in without losing + * that narrowing, so this stays hand-rolled. + * * @param {Object} [pins] - task metadata (or explicit options) carrying * `reviewerModels` / `reviewerEfforts` maps; an absent map falls back to the * Code Review Defaults, an explicitly empty one overrides them. diff --git a/server/lib/llmRoutePin.js b/server/lib/llmRoutePin.js index 96961b8e1f..4f9e6fa5c8 100644 --- a/server/lib/llmRoutePin.js +++ b/server/lib/llmRoutePin.js @@ -17,6 +17,15 @@ * * `null` (never `''`) marks an unset dimension, so "fall through to the next * layer" stays distinguishable from a deliberate empty choice. + * + * That rule has TWO shapes, and picking the wrong one silently crosses + * providers: + * + * - `resolveLlmRoutePin(pin, perCall)` — a saved record pin plus an independent + * one-off pick. The two merge per FIELD, guarded by a provider-id comparison. + * - `pickLlmRoutePinLayer(...layers)` — configuration layers whose provider and + * model were chosen together in one control. The winning layer is taken + * WHOLE, which enforces the same rule without a comparison. */ import { z } from 'zod'; @@ -84,3 +93,38 @@ export function resolveLlmRoutePin(pin, perCall = {}) { providerMatchesPin, }; } + +/** Does this layer pin a route at all? A layer that names no `providerId` pins + * nothing the runtime can key on — the model alone is unresolvable, since the + * runtime looks the provider up first. */ +export function llmRoutePinNamesProvider(layer) { + return Boolean(layer?.providerId); +} + +/** + * The OTHER precedence rule, for pins arranged in configuration LAYERS rather + * than pin-plus-per-call: the most specific layer that names a provider wins as + * a WHOLE, and the least specific layer is the base everything falls through to. + * + * Use this — not `resolveLlmRoutePin` — when each layer's provider and model + * were picked TOGETHER in one control (a drawer's provider + model selects, a + * settings assignment row). There, a layer that names a provider but no model + * means "that provider's default model", so merging the next layer's model in + * per-field would hand one provider a model chosen for another: the same + * never-cross-providers rule as `resolveLlmRoutePin`, enforced by taking the + * winning layer whole instead of by comparing provider ids. + * + * `resolveLlmRoutePin` stays right where the layers are a saved record pin and a + * one-off per-call pick, which are independent choices and DO merge per field. + * + * The final layer is returned even when it names no provider, so a base layer + * that carries only a model still reaches the caller. Returns `null` when no + * layer was supplied at all. + * + * @param {...({ providerId?: string|null, model?: string|null, effort?: string|null }|null|undefined)} layers + * most specific first, base last + * @returns {object|null} + */ +export function pickLlmRoutePinLayer(...layers) { + return layers.find(llmRoutePinNamesProvider) ?? layers[layers.length - 1] ?? null; +} diff --git a/server/lib/llmRoutePin.test.js b/server/lib/llmRoutePin.test.js index 6de9cecd47..13e7ed95f9 100644 --- a/server/lib/llmRoutePin.test.js +++ b/server/lib/llmRoutePin.test.js @@ -4,6 +4,8 @@ import { llmRoutePinSchema, resolveLlmRoutePin, sanitizeLlmRoutePin, + llmRoutePinNamesProvider, + pickLlmRoutePinLayer, } from './llmRoutePin.js'; describe('sanitizeLlmRoutePin', () => { @@ -106,3 +108,38 @@ describe('resolveLlmRoutePin', () => { }); }); }); + +describe('llmRoutePinNamesProvider', () => { + it('is true only for a layer carrying a non-empty providerId', () => { + expect(llmRoutePinNamesProvider({ providerId: 'claude' })).toBe(true); + expect(llmRoutePinNamesProvider({ providerId: '' })).toBe(false); + expect(llmRoutePinNamesProvider({ model: 'opus' })).toBe(false); + expect(llmRoutePinNamesProvider(null)).toBe(false); + expect(llmRoutePinNamesProvider(undefined)).toBe(false); + }); +}); + +describe('pickLlmRoutePinLayer', () => { + it('takes the most specific layer that names a provider, whole', () => { + const specific = { providerId: 'claude' }; + const base = { providerId: 'codex', model: 'gpt-x' }; + // The base model is NOT merged in — it belongs to `codex`, not `claude`. + expect(pickLlmRoutePinLayer(specific, base)).toBe(specific); + }); + + it('skips layers that name no provider', () => { + const base = { providerId: 'codex', model: 'gpt-x' }; + expect(pickLlmRoutePinLayer(null, { model: 'orphan' }, base)).toBe(base); + }); + + it('returns the base layer even when it names no provider', () => { + const base = { model: 'gpt-x' }; + expect(pickLlmRoutePinLayer(null, base)).toBe(base); + }); + + it('returns null when no layer was supplied', () => { + expect(pickLlmRoutePinLayer()).toBeNull(); + expect(pickLlmRoutePinLayer(null)).toBeNull(); + expect(pickLlmRoutePinLayer(undefined, null)).toBeNull(); + }); +}); diff --git a/server/services/appTaskProviderPin.js b/server/services/appTaskProviderPin.js index feb3577245..5138c4f9e8 100644 --- a/server/services/appTaskProviderPin.js +++ b/server/services/appTaskProviderPin.js @@ -21,6 +21,16 @@ * * per-app pin → (api-typed? adopt the Schedule pin instead) → Schedule pin → * the install's default coding agent + * + * Deliberately NOT expressed on `lib/llmRoutePin.js` (#4793). Both of that + * module's rules are synchronous and decide on the pin VALUES alone; this walk is + * async and decides on a provider's resolved TYPE, healing an api-typed pin onto + * the next layer — a branch neither rule has a place for. It also carries two + * semantics the shared rules deliberately do not: the Schedule pin is a lazy + * thunk read at most once (so a resolving per-app pin never pays for it), and the + * model falls through with `??` rather than `||`, so an explicitly-blank per-app + * model stays blank instead of inheriting the Schedule pin's. Migrating would + * change both. */ const NOT_AGENT_CAPABLE = 'provider-not-agent-capable'; diff --git a/server/services/creativeDirector/agentBridge.js b/server/services/creativeDirector/agentBridge.js index ac5df6e4b6..8ad66c9862 100644 --- a/server/services/creativeDirector/agentBridge.js +++ b/server/services/creativeDirector/agentBridge.js @@ -20,6 +20,7 @@ import { buildTreatmentPrompt, buildEvaluatePrompt, buildPlanPrompt } from '../. import { getToolSpecs } from '../creative/toolRegistry.js'; import { getSettings } from '../settings.js'; import { resolveStagePin } from './projectsLogic.js'; +import { llmRoutePinNamesProvider, pickLlmRoutePinLayer } from '../../lib/llmRoutePin.js'; import { recordRun } from './local.js'; import { DELIVERABLE_KINDS, deliverableMark } from './deliverableGate.js'; @@ -53,18 +54,24 @@ async function getStageAssignment(kind, project) { if (kind === 'evaluate') return {}; // Only consult the commission when the project does NOT carry its own pin for // this stage — a drawer choice is the user's explicit override and must win. - const projectPin = project?.modelOverrides?.[kind]?.providerId ? project.modelOverrides[kind] : null; + // Shares `llmRoutePinNamesProvider` with `resolveStagePin`'s own layer test, so + // "what counts as a pin" can't drift between the skip check and the resolve. + const projectPinsStage = llmRoutePinNamesProvider(project?.modelOverrides?.[kind]); const [settings, commissionPin] = await Promise.all([ getSettings().catch(() => ({})), // Lazy + only for a commission-owned project, so the CD graph doesn't take a // static dependency on the commission store for the common bare project. - (!projectPin && project?.commissionId) + (!projectPinsStage && project?.commissionId) ? import('../creativeCommissions/projectControl.js') .then(({ commissionStagePin }) => commissionStagePin(project.commissionId)) .catch(() => null) : null, ]); - const assignment = commissionPin || resolveStagePin(kind, project, settings); + // The commission is the outermost layer of the same whole-layer ladder + // `resolveStagePin` resolves (commission → project override → global + // assignment); `commissionStagePin` returns null unless it names a usable + // provider, so it only ever wins by naming one. + const assignment = pickLlmRoutePinLayer(commissionPin, resolveStagePin(kind, project, settings)); if (!assignment.providerId && !assignment.model) return {}; return { ...(assignment.providerId ? { provider: assignment.providerId, providerId: assignment.providerId } : {}), diff --git a/server/services/creativeDirector/projectsLogic.js b/server/services/creativeDirector/projectsLogic.js index b68ee917aa..d4a1986a8a 100644 --- a/server/services/creativeDirector/projectsLogic.js +++ b/server/services/creativeDirector/projectsLogic.js @@ -15,6 +15,7 @@ import { ServerError } from '../../lib/errorHandler.js'; import { creativeDirectorTreatmentSchema, creativeDirectorPlanSchema } from '../../lib/validation.js'; import { PROJECT_STATUSES, PLAN_STEP_TERMINAL_SUCCESS } from '../../lib/creativeDirectorPresets.js'; import { compareNewerWins } from '../../lib/lwwTimestamp.js'; +import { pickLlmRoutePinLayer } from '../../lib/llmRoutePin.js'; import { sanitizeSoftDeleteFields } from '../../lib/syncWire.js'; import { localImageFilename } from '../../lib/localImageFilename.js'; @@ -72,14 +73,25 @@ export function normalizeRenderBackend(raw) { * per-project override wins when it names a providerId; otherwise the global * `settings.creativeDirector.` assignment applies (which itself may be * empty → the caller falls back to the system default / auto-resolution). - * Returns `{ providerId, model }` with string values ('' when unset). Shared by - * agentBridge (treatment/plan CoS-task pins) and sceneEvaluator (evaluation - * vision-call pin) so the two resolution paths can never drift. + * Shared by agentBridge (treatment/plan CoS-task pins) and sceneEvaluator + * (evaluation vision-call pin) so the two resolution paths can never drift. + * + * The layer precedence is `pickLlmRoutePinLayer`'s — the winning layer is taken + * WHOLE, so a project that names a provider but no model gets that provider's + * default model rather than inheriting the global assignment's (which was picked + * for whatever provider the assignment names). See `lib/llmRoutePin.js` for why + * that differs from the per-field `resolveLlmRoutePin`. + * + * Returns `{ providerId, model }` with STRING values ('' when unset), not the + * shared lib's `null`s: `getStageAssignment` and `resolveVisionEvalTarget` both + * branch on plain falsiness and spread the result into task metadata, so keeping + * the two dimensions as strings is this resolver's own contract. */ export function resolveStagePin(stage, project, settings) { - const override = project?.modelOverrides?.[stage]; - const global = settings?.creativeDirector?.[stage]; - const chosen = override?.providerId ? override : global; + const chosen = pickLlmRoutePinLayer( + project?.modelOverrides?.[stage], + settings?.creativeDirector?.[stage], + ); return { providerId: isStr(chosen?.providerId) ? chosen.providerId : '', model: isStr(chosen?.model) ? chosen.model : '', diff --git a/server/services/creativeDirector/projectsLogic.test.js b/server/services/creativeDirector/projectsLogic.test.js index ee74f29f93..c5c5aa1c86 100644 --- a/server/services/creativeDirector/projectsLogic.test.js +++ b/server/services/creativeDirector/projectsLogic.test.js @@ -80,6 +80,39 @@ describe('modelOverrides (per-project provider/model pins)', () => { // Neither → empty strings. expect(resolveStagePin('plan', {}, settings)).toEqual({ providerId: '', model: '' }); }); + + // The layer precedence is WHOLE-OBJECT, not per-field: the layer that names a + // provider supplies the model too, even when it has none. Pinned because it is + // exactly what separates this resolver from the shared per-field + // `resolveLlmRoutePin` — merging the global model in here would hand the + // project's provider a model the user picked for a different one. + it('resolveStagePin does not inherit the global model into a provider-only override', () => { + const settings = { creativeDirector: { treatment: { providerId: 'global', model: 'gm' } } }; + // Different provider — inheriting the global model would cross providers. + expect(resolveStagePin('treatment', { modelOverrides: { treatment: { providerId: 'proj' } } }, settings)) + .toEqual({ providerId: 'proj', model: '' }); + // SAME provider — still not inherited: the drawer's empty model means "this + // provider's default model", not "fall through to the global assignment". + expect(resolveStagePin('treatment', { modelOverrides: { treatment: { providerId: 'global' } } }, settings)) + .toEqual({ providerId: 'global', model: '' }); + }); + + it('resolveStagePin ignores a model-only override and falls through to the global', () => { + const settings = { creativeDirector: { plan: { providerId: 'global', model: 'gm' } } }; + expect(resolveStagePin('plan', { modelOverrides: { plan: { model: 'orphan' } } }, settings)) + .toEqual({ providerId: 'global', model: 'gm' }); + }); + + it('resolveStagePin keeps a model-only global assignment (the base layer is never skipped)', () => { + const settings = { creativeDirector: { plan: { model: 'gm' } } }; + expect(resolveStagePin('plan', {}, settings)).toEqual({ providerId: '', model: 'gm' }); + }); + + it('resolveStagePin coerces absent layers and non-string fields to empty strings', () => { + expect(resolveStagePin('treatment', null, null)).toEqual({ providerId: '', model: '' }); + expect(resolveStagePin('treatment', { modelOverrides: { treatment: { providerId: 'proj', model: 7 } } }, {})) + .toEqual({ providerId: 'proj', model: '' }); + }); }); describe('buildProjectRecord', () => {