From 356f2c1db4e96a0a43e3d3209d35d97ec4e30291 Mon Sep 17 00:00:00 2001 From: Joonsuh Park Date: Mon, 7 Sep 2026 20:45:17 +0900 Subject: [PATCH] refactor(router): isolate API-key selection capture --- .../010_implementation.md | 32 +++++++++ scripts/test-layout/layout.json | 1 + src/providers/api-key-selection-capture.ts | 10 +++ src/providers/api-key-selection.ts | 9 +-- src/router.ts | 2 +- structure/01_runtime.md | 1 + tests/fixtures/test-layout-expected.json | 1 + .../api-key-selection-capture.test.ts | 69 +++++++++++++++++++ 8 files changed, 117 insertions(+), 8 deletions(-) create mode 100644 devlog/_plan/260907_router_selection_capture/010_implementation.md create mode 100644 src/providers/api-key-selection-capture.ts create mode 100644 tests/providers/api-key-selection-capture.test.ts diff --git a/devlog/_plan/260907_router_selection_capture/010_implementation.md b/devlog/_plan/260907_router_selection_capture/010_implementation.md new file mode 100644 index 0000000000..9ec8b3422f --- /dev/null +++ b/devlog/_plan/260907_router_selection_capture/010_implementation.md @@ -0,0 +1,32 @@ +# Issue #3894: implementation plan + +Satisfy-spec work, triggered by issue #3894 and the request to implement separate draft PRs. Goal: remove the direct router/selection mutation dependency. Non-goals: changing key resolution/failover or eliminating all transitive router cycles. Stop after verified draft PR; report unresolved gates. Escalate if extraction requires behavioral changes. This file records plan and evidence. + +Class C2: one pure helper extracted in the existing provider module convention. Independent branch from 522ce5f8c. + +Current map: router imports api-key-selection for capture; api-key-selection imports router for route resolution. Existing direct helper callers: router and matchesSelection. No package exports change. +Chosen map: both modules import api-key-selection-capture; the old api-key-selection export forwards the same function. New leaf uses only existing OcxProviderConfig and ProviderApiKeySelection type imports. +Rejected alternative: extracting routedProviderConfig would move broad routing dependencies. Other existing transitive cycles stay outside scope. + +File map: +- NEW src/providers/api-key-selection-capture.ts: the existing function body unchanged, plus the two type-only imports. +- MODIFY src/providers/api-key-selection.ts: replace local implementation with named import and compatibility re-export. +- MODIFY src/router.ts: change capture import to the leaf. +- NEW tests/providers/api-key-selection-capture.test.ts: selected/unmatched/missing/duplicate pool cases, immutable snapshot, old-export identity, and Bun-parsed runtime import boundary for leaf and router. Parse actual source, exclude erased type imports; no whole-router acyclicity assertion. +- MODIFY scripts/test-layout/layout.json and tests/fixtures/test-layout-expected.json: register the new test using existing providers domain entries. +- MODIFY structure/01_runtime.md: document the helper ownership and preserved stateful selection direction. + +Verification: helper tests; key-failover, provider-key-store and core-lab-boundary tests; test-layout guards; typecheck; privacy scan. Baseline focused run on unchanged code: 49 passed. Conditional test cases have concrete provider objects; boundary regression returns offending imports rather than scanning prose. + +Audit: extraction is a functional dependency with no mutable globals and no changed auth behavior. Old export is preserved. A boundary test targets this exact scope; broad graph cycles are not called fixed. + +## Verification before draft publication + +- `bun install --frozen-lockfile`: passed; lockfile unchanged. +- Baseline key-failover/provider-key-store/Lab-boundary run: 49 passed. +- Restoring the old router import made the new boundary regression fail; restoring the extraction returned it to green. +- `bun test tests/providers/api-key-selection-capture.test.ts tests/adapters/key-failover.test.ts tests/providers/provider-key-store.test.ts tests/lab/core-lab-boundary.test.ts tests/test-layout.test.ts tests/test-layout-tooling.test.ts`: 73 passed, 0 failed. +- `bun run typecheck`: passed. +- `bun run privacy:scan`: passed. +- Review scope covers the 10-line pure helper, two consumers, compatibility re-export, 7 new tests, two layout entries, and the runtime ownership row. No other router cycle is claimed resolved. +- Whole-suite/maintainer approval is not attested; this is a draft handoff. diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index 60e0f22a0d..fea829d976 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -222,6 +222,7 @@ "api-codex-log-guard.test.ts": "server", "api-debug.test.ts": "server", "api-key-attribution.test.ts": "server", + "api-key-selection-capture.test.ts": "providers", "api-keys-routes.test.ts": "server", "api-storage-cleanup.test.ts": "storage", "api-storage-policy-already-running.test.ts": "storage", diff --git a/src/providers/api-key-selection-capture.ts b/src/providers/api-key-selection-capture.ts new file mode 100644 index 0000000000..302f540969 --- /dev/null +++ b/src/providers/api-key-selection-capture.ts @@ -0,0 +1,10 @@ +import type { OcxProviderConfig } from "../types"; +import type { ProviderApiKeySelection } from "../types/provider"; + +export function captureProviderApiKeySelection(provider: OcxProviderConfig): ProviderApiKeySelection { + return { + entryId: provider.apiKeyPool?.find(entry => entry.key === provider.apiKey)?.id, + reference: provider.apiKey, + revision: provider.apiKeySelectionRevision, + }; +} diff --git a/src/providers/api-key-selection.ts b/src/providers/api-key-selection.ts index 8cf14cfcb3..131c3dfa80 100644 --- a/src/providers/api-key-selection.ts +++ b/src/providers/api-key-selection.ts @@ -6,14 +6,9 @@ import type { ProviderApiKeySelection } from "../types/provider"; import { routedProviderConfig } from "../router"; import { OPENCODE_GO_SESSION_HEADER } from "./opencode-go-transport"; import { resolveProviderTransport, XAI_GROK_COMPATIBILITY, type OcxProviderTransport } from "./xai-transport"; +import { captureProviderApiKeySelection } from "./api-key-selection-capture"; -export function captureProviderApiKeySelection(provider: OcxProviderConfig): ProviderApiKeySelection { - return { - entryId: provider.apiKeyPool?.find(entry => entry.key === provider.apiKey)?.id, - reference: provider.apiKey, - revision: provider.apiKeySelectionRevision, - }; -} +export { captureProviderApiKeySelection } from "./api-key-selection-capture"; function matchesSelection(provider: OcxProviderConfig, expected: ProviderApiKeySelection): boolean { const current = captureProviderApiKeySelection(provider); diff --git a/src/router.ts b/src/router.ts index b2f887f0a9..4baa52bc62 100644 --- a/src/router.ts +++ b/src/router.ts @@ -10,7 +10,7 @@ import { import type { NormalizedComboConfig } from "./combos/types"; import { hasOwnProvider } from "./config/provider-name"; import { providerUsesKeyAuthOverride, resolveProviderApiKey } from "./providers/key-store"; -import { captureProviderApiKeySelection } from "./providers/api-key-selection"; +import { captureProviderApiKeySelection } from "./providers/api-key-selection-capture"; import { assertProviderDestinationAllowed } from "./lib/destination-policy"; import { redactSecretString, redactUrlForLog } from "./lib/redact"; import { diff --git a/structure/01_runtime.md b/structure/01_runtime.md index 1601e5762b..1efd251547 100644 --- a/structure/01_runtime.md +++ b/structure/01_runtime.md @@ -16,6 +16,7 @@ | `src/server/ports.ts` | Owns bind availability and ephemeral-port selection. Temporary probes dispose accepted peers and wait for listener close before reporting success. | | `src/cli/status.ts` / `src/cli/status-probes.ts` | Status snapshot assembly and the shared read-only health/stale-process probes used by status and doctor. Probe evidence keeps recorded-port choice, before/after snapshots and per-call timer cleanup together. | | `src/router.ts` | Provider/model selection before adapter dispatch. Policy execution and ordinary management dry-run share effective-provider capability evidence; unresolved, missing, and disabled providers are excluded before scoring. | +| `src/providers/api-key-selection-capture.ts` | Pure request-owned snapshot of the configured key entry, reference, and revision. The router and stateful selection module share this leaf with type-only dependencies; `api-key-selection.ts` retains the compatibility export and owns persisted selection changes and route resolution. | | `src/types.ts` | Shared config, parsed request, adapter, and event types. | | `src/reasoning-effort.ts` | Codex reasoning-level definitions (`low`/`medium`/`high`/`xhigh`), per-model effort mapping, and catalog effort sanitization. | | `src/codex/shim.ts` | Codex autostart shim: replaces the `codex` binary with a wrapper that auto-starts the proxy on demand. It skips startup for management subcommands even when value-taking global flags precede the subcommand, and transactionally restores complete, stable external launcher replacements without a watcher or PATH rediscovery. | diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index fe613ac71f..398da35149 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -57,6 +57,7 @@ "api-codex-log-guard.test.ts": "server", "api-debug.test.ts": "server", "api-key-attribution.test.ts": "server", + "api-key-selection-capture.test.ts": "providers", "api-keys-routes.test.ts": "server", "api-storage-cleanup.test.ts": "storage", "api-storage-policy-already-running.test.ts": "storage", diff --git a/tests/providers/api-key-selection-capture.test.ts b/tests/providers/api-key-selection-capture.test.ts new file mode 100644 index 0000000000..bd03c9f1e1 --- /dev/null +++ b/tests/providers/api-key-selection-capture.test.ts @@ -0,0 +1,69 @@ +import { describe, expect, test } from "bun:test"; +import { readFileSync } from "node:fs"; +import { captureProviderApiKeySelection } from "../../src/providers/api-key-selection-capture"; +import { captureProviderApiKeySelection as legacyCapture } from "../../src/providers/api-key-selection"; +import type { OcxProviderConfig } from "../../src/types"; +import { repoPath } from "../helpers/repo-root"; + +describe("API-key selection snapshot", () => { + const base: OcxProviderConfig = { adapter: "openai-chat", baseUrl: "https://example.test/v1" }; + + test("captures the selected entry and revision without resolving its reference", () => { + const provider: OcxProviderConfig = { + ...base, + apiKey: "${OCX_CAPTURE_FIXTURE}", + apiKeySelectionRevision: "revision-before", + apiKeyPool: [ + { id: "other", key: "keychain:other" }, + { id: "selected", key: "${OCX_CAPTURE_FIXTURE}" }, + ], + }; + const before = structuredClone(provider); + const snapshot = captureProviderApiKeySelection(provider); + expect(snapshot).toEqual({ entryId: "selected", reference: "${OCX_CAPTURE_FIXTURE}", revision: "revision-before" }); + expect(provider).toEqual(before); + provider.apiKey = "keychain:other"; + provider.apiKeySelectionRevision = "revision-after"; + provider.apiKeyPool![1]!.id = "changed"; + expect(snapshot).toEqual({ entryId: "selected", reference: "${OCX_CAPTURE_FIXTURE}", revision: "revision-before" }); + }); + + test("retains an unmatched reference and absent optional fields", () => { + expect(captureProviderApiKeySelection(base)).toEqual({ entryId: undefined, reference: undefined, revision: undefined }); + expect(captureProviderApiKeySelection({ ...base, apiKey: "keychain:unpooled", apiKeyPool: [] })).toEqual({ + entryId: undefined, reference: "keychain:unpooled", revision: undefined, + }); + }); + + test("preserves first-match semantics when a pool repeats the same reference", () => { + expect(captureProviderApiKeySelection({ + ...base, + apiKey: "keychain:shared", + apiKeyPool: [{ id: "first", key: "keychain:shared" }, { id: "second", key: "keychain:shared" }], + })).toEqual({ entryId: "first", reference: "keychain:shared", revision: undefined }); + }); + + test("preserves the existing export", () => { + expect(legacyCapture).toBe(captureProviderApiKeySelection); + }); +}); + +describe("selection capture dependency boundary", () => { + const transpiler = new Bun.Transpiler({ loader: "ts" }); + const runtimeImports = (source: string) => transpiler.scanImports(transpiler.transformSync(source)).map(entry => entry.path); + + test("the leaf has no runtime imports", () => { + expect(runtimeImports(readFileSync(repoPath("src/providers/api-key-selection-capture.ts"), "utf8"))).toEqual([]); + }); + + test("the router consumes capture without a direct import of the stateful selection module", () => { + const imports = runtimeImports(readFileSync(repoPath("src/router.ts"), "utf8")); + expect(imports).toContain("./providers/api-key-selection-capture"); + expect(imports).not.toContain("./providers/api-key-selection"); + }); + + test("the boundary scanner distinguishes erased types from a runtime dependency", () => { + expect(runtimeImports('import type { T } from "../router"; export const value = 1;')).toEqual([]); + expect(runtimeImports('import "../router"; export const value = 1;')).toEqual(["../router"]); + }); +});