bugfix: scope test failure detection to known test runner commands and enforce single-lock-per-session

Author: cowwoc

.cat/issues/v2/v2.1/fix-post-bash-hook-test-failure-false-positive/PLAN.md

@@ -59,6 +59,11 @@ Explicitly exclude commands or output that are clearly diffs:
 - Don't pattern-match on `git diff` output
 - Don't pattern-match on rendered diff output from tools like `get-output get-diff`
 
+## Assumptions
+
+- A session holds at most one issue lock at a time. `EnforceWorktreePathIsolation` uses the
+  session's single lock file to identify the active worktree path.
+
 ## Scope
 
 ### In scope

.cat/issues/v2/v2.1/fix-post-bash-hook-test-failure-false-positive/STATE.md

@@ -1,8 +1,8 @@
 # State
 
-- **Status:** open
-- **Resolution:** unresolved
-- **Progress:** 0%
+- **Status:** closed
+- **Resolution:** implemented
+- **Progress:** 100%
 - **Dependencies:** []
 - **Blocks:** []
-- **Target Branch:** v2.1
+- **Target Branch:** v2.1

.cat/retrospectives/mistakes-2026-03.json

@@ -275,6 +275,112 @@ else
 String command = commandNode != null ? commandNode.asString() : "";
 ```
 
+### Optional Unwrapping
+When an `Optional` is used only once — immediately in a single method chain like `map().orElse()` or `isPresent()` — inline the constructor call rather than storing it in a named variable:
+
+```java
+// Good — no intermediate variable needed
+Path branchDir = WorktreeContext.forSession(
+    scope.getCatWorkPath(), projectPath, scope.getJsonMapper(), sessionId).
+  map(WorktreeContext::absoluteWorktreePath).
+  orElse(projectPath);
+
+// Avoid — intermediate variable adds no information
+Optional<WorktreeContext> context = WorktreeContext.forSession(
+  scope.getCatWorkPath(), projectPath, scope.getJsonMapper(), sessionId);
+Path branchDir = context.map(WorktreeContext::absoluteWorktreePath).orElse(projectPath);
+```
+
+When extracting a value from an `Optional` with a fallback, use `map().orElse()` instead of an `if (isPresent()) / get()`
+block:
+
+```java
+// Good - concise, idiomatic
+return context.map(WorktreeContext::absoluteWorktreePath).orElse(projectPath);
+
+// Avoid - verbose, error-prone
+if (context.isPresent())
+  return context.get().absoluteWorktreePath();
+return projectPath;
+```
+
+Branch on an `Optional` to select a **value**, not a code path. When both branches perform the same operation on
+different values, extract the `Optional` to a single variable and call the operation once:
+
+```java
+// Good — extract the path, call the operation once
+Path branchDir = context.map(WorktreeContext::absoluteWorktreePath).orElse(projectPath);
+try
+{
+  return GitCommands.getCurrentBranch(branchDir.toString());
+}
+catch (IOException _)
+{
+  return null;
+}
+
+// Avoid — same operation duplicated in both branches
+if (context.isEmpty())
+{
+  try
+  {
+    return GitCommands.getCurrentBranch(projectPath.toString());
+  }
+  catch (IOException _)
+  {
+    return null;
+  }
+}
+try
+{
+  return GitCommands.getCurrentBranch(context.get().absoluteWorktreePath().toString());
+}
+catch (IOException _)
+{
+  return null;
+}
+```
+
+When only the **empty** case has work to do, return early on `isPresent()` rather than wrapping the work in an
+`isEmpty()` block. This avoids a gratuitous level of nesting:
+
+```java
+// Good — early return eliminates nesting
+if (context.isPresent())
+  return null;
+String target = extractCheckoutTarget(command);
+if (!isCheckoutFlag(target))
+  return Result.block("Blocked: Cannot checkout '%s' in main worktree.".formatted(target));
+return null;
+
+// Avoid — work buried inside isEmpty() block
+if (context.isEmpty())
+{
+  String target = extractCheckoutTarget(command);
+  if (!isCheckoutFlag(target))
+    return Result.block("Blocked: Cannot checkout '%s' in main worktree.".formatted(target));
+}
+return null;
+```
+
+When the present-case requires multiple statements (a block body), extract the value using `orElse(null)` and test
+for null rather than using a two-step `isEmpty()` check followed by `.get()`:
+
+```java
+// Good — single null check, no isEmpty/get split
+WorktreeContext context = WorktreeContext.forSession(...).orElse(null);
+if (context == null)
+  return Result.allow();
+// use context directly
+context.absoluteWorktreePath();
+
+// Avoid — isEmpty check followed by separate .get() call
+Optional<WorktreeContext> contextOptional = WorktreeContext.forSession(...);
+if (contextOptional.isEmpty())
+  return Result.allow();
+WorktreeContext context = contextOptional.get();
+```
+
 ### Null-First Conditionals
 When a conditional handles both the null and non-null case, test the null case first. This applies to explicit
 if/else and to early-return patterns:

.claude/rules/java.md

@@ -14,6 +14,7 @@
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.Optional;
 
 /**
  * Resolved worktree context for a session.
@@ -41,18 +42,18 @@ public record WorktreeContext(Path absoluteWorktreePath, Path absoluteProjectDir
    * Resolves the worktree context for a session by looking up the session's lock file and
    * deriving the worktree path.
    * <p>
-   * Returns {@code null} if no active worktree is found for the session (no lock file or
-   * worktree directory does not exist).
+   * A session holds at most one active lock at a time. Returns an empty optional if no
+   * active worktree is found for the session (no lock file or worktree directory does not exist).
    *
    * @param projectCatDir the project CAT directory ({@code {claudeProjectPath}/.cat/work/})
    * @param projectPath the project root directory
    * @param mapper the JSON mapper for reading lock files
    * @param sessionId the session ID to look up
-   * @return the resolved worktree context, or {@code null} if no active worktree exists
+   * @return the resolved worktree context, or empty if no active worktree exists
    * @throws NullPointerException if any parameter is null
    * @throws IllegalArgumentException if {@code sessionId} is blank
    */
-  public static WorktreeContext forSession(Path projectCatDir, Path projectPath, JsonMapper mapper,
+  public static Optional<WorktreeContext> forSession(Path projectCatDir, Path projectPath, JsonMapper mapper,
     String sessionId)
   {
     requireThat(projectCatDir, "projectCatDir").isNotNull();
@@ -64,15 +65,15 @@ public static WorktreeContext forSession(Path projectCatDir, Path projectPath, J
     {
       String issueId = WorktreeLock.findIssueIdForSession(projectCatDir, mapper, sessionId);
       if (issueId == null)
-        return null;
+        return Optional.empty();
 
       Path worktreePath = projectCatDir.resolve("worktrees").resolve(issueId);
       if (!Files.isDirectory(worktreePath))
-        return null;
+        return Optional.empty();
 
       Path absoluteWorktreePath = worktreePath.toAbsolutePath().normalize();
       Path absoluteProjectDirectory = projectPath.toAbsolutePath().normalize();
-      return new WorktreeContext(absoluteWorktreePath, absoluteProjectDirectory);
+      return Optional.of(new WorktreeContext(absoluteWorktreePath, absoluteProjectDirectory));
     }
     catch (IOException e)
     {

client/src/main/java/io/github/cowwoc/cat/hooks/WorktreeContext.java

@@ -14,6 +14,7 @@
 
 import java.io.IOException;
 import java.nio.file.Path;
+import java.util.Optional;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -138,20 +139,18 @@ private Result checkCheckoutInMainWorktree(String command, String sessionId)
       }
     }
 
-    WorktreeContext context = WorktreeContext.forSession(
+    Optional<WorktreeContext> context = WorktreeContext.forSession(
       scope.getCatWorkPath(), projectPath, scope.getJsonMapper(), sessionId);
-    if (context == null)
+    if (context.isPresent())
+      return null;
+    // No active worktree for this session — this is the main context; block checkout
+    String target = extractCheckoutTarget(command);
+    if (!isCheckoutFlag(target))
     {
-      // No active worktree for this session — this is the main context; block checkout
-      String target = extractCheckoutTarget(command);
-      if (!isCheckoutFlag(target))
-      {
-        return Result.block(String.format(
-          "Blocked: Cannot checkout '%s' in main worktree. Use issue worktrees instead.",
-          target));
-      }
+      return Result.block(String.format(
+        "Blocked: Cannot checkout '%s' in main worktree. Use issue worktrees instead.",
+        target));
     }
-
     return null;
   }
 
@@ -209,24 +208,13 @@ private String getCurrentBranch(String command, String sessionId)
     }
 
     // Use lock-based worktree context to determine branch
-    WorktreeContext context = WorktreeContext.forSession(
-      scope.getCatWorkPath(), projectPath, scope.getJsonMapper(), sessionId);
-    if (context == null)
-    {
-      // No active worktree for this session — commands run in main context
-      try
-      {
-        return GitCommands.getCurrentBranch(projectPath.toString());
-      }
-      catch (IOException _)
-      {
-        return null;
-      }
-    }
-    // In a worktree — determine branch from worktree directory
+    Path branchDir = WorktreeContext.forSession(
+        scope.getCatWorkPath(), projectPath, scope.getJsonMapper(), sessionId).
+      map(WorktreeContext::absoluteWorktreePath).
+      orElse(projectPath);
     try
     {
-      return GitCommands.getCurrentBranch(context.absoluteWorktreePath().toString());
+      return GitCommands.getCurrentBranch(branchDir.toString());
     }
     catch (IOException _)
     {

client/src/main/java/io/github/cowwoc/cat/hooks/bash/BlockMainRebase.java

@@ -95,7 +95,8 @@ public Result check(HookInput input)
     requireThat(workingDirectory, "workingDirectory").isNotNull();
     requireThat(sessionId, "sessionId").isNotBlank();
 
-    WorktreeContext context = WorktreeContext.forSession(scope.getCatWorkPath(), projectPath, mapper, sessionId);
+    WorktreeContext context = WorktreeContext.forSession(
+      scope.getCatWorkPath(), projectPath, mapper, sessionId).orElse(null);
     if (context == null)
       return Result.allow();
 

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

@@ -10,19 +10,48 @@
 import io.github.cowwoc.cat.hooks.HookInput;
 import tools.jackson.databind.JsonNode;
 
+import java.util.List;
 import java.util.regex.Pattern;
 
 /**
  * Detect command failures and suggest learning from mistakes.
  * <p>
  * Trigger: PostToolUse for Bash
+ * <p>
+ * Pattern matching is scoped to known test runner commands to avoid false positives from arbitrary
+ * Bash commands whose output happens to contain failure keywords (e.g., git diff showing diff content
+ * with "FAILED" in a file, or cat reading a file with "Exception" in its text).
  */
 public final class DetectFailures implements BashHandler
 {
   private static final Pattern FAILURE_PATTERN = Pattern.compile(
-    "BUILD FAILED|FAILED|ERROR:|error:|Exception|FATAL|fatal:",
+    "BUILD FAILED|FAILED|ERROR:|Exception|FATAL",
     Pattern.CASE_INSENSITIVE);
 
+  /**
+   * Command prefixes that identify known test runners.
+   * <p>
+   * Pattern matching is applied only when the executed command starts with one of these prefixes.
+   * Commands not in this list (e.g., git, cat, javac, get-output) are skipped to prevent false positives
+   * from incidental failure keywords in diff output or file content.
+   */
+  private static final List<String> TEST_RUNNER_PREFIXES = List.of(
+    "mvn ",
+    "mvnw ",
+    "./mvnw ",
+    "gradle ",
+    "gradlew ",
+    "./gradlew ",
+    "bats ",
+    "bats\t",
+    "npm test",
+    "npm run test",
+    "yarn test",
+    "pytest",
+    "cargo test",
+    "go test",
+    "dotnet test");
+
   /**
    * Creates a new handler for detecting command failures.
    */
@@ -31,6 +60,28 @@ public DetectFailures()
     // Handler class
   }
 
+  /**
+   * Checks whether the given command is a known test runner invocation.
+   *
+   * @param command the bash command string to check
+   * @return true if the command starts with a known test runner prefix
+   * @throws NullPointerException if {@code command} is null
+   */
+  private static boolean isTestRunnerCommand(String command)
+  {
+    String normalized = command.strip();
+    for (String prefix : TEST_RUNNER_PREFIXES)
+    {
+      // Prefix entries include a trailing delimiter (space or tab) to prevent startsWith() from
+      // false-matching unrelated commands (e.g., "bats\t" avoids matching "batsman"). The trailing
+      // delimiter also means startsWith() would miss an exact bare-command invocation like "bats"
+      // (no arguments). prefix.strip() removes the delimiter to enable exact bare-command matching.
+      if (normalized.startsWith(prefix) || normalized.equals(prefix.strip()))
+        return true;
+    }
+    return false;
+  }
+
   @Override
   public Result check(HookInput input)
   {
@@ -40,6 +91,12 @@ public Result check(HookInput input)
     if (toolResult.isEmpty())
       return Result.allow();
 
+    // Only apply failure detection to known test runner commands to avoid false positives
+    // from incidental keyword matches in diff output, file content, or unrelated command output.
+    String command = input.getCommand();
+    if (!isTestRunnerCommand(command))
+      return Result.allow();
+
     // Get exit code
     int exitCode = 0;
     JsonNode exitCodeNode = toolResult.get("exit_code");

client/src/main/java/io/github/cowwoc/cat/hooks/bash/post/DetectFailures.java

@@ -18,6 +18,7 @@
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.Optional;
 
 /**
  * Blocks Task and Skill tool calls when a pending-agent-result flag exists.
@@ -56,8 +57,9 @@ public Result check(JsonNode toolInput, String sessionId, String cwd)
     if (!Files.exists(flagPath))
       return Result.allow();
 
-    if (WorktreeContext.forSession(
-      scope.getCatWorkPath(), scope.getProjectPath(), scope.getJsonMapper(), sessionId) == null)
+    Optional<WorktreeContext> worktreeContext = WorktreeContext.forSession(
+      scope.getCatWorkPath(), scope.getProjectPath(), scope.getJsonMapper(), sessionId);
+    if (worktreeContext.isEmpty())
     {
       // No active worktree lock — flag is stale; clean up and allow
       try

client/src/main/java/io/github/cowwoc/cat/hooks/task/EnforceCollectAfterAgent.java

@@ -69,7 +69,7 @@ public Result check(JsonNode toolInput, String sessionId, String cwd)
     try
     {
       context = WorktreeContext.forSession(scope.getCatWorkPath(), scope.getProjectPath(),
-        scope.getJsonMapper(), sessionId);
+        scope.getJsonMapper(), sessionId).orElse(null);
     }
     catch (RuntimeException _)
     {