Repository navigation
test: add snapshotEnv helper and close env and auth cleanup gaps (TA-001 slice 4) - #2335
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 3 functions across 7 files. (1 skipped: 1 … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title is concise, scoped, and accurately describes the snapshotEnv helper and environment and auth cleanup changes. Description check The description includes the required Summary, Impact, Testing, and Notes sections. It explains the changes, impact, commands and results, focused tests, known environmental failures, and limitations. 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 auth test-module cleanup and provider-routing environment isolation. The authored description identifies both areas, states that the changes are test-only, and records no user-facing im… No Hidden Policy Change PASS — the authoritative diff changes only test files, src/test/envSnapshot.ts, andCONTRIBUTING.md. The code saves/restores test environment variables and test module mocks; it does not alter pro…Full details: Risk Surface Disclosed
-
Explanation
The PR touches auth test-module cleanup and provider-routing environment isolation. The authored description identifies both areas, states that the changes are test-only, and records no user-facing impact. This discloses the risk surface and indicates that the PR introduces no blocker.
Full details: No Hidden Policy Change
-
Explanation
PASS — the authoritative diff changes only test files,
src/test/envSnapshot.ts, andCONTRIBUTING.md. The code saves/restores test environment variables and test module mocks; it does not alter product routing defaults, trust decisions, telemetry or network behavior, permissions, or production runtime code. No hidden policy decision requires maintainer alignment.
-
Full details: Docstring Coverage
-
Explanation
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 3 functions across 7 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ 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.
Summary
src/test/envSnapshot.ts:snapshotEnv(keys, { clear? })saves the named variables, can optionally unset them, and returns a restore function. Restore puts back the exact previous values (including empty strings) and deletes the ones that were unset. It is not a blanket scrub. Covered bysrc/test/envSnapshot.test.ts.src/utils/providerFlag.test.ts:CLAUDE_CODE_PROVIDER_ROUTE_ID(set tocommandcodein one test) was missing from the hand-maintained snapshot/restore lists and leaked out of the file. The lists are replaced with the helper, and the route id is included.src/services/api/openaiShim.ollamaTextToolCalls.test.tsandsrc/services/api/openaiShim/xmlToolCallParsing.test.ts: cleanup deleted provider keys instead of restoring whatever was set before. They now use the helper.src/utils/fastMode.test.tsandsrc/hooks/useApiKeyVerification.test.tsx: cleanup re-registered the liveutils/authnamespace, which Bun had already redirected to the mock, so the auth stubs leaked into later files (follow-up noted in test: isolate attribution suites from leaked auth, beta and routing mocks (TA-001 slice 5) #2334). Both now restore from a real-module snapshot copied before any mock is registered, the same pattern as earlier TA-001 slices.CONTRIBUTING.md: one test-guidance bullet on usingsnapshotEnv.Impact
betasandcompacttests' intentional clears are unchanged.--isolatestays in CI and is handled by the next slice.Testing
bun run build, temp config dirs):bun run typecheck: pass.bun run check: pass, including all 8--isolateshards.bun test(no--isolate): 10434 pass, 31 skip, 0 fail across 782 files.gitSettings.test.tstests ("omits git instructions outside a git repository" and "follows the session cwd"). On the build host, a sandboxed tool outside this repo briefly creates an empty.gitdirectory in the system temp dir, and that run overlapped one. The file passes on its own (8/8) and in the full rerun above, so the failures were environmental and unrelated to this diff.Notes
--isolateis the next slice.Summary by CodeRabbit