diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml new file mode 100644 index 0000000..93a3ad2 --- /dev/null +++ b/.github/workflows/windows.yml @@ -0,0 +1,47 @@ +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: Configure Git test behavior + run: git config --global core.autocrlf false + + - 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: Build test-hook CLI + run: cargo build --locked --features test-hooks --target-dir target/test-hooks + + - 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 98aedea..f663857 100644 --- a/README.md +++ b/README.md @@ -49,13 +49,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: @@ -63,14 +63,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. @@ -78,4 +78,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..c400a74 100644 --- a/src/config_editor.rs +++ b/src/config_editor.rs @@ -161,7 +161,10 @@ 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::filesystem::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 +400,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 +410,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/conflicts.rs b/src/conflicts.rs index d083c3a..8d054fc 100644 --- a/src/conflicts.rs +++ b/src/conflicts.rs @@ -7,8 +7,8 @@ use std::{ }; use crate::filesystem::{ - assert_no_symlink_path, copy_tree, hash_dir, manifest_name, replace_dir_bound, safe, - snapshot_transaction, strict_component, validate_state_path, + assert_no_symlink_path, canonicalize_path, copy_tree, hash_dir, manifest_name, + replace_dir_bound, safe, snapshot_transaction, strict_component, validate_state_path, }; use crate::{relationship_key, App}; @@ -78,7 +78,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!( @@ -130,7 +130,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!( @@ -208,7 +208,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" )); @@ -245,7 +245,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 a9347c4..5cb6f97 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; @@ -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) @@ -152,12 +166,46 @@ 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, OPEN_EXISTING, + }; + const GENERIC_READ_ACCESS: u32 = 0x8000_0000; + + 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_ACCESS, + 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() { + return Err(anyhow!("directory changed during open: {}", path.display())); + } + Ok(opened) } #[cfg(unix)] @@ -179,6 +227,75 @@ pub(crate) fn open_directory_file_bound(path: &Path) -> Result { Ok(opened) } +#[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, 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 + .as_os_str() + .encode_wide() + .chain(iter::once(0)) + .collect::>(); + 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( + 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 +344,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 +480,44 @@ 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, 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")?; + let wide = path + .as_os_str() + .encode_wide() + .chain(iter::once(0)) + .collect::>(); + let handle = unsafe { + CreateFileW( + wide.as_ptr(), + 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 }, + 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); @@ -370,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"); @@ -412,9 +589,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 +614,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 +638,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,8 +676,9 @@ impl StateLock { )?, config_identity(config)?, ); + #[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)" )); @@ -506,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") } @@ -579,7 +781,39 @@ 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 + .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 + .downcast_ref::() + .map(|error| error.kind() == std::io::ErrorKind::NotFound) + .unwrap_or(false) + { + drop(directory_file); + Ok(None) + } else { + 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))), @@ -700,7 +934,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) } @@ -805,6 +1039,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 +1179,21 @@ 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)] + { + let file = open_regular_file_bound(path, false, false)?; + let metadata = file.metadata()?; + if let Some(before) = expected { + if before.len() != metadata.len() { + 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 +1483,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 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(()) + } + #[cfg(not(any(unix, windows)))] { let _ = (root, relative); Err(anyhow!( @@ -1243,6 +1509,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)] @@ -1345,7 +1626,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 +1656,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 +1755,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 +1810,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 +1897,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 +2013,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 +2165,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_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" + )); + } + 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 +2302,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 +2348,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 +2513,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/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 ec5830a..dd828ce 100644 --- a/src/main.rs +++ b/src/main.rs @@ -23,10 +23,10 @@ mod worker; pub(crate) use worker::{worker_status, WORKER_STOP_REQUESTED}; use filesystem::{ - assert_no_symlink_path, atomic, 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, FileData, StateLock, + 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, FileData, StateLock, }; #[cfg(unix)] use filesystem::{open_child_file, open_directory_fd}; @@ -303,7 +303,22 @@ 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 @@ -482,7 +497,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 +521,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 +534,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 { @@ -810,13 +825,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 d531935..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, }; @@ -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!( @@ -242,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")); } @@ -296,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" )); @@ -427,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")?; @@ -526,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) @@ -594,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/src/worker.rs b/src/worker.rs index bc403ae..ca6d7e1 100644 --- a/src/worker.rs +++ b/src/worker.rs @@ -11,7 +11,7 @@ use std::{ }; use crate::{ - filesystem::{checked_regular_path, open_advisory_lock, WorkerLease}, + filesystem::{checked_regular_path, lock_is_contended, open_advisory_lock, WorkerLease}, repository, App, }; @@ -28,7 +28,7 @@ pub(crate) 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 10a0a2c..31f2e7a 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 })).sort(); 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({}); }); @@ -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(); @@ -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 () => { 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 }); 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"); }); 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 {