[pitboss] sweep after phase 05: 2 deferred items resolved

src/dynamic/build_sandbox.rs

@@ -333,8 +333,13 @@ pub fn prepare_node(spec: &HarnessSpec, workdir: &Path) -> Result<BuildResult, B
     let lockfile_hash = compute_node_lockfile_hash(workdir);
     let cache_path = build_cache_path(&lockfile_hash, "node", &spec.toolchain_id)?;
 
-    // Cache hit: node_modules already installed.
+    // Cache hit: node_modules already installed. Restore to fresh workdir if
+    // a different finding shares the same cache key but got a new workdir.
     if cache_path.join(".node_cache_done").exists() {
+        let cached_nm = cache_path.join("node_modules");
+        if cached_nm.exists() && !workdir.join("node_modules").exists() {
+            let _ = copy_dir_all(&cached_nm, &workdir.join("node_modules"));
+        }
         return Ok(BuildResult {
             venv_path: cache_path,
             cache_hit: true,
@@ -363,6 +368,12 @@ pub fn prepare_node(spec: &HarnessSpec, workdir: &Path) -> Result<BuildResult, B
         }
         match try_npm_install(workdir) {
             Ok(()) => {
+                // 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<BuildResult, Bu
     let cache_path = build_cache_path(&lockfile_hash, "php", &spec.toolchain_id)?;
 
     if cache_path.join(".php_cache_done").exists() {
+        let cached_vendor = cache_path.join("vendor");
+        if cached_vendor.exists() && !workdir.join("vendor").exists() {
+            let _ = copy_dir_all(&cached_vendor, &workdir.join("vendor"));
+        }
         return Ok(BuildResult {
             venv_path: cache_path,
             cache_hit: true,
@@ -647,6 +681,12 @@ pub fn prepare_php(spec: &HarnessSpec, workdir: &Path) -> Result<BuildResult, Bu
         }
         match try_composer_install(workdir) {
             Ok(()) => {
+                // 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");
+    }
 }

src/dynamic/toolchain.rs

@@ -352,7 +352,7 @@ fn try_package_json_engines(root: &Path) -> Option<ToolchainResolution> {
     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<String> {
     // Find the second quoted value after the colon.
@@ -624,7 +641,7 @@ fn try_composer_json(root: &Path) -> Option<ToolchainResolution> {
     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"));
+    }
 }