Skip to content

Add AI binding support for pi - #123

Open
Maximo-Guk wants to merge 2 commits into
mainfrom
maximo/add-ai-binding-support-for-pi
Open

Add AI binding support for pi#123
Maximo-Guk wants to merge 2 commits into
mainfrom
maximo/add-ai-binding-support-for-pi

Conversation

@Maximo-Guk

@Maximo-Guk Maximo-Guk commented Aug 10, 2026

Copy link
Copy Markdown
Member

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.

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • vitest on the four touched test files (ai-gateway-binding-fetch, ai-gateway, ai-models, web-fetch): 68 passing. (ai-gateway-cost and 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. AiGatewayConfig centralizes the binding-vs-token decision, and the invariants are enforced eagerly in the constructor (USE_BINDING=true without WORKERS_AI throws; no transport at all throws; google enabled 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 google directly (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-authorization sentinel 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.headers replacing a Request's headers, body: null / signal: null clearing, 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.md that the platform example enables google, 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). When init.signal === null, the ?? already falls through, so the inner init.signal === null re-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-authorization and honoring Authorization: null / x-api-key: null to 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? The ai-models.ts binding 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.

@ask-bonk

ask-bonk Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review posted to PR #123.

Summary

I 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 env.WORKERS_AI.gateway().run() instead of HTTPS-with-a-token.

Verification performed:

  • Ran the four affected test files (ai-gateway-binding-fetch, ai-gateway, ai-models, web-fetch) — 68 passing
  • types:check clean, lint:check no errors, manifest golden test 4/4
  • Confirmed the 6 initial test failures were a pre-existing unbuilt-workspace-dependency issue (@gadgets/typed-storage), not caused by this PR

Findings:

  • The design is sound: transport selection is centralized in AiGatewayConfig with eager invariant checks; Google's HTTPS-only path is guarded twice; the binding shim correctly refuses out-of-prefix / inexpressible requests rather than leaking the auth sentinel; same-account cost-log routing and fetch-spec edge cases are handled and tested.
  • Two non-blocking nits: trailing whitespace introduced in env.d.ts, and a slightly redundant signal: null re-check in the shim.
  • One documentation-level question about the behavioral contract with pi's auth-header suppression across upgrades.

Overall assessment: LGTM once the trailing-whitespace nit is addressed.

github run

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

@Maximo-Guk Maximo-Guk Aug 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@Maximo-Guk
Maximo-Guk force-pushed the maximo/add-ai-binding-support-for-pi branch from 59cd827 to 595d66d Compare August 10, 2026 15:56
@ask-bonk

ask-bonk Bot commented Aug 10, 2026

Copy link
Copy Markdown

Follow-up: verifying the two nits from the prior review

I re-checked the two non-blocking nits against the current HEAD (595d66d). Both are already non-issues:

1. Trailing whitespace in env.d.ts — not present. There is no trailing whitespace anywhere in packages/workshop-backend/src/env.d.ts, and the PR diff adds no lines with trailing whitespace:

$ grep -rnP ' +$' packages/workshop-backend/src/env.d.ts
(no matches)
$ git diff origin/main..HEAD -- packages/workshop-backend/src/env.d.ts | grep -P '^\+.*[ \t]+$'
(no matches)

The current line 15 (// ...supported providers are routed through) has no trailing space. Nothing to trim.

2. Redundant signal: null re-check in ai-gateway-binding-fetch.ts:132-133 — the inner check is not dead code:

const signal = init?.signal ??
    (init && "signal" in init && init.signal === null ? undefined : request?.signal);

When ?? falls through (i.e. init.signal is null or undefined), the right-hand branch must distinguish two cases that the fetch spec treats differently:

  • init.signal === null → explicit clear → yield undefined (drop the Request's signal)
  • init.signal absent → yield request?.signal (inherit the Request's signal)

The init.signal === null test is exactly what separates those, so removing it would change behavior (an explicit signal: null would incorrectly fall back to request.signal). The reviewer's own proposed rewrite preserves the same distinction, and they noted the current form is fine. Leaving as-is.

On the CF_AI_GATEWAY_WAI_DIRECTCF_AI_GATEWAY_USE_BINDING design point

Agreed with the reasoning in Maximo's review comment, and it's already the shipped state: commit 595d66d removed CF_AI_GATEWAY_WAI/CF_AI_GATEWAY_WAI_DIRECT in favor of the single CF_AI_GATEWAY_USE_BINDING=false opt-out, and the docs/env.d.ts/AiGatewayConfig all reflect that. Workers AI always rides the gateway; the opt-out (paired with CF_AI_GATEWAY_ACCOUNT_ID) is what enables a cross-account gateway over the token transport for all providers. No lingering references to the old vars remain in the changed files (the only matches are in plans/pi-impl.md, a historical planning doc that isn't part of this PR).

Verification

  • pnpm --filter workshop-backend types:check — clean.
  • vitest run on ai-gateway-binding-fetch, ai-gateway, web-fetch45 passing. (ai-models.test.ts needs @gadgets/typed-storage built first — the same pre-existing workspace-resolution quirk noted earlier, unrelated to this PR.)
  • pnpm lint:check — exit 0 (only pre-existing warnings).

Net: no code change required for either nit; the PR is in good shape.

@ask-bonk

ask-bonk Bot commented Aug 10, 2026

Copy link
Copy Markdown

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 (595d66d):

  1. Trailing whitespace in env.d.ts — does not exist. Neither the file nor the PR diff contains any trailing whitespace.

  2. "Redundant" signal: null re-check in ai-gateway-binding-fetch.ts:132-133 — the inner init.signal === null test is not dead code. When ?? falls through, it's the only thing distinguishing an explicit signal: null (clear the signal → undefined) from an absent signal (inherit request.signal). Removing it would change behavior. The original reviewer even conceded the current form is fine.

  3. CF_AI_GATEWAY_WAI_DIRECTCF_AI_GATEWAY_USE_BINDING — Maximo's design point is already the shipped state; commit 595d66d removed the old vars in favor of the single USE_BINDING=false opt-out, with docs/env.d.ts/AiGatewayConfig all consistent. The only lingering references are in plans/pi-impl.md, a historical planning doc not part of this PR.

Verification: types:check clean, 45 tests passing on the touched files, lint:check exit 0.

github run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant