fix workspace identity follow-ups in quota cleanup

[ndycode]

Summary

• Reuse the production getWorkspaceIdentityKey inside the test/index.test.ts storage mock so the PR no longer reintroduces identity drift in tests.
• Replace the stale flagged-storage dedup comment with the current shared-key explanation and keep flagged normalization on the same workspace identity key used by active cleanup.
• Add direct test/storage.test.ts coverage for organization, account-only, and refresh-token fallback identity-key cases.

Testing

• npm run lint
• npm test -- --run test/storage.test.ts test/index.test.ts
• Not applicable

Notes

• test/storage.test.ts still covers the concurrent flagged-write transaction path.
• This follow-up does not add new logging paths; token redaction behavior is unchanged.

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 consolidates workspace identity logic by moving getWorkspaceIdentityKey from a private index.ts definition into an exported function in lib/storage.ts, then wiring both normalizeFlaggedStorage dedup and the index.ts upsert path through the same shared implementation. the test layer is updated to pull the real function via vi.importActual and the assertion style is migrated from spying on saveFlaggedAccounts to inspecting the in-memory state after withFlaggedAccountStorageTransaction completes.

changes:

• lib/storage.ts: promotes getWorkspaceIdentityKey to a named export; moves normalizeWorkspaceIdentityPart to module scope; normalizeFlaggedStorage drops its local duplicate and the stale "more specific than active storage" comment
• index.ts: removes the private identity-key definition; imports the shared export; no behavioral change to matchesWorkspaceIdentity or upsertFlaggedAccountRecord
• test/index.test.ts: async mock factory with vi.importActual; two assertion sites correctly moved to withFlaggedAccountStorageTransaction + in-memory state; new integration test for org-scoped sibling workspace deactivation
• test/storage.test.ts: new describe("getWorkspaceIdentityKey") unit block covering three branches

issues found:

• the organizationId-only branch (no accountId) of getWorkspaceIdentityKey has no test case in test/storage.test.ts
• the new integration test in test/index.test.ts uses arrayContaining but never asserts that workspace-live is absent from flagged storage — a regression that flags both siblings would still pass
• return { object inside the async mock factory is not indented relative to return, which differs from the rest of the file's style
• windows filesystem / token safety (custom rule): the PR description does not explain how the new shared code path defends against Windows antivirus file-locking during loadFlaggedAccountsUnlocked, nor does it call out a regression test that exercises the concurrent flagged-write path under contention. the PR notes point to existing coverage in test/storage.test.ts, but that should be explicitly stated in the description per project convention

Confidence Score: 4/5

• safe to merge after addressing the missing organizationId-only test branch and adding a negative assertion in the new integration test; no behavioral regression introduced
• the production logic change is a pure deduplication (identical implementations unified under one export), so the risk of a behavioral regression is low. test coverage gaps are minor — the missing branch and negative assertion reduce confidence slightly, and the PR description omits the required windows FS concurrency and token-redaction justification called for by project convention
• test/storage.test.ts (missing organizationId-only branch), test/index.test.ts (missing negative assertion + indentation in mock factory)

Important Files Changed

Filename	Overview
lib/storage.ts	promotes getWorkspaceIdentityKey to a module-level export and unifies flagged-storage dedup with active-cleanup identity logic; normalizeFlaggedStorage drops its local duplicate and calls the shared function; logic is functionally identical to the removed code
index.ts	removes the private getWorkspaceIdentityKey definition and imports the now-exported version from lib/storage.ts; matchesWorkspaceIdentity and upsertFlaggedAccountRecord continue to call the same logic unchanged
test/index.test.ts	converts vi.mock factory to async to import the real getWorkspaceIdentityKey; migrates two assertion sites from saveFlaggedAccounts spy to withFlaggedAccountStorageTransaction + in-memory state; adds a new integration test for org-scoped sibling workspace cleanup — missing a negative assertion that workspace-live stays out of flagged storage, and the return-object indentation inside the async factory is misaligned
test/storage.test.ts	adds direct unit coverage for getWorkspaceIdentityKey — three of four branches exercised; the organizationId-only (no accountId) branch is untested

Sequence Diagram

sequenceDiagram
    participant idx as index.ts (quota cleanup)
    participant storage as lib/storage.ts
    participant fs as Flagged-Accounts JSON

    idx->>storage: withFlaggedAccountStorageTransaction(cb)
    storage->>fs: readFile (locked via mutex)
    fs-->>storage: raw JSON
    storage->>storage: normalizeFlaggedStorage(data)
    storage->>storage: getWorkspaceIdentityKey(account) [shared]
    Note over storage: dedup by identity key<br/>(org+account | account | refreshToken)
    storage-->>idx: cb(loadedStorage, persist)
    idx->>idx: upsertFlaggedAccountRecord(accounts, deadRecord)
    idx->>idx: getWorkspaceIdentityKey(record) [same fn]
    idx->>storage: persist(nextStorage)
    storage->>fs: writeFile (atomic via mutex)

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: test/index.test.ts
Line: 3744-3758

Comment:
**No negative assertion for `workspace-live` in flagged storage**

the test confirms `workspace-dead` was flagged but never asserts that `workspace-live` was NOT added to flagged storage. a regression where both sibling workspaces get flagged would pass all current assertions — `arrayContaining` only requires the listed entry to be present, not that the array is exactly that size.

add:
```ts
expect(
  mockFlaggedStorage.accounts.some((a) => a.accountId === "workspace-live"),
).toBe(false);
```
after the existing `arrayContaining` check.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: test/index.test.ts
Line: 278-342

Comment:
**Mock return-object indentation is misaligned**

`return {` sits at one tab, but the object properties (`getStoragePath`, `loadAccounts`, …) are also at one tab — same level as the keyword, not indented inside the object. while syntactically valid, it's visually misleading and inconsistent with every other `vi.mock` in this file. the closing `};` is also at the same level, making the block look like two separate statements at a glance.

indent all properties one level further than `return {`:
```ts
	return {
		getStoragePath: () => "/mock/path/accounts.json",
		loadAccounts: vi.fn(async () => cloneMockStorage()),
		// ... rest of properties indented one more level
	};
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: test/storage.test.ts
Line: 33-64

Comment:
**Missing `organizationId`-only branch test**

the new `getWorkspaceIdentityKey` describe block covers three of four branches, but the case where `organizationId` is present and `accountId` is absent (returns `organizationId:${org}` with no accountId suffix) has no test. this second branch of the inner ternary is reachable in practice when an org-scoped token has not yet resolved a workspace `accountId`.

add a fourth case:
```ts
it("uses organizationId alone when accountId is absent", () => {
  expect(
    getWorkspaceIdentityKey({
      organizationId: " org-shared ",
      refreshToken: "tok",
    }),
  ).toBe("organizationId:org-shared");
});
```

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: "test: close pr86 gre..."}

Context used:

• Rule used - What: Every code change must explain how it defend... (source)

[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]

Warning

Rate limit exceeded

@ndycode has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 5 minutes and 18 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8436d55a-61a2-4791-aac7-d55aeeb2c2c7

📥 Commits

Reviewing files that changed from the base of the PR and between 2175fe2 and 9a7cbec.

📒 Files selected for processing (4)

• index.ts
• lib/storage.ts
• test/index.test.ts
• test/storage.test.ts

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/pr85-greptile-followups

📝 Coding Plan

• Generate coding plan for human review comments

---

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.}

[ndycode]

@greptile @coderabbitai review

[coderabbitai]

@ndycode Sure, I'll kick off a review of the PR right away!

✅ Actions performed

Review triggered.

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.