Add AI binding support for pi - #123
Conversation
…E_BINDING opt-out
There was a problem hiding this comment.
Review: AI Gateway over the Workers AI binding
Reviewed the full diff, ran the affected tests, type-check, lint, and the manifest golden test. This is a clean, well-documented change. The kernel-facing pieces (ai-gateway.ts, ai-models.ts, env.d.ts) are small and the design is easy to follow.
Verification
viteston the four touched test files (ai-gateway-binding-fetch,ai-gateway,ai-models,web-fetch): 68 passing. (ai-gateway-costand a handful of other files only failed until I built@gadgets/typed-storage— a pre-existing workspace-resolution quirk, unrelated to this PR; it passes once that package is built.)pnpm --filter workshop-backend types:check: clean.pnpm lint:check: no errors (only pre-existing warnings).node --test scripts/release-manifest.test.js: 4/4 pass (manifest-lib change is comment-only).
What's good
- Transport selection is coherent.
AiGatewayConfigcentralizes the binding-vs-token decision, and the invariants are enforced eagerly in the constructor (USE_BINDING=truewithoutWORKERS_AIthrows; no transport at all throws;googleenabled without a token throws). Failing loudly beats a silent HTTPS fallback. - Google's HTTPS-only path is guarded twice — once at config time for enabled providers, and again at routing time for a stored config that names
googledirectly (ai-models.ts:440). Nice defense in depth, with a comment explaining why the second guard is technically unreachable but kept. - The shim refuses to be a general-purpose fetch. Rejecting out-of-prefix URLs and inexpressible in-prefix requests (rather than passing through) is the right call — silent passthrough would ship the
cf-aig-authorizationsentinel to an arbitrary host. The URL-normalized prefix match (ai-gateway-binding-fetch.ts:74-97) closes the lexical-variant hole, and there's a test for it. - Same-account cost-log routing correctly rides the binding whenever the binding transport is active — even for Google inference, which itself goes over HTTPS (
ai-models.ts:433-438). The asymmetry is subtle and the comment calls it out. - Fetch-spec edge cases are handled and tested:
init.headersreplacing a Request's headers,body: null/signal: nullclearing, one-shot stream bodies consumed only for the JSON probe, streaming responses returned untouched. - Docs and dev-server/manifest comments are updated consistently — including the sharp observation in
public-server.mdthat the platform example enablesgoogle, so the platform server itself still needs the token.
Nits (non-blocking)
env.d.ts:15— the reflowed comment line ("...routed through ") now has trailing whitespace. Worth trimming since this is kernel code held to a higher bar.ai-gateway-binding-fetch.ts:132-133— the signal resolution is correct but slightly redundant:init?.signal ?? (init && "signal" in init && init.signal === null ? undefined : request?.signal). Wheninit.signal === null, the??already falls through, so the innerinit.signal === nullre-check is dead. Could simplify to"signal" in (init ?? {}) ? (init!.signal ?? undefined) : request?.signal, but the current form is fine and well-commented.
Questions
- The module doc and code lean on pi recognizing
cf-aig-authorizationand honoringAuthorization: null/x-api-key: nullto suppress placeholder auth headers. That's a behavioral contract with pi (tracked in the upstream issue). Is it worth a brief note in the code that these tests exercise our shim but not pi's suppression behavior, so a pi upgrade could regress the "no auth header reaches the gateway" guarantee without failing our suite? Theai-models.tsbinding tests do assert the absence of those headers end-to-end through pi, which mitigates this — so mostly a documentation thought.
Overall: LGTM once the trailing-whitespace nit is addressed. Solid tests, clear comments, and the capability/account-boundary reasoning (in-account binding vs. cross-account opt-out) is sound.
|
Review posted to PR #123. SummaryI reviewed PR #123 ("Add AI binding support for pi"), which adds a Workers-AI-binding transport for AI Gateway so deployments can route inference through Verification performed:
Findings:
Overall assessment: LGTM once the trailing-whitespace nit is addressed. |
| CF_AI_GATEWAY_API_TOKEN=... | ||
|
|
||
| # To send Workers AI straight to its REST endpoint (no gateway, no cost logs): | ||
| CF_AI_GATEWAY_WAI_DIRECT=true |
There was a problem hiding this comment.
It's unclear to me why we introduced a CF_AI_GATEWAY_WAI_DIRECT env var in the first place, I don't see why we'd want workers-ai traffic to not go through the gateway. I think the CF_AI_GATEWAY_USE_BINDING=false makes a lot more sense, it opts out all the providers from flowing through the binding transport and with CF_AI_GATEWAY_ACCOUNT_ID allows you to use a cross account gateway for all providers
…ays rides the gateway
The WAI knobs existed because Workers AI originally ran on the account-local
WORKERS_AI binding, which can only name same-account gateways; a cross-account
CF_AI_GATEWAY (the internal production shape) therefore needed an escape
hatch. Workers AI has since moved to ordinary gateway routes
(.../{account}/{gateway}/workers-ai/v1), which work cross-account over
HTTPS+token exactly like anthropic/openai -- so the escape hatch no longer
pays for its config surface.
59cd827 to
595d66d
Compare
Follow-up: verifying the two nits from the prior reviewI re-checked the two non-blocking nits against the current HEAD ( 1. Trailing whitespace in The current line 15 ( 2. Redundant const signal = init?.signal ??
(init && "signal" in init && init.signal === null ? undefined : request?.signal);When
The On the
|
|
Posted findings to PR #123. Summary: No code change was needed. I investigated the two nits from the prior review and the design discussion, and verified against the current HEAD (
Verification: |
Why?
We want to enable users to use AI gateway without needing to provide an API token ( which puts them at risk of credential expiry, rotation, etc ) and makes them need to visit the dashboard just to provision a token during the deploy flow.
How?
Workers AI Gateway can be accessed over the AI binding!
https://blog.cloudflare.com/workers-ai-gateway-unification/
https://developers.cloudflare.com/ai-gateway/usage/worker-binding-methods/#envairun
Add a transport helper
createGatewayBindingFetch({binding, baseUrl, gateway})which returns a FetchFunction that intercepts requests to an AI Gateway HTTPS endpoint (gateway.ai.cloudflare.com/v1/{account}/{gateway}/...) and re-issues them through the Workers AI binding's universal endpoint.I've filed this upstream as well, would be awesome if we could get this upstreamed so everyone can use pi with workers-ai binding earendil-works/pi#7838.
How was this tested?
You can test it with the updated deploy flow which is based off this branches head https://gadgets-deploy.gadgets-staging.workers.dev/deploy ( notice you don't need to provision an api token ), and also locally.