Skip to content

fix(runtime): emit consistent token_invalidated body on upstream-401 path - #497

Merged
ndycode merged 2 commits into
mainfrom
fix/496-followup-invalidation-contract
May 31, 2026
Merged

fix(runtime): emit consistent token_invalidated body on upstream-401 path#497
ndycode merged 2 commits into
mainfrom
fix/496-followup-invalidation-contract

Conversation

@ndycode

@ndycode ndycode commented May 31, 2026

Copy link
Copy Markdown
Owner

Summary

Follow-up to #496. Addresses the unresolved Greptile review concerns from that PR's post-merge audit.

The core anti-cascade behavior shipped in #496 is correct, but two contract/robustness items remained:

1. Asymmetric 401 body (Greptile P3 → fixed)
The refresh-failure invalidation path synthesised {error:{message,code:"token_invalidated"}}, but the upstream-401 invalidation path forwarded the raw OpenAI body verbatim — which carries no guaranteed code. A client (or operator tooling) keying off error.code to distinguish "explicitly invalidated" from a generic 401 would catch one path and silently miss the other.

  • New buildTokenInvalidationBody() wraps both paths in the same {error:{message,code:"token_invalidated"}} shape.
  • Preserves the upstream human-readable message when present (error.message or top-level message); falls back to a stable message for non-JSON bodies — no HTML/markup is echoed back to clients.
  • Forces content-type: application/json and drops stale content-length/content-encoding (via the existing responseHeadersForClient) since the body is rewritten.

2. Clock-domain guard (Greptile P2 on now() → documented, no behavior change)
applyMonotonicAuthCooldown deliberately uses Date.now() rather than the proxy's injected now(), because it compares against coolingDownUntil, which is written via markAccountCoolingDownnowMs() (== Date.now()). Both sides must share the real-wall-clock domain; switching to the injected now() would mis-compare an injected-clock value against a real-clock deadline and silently defeat the monotonic cooldown race protection. Added a comment so this isn't "fixed" into a regression.

Note on the P2 concurrency race (generic-401 clobbering the 5-min invalidation cooldown): already correctly closed in #496applyMonotonicAuthCooldown only extends, never shortens, and runs synchronously on both auth-failure paths. No code change needed here.

Test plan

  • Strengthened the existing upstream-401 invalidation test to assert error.code === "token_invalidated" and that the upstream message is preserved.
  • Strengthened the HTML-body test to assert the consistent code and the stable fallback message (no markup leaked).
  • tsc --noEmit: clean
  • eslint (ts + scripts): clean
  • npm run build: clean
  • Full suite: 4054 tests pass (vitest run --maxWorkers=1)

🤖 Generated with Claude Code

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

this pr closes the two unresolved contract items from the #496 post-merge audit: it unifies the 401 response body across both token-invalidation vectors (refresh-failure and upstream-401) through a shared buildTokenInvalidationBody() builder, and documents why applyMonotonicAuthCooldown intentionally uses Date.now() rather than the injected clock.

  • buildTokenInvalidationBody() normalises both exit paths to { error: { message, code: "token_invalidated" } }, extracting the upstream human-readable message (top-level or nested error.message) and falling back to a stable string for non-json/html bodies; content-length and content-encoding are stripped by the existing responseHeadersForClient helper before the rewritten body is sent.
  • the refresh-failure path now calls buildTokenInvalidationBody("") instead of inline literals, so the two paths share a single source of truth for both the code string and the fallback message.
  • eight focused unit tests for buildTokenInvalidationBody cover all extraction branches, and the two integration tests are strengthened to assert error.code and the preserved/fallback message.

Confidence Score: 5/5

safe to merge — both invalidation exit paths now share a single builder with consistent output, all extraction branches are unit-tested, and neither the clock-domain guard nor header handling introduce regressions.

the change is narrow and surgical: a pure body-normalisation layer that falls back safely for any upstream input, with the existing responseHeadersForClient correctly stripping stale content-length and content-encoding before the rewritten json body is sent. both previously reported contract gaps are closed, unit coverage now exercises all message-extraction branches, and the two integration tests verify end-to-end shape on both vectors.

no files require special attention

Important Files Changed

Filename Overview
lib/runtime-rotation-proxy.ts adds buildTokenInvalidationBody() builder with correct message extraction and fallback; wires both invalidation exit paths through it; adds a clock-domain comment on applyMonotonicAuthCooldown; no new issues found
test/runtime-rotation-proxy.test.ts strengthens two existing integration tests to assert error.code/error.message; adds a dedicated buildTokenInvalidationBody describe block covering all five extraction branches including the previously untested nested and priority cases

Sequence Diagram

sequenceDiagram
    participant Client
    participant Proxy as RuntimeRotationProxy
    participant OpenAI as Upstream (OpenAI)

    Note over Proxy,OpenAI: Path A — refresh-failure invalidation
    Proxy->>OpenAI: POST /token/refresh
    OpenAI-->>Proxy: 4xx invalidated
    Proxy->>Proxy: buildTokenInvalidationBody("")
    Note right of Proxy: fallback message, code:"token_invalidated"
    Proxy-->>Client: "401 {error:{message,code:"token_invalidated"}}"

    Note over Proxy,OpenAI: Path B — upstream-401 invalidation
    Proxy->>OpenAI: POST /responses
    OpenAI-->>Proxy: "401 {error:{message:"Encountered invalidated…"}}"
    Proxy->>Proxy: isTokenInvalidationError(bodyText)
    Proxy->>Proxy: buildTokenInvalidationBody(bodyText)
    Note right of Proxy: extracts nested error.message,<br/>code:"token_invalidated",<br/>strips content-length/encoding via<br/>responseHeadersForClient
    Proxy-->>Client: "401 {error:{message,code:"token_invalidated"}}"
Loading

Comments Outside Diff (2)

  1. lib/runtime-rotation-proxy.ts, line 1558-1566 (link)

    P2 refresh-failure path still uses hardcoded literals

    TOKEN_INVALIDATED_CODE and TOKEN_INVALIDATED_FALLBACK_MESSAGE are introduced in this PR but never consumed at the refresh-failure exit point (line 1558–1565). if either constant drifts (even a capitalisation fix), the two invalidation paths silently emit different strings again — exactly the asymmetry this PR exists to prevent. the fix is to replace the inline literals with the new constants (or call buildTokenInvalidationBody("") to also go through the shared builder).

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: lib/runtime-rotation-proxy.ts
    Line: 1558-1566
    
    Comment:
    **refresh-failure path still uses hardcoded literals**
    
    `TOKEN_INVALIDATED_CODE` and `TOKEN_INVALIDATED_FALLBACK_MESSAGE` are introduced in this PR but never consumed at the refresh-failure exit point (line 1558–1565). if either constant drifts (even a capitalisation fix), the two invalidation paths silently emit different strings again — exactly the asymmetry this PR exists to prevent. the fix is to replace the inline literals with the new constants (or call `buildTokenInvalidationBody("")` to also go through the shared builder).
    
    How can I resolve this? If you propose a fix, please make it concise.

    Fix in Codex

  2. test/runtime-rotation-proxy.test.ts, line 1857-1870 (link)

    P2 unit coverage gap for buildTokenInvalidationBody branches

    the new function has five distinct message-extraction branches (top-level message, nested error.message, whitespace-only non-empty string, blank/falsy message with populated error.message, non-JSON body). the integration tests added here exercise the top-level-message and non-JSON-fallback paths, but the nested error.message branch and the "top-level wins over nested" priority rule are untested at unit level. if the extraction priority is ever adjusted, no unit test will catch the regression.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: test/runtime-rotation-proxy.test.ts
    Line: 1857-1870
    
    Comment:
    **unit coverage gap for `buildTokenInvalidationBody` branches**
    
    the new function has five distinct message-extraction branches (top-level `message`, nested `error.message`, whitespace-only non-empty string, blank/falsy `message` with populated `error.message`, non-JSON body). the integration tests added here exercise the top-level-message and non-JSON-fallback paths, but the nested `error.message` branch and the "top-level wins over nested" priority rule are untested at unit level. if the extraction priority is ever adjusted, no unit test will catch the regression.
    
    How can I resolve this? If you propose a fix, please make it concise.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

    Fix in Codex

Reviews (2): Last reviewed commit: "refactor(runtime): route refresh-failure..." | Re-trigger Greptile

…path

The upstream-401 invalidation path forwarded the raw OpenAI body, so
clients keying off error.code saw a stable "token_invalidated" code on
the refresh-failure path but not here (Greptile P3 on #496).

- Add buildTokenInvalidationBody(): wraps both paths in the same
  { error: { message, code: "token_invalidated" } } shape, preserving
  the upstream message when present and falling back to a stable message
  for non-JSON bodies (no markup echoed to clients).
- Force content-type: application/json and drop content-length/-encoding
  via responseHeadersForClient since the body is rewritten.
- Document that applyMonotonicAuthCooldown intentionally uses Date.now()
  (not the injected now()) to match the markAccountCoolingDown/nowMs()
  write domain, guarding against a regression that would defeat the
  monotonic cooldown race protection.
- Strengthen existing upstream-401 and html-body tests to assert the
  client contract (code + message).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai

coderabbitai Bot commented May 31, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

the proxy now normalizes upstream 401 token-invalidation responses into a consistent json error contract { error: { message, code: "token_invalidated" } }. it extracts a message from upstream when possible, falls back to a stable message otherwise, and documents that monotonic cooldown checks must use Date.now.

Changes

token invalidation and auth flow updates

Layer / File(s) Summary
monotonic cooldown documentation
lib/runtime-rotation-proxy.ts:669-673
applyMonotonicAuthCooldown docs clarified that comparisons must use Date.now (real wall-clock) not the injected now() because coolingDownUntil is persisted using Date.now.
token invalidation helper and integration
lib/runtime-rotation-proxy.ts:851-885, lib/runtime-rotation-proxy.ts:1559-1562, lib/runtime-rotation-proxy.ts:1781-1787
introduces TOKEN_INVALIDATED_CODE and TOKEN_INVALIDATED_FALLBACK_MESSAGE and exports buildTokenInvalidationBody(upstreamBodyText) that best-effort parses upstream json for message or error.message and returns { error: { message, code: "token_invalidated" } } as a json string. replaces inline hardcoded payloads with buildTokenInvalidationBody("") on refresh failure and replaces raw upstream-body forwarding for 401 with the normalized json and content-type: application/json.
tests: assertions and new helper tests
test/runtime-rotation-proxy.test.ts:7, test/runtime-rotation-proxy.test.ts:1861-1867, test/runtime-rotation-proxy.test.ts:2038-2045, test/runtime-rotation-proxy.test.ts:2048-2103
imports buildTokenInvalidationBody, updates explicit token-invalidated and non-json 401 tests to parse the json response and assert error.code === "token_invalidated" and correct error.message. adds a buildTokenInvalidationBody suite verifying message precedence and fallback behavior.

Sequence Diagram

sequenceDiagram
  participant Upstream
  participant Proxy
  participant Client
  Upstream->>Proxy: 401 (json or non-json body)
  Proxy->>Proxy: buildTokenInvalidationBody(bodyText)
  Proxy->>Proxy: extract top-level message or error.message or use fallback
  Proxy->>Proxy: apply cooldown and clear session affinity
  Proxy->>Client: 401 with application/json and { error: { code: "token_invalidated", message } }
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • ndycode/codex-multi-auth#496: modifies the same lib/runtime-rotation-proxy.ts:1785 upstream 401 token-invalidation handler and related tests.

Suggested labels

bug

review notes:

  • missing regression tests for malformed json and non-string error.message shapes. see lib/runtime-rotation-proxy.ts:851-885 and add tests for malformed upstream bodies and nested non-string fields.
  • windows edge cases: verify any string trimming and newline handling in buildTokenInvalidationBody works for crlf bodies from windows upstreams. tests currently cover html vs json but not crlf variants. see test/runtime-rotation-proxy.test.ts:2038-2045.
  • concurrency risks: ensure cooldown logic change and persisted coolingDownUntil comparisons do not race with other processes that may use injected now(); audit account manager write/read to confirm Date.now is used consistently. see lib/runtime-rotation-proxy.ts:669-673.
🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check name Status Explanation Resolution
Title check ⚠️ Warning Title follows conventional commits format with correct type and scope; however, it exceeds the 72-character limit at 73 characters. Shorten the title to ≤72 characters. Consider: 'fix(runtime): emit consistent token_invalidated body on 401'
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning PR description is comprehensive but omits critical validation checklist items and risk assessment despite extensive changes to token invalidation contract. Add completed validation checkboxes (npm run lint, typecheck, test, build) and fill Risk/Rollback section with actual levels and procedures before merge.
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/496-followup-invalidation-contract
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch fix/496-followup-invalidation-contract

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
lib/runtime-rotation-proxy.ts (1)

1558-1566: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

consider reusing TOKEN_INVALIDATED_FALLBACK_MESSAGE constant here.

the refresh-failure path at lib/runtime-rotation-proxy.ts:1559 hardcodes the same message that's now defined as a constant at line 852. using the constant keeps both invalidation paths in sync if the message ever changes.

♻️ proposed fix
 res.end(
   JSON.stringify({
     error: {
-      message: "OAuth token has been invalidated. Please re-login.",
-      code: "token_invalidated",
+      message: TOKEN_INVALIDATED_FALLBACK_MESSAGE,
+      code: TOKEN_INVALIDATED_CODE,
     },
   }),
 );
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/runtime-rotation-proxy.ts` around lines 1558 - 1566, Replace the
hardcoded invalidation message in the refresh-failure response (the block that
calls res.writeHead/ res.end to return the OAuth token invalidated JSON) with
the existing TOKEN_INVALIDATED_FALLBACK_MESSAGE constant so both invalidation
paths remain in sync; update the JSON construction to use
TOKEN_INVALIDATED_FALLBACK_MESSAGE for the error.message field and keep the code
property "token_invalidated" unchanged.
test/runtime-rotation-proxy.test.ts (1)

1846-1848: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

missing test for top-level message extraction path.

lib/runtime-rotation-proxy.ts:868-870 checks parsed.message before falling through to parsed.error.message. current tests only cover the nested error.message path and the non-json fallback. a regression test with { message: "top-level msg" } (no error wrapper) would cover the branch at line 869.

🧪 suggested test case
it("extracts top-level message from upstream 401 invalidation body", async () => {
  const now = Date.now();
  const accountManager = new AccountManager(undefined, createStorage(now, 2));
  const invalidationBody = JSON.stringify({
    message: "Token has been invalidated by admin",
  });
  const { calls, fetchImpl } = createRecordingFetch(() =>
    new Response(invalidationBody, {
      status: HTTP_STATUS.UNAUTHORIZED,
      headers: { "content-type": "application/json" },
    }),
  );
  const proxy = await startProxy({ accountManager, fetchImpl });

  const response = await postResponses(proxy, { model: "gpt-5-codex" });
  const body = (await response.json()) as { error: { message: string; code: string } };

  expect(response.status).toBe(HTTP_STATUS.UNAUTHORIZED);
  expect(body.error.code).toBe("token_invalidated");
  expect(body.error.message).toBe("Token has been invalidated by admin");
  expect(calls).toHaveLength(1);
});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/runtime-rotation-proxy.test.ts` around lines 1846 - 1848, Add a
regression test in test/runtime-rotation-proxy.test.ts that covers the top-level
parsed.message extraction path (the branch checked in
lib/runtime-rotation-proxy.ts where parsed.message is used before
parsed.error.message). Create an AccountManager and Recording fetch that returns
a JSON body like { message: "Token has been invalidated by admin" } with
HTTP_STATUS.UNAUTHORIZED, start the proxy via startProxy, call postResponses
(model: "gpt-5-codex"), and assert the response status is
HTTP_STATUS.UNAUTHORIZED, that the returned body.error.code equals
"token_invalidated" and body.error.message equals the top-level message, and
that the upstream fetch was called exactly once (use existing helpers
createRecordingFetch, createStorage, AccountManager, startProxy, postResponses,
HTTP_STATUS to mirror other tests).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@lib/runtime-rotation-proxy.ts`:
- Around line 1558-1566: Replace the hardcoded invalidation message in the
refresh-failure response (the block that calls res.writeHead/ res.end to return
the OAuth token invalidated JSON) with the existing
TOKEN_INVALIDATED_FALLBACK_MESSAGE constant so both invalidation paths remain in
sync; update the JSON construction to use TOKEN_INVALIDATED_FALLBACK_MESSAGE for
the error.message field and keep the code property "token_invalidated"
unchanged.

In `@test/runtime-rotation-proxy.test.ts`:
- Around line 1846-1848: Add a regression test in
test/runtime-rotation-proxy.test.ts that covers the top-level parsed.message
extraction path (the branch checked in lib/runtime-rotation-proxy.ts where
parsed.message is used before parsed.error.message). Create an AccountManager
and Recording fetch that returns a JSON body like { message: "Token has been
invalidated by admin" } with HTTP_STATUS.UNAUTHORIZED, start the proxy via
startProxy, call postResponses (model: "gpt-5-codex"), and assert the response
status is HTTP_STATUS.UNAUTHORIZED, that the returned body.error.code equals
"token_invalidated" and body.error.message equals the top-level message, and
that the upstream fetch was called exactly once (use existing helpers
createRecordingFetch, createStorage, AccountManager, startProxy, postResponses,
HTTP_STATUS to mirror other tests).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 28c3c90f-694c-436e-a500-283957547e1e

📥 Commits

Reviewing files that changed from the base of the PR and between 4569197 and 3180f01.

📒 Files selected for processing (2)
  • lib/runtime-rotation-proxy.ts
  • test/runtime-rotation-proxy.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{ts,tsx,js,mjs}

📄 CodeRabbit inference engine (AGENTS.md)

Use ESM only ("type": "module"), Node >= 18

Files:

  • test/runtime-rotation-proxy.test.ts
  • lib/runtime-rotation-proxy.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Do not use as any, @ts-ignore, or @ts-expect-error TypeScript assertions

Files:

  • test/runtime-rotation-proxy.test.ts
  • lib/runtime-rotation-proxy.ts
{**/scripts/**/*.js,**/test/**/*.ts}

📄 CodeRabbit inference engine (AGENTS.md)

Do not use bare recursive delete logic in Windows-sensitive scripts/tests without retry handling for transient EBUSY/EPERM/ENOTEMPTY errors

Files:

  • test/runtime-rotation-proxy.test.ts
test/**/*.test.ts

📄 CodeRabbit inference engine (test/AGENTS.md)

test/**/*.test.ts: Vitest globals (describe, it, expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
Use removeWithRetry for Windows filesystem cleanup instead of bare fs.rm to handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compiled dist/ files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified in eslint.config.js

Files:

  • test/runtime-rotation-proxy.test.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (README.md)

**/*.{ts,tsx,js,jsx}: Implement default-on runtime Responses rotation for request-bearing forwarded Codex CLI/app sessions, with opt-out support via CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY=0
Store project-scoped accounts under ~/.codex/multi-auth/projects/<project-key>/openai-codex-accounts.json for repo-specific workflows
Support environment variable overrides for configuration including CODEX_MULTI_AUTH_DIR, CODEX_MODE, CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY, CODEX_TUI_COLOR_PROFILE, and others as documented
Implement account health checks with codex-multi-auth check command that validates saved account credentials and state
Implement quota forecasting and budget guards to prevent exhausting account quota within a session
Record local usage in a ledger at ~/.codex/multi-auth/usage/usage-ledger.jsonl for per-project tracking and reporting
Implement bounded outbound request budget so one prompt cannot walk the full account pool indefinitely
Trigger short cooldown instead of continuing aggressive rotation when repeated cross-account 5xx bursts are detected
Stagger proactive token refresh to reduce background refresh bursts across the account pool
Expose recent runtime request metrics in codex-multi-auth status text output and machine-readable metrics in codex-multi-auth report --json
Make OAuth callback listen on port 1455 for login flows
Support device authorization flow via codex-multi-auth login --device-auth as an alternate to browser-based OAuth for headless environments
Implement dashboard hotkeys including Up/Down for navigation, Enter for selection, 1-9 for quick switch, / for search, ? for help, Q to back, S to set account, R to refresh, E to enable/disable, and D to delete
Support named local pool backup export with filename prompt in the Settings > Experimental menu
Implement codex-multi-auth doctor --fix command to diagnose and apply the safest fixes for storage or account state issues
Implement codex-multi-auth fix --dry-run ...

Files:

  • test/runtime-rotation-proxy.test.ts
  • lib/runtime-rotation-proxy.ts
test/**

⚙️ CodeRabbit configuration file

tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.

Files:

  • test/runtime-rotation-proxy.test.ts
**/lib/runtime-rotation-proxy.ts

📄 CodeRabbit inference engine (AGENTS.md)

**/lib/runtime-rotation-proxy.ts: Keep runtime rotation default-on behavior aligned with explicit release and migration documentation
Do not expose account emails or tokens in runtime proxy client response headers or logs

Files:

  • lib/runtime-rotation-proxy.ts
lib/**/*.ts

📄 CodeRabbit inference engine (lib/AGENTS.md)

lib/**/*.ts: All public exports should flow through lib/index.ts or documented package subpaths
Never import from dist/ in source tests or library code
Never suppress type errors

Files:

  • lib/runtime-rotation-proxy.ts
lib/runtime-rotation-proxy.ts

📄 CodeRabbit inference engine (lib/AGENTS.md)

lib/runtime-rotation-proxy.ts: Runtime rotation code must preserve pass-through semantics except for auth/provider headers that intentionally change
Runtime proxy client-facing headers must not expose account emails or tokens
Runtime rotation should fail open to normal official Codex forwarding when startup helpers are unavailable
Never add account emails/tokens to runtime proxy client responses

Files:

  • lib/runtime-rotation-proxy.ts
lib/**

⚙️ CodeRabbit configuration file

focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.

Files:

  • lib/runtime-rotation-proxy.ts
🔇 Additional comments (4)
lib/runtime-rotation-proxy.ts (3)

669-677: LGTM!


851-884: LGTM!


1785-1791: LGTM!

test/runtime-rotation-proxy.test.ts (1)

1860-1866: LGTM!

Also applies to: 2037-2041

…ion builder + unit-test branches

Addresses two Greptile P2 comments on this PR:

- The refresh-failure invalidation exit still hardcoded the body literals
  the new constants were meant to centralise, reopening the exact drift
  risk this PR closes. Route it through buildTokenInvalidationBody(""),
  which yields the same { error: { message: <fallback>, code:
  "token_invalidated" } } so both paths can no longer diverge.
- Export buildTokenInvalidationBody and add 8 unit tests covering all
  message-extraction branches: empty input, top-level message, nested
  error.message, top-level-wins priority, blank-top-level -> nested
  fallback, non-JSON body, and no-usable-message fallback.

Full suite: 4062 tests pass (4054 + 8). typecheck/lint/build clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@ndycode

ndycode commented May 31, 2026

Copy link
Copy Markdown
Owner Author

Addressed both P2 comments in ddfee84:

  1. Refresh-failure path drift — the refresh-failure exit (lib/runtime-rotation-proxy.ts) now calls buildTokenInvalidationBody("") instead of inlining the literals, so both invalidation paths share one source of truth and can't diverge if the constants change.
  2. Unit-coverage gap — exported buildTokenInvalidationBody and added 8 unit tests covering every message-extraction branch: empty input, top-level message, nested error.message, top-level-wins priority, blank-top-level → nested fallback, non-JSON body, and no-usable-message fallback.

Full suite now 4062 passing (4054 + 8 new); typecheck/lint/build clean.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
test/runtime-rotation-proxy.test.ts (1)

1844-2045: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

add a regression for the rewritten 401 headers.

lib/runtime-rotation-proxy.ts:1784-1787 now rewrites both headers and body, but test/runtime-rotation-proxy.test.ts:1844-2045 only asserts the json payload. please also assert content-type is forced to json and stale content-length/content-encoding are absent, so this contract does not regress silently.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/runtime-rotation-proxy.test.ts` around lines 1844 - 2045, The tests that
validate rewritten 401 responses (e.g., the "returns 401 to client and does not
rotate when upstream explicitly invalidates the token" case and similar 401
cases) only assert the JSON payload but not the rewritten headers; since
lib/runtime-rotation-proxy.ts at the rewrite block (around the header/body
rewrite code) now forces JSON and strips stale headers, update these tests
(locate the response returned from postResponses(proxy, ...)) to also assert
response.headers.get("content-type") === "application/json" and that
response.headers.get("content-length") and
response.headers.get("content-encoding") are null/absent so the header-rewrite
contract is covered. Ensure you add the same assertions in the other 401-related
tests in this diff that expect rewritten 401 bodies.
lib/runtime-rotation-proxy.ts (1)

861-884: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

sanitize json-carried invalidation messages before echoing them.

lib/runtime-rotation-proxy.ts:868-875 still reflects any string found in message or error.message verbatim. if upstream sends json with an html fragment in that field, this helper will return it unchanged, which breaks the "no markup echoed" contract on the rewritten 401 path. fall back to the stable message for markup-like content here, and add a vitest regression near test/runtime-rotation-proxy.test.ts:2064-2102.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/runtime-rotation-proxy.ts` around lines 861 - 884, The helper
buildTokenInvalidationBody currently trusts JSON fields verbatim; update it to
reject markup-like messages by validating any extracted direct or nested message
(from parsed.message or parsed.error.message) for HTML/markup (e.g., presence of
'<'/'>' or a tag-like pattern like /<[^>]+>/ or common doctype/script fragments)
and only accept it if it’s plain text; otherwise keep
TOKEN_INVALIDATED_FALLBACK_MESSAGE. Ensure the returned JSON still uses
TOKEN_INVALIDATED_CODE. Add a vitest regression near
test/runtime-rotation-proxy.test.ts that sends upstream JSON with an HTML
fragment in message/error.message and asserts the response body uses
TOKEN_INVALIDATED_FALLBACK_MESSAGE (not the markup).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@lib/runtime-rotation-proxy.ts`:
- Around line 861-884: The helper buildTokenInvalidationBody currently trusts
JSON fields verbatim; update it to reject markup-like messages by validating any
extracted direct or nested message (from parsed.message or parsed.error.message)
for HTML/markup (e.g., presence of '<'/'>' or a tag-like pattern like /<[^>]+>/
or common doctype/script fragments) and only accept it if it’s plain text;
otherwise keep TOKEN_INVALIDATED_FALLBACK_MESSAGE. Ensure the returned JSON
still uses TOKEN_INVALIDATED_CODE. Add a vitest regression near
test/runtime-rotation-proxy.test.ts that sends upstream JSON with an HTML
fragment in message/error.message and asserts the response body uses
TOKEN_INVALIDATED_FALLBACK_MESSAGE (not the markup).

In `@test/runtime-rotation-proxy.test.ts`:
- Around line 1844-2045: The tests that validate rewritten 401 responses (e.g.,
the "returns 401 to client and does not rotate when upstream explicitly
invalidates the token" case and similar 401 cases) only assert the JSON payload
but not the rewritten headers; since lib/runtime-rotation-proxy.ts at the
rewrite block (around the header/body rewrite code) now forces JSON and strips
stale headers, update these tests (locate the response returned from
postResponses(proxy, ...)) to also assert response.headers.get("content-type")
=== "application/json" and that response.headers.get("content-length") and
response.headers.get("content-encoding") are null/absent so the header-rewrite
contract is covered. Ensure you add the same assertions in the other 401-related
tests in this diff that expect rewritten 401 bodies.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 18e3dc02-7892-4d7b-860b-9f84d94ef5d0

📥 Commits

Reviewing files that changed from the base of the PR and between 3180f01 and ddfee84.

📒 Files selected for processing (2)
  • lib/runtime-rotation-proxy.ts
  • test/runtime-rotation-proxy.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{ts,tsx,js,mjs}

📄 CodeRabbit inference engine (AGENTS.md)

Use ESM only ("type": "module"), Node >= 18

Files:

  • test/runtime-rotation-proxy.test.ts
  • lib/runtime-rotation-proxy.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Do not use as any, @ts-ignore, or @ts-expect-error TypeScript assertions

Files:

  • test/runtime-rotation-proxy.test.ts
  • lib/runtime-rotation-proxy.ts
{**/scripts/**/*.js,**/test/**/*.ts}

📄 CodeRabbit inference engine (AGENTS.md)

Do not use bare recursive delete logic in Windows-sensitive scripts/tests without retry handling for transient EBUSY/EPERM/ENOTEMPTY errors

Files:

  • test/runtime-rotation-proxy.test.ts
test/**/*.test.ts

📄 CodeRabbit inference engine (test/AGENTS.md)

test/**/*.test.ts: Vitest globals (describe, it, expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
Use removeWithRetry for Windows filesystem cleanup instead of bare fs.rm to handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compiled dist/ files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified in eslint.config.js

Files:

  • test/runtime-rotation-proxy.test.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (README.md)

**/*.{ts,tsx,js,jsx}: Implement default-on runtime Responses rotation for request-bearing forwarded Codex CLI/app sessions, with opt-out support via CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY=0
Store project-scoped accounts under ~/.codex/multi-auth/projects/<project-key>/openai-codex-accounts.json for repo-specific workflows
Support environment variable overrides for configuration including CODEX_MULTI_AUTH_DIR, CODEX_MODE, CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY, CODEX_TUI_COLOR_PROFILE, and others as documented
Implement account health checks with codex-multi-auth check command that validates saved account credentials and state
Implement quota forecasting and budget guards to prevent exhausting account quota within a session
Record local usage in a ledger at ~/.codex/multi-auth/usage/usage-ledger.jsonl for per-project tracking and reporting
Implement bounded outbound request budget so one prompt cannot walk the full account pool indefinitely
Trigger short cooldown instead of continuing aggressive rotation when repeated cross-account 5xx bursts are detected
Stagger proactive token refresh to reduce background refresh bursts across the account pool
Expose recent runtime request metrics in codex-multi-auth status text output and machine-readable metrics in codex-multi-auth report --json
Make OAuth callback listen on port 1455 for login flows
Support device authorization flow via codex-multi-auth login --device-auth as an alternate to browser-based OAuth for headless environments
Implement dashboard hotkeys including Up/Down for navigation, Enter for selection, 1-9 for quick switch, / for search, ? for help, Q to back, S to set account, R to refresh, E to enable/disable, and D to delete
Support named local pool backup export with filename prompt in the Settings > Experimental menu
Implement codex-multi-auth doctor --fix command to diagnose and apply the safest fixes for storage or account state issues
Implement codex-multi-auth fix --dry-run ...

Files:

  • test/runtime-rotation-proxy.test.ts
  • lib/runtime-rotation-proxy.ts
test/**

⚙️ CodeRabbit configuration file

tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.

Files:

  • test/runtime-rotation-proxy.test.ts
**/lib/runtime-rotation-proxy.ts

📄 CodeRabbit inference engine (AGENTS.md)

**/lib/runtime-rotation-proxy.ts: Keep runtime rotation default-on behavior aligned with explicit release and migration documentation
Do not expose account emails or tokens in runtime proxy client response headers or logs

Files:

  • lib/runtime-rotation-proxy.ts
lib/**/*.ts

📄 CodeRabbit inference engine (lib/AGENTS.md)

lib/**/*.ts: All public exports should flow through lib/index.ts or documented package subpaths
Never import from dist/ in source tests or library code
Never suppress type errors

Files:

  • lib/runtime-rotation-proxy.ts
lib/runtime-rotation-proxy.ts

📄 CodeRabbit inference engine (lib/AGENTS.md)

lib/runtime-rotation-proxy.ts: Runtime rotation code must preserve pass-through semantics except for auth/provider headers that intentionally change
Runtime proxy client-facing headers must not expose account emails or tokens
Runtime rotation should fail open to normal official Codex forwarding when startup helpers are unavailable
Never add account emails/tokens to runtime proxy client responses

Files:

  • lib/runtime-rotation-proxy.ts
lib/**

⚙️ CodeRabbit configuration file

focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.

Files:

  • lib/runtime-rotation-proxy.ts

@ndycode
ndycode merged commit f7f5932 into main May 31, 2026
2 checks passed
ndycode added a commit that referenced this pull request May 31, 2026
Prerelease that ships the cascade OAuth token-invalidation fix from issue
#495 to npm under the `beta` dist-tag: the 401 handler now detects explicit
token-invalidation responses and returns them to the client instead of
rotating through every account (the rotation itself was tripping OpenAI's
anti-abuse detection and invalidating accounts in sequence). Invalidated
accounts get a monotonic 5-minute cooldown, session affinity is cleared, and
both invalidation exit paths emit a consistent token_invalidated error body.
Also adds a configurable minRotationIntervalMs sticky window.

Carries forward multi-workspace support (beta.1) and the pinned-account 503
diagnostic (beta.0). Stable v2.1.13 will land once the issue #486 root cause
is identified and patched.

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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