From 7a0fec0d59a0204cda02f93f43db71d1d0b06db3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Sat, 23 May 2026 14:22:09 +0200 Subject: [PATCH 1/9] feat: brace-mode multi-statement support for expression evaluator Closes #8 User input wrapped in `{ ... }` is now spliced into the wrapper's method body verbatim instead of being treated as a single expression. The mode is detected by a tokenizer-aware brace-match that ignores braces inside string/char/text-block literals, so `"{x}".length()` still routes through expression mode while `{ try { return foo(); } catch (...) }` routes through block mode. In block mode the user is responsible for `return X;` statements; a trailing `return null;` fallthrough guard keeps the wrapper type-correct if the user block doesn't end with a return. This automatically extends to every eval call site routing through `evaluate()` -- jdwp_evaluate_expression, jdwp_assert_expression, breakpoint conditions, logpoint expressions and conditions, exception logpoints, field watchpoint conditions/expressions, and watchers. Tool descriptions on all 8 eval-bearing tool methods/params updated with a `(supports `{ ...; return X; }` block syntax)` suffix so the feature is discoverable from any eval-shaped param in isolation. Tests added: 9 in JdiExpressionEvaluatorRewriteTest covering block-mode detection, nested braces, brace-inside-string-literal escapes, and expression-mode fallback for `{x}.foo()`-style inputs. --- .../java/one/edee/mcp/jdwp/JDWPTools.java | 45 ++++----- .../evaluation/JdiExpressionEvaluator.java | 93 +++++++++++++++++-- .../JdiExpressionEvaluatorRewriteTest.java | 63 +++++++++++++ 3 files changed, 174 insertions(+), 27 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 ccabf09..2e97649 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 @@ -806,10 +806,10 @@ private static boolean isDefaultObjectToString(String value) { } - @McpTool(description = "Evaluate a Java expression in the context of a suspended thread's stack frame. Returns '[VM_DEATH] ...' if the target VM disconnects mid-call.") + @McpTool(description = "Evaluate a Java expression in the context of a suspended thread's stack frame. Two input modes: (1) single expression — `order.getTotal()`, `x + y`. (2) block mode — wrap the input in `{ ... }` to send a statement body with try/catch, intermediate locals, or early `return`; the user writes explicit `return X;` statements to yield a value. Returns '[VM_DEATH] ...' if the target VM disconnects mid-call.") public String jdwp_evaluate_expression( @McpToolParam(description = "Thread unique ID") long threadId, - @McpToolParam(description = "Java expression to evaluate (e.g., 'order.getTotal()', 'x + y', 'name.length()')") String expression, + @McpToolParam(description = "Java expression OR `{ ... }` block to evaluate. Examples: `order.getTotal()`, `x + y`, `{ try { return risky(); } catch (Exception e) { return e.getMessage(); } }`") String expression, @McpToolParam(required = false, description = "Frame index (0 = current frame, default: 0)") @Nullable Integer frameIndex) { try { final VirtualMachine vm = jdiService.getVM(); @@ -839,9 +839,9 @@ public String jdwp_evaluate_expression( } } - @McpTool(description = "Evaluate a Java expression and compare its result against an expected value. Returns 'OK' on match, 'MISMATCH' with actual vs expected on failure, or '[VM_DEATH] ...' if the target VM disconnects mid-call. Comparison is string-based against the same formatting jdwp_evaluate_expression uses (so primitives auto-unbox, strings strip surrounding quotes).") + @McpTool(description = "Evaluate a Java expression and compare its result against an expected value. Returns 'OK' on match, 'MISMATCH' with actual vs expected on failure, or '[VM_DEATH] ...' if the target VM disconnects mid-call. Comparison is string-based against the same formatting jdwp_evaluate_expression uses (so primitives auto-unbox, strings strip surrounding quotes). Supports `{ block }` syntax for multi-statement.") public String jdwp_assert_expression( - @McpToolParam(description = "Java expression (e.g., 'order.getTotal()', 'session.getRole()', 'list.size() == 5')") String expression, + @McpToolParam(description = "Java expression (e.g., 'order.getTotal()', 'session.getRole()', 'list.size() == 5') — supports `{ ...; return X; }` block syntax") String expression, @McpToolParam(description = "Expected value (string-compared against the formatted expression result)") String expected, @McpToolParam(required = false, description = "Thread ID — defaults to the last breakpoint thread") @Nullable Long threadId, @McpToolParam(required = false, description = "Frame index (default: 0)") @Nullable Integer frameIndex) { @@ -1956,12 +1956,12 @@ private String doStep(@Nullable Long threadId, int stepDepth, String label) { } } - @McpTool(description = "Set a breakpoint at a specific line in a class. Supports conditional breakpoints. By default the breakpoint is registered passively: if the class is not yet loaded, the BP is deferred via a ClassPrepareRequest and activates when the JVM loads the class on its own — same behaviour as IntelliJ / Eclipse / jdb. Pass forceLoad=true only when you need the breakpoint to bind immediately and accept that loading the class will run its `` (early static init, eager dependency loads, masked lazy-load diagnostics).") + @McpTool(description = "Set a breakpoint at a specific line in a class. Supports conditional breakpoints — conditions support `{ block }` syntax for multi-statement. By default the breakpoint is registered passively: if the class is not yet loaded, the BP is deferred via a ClassPrepareRequest and activates when the JVM loads the class on its own — same behaviour as IntelliJ / Eclipse / jdb. Pass forceLoad=true only when you need the breakpoint to bind immediately and accept that loading the class will run its `` (early static init, eager dependency loads, masked lazy-load diagnostics).") public String jdwp_set_breakpoint( @McpToolParam(description = "Fully qualified class name (e.g. 'com.example.MyClass')") String className, @McpToolParam(description = "Line number") int lineNumber, @McpToolParam(required = false, description = "Suspend policy: 'all' (default), 'thread', 'none'") @Nullable String suspendPolicy, - @McpToolParam(required = false, description = "Optional condition — only suspend when this evaluates to true (e.g., 'i > 100')") @Nullable String condition, + @McpToolParam(required = false, description = "Optional condition — only suspend when this evaluates to true (e.g., 'i > 100') — supports `{ ...; return X; }` block syntax") @Nullable String condition, @McpToolParam(required = false, description = "Optional ID of a trigger breakpoint — this BP stays disarmed until the trigger fires. Sticky by default: once armed it stays so unless re-disarmed via jdwp_disarm_until_trigger or oneShot=true.") @Nullable Integer triggerBreakpointId, @McpToolParam(required = false, description = "If true, re-disarm this BP after each hit so the next trigger fire re-arms it (IntelliJ-style). Default: false (sticky).") @Nullable Boolean oneShot, @McpToolParam(required = false, description = "If true, force-load the class via Class.forName in the target VM when it isn't already loaded. Default: false (passive — defer via ClassPrepareRequest). Force-load triggers ``, cascades dependent loads, and can mask lazy-init / classloader-leak diagnostics — use sparingly.") @Nullable Boolean forceLoad) { @@ -2097,12 +2097,12 @@ public String jdwp_set_breakpoint( } } - @McpTool(description = "Set a logpoint (non-stopping breakpoint) that evaluates an expression and logs the result without pausing execution. Supports an optional condition — the expression is only logged when the condition evaluates to true. By default the logpoint is registered passively: if the class is not yet loaded, it is deferred via a ClassPrepareRequest and activates on natural load. Pass forceLoad=true to force the class load (runs ``, may mask lazy-init diagnostics).") + @McpTool(description = "Set a logpoint (non-stopping breakpoint) that evaluates an expression and logs the result without pausing execution. Supports an optional condition — the expression is only logged when the condition evaluates to true. Both expression and condition support `{ block }` syntax for multi-statement. By default the logpoint is registered passively: if the class is not yet loaded, it is deferred via a ClassPrepareRequest and activates on natural load. Pass forceLoad=true to force the class load (runs ``, may mask lazy-init diagnostics).") public String jdwp_set_logpoint( @McpToolParam(description = "Fully qualified class name") String className, @McpToolParam(description = "Line number") int lineNumber, - @McpToolParam(description = "Java expression to evaluate and log (e.g., '\"x=\" + x', 'order.getTotal()')") String expression, - @McpToolParam(required = false, description = "Optional condition — only log when this evaluates to true (e.g., 'i > 100')") @Nullable String condition, + @McpToolParam(description = "Java expression to evaluate and log (e.g., '\"x=\" + x', 'order.getTotal()') — supports `{ ...; return X; }` block syntax") String expression, + @McpToolParam(required = false, description = "Optional condition — only log when this evaluates to true (e.g., 'i > 100') — supports `{ ...; return X; }` block syntax") @Nullable String condition, @McpToolParam(required = false, description = "If true, force-load the class via Class.forName in the target VM when it isn't already loaded. Default: false (passive — defer via ClassPrepareRequest). Force-load triggers `` and can mask lazy-init / classloader-leak diagnostics — use sparingly.") @Nullable Boolean forceLoad) { // Track the pending ID outside the try so the catch can clean it up if locationsOfLine // (or any later step) throws after the pending entry has already been registered. @@ -2464,7 +2464,8 @@ public String jdwp_set_exception_breakpoint( "throw of the given exception type. The expression is evaluated against the throwing frame with " + "$exception bound to the thrown object (e.g., \"$exception.getMessage()\"); its result is attached " + "to the event. The optional condition is evaluated with the same $exception binding — the log is " + - "recorded only when the condition is true. Use this for tracing exception flows in long-running " + + "recorded only when the condition is true. Both expression and condition support `{ block }` syntax " + + "for multi-statement. Use this for tracing exception flows in long-running " + "services without halting traffic. By default the logpoint is registered passively: if the exception " + "class is not yet loaded the logpoint is deferred via a ClassPrepareRequest and activates on natural " + "load. Pass forceLoad=true when targeting an exception class the application hasn't referenced yet " + @@ -2472,8 +2473,8 @@ public String jdwp_set_exception_breakpoint( "running `` in the target VM.") public String jdwp_set_exception_logpoint( @McpToolParam(description = "Exception class name (e.g., 'java.sql.SQLException')") String exceptionClass, - @McpToolParam(description = "Java expression evaluated on each hit; $exception is bound to the thrown object") String expression, - @McpToolParam(required = false, description = "Optional condition with $exception bound — only log when this evaluates to true") @Nullable String condition, + @McpToolParam(description = "Java expression evaluated on each hit; $exception is bound to the thrown object — supports `{ ...; return X; }` block syntax") String expression, + @McpToolParam(required = false, description = "Optional condition with $exception bound — only log when this evaluates to true — supports `{ ...; return X; }` block syntax") @Nullable String condition, @McpToolParam(required = false, description = "Log caught exceptions (default: true)") @Nullable Boolean caught, @McpToolParam(required = false, description = "Log uncaught exceptions (default: true)") @Nullable Boolean uncaught, @McpToolParam(required = false, description = "Optional ID of a trigger breakpoint — this logpoint stays disarmed until the trigger fires. Sticky by default.") @Nullable Integer triggerBreakpointId, @@ -2600,15 +2601,16 @@ Exception breakpoint deferred (ID: %d) @McpTool(description = "Set a field watchpoint that suspends the firing thread when the field is read " + "(mode='access'), written (mode='modification'), or both. Conditions are evaluated against the firing " + "frame with $oldValue, $newValue (modification only), $object (null for static), $fieldName, and $mode " + - "bound. By default the BP is registered passively: if the class is not yet loaded it is deferred via " + - "a ClassPrepareRequest and activates on natural load. Pass forceLoad=true to bind immediately and " + - "accept the side effects of running `` in the target VM. ERROR if the field is ambiguous on the " + - "class, missing, or static when objectFilterId is supplied.") + "bound; conditions support `{ block }` syntax for multi-statement. By default the BP is registered " + + "passively: if the class is not yet loaded it is deferred via a ClassPrepareRequest and activates on " + + "natural load. Pass forceLoad=true to bind immediately and accept the side effects of running " + + "`` in the target VM. ERROR if the field is ambiguous on the class, missing, or static when " + + "objectFilterId is supplied.") public String jdwp_set_field_breakpoint( @McpToolParam(description = "Fully-qualified class declaring the field (e.g., 'com.example.Order')") String className, @McpToolParam(description = "Field name (must be unambiguous on the class)") String fieldName, @McpToolParam(description = "Watch mode: 'access', 'modification', or 'both' (case-insensitive)") String mode, - @McpToolParam(required = false, description = "Optional condition with $oldValue, $newValue, $object, $fieldName, $mode bound — fire only when this evaluates to true") @Nullable String condition, + @McpToolParam(required = false, description = "Optional condition with $oldValue, $newValue, $object, $fieldName, $mode bound — fire only when this evaluates to true — supports `{ ...; return X; }` block syntax") @Nullable String condition, @McpToolParam(required = false, description = "Optional thread filter — only fire when this thread (uniqueID) hits the field") @Nullable Long threadFilterId, @McpToolParam(required = false, description = "Optional object filter — only fire on the given instance (object ID from jdwp_get_locals/jdwp_get_fields). Must be omitted for static fields.") @Nullable Long objectFilterId, @McpToolParam(required = false, description = "Optional ID of a trigger breakpoint — this field BP stays disarmed until the trigger fires. Sticky by default.") @Nullable Integer triggerBreakpointId, @@ -2625,6 +2627,7 @@ public String jdwp_set_field_breakpoint( "access or modification of the field. The expression is evaluated against the firing frame with " + "$oldValue, $newValue (modification only), $object (null for static), $fieldName, and $mode bound; " + "its result is attached to the event. Optional condition gates whether the log is recorded. " + + "Both expression and condition support `{ block }` syntax for multi-statement. " + "Use this to trace state transitions in long-running services without halting traffic. By default the " + "logpoint is registered passively: if the class is not yet loaded it is deferred via a " + "ClassPrepareRequest and activates on natural load. Pass forceLoad=true to bind immediately and " + @@ -2633,8 +2636,8 @@ public String jdwp_set_field_logpoint( @McpToolParam(description = "Fully-qualified class declaring the field") String className, @McpToolParam(description = "Field name (must be unambiguous on the class)") String fieldName, @McpToolParam(description = "Watch mode: 'access', 'modification', or 'both' (case-insensitive)") String mode, - @McpToolParam(description = "Java expression evaluated on each hit (e.g., '$oldValue + \" -> \" + $newValue')") String expression, - @McpToolParam(required = false, description = "Optional condition with the same synthetic bindings — log only when true") @Nullable String condition, + @McpToolParam(description = "Java expression evaluated on each hit (e.g., '$oldValue + \" -> \" + $newValue') — supports `{ ...; return X; }` block syntax") String expression, + @McpToolParam(required = false, description = "Optional condition with the same synthetic bindings — log only when true — supports `{ ...; return X; }` block syntax") @Nullable String condition, @McpToolParam(required = false, description = "Optional thread filter — only fire when this thread (uniqueID) hits the field") @Nullable Long threadFilterId, @McpToolParam(required = false, description = "Optional object filter — only fire on the given instance. Must be omitted for static fields.") @Nullable Long objectFilterId, @McpToolParam(required = false, description = "Optional ID of a trigger breakpoint — this logpoint stays disarmed until the trigger fires. Sticky by default.") @Nullable Integer triggerBreakpointId, @@ -3682,11 +3685,11 @@ public String jdwp_get_current_thread() { } } - @McpTool(description = "Attach a watcher to a breakpoint to evaluate a Java expression when hit. Returns the watcher ID.") + @McpTool(description = "Attach a watcher to a breakpoint to evaluate a Java expression when hit. Returns the watcher ID. Supports `{ block }` syntax for multi-statement.") public String jdwp_attach_watcher( @McpToolParam(description = "Breakpoint request ID (from jdwp_overview)") int breakpointId, @McpToolParam(description = "Descriptive label for this watcher (e.g., 'Trace entity ID', 'Check user name')") String label, - @McpToolParam(description = "Java expression to evaluate (e.g., 'entity.id', 'user.name', 'items.size()')") String expression) { + @McpToolParam(description = "Java expression to evaluate (e.g., 'entity.id', 'user.name', 'items.size()') — supports `{ ...; return X; }` block syntax") String expression) { try { if (expression.trim().isEmpty()) { return "Error: No expression provided"; 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 e6617ac..e0ed308 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 @@ -371,10 +371,24 @@ private static String resolveLocalVarType(LocalVariable var, String wrapperPacka /** * Generates a wrapper class with a static `evaluate()` method that accepts the context - * variables as parameters and returns the result of the user expression. The user expression - * is wrapped in `(Object)(...)` so any value type — including primitives via autoboxing — - * can be returned. The bare `this` keyword in the expression is tokenizer-rewritten to `_this` - * because the wrapper class doesn't have a `this` reference (it's a static method). + * variables as parameters and returns the result of the user input. + * + *

Two input modes, picked by {@link #isBlockMode}: + *

    + *
  • Expression mode (default). The input is treated as a single Java expression and + * wrapped as {@code return (Object) ();} so any value type — including primitives + * via autoboxing — flows back as the method's return value.
    • + *
  • Block mode. The trimmed input starts with {@code {} and ends with {@code }}; + * the inside is spliced into the method body verbatim, with a trailing + * {@code return null;} appended so the method is reachable-fall-through-safe even if + * the user forgot a {@code return}. The user is responsible for writing {@code return X;} + * statements to yield a value — block mode supports {@code try/catch}, intermediate + * locals, early {@code return}, and other statement-level constructs that the single- + * expression mode could not express.
    • + *
+ * + *

In both modes, the bare {@code this} keyword is tokenizer-rewritten to {@code _this} + * because the wrapper is a static method. * *

The package portion of {@code className} drives the wrapper's {@code package} declaration. * When the caller routes a non-public {@code this} type into its own package via @@ -395,17 +409,84 @@ private static String generateSourceCode(String className, EvaluationContext con // literals are NOT rewritten — see {@link #rewriteThisKeyword(String)}. final String safeExpression = rewriteThisKeyword(expression); + final String methodBody; + if (isBlockMode(safeExpression)) { + // Extract the body inside the outermost braces — already balanced because + // isBlockMode verified start with `{` and end with `}` and the tokenizer skips + // string/char/text-block content. The trailing `return null;` keeps the method + // type-correct when the user's block falls through without a return; an explicit + // `return X;` inside the user block wins, and `unreachable code` does not occur + // because the unconditional return is a separate statement, not part of the user + // body. + final String trimmed = safeExpression.trim(); + final String userBody = trimmed.substring(1, trimmed.length() - 1); + methodBody = + " // User block:\n" + + userBody + '\n' + + " // Fallthrough guard — block reached end without a `return`.\n" + + " return null;\n"; + } else { + methodBody = + " // User expression:\n" + + " return (Object) (" + safeExpression + ");\n"; + } + final String packageDecl = packageName.isEmpty() ? "" : "package " + packageName + ";\n\n"; return packageDecl + "// Automatically generated class for JDI expression evaluation\n" + "public class " + simpleClassName + " {\n" + " public static Object " + EVALUATION_METHOD_NAME + '(' + methodParameters + ") {\n" + - " // User expression:\n" + - " return (Object) (" + safeExpression + ");\n" + + methodBody + " }\n" + "}\n"; } + /** + * Returns {@code true} when the user input is in block mode — i.e. its trimmed form starts + * with {@code {} and ends with the matching {@code }}. Tokenizer-aware: it makes sure the + * trailing {@code }} is at the top level and not buried inside a string / char / text-block + * literal, so an input like {@code "{x}".length()} (a method call on a string literal that + * happens to start with {@code {}) stays in expression mode. + * + *

Static + package-private so it can be unit-tested without a real {@link StackFrame}. + */ + static boolean isBlockMode(String expression) { + final String trimmed = expression.trim(); + if (trimmed.length() < 2 || trimmed.charAt(0) != '{' || trimmed.charAt(trimmed.length() - 1) != '}') { + return false; + } + // Walk the inside and confirm brace balance returns to zero at exactly the closing `}`. + // Tokenizer-aware so braces inside string / char / text-block literals are ignored. + final int n = trimmed.length() - 1; + int depth = 1; + int i = 1; + while (i < n) { + final char c = trimmed.charAt(i); + if (c == '"') { + i = skipStringLiteral(trimmed, i); + continue; + } + if (c == '\'') { + i = skipCharLiteral(trimmed, i); + continue; + } + if (c == '{') { + depth++; + } else if (c == '}') { + depth--; + if (depth == 0) { + // Hit a top-level `}` before the trailing one — input is something like + // `{stmt;} + foo` so the trailing `}` of the trimmed form is a different + // closer and block-mode does not apply. + return false; + } + } + i++; + } + // Trailing `}` at index n closes the outer brace. + return depth == 1; + } + /** * Get a name suitable to use in the generated wrapper class for the given runtime type. * Handles three cases the wrapper compiler cannot otherwise express: diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java index 7dad664..7ad15cb 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java @@ -383,4 +383,67 @@ void shouldTolerateUnterminatedStringInThisKeyword() { // The bare `this` before the literal should still be rewritten assertThat(result).isEqualTo("_this + \"unterminated"); } + + // ── isBlockMode: detect `{ ... }` block input vs expression input ──────────────────── + + @Test + @DisplayName("isBlockMode: plain expression is NOT a block") + void shouldNotDetectBlockForPlainExpression() { + assertThat(JdiExpressionEvaluator.isBlockMode("foo.bar() + baz")).isFalse(); + } + + @Test + @DisplayName("isBlockMode: `{ stmt; }` IS a block") + void shouldDetectSingleStatementBlock() { + assertThat(JdiExpressionEvaluator.isBlockMode("{ return x; }")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: leading/trailing whitespace is allowed") + void shouldDetectBlockWithSurroundingWhitespace() { + assertThat(JdiExpressionEvaluator.isBlockMode(" \n { return x; }\n ")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: nested braces are matched correctly") + void shouldDetectBlockWithNestedBraces() { + assertThat(JdiExpressionEvaluator.isBlockMode( + "{ if (x > 0) { return x; } else { return 0; } }")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: braces inside a string literal do NOT terminate the outer block") + void shouldNotConfuseBracesInsideStringLiteral() { + // The `}` inside the string would mislead a naive matcher; the tokenizer must skip it. + assertThat(JdiExpressionEvaluator.isBlockMode("{ return \"}\"; }")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: `{x}.foo()` is NOT a block (trailing `}` is not the outer closer)") + void shouldNotDetectBlockWhenClosingBraceIsMidExpression() { + // `{x}` opens-and-closes before `.foo()`, so the trailing closer of the trimmed input + // is `)` — block mode should not apply. + assertThat(JdiExpressionEvaluator.isBlockMode("{x}.foo()")).isFalse(); + } + + @Test + @DisplayName("isBlockMode: brace-balanced internals followed by expression text are NOT a block") + void shouldNotDetectBlockWhenInternalBraceClosesEarly() { + // `{a;}+b;` — the first `}` closes the outer brace at i=4, depth goes to 0 before the + // trailing `}` at i=7. Block detection must reject this. + assertThat(JdiExpressionEvaluator.isBlockMode("{a;}+b;")).isFalse(); + } + + @Test + @DisplayName("isBlockMode: an empty input is NOT a block") + void shouldNotDetectBlockForEmptyInput() { + assertThat(JdiExpressionEvaluator.isBlockMode("")).isFalse(); + assertThat(JdiExpressionEvaluator.isBlockMode(" ")).isFalse(); + } + + @Test + @DisplayName("isBlockMode: a lone `{}` is a (trivial, empty) block") + void shouldDetectEmptyBlock() { + assertThat(JdiExpressionEvaluator.isBlockMode("{}")).isTrue(); + } } From 534f2a35eb40d0a6076d9f5048f139f271ab6645 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Sat, 23 May 2026 14:24:53 +0200 Subject: [PATCH 2/9] =?UTF-8?q?feat:=20diagnostics=20for=20deferred=20BP/L?= =?UTF-8?q?P=20install=20=E2=80=94=20surface=20CLASS=5FPREPARE,=20warn=20o?= =?UTF-8?q?n=20multi-Location=20lines?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #9 Two cheap diagnostics that don't change behaviour but make install-time mysteries easier to debug: 1) CLASS_PREPARE event recorded in EventHistory. Without this, jdwp_get_events shows only [VM_START, VM_DEATH] for sessions that sit on a deferred BP — agents can't tell "class never loaded" from "class loaded but BP did not fire". 2) describeLocation(method+bci) logged at every BP install. When locationsOfLine(line) returns more than one Location (typical for lambdas: one in the enclosing method, one inside the synthetic `lambda$...$N`), we now emit a BP_MULTI_LOCATION event + WARN log + WARNING suffix in the tool response so the "BP installed but silently misses one of the code paths" case is impossible to miss. No behavioural change — the BP-bind logic still uses .get(0). A proper multi-location bind is tracked as a separate concern; this issue is diagnostics-only. Touched: - JdiEventListener: describeLocation helper, CLASS_PREPARE event in EventHistory, BP_MULTI_LOCATION event + WARN log on multi-location lines. - JDWPTools.jdwp_set_breakpoint / jdwp_set_logpoint: WARNING suffix in the synchronous tool response when locations.size() > 1. --- .../java/one/edee/mcp/jdwp/JDWPTools.java | 26 +++++++--- .../one/edee/mcp/jdwp/JdiEventListener.java | 52 ++++++++++++++++++- 2 files changed, 71 insertions(+), 7 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 ccabf09..8f47626 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 @@ -2071,7 +2071,8 @@ public String jdwp_set_breakpoint( // BP opens a window where the BP can fire once before its trigger ever does. JDI // request creation already returns a disabled request; postpone the enable to the very // last step (and only when no chain edge applies). - final BreakpointRequest bpRequest = erm.createBreakpointRequest(locations.get(0)); + final Location boundLocation = locations.get(0); + final BreakpointRequest bpRequest = erm.createBreakpointRequest(boundLocation); bpRequest.setSuspendPolicy(jdiPolicy); final int breakpointId = breakpointTracker.registerBreakpoint(bpRequest); @@ -2086,8 +2087,15 @@ public String jdwp_set_breakpoint( bpRequest.setEnabled(true); } - return String.format("Breakpoint set at %s:%d (ID: %d, suspend: %s%s%s)", - className, lineNumber, breakpointId, policyLabel, conditionInfo, chainInfo); + // Diagnostic — see JdiEventListener.handleClassPrepareEvent for the rationale. A + // line with multiple Locations (typical for lambdas) only gets a BP at the first + // one; the other code paths through the same line silently miss. + final String multiLocSuffix = locations.size() > 1 + ? String.format(", WARNING: line has %d Locations; bound only to %s — other paths will MISS this BP", + locations.size(), JdiEventListener.describeLocation(boundLocation)) + : ""; + return String.format("Breakpoint set at %s:%d (ID: %d, suspend: %s%s%s%s)", + className, lineNumber, breakpointId, policyLabel, conditionInfo, chainInfo, multiLocSuffix); } catch (AbsentInformationException e) { cleanupOrphanPendingBreakpoint(pendingIdForCleanup); return "Error: No line number information available for this class. Compile with debug info (-g)."; @@ -2172,7 +2180,8 @@ public String jdwp_set_logpoint( return String.format("Error: No executable code at line %d in class %s", lineNumber, className); } - final BreakpointRequest bpRequest = erm.createBreakpointRequest(locations.get(0)); + final Location boundLocation = locations.get(0); + final BreakpointRequest bpRequest = erm.createBreakpointRequest(boundLocation); bpRequest.setSuspendPolicy(jdiPolicy); bpRequest.enable(); @@ -2182,8 +2191,13 @@ public String jdwp_set_logpoint( breakpointTracker.setCondition(breakpointId, condition); } - return String.format("Logpoint set at %s:%d (ID: %d, expression: %s%s)", - className, lineNumber, breakpointId, expression, conditionInfo); + // Diagnostic — see jdwp_set_breakpoint for the rationale. + final String multiLocSuffix = locations.size() > 1 + ? String.format(", WARNING: line has %d Locations; bound only to %s — other paths will MISS this LP", + locations.size(), JdiEventListener.describeLocation(boundLocation)) + : ""; + return String.format("Logpoint set at %s:%d (ID: %d, expression: %s%s%s)", + className, lineNumber, breakpointId, expression, conditionInfo, multiLocSuffix); } catch (AbsentInformationException e) { cleanupOrphanPendingBreakpoint(pendingIdForCleanup); return "Error: No line number information available. Compile with debug info (-g)."; diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JdiEventListener.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JdiEventListener.java index f1dbba5..00a57ad 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JdiEventListener.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JdiEventListener.java @@ -168,6 +168,24 @@ private void applyChainEffectsAfterHit(int firingBpId, @Nullable EventRequest fi } } + /** + * Formats a JDI {@link Location} as a diagnostic string of the form + * {@code @bci=}. Used by BP-install paths to log exactly which bytecode + * Location we bound a breakpoint to — important for lambda-bearing lines where the same + * source line maps to multiple Locations (one in the enclosing method, one inside the + * synthetic {@code lambda$...$N} method) and {@code locationsOfLine(line).get(0)} only + * binds the first. + */ + static String describeLocation(Location location) { + try { + return location.method().name() + "@bci=" + location.codeIndex(); + } catch (Exception e) { + // Defensive — method() / codeIndex() can throw on certain pathological synthetic + // methods; fall back to the toString() form so the diagnostic still produces output. + return location.toString(); + } + } + /** * Formats a JDI value for the human-readable logpoint output. Mirrors the format used by * {@link JDIConnectionService#formatFieldValue} but without its side effects (no object cache @@ -1024,6 +1042,18 @@ private void handleClassPrepareEvent(ClassPrepareEvent event) { log.info("[JDI] ClassPrepareEvent for '{}', activating {} deferred breakpoint(s), {} deferred exception breakpoint(s), {} deferred field BP(s)", className, pendingList.size(), pendingExList.size(), pendingFieldList.size()); + // Surface the CPE to agents via jdwp_get_events. Without this, an agent that sets a + // deferred BP and waits on jdwp_resume_until_event sees only [VM_START, VM_DEATH] and + // cannot tell whether the class ever loaded — making it impossible to distinguish + // "class never loaded" from "class loaded but BP did not fire". + eventHistory.record(new EventHistory.DebugEvent("CLASS_PREPARE", + String.format("ClassPrepareEvent for %s, activating %d line BP(s), %d exception BP(s), %d field BP(s)", + className, pendingList.size(), pendingExList.size(), pendingFieldList.size()), + Map.of("class", className, + "lineBpCount", String.valueOf(pendingList.size()), + "exceptionBpCount", String.valueOf(pendingExList.size()), + "fieldBpCount", String.valueOf(pendingFieldList.size())))); + final VirtualMachine vm = event.virtualMachine(); final EventRequestManager erm = vm.eventRequestManager(); @@ -1060,7 +1090,27 @@ private void handleClassPrepareEvent(ClassPrepareEvent event) { log.debug("[JDI] Deferred breakpoint {} promotion lost the race to the opportunistic path; orphan request deleted", id); continue; } - log.info("[JDI] Deferred breakpoint {} activated at {}:{}", id, className, pending.getLineNumber()); + // Diagnostic — log how many Locations were available at this line and which one + // we bound to. A line with multiple Locations (typical for lambdas: one in the + // enclosing method, one in the synthetic `lambda$...$N`) means `.get(0)` only + // covers ONE of the code paths; if the user's code runs through the OTHER + // location, the BP silently misses. The bound-Location detail also helps + // confirm we bound where the user expected vs. an off-by-one source-map slip. + if (locations.size() > 1) { + log.warn("[JDI] Deferred breakpoint {} activated at {}:{} — line has {} Locations; bound only to {} (other paths will MISS this BP)", + id, className, pending.getLineNumber(), locations.size(), describeLocation(location)); + eventHistory.record(new EventHistory.DebugEvent("BP_MULTI_LOCATION", + String.format("BP #%d at %s:%d — line has %d bytecode locations; bound only to %s (other paths will MISS)", + id, className, pending.getLineNumber(), locations.size(), describeLocation(location)), + Map.of("breakpointId", String.valueOf(id), + "class", className, + "line", String.valueOf(pending.getLineNumber()), + "locationCount", String.valueOf(locations.size()), + "boundLocation", describeLocation(location)))); + } else { + log.info("[JDI] Deferred breakpoint {} activated at {}:{} ({})", + id, className, pending.getLineNumber(), describeLocation(location)); + } } catch (AbsentInformationException e) { recordPromotionFailure(id, className, pending.getLineNumber(), From 3fa17d89f3357249f60f532e580da5762122b27d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Sat, 23 May 2026 14:46:22 +0200 Subject: [PATCH 3/9] review: address PR #11 feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Javadoc on generateSourceCode and isBlockMode: replace malformed `{@code {}` and `{@code }}` inline tags (the `}` closes the inline tag prematurely) with plain '{' and '}' references in prose. - isBlockMode now skips Java line (`//…`) and block (`/*…*/`) comments alongside string/char/text-block literals. Without this, valid blocks like `{ // }` + body or `{ /* } */ … }` would be misclassified as expression-mode and fail compilation. - Unterminated comment that swallows the trailing `}` is detected (skip past `n`) and reported as not-block-mode rather than left silently miscounted. - generateSourceCode block-mode body now wraps the user body in `if (__mcpFallthroughGuard) { ... } return null;` where the guard is a non-final local set to true. JLS §15.29 says only final variables initialised with constant expressions are constant expressions, so the compiler cannot prove the post-if `return null;` unreachable. Fixes the "unreachable code" compile error when the user's block ends with an explicit `return X;` (the previous trailing `return null;` was statically unreachable in that case). - evaluate() now SKIPS the bare-field-reference rewrite when the input is in block mode. The rewriter is identifier-level and cannot tell a field reference from a local-variable declaration; in block mode `int count = 1; ...` would otherwise become `int _this.count = 1; ...`. Block-mode users are expected to use explicit `this.field` / `_this.field` references; the keyword rewrite in generateSourceCode still handles `this.field` for them. Tests added (4 in JdiExpressionEvaluatorRewriteTest): - isBlockMode skips `}` inside a `/* ... */` block comment - isBlockMode skips `}` inside a `//` line comment - isBlockMode handles mixed nested braces + comment-buried braces - isBlockMode tolerates unterminated block comment (returns false because the comment swallows the trailing `}`) --- .../evaluation/JdiExpressionEvaluator.java | 110 ++++++++++++++---- .../JdiExpressionEvaluatorRewriteTest.java | 30 +++++ 2 files changed, 120 insertions(+), 20 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 e0ed308..9b580da 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 @@ -378,13 +378,13 @@ private static String resolveLocalVarType(LocalVariable var, String wrapperPacka *

  • Expression mode (default). The input is treated as a single Java expression and * wrapped as {@code return (Object) ();} so any value type — including primitives * via autoboxing — flows back as the method's return value.
    • - *
  • Block mode. The trimmed input starts with {@code {} and ends with {@code }}; - * the inside is spliced into the method body verbatim, with a trailing - * {@code return null;} appended so the method is reachable-fall-through-safe even if - * the user forgot a {@code return}. The user is responsible for writing {@code return X;} - * statements to yield a value — block mode supports {@code try/catch}, intermediate - * locals, early {@code return}, and other statement-level constructs that the single- - * expression mode could not express.
    • + *
  • Block mode. The trimmed input starts with '{' and ends with the matching '}'; + * the inside is spliced into the method body verbatim, guarded by a runtime-only branch + * so that the trailing fallthrough {@code return null;} stays reachable even when the + * user body ends with an explicit {@code return X;}. The user is responsible for writing + * {@code return X;} statements to yield a value — block mode supports {@code try/catch}, + * intermediate locals, early {@code return}, and other statement-level constructs that + * the single-expression mode could not express.
    • * * *

    In both modes, the bare {@code this} keyword is tokenizer-rewritten to {@code _this} @@ -412,18 +412,25 @@ private static String generateSourceCode(String className, EvaluationContext con final String methodBody; if (isBlockMode(safeExpression)) { // Extract the body inside the outermost braces — already balanced because - // isBlockMode verified start with `{` and end with `}` and the tokenizer skips - // string/char/text-block content. The trailing `return null;` keeps the method - // type-correct when the user's block falls through without a return; an explicit - // `return X;` inside the user block wins, and `unreachable code` does not occur - // because the unconditional return is a separate statement, not part of the user - // body. + // isBlockMode verified the structure (start with `{`, balanced end with `}`, + // string/char/text-block/comment regions skipped). + // + // The user body is wrapped in `if (__mcpFallthroughGuard) { ... }` where the guard + // is a non-final local set to true. The compiler can't treat it as a compile-time + // constant (JLS §15.29 — only `final` variables initialised with constant + // expressions qualify), so it does NOT prove the post-if `return null;` unreachable. + // That keeps the wrapper compiling whether or not the user's body ends with an + // explicit `return X;`. At runtime the guard is always true → user body always runs; + // `return null;` is the safety net for blocks that fall through without returning. final String trimmed = safeExpression.trim(); final String userBody = trimmed.substring(1, trimmed.length() - 1); methodBody = " // User block:\n" + + " boolean __mcpFallthroughGuard = true;\n" + + " if (__mcpFallthroughGuard) {\n" + userBody + '\n' + - " // Fallthrough guard — block reached end without a `return`.\n" + + " }\n" + + " // Fallthrough guard — only reached when the user's block doesn't return.\n" + " return null;\n"; } else { methodBody = @@ -443,10 +450,11 @@ private static String generateSourceCode(String className, EvaluationContext con /** * Returns {@code true} when the user input is in block mode — i.e. its trimmed form starts - * with {@code {} and ends with the matching {@code }}. Tokenizer-aware: it makes sure the - * trailing {@code }} is at the top level and not buried inside a string / char / text-block - * literal, so an input like {@code "{x}".length()} (a method call on a string literal that - * happens to start with {@code {}) stays in expression mode. + * with '{' and ends with the matching '}'. Tokenizer-aware: braces inside string / char / + * text-block literals and inside line ({@code //…}) or block ({@code /*…*}{@code /}) comments + * are ignored, so inputs like {@code "{x}".length()} (a method call on a string literal that + * happens to start with '{') stay in expression mode, and a comment containing '}' inside an + * otherwise-block input does not prematurely close the outer block. * *

    Static + package-private so it can be unit-tested without a real {@link StackFrame}. */ @@ -456,7 +464,8 @@ static boolean isBlockMode(String expression) { return false; } // Walk the inside and confirm brace balance returns to zero at exactly the closing `}`. - // Tokenizer-aware so braces inside string / char / text-block literals are ignored. + // Tokenizer-aware so braces inside string / char / text-block literals and Java comments + // are ignored. final int n = trimmed.length() - 1; int depth = 1; int i = 1; @@ -470,6 +479,27 @@ static boolean isBlockMode(String expression) { i = skipCharLiteral(trimmed, i); continue; } + if (c == '/' && i + 1 < n) { + final char next = trimmed.charAt(i + 1); + if (next == '/') { + i = skipLineComment(trimmed, i); + // A line comment that runs to end-of-string would have swallowed the + // trailing `}` (no newline before it) — there is no real closer, so the + // input is not block-mode. + if (i > n) { + return false; + } + continue; + } + if (next == '*') { + i = skipBlockComment(trimmed, i); + // Unterminated block comment swallows the trailing `}` too. Same rationale. + if (i > n) { + return false; + } + continue; + } + } if (c == '{') { depth++; } else if (c == '}') { @@ -487,6 +517,39 @@ static boolean isBlockMode(String expression) { return depth == 1; } + /** + * Returns the index just past the end of a {@code //…} line comment that starts at + * {@code start}. Terminator is the next newline; if no newline is found, returns the end of + * the string (best-effort tolerance — never throws). + */ + private static int skipLineComment(String s, int start) { + final int n = s.length(); + int i = start + 2; + while (i < n && s.charAt(i) != '\n') { + i++; + } + // Caller resumes scanning AT the newline (or end). The newline itself is not part of the + // comment, so we don't advance past it here. + return i; + } + + /** + * Returns the index just past the end of a {@code /*…*}{@code /} block comment that starts at + * {@code start}. If the closing {@code *}{@code /} is missing, returns the end of the string + * (best-effort tolerance — never throws). + */ + private static int skipBlockComment(String s, int start) { + final int n = s.length(); + int i = start + 2; + while (i + 1 < n) { + if (s.charAt(i) == '*' && s.charAt(i + 1) == '/') { + return i + 2; + } + i++; + } + return n; + } + /** * Get a name suitable to use in the generated wrapper class for the given runtime type. * Handles three cases the wrapper compiler cannot otherwise express: @@ -756,13 +819,20 @@ private static ClassLoaderReference findClassLoader(StackFrame frame) throws Jdi // Auto-rewrite bare references to fields of `this` so users can write // `sessions.containsKey(session)` instead of `_this.sessions.containsKey(session)`. + // SKIPPED in block mode — the rewriter is identifier-level and cannot distinguish + // a field reference from a local-variable declaration. Rewriting a statement-body + // input like `int count = 1; ...` would corrupt the declaration into + // `int _this.count = 1; ...`. Block-mode users are expected to use explicit + // `this.field` / `_this.field` references (the keyword rewrite in `generateSourceCode` + // still handles `this.field` → `_this.field`). // The set of rewritable fields depends on what the wrapper can actually see: // - When the wrapper lives in the default isolated package, only public fields on a // public declaring type are reachable. // - When the wrapper lives in `this`'s own package, public/protected/package-private // fields are all reachable. Private fields still aren't, so they stay un-rewritten // and produce a compile error the user can fix (or work around via reflection). - if (thisObject != null && thisObject.referenceType() instanceof ClassType thisClass) { + if (!isBlockMode(expression) + && thisObject != null && thisObject.referenceType() instanceof ClassType thisClass) { final boolean sharingPackage = !wrapperPackage.equals(DEFAULT_EVALUATION_PACKAGE) && wrapperPackage.equals(packageOf(thisClass.name())); if (thisClass.isPublic() || sharingPackage) { diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java index 7ad15cb..175f5ed 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java @@ -446,4 +446,34 @@ void shouldNotDetectBlockForEmptyInput() { void shouldDetectEmptyBlock() { assertThat(JdiExpressionEvaluator.isBlockMode("{}")).isTrue(); } + + @Test + @DisplayName("isBlockMode: `}` inside a `/* ... */` block comment does NOT close the outer block") + void shouldNotConfuseBracesInsideBlockComment() { + // Without comment-skip, the `}` inside the comment would drop depth to 0 and the + // outer block would be rejected as non-block input. + assertThat(JdiExpressionEvaluator.isBlockMode("{ /* close: } */ return 1; }")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: `}` inside a `//` line comment does NOT close the outer block") + void shouldNotConfuseBracesInsideLineComment() { + assertThat(JdiExpressionEvaluator.isBlockMode("{ // }\n return 1; }")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: `/* nested } */` followed by real `}` still balances") + void shouldHandleBlockCommentWithBraceAtVariousDepths() { + // Nested braces in code, plus a `}` hidden inside a comment, plus the real closer. + assertThat(JdiExpressionEvaluator.isBlockMode( + "{ if (x) { /* not a closer: } */ return 1; } return 0; }")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: unterminated block comment is tolerated, treats remainder as comment") + void shouldTolerateUnterminatedBlockComment() { + // `/* …` without a closing `*/` swallows the rest of the input including the trailing `}`. + // The outer brace then never closes → not block-mode. Must not throw. + assertThat(JdiExpressionEvaluator.isBlockMode("{ /* unterminated }")).isFalse(); + } } From b5aa27b1f7bb3834ee3958ed049aef0b1a7c729b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Sat, 23 May 2026 14:51:43 +0200 Subject: [PATCH 4/9] review: address PR #12 feedback - Race-guard install path in jdwp_set_breakpoint AND jdwp_set_logpoint now emits the multi-Location WARNING suffix too, not just the eager path. A lambda-bearing line that loads BETWEEN the initial classesByName check and the ClassPrepareRequest registration goes through the race-guard branch; previously it bound .get(0) and returned without warning, so the lambda silently missed depending on timing. Factored the formatter into a shared `multiLocationSuffix` helper so both paths in both tools use one definition. - describeLocation is now no-throw end-to-end. Even the toString() fallback can raise JDI runtime exceptions (e.g. VMDisconnectedException during VM tear-down); wrapping it ensures a single diagnostic call cannot abort the surrounding BP install path. Final fallback returns the literal "" so the diagnostic still emits. - ClassPrepareEvent diagnostic now distinguishes line breakpoints from logpoints. Logpoints share the pending-line path, so the previous always-"BP" labels would mislead an agent grepping for "logpoint". Uses breakpointTracker.isLogpoint(id) to pick between "BP"/"LP" in the log message and EventHistory entry, and adds a `kind` key to the event metadata. Tests added (4 in JdiEventListenerClassPrepareTest): - CLASS_PREPARE event recorded when a CPE activates pending items - CLASS_PREPARE NOT recorded when no pending items reference the class (short-circuit path) - BP_MULTI_LOCATION recorded for a lambda-bearing line (multiple Locations) with the new "BP " label prefix - BP_MULTI_LOCATION uses "LP " label when the pending entry is a logpoint (has a logpoint expression attached) --- .../java/one/edee/mcp/jdwp/JDWPTools.java | 53 +++++--- .../one/edee/mcp/jdwp/JdiEventListener.java | 35 ++++-- .../JdiEventListenerClassPrepareTest.java | 113 ++++++++++++++++++ 3 files changed, 176 insertions(+), 25 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 8f47626..3d48c39 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 @@ -2037,7 +2037,8 @@ public String jdwp_set_breakpoint( // request while it is still being wired up. JDI delivers events the instant // a request is enabled, so a chained BP enabled before its chain edge is // registered could fire once unchained on a hot code path. - final BreakpointRequest bpRequest = erm.createBreakpointRequest(locations.get(0)); + final Location boundLocation = locations.get(0); + final BreakpointRequest bpRequest = erm.createBreakpointRequest(boundLocation); bpRequest.setSuspendPolicy(jdiPolicy); if (!breakpointTracker.promotePendingToActive(pendingId, bpRequest)) { // Another path (listener or safety-net) won the promotion race; tear @@ -2050,8 +2051,12 @@ public String jdwp_set_breakpoint( } else if (triggerBreakpointId == null) { bpRequest.setEnabled(true); } - return String.format("Breakpoint set at %s:%d (ID: %d, suspend: %s%s%s)", - className, lineNumber, pendingId, policyLabel, conditionInfo, chainInfo); + // Multi-location diagnostic — must apply to the race-guard path too, not + // just the eager path, otherwise lambda-bearing lines silently miss + // depending on whether the class loaded before or after BP registration. + return String.format("Breakpoint set at %s:%d (ID: %d, suspend: %s%s%s%s)", + className, lineNumber, pendingId, policyLabel, conditionInfo, chainInfo, + multiLocationSuffix(locations, boundLocation, "BP")); } } @@ -2090,12 +2095,9 @@ public String jdwp_set_breakpoint( // Diagnostic — see JdiEventListener.handleClassPrepareEvent for the rationale. A // line with multiple Locations (typical for lambdas) only gets a BP at the first // one; the other code paths through the same line silently miss. - final String multiLocSuffix = locations.size() > 1 - ? String.format(", WARNING: line has %d Locations; bound only to %s — other paths will MISS this BP", - locations.size(), JdiEventListener.describeLocation(boundLocation)) - : ""; return String.format("Breakpoint set at %s:%d (ID: %d, suspend: %s%s%s%s)", - className, lineNumber, breakpointId, policyLabel, conditionInfo, chainInfo, multiLocSuffix); + className, lineNumber, breakpointId, policyLabel, conditionInfo, chainInfo, + multiLocationSuffix(locations, boundLocation, "BP")); } catch (AbsentInformationException e) { cleanupOrphanPendingBreakpoint(pendingIdForCleanup); return "Error: No line number information available for this class. Compile with debug info (-g)."; @@ -2152,7 +2154,8 @@ public String jdwp_set_logpoint( final ReferenceType refType = recheck.get(0); final List locations = refType.locationsOfLine(lineNumber); if (!locations.isEmpty()) { - final BreakpointRequest bpRequest = erm.createBreakpointRequest(locations.get(0)); + final Location boundLocation = locations.get(0); + final BreakpointRequest bpRequest = erm.createBreakpointRequest(boundLocation); bpRequest.setSuspendPolicy(jdiPolicy); bpRequest.enable(); if (!breakpointTracker.promotePendingToActive(pendingId, bpRequest)) { @@ -2164,8 +2167,10 @@ public String jdwp_set_logpoint( // VM may already be disconnected — best-effort cleanup. } } - return String.format("Logpoint set at %s:%d (ID: %d, expression: %s%s)", - className, lineNumber, pendingId, expression, conditionInfo); + // Multi-location diagnostic — same rationale as jdwp_set_breakpoint. + return String.format("Logpoint set at %s:%d (ID: %d, expression: %s%s%s)", + className, lineNumber, pendingId, expression, conditionInfo, + multiLocationSuffix(locations, boundLocation, "LP")); } } @@ -2192,12 +2197,9 @@ public String jdwp_set_logpoint( } // Diagnostic — see jdwp_set_breakpoint for the rationale. - final String multiLocSuffix = locations.size() > 1 - ? String.format(", WARNING: line has %d Locations; bound only to %s — other paths will MISS this LP", - locations.size(), JdiEventListener.describeLocation(boundLocation)) - : ""; return String.format("Logpoint set at %s:%d (ID: %d, expression: %s%s%s)", - className, lineNumber, breakpointId, expression, conditionInfo, multiLocSuffix); + className, lineNumber, breakpointId, expression, conditionInfo, + multiLocationSuffix(locations, boundLocation, "LP")); } catch (AbsentInformationException e) { cleanupOrphanPendingBreakpoint(pendingIdForCleanup); return "Error: No line number information available. Compile with debug info (-g)."; @@ -2219,6 +2221,25 @@ private void cleanupOrphanPendingBreakpoint(@Nullable Integer pendingId) { } } + /** + * Formats the multi-location WARNING suffix appended to BP / LP install responses when a + * source line maps to more than one bytecode {@link Location}. Returns an empty string when + * there is only one Location, so the caller can splat the value into a format string + * unconditionally. {@code kind} is the short label ("BP" / "LP") used in the warning text. + *

    + * Shared by both {@code jdwp_set_breakpoint} / {@code jdwp_set_logpoint} AND their race-guard + * paths (where the class loaded between the initial {@code classesByName} check and the + * ClassPrepareRequest registration) so the diagnostic surfaces uniformly regardless of which + * activation path the install took. + */ + private static String multiLocationSuffix(List locations, Location boundLocation, String kind) { + if (locations.size() <= 1) { + return ""; + } + return String.format(", WARNING: line has %d Locations; bound only to %s — other paths will MISS this %s", + locations.size(), JdiEventListener.describeLocation(boundLocation), kind); + } + @McpTool(description = "Clear a breakpoint by its synthetic ID (from jdwp_overview). Routes by kind: line, exception, and field breakpoints share one ID space.") public String jdwp_clear_breakpoint(@McpToolParam(description = "Breakpoint ID to clear") int breakpointId) { try { diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JdiEventListener.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JdiEventListener.java index 00a57ad..1386033 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JdiEventListener.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JdiEventListener.java @@ -175,14 +175,25 @@ private void applyChainEffectsAfterHit(int firingBpId, @Nullable EventRequest fi * source line maps to multiple Locations (one in the enclosing method, one inside the * synthetic {@code lambda$...$N} method) and {@code locationsOfLine(line).get(0)} only * binds the first. + * + *

    No-throw end-to-end. Both the structured form and the {@code toString()} fallback can + * raise JDI runtime exceptions (e.g. {@code VMDisconnectedException}, {@code ClassNotPreparedException}) + * during VM-tear-down races; on any failure we return a fixed placeholder so a single + * diagnostic call cannot abort the surrounding BP install path or tool response. */ static String describeLocation(Location location) { try { return location.method().name() + "@bci=" + location.codeIndex(); } catch (Exception e) { - // Defensive — method() / codeIndex() can throw on certain pathological synthetic - // methods; fall back to the toString() form so the diagnostic still produces output. - return location.toString(); + // method() / codeIndex() failed (e.g. pathological synthetic, VM mid-disconnect). + // Try the JDI toString() form as a softer fallback. + try { + return location.toString(); + } catch (Exception ignored) { + // Even toString() can throw on a half-torn-down Location. Return a fixed marker + // so the diagnostic emits SOMETHING and the caller keeps running. + return ""; + } } } @@ -1096,20 +1107,26 @@ private void handleClassPrepareEvent(ClassPrepareEvent event) { // covers ONE of the code paths; if the user's code runs through the OTHER // location, the BP silently misses. The bound-Location detail also helps // confirm we bound where the user expected vs. an off-by-one source-map slip. + // Logpoints share this pending-line path, so distinguish the label in the log + // and event so an agent grepping for "logpoint" sees its hit. + final boolean isLogpoint = breakpointTracker.isLogpoint(id); + final String kindUpper = isLogpoint ? "LP" : "BP"; + final String kindLower = isLogpoint ? "logpoint" : "breakpoint"; if (locations.size() > 1) { - log.warn("[JDI] Deferred breakpoint {} activated at {}:{} — line has {} Locations; bound only to {} (other paths will MISS this BP)", - id, className, pending.getLineNumber(), locations.size(), describeLocation(location)); + log.warn("[JDI] Deferred {} {} activated at {}:{} — line has {} Locations; bound only to {} (other paths will MISS this {})", + kindLower, id, className, pending.getLineNumber(), locations.size(), describeLocation(location), kindUpper); eventHistory.record(new EventHistory.DebugEvent("BP_MULTI_LOCATION", - String.format("BP #%d at %s:%d — line has %d bytecode locations; bound only to %s (other paths will MISS)", - id, className, pending.getLineNumber(), locations.size(), describeLocation(location)), + String.format("%s #%d at %s:%d — line has %d bytecode locations; bound only to %s (other paths will MISS)", + kindUpper, id, className, pending.getLineNumber(), locations.size(), describeLocation(location)), Map.of("breakpointId", String.valueOf(id), + "kind", kindLower, "class", className, "line", String.valueOf(pending.getLineNumber()), "locationCount", String.valueOf(locations.size()), "boundLocation", describeLocation(location)))); } else { - log.info("[JDI] Deferred breakpoint {} activated at {}:{} ({})", - id, className, pending.getLineNumber(), describeLocation(location)); + log.info("[JDI] Deferred {} {} activated at {}:{} ({})", + kindLower, id, className, pending.getLineNumber(), describeLocation(location)); } } catch (AbsentInformationException e) { diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JdiEventListenerClassPrepareTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JdiEventListenerClassPrepareTest.java index d37e105..a142186 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JdiEventListenerClassPrepareTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JdiEventListenerClassPrepareTest.java @@ -323,6 +323,119 @@ void shouldRecordFailureWhenCreateExceptionRequestThrowsGenericException() throw assertThat(pending.getFailureReason()).contains("ERM rejected the exception request"); } + // ── Diagnostic event emissions (CLASS_PREPARE, BP_MULTI_LOCATION) ──────── + + @Test + @DisplayName("CLASS_PREPARE event recorded in EventHistory when a CPE activates pending items") + void shouldRecordClassPrepareEventInHistory() throws Exception { + ReferenceType refType = mock(ReferenceType.class); + Location loc = mock(Location.class); + when(refType.name()).thenReturn("com.example.Foo"); + when(refType.locationsOfLine(42)).thenReturn(List.of(loc)); + + BreakpointRequest createdBp = mock(BreakpointRequest.class); + EventRequestManager erm = mock(EventRequestManager.class); + when(erm.createBreakpointRequest(loc)).thenReturn(createdBp); + + tracker.registerPendingBreakpoint("com.example.Foo", 42, 2, "ALL"); + + ClassPrepareEvent event = mockClassPrepareEvent(refType, erm); + runListenerWith(listener, mockEventSet(event)); + + assertThat(eventHistory.getRecent(10)) + .anySatisfy(e -> { + assertThat(e.type()).isEqualTo("CLASS_PREPARE"); + assertThat(e.summary()).contains("com.example.Foo"); + assertThat(e.summary()).contains("1 line BP"); + }); + } + + @Test + @DisplayName("CLASS_PREPARE NOT recorded when the CPE has no pending items for the class") + void shouldNotRecordClassPrepareWhenNothingPending() throws Exception { + ReferenceType refType = mock(ReferenceType.class); + when(refType.name()).thenReturn("com.example.Unrelated"); + EventRequestManager erm = mock(EventRequestManager.class); + + // No pending entries for "com.example.Unrelated". + ClassPrepareEvent event = mockClassPrepareEvent(refType, erm); + runListenerWith(listener, mockEventSet(event)); + + // The handler short-circuits at the "all pending lists empty" check before recording. + assertThat(eventHistory.getRecent(10)) + .noneSatisfy(e -> assertThat(e.type()).isEqualTo("CLASS_PREPARE")); + } + + @Test + @DisplayName("BP_MULTI_LOCATION recorded when locationsOfLine returns more than one Location (lambda case)") + void shouldRecordMultiLocationEventForLambdaLine() throws Exception { + ReferenceType refType = mock(ReferenceType.class); + Location enclosingLoc = mock(Location.class); + Location lambdaLoc = mock(Location.class); + // Mock describeLocation's path so the summary is readable. The exact contents aren't + // what the test asserts on, but a sane shape helps diagnosis if the assertion fails. + com.sun.jdi.Method enclosingMethod = mock(com.sun.jdi.Method.class); + when(enclosingMethod.name()).thenReturn("doWork"); + when(enclosingLoc.method()).thenReturn(enclosingMethod); + when(enclosingLoc.codeIndex()).thenReturn(12L); + + when(refType.name()).thenReturn("com.example.WithLambda"); + when(refType.locationsOfLine(99)).thenReturn(List.of(enclosingLoc, lambdaLoc)); + + BreakpointRequest createdBp = mock(BreakpointRequest.class); + EventRequestManager erm = mock(EventRequestManager.class); + when(erm.createBreakpointRequest(enclosingLoc)).thenReturn(createdBp); + + int pendingId = tracker.registerPendingBreakpoint("com.example.WithLambda", 99, 2, "ALL"); + + ClassPrepareEvent event = mockClassPrepareEvent(refType, erm); + runListenerWith(listener, mockEventSet(event)); + + assertThat(eventHistory.getRecent(10)) + .anySatisfy(e -> { + assertThat(e.type()).isEqualTo("BP_MULTI_LOCATION"); + assertThat(e.summary()).contains("#" + pendingId); + assertThat(e.summary()).contains("com.example.WithLambda:99"); + assertThat(e.summary()).contains("2 bytecode locations"); + assertThat(e.summary()).contains("other paths will MISS"); + // Per the new logpoint-vs-breakpoint distinction, this is a plain BP. + assertThat(e.summary()).startsWith("BP "); + }); + } + + @Test + @DisplayName("BP_MULTI_LOCATION label is `LP` when the activated pending entry is a logpoint") + void shouldLabelMultiLocationAsLogpointWhenLogpointMetadataPresent() throws Exception { + ReferenceType refType = mock(ReferenceType.class); + Location enclosingLoc = mock(Location.class); + Location lambdaLoc = mock(Location.class); + com.sun.jdi.Method enclosingMethod = mock(com.sun.jdi.Method.class); + when(enclosingMethod.name()).thenReturn("doWork"); + when(enclosingLoc.method()).thenReturn(enclosingMethod); + when(enclosingLoc.codeIndex()).thenReturn(12L); + + when(refType.name()).thenReturn("com.example.WithLambda"); + when(refType.locationsOfLine(99)).thenReturn(List.of(enclosingLoc, lambdaLoc)); + + BreakpointRequest createdBp = mock(BreakpointRequest.class); + EventRequestManager erm = mock(EventRequestManager.class); + when(erm.createBreakpointRequest(enclosingLoc)).thenReturn(createdBp); + + int pendingId = tracker.registerPendingBreakpoint("com.example.WithLambda", 99, 2, "ALL"); + // Mark this pending entry as a logpoint by attaching an expression. + tracker.setLogpointExpression(pendingId, "x + 1"); + + ClassPrepareEvent event = mockClassPrepareEvent(refType, erm); + runListenerWith(listener, mockEventSet(event)); + + assertThat(eventHistory.getRecent(10)) + .anySatisfy(e -> { + assertThat(e.type()).isEqualTo("BP_MULTI_LOCATION"); + // The diagnostic should use "LP" rather than "BP" because this entry is a logpoint. + assertThat(e.summary()).startsWith("LP "); + }); + } + // ── Test-specific event factory ────────────────────────────────────────── private static ClassPrepareEvent mockClassPrepareEvent(ReferenceType refType, EventRequestManager erm) { From 0c3de5970f1f9cc354a0277c4b222188ca6816c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Sun, 24 May 2026 09:55:11 +0200 Subject: [PATCH 5/9] review: make internal-brace-closes-early test actually exercise that branch The literal was `{a;}+b;` which has no trailing `}`, so isBlockMode rejected it at the cheap end-char guard and never reached the early `depth == 0` branch the test claims to cover. Use `{a;}+b;}` so the input clears the end-char guard and the rejection comes from the brace-balance walk. --- .../evaluation/JdiExpressionEvaluatorRewriteTest.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java index 175f5ed..348fbe7 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java @@ -429,9 +429,12 @@ void shouldNotDetectBlockWhenClosingBraceIsMidExpression() { @Test @DisplayName("isBlockMode: brace-balanced internals followed by expression text are NOT a block") void shouldNotDetectBlockWhenInternalBraceClosesEarly() { - // `{a;}+b;` — the first `}` closes the outer brace at i=4, depth goes to 0 before the - // trailing `}` at i=7. Block detection must reject this. - assertThat(JdiExpressionEvaluator.isBlockMode("{a;}+b;")).isFalse(); + // `{a;}+b;}` — input both starts and ends with a brace (so it clears the cheap + // first/last-char guard), but the FIRST `}` at i=3 drops depth to 0 before the trailing + // `}` at i=7. That trailing brace is therefore a separate closer, not the match for the + // opening one, so this exercises the early `depth == 0` rejection branch — not the + // end-char guard. Block detection must reject it. + assertThat(JdiExpressionEvaluator.isBlockMode("{a;}+b;}")).isFalse(); } @Test From 3af9562bc7b7b8c84202461db449c8a8f759f0cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Sun, 24 May 2026 09:57:29 +0200 Subject: [PATCH 6/9] review: clarify CLASS_PREPARE contract, split BP/LP counts, dedupe describeLocation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses second-round PR #12 review: - Document that CLASS_PREPARE is recorded only when a prepare activates pending work (never for every class load) so the contract is explicit. - Split the line-pending count into BP(s) vs logpoint(s) — both ride the same deferred-line path, so reporting all as "line BP(s)" misattributed logpoints. - Compute describeLocation(location) once and reuse across log/summary/details; it is best-effort and can change/throw during a VM tear-down race. - Add CLASS_PREPARE, BP_MULTI_LOCATION, BP_PROMOTION_FAILED to EventHistory's documented event-type list. - Add test pinning the BP/LP count split. --- .../java/one/edee/mcp/jdwp/EventHistory.java | 3 +- .../one/edee/mcp/jdwp/JdiEventListener.java | 36 ++++++++++++++----- .../JdiEventListenerClassPrepareTest.java | 33 +++++++++++++++++ 3 files changed, 62 insertions(+), 10 deletions(-) diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/EventHistory.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/EventHistory.java index 5e9d1f1..710235d 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/EventHistory.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/EventHistory.java @@ -23,7 +23,8 @@ * `EXCEPTION_LOG_ERROR`, `LOGPOINT`, `LOGPOINT_ERROR`, `FIELD_ACCESS`, `FIELD_MODIFICATION`, * `FIELD_LOGPOINT`, `FIELD_LOGPOINT_ERROR`, `BREAKPOINT_SUPPRESSED`, `EXCEPTION_SUPPRESSED`, * `FIELD_BREAKPOINT_SUPPRESSED`, `CHAIN_ARMED`, `CHAIN_DISARMED`, `CHAIN_BROKEN`, `VM_START`, - * `VM_DEATH`. These are the keys clients can grep on or filter by. + * `VM_DEATH`, `CLASS_PREPARE`, `BP_MULTI_LOCATION`, `BP_PROMOTION_FAILED`. These are the keys + * clients can grep on or filter by. */ @Service public class EventHistory { diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JdiEventListener.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JdiEventListener.java index 1386033..fec9348 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JdiEventListener.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JdiEventListener.java @@ -1046,22 +1046,36 @@ private void handleClassPrepareEvent(ClassPrepareEvent event) { final List> pendingFieldList = breakpointTracker.getPendingFieldBreakpointsForClass(className); + // No deferred work keyed to this class. We deliberately do NOT record a CLASS_PREPARE + // event here: EVERY loaded class fires a prepare, so recording unconditionally would + // bury the one actionable signal — "a class carrying deferred BPs/LPs just loaded" — + // under thousands of irrelevant entries. CLASS_PREPARE is recorded below only when the + // prepare actually activates pending work. if (pendingList.isEmpty() && pendingExList.isEmpty() && pendingFieldList.isEmpty()) { return; } - log.info("[JDI] ClassPrepareEvent for '{}', activating {} deferred breakpoint(s), {} deferred exception breakpoint(s), {} deferred field BP(s)", - className, pendingList.size(), pendingExList.size(), pendingFieldList.size()); + // The pending-line list mixes breakpoints and logpoints (logpoints ride the same + // deferred-line path), so split the counts — reporting everything as "line BP(s)" + // would misattribute deferred logpoints in the diagnostic. + final long lineLpCount = pendingList.stream() + .filter(e -> breakpointTracker.isLogpoint(e.getKey())) + .count(); + final long lineBpCount = pendingList.size() - lineLpCount; + + log.info("[JDI] ClassPrepareEvent for '{}', activating {} deferred line BP(s), {} deferred logpoint(s), {} deferred exception breakpoint(s), {} deferred field BP(s)", + className, lineBpCount, lineLpCount, pendingExList.size(), pendingFieldList.size()); // Surface the CPE to agents via jdwp_get_events. Without this, an agent that sets a // deferred BP and waits on jdwp_resume_until_event sees only [VM_START, VM_DEATH] and // cannot tell whether the class ever loaded — making it impossible to distinguish // "class never loaded" from "class loaded but BP did not fire". eventHistory.record(new EventHistory.DebugEvent("CLASS_PREPARE", - String.format("ClassPrepareEvent for %s, activating %d line BP(s), %d exception BP(s), %d field BP(s)", - className, pendingList.size(), pendingExList.size(), pendingFieldList.size()), + String.format("ClassPrepareEvent for %s, activating %d line BP(s), %d logpoint(s), %d exception BP(s), %d field BP(s)", + className, lineBpCount, lineLpCount, pendingExList.size(), pendingFieldList.size()), Map.of("class", className, - "lineBpCount", String.valueOf(pendingList.size()), + "lineBpCount", String.valueOf(lineBpCount), + "lineLpCount", String.valueOf(lineLpCount), "exceptionBpCount", String.valueOf(pendingExList.size()), "fieldBpCount", String.valueOf(pendingFieldList.size())))); @@ -1112,21 +1126,25 @@ private void handleClassPrepareEvent(ClassPrepareEvent event) { final boolean isLogpoint = breakpointTracker.isLogpoint(id); final String kindUpper = isLogpoint ? "LP" : "BP"; final String kindLower = isLogpoint ? "logpoint" : "breakpoint"; + // describeLocation is best-effort and can change/throw during a VM tear-down + // race, so compute it ONCE and reuse — keeping the log, summary and details + // mutually consistent and avoiding repeated JDI round-trips. + final String boundLocationDesc = describeLocation(location); if (locations.size() > 1) { log.warn("[JDI] Deferred {} {} activated at {}:{} — line has {} Locations; bound only to {} (other paths will MISS this {})", - kindLower, id, className, pending.getLineNumber(), locations.size(), describeLocation(location), kindUpper); + kindLower, id, className, pending.getLineNumber(), locations.size(), boundLocationDesc, kindUpper); eventHistory.record(new EventHistory.DebugEvent("BP_MULTI_LOCATION", String.format("%s #%d at %s:%d — line has %d bytecode locations; bound only to %s (other paths will MISS)", - kindUpper, id, className, pending.getLineNumber(), locations.size(), describeLocation(location)), + kindUpper, id, className, pending.getLineNumber(), locations.size(), boundLocationDesc), Map.of("breakpointId", String.valueOf(id), "kind", kindLower, "class", className, "line", String.valueOf(pending.getLineNumber()), "locationCount", String.valueOf(locations.size()), - "boundLocation", describeLocation(location)))); + "boundLocation", boundLocationDesc))); } else { log.info("[JDI] Deferred {} {} activated at {}:{} ({})", - kindLower, id, className, pending.getLineNumber(), describeLocation(location)); + kindLower, id, className, pending.getLineNumber(), boundLocationDesc); } } catch (AbsentInformationException e) { diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JdiEventListenerClassPrepareTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JdiEventListenerClassPrepareTest.java index a142186..8d4b24d 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JdiEventListenerClassPrepareTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JdiEventListenerClassPrepareTest.java @@ -436,6 +436,39 @@ void shouldLabelMultiLocationAsLogpointWhenLogpointMetadataPresent() throws Exce }); } + @Test + @DisplayName("CLASS_PREPARE summary splits the line-pending count into BP(s) vs logpoint(s)") + void shouldSplitLineBpAndLogpointCountsInClassPrepareSummary() throws Exception { + ReferenceType refType = mock(ReferenceType.class); + Location bpLoc = mock(Location.class); + Location lpLoc = mock(Location.class); + when(refType.name()).thenReturn("com.example.Mixed"); + when(refType.locationsOfLine(10)).thenReturn(List.of(bpLoc)); + when(refType.locationsOfLine(20)).thenReturn(List.of(lpLoc)); + + EventRequestManager erm = mock(EventRequestManager.class); + when(erm.createBreakpointRequest(bpLoc)).thenReturn(mock(BreakpointRequest.class)); + when(erm.createBreakpointRequest(lpLoc)).thenReturn(mock(BreakpointRequest.class)); + + // One plain breakpoint and one logpoint deferred against the same class — both ride the + // pending-line path, so the summary must attribute them to separate buckets. + tracker.registerPendingBreakpoint("com.example.Mixed", 10, 2, "ALL"); + int lpId = tracker.registerPendingBreakpoint("com.example.Mixed", 20, 2, "ALL"); + tracker.setLogpointExpression(lpId, "x + 1"); + + ClassPrepareEvent event = mockClassPrepareEvent(refType, erm); + runListenerWith(listener, mockEventSet(event)); + + assertThat(eventHistory.getRecent(10)) + .anySatisfy(e -> { + assertThat(e.type()).isEqualTo("CLASS_PREPARE"); + assertThat(e.summary()).contains("1 line BP(s)"); + assertThat(e.summary()).contains("1 logpoint(s)"); + assertThat(e.details()).containsEntry("lineBpCount", "1"); + assertThat(e.details()).containsEntry("lineLpCount", "1"); + }); + } + // ── Test-specific event factory ────────────────────────────────────────── private static ClassPrepareEvent mockClassPrepareEvent(ReferenceType refType, EventRequestManager erm) { From f2fb84761aa53658a75793d5adfd11acb2a64456 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Sun, 24 May 2026 10:13:49 +0200 Subject: [PATCH 7/9] review: record BP_MULTI_LOCATION on eager binds; make event-type list exhaustive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously only the deferred (ClassPrepare) activation path recorded a BP_MULTI_LOCATION event + WARN log; the eager and race-guard install paths in jdwp_set_breakpoint / jdwp_set_logpoint only appended a synchronous WARNING suffix. As a result jdwp_get_events showed the diagnostic for deferred binds only. Replace the pure multiLocationSuffix with an instance multiLocationDiagnostic that also logs and records the same BP_MULTI_LOCATION event (identical type and detail keys as the deferred path), so the diagnostic surfaces uniformly regardless of bind timing. The response suffix text is unchanged. Also make EventHistory's documented event-type list exhaustive — it omitted STEP_SUPPRESSED and RECONNECT — and note it must be kept in sync. --- .../java/one/edee/mcp/jdwp/EventHistory.java | 13 ++--- .../java/one/edee/mcp/jdwp/JDWPTools.java | 47 ++++++++++------ .../mcp/jdwp/JDWPToolsSetBreakpointTest.java | 53 ++++++++++++++++++- 3 files changed, 91 insertions(+), 22 deletions(-) diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/EventHistory.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/EventHistory.java index 710235d..81d8ac0 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/EventHistory.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/EventHistory.java @@ -19,12 +19,13 @@ * (MCP tool calls on worker threads) never block each other. * - Cleared on `jdwp_reset`, `jdwp_clear_events`, and {@link JDIConnectionService#cleanupSessionState}. *

    - * Documented event type strings: `BREAKPOINT`, `STEP`, `EXCEPTION`, `EXCEPTION_LOG`, - * `EXCEPTION_LOG_ERROR`, `LOGPOINT`, `LOGPOINT_ERROR`, `FIELD_ACCESS`, `FIELD_MODIFICATION`, - * `FIELD_LOGPOINT`, `FIELD_LOGPOINT_ERROR`, `BREAKPOINT_SUPPRESSED`, `EXCEPTION_SUPPRESSED`, - * `FIELD_BREAKPOINT_SUPPRESSED`, `CHAIN_ARMED`, `CHAIN_DISARMED`, `CHAIN_BROKEN`, `VM_START`, - * `VM_DEATH`, `CLASS_PREPARE`, `BP_MULTI_LOCATION`, `BP_PROMOTION_FAILED`. These are the keys - * clients can grep on or filter by. + * Documented event type strings (exhaustive — keep in sync with every {@code DebugEvent} type + * recorded across the codebase): `BREAKPOINT`, `BREAKPOINT_SUPPRESSED`, `STEP`, `STEP_SUPPRESSED`, + * `EXCEPTION`, `EXCEPTION_SUPPRESSED`, `EXCEPTION_LOG`, `EXCEPTION_LOG_ERROR`, `LOGPOINT`, + * `LOGPOINT_ERROR`, `FIELD_ACCESS`, `FIELD_MODIFICATION`, `FIELD_BREAKPOINT_SUPPRESSED`, + * `FIELD_LOGPOINT`, `FIELD_LOGPOINT_ERROR`, `CHAIN_ARMED`, `CHAIN_DISARMED`, `CHAIN_BROKEN`, + * `CLASS_PREPARE`, `BP_MULTI_LOCATION`, `BP_PROMOTION_FAILED`, `RECONNECT`, `VM_START`, `VM_DEATH`. + * These are the keys clients can grep on or filter by. */ @Service public class EventHistory { 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 3d48c39..dcddfd3 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 @@ -2056,7 +2056,7 @@ public String jdwp_set_breakpoint( // depending on whether the class loaded before or after BP registration. return String.format("Breakpoint set at %s:%d (ID: %d, suspend: %s%s%s%s)", className, lineNumber, pendingId, policyLabel, conditionInfo, chainInfo, - multiLocationSuffix(locations, boundLocation, "BP")); + multiLocationDiagnostic(pendingId, className, lineNumber, locations, boundLocation, "BP")); } } @@ -2097,7 +2097,7 @@ public String jdwp_set_breakpoint( // one; the other code paths through the same line silently miss. return String.format("Breakpoint set at %s:%d (ID: %d, suspend: %s%s%s%s)", className, lineNumber, breakpointId, policyLabel, conditionInfo, chainInfo, - multiLocationSuffix(locations, boundLocation, "BP")); + multiLocationDiagnostic(breakpointId, className, lineNumber, locations, boundLocation, "BP")); } catch (AbsentInformationException e) { cleanupOrphanPendingBreakpoint(pendingIdForCleanup); return "Error: No line number information available for this class. Compile with debug info (-g)."; @@ -2170,7 +2170,7 @@ public String jdwp_set_logpoint( // Multi-location diagnostic — same rationale as jdwp_set_breakpoint. return String.format("Logpoint set at %s:%d (ID: %d, expression: %s%s%s)", className, lineNumber, pendingId, expression, conditionInfo, - multiLocationSuffix(locations, boundLocation, "LP")); + multiLocationDiagnostic(pendingId, className, lineNumber, locations, boundLocation, "LP")); } } @@ -2199,7 +2199,7 @@ public String jdwp_set_logpoint( // Diagnostic — see jdwp_set_breakpoint for the rationale. return String.format("Logpoint set at %s:%d (ID: %d, expression: %s%s%s)", className, lineNumber, breakpointId, expression, conditionInfo, - multiLocationSuffix(locations, boundLocation, "LP")); + multiLocationDiagnostic(breakpointId, className, lineNumber, locations, boundLocation, "LP")); } catch (AbsentInformationException e) { cleanupOrphanPendingBreakpoint(pendingIdForCleanup); return "Error: No line number information available. Compile with debug info (-g)."; @@ -2222,22 +2222,39 @@ private void cleanupOrphanPendingBreakpoint(@Nullable Integer pendingId) { } /** - * Formats the multi-location WARNING suffix appended to BP / LP install responses when a - * source line maps to more than one bytecode {@link Location}. Returns an empty string when - * there is only one Location, so the caller can splat the value into a format string - * unconditionally. {@code kind} is the short label ("BP" / "LP") used in the warning text. - *

    - * Shared by both {@code jdwp_set_breakpoint} / {@code jdwp_set_logpoint} AND their race-guard - * paths (where the class loaded between the initial {@code classesByName} check and the - * ClassPrepareRequest registration) so the diagnostic surfaces uniformly regardless of which - * activation path the install took. + * Emits the multi-location diagnostic for an eager (or race-guard) BP / LP bind and returns the + * WARNING suffix appended to the install response. When the source line maps to more than one + * bytecode {@link Location}, this: + *

      + *
    • logs a WARN line, and
      • + *
    • records a {@code BP_MULTI_LOCATION} {@link EventHistory.DebugEvent} (same type and detail + * keys as the deferred path in {@link JdiEventListener#handleClassPrepareEvent}),
      • + *
    + * so {@code jdwp_get_events} surfaces the diagnostic uniformly whether the bind happened + * immediately or on a later ClassPrepare. Returns an empty string when there is only one + * Location, so the caller can splat the value into a format string unconditionally. {@code kind} + * is the short label ("BP" / "LP") used in the warning text. */ - private static String multiLocationSuffix(List locations, Location boundLocation, String kind) { + private String multiLocationDiagnostic(int id, String className, int lineNumber, + List locations, Location boundLocation, String kind) { if (locations.size() <= 1) { return ""; } + final String kindLower = "LP".equals(kind) ? "logpoint" : "breakpoint"; + final String boundDesc = JdiEventListener.describeLocation(boundLocation); + log.warn("[JDI] {} {} bound at {}:{} — line has {} Locations; bound only to {} (other paths will MISS this {})", + kindLower, id, className, lineNumber, locations.size(), boundDesc, kind); + eventHistory.record(new EventHistory.DebugEvent("BP_MULTI_LOCATION", + String.format("%s #%d at %s:%d — line has %d bytecode locations; bound only to %s (other paths will MISS)", + kind, id, className, lineNumber, locations.size(), boundDesc), + Map.of("breakpointId", String.valueOf(id), + "kind", kindLower, + "class", className, + "line", String.valueOf(lineNumber), + "locationCount", String.valueOf(locations.size()), + "boundLocation", boundDesc))); return String.format(", WARNING: line has %d Locations; bound only to %s — other paths will MISS this %s", - locations.size(), JdiEventListener.describeLocation(boundLocation), kind); + locations.size(), boundDesc, kind); } @McpTool(description = "Clear a breakpoint by its synthetic ID (from jdwp_overview). Routes by kind: line, exception, and field breakpoints share one ID space.") diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsSetBreakpointTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsSetBreakpointTest.java index 65ab456..87eb446 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsSetBreakpointTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsSetBreakpointTest.java @@ -36,6 +36,7 @@ class JDWPToolsSetBreakpointTest { private JDWPTools tools; private VirtualMachine vm; private EventRequestManager erm; + private EventHistory eventHistory; @BeforeEach void setUp() throws Exception { @@ -43,7 +44,7 @@ void setUp() throws Exception { tracker = new BreakpointTracker(); final WatcherManager watcherManager = new WatcherManager(); final JdiExpressionEvaluator evaluator = mock(JdiExpressionEvaluator.class); - final EventHistory eventHistory = new EventHistory(); + eventHistory = new EventHistory(); tools = JDWPToolsTestSupport.newTools( jdiService, tracker, watcherManager, evaluator, eventHistory, new EvaluationGuard(), new JvmDiscoveryService()); @@ -230,4 +231,54 @@ void shouldIncludeConditionInResponseWhenProvided() throws Exception { assertThat(result).contains("condition: i > 0"); } } + + @Nested + @DisplayName("multi-location diagnostic") + class MultiLocationDiagnostic { + + @Test + @DisplayName("eager bind to a line with >1 Location warns in the response AND records BP_MULTI_LOCATION") + void shouldWarnAndRecordEventForMultiLocationEagerBind() throws Exception { + final ReferenceType refType = mock(ReferenceType.class); + final Location bound = mock(Location.class); + final Location lambda = mock(Location.class); + // describeLocation reads method().name() + codeIndex() off the bound Location. + final com.sun.jdi.Method method = mock(com.sun.jdi.Method.class); + when(method.name()).thenReturn("doWork"); + when(bound.method()).thenReturn(method); + when(bound.codeIndex()).thenReturn(7L); + when(jdiService.findLoadedClass("com.example.WithLambda")).thenReturn(refType); + when(refType.locationsOfLine(99)).thenReturn(List.of(bound, lambda)); + when(erm.createBreakpointRequest(bound)).thenReturn(mock(BreakpointRequest.class)); + + final String result = tools.jdwp_set_breakpoint("com.example.WithLambda", 99, "all", null, null, null, null); + + assertThat(result) + .startsWith("Breakpoint set at com.example.WithLambda:99") + .contains("WARNING: line has 2 Locations") + .contains("other paths will MISS this BP"); + + assertThat(eventHistory.getRecent(10)) + .anySatisfy(e -> { + assertThat(e.type()).isEqualTo("BP_MULTI_LOCATION"); + assertThat(e.summary()).startsWith("BP "); + assertThat(e.summary()).contains("com.example.WithLambda:99"); + assertThat(e.summary()).contains("2 bytecode locations"); + assertThat(e.details()).containsEntry("kind", "breakpoint"); + assertThat(e.details()).containsEntry("locationCount", "2"); + }); + } + + @Test + @DisplayName("single-Location bind records NO BP_MULTI_LOCATION event and adds no warning") + void shouldNotWarnOrRecordForSingleLocationBind() throws Exception { + wireEagerSet("com.example.Foo", 10); + + final String result = tools.jdwp_set_breakpoint("com.example.Foo", 10, "all", null, null, null, null); + + assertThat(result).doesNotContain("WARNING"); + assertThat(eventHistory.getRecent(10)) + .noneSatisfy(e -> assertThat(e.type()).isEqualTo("BP_MULTI_LOCATION")); + } + } } From e7d89c9f0b4946571c32f09f805780886d13a745 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Sun, 24 May 2026 10:46:26 +0200 Subject: [PATCH 8/9] review: gate race-guard multi-location diagnostic on promotion win; doc producers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thread 2 & 3 (real bug): both jdwp_set_breakpoint and jdwp_set_logpoint race-guard recheck paths emitted multiLocationDiagnostic (WARN log + BP_MULTI_LOCATION event + response suffix) unconditionally — even when promotePendingToActive lost the race and the just-created request was deleted. The winning path (the ClassPrepare listener) already records its own BP_MULTI_LOCATION, so this double-counted. Move the diagnostic into the promotion-won branch; the lost-race return now notes "(bound by a concurrent activation path)" without re-warning. +2 tests (win/lose race). Thread 1: EventHistory is not single-producer — reword the class javadoc to acknowledge install-time records from MCP worker threads (BP_MULTI_LOCATION from JDWPTools, RECONNECT from JDIConnectionService) and that no global ordering is implied across producers. --- .../java/one/edee/mcp/jdwp/EventHistory.java | 12 ++- .../java/one/edee/mcp/jdwp/JDWPTools.java | 32 +++++--- .../mcp/jdwp/JDWPToolsSetBreakpointTest.java | 73 ++++++++++++++++++- 3 files changed, 103 insertions(+), 14 deletions(-) diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/EventHistory.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/EventHistory.java index 81d8ac0..b1cdb0e 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/EventHistory.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/EventHistory.java @@ -10,13 +10,17 @@ import java.util.concurrent.ConcurrentLinkedDeque; /** - * Bounded ring buffer of debug events captured from the JDI event queue. Records every interesting - * event for post-mortem inspection via the `jdwp_get_events` tool. + * Bounded ring buffer of debug events for post-mortem inspection via the `jdwp_get_events` tool. + * Most events originate from the JDI event queue drained by {@link JdiEventListener}, but it is NOT + * the sole producer: synchronous install-time diagnostics are recorded from MCP worker threads too + * (e.g. {@code BP_MULTI_LOCATION} from {@link JDWPTools}, {@code RECONNECT} from + * {@link JDIConnectionService}). *

    * Behaviour: * - FIFO ring buffer with a fixed cap of {@link #MAX_EVENTS} entries; oldest entries are dropped first. - * - Thread-safe via {@link ConcurrentLinkedDeque}; the producer ({@link JdiEventListener}) and consumers - * (MCP tool calls on worker threads) never block each other. + * - Thread-safe via {@link ConcurrentLinkedDeque}: the listener thread and the MCP worker threads may + * record concurrently, and concurrent readers never block writers. Because there are multiple + * producers, no global ordering is guaranteed beyond each individual {@code record} call. * - Cleared on `jdwp_reset`, `jdwp_clear_events`, and {@link JDIConnectionService#cleanupSessionState}. *

    * Documented event type strings (exhaustive — keep in sync with every {@code DebugEvent} type 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 dcddfd3..dc8f9e6 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 @@ -2041,19 +2041,27 @@ public String jdwp_set_breakpoint( final BreakpointRequest bpRequest = erm.createBreakpointRequest(boundLocation); bpRequest.setSuspendPolicy(jdiPolicy); if (!breakpointTracker.promotePendingToActive(pendingId, bpRequest)) { - // Another path (listener or safety-net) won the promotion race; tear - // down the loser request so no duplicate fires on the target VM. + // Another path (the ClassPrepare listener or the safety-net promoter) won + // the promotion race; tear down the loser request so no duplicate fires on + // the target VM. Crucially, do NOT emit the multi-location diagnostic here: + // the winning path owns the binding and (for the listener) already recorded + // its own BP_MULTI_LOCATION — emitting again would double-count. try { erm.deleteEventRequest(bpRequest); } catch (Exception ignored) { // VM may already be disconnected — best-effort cleanup. } - } else if (triggerBreakpointId == null) { + return String.format("Breakpoint set at %s:%d (ID: %d, suspend: %s%s%s) " + + "(bound by a concurrent activation path)", + className, lineNumber, pendingId, policyLabel, conditionInfo, chainInfo); + } + if (triggerBreakpointId == null) { bpRequest.setEnabled(true); } - // Multi-location diagnostic — must apply to the race-guard path too, not - // just the eager path, otherwise lambda-bearing lines silently miss - // depending on whether the class loaded before or after BP registration. + // Won the promotion — emit the multi-location diagnostic for THIS bind. It must + // apply to the race-guard path too, not just the eager path, otherwise + // lambda-bearing lines silently miss depending on whether the class loaded + // before or after BP registration. return String.format("Breakpoint set at %s:%d (ID: %d, suspend: %s%s%s%s)", className, lineNumber, pendingId, policyLabel, conditionInfo, chainInfo, multiLocationDiagnostic(pendingId, className, lineNumber, locations, boundLocation, "BP")); @@ -2159,15 +2167,21 @@ public String jdwp_set_logpoint( bpRequest.setSuspendPolicy(jdiPolicy); bpRequest.enable(); if (!breakpointTracker.promotePendingToActive(pendingId, bpRequest)) { - // Another path (listener or safety-net) won the promotion race; tear - // down the loser request so no duplicate fires on the target VM. + // Another path (the ClassPrepare listener or the safety-net promoter) won + // the promotion race; tear down the loser request so no duplicate fires on + // the target VM. Do NOT emit the multi-location diagnostic here — the + // winning path owns the binding and its diagnostics. try { erm.deleteEventRequest(bpRequest); } catch (Exception ignored) { // VM may already be disconnected — best-effort cleanup. } + return String.format("Logpoint set at %s:%d (ID: %d, expression: %s%s) " + + "(bound by a concurrent activation path)", + className, lineNumber, pendingId, expression, conditionInfo); } - // Multi-location diagnostic — same rationale as jdwp_set_breakpoint. + // Won the promotion — emit the multi-location diagnostic for THIS bind. Same + // rationale as jdwp_set_breakpoint. return String.format("Logpoint set at %s:%d (ID: %d, expression: %s%s%s)", className, lineNumber, pendingId, expression, conditionInfo, multiLocationDiagnostic(pendingId, className, lineNumber, locations, boundLocation, "LP")); diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsSetBreakpointTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsSetBreakpointTest.java index 87eb446..88416e8 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsSetBreakpointTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsSetBreakpointTest.java @@ -18,7 +18,11 @@ import java.util.List; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -41,7 +45,9 @@ class JDWPToolsSetBreakpointTest { @BeforeEach void setUp() throws Exception { jdiService = mock(JDIConnectionService.class); - tracker = new BreakpointTracker(); + // Spy (delegates to the real impl) so the race-guard tests can force promotePendingToActive + // to win/lose the promotion race deterministically; all other tests see real behaviour. + tracker = spy(new BreakpointTracker()); final WatcherManager watcherManager = new WatcherManager(); final JdiExpressionEvaluator evaluator = mock(JdiExpressionEvaluator.class); eventHistory = new EventHistory(); @@ -281,4 +287,69 @@ void shouldNotWarnOrRecordForSingleLocationBind() throws Exception { .noneSatisfy(e -> assertThat(e.type()).isEqualTo("BP_MULTI_LOCATION")); } } + + @Nested + @DisplayName("race-guard multi-location diagnostic") + class RaceGuardMultiLocationDiagnostic { + + /** + * Drives the race-guard recheck path: the class is NOT loaded eagerly, so a pending entry + + * ClassPrepareRequest are registered, but {@code vm.classesByName} then finds it (it loaded + * between the two checks), and the tool binds inline. + */ + private ReferenceType wireRaceGuard(String className, int line, Location... locations) throws Exception { + final ReferenceType refType = mock(ReferenceType.class); + when(jdiService.findLoadedClass(className)).thenReturn(null); + when(erm.createClassPrepareRequest()).thenReturn(mock(ClassPrepareRequest.class)); + when(vm.classesByName(className)).thenReturn(List.of(refType)); + when(refType.locationsOfLine(line)).thenReturn(List.of(locations)); + when(erm.createBreakpointRequest(locations[0])).thenReturn(mock(BreakpointRequest.class)); + return refType; + } + + @Test + @DisplayName("losing the promotion race emits NO BP_MULTI_LOCATION and notes the concurrent path") + void shouldNotRecordMultiLocationWhenPromotionLost() throws Exception { + final Location bound = mock(Location.class); + final Location lambda = mock(Location.class); + wireRaceGuard("com.example.Lam", 50, bound, lambda); + // Another path already promoted this id — our inline bind loses the race. + doReturn(false).when(tracker).promotePendingToActive(anyInt(), any()); + + final String result = tools.jdwp_set_breakpoint("com.example.Lam", 50, "all", null, null, null, null); + + assertThat(result) + .startsWith("Breakpoint set at com.example.Lam:50") + .contains("bound by a concurrent activation path") + .doesNotContain("WARNING"); + // The winning path owns the diagnostic; we must not double-record it. + assertThat(eventHistory.getRecent(10)) + .noneSatisfy(e -> assertThat(e.type()).isEqualTo("BP_MULTI_LOCATION")); + } + + @Test + @DisplayName("winning the promotion race records BP_MULTI_LOCATION and warns") + void shouldRecordMultiLocationWhenPromotionWon() throws Exception { + final Location bound = mock(Location.class); + final Location lambda = mock(Location.class); + final com.sun.jdi.Method method = mock(com.sun.jdi.Method.class); + when(method.name()).thenReturn("doWork"); + when(bound.method()).thenReturn(method); + when(bound.codeIndex()).thenReturn(3L); + wireRaceGuard("com.example.Lam", 50, bound, lambda); + doReturn(true).when(tracker).promotePendingToActive(anyInt(), any()); + + final String result = tools.jdwp_set_breakpoint("com.example.Lam", 50, "all", null, null, null, null); + + assertThat(result) + .startsWith("Breakpoint set at com.example.Lam:50") + .contains("WARNING: line has 2 Locations") + .doesNotContain("concurrent activation path"); + assertThat(eventHistory.getRecent(10)) + .anySatisfy(e -> { + assertThat(e.type()).isEqualTo("BP_MULTI_LOCATION"); + assertThat(e.details()).containsEntry("locationCount", "2"); + }); + } + } } From 54da83b03a8231383a834ccf0a167a98b95ad9c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Sun, 24 May 2026 10:57:07 +0200 Subject: [PATCH 9/9] review: align EventHistory.record javadoc with multi-producer reality; doc fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - record() method javadoc no longer claims it is called exclusively from JdiEventListener — it documents the multi-producer, any-thread contract to match the class-level javadoc. - Scope the "exhaustive" event-type list to production code (tests record ad-hoc types like "TEST" that are intentionally not listed). - Drop the @link to the private JdiEventListener#handleClassPrepareEvent (doclint can't resolve a private member) — link the class and describe the deferred path in words instead. --- .../src/main/java/one/edee/mcp/jdwp/EventHistory.java | 9 ++++++--- .../src/main/java/one/edee/mcp/jdwp/JDWPTools.java | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/EventHistory.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/EventHistory.java index b1cdb0e..979fc9d 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/EventHistory.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/EventHistory.java @@ -23,8 +23,8 @@ * producers, no global ordering is guaranteed beyond each individual {@code record} call. * - Cleared on `jdwp_reset`, `jdwp_clear_events`, and {@link JDIConnectionService#cleanupSessionState}. *

    - * Documented event type strings (exhaustive — keep in sync with every {@code DebugEvent} type - * recorded across the codebase): `BREAKPOINT`, `BREAKPOINT_SUPPRESSED`, `STEP`, `STEP_SUPPRESSED`, + * Documented event type strings (exhaustive for production code — keep in sync; tests may record + * ad-hoc types that are intentionally not listed here): `BREAKPOINT`, `BREAKPOINT_SUPPRESSED`, `STEP`, `STEP_SUPPRESSED`, * `EXCEPTION`, `EXCEPTION_SUPPRESSED`, `EXCEPTION_LOG`, `EXCEPTION_LOG_ERROR`, `LOGPOINT`, * `LOGPOINT_ERROR`, `FIELD_ACCESS`, `FIELD_MODIFICATION`, `FIELD_BREAKPOINT_SUPPRESSED`, * `FIELD_LOGPOINT`, `FIELD_LOGPOINT_ERROR`, `CHAIN_ARMED`, `CHAIN_DISARMED`, `CHAIN_BROKEN`, @@ -42,7 +42,10 @@ public class EventHistory { /** * Appends an event and evicts the oldest entries until the buffer is at or below {@link #MAX_EVENTS}. - * Called exclusively from {@link JdiEventListener} on the JDI event listener thread; non-blocking. + * Non-blocking, and safe to call from any thread: most records come from the JDI event listener + * thread ({@link JdiEventListener}), but MCP worker threads also record install-time diagnostics + * (e.g. {@code BP_MULTI_LOCATION}, {@code RECONNECT}). The backing {@link ConcurrentLinkedDeque} + * tolerates these concurrent producers. */ public void record(DebugEvent event) { events.addLast(event); 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 dc8f9e6..1532a0d 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 @@ -2242,7 +2242,7 @@ private void cleanupOrphanPendingBreakpoint(@Nullable Integer pendingId) { *

      *
    • logs a WARN line, and
      • *
    • records a {@code BP_MULTI_LOCATION} {@link EventHistory.DebugEvent} (same type and detail - * keys as the deferred path in {@link JdiEventListener#handleClassPrepareEvent}),
      • + * keys as the deferred class-prepare path in {@link JdiEventListener}), *
    * so {@code jdwp_get_events} surfaces the diagnostic uniformly whether the bind happened * immediately or on a later ClassPrepare. Returns an empty string when there is only one