diff --git a/.plans/0001-legacy-environment.md b/.plans/0002-legacy-environment.md similarity index 100% rename from .plans/0001-legacy-environment.md rename to .plans/0002-legacy-environment.md diff --git a/.plans/0003-awt-edt-threading.md b/.plans/0003-awt-edt-threading.md new file mode 100644 index 00000000..5d10865d --- /dev/null +++ b/.plans/0003-awt-edt-threading.md @@ -0,0 +1,196 @@ +# Plan: Route Fiji UI mutation through the AWT Event Dispatch Thread + +> Status: **RFC — deferred from the `awt-refs` branch.** Surfaced by the +> `/review` security pass (EDT violation at the MCP `call_action` trust +> boundary). Not folded into this PR because the fix is cross-cutting (every +> Fiji-touching event shares the pattern) and an inconsistent single-path wrap +> would be worse than none. This document captures the analysis so it is not lost. + +## Context + +AWT's single-thread rule requires all component reads/writes and event dispatch +to happen on the **Event Dispatch Thread (EDT)**. CopilotJ's control paths +violate this: every Fiji-touching event (`run_action`, `run_macro`, `run_script`, +`capture_screen`, `capture_image`) executes on the **Jetty worker thread** (MCP) +or the **WebSocket/daemon thread** (bridge), never on the EDT. + +This is pre-existing — the macro/script paths have always done this — but the +AWT ref-model branch (`awt-refs`) makes it more prominent by exposing structured +component mutation (`Checkbox.setState`, `Scrollbar.setValue`, `Choice.select`, +`TextField.setText`, `List.select`) plus synthesized AWT events (`ItemEvent`, +`AdjustmentEvent`, `TextEvent`, `ActionEvent`) directly to caller-supplied input. + +**Current call chain (`run_action`), all off-EDT:** + +``` +Jetty worker + → McpModule.callEvent (plugin/.../mcp/McpModule.java:185) + → EventHandler.handle (plugin/.../EventHandler.java:158) + → case "run_action" (EventHandler.java:247) + → SnapshotManager.runAction (SnapshotManager.java:107) + → new Snapshot(...).runAction(ref, action, params) + → node.runAction(action, params) (Snapshot.java:236) + → e.g. CheckboxNode.setState (CheckboxNode.java:87) + → component.setState(...) + fire ItemEvent ← off-EDT AWT mutation ❌ +``` + +**Why it mostly works today:** ImageJ's own macro engine, `IJ.run(...)`, and most +SciJava/ImageJ2 listeners are themselves EDT-lax and tolerate off-EDT calls. The +MCP server binds `127.0.0.1`, so the only caller is the user's own local client. +No field data has been lost yet. + +**Why it is still a landmine:** + +1. Deadlock with `RepaintManager` if a listener triggers a synchronous repaint + while the EDT is mid-paint (lock inversion). +2. Listeners that assume the EDT (e.g. call `SwingUtilities`-gated APIs, or + `ModalContext`) throw on the Jetty thread — surfaces to the client as a + generic `InvocationTargetException` / MCP `isError`. +3. Torn state: model updated off-EDT while EDT reads → flicker / inconsistent UI. + +## Decision log + +| Decision | Choice | Why | +| ---------------------------- | ----------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------- | +| Fix in the AWT ref-model PR? | **No — defer** | Too large / cross-cutting; `run_macro`/`run_script`/`capture` share the pattern; an inconsistent single-path fix is worse than none | +| Scope of the fix | **Holistic — all Fiji-touching events** | Consistency; do EDT once, across every path that mutates (or reads) AWT | +| Wrapper primitive | `EventQueue.invokeAndWait` (preferred) | Actions need the result synchronously to build the response; `invokeLater` would require a callback/future shape | +| Where to wrap | **One chokepoint in `EventHandler.handle`** (via an `onEdt(Callable)` helper) | Avoids scattering EDT logic across nodes; `Snapshot`/`ScriptRunner`/`ScreenCapturer` stay thread-agnostic | +| Called _from_ the EDT | Detect & run inline (`isDispatchThread()` short-circuit) | `invokeAndWait` throws `Error` when invoked on the EDT itself | + +## Scope — paths that touch AWT off-EDT + +All dispatch through `EventHandler.handle` (plugin/.../EventHandler.java:204): + +| Event | AWT touch | EDT-critical? | +| ------------------- | -------------------------------------------------------------- | ------------------------------------------------- | +| `run_action` | `component.setState/setValue/select/setText`, fires AWT events | **Yes** (component mutation + listener callbacks) | +| `run_script` | `IJ.run(...)`, macro engine touches UI | **Yes** | +| `take_snapshot` | builds Snapshot tree (reads AWT components) | Read-only but reads AWT state | +| `capture_screen` | `Window.getWindows()`, screenshots | Read-only | +| `capture_image` | `ImagePlus.getBufferedImage()` | Read-only | +| `compare_snapshots` | builds Snapshots | Read-only | + +The mutation paths (`run_action`, `run_script`) are the hard requirement; the +read paths should be wrapped too for consistency so "is this on the EDT?" has +one answer. + +## Proposed approach + +### 1. A single EDT chokepoint + +New helper (e.g. `copilotj.util.AwtEventQueue`, or inline in `EventHandler`): + +```java +/** Runs a Callable on the EDT and returns its result; runs inline if already on the EDT. */ +static T onEdt(final java.util.concurrent.Callable task) { + if (java.awt.EventQueue.isDispatchThread()) { + try { return task.call(); } + catch (RuntimeException e) { throw e; } + catch (Exception e) { throw new RuntimeException(e); } + } + final Object[] holder = new Object[1]; + try { + java.awt.EventQueue.invokeAndWait(() -> { + try { holder[0] = task.call(); } + catch (RuntimeException e) { holder[0] = e; } + catch (Exception e) { holder[0] = new RuntimeException(e); } + }); + } catch (java.lang.reflect.InvocationTargetException e) { + final Throwable c = e.getCause(); + if (c instanceof RuntimeException re) throw re; + throw new RuntimeException(c); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new RuntimeException("EDT task interrupted", e); + } + if (holder[0] instanceof RuntimeException re) throw re; + @SuppressWarnings("unchecked") final T result = (T) holder[0]; + return result; +} +``` + +### 2. Wrap the event cases in `EventHandler.handle` + +```java +case "run_action": { + final SnapshotManager.ActionRequest request = objectMapper.convertValue(payload.data, + SnapshotManager.ActionRequest.class); + final Action.Response result = onEdt(() -> snapshotManager.runAction(request)); + return objectMapper.valueToTree(result); +} +``` + +Do the same for `run_script`, `take_snapshot`, `capture_screen`, `capture_image`, +`compare_snapshots`. + +### 3. Leave node / Snapshot / ScriptRunner code as-is + +They stay thread-agnostic — they run on whatever thread calls them. The EDT +discipline lives only in `EventHandler`. No per-node changes. + +## Risks / tradeoffs + +- **Blocks the Jetty/WS thread on the EDT** (`invokeAndWait` blocks until the EDT + runs the task). Acceptable for a local single-user server; the existing + per-request timeout (`ScriptRequest.timeout`) still bounds wall clock. +- **UI freeze during long actions.** Because the EDT is single-threaded, a long + `run_action`/`run_script` on the EDT freezes the whole Fiji UI for its + duration. Today the UI can repaint during a long action; after the fix it + cannot. This is _correct_ EDT behavior but a UX change. **Mitigation**: scope + the first cut to short actions; for genuinely long work, design an async + variant (`SwingWorker` / `invokeLater` + polling) — a larger follow-up. +- **`invokeAndWait` from the EDT throws `Error`** — mitigated by the + `isDispatchThread()` short-circuit. **Audit** whether any ImageJ listener ever + re-enters `EventHandler` synchronously (see Open questions). +- **Managed mode** (`COPILOTJ_MANAGED=1`): `EventHandler.handle` runs on the + daemon asyncio thread, not Jetty — the same `onEdt(...)` applies; Fiji owns the + EDT regardless, so it resolves. + +## Files to modify + +| File | Change | +| ---------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------- | +| `plugin/.../util/AwtEventQueue.java` (new) **or** `EventHandler` | `onEdt(Callable)` helper | +| `plugin/.../EventHandler.java` | wrap `run_action` (+ `run_script`, `take_snapshot`, `capture_screen`, `capture_image`, `compare_snapshots`) bodies in `onEdt(...)` | + +No changes to `SnapshotManager`, `Snapshot`, the component `*Node.runAction`, +`ScriptRunner`, or `ScreenCapturer`. + +## Verification + +1. `just build-plugin` — compiles. +2. `SwingUtilities.isEventDispatchThread()` probe: temporarily assert inside one + node's `runAction` that `EventQueue.isDispatchThread()` is true → passes after + the wrap, fails before. +3. Smoke: `python3 scripts/mcp_smoke_test.py` — all checks pass; the + round-trip / stale-ref checks now exercise the EDT path. Watch for deadlocks + under `call_action` on Threshold / Brightness·Contrast dialogs (their + listeners repaint). +4. Stress: rapid `run_macro` + `call_action` interleaving under a dialog with + active listeners — no deadlock, no partial repaint. +5. Managed mode: run under `COPILOTJ_MANAGED=1`; confirm `onEdt` still resolves + the Fiji-owned EDT. + +## Open questions + +- Does any ImageJ listener synchronously re-enter `EventHandler` (which would + put us on the EDT mid-`handle`)? The `isDispatchThread()` short-circuit + handles it, but audit `ImagejListener` / `WindowListener` callbacks. +- Wrap the read-paths (`take_snapshot` / `capture_*`) on the EDT too + (consistency) or leave them off (cheap, read-only)? Recommendation: wrap them, + so the EDT question has one answer everywhere. +- Long-action UX: accept the UI freeze for actions >1s (EDT-correct), or design + an async variant for known-long actions up front? + +## Trigger + +Pick this RFC up when any of: + +- A deadlock or partial repaint is observed during `call_action` on a dialog with + active listeners (Threshold, Brightness·Contrast, Color Picker), **or** +- The Python-bridge `call_action` is wired to a real agent (more call volume → + more exposure), **or** +- A new Fiji-touching event makes the off-EDT inconsistency costly. + +Until then, ImageJ's EDT-lax behavior tolerates the current off-EDT mutation. diff --git a/.plans/0004-mcp-yaml-snapshot-format.md b/.plans/0004-mcp-yaml-snapshot-format.md new file mode 100644 index 00000000..c60d5e5c --- /dev/null +++ b/.plans/0004-mcp-yaml-snapshot-format.md @@ -0,0 +1,200 @@ +# Plan: Port the YAML snapshot "access format" to the Java MCP server + +> Status: **RFC — deferred from the `awt-refs` branch.** The `awt-refs` branch added a +> playwright-mcp-style ref model on both the Python and Java sides, but the two consumers +> of the Fiji snapshot present it in **different formats**: the Python path renders a compact +> YAML-like text tree, while the Java MCP `take_snapshot` tool returns raw JSON. This RFC +> captures the design for making them symmetric. Not folded into the resource-cleanup change +> because it is a non-trivial renderer + test-infrastructure effort, and the raw-JSON output +> is correct (if verbose) in the meantime. + +## Context + +CopilotJ has two consumers of the Fiji AWT widget snapshot: + +1. **Python multi-agent path** — `take_snapshot` over the WebSocket bridge → JSON → + deserialized into typed pydantic models (`copilotj/plugin/awt/...`) → rendered as a + compact **YAML-like text tree** via `Response._describe()`. This is the "YAML-like access + format" the LLM reads to pick a ref, e.g.: + + ``` + Snapshot #1 (image: blobs.tif, screen: 1920x1080, scale: 1.00) + - window "Brightness/Contrast" [ref=e1] (id=1): + - scrollbar "Minimum" [ref=e3] (setValue): value=0 + - label: 99.33 % + - button "Apply" [ref=e9] (click) + ``` + +2. **Java MCP path** (`plugin/.../mcp/`) — `take_snapshot` tool → `McpModule.callEvent` → + `EventHandler.handle` → `objectMapper.valueToTree(snapshot)` → **pretty-printed JSON** of + the raw AWT tree. An MCP client (LLM) gets JSON, not the compact YAML. + +**The asymmetry:** Python renders the compact YAML; Java MCP serves raw JSON. Goal: port the +YAML rendering to Java so the MCP `take_snapshot` emits the same compact, ref-annotated tree. + +**Two further gaps in the Java output (beyond the format):** + +- Java has no equivalent of Python's per-window **flattening** (`IjContrastAdjuster`, + `IjThresholdAdjuster`, `IjGenericDialog` merge adjacent labels into widget names, inline + button panels as a transparent `Buttons` group, drop redundant mirrored text fields). Java + renders the raw nested AWT tree for these dialogs. +- Java `describe()` (`ComponentNode`) is a generic single-line stub (`"Button: label=Apply"`, + `"Container"`), used only by `print()` debugging — not by the MCP tool. + +## Decision log + +| Decision | Choice | Why | +| ------------------------ | ---------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| Port now or defer? | **Defer (this RFC)** | Renderer + new JUnit test infra is a sizable, self-contained effort; raw JSON is correct meanwhile | +| Where to render YAML | **New `SnapshotFormatter` class** (pure functions over the node tree) | Mirrors Python's data/view split; keeps `ComponentNode` pure data; trivially unit-testable without live AWT; do **not** overload `describe()` | +| The MCP seam | **Inside `TakeSnapshotTool`**, not `EventHandler` | `EventHandler`'s `take_snapshot` case serves **both** the Python bridge (needs JSON — it discriminates window subclass by the `type` field) and the MCP tool. YAML applies only on the MCP path. | +| Get the snapshot | `handler.getSnapshotManager().capture()` (new public accessor) | `capture()` already `deactivate()`s (SnapshotManager.java:72) and returns a snapshot whose **captured** fields are intact; `runAction()` uses a fresh live snapshot independently. No JSON round-trip. | +| Format exposure | **`take_snapshot` tool returns YAML** (resource layer removed in the prior change) | `take_snapshot`/`fiji_environment` tools already exist; the redundant `fiji://windows`/`fiji://environment` resources were removed. The tool is the LLM-facing view. | +| Programmatic JSON access | **Gone from the tool** | The smoke test is the only programmatic consumer; it is rewritten to **parse the YAML** (the real contract the LLM sees). | +| Flattening | **Phase 1 generic only; flatteners deferred** | Generic renderer ships the symmetric interaction mode for all windows now; per-dialog flatteners are pure polish, byte-match later. | +| Java verification | **Add JUnit 5 + `SnapshotFormatterTest`** | Formatter is pure; mirror `test_awt_snapshot.py` case-by-case for byte-level grammar verification without running Fiji. | + +## Scope + +The MCP `take_snapshot` tool today (`plugin/.../mcp/tools/TakeSnapshotTool.java`) calls +`McpModule.callEvent(handler, "take_snapshot", null)` and wraps the JSON in `TextContent`. +The change moves the tool to render YAML and leaves every other MCP tool, the bridge, and the +Python side untouched. + +| Path | Today | After | +| -------------------------------------------- | --------------- | ---------------------------------- | +| MCP `take_snapshot` tool | raw JSON | **YAML text** | +| MCP `fiji_environment` tool | JSON | JSON (unchanged) | +| `EventHandler` `take_snapshot` case (bridge) | JSON | JSON (unchanged — Python needs it) | +| Python `Response._describe()` rendering | YAML | YAML (unchanged) | +| `scripts/mcp_smoke_test.py` snapshot helpers | parse tool JSON | parse tool **YAML** | + +## The YAML grammar (the spec) + +Authoritative source: `copilotj/test/plugin/test_awt_snapshot.py` + `copilotj/plugin/awt/_base.py`. +The Java renderer must reproduce these exactly. + +- **Snapshot header:** `Snapshot # (image: , screen: x, scale: )` + then each window's lines. No windows → `no window opened`. +- **Node line:** `- "" [ref=eN] : ` — any segment may be omitted: + - `role`: button|checkbox|choice|scrollbar|list|textfield|textarea|label|canvas|unknown|container|window + - `"name"`: quoted — button/checkbox→label, window→title (IjImage/IjTextWindow) or forced "ImageJ"/"ImageJ Console", unknown→name; others→omitted. + - `[ref=eN]`: only when ref non-null (labels & intermediate containers have none — `isRefEligible()=false`). + - `extras`: windows emit `(id=)`; others none. + - `actions`: `(id1,id2,…)` comma-joined **short** ids. Omit if none. Reuse `Action.shortId()` (`lastIndexOf('.')`). + - `: state`: scrollbar→`value=` (orientation is **not** emitted); checkbox→`checked=`; choice/list→`selected= items=[…]`; textfield/textarea→`text=`; label→raw sanitized text (→space, truncate 300+`…`, **no quotes**); button/canvas/unknown/window-leaf→none. +- **Containers:** render `- :` (head + colon, no inline state) then each child subtree indented **2 spaces**. Verbosity LOW hides children (window renders as just `- window … (id=N)`). +- **Windows:** head includes `(id=N)` and name=title; render head + `:` then indented children — except `IjImage`/`IjTextWindow`, which render bespoke detail lines (see below). +- Helpers: `strOrEmpty(null|"")→""`, else quoted+escaped (`\n→\\n`, `\r→\\r`, `\t→\\t`), max 300 then `"…"`. `formatItems` max 8, quoted, trailing `, ...`. Label text is raw (separate helper: no quotes, whitespace→space, truncate 300+`…`). + +Bespoke windows (mirror `copilotj/plugin/awt/window/ij_image.py`, `ij_text_window.py`, `ij_imagej.py`, `ij2_console.py`): + +- `IjImage`: head+`:`, then `type: , size: , path: ` (`typee = bitDepth+"-bit "+imageType` unless already contained); `dimension: …`, `stack: … (channels, slices, frames)`; if calibrated: `calibrated: …`; `resolution: …` (+`, zoom=…` when ≠1); roi one-liner. LOW truncates after the type/size/path line. +- `IjTextWindow`: head+`:`, then `Results Table: (size: N, headings: …)` or `no results table`. +- `IjImageJ` / `Ij2ConsoleWindow`: header line only, forced names "ImageJ" / "ImageJ Console". + +## Proposed approach + +### 1. NEW `plugin/src/main/java/copilotj/awt/SnapshotFormatter.java` + +Pure functions over the existing node tree (no AWT, no Jackson). Methods (mirror Python's +`_describe` family; return `List<String>` internally, join with `"\n"` at the top): +`render(Snapshot)` / `render(Snapshot, Verbosity)`, `describeNode(...)`, container path, +`describeChildren(...)` (Phase-2 flattener hook — no-op in Phase 1), `yamlHead`, `yamlLine`, +`stateInline`, `nodeName` (class→role/name map; force "ImageJ"/"ImageJ Console"), +`headExtras` (windows→`(id=N)`), `actionSegment`, the four bespoke window methods, and +helpers `strOrEmpty` / `formatItems` / `sanitizeLabelText`. + +### 2. MODIFY `TakeSnapshotTool.handle` + +```java +Snapshot snap = handler.getSnapshotManager().capture(); // already deactivate()d; captured fields intact +String yaml = SnapshotFormatter.render(snap, Verbosity.NORMAL); +return CallToolResult(TextContent(yaml)); +``` + +No `deactivate()` needed. Input schema unchanged (no params). Update `definition()` description +to note YAML rendering (keep substring "ref" so the smoke-test metadata check still passes). + +### 3. MODIFY `EventHandler.java` + +Add a public accessor (cross-package caller in `copilotj.mcp.tools`): + +```java +public SnapshotManager getSnapshotManager() { return snapshotManager; } +``` + +Do **not** touch the `"take_snapshot"` case. + +### 4. MODIFY `scripts/mcp_smoke_test.py` + +Rewrite the snapshot helpers to parse the YAML text from `take_snapshot` (regular grammar): +track the current window from `- window "<title>" …(id=N):` lines; a node-line regex extracts +role / `"<name>"` / `[ref=e(\d+)]` / `(actions)` / trailing `: <state>`. + +- `find_action_ref(yaml, action_short_id, window_title=None, label=None)` — match on the short id (e.g. `setState`, not the `.setState` suffix) and optional name. +- `checkbox_state_by_ref(yaml, ref)` — parse `checked=<true|false>` on that ref's line. + The `take_snapshot` tool check asserts the text starts with `Snapshot #`. + +### 5. ADD JUnit 5 + `plugin/src/test/java/copilotj/awt/SnapshotFormatterTest.java` + +Add `org.junit.jupiter:junit-jupiter` (test scope) to `plugin/pom.xml`; mirror +`test_awt_snapshot.py` with lightweight `ComponentNode`/`ContainerNode` stubs (only +`getType/getName/getRef/getActions/isRefEligible/getChildren` needed): button, choice, label, +checkbox, scrollbar, window-tree-two-space-indent, low-verbosity-hides-children. Add a +`just test-plugin` target (`cd plugin && mvn test`); keep `just build-plugin` as +`mvn package -DskipTests`. + +## Phases + +- **Phase 1 (this RFC, when picked up):** generic renderer + bespoke IjImage/IjTextWindow/ImageJ/Console. Every window renders correctly with all refs/actions; specialized dialogs show the raw nested tree (not byte-identical to Python). +- **Phase 2 (follow-up):** per-window flatteners (ContrastAdjuster, ThresholdAdjuster, GenericDialog) + transparent `Buttons` group, so Java YAML byte-matches Python. Likely introduces a `RenderedNode` IR the formatter renders, with flatteners as `List<ComponentNode> → List<RenderedNode>` transforms (keeps JSON/bridge output raw). Also capture dialog/window titles for `IjGenericDialog`/`UnknownWindow` (today they rely on AWT `getName()`, often null). + +## Risks / tradeoffs + +- **Loss of structured JSON from the tool.** The smoke test is the only known programmatic consumer and is rewritten to parse YAML. Any future consumer that wants structured data must parse the YAML (regular, low-risk) or be re-examined. Trade: the tool now returns exactly what the LLM consumes. +- **YAML parsing in the smoke test is more brittle than JSON.** Mitigated by the grammar being regular and exhaustively specified by `test_awt_snapshot.py`; the smoke-test parser is small. +- **EDT.** `capture()` reads AWT state off the Jetty thread — pre-existing (today's MCP `take_snapshot` already does this via `callEvent`). See `.plans/0003-awt-edt-threading.md`. This RFC does not change threading. +- **`IjImageJ`/`Ij2ConsoleWindow` name special-casing.** Their AWT `getName()` is not "ImageJ"/"ImageJ Console"; the formatter forces these names to match Python. +- **Verbosity.** Phase 1 hardcodes NORMAL. The only in-scope verbosity effects are IjImage's LOW truncation and the generic container's child-hiding at LOW. + +## Files to modify (when implemented) + +| File | Change | +| --------------------------------------------------------------------------------------- | ------------------------------------------------------------------- | +| `plugin/.../awt/SnapshotFormatter.java` (new) | The YAML renderer | +| `plugin/.../mcp/tools/TakeSnapshotTool.java` | `handle()` renders YAML via `SnapshotFormatter`; description tweak | +| `plugin/.../EventHandler.java` | Add `public SnapshotManager getSnapshotManager()` | +| `plugin/pom.xml` + `plugin/src/test/java/copilotj/awt/SnapshotFormatterTest.java` (new) | JUnit 5 + grammar tests | +| `scripts/mcp_smoke_test.py` | Parse YAML; drop resource checks (already done in the prior change) | +| `justfile` | `test-plugin` target | + +No changes to `SnapshotManager`, `Snapshot`, the `*Node` classes, `ComponentNode.describe()`, +the Python side, or the bridge `EventHandler` cases. + +## Verification + +1. **Java unit tests:** `cd plugin && mvn test` — all `SnapshotFormatterTest` cases pass (byte-level grammar, no Fiji). +2. **Build:** `just build-plugin`. +3. **Python regression:** `just test` — `test_awt_snapshot.py` and the rest stay green (untouched). +4. **Manual MCP smoke (live Fiji):** `just dev-plugin` then `python3 scripts/mcp_smoke_test.py`: + - `tools/list` → 9 tools, no resources. + - `take_snapshot` → YAML starting with `Snapshot #`. + - `call_action` stability / round-trip / unknown-action / stale-ref all PASS/SKIP (refs parsed from YAML). + +## Open questions + +- Byte-match vs. good-enough for Phase 1? Recommendation: good-enough (generic renderer); byte-match is Phase 2. +- Should the smoke-test YAML parser live in the script only, or also as a reusable helper elsewhere? Recommendation: script-only for now. +- When Phase 2 lands, move flattening fully to Java and drop Python's (single source of truth), or keep both? Recommendation: keep both; they serve different consumers (Python bridge vs. MCP). + +## Trigger + +Pick this RFC up when any of: + +- An MCP client needs the compact ref-annotated tree and the raw JSON is too verbose for the LLM context window, **or** +- The Java MCP `take_snapshot` output is observed to confuse an LLM (e.g. it fails to find refs in the JSON), **or** +- Byte-parity with the Python snapshot rendering becomes a requirement (e.g. cross-path consistency tests). + +Until then, the raw-JSON `take_snapshot` is correct and the ref loop works; only the +presentation differs from the Python path. diff --git a/copilotj/plugin/__main__.py b/copilotj/plugin/__main__.py index 85f4a140..af10960e 100644 --- a/copilotj/plugin/__main__.py +++ b/copilotj/plugin/__main__.py @@ -64,14 +64,14 @@ async def runner(): @cli.command() -@click.option("--snapshot", type=int, help="ID of the snapshot") -@click.option("--action", type=int, help="Id of the action") +@click.option("--ref", type=str, required=True, help="Component ref handle, e.g. e9") +@click.option("--action", type=str, required=True, help="Action short id, e.g. click") @click.option("--parameters", type=str, default=None, help="JSON-encoded parameter list") @click.pass_context -def call_action(ctx, snapshot, action, parameters): - """Capture the image from ImageJ.""" +def call_action(ctx, ref, action, parameters): + """Run an action on a component identified by its ref handle.""" parsed = json.loads(parameters) if parameters else None - run(lambda a: a.call_action, ctx, snapshot, action, parsed) + run(lambda a: a.call_action, ctx, ref, action, parsed) @cli.command() diff --git a/copilotj/plugin/api.py b/copilotj/plugin/api.py index 9a0a5e4b..ddb1914e 100644 --- a/copilotj/plugin/api.py +++ b/copilotj/plugin/api.py @@ -60,10 +60,8 @@ def __init__(self, inner: "PluginAPI", client_id: uuid.UUID) -> None: self._inner = inner self._client_id = client_id - async def call_action( - self, snapshot_id: int, action_id: int, parameters: list[Any] | None = None - ) -> TypedActionResponse: - return await self._request(ActionRequest(snapshot_id=snapshot_id, action_id=action_id, parameters=parameters)) + async def call_action(self, ref: str, action: str, parameters: list[Any] | None = None) -> TypedActionResponse: + return await self._request(ActionRequest(ref=ref, action=action, parameters=parameters)) async def capture_image(self, title: str | None = None) -> IjImagePreviewWithInfoResponse: """Captures the current image.""" diff --git a/copilotj/plugin/awt/_base.py b/copilotj/plugin/awt/_base.py index 2e2e52e4..f263c968 100644 --- a/copilotj/plugin/awt/_base.py +++ b/copilotj/plugin/awt/_base.py @@ -2,8 +2,8 @@ # # SPDX-License-Identifier: Apache-2.0 -from abc import ABC, abstractmethod -from typing import override +from abc import ABC +from typing import Any, override from copilotj.plugin._base import Response, Verbosity @@ -14,11 +14,74 @@ class ComponentBase[T: str](Response, ABC): type: T name: str | None - @abstractmethod - def _describe_one_line(self) -> str: ... + # Playwright-mcp-style ref handle ("e" + int) assigned on the Java side, or + # None for non-ref-eligible nodes (labels, intermediate containers). + ref: str | None = None + # Per-component action metadata (Java-built list of Action-like objects with + # at least a fully-qualified "type"). Typed loosely to avoid a circular + # import with copilotj.plugin.awt.action; only the short id is read here. + actions: list[Any] | None = None + + # ------------------------------------------------------------------ + # YAML rendering (playwright-mcp-style snapshot format) + # + # Each node renders as a single YAML list item, e.g.: + # - button "Apply" [ref=e10] (click) + # - choice "Method" [ref=e5] (selectItem): selected="Huang" items=[...] + # Containers append a trailing ":" and nest children with 2-space indent. + # ------------------------------------------------------------------ + + def role(self) -> str: + """Lowercase short role name (e.g. 'button', 'window'). Override per subclass.""" + return "component" + + def _node_name(self) -> str | None: + """The quoted "name" segment, or None to omit it.""" + return None + + def _state_inline(self) -> str | None: + """The inline `key=value` tail after the colon, or None to omit.""" + return None + + def _head_extras(self) -> str | None: + """Extra bracketed segments after the ref (e.g. '(id=1)' for windows).""" + return None + + def _action_segment(self) -> str | None: + """The '(actionId,...)' segment listing short action ids, or None.""" + if not self.actions: + return None + ids = [_action_short_id(a) for a in self.actions] + return "(" + ",".join(ids) + ")" + + def _yaml_head(self) -> str: + """The line head: `- role "name" [ref=eN] (extras) (actions)`.""" + parts: list[str] = ["-", self.role()] + name = self._node_name() + if name: + parts.append(f'"{name}"') + if self.ref: + parts.append(f"[ref={self.ref}]") + extras = self._head_extras() + if extras: + parts.append(extras) + actions = self._action_segment() + if actions: + parts.append(actions) + return " ".join(parts) + + def _yaml_line(self) -> str: + """Full leaf line: head, plus `: state` when there is inline state.""" + head = self._yaml_head() + state = self._state_inline() + if state is not None: + return f"{head}: {state}" + return head + + @override def _describe(self, *, level: int, verbosity: Verbosity) -> list[str]: - return [self._describe_one_line()] + return [self._yaml_line()] class ActionResponse[T: str, K: Response | None](Response): @@ -42,3 +105,20 @@ def str_or_empty(value: str | None, *, max_length: int = 300) -> str: return f'"{value[:max_length]}..."' return f'"{value}"' + + +def _action_short_id(action: Any) -> str: + """Short action id: the substring after the last '.' of the action type.""" + action_type = getattr(action, "type", None) + if action_type is None and isinstance(action, dict): + action_type = action.get("type") + return str(action_type).rsplit(".", 1)[-1] + + +def format_items(items: list[str], *, max_items: int = 8) -> str: + """Render option strings as `["a","b",...]`, truncating long lists.""" + shown = items[:max_items] + rendered = ", ".join(str_or_empty(i) for i in shown) + if len(items) > max_items: + rendered += ", ..." + return f"[{rendered}]" diff --git a/copilotj/plugin/awt/action.py b/copilotj/plugin/awt/action.py index 5e59e486..8464022e 100644 --- a/copilotj/plugin/awt/action.py +++ b/copilotj/plugin/awt/action.py @@ -12,6 +12,8 @@ from copilotj.plugin.awt.component.button_node import ButtonClickResponse from copilotj.plugin.awt.component.checkbox_node import CheckboxSetStateResponse from copilotj.plugin.awt.component.choice_node import ChoiceSelectItemResponse +from copilotj.plugin.awt.component.list_node import ListSelectResponse +from copilotj.plugin.awt.component.scrollbar_node import ScrollbarSetValueResponse from copilotj.plugin.awt.component.text_area_node import TextAreaSetTextResponse from copilotj.plugin.awt.component.text_field_node import TextFieldSetTextResponse from copilotj.plugin.awt.window.ij_image import IjImageCaptureResponse @@ -115,6 +117,15 @@ class Action(Response): description: str parameters: list[AnyParameterSchema] + @property + def short_id(self) -> str: + """Short action id: the substring after the last '.' of type. + + e.g. ``java.awt.Button.click`` -> ``click``. This is the identifier a + caller passes to ``call_action(ref, action, params)``. + """ + return self.type.rsplit(".", 1)[-1] + @override def _describe(self, *, level: int, verbosity: Verbosity) -> list[str]: description = self.description or self.name @@ -133,6 +144,8 @@ def _describe(self, *, level: int, verbosity: Verbosity) -> list[str]: ButtonClickResponse | CheckboxSetStateResponse | ChoiceSelectItemResponse + | ListSelectResponse + | ScrollbarSetValueResponse | TextAreaSetTextResponse | TextFieldSetTextResponse | IjImageCaptureResponse diff --git a/copilotj/plugin/awt/component/button_node.py b/copilotj/plugin/awt/component/button_node.py index bfebadc8..4be80202 100644 --- a/copilotj/plugin/awt/component/button_node.py +++ b/copilotj/plugin/awt/component/button_node.py @@ -2,9 +2,9 @@ # # SPDX-License-Identifier: Apache-2.0 -from typing import Literal +from typing import Literal, override -from copilotj.plugin.awt._base import ActionResponse, ComponentBase, str_or_empty +from copilotj.plugin.awt._base import ActionResponse, ComponentBase __all__ = ["ButtonNode", "ButtonClickResponse"] @@ -12,8 +12,13 @@ class ButtonNode(ComponentBase[Literal["java.awt.Button"]]): label: str - def _describe_one_line(self) -> str: - return f"Button: label={str_or_empty(self.label)}" + @override + def role(self) -> str: + return "button" + + @override + def _node_name(self) -> str | None: + return self.label ButtonClickResponse = ActionResponse[Literal["java.awt.Button.click"], None] diff --git a/copilotj/plugin/awt/component/canvas_node.py b/copilotj/plugin/awt/component/canvas_node.py index 3f8d501b..c6f6936a 100644 --- a/copilotj/plugin/awt/component/canvas_node.py +++ b/copilotj/plugin/awt/component/canvas_node.py @@ -2,7 +2,7 @@ # # SPDX-License-Identifier: Apache-2.0 -from typing import Literal +from typing import Literal, override from copilotj.plugin.awt._base import ComponentBase @@ -10,5 +10,6 @@ class CanvasNode(ComponentBase[Literal["java.awt.Canvas"]]): - def _describe_one_line(self) -> str: - return "Canvas" + @override + def role(self) -> str: + return "canvas" diff --git a/copilotj/plugin/awt/component/checkbox_node.py b/copilotj/plugin/awt/component/checkbox_node.py index e2a555af..24820e79 100644 --- a/copilotj/plugin/awt/component/checkbox_node.py +++ b/copilotj/plugin/awt/component/checkbox_node.py @@ -2,9 +2,9 @@ # # SPDX-License-Identifier: Apache-2.0 -from typing import Literal +from typing import Literal, override -from copilotj.plugin.awt._base import ActionResponse, ComponentBase, str_or_empty +from copilotj.plugin.awt._base import ActionResponse, ComponentBase __all__ = ["CheckboxNode", "CheckboxSetStateResponse"] @@ -13,8 +13,17 @@ class CheckboxNode(ComponentBase[Literal["java.awt.Checkbox"]]): label: str | None state: bool - def _describe_one_line(self) -> str: - return f"Checkbox: label={str_or_empty(self.label)}, state={self.state}" + @override + def role(self) -> str: + return "checkbox" + + @override + def _node_name(self) -> str | None: + return self.label + + @override + def _state_inline(self) -> str | None: + return f"checked={str(self.state).lower()}" type CheckboxSetStateResponse = ActionResponse[Literal["java.awt.Checkbox.setState"], None] diff --git a/copilotj/plugin/awt/component/choice_node.py b/copilotj/plugin/awt/component/choice_node.py index 520b53a2..f53d8c92 100644 --- a/copilotj/plugin/awt/component/choice_node.py +++ b/copilotj/plugin/awt/component/choice_node.py @@ -2,9 +2,9 @@ # # SPDX-License-Identifier: Apache-2.0 -from typing import Literal +from typing import Literal, override -from copilotj.plugin.awt._base import ActionResponse, ComponentBase, str_or_empty +from copilotj.plugin.awt._base import ActionResponse, ComponentBase, format_items, str_or_empty __all__ = ["ChoiceNode", "ChoiceSelectItemResponse"] @@ -13,9 +13,13 @@ class ChoiceNode(ComponentBase[Literal["java.awt.Choice"]]): items: list[str] selected_item: str - def _describe_one_line(self) -> str: - items_str = ", ".join([str_or_empty(item) for item in self.items]) - return f"Choice: selected={str_or_empty(self.selected_item)}, items=[{items_str}]" + @override + def role(self) -> str: + return "choice" + + @override + def _state_inline(self) -> str | None: + return f"selected={str_or_empty(self.selected_item)} items={format_items(self.items)}" type ChoiceSelectItemResponse = ActionResponse[Literal["java.awt.Choice.selectItem"], None] diff --git a/copilotj/plugin/awt/component/label_node.py b/copilotj/plugin/awt/component/label_node.py index 0fcf27b8..a1c06425 100644 --- a/copilotj/plugin/awt/component/label_node.py +++ b/copilotj/plugin/awt/component/label_node.py @@ -2,9 +2,9 @@ # # SPDX-License-Identifier: Apache-2.0 -from typing import Literal +from typing import Literal, override -from copilotj.plugin.awt._base import ComponentBase, str_or_empty +from copilotj.plugin.awt._base import ComponentBase __all__ = ["LabelNode"] @@ -12,5 +12,16 @@ class LabelNode(ComponentBase[Literal["java.awt.Label"]]): text: str - def _describe_one_line(self) -> str: - return f"Label: text={str_or_empty(self.text)}" + @override + def role(self) -> str: + return "label" + + @override + def _state_inline(self) -> str | None: + # Raw, unquoted text (sanitized to one line, truncated); renders as + # `- label: {text}`. Labels carry no ref and no actions. + text = self.text if self.text is not None else "<empty>" + text = text.replace("\n", " ").replace("\r", " ").replace("\t", " ") + if len(text) > 300: + text = text[:300] + "..." + return text diff --git a/copilotj/plugin/awt/component/list_node.py b/copilotj/plugin/awt/component/list_node.py index 17139ca4..99432d34 100644 --- a/copilotj/plugin/awt/component/list_node.py +++ b/copilotj/plugin/awt/component/list_node.py @@ -2,17 +2,24 @@ # # SPDX-License-Identifier: Apache-2.0 -from typing import Literal +from typing import Literal, override -from copilotj.plugin.awt._base import ComponentBase, str_or_empty +from copilotj.plugin.awt._base import ActionResponse, ComponentBase, format_items, str_or_empty -__all__ = ["ListNode"] +__all__ = ["ListNode", "ListSelectResponse"] class ListNode(ComponentBase[Literal["java.awt.List"]]): items: list[str] selected_item: str | None # can be null in Java AWT List - def _describe_one_line(self) -> str: - items_str = ", ".join([str_or_empty(item) for item in self.items]) - return f"List: selected={str_or_empty(self.selected_item)}, items=[{items_str}]" + @override + def role(self) -> str: + return "list" + + @override + def _state_inline(self) -> str | None: + return f"selected={str_or_empty(self.selected_item)} items={format_items(self.items)}" + + +ListSelectResponse = ActionResponse[Literal["java.awt.List.select"], None] diff --git a/copilotj/plugin/awt/component/scrollbar_node.py b/copilotj/plugin/awt/component/scrollbar_node.py index 77666fc4..ae8e499e 100644 --- a/copilotj/plugin/awt/component/scrollbar_node.py +++ b/copilotj/plugin/awt/component/scrollbar_node.py @@ -2,16 +2,26 @@ # # SPDX-License-Identifier: Apache-2.0 -from typing import Literal +from typing import Literal, override -from copilotj.plugin.awt._base import ComponentBase +from copilotj.plugin.awt._base import ActionResponse, ComponentBase -__all__ = ["ScrollbarNode"] +__all__ = ["ScrollbarNode", "ScrollbarSetValueResponse"] class ScrollbarNode(ComponentBase[Literal["java.awt.Scrollbar"]]): value: int orientation: Literal["horizontal", "vertical"] + minimum: int = 0 + maximum: int = 0 - def _describe_one_line(self) -> str: - return f"Scrollbar: value={self.value}, orientation={self.orientation}" + @override + def role(self) -> str: + return "scrollbar" + + @override + def _state_inline(self) -> str | None: + return f"value={self.value}" + + +ScrollbarSetValueResponse = ActionResponse[Literal["java.awt.Scrollbar.setValue"], None] diff --git a/copilotj/plugin/awt/component/text_area_node.py b/copilotj/plugin/awt/component/text_area_node.py index aa82a655..d2911698 100644 --- a/copilotj/plugin/awt/component/text_area_node.py +++ b/copilotj/plugin/awt/component/text_area_node.py @@ -2,7 +2,7 @@ # # SPDX-License-Identifier: Apache-2.0 -from typing import Literal +from typing import Literal, override from copilotj.plugin.awt._base import ActionResponse, ComponentBase, str_or_empty @@ -12,8 +12,13 @@ class TextAreaNode(ComponentBase[Literal["java.awt.TextArea"]]): text: str | None # text can be null if component.getText() is null - def _describe_one_line(self) -> str: - return f"TextArea: text={str_or_empty(self.text)}" + @override + def role(self) -> str: + return "textarea" + + @override + def _state_inline(self) -> str | None: + return f"text={str_or_empty(self.text)}" type TextAreaSetTextResponse = ActionResponse[Literal["java.awt.TextArea.setText"], None] diff --git a/copilotj/plugin/awt/component/text_field_node.py b/copilotj/plugin/awt/component/text_field_node.py index 81668b8b..513e67ba 100644 --- a/copilotj/plugin/awt/component/text_field_node.py +++ b/copilotj/plugin/awt/component/text_field_node.py @@ -2,7 +2,7 @@ # # SPDX-License-Identifier: Apache-2.0 -from typing import Literal +from typing import Literal, override from copilotj.plugin.awt._base import ActionResponse, ComponentBase, str_or_empty @@ -12,8 +12,13 @@ class TextFieldNode(ComponentBase[Literal["java.awt.TextField"]]): text: str | None # text can be null if component.getText() is null - def _describe_one_line(self) -> str: - return f"TextField: text={str_or_empty(self.text)}" + @override + def role(self) -> str: + return "textfield" + + @override + def _state_inline(self) -> str | None: + return f"text={str_or_empty(self.text)}" type TextFieldSetTextResponse = ActionResponse[Literal["java.awt.TextField.setText"], None] diff --git a/copilotj/plugin/awt/component/unknown_node.py b/copilotj/plugin/awt/component/unknown_node.py index 933864d1..ca6c8a47 100644 --- a/copilotj/plugin/awt/component/unknown_node.py +++ b/copilotj/plugin/awt/component/unknown_node.py @@ -2,11 +2,18 @@ # # SPDX-License-Identifier: Apache-2.0 -from copilotj.plugin.awt._base import ComponentBase, str_or_empty +from typing import override + +from copilotj.plugin.awt._base import ComponentBase __all__ = ["UnknownNode"] class UnknownNode(ComponentBase[str]): - def _describe_one_line(self) -> str: - return f"Unknown: type={self.type}, name={str_or_empty(self.name)}" + @override + def role(self) -> str: + return "unknown" + + @override + def _node_name(self) -> str | None: + return self.name diff --git a/copilotj/plugin/awt/container/container_node.py b/copilotj/plugin/awt/container/container_node.py index 37e279ab..9af47bdd 100644 --- a/copilotj/plugin/awt/container/container_node.py +++ b/copilotj/plugin/awt/container/container_node.py @@ -30,25 +30,20 @@ class ContainerNodeBase[T: str](ComponentBase[T], ABC): is_container: Literal[True] # Mark this class as a container, used for pydantic validation children: list[TypedComponentNode] | None - def _describe_one_line(self) -> str: - """Provides a single-line description for the container itself.""" - return self.type + @override + def role(self) -> str: + return "container" @override def _describe(self, *, level: int, verbosity: Verbosity) -> list[str]: - """Describe the container and its children in a tree-like structure. - - Generates a multi-line string list representing the container and its children in a tree-like structure. - """ - # Start with the current container's own one-line description. - # The 'name' attribute is inherited from ComponentBase and should be set - # when the node is created if it's available from the Java side. - # Example: description = f"{self._describe_one_line()} (name='{getattr(self, 'name', 'N/A')}')" - description = self._describe_one_line() - lines = [description] - if verbosity < Verbosity.NORMAL: - return lines - + """Render this container and its children as a 2-space-indented YAML tree.""" + has_children = bool(self.children) and verbosity >= Verbosity.NORMAL + if not has_children: + # No children to show: render like a leaf (own head + optional state). + return [self._yaml_line()] + + # Container with children: head + ":" then children indented 2 spaces. + lines = [self._yaml_head() + ":"] lines.extend(self._describe_children(self.children, level=level, verbosity=verbosity)) return lines @@ -56,28 +51,15 @@ def _describe(self, *, level: int, verbosity: Verbosity) -> list[str]: def _describe_children( cls, children: list[TypedComponentNode] | None, *, level: int, verbosity: Verbosity ) -> list[str]: - lines = [] - num_children = len(children) if children else 0 - for i, child in enumerate(children or []): - is_last_child = i == num_children - 1 - - # Determine the prefix for connecting to the child - connector_prefix = "└── " if is_last_child else "├── " - - # Recursively get the description lines for the child - # Pass down the incremented level and the same verbosity - child_description_lines = child._describe(level=level + 1, verbosity=verbosity) - - if child_description_lines: - # Add the first line of the child's description with the connector - lines.append(connector_prefix + child_description_lines[0]) - - # Determine the prefix for subsequent lines of this child's description - # This ensures multi-line descriptions from children are indented correctly - indentation_prefix = " " if is_last_child else "│ " - for child_line in child_description_lines[1:]: - lines.append(indentation_prefix + child_line) + """Render each child's subtree and indent it one level (2 spaces). + Windows override this to flatten/merge children (e.g. merge a Label into + the following widget) before delegating to ``super()``. + """ + lines: list[str] = [] + for child in children or []: + for child_line in child._describe(level=level + 1, verbosity=verbosity): + lines.append(" " + child_line) return lines diff --git a/copilotj/plugin/awt/snapshot.py b/copilotj/plugin/awt/snapshot.py index 589987cc..b67310e5 100644 --- a/copilotj/plugin/awt/snapshot.py +++ b/copilotj/plugin/awt/snapshot.py @@ -7,7 +7,6 @@ from typing import override from copilotj.plugin._base import FromTo, Request, Response, Verbosity -from copilotj.plugin.awt.action import Action from copilotj.plugin.awt.window import TypedWindow, TypedWindowDifference __all__ = ["SnapshotSummary", "TakeSnapshotRequest", "SnapshotDifference"] @@ -17,7 +16,6 @@ class SnapshotSummary(Response): id: int current_image: str | None windows: list[TypedWindow] - actions: list[Action] | None screen_width: int screen_height: int @@ -26,27 +24,17 @@ class SnapshotSummary(Response): @override def _describe(self, *, level: int, verbosity: Verbosity) -> list[str]: - heading = "#" * level - - lines = [] - lines.append(f"{heading} Summary of Snapshot #{self.id}") - lines.append(f"Current image: {self.current_image}") - lines.append(f"Screen size: {self.screen_width}x{self.screen_height}") - lines.append(f"GUI scale: {self.gui_scale}") - - lines.append(f"{heading}# Windows") - for x in self.windows: - for i, line in enumerate(x._describe(level=level + 1, verbosity=verbosity)): - prefix = "- " if i == 0 else " " - lines.append(prefix + line) + lines = [ + f"Snapshot #{self.id} (image: {self.current_image or 'N/A'}, " + f"screen: {self.screen_width}x{self.screen_height}, scale: {self.gui_scale})" + ] if len(self.windows) == 0: - lines.append("No window opened") + lines.append("no window opened") + return lines - # if self.actions is not None and len(self.actions) > 0: - # lines.append(f"{heading}# Actions") - # for i, action in enumerate(self.actions): - # lines.append(f"{i}. {action}") # TODO: improve description + for window in self.windows: + lines.extend(window._describe(level=level + 1, verbosity=verbosity)) return lines @@ -80,7 +68,7 @@ def _describe(self, *, level: int, verbosity: Verbosity) -> list[str]: return lines lines.append("Changes:") - lines.extend("- " + line for line in diff_lines) + lines.extend(" " + line for line in diff_lines) return lines @@ -98,35 +86,31 @@ def any_opened(self) -> bool: @override def _describe(self, *, level: int, verbosity: Verbosity) -> list[str]: - heading = "#" * level + lines = ["Windows difference:"] - lines = [] - lines.append(f"{heading} Windows Difference") - - if not self.any_opened: - lines.append("No window opened") + if not self.any_opened(): + lines.append(" no window opened") return lines groups = ( (self.added, "added", Verbosity.HIGH if verbosity >= Verbosity.NORMAL else verbosity), (self.changed, "changed", verbosity), - (self.removed, "removed", verbosity.LOW if verbosity <= Verbosity.NORMAL else verbosity), - (self.unchanged, "unchanged", verbosity.LOW if verbosity <= Verbosity.NORMAL else verbosity), + (self.removed, "removed", Verbosity.LOW if verbosity <= Verbosity.NORMAL else verbosity), + (self.unchanged, "unchanged", Verbosity.LOW if verbosity <= Verbosity.NORMAL else verbosity), ) for xs, group, verb in groups: if len(xs) == 0: continue - lines.append(f"Following windows was {group}:") + lines.append(f" {group}:") for x in xs: - for i, line in enumerate(x._describe(level=level + 1, verbosity=verb)): - prefix = "- " if i == 0 else " " - lines.append(prefix + line) + for line in x._describe(level=level + 1, verbosity=verb): + lines.append(" " + line) no_actions = "/".join(action for xs, action, _ in groups if len(xs) == 0) if no_actions: - lines.append(f"No windows was {no_actions}.") + lines.append(f" no windows {no_actions}.") return lines @@ -145,10 +129,7 @@ def any_opened(self) -> bool: @override def _describe(self, *, level: int, verbosity: Verbosity) -> list[str]: - heading = "#" * level - - lines = [] - lines.append(f"{heading} Snapshot Difference") + lines = ["Snapshot difference:"] if self.current_image is not None: lines.append(f"Current image changed from {self.current_image.from_} to {self.current_image.to}") diff --git a/copilotj/plugin/awt/window/_componment.py b/copilotj/plugin/awt/window/_componment.py index b6e1b892..9384bab1 100644 --- a/copilotj/plugin/awt/window/_componment.py +++ b/copilotj/plugin/awt/window/_componment.py @@ -2,17 +2,16 @@ # # SPDX-License-Identifier: Apache-2.0 -from typing import cast, override +from typing import override from copilotj.plugin._base import Verbosity -from copilotj.plugin.awt._base import str_or_empty from copilotj.plugin.awt.component.button_node import ButtonNode from copilotj.plugin.awt.component.canvas_node import CanvasNode from copilotj.plugin.awt.component.choice_node import ChoiceNode from copilotj.plugin.awt.component.scrollbar_node import ScrollbarNode from copilotj.plugin.awt.component.text_area_node import TextAreaNode from copilotj.plugin.awt.component.text_field_node import TextFieldNode -from copilotj.plugin.awt.container.container_node import ContainerNodeBase, TypedComponentNode +from copilotj.plugin.awt.container.container_node import ContainerNodeBase __all__ = [ "Buttons", @@ -25,59 +24,57 @@ class Buttons(ContainerNodeBase[str]): + """A transparent group of buttons: renders its children inline (no group head line).""" + def __init__(self, *children: ButtonNode): assert all(isinstance(a, ButtonNode) for a in children), "All children must be ButtonNode instances" super().__init__(type="copilotj.Buttons", name="Buttons", is_container=True, children=list(children)) @override - def _describe_one_line(self) -> str: - buttons = cast(list[ButtonNode], self.children or []) - return f"Buttons: {', '.join([str_or_empty(button.label) for button in buttons])}" - - @override - @classmethod - def _describe_children( - cls, children: list[TypedComponentNode] | None, *, level: int, verbosity: Verbosity - ) -> list[str]: - return [] + def _describe(self, *, level: int, verbosity: Verbosity) -> list[str]: + # Transparent: emit each button's line directly so they appear at the + # parent's indent level (the parent adds the 2-space indent). + lines: list[str] = [] + for child in self.children or []: + lines.extend(child._describe(level=level, verbosity=verbosity)) + return lines class CanvasWithLabel(CanvasNode): label: str @override - def _describe_one_line(self) -> str: - return f"Canvas: label={str_or_empty(self.label)}" + def _node_name(self) -> str | None: + return self.label class ChoiceWithLabel(ChoiceNode): label: str @override - def _describe_one_line(self) -> str: - items_str = ", ".join([str_or_empty(item) for item in self.items]) - return f"Choice: label={str_or_empty(self.label)}, selected={str_or_empty(self.selected_item)}, items=[{items_str}]" + def _node_name(self) -> str | None: + return self.label class ScrollbarWithLabel(ScrollbarNode): label: str @override - def _describe_one_line(self) -> str: - return f"Scrollbar: label={str_or_empty(self.label)}, value={self.value}, orientation={self.orientation}" + def _node_name(self) -> str | None: + return self.label class TextAreaWithLabel(TextAreaNode): label: str @override - def _describe_one_line(self) -> str: - return f"TextArea: label={str_or_empty(self.label)}, text={str_or_empty(self.text)}" + def _node_name(self) -> str | None: + return self.label class TextFieldWithLabel(TextFieldNode): label: str @override - def _describe_one_line(self) -> str: - return f"TextField: label={str_or_empty(self.label)}, text={str_or_empty(self.text)}" + def _node_name(self) -> str | None: + return self.label diff --git a/copilotj/plugin/awt/window/awt_window.py b/copilotj/plugin/awt/window/awt_window.py index b620d8af..9bed00bf 100644 --- a/copilotj/plugin/awt/window/awt_window.py +++ b/copilotj/plugin/awt/window/awt_window.py @@ -14,9 +14,19 @@ class AwtWindowBase[T: str](ContainerNodeBase[T]): id: int @override - def _describe_one_line(self) -> str: - """Provides a single-line description for the AWT window itself.""" - return f"{self.name}: id={self.id}, type={self.type}" + def role(self) -> str: + return "window" + + @override + def _node_name(self) -> str | None: + # Windows usually expose their title via a `title` field (e.g. IjImage, + # IjTextWindow); those override _node_name. Others fall back to the AWT + # component name, which may be None. + return self.name + + @override + def _head_extras(self) -> str | None: + return f"(id={self.id})" class AwtWindowDifferenceBase[T: str](Response): diff --git a/copilotj/plugin/awt/window/ij2_console.py b/copilotj/plugin/awt/window/ij2_console.py index 33b12f6b..cd4783dc 100644 --- a/copilotj/plugin/awt/window/ij2_console.py +++ b/copilotj/plugin/awt/window/ij2_console.py @@ -14,9 +14,9 @@ class Ij2ConsoleWindow(AwtWindowBase[Literal["ij.Console"]]): @override - def _describe_one_line(self) -> str: + def _node_name(self) -> str | None: return "ImageJ Console" @override def _describe(self, *, level: int, verbosity: Verbosity) -> list[str]: - return [self._describe_one_line()] + return [self._yaml_head()] diff --git a/copilotj/plugin/awt/window/ij_contrast_adjuster.py b/copilotj/plugin/awt/window/ij_contrast_adjuster.py index d4cf433a..16ea498b 100644 --- a/copilotj/plugin/awt/window/ij_contrast_adjuster.py +++ b/copilotj/plugin/awt/window/ij_contrast_adjuster.py @@ -14,47 +14,16 @@ class IjContrastAdjuster(AwtWindowBase[Literal["ij.plugin.frame.ContrastAdjuster"]]): - """Window of Image > Adjust > Brightness/Contrast - - Original description: - - ``` - - dialog1: id=1, type=ij.plugin.frame.ContrastAdjuster - ├── Canvas - ├── java.awt.Panel - │ ├── Label: text="0" - │ └── Label: text="255" - ├── Scrollbar: value=0, orientation=horizontal - ├── java.awt.Panel - │ └── Label: text="Minimum" - ├── Scrollbar: value=255, orientation=horizontal - ├── java.awt.Panel - │ └── Label: text="Maximum" - ├── Scrollbar: value=128, orientation=horizontal - ├── java.awt.Panel - │ └── Label: text="Brightness" - ├── Scrollbar: value=128, orientation=horizontal - ├── java.awt.Panel - │ └── Label: text="Contrast" - └── java.awt.Panel - ├── Button: label="Auto" - ├── Button: label="Reset" - ├── Button: label="Set" - └── Button: label="Apply" - ``` - - New description: - - ```console - - dialog1: id=1, type=ij.plugin.frame.ContrastAdjuster - ├── Canvas - ├── Label: text="Min: 0, Max: 255" - ├── Scrollbar: label="Minimum", value=0, orientation=horizontal - ├── Scrollbar: label="Maximum", value=255, orientation=horizontal - ├── Scrollbar: label="Brightness", value=128, orientation=horizontal - ├── Scrollbar: label="Contrast", value=128, orientation=horizontal - └── Buttons: "Auto", "Reset", "Set", "Apply" - ``` + """Window of Image > Adjust > Brightness/Contrast. + + Renders as a playwright-mcp-style YAML window: each widget carries a + ``[ref=eN]`` handle and its short action id inline (e.g. ``(setValue)``). + See ``copilotj/test/plugin/test_awt_snapshot.py`` for the exact grammar. + + The raw AWT tree is flattened: the two min/max labels are merged into one + ``Min/Max`` label, each scrollbar gets a descriptive label (Minimum, Maximum, + Brightness, Contrast) merged from its adjacent panel label, and the action + buttons are inlined as a transparent ``Buttons`` group. """ @override diff --git a/copilotj/plugin/awt/window/ij_image.py b/copilotj/plugin/awt/window/ij_image.py index 861a81f3..4867245c 100644 --- a/copilotj/plugin/awt/window/ij_image.py +++ b/copilotj/plugin/awt/window/ij_image.py @@ -77,17 +77,19 @@ class IjImage(AwtWindowBase[TYPE]): zoom_factor: float roi: RectangleRoi | DescribedRoi | None + @override + def _node_name(self) -> str | None: + return self.title + @override def _describe(self, *, level: int, verbosity: Verbosity) -> list[str]: - lines = [] + lines = [self._yaml_head() + ":"] typee = ( f"{self.bit_depth}-bit {self.image_type}" if f"{self.bit_depth}-bit" not in self.image_type else self.image_type ) - lines.append( - f"Image: {self.title} (id: {self.id}, type: {typee}, size: {self.size}, Path: {self.path or 'N/A'})" - ) + lines.append(f" type: {typee}, size: {self.size}, path: {self.path or 'N/A'}") if verbosity < Verbosity.NORMAL: return lines @@ -98,28 +100,28 @@ def _describe(self, *, level: int, verbosity: Verbosity) -> list[str]: else f"{self.width}x{self.height}x{self.depth} voxels" ) ss, ch, sl, fr = self.stack_size, self.channels, self.slices, self.frames - lines.append(f"Image dimension: {dimension}, stack size: {ss} ({ch} channels, {sl} slices, {fr} frames)") + lines.append(f" dimension: {dimension}, stack: {ss} ({ch} channels, {sl} slices, {fr} frames)") if self.calibrated: lines.append( - f"Width: {self.calibrated_width:.2f} {self.x_unit}, " - f"Height: {self.calibrated_height:.2f} {self.y_unit}, " - f"Depth: {self.calibrated_depth:.2f} {self.z_unit}" + f" calibrated: width={self.calibrated_width:.2f} {self.x_unit}, " + f"height={self.calibrated_height:.2f} {self.y_unit}, " + f"depth={self.calibrated_depth:.2f} {self.z_unit}" ) if self.x_resolution == self.y_resolution and self.x_unit == self.y_unit: - resolution = f"Resolution: {self.x_resolution} pixels per {self.x_unit}" + resolution = f"resolution: {self.x_resolution} pixels per {self.x_unit}" else: rx, ry = self.x_resolution, self.y_resolution - resolution = f"X Resolution: {rx} pixels per {self.x_unit}, Y Resolution: {ry} pixels per {self.y_unit}" + resolution = f"resolution: x={rx} per {self.x_unit}, y={ry} per {self.y_unit}" if self.zoom_factor != 1: - lines.append(f"{resolution}, Zoom factor: {self.zoom_factor:.2f}") + lines.append(f" {resolution}, zoom={self.zoom_factor:.2f}") else: - lines.append(resolution) + lines.append(f" {resolution}") if self.roi: - lines.append(self.roi._describe_one_line(level=level, verbosity=verbosity)) + lines.append(" " + self.roi._describe_one_line(level=level, verbosity=verbosity)) return lines diff --git a/copilotj/plugin/awt/window/ij_imagej.py b/copilotj/plugin/awt/window/ij_imagej.py index 09ff4cc2..ee07f7a8 100644 --- a/copilotj/plugin/awt/window/ij_imagej.py +++ b/copilotj/plugin/awt/window/ij_imagej.py @@ -12,10 +12,11 @@ class IjImageJ(AwtWindowBase[Literal["ij.ImageJ"]]): @override - def _describe_one_line(self) -> str: - """Provides a single-line description for the AWT window itself.""" - return "ImageJ Main Window" + def _node_name(self) -> str | None: + return "ImageJ" @override def _describe(self, *, level: int, verbosity: Verbosity) -> list[str]: - return [self._describe_one_line()] + # The main toolbar's individual buttons aren't useful to enumerate, so + # render just the window header line. + return [self._yaml_head()] diff --git a/copilotj/plugin/awt/window/ij_text_window.py b/copilotj/plugin/awt/window/ij_text_window.py index 2b75a693..10e8aac8 100644 --- a/copilotj/plugin/awt/window/ij_text_window.py +++ b/copilotj/plugin/awt/window/ij_text_window.py @@ -57,19 +57,17 @@ class IjTextWindow(AwtWindowBase[TYPE]): results_table: ResultsTableSummary | None @override - def _describe(self, *, level: int, verbosity: Verbosity) -> list[str]: - if verbosity <= Verbosity.LOW: - if self.results_table is not None: - return [f"Text Window: {self.title} (id: {self.id}, with results table)"] - else: - return [f"Text Window: {self.title} (id: {self.id}, no results table present)"] + def _node_name(self) -> str | None: + return self.title - lines = [f"Text Window: {self.title} (id: {self.id})"] + @override + def _describe(self, *, level: int, verbosity: Verbosity) -> list[str]: + lines = [self._yaml_head() + ":"] if self.results_table is not None: - lines.extend(self.results_table._describe(level=level + 1, verbosity=verbosity)) + for line in self.results_table._describe(level=level + 1, verbosity=verbosity): + lines.append(" " + line) else: - lines.append("No results table present.") - + lines.append(" no results table") return lines diff --git a/copilotj/plugin/awt/window/ij_threshold_adjuster.py b/copilotj/plugin/awt/window/ij_threshold_adjuster.py index da8097b7..f9a3e47b 100644 --- a/copilotj/plugin/awt/window/ij_threshold_adjuster.py +++ b/copilotj/plugin/awt/window/ij_threshold_adjuster.py @@ -16,52 +16,15 @@ class IjThresholdAdjuster(AwtWindowBase[Literal["ij.plugin.frame.ThresholdAdjuster"]]): """Window of Image > Adjust > Threshold... - Original description: - - ``` - - dialog0: id=1, type=ij.plugin.frame.ThresholdAdjuster - ├── Canvas - ├── Label: text="below: 0.00 %, above: 8.71 %" - ├── Scrollbar: value=0, orientation=horizontal - ├── TextField: text="0" - ├── Scrollbar: value=48, orientation=horizontal - ├── TextField: text="48" - ├── java.awt.Panel - │ ├── Choice: selected="Huang", items=["Default", "Huang", "Intermodes", "IsoData", "IJ_IsoData", "Li", - "MaxEntropy", "Mean", "MinError", "Minimum", "Moments", "Otsu", "Percentile", "RenyiEntropy", "Shanbhag", - "Triangle", "Yen"] - │ └── Choice: selected="Over/Under", items=["Red", "B&W", "Over/Under"] - ├── java.awt.Panel - │ ├── Checkbox: label="Dark background", state=False - │ ├── Checkbox: label="Stack histogram", state=False - │ ├── Checkbox: label="Don't reset range", state=False - │ ├── Checkbox: label="Raw values", state=False - │ └── Checkbox: label="16-bit histogram", state=False - └── java.awt.Panel - ├── Button: label="Auto" - ├── Button: label="Apply" - ├── Button: label="Reset" - └── Button: label="Set" - ``` - - New description: - ```console - - dialog1: id=1, type=ij.plugin.frame.ThresholdAdjuster - ├── Canvas: label="Histogram Canvas" - ├── Label: text="99.33 %" - ├── Scrollbar: label="Lower threshold", value=0, orientation=horizontal - ├── Scrollbar: label="Upper threshold", value=98, orientation=horizontal - ├── Choice: label="Method", selected="RenyiEntropy", items=["Default", "Huang", "Intermodes", "IsoData", - "IJ_IsoData", "Li", "MaxEntropy", "Mean", "MinError", "Minimum", "Moments", "Otsu", "Percentile", "RenyiEntropy", - "Shanbhag", "Triangle", "Yen"] - ├── Choice: label="Preview mode", selected="Red", items=["Red", "B&W", "Over/Under"] - ├── Checkbox: label="Dark background", state=False - ├── Checkbox: label="Stack histogram", state=False - ├── Checkbox: label="Don't reset range", state=False - ├── Checkbox: label="Raw values", state=False - ├── Checkbox: label="16-bit histogram", state=False - └── Buttons: "Auto", "Apply", "Reset", "Set" - ```` + Renders as a playwright-mcp-style YAML window: each widget carries a + ``[ref=eN]`` handle and its short action id inline (e.g. ``(selectItem)``). + See ``copilotj/test/plugin/test_awt_snapshot.py`` for the exact grammar. + + The raw AWT tree is flattened for readability: the histogram canvas and each + scrollbar/choice get a descriptive label merged from the adjacent ``Label``, + the redundant text fields mirroring the scrollbars are dropped, the two + ``Choice`` widgets and the checkboxes are pulled out of their panels, and the + four action buttons are inlined as a transparent ``Buttons`` group. """ @override diff --git a/copilotj/plugin/snapshot_manager.py b/copilotj/plugin/snapshot_manager.py index 9c156a31..dd8c12be 100644 --- a/copilotj/plugin/snapshot_manager.py +++ b/copilotj/plugin/snapshot_manager.py @@ -17,6 +17,6 @@ class CompareSnapshotRequest(Request[SnapshotDifference], event="compare_snapsho class ActionRequest(Request[Any], event="run_action", response_type=TypedActionResponse): - snapshot_id: int - action_id: int + ref: str + action: str parameters: list[Any] | None diff --git a/copilotj/test/plugin/__init__.py b/copilotj/test/plugin/__init__.py new file mode 100644 index 00000000..d8d52ffa --- /dev/null +++ b/copilotj/test/plugin/__init__.py @@ -0,0 +1,3 @@ +# SPDX-FileCopyrightText: Copyright contributors to the CopilotJ project. +# +# SPDX-License-Identifier: Apache-2.0 diff --git a/copilotj/test/plugin/test_action_response.py b/copilotj/test/plugin/test_action_response.py new file mode 100644 index 00000000..d595cd59 --- /dev/null +++ b/copilotj/test/plugin/test_action_response.py @@ -0,0 +1,44 @@ +# SPDX-FileCopyrightText: Copyright contributors to the CopilotJ project. +# +# SPDX-License-Identifier: Apache-2.0 + +"""Regression tests for the call_action response contract. + +Locks in the D1 fix from the /review pass: every action response must carry its +fully-qualified action type (e.g. ``java.awt.Button.click``), and the +``TypedActionResponse`` union must accept each one — including the scrollbar and +list branches that were missing before. Short ids (``click``) must be rejected. +""" + +import pytest +from pydantic import TypeAdapter, ValidationError + +from copilotj.plugin.awt.action import TypedActionResponse + +# TypedActionResponse is a `type` union alias, not a BaseModel — validate via TypeAdapter. +_RESPONSE = TypeAdapter(TypedActionResponse) + +# Action types whose response result is None (the simple mutations). The +# image/results-table responses carry non-None results and are exercised elsewhere. +_NONE_RESULT_ACTION_TYPES = [ + "java.awt.Button.click", + "java.awt.Checkbox.setState", + "java.awt.Choice.selectItem", + "java.awt.List.select", + "java.awt.Scrollbar.setValue", + "java.awt.TextArea.setText", + "java.awt.TextField.setText", +] + + +@pytest.mark.parametrize("full_type", _NONE_RESULT_ACTION_TYPES) +def test_typed_action_response_accepts_each_action_type(full_type): + """Each fully-qualified action type must validate against the union.""" + resp = _RESPONSE.validate_python({"type": full_type, "result": None}) + assert resp.type == full_type + + +def test_short_action_id_is_rejected(): + """The contract is fully-qualified types; a bare short id must not validate.""" + with pytest.raises(ValidationError): + _RESPONSE.validate_python({"type": "click", "result": None}) diff --git a/copilotj/test/plugin/test_awt_snapshot.py b/copilotj/test/plugin/test_awt_snapshot.py new file mode 100644 index 00000000..4ba883bb --- /dev/null +++ b/copilotj/test/plugin/test_awt_snapshot.py @@ -0,0 +1,136 @@ +# SPDX-FileCopyrightText: Copyright contributors to the CopilotJ project. +# +# SPDX-License-Identifier: Apache-2.0 + +"""Tests for the playwright-mcp-style AWT snapshot YAML format. + +Covers: per-element ref handles, inline short action ids, the `- role "name" +[ref=eN] (actions): state` line grammar, 2-space indentation of children, +labels carrying no ref, and the transparent `Buttons` group. +""" + +from typing import Literal, override + +from copilotj.plugin._base import Verbosity +from copilotj.plugin.awt.component import ButtonNode, CheckboxNode, ChoiceNode, LabelNode, ScrollbarNode +from copilotj.plugin.awt.window._componment import Buttons +from copilotj.plugin.awt.window.awt_window import AwtWindowBase + +CLICK = [{"type": "java.awt.Button.click", "name": "Click", "description": "d", "parameters": []}] +SELECT = [{"type": "java.awt.Choice.selectItem", "name": "Select", "description": "d", "parameters": []}] +SET_STATE = [{"type": "java.awt.Checkbox.setState", "name": "Set", "description": "d", "parameters": []}] +SET_VALUE = [{"type": "java.awt.Scrollbar.setValue", "name": "Set", "description": "d", "parameters": []}] + + +def _ref(ref: str) -> str: + """Mirror ComponentBase._yaml_head's ref token: f'[ref={ref}]'.""" + return f"[ref={ref}]" + + +class _TestWindow(AwtWindowBase[Literal["test.Window"]]): + """Minimal concrete window for exercising the inherited container rendering.""" + + @override + def _node_name(self) -> str | None: + return "Test" + + +def test_button_line(): + b = ButtonNode(type="java.awt.Button", name="ap", ref="e9", label="Apply", actions=CLICK) + assert b._yaml_line() == f'- button "Apply" {_ref("e9")} (click)' + + +def test_choice_line_with_items(): + c = ChoiceNode( + type="java.awt.Choice", + name=None, + ref="e5", + items=["Default", "Huang", "RenyiEntropy"], + selected_item="RenyiEntropy", + actions=SELECT, + ) + assert c._yaml_line() == ( + f'- choice {_ref("e5")} (selectItem): selected="RenyiEntropy" items=["Default", "Huang", "RenyiEntropy"]' + ) + + +def test_label_has_no_ref_and_raw_text(): + label = LabelNode(type="java.awt.Label", name=None, text="99.33 %") + assert label.ref is None + assert label._yaml_line() == "- label: 99.33 %" + + +def test_checkbox_and_scrollbar_lines(): + ch = CheckboxNode( + type="java.awt.Checkbox", name=None, ref="e7", label="Dark background", state=False, actions=SET_STATE + ) + assert ch._yaml_line() == f'- checkbox "Dark background" {_ref("e7")} (setState): checked=false' + + sb = ScrollbarNode( + type="java.awt.Scrollbar", + name=None, + ref="e3", + value=0, + orientation="horizontal", + minimum=0, + maximum=255, + actions=SET_VALUE, + ) + assert sb._yaml_line() == f"- scrollbar {_ref('e3')} (setValue): value=0" + + +def test_buttons_are_transparent(): + """Buttons emits each button inline at the parent level (no group head line).""" + btns = Buttons( + ButtonNode(type="java.awt.Button", name="a", ref="e9", label="Auto", actions=CLICK), + ButtonNode(type="java.awt.Button", name="p", ref="e10", label="Apply", actions=CLICK), + ) + out = "\n".join(btns._describe(level=1, verbosity=Verbosity.NORMAL)) + assert out == f'- button "Auto" {_ref("e9")} (click)\n- button "Apply" {_ref("e10")} (click)' + + +def test_window_tree_two_space_indent(): + """A window renders a `- window ... (id=N):` header and nests children 2 spaces.""" + win = _TestWindow( + type="test.Window", + name=None, + ref="e1", + id=1, + is_container=True, + children=[ + ScrollbarNode( + type="java.awt.Scrollbar", + name=None, + ref="e3", + value=0, + orientation="horizontal", + minimum=0, + maximum=255, + actions=SET_VALUE, + ), + LabelNode(type="java.awt.Label", name=None, text="99.33 %"), + ButtonNode(type="java.awt.Button", name="a", ref="e9", label="Apply", actions=CLICK), + ], + ) + out = "\n".join(win._describe(level=1, verbosity=Verbosity.NORMAL)) + assert out == "\n".join( + [ + f'- window "Test" {_ref("e1")} (id=1):', + f" - scrollbar {_ref('e3')} (setValue): value=0", + " - label: 99.33 %", + f' - button "Apply" {_ref("e9")} (click)', + ] + ) + + +def test_low_verbosity_hides_children(): + win = _TestWindow( + type="test.Window", + name=None, + ref="e1", + id=1, + is_container=True, + children=[ButtonNode(type="java.awt.Button", name="a", ref="e9", label="Apply", actions=CLICK)], + ) + out = "\n".join(win._describe(level=1, verbosity=Verbosity.LOW)) + assert out == f'- window "Test" {_ref("e1")} (id=1)' diff --git a/copilotj/test/plugin/test_mcp_smoke_helpers.py b/copilotj/test/plugin/test_mcp_smoke_helpers.py new file mode 100644 index 00000000..42f134c0 --- /dev/null +++ b/copilotj/test/plugin/test_mcp_smoke_helpers.py @@ -0,0 +1,131 @@ +# SPDX-FileCopyrightText: Copyright contributors to the CopilotJ project. +# +# SPDX-License-Identifier: Apache-2.0 + +"""Unit tests for the snapshot-parsing helpers in ``scripts/mcp_smoke_test.py``. + +These run under ``just test`` (no live Fiji needed) so the JSON-walking logic — +the part most likely to silently break when the snapshot shape changes — stays +CI-covered. The helpers are loaded straight from the script via ``importlib``. +""" + +import importlib.util +import json +import sys +from pathlib import Path +from types import ModuleType + + +def _load_smoke_module() -> ModuleType: + """Import scripts/mcp_smoke_test.py as a module (its ``__main__`` guard is no-op).""" + root = Path(__file__).resolve() + while root != root.parent: + if (root / "scripts" / "mcp_smoke_test.py").exists(): + break + root = root.parent + spec = importlib.util.spec_from_file_location("mcp_smoke_test", root / "scripts" / "mcp_smoke_test.py") + assert spec is not None and spec.loader is not None + mod = importlib.util.module_from_spec(spec) + # Register before exec so the @dataclass decorator can resolve the module. + sys.modules["mcp_smoke_test"] = mod + spec.loader.exec_module(mod) + return mod + + +_smoke = _load_smoke_module() +_walk_components = _smoke._walk_components +find_action_ref = _smoke.find_action_ref +checkbox_state_by_ref = _smoke.checkbox_state_by_ref + + +def _snapshot_text() -> str: + """A minimal take_snapshot result mirroring the ref-model JSON shape.""" + snap = { + "id": 1, + "timestamp": "2026-06-23T00:00:00", + "current_image": None, + "windows": [ + { + "type": "ij.GenericDialog", + "id": 1, + "ref": "e1", + "name": "ROI Manager", + "children": [ + { + "type": "java.awt.Checkbox", + "name": None, + "ref": "e2", + "label": "Show All", + "state": False, + "actions": [ + { + "type": "java.awt.Checkbox.setState", + "name": "Set State", + "description": "d", + "parameters": [], + } + ], + }, + {"type": "java.awt.Label", "name": None, "ref": None, "text": "Count: 0"}, + { + "type": "java.awt.Button", + "name": "add", + "ref": "e3", + "label": "Add", + "actions": [ + {"type": "java.awt.Button.click", "name": "Click", "description": "d", "parameters": []} + ], + }, + ], + } + ], + "screen_width": 1920, + "screen_height": 1080, + "gui_scale": "1.00", + } + return json.dumps(snap) + + +def test_walk_components_yields_every_node_with_window_context(): + nodes = list(_walk_components(_snapshot_text())) + # window, checkbox, label, button — label included even though it has no ref. + assert [n.get("ref") for n, _ in nodes] == ["e1", "e2", None, "e3"] + window_node = nodes[0][0] + # Every node shares the same containing window. + assert all(w is nodes[0][1] for _, w in nodes) + assert (window_node.get("name")) == "ROI Manager" + + +def test_find_action_ref_by_label_is_deterministic(): + ref, action = find_action_ref(_snapshot_text(), ".setState", label="Show All") + assert (ref, action) == ("e2", "setState") + + +def test_find_action_ref_unscoped_returns_first_match(): + assert find_action_ref(_snapshot_text(), ".setState") == ("e2", "setState") + assert find_action_ref(_snapshot_text(), ".click") == ("e3", "click") + + +def test_find_action_ref_label_miss_returns_none(): + assert find_action_ref(_snapshot_text(), ".setState", label="Nonexistent") == (None, None) + + +def test_find_action_ref_window_title_scopes_via_name_field(): + # The window carries its title in `name`; window_title matches it as a substring. + assert find_action_ref(_snapshot_text(), ".setState", window_title="ROI Manager") == ("e2", "setState") + assert find_action_ref(_snapshot_text(), ".setState", window_title="Other Window") == (None, None) + + +def test_checkbox_state_by_ref(): + snap = _snapshot_text() + assert checkbox_state_by_ref(snap, "e2") is False + assert checkbox_state_by_ref(snap, "e999") is None # unknown ref + assert checkbox_state_by_ref(snap, "e3") is None # button has no `state` field + + +def test_helpers_are_robust_to_bad_input(): + assert find_action_ref("", ".setState") == (None, None) + assert find_action_ref("not valid json", ".setState") == (None, None) + assert list(_walk_components("")) == [] + assert list(_walk_components("not valid json")) == [] + assert checkbox_state_by_ref("", "e2") is None diff --git a/plugin/src/main/java/copilotj/SnapshotManager.java b/plugin/src/main/java/copilotj/SnapshotManager.java index 7cde5339..6c44d315 100644 --- a/plugin/src/main/java/copilotj/SnapshotManager.java +++ b/plugin/src/main/java/copilotj/SnapshotManager.java @@ -13,6 +13,7 @@ import org.scijava.log.LogService; import copilotj.awt.Action; +import copilotj.awt.ComponentIdentifier; import copilotj.awt.Snapshot; import copilotj.awt.WindowIdentifier; @@ -23,8 +24,8 @@ public static class CompareRequest { } public static class ActionRequest { - public int snapshotId; - public int actionId; + public String ref; + public String action; public List<Object> parameters; } @@ -32,6 +33,7 @@ public static class ActionRequest { private final LogService log; private final ImagejListener listener; private final WindowIdentifier identifier; + private final ComponentIdentifier componentIdentifier; private final Queue<Snapshot> snapshots = new ArrayDeque<>(MAX_SNAPSHOTS); private int nextId = 1; @@ -40,6 +42,7 @@ public SnapshotManager(final LogService log, final boolean debug) { this.log = log; this.listener = new ImagejListener(10240, debug); this.identifier = new WindowIdentifier(); + this.componentIdentifier = new ComponentIdentifier(); } // FIXME: remove this method @@ -58,7 +61,7 @@ public Snapshot capture() { private Snapshot capture(final boolean save) { final int id = nextId++; - final Snapshot snapshot = new Snapshot(log, identifier, id); + final Snapshot snapshot = new Snapshot(log, identifier, componentIdentifier, id); if (!save) { return snapshot; } @@ -96,14 +99,11 @@ public Snapshot.Difference compare(final CompareRequest request) { } public Action.Response runAction(final ActionRequest request) { - final Snapshot snapshot = this.get(request.snapshotId); - if (snapshot == null) { - throw new IllegalArgumentException("Snapshot not found"); - } - - // TODO: use old snapshot as reference - final int id = nextId++; - final Snapshot newSnapshot = new Snapshot(log, identifier, id); - return newSnapshot.runAction(request.actionId, request.parameters); + // Actions always run against a freshly captured snapshot: saved snapshots are + // deactivated (their live AWT components are nulled), and refs are stable + // identities, so the same ref resolves to the same component on the fresh + // snapshot. A missing ref yields a clean "not found" error. + final Snapshot fresh = new Snapshot(log, identifier, componentIdentifier, nextId++); + return fresh.runAction(request.ref, request.action, request.parameters); } } diff --git a/plugin/src/main/java/copilotj/awt/Action.java b/plugin/src/main/java/copilotj/awt/Action.java index 5dddffb8..9dbb3b98 100644 --- a/plugin/src/main/java/copilotj/awt/Action.java +++ b/plugin/src/main/java/copilotj/awt/Action.java @@ -9,14 +9,11 @@ import java.util.ArrayList; import java.util.List; -import com.fasterxml.jackson.annotation.JsonIgnore; - public class Action { public static class Builder { private final String type; private final String name; private final String description; - private final List<String> path; private final List<ParameterSchema> parameters; public Builder(final String type, final String name, final String description) { @@ -28,7 +25,6 @@ public Builder(final String type, final String name, final String description) { this.type = type; this.name = name; this.description = description; - this.path = new ArrayList<>(); this.parameters = new ArrayList<>(); } @@ -304,18 +300,22 @@ public static Builder builder(final String type, final String name, final String public String description; public final List<ParameterSchema> parameters; - @JsonIgnore - public final List<String> path; - private Action(final Builder builder) { this.type = builder.type; - this.path = builder.path; this.name = builder.name; this.description = builder.description; this.parameters = builder.parameters; } - public void addPath(final String path) { - this.path.add(path); + /** + * Returns the short, human-friendly action id: the substring after the last '.' + * of {@link #type} (e.g. {@code java.awt.Button.click} -> {@code click}). This + * is the identifier callers pass to {@code call_action(ref, action, params)}. + * + * @return the short action id. + */ + public String shortId() { + final int idx = type.lastIndexOf('.'); + return idx < 0 ? type : type.substring(idx + 1); } } diff --git a/plugin/src/main/java/copilotj/awt/ComponentIdentifier.java b/plugin/src/main/java/copilotj/awt/ComponentIdentifier.java new file mode 100644 index 00000000..564ff7e2 --- /dev/null +++ b/plugin/src/main/java/copilotj/awt/ComponentIdentifier.java @@ -0,0 +1,54 @@ +/** + * SPDX-FileCopyrightText: Copyright contributors to the CopilotJ project. + * + * SPDX-License-Identifier: Apache-2.0 + */ + +package copilotj.awt; + +import java.awt.Component; +import java.util.Map; +import java.util.WeakHashMap; +import java.util.concurrent.atomic.AtomicInteger; + +/** + * Assigns stable, playwright-mcp-style refs ({@code "e" + int}) to AWT + * components, keyed on the live {@link Component} instance. + * + * <p> + * The mapping is held in a {@link WeakHashMap}, so it does not prevent components + * from being garbage collected, and it persists across snapshots as long as the + * component stays alive on screen. The same component always receives the same + * ref; a brand-new component receives the next monotonically increasing ref; a + * component that was closed/GC'd has its ref vanish (callers then get a clean + * "ref not found" error instead of silently targeting a different element). + * </p> + * + * <p> + * This is the component-level sibling of {@link WindowIdentifier}. Both live + * once in {@code SnapshotManager} so their state survives across snapshots. + * </p> + */ +public class ComponentIdentifier { + + private final Map<Component, Integer> componentToRef = new WeakHashMap<>(); + private final AtomicInteger nextRef = new AtomicInteger(0); + + /** + * Gets the stable ref number for the given AWT component, allocating a new one + * on first sight. + * + * @param component The AWT component. Must not be null. + * @return A unique integer ref for the component. + * @throws IllegalArgumentException if the component is null. + */ + public synchronized int getRef(final Component component) { + if (component == null) { + throw new IllegalArgumentException("Component cannot be null"); + } + // computeIfAbsent ensures the ref is generated only once per component and + // that the operation is atomic under the synchronized method. The + // WeakHashMap lets the entry be reclaimed once the component is GC'd. + return componentToRef.computeIfAbsent(component, k -> nextRef.getAndIncrement()); + } +} diff --git a/plugin/src/main/java/copilotj/awt/Snapshot.java b/plugin/src/main/java/copilotj/awt/Snapshot.java index 822e0df6..8e304e92 100644 --- a/plugin/src/main/java/copilotj/awt/Snapshot.java +++ b/plugin/src/main/java/copilotj/awt/Snapshot.java @@ -10,7 +10,7 @@ import java.awt.Window; import java.time.LocalDateTime; import java.util.ArrayList; -import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; @@ -23,6 +23,8 @@ import ij.WindowManager; import ij.gui.ImageWindow; import copilotj.ImagejListener; +import copilotj.awt.component.ComponentNode; +import copilotj.awt.container.ContainerNode; import copilotj.awt.window.AbstractAwtWindow; import copilotj.awt.window.AwtWindow; import copilotj.awt.window.AwtWindowProvider; @@ -119,7 +121,6 @@ public static Difference compare(final Snapshot early, final Snapshot later, fin public final LocalDateTime timestamp; public final String currentImage; // Can be null if no window is active public final List<AwtWindow> windows; - public final List<Action> actions; // The following fields usually not changed, so we don't capture them in the // snapshot. however, we add them as they are useful for LLM. @@ -128,10 +129,17 @@ public static Difference compare(final Snapshot early, final Snapshot later, fin public final String guiScale; private final WindowIdentifier identifier; + private final ComponentIdentifier componentIdentifier; - public Snapshot(final LogService log, final WindowIdentifier identifier, final int id) { + // Ref handle -> node, populated while the AWT components are still live (in the + // constructor, before deactivate()). Used to resolve call_action(ref, ...). + private final Map<String, ComponentNode> refToNode = new HashMap<>(); + + public Snapshot(final LogService log, final WindowIdentifier identifier, + final ComponentIdentifier componentIdentifier, final int id) { this.id = id; this.identifier = identifier; + this.componentIdentifier = componentIdentifier; this.timestamp = LocalDateTime.now(); @@ -155,7 +163,7 @@ public Snapshot(final LogService log, final WindowIdentifier identifier, final i log.debug("Invalid window encountered and skipped: " + e.getMessage()); } } - this.actions = buildActions(); + assignRefs(); final Dimension screen = IJ.getScreenSize(); this.screenWidth = screen.width; @@ -169,51 +177,81 @@ public void deactivate() { } } - public Action.Response runAction(final int actionId, final List<Object> parameters) { - if (actionId < 0 || actionId >= actions.size()) { - throw new IllegalArgumentException("Invalid action ID: " + actionId); + /** + * Walks the window/component tree depth-first and assigns stable ref handles + * (via the session-scoped {@link ComponentIdentifier}) to every ref-eligible + * node, populating {@link #refToNode}. Must run while the AWT components are + * still live (i.e. in the constructor, before {@link #deactivate()}). + */ + private void assignRefs() { + for (final AwtWindow window : windows) { + assignRefs((ComponentNode) window); } + } - final Action action = actions.get(actionId); - if (action == null) { - throw new IllegalArgumentException("Action not found: " + actionId); + private void assignRefs(final ComponentNode node) { + node.assignRef(componentIdentifier); + if (node.getRef() != null) { + refToNode.put(node.getRef(), node); } - - final List<String> path = new ArrayList<>(action.path); - Collections.reverse(path); - - final String windowIdStr = path.remove(path.size() - 1); // remove window id from path - final int targetWindowId = Integer.parseInt(windowIdStr); - AwtWindow window = null; - for (final AwtWindow w : this.windows) { - if (w.getId() == targetWindowId) { - window = w; - break; + if (node instanceof ContainerNode) { + final ContainerNode container = (ContainerNode) node; + final List<ComponentNode> children = container.getChildren(); + if (children != null) { + for (final ComponentNode child : children) { + assignRefs(child); + } } } - if (window == null) { - throw new IllegalArgumentException("Window not found with ID: " + targetWindowId); - } + } - final Object result = window.runAction(path, action.type, parameters); - return new Action.Response(action.type, result); + /** + * Resolves a ref handle to its node in this snapshot, or {@code null} if the + * ref is not present (e.g. the element was closed since the snapshot the + * caller holds). + * + * @param ref the ref handle (e.g. {@code "e5"}). + * @return the node, or {@code null}. + */ + public ComponentNode nodeByRef(final String ref) { + return refToNode.get(ref); } - private List<Action> buildActions() { - final List<Action> actions = new ArrayList<>(); - for (final AwtWindow window : windows) { - // window should not be null here as we populate the list carefully - - final List<Action> windowActions = window.getActions(); - if (windowActions != null && !windowActions.isEmpty()) { - for (final Action action : windowActions) { - action.path.add(String.valueOf(window.getId())); // Add window ID as string - action.description = "Window #" + window.getId() + ": " + action.description; // Add window ID to description - Collections.reverse(action.path); // Path is component -> window; reverse to make it window -> component - actions.add(action); + /** + * Runs an action on the node identified by {@code ref}, dispatching by the + * short action id (e.g. {@code click}, {@code selectItem}). + * + * @param ref the ref handle of the target node. + * @param action the short action id. + * @param parameters the action parameters, or {@code null}. + * @return the action response. + * @throws IllegalArgumentException if the ref is not found in this snapshot. + */ + public Action.Response runAction(final String ref, final String action, final List<Object> parameters) { + final ComponentNode node = refToNode.get(ref); + if (node == null) { + throw new IllegalArgumentException( + "Ref " + ref + " not found in the current snapshot. Try capturing a new snapshot."); + } + final Object result = node.runAction(action, parameters); + return new Action.Response(resolveActionType(node, action), result); + } + + /** + * Resolves the short action id to its fully-qualified action type, falling back + * to the short id if the node exposes no matching action. The response must + * carry the fully-qualified type so clients (e.g. the Python bridge's + * TypedActionResponse union) can discriminate on it. + */ + private static String resolveActionType(final ComponentNode node, final String action) { + final List<Action> actions = node.getActions(); + if (actions != null) { + for (final Action a : actions) { + if (a.shortId().equals(action)) { + return a.type; } } } - return actions; + return action; } } diff --git a/plugin/src/main/java/copilotj/awt/component/AbstractComponentNode.java b/plugin/src/main/java/copilotj/awt/component/AbstractComponentNode.java index 027f1b31..eedeb77e 100644 --- a/plugin/src/main/java/copilotj/awt/component/AbstractComponentNode.java +++ b/plugin/src/main/java/copilotj/awt/component/AbstractComponentNode.java @@ -10,10 +10,27 @@ import java.util.List; import copilotj.awt.Action; +import copilotj.awt.ComponentIdentifier; public abstract class AbstractComponentNode<T extends Component> implements ComponentNode { public final String type; public final String name; // AWT component's name property + + /** + * Playwright-mcp-style ref handle ({@code "e" + int}) for this component, or + * {@code null} when the node is not ref-eligible. Assigned during snapshot + * build via {@link #assignRef(ComponentIdentifier)} and serialized to clients. + */ + public String ref; + + /** + * The actions available on this component, captured at snapshot time (while + * the underlying AWT component is still live). Serialized per-component so the + * client can show each widget's short action ids inline. May be {@code null} + * or empty for descriptive-only nodes. + */ + public List<Action> actions; + protected T component; protected AbstractComponentNode(final String type, final T component) { @@ -33,24 +50,35 @@ public String getType() { } @Override - public boolean isActivate() { - return component != null && component.isShowing(); + public String getRef() { + return ref; } @Override - public void deactivate() { - this.component = null; + public List<Action> getActions() { + return actions; } @Override - public List<Action> getActions() { - return null; + public boolean isRefEligible() { + return true; } @Override - public Object runAction(final List<String> path, final String type, final List<Object> parameters) { - // TODO: change to register manager - throw new UnsupportedOperationException("Action not supported: " + type); + public void assignRef(final ComponentIdentifier identifier) { + if (isRefEligible() && component != null) { + this.ref = "e" + identifier.getRef(component); + } + } + + @Override + public boolean isActivate() { + return component != null && component.isShowing(); + } + + @Override + public void deactivate() { + this.component = null; } @Override diff --git a/plugin/src/main/java/copilotj/awt/component/ButtonNode.java b/plugin/src/main/java/copilotj/awt/component/ButtonNode.java index 432406c1..e386aa24 100644 --- a/plugin/src/main/java/copilotj/awt/component/ButtonNode.java +++ b/plugin/src/main/java/copilotj/awt/component/ButtonNode.java @@ -36,6 +36,8 @@ public ComponentNode tryCreate(final Component component) { public ButtonNode(final Button component) { super(TYPE, component); this.label = component.getLabel() != null ? component.getLabel().trim() : null; + this.actions = Collections.singletonList( + Action.builder(TYPE + ".click", "Click", "Click on the button \"" + this.label + "\"").build()); } @Override @@ -44,31 +46,21 @@ public String describe() { } @Override - public List<Action> getActions() { - final Action click = Action - .builder(TYPE + ".click", "Click", "Click on the button \"" + this.label + "\"") - .build(); - return Collections.singletonList(click); - } - - @Override - public Object runAction(final List<String> path, final String type, final List<Object> parameters) { + public Object runAction(final String action, final List<Object> parameters) { if (!this.isActivate()) { throw new IllegalStateException("Button is not activated"); - } else if (path.size() != 0) { - throw new IllegalArgumentException("Path must be empty for ButtonNode"); } - switch (type) { - case TYPE + ".click": + switch (action) { + case "click": if (parameters != null && parameters.size() != 0) { - throw new IllegalArgumentException("Action 'Click' does not accept any parameters. Found: " + parameters); + throw new IllegalArgumentException("Action 'click' does not accept any parameters. Found: " + parameters); } return click(); default: - throw new IllegalArgumentException("Unknown action type: " + type); + throw new IllegalArgumentException("Unknown action: " + action + " for ButtonNode"); } } diff --git a/plugin/src/main/java/copilotj/awt/component/CheckboxNode.java b/plugin/src/main/java/copilotj/awt/component/CheckboxNode.java index 6cd3e832..a375d379 100644 --- a/plugin/src/main/java/copilotj/awt/component/CheckboxNode.java +++ b/plugin/src/main/java/copilotj/awt/component/CheckboxNode.java @@ -6,8 +6,8 @@ package copilotj.awt.component; -import java.awt.Component; import java.awt.Checkbox; +import java.awt.Component; import java.awt.event.ItemEvent; import java.awt.event.ItemListener; import java.util.Collections; @@ -38,6 +38,10 @@ public CheckboxNode(final Checkbox component) { super(TYPE, component); this.label = component.getLabel() != null ? component.getLabel().trim() : null; this.state = component.getState(); + this.actions = Collections.singletonList(Action + .builder(TYPE + ".setState", "Set State", "Sets the checkbox to a specific state.") + .addBooleanParameter("state", "The desired state (true for checked, false for unchecked)") + .build()); } @Override @@ -46,24 +50,13 @@ public String describe() { } @Override - public List<Action> getActions() { - final Action setStateAction = Action - .builder(TYPE + ".setState", "Set State", "Sets the checkbox to a specific state.") - .addBooleanParameter("state", "The desired state (true for checked, false for unchecked)") - .build(); - return Collections.singletonList(setStateAction); - } - - @Override - public Object runAction(final List<String> path, final String type, final List<Object> parameters) { + public Object runAction(final String action, final List<Object> parameters) { if (!this.isActivate()) { throw new IllegalStateException("Checkbox is not activated"); - } else if (path.size() != 0) { - throw new IllegalArgumentException("Path must be empty for CheckboxNode actions like 'setState'"); } - switch (type) { - case TYPE + ".setState": + switch (action) { + case "setState": if (parameters == null || parameters.size() != 1) { throw new IllegalArgumentException( "Action 'setState' requires exactly one boolean 'state' parameter. Found: " + @@ -80,13 +73,13 @@ public Object runAction(final List<String> path, final String type, final List<O return setState((Boolean) param); default: - throw new IllegalArgumentException("Unknown action type: " + type + " for CheckboxNode"); + throw new IllegalArgumentException("Unknown action: " + action + " for CheckboxNode"); } } /** * Sets the state of the checkbox component and notifies listeners. - * + * * @param desiredState The desired state (true for checked, false for * unchecked). * @return null diff --git a/plugin/src/main/java/copilotj/awt/component/ChoiceNode.java b/plugin/src/main/java/copilotj/awt/component/ChoiceNode.java index ca2fb31b..6a6ca90b 100644 --- a/plugin/src/main/java/copilotj/awt/component/ChoiceNode.java +++ b/plugin/src/main/java/copilotj/awt/component/ChoiceNode.java @@ -6,13 +6,13 @@ package copilotj.awt.component; -import java.awt.Component; import java.awt.Choice; +import java.awt.Component; import java.awt.event.ItemEvent; import java.awt.event.ItemListener; +import java.util.Arrays; import java.util.Collections; import java.util.List; -import java.util.Arrays; import copilotj.awt.Action; @@ -43,6 +43,10 @@ public ChoiceNode(final Choice component) { for (int i = 0; i < component.getItemCount(); i++) { this.items[i] = component.getItem(i); } + this.actions = Collections.singletonList(Action + .builder(TYPE + ".selectItem", "Select Item", "Selects an item in the choice menu.") + .addStringParameter("item", "The item to select", Arrays.asList(this.items)) + .build()); } @Override @@ -55,24 +59,13 @@ public String describe() { } @Override - public List<Action> getActions() { - final Action selectItemAction = Action - .builder(TYPE + ".selectItem", "Select Item", "Selects an item in the choice menu.") - .addStringParameter("item", "The item to select", Arrays.asList(this.items)) - .build(); - return Collections.singletonList(selectItemAction); - } - - @Override - public Object runAction(final List<String> path, final String type, final List<Object> parameters) { + public Object runAction(final String action, final List<Object> parameters) { if (!this.isActivate()) { throw new IllegalStateException("Choice is not activated"); - } else if (path.size() != 0) { - throw new IllegalArgumentException("Path must be empty for ChoiceNode actions like 'selectItem'"); } - switch (type) { - case TYPE + ".selectItem": + switch (action) { + case "selectItem": if (parameters == null || parameters.size() != 1) { throw new IllegalArgumentException( "Action 'selectItem' requires exactly one string 'item' parameter. Found: " + @@ -83,19 +76,19 @@ public Object runAction(final List<String> path, final String type, final List<O if (!(param instanceof String)) { throw new IllegalArgumentException( "Action 'selectItem' requires a string 'item' parameter, but got " + - param.getClass().getSimpleName()); + (param != null ? param.getClass().getSimpleName() : "null")); } return selectItem((String) param); default: - throw new IllegalArgumentException("Unknown action type: " + type + " for ChoiceNode"); + throw new IllegalArgumentException("Unknown action: " + action + " for ChoiceNode"); } } /** * Selects an item in the choice component and notifies listeners. - * + * * @param item The item to select. * @return null */ diff --git a/plugin/src/main/java/copilotj/awt/component/ComponentNode.java b/plugin/src/main/java/copilotj/awt/component/ComponentNode.java index 5176ab71..ba180a12 100644 --- a/plugin/src/main/java/copilotj/awt/component/ComponentNode.java +++ b/plugin/src/main/java/copilotj/awt/component/ComponentNode.java @@ -11,6 +11,7 @@ import com.fasterxml.jackson.annotation.JsonIgnore; import copilotj.awt.Action; +import copilotj.awt.ComponentIdentifier; /** * @see https://docs.oracle.com/javase/8/docs/api/java/awt/Component.html @@ -25,19 +26,64 @@ public interface WithLabel extends ComponentNode { public String getType(); + /** + * Returns the actions available on this component (captured at snapshot time), + * or {@code null}/empty for descriptive-only nodes. Used to resolve a short + * action id to its fully-qualified type when building an action response. + * + * @return the actions list, or {@code null}. + */ + public List<Action> getActions(); + + /** + * Returns the playwright-mcp-style ref handle ({@code "e" + int}) assigned to + * this component, or {@code null} if this node is not ref-eligible (e.g. + * labels and intermediate containers). + * + * @return the ref handle, or {@code null}. + */ + public String getRef(); + + /** + * Returns whether this node should receive a ref handle. Ref-eligible nodes + * are windows and actionable leaf components; descriptive-only nodes (labels) + * and intermediate non-window containers opt out. + * + * @return {@code true} if this node should be assigned a ref. + */ @JsonIgnore - public boolean isActivate(); + public boolean isRefEligible(); - public void deactivate(); + /** + * Assigns (or re-affirms) the stable ref for this component using the + * session-scoped {@link ComponentIdentifier}. No-op for non-ref-eligible + * nodes. + * + * @param identifier the session-scoped component identifier. + */ + public void assignRef(final ComponentIdentifier identifier); @JsonIgnore - public List<Action> getActions(); + public boolean isActivate(); - public Object runAction(final List<String> path, final String type, final List<Object> parameters); + public void deactivate(); + + /** + * Runs an action on this node, addressed by its short id (e.g. {@code click}, + * {@code selectItem}). The node owns the dispatch for its own action set; the + * caller resolves a ref to this node first, so no path navigation is needed. + * + * @param action the short action id (see {@code Action.shortId()}). + * @param parameters the action parameters, or {@code null}. + * @return the action result (may be {@code null}). + */ + public default Object runAction(final String action, final List<Object> parameters) { + throw new UnsupportedOperationException("Action not supported: " + action); + } /** * Returns a concise, single-line description of the component node. - * + * * @return A string description. */ public abstract String describe(); diff --git a/plugin/src/main/java/copilotj/awt/component/LabelNode.java b/plugin/src/main/java/copilotj/awt/component/LabelNode.java index f2698003..fedd6d2f 100644 --- a/plugin/src/main/java/copilotj/awt/component/LabelNode.java +++ b/plugin/src/main/java/copilotj/awt/component/LabelNode.java @@ -27,6 +27,14 @@ public LabelNode(final Label component) { this.text = component.getText() != null ? component.getText().trim() : null; } + /** + * Labels are purely descriptive and never receive a ref handle. + */ + @Override + public boolean isRefEligible() { + return false; + } + @Override public String describe() { return "Label: text=" + text; diff --git a/plugin/src/main/java/copilotj/awt/component/ListNode.java b/plugin/src/main/java/copilotj/awt/component/ListNode.java index 7fe5da24..0b3af297 100644 --- a/plugin/src/main/java/copilotj/awt/component/ListNode.java +++ b/plugin/src/main/java/copilotj/awt/component/ListNode.java @@ -40,6 +40,12 @@ public ListNode(final List component) { super(TYPE, component); this.items = component.getItems(); this.selectedItem = component.getSelectedItem(); + if (component.isEnabled()) { // AWT List doesn't have isEditable + this.actions = Collections.singletonList(Action + .builder(this.type + ".select", "Select Item", "Selects an item in the list.") + .addStringParameter("item", "The item to select", Arrays.asList(this.items)) + .build()); + } } @Override @@ -52,29 +58,13 @@ public String describe() { } @Override - public java.util.List<Action> getActions() { - if (!this.component.isEnabled()) { // AWT List doesn't have isEditable - return null; - } - - final Action selectAction = Action - .builder(this.type + ".select", "Select Item", "Selects an item in the list.") - .addStringParameter("item", "The item to select", Arrays.asList(this.items)) - .build(); - return Collections.singletonList(selectAction); - } - - @Override - public Object runAction(final java.util.List<String> path, final String type, - final java.util.List<Object> parameters) { + public Object runAction(final String action, final java.util.List<Object> parameters) { if (!this.isActivate()) { throw new IllegalStateException("List is not activated"); - } else if (path.size() != 0) { - throw new IllegalArgumentException("Path must be empty for ListNode actions like 'select'"); } - switch (type) { - case TYPE + ".select": + switch (action) { + case "select": if (parameters == null || parameters.size() != 1) { throw new IllegalArgumentException( "Action 'select' requires exactly one string 'item' parameter. Found: " + @@ -85,12 +75,12 @@ public Object runAction(final java.util.List<String> path, final String type, if (!(param instanceof String)) { throw new IllegalArgumentException( "Action 'select' requires a string 'item' parameter, but got " + - param.getClass().getSimpleName()); + (param != null ? param.getClass().getSimpleName() : "null")); } return selectItem((String) param); default: - throw new IllegalArgumentException("Unknown action type: " + type + " for ListNode"); + throw new IllegalArgumentException("Unknown action: " + action + " for ListNode"); } } diff --git a/plugin/src/main/java/copilotj/awt/component/ScrollbarNode.java b/plugin/src/main/java/copilotj/awt/component/ScrollbarNode.java index 0258a31f..abdc93fd 100644 --- a/plugin/src/main/java/copilotj/awt/component/ScrollbarNode.java +++ b/plugin/src/main/java/copilotj/awt/component/ScrollbarNode.java @@ -8,6 +8,12 @@ import java.awt.Component; import java.awt.Scrollbar; +import java.awt.event.AdjustmentEvent; +import java.awt.event.AdjustmentListener; +import java.util.Collections; +import java.util.List; + +import copilotj.awt.Action; public class ScrollbarNode extends AbstractComponentNode<Scrollbar> { public static class Provider implements ComponentNodeProvider { @@ -20,13 +26,27 @@ public ComponentNode tryCreate(final Component component) { } } + private static final String TYPE = "java.awt.Scrollbar"; + public final int value; + public final int minimum; + public final int maximum; + public final int visibleAmount; private final int orientation; // e.g., Scrollbar.VERTICAL or Scrollbar.HORIZONTAL public ScrollbarNode(final Scrollbar component) { - super("java.awt.Scrollbar", component); + super(TYPE, component); this.value = component.getValue(); + this.minimum = component.getMinimum(); + this.maximum = component.getMaximum(); + this.visibleAmount = component.getVisibleAmount(); this.orientation = component.getOrientation(); + if (component.isEnabled()) { + this.actions = Collections.singletonList(Action + .builder(TYPE + ".setValue", "Set Value", "Sets the scrollbar value.") + .addIntegerParameter("value", "The new value", this.minimum, this.maximum) + .build()); + } } public String getOrientation() { @@ -46,4 +66,61 @@ public String getOrientation() { public String describe() { return "Scrollbar: value=" + value + ", orientation=" + this.getOrientation(); } + + @Override + public Object runAction(final String action, final List<Object> parameters) { + if (!this.isActivate()) { + throw new IllegalStateException("Scrollbar is not activated"); + } + + switch (action) { + case "setValue": + if (parameters == null || parameters.size() != 1) { + throw new IllegalArgumentException( + "Action 'setValue' requires exactly one integer 'value' parameter. Found: " + + (parameters == null ? 0 : parameters.size()) + " parameters."); + } + + final Object param = parameters.get(0); + if (!(param instanceof Integer)) { + throw new IllegalArgumentException( + "Action 'setValue' requires an integer 'value' parameter, but got " + + (param != null ? param.getClass().getSimpleName() : "null")); + } + + return setValue((Integer) param); + + default: + throw new IllegalArgumentException("Unknown action: " + action + " for ScrollbarNode"); + } + } + + /** + * Sets the value of the scrollbar component and notifies adjustment listeners. + * + * @param value The new value (clamped to the scrollbar's range by AWT). + * @return null + */ + public Object setValue(final int value) { + if (!component.isEnabled()) { + throw new IllegalStateException("Scrollbar is not enabled"); + } + + this.component.setValue(value); + + // Notify listeners. AWT Scrollbar fires an AdjustmentEvent when its value + // changes; we mimic a user-driven (TRACK) adjustment so ImageJ listeners + // (e.g. the Brightness/Contrast or Threshold adjusters) react. + final AdjustmentEvent event = new AdjustmentEvent( + this.component, + AdjustmentEvent.ADJUSTMENT_VALUE_CHANGED, + AdjustmentEvent.TRACK, + this.component.getValue()); + + for (final AdjustmentListener listener : this.component.getAdjustmentListeners()) { + listener.adjustmentValueChanged(event); + } + + return null; + } } diff --git a/plugin/src/main/java/copilotj/awt/component/TextAreaNode.java b/plugin/src/main/java/copilotj/awt/component/TextAreaNode.java index 1118491f..c22e3468 100644 --- a/plugin/src/main/java/copilotj/awt/component/TextAreaNode.java +++ b/plugin/src/main/java/copilotj/awt/component/TextAreaNode.java @@ -33,6 +33,12 @@ public ComponentNode tryCreate(final Component component) { public TextAreaNode(final TextArea component) { super(TYPE, component); this.text = component.getText() != null ? component.getText().trim() : null; + if (component.isEditable()) { + this.actions = Collections.singletonList(Action + .builder(TYPE + ".setText", "Set Text", "Sets the text of the text area.") + .addStringParameter("text", "The text to set") + .build()); + } } @Override @@ -43,28 +49,13 @@ public String describe() { } @Override - public List<Action> getActions() { - if (!this.component.isEditable()) { - return null; - } - - final Action setTextAction = Action - .builder(TYPE + ".setText", "Set Text", "Sets the text of the text area.") - .addStringParameter("text", "The text to set") - .build(); - return Collections.singletonList(setTextAction); - } - - @Override - public Object runAction(final List<String> path, final String type, final List<Object> parameters) { + public Object runAction(final String action, final List<Object> parameters) { if (!this.isActivate()) { throw new IllegalStateException("TextArea is not activated"); - } else if (path.size() != 0) { - throw new IllegalArgumentException("Path must be empty for TextAreaNode actions like 'setText'"); } - switch (type) { - case TYPE + ".setText": + switch (action) { + case "setText": if (parameters == null || parameters.size() != 1) { throw new IllegalArgumentException( "Action 'setText' requires exactly one string 'text' parameter. Found: " + @@ -83,13 +74,13 @@ public Object runAction(final List<String> path, final String type, final List<O return setText((String) param); default: - throw new IllegalArgumentException("Unknown action type: " + type + " for TextAreaNode"); + throw new IllegalArgumentException("Unknown action: " + action + " for TextAreaNode"); } } /** * Sets the text of the text area component and notifies listeners. - * + * * @param text The text to set. * @return null */ diff --git a/plugin/src/main/java/copilotj/awt/component/TextFieldNode.java b/plugin/src/main/java/copilotj/awt/component/TextFieldNode.java index 025a15ee..1804201a 100644 --- a/plugin/src/main/java/copilotj/awt/component/TextFieldNode.java +++ b/plugin/src/main/java/copilotj/awt/component/TextFieldNode.java @@ -33,6 +33,12 @@ public ComponentNode tryCreate(final Component component) { public TextFieldNode(final TextField component) { super(TYPE, component); this.text = component.getText() != null ? component.getText().trim() : null; + if (component.isEditable()) { + this.actions = Collections.singletonList(Action + .builder(TYPE + ".setText", "Set Text", "Sets the text of the text field.") + .addStringParameter("text", "The text to set") + .build()); + } } @Override @@ -41,27 +47,13 @@ public String describe() { } @Override - public List<Action> getActions() { - if (this.component.isEditable()) { - final Action setTextAction = Action - .builder(TYPE + ".setText", "Set Text", "Sets the text of the text field.") - .addStringParameter("text", "The text to set") - .build(); - return Collections.singletonList(setTextAction); - } - return Collections.emptyList(); - } - - @Override - public Object runAction(final List<String> path, final String type, final List<Object> parameters) { + public Object runAction(final String action, final List<Object> parameters) { if (!this.isActivate()) { throw new IllegalStateException("TextField is not activated"); - } else if (path.size() != 0) { - throw new IllegalArgumentException("Path must be empty for TextFieldNode actions like 'setText'"); } - switch (type) { - case TYPE + ".setText": + switch (action) { + case "setText": if (parameters == null || parameters.size() != 1) { throw new IllegalArgumentException( "Action 'setText' requires exactly one string 'text' parameter. Found: " + @@ -80,13 +72,13 @@ public Object runAction(final List<String> path, final String type, final List<O return setText((String) param); default: - throw new IllegalArgumentException("Unknown action type: " + type + " for TextFieldNode"); + throw new IllegalArgumentException("Unknown action: " + action + " for TextFieldNode"); } } /** * Sets the text of the text field component and notifies listeners. - * + * * @param text The text to set. * @return null */ diff --git a/plugin/src/main/java/copilotj/awt/container/AbstractContainerNode.java b/plugin/src/main/java/copilotj/awt/container/AbstractContainerNode.java index ffc3fe29..b63a85f8 100644 --- a/plugin/src/main/java/copilotj/awt/container/AbstractContainerNode.java +++ b/plugin/src/main/java/copilotj/awt/container/AbstractContainerNode.java @@ -10,10 +10,7 @@ import java.awt.Container; import java.util.ArrayList; import java.util.List; -import java.util.regex.Matcher; -import java.util.regex.Pattern; -import copilotj.awt.Action; import copilotj.awt.component.AbstractComponentNode; import copilotj.awt.component.ComponentNode; import copilotj.awt.component.ComponentNodeProvider; @@ -42,6 +39,16 @@ public AbstractContainerNode(final String type, final T component) { this.children = children.size() > 0 ? children : null; } + /** + * Intermediate (non-window) containers are not ref-eligible: they are + * recursed into during ref numbering but never receive a ref handle + * themselves, since actions are addressed to their leaf children. + */ + @Override + public boolean isRefEligible() { + return false; + } + @Override public boolean isActivate() { return super.isActivate() && (children == null || children.stream().allMatch(ComponentNode::isActivate)); @@ -55,66 +62,6 @@ public void deactivate() { } } - @Override - public List<Action> getActions() { - if (children == null || children.size() == 0) { - return null; - } - - final List<Action> actions = new ArrayList<>(); - for (int i = 0; i < children.size(); i++) { - final ComponentNode child = children.get(i); - if (child == null) { - throw new IllegalStateException("Child node is null at index " + i); - } - - final List<Action> childActions = child.getActions(); - if (childActions != null && childActions.size() > 0) { - for (final Action action : childActions) { - action.path.add("children[" + i + "]"); - actions.add(action); - } - } - } - - if (actions.size() == 0) { - return null; - } - return actions; - } - - @Override - public Object runAction(final List<String> path, final String type, final List<Object> parameters) { - if (!this.isActivate()) { - throw new IllegalStateException("Button is not activated"); - } else if (children == null || children.size() == 0) { - throw new IllegalStateException("No children to run action on"); - } else if (path.size() < 1) { - throw new IllegalArgumentException("Path must contain at least 1 element"); - } - - // Extract the index from the path - final String indexString = path.remove(path.size() - 1); - final Pattern pattern = Pattern.compile("children\\[(\\d+)\\]", Pattern.CASE_INSENSITIVE); - final Matcher matcher = pattern.matcher(indexString); - if (!matcher.find()) { - throw new IllegalArgumentException("Invalid path: " + indexString); - } - - final int index; - try { - index = Integer.parseInt(matcher.group(1)); - } catch (final NumberFormatException e) { - throw new IllegalArgumentException("Invalid index in path: " + indexString, e); - } - - if (index < 0 || index >= children.size()) { - throw new IndexOutOfBoundsException("Index out of bounds: " + index); - } - - return children.get(index).runAction(path, type, parameters); - } - @Override public String describe() { return "Container"; // Generic container description, could be improved if needed diff --git a/plugin/src/main/java/copilotj/awt/window/AbstractAwtWindow.java b/plugin/src/main/java/copilotj/awt/window/AbstractAwtWindow.java index 22de4734..98192a09 100644 --- a/plugin/src/main/java/copilotj/awt/window/AbstractAwtWindow.java +++ b/plugin/src/main/java/copilotj/awt/window/AbstractAwtWindow.java @@ -27,6 +27,17 @@ public int getId() { return id; } + /** + * Windows are ref-eligible even though they extend + * {@link copilotj.awt.container.AbstractContainerNode} (which opts out): a + * window can carry window-level actions (e.g. capture) and is addressable by + * ref like any other element. + */ + @Override + public boolean isRefEligible() { + return true; + } + @Override public String getType() { return type; diff --git a/plugin/src/main/java/copilotj/awt/window/AwtWindow.java b/plugin/src/main/java/copilotj/awt/window/AwtWindow.java index b26368ea..34ea342f 100644 --- a/plugin/src/main/java/copilotj/awt/window/AwtWindow.java +++ b/plugin/src/main/java/copilotj/awt/window/AwtWindow.java @@ -6,10 +6,7 @@ package copilotj.awt.window; -import java.util.List; - import copilotj.ImagejListener; -import copilotj.awt.Action; public interface AwtWindow { public static class Difference { @@ -36,9 +33,5 @@ public Difference(final AwtWindow from, final AwtWindow to) { public Difference compare(final AwtWindow from, final ImagejListener.HistoryResponse history); - public List<Action> getActions(); - public void deactivate(); - - public Object runAction(final List<String> path, final String type, final List<Object> parameters); } diff --git a/plugin/src/main/java/copilotj/awt/window/IjImage.java b/plugin/src/main/java/copilotj/awt/window/IjImage.java index 4077b69e..fcdc395c 100644 --- a/plugin/src/main/java/copilotj/awt/window/IjImage.java +++ b/plugin/src/main/java/copilotj/awt/window/IjImage.java @@ -305,6 +305,8 @@ public static ImagePreviewWithInfo captureImage(final WindowIdentifier identifie public IjImage(final WindowIdentifier identifier, final ImageWindow w) { super(identifier, w, TYPE); + this.actions = Collections.singletonList( + Action.builder(TYPE + ".capture", "Capture", "Capture image in current window").build()); final ImagePlus imp = w.getImagePlus(); // this.id = imp.getID(); // NOTE: we dont use Ij window id @@ -409,31 +411,21 @@ public AwtWindow.Difference compare(final AwtWindow from, final ImagejListener.H } @Override - public List<Action> getActions() { - final Action captureImage = Action - .builder(TYPE + ".capture", "Capture", "Capture image in current window") - .build(); - return Collections.singletonList(captureImage); - } - - @Override - public Object runAction(final List<String> path, final String type, final List<Object> parameters) { + public Object runAction(final String action, final List<Object> parameters) { if (!this.isActivate()) { throw new IllegalStateException("Window is not active"); - } else if (path.size() != 0) { - throw new IllegalArgumentException("Path must be empty for IjImage"); } - switch (type) { - case TYPE + ".capture": + switch (action) { + case "capture": if (parameters != null && parameters.size() != 0) { - throw new IllegalArgumentException("Action 'Capture' does not accept any parameters. Found: " + parameters); + throw new IllegalArgumentException("Action 'capture' does not accept any parameters. Found: " + parameters); } return captureImage(this.identifier, this.component.getImagePlus()); default: - return super.runAction(path, type, parameters); + throw new IllegalArgumentException("Unknown action: " + action + " for IjImage"); } } diff --git a/plugin/src/main/java/copilotj/awt/window/IjTextWindow.java b/plugin/src/main/java/copilotj/awt/window/IjTextWindow.java index 7344ee45..4337caca 100644 --- a/plugin/src/main/java/copilotj/awt/window/IjTextWindow.java +++ b/plugin/src/main/java/copilotj/awt/window/IjTextWindow.java @@ -156,6 +156,12 @@ public IjTextWindow(final WindowIdentifier identifier, final TextWindow w) { final ResultsTable t = w.getResultsTable(); // Can be null this.resultsTable = t != null ? new ResultsTableSummary(t) : null; + + this.actions = Collections.singletonList(Action + .builder(TYPE + ".getResultsTable", "Get Results Table", "Get detail rows of the results table") + .addIntegerParameter("offset", "Offset of the rows") // TODO: default to 0 + .addIntegerParameter("limit", "Limit of the rows") + .build()); } @Override @@ -183,49 +189,37 @@ public AwtWindow.Difference compare(final AwtWindow from, final ImagejListener.H } @Override - public List<Action> getActions() { - final Action getResultsTable = Action - .builder(TYPE + ".getResultsTable", "Get Results Table", "Get detail rows of the results table") - .addIntegerParameter("offset", "Offset of the rows") // TODO: default to 0 - .addIntegerParameter("limit", "Limit of the rows") - .build(); - return Collections.singletonList(getResultsTable); - } - - @Override - public Object runAction(final List<String> path, final String type, final List<Object> parameters) { + public Object runAction(final String action, final List<Object> parameters) { if (!this.isActivate()) { throw new IllegalStateException("Window is not active"); - } else if (path.size() != 0) { - throw new IllegalArgumentException("Path must be empty for IjImage"); } - switch (type) { - case TYPE + ".get_results_table": + switch (action) { + case "getResultsTable": if (parameters == null || parameters.size() != 2) { throw new IllegalArgumentException( - "Action 'Get Results Table' requires exactly 2 parameters (offset, limit). Found: " + "Action 'getResultsTable' requires exactly 2 parameters (offset, limit). Found: " + (parameters != null ? parameters.size() : "null")); } final Object offset = parameters.get(0); if (!(offset instanceof Integer)) { throw new IllegalArgumentException( - "Action 'Get Results Table' requires an int 'offset' parameter, but got " + + "Action 'getResultsTable' requires an int 'offset' parameter, but got " + (offset != null ? offset.getClass().getSimpleName() : "null")); } final Object limit = parameters.get(1); if (!(limit instanceof Integer)) { throw new IllegalArgumentException( - "Action 'Get Results Table' requires an int 'limit' parameter, but got " + + "Action 'getResultsTable' requires an int 'limit' parameter, but got " + (limit != null ? limit.getClass().getSimpleName() : "null")); } return getResultsTable((Integer) offset, (Integer) limit); default: - return super.runAction(path, type, parameters); + throw new IllegalArgumentException("Unknown action: " + action + " for IjTextWindow"); } } diff --git a/plugin/src/main/java/copilotj/mcp/McpModule.java b/plugin/src/main/java/copilotj/mcp/McpModule.java index 0af63045..d293bbaa 100644 --- a/plugin/src/main/java/copilotj/mcp/McpModule.java +++ b/plugin/src/main/java/copilotj/mcp/McpModule.java @@ -24,8 +24,6 @@ import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule; import copilotj.EventHandler; -import copilotj.mcp.resources.EnvironmentResource; -import copilotj.mcp.resources.WindowsResource; import copilotj.mcp.prompts.AnalyzeBioimagePrompt; import copilotj.mcp.prompts.DebugMacroPrompt; import copilotj.mcp.tools.CallActionTool; @@ -100,20 +98,18 @@ private static void startInternal(EventHandler handler, String host, int port) t var listOperationsTool = new ListOperationsTool(handler); var folderSummaryTool = new FolderSummaryTool(); - var envResource = new EnvironmentResource(handler); - var windowsResource = new WindowsResource(handler); - var analyzePrompt = new AnalyzeBioimagePrompt(); var debugPrompt = new DebugMacroPrompt(); mcpServer = McpServer.sync(transport) .serverInfo("CopilotJ", "0.1.0") .instructions( - "CopilotJ provides Fiji/ImageJ2 bioimage analysis tools. " - + "Use capture_fiji_screen to see the current Fiji state, " - + "run_macro to execute ImageJ macros, and capture_image to " - + "inspect specific images. Start by checking the environment " - + "with fiji_environment.") + "CopilotJ drives Fiji/ImageJ2 for bioimage analysis. Start with " + + "fiji_environment and capture_fiji_screen to see the current state. " + + "Run macros with run_macro and inspect images with capture_image. " + + "To operate Fiji's UI (dialogs, buttons, sliders, checkboxes), call " + + "take_snapshot for a component tree where each widget has a ref handle and an actions " + + "list, then call_action(ref, action, parameters) to drive it; re-snapshot after.") // Tools — use method references for BiFunction compatibility .toolCall(RunMacroTool.definition(), runMacroTool::handle) .toolCall(RunScriptTool.definition(), runScriptTool::handle) @@ -124,12 +120,6 @@ private static void startInternal(EventHandler handler, String host, int port) t .toolCall(FijiEnvironmentTool.definition(), fijiEnvironmentTool::handle) .toolCall(ListOperationsTool.definition(), listOperationsTool::handle) .toolCall(FolderSummaryTool.definition(), folderSummaryTool::handle) - // Resources - .resources( - new McpServerFeatures.SyncResourceSpecification( - EnvironmentResource.definition(), envResource::handle), - new McpServerFeatures.SyncResourceSpecification( - WindowsResource.definition(), windowsResource::handle)) // Prompts .prompts( new McpServerFeatures.SyncPromptSpecification( diff --git a/plugin/src/main/java/copilotj/mcp/resources/EnvironmentResource.java b/plugin/src/main/java/copilotj/mcp/resources/EnvironmentResource.java deleted file mode 100644 index c934c53b..00000000 --- a/plugin/src/main/java/copilotj/mcp/resources/EnvironmentResource.java +++ /dev/null @@ -1,45 +0,0 @@ -/** - * SPDX-FileCopyrightText: Copyright contributors to the CopilotJ project. - * - * SPDX-License-Identifier: Apache-2.0 - */ - -package copilotj.mcp.resources; - -import java.util.List; - -import copilotj.EventHandler; -import copilotj.mcp.McpModule; - -import io.modelcontextprotocol.server.McpSyncServerExchange; -import io.modelcontextprotocol.spec.McpSchema; - -public class EnvironmentResource { - - private final EventHandler handler; - - public EnvironmentResource(EventHandler handler) { - this.handler = handler; - } - - public static McpSchema.Resource definition() { - return McpSchema.Resource.builder() - .uri("fiji://environment") - .name("fiji-environment") - .description("Fiji/ImageJ2 environment information") - .mimeType("application/json") - .build(); - } - - public McpSchema.ReadResourceResult handle(McpSyncServerExchange exchange, McpSchema.ReadResourceRequest request) { - try { - String result = McpModule.callEvent(handler, "summarise_environment", null); - return new McpSchema.ReadResourceResult( - List.of(new McpSchema.TextResourceContents(request.uri(), "application/json", result))); - } catch (Exception e) { - return new McpSchema.ReadResourceResult( - List.of(new McpSchema.TextResourceContents(request.uri(), "application/json", - "{\"error\": \"Fiji not connected\"}"))); - } - } -} diff --git a/plugin/src/main/java/copilotj/mcp/resources/WindowsResource.java b/plugin/src/main/java/copilotj/mcp/resources/WindowsResource.java deleted file mode 100644 index 2727a952..00000000 --- a/plugin/src/main/java/copilotj/mcp/resources/WindowsResource.java +++ /dev/null @@ -1,45 +0,0 @@ -/** - * SPDX-FileCopyrightText: Copyright contributors to the CopilotJ project. - * - * SPDX-License-Identifier: Apache-2.0 - */ - -package copilotj.mcp.resources; - -import java.util.List; - -import copilotj.EventHandler; -import copilotj.mcp.McpModule; - -import io.modelcontextprotocol.server.McpSyncServerExchange; -import io.modelcontextprotocol.spec.McpSchema; - -public class WindowsResource { - - private final EventHandler handler; - - public WindowsResource(EventHandler handler) { - this.handler = handler; - } - - public static McpSchema.Resource definition() { - return McpSchema.Resource.builder() - .uri("fiji://windows") - .name("fiji-windows") - .description("Currently open Fiji windows") - .mimeType("application/json") - .build(); - } - - public McpSchema.ReadResourceResult handle(McpSyncServerExchange exchange, McpSchema.ReadResourceRequest request) { - try { - String result = McpModule.callEvent(handler, "take_snapshot", null); - return new McpSchema.ReadResourceResult( - List.of(new McpSchema.TextResourceContents(request.uri(), "application/json", result))); - } catch (Exception e) { - return new McpSchema.ReadResourceResult( - List.of(new McpSchema.TextResourceContents(request.uri(), "application/json", - "{\"error\": \"Fiji not connected\"}"))); - } - } -} diff --git a/plugin/src/main/java/copilotj/mcp/tools/CallActionTool.java b/plugin/src/main/java/copilotj/mcp/tools/CallActionTool.java index 49fe13f8..228866cf 100644 --- a/plugin/src/main/java/copilotj/mcp/tools/CallActionTool.java +++ b/plugin/src/main/java/copilotj/mcp/tools/CallActionTool.java @@ -26,24 +26,32 @@ public CallActionTool(EventHandler handler) { public static McpSchema.Tool definition() { return McpSchema.Tool.builder() .name("call_action") - .description("Execute a UI action from a previous snapshot. " - + "First call take_snapshot() to get available actions and their IDs, " - + "then call this with the snapshot_id and action_id.") + .description("Execute a UI action on a component identified by its ref handle. " + + "First call take_snapshot() to inspect the component tree: each actionable " + + "component carries a ref handle and a per-component actions list. Then call " + + "this with the component's ref, the action's short id (e.g. click, setState, " + + "selectItem, setValue), and the positional parameters. A ref stays valid while the " + + "underlying component is alive; if it is not found the widget closed or the UI " + + "changed, so capture a new snapshot. After the action, take a fresh snapshot to " + + "observe the result.") .inputSchema(new McpSchema.JsonSchema("object", Map.of( - "snapshot_id", Map.of("type", "integer", "description", "Snapshot ID from take_snapshot"), - "action_id", Map.of("type", "integer", "description", "Action ID within the snapshot"), - "parameters", Map.of("type", "array", "description", "Action parameters", "items", Map.of()) + "ref", Map.of("type", "string", "description", "Component ref handle from take_snapshot (e.g. e5)"), + "action", Map.of("type", "string", + "description", "Action short id — the part after the last dot of the action type (e.g. click, setState)"), + "parameters", Map.of("type", "array", + "description", "Positional arguments in the order given by the action's parameters[] in the snapshot, e.g. [false] for setState or [42] for setValue", + "items", Map.of()) ), - List.of("snapshot_id", "action_id"), true, null, null)) + List.of("ref", "action"), true, null, null)) .build(); } public McpSchema.CallToolResult handle(McpSyncServerExchange exchange, McpSchema.CallToolRequest request) { Map<String, Object> args = request.arguments(); var data = new java.util.LinkedHashMap<String, Object>(); - data.put("snapshot_id", args.get("snapshot_id")); - data.put("action_id", args.get("action_id")); + data.put("ref", args.get("ref")); + data.put("action", args.get("action")); if (args.containsKey("parameters")) { data.put("parameters", args.get("parameters")); } diff --git a/plugin/src/main/java/copilotj/mcp/tools/TakeSnapshotTool.java b/plugin/src/main/java/copilotj/mcp/tools/TakeSnapshotTool.java index c8d607b9..2822975e 100644 --- a/plugin/src/main/java/copilotj/mcp/tools/TakeSnapshotTool.java +++ b/plugin/src/main/java/copilotj/mcp/tools/TakeSnapshotTool.java @@ -27,8 +27,11 @@ public static McpSchema.Tool definition() { return McpSchema.Tool.builder() .name("take_snapshot") .description("Get a structured snapshot of the current Fiji UI state. " - + "Returns open windows, available actions, current image name, and screen dimensions. " - + "Use this to understand what's open before running commands.") + + "Returns open windows and their component trees; each actionable component carries " + + "a ref handle and a per-component actions list. A ref is stable for the lifetime of " + + "the same live component instance; if Fiji rebuilds a control it gets a new ref. Pass " + + "the ref plus an action's short id to call_action; each action's parameters[] documents " + + "what to pass. Also returns the current image name and screen dimensions.") .inputSchema(new McpSchema.JsonSchema("object", Map.of(), null, true, null, null)) .build(); } diff --git a/scripts/mcp_smoke_test.py b/scripts/mcp_smoke_test.py index bc437877..27461f9d 100755 --- a/scripts/mcp_smoke_test.py +++ b/scripts/mcp_smoke_test.py @@ -39,7 +39,7 @@ "list_operations", "folder_summary", } -EXPECTED_RESOURCES = {"fiji://environment", "fiji://windows"} +EXPECTED_RESOURCES = set() # windows/environment are now tools (take_snapshot, fiji_environment) EXPECTED_PROMPTS = {"analyze_bioimage", "debug_macro"} # Substrings produced by the Java side when Fiji is not reachable (stable across @@ -316,24 +316,99 @@ def call_tool_quiet(client: McpClient, name: str, args: dict) -> str: return first_text(res or {}).strip() -def find_action_index(snap_text: str, type_suffix: str) -> tuple[int | None, int | None]: - """Parse a take_snapshot result and locate the first action whose ``type`` - ends with ``type_suffix``. +def _walk_components(snap_text: str): + """Yield ``(node, window)`` for every component dict in a take_snapshot tree. - Returns ``(snapshot_id, action_index)``, or ``(None, None)`` if the snapshot - can't be parsed, or ``(snapshot_id, None)`` if it has no matching action. + ``window`` is the top-level window node containing ``node`` (the window node + itself on the first yield of each window). All snapshot-parsing helpers build + on this single traversal (DRY) rather than each re-walking the tree. """ if not snap_text: - return None, None + return try: snap = json.loads(snap_text) except json.JSONDecodeError: - return None, None - snap_id = snap.get("id") - for i, action in enumerate(snap.get("actions") or []): - if isinstance(action, dict) and str(action.get("type", "")).endswith(type_suffix): - return snap_id, i - return snap_id, None + return + + def walk(node): + if isinstance(node, dict): + yield node + for child in node.get("children") or []: + yield from walk(child) + + for window in snap.get("windows") or []: + for node in walk(window): + yield node, window + + +def find_action_ref( + snap_text: str, + type_suffix: str, + *, + window_title: str | None = None, + label: str | None = None, +) -> tuple[str | None, str | None]: + """Locate the first component whose action ``type`` ends with ``type_suffix``. + + Returns ``(ref, action_short_id)``, where ``action_short_id`` is the substring + after the last dot of the action type (e.g. ``java.awt.Checkbox.setState`` -> + ``setState``). Optional ``window_title`` (substring of the window's title/name) + and ``label`` (exact component label) scope the search so the target is + deterministic rather than "the first match anywhere in the tree". Returns + ``(None, None)`` if no match is found or the snapshot can't be parsed. + """ + for node, window in _walk_components(snap_text): + ref = node.get("ref") + if ref is None: + continue + if label is not None and node.get("label") != label: + continue + if window_title is not None: + wtitle = "" + if isinstance(window, dict): + wtitle = window.get("title") or window.get("name") or "" + if window_title not in wtitle: + continue + for action in node.get("actions") or []: + if isinstance(action, dict) and str(action.get("type", "")).endswith(type_suffix): + return ref, str(action.get("type", "")).rsplit(".", 1)[-1] + return None, None + + +def checkbox_state_by_ref(snap_text: str, ref: str) -> bool | None: + """Return the ``state`` of the checkbox identified by ``ref``, or ``None``.""" + for node, _ in _walk_components(snap_text): + if node.get("ref") == ref and "state" in node: + return bool(node["state"]) + return None + + +def expect_tool_error( + client: McpClient, + results: Results, + name: str, + args: dict, + needle: str, +) -> None: + """Assert a tools/call returns ``isError`` whose text contains ``needle``. + + Used for the ref-model's error contracts (stale/bogus ref -> "not found"; + unknown action -> "unknown action"). A Fiji-unreachable result downgrades to + SKIP; anything else is a FAIL. + """ + try: + res = client.call("tools/call", {"name": "call_action", "arguments": args}) + except McpError as e: + results.add(name, "FAIL", str(e)[:200]) + return + res = res or {} + text = first_text(res).strip() + if res.get("isError") and needle in text.lower(): + results.add(name, "PASS", text[:140]) + elif fiji_down(text): + results.add(name, "SKIP", "Fiji unreachable") + else: + results.add(name, "FAIL", f"expected isError + '{needle}', got: {text[:200]}") # --------------------------------------------------------------------------- # @@ -417,9 +492,11 @@ def main() -> int: results.add("notifications/initialized", "FAIL", str(e)[:200]) # 3. tools/list — expect exactly the 9 tools. + tools: list[dict] = [] try: res = client.call("tools/list") - names = {t.get("name") for t in (res or {}).get("tools", [])} + tools = (res or {}).get("tools", []) + names = {t.get("name") for t in tools} if names == EXPECTED_TOOLS: results.add("tools/list", "PASS", f"{len(names)} tools") else: @@ -431,12 +508,33 @@ def main() -> int: except McpError as e: results.add("tools/list", "FAIL", str(e)[:200]) - # 4. resources/list. + # 3b. Tool metadata — descriptions/schemas must teach the snapshot->ref->action + # loop, otherwise the metadata change can regress while every other check still + # passes (codex #5). + tools_by_name = {t.get("name"): t for t in tools if isinstance(t, dict)} + ts_desc = (tools_by_name.get("take_snapshot") or {}).get("description", "") + ca = tools_by_name.get("call_action") or {} + ca_desc = ca.get("description", "") + ca_params = ((ca.get("inputSchema") or {}).get("properties") or {}).get("parameters", {}) + ca_params_desc = ca_params.get("description", "") if isinstance(ca_params, dict) else "" + md_problems: list[str] = [] + if "ref" not in ts_desc: + md_problems.append("take_snapshot description lacks 'ref'") + if "ref" not in ca_desc: + md_problems.append("call_action description lacks 'ref'") + if not ca_params_desc or ca_params_desc.strip() == "Action parameters": + md_problems.append("call_action parameters description is still the bare 'Action parameters'") + if md_problems: + results.add("tool metadata", "FAIL", "; ".join(md_problems)) + else: + results.add("tool metadata", "PASS", "descriptions + parameters schema teach the ref loop") + + # 4. resources/list — no resources are exposed (windows/environment are tools). try: res = client.call("resources/list") uris = {r.get("uri") for r in (res or {}).get("resources", [])} if uris == EXPECTED_RESOURCES: - results.add("resources/list", "PASS", f"{len(uris)} resources") + results.add("resources/list", "PASS", "no resources (converted to tools)") else: results.add( "resources/list", @@ -476,44 +574,88 @@ def main() -> int: call_tool(client, "run_macro", {"script": 'print("smoke test");'}, results) call_tool(client, "run_script", {"language": "macro", "script": 'print("py smoke");'}, results) call_tool(client, "take_snapshot", {}, results) - # call_action needs a real snapshot_id + action_id pointing at an interactable - # widget. A bare Fiji has none, so open ROI Manager (non-modal; its "Show All"/ - # "Labels" checkboxes expose a safe, side-effect-free checkbox.setState action), - # re-snapshot, then toggle it. SKIP if no such action is discoverable. + # call_action + the ref-model invariants need an interactable widget. A bare Fiji + # has none, so open ROI Manager (its "Show All" checkbox exposes a safe + # checkbox.setState action) and target it deterministically by label (codex #3). call_tool_quiet(client, "run_macro", {"script": 'run("ROI Manager...");'}) - snap_id, action_idx = find_action_index(call_tool_quiet(client, "take_snapshot", {}), ".setState") - if snap_id is None or action_idx is None: - results.add("call_action", "SKIP", "no checkbox action in snapshot (ROI Manager unavailable?)") + snap_a = call_tool_quiet(client, "take_snapshot", {}) + ref, action_name = find_action_ref(snap_a, ".setState", label="Show All") + if ref is None: + # Fall back to any setState if the named label isn't present. + ref, action_name = find_action_ref(snap_a, ".setState") + + # Check 1 — ref stability: the same widget keeps the same ref across snapshots + # that don't change the UI (scoped to the targeted checkbox; codex #4). + snap_b = call_tool_quiet(client, "take_snapshot", {}) + ref_b, _ = find_action_ref(snap_b, ".setState", label="Show All") if ref else (None, None) + if ref is None or action_name is None: + results.add("call_action (stability)", "SKIP", "no checkbox action (ROI Manager unavailable?)") + elif ref_b == ref: + results.add("call_action (stability)", "PASS", f"{ref} stable across 2 snapshots") else: - call_tool( - client, "call_action", {"snapshot_id": snap_id, "action_id": action_idx, "parameters": [False]}, results + results.add("call_action (stability)", "FAIL", f"{ref} -> {ref_b} (expected stable)") + + if ref is not None and action_name is not None: + # Check 3 — round-trip: flip the checkbox, re-snapshot, verify, restore. + before = checkbox_state_by_ref(snap_a, ref) + if before is None: + results.add("call_action (round-trip)", "SKIP", "could not read checkbox state") + else: + target = not before + call_tool_quiet( + client, + "call_action", + {"ref": ref, "action": action_name, "parameters": [target]}, + ) + after = checkbox_state_by_ref(call_tool_quiet(client, "take_snapshot", {}), ref) + call_tool_quiet( + client, + "call_action", + {"ref": ref, "action": action_name, "parameters": [before]}, # restore + ) + if after == target: + results.add("call_action (round-trip)", "PASS", f"state {before}->{after} (restored)") + else: + results.add("call_action (round-trip)", "FAIL", f"expected {target}, got {after}") + + # Check 4 — unknown action id on a valid ref -> distinct error contract. + expect_tool_error( + client, + results, + "call_action (unknown action)", + {"ref": ref, "action": "bogusAction", "parameters": []}, + "unknown action", + ) + + # Check 2 — stale ref: close the window, then the ref must be "not found" + # (the real production invariant — a previously-valid ref goes stale; codex #1). + call_tool_quiet( + client, + "run_macro", + {"script": 'selectWindow("ROI Manager"); run("Close");'}, ) + expect_tool_error( + client, + results, + "call_action (stale ref)", + {"ref": ref, "action": action_name, "parameters": [False]}, + "not found", + ) + + # Cheap extra: a fabricated ref also yields "not found" (stable baseline that + # doesn't depend on the window-close succeeding). + expect_tool_error( + client, + results, + "call_action (bogus ref)", + {"ref": "e999999", "action": "setState", "parameters": [False]}, + "not found", + ) since = (datetime.now(timezone.utc) - timedelta(minutes=5)).isoformat() call_tool(client, "list_operations", {"since": since}, results) call_tool(client, "capture_image", {}, results) call_tool(client, "capture_fiji_screen", {}, results) - # 15. resources/read for both URIs. - for uri in sorted(EXPECTED_RESOURCES): - try: - res = client.call("resources/read", {"uri": uri}) - text = "" - for item in (res or {}).get("contents", []): - if isinstance(item, dict) and "text" in item: - text = item.get("text", "") - break - text = text.strip() - if fiji_down(text): - results.add(f"resources/read {uri}", "SKIP", text[:140]) - else: - results.add( - f"resources/read {uri}", - "PASS", - text.replace("\n", " ")[:140] or "ok", - ) - except McpError as e: - results.add(f"resources/read {uri}", "FAIL", str(e)[:200]) - # 16-17. prompts/get for both prompts (debug_macro requires both args). try: res = client.call(