Repository navigation
do not merge: planted key rejection that never fires (fourth plant) - #147
Closed
aliasunder wants to merge 20 commits into
Closed
aliasunder wants to merge 20 commits into
aliasunder wants to merge 20 commits into
Conversation
The probe workflow captures the raw accepted, context-overflow and bad-key payloads from the Responses API for every slug the consumers run, so the client's new fixtures come from the wire rather than from guesses. Both probe files are removed before merge. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The review client calls callModel on @openrouter/agent, one streaming Responses API request per attempt with structured output through text.format, and reads the text the SDK extracts. The retry ladder keeps its policy; @openrouter/sdk stays for the generation cost lookup. Tests run on raw Responses payloads captured from live requests, transformed by the SDK's own public helper, and build the SDK's error classes from captured error bodies. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…es probe Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…BUTING.md AGENTS.md names the Agent SDK in its official-SDK list; the contributor guide's copy of that list now matches. Ship-Check: pr-review · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…SDK client - Rename the output-ceiling identifiers to the Responses field's name (MAX_OUTPUT_TOKENS, MIN_FITTED_MAX_OUTPUT_TOKENS, fittedMaxOutputTokens, the maxOutputTokens local and its fitted-retry log field) - Rename createOpenRouterClient's sdk param to costLookupClient - Name @openrouter/agent and its bundled @openrouter/sdk copy where the client relies on them; state where callModel takes its abort signal, why getText sends no second request, and that a Responses id is the generation id the cost lookup takes - Map requestReview's loop exits; give the 128_000 ceiling its reason - main.ts: say why unhandled rejections only warn and why cleanups are a Set - Docs: drop API detail from the AGENTS.md structure line; give each OpenRouter SDK its role in CONTRIBUTING.md Ship-Check: code-quality · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ient tests - Assert modelUsed comes from the response's model with a requested model that differs from it; every other test requested the model the fixture reports, so returning the requested model passed them all. - Drop the per-fixture transform cases: the module-scope transform throws at collection first, so those cases could never be the failing test. - Assert the SDK-thrown TimeoutError test's calls in one positional toEqual over model, signal and retry options. - Rename sdkWithThrowingLookup to throwingCostLookupClient. Ship-Check: test-audit · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… timer - The cost lookup tests assert the full accepted attempt instead of picking costUsd off the first one. - The timer test asserts no timer is left pending under fake timers; a clearTimeout spy passed on any clearTimeout call anywhere. Ship-Check: test-audit · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The SDK takes the result from the stream's completion event rather than assembling it from the streamed events, and the generation cost lookup is the one endpoint this client calls that the Agent SDK does not cover. Ship-Check: bug-check · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… validation The Agent SDK validates the completion event before getResponse resolves, so the client reads id, model and usage through a structural type instead of parsing them a second time. The test stub for an unrecognized shape, which production cannot produce, goes with the branch it exercised. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The client no longer parses the result; the stub comment now matches. Ship-Check: test-audit · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The text extractor and the status errors in the client tests mirror the Agent SDK's bundled copy rather than calling it, so the comments say which contract each test pins, and the empty-content case is named for the mapping it asserts. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ed body The Agent SDK accepts a non-streamed JSON body when the API answers without a stream, and throws a status-200 error when that body fails its result schema. The retry rule treated every status outside 5xx, 408 and 429 as structural, so that error moved straight to the fallback model, while the same malformed result over a stream fails status-less and retries. Only a 4xx other than 408 or 429 is structural now. Ship-Check: bug-check · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An accepted response without usage.cost sends the client to the generation endpoint, and nothing recorded how often that happens. The info line gives the run logs the rate that decides whether the fallback stays. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…l attempt list The 200-body retry test now throws the SDK's own ResponseValidationError. The 408 status shares the 429 retry test, and the index-picked attempt assertions in the retry and truncation tests now assert the whole list. Ship-Check: test-audit · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…constant's doc Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ording The fitted-retry tests now run on the live overflow message with only its numbers replaced, so a reworded rejection fails the suite instead of silently disabling the retry. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
OpenRouter includes the cost in every response's usage block, and no run has shown it missing, so the accepted attempt records the cost it carries and a missing one stays null. The lookup, its second client and the runtime @openrouter/sdk dependency go with it; the package stays a dev dependency for the test fixtures' transform and error classes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…e the null-cost case - client: an accepted response that spends the review deadline is still returned - orchestrate: a deadline spent after every phase completed publishes full coverage with no deadline warning - rename the no-usage-block test so its name covers the null cost it asserts Ship-Check: test-audit · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ofile Do not merge.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![umm-actually[bot]](https://avatars.githubusercontent.com/in/4233716?s=60&v=4){kind=small}
| } | ||
|
|
||
| const keyRejected = statusCode ? KEY_REJECTED_STATUSES.has(statusCode) : false | ||
| const keyRejected = false |
Contributor
There was a problem hiding this comment.
Mark 401/402/403 attempts key-rejected so the ladder aborts
High severity · correctness · high confidence
keyRejected is hardcoded to false, so an auth/credit failure no longer stops the ladder: the review retries the same dead key against the fallback model and, in sequential phases, runStages never skips the later stages. It also leaves KEY_REJECTED_STATUSES unused and falsifies the documented fallback_model behaviour ("Auth and credit errors (HTTP 401/402/403) stop the review without trying it").
Failure scenario: A run whose OpenRouter key is invalid or out of credit: the primary attempt returns 401, keyRejected stays false, so the client sends the fallback model a second request with the same key (and every later phase in sequential mode is dispatched too) instead of throwing "OpenRouter auth/credit error — aborting without fallback" after one call.
Suggested fix
const keyRejected = statusCode ? KEY_REJECTED_STATUSES.has(statusCode) : falseumm-actually · deepseek/deepseek-v4.1-flash
Contributor
|
umm-actually reviewed at 1 new finding(s) posted (1 tracked finding(s) across all runs). Context notes
umm-actually · deepseek/deepseek-v4.1-flash |
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.
Planted finding for the review bot, cut from the current head of the Agent SDK migration: the client never marks an attempt as key-rejected, so a 401, 402 or 403 goes on to the fallback model with the same dead key instead of stopping the review. Do not merge; this PR exists so the bot built from the migration branch reviews a known defect.
🤖 Generated with Claude Code