fix: prevent roadmap.json cross-project contamination on project switch#1886
fix: prevent roadmap.json cross-project contamination on project switch#1886Mitsu13Ion wants to merge 3 commits intoAndyMik90:developfrom
Conversation
Race condition: when switching projects, the roadmap store holds stale data during async load. Any save triggered in that gap writes Project A's roadmap to Project B's file. Fix by validating roadmap.projectId matches the target projectId before every save, and clearing the store immediately on load.
Summary of ChangesHello @Mitsu13Ion, 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 data integrity issue where Highlights
Changelog
Activity
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
|
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughReplaces use of the store's active project with the event-provided projectId when handling TASK_STATUS_CHANGE and roadmap persistence; adds guards in reconciliation and load to clear or validate roadmap state to prevent cross-project saves. Changes
Sequence Diagram(s)sequenceDiagram
participant IPC as IPC Event (TASK_STATUS_CHANGE)
participant UseIpc as useIpc.ts
participant Store as RoadmapStore
participant ElectronAPI as window.electronAPI.saveRoadmap
IPC->>UseIpc: emit TASK_STATUS_CHANGE (specId, projectId)
UseIpc->>Store: markFeatureDone(specId)
Store-->>UseIpc: updatedRoadmap (if roadmap.projectId == eventProjectId)
UseIpc->>ElectronAPI: saveRoadmap(updatedRoadmap, eventProjectId)
ElectronAPI-->>UseIpc: save result / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![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
The pull request effectively addresses the race condition that leads to roadmap.json cross-project contamination. By validating the projectId from the IPC event against the roadmap currently in the store, and by clearing the roadmap store immediately upon project switching, it ensures that stale data is not persisted to the wrong project file. I have a few suggestions to further improve the robustness of the project switching logic and maintain backward compatibility.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…itch Prevents stale competitor analysis and generation progress from the previous project showing during async load of the new project's data.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
PR #1886 reviewed: 13 findings (0 structural issues). Verdict: approve.
Findings (13 selected of 13 total)
🔵 [SEC-1] [LOW] No validation that eventProjectId is a safe identifier before passing to saveRoadmap
📁 apps/frontend/src/renderer/hooks/useIpc.ts:228
The eventProjectId (aliased from projectId callback parameter) is passed directly to window.electronAPI.saveRoadmap() without any validation that it is a well-formed project identifier. If the IPC event source can be influenced by an attacker, a crafted projectId could potentially cause path traversal in the backend file-save logic (e.g., ../../etc/cron.d/evil). However, the risk is mitigated by the fact that rm.projectId === eventProjectId check requires matching an existing roadmap's projectId, limiting exploitation. The actual severity depends on the backend saveRoadmap implementation.
Suggested fix:
Validate eventProjectId against an allowlist or UUID/slug pattern before passing it to saveRoadmap. E.g., `if (!/^[a-zA-Z0-9_-]+$/.test(eventProjectId)) return;`
🔵 [SEC-2] [LOW] TOCTOU race between projectId check and save in reconcileLinkedFeatures
📁 apps/frontend/src/renderer/stores/roadmap-store.ts:716
In reconcileLinkedFeatures, the code checks updatedRoadmap.projectId === projectId then proceeds to save. Between the check and the saveRoadmap call, another async operation could swap the roadmap in the store. While this PR significantly narrows the window compared to before, a true atomic guard (e.g., a lock or version counter) would fully eliminate the race. This is a defense-in-depth concern rather than a directly exploitable vulnerability.
Suggested fix:
Capture the roadmap reference once and pass that captured reference to saveRoadmap rather than re-reading from the store, or introduce a generation/version counter to detect stale state.
🟡 [QLT-1] [MEDIUM] Redundant variable assignment for projectId
📁 apps/frontend/src/renderer/hooks/useIpc.ts:227
The line const eventProjectId = projectId; creates an unnecessary alias. The projectId parameter from the callback is already available and its name is sufficiently descriptive in context. The added comment explains the intent well, but the alias adds no clarity and introduces a redundant variable.
Suggested fix:
Remove `const eventProjectId = projectId;` and use `projectId` directly in the subsequent checks. The comment explaining not to use the store's activeProjectId is sufficient.
🟡 [QLT-2] [MEDIUM] Double state read and double projectId check is fragile
📁 apps/frontend/src/renderer/hooks/useIpc.ts:229
The code reads useRoadmapStore.getState() twice — once before markFeatureDoneBySpecId and once after — and checks rm.projectId === eventProjectId both times. While the second check guards against a theoretical race during markFeatureDoneBySpecId, markFeatureDoneBySpecId is a synchronous Zustand mutation that cannot change projectId. The double-check pattern adds complexity without real protection. If the concern is truly about concurrency, a more robust pattern (e.g., a mutex or transactional update) would be warranted.
Suggested fix:
Consider reading state once after the mutation. The pre-mutation check is sufficient to decide whether to proceed; the post-mutation check is redundant for a synchronous store update.
🟠 [QLT-3] [HIGH] markFeatureDoneBySpecId silently skipped without logging
📁 apps/frontend/src/renderer/hooks/useIpc.ts:229
When rm is null, eventProjectId is falsy, or the projectId doesn't match, the feature completion is silently dropped with no log or warning. This could make debugging very difficult in production — a task completes successfully but the roadmap is never updated, and there's no trace of why.
Suggested fix:
Add an else branch with a debug/warn log: `console.warn('[useIpc] Skipping roadmap update: project mismatch or missing roadmap', { eventProjectId, roadmapProjectId: rm?.projectId });`
🟡 [QLT-4] [MEDIUM] Missing edge case: roadmap loaded after event arrives
📁 apps/frontend/src/renderer/hooks/useIpc.ts:228
If a task completion event arrives after loadRoadmap has cleared the store (setRoadmap(null)) but before the new roadmap has been loaded, the feature done mark will be permanently lost. The fix prevents cross-project contamination but introduces a window where legitimate same-project events are dropped. Consider queuing events or re-reconciling after load.
Suggested fix:
Consider maintaining a pending operations queue for task completion events that arrive during the async gap of loadRoadmap, and replay them once the roadmap is loaded.
🔵 [QLT-5] [LOW] Clearing store state without checking if already null
📁 apps/frontend/src/renderer/stores/roadmap-store.ts:733
In loadRoadmap, setRoadmap(null), setCompetitorAnalysis(null), and setGenerationStatus are called unconditionally. While not a bug, these trigger Zustand state updates and potential re-renders even when the store is already in the cleared state (e.g., on first load or consecutive calls).
Suggested fix:
Consider guarding with a check: `if (store.roadmap !== null) store.setRoadmap(null);` to avoid unnecessary re-renders.
🔵 [QLT-6] [LOW] Inconsistent error handling pattern between reconcile and IPC save
📁 apps/frontend/src/renderer/stores/roadmap-store.ts:718
Both the reconciliation save (roadmap-store.ts:718) and the IPC save (useIpc.ts:234) use .catch() to log errors but neither propagates or handles them further. While consistent with each other, the lack of any user-facing feedback or retry mechanism could lead to silent data loss.
Suggested fix:
Consider a centralized save-with-retry utility or at minimum a user-facing notification on persistent save failures.
🟠 [DEEP-1] [HIGH] Race condition: roadmap state can change between projectId check and markFeatureDoneBySpecId mutation
📁 apps/frontend/src/renderer/hooks/useIpc.ts:228
Between the check rm.projectId === eventProjectId and the call to markFeatureDoneBySpecId(taskId), another async operation (e.g., loadRoadmap for a different project) could replace the roadmap in the store. The code reads rm from state, validates projectId, then calls markFeatureDoneBySpecId which mutates whatever roadmap is currently in the store — not necessarily the one that was validated. The second check (updatedRm.projectId === eventProjectId) mitigates the save, but the mutation itself (markFeatureDoneBySpecId) would still corrupt the wrong project's in-memory roadmap.
Suggested fix:
Pass the eventProjectId into markFeatureDoneBySpecId and have it internally verify the roadmap's projectId before mutating, or perform the check and mutation atomically inside the store action.
🟡 [DEEP-2] [MEDIUM] reconcileLinkedFeatures applies mutations to store before checking projectId on save
📁 apps/frontend/src/renderer/stores/roadmap-store.ts:715
The guard at line 715 only prevents the save when projectId doesn't match, but the mutations that set hasChanges = true (earlier in the function, not shown in diff) have already been applied to the store's roadmap state. If a project switch happened during the async reconciliation, those mutations would have been applied to the new project's roadmap in memory. The guard prevents persisting the corrupted state, but the in-memory state is still wrong until the next load.
Suggested fix:
Check `updatedRoadmap.projectId === projectId` before applying any mutations to the store, or revert the mutations if the projectId no longer matches after the async gap.
🟡 [DEEP-3] [MEDIUM] Clearing roadmap to null on project switch may cause UI flash or null reference errors
📁 apps/frontend/src/renderer/stores/roadmap-store.ts:734
Setting roadmap to null immediately in loadRoadmap before the new roadmap is loaded creates a window where any component reading useRoadmapStore.getState().roadmap will see null. If components don't defensively handle null roadmap (and the old code didn't need to since roadmap was only replaced, never cleared mid-session), this could cause runtime errors or a visible UI flash/flicker showing an empty state before the new roadmap loads.
Suggested fix:
Ensure all UI consumers of the roadmap store handle null roadmap gracefully, or use a loading flag instead of nulling the roadmap, and only clear the old roadmap when the new one is ready to replace it.
🟡 [DEEP-4] [MEDIUM] Task completion events for previous project are silently dropped instead of deferred
📁 apps/frontend/src/renderer/hooks/useIpc.ts:227
When a task completes for project A but the user has switched to project B, the markFeatureDoneBySpecId call is skipped entirely. The roadmap.json for project A is never updated to reflect the task completion. This means the feature won't be marked as done until some other reconciliation mechanism runs (if one exists). The old bug was that it would write to the wrong project's file; the new behavior is that legitimate updates are lost.
Suggested fix:
Queue the update for the original project and apply it when that project's roadmap is next loaded, or load the correct project's roadmap from disk, apply the mutation, and save it back without going through the store.
🔵 [DEEP-5] [LOW] Redundant double-read of store state after markFeatureDoneBySpecId
📁 apps/frontend/src/renderer/hooks/useIpc.ts:226
The code reads useRoadmapStore.getState().roadmap before the mutation to check projectId, then reads it again after the mutation. The first read's rm variable is never used for the save — only for the guard. While not a bug, it's slightly confusing and the pattern could be simplified by reading state once after mutation and checking projectId there.
Suggested fix:
Simplify to a single state read after the mutation, checking projectId at that point.
This review was generated by Auto Claude.
I think that, on the contrary, changes need to be made to the review agent because it is completely off the mark as a review. Findings that are wrong DEEP-1 (HIGH) and SEC-2 — they claim there is a race condition between the check and markFeatureDoneBySpecId… but that is a synchronous mutation in a single-threaded event loop. Nothing can interrupt between the two. False positive. DEEP-2 — same mistake: the mutations in the for loop of reconcileLinkedFeatures are synchronous after the single await getTasks. Out-of-scope findings (pre-existing, not introduced by this PR): SEC-1 — validation of projectId input → this is internal Electron IPC, not user input. And it was already like this before. QLT-6 — no retry on save → pre-existing pattern, unchanged. QLT-4 / DEEP-4 — events lost during the async gap → reconcileLinkedFeatures catches that on the next loadRoadmap. Not a new issue. DEEP-3 — UI flash with null → the store was already doing setRoadmap(null) in the else branch (line 799), so the components already handle null. minor findings: QLT-1 — the eventProjectId alias is stylistically debatable, but it clarifies intent. Not a problem. QLT-3 — a console.warn on the skip could help with debugging. The only potentially useful point, but definitely not HIGH. QLT-5 — re-render on setRoadmap(null) when already null → negligible micro-optimization. |
2 participants
Summary
roadmap.jsonwith another project's dataroadmap.projectIdvalidation before every save inuseIpc.tsandroadmap-store.tsloadRoadmap) to prevent stale saves during async gapTest plan
roadmap.jsonstill has Project A's features after merge completesroadmap.jsonis unchangedcd apps/frontend && npm test— all 3467 tests passcd apps/frontend && npx tsc --noEmit— no type errors🤖 Generated with Claude Code
Summary by CodeRabbit