Repository navigation
🎚️ feat: Add Never-Throwing Rerank Outcomes With Per-Document System One, Cross-Encoder, BM25, and Fallback Chains - #630
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ec8ac2b70
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (gate && reranker.kind !== 'none') { | ||
| let why: string | null | undefined; | ||
| try { | ||
| why = await gate(reranker); |
There was a problem hiding this comment.
When gate returns a slow or never-settling promise, this direct await bypasses both the request deadline and caller abort signal, so rank() can hang indefinitely despite advertising that deadlineMs bounds the whole chain. Even if the gate eventually resolves after the budget expires, execution proceeds to runStep and may start another provider request; race the gate against the remaining deadline/abort and recheck expiration before invoking the step.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c905ab5. withFallback now waits for the gate through consult(), which races it against the chain's remaining deadline and the request signal; a gate still pending is abandoned and the chain stops with timeout or aborted. After a gate allows a step, abort and expiry are checked again before runStep, so no step starts on a spent budget. Proof: withFallback › abandons a gate at the chain deadline and starts no step after it (a never-settling gate with deadlineMs: 50 returns timeout well under 500 ms; a gate resolving null at 200 ms returns timeout before 180 ms; an abort at 20 ms returns aborted; the step is never called). It fails with the race removed.
| return deadline.signal.aborted && !isAborted(outer) | ||
| ? asTimeout(outcome) | ||
| : outcome; |
There was a problem hiding this comment.
If an OutcomeReranker ignores the deadline abort but resolves successfully during the 50 ms grace window, deadline.signal.aborted is true here, yet asTimeout() only rewrites an existing aborted reason and leaves the ok outcome unchanged. The fallback chain therefore accepts the late result and skips the next step even though this step's deadline already expired; convert any outcome completed after the deadline into a timeout failure while preserving its reported usage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this; the behavior is intended. The withFallback contract defines a failed step as one that "throws, rejects, returns something that breaks the RerankOutcome contract, or outlives its deadline plus STEP_GRACE_MS": a full, validated answer inside deadline+grace is by definition not a failed step. Accepting it costs no extra latency, because the timeout path also returns at deadline+grace; converting it to a timeout would discard a valid ranking and then either spend more time and money on the next step or, when the step deadline is the chain's remaining budget, return identity order. It also matches the package's own adapters: toOutcomeReranker and createCrossEncoderReranker both return ok when the underlying call resolves after its signal aborted. asTimeout only relabels the step's own deadline abort as timeout for the chain's caller. In c905ab5 the STEP_GRACE_MS JSDoc and README now say a full answer within the grace is accepted, and withFallback › accepts a full answer that arrives within the step grace after its deadline pins it.
| const keyFor = (document: string): RerankCacheKey => ({ | ||
| query, | ||
| document, | ||
| model: configured, | ||
| rubric: rubricKey, | ||
| }); |
There was a problem hiding this comment.
When callers supply state and share a cache, two rerankers with the same query, document, model, rubric, and instructions but different effective decision state produce the same key here. The second reranker then silently reuses a score measured under the first state instead of invoking its model, which corrupts context- or tenant-specific rankings; include the generated state or a caller-provided state namespace in the cache identity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c905ab5. RerankCacheKey has a state field: the serialized decision state when a custom state builds the request, empty for the default { query, document }, and createScoreCache keys on it. The state is built once per document and shared by the key and every request (including the re-issue), so the key always matches what was measured; a state that cannot be built or serialized leaves that document uncached. Proof: System One per-document ranking › keys cached scores on a custom state, so rerankers judging in different states never share one (tenant a scores 0, tenant b on a shared cache scores 2 with calls: 1, cached: 0, and a third reranker with tenant b hits the cache). It fails with key.state dropped from the cache key. README updated.
| isDecisionObject(value) && | ||
| typeof value.status === 'string' && | ||
| STATUSES.has(value.status) && | ||
| isRerankPermutation(value.results, documents) && |
There was a problem hiding this comment.
For an injected OutcomeReranker that returns every index exactly once but leaves the entries unsorted (or places a null score before a numeric score), isOutcome() accepts it because this check validates only the permutation. withFallback can consequently return an ok outcome whose displayed order violates the documented score-descending invariant rather than trying the next step; validate ordering and stable ties here as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c905ab5, for the whole contract rather than just the order. isOutcome now also requires isRankedOrder(results) (score descending, ties in input order, null last, kept boolean when set), a known provider, a string model, a string-or-null reason, a finite threshold and boolean pinVerified when present, and every attempt well-formed (known provider, string model, valid status, string-or-null reason, finite durationMs, usage object), so rerankCost can no longer throw on a passed-through attempt. withFallback also refuses at construction a step whose reranker has an unknown kind or non-string model, since failed and gate-skipped attempts are labelled with them. Proof: the withFallback › treats a step returning %s as invalid_results and moves on table gained unsorted scores, a null before a number, reversed ties, a non-boolean kept, an attempt without usage, an unknown provider, a non-string reason, a non-numeric threshold and a non-boolean pinVerified; the order cases fail with isRankedOrder removed, the attempt case with isAttempt reverted, and the field cases with those checks removed. README and JSDoc updated.
| async rank(request: RankRequest): Promise<RerankOutcome> { | ||
| return identityOutcome(documentCount(request), { reason }); | ||
| }, |
There was a problem hiding this comment.
A valid call to noneReranker returns identityOutcome, whose attempts array is empty. The same empty array is spread by withFallback, so a configured none step that actually ran is absent from the promised per-step attempt history, preventing callers from reconstructing which fallback was selected or why it was skipped; return a skipped attempt with this reason and zero usage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c905ab5, in both places. noneReranker now returns one skipped attempt (provider none, its reason, zero usage) via stepOutcome({ called: false }). The aggregator in withFallback also records a step that ran but reported no attempts (any custom OutcomeReranker) from its own provider, model, status, reason and usage, so attempts keeps every step. Proof: withFallback › records a step that ran but reported no attempt, so the history keeps every step (standalone noneReranker attempt plus a silent custom bm25 step in a chain); it fails with either half reverted. The existing gate test now expects ['skipped', 'no_api_key'] for the none step.
| if (n === 0 || per === 0 || kind === 'bm25' || kind === 'none') { | ||
| return { estimatedTokens: 0, estimatedSearchUnits: 0 }; |
There was a problem hiding this comment.
When a caller passes tokensPerDocument: 0—for example, to disable token estimates for a provider billed only in search units—this early return also zeros estimatedSearchUnits. Cross-encoder search-unit billing depends on the document count, not the token estimate, so timed-out requests can disappear from budgets and rerankCost; compute search units independently whenever the cross-encoder received documents.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c905ab5. estimatedUsage no longer returns early on a zero token estimate: search units are Math.ceil(n / 100) for any cross-encoder request with documents, and only estimatedTokens uses the per-document figure. The same coupling in fallback.ts's abandonedUsage (it returned no usage when the token estimate was 0) now checks the kind instead. Proof: cross-encoder ranking › estimates search units from the document count even with no token estimate (estimatedUsage('cross-encoder', 4, 0) is {0, 1}, a timed-out request with estimatedTokensPerDocument: 0 carries estimatedSearchUnits: 1, and rerankCost prices it at 0.002); it fails with the old guard restored.
| const hit = cache.get(keys[index]); | ||
| if (typeof hit === 'number' && Number.isFinite(hit)) { | ||
| run.scores[index] = hit; | ||
| run.usage.cached += 1; |
There was a problem hiding this comment.
When a host cache is corrupted or populated externally with a finite score below zero or above the highest rubric index, this check accepts it without the range validation applied to provider responses. A fully cached request can therefore return ok with an impossible score that dominates the ranking or produces incorrect kept values; treat out-of-range cache values as misses.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c905ab5. One inRubric predicate (finite, >= 0, <= rubric.length - 1) now guards provider answers in document mode, batch answers, and cache reads; an out-of-range cached value is a miss, so the document is scored by the model. Proof: System One per-document ranking › treats an out-of-rubric score from a host cache as a miss (a host cache returning 99 and -5: both documents are sent, cached: 0, and the provider's scores win); it fails with the old finite-only cache check.
| if (run.stop !== null || !live(signal)) { | ||
| return; | ||
| } | ||
| run.usage.calls += 1; |
There was a problem hiding this comment.
When the decision model refuses locally—for example, an open breaker returns circuit_open before transport—this counter is incremented before model.decide, and refused() never removes it. The outcome consequently reports a sent call even though the test scenario explicitly sends no HTTP request, making request-budget and attempt telemetry inaccurate; only count transport work or undo the count for failures known to occur before dispatch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c905ab5, in every mode. Document mode takes back the call when model.decide rejects with circuitOpen (refused before dispatch) and does not count it as a reply, so nothing is estimated; a step that ended with calls: 0 is a skipped attempt rather than failed. toOutcomeReranker (batch mode) does the same: an open-breaker rejection is calls: 0, no estimate, called: false. The cross-encoder already counted at fetch; its failure path now passes called: usage.calls > 0 so a failure before any fetch is skipped too. Proof: System One per-document ranking › an open breaker is circuit_open and sends nothing now also ranks 4 documents at concurrency 4 and in batch mode behind the open breaker, expecting calls: 0, estimatedTokens: 0 and a skipped attempt; it fails with the decrement, the called flag, or the batch change reverted. README and the toOutcomeReranker JSDoc updated.
| export function createScoreCache( | ||
| options: ScoreCacheOptions = {} | ||
| ): MemoryScoreCache { | ||
| const max = Math.max(1, Math.floor(options.max ?? 20_000)); |
There was a problem hiding this comment.
If max is NaN or Infinity—for example after parsing malformed host configuration—this expression preserves that non-finite value. The eviction comparison then never removes entries, turning the advertised bounded cache into an indefinitely growing map; validate max as a finite positive integer before constructing the cache.
AGENTS.md reference: AGENTS.md:L45-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c905ab5, for max and ttlMs. createScoreCache now throws DecisionError('bad_request', 'invalid score cache configuration') unless max is a positive safe integer and ttlMs is a positive finite number, matching how the other rerank constructors refuse bad configuration. Proof: System One per-document ranking › refuses a score cache bound that would unbound it (NaN, Infinity, 0 and 1.5 for max; NaN, Infinity and -1 for ttlMs; plus eviction at max: 2); it fails with the old Math.max(1, Math.floor(...)). README and option JSDoc updated.
7f862be to
ed2e58c
Compare
c905ab5 to
e11bd03
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e11bd033cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
| }); | ||
| try { | ||
| const outcome = await Promise.race([work, late]); |
There was a problem hiding this comment.
When an injected OutcomeReranker ignores its signal, aborting the caller does not settle this race because the only competing promise fires at the step deadline; without a step or request deadline, line 308 returns the unresolved work directly. Consequently rank() can remain pending until the full step timeout—or forever—after cancellation. Add an abort contender that returns an aborted outcome and removes its listener when the work settles.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 51c5d41. runStep now races an abort contender for the caller's signal whenever one is passed, with or without a step or request deadline. The early return work only applies when there is neither a deadline nor a signal. If the step is still pending STEP_GRACE_MS after the abort, the wait ends as a failed aborted step charged at abandonedUsage. An already-aborted signal resolves immediately, and the listener is removed in finally. The grace lets a cooperative step still report its own spend, the same way it can at the step deadline.
Tests in src/rerank/outcome.test.ts:
withFallback › a caller abort ends the wait for a step that ignores it, with or without a deadlinecovers bothdeadlineMs: undefinedand2000. It expectsabortedwithin 500 ms, nottimeout. It fails with fallback.ts reverted (the chain is still pending at 1 s).a cooperative step that honors the caller abort reports its own spendguards the grace.
| isRerankPermutation(value.results, documents) && | ||
| isRankedOrder(value.results) && |
There was a problem hiding this comment.
When an injected step returns status: 'ok' with a full, sorted permutation containing one or more null scores, these checks accept it and withFallback treats the step as the answer instead of trying the next provider. This exposes a partial ranking as successful even though native rerankers use skipped for unscored documents; validate status/result semantics so a non-skipped outcome cannot contain null scores.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 51c5d41. isOutcome now also requires two things: an outcome whose status is not skipped must score every document, and every entry's kept must be undefined unless the outcome carries a threshold. A violating step is invalid_results, and the chain falls through to the next step.
Tests: new rows in withFallback › treats a step returning %s as invalid_results and moves on for a null score in an answer, a null score in a degraded answer, and kept without a threshold. All three fail with fallback.ts reverted. The README and withFallback JSDoc now list both rules.
| } | ||
| } | ||
|
|
||
| const expiresAt = performance.now() + deadlineMs; |
There was a problem hiding this comment.
When document mode uses a cache with custom state, all state builders and cache lookups run before this expiration time and the later withDecisionDeadline are started. If either synchronous host callback consumes the requested budget, the reranker still begins provider requests with a fresh full deadline, so the call can outlive the advertised whole-ranking deadline and dispatch work after it should have expired; anchor expiration at startedAt and pass only the remaining budget to the provider phase.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 51c5d41. Document mode now sets expiresAt = startedAt + deadlineMs. If the budget is gone after state building and cache lookups and any document is still unscored, it concludes as a not-called timeout without sending anything. Otherwise withDecisionDeadline gets only what is left. A ranking the cache answers in full is still served, since serving it sends nothing.
The same class showed up inside ask: a slow custom state builder could finish after the deadline and still dispatch. Each request now checks the remaining time after building its state. Past the deadline it sends nothing, and that request is neither counted nor estimated.
Tests in src/rerank/outcome.test.ts:
counts its deadline from the call, so slow cache lookups spend it and nothing is sent after itcovers the slow cache: 0 requests,skipped/timeout, andrerank()rejects withtimeout. It also covers the full-cache-hit case still beingok. It fails with only theexpiresAtanchor reverted.sends no request once building a custom state has spent the deadlinefails with only the dispatch check disabled.
| const question = scoreQuestion( | ||
| options.instructions ?? DEFAULT_INSTRUCTIONS, | ||
| rubric | ||
| ); |
There was a problem hiding this comment.
When instructions is an array or object, scoreQuestion retains the caller-owned reference while rubricKey serializes its current value only once. If the caller mutates that object after constructing the reranker, subsequent requests are judged under the changed instructions but cache reads and writes still use the old identity, allowing a shared cache to return scores measured under different instructions; deep-snapshot the instruction value before using it for both the question and key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 51c5d41. Document mode now takes one snapshot, JSON.parse(JSON.stringify(options.instructions ?? DEFAULT_INSTRUCTIONS)), at construction. That snapshot builds both scoreQuestion(...) and rubricKey, so later mutation by the caller changes neither the question nor its cache key. Instructions that can't be serialized (circular) now throw invalid reranker configuration instead of a raw TypeError. JSON round-tripping doesn't change JSON.stringify output, so keys from existing caches are unchanged.
Tests:
snapshots structured instructions, so a caller mutating them never splits what is asked from what is cachedmutatesstrictafter the first ranking and asserts that every request still sent{ strict: false }. It fails with the snapshot reverted.- The circular case is in
rejects a malformed timeout, concurrency, token estimate, or instructions instead of coercing it.
| const problem = rankRequestProblem(request); | ||
| if (problem !== undefined) { | ||
| return stepOutcome({ ...base, reason: problem, called: false }); | ||
| } | ||
| try { |
There was a problem hiding this comment.
For a direct BM25 call whose signal is already aborted, request validation succeeds and the implementation tokenizes and scores every document before returning an ok outcome. Large inputs therefore consume synchronous CPU and can be served after their caller has cancelled, unlike the other outcome rerankers; check isAborted(request.signal) before beginning the scoring pass and return an aborted skipped outcome.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 51c5d41. BM25 rank() now returns skipped/aborted with called: false when the signal is already aborted. The check comes right after request validation, in the same order as the cross-encoder. The other OutcomeRerankers (toOutcomeReranker, cross-encoder, System One) already checked this. Scoring is synchronous, so the BM25 JSDoc and README now say that neither an abort nor deadlineMs stops a pass once it starts. The fix doesn't claim mid-run cancellation.
Test: createBm25Reranker › answers an already-aborted request as aborted without tokenizing anything in src/rerank/bm25.test.ts asserts that the tokenizer is never called. It fails with bm25.ts reverted.
| if (usage.calls > 0 && (reason === 'timeout' || reason === 'aborted')) { | ||
| Object.assign( | ||
| usage, | ||
| estimatedUsage(kind, documents, options.estimatedTokensPerDocument) |
There was a problem hiding this comment.
When estimatedTokensPerDocument is NaN, infinite, or negative—for example after parsing malformed host configuration—this timeout path passes it to estimatedUsage, whose count sanitizer silently converts it to zero. A timed-out cross-encoder request can then disappear from token-based budgets and cost telemetry even though it was sent; validate this option as a finite nonnegative number when constructing the reranker (and apply the same validation to the equivalent System One option).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 51c5d41. Both constructors now reject an estimatedTokensPerDocument that is not undefined or a finite number of at least 0 (the shared isTokenEstimate in outcome.ts), and throw invalid reranker configuration. 0 stays a valid opt-out.
The rest of the class is also covered in System One:
timeoutMsis validated at construction.- A
concurrencythat is not a positive integer throws instead of being silently clamped to 1. estimatedTokensPerDocumentjoins the document-only options, so batch mode, which never used it, now rejects it like the others.
Tests:
cross-encoder ranking › rejects a token estimate that is not a finite count instead of charging nothingcovers NaN, -5, Infinity, andNumber('abc').System One per-document ranking › rejects a malformed timeout, concurrency, token estimate, or instructions instead of coercing it.rejects document-mode options in batch mode.
Each fails with its file's fix reverted.
fb6f2b5 to
83533ae
Compare
51c5d41 to
8829f97
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8829f9733e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (run.stop !== null || !live(signal)) { | ||
| return; | ||
| } | ||
| run.usage.calls += 1; |
There was a problem hiding this comment.
When a document-mode caller supplies a state callback that throws, this increments usage.calls before stateFor(index) runs, and the catch path treats the local exception as a replied network failure without undoing the increment. The retry repeats this, so no model.decide call is made but the outcome reports two sent calls per document, corrupting request budgets and telemetry; construct the state before incrementing or roll the count back for every pre-dispatch failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7db710b. ask now counts a call only immediately before model.decide runs (after stateFor and the deadline check), and refused(error, sent) counts a reply only for a dispatched request. A throwing custom state or a spent deadline is therefore neither a call nor a reply, and the circuit-open rollback applies only to a dispatched call. Test: src/rerank/outcome.test.ts › 'counts nothing for a custom state that throws, since no request is sent'. With the count moved back before stateFor, it fails (calls: 6 and a failed attempt for three documents and zero decide calls); with the fix it gets usage all zero and a skipped/error attempt, and a state that throws for one of three documents gives calls: 2 and partial.
| ATTEMPT_STATUSES.has(value.status) && | ||
| isReason(value.reason) && | ||
| isFiniteNumber(value.durationMs) && | ||
| isDecisionObject(value.usage) |
There was a problem hiding this comment.
For an injected OutcomeReranker, an otherwise valid outcome whose attempt has usage: {} or contains negative/NaN counters passes this check because only object-ness is tested. withFallback then returns that malformed attempt verbatim while sanitizing the top-level usage to zero, leaving the attempt history inconsistent with the advertised total and unsafe for telemetry consumers. Fresh evidence in the current tree is that isAttempt still does not inspect any of the seven required counters; validate each as a finite nonnegative number before accepting the outcome.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7db710b. isAttempt now requires isRerankUsage(value.usage), a new check in outcome.ts that all seven USAGE_KEYS are finite numbers >= 0, and durationMs >= 0. An attempt that fails either check makes the step a broken step, and the chain falls through. The withFallback JSDoc now spells out that rule. Test: src/rerank/outcome.test.ts › 'treats an attempt with a malformed usage counter or duration as a broken step' covers {calls:-3, inputTokens:NaN}, {}, a missing counter and durationMs: -1, and asserts that every attempt passed through has seven finite, non-negative counters. It fails when isAttempt is put back to isDecisionObject(value.usage).
| !Array.isArray(request.documents) || | ||
| typeof request.query !== 'string' || | ||
| !request.query.trim() || | ||
| request.documents.some((document) => typeof document !== 'string') |
There was a problem hiding this comment.
When a caller supplies a sparse array such as new Array<string>(2), Array.prototype.some skips its holes, so this validator accepts the request even though neither entry is a string. The cross-encoder then serializes those holes as null and sends a billable malformed request, while System One can submit states with missing documents instead of returning the promised invalid_request identity outcome; validate every numeric slot rather than relying on some over the sparse input.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7db710b, together with the rest of the hole-skipping pattern. A new everySlot(values, test) in outcome.ts checks every index, holes included. It now backs rankRequestProblem (request documents), the System One rubric config check, the withFallback steps check (a sparse chain used to get past steps.some), and isOutcome's attempts check (attempts.every used to let a hole through into the history). Tests in src/rerank/outcome.test.ts:
- 'sparse document arrays › is invalid_request for every reranker, before any request' covers System One in document and batch mode, the cross-encoder, BM25 and withFallback: every one returns
invalid_requestidentity order with zero usage, nodecidecall and no HTTP request. - 'refuses a sparse rubric at construction'
- 'refuses a sparse chain at construction'
- 'treats a sparse attempts array as a broken step'
Each of these fails when its own check is put back to .some/.every. isRerankPermutation was already safe because it iterates with for-of.
| request.signal | ||
| ); | ||
| try { | ||
| onAttempt?.(reranker, outcome); |
There was a problem hiding this comment.
When a caller supplies an async onAttempt callback and it throws or returns a rejected promise, this try only catches a synchronous exception from invoking the function; the returned promise is discarded and its rejection becomes unhandled, potentially terminating the host process even though bookkeeping is documented not to break ranking. TypeScript permits an async function where a void-returning callback is expected, so attach a rejection handler or await the hook inside the existing error boundary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7db710b for every void hook. A new internal callHook (src/decisions/hook.ts, not exported) drops a synchronous throw and, when the hook returns a thenable, attaches a no-op rejection handler without awaiting it, so this adds no latency. It now wraps onAttempt (fallback.ts), onAnswered in both transport.ts and structuredChat.ts, and score cache set and get (systemone.ts). For get, a promise is a miss. gate was already safe because consult chains it through .then(ok, error). The JSDoc on onAttempt, onAnswered and RerankScoreCache now says the hook is not awaited and a rejection is ignored. Each test listens for unhandledRejection:
outcome.test.ts› 'ignores an async onAttempt that rejects, without an unhandled rejection'outcome.test.ts› 'ignores a score cache whose methods reject, without an unhandled rejection'decisions.test.ts› 'ignores an async onAnswered that rejects, …'structuredOutput.test.ts› 'ignores an async onAnswered that rejects, …' (both Anthropic adapters)hook.test.ts
All of them fail when callHook only catches synchronous throws.
| (tokens * count(price.inputPerMTok)) / 1e6 + | ||
| (output * count(price.outputPerMTok)) / 1e6 + | ||
| units * count(price.perSearchUnit); |
There was a problem hiding this comment.
When an attempt reports more than one billable dimension but the matched price entry omits one of the corresponding optional rates—for example, token and search-unit usage with only inputPerMTok configured—count(undefined) silently prices the missing dimension at zero. Because a price object was found, the model is also omitted from unpriced, so callers receive a deceptively complete but understated cost; treat a used dimension with no explicit finite rate as unpriced rather than free.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7db710b. rerankCost now prices an attempt through attemptCost, and a rate counts only when it is a finite number >= 0. An absent, NaN, negative or non-number rate is missing, and an explicit 0 is free. Each reported dimension the attempt used needs its own rate: inputTokens needs inputPerMTok, outputTokens needs outputPerMTok, and searchUnits needs perSearchUnit. Otherwise the attempt goes in unpriced and adds nothing to usd.
The cross-encoder estimate books estimatedTokens and estimatedSearchUnits together because the SDK cannot tell which one the host bills. So an estimate is priced by whichever of those two rates the entry has, and it is unpriced only when the entry has neither. A timed-out Cohere attempt priced {perSearchUnit} therefore stays priced. The JSDoc on RerankPrice, RerankCost.unpriced and rerankCost and the README cost paragraph now state this rule.
Tests: src/rerank/outcome.test.ts › 'rerankCost rates' (3 tests) covers your two examples plus an empty entry, invalid rates versus an explicit 0, and the estimate pricing. All three fail against the old count(price.*) pricing. The existing table test still passes unchanged.
7db710b to
4f086d1
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f086d1664
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (value.threshold !== undefined || | ||
| value.results.every((entry) => entry.kept === undefined)) && |
There was a problem hiding this comment.
When an injected OutcomeReranker returns a finite threshold, this accepts any boolean kept values without checking that they equal score >= threshold. A step can therefore return { score: 0.9, kept: false } and { score: 0.1, kept: true }, be treated as successful, and cause consumers to retain the wrong documents; require every kept flag to be present and consistent with the declared threshold.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 53c5fd4. isOutcome (src/rerank/fallback.ts) now requires, when a step declares a threshold, that every entry carries kept === (score !== null && score >= threshold), the same rule rankedEntries uses. That covers contradicting, missing, and partial flags together, and a step that breaks it is a failed step (invalid_results). The withFallback JSDoc and the README now say this. Tests in src/rerank/outcome.test.ts: the table 'treats a step returning %s as invalid_results and moves on' gains 'kept flags contradicting their threshold', 'a threshold without kept flags', and 'a threshold with kept on only some entries', and 'passes through kept flags that match their threshold' is the positive case. With the check reverted, the 3 new table cases fail.
| ? outcome.attempts | ||
| : [ownAttempt(outcome)]) | ||
| ); | ||
| addRerankUsage(usage, usageOf(outcome.usage)); |
There was a problem hiding this comment.
When an injected step reports billable usage in its validated attempts but supplies zero, missing, or contradictory top-level usage, this line trusts and sanitizes that second copy instead of the attempts. The returned chain can consequently show a charged request in attempts while its documented aggregate usage reports no spend, breaking request budgets and telemetry; sum the attempts appended above or reject an outcome whose aggregate does not match them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 53c5fd4. withFallback now sums the chain's usage from the attempts it appends: the step's validated attempts, or ownAttempt(outcome) when it reported none. It no longer reads the step's own top-level usage, so usage always matches attempts and rerankCost. All built-in steps already kept the two equal (stepOutcome, failedStep, ownAttempt, nested withFallback), so their results don't change. Test: 'sums the chain usage from the step attempts when its own total is %s' (missing / all zero / contradicting, calls: 7) in src/rerank/outcome.test.ts. All 3 cases fail with the old addRerankUsage(usage, usageOf(outcome.usage)).
| options.modelFor(model).decide({ | ||
| ...probeRequest(), | ||
| timeoutMs: options.timeoutMs ?? 15_000, | ||
| ...(options.signal !== undefined ? { signal: options.signal } : {}), | ||
| }); |
There was a problem hiding this comment.
When modelFor supplies a custom DecisionModel whose decide() never settles and ignores its request signal, probePin() also never settles after either timeoutMs or caller cancellation because those values are only forwarded to the model. This can indefinitely block pin verification on the first sentinel request; wrap each probe call in withDecisionDeadline rather than relying on every injected model to enforce the limit itself.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 53c5fd4, together with the other places that had the same problem. probePin now runs each request through withDecisionDeadline with timeoutMs and the caller's signal. The model is built inside the operation, so a throwing modelFor is still caught. The readiness probe() (src/decisions/probe.ts) had the same unbounded await and is wrapped the same way: it forwards timeoutMs when given and otherwise bounds the wait at the new PROBE_MAX_WAIT_MS (60 s). A grep found one more: toOutcomeReranker awaited an injected Reranker.rerank with no bound, and it is now bounded by deadlineMs/signal whenever either is set. Tests use a never-settling model/reranker: src/rerank/outcome.test.ts 'probePin input and deadlines > a model that ignores its deadline and signal' (timeoutMs, mid-flight abort, pre-aborted) and 'toOutcomeReranker with a reranker that ignores its deadline and signal' (deadline is a charged timeout, mid-flight abort), plus src/decisions/breaker.test.ts 'bounds a model that ignores its deadline and signal' (same three cases for probe). Every one of them is still pending at its window with the wrap reverted.
| dispatch.sent = true; | ||
| run.usage.calls += 1; | ||
| return model.decide({ |
There was a problem hiding this comment.
When document mode uses createDecisionModel with its default maxRetries: 2, one model.decide() invocation can send up to three HTTP requests, but this counter increases only once. If a candidate is then re-issued by the reranker, the outcome can report two calls after as many as six transport attempts, contradicting the RerankUsage.calls contract that includes retries and allowing request budgets to substantially undercount provider traffic; count at the transport-attempt boundary or otherwise expose and reconcile the model's retry count.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 53c5fd4 by counting where each transport attempt goes out. DecisionRequest (and RerankRequest) gain an optional onRequest(). The transport calls it once per HTTP request, just before the fetch, and that includes retries and the re-send after a 401 refresh. createHttpDecisionModel forwards it. In document mode, System One still counts the first request at dispatch, so a custom model that never calls the hook is counted one per decide(). Each later hook call adds a call and marks the request before it as replied, so a retried request is only estimated if a deadline abandons it. Batch mode goes through toOutcomeReranker, which now counts each reported retry too. RerankUsage.calls, the README and the DecisionRequest JSDoc document this. Tests in src/rerank/outcome.test.ts 'System One usage counts transport retries', all using a real HTTP stub and maxRetries: 2: document mode 503,503,ok → calls 6 for 2 docs; a 401 refresh re-send → calls 2; 503 then a hang under the deadline → calls 2 with estimatedTokens 600 (only the abandoned one); batch 503,ok → calls 2. With the transport's hook call removed, all 4 fail.
| const sentinel = options.sentinel ?? PIN_SENTINEL_MODEL; | ||
| if ( | ||
| pin === '' || | ||
| sentinel === pin || | ||
| typeof options.modelFor !== 'function' |
There was a problem hiding this comment.
When a configured sentinel is empty or whitespace, it passes this validation even though it is not the deliberately nonexistent model ID that the probe relies on. If the host returns 404 for a missing or blank model but aliases every syntactically valid unknown model to a default checkpoint, the sentinel request passes and the subsequent echoed pin falsely marks that host as verified, potentially enabling thresholds calibrated for a model it does not serve; trim the sentinel and reject it when empty before probing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 53c5fd4. probePin now uses PIN_SENTINEL_MODEL only when sentinel is undefined. Otherwise the sentinel must be a string, it is trimmed, and if it is empty or equals the trimmed pin the result is invalid_request with no request sent. The trimmed sentinel is what gets sent. Tests in src/rerank/outcome.test.ts 'probePin input and deadlines' use a routing front that 404s a missing or blank model in front of an aliasing backend. 'refuses a blank sentinel "" / " "' and 'refuses a sentinel that is the pin once both are trimmed' each expect invalid_request with zero calls. 'still catches an aliasing host with a custom sentinel' expects the trimmed custom sentinel to be sent. With the old options.sentinel ?? PIN_SENTINEL_MODEL all 4 fail, and the blank ones return verified: true.
4411e46 to
addba2d
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: addba2d954
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const stateFor = (index: number): DecisionState => | ||
| (states[index] ??= stateOf(query, documents[index])); |
There was a problem hiding this comment.
When a custom state returns a caller-owned object and caching is enabled, stateFor retains that mutable reference while keyFor serializes its current value. The model receives the reference later in a microtask, so a caller that mutates it immediately after rank() returns can have state B scored and stored under state A's cache key; a later state-A request then reuses the wrong score. Snapshot the state when it is first built and use that snapshot for both serialization and dispatch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 100c354 (8a7a80f rebased onto #625's 3979477). When a cache is configured, keyFor now replaces states[index] with JSON.parse of the exact string it put into the key, and dispatch sends that snapshot. Mutating the caller's object after rank() therefore changes neither the key nor the scored payload. A state that cannot be serialized stays uncached and is sent as is, the same as before. This was the last caller-owned value read twice on this path: instructions is snapshotted at construction, rubric and documents are copied, pin is rebuilt, and query is a string. Test: src/rerank/outcome.test.ts 'snapshots a custom state with its cache key, so a caller mutating it after rank never caches one state under another'. It flips the shared state to B right after rank() and asserts that the model saw A and scored 2, and that a later A request is a cache hit worth 2. Removing the two-line snapshot makes it fail (the model sees B).
| STATUSES.has(value.status) && | ||
| isRerankKind(value.provider) && | ||
| typeof value.model === 'string' && | ||
| isReason(value.reason) && |
There was a problem hiding this comment.
When an injected or nested reranker returns a fully scored status: 'degraded' outcome with reason: null, this predicate accepts it. If it is the first chain step, withFallback then returns status: 'degraded', reason: null, violating the public outcome contract and leaving callers unable to identify the failure that caused degradation; require a non-null reason for degraded outcomes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 100c354 (8a7a80f rebased onto #625's 3979477), across the whole predicate. isOutcome and isAttempt now share reasonFits: reason must be null exactly when the status is ok and a non-empty string otherwise. A breach (degraded/null, degraded/'', skipped/null or '', ok with a reason, failed/null, skipped attempt with '') becomes a failed invalid_results step. Related changes: ownAttempt gives an answering (degraded) step's synthesized ok attempt a null reason; the answer path backstops with 'error' instead of null; stepOutcome maps an empty reason to 'error'; and noneReranker({ reason: '' }) reports disabled. Tests: seven new rows in 'treats a step returning %s as invalid_results and moves on', plus 'never answers degraded without a reason, and an empty none reason is disabled', which covers step 0 alone, nested withFallback, the ownAttempt reason and noneReranker(''). With the fix reverted, 9 of them fail. I did not add a status-vs-attempts consistency rule (a skipped outcome carrying an ok attempt). A kind: 'none' step can legitimately report history from inside a nested chain, and that rule is a separate contract decision from this finding.
| const requiresAuth = options.requiresAuth !== false; | ||
| if ( | ||
| model === '' || | ||
| validDeadline(timeoutMs) === undefined || |
There was a problem hiding this comment.
When options.timeoutMs is positive but fractional, such as 0.5, validDeadline rounds it up and this constructor accepts it, but the original fractional value is retained and passed into the transport. withDecisionDeadline then rejects it for not being a safe integer, so every otherwise valid cross-encoder ranking returns an error without sending a request; System One batch mode has the same validator/raw-value pattern. Either reject fractional configured timeouts or retain the normalized value returned by validDeadline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 100c354 (8a7a80f rebased onto #625's 3979477). createCrossEncoderReranker and createSystemOneReranker (both modes) now also require Number.isSafeInteger(timeoutMs), matching probePin and withFallback step deadlines, so a fractional configured timeout throws invalid reranker configuration at construction instead of failing every call in withDecisionDeadline. I swept every other validDeadline caller: each one either uses the rounded value or already checks for an integer. I corrected validDeadline's JSDoc (it rounds fractions up) and the README and option JSDoc. Tests: 'rejects a fractional timeout at construction instead of failing every ranking' (cross-encoder, 0.5 and (0.1 + 0.2) * 1000, plus an integer control that ranks ok with one request), and new rows in System One's 'rejects a malformed timeout...' (document and batch). Both fail without the two added checks.
| if ( | ||
| request.deadlineMs !== undefined && | ||
| validDeadline(request.deadlineMs) === undefined | ||
| ) { | ||
| return 'invalid_request'; |
There was a problem hiding this comment.
When a JavaScript caller supplies an otherwise valid request with a malformed signal, this validator accepts it, after which runStep calls AbortSignal.any or signal.addEventListener. Those operations reject or throw for a non-AbortSignal value, and withFallback.rank() has no enclosing catch, so the supposedly never-rejecting API rejects instead of returning an invalid_request identity outcome. Validate the optional signal before the fallback chain uses it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 100c354 (8a7a80f rebased onto #625's 3979477). rankRequestProblem now requires signal to be undefined or a real AbortSignal (the new isRequestSignal; a duck-typed object is refused too, because AbortSignal.any and fetch refuse it). Every entry point that calls it (withFallback, System One in both modes and its rejecting rerank, the cross-encoder, BM25, toOutcomeReranker) now answers invalid_request with no request, instead of rejecting (withFallback) or reporting error. probePin applies the same check. withFallback.rank also wraps the chain in a last-resort catch that returns identity order with the first reason (or error) and the attempts and usage so far, so 'never throws' no longer depends on every helper being total. Tests, in a new 'malformed request signals' describe: {}, null, a string and a duck-typed signal through withFallback (no deadline, request deadline, step deadline, gate) and through every reranker and probePin, plus a real-signal control and a getter that throws mid-chain to exercise the catch. Removing the validator fails 8 of them, and removing the catch fails the getter test.
8a7a80f to
100c354
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 100c354e8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| usageOf(reported) | ||
| ); | ||
| }, | ||
| () => failedStep(reranker, n, 'error', elapsedMs(startedAt)) |
There was a problem hiding this comment.
When an injected step honors its signal by rejecting after a caller abort or step deadline, this rejection handler usually wins the race before the 50 ms abandonment timer and hard-codes the failure as error. asTimeout() only rewrites an aborted reason, so deadline fallback is reported as degraded because of error rather than timeout, and caller cancellation is likewise mislabeled. Derive the rejection reason from the outer and deadline signals before constructing the failed step.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3fb89be. In runStep (src/rerank/fallback.ts), a step that rejects, or resolves nothing, is now named from the step's combined signal: aborted if that signal fired, otherwise error. The existing asTimeout then reports a step deadline as timeout, and a caller abort stays aborted. A malformed but non-null answer is still invalid_results. Proving tests are in outcome.test.ts under withFallback steps that reject once stopped. A step that rejects, or resolves undefined, at its 30 ms deadline is degraded/timeout. On the caller's abort it is skipped/aborted, and a plain rejection is still error. With the fix reverted, all 4 stopped-step cases fail.
| async rank(request: RankRequest): Promise<RerankOutcome> { | ||
| return stepOutcome({ | ||
| kind: 'none', | ||
| documents: documentCount(request), | ||
| startedAt: performance.now(), | ||
| model: '', | ||
| reason, |
There was a problem hiding this comment.
When noneReranker is used directly with a blank query, sparse/non-string documents, or an invalid deadline or signal, it bypasses the shared request validation and returns its configured reason (usually disabled) instead of the public invalid_request outcome. This makes malformed calls indistinguishable from an intentionally disabled reranker and breaks the request invariant implemented by every other outcome reranker; run this path through the shared request validator before applying the configured fallback reason.
AGENTS.md reference: AGENTS.md:L32-L36
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3fb89be. noneReranker().rank now reports rankRequestProblem(request) ?? reason. A blank query, sparse or non-string documents, a bad deadlineMs, a duck-typed signal, or a null request is invalid_request, and no documents is empty. The configured reason (disabled by default) is used only for a valid request. Proving tests are in outcome.test.ts: noneReranker requests covers each malformed shape plus the empty case and the own-reason case. A direct noneReranker() is also now in both "every reranker" tests (sparse documents and malformed signals). With the fix reverted, 13 tests fail. The same commit removes the outdated README example that said noneReranker reports no attempt.
| return reranker.rerank({ | ||
| query: request.query, | ||
| documents: request.documents, | ||
| ...(signal !== undefined ? { signal } : {}), | ||
| ...(deadlineMs !== undefined ? { timeoutMs: deadlineMs } : {}), |
There was a problem hiding this comment.
When toOutcomeReranker wraps a reranker whose configured timeout is shorter than RankRequest.deadlineMs, forwarding the deadline as timeoutMs replaces and extends that configured timeout. For example, a reranker configured to give up after 200 ms receives a 1,500 ms override and can delay the call or a fallback chain for the full 1,500 ms, contrary to the RankRequest contract that deadlines only cap a reranker's own timeout. Enforce the deadline externally without forwarding it as an override.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3fb89be. toOutcomeReranker no longer forwards RankRequest.deadlineMs as the wrapped reranker's timeoutMs, which would replace its configured timeout. The deadline now reaches the reranker only as the abort signal from withDecisionDeadline, and the same race still bounds the wait. A reranker configured shorter therefore gives up at its own timeout, and a longer one is cut off at the deadline as timeout. Batch System One's .rank already passes Math.min(deadline, timeoutMs), so it keeps its cap. I also updated the JSDoc, the README and the PR body. Proving tests are in outcome.test.ts under toOutcomeReranker deadline. The wrapped reranker gives up at req.timeoutMs ?? 200 against a host that answers in 1 s. With deadlineMs: 1500, it is skipped/timeout in under 800 ms and sees timeoutMs undefined, and withFallback([wrapped, bm25]) degrades to bm25 in under 800 ms. With the forwarding restored, both tests fail.
3fb89be to
98f22ed
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98f22eddfd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| run.replied += 1; | ||
| dispatch.onWire = false; | ||
| dispatch.answered = status !== null && status >= 200 && status < 300; |
There was a problem hiding this comment.
When one document receives a 2xx status but its response body stream fails, the transport reports that status and then retries the request as a network failure. Marking the dispatch as answered here means that if another concurrent document subsequently stops the step with a 429, quota error, or pin mismatch while the first request is backing off, stopStep will not abort that dispatch and it can retry after the step has globally stopped. Only mark the dispatch answered after model.decide() resolves with a parsed answer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 80df154. The premise held, and the root cause was in the transport: readResponse ran in a try/finally that reported response.status even when the body stream threw (for example undici's terminated). The failure was then retried as network. A probe against a real server showed two extra requests reaching a quota-stopped host, whichever of the two orderings happened.
What changed:
src/decisions/transport.ts: a 2xx whose body breaks off or overflows is now reported toonResponseasnull. The status is reported only after the body is read in full, as the JSDoc already said.src/rerank/systemone.ts: added a defence for any other path that retries after a 2xx. WhenonRequeststarts a retry after the step has stopped, it now halts the dispatch before the request leaves, and the request is not counted.
I did not take the suggested fix of marking the dispatch answered only after decide() resolves. The dispatch leaves dispatches right after that, so in effect the exemption would be gone. A fully read, paid 2xx could then be halted while it was still being parsed.
Proving tests:
outcome.test.ts, "halts a document whose 2xx body broke off when another document stops the step after it / before it": the server sees only['alpha','beta']withcalls: 2. Both cases fail with the previoustransport.ts.outcome.test.ts, "does not send a retry the model starts after a 2xx once the step has stopped": fails with the previoussystemone.ts.decisions.test.ts, "reports a 2xx whose body broke off as no reply": expects[null, 200]. With the previoustransport.tsit receives[200, 200].
| }, | ||
| (status, retryAfterMs) => { | ||
| tally.ended += 1; | ||
| tally.backoff ??= retriedReply(id, status, retryAfterMs); |
There was a problem hiding this comment.
When retries receive multiple retryable replies, this retains the first one forever. For example, with retries enabled, a 500 followed by a 429 whose Retry-After extends past the deadline leaves no request in flight, but the catch path classifies the timeout from the stale 500 and reports server_error instead of rate_limited. This can drive fallback and backoff bookkeeping from the wrong provider condition; replace or clear the tracked reply on each response so it represents the backoff that was actually interrupted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 80df154. One small correction to the finding: the stale 500 is reported as error, not server_error, because REASON_FOR_FAILURE has no server_error entry. It should have been rate_limited.
The same first-reply-wins ??= pattern was in three places, and all three are fixed:
src/rerank/crossEncoder.ts: now keeps the latest reply (tally.backoff = retriedReply(...)). A later 401, a dropped connection or a 200 clears it.src/rerank/outcome.ts: same change intoOutcomeReranker.src/rerank/systemone.ts: documents run concurrently there, so one slot for the whole step would be wrong whichever reply it kept. With last-wins, a score on another document would hide a live backoff. It is nowrun.backoffs: Map<Dispatch, DecisionError>. Each document's entry is replaced when that document's request ends, and removed on success or on a failure while the step is still live. A failure caused by the step's deadline leaves its entry in place.
The JSDoc and READMEs now say "after the latest request ended with a 429 or 5xx, before its retry is sent".
Proving tests, all in outcome.test.ts under "a deadline reports the backoff it interrupted, not an earlier one":
- Cross-encoder, a 500 then a 429 past the deadline: reports
rate_limited. - Cross-encoder, a 500 then a 401 whose refresh hangs: reports
timeout. toOutcomeReranker, a 500 then a 429: reportsrate_limited.- System One, a document whose 500 ended in a score: a later deadline reports
timeout.
All four fail with the previous ??= code. A fifth test guards the per-document design: a score on another document does not hide a live 503 backoff.
80df154 to
ebce1fd
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebce1fdf7c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (value.status === 'skipped' || | ||
| value.results.every((entry) => entry.score !== null)) && |
There was a problem hiding this comment.
When an injected reranker returns status: 'skipped' with a sorted permutation containing scores, this branch accepts it even though the public outcome contract guarantees that skipped results are identity-ordered with every score null. withFallback consequently exposes the malformed outcome to onAttempt and preserves its attempts and usage instead of recording invalid_results, so bookkeeping can act on scores from a step that claims not to have answered. Require every skipped result to have a null score; ranked-order validation will then also enforce identity order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 784e2dc. isOutcome now checks the score rule both ways: an answer scores every document and a skipped outcome scores none, so isRankedOrder also forces identity order on a skipped step. The same commit closes the matching attempt gap: a skipped outcome may not carry an ok attempt, and an answer that reports attempts must include one. Such a step becomes failed/invalid_results, so onAttempt sees identity order and the history records the failure. Proving tests are three new rows in the treats a step returning %s as invalid_results and moves on table in src/rerank/outcome.test.ts: "a skipped outcome with scores", "a skipped outcome with an ok attempt" and "an answer with no ok attempt". With the previous fallback.ts all three fail; with the fix they pass. First-party producers (identityOutcome, stepOutcome, nested withFallback) already met both rules.
| return { | ||
| verified: false, | ||
| reason: 'sentinel_accepted', | ||
| ...(echoed !== undefined ? { echoed } : {}), | ||
| sentinelStatus: 200, |
There was a problem hiding this comment.
When the sentinel endpoint accepts the nonexistent model with another successful status, such as HTTP 201 and a valid decision body, the shared transport treats it as successful and ask() resolves, but this branch reports sentinelStatus: 200. The field is documented as the status the sentinel request actually received, so diagnostics become incorrect for any non-200 2xx response—and for injected non-HTTP models there is no observed status at all. Omit the field when the status is unavailable or propagate the actual successful response status.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 784e2dc. probePin no longer hard-codes 200. The sentinel request now passes onResponse to decide() and reports the last HTTP status the model gave there. The SDK HTTP model reports the real success status (transport.ts ended(response.status)). A model that reports no status, such as an injected non-HTTP one, gets no sentinelStatus. Proving test: reports the success status the sentinel really got, and none from a model that reports no status in src/rerank/outcome.test.ts, covering a real 201 host and an injected model. With the previous pin.ts it fails (sentinelStatus received 200, expected 201). The existing 200 test still passes, now with an observed 200. The README and the PinProbe JSDoc are updated to match.
| const problem = rankRequestProblem(request); | ||
| if (problem !== undefined) { | ||
| return stepOutcome({ ...base, reason: problem, called: false }); | ||
| } | ||
| if (documents > maxDocuments) { |
There was a problem hiding this comment.
When documents.length exceeds maxDocuments, this calls rankRequestProblem first, and that validator walks every array slot before the cheap size check runs. A caller can therefore make the synchronous rank() path scan millions of candidates even though the default cap is 1,000 and the request will inevitably be rejected; System One document mode has the same ordering. Check the array shape and configured cap before the per-document validation so the cap actually bounds rejection work.
AGENTS.md reference: AGENTS.md:L39-L42
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this; checked against 784e2dc. Moving the cap check ahead of the slot scan would not make the cap bound rejection work, for these reasons:
- The rejection is O(documents) by contract. Both call sites return
stepOutcome({ documents: n, reason: 'too_many_documents' }):crossEncoder.ts(the branch right afterrankRequestProblem) and System Onerefuse. For a non-ok step,stepOutcomebuildsidentityOutcome(step.documents).results(outcome.ts), which allocates one{ index, score: null }per document, because askippedoutcome must be a full identity permutation the caller can serve. A cap checked earlier still pays for this. - Measured at ebce1fd (throwaway tsx probe on a real
Array(n).fill('doc')):- n=1e6: the
rankRequestProblemscan took 1.3 ms, the identity results 76 ms and the wholerank()rejection 40 ms. - n=1e7: the scan took 6.9 ms, the identity results 411 ms and the whole rejection 525 ms.
- The scan is about 1-2% of the rejection, and cheaper than the caller building the array (1.8 ms and 17.7 ms).
- n=1e6: the
- Sparse arrays cannot stretch the scan.
everySlotstops at the first hole: a 1e8-length array with one filled slot isinvalid_requestin 0.1 ms. A long scan needs a caller that has already filled every slot with a string, which is O(n) on its side. - The composed path scans regardless.
withFallback.rankrunsrankRequestProblemover every slot before any step is dispatched (fallback.ts,if (rankRequestProblem(request) === 'invalid_request')). Reordering inside the cross-encoder or System One would not remove that scan. - The reorder would change reported reasons. An oversized request that is also malformed would report
too_many_documentsinstead ofinvalid_request, and System One's rejectingrerank()would throwunsupported_questioninstead ofbad_request.
Truly bounding rejection cost would mean rejected outcomes stop listing every document. That is a change to the identity-results contract, which is out of scope for this PR.
784e2dc to
979cce7
Compare
e79820a to
93933aa
Compare
…One, Cross-Encoder, BM25, and Fallback Chains System One reranking batched every candidate into one request, and its scores followed input order: in Codegraph's evaluation (n=329) the same batch reached hit@1 22.8% in embedding order and 5.8% with the candidates reversed. createSystemOneReranker now asks one score question per candidate, one request each, by default (mode: 'document'); mode: 'batch' keeps the earlier request. Document mode runs requests under a semaphore, re-issues unscored candidates once inside the deadline, stops on unauthorized, quota, an open breaker, or a pin mismatch, can pin a checkpoint whose echo every response must match, sets kept only with a threshold calibrated for that pin, and can reuse scores from a cache keyed by document text. A never-throwing outcome layer sits beside the rejecting Reranker contract, which createWebSearchReranker keeps using unchanged: RerankOutcome is a full permutation with null scores for unscored documents, status ok, degraded, or skipped, the first failed step's reason, every attempt, and usage that charges requests a deadline abandoned at estimatedUsage. On top of it: createCrossEncoderReranker (Cohere-shaped POST through the decision transport), createBm25Reranker with corpusStats and Codegraph's code tokenizer, noneReranker, withFallback with per-step deadlines, a late-timer grace, and permutation validation, probePin (a sentinel model must 404 and the pin must echo), and rerankCost over a caller-supplied price table. The SDK ships no prices. Everything stays on the light ./rerank subpath.
…Order on Invalid Requests withFallback now answers a blank query or invalid deadline with identity order over every document, and a request's deadlineMs bounds the whole chain: each step gets the lesser of its own deadline and what is left, and the chain stops with timeout once nothing is left. System One document mode records a request the model timed out on its own clock as timeout (charged as abandoned) instead of falling through to bad_response, and batch mode reports an oversized candidate set as too_many_documents like document mode and the cross-encoder. createBm25Reranker accepts any ReadonlyMap of idf, including a Map from another realm, and the cross-encoder docs drop TEI (different request and response shape) and note the 256 KiB response cap for hosts that echo documents.
…comes, Cache Keys, and Charged Usage - withFallback races an async gate against the chain deadline and the caller's abort, and re-checks both before a step starts. - withFallback validates the whole RerankOutcome contract of a step (score order, ties, null placement, kept, attempt shape, provider, reason, threshold, pinVerified) and constructs only with known reranker kinds. - noneReranker reports one skipped attempt; a step that ran but reported no attempt is recorded as one. - System One cache keys carry a custom state; out-of-rubric cached values are misses; createScoreCache rejects non-finite or non-positive bounds. - An open breaker's pre-dispatch refusal is not counted as a call or estimated, and a step that sent nothing is a skipped attempt (document mode, batch mode, and the cross-encoder). - estimatedUsage charges cross-encoder search units from the document count even when the token estimate is zero.
…es From the Call, and Reject Malformed Step Outcomes and Estimates - withFallback: a caller abort now ends the wait for a step that ignores its signal, with or without a deadline, as `aborted` after STEP_GRACE_MS (charged as abandoned); a cooperative step still reports its own spend. - withFallback: a step answering `ok`/`degraded` must score every document, and `kept` requires a `threshold`; otherwise it is invalid_results and the chain falls through. - System One (document mode): the deadline counts from the call, so state building and cache lookups spend it; nothing is sent after it, and a ranking the cache answers in full is still served. Structured instructions are snapshotted so the question and its cache key never drift. timeoutMs, concurrency, and estimatedTokensPerDocument are validated at construction; the estimate is document-mode only. - Cross-encoder: estimatedTokensPerDocument must be a finite count >= 0. - BM25: an already-aborted request is `aborted` before tokenizing.
…lot and Attempt Counter, Contain Async Hooks, and Leave Rateless Usage Unpriced - System One (document mode): a call is counted only when `model.decide` is invoked, so a throwing custom `state` or a spent deadline sends and charges nothing and is neither a call nor a reply. - Sparse arrays: `everySlot` checks every index, holes included, for request documents, the System One rubric, withFallback steps, and a step outcome's attempts; a sparse request is `invalid_request` before any request in every reranker. - withFallback: an attempt's usage must carry all seven counters as finite counts of at least zero and its duration must be at least zero, or the step is a broken step. - Hooks: `callHook` drops a synchronous throw and handles a returned rejection without awaiting it, for `onAttempt`, both `onAnswered` hooks, and score cache `get`/`set`, so an async hook cannot cause an unhandled rejection. - rerankCost: each reported dimension needs its own finite, non-negative rate (`0` is free); an abandoned request's estimate is priced by the token or search-unit rate the entry has and unpriced only with neither.
…k Kept Flags, Sum Chain Usage From Attempts, and Reject Blank Pin Sentinels - withFallback: a step with a threshold must set kept on every entry, true exactly where the score reaches it; the chain's usage is summed from the validated attempts, never the step's own total. - probePin, probe, and toOutcomeReranker bound the injected call with withDecisionDeadline, so a model or reranker that ignores its deadline and signal cannot hold them. - probePin: a blank sentinel, or one equal to the pin once trimmed, is invalid_request with no request sent. - DecisionRequest/RerankRequest gain onRequest, called by the transport once per HTTP request (retries and the 401 re-send included); System One in both modes counts those retries as calls.
The restack onto the credentials breaker adds a recheck after a second 401, which can send a third key. onRequest fires before every fetch, so the recheck's re-send is counted; this pins it (three requests, calls: 3) and the onRequest docs now say every re-send after a 401.
…by the Request Deadline, and Estimate Only In-Flight Requests - A request's deadlineMs now caps a reranker's own timeout instead of replacing it (System One document and batch modes, cross-encoder), so a short-timeout step under a long chain deadline still falls back in time. - DecisionModel.reportsRequests and DecisionRequest.onResponse: the HTTP model reports every request it sends and every one that ends; withBreaker keeps the flag. System One and toOutcomeReranker count only reported requests for such a model, so a minter that fails, returns nothing, or hangs, an unserializable state, or an abort before dispatch counts no call and leaves a skipped attempt. - Estimates charge only requests still in flight; a deadline during a 429/5xx backoff charges nothing and reports the reply (rate_limited). - rate_limited stops a System One step: nothing is re-issued into a 429. - BM25 skips a query with no terms as unsupported instead of ranking all zeros; the ASCII-only tokenizer is documented. - probePin rejects an invalid timeoutMs as invalid_request. - withFallback hands onAttempt a copy of the outcome. - Test racers clear their timers, so the jest run exits without a forced worker exit.
…s First Error, and Report Only Real Sentinel Statuses For a decision model that reports its requests, a 429 now stops a document-mode step on the reply instead of after the transport's retries, and a document between requests (minting, backing off, refreshing after a 401) is halted once the step has stopped, so nothing is retried or re-sent into a throttled host and the chain falls back without waiting out a Retry-After. onResponse carries a reply's Retry-After, so rerank() rejects with the 429's retryAfterMs. The first failure that stops the step keeps its own error, so rerank() never rejects with a later one. probePin omits sentinelStatus when an open breaker refused the sentinel without a request.
…s, Reject Fractional Timeouts, and Validate Request Signals - System One document mode sends the JSON snapshot its cache key was built from, so a caller mutating a custom state after `rank` cannot have one state scored and cached under another's key. - withFallback's step guard requires `reason` to be null exactly when an outcome's or attempt's status is `ok` and a non-empty string otherwise; a step-0 `degraded` outcome without a reason no longer leaks through. An answering step's own attempt carries no reason, and an empty `noneReranker` or step reason falls back to `disabled` / `error`. - The cross-encoder and System One constructors reject a fractional `timeoutMs` instead of accepting it and failing every call in the deadline wrapper; validDeadline's JSDoc says it rounds up. - `rankRequestProblem` and `probePin` treat a `signal` that is not a real AbortSignal as `invalid_request`, and `withFallback.rank` has a last-resort catch so it resolves even if a helper throws.
…ranker Requests, and Cap a Wrapped Reranker's Timeout Instead of Overriding It - withFallback: a step that rejects or resolves nothing after its deadline or the caller's abort fired is recorded as timeout/aborted, not error. - noneReranker runs the shared request validator, so a malformed call is invalid_request and an empty one empty, not its configured reason. - toOutcomeReranker no longer forwards RankRequest.deadlineMs as the wrapped reranker's timeoutMs (which replaces its configured timeout); the deadline reaches it as its abort signal and bounds the wait, so it caps and never extends.
…er a Stop, and Track Only the Latest Backoff A 2xx whose body broke off was reported to onResponse with its status, so System One marked the dispatch answered and stopStep left its retry alone: a stopped step kept sending into the host. The transport now reports such a request as null, and System One halts any retry that starts after a stop. The deadline's backoff reclassification kept the first retried reply forever. The cross-encoder and toOutcomeReranker now keep only the latest ended request's reply, and System One keeps one per document, dropped when that document's next request ends or it fails while the step is live.
…nswering Attempt, and Report Only a Sentinel Status the Model Observed
979cce7 to
e5a5338
Compare
What this unblocks
codegraph is a separate code-intelligence service. It is moving its reranking off its own copy and onto this SDK's light
./reranksubpath (#623). Its copy ranks differently from the SDK and has rerankers the SDK lacks, so it can't switch until the SDK matches it. LibreChat backends get the same rerankers through@librechat/agents/rerankwithout loading LangChain.What is wrong today
createSystemOneRerankersends every candidate in one request. In codegraph's evaluation (paraphrased developer questions, n=329), the same batch reached hit@1 22.8% with candidates in embedding order and 5.8% with the same candidates reversed. Scoring each candidate on its own (jev-1.13.0) reached hit@1 23.4–23.7% over two runs, against 7.6% for the embedding order alone./rerank), no BM25, and no way to chain them so that a slow or refused step falls back to the next one.Reranker.rerankrejects on failure. A search path then has to wrap each call, rebuild the input order, and guess what a deadline-abandoned request cost.What changes
A never-throwing outcome layer (
src/rerank/types.ts,outcome.ts).OutcomeReranker.rank({ query, documents, deadlineMs?, signal? })always resolves to aRerankOutcome:resultsis always a full permutation of the input. It is ordered by score, withnullscores last. Askippedoutcome is identity order, so a caller can always serveresults.map(r => documents[r.index]).statusisok,degradedorskipped.reasonisnullexactly whenok; otherwise it is the first failed step's reason, never empty.attemptslists every step that ran or was skipped.usageis summed over attempts.callscounts requests actually sent: the SDK's HTTP decision model now declaresreportsRequestsand reports each request's start (onRequest) and end (onResponse(status)), andwithBreakerkeeps the flag, so adecide()that failed before sending (a token minter that failed, returned nothing or hung, a state that cannot be serialized, an abort before dispatch, an open breaker) counts no call and leaves askippedattempt. A model that does not report counts one call perdecide().estimatedUsage(kind, documents), because the host still bills it. A deadline that lands after a request ended with a 429 or 5xx reply, before its retry is sent, charges nothing and reports that reply (rate_limitedfor a 429) instead oftimeout. Only the latest reply counts (a later 401, dropped connection or score ends the backoff), and System One's document mode keeps one per document, so another document's score does not hide it. A 2xx whose body breaks off is reported toonResponseasnull, not as an answered reply.deadlineMs, or asignalthat is not a realAbortSignalisinvalid_requestin identity order, with no request sent.RankRequest.deadlineMscaps a reranker's owntimeoutMsand never extends it, so a 200 ms System One step under a 1.5 s chain deadline still falls back after 200 ms. The rejectingrerank({ timeoutMs })keeps its override;toOutcomeRerankerhands the deadline to the wrapped reranker only as its abort signal, never astimeoutMs, so it caps that reranker's configured timeout too.rerankCost(outcome, prices)takes a price table the caller supplies. The SDK ships no prices.The #588
Rerankercontract andcreateWebSearchRerankerare unchanged.toOutcomeRerankerwraps any rejectingReranker.System One per-document mode (
systemone.ts), now the default (mode: 'document'):concurrency, default 64, or a shared one);unauthorized,quota,rate_limited, an open breaker (🔌 feat: Add Decision Credentials, a Breaker, and a Readiness Probe #625) or a pin mismatch stops the step: the remaining candidates are not sent and nothing is re-issued. Over a model that reports its requests (the SDK's HTTP model), a 429 stops the step on the reply instead of after the transport'smaxRetries, and a candidate between requests (minting a token, backing off from a 5xx or from a 2xx whose body broke off, refreshing after a 401) is halted, as is any retry that starts after the stop, so nothing is retried or re-sent into a throttled host and the chain falls back without waiting out aRetry-After; a request already on the wire may finish;rerank()rejects with the error of the failure that stopped the step first (a 429 with itsretryAfterMs), never a later one;pinrequires every response to echo the pinned model, andkeptis set only when a threshold calibrated for that pin is supplied;cache(createScoreCache()or a host store) is keyed by query, document text, model, rubric and, when a customstatebuilds the request, that state; the request then sends a JSON snapshot of the same state, so mutating the returned object afterrankcannot score one state under another's key. A cached value outside the rubric is a miss.mode: 'batch'keeps the earlier single request. An oversized candidate set istoo_many_documentsin both modes. A configuredtimeoutMsmust be whole milliseconds up to an hour (System One and the cross-encoder throw at construction otherwise).More rerankers:
createCrossEncoderReranker({ url, model, apiKey? })sendsPOST { model, query, documents }and readsresults[{ index, relevance_score }]back. It uses the decision transport, so a 401 re-mints the token once and a 402 isquota. Cohere, Jina, Infinity and the ClickHouse gateway (POST {GATEWAY_URL}/rerank, modelsqwen3-reranker-8bandcohere-rerank-v3-5) speak this shape. TEI does not.createBm25Reranker({ idf, avgdl, tokenize? })withcorpusStats(documents)and a code-awaretokenizeCode. It takes anyReadonlyMapof idf.tokenizeCodereads ASCII only; a query it leaves no terms in (CJK, only stop words) isskippedasunsupportedrather than an all-zero ranking, so a chain moves on.noneReranker()returns identity order without a request, recorded as oneskippedattempt. A malformed request is stillinvalid_requestand no documentsempty, as for every other reranker.withFallback(steps, { gate, onAttempt })(fallback.ts):ok, a later step isdegraded, and none answering isskipped.deadlineMsbounds the whole chain. Each step gets the lesser of its own deadline and what is left, and the chain stops withtimeoutonce nothing is left.RerankOutcomecontract (a full permutation in score order, every document scored by an answer and none by askippedoutcome, attempts that agree with the status (nookattempt in askippedoutcome, at least one in an answer that reports attempts), well-formed attempts and fields, and areasonthat isnullexactly when the outcome's or attempt's status isokand non-empty otherwise) counts as a failed step. A step that rejects or resolves nothing after its deadline or the caller's abort fired is recorded astimeoutoraborted, noterror. A step that reported no attempt is recorded as one.signal, is identity order over every document. A last-resort catch keepsrankfrom rejecting even if a helper throws.onAttemptgets a copy of each step's outcome, so a hook that sorts or trims it cannot change the chain's validated result.Decision API additions (
src/decisions, on top of #625): optionalDecisionModel.reportsRequests,DecisionRequest.onResponse(status, retryAfterMs?)(a reply'sRetry-Afterwhen it was not OK and sent one), a sixthonResponseargument onTransport, andRerankRequest.onResponse/Reranker.reportsRequests. All are optional; existing callers and models are unaffected.probePin({ modelFor, pin, sentinel? })(pin.ts). It checks that a host really serves a checkpoint. A sentinel model must be refused with a 404, and the pin must be echoed. An invalidtimeoutMsor asignalthat is not a realAbortSignalisinvalid_requestwithout a request.sentinelStatusis only a status the sentinel request got, so an open breaker (circuit_open) reports none, and an accepted sentinel reports the success status the model passed toonResponse(a 201 is 201), or none from a model that reports no status. It never throws.Related: #66 adds a configurable
customweb-search reranker type with its own URL and model. The cross-encoder here belongs to the index-based outcome layer and is not a web-search reranker type. The web-search Jina and Cohere rerankers stay as they are, so the two changes do not overlap.Measured
Loaded from the built package in a fresh Node process, through its own exports map, in ESM and CJS:
./rerank./decisionsFor comparison, importing
rerank/systemonethrough the root barrel pulls about 289 modules.How it was tested
NODE_OPTIONS=--experimental-vm-modules npx jest src/rerank src/decisions src/tools/search: 27 suites, 628 tests pass in each of three runs, with no "worker process has failed to exit gracefully" warning. Before this change the warning appeared in 5 of 12 runs on the same base: race helpers inoutcome.test.tsandbreaker.test.tsleft 1-second timers running after the race was won; they now clear them.src/rerankalone: 3 suites, 181 tests.npx jest src/tools/search: 16 suites, 267 tests pass. The web-search reranker path is unchanged.outcome.test.ts: a request deadline caps a step's own timeout (document, batch, cross-encoder); an HTTP model whose minter returns nothing or hangs, an unserializable state, and an abort before dispatch send, count and charge nothing (document and batch modes, directly and throughwithBreaker); a 429 withRetry-Afteror a 500 under a deadline charges no estimate and reportsrate_limited/error(cross-encoder, document and batch); an always-429 host gets one request per document; a BM25 query with no terms isskippedand the chain continues;probePinrejects invalidtimeoutMs;onAttemptmutations do not reach the result. Each fix was removed in turn and its tests failed, then passed again with the fix restored.outcome.test.ts:toOutcomeReranker deadline(a wrapped reranker configured for 200 ms times out at 200 ms under a 1.5 s deadline, and a chain falls back to BM25),noneReranker requests(a directnoneReranker()answers each malformed request asinvalid_requestand no documents asempty; it is also in both "every reranker" tests), andwithFallback steps that reject once stopped(a step that rejects or resolves nothing at its deadline istimeout, on the caller's abortaborted, on its ownerror). With each fix removed in turn, its tests fail.outcome.test.ts(a stopped System One step sends nothing more), over a model withmaxRetries: 2: an always-429 host gets exactly one request per document and the step returns without a backoff;rerank()rejects at once with the 429 and itsretryAfterMs(5 s) after one request; a candidate backing off from a 503 is not retried once another candidate's 402 stops the step; a pin mismatch followed by a 401 makesrerank()reject with the pin'smalformed_response, matchingrank()'spin_mismatch.probePinthrough a tripped shared breaker reportscircuit_openwith nosentinelStatusand sends nothing. With the previoussystemone.ts,transport.ts,outcome.ts,types.tsandcrossEncoder.tsrestored, all four stop tests fail (with onlytransport.tsrestored, the Retry-After one does); with the previouspin.ts, the sentinel test fails. A 2xx whose body breaks off is not retried once another candidate's 402 stops the step, whether the stop lands before or after it (fails with the previoustransport.ts), and a model that retries after reporting a 2xx is halted before the retry leaves (fails with the previoussystemone.ts);decisions.test.tspins that the transport reports such a body asnull.outcome.test.ts(a deadline reports the backoff it interrupted, not an earlier one): a 500 then a 429 past the deadline israte_limited(cross-encoder andtoOutcomeReranker), a 500 then a 401 whose refresh hangs istimeout, a System One document whose 500 ended in a score leaves a later deadline astimeout, and another document's score does not hide a live 503 backoff. The first four fail with the previous??=first-reply record.outcome.test.tsuses stub HTTP servers and stub decision models. It covers:unauthorized,quotaor a pin mismatch;timeout, notbad_response;too_many_documentsin both modes without a request;quota;withFallbackok/degraded/skipped, per-step deadlines and the late timer, outcome contract validation,gate(including a slow or never-settling one) andonAttempt, a recordednonestep, and usage summed over attempts;probePin;rerankCostwith a caller-supplied table.bm25.test.tscovers scoring,corpusStats,tokenizeCode, an idfReadonlyMapthat is not aMap, and an already-aborted request answered without tokenizing.fallback.ts,systemone.tsandbm25.tsrestored, five tests fail: the chain deadline, identity order on an invalid request, the model's own timeout,too_many_documentsin batch mode, and the non-Mapidf. With the code restored, all pass.npx tsc --noEmitpasses.npm run buildpasses.npm run test:light-entriespasses (16 of 16), andnpm run check:circular-depsreports no cycles.Stack
This PR is 4 of 4. Together they let codegraph depend on the SDK's decisions and rerank modules:
./decisionsand./reranksubpaths (basemain).quotafailure.Merge #623, then #624, then #625, then this PR.