Repository navigation
refactor(openrouter): send review requests through the Agent SDK - #143
aliasunder wants to merge 19 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>
|
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}
|
umm-actually re-reviewed at 1 new finding(s) posted (17 tracked finding(s) across all runs). Context notes
umm-actually · deepseek/deepseek-v4.1-flash |
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>
|
Add @openrouter/agent to CONTRIBUTING's official SDK list
This PR adds @openrouter/agent to package.json and swept the identical SDK list in AGENTS.md, but the contributor-facing copy in CONTRIBUTING.md still names only @openrouter/sdk, so the doc that tells contributors which SDKs to reach for understates what the review path uses. Failure scenario: A contributor adding another OpenRouter call reads CONTRIBUTING.md's list, does not see the agent package, and hand-rolls a fetch or reuses the REST client for the review request; the review path then carries two request implementations that drift apart on retry, timeout and structured-output handling. Suggested fix- Use the official SDKs (`@actions/core`, `@actions/github`,
`@openrouter/agent`, `@openrouter/sdk`, `parse-diff`) — don't hand-roll what they already doumm-actually · deepseek/deepseek-v4.1-flash |
…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>
|
Fixed in 3f4a713: CONTRIBUTING.md's official SDK list names @openrouter/agent beside @openrouter/sdk, with each package's role. 🔍 ship-check · pr-monitor · claude-fable-5-1 |
… 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>
|
Restate 'one request per review phase' for the agent run
The 'one request per review phase' mechanism claim predates the Agent SDK: a phase is now one callModel run, which may issue additional billed requests (the SDK's empty-final retry / final turn), so the sentence misstates the request and cost unit a phase represents. Failure scenario: An operator reading a parallel run's cost table expects exactly one billed request per phase; a phase whose response comes back empty makes the SDK issue a second request whose tokens and cost never appear in the summary, and the doc's one-request framing leaves no way to notice the gap. Suggested fixState the run granularity, e.g.: "Sends one model call per review phase to OpenRouter (see the `phases` input for dispatch modes); the SDK may issue additional requests inside a call, such as its empty-final retry."umm-actually · deepseek/deepseek-v4.1-flash |
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>
|
Strip PR numbers, SHAs and review chronology from the PR description
The PR description carries internal references — PR #145 and #144, the commit SHAs 8c178c5 and ff7cff2, links to prior review-bot discussion comments, and the chronology of an earlier planted finding — which AGENTS.md forbids in public artifacts, PR descriptions included. Those references are permanently visible to anyone reading the public repository, while the description never explains the SDK migration a future reader needs. Failure scenario: A reader browsing the public repository opens this PR and learns which evaluation PRs planted which bugs, the commits they were cut from, and the bot discussion links used to score them, while someone later hunting the rationale for the SDK migration finds only experiment chronology in the PR body. Suggested fixKeep the description to the change itself: the review request moving to `@openrouter/agent`'s `callModel` (Responses API, `text.format` structured output), `@openrouter/sdk` staying for the generation cost lookup, the retry ladder it preserves, and the fixtures the captures became. Move the plant/verification narrative to the harness or issue tracker entries instead of the PR body.umm-actually · deepseek/deepseek-v4.1-flash |
…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>
Summary
Moves the review request from
@openrouter/sdk'schat.sendtocallModelfrom@openrouter/agent, one streaming Responses API request per attempt with structured output throughtext.format. The retry ladder keeps its attempt counts, timeout advance, fitted-ceiling retry and key rejection, with one change to what counts as structural: only a 4xx outside 408 and 429 skips the same-model retry now, so the status-200 error the SDK throws when a non-streamed body fails its result schema retries once like a network error instead of advancing straight to the fallback.The generation cost lookup is gone: OpenRouter includes the cost in every response's usage block, the probe found it on every slug, and no run has shown it missing, so an accepted attempt records the cost it carries and a missing one stays null in the cost table.
@openrouter/sdkmoves to the dev dependencies, where the tests use its result transform and error classes.A temporary probe workflow on this branch captured raw Responses API payloads with the repo's key: an accepted payload for every model slug the consumers run that OpenRouter still lists, a forced context overflow, and a bad key. The captures drive the client's tests as fixtures, run through the SDK's own result transform, and the error fixtures build the SDK's error classes. The probe files were removed once the captures landed.
What the probe showed
response.completedevent carries the message text and a numericusage.cost; the SDK'sgetText()extracts the text.error.message, worded as on the chat path, so the fitted-ceiling retry reads the same pattern.The first consumer bump that runs GLM as its primary is the live check for that model; the self-review here runs DeepSeek.
Planted Finding
Head: 6349479
Plant PR #147, cut from this head, made the client never mark an attempt as key-rejected, so a 401, 402 or 403 would go on to the fallback model with the same dead key. The bot built from that branch filed it as a high-severity correctness finding, "Mark 401/402/403 attempts key-rejected so the ladder aborts": #147 (comment). The comment names the planted rule.
Three earlier plants on earlier heads were caught the same way: the same key-rejection plant from
ab65d28(#146 (comment)), and a retry on every 4xx from8c178c5(#145 (comment)) and fromff7cff2(#144 (comment)).🤖 Generated with Claude Code