[jatmn]

Summary

• TA-001 round 2, ProviderManager slice. One test file changed: src/utils/githubModelsCredentials.hydrate.test.ts.
• Root cause: the hydrate tests call mock.module('./secureStorage/index.js', ...) with a factory that returns only read. mock.restore() in afterEach does not undo mock.module registrations, so the last stub stayed registered for the rest of the process. When src/components/ProviderManager.test.tsx ran later in the same process (for example bun test --randomize --seed=2027 without --isolate), getXaiSecureStorage().readAsync was undefined and 74 of its 76 tests failed with TypeError: getXaiSecureStorage().readAsync is not a function.
• Found by bisecting the 418 files that ran before ProviderManager in the seed-2027 order. The failure reproduces with just this pair: hydrate tests first, then ProviderManager.
• Fix: before any mock is registered, copy the real secureStorage exports from a cache-busted import (?real=<nonce>), and re-register that copy in afterAll. This is the same pattern earlier TA-001 slices used.
• While in the file, the hand-written env save/restore now uses snapshotEnv, as CONTRIBUTING asks. The snapshot is taken after the shared mutation lock is acquired and restored before it is released. The same six variables are covered and none are cleared, so env behavior is unchanged.

Impact

• user-facing impact: none (test-only).
• developer/maintainer impact: removes the largest single source of failures in the randomized no---isolate runs. --isolate stays in CI.

Testing

• I ran the required local preflight.
• exact commands and results (Bun 1.3.13, temp config dirs):
  • bun run typecheck: pass.
  • bun run check: pass.
• focused tests:
  • bun test ./src/utils/githubModelsCredentials.hydrate.test.ts ./src/components/ProviderManager.test.tsx: 79 pass, 0 fail (before the fix: 74 ProviderManager failures).
  • bun test ./src/utils/githubModelsCredentials.hydrate.test.ts: 3 pass.
• no---isolate runs (8 native shards plus --randomize seeds 2027, 101 and 6001): in progress, results will be added here.
• documented skipped checks, platform limitations, or verified pre-existing failures: see above.

Notes

• provider/model path tested: GitHub Models credential hydration and the ProviderManager flows that read xAI secure storage (test-only).
• screenshots attached (if UI changed): n/a
• follow-up work or known limitations: other order-dependent failures in the randomized runs belong to separate TA-001 round 2 slices.

Summary by CodeRabbit

• Tests
  • Improved cleanup of environment variables and secure-storage state in credential hydration tests. This helps keep test runs isolated and reduces the chance that state from one run affects another. There are no user-facing feature or behavior changes in this update.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Source mode: Auto
• Repository: Twigpine/openclaude
• Branch: test/ta001r2-providermanager-leak
• File: .coderabbit.yaml
• Commit: 4afc903
• Layers: Repository YAML
• Review profile: ASSERTIVE
• Plan: Essentials
• Run ID: 54d51d7a-537f-4e8e-b528-8784afec54e2

📥 Commits

Reviewing files that changed from the base of the PR and between 4eae36a and 4afc903.

📒 Files selected for processing: 1

• src/utils/githubModelsCredentials.hydrate.test.ts

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

📜 Recent review details

⏰ Context from checks skipped due to timeout: 19

• GitHub Check: unit-tests (1)
• GitHub Check: unit-tests (5)
• GitHub Check: unit-tests (4)
• GitHub Check: unit-tests (8)
• GitHub Check: unit-tests (7)
• GitHub Check: unit-tests (2)
• GitHub Check: launcher-node-floor
• GitHub Check: unit-tests (6)
• GitHub Check: unit-tests (3)
• GitHub Check: smoke-and-tests
• GitHub Check: launcher-compatibility (22)
• GitHub Check: typecheck
• GitHub Check: dedicated-tests
• GitHub Check: launcher-compatibility (24.11.x)
• GitHub Check: changes
• GitHub Check: CodeQL (actions)
• GitHub Check: pr-intent-scan
• GitHub Check: CodeQL (javascript-typescript)
• GitHub Check: Analyze (javascript-typescript)

🧰 Additional context used

📓 Path-based instructions: 2

Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions.

⚙️ CodeRabbit configuration file

Files:

• src/utils/githubModelsCredentials.hydrate.test.ts

Apply the OpenClaude maintainer review rubric from AGENTS.md.

⚙️ CodeRabbit configuration file

Files:

• src/utils/githubModelsCredentials.hydrate.test.ts

🪛 ast-grep (0.45.3)

src/utils/githubModelsCredentials.hydrate.test.ts

[warning] 13-13: Do not use Math.random() to generate security-sensitive values such as tokens, secrets, passwords, API keys, salts, nonces, OTPs, or session IDs. Math.random() is not cryptographically secure and is predictable. Use crypto.randomBytes()/crypto.randomUUID() (Node) or crypto.getRandomValues() (Web Crypto) instead.
Context: Math.random()
Note: [CWE-330] Use of Insufficiently Random Values.

(insecure-random-security-token-typescript)

🔇 Additional comments: 2

src/utils/githubModelsCredentials.hydrate.test.ts: 2

6-21: LGTM!

38-55: LGTM!

📝 Walkthrough

The hydrate test now restores the real secure-storage module after all tests. It uses snapshotEnv to restore six environment variables after each test.

Changes: Hydrate test isolation

Layer / File(s)	Summary
Test setup and teardown src/utils/githubModelsCredentials.hydrate.test.ts	The test captures the real secure-storage module and restores it after all tests. It replaces manual environment variable cleanup with snapshotEnv and its restore function.

Priority: ⬇️ Low

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

Change: Other

Suggested reviewers: rithulkamesh

Merge Risk: ⚪ Minimal · up to 4afc9

No merge-blocking risk was identified in this test-isolation change.

Pre-merge checks: 6 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 6 passed

Check name	Status	Explanation
Title check		The title is concise, scoped to the test change, and accurately describes restoring real secureStorage exports after the hydrate tests.
Description check		The description covers the change, root cause, impact, testing commands, results, and known limitations. The randomized no---isolate test results are still in progress, but this is a non-blocking ga…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Risk Surface Disclosed		The PR touches an auth-adjacent secure-storage and provider test path, but the diff changes only src/utils/githubModelsCredentials.hydrate.test.ts. The PR description identifies the GitHub Models cr…
No Hidden Policy Change		PASS — The PR changes only src/utils/githubModelsCredentials.hydrate.test.ts. The changes restore mocked secureStorage exports and replace test environment cleanup with snapshotEnv. No product c…

Full details: Description check

Explanation

The description covers the change, root cause, impact, testing commands, results, and known limitations. The randomized no---isolate test results are still in progress, but this is a non-blocking gap because the focused tests and required preflight checks passed.

Full details: Risk Surface Disclosed

Explanation

The PR touches an auth-adjacent secure-storage and provider test path, but the diff changes only src/utils/githubModelsCredentials.hydrate.test.ts. The PR description identifies the GitHub Models credential hydration and ProviderManager/xAI secure-storage risk surface, labels the change test-only, and states that user-facing impact is none. This discloses that the change introduces no product blocker.

Full details: No Hidden Policy Change

Explanation

PASS — The PR changes only src/utils/githubModelsCredentials.hydrate.test.ts. The changes restore mocked secureStorage exports and replace test environment cleanup with snapshotEnv. No product code, routing defaults, telemetry/network behavior, trust model, or permission policy changes are present.

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[jatmn]

— jatmn's PR Review agent

🟢 Thanks for the contribution. I do not see any actionable issues from my review.

Merge readiness

The PR description still lists full no---isolate randomized shard runs as "in progress" while typecheck, bun run check, and the focused hydrate + ProviderManager pair are marked done. CI exercises tests with per-shard --isolate (see scripts/run-tests.ts), so merge is not blocked, but updating the body with the finished randomized-run results (or noting that local verification used the pairwise and seed-2027 commands) would close the evidence gap for maintainers tracking TA-001 round 2.

Branch is current with main (merge-base equals live base; mergeable_state: clean). Required checks on head 4afc9036623dcb37d0ec88eefa7e644f2fcac79c are green, including typecheck, smoke, dedicated tests, all eight unit-test shards, launcher compatibility, CodeQL, and pr-intent-scan.