refactor: add injectable env to expandEnvVars; remove $HOME dependency from tests

Author: cowwoc

.cat/issues/v2/v2.1/refactor-env-var-expansion-tests/index.json

@@ -1,5 +1,7 @@
 {
-  "status" : "open",
-  "dependencies" : [],
-  "blocks" : []
-}
+  "status" : "closed",
+  "resolution" : "implemented",
+  "dependencies" : [ ],
+  "blocks" : [ ],
+  "target_branch" : "v2.1"
+}

client/src/main/java/io/github/cowwoc/cat/claude/hook/ShellParser.java

@@ -12,6 +12,7 @@
 import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.function.Function;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -139,8 +140,28 @@ public static Path resolvePath(String path, String base)
    * @throws NullPointerException if {@code target} is null
    */
   public static String expandEnvVars(String target)
+  {
+    return expandEnvVars(target, System::getenv);
+  }
+
+  /**
+   * Expands {@code $VAR} and {@code ${VAR}} references in a string using the supplied lookup
+   * function.
+   * <p>
+   * Returns {@code null} if any variable is unset (i.e., the lookup function returns {@code null}
+   * for that variable name), so the caller can fall back to conservative behavior rather than
+   * evaluating a partially-expanded path.
+   *
+   * @param target    the string containing variable references to expand
+   * @param envLookup a function mapping variable names to their values; returns {@code null} if
+   *                  the variable is unset
+   * @return the fully expanded string, or {@code null} if any variable was undefined
+   * @throws NullPointerException if {@code target} or {@code envLookup} are null
+   */
+  public static String expandEnvVars(String target, Function<String, String> envLookup)
   {
     requireThat(target, "target").isNotNull();
+    requireThat(envLookup, "envLookup").isNotNull();
     Matcher varMatcher = ENV_VAR_EXPAND_PATTERN.matcher(target);
     StringBuilder result = new StringBuilder();
     int lastEnd = 0;
@@ -153,7 +174,7 @@ public static String expandEnvVars(String target)
         varName = varMatcher.group(1);
       else
         varName = varMatcher.group(2);
-      String value = System.getenv(varName);
+      String value = envLookup.apply(varName);
       if (value == null)
         return null;
       result.append(value);

client/src/main/java/io/github/cowwoc/cat/claude/hook/bash/BlockWorktreeIsolationViolation.java

@@ -17,6 +17,8 @@
 import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Map;
+import java.util.function.Function;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -39,8 +41,9 @@
  * Only paths under the project directory are checked. Writes to {@code /tmp} or other locations
  * outside the project directory are allowed. If no session lock exists, all commands are allowed.
  * <p>
- * Write targets containing {@code $VAR} or {@code ${VAR}} references are expanded using
- * {@link ShellParser#expandEnvVars(String)} before the isolation check. If any variable is unset,
+ * Write targets containing {@code $VAR} or {@code ${VAR}} references are expanded before the
+ * isolation check. Use the second constructor to inject a controlled environment lookup for
+ * testing, or the first constructor to use the system environment. If any variable is unset,
  * the path cannot be verified and the command is blocked conservatively. Backtick expressions
  * (e.g., {@code `cmd`}) cannot be expanded statically and are always blocked conservatively.
  */
@@ -71,9 +74,12 @@ public final class BlockWorktreeIsolationViolation implements BashHandler
 
   private final Path projectPath;
   private final JsonMapper mapper;
+  private final Function<String, String> envLookup;
 
   /**
    * Creates a new handler for blocking worktree isolation violations.
+   * <p>
+   * Uses the system environment when expanding shell variable references in write targets.
    *
    * @param scope the JVM scope providing access to shared resources
    * @throws NullPointerException if {@code scope} is null
@@ -82,6 +88,27 @@ public BlockWorktreeIsolationViolation(ClaudeHook scope)
   {
     this.projectPath = scope.getProjectPath();
     this.mapper = scope.getJsonMapper();
+    this.envLookup = System::getenv;
+  }
+
+  /**
+   * Creates a new handler for blocking worktree isolation violations with an injectable
+   * environment map.
+   * <p>
+   * Uses the provided environment map when expanding shell variable references in write targets.
+   * This constructor is intended for testing, where a controlled fake environment can be
+   * substituted for the system environment.
+   *
+   * @param scope the JVM scope providing access to shared resources
+   * @param env   the environment map used for shell variable expansion
+   * @throws NullPointerException if {@code scope} or {@code env} are null
+   */
+  public BlockWorktreeIsolationViolation(ClaudeHook scope, Map<String, String> env)
+  {
+    requireThat(env, "env").isNotNull();
+    this.projectPath = scope.getProjectPath();
+    this.mapper = scope.getJsonMapper();
+    this.envLookup = env::get;
   }
 
   @Override
@@ -118,7 +145,7 @@ public Result check(ClaudeHook scope)
       }
       if (target.contains("$"))
       {
-        String expanded = ShellParser.expandEnvVars(target);
+        String expanded = ShellParser.expandEnvVars(target, envLookup);
         if (expanded == null)
         {
           String message = """

client/src/test/java/io/github/cowwoc/cat/client/test/BlockWorktreeIsolationViolationTest.java

@@ -9,12 +9,12 @@
 import io.github.cowwoc.cat.claude.hook.BashHandler;
 
 import io.github.cowwoc.cat.claude.hook.bash.BlockWorktreeIsolationViolation;
-import org.testng.SkipException;
 import org.testng.annotations.Test;
 
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.Map;
 
 import static io.github.cowwoc.requirements13.java.DefaultJavaValidators.requireThat;
 
@@ -677,26 +677,28 @@ public void readOnlyCommandIsAllowed() throws IOException
   @Test
   public void allowsRedirectWhenEnvVarExpandsToWorktreePath() throws IOException
   {
-    String home = System.getenv("HOME");
-    if (home == null || home.isBlank())
-      throw new SkipException("HOME environment variable is not set; skipping env-var expansion test");
-    Path projectPath = Files.createTempDirectory(Path.of(home), "bwiv-test-");
-    try (TestClaudeHook scope = new TestClaudeHook(projectPath, projectPath, projectPath))
+    Path fakeHome = Files.createTempDirectory("fake-home-");
+    try
     {
-      TestUtils.writeLockFile(scope, ISSUE_ID, SESSION_ID);
-      Path worktreeDir = TestUtils.createWorktreeDir(scope, ISSUE_ID);
-      String relativePath = Path.of(home).relativize(worktreeDir.resolve("file.txt")).toString();
-      String command = "echo foo > ${HOME}/" + relativePath;
+      Path projectPath = Files.createTempDirectory(fakeHome, "bwiv-test-");
+      try (TestClaudeHook scope = new TestClaudeHook(projectPath, projectPath, projectPath))
+      {
+        TestUtils.writeLockFile(scope, ISSUE_ID, SESSION_ID);
+        Path worktreeDir = TestUtils.createWorktreeDir(scope, ISSUE_ID);
+        String relativePath = fakeHome.relativize(worktreeDir.resolve("file.txt")).toString();
+        String command = "echo foo > ${HOME}/" + relativePath;
+        Map<String, String> env = Map.of("HOME", fakeHome.toString());
 
-      BlockWorktreeIsolationViolation handler = new BlockWorktreeIsolationViolation(scope);
-      BashHandler.Result result = handler.check(
-        TestUtils.bashHook(command, projectPath.toString(), SESSION_ID, scope));
+        BlockWorktreeIsolationViolation handler = new BlockWorktreeIsolationViolation(scope, env);
+        BashHandler.Result result = handler.check(
+          TestUtils.bashHook(command, projectPath.toString(), SESSION_ID, scope));
 
-      requireThat(result.blocked(), "blocked").isFalse();
+        requireThat(result.blocked(), "blocked").isFalse();
+      }
     }
     finally
     {
-      TestUtils.deleteDirectoryRecursively(projectPath);
+      TestUtils.deleteDirectoryRecursively(fakeHome);
     }
   }
 
@@ -711,26 +713,28 @@ public void allowsRedirectWhenEnvVarExpandsToWorktreePath() throws IOException
   @Test
   public void allowsRedirectWhenBareEnvVarExpandsToWorktreePath() throws IOException
   {
-    String home = System.getenv("HOME");
-    if (home == null || home.isBlank())
-      throw new SkipException("HOME environment variable is not set; skipping env-var expansion test");
-    Path projectPath = Files.createTempDirectory(Path.of(home), "bwiv-test-");
-    try (TestClaudeHook scope = new TestClaudeHook(projectPath, projectPath, projectPath))
+    Path fakeHome = Files.createTempDirectory("fake-home-");
+    try
     {
-      TestUtils.writeLockFile(scope, ISSUE_ID, SESSION_ID);
-      Path worktreeDir = TestUtils.createWorktreeDir(scope, ISSUE_ID);
-      String relativePath = Path.of(home).relativize(worktreeDir.resolve("file.txt")).toString();
-      String command = "echo foo > $HOME/" + relativePath;
+      Path projectPath = Files.createTempDirectory(fakeHome, "bwiv-test-");
+      try (TestClaudeHook scope = new TestClaudeHook(projectPath, projectPath, projectPath))
+      {
+        TestUtils.writeLockFile(scope, ISSUE_ID, SESSION_ID);
+        Path worktreeDir = TestUtils.createWorktreeDir(scope, ISSUE_ID);
+        String relativePath = fakeHome.relativize(worktreeDir.resolve("file.txt")).toString();
+        String command = "echo foo > $HOME/" + relativePath;
+        Map<String, String> env = Map.of("HOME", fakeHome.toString());
 
-      BlockWorktreeIsolationViolation handler = new BlockWorktreeIsolationViolation(scope);
-      BashHandler.Result result = handler.check(
-        TestUtils.bashHook(command, projectPath.toString(), SESSION_ID, scope));
+        BlockWorktreeIsolationViolation handler = new BlockWorktreeIsolationViolation(scope, env);
+        BashHandler.Result result = handler.check(
+          TestUtils.bashHook(command, projectPath.toString(), SESSION_ID, scope));
 
-      requireThat(result.blocked(), "blocked").isFalse();
+        requireThat(result.blocked(), "blocked").isFalse();
+      }
     }
     finally
     {
-      TestUtils.deleteDirectoryRecursively(projectPath);
+      TestUtils.deleteDirectoryRecursively(fakeHome);
     }
   }
 
@@ -746,27 +750,29 @@ public void allowsRedirectWhenBareEnvVarExpandsToWorktreePath() throws IOExcepti
   @Test
   public void blocksRedirectWhenEnvVarExpandsOutsideWorktree() throws IOException
   {
-    String home = System.getenv("HOME");
-    if (home == null || home.isBlank())
-      throw new SkipException("HOME environment variable is not set; skipping env-var expansion test");
-    Path projectPath = Files.createTempDirectory(Path.of(home), "bwiv-test-");
-    try (TestClaudeHook scope = new TestClaudeHook(projectPath, projectPath, projectPath))
-    {
-      TestUtils.writeLockFile(scope, ISSUE_ID, SESSION_ID);
-      TestUtils.createWorktreeDir(scope, ISSUE_ID);
-      String relativePath = Path.of(home).relativize(projectPath.resolve("plugin/file.txt")).toString();
-      String command = "echo foo > ${HOME}/" + relativePath;
-
-      BlockWorktreeIsolationViolation handler = new BlockWorktreeIsolationViolation(scope);
-      BashHandler.Result result = handler.check(
-        TestUtils.bashHook(command, projectPath.toString(), SESSION_ID, scope));
-
-      requireThat(result.blocked(), "blocked").isTrue();
-      requireThat(result.reason(), "reason").contains("isolation violation");
+    Path fakeHome = Files.createTempDirectory("fake-home-");
+    try
+    {
+      Path projectPath = Files.createTempDirectory(fakeHome, "bwiv-test-");
+      try (TestClaudeHook scope = new TestClaudeHook(projectPath, projectPath, projectPath))
+      {
+        TestUtils.writeLockFile(scope, ISSUE_ID, SESSION_ID);
+        TestUtils.createWorktreeDir(scope, ISSUE_ID);
+        String relativePath = fakeHome.relativize(projectPath.resolve("plugin/file.txt")).toString();
+        String command = "echo foo > ${HOME}/" + relativePath;
+        Map<String, String> env = Map.of("HOME", fakeHome.toString());
+
+        BlockWorktreeIsolationViolation handler = new BlockWorktreeIsolationViolation(scope, env);
+        BashHandler.Result result = handler.check(
+          TestUtils.bashHook(command, projectPath.toString(), SESSION_ID, scope));
+
+        requireThat(result.blocked(), "blocked").isTrue();
+        requireThat(result.reason(), "reason").contains("isolation violation");
+      }
     }
     finally
     {
-      TestUtils.deleteDirectoryRecursively(projectPath);
+      TestUtils.deleteDirectoryRecursively(fakeHome);
     }
   }
 

client/src/test/java/io/github/cowwoc/cat/client/test/EnforceJvmScopeEnvAccessTest.java

@@ -146,7 +146,8 @@ public void whitelistedFilesCallSystemGetenv() throws IOException
       boolean fileExists = Files.exists(filePath);
       requireThat(fileExists, "fileExists").withContext(relativePath, "whitelistedFile").isTrue();
       String content = Files.readString(filePath);
-      boolean hasGetenv = content.contains("System.getenv(");
+      // Accept both direct call System.getenv( and method reference System::getenv
+      boolean hasGetenv = content.contains("System.getenv(") || content.contains("System::getenv");
       requireThat(hasGetenv, "hasGetenv").withContext(relativePath, "whitelistedFile").isTrue();
     }
   }