From 62bd480db36c4a9b956b0295d1c5ec4c72df65a6 Mon Sep 17 00:00:00 2001 From: pitboss Date: Tue, 12 May 2026 02:35:58 -0400 Subject: [PATCH] [pitboss] sweep after phase 05: 2 deferred items resolved --- src/dynamic/build_sandbox.rs | 69 ++++++++++++++++++++++++++++++++- src/dynamic/toolchain.rs | 74 +++++++++++++++++++++++++++++++++++- 2 files changed, 140 insertions(+), 3 deletions(-) diff --git a/src/dynamic/build_sandbox.rs b/src/dynamic/build_sandbox.rs index 9af56a84..15d1392a 100644 --- a/src/dynamic/build_sandbox.rs +++ b/src/dynamic/build_sandbox.rs @@ -333,8 +333,13 @@ pub fn prepare_node(spec: &HarnessSpec, workdir: &Path) -> Result Result { + // Persist node_modules to cache so future runs with the same + // package.json but a fresh workdir can restore without re-running npm. + let nm_src = workdir.join("node_modules"); + if nm_src.exists() { + let _ = copy_dir_all(&nm_src, &cache_path.join("node_modules")); + } let _ = std::fs::write(cache_path.join(".node_cache_done"), b"done"); return Ok(BuildResult { venv_path: cache_path, @@ -396,6 +407,25 @@ fn try_npm_install(workdir: &Path) -> Result<(), String> { Ok(()) } +/// Recursively copy a directory tree from `src` to `dst`. +/// +/// Silently skips entries that cannot be copied. Used to persist +/// `node_modules`/`vendor` to the build cache and restore them on cache hit. +fn copy_dir_all(src: &Path, dst: &Path) -> std::io::Result<()> { + std::fs::create_dir_all(dst)?; + for entry in std::fs::read_dir(src)? { + let entry = entry?; + let ty = entry.file_type()?; + let dst_path = dst.join(entry.file_name()); + if ty.is_dir() { + copy_dir_all(&entry.path(), &dst_path)?; + } else { + std::fs::copy(entry.path(), &dst_path)?; + } + } + Ok(()) +} + fn compute_node_lockfile_hash(workdir: &Path) -> String { let mut h = Hasher::new(); for fname in &["package.json", "package-lock.json", "yarn.lock", "pnpm-lock.yaml"] { @@ -620,6 +650,10 @@ pub fn prepare_php(spec: &HarnessSpec, workdir: &Path) -> Result Result { + // Persist vendor/ to cache so future runs with the same + // composer.json but a fresh workdir can restore without re-running composer. + let vendor_src = workdir.join("vendor"); + if vendor_src.exists() { + let _ = copy_dir_all(&vendor_src, &cache_path.join("vendor")); + } let _ = std::fs::write(cache_path.join(".php_cache_done"), b"done"); return Ok(BuildResult { venv_path: cache_path, @@ -738,4 +778,31 @@ mod tests { let h2 = compute_java_source_hash(dir.path()); assert_eq!(h1, h2); } + + #[test] + fn copy_dir_all_copies_recursively() { + let src = tempfile::TempDir::new().unwrap(); + let dst = tempfile::TempDir::new().unwrap(); + + std::fs::write(src.path().join("a.txt"), b"hello").unwrap(); + std::fs::create_dir(src.path().join("sub")).unwrap(); + std::fs::write(src.path().join("sub").join("b.txt"), b"world").unwrap(); + + copy_dir_all(src.path(), dst.path()).unwrap(); + + assert_eq!(std::fs::read(dst.path().join("a.txt")).unwrap(), b"hello"); + assert_eq!(std::fs::read(dst.path().join("sub").join("b.txt")).unwrap(), b"world"); + } + + #[test] + fn copy_dir_all_creates_dst_if_absent() { + let src = tempfile::TempDir::new().unwrap(); + std::fs::write(src.path().join("x.txt"), b"x").unwrap(); + + let dst_parent = tempfile::TempDir::new().unwrap(); + let dst = dst_parent.path().join("new_dir"); + // dst does not yet exist — copy_dir_all must create it. + copy_dir_all(src.path(), &dst).unwrap(); + assert_eq!(std::fs::read(dst.join("x.txt")).unwrap(), b"x"); + } } diff --git a/src/dynamic/toolchain.rs b/src/dynamic/toolchain.rs index 1cb1b014..363ebdc2 100644 --- a/src/dynamic/toolchain.rs +++ b/src/dynamic/toolchain.rs @@ -352,7 +352,7 @@ fn try_package_json_engines(root: &Path) -> Option { let mut in_engines = false; for line in content.lines() { let trimmed = line.trim(); - if trimmed.contains("\"engines\"") { + if json_line_has_key(trimmed, "engines") { in_engines = true; } if in_engines && trimmed.contains("\"node\"") { @@ -410,6 +410,23 @@ fn map_node_version(version: &str, origin: PinOrigin) -> ToolchainResolution { } } +/// Return true if `line` contains `"key":` as a JSON object key assignment. +/// +/// Prevents false-positives from values like `"type": "require"` that would +/// otherwise match a plain `contains("\"key\"")` check. +fn json_line_has_key(line: &str, key: &str) -> bool { + let needle = format!("\"{key}\""); + let mut search = line; + while let Some(pos) = search.find(needle.as_str()) { + let rest = &search[pos + needle.len()..]; + if rest.trim_start().starts_with(':') { + return true; + } + search = &search[pos + 1..]; + } + false +} + /// Extract a version string from a JSON value like `">=18"` or `"20.x"`. fn extract_version_from_json_value(line: &str) -> Option { // Find the second quoted value after the colon. @@ -624,7 +641,7 @@ fn try_composer_json(root: &Path) -> Option { let mut in_require = false; for line in content.lines() { let trimmed = line.trim(); - if trimmed.contains("\"require\"") { + if json_line_has_key(trimmed, "require") { in_require = true; } if in_require && trimmed.contains("\"php\"") { @@ -884,4 +901,57 @@ mod tests { assert_eq!(r.toolchain_id, "php-8"); assert_eq!(r.pin_origin, PinOrigin::SystemDefault); } + + #[test] + fn php_composer_json_require_dev_before_require() { + // "require-dev" must not shadow the real "require" block even when it + // appears first. The tightened json_line_has_key check prevents false + // activation on the "require-dev" key. + let dir = TempDir::new().unwrap(); + fs::write( + dir.path().join("composer.json"), + "{\n \"require-dev\": {\n \"php\": \"^7.0\"\n },\n \"require\": {\n \"php\": \">=8.1\"\n }\n}", + ).unwrap(); + let r = resolve_php(dir.path()); + assert_eq!(r.toolchain_id, "php-8.1"); + assert_eq!(r.pin_origin, PinOrigin::ComposerJson); + } + + #[test] + fn php_composer_json_require_as_value_not_matched() { + // "require" appearing as a string value (not a key) must not activate + // in_require and cause a php constraint from an unrelated block to be + // returned. Without the json_line_has_key fix, a line like + // `"type": "require"` would set in_require=true, letting the "php" + // key inside require-dev be matched instead of falling through. + let dir = TempDir::new().unwrap(); + fs::write( + dir.path().join("composer.json"), + "{\n \"extra\": {\"type\": \"require\"},\n \"require-dev\": {\n \"php\": \"^7.0\"\n }\n}", + ).unwrap(); + let r = resolve_php(dir.path()); + // No real "require": key present — must fall back to system default. + assert_eq!(r.pin_origin, PinOrigin::SystemDefault); + } + + // ── json_line_has_key unit tests ───────────────────────────────────────── + + #[test] + fn json_line_has_key_matches_exact_key() { + assert!(json_line_has_key(r#" "require": {"#, "require")); + assert!(json_line_has_key(r#"{"require": {}}"#, "require")); + assert!(json_line_has_key(r#" "engines" : {"#, "engines")); + } + + #[test] + fn json_line_has_key_rejects_key_in_value() { + assert!(!json_line_has_key(r#" "type": "require","#, "require")); + assert!(!json_line_has_key(r#" "desc": "engines config","#, "engines")); + } + + #[test] + fn json_line_has_key_rejects_superstring_key() { + // "require-dev" does not contain "require" as a quoted key. + assert!(!json_line_has_key(r#" "require-dev": {"#, "require")); + } }