Skip to content

name the whole-layer LLM route-pin rule in the shared lib and put the CD stage pins on it - #4822

Merged
atomantic merged 1 commit into
mainfrom
claim/issue-4793
Aug 22, 2026
Merged

name the whole-layer LLM route-pin rule in the shared lib and put the CD stage pins on it#4822
atomantic merged 1 commit into
mainfrom
claim/issue-4793

Conversation

@atomantic

Copy link
Copy Markdown
Owner

Summary

Picks up the "move opportunistically" remainder #4817 left on #4793 — the three sites deliberately not migrated because none of them is mechanical.

server/lib/llmRoutePin.js shipped with one precedence rule: a saved record pin merged per field with an independent per-call pick, guarded by a provider-id comparison. The Creative Director stage pins answer a different question — three configuration layers (commission → project override → global AI Assignment) whose provider and model were picked together in one control, so the winning layer has to be taken whole. Merging per field there would hand one provider a model chosen for another: the same never-cross-providers rule the module already owns, enforced differently.

That difference is exactly why resolveStagePin was left hand-rolled. Naming the second rule lets it use the shared lib without a behavior change:

  • pickLlmRoutePinLayer(...layers) — most specific layer naming a provider wins whole; the final layer is the base everything falls through to (so a model-only global assignment still reaches the caller); null when no layer was supplied.
  • llmRoutePinNamesProvider(layer) — the one definition of "this layer pins a route".
  • resolveStagePin drops its inline pick for it, keeping its ''-not-null string contract (both callers branch on plain falsiness and spread the result into task metadata).
  • agentBridge.getStageAssignment stops re-deriving "does this project pin the stage?" with its own copy of the provider test — a copy that could have drifted from the resolver's silently — and expresses the commission layer as the ladder it is.
  • Module docblock and the server/lib/README.md row now name both rules and when each applies, so the next hand-roll has an obvious home.

Behavior-preserving. Characterization tests pinning today's outputs were added to projectsLogic.test.js first, verified green against the pre-refactor code, and pass unchanged after — including the same-provider case that is precisely what separates the two rules (a project pinning the global's own provider with no model still gets that provider's default, not the assignment's model).

Test plan

  • cd server && npm test — 1574 files / 32987 tests pass (also globs ../scripts, ../lib, ../autofixer).
  • New server/lib/llmRoutePin.test.js cases for pickLlmRoutePinLayer / llmRoutePinNamesProvider: whole-layer pick, skipping provider-less layers, base-layer fallthrough, zero/all-null args.
  • New projectsLogic.test.js characterization cases: provider-only override does not inherit the global model (different provider and same provider), model-only override falls through, model-only global assignment survives, absent layers and non-string fields coerce to ''.
  • server/lib/index.test.js (barrel + README enumeration) green with the new exports.
  • No client changes.

Remaining

Two of the three sites stay hand-rolled. Both now carry a comment at the definition saying why, so this isn't re-litigated:

  • server/lib/cosValidation.js resolveReviewerPins — not a { providerId, model, effort } triple at all. It is two maps keyed by reviewer slug with no provider dimension (the slug is the routing key), its precedence is whole-map (an explicitly empty task map overrides the defaults), and its effort is validated against each reviewer's own ladder rather than the union EFFORT_LEVELS enum. An optional allowed-effort-levels parameter on the shared lib would not help: the shapes never meet, so there is nothing for it to hang on.
  • server/services/appTaskProviderPin.js — both shared rules are synchronous and decide on pin values; this walk is async and decides on a provider's resolved type, healing an api-typed pin onto the next layer. It also reads the Schedule pin through a lazy thunk (at most once, skipped entirely when the per-app pin resolves) and falls the model through with ?? rather than ||, so an explicitly-blank per-app model stays blank. Migrating would change both.

Out of scope per the issue's own notes: the render-target ladder (renderTargets.js, cloudProviderConfig.js) is an image/video ladder, not an LLM route pin; and re-keying the FableLoom pin to loom.llm = { play: {...} } needs a migration for saved playSettings with no behavior gain.

Refs #4793

…nd put the CD stage pins on it (#4793)

The shared `llmRoutePin.js` only knew one precedence rule — a record pin merged
per field with an independent per-call pick. The Creative Director stage pins
answer a different question: three CONFIGURATION layers (commission → project
override → global assignment) whose provider and model were picked together in
one control, so the winning layer has to be taken whole. Merging per field there
would hand one provider a model chosen for another, which is the same
never-cross-providers rule the module already owns, just enforced differently.

That difference is why `resolveStagePin` was left hand-rolled. Naming it —
`pickLlmRoutePinLayer` plus the `llmRoutePinNamesProvider` layer test — lets the
resolver drop its inline pick and lets `agentBridge` stop re-deriving "does this
project pin the stage?" with its own copy of the same test, which could have
drifted from the resolver's silently.

Behavior-preserving: characterization tests pinning today's outputs (including
the same-provider case that separates the two rules) were added first and pass
unchanged against the refactor.

`resolveReviewerPins` and `appTaskProviderPin` stay hand-rolled; both now carry a
comment saying why, so the next reader doesn't re-litigate it.
@atomantic
atomantic merged commit ed939ef into main Aug 22, 2026
7 checks passed
@atomantic
atomantic deleted the claim/issue-4793 branch August 22, 2026 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant