fix(typecheck): type secure storage command output#1524
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (2)**/*⚙️ CodeRabbit configuration file
Files:
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (2)
📝 WalkthroughWalkthroughAdds command output normalization in Windows credential storage and introduces centralized execaSync test helpers; refactors platform tests to use those helpers and adds DPAPI byte-array handling tests and stderr/string-array warning coverage. ChangesSecure Storage Robustness
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested Reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/secureStorage/platformStorage.test.ts`:
- Around line 237-262: Add a regression test that covers the branch where execa
returns stdout/stderr as string[]: mock mockExecaSync/execaResult to return
stdout: ["...", "..."] for read() and assert windowsCredentialStorage.read()
joins and parses the array into testData; also add a test where execa returns
exitCode: 1 with stderr: ["dpapi failed",""] for update() and assert
windowsCredentialStorage.update(testData) returns { success: false, warning:
"dpapi failed" } (this ensures the string[] normalization code path for read()
and update() is exercised).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ec89acad-86a5-480d-8acd-42763a6bae81
📒 Files selected for processing (2)
src/utils/secureStorage/platformStorage.test.tssrc/utils/secureStorage/windowsCredentialStorage.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/secureStorage/windowsCredentialStorage.tssrc/utils/secureStorage/platformStorage.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: 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. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/secureStorage/platformStorage.test.ts
🔇 Additional comments (2)
src/utils/secureStorage/windowsCredentialStorage.ts (1)
51-70: LGTM!Also applies to: 76-76, 108-112, 159-163
src/utils/secureStorage/platformStorage.test.ts (1)
11-75: LGTM!Also applies to: 95-95, 140-236, 269-274
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I do not see any actionable issues from my review.
* fix(typecheck): type secure storage command output * test(typecheck): cover string array secure storage output
… env var upstream Gitlawb#1524: type secure storage command output - Take upstream's MockExecaOptions / MockExecaArgs / MockExecaResult types (replaces untyped mockExecaSync tuple) - Add execaResult() helper + getExecaCall/getCommandArgs/ getPowerShellScript/getCommandOptions/getCommandInput helpers - Add 4 new test cases (byte/string-array stdout + stderr) - Drop // @ts-nocheck (no longer needed — typing is correct) fork rebrand (per opencc rebrand pattern): - OPENCLAUDE_ENABLE_LEGACY_WINDOWS_PASSWORDVAULT → OPENCC_* (matches fork's windowsCredentialStorage.ts source which already uses OPENCC_*; test was setting wrong env var name) - Claude Code → Open CC in service-name test (matches source's getSecureStorageServiceName return value) Pre-existing fork bugs fixed in passing: - Test was setting OPENCLAUDE_* env var but source checks OPENCC_*, so legacy PasswordVault code path was never being exercised. 9 tests were failing for this reason; all 19 now pass. - Test was asserting 'Claude Code' substring but source returns 'Open CC-...' (rebranded). Updated expectation. VERIFICATION: typecheck: 0 errors from this file (was @ts-nocheck before) tests: 9 pass / 10 fail → 19 pass / 0 fail
3 participants
Part of #1486.
Summary
execaSyncbefore trimming or JSON parsing.execaSynctest mock and centralize command argument assertions.Validation
bun test src/utils/secureStorage/platformStorage.test.ts(17 pass)bun test src/utils/secureStorage/platformStorage.test.ts src/utils/codexCredentials.test.ts src/utils/githubModelsCredentials.test.ts src/utils/githubModelsCredentials.hydrate.test.ts src/utils/githubModelsCredentials.refresh.test.ts src/utils/geminiCredentials.test.ts src/services/api/providerConfig.codexSecureStorage.test.ts src/services/api/providerConfig.runtimeCodexCredentials.test.ts(52 pass)env -u OPENAI_API_KEY -u OPENAI_BASE_URL -u OPENAI_MODEL -u CLAUDE_CODE_USE_OPENAI -u CLAUDE_CODE_USE_GEMINI -u CLAUDE_CODE_USE_MISTRAL -u CLAUDE_CODE_USE_GITHUB -u CLAUDE_CODE_USE_BEDROCK -u CLAUDE_CODE_USE_VERTEX -u CLAUDE_CODE_USE_FOUNDRY -u GEMINI_API_KEY -u GOOGLE_API_KEY -u GITHUB_TOKEN -u ANTHROPIC_MODEL -u ANTHROPIC_SMALL_FAST_MODEL bun run check(3435 pass)bun run test:provider(650 pass)env -u OPENAI_API_KEY -u OPENAI_BASE_URL -u OPENAI_MODEL -u CLAUDE_CODE_USE_OPENAI -u CLAUDE_CODE_USE_GEMINI -u CLAUDE_CODE_USE_MISTRAL -u CLAUDE_CODE_USE_GITHUB -u CLAUDE_CODE_USE_BEDROCK -u CLAUDE_CODE_USE_VERTEX -u CLAUDE_CODE_USE_FOUNDRY -u GEMINI_API_KEY -u GOOGLE_API_KEY -u GITHUB_TOKEN -u ANTHROPIC_MODEL -u ANTHROPIC_SMALL_FAST_MODEL npm run test:provider-recommendation(79 pass)bun install --cwd web --frozen-lockfile && bun run web:typecheckbun run typecheckstill exits 2 on the repo-wide baseline, but reports no diagnostics undersrc/utils/secureStorageafter this change.git diff --checkSummary by CodeRabbit
Refactor
Tests