feat(bench): check model availability before dispatch#621
Conversation
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 0b6fce6f
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
tangletools · auto-approval · reason: drewstone_author · 2026-07-25T01:23:36Z
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Concerns | 3 (3 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 108.8s (2 bridge agents) |
| Total | 108.8s |
💰 Value — sound-with-nits
Adds a one-token-per-model availability probe before benchmark dispatch that fails fast on dead/deprecated models; reuses the existing cheapest router primitive and respects the injected-transport-skip convention — coherent and in-grain, with one missed efficiency knob.
- What it does: runBenchmark now calls preflightModels() before opening any task: for live router runs (no injected
completetransport) it sends onemax_tokens: 1request per unique model (worker model + analyst model, deduped via Set) through the existingrouterChatWithUsage. A failure throws a model-named error before any surface.open()/supervisor spawn. Injectedcompletetransports skip it by default ( - Goals it achieves: Fail fast: a fleet benchmark run pointed at a deprecated, misconfigured, or quota-exhausted model currently wastes compute (opens artifacts, spawns the supervisor, drives tool loops) before the first router call fails mid-task. This turns that into one cheap request up front with a clear, model-named error. Secondary goal: keep offline/deterministic runs unaffected by defaulting the check off for
- Assessment: Good change, in the grain of the codebase. (1) It reuses
routerChatWithUsage, which the router-client header explicitly bills as 'the cheapest dial, no sandbox' — the right primitive for a probe. (2) It inherits the injected-complete-skips-network convention already established forrouterChatWithUsage/routerChatWithTools(router-client.ts:81, :209-211), so the skip-when-injected default is - Better / existing approach: No existing model-availability checker to reuse (grepped preflight|availability|healthCheck|warmup|modelCheck across src — the four preflight() call sites are corpus-integrity, circuit-breaker, and knowledge-Q&A, none touch models). A
/modelslisting endpoint would be a weaker check than a real one-token completion (listed ≠ serving). The one-token completion is the right call. One available imp - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content
🎯 Usefulness — sound-with-nits
Adds a cheap one-token-per-model availability probe at the top of runBenchmark, reusing the existing router client and respecting the injected-complete offline seam — every existing caller gets fast-fail for free.
- Integration:
preflightModelsruns as the first step ofrunBenchmark(src/runtime/run-benchmark.ts:178), so every existing caller automatically benefits. Confirmed live callers:src/runtime/strategy-evolution.ts:409(the multi-phase evolution loop, which calls runBenchmark repeatedly and gains the most from failing fast on a bad model),examples/strategy-suite/strategy-suite.ts:181, `bench/src/example - Fit with existing patterns: Matches the codebase grain precisely. (1) Reuses
routerChatWithUsage, which already encapsulates transient retry (429/5xx backoff) and the temperature-1 retry for thinking models (router-client.ts:86-111) — the preflight inherits that resilience for free, verified by the 'retries at temperature one' test. (2) Honors the samecomplete-injection offline seam the rest of the router surface uses - Real-world viability: Solid on the real paths. Parallel
Promise.allacross unique models is correct (independent probes). The probe sendsmaxTokens: 1, so it's one cheap request per unique model; content is discarded —routerChatWithUsageonly needsres.okto pass, so empty-content responses from thinking models don't cause false failures. Error wrapping preserves the model name and upstream message (`Benchmark - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
🎯 Usefulness Audit
🟡 Preflight probe omits reasoningEffort: 'none' for thinking models [robustness] ``
The default check at
src/runtime/run-benchmark.ts:147callsrouterChatWithUsagewith{ maxTokens: 1 }but noreasoningEffort. The router-client docs (router-client.ts:57-63) call 'none' load-bearing for binary/single-token decisions on thinking models, else they burn the token budget in reasoning_content. maxTokens:1 caps output tokens but not reasoning tokens on providers that meter them separately, so a thinking-model probe is slower/costlier than necessary. Correctness is unaffected
🟡 No 'check once, then trust' mode for repeated bench callers [ergonomics] ``
runStrategyEvolution(src/runtime/strategy-evolution.ts:409) callsrunBenchmarkonce per phase × generation, and each call now defaults to re-probing every unique model. For a long evolution run against known-good live models this is many redundant one-token round-trips. The escape hatch (modelPreflight: false) disables the check entirely rather than letting a caller pre-verify once and skip thereafter. A caller can thread their own once-cached callback, but it isn't ergonomic. Acceptabl
💰 Value Audit
🟡 Preflight omits reasoningEffort:'none' — the codebase's own knob for exactly this case [maintenance] ``
router-client.ts:56-63 documents
reasoningEffort: 'none'as the load-bearing value for 'binary/single-token decisions (routing, gating)' on thinking models, because otherwise they burn the whole token budget inside the think block and on slow backends that becomes a client timeout. A model-availability probe sending max_tokens:1 IS a single-token decision — the exact use case that knob was added for. run-benchmark.ts:147-155 passes only { maxTokens: 1 } and omits reasoningEffort, so a prefligh
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
✅ No Blockers —
|
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 2 non-blocking findings — 0b6fce6f
Full multi-shot audit completed 1/1 planned shots over 2 changed files. Global verifier still owns final merge decision.
Full immutable report for this review: trace
Summary comment for this run: full summary
tangletools · 2026-07-25T01:35:23Z · immutable trace
Closes #526.
runBenchmarknow sends one one-token request per unique live worker or analyst model before opening any task. A failed check aborts before fleet dispatch; injected completion transports remain deterministic and skip it by default. Callers can disable the check or provide a custom checker.Checks: focused 52/52; new regressions 7/7; root and example TypeScript; lint 491/491 files.