feat(evaluation): local-project classpath fallback for expression eval#33
Merged
Conversation
… eval 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
There was a problem hiding this comment.
Pull request overview
Adds a local-project classpath fallback to expression evaluation so ECJ/JDT compilation can resolve types even when the target VM’s classloader walk is sparse/opaque (e.g., Tomcat / Spring Boot dev-tools). This integrates a new provider into the evaluator’s classpath configuration and surfaces diagnostics via jdwp_diagnose, with extensive accompanying tests and documentation.
Changes:
- Introduces
LocalProjectClasspathProvider(env override + filesystem scan + Maven dependency classpath) and unions it with remote-discovered classpath inJdiExpressionEvaluator. - Adds
ProcessBuilderMavenRunnerto run Maven with timeout/stdout draining and harvesttarget/.jdwp-mcp-classpathoutputs. - Expands
jdwp_diagnoseoutput + docs and adds a broad test suite around the new behavior.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/java-debug/references/troubleshooting.md | Adds troubleshooting guidance for “X cannot be resolved” eval failures and operator steps. |
| README.md | Documents the new LocalProjectClasspathProvider in the architecture overview. |
| docs/expression-evaluation.md | Detailed design/behavior docs for the local-project classpath fallback. |
| jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java | Wires provider into tools and renders a local-classpath diagnose block. |
| jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java | Unions remote + local classpath for ECJ configuration; resets local cache on reconnect. |
| jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProvider.java | Implements the local-project classpath discovery (env/filesystem/Maven) with caching/breakdown. |
| jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunner.java | Implements Maven shell-out + harvesting of output files under target/. |
| jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsTestSupport.java | Updates test scaffolding for new JDWPTools constructor/provider dependency. |
| jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsDiagnoseTest.java | Adds assertions for the new diagnose “Local project classpath” section. |
| jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/ProcessBuilderMavenRunnerTest.java | Tests harvesting rules, no-delete policy, interrupt behavior, and Windows wrapper selection. |
| jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderTestSupport.java | Adds helper for constructing provider instances in tests. |
| jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderMavenTest.java | Tests Maven command shape, wrapper preference, timeout, and pom presence handling. |
| jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderFilesystemScanTest.java | Tests scan depth boundary and skip directory behavior for target/classes discovery. |
| jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderEnvOverrideTest.java | Tests env parsing (separator, trimming, blanks) and breakdown semantics. |
| jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/LocalProjectClasspathProviderCacheTest.java | Tests memoization/reset, concurrency behavior, and breakdown immutability/peek semantics. |
| jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorLocalClasspathUnionTest.java | Tests remote-first union ordering, dedupe, empty-remote behavior, and Windows-path heuristic. |
| jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorConfigureClasspathTest.java | Updates evaluator tests to inject a provider seam. |
| jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorGetDeclaredTypeTest.java | Updates evaluator tests to inject a provider seam. |
| jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorAbsentLocalsTest.java | Updates evaluator tests to inject a provider seam. |
| jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorPackageFallbackTest.java | Updates evaluator tests to inject a provider seam. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Contributor
Author
|
Addressed all 10 review findings in 90edc54. Mapping:
Additional tests added:
Removed the old Module sweep: 1062 tests, 0 failures, 3 skipped (2 pre-existing TOCTOU + 1 Windows-only conditional). Compile clean; no new NullAway warnings. |
- 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.
Contributor
Author
|
Addressed all 3 new portability findings in the latest commit:
Module sweep: 1062 tests, 0 failures, 3 skipped. |
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.
Contributor
Author
Cross-OS portability auditReviewed every production and test file touched in this PR for Linux / macOS / Windows correctness. Two real issues found and fixed; the rest is already portable. Fixed in latest commit
Audited and already portable
Documented tradeoffs (not fixed — accepted)
Module sweep: 1062 tests, 0 failures, 3 skipped. |
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).
Contributor
Author
|
Addressed both new findings.
1062 tests / 0 failures / 3 skipped. |
- 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 <root>/a/b/c/d/e/target (depth 7). - Symlinked pom.xml at the working directory must not trigger Maven.
Contributor
Author
|
Addressed all three findings from the latest Copilot review in commit
Build: 1064 tests, 0 failures, 3 skipped on Linux JDK 17. Resolved all three review threads. |
- 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.
Contributor
Author
|
Addressed both findings from the latest Copilot review:
Build: 1066 tests, 0 failures, 3 skipped on Linux JDK 17. Resolved both review threads. |
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.
…nits
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).
…nostics 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
LocalProjectClasspathProvideras an additive fallback to fill gaps the remoteClasspathDiscovererwalk cannot see (Tomcat / Spring Boot dev-tools / customURLClassLoaders that hide JARs fromgetURLs()).JDWP_EXTRA_CLASSPATHenv var, depth-5 filesystem scan fortarget/classes+target/test-classes, Mavendependency:build-classpath. Synchronized + memoized — Maven runs at most once per JDI connection;reset()ties cache lifecycle to the connection edge.ProcessBuilderMavenRunnershells out with 180s timeout, 64KB stdout capture for diagnosable WARN logs, daemon drainer so the child never blocks on pipe buffer.InterruptedExceptiondestroys the child and restores the interrupt flag.jdwp_diagnoseshows a per-source breakdown so operators can inspect what was scanned. Cold-cache diagnose uses a non-blocking peek so it never triggers the 180s Maven call.Closes #32.
File-safety invariant
Files.delete*/Files.move*in production code (grep-verified).target/.jdwp-mcp-classpath(Maven-owned, gitignored, wiped bymvn clean).target/. A.jdwp-mcp-classpathanywhere else is ignored.Resolution semantics
Remote-first union ordering means live target VM types win JDT resolution. Local entries only contribute classes the remote walk could not see, mitigating source/binary drift between the developer's checkout and the deployed target.
Tests
+23 new tests across 7 files; 4 evaluator-test
setUpblocks refactored onto a sharedLocalProjectClasspathProviderTestSupporthelper.Module sweep: 1059 tests, 0 failures, 3 skipped (2 pre-existing TOCTOU race tests + 1
@EnabledOnOs(WINDOWS)Maven-wrapper test).Test plan
jdwp_diagnosebefore any expression evaluation → confirm "Local project classpath" section renders with "(not yet computed)" fast-path; diagnose returns immediately.jdwp_evaluate_expression "1+1"→ confirm it succeeds even on a target VM where remote discovery is sparse.jdwp_diagnoseagain → confirm full breakdown now appears with non-zerofilesystem=andmaven=counts.JDWP_EXTRA_CLASSPATH=/some/jar:/another/path, reconnect, repeat → confirm env-override appears in breakdown and "Override env: JDWP_EXTRA_CLASSPATH (set)"./tmp), reconnect → confirm local breakdown shows zeros; remote-only discovery still works for projects where the target VM's classloader walk is healthy.~/.m2to verify Maven timeout and WARN-log behavior on slow first run.