Surface platform 'upgrade' offers in bench CLI - #441
HeyGarrison wants to merge 1 commit into
Conversation
The v1 API can now attach a machine-readable 'upgrade' block to read responses (and to 403s on subscription-locked benchmarks). Thread it through so LLM agents using the CLI/API learn the paid daily tier exists: - @benchsdk/api: new BenchmarkUpgradeNotice type; responses may include 'upgrade'; BenchmarkClientConfig gains onUpgradeNotice, invoked for every response body that carries one (covers list endpoints whose methods return only data.items, and error bodies via details.upgrade). - bench CLI: collects notices during the request, prints a deduped 'Note: ... / Upgrade: <billingUrl>' to stderr after output, including on the 403 failure path; the upgrade key is stripped from table output but kept in JSON for programmatic consumers.
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Contributor License AgreementAll contributors are covered by a CLA. |
![superagent-security[bot]](https://avatars.githubusercontent.com/in/3287076?s=60&v=4){kind=small}
| pendingUpgradeNotices.set(`${notice.category}:${notice.message}`, notice); | ||
| } | ||
|
|
||
| export function printUpgradeNotices(): void { |
There was a problem hiding this comment.
P2: Sanitize server-controlled upgrade text before printing it to the terminal
API-controlled message and URL are printed directly to stderr without validation or terminal escaping.
Validate trusted URL origins and strip terminal control characters before printing upgrade notices.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="packages/benchsdk-cli/src/output.ts">
<violation number="1" location="packages/benchsdk-cli/src/output.ts:21">
<priority>P2</priority>
<title>Sanitize server-controlled upgrade text before printing it to the terminal</title>
<evidence>printUpgradeNotices() writes notice.message and notice.billingUrl/learnMoreUrl directly with console.error. These values originate from the API response and are only narrowed to object shape by the client, so control characters or an attacker-controlled URL could be emitted to a user's terminal and used for terminal manipulation or phishing.</evidence>
<recommendation>Validate the upgrade notice fields before invoking the callback, require billing and learn-more URLs to use an expected HTTPS origin, and strip or escape terminal control characters from message and URL text before printing. Preserve the raw structured value only for explicitly requested JSON output.</recommendation>
</violation>
</file>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| if (notice && typeof notice === 'object') { | ||
| config.onUpgradeNotice(notice as BenchmarkUpgradeNotice); |
There was a problem hiding this comment.
🟡 Upgrade callback exceptions replace responses
When onUpgradeNotice throws, emitUpgradeNotice rejects successful requests and replaces failed requests' BenchmarkApiError. Callers lose valid results or structured error details.
Learn more
The notice hook runs inside the request's success and failure paths. A thrown callback exception escapes before the successful value returns or the normal BenchmarkApiError is constructed. This makes an observational hook alter the API operation's established result contract.
Example: A client configures onUpgradeNotice to write telemetry, but its telemetry library throws. A 200 response now rejects with the telemetry error. A 403 response also rejects with that error instead of exposing status 403 and the response body.
Recommended fix: Invoke config.onUpgradeNotice inside an exception boundary that preserves the request result. If hook failures need visibility, route them to a separate error callback without replacing the HTTP result.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Companion to computesdk/benchmarks-platform#290, which adds a machine-readable
upgradeoffer to v1 API read responses (and to403s on benchmarks locked behind the per-category daily subscription). This makesbenchand@benchsdk/apiconsumers — including LLM agents driving the CLI — aware the paid daily tier exists when they're reading weekly-stale data.BenchmarkClientConfig.onUpgradeNoticeis invoked fromrequest()whenever a response body carriesupgrade(top-level on success,details.upgradeon errors). A config hook rather than per-method plumbing so list endpoints whose methods returndata.items(e.g.listBenchmarks,listRuns) still surface the notice.BenchmarkUpgradeNoticeis exported from@benchsdk/api, andupgrade?is added toBenchmarkResultsOverview/BenchmarkRunResultsso JSON consumers see it in-band.category:message) footer to stderr after output:It also fires on the 403 error path before
printErrorAndExit, so locked benchmarks still advertise the offer.stripUpgradeFieldremoves the top-levelupgradekey in table mode only;--format jsonkeeps it in-band for programmatic consumers.Verification
Against a local platform build with a seeded unentitled org:
bench runs list <daily-slug>→ stderr notice + API 403;bench runs list <weekly-slug>→ table + stderr notice; entitled org → no notice.pnpm --filter @benchsdk/cli testgreen (32 tests);client.test.tsassertion updated for the newonUpgradeNoticeconfig arg.Known pre-existing:
pnpm --filter @benchsdk/cli typecheckreports vitestMockInstancetype errors insrc/__tests__/output.test.tson an untouched file — same onmain.Link to Devin session: https://app.devin.ai/sessions/2b242a1e747c4dfc8237cd2ad36e50aa
Open in Devin Desktop: https://app.devin.ai/desktop/session/2b242a1e747c4dfc8237cd2ad36e50aa?variant=devin
Requested by: @HeyGarrison