From 02ffb516cc129fbf55b66fb538c200a240cb1d06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 12:33:47 +0100 Subject: [PATCH 1/3] fix: refuse XML entity declarations and join text chunks once The validator's XML parser appended character data chunk by chunk, which is quadratic. A ~400-byte file of nested internal entities, or a few MB of escaped text such as x< repeated, made it spin for tens of seconds to hours. Register expat's EntityDeclHandler to refuse entity declarations (plain DOCTYPE stays valid), and collect text chunks per element, joined once. Bump to 0.2.1 and add mutation-test guidance to CONTRIBUTING. --- .claude-plugin/plugin.json | 2 +- CONTRIBUTING.md | 4 +++ plugin.json | 2 +- scripts/plccheck/xmlutil.py | 19 +++++++---- tests/test_formats.py | 64 +++++++++++++++++++++++++++++++++++++ 5 files changed, 83 insertions(+), 8 deletions(-) diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index c87a752..01668c1 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "$schema": "https://anthropic.com/claude-code/plugin.schema.json", "name": "plc-programming", - "version": "0.2.0", + "version": "0.2.1", "description": "PLC programming across IEC 61131-3 (ST, LD, FBD, SFC, IL), PLCopen XML / IEC 61131-10, Siemens TIA (SCL, SimaticML, SIMATIC SD), Schneider (Machine Expert, Control Expert), CODESYS, TwinCAT, ABB Freelance, Rockwell L5X/L5K, IEC 61499, AutomationML, DEXPI, PackML/ISA-88 and OPC UA: design, write, convert, review and debug programs, with a stdlib-only structural validator.", "author": { "name": "SmailG", diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 57cc341..909fbc5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -42,5 +42,9 @@ claude plugin validate . claude plugin eval . --trust-plugin --allow-tools Write Edit "Bash(python3 *)" --no-publish ``` +Run mutation tests (by hand or with an AI agent) one at a time, each as +`timeout -k 10 300 `: an external wrapper, because a timeout inside the runner dies with +it. Check for leftover processes at the end. + Read an eval's `Δ` (with plugin minus without), not its score: a case the baseline already passes shows nothing about the plugin. See the README for the macOS eval workaround. diff --git a/plugin.json b/plugin.json index bfedbe6..b97a096 100644 --- a/plugin.json +++ b/plugin.json @@ -1,7 +1,7 @@ { "name": "plc-programming", "displayName": "PLC Programming", - "version": "0.2.0", + "version": "0.2.1", "description": "PLC programming across IEC 61131-3 (ST, LD, FBD, SFC, IL), PLCopen XML / IEC 61131-10, Siemens TIA, Schneider, CODESYS, TwinCAT, ABB Freelance, Rockwell L5X/L5K, IEC 61499, AutomationML, DEXPI, PackML/ISA-88 and OPC UA with a stdlib-only structural validator.", "author": { "name": "SmailG", diff --git a/scripts/plccheck/xmlutil.py b/scripts/plccheck/xmlutil.py index 4c6b20f..af04584 100644 --- a/scripts/plccheck/xmlutil.py +++ b/scripts/plccheck/xmlutil.py @@ -1,7 +1,7 @@ """Minimal namespace-aware XML tree with line numbers (expat, stdlib only). ElementTree drops line numbers, and a finding without a line is half as useful. -External entities and DTDs are never fetched. +External entities and DTDs are never fetched, and entity declarations are refused. """ from __future__ import annotations @@ -66,6 +66,7 @@ def parse_bytes(data: bytes) -> Node: parser.SetParamEntityParsing(xml.parsers.expat.XML_PARAM_ENTITY_PARSING_NEVER) root: list[Node] = [] stack: list[Node] = [] + chunks: list[list[str]] = [] # character data of each open element, joined once at its end def split(name: str) -> tuple[str, str]: if "}" in name: @@ -81,20 +82,26 @@ def start(name, attrs): else: root.append(node) stack.append(node) + chunks.append([]) def end(_name): - stack.pop() + node = stack.pop() + node.text = "".join(chunks.pop()) def chars(data): if stack: - node = stack[-1] - if not node.text: - node.text_line = parser.CurrentLineNumber - node.text += data + if not chunks[-1]: + stack[-1].text_line = parser.CurrentLineNumber + chunks[-1].append(data) + + def entity_decl(*_args): + # Expanding entities makes the work grow with the nesting, not the file size. + raise XmlError("XML entity declarations are not supported", parser.CurrentLineNumber) parser.StartElementHandler = start parser.EndElementHandler = end parser.CharacterDataHandler = chars + parser.EntityDeclHandler = entity_decl try: parser.Parse(data, True) except xml.parsers.expat.ExpatError as exc: diff --git a/tests/test_formats.py b/tests/test_formats.py index 4f0b871..5bbdab9 100644 --- a/tests/test_formats.py +++ b/tests/test_formats.py @@ -17,6 +17,7 @@ sys.path.insert(0, str(HERE.parent / "scripts")) import plc_validate # noqa: E402 +from plccheck.xmlutil import XmlError, parse_bytes # noqa: E402 PLCOPEN = """ @@ -454,6 +455,69 @@ def test_broken_xml(self): self.assertEqual(codes({"x.TcPOU": ""})[1], ["XML001"]) +class XmlEntityGuard(unittest.TestCase): + """An entity declaration is refused; a bare DOCTYPE and plain text are not.""" + + ENTITY_DOC = '\n\n]>\n&x;\n' + + def refused(self, data: bytes) -> XmlError: + with self.assertRaises(XmlError) as ctx: + parse_bytes(data) + return ctx.exception + + def test_entity_declaration_is_refused_with_its_line(self): + exc = self.refused(self.ENTITY_DOC.encode()) + self.assertEqual(str(exc), "XML entity declarations are not supported") + self.assertEqual(exc.line, 3) + + def test_utf16_entity_declaration_is_refused(self): + exc = self.refused(self.ENTITY_DOC.replace('"1.0"?>', '"1.0" encoding="utf-16"?>').encode("utf-16")) + self.assertEqual((str(exc), exc.line), ("XML entity declarations are not supported", 3)) + + def test_every_declaration_kind_is_refused(self): + kinds = { + "internal": '', + "parameter": '', + "external": '', + "unparsed": '', + } + for name, decl in kinds.items(): + with self.subTest(kind=name): + exc = self.refused(f"".encode()) + self.assertEqual(str(exc), "XML entity declarations are not supported") + + def test_entity_declaration_is_an_xml001_finding(self): + doc = PLCOPEN.replace("]>\n Path: + tmp = tempfile.TemporaryDirectory() + self.addCleanup(tmp.cleanup) + path = Path(tmp.name) / name + path.write_text(content, encoding="utf-8") + return path + + def test_doctype_with_external_dtd_still_validates(self): + dtd = '\n' + fbdk = FBT.replace("" + "x<" * 200 + "intail&\nend").encode() + root = parse_bytes(data) + self.assertEqual(root.text, "x<" * 200 + "tail&cd\nend") + self.assertEqual(root.text_line, 1) + self.assertEqual([c.text for c in root.children], ["in", ""]) + + def test_text_line_is_the_first_chunk_line(self): + root = parse_bytes(b"\n\nlate") + self.assertEqual((root.text, root.text_line), ("\n\nlate", 1)) + root = parse_bytes(b"\n\nx") + self.assertEqual((root.text, root.text_line), ("\n\n", 1)) + + class ShippedTemplates(unittest.TestCase): """Every template a skill hands to the model must itself pass the validator.""" From 12a8ea555171d7a9673514477628149f66fb6a1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 12:47:11 +0100 Subject: [PATCH 2/3] fix: stop the format sniffer at entity declarations and read only regular files for AML links sniff_root parses the document head before parse_bytes does, and had no entity guard: a root attribute or an ATTLIST default that references nested entities expanded to a huge value from a small file. It now stops at the first entity declaration and reports a root that matches no format, so the edit-time hook skips the file like any non-PLC .xml (returning None would make it block edits to ordinary .xml files that declare entities). Validating such a file with an explicit format still reports XML001. An AutomationML refURI that is an absolute path (for example a directory, FIFO or device) was read with read_bytes(). The linked document is now parsed only when the target is a regular file; a target that does not exist still gets AM011. Adds a scaling-ratio test for the text-chunk join, so reverting it to repeated string concatenation fails. --- scripts/plccheck/aml.py | 3 +- scripts/plccheck/xmlutil.py | 23 ++++++++- tests/test_formats.py | 96 +++++++++++++++++++++++++++++++++++-- 3 files changed, 115 insertions(+), 7 deletions(-) diff --git a/scripts/plccheck/aml.py b/scripts/plccheck/aml.py index 82edad6..c3f6ecc 100644 --- a/scripts/plccheck/aml.py +++ b/scripts/plccheck/aml.py @@ -72,7 +72,8 @@ def check_aml(root: Node, path: Path | None = None) -> list[Finding]: continue if not target.exists(): out.append(warning("AM011", f"refURI target {target.name} not found next to this file", value.line)) - elif fragment and cls.endswith("PLCopenXMLInterface"): + elif fragment and cls.endswith("PLCopenXMLInterface") and target.is_file(): + # An absolute refURI can name a directory, FIFO or device; only a regular file is read. try: doc = parse_file(target) except XmlError: diff --git a/scripts/plccheck/xmlutil.py b/scripts/plccheck/xmlutil.py index af04584..0c55b02 100644 --- a/scripts/plccheck/xmlutil.py +++ b/scripts/plccheck/xmlutil.py @@ -1,7 +1,10 @@ """Minimal namespace-aware XML tree with line numbers (expat, stdlib only). ElementTree drops line numbers, and a finding without a line is half as useful. -External entities and DTDs are never fetched, and entity declarations are refused. +External entities and DTDs are never fetched, and entity declarations are refused. Because +entity declarations precede the root element, the format sniffer stops at the first one too: +a file that declares an entity is not detected as any format, so the edit-time hook skips it, +and validating it with an explicit format reports XML001. """ from __future__ import annotations @@ -55,6 +58,10 @@ def all_text(self) -> str: return self.text + "".join(c.all_text() for c in self.children) +class _EntityDeclared(Exception): + """Raised inside sniff_root's parser at the first entity declaration.""" + + class XmlError(Exception): def __init__(self, message: str, line: int | None): super().__init__(message) @@ -116,7 +123,13 @@ def parse_file(path: Path) -> Node: def sniff_root(path: Path) -> tuple[str, str] | None: - """(namespace, local name) of the root element, or None if it is not XML.""" + """(namespace, local name) of the root element, or None if it is not XML. + + A document that declares an entity gives ("", ""), which matches no format: the declaration + comes before the root, so the parser stops before any entity is expanded (a root attribute + or an ATTLIST default could otherwise expand to megabytes). None would read as broken XML, + and the hook would then block edits to ordinary non-PLC .xml files that declare entities. + """ try: head = path.read_bytes()[:4096] except OSError: @@ -131,11 +144,17 @@ def start(name, _attrs): found.append(name) raise StopIteration + def entity_decl(*_args): + raise _EntityDeclared + parser.StartElementHandler = start + parser.EntityDeclHandler = entity_decl try: parser.Parse(head, False) except StopIteration: pass + except _EntityDeclared: + return "", "" except xml.parsers.expat.ExpatError: return None if not found: diff --git a/tests/test_formats.py b/tests/test_formats.py index 5bbdab9..7032694 100644 --- a/tests/test_formats.py +++ b/tests/test_formats.py @@ -10,6 +10,7 @@ import subprocess import sys import tempfile +import time import unittest from pathlib import Path @@ -17,8 +18,9 @@ sys.path.insert(0, str(HERE.parent / "scripts")) import plc_validate # noqa: E402 -from plccheck.xmlutil import XmlError, parse_bytes # noqa: E402 +from plccheck.xmlutil import XmlError, parse_bytes, sniff_root # noqa: E402 +TC6_NS = "http://www.plcopen.org/xml/tc6_0201" PLCOPEN = """ @@ -442,6 +444,24 @@ def test_aml_rules(self): bad = AML.replace('ID="i1"', 'ID="e1"').replace("file:///plc.xml#Main", "file:///missing.xml") self.assertEqual(codes({"plant.aml": bad})[1], ["AM003", "AM004", "AM011"]) + def test_aml_link_to_a_directory_is_skipped_and_to_a_file_is_checked(self): + # A target that exists but is not a regular file is never read. + with tempfile.TemporaryDirectory() as d: + base = Path(d) + (base / "docs").mkdir() + (base / "plc.xml").write_text(PLCOPEN, encoding="utf-8") + targets = { + "relative directory": ("docs#Main", []), + "absolute directory": (f"{base / 'docs'}#Main", []), + "regular file": ("plc.xml#Main", ["AM012"]), + } + for name, (ref, expected) in targets.items(): + with self.subTest(target=name): + aml = base / "plant.aml" + aml.write_text(AML.replace("file:///plc.xml#Main", ref), encoding="utf-8") + fmt, findings = plc_validate.check_file(aml, None) + self.assertEqual((fmt, sorted(f.code for f in findings)), ("aml", expected)) + def test_dexpi_rules(self): bad = DEXPI.replace('ID="pif1"', 'ID="pump1"').replace('ItemID="pump1"', 'ItemID="nothing"') self.assertEqual(codes({"dexpi.xml": bad})[1], ["DX003", "DX004"]) @@ -458,6 +478,7 @@ def test_broken_xml(self): class XmlEntityGuard(unittest.TestCase): """An entity declaration is refused; a bare DOCTYPE and plain text are not.""" + PLCOPEN_ENTITY = PLCOPEN.replace("]>\n XmlError: @@ -486,11 +507,51 @@ def test_every_declaration_kind_is_refused(self): exc = self.refused(f"".encode()) self.assertEqual(str(exc), "XML entity declarations are not supported") - def test_entity_declaration_is_an_xml001_finding(self): - doc = PLCOPEN.replace("]>\n]>\n' + self.assertEqual(sniff_root(self.write("a.xml", doc)), ("", "")) + + def test_sniff_root_does_not_expand_an_entity_used_in_an_attlist_default(self): + doc = f']>\n' + self.assertEqual(sniff_root(self.write("a.xml", doc)), ("", "")) + + def test_utf16_plcopen_with_an_entity_is_xml_broken_then_xml001(self): + # sniff_root only reads ASCII-compatible heads, so a UTF-16 file never reaches the + # entity guard there; parse_bytes then reports the declaration. + doc = self.PLCOPEN_ENTITY.replace('encoding="utf-8"', 'encoding="utf-16"') + path = self.write("a.xml", "") + path.write_bytes(doc.encode("utf-16")) + fmt, findings = plc_validate.check_file(path, None) + self.assertEqual((fmt, [(f.code, f.line) for f in findings]), ("xml-broken", [("XML001", 2)])) + + def test_entity_text_outside_a_declaration_is_not_refused(self): + entity = '' + docs = { + "comment": f'', + "comment in subset": f']>', + "processing instruction": f'', + "cdata": f'', + "element, attlist, notation": ( + '' + f']>'), + } + for name, doc in docs.items(): + with self.subTest(case=name): + self.assertEqual(parse_bytes(doc.encode()).tag, "project") + self.assertEqual(sniff_root(self.write("a.xml", doc)), (TC6_NS, "project")) + def write(self, name: str, content: str) -> Path: tmp = tempfile.TemporaryDirectory() self.addCleanup(tmp.cleanup) @@ -518,6 +579,33 @@ def test_text_line_is_the_first_chunk_line(self): self.assertEqual((root.text, root.text_line), ("\n\n", 1)) +class XmlTextJoinScaling(unittest.TestCase): + """Text split into many chunks (one per entity reference) must parse in linear time.""" + + RUNS = 3 + SMALL, BIG = 40_000, 160_000 # BIG is 4x SMALL: linear work is about 4x, quadratic about 16x + MAX_RATIO = 8 + TIMER_FLOOR = 0.01 # seconds; keeps the ratio meaningful if the small run is below timer noise + + def best_of_runs(self, n: int) -> float: + data = (f'' + "x<" * n + "").encode() + best = float("inf") + for _ in range(self.RUNS): + start = time.perf_counter() + parse_bytes(data) + best = min(best, time.perf_counter() - start) + return best + + def test_text_join_time_grows_linearly_with_the_chunk_count(self): + # A ratio, not an absolute bound, so a slow CI runner does not flake: both sizes slow + # down together. Best of three drops a run that a scheduling hiccup inflated, which would + # otherwise push the ratio up. Appending each chunk to node.text copies the text so far + # each time (quadratic) and lands near 16x; one join per element lands near 4x. + t_small, t_big = self.best_of_runs(self.SMALL), self.best_of_runs(self.BIG) + self.assertLess(t_big, self.MAX_RATIO * max(t_small, self.TIMER_FLOOR), + f"{self.SMALL} chunks took {t_small:.3f}s, {self.BIG} chunks took {t_big:.3f}s") + + class ShippedTemplates(unittest.TestCase): """Every template a skill hands to the model must itself pass the validator.""" From f1cd0470426ab2d9576519189e6d9b5078e834cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 12:47:11 +0100 Subject: [PATCH 3/3] docs: name both plugin manifests in the version rule and test that they agree The version lives in .claude-plugin/plugin.json and the root plugin.json, but CONTRIBUTING.md and the PR template named only the first. Both now name both, and a test asserts the two version fields are equal. The mutation-test paragraph moves under its own heading after the eval paragraph, with the GNU timeout note for macOS and a hint for finding leftover processes. --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- CONTRIBUTING.md | 16 ++++++++++------ tests/test_skills.py | 9 +++++++++ 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 61ab843..6a7f7ee 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -11,5 +11,5 @@ - [ ] `python3 -m unittest discover -s tests` passes - [ ] A new validator rule has a must-catch input and a valid control that must pass - [ ] New facts in a reference file cite their source, or are marked unverified -- [ ] Version bumped in `.claude-plugin/plugin.json` +- [ ] Version bumped in both `.claude-plugin/plugin.json` and the root `plugin.json` - [ ] README updated if a skill, command or validator rule changed diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 909fbc5..517b174 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -28,8 +28,9 @@ privately, as described in [SECURITY.md](SECURITY.md). Pull requests use the tem ask the user rather than assume. - **The hook must never break a session:** non-PLC files and unreadable events exit 0, and a validator bug exits 1, which Claude Code shows but does not block on. Only real errors exit 2. -- **Bump the version** in `.claude-plugin/plugin.json` on every change; `claude plugin update` - does nothing while it is unchanged. +- **Bump the version** in both `.claude-plugin/plugin.json` and the root `plugin.json` on every + change, to the same value (a test checks it); `claude plugin update` does nothing while it is + unchanged. - Do not bundle vendor schemas or manuals whose licence does not clearly permit redistribution. - **Test fixtures for the `.project` reader are synthetic.** Never commit a real `.project`, or names, text or guids taken from one: it is someone's program. @@ -42,9 +43,12 @@ claude plugin validate . claude plugin eval . --trust-plugin --allow-tools Write Edit "Bash(python3 *)" --no-publish ``` -Run mutation tests (by hand or with an AI agent) one at a time, each as -`timeout -k 10 300 `: an external wrapper, because a timeout inside the runner dies with -it. Check for leftover processes at the end. - Read an eval's `Δ` (with plugin minus without), not its score: a case the baseline already passes shows nothing about the plugin. See the README for the macOS eval workaround. + +### Mutation tests + +Run mutation tests (by hand or with an AI agent) one at a time, each as +`timeout -k 10 300 `: an external wrapper, because a timeout inside the runner dies with +it. That is GNU `timeout` (macOS: `brew install coreutils`, then `gtimeout`). Check for +leftover processes at the end: `ps -eo pid,etime,pcpu,command | grep `. diff --git a/tests/test_skills.py b/tests/test_skills.py index 30b0201..c4f2f0c 100644 --- a/tests/test_skills.py +++ b/tests/test_skills.py @@ -6,6 +6,7 @@ from __future__ import annotations +import json import re import sys import unittest @@ -97,6 +98,14 @@ def test_every_iecst_block(self): self.assertGreater(blocks, 10, "no iecst blocks found; the fence pattern is wrong") +class PluginManifests(unittest.TestCase): + def test_both_manifests_carry_the_same_version(self): + versions = {str(rel): json.loads((ROOT / rel).read_text(encoding="utf-8"))["version"] + for rel in (Path("plugin.json"), Path(".claude-plugin") / "plugin.json")} + self.assertEqual(len(versions), 2) + self.assertEqual(len(set(versions.values())), 1, versions) + + class CrossReferences(unittest.TestCase): def test_relative_links_resolve(self): link = re.compile(r"\[[^\]]*\]\(([^)\s#]+)(?:#[^)]*)?\)")