feat: add WINDOWS_MODE for a vault on a Windows C: drive#164
Merged
Conversation
Running vault-cortex through Docker Desktop with the vault on a Windows drive crosses the Docker Desktop <-> WSL2 bridge, where two things silently break: live re-indexing (chokidar's native inotify events don't cross the bridge) and vault_move_note (atomicWriteFileExclusive's hard-link write is unsupported). Reads, writes, and search still work. Add a single explicit env var, WINDOWS_MODE (default false), surfaced on VaultConfig as windowsBindMount and threaded through the existing config plumbing. When on, it deterministically switches both affected behaviors to bridge-safe variants: - file watcher uses chokidar polling (interval baked at 300ms) - atomicWriteFileExclusive uses a check-then-rename no-clobber write instead of a hard link (preserving the EEXIST contract for moveNote) The user-facing name is plain so a non-technical Windows user can flip it without knowing what a bind mount is; it's safe to leave on for any Windows setup. Relocate fileExists into vault-filesystem (beside the other path helpers) so the rename strategy can reuse it. Docs scoped to the local deploy + the canonical README table; remote (named volumes) is unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ZAjKyViw114zXf95QfWcN
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…ion) The hardLinksUnsupported fallback used check-then-rename, which has a TOCTOU window: a file created between the existence check and rename() could be clobbered, losing data under concurrent vault_move_note calls. Reserve the destination atomically with an O_EXCL create (the 'wx' flag) instead — which throws EEXIST race-free and is far more portable than hard links (it's link() specifically that the bridge can't do) — then rename the staged temp over the empty placeholder so the content still lands atomically, cleaning up the placeholder if the swap fails. This also drops the synthesized EEXIST (the real one now propagates) and the fallback's need for fileExists, so revert that helper back to private in note-mover rather than exporting it from vault-filesystem. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ZAjKyViw114zXf95QfWcN
Sweep of the PR's new tests against the test conventions: - config: the fail-fast test used a bare toThrow(); pin it to /WINDOWS_MODE/ so it asserts the env-var parse rejected, not just that something threw. - note-mover: the Windows-mode move test asserted two result fields; switch to a whole-object toEqual (matching the sibling test). Rename both Windows-mode tests to describe the configured mode rather than imply the assertion proves the rename code path — the no-clobber refusal is enforced by moveNote's preflight here, while the rename-path O_EXCL/wx guard is unit-tested in vault-filesystem.test.ts. Note that in the describe comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ZAjKyViw114zXf95QfWcN
Owner
Author
|
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughIntroduces ChangesWINDOWS_MODE feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
aliasunder
commented
Jun 21, 2026
aliasunder
commented
Jun 21, 2026
Address review: the exclusive-write fallback flag reads clearer named for its positive state. Rename atomicWriteFileExclusive's option from `hardLinksUnsupported` to `hardLinksSupported` (default true), and flip the branch so the link fast-path uses a positive condition and an early return, with the rename fallback flowing beneath it — no `else`. note-mover passes `hardLinksSupported: !windowsBindMount`; tests updated to match. No behavior change. Also codify the affirmative-boolean-naming convention in AGENTS.md alongside the early-return rule it pairs with. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ZAjKyViw114zXf95QfWcN
aliasunder
commented
Jun 21, 2026
Gate the `interval` watch option on `usePolling` instead of always passing it. interval only drives chokidar's polling backend; passing it unconditionally relied on chokidar ignoring it for native events, an implementation detail a future release needn't preserve. Now interval is present only when we've explicitly opted into polling, so a change to how chokidar treats an interval set without usePolling can't affect us. No behavior change on either path today. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ZAjKyViw114zXf95QfWcN
Add a focused unit suite that mocks chokidar's watch() and asserts the exact options startFileWatcher builds: interval is omitted unless usePolling is true, and is 300ms when polling. Kept separate from file-watcher.test.ts so the real-chokidar integration tests there are unaffected. Both directions are mutation-verified (always-pass fails the omit tests; never-pass fails the polling test). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ZAjKyViw114zXf95QfWcN
tsc (which the CI `checks` job and the Docker build:server both run over
src/**, tests included) rejected the stub's return-type annotation:
`{ on: (event: string) => unknown }` declares a 1-arg `on`, but the method
takes two (event + handler), so the object wasn't assignable (TS2322). Give
it a proper recursive `FakeWatcher` type so `.on` stays chainable. This also
clears trivy-pr, which failed only because the image build cascaded from the
same tsc error.
Runtime behavior unchanged; vitest (esbuild) didn't catch it — re-ran
`npm run build` to confirm.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011ZAjKyViw114zXf95QfWcN
A separate test file with no matching source file was a smell. Merge the
chokidar-options assertions back into file-watcher.test.ts using
vi.mock("chokidar", { spy: true }): spy mode keeps the real implementation so
the existing integration tests still watch real temp dirs, while the new
"watch options" suite overrides watch() per-test (vi.mocked + mockImplementation)
to inspect the options object, then mockRestore hands the real, spied watch
back. Gating is still mutation-verified through the spied tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011ZAjKyViw114zXf95QfWcN
watchOptionsFor was ambiguous — "options" meant both the FileWatcherOptions input and the chokidar watch options output. Rename to chokidarOptionsFrom, the param to watcherOptions, and the locals to chokidarOptions, so input and output read distinctly. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ZAjKyViw114zXf95QfWcN
aliasunder
pushed a commit
that referenced
this pull request
Jun 21, 2026
Resolve server.json: keep this branch's "25 tools" description with main's 0.20.2 version. Everything else auto-merged — #167 patchNote duplicate-heading rejection, #165 link-domain module, and #164 WINDOWS_MODE integrate cleanly alongside vault_delete_span. Tool-count references are a consistent 25; full suite green (898 tests), lint, prettier, and build all pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Running vault-cortex through Docker Desktop with the vault on a Windows drive (
C:\Users\you\Vault) crosses the Docker Desktop ↔ WSL2 filesystem bridge, where two things silently break:vault_move_notefails —atomicWriteFileExclusivecreates the destination withfs.link()(hard link, for atomic no-clobber), which the bridge can't do, solink()throws and the move aborts.Reads, writes, and search keep working. Until now the only fix was the documented "put your vault in WSL2 ext4" workaround. This makes a plain
C:mount work out of the box.Approach — one explicit flag
A single env var,
WINDOWS_MODE(defaultfalse), surfaced aswindowsBindMountonVaultConfigand threaded through the existing config plumbing (no new wiring). When on, it deterministically switches both affected behaviors to bridge-safe variants:usePollingwith a baked-in 300msinterval(re-index latency is already governed by the 2000msawaitWriteFinishwindow, so the perceived cost is negligible).atomicWriteFileExclusivetakes ahardLinksUnsupportedoption that switches to a check-then-renameno-clobber write, synthesizingEEXISTsomoveNote's existing catch is unchanged.fileExistsrelocated intovault-filesystem.tsbeside the other path helpers for reuse.windows bind-mount mode enabledonce.The user-facing name is deliberately plain so a non-technical Windows user can flip it without knowing what a bind mount is; the precise
windowsBindMountname lives internally. It's safe to leave on for any Windows setup (polling and rename work on every filesystem — just marginally more CPU), so the guidance is foolproof.The move path is now deterministic (the flag picks rename vs. link) rather than attempting
link()and sniffing error codes — no fragile error-code set, no per-call fallback.Scope
cli/templates/local/docker-compose.yml(byte-identical bundle) vianpm run sync:cli-templates.Tests
npm run build,npm run lint, andnpm testall green — 838 tests (+12 new) across config (WINDOWS_MODEparse + default + fail-fast), watcher (indexes under polling),vault-filesystem(rename-strategy writes + no-clobber +fileExists), andnote-mover(windows-mode move + existing-destination refusal). Each new behavior was mutation-checked: disabling the rename-path no-clobber guard fails the EEXIST test for that reason, and since that guard lives only in the rename branch, it also confirms the branch is exercised.Not reproducible in CI: a real Windows + Docker Desktop +
C:-mount smoke test (no WSL2 bridge on Linux) — worth a manual pass before merge.🤖 Generated with Claude Code
https://claude.ai/code/session_011ZAjKyViw114zXf95QfWcN
Generated by Claude Code
Summary by CodeRabbit
New Features
Documentation