Add Yrs CRDT library and cross-platform dev workflow with Windows junction support #45
Conversation
Co-authored-by: TWDickson <3435304+TWDickson@users.noreply.github.com>
…upport Co-authored-by: TWDickson <3435304+TWDickson@users.noreply.github.com>
…curity improvements Co-authored-by: TWDickson <3435304+TWDickson@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (IS_WINDOWS) { | ||
| // On Windows, use rmdir for junctions - safer than spawnSync | ||
| try { | ||
| const result = spawnSync('cmd', ['/c', 'rmdir', targetPluginDir], { stdio: 'ignore' }); |
Check warning
Code scanning / CodeQL
Shell command built from environment values Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 14 days ago
In general, the safest fix is to avoid calling external shell commands with untrusted or environment-derived values. Instead, use Node’s filesystem APIs (fs.unlinkSync, fs.rmSync, etc.) to operate on paths directly, or if you must call external programs, ensure they are invoked without a shell and with parameters passed as separate arguments.
For this specific code, we can remove the Windows-specific spawnSync('cmd', ['/c', 'rmdir', targetPluginDir], { stdio: 'ignore' }) and use Node’s fs.rmSync / fs.unlinkSync for junctions as well. The comment says “On Windows, use rmdir for junctions - safer than spawnSync”, but modern Node (v14.14+ / v16+) supports removing directories and junctions recursively with fs.rmSync(targetPluginDir, { recursive: true, force: true }). We already use fs.rmSync for directories in the same file (line 82), so this change keeps behavior consistent and removes the need for any shell call.
Concretely:
- In
scripts/symlink-plugin.mjs, in the block that handles removal of an existing symlink/junction for Windows (if (IS_WINDOWS)), replace thespawnSync-based logic and fallbackfs.unlinkSynccalls with a single call tofs.rmSync(targetPluginDir, { recursive: true, force: true })wrapped in a try/catch that falls back tofs.unlinkSyncif needed. - No new imports are required;
fsis already imported at the top. - No other files or regions need to be changed.
| @@ -65,13 +65,11 @@ | ||
| if (stats.isSymbolicLink()) { | ||
| console.log('⚠️ Removing existing symlink/junction...'); | ||
| if (IS_WINDOWS) { | ||
| // On Windows, use rmdir for junctions - safer than spawnSync | ||
| // On Windows, remove junctions/symlinks using filesystem APIs to avoid shell usage | ||
| try { | ||
| const result = spawnSync('cmd', ['/c', 'rmdir', targetPluginDir], { stdio: 'ignore' }); | ||
| if (result.status !== 0) { | ||
| fs.unlinkSync(targetPluginDir); | ||
| } | ||
| fs.rmSync(targetPluginDir, { recursive: true, force: true }); | ||
| } catch (err) { | ||
| // Fallback in case rmSync cannot handle this link type | ||
| fs.unlinkSync(targetPluginDir); | ||
| } | ||
| } else { |
| const result = spawnSync('cmd', ['/c', 'mklink', '/J', targetPluginDir, projectRoot], { | ||
| stdio: 'inherit' | ||
| }); |
Check warning
Code scanning / CodeQL
Shell command built from environment values Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 14 days ago
In general, the safest fix is to avoid invoking shell utilities when Node’s standard library can perform the same action. Here, spawnSync('cmd', ['/c', 'rmdir', ...]) is already called earlier only as a fallback for removing a junction, and the main flagged call spawnSync('cmd', ['/c', 'mklink', '/J', targetPluginDir, projectRoot], ...) attempts to create a junction that can also be created via fs.symlinkSync with type 'junction' on Windows. So the robust fix is to stop using cmd/mklink and instead use Node’s fs.symlinkSync for junction creation. This removes the shell command entirely and with it any dependence on tainted paths in a shell context.
Concretely, in scripts/symlink-plugin.mjs, within the try block that starts on line 87, we will replace the inner try { ... spawnSync('cmd', ...) ... } catch (junctionErr) { ... } structure with a single direct call to fs.symlinkSync(projectRoot, targetPluginDir, 'junction'); followed by the success logging. This preserves the documented behavior (“Windows junction support (no admin required!)”), as fs.symlinkSync with type 'junction' on Windows creates a directory junction without requiring admin privileges when supported. If it fails (e.g., due to permissions), the outer catch (err) that already exists for the entire try block will handle and report the failure, so we do not lose any error handling logic. No new imports or helper functions are required; fs is already imported at the top of the file, and we are not adding any third-party packages.
| @@ -86,23 +86,10 @@ | ||
| // Create symlink or junction | ||
| try { | ||
| if (IS_WINDOWS) { | ||
| // Try junction first (works without admin) | ||
| console.log(' Attempting Windows junction (no admin needed)...'); | ||
| try { | ||
| // Use spawnSync with array arguments to avoid command injection | ||
| const result = spawnSync('cmd', ['/c', 'mklink', '/J', targetPluginDir, projectRoot], { | ||
| stdio: 'inherit' | ||
| }); | ||
| if (result.status !== 0) { | ||
| throw new Error('Junction creation failed'); | ||
| } | ||
| console.log('✅ Junction created successfully!\n'); | ||
| } catch (junctionErr) { | ||
| // Fallback to symlink (requires admin) | ||
| console.log(' Junction failed, trying symlink (requires admin)...'); | ||
| fs.symlinkSync(projectRoot, targetPluginDir, 'junction'); | ||
| console.log('✅ Symlink created successfully!\n'); | ||
| } | ||
| // Create a junction on Windows (works without admin on supported systems) | ||
| console.log(' Creating Windows junction (no admin needed)...'); | ||
| fs.symlinkSync(projectRoot, targetPluginDir, 'junction'); | ||
| console.log('✅ Junction created successfully!\n'); | ||
| } else { | ||
| // Mac/Linux use regular symlink | ||
| fs.symlinkSync(projectRoot, targetPluginDir, 'dir'); |
|
@copilot Your PR is creating folders like this: |
Could you clarify the concern? The script creates Should I:
|
|
@copilot So directories are showing up with extra characters on windows. I'm seeing 'vault-a[char]' windows is rendering a dot while VS code and copying here display a missing character. Lets' also include a bit of documentation around how to actually build and run this |
…add comprehensive build documentation Co-authored-by: TWDickson <3435304+TWDickson@users.noreply.github.com>
Fixed in commit 420cdac: Windows display issue: Removed Documentation: Added comprehensive
The script now only creates directories and fixture files - no hidden files that could cause display issues on Windows. |
- Check if directories exist before creating (don't overwrite vault-a) - Only create fixture files if they don't exist - Add .gitkeep to vault-b for git tracking - Improve logging to show existing vs created items - Update README to clarify git tracking strategy
✅ Implementation Complete: Issue #23 - Add Yrs CRDT and Cross-Platform Dev Workflow
1. Add Yrs Dependency to WASM Module ✅
yrs = "0.21"to wasm/Cargo.tomlyrs::Docin wasm/src/lib.rsverify_yrs()function with Rust tests (3 tests)2. TypeScript Integration ✅
3. Cross-Platform Dev Scripts ✅
4. Documentation ✅
5. Testing & Verification ✅
6. Final Validation ✅
🎯 Summary
Original prompt
Add Yrs CRDT Dependency and Cross-Platform Dev Workflow
Fixes #23
Overview
This PR adds the Yrs CRDT library to the WASM module and enhances the development workflow to be fully cross-platform, with special support for Windows junctions.
Changes
1. Add Yrs Dependency (Issue #23)
wasm/Cargo.toml:
yrs = "0.21"dependencywasm/src/lib.rs:
yrs::Docto verify compilationverify_yrs()function that creates a Yrs documenttest_verify_yrs()- Verify success messagetest_verify_yrs_creates_doc()- Verify Doc creation doesn't panictest_verify_yrs_wasm()- WASM-specific browser testtest/unit/wasm-yrs-integration.test.ts (NEW):
verify_yrs()main.ts:
verify_yrsfrom WASM moduleverify_yrs()during WASM initialization to log status2. Cross-Platform Dev Workflow
scripts/symlink-plugin.mjs (ENHANCED):
process.platform)OBSIDIAN_TEST_VAULTenvironment variablescripts/setup-test-vaults.mjs (NEW - REPLACES .sh):
scripts/setup-test-vaults.shfrom Bash to Node.jsfsandpathscripts/copy-plugin.mjs (NEW):
.vault-pathpackage.json:
setup:test-vaultsto use Node.js script instead of bashcopy:pluginscript for Windows fallbackdev:copyscript for auto-copy on changes (alternative to symlink workflow)3. Testing
All tests pass:
npm run test:rustnpm testCoverage:
Benefits
For Issue #23:
verify_yrs()function proves library worksFor Cross-Platform Development:
Testing Instructions
1. Test Yrs Integration
2. Test Cross-Platform Workflow
Mac/Linux:
npm run symlink:plugin -- ~/Documents/ObsidianVault npm run devWindows:
npm run symlink:plugin -- C:\Users\YourName\Documents\ObsidianVault npm run devWindows Fallback (if junctions fail):
npm run build npm run copy:plugin -- C:\Users\YourName\Documents\ObsidianVault3. Verify in Obsidian
Migration Notes
Bash → Node.js
scripts/setup-test-vaults.sh→scripts/setup-test-vaults.mjsWindows Junctions
Files Changed
Modified:
wasm/Cargo.toml- Add yrs dependencywasm/src/lib.rs- Add verify_yrs() function and testsmain.ts- Import and use verify_yrs()package.json- Update scripts for cross-platform supportscripts/symlink-plugin.mjs- Add junction supportAdded:
test/unit/wasm-yrs-integration.test.ts- TypeScript integration testsscripts/setup-test-vaults.mjs- Node.js version of setup script...This pull request was created from Copilot chat.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.