feat(auto-routing): owner-configurable Efficient model pools - #4846
Conversation
A bare session target intermittently resolves to a same-named window and fails with "index 0 in use". Use the session: colon form and record the learning.
… contracts Add Pool entry / Efficient model pool schemas with a collision-safe poolEntryKey, optional canonical variant on routing candidates and benchmark decisions with legacy reasoningEffort compatibility, the sparse CustomRoutingTableSchema, benchmark profile registry wire types, and owner pool settings contracts. Extend ADR 0002 with owner-pool invariants and record Auto Routing scope and canonical terms in CONTEXT.md.
…results Composite (model, variant) primary keys across run_models, model_summaries, and case_results with a generated migration that backfills variant from reasoning_effort; exact-pair queue messages, container lanes, carry matching, and summaries; routing-table candidates self-describe their variant. The published platform routing table JSON is unchanged in shape. Bounded contract extensions: optional variant on BenchmarkModelSummary and BenchmarkDeciderModel with legacy reasoningEffort compatibility.
…-e6e3 # Conflicts: # .kilo_workflow/dispatch-role.sh
When kilo-auto/efficient decisions carry a canonical variant, apply the full getModelVariants settings (reasoning + verbosity). Missing catalog keys fall back to balanced; legacy effort-only decisions stay unchanged.
Store Efficient model pools on the routing-mode DO, admit profiles before settings PUT, and resolve custom tables with exact-pair sticky and custom-only isActive fail-closed.
…atomic admission Add benchmark_profiles and profile_request_events D1 tables, one all-or-nothing admission batch with 10-per-owner-per-24h quota and global dedupe, concurrency-safe guarded upserts and conditional charges, and the internal register/status endpoints.
Cover the charged-event, register-upsert, and status-insert builders against node:sqlite so column-count or guard regressions fail the suite behind the drizzle mock.
Validate Efficient pool entries against the owner's effective managed catalog, proxy worker settings, annotate unavailable saved entries, and preserve benchmark quota 429 retryAt for the UI.
…, sparse custom tables Wire startRun(purpose, entries), terminal profile transitions, pending-batch drain on the existing one-active-decider slot with platform priority, POST /admin/custom-routing-table with provenance-bound assembly, and ops docs. Platform artifact path unchanged.
Extend AutoRoutingModeCard to manage mode + 1-10 pool entries via /api/auto-routing/settings, with benchmark status, polling, retry, and org billing-manager edit permission isolation.
…-e6e3 # Conflicts: # .kilo_workflow/dispatch-role.sh
A mixed save (mode set + pool cleared, or the reverse) fanned out to independent multi-key put and delete ops via Promise.all; one succeeding while the other threw diverged mode and pool despite the 'commit or fail together' contract. Wrap the mutation in ctx.storage.transaction and cover rollback with an honest transactional storage stub.
Production promotes web before workers, so every merge runs new web against workers that lack /admin/routing-settings (JSON 404). Load now falls back to the legacy mode route and reports poolSupported: false, hiding pool controls instead of failing the card; mode saves from that state use the legacy mode endpoint, which never touches pool keys at any worker version. A settings PUT that 404s (rollback mid-session from a supported UI) answers a retryable 503 rather than silently mis-saving.
…gration truth ADR 0002 and the benchmark README now match the shipped behavior: owner mode+pool commit in one DO storage transaction; the web-before-worker deploy window serves the legacy-synthesis fallback (poolSupported: false, mode-only legacy saves, retryable 503 on settings PUT 404); admission's DO-after-admit semantics are written down; and the rollback section stops claiming the D1 schema is additive now that 0005 rebuilds profile tables with backfill.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of Files Reviewed (1 file)
Scope Notes
Previous Review Summaries (4 snapshots, latest commit 0e09154)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 0e09154)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of Files Reviewed (4 files)
Scope Notes
Previous review (commit 061ff6f)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of Files Reviewed (1 file)
Scope Notes
Previous review (commit 3239b9d)Status: No Issues Found | Recommendation: Merge Executive SummaryThis incremental review confirms the prior admin-client/route.ts graceful-502 findings are fixed (JSON parse errors and schema-mismatched 2xx bodies now safely degrade to Files Reviewed (4 files)
Previous review (commit 608e3d5)Status: 3 Issues Found | Recommendation: Address before merge Executive SummaryThe most notable risk is in Overview
Issue Details (click to expand)WARNING
Files Reviewed (60 files)
Reviewed by claude-opus-5 · Input: 22 · Output: 2.6K · Cached: 384.6K Review guidance: REVIEW.md from base branch |
Verifier-authored from the owner-pool E2E run: deterministic benchmark profile D1 fixture facts (engine identity, switch_cost_factor floor, config prerequisites, provenance binding, uncharged stale re-admission) and local gateway/auth facts (org-context bearer path, balance top-up counter, fake-login cookie behavior, org deny-list fixture lever).
fetchAutoRoutingSettingsAdmin parsed success bodies with .parse(), so an unexpected-shape or non-JSON 2xx (old/rolled-back worker, proxy error page) threw through the route and surfaced as an unhandled 500 instead of the intended graceful 502. safeParse plus a guarded .json() now return the 502 result the route already maps, making the deploy-order safety net reachable. Addresses two Kilobot review threads on PR #4846.
|
(bot) @kilocode-bot please review the latest head 3239b9d — it addresses your two malformed-response findings from the previous head; the third thread was replied to with rationale and resolved. |
Follow-up review at
|
|
Review already posted at #4846 (comment) — both findings confirmed fixed, third thread confirmed unchanged/out of scope. No further action needed from my side; the non-blocking observations (404-body edge case, missing logging, |
…-e6e3 # Conflicts: # .kilo_workflow/learnings/tmux-new-window-index-in-use-name-prefix-collision.md
…ngs PUT Spread the request body first so parsed.data.ownerType/ownerId always come from resolveOwner (auth/query), matching the comment's claim structurally. Benign today (downstream uses owner.*), but removes the future-trap of parsed.data looking authoritative for owner identity.
iscekic
left a comment
iscekic
left a comment
There was a problem hiding this comment.
Self-review pass complete — all 65 files read in full against the ADR invariants.
Verified end-to-end: exact-pair identity holds at every hop (picker → contracts XOR rules → D1 composite keys with rollback-safe rebuild/backfill → provenance-bound sparse assembly → fail-closed custom decide → variant-aware stickiness → gateway getModelVariants); admission-before-persist ordering with all-or-nothing quota and globally deduplicated charging; platform/profile single-slot sharing with platform priority; the deploy-order matrix (404→legacy synthesis with poolSupported: false, mode-only legacy saves, PUT 404→retryable 503, no legacy PUT fallback).
One finding, fixed directly in 061ff6f: the settings PUT contract parse spread the client body after auth-resolved owner identity, letting the body override ownerType/ownerId in parsed.data (benign today since downstream uses owner.*, but a future-trap contradicting the comment). Now auth identity always wins; 26/26 route tests pass.
Non-blocking nits noted but not actioned: BenchmarkProfileOwnerTypeSchema/AutoRoutingModeOwnerTypeSchema enum sync rests on a comment; a dead unknown_variant fallback branch in validateSingleEntry.
jeanduplessis
left a comment
jeanduplessis
left a comment
There was a problem hiding this comment.
Review finding: profile claim failures can repeatedly launch duplicate benchmark runs.
Swallowing markProfilesRunningForRun errors left profiles pending while work still enqueued, so drain could start duplicate runs after transient D1 failures. Fail the inserted run, free the slot, and rethrow before enqueue. Split Efficient model pools into ADR 0003; restore ADR 0002.
2 participants
Summary
What.
kilo-auto/efficientcan now be customized per owner: a personal user or an organization configures 1–10 exact(managed model, canonical catalog variant)pairs on the existing Auto routing card. Missing exact pairs are benchmarked on demand through the existing benchmark Worker; routing then decides only among ready, compatible entries and keeps the balanced fallback whenever none are usable.Why. Owners want the Efficient auto model constrained to the exact models and variants they trust (same-model
xhighvsmaxstay distinct end-to-end) without losing the platform default when their pool cannot serve.How.
packages/auto-routing-contracts), ADR 0002 updates, canonical terms in CONTEXT.md.reasoningEffortdecisions stay parseable.poolSupported: falsehides pool controls; mode saves stay mode-only at every worker version; a mid-session rollback answers a retryable 503 instead of mis-saving). Malformed worker settings responses degrade to a graceful 502.Verification
claude-haiku-4.5; clearing restored personalclaude-sonnet-4.5, instrumented proof; mode and pool saves independent).xhigh+maxpersist and render as distinct rows and distinct D1 PKs.retryAtcopy, atomically (zero partial rows).kilo-auto/efficientcalls only): seeded ready pool constrained the decision to pool entries (custom table, 18 routes, threshold-winner xhigh); stale profiles → balanced fallback with uncharged re-admission; no pool → default platform path.Visual Changes
Local screenshots of the Auto routing card on
/profile(desktop + 375px). Survey modal may appear for fresh seed users — dismiss it; the pool editor is underneath.Reviewer Notes
getModelVariants); admission vs save atomicity across the two workers (quota 429 withretryAt, all-or-nothing D1 admission, one DO storage transaction); the deploy-order fallback matrix (old web + new worker, new web + old worker, rollback mid-session).Follow-ups noted during review (non-blocking)
createWorkerAdminFetchkeeps the throw-on-malformed convention for the sibling admin routes; the settings client now sets the safer safeParse→502 precedent those can adopt.502rather than the upstream status; genuine worker error responses are always JSON, so this only affects edge cases that surface as the same retryable load/save error either way.