Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions middleware/packages/harness-orchestrator/src/buildOrchestrator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,16 @@ export interface BuiltOrchestrator {
readonly orchestrator: Orchestrator;
/** The `chatAgent` bundle — verifier-wrapped agent + raw orchestrator. */
readonly bundle: ChatAgentBundle;
/**
* The resolved orchestrator model after the per-Agent overlay was applied —
* i.e. the actual id the turn loop will send to the provider for this Agent.
* Mirrors `config.model`; carried out so callers (registry log, /admin UI)
* can read it without re-computing the overlay. Issue #296 acceptance #4.
*/
readonly effectiveModel: string;
/** Same as `config.modelRouting` — surfaced so callers can describe the
* per-turn routing the Agent is on without inspecting the orchestrator. */
readonly effectiveModelRouting?: ModelRoutingConfig;
}

/**
Expand Down Expand Up @@ -331,6 +341,8 @@ export function buildOrchestratorForAgent(
sessionLogger,
chatSessionStore,
},
effectiveModel: config.model,
...(config.modelRouting ? { effectiveModelRouting: config.modelRouting } : {}),
};
}

Expand All @@ -356,5 +368,7 @@ export function buildOrchestratorForAgent(
return {
orchestrator,
bundle: { agent, raw: orchestrator, sessionLogger, chatSessionStore },
effectiveModel: config.model,
...(config.modelRouting ? { effectiveModelRouting: config.modelRouting } : {}),
};
}
7 changes: 6 additions & 1 deletion middleware/packages/harness-orchestrator/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ export type { ScopedMemoryStoreOptions } from './registry/scopedMemoryStore.js';
export {
ConfigStore,
ConfigValidationError,
validateModelRef,
validateModelRoutingShape,
} from './registry/configStore.js';
export type {
AgentInput,
Expand Down Expand Up @@ -123,7 +125,10 @@ export type {
SubAgentGraph,
SubAgentToolDeps,
} from './registry/subAgentTools.js';
export { resolveAgentModelRouting } from './registry/agentRuntime.js';
export {
DEFAULT_ORCHESTRATOR_MODEL,
resolveAgentModelRouting,
} from './registry/agentRuntime.js';
export type { ResolvedAgentRuntime } from './registry/agentRuntime.js';

// Per-Agent Orchestrator factory (US3) — re-exported so US4-style external
Expand Down
11 changes: 6 additions & 5 deletions middleware/packages/harness-orchestrator/src/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import type { TurnHookRunner } from './turnHooks.js';
import type { ChatSessionStore } from './chatSessionStore.js';
import type { NativeToolRegistry } from './nativeToolRegistry.js';
import type { Orchestrator } from './orchestrator.js';
import { DEFAULT_ORCHESTRATOR_MODEL } from './registry/agentRuntime.js';
import { ConfigStore } from './registry/configStore.js';
import {
OrchestratorRegistry,
Expand Down Expand Up @@ -139,11 +140,11 @@ const GRAPH_POOL_SERVICE = 'graphPool';
const PLUGIN_CAPABILITIES_SERVICE = 'pluginCapabilities';

// Fallback when the operator has not set `orchestrator_model` in the install
// config. Must be a currently-served Anthropic model id — a stale/typo'd id
// makes every turn fail with `404 not_found_error` (the orchestrator main
// loop has no other model source). Kept in sync with the kernel default
// `ORCHESTRATOR_MODEL` in middleware/src/config.ts.
const DEFAULT_MODEL = 'claude-opus-4-8';
// config. Shared with the registry's per-instance resolution (tier 3) so the
// install-config default and the per-Agent overlay fall back to the same id.
// Kept in sync with the kernel default `ORCHESTRATOR_MODEL` in
// middleware/src/config.ts.
const DEFAULT_MODEL = DEFAULT_ORCHESTRATOR_MODEL;
// 8192, not 4096: a verbose preamble + a large structured tool call (e.g. a
// multi-sheet create_xlsx with formulas) truncates at 4096 → `max_tokens`
// mid-tool-call, so the file is never built. Also enforced as a floor below so
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { Pool } from 'pg';

import { ConfigValidationError } from './configStore.js';
import { ConfigValidationError, validateModelRef } from './configStore.js';

/**
* Agent Builder graph store (P0).
Expand Down Expand Up @@ -316,6 +316,12 @@ export class AgentGraphStore {
}

async createSubAgent(input: SubAgentInput): Promise<SubAgentRow> {
// Issue #296 follow-up — guard sub-agent model writes against unknown
// ids the same way `setModelRouting` guards the orchestrator model.
// Empty / null skips validation (= inherit parent agent / platform).
if (input.model != null && input.model.trim() !== '') {
validateModelRef('subAgent.model', input.model.trim());
}
try {
const { rows } = await this.pool.query<SubAgentDbRow>(
`INSERT INTO agent_subagents
Expand Down Expand Up @@ -347,6 +353,12 @@ export class AgentGraphStore {
}

async updateSubAgent(id: string, patch: SubAgentPatch): Promise<SubAgentRow> {
// Issue #296 follow-up — see `createSubAgent` rationale. `null` is the
// wire form for "clear" (no validation); empty-string is treated the
// same so the UI dropdown's "(default)" choice round-trips cleanly.
if (patch.model != null && patch.model.trim() !== '') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR — a pinned sub-agent model can never be cleared back to "inherit".

This comment says null is the wire form for "clear", but the UPDATE below uses model = COALESCE($4, model) (line 366) with $4 = patch.model ?? null (line 379). COALESCE(NULL, model) keeps the existing value, so passing model: null is a no-op — an operator can't revert a sub-agent from a pinned model back to inherit-parent. (The sibling setSubAgentSkill uses a direct write precisely because COALESCE can't clear a column; there's no equivalent for model.)

Separately, createSubAgent persists '' rather than NULL for the empty-string "(default)" choice — input.model ?? null (line 336) doesn't catch ''. Benign at runtime (resolveSubAgentModel('') → parent default) but it leaves dirty data.

Please give model a real clear path: branch on patch.model === null → write model = NULL (or add a dedicated clearSubAgentModel direct write, mirroring setSubAgentSkill), and bind input.model?.trim() || null in createSubAgent so the column stays clean.

validateModelRef('subAgent.model', patch.model.trim());
}
const { rows } = await this.pool.query<SubAgentDbRow>(
`UPDATE agent_subagents SET
name = COALESCE($2, name),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { resolveModelRef } from '@omadia/llm-provider';

import type { ModelRoutingConfig as RuntimeModelRouting } from '../modelRouter.js';

/**
Expand All @@ -17,6 +19,60 @@ import type { ModelRoutingConfig as RuntimeModelRouting } from '../modelRouter.j

const DEFAULT_CLASSIFIER_MODEL = 'claude-haiku-4-5';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR — the default classifier is an id your own validator would reject. claude-haiku-4-5 isn't in the registry (the registered Haiku is claude-haiku-4-5-20251001, alias haikumodelRegistry.ts:12-16), so resolveModelRef('claude-haiku-4-5') is undefined. It survives only because resolveModelIdForProvider passes registry-unknown refs through raw and Anthropic happens to serve the undated alias — yet if an operator typed claude-haiku-4-5 into the triage field, validateModelRef would throw "not registered". Please align this default to the dated claude-haiku-4-5-20251001 (or the haiku alias) so the code's own default and its write-validation agree. Non-blocking.


/**
* Hard fallback orchestrator model — the last tier of the per-instance model
* resolution (issue #296 AC#2):
*
* 1. the Agent's own `model_routing.main` (operator's per-instance choice)
* 2. the global seeded platform default (`orchestrator_model` install config,
* itself seeded from the `ORCHESTRATOR_MODEL` env in middleware/src/config.ts)
* 3. this constant — so an empty / misconfigured platform default never yields
* an empty model id (which would 404 on every turn).
*
* Must be a currently-served model id, kept in sync with `ORCHESTRATOR_MODEL`
* (middleware/src/config.ts) and the plugin's install-config fallback.
*/
export const DEFAULT_ORCHESTRATOR_MODEL = 'claude-opus-4-8';

/**
* Resolve a model ref to the active provider's concrete bare `modelId`
* (issue #296).
*
* Both the orchestrator main loop AND in-process sub-agents send `model` RAW to
* a single concrete provider adapter — there is no ref→modelId resolution in
* the send path. The Admin picker stores a provider-qualified id
* (`anthropic:claude-opus-4-8`) or a legacy alias (`opus`); sending either raw
* 404s every turn. Returns:
* - registry-known, same provider → the bare vendor `modelId`
* - registry-known, DIFFERENT provider than `activeProvider` → `undefined`
* (cross-provider is out of scope and would 404 on the wrong adapter — the
* caller falls back to its own default)
* - registry-UNKNOWN (not in the curated set) → the raw trimmed ref. The
* registry is a curated subset, not the universe of valid API ids — an id
* the registry does not list may still be served (e.g. an undated default
* or an operator-typed id). Passing it through preserves pre-resolution
* behaviour, matching the `resolveModelRef(x)?.modelId ?? x` contract used
* elsewhere (e.g. `builderPreviewPrompt`).
* - empty / whitespace → `undefined` (no model specified → caller falls back)
*
* The CLI provider owns its own alias scheme (`sonnet`/`opus`) and must be
* handled by the caller BEFORE this — pass its refs through untouched.
*/
export function resolveModelIdForProvider(
ref: string | null | undefined,
activeProvider: string | undefined,
): string | undefined {
const trimmed = ref?.trim();
if (!trimmed) return undefined;
const info = resolveModelRef(
trimmed,
activeProvider ? { defaultProvider: activeProvider } : {},
);
if (info === undefined) return trimmed;
if (activeProvider && info.provider !== activeProvider) return undefined;
return info.modelId;
}

export interface ResolvedAgentRuntime {
/** Primary model override (the agent's `main`), if set. */
readonly model?: string;
Expand Down
50 changes: 46 additions & 4 deletions middleware/packages/harness-orchestrator/src/registry/applyDiff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@ import type {
SubAgentRow,
ToolGrantRow,
} from './agentGraphStore.js';
import { resolveAgentModelRouting } from './agentRuntime.js';
import {
DEFAULT_ORCHESTRATOR_MODEL,
resolveAgentModelRouting,
resolveModelIdForProvider,
} from './agentRuntime.js';
import type {
AgentPluginRow,
AgentRow,
Expand Down Expand Up @@ -160,18 +164,56 @@ export function buildForAgent(
// platform default: `main` overrides the model, `triage` mode adds per-turn
// Haiku→Sonnet/Opus routing. Falls back to the platform runtime when unset.
const routing = resolveAgentModelRouting(agent.modelRouting);

// The orchestrator hands `model` to a SINGLE concrete provider adapter, which
// sends it RAW to the wire API (no ref→modelId resolution in the send path).
// The Admin picker stores a provider-qualified id / alias, so resolve the
// per-Agent overlay to the active provider's concrete `modelId` HERE — see
// `resolveModelIdForProvider` (issue #296). The CLI provider owns its own
// alias scheme so its refs pass through untouched; the platform default
// (`runtime.model`, operator-set env) is left as-is — it works raw today and
// resolving it would change established behaviour.
const activeProvider = deps.provider?.id;
const resolveOverlay = (ref: string | undefined): string | undefined =>
activeProvider === 'claude-cli'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKER (claude-cli deployments) — provider-qualified picker ids reach the CLI unresolved, so every per-Agent turn fails.

This activeProvider === 'claude-cli' branch skips resolution and passes the ref through raw. The new picker, though, writes provider-qualified ids, and on a keyless CLI deployment /v1/builder/models offers only claude-cli:opus-cli / sonnet-cli / haiku-cli (registered via listModels(), builtinLlmProviders.ts:136-159). validateModelRef accepts them, so model_routing.main = 'claude-cli:opus-cli' persists. At build the ref stays raw → buildOrchestrator.ts:335 runs config.model.replace(/-cli$/, '')'claude-cli:opus', which CliChatAgent passes verbatim as claude -p --model claude-cli:opus (cliChatAgent.ts:608). That's an invalid model id (the CLI expects the bare opus), so every turn for that Agent fails — and on a keyless deployment there is no other pick to choose.

Please route the CLI branch through the resolver as well, so it yields the bare modelId first:

const resolveOverlay = (ref: string | undefined): string | undefined =>
  resolveModelIdForProvider(ref, activeProvider) ?? ref?.trim() || undefined;

resolveModelIdForProvider('claude-cli:opus-cli', 'claude-cli') returns 'opus-cli' (→ .replace(/-cli$/,'')'opus'), while a legacy bare 'opus' resolves cross-provider to undefined and still falls through to ref.trim() untouched. Apply the same to the overlayRouting sub-models. Please also add a depsWithProvider('claude-cli') case to orchestratorRegistry.test.ts — the current tests only exercise anthropic/openai, which is why this slipped.

? ref?.trim() || undefined
: resolveModelIdForProvider(ref, activeProvider);

// Per-instance model resolution (issue #296 AC#2), three tiers:
// 1. the Agent's `model_routing.main` (operator's per-Agent choice)
// 2. the global seeded platform default `runtime.model`
// (= `orchestrator_model` install config = `ORCHESTRATOR_MODEL` env)
// 3. `DEFAULT_ORCHESTRATOR_MODEL` — guards against an empty / whitespace
// platform default so the turn loop never gets an empty model id.
const model =
resolveOverlay(routing.model) ||
runtime.model?.trim() ||
DEFAULT_ORCHESTRATOR_MODEL;

// Resolve the per-turn routing sub-models the same way. Any sub-model that
// does not resolve to the active provider falls back to the resolved `model`
// so every id the turn loop sends is a valid same-provider `modelId`.
const overlayRouting = routing.modelRouting
? {
classifierModel:
resolveOverlay(routing.modelRouting.classifierModel) ?? model,
simpleModel: resolveOverlay(routing.modelRouting.simpleModel) ?? model,
complexModel: resolveOverlay(routing.modelRouting.complexModel) ?? model,
}
: undefined;

return buildOrchestratorForAgent(
{
agentId: agent.slug,
model: routing.model ?? runtime.model,
model,
maxTokens: runtime.maxTokens,
maxToolIterations: runtime.maxToolIterations,
// Per-turn model routing: prefer the agent's own persisted routing
// (Agent Builder P5); otherwise fall back to the platform default
// `runtime.modelRouting` so registry-managed orchestrators still emit
// `turn_routing` and the UI renders the Haiku-triage badge (origin/main).
...((routing.modelRouting ?? runtime.modelRouting)
? { modelRouting: routing.modelRouting ?? runtime.modelRouting }
...((overlayRouting ?? runtime.modelRouting)
? { modelRouting: overlayRouting ?? runtime.modelRouting }
: {}),
...(runtime.loopRepeatSoft !== undefined
? { loopRepeatSoft: runtime.loopRepeatSoft }
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import type { Pool } from 'pg';

import { resolveModelRef } from '@omadia/llm-provider';

import {
AgentGraphStore,
type ScheduleRow,
Expand Down Expand Up @@ -110,6 +112,67 @@ export class ConfigValidationError extends Error {

const SLUG_RE = /^[a-z0-9](?:[a-z0-9-]{0,62}[a-z0-9])?$/;

/**
* Validate persisted `model_routing` JSON before write. Shape:
* { mode: 'single'|'triage', main: <ref>, triage?: <ref>, simple?: <ref> }
* Each `<ref>` must resolve via `resolveModelRef` (provider-qualified id,
* legacy alias, bare vendor id, or `class:*`). An empty `main` is illegal —
* use `null` to clear. Optional fields may be absent or empty-string (latter
* treated as absent so a UI dropdown's "(default)" choice round-trips cleanly).
*/
export function validateModelRoutingShape(
routing: Record<string, unknown>,
): void {
const mode = routing['mode'];
if (mode !== 'single' && mode !== 'triage') {
throw new ConfigValidationError(
`modelRouting.mode must be 'single' or 'triage' (got ${JSON.stringify(mode)})`,
);
}
const main = routing['main'];
if (typeof main !== 'string' || main.trim() === '') {
throw new ConfigValidationError(
`modelRouting.main is required (clear routing by passing null instead)`,
);
}
validateModelRef(`modelRouting.main`, main.trim());
if (mode === 'triage') {
for (const key of ['triage', 'simple'] as const) {
const raw = routing[key];
if (raw === undefined || raw === null) continue;
if (typeof raw !== 'string') {
throw new ConfigValidationError(
`modelRouting.${key} must be a string (got ${typeof raw})`,
);
}
const trimmed = raw.trim();
if (trimmed === '') continue;
validateModelRef(`modelRouting.${key}`, trimmed);
}
}
}

/**
* Throw `ConfigValidationError` when `ref` is non-empty and does not resolve
* to any model registered with `@omadia/llm-provider`. Used by every persisted
* model-id surface (orchestrator routing, sub-agent overrides) so the operator
* cannot pin runtime to an id the live provider set does not serve — an
* unknown id would 404 at every turn. Empty / whitespace `ref` is rejected —
* callers should skip the validator when clearing the field instead.
*/
export function validateModelRef(field: string, ref: string): void {
if (typeof ref !== 'string' || ref.trim() === '') {
throw new ConfigValidationError(
`${field} must be a non-empty model ref (clear with null instead)`,
);
}
if (resolveModelRef(ref.trim()) === undefined) {
throw new ConfigValidationError(
`${field} '${ref}' is not registered with any installed LLM provider`,
);
}
}

interface AgentDbRow {
id: string;
slug: string;
Expand Down Expand Up @@ -283,11 +346,18 @@ export class ConfigStore {
}

/** Agent Builder — set (or clear, with null) the per-agent model routing.
* Direct write (not COALESCE) so the operator can disable routing. */
* Direct write (not COALESCE) so the operator can disable routing.
*
* Validates `main`/`triage`/`simple` against `@omadia/llm-provider` so an
* operator (or a stale REST client) cannot pin an agent to a model id that
* no installed provider serves — that would crash every turn at runtime
* with `404 not_found_error`. `null` clears routing back to the platform
* default and skips validation. */
async setModelRouting(
id: string,
routing: Record<string, unknown> | null,
): Promise<AgentRow> {
if (routing) validateModelRoutingShape(routing);
const { rows } = await this.pool.query<AgentDbRow>(
`UPDATE agents SET model_routing = $2::jsonb, updated_at = now()
WHERE id = $1 RETURNING *`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,10 @@ export class OrchestratorRegistry {
agentId: action.agent.id,
plugins: plugins.filter((p) => p.enabled).map((p) => p.pluginId),
memoryScope,
effectiveModel: built.effectiveModel,
...(built.effectiveModelRouting
? { effectiveModelRouting: built.effectiveModelRouting }
: {}),
});
return;
}
Expand Down Expand Up @@ -376,6 +380,10 @@ export class OrchestratorRegistry {
agentId: action.agent.id,
reason: action.reason,
replaced: !!before,
effectiveModel: built.effectiveModel,
...(built.effectiveModelRouting
? { effectiveModelRouting: built.effectiveModelRouting }
: {}),
});
return;
}
Expand Down
Loading
Loading