fix: deduplicateAccounts fixpoint loop and identity property tests

[ndycode]

Summary

• A new fast-check property suite for the account identity-matching layer found a real bug on its 11th generated pool: deduplicateAccounts was not idempotent — one pass could return a list still containing two records for the same account. This PR fixes the implementation and lands the suite that caught it.

What Changed

The bug (lib/storage.ts): deduplicateAccountsByIdentity made a single pass, and a newest-wins merge in one matching tier can install an account that duplicates an earlier survivor through a different tier. Minimal reproduction (now a deterministic test):

1. {accountId: acc-1, refreshToken: rt-1} (no email) is kept.
2. {email: carol@example.com, refreshToken: rt-2} (no accountId) is kept.
3. {accountId: acc-1, email: carol@example.com, refreshToken: rt-1} arrives — the email tier matches record 2 first and replaces that slot.

The result keeps both record 1 and record 3, which are the same account by accountId + refresh token; a second pass would have merged them. The fix: re-run the dedup pass until the length stabilizes. Every merge strictly shrinks the array, so the loop terminates after at most accounts.length passes; the kept entry is still the most-recently-used one, exactly as the function's contract documents.

The suite (test/property/account-identity.property.test.ts, 9 tests):

• normalizeEmailKey: idempotence, case/whitespace insensitivity, undefined on blank input
• findMatchingAccountIndex: the matched account is invariant under pool permutation (pools generated with strictly distinct lastUsed so newest-wins has a unique answer); candidate email respelling never changes the result; any match shares at least one identity facet with the candidate; candidates with all-foreign facets never match; email-only candidates are vetoed when the email maps to multiple accountIds
• deduplicateAccounts: output is a subset of input (object identity — dedup never synthesizes), idempotence, and a deterministic replay of the discovered counterexample

Validation

• npm run typecheck
• npx eslint lib/storage.ts test/property/account-identity.property.test.ts --max-warnings=0
• npm test -- test/property/account-identity.property.test.ts — 9/9
• Targeted suites for every consumer of the matching/dedup path (storage, storage-async, import-export, account-pool, account-port, rotation-integration, refresh-guardian, runtime-auth-facade, codex-manager-login-workspace-512): only the 23 known environment failures in storage.test.ts, name-diffed exactly against docs/audits/evidence/test-baseline-2026-06-10.txt
• Full-suite canary: 60 failures = the 58-name environment baseline + the 2 beta.2 version-drift names that fix: sync plugin manifest and docs portal to v2.3.0-beta.2 #577 fixes on main (this branch is based on main without it); zero failures attributable to this change

Docs and Governance Checklist

• No user-visible command/setting/path surface changed; behavior change is strictly "dedup now actually removes all duplicates", matching the existing doc comment ("Removes duplicate accounts, keeping the most recently used entry")

Risk and Rollback

• Risk level: low-moderate — deduplicateAccounts runs on the storage load path, so the fix can merge account records that previously (incorrectly) survived as duplicates. The kept record is always the most-recently-used of the merged set, which is the documented contract; pools without the multi-tier corner are byte-identical to before (the first pass is unchanged, and a length-stable pass is a pure copy).
• Rollback plan: revert the commit; the property suite's idempotence assertion and the deterministic replay will fail again, flagging the regression.

Additional Notes

• Companion to the property suites in test: property-check the unsupported-model fallback invariants #574/test: property-check write-queue serialization and clamp invariants #575; same conventions (plain fc.assert, explicit vitest imports, small identity alphabets so generated pools actually collide on facets).

https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB

---

Generated by 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

fixes deduplicateAccountsByIdentity so it re-runs until the output length stabilises, addressing a real idempotence failure where a newest-wins email-tier merge could leave an earlier survivor still matching a second record through a different tier. the property suite that caught the bug is also landed, including a new deterministic 3-shrinking-pass test that pins the full convergence chain.

• lib/storage.ts: split the inner loop into deduplicateAccountsByIdentityPass (readonly T[]), wrapped by a new deduplicateAccountsByIdentity that loops until next.length === current.length; the length check is a sound fixpoint condition because every merge reduces the count by exactly one, so length-stable ↔ zero merges ↔ content-stable.
• test/property/account-identity.property.test.ts: 9 fast-check/deterministic tests for normalizeEmailKey, findMatchingAccountIndex, and deduplicateAccounts, including explicit replay of the discovered counterexample and a new 3-pass convergence pin.

Confidence Score: 5/5

the change is safe to merge; the fixpoint loop is a pure in-memory operation on small arrays, the termination argument is airtight, and pools without the multi-tier corner case are byte-identical to the previous output.

the fix is narrowly scoped to deduplicateAccountsByIdentity and does not touch storage I/O, token handling, or any concurrent path. the length-stable fixpoint condition is provably correct, the property suite fully exercises the affected code paths, and the deterministic regression pins the exact counterexample.

no files require special attention; both changed files are clean.

Important Files Changed

Filename	Overview
lib/storage.ts	renamed single-pass helper to deduplicateAccountsByIdentityPass (now takes readonly T[]), then wrapped it in a new deduplicateAccountsByIdentity that loops to the length-stable fixpoint — termination is sound because every merge strictly shrinks the array by exactly one entry
test/property/account-identity.property.test.ts	new fast-check property suite (9 tests) covering normalizeEmailKey idempotence/blank handling, findMatchingAccountIndex pool-permutation invariance/email-respelling/facet-sharing/foreign-facet/ambiguity-veto, and deduplicateAccounts subset-of-input, idempotence, 2-shrinking-pass regression, and a new 3-shrinking-pass deterministic pin

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["deduplicateAccounts(accounts: T[])"] --> B["deduplicateAccountsByIdentity(accounts)"]
    B --> C["current = deduplicateAccountsByIdentityPass(accounts)"]
    C --> D{"next = deduplicateAccountsByIdentityPass(current)\nnext.length === current.length?"}
    D -- "yes (fixpoint reached)" --> E["return next"]
    D -- "no (merged ≥1 account)" --> F["current = next"]
    F --> D
    G["deduplicateAccountsByIdentityPass(accounts: readonly T[])"] --> H["for each account"]
    H --> I{"findMatchingAccountIndex\nin deduplicated?"}
    I -- "no" --> J["push account"]
    I -- "yes" --> K["deduplicated[i] = selectNewestAccount(existing, account)"]
    J --> H
    K --> H

Loading

_{Reviews (2): Last reviewed commit: "test: pin three-pass dedup convergence c..." | Re-trigger Greptile}

[chatgpt-codex-connector]

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]

📝 Walkthrough

Walkthrough

account deduplication now uses fixpoint iteration to eliminate duplicates introduced by newest-wins merges across identity tiers. a single-pass helper repeatedly applies until the array size stabilizes. comprehensive property-based tests verify idempotence, facet matching, and convergence.

Changes

Account Deduplication Fixpoint Convergence

Layer / File(s)	Summary
Deduplication Fixpoint Logic lib/storage.ts:1055-1089	deduplicateAccountsByIdentityPass performs one round of dedup; deduplicateAccountsByIdentity now loops the pass until array length stops changing, guaranteeing convergence after cross-tier merges.
Property-Based Test Suite for Dedup Helpers test/property/account-identity.property.test.ts:1-269	Tests cover normalizeEmailKey, findMatchingAccountIndex, and deduplicateAccounts using small identity alphabets to force collisions. verifies idempotence, pool-order invariance, facet-based matching, fixpoint convergence across chains, and that dedup returns only original accounts.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• ndycode/codex-multi-auth#90: Both PRs modify identity-based account matching in lib/storage.ts—this PR fixes deduplicateAccountsByIdentity convergence for cross-tier duplicates while #90 changes findMatchingAccountIndex behavior.

Suggested labels

bug

---

review flags

the fixpoint loop in lib/storage.ts:1074-1089 looks correct for the stated problem, but flag:

1. no explicit iteration limit: the while loop runs until result.length === accounts.length. safe for deterministic inputs, but if dedup isn't actually idempotent per pass, this could loop forever. the tests do verify idempotence in test/property/account-identity.property.test.ts:194-269, so this should hold, but consider adding a safety counter or explicit assertion that the pass is genuinely convergent.

2. windows locale edge case: email normalization touches case folding and whitespace. test/property/account-identity.property.test.ts:16-48 generates casing/whitespace variants, which is solid, but confirm this works across all locales. the turkish i/I edge case (where locale-aware case folding differs) isn't explicitly tested. verify email normalization is locale-independent or documented as such.

3. missing explicit regression test for the cross-tier chain scenario: the property tests are comprehensive, but the "deterministic counterexamples" mentioned in test/property/account-identity.property.test.ts:194-269 should be spelled out. if you have a concrete case that motivated the fixpoint fix (e.g., account a → b, b → c requiring two passes), add a minimal hardcoded test alongside the property tests so future devs see the exact problem.

4. concurrency risk: the tests are single-threaded. if deduplicateAccountsByIdentity runs concurrently on shared account pools in production, the fixpoint loop is not atomic. concurrent mutations between passes could cause inconsistent dedup. verify the caller doesn't allow concurrent invocations on overlapping pools.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 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.
Title check	✅ Passed	the title follows conventional commits format (fix: summary), is 66 chars (under 72), lowercase imperative, and directly describes the two main changes: the fixpoint loop fix and the property test suite addition.
Description check	✅ Passed	the description covers all required sections: summary explains the bug and fix, what changed details both the bug and the test suite, validation documents extensive testing including targeted suites and full-suite canary, risk/rollback assessed, and docs/governance checklist completed.

_{✏️ 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 claude/audit-60-identity-property-tests

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch claude/audit-60-identity-property-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

• X
• Mastodon
• Reddit
• LinkedIn

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