Repository navigation
fix(auto-routing): address custom router review follow-ups - #8084
Merged
Merged
Conversation
Cap decision-token lifetime at verification, document fallback classification fields, check fallback entries against request capabilities, bound public request bodies, enforce the profile admission quota inside the admission batch, and resume unacknowledged grader usage on a retried client eval upload.
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe incremental change resolves the previously reported reliability gap: unreadable pending grader usage no longer wedges retries, and the added test covers the drop-and-regrade path. Files Reviewed (2 files)
Previous Review Summary (commit 5dd7335)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 5dd7335)Status: 1 Issues Found | Recommendation: Address before merge Executive SummaryOne low-severity reliability gap in the client-eval grader-usage retry path: a stored pending usage record that cannot be parsed permanently blocks retries for that case. Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (16 files)
Reviewed by deepseek-v4.1-flash-1017 · Input: 35.4K · Output: 5.6K · Cached: 290.8K Review guidance: REVIEW.md from base branch |
…usage is unreadable
…iew-followups # Conflicts: # services/auto-routing-benchmark/migrations/meta/0025_snapshot.json # services/auto-routing-benchmark/migrations/meta/_journal.json # services/auto-routing-benchmark/src/profiles.ts
eshurakov
approved these changes
Oct 19, 2026
This was referenced Oct 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up for maintainer review remarks left open on #7784, #7791 and #7793 after they merged.
Remarks and resolutions
#7784: decision-token lifetime is not enforced (thread)
Fixed.
verifyAutoDecisionnow rejects a token whose expiry is later thanAUTO_DECISION_TTL_MSplus a small clock-skew margin; the signer and the verifier run on different hosts, so the margin is needed. Test: a token signed with a longer expiry is rejected, and a full-lifetime token is still accepted.#7784:
fallback: truewith classifier fields (thread)The contract comment was wrong. The worker sets
fallback: truewhen no routing decision names a servable entry, and that includes the case where the classifier succeeded but the table has no servable pick.taskType/subtaskTypethen report that classification, and decision logs and the CLI use it. I corrected the field comment and did not add a refinement. Test: the schema accepts a fallback decision that carries a classification.#7791: fallback can pick an entry that cannot serve the request (thread)
Fixed.
DecisionRunnow carries the request constraints and the capability map. Fallback picks only a servable entry that is not known to be inactive, has every enforced required modality, and does not have a context window that is provably too small. If no entry qualifies, it returnscustom_router_unavailable. Tests: a default entry without image support is skipped, and a request after a classifier failure with no compatible entry returns 422.#7791: public request bodies are buffered without a size limit (thread)
Fixed. Public routes read the body through
readBoundedStreamand stop at the first byte over the limit, soContent-Lengthis not trusted. An oversized body returnsinvalid_request. The limit matches the largest upstream model request size, because a decide request carries the whole conversation. Test: a body exactly at the limit is routed, and one byte more is rejected.#7793: concurrent admissions can exceed the per-router quota (thread)
Fixed. A charged admission batch now ends with a guard statement. The guard recounts the rolling events inside the same D1 transaction and fails the batch when that batch's own events go over the limit, so the whole admission rolls back. The caller maps that rollback to the normal quota error. Test (real SQLite): two concurrent requests at the boundary admit exactly one model.
#7793: billing failure followed by a failed compensating delete leaves an unbilled result (thread)
Fixed. A graded result now stores its grader usage record in
client_case_results.pending_grader_usage(new nullable column, generated migration). The column is cleared only after the usage writer acknowledges the record. A retry that finds a stored result with a pending record sends the same record again (same usage ID, same payer) before it returns the score. The existing delete on billing failure is unchanged. Test: billing fails, the delete fails, then a retry records the identical usage once, and later retries do not charge again.Verification
On a disposable VM:
tsgotypecheck forpackages/auto-routing-contracts,services/auto-routing,services/auto-routing-benchmarkandpackages/web-shared; oxlint and oxfmt on the changed sources; full vitest suites for the three auto-routing packages (all passed). Each new regression test failed against themainsources and passed with this change.