SUP-2243 Guard vault root duplicate notes - #69
Conversation
📝 WalkthroughWalkthroughThis PR introduces a zero-byte vault-root duplicate guard that prevents Obsidian note shadowing. A new module exports the repair function and path/title helpers, integrated into note writing and journal linking to detect and safely delete stale root duplicates. Comprehensive tests validate direct and integrated behavior. ChangesVault Root Duplicate Guard
Sequence Diagram(s)sequenceDiagram
participant Caller
participant repairZeroByteVaultRootDuplicate
participant Filesystem as fs (stat/unlink)
Caller->>repairZeroByteVaultRootDuplicate: call with noteTitle, vault paths
repairZeroByteVaultRootDuplicate->>Filesystem: stat canonical Notes path
Filesystem-->>repairZeroByteVaultRootDuplicate: file exists, size
repairZeroByteVaultRootDuplicate->>Filesystem: stat vault root path
Filesystem-->>repairZeroByteVaultRootDuplicate: file exists, size
alt Root is zero-byte AND Notes is non-empty
repairZeroByteVaultRootDuplicate->>Filesystem: unlink zero-byte root
Filesystem-->>repairZeroByteVaultRootDuplicate: success
repairZeroByteVaultRootDuplicate-->>Caller: {repaired: true}
else Conditions not met
repairZeroByteVaultRootDuplicate-->>Caller: {repaired: false, reason}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dac56762c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const vaultRootDuplicate = repairZeroByteVaultRootDuplicate({ | ||
| noteTitle: safeName, | ||
| notesDir, | ||
| notesFilePath: filePath, | ||
| }); |
There was a problem hiding this comment.
When JARVOS_NOTES_DIR or paths.notes points to a folder that is not literally named Notes, this call cannot infer the vault root from notesDir, so repairZeroByteVaultRootDuplicate returns checked: false and leaves the zero-byte root duplicate behind. This shows up especially when journal backlinking is disabled (JARVOS_JOURNAL_BACKLINK=0), because the linker path does pass getVaultDir() and therefore is the only path that would repair custom note directories.
Useful? React with 👍 / 👎.
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
`@modules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/src/lib/vault-root-duplicate-guard.js`:
- Around line 56-72: The guard currently lets fileStat and fs.unlinkSync throw
and abort the calling flow; modify the function that uses fileStat and
fs.unlinkSync (vault-root-duplicate-guard.js) to make cleanup best-effort by
wrapping the fileStat(canonicalPath) and fs.unlinkSync(rootPath) calls in
try/catch blocks: if fileStat throws treat it as a non-fatal “not repaired”
result (return checked:true, repaired:false and include the error message in
reason), and when unlinkSync throws catch the error, log or include the error in
the returned reason and return checked:true, repaired:false instead of
rethrowing; keep existing successful-path behavior when no errors occur.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8af8568-8334-43f2-81e8-c3e60b0bcfb8
📒 Files selected for processing (4)
modules/jarvos-secondbrain/bridge/provenance/src/link-to-journal.jsmodules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/src/lib/vault-root-duplicate-guard.jsmodules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/src/write-to-vault.jsmodules/jarvos-secondbrain/tests/vault-root-duplicate-guard.test.js
| const rootStat = fileStat(rootPath); | ||
| if (!rootStat) { | ||
| return { checked: true, repaired: false, rootPath, notesPath: canonicalPath, reason: 'no root duplicate' }; | ||
| } | ||
|
|
||
| if (rootStat.size !== 0) { | ||
| return { checked: true, repaired: false, rootPath, notesPath: canonicalPath, reason: 'root duplicate is not zero-byte' }; | ||
| } | ||
|
|
||
| const canonicalStat = fileStat(canonicalPath); | ||
| if (!canonicalStat || canonicalStat.size === 0) { | ||
| return { checked: true, repaired: false, rootPath, notesPath: canonicalPath, reason: 'matching populated Notes file not found' }; | ||
| } | ||
|
|
||
| fs.unlinkSync(rootPath); | ||
| return { checked: true, repaired: true, rootPath, notesPath: canonicalPath, reason: 'removed zero-byte vault-root duplicate' }; | ||
| } |
There was a problem hiding this comment.
Make the repair best-effort so cleanup failures don't abort the primary flow.
This guard runs inline on the main write path (write-to-vault.js line 133, after the note is already persisted) and the journal-link path (link-to-journal.js line 57). Both fileStat (which rethrows any non-ENOENT stat error) and fs.unlinkSync can throw on transient/permission conditions (e.g., EPERM/EBUSY/races). If that happens, writeNoteFile/linkNoteToJournal will throw and report the entire operation as failed even though the note was written successfully. Since this is a cleanup guard, consider catching and surfacing the error as a non-fatal status instead of propagating.
🛡️ One option: wrap the unlink so failures degrade to a status
- fs.unlinkSync(rootPath);
- return { checked: true, repaired: true, rootPath, notesPath: canonicalPath, reason: 'removed zero-byte vault-root duplicate' };
+ try {
+ fs.unlinkSync(rootPath);
+ } catch (error) {
+ return { checked: true, repaired: false, rootPath, notesPath: canonicalPath, reason: `unlink failed: ${error.code || error.message}` };
+ }
+ return { checked: true, repaired: true, rootPath, notesPath: canonicalPath, reason: 'removed zero-byte vault-root duplicate' };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const rootStat = fileStat(rootPath); | |
| if (!rootStat) { | |
| return { checked: true, repaired: false, rootPath, notesPath: canonicalPath, reason: 'no root duplicate' }; | |
| } | |
| if (rootStat.size !== 0) { | |
| return { checked: true, repaired: false, rootPath, notesPath: canonicalPath, reason: 'root duplicate is not zero-byte' }; | |
| } | |
| const canonicalStat = fileStat(canonicalPath); | |
| if (!canonicalStat || canonicalStat.size === 0) { | |
| return { checked: true, repaired: false, rootPath, notesPath: canonicalPath, reason: 'matching populated Notes file not found' }; | |
| } | |
| fs.unlinkSync(rootPath); | |
| return { checked: true, repaired: true, rootPath, notesPath: canonicalPath, reason: 'removed zero-byte vault-root duplicate' }; | |
| } | |
| const rootStat = fileStat(rootPath); | |
| if (!rootStat) { | |
| return { checked: true, repaired: false, rootPath, notesPath: canonicalPath, reason: 'no root duplicate' }; | |
| } | |
| if (rootStat.size !== 0) { | |
| return { checked: true, repaired: false, rootPath, notesPath: canonicalPath, reason: 'root duplicate is not zero-byte' }; | |
| } | |
| const canonicalStat = fileStat(canonicalPath); | |
| if (!canonicalStat || canonicalStat.size === 0) { | |
| return { checked: true, repaired: false, rootPath, notesPath: canonicalPath, reason: 'matching populated Notes file not found' }; | |
| } | |
| try { | |
| fs.unlinkSync(rootPath); | |
| } catch (error) { | |
| return { checked: true, repaired: false, rootPath, notesPath: canonicalPath, reason: `unlink failed: ${error.code || error.message}` }; | |
| } | |
| return { checked: true, repaired: true, rootPath, notesPath: canonicalPath, reason: 'removed zero-byte vault-root duplicate' }; | |
| } |
🤖 Prompt for 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.
In
`@modules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/src/lib/vault-root-duplicate-guard.js`
around lines 56 - 72, The guard currently lets fileStat and fs.unlinkSync throw
and abort the calling flow; modify the function that uses fileStat and
fs.unlinkSync (vault-root-duplicate-guard.js) to make cleanup best-effort by
wrapping the fileStat(canonicalPath) and fs.unlinkSync(rootPath) calls in
try/catch blocks: if fileStat throws treat it as a non-fatal “not repaired”
result (return checked:true, repaired:false and include the error message in
reason), and when unlinkSync throws catch the error, log or include the error in
the returned reason and return checked:true, repaired:false instead of
rethrowing; keep existing successful-path behavior when no errors occur.
Summary
Landing rationale
This is the portable jarvOS fix. The local clawd copy needs the same active-runtime backport until this upstream change is merged and consumed downstream.
Verification
Summary by CodeRabbit
Bug Fixes
Tests