Merge branch '2.1-improve-undefined-var-warning' into v2.1

Author: cowwoc

.cat/issues/v2/v2.1/improve-undefined-var-warning/index.json

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

.cat/issues/v2/v2.1/improve-undefined-var-warning/plan.md

@@ -15,54 +15,117 @@ cmd > "${SESSION_DIR}/squash-complete-${ISSUE_ID}"
 ```
 
 ## Expected vs Actual
-- **Expected:** `WARNING: Cannot verify Bash redirect to variable-expanded path: "${SESSION_DIR}/squash-complete-${ISSUE_ID}"\n\nUndefined variables: SESSION_DIR, ISSUE_ID\n...`
+- **Expected:** `WARNING: Cannot verify Bash redirect to variable-expanded path: "${SESSION_DIR}/squash-complete-${ISSUE_ID}"\n\nUndefined variable(s): SESSION_DIR, ISSUE_ID\n...`
 - **Actual:** `WARNING: Cannot verify Bash redirect to variable-expanded path: "${SESSION_DIR}/squash-complete-${ISSUE_ID}"\n\nOne or more variables in the path could not be resolved.\n...`
 
 ## Root Cause
 `ShellParser.expandEnvVars()` returns `null` on the first undefined variable but does not report which
 variable failed. The caller in `BlockWorktreeIsolationViolation` has no way to name the undefined
 variable(s) without re-scanning the target string.
 
+The fix adds `ShellParser.findUndefinedVars()` which performs a second pass to collect the names,
+called only when `expandEnvVars` already returned `null`.
+
 ## Risk Assessment
 - **Risk Level:** LOW
-- **Regression Risk:** Only changes the text of a warning message; no logic changes
-- **Mitigation:** Add a unit test for the new `findUndefinedVars` method
+- **Regression Risk:** Only changes the text of a warning message and adds a new method. No logic changes to existing methods.
+- **Mitigation:** Unit tests for `findUndefinedVars`; full `mvn verify` confirms no regressions.
 
 ## Files to Modify
 - `client/src/main/java/io/github/cowwoc/cat/claude/hook/ShellParser.java` — add `findUndefinedVars(String, Function<String,String>)` method
-- `client/src/main/java/io/github/cowwoc/cat/claude/hook/bash/BlockWorktreeIsolationViolation.java` — replace "One or more variables..." with a list of the undefined variable names
-- `client/src/test/java/io/github/cowwoc/cat/client/test/ShellParserTest.java` (or equivalent) — add tests for `findUndefinedVars`
-
-## Test Cases
-- [ ] Path with one undefined variable → list shows that one variable name
-- [ ] Path with two undefined variables → list shows both names in order
-- [ ] Path with all variables defined → returns empty list (no warning triggered)
+- `client/src/main/java/io/github/cowwoc/cat/claude/hook/bash/BlockWorktreeIsolationViolation.java` — replace "One or more variables..." with specific variable names from `findUndefinedVars`
+- `client/src/test/java/io/github/cowwoc/cat/client/test/ShellParserTest.java` — add tests for `findUndefinedVars`
 
 ## Pre-conditions
 - [ ] All dependent issues are closed
 
 ## Jobs
 
-### Job 1: Add findUndefinedVars to ShellParser
-- Add `public static List<String> findUndefinedVars(String target, Function<String, String> envLookup)` to `ShellParser`
-  - Scan `target` for `${VAR}` and `$VAR` references using `ENV_VAR_EXPAND_PATTERN`
-  - For each match, call `envLookup`; if it returns `null`, add the variable name to the result list
-  - Return the list (preserving order of appearance, no deduplication needed)
-  - Files: `client/src/main/java/io/github/cowwoc/cat/claude/hook/ShellParser.java`
-
-### Job 2: Update warning message in BlockWorktreeIsolationViolation
-- After `expandEnvVars` returns `null`, call `ShellParser.findUndefinedVars(target, mergedLookup)` to get the undefined names
-- Replace "One or more variables in the path could not be resolved." with "Undefined variable(s): ${names joined by ", "}"
-  - Files: `client/src/main/java/io/github/cowwoc/cat/claude/hook/bash/BlockWorktreeIsolationViolation.java`
-
-### Job 3: Add tests
-- Add unit tests for `findUndefinedVars`:
-  - One undefined variable
-  - Two undefined variables
-  - All defined (empty result)
-  - Files: test source file for `ShellParser`
+### Job 1
+Steps must be executed sequentially (each step depends on the previous):
+
+1. **Write failing tests first** (TDD) in `client/src/test/java/io/github/cowwoc/cat/client/test/ShellParserTest.java`:
+   - `findUndefinedVars_oneUndefinedVariable` — path `"${SESSION_DIR}/file"` with no env lookup hits → returns `["SESSION_DIR"]`
+   - `findUndefinedVars_twoUndefinedVariables` — path `"${SESSION_DIR}/squash-complete-${ISSUE_ID}"` with no env lookup hits → returns `["SESSION_DIR", "ISSUE_ID"]`
+   - `findUndefinedVars_allDefined` — path `"${SESSION_DIR}/file"` with lookup returning `/tmp` for `SESSION_DIR` → returns empty list
+   - Tests call `ShellParser.findUndefinedVars(target, envLookup)` with a lambda for `envLookup`
+   - Tests will fail to compile until Step 2 adds the method
+
+2. **Add `findUndefinedVars` to `ShellParser`** in `client/src/main/java/io/github/cowwoc/cat/claude/hook/ShellParser.java`:
+   - Signature: `public static List<String> findUndefinedVars(String target, Function<String, String> envLookup)`
+   - Javadoc: document parameters, return value, NullPointerException for null inputs
+   - Body: use `ENV_VAR_EXPAND_PATTERN` (already defined in the class) to scan `target`:
+     ```java
+     List<String> undefined = new ArrayList<>();
+     Matcher varMatcher = ENV_VAR_EXPAND_PATTERN.matcher(target);
+     while (varMatcher.find())
+     {
+       String varName;
+       if (varMatcher.group(1) != null)
+         varName = varMatcher.group(1);
+       else
+         varName = varMatcher.group(2);
+       if (envLookup.apply(varName) == null)
+         undefined.add(varName);
+     }
+     return undefined;
+     ```
+   - Add `requireThat` validation for `target` and `envLookup` (same pattern as `expandEnvVars`)
+   - Place the method directly after the existing `expandEnvVars(String, Function<String,String>)` method
+   - The `List` import is already present in the file (`import java.util.List;`); verify before adding imports
+
+3. **Update warning message** in `client/src/main/java/io/github/cowwoc/cat/claude/hook/bash/BlockWorktreeIsolationViolation.java`:
+   - Find the block at line ~162 where `expanded == null` is handled
+   - After `String expanded = ShellParser.expandEnvVars(target, mergedLookup);` returns null, call:
+     ```java
+     List<String> undefinedVars = ShellParser.findUndefinedVars(target, mergedLookup);
+     String undefinedList = String.join(", ", undefinedVars);
+     ```
+   - Build the second line of the warning from `undefinedList`:
+     - If `undefinedList` is non-empty (the common case): use `"Undefined variable(s): " + undefinedList`
+     - If `undefinedList` is empty (e.g., the `$` comes from `$(...)` command substitution, which the pattern does not match): fall back to `"One or more variables in the path could not be resolved."`
+     - Implement this as:
+       ```java
+       String variableLine;
+       if (undefinedList.isEmpty())
+         variableLine = "One or more variables in the path could not be resolved.";
+       else
+         variableLine = "Undefined variable(s): " + undefinedList;
+       ```
+   - The full updated message template (replacing lines 162-175) should be:
+     ```java
+     List<String> undefinedVars = ShellParser.findUndefinedVars(target, mergedLookup);
+     String undefinedList = String.join(", ", undefinedVars);
+     String variableLine;
+     if (undefinedList.isEmpty())
+       variableLine = "One or more variables in the path could not be resolved.";
+     else
+       variableLine = "Undefined variable(s): " + undefinedList;
+     String message = """
+       WARNING: Cannot verify Bash redirect to variable-expanded path: %s
+
+       %s
+       Variables must be defined earlier in the same script as a simple literal assignment, e.g.:
+         VAR="/absolute/path"
+         cmd > "${VAR}"
+
+       Variables set via command substitution ($(...)) or unset variables cannot be resolved statically.
+       If this targets a path outside your worktree, it bypasses worktree isolation.
+       Use the Edit or Write tools with an explicit absolute path instead:
+         Use: %s/plugin/file.txt
+         Not: $UNSET_VAR/plugin/file.txt""".formatted(target, variableLine, context.absoluteWorktreePath());
+     ```
+   - Add `import java.util.List;` if not already present (check existing imports first)
+
+4. **Run `mvn -f client/pom.xml verify -e`** and confirm exit code 0
+
+5. **Commit all changes** in a single commit:
+   - Stage: `ShellParser.java`, `BlockWorktreeIsolationViolation.java`, `ShellParserTest.java`, `index.json`
+   - Commit type: `bugfix:`
+   - Message: `bugfix: name undefined variables in worktree isolation warning`
+   - Update `index.json` in same commit: `status: "closed"`, `resolution: "implemented"`
 
 ## Post-conditions
-- [ ] Warning message names the specific undefined variable(s) (e.g., `Undefined variable(s): SESSION_DIR, ISSUE_ID`)
-- [ ] `ShellParser.findUndefinedVars` unit tests pass
+- [ ] Warning message names the specific undefined variable(s) (e.g., `Undefined variable(s): SESSION_DIR, ISSUE_ID`) instead of the generic "One or more variables..."
+- [ ] `ShellParser.findUndefinedVars` tests pass: one undefined var, two undefined vars, all defined
 - [ ] `mvn -f client/pom.xml verify -e` exits 0 with no new failures

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

@@ -177,12 +177,7 @@ public static String expandEnvVars(String target, Function<String, String> envLo
     while (varMatcher.find())
     {
       result.append(target, lastEnd, varMatcher.start());
-      // group(1) is the name from ${VAR}; group(2) is the name from $VAR
-      String varName;
-      if (varMatcher.group(1) != null)
-        varName = varMatcher.group(1);
-      else
-        varName = varMatcher.group(2);
+      String varName = extractVarName(varMatcher);
       String value = envLookup.apply(varName);
       if (value == null)
         return null;
@@ -193,6 +188,52 @@ public static String expandEnvVars(String target, Function<String, String> envLo
     return result.toString();
   }
 
+  /**
+   * Returns the names of all {@code $VAR} and {@code ${VAR}} references in {@code target} whose
+   * values are not present in {@code envLookup}.
+   * <p>
+   * This method performs a second-pass scan used when {@link #expandEnvVars(String, Function)}
+   * already returned {@code null}, to report which specific variable(s) could not be resolved.
+   *
+   * @param target    the string containing variable references to inspect
+   * @param envLookup a function mapping variable names to their values; returns {@code null} if
+   *                  the variable is unset
+   * @return a list of variable names for which {@code envLookup} returned {@code null}, in the
+   *         order they appear in {@code target}; empty if all variables are defined
+   * @throws NullPointerException if {@code target} or {@code envLookup} are null
+   */
+  public static List<String> findUndefinedVars(String target, Function<String, String> envLookup)
+  {
+    requireThat(target, "target").isNotNull();
+    requireThat(envLookup, "envLookup").isNotNull();
+    List<String> undefined = new ArrayList<>();
+    Matcher varMatcher = ENV_VAR_EXPAND_PATTERN.matcher(target);
+    while (varMatcher.find())
+    {
+      String varName = extractVarName(varMatcher);
+      if (envLookup.apply(varName) == null)
+        undefined.add(varName);
+    }
+    return undefined;
+  }
+
+  /**
+   * Extracts the variable name from a matcher positioned on a match of
+   * {@code ENV_VAR_EXPAND_PATTERN}.
+   * <p>
+   * Group 1 captures the name from the {@code ${VAR}} form; group 2 captures it from
+   * the {@code $VAR} form. Exactly one of the two groups is non-null on every match.
+   *
+   * @param matcher a matcher that has just found a match
+   * @return the captured variable name
+   */
+  private static String extractVarName(Matcher matcher)
+  {
+    if (matcher.group(1) != null)
+      return matcher.group(1);
+    return matcher.group(2);
+  }
+
   /**
    * Scans {@code script} for simple literal variable assignments and returns them as a map.
    * <p>

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

@@ -18,6 +18,7 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
+import java.util.StringJoiner;
 import java.util.function.Function;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -159,10 +160,22 @@ public Result check(ClaudeHook scope)
         String expanded = ShellParser.expandEnvVars(target, mergedLookup);
         if (expanded == null)
         {
+          List<String> undefinedVars = ShellParser.findUndefinedVars(target, mergedLookup);
+          if (undefinedVars.isEmpty())
+          {
+            // expandEnvVars returned null only when a variable is undefined, so this list
+            // cannot be empty. A non-empty list here means the pattern missed a $(...) form.
+            throw new AssertionError(
+              "findUndefinedVars returned empty list after expandEnvVars returned null for: " + target);
+          }
+          StringJoiner joiner = new StringJoiner(", ");
+          for (String varName : undefinedVars)
+            joiner.add(varName);
+          String variableLine = "Undefined variable(s): " + joiner;
           String message = """
             WARNING: Cannot verify Bash redirect to variable-expanded path: %s
 
-            One or more variables in the path could not be resolved.
+            %s
             Variables must be defined earlier in the same script as a simple literal assignment, e.g.:
               VAR="/absolute/path"
               cmd > "${VAR}"
@@ -171,7 +184,7 @@ Variables set via command substitution ($(...)) or unset variables cannot be res
             If this targets a path outside your worktree, it bypasses worktree isolation.
             Use the Edit or Write tools with an explicit absolute path instead:
               Use: %s/plugin/file.txt
-              Not: $UNSET_VAR/plugin/file.txt""".formatted(target, context.absoluteWorktreePath());
+              Not: $UNSET_VAR/plugin/file.txt""".formatted(target, variableLine, context.absoluteWorktreePath());
           return Result.block(message);
         }
         // Replacing the variable reference with the concrete path lets the worktree isolation

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

@@ -563,7 +563,7 @@ public void variableExpansionDollarSignIsBlocked() throws IOException
         TestUtils.bashHook(command, projectPath.toString(), SESSION_ID, scope));
 
       requireThat(result.blocked(), "blocked").isTrue();
-      requireThat(result.reason(), "reason").contains("could not be resolved");
+      requireThat(result.reason(), "reason").contains("Undefined variable(s): BWIV_TEST_UNDEFINED_ENV_VAR");
       requireThat(result.reason(), "reason").contains(
         worktreeDir.toAbsolutePath().normalize().toString());
     }
@@ -935,7 +935,7 @@ public void redirectRemainsBlockedWhenVariableAssignedViaCommandSubstitution() t
         TestUtils.bashHook(command, projectPath.toString(), SESSION_ID, scope));
 
       requireThat(result.blocked(), "blocked").isTrue();
-      requireThat(result.reason(), "reason").contains("could not be resolved");
+      requireThat(result.reason(), "reason").contains("Undefined variable(s): OUT");
     }
     finally
     {