From 68a9f6fa1928fd5c28a51344de216a704440c194 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Sun, 18 Oct 2026 01:46:32 +0000 Subject: [PATCH 01/21] ci: report user-facing CLI surface changes on every PR Add dokimi/scripts/cli_surface.py (dump the argparse tree, diff two dumps) and the cli-interface workflow: it diffs the CLI surface of the base tip and the PR head and, when they differ, labels the PR cli-interface-change and keeps a sticky comment listing added, removed and changed commands/options. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011Y9BUfAXTy2iFSheZYGGXN Signed-off-by: Nikolay Petrov --- .github/workflows/cli-interface.yml | 111 ++++++++++++++++++++ dokimi/scripts/cli_surface.py | 152 ++++++++++++++++++++++++++++ dokimi/tests/test_cli_surface.py | 87 ++++++++++++++++ 3 files changed, 350 insertions(+) create mode 100644 .github/workflows/cli-interface.yml create mode 100755 dokimi/scripts/cli_surface.py create mode 100644 dokimi/tests/test_cli_surface.py diff --git a/.github/workflows/cli-interface.yml b/.github/workflows/cli-interface.yml new file mode 100644 index 00000000..6b8622e7 --- /dev/null +++ b/.github/workflows/cli-interface.yml @@ -0,0 +1,111 @@ +# Copyright (C) 2026 Intel Corporation +# SPDX-License-Identifier: Apache-2.0 + +name: cli-interface + +# Makes every change to how users call dokimi visible on the PR: dumps the CLI +# surface (commands, options, arguments, defaults, choices) of the base +# branch's tip and of the PR head with dokimi/scripts/cli_surface.py, diffs +# them and, when they differ, labels the PR `cli-interface-change` and posts a +# sticky comment listing each added, removed or changed command and option. +# The comment is updated on every push, and the label removed once the surface +# matches the base again. The check reports; it never fails a PR for a change. +on: + pull_request: + +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number }} + cancel-in-progress: true + +defaults: + run: + working-directory: dokimi + +# intel-ubuntu-latest: see ci.yml. +jobs: + cli-surface: + name: CLI interface diff + runs-on: ${{ github.repository_owner == 'intel' && 'intel-ubuntu-latest' || 'ubuntu-latest' }} + permissions: + contents: read + pull-requests: write # label and comment + issues: write # create the label on first use + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + fetch-depth: 0 + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: '3.13' + - name: Install dependencies + run: pip install -r requirements.lock.txt + # The base branch's current tip (fetched by the checkout), as in the other + # diff-against-base steps. git archive, not a worktree: no git state changes. + # Each dump imports the dokimi of its own tree through PYTHONPATH; the + # head's script reads both, so the dump format is the same on each side. + - name: Dump and diff the CLI surface + id: diff + env: + BASE_REF: ${{ github.event.pull_request.base.ref }} + run: | + out="$RUNNER_TEMP/cli-surface" + rm -rf "$out" && mkdir -p "$out/base" + git -C .. archive "origin/$BASE_REF" dokimi | tar -x -C "$out/base" + PYTHONPATH="$out/base/dokimi" python scripts/cli_surface.py dump "$out/base.json" + PYTHONPATH=. python scripts/cli_surface.py dump "$out/head.json" + rc=0 + python scripts/cli_surface.py diff "$out/base.json" "$out/head.json" --markdown \ + > "$out/diff.md" || rc=$? + cat "$out/diff.md" + if [ "$rc" -gt 1 ]; then exit "$rc"; fi + echo "changed=$rc" >> "$GITHUB_OUTPUT" + echo "file=$out/diff.md" >> "$GITHUB_OUTPUT" + cat "$out/diff.md" >> "$GITHUB_STEP_SUMMARY" + # A fork PR's token is read-only: the diff is then in the job summary only. + - name: Label and comment on the PR + uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 + env: + CHANGED: ${{ steps.diff.outputs.changed }} + DIFF_FILE: ${{ steps.diff.outputs.file }} + with: + script: | + const fs = require('fs'); + const { owner, repo } = context.repo; + const issue_number = context.issue.number; + const label = 'cli-interface-change'; + const marker = ''; + const changed = process.env.CHANGED === '1'; + try { + const comments = await github.paginate(github.rest.issues.listComments, + { owner, repo, issue_number }); + const existing = comments.find((c) => c.body && c.body.includes(marker)); + if (changed) { + const diff = fs.readFileSync(process.env.DIFF_FILE, 'utf8'); + const body = `${marker}\n${diff}\n_Updated on every push; the \`${label}\` ` + + 'label is on while the diff is non-empty._'; + try { + await github.rest.issues.createLabel({ owner, repo, name: label, color: 'B60205', + description: "Changes dokimi's user-facing CLI surface" }); + } catch (e) { /* already exists */ } + await github.rest.issues.addLabels({ owner, repo, issue_number, labels: [label] }); + if (existing) { + await github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, body }); + } else { + await github.rest.issues.createComment({ owner, repo, issue_number, body }); + } + } else { + try { + await github.rest.issues.removeLabel({ owner, repo, issue_number, name: label }); + } catch (e) { /* was not applied */ } + if (existing) { + await github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, + body: `${marker}\nThe CLI surface now matches the base branch: no user-facing CLI change.` }); + } + } + } catch (e) { + core.warning(`Could not label or comment (fork PR token is read-only?): ${e.message}`); + } diff --git a/dokimi/scripts/cli_surface.py b/dokimi/scripts/cli_surface.py new file mode 100755 index 00000000..0de0d79e --- /dev/null +++ b/dokimi/scripts/cli_surface.py @@ -0,0 +1,152 @@ +#!/usr/bin/env python +# Copyright (C) 2026 Intel Corporation +# SPDX-License-Identifier: Apache-2.0 + +"""Dump and diff dokimi's user-facing CLI surface (commands, options, arguments). + +The `cli-interface` workflow (.github/workflows/cli-interface.yml) dumps the +surface of the PR's base and of its head, diffs the two and, when they differ, +labels the PR `cli-interface-change` and posts a sticky comment listing every +added, removed or changed command and option. So each change to how users call +dokimi is visible in review and stays on record in the PR. + +The dump walks the argparse tree `dokimi.cli.build_parser()` returns, of +whichever `dokimi` is first on sys.path: the workflow points PYTHONPATH at a +checkout of each ref in turn. + +Usage: + python scripts/cli_surface.py dump [OUT.json] + python scripts/cli_surface.py diff BASE.json HEAD.json [--markdown] + +`diff` exits 0 when the surfaces match, 1 when they differ. +""" + +from __future__ import annotations + +import argparse +import json +import sys +from pathlib import Path +from typing import Any + +# The attributes of an option or argument compared between base and head. +FIELDS = ("required", "is_flag", "nargs", "default", "type", "choices", "hidden", "action") + + +def _value(value: Any) -> Any: + """A JSON value standing for `value`, stable across runs (no object addresses).""" + if value is None or isinstance(value, (bool, int, float, str)): + return value + if isinstance(value, (list, tuple)): + return [_value(v) for v in value] + if isinstance(value, dict): + return {str(k): _value(v) for k, v in value.items()} + name = getattr(value, "__name__", None) + return name if isinstance(name, str) else type(value).__name__ + + +def _param(action: argparse.Action) -> dict[str, Any]: + is_option = bool(action.option_strings) + return { + "key": "/".join(sorted(action.option_strings)) if is_option else action.dest, + "kind": "option" if is_option else "argument", + "required": bool(action.required), + "is_flag": is_option and action.nargs == 0, + "nargs": _value(action.nargs), + "default": _value(action.default), + "type": _value(action.type), + "choices": sorted(str(c) for c in action.choices) if action.choices else None, + "hidden": action.help == argparse.SUPPRESS, + "action": type(action).__name__, + } + + +def _command(parser: argparse.ArgumentParser, path: str) -> dict[str, Any]: + node: dict[str, Any] = {"path": path, "params": [], "subcommands": {}} + for action in parser._actions: + if isinstance(action, argparse._SubParsersAction): + for name, sub in sorted(action.choices.items()): + node["subcommands"][name] = _command(sub, f"{path} {name}") + elif not isinstance(action, (argparse._HelpAction, argparse._VersionAction)): + node["params"].append(_param(action)) + node["params"].sort(key=lambda p: (p["kind"], p["key"])) + return node + + +def dump_surface() -> dict[str, Any]: + """The command tree reachable from `dokimi --help`.""" + from dokimi.cli import build_parser + + return _command(build_parser(), "dokimi") + + +def _flatten(node: dict[str, Any], out: dict[str, dict[str, Any]]) -> dict[str, dict[str, Any]]: + out[node["path"]] = node + for sub in node["subcommands"].values(): + _flatten(sub, out) + return out + + +def _diff_params(old: list[dict[str, Any]], new: list[dict[str, Any]]) -> list[str]: + old_by = {p["key"]: p for p in old} + new_by = {p["key"]: p for p in new} + lines = [f" - removed {old_by[k]['kind']} `{k}`" for k in sorted(old_by.keys() - new_by.keys())] + lines += [f" + added {new_by[k]['kind']} `{k}`" for k in sorted(new_by.keys() - old_by.keys())] + for k in sorted(old_by.keys() & new_by.keys()): + changed = [f"{f}: {old_by[k].get(f)!r} -> {new_by[k].get(f)!r}" + for f in FIELDS if old_by[k].get(f) != new_by[k].get(f)] + if changed: + lines.append(f" ~ changed {new_by[k]['kind']} `{k}`: " + "; ".join(changed)) + return lines + + +def diff_surfaces(base: dict[str, Any], head: dict[str, Any]) -> list[str]: + """Human-readable diff lines; empty when the surfaces match.""" + old, new = _flatten(base, {}), _flatten(head, {}) + lines = [f"- removed command `{p}`" for p in sorted(old.keys() - new.keys())] + lines += [f"+ added command `{p}`" for p in sorted(new.keys() - old.keys())] + for path in sorted(old.keys() & new.keys()): + param_lines = _diff_params(old[path]["params"], new[path]["params"]) + if param_lines: + lines.append(f"~ `{path}` options changed:") + lines += param_lines + return lines + + +def render(lines: list[str], *, markdown: bool) -> str: + if not lines: + return "No user-facing CLI surface changes detected.\n" + body = "\n".join(lines) + if markdown: + return ("## CLI interface change detected\n\n" + "This PR changes dokimi's user-facing CLI surface (commands and/or options):\n\n" + f"```diff\n{body}\n```\n") + return f"CLI interface change detected:\n{body}\n" + + +def main(argv: list[str] | None = None) -> int: + p = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + sub = p.add_subparsers(dest="cmd", required=True) + d = sub.add_parser("dump", help="write the CLI surface of the importable dokimi as JSON") + d.add_argument("out", nargs="?", type=Path) + c = sub.add_parser("diff", help="diff two dumps; exit 1 when they differ") + c.add_argument("base", type=Path) + c.add_argument("head", type=Path) + c.add_argument("--markdown", action="store_true", help="format for a PR comment") + args = p.parse_args(argv) + if args.cmd == "dump": + text = json.dumps(dump_surface(), indent=2, sort_keys=True) + "\n" + if args.out: + args.out.write_text(text, encoding="utf-8") + else: + sys.stdout.write(text) + return 0 + base = json.loads(args.base.read_text(encoding="utf-8")) + head = json.loads(args.head.read_text(encoding="utf-8")) + lines = diff_surfaces(base, head) + sys.stdout.write(render(lines, markdown=args.markdown)) + return 1 if lines else 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/dokimi/tests/test_cli_surface.py b/dokimi/tests/test_cli_surface.py new file mode 100644 index 00000000..cfeaf251 --- /dev/null +++ b/dokimi/tests/test_cli_surface.py @@ -0,0 +1,87 @@ +# Copyright (C) 2026 Intel Corporation +# SPDX-License-Identifier: Apache-2.0 + +"""Tests for scripts/cli_surface.py — the CLI-interface change report on PRs.""" + +from __future__ import annotations + +import argparse +import importlib.util +import json +from pathlib import Path + +import pytest + +PROJECT = Path(__file__).resolve().parents[1] +_SCRIPT = PROJECT / "scripts" / "cli_surface.py" +_spec = importlib.util.spec_from_file_location("cli_surface", _SCRIPT) +assert _spec and _spec.loader +cs = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(cs) + + +def _parser(extra: bool = False, default: int = 1, drop_cmd: bool = False) -> argparse.ArgumentParser: + p = argparse.ArgumentParser(prog="dokimi") + sub = p.add_subparsers(dest="cmd", required=True) + if not drop_cmd: + r = sub.add_parser("run") + r.add_argument("questions") + r.add_argument("--n", type=int, default=default) + r.add_argument("--mode", choices=["a", "b"], default="a") + if extra: + r.add_argument("--dry-run", action="store_true") + g = sub.add_parser("repo") + gs = g.add_subparsers(dest="repo_cmd", required=True) + gs.add_parser("scan").add_argument("--hidden", help=argparse.SUPPRESS) + return p + + +def _surface(**kw: bool | int) -> dict: + return cs._command(_parser(**kw), "dokimi") + + +def test_dump_of_the_real_cli_is_json_and_deterministic(): + first = json.dumps(cs.dump_surface(), sort_keys=True) + assert first == json.dumps(cs.dump_surface(), sort_keys=True) + assert "0x" not in first # no object addresses leaking in from defaults or types + assert "run" in json.loads(first)["subcommands"] + + +def test_nested_subcommands_and_hidden_options_are_walked(): + flat = cs._flatten(_surface(), {}) + assert set(flat) == {"dokimi", "dokimi run", "dokimi repo", "dokimi repo scan"} + (hidden,) = flat["dokimi repo scan"]["params"] + assert hidden["hidden"] is True + + +def test_identical_surfaces_have_no_diff(): + assert cs.diff_surfaces(_surface(), _surface()) == [] + + +@pytest.mark.parametrize("base, head, expected", [ + ({}, {"extra": True}, "+ added option `--dry-run`"), + ({"extra": True}, {}, "- removed option `--dry-run`"), + ({}, {"default": 2}, "~ changed option `--n`: default: 1 -> 2"), + ({}, {"drop_cmd": True}, "- removed command `dokimi run`"), + ({"drop_cmd": True}, {}, "+ added command `dokimi run`"), +]) +def test_each_kind_of_change_is_reported(base, head, expected): + lines = cs.diff_surfaces(_surface(**base), _surface(**head)) + assert any(expected in line for line in lines), lines + + +def test_main_diff_exit_codes_and_markdown(tmp_path, capsys): + a, b = tmp_path / "a.json", tmp_path / "b.json" + a.write_text(json.dumps(_surface()), encoding="utf-8") + b.write_text(json.dumps(_surface(extra=True)), encoding="utf-8") + assert cs.main(["diff", str(a), str(a)]) == 0 + assert "No user-facing CLI surface changes" in capsys.readouterr().out + assert cs.main(["diff", str(a), str(b), "--markdown"]) == 1 + out = capsys.readouterr().out + assert out.startswith("## CLI interface change detected") and "```diff" in out + + +def test_main_dump_writes_file(tmp_path): + out = tmp_path / "s.json" + assert cs.main(["dump", str(out)]) == 0 + assert json.loads(out.read_text(encoding="utf-8"))["path"] == "dokimi" From 085837a6ebed7257129c13fb28c29be64d6c0fc1 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Sun, 18 Oct 2026 01:46:32 +0000 Subject: [PATCH 02/21] ci(cli-interface): track positional order, --version, required subcommands Address review: compare positional argument order, keep --version (not its value), record whether a subcommand is required, list the params of added and removed commands, drop issues: write, and only update the bot's own comment. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011Y9BUfAXTy2iFSheZYGGXN Signed-off-by: Nikolay Petrov --- .github/workflows/cli-interface.yml | 6 ++--- dokimi/scripts/cli_surface.py | 30 +++++++++++++++++------ dokimi/tests/test_cli_surface.py | 38 +++++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 10 deletions(-) diff --git a/.github/workflows/cli-interface.yml b/.github/workflows/cli-interface.yml index 6b8622e7..6de8d99e 100644 --- a/.github/workflows/cli-interface.yml +++ b/.github/workflows/cli-interface.yml @@ -31,8 +31,7 @@ jobs: runs-on: ${{ github.repository_owner == 'intel' && 'intel-ubuntu-latest' || 'ubuntu-latest' }} permissions: contents: read - pull-requests: write # label and comment - issues: write # create the label on first use + pull-requests: write # create/apply the label, write the comment steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: @@ -82,7 +81,8 @@ jobs: try { const comments = await github.paginate(github.rest.issues.listComments, { owner, repo, issue_number }); - const existing = comments.find((c) => c.body && c.body.includes(marker)); + const existing = comments.find((c) => c.user?.login === 'github-actions[bot]' && + c.body && c.body.includes(marker)); if (changed) { const diff = fs.readFileSync(process.env.DIFF_FILE, 'utf8'); const body = `${marker}\n${diff}\n_Updated on every push; the \`${label}\` ` + diff --git a/dokimi/scripts/cli_surface.py b/dokimi/scripts/cli_surface.py index 0de0d79e..a79cc4d0 100755 --- a/dokimi/scripts/cli_surface.py +++ b/dokimi/scripts/cli_surface.py @@ -30,7 +30,8 @@ from typing import Any # The attributes of an option or argument compared between base and head. -FIELDS = ("required", "is_flag", "nargs", "default", "type", "choices", "hidden", "action") +FIELDS = ("required", "is_flag", "nargs", "default", "type", "choices", "hidden", "action", + "position") def _value(value: Any) -> Any: @@ -45,9 +46,10 @@ def _value(value: Any) -> Any: return name if isinstance(name, str) else type(value).__name__ -def _param(action: argparse.Action) -> dict[str, Any]: +def _param(action: argparse.Action, position: int | None) -> dict[str, Any]: is_option = bool(action.option_strings) return { + "position": position, # a positional's index: swapping two is a change "key": "/".join(sorted(action.option_strings)) if is_option else action.dest, "kind": "option" if is_option else "argument", "required": bool(action.required), @@ -62,13 +64,18 @@ def _param(action: argparse.Action) -> dict[str, Any]: def _command(parser: argparse.ArgumentParser, path: str) -> dict[str, Any]: - node: dict[str, Any] = {"path": path, "params": [], "subcommands": {}} + node: dict[str, Any] = {"path": path, "params": [], "subcommands": {}, + "subcommand_required": None} + position = 0 for action in parser._actions: if isinstance(action, argparse._SubParsersAction): + node["subcommand_required"] = bool(action.required) for name, sub in sorted(action.choices.items()): node["subcommands"][name] = _command(sub, f"{path} {name}") - elif not isinstance(action, (argparse._HelpAction, argparse._VersionAction)): - node["params"].append(_param(action)) + elif not isinstance(action, argparse._HelpAction): + # --version is kept (its version string is not dumped, so a release is no change) + node["params"].append(_param(action, None if action.option_strings else position)) + position += 0 if action.option_strings else 1 node["params"].sort(key=lambda p: (p["kind"], p["key"])) return node @@ -103,9 +110,18 @@ def _diff_params(old: list[dict[str, Any]], new: list[dict[str, Any]]) -> list[s def diff_surfaces(base: dict[str, Any], head: dict[str, Any]) -> list[str]: """Human-readable diff lines; empty when the surfaces match.""" old, new = _flatten(base, {}), _flatten(head, {}) - lines = [f"- removed command `{p}`" for p in sorted(old.keys() - new.keys())] - lines += [f"+ added command `{p}`" for p in sorted(new.keys() - old.keys())] + lines: list[str] = [] + for path in sorted(old.keys() - new.keys()): + lines.append(f"- removed command `{path}`") + lines += [f" - {p['kind']} `{p['key']}`" for p in old[path]["params"]] + for path in sorted(new.keys() - old.keys()): + lines.append(f"+ added command `{path}`") + lines += [f" + {p['kind']} `{p['key']}`" for p in new[path]["params"]] for path in sorted(old.keys() & new.keys()): + if old[path].get("subcommand_required") != new[path].get("subcommand_required"): + lines.append(f"~ `{path}` subcommand required: " + f"{old[path].get('subcommand_required')!r} -> " + f"{new[path].get('subcommand_required')!r}") param_lines = _diff_params(old[path]["params"], new[path]["params"]) if param_lines: lines.append(f"~ `{path}` options changed:") diff --git a/dokimi/tests/test_cli_surface.py b/dokimi/tests/test_cli_surface.py index cfeaf251..9338b248 100644 --- a/dokimi/tests/test_cli_surface.py +++ b/dokimi/tests/test_cli_surface.py @@ -85,3 +85,41 @@ def test_main_dump_writes_file(tmp_path): out = tmp_path / "s.json" assert cs.main(["dump", str(out)]) == 0 assert json.loads(out.read_text(encoding="utf-8"))["path"] == "dokimi" + + +def _positional_parser(order: tuple[str, str], required: bool = True, + version: str | None = "1") -> argparse.ArgumentParser: + p = argparse.ArgumentParser(prog="dokimi") + if version: + p.add_argument("--version", action="version", version=version) + sub = p.add_subparsers(dest="cmd", required=required) + c = sub.add_parser("cmp") + for name in order: + c.add_argument(name) + return p + + +def test_swapping_positionals_is_a_change(): + a = cs._command(_positional_parser(("base", "head")), "dokimi") + b = cs._command(_positional_parser(("head", "base")), "dokimi") + assert any("position: 0 -> 1" in line for line in cs.diff_surfaces(a, b)) + + +def test_subcommand_required_change_is_reported(): + a = cs._command(_positional_parser(("x", "y")), "dokimi") + b = cs._command(_positional_parser(("x", "y"), required=False), "dokimi") + assert any("subcommand required: True -> False" in line for line in cs.diff_surfaces(a, b)) + + +def test_version_option_tracked_but_not_its_value(): + a = cs._command(_positional_parser(("x", "y"), version="1"), "dokimi") + b = cs._command(_positional_parser(("x", "y"), version="2"), "dokimi") + c = cs._command(_positional_parser(("x", "y"), version=None), "dokimi") + assert cs.diff_surfaces(a, b) == [] # a release bump is no CLI change + assert " - removed option `--version`" in cs.diff_surfaces(a, c) + + +def test_added_command_lists_its_params(): + lines = cs.diff_surfaces(_surface(drop_cmd=True), _surface()) + assert "+ added command `dokimi run`" in lines + assert " + option `--n`" in lines and " + argument `questions`" in lines From 2a64ff8304ae8928efc97c669480d460e72d2ff5 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Sun, 18 Oct 2026 02:09:22 +0000 Subject: [PATCH 03/21] ci(cli-interface): compare Path defaults by value and const Signed-off-by: Nikolay Petrov Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011Y9BUfAXTy2iFSheZYGGXN --- dokimi/scripts/cli_surface.py | 7 +++++-- dokimi/tests/test_cli_surface.py | 12 ++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/dokimi/scripts/cli_surface.py b/dokimi/scripts/cli_surface.py index a79cc4d0..4399ee86 100755 --- a/dokimi/scripts/cli_surface.py +++ b/dokimi/scripts/cli_surface.py @@ -26,12 +26,12 @@ import argparse import json import sys -from pathlib import Path +from pathlib import Path, PurePath from typing import Any # The attributes of an option or argument compared between base and head. FIELDS = ("required", "is_flag", "nargs", "default", "type", "choices", "hidden", "action", - "position") + "position", "const") def _value(value: Any) -> Any: @@ -42,6 +42,8 @@ def _value(value: Any) -> Any: return [_value(v) for v in value] if isinstance(value, dict): return {str(k): _value(v) for k, v in value.items()} + if isinstance(value, PurePath): + return str(value) name = getattr(value, "__name__", None) return name if isinstance(name, str) else type(value).__name__ @@ -56,6 +58,7 @@ def _param(action: argparse.Action, position: int | None) -> dict[str, Any]: "is_flag": is_option and action.nargs == 0, "nargs": _value(action.nargs), "default": _value(action.default), + "const": _value(action.const), "type": _value(action.type), "choices": sorted(str(c) for c in action.choices) if action.choices else None, "hidden": action.help == argparse.SUPPRESS, diff --git a/dokimi/tests/test_cli_surface.py b/dokimi/tests/test_cli_surface.py index 9338b248..74cbbf8b 100644 --- a/dokimi/tests/test_cli_surface.py +++ b/dokimi/tests/test_cli_surface.py @@ -123,3 +123,15 @@ def test_added_command_lists_its_params(): lines = cs.diff_surfaces(_surface(drop_cmd=True), _surface()) assert "+ added command `dokimi run`" in lines assert " + option `--n`" in lines and " + argument `questions`" in lines + + +@pytest.mark.parametrize("old, new, expected", [ + ({"default": Path("a")}, {"default": Path("b")}, "default: 'a' -> 'b'"), + ({"nargs": "?", "const": "x"}, {"nargs": "?", "const": "y"}, "const: 'x' -> 'y'"), +]) +def test_path_defaults_and_const_are_compared(old, new, expected): + def surface(kw): + p = argparse.ArgumentParser(prog="dokimi") + p.add_argument("--opt", **kw) + return cs._command(p, "dokimi") + assert any(expected in line for line in cs.diff_surfaces(surface(old), surface(new))) From a67e1e79271b42f0dc189c44fc436e7474911e5d Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Mon, 19 Oct 2026 01:04:15 -0700 Subject: [PATCH 04/21] fix(cli-surface): capture mutually exclusive group membership and required status The snapshot now serializes group membership (group_key) and group-level required status (group_required) for each parameter. This allows diff_surfaces to detect changes to which parameters are in mutually exclusive groups and whether those groups are required. Adds regression tests that verify both group membership and group-level required changes are detected. Fixes copilot-pull-request-reviewer feedback on PR #88. Signed-off-by: Nikolay Petrov --- dokimi/scripts/cli_surface.py | 20 +++++++++++++--- dokimi/tests/test_cli_surface.py | 41 ++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 3 deletions(-) diff --git a/dokimi/scripts/cli_surface.py b/dokimi/scripts/cli_surface.py index 4399ee86..f3c8236c 100755 --- a/dokimi/scripts/cli_surface.py +++ b/dokimi/scripts/cli_surface.py @@ -31,7 +31,7 @@ # The attributes of an option or argument compared between base and head. FIELDS = ("required", "is_flag", "nargs", "default", "type", "choices", "hidden", "action", - "position", "const") + "position", "const", "group_key", "group_required") def _value(value: Any) -> Any: @@ -48,7 +48,8 @@ def _value(value: Any) -> Any: return name if isinstance(name, str) else type(value).__name__ -def _param(action: argparse.Action, position: int | None) -> dict[str, Any]: +def _param(action: argparse.Action, position: int | None, group_key: str | None = None, + group_required: bool | None = None) -> dict[str, Any]: is_option = bool(action.option_strings) return { "position": position, # a positional's index: swapping two is a change @@ -63,6 +64,8 @@ def _param(action: argparse.Action, position: int | None) -> dict[str, Any]: "choices": sorted(str(c) for c in action.choices) if action.choices else None, "hidden": action.help == argparse.SUPPRESS, "action": type(action).__name__, + "group_key": group_key, + "group_required": group_required, } @@ -70,6 +73,13 @@ def _command(parser: argparse.ArgumentParser, path: str) -> dict[str, Any]: node: dict[str, Any] = {"path": path, "params": [], "subcommands": {}, "subcommand_required": None} position = 0 + + # Build a map from action to its mutually exclusive group (if any). + action_to_group: dict[argparse.Action, tuple[int, bool]] = {} # action -> (group_id, required) + for group_id, group in enumerate(parser._mutually_exclusive_groups): + for action in group._group_actions: + action_to_group[action] = (group_id, group.required) + for action in parser._actions: if isinstance(action, argparse._SubParsersAction): node["subcommand_required"] = bool(action.required) @@ -77,7 +87,11 @@ def _command(parser: argparse.ArgumentParser, path: str) -> dict[str, Any]: node["subcommands"][name] = _command(sub, f"{path} {name}") elif not isinstance(action, argparse._HelpAction): # --version is kept (its version string is not dumped, so a release is no change) - node["params"].append(_param(action, None if action.option_strings else position)) + group_info = action_to_group.get(action) + group_key = f"group_{group_info[0]}" if group_info else None + group_required = group_info[1] if group_info else None + node["params"].append(_param(action, None if action.option_strings else position, + group_key, group_required)) position += 0 if action.option_strings else 1 node["params"].sort(key=lambda p: (p["kind"], p["key"])) return node diff --git a/dokimi/tests/test_cli_surface.py b/dokimi/tests/test_cli_surface.py index 74cbbf8b..95541696 100644 --- a/dokimi/tests/test_cli_surface.py +++ b/dokimi/tests/test_cli_surface.py @@ -135,3 +135,44 @@ def surface(kw): p.add_argument("--opt", **kw) return cs._command(p, "dokimi") assert any(expected in line for line in cs.diff_surfaces(surface(old), surface(new))) + + +def _mutually_exclusive_parser(group_required: bool = True) -> argparse.ArgumentParser: + p = argparse.ArgumentParser(prog="dokimi") + sub = p.add_subparsers(dest="cmd", required=True) + run_p = sub.add_parser("run") + group = run_p.add_mutually_exclusive_group(required=group_required) + group.add_argument("--tasks", default="", help="Task names") + group.add_argument("--all", action="store_true", help="All tasks") + return p + + +def test_mutually_exclusive_group_membership_is_captured(): + surface = cs._command(_mutually_exclusive_parser(), "dokimi") + run_cmd = surface["subcommands"]["run"] + params_by_key = {p["key"]: p for p in run_cmd["params"]} + assert params_by_key["--tasks"]["group_key"] == "group_0" + assert params_by_key["--tasks"]["group_required"] is True + assert params_by_key["--all"]["group_key"] == "group_0" + assert params_by_key["--all"]["group_required"] is True + + +def test_mutually_exclusive_group_required_change_is_reported(): + a = cs._command(_mutually_exclusive_parser(group_required=True), "dokimi") + b = cs._command(_mutually_exclusive_parser(group_required=False), "dokimi") + lines = cs.diff_surfaces(a, b) + assert any("group_required: True -> False" in line for line in lines) + + +def test_mutually_exclusive_group_membership_removal_is_reported(): + p_with_group = _mutually_exclusive_parser() + p_without = argparse.ArgumentParser(prog="dokimi") + sub = p_without.add_subparsers(dest="cmd", required=True) + run_p = sub.add_parser("run") + run_p.add_argument("--tasks", default="") + run_p.add_argument("--all", action="store_true") + + a = cs._command(p_with_group, "dokimi") + b = cs._command(p_without, "dokimi") + lines = cs.diff_surfaces(a, b) + assert any("group_key" in line for line in lines) From 897ff660ab17920c7c02c69cf765036bb17de929 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Mon, 19 Oct 2026 07:08:09 -0700 Subject: [PATCH 05/21] fix(cli-surface): key groups by members, not creation order group_key was f'group_{index}' which changed when a new group was added before an existing one, even though the group members stayed the same. This caused reordering and new groups to falsely report unchanged options as changed. Fix: key the group by its sorted member keys instead. Update tests to verify the new group_key format. Signed-off-by: Nikolay Petrov --- dokimi/scripts/cli_surface.py | 17 +++++++++++++---- dokimi/tests/test_cli_surface.py | 9 ++++++--- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/dokimi/scripts/cli_surface.py b/dokimi/scripts/cli_surface.py index f3c8236c..0f6dd3f0 100755 --- a/dokimi/scripts/cli_surface.py +++ b/dokimi/scripts/cli_surface.py @@ -75,10 +75,19 @@ def _command(parser: argparse.ArgumentParser, path: str) -> dict[str, Any]: position = 0 # Build a map from action to its mutually exclusive group (if any). - action_to_group: dict[argparse.Action, tuple[int, bool]] = {} # action -> (group_id, required) - for group_id, group in enumerate(parser._mutually_exclusive_groups): + action_to_group: dict[argparse.Action, tuple[str, bool]] = {} # action -> (group_key, required) + for group in parser._mutually_exclusive_groups: + # Key the group by its members' keys, not creation order, so reordering groups + # does not report unchanged options as changed. + member_keys = [] for action in group._group_actions: - action_to_group[action] = (group_id, group.required) + if action.option_strings: + member_keys.append("/".join(sorted(action.option_strings))) + else: + member_keys.append(action.dest) + group_key = "|".join(sorted(member_keys)) + for action in group._group_actions: + action_to_group[action] = (group_key, group.required) for action in parser._actions: if isinstance(action, argparse._SubParsersAction): @@ -88,7 +97,7 @@ def _command(parser: argparse.ArgumentParser, path: str) -> dict[str, Any]: elif not isinstance(action, argparse._HelpAction): # --version is kept (its version string is not dumped, so a release is no change) group_info = action_to_group.get(action) - group_key = f"group_{group_info[0]}" if group_info else None + group_key = group_info[0] if group_info else None group_required = group_info[1] if group_info else None node["params"].append(_param(action, None if action.option_strings else position, group_key, group_required)) diff --git a/dokimi/tests/test_cli_surface.py b/dokimi/tests/test_cli_surface.py index 95541696..53a95c1f 100644 --- a/dokimi/tests/test_cli_surface.py +++ b/dokimi/tests/test_cli_surface.py @@ -151,9 +151,11 @@ def test_mutually_exclusive_group_membership_is_captured(): surface = cs._command(_mutually_exclusive_parser(), "dokimi") run_cmd = surface["subcommands"]["run"] params_by_key = {p["key"]: p for p in run_cmd["params"]} - assert params_by_key["--tasks"]["group_key"] == "group_0" + # Group key is stable across reordering: based on sorted member keys, not creation order + group_key = "--all|--tasks" # sorted pair of the group members + assert params_by_key["--tasks"]["group_key"] == group_key assert params_by_key["--tasks"]["group_required"] is True - assert params_by_key["--all"]["group_key"] == "group_0" + assert params_by_key["--all"]["group_key"] == group_key assert params_by_key["--all"]["group_required"] is True @@ -175,4 +177,5 @@ def test_mutually_exclusive_group_membership_removal_is_reported(): a = cs._command(p_with_group, "dokimi") b = cs._command(p_without, "dokimi") lines = cs.diff_surfaces(a, b) - assert any("group_key" in line for line in lines) + group_key = "--all|--tasks" + assert any(f"group_key: '{group_key}' -> None" in line for line in lines) From 5dba8b5f20837b780189f070906ec08dee621bf2 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Mon, 19 Oct 2026 07:13:17 -0700 Subject: [PATCH 06/21] fix(cli-surface): compare validators, enums and sets by content; test every field Signed-off-by: Nikolay Petrov --- dokimi/scripts/cli_surface.py | 32 +++++++++++++++- dokimi/tests/test_cli_surface.py | 64 ++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 1 deletion(-) diff --git a/dokimi/scripts/cli_surface.py b/dokimi/scripts/cli_surface.py index 0f6dd3f0..0c64d25e 100755 --- a/dokimi/scripts/cli_surface.py +++ b/dokimi/scripts/cli_surface.py @@ -24,7 +24,11 @@ from __future__ import annotations import argparse +import enum +import hashlib +import inspect import json +import re import sys from pathlib import Path, PurePath from typing import Any @@ -34,6 +38,14 @@ "position", "const", "group_key", "group_required") +def _filled(cell: Any) -> bool: + try: + _ = cell.cell_contents + except ValueError: + return False + return True + + def _value(value: Any) -> Any: """A JSON value standing for `value`, stable across runs (no object addresses).""" if value is None or isinstance(value, (bool, int, float, str)): @@ -44,8 +56,26 @@ def _value(value: Any) -> Any: return {str(k): _value(v) for k, v in value.items()} if isinstance(value, PurePath): return str(value) + if isinstance(value, (set, frozenset)): + return sorted(repr(_value(v)) for v in value) + if isinstance(value, enum.Enum): + return repr(value) + if inspect.isfunction(value): + # A validator's name alone hides a loosened body: add a hash of its source. + try: + src = inspect.getsource(value) + except (OSError, TypeError): + src = "nosource" + # Captured values and defaults (a factory's `lo`) change what the function accepts too. + cells = [c.cell_contents for c in (value.__closure__ or ()) if _filled(c)] + src += repr([_value(c) for c in cells]) + repr(_value(value.__defaults__)) + digest = hashlib.sha256(src.encode("utf-8")).hexdigest()[:12] + return f"{value.__module__}.{value.__qualname__}#{digest}" name = getattr(value, "__name__", None) - return name if isinstance(name, str) else type(value).__name__ + if isinstance(name, str): + return name + text = repr(value) # e.g. FileType('r'); object addresses would differ run to run + return type(value).__name__ if re.search(r" at 0x[0-9a-fA-F]+", text) else text def _param(action: argparse.Action, position: int | None, group_key: str | None = None, diff --git a/dokimi/tests/test_cli_surface.py b/dokimi/tests/test_cli_surface.py index 53a95c1f..b8e7209a 100644 --- a/dokimi/tests/test_cli_surface.py +++ b/dokimi/tests/test_cli_surface.py @@ -6,6 +6,7 @@ from __future__ import annotations import argparse +import enum import importlib.util import json from pathlib import Path @@ -179,3 +180,66 @@ def test_mutually_exclusive_group_membership_removal_is_reported(): lines = cs.diff_surfaces(a, b) group_key = "--all|--tasks" assert any(f"group_key: '{group_key}' -> None" in line for line in lines) + + +class _Mode(enum.Enum): + A = 1 + B = 2 + + +def _make_validator(lo: int): + def positive(s: str) -> int: + v = int(s) + if v < lo: + raise argparse.ArgumentTypeError("too small") + return v + return positive + + +def _one(**kw) -> dict: + p = argparse.ArgumentParser(prog="dokimi") + sub = p.add_subparsers(dest="cmd", required=True) + sub.add_parser("run").add_argument("--n", **kw) + return cs._command(p, "dokimi") + + +@pytest.mark.parametrize("old, new", [ + ({"type": _make_validator(1)}, {"type": _make_validator(0)}), + ({"default": _Mode.A}, {"default": _Mode.B}), + ({"default": {"a"}}, {"default": {"a", "b"}}), + ({"type": argparse.FileType("r")}, {"type": argparse.FileType("w")}), +]) +def test_opaque_types_and_defaults_are_compared_by_content(old, new): + assert cs.diff_surfaces(_one(**old), _one(**new)) + + +def test_equal_opaque_values_are_stable(): + kw = {"type": _make_validator(1), "default": {"a", "b"}} + assert cs.diff_surfaces(_one(**kw), _one(type=_make_validator(1), default={"b", "a"})) == [] + + +def _flag_variants(): + return [ + ({}, {"required": True}, "required: False -> True"), + ({"type": int}, {"type": str}, "type: 'int' -> 'str'"), + ({"choices": ["a"]}, {"choices": ["a", "b"]}, "choices:"), + ({"nargs": "?"}, {"nargs": "*"}, "nargs: '?' -> '*'"), + ({"action": "store_true"}, {"action": "store_false"}, "action: '_StoreTrueAction' -> '_StoreFalseAction'"), + ({"action": "store_true"}, {"action": "store_true", "help": argparse.SUPPRESS}, "hidden: False -> True"), + ({"action": "store_true"}, {"nargs": "?"}, "is_flag: True -> False"), + ] + + +@pytest.mark.parametrize("old, new, expected", _flag_variants()) +def test_each_field_change_is_reported(old, new, expected): + lines = cs.diff_surfaces(_one(**old), _one(**new)) + assert any(expected in line for line in lines), lines + + +def test_reordered_choices_are_no_change(): + assert cs.diff_surfaces(_one(choices=["a", "b"]), _one(choices=["b", "a"])) == [] + + +def test_removed_command_lists_its_options(): + lines = cs.diff_surfaces(_surface(), _surface(drop_cmd=True)) + assert " - option `--n`" in lines and " - argument `questions`" in lines From f863272ea61c61322cc6fb9588425bc7d334e6b4 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Mon, 19 Oct 2026 07:54:34 -0700 Subject: [PATCH 07/21] fix(cli-surface): add missing permissions, choices type handling, header wording - Add `issues: write` permission to github-script step (uses github.rest.issues.* APIs) - Serialize choices using _value() with repr() sorting, preserving type info (e.g., [1,2] vs ['1','2']) - Add test for mixed-type choices; verify old code fails on it - Change header from "options changed" to "parameters changed" (includes arguments) Signed-off-by: Nikolay Petrov --- .github/workflows/cli-interface.yml | 1 + dokimi/scripts/cli_surface.py | 4 ++-- dokimi/tests/test_cli_surface.py | 5 +++++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/.github/workflows/cli-interface.yml b/.github/workflows/cli-interface.yml index 6de8d99e..daff3a19 100644 --- a/.github/workflows/cli-interface.yml +++ b/.github/workflows/cli-interface.yml @@ -32,6 +32,7 @@ jobs: permissions: contents: read pull-requests: write # create/apply the label, write the comment + issues: write # github.rest.issues.* APIs for labels and comments steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: diff --git a/dokimi/scripts/cli_surface.py b/dokimi/scripts/cli_surface.py index 0c64d25e..492aedf0 100755 --- a/dokimi/scripts/cli_surface.py +++ b/dokimi/scripts/cli_surface.py @@ -91,7 +91,7 @@ def _param(action: argparse.Action, position: int | None, group_key: str | None "default": _value(action.default), "const": _value(action.const), "type": _value(action.type), - "choices": sorted(str(c) for c in action.choices) if action.choices else None, + "choices": sorted(repr(_value(c)) for c in action.choices) if action.choices else None, "hidden": action.help == argparse.SUPPRESS, "action": type(action).__name__, "group_key": group_key, @@ -180,7 +180,7 @@ def diff_surfaces(base: dict[str, Any], head: dict[str, Any]) -> list[str]: f"{new[path].get('subcommand_required')!r}") param_lines = _diff_params(old[path]["params"], new[path]["params"]) if param_lines: - lines.append(f"~ `{path}` options changed:") + lines.append(f"~ `{path}` parameters changed:") lines += param_lines return lines diff --git a/dokimi/tests/test_cli_surface.py b/dokimi/tests/test_cli_surface.py index b8e7209a..09ad2eb4 100644 --- a/dokimi/tests/test_cli_surface.py +++ b/dokimi/tests/test_cli_surface.py @@ -240,6 +240,11 @@ def test_reordered_choices_are_no_change(): assert cs.diff_surfaces(_one(choices=["a", "b"]), _one(choices=["b", "a"])) == [] +def test_choices_with_different_types_are_reported(): + # [1, 2] and ['1', '2'] are distinct and should be reported as changed + assert cs.diff_surfaces(_one(choices=[1, 2]), _one(choices=["1", "2"])) + + def test_removed_command_lists_its_options(): lines = cs.diff_surfaces(_surface(), _surface(drop_cmd=True)) assert " - option `--n`" in lines and " - argument `questions`" in lines From fcf2c1daf6f4bf171eb4e57561c117ebecc959f6 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Mon, 19 Oct 2026 13:17:39 -0700 Subject: [PATCH 08/21] fix(ci): keep the CLI interface check from failing a PR on a dump error A dump that raises (a PR that moves build_parser) failed the job and left a stale label. Report it as an error in the comment and keep the label as it is. Add tests for an option placed before a positional, a validator whose __defaults__ changed, and a default object() whose address must not reach the dump. Signed-off-by: Nikolay Petrov --- .github/workflows/cli-interface.yml | 26 +++++++++++++++++++--- dokimi/tests/test_cli_surface.py | 34 +++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 3 deletions(-) diff --git a/.github/workflows/cli-interface.yml b/.github/workflows/cli-interface.yml index daff3a19..f52b81c2 100644 --- a/.github/workflows/cli-interface.yml +++ b/.github/workflows/cli-interface.yml @@ -55,8 +55,19 @@ jobs: out="$RUNNER_TEMP/cli-surface" rm -rf "$out" && mkdir -p "$out/base" git -C .. archive "origin/$BASE_REF" dokimi | tar -x -C "$out/base" - PYTHONPATH="$out/base/dokimi" python scripts/cli_surface.py dump "$out/base.json" - PYTHONPATH=. python scripts/cli_surface.py dump "$out/head.json" + # A dump that raises (e.g. a PR that moves build_parser) must not fail the + # job: report "error" and leave the label as it is. + if ! PYTHONPATH="$out/base/dokimi" python scripts/cli_surface.py dump "$out/base.json" \ + || ! PYTHONPATH=. python scripts/cli_surface.py dump "$out/head.json"; then + printf '%s\n' "## CLI interface check could not run" "" \ + "The CLI surface could not be dumped from the base or the head; see the job log." \ + > "$out/diff.md" + cat "$out/diff.md" + echo "changed=error" >> "$GITHUB_OUTPUT" + echo "file=$out/diff.md" >> "$GITHUB_OUTPUT" + cat "$out/diff.md" >> "$GITHUB_STEP_SUMMARY" + exit 0 + fi rc=0 python scripts/cli_surface.py diff "$out/base.json" "$out/head.json" --markdown \ > "$out/diff.md" || rc=$? @@ -84,7 +95,16 @@ jobs: { owner, repo, issue_number }); const existing = comments.find((c) => c.user?.login === 'github-actions[bot]' && c.body && c.body.includes(marker)); - if (changed) { + if (process.env.CHANGED === 'error') { + // The surface is unknown: say so, and leave the label as it is. + const diff = fs.readFileSync(process.env.DIFF_FILE, 'utf8'); + const body = `${marker}\n${diff}`; + if (existing) { + await github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, body }); + } else { + await github.rest.issues.createComment({ owner, repo, issue_number, body }); + } + } else if (changed) { const diff = fs.readFileSync(process.env.DIFF_FILE, 'utf8'); const body = `${marker}\n${diff}\n_Updated on every push; the \`${label}\` ` + 'label is on while the diff is non-empty._'; diff --git a/dokimi/tests/test_cli_surface.py b/dokimi/tests/test_cli_surface.py index 09ad2eb4..ca62665f 100644 --- a/dokimi/tests/test_cli_surface.py +++ b/dokimi/tests/test_cli_surface.py @@ -106,6 +106,22 @@ def test_swapping_positionals_is_a_change(): assert any("position: 0 -> 1" in line for line in cs.diff_surfaces(a, b)) +def test_an_option_before_a_positional_does_not_move_the_positional(): + a = cs._command(_positional_parser(("base", "head")), "dokimi") + p = argparse.ArgumentParser(prog="dokimi") + p.add_argument("--version", action="version", version="1") + sub = p.add_subparsers(dest="cmd", required=True) + c = sub.add_parser("cmp") + c.add_argument("--opt") + c.add_argument("base") + c.add_argument("head") + b = cs._command(p, "dokimi") + assert cs.diff_surfaces(a, b) == [ + "~ `dokimi cmp` parameters changed:", + " + added option `--opt`", + ] + + def test_subcommand_required_change_is_reported(): a = cs._command(_positional_parser(("x", "y")), "dokimi") b = cs._command(_positional_parser(("x", "y"), required=False), "dokimi") @@ -213,6 +229,24 @@ def test_opaque_types_and_defaults_are_compared_by_content(old, new): assert cs.diff_surfaces(_one(**old), _one(**new)) +def test_a_validator_default_is_compared_by_content(): + def with_default(lo: int): + def positive(s: str, lo: int = lo) -> int: + if int(s) < lo: + raise argparse.ArgumentTypeError("too small") + return int(s) + return positive + + assert cs.diff_surfaces(_one(type=with_default(1)), _one(type=with_default(0))) + + +def test_default_objects_are_dumped_without_addresses(): + a = _one(default=object()) + b = _one(default=object()) + assert cs.diff_surfaces(a, b) == [] + assert "0x" not in json.dumps(a) + + def test_equal_opaque_values_are_stable(): kw = {"type": _make_validator(1), "default": {"a", "b"}} assert cs.diff_surfaces(_one(**kw), _one(type=_make_validator(1), default={"b", "a"})) == [] From 53f2c03baf05ad99997eb9f24eb9c0825a55a06a Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Mon, 19 Oct 2026 13:41:25 -0700 Subject: [PATCH 09/21] test(cli-surface): compare an opaque type without argparse.FileType argparse.FileType warns on construction in Python 3.14, and filterwarnings turns that into a collection error for this parametrize list. A small opaque type with a stable repr tests the same content comparison. Signed-off-by: Nikolay Petrov --- dokimi/tests/test_cli_surface.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/dokimi/tests/test_cli_surface.py b/dokimi/tests/test_cli_surface.py index ca62665f..b813533d 100644 --- a/dokimi/tests/test_cli_surface.py +++ b/dokimi/tests/test_cli_surface.py @@ -203,6 +203,20 @@ class _Mode(enum.Enum): B = 2 +class _Opener: + """An opaque argparse type with a stable repr. argparse.FileType would be the real one, but its + construction warns on Python 3.14, and filterwarnings=error makes that a collection error.""" + + def __init__(self, mode: str) -> None: + self.mode = mode + + def __call__(self, path: str) -> str: + return path + + def __repr__(self) -> str: + return f"_Opener({self.mode!r})" + + def _make_validator(lo: int): def positive(s: str) -> int: v = int(s) @@ -223,7 +237,7 @@ def _one(**kw) -> dict: ({"type": _make_validator(1)}, {"type": _make_validator(0)}), ({"default": _Mode.A}, {"default": _Mode.B}), ({"default": {"a"}}, {"default": {"a", "b"}}), - ({"type": argparse.FileType("r")}, {"type": argparse.FileType("w")}), + ({"type": _Opener("r")}, {"type": _Opener("w")}), ]) def test_opaque_types_and_defaults_are_compared_by_content(old, new): assert cs.diff_surfaces(_one(**old), _one(**new)) From 5595076b973f81ce4f6c4986060c2a5b6daeb6cd Mon Sep 17 00:00:00 2001 From: Alexander Rybkin Date: Tue, 20 Oct 2026 07:06:48 +0000 Subject: [PATCH 10/21] fix(ci): diff the CLI surface against the merge base, not the base tip The head is the merge commit made when the event fired; the base was the branch tip at job start. A CLI change merged to main in between (queue, re-run) was reported as removed by the PR. merge-base is HEAD^1 on CI and the fork point under make ci-local. Signed-off-by: Alexander Rybkin --- .github/workflows/cli-interface.yml | 8 ++-- dokimi/tests/test_cli_surface.py | 69 +++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 3 deletions(-) diff --git a/.github/workflows/cli-interface.yml b/.github/workflows/cli-interface.yml index f52b81c2..c549da14 100644 --- a/.github/workflows/cli-interface.yml +++ b/.github/workflows/cli-interface.yml @@ -43,8 +43,9 @@ jobs: python-version: '3.13' - name: Install dependencies run: pip install -r requirements.lock.txt - # The base branch's current tip (fetched by the checkout), as in the other - # diff-against-base steps. git archive, not a worktree: no git state changes. + # The base is the merge base of HEAD and the base branch, as in the other + # diff-against-base steps: on CI that is the merge commit's own base, even if + # the base branch moved on since. git archive, not a worktree: no git state changes. # Each dump imports the dokimi of its own tree through PYTHONPATH; the # head's script reads both, so the dump format is the same on each side. - name: Dump and diff the CLI surface @@ -54,7 +55,8 @@ jobs: run: | out="$RUNNER_TEMP/cli-surface" rm -rf "$out" && mkdir -p "$out/base" - git -C .. archive "origin/$BASE_REF" dokimi | tar -x -C "$out/base" + base=$(git -C .. merge-base HEAD "origin/$BASE_REF") + git -C .. archive "$base" dokimi | tar -x -C "$out/base" # A dump that raises (e.g. a PR that moves build_parser) must not fail the # job: report "error" and leave the label as it is. if ! PYTHONPATH="$out/base/dokimi" python scripts/cli_surface.py dump "$out/base.json" \ diff --git a/dokimi/tests/test_cli_surface.py b/dokimi/tests/test_cli_surface.py index b813533d..147776b0 100644 --- a/dokimi/tests/test_cli_surface.py +++ b/dokimi/tests/test_cli_surface.py @@ -296,3 +296,72 @@ def test_choices_with_different_types_are_reported(): def test_removed_command_lists_its_options(): lines = cs.diff_surfaces(_surface(), _surface(drop_cmd=True)) assert " - option `--n`" in lines and " - argument `questions`" in lines + + +_CLI = """import argparse + + +def build_parser(): + p = argparse.ArgumentParser(prog="dokimi") + p.add_argument("--old") +{extra} return p +""" + + +def _run_diff_step(tmp_path: Path, *, pr_adds: bool) -> str: + """Run the workflow's dump-and-diff step on a merge commit whose base branch moved on.""" + import os + import shutil + import subprocess + import sys + + import yaml + + if not all(shutil.which(t) for t in ("bash", "git", "tar")): + pytest.skip("needs bash, git and tar") + wf = yaml.safe_load((PROJECT.parent / ".github/workflows/cli-interface.yml").read_text(encoding="utf-8")) + (step,) = [s for s in wf["jobs"]["cli-surface"]["steps"] if s.get("id") == "diff"] + repo = tmp_path / "repo" + pkg = repo / "dokimi" / "dokimi" + pkg.mkdir(parents=True) + (repo / "dokimi" / "scripts").mkdir() + shutil.copy(_SCRIPT, repo / "dokimi" / "scripts" / "cli_surface.py") + (pkg / "__init__.py").write_text("", encoding="utf-8") + env = {k: v for k, v in os.environ.items() if not k.startswith(("GIT_", "PYTHON"))} + env.update(GIT_AUTHOR_NAME="t", GIT_AUTHOR_EMAIL="t@t", GIT_COMMITTER_NAME="t", + GIT_COMMITTER_EMAIL="t@t", GIT_CONFIG_NOSYSTEM="1", GIT_CONFIG_GLOBAL=os.devnull) + + def git(*args: str) -> str: + return subprocess.run(["git", "-c", "commit.gpgsign=false", *args], cwd=repo, env=env, + check=True, capture_output=True, text=True).stdout.strip() + + def commit(extra: str, msg: str) -> str: + (pkg / "cli.py").write_text(_CLI.format(extra=extra), encoding="utf-8") + git("add", "-A") + git("commit", "-q", "--allow-empty", "-m", msg) + return git("rev-parse", "HEAD") + + git("init", "-q", "-b", "main") + fork_point = commit("", "base") + commit(' p.add_argument("--pr-opt")\n' if pr_adds else "", "pr") + pr = git("rev-parse", "HEAD") + git("checkout", "-q", "--detach", fork_point) + git("merge", "-q", "--no-ff", "-m", "merge", pr) # what refs/pull/N/merge is + merge = git("rev-parse", "HEAD") + git("checkout", "-q", fork_point) + main_moved = commit(' p.add_argument("--main-opt")\n', "main moves on") + git("update-ref", "refs/remotes/origin/main", main_moved) + git("checkout", "-q", "--detach", merge) + output = tmp_path / "output" + env.update(BASE_REF="main", RUNNER_TEMP=str(tmp_path), GITHUB_OUTPUT=str(output), + GITHUB_STEP_SUMMARY=str(tmp_path / "summary"), + PATH=os.path.dirname(sys.executable) + os.pathsep + env.get("PATH", "")) + subprocess.run(["bash", "-e", "-c", step["run"]], cwd=repo / "dokimi", env=env, check=True, + capture_output=True, text=True) + return output.read_text(encoding="utf-8") + + +def test_workflow_ignores_base_branch_changes_made_after_the_merge_commit(tmp_path): + # main gained --main-opt after the merge commit was made: that is not this PR's change + assert "changed=0" in _run_diff_step(tmp_path, pr_adds=False) + assert "changed=1" in _run_diff_step(tmp_path / "b", pr_adds=True) From f8e2e23259feee7071d69fe34dde6739d793997c Mon Sep 17 00:00:00 2001 From: Alexander Rybkin Date: Tue, 20 Oct 2026 07:08:05 +0000 Subject: [PATCH 11/21] fix(cli-surface): hash a validator by its code, not its source text Rewording the docstring or a comment of positive_int, shared by 69 options, reported all 69 as changed. Hash the AST without docstrings instead; the new test also pins that a changed literal in the body is still reported. Signed-off-by: Alexander Rybkin --- dokimi/scripts/cli_surface.py | 27 ++++++++++++++++++++++----- dokimi/tests/test_cli_surface.py | 30 ++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 5 deletions(-) diff --git a/dokimi/scripts/cli_surface.py b/dokimi/scripts/cli_surface.py index 492aedf0..92836d82 100755 --- a/dokimi/scripts/cli_surface.py +++ b/dokimi/scripts/cli_surface.py @@ -24,12 +24,14 @@ from __future__ import annotations import argparse +import ast import enum import hashlib import inspect import json import re import sys +import textwrap from pathlib import Path, PurePath from typing import Any @@ -46,6 +48,24 @@ def _filled(cell: Any) -> bool: return True +def _code(fn: Any) -> str: + """A function's code without docstrings, comments or layout: rewording them is no CLI change.""" + try: + src = textwrap.dedent(inspect.getsource(fn)) + except (OSError, TypeError): + return "nosource" + try: + tree = ast.parse(src) + except SyntaxError: # e.g. a lambda, whose source is the whole line around it + return src + for node in ast.walk(tree): + if (isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)) and node.body + and isinstance(node.body[0], ast.Expr) and isinstance(node.body[0].value, ast.Constant) + and isinstance(node.body[0].value.value, str)): + node.body = node.body[1:] or [ast.Pass()] + return ast.dump(tree) + + def _value(value: Any) -> Any: """A JSON value standing for `value`, stable across runs (no object addresses).""" if value is None or isinstance(value, (bool, int, float, str)): @@ -61,11 +81,8 @@ def _value(value: Any) -> Any: if isinstance(value, enum.Enum): return repr(value) if inspect.isfunction(value): - # A validator's name alone hides a loosened body: add a hash of its source. - try: - src = inspect.getsource(value) - except (OSError, TypeError): - src = "nosource" + # A validator's name alone hides a loosened body: add a hash of its code. + src = _code(value) # Captured values and defaults (a factory's `lo`) change what the function accepts too. cells = [c.cell_contents for c in (value.__closure__ or ()) if _filled(c)] src += repr([_value(c) for c in cells]) + repr(_value(value.__defaults__)) diff --git a/dokimi/tests/test_cli_surface.py b/dokimi/tests/test_cli_surface.py index 147776b0..ba409d68 100644 --- a/dokimi/tests/test_cli_surface.py +++ b/dokimi/tests/test_cli_surface.py @@ -365,3 +365,33 @@ def test_workflow_ignores_base_branch_changes_made_after_the_merge_commit(tmp_pa # main gained --main-opt after the merge commit was made: that is not this PR's change assert "changed=0" in _run_diff_step(tmp_path, pr_adds=False) assert "changed=1" in _run_diff_step(tmp_path / "b", pr_adds=True) + + +_VALIDATOR = '''def positive_int(text): + """Reject non-positive integers.""" + value = int(text) + if value < 1: # zero is not positive + raise ValueError(text) + return value +''' + + +def _file_validator(tmp_path: Path, name: str, source: str): + """`positive_int` from a module file `validators.py`: same module and qualname every time.""" + path = tmp_path / name / "validators.py" + path.parent.mkdir() + path.write_text(source, encoding="utf-8") + spec = importlib.util.spec_from_file_location("validators", path) + assert spec and spec.loader + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module.positive_int + + +def test_a_validator_is_compared_by_code_not_by_its_docstring_or_comments(tmp_path): + base = _one(type=_file_validator(tmp_path, "base", _VALIDATOR)) + reworded = _VALIDATOR.replace("Reject non-positive", "Reject zero and negative").replace( + "# zero is not positive", "# 0 is rejected too").replace(" return value", "\n return value") + assert cs.diff_surfaces(base, _one(type=_file_validator(tmp_path, "reworded", reworded))) == [] + loosened = _one(type=_file_validator(tmp_path, "loosened", _VALIDATOR.replace("< 1", "< 0"))) + assert any("type:" in line for line in cs.diff_surfaces(base, loosened)) From 991b86154207226e3cd78c8acecc6ef64711251c Mon Sep 17 00:00:00 2001 From: Alexander Rybkin Date: Tue, 20 Oct 2026 07:09:02 +0000 Subject: [PATCH 12/21] fix(cli-surface): tell choices=[] from no choices choices=[] rejects every value and None accepts any, but the truthiness check dumped both as None, so adding or removing the restriction was silent. Signed-off-by: Alexander Rybkin --- dokimi/scripts/cli_surface.py | 2 +- dokimi/tests/test_cli_surface.py | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/dokimi/scripts/cli_surface.py b/dokimi/scripts/cli_surface.py index 92836d82..568cab37 100755 --- a/dokimi/scripts/cli_surface.py +++ b/dokimi/scripts/cli_surface.py @@ -108,7 +108,7 @@ def _param(action: argparse.Action, position: int | None, group_key: str | None "default": _value(action.default), "const": _value(action.const), "type": _value(action.type), - "choices": sorted(repr(_value(c)) for c in action.choices) if action.choices else None, + "choices": sorted(repr(_value(c)) for c in action.choices) if action.choices is not None else None, "hidden": action.help == argparse.SUPPRESS, "action": type(action).__name__, "group_key": group_key, diff --git a/dokimi/tests/test_cli_surface.py b/dokimi/tests/test_cli_surface.py index ba409d68..49814004 100644 --- a/dokimi/tests/test_cli_surface.py +++ b/dokimi/tests/test_cli_surface.py @@ -395,3 +395,8 @@ def test_a_validator_is_compared_by_code_not_by_its_docstring_or_comments(tmp_pa assert cs.diff_surfaces(base, _one(type=_file_validator(tmp_path, "reworded", reworded))) == [] loosened = _one(type=_file_validator(tmp_path, "loosened", _VALIDATOR.replace("< 1", "< 0"))) assert any("type:" in line for line in cs.diff_surfaces(base, loosened)) + + +def test_empty_choices_are_not_unrestricted(): + # choices=[] rejects every value; None accepts any + assert any("choices: None -> []" in line for line in cs.diff_surfaces(_one(), _one(choices=[]))) From 7ae66966fb389063db1204aa3c856d9919076a39 Mon Sep 17 00:00:00 2001 From: Alexander Rybkin Date: Tue, 20 Oct 2026 07:09:02 +0000 Subject: [PATCH 13/21] fix(cli-surface): count the subcommand as a positional slot Moving a positional across add_subparsers() turns `dokimi WS run` into `dokimi run WS`, but both dumped the positional at index 0. Signed-off-by: Alexander Rybkin --- dokimi/scripts/cli_surface.py | 1 + dokimi/tests/test_cli_surface.py | 14 ++++++++++++++ 2 files changed, 15 insertions(+) diff --git a/dokimi/scripts/cli_surface.py b/dokimi/scripts/cli_surface.py index 568cab37..8c6482ee 100755 --- a/dokimi/scripts/cli_surface.py +++ b/dokimi/scripts/cli_surface.py @@ -139,6 +139,7 @@ def _command(parser: argparse.ArgumentParser, path: str) -> dict[str, Any]: for action in parser._actions: if isinstance(action, argparse._SubParsersAction): node["subcommand_required"] = bool(action.required) + position += 1 # the subcommand takes a positional slot too for name, sub in sorted(action.choices.items()): node["subcommands"][name] = _command(sub, f"{path} {name}") elif not isinstance(action, argparse._HelpAction): diff --git a/dokimi/tests/test_cli_surface.py b/dokimi/tests/test_cli_surface.py index 49814004..cbf6e221 100644 --- a/dokimi/tests/test_cli_surface.py +++ b/dokimi/tests/test_cli_surface.py @@ -400,3 +400,17 @@ def test_a_validator_is_compared_by_code_not_by_its_docstring_or_comments(tmp_pa def test_empty_choices_are_not_unrestricted(): # choices=[] rejects every value; None accepts any assert any("choices: None -> []" in line for line in cs.diff_surfaces(_one(), _one(choices=[]))) + + +def test_moving_a_positional_across_the_subcommand_is_a_change(): + def parser(before: bool) -> argparse.ArgumentParser: + p = argparse.ArgumentParser(prog="dokimi") + if before: + p.add_argument("workspace") + p.add_subparsers(dest="cmd", required=True).add_parser("run") + if not before: + p.add_argument("workspace") + return p + # `dokimi WS run` becomes `dokimi run WS` + lines = cs.diff_surfaces(cs._command(parser(True), "dokimi"), cs._command(parser(False), "dokimi")) + assert any("position: 0 -> 1" in line for line in lines), lines From 85959537110502c0e12b7985324566ae685b155b Mon Sep 17 00:00:00 2001 From: Alexander Rybkin Date: Tue, 20 Oct 2026 07:09:02 +0000 Subject: [PATCH 14/21] fix(cli-surface): dump -h/--help like any other option Skipping _HelpAction hid add_help=False and a dropped -h alias. Help text is not dumped, so only the option itself is compared. Signed-off-by: Alexander Rybkin --- dokimi/scripts/cli_surface.py | 4 ++-- dokimi/tests/test_cli_surface.py | 8 +++++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/dokimi/scripts/cli_surface.py b/dokimi/scripts/cli_surface.py index 8c6482ee..5a821325 100755 --- a/dokimi/scripts/cli_surface.py +++ b/dokimi/scripts/cli_surface.py @@ -142,8 +142,8 @@ def _command(parser: argparse.ArgumentParser, path: str) -> dict[str, Any]: position += 1 # the subcommand takes a positional slot too for name, sub in sorted(action.choices.items()): node["subcommands"][name] = _command(sub, f"{path} {name}") - elif not isinstance(action, argparse._HelpAction): - # --version is kept (its version string is not dumped, so a release is no change) + else: + # -h/--help and --version are kept (help text and the version string are not dumped) group_info = action_to_group.get(action) group_key = group_info[0] if group_info else None group_required = group_info[1] if group_info else None diff --git a/dokimi/tests/test_cli_surface.py b/dokimi/tests/test_cli_surface.py index cbf6e221..cc12389b 100644 --- a/dokimi/tests/test_cli_surface.py +++ b/dokimi/tests/test_cli_surface.py @@ -51,7 +51,7 @@ def test_dump_of_the_real_cli_is_json_and_deterministic(): def test_nested_subcommands_and_hidden_options_are_walked(): flat = cs._flatten(_surface(), {}) assert set(flat) == {"dokimi", "dokimi run", "dokimi repo", "dokimi repo scan"} - (hidden,) = flat["dokimi repo scan"]["params"] + (hidden,) = [p for p in flat["dokimi repo scan"]["params"] if p["key"] == "--hidden"] assert hidden["hidden"] is True @@ -414,3 +414,9 @@ def parser(before: bool) -> argparse.ArgumentParser: # `dokimi WS run` becomes `dokimi run WS` lines = cs.diff_surfaces(cs._command(parser(True), "dokimi"), cs._command(parser(False), "dokimi")) assert any("position: 0 -> 1" in line for line in lines), lines + + +def test_removing_the_help_option_is_a_change(): + def surface(**kw: bool) -> dict: + return cs._command(argparse.ArgumentParser(prog="dokimi", **kw), "dokimi") + assert " - removed option `--help/-h`" in cs.diff_surfaces(surface(), surface(add_help=False)) From 5f997d3e0e0fcf7865fb3823bf9edd4238cdef3d Mon Sep 17 00:00:00 2001 From: Alexander Rybkin Date: Tue, 20 Oct 2026 07:09:02 +0000 Subject: [PATCH 15/21] fix(cli-surface): record allow_abbrev per command With it off, `--ver` stops meaning `--version` and exits 2; the dump had no field for it, so the change gave an empty diff. Signed-off-by: Alexander Rybkin --- dokimi/scripts/cli_surface.py | 6 +++++- dokimi/tests/test_cli_surface.py | 7 +++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/dokimi/scripts/cli_surface.py b/dokimi/scripts/cli_surface.py index 5a821325..08601daa 100755 --- a/dokimi/scripts/cli_surface.py +++ b/dokimi/scripts/cli_surface.py @@ -118,7 +118,7 @@ def _param(action: argparse.Action, position: int | None, group_key: str | None def _command(parser: argparse.ArgumentParser, path: str) -> dict[str, Any]: node: dict[str, Any] = {"path": path, "params": [], "subcommands": {}, - "subcommand_required": None} + "subcommand_required": None, "allow_abbrev": parser.allow_abbrev} position = 0 # Build a map from action to its mutually exclusive group (if any). @@ -196,6 +196,10 @@ def diff_surfaces(base: dict[str, Any], head: dict[str, Any]) -> list[str]: lines.append(f"~ `{path}` subcommand required: " f"{old[path].get('subcommand_required')!r} -> " f"{new[path].get('subcommand_required')!r}") + if old[path].get("allow_abbrev") != new[path].get("allow_abbrev"): + # with it, `--ver` is accepted for `--version` + lines.append(f"~ `{path}` option abbreviations allowed: " + f"{old[path].get('allow_abbrev')!r} -> {new[path].get('allow_abbrev')!r}") param_lines = _diff_params(old[path]["params"], new[path]["params"]) if param_lines: lines.append(f"~ `{path}` parameters changed:") diff --git a/dokimi/tests/test_cli_surface.py b/dokimi/tests/test_cli_surface.py index cc12389b..2fb94d99 100644 --- a/dokimi/tests/test_cli_surface.py +++ b/dokimi/tests/test_cli_surface.py @@ -420,3 +420,10 @@ def test_removing_the_help_option_is_a_change(): def surface(**kw: bool) -> dict: return cs._command(argparse.ArgumentParser(prog="dokimi", **kw), "dokimi") assert " - removed option `--help/-h`" in cs.diff_surfaces(surface(), surface(add_help=False)) + + +def test_allow_abbrev_change_is_reported(): + def surface(abbrev: bool) -> dict: + return cs._command(argparse.ArgumentParser(prog="dokimi", allow_abbrev=abbrev), "dokimi") + assert cs.diff_surfaces(surface(True), surface(False)) == [ + "~ `dokimi` option abbreviations allowed: True -> False"] From da438299ef17496adabeef4bc2a97a4008461ee3 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Tue, 20 Oct 2026 01:01:40 -0700 Subject: [PATCH 16/21] docs(cli-surface): state that validators reading module state are not compared A validator that consults module state at parse time can reject more input without any change in the parser dump, so the diff is empty for it. Say so in the module docstring, as the review asked. Signed-off-by: Nikolay Petrov --- dokimi/scripts/cli_surface.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dokimi/scripts/cli_surface.py b/dokimi/scripts/cli_surface.py index 08601daa..b9420932 100755 --- a/dokimi/scripts/cli_surface.py +++ b/dokimi/scripts/cli_surface.py @@ -14,6 +14,11 @@ whichever `dokimi` is first on sys.path: the workflow points PYTHONPATH at a checkout of each ref in turn. +Limit: only the parser tree is compared. A validator that reads module state +when it runs (for example one that checks against `JUDGE_ROLES`) can reject +more input after a change without any change in the dump, so the diff is empty +for it. Review such changes by hand. + Usage: python scripts/cli_surface.py dump [OUT.json] python scripts/cli_surface.py diff BASE.json HEAD.json [--markdown] From a0be71f4d9e1af88eb9369be5e84fcd59d080e9a Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Tue, 20 Oct 2026 04:04:11 -0700 Subject: [PATCH 17/21] fix(cli-surface): hash keyword-only defaults, classes and enums; test the error path Address review of #88: - A validator's keyword-only defaults and a self-referencing closure are now covered: the hash includes __kwdefaults__, and a function reached again through its own closure is named, not hashed again (no RecursionError). - A class used as type= (an enum, or a class with methods) is hashed by its attributes, so a new enum member or a changed method is reported. - Rename the per-parameter group variables so mypy's str/None assignment error goes away. - Add a workflow test for the changed=error path (head dump raises). - Describe the workflow's base as the merge base, not the base branch tip. Signed-off-by: Nikolay Petrov --- .github/workflows/cli-interface.yml | 6 +-- dokimi/scripts/cli_surface.py | 47 ++++++++++++++++----- dokimi/tests/test_cli_surface.py | 64 +++++++++++++++++++++++++---- 3 files changed, 95 insertions(+), 22 deletions(-) diff --git a/.github/workflows/cli-interface.yml b/.github/workflows/cli-interface.yml index c549da14..3a8a281d 100644 --- a/.github/workflows/cli-interface.yml +++ b/.github/workflows/cli-interface.yml @@ -4,9 +4,9 @@ name: cli-interface # Makes every change to how users call dokimi visible on the PR: dumps the CLI -# surface (commands, options, arguments, defaults, choices) of the base -# branch's tip and of the PR head with dokimi/scripts/cli_surface.py, diffs -# them and, when they differ, labels the PR `cli-interface-change` and posts a +# surface (commands, options, arguments, defaults, choices) of the merge base of +# the PR with its base branch and of the PR head with dokimi/scripts/cli_surface.py, +# diffs them and, when they differ, labels the PR `cli-interface-change` and posts a # sticky comment listing each added, removed or changed command and option. # The comment is updated on every push, and the label removed once the surface # matches the base again. The check reports; it never fails a PR for a change. diff --git a/dokimi/scripts/cli_surface.py b/dokimi/scripts/cli_surface.py index b9420932..9d14ff3d 100755 --- a/dokimi/scripts/cli_surface.py +++ b/dokimi/scripts/cli_surface.py @@ -45,6 +45,11 @@ "position", "const", "group_key", "group_required") +# Class attributes that name or locate the class rather than define it. +_CLASS_NOISE = frozenset({"__dict__", "__doc__", "__firstlineno__", "__module__", "__qualname__", + "__weakref__"}) + + def _filled(cell: Any) -> bool: try: _ = cell.cell_contents @@ -71,28 +76,48 @@ def _code(fn: Any) -> str: return ast.dump(tree) -def _value(value: Any) -> Any: - """A JSON value standing for `value`, stable across runs (no object addresses).""" +def _value(value: Any, _seen: frozenset[int] = frozenset()) -> Any: + """A JSON value standing for `value`, stable across runs (no object addresses). + + `_seen` holds the functions being hashed up the call chain: a validator that reaches + itself through a closure cell is named, not hashed again. + """ if value is None or isinstance(value, (bool, int, float, str)): return value if isinstance(value, (list, tuple)): - return [_value(v) for v in value] + return [_value(v, _seen) for v in value] if isinstance(value, dict): - return {str(k): _value(v) for k, v in value.items()} + return {str(k): _value(v, _seen) for k, v in value.items()} if isinstance(value, PurePath): return str(value) if isinstance(value, (set, frozenset)): - return sorted(repr(_value(v)) for v in value) + return sorted(repr(_value(v, _seen)) for v in value) if isinstance(value, enum.Enum): return repr(value) - if inspect.isfunction(value): + if inspect.isfunction(value) and id(value) not in _seen: # A validator's name alone hides a loosened body: add a hash of its code. src = _code(value) - # Captured values and defaults (a factory's `lo`) change what the function accepts too. + # Captured values and defaults (a factory's `lo`, a keyword-only `lim`) change what the + # function accepts too. + seen = _seen | {id(value)} cells = [c.cell_contents for c in (value.__closure__ or ()) if _filled(c)] - src += repr([_value(c) for c in cells]) + repr(_value(value.__defaults__)) + src += repr([_value(c, seen) for c in cells]) + src += repr(_value(value.__defaults__, seen)) + repr(_value(value.__kwdefaults__, seen)) + digest = hashlib.sha256(src.encode("utf-8")).hexdigest()[:12] + return f"{value.__module__}.{value.__qualname__}#{digest}" + if inspect.isclass(value) and value.__module__ != "builtins" and id(value) not in _seen: + # A class used as a type is hashed by its attributes (their code, through _value), so a + # changed method or a new enum member changes it. Its source is not used: a class defined + # in a module that is not in sys.modules has none. + seen = _seen | {id(value)} + attrs = {k: v for k, v in vars(value).items() if k not in _CLASS_NOISE} + src = repr(_value(attrs, seen)) + if issubclass(value, enum.Enum): + src += repr([(m.name, _value(m.value)) for m in value]) digest = hashlib.sha256(src.encode("utf-8")).hexdigest()[:12] return f"{value.__module__}.{value.__qualname__}#{digest}" + if inspect.isfunction(value) and id(value) in _seen: # reached again through its own closure + return f"{value.__module__}.{value.__qualname__}" name = getattr(value, "__name__", None) if isinstance(name, str): return name @@ -150,10 +175,10 @@ def _command(parser: argparse.ArgumentParser, path: str) -> dict[str, Any]: else: # -h/--help and --version are kept (help text and the version string are not dumped) group_info = action_to_group.get(action) - group_key = group_info[0] if group_info else None - group_required = group_info[1] if group_info else None + param_group = group_info[0] if group_info else None + param_required = group_info[1] if group_info else None node["params"].append(_param(action, None if action.option_strings else position, - group_key, group_required)) + param_group, param_required)) position += 0 if action.option_strings else 1 node["params"].sort(key=lambda p: (p["kind"], p["key"])) return node diff --git a/dokimi/tests/test_cli_surface.py b/dokimi/tests/test_cli_surface.py index 2fb94d99..2b71b053 100644 --- a/dokimi/tests/test_cli_surface.py +++ b/dokimi/tests/test_cli_surface.py @@ -254,6 +254,39 @@ def positive(s: str, lo: int = lo) -> int: assert cs.diff_surfaces(_one(type=with_default(1)), _one(type=with_default(0))) +def test_a_validator_keyword_only_default_is_compared_by_content(): + def kw(lim: int): + def take(s: str, *, lim: int = lim) -> str: + return s[:lim] + return take + + assert cs.diff_surfaces(_one(type=kw(3)), _one(type=kw(30))) + + +def test_a_validator_that_calls_itself_through_a_closure_is_dumped(): + def mk(): + def loop(s: str) -> str: + return loop(s) if s == "x" else s + return loop + + assert cs.diff_surfaces(_one(type=mk()), _one(type=mk())) == [] + + +def test_enum_type_is_compared_by_its_members(): + one = enum.Enum("Mode", "a") + two = enum.Enum("Mode", "a b") + assert cs.diff_surfaces(_one(type=one), _one(type=two)) + assert cs.diff_surfaces(_one(type=one), _one(type=enum.Enum("Mode", "a"))) == [] + + +def test_class_type_is_compared_by_its_code(tmp_path): + source = "class Check:\n def __call__(self, s):\n return s\n" + loosened = source.replace("return s", "return s[:1]") + base = _one(type=_file_validator(tmp_path, "base", source, attr="Check")) + head = _one(type=_file_validator(tmp_path, "head", loosened, attr="Check")) + assert cs.diff_surfaces(base, head) + + def test_default_objects_are_dumped_without_addresses(): a = _one(default=object()) b = _one(default=object()) @@ -308,8 +341,11 @@ def build_parser(): """ -def _run_diff_step(tmp_path: Path, *, pr_adds: bool) -> str: - """Run the workflow's dump-and-diff step on a merge commit whose base branch moved on.""" +def _run_diff_step(tmp_path: Path, *, pr_adds: bool, pr_moves_build_parser: bool = False) -> str: + """Run the workflow's dump-and-diff step on a merge commit whose base branch moved on. + + With `pr_moves_build_parser` the PR deletes dokimi/cli.py, so the head dump raises. + """ import os import shutil import subprocess @@ -335,15 +371,19 @@ def git(*args: str) -> str: return subprocess.run(["git", "-c", "commit.gpgsign=false", *args], cwd=repo, env=env, check=True, capture_output=True, text=True).stdout.strip() - def commit(extra: str, msg: str) -> str: - (pkg / "cli.py").write_text(_CLI.format(extra=extra), encoding="utf-8") + def commit(extra: str, msg: str, *, cli: bool = True) -> str: + if cli: + (pkg / "cli.py").write_text(_CLI.format(extra=extra), encoding="utf-8") + else: + (pkg / "cli.py").unlink(missing_ok=True) git("add", "-A") git("commit", "-q", "--allow-empty", "-m", msg) return git("rev-parse", "HEAD") git("init", "-q", "-b", "main") fork_point = commit("", "base") - commit(' p.add_argument("--pr-opt")\n' if pr_adds else "", "pr") + commit(' p.add_argument("--pr-opt")\n' if pr_adds else "", "pr", + cli=not pr_moves_build_parser) pr = git("rev-parse", "HEAD") git("checkout", "-q", "--detach", fork_point) git("merge", "-q", "--no-ff", "-m", "merge", pr) # what refs/pull/N/merge is @@ -367,6 +407,14 @@ def test_workflow_ignores_base_branch_changes_made_after_the_merge_commit(tmp_pa assert "changed=1" in _run_diff_step(tmp_path / "b", pr_adds=True) +def test_workflow_reports_error_and_passes_when_the_head_cannot_be_dumped(tmp_path): + # a PR that moves build_parser: the head dump raises; the step must still exit 0 (checked by + # _run_diff_step) and report changed=error so the label is left alone + output = _run_diff_step(tmp_path, pr_adds=False, pr_moves_build_parser=True) + assert "changed=error" in output + assert "changed=0" not in output and "changed=1" not in output + + _VALIDATOR = '''def positive_int(text): """Reject non-positive integers.""" value = int(text) @@ -376,8 +424,8 @@ def test_workflow_ignores_base_branch_changes_made_after_the_merge_commit(tmp_pa ''' -def _file_validator(tmp_path: Path, name: str, source: str): - """`positive_int` from a module file `validators.py`: same module and qualname every time.""" +def _file_validator(tmp_path: Path, name: str, source: str, attr: str = "positive_int"): + """`attr` from a module file `validators.py`: same module and qualname every time.""" path = tmp_path / name / "validators.py" path.parent.mkdir() path.write_text(source, encoding="utf-8") @@ -385,7 +433,7 @@ def _file_validator(tmp_path: Path, name: str, source: str): assert spec and spec.loader module = importlib.util.module_from_spec(spec) spec.loader.exec_module(module) - return module.positive_int + return getattr(module, attr) def test_a_validator_is_compared_by_code_not_by_its_docstring_or_comments(tmp_path): From 9c281300d66c64c3520ec6e43cd2024eceb67270 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Tue, 20 Oct 2026 14:36:42 -0700 Subject: [PATCH 18/21] fix(cli-surface): hash inherited, static, class and property members in sorted order A class used as type= is now hashed over its whole MRO, with static methods, classmethods and properties hashed by their functions, and the member order no longer changes the hash. The Limit paragraph names what is still compared by name only. Tests cover each case, and the error path of the workflow step now checks diff.md and the file= output. Signed-off-by: Nikolay Petrov --- dokimi/scripts/cli_surface.py | 23 ++++++++++++---- dokimi/tests/test_cli_surface.py | 47 ++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 6 deletions(-) diff --git a/dokimi/scripts/cli_surface.py b/dokimi/scripts/cli_surface.py index 9d14ff3d..8d883be0 100755 --- a/dokimi/scripts/cli_surface.py +++ b/dokimi/scripts/cli_surface.py @@ -17,7 +17,8 @@ Limit: only the parser tree is compared. A validator that reads module state when it runs (for example one that checks against `JUDGE_ROLES`) can reject more input after a change without any change in the dump, so the diff is empty -for it. Review such changes by hand. +for it. Review such changes by hand. Callable instances, `functools.partial` objects +and metaclasses are reduced to their name: a change inside them is not compared. Usage: python scripts/cli_surface.py dump [OUT.json] @@ -94,6 +95,10 @@ def _value(value: Any, _seen: frozenset[int] = frozenset()) -> Any: return sorted(repr(_value(v, _seen)) for v in value) if isinstance(value, enum.Enum): return repr(value) + if isinstance(value, (staticmethod, classmethod)): # a class's members: hash the function + return _value(value.__func__, _seen) + if isinstance(value, property): + return [_value(f, _seen) for f in (value.fget, value.fset, value.fdel)] if inspect.isfunction(value) and id(value) not in _seen: # A validator's name alone hides a loosened body: add a hash of its code. src = _code(value) @@ -106,12 +111,18 @@ def _value(value: Any, _seen: frozenset[int] = frozenset()) -> Any: digest = hashlib.sha256(src.encode("utf-8")).hexdigest()[:12] return f"{value.__module__}.{value.__qualname__}#{digest}" if inspect.isclass(value) and value.__module__ != "builtins" and id(value) not in _seen: - # A class used as a type is hashed by its attributes (their code, through _value), so a - # changed method or a new enum member changes it. Its source is not used: a class defined - # in a module that is not in sys.modules has none. + # A class used as a type is hashed by its attributes, inherited ones included (their code, + # through _value), in sorted order so the definition order does not matter. Its source is + # not used: a class defined in a module that is not in sys.modules has none. seen = _seen | {id(value)} - attrs = {k: v for k, v in vars(value).items() if k not in _CLASS_NOISE} - src = repr(_value(attrs, seen)) + attrs: dict[str, Any] = {} + for base in value.__mro__: + if base.__module__ != "builtins": + for k, v in vars(base).items(): + if k not in _CLASS_NOISE: + attrs.setdefault(k, v) # the most derived definition wins + bases = [f"{b.__module__}.{b.__qualname__}" for b in value.__mro__ if b.__module__ != "builtins"] + src = repr(_value(dict(sorted(attrs.items())), seen)) + repr(bases) if issubclass(value, enum.Enum): src += repr([(m.name, _value(m.value)) for m in value]) digest = hashlib.sha256(src.encode("utf-8")).hexdigest()[:12] diff --git a/dokimi/tests/test_cli_surface.py b/dokimi/tests/test_cli_surface.py index 2b71b053..47503013 100644 --- a/dokimi/tests/test_cli_surface.py +++ b/dokimi/tests/test_cli_surface.py @@ -413,6 +413,10 @@ def test_workflow_reports_error_and_passes_when_the_head_cannot_be_dumped(tmp_pa output = _run_diff_step(tmp_path, pr_adds=False, pr_moves_build_parser=True) assert "changed=error" in output assert "changed=0" not in output and "changed=1" not in output + # the github-script step reads diff.md through the file= output: it must be the error text + diff_md = tmp_path / "cli-surface" / "diff.md" + assert f"file={diff_md}" in output + assert "## CLI interface check could not run" in diff_md.read_text(encoding="utf-8") _VALIDATOR = '''def positive_int(text): @@ -475,3 +479,46 @@ def surface(abbrev: bool) -> dict: return cs._command(argparse.ArgumentParser(prog="dokimi", allow_abbrev=abbrev), "dokimi") assert cs.diff_surfaces(surface(True), surface(False)) == [ "~ `dokimi` option abbreviations allowed: True -> False"] + + +_CLASS_MEMBERS = { + "staticmethod": " @staticmethod\n def run(s):\n return BODY\n", + "classmethod": " @classmethod\n def run(cls, s):\n return BODY\n", + "property": " @property\n def run(self):\n return BODY\n", +} + + +@pytest.mark.parametrize("kind", sorted(_CLASS_MEMBERS)) +def test_a_class_type_is_compared_by_its_static_method_classmethod_and_property(tmp_path, kind): + def loaded(name: str, body: str): + source = "class Check:\n" + _CLASS_MEMBERS[kind].replace("BODY", body) + return _one(type=_file_validator(tmp_path, f"{kind}-{name}", source, attr="Check")) + assert cs.diff_surfaces(loaded("base", "1"), loaded("loose", "2")) + + +def test_a_class_type_is_compared_by_code_it_inherits(tmp_path): + def loaded(name: str, body: str): + source = f"class Base:\n def check(self, s):\n return {body}\n\nclass Check(Base):\n pass\n" + return _one(type=_file_validator(tmp_path, name, source, attr="Check")) + assert cs.diff_surfaces(loaded("base", "s"), loaded("loose", "s[:1]")) + + +def test_the_order_of_a_class_members_does_not_change_its_hash(tmp_path): + ab = "class Check:\n def a(self, s):\n return s\n\n def b(self, s):\n return s\n" + ba = "class Check:\n def b(self, s):\n return s\n\n def a(self, s):\n return s\n" + assert cs.diff_surfaces(_one(type=_file_validator(tmp_path, "ab", ab, attr="Check")), + _one(type=_file_validator(tmp_path, "ba", ba, attr="Check"))) == [] + + +def test_a_class_that_holds_itself_is_dumped(tmp_path): + cls = _file_validator(tmp_path, "selfref", "class Check:\n me = None\n\nCheck.me = Check\n", + attr="Check") + assert cs.diff_surfaces(_one(type=cls), _one(type=cls)) == [] + + +def test_a_class_docstring_is_not_a_change(tmp_path): + base = _file_validator(tmp_path, "doc-base", "class Check:\n def run(self, s):\n return s\n", + attr="Check") + reworded = _file_validator(tmp_path, "doc-new", "import os\n\nclass Check:\n '''Reworded.'''\n" + " def run(self, s):\n return s\n", attr="Check") + assert cs.diff_surfaces(_one(type=base), _one(type=reworded)) == [] From cf28a0c4067318d590920db553ebd1997f75a804 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Tue, 20 Oct 2026 21:58:30 -0700 Subject: [PATCH 19/21] docs(ci): say fork PRs see the cli diff only in the job summary Fork PR tokens are read-only, so the label and comment are skipped there. refactor(cli-surface): drop redundant enum branch in class hash vars(cls) already carries _member_map_, so the extra member block added nothing. Signed-off-by: Nikolay Petrov --- .github/workflows/cli-interface.yml | 2 ++ dokimi/scripts/cli_surface.py | 2 -- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/cli-interface.yml b/.github/workflows/cli-interface.yml index 3a8a281d..d1de2a65 100644 --- a/.github/workflows/cli-interface.yml +++ b/.github/workflows/cli-interface.yml @@ -10,6 +10,8 @@ name: cli-interface # sticky comment listing each added, removed or changed command and option. # The comment is updated on every push, and the label removed once the surface # matches the base again. The check reports; it never fails a PR for a change. +# Fork PRs get a read-only token, so there the label and comment are skipped and +# the CLI diff is shown only in the job summary. on: pull_request: diff --git a/dokimi/scripts/cli_surface.py b/dokimi/scripts/cli_surface.py index 8d883be0..e5244943 100755 --- a/dokimi/scripts/cli_surface.py +++ b/dokimi/scripts/cli_surface.py @@ -123,8 +123,6 @@ def _value(value: Any, _seen: frozenset[int] = frozenset()) -> Any: attrs.setdefault(k, v) # the most derived definition wins bases = [f"{b.__module__}.{b.__qualname__}" for b in value.__mro__ if b.__module__ != "builtins"] src = repr(_value(dict(sorted(attrs.items())), seen)) + repr(bases) - if issubclass(value, enum.Enum): - src += repr([(m.name, _value(m.value)) for m in value]) digest = hashlib.sha256(src.encode("utf-8")).hexdigest()[:12] return f"{value.__module__}.{value.__qualname__}#{digest}" if inspect.isfunction(value) and id(value) in _seen: # reached again through its own closure From dccdf2a549db4a08839b3ac83475696a47ebe063 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Wed, 21 Oct 2026 11:53:06 -0700 Subject: [PATCH 20/21] fix(cli-surface): stable enum and builtin-base class hashes; crashed diff exits 2 and reports error Signed-off-by: Nikolay Petrov --- .github/workflows/cli-interface.yml | 13 +++-- dokimi/scripts/cli_surface.py | 26 ++++++--- dokimi/tests/test_cli_surface.py | 81 ++++++++++++++++++++++++++++- 3 files changed, 105 insertions(+), 15 deletions(-) diff --git a/.github/workflows/cli-interface.yml b/.github/workflows/cli-interface.yml index d1de2a65..2ab74dbe 100644 --- a/.github/workflows/cli-interface.yml +++ b/.github/workflows/cli-interface.yml @@ -59,24 +59,27 @@ jobs: rm -rf "$out" && mkdir -p "$out/base" base=$(git -C .. merge-base HEAD "origin/$BASE_REF") git -C .. archive "$base" dokimi | tar -x -C "$out/base" - # A dump that raises (e.g. a PR that moves build_parser) must not fail the + # A dump or diff that raises (e.g. a PR that moves build_parser) must not fail the # job: report "error" and leave the label as it is. - if ! PYTHONPATH="$out/base/dokimi" python scripts/cli_surface.py dump "$out/base.json" \ - || ! PYTHONPATH=. python scripts/cli_surface.py dump "$out/head.json"; then + report_error() { printf '%s\n' "## CLI interface check could not run" "" \ - "The CLI surface could not be dumped from the base or the head; see the job log." \ + "The CLI surface could not be dumped or diffed from the base or the head; see the job log." \ > "$out/diff.md" cat "$out/diff.md" echo "changed=error" >> "$GITHUB_OUTPUT" echo "file=$out/diff.md" >> "$GITHUB_OUTPUT" cat "$out/diff.md" >> "$GITHUB_STEP_SUMMARY" exit 0 + } + if ! PYTHONPATH="$out/base/dokimi" python scripts/cli_surface.py dump "$out/base.json" \ + || ! PYTHONPATH=. python scripts/cli_surface.py dump "$out/head.json"; then + report_error fi rc=0 python scripts/cli_surface.py diff "$out/base.json" "$out/head.json" --markdown \ > "$out/diff.md" || rc=$? + if [ "$rc" -gt 1 ]; then report_error; fi cat "$out/diff.md" - if [ "$rc" -gt 1 ]; then exit "$rc"; fi echo "changed=$rc" >> "$GITHUB_OUTPUT" echo "file=$out/diff.md" >> "$GITHUB_OUTPUT" cat "$out/diff.md" >> "$GITHUB_STEP_SUMMARY" diff --git a/dokimi/scripts/cli_surface.py b/dokimi/scripts/cli_surface.py index e5244943..763871a9 100755 --- a/dokimi/scripts/cli_surface.py +++ b/dokimi/scripts/cli_surface.py @@ -8,7 +8,8 @@ surface of the PR's base and of its head, diffs the two and, when they differ, labels the PR `cli-interface-change` and posts a sticky comment listing every added, removed or changed command and option. So each change to how users call -dokimi is visible in review and stays on record in the PR. +dokimi is visible in review and stays on record in the PR. A PR from a fork has a +read-only token: it gets the diff in the job summary only, with no label or comment. The dump walks the argparse tree `dokimi.cli.build_parser()` returns, of whichever `dokimi` is first on sys.path: the workflow points PYTHONPATH at a @@ -19,12 +20,16 @@ more input after a change without any change in the dump, so the diff is empty for it. Review such changes by hand. Callable instances, `functools.partial` objects and metaclasses are reduced to their name: a change inside them is not compared. +A class used as `type=` is hashed by the attributes of its non-builtin bases and its +own, and by its base names (builtin ones included); the attributes of a builtin base +are not hashed, and neither is the body of a `functools.lru_cache` or `cached_property` +member (only its name). Usage: python scripts/cli_surface.py dump [OUT.json] python scripts/cli_surface.py diff BASE.json HEAD.json [--markdown] -`diff` exits 0 when the surfaces match, 1 when they differ. +`diff` exits 0 when the surfaces match, 1 when they differ, 2 when a dump cannot be read. """ from __future__ import annotations @@ -48,7 +53,8 @@ # Class attributes that name or locate the class rather than define it. _CLASS_NOISE = frozenset({"__dict__", "__doc__", "__firstlineno__", "__module__", "__qualname__", - "__weakref__"}) + "__weakref__", + "_value2member_map_"}) # the last is keyed by str(value), which may hold an address def _filled(cell: Any) -> bool: @@ -94,7 +100,7 @@ def _value(value: Any, _seen: frozenset[int] = frozenset()) -> Any: if isinstance(value, (set, frozenset)): return sorted(repr(_value(v, _seen)) for v in value) if isinstance(value, enum.Enum): - return repr(value) + return [repr(type(value)), _value(value.name, _seen), _value(value.value, _seen)] if isinstance(value, (staticmethod, classmethod)): # a class's members: hash the function return _value(value.__func__, _seen) if isinstance(value, property): @@ -121,7 +127,7 @@ def _value(value: Any, _seen: frozenset[int] = frozenset()) -> Any: for k, v in vars(base).items(): if k not in _CLASS_NOISE: attrs.setdefault(k, v) # the most derived definition wins - bases = [f"{b.__module__}.{b.__qualname__}" for b in value.__mro__ if b.__module__ != "builtins"] + bases = [f"{b.__module__}.{b.__qualname__}" for b in value.__mro__ if b is not object] src = repr(_value(dict(sorted(attrs.items())), seen)) + repr(bases) digest = hashlib.sha256(src.encode("utf-8")).hexdigest()[:12] return f"{value.__module__}.{value.__qualname__}#{digest}" @@ -274,9 +280,13 @@ def main(argv: list[str] | None = None) -> int: else: sys.stdout.write(text) return 0 - base = json.loads(args.base.read_text(encoding="utf-8")) - head = json.loads(args.head.read_text(encoding="utf-8")) - lines = diff_surfaces(base, head) + try: + base = json.loads(args.base.read_text(encoding="utf-8")) + head = json.loads(args.head.read_text(encoding="utf-8")) + lines = diff_surfaces(base, head) + except (OSError, ValueError, KeyError) as exc: # an uncaught exception would exit 1: "surfaces differ" + print(f"cli_surface: cannot diff: {type(exc).__name__}: {exc}", file=sys.stderr) + return 2 sys.stdout.write(render(lines, markdown=args.markdown)) return 1 if lines else 0 diff --git a/dokimi/tests/test_cli_surface.py b/dokimi/tests/test_cli_surface.py index 47503013..1d722a3c 100644 --- a/dokimi/tests/test_cli_surface.py +++ b/dokimi/tests/test_cli_surface.py @@ -341,7 +341,8 @@ def build_parser(): """ -def _run_diff_step(tmp_path: Path, *, pr_adds: bool, pr_moves_build_parser: bool = False) -> str: +def _run_diff_step(tmp_path: Path, *, pr_adds: bool, pr_moves_build_parser: bool = False, + truncate_dump: bool = False) -> str: """Run the workflow's dump-and-diff step on a merge commit whose base branch moved on. With `pr_moves_build_parser` the PR deletes dokimi/cli.py, so the head dump raises. @@ -396,7 +397,12 @@ def commit(extra: str, msg: str, *, cli: bool = True) -> str: env.update(BASE_REF="main", RUNNER_TEMP=str(tmp_path), GITHUB_OUTPUT=str(output), GITHUB_STEP_SUMMARY=str(tmp_path / "summary"), PATH=os.path.dirname(sys.executable) + os.pathsep + env.get("PATH", "")) - subprocess.run(["bash", "-e", "-c", step["run"]], cwd=repo / "dokimi", env=env, check=True, + script = step["run"] + if truncate_dump: # both dumps succeed, then one is cut short: only the diff raises + marker = "rc=0\n" + assert marker in script + script = script.replace(marker, marker + "printf '{' > \"$out/head.json\"\n", 1) + subprocess.run(["bash", "-e", "-c", script], cwd=repo / "dokimi", env=env, check=True, capture_output=True, text=True) return output.read_text(encoding="utf-8") @@ -419,6 +425,23 @@ def test_workflow_reports_error_and_passes_when_the_head_cannot_be_dumped(tmp_pa assert "## CLI interface check could not run" in diff_md.read_text(encoding="utf-8") +def test_workflow_reports_error_and_passes_when_the_diff_crashes(tmp_path): + # an uncaught exception in `diff` must not read as "CLI changed" (changed=1) + output = _run_diff_step(tmp_path, pr_adds=False, truncate_dump=True) + assert "changed=error" in output + assert "changed=0" not in output and "changed=1" not in output + + +def test_main_diff_of_an_unreadable_dump_exits_2(tmp_path, capsys): + bad, ok = tmp_path / "bad.json", tmp_path / "ok.json" + bad.write_text("{", encoding="utf-8") + ok.write_text(json.dumps(_surface()), encoding="utf-8") + assert cs.main(["diff", str(bad), str(bad), "--markdown"]) == 2 + captured = capsys.readouterr() + assert captured.out == "" and "cannot diff" in captured.err + assert cs.main(["diff", str(ok), str(tmp_path / "missing.json")]) == 2 + + _VALIDATOR = '''def positive_int(text): """Reject non-positive integers.""" value = int(text) @@ -522,3 +545,57 @@ def test_a_class_docstring_is_not_a_change(tmp_path): reworded = _file_validator(tmp_path, "doc-new", "import os\n\nclass Check:\n '''Reworded.'''\n" " def run(self, s):\n return s\n", attr="Check") assert cs.diff_surfaces(_one(type=base), _one(type=reworded)) == [] + + +def test_an_enum_member_without_a_stable_repr_is_dumped_the_same_every_time(tmp_path): + # two loads of the same source: the members' objects live at different addresses + src = "import enum\n\nclass Obj(enum.Enum):\n a = object()\n" + one = _one(type=_file_validator(tmp_path, "enum-1", src, attr="Obj")) + two = _one(type=_file_validator(tmp_path, "enum-2", src, attr="Obj")) + assert cs.diff_surfaces(one, two) == [] + + +def test_a_subclass_override_wins_over_the_base_definition(tmp_path): + def loaded(name: str, body: str): + source = (f"class Base:\n def check(self, s):\n return s\n\n" + f"class Check(Base):\n def check(self, s):\n return {body}\n") + return _one(type=_file_validator(tmp_path, name, source, attr="Check")) + assert cs.diff_surfaces(loaded("strict", "s"), loaded("loose", "True")) + + +def test_classes_with_the_same_members_and_different_bases_differ(tmp_path): + def loaded(name: str, base: str): + source = f"class A:\n pass\n\nclass B:\n pass\n\nclass Check({base}):\n pass\n" + return _one(type=_file_validator(tmp_path, name, source, attr="Check")) + assert cs.diff_surfaces(loaded("a", "A"), loaded("b", "B")) + + +def test_a_builtin_base_of_a_class_type_is_a_change(tmp_path): + def loaded(name: str, base: str): + return _one(type=_file_validator(tmp_path, name, f"class Check({base}):\n pass\n", attr="Check")) + assert cs.diff_surfaces(loaded("i", "int"), loaded("s", "str")) + + +def test_a_property_setter_only_change_is_reported(tmp_path): + def loaded(name: str, body: str): + source = ("class Check:\n @property\n def v(self):\n return 1\n\n" + f" @v.setter\n def v(self, x):\n {body}\n") + return _one(type=_file_validator(tmp_path, name, source, attr="Check")) + assert cs.diff_surfaces(loaded("a", "pass"), loaded("b", "self.x = x")) + + +def test_a_set_default_is_dumped_in_the_same_order_under_any_hash_seed(): + import os + import subprocess + import sys + + code = ("import importlib.util, json, sys\n" + "spec = importlib.util.spec_from_file_location('cs', sys.argv[1])\n" + "m = importlib.util.module_from_spec(spec); spec.loader.exec_module(m)\n" + "print(json.dumps(m._value(frozenset({'alpha', 'beta', 'gamma'}))))\n") + outs = { + subprocess.run([sys.executable, "-c", code, str(_SCRIPT)], capture_output=True, text=True, check=True, + env={**os.environ, "PYTHONHASHSEED": seed}).stdout + for seed in ("1", "2", "3", "4") + } + assert len(outs) == 1, outs From e4ef0b0d7fcf2668b479bf418ad49f7a0e562437 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Wed, 21 Oct 2026 12:00:27 -0700 Subject: [PATCH 21/21] ci(cli-interface): upload the diff result as an artifact for a workflow_run poster Signed-off-by: Nikolay Petrov --- .github/workflows/cli-interface.yml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/.github/workflows/cli-interface.yml b/.github/workflows/cli-interface.yml index 2ab74dbe..72bc4f68 100644 --- a/.github/workflows/cli-interface.yml +++ b/.github/workflows/cli-interface.yml @@ -83,6 +83,24 @@ jobs: echo "changed=$rc" >> "$GITHUB_OUTPUT" echo "file=$out/diff.md" >> "$GITHUB_OUTPUT" cat "$out/diff.md" >> "$GITHUB_STEP_SUMMARY" + # The result for the cli-interface-comment workflow (workflow_run, base-repo write + # token), which labels and comments for fork PRs this job's read-only token cannot. + # Plain data only: the poster treats it as untrusted and re-validates every field. + - name: Package the result + env: + CHANGED: ${{ steps.diff.outputs.changed }} + DIFF_FILE: ${{ steps.diff.outputs.file }} + PR_NUMBER: ${{ github.event.pull_request.number }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + mkdir -p "$RUNNER_TEMP/cli-interface-result" + python -c 'import json, os; json.dump({"pr_number": int(os.environ["PR_NUMBER"]), "head_sha": os.environ["HEAD_SHA"], "changed": os.environ["CHANGED"], "diff": open(os.environ["DIFF_FILE"], encoding="utf-8").read()}, open(os.environ["RUNNER_TEMP"] + "/cli-interface-result/result.json", "w", encoding="utf-8"))' + - uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 + with: + name: cli-interface-result + path: ${{ runner.temp }}/cli-interface-result/result.json + retention-days: 1 + if-no-files-found: error # A fork PR's token is read-only: the diff is then in the job summary only. - name: Label and comment on the PR uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0