diff --git a/.github/workflows/check-skill-sync.yml b/.github/workflows/check-skill-sync.yml index c7061f4..e0169d3 100644 --- a/.github/workflows/check-skill-sync.yml +++ b/.github/workflows/check-skill-sync.yml @@ -4,14 +4,14 @@ on: push: branches: [main] paths: - - "plugins/claude-code/skills/drawio/SKILL.md" - - "plugins/codex/drawio/skills/drawio/SKILL.md" - - "plugins/copilot/skills/drawio/SKILL.md" + - "plugins/claude-code/skills/drawio/**" + - "plugins/codex/drawio/skills/drawio/**" + - "plugins/copilot/skills/drawio/**" pull_request: paths: - - "plugins/claude-code/skills/drawio/SKILL.md" - - "plugins/codex/drawio/skills/drawio/SKILL.md" - - "plugins/copilot/skills/drawio/SKILL.md" + - "plugins/claude-code/skills/drawio/**" + - "plugins/codex/drawio/skills/drawio/**" + - "plugins/copilot/skills/drawio/**" workflow_dispatch: permissions: @@ -24,27 +24,30 @@ jobs: steps: - uses: actions/checkout@v5 - - name: Verify port SKILL.md copies are byte-identical to the Claude Code copy + - name: Verify port skill copies are byte-identical to the Claude Code copies run: | - claude="plugins/claude-code/skills/drawio/SKILL.md" status=0 - for port in \ - "plugins/codex/drawio/skills/drawio/SKILL.md" \ - "plugins/copilot/skills/drawio/SKILL.md"; do - - if [ ! -f "$port" ]; then - echo "$port does not exist (port not present) — nothing to check." - continue - fi - - if cmp -s "$claude" "$port"; then - echo "OK: $port is byte-identical to $claude." - else - echo "::error file=$port::Must be byte-identical to $claude — mirror the edit to both copies." - diff -u "$claude" "$port" || true - status=1 - fi + for file in "SKILL.md" "scripts/fix_edge_parents.py"; do + claude="plugins/claude-code/skills/drawio/$file" + + for port in \ + "plugins/codex/drawio/skills/drawio/$file" \ + "plugins/copilot/skills/drawio/$file"; do + + if [ ! -f "$port" ]; then + echo "$port does not exist (port not present) — nothing to check." + continue + fi + + if cmp -s "$claude" "$port"; then + echo "OK: $port is byte-identical to $claude." + else + echo "::error file=$port::Must be byte-identical to $claude — mirror the edit to both copies." + diff -u "$claude" "$port" || true + status=1 + fi + done done exit $status diff --git a/.github/workflows/test-scripts.yml b/.github/workflows/test-scripts.yml new file mode 100644 index 0000000..7e81563 --- /dev/null +++ b/.github/workflows/test-scripts.yml @@ -0,0 +1,41 @@ +name: Test Scripts + +on: + push: + branches: [main] + paths: + - "plugins/tests/**" + - "plugins/claude-code/skills/drawio/scripts/**" + - "plugins/codex/drawio/skills/drawio/scripts/**" + - "plugins/copilot/skills/drawio/scripts/**" + - ".github/workflows/test-scripts.yml" + pull_request: + paths: + - "plugins/tests/**" + - "plugins/claude-code/skills/drawio/scripts/**" + - "plugins/codex/drawio/skills/drawio/scripts/**" + - "plugins/copilot/skills/drawio/scripts/**" + - ".github/workflows/test-scripts.yml" + workflow_dispatch: + +permissions: + contents: read + +jobs: + unittest: + runs-on: ubuntu-latest + + strategy: + fail-fast: false + matrix: + python-version: ["3.9", "3.14"] + + steps: + - uses: actions/checkout@v5 + + - uses: actions/setup-python@v5 + with: + python-version: ${{ matrix.python-version }} + + - name: Run the test suite + run: python -m unittest discover -s plugins/tests -v diff --git a/.gitignore b/.gitignore index 1c5b955..15673cf 100644 --- a/.gitignore +++ b/.gitignore @@ -36,6 +36,10 @@ pnpm-lock.yaml # TypeScript cache *.tsbuildinfo +# Python cache +__pycache__/ +*.pyc + # Claude Code .claude/ /test/ diff --git a/CLAUDE.md b/CLAUDE.md index e427aa8..a0c3438 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -12,9 +12,10 @@ The official draw.io MCP (Model Context Protocol) server that enables LLMs to op - **`mcp-tool-server/`** — Original MCP tool server (stdio-based, opens browser). Published as `@drawio/mcp` on npm. - **`project-instructions/`** — Claude Project instructions (no MCP required, no install). - **`plugins/`** — Assistant-side plugins grouped by host, one subdirectory per AI assistant. - - **`plugins/claude-code/`** — Claude Code plugin: ships the `drawio` skill (generates native `.drawio` files, authored as Mermaid — converted + laid out by the desktop CLI — or as XML directly with optional ELK `--layout`; exports to PNG/SVG/PDF, or opens as a browser URL via `app.diagrams.net`). Mermaid conversion, ELK layout, and image export need draw.io Desktop; plain XML `.drawio`/`url` output does not. Installable via the repo-root marketplace or `claude --plugin-dir ./plugins/claude-code`. No MCP required. - - **`plugins/codex/drawio/`** — Codex CLI plugin: the Codex port of the Claude Code plugin, shipping the same `drawio` skill. `skills/drawio/SKILL.md` is byte-identical to the Claude plugin's copy (Codex uses the same `/drawio:drawio` invocation and fetches the same shared references from GitHub). Differs only in host wrapping: a `.codex-plugin/plugin.json` manifest with an `interface` block (official draw.io SVG logo, `brandColor`, default prompts). Nested under `codex/` because Codex requires the plugin root folder name to equal `plugin.json` `"name"` (`drawio`). No MCP required. - - **`plugins/copilot/`** — GitHub Copilot CLI plugin: the Copilot port of the Claude Code plugin, shipping the same `drawio` skill. `skills/drawio/SKILL.md` is byte-identical to the Claude plugin's copy (the in-skill `/drawio:drawio` example lines are model-facing; Copilot's user-facing command is plain `/drawio` since Copilot doesn't prefix plugin skills). Differs only in host wrapping: a root `plugin.json` manifest (Copilot's format, `skills` directory list); no folder-name rule, so `copilot/` is itself the plugin root. The same skill folder also works in other Copilot surfaces (VS Code agent mode, coding agent, code review) when copied to a repo's `.github/skills/`. No MCP required. + - **`plugins/claude-code/`** — Claude Code plugin: ships the `drawio` skill (generates native `.drawio` files, authored as Mermaid — converted + laid out by the desktop CLI — or as XML directly with optional ELK `--layout`; exports to PNG/SVG/PDF, or opens as a browser URL via `app.diagrams.net`). Mermaid conversion, ELK layout, and image export need draw.io Desktop; plain XML `.drawio`/`url` output does not. The skill also bundles `skills/drawio/scripts/fix_edge_parents.py`, a stdlib-only Python helper it runs on every generated `.drawio` (before any layout pass) to set each edge's `parent` to the nearest common ancestor of its `source` and `target` — the rule [`mxGraphModel.updateEdgeParent`](https://github.com/jgraph/drawio/blob/v31.1.8/src/main/webapp/mxgraph/src/model/mxGraphModel.js#L832-L885) applies (the script is a port of draw.io's bundled copy, and credits its copyright holders in its docstring), and what a layout pass needs in order to lay the diagram out correctly. Installable via the repo-root marketplace or `claude --plugin-dir ./plugins/claude-code`. No MCP required. + - **`plugins/codex/drawio/`** — Codex CLI plugin: the Codex port of the Claude Code plugin, shipping the same `drawio` skill. `skills/drawio/SKILL.md` and `skills/drawio/scripts/fix_edge_parents.py` are byte-identical to the Claude plugin's copies (Codex uses the same `/drawio:drawio` invocation and fetches the same shared references from GitHub). Differs only in host wrapping: a `.codex-plugin/plugin.json` manifest with an `interface` block (official draw.io SVG logo, `brandColor`, default prompts). Nested under `codex/` because Codex requires the plugin root folder name to equal `plugin.json` `"name"` (`drawio`). No MCP required. + - **`plugins/copilot/`** — GitHub Copilot CLI plugin: the Copilot port of the Claude Code plugin, shipping the same `drawio` skill. `skills/drawio/SKILL.md` and `skills/drawio/scripts/fix_edge_parents.py` are byte-identical to the Claude plugin's copies (the in-skill `/drawio:drawio` example lines are model-facing; Copilot's user-facing command is plain `/drawio` since Copilot doesn't prefix plugin skills). Differs only in host wrapping: a root `plugin.json` manifest (Copilot's format, `skills` directory list); no folder-name rule, so `copilot/` is itself the plugin root. The same skill folder also works in other Copilot surfaces (VS Code agent mode, coding agent, code review) when copied to a repo's `.github/skills/`. No MCP required. + - **`plugins/tests/`** — Python `unittest` suite for the scripts the plugins bundle (currently `fix_edge_parents.py`, tested through the Claude Code copy — the Codex and Copilot copies are byte-identical, which `check-skill-sync.yml` enforces). Run `python3 -m unittest discover -s plugins/tests` from the repo root; CI runs it via `.github/workflows/test-scripts.yml`. The JavaScript projects keep their own tests next to their source (`mcp-tool-server/test/`, `node --test`). - **`shape-search/`** — Shape search index generator. Loads draw.io's `app.min.js` via jsdom to extract all shape styles and tags into `search-index.json`, which powers the `search_shapes` MCP tool. Re-run after updating `drawio-dev` to pick up new or changed shapes. Most subdirectories have their own `CLAUDE.md` with implementation details. diff --git a/plugins/README.md b/plugins/README.md index c6adeae..978f1b0 100644 --- a/plugins/README.md +++ b/plugins/README.md @@ -41,7 +41,8 @@ plugins/ ├── claude-code/ ← Claude Code plugin (plugin root) ├── codex/ ← Codex host group │ └── drawio/ ← Codex plugin root (folder name == plugin.json "name") -└── copilot/ ← GitHub Copilot CLI plugin (plugin root) +├── copilot/ ← GitHub Copilot CLI plugin (plugin root) +└── tests/ ← tests for the scripts the plugins bundle ``` Codex normalizes a plugin's root folder name to match its `plugin.json` `"name"`, so the @@ -50,6 +51,10 @@ Claude Code and Copilot have no such rule, so `claude-code/` and `copilot/` are the plugin roots. If another assistant (Cursor, etc.) is added later, it follows the same pattern in its own way. +[`tests/`](tests/README.md) is not a plugin: it holds the Python `unittest` suite for the +scripts the plugins bundle (`python3 -m unittest discover -s plugins/tests` from the repo +root). + The draw.io guidance itself — *how* to generate `.drawio` files, embed XML in PNG/SVG/PDF, and produce `app.diagrams.net` URLs — is shared. Only the wrapping (manifest format, file layout, invocation prefix) differs per host, and each host has its own plugin/skill model, so the wrapping is not assumed to be uniform. The single source of truth for draw.io XML generation guidance lives at [`../shared/xml-reference.md`](../shared/xml-reference.md) — every plugin references that file rather than duplicating its contents. diff --git a/plugins/claude-code/DEVELOPING.md b/plugins/claude-code/DEVELOPING.md index 652c039..4f10ac8 100644 --- a/plugins/claude-code/DEVELOPING.md +++ b/plugins/claude-code/DEVELOPING.md @@ -10,10 +10,11 @@ Previously distributed as a bare `SKILL.md` users copied into `~/.claude/skills/ |------|---------| | `.claude-plugin/plugin.json` | Plugin manifest — the single source of truth for name, version, description, author, license | | `skills/drawio/SKILL.md` | The skill itself (its folder name `drawio` becomes the second half of the `/drawio:drawio` invocation) | +| `skills/drawio/scripts/fix_edge_parents.py` | Bundled helper the skill runs on every generated `.drawio`: sets each edge's `parent` to the nearest common ancestor of its `source` and `target` (see [Edge parents](#edge-parents)) | | `README.md` | Installation and usage documentation | | `../../.claude-plugin/marketplace.json` | Marketplace manifest at the repo root; lists this plugin with `source: "./plugins/claude-code"` and inherits the rest of its metadata from `plugin.json` | -**⚠️ Mirrored skill:** the Codex plugin ships a byte-identical copy of `skills/drawio/SKILL.md` at `plugins/codex/drawio/skills/drawio/SKILL.md`. Any edit to the skill must be applied to both files — CI ([`check-skill-sync.yml`](../../.github/workflows/check-skill-sync.yml)) fails if the copies drift. +**⚠️ Mirrored skill:** the Codex and Copilot plugins ship byte-identical copies of `skills/drawio/SKILL.md` and `skills/drawio/scripts/fix_edge_parents.py` at `plugins/codex/drawio/skills/drawio/` and `plugins/copilot/skills/drawio/`. Any edit to either file must be applied to all copies — CI ([`check-skill-sync.yml`](../../.github/workflows/check-skill-sync.yml)) fails if they drift. ## How It Works @@ -23,16 +24,60 @@ Everything becomes a native `.drawio` file first, then is delivered in the reque 2. Claude picks an authoring route: - **Mermaid** (preferred for standard types when the desktop CLI is present) — writes a `.mmd` file, then `drawio -x -f xml -o name.drawio name.mmd` converts and lays it out; the `.mmd` is deleted - **XML** — writes mxGraphModel XML to `name.drawio`; optionally `drawio -x -f xml --layout -o name.drawio name.drawio` applies an ELK layout (reading and overwriting the same path is supported) -3. Format-specific handling (identical for both routes, since both produce a `.drawio`): +3. `python3 scripts/fix_edge_parents.py name.drawio` repairs the edge `parent` attributes before any layout pass (see [Edge parents](#edge-parents)) +4. Format-specific handling (identical for both routes, since both produce a `.drawio`): - **png/svg/pdf** — the draw.io CLI exports to `.drawio.png` / `.drawio.svg` / `.drawio.pdf` with `--embed-diagram`, then deletes the source `.drawio` file - **url** — a `node -e` one-liner reads the `.drawio` file, compresses it with `zlib.deflateRawSync` + base64, builds `https://app.diagrams.net/?...#create={type,compressed,data}`, and opens it in the browser. The `.drawio` file is kept for persistence. - **default** — no extra step, the `.drawio` file is the output -4. The result is opened for viewing (`open` / `xdg-open` / `start`; on Windows/WSL2, `url` mode uses a temp `.url` file because `cmd.exe` strips the `#create=...` fragment) +5. The result is opened for viewing (`open` / `xdg-open` / `start`; on Windows/WSL2, `url` mode uses a temp `.url` file because `cmd.exe` strips the `#create=...` fragment) Default output is `.drawio` (no export). The user requests another output mode by mentioning the format: `/drawio:drawio png ...`, `/drawio:drawio svg: ...`, `/drawio:drawio url ...`, etc. **Mermaid → PNG caveat:** direct `.mmd` → PNG with `-e` crashes in current draw.io Desktop (`writePngWithText` receives an undefined `args.xml` at the embed step in `electron.js`). The skill always converts Mermaid to `.drawio` first and exports that, which sidesteps the bug and yields a correct embed. The layout names, JSON schema, and CLI verification are documented in `SKILL.md`. +## Edge parents + +mxGraph expects an edge's `parent` to be the nearest common ancestor of its `source` and `target` cells. LLM-authored XML (and, for `subgraph` blocks, Mermaid conversion) tends to leave every edge on the default layer (`parent="1"`) instead. Such a file renders correctly, but the ELK/`--layout` passes lay it out wrongly. + +`skills/drawio/scripts/fix_edge_parents.py` is the fix, and `SKILL.md` makes running it a pipeline step of its own — after authoring, before `--layout` and before delivery: + +```bash +python3 scripts/fix_edge_parents.py diagram.drawio +``` + +- **Rule** — a faithful port of [`mxGraphModel.updateEdgeParent`](https://github.com/jgraph/drawio/blob/v31.1.8/src/main/webapp/mxgraph/src/model/mxGraphModel.js#L832-L885) (with [`getOrigin`](https://github.com/jgraph/drawio/blob/v31.1.8/src/main/webapp/mxgraph/src/model/mxGraphModel.js#L893-L918), [`getNearestCommonAncestor`](https://github.com/jgraph/drawio/blob/v31.1.8/src/main/webapp/mxgraph/src/model/mxGraphModel.js#L930-L970) and [`mxGeometry.translate`](https://github.com/jgraph/drawio/blob/v31.1.8/src/main/webapp/mxgraph/src/model/mxGeometry.js#L299-L337), as bundled with draw.io v31.1.8): a relative source (a port, a label) resolves to its owner, a self-loop takes its terminal's parent, the root layer's own children are never pulled into a sibling layer, and the edge geometry (`x`/`y`, `sourcePoint`, `targetPoint`, `Array as="points"`) is translated by the origin delta so the edge does not move on screen. `as="offset"` points are left alone, matching `mxGeometry.translate`. The port is deliberately transcription-close: `update_edge_parent()` follows the JavaScript statement by statement, and the model helpers keep the upstream names (`get_parent`, `get_terminal`, `get_origin`, `get_nearest_common_ancestor`, `is_ancestor`, `is_edge`, `translate`) and its local variable names (`source`, `target`, `cell`, `geo`, `origin1`/`origin2`, `dx`/`dy`). +- **draw.io's override** — draw.io sets [`ignoreRelativeEdgeParent = false`](https://github.com/jgraph/drawio/blob/v31.1.8/src/main/webapp/js/grapheditor/Graph.js#L175) ("Keeps edges between relative child cells inside parent"), where bare mxGraph defaults it to `true`. So a relative *target* is **not** resolved to its owner and the edge stays inside the cell that owns the port. The script follows draw.io, not the mxGraph default; `IGNORE_RELATIVE_EDGE_PARENT` at the top of the file is the switch. No other `mxGraphModel`/`mxGeometry`/`mxCell` prototype override in the draw.io webapp touches the ported code paths. +- **Byte-preserving** — the file is parsed with `expat` while recording start-tag offsets (`parser.CurrentByteIndex`), and only the attributes that must change are spliced back into the original bytes. No reformatting, no attribute reordering, no dropped declaration. Re-running it is a no-op. +- **Inputs** — a bare `` or an ``; every page is processed, compressed pages included (`deflateRaw` + base64, re-encoded the same way). A compressed page is read as the concatenation of its text runs, the string `Editor.parseDiagramNode` decodes, so character references and CDATA framing are handled; a page that ends up rewritten has its whole content replaced, which drops any comment sitting inside it. Non-ASCII content (labels, ids, escaped markup, a UTF-8 BOM, single-byte encodings such as `ISO-8859-1`) is handled byte-for-byte; UTF-16/32 input is rejected with an error rather than patched, since byte-offset splicing assumes single-byte markup. +- **Dependencies** — Python 3 standard library only. No draw.io Desktop, no network, no npm/pip packages. +- **CLI** — one or more paths (in place), `-` for stdin → stdout, `-n`/`--dry-run` to report only, `-q`/`--quiet` to silence the per-file report on stderr. Exit status is 1 if a file could not be read or parsed. The report names every edge it touched: + + ``` + diagram.drawio: 1 edge parent(s) fixed + e2: parent="1" -> parent="vpc" + ``` + +- **Deviations from upstream** — kept to these three, everything else follows the JavaScript: (1) `updateEdgeParent` finishes with `this.add(cell, edge, this.getChildCount(cell))`, making the edge the *last* child of its new parent; the script only rewrites the `parent` attribute and leaves the element where it is in the document, so the child index (z-order within the parent) can differ. (2) A zero origin delta skips the geometry translation instead of adding `x="0" y="0"`. (3) Parent walks stop on a cell parented to itself or to a cycle, which upstream cannot represent and would recurse on forever. +- **Attribute semantics** — flags are read the way mxGraph evaluates them (`edge != 0`, `if (geo.relative)`), so only a missing, empty or numerically zero value is false; coordinates are read with JavaScript `parseFloat` semantics and non-finite results become 0, matching `mxObjectCodec.convertAttributeFromXml`; numbers are written back with shortest-round-trip formatting, as JavaScript stringifies them. +- **Tests** — [`plugins/tests/`](../tests/README.md), `python3 -m unittest discover -s plugins/tests` from the repo root (stdlib only, also run by [`test-scripts.yml`](../../.github/workflows/test-scripts.yml)). The fixtures are compared byte for byte, so a change that reformats anything fails the suite. +- **Attribution** — the ported files carry `Copyright (c) 2006-2018, JGraph Holdings Ltd` / `Copyright (c) 2006-2018, draw.io AG` (`mxGraphModel.js`) and `Copyright (c) 2006-2015, JGraph Holdings Ltd` / `Copyright (c) 2006-2015, draw.io AG` (`mxGeometry.js`). The script's docstring reproduces both notices, links each ported function, and points at [draw.io's `LICENSE`](https://github.com/jgraph/drawio/blob/v31.1.8/LICENSE) (Apache 2.0) for the terms. + +The shared `shared/xml-reference.md` still tells authors to give cross-container edges `parent="1"`; `SKILL.md` states that the nearest-common-ancestor rule takes precedence, and the script repairs the output either way. + +### Why byte-preserving + +Rewriting the file through a DOM (`ElementTree` or similar) would have been far less code. Splicing bytes was chosen anyway, for two reasons: + +- **The agent's context stays accurate.** Nothing changes except the `parent` attributes, plus any waypoints that have to shift with them, so the file on disk still matches the XML the agent holds in context apart from exactly those attributes — and the report (`e2: parent="1" -> parent="vpc"`) accounts for that difference one-to-one. +- **Users can run it on existing diagrams.** Re-serializing changes the XML declaration, indentation, attribute order (draw.io writes `id` → `value` → `style` …) and `` framing. Semantically identical, but in a Git-tracked `.drawio` a one-attribute fix would surface as a whole-file diff. + +The script works as follows: + +1. The script records the byte offsets of each start/end tag reported by the Expat parser. +2. After modifying the attributes of the in-memory model, the script re-scans the open tag and records the byte offsets of the attributes. +3. The script makes patches. +4. The script applies the patches from end to beginning. + ## URL Mode Compatibility The `url` mode produces the exact same `https://app.diagrams.net/#create=...` URL format as the MCP Tool Server (`mcp-tool-server/src/index.js`). Node.js's built-in `zlib.deflateRawSync` and `pako.deflateRaw` both implement RFC 1951, so their outputs are interchangeable. No external npm dependencies are added to the skill — only Node.js built-ins (`zlib`, `child_process`, `fs`, `os`, `path`). diff --git a/plugins/claude-code/README.md b/plugins/claude-code/README.md index 3019421..a4915aa 100644 --- a/plugins/claude-code/README.md +++ b/plugins/claude-code/README.md @@ -8,16 +8,18 @@ When you ask Claude Code to create a diagram, it will: 1. Choose how to author it — **Mermaid** for standard types (flowchart, sequence, class, state, ER, gantt, mindmap…) when the desktop app is installed, or **draw.io XML** for custom styling, precise positioning, specific shape libraries, or when no desktop app is present 2. Produce a native `.drawio` file — convert the Mermaid with the desktop CLI, or write the XML directly (optionally running an ELK `--layout` pass so you don't hand-place cells) -3. Handle the requested output: +3. Run the bundled `fix_edge_parents.py` over the file so every connector is parented to the innermost container holding both of its endpoints — the rule the draw.io editor applies, and what keeps a layout pass from laying the diagram out wrongly +4. Handle the requested output: - PNG / SVG / PDF — export using the draw.io desktop CLI - `url` — compress the XML with Node.js's built-in `zlib` and open `https://app.diagrams.net/#create=...` in your browser (keeps the `.drawio` file as a local copy) - *(default)* — leave the `.drawio` file as-is -4. Open the result +5. Open the result ## Prerequisites - [Claude Code](https://docs.anthropic.com/en/docs/claude-code) installed - [draw.io Desktop](https://github.com/jgraph/drawio-desktop/releases) installed — required for Mermaid conversion, ELK layout, and PNG/SVG/PDF export. Not needed for plain XML `.drawio` or `url` output, which Claude can produce with no desktop app +- Python 3 on your `PATH` — used by the bundled edge-parent fixer (standard library only, no packages to install). Without it Claude sets the edge parents itself while authoring ## Installation diff --git a/plugins/claude-code/skills/drawio/SKILL.md b/plugins/claude-code/skills/drawio/SKILL.md index 54ead51..1d0ab04 100644 --- a/plugins/claude-code/skills/drawio/SKILL.md +++ b/plugins/claude-code/skills/drawio/SKILL.md @@ -33,23 +33,43 @@ Every diagram becomes a native `.drawio` file first, then is delivered in the re ``` Delete the `.mmd` afterward — the `.drawio` is the artifact. draw.io's Mermaid parser has already laid the diagram out, so no `--layout` is needed. - **XML**: write the mxGraphModel XML to `diagram.drawio` (see [XML format](#xml-format)). Optionally apply an ELK layout (see [ELK layout for XML](#elk-layout-for-xml)). -2. **Deliver** (identical for both sources): +2. **Fix edge parents** — see [Edge parents](#edge-parents): + ```bash + python3 scripts/fix_edge_parents.py diagram.drawio + ``` +3. **Deliver** (identical for both sources): - *(no format)* → keep `diagram.drawio` and open it. - **png / svg / pdf** → export from the `.drawio` with embedded XML, then delete the source `.drawio`: ```bash drawio -x -f png -e -b 10 -o diagram.drawio.png diagram.drawio ``` - **url** → build a browser URL from the `.drawio` XML, open it, and keep the `.drawio` as a local copy (see [Browser URL output](#browser-url-output)). -3. **Open the result** — the exported file, the URL, or the `.drawio`. If the open command fails, print the absolute path (or URL) so the user can open it manually. +4. **Open the result** — the exported file, the URL, or the `.drawio`. If the open command fails, print the absolute path (or URL) so the user can open it manually. **Always convert Mermaid to `.drawio` first, then export** — do not export a `.mmd` straight to an image. Direct Mermaid → PNG export with `-e` is broken in current draw.io Desktop (the embedded-XML step crashes); the two-step path (convert, then export the `.drawio`) is reliable and produces an editable embed. See [Troubleshooting](#troubleshooting). If Mermaid was requested but no desktop CLI is available, fall back to authoring the same diagram directly as XML. +## Edge parents + +An edge's `parent` must be the **nearest common ancestor** of its `source` and `target` cells — the innermost container holding both endpoints, or the layer when there is none. Otherwise a layout pass lays the diagram out wrongly. This overrides the "cross-container edges use `parent="1"`" advice in the [XML reference](#xml-reference). + +Don't work it out by hand — a script bundled with this skill fixes every edge in place: + +```bash +python3 scripts/fix_edge_parents.py diagram.drawio +``` + +- The path is relative to this `SKILL.md`; use the absolute path from another working directory. +- Run it on **every** `.drawio` — Mermaid output too, since `subgraph` blocks become containers — and **before** any `--layout` pass. Running it twice is a no-op. +- If `python3` is unavailable, set each edge's `parent` to the nearest common ancestor while authoring instead. + ## ELK layout for XML XML-authored diagrams can be auto-positioned by the CLI's `--layout` pass — the same ELK layouts as the editor's *Arrange ▸ Layout* menu and the same engine the draw.io MCP app server uses. Generate the cells with approximate (or even `0,0`) positions and let ELK place them; you only have to get the graph *structure* — nodes and edges — right. +Run `python3 scripts/fix_edge_parents.py diagram.drawio` first — see [Edge parents](#edge-parents). + Add `--layout ` to any CLI call that reads your XML. The simplest form lays out in place after you write the file (reading and overwriting the same path is supported): ```bash @@ -359,6 +379,7 @@ Every diagram must have this structure: - Cell `id="0"` is the root layer - Cell `id="1"` is the default parent layer - All diagram elements use `parent="1"` unless using multiple layers +- An edge's `parent` is the nearest common ancestor of its `source` and `target`, not necessarily `1` — see [Edge parents](#edge-parents) (The example above uses an XML comment only to point out where cells go — never emit comments in real output; see [XML well-formedness](#critical-xml-well-formedness).) @@ -374,6 +395,7 @@ https://raw.githubusercontent.com/jgraph/drawio-mcp/main/shared/xml-reference.md | draw.io CLI not found | Desktop app not installed or not on PATH | Author as XML and deliver a `.drawio` file or `url` (Mermaid conversion, ELK layout, and image export all need the desktop app). Tell the user they can install the draw.io desktop app to enable those | | Mermaid → PNG export crashes | Direct `.mmd` → PNG with `-e` is broken in current draw.io Desktop (embedded-XML step) | Use the two-step path: convert Mermaid to `.drawio` first (`-f xml`), then export the `.drawio` to PNG — the intermediate file embeds correctly | | Blank diagram from Mermaid | Misspelled type keyword, or a syntax error (bad node ID, unquoted label) | Check the [Mermaid reference](#mermaid-syntax-reference); the first non-directive line's keyword selects the diagram type | +| Layout comes out wrong after a `--layout` pass | An edge's `parent` is not the nearest common ancestor of its `source` and `target` | Run `python3 scripts/fix_edge_parents.py diagram.drawio` before the `--layout` call | | Layout does nothing / errors | Unknown preset name, custom JSON not an array, or a desktop build too old for `--layout` / `.mmd` input | Use a preset from [Layout presets](#layout-presets) or a JSON array starting with `[`; on an old desktop build, author as XML with explicit positions and tell the user updating draw.io Desktop enables Mermaid conversion and layouts | | Export produces empty/corrupt file | Invalid XML (e.g. double hyphens in comments, unescaped special characters) | Validate XML well-formedness before writing; see the XML well-formedness section below | | Diagram opens but looks blank | Missing root cells `id="0"` and `id="1"` | Ensure the basic mxGraphModel structure is complete | diff --git a/plugins/claude-code/skills/drawio/scripts/fix_edge_parents.py b/plugins/claude-code/skills/drawio/scripts/fix_edge_parents.py new file mode 100755 index 0000000..26d31c9 --- /dev/null +++ b/plugins/claude-code/skills/drawio/scripts/fix_edge_parents.py @@ -0,0 +1,660 @@ +#!/usr/bin/env python3 +"""Reparent draw.io edges onto the nearest common ancestor of their endpoints. + +draw.io (mxGraph) expects the ``parent`` of an edge to be the nearest +common ancestor of its ``source`` and ``target`` cells. Generated diagrams +usually put every edge on the default layer (``parent="1"``) instead, which +renders fine but makes the layout passes (Arrange > Layout, ``--layout``) +lay the diagram out wrongly. This script rewrites those ``parent`` +attributes, applying the same rule as ``mxGraphModel.updateEdgeParent``, and +shifts any absolute edge geometry (waypoints, source/target points) so the +edge keeps its on-screen position. + +Usage: + fix_edge_parents.py diagram.drawio # fix in place + fix_edge_parents.py a.drawio b.drawio # fix several files + fix_edge_parents.py --dry-run diagram.drawio # report only, write nothing + cat diagram.drawio | fix_edge_parents.py - # stdin to stdout + +Reads ``.drawio``/``.xml`` files holding either a bare ```` or an +```` (every page is processed, compressed pages included). Everything +except the attributes it has to change is preserved byte for byte. + +Ported from the mxGraph copy bundled with draw.io v31.1.8, specifically +``mxGraphModel.updateEdgeParent`` and the helpers it calls. The paths below +are relative to + + https://github.com/jgraph/drawio/blob/v31.1.8/src/main/webapp/ + + updateEdgeParent mxgraph/src/model/mxGraphModel.js#L832-L885 + getOrigin mxgraph/src/model/mxGraphModel.js#L893-L918 + getNearestCommonAncestor mxgraph/src/model/mxGraphModel.js#L930-L970 + mxGeometry.translate mxgraph/src/model/mxGeometry.js#L299-L337 + +draw.io then overrides one of the flags they read: + + ignoreRelativeEdgeParent = false js/grapheditor/Graph.js#L175 + +Those upstream files carry the following copyright notices: + + mxGraphModel.js: Copyright (c) 2006-2018, JGraph Holdings Ltd + Copyright (c) 2006-2018, draw.io AG + mxGeometry.js: Copyright (c) 2006-2015, JGraph Holdings Ltd + Copyright (c) 2006-2015, draw.io AG + +See https://github.com/jgraph/drawio/blob/v31.1.8/LICENSE for the terms they +are published under. +""" + +# Outline: +# +# 1. Parse the XML document with the Expat parser and record the byte offsets +# of each element. +# 2. Rewrite attributes in the in-memory model. Record updated attributes. +# 3. Update attributes in the source document. + +import argparse +import base64 +import re +import sys +import urllib.parse +import zlib +from xml.parsers import expat + +WRAPPER_ELEMENTS = ("object", "UserObject") + +TAG_NAME_RE = re.compile(rb"<([^\s/>]+)") +ATTRIBUTE_RE = re.compile(rb"\s*([^\s=/>]+)\s*=\s*(\"[^\"]*\"|'[^']*')") +NUMBER_RE = re.compile(r"[+-]?(\d+\.?\d*|\.\d+)([eE][+-]?\d+)?") + +# mxGraph defaults this to true; draw.io turns it off, "Keeps edges between +# relative child cells inside parent" (js/grapheditor/Graph.js). +IGNORE_RELATIVE_EDGE_PARENT = False + + +def is_true(value): + """Convert an attribute value to a boolean. + + Only a missing, empty or numerically zero value is false — every other + spelling (``1``, ``true``, even ``false``) is true. + """ + if value is None or value.strip() == "": + return False + + try: + return float(value) != 0 + except ValueError: + return True + + +def to_float(value): + """Convert an attribute value to a float. + + A missing, unparsable or non-finite value reads as 0. + """ + # mxObjectCodec.convertAttributeFromXml runs parseFloat over the raw + # attribute and substitutes 0 for NaN and infinities. + match = NUMBER_RE.match(value.lstrip()) if value is not None else None + + if match is None: + return 0.0 + + number = float(match.group(0)) + + return number if -float("inf") < number < float("inf") else 0.0 + + +def format_number(value): + """Render a float value like JavaScript.""" + if value == int(value): + return str(int(value)) + + return repr(value) + + +class Node: + """One XML element, with the byte offsets of its tags.""" + + __slots__ = ("attrs", "children", "end", "parent", "start", "tag", "text") + + def __init__(self, tag, attrs, parent, start): + self.tag = tag + self.attrs = attrs + self.parent = parent + self.children = [] + self.start = start + self.end = start + self.text = [] + + def first(self, tag): + """Return the first direct child with the given tag name.""" + for child in self.children: + if child.tag == tag: + return child + + return None + + def walk(self): + """Yield this node and every descendant, in document order.""" + yield self + + for child in self.children: + yield from child.walk() + + +def parse_document(data): + """Parse XML bytes into a Node tree that remembers tag offsets. + + Raises expat.ExpatError unless the document is well formed. + """ + # CurrentByteIndex is what makes the byte-level patching possible: it + # locates each start tag in the original bytes. + # + # > During a callback reporting a parse event they indicate the location + # > of the first of the sequence of characters that generated the event. + # + # https://docs.python.org/3.9/library/pyexpat.html + document = Node("#document", {}, None, -1) + stack = [document] + parser = expat.ParserCreate() + + def start_element(tag, attrs): + node = Node(tag, attrs, stack[-1], parser.CurrentByteIndex) + stack[-1].children.append(node) + stack.append(node) + + def end_element(tag): + stack[-1].end = parser.CurrentByteIndex + stack.pop() + + def character_data(data): + # The text contents is processed later only for . + if stack[-1].tag == "diagram": + stack[-1].text.append(data) + + parser.StartElementHandler = start_element + parser.EndElementHandler = end_element + parser.CharacterDataHandler = character_data + parser.Parse(data, True) + + return document + + +class Cell: + """An mxCell.""" + + __slots__ = ("geometry", "id", "node") + + # Note that ``cell_id`` may be the ``id`` of the wrapper ``object`` or + # ``UserObject``. So we keep it explicitly aside from the ``node``. + def __init__(self, cell_id, node, geometry): + self.id = cell_id + self.node = node + self.geometry = geometry + + +class GraphModel: + """The cells of one ````, with the mxGraph tree helpers.""" + + def __init__(self, root_node): + # Dictionary from id to cell. + self.cells = {} + self.cell_list = [] + + # When a cell is wrapped in ``object`` or ``UserObject``, its id is + # the attribute of the wrapper. + for child in root_node.children: + if child.tag == "mxCell": + node = child + cell_id = child.attrs.get("id") + elif child.tag in WRAPPER_ELEMENTS: + node = child.first("mxCell") + cell_id = child.attrs.get("id") + else: + continue + + if node is None or cell_id is None or cell_id in self.cells: + continue + + cell = Cell(cell_id, node, node.first("mxGeometry")) + self.cells[cell_id] = cell + self.cell_list.append(cell) + + self.root = self._find_root() + + def _find_root(self): + """The model root — the last parentless cell (normally id="0").""" + # mxGraphModelCodec.decodeRoot keeps the last parentless cell it + # decodes as the root. + root = None + + for cell in self.cell_list: + if self.get_parent(cell) is None: + root = cell + + return root + + def get_parent(self, cell): + parent_id = cell.node.attrs.get("parent") + + # mxCodec cannot build a cell that is its own parent, so upstream + # never sees one; here it is read as parentless to keep walks finite. + if parent_id is None or parent_id == cell.id: + return None + + return self.cells.get(parent_id) + + def is_edge(self, cell): + return is_true(cell.node.attrs.get("edge")) + + def ancestors(self, cell): + """The cell itself, then every ancestor up to the root.""" + chain = [] + seen = set() + current = cell + + while current is not None and current.id not in seen: + seen.add(current.id) + chain.append(current) + current = self.get_parent(current) + + return chain + + def is_ancestor(self, parent, child): + return any(cell is parent for cell in self.ancestors(child)) + + def get_nearest_common_ancestor(self, cell1, cell2): + """The deepest cell that is an ancestor of both. + + A candidate whose own parent is null (the model root) is not + returned, matching getNearestCommonAncestor. + """ + path = {cell.id for cell in self.ancestors(cell2)} + + for cell in self.ancestors(cell1): + if cell.id in path: + return cell if self.get_parent(cell) is not None else None + + return None + + def get_origin(self, cell): + """Absolute offset of a cell's coordinates (mxGraph getOrigin).""" + # mxGraphModel.getOrigin recurses up to the root; we use an iteration + # instead. + x = 0.0 + y = 0.0 + + for current in self.ancestors(cell): + if self.is_edge(current) or current.geometry is None: + continue + + x += to_float(current.geometry.attrs.get("x")) + y += to_float(current.geometry.attrs.get("y")) + + return x, y + + def get_terminal(self, edge, is_source): + """The cell an edge points at, before any relative resolution. + + If is_source is True, return the source cell. Return the target cell + otherwise. + """ + attribute = "source" if is_source else "target" + + return self.cells.get(edge.node.attrs.get(attribute)) + + +def set_attribute(node, name, value, edits): + """Record an attribute change and apply it to the in-memory model.""" + node.attrs[name] = value + edits.append((node, name, value)) + + +def translate_point(node, dx, dy, edits): + x = to_float(node.attrs.get("x")) + dx + y = to_float(node.attrs.get("y")) + dy + set_attribute(node, "x", format_number(x), edits) + set_attribute(node, "y", format_number(y), edits) + + +def translate(geo, dx, dy, edits): + """Shift an edge geometry, mirroring mxGeometry.translate.""" + # mxGeometry.translate has no such early return. Skipping a zero delta + # keeps the file byte-identical when both origins coincide. + if dx == 0 and dy == 0: + return + + # Translates the geometry + if not is_true(geo.attrs.get("relative")): + translate_point(geo, dx, dy, edits) + + for child in geo.children: + tag = child.tag + role = child.attrs.get("as") + + # Translates the source and target points + if tag == "mxPoint" and role in ("sourcePoint", "targetPoint"): + translate_point(child, dx, dy, edits) + elif tag == "Array" and role == "points": + # Translate the control points + for point in child.children: + if point.tag == "mxPoint": + translate_point(point, dx, dy, edits) + + +def update_edge_parent(model, edge, root, edits): + """Move one edge onto the nearest common ancestor of its terminals. + + Returns an (edge id, old parent id, new parent id) tuple when the edge + was moved, otherwise None. + """ + # Transcribed statement by statement from mxGraphModel.updateEdgeParent. + # The `seen` sets are the one addition: they stop the terminal walks on a + # parent cycle, which the JavaScript loops on forever. + source = model.get_terminal(edge, True) + target = model.get_terminal(edge, False) + seen = set() + + # Uses the first non-relative descendants of the source terminal + while ( + source is not None + and source.id not in seen + and not model.is_edge(source) + and source.geometry is not None + and is_true(source.geometry.attrs.get("relative")) + ): + seen.add(source.id) + source = model.get_parent(source) + + seen = set() + + # Uses the first non-relative descendants of the target terminal + while ( + target is not None + and target.id not in seen + and IGNORE_RELATIVE_EDGE_PARENT + and not model.is_edge(target) + and target.geometry is not None + and is_true(target.geometry.attrs.get("relative")) + ): + seen.add(target.id) + target = model.get_parent(target) + + if not ( + model.is_ancestor(root, source) and model.is_ancestor(root, target) + ): + return None + + if source is target: + cell = model.get_parent(source) + else: + cell = model.get_nearest_common_ancestor(source, target) + + if ( + cell is None + or ( + model.get_parent(cell) is model.root + and not model.is_ancestor(cell, edge) + ) + or model.get_parent(edge) is cell + ): + return None + + old_parent = model.get_parent(edge) + geo = edge.geometry + + if geo is not None: + origin1 = model.get_origin(old_parent) + origin2 = model.get_origin(cell) + + dx = origin2[0] - origin1[0] + dy = origin2[1] - origin1[1] + + translate(geo, -dx, -dy, edits) + + set_attribute(edge.node, "parent", cell.id, edits) + + return ( + edge.id, + old_parent.id if old_parent is not None else None, + cell.id, + ) + + +def fix_model(model, edits): + """Reparent every edge of one model. + + Returns a list of (edge id, old, new) tuples. + """ + changes = [] + root = model.root + + if root is None: + return changes + + for edge in model.cell_list: + if not model.is_edge(edge) or not model.is_ancestor(root, edge): + continue + + change = update_edge_parent(model, edge, root, edits) + + if change is not None: + changes.append(change) + + return changes + + +def escape_attribute(value): + return ( + value.replace("&", "&") + .replace("<", "<") + .replace(">", ">") + .replace('"', """) + ) + + +def scan_start_tag(data, start): + """Take apart the start tag at `start`. + + Returns the offset just past its ``>``, the value span of each attribute, + and the offset at which a new attribute can be inserted. + """ + # The tag comes from a completed parse, so its syntax is known good. + position = TAG_NAME_RE.match(data, start).end() + values = {} + + while True: + attribute = ATTRIBUTE_RE.match(data, position) + + if attribute is None: + break + + span = (attribute.start(2) + 1, attribute.end(2) - 1) + values[attribute.group(1)] = span + position = attribute.end() + + # Only whitespace and an optional '/' can precede the closing '>' here. + return data.index(b">", position) + 1, values, position + + +def build_attribute_patches(data, edits): + """Turn recorded attribute edits into (start, end, replacement) patches.""" + grouped = {} + + for node, name, value in edits: + grouped.setdefault(id(node), (node, []))[1].append((name, value)) + + patches = [] + + for node, attribute_edits in grouped.values(): + values, insert_at = scan_start_tag(data, node.start)[1:] + added = b"" + + for name, value in attribute_edits: + encoded_name = name.encode("utf-8") + encoded_value = escape_attribute(value).encode("utf-8") + span = values.get(encoded_name) + + if span is None: + added += b" " + encoded_name + b'="' + encoded_value + b'"' + else: + patches.append((span[0], span[1], encoded_value)) + + if added: + patches.append((insert_at, insert_at, added)) + + return patches + + +def apply_patches(data, patches): + for start, end, replacement in sorted(patches, reverse=True): + data = data[:start] + replacement + data[end:] + + return data + + +def decode_diagram(text): + """Decode draw.io's deflateRaw + base64 page payload, or None.""" + try: + inflated = zlib.decompress(base64.b64decode(text, validate=True), -15) + + return urllib.parse.unquote(inflated.decode("utf-8")) + except (ValueError, zlib.error, UnicodeDecodeError): + return None + + +def encode_diagram(xml): + """Re-encode a page payload the way draw.io writes it.""" + compressor = zlib.compressobj(9, zlib.DEFLATED, -15) + quoted = urllib.parse.quote(xml, safe="!~*'()").encode("utf-8") + deflated = compressor.compress(quoted) + compressor.flush() + + return base64.b64encode(deflated) + + +def fix_xml(data): + """Fix every model in an XML document; returns (new bytes, changes).""" + # Patching by byte offset assumes markup is one byte per character, which + # holds for UTF-8 and the single-byte encodings but not for UTF-16/32. + if data.startswith((b"\xff\xfe", b"\xfe\xff")) or b"\x00" in data[:4]: + raise ValueError( + "UTF-16/UTF-32 is not supported; save the file as UTF-8" + ) + + document = parse_document(data) + # Three collections travel through the run, none of them from mxGraph: + # edits (node, attribute, value) — every attribute the model layer + # wants rewritten, filled in by update_edge_parent + # changes (edge id, old parent id, new parent id) — the same work as + # seen by the caller, which is what gets reported + # patches (start, end, bytes) — byte spans of `data` to splice, built + # from `edits` plus any page re-encoded after a nested fix + edits = [] + changes = [] + patches = [] + + for node in document.walk(): + if node.tag == "mxGraphModel": + root_node = node.first("root") + + if root_node is not None: + changes.extend(fix_model(GraphModel(root_node), edits)) + elif node.tag == "diagram" and node.first("mxGraphModel") is None: + payload = "".join(node.text).strip() + + if not payload: + continue + + inner = decode_diagram(payload) + + if inner is None: + continue + + fixed, inner_changes = fix_xml(inner.encode("utf-8")) + + if inner_changes: + changes.extend(inner_changes) + # Replace the whole content. This replaces any comments and + # other non-text contents in the element. + start = scan_start_tag(data, node.start)[0] + payload = encode_diagram(fixed.decode("utf-8")) + patches.append((start, node.end, payload)) + + patches.extend(build_attribute_patches(data, edits)) + + return apply_patches(data, patches), changes + + +def report(label, changes, stream): + if not changes: + stream.write(f"{label}: no edge parents to fix\n") + + return + + stream.write(f"{label}: {len(changes)} edge parent(s) fixed\n") + + for edge_id, old_parent, new_parent in changes: + old = "" if old_parent is None else old_parent + stream.write(f' {edge_id}: parent="{old}" -> parent="{new_parent}"\n') + + +def process(path, options): + """Fix one file (or stdin when path is '-'); returns the change count.""" + if path == "-": + data = sys.stdin.buffer.read() + else: + with open(path, "rb") as handle: + data = handle.read() + + fixed, changes = fix_xml(data) + + if path == "-": + sys.stdout.buffer.write(data if options.dry_run else fixed) + elif changes and not options.dry_run: + with open(path, "wb") as handle: + handle.write(fixed) + + if not options.quiet: + report("" if path == "-" else path, changes, sys.stderr) + + return len(changes) + + +def main(argv=None): + parser = argparse.ArgumentParser( + description="Set every draw.io edge's parent to the nearest common " + "ancestor of its source and target." + ) + parser.add_argument( + "files", + metavar="FILE", + nargs="+", + help=".drawio/.xml file to fix in place ('-' for stdin to stdout)", + ) + parser.add_argument( + "-n", + "--dry-run", + action="store_true", + help="report what would change without writing files", + ) + parser.add_argument( + "-q", + "--quiet", + action="store_true", + help="suppress the per-file report", + ) + options = parser.parse_args(argv) + status = 0 + + for path in options.files: + try: + process(path, options) + except (OSError, ValueError, expat.ExpatError) as error: + sys.stderr.write(f"{path}: {error}\n") + status = 1 + + return status + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/plugins/codex/drawio/DEVELOPING.md b/plugins/codex/drawio/DEVELOPING.md index d33def2..a0a9307 100644 --- a/plugins/codex/drawio/DEVELOPING.md +++ b/plugins/codex/drawio/DEVELOPING.md @@ -8,9 +8,9 @@ No MCP server required. This is the Codex counterpart of the [Claude Code plugin](../../claude-code/README.md). The skill body is host-agnostic — it drives the draw.io Desktop CLI directly — so -`skills/drawio/SKILL.md` is kept **byte-identical** to the Claude plugin's `SKILL.md` -(including the `/drawio:drawio` invocation examples, which Codex uses too). Only the host -wrapping differs: the manifest schema (`.codex-plugin/plugin.json` vs `.claude-plugin/plugin.json`), +`skills/drawio/SKILL.md` and the helper it ships (`skills/drawio/scripts/fix_edge_parents.py`) +are kept **byte-identical** to the Claude plugin's copies (including the `/drawio:drawio` +invocation examples, which Codex uses too). Only the host wrapping differs: the manifest schema (`.codex-plugin/plugin.json` vs `.claude-plugin/plugin.json`), the `interface` block (Codex-only — logo, brand color, default prompts), and the marketplace format. @@ -20,6 +20,7 @@ format. |------|---------| | `.codex-plugin/plugin.json` | Codex plugin manifest — name, version, description, author, license, and the `interface` block (display name, logo, `brandColor`, default prompts) | | `skills/drawio/SKILL.md` | The skill itself (its folder name `drawio` becomes the second half of the `/drawio:drawio` invocation); byte-identical to the Claude plugin's copy | +| `skills/drawio/scripts/fix_edge_parents.py` | Bundled helper the skill runs on every generated `.drawio` — sets each edge's `parent` to the nearest common ancestor of its `source` and `target`, which a layout pass needs in order to lay the diagram out correctly; byte-identical to the Claude plugin's copy (documented in [its DEVELOPING.md](../../claude-code/DEVELOPING.md#edge-parents)) | | `assets/drawio-logo.svg` | Official draw.io logo (vector `drawio-desktop` icon), referenced by `interface.composerIcon`/`logo`/`logoDark` | | `README.md` | Installation and usage documentation | | `../../../.agents/plugins/marketplace.json` | Codex marketplace manifest at the repo root; lists this plugin with `source.path: "./plugins/codex/drawio"` and inherits the rest of its metadata from `plugin.json` | @@ -36,6 +37,7 @@ plugins/codex/ ← host group directory └── drawio/ ← Codex plugin root (folder name == plugin.json "name" = "drawio") ├── .codex-plugin/plugin.json ├── skills/drawio/SKILL.md + ├── skills/drawio/scripts/fix_edge_parents.py ├── assets/drawio-logo.svg ├── README.md └── DEVELOPING.md diff --git a/plugins/codex/drawio/README.md b/plugins/codex/drawio/README.md index 9ac00a7..44813c1 100644 --- a/plugins/codex/drawio/README.md +++ b/plugins/codex/drawio/README.md @@ -20,13 +20,16 @@ When you ask Codex to create a diagram, it will: present 2. Produce a native `.drawio` file — convert the Mermaid with the desktop CLI, or write the XML directly (optionally running an ELK `--layout` pass so you don't hand-place cells) -3. Handle the requested output: +3. Run the bundled `fix_edge_parents.py` over the file so every connector is parented to the + innermost container holding both of its endpoints — the rule the draw.io editor applies, + and what keeps a layout pass from laying the diagram out wrongly +4. Handle the requested output: - PNG / SVG / PDF — export using the draw.io desktop CLI - `url` — compress the XML with Node.js's built-in `zlib` and open `https://app.diagrams.net/#create=...` in your browser (keeps the `.drawio` file as a local copy) - *(default)* — leave the `.drawio` file as-is -4. Open the result +5. Open the result ## Prerequisites @@ -34,6 +37,8 @@ When you ask Codex to create a diagram, it will: - [draw.io Desktop](https://github.com/jgraph/drawio-desktop/releases) installed — required for Mermaid conversion, ELK layout, and PNG/SVG/PDF export. Not needed for plain XML `.drawio` or `url` output, which Codex can produce with no desktop app +- Python 3 on your `PATH` — used by the bundled edge-parent fixer (standard library only, no + packages to install). Without it Codex sets the edge parents itself while authoring ## Installation diff --git a/plugins/codex/drawio/skills/drawio/SKILL.md b/plugins/codex/drawio/skills/drawio/SKILL.md index 54ead51..1d0ab04 100644 --- a/plugins/codex/drawio/skills/drawio/SKILL.md +++ b/plugins/codex/drawio/skills/drawio/SKILL.md @@ -33,23 +33,43 @@ Every diagram becomes a native `.drawio` file first, then is delivered in the re ``` Delete the `.mmd` afterward — the `.drawio` is the artifact. draw.io's Mermaid parser has already laid the diagram out, so no `--layout` is needed. - **XML**: write the mxGraphModel XML to `diagram.drawio` (see [XML format](#xml-format)). Optionally apply an ELK layout (see [ELK layout for XML](#elk-layout-for-xml)). -2. **Deliver** (identical for both sources): +2. **Fix edge parents** — see [Edge parents](#edge-parents): + ```bash + python3 scripts/fix_edge_parents.py diagram.drawio + ``` +3. **Deliver** (identical for both sources): - *(no format)* → keep `diagram.drawio` and open it. - **png / svg / pdf** → export from the `.drawio` with embedded XML, then delete the source `.drawio`: ```bash drawio -x -f png -e -b 10 -o diagram.drawio.png diagram.drawio ``` - **url** → build a browser URL from the `.drawio` XML, open it, and keep the `.drawio` as a local copy (see [Browser URL output](#browser-url-output)). -3. **Open the result** — the exported file, the URL, or the `.drawio`. If the open command fails, print the absolute path (or URL) so the user can open it manually. +4. **Open the result** — the exported file, the URL, or the `.drawio`. If the open command fails, print the absolute path (or URL) so the user can open it manually. **Always convert Mermaid to `.drawio` first, then export** — do not export a `.mmd` straight to an image. Direct Mermaid → PNG export with `-e` is broken in current draw.io Desktop (the embedded-XML step crashes); the two-step path (convert, then export the `.drawio`) is reliable and produces an editable embed. See [Troubleshooting](#troubleshooting). If Mermaid was requested but no desktop CLI is available, fall back to authoring the same diagram directly as XML. +## Edge parents + +An edge's `parent` must be the **nearest common ancestor** of its `source` and `target` cells — the innermost container holding both endpoints, or the layer when there is none. Otherwise a layout pass lays the diagram out wrongly. This overrides the "cross-container edges use `parent="1"`" advice in the [XML reference](#xml-reference). + +Don't work it out by hand — a script bundled with this skill fixes every edge in place: + +```bash +python3 scripts/fix_edge_parents.py diagram.drawio +``` + +- The path is relative to this `SKILL.md`; use the absolute path from another working directory. +- Run it on **every** `.drawio` — Mermaid output too, since `subgraph` blocks become containers — and **before** any `--layout` pass. Running it twice is a no-op. +- If `python3` is unavailable, set each edge's `parent` to the nearest common ancestor while authoring instead. + ## ELK layout for XML XML-authored diagrams can be auto-positioned by the CLI's `--layout` pass — the same ELK layouts as the editor's *Arrange ▸ Layout* menu and the same engine the draw.io MCP app server uses. Generate the cells with approximate (or even `0,0`) positions and let ELK place them; you only have to get the graph *structure* — nodes and edges — right. +Run `python3 scripts/fix_edge_parents.py diagram.drawio` first — see [Edge parents](#edge-parents). + Add `--layout ` to any CLI call that reads your XML. The simplest form lays out in place after you write the file (reading and overwriting the same path is supported): ```bash @@ -359,6 +379,7 @@ Every diagram must have this structure: - Cell `id="0"` is the root layer - Cell `id="1"` is the default parent layer - All diagram elements use `parent="1"` unless using multiple layers +- An edge's `parent` is the nearest common ancestor of its `source` and `target`, not necessarily `1` — see [Edge parents](#edge-parents) (The example above uses an XML comment only to point out where cells go — never emit comments in real output; see [XML well-formedness](#critical-xml-well-formedness).) @@ -374,6 +395,7 @@ https://raw.githubusercontent.com/jgraph/drawio-mcp/main/shared/xml-reference.md | draw.io CLI not found | Desktop app not installed or not on PATH | Author as XML and deliver a `.drawio` file or `url` (Mermaid conversion, ELK layout, and image export all need the desktop app). Tell the user they can install the draw.io desktop app to enable those | | Mermaid → PNG export crashes | Direct `.mmd` → PNG with `-e` is broken in current draw.io Desktop (embedded-XML step) | Use the two-step path: convert Mermaid to `.drawio` first (`-f xml`), then export the `.drawio` to PNG — the intermediate file embeds correctly | | Blank diagram from Mermaid | Misspelled type keyword, or a syntax error (bad node ID, unquoted label) | Check the [Mermaid reference](#mermaid-syntax-reference); the first non-directive line's keyword selects the diagram type | +| Layout comes out wrong after a `--layout` pass | An edge's `parent` is not the nearest common ancestor of its `source` and `target` | Run `python3 scripts/fix_edge_parents.py diagram.drawio` before the `--layout` call | | Layout does nothing / errors | Unknown preset name, custom JSON not an array, or a desktop build too old for `--layout` / `.mmd` input | Use a preset from [Layout presets](#layout-presets) or a JSON array starting with `[`; on an old desktop build, author as XML with explicit positions and tell the user updating draw.io Desktop enables Mermaid conversion and layouts | | Export produces empty/corrupt file | Invalid XML (e.g. double hyphens in comments, unescaped special characters) | Validate XML well-formedness before writing; see the XML well-formedness section below | | Diagram opens but looks blank | Missing root cells `id="0"` and `id="1"` | Ensure the basic mxGraphModel structure is complete | diff --git a/plugins/codex/drawio/skills/drawio/scripts/fix_edge_parents.py b/plugins/codex/drawio/skills/drawio/scripts/fix_edge_parents.py new file mode 100755 index 0000000..26d31c9 --- /dev/null +++ b/plugins/codex/drawio/skills/drawio/scripts/fix_edge_parents.py @@ -0,0 +1,660 @@ +#!/usr/bin/env python3 +"""Reparent draw.io edges onto the nearest common ancestor of their endpoints. + +draw.io (mxGraph) expects the ``parent`` of an edge to be the nearest +common ancestor of its ``source`` and ``target`` cells. Generated diagrams +usually put every edge on the default layer (``parent="1"``) instead, which +renders fine but makes the layout passes (Arrange > Layout, ``--layout``) +lay the diagram out wrongly. This script rewrites those ``parent`` +attributes, applying the same rule as ``mxGraphModel.updateEdgeParent``, and +shifts any absolute edge geometry (waypoints, source/target points) so the +edge keeps its on-screen position. + +Usage: + fix_edge_parents.py diagram.drawio # fix in place + fix_edge_parents.py a.drawio b.drawio # fix several files + fix_edge_parents.py --dry-run diagram.drawio # report only, write nothing + cat diagram.drawio | fix_edge_parents.py - # stdin to stdout + +Reads ``.drawio``/``.xml`` files holding either a bare ```` or an +```` (every page is processed, compressed pages included). Everything +except the attributes it has to change is preserved byte for byte. + +Ported from the mxGraph copy bundled with draw.io v31.1.8, specifically +``mxGraphModel.updateEdgeParent`` and the helpers it calls. The paths below +are relative to + + https://github.com/jgraph/drawio/blob/v31.1.8/src/main/webapp/ + + updateEdgeParent mxgraph/src/model/mxGraphModel.js#L832-L885 + getOrigin mxgraph/src/model/mxGraphModel.js#L893-L918 + getNearestCommonAncestor mxgraph/src/model/mxGraphModel.js#L930-L970 + mxGeometry.translate mxgraph/src/model/mxGeometry.js#L299-L337 + +draw.io then overrides one of the flags they read: + + ignoreRelativeEdgeParent = false js/grapheditor/Graph.js#L175 + +Those upstream files carry the following copyright notices: + + mxGraphModel.js: Copyright (c) 2006-2018, JGraph Holdings Ltd + Copyright (c) 2006-2018, draw.io AG + mxGeometry.js: Copyright (c) 2006-2015, JGraph Holdings Ltd + Copyright (c) 2006-2015, draw.io AG + +See https://github.com/jgraph/drawio/blob/v31.1.8/LICENSE for the terms they +are published under. +""" + +# Outline: +# +# 1. Parse the XML document with the Expat parser and record the byte offsets +# of each element. +# 2. Rewrite attributes in the in-memory model. Record updated attributes. +# 3. Update attributes in the source document. + +import argparse +import base64 +import re +import sys +import urllib.parse +import zlib +from xml.parsers import expat + +WRAPPER_ELEMENTS = ("object", "UserObject") + +TAG_NAME_RE = re.compile(rb"<([^\s/>]+)") +ATTRIBUTE_RE = re.compile(rb"\s*([^\s=/>]+)\s*=\s*(\"[^\"]*\"|'[^']*')") +NUMBER_RE = re.compile(r"[+-]?(\d+\.?\d*|\.\d+)([eE][+-]?\d+)?") + +# mxGraph defaults this to true; draw.io turns it off, "Keeps edges between +# relative child cells inside parent" (js/grapheditor/Graph.js). +IGNORE_RELATIVE_EDGE_PARENT = False + + +def is_true(value): + """Convert an attribute value to a boolean. + + Only a missing, empty or numerically zero value is false — every other + spelling (``1``, ``true``, even ``false``) is true. + """ + if value is None or value.strip() == "": + return False + + try: + return float(value) != 0 + except ValueError: + return True + + +def to_float(value): + """Convert an attribute value to a float. + + A missing, unparsable or non-finite value reads as 0. + """ + # mxObjectCodec.convertAttributeFromXml runs parseFloat over the raw + # attribute and substitutes 0 for NaN and infinities. + match = NUMBER_RE.match(value.lstrip()) if value is not None else None + + if match is None: + return 0.0 + + number = float(match.group(0)) + + return number if -float("inf") < number < float("inf") else 0.0 + + +def format_number(value): + """Render a float value like JavaScript.""" + if value == int(value): + return str(int(value)) + + return repr(value) + + +class Node: + """One XML element, with the byte offsets of its tags.""" + + __slots__ = ("attrs", "children", "end", "parent", "start", "tag", "text") + + def __init__(self, tag, attrs, parent, start): + self.tag = tag + self.attrs = attrs + self.parent = parent + self.children = [] + self.start = start + self.end = start + self.text = [] + + def first(self, tag): + """Return the first direct child with the given tag name.""" + for child in self.children: + if child.tag == tag: + return child + + return None + + def walk(self): + """Yield this node and every descendant, in document order.""" + yield self + + for child in self.children: + yield from child.walk() + + +def parse_document(data): + """Parse XML bytes into a Node tree that remembers tag offsets. + + Raises expat.ExpatError unless the document is well formed. + """ + # CurrentByteIndex is what makes the byte-level patching possible: it + # locates each start tag in the original bytes. + # + # > During a callback reporting a parse event they indicate the location + # > of the first of the sequence of characters that generated the event. + # + # https://docs.python.org/3.9/library/pyexpat.html + document = Node("#document", {}, None, -1) + stack = [document] + parser = expat.ParserCreate() + + def start_element(tag, attrs): + node = Node(tag, attrs, stack[-1], parser.CurrentByteIndex) + stack[-1].children.append(node) + stack.append(node) + + def end_element(tag): + stack[-1].end = parser.CurrentByteIndex + stack.pop() + + def character_data(data): + # The text contents is processed later only for . + if stack[-1].tag == "diagram": + stack[-1].text.append(data) + + parser.StartElementHandler = start_element + parser.EndElementHandler = end_element + parser.CharacterDataHandler = character_data + parser.Parse(data, True) + + return document + + +class Cell: + """An mxCell.""" + + __slots__ = ("geometry", "id", "node") + + # Note that ``cell_id`` may be the ``id`` of the wrapper ``object`` or + # ``UserObject``. So we keep it explicitly aside from the ``node``. + def __init__(self, cell_id, node, geometry): + self.id = cell_id + self.node = node + self.geometry = geometry + + +class GraphModel: + """The cells of one ````, with the mxGraph tree helpers.""" + + def __init__(self, root_node): + # Dictionary from id to cell. + self.cells = {} + self.cell_list = [] + + # When a cell is wrapped in ``object`` or ``UserObject``, its id is + # the attribute of the wrapper. + for child in root_node.children: + if child.tag == "mxCell": + node = child + cell_id = child.attrs.get("id") + elif child.tag in WRAPPER_ELEMENTS: + node = child.first("mxCell") + cell_id = child.attrs.get("id") + else: + continue + + if node is None or cell_id is None or cell_id in self.cells: + continue + + cell = Cell(cell_id, node, node.first("mxGeometry")) + self.cells[cell_id] = cell + self.cell_list.append(cell) + + self.root = self._find_root() + + def _find_root(self): + """The model root — the last parentless cell (normally id="0").""" + # mxGraphModelCodec.decodeRoot keeps the last parentless cell it + # decodes as the root. + root = None + + for cell in self.cell_list: + if self.get_parent(cell) is None: + root = cell + + return root + + def get_parent(self, cell): + parent_id = cell.node.attrs.get("parent") + + # mxCodec cannot build a cell that is its own parent, so upstream + # never sees one; here it is read as parentless to keep walks finite. + if parent_id is None or parent_id == cell.id: + return None + + return self.cells.get(parent_id) + + def is_edge(self, cell): + return is_true(cell.node.attrs.get("edge")) + + def ancestors(self, cell): + """The cell itself, then every ancestor up to the root.""" + chain = [] + seen = set() + current = cell + + while current is not None and current.id not in seen: + seen.add(current.id) + chain.append(current) + current = self.get_parent(current) + + return chain + + def is_ancestor(self, parent, child): + return any(cell is parent for cell in self.ancestors(child)) + + def get_nearest_common_ancestor(self, cell1, cell2): + """The deepest cell that is an ancestor of both. + + A candidate whose own parent is null (the model root) is not + returned, matching getNearestCommonAncestor. + """ + path = {cell.id for cell in self.ancestors(cell2)} + + for cell in self.ancestors(cell1): + if cell.id in path: + return cell if self.get_parent(cell) is not None else None + + return None + + def get_origin(self, cell): + """Absolute offset of a cell's coordinates (mxGraph getOrigin).""" + # mxGraphModel.getOrigin recurses up to the root; we use an iteration + # instead. + x = 0.0 + y = 0.0 + + for current in self.ancestors(cell): + if self.is_edge(current) or current.geometry is None: + continue + + x += to_float(current.geometry.attrs.get("x")) + y += to_float(current.geometry.attrs.get("y")) + + return x, y + + def get_terminal(self, edge, is_source): + """The cell an edge points at, before any relative resolution. + + If is_source is True, return the source cell. Return the target cell + otherwise. + """ + attribute = "source" if is_source else "target" + + return self.cells.get(edge.node.attrs.get(attribute)) + + +def set_attribute(node, name, value, edits): + """Record an attribute change and apply it to the in-memory model.""" + node.attrs[name] = value + edits.append((node, name, value)) + + +def translate_point(node, dx, dy, edits): + x = to_float(node.attrs.get("x")) + dx + y = to_float(node.attrs.get("y")) + dy + set_attribute(node, "x", format_number(x), edits) + set_attribute(node, "y", format_number(y), edits) + + +def translate(geo, dx, dy, edits): + """Shift an edge geometry, mirroring mxGeometry.translate.""" + # mxGeometry.translate has no such early return. Skipping a zero delta + # keeps the file byte-identical when both origins coincide. + if dx == 0 and dy == 0: + return + + # Translates the geometry + if not is_true(geo.attrs.get("relative")): + translate_point(geo, dx, dy, edits) + + for child in geo.children: + tag = child.tag + role = child.attrs.get("as") + + # Translates the source and target points + if tag == "mxPoint" and role in ("sourcePoint", "targetPoint"): + translate_point(child, dx, dy, edits) + elif tag == "Array" and role == "points": + # Translate the control points + for point in child.children: + if point.tag == "mxPoint": + translate_point(point, dx, dy, edits) + + +def update_edge_parent(model, edge, root, edits): + """Move one edge onto the nearest common ancestor of its terminals. + + Returns an (edge id, old parent id, new parent id) tuple when the edge + was moved, otherwise None. + """ + # Transcribed statement by statement from mxGraphModel.updateEdgeParent. + # The `seen` sets are the one addition: they stop the terminal walks on a + # parent cycle, which the JavaScript loops on forever. + source = model.get_terminal(edge, True) + target = model.get_terminal(edge, False) + seen = set() + + # Uses the first non-relative descendants of the source terminal + while ( + source is not None + and source.id not in seen + and not model.is_edge(source) + and source.geometry is not None + and is_true(source.geometry.attrs.get("relative")) + ): + seen.add(source.id) + source = model.get_parent(source) + + seen = set() + + # Uses the first non-relative descendants of the target terminal + while ( + target is not None + and target.id not in seen + and IGNORE_RELATIVE_EDGE_PARENT + and not model.is_edge(target) + and target.geometry is not None + and is_true(target.geometry.attrs.get("relative")) + ): + seen.add(target.id) + target = model.get_parent(target) + + if not ( + model.is_ancestor(root, source) and model.is_ancestor(root, target) + ): + return None + + if source is target: + cell = model.get_parent(source) + else: + cell = model.get_nearest_common_ancestor(source, target) + + if ( + cell is None + or ( + model.get_parent(cell) is model.root + and not model.is_ancestor(cell, edge) + ) + or model.get_parent(edge) is cell + ): + return None + + old_parent = model.get_parent(edge) + geo = edge.geometry + + if geo is not None: + origin1 = model.get_origin(old_parent) + origin2 = model.get_origin(cell) + + dx = origin2[0] - origin1[0] + dy = origin2[1] - origin1[1] + + translate(geo, -dx, -dy, edits) + + set_attribute(edge.node, "parent", cell.id, edits) + + return ( + edge.id, + old_parent.id if old_parent is not None else None, + cell.id, + ) + + +def fix_model(model, edits): + """Reparent every edge of one model. + + Returns a list of (edge id, old, new) tuples. + """ + changes = [] + root = model.root + + if root is None: + return changes + + for edge in model.cell_list: + if not model.is_edge(edge) or not model.is_ancestor(root, edge): + continue + + change = update_edge_parent(model, edge, root, edits) + + if change is not None: + changes.append(change) + + return changes + + +def escape_attribute(value): + return ( + value.replace("&", "&") + .replace("<", "<") + .replace(">", ">") + .replace('"', """) + ) + + +def scan_start_tag(data, start): + """Take apart the start tag at `start`. + + Returns the offset just past its ``>``, the value span of each attribute, + and the offset at which a new attribute can be inserted. + """ + # The tag comes from a completed parse, so its syntax is known good. + position = TAG_NAME_RE.match(data, start).end() + values = {} + + while True: + attribute = ATTRIBUTE_RE.match(data, position) + + if attribute is None: + break + + span = (attribute.start(2) + 1, attribute.end(2) - 1) + values[attribute.group(1)] = span + position = attribute.end() + + # Only whitespace and an optional '/' can precede the closing '>' here. + return data.index(b">", position) + 1, values, position + + +def build_attribute_patches(data, edits): + """Turn recorded attribute edits into (start, end, replacement) patches.""" + grouped = {} + + for node, name, value in edits: + grouped.setdefault(id(node), (node, []))[1].append((name, value)) + + patches = [] + + for node, attribute_edits in grouped.values(): + values, insert_at = scan_start_tag(data, node.start)[1:] + added = b"" + + for name, value in attribute_edits: + encoded_name = name.encode("utf-8") + encoded_value = escape_attribute(value).encode("utf-8") + span = values.get(encoded_name) + + if span is None: + added += b" " + encoded_name + b'="' + encoded_value + b'"' + else: + patches.append((span[0], span[1], encoded_value)) + + if added: + patches.append((insert_at, insert_at, added)) + + return patches + + +def apply_patches(data, patches): + for start, end, replacement in sorted(patches, reverse=True): + data = data[:start] + replacement + data[end:] + + return data + + +def decode_diagram(text): + """Decode draw.io's deflateRaw + base64 page payload, or None.""" + try: + inflated = zlib.decompress(base64.b64decode(text, validate=True), -15) + + return urllib.parse.unquote(inflated.decode("utf-8")) + except (ValueError, zlib.error, UnicodeDecodeError): + return None + + +def encode_diagram(xml): + """Re-encode a page payload the way draw.io writes it.""" + compressor = zlib.compressobj(9, zlib.DEFLATED, -15) + quoted = urllib.parse.quote(xml, safe="!~*'()").encode("utf-8") + deflated = compressor.compress(quoted) + compressor.flush() + + return base64.b64encode(deflated) + + +def fix_xml(data): + """Fix every model in an XML document; returns (new bytes, changes).""" + # Patching by byte offset assumes markup is one byte per character, which + # holds for UTF-8 and the single-byte encodings but not for UTF-16/32. + if data.startswith((b"\xff\xfe", b"\xfe\xff")) or b"\x00" in data[:4]: + raise ValueError( + "UTF-16/UTF-32 is not supported; save the file as UTF-8" + ) + + document = parse_document(data) + # Three collections travel through the run, none of them from mxGraph: + # edits (node, attribute, value) — every attribute the model layer + # wants rewritten, filled in by update_edge_parent + # changes (edge id, old parent id, new parent id) — the same work as + # seen by the caller, which is what gets reported + # patches (start, end, bytes) — byte spans of `data` to splice, built + # from `edits` plus any page re-encoded after a nested fix + edits = [] + changes = [] + patches = [] + + for node in document.walk(): + if node.tag == "mxGraphModel": + root_node = node.first("root") + + if root_node is not None: + changes.extend(fix_model(GraphModel(root_node), edits)) + elif node.tag == "diagram" and node.first("mxGraphModel") is None: + payload = "".join(node.text).strip() + + if not payload: + continue + + inner = decode_diagram(payload) + + if inner is None: + continue + + fixed, inner_changes = fix_xml(inner.encode("utf-8")) + + if inner_changes: + changes.extend(inner_changes) + # Replace the whole content. This replaces any comments and + # other non-text contents in the element. + start = scan_start_tag(data, node.start)[0] + payload = encode_diagram(fixed.decode("utf-8")) + patches.append((start, node.end, payload)) + + patches.extend(build_attribute_patches(data, edits)) + + return apply_patches(data, patches), changes + + +def report(label, changes, stream): + if not changes: + stream.write(f"{label}: no edge parents to fix\n") + + return + + stream.write(f"{label}: {len(changes)} edge parent(s) fixed\n") + + for edge_id, old_parent, new_parent in changes: + old = "" if old_parent is None else old_parent + stream.write(f' {edge_id}: parent="{old}" -> parent="{new_parent}"\n') + + +def process(path, options): + """Fix one file (or stdin when path is '-'); returns the change count.""" + if path == "-": + data = sys.stdin.buffer.read() + else: + with open(path, "rb") as handle: + data = handle.read() + + fixed, changes = fix_xml(data) + + if path == "-": + sys.stdout.buffer.write(data if options.dry_run else fixed) + elif changes and not options.dry_run: + with open(path, "wb") as handle: + handle.write(fixed) + + if not options.quiet: + report("" if path == "-" else path, changes, sys.stderr) + + return len(changes) + + +def main(argv=None): + parser = argparse.ArgumentParser( + description="Set every draw.io edge's parent to the nearest common " + "ancestor of its source and target." + ) + parser.add_argument( + "files", + metavar="FILE", + nargs="+", + help=".drawio/.xml file to fix in place ('-' for stdin to stdout)", + ) + parser.add_argument( + "-n", + "--dry-run", + action="store_true", + help="report what would change without writing files", + ) + parser.add_argument( + "-q", + "--quiet", + action="store_true", + help="suppress the per-file report", + ) + options = parser.parse_args(argv) + status = 0 + + for path in options.files: + try: + process(path, options) + except (OSError, ValueError, expat.ExpatError) as error: + sys.stderr.write(f"{path}: {error}\n") + status = 1 + + return status + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/plugins/copilot/DEVELOPING.md b/plugins/copilot/DEVELOPING.md index 2b326a1..f26935c 100644 --- a/plugins/copilot/DEVELOPING.md +++ b/plugins/copilot/DEVELOPING.md @@ -8,8 +8,8 @@ embedded XML) via the desktop CLI, or a browser URL that opens the diagram direc This is the Copilot counterpart of the [Claude Code plugin](../claude-code/README.md). The skill body is host-agnostic — it drives the draw.io Desktop CLI directly — so -`skills/drawio/SKILL.md` is kept **byte-identical** to the Claude plugin's `SKILL.md`. That -includes the `/drawio:drawio` lines in its examples section: those are model-facing +`skills/drawio/SKILL.md` and the helper it ships (`skills/drawio/scripts/fix_edge_parents.py`) +are kept **byte-identical** to the Claude plugin's copies. That includes the `/drawio:drawio` lines in its examples section: those are model-facing (request → output mapping), while the user-facing explicit command in Copilot is plain `/drawio` — Copilot does not prefix skills with the plugin name. Only the host wrapping differs: the manifest (root `plugin.json` vs `.claude-plugin/plugin.json`) and the @@ -21,6 +21,7 @@ marketplace manifest location and schema. |------|---------| | `plugin.json` | Copilot plugin manifest at the plugin root — name, version, description, author, license, keywords, and the `skills` directory list | | `skills/drawio/SKILL.md` | The skill itself (its frontmatter `name` becomes the `/drawio` command); byte-identical to the Claude plugin's copy | +| `skills/drawio/scripts/fix_edge_parents.py` | Bundled helper the skill runs on every generated `.drawio` — sets each edge's `parent` to the nearest common ancestor of its `source` and `target`, which a layout pass needs in order to lay the diagram out correctly; byte-identical to the Claude plugin's copy (documented in [its DEVELOPING.md](../claude-code/DEVELOPING.md#edge-parents)) | | `README.md` | Installation and usage documentation | | `../../.github/plugin/marketplace.json` | Copilot marketplace manifest at the repo root; lists this plugin with `source: "./plugins/copilot"` | @@ -33,6 +34,7 @@ Codex), so this directory is itself the plugin root — the same shape as `claud plugins/copilot/ ← Copilot plugin root ├── plugin.json ├── skills/drawio/SKILL.md +├── skills/drawio/scripts/fix_edge_parents.py ├── README.md └── DEVELOPING.md ``` diff --git a/plugins/copilot/README.md b/plugins/copilot/README.md index 1a38bab..cab0629 100644 --- a/plugins/copilot/README.md +++ b/plugins/copilot/README.md @@ -22,13 +22,16 @@ When you ask Copilot to create a diagram, it will: present 2. Produce a native `.drawio` file — convert the Mermaid with the desktop CLI, or write the XML directly (optionally running an ELK `--layout` pass so you don't hand-place cells) -3. Handle the requested output: +3. Run the bundled `fix_edge_parents.py` over the file so every connector is parented to the + innermost container holding both of its endpoints — the rule the draw.io editor applies, + and what keeps a layout pass from laying the diagram out wrongly +4. Handle the requested output: - PNG / SVG / PDF — export using the draw.io desktop CLI - `url` — compress the XML with Node.js's built-in `zlib` and open `https://app.diagrams.net/#create=...` in your browser (keeps the `.drawio` file as a local copy) - *(default)* — leave the `.drawio` file as-is -4. Open the result +5. Open the result ## Prerequisites @@ -36,6 +39,8 @@ When you ask Copilot to create a diagram, it will: - [draw.io Desktop](https://github.com/jgraph/drawio-desktop/releases) installed — required for Mermaid conversion, ELK layout, and PNG/SVG/PDF export. Not needed for plain XML `.drawio` or `url` output, which Copilot can produce with no desktop app +- Python 3 on your `PATH` — used by the bundled edge-parent fixer (standard library only, no + packages to install). Without it Copilot sets the edge parents itself while authoring ## Installation diff --git a/plugins/copilot/skills/drawio/SKILL.md b/plugins/copilot/skills/drawio/SKILL.md index 54ead51..1d0ab04 100644 --- a/plugins/copilot/skills/drawio/SKILL.md +++ b/plugins/copilot/skills/drawio/SKILL.md @@ -33,23 +33,43 @@ Every diagram becomes a native `.drawio` file first, then is delivered in the re ``` Delete the `.mmd` afterward — the `.drawio` is the artifact. draw.io's Mermaid parser has already laid the diagram out, so no `--layout` is needed. - **XML**: write the mxGraphModel XML to `diagram.drawio` (see [XML format](#xml-format)). Optionally apply an ELK layout (see [ELK layout for XML](#elk-layout-for-xml)). -2. **Deliver** (identical for both sources): +2. **Fix edge parents** — see [Edge parents](#edge-parents): + ```bash + python3 scripts/fix_edge_parents.py diagram.drawio + ``` +3. **Deliver** (identical for both sources): - *(no format)* → keep `diagram.drawio` and open it. - **png / svg / pdf** → export from the `.drawio` with embedded XML, then delete the source `.drawio`: ```bash drawio -x -f png -e -b 10 -o diagram.drawio.png diagram.drawio ``` - **url** → build a browser URL from the `.drawio` XML, open it, and keep the `.drawio` as a local copy (see [Browser URL output](#browser-url-output)). -3. **Open the result** — the exported file, the URL, or the `.drawio`. If the open command fails, print the absolute path (or URL) so the user can open it manually. +4. **Open the result** — the exported file, the URL, or the `.drawio`. If the open command fails, print the absolute path (or URL) so the user can open it manually. **Always convert Mermaid to `.drawio` first, then export** — do not export a `.mmd` straight to an image. Direct Mermaid → PNG export with `-e` is broken in current draw.io Desktop (the embedded-XML step crashes); the two-step path (convert, then export the `.drawio`) is reliable and produces an editable embed. See [Troubleshooting](#troubleshooting). If Mermaid was requested but no desktop CLI is available, fall back to authoring the same diagram directly as XML. +## Edge parents + +An edge's `parent` must be the **nearest common ancestor** of its `source` and `target` cells — the innermost container holding both endpoints, or the layer when there is none. Otherwise a layout pass lays the diagram out wrongly. This overrides the "cross-container edges use `parent="1"`" advice in the [XML reference](#xml-reference). + +Don't work it out by hand — a script bundled with this skill fixes every edge in place: + +```bash +python3 scripts/fix_edge_parents.py diagram.drawio +``` + +- The path is relative to this `SKILL.md`; use the absolute path from another working directory. +- Run it on **every** `.drawio` — Mermaid output too, since `subgraph` blocks become containers — and **before** any `--layout` pass. Running it twice is a no-op. +- If `python3` is unavailable, set each edge's `parent` to the nearest common ancestor while authoring instead. + ## ELK layout for XML XML-authored diagrams can be auto-positioned by the CLI's `--layout` pass — the same ELK layouts as the editor's *Arrange ▸ Layout* menu and the same engine the draw.io MCP app server uses. Generate the cells with approximate (or even `0,0`) positions and let ELK place them; you only have to get the graph *structure* — nodes and edges — right. +Run `python3 scripts/fix_edge_parents.py diagram.drawio` first — see [Edge parents](#edge-parents). + Add `--layout ` to any CLI call that reads your XML. The simplest form lays out in place after you write the file (reading and overwriting the same path is supported): ```bash @@ -359,6 +379,7 @@ Every diagram must have this structure: - Cell `id="0"` is the root layer - Cell `id="1"` is the default parent layer - All diagram elements use `parent="1"` unless using multiple layers +- An edge's `parent` is the nearest common ancestor of its `source` and `target`, not necessarily `1` — see [Edge parents](#edge-parents) (The example above uses an XML comment only to point out where cells go — never emit comments in real output; see [XML well-formedness](#critical-xml-well-formedness).) @@ -374,6 +395,7 @@ https://raw.githubusercontent.com/jgraph/drawio-mcp/main/shared/xml-reference.md | draw.io CLI not found | Desktop app not installed or not on PATH | Author as XML and deliver a `.drawio` file or `url` (Mermaid conversion, ELK layout, and image export all need the desktop app). Tell the user they can install the draw.io desktop app to enable those | | Mermaid → PNG export crashes | Direct `.mmd` → PNG with `-e` is broken in current draw.io Desktop (embedded-XML step) | Use the two-step path: convert Mermaid to `.drawio` first (`-f xml`), then export the `.drawio` to PNG — the intermediate file embeds correctly | | Blank diagram from Mermaid | Misspelled type keyword, or a syntax error (bad node ID, unquoted label) | Check the [Mermaid reference](#mermaid-syntax-reference); the first non-directive line's keyword selects the diagram type | +| Layout comes out wrong after a `--layout` pass | An edge's `parent` is not the nearest common ancestor of its `source` and `target` | Run `python3 scripts/fix_edge_parents.py diagram.drawio` before the `--layout` call | | Layout does nothing / errors | Unknown preset name, custom JSON not an array, or a desktop build too old for `--layout` / `.mmd` input | Use a preset from [Layout presets](#layout-presets) or a JSON array starting with `[`; on an old desktop build, author as XML with explicit positions and tell the user updating draw.io Desktop enables Mermaid conversion and layouts | | Export produces empty/corrupt file | Invalid XML (e.g. double hyphens in comments, unescaped special characters) | Validate XML well-formedness before writing; see the XML well-formedness section below | | Diagram opens but looks blank | Missing root cells `id="0"` and `id="1"` | Ensure the basic mxGraphModel structure is complete | diff --git a/plugins/copilot/skills/drawio/scripts/fix_edge_parents.py b/plugins/copilot/skills/drawio/scripts/fix_edge_parents.py new file mode 100755 index 0000000..26d31c9 --- /dev/null +++ b/plugins/copilot/skills/drawio/scripts/fix_edge_parents.py @@ -0,0 +1,660 @@ +#!/usr/bin/env python3 +"""Reparent draw.io edges onto the nearest common ancestor of their endpoints. + +draw.io (mxGraph) expects the ``parent`` of an edge to be the nearest +common ancestor of its ``source`` and ``target`` cells. Generated diagrams +usually put every edge on the default layer (``parent="1"``) instead, which +renders fine but makes the layout passes (Arrange > Layout, ``--layout``) +lay the diagram out wrongly. This script rewrites those ``parent`` +attributes, applying the same rule as ``mxGraphModel.updateEdgeParent``, and +shifts any absolute edge geometry (waypoints, source/target points) so the +edge keeps its on-screen position. + +Usage: + fix_edge_parents.py diagram.drawio # fix in place + fix_edge_parents.py a.drawio b.drawio # fix several files + fix_edge_parents.py --dry-run diagram.drawio # report only, write nothing + cat diagram.drawio | fix_edge_parents.py - # stdin to stdout + +Reads ``.drawio``/``.xml`` files holding either a bare ```` or an +```` (every page is processed, compressed pages included). Everything +except the attributes it has to change is preserved byte for byte. + +Ported from the mxGraph copy bundled with draw.io v31.1.8, specifically +``mxGraphModel.updateEdgeParent`` and the helpers it calls. The paths below +are relative to + + https://github.com/jgraph/drawio/blob/v31.1.8/src/main/webapp/ + + updateEdgeParent mxgraph/src/model/mxGraphModel.js#L832-L885 + getOrigin mxgraph/src/model/mxGraphModel.js#L893-L918 + getNearestCommonAncestor mxgraph/src/model/mxGraphModel.js#L930-L970 + mxGeometry.translate mxgraph/src/model/mxGeometry.js#L299-L337 + +draw.io then overrides one of the flags they read: + + ignoreRelativeEdgeParent = false js/grapheditor/Graph.js#L175 + +Those upstream files carry the following copyright notices: + + mxGraphModel.js: Copyright (c) 2006-2018, JGraph Holdings Ltd + Copyright (c) 2006-2018, draw.io AG + mxGeometry.js: Copyright (c) 2006-2015, JGraph Holdings Ltd + Copyright (c) 2006-2015, draw.io AG + +See https://github.com/jgraph/drawio/blob/v31.1.8/LICENSE for the terms they +are published under. +""" + +# Outline: +# +# 1. Parse the XML document with the Expat parser and record the byte offsets +# of each element. +# 2. Rewrite attributes in the in-memory model. Record updated attributes. +# 3. Update attributes in the source document. + +import argparse +import base64 +import re +import sys +import urllib.parse +import zlib +from xml.parsers import expat + +WRAPPER_ELEMENTS = ("object", "UserObject") + +TAG_NAME_RE = re.compile(rb"<([^\s/>]+)") +ATTRIBUTE_RE = re.compile(rb"\s*([^\s=/>]+)\s*=\s*(\"[^\"]*\"|'[^']*')") +NUMBER_RE = re.compile(r"[+-]?(\d+\.?\d*|\.\d+)([eE][+-]?\d+)?") + +# mxGraph defaults this to true; draw.io turns it off, "Keeps edges between +# relative child cells inside parent" (js/grapheditor/Graph.js). +IGNORE_RELATIVE_EDGE_PARENT = False + + +def is_true(value): + """Convert an attribute value to a boolean. + + Only a missing, empty or numerically zero value is false — every other + spelling (``1``, ``true``, even ``false``) is true. + """ + if value is None or value.strip() == "": + return False + + try: + return float(value) != 0 + except ValueError: + return True + + +def to_float(value): + """Convert an attribute value to a float. + + A missing, unparsable or non-finite value reads as 0. + """ + # mxObjectCodec.convertAttributeFromXml runs parseFloat over the raw + # attribute and substitutes 0 for NaN and infinities. + match = NUMBER_RE.match(value.lstrip()) if value is not None else None + + if match is None: + return 0.0 + + number = float(match.group(0)) + + return number if -float("inf") < number < float("inf") else 0.0 + + +def format_number(value): + """Render a float value like JavaScript.""" + if value == int(value): + return str(int(value)) + + return repr(value) + + +class Node: + """One XML element, with the byte offsets of its tags.""" + + __slots__ = ("attrs", "children", "end", "parent", "start", "tag", "text") + + def __init__(self, tag, attrs, parent, start): + self.tag = tag + self.attrs = attrs + self.parent = parent + self.children = [] + self.start = start + self.end = start + self.text = [] + + def first(self, tag): + """Return the first direct child with the given tag name.""" + for child in self.children: + if child.tag == tag: + return child + + return None + + def walk(self): + """Yield this node and every descendant, in document order.""" + yield self + + for child in self.children: + yield from child.walk() + + +def parse_document(data): + """Parse XML bytes into a Node tree that remembers tag offsets. + + Raises expat.ExpatError unless the document is well formed. + """ + # CurrentByteIndex is what makes the byte-level patching possible: it + # locates each start tag in the original bytes. + # + # > During a callback reporting a parse event they indicate the location + # > of the first of the sequence of characters that generated the event. + # + # https://docs.python.org/3.9/library/pyexpat.html + document = Node("#document", {}, None, -1) + stack = [document] + parser = expat.ParserCreate() + + def start_element(tag, attrs): + node = Node(tag, attrs, stack[-1], parser.CurrentByteIndex) + stack[-1].children.append(node) + stack.append(node) + + def end_element(tag): + stack[-1].end = parser.CurrentByteIndex + stack.pop() + + def character_data(data): + # The text contents is processed later only for . + if stack[-1].tag == "diagram": + stack[-1].text.append(data) + + parser.StartElementHandler = start_element + parser.EndElementHandler = end_element + parser.CharacterDataHandler = character_data + parser.Parse(data, True) + + return document + + +class Cell: + """An mxCell.""" + + __slots__ = ("geometry", "id", "node") + + # Note that ``cell_id`` may be the ``id`` of the wrapper ``object`` or + # ``UserObject``. So we keep it explicitly aside from the ``node``. + def __init__(self, cell_id, node, geometry): + self.id = cell_id + self.node = node + self.geometry = geometry + + +class GraphModel: + """The cells of one ````, with the mxGraph tree helpers.""" + + def __init__(self, root_node): + # Dictionary from id to cell. + self.cells = {} + self.cell_list = [] + + # When a cell is wrapped in ``object`` or ``UserObject``, its id is + # the attribute of the wrapper. + for child in root_node.children: + if child.tag == "mxCell": + node = child + cell_id = child.attrs.get("id") + elif child.tag in WRAPPER_ELEMENTS: + node = child.first("mxCell") + cell_id = child.attrs.get("id") + else: + continue + + if node is None or cell_id is None or cell_id in self.cells: + continue + + cell = Cell(cell_id, node, node.first("mxGeometry")) + self.cells[cell_id] = cell + self.cell_list.append(cell) + + self.root = self._find_root() + + def _find_root(self): + """The model root — the last parentless cell (normally id="0").""" + # mxGraphModelCodec.decodeRoot keeps the last parentless cell it + # decodes as the root. + root = None + + for cell in self.cell_list: + if self.get_parent(cell) is None: + root = cell + + return root + + def get_parent(self, cell): + parent_id = cell.node.attrs.get("parent") + + # mxCodec cannot build a cell that is its own parent, so upstream + # never sees one; here it is read as parentless to keep walks finite. + if parent_id is None or parent_id == cell.id: + return None + + return self.cells.get(parent_id) + + def is_edge(self, cell): + return is_true(cell.node.attrs.get("edge")) + + def ancestors(self, cell): + """The cell itself, then every ancestor up to the root.""" + chain = [] + seen = set() + current = cell + + while current is not None and current.id not in seen: + seen.add(current.id) + chain.append(current) + current = self.get_parent(current) + + return chain + + def is_ancestor(self, parent, child): + return any(cell is parent for cell in self.ancestors(child)) + + def get_nearest_common_ancestor(self, cell1, cell2): + """The deepest cell that is an ancestor of both. + + A candidate whose own parent is null (the model root) is not + returned, matching getNearestCommonAncestor. + """ + path = {cell.id for cell in self.ancestors(cell2)} + + for cell in self.ancestors(cell1): + if cell.id in path: + return cell if self.get_parent(cell) is not None else None + + return None + + def get_origin(self, cell): + """Absolute offset of a cell's coordinates (mxGraph getOrigin).""" + # mxGraphModel.getOrigin recurses up to the root; we use an iteration + # instead. + x = 0.0 + y = 0.0 + + for current in self.ancestors(cell): + if self.is_edge(current) or current.geometry is None: + continue + + x += to_float(current.geometry.attrs.get("x")) + y += to_float(current.geometry.attrs.get("y")) + + return x, y + + def get_terminal(self, edge, is_source): + """The cell an edge points at, before any relative resolution. + + If is_source is True, return the source cell. Return the target cell + otherwise. + """ + attribute = "source" if is_source else "target" + + return self.cells.get(edge.node.attrs.get(attribute)) + + +def set_attribute(node, name, value, edits): + """Record an attribute change and apply it to the in-memory model.""" + node.attrs[name] = value + edits.append((node, name, value)) + + +def translate_point(node, dx, dy, edits): + x = to_float(node.attrs.get("x")) + dx + y = to_float(node.attrs.get("y")) + dy + set_attribute(node, "x", format_number(x), edits) + set_attribute(node, "y", format_number(y), edits) + + +def translate(geo, dx, dy, edits): + """Shift an edge geometry, mirroring mxGeometry.translate.""" + # mxGeometry.translate has no such early return. Skipping a zero delta + # keeps the file byte-identical when both origins coincide. + if dx == 0 and dy == 0: + return + + # Translates the geometry + if not is_true(geo.attrs.get("relative")): + translate_point(geo, dx, dy, edits) + + for child in geo.children: + tag = child.tag + role = child.attrs.get("as") + + # Translates the source and target points + if tag == "mxPoint" and role in ("sourcePoint", "targetPoint"): + translate_point(child, dx, dy, edits) + elif tag == "Array" and role == "points": + # Translate the control points + for point in child.children: + if point.tag == "mxPoint": + translate_point(point, dx, dy, edits) + + +def update_edge_parent(model, edge, root, edits): + """Move one edge onto the nearest common ancestor of its terminals. + + Returns an (edge id, old parent id, new parent id) tuple when the edge + was moved, otherwise None. + """ + # Transcribed statement by statement from mxGraphModel.updateEdgeParent. + # The `seen` sets are the one addition: they stop the terminal walks on a + # parent cycle, which the JavaScript loops on forever. + source = model.get_terminal(edge, True) + target = model.get_terminal(edge, False) + seen = set() + + # Uses the first non-relative descendants of the source terminal + while ( + source is not None + and source.id not in seen + and not model.is_edge(source) + and source.geometry is not None + and is_true(source.geometry.attrs.get("relative")) + ): + seen.add(source.id) + source = model.get_parent(source) + + seen = set() + + # Uses the first non-relative descendants of the target terminal + while ( + target is not None + and target.id not in seen + and IGNORE_RELATIVE_EDGE_PARENT + and not model.is_edge(target) + and target.geometry is not None + and is_true(target.geometry.attrs.get("relative")) + ): + seen.add(target.id) + target = model.get_parent(target) + + if not ( + model.is_ancestor(root, source) and model.is_ancestor(root, target) + ): + return None + + if source is target: + cell = model.get_parent(source) + else: + cell = model.get_nearest_common_ancestor(source, target) + + if ( + cell is None + or ( + model.get_parent(cell) is model.root + and not model.is_ancestor(cell, edge) + ) + or model.get_parent(edge) is cell + ): + return None + + old_parent = model.get_parent(edge) + geo = edge.geometry + + if geo is not None: + origin1 = model.get_origin(old_parent) + origin2 = model.get_origin(cell) + + dx = origin2[0] - origin1[0] + dy = origin2[1] - origin1[1] + + translate(geo, -dx, -dy, edits) + + set_attribute(edge.node, "parent", cell.id, edits) + + return ( + edge.id, + old_parent.id if old_parent is not None else None, + cell.id, + ) + + +def fix_model(model, edits): + """Reparent every edge of one model. + + Returns a list of (edge id, old, new) tuples. + """ + changes = [] + root = model.root + + if root is None: + return changes + + for edge in model.cell_list: + if not model.is_edge(edge) or not model.is_ancestor(root, edge): + continue + + change = update_edge_parent(model, edge, root, edits) + + if change is not None: + changes.append(change) + + return changes + + +def escape_attribute(value): + return ( + value.replace("&", "&") + .replace("<", "<") + .replace(">", ">") + .replace('"', """) + ) + + +def scan_start_tag(data, start): + """Take apart the start tag at `start`. + + Returns the offset just past its ``>``, the value span of each attribute, + and the offset at which a new attribute can be inserted. + """ + # The tag comes from a completed parse, so its syntax is known good. + position = TAG_NAME_RE.match(data, start).end() + values = {} + + while True: + attribute = ATTRIBUTE_RE.match(data, position) + + if attribute is None: + break + + span = (attribute.start(2) + 1, attribute.end(2) - 1) + values[attribute.group(1)] = span + position = attribute.end() + + # Only whitespace and an optional '/' can precede the closing '>' here. + return data.index(b">", position) + 1, values, position + + +def build_attribute_patches(data, edits): + """Turn recorded attribute edits into (start, end, replacement) patches.""" + grouped = {} + + for node, name, value in edits: + grouped.setdefault(id(node), (node, []))[1].append((name, value)) + + patches = [] + + for node, attribute_edits in grouped.values(): + values, insert_at = scan_start_tag(data, node.start)[1:] + added = b"" + + for name, value in attribute_edits: + encoded_name = name.encode("utf-8") + encoded_value = escape_attribute(value).encode("utf-8") + span = values.get(encoded_name) + + if span is None: + added += b" " + encoded_name + b'="' + encoded_value + b'"' + else: + patches.append((span[0], span[1], encoded_value)) + + if added: + patches.append((insert_at, insert_at, added)) + + return patches + + +def apply_patches(data, patches): + for start, end, replacement in sorted(patches, reverse=True): + data = data[:start] + replacement + data[end:] + + return data + + +def decode_diagram(text): + """Decode draw.io's deflateRaw + base64 page payload, or None.""" + try: + inflated = zlib.decompress(base64.b64decode(text, validate=True), -15) + + return urllib.parse.unquote(inflated.decode("utf-8")) + except (ValueError, zlib.error, UnicodeDecodeError): + return None + + +def encode_diagram(xml): + """Re-encode a page payload the way draw.io writes it.""" + compressor = zlib.compressobj(9, zlib.DEFLATED, -15) + quoted = urllib.parse.quote(xml, safe="!~*'()").encode("utf-8") + deflated = compressor.compress(quoted) + compressor.flush() + + return base64.b64encode(deflated) + + +def fix_xml(data): + """Fix every model in an XML document; returns (new bytes, changes).""" + # Patching by byte offset assumes markup is one byte per character, which + # holds for UTF-8 and the single-byte encodings but not for UTF-16/32. + if data.startswith((b"\xff\xfe", b"\xfe\xff")) or b"\x00" in data[:4]: + raise ValueError( + "UTF-16/UTF-32 is not supported; save the file as UTF-8" + ) + + document = parse_document(data) + # Three collections travel through the run, none of them from mxGraph: + # edits (node, attribute, value) — every attribute the model layer + # wants rewritten, filled in by update_edge_parent + # changes (edge id, old parent id, new parent id) — the same work as + # seen by the caller, which is what gets reported + # patches (start, end, bytes) — byte spans of `data` to splice, built + # from `edits` plus any page re-encoded after a nested fix + edits = [] + changes = [] + patches = [] + + for node in document.walk(): + if node.tag == "mxGraphModel": + root_node = node.first("root") + + if root_node is not None: + changes.extend(fix_model(GraphModel(root_node), edits)) + elif node.tag == "diagram" and node.first("mxGraphModel") is None: + payload = "".join(node.text).strip() + + if not payload: + continue + + inner = decode_diagram(payload) + + if inner is None: + continue + + fixed, inner_changes = fix_xml(inner.encode("utf-8")) + + if inner_changes: + changes.extend(inner_changes) + # Replace the whole content. This replaces any comments and + # other non-text contents in the element. + start = scan_start_tag(data, node.start)[0] + payload = encode_diagram(fixed.decode("utf-8")) + patches.append((start, node.end, payload)) + + patches.extend(build_attribute_patches(data, edits)) + + return apply_patches(data, patches), changes + + +def report(label, changes, stream): + if not changes: + stream.write(f"{label}: no edge parents to fix\n") + + return + + stream.write(f"{label}: {len(changes)} edge parent(s) fixed\n") + + for edge_id, old_parent, new_parent in changes: + old = "" if old_parent is None else old_parent + stream.write(f' {edge_id}: parent="{old}" -> parent="{new_parent}"\n') + + +def process(path, options): + """Fix one file (or stdin when path is '-'); returns the change count.""" + if path == "-": + data = sys.stdin.buffer.read() + else: + with open(path, "rb") as handle: + data = handle.read() + + fixed, changes = fix_xml(data) + + if path == "-": + sys.stdout.buffer.write(data if options.dry_run else fixed) + elif changes and not options.dry_run: + with open(path, "wb") as handle: + handle.write(fixed) + + if not options.quiet: + report("" if path == "-" else path, changes, sys.stderr) + + return len(changes) + + +def main(argv=None): + parser = argparse.ArgumentParser( + description="Set every draw.io edge's parent to the nearest common " + "ancestor of its source and target." + ) + parser.add_argument( + "files", + metavar="FILE", + nargs="+", + help=".drawio/.xml file to fix in place ('-' for stdin to stdout)", + ) + parser.add_argument( + "-n", + "--dry-run", + action="store_true", + help="report what would change without writing files", + ) + parser.add_argument( + "-q", + "--quiet", + action="store_true", + help="suppress the per-file report", + ) + options = parser.parse_args(argv) + status = 0 + + for path in options.files: + try: + process(path, options) + except (OSError, ValueError, expat.ExpatError) as error: + sys.stderr.write(f"{path}: {error}\n") + status = 1 + + return status + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/plugins/tests/README.md b/plugins/tests/README.md new file mode 100644 index 0000000..a271cc1 --- /dev/null +++ b/plugins/tests/README.md @@ -0,0 +1,67 @@ +# Tests + +Tests for the scripts the plugins bundle. Currently covers +[`fix_edge_parents.py`](../claude-code/skills/drawio/scripts/fix_edge_parents.py), +the helper the `drawio` skill runs on every generated `.drawio` file to set each +edge's `parent` to the nearest common ancestor of its `source` and `target`. + +The Claude Code copy is the one under test. The Codex and Copilot plugins ship +byte-identical copies of it. +[`check-skill-sync.yml`](../../.github/workflows/check-skill-sync.yml) checks +them identical. + +Python 3 standard library only (`unittest`) — nothing to install. + +## Running + +From the repository root: + +```bash +python3 -m unittest discover -s plugins/tests +``` + +A single module, or a single test: + +```bash +python3 -m unittest discover -s plugins/tests -p test_cli.py +python3 plugins/tests/test_geometry.py NumberFormatTest +``` + +Add `-v` for per-test output. CI runs the same command from +[`test-scripts.yml`](../../.github/workflows/test-scripts.yml). + +## Test files + +| File | Description | +|------|-------------| +| `helpers.py` | Helper functions. Also imports `fix_edge_parents.py` with adjusted `sys.path`. | +| `test_edge_parents.py` | The parenting rule: nearest common ancestor, self-loops, relative terminals, layer boundaries, `` wrappers, flag truthiness | +| `test_geometry.py` | Waypoint/endpoint translation, `as="offset"` left alone, `parseFloat`/number formatting semantics | +| `test_file_formats.py` | `mxGraphModel` vs `mxfile`, compressed pages (comments, CDATA, character references), encodings (UTF-8, BOM, single-byte, UTF-16 refusal), byte preservation, idempotence | +| `test_cli.py` | CLI arguments, in-place vs `--dry-run`, `--quiet`, stdin/stdout, exit status, report text, non-UTF-8 locale | + +Inputs are built as ElementTree elements using helper functions +(for example, `vertex(id="a", parent="box", x=20, y=40)`) and serialized by +`model`. For tests checking byte-for-byte property, inputs and outputs are +handled as strings or byte sequences. + +## Fixtures + +Small input data are embedded in test modules. Larger realistic inputs are +placed under `fixtures/` directory. + +| Fixture | Purpose | +|---------|---------| +| `aws-containers.drawio` / `.expected.drawio` | Nested containers (AWS → VPC → subnets); the edge between subnets moves to the VPC and its waypoint shifts by the origin delta | +| `non-ascii.drawio` / `.expected.drawio` | Japanese labels, emoji, astral characters, non-ASCII cell ids, escaped markup, numeric character references, a single-quoted attribute, an XML declaration | + + +To regenerate `.expected.drawio` files: + +```bash +cd plugins +cp tests/fixtures/NAME.drawio tests/fixtures/NAME.expected.drawio +python3 claude-code/skills/drawio/scripts/fix_edge_parents.py tests/fixtures/NAME.expected.drawio +``` + +and review the diff before committing. diff --git a/plugins/tests/fixtures/aws-containers.drawio b/plugins/tests/fixtures/aws-containers.drawio new file mode 100644 index 0000000..224deac --- /dev/null +++ b/plugins/tests/fixtures/aws-containers.drawio @@ -0,0 +1,53 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/plugins/tests/fixtures/aws-containers.expected.drawio b/plugins/tests/fixtures/aws-containers.expected.drawio new file mode 100644 index 0000000..3d5258d --- /dev/null +++ b/plugins/tests/fixtures/aws-containers.expected.drawio @@ -0,0 +1,53 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/plugins/tests/fixtures/non-ascii.drawio b/plugins/tests/fixtures/non-ascii.drawio new file mode 100644 index 0000000..c41c415 --- /dev/null +++ b/plugins/tests/fixtures/non-ascii.drawio @@ -0,0 +1,34 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/plugins/tests/fixtures/non-ascii.expected.drawio b/plugins/tests/fixtures/non-ascii.expected.drawio new file mode 100644 index 0000000..b05b062 --- /dev/null +++ b/plugins/tests/fixtures/non-ascii.expected.drawio @@ -0,0 +1,34 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/plugins/tests/helpers.py b/plugins/tests/helpers.py new file mode 100644 index 0000000..699e825 --- /dev/null +++ b/plugins/tests/helpers.py @@ -0,0 +1,229 @@ +"""Shared helpers for the fix_edge_parents.py tests.""" + +import base64 +import sys +import urllib.parse +import zlib +from pathlib import Path +from xml.etree import ElementTree + +sys.path.append( + str( + Path(__file__).resolve().parent.parent + / "claude-code/skills/drawio/scripts" + ) +) + +import fix_edge_parents as script + +TESTS_DIR = Path(__file__).resolve().parent +PLUGINS_DIR = TESTS_DIR.parent +FIXTURES = TESTS_DIR / "fixtures" + +SCRIPT = PLUGINS_DIR / "claude-code/skills/drawio/scripts/fix_edge_parents.py" + +WRAPPER_TAGS = ("object", "UserObject") + + +def fix(xml): + """Run the fixer over an XML string; returns (output string, changes).""" + fixed, changes = script.fix_xml(xml.encode("utf-8")) + + return fixed.decode("utf-8"), changes + + +def cell(*, id=None, parent=None, geometry=None, **attributes): + """An mxCell element; a layer when it carries no vertex or edge flag.""" + element = ElementTree.Element("mxCell") + + if id is not None: + element.set("id", id) + + for name, value in attributes.items(): + element.set(name, str(value)) + + if parent is not None: + element.set("parent", parent) + + if geometry is not None: + element.append(geometry) + + return element + + +def vertex( + *, id, parent, x=0, y=0, width=80, height=40, relative=False, **attributes +): + """A vertex mxCell with its geometry.""" + geometry = ElementTree.Element( + "mxGeometry", + {"x": str(x), "y": str(y), "width": str(width), "height": str(height)}, + ) + + if relative: + geometry.set("relative", "1") + + geometry.set("as", "geometry") + + return cell( + id=id, parent=parent, geometry=geometry, **attributes, vertex="1" + ) + + +def edge( + *, + id, + parent, + source, + target, + flag="1", + relative=True, + points=None, + source_point=None, + target_point=None, + offset=None, + geometry=True, + **attributes, +): + """An edge mxCell, with the geometry its waypoints and end points need.""" + element = cell(id=id, parent=parent, **attributes, edge=flag) + element.set("source", source) + element.set("target", target) + + if not geometry: + return element + + geo = ElementTree.SubElement(element, "mxGeometry") + + if relative: + geo.set("relative", "1") + + geo.set("as", "geometry") + + for name, point in ( + ("sourcePoint", source_point), + ("targetPoint", target_point), + ("offset", offset), + ): + if point is not None: + ElementTree.SubElement( + geo, + "mxPoint", + {"x": str(point[0]), "y": str(point[1]), "as": name}, + ) + + if points: + array = ElementTree.SubElement(geo, "Array", {"as": "points"}) + + for point in points: + ElementTree.SubElement( + array, "mxPoint", {"x": str(point[0]), "y": str(point[1])} + ) + + return element + + +def wrapped(inner, *, element="object", **attributes): + """An / holding a cell, as draw.io writes a cell + that carries metadata.""" + wrapper = ElementTree.Element(element, {}) + + for name, value in attributes.items(): + wrapper.set(name, str(value)) + + wrapper.append(inner) + + return wrapper + + +def model(cells, *, prolog=""): + """Serialize cells into a minimal mxGraphModel document.""" + root = ElementTree.Element("mxGraphModel") + cell_parent = ElementTree.SubElement(root, "root") + cell_parent.append(cell(id="0")) + cell_parent.append(cell(id="1", parent="0")) + + for element in cells: + cell_parent.append(element) + + ElementTree.indent(root, space=" ") + markup = ElementTree.tostring(root, encoding="unicode") + + return prolog + markup.replace(" />", "/>") + "\n" + + +def compress(xml): + """Encode a page payload the way draw.io writes a compressed diagram.""" + compressor = zlib.compressobj(9, zlib.DEFLATED, -15) + quoted = urllib.parse.quote(xml, safe="!~*'()").encode("utf-8") + deflated = compressor.compress(quoted) + compressor.flush() + + return base64.b64encode(deflated).decode("ascii") + + +def decompress(payload): + """Decode a compressed page payload back to XML.""" + inflated = zlib.decompress(base64.b64decode(payload), -15) + + return urllib.parse.unquote(inflated.decode("utf-8")) + + +def read_fixture(name): + """Fixture contents as text.""" + return (FIXTURES / name).read_text(encoding="utf-8") + + +def read_fixture_bytes(name): + """Fixture contents as bytes.""" + return (FIXTURES / name).read_bytes() + + +def element_of(xml, element_id): + """The element carrying that id, whatever its tag.""" + return ElementTree.fromstring(xml).find(f'.//*[@id="{element_id}"]') + + +def text_of(xml, element_id): + """The text content of the element with the given id and its descendants.""" + return "".join(element_of(xml, element_id).itertext()) + + +def mxcell_of(xml, cell_id): + """The mxCell holding that cell's attributes, unwrapped if needed.""" + element = element_of(xml, cell_id) + + if element is not None and element.tag in WRAPPER_TAGS: + return element.find("mxCell") + + return element + + +def parent_of(xml, cell_id): + """The parent attribute of a cell.""" + cell = mxcell_of(xml, cell_id) + + return None if cell is None else cell.get("parent") + + +def geometry_of(xml, cell_id): + """The mxGeometry of a cell.""" + return mxcell_of(xml, cell_id).find("mxGeometry") + + +def points_of(xml, cell_id): + """The waypoints of an edge, as (x, y) attribute strings.""" + array = geometry_of(xml, cell_id).find('Array[@as="points"]') + + if array is None: + return [] + + return [ + (point.get("x"), point.get("y")) for point in array.findall("mxPoint") + ] + + +def point_of(xml, cell_id, name): + """A named mxPoint of a geometry, as (x, y) attribute strings.""" + point = geometry_of(xml, cell_id).find(f'mxPoint[@as="{name}"]') + + return None if point is None else (point.get("x"), point.get("y")) diff --git a/plugins/tests/test_cli.py b/plugins/tests/test_cli.py new file mode 100644 index 0000000..f99d164 --- /dev/null +++ b/plugins/tests/test_cli.py @@ -0,0 +1,149 @@ +"""Command line behaviour: arguments, exit status, and the report.""" + +import os +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +from helpers import SCRIPT, read_fixture_bytes + +SAMPLE = read_fixture_bytes("aws-containers.drawio") +EXPECTED = read_fixture_bytes("aws-containers.expected.drawio") + + +def run(*args, stdin=None, env=None): + """Invoke the script; returns the CompletedProcess.""" + return subprocess.run( + [sys.executable, str(SCRIPT)] + list(args), + input=stdin, + capture_output=True, + env=env, + check=False, + ) + + +class CommandLineTest(unittest.TestCase): + def setUp(self): + self.directory = tempfile.TemporaryDirectory() + self.addCleanup(self.directory.cleanup) + + def write_file(self, name="diagram.drawio", content=SAMPLE): + path = Path(self.directory.name) / name + path.write_bytes(content) + + return path + + def test_fixes_a_file_in_place(self): + path = self.write_file() + result = run(str(path)) + + self.assertEqual(result.returncode, 0) + self.assertEqual(path.read_bytes(), EXPECTED) + + def test_reports_every_edge_it_moved(self): + path = self.write_file() + report = run(str(path)).stderr.decode("utf-8") + + self.assertIn("1 edge parent(s) fixed", report) + self.assertIn('e2: parent="1" -> parent="vpc"', report) + self.assertIn(str(path), report) + + def test_reports_when_there_is_nothing_to_do(self): + path = self.write_file(content=EXPECTED) + report = run(str(path)).stderr.decode("utf-8") + + self.assertIn("no edge parents to fix", report) + + def test_dry_run_reports_without_writing(self): + path = self.write_file() + result = run("--dry-run", str(path)) + report = result.stderr.decode("utf-8") + + self.assertEqual(result.returncode, 0) + self.assertEqual(path.read_bytes(), SAMPLE) + self.assertIn("1 edge parent(s) fixed", report) + self.assertIn('e2: parent="1" -> parent="vpc"', report) + self.assertIn(str(path), report) + + def test_quiet_suppresses_the_report(self): + path = self.write_file() + result = run("--quiet", str(path)) + + self.assertEqual(result.stderr, b"") + self.assertEqual(path.read_bytes(), EXPECTED) + + def test_several_files_in_one_call(self): + first = self.write_file(name="one.drawio") + second = self.write_file(name="two.drawio") + result = run(str(first), str(second)) + + self.assertEqual(result.returncode, 0) + self.assertEqual(first.read_bytes(), EXPECTED) + self.assertEqual(second.read_bytes(), EXPECTED) + + def test_stdin_to_stdout(self): + result = run("-", stdin=SAMPLE) + + self.assertEqual(result.returncode, 0) + self.assertEqual(result.stdout, EXPECTED) + self.assertIn("", result.stderr.decode("utf-8")) + + def test_stdin_dry_run_passes_the_input_through(self): + result = run("-", "--dry-run", stdin=SAMPLE) + + self.assertEqual(result.stdout, SAMPLE) + + def test_missing_file_fails(self): + result = run(str(Path(self.directory.name) / "nope.drawio")) + + self.assertEqual(result.returncode, 1) + self.assertIn("nope.drawio", result.stderr.decode("utf-8")) + + def test_malformed_xml_fails_without_writing(self): + content = b"" + path = self.write_file(content=content) + result = run(str(path)) + + self.assertEqual(result.returncode, 1) + self.assertEqual(path.read_bytes(), content) + + def test_utf16_file_is_refused_and_left_alone(self): + content = SAMPLE.decode("utf-8").encode("utf-16") + path = self.write_file(content=content) + result = run(str(path)) + + self.assertEqual(result.returncode, 1) + self.assertIn("UTF-16", result.stderr.decode("utf-8")) + self.assertEqual(path.read_bytes(), content) + + def test_one_bad_file_does_not_stop_the_others(self): + good = self.write_file(name="good.drawio") + result = run(str(Path(self.directory.name) / "nope.drawio"), str(good)) + + self.assertEqual(result.returncode, 1) + self.assertEqual(good.read_bytes(), EXPECTED) + + def test_runs_under_a_non_utf8_locale(self): + path = self.write_file( + name="non-ascii.drawio", + content=read_fixture_bytes("non-ascii.drawio"), + ) + env = dict(os.environ, LC_ALL="C", LANG="C") + result = run(str(path), env=env) + + self.assertEqual(result.returncode, 0) + self.assertEqual( + path.read_bytes(), read_fixture_bytes("non-ascii.expected.drawio") + ) + + def test_help_is_available(self): + result = run("--help") + + self.assertEqual(result.returncode, 0) + self.assertIn("nearest common ancestor", result.stdout.decode("utf-8")) + + +if __name__ == "__main__": + unittest.main() diff --git a/plugins/tests/test_edge_parents.py b/plugins/tests/test_edge_parents.py new file mode 100644 index 0000000..4177c61 --- /dev/null +++ b/plugins/tests/test_edge_parents.py @@ -0,0 +1,226 @@ +"""The parenting rule itself — the port of mxGraphModel.updateEdgeParent.""" + +import unittest + +from helpers import ( + cell, + edge, + element_of, + fix, + model, + parent_of, + read_fixture, + vertex, + wrapped, +) + + +def port(*, id, parent): + """A port: a child positioned relative to the cell owning it.""" + return vertex(id=id, parent=parent, width=10, height=10, relative=True) + + +class NearestCommonAncestorTest(unittest.TestCase): + def test_edge_between_container_children_moves_into_the_container(self): + xml = model( + [ + vertex(id="box", parent="1"), + vertex(id="a", parent="box"), + vertex(id="b", parent="box"), + edge(id="e", parent="1", source="a", target="b"), + ] + ) + out, changes = fix(xml) + + self.assertEqual(changes, [("e", "1", "box")]) + self.assertEqual(parent_of(out, "e"), "box") + + def test_edge_leaving_the_container_stays_on_the_layer(self): + xml = model( + [ + vertex(id="box", parent="1"), + vertex(id="a", parent="box"), + vertex(id="out", parent="1"), + edge(id="e", parent="1", source="a", target="out"), + ] + ) + out, changes = fix(xml) + + self.assertEqual(changes, []) + self.assertEqual(out, xml) + + def test_deeply_nested_common_ancestor(self): + """The AWS sample: `alb` and `ecs` sit in different subnets of `vpc`, + so the edge between them belongs to `vpc`. + """ + _out, changes = fix(read_fixture("aws-containers.drawio")) + + self.assertEqual(changes, [("e2", "1", "vpc")]) + + def test_terminal_that_is_an_ancestor_of_the_other(self): + xml = model( + [ + vertex(id="box", parent="1"), + vertex(id="a", parent="box"), + edge(id="e", parent="1", source="a", target="box"), + ] + ) + _out, changes = fix(xml) + + self.assertEqual(changes, [("e", "1", "box")]) + + def test_self_loop_takes_the_parent_of_its_terminal(self): + xml = model( + [ + vertex(id="box", parent="1"), + vertex(id="a", parent="box"), + edge(id="e", parent="1", source="a", target="a"), + ] + ) + _out, changes = fix(xml) + + self.assertEqual(changes, [("e", "1", "box")]) + + def test_edge_inside_a_container_is_moved_back_up_to_the_layer(self): + xml = model( + [ + vertex(id="box", parent="1"), + vertex(id="a", parent="1"), + vertex(id="b", parent="1"), + edge(id="e", parent="box", source="a", target="b"), + ] + ) + _out, changes = fix(xml) + + self.assertEqual(changes, [("e", "box", "1")]) + + def test_edges_are_never_moved_between_layers(self): + xml = model( + [ + cell(id="2", parent="0", value="Other layer"), + vertex(id="a", parent="2"), + vertex(id="b", parent="2"), + edge(id="e", parent="1", source="a", target="b"), + ] + ) + out, changes = fix(xml) + + self.assertEqual(changes, []) + self.assertEqual(out, xml) + + +class TerminalResolutionTest(unittest.TestCase): + def test_relative_source_resolves_to_its_owner(self): + xml = model( + [ + vertex(id="box", parent="1"), + vertex(id="n1", parent="box"), + port(id="p1", parent="n1"), + edge(id="e", parent="1", source="p1", target="n1"), + ] + ) + _out, changes = fix(xml) + + self.assertEqual(changes, [("e", "1", "box")]) + + def test_relative_target_is_not_resolved(self): + """draw.io sets ignoreRelativeEdgeParent=false, unlike bare mxGraph. + + Resolving the target would make this a self-loop landing on "box"; + draw.io keeps the edge inside "n2", the cell owning the port. + """ + xml = model( + [ + vertex(id="box", parent="1"), + vertex(id="n2", parent="box"), + port(id="p2", parent="n2"), + edge(id="e", parent="1", source="n2", target="p2"), + ] + ) + _out, changes = fix(xml) + + self.assertEqual(changes, [("e", "1", "n2")]) + + def test_missing_terminal_is_left_alone(self): + xml = model( + [ + vertex(id="box", parent="1"), + vertex(id="a", parent="box"), + edge(id="e", parent="1", source="a", target="nowhere"), + ] + ) + out, changes = fix(xml) + + self.assertEqual(changes, []) + self.assertEqual(out, xml) + + def test_edge_without_a_parent_is_outside_the_model_tree(self): + xml = model( + [ + vertex(id="box", parent="1"), + vertex(id="a", parent="box"), + vertex(id="b", parent="box"), + edge( + id="e", parent=None, source="a", target="b", geometry=False + ), + ] + ) + out, changes = fix(xml) + + self.assertEqual(changes, []) + self.assertEqual(out, xml) + + +class CellIdentityTest(unittest.TestCase): + def test_object_wrapper_carries_the_id(self): + xml = model( + [ + wrapped(vertex(id=None, parent="1"), id="box"), + wrapped(vertex(id=None, parent="box"), id="a"), + wrapped(vertex(id=None, parent="box"), id="b"), + wrapped( + edge(id=None, parent="1", source="a", target="b"), + id="e", + ), + ] + ) + out, changes = fix(xml) + wrapper = element_of(out, "e") + wrapped_cell = wrapper.find("mxCell") + + self.assertEqual(changes, [("e", "1", "box")]) + self.assertEqual(wrapper.tag, "object") + self.assertIsNone(wrapped_cell.get("id")) + self.assertEqual(wrapped_cell.get("parent"), "box") + + def test_flag_truthiness_matches_mxgraph(self): + """mxCell.isEdge() is `edge != 0`: only 0 and "" are false.""" + xml = model( + [ + vertex(id="box", parent="1"), + vertex(id="a", parent="box"), + vertex(id="b", parent="box"), + *( + edge( + id=f"e{index}", + parent="1", + source="a", + target="b", + flag=flag, + ) + for index, flag in enumerate( + ("1", "true", "false", "0", "") + ) + ), + ] + ) + _out, changes = fix(xml) + + self.assertEqual( + changes, + [("e0", "1", "box"), ("e1", "1", "box"), ("e2", "1", "box")], + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/plugins/tests/test_file_formats.py b/plugins/tests/test_file_formats.py new file mode 100644 index 0000000..cf996d1 --- /dev/null +++ b/plugins/tests/test_file_formats.py @@ -0,0 +1,274 @@ +"""Input formats, encodings, and the byte-preserving guarantee.""" + +import codecs +import re +import unittest + +from helpers import ( + compress, + decompress, + edge, + element_of, + fix, + model, + mxcell_of, + parent_of, + read_fixture, + read_fixture_bytes, + script, + text_of, + vertex, +) + + +MODEL = model( + [ + vertex(id="c", parent="1", x=10, y=20, width=300, height=200), + vertex(id="a", parent="c", x=0, y=0), + vertex(id="b", parent="c", x=100, y=0), + edge(id="e", parent="1", source="a", target="b"), + ] +) +EXPECTED_CHANGE = ("e", "1", "c") +EDGE_ID = "e" +EXPECTED_PARENT = "c" + + +def model_with_edge_attribute(attribute, *, prolog=""): + """A model whose edge carries `attribute` as its value attribute. + + `attribute` is a raw string in form ``value="..."`` or ``value='...'``. + Special characters in it are not replaced with entity references. + """ + xml = model( + [ + vertex(id="c", parent="1", x=10, y=20, width=300, height=200), + vertex(id="a", parent="c", x=0, y=0), + vertex(id="b", parent="c", x=100, y=0), + edge( + id="e", parent="1", source="a", target="b", value="PLACEHOLDER" + ), + ], + prolog=prolog, + ) + + return re.sub(r"""value=["']PLACEHOLDER["']""", attribute, xml, count=1) + + +def page_with_text(content, id="p", name="C"): + """A page () with the given raw content.""" + return f'{content}' + + +def file_with_pages(*pages): + """An mxfile holding given raw pages.""" + separator = "\n " + + return f"\n {separator.join(pages)}\n\n" + + +class BytePreservationTest(unittest.TestCase): + def test_only_the_parent_attribute_changes(self): + original = read_fixture_bytes("aws-containers.drawio") + expected = read_fixture_bytes("aws-containers.expected.drawio") + fixed, changes = script.fix_xml(original) + + self.assertEqual(fixed, expected) + self.assertEqual(changes, [("e2", "1", "vpc")]) + + def test_a_second_run_is_a_no_op(self): + expected = read_fixture_bytes("aws-containers.expected.drawio") + fixed, changes = script.fix_xml(expected) + + self.assertEqual(fixed, expected) + self.assertEqual(changes, []) + + def test_an_attribute_value_that_looks_like_a_parent_attribute(self): + """A `parent="1"` inside a value must not be taken for the real one.""" + out, _changes = fix( + model_with_edge_attribute("""value='parent="1"'""") + ) + + self.assertEqual(parent_of(out, EDGE_ID), EXPECTED_PARENT) + self.assertEqual(mxcell_of(out, EDGE_ID).get("value"), 'parent="1"') + + def test_character_references_in_unchanged_attributes_are_keeped(self): + out, _changes = fix( + # value="ABC & " + model_with_edge_attribute('value="ABC & <b>"') + ) + + self.assertIn('value="ABC & <b>"', out) + self.assertEqual(mxcell_of(out, EDGE_ID).get("value"), "ABC & ") + + def test_quoting_style_keeped(self): + out, _changes = fix(model_with_edge_attribute("value='foo'")) + + self.assertIn("value='foo'", out) + self.assertEqual(mxcell_of(out, EDGE_ID).get("value"), "foo") + + def test_xml_declaration(self): + prolog = '\n' + fixed, _changes = fix(prolog + MODEL) + + self.assertTrue( + fixed.startswith('') + ) + + +class DocumentShapeTest(unittest.TestCase): + """.drawio files may contain multiple pages and compressed contents.""" + + def test_bare_mxgraphmodel(self): + _out, changes = fix(MODEL) + + self.assertEqual(changes, [EXPECTED_CHANGE]) + + def test_mxfile_processes_every_page(self): + page1 = model( + [ + vertex(id="c1", parent="1", x=10, y=20, width=300, height=200), + vertex(id="a1", parent="c1", x=0, y=0), + vertex(id="b1", parent="c1", x=100, y=0), + edge(id="e1", parent="1", source="a1", target="b1"), + ] + ) + page2 = model( + [ + vertex(id="c2", parent="1", x=10, y=20, width=300, height=200), + vertex(id="a2", parent="c2", x=0, y=0), + vertex(id="b2", parent="c2", x=100, y=0), + edge(id="e2", parent="1", source="a2", target="b2"), + ] + ) + xml = file_with_pages( + page_with_text(page1, id="page-1", name="Uncompressed"), + page_with_text(compress(page2), id="page-2", name="Compressed"), + ) + out, changes = fix(xml) + page2_fixed = decompress(text_of(out, "page-2")) + + self.assertEqual(changes, [("e1", "1", "c1"), ("e2", "1", "c2")]) + self.assertEqual(element_of(out, "page-1").get("name"), "Uncompressed") + self.assertEqual(element_of(out, "page-2").get("name"), "Compressed") + self.assertEqual(parent_of(out, "e1"), "c1") + self.assertEqual(parent_of(page2_fixed, "e2"), "c2") + + def test_compressed_page_is_re_encoded_the_same_way(self): + xml = file_with_pages( + page_with_text( + compress(model_with_edge_attribute('value="😀"')), + id="p", + ) + ) + out, _changes = fix(xml) + payload = text_of(out, "p") + page = decompress(payload) + + self.assertEqual(mxcell_of(page, EDGE_ID).get("value"), "😀") + + def test_page_without_content_is_skipped(self): + for closer in (">", "/>"): + with self.subTest(content=closer): + xml = '\n" + out, changes = fix(xml) + + self.assertEqual(changes, []) + self.assertEqual(out, xml) + + def test_page_with_unreadable_payload_is_skipped(self): + xml = file_with_pages(page_with_text("nope")) + out, changes = fix(xml) + + self.assertEqual(changes, []) + self.assertEqual(out, xml) + + def test_payload_is_read_around_comments_and_cdata(self): + payload = compress(MODEL) + contents = { + "no comments": payload, + "comment first": "" + payload, + "comment last": payload + "", + "comment inside": payload[:10] + "" + payload[10:], + "cdata": f"{payload[:5]}{payload[10:]}", + "character reference": f"&#{ord(payload[0])};{payload[1:]}", + "surrounding space": f"\n {payload}\n ", + } + + for name, content in contents.items(): + with self.subTest(content=name): + out, changes = fix( + file_with_pages(page_with_text(content, id="p")) + ) + fixed_page = decompress(text_of(out, "p")) + + edge_id, _old, new_parent = EXPECTED_CHANGE + + self.assertEqual(changes, [EXPECTED_CHANGE]) + self.assertEqual(parent_of(fixed_page, edge_id), new_parent) + + def test_a_rewritten_page_holds_nothing_but_the_payload(self): + xml = file_with_pages( + page_with_text("" + compress(MODEL)) + ) + out, _changes = fix(xml) + fixed_model, _changes = fix(MODEL) + # Comments around the fixed compressed content are removed. + expected = file_with_pages(page_with_text(compress(fixed_model))) + + self.assertEqual(out, expected) + + def test_an_unchanged_page_keeps_its_markup(self): + fixed_model, _changes = fix(MODEL) + xml = file_with_pages( + page_with_text(f"{compress(fixed_model)}") + ) + out, changes = fix(xml) + + self.assertEqual(changes, []) + self.assertEqual(out, xml) + + +class EncodingTest(unittest.TestCase): + def test_utf8_labels_ids(self): + original = read_fixture_bytes("non-ascii.drawio") + expected = read_fixture_bytes("non-ascii.expected.drawio") + fixed, changes = script.fix_xml(original) + + self.assertEqual(fixed, expected) + self.assertEqual(changes, [("辺①", "1", "容器")]) + self.assertFalse(fixed.startswith(codecs.BOM_UTF8)) + + def test_utf8_bom_is_preserved(self): + raw = codecs.BOM_UTF8 + read_fixture_bytes("non-ascii.drawio") + fixed, changes = script.fix_xml(raw) + + self.assertTrue(fixed.startswith(codecs.BOM_UTF8)) + self.assertEqual(changes, [("辺①", "1", "容器")]) + + def test_single_byte_encoding(self): + prolog = '\n' + raw = model_with_edge_attribute( + 'value="Ünïcödé Grüße"', prolog=prolog + ).encode("latin-1") + fixed, changes = script.fix_xml(raw) + + self.assertEqual(changes, [EXPECTED_CHANGE]) + self.assertIn('value="Ünïcödé Grüße"'.encode("latin-1"), fixed) + + def test_utf16_is_refused(self): + prolog = '\n' + raw = (prolog + MODEL).encode("utf-16") + + with self.assertRaises(ValueError): + script.fix_xml(raw) + + def test_utf16_without_a_bom_is_refused(self): + raw = MODEL.encode("utf-16-be") + + with self.assertRaises(ValueError): + script.fix_xml(raw) + + +if __name__ == "__main__": + unittest.main() diff --git a/plugins/tests/test_geometry.py b/plugins/tests/test_geometry.py new file mode 100644 index 0000000..ae67d5d --- /dev/null +++ b/plugins/tests/test_geometry.py @@ -0,0 +1,172 @@ +"""Geometry translation and number handling (mxGeometry.translate).""" + +import unittest + +from helpers import ( + edge, + fix, + geometry_of, + model, + point_of, + points_of, + read_fixture, + script, + vertex, +) + + +class TranslationTest(unittest.TestCase): + def test_waypoints_and_endpoints_shift_by_the_origin_delta(self): + """Every absolute point of the edge is subtracted the new parent's + origin, (100, 50). + """ + xml = model( + [ + vertex(id="c", parent="1", x=100, y=50, width=400, height=300), + vertex(id="a", parent="c", x=20, y=20), + vertex(id="b", parent="c", x=220, y=200), + edge( + id="e", + parent="1", + source="a", + target="b", + source_point=(160, 90), + target_point=(360, 270), + offset=(5, -5), + points=[(200, 90), (200, 270)], + ), + ] + ) + out, changes = fix(xml) + + self.assertEqual(changes, [("e", "1", "c")]) + self.assertEqual(point_of(out, "e", "sourcePoint"), ("60", "40")) + self.assertEqual(point_of(out, "e", "targetPoint"), ("260", "220")) + self.assertEqual(points_of(out, "e"), [("100", "40"), ("100", "220")]) + + def test_offset_point_is_left_alone(self): + """mxGeometry.translate leaves `as="offset"` where it is.""" + source = read_fixture("aws-containers.drawio") + out, _changes = fix(source) + + self.assertEqual( + point_of(out, "e2", "offset"), point_of(source, "e2", "offset") + ) + + def test_non_relative_geometry_position_is_translated(self): + """A geometry without `relative` has its own x/y shifted too.""" + xml = model( + [ + vertex(id="c", parent="1", x=100, y=50, width=400, height=300), + vertex(id="a", parent="c", x=20, y=20), + vertex(id="b", parent="c", x=220, y=200), + edge( + id="e", parent="1", source="a", target="b", relative=False + ), + ] + ) + out, _changes = fix(xml) + geometry = geometry_of(out, "e") + + self.assertEqual(geometry.get("x"), "-100") + self.assertEqual(geometry.get("y"), "-50") + + def test_zero_delta_leaves_the_geometry_untouched(self): + """The container sits at the origin, so nothing has to move.""" + xml = model( + [ + vertex(id="c", parent="1", x=0, y=0, width=400, height=300), + vertex(id="a", parent="c", x=20, y=20), + vertex(id="b", parent="c", x=220, y=20), + edge( + id="e", + parent="1", + source="a", + target="b", + relative=False, + points=[(150, 40)], + ), + ] + ) + out, changes = fix(xml) + + self.assertEqual(changes, [("e", "1", "c")]) + self.assertEqual(set(geometry_of(out, "e").keys()), {"as"}) + self.assertEqual(points_of(out, "e"), [("150", "40")]) + + def test_fractional_coordinates_keep_their_precision(self): + """(30, 70) less the container's (10.5, 0.25).""" + xml = model( + [ + vertex( + id="c", parent="1", x=10.5, y=0.25, width=200, height=100 + ), + vertex(id="a", parent="c", x=0, y=0, width=10, height=10), + vertex(id="b", parent="c", x=50, y=0, width=10, height=10), + edge( + id="e", + parent="1", + source="a", + target="b", + points=[(30, 70)], + ), + ] + ) + out, _changes = fix(xml) + + self.assertEqual(points_of(out, "e"), [("19.5", "69.75")]) + + def test_deeply_nested_origins_add_up(self): + xml = model( + [ + vertex(id="c1", parent="1", x=10, y=20, width=200, height=200), + vertex( + id="c2", parent="c1", x=30, y=40, width=100, height=100 + ), + vertex(id="a", parent="c2", x=50, y=50), + vertex(id="b", parent="c2", x=60, y=60), + edge( + id="e", + parent="1", + source="a", + target="b", + relative=False, + points=[(70, 80)], + ), + ] + ) + out, _changes = fix(xml) + + # (70 - 30 - 10, 80 - 40 - 20) + self.assertEqual(points_of(out, "e"), [("30", "20")]) + + +class NumberFormatTest(unittest.TestCase): + def test_to_float_follows_javascript_parsefloat(self): + self.assertEqual(script.to_float("12"), 12.0) + self.assertEqual(script.to_float(" 12.5 "), 12.5) + self.assertEqual(script.to_float("-3.5e2"), -350.0) + self.assertEqual(script.to_float("12abc"), 12.0) + + def test_to_float_treats_unusable_values_as_zero(self): + for value in (None, "", "abc", "0x10", "NaN"): + self.assertEqual(script.to_float(value), 0.0, value) + + def test_format_number_matches_javascript_stringification(self): + self.assertEqual(script.format_number(120.0), "120") + self.assertEqual(script.format_number(-0.0), "0") + self.assertEqual(script.format_number(12.5), "12.5") + self.assertEqual( + script.format_number(0.1 + 0.2), "0.30000000000000004" + ) + + def test_flag_truthiness(self): + for value in ("1", "true", "false", "yes", "0x10", " 2 "): + self.assertTrue(script.is_true(value), value) + + for value in (None, "", " ", "0", "00", "0.0"): + self.assertFalse(script.is_true(value), value) + + +if __name__ == "__main__": + unittest.main()