feat: add purge-check goal to detect and remove orphaned build directories by gnodet · Pull Request #356 · apache/maven-clean-plugin

gnodet · 2026-09-26T17:06:34Z

Summary

Adds a new purge-check goal to detect and remove orphaned build output directories left behind by sub-projects that have been removed from the reactor.

Problem

When a sub-project is removed from the reactor (e.g. after git pull or a branch switch), its target/ directory may remain on disk. Since the sub-project is no longer in the reactor, mvn clean cannot know about it and skips it. This causes issues with plugins that scan the project tree recursively — notably RAT which finds unlicensed build artifacts and fails the build.

See #315, apache/maven-resolver#1943, apache/maven#11800.

Solution

New goal clean:purge-check with heuristic:

A direct child directory of basedir is considered orphaned when its only non-hidden child is the build output directory (typically target/). A live sub-project always has at least a pom.xml alongside its build directory.

This avoids false positives while covering the exact scenario of a removed sub-project (which leaves only a target/ behind).

The goal uses NIO2 (DirectoryStream, Files.isDirectory), respects ${project.build.directory} for the build dir name, and reuses the existing Cleaner for deletion.

Lifecycle integration

This PR only adds the goal. The binding to the initialize phase of the default lifecycle will be done separately in Maven core 4.1.0, so it runs automatically on every build (not just mvn clean), preventing stale artifacts from interfering with RAT, Checkstyle, etc.

Users on earlier Maven versions can bind it manually:

<execution>
  <id>purge-check</id>
  <phase>initialize</phase>
  <goals><goal>purge-check</goal></goals>
</execution>

Changes

CleanOrphansMojo — new mojo, NIO2, reuses Cleaner
src/it/purge-check-orphan/ — IT: creates an orphaned sub-orphan/target/ via setup.groovy, verifies it is removed while sub-existing/target/ (which has siblings) is left intact

Fixes #315

gnodet-bot

Solid implementation. The NIO2 usage is correct (DirectoryStreams properly closed via try-with-resources), the orphan detection heuristic is conservative and well-documented, and the Cleaner API is used consistently with CleanMojo. The IT coverage is good.

Two low-severity observations below.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot · 2026-09-26T17:13:45Z

+     *
+     * @param  basedir      the directory to scan
+     * @param  buildDirName the name of the build output directory (e.g. {@code target})
+     * @return              a possibly-empty list of build directories to delete


💭 Design observation: After cleaner.delete(buildDir), the parent directory (e.g. sub-orphan/) is left empty on disk — the mojo identifies it as orphaned but only removes its build output. This is fine for the stated goal (preventing RAT from finding stale build artifacts), but if a full cleanup is desired, the parent directory could also be removed after the build dir deletion, since the mojo already confirmed its only visible content was target/. Not a bug, just a question of scope.

gnodet-bot

Re-review after 9f617c6 — the new commit correctly restores the missing closing brace and Javadoc opener in CleanerTest.java. Syntax fix is accurate.

Two observations from the previous review remain outstanding (both non-blocking):

verify.groovy:32-34 — the triple-OR assertion is logically correct but hard to reason about; the simpler !log.readLines().any { ... } alternative was suggested
CleanOrphansMojo.java:171 — after cleaner.delete(buildDir), the parent directory (e.g. sub-orphan/) is left empty on disk; whether to also remove it is a scope decision

Neither blocks the PR.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot · 2026-09-26T17:16:33Z

        // The original file should still exist because it could not be deleted.
        assertTrue(exists(file), "File should still exist when batch retry also fails");
+    }



✅ Confirmed: Correct fix — the missing } closing batchRetryLogsWarningAfterRetryWhenStillFailing() and the missing /** Javadoc opener for deleteWithFollowSymlinksDeletesTarget() are both restored. Without this, the file wouldn't compile.

gnodet-bot

Two observations — one dead code, one confirmation of gnodet-bot's readability finding.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot · 2026-09-26T17:18:20Z

+     * The current session.
+     */
+    @Inject
+    private Session session;


⚠️ Dead code: Session is injected but never referenced anywhere in this class. In CleanMojo, it's used for the fast deletion feature (background cleaner). Since CleanOrphansMojo doesn't support fast deletion, both the field and its import can be removed.

Suggested change

* The current session.

*/

@Inject

private Session session;

Fixed in 70233df — removed both the @Inject Session session field and its import.

gnodet-bot · 2026-09-26T17:18:21Z

+assert !log.contains('sub-existing') || log.contains('Removing orphaned') == false \
+    || !log.find('Removing orphaned.*sub-existing') \
+    : "sub-existing/target/ must not be treated as orphaned"


Confirming gnodet-bot's readability finding — and adding that the logic is also partially dead. Given that line 36 requires log.contains('Removing orphaned build directory'), condition B (log.contains('Removing orphaned') == false) can never be true at this point. Condition A (!log.contains('sub-existing')) is also redundant — if sub-existing doesn't appear in the log at all, condition C already covers it (no match for 'Removing orphaned.*sub-existing').

The only condition that matters is C. Suggested simplification:

Suggested change

assert !log.contains('sub-existing') || log.contains('Removing orphaned') == false \

|| !log.find('Removing orphaned.*sub-existing') \

: "sub-existing/target/ must not be treated as orphaned"

assert !log.find('Removing orphaned.*sub-existing') \

: "sub-existing/target/ must not be treated as orphaned"

…ories When a sub-project is removed from the reactor (e.g. after a git pull or a branch switch), its target/ directory may remain on disk. Since the sub-project is no longer part of the reactor, mvn clean cannot know about it and skips it. This causes issues with plugins that scan the project tree recursively (RAT, Checkstyle, etc.) which find unlicensed or stale build artifacts. The new purge-check goal detects such orphaned directories using a simple heuristic: a direct child directory of the current basedir is considered orphaned when its only non-hidden child is the build output directory (typically target/). A live sub-project always has at least a pom.xml alongside its build directory. The goal is designed to be bound to the initialize phase of the default lifecycle by Maven core, so it runs automatically on every build - not just mvn clean - preventing stale artifacts from interfering with plugins like RAT. Fixes apache#315

Replace the two-pass approach (one DirectoryStream on basedir, one on each child via isOnlyNonHiddenChild) with a single stream per child via orphanBuildDir(). The method now returns the Path directly instead of a boolean, eliminating the redundant child.resolve() and Files.isDirectory() pre-check.

gnodet-bot

Re-review after 59acbe7 — the refactor to a single DirectoryStream per candidate directory (orphanBuildDir()) is clean and correct: resource handling is proper (try-with-resources), early return on second visible entry is efficient, and the edge cases (empty dir, non-directory sole entry, wrong name) are all handled.

Two findings from previous reviews remain outstanding:

Session injection (line 80) — still injected, still unused
verify.groovy assertion (lines 32-34) — still contains the triple-OR with dead logic

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot · 2026-09-26T17:32:08Z

+    /**
+     * The current session.
+     */
+    @Inject
+    private Session session;


⚠️ Still outstanding from previous review: Session is injected but never referenced anywhere in this class. The @Inject field and the import org.apache.maven.api.Session can both be removed.

Suggested change

/**

* The current session.

*/

@Inject

private Session session;

Fixed in 70233df.

gnodet-bot · 2026-09-26T17:32:08Z

+assert !log.contains('sub-existing') || log.contains('Removing orphaned') == false \
+    || !log.find('Removing orphaned.*sub-existing') \
+    : "sub-existing/target/ must not be treated as orphaned"


💡 Still outstanding from previous review: This triple-OR contains dead logic — given that line 36 requires log.contains('Removing orphaned build directory'), condition B (log.contains('Removing orphaned') == false) can never be true at that point. Condition A is also redundant. Only condition C matters:

Suggested change

assert !log.contains('sub-existing') || log.contains('Removing orphaned') == false \

|| !log.find('Removing orphaned.*sub-existing') \

: "sub-existing/target/ must not be treated as orphaned"

assert !log.find('Removing orphaned.*sub-existing') \

: "sub-existing/target/ must not be treated as orphaned"

…uildDir

…irectory When a child directory's only visible content is target/, the directory itself has no reason to remain on disk — there are no source files left. Removing child/ entirely is cleaner than leaving an empty shell behind. The temporary-removal case is not a concern: a module commented out in the parent pom still has its pom.xml alongside target/, so the heuristic correctly leaves it alone.

gnodet-bot

Re-review after 90e0375 — the two new commits correctly address the previous findings:

verify.groovy assertion — ✅ Addressed. Replaced the triple-OR with the simplified !log.find('Removing orphaned.*sub-existing') as suggested.
Parent directory left on disk — ✅ Addressed. The mojo now removes the entire orphaned parent directory (not just its target/ subdirectory). isOrphanDirectory() returns boolean, findOrphanBuildDirectories() collects the parent path, IT verifies !orphanDir.exists() on the whole parent. Clean refactor.

One finding from previous reviews remains outstanding:

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot · 2026-09-26T17:54:10Z

+     * The current session.
+     */
+    @Inject
+    private Session session;


⚠️ Still outstanding from previous reviews: Session is injected but never referenced anywhere in this class. The @Inject field and the import org.apache.maven.api.Session (line 29) are both dead code. In CleanMojo, Session is used for the background cleaner feature — CleanOrphansMojo doesn't support fast/background deletion, so this field has no purpose.

Suggested change

private Session session;

Fixed in 70233df.

gnodet-bot

Re-review after 70233df — all three previous findings are now addressed:

verify.groovy assertion — ✅ Simplified to !log.find('Removing orphaned.*sub-existing')
Parent directory cleanup — ✅ Mojo now removes the entire orphaned directory, not just target/
Unused Session injection — ✅ Removed in 70233df (both field and import)

No new issues in the incremental diff. Code is solid.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot

Re-review after 70233df — the final outstanding finding is now addressed:

Session injection (dead code) — ✅ Addressed. Both the @Inject private Session session field and its import are removed. This was the last remaining item from previous reviews.

All findings from previous review cycles are now resolved. Code is clean — NIO2 resource handling is correct, orphan detection heuristic is conservative and well-tested, IT coverage validates both the positive case (orphan removed) and the negative case (active sub-project preserved).

This review was generated by an AI agent, Hermès on behalf of @gnodet.

elharo

The strategy for identifying orphaned files seems sound. I'm not convinced a new goal is needed. This seems like something the existing clean goal should just do.

gnodet · 2026-09-26T21:04:59Z

Thank you for the feedback! I'd like to clarify the use case a bit, because the problem this goal addresses isn't really about the clean goal itself.

The issue (#315) describes a situation where a sub-module is removed or renamed from a multi-module build. After that, the old target/ directory stays on disk indefinitely — even if you run mvn clean, because clean only cleans modules that are still in the reactor. The orphaned target/ directory is effectively invisible to Maven.

The typical scenario is: git pull --rebase && mvn install. A colleague removed or renamed a sub-module upstream; after the rebase the directory is gone from the POM but the target/ is still there on disk. The next build happily picks up stale classes, reports, or generated files from it.

This causes real problems with plugins that scan the project tree by filesystem path rather than by reactor membership:

Apache RAT recursively scans target/ and fails on binary/generated files
Checkstyle, SpotBugs, maven-site-plugin can pick up stale classes or reports

The purge-check goal addresses this by running in the initialize phase — before any of those plugins — and removing target/ directories whose parent directory no longer has a pom.xml. Binding to initialize is important because clean is a separate lifecycle: when a user runs mvn install or mvn verify, the clean lifecycle is not invoked at all, so there is no opportunity for the clean goal to remove orphaned directories before RAT or Checkstyle get to them.

The heuristic is intentionally conservative: a directory is flagged as orphaned only if target/ is its sole visible child. A directory that still contains a pom.xml — whether it was renamed or is simply a clean module — is never touched.

One alternative we considered was hardcoding this behavior directly in maven-core — which was actually the original idea. It would be slightly faster (no plugin classloading overhead) and always present without any binding. However, embedding file deletion logic in maven-core makes it harder to discover, configure, or disable on a per-project basis. Keeping it in maven-clean-plugin makes the behavior visible in mvn help:effective-pom and the build plan, and lets projects opt out with -Dmaven.clean.purgeCheck.skip=true or override parameters the usual way.

The intent is to bind this goal into both the default and clean lifecycles in Maven 4.1.0 (apache/maven#11800), so that orphan cleanup becomes automatic for all users without any plugin configuration — similar to how maven-clean-plugin:clean is already bound to the clean phase today. This PR is the first step toward that.

Does that help clarify the rationale? Happy to adjust the implementation or the naming if you have suggestions.

desruisseaux · 2026-09-27T09:43:46Z

+    /**
+     * Whether to force the deletion of read-only files inside orphaned build directories.
+     *
+     * @since 3.5.1


This MOJO did not existed in 3.5.1. Therefore, it seems to me that this MOJO should not contain any @since tag (except for the class).

Fixed in a5b5e68 — removed all parameter-level @since tags and updated the class-level tag to @since 4.0.0-beta-3.

desruisseaux · 2026-09-27T09:45:39Z

+ * (not just its build output subdirectory) is removed, since an orphaned directory with no
+ * source files has no reason to remain on disk.</p>
+ *
+ * @since 3.5.1


Should it be @since 4.0.0-beta-3?

Yes, fixed in a5b5e68.

@SInCE

…sion The Mojo is new in 4.0.0-beta-3, not 3.5.1. Parameter-level @SInCE tags are redundant when all parameters were introduced with the class itself.

gnodet-bot

Re-review after a5b5e68 — the new commit correctly addresses @desruisseaux's feedback:

Parameter-level @since 3.5.1 tags — ✅ Removed. This mojo is new and never existed in 3.5.1, so parameter-level @since tags were incorrect.
Class-level @since — ✅ Updated from 3.5.1 to 4.0.0-beta-3, matching the target milestone.

No logic changes, Javadoc-only fix. All previous findings remain addressed.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

desruisseaux · 2026-09-27T09:56:01Z

+     */
+    private boolean isOrphanDirectory(Path child, String buildDirName) throws IOException {
+        Path sole = null;
+        try (DirectoryStream<Path> entries = Files.newDirectoryStream(child, this::isVisible)) {


Use !Files.isHidden(Path) instead.

Fixed in 221f14a — replaced with !Files.isHidden(p).

desruisseaux · 2026-09-27T09:56:49Z

+     * Returns {@code true} if the path is not a hidden file (i.e. its name does not start with a
+     * dot). Symbolic links are not followed — if the link itself is hidden, it is excluded.
+     */
+    private boolean isVisible(Path path) {


Delete after the replacement by Files.isHidden(Path). Side note: this method should have been static.

Fixed in 221f14a — isVisible() method removed.

@desruisseaux

Address review feedback from @desruisseaux: replace the custom isVisible() method (dot-prefix check) with the standard Files.isHidden() API, which is platform-aware. Remove the now unused isVisible() method.

gnodet-bot

Re-review after CI_GREEN on a5b5e68 — CI passes. All previous bot findings remain addressed.

Two new inline comments from @desruisseaux (posted after the last bot APPROVE) are outstanding:

isVisible() → Files.isHidden() — the custom isVisible(Path) method (line 208) checks !name.startsWith("."), but Files.isHidden(Path) is the proper NIO2 API. It does the same thing on Unix, and additionally handles the Windows hidden-attribute — more portable.
Remove the isVisible() method after switching to Files.isHidden().

Both are valid suggestions. Needs one more commit before merge.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot · 2026-09-27T09:59:52Z

+     * dot). Symbolic links are not followed — if the link itself is hidden, it is excluded.
+     */
+    private boolean isVisible(Path path) {
+        String name = path.getFileName().toString();
+        return !name.startsWith(".");
+    }
+
+    /**


🗑️ Remove after switching to Files.isHidden() — this method becomes dead code once the DirectoryStream filter uses Files.isHidden() directly.

Suggested change

* dot). Symbolic links are not followed — if the link itself is hidden, it is excluded.

*/

private boolean isVisible(Path path) {

String name = path.getFileName().toString();

return !name.startsWith(".");

}

/**

gnodet-bot

Re-review after 221f14a — the last outstanding committer feedback is now addressed:

isVisible() → Files.isHidden() — ✅ Addressed. The DirectoryStream filter now uses p -> !Files.isHidden(p) instead of the custom isVisible() method. More portable (handles Windows hidden attribute in addition to Unix dot-prefix).
Dead isVisible() method — ✅ Removed.

All findings from all review cycles are resolved. Code is solid.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot

Re-review after e92d1eb — the new commit addresses @desruisseaux's suggestion to test Files.isDirectory last: the filename string comparison (sole.getFileName().toString().equals(buildDirName)) now precedes the filesystem call, avoiding an unnecessary I/O operation when the name doesn't match. Logically equivalent, strictly better.

All findings from all previous review cycles remain addressed. Code is solid.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot

Re-review after b6b800b — Spotless formatting only: the return statement in isOrphanDirectory() was collapsed from 3 lines to 1. No logic change. All previous findings remain addressed.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

rmannibucau

Ok for me as a manual option but ultimately for maven 4.0.0 it must be automatic and in core since core owns that fake repo and reactor thing, not plugins IMHO

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved symlink handling, scan-error behavior, test coverage, and documentation updates block approval.

Review effort: Lite
Findings: 1 · 1

Open (2)

Exclude symbolic-link children from orphan scanning · New
Continue scanning after per-child inspection failures · New

What changed in this PR

Adds clean:purge-check to detect and remove orphaned sub-project build directories using NIO2 and the existing Cleaner.

Changes:

Implements orphan detection and cleanup.
Adds Maven 4 integration-test fixtures and verification.
Configures Invoker execution for the integration test.

File	Summary
`src/main/java/org/apache/maven/plugins/clean/CleanOrphansMojo.java`	Implements the new purge goal; symlink handling and per-child inspection errors require changes.
`src/it/purge-check-orphan/verify.groovy`	Verifies purge behavior; should directly assert preservation of the active fixture.
`src/it/purge-check-orphan/sub-existing/pom.xml`	Defines the active sub-project fixture.
`src/it/purge-check-orphan/setup.groovy`	Creates orphan and active-project test fixtures.
`src/it/purge-check-orphan/pom.xml`	Configures the integration-test project and goal execution.
`src/it/purge-check-orphan/invoker.properties`	Restricts execution to Maven 4.

The user-facing goal documentation also requires updating before publication.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

gnodet · 2026-09-27T16:48:54Z

+        if (!Files.isDirectory(basedir)) {
+            return result;
+        }
+        try (DirectoryStream<Path> children = Files.newDirectoryStream(basedir, Files::isDirectory)) {


Fixed in 2cc8c28 — Files.isDirectory() now uses LinkOption.NOFOLLOW_LINKS in both the directory stream filter and the final orphan check, so symlinks are excluded from scanning.

gnodet · 2026-09-27T16:48:55Z

+        try (DirectoryStream<Path> children = Files.newDirectoryStream(basedir, Files::isDirectory)) {
+            for (Path child : children) {
+                if (isOrphanDirectory(child, buildDirName)) {
+                    result.add(child);
+                }
+            }
+        } catch (IOException e) {
+            logger.warn("Could not scan " + basedir + " for orphaned build directories: " + e.getMessage());
+        }
+        return result;


Fixed in 2cc8c28 — IOException from isOrphanDirectory() is now caught per-child inside the loop, so an unreadable sibling no longer aborts the scan of remaining children.

gnodet-bot

Re-review — assessing new Copilot findings on b6b800b

Two new findings from Copilot arrived after the last bot APPROVE. Both are valid defensive improvements, neither is blocking:

Symlink handling (line 164) — Confirmed valid. Files.newDirectoryStream(basedir, Files::isDirectory) follows symlinks by default, so a symlink pointing to a directory whose only visible child is target/ would be falsely treated as orphaned. Fix: add && !Files.isSymbolicLink(child) to the filter or as a guard in isOrphanDirectory(). In practice, symlinks as Maven sub-module directories are extremely rare, but the defense-in-depth is consistent with the Cleaner's followSymlinks=false default.
IOException resilience (line 173) — Confirmed valid. An IOException from isOrphanDirectory() for one child aborts the entire loop. Moving the try/catch inside the loop (warn per child, continue scanning) would be more resilient. The outer catch already degrades gracefully (warns instead of failing), so the impact is limited to missing subsequent orphan detections when one child is unreadable.

Both are edge cases unlikely to trigger in normal Maven usage, but worth addressing in a follow-up.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot · 2026-09-27T16:46:54Z

+        if (!Files.isDirectory(basedir)) {
+            return result;
+        }
+        try (DirectoryStream<Path> children = Files.newDirectoryStream(basedir, Files::isDirectory)) {


💡 Copilot finding confirmed: Files::isDirectory follows symlinks by default. A symlink child pointing to a directory with only target/ inside would be treated as orphaned and deleted. Adding a !Files.isSymbolicLink(child) guard would be consistent with the Cleaner's default followSymlinks=false behavior.

Suggested change

try (DirectoryStream<Path> children = Files.newDirectoryStream(basedir, Files::isDirectory)) {

try (DirectoryStream<Path> children =

Files.newDirectoryStream(basedir, p -> Files.isDirectory(p) && !Files.isSymbolicLink(p))) {

gnodet-bot · 2026-09-27T16:46:54Z

+        } catch (IOException e) {
+            logger.warn("Could not scan " + basedir + " for orphaned build directories: " + e.getMessage());
+        }
+        return result;


💡 Copilot finding confirmed: If isOrphanDirectory() throws IOException for one child (e.g. permission denied), the entire loop exits and remaining children are never scanned. Moving the error handling inside the loop would be more resilient:

Suggested change

return result;

for (Path child : children) {

try {

if (isOrphanDirectory(child, buildDirName)) {

result.add(child);

}

} catch (IOException e) {

logger.warn("Could not inspect " + child + ": " + e.getMessage());

}

}

- Use LinkOption.NOFOLLOW_LINKS in Files.isDirectory() calls to prevent symlinks pointing to directories from being falsely treated as orphans - Catch IOException per-child in findOrphanBuildDirectories() so an unreadable directory does not abort the scan of remaining siblings

gnodet-bot

Re-review after 2cc8c28 — the new commit correctly addresses the two outstanding Copilot findings:

Symlink exclusion — ✅ Addressed. Both Files.isDirectory() calls now use LinkOption.NOFOLLOW_LINKS: the candidate scan filter (line 165) excludes symlinks from being considered as potential orphans, and the final check in isOrphanDirectory() (line 205) prevents treating symlink targets as build directories. Consistent with the Cleaner's followSymlinks=false default.
Per-child IOException resilience — ✅ Addressed. The try/catch is now inside the for-loop, so an IOException from isOrphanDirectory() on one child logs a warning and continues scanning remaining siblings. The outer catch remains for failure to open the root stream.

All findings from all previous review cycles are resolved. Code is solid.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot

Re-review after 2cc8c28 — the two outstanding Copilot findings are now addressed:

Symlink handling — ✅ Files.isDirectory(p, LinkOption.NOFOLLOW_LINKS) in both the directory stream filter and isOrphanDirectory() return. Symlinks to directories are excluded from orphan scanning, consistent with the Cleaner's followSymlinks=false default.
Per-child IOException resilience — ✅ try/catch moved inside the for (Path child : children) loop. An unreadable sibling no longer aborts the scan — a warning is logged and remaining children continue to be inspected.

All findings from all review cycles are resolved. Code is solid.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet · 2026-09-27T19:51:36Z

Ok for me as a manual option but ultimately for maven 4.0.0 it must be automatic and in core since core owns that fake repo and reactor thing, not plugins IMHO

@rmannibucau This is not the [root]/.mvn/target directory, this is normal directories, made orphaned because if a supression of a subproject in git for example. The idea to keep it a plugin is that it can be moved inside the lifecycle, and allows overriding / disabling / configuration as needed.

gnodet-bot

Re-review after 2cc8c28 — both outstanding findings are now addressed:

Symlink exclusion — ✅ Addressed. Files.isDirectory(p, LinkOption.NOFOLLOW_LINKS) is now used in both the directory stream filter (line 165) and the final orphan check (line 202). Symlinks to directories are no longer followed, consistent with the Cleaner's default followSymlinks=false behavior.
Per-child IOException resilience — ✅ Addressed. The try/catch is now inside the for (Path child : children) loop, so an unreadable sibling logs a warning and scanning continues for remaining children.

All findings from all review cycles are resolved.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot

Re-review after 2cc8c28 — the new commit correctly addresses both Copilot findings:

Symlink handling — ✅ Addressed. Files.newDirectoryStream filter now uses Files.isDirectory(p, LinkOption.NOFOLLOW_LINKS) instead of the default Files::isDirectory. The isOrphanDirectory() final check also uses NOFOLLOW_LINKS. Symlinks are no longer followed, consistent with the Cleaner's default followSymlinks=false behavior.
IOException resilience — ✅ Addressed. isOrphanDirectory() failures are now caught per-child inside the loop (catch (IOException e) { logger.warn(...) }), so an unreadable sibling no longer aborts the scan of remaining children. The outer catch still handles failure to open the root stream.

All findings from all review cycles are resolved. Code is solid.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

rmannibucau · 2026-09-27T21:00:54Z

@gnodet not sure what it does changes, if we do leak - and we do - and want to fix it, it is in core it belongs IMHO, clean can't be made aware of all of that nor if it changed. Typically the algo there is "any folder with target as single child", this is very fragile and will delete folder it shouldn't for ex - in particular cause it totally ignores the reactors cause the module is not more inside but also means it can visit folder with target dir which were always outside the reactor. At least core could ensure to put a target/.reactor with a timestamp or alike inside (or nothing but think some meta can help). Another issue is that a module which is "src+pom free" will be deleted (extensions enable to do that).

So really it is bound to core to me

gnodet mentioned this pull request Sep 26, 2026

Clean fails to clean orphaned build files #315

Open

gnodet-bot reviewed Sep 26, 2026

View reviewed changes

gnodet added 2 commits September 26, 2026 17:24

fix: apply spotless formatting to IT pom files

fb5f3cf

gnodet force-pushed the feat/purge-check-orphan-build-dirs branch from 9f617c6 to fb5f3cf Compare September 26, 2026 17:24

gnodet-bot reviewed Sep 26, 2026

View reviewed changes

gnodet added 2 commits September 26, 2026 17:33

refactor: move post-loop checks outside try-with-resources in orphanB…

83452d7

…uildDir

gnodet-bot reviewed Sep 26, 2026

View reviewed changes

gnodet marked this pull request as ready for review September 26, 2026 20:29

fix: remove unused Session injection from CleanOrphansMojo

70233df

gnodet-bot approved these changes Sep 26, 2026

View reviewed changes

elharo reviewed Sep 26, 2026

View reviewed changes

gnodet requested review from Bukama, cstamas, desruisseaux, hboutemy, rmannibucau, slachiewicz and slawekjaranowski September 27, 2026 08:29

desruisseaux reviewed Sep 27, 2026

View reviewed changes

fix: remove incorrect @SInCE tags on parameters, fix class @SInCE ver…

a5b5e68

…sion The Mojo is new in 4.0.0-beta-3, not 3.5.1. Parameter-level @SInCE tags are redundant when all parameters were introduced with the class itself.

gnodet-bot approved these changes Sep 27, 2026

View reviewed changes

desruisseaux reviewed Sep 27, 2026

View reviewed changes

Use Files.isHidden() instead of custom isVisible() method

221f14a

Address review feedback from @desruisseaux: replace the custom isVisible() method (dot-prefix check) with the standard Files.isHidden() API, which is platform-aware. Remove the now unused isVisible() method.

gnodet-bot reviewed Sep 27, 2026

View reviewed changes

gnodet-bot approved these changes Sep 27, 2026

View reviewed changes

desruisseaux reviewed Sep 27, 2026

View reviewed changes

Comment thread src/main/java/org/apache/maven/plugins/clean/CleanOrphansMojo.java Outdated

gnodet added 2 commits September 27, 2026 10:05

Reorder conditions: check filename before Files.isDirectory()

e92d1eb

fix: apply spotless formatting to CleanOrphansMojo

b6b800b

gnodet-bot approved these changes Sep 27, 2026

View reviewed changes

desruisseaux approved these changes Sep 27, 2026

View reviewed changes

gnodet-bot approved these changes Sep 27, 2026

View reviewed changes

rmannibucau approved these changes Sep 27, 2026

View reviewed changes

cstamas requested a lite review from Copilot September 27, 2026 16:39

Copilot started reviewing on behalf of cstamas September 27, 2026 16:40 View session

Copilot AI reviewed Sep 27, 2026

View reviewed changes

gnodet-bot reviewed Sep 27, 2026

View reviewed changes

gnodet-bot approved these changes Sep 27, 2026

View reviewed changes

cstamas approved these changes Sep 27, 2026

View reviewed changes

gnodet-bot approved these changes Sep 27, 2026

View reviewed changes

	try (DirectoryStream<Path> children = Files.newDirectoryStream(basedir, Files::isDirectory)) {
	try (DirectoryStream<Path> children =
	Files.newDirectoryStream(basedir, p -> Files.isDirectory(p) && !Files.isSymbolicLink(p))) {

Conversation

gnodet commented Sep 26, 2026

Summary

Problem

Solution

Lifecycle integration

Changes

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet commented Sep 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

desruisseaux Sep 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet commented Sep 26, 2026 •

edited

Loading

desruisseaux Sep 27, 2026 •

edited

Loading