Skip to content

fix(codex): prevent duplicate pool registration of native main account and fix access token email extraction - #7399

Closed
terrytan95 wants to merge 3 commits into
lidge-jun:devfrom
terrytan95:fix/codex-pool-main-account-collision
Closed

terrytan95 wants to merge 3 commits into
lidge-jun:devfrom
terrytan95:fix/codex-pool-main-account-collision

Conversation

@terrytan95

@terrytan95 terrytan95 commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Reject pool additions and reauthentication that duplicate the native Codex main account by ChatGPT account ID, personal/workspace plan bucket, and email (including missing email). Preserve distinct workspace members, workspace IDs, and personal/workspace subscriptions.
  • Extract and normalize OpenAI namespaced profile emails. Keep missing identity email separate from the display alias until identity and collision checks finish.
  • Fall back to the candidate access-token plan when WHAM fails, times out, or omits its plan; a usable WHAM plan remains authoritative.
  • Add nine login-flow regression cases proving duplicate rejection before warmup/persistence and legitimate account coexistence. Update the account contract and user documentation.

Closes #7398

Verification

Verified commit 306b2e2db131ad1c06b3c44d3e45e314016326e8 locally on macOS:

  • bun test tests/codex-integration/codex-auth-api.test.ts tests/codex-integration/codex-auth-collision.test.ts tests/oauth/chatgpt-oauth.test.ts: 391 passed, 0 failed.
  • bun run test:changed: 34,922 passed, 43 skipped, 0 failed across 1,553 files; comparison merge base d3dafd39e285395ab16a1b0d608ff5dfbc99e096.
  • bun run typecheck: passed.
  • bun test tests/test-layout.test.ts tests/test-layout-tooling.test.ts tests/ci-workflows/file-size-ratchet.test.ts: 36 passed, 0 failed.
  • bun run structure:check and bun run privacy:scan: passed.
  • In docs-site: bun install --frozen-lockfile and bun run build: passed; 585 pages and 83,057 internal links checked. Existing sshconfig syntax-highlighting warnings remain.
  • git diff --check: passed.

The standalone full suite and live OAuth were not run. The import-graph suite provides broad coverage, with the source-reading API and layout/file-size tests also run explicitly. This PR remains Draft pending maintainer security review; local results do not replace required CI.

Current delivery blocker: hygiene and enforce-target report unsponsored_surface. Per MAINTAINERS.md, a maintainer must review the authentication changes and apply maintainer-sponsored. No approval or sponsorship has been self-applied.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults by a maintainer.
  • Required local validation passed; commands, results, and any full-suite exception are documented.
  • I pushed my PR to a recent dev commit (at most 20 behind; a maintainer may still ask for the exact tip before merge).
  • I resolved all correct Codex and CodeRabbit findings.
  • My PR is ready for review.

Summary by CodeRabbit

  • Bug Fixes

    • Codex account registration and reauthentication now detect duplicate logins across the native account and managed accounts. Accounts are compared using account identity, personal or workspace plan, and email; missing email information is not treated as proof that accounts differ.
    • Login now handles email and subscription-plan information more consistently when details are missing or unavailable.
  • Documentation

    • Updated the Codex integration guide to explain duplicate-account checks and supported account distinctions.

@coderabbitai

coderabbitai Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: fd8cde82-9064-4995-ae9a-13119a2930e4
📥 Commits

Reviewing files that changed from the base of the PR and between fc0cd97 and 306b2e2.

📒 Files selected for processing: 6
  • docs-site/src/content/docs/guides/codex-integration.md
  • src/codex/auth-api/login-flow.ts
  • src/codex/auth-collision.ts
  • structure/providers/openai-accounts.md
  • tests/codex-integration/codex-auth-api.test.ts
  • tests/helpers/codex-main-collision.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

📝 Walkthrough

OAuth email extraction now checks and normalizes OpenAI profile emails. Codex login preserves token-derived plan information when WHAM omits it. Collision checks include the native main account and compare account ID, plan classification, and email.

Changes: Native Account Collision

Layer / File(s) Summary
Normalize OAuth profile emails
src/oauth/chatgpt-shared.ts, tests/oauth/chatgpt-oauth.test.ts
extractEmail trims and lowercases nonblank emails. It checks the OpenAI profile claim when the top-level email is missing or blank. Tests cover profile extraction and access-token fallback.
Check native account collisions
src/codex/auth-collision.ts, src/codex/auth-api/login-flow.ts, structure/providers/openai-accounts.md, docs-site/src/content/docs/guides/codex-integration.md
Login derives plan information from the access token and uses a usable WHAM plan when available. Collision checks include the native main account unless __main__ is excluded. A matching account ID collides when plan classifications match and either email is unavailable or both emails match.
Cover collision and login cases
tests/codex-integration/codex-auth-collision.test.ts, tests/helpers/codex-main-collision.ts, tests/codex-integration/codex-auth-api.test.ts
Tests cover matching and excluded native accounts, different account details, and WHAM errors, timeouts, missing plans, and plan overrides. Blocked cases assert that credentials are not added and warmup does not occur.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 306b2

The collision tests exercise the intended candidate identities. No actionable merge-blocking issue remains after normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 7 files. (2 skipped: 2… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title accurately summarizes the two main changes: preventing duplicate native main-account registration and fixing access-token email extraction.
Linked Issues check Passed Issue #7398 has active coding requirements. src/codex/auth-collision.ts updates checkAccountIdCollision to read the native token store, compare ChatGPT account ID and personal-versus-workspace pla…
Out of Scope Changes check Passed The changed source files implement native-main collision detection, OAuth email extraction, and WHAM plan fallback. The changed tests validate those requirements and the required legitimate account co…
Full details: Linked Issues check

Explanation

Issue #7398 has active coding requirements. src/codex/auth-collision.ts updates checkAccountIdCollision to read the native token store, compare ChatGPT account ID and personal-versus-workspace plan class, and reject matching normalized emails when either email is missing or both emails match. The check skips the native account when excludeAccountId is MAIN_CODEX_ACCOUNT_ID. src/oauth/chatgpt-shared.ts reads top-level and OpenAI namespaced profile email claims, trims values, and lowercases them. src/codex/auth-api/login-flow.ts derives the initial plan from the candidate access token, keeps that plan when WHAM fails or omits its plan, and lets a usable WHAM plan override it. It applies the collision check before the later persistence and warmup path, then applies the account-ID email fallback only after the identity checks. The added collision and OAuth tests cover duplicate personal and workspace accounts, missing or blank email, distinct members, distinct workspaces and plan buckets, explicit __main__ exclusion, WHAM failure/timeout/missing-plan cases, and rejection before credential persistence or warmup. The documentation and account contract updates describe the new behavior. These changes satisfy the coding requirements in #7398.

Full details: Out of Scope Changes check

Explanation

The changed source files implement native-main collision detection, OAuth email extraction, and WHAM plan fallback. The changed tests validate those requirements and the required legitimate account coexistence cases. The documentation and account contract changes explain the same authentication behavior. No unrelated product behavior or unrelated cleanup appears in the supplied whole-PR evidence. The reported maintainer sponsorship and security-review status are process items, not out-of-scope code changes.

Full details: Docstring Coverage

Explanation

Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 7 files. (2 skipped: 2 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@github-actions github-actions Bot added the intake: hygiene-blocked Deterministic PR hygiene checks failed label Oct 19, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Deterministic hygiene checks failed.

  • unsponsored_surface — This changes an authentication, workflow, release-automation, or dependency surface. MAINTAINERS.md requires security review for these; ask a maintainer to apply maintainer-sponsored once they have reviewed it. Paths: src/codex/auth-collision.ts, src/oauth/chatgpt-shared.ts.

@github-actions github-actions Bot added the bug Something isn't working label Oct 19, 2026
@github-actions

github-actions Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • hygiene: missing_coauthor_credit.

What to do

  • Fix missing_coauthor_credit — This pull request says it reimplements, supersedes, carries, or rebases another author's pull request, but no Co-authored-by trailer names that author. Prose in a commit body is not read by anything; the trailer is what GitHub counts. Add it to the description or a commit, or obtain attribution-approved. Paths: #7414.
  • Tick all four boxes in the PR description once you're done (currently 3/4).

Review readiness checklist

  • ✅ Required local validation passed; commands, results, and any full-suite exception are documented.
  • ✅ I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ⬜ My PR is ready for review.

3/4 boxes ticked.

This pull request was already a draft. Its draft status will be preserved after every issue above is resolved.
@terrytan95 Tick the boxes once required local validation has passed with commands, results, and any full-suite exception documented, your branch is on the latest dev commit, and every correct Codex and CodeRabbit finding is resolved.

@github-actions
github-actions Bot marked this pull request as draft October 19, 2026 22:00
@terrytan95
terrytan95 marked this pull request as ready for review October 19, 2026 22:07
@github-actions
github-actions Bot marked this pull request as draft October 19, 2026 22:08
@terrytan95
terrytan95 marked this pull request as ready for review October 19, 2026 22:15
@github-actions
github-actions Bot marked this pull request as draft October 19, 2026 22:15
@terrytan95
terrytan95 marked this pull request as ready for review October 19, 2026 22:44
@github-actions
github-actions Bot marked this pull request as draft October 19, 2026 22:44
lidge-jun added a commit that referenced this pull request Oct 19, 2026
…7398) (#7414)

Builds on #7399 by @terrytan95: reject pool registrations that duplicate the native main login (account id + plan bucket + email), read the namespaced OpenAI profile email, keep the JWT plan when WHAM fails, and exclude already-stored duplicates of the observed native main from rotation, pinning and exact routing (dashboard reason duplicate_main).

Closes #7398

Co-authored-by: terrytan95 <10609214+terrytan95@users.noreply.github.com>
@terrytan95
terrytan95 marked this pull request as ready for review October 20, 2026 04:20
@github-actions
github-actions Bot marked this pull request as draft October 20, 2026 04:20
Preserve the implementation and regression coverage carried into dev by PR lidge-jun#7414, including the original PR lidge-jun#7399 fixes and its additional native-main admission and stored-duplicate protections. The resulting tree matches dev at 1202598.
@terrytan95
terrytan95 marked this pull request as ready for review October 20, 2026 04:55
@github-actions
github-actions Bot marked this pull request as draft October 20, 2026 04:55
@Ingwannu

Copy link
Copy Markdown
Owner

Follow-up intake at 17ea2c43041a3907a657405449179c6603f8e6ea against dev 12025981e5a304cdb6b1b54f525ba8554f623977. The live PR API now reports changedFiles=0 against the current dev tip, which is merged #7414. Its original collision/profile-email repair has been absorbed; there is no remaining reviewable diff in this PR. Closing as absorbed by #7414, preserving the contributor branch and existing attribution. Any follow-up not present on dev should be a focused new diff with current-head evidence. This is not another merge or a release claim.

@Ingwannu Ingwannu closed this Oct 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working intake: hygiene-blocked Deterministic PR hygiene checks failed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants