fix(auth): remove deprecated OpenAuth dependency - #661
Conversation
📝 Walkthroughthis is a minor-risk dependency removal. reviewers should focus on the local PKCE implementation and compatibility across supported runtimes and windows environments. concurrent authorization flows should also be checked for independent verifier state. validation includes 64 auth tests, linting, type checking, package checks, and 5,326 passing tests in the full suite. 28 unrelated platform, wrapper, and storage tests failed. changes
risks
Walkthroughthe authorization flow now generates pkce values locally with Changeslocal pkce localization
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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. Comment |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/auth.test.ts`:
- Around line 218-224: Extend the PKCE verifier assertions in the relevant auth
test to validate that flow.pkce.verifier contains only the unpadded base64url
alphabet generated by the auth implementation, rejecting +, /, and = characters.
Keep the existing deterministic length and SHA-256 challenge assertions
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c7022269-4453-44e5-88bf-80352890f73a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
lib/auth/auth.tspackage.jsontest/auth.test.ts
💤 Files with no reviewable changes (1)
- package.json
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (10)
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Write Vitest test suites with globals enabled (describe, it, expect)
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, and ENOTEMPTY errors
Do not rely on dist/ in tests; use source files instead
Do not skip tests without justification
Relax lint rules for test files as configured in eslint.config.js
Files:
test/auth.test.ts
test/**/auth.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test OAuth PKCE flow and JWT decoding in auth.test.ts
Files:
test/auth.test.ts
**/*.{ts,js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,js,mjs}: Use ESM modules throughout the project; the package is configured with"type": "module".
Do not useas any,@ts-ignore, or@ts-expect-error.
Files:
test/auth.test.tslib/auth/auth.ts
test/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Windows-sensitive filesystem tests and helpers must use retry handling for transient lock-related cleanup and write failures.
Files:
test/auth.test.ts
**/*
📄 CodeRabbit inference engine (AGENTS.md)
Source changes belong in
index.ts,lib/, andscripts/;dist/is generated output and local temporary/cache directories must not be edited.
Files:
test/auth.test.tslib/auth/auth.ts
**/*.{js,ts,mjs,cjs}
📄 CodeRabbit inference engine (README.md)
**/*.{js,ts,mjs,cjs}: Do not publish or replace a globalcodexbinary; official OpenAI installation paths must retain ownership of thecodexcommand.
Keep OAuth credentials local and restrict runtime rotation and local bridges to loopback interfaces.
Require hashed local client tokens to protect the optional loopback bridge.
Responsesbackground: truecompatibility must remain opt-in; requests using it must use statefulstore=truerouting rather than statelessstore=falserouting.
Never run npm install or update commands automatically; only display a manual upgrade notice when appropriate.
Experimental synchronization and backup flows must be non-destructive by default: preview before applying sync, preserve destination-only accounts, and fail safely on backup filename collisions.
Keep account storage project-scoped under the configured multi-auth root when operating in repo-specific workflows.
Files:
test/auth.test.tslib/auth/auth.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/auth.test.ts
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: Route all public exports throughlib/index.tsor documented package subpaths.
Keep module dependencies acyclic and preserve the layeringtypes/constants → storage → accounts → runtime → manager/CLI; lower layers must not import higher layers.
Preserve runtime rotation pass-through semantics except for intentionally changed auth or provider headers.
Deduplicate emails usingnormalizeEmailKey(), which trims and lowercases the email.
Use classes for state requiring multiple independent instances or dependency injection, includingAccountManager,CircuitBreaker,SessionAffinityStore, and theCodexErrorhierarchy. Reserve module-level state for genuinely process-global concerns and provide a test reset helper for such state.
Never import fromdist/in source tests or library code.
Never suppress type errors.
Never patch official Codex application binaries for desktop routing.
Never use bare recursive cleanup in Windows-sensitive paths without retry handling.
Files:
lib/auth/auth.ts
lib/auth/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Never hardcode OAuth ports; use the existing authentication constants and helpers.
Do not hardcode the OAuth callback port; use existing constants or helpers. The callback port is 1455.
Files:
lib/auth/auth.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/auth/auth.ts
🧠 Learnings (2)
📚 Learning: 2026-06-04T06:14:18.093Z
Learnt from: ndycode
Repo: ndycode/codex-multi-auth PR: 510
File: test/scheduling-strategy-config.test.ts:1-1
Timestamp: 2026-06-04T06:14:18.093Z
Learning: In ndycode/codex-multi-auth, do not flag explicit imports from "vitest" (e.g., describe, it, expect, beforeEach/afterEach, etc.) in test files as issues—even if the Vitest config sets `globals: true`. The repo’s established convention is to keep these imports for consistency with neighboring tests; removing them would make files outliers.
Applied to files:
test/auth.test.ts
📚 Learning: 2026-06-04T06:14:24.975Z
Learnt from: ndycode
Repo: ndycode/codex-multi-auth PR: 510
File: test/runtime-rotation-proxy.test.ts:2478-2491
Timestamp: 2026-06-04T06:14:24.975Z
Learning: In ndycode/codex-multi-auth test files (e.g. `test/*.test.ts`), when creating V3 storage fixtures for accounts, it’s an intentional convention to use `as never` for deliberately minimal stored-account objects that only include `refreshToken`, `addedAt`, and `lastUsed`. Do not treat `as never` here as a type-safety problem: optional/other fields are expected to be populated by the runtime during execution, and the cast is used solely to keep the fixture minimal and consistent across existing tests.
Applied to files:
test/auth.test.ts
🔇 Additional comments (2)
lib/auth/auth.ts (1)
1-1: LGTM!Also applies to: 412-421, 430-430
test/auth.test.ts (1)
2-2: LGTM!
Summary
@openauthjs/openauth/pkceusage with Node's built-in crypto APIsMotivation
@openauthjs/openauth@0.4.3declaresarctic ^2.2.2as a peerdependency. npm therefore installs deprecated versions of
arcticand@oslojs/*for every installation of codex-multi-auth.codex-multi-auth uses OpenAuth only for
generatePKCE. Since the packagealready requires Node >=18.17, the same operation can be implemented directly
with
node:cryptowithout carrying the additional dependency chain.The implementation preserves the existing behavior:
Verification
npm ci— clean install without Arctic/Oslo deprecation warningsnpm test -- test/auth.test.ts— 64 tests passednpm run lintnpm run typechecknpx --yes npm@11.6.2 run pack:check— 811,749 bytes across 920 filesThe full suite was also attempted locally on macOS with Node 24.17:
5,326 tests passed and 28 unrelated existing platform/wrapper/storage tests
failed. None of the failing files overlap this change.
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
the pr replaces the deprecated openauth pkce helper with node’s built-in cryptographic apis and removes the obsolete dependency chain.
Confidence Score: 5/5
the pr appears safe to merge.
no blocking failure remains.
Important Files Changed
Reviews (2): Last reviewed commit: "test(auth): assert PKCE verifier alphabe..." | Re-trigger Greptile
Context used: