From 8eda5a679b368eee2f6c9838dc3b2f2422520ce6 Mon Sep 17 00:00:00 2001 From: vedanthvasudev Date: Wed, 22 Apr 2026 15:16:47 +0100 Subject: [PATCH] fix: apply post-v1.9.16 review findings (batch 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up pass addressing the non-blocking findings from the v1.9.16 code review. Focus is on doc/behaviour parity and covering error branches the batch-1 fix quietly relied on. Correctness — symmetric DELETE handling - GitChangeDetector.uncommittedChanges and stagedChanges now surface DiffEntry.DELETE through the old path, matching committedChanges. v1.9.16 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/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 again. Coverage for previously-untested error branches - GitChangeDetector shallow-clone MissingObjectException: exercised via a synthesised .git/refs/heads/ghost pointing at a SHA that isn't in the object store — same state JGit sees inside a shallow clone, without requiring a real shallow-clone fixture. - PathToClassMapper.isIgnored InvalidPathException: exercised with a NUL byte in the filename (the portable way to make the default FileSystem reject a path on any platform). - Malformed glob in ignorePaths: parity with the existing outOfScope*Dirs error-message regression test, locks in the key+index+pattern shape of the IllegalStateException. - All three branches already shipped correct behaviour in v1.9.16; this PR adds the coverage that should have been part of that patch. Docs - AffectedTestTask.getTransitiveDepth() now says "Default: 4" with rationale. v1.9.16 updated TransitiveStrategy and AffectedTestsExtension but missed this third Javadoc site. - OutOfScopeMatchers class Javadoc drops the '**' escapes for '**' (comment-terminator was never a risk — only '*/' is) and adds a one-liner explaining why the opening-brace escape '{' is unavoidable. - hasGlobMetachar now documents why only the opening forms '[' and '{' count — an unbalanced ']' or '}' in a literal directory name is a far more likely reality than the user meaning "glob", and promoting it to glob compilation would convert typo-in-literal into a build-breaking glob syntax error. - AffectedTestTask.JAVA_FQN documents the intentional non-validation of Java reserved words: the discovery strategies read real filenames, so an FQN shaped like a keyword only arrives adversarially and Gradle's '--tests' matcher reports "no tests found" for it, never a compile failure. --- CHANGELOG.md | 45 +++++++ .../core/git/GitChangeDetector.java | 64 +++++----- .../core/mapping/OutOfScopeMatchers.java | 24 +++- .../core/git/GitChangeDetectorTest.java | 111 ++++++++++++++++++ .../core/mapping/PathToClassMapperTest.java | 48 ++++++++ .../gradle/AffectedTestTask.java | 22 +++- 6 files changed, 272 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index df21953..c904f97 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/affected-tests-core/src/main/java/io/affectedtests/core/git/GitChangeDetector.java b/affected-tests-core/src/main/java/io/affectedtests/core/git/GitChangeDetector.java index 8170e76..1ce40ba 100644 --- a/affected-tests-core/src/main/java/io/affectedtests/core/git/GitChangeDetector.java +++ b/affected-tests-core/src/main/java/io/affectedtests/core/git/GitChangeDetector.java @@ -140,55 +140,47 @@ private Set 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 uncommittedChanges(Git git) throws GitAPIException { Set files = new LinkedHashSet<>(); - List 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 stagedChanges(Git git) throws GitAPIException { Set files = new LinkedHashSet<>(); - List 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. + * + *

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 diffs, Set 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 { diff --git a/affected-tests-core/src/main/java/io/affectedtests/core/mapping/OutOfScopeMatchers.java b/affected-tests-core/src/main/java/io/affectedtests/core/mapping/OutOfScopeMatchers.java index c595352..4133f7a 100644 --- a/affected-tests-core/src/main/java/io/affectedtests/core/mapping/OutOfScopeMatchers.java +++ b/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 @@ * *

Each raw entry is classified into one of two semantics based on * whether it contains any glob metacharacter ({@code *}, {@code ?}, - * {@code [}, {): + * {@code [}, or the opening brace — which has to be written here as + * {@code {} because Javadoc would otherwise close the + * {@code @code} tag at the very character we are trying to document): * *

    - *
  • 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 + *
  • 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.
    • *
  • 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> 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 }} 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); diff --git a/affected-tests-core/src/test/java/io/affectedtests/core/git/GitChangeDetectorTest.java b/affected-tests-core/src/test/java/io/affectedtests/core/git/GitChangeDetectorTest.java index 04cc53c..1987b33 100644 --- a/affected-tests-core/src/test/java/io/affectedtests/core/git/GitChangeDetectorTest.java +++ b/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 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 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"); diff --git a/affected-tests-core/src/test/java/io/affectedtests/core/mapping/PathToClassMapperTest.java b/affected-tests-core/src/test/java/io/affectedtests/core/mapping/PathToClassMapperTest.java index 332cd97..b5500d7 100644 --- a/affected-tests-core/src/test/java/io/affectedtests/core/mapping/PathToClassMapperTest.java +++ b/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 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 `/` diff --git a/affected-tests-gradle/src/main/java/io/affectedtests/gradle/AffectedTestTask.java b/affected-tests-gradle/src/main/java/io/affectedtests/gradle/AffectedTestTask.java index 4cedf42..77c2eb9 100644 --- a/affected-tests-gradle/src/main/java/io/affectedtests/gradle/AffectedTestTask.java +++ b/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. + * + *

    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_$]*)*");