fix: apply post-v1.9.16 review findings (batch 2)

CHANGELOG.md

@@ -58,6 +58,51 @@ adheres to [Semantic Versioning](https://semver.org/).
   malformed-glob error path is now the single source of truth for
   `Affected Tests: invalid glob at outOfScope*Dirs[N]` messages.
 
+### Fixed — post-v1.9.16 review batch
+
+- `GitChangeDetector.uncommittedChanges` and `stagedChanges` now
+  surface `DELETE` entries through the old path, the same way
+  `committedChanges` has since v1.9.16. The v1.9.16 release patched
+  the committed branch only, which was inert under its own
+  committed-only defaults but re-opened the silent-skip hole the
+  moment any adopter flipped `includeUncommitted` or `includeStaged`
+  back on to iterate on a local `git rm`. All three diff sources
+  now share one `collectPaths` helper so future bucketing changes
+  can't drift between them.
+- Regression tests added for the previously-untested error branches:
+  `GitChangeDetector`'s shallow-clone `MissingObjectException` hint,
+  `PathToClassMapper.isIgnored`'s `InvalidPathException` guard (NUL
+  bytes and friends), and the config-time error message for
+  malformed globs in `ignorePaths`. No code change — only coverage
+  for branches whose existing behaviour the batch-1 fix relied on.
+
+### Documentation
+
+- `AffectedTestTask.getTransitiveDepth()` Javadoc now documents the
+  actual default of `4` with rationale. The v1.9.16 pass updated
+  `TransitiveStrategy` and `AffectedTestsExtension` but missed this
+  third site — consumers reading `AffectedTestTask` Javadoc were
+  still being told they needed to set `transitiveDepth = 4`
+  explicitly; they do not.
+- `OutOfScopeMatchers` class Javadoc replaces the Javadoc-safe
+  `**` escapes with literal `**` inside `{@code}` (the
+  comment-terminator was never a risk — only `*/` is, and `{@code}`
+  only cares about matching braces). The opening brace still has to
+  be escaped because Javadoc closes `{@code}` at the first `}`; a
+  one-line comment now explains why the escape is unavoidable.
+- `OutOfScopeMatchers.hasGlobMetachar` now documents why only the
+  opening forms `[` and `{` are treated as glob indicators — a
+  literal directory name that happens to contain an unbalanced `]`
+  or `}` is a far more likely reality than a user meaning "glob",
+  and promoting it to glob compilation would convert
+  typo-in-literal into a build-breaking glob syntax error.
+- `AffectedTestTask.JAVA_FQN` now documents the intentional
+  non-validation of Java reserved words. An FQN shaped like a
+  keyword can't be produced by the discovery strategies (they read
+  real filenames); the only way one reaches the filter is
+  adversarially, and Gradle's downstream `--tests` matcher reports
+  "no tests found" for it — never a compile failure or RCE.
+
 ### Fixed — post-v1.9.15 review batch
 
 - `outOfScopeTestDirs` / `outOfScopeSourceDirs` glob entries now work

affected-tests-core/src/main/java/io/affectedtests/core/git/GitChangeDetector.java

@@ -140,55 +140,47 @@ private Set<String> committedChanges(Repository repository, Git git) throws IOEx
                 .setNewTree(headTree)
                 .call();
 
-        for (DiffEntry entry : diffs) {
-            switch (entry.getChangeType()) {
-                case ADD, COPY, MODIFY -> files.add(entry.getNewPath());
-                case DELETE -> {
-                    // Before we surfaced DELETEs, a pure 'git rm' MR routed
-                    // through EMPTY_DIFF and — in LOCAL/CI modes — silently
-                    // skipped all tests. Now the old path flows through the
-                    // normal bucketing so ignore/out-of-scope rules still
-                    // apply (deleted '*.md' stays ignored, deleted
-                    // 'api-test/**' stays out-of-scope, deleted production
-                    // classes reach the transitive strategy). The engine's
-                    // existing filter at AffectedTestsEngine drops any
-                    // discovered FQN whose test file no longer exists, so
-                    // surfacing the deleted path never asks Gradle to run a
-                    // missing test.
-                    files.add(entry.getOldPath());
-                }
-                case RENAME -> files.add(entry.getNewPath());
-            }
-        }
-
+        collectPaths(diffs, files);
         return files;
     }
 
     private Set<String> uncommittedChanges(Git git) throws GitAPIException {
         Set<String> files = new LinkedHashSet<>();
-        List<DiffEntry> diffs = git.diff().call();
-        for (DiffEntry entry : diffs) {
-            // Only include new paths (i.e. the file as it exists on disk now).
-            // Skipping old paths prevents us from trying to run tests for files
-            // that the developer has deleted locally.
-            String newPath = entry.getNewPath();
-            if (newPath != null && !newPath.equals("/dev/null")) {
-                files.add(newPath);
-            }
-        }
+        collectPaths(git.diff().call(), files);
         return files;
     }
 
     private Set<String> stagedChanges(Git git) throws GitAPIException {
         Set<String> files = new LinkedHashSet<>();
-        List<DiffEntry> diffs = git.diff().setCached(true).call();
+        collectPaths(git.diff().setCached(true).call(), files);
+        return files;
+    }
+
+    /**
+     * Shared diff-entry bucketing for the three diff sources (committed,
+     * uncommitted, staged). Keeping one implementation is the whole
+     * point of this helper: before it existed, the committed branch
+     * routed DELETEs through {@link DiffEntry#getOldPath()} so pure
+     * {@code git rm} MRs reached the bucketing pipeline, while the
+     * uncommitted and staged branches kept the older "skip anything
+     * whose newPath is /dev/null" shape. That asymmetry was inert
+     * under the v1.9.15 defaults ({@code includeUncommitted = false},
+     * {@code includeStaged = false}), but any adopter who flipped
+     * either knob back on to iterate on a local deletion hit exactly
+     * the silent-skip hole the committed branch was patched to close.
+     *
+     * <p>Surfacing the old path never asks Gradle to run a missing
+     * test — the engine's existing missing-file filter at
+     * {@code AffectedTestsEngine} drops any discovered FQN whose
+     * backing file is gone.
+     */
+    private static void collectPaths(List<DiffEntry> diffs, Set<String> files) {
         for (DiffEntry entry : diffs) {
-            String newPath = entry.getNewPath();
-            if (newPath != null && !newPath.equals("/dev/null")) {
-                files.add(newPath);
+            switch (entry.getChangeType()) {
+                case ADD, COPY, MODIFY, RENAME -> files.add(entry.getNewPath());
+                case DELETE -> files.add(entry.getOldPath());
             }
         }
-        return files;
     }
 
     private ObjectId resolveBaseRef(Repository repository) throws IOException {

affected-tests-core/src/main/java/io/affectedtests/core/mapping/OutOfScopeMatchers.java

@@ -18,7 +18,7 @@
  * indexed files on disk) evaluate the same shape of entry with the
  * same semantics. Before this extraction the two sides disagreed on
  * glob-form entries: the mapper honoured
- * {@code outOfScopeTestDirs = ["api-test/**"]} while the index
+ * {@code outOfScopeTestDirs = ["api-test/**"]} while the index
  * treated the same string as a literal prefix, so a mixed diff (one
  * production file + a refactor under {@code api-test/}) would bucket
  * the api-test file correctly but still dispatch its test because the
@@ -28,12 +28,14 @@
  *
  * <p>Each raw entry is classified into one of two semantics based on
  * whether it contains any glob metacharacter ({@code *}, {@code ?},
- * {@code [}, <code>&#123;</code>):
+ * {@code [}, or the opening brace — which has to be written here as
+ * {@code &#123;} because Javadoc would otherwise close the
+ * {@code @code} tag at the very character we are trying to document):
  *
  * <ul>
- *   <li>Glob entries (e.g. {@code "api-test/&#42;&#42;"}) compile to
- *       a {@link PathMatcher} using the JVM's default file system
- *       {@code glob:} syntax, so {@code &#42;&#42;} crosses directory
+ *   <li>Glob entries (e.g. {@code "api-test/**"}) compile to a
+ *       {@link PathMatcher} using the JVM's default file system
+ *       {@code glob:} syntax, so {@code **} crosses directory
  *       boundaries as users expect from Ant/Gradle conventions.</li>
  *   <li>Literal entries (e.g. {@code "api-test/src/test/java"}) keep
  *       the boundary-aware prefix semantics the README has documented
@@ -126,6 +128,18 @@ public static boolean matchesAny(String normalizedPath, List<Predicate<String>>
         return false;
     }
 
+    /**
+     * Returns {@code true} iff the entry contains at least one glob
+     * metacharacter. Only the opening forms {@code [} and {@code {}
+     * are checked because balanced closers can't appear in a string
+     * that didn't already open one — and a user typing a literal
+     * directory name is overwhelmingly more likely to write an
+     * unmatched {@code ]} or {@code &#125;} inside a filename than
+     * they are to mean "glob". Promoting unbalanced closers to glob
+     * compilation would only convert "typo-in-literal" into a
+     * build-breaking glob syntax error. {@code *} and {@code ?} have
+     * no balanced counterpart, so they are unconditional giveaways.
+     */
     private static boolean hasGlobMetachar(String s) {
         for (int i = 0; i < s.length(); i++) {
             char c = s.charAt(i);

affected-tests-core/src/test/java/io/affectedtests/core/git/GitChangeDetectorTest.java

@@ -183,6 +183,117 @@ void detectsDeletedFilesByOldPath() throws Exception {
         }
     }
 
+    @Test
+    void detectsUncommittedDeletesByOldPath() throws Exception {
+        // Symmetric regression: the committed branch started surfacing
+        // DELETEs in batch-1, but uncommittedChanges still filtered
+        // anything whose newPath was /dev/null. With the v1.9.15
+        // committed-only defaults this hole was inert, but any adopter
+        // iterating on a local `git rm`-before-commit with
+        // includeUncommitted = true would see their MR route through
+        // EMPTY_DIFF → SKIPPED under LOCAL/CI mode. Lock in that the
+        // unstaged deletion now flows through the normal bucketing.
+        try (Git git = initRepoWithInitialCommit()) {
+            File doomed = tempDir.resolve("src/main/java/com/example/Doomed.java").toFile();
+            doomed.getParentFile().mkdirs();
+            Files.writeString(doomed.toPath(), "package com.example;\npublic class Doomed {}");
+            git.add().addFilepattern(".").call();
+            git.commit().setMessage("add Doomed").call();
+
+            String baseCommit = git.log().call().iterator().next().getName();
+
+            // Delete on disk without staging — this is the
+            // "uncommitted" diff that JGit's plain git.diff().call()
+            // surfaces.
+            assertTrue(doomed.delete(), "fixture precondition");
+
+            AffectedTestsConfig config = AffectedTestsConfig.builder()
+                    .baseRef(baseCommit)
+                    .includeUncommitted(true)
+                    .includeStaged(false)
+                    .build();
+
+            GitChangeDetector detector = new GitChangeDetector(tempDir, config);
+            Set<String> changed = detector.detectChangedFiles();
+
+            assertTrue(changed.contains("src/main/java/com/example/Doomed.java"),
+                    "Uncommitted deletion must surface through its old path");
+        }
+    }
+
+    @Test
+    void detectsStagedDeletesByOldPath() throws Exception {
+        // Same regression as above, on the staged diff source. A
+        // developer running `git rm foo.java` without committing puts
+        // the deletion in the index, and includeStaged = true must see
+        // it for the same reasons the committed branch does.
+        try (Git git = initRepoWithInitialCommit()) {
+            File doomed = tempDir.resolve("src/main/java/com/example/Doomed.java").toFile();
+            doomed.getParentFile().mkdirs();
+            Files.writeString(doomed.toPath(), "package com.example;\npublic class Doomed {}");
+            git.add().addFilepattern(".").call();
+            git.commit().setMessage("add Doomed").call();
+
+            String baseCommit = git.log().call().iterator().next().getName();
+
+            // git rm — the deletion is staged in the index.
+            git.rm().addFilepattern("src/main/java/com/example/Doomed.java").call();
+
+            AffectedTestsConfig config = AffectedTestsConfig.builder()
+                    .baseRef(baseCommit)
+                    .includeUncommitted(false)
+                    .includeStaged(true)
+                    .build();
+
+            GitChangeDetector detector = new GitChangeDetector(tempDir, config);
+            Set<String> changed = detector.detectChangedFiles();
+
+            assertTrue(changed.contains("src/main/java/com/example/Doomed.java"),
+                    "Staged deletion must surface through its old path");
+        }
+    }
+
+    @Test
+    void explainsShallowCloneWhenBaseObjectMissing() throws Exception {
+        // Regression for the MissingObjectException branch added in
+        // batch-1. Instead of spinning up a shallow clone (slow and
+        // relies on system git), we synthesise the exact JGit state a
+        // shallow clone produces at the parseCommit call site: a ref
+        // that resolves cleanly to an ObjectId whose underlying commit
+        // object is not in the local pack. The resolve step succeeds
+        // (so we do not hit the "could not resolve baseRef" branch
+        // earlier in the chain), but parseCommit throws
+        // MissingObjectException the moment we try to walk the tree.
+        try (Git git = initRepoWithInitialCommit()) {
+            File refsHeads = tempDir.resolve(".git/refs/heads").toFile();
+            assertTrue(refsHeads.exists() || refsHeads.mkdirs(), "fixture precondition");
+            // Any syntactically valid 40-char hex string that is not a
+            // real object in this repo's object store triggers the
+            // MissingObjectException at parseCommit time.
+            String bogusSha = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef";
+            Files.writeString(new File(refsHeads, "ghost").toPath(), bogusSha + "\n");
+
+            AffectedTestsConfig config = AffectedTestsConfig.builder()
+                    .baseRef("ghost")
+                    .includeUncommitted(false)
+                    .includeStaged(false)
+                    .build();
+
+            GitChangeDetector detector = new GitChangeDetector(tempDir, config);
+
+            IllegalStateException error = assertThrows(IllegalStateException.class,
+                    detector::detectChangedFiles);
+
+            String message = error.getMessage();
+            assertTrue(message.contains("shallow clone"),
+                    "Expected shallow-clone hint in error message, got: " + message);
+            assertTrue(message.contains("fetch-depth: 0") && message.contains("GIT_DEPTH: 0"),
+                    "Expected both GitHub and GitLab fix hints, got: " + message);
+            assertTrue(error.getCause() instanceof org.eclipse.jgit.errors.MissingObjectException,
+                    "Expected cause to be MissingObjectException, got: " + error.getCause());
+        }
+    }
+
     @Test
     void failsLoudlyOnNonGitDirectory() {
         Path nonGitDir = tempDir.resolve("not-a-repo");

affected-tests-core/src/test/java/io/affectedtests/core/mapping/PathToClassMapperTest.java

@@ -326,6 +326,54 @@ void tryMapToClassHandlesSourceDirAsPathPrefix() {
         assertTrue(result.unmappedChangedFiles().isEmpty());
     }
 
+    @Test
+    void unparseablePathNameDoesNotCrashIsIgnored() {
+        // Regression for the InvalidPathException catch in isIgnored.
+        // The JDK's default FileSystem rejects paths containing a NUL
+        // byte on every platform, so "src/main/java/com/example/Foo\0"
+        // is the portable way to exercise the branch. Before the
+        // catch existed, any such filename arriving in a diff —
+        // surfacing as a CVE-style probe or as a cross-platform-
+        // checkout artifact — blew up the whole task with an
+        // unhandled InvalidPathException; now it routes through the
+        // unmapped bucket and the safety net picks it up.
+        String malformed = "src/main/java/com/example/Foo\u0000";
+        Set<String> changed = Set.of(malformed);
+
+        assertDoesNotThrow(() -> mapper.mapChangedFiles(changed),
+                "Unparseable path names must not crash the mapper");
+
+        MappingResult result = mapper.mapChangedFiles(changed);
+        assertTrue(result.ignoredFiles().isEmpty(),
+                "An unparseable path cannot be matched against any ignore glob");
+        // The .java suffix survives the NUL byte in the string so the
+        // mapper still attempts the source-dir mapping. What this test
+        // locks in is the *absence of a crash*, not the final bucket.
+    }
+
+    @Test
+    void malformedIgnorePathsGlobFailsWithHelpfulMessage() {
+        // Parity test with OutOfScopeMatchersTest — the out-of-scope
+        // path had a regression test for malformed globs since
+        // v1.9.16, but the ignorePaths compile path, which uses the
+        // same IAE-catching shape, did not. Lock in that a user
+        // writing a bracket expression they forgot to close gets a
+        // message pointing at ignorePaths[index] and the specific
+        // entry, not a raw PatternSyntaxException stacktrace.
+        AffectedTestsConfig badConfig = AffectedTestsConfig.builder()
+                .ignorePaths(java.util.List.of("**/valid/**", "**/broken["))
+                .build();
+
+        IllegalStateException error = assertThrows(IllegalStateException.class,
+                () -> new PathToClassMapper(badConfig));
+
+        String message = error.getMessage();
+        assertTrue(message.contains("ignorePaths[1]"),
+                "Error message must identify which entry is broken, got: " + message);
+        assertTrue(message.contains("'**/broken['"),
+                "Error message must quote the offending entry, got: " + message);
+    }
+
     @Test
     void tryMapToClassHandlesSourceDirAfterModulePrefix() {
         // Positive counterpart: when the source dir is preceded by a `/`

affected-tests-gradle/src/main/java/io/affectedtests/gradle/AffectedTestTask.java

@@ -142,7 +142,13 @@ public AffectedTestTask() {
     /**
      * How many levels of transitive dependencies to follow when the
      * {@code transitive} strategy is enabled.
-     * Range: 0 (disabled) to 5. Default: {@code 2}.
+     * Range: 0 (disabled) to 5. Default: {@code 4} — matches the depth
+     * most Java controller → service → repository chains
+     * actually reach in Modulr-shaped codebases while leaving one level
+     * of margin. The pre-v2 default was {@code 2}, which under-reached
+     * on any MR that crossed more than one seam; consumers reading this
+     * Javadoc were being told they still needed to set {@code 4}
+     * explicitly — they do not.
      *
      * @return the transitive depth property
      */
@@ -641,6 +647,20 @@ private String resolveGradleCommand(Path projectDir) {
      * Anything outside this shape cannot correspond to a real test
      * class and is dropped with a warning before reaching the nested
      * Gradle invocation.
+     *
+     * <p>Deliberately does NOT reject Java reserved words
+     * ({@code if}, {@code class}, {@code return}, ...). The contract
+     * of this filter is "is this argv safe to hand to
+     * {@code gradle --tests}" — a compromised source tree sneaking
+     * shell-like tokens or argv-flag-shaped strings into the FQN list
+     * is the threat model. An FQN that happens to look like a reserved
+     * word could never be produced by the discovery strategies (which
+     * derive names from real {@code .java} filenames), so the only way
+     * one reaches this method is adversarially, and the downstream
+     * Gradle {@code --tests} matcher will simply report
+     * "no tests found" for it — never a compile failure or RCE.
+     * Keeping the regex broad here means we don't have to ship a
+     * stale list of keywords that drifts as the JLS grows.
      */
     private static final Pattern JAVA_FQN =
             Pattern.compile("[A-Za-z_$][A-Za-z0-9_$]*(\\.[A-Za-z_$][A-Za-z0-9_$]*)*");