From 9f161959e562bd56b545fc996d3ac77ee0bbe6ce Mon Sep 17 00:00:00 2001 From: Nicholai Vogel Date: Thu, 17 Sep 2026 01:57:31 -0600 Subject: [PATCH 1/8] Add native Windows filesystem compatibility --- .github/workflows/windows.yml | 41 +++ README.md | 10 +- src/config_editor.rs | 91 +++++- src/filesystem.rs | 567 ++++++++++++++++++++++++++++++++-- src/recovery.rs | 6 +- tests/test_harness.ts | 5 +- 6 files changed, 674 insertions(+), 46 deletions(-) create mode 100644 .github/workflows/windows.yml diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml new file mode 100644 index 0000000..c511fce --- /dev/null +++ b/.github/workflows/windows.yml @@ -0,0 +1,41 @@ +name: Windows + +on: + pull_request: + push: + branches: + - main + +permissions: + contents: read + +jobs: + test: + name: Windows build and integration tests + runs-on: windows-latest + steps: + - name: Check out repository + uses: actions/checkout@v4 + + - name: Install Rust + uses: dtolnay/rust-toolchain@stable + + - name: Install Bun + uses: oven-sh/setup-bun@v2 + with: + bun-version: 1.4.2 + + - name: Build native CLI + run: cargo build --locked + + - name: Run Rust tests + run: cargo test --locked + + - name: Install JavaScript test dependencies + run: bun install --frozen-lockfile + + - name: Type-check JavaScript harness + run: bun x tsc --noEmit + + - name: Run Windows integration tests + run: bun test diff --git a/README.md b/README.md index 7049779..64bead3 100644 --- a/README.md +++ b/README.md @@ -47,13 +47,13 @@ Subscribe records a stable source-plus-relative-path relationship key, so same-n `restore --from PATH` is an explicit, noninteractive recovery operation. PATH must be one durable deletion recovery directory beneath the configured `recovery/` root and contain a strict `package/` snapshot; the manifest supplies the skill name unless matching `--skill NAME` is provided. It stages and validates the complete no-follow snapshot, preserves operational-looking files and Unix modes, and installs only when the canonical destination is absent. Existing differing content, malformed or linked content, path escapes, and source/destination overlap fail without overwrite. An identical validated destination returns `already_present`. Restore rehydrates canonical package content only: it never recreates subscriptions, publications, harness links, sets, or remote effects; use their explicit commands separately. -State loads strictly validate every local adoption record (identity, absolute no-follow source, canonical library destination, hash, and status); tampered records are rejected before status, doctor, or mutation. `SKILL.md` must begin with exactly one unquoted `name: ` line; prose or duplicate/conflicting names are rejected. Imports stage content and roll back the newly installed package if provenance state cannot be persisted. Skill names and source-relative package paths reject absolute paths, parent traversal, separators where a package key is required, and control characters. Symlinks are rejected; regular-file scans use no-follow opens on Unix, and live package/publication replacements are staged and swapped through an anchored directory rename. Managed snapshots are rechecked before commit or replacement and abort when a concurrent change is detected; import installation uses an anchored no-replace primitive (Linux `renameat2`, Windows native no-overwrite move) and fails closed where unavailable. External writers are not fully serialized. Bundled files are never executed. `SKILLSYNC_TEST_FAIL_STATE_SAVE=1` is a test-only failure-injection hook for exercising rollback; it is not a configuration or recovery bypass. +State loads strictly validate every local adoption record (identity, absolute no-follow source, canonical library destination, hash, and status); tampered records are rejected before status, doctor, or mutation. `SKILL.md` must begin with exactly one unquoted `name: ` line; prose or duplicate/conflicting names are rejected. Imports stage content and roll back the newly installed package if provenance state cannot be persisted. Skill names and source-relative package paths reject absolute paths, parent traversal, separators where a package key is required, and control characters. Symlinks and Windows reparse points are rejected; regular-file scans use no-follow opens on Unix and handle-bound reparse checks on Windows, and live package/publication replacements are staged through identity-checked native directory moves. Managed snapshots are rechecked before commit or replacement and abort when a concurrent change is detected; import installation uses an anchored no-replace primitive (Linux `renameat2`, Windows native no-overwrite move) and fails closed where unavailable. External writers are not fully serialized. Bundled files are never executed. `SKILLSYNC_TEST_FAIL_STATE_SAVE=1` is a test-only failure-injection hook for exercising rollback; it is not a configuration or recovery bypass. Package copies exclude operational files such as `.env`, logs, credential directories, and private keys. Publication updates managed files in `skills/` without deleting the destination package wholesale, preserving unrelated operational files. Destination changes that cannot be attributed to the recorded publication are rejected. Publication records are written only after a meaningful commit and successful push; a durable pending-publication intent is written before each push so a failed push or post-push state write can be retried without losing the relationship. Mutating commands share a persistent advisory lock in the config directory; the lock is held for a whole operation or worker run, and status probes the lock rather than trusting a stale PID. Updates compare the durable baseline, live package, and fetched upstream package. `conflicts list` inventories paused conflicts, and `conflicts show RELATIONSHIP` is inspection-only: it validates and displays an immutable manifest with base/local/incoming hashes and evidence paths. Manual side selection, resolve, resume, and isolated workspace editing remain explicitly deferred; no semantic or automatic conflict resolution is performed. Deletion recovery remains separate via `restore`. -`config edit` is supported only from an interactive terminal. It creates a uniquely named no-follow temporary file inside the config directory, invokes `VISUAL` before `EDITOR` as an executable with that temporary path as its sole argument, then validates and publishes the result without replacing a config created concurrently. Existing configs are published only when their identity and bytes remain unchanged; symlink/reparse substitutions and editor-replaced temporary files fail closed. It inherits terminal streams and reports editor failures. Native Windows publication deliberately fails closed and is unavailable in this implementation; native Windows runtime is not exercised here. It never parses editor command strings or runs through a shell; `--json` and redirected stdin/stdout fail instead of launching an editor. +`config edit` is supported only from an interactive terminal. It creates a uniquely named no-follow temporary file inside the config directory, invokes `VISUAL` before `EDITOR` as an executable with that temporary path as its sole argument, then validates and publishes the result without replacing a config created concurrently. Existing configs are published only when their identity and bytes remain unchanged; symlink/reparse substitutions and editor-replaced temporary files fail closed. It inherits terminal streams and reports editor failures. Unix publishes through directory handles; Windows publishes through reparse-protected file handles and verifies the directory/file identity before and after publication. It never parses editor command strings or runs through a shell; `--json` and redirected stdin/stdout fail instead of launching an editor. ## Testing the native binary, then run the TypeScript integration suite with Bun: @@ -61,14 +61,14 @@ Updates compare the durable baseline, live package, and fetched upstream package cargo build bun install bun test -bunx tsc --noEmit +bun x tsc --noEmit ``` The Bun suite uses isolated temporary Git repositories and configuration roots. It covers nested selection, default-branch checkout, local/upstream merge preservation, conflict recovery, scoped publication, operational-file retention, multiple publication destinations, unsubscribe/unpublish retention, foreground worker locking, JSON failures, and tampered state paths. ## Honest limits -The explicit harness integration is deliberately narrow: `harness link` creates one native directory symlink from a caller-supplied existing harness skill root to one canonical library skill, and edits through that link write back to the canonical package. `harness unlink` removes only a recorded link whose target still resolves to the expected canonical skill; canonical content is retained. Existing unrelated harness files are preserved and collisions, symlink/reparse roots, and unsafe names are rejected. `harness list`, `status`, and `doctor` report these relationships. Skillsync does not currently discover harnesses automatically, filter harness skills, reload running harnesses, curate Hermes autonomous learning, or promise universal harness compatibility; there is no silent copy fallback. Windows uses native directory symlinks and reports privilege/API failures. Windows directory-link and import runtime behavior is compile-checked in this environment but not runtime exercised; native Windows verification is not claimed. +The explicit harness integration is deliberately narrow: `harness link` creates one native directory symlink from a caller-supplied existing harness skill root to one canonical library skill, and edits through that link write back to the canonical package. `harness unlink` removes only a recorded link whose target still resolves to the expected canonical skill; canonical content is retained. Existing unrelated harness files are preserved and collisions, symlink/reparse roots, and unsafe names are rejected. `harness list`, `status`, and `doctor` report these relationships. Skillsync does not currently discover harnesses automatically, filter harness skills, reload running harnesses, curate Hermes autonomous learning, or promise universal harness compatibility; there is no silent copy fallback. Windows uses native directory symlinks and reports privilege/API failures. Windows build, CLI integration, import, replacement, recovery, publication, locking, and launcher paths run in the hosted `windows-latest` workflow; interactive editor behavior still requires a real terminal session. The foreground worker is supported for `worker --once` and `worker --interval SECONDS`; it owns the same advisory state lock as mutating CLI commands. Named local sets are supported for organizing canonical library skills (`set create/list/show/add/remove`); membership is portable state only. Set publication, set subscription metadata, personal-library sync, and membership-change propagation remain unsupported. The TUI is intentionally read-only: it has no onboarding, mutation actions, worker registration, or semantic conflict resolution, and its capability display is not a claim that those operations are available. Registry integration, Hermes autonomous curation, and automatic harness discovery/filtering/reload remain unsupported. Semantic conflict resolution, source-specific authentication, and cross-device worker coordination remain unsupported. `unsubscribe` and `unpublish` remove only the relationship and retain installed/published content. `delete NAME --yes` is the only canonical-library deletion surface: it fails closed for active subscriptions, publications, harness links, or set membership, stages a complete recoverable package snapshot under `recovery/`, then removes only that package. Safe no-replace directory quarantine is verified on Linux (`renameat2`) and Windows (native no-overwrite move); on other Unix platforms deletion fails closed because this runtime has no portable atomic no-replace directory primitive. Set membership is intentionally not auto-removed; remove the skill from every set first. Harness set enablement is an explicit one-time expansion: it records the set name, root, and member identities, creates only native per-skill directory links, and later set membership changes are not propagated. `harness disable` removes only those recorded links after target validation and retains canonical content. Empty named sets are valid no-op enablements: they persist and reload as healthy relationships, repeated enablement is `already_enabled`, and disable removes only the relationship without filesystem mutation. Automatic harness discovery/filtering/reload remains unsupported. Repeated or missing deletion is reported as `already_absent` and never affects unrelated paths. @@ -76,4 +76,4 @@ The foreground worker is supported for `worker --once` and `worker --interval SE `npm/` contains a private, development-only npm launcher package. Its `skillsync` bin forwards arguments and standard streams to a bundled platform/architecture native binary, or to the explicit `SKILLSYNC_NATIVE_BIN` override used by tests and deployments. It never searches `PATH` or an npx cache, downloads artifacts, installs a worker, or registers startup. Missing artifacts fail clearly. -This is launcher plumbing only, not a published `skillsync` npm distribution. Package ownership, release publication, and the precompiled native artifact matrix remain unresolved; no native artifacts are bundled in this repository. Durable installation and sign-in startup registration remain unsupported. \ No newline at end of file +This is launcher plumbing only, not a published `skillsync` npm distribution. Package ownership, release publication, and the precompiled native artifact matrix remain unresolved; no native artifacts are bundled in this repository. Durable installation and sign-in startup registration remain unsupported. diff --git a/src/config_editor.rs b/src/config_editor.rs index 6c18936..f997bbe 100644 --- a/src/config_editor.rs +++ b/src/config_editor.rs @@ -41,7 +41,13 @@ fn same_object(a: &fs::Metadata, b: &fs::Metadata) -> bool { use std::os::unix::fs::MetadataExt; a.dev() == b.dev() && a.ino() == b.ino() } - #[cfg(not(unix))] + #[cfg(windows)] + { + use std::os::windows::fs::MetadataExt; + a.volume_serial_number() == b.volume_serial_number() + && a.file_index() == b.file_index() + } + #[cfg(not(any(unix, windows)))] { a.len() == b.len() && a.modified().ok() == b.modified().ok() } @@ -162,6 +168,8 @@ pub(crate) fn edit_config(a: &super::App, json: bool) -> Result Result Result<()> { + target.set_len(0)?; + target.rewind()?; + #[cfg(feature = "test-hooks")] + if std::env::var_os("SKILLSYNC_TEST_CONFIG_PUBLISH_FAILURE").is_some() { + std::env::remove_var("SKILLSYNC_TEST_CONFIG_PUBLISH_FAILURE"); + return Err(anyhow!("injected config publication failure")); + } + target.write_all(&edited)?; + target.sync_all()?; + target.rewind()?; + let mut verified = Vec::new(); + target.read_to_end(&mut verified)?; + if verified != edited { + return Err(anyhow!("published config bytes do not match the editor result")); + } + Ok(()) + })(); + if let Err(error) = publish_result { + if let Err(recovery) = restore_target(&mut target, original_bytes) { + return Err(anyhow!("recovery required: {error}; restoring original config failed: {recovery}")); + } + return Err(error); + } + verify_original_unchanged(&path, &(original_metadata.clone(), edited.clone())) + .map_err(|error| anyhow!("config publication target changed: {error}"))?; + } else { + let mut target = super::open_regular_file_bound(&path, true, true)?; + let publication = (|| -> Result<()> { + target.write_all(&edited)?; + target.sync_all()?; + target.rewind()?; + let mut verified = Vec::new(); + target.read_to_end(&mut verified)?; + if verified != edited { + return Err(anyhow!("published config bytes do not match the editor result")); + } + Ok(()) + })(); + if let Err(error) = publication { + drop(target); + let _ = fs::remove_file(&path); + return Err(error); + } } - return Err(anyhow!("native Windows config publication is unavailable: no safe handle-anchored replacement primitive is guaranteed")); + } + #[cfg(not(any(unix, windows)))] + { + let _ = config_identity; + return Err(anyhow!("native config publication is unavailable on this platform")); } #[cfg(unix)] if original.is_none() { @@ -343,13 +405,8 @@ fn verify_original_unchanged(path: &Path, original: &(fs::Metadata, Vec)) -> if !current_metadata.is_file() || current_metadata.file_type().is_symlink() { return Err(anyhow!("config path changed while it was being edited")); } - #[cfg(unix)] - { - use std::os::unix::fs::MetadataExt; - if current_metadata.dev() != original.0.dev() || current_metadata.ino() != original.0.ino() - { - return Err(anyhow!("config path changed while it was being edited")); - } + if !same_object(¤t_metadata, &original.0) { + return Err(anyhow!("config path changed while it was being edited")); } let current = super::read_regular_file(path, Some(&original.0))?; if current != original.1 { @@ -358,7 +415,7 @@ fn verify_original_unchanged(path: &Path, original: &(fs::Metadata, Vec)) -> Ok(()) } -#[cfg(unix)] +#[cfg(any(unix, windows))] fn restore_target(target: &mut fs::File, original: &[u8]) -> Result<()> { target.set_len(0)?; target.rewind()?; diff --git a/src/filesystem.rs b/src/filesystem.rs index a9347c4..1fda4ed 100644 --- a/src/filesystem.rs +++ b/src/filesystem.rs @@ -2,7 +2,7 @@ use crate::{unique_stamp, App}; use anyhow::{anyhow, Context, Result}; use fs2::FileExt; use sha2::{Digest, Sha256}; -#[cfg(unix)] +#[cfg(any(unix, windows))] use std::io::Read; #[cfg(unix)] use std::os::unix::fs::PermissionsExt; @@ -152,12 +152,48 @@ fn open_directory_file_no_create(path: &Path) -> Result { /// Bind a pathname lookup to the directory identity observed immediately before it. /// The retained descriptor is used after the comparison; replacements fail closed. -#[cfg(not(unix))] +#[cfg(windows)] pub(crate) fn open_directory_file_bound(path: &Path) -> Result { - Err(anyhow!( - "safe descriptor-relative traversal unavailable on this platform: {}", - path.display() - )) + use std::{iter, os::windows::ffi::OsStrExt, os::windows::io::FromRawHandle}; + use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE; + use windows_sys::Win32::Storage::FileSystem::{ + CreateFileW, FILE_FLAG_BACKUP_SEMANTICS, FILE_FLAG_OPEN_REPARSE_POINT, + FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE, GENERIC_READ, OPEN_EXISTING, + }; + + let expected = fs::symlink_metadata(path)?; + if expected.file_type().is_symlink() || !expected.is_dir() { + return Err(anyhow!("regular directory required: {}", path.display())); + } + reject_reparse_point(path, "directory")?; + let wide = path + .as_os_str() + .encode_wide() + .chain(iter::once(0)) + .collect::>(); + let handle = unsafe { + CreateFileW( + wide.as_ptr(), + GENERIC_READ, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + std::ptr::null(), + OPEN_EXISTING, + FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT, + std::ptr::null_mut(), + ) + }; + if handle == INVALID_HANDLE_VALUE { + return Err(std::io::Error::last_os_error().into()); + } + let opened = unsafe { fs::File::from_raw_handle(handle as _) }; + let actual = opened.metadata()?; + if !actual.is_dir() + || actual.file_type().is_symlink() + || windows_metadata_identity(&actual) != windows_metadata_identity(&expected) + { + return Err(anyhow!("directory changed during open: {}", path.display())); + } + Ok(opened) } #[cfg(unix)] @@ -179,6 +215,84 @@ pub(crate) fn open_directory_file_bound(path: &Path) -> Result { Ok(opened) } +#[cfg(windows)] +fn windows_metadata_identity(metadata: &fs::Metadata) -> (u32, u32, u32) { + use std::os::windows::fs::MetadataExt; + ( + metadata.volume_serial_number(), + (metadata.file_index() >> 32) as u32, + metadata.file_index() as u32, + ) +} + +#[cfg(windows)] +fn windows_file_identity(file: &fs::File) -> Result<(u32, u32, u32)> { + use std::os::windows::io::AsRawHandle; + use windows_sys::Win32::Storage::FileSystem::{ + GetFileInformationByHandle, BY_HANDLE_FILE_INFORMATION, + }; + let mut info = unsafe { std::mem::zeroed::() }; + let ok = unsafe { GetFileInformationByHandle(file.as_raw_handle() as _, &mut info) != 0 }; + if !ok { + return Err(std::io::Error::last_os_error().into()); + } + Ok(( + info.dwVolumeSerialNumber, + info.nFileIndexHigh, + info.nFileIndexLow, + )) +} + +#[cfg(windows)] +pub(crate) fn windows_path_identity(path: &Path) -> Result<(u32, u32, u32)> { + let file = open_directory_file_bound(path)?; + windows_file_identity(&file) +} + +#[cfg(windows)] +pub(crate) fn open_regular_file_bound( + path: &Path, + write: bool, + create_new: bool, +) -> Result { + use std::{iter, os::windows::ffi::OsStrExt, os::windows::io::FromRawHandle}; + use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE; + use windows_sys::Win32::Storage::FileSystem::{ + CreateFileW, CREATE_NEW, FILE_ATTRIBUTE_NORMAL, FILE_FLAG_OPEN_REPARSE_POINT, + FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE, GENERIC_READ, GENERIC_WRITE, + OPEN_EXISTING, + }; + + reject_reparse_point(path, "regular file")?; + let wide = path + .as_os_str() + .encode_wide() + .chain(iter::once(0)) + .collect::>(); + let access = GENERIC_READ | if write { GENERIC_WRITE } else { 0 }; + let disposition = if create_new { CREATE_NEW } else { OPEN_EXISTING }; + let handle = unsafe { + CreateFileW( + wide.as_ptr(), + access, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + std::ptr::null(), + disposition, + FILE_ATTRIBUTE_NORMAL | FILE_FLAG_OPEN_REPARSE_POINT, + std::ptr::null_mut(), + ) + }; + if handle == INVALID_HANDLE_VALUE { + return Err(std::io::Error::last_os_error().into()); + } + let file = unsafe { fs::File::from_raw_handle(handle as _) }; + let metadata = file.metadata()?; + if !metadata.is_file() || metadata.file_type().is_symlink() { + return Err(anyhow!("regular file required: {}", path.display())); + } + Ok(file) +} + #[cfg(unix)] fn entry_mode_at( parent: std::os::fd::RawFd, @@ -227,6 +341,29 @@ fn verify_bound_parent(path: &Path, expected: &fs::File) -> Result<()> { Ok(()) } +#[cfg(windows)] +fn verify_bound_parent(path: &Path, expected: &fs::File) -> Result<()> { + let parent = path + .parent() + .ok_or_else(|| anyhow!("destination has no parent"))?; + let actual = open_directory_file_bound(parent)?; + if windows_file_identity(expected)? != windows_file_identity(&actual)? { + return Err(anyhow!( + "destination parent changed during replacement: {}", + parent.display() + )); + } + Ok(()) +} + +#[cfg(not(any(unix, windows)))] +pub(crate) fn open_directory_file_bound(path: &Path) -> Result { + Err(anyhow!( + "safe descriptor-relative traversal unavailable on this platform: {}", + path.display() + )) +} + #[cfg(unix)] fn destination_directory_exists( parent: &fs::File, @@ -340,7 +477,43 @@ pub(crate) fn open_child_file(config: &Path, name: &str, create: bool) -> Result Ok(unsafe { fs::File::from_raw_fd(fd) }) } -#[cfg(not(unix))] +#[cfg(windows)] +pub(crate) fn open_child_file(config: &Path, name: &str, create: bool) -> Result { + use std::{iter, os::windows::ffi::OsStrExt, os::windows::io::FromRawHandle}; + use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE; + use windows_sys::Win32::Storage::FileSystem::{ + CreateFileW, FILE_ATTRIBUTE_NORMAL, FILE_FLAG_OPEN_REPARSE_POINT, FILE_SHARE_DELETE, + FILE_SHARE_READ, FILE_SHARE_WRITE, GENERIC_READ, GENERIC_WRITE, OPEN_ALWAYS, + OPEN_EXISTING, + }; + assert_no_symlink_path(config, Path::new("."))?; + let path = config.join(name); + reject_reparse_point(&path, "advisory lock")?; + let wide = path + .as_os_str() + .encode_wide() + .chain(iter::once(0)) + .collect::>(); + let handle = unsafe { + CreateFileW( + wide.as_ptr(), + GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + std::ptr::null(), + if create { OPEN_ALWAYS } else { OPEN_EXISTING }, + FILE_ATTRIBUTE_NORMAL | FILE_FLAG_OPEN_REPARSE_POINT, + std::ptr::null_mut(), + ) + }; + if handle == INVALID_HANDLE_VALUE { + return Err(std::io::Error::last_os_error().into()); + } + let file = unsafe { fs::File::from_raw_handle(handle as _) }; + ensure_regular_file(&file, name)?; + Ok(file) +} + +#[cfg(not(any(unix, windows)))] pub(crate) fn open_child_file(config: &Path, name: &str, create: bool) -> Result { let mut options = fs::OpenOptions::new(); options.read(true).write(true).create(create); @@ -412,9 +585,15 @@ struct ConfigIdentity { dev: u64, #[cfg(unix)] ino: u64, - #[cfg(not(unix))] + #[cfg(windows)] + volume: u32, + #[cfg(windows)] + file_index_high: u32, + #[cfg(windows)] + file_index_low: u32, + #[cfg(not(any(unix, windows)))] len: u64, - #[cfg(not(unix))] + #[cfg(not(any(unix, windows)))] modified: Option, } @@ -431,7 +610,16 @@ fn config_identity(config: &Path) -> Result { ino: metadata.ino(), }) } - #[cfg(not(unix))] + #[cfg(windows)] + { + let (volume, file_index_high, file_index_low) = windows_path_identity(config)?; + Ok(ConfigIdentity { + volume, + file_index_high, + file_index_low, + }) + } + #[cfg(not(any(unix, windows)))] { Ok(ConfigIdentity { len: metadata.len(), @@ -446,7 +634,13 @@ impl ConfigIdentity { { self.dev == current.dev && self.ino == current.ino } - #[cfg(not(unix))] + #[cfg(windows)] + { + self.volume == current.volume + && self.file_index_high == current.file_index_high + && self.file_index_low == current.file_index_low + } + #[cfg(not(any(unix, windows)))] { self.len == current.len && self.modified == current.modified } @@ -478,6 +672,7 @@ impl StateLock { )?, config_identity(config)?, ); + #[cfg(unix)] if let Err(error) = file.try_lock_exclusive() { if error.kind() == std::io::ErrorKind::WouldBlock { return Err(anyhow!( @@ -579,7 +774,24 @@ fn open_existing_read_lock( ensure_regular_file(&file, "state.lock")?; Ok(Some((file, identity, Some(directory_file)))) } - #[cfg(not(unix))] + #[cfg(windows)] + { + let directory_file = match open_directory_file_bound(config) { + Ok(file) => file, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(None), + Err(error) => return Err(error), + }; + let identity = config_identity(config)?; + match open_regular_file_bound(&config.join(name), false, false) { + Ok(file) => Ok(Some((file, identity, None))), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + drop(directory_file); + Ok(None) + } + Err(error) => Err(error), + } + } + #[cfg(not(any(unix, windows)))] { match fs::OpenOptions::new().read(true).open(config.join(name)) { Ok(file) => Ok(Some((file, config_identity(config)?, None))), @@ -805,6 +1017,7 @@ pub(crate) fn atomic_portable(p: &Path, b: &[u8]) -> Result<()> { assert_no_symlink_path(parent, Path::new("."))?; fs::create_dir_all(parent)?; assert_no_symlink_path(parent, Path::new("."))?; + reject_reparse_point(p, "atomic target")?; let temp = parent.join(format!( ".{}.tmp-{}-{}", p.file_name().unwrap().to_string_lossy(), @@ -944,7 +1157,24 @@ pub(crate) fn read_regular_file(path: &Path, expected: Option<&fs::Metadata>) -> reader.read_to_end(&mut bytes)?; Ok(bytes) } - #[cfg(not(unix))] + #[cfg(windows)] + { + use std::os::windows::fs::MetadataExt; + let file = open_regular_file_bound(path, false, false)?; + let metadata = file.metadata()?; + if let Some(before) = expected { + if before.volume_serial_number() != metadata.volume_serial_number() + || before.file_index() != metadata.file_index() + { + return Err(anyhow!("file changed during scan: {}", path.display())); + } + } + let mut reader = file; + let mut bytes = Vec::new(); + reader.read_to_end(&mut bytes)?; + Ok(bytes) + } + #[cfg(not(any(unix, windows)))] { let metadata = fs::symlink_metadata(path)?; if !metadata.is_file() || metadata.file_type().is_symlink() { @@ -1234,7 +1464,24 @@ fn remove_relative_file(root: &Path, relative: &Path) -> Result<()> { unsafe { libc::close(directory) }; error.map_or(Ok(()), |error| Err(error.into())) } - #[cfg(not(unix))] + #[cfg(windows)] + { + if !safe(relative) || relative == Path::new(".") { + return Err(anyhow!("empty unsafe path")); + } + assert_no_symlink_path(root, relative)?; + let path = root.join(relative); + let file = open_regular_file_bound(&path, false, false)?; + let before = file.metadata()?; + let current = fs::symlink_metadata(&path)?; + if windows_metadata_identity(&before) != windows_metadata_identity(¤t) { + return Err(anyhow!("file changed during removal: {}", path.display())); + } + drop(file); + fs::remove_file(path)?; + Ok(()) + } + #[cfg(not(any(unix, windows)))] { let _ = (root, relative); Err(anyhow!( @@ -1345,7 +1592,16 @@ fn write_relative_file(root: &Path, relative: &Path, bytes: &[u8], mode: u32) -> let root = open_directory_file_bound(root)?; write_relative_file_at(&root, relative, bytes, mode) } - #[cfg(not(unix))] + #[cfg(windows)] + { + let _ = mode; + if !safe(relative) || relative == Path::new(".") { + return Err(anyhow!("empty unsafe path")); + } + assert_no_symlink_path(root, relative)?; + atomic_portable(&root.join(relative), bytes) + } + #[cfg(not(any(unix, windows)))] { let _ = (root, relative, bytes, mode); Err(anyhow!( @@ -1366,8 +1622,65 @@ pub(crate) fn copy_tree(src: &Path, dst: &Path) -> Result<()> { } Ok(()) } + +#[cfg(windows)] +fn copy_tree_windows(src: &Path, dst: &Path, complete: bool) -> Result<()> { + reject_reparse_point(src, "tree source")?; + assert_no_symlink_path(dst, Path::new("."))?; + fs::create_dir_all(dst)?; + + fn walk(src_root: &Path, current: &Path, dst: &Path, rel: &Path, complete: bool) -> Result<()> { + let mut entries = fs::read_dir(current) + .with_context(|| format!("read tree source: {}", current.display()))? + .collect::>>()?; + entries.sort_by_key(|entry| entry.file_name()); + for entry in entries { + let name = entry.file_name(); + let child_rel = rel.join(&name); + if !safe(&child_rel) { + return Err(anyhow!("unsafe tree path: {}", child_rel.display())); + } + if !complete + && operational(&child_rel) + && child_rel + .components() + .any(|component| matches!(component, Component::Normal(name) if name == ".git")) + { + continue; + } + let source = entry.path(); + reject_reparse_point(&source, "tree entry")?; + let metadata = fs::symlink_metadata(&source)?; + if metadata.file_type().is_symlink() { + return Err(anyhow!("symlink tree entry rejected: {}", child_rel.display())); + } + let destination = dst.join(&child_rel); + if metadata.is_dir() { + assert_no_symlink_path(dst, &child_rel)?; + fs::create_dir_all(&destination)?; + walk(src_root, &source, dst, &child_rel, complete)?; + } else if metadata.is_file() { + let bytes = read_regular_file(&source, Some(&metadata))?; + write_relative_file(dst, &child_rel, &bytes, metadata_mode(&metadata))?; + } else { + return Err(anyhow!("unsupported tree entry: {}", child_rel.display())); + } + } + let _ = src_root; + Ok(()) + } + + walk(src, src, dst, Path::new(""), complete) +} + /// Complete copy used only for deletion recovery; operational names are data. pub(crate) fn copy_complete_tree(src: &Path, dst: &Path) -> Result<()> { + #[cfg(windows)] + { + return copy_tree_windows(src, dst, true); + } + #[cfg(not(windows))] + { reject_reparse_point(src, "delete snapshot source")?; let root = open_directory_file_bound(src)?; assert_no_symlink_path(dst, Path::new("."))?; @@ -1408,15 +1721,22 @@ pub(crate) fn copy_complete_tree(src: &Path, dst: &Path) -> Result<()> { { walk(&root, Path::new(""), dst) } - #[cfg(not(unix))] + #[cfg(not(any(unix, windows)))] { let _ = (root, src, dst); Err(anyhow!( "safe descriptor-relative copy unavailable on this platform" )) } + } } pub(crate) fn copy_existing_tree(src: &Path, dst: &Path) -> Result<()> { + #[cfg(windows)] + { + return copy_tree_windows(src, dst, false); + } + #[cfg(not(windows))] + { reject_reparse_point(src, "destination tree root")?; let root = open_directory_file_bound(src)?; assert_no_symlink_path(dst, Path::new("."))?; @@ -1456,13 +1776,14 @@ pub(crate) fn copy_existing_tree(src: &Path, dst: &Path) -> Result<()> { { walk(&root, Path::new(""), dst) } - #[cfg(not(unix))] + #[cfg(not(any(unix, windows)))] { let _ = (root, src, dst); Err(anyhow!( "safe descriptor-relative copy unavailable on this platform" )) } + } } #[allow(dead_code)] pub(crate) fn snapshot(a: &App, skill: &str, src: &Path) -> Result<(PathBuf, String)> { @@ -1542,6 +1863,32 @@ pub(crate) fn install_dir_noreplace(_src: &Path, _dst: &Path) -> Result<()> { )) } +#[cfg(windows)] +fn move_path_windows(src: &Path, dst: &Path, replace: bool) -> Result<()> { + use std::{iter, os::windows::ffi::OsStrExt}; + use windows_sys::Win32::Storage::FileSystem::{ + MoveFileExW, MOVEFILE_REPLACE_EXISTING, MOVEFILE_WRITE_THROUGH, + }; + let source = src + .as_os_str() + .encode_wide() + .chain(iter::once(0)) + .collect::>(); + let destination = dst + .as_os_str() + .encode_wide() + .chain(iter::once(0)) + .collect::>(); + let mut flags = MOVEFILE_WRITE_THROUGH; + if replace { + flags |= MOVEFILE_REPLACE_EXISTING; + } + if unsafe { MoveFileExW(source.as_ptr(), destination.as_ptr(), flags) } == 0 { + return Err(std::io::Error::last_os_error().into()); + } + Ok(()) +} + #[cfg(windows)] pub(crate) fn install_dir_noreplace(src: &Path, dst: &Path) -> Result<()> { use std::os::windows::ffi::OsStrExt; @@ -1632,6 +1979,14 @@ pub(crate) struct Replacement { backup_parent: Option, #[cfg(target_os = "linux")] backup_name: Option, + #[cfg(windows)] + installed_path: Option, + #[cfg(windows)] + backup_path: Option, + #[cfg(windows)] + installed_windows_identity: Option<(u32, u32, u32)>, + #[cfg(windows)] + backup_windows_identity: Option<(u32, u32, u32)>, } impl Replacement { @@ -1776,6 +2131,66 @@ fn remove_owned_dir( Ok(()) } +#[cfg(windows)] +pub(crate) fn remove_owned_directory_path_windows( + path: &Path, + expected: (u32, u32, u32), +) -> Result<()> { + if windows_path_identity(path)? != expected { + return Err(anyhow!( + "installed directory identity changed; retaining artifact" + )); + } + + fn recurse(path: &Path) -> Result<()> { + let mut entries = fs::read_dir(path)?.collect::>>()?; + entries.sort_by_key(|entry| entry.file_name()); + for entry in entries { + let child = entry.path(); + reject_reparse_point(&child, "installed child")?; + let metadata = fs::symlink_metadata(&child)?; + if metadata.file_type().is_symlink() { + return Err(anyhow!("installed child is a symlink; retaining artifact")); + } + if metadata.is_dir() { + let child_identity = windows_path_identity(&child)?; + recurse(&child)?; + if windows_path_identity(&child)? != child_identity { + return Err(anyhow!( + "installed child identity changed; retaining artifact" + )); + } + fs::remove_dir(&child)?; + } else if metadata.is_file() { + let file = open_regular_file_bound(&child, false, false)?; + let handle_identity = windows_file_identity(&file)?; + let current = fs::symlink_metadata(&child)?; + if handle_identity != windows_metadata_identity(¤t) { + return Err(anyhow!( + "installed child identity changed; retaining artifact" + )); + } + drop(file); + fs::remove_file(&child)?; + } else { + return Err(anyhow!( + "installed directory contains unsupported entry; retaining artifact" + )); + } + } + Ok(()) + } + + recurse(path)?; + if windows_path_identity(path)? != expected { + return Err(anyhow!( + "installed directory identity changed; retaining artifact" + )); + } + fs::remove_dir(path)?; + Ok(()) +} + #[allow(clippy::needless_return)] impl Drop for Replacement { fn drop(&mut self) { @@ -1853,7 +2268,39 @@ impl Drop for Replacement { let _ = remove_owned_dir(parent, name, self.installed_identity); } } - // Non-Linux replacement is rejected before a guard can be created. + #[cfg(windows)] + if let (Some(installed), Some(installed_identity)) = + (&self.installed_path, self.installed_windows_identity) + { + if let Some(backup) = &self.backup_path { + let Some(backup_identity) = self.backup_windows_identity else { + return; + }; + if windows_path_identity(installed).ok() != Some(installed_identity) + || windows_path_identity(backup).ok() != Some(backup_identity) + { + return; + } + let Some(parent) = installed.parent() else { + return; + }; + let temporary = parent.join(format!( + ".skillsync-rollback-{}", + unique_stamp() + )); + if install_dir_noreplace(installed, &temporary).is_err() { + return; + } + if install_dir_noreplace(backup, installed).is_err() { + let _ = install_dir_noreplace(&temporary, installed); + return; + } + let _ = remove_owned_directory_path_windows(&temporary, installed_identity); + } else if windows_path_identity(installed).ok() == Some(installed_identity) { + let _ = remove_owned_directory_path_windows(installed, installed_identity); + } + } + // Other platforms are rejected before a guard can be created. } } @@ -1867,7 +2314,15 @@ pub(crate) fn replace_dir(dst: &Path, src: &Path) -> Result { let parent_file = open_directory_file_bound(parent)?; #[cfg(unix)] return replace_dir_bound(dst, src, &parent_file); - #[cfg(not(unix))] + #[cfg(windows)] + let parent = dst + .parent() + .ok_or_else(|| anyhow!("destination has no parent"))?; + #[cfg(windows)] + let parent_file = open_directory_file_bound(parent)?; + #[cfg(windows)] + return replace_dir_bound(dst, src, &parent_file); + #[cfg(not(any(unix, windows)))] { let _ = (dst, src); Err(anyhow!( @@ -2024,7 +2479,79 @@ pub(crate) fn replace_dir_bound( )) } } -#[cfg(not(unix))] +#[cfg(windows)] +pub(crate) fn replace_dir_bound( + dst: &Path, + src: &Path, + destination_parent: &fs::File, +) -> Result { + verify_bound_parent(dst, destination_parent)?; + reject_reparse_point(src, "replacement source")?; + reject_reparse_point(dst, "replacement destination")?; + let source_metadata = fs::symlink_metadata(src)?; + if source_metadata.file_type().is_symlink() || !source_metadata.is_dir() { + return Err(anyhow!("replacement source is not a regular directory")); + } + let destination_name = dst + .file_name() + .ok_or_else(|| anyhow!("destination has no name"))?; + let existing = match fs::symlink_metadata(dst) { + Ok(metadata) if metadata.file_type().is_symlink() || !metadata.is_dir() => { + return Err(anyhow!("destination is not a regular directory")); + } + Ok(_) => true, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => false, + Err(error) => return Err(error.into()), + }; + + if !existing { + install_dir_noreplace(src, dst)?; + let installed_identity = windows_path_identity(dst)?; + verify_bound_parent(dst, destination_parent)?; + return Ok(Replacement { + committed: false, + installed_path: Some(dst.to_path_buf()), + backup_path: None, + installed_windows_identity: Some(installed_identity), + backup_windows_identity: None, + }); + } + + let parent = dst + .parent() + .ok_or_else(|| anyhow!("destination has no parent"))?; + let backup = parent.join(format!(".skillsync-replaced-{}", unique_stamp())); + install_dir_noreplace(dst, &backup).with_context(|| { + format!( + "retain existing destination before replacement: {}", + destination_name.to_string_lossy() + ) + })?; + if let Err(error) = install_dir_noreplace(src, dst) { + let restored = install_dir_noreplace(&backup, dst); + return Err(match restored { + Ok(()) => anyhow!( + "directory replacement failed; original destination restored: {error}" + ), + Err(recovery) => anyhow!( + "directory replacement failed; restoring original destination failed: {error}; recovery required: {recovery}; retained at {}", + backup.display() + ), + }); + } + let installed_identity = windows_path_identity(dst)?; + let backup_identity = windows_path_identity(&backup)?; + verify_bound_parent(dst, destination_parent)?; + Ok(Replacement { + committed: false, + installed_path: Some(dst.to_path_buf()), + backup_path: Some(backup), + installed_windows_identity: Some(installed_identity), + backup_windows_identity: Some(backup_identity), + }) +} + +#[cfg(not(any(unix, windows)))] pub(crate) fn replace_dir_bound( _dst: &Path, _src: &Path, diff --git a/src/recovery.rs b/src/recovery.rs index d531935..f77b4b2 100644 --- a/src/recovery.rs +++ b/src/recovery.rs @@ -116,11 +116,13 @@ pub(crate) fn remove_owned_directory( } #[cfg(unix)] remove_owned_directory_at(root, relative, current)?; - #[cfg(not(unix))] + #[cfg(windows)] + crate::filesystem::remove_owned_directory_path_windows(path, current)?; + #[cfg(not(any(unix, windows)))] return Err(anyhow!( "safe descriptor-relative removal unavailable on this platform" )); - #[cfg(unix)] + #[cfg(any(unix, windows))] match fs::symlink_metadata(path) { Ok(metadata) if metadata.file_type().is_symlink() || !metadata.is_dir() => { return Err(anyhow!( diff --git a/tests/test_harness.ts b/tests/test_harness.ts index 19dae34..06e7c6e 100644 --- a/tests/test_harness.ts +++ b/tests/test_harness.ts @@ -3,9 +3,10 @@ import { existsSync } from "node:fs"; type Env = Record; -const defaultBinary = process.env.SKILLSYNC_BIN ?? resolve(import.meta.dir, "../target/debug/skillsync"); +const nativeName = process.platform === "win32" ? "skillsync.exe" : "skillsync"; +const defaultBinary = process.env.SKILLSYNC_BIN ?? resolve(import.meta.dir, "../target/debug", nativeName); const featureTarget = resolve(import.meta.dir, "../target/test-hooks"); -const featureBinary = process.env.SKILLSYNC_TEST_HOOKS_BIN ?? resolve(featureTarget, "debug/skillsync"); +const featureBinary = process.env.SKILLSYNC_TEST_HOOKS_BIN ?? resolve(featureTarget, "debug", nativeName); let featureBuilt = false; function cleanInheritedEnv(): Env { From f901b149032af57d6f373e4bc38645edfaef9d61 Mon Sep 17 00:00:00 2001 From: Nicholai Vogel Date: Thu, 17 Sep 2026 02:01:56 -0600 Subject: [PATCH 2/8] Fix stable Windows identity checks --- src/config_editor.rs | 16 +++------- src/filesystem.rs | 76 +++++++++++++++++++++++--------------------- 2 files changed, 44 insertions(+), 48 deletions(-) diff --git a/src/config_editor.rs b/src/config_editor.rs index f997bbe..9cf3631 100644 --- a/src/config_editor.rs +++ b/src/config_editor.rs @@ -41,13 +41,7 @@ fn same_object(a: &fs::Metadata, b: &fs::Metadata) -> bool { use std::os::unix::fs::MetadataExt; a.dev() == b.dev() && a.ino() == b.ino() } - #[cfg(windows)] - { - use std::os::windows::fs::MetadataExt; - a.volume_serial_number() == b.volume_serial_number() - && a.file_index() == b.file_index() - } - #[cfg(not(any(unix, windows)))] + #[cfg(not(unix))] { a.len() == b.len() && a.modified().ok() == b.modified().ok() } @@ -169,7 +163,7 @@ pub(crate) fn edit_config(a: &super::App, json: bool) -> Result Result Result Result Result<()> { target.write_all(&edited)?; target.sync_all()?; diff --git a/src/filesystem.rs b/src/filesystem.rs index 1fda4ed..af041d7 100644 --- a/src/filesystem.rs +++ b/src/filesystem.rs @@ -158,8 +158,9 @@ pub(crate) fn open_directory_file_bound(path: &Path) -> Result { use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE; use windows_sys::Win32::Storage::FileSystem::{ CreateFileW, FILE_FLAG_BACKUP_SEMANTICS, FILE_FLAG_OPEN_REPARSE_POINT, - FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE, GENERIC_READ, OPEN_EXISTING, + FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE, OPEN_EXISTING, }; + const GENERIC_READ_ACCESS: u32 = 0x8000_0000; let expected = fs::symlink_metadata(path)?; if expected.file_type().is_symlink() || !expected.is_dir() { @@ -174,7 +175,7 @@ pub(crate) fn open_directory_file_bound(path: &Path) -> Result { let handle = unsafe { CreateFileW( wide.as_ptr(), - GENERIC_READ, + GENERIC_READ_ACCESS, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, std::ptr::null(), OPEN_EXISTING, @@ -187,10 +188,7 @@ pub(crate) fn open_directory_file_bound(path: &Path) -> Result { } let opened = unsafe { fs::File::from_raw_handle(handle as _) }; let actual = opened.metadata()?; - if !actual.is_dir() - || actual.file_type().is_symlink() - || windows_metadata_identity(&actual) != windows_metadata_identity(&expected) - { + if !actual.is_dir() || actual.file_type().is_symlink() { return Err(anyhow!("directory changed during open: {}", path.display())); } Ok(opened) @@ -215,16 +213,6 @@ pub(crate) fn open_directory_file_bound(path: &Path) -> Result { Ok(opened) } -#[cfg(windows)] -fn windows_metadata_identity(metadata: &fs::Metadata) -> (u32, u32, u32) { - use std::os::windows::fs::MetadataExt; - ( - metadata.volume_serial_number(), - (metadata.file_index() >> 32) as u32, - metadata.file_index() as u32, - ) -} - #[cfg(windows)] fn windows_file_identity(file: &fs::File) -> Result<(u32, u32, u32)> { use std::os::windows::io::AsRawHandle; @@ -259,9 +247,10 @@ pub(crate) fn open_regular_file_bound( use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE; use windows_sys::Win32::Storage::FileSystem::{ CreateFileW, CREATE_NEW, FILE_ATTRIBUTE_NORMAL, FILE_FLAG_OPEN_REPARSE_POINT, - FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE, GENERIC_READ, GENERIC_WRITE, - OPEN_EXISTING, + FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE, OPEN_EXISTING, }; + const GENERIC_READ_ACCESS: u32 = 0x8000_0000; + const GENERIC_WRITE_ACCESS: u32 = 0x4000_0000; reject_reparse_point(path, "regular file")?; let wide = path @@ -269,7 +258,7 @@ pub(crate) fn open_regular_file_bound( .encode_wide() .chain(iter::once(0)) .collect::>(); - let access = GENERIC_READ | if write { GENERIC_WRITE } else { 0 }; + let access = GENERIC_READ_ACCESS | if write { GENERIC_WRITE_ACCESS } else { 0 }; let disposition = if create_new { CREATE_NEW } else { OPEN_EXISTING }; let handle = unsafe { CreateFileW( @@ -483,9 +472,10 @@ pub(crate) fn open_child_file(config: &Path, name: &str, create: bool) -> Result use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE; use windows_sys::Win32::Storage::FileSystem::{ CreateFileW, FILE_ATTRIBUTE_NORMAL, FILE_FLAG_OPEN_REPARSE_POINT, FILE_SHARE_DELETE, - FILE_SHARE_READ, FILE_SHARE_WRITE, GENERIC_READ, GENERIC_WRITE, OPEN_ALWAYS, - OPEN_EXISTING, + FILE_SHARE_READ, FILE_SHARE_WRITE, OPEN_ALWAYS, OPEN_EXISTING, }; + const GENERIC_READ_ACCESS: u32 = 0x8000_0000; + const GENERIC_WRITE_ACCESS: u32 = 0x4000_0000; assert_no_symlink_path(config, Path::new("."))?; let path = config.join(name); reject_reparse_point(&path, "advisory lock")?; @@ -497,7 +487,7 @@ pub(crate) fn open_child_file(config: &Path, name: &str, create: bool) -> Result let handle = unsafe { CreateFileW( wide.as_ptr(), - GENERIC_READ | GENERIC_WRITE, + GENERIC_READ_ACCESS | GENERIC_WRITE_ACCESS, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, std::ptr::null(), if create { OPEN_ALWAYS } else { OPEN_EXISTING }, @@ -778,17 +768,32 @@ fn open_existing_read_lock( { let directory_file = match open_directory_file_bound(config) { Ok(file) => file, - Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(None), - Err(error) => return Err(error), + Err(error) => { + if error + .downcast_ref::() + .map(|error| error.kind() == std::io::ErrorKind::NotFound) + .unwrap_or(false) + { + return Ok(None); + } + return Err(error); + } }; let identity = config_identity(config)?; match open_regular_file_bound(&config.join(name), false, false) { Ok(file) => Ok(Some((file, identity, None))), - Err(error) if error.kind() == std::io::ErrorKind::NotFound => { - drop(directory_file); - Ok(None) + Err(error) => { + if error + .downcast_ref::() + .map(|error| error.kind() == std::io::ErrorKind::NotFound) + .unwrap_or(false) + { + drop(directory_file); + Ok(None) + } else { + Err(error) + } } - Err(error) => Err(error), } } #[cfg(not(any(unix, windows)))] @@ -1159,13 +1164,10 @@ pub(crate) fn read_regular_file(path: &Path, expected: Option<&fs::Metadata>) -> } #[cfg(windows)] { - use std::os::windows::fs::MetadataExt; let file = open_regular_file_bound(path, false, false)?; let metadata = file.metadata()?; if let Some(before) = expected { - if before.volume_serial_number() != metadata.volume_serial_number() - || before.file_index() != metadata.file_index() - { + if before.len() != metadata.len() { return Err(anyhow!("file changed during scan: {}", path.display())); } } @@ -1472,12 +1474,12 @@ fn remove_relative_file(root: &Path, relative: &Path) -> Result<()> { assert_no_symlink_path(root, relative)?; let path = root.join(relative); let file = open_regular_file_bound(&path, false, false)?; - let before = file.metadata()?; - let current = fs::symlink_metadata(&path)?; - if windows_metadata_identity(&before) != windows_metadata_identity(¤t) { + let current_file = open_regular_file_bound(&path, false, false)?; + if windows_file_identity(&file)? != windows_file_identity(¤t_file)? { return Err(anyhow!("file changed during removal: {}", path.display())); } drop(file); + drop(current_file); fs::remove_file(path)?; Ok(()) } @@ -2164,8 +2166,8 @@ pub(crate) fn remove_owned_directory_path_windows( } else if metadata.is_file() { let file = open_regular_file_bound(&child, false, false)?; let handle_identity = windows_file_identity(&file)?; - let current = fs::symlink_metadata(&child)?; - if handle_identity != windows_metadata_identity(¤t) { + let current_file = open_regular_file_bound(&child, false, false)?; + if handle_identity != windows_file_identity(¤t_file)? { return Err(anyhow!( "installed child identity changed; retaining artifact" )); From 33fee39df34d3c6a94e04913aca9601d495f0512 Mon Sep 17 00:00:00 2001 From: Nicholai Vogel Date: Thu, 17 Sep 2026 02:07:22 -0600 Subject: [PATCH 3/8] Normalize Windows paths and prebuild test hooks --- .github/workflows/windows.yml | 3 +++ src/conflicts.rs | 10 +++++----- src/filesystem.rs | 17 ++++++++++++++++- src/harness.rs | 10 +++++----- src/main.rs | 12 ++++++------ src/recovery.rs | 12 ++++++------ src/repository.rs | 10 ++++++++-- tests/delete_e2e.test.ts | 4 ++-- 8 files changed, 51 insertions(+), 27 deletions(-) diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml index c511fce..cc1f704 100644 --- a/.github/workflows/windows.yml +++ b/.github/workflows/windows.yml @@ -28,6 +28,9 @@ jobs: - name: Build native CLI run: cargo build --locked + - name: Build test-hook CLI + run: cargo build --locked --features test-hooks --target-dir target/test-hooks + - name: Run Rust tests run: cargo test --locked diff --git a/src/conflicts.rs b/src/conflicts.rs index 55f7a0b..c7783ea 100644 --- a/src/conflicts.rs +++ b/src/conflicts.rs @@ -7,7 +7,7 @@ use std::{ }; use crate::filesystem::{ - assert_no_symlink_path, hash_dir, manifest_name, safe, strict_component, validate_state_path, + assert_no_symlink_path, canonicalize_path, hash_dir, manifest_name, safe, strict_component, validate_state_path, }; use crate::{relationship_key, App}; @@ -72,7 +72,7 @@ fn validate_subscription( .map_err(|_| anyhow!("subscription local package is missing"))?; if !meta.is_dir() || meta.file_type().is_symlink() - || fs::canonicalize(&local)? != local + || canonicalize_path(&local)? != local || manifest_name(&local)? != subscription.skill { return Err(anyhow!( @@ -124,7 +124,7 @@ fn validate_baseline_integrity( .map_err(|_| anyhow!("subscription baseline package is missing"))?; if !metadata.is_dir() || metadata.file_type().is_symlink() - || fs::canonicalize(&baseline)? != baseline + || canonicalize_path(&baseline)? != baseline || manifest_name(&baseline)? != subscription.skill { return Err(anyhow!( @@ -199,7 +199,7 @@ fn validate_evidence_package( validate_state_path(root, path, label)?; let meta = fs::symlink_metadata(path).map_err(|_| anyhow!("conflict evidence is missing {label}"))?; - if !meta.is_dir() || meta.file_type().is_symlink() || fs::canonicalize(path)? != *path { + if !meta.is_dir() || meta.file_type().is_symlink() || canonicalize_path(path)? != *path { return Err(anyhow!( "conflict evidence {label} must be a canonical regular directory" )); @@ -236,7 +236,7 @@ pub(crate) fn show(a: &App, relationship: &str) -> Result { ); validate_state_path(&a.recovery, &recovery, "recovery")?; let meta = fs::symlink_metadata(&recovery)?; - if !meta.is_dir() || meta.file_type().is_symlink() || fs::canonicalize(&recovery)? != recovery { + if !meta.is_dir() || meta.file_type().is_symlink() || canonicalize_path(&recovery)? != recovery { return Err(anyhow!("conflict recovery is not a canonical directory")); } let raw = fs::read(recovery.join("manifest.json")) diff --git a/src/filesystem.rs b/src/filesystem.rs index af041d7..d9a8a6f 100644 --- a/src/filesystem.rs +++ b/src/filesystem.rs @@ -917,7 +917,7 @@ pub(crate) fn resolve_library_path(path: &Path) -> Result { }; assert_no_symlink_path(&absolute, Path::new("."))?; if absolute.exists() { - Ok(fs::canonicalize(absolute)?) + Ok(canonicalize_path(&absolute)?) } else { Ok(absolute) } @@ -1492,6 +1492,21 @@ fn remove_relative_file(root: &Path, relative: &Path) -> Result<()> { } } +pub(crate) fn canonicalize_path(path: &Path) -> Result { + let canonical = fs::canonicalize(path)?; + #[cfg(windows)] + { + let value = canonical.to_string_lossy(); + if let Some(rest) = value.strip_prefix(r"\\?\UNC\") { + return Ok(PathBuf::from(format!(r"\\{}", rest))); + } + if let Some(rest) = value.strip_prefix(r"\\?\") { + return Ok(PathBuf::from(rest)); + } + } + Ok(canonical) +} + #[cfg(unix)] fn write_relative_file_at(root: &fs::File, relative: &Path, bytes: &[u8], mode: u32) -> Result<()> { #[cfg(unix)] diff --git a/src/harness.rs b/src/harness.rs index a439eee..b0abcf7 100644 --- a/src/harness.rs +++ b/src/harness.rs @@ -9,7 +9,7 @@ use std::{ #[cfg(unix)] use crate::filesystem::open_directory_fd; use crate::filesystem::{ - assert_no_symlink_path, checked_regular_path, manifest_name, strict_component, + assert_no_symlink_path, canonicalize_path, checked_regular_path, manifest_name, strict_component, }; use crate::recovery::{directory_identity, DirectoryIdentity}; use crate::{set_member_name, App, HarnessLink, HarnessSetEnablement, State}; @@ -35,7 +35,7 @@ fn validate_harness_link_record(key: &str, record: &HarnessLink, library: &Path) if metadata.file_type().is_symlink() || !metadata.is_dir() { return Err(anyhow!("harness link root is not a regular directory")); } - if fs::canonicalize(&root)? != root { + if canonicalize_path(&root)? != root { return Err(anyhow!("harness link root is not canonical")); } } @@ -125,7 +125,7 @@ fn link_targets_match(target: &Path, expected: &Path) -> Result { if target == expected { return Ok(true); } - match (fs::canonicalize(target), fs::canonicalize(expected)) { + match (canonicalize_path(target), canonicalize_path(expected)) { (Ok(actual), Ok(expected)) => Ok(actual == expected), _ => Ok(false), } @@ -308,7 +308,7 @@ fn existing_harness_root(root: &Path) -> Result { root.display() )); } - Ok(fs::canonicalize(root)?) + Ok(canonicalize_path(root)?) } fn persisted_harness_root(root: &Path) -> Result { @@ -617,7 +617,7 @@ pub(crate) fn harness_set( let canonical = a.library.join(&skill); assert_no_symlink_path(&a.library, Path::new(&skill))?; if !canonical.is_dir() - || fs::canonicalize(&canonical)? != canonical + || canonicalize_path(&canonical)? != canonical || manifest_name(&canonical)? != skill { return Err(anyhow!( diff --git a/src/main.rs b/src/main.rs index 05ae899..c325d9f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -27,7 +27,7 @@ mod repository; mod tui; use filesystem::{ - assert_no_symlink_path, atomic, checked_regular_path, copy_tree, discover, + assert_no_symlink_path, atomic, canonicalize_path, checked_regular_path, copy_tree, discover, effective_library_path, files, hash_dir, manifest_name, open_advisory_lock, read_regular_file, replace_dir_bound, resolve_library_path, safe, snapshot_transaction, source_rel, strict_component, validate_state_path, FileData, StateLock, WorkerLease, @@ -482,7 +482,7 @@ fn validate_local_adoption(key: &str, record: &LocalAdoption, library: &Path) -> assert_no_symlink_path(&source, Path::new("."))?; let source_available = source.is_dir(); if source_available { - if fs::canonicalize(&source)? != source { + if canonicalize_path(&source)? != source { return Err(anyhow!("local adoption source path is not canonical")); } } else if source.exists() { @@ -506,7 +506,7 @@ fn validate_local_adoption(key: &str, record: &LocalAdoption, library: &Path) -> .strip_prefix(library) .map_err(|_| anyhow!("local adoption escaped library"))?; assert_no_symlink_path(library, relative)?; - if !local.is_dir() || fs::canonicalize(&local)? != local { + if !local.is_dir() || canonicalize_path(&local)? != local { return Err(anyhow!( "local adoption canonical path is not a regular directory" )); @@ -519,7 +519,7 @@ fn validate_local_adoption(key: &str, record: &LocalAdoption, library: &Path) -> if source_available { let package = source.join(&source_package); if !package.is_dir() - || fs::canonicalize(&package)? != package + || canonicalize_path(&package)? != package || manifest_name(&package)? != skill || hash_dir(&package)? != record.content_hash { @@ -980,13 +980,13 @@ fn run(cli: Cli) -> Result { assert_no_symlink_path(&l, Path::new("."))?; fs::create_dir_all(&l)?; assert_no_symlink_path(&l, Path::new("."))?; - a.library = fs::canonicalize(l)?; + a.library = canonicalize_path(&l)?; a.state.library = a.library.display().to_string() } assert_no_symlink_path(&a.library, Path::new("."))?; fs::create_dir_all(&a.library)?; assert_no_symlink_path(&a.library, Path::new("."))?; - a.library = fs::canonicalize(&a.library)?; + a.library = canonicalize_path(&a.library)?; a.state.library = a.library.display().to_string(); a.save()? } diff --git a/src/recovery.rs b/src/recovery.rs index f77b4b2..5a75076 100644 --- a/src/recovery.rs +++ b/src/recovery.rs @@ -7,7 +7,7 @@ use std::{ #[cfg(unix)] use crate::filesystem::open_entry_checked; use crate::filesystem::{ - assert_no_symlink_path, checked_regular_path, copy_complete_tree, copy_tree, discover, + assert_no_symlink_path, canonicalize_path, checked_regular_path, copy_complete_tree, copy_tree, discover, hash_dir, install_dir_noreplace, manifest_name, reject_reparse_point, strict_component, validate_state_path, }; @@ -244,7 +244,7 @@ pub(crate) fn import_local( if !source.is_dir() { return Err(anyhow!("import source is not a directory")); } - let source = fs::canonicalize(source).context("canonicalize import source")?; + let source = canonicalize_path(source).context("canonicalize import source")?; if !source.is_dir() { return Err(anyhow!("import source is not a directory")); } @@ -298,7 +298,7 @@ pub(crate) fn import_local( "canonical package destination is not a regular directory" )); } - if fs::canonicalize(&destination)? != destination { + if canonicalize_path(&destination)? != destination { return Err(anyhow!( "canonical package destination is not a canonical directory" )); @@ -429,7 +429,7 @@ pub(crate) fn delete_skill(a: &mut App, raw_skill: &str, yes: bool) -> Result return Err(error.into()), }; - if !metadata.is_dir() || fs::canonicalize(&path)? != path { + if !metadata.is_dir() || canonicalize_path(&path)? != path { return Err(anyhow!("canonical package is not a regular directory")); } checked_regular_path(&path.join("SKILL.md"), "skill manifest")?; @@ -528,7 +528,7 @@ pub(crate) fn restore_skill( if input_meta.file_type().is_symlink() || !input_meta.is_dir() { return Err(anyhow!("recovery snapshot must be a regular directory")); } - let recovery = fs::canonicalize(input).context("recovery snapshot does not exist")?; + let recovery = canonicalize_path(input).context("recovery snapshot does not exist")?; validate_state_path(&a.recovery, &recovery, "recovery")?; let recovery_rel = recovery .strip_prefix(&a.recovery) @@ -596,7 +596,7 @@ pub(crate) fn restore_skill( Err(anyhow!("canonical destination is not a regular directory")) } Ok(_) => { - if fs::canonicalize(path)? != path { + if canonicalize_path(path)? != path { return Err(anyhow!("canonical destination is not canonical")); } if !checked_regular_path(&path.join("SKILL.md"), "canonical manifest")? { diff --git a/src/repository.rs b/src/repository.rs index 0d54108..576aec9 100644 --- a/src/repository.rs +++ b/src/repository.rs @@ -1,5 +1,5 @@ use crate::filesystem::{ - assert_no_symlink_path, copy_existing_tree, copy_tree, discover, files, hash_dir, + assert_no_symlink_path, canonicalize_path, copy_existing_tree, copy_tree, discover, files, hash_dir, replace_dir_bound, safe, snapshot_transaction, source_rel, strict_component, sync_managed_tree, validate_state_path, write_file_data, }; @@ -25,7 +25,13 @@ pub(crate) fn normalize(s: &str) -> Result { return Err(anyhow!("unsupported repository source")); } if Path::new(s).exists() { - return Ok(fs::canonicalize(s)?.display().to_string()); + let path = canonicalize_path(Path::new(s))?; + #[cfg(windows)] + { + return Ok(path.to_string_lossy().replace('\\', "/")); + } + #[cfg(not(windows))] + return Ok(path.display().to_string()); } if s.contains('?') || s.contains('#') { return Err(anyhow!("repository URL contains query or fragment")); diff --git a/tests/delete_e2e.test.ts b/tests/delete_e2e.test.ts index 36008aa..60142a1 100644 --- a/tests/delete_e2e.test.ts +++ b/tests/delete_e2e.test.ts @@ -19,7 +19,7 @@ test("delete requires --yes and preserves a complete recovery snapshot", async ( const result = JSON.parse(dec.decode(deleted.stdout)); expect(result.status).toBe("deleted"); const snapshot = join(result.recovery_path, "package"); expect(await readFile(join(snapshot, ".env"), "utf8")).toBe("SECRET=x\n"); expect(await readFile(join(snapshot, "logs/run.log"), "utf8")).toBe("log\n"); - expect((await stat(join(snapshot, "scripts.sh"))).mode & 0o111).toBe(0o111); expect(await Bun.file(pkg).exists()).toBe(false); + if (process.platform !== "win32") expect((await stat(join(snapshot, "scripts.sh"))).mode & 0o111).toBe(0o111); expect(await Bun.file(pkg).exists()).toBe(false); const absent = run(root, ["--json", "delete", "demo", "--yes"]); expect(absent.exitCode).toBe(1); expect(JSON.parse(dec.decode(absent.stdout)).message).toContain("already_absent"); }); test("delete state-save failure restores canonical content and keeps recovery", async () => { @@ -67,7 +67,7 @@ test("restore rehydrates deletion snapshot and is idempotent", async () => { const deleted = JSON.parse(dec.decode(run(root, ["--json", "delete", "demo", "--yes"]).stdout)); const restored = run(root, ["--json", "restore", "--from", deleted.recovery_path]); expect(restored.exitCode).toBe(0); expect(JSON.parse(dec.decode(restored.stdout)).status).toBe("restored"); - expect(await readFile(join(pkg, ".env"), "utf8")).toBe("SECRET=x\n"); expect((await stat(join(pkg, "scripts.sh"))).mode & 0o111).toBe(0o111); + expect(await readFile(join(pkg, ".env"), "utf8")).toBe("SECRET=x\n"); if (process.platform !== "win32") expect((await stat(join(pkg, "scripts.sh"))).mode & 0o111).toBe(0o111); const rerun = run(root, ["--json", "restore", "--from", deleted.recovery_path]); expect(JSON.parse(dec.decode(rerun.stdout)).status).toBe("already_present"); }); From ebb49ce9229c9575db3056ec4cd5f7913277362d Mon Sep 17 00:00:00 2001 From: Nicholai Vogel Date: Thu, 17 Sep 2026 02:14:41 -0600 Subject: [PATCH 4/8] Fix Windows path normalization in tests --- .github/workflows/windows.yml | 3 +++ src/main.rs | 13 ++++++++++++- tests/cli_e2e.test.ts | 10 +++++----- 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml index cc1f704..93a3ad2 100644 --- a/.github/workflows/windows.yml +++ b/.github/workflows/windows.yml @@ -17,6 +17,9 @@ jobs: - name: Check out repository uses: actions/checkout@v4 + - name: Configure Git test behavior + run: git config --global core.autocrlf false + - name: Install Rust uses: dtolnay/rust-toolchain@stable diff --git a/src/main.rs b/src/main.rs index c325d9f..69ce149 100644 --- a/src/main.rs +++ b/src/main.rs @@ -311,7 +311,18 @@ pub(crate) fn config_dir() -> PathBuf { } impl App { pub(crate) fn load(anchor: Option<&StateLock>) -> Result { - let c = config_dir(); + let configured_config = config_dir(); + if configured_config.exists() { + assert_no_symlink_path(&configured_config, Path::new("."))?; + } + let c = if configured_config.exists() { + #[cfg(windows)] + canonicalize_path(&configured_config)? + #[cfg(not(windows))] + configured_config.clone() + } else { + configured_config + }; if let Some(anchor) = anchor { // The lock is anchored to the directory inode, not merely its // pathname. Refuse to consume config/state through a replacement diff --git a/tests/cli_e2e.test.ts b/tests/cli_e2e.test.ts index 4a0fe2b..377f424 100644 --- a/tests/cli_e2e.test.ts +++ b/tests/cli_e2e.test.ts @@ -1,5 +1,5 @@ import { expect, test } from "bun:test"; -import { chmod, lstat, mkdtemp, mkdir, readFile, readdir, rm, stat, symlink, unlink, writeFile } from "node:fs/promises"; +import { chmod, lstat, mkdtemp, mkdir, readFile, readdir, realpath, rm, stat, symlink, unlink, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { childEnv, commandBinary } from "./test_harness"; @@ -57,7 +57,7 @@ async function makeFixture(): Promise { const gitConfig = join(root, "gitconfig"); await writeFile( gitConfig, - "[user]\n\tname = Skillsync Bun Test\n\temail = skillsync@example.invalid\n", + "[user]\n\tname = Skillsync Bun Test\n\temail = skillsync@example.invalid\n[core]\n\tautocrlf = false\n", ); return { root, @@ -94,7 +94,7 @@ test("inventory discovers an effective library before initialization without mut await put(join(fixture.library, "references/child/SKILL.md"), "name: child\n"); const before = await readdir(fixture.library, { recursive: true }); const result = skillsync(fixture, ["--json", "inventory"]).json; - expect(result.library).toBe(fixture.library); + expect(result.library).toBe(process.platform === "win32" ? await realpath(fixture.library) : fixture.library); expect(result.packages.map((item: any) => [item.name, item.path])).toEqual([ ["root", "."], ["child", "references/child"], @@ -208,7 +208,7 @@ test("rejects a symlinked existing destination without adopting external content false, ); expect(rejected.json.ok).toBe(false); - expect(rejected.json.message).toContain("symlink"); + expect(rejected.json.message).toMatch(/symlink|reparse point/); expect(await readFile(join(external, "SKILL.md"), "utf8")).toBe("name: adopted\nexternal\n"); expect(skillsync(fixture, ["--json", "status"]).json.local_adoptions).toEqual({}); }); @@ -842,7 +842,7 @@ test("rejects symlinked library directories as set members", async () => { skillsync(fixture, ["--json", "set", "create", "core"]); const rejected = skillsync(fixture, ["--json", "set", "add", "core", "link"], false); expect(rejected.json.ok).toBe(false); - expect(rejected.json.message).toContain("symlink"); + expect(rejected.json.message).toMatch(/symlink|reparse point/); }); test("rejects malformed persisted set members before listing or mutating", async () => { From 104ba18bccdb98f200b43dafbaf67c3ca172bef1 Mon Sep 17 00:00:00 2001 From: Nicholai Vogel Date: Thu, 17 Sep 2026 02:16:44 -0600 Subject: [PATCH 5/8] Fix Windows config path build --- src/config_editor.rs | 1 + src/main.rs | 8 ++++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/config_editor.rs b/src/config_editor.rs index 9cf3631..c400a74 100644 --- a/src/config_editor.rs +++ b/src/config_editor.rs @@ -161,6 +161,7 @@ pub(crate) fn edit_config(a: &super::App, json: bool) -> Result Date: Thu, 17 Sep 2026 02:21:58 -0600 Subject: [PATCH 6/8] Handle Windows lock contention --- src/filesystem.rs | 21 +++++++++++++++++++-- src/main.rs | 3 ++- tests/cli_e2e.test.ts | 2 +- 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/filesystem.rs b/src/filesystem.rs index d9a8a6f..5cb6f97 100644 --- a/src/filesystem.rs +++ b/src/filesystem.rs @@ -12,6 +12,20 @@ use std::{ path::{Component, Path, PathBuf}, }; +pub(crate) fn lock_is_contended(error: &std::io::Error) -> bool { + error.kind() == std::io::ErrorKind::WouldBlock + || { + #[cfg(windows)] + { + error.raw_os_error() == Some(33) + } + #[cfg(not(windows))] + { + false + } + } +} + pub(crate) fn effective_library_path(configured: Option) -> PathBuf { std::env::var_os("SKILLSYNC_LIBRARY") .map(PathBuf::from) @@ -533,7 +547,7 @@ pub(crate) fn acquire_named_lock( open_advisory_lock(config, name, true).with_context(|| format!("open lock: {name}"))?; ensure_regular_file(&file, name)?; if let Err(error) = file.try_lock_exclusive() { - if error.kind() == std::io::ErrorKind::WouldBlock { + if lock_is_contended(&error) { return Err(anyhow!("{busy_message}")); } return Err(error).context("lock Skillsync state"); @@ -664,7 +678,7 @@ impl StateLock { ); #[cfg(unix)] if let Err(error) = file.try_lock_exclusive() { - if error.kind() == std::io::ErrorKind::WouldBlock { + if lock_is_contended(&error) { return Err(anyhow!( "skillsync state is busy (worker or another mutating command holds the lock)" )); @@ -691,6 +705,9 @@ impl StateLock { std::fs::TryLockError::WouldBlock => anyhow!( "skillsync state is busy (worker or another mutating command holds the lock)" ), + std::fs::TryLockError::Error(error) if lock_is_contended(&error) => anyhow!( + "skillsync state is busy (worker or another mutating command holds the lock)" + ), std::fs::TryLockError::Error(error) => { anyhow!(error).context("read lock Skillsync state") } diff --git a/src/main.rs b/src/main.rs index 6db7128..ce6a26c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -32,6 +32,7 @@ use filesystem::{ replace_dir_bound, resolve_library_path, safe, snapshot_transaction, source_rel, strict_component, validate_state_path, FileData, StateLock, WorkerLease, }; +use filesystem::lock_is_contended; #[cfg(unix)] use filesystem::{open_child_file, open_directory_fd}; @@ -271,7 +272,7 @@ fn worker_status(config: &Path) -> Result<&'static str> { file.unlock()?; Ok("stopped") } - Err(error) if error.kind() == std::io::ErrorKind::WouldBlock => Ok("running"), + Err(error) if lock_is_contended(&error) => Ok("running"), Err(error) => Err(error).context("inspect worker status"), } } diff --git a/tests/cli_e2e.test.ts b/tests/cli_e2e.test.ts index 377f424..afef30d 100644 --- a/tests/cli_e2e.test.ts +++ b/tests/cli_e2e.test.ts @@ -494,7 +494,7 @@ test("local Git subscribe, merge, conflict recovery, and scoped publication", as const failed = skillsync(fixture, ["--json", "subscribe", upstream], false); expect(failed.json.ok).toBe(false); -}); +}, { timeout: 30000 }); test("tampered persisted paths are rejected without reading outside state", async () => { const fixture = await makeFixture(); From 4cce5933b1e7eec414119fa17fb28110daa5c7f7 Mon Sep 17 00:00:00 2001 From: Nicholai Vogel Date: Thu, 17 Sep 2026 02:30:10 -0600 Subject: [PATCH 7/8] Restore shared file data import --- src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.rs b/src/main.rs index 872f894..dd828ce 100644 --- a/src/main.rs +++ b/src/main.rs @@ -26,7 +26,7 @@ use filesystem::{ assert_no_symlink_path, atomic, canonicalize_path, checked_regular_path, copy_tree, discover, effective_library_path, files, hash_dir, manifest_name, read_regular_file, replace_dir_bound, resolve_library_path, safe, snapshot_transaction, source_rel, - strict_component, validate_state_path, StateLock, + strict_component, validate_state_path, FileData, StateLock, }; #[cfg(unix)] use filesystem::{open_child_file, open_directory_fd}; From ccf3f3f37b8e3fa26b4af0e76fffe2e28ad667a5 Mon Sep 17 00:00:00 2001 From: Nicholai Vogel Date: Thu, 17 Sep 2026 02:34:28 -0600 Subject: [PATCH 8/8] Stabilize Windows conflict tests --- tests/conflict_resolution_e2e.test.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/tests/conflict_resolution_e2e.test.ts b/tests/conflict_resolution_e2e.test.ts index c221af1..602d29c 100644 --- a/tests/conflict_resolution_e2e.test.ts +++ b/tests/conflict_resolution_e2e.test.ts @@ -28,7 +28,10 @@ async function fixture(): Promise { const library = join(root, "library"); const gitConfig = join(root, "gitconfig"); await mkdir(home, { recursive: true }); - await writeFile(gitConfig, "[user]\n\tname = Skillsync Bun Test\n\temail = skillsync@example.invalid\n"); + await writeFile( + gitConfig, + "[user]\n\tname = Skillsync Bun Test\n\temail = skillsync@example.invalid\n[core]\n\tautocrlf = false\n", + ); fixtureRoots.add(root); return { root, @@ -122,7 +125,7 @@ test("explicit conflict selection resumes incoming and retains immutable evidenc expect(repeated.status).toBe("synced"); expect(repeated.recovery_retained).toBe(true); expect(await readFile(livePath, "utf8")).toBe("name: demo\nincoming change\n"); -}); +}, { timeout: 30000 }); test("rejects a tampered current-version conflict selection on repeated resume", async () => { const { f, relationship, statePath } = await conflictFixture(); @@ -135,7 +138,7 @@ test("rejects a tampered current-version conflict selection on repeated resume", const rejected = run(f, ["--json", "conflicts", "resume", relationship], false); expect(rejected.json.ok).toBe(false); expect(rejected.json.message).toContain("invalid conflict side selection"); -}); +}, { timeout: 30000 }); test("stale live content blocks conflict selection without mutation", async () => { const { f, relationship, statePath, livePath } = await conflictFixture(); @@ -149,7 +152,7 @@ test("stale live content blocks conflict selection without mutation", async () = expect(await readFile(statePath, "utf8")).toBe(beforeState); expect(await readdir(join(f.config, "recovery"))).toEqual(beforeRecovery); expect(await readFile(livePath, "utf8")).toBe("name: demo\nnewer user edit\n"); -}); +}, { timeout: 30000 }); test("migrates pre-v5 baseline source identity for conflict inspection", async () => { const { f, relationship, statePath } = await conflictFixture(); @@ -163,7 +166,7 @@ test("migrates pre-v5 baseline source identity for conflict inspection", async ( expect(shown.status).toBe("conflict"); expect(shown.baseline_source).toBe(state.subscriptions[relationship].source); expect(shown.baseline_source_path).toBe(state.subscriptions[relationship].source_path); -}); +}, { timeout: 30000 }); test("resume rolls back live and baseline on state-save failure", async () => { const { f, relationship, statePath, livePath } = await conflictFixture(); @@ -186,7 +189,7 @@ test("resume rolls back live and baseline on state-save failure", async () => { expect(await readFile(livePath, "utf8")).toBe("name: demo\nlocal change\n"); expect(await readFile(join(baselinePath, "SKILL.md"), "utf8")).toBe(beforeBaseline); expect(await readdir(join(f.config, "recovery"))).toEqual(beforeRecovery); -}); +}, { timeout: 30000 }); test("resume rolls back both replacements when the second commit preparation fails", async () => { const { f, relationship, statePath, livePath } = await conflictFixture(); @@ -207,4 +210,4 @@ test("resume rolls back both replacements when the second commit preparation fai expect(await readFile(statePath, "utf8")).toBe(beforeState); expect(await readFile(livePath, "utf8")).toBe("name: demo\nlocal change\n"); expect(await readFile(join(baselinePath, "SKILL.md"), "utf8")).toBe(beforeBaseline); -}); +}, { timeout: 30000 });