diff --git a/.github/workflows/cli-interface.yml b/.github/workflows/cli-interface.yml new file mode 100644 index 00000000..72bc4f68 --- /dev/null +++ b/.github/workflows/cli-interface.yml @@ -0,0 +1,157 @@ +# 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 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. +# 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: + +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 # 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: + 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 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 + id: diff + env: + BASE_REF: ${{ github.event.pull_request.base.ref }} + run: | + out="$RUNNER_TEMP/cli-surface" + 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 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. + report_error() { + printf '%s\n' "## CLI interface check could not run" "" \ + "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" + 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 + 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.user?.login === 'github-actions[bot]' && + c.body && c.body.includes(marker)); + 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._'; + 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..763871a9 --- /dev/null +++ b/dokimi/scripts/cli_surface.py @@ -0,0 +1,295 @@ +#!/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. 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 +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. 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, 2 when a dump cannot be read. +""" + +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 + +# The attributes of an option or argument compared between base and head. +FIELDS = ("required", "is_flag", "nargs", "default", "type", "choices", "hidden", "action", + "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__", + "_value2member_map_"}) # the last is keyed by str(value), which may hold an address + + +def _filled(cell: Any) -> bool: + try: + _ = cell.cell_contents + except ValueError: + return False + 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, _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, _seen) for v in value] + if isinstance(value, dict): + 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, _seen)) for v in value) + if isinstance(value, enum.Enum): + 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): + 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) + # 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, 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, 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: 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 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}" + 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 + 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, + 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 + "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), + "const": _value(action.const), + "type": _value(action.type), + "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, + "group_required": group_required, + } + + +def _command(parser: argparse.ArgumentParser, path: str) -> dict[str, Any]: + node: dict[str, Any] = {"path": path, "params": [], "subcommands": {}, + "subcommand_required": None, "allow_abbrev": parser.allow_abbrev} + position = 0 + + # Build a map from action to its mutually exclusive group (if any). + 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: + 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): + 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}") + else: + # -h/--help and --version are kept (help text and the version string are not dumped) + group_info = action_to_group.get(action) + 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, + param_group, param_required)) + position += 0 if action.option_strings else 1 + 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: 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}") + 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:") + 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 + 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 + + +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..1d722a3c --- /dev/null +++ b/dokimi/tests/test_cli_surface.py @@ -0,0 +1,601 @@ +# 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 enum +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,) = [p for p in flat["dokimi repo scan"]["params"] if p["key"] == "--hidden"] + 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" + + +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_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") + 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 + + +@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))) + + +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"]} + # 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_key + 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) + 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 + + +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) + 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": _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)) + + +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_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()) + 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"})) == [] + + +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_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 + + +_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, 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. + """ + 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, *, 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", + 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 + 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", "")) + 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") + + +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) + + +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 + # 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") + + +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) + if value < 1: # zero is not positive + raise ValueError(text) + return value +''' + + +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") + 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 getattr(module, attr) + + +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)) + + +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 + + +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"] + + +_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)) == [] + + +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