From 11a54e6bd86ed9449f9ba5292acfb568aab7b027 Mon Sep 17 00:00:00 2001 From: Sebastian Tramp Date: Tue, 13 Oct 2026 16:43:31 +0200 Subject: [PATCH 1/2] update to cmem-plugin-template v9.9.0 Replays v9.5.0 -> v9.9.0. TRY400 joins the ruff ignore list because PluginLogger has no exception(), PLR2004 is relaxed under tests/ because an assertion's expected value is the literal it asserts, .gitlab-ci.yml learns to include .gitlab-ci-custom.yml when that file exists, and the agent skills and rules shipped under .claude/ are updated. Verified against a project freshly rendered from the same answers: pyproject.toml differs only by this project's own dependencies, and no list valued setting lost an entry. task check:ruff, check:mypy and check:trivy pass here; pytest is left to the pipeline. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VbqmemAwRmKUxpCncz68L6 --- .claude/hooks/template-feedback.py | 284 +++++++++++++----- .claude/rules/copier-template.md | 16 +- .claude/settings.json | 2 +- .claude/skills/copier-update/SKILL.md | 38 +++ .claude/skills/plugin-documentation/SKILL.md | 16 +- .claude/skills/plugin-implementation/SKILL.md | 269 ++++++++++++++++- .claude/skills/plugin-testing/SKILL.md | 26 ++ .copier-answers.yml | 2 +- .github/workflows/check.yml | 13 +- Taskfile.yaml | 80 +++-- pyproject.toml | 9 + 11 files changed, 649 insertions(+), 106 deletions(-) diff --git a/.claude/hooks/template-feedback.py b/.claude/hooks/template-feedback.py index 6c89b37..040637f 100644 --- a/.claude/hooks/template-feedback.py +++ b/.claude/hooks/template-feedback.py @@ -1,18 +1,25 @@ #!/usr/bin/env python3 """Notice friction with the cmem-plugin-template at the end of a session. -This is a Claude Code ``Stop`` hook, wired up in ``.claude/settings.json`` and -run through ``task template:feedback-check``. It belongs to the template - see -``.claude/rules/copier-template.md``. +This is a Claude Code ``Stop`` hook. ``.claude/settings.json`` runs it +directly, and not through a task, because a task runner writes to stdout and +picks its own exit codes - and stdout is this hook's JSON channel. It reads the +hook payload from stdin, so a hand run needs one:: + + echo '{}' | python3 .claude/hooks/template-feedback.py + +It belongs to the template - see ``.claude/rules/copier-template.md``. It says nothing at all unless the session left evidence that a template owned file got in the way, and then it asks the agent to consider whether that is worth reporting upstream. Deciding there is nothing to report is a valid answer; the hook fires at most once per session either way. -Two rules govern everything below. Print **nothing** unless there is something -to say, because stdout is the hook's JSON channel. And always exit 0, whatever -goes wrong - a broken hook must never keep a session from ending. +Three rules govern everything below. Print **nothing** unless there is +something to say, because stdout is the hook's JSON channel. Always exit 0, +whatever goes wrong - a broken hook must never keep a session from ending. And +when a fact cannot be established, drop the evidence rather than guessing: a +false positive blocks a session, a false negative only misses one report. This file is not part of the package and is not covered by ``task check``, so keep it small, dependency free and readable. @@ -23,6 +30,7 @@ import subprocess import sys import tempfile +import tomllib from pathlib import Path OPT_OUT = Path(".claude") / "no-template-feedback" @@ -38,16 +46,25 @@ "Taskfile.yaml", ) -# An added line silencing a check. The rules forbid these outright, so a new -# one always means a rule lost an argument with real code. -SILENCER = re.compile(r"^\+.*#\s*(noqa|type:\s*ignore)") +# The one template owned file that *documents* what a conflict looks like, in +# a fenced example. Every copier update that rewrites it adds those markers to +# the diff, which would block the session during the very workflow this hook +# exists to support. +CONFLICT_PROSE = ".claude/skills/copier-update/SKILL.md" -# A rule code added to a list in pyproject.toml - almost always the ruff -# ignore list growing. -RUFF_RULE = re.compile(r'^\+\s*"[A-Z]{1,4}[0-9]{1,4}"') +# Suffixes the suppression comments below can appear in at all. Without this, +# a CHANGELOG entry or a rules file merely naming `# noqa` counts as one. +PYTHON_SUFFIXES = (".py", ".pyi") -# What copier leaves behind when an update could not be merged. -CONFLICT = re.compile(r"^\+?<{7} ") +# A comment silencing a check. The rules forbid these outright, so a new one +# means a rule lost an argument with real code. Counted per file against the +# removed side, so editing a line that already carried one is not a finding. +SILENCER = re.compile(r"#\s*(noqa|type:\s*ignore)") + +# What copier leaves behind when an update could not be merged. Only an added +# line counts; a marker already committed is this project's problem, not the +# template's. +CONFLICT = re.compile(r"^\+<{7} ") # A changed `_commit` in the copier answers file. Its presence means the # working tree *is* a copier update, so the template owned files it rewrote @@ -58,114 +75,243 @@ # it does even when the output is a pipe. ANSI = re.compile(r"\x1b\[[0-9;]*m") +# Enough of a file to find a suppression in, without reading a data fixture +# somebody left untracked. +READ_LIMIT = 512 * 1024 + +# The shortest useful `git status --porcelain` record: "XY " and one character +# of path. +MIN_STATUS_FIELD = 4 + TIMEOUT = 10 -def git(*args: str) -> str: - """Run a git command and return its plain output, or "" on any failure. +def git(*args: str) -> str | None: + """Run a git command and return its plain output, or None if it failed. + + "Failed" and "said nothing" must stay apart. `git diff HEAD` exits non-zero + on an unborn HEAD, and a caller reading that as an empty diff would go half + blind - seeing every untracked file through `git status` while believing + nothing had changed. The same applies when git is missing from PATH or + another process holds `index.lock`. - Colour is both switched off and stripped afterwards. ``color.diff`` is a - common setting, it outranks ``color.ui``, and it paints diff output even - into a pipe - which would leave every pattern above matching nothing at - all, silently. + The configuration passed here pins the output format the parsers assume: + colour off (`color.diff` outranks `color.ui` and paints even into a pipe), + the `a/` and `b/` diff prefixes on, and paths unquoted. """ + command = [ + "git", + "--no-pager", + "-c", + "color.ui=false", + "-c", + "core.quotePath=false", + "-c", + "diff.noprefix=false", + "-c", + "diff.mnemonicPrefix=false", + *args, + ] try: result = subprocess.run( - ["git", "--no-pager", "-c", "color.ui=false", *args], + command, capture_output=True, text=True, timeout=TIMEOUT, check=False, ) except (OSError, subprocess.SubprocessError): - return "" - return ANSI.sub("", result.stdout) if result.returncode == 0 else "" + return None + if result.returncode != 0: + return None + return ANSI.sub("", result.stdout) -def attributed(diff: str) -> list[tuple[str, str]]: - """Pair every diff line with the file it belongs to. +def entries(status: str) -> list[tuple[str, str]]: + """Return the (status, path) pairs of a `git status --porcelain -uall -z`. - A flat scan of a diff cannot tell a project's own code from a template - owned file that merely *writes about* the thing being looked for - the - rules file discusses `# noqa`, and matching that prose reports the template - to itself. + `-z` is what makes this safe to parse: paths arrive verbatim, so neither + `core.quotePath` nor a space in a name can defeat a prefix or suffix test. + A rename is emitted as the new path followed by the original one, and only + the new path is of any interest here. """ + fields = [field for field in status.split("\0") if field] pairs = [] - path = "" + index = 0 + while index < len(fields): + field = fields[index] + index += 1 + if len(field) < MIN_STATUS_FIELD: + continue + code, path = field[:2], field[3:] + pairs.append((code, path)) + if code[0] in "RC" or code[1] in "RC": + index += 1 # the original path of a rename or copy + return pairs + + +def attributed(diff: str) -> list[tuple[str | None, str]]: + """Pair every content line of a diff with the file it belongs to. + + A flat scan cannot tell a project's own code from a template owned file + that merely *writes about* the thing being looked for - the rules file + discusses `# noqa`, and matching that prose reports the template to itself. + + A line whose file could not be established is paired with None, and every + caller drops those. That happens for a deletion, whose `+++` side is + `/dev/null`, and it is the safe direction: unattributed evidence is no + evidence. + """ + pairs: list[tuple[str | None, str]] = [] + path: str | None = None + in_header = False for line in diff.splitlines(): - if line.startswith("+++ b/"): - # Git pads the path with a tab, and quotes it when it contains - # spaces - which template owned paths do not, in a rendered project. - path = line[6:].split("\t")[0].strip().strip('"') - else: + if line.startswith("diff --git "): + path, in_header = None, True + elif not in_header: pairs.append((path, line)) + elif line.startswith("@@"): + in_header = False + elif line.startswith("+++ b/"): + # Git pads the path with a tab when the name needs it. + path = line[len("+++ b/") :].split("\t")[0] return pairs -def marker_for(session_id: str) -> Path: - """Return the once-per-session marker file for a session id.""" - safe = re.sub(r"[^A-Za-z0-9_-]", "", session_id)[:64] or "unknown" - return Path(tempfile.gettempdir()) / f"cmem-template-feedback-{safe}" +def owned(path: str | None) -> bool: + """Say whether a path is one the template writes.""" + return path is not None and path.startswith(TEMPLATE_OWNED) + + +def added_silencers(pairs: list[tuple[str | None, str]], untracked: list[str]) -> bool: + """Say whether this project gained a suppression comment. + + Three things have to line up. The file must be one this project authored - + a `# noqa` inside a template owned file was written by the template. It + must be Python, or prose naming the comment counts as using it. And the + file must end up with more of them than it started with, so editing a line + that has always carried one is not a finding. + + Untracked files are read from disk, because `git diff HEAD` does not list + them and writing a new module is the most common way a suppression arrives. + """ + counts: dict[str, int] = {} + for path, line in pairs: + if owned(path) or path is None or not path.endswith(PYTHON_SUFFIXES): + continue + if not SILENCER.search(line[1:]): + continue + if line.startswith("+"): + counts[path] = counts.get(path, 0) + 1 + elif line.startswith("-"): + counts[path] = counts.get(path, 0) - 1 + if any(count > 0 for count in counts.values()): + return True + for path in untracked: + if owned(path) or not path.endswith(PYTHON_SUFFIXES): + continue + try: + text = Path(path).read_text(encoding="utf-8", errors="replace")[:READ_LIMIT] + except OSError: + continue + if SILENCER.search(text): + return True + return False def collect_evidence() -> list[str]: """Return human readable evidence of template friction, newest first.""" - evidence = [] - - status = git("status", "--porcelain") + status = git("status", "--porcelain", "-uall", "-z") diff = git("diff", "HEAD") - diff_lines = diff.splitlines() + if status is None or diff is None: + return [] + + pairs = attributed(diff) + listed = entries(status) + evidence = [] # A `copier update` rewrites every template owned path by definition. Do not # report the act of taking a new template version as friction with it - a # suppression written by hand while resolving an update conflict is still a # real finding, and is caught below because it lands in this project's own # source rather than in a template owned file. - updating = any(COPIER_UPDATE.match(line) for line in diff_lines) - - touched = sorted( - { - path - for line in status.splitlines() - for path in [line[3:].split(" -> ")[-1].strip()] - if path.startswith(TEMPLATE_OWNED) - } - ) + updating = any(COPIER_UPDATE.match(line) for _, line in pairs) + + touched = sorted({path for _, path in listed if path.startswith(TEMPLATE_OWNED)}) if touched and not updating: evidence.append(f"template owned files were changed here: {', '.join(touched)}") - rejects = [ - line[3:].strip() for line in status.splitlines() if line[3:].strip().endswith(".rej") - ] + rejects = sorted({path for _, path in listed if path.endswith(".rej")}) if rejects: evidence.append(f"a copier update left rejected hunks behind: {', '.join(rejects)}") - # Only in code this project actually authored. A `# noqa` inside a template - # owned file was written by the template, so it can never be evidence of - # this project working around anything. - if any( - SILENCER.match(line) - for path, line in attributed(diff) - if not path.startswith(TEMPLATE_OWNED) - ): + if added_silencers(pairs, [path for code, path in listed if code == "??"]): evidence.append("a '# noqa' or '# type: ignore' was added") # Conflict markers are the opposite case: one inside a template owned file # is precisely the "the update could not be merged" report worth having. - if any(CONFLICT.match(line) for line in diff_lines): + if any( + CONFLICT.match(line) + for path, line in pairs + if path is not None and path != CONFLICT_PROSE + ): evidence.append("conflict markers are still in the working tree") # `pyproject.toml` is not template owned, but its lint configuration is # rendered - so during an update a new ignore entry arrived with the # template rather than being chosen here. - if not updating and any( - RUFF_RULE.match(line) for line in git("diff", "HEAD", "--", "pyproject.toml").splitlines() - ): - evidence.append("a lint rule was added to the ignore list in pyproject.toml") + try: + current = Path("pyproject.toml").read_text(encoding="utf-8") + except OSError: + current = None + silenced = set() if updating else added_ignores(git("show", "HEAD:pyproject.toml"), current) + if silenced: + evidence.append( + "these lint rules joined the ruff ignore list in pyproject.toml: " + + ", ".join(sorted(silenced)) + ) return evidence +def added_ignores(before: str | None, after: str | None) -> set[str]: + """Return the rule codes `[tool.ruff.lint] ignore` gained, comparing two files. + + Reading the table is the whole point. Matching a quoted rule code against + the diff cannot tell which table it is in, so it fired on `extend-select`, + which *tightens* linting, on a `[tool.deptry]` entry, and on the + `per-file-ignores` relaxation for tests that the shipped rules prescribe - + reporting the project for doing what it was told. + + `per-file-ignores` is deliberately not looked at for that reason. Anything + unreadable on either side yields no codes, because a suppression that + cannot be established is not evidence. + """ + + def ignores(source: str | None) -> set[str] | None: + if source is None: + return None + try: + lint = tomllib.loads(source)["tool"]["ruff"]["lint"] + except (tomllib.TOMLDecodeError, ValueError, KeyError, TypeError): + return None + listed = lint.get("ignore") if isinstance(lint, dict) else None + if not isinstance(listed, list): + return set() + return {code for code in listed if isinstance(code, str)} + + old, new = ignores(before), ignores(after) + if old is None or new is None: + return set() + return new - old + + +def marker_for(session_id: str) -> Path: + """Return the once-per-session marker file for a session id.""" + safe = re.sub(r"[^A-Za-z0-9_-]", "", session_id)[:64] or "unknown" + return Path(tempfile.gettempdir()) / f"cmem-template-feedback-{safe}" + + def main() -> None: """Read the hook payload, and block once if there is something to report.""" try: diff --git a/.claude/rules/copier-template.md b/.claude/rules/copier-template.md index 1ae835f..61277e6 100644 --- a/.claude/rules/copier-template.md +++ b/.claude/rules/copier-template.md @@ -13,7 +13,10 @@ the next `copier update` either reverts them or turns them into a merge conflict. Add project specific build steps to `TaskfileCustom.yaml` instead - note the -`.yaml` spelling, since only that one is included. Project specific agent +`.yaml` spelling, since only that one is included. A GitLab pipeline takes +project specific jobs from `.gitlab-ci-custom.yml` the same way, included when +that file exists; it can add to a templated job but not redefine one, and +`.github/workflows/` has no equivalent hook. Project specific agent instructions belong in `CLAUDE.md`, which the template never writes and which is read alongside these rules. Personal tool permissions belong in `.claude/settings.local.json`, which is git-ignored. @@ -49,9 +52,20 @@ silence the rule for code nobody has looked at. Report it with the `template-feedback` skill too, since a rule that is wrong here is usually wrong in other projects as well. +One class of that is already settled and needs neither a human nor a report: +`PLR0913` and `PLR0917` on a signature whose arity a framework fixes rather +than the author - a plugin constructor with one argument per `PluginParameter`, +a Click command with one per option. Suppress both on the `def`, with the +reason above it. Neither escape ruff suggests is available, since keyword-only +arguments answer `PLR0917` but not `PLR0913`, and folding the arguments into +one object breaks the one-argument-per-option mapping the framework relies on. + ## Every user visible change gets a changelog entry `CHANGELOG.md` follows [Keep a Changelog](https://keepachangelog.com/). Add an entry under `## [Unreleased]` in the matching `### Added`, `### Changed`, `### Fixed` or `### Removed` section. The trigger is whether a user would notice, not whether behaviour changed. + +Do not cite an issue or Jira ticket. The people reading this file cannot open +them, so the entry has to stand on its own. diff --git a/.claude/settings.json b/.claude/settings.json index 81b4dd4..b2845b5 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -30,7 +30,7 @@ "hooks": [ { "type": "command", - "command": "task template:feedback-check", + "command": "python3 .claude/hooks/template-feedback.py", "timeout": 30 } ] diff --git a/.claude/skills/copier-update/SKILL.md b/.claude/skills/copier-update/SKILL.md index ab86916..ac66f16 100644 --- a/.claude/skills/copier-update/SKILL.md +++ b/.claude/skills/copier-update/SKILL.md @@ -63,6 +63,44 @@ the template actually changed, such as a bumped dependency constraint. Example files that were deleted after generation can reappear when the template changes them. Delete them again. +## What the merge will not tell you + +A conflict is raised only where copier could not decide. The cases that cost +the most time are the silent ones, and both directions have happened: + +- **The template's new value is not applied.** The template set + `cmem-cmemc` to `^25.3.0` in v7.3.0 and lowered it again to `>=24.2.0` in + v8.0.0. Four repositories updated straight through that release and on to + v9.6.1, and every one still carried the caret afterwards. No commit in them + re-introduced it and no conflict was ever raised. It stayed harmless for a + year, until click 8.5 began rejecting an empty colour that cmemc 25 passes + through - and then several test suites failed at once with + `Unknown color ''`. +- **This project's value is replaced by the template's.** A project carrying its + own `[tool.ruff.lint] ignore` and `per-file-ignores` entries lost them + wholesale to the template's shorter lists, and the update appeared to add 409 + lint findings. They were not debt; the configuration that had silenced them + was gone. + +So do not read "no conflicts" as "nothing to check". After updating, look at +`pyproject.toml` deliberately - at dependency constraints, and at every +list-valued setting, because a list is replaced whole rather than merged +entry by entry. + +The reliable check is to render a throwaway project from the same answers and +diff against it, which shows both what should have arrived and what should not +have: + +```bash +copier copy --trust --defaults --vcs-ref \ + -d project_slug= -d project_type= \ + gh:eccenca/cmem-plugin-template /tmp/reference +diff /tmp/reference/pyproject.toml pyproject.toml +``` + +Treat a sudden jump in lint findings as suspected configuration loss rather +than real debt, and check that first. + ## After updating ```bash diff --git a/.claude/skills/plugin-documentation/SKILL.md b/.claude/skills/plugin-documentation/SKILL.md index 4ef700e..d4b5fbd 100644 --- a/.claude/skills/plugin-documentation/SKILL.md +++ b/.claude/skills/plugin-documentation/SKILL.md @@ -11,8 +11,8 @@ This governs the text a user reads inside eccenca Corporate Memory: the `label`, Python docstrings, error messages or identifiers. Those are read by developers and follow the ordinary code conventions. -`documentation` is rendered as Markdown, so backticks, `**bold**` and links all -work. `description` is a single line shown next to the task in the task list. +`documentation` is rendered as Markdown, so backticks, `**bold**`, links, `##` +headings and lists all work. `description` is a single line shown next to the task in the task list. ## Ground every claim in the code @@ -66,6 +66,18 @@ lines. Do not pad a simple task to match a complex one, and do not cut a complex one down to match a simple one. A task with three mutually exclusive modes genuinely needs more words than one with a single input port. +Mark the beats by length, not by taste. While the block is short - which for +most tasks it is - write it as one run of paragraphs and let the beats follow +each other unannounced. Once it passes roughly twenty lines, give each beat a +`##` heading named after it, so a reader scans for the beat they need instead of +reading until they find it. The caveats beat is the one that stops working as +prose first, and reads best as a list with the claim in bold at the front of +each item. + +Letting the length decide is what keeps sibling tasks in one package looking +like one product. A user meets several of them in a workspace, and two +documentation styles there read as two different things. + ## Parameters The first sentence says what the parameter controls. Add further sentences only diff --git a/.claude/skills/plugin-implementation/SKILL.md b/.claude/skills/plugin-implementation/SKILL.md index 231baf0..7897ada 100644 --- a/.claude/skills/plugin-implementation/SKILL.md +++ b/.claude/skills/plugin-implementation/SKILL.md @@ -46,6 +46,39 @@ use `setup_cmempy_user_access()`. Plenty of existing plugins still call it - that is legacy, not a pattern to copy. `cmem-plugin-base` continues to depend on `cmem-cmempy` transitively, which does not make it available for new code. +## The plugin identifier + +Set `plugin_id` explicitly on every `@Plugin`: + +```python +@Plugin( + label="Create thing", + plugin_id="cmem_plugin_example-CreateThing", + ... +) +``` + +Left unset, it is generated from where the code happens to sit - +`generate_id(module_path + "-" + ClassName)`, so a task in +`cmem_plugin_example/tasks/create.py` is called +`cmem_plugin_example-tasks-create-CreateThing`. A workflow references a plugin +by that identifier, so moving the module or renaming the class changes the +identity of an already deployed task and orphans every workflow task built on +it. That turns an ordinary refactoring into a breaking change, without an error +anywhere. + +Name it `-`. The prefix carries the weight - it keeps the +identifier distinct from every other plugin in the same workspace - and +`` is the class name, or a shortened form where the class repeats +something the package name already says: `cmem_plugin_ssh-Download` for +`DownloadFiles`. An identifier you supply bypasses `generate_id()` and is +therefore never sanitised, so keep it to `[a-zA-Z0-9_-]` yourself. + +**For a task that is already deployed, write down the identifier it generates +today, verbatim** - work it out with the formula above rather than inventing a +tidier one. The point of setting the field is to stop the identifier moving; a +first commit that changes it breaks exactly what it is there to protect. + ## Logging The base class already provides a logger as `self.log`. Use it: @@ -65,7 +98,12 @@ formatted string. The `logging` idiom `self.log.info("Fetched %s records", count)` raises `TypeError` at runtime, so an f-string is the form to use - the ruff rule `G004` (logging statement uses f-string) is in the `ignore` list accordingly. -The same gap is why `self.log` cannot be passed as a `logger` argument. +There is no `exception()` either, so a failure logged inside an `except` block +uses `error()` - ruff's `TRY400` asks for `logging.exception` there, and is in +the `ignore` list for the same reason as `G004`. Do not downgrade the call to +`warning()` to quieten the rule: that understates a failure that is usually +about to be re-raised. The same gap is why `self.log` cannot be passed as a +`logger` argument. ## The icon @@ -106,10 +144,95 @@ input_ports=FixedNumberOfInputs([FixedSchemaPort(schema=MY_SCHEMA)]), output_port=FixedSchemaPort(schema=MY_SCHEMA), ``` -Use `UnknownSchemaPort` when the schema is only known at runtime, and -`FlexibleNumberOfInputs` when the task genuinely accepts any number of inputs. A task that consumes nothing declares `FixedNumberOfInputs([])`. +Two independent things are being declared here, and they are easy to confuse. +`FixedNumberOfInputs` and `FlexibleNumberOfInputs` say how many inputs the task +takes. `FixedSchemaPort`, `FlexibleSchemaPort` and `UnknownSchemaPort` say what +a single port's schema is. `FlexibleNumberOfInputs` makes every one of its +inputs a flexible schema port, which is why the two get read as one choice. + +Prefer a **fixed** schema on every port, and reach for a flexible one only when +the task really cannot know its schema. A fixed schema lets DataIntegration +check a connection while the workflow is being drawn, which turns a runtime +abort into an error the author sees in the editor. + +A flexible **input** schema costs more than it looks - whether it is written +`FixedNumberOfInputs([FlexibleSchemaPort()])` or `FlexibleNumberOfInputs()`. +An operator declaring one has been seen to reject a file dataset outright, +aborting before it receives a single entity, with an entity count of 0 and + +```text +array assignment index out of range: 0 +``` + +A flexible port requests no paths, and reading a file dataset with an empty +requested schema appears to be the trigger - the same file read by a transform, +which requests named paths, is fine. The message names an array index, so it +reads like a bug in the plugin rather than a schema negotiation that never +happened. If a "process whatever arrives" task has to exist, say in its +documentation that its input comes from another task rather than from a dataset. + +`UnknownSchemaPort` is the safer of the two escapes: it says the schema is not +known in advance, without asking DataIntegration to adapt this port to whatever +is connected. + +A port can also depend on a parameter's current value instead of being fixed +at write time. Assign `input_ports`/`output_port` in `__init__` from a +condition on `self`, and the port the editor shows follows the parameter: + +```python +self.input_ports = ( + FixedNumberOfInputs([]) + if self.source_file.strip() + else FixedNumberOfInputs([FixedSchemaPort(schema=MY_SCHEMA)]) +) +``` + +This is the standard way to offer two mutually exclusive ways of supplying the +same thing - a parameter here versus a connected input - rather than accepting +both and picking one at runtime. A boolean or an enum parameter drives the same +pattern with an `if`/`match` in place of the empty-string check. + +### What `execute()` receives + +The declaration settles what `inputs` can hold, and the two directions are not +symmetric. + +Under `FixedNumberOfInputs` it never carries **more** than the declared number +of ports. The workflow editor offers exactly the handlers a task declares, so a +one-port task cannot be connected to two upstream tasks. Code that warns about +or trims a surplus is unreachable from a workflow, and it especially must not +be mentioned in the task's `documentation`: telling a user to avoid a state they +cannot configure is worse than saying nothing. Calling `execute()` directly from +Python bypasses the editor entirely, which is why a test - or a code review +reading the body alone - can "reproduce" the surplus and make a non-issue look +like a defect. + +It can carry **fewer**. A declared port the workflow leaves unconnected is +absent from the sequence rather than arriving as an empty `Entities`, and +DataIntegration runs the workflow anyway, so `inputs[0]` raises `IndexError` +on a task whose one port nobody wired. That case is real, and each task decides +what it means: + +```python +entities = list(inputs[index].entities) if index < len(inputs) else [] +``` + +reads an unconnected port as an empty one - the right call when input is an +optional addition to what parameters already supply. Where the input is the +whole point, fail with a message naming the cause instead: + +```python +if not inputs: + raise ValueError("No input was given.") +``` + +Both are used across the fleet. With several declared ports, do not additionally +assume that a partly connected task keeps each remaining port at its declared +index; drive the loop from `len(inputs)` or accept only what you can identify +from the entities themselves. + ## Honouring cancellation A long-running task must stop when the user cancels the workflow. Check the @@ -127,7 +250,9 @@ for entity in inputs[0].entities: The `suppress(AttributeError)` is required, not defensive noise: `context.workflow` is absent in some contexts - notably the test contexts - and -an unguarded check raises there while working in production. +an unguarded check raises there while working in production. The `inputs[0]` in +front of it is shorthand for a task whose port is connected; see *What +`execute()` receives* for the case where it is not. ## Reporting progress @@ -152,6 +277,54 @@ context.report.update( the work is finished shows a user nothing while the task is running, which is exactly when they are looking. +## The constructor decides the form, not the decorator list + +The `parameters=[...]` list in `@Plugin` reads as the order a user sees on the +task form. It is not. The descriptor is built by walking +`inspect.signature(__init__)` and looking each argument name up in the decorator +list, so the **constructor's argument order is the form's field order** and the +decorator list is only a lookup table for labels, descriptions and types. + +Two things follow: + +- Reordering the decorator list changes nothing a user can see, which is + unpleasant to debug because the file you edited is the one that looks + authoritative. Keep the two lists in the same order and reorder them together, + so the source reads the way the form does. +- A decorator entry whose name matches no constructor argument is **dropped + without an error**. A renamed argument therefore silently takes its label and + description with it, and the field appears bare rather than missing. + +Order the constructor the way the decision reads: a parameter goes before the +ones that depend on it, and above the flag that switches it on. Reordering is +safe because the framework instantiates by keyword - but check that nothing +constructs the class positionally first, which tests often do. + +## The one parameter name the task form claims + +Do not name a parameter `tags`. Every task dialog carries a "Tags" field for the +item's own tags, and that widget's value arrives on a plugin parameter of the +same name in place of the string the parameter declares, so saving the task - or +any other task action - fails with: + +```text +Invalid value for plugin parameter 'tags' of plugin '-'. +Got a nested object with the keys createdItems, selectedItems, but expected a string. +``` + +An empty tag list fails just as well, because the widget still posts both keys +as empty lists. Nothing local catches it: discovery succeeds, the descriptors +show the parameter present, labelled and typed as a string, and `task check` is +green - the failure exists only against a deployment, in the dialog. + +Name the parameter for what it carries - `notification_tags`, `issue_tags` - and +keep the user-visible wording in the `label`, which can still read "Tags". + +`label` and `description` are **not** claimed, although the dialog shows the +item's own fields under those names as well. `cmem-plugin-shapes`, +`cmem-plugin-metadata` and `cmem-plugin-vocab` all ship a parameter called +`label`, and `cmem-plugin-metadata` one called `description`. + ## A constructor with six or more parameters Ruff's `PLR0913` and `PLR0917` both complain about a long argument list, and a @@ -195,6 +368,57 @@ The value arrives as a `Password`; call `.decrypt()` only where it is used. Typing it as `str` puts the secret in plain text in the task configuration and in the project export. +An **empty secret field is still truthy**. A blank parameter arrives as a +`Password` wrapping an empty value, and `Password` defines neither `__bool__` +nor `__len__`, so the obvious guard never fires: + +```python +if self.api_key: # True even when the field was left blank + ... +``` + +A plugin whose behaviour depends on whether a secret was supplied takes the +wrong branch, and the failure surfaces much later - as a library error deep +inside whatever consumes the secret, or as a request sent with an empty +credential. Ask the question against the decrypted value, through a helper that +also accepts the plain `str` that a default value or a test supplies, and keep +the answer rather than the plaintext: + +```python +def _secret(value: Password | str) -> str: + return value if isinstance(value, str) else value.decrypt() + +self.signing = bool(_secret(private_key)) # keep the answer, not the secret +``` + +Tests hide this rather than catching it: a test passing a plain `str` exercises +the naive guard correctly, so the mistake only appears against a deployment, +where DataIntegration hands over a `Password`. + +## Validating an IRI the user types + +A knowledge graph IRI **is not necessarily a URL**. `urn:example:data` and +`urn:uuid:...` are ordinary ways to name a graph, and the store imports into and +queries one without complaint - so `validators.url()`, the obvious import, +rejects a graph that works. The trap is quiet: the check looks right, and the +resulting *Invalid value for parameter ...* never mentions that the scheme was +what failed. + +`GraphParameterType` covers the case where the user picks a graph. Hand-written +validation is for the ones they type - an ignore list, a target graph that does +not exist yet, a class or property filter - and it has to accept a URN as well: + +```python +URN = re.compile(r"^urn:[a-z0-9][a-z0-9-]{1,31}:\S+$", re.IGNORECASE) + +if not validators.url(iri) and not URN.match(iri): + raise ValueError(f"{iri} is neither a URL nor a URN.") +``` + +The namespace identifier is two to thirty-two characters starting with an +alphanumeric one, and the namespace specific string must be non-empty +(RFC 8141). + ## Custom parameter types A `PluginParameter` without an explicit `param_type` gets one derived from the @@ -258,6 +482,43 @@ class CollectionParameterType(StringParameterType): - Return a **stable order**. Sort at the end; do not deduplicate with `set()` after sorting, because that throws the ordering away again. +### A choice dropdown is not shown in its declared order + +`ChoiceParameterType` does the very thing that last bullet warns against, so +the `OrderedDict` it takes is **not** the order the user sees: `autocomplete()` +sorts by label and then returns `list(set(result))`, and `Autocompletion` is a +frozen dataclass, so the set reorders by hash bucket and discards the sort. +Python randomizes string hashing per process, so the dropdown comes out +differently after every restart of DataIntegration - which also means one run in +three looks plausible enough to miss. Nothing local shows it: the `OrderedDict` +is intact in the source and in the descriptors, and `task check` is green. + +So for a choice whose order carries meaning - priorities, log levels, sizes, +compression levels, conflict policies - subclass it until + is released: + +```python +class OrderedChoiceParameterType(ChoiceParameterType): + """A choice whose dropdown keeps the order the choices were declared in.""" + + def autocomplete( + self, + query_terms: list[str], + depend_on_parameter_values: list[Any], + context: PluginContext, + ) -> list[Autocompletion]: + """Return the choices matching all query terms, in their declared order.""" + terms = [term.lower() for term in query_terms] + return [ + Autocompletion(value=value, label=label) + for value, label in self.choice_list.items() + if all(term in label.lower() for term in terms) + ] +``` + +Numbering the labels does not rescue it - `(1)` to `(5)` only feeds the sort +that is thrown away. + ### The flags - `allow_only_autocompleted_values = True` makes the parameter a closed diff --git a/.claude/skills/plugin-testing/SKILL.md b/.claude/skills/plugin-testing/SKILL.md index 0c9de02..f36d5ab 100644 --- a/.claude/skills/plugin-testing/SKILL.md +++ b/.claude/skills/plugin-testing/SKILL.md @@ -91,6 +91,32 @@ broken plugin fails the assertion instead of the fixture. Never point a test at assets that happen to exist in the deployment: the next person runs the suite somewhere else. +The teardown runs **before** the test as well. A fixture whose asset has a +fixed id leaves it behind whenever a run crashes or is killed, and every later +run then fails in setup on the leftover rather than in the test it was meant to +exercise. Deleting first, tolerating an asset that is not there, makes the suite +self-healing: + +```python +@pytest.fixture +def project() -> Iterator[Client]: + """Provide an empty project, whatever the last run left behind.""" + client = get_client(PROJECT_ID) + client.projects.delete_item(PROJECT_ID, skip_if_missing=True) + client.projects.create_item(Project(name=PROJECT_ID)) + yield client + client.projects.delete_item(PROJECT_ID, skip_if_missing=True) +``` + +**Delete what the test created; never restore the whole store.** Snapshotting +the triple store before a test and importing the snapshot back afterwards does +restore the assets, and also reverts everything else written to the deployment +in between. It looks thorough rather than dangerous, which is why it gets +written - but integration tests commonly point at one shared instance, so the +blast radius is every other suite and every person using it, not the handful of +assets being undone. It is also slow: replacing such a snapshot with targeted +deletion took one suite from about 127 seconds to 22. + ## Finishing Run `task check:pytest` before finishing, and `task check` before considering diff --git a/.copier-answers.yml b/.copier-answers.yml index e522be6..f677178 100644 --- a/.copier-answers.yml +++ b/.copier-answers.yml @@ -1,5 +1,5 @@ # Changes here will be overwritten by Copier -_commit: v9.5.0 +_commit: v9.9.0 _src_path: gh:eccenca/cmem-plugin-template author_mail: cmempy-developer@eccenca.com author_name: eccenca GmbH diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index b049176..53fc94b 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -12,6 +12,15 @@ on: # Allows you to run this workflow manually from the Actions tab workflow_dispatch: +# Least privilege, and enough for the two reporting steps at the end of the job: +# the junit report creates check runs, the coverage report writes a pull request +# comment. A pull request from a fork gets a read-only token whatever is written +# here, which is why both of those steps are allowed to fail. +permissions: + contents: read + checks: write + pull-requests: write + jobs: check: @@ -76,12 +85,14 @@ jobs: - name: Publish Test Report in Action uses: mikepenz/action-junit-report@v6 if: always() # always run even if the previous step fails + continue-on-error: true # a fork pull request cannot be given `checks: write` with: report_paths: dist/junit-*.xml - name: Publish Test and Coverage Report as PR comment - uses: xportation/junit-coverage-report@main + uses: xportation/junit-coverage-report@v1.0.3 if: github.event_name == 'pull_request' + continue-on-error: true # a fork pull request cannot comment on itself with: junit-path: dist/junit-pytest.xml coverage-path: dist/coverage.xml diff --git a/Taskfile.yaml b/Taskfile.yaml index 715e490..aade49a 100644 --- a/Taskfile.yaml +++ b/Taskfile.yaml @@ -7,12 +7,11 @@ version: '3' dotenv: ['.copier-answers.env', '.env'] -.preparation: &preparation - deps: - - poetry:install - - check:prepare - vars: + # $package_dir is expanded by the shell, from the .copier-answers.env entry + # that `dotenv` above loads. An exported package_dir of your own outranks that + # entry, which the `package:exists` task below turns into a loud failure + # instead of a check that quietly looks at nothing. PACKAGE: $package_dir DIST_DIR: dist @@ -36,11 +35,33 @@ tasks: - task --list # {{{ preparation tasks - check:prepare: + # These are plain internal tasks rather than a YAML anchor on purpose. A merge + # key only supplies keys the mapping does not already have, so a task with any + # `deps` of its own silently loses everything the anchor meant to add - which + # is what `build` used to do. Naming them in a `deps` list composes instead. + package:exists: internal: true summary: | - prepare check targets by creating appropriate directory + fail loudly when PACKAGE does not name a directory run: once + preconditions: + - sh: '[ -d "$package_dir" ]' + msg: > + The package directory does not exist. Its name comes from the + package_dir entry of .copier-answers.env, and an exported package_dir + of your own silently outranks that entry - unset it. Without this + check, an empty value dropped the package from every command line and + the checks passed having looked at nothing but tests. + + prepare: + internal: true + summary: | + everything a check or format task needs first: the virtualenv, a package + directory that exists, and the directory the reports are written to + run: once + deps: + - poetry:install + - package:exists cmds: - mkdir -p {{.DIST_DIR}}/coverage @@ -50,32 +71,29 @@ tasks: cmds: - poetry install + # Neither format task depends on `poetry:install`, unlike the check tasks. + # `format:fix` is what the Claude Code PostToolUse hook runs after every edit, + # and `poetry install` refuses to run while pyproject.toml has been changed + # without the lock file being regenerated - exactly what a dependency bump or + # a template update looks like. Formatting would then quietly stop for the + # rest of the session. On a clone with no virtualenv yet, run `task check` or + # `task poetry:install` first. format:fix: desc: Format Python files and fix obvious issues - <<: *preparation + deps: + - package:exists cmds: - poetry run ruff format tests {{.PACKAGE}} - poetry run ruff check tests {{.PACKAGE}} --fix-only format:fix-unsafe: desc: Format Python files and fix 'unsafe' issues - <<: *preparation + deps: + - package:exists cmds: - poetry run ruff format tests {{.PACKAGE}} - poetry run ruff check tests {{.PACKAGE}} --fix-only --unsafe-fixes - template:feedback-check: - desc: Look for findings worth reporting to the cmem-plugin-template - summary: | - Reads a Claude Code Stop hook payload on stdin and asks the agent to - consider reporting upstream when this working tree shows that a template - owned file got in the way. Prints nothing when there is nothing to say. - - Create an empty .claude/no-template-feedback file to switch it off. - silent: true - cmds: - - python3 .claude/hooks/template-feedback.py - clean: desc: Removes dist, *.pyc and some caches cmds: @@ -102,7 +120,8 @@ tasks: check:pytest: desc: Run unit and integration tests platforms: [darwin, linux, windows] - <<: *preparation + deps: + - prepare cmds: # --memray is not used on windows - platforms: [windows] @@ -133,7 +152,8 @@ tasks: check:mypy: desc: Complain about typing errors - <<: *preparation + deps: + - prepare cmds: - poetry run mypy -p tests -p {{.PACKAGE}} --junit-xml {{.JUNIT_FILE}} vars: @@ -141,7 +161,8 @@ tasks: check:trivy: desc: Scan for vulnerabilities using Trivy - <<: *preparation + deps: + - prepare cmds: - > poetry run trivy fs @@ -153,13 +174,15 @@ tasks: check:deptry: desc: Complain about unused or missing dependencies - <<: *preparation + deps: + - prepare cmds: - poetry run deptry . check:ruff: desc: Complain about everything else - <<: *preparation + deps: + - prepare cmds: - poetry run ruff check --exit-zero tests {{.PACKAGE}} {{.XML_PARAMS}} - poetry run ruff check --output-format=concise tests {{.PACKAGE}} @@ -172,8 +195,11 @@ tasks: build: desc: Build a tarball and a wheel package - <<: *preparation + # `prepare` is deliberately not among these: `build` does not write into + # dist/coverage, and it would race with `clean`, since Task runs deps in + # parallel. deps: + - poetry:install - clean cmds: - poetry build diff --git a/pyproject.toml b/pyproject.toml index 868d287..5bde381 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -58,6 +58,13 @@ bump = true warn_return_any = true ignore_missing_imports = true +# Deliberately empty. pytest walks upward until a file *loads* a configuration, +# and only the presence of this table makes pyproject.toml one - without it the +# walk carries on into the parent directories, where any pytest.ini, tox.ini or +# setup.cfg, even an empty one, would set this project's rootdir and could push +# its own addopts into this test suite. Do not tidy it away. +[tool.pytest.ini_options] + [tool.coverage.report] exclude_also = [ "def __repr__", @@ -96,10 +103,12 @@ ignore = [ "ISC001", # single-line-implicit-string-concatenation "PD", # opinionated linting for pandas code "TRY003", # Avoid specifying long messages outside the exception class + "TRY400", # PluginLogger has no exception(), so error() is the only option ] [tool.ruff.lint.per-file-ignores] "tests/**/*.py" = [ "S101", # asserts allowed in tests "ARG", # unused function args (pytest fixtures) + "PLR2004", # in an assertion the expected value is the literal ] From 9eda4b23e184a3de4c30cbdb8384519dcf3f3927 Mon Sep 17 00:00:00 2001 From: Sebastian Tramp Date: Tue, 13 Oct 2026 16:43:31 +0200 Subject: [PATCH 2/2] refresh the locked packages trivy reports as vulnerable task check:trivy fails on any finding and trivy's database has moved on since this lock was written: pyjwt 2.13.0 carries thirteen advisories and urllib3 2.7.0 three. Updating only the flagged packages keeps the change reviewable and makes the scan clean again. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VbqmemAwRmKUxpCncz68L6 --- poetry.lock | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/poetry.lock b/poetry.lock index 021e6a7..bbbe385 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1823,14 +1823,14 @@ windows-terminal = ["colorama (>=0.4.6)"] [[package]] name = "pyjwt" -version = "2.13.0" +version = "2.15.1" description = "JSON Web Token implementation in Python" optional = false python-versions = ">=3.9" groups = ["main", "dev"] files = [ - {file = "pyjwt-2.13.0-py3-none-any.whl", hash = "sha256:66adcc2aff09b3f1bbd95fc1e1577df8ac8723c978552fd43304c8a290ac5728"}, - {file = "pyjwt-2.13.0.tar.gz", hash = "sha256:41571c89ca91598c79e8ef18a2d07367d4810fbbd6f637794879baf1b7703423"}, + {file = "pyjwt-2.15.1-py3-none-any.whl", hash = "sha256:42d59d631f7768a1028a64c7ff581a9bf7519804daf91fc5b6c56e30eec5e193"}, + {file = "pyjwt-2.15.1.tar.gz", hash = "sha256:4f259e80cdfb6b3fc18a7de51fd1ef9ec79652f25019bae68975ca2468a34df8"}, ] [package.extras] @@ -2382,14 +2382,14 @@ typing-extensions = ">=4.15.0" [[package]] name = "urllib3" -version = "2.7.0" +version = "2.8.0" description = "HTTP library with thread-safe connection pooling, file post, and more." optional = false python-versions = ">=3.10" groups = ["main", "dev"] files = [ - {file = "urllib3-2.7.0-py3-none-any.whl", hash = "sha256:9fb4c81ebbb1ce9531cce37674bbc6f1360472bc18ca9a553ede278ef7276897"}, - {file = "urllib3-2.7.0.tar.gz", hash = "sha256:231e0ec3b63ceb14667c67be60f2f2c40a518cb38b03af60abc813da26505f4c"}, + {file = "urllib3-2.8.0-py3-none-any.whl", hash = "sha256:0cf3cae568d36aa9576b28dfb35f11328f1cb974ca7647d9475ebb86c75ac6e3"}, + {file = "urllib3-2.8.0.tar.gz", hash = "sha256:63bf2ead4c879426ebf22ef2a781eeb4aa3b4ae798a0435506f8687fd5bb9b63"}, ] [package.extras]