diff --git a/devlog/_plan/260927_release_train_4/provider-compat/000_plan.md b/devlog/_plan/260927_release_train_4/provider-compat/000_plan.md new file mode 100644 index 00000000000..f3d339f1e8a --- /dev/null +++ b/devlog/_plan/260927_release_train_4/provider-compat/000_plan.md @@ -0,0 +1,41 @@ +# Release train 4: provider and API compatibility + +At `origin/dev` `24b2f39b77` (2026-09-27), carry only the managed Pool Images admission fix from #5927 and the response-tier evidence fix from #5497, each as a separate PR rebased on the then-current `dev`. The other candidates need permission, contract, safety, or performance evidence before they change a user path. The P1 Codex App report #4213 remains open until a sanitized failed call identifies the actual route and auth context; the Images route already exists, but can return a configuration error when no capable upstream is available. + +## Loop spec + +- **Archetype / trigger:** Satisfy-spec HOTL release lane, authorized by the train-4 provider-compat packet. +- **Goal / stop:** Record all 7 PR and 6 issue verdicts, merge each safe carried fix after exact-head required CI, comment and close only resolved items, and confirm post-merge `dev` CI. A blocked upstream contract or missing live reproduction is a documented deferral, not a reason to widen scope. +- **Non-goals:** No main/preview, release, deployment, version, GUI, client integration, account-pool redesign, keyless admission bypass, TLS fingerprint change, or logging of request bodies, credentials or account identifiers. Qoder client work belongs to clients-proxy. +- **Verifier:** Focused adapter/server fixtures prove credential substitution and tier observation. `bun run typecheck`, `bun run test:changed`, `bun run structure:check`, `bun run privacy:scan`, `git diff --check`, exact-head required CI and post-merge `dev` CI prove their stated scopes. Seven simultaneous lanes make the local full suite disproportionately expensive; record every focused command and leave broad suite coverage to CI. Conditional cases and what must be observed are in the decade docs. `package.json` defines all named scripts; the six staged roadmap docs were checked with `git diff --cached --check` (exit 0), which reads the staged diff. Use `git diff --check` for later unstaged implementation edits. Run each other verifier before claiming it passed. +- **Memory / terminal outcomes:** This numbered unit is the plan and evidence record. DONE means all criteria above have current evidence. NOOP means an item already has verified coverage. UNSAFE means a proposed contract cannot be safely admitted. NEEDS_HUMAN means a missing external decision or sanitized reproduction blocks a specific item. BLOCKED follows the host threshold, not a single delayed CI check. No user-imposed token or wall-clock bound exists; use the current GitHub credentials only for this repository and the dedicated worktree only for source changes. +- **Escalation:** A discovered credential-destination change, live admission bypass, inability to obtain exact-head required CI, or current `dev` union break triggers replanning or maintainer security review before merge. + +## Work phases and dependencies + +| Phase | Depends on | Deliverable | +| --- | --- | --- | +| `wp0` | — | Docs-only candidate inventory and executable decade docs (`001`, `010`, `020`, `030`, `040`). No production patch. | +| `wp1` | `wp0` | Carry #5927 on latest `dev`, security review, test, PR, merge, then post the initial #4213 diagnostic comment using the Images result; see `010_images.md`. | +| `wp2` | `wp1` | Carry #5497 on latest `dev`, test, PR, merge; see `020_response_tier.md`. | +| `wp3` | `wp2` | Recheck #4213 after the initial Images comment, diagnose #4143 with current code, and triage the remaining issues; see `030_issues.md`. | +| `wp4` | `wp3` | Post candidate dispositions, close carried originals and verified-resolved issues, check post-merge `dev` CI; see `040_integration.md`. | + +`wp1` and `wp2` each make one cohesive code contract reviewable. Later phase P must recheck this plan against the latest `origin/dev` and amend stale line/path assumptions before implementation. Each phase gets P→A→B→C→D and a factual D entry here. The docs-only `wp0` locks this roadmap before code changes. + +## Files and source of truth + +Only `devlog/_plan/260927_release_train_4/provider-compat/` is this lane's planning write scope. `src/AGENTS.md` and `structure/INDEX.md` map the eventual runtime changes to `structure/data-planes/images.md`, `structure/providers-and-adapters.md`, `structure/transports/responses.md`, `structure/config.md`, `structure/runtime.md`, and `structure/gui-and-management-api.md`; update each relevant owner with its code change. `docs-site/src/content/docs/guides/codex-integration.md` and the provider configuration reference own user-facing text. No new top-level document convention is introduced. + +## Architect consultation and audit + +Architect handle `01a0e33e-0d4f-7331-9a06-9676194d930e` proposed T4-D0 (docs first), T4-C1 (Pool Images), T4-C2 (tier authority), and T4-I1 (issue-only diagnostics) against `24b2f39b77`. Accept D0, C1, C2 and I1. Amend C1's test location: `tests/server/server-images.test.ts` is exactly at its 2,755-line ratchet cap (`tests/fixtures/file-size-baseline.json:61`), so new cases go into a registered sibling file. Amend D0: general structure docs change with the owning code phase, while this phase records the planned before/after diff. Keep the architect's two code contracts separate so the pricing observation and tier policy land atomically. Same-handle reflection returned **ALIGNED** for all four IDs. Its three refinements were folded into the latest decade docs: name the negative Pool guard fixture in the sibling, name concrete cost/config test paths, and post the #4213 diagnostic after the Images result instead of waiting for the tier PR. + +Independent reviewer handle `01a0e348-c775-7a50-b2e5-25d11a5efb92` first returned FAIL on outbound header ownership and the #4213 comment timing/auth context. Both blockers were folded into `010_images.md`, `000_plan.md` and `030_issues.md`; the same reviewer re-audited and returned **PASS**. Its two nonblocking refinements were applied: split the safe-send and pre-fetch refusal fixture assertions, and name the staged-diff check accurately. The validator alone does not enforce the planned strict final Authorization shape; implementation must add it and trigger its negative fixture. + +## Phase results + +- **wp0 D, 2026-09-27:** Six docs committed at `d3f1dc9954` on `codex/t4-provider-compat-images`; independent roadmap audit ended PASS after two blockers were folded. `git diff HEAD^ HEAD --check` exited 0, privacy scan passed, and the inventory names 13/13 assigned candidates. No production source changed. **Direction for wp1 P:** recheck the Images plan at current `origin/dev`, then implement #5927 with separate Pool/Direct and final-header fixtures before opening a dev PR. + +Append the exact branch/PR, source and CI SHA, commands/results, issue disposition, and D conclusion for later phases without claiming an unrun suite. +- **wp1 resume, 2026-09-28:** A successor task picked up the lane from the stopped thread's handoff, whose content is folded here, and removed the transfer note. The Codex P1 on #6097 (scoped key could reach Pool credential resolution before the scope check) is fixed by filtering forward candidates by admission scope before `resolveFirstUsableOpenAiSidecar`; see `010_images.md` "wp1 re-entry". An independent sol reviewer re-audited the plan (PASS) and re-verified all eleven non-carried verdicts in `001_candidate_inventory.md` against `origin/dev` `24b2f39b77` (AGREE on all; deferred PR heads unchanged). diff --git a/devlog/_plan/260927_release_train_4/provider-compat/001_candidate_inventory.md b/devlog/_plan/260927_release_train_4/provider-compat/001_candidate_inventory.md new file mode 100644 index 00000000000..5f30dd5a29f --- /dev/null +++ b/devlog/_plan/260927_release_train_4/provider-compat/001_candidate_inventory.md @@ -0,0 +1,28 @@ +# Candidate inventory and release decision + +Read the final PR diffs against `origin/dev` `24b2f39b77` on 2026-09-27. The review is a release decision, not a claim that a candidate PR's own CI passed. PR head SHAs and re-entry requirements must be refreshed before commenting or integrating. + +| Candidate | Head at review | Train-4 decision and evidence | Re-entry condition | +| --- | --- | --- | --- | +| #5995 keyless Zen | `25ccb594a31d` | **Defer, keep open.** The published diff mints OpenCode-shaped session and User-Agent identity in `src/adapters/opencode-free-session.ts` and injects `shell`/`read` declarations in `src/adapters/opencode-free-tools.ts`. The keyless tier has no published third-party integration contract; OpenCode's keyed Zen guide describes API-key access. A later injected-tool rewrite cannot be assumed safe across multi-turn Responses history. No credential/admission bypass goes into this release. | OpenCode authorization for another harness and a three-turn tool-call/continuation fixture, followed by a fresh security review. | +| #5927 Pool Images | `6ff9e6338daa` | **Carry scoped onto current dev.** `src/server/images.ts:663-678` currently rejects every forward candidate when the incoming bearer is a proxy admission secret. Pool resolves a separate stored credential in `src/providers/openai-sidecar.ts:197-216`. Exclude only Direct, preserve explicit keyed and fallback precedence, then verify the final outbound credential. The PR's test additions target a test file exactly at its cap. | Implement `010_images.md`; use a sibling fixture file and exact-head CI. Credit @rrmlima. | +| #5497 Fastwire tier authority | `1152e3c9fc21` | **Carry scoped onto current dev.** `src/server/responses/core-normalize.ts:223` already treats canonical ChatGPT echoes as observational. Other relays can still downgrade Fast on a non-authoritative `default` echo, and `src/usage/cost.ts:441` can read it as pricing evidence. Optional provider declaration must travel through configuration, attempt log, and pricing together. The PR is draft and conflicts. | Implement `020_response_tier.md` with JSON/SSE, durable log and cost fixtures. Credit @hulkbig. | +| #3282 Copilot context tier | `351d8ce04c14` | **Defer, keep open.** The 39-file diff adds a separate per-model map and advertises a million tokens for any configured model while current `src/types/provider.ts:264` has model capability declarations. Model eligibility is conditional; the old catalog hunk moved to `src/codex/catalog/model-hints.ts`. GUI is owned by another lane. | Model-specific authoritative tier/window proof, reduced current-dev adapter contract, and coordinated GUI work. | +| #6003 first-output failover | `8442c7a7a5b4` | **Defer, keep open.** `src/server/first-output-failover.ts` eagerly calls `req.text()` for all requests when any rule exists, before the existing decoded-body limit. It starts the deadline after the first handler returns headers. These paths threaten compressed/large requests and miss header delay. | Bound and decode only eligible requests; start one deadline before dispatch; prove cancellation, real delta, and second-provider disclosure with fixtures. | +| #4732 registry cache | `44bc1015d218` | **Defer, keep open.** Its `src/router.ts` maps may help, but no representative benchmark is supplied. Its older `src/responses/state.ts` snapshot rewrite conflicts with current spill-reference selection in `src/responses/state/snapshot-select.ts`. | Isolate router-only optimization, benchmark current dev, and keep snapshot byte/restore semantics unchanged. | +| #3741 Antigravity TLS | `9764129f3ef7` | **Defer, keep open.** The old `src/server/responses/fetch-helpers.ts` integration predates current physical-send egress policy; the opt-in fingerprint transport crosses an authenticated destination boundary. Value is not yet demonstrated against the integration and review risk. | Reimplement at current egress boundary with fail-closed proxy/destination fixtures, explicit security review, and full exact-head CI. | + +## Issues + +| Issue | Train-4 decision and source evidence | Next observable proof | +| --- | --- | --- | +| #4213 P1 Codex App trial/image surfaces | **Keep open, diagnose and comment.** `src/server/index.ts:402-430` admits `POST /v1/images/generations`; `src/server/images.ts:678-690` can return 400 if no capable upstream is configured. The app trial call's method/path/status has never been captured; the default-deny 404 at `src/server/index/serve-options.ts:316-320` is a possible route, not a verified root cause. | Sanitized OCX/App versions and method/path/status for each symptom, plus whether the image request had a capable authenticated upstream. No body, token, query secret or account ID. | +| #4143 desktop-only Terra | **Keep open pending current reproduction.** The report is from 2.49/Linux. Current `src/codex/catalog/native-models.ts:222,339` contains Terra, and docs describe `disabledModels`, suppression and stale app-server catalogs. | Current OCX/Desktop/CLI versions, selected home, whether Sol/Luna appear, redacted `disabledModels`, and catalog SHA/path after sync and restart. | +| #3506 Cursor/Grok no-progress loop | **Defer, keep open.** `src/adapters/cursor/live-transport.ts:240-250,1094-1104` ends a turn when the client owns a tool; the result arrives in another request. A per-request proxy cutoff cannot classify the whole client trajectory. | A client progress signal or objectively defined cross-request budget and a sanitized trajectory fixture. | +| #3765 Claude Messages→Astra cache plateau | **Defer, keep open.** `src/claude/inbound.ts:465-502` has different metadata/system fallback shapes; cache counters alone cannot identify the changed prefix or upstream placement. | Redacted warm/cold wire-shape comparison with stable system prefix and cache identity, no prompt text. | +| #5270 Qoder MCP/tool calls | **Defer adapter implementation; keep open.** `src/adapters/qoder/adapter.ts:17-29` intentionally disables provider tools and persistence. Enabling tools requires preserving Codex-owned execution and call IDs; Qoder client installation belongs to clients-proxy. | Versioned Qoder tool intent/continuation fixture and an adapter-only mapping contract. | +| #2511 provider request byte budget | **Design only; keep open.** `src/server/responses/outbound-body-guard.ts:67-89` already implements the refusal half. Image downscale/prune changes prompt content and needs a deterministic nesting/newest-turn policy. | Design with exact byte accounting, mutation order, and restoration/consent limits before code. | + +## Decision provenance + +The original PR authors are @rrmlima (#5927) and @hulkbig (#5497); carried commits need `Co-authored-by` trailers. Each original PR receives a link and thanks after its replacement lands. A rejected-but-viable PR stays open with a concrete English reason. Closed issues require a merged dev fix and a comment linking the merge. Existing issue comments for #3506, #5270 and #2511 already explain their core limitations; the train-4 disposition will link or update them only where new evidence changes it. diff --git a/devlog/_plan/260927_release_train_4/provider-compat/010_images.md b/devlog/_plan/260927_release_train_4/provider-compat/010_images.md new file mode 100644 index 00000000000..54891ec0b75 --- /dev/null +++ b/devlog/_plan/260927_release_train_4/provider-compat/010_images.md @@ -0,0 +1,52 @@ +# Phase 1 — managed Pool Images admission + +Dependency: `wp0` and current `origin/dev`. Source candidate: #5927 by @rrmlima, reviewed at `6ff9e6338daa`. Class C4 because the proxy admission bearer and an upstream credential share this route. Scope is the standalone `POST /v1/images/generations` and `/v1/images/edits` forward-candidate decision; no account-pool policy, new provider, route allowlist, or Images API shape is added. + +## Exact change map + +| Path | Action | Before → after | +| --- | --- | --- | +| `src/server/images.ts` | MODIFY | At `handleImages`, `skipOpenAiForwardForAdmissionBearer` makes **all** `candidates.forwardCandidates` ineligible. Replace it with `callerBearerMayBeForwarded` and `eligibleForwardCandidates`: if the bearer is a proxy admission secret, filter out only `accountMode === "direct"`; otherwise use the original list. Pass that exact list to `resolveFirstUsableOpenAiSidecar`. Build the final outbound header set as one canonical `Headers` instance, using `.set` so the selected managed or keyed credential has sole ownership of Authorization. Before the paid fetch, use a separate full-value single-Bearer syntax check **and** `validateForwardAdmissionCredential`; the latter only identifies a proxy secret and cannot replace the syntax check. Guard credential materialization and header assembly: an invalid value yields one fixed, secret-free internal error, no fallback send, and release of any acquired probe lease. Never print the thrown object or headers. Security-specific review details stay in ignored scratch under the root AGENTS rule. | +| `tests/server/server-images-pool-admission.test.ts` | NEW | A self-contained `handleImages` fixture with `createTempHome()` and a fake fetch/Images upstream, using synthetic credentials only. This exercises the real handler and account materialization without importing another test file. Separate cases: (1) Pool success sends one request with one Pool Authorization and no admission secret anywhere in outbound headers; (2) Pool auth failure returns its 401 without a keyed-provider send; (3) Direct remains ineligible with a proxy bearer; (4) conflicting configured/selected Authorization values still send exactly one selected credential, with one safe upstream request; (5) invalid credential materialization and (6) a final assembled header that fails the separate full-value syntax check each give fixed pre-fetch refusal, zero upstream requests, no retained probe lease, and no credential/admission token/prompt in logs. Existing files cover explicit keyed, CCA, ordinary cancellation and scoped auth. Only fake tokens, account IDs and prompts are used. | +| `scripts/test-layout/layout.json` and `tests/fixtures/test-layout-expected.json` | MODIFY | Add `server-images-pool-admission.test.ts` to the `server` explicit mappings in both registries. The existing `tests/server/server-images.test.ts` is exactly 2,755 lines, its ratchet cap, so do not append tests there or raise the cap. | +| `structure/data-planes/images.md` | MODIFY | Document the admitted Pool path: a proxy bearer excludes Direct, while managed Pool substitutes its credential and retains Pool auth-error precedence. State that the outbound bearer is validated before the upstream send. No wildcard route/credential promise. | +| `docs-site/src/content/docs/guides/codex-integration.md` | MODIFY | In Built-in image generation, clarify that a proxy admission token authenticates the local request and can still use managed Pool because Pool supplies its own upstream credential. Direct cannot forward the proxy token; a failed selected Pool account does not trigger a billed keyed fallback. Keep the existing xAI/CCA/explicit-keyed statements. | + +`validateForwardAdmissionCredential` checks the Authorization bearer for a proxy admission secret at `src/server/auth-cors.ts:554-557`; the new full-value syntax check is a separate pre-fetch condition. The test additionally asserts that **no** outbound header value contains the synthetic proxy secret. Do not describe the existing helper as a generic arbitrary-header scanner. + +## Activation and observable checks + +1. Proxy admission bearer + valid Pool credential + no explicit Images provider: one intercepted Images call uses the stored Pool bearer; returned JSON stays in the Codex Images shape. The fake upstream's captured header set contains no proxy secret. +2. Same bearer + selected Pool account without usable credential + configured keyed provider: 401 from Pool and zero intercepted keyed calls. This observes the fail-closed paid-provider precedence. +3. Same bearer + Direct-only route: no caller-token forward; existing keyed/CCA or configuration error wins according to current selection. +4. Invalid credential materialization and final-header validation are each triggered with synthetic fixture values: fixed-text 500 before any network send, no retained probe lease, and no credential or prompt in logs. A separately triggered header-ownership case sends one safe selected credential. Detailed security reproduction stays in ignored scratch until the fix is public. + +Once the Images PR is on `dev`, post the initial #4213 comment from `030_issues.md` immediately. It may link this limited Pool fix while explicitly leaving the Astra-trial cause unverified. `wp3` later rechecks both symptoms and final issue disposition. + +## Verification and review + +Run `bun test tests/server/server-images-pool-admission.test.ts tests/server/api-key-scope-images.test.ts`, `bun test tests/test-layout-tooling.test.ts tests/ci-workflows/file-size-ratchet.test.ts`, `bun run test:changed`, `bun run typecheck`, `bun run privacy:scan`, `bun run structure:check`, `git diff --check`, and the docs-site frozen-install/build required by `docs-site/AGENTS.md`. `tests/server/server-images.test.ts` must still run in CI, but a local direct run from this managed worktree failed 82 cases before assertions because its cleanup path lies under the protected real `.codex` tree; do not bypass that safety guard. `bun run test:changed` may select the same file and encounter the same guard, so record its exact result without treating it as a pass. `bun test tests/server/api-key-scope-images.test.ts` passed 7/7 after `bun install --frozen-lockfile` restored missing dependencies, and baseline `bun run typecheck` exited 0. The new fixture must use `createTempHome()` outside that tree. The focused commands name their changed paths directly; the structure and privacy scripts follow `package.json`. Do not claim a suite from a skipped or stale output. A dedicated read-only security reviewer checks credential substitution, outbound header values, auth-error precedence and lease release against the final diff. The PR targets latest `dev`, carries @rrmlima's `Co-authored-by`, and merges only after exact-head required CI and review concerns pass. Record current dev union and post-merge run. + +## wp1 stale check and design decisions + +`origin/dev` remained `24b2f39b77` at wp1 entry, and `src/server/images.ts:660-700` still had the candidate filter described above. Architect handle `01a0e353-8511-7c71-be31-c7a99b8de08b` proposed IMG-WP1-01 (Pool/Direct filter), IMG-WP1-02 (single `Headers` owner), IMG-WP1-03 (strict final authorization), IMG-WP1-04 (pre-fetch lease release), and IMG-WP1-05 (sibling fixture). Accept all five. Keep the implementation inside the existing handler and account materializer; no new production abstraction or endpoint is needed. The alternative of requiring every user to change the client's auth-header shape does not repair standard bearer-authenticated Images calls. The detailed security analysis stays in ignored scratch under the root AGENTS rule. Same-architect reflection returned **ALIGNED**. Independent A reviewer `01a0e35c-07fe-7253-a481-2d1516e20c99` first found two blockers in pre-fetch failure handling and tracked security detail; after amendments and a final full-value check clarification, the same reviewer returned **PASS**. + +## Build and validation record before PR + +- The new fixture was red on the old handler: 1 pass, 5 fails, including Pool success returning 400 and Pool auth failure falling through to keyed 200. After the change, its eight cases pass, including the independent final admission guard case. `tests/server/api-key-scope-images.test.ts` and layout/ratchet guards passed in the 39-case focused run; typecheck, privacy scan, structure check, and docs-site frozen install/build completed successfully. +- `bun run test:changed` ran 2,615 tests across 133 files: 2,352 pass, 1 skip, 262 fail. Every failure is a test fixture cleanup inside the protected real `.codex` tree of this managed worktree. The separate `fixture upstream connection reset` line belongs to a passing injected-failure test. Do not call this suite green or bypass the removal guard; hosted CI checks the full path outside this worktree layout. +- Independent security reviewer `01a0e36a-9180-74e3-ab92-2034c4b753e8` first requested a distinct final admission-guard fixture. It was added and run; the same reviewer rechecked the staged diff and returned **SECURITY VERDICT: PASS**. This is a read-only security review, not a live-provider test. + +## wp1 re-entry: scope before Pool credential I/O (2026-09-28) + +Previous D direction (from `_handoff.md`): resolve the Codex P1 on #6097 so a scoped admission key cannot reach Pool credential resolution before `admissionScopeDenial`, keep the destination scope check for keyed/CCA fallback, then revalidate, push, and merge after exact-head CI. + +Plan, confirmed against `src/server/images.ts:667-760` at `acc750effa`: + +- Compute `relaySelector` before forward resolution. Filter `eligibleForwardCandidates` a second time with `admissionScopeDenial(config, admission, relaySelector, { providerName: candidate.providerName, modelId: relaySelector })` and keep the first denial response. Only allowed candidates reach `resolveFirstUsableOpenAiSidecar`, so a forbidden key never touches a stored credential, refresh, probe lease or account state. +- Keep the post-resolution check on `forward.providerName` as defence in depth; it is a no-op for a candidate that passed the pre-filter. +- When no forward candidate remains and no keyed provider exists, still try CCA (which applies its own scope check), then return the recorded scope denial (403) before the generic configuration 400. A keyed provider still runs its own branch check, so a key scoped to that provider keeps working. +- Tests: the two local WIP cases (forbidden Pool → 403 with zero sends; allowed Pool+model → 200 with the Pool bearer). Add one more: a key allowed only for the keyed provider falls through to the keyed branch without Pool resolution. + +Review dispositions on #6097: the Codex co-author finding is already satisfied by the genuine trailer on `acc750effa`; the squash message will repeat it as a standalone trailer. The CodeRabbit request to apply an RFC 6750 `b64token` pattern to the managed Pool bearer is declined: the Pool token is a stored credential the proxy never builds from request input, the header is assembled through `Headers.set`, and a stricter local pattern would only turn an upstream 401 into a local 500 without closing a reachable path. +\nSecurity re-review (sol reviewer) of `46515c2a2e` returned FAIL with two findings, both present on `origin/dev` before this PR. Accepted: a client cancel during the in-flight Images send returned 499 without releasing a selected Pool account's recovery probe lease; the 499 branch now releases it, with a red-green fixture. Rebutted for this PR and recorded as follow-up: `selectImagesProvider` (`src/providers/openai-sidecar.ts:263,301`, unchanged here) resolves the keyed provider's API-key reference before the keyed branch scope check. That is a local config/keychain read with no network send or account-state change, and moving it changes keyed selection and error precedence for every Images caller; it belongs in a separate hardening change.\n \ No newline at end of file diff --git a/devlog/_plan/260927_release_train_4/provider-compat/020_response_tier.md b/devlog/_plan/260927_release_train_4/provider-compat/020_response_tier.md new file mode 100644 index 00000000000..1062097fc81 --- /dev/null +++ b/devlog/_plan/260927_release_train_4/provider-compat/020_response_tier.md @@ -0,0 +1,34 @@ +# Phase 2 — relay response-tier evidence + +Dependency: merged `wp1` and refreshed `origin/dev`. Source candidate: #5497 by @hulkbig, final reviewed head `1152e3c9fc2130a0518e4dc5c79de8f16505607b`. Class C3 for configuration, Responses observation and pricing persistence. The feature does not enable Fast, alter the outbound `service_tier`, or decide a provider's actual bill; it marks whether a *response echo* is proof. + +## Exact change map + +| Path | Action | Before → after | +| --- | --- | --- | +| `src/types/provider.ts` | MODIFY | Add optional `responseTierAuthoritative?: boolean` to `OcxProviderConfig` and `AttemptTierOutcome`. Absent retains existing relay authority. Canonical ChatGPT forwarding stays observational even if a user sets `true`. | +| `src/config/schema/leaf-validators.ts`, `src/server/auth-cors.ts`, `src/providers/model-rename-fields.ts` | MODIFY | Validate a real optional boolean, admit it to the provider editor DTO/policy, and classify it as a non-model-keyed provider field during rename. Reject `"false"`, `0`, `null`, or object rather than silently changing meaning. | +| `src/providers/openai-tiers-destination.ts` | MODIFY | Add `responseTierAuthorityForProvider(provider)` returning `false` for canonical ChatGPT-forward destination and the declared boolean otherwise. Keep current `isCanonicalOpenAiForwardProvider` destination test and unrelated service-tier support logic. | +| `src/server/responses/core-normalize.ts` | MODIFY | Replace the current one-off `isCanonicalOpenAiForwardProvider(route.provider) ? false : undefined` observation argument with `responseTierAuthorityForProvider(route.provider)`. The final routed provider, not requested alias, owns the decision. | +| `src/providers/fastwire.ts` | MODIFY | Carry `context.responseTierAuthoritative` into the attempt outcome only when defined. Preserve raw sanitized response echo and current request decision logic. | +| `src/usage/log.ts` | MODIFY | `normalizeAttemptTierOutcome` validates, retains and rehydrates the optional boolean. Old records without it continue reading with legacy semantics. | +| `src/usage/cost.ts` | MODIFY | `serviceTierContextFromOutcome` consults raw `responseServiceTier` only when `responseTierAuthoritative !== false`. A non-authoritative echo remains in logs, while an eligible requested priority tier remains an assumption for an estimate. A local unsupported-route downgrade still stays downgraded. | +| `tests/routing/fastwire-response-authority.test.ts` | NEW | Copy the final candidate test from `1152e3c9fc21:tests/routing/fastwire-response-authority.test.ts` as the starting fixture, then adapt current test contracts. It covers JSON/SSE relay and official API sends, raw echo, assumption versus downgrade, persisted attempt/final entries, editor redaction, malformed config and pricing. | +| `tests/providers/model-rename-migration.test.ts` | MODIFY | Assert provider rename keeps the optional field with provider scope and never treats it as a model-keyed map. | +| `scripts/test-layout/layout.json`, `tests/fixtures/test-layout-expected.json` | MODIFY | Add the new routing test file to both explicit maps. Do not change file-size caps. | +| `structure/config.md`, `structure/providers-and-adapters.md`, `structure/runtime.md`, `structure/transports/responses.md`, `structure/gui-and-management-api.md` | MODIFY | State the optional observation policy, final-route ownership, log/price provenance and editor field round trip. Update only currently true statements; keep core routing and tier request policy intact. | +| `docs-site/src/content/docs/guides/codex-integration.md`, `docs-site/src/content/docs/reference/configuration/providers.md`, `docs-site/src/content/docs/zh-cn/reference/configuration/providers.md` | MODIFY | Explain the optional relay declaration, known canonical exception, raw echo versus confirmed grant and pricing estimates in English and directly affected translation. No promise that the relay bills at priority. | + +This is one PR and one atomic runtime contract. Do not split observation from cost: current `src/usage/cost.ts:441-450` otherwise turns the raw echo back into pricing confirmation. Do not carry unrelated changes from the older branch blindly; compare every hunk with current `dev` before applying. Search the provider field name, the outcome field name and both boolean values across config serialize/deserialize, editor, rename, route, log and cost consumers. Creation: JSON/editor provider input. Serialization: saved provider config and attempt log. Deserialization: leaf validator and usage entry normalizer. Consumers: final route observation, Fastwire tracker, tooltip/price estimate. No new enum value is added. + +## Activation and observable checks + +1. Relay with declaration `false`, eligible outbound priority request, response echo `default`: outbound wire remains priority, attempt is `applied/assumed`, raw echo remains `default`, no response-declined downgrade or confirmation in price provenance. Trigger in both JSON and SSE fixtures. +2. Same relay with response echo `priority`: still `assumed`, because the echo cannot prove a granted tier. This protects cost from a false confirmation. +3. Absent or `true` declaration with the same echo: legacy authoritative judgment is unchanged. Canonical ChatGPT forwarding remains observational even when the declaration says true. +4. Malformed value fails config validation; provider editor round-trips a valid value without returning the API key. Persisted new record retains the flag; old record lacking it remains readable. +5. Unsupported Fast route stays downgraded even when `false` is declared. The field cannot manufacture capability. + +## Verification + +Run `bun test tests/routing/fastwire-response-authority.test.ts tests/routing/fastwire-observability.test.ts tests/providers/model-rename-migration.test.ts tests/providers/provider-config-validation.test.ts tests/usage/usage-cost.test.ts tests/usage/cost-cap-unknown-evidence.test.ts`; these direct paths exercise the changed route, config and cost contracts. Also run `bun run test:changed`, `bun run typecheck`, `bun run privacy:scan`, `bun run structure:check`, `git diff --check`, and the docs-site frozen-install/build required by `docs-site/AGENTS.md`. The test fixture has a fake relay; it is not live evidence of a particular provider's billing. Record the resource exception for the local full suite and rely on completed exact-head required CI before merge. The PR credits @hulkbig with a `Co-authored-by` trailer, targets latest `dev`, and gets an independent review of the full current-dev diff. diff --git a/devlog/_plan/260927_release_train_4/provider-compat/030_issues.md b/devlog/_plan/260927_release_train_4/provider-compat/030_issues.md new file mode 100644 index 00000000000..7905419d799 --- /dev/null +++ b/devlog/_plan/260927_release_train_4/provider-compat/030_issues.md @@ -0,0 +1,32 @@ +# Phase 3 — Codex App and provider issue disposition + +Dependency: both selected code PRs merged. This is an issue-analysis and comment phase. It changes no runtime source, listener policy, or installed Codex app. Recheck current `origin/dev`, each issue's newest comment and the merged Images behavior before posting. Comments must be in English, cite code or merged PRs, and never request a body, token, query secret or account identifier. + +## Exact external write map + +| Issue | Action | Before → after | +| --- | --- | --- | +| #4213 | Recheck the initial comment posted after `wp1`; ADD a correction only if fresh evidence arrives; keep open | Current issue has a bypass report but no failing Astra method/path/status. The initial comment separates the known `/v1/images/generations` admission/config branch from an unverified app trial path, links the merged Pool Images fix if relevant, and gives a concrete diagnostic capture plan. Do not claim the trial prompt fixed or widen `/v1/*` on inference. | +| #4143 | ADD a comment; keep open unless a current fixture proves resolution | Old 2.49 Linux Desktop-only Terra screenshot becomes an explicit request for current version, catalog generation and suppression facts. Current native roster already contains Terra; do not call that proof the user's app picker is repaired. | +| #3506 | Existing owner comment explains client-owned tool continuation. Keep open; add a train-4 note only if new evidence changes the contract. | No proxy one-request timeout is proposed for a cross-request no-progress trajectory. | +| #3765 | ADD a comment; keep open | Request a redacted stable-prefix warm/cold comparison and clarify that cache counters alone do not show where the prefix changed. | +| #5270 | Existing owner comment records intentional tools-disabled Qoder adapter. Keep open; add a train-4 note only if the other lane lands a relevant client integration. | Adapter tool-call bridge requires versioned Qoder intent and call-ID continuation fixture; client integration remains outside this lane. | +| #2511 | ADD a design-only comment if the issue lacks a current scoped decision; keep open | Current refusal guard is distinct from the requested automatic image mutation. Record a deterministic proposed downscale→prune policy and the unresolved input-loss choice. | + +## #4213 causal boundary and discriminating capture + +Known: the fixed loopback allowlist in `src/server/index.ts:402-430` admits `POST /v1/images/generations`, and `src/server/images.ts:678-690` returns a configuration 400 when no capable OpenAI/CCA path is usable. The default-deny handler in `src/server/index/serve-options.ts:316-320` answers an unknown path with 404. None is a captured failing Astra-trial request. Hypotheses and falsifiers: + +1. An app trial/entitlement call reaches a non-allowlisted `/v1` path and gets 404. Falsifier: the sanitized failing call is allowed or never reaches the proxy. +2. The trial prompt is gated by app/account state independent of a proxied inference endpoint; the restart in the report changes that state. Falsifier: same app/account session shows a deterministic proxy-visible failing method/path/status when only the endpoint changes. +3. The image tool reaches Images but has no capable authenticated upstream. Falsifier: response is not that handler's 400, or a capable upstream is proven present and the failure is elsewhere. + +The route/auth contract needed to proxy any new native app surface is unknown. Ask for separate sanitized `method + path + status` records for trial and image symptoms, exact OCX/App versions, a yes/no about a capable image upstream, and whether the Images request used proxy admission auth or a direct ChatGPT bearer (category only, no value or account identifier). If the image status is 400, first determine which Images branch returned it; if 401/403, inspect admission/auth policy; if 404, identify exact path; if 2xx but app still fails, inspect response shape without prompt content. A new path needs documented upstream destination, credential owner, and negative fixtures before allowlisting. This is a concrete plan and a statement of what is known, not an invented root cause. + +## #2511 design decision only + +The existing opt-in `maxUpstreamBodyBytes` checks the final serialized outbound request in `src/server/responses/outbound-body-guard.ts:67-89`; leave its refusal behavior intact. A future remediation could run only after routing selects the provider and before each physical send: measure the *actual* serialized body; if over limit, downscale eligible inline images one by one using deterministic quality steps and reserialize; if still over, prune oldest eligible image parts while preserving the newest user turn and every non-image text/tool/call-ID relation; reserialize and stop when under budget. If preserving those invariants cannot fit, return the current bounded error. Repeat measurement for retries and adapters that rebuild wire bodies. Do not log or persist original/resized bytes or content-derived tags. Unknowns that block code: whether the user has approved lossy image mutation, which image encodings can be safely rewritten, order among multiple images in one turn, and whether tool-result images can be removed without breaking call pairing. Resolve these in the issue before implementation. + +## Verification + +For each posted comment, read it back from GitHub and confirm number, body, open/closed state and link. A current source/merged-commit link is needed for any claim of resolution. For #4213 and #4143, no local user's app configuration is changed. If the diagnostic records arrive during this phase, amend this doc, run a separate repair cycle if a bounded regression is feasible, and update the issue with verified evidence. diff --git a/devlog/_plan/260927_release_train_4/provider-compat/040_integration.md b/devlog/_plan/260927_release_train_4/provider-compat/040_integration.md new file mode 100644 index 00000000000..ed169efa838 --- /dev/null +++ b/devlog/_plan/260927_release_train_4/provider-compat/040_integration.md @@ -0,0 +1,23 @@ +# Phase 4 — PR and dev integration receipts + +Dependency: `wp3`. Recheck every candidate's latest head, base, review state, and comments before any external write. This phase records final disposition and confirms integration; it does not alter main/preview, tags, versions or other lanes. + +## Exact external write map + +| Target | Action | Before → after | +| --- | --- | --- | +| Carried #5927 and #5497 | ADD appreciative comments linking each replacement PR and merged dev SHA, then CLOSE the superseded originals | Original contribution remains credited by `Co-authored-by` in the replacement branch commit or PR description. Do not close until replacement is on `dev`. | +| Deferred #5995, #3282, #6003, #4732, #3741 | ADD one English comment each with a concrete blocking contract and re-entry proof; KEEP open | #5995 needs permission + multi-turn fixture; #3282 needs model-specific context evidence and reduced non-GUI diff; #6003 needs bounded decoded request handling and deadline/cancellation fixtures; #4732 needs benchmark and snapshot restore semantics; #3741 needs current egress integration and security review. Avoid duplicates if a newer author revision already addresses the blocker. | +| Issues #4213, #4143, #3506, #3765, #5270, #2511 | CLOSE only if a specific merged dev PR actually resolves that issue, with an exact link; otherwise keep open with the reason in `030_issues.md` and GitHub comments | `dev` PR merge does not auto-close an issue. Do not present a partial Images improvement as resolution of both #4213 symptoms. | +| `origin/dev` | READ CI status and latest merge commits | Confirm the required integration workflow for each merged head, then the current dev CI run. If this lane's change breaks dev, fix it through a new authorized PR and recheck. | + +## Merge gate repeated per selected PR + +1. Fetch current `origin/dev` and compare the candidate branch's merge base. Rebase before push if stale, rerun the affected local checks, and inspect the union for file-size ratchet and exhaustive union/locale/count drift. +2. PR targets `dev`, uses all Summary/Verification/Checklist sections, documents the seven-lane local full-suite exception with exact focused commands/results, includes the original author's `Co-authored-by` trailer, and names any required security review. No screenshots are needed when no `gui/` path changes. +3. Wait for every **required** check on the exact PR head SHA to finish `success`. Check rollup absence, pending, skipped, cancelled, or old-head results are not passing evidence. Resolve valid Codex/CodeRabbit findings and current maintainer objections. +4. Record maintainer integration choice and SHA evidence in the PR, merge through a PR only, fetch and verify dev merge SHA, then inspect dev CI. Never force-push dev. + +## Evidence and final report + +Append the current PR number, head SHA, exact check run URL/attempt/conclusion, merge SHA, original-PR closure link, issue disposition links, commands/exit codes, and remaining limitations to `000_plan.md`. The final user report lists each assigned PR and issue, the two carried methods, dev CI result, test scope, risk, and overlap files. If a required check or branch protection is pending, keep the relevant PR open and state its exact status instead of claiming completion. diff --git a/docs-site/src/content/docs/guides/codex-integration.md b/docs-site/src/content/docs/guides/codex-integration.md index 2a95f14de68..671c63c1940 100644 --- a/docs-site/src/content/docs/guides/codex-integration.md +++ b/docs-site/src/content/docs/guides/codex-integration.md @@ -227,8 +227,10 @@ This is separate from the [Image Bridge](/guides/image-bridge/), which only acti **Responses** turn lists the hosted `image_generation` tool while a non-OpenAI model is selected. Standalone `/images/generations` calls never enter that bridge. -- **One mode-aware forward candidate:** Pool selects an eligible main/added account; Direct uses the - caller OAuth bearer. The configured mode applies consistently to the image request. +- **One mode-aware forward candidate:** Pool selects an eligible main/added account and uses its + stored ChatGPT credential even when the client authenticates to opencodex with a proxy admission + bearer. Direct uses the caller's ChatGPT OAuth bearer and cannot forward a proxy admission token. + The configured mode applies consistently to the image request. - **OpenAI API-key provider:** it is used only when no forward candidate owns an authentication failure. A broken/expired Pool credential is never hidden behind separately billed API usage. - **Explicit custom provider:** set `images.provider` to the id of a custom API-key diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index f3f16fae58b..0cca11009a2 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -1666,6 +1666,7 @@ "server-google-antigravity-oauth-429-budget.test.ts": "server", "server-gui-bundle-freshness.test.ts": "server", "server-images-bodyless-content-length.test.ts": "server", + "server-images-pool-admission.test.ts": "server", "server-images.test.ts": "server", "server-key-failover-e2e.test.ts": "server", "server-jev-combo-e2e.test.ts": "server", diff --git a/src/server/images.ts b/src/server/images.ts index fdf944b1c3f..5eeb669738d 100644 --- a/src/server/images.ts +++ b/src/server/images.ts @@ -56,6 +56,14 @@ import { codexModelAvailabilityErrorResponse } from "./responses/codex-auth-erro export type ImagesEndpoint = "generations" | "edits"; +function assertImagesUpstreamAuthorization(headers: Headers, config: OcxConfig): void { + const authorization = headers.get("authorization")?.trim() ?? ""; + if (!/^Bearer[\t ]+[^\s,]+$/i.test(authorization)) { + throw new TypeError("invalid image upstream authorization"); + } + validateForwardAdmissionCredential(headers, config); +} + /** Image generation is slow (tens of seconds); bound a hung upstream, not a working one. */ const IMAGES_UPSTREAM_TIMEOUT_MS = 300_000; @@ -659,25 +667,41 @@ export async function handleImages( } const explicitKeyedProvider = config.images?.provider !== undefined && candidates.keyed !== undefined; // Admission bearer is valid proxy auth (requireApiAuth already passed) but must never be - // forwarded as OpenAI ChatGPT credentials. When the caller sent it, skip OpenAI forward - // and allow CCA / keyed paths instead of rejecting the whole request. - let skipOpenAiForwardForAdmissionBearer = false; + // forwarded as OpenAI ChatGPT credentials. Pool replaces it with a stored credential; + // Direct would consume the caller bearer, so only Direct becomes ineligible. + let callerBearerMayBeForwarded = true; if (!explicitKeyedProvider) { try { validateForwardAdmissionCredential(req.headers, config); } catch (err) { if (err instanceof ForwardAdmissionCredentialError) { - skipOpenAiForwardForAdmissionBearer = true; + callerBearerMayBeForwarded = false; } else { throw err; } } } - const canUseOpenAiForward = !skipOpenAiForwardForAdmissionBearer && candidates.forwardCandidates.length > 0; + // Both relay branches copy the body upstream, so the destination is the + // provider chosen in that branch and the model the caller named. A scoped key + // is checked against each forward destination BEFORE any stored credential is + // resolved, refreshed or leased, so a forbidden destination spends nothing. + const relaySelector = requestedImageSelector(body); + let forwardScopeDenial: Response | undefined; + const eligibleForwardCandidates = candidates.forwardCandidates.filter(candidate => { + if (!callerBearerMayBeForwarded && candidate.accountMode === "direct") return false; + const denial = admissionScopeDenial(config, admission, relaySelector, { + providerName: candidate.providerName, + modelId: relaySelector, + }); + if (denial) forwardScopeDenial ??= denial; + return denial === undefined; + }); + const canUseOpenAiForward = eligibleForwardCandidates.length > 0; if (!canUseOpenAiForward && !candidates.keyed) { const ccaResponse = await tryCcaImageGeneration(body, config, logCtx, req.signal, endpoint, admission); if (ccaResponse) return ccaResponse; + if (forwardScopeDenial) return forwardScopeDenial; // 400, not 5xx: codex retries every 5xx up to 5 total attempts, and this is a permanent // configuration state that must surface on the first attempt. return formatErrorResponse( @@ -696,7 +720,7 @@ export async function handleImages( let forwardAuthError: Response | undefined; if (canUseOpenAiForward) { try { - forward = await resolveFirstUsableOpenAiSidecar(candidates.forwardCandidates, req.headers, config, { + forward = await resolveFirstUsableOpenAiSidecar(eligibleForwardCandidates, req.headers, config, { beginCodexAccountSelection: codexAccountSelectionForTurn(turnAdmissionLease), }); if (forward) logCtx.provider = formatCodexProviderForLog(forward.providerName, codexLogAccountId(forward.authContext), config); @@ -708,38 +732,48 @@ export async function handleImages( } else if (err instanceof CodexThreadAffinityExpiredError) { forwardAuthError = formatErrorResponse(409, "invalid_request_error", "Codex thread account affinity expired; start a new session"); } else if (err instanceof CodexAuthContextError) { - const safeAccountLabel = formatCodexProviderForLog("openai", err.accountId, config); - console.error(`[images] Pool account ${safeAccountLabel} token failed; reauthentication required`); + console.error("[images] Pool credential failed; reauthentication required"); forwardAuthError = formatErrorResponse(401, "authentication_error", "Selected Codex account needs reauthentication"); } else if (err instanceof CodexModelAvailabilityError) { forwardAuthError = codexModelAvailabilityErrorResponse(err); } else if (err instanceof CodexPoolAuthenticationError) { forwardAuthError = formatErrorResponse(401, "authentication_error", err.message); + } else if (err instanceof TypeError) { + // The sidecar releases any acquired probe lease if credential materialization fails. + return formatErrorResponse(500, "internal_error", "image generation request failed forward credential validation"); } else { throw err; } } } - const headers: Record = { "content-type": "application/json" }; + const headers = new Headers({ "content-type": "application/json" }); + const releaseForwardProbe = (): void => { + try { forward?.releaseProbeLease?.(); } + catch { console.error("[images] Failed to release probe lease"); } + }; let url: string; - // Both relay branches copy the body upstream, so the destination is the - // provider chosen in that branch and the model the caller named. Each branch - // is checked as it is entered, before it resolves a credential or commits a - // key rotation, so a refused request spends nothing. - const relaySelector = requestedImageSelector(body); + // Each branch is re-checked as it is entered: the forward check is defence in + // depth for the pre-filter above, and the keyed check runs before a key + // rotation is committed. if (forward) { const denial = admissionScopeDenial(config, admission, relaySelector, { providerName: forward.providerName, modelId: relaySelector, }); if (denial) { - forward.releaseProbeLease?.(); + releaseForwardProbe(); return denial; } const { provider } = forward; - if (provider.headers) Object.assign(headers, provider.headers); - for (const [name, value] of forward.headers) headers[name] = value; + try { + for (const [name, value] of Object.entries(provider.headers ?? {})) headers.set(name, value); + for (const [name, value] of forward.headers) headers.set(name, value); + assertImagesUpstreamAuthorization(headers, config); + } catch { + releaseForwardProbe(); + return formatErrorResponse(500, "internal_error", "image generation request failed forward credential validation"); + } // The ChatGPT codex backend takes bare paths (matches the adapter's `${baseUrl}/responses`). url = `${provider.baseUrl}/images/${endpoint}`; } else if (forwardAuthError) { @@ -783,8 +817,13 @@ export async function handleImages( ); } const apiKey = warmKeyProvider?.apiKey ?? candidates.keyed.apiKey; - if (provider.headers) Object.assign(headers, provider.headers); - headers["authorization"] = `Bearer ${apiKey}`; + try { + for (const [name, value] of Object.entries(provider.headers ?? {})) headers.set(name, value); + headers.set("authorization", `Bearer ${apiKey}`); + assertImagesUpstreamAuthorization(headers, config); + } catch { + return formatErrorResponse(500, "internal_error", "image generation request failed forward credential validation"); + } logCtx.provider = providerName; // Keyed providers tolerate baseUrl with or without /v1 (mirrors openai-responses.ts). url = `${provider.baseUrl.replace(/\/v1\/?$/, "")}/v1/images/${endpoint}`; @@ -799,6 +838,11 @@ export async function handleImages( ); } + if (req.signal.aborted) { + releaseForwardProbe(); + return formatErrorResponse(499, "client_closed_request", `image ${endpoint} request canceled by client`); + } + const timeoutMs = config.images?.timeoutMs ?? IMAGES_UPSTREAM_TIMEOUT_MS; const linkedSignal = signalWithTimeout(timeoutMs, req.signal); const sidecarExit = sidecarEnter("images"); @@ -846,6 +890,9 @@ export async function handleImages( // Client cancel first: it aborts the linked signal too, and must not be logged as an // upstream failure (499 maps to client_closed_request in the request log). if (req.signal.aborted) { + // No upstream outcome is recorded for a client cancel, so a recovery probe + // lease taken by the selected Pool account must be returned explicitly. + releaseForwardProbe(); return formatErrorResponse(499, "client_closed_request", `image ${endpoint} request canceled by client`); } if (linkedSignal.signal.aborted || (err instanceof Error && err.name === "TimeoutError")) { diff --git a/structure/data-planes/images.md b/structure/data-planes/images.md index 97c8bced793..d7ef1b85f12 100644 --- a/structure/data-planes/images.md +++ b/structure/data-planes/images.md @@ -42,7 +42,11 @@ separately billed generation. On non-loopback binds, data-plane authentication and origin policy cover both Images routes. An explicit keyed Images provider accepts the proxy admission secret as either an OpenAI-style bearer or `x-opencodex-api-key` because the provider key replaces caller authorization before fetch. The -ChatGPT forward path still requires the dedicated header so its upstream bearer remains distinct. +ChatGPT Direct path still requires the dedicated header so its caller-owned upstream bearer remains +distinct. A proxy admission bearer leaves managed Pool eligible: Pool replaces it with its stored +credential, while Direct cannot forward it. A selected Pool authentication failure remains its own +error rather than falling through to a separately billed keyed provider. The outbound Images send +has one selected Authorization value, validated before the non-idempotent upstream POST. The keyed path never enters `handleResponses`, so `src/server/images.ts` repeats `selectProactiveApiKeyTransport` inside the keyed branch and rebuilds Authorization from the returned clone rather than the earlier snapshot. @@ -52,8 +56,11 @@ on: the ChatGPT forward account, the keyed provider, the xAI Imagine bridge, or fallback. It is evaluated against that destination rather than the selector in the body, because the bridge and the fallback choose their own model, and a body that names no model cannot satisfy a model list. A refusal is the same 403 the scope returns on the routed path, and a key with no -scope reaches every destination as before. Coverage lives in -`tests/server/api-key-scope-images.test.ts`. +scope reaches every destination as before. Forward candidates are filtered by that scope before any +stored Pool credential is resolved, refreshed or leased, so a forbidden key never reaches account +state; when no allowed destination remains, the 403 wins over the generic configuration 400. +Coverage lives in `tests/server/api-key-scope-images.test.ts` and +`tests/server/server-images-pool-admission.test.ts`. The API-key `openai-responses` path also adapts Codex's private standalone image tool to the public Responses tool surface. A complete `image_gen` namespace is lowered to safe diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index 01768b541df..fd22fa5ccca 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -1514,6 +1514,7 @@ "server-google-antigravity-oauth-429-budget.test.ts": "server", "server-gui-bundle-freshness.test.ts": "server", "server-images-bodyless-content-length.test.ts": "server", + "server-images-pool-admission.test.ts": "server", "server-images.test.ts": "server", "server-key-failover-e2e.test.ts": "server", "server-jev-combo-e2e.test.ts": "server", diff --git a/tests/server/server-images-pool-admission.test.ts b/tests/server/server-images-pool-admission.test.ts new file mode 100644 index 00000000000..022de3186cd --- /dev/null +++ b/tests/server/server-images-pool-admission.test.ts @@ -0,0 +1,301 @@ +import { afterEach, beforeEach, expect, test } from "bun:test"; +import { saveCodexAccountCredential } from "../../src/codex/account-store"; +import { clearAccountNeedsReauth } from "../../src/codex/account-runtime-state"; +import { clearAccountQuota, setAccountQuotaFromParsed } from "../../src/codex/quota"; +import { + CODEX_QUOTA_PROBE_INTERVAL_MS, + canAcquireCodexQuotaProbeLease, + clearCodexUpstreamHealth, + clearThreadAccountMap, + getCodexUpstreamHealth, + recordCodexUpstreamOutcome, +} from "../../src/codex/routing"; +import { saveConfig } from "../../src/config"; +import type { DataPlaneAdmission } from "../../src/server/auth-cors"; +import { handleImages } from "../../src/server/images"; +import type { RequestLogContext } from "../../src/server/request-log"; +import type { OcxConfig, OcxProviderConfig } from "../../src/types"; +import { createTempHome, type TempHome } from "../helpers/temp-home"; + +const ADMISSION_SECRET = "pool-admission-fixture"; +const POOL_TOKEN = "pool-access-fixture"; +const PROMPT = "private-image-fixture"; +const originalFetch = globalThis.fetch; +const previousAdmissionSecret = process.env.OPENCODEX_API_AUTH_TOKEN; +let home: TempHome | undefined; +let sent: Array<{ url: string; headers: Headers }> = []; + +beforeEach(() => { + home = createTempHome("ocx-images-pool-admission-"); + process.env.OPENCODEX_API_AUTH_TOKEN = ADMISSION_SECRET; + clearCodexUpstreamHealth(); + clearThreadAccountMap(); + clearAccountQuota(); + clearAccountNeedsReauth("pool-img"); + sent = []; + globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { + const url = typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url; + if (!url.endsWith("/images/generations")) throw new Error("unexpected fixture upstream"); + sent.push({ url, headers: new Headers(init?.headers) }); + return Response.json({ created: 1, data: [{ b64_json: "aGk=" }] }); + }) as typeof fetch; +}); + +afterEach(() => { + globalThis.fetch = originalFetch; + if (previousAdmissionSecret === undefined) delete process.env.OPENCODEX_API_AUTH_TOKEN; + else process.env.OPENCODEX_API_AUTH_TOKEN = previousAdmissionSecret; + clearCodexUpstreamHealth(); + clearThreadAccountMap(); + clearAccountQuota(); + clearAccountNeedsReauth("pool-img"); + home?.remove(); + home = undefined; +}); + +function imageConfig(mode: "pool" | "direct" = "pool", options: { + providerHeaders?: Record; + keyed?: boolean; +} = {}): OcxConfig { + const openai: OcxProviderConfig = { + adapter: "openai-responses", + baseUrl: "https://chatgpt.com/backend-api/codex", + authMode: "forward", + codexAccountMode: mode, + ...(options.providerHeaders ? { headers: options.providerHeaders } : {}), + }; + return { + port: 0, + defaultProvider: "openai", + openaiProviderTierVersion: 2, + providers: { + openai, + ...(options.keyed ? { + "openai-apikey": { + adapter: "openai-responses", + baseUrl: "https://api.openai.com/v1", + authMode: "key", + apiKey: "keyed-image-fixture", + } satisfies OcxProviderConfig, + } : {}), + }, + codexAccounts: [{ id: "pool-img", email: "pool-img@example.test", plan: "team", isMain: false }], + activeCodexAccountId: "pool-img", + accountPoolStrategy: "fill-first", + } as OcxConfig; +} + +function savePool(config: OcxConfig, accessToken = POOL_TOKEN): void { + saveConfig(config); + saveCodexAccountCredential("pool-img", { + accessToken, + refreshToken: "pool-refresh-fixture", + expiresAt: Date.now() + 3_600_000, + chatgptAccountId: "acct-pool-fixture", + }); + setAccountQuotaFromParsed("pool-img", { weeklyPercent: 10, weeklyResetAt: Date.now() / 1000 + 3_600 }); +} + +function request(): Request { + return new Request("http://127.0.0.1/v1/images/generations", { + method: "POST", + headers: { "content-type": "application/json", authorization: `Bearer ${ADMISSION_SECRET}` }, + body: JSON.stringify({ model: "gpt-image-2", prompt: PROMPT }), + }); +} + +function logContext(): RequestLogContext { + return { model: "image_gen", provider: "" }; +} + +function markRecoveryProbeDue(config: OcxConfig): number { + const old = Date.now() - CODEX_QUOTA_PROBE_INTERVAL_MS - 1_000; + recordCodexUpstreamOutcome(config, "pool-img", 429, { + now: old, + resetAt: Date.now() + 60 * 60_000, + }); + expect(getCodexUpstreamHealth("pool-img")?.cooldownUntil).toBeGreaterThan(Date.now()); + expect(canAcquireCodexQuotaProbeLease("pool-img")).toBe(true); + return old; +} + +async function callImages(config: OcxConfig, admission?: DataPlaneAdmission): Promise { + return handleImages(request(), config, "generations", logContext(), undefined, admission); +} + +test("proxy admission bearer uses managed Pool Images credentials", async () => { + const config = imageConfig(); + savePool(config); + const response = await callImages(config); + expect(response.status).toBe(200); + expect(sent).toHaveLength(1); + expect(sent[0]!.url).toBe("https://chatgpt.com/backend-api/codex/images/generations"); + expect(sent[0]!.headers.get("authorization")).toBe(`Bearer ${POOL_TOKEN}`); + expect(sent[0]!.headers.get("chatgpt-account-id")).toBe("acct-pool-fixture"); + expect([...sent[0]!.headers.values()].some(value => value.includes(ADMISSION_SECRET))).toBe(false); +}); + +test("Pool authentication failure does not fall back to a billed keyed Images provider", async () => { + const config = imageConfig("pool", { keyed: true }); + saveConfig(config); + const logs: string[] = []; + const originalError = console.error; + console.error = (...args: unknown[]) => { logs.push(args.map(String).join(" ")); }; + let response: Response; + try { + response = await callImages(config); + } finally { + console.error = originalError; + } + expect(response.status).toBe(401); + expect(sent).toHaveLength(0); + expect(logs).toEqual(["[images] Pool credential failed; reauthentication required"]); +}); + +test("scoped admission denies Pool before resolving a forbidden credential", async () => { + const config = { + ...imageConfig(), + apiKeys: [{ + id: "scoped-images", name: "scoped-images", key: ADMISSION_SECRET, + createdAt: "2026-09-27T00:00:00.000Z", allowedProviders: ["another-provider"], + }], + } as OcxConfig; + saveConfig(config); + const admission: DataPlaneAdmission = { kind: "configured", keyId: "scoped-images", source: "bearer" }; + const response = await callImages(config, admission); + expect(response.status).toBe(403); + expect(sent).toHaveLength(0); +}); + +test("scoped admission still sends Images through an allowed Pool destination", async () => { + const config = { + ...imageConfig(), + apiKeys: [{ + id: "scoped-images", name: "scoped-images", key: ADMISSION_SECRET, + createdAt: "2026-09-27T00:00:00.000Z", allowedProviders: ["openai"], + allowedModels: ["gpt-image-2"], + }], + } as OcxConfig; + savePool(config); + const admission: DataPlaneAdmission = { kind: "configured", keyId: "scoped-images", source: "bearer" }; + const response = await callImages(config, admission); + expect(response.status).toBe(200); + expect(sent).toHaveLength(1); + expect(sent[0]!.headers.get("authorization")).toBe(`Bearer ${POOL_TOKEN}`); +}); + +test("key scoped to the keyed provider skips Pool resolution and sends keyed Images", async () => { + const config = { + ...imageConfig("pool", { keyed: true }), + apiKeys: [{ + id: "scoped-images", name: "scoped-images", key: ADMISSION_SECRET, + createdAt: "2026-09-27T00:00:00.000Z", allowedProviders: ["openai-apikey"], + }], + } as OcxConfig; + // No Pool credential is stored: resolving Pool first would return its 401. + saveConfig(config); + const admission: DataPlaneAdmission = { kind: "configured", keyId: "scoped-images", source: "bearer" }; + const response = await callImages(config, admission); + expect(response.status).toBe(200); + expect(sent).toHaveLength(1); + expect(sent[0]!.url).toBe("https://api.openai.com/v1/images/generations"); + expect(sent[0]!.headers.get("authorization")).toBe("Bearer keyed-image-fixture"); +}); + +test("proxy admission bearer remains ineligible for Direct forwarding", async () => { + const config = imageConfig("direct", { keyed: true }); + saveConfig(config); + const response = await callImages(config); + expect(response.status).toBe(200); + expect(sent).toHaveLength(1); + expect(sent[0]!.url).toBe("https://api.openai.com/v1/images/generations"); + expect(sent[0]!.headers.get("authorization")).toBe("Bearer keyed-image-fixture"); + expect([...sent[0]!.headers.values()].some(value => value.includes(ADMISSION_SECRET))).toBe(false); +}); + +test("selected Pool bearer owns Authorization despite a differently cased configured header", async () => { + const config = imageConfig("pool", { providerHeaders: { Authorization: "Bearer configured-fixture" } }); + savePool(config); + const response = await callImages(config); + expect(response.status).toBe(200); + expect(sent).toHaveLength(1); + expect(sent[0]!.headers.get("authorization")).toBe(`Bearer ${POOL_TOKEN}`); + expect([...sent[0]!.headers.values()].some(value => value.includes("configured-fixture"))).toBe(false); +}); + +async function expectPreFetchCredentialRefusal(config: OcxConfig, old: number, privateValue: string): Promise { + const logs: string[] = []; + const originalError = console.error; + console.error = (...args: unknown[]) => { logs.push(args.map(String).join(" ")); }; + let response: Response; + try { + response = await callImages(config); + } finally { + console.error = originalError; + } + expect(response.status).toBe(500); + const body = await response.text(); + expect(body).toContain("image generation request failed forward credential validation"); + expect(body).not.toContain(privateValue); + expect(body).not.toContain(ADMISSION_SECRET); + expect(body).not.toContain(PROMPT); + expect(sent).toHaveLength(0); + expect(getCodexUpstreamHealth("pool-img")?.probeLeaseId).toBeUndefined(); + expect(getCodexUpstreamHealth("pool-img")?.lastProbeAt).toBeGreaterThan(old); + expect(logs.join(" ")).not.toContain(privateValue); + expect(logs.join(" ")).not.toContain(ADMISSION_SECRET); + expect(logs.join(" ")).not.toContain(PROMPT); +} + +test("invalid selected Pool bearer is refused before send and releases its probe lease", async () => { + const config = imageConfig(); + const invalidToken = "bad,credential-fixture"; + savePool(config, invalidToken); + const old = markRecoveryProbeDue(config); + await expectPreFetchCredentialRefusal(config, old, invalidToken); +}); + +test("a selected Pool token matching proxy admission never reaches the upstream", async () => { + const config = imageConfig(); + savePool(config, ADMISSION_SECRET); + const old = markRecoveryProbeDue(config); + await expectPreFetchCredentialRefusal(config, old, ADMISSION_SECRET); +}); + +test("invalid Pool credential materialization is refused without a send or private log", async () => { + const config = imageConfig(); + const invalidToken = "bad\ncredential-fixture"; + savePool(config, invalidToken); + const old = markRecoveryProbeDue(config); + await expectPreFetchCredentialRefusal(config, old, invalidToken); +}); + +test("invalid configured header assembly is refused before an Images send", async () => { + const invalidValue = "private\nheader-fixture"; + const config = imageConfig("pool", { providerHeaders: { "x-image-fixture": invalidValue } }); + savePool(config); + const old = markRecoveryProbeDue(config); + await expectPreFetchCredentialRefusal(config, old, invalidValue); +}); + +test("client cancel during a Pool Images send releases the recovery probe lease", async () => { + const config = imageConfig(); + savePool(config); + markRecoveryProbeDue(config); + const controller = new AbortController(); + globalThis.fetch = ((_input: RequestInfo | URL, init?: RequestInit) => new Promise((_resolve, reject) => { + sent.push({ url: String(_input), headers: new Headers(init?.headers) }); + init?.signal?.addEventListener("abort", () => reject(new DOMException("aborted", "AbortError")), { once: true }); + controller.abort(); + })) as typeof fetch; + const req = new Request("http://127.0.0.1/v1/images/generations", { + method: "POST", + headers: { "content-type": "application/json", authorization: `Bearer ${ADMISSION_SECRET}` }, + body: JSON.stringify({ model: "gpt-image-2", prompt: PROMPT }), + signal: controller.signal, + }); + const response = await handleImages(req, config, "generations", logContext()); + expect(response.status).toBe(499); + expect(sent).toHaveLength(1); + expect(getCodexUpstreamHealth("pool-img")?.probeLeaseId).toBeUndefined(); +});