Skip to content

test: add Codex provider auth lifecycle regression matrix - #2268

Open
lsm wants to merge 1 commit into
devfrom
space/create-provider-auth-lifecycle-regression-matrix
Open

test: add Codex provider auth lifecycle regression matrix#2268
lsm wants to merge 1 commit into
devfrom
space/create-provider-auth-lifecycle-regression-matrix

Conversation

@lsm

@lsm lsm commented Jul 27, 2026

Copy link
Copy Markdown
Owner

Adds a provider auth lifecycle regression matrix (provider-auth-lifecycle-matrix.test.ts) that walks the full lifecycle — login, authenticated call, logout, credential removal, stale-token cleanup, failed credential, and re-add after removal — on the Codex bridge provider, parametrized over OAuth and API-key auth modes.

The existing Codex suite tests each stage in isolation; this one composes the stages on a single instance (login → call → logout → stale-cleanup → re-add) to pin the cross-stage state-machine transitions that broke across stale-token and removal/re-add flows — the dogfood paths called out in the task. 19 tests, all passing.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@lsm lsm left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🤖 Review by glm-5.1 (GLM)

Model: glm-5.1 | Client: NeoKai | Provider: GLM (Zhipu)

Recommendation: APPROVE (posted as COMMENT only because the PR author and reviewer share the same GitHub identity, so GitHub rejects APPROVE — this is an explicit approve).

What this adds

A single test-only file, provider-auth-lifecycle-matrix.test.ts (478 lines, +478/−0, no source/config changes), walking all 7 lifecycle stages from task #755 — login, authenticated call, logout, credential removal, stale-token cleanup, failed credential, re-add after removal — parametrized over OAuth and API-key on AnthropicToCodexBridgeProvider. The composed walks at the bottom drive login → call → logout → re-add on one instance, which is the genuine differentiator from the sibling per-scenario suite.

Verification performed

  • Runs green: 19/19 pass (73 expects). Sibling anthropic-to-codex-bridge-provider.test.ts still 67/67.
  • Gates: biome clean, tsc --noEmit clean (oxlint ignores *.test.ts by config — tests are exempt, as expected).
  • Assertion correctness: read the full provider (getBridgeAuth, setCredentials, logout, refreshStoredOauthCredentials, probeUpstream, verifyCredentials) and traced every assertion to its implementation path — including the two fetch seams the header documents (injected fetchImpl for probes, module-global fetch for refreshCodexToken), the cachedBridgeAuth undefined-vs-null tri-state, and the definitive/transient refresh split (4xx except 408/429 → definitive).
  • Teeth proven by mutation testing (introduced each bug class, ran the suite, reverted — source restored byte-identical):
    • Stale-cache bug (remove cachedBridgeAuth = undefined from logout()): 9/19 fail — every logout / removal / re-add / stale-cleanup case fires, including both composed walks.
    • Stale-token-cleanup bug (skip logout() on definitive refresh failure): 3/19 fail — both definitive-cleanup cases plus the "regression target" composed walk.
    • The suite is not vacuous; it catches exactly the dogfood paths called out in the task.

Non-blocking observations (not requesting changes)

  1. Two composed refresh spies lack try/finally. The per-mode refresh tests (lines 312–357) wrap spyOn(globalThis,'fetch') in try/finally; the two composed refresh tests (lines 445–447, 470–472) call mockRestore() inline without a guard. Today this is harmless — they are the last tests in the file and nothing downstream depends on a clean globalThis.fetch — but if a later test is ever appended below them, a failed assertion could leak the mock. Optional: wrap for consistency.
  2. Probe cache makes the re-add getModels assertion rely on isAvailable(). verifyCredentials caches a successful probe for 30s keyed by bridgeAuthCacheKey(auth); after logout + re-add with identical creds, the second getModels hits the cache and does not re-probe. The "no stale leak" guarantee is actually carried by the isAvailable() check (which goes through getBridgeAuthcachedBridgeAuth), not the probe — so the assertion is correct, just not re-verifying the upstream probe on re-add. This is the implementation's intended behavior, not a test defect; flagging only so future readers interpret the assertion correctly.

Both are informational; neither warrants a change cycle. Nice work — the matrix is well-scoped, correctly targets the regression class, and the composed walks add real signal the isolated suite lacks.

Walks the full auth lifecycle (login, authenticated call, logout, credential
removal, stale-token cleanup, failed credential, re-add after removal) on the
Codex bridge provider — the regression target — parametrized over OAuth and
API-key auth modes. Unlike the sibling per-scenario suite, this composes the
stages on one instance to pin the cross-stage state-machine transitions that
broke across stale-token and removal/re-add flows.
@lsm
lsm force-pushed the space/create-provider-auth-lifecycle-regression-matrix branch from c6cf02c to ad9eff5 Compare July 27, 2026 16:50

@lsm lsm left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🤖 Review by glm-5.1 (GLM)

Model: glm-5.1 | Client: NeoKai | Provider: GLM (Zhipu)

Recommendation: APPROVE — confirmed after investigating the red 1-core gate.

Follow-up on the CI note. I verified empirically that the failing Daemon Unit Tests (1-core) job is a pre-existing dev-branch flake, not a regression from this PR:

  • Same failure on dev without this change: dev push run 30284369448 (conclusion: failure) fails at the identical location — provider-service.test.ts:1379, getProviderService > should return ProviderService instance, expect(typeof service.getDefaultProvider).toBe('function') → not a function. Same file, same line, same assertion, no PR diff present.
  • The test itself documents the fragility: lines 1377–1378 note that instanceof "breaks when Bun loads the class from a different module instance" — this is a Bun-on-Linux module-instance loading race, not logic this PR touches.
  • No contamination from the new file: local co-run of provider-auth-lifecycle-matrix.test.ts + provider-service.test.ts in one process → 97 pass / 0 fail. (The flake is Linux-CI-specific and doesn't reproduce on macOS.)
  • This PR's own gates are green in CI: Lint, Knip, Format & Type Check passes; the new suite is 19/19 in isolation and 10487/10488 in the shard (the 1 being the pre-existing flake). 4-space-migrations-a is green in the latest run (the earlier timeout cleared on rerun, as reported).

The matrix diff remains approved on its merits. Filing this so the red All Tests Pass meta-gate isn't mistaken for a regression when this is reviewed for merge — the underlying flake is a dev-wide issue worth a separate fix.

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