feat: auto model routing for SkillFlows#51
Conversation
|
Hi @RiteshTiwari1 — apologies for the lack of review, and thanks for putting this together. The routing logic in model-routing.ts looks solid — clean priority chain, and defaults to the reasoning tier when a task is ambiguous so cost savings don't quietly hurt quality. One thing that's changed since this was opened: src/voice/server.ts, where the routing gets wired into the SkillFlow execution loop, was removed in the 2.0 split (#56) — voice moved into its own @open-gitagent/voice package. That's why this now shows conflicts. The loader.ts, skills.ts, and workflows.ts changes still apply cleanly; it's just the execution wiring that needs a new home. Let us know if you'd like to pick this back up against the new structure — happy to help figure out where it should hook in. Thanks again for the contribution. |
e99c8ac to
f3ea30f
Compare
|
Thanks @Nivesh353. Rebased onto main (v2.0.2) and dropped the The per-step wiring belongs in Two questions:
|
|
Thanks @RiteshTiwari1
|
shreyas-lyzr
left a comment
There was a problem hiding this comment.
Four issues across the new model-routing module. The most significant is the classifier design — covered in the inline comments below.
- Keyword classifier produces systematic misclassifications (blocking — see inline).
- 'search' is bucketed as reasoning; common lightweight use (grep/find/lookup) will always overpay.
- resolveModelAlias returns undefined when a tier alias is configured but the routing block omits that tier's model string — silently skips the tier instead of warning. This can silently downgrade to fallback with no observability.
- No integration/end-to-end test exercises resolveRoutedModel being called from actual SkillFlow execution — the unit tests cover the logic in isolation but there's no test proving the plumbing from workflows.ts through to the routed model call.
Security pass: no CVEs in changed deps (no new packages added), no hardcoded secrets, no injection surface — the classifyText input is only used in a regex match, not in any eval/exec/query path.
| const DEFAULT_LIGHTWEIGHT = [ | ||
| "summ", "extract", "classif", "transform", "format", "convert", | ||
| "parse", "fetch", "read", "load", "lookup", "normaliz", "translat", | ||
| "rephrase", "rewrite", "tag", "label", "render", |
There was a problem hiding this comment.
Blocking: the keyword-prefix classifier is fundamentally fragile for this use case.
The core problem is that task complexity is a semantic property that does not reliably map to a fixed prefix vocabulary. Real SkillFlow prompts will break this in both directions:
Lightweight classified as reasoning (false-positive tax):
- "verify the file exists" → matches 'verif' → reasoning (but this is a trivial existence check)
- "review the spelling in this string" → matches 'review' → reasoning
- "validate that the field is not empty" → matches 'validat' → reasoning
- "search for the word hello in the file" → matches 'search' → reasoning
Reasoning classified as lightweight (silent quality downgrade, the worse case):
- "read the code and identify bugs" → matches 'read' → lightweight (but this is a debugging task)
- "fetch user data and detect anomalies" → matches 'fetch' → lightweight (complex analysis)
- "parse and understand the business requirements" → matches 'parse' → lightweight
- "load the config, then decide which deployment strategy to use" → matches 'load' → lightweight
The read and fetch entries in DEFAULT_LIGHTWEIGHT are especially risky: they fire on the first verb in a compound prompt, classifying the whole task by only part of it.
Suggested fix — replace classifyTaskTier with a single cheap model call:
async function classifyTaskTierLLM(
classifyText: string,
lightweightModel: string, // the cheap model is appropriate to classify itself
): Promise<ModelTier> {
const prompt = [
"Classify the following task as either 'lightweight' (summarize, extract, format,",
"convert, parse, fetch, render — mechanical, no reasoning required) or 'reasoning'",
"(plan, analyze, decide, debug, review, validate logic, orchestrate — needs judgment).",
"Reply with exactly one word: lightweight or reasoning.",
"",
"Task: " + classifyText,
].join("\n");
const response = await callModel(lightweightModel, prompt, { max_tokens: 5 });
const word = response.trim().toLowerCase();
return word === "lightweight" ? "lightweight" : "reasoning"; // unknown → reasoning (safe default)
}This costs one gpt-4o-mini call per auto-classified step (~0.0001 USD at current pricing), which is negligible compared to the step itself, and it handles compound prompts, novel verbs, and context correctly. The existing user-rule override path (rules:) can stay as-is for deterministic overrides where operators want guaranteed behavior.
If a synchronous API is required (e.g. the call site can't be made async), the keyword approach is acceptable as a degraded fallback, but the DEFAULT_LIGHTWEIGHT list should at minimum remove 'read', 'fetch', 'load', and 'search' — these verbs are too context-sensitive to use as tier signals.
| ]; | ||
| const DEFAULT_REASONING = [ | ||
| "search", "analy", "plan", "decid", "decision", "orchestrat", "solve", | ||
| "reason", "validat", "evaluat", "review", "audit", "diagnos", "debug", |
There was a problem hiding this comment.
The 'search' keyword in DEFAULT_REASONING catches all uses of the word — including trivial lookup/grep steps that should route to the lightweight model. Consider removing it or narrowing it to more specific terms like 'research', 'investigate', or 'deep-search'. As-is, any step prompt containing 'search' (e.g. 'search the file for pattern X', 'full-text search the index') pays the reasoning model rate.
| if (!ref) return undefined; | ||
| if (ref === "lightweight") return routing?.lightweight || undefined; | ||
| if (ref === "reasoning") return routing?.reasoning || undefined; | ||
| return ref; |
There was a problem hiding this comment.
Silent fallback when a tier alias has no backing model.
If a SKILL.md specifies model: lightweight but the agent.yaml routing block omits lightweight:, resolveModelAlias returns undefined, and the caller falls through to primaryModel without any log or warning. The operator's explicit intention ("use a lightweight model for this skill") is silently ignored.
Consider emitting a warning — or throwing — when an alias resolves to undefined:
export function resolveModelAlias(ref: string | undefined, routing?: RoutingConfig): string | undefined {
if (!ref) return undefined;
if (ref === "lightweight") {
if (!routing?.lightweight) {
console.warn(`[routing] tier alias 'lightweight' used but routing.lightweight is not configured; falling through`);
}
return routing?.lightweight || undefined;
}
if (ref === "reasoning") {
if (!routing?.reasoning) {
console.warn(`[routing] tier alias 'reasoning' used but routing.reasoning is not configured; falling through`);
}
return routing?.reasoning || undefined;
}
return ref;
}| }); | ||
| assert.equal(r.model, "openai:gpt-5-reasoning"); | ||
| assert.equal(r.source, "fallback"); | ||
| }); |
There was a problem hiding this comment.
The test suite only covers the pure functions in model-routing.ts. There is no test that wires a SkillFlowStep with a model field through loadFlowDefinition/discoverWorkflows and verifies that the model field is preserved and passed to resolveRoutedModel correctly. A roundtrip test (serialize → deserialize via saveFlowDefinition → loadFlowDefinition, assert step.model survives) would catch regressions in the YAML plumbing.
|
Thanks @shreyas-lyzr , all four are fair. 2, 3, 4 I'll fix: drop search from the reasoning defaults, warn when a tier alias resolves to no configured model, and add a save→load roundtrip test for step model. On 1: agreed keywords can't capture semantic complexity. Quick check before I rework —> an LLM classify call has to be async, so it'd live in voice's |
Addresses #48
Summary
Adds automatic model routing for SkillFlows. Each step is classified by
complexity at runtime and routed to the right model — lightweight tasks
(summarize / extract / classify / transform) run on a cheaper model, while
reasoning-intensive tasks (planning / decision-making / tool orchestration)
stay on the configured reasoning model. Reduces token usage and cost without
sacrificing quality on complex steps.
How it works
Model resolution per step, in priority order:
model:inworkflows/*.yamlmodel:inSKILL.mdmodel.routingtierspreferredmodelUnknown / ambiguous tasks default to reasoning, so cost optimization never
silently degrades quality. Routing is opt-in — active only when a
model.routingblock is present, so existing agents are unaffected.Configuration (
agent.yaml)Scope note
Routing is applied at the SkillFlow step level — where GitAgent runs
discrete tasks in sequence. Per-turn routing inside a single agent loop would
need deeper
pi-agent-corechanges; happy to follow up if that's preferred.