fix(windows): initialize known_marketplaces.json to prevent CLI crashes #1107
Conversation
|
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a Windows-specific startup check that validates and repairs Changes
Sequence Diagram(s)sequenceDiagram
participant App as Electron App
participant FS as File System
participant CLI as Claude CLI (startup)
participant Logger as Logger
App->>FS: Resolve homedir & plugins path
App->>FS: Check plugins dir exists
FS-->>App: exists / not exists
alt not exists
App->>FS: mkdirSync(plugins)
FS-->>App: created
end
App->>FS: Read known_marketplaces.json
FS-->>App: file contents or error
alt missing/empty/whitespace/array/invalid
App->>FS: Write "{}" to known_marketplaces.json
FS-->>App: write success / error
App->>Logger: log fix or error
else valid object
App->>Logger: log no-op
end
App->>CLI: continue startup (cleanupStaleUpdateMetadata, other flows)
CLI-->>App: startup proceeds without parse error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
Summary of ChangesHello @youngmrz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue on Windows where an improperly formatted Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thanks for your first PR!
A maintainer will review it soon. Please make sure:
- Your branch is synced with
develop - CI checks pass
- You've followed our contribution guide
Welcome to the Auto Claude community!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fix for a crash on Windows related to a corrupted known_marketplaces.json file. The approach of adding a validation and repair function, fixKnownMarketplacesJson, that runs at application startup is sound and directly addresses the described issue. The implementation correctly handles several scenarios of file corruption. My review focuses on refactoring opportunities within this new function to enhance code clarity, simplify logic, and improve robustness. The proposed changes will make the new code more maintainable while preserving its intended behavior.
apps/frontend/src/main/index.ts
Outdated
| if (!existsSync(pluginsDir)) { | ||
| try { | ||
| mkdirSync(pluginsDir, { recursive: true }); | ||
| console.log('[main] Created Claude plugins directory:', pluginsDir); | ||
| } catch (e) { | ||
| console.warn('[main] Failed to create plugins directory:', e); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for ensuring the plugins directory exists can be simplified. Instead of checking for its existence with existsSync before calling mkdirSync (a "Look Before You Leap" approach), you can directly call mkdirSync and handle potential errors. With the { recursive: true } option, mkdirSync doesn't throw an error if the directory already exists. Furthermore, its return value can be used to conditionally log the creation message, making the code more concise and avoiding a potential race condition.
| if (!existsSync(pluginsDir)) { | |
| try { | |
| mkdirSync(pluginsDir, { recursive: true }); | |
| console.log('[main] Created Claude plugins directory:', pluginsDir); | |
| } catch (e) { | |
| console.warn('[main] Failed to create plugins directory:', e); | |
| return; | |
| } | |
| } | |
| // Ensure plugins directory exists | |
| try { | |
| const createdPath = mkdirSync(pluginsDir, { recursive: true }); | |
| if (createdPath) { | |
| console.log('[main] Created Claude plugins directory:', pluginsDir); | |
| } | |
| } catch (e) { | |
| console.warn('[main] Failed to create plugins directory:', e); | |
| return; | |
| } |
apps/frontend/src/main/index.ts
Outdated
| } else { | ||
| try { | ||
| const content = readFileSync(marketplacesPath, 'utf-8').trim(); | ||
| // Fix if empty, contains only whitespace, or is an array instead of object | ||
| if (content === '' || content === '[]') { | ||
| needsFix = true; | ||
| console.log('[main] Detected corrupted known_marketplaces.json:', content || '(empty)'); | ||
| } else { | ||
| // Validate it's a proper JSON object (not array) | ||
| const parsed = JSON.parse(content); | ||
| if (Array.isArray(parsed)) { | ||
| needsFix = true; | ||
| console.log('[main] known_marketplaces.json contains array instead of object'); | ||
| } | ||
| } | ||
| } catch (e) { | ||
| // If we can't read or parse it, try to fix it | ||
| needsFix = true; | ||
| console.warn('[main] Failed to read known_marketplaces.json:', e); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic to determine if known_marketplaces.json needs fixing can be simplified and made more robust. The explicit checks for content === '' or content === '[]' are redundant because JSON.parse will either fail on an empty string or produce an array that can be type-checked. By consolidating the validation, you can create a cleaner and more comprehensive check that ensures the file contains a valid JSON object, correctly handling other invalid top-level JSON values like null or primitives (e.g., "a string"), which the current logic does not account for.
} else {
try {
const content = readFileSync(marketplacesPath, 'utf-8');
const parsed = JSON.parse(content);
// The file must contain a JSON object. Arrays, null, or other primitives are invalid.
if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) {
needsFix = true;
console.log('[main] known_marketplaces.json does not contain a valid JSON object, fixing.');
}
} catch (e) {
// If we can't read or parse it (e.g., empty, malformed), it needs fixing.
needsFix = true;
console.warn('[main] Failed to read or parse known_marketplaces.json, fixing:', e);
}
}![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/index.ts`:
- Around line 5-6: Move the mkdirSync import into the same fs import group that
contains readFileSync/writeFileSync (so all fs functions are consolidated)
because mkdirSync is only used by fixKnownMarketplacesJson; update the top
imports to import { existsSync, readFileSync, writeFileSync, mkdirSync } from
'fs' and remove the separate import of mkdirSync to keep imports organized and
local references unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/src/main/index.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.
Files:
apps/frontend/src/main/index.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
apps/frontend/src/main/index.ts (2)
106-167: Well-structured fix with good error handling.The function properly handles all documented failure cases:
- Missing file
- Empty file
- Array-valued JSON (
[])- Non-array but still array-type JSON content
- Malformed/unparseable JSON
The error handling with graceful degradation (logging warnings and continuing) is appropriate for a startup fix.
311-313: Correct placement in startup sequence.The fix runs early in the startup flow, before
AgentManagerinitialization (line 339),preWarmToolCache(line 430), andinitializeClaudeProfileManager(line 439)—all of which may invoke the Claude CLI. This ensures the file is validated before any potential crash point.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🔴 BLOCKED
🔴 Blocked - 1 CI check(s) failing. Fix CI before merge.
Blocked: 1 CI check(s) failing. Fix CI before merge.
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
- CI Failed: Require Re-review on Push
Findings Summary
- Medium: 1 issue(s)
- Low: 1 issue(s)
Generated by Auto Claude PR Review
Findings (2 selected of 2 total)
🟡 [20d9707283ef] [MEDIUM] Direct process.platform check violates platform abstraction guidelines
📁 apps/frontend/src/main/index.ts:115
The function uses process.platform !== 'win32' directly instead of using isWindows() from the platform module at ./platform. CLAUDE.md explicitly states: 'Import from these modules instead of checking process.platform directly' and marks direct checks as 'WRONG'. Other files in the codebase (env-utils.ts, cli-tool-manager.ts, terminal/pty-manager.ts) properly use the platform abstraction.
Suggested fix:
Import and use the platform abstraction:
```typescript
import { isWindows } from './platform';
function fixKnownMarketplacesJson(): void {
if (!isWindows()) {
return;
}
// ...
}
#### 🔵 [7af149428765] [LOW] TOCTOU race comment understates potential data loss scenario
📁 `apps/frontend/src/main/index.ts:149`
The comment claims the read-then-write TOCTOU race is 'benign' because overwriting valid JSON with '{}' is 'also valid JSON'. While technically true, this could overwrite meaningful marketplace configurations if another process writes valid data between the read (line 137) and write (line 149). However, since this runs at app startup before any Claude CLI calls with a tiny race window, the practical risk is very low.
**Suggested fix:**
Consider re-validating before writing to minimize data loss risk:
if (needsFix) {
try {
// Re-check before writing
try {
const recheck = JSON.parse(readFileSync(marketplacesPath, 'utf-8'));
if (!Array.isArray(recheck) && Object.keys(recheck).length > 0) return;
} catch { /* still broken */ }
writeFileSync(marketplacesPath, '{}', 'utf-8');
} catch (e) { ... }
}
---
*This review was generated by Auto Claude.*
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/index.ts`:
- Around line 115-117: Add an import for the platform abstraction functions at
the top of the file (isWindows and isMacOS from the platform abstraction module)
and replace every direct process.platform check with the corresponding helper:
change process.platform === 'win32' to isWindows(), process.platform !== 'win32'
to !isWindows(), process.platform === 'darwin' to isMacOS(), and
process.platform !== 'darwin' to !isMacOS() across all occurrences (eight
violations referenced), updating any conditional expressions or early returns
that currently reference process.platform to use isWindows()/isMacOS() instead.
♻️ Duplicate comments (3)
apps/frontend/src/main/index.ts (3)
5-6: Import organization could be consolidated.The
mkdirSyncimport here could be grouped with the otherfsimports on line 31 (readFileSync,writeFileSync, etc.) for better organization. However, this is a minor style concern and does not affect functionality.
122-131: Simplify directory creation to avoid TOCTOU pattern.The
existsSynccheck beforemkdirSynccreates a race condition. With{ recursive: true },mkdirSyncwon't throw if the directory already exists, and its return value indicates whether it created the directory.♻️ Proposed fix
// Ensure plugins directory exists - if (!existsSync(pluginsDir)) { - try { - mkdirSync(pluginsDir, { recursive: true }); + try { + const created = mkdirSync(pluginsDir, { recursive: true }); + if (created) { console.log('[main] Created Claude plugins directory:', pluginsDir); - } catch (e) { - console.warn('[main] Failed to create plugins directory:', e); - return; } + } catch (e) { + console.warn('[main] Failed to create plugins directory:', e); + return; }
136-149: Simplify JSON validation logic.The explicit check for
content === '[]'on line 139 is partially redundant since line 145 already checksArray.isArray(parsed). However, the current logic misses other invalid top-level JSON values likenull, numbers, or strings (e.g.,"hello"parses successfully but isn't a valid object).♻️ Proposed consolidation
let needsFix = false; try { const content = readFileSync(marketplacesPath, 'utf-8').trim(); - // Fix if empty, contains only whitespace, or is an array instead of object - if (content === '' || content === '[]') { + if (content === '') { needsFix = true; console.log('[main] Detected corrupted known_marketplaces.json:', content || '(empty)'); } else { - // Validate it's a proper JSON object (not array) + // Validate it's a proper JSON object (not array, null, or primitive) const parsed = JSON.parse(content); - if (Array.isArray(parsed)) { + if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { needsFix = true; - console.log('[main] known_marketplaces.json contains array instead of object'); + console.log('[main] known_marketplaces.json does not contain a valid JSON object'); } } } catch (e) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/src/main/index.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: All user-facing text in the frontend MUST use i18n translation keys fromreact-i18next, not hardcoded strings
Use translation key formatnamespace:section.key(e.g.,navigation:items.githubPRs) when referencing translations in code
For error messages with dynamic content, use i18n interpolation with syntax liket('errors:task.parseError', { error: errorMessage })
Files:
apps/frontend/src/main/index.ts
**/*.{ts,tsx,js,py}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,py}: Do not checkprocess.platformdirectly in code - always import platform detection functions from the platform abstraction module
Never hardcode platform-specific paths likeC:\Program Files\,/opt/homebrew/bin/, or/usr/local/bin/directly in code
Files:
apps/frontend/src/main/index.ts
apps/frontend/src/main/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use platform abstraction functions like
isWindows(),isMacOS(),isLinux(),getPathDelimiter(),getExecutableExtension(),findExecutable(),joinPaths()from the platform module instead of hardcoding paths or platform checks
Files:
apps/frontend/src/main/index.ts
apps/frontend/**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Frontend code must be built with Electron, React, and TypeScript
Files:
apps/frontend/src/main/index.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/index.ts
🧠 Learnings (1)
📚 Learning: 2026-01-16T09:10:31.701Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-16T09:10:31.701Z
Learning: Applies to apps/frontend/src/main/**/*.{ts,tsx} : Use platform abstraction functions like `isWindows()`, `isMacOS()`, `isLinux()`, `getPathDelimiter()`, `getExecutableExtension()`, `findExecutable()`, `joinPaths()` from the platform module instead of hardcoding paths or platform checks
Applied to files:
apps/frontend/src/main/index.ts
🧬 Code graph analysis (1)
apps/frontend/src/main/index.ts (2)
scripts/bump-version.js (2)
content(146-146)content(161-161)apps/frontend/scripts/download-python.cjs (1)
content(567-567)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Seer Code Review
- GitHub Check: test-python (3.13, windows-latest)
- GitHub Check: test-python (3.12, macos-latest)
- GitHub Check: test-python (3.12, windows-latest)
- GitHub Check: test-frontend (windows-latest)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
apps/frontend/src/main/index.ts (2)
159-169: LGTM - Write operation with good idempotency documentation.The comment explaining the benign TOCTOU race is helpful. Writing
{}is indeed idempotent, and the error handling is appropriate.
314-316: LGTM - Correct placement in startup sequence.The function is appropriately called early in the startup flow, before any Claude CLI operations might occur. The comment clearly explains the purpose.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
youngmrz
force-pushed
the
fix/windows-marketplace-init
branch
3 times, most recently
from
d4bbfa6 to
b1b355c
Compare
On Windows, an empty or corrupted known_marketplaces.json in the Claude
CLI config (C:\Users\<user>\.claude\plugins\known_marketplaces.json)
causes crashes. The file must contain a valid JSON object, not be empty
or contain an array.
This fix runs at app startup before any Claude CLI calls and:
- Creates the plugins directory if missing
- Writes {} to the file if it's missing, empty, or contains []
- Handles malformed JSON by rewriting the file with {}
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Remove the existsSync + readFileSync pattern which has a potential race condition (file could be deleted between check and read). Instead, just try to read the file directly and handle ENOENT in the catch block. This approach is simpler and avoids the CodeQL file system race warning. Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: youngmrz <[email protected]>
The theoretical race between reading the file to check if it needs fixing
and writing the fix is benign because writing `{}` is idempotent - if
another process already fixed the file, overwriting valid JSON with `{}`
still results in valid JSON.
Addresses bot feedback on line 161.
Co-Authored-By: Claude Opus 4.5 <[email protected]>
…latform check Addresses review feedback requiring platform abstraction usage per CLAUDE.md guidelines.
- Remove redundant existsSync check before mkdirSync (recursive is idempotent) - Consolidate fs imports to single location (line 31) Addresses bot review feedback about LBYL pattern and import organization. Co-Authored-By: Claude Opus 4.5 <[email protected]>
youngmrz
force-pushed
the
fix/windows-marketplace-init
branch
from
b1b355c to
be97c96
Compare
… main Replace remaining direct process.platform === 'darwin' and 'win32' checks with isMacOS() and isWindows() from the platform module, per coding guidelines. Co-Authored-By: Claude Opus 4.5 <[email protected]>
3 participants
Summary
On Windows, an empty or corrupted
known_marketplaces.jsonin the Claude CLI config causes crashes. This fix runs at app startup and ensures the file contains valid JSON.Changes
{}to the file if it's missing, empty, or contains[]{}Fixes
Closes #1095
Test Plan
~/.claude/plugins/known_marketplaces.json[]in file{}after startup🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.