Repository navigation
test: restore leaked provider mocks and Concentrate env in single-process runs - #2328
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📜 Recent review details
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| 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 2 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title is concise, scoped to tests, and accurately describes restoring leaked provider mocks and Concentrate environment handling in single-process runs. Description check The description includes all required sections, explains the changes and rationale, documents impact, provides detailed validation results, and records limitations and follow-up work. 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 PASS. The diff changes only test files, but it does cover provider routing and Concentrate environment handling. The PR description identifies this risk surface, explains the provider-mock and environ… No Hidden Policy Change PASS. The PR changes only src/utils/envFile.test.ts,src/utils/status.test.ts, andsrc/utils/thinking.test.ts. The diff adds test environment cleanup and restores the real provider module after …Full details: Risk Surface Disclosed
-
Explanation
PASS. The diff changes only test files, but it does cover provider routing and Concentrate environment handling. The PR description identifies this risk surface, explains the provider-mock and environment leakage, and states that the change is test-only with no user-facing impact. This discloses that it introduces no blocker.
Full details: No Hidden Policy Change
-
Explanation
PASS. The PR changes only
src/utils/envFile.test.ts,src/utils/status.test.ts, andsrc/utils/thinking.test.ts. The diff adds test environment cleanup and restores the real provider module after test mocks. It changes no product routing, trust, telemetry/network, permission, or production policy code. No maintainer policy alignment is required.
-
✨ 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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
— jatmn's PR Review agent
🟢 Thanks for the contribution. I do not see any actionable issues from my review.
Merge readiness
Rebase onto current main before merge when convenient: the branch is 2 commits behind live main (3a299281bacfb14bcf73a8c7d871477fbe2c6f49). GitHub reports mergeable_state: blocked (required checks / branch policy), not a textual conflict on this diff.
CI at head 834751a2c860ffac73e412d681c141425c8f5661: all required PR checks observed success — PR Checks (typecheck, smoke-and-tests, dedicated-tests, unit-tests shards 1–8, launcher jobs), Code Quality: PR #2328, and CodeQL (CodeQL, CodeQL (javascript-typescript), CodeQL (actions)). web skipped (no web diff). CodeRabbit commit status was still pending during preparation; no inline CodeRabbit review threads exist yet on this head.
Merge-base decision: live-base-three-dot — reviewed 19bb39c8845a6920bb04ad8d70652ad520861b87..834751a2c860ffac73e412d681c141425c8f5661 (one commit).
Scope: Surgical TA-001 slice — three test files only (+18/−2). No production, workflow, or scripts/run-tests.ts changes. Appropriate for upgrade-plan sequencing.
Assessment (requested judgments)
Cache-bust restore pattern: Correct and consistent with the established pattern in domainCheck.test.ts (and siblings such as client.test.ts / compact.test.ts). Top-level import(\./model/providers.js?real=…`)snapshots real exports before any test mocks;afterEachcallsmock.restore()thenmock.module('./model/providers.js', () => realProviders)because Bun keepsmock.moduleregistrations process-wide aftermock.restore(). Spreading into realProvidersis intentional:thinking.test.tsmust notimport('./model/providers.js')after restore (that would re-enter the stale mock). This does not affect production module loading. Under CI--isolate` (per-file processes), the restore is harmless extra hygiene; under single-process local runs it fixes the stated leak class.
Concentrate TEST_ENV_KEYS: Fixes the real leak for the in-file test loads documented Concentrate env-only provider setup values, which sets CONCENTRATE_* via loadEnvFile but previously omitted those keys from TEST_ENV_KEYS, so afterEach could leave them in process.env for later files. The three keys match the documented Concentrate env-only surface and align with other provider triples in the same list. This test does not use rememberLoadedEnvFileValues; clearing process.env is the right boundary.
Root cause claim: Accurate for the two leak classes this PR targets: (1) persistent Bun mock.module on providers.js after status / thinking tests, and (2) Concentrate env vars surviving envFile.test.ts cleanup.
commandOnlySession / “a launch that only runs local commands leaves no transcript”: Not a required finding against this PR. The diff does not touch sessionStorage.commandOnlySession.test.ts. In default path ordering, envFile.test.ts runs before sessionStorage.commandOnlySession.test.ts, which runs before status.test.ts / thinking.test.ts, so provider-mock restores here cannot run before that test. I ran bun test src/utils/envFile.test.ts src/utils/sessionStorage.commandOnlySession.test.ts at both merge-base 19bb39c8 and head 834751a2 — 50 pass / 0 fail both times. Sharded CI (scripts/run-tests.ts --max-concurrency=1) is green at head. The author’s whole-repo single-process tally showing this case among remaining failures is consistent with broader TA-001 order pollution, not causation from this three-file patch; treat it as a follow-up slice, not a regression introduced by these hunks.
Validation run (review checkout, head): bun test src/utils/status.test.ts src/utils/thinking.test.ts src/utils/envFile.test.ts src/utils/sessionStorage.commandOnlySession.test.ts — 69 pass / 0 fail.
Summary
bun testrun.src/utils/status.test.tsandsrc/utils/thinking.test.tsregistermock.module('./model/providers.js', ...), which Bun keeps process-wide;mock.restore()does not unregister it. Both files now take a cache-busted snapshot of the real module before mocking and re-register it right aftermock.restore()in their existingafterEach(same pattern assrc/tools/WebFetchTool/domainCheck.test.ts).thinking.test.tsbuilds its override from that snapshot instead of the live namespace.src/utils/envFile.test.tsloadsCONCENTRATE_API_KEY,CONCENTRATE_BASE_URLandCONCENTRATE_MODELfrom a fixture but did not list them inTEST_ENV_KEYS, so they stayed inprocess.envand later files routed to Concentrate. They are now in the list its hooks already clear and restore.src/utils/http.test.ts("WebFetch preserves the Anthropic support URL for first-party endpoints") fail when run after them in one process:getAPIProvider()returnedopenai, soisFirstPartyAnthropicProvider()was false.Impact
scripts/run-tests.tschange;--isolatestays in CI until all TA-001 slices land.Testing
bun run check: passbun run typecheck: passbun run security:pr-scan -- --base origin/main --head HEAD: no suspicious additionsgit diff --check: clean--isolate):bun test src/utils/status.test.ts src/utils/http.test.ts14 pass / 1 fail;bun test src/utils/thinking.test.ts src/utils/http.test.ts7 pass / 1 failbun run scripts/run-tests.ts --feature=UNATTENDED_RETRY --max-concurrency=1 src/utils/envFile.test.ts src/utils/thinking.test.ts src/utils/status.test.ts src/utils/http.test.ts(isolated, as CI): 67 pass / 0 failbun testafterbun run build(Bun 1.3.14, base 19bb39c): main 10,375 pass / 44 skip / 40 fail / 4 errors; this PR 10,394 pass / 44 skip / 21 fail / 0 errors. The http victim now passes, and 40 other previously failing betas, fastMode, context, claude.lifecycle, set-context-window and some attribution cases pass too (sideQuery attribution, MiniMax China, modelCost, mcp/config, profiles, paths, PromptInput and commit-message still fail). One test newly fails in that run,a launch that only runs local commands leaves no transcript(sessionStorage.commandOnlySession). It was already on the TA-001 pollution list from an earlier run, runs before status/thinking, and passes alone; it is left for a later slice.--isolate.Notes
Summary by CodeRabbit