From 16e8eef1a0a1041762c2659f7c306c9344d05745 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Thu, 28 May 2026 15:27:35 +0200 Subject: [PATCH 01/10] feat(evaluation): add local-project classpath fallback for expression eval MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Expression evaluation fails with "X cannot be resolved" when the target JVM's classloader hierarchy is opaque to JDI (Tomcat / Spring Boot dev-tools / custom URLClassLoaders that hide JARs from getURLs()). Even trivial expressions like `1+1` crash when the wrapper's _this type can't be resolved. Adds LocalProjectClasspathProvider as an additive fallback unioned into JdiExpressionEvaluator.configureCompilerClasspath. Three composable sources: JDWP_EXTRA_CLASSPATH env var, depth-5 filesystem scan for target/classes + target/test-classes, and Maven dependency:build-classpath. Synchronized memoization keeps Maven runs to at most one per JDI connection; reset() ties cache lifetime to the connection edge that already wipes the compiler. ProcessBuilderMavenRunner shells out with a 180s timeout (cold-cache mvn may download dependencies), captures 64KB of stdout for diagnosable WARN logs on failure, and uses a daemon drainer so the child never blocks on pipe buffer. InterruptedException destroys the child and restores the interrupt flag. File-safety invariant: zero Files.delete*/Files.move* in production code; Maven output stays in target/.jdwp-mcp-classpath (Maven-owned, gitignored); harvester gates on direct parent being target/; symlinks not followed. jdwp_diagnose exposes a per-source breakdown so operators can inspect what was scanned. Cold-cache diagnose uses peekCachedBreakdown() so it never triggers the 180s Maven call — discovery happens organically on first expression evaluation. Three-way env state: (unset) / (set, no entries) / (set). Closes #32 --- README.md | 5 +- docs/expression-evaluation.md | 25 + .../java/one/edee/mcp/jdwp/JDWPTools.java | 65 ++- .../evaluation/JdiExpressionEvaluator.java | 147 +++++- .../LocalProjectClasspathProvider.java | 429 ++++++++++++++++++ .../evaluation/ProcessBuilderMavenRunner.java | 278 ++++++++++++ .../edee/mcp/jdwp/JDWPToolsDiagnoseTest.java | 151 ++++++ .../edee/mcp/jdwp/JDWPToolsTestSupport.java | 58 ++- ...diExpressionEvaluatorAbsentLocalsTest.java | 4 +- ...essionEvaluatorConfigureClasspathTest.java | 10 +- ...xpressionEvaluatorGetDeclaredTypeTest.java | 4 +- ...ssionEvaluatorLocalClasspathUnionTest.java | 193 ++++++++ ...xpressionEvaluatorPackageFallbackTest.java | 4 +- ...ocalProjectClasspathProviderCacheTest.java | 182 ++++++++ ...ojectClasspathProviderEnvOverrideTest.java | 100 ++++ ...ctClasspathProviderFilesystemScanTest.java | 170 +++++++ ...ocalProjectClasspathProviderMavenTest.java | 165 +++++++ ...alProjectClasspathProviderTestSupport.java | 31 ++ .../ProcessBuilderMavenRunnerTest.java | 159 +++++++ .../java-debug/references/troubleshooting.md | 18 + 20 files changed, 2172 insertions(+), 26 deletions(-) create mode 100644 jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java create mode 100644 jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java create mode 100644 jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorLocalClasspathUnionTest.java create mode 100644 jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderCacheTest.java create mode 100644 jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderEnvOverrideTest.java create mode 100644 jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderFilesystemScanTest.java create mode 100644 jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderMavenTest.java create mode 100644 jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderTestSupport.java create mode 100644 jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunnerTest.java diff --git a/README.md b/README.md index 23a5428..a41708d 100644 --- a/README.md +++ b/README.md @@ -821,8 +821,9 @@ The server is `SYNC` mode, `web-application-type=none` — JSON over STDIO, no H 1. **JdiExpressionEvaluator** — Analyzes the stack frame, generates a wrapper class with a UUID name, delegates compilation, caches results. 2. **ClasspathDiscoverer** — Walks target JVM classloader hierarchy (including Tomcat/container) to find all JARs. Uses **JdkDiscoveryService** to locate a local JDK matching the target version. -3. **InMemoryJavaCompiler** — Compiles Java source to bytecode using Eclipse JDT (ECJ), entirely in memory. -4. **RemoteCodeExecutor** — Injects bytecode via `ClassLoader.defineClass()` and invokes it. +3. **LocalProjectClasspathProvider** — Additive fallback for when the target's classloader hierarchy hides JARs (Tomcat, Spring Boot dev-tools, custom `URLClassLoader`s). Composes `JDWP_EXTRA_CLASSPATH` (override), a depth-5 scan of `target/classes` / `target/test-classes` under the server's CWD, and `mvn dependency:build-classpath`. Set `JDWP_EXTRA_CLASSPATH=/path/extra.jar:/path/more.jar` (colon/semicolon-separated) to plug specific gaps; `jdwp_diagnose` shows a per-source breakdown. Details in [docs/expression-evaluation.md](docs/expression-evaluation.md#localprojectclasspathprovider--local-project-classpath-fallback). +4. **InMemoryJavaCompiler** — Compiles Java source to bytecode using Eclipse JDT (ECJ), entirely in memory. +5. **RemoteCodeExecutor** — Injects bytecode via `ClassLoader.defineClass()` and invokes it. ### Watcher system (`watchers/`) diff --git a/docs/expression-evaluation.md b/docs/expression-evaluation.md index ac6f1b0..4810ac8 100644 --- a/docs/expression-evaluation.md +++ b/docs/expression-evaluation.md @@ -74,6 +74,31 @@ Walks the target VM's classloader hierarchy to collect all JARs. The initial `ja Each URL is dereferenced to its path string and added to the deduplicated list. The result is cached in `JDIConnectionService.cachedClasspath`. See [memory-and-references.md](memory-and-references.md) § 9 for the allocation behaviour during the walk (transient mirrors, not cached). +### `LocalProjectClasspathProvider` — local-project classpath fallback + +The remote classloader walk above is the source of truth, but it does not see everything. Tomcat's `WebappClassLoaderBase`, Spring Boot's `LaunchedClassLoader` / dev-tools `RestartClassLoader`, and arbitrary user-written `URLClassLoader`s hide their JARs from `getURLs()` or expose them only behind reflection — so JDT can fail to resolve types that are clearly visible to the running application. To plug those gaps, `LocalProjectClasspathProvider` (Spring singleton in the `evaluation/` package) computes a second, additive classpath from the **MCP server's own working directory** and `JdiExpressionEvaluator.configureCompilerClasspath` unions it with the remote-discovered one before handing the result to `InMemoryJavaCompiler.configure`. + +**Three sources, composed in order:** + +1. **`JDWP_EXTRA_CLASSPATH` env var** — colon/semicolon-separated (parsed with `File.pathSeparator`) list of jars and class directories. Explicit override for cases the other sources can't reach (e.g. a JAR installed outside the project, or a vendor build artefact). +2. **Filesystem scan** — walks the CWD up to depth 5 looking for `target/classes` and `target/test-classes` directories. Multi-module reactors are covered without the provider needing to parse a `pom.xml`. +3. **Maven `dependency:build-classpath`** — for each detected module, invokes `./mvnw dependency:build-classpath -Dmdep.outputFile=target/.jdwp-mcp-classpath` (preferring `./mvnw` over `mvn`) and harvests the printed list. Synchronous, capped at 180 seconds, output captured into a 64 KB buffer so failures are diagnosable. + +**Union order matters.** The merge in `configureCompilerClasspath` is `[remote..., local-only...]` (de-duplicated, host `File.pathSeparator` as the joiner). JDT resolves types left-to-right, so a class present in BOTH a remote entry and a stale local copy binds against the remote definition first — the live target VM always wins on resolution. The local entries are reachable only as a fallback for classes the remote view did not provide. + +**Limitations.** + +- **Source/binary drift.** If the local checkout is on a different commit than the running target, the *remote* definition still wins because of merge order — but any class missing from the remote view binds against whatever the local jar contains. Rebuild locally to align if eval results look stale. +- **Gradle is not supported in v1.** Only Maven layouts contribute via the Maven source; Gradle projects fall back to filesystem scan + env-override only. +- **First call is slow.** Cold-cache Maven takes 1–3 minutes; the result is memoized for the JDI connection's lifetime and invalidated on the same edges that reset `InMemoryJavaCompiler` (disconnect / first use of a fresh connection). +- **No targeted-module mode.** All modules detected in the reactor union into one classpath; the provider does not try to match the breakpoint frame back to its owning module. + +**Inspection.** `jdwp_diagnose` shows a `Local project classpath` block listing the CWD, whether a `pom.xml` was found, a per-source breakdown (`env-override=N, filesystem=N, maven=N`), the total entry count, and whether the env-var override is set. The provider also logs every step under the `[LocalClasspath]` prefix — `INFO` for source contributions and timings, `WARN` for Maven non-zero exits / timeouts / malformed env values, `DEBUG` for per-entry tracing and Maven stdout on failure. + +**How to disable.** Don't set `JDWP_EXTRA_CLASSPATH` AND launch the MCP server from a directory that has no `pom.xml` and no `target/classes` under it (e.g. `/tmp`). With all three sources silent, the merged classpath is identical to the remote-discovered one. + +If both sides come up empty, `configureCompilerClasspath` throws a `JdiEvaluationException` that names the server's CWD and the two recovery levers (restart from a Maven project, or set `JDWP_EXTRA_CLASSPATH`). + ### `JdkDiscoveryService` Locates a local JDK matching the target JVM's major version. JDT needs `--system ` to resolve `java.*` system classes. Search strategy: diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java index 0a18740..9371866 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java @@ -7,6 +7,7 @@ import one.edee.mcp.jdwp.discovery.JvmDescriptor; import one.edee.mcp.jdwp.discovery.JvmDiscoveryService; import one.edee.mcp.jdwp.evaluation.JdiExpressionEvaluator; +import one.edee.mcp.jdwp.evaluation.LocalProjectClasspathProvider; import one.edee.mcp.jdwp.marks.MarkInfo; import one.edee.mcp.jdwp.marks.MarkedInstanceRegistry; import one.edee.mcp.jdwp.watchers.Watcher; @@ -109,13 +110,21 @@ public class JDWPTools { * status). Lifecycle is owned by {@link JDIConnectionService}; this class only reads snapshots. */ private final JdiHealthMonitor healthMonitor; + /** + * Local-project classpath fallback. Surfaced through {@code jdwp_diagnose} so operators can + * see which sources (env override / filesystem / Maven) contributed to the additive classpath + * fed into expression evaluation. Memoized per JDI connection; the diagnose call may trigger + * the first (slow) discovery if no expression has been evaluated yet. + */ + private final LocalProjectClasspathProvider localClasspathProvider; public JDWPTools(JDIConnectionService jdiService, BreakpointTracker breakpointTracker, WatcherManager watcherManager, JdiExpressionEvaluator expressionEvaluator, EventHistory eventHistory, EvaluationGuard evaluationGuard, JvmDiscoveryService jvmDiscoveryService, MarkedInstanceRegistry markedInstances, - JdiHealthMonitor healthMonitor) { + JdiHealthMonitor healthMonitor, + LocalProjectClasspathProvider localClasspathProvider) { this.jdiService = jdiService; this.breakpointTracker = breakpointTracker; this.watcherManager = watcherManager; @@ -125,6 +134,7 @@ public JDWPTools(JDIConnectionService jdiService, BreakpointTracker breakpointTr this.jvmDiscoveryService = jvmDiscoveryService; this.markedInstances = markedInstances; this.healthMonitor = healthMonitor; + this.localClasspathProvider = localClasspathProvider; } /** @@ -1690,10 +1700,63 @@ private String buildFullDiagnosticReport(boolean inspectAll) { out.append('\n').append(buildDiagnosticReport(false, null)); } + // Local project classpath section: shows which entries the additive fallback contributed + // to expression evaluation. Independent of connection state so operators can sanity-check + // their CWD even before attaching. May trigger the first (cold-cache, slow) discovery — + // intentional; the header explicitly warns about the latency. + out.append(renderLocalClasspathBlock()); + out.append(renderLocalJvmsBlock(status, inspectAll)); return out.toString(); } + /** + * Renders the "Local project classpath" diagnose section. NON-BLOCKING by design — calls into + * {@link LocalProjectClasspathProvider#peekCachedBreakdown()} so the report never waits on a + * cold-cache Maven invocation (which can take up to ~3 min). When the cache is cold the block + * prints a "not yet computed" hint; the breakdown populates organically the next time + * expression evaluation runs. + * + *

The env-state line is three-way: + * {@code (unset)} when the variable is absent, {@code (set, no entries)} when present but + * blank, and {@code (set)} when present with at least one entry. This distinction matters to + * operators who have set the variable but see it rendered as if absent. + */ + private String renderLocalClasspathBlock() { + final StringBuilder out = new StringBuilder("\n▸ Local project classpath\n"); + out.append(" (computed once per JDI connection; first call may take up to 3 min on cold Maven cache)\n"); + try { + final LocalProjectClasspathProvider.Breakdown breakdown = + localClasspathProvider.peekCachedBreakdown(); + out.append(" CWD: ").append(localClasspathProvider.getWorkingDirectory()).append('\n'); + out.append(" pom.xml: ").append(localClasspathProvider.hasPomAtRoot() ? "yes" : "no").append('\n'); + final boolean envSet = localClasspathProvider.isEnvOverrideSet(); + if (breakdown == null) { + out.append(" (not yet computed; will populate on first expression evaluation)\n"); + final String envState = envSet ? "(set)" : "(unset)"; + out.append(" Override env: JDWP_EXTRA_CLASSPATH ").append(envState).append('\n'); + } else { + final String envState; + if (!envSet) { + envState = "(unset)"; + } else if (breakdown.envOverride() > 0) { + envState = "(set)"; + } else { + envState = "(set, no entries)"; + } + out.append(" Sources: env-override=").append(breakdown.envOverride()) + .append(", filesystem=").append(breakdown.filesystem()) + .append(", maven=").append(breakdown.maven()).append('\n'); + out.append(" Total local entries: ").append(breakdown.all().size()).append('\n'); + out.append(" Override env: JDWP_EXTRA_CLASSPATH ").append(envState).append('\n'); + } + } catch (Exception e) { + log.warn("Local classpath discovery failed during diagnose", e); + out.append(" (local classpath discovery failed: ").append(e.getMessage()).append(")\n"); + } + return out.toString(); + } + /** * Runs discovery and renders the local-JVM inventory block. Discovery errors are caught * and surfaced as a one-line note instead of breaking the whole report. Accepts a diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java index 109f20d..589192f 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java @@ -10,6 +10,7 @@ import org.slf4j.LoggerFactory; import org.springframework.stereotype.Service; +import java.io.File; import java.util.*; import java.util.concurrent.ConcurrentHashMap; import java.util.stream.Collectors; @@ -67,6 +68,13 @@ public class JdiExpressionEvaluator { private final InMemoryJavaCompiler compiler; private final JDIConnectionService jdiConnectionService; private final EvaluationGuard evaluationGuard; + /** + * Additive local-project classpath. Unioned with the remote classloader-discovered classpath in + * {@link #configureCompilerClasspath} to fill gaps the remote walk cannot see (Tomcat / Spring + * Boot dev-tools / custom URLClassLoaders that hide their JARs from {@code getURLs()}). Reset + * on every reconnect so a new connection sees the current project layout. + */ + private final LocalProjectClasspathProvider localClasspathProvider; /** * Compilation cache. Key is `contextSignature + "###" + expression`, so two frames with the @@ -79,11 +87,13 @@ public class JdiExpressionEvaluator { public JdiExpressionEvaluator( InMemoryJavaCompiler compiler, JDIConnectionService jdiConnectionService, - EvaluationGuard evaluationGuard + EvaluationGuard evaluationGuard, + LocalProjectClasspathProvider localClasspathProvider ) { this.compiler = compiler; this.jdiConnectionService = jdiConnectionService; this.evaluationGuard = evaluationGuard; + this.localClasspathProvider = localClasspathProvider; } /** @@ -841,10 +851,10 @@ private static String resolveWrapperPackage(@Nullable ObjectReference thisObject // Refuse to define classes into restricted JDK packages — `java.*` is enforced by the JVM // itself, the rest are flagged here so we get a clean log message rather than a runtime // SecurityException far from the decision site. - if (pkg.isEmpty() || pkg.startsWith("java.") || pkg.equals("java") - || pkg.startsWith("javax.") || pkg.equals("javax") - || pkg.startsWith("sun.") || pkg.equals("sun") - || pkg.startsWith("jdk.") || pkg.equals("jdk")) { + if (pkg.isEmpty() || pkg.startsWith("java.") || "java".equals(pkg) + || pkg.startsWith("javax.") || "javax".equals(pkg) + || pkg.startsWith("sun.") || "sun".equals(pkg) + || pkg.startsWith("jdk.") || "jdk".equals(pkg)) { return DEFAULT_EVALUATION_PACKAGE; } return pkg; @@ -1127,8 +1137,10 @@ private static ClassLoaderReference findClassLoader(StackFrame frame) throws Jdi * Configures the compiler with the target JVM's classpath. Skips if already configured for the * current connection. Automatically reconfigures after a disconnect/reconnect cycle (detected * via null JDK path): clears the compilation cache AND resets the compiler config because stale - * bytecode and a stale classpath may reference classes from a previous connection. Must be - * called BEFORE any expression evaluation to avoid nested JDI calls. + * bytecode and a stale classpath may reference classes from a previous connection. Also resets + * the {@link LocalProjectClasspathProvider} cache on the same edge so a reconnect into a + * different working directory sees the new project layout. Must be called BEFORE any expression + * evaluation to avoid nested JDI calls. * * @param suspendedThread a thread already suspended at a breakpoint (REQUIRED) * @throws JdiEvaluationException if classpath/JDK discovery fails for the current connection; @@ -1156,8 +1168,12 @@ public synchronized void configureCompilerClasspath(ThreadReference suspendedThr // the compiler config before rediscovering: if discovery then fails, compile() refuses // with a clear "not configured" error instead of silently emitting bytecode resolved // against the previous target's classpath. + // The local-classpath provider lives in the same Spring singleton and may hold a + // memoised view of a *previous* connection's project layout — reset it on the same + // connection-lifecycle edge so the next discover() sees the current CWD/env/Maven view. compilationCache.clear(); compiler.reset(); + localClasspathProvider.reset(); final long startTime = System.currentTimeMillis(); @@ -1166,7 +1182,7 @@ public synchronized void configureCompilerClasspath(ThreadReference suspendedThr // that into an actionable exception rather than returning silently — otherwise the // caller proceeds to compile() and the user only sees a raw JDT diagnostic (e.g. // "io cannot be resolved") with no hint that the real cause was a failed discovery. - final String classpath = jdiConnectionService.discoverClasspath(suspendedThread); + final String remoteClasspath = jdiConnectionService.discoverClasspath(suspendedThread); final String jdkPath = jdiConnectionService.getDiscoveredJdkPath(); if (jdkPath == null) { @@ -1175,15 +1191,37 @@ public synchronized void configureCompilerClasspath(ThreadReference suspendedThr + "the target VM could be located, so application types cannot be resolved. " + "Check the server log (search '[JDI]') for the underlying cause."); } - if (classpath == null || classpath.isEmpty()) { + + // Local classpath augments — does NOT replace — the remote one. The union goes + // [remote..., local-only...] so live target VM entries continue to win on JDT + // resolution; local entries only fill gaps the remote walk could not see (Tomcat / + // Spring Boot / custom URLClassLoaders that hide their JARs from getURLs()). + // Resolution order: ECJ scans the classpath left-to-right (InMemoryJavaCompiler + // passes the joined string as `-classpath`), so a class present in BOTH a remote and + // a stale local entry binds against the remote definition first — desired behaviour + // for the source/binary-drift risk. + final long mergeStart = System.currentTimeMillis(); + final Set localEntries = localClasspathProvider.discover(); + final String mergedClasspath = mergeClasspaths(remoteClasspath, localEntries); + log.info("[LocalClasspath] Merged classpath: {} remote + {} local-only entries in {}ms", + countEntries(remoteClasspath), localEntries.size(), + System.currentTimeMillis() - mergeStart); + + if (mergedClasspath.isEmpty()) { throw new JdiEvaluationException( - "Classpath discovery failed for the current connection: the target VM classpath " - + "could not be determined, so application types cannot be resolved. " - + "Check the server log (search '[JDI]') for the underlying cause."); + "Classpath discovery failed for the current connection: neither the target VM " + + "classloader hierarchy nor the local project yielded any classpath entries. " + + "The MCP server was launched from the directory '" + + Objects.toString(System.getProperty("user.dir"), "") + "'. " + + "Either (a) restart the server from a directory containing a Maven project " + + "(pom.xml + target/classes), or (b) set the JDWP_EXTRA_CLASSPATH environment " + + "variable to a colon/semicolon-separated list of jars and class directories. " + + "Run jdwp_diagnose to inspect what was scanned, and check the server log for " + + "'[LocalClasspath]' and '[Discoverer]' entries."); } final int version = jdiConnectionService.getTargetMajorVersion(); - compiler.configure(jdkPath, classpath, version); + compiler.configure(jdkPath, mergedClasspath, version); log.info("[Evaluator] Compiler configured in {}ms", System.currentTimeMillis() - startTime); } finally { evaluationGuard.exit(guardedThreadId); @@ -1216,6 +1254,89 @@ public void prewarmClasspath(ThreadReference suspendedThread) { } } + /** + * Unions the remote-discovered classpath (target VM) with the local-project entries, deduping + * while preserving insertion order so remote entries appear first. Putting remote entries first + * ensures JDT binds against the live target's definition when the same class also appears in a + * (possibly stale) local entry. + * + *

Splits the remote string via {@link #splitRemoteClasspath} so a single-entry Windows path + * like {@code "C:\foo"} stays intact rather than being shredded on the colon. + * + * @param remote remote-discovered classpath joined by the target VM's path separator; + * {@code null}, empty, and blank are tolerated + * @param local local-project entries in insertion order (typically from + * {@link LocalProjectClasspathProvider#discover()}) + * @return single string joined by the host {@link File#pathSeparator}, suitable to hand directly + * to the in-memory compiler's {@code -classpath} argument + */ + private static String mergeClasspaths(@Nullable String remote, Set local) { + final Set union = new LinkedHashSet<>(); + if (remote != null && !remote.isEmpty()) { + for (String entry : splitRemoteClasspath(remote)) { + final String trimmed = entry.trim(); + if (!trimmed.isEmpty()) { + union.add(trimmed); + } + } + } + union.addAll(local); + return String.join(File.pathSeparator, union); + } + + /** + * Counts the non-blank entries in a remote-discovered classpath string. Used purely for the + * INFO log line that summarises the merge result. Honours the same single-entry Windows + * heuristic as {@link #mergeClasspaths} so the two stay in lockstep. + */ + private static int countEntries(@Nullable String classpath) { + if (classpath == null || classpath.isEmpty()) { + return 0; + } + int count = 0; + for (String e : splitRemoteClasspath(classpath)) { + if (!e.trim().isEmpty()) { + count++; + } + } + return count; + } + + /** + * Splits a remote classpath into entries. Three cases: + *

    + *
  1. Contains {@code ;} — Windows-style classpath, split on {@code ;}.
    2. + *
  2. Looks like a single Windows path ({@code :\...}) — treat as one entry.
    3. + *
  3. Otherwise — Unix-style classpath, split on {@code :}.
    4. + *
+ */ + private static String[] splitRemoteClasspath(String classpath) { + if (classpath.contains(";")) { + return classpath.split(";", -1); + } + if (looksLikeSingleWindowsPath(classpath)) { + return new String[] { classpath }; + } + return classpath.split(":", -1); + } + + /** + * Heuristic: {@code true} when {@code value} starts with {@code :\} or {@code :/}, + * indicating a single Windows path that must not be split on {@code :}. Multi-entry Windows + * classpaths contain {@code ;} and are filtered by the caller before this check runs. + * + *

Limitation. A Unix-style classpath whose first entry happens to look like a Windows + * drive prefix (vanishingly unlikely in practice) would be misclassified as a single entry. This + * tradeoff is accepted because the alternative — host-OS sniffing — fails when the MCP server + * and target VM run on different operating systems. + */ + private static boolean looksLikeSingleWindowsPath(String value) { + return value.length() >= 3 + && Character.isLetter(value.charAt(0)) + && value.charAt(1) == ':' + && (value.charAt(2) == '\\' || value.charAt(2) == '/'); + } + /** * Captures the variable names, types, values, and a derived signature from a stack frame for * expression compilation. The signature is used as part of the compilation cache key so frames diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java new file mode 100644 index 0000000..8c27733 --- /dev/null +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java @@ -0,0 +1,429 @@ +package one.edee.mcp.jdwp.evaluation; + +import org.jspecify.annotations.Nullable; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Service; + +import java.io.File; +import java.nio.file.AccessDeniedException; +import java.nio.file.Files; +import java.nio.file.LinkOption; +import java.nio.file.Path; +import java.util.Collections; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; +import java.util.function.Function; +import java.util.regex.Pattern; +import java.util.stream.Stream; + +/** + * Computes a local-project classpath used as an additive fallback when remote classloader-based + * discovery returns an incomplete view of the target VM's classpath (Tomcat / Spring Boot dev-tools + * / custom URLClassLoaders that hide their JARs from {@code getURLs()}). + * + *

Three composable sources, evaluated in order. The result preserves insertion order so the + * first source to contribute an entry wins. The caller is responsible for unioning the returned + * entries with whatever the remote discovery produced — this provider never consults the target VM. + *

    + *
  • {@code JDWP_EXTRA_CLASSPATH} env var, parsed by {@link File#pathSeparator}
    • + *
  • Filesystem scan of {@code target/classes} / {@code target/test-classes} under the server CWD
    • + *
  • Maven {@code dependency:build-classpath} per detected {@code pom.xml}
    • + *
+ * + *

The first {@link #discoverWithBreakdown()} call is memoised; subsequent calls return the cached + * result until {@link #reset()} is invoked (typically on JDI reconnect). Discovery is synchronised + * so concurrent callers see the same memoised snapshot. + * + *

Registered as a Spring {@code @Service}, but the second constructor takes seams so the env + * lookup and Maven invocation can be stubbed in tests without touching real environment variables + * or shelling out. + */ +@Service +public class LocalProjectClasspathProvider { + + private static final Logger log = LoggerFactory.getLogger(LocalProjectClasspathProvider.class); + /** Environment variable callers can set to inject extra classpath entries verbatim. */ + private static final String ENV_NAME = "JDWP_EXTRA_CLASSPATH"; + /** + * Filesystem-scan depth cap. Five levels covers reactor / module / sub-module layouts without + * walking into Node / Gradle / IDE caches that an unbounded walk would otherwise stat-flood. + */ + private static final int MAX_SCAN_DEPTH = 5; + /** + * Cold-cache {@code mvn} may download dependencies on first run, which routinely takes 1-3 + * minutes on a fresh machine. 180s covers the common case; a real timeout still logs a clear + * WARN with the captured stdout so the user knows what to retry. + */ + private static final int MAVEN_TIMEOUT_SECONDS = 180; + /** + * Directory names the filesystem scan must never descend into — VCS, IDE, package-manager + * caches, and {@code target} itself (we look at it explicitly and never recurse). Without the + * list a {@code target/} hit in {@code node_modules} would balloon I/O and yield junk entries. + */ + private static final Set SKIP_DIRS = Set.of( + ".git", ".idea", ".vscode", "node_modules", ".gradle", ".mvn", + "target" // skip target itself — we look at it explicitly, never recurse into it + ); + + /** + * Subset of {@link #SKIP_DIRS} that other components (notably the Maven-output harvester) must + * also skip when walking the project tree. {@code target} is excluded here because the harvester + * specifically looks for files whose direct parent is {@code target/} — that gate handles the + * "skip nested target dirs we shouldn't descend into" concern without needing this list. + */ + static final Set HARVEST_SKIP_DIRS = Set.of( + ".git", ".idea", ".vscode", "node_modules", ".gradle", ".mvn" + ); + + /** + * Per-source breakdown of the most recent discovery; exposed by {@code jdwp_diagnose}. + * + * @param envOverride number of entries contributed by {@code JDWP_EXTRA_CLASSPATH} + * @param filesystem number of {@code target/classes} / {@code target/test-classes} entries the + * filesystem scan added on top of the env override + * @param maven number of entries Maven's {@code dependency:build-classpath} added on top + * of env + filesystem (dedup'd against earlier sources) + * @param all insertion-ordered union of all contributing entries; never null + */ + public record Breakdown(int envOverride, int filesystem, int maven, Set all) {} + + /** Root the scan is anchored at; usually the MCP server's launch directory. */ + private final Path workingDirectory; + /** Indirection for {@link System#getenv(String)} so tests can drive the env-override branch. */ + private final Function envLookup; + /** Indirection for the Maven shell-out so tests can stub success/failure modes. */ + private final MavenRunner mavenRunner; + /** Memoised result of the first {@link #discoverWithBreakdown()} call; cleared by {@link #reset()}. */ + private @Nullable Breakdown cachedBreakdown; + + /** + * Test seam: build the provider with explicit working directory, env-lookup function, and Maven + * runner. Spring wires the no-seam constructor in production — tests use this one to drive every + * code path without touching real environment variables or shelling out. + */ + public LocalProjectClasspathProvider(Path workingDirectory, + Function envLookup, + MavenRunner mavenRunner) { + this.workingDirectory = workingDirectory; + this.envLookup = envLookup; + this.mavenRunner = mavenRunner; + } + + /** + * Default Spring wiring: uses the JVM's launch directory as the working directory and the real + * process environment for the env-var lookup. The {@link ProcessBuilderMavenRunner} is injected + * so Maven invocations go through the real shell-out path in production. + */ + @Autowired + public LocalProjectClasspathProvider(ProcessBuilderMavenRunner mavenRunner) { + this(Path.of(System.getProperty("user.dir")), System::getenv, mavenRunner); + } + + /** + * Returns the union of all contributing entries from the memoised breakdown. Convenience + * wrapper around {@link #discoverWithBreakdown()} for callers that only need the merged set. + * + * @return insertion-ordered, immutable set of local classpath entries + */ + public Set discover() { + return discoverWithBreakdown().all(); + } + + /** + * Working directory the provider is rooted at — exposed for the {@code jdwp_diagnose} report so + * operators see the absolute path the scan started from without re-reading {@code user.dir}. + */ + public Path getWorkingDirectory() { + return workingDirectory; + } + + /** + * True when a {@code pom.xml} sits at the working-directory root. Cheap I/O; called only from + * the diagnose path (once per call). Surfaced so the report can say "pom.xml: yes/no" without + * duplicating the path logic. + */ + public boolean hasPomAtRoot() { + return Files.isRegularFile(workingDirectory.resolve("pom.xml")); + } + + /** + * Non-blocking peek at the cached breakdown. Returns {@code null} when no discovery has run + * yet on this provider instance. Used by the diagnose path so the report never blocks the + * caller waiting for a cold-cache Maven invocation; if the cache is cold, the diagnose + * renderer prints a one-line "not yet computed" hint and moves on. + */ + public synchronized @Nullable Breakdown peekCachedBreakdown() { + return cachedBreakdown; + } + + /** + * Reports whether the {@code JDWP_EXTRA_CLASSPATH} env var is present in the lookup, regardless + * of whether it parses to any entries. The diagnose path uses this to distinguish three states: + *

    + *
  • {@code (unset)} — the variable is absent from the environment
    • + *
  • {@code (set, no entries)} — set but blank or whitespace-only
    • + *
  • {@code (set)} — set with at least one entry
    • + *
+ * Operators chase a frustrating false-negative when "set-but-blank" is rendered the same as + * "unset". Exposing presence separately fixes the conflation without touching the breakdown. + */ + public boolean isEnvOverrideSet() { + return envLookup.apply(ENV_NAME) != null; + } + + /** + * Runs (or returns the memoised result of) the three-source discovery. Synchronised so two + * concurrent callers cannot both pay the Maven invocation cost; the second caller blocks and + * receives the first caller's cached breakdown. + * + *

On an empty result, emits one INFO log line that explains which sources had what state + * (env unset vs blank, {@code target/} present, {@code pom.xml} present) so operators can + * diagnose "why is nothing being added" without enabling DEBUG. + * + * @return memoised breakdown — never null; {@link Breakdown#all()} is immutable + */ + public synchronized Breakdown discoverWithBreakdown() { + final Breakdown cached = cachedBreakdown; + if (cached != null) { + return cached; + } + final Set entries = new LinkedHashSet<>(); + addEnvOverride(entries); + final int afterEnv = entries.size(); + addFilesystemScan(entries); + final int afterFs = entries.size(); + addMavenDependencies(entries); + final int afterMaven = entries.size(); + final Breakdown breakdown = new Breakdown( + afterEnv, + afterFs - afterEnv, + afterMaven - afterFs, + // Wrap a defensive LinkedHashSet copy so the cached view is immutable but still + // preserves insertion order — callers rely on containsExactly-style assertions and + // Set.copyOf would drop order (it returns a hash-based immutable set). + Collections.unmodifiableSet(new LinkedHashSet<>(entries)) + ); + cachedBreakdown = breakdown; + if (entries.isEmpty()) { + // Required by the logging policy: ONE INFO line on empty result explaining the why. + log.info("[LocalClasspath] discover() found 0 entries — env={}, fs={}, maven={} (cwd={}, pom.xml={})", + envLookup.apply(ENV_NAME) == null ? "unset" : "set-but-empty", + Files.isDirectory(workingDirectory.resolve("target")) ? "target/-present" : "no-target/", + Files.isRegularFile(workingDirectory.resolve("pom.xml")) ? "pom-present" : "no-pom", + workingDirectory, + Files.isRegularFile(workingDirectory.resolve("pom.xml"))); + } + return breakdown; + } + + /** + * Clears the memoised breakdown so the next {@link #discoverWithBreakdown()} call re-scans. + * Called by {@link JdiExpressionEvaluator#configureCompilerClasspath} on a JDI reconnect because + * the new connection may target a different project layout — a stale breakdown would mismatch. + */ + public synchronized void reset() { + cachedBreakdown = null; + log.debug("[LocalClasspath] Cache reset"); + } + + /** + * Adds Maven-resolved dependency jars to {@code entries}. No-ops cleanly when no {@code pom.xml} + * sits at the working-directory root. Logs at INFO with the count and elapsed time on every + * outcome (added > 0, added == 0, failure) so operators see one line per discovery. + */ + private void addMavenDependencies(Set entries) { + if (!Files.isRegularFile(workingDirectory.resolve("pom.xml"))) { + log.debug("[LocalClasspath] No pom.xml under {} — skipping Maven source", workingDirectory); + return; + } + final List command = buildMavenCommand(); + final long startTime = System.currentTimeMillis(); + try { + final List jars = mavenRunner.run(command, workingDirectory, MAVEN_TIMEOUT_SECONDS); + final int before = entries.size(); + entries.addAll(jars); + final int added = entries.size() - before; + final long elapsed = System.currentTimeMillis() - startTime; + if (added > 0) { + log.info("[LocalClasspath] Maven contributed {} dependency jars in {}ms", added, elapsed); + } else { + // "Reason-out on empty" — required by the logging policy. The runner already logs the + // shell-level failure; this line gives an operator the user-facing context. + log.info("[LocalClasspath] Maven contributed 0 jars in {}ms — check earlier WARN lines " + + "for the cause (timeout, non-zero exit, missing output files)", elapsed); + } + } catch (Exception e) { + log.warn("[LocalClasspath] Maven dependency:build-classpath failed after {}ms: {}: {} (cwd={})", + System.currentTimeMillis() - startTime, e.getClass().getSimpleName(), e.getMessage(), + workingDirectory); + } + } + + /** + * Builds the {@code dependency:build-classpath} command line, picking up the project's Maven + * wrapper when available (see {@link #resolveMavenExecutable}). The output file lives under each + * module's {@code target/}, never at a bare project-root filename. + */ + private List buildMavenCommand() { + final String executable = resolveMavenExecutable(); + // `-q` keeps Maven CLI noise off the operator's terminal; the runner captures stdout to a + // size-capped buffer so the WARN path can still surface diagnostic output on failure. + // `-DincludeScope=runtime` matches what's actually on the app's runtime classpath. + // The output file lives under each module's `target/` — Maven-owned build output, gitignored + // everywhere, wiped by `mvn clean`. NEVER use a bare filename here (`.jdwp-mcp-classpath`): + // that would land in project source directories which we must not write to. See the + // file-safety policy. + // We do NOT pass `-Dmdep.pathSeparator=:` — that flag forces a Unix separator on Windows and + // would corrupt drive letters in jar paths. Letting Maven use the platform default and + // splitting on File.pathSeparator on the read side keeps things consistent. + return List.of( + executable, + "-q", + "dependency:build-classpath", + "-DincludeScope=runtime", + "-Dmdep.outputFile=target/.jdwp-mcp-classpath" + ); + } + + /** + * Picks the Maven executable in priority order: {@code mvnw} (Unix wrapper) when executable, + * {@code mvnw.cmd} (Windows wrapper) when running on Windows and the file exists, then plain + * {@code mvn} from the PATH. {@code mvnw.cmd} cannot be probed with + * {@link Files#isExecutable(Path)} on Windows because the NTFS execute bit isn't a thing — + * {@link Files#isRegularFile(Path, LinkOption...)} is the right gate. + */ + private String resolveMavenExecutable() { + final Path mvnw = workingDirectory.resolve("mvnw"); + if (Files.isExecutable(mvnw)) { + return mvnw.toAbsolutePath().toString(); + } + if (isWindows()) { + final Path mvnwCmd = workingDirectory.resolve("mvnw.cmd"); + if (Files.isRegularFile(mvnwCmd)) { + return mvnwCmd.toAbsolutePath().toString(); + } + } + return "mvn"; + } + + private static boolean isWindows() { + return File.separatorChar == '\\'; + } + + /** + * Walks the working directory for {@code target/classes} / {@code target/test-classes} dirs and + * adds each as an entry. Always emits one INFO line summarising the count and elapsed time so + * operators can see whether the scan contributed or not without enabling DEBUG. + */ + private void addFilesystemScan(Set entries) { + final long startTime = System.currentTimeMillis(); + final int before = entries.size(); + scanForClassDirs(workingDirectory, 0, entries); + final int added = entries.size() - before; + final long elapsed = System.currentTimeMillis() - startTime; + if (added > 0) { + log.info("[LocalClasspath] Filesystem scan contributed {} target/classes entries in {}ms", + added, elapsed); + } else { + log.info("[LocalClasspath] Filesystem scan found 0 target/classes under {} (depth <= {}, {}ms)", + workingDirectory, MAX_SCAN_DEPTH, elapsed); + } + } + + /** + * Recursively probes {@code dir} (and descendants, up to {@link #MAX_SCAN_DEPTH}) for + * {@code target/classes} / {@code target/test-classes}. Skips {@link #SKIP_DIRS} and + * dot-prefixed directories. Tolerates per-directory failures (logged at WARN) rather than + * aborting — a partial classpath beats no classpath. + */ + private static void scanForClassDirs(Path dir, int depth, Set entries) { + if (depth > MAX_SCAN_DEPTH || !Files.isDirectory(dir)) { + return; + } + // At each directory, probe for target/classes and target/test-classes BEFORE recursing — we + // want a module's classes whether or not it has child modules. + final Path classes = dir.resolve("target/classes"); + if (Files.isDirectory(classes)) { + entries.add(classes.toString()); + } + final Path testClasses = dir.resolve("target/test-classes"); + if (Files.isDirectory(testClasses)) { + entries.add(testClasses.toString()); + } + if (depth == MAX_SCAN_DEPTH) { + return; + } + try (Stream children = Files.list(dir)) { + children + .filter(Files::isDirectory) + .filter(p -> { + final String name = p.getFileName().toString(); + return !name.startsWith(".") && !SKIP_DIRS.contains(name); + }) + .forEach(child -> scanForClassDirs(child, depth + 1, entries)); + } catch (AccessDeniedException e) { + // Permission denied on a single directory is expected (CI agents, restricted dev envs). + // Log at WARN so an operator sees it once, but never abort the whole scan. + log.warn("[LocalClasspath] Permission denied listing {} — skipping subtree", dir); + } catch (Exception e) { + // Any other failure on a single directory is unexpected — log it visibly. We do NOT + // throw: a partial classpath is better than no classpath, and the agent will see the + // diagnose breakdown to understand what happened. + log.warn("[LocalClasspath] Could not list {}: {}: {}", + dir, e.getClass().getSimpleName(), e.getMessage()); + } + } + + /** + * Parses {@link #ENV_NAME} (when set) on the host {@link File#pathSeparator} and adds each + * non-blank, non-duplicate entry. Logs at INFO with the added count; at DEBUG when the variable + * is unset or blank. + */ + private void addEnvOverride(Set entries) { + final String raw = envLookup.apply(ENV_NAME); + if (raw == null || raw.isBlank()) { + log.debug("[LocalClasspath] {} is unset or blank — env override contributed 0 entries", ENV_NAME); + return; + } + // Split on the HOST OS path separator. A regex like `[;:]` is WRONG on Windows because + // `C:\foo` contains a colon — splitting would shred the drive letter. The env var is set + // by the user on the host, so the host's File.pathSeparator is the correct delimiter. + final String[] parts = raw.split(Pattern.quote(File.pathSeparator), -1); + int added = 0; + for (String part : parts) { + final String trimmed = part.trim(); + if (!trimmed.isEmpty() && entries.add(trimmed)) { + added++; + } + } + log.info("[LocalClasspath] {} contributed {} entries", ENV_NAME, added); + } + + /** + * Seam for executing a Maven command and harvesting its output. The production implementation + * is {@link ProcessBuilderMavenRunner}; tests provide in-memory stubs. + * + *

Implementations MUST follow the empty-list-on-failure contract: any non-zero exit, + * timeout, exception, or interruption is reflected by returning an empty list (after logging + * the cause). This keeps the provider's outer flow declarative — it can union the result with + * other sources without per-source null/error handling. + */ + @FunctionalInterface + public interface MavenRunner { + /** + * Runs {@code command} with {@code workingDirectory} as the process CWD, bounded by + * {@code timeoutSeconds}, and returns the classpath entries the command's output files + * contributed. + * + * @param command full argv (executable + arguments) to invoke + * @param workingDirectory CWD for the spawned process; output files are harvested under it + * @param timeoutSeconds wall-clock budget; on timeout the implementation MUST kill the + * process and return an empty list + * @return harvested classpath entries; empty on any failure (never null) + */ + List run(List command, Path workingDirectory, int timeoutSeconds); + } +} diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java new file mode 100644 index 0000000..ca1db3e --- /dev/null +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java @@ -0,0 +1,278 @@ +package one.edee.mcp.jdwp.evaluation; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.stereotype.Service; + +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; +import java.util.concurrent.TimeUnit; +import java.util.regex.Pattern; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +/** + * Production {@link LocalProjectClasspathProvider.MavenRunner} that shells out to {@code mvn} (or + * the project's Maven wrapper) via {@link ProcessBuilder} and harvests the per-module + * {@code .jdwp-mcp-classpath} files Maven writes under each module's {@code target/}. + * + *

Responsibility boundary: this class owns the process invocation, timeout enforcement, stdout + * draining, and output-file collection. It NEVER deletes files — {@code target/} is Maven-owned + * and gets wiped by {@code mvn clean}; arbitrary deletion in the project tree is prohibited by the + * file-safety policy. Honours the {@link LocalProjectClasspathProvider.MavenRunner} empty-list-on-failure + * contract: every error path returns {@link List#of()} after logging. + * + *

Thread model: each {@link #run} invocation spawns ONE daemon platform thread named + * {@code jdwp-mcp-maven-stdout-drainer} to drain the child process's merged stdout/stderr stream. + * The drainer terminates when the child closes its pipe (i.e. when the process exits or is forcibly + * destroyed), so its lifetime is bounded by the process. Platform (not virtual) threads are used + * because the project targets Java 17. + */ +@Service +public class ProcessBuilderMavenRunner implements LocalProjectClasspathProvider.MavenRunner { + + /** Filename Maven writes the classpath into under each module's {@code target/}. */ + private static final String OUTPUT_FILE_NAME = ".jdwp-mcp-classpath"; + /** Upper bound on captured child stdout/stderr included in the WARN log on failure. */ + private static final int STDOUT_CAPTURE_BYTES = 64 * 1024; + private static final Logger log = LoggerFactory.getLogger(ProcessBuilderMavenRunner.class); + + /** Seam for the actual process execution; replaced in tests to drive timeout / failure paths. */ + private final CommandExecutor commandExecutor; + + /** Default Spring wiring — uses the real {@link ProcessBuilder}-based command executor. */ + public ProcessBuilderMavenRunner() { + this(ProcessBuilderMavenRunner::executeRealCommand); + } + + /** Test seam: swap in a stub {@link CommandExecutor} to drive success, failure, and timeout. */ + ProcessBuilderMavenRunner(CommandExecutor commandExecutor) { + this.commandExecutor = commandExecutor; + } + + @Override + public List run(List command, Path workingDirectory, int timeoutSeconds) { + // Snapshot any pre-existing descendant PIDs so the cleanup below only targets processes + // that this call spawned. Without the snapshot, an InterruptedException from the executor + // would either leak the spawned child (current behaviour) or, worse, race against unrelated + // descendant processes started for some other reason. + final Set preExistingDescendants = ProcessHandle.current().descendants() + .map(ProcessHandle::pid) + .collect(Collectors.toSet()); + try { + final int exitCode = commandExecutor.execute(new CommandRequest(command, workingDirectory, timeoutSeconds)); + // executeRealCommand already logs WARN with the captured stdout on non-zero/timeout; + // no second log line here. + if (exitCode != 0) { + return List.of(); + } + return harvestOutputFiles(workingDirectory); + } catch (InterruptedException ie) { + // Restore the interrupt flag so callers higher up the stack can observe the cancellation. + // Then destroy any descendant process this call spawned — the child holds an OS pipe + // open and would block any caller that later tries to drain stdout. Targeted at PIDs + // new since the snapshot taken before the executor call. + destroyNewDescendants(preExistingDescendants); + Thread.currentThread().interrupt(); + log.warn("[LocalClasspath] Maven invocation interrupted (cwd={}); destroyed spawned descendants", + workingDirectory); + return List.of(); + } catch (Exception e) { + log.warn("[LocalClasspath] Maven invocation failed: {}: {} (cwd={})", + e.getClass().getSimpleName(), e.getMessage(), workingDirectory); + return List.of(); + } + } + + private static void destroyNewDescendants(Set preExistingPids) { + // Snapshot the new descendants up-front so we can both issue destroy AND await termination + // before returning. Issuing destroy without awaiting leaves the caller racing the OS — a + // test that checks `isAlive()` immediately after run() can flake. 2s is enough for SIGKILL + // to land on a sleep(1)-style process on any sane kernel. + final List newDescendants = ProcessHandle.current().descendants() + .filter(h -> !preExistingPids.contains(h.pid())) + .toList(); + newDescendants.forEach(ProcessHandle::destroyForcibly); + for (ProcessHandle h : newDescendants) { + try { + h.onExit().get(2, TimeUnit.SECONDS); + } catch (Exception ignored) { + // Best-effort: a stuck/orphaned process is logged elsewhere; we already issued + // destroyForcibly. The caller still gets a timely return. + } + } + } + + /** + * Walks {@code root} (depth 5, covering {@code ///target/}) for + * Maven-written classpath files and aggregates their entries. + * + *

File-safety invariant. A candidate file is accepted ONLY if BOTH conditions hold: + * its direct parent directory is named {@code target} AND no ancestor segment between + * {@code root} and the candidate is in {@link LocalProjectClasspathProvider#HARVEST_SKIP_DIRS}. + * This gate keeps the harvester confined to Maven-owned build output even on projects that + * happen to contain a stray file with the matching name elsewhere in the tree. The harvester + * NEVER deletes; the next Maven run overwrites the file in place and {@code mvn clean} removes + * it. + */ + private static List harvestOutputFiles(Path root) throws IOException { + final Set entries = new LinkedHashSet<>(); + // File-safety: ONLY accept candidates whose direct parent directory is named `target`. + // Maven-owned build output, gitignored, wiped by `mvn clean`. A bare match on file name + // anywhere in the tree would risk picking up a file we did not create — see the + // file-safety policy. Depth-5 covers: ///target/. + try (Stream walk = Files.walk(root, 5)) { + walk.filter(p -> p.getFileName() != null + && OUTPUT_FILE_NAME.equals(p.getFileName().toString()) + && p.getParent() != null + && p.getParent().getFileName() != null + && "target".equals(p.getParent().getFileName().toString()) + && !hasSkippedAncestor(root, p)) + .forEach(file -> { + try { + final String content = Files.readString(file).trim(); + if (content.isEmpty()) { + log.debug("[LocalClasspath] Output file {} was empty", file); + return; + } + int parsed = 0; + // Use the HOST File.pathSeparator: Maven wrote this file on the host, so + // its entries are separated by the host's separator. Splitting on `[;:]` + // would shred `C:\foo` on Windows. + final String separator = Pattern.quote(File.pathSeparator); + for (String part : content.split(separator, -1)) { + final String trimmed = part.trim(); + if (!trimmed.isEmpty() && entries.add(trimmed)) { + parsed++; + } + } + log.debug("[LocalClasspath] Harvested {} new entries from {}", parsed, file); + // NO DELETION. The file is in a Maven-owned `target/` directory; the next + // Maven run overwrites it in place, and `mvn clean` removes it. Deleting + // arbitrary files in the project tree is forbidden by the file-safety policy. + } catch (IOException e) { + log.warn("[LocalClasspath] Could not read classpath output {}: {}: {}", + file, e.getClass().getSimpleName(), e.getMessage()); + } + }); + } + return new ArrayList<>(entries); + } + + /** + * Rejects candidates whose path between {@code root} (exclusive) and the candidate's parent + * (inclusive) contains a segment listed in + * {@link LocalProjectClasspathProvider#HARVEST_SKIP_DIRS}. Shares the provider's skip list so + * additions there extend both code paths uniformly. + */ + private static boolean hasSkippedAncestor(Path root, Path candidate) { + final Path relative; + try { + relative = root.relativize(candidate); + } catch (IllegalArgumentException e) { + // Different filesystem roots; cannot relativize. Fall back to skipping the candidate. + return true; + } + // Iterate every segment except the final file name — only directories count as ancestors. + final int segments = relative.getNameCount(); + for (int i = 0; i < segments - 1; i++) { + final String name = relative.getName(i).toString(); + if (LocalProjectClasspathProvider.HARVEST_SKIP_DIRS.contains(name)) { + return true; + } + } + return false; + } + + /** + * Runs the Maven command via {@link ProcessBuilder} with stderr merged into stdout. A daemon + * platform thread drains the merged stream into a {@link #STDOUT_CAPTURE_BYTES}-bounded buffer + * so the child never blocks on a full pipe and the WARN log on failure includes Maven's actual + * diagnostic output. Returns the process exit code, or {@code -1} if the wall-clock budget was + * exceeded (after {@link Process#destroyForcibly()}). On {@link InterruptedException}, kills + * the child, restores the interrupt flag, and rethrows so the caller can propagate cancellation. + */ + private static int executeRealCommand(CommandRequest req) throws IOException, InterruptedException { + final ProcessBuilder pb = new ProcessBuilder(req.command()) + .directory(req.workingDirectory().toFile()) + .redirectErrorStream(true); + final Process process = pb.start(); + // Drain stdout so the process never blocks on a full pipe buffer (a common cause of + // ProcessBuilder hangs). Capture the first STDOUT_CAPTURE_BYTES into a buffer so the WARN + // log on non-zero exit / timeout can include Maven's actual diagnostic output. The cap + // prevents a runaway log volume from a Maven misconfiguration. + // NOTE: a daemon platform thread is used here rather than a virtual thread because the + // project targets Java 17 (virtual threads were finalized in Java 21). The semantics are + // identical: "drain in the background, don't block the process pipe". + final ByteArrayOutputStream captured = new ByteArrayOutputStream(); + final Thread drainer = new Thread(() -> { + try (var in = process.getInputStream()) { + final byte[] buf = new byte[4096]; + int n; + while ((n = in.read(buf)) > 0) { + if (captured.size() < STDOUT_CAPTURE_BYTES) { + captured.write(buf, 0, Math.min(n, STDOUT_CAPTURE_BYTES - captured.size())); + } + // Continue reading even after the cap so the process never blocks on output. + } + } catch (IOException ignored) {} + }, "jdwp-mcp-maven-stdout-drainer"); + drainer.setDaemon(true); + drainer.start(); + try { + if (!process.waitFor(req.timeoutSeconds(), TimeUnit.SECONDS)) { + process.destroyForcibly(); + // Let any in-flight writes from the child reach the drainer before snapshotting + // the buffer: otherwise the WARN below would print a truncated tail. + try { + drainer.join(500); + } catch (InterruptedException ie) { + Thread.currentThread().interrupt(); + } + log.warn("[LocalClasspath] Maven invocation timed out after {}s. Captured output (up to {} bytes):\n{}", + req.timeoutSeconds(), STDOUT_CAPTURE_BYTES, + captured.toString(StandardCharsets.UTF_8)); + return -1; + } + } catch (InterruptedException ie) { + // waitFor was interrupted — kill the child, restore the interrupt flag, propagate so + // the outer run() returns an empty list and surfaces the cancellation to its caller. + process.destroyForcibly(); + Thread.currentThread().interrupt(); + throw ie; + } + drainer.join(1000); + if (drainer.isAlive()) { + // The drainer outlived its budget — a very large Maven output stream or a stuck child + // still holding the pipe open. We have already collected the exit code, so this is not + // a correctness issue, but a one-line DEBUG helps anyone investigating slow shutdowns. + log.debug("[LocalClasspath] stdout drainer still alive after 1s join — leaving as daemon"); + } + final int exit = process.exitValue(); + if (exit != 0) { + log.warn("[LocalClasspath] Maven exited with code {}. Captured output (up to {} bytes):\n{}", + exit, STDOUT_CAPTURE_BYTES, + captured.toString(StandardCharsets.UTF_8)); + } else { + log.debug("[LocalClasspath] Maven succeeded ({} bytes of output captured)", captured.size()); + } + return exit; + } + + /** Seam for the actual process execution; returns the child's exit code (or {@code -1} on timeout). */ + @FunctionalInterface + interface CommandExecutor { + int execute(CommandRequest request) throws Exception; + } + + /** Bundles the inputs the {@link CommandExecutor} needs so the seam stays a single-arg lambda. */ + record CommandRequest(List command, Path workingDirectory, int timeoutSeconds) {} +} diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsDiagnoseTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsDiagnoseTest.java index 81d6a9b..ec73972 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsDiagnoseTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsDiagnoseTest.java @@ -4,6 +4,7 @@ import one.edee.mcp.jdwp.discovery.JvmDescriptor; import one.edee.mcp.jdwp.discovery.JvmDiscoveryService; import one.edee.mcp.jdwp.evaluation.JdiExpressionEvaluator; +import one.edee.mcp.jdwp.evaluation.LocalProjectClasspathProvider; import one.edee.mcp.jdwp.watchers.WatcherManager; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; @@ -11,6 +12,7 @@ import org.mockito.ArgumentCaptor; import org.mockito.Mockito; +import java.nio.file.Path; import java.time.Instant; import java.util.List; @@ -261,4 +263,153 @@ void shouldPassConnectedHostAndPortToConfirmAll() { assertThat(hostCaptor.getValue()).isEqualTo("10.0.0.5"); assertThat(portCaptor.getValue()).isEqualTo(5050); } + + /** + * The local-classpath section answers two questions an agent asks when expression evaluation + * fails with "X cannot be resolved": (1) did the MCP server pick up the local project's + * classes/jars? and (2) which source(s) contributed? The test pins a stub provider with a known + * breakdown so we can assert the exact formatted output without shelling out to Maven. + */ + @Test + @DisplayName("local-classpath section renders header + per-source breakdown + env state (unset)") + void shouldRenderLocalClasspathSectionWithUnsetEnv() { + when(jdiService.getConnectionStatus()).thenReturn(new JDIConnectionService.ConnectionStatus( + false, null, 0, null, null + )); + final Path cwd = Path.of(System.getProperty("user.dir")); + final LocalProjectClasspathProvider stub = new LocalProjectClasspathProvider( + cwd, + name -> null, + (command, workingDirectory, timeoutSeconds) -> List.of( + "/tmp/dep-a.jar", "/tmp/dep-b.jar", "/tmp/dep-c.jar" + ) + ); + // Warm the breakdown cache so the diagnose path's non-blocking peek has something to render. + // Production warming happens on the first expression-evaluation flow, not on the diagnose + // path itself — see renderLocalClasspathBlock for why the diagnose call never blocks. + stub.discoverWithBreakdown(); + final JDWPTools toolsWithStub = JDWPToolsTestSupport.newTools( + jdiService, new BreakpointTracker(), new WatcherManager(), + mock(JdiExpressionEvaluator.class), new EventHistory(), new EvaluationGuard(), + discovery, stub); + + final String result = toolsWithStub.jdwp_diagnose(null); + + assertThat(result).contains("Local project classpath"); + assertThat(result).contains("(computed once per JDI connection"); + assertThat(result).contains("CWD: " + cwd); + // The stub forces Maven to contribute 3 jars; filesystem and env may legitimately add + // entries from the running build's tree but the maven count must be exactly 3. + assertThat(result).containsPattern("Sources: env-override=0, filesystem=\\d+, maven=3"); + assertThat(result).containsPattern("Total local entries: \\d+"); + assertThat(result).contains("Override env: JDWP_EXTRA_CLASSPATH (unset)"); + } + + /** + * Complementary case: when {@code JDWP_EXTRA_CLASSPATH} is set in the process environment, the + * diagnose output must say so. We don't poke {@link System#getenv} — the stub provider takes a + * lookup function so we can flip the bit cleanly. The env contribution count must reflect the + * stub's payload. + */ + @Test + @DisplayName("local-classpath section reflects JDWP_EXTRA_CLASSPATH when set") + void shouldRenderLocalClasspathSectionWithSetEnv() { + when(jdiService.getConnectionStatus()).thenReturn(new JDIConnectionService.ConnectionStatus( + false, null, 0, null, null + )); + final Path cwd = Path.of(System.getProperty("user.dir")); + final LocalProjectClasspathProvider stub = new LocalProjectClasspathProvider( + cwd, + name -> "JDWP_EXTRA_CLASSPATH".equals(name) + ? "/opt/extra1.jar" + java.io.File.pathSeparator + "/opt/extra2.jar" + : null, + (command, workingDirectory, timeoutSeconds) -> List.of() + ); + // Warm the cache so the non-blocking peek in the diagnose path has data to render. + stub.discoverWithBreakdown(); + final JDWPTools toolsWithStub = JDWPToolsTestSupport.newTools( + jdiService, new BreakpointTracker(), new WatcherManager(), + mock(JdiExpressionEvaluator.class), new EventHistory(), new EvaluationGuard(), + discovery, stub); + + final String result = toolsWithStub.jdwp_diagnose(null); + + assertThat(result).contains("Local project classpath"); + assertThat(result).containsPattern("Sources: env-override=2, filesystem=\\d+, maven=0"); + // Env state is read by the diagnose path from the same provider (or System.getenv) — when + // the provider says the env contributed >0 entries, the marker must read "(set)". + assertThat(result).contains("Override env: JDWP_EXTRA_CLASSPATH (set)"); + } + + /** + * The diagnose path must never block on a cold local-classpath cache. When the provider has + * never run discovery the renderer must call {@code peekCachedBreakdown()} (non-blocking), + * print a "not yet computed; will populate after first expression evaluation" hint, and return. + * Discovery happens organically when expression evaluation needs it. + */ + @Test + @DisplayName("does not block on a cold local-classpath cache — renders a 'not yet computed' hint instead") + void shouldRenderLocalClasspathSectionWithoutBlockingOnColdCache() { + when(jdiService.getConnectionStatus()).thenReturn(new JDIConnectionService.ConnectionStatus( + false, null, 0, null, null + )); + final Path cwd = Path.of(System.getProperty("user.dir")); + // Slow provider — sleeps 2s inside the Maven runner. If diagnose blocks, the call exceeds + // the 500ms budget; the fix path skips discovery entirely. + final LocalProjectClasspathProvider slowStub = new LocalProjectClasspathProvider( + cwd, + name -> null, + (command, workingDirectory, timeoutSeconds) -> { + try { Thread.sleep(2000); } catch (InterruptedException e) { Thread.currentThread().interrupt(); } + return List.of("/m2/slow.jar"); + } + ); + // Write a pom.xml-like fixture into a fresh temp dir so the slow Maven path is even reached. + final JDWPTools toolsWithSlowStub = JDWPToolsTestSupport.newTools( + jdiService, new BreakpointTracker(), new WatcherManager(), + mock(JdiExpressionEvaluator.class), new EventHistory(), new EvaluationGuard(), + discovery, slowStub); + + final long start = System.nanoTime(); + final String result = toolsWithSlowStub.jdwp_diagnose(null); + final long elapsedMs = (System.nanoTime() - start) / 1_000_000L; + + assertThat(result).contains("Local project classpath"); + assertThat(result).contains("not yet computed"); + assertThat(elapsedMs) + .as("diagnose must not block on cold-cache Maven discovery") + .isLessThan(500L); + } + + /** + * When {@code JDWP_EXTRA_CLASSPATH} is set in the environment but parses to zero entries (all + * blank tokens), the diagnose line must read {@code (set, no entries)} — not {@code (unset)}, + * which is indistinguishable from a truly absent env var. Three-way state: + * {@code (unset)} / {@code (set, no entries)} / {@code (set)}. + */ + @Test + @DisplayName("renders '(set, no entries)' when JDWP_EXTRA_CLASSPATH is set but parses to zero entries") + void shouldRenderSetNoEntriesWhenEnvIsSetButBlank() { + when(jdiService.getConnectionStatus()).thenReturn(new JDIConnectionService.ConnectionStatus( + false, null, 0, null, null + )); + final Path cwd = Path.of(System.getProperty("user.dir")); + final LocalProjectClasspathProvider stub = new LocalProjectClasspathProvider( + cwd, + name -> "JDWP_EXTRA_CLASSPATH".equals(name) ? "" : null, + (command, workingDirectory, timeoutSeconds) -> List.of() + ); + // Warm the cache so the diagnose path can see the 0-entry breakdown and distinguish + // "set but empty" from "unset" in its render. + stub.discoverWithBreakdown(); + final JDWPTools toolsWithStub = JDWPToolsTestSupport.newTools( + jdiService, new BreakpointTracker(), new WatcherManager(), + mock(JdiExpressionEvaluator.class), new EventHistory(), new EvaluationGuard(), + discovery, stub); + + final String result = toolsWithStub.jdwp_diagnose(null); + + assertThat(result).contains("Override env: JDWP_EXTRA_CLASSPATH (set, no entries)"); + assertThat(result).doesNotContain("Override env: JDWP_EXTRA_CLASSPATH (unset)"); + } } diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsTestSupport.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsTestSupport.java index b808680..42877ca 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsTestSupport.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsTestSupport.java @@ -2,9 +2,13 @@ import one.edee.mcp.jdwp.discovery.JvmDiscoveryService; import one.edee.mcp.jdwp.evaluation.JdiExpressionEvaluator; +import one.edee.mcp.jdwp.evaluation.LocalProjectClasspathProvider; import one.edee.mcp.jdwp.marks.MarkedInstanceRegistry; import one.edee.mcp.jdwp.watchers.WatcherManager; +import java.nio.file.Path; +import java.util.List; + import static org.mockito.Mockito.mock; /** @@ -45,7 +49,21 @@ static JDWPTools newToolsWithMocks() { new EvaluationGuard(), mock(JvmDiscoveryService.class), new MarkedInstanceRegistry(), - new JdiHealthMonitor() + new JdiHealthMonitor(), + defaultEmptyClasspathProvider() + ); + } + + /** + * Builds a deterministic {@link LocalProjectClasspathProvider} that never shells out to Maven + * or scans the filesystem. Used as the default for tests that do not exercise the local + * classpath surface — keeps the diagnose path fast and reproducible. + */ + static LocalProjectClasspathProvider defaultEmptyClasspathProvider() { + return new LocalProjectClasspathProvider( + Path.of(System.getProperty("user.dir")), + name -> null, + (command, workingDirectory, timeoutSeconds) -> List.of() ); } @@ -72,7 +90,37 @@ static JDWPTools newTools( evaluationGuard, jvmDiscoveryService, new MarkedInstanceRegistry(), - new JdiHealthMonitor() + new JdiHealthMonitor(), + defaultEmptyClasspathProvider() + ); + } + + /** + * Overload that accepts a caller-supplied {@link LocalProjectClasspathProvider} — needed by + * tests that verify the local-classpath diagnose section. Defaults the other "extra" seams + * (MarkedInstanceRegistry, JdiHealthMonitor) to fresh instances. + */ + static JDWPTools newTools( + JDIConnectionService jdiService, + BreakpointTracker breakpointTracker, + WatcherManager watcherManager, + JdiExpressionEvaluator expressionEvaluator, + EventHistory eventHistory, + EvaluationGuard evaluationGuard, + JvmDiscoveryService jvmDiscoveryService, + LocalProjectClasspathProvider localClasspathProvider + ) { + return new JDWPTools( + jdiService, + breakpointTracker, + watcherManager, + expressionEvaluator, + eventHistory, + evaluationGuard, + jvmDiscoveryService, + new MarkedInstanceRegistry(), + new JdiHealthMonitor(), + localClasspathProvider ); } @@ -99,7 +147,8 @@ static JDWPTools newTools( evaluationGuard, jvmDiscoveryService, markedInstances, - new JdiHealthMonitor() + new JdiHealthMonitor(), + defaultEmptyClasspathProvider() ); } @@ -127,7 +176,8 @@ static JDWPTools newTools( evaluationGuard, jvmDiscoveryService, markedInstances, - healthMonitor + healthMonitor, + defaultEmptyClasspathProvider() ); } } diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorAbsentLocalsTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorAbsentLocalsTest.java index d0ba755..cd40853 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorAbsentLocalsTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorAbsentLocalsTest.java @@ -53,7 +53,9 @@ class JdiExpressionEvaluatorAbsentLocalsTest { void setUp() throws Exception { compiler = mock(InMemoryJavaCompiler.class); final JDIConnectionService jdiService = mock(JDIConnectionService.class); - evaluator = new JdiExpressionEvaluator(compiler, jdiService, new EvaluationGuard()); + evaluator = new JdiExpressionEvaluator( + compiler, jdiService, new EvaluationGuard(), + LocalProjectClasspathProviderTestSupport.noOpProvider()); // Echo back the requested class name so the post-compile bytecode lookup hits. when(compiler.compile(anyString(), anyString())) .thenAnswer(inv -> Map.of(inv.getArgument(0, String.class), new byte[]{1, 2, 3})); diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorConfigureClasspathTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorConfigureClasspathTest.java index b061048..1ad8cd9 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorConfigureClasspathTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorConfigureClasspathTest.java @@ -13,7 +13,6 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; -import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -22,7 +21,7 @@ * {@link JdiExpressionEvaluator#configureCompilerClasspath(ThreadReference)} and * {@link JdiExpressionEvaluator#prewarmClasspath(ThreadReference)}. * - *

Covers the fixes for the classpath-warming lifecycle (GH #30): a failed discovery must reset + *

Covers the classpath-warming lifecycle contract: a failed discovery must reset * the compiler (so no stale config survives), surface an actionable exception from the strict path, * and stay silent on the best-effort pre-warm path. */ @@ -37,7 +36,12 @@ class JdiExpressionEvaluatorConfigureClasspathTest { void setUp() { compiler = mock(InMemoryJavaCompiler.class); jdiService = mock(JDIConnectionService.class); - evaluator = new JdiExpressionEvaluator(compiler, jdiService, new EvaluationGuard()); + // No-op local-classpath provider — these tests only care about the remote-side reset/configure + // semantics; LocalProjectClasspathProviderTestSupport supplies a provider that never touches + // the real environment, filesystem, or Maven. + evaluator = new JdiExpressionEvaluator( + compiler, jdiService, new EvaluationGuard(), + LocalProjectClasspathProviderTestSupport.noOpProvider()); thread = mock(ThreadReference.class); when(thread.uniqueID()).thenReturn(42L); } diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorGetDeclaredTypeTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorGetDeclaredTypeTest.java index c6fa2b3..ae0924c 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorGetDeclaredTypeTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorGetDeclaredTypeTest.java @@ -41,7 +41,9 @@ class JdiExpressionEvaluatorGetDeclaredTypeTest { void setUp() { InMemoryJavaCompiler compiler = mock(InMemoryJavaCompiler.class); JDIConnectionService jdiService = mock(JDIConnectionService.class); - evaluator = new JdiExpressionEvaluator(compiler, jdiService, new EvaluationGuard()); + evaluator = new JdiExpressionEvaluator( + compiler, jdiService, new EvaluationGuard(), + LocalProjectClasspathProviderTestSupport.noOpProvider()); } @Nested diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorLocalClasspathUnionTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorLocalClasspathUnionTest.java new file mode 100644 index 0000000..e0b5e66 --- /dev/null +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorLocalClasspathUnionTest.java @@ -0,0 +1,193 @@ +package one.edee.mcp.jdwp.evaluation; + +import com.sun.jdi.ThreadReference; +import one.edee.mcp.jdwp.EvaluationGuard; +import one.edee.mcp.jdwp.JDIConnectionService; +import one.edee.mcp.jdwp.evaluation.exceptions.JdiEvaluationException; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; + +import java.io.File; +import java.nio.file.Path; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Covers the {@link JdiExpressionEvaluator#configureCompilerClasspath(ThreadReference)} union + * semantics introduced when the local-project classpath fallback was wired in: + * the compiler classpath received by {@link InMemoryJavaCompiler#configure} is the merger of + * the remote (target VM) classpath and the local-project entries — remote-first, deduped, + * joined with {@link File#pathSeparator}. + * + *

The local entries must NOT replace the remote ones; they must augment them, so live + * target VM JAR locations continue to win on JDT type resolution and local entries only fill + * gaps the remote classloader walk could not see (Tomcat / Spring Boot dev-tools / custom + * URLClassLoaders that hide their JARs from {@code getURLs()}). + */ +@DisplayName("JdiExpressionEvaluator — remote+local classpath union") +class JdiExpressionEvaluatorLocalClasspathUnionTest { + + private InMemoryJavaCompiler compiler; + private JDIConnectionService jdiService; + private ThreadReference thread; + + @BeforeEach + void setUp() { + compiler = mock(InMemoryJavaCompiler.class); + jdiService = mock(JDIConnectionService.class); + thread = mock(ThreadReference.class); + when(thread.uniqueID()).thenReturn(42L); + } + + @Test + @DisplayName("compiler receives the union of remote and local entries — remote first, deduped") + void shouldFeedCompilerTheUnionOfRemoteAndLocalEntries() throws Exception { + // First call (guard) returns null → proceed; second call (post-discovery) returns the path + // so the configure-success branch is taken. + when(jdiService.getDiscoveredJdkPath()).thenReturn(null, "/opt/jdk"); + when(jdiService.discoverClasspath(thread)).thenReturn("/r1:/r2"); + when(jdiService.getTargetMajorVersion()).thenReturn(21); + + // Local provider contributes /r1 (overlapping with remote — must be deduped) plus two + // new entries. The remote/local separator handling is host-agnostic (the prod code + // detects `;` vs `:` from the remote content), and the JOINED output uses the host + // File.pathSeparator. + final Set localEntries = new LinkedHashSet<>(List.of("/r1", "/local1", "/local2")); + final LocalProjectClasspathProvider local = stubLocalProvider(localEntries); + + final JdiExpressionEvaluator evaluator = new JdiExpressionEvaluator( + compiler, jdiService, new EvaluationGuard(), local); + + evaluator.configureCompilerClasspath(thread); + + final ArgumentCaptor cpCaptor = ArgumentCaptor.forClass(String.class); + verify(compiler).configure(eq("/opt/jdk"), cpCaptor.capture(), eq(21)); + + final List entries = List.of(cpCaptor.getValue().split(java.util.regex.Pattern.quote(File.pathSeparator), -1)); + // Remote-first, /r1 deduped (appears once), local-only entries appended in the order + // the provider returned them. + assertThat(entries).containsExactly("/r1", "/r2", "/local1", "/local2"); + } + + @Test + @DisplayName("local classpath is still used when remote discovery returns empty") + void shouldUseLocalClasspathWhenRemoteDiscoveryReturnsEmpty() throws Exception { + // Remote discovery yields null classpath but JDK was discovered → previously this would + // throw "classpath could not be determined"; the new behaviour is that local entries + // alone are enough to keep evaluation alive. + when(jdiService.getDiscoveredJdkPath()).thenReturn(null, "/opt/jdk"); + when(jdiService.discoverClasspath(thread)).thenReturn(null); + when(jdiService.getTargetMajorVersion()).thenReturn(21); + + final Set localEntries = new LinkedHashSet<>(List.of("/local/a", "/local/b")); + final LocalProjectClasspathProvider local = stubLocalProvider(localEntries); + + final JdiExpressionEvaluator evaluator = new JdiExpressionEvaluator( + compiler, jdiService, new EvaluationGuard(), local); + + assertThatCode(() -> evaluator.configureCompilerClasspath(thread)).doesNotThrowAnyException(); + + final ArgumentCaptor cpCaptor = ArgumentCaptor.forClass(String.class); + verify(compiler).configure(eq("/opt/jdk"), cpCaptor.capture(), anyInt()); + final List entries = List.of(cpCaptor.getValue().split(java.util.regex.Pattern.quote(File.pathSeparator), -1)); + assertThat(entries).containsExactly("/local/a", "/local/b"); + } + + /** + * Separator detection must not shred a single-entry Windows-style remote path. A bare + * content-based heuristic ({@code remote.contains(";")}) treats {@code "C:\foo"} as colon-separated + * and breaks the drive letter into {@code "C"} + {@code "\foo"}. The provider must use the host + * {@link File#pathSeparator} for both detection and splitting. + */ + @Test + @DisplayName("does not shred a single-entry Windows-style remote path on `:` separator") + void shouldNotShredSingleEntryWindowsPath() throws Exception { + when(jdiService.getDiscoveredJdkPath()).thenReturn(null, "/opt/jdk"); + when(jdiService.discoverClasspath(thread)).thenReturn("C:\\foo"); + when(jdiService.getTargetMajorVersion()).thenReturn(21); + + final LocalProjectClasspathProvider local = stubLocalProvider(Set.of()); + + final JdiExpressionEvaluator evaluator = new JdiExpressionEvaluator( + compiler, jdiService, new EvaluationGuard(), local); + + evaluator.configureCompilerClasspath(thread); + + final ArgumentCaptor cpCaptor = ArgumentCaptor.forClass(String.class); + verify(compiler).configure(eq("/opt/jdk"), cpCaptor.capture(), anyInt()); + + final String merged = cpCaptor.getValue(); + // The merged classpath must contain the original Windows entry verbatim — `C:\foo` survives + // the host-separator join because the provider's separator-detection treats a single + // drive-letter path as one entry. Asserting on the merged string (not on entries split by + // the host pathSeparator) keeps the check meaningful on non-Windows hosts where the host + // separator IS `:` and would shred the value at the assertion side. + assertThat(merged) + .as("single-entry Windows path must not be shredded on the `:` heuristic") + .isEqualTo("C:\\foo"); + } + + /** + * When both the remote classpath is empty/null and the local provider contributes zero entries, + * {@code configureCompilerClasspath} throws an actionable {@link JdiEvaluationException} whose + * message embeds the working directory. If {@code user.dir} has been cleared (some sandboxed / + * restricted JVM launch options do this), the message must fall back to a placeholder instead + * of containing the literal string {@code "null"}. + */ + @Test + @DisplayName("does not embed the literal string 'null' when user.dir is unset") + void shouldNotEmitLiteralNullInExceptionMessage() throws Exception { + when(jdiService.getDiscoveredJdkPath()).thenReturn(null, "/opt/jdk"); + when(jdiService.discoverClasspath(thread)).thenReturn(null); + when(jdiService.getTargetMajorVersion()).thenReturn(21); + + final LocalProjectClasspathProvider local = stubLocalProvider(Set.of()); + + final JdiExpressionEvaluator evaluator = new JdiExpressionEvaluator( + compiler, jdiService, new EvaluationGuard(), local); + + final String savedUserDir = System.getProperty("user.dir"); + System.clearProperty("user.dir"); + try { + assertThatCode(() -> evaluator.configureCompilerClasspath(thread)) + .isInstanceOf(JdiEvaluationException.class) + .satisfies(e -> assertThat(e.getMessage()) + .as("exception message must not embed the literal string \"null\"") + .doesNotContain("directory 'null'")); + } finally { + if (savedUserDir != null) { + System.setProperty("user.dir", savedUserDir); + } + } + } + + /** + * Builds a seam-taking {@link LocalProjectClasspathProvider} whose {@code discover()} returns + * the fixed entry set without touching the real environment, filesystem, or Maven. Uses the + * env-lookup seam to inject the entries via the {@code JDWP_EXTRA_CLASSPATH} pathway — this + * keeps the test self-contained (no need to mock the public synchronized method). + */ + private static LocalProjectClasspathProvider stubLocalProvider(Set entries) { + // Build a JDWP_EXTRA_CLASSPATH-style value joined with the host File.pathSeparator — + // this is the exact format the provider's env-override path expects. + final String joined = String.join(File.pathSeparator, entries); + return new LocalProjectClasspathProvider( + // Working directory is a temp-ish path with no pom.xml / target/, so the filesystem and + // Maven sources contribute nothing and the env-override path supplies every entry. + Path.of(System.getProperty("java.io.tmpdir")), + name -> "JDWP_EXTRA_CLASSPATH".equals(name) ? joined : null, + (c, d, t) -> List.of() + ); + } +} diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorPackageFallbackTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorPackageFallbackTest.java index d20b417..2f815ed 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorPackageFallbackTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorPackageFallbackTest.java @@ -47,7 +47,9 @@ class JdiExpressionEvaluatorPackageFallbackTest { void setUp() throws Exception { compiler = mock(InMemoryJavaCompiler.class); final JDIConnectionService jdiService = mock(JDIConnectionService.class); - evaluator = new JdiExpressionEvaluator(compiler, jdiService, new EvaluationGuard()); + evaluator = new JdiExpressionEvaluator( + compiler, jdiService, new EvaluationGuard(), + LocalProjectClasspathProviderTestSupport.noOpProvider()); // Compiler is mocked — echo back the requested class name so the bytecode lookup hits. when(compiler.compile(anyString(), anyString())) .thenAnswer(inv -> Map.of(inv.getArgument(0, String.class), new byte[]{1, 2, 3})); diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderCacheTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderCacheTest.java new file mode 100644 index 0000000..4f5d121 --- /dev/null +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderCacheTest.java @@ -0,0 +1,182 @@ +package one.edee.mcp.jdwp.evaluation; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Tests for the memoisation, reset semantics, per-source breakdown shape, immutability of the + * cached view, and concurrent-access safety of {@link LocalProjectClasspathProvider}. + */ +@DisplayName("LocalProjectClasspathProvider — caching and breakdown") +class LocalProjectClasspathProviderCacheTest { + + @Test + @DisplayName("subsequent discover() and discoverWithBreakdown() calls reuse the cached result") + void shouldReuseCachedResultOnSubsequentCalls(@TempDir Path tmp) throws Exception { + Files.writeString(tmp.resolve("pom.xml"), ""); + final AtomicInteger mavenInvocations = new AtomicInteger(); + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, + (cmd, cwd, t) -> { mavenInvocations.incrementAndGet(); return List.of("/m2/foo.jar"); } + ); + + provider.discover(); + provider.discover(); + provider.discoverWithBreakdown(); + + assertThat(mavenInvocations.get()).isEqualTo(1); + } + + @Test + @DisplayName("reset() clears the cache so the next discover() recomputes from sources") + void shouldClearCacheAndForceRecomputationOnReset(@TempDir Path tmp) throws Exception { + Files.writeString(tmp.resolve("pom.xml"), ""); + final AtomicInteger mavenInvocations = new AtomicInteger(); + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, + (cmd, cwd, t) -> { mavenInvocations.incrementAndGet(); return List.of("/m2/foo.jar"); } + ); + + provider.discover(); + provider.reset(); + provider.discover(); + + assertThat(mavenInvocations.get()).isEqualTo(2); + } + + @Test + @DisplayName("breakdown reports per-source entry counts and a total matching the union") + void shouldReportPerSourceCountsInBreakdown(@TempDir Path tmp) throws Exception { + Files.createDirectories(tmp.resolve("target/classes")); + Files.writeString(tmp.resolve("pom.xml"), ""); + + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, + envName -> "JDWP_EXTRA_CLASSPATH".equals(envName) + ? "/opt/extra.jar" + : null, + (cmd, cwd, t) -> List.of("/m2/a.jar", "/m2/b.jar") + ); + + final var breakdown = provider.discoverWithBreakdown(); + + assertThat(breakdown.envOverride()).isEqualTo(1); + assertThat(breakdown.filesystem()).isEqualTo(1); + assertThat(breakdown.maven()).isEqualTo(2); + assertThat(breakdown.all()).hasSize(4); + } + + /** + * Concurrent callers reaching {@link LocalProjectClasspathProvider#discover()} from the eval + * path and the diagnose path must not race the underlying Maven invocation. Memoisation + + * synchronisation means Maven runs at most once per cache lifetime even under contention. + */ + @Test + @DisplayName("concurrent discover() invocations trigger Maven at most once") + void shouldInvokeMavenAtMostOnceUnderConcurrentDiscovery(@TempDir Path tmp) throws Exception { + Files.writeString(tmp.resolve("pom.xml"), ""); + final AtomicInteger mavenInvocations = new AtomicInteger(); + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, + (cmd, cwd, t) -> { + mavenInvocations.incrementAndGet(); + try { Thread.sleep(50); } catch (InterruptedException e) { Thread.currentThread().interrupt(); } + return List.of("/m2/foo.jar"); + } + ); + + final int threadCount = 8; + final var pool = java.util.concurrent.Executors.newFixedThreadPool(threadCount); + try { + final var futures = new java.util.ArrayList>(); + for (int i = 0; i < threadCount; i++) { + futures.add(pool.submit(provider::discover)); + } + for (var f : futures) f.get(); + } finally { + pool.shutdown(); + } + + assertThat(mavenInvocations.get()).isEqualTo(1); + } + + /** + * The cached breakdown's {@code all()} set must be unmodifiable so consumers cannot poison the + * cache by mutating the returned view. The provider wraps a defensive LinkedHashSet copy in + * {@link java.util.Collections#unmodifiableSet} — pin the invariant. + */ + @Test + @DisplayName("the cached breakdown.all() view is unmodifiable") + void shouldExposeUnmodifiableEntrySetInBreakdown(@TempDir Path tmp) throws Exception { + Files.writeString(tmp.resolve("pom.xml"), ""); + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, + (cmd, cwd, t) -> List.of("/m2/foo.jar") + ); + + final var breakdown = provider.discoverWithBreakdown(); + + assertThat(breakdown.all()) + .as("cached breakdown view must be unmodifiable") + .isUnmodifiable(); + } + + /** + * The cached breakdown is a value object — the second call must return the SAME reference as + * the first so callers can identity-compare to detect a cache hit without measuring elapsed + * time. This pins the memoisation seam more strictly than the "invocation count == 1" test. + */ + @Test + @DisplayName("the second discoverWithBreakdown() returns the same Breakdown reference as the first") + void shouldReturnSameBreakdownReferenceOnSecondCall(@TempDir Path tmp) throws Exception { + Files.writeString(tmp.resolve("pom.xml"), ""); + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, + (cmd, cwd, t) -> List.of("/m2/foo.jar") + ); + + final var first = provider.discoverWithBreakdown(); + final var second = provider.discoverWithBreakdown(); + + assertThat(second).isSameAs(first); + } + + /** + * The non-blocking peek seam used by the diagnose renderer must return null before any + * discovery has run — otherwise the renderer cannot tell "cold cache, do not block" from + * "warm cache with zero entries". + */ + @Test + @DisplayName("peekCachedBreakdown() returns null before the first discovery") + void shouldReturnNullFromPeekBeforeDiscovery(@TempDir Path tmp) { + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, (cmd, cwd, t) -> List.of() + ); + + assertThat(provider.peekCachedBreakdown()).isNull(); + } + + @Test + @DisplayName("peekCachedBreakdown() returns the populated breakdown after discovery has run") + void shouldReturnPopulatedBreakdownFromPeekAfterDiscovery(@TempDir Path tmp) throws Exception { + Files.writeString(tmp.resolve("pom.xml"), ""); + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, + (cmd, cwd, t) -> List.of("/m2/foo.jar") + ); + + provider.discover(); + + assertThat(provider.peekCachedBreakdown()) + .isNotNull() + .satisfies(b -> assertThat(b.maven()).isEqualTo(1)); + } +} diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderEnvOverrideTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderEnvOverrideTest.java new file mode 100644 index 0000000..7fb808f --- /dev/null +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderEnvOverrideTest.java @@ -0,0 +1,100 @@ +package one.edee.mcp.jdwp.evaluation; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import java.nio.file.Path; +import java.util.List; +import java.util.Set; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Tests for the {@code JDWP_EXTRA_CLASSPATH} env-override source of + * {@link LocalProjectClasspathProvider}. Pins the parser's behaviour on the host + * {@link java.io.File#pathSeparator}, on whitespace and blank-entry trimming, and on the + * special "set but empty" case that the diagnose path treats differently from "unset". + */ +@DisplayName("LocalProjectClasspathProvider — JDWP_EXTRA_CLASSPATH env override") +class LocalProjectClasspathProviderEnvOverrideTest { + + private static final String SEP = java.io.File.pathSeparator; + + @Test + @DisplayName("parses File.pathSeparator-delimited env var into insertion-ordered entries") + void shouldParsePathSeparatorDelimitedEnvVarIntoOrderedEntries() { + // Use File.pathSeparator so the test is correct on both Linux (`:`) and Windows (`;`). + // Splitting on `[;:]` is broken on Windows because `C:\foo` contains a colon — the env + // var is parsed by the MCP server on the HOST OS, so the host separator is the right one. + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + Path.of("/tmp/no-such-project"), + envName -> "JDWP_EXTRA_CLASSPATH".equals(envName) + ? "/opt/libs/a.jar" + SEP + "/opt/libs/b.jar" + SEP + "/srv/classes" + : null, + (cmd, cwd, timeoutSeconds) -> List.of() // no-op Maven runner + ); + + final Set entries = provider.discover(); + + assertThat(entries).containsExactly( + "/opt/libs/a.jar", "/opt/libs/b.jar", "/srv/classes" + ); + } + + @Test + @DisplayName("returns empty set when env var is an empty string") + void shouldReturnEmptySetWhenEnvVarIsEmpty() { + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + Path.of("/tmp/no-such-project"), + envName -> "", + (cmd, cwd, timeoutSeconds) -> List.of() + ); + + assertThat(provider.discover()).isEmpty(); + } + + @Test + @DisplayName("returns empty set when env lookup returns null (unset)") + void shouldReturnEmptySetWhenEnvVarIsUnset() { + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + Path.of("/tmp/no-such-project"), + envName -> null, + (cmd, cwd, timeoutSeconds) -> List.of() + ); + + assertThat(provider.discover()).isEmpty(); + } + + @Test + @DisplayName("trims whitespace around entries and drops empty/blank tokens") + void shouldTrimWhitespaceAndDropBlankEntries() { + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + Path.of("/tmp/no-such-project"), + envName -> " /a.jar " + SEP + SEP + " /b.jar ", + (cmd, cwd, timeoutSeconds) -> List.of() + ); + + assertThat(provider.discover()).containsExactly("/a.jar", "/b.jar"); + } + + /** + * Even when {@code JDWP_EXTRA_CLASSPATH} is set but parses to zero usable entries (e.g. all + * tokens were blank), the breakdown must report it as a 0-count source. The diagnose path + * downstream is supposed to differentiate "set but empty" from "unset" — pinning the count + * here is the cleanest seam to that rendering. + */ + @Test + @DisplayName("breakdown reports zero env-override entries when env is set but only blanks") + void shouldReportZeroEnvEntriesWhenEnvIsBlanksOnly() { + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + Path.of("/tmp/no-such-project"), + envName -> " " + SEP + " " + SEP + " ", + (cmd, cwd, timeoutSeconds) -> List.of() + ); + + final LocalProjectClasspathProvider.Breakdown breakdown = provider.discoverWithBreakdown(); + + assertThat(breakdown.envOverride()).isZero(); + assertThat(breakdown.all()).isEmpty(); + } +} diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderFilesystemScanTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderFilesystemScanTest.java new file mode 100644 index 0000000..55680a4 --- /dev/null +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderFilesystemScanTest.java @@ -0,0 +1,170 @@ +package one.edee.mcp.jdwp.evaluation; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Set; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Tests for the filesystem-scan source of {@link LocalProjectClasspathProvider}. Pins the + * inclusive depth-5 boundary, the {@code SKIP_DIRS} blacklist, the "module classes regardless of + * sub-modules" probe, and the independence of {@code target/classes} from {@code target/test-classes}. + */ +@DisplayName("LocalProjectClasspathProvider — filesystem scan for target/classes") +class LocalProjectClasspathProviderFilesystemScanTest { + + @Test + @DisplayName("picks up target/classes and target/test-classes at the working-directory root") + void shouldPickUpTargetClassesAtCwdRoot(@TempDir Path tmp) throws Exception { + Files.createDirectories(tmp.resolve("target/classes")); + Files.createDirectories(tmp.resolve("target/test-classes")); + + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, (cmd, cwd, t) -> List.of() + ); + + final Set entries = provider.discover(); + + assertThat(entries).contains( + tmp.resolve("target/classes").toString(), + tmp.resolve("target/test-classes").toString() + ); + } + + @Test + @DisplayName("walks into reactor modules up to depth 5; entries deeper than depth 5 are excluded") + void shouldWalkIntoReactorModulesUpToDepthFive(@TempDir Path tmp) throws Exception { + // Real-world reactors are deeper than 3 — e.g. parent/group/subgroup/module/target/classes + // is depth 4 from CWD. Depth 5 matches the harvester and covers the layouts seen in + // projects like Camel / Hadoop / Eclipse. Cost stays bounded by SKIP_DIRS. + Files.createDirectories(tmp.resolve("module-a/target/classes")); + Files.createDirectories(tmp.resolve("group/subgroup/module/target/classes")); + // depth 6 — must NOT be picked up to avoid runaway scans + Files.createDirectories(tmp.resolve("a/b/c/d/e/module/target/classes")); + + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, (cmd, cwd, t) -> List.of() + ); + + final Set entries = provider.discover(); + + assertThat(entries) + .contains(tmp.resolve("module-a/target/classes").toString()) + .contains(tmp.resolve("group/subgroup/module/target/classes").toString()) + .doesNotContain(tmp.resolve("a/b/c/d/e/module/target/classes").toString()); + } + + /** + * Boundary test that nails the INCLUSIVE side of MAX_SCAN_DEPTH = 5: with the scan starting at + * depth 0 on the CWD itself and {@code depth == MAX_SCAN_DEPTH} permitted to probe but not + * recurse, a target/classes whose module directory is 5 levels under CWD is the deepest one + * the scanner is required to find. The complementary "depth 6" test pins exclusion just past + * the boundary so a future off-by-one tightening (`>` → `>=`) trips immediately. + */ + @Test + @DisplayName("picks up target/classes at the inclusive depth-5 boundary") + void shouldPickUpTargetClassesAtInclusiveDepthFiveBoundary(@TempDir Path tmp) throws Exception { + // tmp (depth 0) → a (1) → b (2) → c (3) → d (4) → e (5) → target/classes probed here. + final Path edge = tmp.resolve("a/b/c/d/e/target/classes"); + Files.createDirectories(edge); + + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, (cmd, cwd, t) -> List.of() + ); + + assertThat(provider.discover()).contains(edge.toString()); + } + + @Test + @DisplayName("skips hidden directories and noise directories on the scan path") + void shouldSkipHiddenAndNoiseDirectories(@TempDir Path tmp) throws Exception { + Files.createDirectories(tmp.resolve("module-a/target/classes")); + Files.createDirectories(tmp.resolve(".git/target/classes")); + Files.createDirectories(tmp.resolve("node_modules/foo/target/classes")); + + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, (cmd, cwd, t) -> List.of() + ); + + assertThat(provider.discover()) + .contains(tmp.resolve("module-a/target/classes").toString()) + .doesNotContain(tmp.resolve(".git/target/classes").toString()) + .doesNotContain(tmp.resolve("node_modules/foo/target/classes").toString()); + } + + /** + * Parameterised sweep across every {@code SKIP_DIRS} entry. Even though three of these names + * also start with `.` and would be filtered by the hidden-name guard, the scan code is + * documented to honour SKIP_DIRS independently. Pinning each name keeps the rule observable. + */ + @ParameterizedTest(name = "skips ''{0}'' subtree even when it contains a valid target/classes") + @ValueSource(strings = {".git", ".idea", ".vscode", "node_modules", ".gradle", ".mvn"}) + @DisplayName("every SKIP_DIRS entry hides target/classes beneath it from the scan") + void shouldSkipEverySkipDirsEntry(String skipDir, @TempDir Path tmp) throws Exception { + Files.createDirectories(tmp.resolve(skipDir + "/module/target/classes")); + Files.createDirectories(tmp.resolve("visible/target/classes")); + + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, (cmd, cwd, t) -> List.of() + ); + + final Set entries = provider.discover(); + + assertThat(entries) + .contains(tmp.resolve("visible/target/classes").toString()) + .doesNotContain(tmp.resolve(skipDir + "/module/target/classes").toString()); + } + + /** + * The scanner skips nested {@code target/} directories — once we enter a {@code target/} we + * never recurse into a {@code target/foo/target/classes}. Anything nested inside the build + * output belongs to Maven and must not contribute to the project's source classpath. + */ + @Test + @DisplayName("does not recurse into a nested target/ inside another target/") + void shouldNotRecurseIntoNestedTargetInsideTarget(@TempDir Path tmp) throws Exception { + Files.createDirectories(tmp.resolve("target/classes")); + Files.createDirectories(tmp.resolve("target/sub/target/classes")); + + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, (cmd, cwd, t) -> List.of() + ); + + final Set entries = provider.discover(); + + assertThat(entries).contains(tmp.resolve("target/classes").toString()); + assertThat(entries).doesNotContain(tmp.resolve("target/sub/target/classes").toString()); + } + + /** + * A module with only {@code target/classes} (no {@code target/test-classes}) must still + * contribute its main classes — and vice versa. The two probes are independent so a + * test-classes-only module (e.g. a fixtures jar) is also picked up. + */ + @Test + @DisplayName("target/classes and target/test-classes contribute independently") + void shouldContributeTargetClassesAndTestClassesIndependently(@TempDir Path tmp) throws Exception { + Files.createDirectories(tmp.resolve("main-only/target/classes")); + Files.createDirectories(tmp.resolve("test-only/target/test-classes")); + + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, (cmd, cwd, t) -> List.of() + ); + + final Set entries = provider.discover(); + + assertThat(entries) + .contains(tmp.resolve("main-only/target/classes").toString()) + .contains(tmp.resolve("test-only/target/test-classes").toString()) + .doesNotContain(tmp.resolve("main-only/target/test-classes").toString()) + .doesNotContain(tmp.resolve("test-only/target/classes").toString()); + } +} diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderMavenTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderMavenTest.java new file mode 100644 index 0000000..7255f4e --- /dev/null +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderMavenTest.java @@ -0,0 +1,165 @@ +package one.edee.mcp.jdwp.evaluation; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Tests for the Maven {@code dependency:build-classpath} source of + * {@link LocalProjectClasspathProvider}. Pins the command-line shape (quiet mode, runtime scope, + * output file under {@code target/}), the {@code mvnw} preference rule, the silent-skip when no + * {@code pom.xml} is present, and the timeout the runner is invoked with. + */ +@DisplayName("LocalProjectClasspathProvider — Maven dependency:build-classpath") +class LocalProjectClasspathProviderMavenTest { + + @Test + @DisplayName("invokes Maven dependency:build-classpath when pom.xml is at the working directory") + void shouldInvokeMavenWhenPomXmlExists(@TempDir Path tmp) throws Exception { + Files.writeString(tmp.resolve("pom.xml"), ""); + + final AtomicReference> capturedCmd = new AtomicReference<>(); + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, + envName -> null, + (cmd, cwd, t) -> { + capturedCmd.set(cmd); + return List.of( + "/home/user/.m2/repository/org/foo/foo-1.0.jar", + "/home/user/.m2/repository/org/bar/bar-2.0.jar" + ); + } + ); + + final Set entries = provider.discover(); + + assertThat(capturedCmd.get()).anyMatch(s -> s.contains("dependency:build-classpath")); + // SAFETY: the output file MUST land under target/ (Maven-owned) — never bare in module root. + // This assertion enforces the file-safety policy at the design level: if anyone changes + // the flag to land elsewhere, this test fails. + assertThat(capturedCmd.get()) + .as("mdep.outputFile must point under target/ — file-safety policy") + .anyMatch(s -> s.equals("-Dmdep.outputFile=target/.jdwp-mcp-classpath")); + assertThat(entries).contains( + "/home/user/.m2/repository/org/foo/foo-1.0.jar", + "/home/user/.m2/repository/org/bar/bar-2.0.jar" + ); + } + + /** + * Quiet mode (-q) is essential: Maven's default output is verbose enough to drown the MCP + * server's diagnostic stream. The runner's stdout capture keeps a size-capped buffer for + * the WARN log on failure, so the quiet flag does not blind operators when something breaks. + */ + @Test + @DisplayName("Maven command includes -q (quiet) to suppress Maven CLI noise") + void shouldPassQuietFlagToMaven(@TempDir Path tmp) throws Exception { + Files.writeString(tmp.resolve("pom.xml"), ""); + + final AtomicReference> capturedCmd = new AtomicReference<>(); + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, (cmd, cwd, t) -> { capturedCmd.set(cmd); return List.of(); } + ); + provider.discover(); + + assertThat(capturedCmd.get()).contains("-q"); + } + + /** + * {@code -DincludeScope=runtime} matches what is actually on the app's runtime classpath at + * the breakpoint — Maven's default would include test-scope deps too, which routinely + * differ from what the target VM actually sees. + */ + @Test + @DisplayName("Maven command pins -DincludeScope=runtime to match the target VM's runtime classpath") + void shouldPassIncludeScopeRuntimeToMaven(@TempDir Path tmp) throws Exception { + Files.writeString(tmp.resolve("pom.xml"), ""); + + final AtomicReference> capturedCmd = new AtomicReference<>(); + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, (cmd, cwd, t) -> { capturedCmd.set(cmd); return List.of(); } + ); + provider.discover(); + + assertThat(capturedCmd.get()).contains("-DincludeScope=runtime"); + } + + /** + * Cold-cache Maven invocations may need to download dependencies on first run, which routinely + * takes 1-3 minutes. The provider must hand the runner a 180s timeout that accommodates the + * common cold case rather than a shorter default that would fire spuriously on a fresh machine. + */ + @Test + @DisplayName("Maven runner is invoked with a 180-second timeout (cold-cache safety margin)") + void shouldInvokeMavenRunnerWithA180SecondTimeout(@TempDir Path tmp) throws Exception { + Files.writeString(tmp.resolve("pom.xml"), ""); + + final AtomicInteger capturedTimeout = new AtomicInteger(-1); + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, + (cmd, cwd, t) -> { capturedTimeout.set(t); return List.of(); } + ); + provider.discover(); + + assertThat(capturedTimeout.get()).isEqualTo(180); + } + + @Test + @DisplayName("prefers an executable ./mvnw wrapper over a bare 'mvn'") + void shouldPreferMvnwOverMvn(@TempDir Path tmp) throws Exception { + Files.writeString(tmp.resolve("pom.xml"), ""); + final Path mvnw = tmp.resolve("mvnw"); + Files.writeString(mvnw, "#!/bin/sh\n"); + mvnw.toFile().setExecutable(true); + + final AtomicReference> capturedCmd = new AtomicReference<>(); + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, (cmd, cwd, t) -> { capturedCmd.set(cmd); return List.of(); } + ); + provider.discover(); + + assertThat(capturedCmd.get().get(0)).endsWith("mvnw"); + } + + @Test + @DisplayName("skips Maven entirely when no pom.xml is at the working directory") + void shouldSkipMavenWhenNoPomXml(@TempDir Path tmp) { + final AtomicReference invoked = new AtomicReference<>(false); + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, (cmd, cwd, t) -> { invoked.set(true); return List.of(); } + ); + provider.discover(); + assertThat(invoked.get()).isFalse(); + } + + /** + * A {@code pom.xml} that exists but is structurally empty is a real-world case (e.g. partial + * sync, broken build) — the provider must still invoke Maven (it's the runner's job to deal + * with the failure) and tolerate an empty result list without throwing. + */ + @Test + @DisplayName("invokes Maven when pom.xml is present even if empty; tolerates empty result list") + void shouldInvokeMavenWhenPomXmlIsEmpty(@TempDir Path tmp) throws Exception { + Files.writeString(tmp.resolve("pom.xml"), ""); + + final AtomicReference invoked = new AtomicReference<>(false); + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, + (cmd, cwd, t) -> { invoked.set(true); return List.of(); } + ); + + final Set entries = provider.discover(); + + assertThat(invoked.get()).isTrue(); + assertThat(entries).isEmpty(); + } +} diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderTestSupport.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderTestSupport.java new file mode 100644 index 0000000..8601b67 --- /dev/null +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderTestSupport.java @@ -0,0 +1,31 @@ +package one.edee.mcp.jdwp.evaluation; + +import java.nio.file.Path; +import java.util.List; + +/** + * Shared scaffolding for tests that construct a {@link LocalProjectClasspathProvider}. Centralises + * the "no-op" provider — an instance with a working directory that is guaranteed to contain neither + * a {@code pom.xml} nor a {@code target/} tree, an env lookup that always returns {@code null}, and + * a Maven runner that returns an empty list without ever shelling out. Used by every evaluator-side + * test that only needs the provider's *shape* rather than its real I/O behaviour. + */ +final class LocalProjectClasspathProviderTestSupport { + + private LocalProjectClasspathProviderTestSupport() { + } + + /** + * Returns a deterministic provider that contributes zero entries from every source. Working + * directory is the JVM's temp dir — guaranteed to exist but neither a Maven project nor a + * reactor with {@code target/classes} sub-trees, so the filesystem and Maven sources are + * structurally silent. + */ + static LocalProjectClasspathProvider noOpProvider() { + return new LocalProjectClasspathProvider( + Path.of(System.getProperty("java.io.tmpdir")), + name -> null, + (command, workingDirectory, timeoutSeconds) -> List.of() + ); + } +} diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunnerTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunnerTest.java new file mode 100644 index 0000000..c06c638 --- /dev/null +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunnerTest.java @@ -0,0 +1,159 @@ +package one.edee.mcp.jdwp.evaluation; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.EnabledOnOs; +import org.junit.jupiter.api.condition.OS; +import org.junit.jupiter.api.io.TempDir; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.concurrent.atomic.AtomicReference; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Tests for {@link ProcessBuilderMavenRunner}: the {@code .jdwp-mcp-classpath} harvester (parent + * must be {@code target/}, content split on the host path separator, files never deleted), the + * non-zero exit propagation, and Windows-specific wrapper preference. + */ +@DisplayName("ProcessBuilderMavenRunner — harvest, no-delete, exit propagation") +class ProcessBuilderMavenRunnerTest { + + @Test + @DisplayName("only harvests .jdwp-mcp-classpath files whose direct parent is a target/ directory") + void shouldOnlyHarvestOutputFilesUnderTargetParent(@TempDir Path tmp) throws Exception { + // Per the file-safety policy, the harvester accepts a candidate ONLY when its direct + // parent directory is named `target/`. Maven writes there because mdep.outputFile is + // `target/.jdwp-mcp-classpath`. Anything else is NOT ours and must be ignored. + Files.createDirectories(tmp.resolve("target")); + Files.writeString(tmp.resolve("target/.jdwp-mcp-classpath"), + "/m2/foo.jar:/m2/bar.jar"); + Files.createDirectories(tmp.resolve("module-a/target")); + Files.writeString(tmp.resolve("module-a/target/.jdwp-mcp-classpath"), + "/m2/baz.jar:/m2/foo.jar"); // overlap — must dedupe + + // SAFETY: a file with the same name OUTSIDE a target/ dir (e.g. left over from a previous + // buggy run, or a name collision with something the user has) MUST be ignored. + Files.writeString(tmp.resolve(".jdwp-mcp-classpath"), + "/should/not/be/harvested.jar"); + + final ProcessBuilderMavenRunner runner = new ProcessBuilderMavenRunner(req -> 0); + + final List result = runner.run(List.of("echo"), tmp, 10); + + assertThat(result).containsExactlyInAnyOrder("/m2/foo.jar", "/m2/bar.jar", "/m2/baz.jar"); + assertThat(result).doesNotContain("/should/not/be/harvested.jar"); + } + + @Test + @DisplayName("does not delete harvested files (re-runs of Maven overwrite them in place)") + void shouldNotDeleteHarvestedFiles(@TempDir Path tmp) throws Exception { + // File-safety invariant: the runner reads but never deletes. Re-runs of Maven overwrite + // the file in place; `mvn clean` is the only thing that removes it. + Files.createDirectories(tmp.resolve("target")); + final Path output = tmp.resolve("target/.jdwp-mcp-classpath"); + Files.writeString(output, "/m2/foo.jar"); + + final ProcessBuilderMavenRunner runner = new ProcessBuilderMavenRunner(req -> 0); + runner.run(List.of("echo"), tmp, 10); + + assertThat(Files.exists(output)) + .as("Harvester must NEVER delete files — see file-safety policy") + .isTrue(); + } + + @Test + @DisplayName("returns empty list when the command executor reports a non-zero exit code") + void shouldReturnEmptyListWhenMavenExitsNonzero(@TempDir Path tmp) throws Exception { + Files.createDirectories(tmp.resolve("target")); + Files.writeString(tmp.resolve("target/.jdwp-mcp-classpath"), "/m2/foo.jar"); + final ProcessBuilderMavenRunner runner = new ProcessBuilderMavenRunner(req -> 1); + + final List result = runner.run(List.of("false"), tmp, 10); + + assertThat(result).isEmpty(); + } + + /** + * When the {@link ProcessBuilderMavenRunner.CommandExecutor} lambda throws + * {@link InterruptedException}, the executor must call {@code destroyForcibly()} on the spawned + * process before propagating, otherwise the child process is leaked. + */ + @Test + @DisplayName("destroys the spawned process when waitFor is interrupted") + void shouldDestroyProcessOnInterrupt(@TempDir Path tmp) throws Exception { + Files.createDirectories(tmp.resolve("target")); + // Stage a sentinel that we can use to confirm the runner did NOT manage to read the + // output file (because the executor threw before harvest). + Files.writeString(tmp.resolve("target/.jdwp-mcp-classpath"), "/sentinel.jar"); + + final AtomicReference spawned = new AtomicReference<>(); + final ProcessBuilderMavenRunner runner = new ProcessBuilderMavenRunner(req -> { + final Process p = new ProcessBuilder("sleep", "30").redirectErrorStream(true).start(); + spawned.set(p); + throw new InterruptedException("simulated interrupt during waitFor"); + }); + + // Run shouldn't blow up at the call site — the runner is documented to swallow the + // exception, log a WARN, and return an empty list. The bug is in the leak, not the API. + final List result = runner.run(List.of("sleep", "30"), tmp, 10); + + assertThat(result).isEmpty(); + assertThat(spawned.get()) + .as("Spawned process must not be leaked when waitFor is interrupted") + .isNotNull(); + assertThat(spawned.get().isAlive()) + .as("Spawned process must be destroyed on interrupt — currently leaked") + .isFalse(); + } + + /** + * {@link Files#walk(Path, int, java.nio.file.FileVisitOption...)} descends into every directory + * up to the depth limit. The harvester must skip non-Maven trees ({@code node_modules}, + * {@code .git}, {@code .idea}, etc.) so a stray + * {@code node_modules/foo/target/.jdwp-mcp-classpath} (e.g. a packaged JS module shipping its own + * Maven artifact) is NOT picked up. + */ + @Test + @DisplayName("does not harvest .jdwp-mcp-classpath files under SKIP_DIRS (e.g. node_modules)") + void shouldSkipNodeModulesDuringHarvest(@TempDir Path tmp) throws Exception { + Files.createDirectories(tmp.resolve("target")); + Files.writeString(tmp.resolve("target/.jdwp-mcp-classpath"), "/real/m2/foo.jar"); + Files.createDirectories(tmp.resolve("node_modules/foo/target")); + Files.writeString(tmp.resolve("node_modules/foo/target/.jdwp-mcp-classpath"), + "/from/node_modules/should-not-be-harvested.jar"); + + final ProcessBuilderMavenRunner runner = new ProcessBuilderMavenRunner(req -> 0); + + final List result = runner.run(List.of("echo"), tmp, 10); + + assertThat(result) + .contains("/real/m2/foo.jar") + .doesNotContain("/from/node_modules/should-not-be-harvested.jar"); + } + + /** + * On Windows the wrapper script is {@code mvnw.cmd}, not {@code mvnw}. The provider must probe + * for both, preferring {@code mvnw} (Unix) when present and falling back to {@code mvnw.cmd} on + * Windows hosts. This test is meaningful only on a real Windows runner — the + * {@link EnabledOnOs} skips it elsewhere. + */ + @Test + @DisplayName("prefers mvnw.cmd on Windows when no Unix mvnw wrapper exists") + @EnabledOnOs(OS.WINDOWS) + void shouldPreferMvnwCmdOnWindows(@TempDir Path tmp) throws Exception { + Files.writeString(tmp.resolve("pom.xml"), ""); + final Path mvnwCmd = tmp.resolve("mvnw.cmd"); + Files.writeString(mvnwCmd, "@echo off\r\n"); + + final AtomicReference> capturedCmd = new AtomicReference<>(); + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, (cmd, cwd, t) -> { capturedCmd.set(cmd); return List.of(); } + ); + provider.discover(); + + assertThat(capturedCmd.get().get(0)).endsWith("mvnw.cmd"); + } +} diff --git a/skills/java-debug/references/troubleshooting.md b/skills/java-debug/references/troubleshooting.md index 8c145a7..ca92644 100644 --- a/skills/java-debug/references/troubleshooting.md +++ b/skills/java-debug/references/troubleshooting.md @@ -32,3 +32,21 @@ Only kill a process you launched yourself in this session. If the process is unr ## Both processes gone after a crash The test JVM exits when its debugger detaches; the MCP server may also have crashed. Reconnect the MCP server first (`/mcp`), then relaunch the test JVM from scratch. + +## Expression evaluation fails with "X cannot be resolved" + +Symptoms: `jdwp_evaluate_expression`, `jdwp_assert_expression`, a logpoint, or a conditional breakpoint returns a JDT compile diagnostic like `X cannot be resolved to a type` / `cannot be resolved to a variable`, or `configureCompilerClasspath` throws `Classpath discovery failed for the current connection: neither the target VM classloader hierarchy nor the local project yielded any classpath entries`. + +The target type is visible to the running JVM but invisible to the wrapper compile. This happens when the target's classloader hierarchy hides JARs from `getURLs()` — Tomcat's `WebappClassLoaderBase`, Spring Boot's `LaunchedClassLoader`, dev-tools `RestartClassLoader`, or any custom `URLClassLoader`. The MCP server augments the discovered classpath with entries from its own CWD (`target/classes`, `target/test-classes`, Maven-resolved dependencies) to plug those gaps. If the local sources are empty too, the compile breaks. + +Work through these in order: + +1. **Run `jdwp_diagnose` and read the `Local project classpath` block.** It lists the server's CWD, whether `pom.xml` was found, and a per-source breakdown — `Sources: env-override=N, filesystem=N, maven=N`. All zeros means the fallback contributed nothing; that's the gap to fill. + +2. **Set `JDWP_EXTRA_CLASSPATH` to add specific jars or class directories.** Colon-separated on Linux/macOS, semicolon-separated on Windows (parsed with `File.pathSeparator`). Restart the MCP server after setting the variable — the provider memoizes for the JDI connection's lifetime. This is the right lever when a single JAR is missing or the project is Gradle (Gradle is not auto-detected in v1). + +3. **Restart the MCP server from a directory containing a Maven `pom.xml`.** The filesystem scan walks depth-5 looking for `target/classes` / `target/test-classes`, and `dependency:build-classpath` runs per detected module. First call on a cold Maven cache can take up to 3 minutes — that's expected, not a hang. `jdwp_diagnose` warns about the latency in the section header. + +4. **Check the server log for `[LocalClasspath]` lines.** Every source emits at least one entry: `INFO` for successful contributions and timings, `WARN` for Maven non-zero exits / timeouts / malformed env values (with the captured stdout). The same prefix covers `LocalProjectClasspathProvider`, `ProcessBuilderMavenRunner`, and the union step in `JdiExpressionEvaluator`. Remote-side failures use the `[Discoverer]` / `[JDI]` prefixes — both worth grepping when eval breaks. + +Remote-discovered entries win on resolution (they come first in the merged classpath), so the local fallback can only *add* types — it cannot mask a stale remote definition. If you rebuilt locally and expect the eval to pick up the new bytecode but it doesn't, the remote target is still authoritative for any class it already exposes. From 90edc54592fe04d3c6a8165c3d4889502c0fdd11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Thu, 28 May 2026 15:55:06 +0200 Subject: [PATCH 02/10] fix(evaluation): address Copilot review on PR #33 Symlink invariants - Filesystem scan now passes LinkOption.NOFOLLOW_LINKS to every isDirectory probe and rejects symlinked children before recursing, so the documented "symlinks not followed" rule actually holds. - Harvester adds Files.isRegularFile(p, NOFOLLOW_LINKS) to its filter chain, so a symlink at target/.jdwp-mcp-classpath cannot be used to redirect the read at a file outside the project tree. Process ownership - Removed the run()-level descendant-tracking that used ProcessHandle.current().descendants() PID diff; that would forcibly kill any unrelated subprocess another server component happened to start in the same window. Cleanup responsibility is now scoped to the spawned Process inside executeRealCommand, which knows exactly which tree it owns. New destroyProcessTree() destroys the Maven process and its own descendants (Maven can fork helpers) without touching siblings. Concurrency - ByteArrayOutputStream is no longer relied on for cross-thread safety. An explicit monitor guards both the drainer's writes and the main thread's toString()/size() snapshots, so the WARN log on timeout sees a consistent buffer. Cold-cache UX - Diagnose env-state now renders three-way (unset)/(set, no entries)/(set) on cold cache as well, via a new envOverrideHasEntries() helper that parses the env var without triggering discovery. Stale "may trigger slow discovery" comments updated to match the actual non-blocking peek. Test hygiene - defaultEmptyClasspathProvider() and noOpProvider() now root the filesystem scan at a guaranteed-non-existent path so the scan short-circuits at the first isDirectory probe. Previously user.dir /java.io.tmpdir leaked unrelated target/classes trees into tests. - Same fix applied to stubLocalProvider() in the union test. - shouldDestroyProcessOnInterrupt replaced by two tests that exercise the real invariants: run() must NOT kill unrelated subprocesses, executeRealCommand DOES destroy its own process tree. - Added explicit symlink tests for both filesystem scan and harvest. Hardening - LocalProjectClasspathProvider default constructor null-guards System.getProperty("user.dir") with a "." fallback so a sandboxed JVM launch can still construct the bean. Module sweep: 1062 tests, 0 failures, 3 skipped. --- .../java/one/edee/mcp/jdwp/JDWPTools.java | 41 ++++-- .../LocalProjectClasspathProvider.java | 40 +++++- .../evaluation/ProcessBuilderMavenRunner.java | 111 +++++++++------ .../edee/mcp/jdwp/JDWPToolsDiagnoseTest.java | 2 +- .../edee/mcp/jdwp/JDWPToolsTestSupport.java | 9 +- ...ssionEvaluatorLocalClasspathUnionTest.java | 7 +- ...ctClasspathProviderFilesystemScanTest.java | 45 ++++++ ...alProjectClasspathProviderTestSupport.java | 9 +- .../ProcessBuilderMavenRunnerTest.java | 129 ++++++++++++++---- 9 files changed, 294 insertions(+), 99 deletions(-) diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java index 9371866..e4cde5f 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java @@ -113,8 +113,11 @@ public class JDWPTools { /** * Local-project classpath fallback. Surfaced through {@code jdwp_diagnose} so operators can * see which sources (env override / filesystem / Maven) contributed to the additive classpath - * fed into expression evaluation. Memoized per JDI connection; the diagnose call may trigger - * the first (slow) discovery if no expression has been evaluated yet. + * fed into expression evaluation. Memoised per JDI connection; the diagnose path reads the + * cached breakdown via {@link LocalProjectClasspathProvider#peekCachedBreakdown()} and never + * triggers discovery itself — discovery happens organically on the first expression + * evaluation. When the cache is cold the diagnose section prints a "not yet computed" hint + * instead of blocking. */ private final LocalProjectClasspathProvider localClasspathProvider; @@ -1702,8 +1705,9 @@ private String buildFullDiagnosticReport(boolean inspectAll) { // Local project classpath section: shows which entries the additive fallback contributed // to expression evaluation. Independent of connection state so operators can sanity-check - // their CWD even before attaching. May trigger the first (cold-cache, slow) discovery — - // intentional; the header explicitly warns about the latency. + // their CWD even before attaching. NON-BLOCKING: reads only the cached breakdown via + // peekCachedBreakdown(); never triggers a Maven invocation. When the cache is cold the + // block prints a "not yet computed" hint and discovery happens on the first eval. out.append(renderLocalClasspathBlock()); out.append(renderLocalJvmsBlock(status, inspectAll)); @@ -1724,26 +1728,17 @@ private String buildFullDiagnosticReport(boolean inspectAll) { */ private String renderLocalClasspathBlock() { final StringBuilder out = new StringBuilder("\n▸ Local project classpath\n"); - out.append(" (computed once per JDI connection; first call may take up to 3 min on cold Maven cache)\n"); + out.append(" (read from cache; populated on first expression evaluation)\n"); try { final LocalProjectClasspathProvider.Breakdown breakdown = localClasspathProvider.peekCachedBreakdown(); out.append(" CWD: ").append(localClasspathProvider.getWorkingDirectory()).append('\n'); out.append(" pom.xml: ").append(localClasspathProvider.hasPomAtRoot() ? "yes" : "no").append('\n'); - final boolean envSet = localClasspathProvider.isEnvOverrideSet(); + final String envState = renderEnvState(breakdown); if (breakdown == null) { out.append(" (not yet computed; will populate on first expression evaluation)\n"); - final String envState = envSet ? "(set)" : "(unset)"; out.append(" Override env: JDWP_EXTRA_CLASSPATH ").append(envState).append('\n'); } else { - final String envState; - if (!envSet) { - envState = "(unset)"; - } else if (breakdown.envOverride() > 0) { - envState = "(set)"; - } else { - envState = "(set, no entries)"; - } out.append(" Sources: env-override=").append(breakdown.envOverride()) .append(", filesystem=").append(breakdown.filesystem()) .append(", maven=").append(breakdown.maven()).append('\n'); @@ -1757,6 +1752,22 @@ private String renderLocalClasspathBlock() { return out.toString(); } + /** + * Resolves the three-way env-state marker rendered after {@code JDWP_EXTRA_CLASSPATH}. When the + * cache is warm ({@code breakdown != null}) the entry-count from the breakdown is authoritative; + * when cold we still answer correctly by parsing the env var directly via + * {@link LocalProjectClasspathProvider#envOverrideHasEntries()} — no discovery triggered. + */ + private String renderEnvState(LocalProjectClasspathProvider.@Nullable Breakdown breakdown) { + if (!localClasspathProvider.isEnvOverrideSet()) { + return "(unset)"; + } + final boolean hasEntries = breakdown != null + ? breakdown.envOverride() > 0 + : localClasspathProvider.envOverrideHasEntries(); + return hasEntries ? "(set)" : "(set, no entries)"; + } + /** * Runs discovery and renders the local-JVM inventory block. Discovery errors are caught * and surfaced as a one-line note instead of breaking the whole report. Accepts a diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java index 8c27733..5596d06 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java @@ -14,6 +14,7 @@ import java.util.Collections; import java.util.LinkedHashSet; import java.util.List; +import java.util.Objects; import java.util.Set; import java.util.function.Function; import java.util.regex.Pattern; @@ -119,7 +120,11 @@ public LocalProjectClasspathProvider(Path workingDirectory, */ @Autowired public LocalProjectClasspathProvider(ProcessBuilderMavenRunner mavenRunner) { - this(Path.of(System.getProperty("user.dir")), System::getenv, mavenRunner); + // `user.dir` is documented as always-present on a normal JVM launch, but a sandboxed + // / restricted launch can drop it. Defaulting to "." rather than letting Path.of(null) + // NPE keeps the bean constructible — the scan then runs against the process's effective + // working directory, which is the same place "." would resolve to anyway. + this(Path.of(Objects.toString(System.getProperty("user.dir"), ".")), System::getenv, mavenRunner); } /** @@ -174,6 +179,25 @@ public boolean isEnvOverrideSet() { return envLookup.apply(ENV_NAME) != null; } + /** + * Reports whether the {@code JDWP_EXTRA_CLASSPATH} env var has any non-blank entries after + * splitting on the host {@link File#pathSeparator}. Lets the diagnose path render the three-way + * "set, no entries" state on a cold cache without triggering full discovery — the answer + * depends only on the env var, not on the filesystem or Maven. + */ + public boolean envOverrideHasEntries() { + final String raw = envLookup.apply(ENV_NAME); + if (raw == null || raw.isBlank()) { + return false; + } + for (String part : raw.split(Pattern.quote(File.pathSeparator), -1)) { + if (!part.trim().isEmpty()) { + return true; + } + } + return false; + } + /** * Runs (or returns the memoised result of) the three-source discovery. Synchronised so two * concurrent callers cannot both pay the Maven invocation cost; the second caller blocks and @@ -340,17 +364,20 @@ private void addFilesystemScan(Set entries) { * aborting — a partial classpath beats no classpath. */ private static void scanForClassDirs(Path dir, int depth, Set entries) { - if (depth > MAX_SCAN_DEPTH || !Files.isDirectory(dir)) { + // NOFOLLOW_LINKS everywhere: a symlink to another tree on the same filesystem could create + // walk cycles or pull in jars from outside the project. The documented invariant is that + // this scan never follows links — enforce it at every probe. + if (depth > MAX_SCAN_DEPTH || !Files.isDirectory(dir, LinkOption.NOFOLLOW_LINKS)) { return; } // At each directory, probe for target/classes and target/test-classes BEFORE recursing — we // want a module's classes whether or not it has child modules. final Path classes = dir.resolve("target/classes"); - if (Files.isDirectory(classes)) { + if (Files.isDirectory(classes, LinkOption.NOFOLLOW_LINKS)) { entries.add(classes.toString()); } final Path testClasses = dir.resolve("target/test-classes"); - if (Files.isDirectory(testClasses)) { + if (Files.isDirectory(testClasses, LinkOption.NOFOLLOW_LINKS)) { entries.add(testClasses.toString()); } if (depth == MAX_SCAN_DEPTH) { @@ -358,7 +385,10 @@ private static void scanForClassDirs(Path dir, int depth, Set entries) { } try (Stream children = Files.list(dir)) { children - .filter(Files::isDirectory) + // `Files::isDirectory` (the method reference) FOLLOWS links — use the explicit + // overload with NOFOLLOW_LINKS so a symlinked directory is treated as "not a + // directory" and skipped. + .filter(p -> Files.isDirectory(p, LinkOption.NOFOLLOW_LINKS)) .filter(p -> { final String name = p.getFileName().toString(); return !name.startsWith(".") && !SKIP_DIRS.contains(name); diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java index ca1db3e..b14752c 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java @@ -9,6 +9,7 @@ import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; +import java.nio.file.LinkOption; import java.nio.file.Path; import java.util.ArrayList; import java.util.LinkedHashSet; @@ -16,7 +17,6 @@ import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.regex.Pattern; -import java.util.stream.Collectors; import java.util.stream.Stream; /** @@ -60,13 +60,6 @@ public ProcessBuilderMavenRunner() { @Override public List run(List command, Path workingDirectory, int timeoutSeconds) { - // Snapshot any pre-existing descendant PIDs so the cleanup below only targets processes - // that this call spawned. Without the snapshot, an InterruptedException from the executor - // would either leak the spawned child (current behaviour) or, worse, race against unrelated - // descendant processes started for some other reason. - final Set preExistingDescendants = ProcessHandle.current().descendants() - .map(ProcessHandle::pid) - .collect(Collectors.toSet()); try { final int exitCode = commandExecutor.execute(new CommandRequest(command, workingDirectory, timeoutSeconds)); // executeRealCommand already logs WARN with the captured stdout on non-zero/timeout; @@ -76,14 +69,13 @@ public List run(List command, Path workingDirectory, int timeout } return harvestOutputFiles(workingDirectory); } catch (InterruptedException ie) { - // Restore the interrupt flag so callers higher up the stack can observe the cancellation. - // Then destroy any descendant process this call spawned — the child holds an OS pipe - // open and would block any caller that later tries to drain stdout. Targeted at PIDs - // new since the snapshot taken before the executor call. - destroyNewDescendants(preExistingDescendants); + // executeRealCommand kills its own child process and its descendants before throwing — + // process ownership is scoped to whoever spawned it. Restore the interrupt flag and + // return so the caller higher up the stack can observe the cancellation. We deliberately + // do NOT consult `ProcessHandle.current().descendants()` here: that would forcibly kill + // any unrelated subprocess another server component happened to start in the same window. Thread.currentThread().interrupt(); - log.warn("[LocalClasspath] Maven invocation interrupted (cwd={}); destroyed spawned descendants", - workingDirectory); + log.warn("[LocalClasspath] Maven invocation interrupted (cwd={})", workingDirectory); return List.of(); } catch (Exception e) { log.warn("[LocalClasspath] Maven invocation failed: {}: {} (cwd={})", @@ -92,25 +84,6 @@ public List run(List command, Path workingDirectory, int timeout } } - private static void destroyNewDescendants(Set preExistingPids) { - // Snapshot the new descendants up-front so we can both issue destroy AND await termination - // before returning. Issuing destroy without awaiting leaves the caller racing the OS — a - // test that checks `isAlive()` immediately after run() can flake. 2s is enough for SIGKILL - // to land on a sleep(1)-style process on any sane kernel. - final List newDescendants = ProcessHandle.current().descendants() - .filter(h -> !preExistingPids.contains(h.pid())) - .toList(); - newDescendants.forEach(ProcessHandle::destroyForcibly); - for (ProcessHandle h : newDescendants) { - try { - h.onExit().get(2, TimeUnit.SECONDS); - } catch (Exception ignored) { - // Best-effort: a stuck/orphaned process is logged elsewhere; we already issued - // destroyForcibly. The caller still gets a timely return. - } - } - } - /** * Walks {@code root} (depth 5, covering {@code ///target/}) for * Maven-written classpath files and aggregates their entries. @@ -135,7 +108,11 @@ private static List harvestOutputFiles(Path root) throws IOException { && p.getParent() != null && p.getParent().getFileName() != null && "target".equals(p.getParent().getFileName().toString()) - && !hasSkippedAncestor(root, p)) + && !hasSkippedAncestor(root, p) + // Symlinks must not be followed: a symlink at target/.jdwp-mcp-classpath + // could point at any file on the host. The harvester is invariant-bound + // to read only Maven-written output it found in-tree. + && Files.isRegularFile(p, LinkOption.NOFOLLOW_LINKS)) .forEach(file -> { try { final String content = Files.readString(file).trim(); @@ -213,13 +190,20 @@ private static int executeRealCommand(CommandRequest req) throws IOException, In // project targets Java 17 (virtual threads were finalized in Java 21). The semantics are // identical: "drain in the background, don't block the process pipe". final ByteArrayOutputStream captured = new ByteArrayOutputStream(); + // Explicit monitor: ByteArrayOutputStream's per-method synchronization is an OpenJDK + // implementation detail and does not give the JMM guarantee we need across "write from + // drainer + read from main on the timeout path". A shared lock makes the cap check + write + // atomic and the toString() snapshot consistent. + final Object capturedLock = new Object(); final Thread drainer = new Thread(() -> { try (var in = process.getInputStream()) { final byte[] buf = new byte[4096]; int n; while ((n = in.read(buf)) > 0) { - if (captured.size() < STDOUT_CAPTURE_BYTES) { - captured.write(buf, 0, Math.min(n, STDOUT_CAPTURE_BYTES - captured.size())); + synchronized (capturedLock) { + if (captured.size() < STDOUT_CAPTURE_BYTES) { + captured.write(buf, 0, Math.min(n, STDOUT_CAPTURE_BYTES - captured.size())); + } } // Continue reading even after the cap so the process never blocks on output. } @@ -229,7 +213,7 @@ private static int executeRealCommand(CommandRequest req) throws IOException, In drainer.start(); try { if (!process.waitFor(req.timeoutSeconds(), TimeUnit.SECONDS)) { - process.destroyForcibly(); + destroyProcessTree(process); // Let any in-flight writes from the child reach the drainer before snapshotting // the buffer: otherwise the WARN below would print a truncated tail. try { @@ -239,13 +223,15 @@ private static int executeRealCommand(CommandRequest req) throws IOException, In } log.warn("[LocalClasspath] Maven invocation timed out after {}s. Captured output (up to {} bytes):\n{}", req.timeoutSeconds(), STDOUT_CAPTURE_BYTES, - captured.toString(StandardCharsets.UTF_8)); + snapshotCaptured(captured, capturedLock)); return -1; } } catch (InterruptedException ie) { - // waitFor was interrupted — kill the child, restore the interrupt flag, propagate so - // the outer run() returns an empty list and surfaces the cancellation to its caller. - process.destroyForcibly(); + // waitFor was interrupted — kill the child AND its descendants (Maven can fork helpers), + // restore the interrupt flag, propagate so the outer run() returns empty and surfaces + // the cancellation. Only THIS Maven process and its descendants are touched — never + // unrelated subprocesses owned by other server components. + destroyProcessTree(process); Thread.currentThread().interrupt(); throw ie; } @@ -260,13 +246,50 @@ private static int executeRealCommand(CommandRequest req) throws IOException, In if (exit != 0) { log.warn("[LocalClasspath] Maven exited with code {}. Captured output (up to {} bytes):\n{}", exit, STDOUT_CAPTURE_BYTES, - captured.toString(StandardCharsets.UTF_8)); + snapshotCaptured(captured, capturedLock)); } else { - log.debug("[LocalClasspath] Maven succeeded ({} bytes of output captured)", captured.size()); + log.debug("[LocalClasspath] Maven succeeded ({} bytes of output captured)", + snapshotSize(captured, capturedLock)); } return exit; } + /** + * Forcibly destroys the given process and every descendant in its tree, then awaits each one's + * exit for up to 2s. Bounded to the started process — does NOT touch sibling subprocesses + * owned by other components, which a {@code ProcessHandle.current().descendants()} sweep would. + */ + private static void destroyProcessTree(Process process) { + final List tree = process.descendants().toList(); + // Kill children first so the parent can't re-fork during the window between the kill calls. + tree.forEach(ProcessHandle::destroyForcibly); + process.destroyForcibly(); + for (ProcessHandle h : tree) { + try { + h.onExit().get(2, TimeUnit.SECONDS); + } catch (Exception ignored) { + // Best-effort — we already issued destroyForcibly; nothing more to do here. + } + } + try { + process.onExit().get(2, TimeUnit.SECONDS); + } catch (Exception ignored) { + // Same as above. + } + } + + private static String snapshotCaptured(ByteArrayOutputStream captured, Object lock) { + synchronized (lock) { + return captured.toString(StandardCharsets.UTF_8); + } + } + + private static int snapshotSize(ByteArrayOutputStream captured, Object lock) { + synchronized (lock) { + return captured.size(); + } + } + /** Seam for the actual process execution; returns the child's exit code (or {@code -1} on timeout). */ @FunctionalInterface interface CommandExecutor { diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsDiagnoseTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsDiagnoseTest.java index ec73972..4867999 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsDiagnoseTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsDiagnoseTest.java @@ -296,7 +296,7 @@ jdiService, new BreakpointTracker(), new WatcherManager(), final String result = toolsWithStub.jdwp_diagnose(null); assertThat(result).contains("Local project classpath"); - assertThat(result).contains("(computed once per JDI connection"); + assertThat(result).contains("(read from cache; populated on first expression evaluation)"); assertThat(result).contains("CWD: " + cwd); // The stub forces Maven to contribute 3 jars; filesystem and env may legitimately add // entries from the running build's tree but the maven count must be exactly 3. diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsTestSupport.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsTestSupport.java index 42877ca..04b448c 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsTestSupport.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsTestSupport.java @@ -55,13 +55,14 @@ static JDWPTools newToolsWithMocks() { } /** - * Builds a deterministic {@link LocalProjectClasspathProvider} that never shells out to Maven - * or scans the filesystem. Used as the default for tests that do not exercise the local - * classpath surface — keeps the diagnose path fast and reproducible. + * Builds a deterministic {@link LocalProjectClasspathProvider} that contributes zero entries + * from every source. Working directory is a guaranteed-non-existent path so the depth-5 + * filesystem scan short-circuits at the first {@code isDirectory} probe — keeps the diagnose + * path fast and reproducible regardless of what {@code user.dir} happens to contain. */ static LocalProjectClasspathProvider defaultEmptyClasspathProvider() { return new LocalProjectClasspathProvider( - Path.of(System.getProperty("user.dir")), + Path.of("/nonexistent/jdwp-mcp-default-empty-" + java.util.UUID.randomUUID()), name -> null, (command, workingDirectory, timeoutSeconds) -> List.of() ); diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorLocalClasspathUnionTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorLocalClasspathUnionTest.java index e0b5e66..4626185 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorLocalClasspathUnionTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorLocalClasspathUnionTest.java @@ -183,9 +183,10 @@ private static LocalProjectClasspathProvider stubLocalProvider(Set entri // this is the exact format the provider's env-override path expects. final String joined = String.join(File.pathSeparator, entries); return new LocalProjectClasspathProvider( - // Working directory is a temp-ish path with no pom.xml / target/, so the filesystem and - // Maven sources contribute nothing and the env-override path supplies every entry. - Path.of(System.getProperty("java.io.tmpdir")), + // Guaranteed-non-existent path so the depth-5 filesystem scan short-circuits at the + // first isDirectory probe. java.io.tmpdir is unsafe — CI machines and dev hosts can + // legitimately have target/classes subtrees there from unrelated tooling. + Path.of("/nonexistent/jdwp-mcp-union-test-" + java.util.UUID.randomUUID()), name -> "JDWP_EXTRA_CLASSPATH".equals(name) ? joined : null, (c, d, t) -> List.of() ); diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderFilesystemScanTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderFilesystemScanTest.java index 55680a4..82055d7 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderFilesystemScanTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderFilesystemScanTest.java @@ -123,6 +123,51 @@ void shouldSkipEverySkipDirsEntry(String skipDir, @TempDir Path tmp) throws Exce .doesNotContain(tmp.resolve(skipDir + "/module/target/classes").toString()); } + /** + * Symlinks must not be followed by the filesystem scan: a symlink to a directory outside the + * project tree could pull in unrelated jars (or, in a malicious workspace, escape the project + * root entirely). The documented invariant is "symlinks not followed" — enforce it. + */ + @Test + @org.junit.jupiter.api.condition.EnabledOnOs({ + org.junit.jupiter.api.condition.OS.LINUX, + org.junit.jupiter.api.condition.OS.MAC + }) + @DisplayName("does not traverse symlinked directories during filesystem scan") + void shouldNotFollowSymlinkedDirectoriesDuringScan(@TempDir Path tmp) throws Exception { + // A module that lives OUTSIDE the project root, in a sibling temp tree. + final Path outside = Files.createTempDirectory("jdwp-mcp-outside-tree-"); + try { + Files.createDirectories(outside.resolve("target/classes")); + + // Inside the project tree: a symlinked directory pointing at the outside module. + // If symlinks were followed, scanning would descend into it and add + // outside/target/classes to the entries — a project-boundary escape. + Files.createSymbolicLink( + tmp.resolve("linked-module"), + outside + ); + // A real (non-symlinked) module for sanity. + Files.createDirectories(tmp.resolve("real-module/target/classes")); + + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, (cmd, cwd, t) -> List.of() + ); + + final Set entries = provider.discover(); + + assertThat(entries) + .contains(tmp.resolve("real-module/target/classes").toString()) + .as("Symlinked directories must not be traversed — symlink-not-followed invariant") + .doesNotContain(outside.resolve("target/classes").toString()); + } finally { + // Manual cleanup since the outside dir is not @TempDir-managed. + Files.deleteIfExists(outside.resolve("target/classes")); + Files.deleteIfExists(outside.resolve("target")); + Files.deleteIfExists(outside); + } + } + /** * The scanner skips nested {@code target/} directories — once we enter a {@code target/} we * never recurse into a {@code target/foo/target/classes}. Anything nested inside the build diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderTestSupport.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderTestSupport.java index 8601b67..897f9b9 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderTestSupport.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderTestSupport.java @@ -17,13 +17,14 @@ private LocalProjectClasspathProviderTestSupport() { /** * Returns a deterministic provider that contributes zero entries from every source. Working - * directory is the JVM's temp dir — guaranteed to exist but neither a Maven project nor a - * reactor with {@code target/classes} sub-trees, so the filesystem and Maven sources are - * structurally silent. + * directory is a guaranteed-non-existent path so the depth-5 filesystem scan short-circuits + * at the first {@code isDirectory} probe — keeps tests fast and isolated from whatever happens + * to live under {@code java.io.tmpdir} on the running host (CI machines sometimes contain + * unrelated {@code target/classes} trees there). */ static LocalProjectClasspathProvider noOpProvider() { return new LocalProjectClasspathProvider( - Path.of(System.getProperty("java.io.tmpdir")), + Path.of("/nonexistent/jdwp-mcp-no-op-" + java.util.UUID.randomUUID()), name -> null, (command, workingDirectory, timeoutSeconds) -> List.of() ); diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunnerTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunnerTest.java index c06c638..e7a9c87 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunnerTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunnerTest.java @@ -77,36 +77,119 @@ void shouldReturnEmptyListWhenMavenExitsNonzero(@TempDir Path tmp) throws Except } /** - * When the {@link ProcessBuilderMavenRunner.CommandExecutor} lambda throws - * {@link InterruptedException}, the executor must call {@code destroyForcibly()} on the spawned - * process before propagating, otherwise the child process is leaked. + * Process-ownership invariant — the executor is responsible for cleaning up the child process + * IT spawned; the {@code run()} wrapper must NOT consult {@code ProcessHandle.current().descendants()} + * to kill processes by PID diff, since that would forcibly destroy any unrelated subprocess + * another server component happened to start in the same window. This test asserts the + * non-overreach: a sibling subprocess started by the test stays alive even when the + * executor throws {@link InterruptedException}. + * + *

The real "destroy MY child on interrupt" behaviour lives in + * {@link ProcessBuilderMavenRunner#executeRealCommand} and is exercised by + * {@link #shouldDestroyOwnedProcessTreeOnTimeout} below using the production code path. */ @Test - @DisplayName("destroys the spawned process when waitFor is interrupted") - void shouldDestroyProcessOnInterrupt(@TempDir Path tmp) throws Exception { + @DisplayName("run() does NOT destroy unrelated sibling subprocesses when the executor throws InterruptedException") + void shouldNotOverkillUnrelatedSubprocessesOnInterrupt(@TempDir Path tmp) throws Exception { Files.createDirectories(tmp.resolve("target")); - // Stage a sentinel that we can use to confirm the runner did NOT manage to read the - // output file (because the executor threw before harvest). - Files.writeString(tmp.resolve("target/.jdwp-mcp-classpath"), "/sentinel.jar"); - final AtomicReference spawned = new AtomicReference<>(); - final ProcessBuilderMavenRunner runner = new ProcessBuilderMavenRunner(req -> { - final Process p = new ProcessBuilder("sleep", "30").redirectErrorStream(true).start(); - spawned.set(p); - throw new InterruptedException("simulated interrupt during waitFor"); - }); + // Sibling: a subprocess started by "some other component" before run() is invoked. It + // must survive run()'s cleanup — that's the invariant Copilot flagged. + final Process sibling = new ProcessBuilder("sleep", "5").redirectErrorStream(true).start(); + try { + final ProcessBuilderMavenRunner runner = new ProcessBuilderMavenRunner(req -> { + throw new InterruptedException("simulated interrupt; stub does not spawn anything"); + }); + + final List result = runner.run(List.of("anything"), tmp, 10); + + assertThat(result).isEmpty(); + assertThat(sibling.isAlive()) + .as("Sibling subprocess must NOT be destroyed by run() — process ownership is scoped to the executor") + .isTrue(); + // Interrupt flag must be restored so the caller can observe the cancellation. + assertThat(Thread.interrupted()) + .as("run() must restore the interrupt flag") + .isTrue(); + } finally { + sibling.destroyForcibly(); + sibling.onExit().get(2, java.util.concurrent.TimeUnit.SECONDS); + } + } - // Run shouldn't blow up at the call site — the runner is documented to swallow the - // exception, log a WARN, and return an empty list. The bug is in the leak, not the API. - final List result = runner.run(List.of("sleep", "30"), tmp, 10); + /** + * The production {@link ProcessBuilderMavenRunner#executeRealCommand} (used via the default, + * no-arg constructor) must destroy the spawned process and its descendants on timeout. This + * exercises the real ProcessBuilder code path with a short timeout against {@code sleep 30}. + */ + @Test + @org.junit.jupiter.api.condition.EnabledOnOs({ + org.junit.jupiter.api.condition.OS.LINUX, + org.junit.jupiter.api.condition.OS.MAC + }) + @DisplayName("executeRealCommand destroys its own process tree on timeout") + void shouldDestroyOwnedProcessTreeOnTimeout(@TempDir Path tmp) throws Exception { + // Real production runner — exercises executeRealCommand via the default constructor. + final ProcessBuilderMavenRunner runner = new ProcessBuilderMavenRunner(); + + // Snapshot PIDs of the current JVM's descendants before the call so we can verify + // after-state. + final java.util.Set before = ProcessHandle.current().descendants() + .map(ProcessHandle::pid) + .collect(java.util.stream.Collectors.toSet()); + + final long startTime = System.currentTimeMillis(); + // 1s timeout against a 30s sleep — the runner must terminate the child within a few + // seconds of the budget, not 30s later. + final List result = runner.run(List.of("sleep", "30"), tmp, 1); + final long elapsed = System.currentTimeMillis() - startTime; assertThat(result).isEmpty(); - assertThat(spawned.get()) - .as("Spawned process must not be leaked when waitFor is interrupted") - .isNotNull(); - assertThat(spawned.get().isAlive()) - .as("Spawned process must be destroyed on interrupt — currently leaked") - .isFalse(); + assertThat(elapsed) + .as("Runner must enforce timeout — should return within ~5s, not wait the full 30") + .isLessThan(8_000); + + // All descendants that appeared during the call must be gone (destroyed by executeRealCommand). + final java.util.List stillAlive = ProcessHandle.current().descendants() + .filter(h -> !before.contains(h.pid())) + .filter(ProcessHandle::isAlive) + .toList(); + assertThat(stillAlive) + .as("executeRealCommand must destroy every descendant it spawned") + .isEmpty(); + } + + /** + * File-safety: the harvester reads files only when they are regular non-link files. A symlink + * named {@code .jdwp-mcp-classpath} living inside a {@code target/} directory must NOT be + * followed — it could point to any file on the host, breaking the "we only read Maven-written + * output we found in-tree" invariant. + */ + @Test + @org.junit.jupiter.api.condition.EnabledOnOs({ + org.junit.jupiter.api.condition.OS.LINUX, + org.junit.jupiter.api.condition.OS.MAC + }) + @DisplayName("does not follow a symlink at target/.jdwp-mcp-classpath") + void shouldNotFollowSymlinkAtHarvestPath(@TempDir Path tmp) throws Exception { + // Outside-the-tree file the symlink would point at. + final Path elsewhere = tmp.resolve("somewhere-else.txt"); + Files.writeString(elsewhere, "/from/outside/should-not-be-harvested.jar"); + + // target/.jdwp-mcp-classpath is a symlink to the file outside the project. + Files.createDirectories(tmp.resolve("target")); + Files.createSymbolicLink( + tmp.resolve("target/.jdwp-mcp-classpath"), + elsewhere + ); + + final ProcessBuilderMavenRunner runner = new ProcessBuilderMavenRunner(req -> 0); + + final List result = runner.run(List.of("echo"), tmp, 10); + + assertThat(result) + .as("Symlinks at the harvest path must not be followed — file-safety invariant") + .doesNotContain("/from/outside/should-not-be-harvested.jar"); } /** From c1f4ce435ed19dd87d2ac14a7c4a0278554cf7a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Thu, 28 May 2026 16:05:53 +0200 Subject: [PATCH 03/10] fix(evaluation): make new tests OS-portable per Copilot review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Harvester fixture uses File.pathSeparator instead of hard-coded ':' so the test runs correctly on Windows. - shouldNotOverkillUnrelatedSubprocessesOnInterrupt gated with @EnabledOnOs({LINUX, MAC}) — the sibling-process trick relies on the external `sleep` command which Windows does not ship by default. - Env-override tests no longer root the provider at /tmp/no-such-project (not guaranteed absent on every host); use a guaranteed-non-existent random path so the depth-5 filesystem scan short-circuits cleanly. --- ...rojectClasspathProviderEnvOverrideTest.java | 18 +++++++++++++----- .../ProcessBuilderMavenRunnerTest.java | 13 ++++++++++--- 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderEnvOverrideTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderEnvOverrideTest.java index 7fb808f..69ae2cd 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderEnvOverrideTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderEnvOverrideTest.java @@ -19,6 +19,14 @@ class LocalProjectClasspathProviderEnvOverrideTest { private static final String SEP = java.io.File.pathSeparator; + /** + * Guaranteed-non-existent working directory so the depth-5 filesystem scan short-circuits at the + * first {@code isDirectory} probe. A relative {@code /tmp/no-such-project} would be unreliable on + * hosts where that path happens to exist (or where {@code /tmp} contains + * {@code target/classes} subtrees from unrelated tooling). + */ + private static final Path NONEXISTENT_CWD = + Path.of("/nonexistent/jdwp-mcp-env-test-" + java.util.UUID.randomUUID()); @Test @DisplayName("parses File.pathSeparator-delimited env var into insertion-ordered entries") @@ -27,7 +35,7 @@ void shouldParsePathSeparatorDelimitedEnvVarIntoOrderedEntries() { // Splitting on `[;:]` is broken on Windows because `C:\foo` contains a colon — the env // var is parsed by the MCP server on the HOST OS, so the host separator is the right one. final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( - Path.of("/tmp/no-such-project"), + NONEXISTENT_CWD, envName -> "JDWP_EXTRA_CLASSPATH".equals(envName) ? "/opt/libs/a.jar" + SEP + "/opt/libs/b.jar" + SEP + "/srv/classes" : null, @@ -45,7 +53,7 @@ void shouldParsePathSeparatorDelimitedEnvVarIntoOrderedEntries() { @DisplayName("returns empty set when env var is an empty string") void shouldReturnEmptySetWhenEnvVarIsEmpty() { final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( - Path.of("/tmp/no-such-project"), + NONEXISTENT_CWD, envName -> "", (cmd, cwd, timeoutSeconds) -> List.of() ); @@ -57,7 +65,7 @@ void shouldReturnEmptySetWhenEnvVarIsEmpty() { @DisplayName("returns empty set when env lookup returns null (unset)") void shouldReturnEmptySetWhenEnvVarIsUnset() { final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( - Path.of("/tmp/no-such-project"), + NONEXISTENT_CWD, envName -> null, (cmd, cwd, timeoutSeconds) -> List.of() ); @@ -69,7 +77,7 @@ void shouldReturnEmptySetWhenEnvVarIsUnset() { @DisplayName("trims whitespace around entries and drops empty/blank tokens") void shouldTrimWhitespaceAndDropBlankEntries() { final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( - Path.of("/tmp/no-such-project"), + NONEXISTENT_CWD, envName -> " /a.jar " + SEP + SEP + " /b.jar ", (cmd, cwd, timeoutSeconds) -> List.of() ); @@ -87,7 +95,7 @@ void shouldTrimWhitespaceAndDropBlankEntries() { @DisplayName("breakdown reports zero env-override entries when env is set but only blanks") void shouldReportZeroEnvEntriesWhenEnvIsBlanksOnly() { final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( - Path.of("/tmp/no-such-project"), + NONEXISTENT_CWD, envName -> " " + SEP + " " + SEP + " ", (cmd, cwd, timeoutSeconds) -> List.of() ); diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunnerTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunnerTest.java index e7a9c87..f61e6c5 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunnerTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunnerTest.java @@ -28,11 +28,13 @@ void shouldOnlyHarvestOutputFilesUnderTargetParent(@TempDir Path tmp) throws Exc // parent directory is named `target/`. Maven writes there because mdep.outputFile is // `target/.jdwp-mcp-classpath`. Anything else is NOT ours and must be ignored. Files.createDirectories(tmp.resolve("target")); + // Use File.pathSeparator (':' on Unix, ';' on Windows) so the fixture matches what the + // harvester actually splits on; a hard-coded ':' breaks the test on Windows runners. Files.writeString(tmp.resolve("target/.jdwp-mcp-classpath"), - "/m2/foo.jar:/m2/bar.jar"); + String.join(java.io.File.pathSeparator, "/m2/foo.jar", "/m2/bar.jar")); Files.createDirectories(tmp.resolve("module-a/target")); Files.writeString(tmp.resolve("module-a/target/.jdwp-mcp-classpath"), - "/m2/baz.jar:/m2/foo.jar"); // overlap — must dedupe + String.join(java.io.File.pathSeparator, "/m2/baz.jar", "/m2/foo.jar")); // overlap — must dedupe // SAFETY: a file with the same name OUTSIDE a target/ dir (e.g. left over from a previous // buggy run, or a name collision with something the user has) MUST be ignored. @@ -89,12 +91,17 @@ void shouldReturnEmptyListWhenMavenExitsNonzero(@TempDir Path tmp) throws Except * {@link #shouldDestroyOwnedProcessTreeOnTimeout} below using the production code path. */ @Test + @org.junit.jupiter.api.condition.EnabledOnOs({ + org.junit.jupiter.api.condition.OS.LINUX, + org.junit.jupiter.api.condition.OS.MAC + }) @DisplayName("run() does NOT destroy unrelated sibling subprocesses when the executor throws InterruptedException") void shouldNotOverkillUnrelatedSubprocessesOnInterrupt(@TempDir Path tmp) throws Exception { Files.createDirectories(tmp.resolve("target")); // Sibling: a subprocess started by "some other component" before run() is invoked. It - // must survive run()'s cleanup — that's the invariant Copilot flagged. + // must survive run()'s cleanup — process ownership is scoped to the executor. Uses + // `sleep` so the test is Unix-only — guarded by @EnabledOnOs above. final Process sibling = new ProcessBuilder("sleep", "5").redirectErrorStream(true).start(); try { final ProcessBuilderMavenRunner runner = new ProcessBuilderMavenRunner(req -> { From 2aebf814a1f9e71ef0b99a4b3443e3937cd8381e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Thu, 28 May 2026 16:10:34 +0200 Subject: [PATCH 04/10] fix(evaluation): cross-OS portability audit fixes resolveMavenExecutable: - Skip the Unix `mvnw` probe on Windows. Files.isExecutable is ACL-based on Windows and may report a bare-name Unix shell script as executable, but ProcessBuilder can't actually invoke a Unix script on Windows. Now Windows only probes `mvnw.cmd`, then falls back to PATH `mvn` (which Windows resolves via PATHEXT). Test paths: - Replaced `Path.of("/nonexistent/...")` in 4 test seam helpers with `Path.of(System.getProperty("java.io.tmpdir"), "jdwp-mcp-...-UUID")`. Functionally equivalent on Linux/macOS (path that doesn't exist), but uses the platform-correct temp dir as the parent on every OS. --- .../LocalProjectClasspathProvider.java | 28 +++++++++++++------ .../edee/mcp/jdwp/JDWPToolsTestSupport.java | 5 +++- ...ssionEvaluatorLocalClasspathUnionTest.java | 9 +++--- ...ojectClasspathProviderEnvOverrideTest.java | 7 ++--- ...alProjectClasspathProviderTestSupport.java | 6 +++- 5 files changed, 36 insertions(+), 19 deletions(-) diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java index 5596d06..454adde 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java @@ -313,22 +313,32 @@ private List buildMavenCommand() { } /** - * Picks the Maven executable in priority order: {@code mvnw} (Unix wrapper) when executable, - * {@code mvnw.cmd} (Windows wrapper) when running on Windows and the file exists, then plain - * {@code mvn} from the PATH. {@code mvnw.cmd} cannot be probed with - * {@link Files#isExecutable(Path)} on Windows because the NTFS execute bit isn't a thing — - * {@link Files#isRegularFile(Path, LinkOption...)} is the right gate. + * Picks the Maven executable in priority order: + *

    + *
  • On Unix-likes (Linux/macOS): {@code mvnw} when executable, then PATH {@code mvn}.
    • + *
  • On Windows: {@code mvnw.cmd} when present, then PATH {@code mvn} (which Windows + * resolves to {@code mvn.cmd} via {@code PATHEXT}).
    • + *
+ *

The Unix-style {@code mvnw} probe is intentionally skipped on Windows: even though + * {@link Files#isExecutable(Path)} may still report the Unix shell script as executable (the + * Windows check is ACL-based, not extension-based), {@link ProcessBuilder} cannot invoke a + * shell script directly on Windows and the launch would fail. {@code mvnw.cmd} is the only + * sane wrapper choice there. + *

{@code mvnw.cmd} cannot be probed with {@link Files#isExecutable(Path)} because the + * NTFS executable bit isn't a thing — {@link Files#isRegularFile(Path, LinkOption...)} is the + * right gate. */ private String resolveMavenExecutable() { - final Path mvnw = workingDirectory.resolve("mvnw"); - if (Files.isExecutable(mvnw)) { - return mvnw.toAbsolutePath().toString(); - } if (isWindows()) { final Path mvnwCmd = workingDirectory.resolve("mvnw.cmd"); if (Files.isRegularFile(mvnwCmd)) { return mvnwCmd.toAbsolutePath().toString(); } + return "mvn"; + } + final Path mvnw = workingDirectory.resolve("mvnw"); + if (Files.isExecutable(mvnw)) { + return mvnw.toAbsolutePath().toString(); } return "mvn"; } diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsTestSupport.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsTestSupport.java index 04b448c..2601597 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsTestSupport.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsTestSupport.java @@ -61,8 +61,11 @@ static JDWPTools newToolsWithMocks() { * path fast and reproducible regardless of what {@code user.dir} happens to contain. */ static LocalProjectClasspathProvider defaultEmptyClasspathProvider() { + // Portable non-existent path: tmpdir is platform-correct (e.g. /tmp on Linux, + // C:\Users\...\AppData\Local\Temp on Windows); appending a UUID-named child that we + // never create gives us a guaranteed-absent root for the depth-5 scan to short-circuit on. return new LocalProjectClasspathProvider( - Path.of("/nonexistent/jdwp-mcp-default-empty-" + java.util.UUID.randomUUID()), + Path.of(System.getProperty("java.io.tmpdir"), "jdwp-mcp-default-empty-" + java.util.UUID.randomUUID()), name -> null, (command, workingDirectory, timeoutSeconds) -> List.of() ); diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorLocalClasspathUnionTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorLocalClasspathUnionTest.java index 4626185..3248a0b 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorLocalClasspathUnionTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorLocalClasspathUnionTest.java @@ -183,10 +183,11 @@ private static LocalProjectClasspathProvider stubLocalProvider(Set entri // this is the exact format the provider's env-override path expects. final String joined = String.join(File.pathSeparator, entries); return new LocalProjectClasspathProvider( - // Guaranteed-non-existent path so the depth-5 filesystem scan short-circuits at the - // first isDirectory probe. java.io.tmpdir is unsafe — CI machines and dev hosts can - // legitimately have target/classes subtrees there from unrelated tooling. - Path.of("/nonexistent/jdwp-mcp-union-test-" + java.util.UUID.randomUUID()), + // Portable non-existent root: tmpdir is platform-correct on every host; appending a + // UUID-named child we never create gives a guaranteed-absent root, so the depth-5 + // scan short-circuits at the first isDirectory probe and never walks into whatever + // tmpdir itself contains. + Path.of(System.getProperty("java.io.tmpdir"), "jdwp-mcp-union-test-" + java.util.UUID.randomUUID()), name -> "JDWP_EXTRA_CLASSPATH".equals(name) ? joined : null, (c, d, t) -> List.of() ); diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderEnvOverrideTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderEnvOverrideTest.java index 69ae2cd..3a05715 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderEnvOverrideTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderEnvOverrideTest.java @@ -21,12 +21,11 @@ class LocalProjectClasspathProviderEnvOverrideTest { private static final String SEP = java.io.File.pathSeparator; /** * Guaranteed-non-existent working directory so the depth-5 filesystem scan short-circuits at the - * first {@code isDirectory} probe. A relative {@code /tmp/no-such-project} would be unreliable on - * hosts where that path happens to exist (or where {@code /tmp} contains - * {@code target/classes} subtrees from unrelated tooling). + * first {@code isDirectory} probe. Built from {@code java.io.tmpdir} (platform-correct on every + * host) with a UUID-named child we never create — portable across Linux, macOS, and Windows. */ private static final Path NONEXISTENT_CWD = - Path.of("/nonexistent/jdwp-mcp-env-test-" + java.util.UUID.randomUUID()); + Path.of(System.getProperty("java.io.tmpdir"), "jdwp-mcp-env-test-" + java.util.UUID.randomUUID()); @Test @DisplayName("parses File.pathSeparator-delimited env var into insertion-ordered entries") diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderTestSupport.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderTestSupport.java index 897f9b9..52b152e 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderTestSupport.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderTestSupport.java @@ -23,8 +23,12 @@ private LocalProjectClasspathProviderTestSupport() { * unrelated {@code target/classes} trees there). */ static LocalProjectClasspathProvider noOpProvider() { + // Portable non-existent path: tmpdir is platform-correct (/tmp on Linux, + // C:\Users\...\AppData\Local\Temp on Windows); appending a UUID-named child we never + // create gives a guaranteed-absent root so the depth-5 scan short-circuits at the first + // isDirectory probe. return new LocalProjectClasspathProvider( - Path.of("/nonexistent/jdwp-mcp-no-op-" + java.util.UUID.randomUUID()), + Path.of(System.getProperty("java.io.tmpdir"), "jdwp-mcp-no-op-" + java.util.UUID.randomUUID()), name -> null, (command, workingDirectory, timeoutSeconds) -> List.of() ); From 85ef01e5b041efbe92d608319ebd1cdcf1e3c9f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Thu, 28 May 2026 16:19:14 +0200 Subject: [PATCH 05/10] fix(evaluation): prune skip-dirs at descent + accurate merge log MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Harvester I/O - Rewrote harvestOutputFiles using Files.walkFileTree with preVisitDirectory pruning. Skip-listed directories (node_modules, .git, .idea, .gradle, .mvn, .vscode) now return SKIP_SUBTREE on descent so their contents are never enumerated. On large repos the previous Files.walk(root, 5) + post-filter could enumerate tens of thousands of files inside node_modules/ alone before discarding them. - Symlink rejection now uses attrs.isRegularFile() from the visitor's BasicFileAttributes (no FOLLOW_LINKS option) instead of an extra Files.isRegularFile call. - Extracted readOutputFile helper so the visitor body stays focused. Merge log accuracy - The previous INFO line said "{} local-only entries" but reported localEntries.size(), which includes entries that overlap with the remote classpath and are deduped during merge. Now reports three numbers: "{} remote + {} local entries → {} merged" so an operator can see the overlap explicitly (merged < remote + local). --- .../evaluation/JdiExpressionEvaluator.java | 10 +- .../evaluation/ProcessBuilderMavenRunner.java | 143 ++++++++++-------- 2 files changed, 88 insertions(+), 65 deletions(-) diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java index 589192f..881d998 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java @@ -1203,8 +1203,14 @@ public synchronized void configureCompilerClasspath(ThreadReference suspendedThr final long mergeStart = System.currentTimeMillis(); final Set localEntries = localClasspathProvider.discover(); final String mergedClasspath = mergeClasspaths(remoteClasspath, localEntries); - log.info("[LocalClasspath] Merged classpath: {} remote + {} local-only entries in {}ms", - countEntries(remoteClasspath), localEntries.size(), + // Report three numbers so an operator can see overlap explicitly: + // remote + local are the raw source counts; merged is the deduped union actually + // handed to the compiler. Some local entries may overlap with remote (live target VM + // and local project share JARs) — the overlap shows up as "merged < remote + local". + final int remoteCount = countEntries(remoteClasspath); + final int mergedCount = countEntries(mergedClasspath); + log.info("[LocalClasspath] Merged classpath: {} remote + {} local entries → {} merged in {}ms", + remoteCount, localEntries.size(), mergedCount, System.currentTimeMillis() - mergeStart); if (mergedClasspath.isEmpty()) { diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java index b14752c..a65f5a7 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java @@ -8,16 +8,19 @@ import java.io.File; import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.nio.file.FileVisitOption; +import java.nio.file.FileVisitResult; import java.nio.file.Files; -import java.nio.file.LinkOption; import java.nio.file.Path; +import java.nio.file.SimpleFileVisitor; +import java.nio.file.attribute.BasicFileAttributes; import java.util.ArrayList; +import java.util.EnumSet; import java.util.LinkedHashSet; import java.util.List; import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.regex.Pattern; -import java.util.stream.Stream; /** * Production {@link LocalProjectClasspathProvider.MavenRunner} that shells out to {@code mvn} (or @@ -89,84 +92,98 @@ public List run(List command, Path workingDirectory, int timeout * Maven-written classpath files and aggregates their entries. * *

File-safety invariant. A candidate file is accepted ONLY if BOTH conditions hold: - * its direct parent directory is named {@code target} AND no ancestor segment between + * its direct parent directory is named {@code target}, AND no ancestor directory between * {@code root} and the candidate is in {@link LocalProjectClasspathProvider#HARVEST_SKIP_DIRS}. * This gate keeps the harvester confined to Maven-owned build output even on projects that * happen to contain a stray file with the matching name elsewhere in the tree. The harvester * NEVER deletes; the next Maven run overwrites the file in place and {@code mvn clean} removes * it. + * + *

Uses {@link Files#walkFileTree} with {@code preVisitDirectory} pruning so skip-listed + * directories ({@code node_modules}, {@code .git}, {@code .idea}, {@code .gradle}, etc.) + * are never descended into — a meaningful saving on large repos where {@code node_modules/} + * alone can hold tens of thousands of files. Symlinks are not followed because no + * {@link FileVisitOption#FOLLOW_LINKS} is passed. */ private static List harvestOutputFiles(Path root) throws IOException { final Set entries = new LinkedHashSet<>(); - // File-safety: ONLY accept candidates whose direct parent directory is named `target`. - // Maven-owned build output, gitignored, wiped by `mvn clean`. A bare match on file name - // anywhere in the tree would risk picking up a file we did not create — see the - // file-safety policy. Depth-5 covers: ///target/. - try (Stream walk = Files.walk(root, 5)) { - walk.filter(p -> p.getFileName() != null - && OUTPUT_FILE_NAME.equals(p.getFileName().toString()) - && p.getParent() != null - && p.getParent().getFileName() != null - && "target".equals(p.getParent().getFileName().toString()) - && !hasSkippedAncestor(root, p) - // Symlinks must not be followed: a symlink at target/.jdwp-mcp-classpath - // could point at any file on the host. The harvester is invariant-bound - // to read only Maven-written output it found in-tree. - && Files.isRegularFile(p, LinkOption.NOFOLLOW_LINKS)) - .forEach(file -> { - try { - final String content = Files.readString(file).trim(); - if (content.isEmpty()) { - log.debug("[LocalClasspath] Output file {} was empty", file); - return; - } - int parsed = 0; - // Use the HOST File.pathSeparator: Maven wrote this file on the host, so - // its entries are separated by the host's separator. Splitting on `[;:]` - // would shred `C:\foo` on Windows. - final String separator = Pattern.quote(File.pathSeparator); - for (String part : content.split(separator, -1)) { - final String trimmed = part.trim(); - if (!trimmed.isEmpty() && entries.add(trimmed)) { - parsed++; - } - } - log.debug("[LocalClasspath] Harvested {} new entries from {}", parsed, file); - // NO DELETION. The file is in a Maven-owned `target/` directory; the next - // Maven run overwrites it in place, and `mvn clean` removes it. Deleting - // arbitrary files in the project tree is forbidden by the file-safety policy. - } catch (IOException e) { - log.warn("[LocalClasspath] Could not read classpath output {}: {}: {}", - file, e.getClass().getSimpleName(), e.getMessage()); + Files.walkFileTree(root, EnumSet.noneOf(FileVisitOption.class), 5, new SimpleFileVisitor() { + @Override + public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) { + // Don't skip the root itself even if its name happens to match a SKIP_DIR. + if (!dir.equals(root)) { + final Path name = dir.getFileName(); + if (name != null + && LocalProjectClasspathProvider.HARVEST_SKIP_DIRS.contains(name.toString())) { + return FileVisitResult.SKIP_SUBTREE; } - }); - } + } + return FileVisitResult.CONTINUE; + } + + @Override + public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) { + // Symlinks must not be followed: a symlink at target/.jdwp-mcp-classpath could + // point at any file on the host. attrs.isRegularFile() is false for symlinks + // because we did NOT pass FOLLOW_LINKS. + if (!attrs.isRegularFile()) { + return FileVisitResult.CONTINUE; + } + final Path name = file.getFileName(); + if (name == null || !OUTPUT_FILE_NAME.equals(name.toString())) { + return FileVisitResult.CONTINUE; + } + final Path parent = file.getParent(); + if (parent == null || parent.getFileName() == null + || !"target".equals(parent.getFileName().toString())) { + return FileVisitResult.CONTINUE; + } + readOutputFile(file, entries); + return FileVisitResult.CONTINUE; + } + + @Override + public FileVisitResult visitFileFailed(Path file, IOException exc) { + // Permission denied / transient I/O on a single entry must not abort the whole + // walk — log at debug and continue, matching the "partial classpath beats no + // classpath" policy used by the provider's filesystem scan. + log.debug("[LocalClasspath] Skipping unreadable path {}: {}", file, exc.getMessage()); + return FileVisitResult.CONTINUE; + } + }); return new ArrayList<>(entries); } /** - * Rejects candidates whose path between {@code root} (exclusive) and the candidate's parent - * (inclusive) contains a segment listed in - * {@link LocalProjectClasspathProvider#HARVEST_SKIP_DIRS}. Shares the provider's skip list so - * additions there extend both code paths uniformly. + * Reads a Maven-written {@code .jdwp-mcp-classpath} file, splits on the host + * {@link File#pathSeparator}, and appends non-blank, non-duplicate entries to {@code entries}. + * Maven wrote this file on the host so its entries are separated by the host's separator — + * splitting on {@code [;:]} would shred {@code C:\foo} on Windows. + * + *

NO DELETION. The file lives in a Maven-owned {@code target/} directory; the next Maven + * run overwrites it in place and {@code mvn clean} removes it. Deleting arbitrary files in + * the project tree is forbidden by the file-safety policy. */ - private static boolean hasSkippedAncestor(Path root, Path candidate) { - final Path relative; + private static void readOutputFile(Path file, Set entries) { try { - relative = root.relativize(candidate); - } catch (IllegalArgumentException e) { - // Different filesystem roots; cannot relativize. Fall back to skipping the candidate. - return true; - } - // Iterate every segment except the final file name — only directories count as ancestors. - final int segments = relative.getNameCount(); - for (int i = 0; i < segments - 1; i++) { - final String name = relative.getName(i).toString(); - if (LocalProjectClasspathProvider.HARVEST_SKIP_DIRS.contains(name)) { - return true; + final String content = Files.readString(file).trim(); + if (content.isEmpty()) { + log.debug("[LocalClasspath] Output file {} was empty", file); + return; + } + int parsed = 0; + final String separator = Pattern.quote(File.pathSeparator); + for (String part : content.split(separator, -1)) { + final String trimmed = part.trim(); + if (!trimmed.isEmpty() && entries.add(trimmed)) { + parsed++; + } } + log.debug("[LocalClasspath] Harvested {} new entries from {}", parsed, file); + } catch (IOException e) { + log.warn("[LocalClasspath] Could not read classpath output {}: {}: {}", + file, e.getClass().getSimpleName(), e.getMessage()); } - return false; } /** From 8b1830df6f006c51cd6e642dc4edda5eef83f5f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Thu, 28 May 2026 16:33:18 +0200 Subject: [PATCH 06/10] fix(evaluation): deeper reactor harvest + symlink-safe pom probes - Harvester walk cap is now scan-depth + 2 (= 7) so modules at the filesystem-scan boundary still expose their target/.jdwp-mcp-classpath. - Every pom.xml probe (hasPomAtRoot, Maven gate, empty-result log) now uses LinkOption.NOFOLLOW_LINKS for consistency with the rest of the provider's symlink-not-followed invariant. - Realigned the misleading "temp dir + pom.xml fixture" comment in the cold-cache diagnose test. Regression tests: - target/.jdwp-mcp-classpath at /a/b/c/d/e/target (depth 7). - Symlinked pom.xml at the working directory must not trigger Maven. --- .../LocalProjectClasspathProvider.java | 22 ++++++++--- .../evaluation/ProcessBuilderMavenRunner.java | 18 +++++++-- .../edee/mcp/jdwp/JDWPToolsDiagnoseTest.java | 6 ++- ...ocalProjectClasspathProviderMavenTest.java | 38 +++++++++++++++++++ .../ProcessBuilderMavenRunnerTest.java | 26 +++++++++++++ 5 files changed, 101 insertions(+), 9 deletions(-) diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java index 454adde..ca4ae8a 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java @@ -151,7 +151,11 @@ public Path getWorkingDirectory() { * duplicating the path logic. */ public boolean hasPomAtRoot() { - return Files.isRegularFile(workingDirectory.resolve("pom.xml")); + // NOFOLLOW_LINKS: the provider's invariant is "symlinks are never followed". A symlinked + // pom.xml could redirect Maven discovery at an arbitrary tree on the host filesystem; we + // treat it as "no pom" so the Maven source is skipped, matching how the filesystem scan + // treats symlinked target/classes directories. + return Files.isRegularFile(workingDirectory.resolve("pom.xml"), LinkOption.NOFOLLOW_LINKS); } /** @@ -233,12 +237,17 @@ public synchronized Breakdown discoverWithBreakdown() { cachedBreakdown = breakdown; if (entries.isEmpty()) { // Required by the logging policy: ONE INFO line on empty result explaining the why. + // NOFOLLOW_LINKS on every probe — keep the diagnostic readout consistent with the + // "symlinks not followed" invariant used by every other filesystem probe in this class. + final boolean pomPresent = + Files.isRegularFile(workingDirectory.resolve("pom.xml"), LinkOption.NOFOLLOW_LINKS); log.info("[LocalClasspath] discover() found 0 entries — env={}, fs={}, maven={} (cwd={}, pom.xml={})", envLookup.apply(ENV_NAME) == null ? "unset" : "set-but-empty", - Files.isDirectory(workingDirectory.resolve("target")) ? "target/-present" : "no-target/", - Files.isRegularFile(workingDirectory.resolve("pom.xml")) ? "pom-present" : "no-pom", + Files.isDirectory(workingDirectory.resolve("target"), LinkOption.NOFOLLOW_LINKS) + ? "target/-present" : "no-target/", + pomPresent ? "pom-present" : "no-pom", workingDirectory, - Files.isRegularFile(workingDirectory.resolve("pom.xml"))); + pomPresent); } return breakdown; } @@ -259,7 +268,10 @@ public synchronized void reset() { * outcome (added > 0, added == 0, failure) so operators see one line per discovery. */ private void addMavenDependencies(Set entries) { - if (!Files.isRegularFile(workingDirectory.resolve("pom.xml"))) { + // NOFOLLOW_LINKS: same invariant as hasPomAtRoot — a symlinked pom.xml must not trigger + // Maven discovery, because Maven would resolve the link and pull dependencies from a tree + // outside the configured workingDirectory. + if (!Files.isRegularFile(workingDirectory.resolve("pom.xml"), LinkOption.NOFOLLOW_LINKS)) { log.debug("[LocalClasspath] No pom.xml under {} — skipping Maven source", workingDirectory); return; } diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java index a65f5a7..e15994b 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java @@ -46,6 +46,13 @@ public class ProcessBuilderMavenRunner implements LocalProjectClasspathProvider. private static final String OUTPUT_FILE_NAME = ".jdwp-mcp-classpath"; /** Upper bound on captured child stdout/stderr included in the WARN log on failure. */ private static final int STDOUT_CAPTURE_BYTES = 64 * 1024; + /** + * Walk cap for the classpath-file harvester: the provider's filesystem scan depth ({@code 5}) + * plus two — covering {@code /target/.jdwp-mcp-classpath} for modules sitting at the + * scan boundary. Keeping it relative to the scan depth (rather than a magic literal) keeps the + * two limits coupled: a future bump to one is mirrored in the other. + */ + private static final int HARVEST_MAX_DEPTH = 7; private static final Logger log = LoggerFactory.getLogger(ProcessBuilderMavenRunner.class); /** Seam for the actual process execution; replaced in tests to drive timeout / failure paths. */ @@ -88,8 +95,13 @@ public List run(List command, Path workingDirectory, int timeout } /** - * Walks {@code root} (depth 5, covering {@code ///target/}) for - * Maven-written classpath files and aggregates their entries. + * Walks {@code root} for Maven-written classpath files and aggregates their entries. + * + *

Depth. The walk's max depth is {@link #HARVEST_MAX_DEPTH} (= filesystem-scan depth + * + 2), so a module discovered at the scan boundary still has its {@code target/} + * reachable. Counting from the root, {@code /a/b/c/d/e/target/.jdwp-mcp-classpath} is + * seven levels deep; the previous fixed depth of 5 would silently drop reactor modules at the + * deeper boundary even though the scan correctly discovered them. * *

File-safety invariant. A candidate file is accepted ONLY if BOTH conditions hold: * its direct parent directory is named {@code target}, AND no ancestor directory between @@ -107,7 +119,7 @@ public List run(List command, Path workingDirectory, int timeout */ private static List harvestOutputFiles(Path root) throws IOException { final Set entries = new LinkedHashSet<>(); - Files.walkFileTree(root, EnumSet.noneOf(FileVisitOption.class), 5, new SimpleFileVisitor() { + Files.walkFileTree(root, EnumSet.noneOf(FileVisitOption.class), HARVEST_MAX_DEPTH, new SimpleFileVisitor() { @Override public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) { // Don't skip the root itself even if its name happens to match a SKIP_DIR. diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsDiagnoseTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsDiagnoseTest.java index 4867999..199c869 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsDiagnoseTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsDiagnoseTest.java @@ -364,7 +364,11 @@ void shouldRenderLocalClasspathSectionWithoutBlockingOnColdCache() { return List.of("/m2/slow.jar"); } ); - // Write a pom.xml-like fixture into a fresh temp dir so the slow Maven path is even reached. + // The CWD is the project root (which has a real pom.xml), so the slow Maven runner WOULD be + // triggered if diagnose ran a full discovery. The point of this test is that it doesn't: + // the diagnose path peeks the cold cache and renders "not yet computed", never invoking + // the runner. If the fast path regresses, the slow stub's 2s sleep blows the 500ms budget + // below. final JDWPTools toolsWithSlowStub = JDWPToolsTestSupport.newTools( jdiService, new BreakpointTracker(), new WatcherManager(), mock(JdiExpressionEvaluator.class), new EventHistory(), new EvaluationGuard(), diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderMavenTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderMavenTest.java index 7255f4e..3955287 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderMavenTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderMavenTest.java @@ -146,6 +146,44 @@ void shouldSkipMavenWhenNoPomXml(@TempDir Path tmp) { * sync, broken build) — the provider must still invoke Maven (it's the runner's job to deal * with the failure) and tolerate an empty result list without throwing. */ + /** + * Symlink-safety: a {@code pom.xml} that is itself a symbolic link must NOT trigger Maven + * discovery. Maven would resolve the link and pull dependencies from the linked target, which + * may live outside the configured working directory — the provider's documented invariant is + * "symlinks are never followed". + */ + @Test + @org.junit.jupiter.api.condition.EnabledOnOs({ + org.junit.jupiter.api.condition.OS.LINUX, + org.junit.jupiter.api.condition.OS.MAC + }) + @DisplayName("skips Maven when pom.xml at the working directory is a symlink") + void shouldSkipMavenWhenPomXmlIsSymlink(@TempDir Path tmp) throws Exception { + // Real pom in a sibling directory; the provider's working directory holds a SYMLINK to it. + final Path realPomDir = tmp.resolve("real-project"); + Files.createDirectories(realPomDir); + Files.writeString(realPomDir.resolve("pom.xml"), ""); + + final Path workDir = tmp.resolve("workdir"); + Files.createDirectories(workDir); + Files.createSymbolicLink(workDir.resolve("pom.xml"), realPomDir.resolve("pom.xml")); + + final AtomicReference invoked = new AtomicReference<>(false); + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + workDir, envName -> null, + (cmd, cwd, t) -> { invoked.set(true); return List.of(); } + ); + + provider.discover(); + + assertThat(invoked.get()) + .as("Symlinked pom.xml must not trigger Maven — symlinks-not-followed invariant") + .isFalse(); + assertThat(provider.hasPomAtRoot()) + .as("hasPomAtRoot() must also report false for a symlinked pom.xml — same invariant") + .isFalse(); + } + @Test @DisplayName("invokes Maven when pom.xml is present even if empty; tolerates empty result list") void shouldInvokeMavenWhenPomXmlIsEmpty(@TempDir Path tmp) throws Exception { diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunnerTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunnerTest.java index f61e6c5..6342144 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunnerTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunnerTest.java @@ -224,6 +224,32 @@ void shouldSkipNodeModulesDuringHarvest(@TempDir Path tmp) throws Exception { .doesNotContain("/from/node_modules/should-not-be-harvested.jar"); } + /** + * Deep-reactor regression: a module discovered at the filesystem scan's depth boundary (5 levels + * below {@code workingDirectory}) puts its {@code target/.jdwp-mcp-classpath} two more levels + * down — total depth 7. A shallower harvest cap silently drops Maven-resolved dependencies for + * legitimate reactor layouts. + */ + @Test + @DisplayName("harvests .jdwp-mcp-classpath at reactor-boundary depth (module at scan depth 5 → file at depth 7)") + void shouldHarvestOutputFileAtReactorBoundaryDepth(@TempDir Path tmp) throws Exception { + // Path: /a/b/c/d/e/target/.jdwp-mcp-classpath + // Counting from as depth 0: a=1, b=2, c=3, d=4, e=5 (matches MAX_SCAN_DEPTH=5), + // target=6, file=7. The harvest cap must reach depth 7. + final Path boundaryModule = tmp.resolve("a/b/c/d/e/target"); + Files.createDirectories(boundaryModule); + Files.writeString(boundaryModule.resolve(".jdwp-mcp-classpath"), + String.join(java.io.File.pathSeparator, "/m2/deep.jar")); + + final ProcessBuilderMavenRunner runner = new ProcessBuilderMavenRunner(req -> 0); + + final List result = runner.run(List.of("echo"), tmp, 10); + + assertThat(result) + .as("Maven output at /a/b/c/d/e/target/.jdwp-mcp-classpath must be reachable — depth 7") + .containsExactly("/m2/deep.jar"); + } + /** * On Windows the wrapper script is {@code mvnw.cmd}, not {@code mvnw}. The provider must probe * for both, preferring {@code mvnw} (Unix) when present and falling back to {@code mvnw.cmd} on From d02c25d10bb232df44f3cb0cafa44c2d3020d5fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Thu, 28 May 2026 19:37:08 +0200 Subject: [PATCH 07/10] fix(evaluation): symlink-safe wrapper probes + leak-proof process tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - resolveMavenExecutable now uses LinkOption.NOFOLLOW_LINKS on both the Windows mvnw.cmd probe and the Unix mvnw probe. A symlinked wrapper could redirect Maven execution at a script outside the working directory, defeating the symlinks-never-followed invariant the rest of the provider enforces. - destroyProcessTree wraps process.descendants() in try/catch and moves the root destroyForcibly() into finally. Under restricted security policies descendants() can throw; with the previous flow that would leave the Maven subprocess running — exactly the leak the cleanup exists to prevent. Regression tests: - Symlinked mvnw at the working directory falls back to PATH 'mvn'. - Stub Process whose descendants() throws still gets destroyForcibly()d. --- .../LocalProjectClasspathProvider.java | 11 ++++- .../evaluation/ProcessBuilderMavenRunner.java | 30 +++++++++++--- ...ocalProjectClasspathProviderMavenTest.java | 36 +++++++++++++++++ .../ProcessBuilderMavenRunnerTest.java | 40 +++++++++++++++++++ 4 files changed, 110 insertions(+), 7 deletions(-) diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java index ca4ae8a..31525cb 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java @@ -343,13 +343,20 @@ private List buildMavenCommand() { private String resolveMavenExecutable() { if (isWindows()) { final Path mvnwCmd = workingDirectory.resolve("mvnw.cmd"); - if (Files.isRegularFile(mvnwCmd)) { + // NOFOLLOW_LINKS: a symlinked mvnw.cmd could point at a script outside the working + // directory and redirect Maven execution there, defeating the "symlinks never + // followed" invariant the rest of this class enforces. + if (Files.isRegularFile(mvnwCmd, LinkOption.NOFOLLOW_LINKS)) { return mvnwCmd.toAbsolutePath().toString(); } return "mvn"; } final Path mvnw = workingDirectory.resolve("mvnw"); - if (Files.isExecutable(mvnw)) { + // Same invariant: reject a symlinked wrapper outright. Files.isExecutable() FOLLOWS + // symlinks, so we gate on isRegularFile(..., NOFOLLOW_LINKS) first; only when the entry + // is a real file do we ask whether it is executable. (isExecutable then follows the link + // to query the ACL, but at that point we have already proven it is not a link.) + if (Files.isRegularFile(mvnw, LinkOption.NOFOLLOW_LINKS) && Files.isExecutable(mvnw)) { return mvnw.toAbsolutePath().toString(); } return "mvn"; diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java index e15994b..a9f60be 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java @@ -287,12 +287,32 @@ private static int executeRealCommand(CommandRequest req) throws IOException, In * Forcibly destroys the given process and every descendant in its tree, then awaits each one's * exit for up to 2s. Bounded to the started process — does NOT touch sibling subprocesses * owned by other components, which a {@code ProcessHandle.current().descendants()} sweep would. + * + *

Descendant enumeration is best-effort: {@link Process#descendants()} can throw under + * restricted security policies or when handle inspection is unavailable. A failure there must + * NOT prevent root-process cleanup — that would leak the Maven invocation we started, the very + * thing this method exists to prevent. The root {@code destroyForcibly()} therefore runs in + * {@code finally}, after a logged best-effort sweep of descendants. */ - private static void destroyProcessTree(Process process) { - final List tree = process.descendants().toList(); - // Kill children first so the parent can't re-fork during the window between the kill calls. - tree.forEach(ProcessHandle::destroyForcibly); - process.destroyForcibly(); + // Package-private so the regression test can exercise the descendants()-throwing path + // with a stub Process; production callers use it through the timeout / interrupt branches above. + static void destroyProcessTree(Process process) { + List tree = List.of(); + try { + tree = process.descendants().toList(); + // Kill children first so the parent can't re-fork during the window between the kill calls. + tree.forEach(ProcessHandle::destroyForcibly); + } catch (Exception e) { + // Restricted env, unavailable handles, anything else — log and fall through to root + // cleanup. The root destroy below still runs because it is in finally. + log.warn("[LocalClasspath] Could not enumerate / destroy descendants of Maven process: {}: {}", + e.getClass().getSimpleName(), e.getMessage()); + } finally { + // ALWAYS destroy the root — even if descendant enumeration above blew up. Skipping this + // would leak the Maven subprocess we own, which is exactly the failure mode the caller + // (timeout or interrupt path) is trying to prevent. + process.destroyForcibly(); + } for (ProcessHandle h : tree) { try { h.onExit().get(2, TimeUnit.SECONDS); diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderMavenTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderMavenTest.java index 3955287..e7ed1fc 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderMavenTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderMavenTest.java @@ -113,6 +113,42 @@ void shouldInvokeMavenRunnerWithA180SecondTimeout(@TempDir Path tmp) throws Exce assertThat(capturedTimeout.get()).isEqualTo(180); } + /** + * Symlink-safety for the Maven wrapper: a {@code mvnw} that is itself a symlink (e.g. to a + * script outside the working directory) must NOT be selected as the executable. The provider + * must fall back to PATH {@code mvn} instead, matching the symlinks-never-followed invariant + * enforced everywhere else in this class. Linux/macOS only — Windows can't create symlinks + * without elevated rights, and the Windows path is exercised by its own test. + */ + @Test + @org.junit.jupiter.api.condition.EnabledOnOs({ + org.junit.jupiter.api.condition.OS.LINUX, + org.junit.jupiter.api.condition.OS.MAC + }) + @DisplayName("rejects a symlinked mvnw wrapper and falls back to PATH 'mvn'") + void shouldRejectSymlinkedMvnwAndFallBackToPathMvn(@TempDir Path tmp) throws Exception { + Files.writeString(tmp.resolve("pom.xml"), ""); + + // Real wrapper script in a sibling dir; the working directory hosts only a SYMLINK to it. + final Path realMvnwDir = tmp.resolve("real-tools"); + Files.createDirectories(realMvnwDir); + final Path realMvnw = realMvnwDir.resolve("mvnw"); + Files.writeString(realMvnw, "#!/bin/sh\necho 'should not be invoked'\n"); + realMvnw.toFile().setExecutable(true); + + Files.createSymbolicLink(tmp.resolve("mvnw"), realMvnw); + + final AtomicReference> capturedCmd = new AtomicReference<>(); + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, (cmd, cwd, t) -> { capturedCmd.set(cmd); return List.of(); } + ); + provider.discover(); + + assertThat(capturedCmd.get().get(0)) + .as("Symlinked mvnw must be rejected — symlinks-not-followed invariant; fall back to PATH 'mvn'") + .isEqualTo("mvn"); + } + @Test @DisplayName("prefers an executable ./mvnw wrapper over a bare 'mvn'") void shouldPreferMvnwOverMvn(@TempDir Path tmp) throws Exception { diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunnerTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunnerTest.java index 6342144..08a8e9c 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunnerTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunnerTest.java @@ -250,6 +250,46 @@ void shouldHarvestOutputFileAtReactorBoundaryDepth(@TempDir Path tmp) throws Exc .containsExactly("/m2/deep.jar"); } + /** + * Restricted-environment safety: {@link Process#descendants()} can throw under tight security + * policies or when handle inspection is unavailable. If that happens during a timeout or + * interrupt, the root Maven process must still be destroyed — otherwise the very subprocess + * we are trying to clean up leaks. The fix puts {@code root.destroyForcibly()} in a + * {@code finally} so it runs regardless of how descendant enumeration fails. + */ + @Test + @DisplayName("destroyProcessTree still destroys the root when descendants() throws") + void shouldDestroyRootEvenIfDescendantsThrows() { + final java.util.concurrent.atomic.AtomicBoolean rootDestroyed = new java.util.concurrent.atomic.AtomicBoolean(false); + + // Stub Process: descendants() blows up to simulate a restricted env where handle inspection + // is unavailable. Only the methods destroyProcessTree actually touches are overridden. + final Process stub = new Process() { + @Override public java.io.OutputStream getOutputStream() { return java.io.OutputStream.nullOutputStream(); } + @Override public java.io.InputStream getInputStream() { return java.io.InputStream.nullInputStream(); } + @Override public java.io.InputStream getErrorStream() { return java.io.InputStream.nullInputStream(); } + @Override public int waitFor() { return 0; } + @Override public int exitValue() { return 0; } + @Override public void destroy() { /* unused by destroyProcessTree */ } + @Override public Process destroyForcibly() { + rootDestroyed.set(true); + return this; + } + @Override public java.util.stream.Stream descendants() { + throw new SecurityException("simulated restricted environment"); + } + @Override public java.util.concurrent.CompletableFuture onExit() { + return java.util.concurrent.CompletableFuture.completedFuture(this); + } + }; + + ProcessBuilderMavenRunner.destroyProcessTree(stub); + + assertThat(rootDestroyed.get()) + .as("Root process must be destroyed even when descendants() throws — no leaked Maven subprocess") + .isTrue(); + } + /** * On Windows the wrapper script is {@code mvnw.cmd}, not {@code mvnw}. The provider must probe * for both, preferring {@code mvnw} (Unix) when present and falling back to {@code mvnw.cmd} on From b46d17fe51d4e6a9b7a0762f31988de5b5e7c88f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Thu, 28 May 2026 21:07:08 +0200 Subject: [PATCH 08/10] fix(evaluation): defer Maven-backed local classpath off the event thread Address the latest Copilot review on PR #33 (commit d02c25d): - prewarmClasspath() ran the full configureCompilerClasspath() from the JDI event-listener thread, which invokes the local provider and can shell out to Maven (180s timeout). The first breakpoint hit could thus stall event processing for minutes even if the user never evaluated anything. Split configure into a public (full) form and a private (thread, includeLocal) worker; prewarm passes includeLocal=false to warm only remote classpath + JDK detection. A new localClasspathMerged flag lets the first real evaluation/logpoint/condition fall through the short-circuit and complete the local merge on the MCP worker thread. The cache-reset block is guarded on !jdkReady so the deferred-merge pass never wipes valid remote discovery. - Reword the "Maven contributed 0 jars" INFO line so zero jars no longer implies failure (a project with no runtime deps is a valid outcome). - Move an orphaned Javadoc block onto the empty-pom test it describes. Tests: update shouldShortCircuitWhenAlreadyConfigured for the new "fully configured = JDK discovered AND local merged" semantics; add prewarmDefersLocalMergeUntilFirstEvaluation. Document the deferral in docs/expression-evaluation.md. Full suite: 1067 passing. --- docs/expression-evaluation.md | 2 +- .../evaluation/JdiExpressionEvaluator.java | 91 +++++++++++++++---- .../LocalProjectClasspathProvider.java | 10 +- ...essionEvaluatorConfigureClasspathTest.java | 43 ++++++++- ...ocalProjectClasspathProviderMavenTest.java | 10 +- 5 files changed, 127 insertions(+), 29 deletions(-) diff --git a/docs/expression-evaluation.md b/docs/expression-evaluation.md index 4810ac8..78cdbc7 100644 --- a/docs/expression-evaluation.md +++ b/docs/expression-evaluation.md @@ -90,7 +90,7 @@ The remote classloader walk above is the source of truth, but it does not see ev - **Source/binary drift.** If the local checkout is on a different commit than the running target, the *remote* definition still wins because of merge order — but any class missing from the remote view binds against whatever the local jar contains. Rebuild locally to align if eval results look stale. - **Gradle is not supported in v1.** Only Maven layouts contribute via the Maven source; Gradle projects fall back to filesystem scan + env-override only. -- **First call is slow.** Cold-cache Maven takes 1–3 minutes; the result is memoized for the JDI connection's lifetime and invalidated on the same edges that reset `InMemoryJavaCompiler` (disconnect / first use of a fresh connection). +- **First call is slow, but never on the event thread.** Cold-cache Maven takes 1–3 minutes; the result is memoized for the JDI connection's lifetime and invalidated on the same edges that reset `InMemoryJavaCompiler` (disconnect / first use of a fresh connection). The speculative `prewarmClasspath` — which fires on the JDI event-listener thread the instant a breakpoint hits — warms **only** the remote classpath + JDK detection and skips the local provider entirely, so a Maven shell-out can never stall event processing for a user who never evaluates anything. The local fallback (including Maven) is paid lazily on the **first actual evaluation / logpoint / condition**, on the MCP worker thread. - **No targeted-module mode.** All modules detected in the reactor union into one classpath; the provider does not try to match the breakpoint frame back to its owning module. **Inspection.** `jdwp_diagnose` shows a `Local project classpath` block listing the CWD, whether a `pom.xml` was found, a per-source breakdown (`env-override=N, filesystem=N, maven=N`), the total entry count, and whether the env-var override is set. The provider also logs every step under the `[LocalClasspath]` prefix — `INFO` for source contributions and timings, `WARN` for Maven non-zero exits / timeouts / malformed env values, `DEBUG` for per-entry tracing and Maven stdout on failure. diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java index 881d998..1dfd141 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java @@ -84,6 +84,16 @@ public class JdiExpressionEvaluator { */ private final Map compilationCache = new ConcurrentHashMap<>(); + /** + * Whether the (potentially Maven-blocking) local-project classpath has already been merged into + * the compiler config for the current connection. Distinct from "JDK path discovered": the + * best-effort {@link #prewarmClasspath} warms only the remote classpath + JDK detection and + * leaves this {@code false}, so the first real evaluation still completes the local merge. + * Reset to {@code false} on the reconnect edge (null JDK path) alongside the other caches. + * Guarded by the {@code synchronized} monitor of {@link #configureCompilerClasspath}. + */ + private boolean localClasspathMerged = false; + public JdiExpressionEvaluator( InMemoryJavaCompiler compiler, JDIConnectionService jdiConnectionService, @@ -1134,13 +1144,14 @@ private static ClassLoaderReference findClassLoader(StackFrame frame) throws Jdi } /** - * Configures the compiler with the target JVM's classpath. Skips if already configured for the - * current connection. Automatically reconfigures after a disconnect/reconnect cycle (detected - * via null JDK path): clears the compilation cache AND resets the compiler config because stale - * bytecode and a stale classpath may reference classes from a previous connection. Also resets - * the {@link LocalProjectClasspathProvider} cache on the same edge so a reconnect into a - * different working directory sees the new project layout. Must be called BEFORE any expression - * evaluation to avoid nested JDI calls. + * Configures the compiler with the target JVM's classpath, including the local-project classpath + * fallback. Skips if already fully configured for the current connection. Automatically + * reconfigures after a disconnect/reconnect cycle (detected via null JDK path): clears the + * compilation cache AND resets the compiler config because stale bytecode and a stale classpath + * may reference classes from a previous connection. Also resets the + * {@link LocalProjectClasspathProvider} cache on the same edge so a reconnect into a different + * working directory sees the new project layout. Must be called BEFORE any expression evaluation + * to avoid nested JDI calls. * * @param suspendedThread a thread already suspended at a breakpoint (REQUIRED) * @throws JdiEvaluationException if classpath/JDK discovery fails for the current connection; @@ -1150,8 +1161,33 @@ private static ClassLoaderReference findClassLoader(StackFrame frame) throws Jdi */ public synchronized void configureCompilerClasspath(ThreadReference suspendedThread) throws JdiEvaluationException { - // Self-healing: if JDK path is already set, classpath is already configured for this connection - if (jdiConnectionService.getDiscoveredJdkPath() != null) { + configureCompilerClasspath(suspendedThread, true); + } + + /** + * Worker for {@link #configureCompilerClasspath(ThreadReference)} and {@link #prewarmClasspath}. + * + *

{@code includeLocal} controls whether the local-project classpath fallback (env override + + * filesystem scan + Maven {@code dependency:build-classpath}) is merged in. The real evaluation + * paths pass {@code true}; the speculative {@link #prewarmClasspath} — which runs on the JDI + * event-listener thread the instant a breakpoint hits, before the user has asked for anything — + * passes {@code false}. The local fallback can shell out to Maven with a multi-minute timeout, so + * paying it on the event-listener thread would block event processing (and every other breakpoint) + * for minutes even if the user never evaluates an expression. Prewarm therefore warms only the + * remote classpath + JDK detection, and {@link #localClasspathMerged} stays {@code false} so the + * first actual evaluation/logpoint/condition completes the local merge. + * + * @param suspendedThread a thread already suspended at a breakpoint (REQUIRED) + * @param includeLocal whether to merge the (possibly Maven-blocking) local-project classpath + */ + private synchronized void configureCompilerClasspath(ThreadReference suspendedThread, boolean includeLocal) + throws JdiEvaluationException { + final boolean jdkReady = jdiConnectionService.getDiscoveredJdkPath() != null; + // Self-healing short-circuit. The remote classpath + JDK are warm once the JDK path is set, + // which is all a prewarm (includeLocal=false) cares about. A real evaluation additionally + // needs the local fallback merged — so it only short-circuits once localClasspathMerged is + // true, otherwise it falls through to complete the merge prewarm deliberately skipped. + if (jdkReady && (!includeLocal || localClasspathMerged)) { return; } @@ -1171,9 +1207,15 @@ public synchronized void configureCompilerClasspath(ThreadReference suspendedThr // The local-classpath provider lives in the same Spring singleton and may hold a // memoised view of a *previous* connection's project layout — reset it on the same // connection-lifecycle edge so the next discover() sees the current CWD/env/Maven view. - compilationCache.clear(); - compiler.reset(); - localClasspathProvider.reset(); + // Guard on !jdkReady: when we fall through here with the JDK already discovered (a real + // evaluation finishing the merge a prewarm deferred), the remote discovery is still valid + // and must NOT be wiped. + if (!jdkReady) { + compilationCache.clear(); + compiler.reset(); + localClasspathProvider.reset(); + localClasspathMerged = false; + } final long startTime = System.currentTimeMillis(); @@ -1201,7 +1243,11 @@ public synchronized void configureCompilerClasspath(ThreadReference suspendedThr // a stale local entry binds against the remote definition first — desired behaviour // for the source/binary-drift risk. final long mergeStart = System.currentTimeMillis(); - final Set localEntries = localClasspathProvider.discover(); + // includeLocal=false (prewarm) skips the local provider entirely — it can shell out to + // Maven with a multi-minute timeout, which must never run on the JDI event-listener + // thread. The first real evaluation re-enters with includeLocal=true and completes the + // merge below. + final Set localEntries = includeLocal ? localClasspathProvider.discover() : Set.of(); final String mergedClasspath = mergeClasspaths(remoteClasspath, localEntries); // Report three numbers so an operator can see overlap explicitly: // remote + local are the raw source counts; merged is the deduped union actually @@ -1209,7 +1255,8 @@ public synchronized void configureCompilerClasspath(ThreadReference suspendedThr // and local project share JARs) — the overlap shows up as "merged < remote + local". final int remoteCount = countEntries(remoteClasspath); final int mergedCount = countEntries(mergedClasspath); - log.info("[LocalClasspath] Merged classpath: {} remote + {} local entries → {} merged in {}ms", + log.info("[LocalClasspath] Merged classpath: {} remote + {} local entries → {} merged in {}ms" + + (includeLocal ? "" : " (prewarm — local fallback deferred to first evaluation)"), remoteCount, localEntries.size(), mergedCount, System.currentTimeMillis() - mergeStart); @@ -1228,6 +1275,11 @@ public synchronized void configureCompilerClasspath(ThreadReference suspendedThr final int version = jdiConnectionService.getTargetMajorVersion(); compiler.configure(jdkPath, mergedClasspath, version); + // Mark the local fallback merged only on the real-evaluation path. A prewarm leaves this + // false so the first actual evaluation falls through the short-circuit and merges it. + if (includeLocal) { + localClasspathMerged = true; + } log.info("[Evaluator] Compiler configured in {}ms", System.currentTimeMillis() - startTime); } finally { evaluationGuard.exit(guardedThreadId); @@ -1237,10 +1289,17 @@ public synchronized void configureCompilerClasspath(ThreadReference suspendedThr /** * Best-effort pre-warm of the compiler classpath at the first thread suspension, so the agent's * first {@code evaluate_expression} (or the first logpoint/condition hit) does not pay the - * one-time classpath discovery cost — which can take 1-3s while it walks the target VM's + * one-time remote classpath discovery cost — which can take 1-3s while it walks the target VM's * classloader hierarchy via {@code invokeMethod} — on the critical path. Discovery is paid once * per connection; subsequent calls short-circuit on the cached JDK path. *

+ * Warms ONLY the remote classpath + JDK detection (it passes {@code includeLocal=false}). The + * local-project fallback is deliberately NOT run here: it can shell out to Maven with a + * multi-minute timeout, and this method runs on the JDI event-listener thread the moment a + * breakpoint hits — blocking that thread on Maven would stall event processing (and every other + * breakpoint) for minutes even if the user never evaluates anything. The first real evaluation + * completes the local merge via {@link #configureCompilerClasspath(ThreadReference)}. + *

* Unlike {@link #configureCompilerClasspath}, this NEVER throws: it is invoked from the JDI * event listener while a thread is parked for inspection, and a warming failure must not disrupt * the breakpoint flow. Any failure is logged at debug and left for the real evaluation call to @@ -1253,7 +1312,7 @@ public void prewarmClasspath(ThreadReference suspendedThread) { return; } try { - configureCompilerClasspath(suspendedThread); + configureCompilerClasspath(suspendedThread, false); } catch (Exception e) { log.debug("[Evaluator] Classpath pre-warm failed (will retry on first evaluation): {}", e.getMessage()); diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java index 31525cb..c90b828 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java @@ -286,10 +286,12 @@ private void addMavenDependencies(Set entries) { if (added > 0) { log.info("[LocalClasspath] Maven contributed {} dependency jars in {}ms", added, elapsed); } else { - // "Reason-out on empty" — required by the logging policy. The runner already logs the - // shell-level failure; this line gives an operator the user-facing context. - log.info("[LocalClasspath] Maven contributed 0 jars in {}ms — check earlier WARN lines " - + "for the cause (timeout, non-zero exit, missing output files)", elapsed); + // "Reason-out on empty" — required by the logging policy. 0 jars is NOT necessarily a + // failure: a project with no runtime dependencies (or a filtered classpath) legitimately + // yields nothing. The runner already logs a WARN on a genuine shell-level failure, so + // point operators there for the failure case without implying one happened. + log.info("[LocalClasspath] Maven contributed 0 jars in {}ms (no runtime deps, or Maven " + + "failed — check earlier WARN lines for timeout / non-zero exit / missing output)", elapsed); } } catch (Exception e) { log.warn("[LocalClasspath] Maven dependency:build-classpath failed after {}ms: {}: {} (cwd={})", diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorConfigureClasspathTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorConfigureClasspathTest.java index 1ad8cd9..1e3b7d7 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorConfigureClasspathTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorConfigureClasspathTest.java @@ -73,10 +73,18 @@ void shouldResetCompilerOnDiscoveryFailure() { } @Test - @DisplayName("Already-configured connection short-circuits without touching the compiler") + @DisplayName("A completed full configure short-circuits a second call without re-discovering") void shouldShortCircuitWhenAlreadyConfigured() throws Exception { - // A non-null discovered JDK path means the connection is already configured. - when(jdiService.getDiscoveredJdkPath()).thenReturn("/opt/jdk"); + // First call: discovery succeeds and the local fallback is merged → the connection is fully + // configured. A non-null JDK path alone is NOT enough to short-circuit a real evaluation now + // (prewarm leaves it set with the local fallback still pending); only a completed full + // configure is. Drive one full configure, then assert the second call does nothing. + when(jdiService.getDiscoveredJdkPath()).thenReturn(null, "/opt/jdk"); + when(jdiService.discoverClasspath(thread)).thenReturn("/app/classes"); + when(jdiService.getTargetMajorVersion()).thenReturn(21); + + evaluator.configureCompilerClasspath(thread); + org.mockito.Mockito.clearInvocations(compiler, jdiService); evaluator.configureCompilerClasspath(thread); @@ -85,6 +93,35 @@ void shouldShortCircuitWhenAlreadyConfigured() throws Exception { verify(jdiService, never()).discoverClasspath(any()); } + @Test + @DisplayName("prewarm warms remote+JDK but defers local merge; first real eval completes it") + void prewarmDefersLocalMergeUntilFirstEvaluation() throws Exception { + // prewarm runs on the JDI event-listener thread and must never invoke the local provider + // (which can shell out to Maven for minutes). It warms only remote discovery + JDK detection; + // the first real configureCompilerClasspath then completes the local merge. + final LocalProjectClasspathProvider local = mock(LocalProjectClasspathProvider.class); + when(local.discover()).thenReturn(java.util.Set.of("/local/a")); + final JdiExpressionEvaluator deferEvaluator = new JdiExpressionEvaluator( + compiler, jdiService, new EvaluationGuard(), local); + + // prewarm reads the JDK path twice before discovery (outer guard + inner gate); both see + // null, then discovery sets it for every subsequent read. + when(jdiService.getDiscoveredJdkPath()).thenReturn(null, null, "/opt/jdk"); + when(jdiService.discoverClasspath(thread)).thenReturn("/app/classes"); + when(jdiService.getTargetMajorVersion()).thenReturn(21); + + deferEvaluator.prewarmClasspath(thread); + + // Remote discovery happened; the (potentially Maven-shelling) local discover() was NOT run + // during prewarm. The cheap cache reset() may still fire on the fresh-connection edge. + verify(jdiService).discoverClasspath(thread); + verify(local, never()).discover(); + + // First real evaluation falls through the JDK-path short-circuit and merges the local fallback. + deferEvaluator.configureCompilerClasspath(thread); + verify(local).discover(); + } + @Test @DisplayName("Successful discovery resets then configures the compiler with discovered values") void shouldConfigureOnSuccessfulDiscovery() throws Exception { diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderMavenTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderMavenTest.java index e7ed1fc..72233c5 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderMavenTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderMavenTest.java @@ -177,11 +177,6 @@ void shouldSkipMavenWhenNoPomXml(@TempDir Path tmp) { assertThat(invoked.get()).isFalse(); } - /** - * A {@code pom.xml} that exists but is structurally empty is a real-world case (e.g. partial - * sync, broken build) — the provider must still invoke Maven (it's the runner's job to deal - * with the failure) and tolerate an empty result list without throwing. - */ /** * Symlink-safety: a {@code pom.xml} that is itself a symbolic link must NOT trigger Maven * discovery. Maven would resolve the link and pull dependencies from the linked target, which @@ -220,6 +215,11 @@ void shouldSkipMavenWhenPomXmlIsSymlink(@TempDir Path tmp) throws Exception { .isFalse(); } + /** + * A {@code pom.xml} that exists but is structurally empty is a real-world case (e.g. partial + * sync, broken build) — the provider must still invoke Maven (it's the runner's job to deal + * with the failure) and tolerate an empty result list without throwing. + */ @Test @DisplayName("invokes Maven when pom.xml is present even if empty; tolerates empty result list") void shouldInvokeMavenWhenPomXmlIsEmpty(@TempDir Path tmp) throws Exception { From 5cf50080fecfdf5f12a5924122b3a92005e88051 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Thu, 28 May 2026 21:39:09 +0200 Subject: [PATCH 09/10] fix(evaluation): reject symlinked target/ in filesystem scan; review nits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent full-PR review (my own reviewer agents) found one real bug plus a few robustness/coverage gaps: - BUG: scanForClassDirs probed dir.resolve("target/classes") with NOFOLLOW_LINKS, but NOFOLLOW only guards the FINAL path component — the intermediate `target` segment was still resolved through symlinks. A symlinked target/ pointing outside the project tree would pull external classes onto the compile classpath, breaching the documented "symlinks are never followed" invariant. Now probe `target` itself with NOFOLLOW first and only resolve its leaves once confirmed a real dir. Regression test added (shouldNotFollowSymlinkedTargetDirectory). - Couple ProcessBuilderMavenRunner.HARVEST_MAX_DEPTH to LocalProjectClasspathProvider.MAX_SCAN_DEPTH at compile time (made the latter package-private) so a future scan-depth bump can't silently drop boundary modules — the prior "coupling" was comment-only on a literal 7. - Harden the stdout drainer loop: `!= -1` instead of `> 0` so a spurious zero-length read can't exit early and re-introduce a pipe-block hang. - Close a test gap: the cold-cache branch of renderEnvState (envOverrideHasEntries fallback) had no coverage. Added two cold-cache diagnose tests for (set) and (set, no entries). Full suite: 1070 passing (+3 new). --- .../LocalProjectClasspathProvider.java | 27 ++++++--- .../evaluation/ProcessBuilderMavenRunner.java | 16 +++-- .../edee/mcp/jdwp/JDWPToolsDiagnoseTest.java | 60 +++++++++++++++++++ ...ctClasspathProviderFilesystemScanTest.java | 52 ++++++++++++++++ 4 files changed, 141 insertions(+), 14 deletions(-) diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java index c90b828..fcd94cb 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java @@ -52,7 +52,9 @@ public class LocalProjectClasspathProvider { * Filesystem-scan depth cap. Five levels covers reactor / module / sub-module layouts without * walking into Node / Gradle / IDE caches that an unbounded walk would otherwise stat-flood. */ - private static final int MAX_SCAN_DEPTH = 5; + // Package-private (not private) so ProcessBuilderMavenRunner.HARVEST_MAX_DEPTH can derive from + // it at compile time, keeping the harvester's walk depth genuinely coupled to the scan depth. + static final int MAX_SCAN_DEPTH = 5; /** * Cold-cache {@code mvn} may download dependencies on first run, which routinely takes 1-3 * minutes on a fresh machine. 180s covers the common case; a real timeout still logs a clear @@ -403,13 +405,22 @@ private static void scanForClassDirs(Path dir, int depth, Set entries) { } // At each directory, probe for target/classes and target/test-classes BEFORE recursing — we // want a module's classes whether or not it has child modules. - final Path classes = dir.resolve("target/classes"); - if (Files.isDirectory(classes, LinkOption.NOFOLLOW_LINKS)) { - entries.add(classes.toString()); - } - final Path testClasses = dir.resolve("target/test-classes"); - if (Files.isDirectory(testClasses, LinkOption.NOFOLLOW_LINKS)) { - entries.add(testClasses.toString()); + // NOFOLLOW_LINKS only governs the FINAL path component, so a single two-segment resolve like + // dir.resolve("target/classes") would still follow a symlinked `target` — defeating the + // invariant. Probe `target` itself first: isDirectory(NOFOLLOW) is false for a symlink, so a + // linked `target` (pointing at a tree outside the project) is rejected here. Only once it is + // confirmed to be a real directory do we resolve its leaves, where NOFOLLOW then guards the + // last component (`classes` / `test-classes`). + final Path target = dir.resolve("target"); + if (Files.isDirectory(target, LinkOption.NOFOLLOW_LINKS)) { + final Path classes = target.resolve("classes"); + if (Files.isDirectory(classes, LinkOption.NOFOLLOW_LINKS)) { + entries.add(classes.toString()); + } + final Path testClasses = target.resolve("test-classes"); + if (Files.isDirectory(testClasses, LinkOption.NOFOLLOW_LINKS)) { + entries.add(testClasses.toString()); + } } if (depth == MAX_SCAN_DEPTH) { return; diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java index a9f60be..7379db5 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java @@ -47,12 +47,13 @@ public class ProcessBuilderMavenRunner implements LocalProjectClasspathProvider. /** Upper bound on captured child stdout/stderr included in the WARN log on failure. */ private static final int STDOUT_CAPTURE_BYTES = 64 * 1024; /** - * Walk cap for the classpath-file harvester: the provider's filesystem scan depth ({@code 5}) - * plus two — covering {@code /target/.jdwp-mcp-classpath} for modules sitting at the - * scan boundary. Keeping it relative to the scan depth (rather than a magic literal) keeps the - * two limits coupled: a future bump to one is mirrored in the other. + * Walk cap for the classpath-file harvester: the provider's filesystem scan depth plus two — + * covering {@code /target/.jdwp-mcp-classpath} for modules sitting at the scan boundary + * (the file is two segments — {@code target/.jdwp-mcp-classpath} — below the module dir). Derived + * from {@link LocalProjectClasspathProvider#MAX_SCAN_DEPTH} at compile time so a future bump to + * the scan depth is mirrored here automatically rather than silently dropping boundary modules. */ - private static final int HARVEST_MAX_DEPTH = 7; + private static final int HARVEST_MAX_DEPTH = LocalProjectClasspathProvider.MAX_SCAN_DEPTH + 2; private static final Logger log = LoggerFactory.getLogger(ProcessBuilderMavenRunner.class); /** Seam for the actual process execution; replaced in tests to drive timeout / failure paths. */ @@ -228,7 +229,10 @@ private static int executeRealCommand(CommandRequest req) throws IOException, In try (var in = process.getInputStream()) { final byte[] buf = new byte[4096]; int n; - while ((n = in.read(buf)) > 0) { + // `!= -1` (not `> 0`): EOF is the only loop-exit. A spurious zero-length read must + // keep draining, or the child could block on a full stdout pipe — the exact hang + // this drainer exists to prevent. + while ((n = in.read(buf)) != -1) { synchronized (capturedLock) { if (captured.size() < STDOUT_CAPTURE_BYTES) { captured.write(buf, 0, Math.min(n, STDOUT_CAPTURE_BYTES - captured.size())); diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsDiagnoseTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsDiagnoseTest.java index 199c869..f09a24c 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsDiagnoseTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsDiagnoseTest.java @@ -416,4 +416,64 @@ jdiService, new BreakpointTracker(), new WatcherManager(), assertThat(result).contains("Override env: JDWP_EXTRA_CLASSPATH (set, no entries)"); assertThat(result).doesNotContain("Override env: JDWP_EXTRA_CLASSPATH (unset)"); } + + /** + * Cold-cache three-way env state. The warm-cache tests above exercise {@code renderEnvState}'s + * {@code breakdown != null} branch (entry count from the breakdown). This pins the COLD branch — + * where the renderer must answer from {@link LocalProjectClasspathProvider#envOverrideHasEntries()} + * by parsing the env var directly, WITHOUT triggering discovery. The cache is deliberately left + * cold (no {@code discoverWithBreakdown()} call), so the "not yet computed" hint must appear + * alongside the correct marker. Without this, the entire reason {@code envOverrideHasEntries()} + * exists in the diagnose path would be untested. + */ + @Test + @DisplayName("cold cache: renders '(set)' from envOverrideHasEntries() without triggering discovery") + void shouldRenderSetOnColdCacheWhenEnvHasEntries() { + when(jdiService.getConnectionStatus()).thenReturn(new JDIConnectionService.ConnectionStatus( + false, null, 0, null, null + )); + final Path cwd = Path.of(System.getProperty("user.dir")); + final LocalProjectClasspathProvider stub = new LocalProjectClasspathProvider( + cwd, + name -> "JDWP_EXTRA_CLASSPATH".equals(name) + ? "/opt/extra1.jar" + java.io.File.pathSeparator + "/opt/extra2.jar" + : null, + (command, workingDirectory, timeoutSeconds) -> List.of() + ); + // Cache intentionally NOT warmed — peekCachedBreakdown() returns null in the renderer. + final JDWPTools toolsWithStub = JDWPToolsTestSupport.newTools( + jdiService, new BreakpointTracker(), new WatcherManager(), + mock(JdiExpressionEvaluator.class), new EventHistory(), new EvaluationGuard(), + discovery, stub); + + final String result = toolsWithStub.jdwp_diagnose(null); + + assertThat(result).contains("not yet computed"); + assertThat(result).contains("Override env: JDWP_EXTRA_CLASSPATH (set)"); + } + + @Test + @DisplayName("cold cache: renders '(set, no entries)' from envOverrideHasEntries() for a blank env") + void shouldRenderSetNoEntriesOnColdCacheWhenEnvIsBlank() { + when(jdiService.getConnectionStatus()).thenReturn(new JDIConnectionService.ConnectionStatus( + false, null, 0, null, null + )); + final Path cwd = Path.of(System.getProperty("user.dir")); + final LocalProjectClasspathProvider stub = new LocalProjectClasspathProvider( + cwd, + name -> "JDWP_EXTRA_CLASSPATH".equals(name) ? " " : null, + (command, workingDirectory, timeoutSeconds) -> List.of() + ); + // Cache intentionally NOT warmed — exercises the cold envOverrideHasEntries() fallback. + final JDWPTools toolsWithStub = JDWPToolsTestSupport.newTools( + jdiService, new BreakpointTracker(), new WatcherManager(), + mock(JdiExpressionEvaluator.class), new EventHistory(), new EvaluationGuard(), + discovery, stub); + + final String result = toolsWithStub.jdwp_diagnose(null); + + assertThat(result).contains("not yet computed"); + assertThat(result).contains("Override env: JDWP_EXTRA_CLASSPATH (set, no entries)"); + assertThat(result).doesNotContain("Override env: JDWP_EXTRA_CLASSPATH (unset)"); + } } diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderFilesystemScanTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderFilesystemScanTest.java index 82055d7..8f6a8a4 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderFilesystemScanTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderFilesystemScanTest.java @@ -168,6 +168,58 @@ void shouldNotFollowSymlinkedDirectoriesDuringScan(@TempDir Path tmp) throws Exc } } + /** + * Symlink-safety, deeper case: a symlinked {@code target/} directory must NOT be followed. This + * is distinct from {@link #shouldNotFollowSymlinkedDirectoriesDuringScan} (which symlinks a whole + * module): here the module dir is real but its {@code target} child is a symlink to a build tree + * outside the project. A naive {@code dir.resolve("target/classes")} probe with NOFOLLOW_LINKS + * only guards the final {@code classes} component and would still resolve through the linked + * {@code target}, pulling external classes onto the compile classpath. The scan must probe + * {@code target} itself with NOFOLLOW first and reject it when it is a link. + */ + @Test + @org.junit.jupiter.api.condition.EnabledOnOs({ + org.junit.jupiter.api.condition.OS.LINUX, + org.junit.jupiter.api.condition.OS.MAC + }) + @DisplayName("does not follow a symlinked target/ directory") + void shouldNotFollowSymlinkedTargetDirectory(@TempDir Path tmp) throws Exception { + // A build tree OUTSIDE the project root holding the classes we must NOT reach. + final Path outside = Files.createTempDirectory("jdwp-mcp-outside-target-"); + try { + Files.createDirectories(outside.resolve("classes")); + Files.createDirectories(outside.resolve("test-classes")); + + // A real module directory whose `target` is a SYMLINK to the outside build tree. + final Path module = tmp.resolve("linked-target-module"); + Files.createDirectories(module); + Files.createSymbolicLink(module.resolve("target"), outside); + + // A real (non-symlinked) module for sanity — proves the scan still works. + Files.createDirectories(tmp.resolve("real-module/target/classes")); + + final LocalProjectClasspathProvider provider = new LocalProjectClasspathProvider( + tmp, envName -> null, (cmd, cwd, t) -> List.of() + ); + + final Set entries = provider.discover(); + + assertThat(entries) + .contains(tmp.resolve("real-module/target/classes").toString()) + .as("a symlinked target/ must not be followed — symlink-not-followed invariant") + .doesNotContain( + module.resolve("target/classes").toString(), + module.resolve("target/test-classes").toString(), + outside.resolve("classes").toString(), + outside.resolve("test-classes").toString()); + } finally { + // Manual cleanup since the outside dir is not @TempDir-managed. + Files.deleteIfExists(outside.resolve("classes")); + Files.deleteIfExists(outside.resolve("test-classes")); + Files.deleteIfExists(outside); + } + } + /** * The scanner skips nested {@code target/} directories — once we enter a {@code target/} we * never recurse into a {@code target/foo/target/classes}. Anything nested inside the build From 33e539b80b56c7f8fc42195081d77238d87d74eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Thu, 28 May 2026 21:42:38 +0200 Subject: [PATCH 10/10] fix(evaluation): bound process-tree exit wait; accurate scan/log diagnostics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Knock down the three remaining minor review notes: - destroyProcessTree awaited each descendant's onExit().get(2s) serially → O(N)×2s on a wide Maven fork tree, which could overrun the caller's cleanup budget. Combine all exit futures (descendants + root) and wait once via CompletableFuture.allOf, so worst-case stays ~2s regardless of tree width. - Filesystem scan: Files.list() is lazy, so a permission failure during iteration surfaces as UncheckedIOException (wrapping AccessDeniedException) and missed the dedicated handler, logging the generic "could not list" wording. Unwrap it so a denied subtree gets the accurate "permission denied — skipping subtree" message. - countEntries javadoc claimed it stays "in lockstep" with mergeClasspaths; that only holds same-OS. Reword to state the host-separator vs content- heuristic divergence is an accepted cosmetic imprecision (log-only, never drives control flow), not a correctness issue. Full suite: 1070 passing. --- .../evaluation/JdiExpressionEvaluator.java | 12 +++++++++--- .../LocalProjectClasspathProvider.java | 12 ++++++++++++ .../evaluation/ProcessBuilderMavenRunner.java | 19 +++++++++++-------- 3 files changed, 32 insertions(+), 11 deletions(-) diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java index 1dfd141..e237b82 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java @@ -1350,9 +1350,15 @@ private static String mergeClasspaths(@Nullable String remote, Set local } /** - * Counts the non-blank entries in a remote-discovered classpath string. Used purely for the - * INFO log line that summarises the merge result. Honours the same single-entry Windows - * heuristic as {@link #mergeClasspaths} so the two stay in lockstep. + * Counts the non-blank entries in a classpath string, reusing {@link #splitRemoteClasspath}'s + * separator heuristic. Used purely for the INFO log line summarising the merge result — the + * value never drives control flow. + * + *

Caveat: {@code mergeClasspaths} always joins with the host {@link File#pathSeparator}, + * whereas this re-detects the separator from content. In the same-OS case (the only one that + * actually compiles — a Windows target's paths don't resolve on a Unix host and vice-versa) + * the two agree. In a hypothetical cross-OS mix the logged count can under-report; that is an + * accepted cosmetic imprecision, not a correctness issue. */ private static int countEntries(@Nullable String classpath) { if (classpath == null || classpath.isEmpty()) { diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java index fcd94cb..c75da92 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java @@ -7,6 +7,7 @@ import org.springframework.stereotype.Service; import java.io.File; +import java.io.UncheckedIOException; import java.nio.file.AccessDeniedException; import java.nio.file.Files; import java.nio.file.LinkOption; @@ -440,6 +441,17 @@ private static void scanForClassDirs(Path dir, int depth, Set entries) { // Permission denied on a single directory is expected (CI agents, restricted dev envs). // Log at WARN so an operator sees it once, but never abort the whole scan. log.warn("[LocalClasspath] Permission denied listing {} — skipping subtree", dir); + } catch (UncheckedIOException e) { + // Files.list() is lazy: a permission failure hit while ITERATING entries (inside the + // terminal forEach) surfaces as UncheckedIOException wrapping the real IOException, not + // as a bare AccessDeniedException. Unwrap so a denied subtree still gets the accurate + // "permission denied" message rather than the generic catch-all below. + if (e.getCause() instanceof AccessDeniedException) { + log.warn("[LocalClasspath] Permission denied listing {} — skipping subtree", dir); + } else { + log.warn("[LocalClasspath] Could not list {}: {}: {}", + dir, e.getClass().getSimpleName(), e.getMessage()); + } } catch (Exception e) { // Any other failure on a single directory is unexpected — log it visibly. We do NOT // throw: a partial classpath is better than no classpath, and the agent will see the diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java index 7379db5..e48e244 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java @@ -19,6 +19,7 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Set; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; import java.util.regex.Pattern; @@ -317,17 +318,19 @@ static void destroyProcessTree(Process process) { // (timeout or interrupt path) is trying to prevent. process.destroyForcibly(); } - for (ProcessHandle h : tree) { - try { - h.onExit().get(2, TimeUnit.SECONDS); - } catch (Exception ignored) { - // Best-effort — we already issued destroyForcibly; nothing more to do here. - } + // Confirm the whole tree actually exited, but bound the TOTAL wait — not 2s PER handle. + // A Maven reactor can fork several surefire/exec helpers; awaiting them serially would be + // O(N) × 2s and could blow past the caller's cleanup budget. Combine every exit future + // (descendants + root) and wait once, so worst-case stays ~2s regardless of tree width. + final CompletableFuture[] exits = new CompletableFuture[tree.size() + 1]; + for (int i = 0; i < tree.size(); i++) { + exits[i] = tree.get(i).onExit(); } + exits[tree.size()] = process.onExit(); try { - process.onExit().get(2, TimeUnit.SECONDS); + CompletableFuture.allOf(exits).get(2, TimeUnit.SECONDS); } catch (Exception ignored) { - // Same as above. + // Best-effort — we already issued destroyForcibly on every process; nothing more to do. } }