Skip to content

fix(auth): prioritize exact accountId during credential import - #1656

Closed
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:agent/prioritize-import-account-id
Closed

fix(auth): prioritize exact accountId during credential import#1656
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:agent/prioritize-import-account-id

Conversation

@luvs01

@luvs01 luvs01 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Search the complete stored account set for an exact verified accountId before considering a legacy email-only migration row.
  • Preserve legacy migration only when no stable identity match exists, and keep incoming email-only credentials from overwriting stable-ID rows.
  • Add a mixed-state regression proving an earlier legacy row cannot take an update intended for a later exact-ID row, while the existing active selection remains unchanged.

The previous single-pass predicate performed email fallback while scanning each row. An earlier email-only row could therefore win before a later exact accountId row was examined, leaving the stable row stale and creating duplicate stable identities after the update.

Verification

  • Base: dev at a1e5192b75edbf6dcacae51a30912fab93906f87; exact head: b525150e6f40af7579d68d0c2ff762d02dde414f.
  • Bun 1.3.14: bun test --isolate tests/account-import.test.ts — 19 pass, 0 fail.
  • Bun 1.4.0-canary.1: the same focused command — 19 pass, 0 fail.
  • bun run typecheck passed on both runtimes.
  • bun run privacy:scan and git diff --check passed.
  • Independent scoped security/correctness review found no actionable P0-P2 issue. Exact-head maintained full CI and maintainer security review are still required.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. This restores the existing credential-import identity contract and adds no user-facing command or configuration.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. Independent scoped review found no actionable P0-P2 issue; maintainer security review and sponsorship remain required.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • All CI tests are green on my local testing.

  • I pushed my PR to the latest dev commit.

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

Summary by CodeRabbit

  • Bug Fixes

    • Improved account import handling when legacy email-only records and stable account identities coexist.
    • Re-importing an existing identity now updates the correct account without creating duplicates or altering the active account.
  • Tests

    • Added coverage for atomic identity updates and preservation of legacy account records.

@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 43cc7e3d-bed7-4b2b-99f4-677dc377d343

📥 Commits

Reviewing files that changed from the base of the PR and between c2eb678 and 7192129.

📒 Files selected for processing (2)
  • src/oauth/store.ts
  • tests/account-import.test.ts

📝 Walkthrough

Walkthrough

The OAuth credential upsert logic now uses a reusable email-only matcher after exact accountId matching. A new account import test confirms that stable identity rows update without changing legacy rows or creating duplicates.

Changes

OAuth identity matching

Layer / File(s) Summary
Identity upsert behavior and persistence validation
src/oauth/store.ts, tests/account-import.test.ts
upsertCredentialByIdentity checks exact accountId matches first and uses shared email-only matching for fallback cases. The test confirms that a stable Google identity updates only its matching row, preserves the legacy row and active account, and maintains a single identity row.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 71921

This localized credential-import fix prioritizes exact account identity matches and preserves legacy fallback behavior; no actionable merge-blocking risk remains after normal checks and review.

Possibly related PRs

Suggested reviewers: wibias, lidge-jun, agenthits

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: prioritizing exact accountId matches during credential import.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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 intake: hygiene-blocked Deterministic PR hygiene checks failed bug Something isn't working labels Aug 14, 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/oauth/store.ts.

@luvs01

luvs01 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@github-actions

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • hygiene: unsponsored_surface.

What to do

  • Fix 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/oauth/store.ts.

Review readiness checklist

  • ✅ All CI tests are green on my local testing.
  • ✅ I pushed my PR to the latest dev commit.
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ✅ My PR is ready for review.

4/4 boxes ticked.

This pull request was already a draft. Its draft status will be preserved after every issue above is resolved.

@luvs01
luvs01 force-pushed the agent/prioritize-import-account-id branch from 7192129 to b525150 Compare August 14, 2026 04:06
@luvs01
luvs01 marked this pull request as ready for review August 14, 2026 04:06
@github-actions
github-actions Bot marked this pull request as draft August 14, 2026 04:06

@lidge-jun lidge-jun left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[Repository bug audit · 2026-08-14]

The change is precise and low risk: exact stable accountId matching is evaluated across the full account set before falling back to an email-only legacy row, and the regression protects active-account selection and duplicate identity count.

No code-level blocker found. Rebase onto current dev, resolve the shared OAuth-store overlap with #1663, and rerun account-import/OAuth-store tests plus exact-head CI. Prefer landing this focused identity-selection fix before the broader forced-login preservation change.

@lidge-jun

Copy link
Copy Markdown
Owner

Cherry-picked onto dev as part of the bug resolution campaign (commit-and-merge loop). Changes verified with typecheck and focused tests.

@lidge-jun lidge-jun closed this Aug 14, 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