Add AI binding support for pi - #123
Conversation
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
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: |
|
Looks like upstream PR was merged, shall we update to use that? |
|
Just waiting on a PI release before we can use the upstreamed changes, I'll put up a PR to update once it gets released. The vendored transport changes in this PR are identical to what got merged in to PI, I would like to get this out so we can start battle testing what we upstreamed in to PI. |
|
Drafting this for the time being as I work through on updating the PI implementation to be smarter & more efficient |
595d66d to
24fbea1
Compare
|
The shim's One thing to verify: for openai, pi's I've completed a thorough review. Let me do a final check on the Let me also double check there is no issue with I've completed my review. The PR is well-constructed with strong test coverage, and the previous review concerns (trailing whitespace, the signal re-check, the I could not run the test suite or type-check because workspace dependencies aren't installed in this read-only environment (no I found no discrete, actionable defects introduced by this change. The design is sound (transport selection centralized with eager invariants, Google double-guarded, sentinel/auth headers proven stripped end-to-end through pi's real SDK, URL-normalized prefix matching, fetch-spec edge cases handled), the docs/env/dev-server/manifest comments are all consistent, and no stale Note for context: the PR was moved to draft by the author on 2026-08-13 pending a pi release and further implementation work, so it may not be intended for merge as-is. LGTM! |
24fbea1 to
0672c92
Compare
|
No trailing whitespace. Let me do one last careful check for a genuine defect — the interaction between the I've completed a thorough review. Let me summarize my findings. I've completed a thorough review of PR #123 at HEAD Review findingsI inspected all 13 changed files, traced the binding shim's control flow, the No actionable defects found. Specifically verified:
I could not run the test suite or type-check in this read-only environment (no Note for context: the author moved the PR to draft on 2026-08-13 pending a pi release and further implementation work, so it may not be intended for merge as-is. LGTM! |
0672c92 to
9b54ad4
Compare
…E_BINDING opt-out
…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.
9b54ad4 to
f8704a2
Compare
|
There's a minor coverage gap: the shim inherits a Request input's signal ( I've completed a thorough review. My analysis: Files reviewed (13): the new binding shim, Verification I could perform (read-only, no
I could not run the test suite or type-check in this read-only environment, but the tests are self-consistent with the implementation and prior CI was green. I found no discrete, actionable defects introduced by this change. The only observations are non-blocking: a small test-coverage gap (no test asserts a Request's own signal is forwarded when init omits it — though the sibling clear-signal case exercises the same branch), and the PR was moved to draft by the author on 2026-08-13 pending a pi release, so it may not be intended for merge as-is. LGTM! |
|
The manifest change is comment-only; the I've completed a thorough review of PR #123 at HEAD Review findingsI inspected all 13 changed files: the new binding shim ( No discrete, actionable defects found. Specifically verified:
I could not run the test suite or type-check in this read-only environment (no Non-blocking observations only (not posted inline): a minor test-coverage gap — no test asserts a Request's own signal is forwarded when init omits LGTM! |
f8704a2 to
6f18507
Compare
|
So Now the concern: This is actually a behavior improvement over the old code: previously I've completed a thorough review. Let me compile my findings. Review SummaryI reviewed PR #123 at HEAD I inspected all 13 changed files, traced the shim's control flow, the What I verified
Notes (non-blocking, not posted inline)
I found no discrete, actionable defects introduced by this change. LGTM! |
Binding-routed gateway traffic went through gateway().run(), the
universal endpoint's [{provider, endpoint, headers, query}] envelope,
which a vendored shim built by splicing the request body into a JSON
string. The gateway also serves its provider-native passthrough over the
AI binding at /ai-gateway/gateways/{gateway}/{provider}/... -- the HTTPS
path minus the account id, since the binding channel carries identity --
and that route accepts exactly the requests pi's API impls already
produce.
So there is nothing left to translate. Binding-routed models take the
binding host as their gateway root and pi's fetch option is the binding's
own fetch, unwrapped; the envelope, the splice, the JSON-object scanner
and the whole vendored ai-gateway-binding-fetch module go away. Method,
headers, query string and the body stream ride through untouched, so
multi-MB prompt bodies are never copied in the isolate.
Google keeps its HTTPS root automatically: `binding` is resolved per
provider, and bindingFor() returns undefined for google (its adapter
can't take a custom fetch). cf-aig-authorization now reaches the gateway,
which recognizes the pre-authentication sentinel and strips it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6f18507 to
976a0e7
Compare
|
node_modules aren't fully installed (read-only env). The Let me do one final review of a potential real correctness concern: the In the constructor: if Now, one genuinely subtle case: Google in hybrid mode with the log route. I've completed a thorough review of the actual code at HEAD Review notesThe earlier What I verified
Non-blocking observations (not posted inline)
I found no discrete, actionable defects introduced by this change. LGTM! |
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.