diff --git a/crates/code-intel-cli/src/artifacts.rs b/crates/code-intel-cli/src/artifacts.rs index 641b5e5c..95ac2c54 100644 --- a/crates/code-intel-cli/src/artifacts.rs +++ b/crates/code-intel-cli/src/artifacts.rs @@ -320,6 +320,76 @@ pub(crate) fn resolve_artifact_root(explicit: Option<&Path>) -> Result .join("artifacts")) } +/// Creates `path` (and any missing ancestors), tolerating a Windows quirk +/// this crate's issue #279 investigation verified against a real, pre-existing +/// cross-drive directory symlink (`$env:LOCALAPPDATA\code-intel\artifacts` on the +/// investigating machine, symlinked onto another drive to route a cache +/// directory to a bigger/faster disk — a normal setup, not an exotic one): +/// `CreateDirectoryW` for a brand-new subdirectory can return +/// `ERROR_ALREADY_EXISTS` (os error 183) when an ancestor path component is a +/// symlink onto a *different* volume, even though the target does not exist +/// before or after the failed call, and an immediate retry against the same +/// unresolved path fails identically — not a race. `create_dir_all`'s own +/// recovery (treat `AlreadyExists` as success when `path.is_dir()`) can't +/// catch this, because `is_dir()` resolves through the very same reparse +/// point and also reports `false`. +/// +/// This is the root cause of "create repository authority root: … already +/// exists (os error 183)" (exit 74): a `code-intel .` quick-start run hit it +/// on the very first invocation, not merely "the second run." A same-drive +/// symlink does not reproduce it. Curiously, neither does a *freshly created* +/// cross-drive symlink within the same process that then immediately reads +/// through it (tried against several drives, and against an external process +/// creating the link) — only the long-settled, pre-existing one did, every +/// time. Whatever OS-level cache this depends on evidently needs the reparse +/// point to have existed for a while, which makes the exact trigger resist +/// synthetic, on-demand reproduction in a test process even though it is +/// consistently, deterministically reproducible on an affected real machine. +/// +/// The workaround: canonicalize the nearest already-existing ancestor +/// (resolving past the symlink) and retry the create against the resolved +/// form. Returns the path now confirmed to exist — `path` unchanged in the +/// common (non-symlinked) case, or the resolved replacement when the +/// workaround fired. Callers must use the returned path for further nested +/// creates underneath it, since `path` itself remains unusable for that. +/// +/// A genuine collision (something real blocking the path, e.g. a file with +/// that name) still fails, returning the underlying `io::Error` unchanged so +/// callers can report it with full path context. +pub(crate) fn ensure_directory(path: &Path) -> std::io::Result { + match fs::create_dir_all(path) { + Ok(()) => Ok(path.to_path_buf()), + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists && !path.is_dir() => { + let Some(existing_ancestor) = path.ancestors().find(|candidate| candidate.is_dir()) + else { + return Err(error); + }; + let resolved_ancestor = fs::canonicalize(existing_ancestor)?; + let resolved_target = + resolved_retry_target(path, existing_ancestor, &resolved_ancestor); + fs::create_dir_all(&resolved_target)?; + Ok(resolved_target) + } + Err(error) => Err(error), + } +} + +/// Path arithmetic for the recovery branch above, split out so it is +/// unit-testable without depending on the Windows-only OS state that reaches +/// it in production: rejoins the part of `path` beyond `existing_ancestor` +/// onto `resolved_ancestor` (the canonicalized, symlink-free form of that +/// same ancestor). +fn resolved_retry_target( + path: &Path, + existing_ancestor: &Path, + resolved_ancestor: &Path, +) -> PathBuf { + let remainder = path + .strip_prefix(existing_ancestor) + .unwrap_or(Path::new("")); + resolved_ancestor.join(remainder) +} + /// Composes a DAG staging directory the way `resume` and `artifact index` read /// runs back: `//`. The PowerShell launcher owned /// this composition until it was retired, and the failure it existed to prevent diff --git a/crates/code-intel-cli/src/artifacts_tests.rs b/crates/code-intel-cli/src/artifacts_tests.rs index b497e199..a1b58ee5 100644 --- a/crates/code-intel-cli/src/artifacts_tests.rs +++ b/crates/code-intel-cli/src/artifacts_tests.rs @@ -148,3 +148,173 @@ fn classify_contract_requires_research_for_upstream_or_tool_blockers() { assert!(provider_quota > 0 || local_tool_error > 0 || sentrux_fail > 0); } + +#[test] +fn ensure_directory_creates_a_fresh_nested_directory() { + let dir = unique_temp_dir("ensure-directory-fresh"); + let target = dir.join("nested").join("leaf"); + + let resolved = ensure_directory(&target).expect("create a fresh nested directory"); + assert_eq!(resolved, target); + assert!(resolved.is_dir()); + + let _ = fs::remove_dir_all(&dir); +} + +#[test] +fn ensure_directory_is_idempotent_for_an_existing_directory() { + // Re-running against an already-analyzed repo is the ordinary case, not + // an exotic one: the authority root container directory is meant to be + // reused run after run. + let dir = unique_temp_dir("ensure-directory-idempotent"); + fs::create_dir_all(&dir).expect("fixture dir"); + + let resolved = ensure_directory(&dir).expect("re-create an already-existing directory"); + assert_eq!(resolved, dir); + assert!(resolved.is_dir()); + + let _ = fs::remove_dir_all(&dir); +} + +#[test] +fn ensure_directory_reports_a_real_file_collision_with_its_path_preserved() { + // A genuine blocker (something real occupying the path) must still fail + // — self-healing is only for the false-positive Windows symlink case + // below, never for an actual collision. + let dir = unique_temp_dir("ensure-directory-file-collision"); + fs::create_dir_all(&dir).expect("fixture parent"); + let blocked = dir.join("blocked"); + fs::write(&blocked, b"not a directory").expect("fixture blocking file"); + + let error = + ensure_directory(&blocked).expect_err("a real file must still block directory creation"); + assert_eq!(error.kind(), std::io::ErrorKind::AlreadyExists); + + let _ = fs::remove_dir_all(&dir); +} + +/// Issue #279's root cause, isolated from the rest of `code-intel`: on +/// Windows, `CreateDirectoryW` for a brand-new subdirectory can return +/// `ERROR_ALREADY_EXISTS` (os error 183) when an ancestor path component is a +/// symlink onto a *different* volume, even though the target does not exist +/// before or after the failed call, and an immediate retry against the same +/// unresolved path fails identically — not a race. +/// `create_dir_all`'s own recovery (`AlreadyExists` + `path.is_dir()` == +/// success) can't catch this: `is_dir()` resolves through the same reparse +/// point and also reports `false`. +/// +/// This was verified against a real, pre-existing cross-drive directory +/// symlink and reproduced every time. It is deliberately *not* a hard +/// assertion here, because it turns out not to be reliably fabricatable +/// on demand: neither a same-drive symlink, nor a *freshly created* +/// cross-drive symlink (this process or an external `mklink /D` process, +/// against multiple different drives, against both a brand-new empty target +/// and the real long-settled one) reproduced it — only the long-settled, +/// already-existing one did. Whatever OS-level state this depends on +/// evidently needs the reparse point to have existed for a while, which a +/// single test process cannot manufacture. So: attempt the real thing: +/// if this host happens to reproduce the quirk, assert `ensure_directory` +/// recovers from it; if not (the likely outcome, including in CI, which is a +/// freshly provisioned VM with no aged filesystem state), skip rather than +/// assert something false. `resolved_retry_target_rejoins_the_remainder_ +/// under_the_canonicalized_ancestor` below covers the recovery arithmetic +/// deterministically instead. Mirrors this crate's existing convention for +/// environment-dependent symlink privilege (see `decision_record.rs`'s +/// `symlink_file(...).is_ok()` checks) rather than requiring elevation. +#[cfg(windows)] +#[test] +fn ensure_directory_recovers_from_a_spurious_already_exists_through_a_cross_volume_symlink() { + let Some(other_drive) = second_writable_drive() else { + eprintln!("skipping: no second drive available to build a cross-volume symlink"); + return; + }; + + let real_target = other_drive.join(format!( + "code-intel-ensure-directory-target-{}", + std::process::id() + )); + fs::create_dir_all(&real_target).expect("create real target on the other drive"); + + let link_parent = unique_temp_dir("ensure-directory-symlink-parent"); + fs::create_dir_all(&link_parent).expect("create link parent"); + let link = link_parent.join("artifacts"); + if std::os::windows::fs::symlink_dir(&real_target, &link).is_err() { + eprintln!("skipping: could not create a directory symlink (Developer Mode / privilege?)"); + let _ = fs::remove_dir_all(&real_target); + let _ = fs::remove_dir_all(&link_parent); + return; + } + + let child = link.join("repo-name"); + + // First, confirm this environment actually reproduces the quirk being + // worked around — if it doesn't, the rest of the assertion proves + // nothing. + let raw = fs::create_dir_all(&child); + let reproduces_quirk = matches!( + &raw, + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists + ) && !child.is_dir(); + if !reproduces_quirk { + eprintln!("skipping: this host did not reproduce the cross-volume symlink quirk: {raw:?}"); + let _ = fs::remove_dir_all(&real_target); + let _ = fs::remove_dir_all(&link_parent); + return; + } + + let resolved = ensure_directory(&child) + .unwrap_or_else(|error| panic!("did not recover from the symlink quirk: {error}")); + assert!(resolved.is_dir(), "resolved={}", resolved.display()); + + let _ = fs::remove_dir_all(&real_target); + let _ = fs::remove_dir_all(&link_parent); +} + +/// Finds a drive letter distinct from `env::temp_dir()`'s own that exists and +/// is writable, for building a genuinely cross-volume symlink. Returns `None` +/// on a lone-drive machine rather than guessing or failing. +#[cfg(windows)] +fn second_writable_drive() -> Option { + let own_letter = env::temp_dir() + .to_str() + .and_then(|path| path.chars().next()) + .map(|letter| letter.to_ascii_uppercase()); + for letter in ['D', 'E', 'F', 'G'] { + if Some(letter) == own_letter { + continue; + } + let root = PathBuf::from(format!("{letter}:\\")); + if !root.is_dir() { + continue; + } + let probe = root.join(format!("code-intel-drive-probe-{}", std::process::id())); + if fs::create_dir(&probe).is_ok() { + let _ = fs::remove_dir(&probe); + return Some(root); + } + } + None +} + +/// Deterministic, platform-independent coverage for `ensure_directory`'s +/// recovery arithmetic (rejoining the part of the original target beyond the +/// existing ancestor onto that ancestor's canonicalized form) — the part of +/// the fix that does not depend on fabricating the Windows-only OS quirk +/// covered on a best-effort basis above. +#[test] +fn resolved_retry_target_rejoins_the_remainder_under_the_canonicalized_ancestor() { + let path = Path::new("artifacts").join("fixture-repo"); + let existing_ancestor = Path::new("artifacts"); + let resolved_ancestor = Path::new("real-drive").join("cache").join("code-intel"); + assert_eq!( + resolved_retry_target(&path, existing_ancestor, &resolved_ancestor), + resolved_ancestor.join("fixture-repo") + ); + + // The target itself, with nothing beyond the existing ancestor, rejoins + // to exactly the resolved ancestor. + assert_eq!( + resolved_retry_target(existing_ancestor, existing_ancestor, &resolved_ancestor), + resolved_ancestor + ); +} diff --git a/crates/code-intel-cli/src/authoritative_run/execution_kernel.rs b/crates/code-intel-cli/src/authoritative_run/execution_kernel.rs index 89a84e64..390f7ec3 100644 --- a/crates/code-intel-cli/src/authoritative_run/execution_kernel.rs +++ b/crates/code-intel-cli/src/authoritative_run/execution_kernel.rs @@ -216,9 +216,12 @@ fn nest_authority_root(authority_root: &Path, repo_name: &str) -> Result" used to + /// carry zero path context at either site. A file sitting where the + /// repo-nested directory needs to go is a portable, deterministic way to + /// exercise a *genuine* block without depending on the Windows-only + /// cross-volume-symlink quirk `ensure_directory` self-heals (covered by + /// `artifacts_tests.rs`). + #[test] + fn nest_authority_root_names_the_path_when_something_real_blocks_it() { + let root = unique_temp_dir("nest-blocked"); + std::fs::create_dir_all(&root).expect("fixture authority root"); + let blocked = root.join("widget-repo"); + std::fs::write(&blocked, b"not a directory").expect("fixture blocking file"); + + let error = nest_authority_root(&root, "widget-repo") + .expect_err("a real file must still block authority root creation"); + assert_eq!(error.exit_code, 74); + assert!( + error.message.contains(&blocked.display().to_string()), + "message omits the blocked path: {}", + error.message + ); + + std::fs::remove_dir_all(&root).ok(); + } } diff --git a/crates/code-intel-cli/src/project_context.rs b/crates/code-intel-cli/src/project_context.rs index 701a0d75..ab026f8b 100644 --- a/crates/code-intel-cli/src/project_context.rs +++ b/crates/code-intel-cli/src/project_context.rs @@ -132,11 +132,18 @@ impl ProjectContext { pub(crate) fn run(&self, intent: RunIntent) -> Result { ensure_run_authority(&self.artifact_root, &self.repo, &self.repo_path)?; - fs::create_dir_all(&self.artifact_root) - .map_err(|error| ProjectError::host_io(format!("create artifact root: {error}")))?; - let authority_root = self.artifact_root.join(&self.repo); - fs::create_dir_all(&authority_root).map_err(|error| { - ProjectError::host_io(format!("create repository authority root: {error}")) + let artifact_root = artifacts::ensure_directory(&self.artifact_root).map_err(|error| { + ProjectError::host_io(format!( + "create artifact root {}: {error}", + self.artifact_root.display() + )) + })?; + let authority_root = artifact_root.join(&self.repo); + let authority_root = artifacts::ensure_directory(&authority_root).map_err(|error| { + ProjectError::host_io(format!( + "create repository authority root {}: {error}", + authority_root.display() + )) })?; let nonce = SystemTime::now() .duration_since(UNIX_EPOCH) diff --git a/crates/code-intel-cli/src/project_context/tests.rs b/crates/code-intel-cli/src/project_context/tests.rs index 7126d6d1..31ad4678 100644 --- a/crates/code-intel-cli/src/project_context/tests.rs +++ b/crates/code-intel-cli/src/project_context/tests.rs @@ -67,6 +67,50 @@ fn resolves_native_paths_and_repository_key_when_directories_contain_spaces() { let _ = fs::remove_dir_all(root); } +/// Issue #279: this call site and `execution_kernel.rs::nest_authority_root` +/// share the exact "create repository authority root: " +/// message template — this one is what the `code-intel .` quick start +/// actually goes through. A file sitting where the per-repo authority +/// directory needs to go is a portable, deterministic way to exercise a +/// *genuine* block. (The Windows-only cross-volume-symlink false positive +/// this bug was actually filed for needs a real second drive to reproduce; +/// it is covered by `artifacts_tests.rs`'s `ensure_directory_*` tests +/// instead, against the shared helper directly.) +#[test] +fn run_names_the_authority_root_path_when_something_real_blocks_it() { + let root = env::temp_dir().join(format!( + "code-intel-project-context-authority-blocked-{}", + process::id() + )); + let repo = root.join("fixture-repo"); + let artifact_root = root.join("artifacts"); + fs::create_dir_all(&repo).expect("create repository fixture"); + fs::create_dir_all(&artifact_root).expect("create artifact root fixture"); + let blocked = artifact_root.join("fixture-repo"); + fs::write(&blocked, b"not a directory").expect("fixture blocking file"); + + let context = ProjectContext::resolve( + ProjectSelector::new(repo.clone()).with_artifact_root(Some(artifact_root.clone())), + ) + .expect("resolve project context"); + + // `RunAnswer` does not derive `Debug`, so `expect_err` (which requires it + // on the `Ok` side too) is not available here — match instead. + match context.run(RunIntent::new(RunMode::Lite)) { + Ok(_) => panic!("a real file must still block authority root creation"), + Err(error) => { + assert_eq!(error.exit_code(), 74); + assert!( + error.message().contains(&blocked.display().to_string()), + "message omits the blocked path: {}", + error.message() + ); + } + } + + let _ = fs::remove_dir_all(root); +} + #[test] fn explicit_repository_key_cannot_escape_the_artifact_root() { let root = env::temp_dir().join(format!("code-intel-project-context-key-{}", process::id())); diff --git a/crates/code-intel-cli/tests/primary_entry.rs b/crates/code-intel-cli/tests/primary_entry.rs index 5ac06d1f..24f0371c 100644 --- a/crates/code-intel-cli/tests/primary_entry.rs +++ b/crates/code-intel-cli/tests/primary_entry.rs @@ -124,6 +124,55 @@ fn project_query_resolves_repository_context_before_loading_evidence() { let _ = std::fs::remove_dir_all(root); } +/// Issue #279: `code-intel .` (this file's +/// `root_help_leads_with_the_compiled_primary_entry` confirms it's the +/// documented quick start) used to fold the authority-root bootstrap's IO +/// error — including a Windows-only false +/// positive from a symlinked artifact root — into a bare "create repository +/// authority root: " with no path at all. Drives the exact +/// call site (`project_context.rs::run`) end to end through the compiled +/// binary. A file occupying the per-repo authority directory is a portable, +/// deterministic way to force a genuine block; the symlink false positive +/// itself needs a real second drive, so it's covered directly against the +/// shared helper in `artifacts_tests.rs` instead. +#[test] +fn root_entry_names_the_authority_root_path_when_a_file_blocks_it() { + let root = std::env::temp_dir().join(format!( + "code-intel-primary-entry-authority-blocked-{}", + std::process::id() + )); + let repo = root.join("fixture-repo"); + let artifacts = root.join("artifacts"); + std::fs::create_dir_all(&repo).expect("create repository fixture"); + std::fs::create_dir_all(&artifacts).expect("create artifact root fixture"); + let blocked = artifacts.join("fixture-repo"); + std::fs::write(&blocked, b"not a directory").expect("fixture blocking file"); + + let output = common::cli() + .arg(&repo) + .args(["--mode", "lite", "--json"]) + .env("CODE_INTEL_ARTIFACT_ROOT", &artifacts) + .output() + .expect("run code-intel against a blocked authority root"); + + assert_eq!(output.status.code(), Some(74)); + assert!(output.stderr.is_empty()); + let result: serde_json::Value = + serde_json::from_slice(&output.stdout).expect("error output is JSON"); + assert_eq!(result["schema"], "code-intel-primary-result.v1"); + assert_eq!(result["outcome"], "error"); + assert_eq!(result["exitCode"], 74); + let diagnostic = result["diagnostic"] + .as_str() + .expect("diagnostic is a string"); + assert!( + diagnostic.contains(&blocked.display().to_string()), + "diagnostic omits the blocked path: {diagnostic}" + ); + + let _ = std::fs::remove_dir_all(root); +} + #[test] fn named_commands_are_not_misclassified_as_repository_paths() { let output = common::cli()