From ab75e3572cc5ffab21c87d15aefbac5ad4ba7102 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Tue, 22 Sep 2026 14:29:13 +0200 Subject: [PATCH 1/5] feat(review): skip the engine when no analysed file changed, and end the comment with a machine-readable line A pull request whose changed files are all outside the analysed scope (docs, configuration, CI, tests the ignore file excludes, a language the engine does not read) cannot have moved the architecture, yet it cost a full run and its comment's "0 changed components" was whatever the diff of two graphs happened to say. A new step lists the pull request's files (the checkout is the head alone, so from the API) and counts them against the engine's own rules, imported from the installed engine: its extension map and its ignore manager over the checked-out repository. When none is analysed and a base graph is at hand, the analysis step is skipped, the base is published as the head, the diff renders empty, and the comment says "0 changed components (no analysed file changed)". A file list that cannot be read, an engine that cannot be imported or a missing base all mean "run as usual". Every review comment now ends with an HTML comment carrying the platform URL, the changed-component count, the file counts and the head sha, so the web platform's dashboard can stop parsing the prose and the mermaid block. The artifact's metadata.json carries scope_skipped, changed_files and analysed_files, as strings like every other field there. Co-Authored-By: Claude Fable 5.1 --- README.md | 6 + action.yml | 41 +++++- scripts/action/build-review-artifact.sh | 6 +- scripts/action/build-review-comment.sh | 17 ++- scripts/action/scope-check.sh | 67 ++++++++++ scripts/action/scope_check.py | 58 ++++++++ tests/test_review_comment.py | 72 ++++++++++ tests/test_scope_check.py | 168 ++++++++++++++++++++++++ 8 files changed, 425 insertions(+), 10 deletions(-) create mode 100755 scripts/action/scope-check.sh create mode 100644 scripts/action/scope_check.py create mode 100644 tests/test_review_comment.py create mode 100644 tests/test_scope_check.py diff --git a/README.md b/README.md index 7c53bf6..da91bc8 100644 --- a/README.md +++ b/README.md @@ -98,6 +98,12 @@ Because artifacts are readable and writable from every trigger, all paths behave Fork pull requests never carry an analysis forward. They are reviewed on request, each review starts from the base, and nothing they produce is read by a run on this repository's own code: untrusted code must not shape state that a later run loads. The action also accepts `pull_request_target`, which runs on the base branch ref and lets both share one chain; that trigger has its own trade-offs (a PR that adds this workflow will not run it until merged, and the fork gate becomes load-bearing), so `pull_request` remains the recommended default. +### Pull requests that change nothing analysed + +A pull request whose changed files are all outside the analysed scope (docs, configuration, CI, tests the ignore file excludes, or a language the engine does not read) cannot have moved the architecture. Before analysing, a review run lists the pull request's files and counts them against the engine's own rules: its language extensions and `.codeboarding/.codeboardingignore` over the checked-out repository. When none is analysed and a base graph is at hand (a published base, or the baseline the sync workflow committed), the engine is not run: the base is published as the head, the diff renders as empty, and the comment says `0 changed components (no analysed file changed)`, so the zero is known to be decided from the files rather than measured by a diff. Nothing is skipped when the file list cannot be read, when the engine cannot be imported, or when no base exists. + +Every review comment ends with a machine-readable HTML comment, ``, and the review artifact's `metadata.json` carries `scope_skipped`, `changed_files` and `analysed_files` (strings, like every other field there). + ## Authentication and providers The `llm` input is required and says where analysis credentials come from. There are diff --git a/action.yml b/action.yml index b6c5f17..0084603 100644 --- a/action.yml +++ b/action.yml @@ -507,9 +507,29 @@ runs: PR_URL: ${{ steps.sync_commit.outputs.sync_pr_url }} run: "$GITHUB_ACTION_PATH/scripts/action/sync-summary.sh" + # A pull request that changes nothing the engine would analyse (docs, config, CI, tests + # it ignores, a language it does not read) cannot have moved the architecture, so its + # head analysis IS the base analysis. When a base graph is at hand, the run below is + # skipped and the base is published as the head; the comment says "0 changed components + # (no analysed file changed)" so the zero is known to be decided, not measured. + - name: Check whether any analysed file changed + id: scope + if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' + shell: bash + env: + ACTION_PATH: ${{ github.action_path }} + GH_TOKEN: ${{ inputs.github_token }} + GH_ENTERPRISE_TOKEN: ${{ inputs.github_token }} + GH_HOST: ${{ github.server_url }} + REPOSITORY: ${{ github.repository }} + PR_NUMBER: ${{ steps.guard.outputs.pr_number }} + CHECKOUT_DIR: ${{ github.workspace }}/.codeboarding-target + BASE_DIR: ${{ runner.temp }}/cb-state/${{ github.action }}/base + run: "$GITHUB_ACTION_PATH/scripts/action/scope-check.sh" + - name: Analyze pull request id: review_analyze - if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' + if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.scope.outputs.skip != 'true' shell: bash env: ACTION_PATH: ${{ github.action_path }} @@ -537,7 +557,7 @@ runs: # so a later rendering or posting failure must not throw it away. Only the # next run reads this, hence the short retention. - name: Publish this analysis for the next run - if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.state.outputs.warmstart_name != '' && github.server_url == 'https://github.com' + if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.scope.outputs.skip != 'true' && steps.state.outputs.warmstart_name != '' && github.server_url == 'https://github.com' continue-on-error: true uses: actions/upload-artifact@v4 with: @@ -572,8 +592,8 @@ runs: shell: bash env: ACTION_PATH: ${{ github.action_path }} - BASE_ANALYSIS_PATH: ${{ steps.review_analyze.outputs.base_analysis_path }} - HEAD_ANALYSIS_PATH: ${{ steps.review_analyze.outputs.analysis_path }} + BASE_ANALYSIS_PATH: ${{ steps.review_analyze.outputs.base_analysis_path || steps.scope.outputs.base_analysis_path }} + HEAD_ANALYSIS_PATH: ${{ steps.review_analyze.outputs.analysis_path || steps.scope.outputs.analysis_path }} run: "$GITHUB_ACTION_PATH/scripts/action/render-review.sh" - name: Build review artifact @@ -581,12 +601,15 @@ runs: if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' shell: bash env: - ANALYSIS_PATH: ${{ steps.review_analyze.outputs.analysis_path }} + ANALYSIS_PATH: ${{ steps.review_analyze.outputs.analysis_path || steps.scope.outputs.analysis_path }} BASE_ARTIFACT_NAME: ${{ steps.state.outputs.base_name }} BASE_ARTIFACT_ID: ${{ steps.publish_base.outputs.artifact-id || steps.fetch_base.outputs.artifact_id }} - BASE_ANALYSIS_PATH: ${{ steps.review_analyze.outputs.base_analysis_path }} + BASE_ANALYSIS_PATH: ${{ steps.review_analyze.outputs.base_analysis_path || steps.scope.outputs.base_analysis_path }} INLINE_BASE: ${{ steps.publish_base.outputs.artifact-id == '' && steps.fetch_base.outputs.artifact_id == '' }} - ANALYSIS_MODE: ${{ steps.review_analyze.outputs.analysis_mode }} + ANALYSIS_MODE: ${{ steps.review_analyze.outputs.analysis_mode || steps.scope.outputs.analysis_mode }} + SCOPE_SKIPPED: ${{ steps.scope.outputs.skip }} + CHANGED_FILES: ${{ steps.scope.outputs.changed_files }} + ANALYSED_FILES: ${{ steps.scope.outputs.analysed_files }} BASE_SHA: ${{ steps.guard.outputs.base_sha }} MERGE_BASE_SHA: ${{ steps.guard.outputs.merge_base_sha }} MERGE_BASE_RESOLVED: ${{ steps.guard.outputs.merge_base_resolved }} @@ -622,6 +645,10 @@ runs: BEHIND_BY: ${{ steps.guard.outputs.behind_by }} BASE_REF: ${{ steps.guard.outputs.base_ref }} MERGE_BASE_RESOLVED: ${{ steps.guard.outputs.merge_base_resolved }} + HEAD_SHA: ${{ steps.guard.outputs.head_sha }} + SCOPE_SKIPPED: ${{ steps.scope.outputs.skip }} + CHANGED_FILES: ${{ steps.scope.outputs.changed_files }} + ANALYSED_FILES: ${{ steps.scope.outputs.analysed_files }} run: "$GITHUB_ACTION_PATH/scripts/action/build-review-comment.sh" - name: Post review comment diff --git a/scripts/action/build-review-artifact.sh b/scripts/action/build-review-artifact.sh index 0dc5e06..67a587f 100755 --- a/scripts/action/build-review-artifact.sh +++ b/scripts/action/build-review-artifact.sh @@ -36,9 +36,13 @@ jq -n \ --arg chain_depth "$CHAIN_DEPTH" \ --arg base_artifact "$BASE_ARTIFACT_NAME" \ --arg base_artifact_id "$BASE_ARTIFACT_ID" \ + --arg scope_skipped "${SCOPE_SKIPPED:-false}" \ + --arg changed_files "${CHANGED_FILES:-}" \ + --arg analysed_files "${ANALYSED_FILES:-}" \ '{kind: $kind, mode: $mode, base_sha: $base_sha, merge_base_sha: $merge_base_sha, pr_base_sha: $merge_base_sha, merge_base_resolved: $merge_base_resolved, head_sha: $head_sha, pr_number: $pr_number, seed_source: $seed_source, chain_depth: $chain_depth, - base_artifact: $base_artifact, base_artifact_id: $base_artifact_id}' \ + base_artifact: $base_artifact, base_artifact_id: $base_artifact_id, + scope_skipped: $scope_skipped, changed_files: $changed_files, analysed_files: $analysed_files}' \ > "${RUNNER_TEMP}/cb-review-artifact/metadata.json" echo "artifact_dir=${RUNNER_TEMP}/cb-review-artifact" >> "$GITHUB_OUTPUT" diff --git a/scripts/action/build-review-comment.sh b/scripts/action/build-review-comment.sh index 14d7ba2..61ce58b 100755 --- a/scripts/action/build-review-comment.sh +++ b/scripts/action/build-review-comment.sh @@ -14,9 +14,17 @@ RUN_URL="${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/actions/runs/${GITHUB_RUN_ID} # came from here is a github.com referrer, which most clients strip and a link # pasted into chat never had. Constant across runs on purpose: a run id here # would scatter one pull request's clicks across a new value per re-run. -WEBVIEW_URL="https://app.codeboarding.org/${GITHUB_REPOSITORY}/pull/${PR_NUMBER}?utm_source=github&utm_medium=pr_comment&utm_campaign=gh_action" +PLATFORM_URL="https://app.codeboarding.org/${GITHUB_REPOSITORY}/pull/${PR_NUMBER}" +WEBVIEW_URL="${PLATFORM_URL}?utm_source=github&utm_medium=pr_comment&utm_campaign=gh_action" BODY="${RUNNER_TEMP}/review-comment.md" -printf '### CodeBoarding review\n\n**Status:** %s changed %s\n' "$N_CHANGED" "$COMPONENT_NOUN" > "$BODY" +# The status line is what the web platform reads the count from, so its shape is a contract. +# A run the scope check skipped says so in the same line: its zero is decided from the +# changed files, not from a diff of two graphs, and a reader deserves to know which. +STATUS="${N_CHANGED} changed ${COMPONENT_NOUN}" +if [ "${SCOPE_SKIPPED:-false}" = true ]; then + STATUS="${STATUS} (no analysed file changed)" +fi +printf '### CodeBoarding review\n\n**Status:** %s\n' "$STATUS" > "$BODY" printf '\nSee the full change in [CodeBoarding](%s).\n' "$WEBVIEW_URL" >> "$BODY" # The diagram compares against the merge base, so commits landed on the base # branch since this PR forked are excluded. Say so rather than hide it. @@ -40,5 +48,10 @@ fi printf '[download artifacts](%s) · ' "$ARTIFACT_URL" fi printf 'run [%s](%s)\n' "$GITHUB_RUN_ID" "$RUN_URL" + # The machine-readable line: what a reader of the comment (the web platform's dashboard, an + # agent) needs without parsing the prose or the diagram. An HTML comment renders as nothing. + # Keep it one line, `key=value` pairs, values without spaces, so a regex over it stays trivial. + printf '\n' \ + "$PLATFORM_URL" "$N_CHANGED" "${CHANGED_FILES:-}" "${ANALYSED_FILES:-}" "${HEAD_SHA:-}" } >> "$BODY" echo "path=$BODY" >> "$GITHUB_OUTPUT" diff --git a/scripts/action/scope-check.sh b/scripts/action/scope-check.sh new file mode 100755 index 0000000..05ada34 --- /dev/null +++ b/scripts/action/scope-check.sh @@ -0,0 +1,67 @@ +#!/usr/bin/env bash +# Decides whether a review run can skip the engine: when none of the pull request's changed +# files is one the engine would analyse (docs, config, CI, tests it ignores, a language it +# does not read), the architecture cannot have moved, and the base analysis IS the head +# analysis. The run then publishes the base as the head, renders an empty diff, and says so. +# +# Outputs: changed_files, analysed_files, skip (true only when the shortcut applies), and on +# skip the analysis_path / base_analysis_path / analysis_mode the later steps read in place of +# the analysis step's. Never fails the job: a read that did not answer means "run as usual". +set -euo pipefail + +changed=0 +analysed=0 +skip=false + +emit() { + printf 'changed_files=%s\nanalysed_files=%s\nskip=%s\n' "$changed" "$analysed" "$skip" >> "$GITHUB_OUTPUT" +} + +# The checkout is the head commit alone, so the changed files come from the API, not git. +files="$(gh api "repos/${REPOSITORY}/pulls/${PR_NUMBER}/files" --paginate --jq '.[].filename' 2>/dev/null)" || { + echo "::notice::Could not list the pull request's files; analysing as usual." + emit; exit 0 +} +if [ -z "$files" ]; then + echo "::notice::The pull request changes no files; analysing as usual." + emit; exit 0 +fi + +# From the action's own directory, so the analysed repository's modules cannot shadow the +# engine's on sys.path (see the install step in action.yml). +counts="$(cd "$ACTION_PATH" && printf '%s\n' "$files" | python3 scripts/action/scope_check.py --repo-root "$CHECKOUT_DIR")" || { + echo "::notice::Could not decide the analysed scope; analysing as usual." + emit; exit 0 +} +changed="$(printf '%s' "$counts" | jq -r '.changed')" +analysed="$(printf '%s' "$counts" | jq -r '.analysed')" +if [ "$changed" -eq 0 ] || [ "$analysed" -gt 0 ]; then + emit; exit 0 +fi + +# Nothing analysed changed. The base graph must already exist for the shortcut: a published +# base from the fetch step, or the baseline the sync workflow committed. Computing one would +# be the run this exists to skip. +base="" +if [ -f "${BASE_DIR:-}/analysis.json" ]; then + base="${BASE_DIR}/analysis.json" +elif [ -f "${CHECKOUT_DIR}/.codeboarding/analysis.json" ]; then + base="${CHECKOUT_DIR}/.codeboarding/analysis.json" +fi +if [ -z "$base" ]; then + echo "::notice::No analysed file changed, but no base analysis is at hand; analysing as usual." + emit; exit 0 +fi + +work="${RUNNER_TEMP}/codeboarding-scope" +mkdir -p "$work" +cp "$base" "$work/analysis.json" +# The base's health report, when it sits beside the base, describes the head too. +if [ -f "$(dirname "$base")/health/health_report.json" ]; then + mkdir -p "$work/health" + cp "$(dirname "$base")/health/health_report.json" "$work/health/health_report.json" +fi +skip=true +echo "::notice::No analysed file changed (${changed} files, all outside the analysed scope); reusing the base analysis as the head." +emit +printf 'analysis_path=%s\nbase_analysis_path=%s\nanalysis_mode=unchanged\n' "$work/analysis.json" "$base" >> "$GITHUB_OUTPUT" diff --git a/scripts/action/scope_check.py b/scripts/action/scope_check.py new file mode 100644 index 0000000..3d84a82 --- /dev/null +++ b/scripts/action/scope_check.py @@ -0,0 +1,58 @@ +"""Count how many of a pull request's changed files the engine would analyse. + +Reads one path per line on stdin and prints a JSON object: ``changed`` (paths read) and +``analysed`` (paths in the analysed scope). A path is analysed when its extension belongs to a +language the engine reads and the engine's own ignore rules (``.codeboarding/.codeboardingignore`` +over the repository root, plus the directories it always drops) let it through. Both come from +the installed engine, so this says exactly what the engine would look at, no more and no less. + +When the engine cannot be imported every path counts as analysed: the answer this feeds is +"skip the run", and a doubt must never skip one. +""" + +from __future__ import annotations + +import argparse +import json +import sys +from pathlib import Path + + +def _engine_scope(repo_root: Path): + """The engine's extension map and ignore manager, or None when it is not importable.""" + try: + from repo_utils.ignore import RepoIgnoreManager # type: ignore[import-not-found] + from static_analyzer.config import SOURCE_EXTENSION_TO_LANGUAGE # type: ignore[import-not-found] + except Exception: # noqa: BLE001 - any import failure means "do not decide" + return None + extensions = {str(ext).lower() for ext in SOURCE_EXTENSION_TO_LANGUAGE} + return extensions, RepoIgnoreManager(repo_root) + + +def count_analysed(paths: list[str], repo_root: Path) -> dict[str, int]: + scope = _engine_scope(repo_root) + if scope is None: + return {"changed": len(paths), "analysed": len(paths)} + extensions, ignores = scope + analysed = 0 + for raw in paths: + path = Path(raw) + if path.suffix.lower() not in extensions: + continue + if ignores.should_ignore(path): + continue + analysed += 1 + return {"changed": len(paths), "analysed": analysed} + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--repo-root", required=True, help="The checked-out repository, where the ignore file lives.") + args = parser.parse_args() + paths = [line.strip() for line in sys.stdin.read().splitlines() if line.strip()] + print(json.dumps(count_analysed(paths, Path(args.repo_root)))) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_review_comment.py b/tests/test_review_comment.py new file mode 100644 index 0000000..e5d8b7b --- /dev/null +++ b/tests/test_review_comment.py @@ -0,0 +1,72 @@ +"""The review comment's shape: the status line the web platform reads, and the machine-readable line.""" + +import os +import re +import subprocess +import tempfile +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +BUILD_COMMENT = ROOT / "scripts" / "action" / "build-review-comment.sh" + + +def _build(root: Path, **extra: str) -> str: + diagram = root / "diagram.md" + diagram.write_text("```mermaid\ngraph LR\n```\n", encoding="utf-8") + output = root / "github-output" + output.write_text("", encoding="utf-8") + env = { + "PATH": os.environ["PATH"], + "GITHUB_OUTPUT": str(output), + "RUNNER_TEMP": str(root), + "GITHUB_SERVER_URL": "https://github.com", + "GITHUB_REPOSITORY": "owner/repo", + "GITHUB_RUN_ID": "1234", + "DIAGRAM": str(diagram), + "N_CHANGED": "3", + "ARTIFACT_URL": "", + "PR_NUMBER": "605", + "HEAD_SHA": "abc123", + **extra, + } + subprocess.run([str(BUILD_COMMENT)], env=env, capture_output=True, text=True, check=True) + outputs: dict[str, str] = {} + for line in output.read_text(encoding="utf-8").splitlines(): + key, _, value = line.partition("=") + outputs[key] = value + return Path(outputs["path"]).read_text(encoding="utf-8") + + +class ReviewCommentTests(unittest.TestCase): + def test_status_line_and_platform_link_as_the_web_platform_reads_them(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + body = _build(Path(tmp)) + self.assertTrue(body.startswith("### CodeBoarding review\n\n**Status:** 3 changed components\n")) + self.assertIn( + "See the full change in [CodeBoarding](https://app.codeboarding.org/owner/repo/pull/605?utm_source=github", + body, + ) + + def test_the_machine_readable_line_ends_the_body(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + body = _build(Path(tmp), CHANGED_FILES="4", ANALYSED_FILES="2") + last = body.rstrip("\n").splitlines()[-1] + self.assertEqual( + last, + "", + ) + # The status regex the web platform uses must still find the status line, not the marker. + match = re.search(r"\*\*Status:\*\*\s*(\d+)\s+changed\s+components?", body) + self.assertIsNotNone(match) + self.assertEqual(match.group(1) if match else None, "3") + + def test_a_skipped_run_says_its_zero_was_decided_from_the_files(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + body = _build(Path(tmp), N_CHANGED="0", SCOPE_SKIPPED="true", CHANGED_FILES="2", ANALYSED_FILES="0") + self.assertIn("**Status:** 0 changed components (no analysed file changed)\n", body) + self.assertIn("changed=0 changed_files=2 analysed_files=0", body) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_scope_check.py b/tests/test_scope_check.py new file mode 100644 index 0000000..479ec59 --- /dev/null +++ b/tests/test_scope_check.py @@ -0,0 +1,168 @@ +"""The scope check: which changed files the engine would analyse, and the shortcut it allows.""" + +import json +import os +import subprocess +import tempfile +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +SCOPE_CHECK_PY = ROOT / "scripts" / "action" / "scope_check.py" +SCOPE_CHECK_SH = ROOT / "scripts" / "action" / "scope-check.sh" + +# A stand-in for the installed engine: the extension map and the ignore manager with the +# rules the real ones apply (extension by suffix, hidden and always-excluded directories, then +# the ignore file's patterns), small enough to read in one go. +FAKE_ENGINE = { + "static_analyzer/__init__.py": "", + "static_analyzer/config.py": ( + "SOURCE_EXTENSION_TO_LANGUAGE = {ext: 'x' for ext in ('.py', '.ts', '.tsx', '.java', '.go', '.php', '.rs', '.cs', '.cpp')}\n" + ), + "repo_utils/__init__.py": "", + "repo_utils/ignore.py": ( + "from pathlib import Path\n" + "import fnmatch\n" + "ALWAYS = {'.git', '.codeboarding', 'node_modules', '__pycache__', 'build', 'dist', 'coverage', 'target'}\n" + "class RepoIgnoreManager:\n" + " def __init__(self, repo_root):\n" + " self.repo_root = Path(repo_root)\n" + " f = self.repo_root / '.codeboarding' / '.codeboardingignore'\n" + " self.patterns = [l.strip() for l in f.read_text().splitlines() if l.strip() and not l.startswith('#')] if f.exists() else []\n" + " def should_ignore(self, path):\n" + " rel = Path(path)\n" + " if rel.is_absolute(): rel = rel.relative_to(self.repo_root)\n" + " if any(p in ALWAYS or p.startswith('.') for p in rel.parts[:-1]): return True\n" + " return any(fnmatch.fnmatch(str(rel), pat) or fnmatch.fnmatch(str(rel), pat.rstrip('/') + '/*') for pat in self.patterns)\n" + ), +} + + +def _plant(root: Path, files: dict) -> None: + for name, text in files.items(): + target = root / name + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(text, encoding="utf-8") + + +def _run_py(paths: list, repo_root: Path, engine: Path) -> dict: + result = subprocess.run( + ["python3", str(SCOPE_CHECK_PY), "--repo-root", str(repo_root)], + input="\n".join(paths) + "\n", + env={"PATH": os.environ["PATH"], "PYTHONPATH": str(engine)}, + capture_output=True, + text=True, + check=True, + ) + return json.loads(result.stdout) + + +class ScopeCheckPyTests(unittest.TestCase): + def test_counts_only_files_the_engine_would_analyse(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + engine = root / "engine" + _plant(engine, FAKE_ENGINE) + repo = root / "repo" + _plant(repo, {".codeboarding/.codeboardingignore": "# ignore\n**/tests/**\nvendor/\n"}) + counts = _run_py( + [ + "README.md", + "docs/development/architecture.md", + ".github/workflows/ci.yml", + "src/lib/turn.ts", + "src/lib/tests/turn_test.ts", + "node_modules/x/index.js", + "vendor/lib.py", + "gradle/libs.versions.toml", + ], + repo, + engine, + ) + self.assertEqual(counts, {"changed": 8, "analysed": 1}) + + def test_without_the_engine_every_file_counts_as_analysed(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + counts = _run_py(["README.md", "docs/a.md"], root / "repo", root / "no-engine") + self.assertEqual(counts, {"changed": 2, "analysed": 2}) + + +class ScopeCheckShTests(unittest.TestCase): + def _run(self, root: Path, files: list, base: Path | None) -> dict: + fake_bin = root / "bin" + fake_bin.mkdir(exist_ok=True) + gh = fake_bin / "gh" + gh.write_text("#!/usr/bin/env bash\n" + "".join(f"printf '%s\\n' '{f}'\n" for f in files), encoding="utf-8") + gh.chmod(0o755) + engine = root / "engine" + _plant(engine, FAKE_ENGINE) + checkout = root / "checkout" + checkout.mkdir(exist_ok=True) + output = root / "github-output" + output.write_text("", encoding="utf-8") + temp = root / "runner-temp" + temp.mkdir(exist_ok=True) + env = { + "PATH": f"{fake_bin}:{os.environ['PATH']}", + "PYTHONPATH": str(engine), + "ACTION_PATH": str(ROOT), + "REPOSITORY": "owner/repo", + "PR_NUMBER": "7", + "CHECKOUT_DIR": str(checkout), + "BASE_DIR": str(base) if base else str(root / "no-base"), + "GITHUB_OUTPUT": str(output), + "RUNNER_TEMP": str(temp), + } + result = subprocess.run([str(SCOPE_CHECK_SH)], env=env, capture_output=True, text=True, check=False) + self.assertEqual(result.returncode, 0, result.stderr) + out = {} + for line in output.read_text(encoding="utf-8").splitlines(): + key, _, value = line.partition("=") + out[key] = value + return out + + def test_skips_when_nothing_analysed_changed_and_a_base_is_at_hand(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + base = root / "base" + base.mkdir() + (base / "analysis.json").write_text('{"description": "base"}', encoding="utf-8") + out = self._run(root, ["README.md", ".github/workflows/ci.yml"], base) + self.assertEqual(out["skip"], "true") + self.assertEqual(out["changed_files"], "2") + self.assertEqual(out["analysed_files"], "0") + self.assertEqual(out["analysis_mode"], "unchanged") + self.assertEqual(out["base_analysis_path"], str(base / "analysis.json")) + self.assertEqual(Path(out["analysis_path"]).read_text(encoding="utf-8"), '{"description": "base"}') + + def test_runs_as_usual_when_an_analysed_file_changed(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + base = root / "base" + base.mkdir() + (base / "analysis.json").write_text("{}", encoding="utf-8") + out = self._run(root, ["README.md", "src/a.py"], base) + self.assertEqual(out["skip"], "false") + self.assertEqual(out["analysed_files"], "1") + self.assertNotIn("analysis_path", out) + + def test_runs_as_usual_without_a_base_to_reuse(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + out = self._run(root, ["README.md"], None) + self.assertEqual(out["skip"], "false") + self.assertEqual(out["analysed_files"], "0") + + def test_reuses_the_committed_baseline_when_no_base_was_published(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + checkout = root / "checkout" + _plant(checkout, {".codeboarding/analysis.json": '{"description": "committed"}'}) + out = self._run(root, ["docs/a.md"], None) + self.assertEqual(out["skip"], "true") + self.assertEqual(out["base_analysis_path"], str(checkout / ".codeboarding" / "analysis.json")) + + +if __name__ == "__main__": + unittest.main() From 16427fd0192105b7fab72216729b4255cf819803 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Tue, 22 Sep 2026 17:57:41 +0200 Subject: [PATCH 2/5] refactor(review): relay the engine's early-exit verdict instead of deciding it here The engine already skips re-detailing when an incremental finds no cluster or membership deltas (CodeBoarding #605 took that path today with no model call), so the scope pre-check that listed the pull request's files and decided the same thing in bash is gone, and the action is a wrapper again. What stays is the relay: the artifact step reads `metadata.incremental_unchanged` from the analysis the engine wrote (CodeBoarding #608) and carries it into the review artifact's metadata and, through an output, into the comment, whose status line says "(nothing analysed changed)" and whose machine-readable line says `unchanged=true`. An analysis written before the field existed reads as false, the safe direction. The progress comment now carries the platform link from the start, so a pull request can be opened there while the run is still going. Co-Authored-By: Claude Fable 5.1 --- README.md | 4 +- action.yml | 45 ++----- scripts/action/build-review-artifact.sh | 13 +- scripts/action/build-review-comment.sh | 13 +- scripts/action/scope-check.sh | 67 ---------- scripts/action/scope_check.py | 58 -------- tests/test_review_artifact_unchanged.py | 74 +++++++++++ tests/test_review_comment.py | 12 +- tests/test_scope_check.py | 168 ------------------------ 9 files changed, 110 insertions(+), 344 deletions(-) delete mode 100755 scripts/action/scope-check.sh delete mode 100644 scripts/action/scope_check.py create mode 100644 tests/test_review_artifact_unchanged.py delete mode 100644 tests/test_scope_check.py diff --git a/README.md b/README.md index da91bc8..acaaa89 100644 --- a/README.md +++ b/README.md @@ -100,9 +100,9 @@ Fork pull requests never carry an analysis forward. They are reviewed on request ### Pull requests that change nothing analysed -A pull request whose changed files are all outside the analysed scope (docs, configuration, CI, tests the ignore file excludes, or a language the engine does not read) cannot have moved the architecture. Before analysing, a review run lists the pull request's files and counts them against the engine's own rules: its language extensions and `.codeboarding/.codeboardingignore` over the checked-out repository. When none is analysed and a base graph is at hand (a published base, or the baseline the sync workflow committed), the engine is not run: the base is published as the head, the diff renders as empty, and the comment says `0 changed components (no analysed file changed)`, so the zero is known to be decided from the files rather than measured by a diff. Nothing is skipped when the file list cannot be read, when the engine cannot be imported, or when no base exists. +A pull request whose changed files are all outside what the engine analyses (docs, configuration, CI, tests the ignore file excludes, or a language the engine does not read) cannot have moved the architecture, and the engine decides that itself: its incremental run finds no cluster or membership deltas, rewrites the baseline as the head without re-detailing, and consults no model. The run still costs its setup (checkout, install, base fetch, static analysis), which is a minute or two, but no tokens. The engine records that decision in the analysis metadata (`incremental_unchanged: true`), and the action relays it: the comment says `0 changed components (nothing analysed changed)`, so the zero is known to be decided rather than reported by a model, and the review artifact's `metadata.json` carries `incremental_unchanged` (a string, like every other field there). -Every review comment ends with a machine-readable HTML comment, ``, and the review artifact's `metadata.json` carries `scope_skipped`, `changed_files` and `analysed_files` (strings, like every other field there). +Every review comment ends with a machine-readable HTML comment, ``, for readers that should not parse the prose or the diagram. The progress comment carries the platform link from the start, so a pull request can be opened there while the run is still going. ## Authentication and providers diff --git a/action.yml b/action.yml index 0084603..d435ba0 100644 --- a/action.yml +++ b/action.yml @@ -333,6 +333,8 @@ runs: ⏳ CodeBoarding is analyzing this pull request's architecture changes. + Open it in [CodeBoarding](https://app.codeboarding.org/${{ github.repository }}/pull/${{ steps.guard.outputs.pr_number }}?utm_source=github&utm_medium=pr_comment&utm_campaign=gh_action) meanwhile: the files, comments and review are there already, and the diff appears when the run finishes. + run [${{ github.run_id }}](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}) · attempt ${{ github.run_attempt }} - name: Checkout analysis target @@ -507,29 +509,9 @@ runs: PR_URL: ${{ steps.sync_commit.outputs.sync_pr_url }} run: "$GITHUB_ACTION_PATH/scripts/action/sync-summary.sh" - # A pull request that changes nothing the engine would analyse (docs, config, CI, tests - # it ignores, a language it does not read) cannot have moved the architecture, so its - # head analysis IS the base analysis. When a base graph is at hand, the run below is - # skipped and the base is published as the head; the comment says "0 changed components - # (no analysed file changed)" so the zero is known to be decided, not measured. - - name: Check whether any analysed file changed - id: scope - if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' - shell: bash - env: - ACTION_PATH: ${{ github.action_path }} - GH_TOKEN: ${{ inputs.github_token }} - GH_ENTERPRISE_TOKEN: ${{ inputs.github_token }} - GH_HOST: ${{ github.server_url }} - REPOSITORY: ${{ github.repository }} - PR_NUMBER: ${{ steps.guard.outputs.pr_number }} - CHECKOUT_DIR: ${{ github.workspace }}/.codeboarding-target - BASE_DIR: ${{ runner.temp }}/cb-state/${{ github.action }}/base - run: "$GITHUB_ACTION_PATH/scripts/action/scope-check.sh" - - name: Analyze pull request id: review_analyze - if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.scope.outputs.skip != 'true' + if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' shell: bash env: ACTION_PATH: ${{ github.action_path }} @@ -557,7 +539,7 @@ runs: # so a later rendering or posting failure must not throw it away. Only the # next run reads this, hence the short retention. - name: Publish this analysis for the next run - if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.scope.outputs.skip != 'true' && steps.state.outputs.warmstart_name != '' && github.server_url == 'https://github.com' + if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.state.outputs.warmstart_name != '' && github.server_url == 'https://github.com' continue-on-error: true uses: actions/upload-artifact@v4 with: @@ -592,8 +574,8 @@ runs: shell: bash env: ACTION_PATH: ${{ github.action_path }} - BASE_ANALYSIS_PATH: ${{ steps.review_analyze.outputs.base_analysis_path || steps.scope.outputs.base_analysis_path }} - HEAD_ANALYSIS_PATH: ${{ steps.review_analyze.outputs.analysis_path || steps.scope.outputs.analysis_path }} + BASE_ANALYSIS_PATH: ${{ steps.review_analyze.outputs.base_analysis_path }} + HEAD_ANALYSIS_PATH: ${{ steps.review_analyze.outputs.analysis_path }} run: "$GITHUB_ACTION_PATH/scripts/action/render-review.sh" - name: Build review artifact @@ -601,15 +583,12 @@ runs: if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' shell: bash env: - ANALYSIS_PATH: ${{ steps.review_analyze.outputs.analysis_path || steps.scope.outputs.analysis_path }} + ANALYSIS_PATH: ${{ steps.review_analyze.outputs.analysis_path }} BASE_ARTIFACT_NAME: ${{ steps.state.outputs.base_name }} BASE_ARTIFACT_ID: ${{ steps.publish_base.outputs.artifact-id || steps.fetch_base.outputs.artifact_id }} - BASE_ANALYSIS_PATH: ${{ steps.review_analyze.outputs.base_analysis_path || steps.scope.outputs.base_analysis_path }} + BASE_ANALYSIS_PATH: ${{ steps.review_analyze.outputs.base_analysis_path }} INLINE_BASE: ${{ steps.publish_base.outputs.artifact-id == '' && steps.fetch_base.outputs.artifact_id == '' }} - ANALYSIS_MODE: ${{ steps.review_analyze.outputs.analysis_mode || steps.scope.outputs.analysis_mode }} - SCOPE_SKIPPED: ${{ steps.scope.outputs.skip }} - CHANGED_FILES: ${{ steps.scope.outputs.changed_files }} - ANALYSED_FILES: ${{ steps.scope.outputs.analysed_files }} + ANALYSIS_MODE: ${{ steps.review_analyze.outputs.analysis_mode }} BASE_SHA: ${{ steps.guard.outputs.base_sha }} MERGE_BASE_SHA: ${{ steps.guard.outputs.merge_base_sha }} MERGE_BASE_RESOLVED: ${{ steps.guard.outputs.merge_base_resolved }} @@ -646,9 +625,9 @@ runs: BASE_REF: ${{ steps.guard.outputs.base_ref }} MERGE_BASE_RESOLVED: ${{ steps.guard.outputs.merge_base_resolved }} HEAD_SHA: ${{ steps.guard.outputs.head_sha }} - SCOPE_SKIPPED: ${{ steps.scope.outputs.skip }} - CHANGED_FILES: ${{ steps.scope.outputs.changed_files }} - ANALYSED_FILES: ${{ steps.scope.outputs.analysed_files }} + # The engine's own word, relayed by the artifact step: true when the incremental took its + # early exit and nothing was re-detailed, so the comment's zero is known to be decided. + UNCHANGED: ${{ steps.review_artifact.outputs.unchanged }} run: "$GITHUB_ACTION_PATH/scripts/action/build-review-comment.sh" - name: Post review comment diff --git a/scripts/action/build-review-artifact.sh b/scripts/action/build-review-artifact.sh index 67a587f..c278ff1 100755 --- a/scripts/action/build-review-artifact.sh +++ b/scripts/action/build-review-artifact.sh @@ -18,6 +18,12 @@ fi HEALTH_REPORT="$(dirname "$ANALYSIS_PATH")/health/health_report.json" [ ! -f "$HEALTH_REPORT" ] || cp "$HEALTH_REPORT" "${RUNNER_TEMP}/cb-review-artifact/health_report.json" +# The engine's own verdict, read from the analysis it wrote: true when the incremental took +# its early exit (no cluster or membership deltas, nothing re-detailed, no model consulted), +# so the comment's "0 changed components" is a decided fact rather than a model's word. An +# analysis written before the field existed reads as false, which is the safe direction. +UNCHANGED="$(jq -r 'if .metadata.incremental_unchanged == true then "true" else "false" end' "$ANALYSIS_PATH" 2>/dev/null || echo false)" + # base_sha stays the event's base branch tip for consumers that key on it; # merge_base_sha records the commit the diagram actually compared against. # pr_base_sha carries the same value under the name the webview already reads: @@ -36,13 +42,12 @@ jq -n \ --arg chain_depth "$CHAIN_DEPTH" \ --arg base_artifact "$BASE_ARTIFACT_NAME" \ --arg base_artifact_id "$BASE_ARTIFACT_ID" \ - --arg scope_skipped "${SCOPE_SKIPPED:-false}" \ - --arg changed_files "${CHANGED_FILES:-}" \ - --arg analysed_files "${ANALYSED_FILES:-}" \ + --arg incremental_unchanged "$UNCHANGED" \ '{kind: $kind, mode: $mode, base_sha: $base_sha, merge_base_sha: $merge_base_sha, pr_base_sha: $merge_base_sha, merge_base_resolved: $merge_base_resolved, head_sha: $head_sha, pr_number: $pr_number, seed_source: $seed_source, chain_depth: $chain_depth, base_artifact: $base_artifact, base_artifact_id: $base_artifact_id, - scope_skipped: $scope_skipped, changed_files: $changed_files, analysed_files: $analysed_files}' \ + incremental_unchanged: $incremental_unchanged}' \ > "${RUNNER_TEMP}/cb-review-artifact/metadata.json" echo "artifact_dir=${RUNNER_TEMP}/cb-review-artifact" >> "$GITHUB_OUTPUT" +echo "unchanged=$UNCHANGED" >> "$GITHUB_OUTPUT" diff --git a/scripts/action/build-review-comment.sh b/scripts/action/build-review-comment.sh index 61ce58b..235c089 100755 --- a/scripts/action/build-review-comment.sh +++ b/scripts/action/build-review-comment.sh @@ -18,11 +18,12 @@ PLATFORM_URL="https://app.codeboarding.org/${GITHUB_REPOSITORY}/pull/${PR_NUMBER WEBVIEW_URL="${PLATFORM_URL}?utm_source=github&utm_medium=pr_comment&utm_campaign=gh_action" BODY="${RUNNER_TEMP}/review-comment.md" # The status line is what the web platform reads the count from, so its shape is a contract. -# A run the scope check skipped says so in the same line: its zero is decided from the -# changed files, not from a diff of two graphs, and a reader deserves to know which. +# When the engine's incremental took its early exit, the same line says so: that zero was +# decided (no cluster or membership deltas, no model consulted), not reported by a diff of +# two re-detailed graphs, and a reader deserves to know which. STATUS="${N_CHANGED} changed ${COMPONENT_NOUN}" -if [ "${SCOPE_SKIPPED:-false}" = true ]; then - STATUS="${STATUS} (no analysed file changed)" +if [ "${UNCHANGED:-false}" = true ]; then + STATUS="${STATUS} (nothing analysed changed)" fi printf '### CodeBoarding review\n\n**Status:** %s\n' "$STATUS" > "$BODY" printf '\nSee the full change in [CodeBoarding](%s).\n' "$WEBVIEW_URL" >> "$BODY" @@ -51,7 +52,7 @@ fi # The machine-readable line: what a reader of the comment (the web platform's dashboard, an # agent) needs without parsing the prose or the diagram. An HTML comment renders as nothing. # Keep it one line, `key=value` pairs, values without spaces, so a regex over it stays trivial. - printf '\n' \ - "$PLATFORM_URL" "$N_CHANGED" "${CHANGED_FILES:-}" "${ANALYSED_FILES:-}" "${HEAD_SHA:-}" + printf '\n' \ + "$PLATFORM_URL" "$N_CHANGED" "${UNCHANGED:-false}" "${HEAD_SHA:-}" } >> "$BODY" echo "path=$BODY" >> "$GITHUB_OUTPUT" diff --git a/scripts/action/scope-check.sh b/scripts/action/scope-check.sh deleted file mode 100755 index 05ada34..0000000 --- a/scripts/action/scope-check.sh +++ /dev/null @@ -1,67 +0,0 @@ -#!/usr/bin/env bash -# Decides whether a review run can skip the engine: when none of the pull request's changed -# files is one the engine would analyse (docs, config, CI, tests it ignores, a language it -# does not read), the architecture cannot have moved, and the base analysis IS the head -# analysis. The run then publishes the base as the head, renders an empty diff, and says so. -# -# Outputs: changed_files, analysed_files, skip (true only when the shortcut applies), and on -# skip the analysis_path / base_analysis_path / analysis_mode the later steps read in place of -# the analysis step's. Never fails the job: a read that did not answer means "run as usual". -set -euo pipefail - -changed=0 -analysed=0 -skip=false - -emit() { - printf 'changed_files=%s\nanalysed_files=%s\nskip=%s\n' "$changed" "$analysed" "$skip" >> "$GITHUB_OUTPUT" -} - -# The checkout is the head commit alone, so the changed files come from the API, not git. -files="$(gh api "repos/${REPOSITORY}/pulls/${PR_NUMBER}/files" --paginate --jq '.[].filename' 2>/dev/null)" || { - echo "::notice::Could not list the pull request's files; analysing as usual." - emit; exit 0 -} -if [ -z "$files" ]; then - echo "::notice::The pull request changes no files; analysing as usual." - emit; exit 0 -fi - -# From the action's own directory, so the analysed repository's modules cannot shadow the -# engine's on sys.path (see the install step in action.yml). -counts="$(cd "$ACTION_PATH" && printf '%s\n' "$files" | python3 scripts/action/scope_check.py --repo-root "$CHECKOUT_DIR")" || { - echo "::notice::Could not decide the analysed scope; analysing as usual." - emit; exit 0 -} -changed="$(printf '%s' "$counts" | jq -r '.changed')" -analysed="$(printf '%s' "$counts" | jq -r '.analysed')" -if [ "$changed" -eq 0 ] || [ "$analysed" -gt 0 ]; then - emit; exit 0 -fi - -# Nothing analysed changed. The base graph must already exist for the shortcut: a published -# base from the fetch step, or the baseline the sync workflow committed. Computing one would -# be the run this exists to skip. -base="" -if [ -f "${BASE_DIR:-}/analysis.json" ]; then - base="${BASE_DIR}/analysis.json" -elif [ -f "${CHECKOUT_DIR}/.codeboarding/analysis.json" ]; then - base="${CHECKOUT_DIR}/.codeboarding/analysis.json" -fi -if [ -z "$base" ]; then - echo "::notice::No analysed file changed, but no base analysis is at hand; analysing as usual." - emit; exit 0 -fi - -work="${RUNNER_TEMP}/codeboarding-scope" -mkdir -p "$work" -cp "$base" "$work/analysis.json" -# The base's health report, when it sits beside the base, describes the head too. -if [ -f "$(dirname "$base")/health/health_report.json" ]; then - mkdir -p "$work/health" - cp "$(dirname "$base")/health/health_report.json" "$work/health/health_report.json" -fi -skip=true -echo "::notice::No analysed file changed (${changed} files, all outside the analysed scope); reusing the base analysis as the head." -emit -printf 'analysis_path=%s\nbase_analysis_path=%s\nanalysis_mode=unchanged\n' "$work/analysis.json" "$base" >> "$GITHUB_OUTPUT" diff --git a/scripts/action/scope_check.py b/scripts/action/scope_check.py deleted file mode 100644 index 3d84a82..0000000 --- a/scripts/action/scope_check.py +++ /dev/null @@ -1,58 +0,0 @@ -"""Count how many of a pull request's changed files the engine would analyse. - -Reads one path per line on stdin and prints a JSON object: ``changed`` (paths read) and -``analysed`` (paths in the analysed scope). A path is analysed when its extension belongs to a -language the engine reads and the engine's own ignore rules (``.codeboarding/.codeboardingignore`` -over the repository root, plus the directories it always drops) let it through. Both come from -the installed engine, so this says exactly what the engine would look at, no more and no less. - -When the engine cannot be imported every path counts as analysed: the answer this feeds is -"skip the run", and a doubt must never skip one. -""" - -from __future__ import annotations - -import argparse -import json -import sys -from pathlib import Path - - -def _engine_scope(repo_root: Path): - """The engine's extension map and ignore manager, or None when it is not importable.""" - try: - from repo_utils.ignore import RepoIgnoreManager # type: ignore[import-not-found] - from static_analyzer.config import SOURCE_EXTENSION_TO_LANGUAGE # type: ignore[import-not-found] - except Exception: # noqa: BLE001 - any import failure means "do not decide" - return None - extensions = {str(ext).lower() for ext in SOURCE_EXTENSION_TO_LANGUAGE} - return extensions, RepoIgnoreManager(repo_root) - - -def count_analysed(paths: list[str], repo_root: Path) -> dict[str, int]: - scope = _engine_scope(repo_root) - if scope is None: - return {"changed": len(paths), "analysed": len(paths)} - extensions, ignores = scope - analysed = 0 - for raw in paths: - path = Path(raw) - if path.suffix.lower() not in extensions: - continue - if ignores.should_ignore(path): - continue - analysed += 1 - return {"changed": len(paths), "analysed": analysed} - - -def main() -> int: - parser = argparse.ArgumentParser(description=__doc__) - parser.add_argument("--repo-root", required=True, help="The checked-out repository, where the ignore file lives.") - args = parser.parse_args() - paths = [line.strip() for line in sys.stdin.read().splitlines() if line.strip()] - print(json.dumps(count_analysed(paths, Path(args.repo_root)))) - return 0 - - -if __name__ == "__main__": - sys.exit(main()) diff --git a/tests/test_review_artifact_unchanged.py b/tests/test_review_artifact_unchanged.py new file mode 100644 index 0000000..1c02b35 --- /dev/null +++ b/tests/test_review_artifact_unchanged.py @@ -0,0 +1,74 @@ +"""The artifact relays the engine's early-exit verdict, and reads a pre-flag analysis as not unchanged.""" + +import json +import os +import subprocess +import tempfile +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +BUILD_ARTIFACT = ROOT / "scripts" / "action" / "build-review-artifact.sh" + + +def _build(root: Path, analysis: str) -> tuple[dict, dict]: + head = root / "head.json" + head.write_text(analysis, encoding="utf-8") + base = root / "base.json" + base.write_text('{"components": ["base"]}', encoding="utf-8") + output = root / "github-output" + output.write_text("", encoding="utf-8") + result = subprocess.run( + [str(BUILD_ARTIFACT)], + env={ + "PATH": os.environ["PATH"], + "RUNNER_TEMP": str(root), + "GITHUB_OUTPUT": str(output), + "ANALYSIS_PATH": str(head), + "BASE_ARTIFACT_NAME": "codeboarding-base-cfg-mergebasesha", + "BASE_ARTIFACT_ID": "4242", + "BASE_ANALYSIS_PATH": str(base), + "INLINE_BASE": "false", + "ANALYSIS_MODE": "incremental", + "BASE_SHA": "tip-sha", + "MERGE_BASE_SHA": "merge-base-sha", + "MERGE_BASE_RESOLVED": "true", + "HEAD_SHA": "head-sha", + "PR_NUMBER": "81", + "SEED_SOURCE": "pr-chain", + "CHAIN_DEPTH": "2", + }, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 0, result.stderr or result.stdout + metadata = json.loads((root / "cb-review-artifact" / "metadata.json").read_text(encoding="utf-8")) + outputs: dict[str, str] = {} + for line in output.read_text(encoding="utf-8").splitlines(): + key, _, value = line.partition("=") + outputs[key] = value + return metadata, outputs + + +class ReviewArtifactUnchangedTests(unittest.TestCase): + def test_the_early_exit_flag_is_relayed_to_the_metadata_and_the_comment_step(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + metadata, outputs = _build(Path(tmp), '{"metadata": {"incremental_unchanged": true}, "components": []}') + # A string, like every other `--arg` field the webview reads from this file. + self.assertEqual(metadata["incremental_unchanged"], "true") + self.assertEqual(outputs["unchanged"], "true") + + def test_a_re_detailed_run_and_a_pre_flag_analysis_both_read_as_not_unchanged(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + metadata, outputs = _build(Path(tmp), '{"metadata": {"incremental_unchanged": false}, "components": []}') + self.assertEqual(metadata["incremental_unchanged"], "false") + self.assertEqual(outputs["unchanged"], "false") + with tempfile.TemporaryDirectory() as tmp: + metadata, outputs = _build(Path(tmp), '{"components": ["head"]}') + self.assertEqual(metadata["incremental_unchanged"], "false") + self.assertEqual(outputs["unchanged"], "false") + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_review_comment.py b/tests/test_review_comment.py index e5d8b7b..a1fcc29 100644 --- a/tests/test_review_comment.py +++ b/tests/test_review_comment.py @@ -50,22 +50,22 @@ def test_status_line_and_platform_link_as_the_web_platform_reads_them(self) -> N def test_the_machine_readable_line_ends_the_body(self) -> None: with tempfile.TemporaryDirectory() as tmp: - body = _build(Path(tmp), CHANGED_FILES="4", ANALYSED_FILES="2") + body = _build(Path(tmp)) last = body.rstrip("\n").splitlines()[-1] self.assertEqual( last, - "", + "", ) # The status regex the web platform uses must still find the status line, not the marker. match = re.search(r"\*\*Status:\*\*\s*(\d+)\s+changed\s+components?", body) self.assertIsNotNone(match) self.assertEqual(match.group(1) if match else None, "3") - def test_a_skipped_run_says_its_zero_was_decided_from_the_files(self) -> None: + def test_the_engines_early_exit_is_said_in_the_status_and_the_marker(self) -> None: with tempfile.TemporaryDirectory() as tmp: - body = _build(Path(tmp), N_CHANGED="0", SCOPE_SKIPPED="true", CHANGED_FILES="2", ANALYSED_FILES="0") - self.assertIn("**Status:** 0 changed components (no analysed file changed)\n", body) - self.assertIn("changed=0 changed_files=2 analysed_files=0", body) + body = _build(Path(tmp), N_CHANGED="0", UNCHANGED="true") + self.assertIn("**Status:** 0 changed components (nothing analysed changed)\n", body) + self.assertIn("changed=0 unchanged=true head=abc123", body) if __name__ == "__main__": diff --git a/tests/test_scope_check.py b/tests/test_scope_check.py deleted file mode 100644 index 479ec59..0000000 --- a/tests/test_scope_check.py +++ /dev/null @@ -1,168 +0,0 @@ -"""The scope check: which changed files the engine would analyse, and the shortcut it allows.""" - -import json -import os -import subprocess -import tempfile -import unittest -from pathlib import Path - -ROOT = Path(__file__).resolve().parents[1] -SCOPE_CHECK_PY = ROOT / "scripts" / "action" / "scope_check.py" -SCOPE_CHECK_SH = ROOT / "scripts" / "action" / "scope-check.sh" - -# A stand-in for the installed engine: the extension map and the ignore manager with the -# rules the real ones apply (extension by suffix, hidden and always-excluded directories, then -# the ignore file's patterns), small enough to read in one go. -FAKE_ENGINE = { - "static_analyzer/__init__.py": "", - "static_analyzer/config.py": ( - "SOURCE_EXTENSION_TO_LANGUAGE = {ext: 'x' for ext in ('.py', '.ts', '.tsx', '.java', '.go', '.php', '.rs', '.cs', '.cpp')}\n" - ), - "repo_utils/__init__.py": "", - "repo_utils/ignore.py": ( - "from pathlib import Path\n" - "import fnmatch\n" - "ALWAYS = {'.git', '.codeboarding', 'node_modules', '__pycache__', 'build', 'dist', 'coverage', 'target'}\n" - "class RepoIgnoreManager:\n" - " def __init__(self, repo_root):\n" - " self.repo_root = Path(repo_root)\n" - " f = self.repo_root / '.codeboarding' / '.codeboardingignore'\n" - " self.patterns = [l.strip() for l in f.read_text().splitlines() if l.strip() and not l.startswith('#')] if f.exists() else []\n" - " def should_ignore(self, path):\n" - " rel = Path(path)\n" - " if rel.is_absolute(): rel = rel.relative_to(self.repo_root)\n" - " if any(p in ALWAYS or p.startswith('.') for p in rel.parts[:-1]): return True\n" - " return any(fnmatch.fnmatch(str(rel), pat) or fnmatch.fnmatch(str(rel), pat.rstrip('/') + '/*') for pat in self.patterns)\n" - ), -} - - -def _plant(root: Path, files: dict) -> None: - for name, text in files.items(): - target = root / name - target.parent.mkdir(parents=True, exist_ok=True) - target.write_text(text, encoding="utf-8") - - -def _run_py(paths: list, repo_root: Path, engine: Path) -> dict: - result = subprocess.run( - ["python3", str(SCOPE_CHECK_PY), "--repo-root", str(repo_root)], - input="\n".join(paths) + "\n", - env={"PATH": os.environ["PATH"], "PYTHONPATH": str(engine)}, - capture_output=True, - text=True, - check=True, - ) - return json.loads(result.stdout) - - -class ScopeCheckPyTests(unittest.TestCase): - def test_counts_only_files_the_engine_would_analyse(self) -> None: - with tempfile.TemporaryDirectory() as tmp: - root = Path(tmp) - engine = root / "engine" - _plant(engine, FAKE_ENGINE) - repo = root / "repo" - _plant(repo, {".codeboarding/.codeboardingignore": "# ignore\n**/tests/**\nvendor/\n"}) - counts = _run_py( - [ - "README.md", - "docs/development/architecture.md", - ".github/workflows/ci.yml", - "src/lib/turn.ts", - "src/lib/tests/turn_test.ts", - "node_modules/x/index.js", - "vendor/lib.py", - "gradle/libs.versions.toml", - ], - repo, - engine, - ) - self.assertEqual(counts, {"changed": 8, "analysed": 1}) - - def test_without_the_engine_every_file_counts_as_analysed(self) -> None: - with tempfile.TemporaryDirectory() as tmp: - root = Path(tmp) - counts = _run_py(["README.md", "docs/a.md"], root / "repo", root / "no-engine") - self.assertEqual(counts, {"changed": 2, "analysed": 2}) - - -class ScopeCheckShTests(unittest.TestCase): - def _run(self, root: Path, files: list, base: Path | None) -> dict: - fake_bin = root / "bin" - fake_bin.mkdir(exist_ok=True) - gh = fake_bin / "gh" - gh.write_text("#!/usr/bin/env bash\n" + "".join(f"printf '%s\\n' '{f}'\n" for f in files), encoding="utf-8") - gh.chmod(0o755) - engine = root / "engine" - _plant(engine, FAKE_ENGINE) - checkout = root / "checkout" - checkout.mkdir(exist_ok=True) - output = root / "github-output" - output.write_text("", encoding="utf-8") - temp = root / "runner-temp" - temp.mkdir(exist_ok=True) - env = { - "PATH": f"{fake_bin}:{os.environ['PATH']}", - "PYTHONPATH": str(engine), - "ACTION_PATH": str(ROOT), - "REPOSITORY": "owner/repo", - "PR_NUMBER": "7", - "CHECKOUT_DIR": str(checkout), - "BASE_DIR": str(base) if base else str(root / "no-base"), - "GITHUB_OUTPUT": str(output), - "RUNNER_TEMP": str(temp), - } - result = subprocess.run([str(SCOPE_CHECK_SH)], env=env, capture_output=True, text=True, check=False) - self.assertEqual(result.returncode, 0, result.stderr) - out = {} - for line in output.read_text(encoding="utf-8").splitlines(): - key, _, value = line.partition("=") - out[key] = value - return out - - def test_skips_when_nothing_analysed_changed_and_a_base_is_at_hand(self) -> None: - with tempfile.TemporaryDirectory() as tmp: - root = Path(tmp) - base = root / "base" - base.mkdir() - (base / "analysis.json").write_text('{"description": "base"}', encoding="utf-8") - out = self._run(root, ["README.md", ".github/workflows/ci.yml"], base) - self.assertEqual(out["skip"], "true") - self.assertEqual(out["changed_files"], "2") - self.assertEqual(out["analysed_files"], "0") - self.assertEqual(out["analysis_mode"], "unchanged") - self.assertEqual(out["base_analysis_path"], str(base / "analysis.json")) - self.assertEqual(Path(out["analysis_path"]).read_text(encoding="utf-8"), '{"description": "base"}') - - def test_runs_as_usual_when_an_analysed_file_changed(self) -> None: - with tempfile.TemporaryDirectory() as tmp: - root = Path(tmp) - base = root / "base" - base.mkdir() - (base / "analysis.json").write_text("{}", encoding="utf-8") - out = self._run(root, ["README.md", "src/a.py"], base) - self.assertEqual(out["skip"], "false") - self.assertEqual(out["analysed_files"], "1") - self.assertNotIn("analysis_path", out) - - def test_runs_as_usual_without_a_base_to_reuse(self) -> None: - with tempfile.TemporaryDirectory() as tmp: - root = Path(tmp) - out = self._run(root, ["README.md"], None) - self.assertEqual(out["skip"], "false") - self.assertEqual(out["analysed_files"], "0") - - def test_reuses_the_committed_baseline_when_no_base_was_published(self) -> None: - with tempfile.TemporaryDirectory() as tmp: - root = Path(tmp) - checkout = root / "checkout" - _plant(checkout, {".codeboarding/analysis.json": '{"description": "committed"}'}) - out = self._run(root, ["docs/a.md"], None) - self.assertEqual(out["skip"], "true") - self.assertEqual(out["base_analysis_path"], str(checkout / ".codeboarding" / "analysis.json")) - - -if __name__ == "__main__": - unittest.main() From c66b36f61ea439bf044c78fcfab5597f4fc338e2 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Tue, 22 Sep 2026 18:43:15 +0200 Subject: [PATCH 3/5] fix(comment): call a review unchanged only when the engine's flag and the diff's zero agree The engine's early exit says the clusters and their membership held. A body-only edit keeps them while moving method hashes, which the diff counts as a modified component; and a run seeded from the pull request's previous head takes the exit for a docs-only push on top of real changes. Either way the diff is not zero, and the comment must not say nothing changed. Co-Authored-By: Claude Fable 5.1 --- README.md | 2 +- scripts/action/build-review-comment.sh | 16 +++++++++++----- tests/test_review_comment.py | 10 ++++++++++ 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index acaaa89..79f7e99 100644 --- a/README.md +++ b/README.md @@ -100,7 +100,7 @@ Fork pull requests never carry an analysis forward. They are reviewed on request ### Pull requests that change nothing analysed -A pull request whose changed files are all outside what the engine analyses (docs, configuration, CI, tests the ignore file excludes, or a language the engine does not read) cannot have moved the architecture, and the engine decides that itself: its incremental run finds no cluster or membership deltas, rewrites the baseline as the head without re-detailing, and consults no model. The run still costs its setup (checkout, install, base fetch, static analysis), which is a minute or two, but no tokens. The engine records that decision in the analysis metadata (`incremental_unchanged: true`), and the action relays it: the comment says `0 changed components (nothing analysed changed)`, so the zero is known to be decided rather than reported by a model, and the review artifact's `metadata.json` carries `incremental_unchanged` (a string, like every other field there). +A pull request whose changed files are all outside what the engine analyses (docs, configuration, CI, tests the ignore file excludes, or a language the engine does not read) cannot have moved the architecture, and the engine decides that itself: its incremental run finds no cluster or membership deltas, rewrites the baseline as the head without re-detailing, and consults no model. The run still costs its setup (checkout, install, base fetch, static analysis), which is a minute or two, but no tokens. The engine records that decision in the analysis metadata (`incremental_unchanged: true`), and the action relays it: when the diff also found nothing, the comment says `0 changed components (nothing analysed changed)`, so the zero is known to be decided rather than reported by a model, and the review artifact's `metadata.json` carries `incremental_unchanged` (a string, like every other field there). The engine's flag alone is not the verdict: a body-only edit keeps the clusters while moving method hashes, which the diff counts as a modified component, and a run seeded from the pull request's previous head can take the early exit for a docs-only push on top of real changes. The comment says unchanged only when both agree. Every review comment ends with a machine-readable HTML comment, ``, for readers that should not parse the prose or the diagram. The progress comment carries the platform link from the start, so a pull request can be opened there while the run is still going. diff --git a/scripts/action/build-review-comment.sh b/scripts/action/build-review-comment.sh index 235c089..1383133 100755 --- a/scripts/action/build-review-comment.sh +++ b/scripts/action/build-review-comment.sh @@ -18,11 +18,17 @@ PLATFORM_URL="https://app.codeboarding.org/${GITHUB_REPOSITORY}/pull/${PR_NUMBER WEBVIEW_URL="${PLATFORM_URL}?utm_source=github&utm_medium=pr_comment&utm_campaign=gh_action" BODY="${RUNNER_TEMP}/review-comment.md" # The status line is what the web platform reads the count from, so its shape is a contract. -# When the engine's incremental took its early exit, the same line says so: that zero was -# decided (no cluster or membership deltas, no model consulted), not reported by a diff of -# two re-detailed graphs, and a reader deserves to know which. +# When the engine's incremental took its early exit AND the diff found nothing, the same line +# says so: that zero was decided (no cluster or membership deltas, no model consulted), not +# reported by a diff of two re-detailed graphs. Both halves are needed. The engine's flag alone +# says the clusters held; a body-only edit still moves method hashes, which the diff counts as +# a modified component, and a run seeded from this pull request's previous head can take the +# early exit for a docs-only push on top of real changes. So the verdict is the engine's word +# and the diff's zero together, and neither alone. STATUS="${N_CHANGED} changed ${COMPONENT_NOUN}" -if [ "${UNCHANGED:-false}" = true ]; then +VERDICT_UNCHANGED=false +if [ "${UNCHANGED:-false}" = true ] && [ "$N_CHANGED" = "0" ]; then + VERDICT_UNCHANGED=true STATUS="${STATUS} (nothing analysed changed)" fi printf '### CodeBoarding review\n\n**Status:** %s\n' "$STATUS" > "$BODY" @@ -53,6 +59,6 @@ fi # agent) needs without parsing the prose or the diagram. An HTML comment renders as nothing. # Keep it one line, `key=value` pairs, values without spaces, so a regex over it stays trivial. printf '\n' \ - "$PLATFORM_URL" "$N_CHANGED" "${UNCHANGED:-false}" "${HEAD_SHA:-}" + "$PLATFORM_URL" "$N_CHANGED" "$VERDICT_UNCHANGED" "${HEAD_SHA:-}" } >> "$BODY" echo "path=$BODY" >> "$GITHUB_OUTPUT" diff --git a/tests/test_review_comment.py b/tests/test_review_comment.py index a1fcc29..5445cf8 100644 --- a/tests/test_review_comment.py +++ b/tests/test_review_comment.py @@ -67,6 +67,16 @@ def test_the_engines_early_exit_is_said_in_the_status_and_the_marker(self) -> No self.assertIn("**Status:** 0 changed components (nothing analysed changed)\n", body) self.assertIn("changed=0 unchanged=true head=abc123", body) + def test_the_early_exit_alone_is_not_the_verdict(self) -> None: + # A run seeded from the pull request's previous head takes the early exit for a docs-only + # push on top of real changes, and a body-only edit keeps the clusters while moving method + # hashes. In both the diff is not zero, and the comment must not call it unchanged. + with tempfile.TemporaryDirectory() as tmp: + body = _build(Path(tmp), N_CHANGED="3", UNCHANGED="true") + self.assertIn("**Status:** 3 changed components\n", body) + self.assertNotIn("nothing analysed changed", body) + self.assertIn("changed=3 unchanged=false head=abc123", body) + if __name__ == "__main__": unittest.main() From 97bdf2e47b3afa52ce271f9a16c79cb8ae67ec10 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Tue, 22 Sep 2026 19:05:37 +0200 Subject: [PATCH 4/5] chore(artifact): follow the engine's rename to structure_unchanged Co-Authored-By: Claude Fable 5.1 --- README.md | 2 +- scripts/action/build-review-artifact.sh | 6 +++--- tests/test_review_artifact_unchanged.py | 10 +++++----- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 79f7e99..41ddb86 100644 --- a/README.md +++ b/README.md @@ -100,7 +100,7 @@ Fork pull requests never carry an analysis forward. They are reviewed on request ### Pull requests that change nothing analysed -A pull request whose changed files are all outside what the engine analyses (docs, configuration, CI, tests the ignore file excludes, or a language the engine does not read) cannot have moved the architecture, and the engine decides that itself: its incremental run finds no cluster or membership deltas, rewrites the baseline as the head without re-detailing, and consults no model. The run still costs its setup (checkout, install, base fetch, static analysis), which is a minute or two, but no tokens. The engine records that decision in the analysis metadata (`incremental_unchanged: true`), and the action relays it: when the diff also found nothing, the comment says `0 changed components (nothing analysed changed)`, so the zero is known to be decided rather than reported by a model, and the review artifact's `metadata.json` carries `incremental_unchanged` (a string, like every other field there). The engine's flag alone is not the verdict: a body-only edit keeps the clusters while moving method hashes, which the diff counts as a modified component, and a run seeded from the pull request's previous head can take the early exit for a docs-only push on top of real changes. The comment says unchanged only when both agree. +A pull request whose changed files are all outside what the engine analyses (docs, configuration, CI, tests the ignore file excludes, or a language the engine does not read) cannot have moved the architecture, and the engine decides that itself: its incremental run finds no cluster or membership deltas, rewrites the baseline as the head without re-detailing, consults no model, and checks after finalisation that the saved components, membership and relations still equal the baseline's. The run still costs its setup (checkout, install, base fetch, static analysis), which is a minute or two, but no tokens. The engine records that decision in the analysis metadata (`structure_unchanged: true`), and the action relays it: when the diff also found nothing, the comment says `0 changed components (nothing analysed changed)`, so the zero is known to be decided rather than reported by a model, and the review artifact's `metadata.json` carries `structure_unchanged` (a string, like every other field there). The engine's flag alone is not the verdict: a body-only edit keeps the clusters while moving method hashes, which the diff counts as a modified component, and a run seeded from the pull request's previous head can take the early exit for a docs-only push on top of real changes. The comment says unchanged only when both agree. Every review comment ends with a machine-readable HTML comment, ``, for readers that should not parse the prose or the diagram. The progress comment carries the platform link from the start, so a pull request can be opened there while the run is still going. diff --git a/scripts/action/build-review-artifact.sh b/scripts/action/build-review-artifact.sh index c278ff1..36a46a7 100755 --- a/scripts/action/build-review-artifact.sh +++ b/scripts/action/build-review-artifact.sh @@ -22,7 +22,7 @@ HEALTH_REPORT="$(dirname "$ANALYSIS_PATH")/health/health_report.json" # its early exit (no cluster or membership deltas, nothing re-detailed, no model consulted), # so the comment's "0 changed components" is a decided fact rather than a model's word. An # analysis written before the field existed reads as false, which is the safe direction. -UNCHANGED="$(jq -r 'if .metadata.incremental_unchanged == true then "true" else "false" end' "$ANALYSIS_PATH" 2>/dev/null || echo false)" +UNCHANGED="$(jq -r 'if .metadata.structure_unchanged == true then "true" else "false" end' "$ANALYSIS_PATH" 2>/dev/null || echo false)" # base_sha stays the event's base branch tip for consumers that key on it; # merge_base_sha records the commit the diagram actually compared against. @@ -42,12 +42,12 @@ jq -n \ --arg chain_depth "$CHAIN_DEPTH" \ --arg base_artifact "$BASE_ARTIFACT_NAME" \ --arg base_artifact_id "$BASE_ARTIFACT_ID" \ - --arg incremental_unchanged "$UNCHANGED" \ + --arg structure_unchanged "$UNCHANGED" \ '{kind: $kind, mode: $mode, base_sha: $base_sha, merge_base_sha: $merge_base_sha, pr_base_sha: $merge_base_sha, merge_base_resolved: $merge_base_resolved, head_sha: $head_sha, pr_number: $pr_number, seed_source: $seed_source, chain_depth: $chain_depth, base_artifact: $base_artifact, base_artifact_id: $base_artifact_id, - incremental_unchanged: $incremental_unchanged}' \ + structure_unchanged: $structure_unchanged}' \ > "${RUNNER_TEMP}/cb-review-artifact/metadata.json" echo "artifact_dir=${RUNNER_TEMP}/cb-review-artifact" >> "$GITHUB_OUTPUT" echo "unchanged=$UNCHANGED" >> "$GITHUB_OUTPUT" diff --git a/tests/test_review_artifact_unchanged.py b/tests/test_review_artifact_unchanged.py index 1c02b35..e1739ea 100644 --- a/tests/test_review_artifact_unchanged.py +++ b/tests/test_review_artifact_unchanged.py @@ -54,19 +54,19 @@ def _build(root: Path, analysis: str) -> tuple[dict, dict]: class ReviewArtifactUnchangedTests(unittest.TestCase): def test_the_early_exit_flag_is_relayed_to_the_metadata_and_the_comment_step(self) -> None: with tempfile.TemporaryDirectory() as tmp: - metadata, outputs = _build(Path(tmp), '{"metadata": {"incremental_unchanged": true}, "components": []}') + metadata, outputs = _build(Path(tmp), '{"metadata": {"structure_unchanged": true}, "components": []}') # A string, like every other `--arg` field the webview reads from this file. - self.assertEqual(metadata["incremental_unchanged"], "true") + self.assertEqual(metadata["structure_unchanged"], "true") self.assertEqual(outputs["unchanged"], "true") def test_a_re_detailed_run_and_a_pre_flag_analysis_both_read_as_not_unchanged(self) -> None: with tempfile.TemporaryDirectory() as tmp: - metadata, outputs = _build(Path(tmp), '{"metadata": {"incremental_unchanged": false}, "components": []}') - self.assertEqual(metadata["incremental_unchanged"], "false") + metadata, outputs = _build(Path(tmp), '{"metadata": {"structure_unchanged": false}, "components": []}') + self.assertEqual(metadata["structure_unchanged"], "false") self.assertEqual(outputs["unchanged"], "false") with tempfile.TemporaryDirectory() as tmp: metadata, outputs = _build(Path(tmp), '{"components": ["head"]}') - self.assertEqual(metadata["incremental_unchanged"], "false") + self.assertEqual(metadata["structure_unchanged"], "false") self.assertEqual(outputs["unchanged"], "false") From 44ab29791c19a561e89fbc32ce88fbe98aa7ecab Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Tue, 22 Sep 2026 23:39:40 +0200 Subject: [PATCH 5/5] feat(review): decide "no analysed file changed" from the two analyses' file hashes The engine records a whole-file content hash for every file a component owns. The render step already holds the base and head analyses, so it counts the analysed files whose hash differs, including added and removed ones, and reports the count next to the component count. At zero no byte of analysed code changed, and the comment says so; a non-zero component count at the same time is the analysis grouping the same code differently, and the comment says that too. This replaces relaying an engine flag (CodeBoarding #608, now closed). The flag compared a run against its own seed, so a re-run seeded from the pull request's previous head, a body-only edit and clustering noise each made it disagree with what the pull request did. The hash count compares against the merge base, needs no engine change and no engine release, and refuses to count ("unknown") when an analysis has a file without a hash. The machine-readable line carries it as analysed_files_changed=, and the review artifact's metadata as analysed_files_changed. Co-Authored-By: Claude Opus 5.5 (1M context) --- README.md | 4 +- action.yml | 6 +- scripts/action/build-review-artifact.sh | 11 +--- scripts/action/build-review-comment.sh | 24 ++++---- scripts/action/render-review.sh | 3 +- scripts/diff_to_mermaid.py | 58 +++++++++++++------ tests/test_diff_to_mermaid.py | 37 ++++++++++++ ...ed.py => test_review_artifact_metadata.py} | 28 ++++----- tests/test_review_comment.py | 30 +++++----- 9 files changed, 127 insertions(+), 74 deletions(-) rename tests/{test_review_artifact_unchanged.py => test_review_artifact_metadata.py} (60%) diff --git a/README.md b/README.md index 41ddb86..8f13b94 100644 --- a/README.md +++ b/README.md @@ -100,9 +100,9 @@ Fork pull requests never carry an analysis forward. They are reviewed on request ### Pull requests that change nothing analysed -A pull request whose changed files are all outside what the engine analyses (docs, configuration, CI, tests the ignore file excludes, or a language the engine does not read) cannot have moved the architecture, and the engine decides that itself: its incremental run finds no cluster or membership deltas, rewrites the baseline as the head without re-detailing, consults no model, and checks after finalisation that the saved components, membership and relations still equal the baseline's. The run still costs its setup (checkout, install, base fetch, static analysis), which is a minute or two, but no tokens. The engine records that decision in the analysis metadata (`structure_unchanged: true`), and the action relays it: when the diff also found nothing, the comment says `0 changed components (nothing analysed changed)`, so the zero is known to be decided rather than reported by a model, and the review artifact's `metadata.json` carries `structure_unchanged` (a string, like every other field there). The engine's flag alone is not the verdict: a body-only edit keeps the clusters while moving method hashes, which the diff counts as a modified component, and a run seeded from the pull request's previous head can take the early exit for a docs-only push on top of real changes. The comment says unchanged only when both agree. +A pull request whose changed files are all outside what the engine analyses (docs, configuration, CI, tests the ignore file excludes, or a language the engine does not read) cannot have moved the architecture. The action says so by comparing the two analyses it already holds: the engine records a content hash for every file a component owns, and the review counts the analysed files whose hash differs between the base and the head, including files added or removed. At zero, no byte of analysed code changed, and the comment says `(no analysed file changed)` after the component count. If the count of changed components is not zero at the same time, the analysis grouped the same code differently, and the comment says that too rather than presenting it as a change. The review artifact's `metadata.json` carries the count as `analysed_files_changed` (a string, like every other field there, and `unknown` when either analysis has a file without a hash). -Every review comment ends with a machine-readable HTML comment, ``, for readers that should not parse the prose or the diagram. The progress comment carries the platform link from the start, so a pull request can be opened there while the run is still going. +Every review comment ends with a machine-readable HTML comment, ``, for readers that should not parse the prose or the diagram. The progress comment carries the platform link from the start, so a pull request can be opened there while the run is still going. ## Authentication and providers diff --git a/action.yml b/action.yml index d435ba0..a371995 100644 --- a/action.yml +++ b/action.yml @@ -596,6 +596,7 @@ runs: PR_NUMBER: ${{ steps.guard.outputs.pr_number }} SEED_SOURCE: ${{ steps.review_analyze.outputs.seed_source }} CHAIN_DEPTH: ${{ steps.review_analyze.outputs.chain_depth }} + ANALYSED_FILES_CHANGED: ${{ steps.review_render.outputs.analysed_files_changed }} run: "$GITHUB_ACTION_PATH/scripts/action/build-review-artifact.sh" - name: Upload review artifact @@ -625,9 +626,8 @@ runs: BASE_REF: ${{ steps.guard.outputs.base_ref }} MERGE_BASE_RESOLVED: ${{ steps.guard.outputs.merge_base_resolved }} HEAD_SHA: ${{ steps.guard.outputs.head_sha }} - # The engine's own word, relayed by the artifact step: true when the incremental took its - # early exit and nothing was re-detailed, so the comment's zero is known to be decided. - UNCHANGED: ${{ steps.review_artifact.outputs.unchanged }} + # Analysed files whose content hash differs between base and head, from the render step. + ANALYSED_FILES_CHANGED: ${{ steps.review_render.outputs.analysed_files_changed }} run: "$GITHUB_ACTION_PATH/scripts/action/build-review-comment.sh" - name: Post review comment diff --git a/scripts/action/build-review-artifact.sh b/scripts/action/build-review-artifact.sh index 36a46a7..acdf52a 100755 --- a/scripts/action/build-review-artifact.sh +++ b/scripts/action/build-review-artifact.sh @@ -18,12 +18,6 @@ fi HEALTH_REPORT="$(dirname "$ANALYSIS_PATH")/health/health_report.json" [ ! -f "$HEALTH_REPORT" ] || cp "$HEALTH_REPORT" "${RUNNER_TEMP}/cb-review-artifact/health_report.json" -# The engine's own verdict, read from the analysis it wrote: true when the incremental took -# its early exit (no cluster or membership deltas, nothing re-detailed, no model consulted), -# so the comment's "0 changed components" is a decided fact rather than a model's word. An -# analysis written before the field existed reads as false, which is the safe direction. -UNCHANGED="$(jq -r 'if .metadata.structure_unchanged == true then "true" else "false" end' "$ANALYSIS_PATH" 2>/dev/null || echo false)" - # base_sha stays the event's base branch tip for consumers that key on it; # merge_base_sha records the commit the diagram actually compared against. # pr_base_sha carries the same value under the name the webview already reads: @@ -42,12 +36,11 @@ jq -n \ --arg chain_depth "$CHAIN_DEPTH" \ --arg base_artifact "$BASE_ARTIFACT_NAME" \ --arg base_artifact_id "$BASE_ARTIFACT_ID" \ - --arg structure_unchanged "$UNCHANGED" \ + --arg analysed_files_changed "${ANALYSED_FILES_CHANGED:-unknown}" \ '{kind: $kind, mode: $mode, base_sha: $base_sha, merge_base_sha: $merge_base_sha, pr_base_sha: $merge_base_sha, merge_base_resolved: $merge_base_resolved, head_sha: $head_sha, pr_number: $pr_number, seed_source: $seed_source, chain_depth: $chain_depth, base_artifact: $base_artifact, base_artifact_id: $base_artifact_id, - structure_unchanged: $structure_unchanged}' \ + analysed_files_changed: $analysed_files_changed}' \ > "${RUNNER_TEMP}/cb-review-artifact/metadata.json" echo "artifact_dir=${RUNNER_TEMP}/cb-review-artifact" >> "$GITHUB_OUTPUT" -echo "unchanged=$UNCHANGED" >> "$GITHUB_OUTPUT" diff --git a/scripts/action/build-review-comment.sh b/scripts/action/build-review-comment.sh index 1383133..96743a6 100755 --- a/scripts/action/build-review-comment.sh +++ b/scripts/action/build-review-comment.sh @@ -18,20 +18,18 @@ PLATFORM_URL="https://app.codeboarding.org/${GITHUB_REPOSITORY}/pull/${PR_NUMBER WEBVIEW_URL="${PLATFORM_URL}?utm_source=github&utm_medium=pr_comment&utm_campaign=gh_action" BODY="${RUNNER_TEMP}/review-comment.md" # The status line is what the web platform reads the count from, so its shape is a contract. -# When the engine's incremental took its early exit AND the diff found nothing, the same line -# says so: that zero was decided (no cluster or membership deltas, no model consulted), not -# reported by a diff of two re-detailed graphs. Both halves are needed. The engine's flag alone -# says the clusters held; a body-only edit still moves method hashes, which the diff counts as -# a modified component, and a run seeded from this pull request's previous head can take the -# early exit for a docs-only push on top of real changes. So the verdict is the engine's word -# and the diff's zero together, and neither alone. +# ANALYSED_FILES_CHANGED counts the analysed files whose content hash differs between base and +# head ("unknown" when the analyses cannot say). At zero no analysed code changed, so the status +# says so, and a non-zero component count is the analysis grouping the same code differently. +ANALYSED_FILES_CHANGED="${ANALYSED_FILES_CHANGED:-unknown}" STATUS="${N_CHANGED} changed ${COMPONENT_NOUN}" -VERDICT_UNCHANGED=false -if [ "${UNCHANGED:-false}" = true ] && [ "$N_CHANGED" = "0" ]; then - VERDICT_UNCHANGED=true - STATUS="${STATUS} (nothing analysed changed)" +if [ "$ANALYSED_FILES_CHANGED" = "0" ]; then + STATUS="${STATUS} (no analysed file changed)" fi printf '### CodeBoarding review\n\n**Status:** %s\n' "$STATUS" > "$BODY" +if [ "$ANALYSED_FILES_CHANGED" = "0" ] && [ "$N_CHANGED" != "0" ]; then + printf '\nNo file CodeBoarding analyses changed in this pull request, so the components marked below differ only because the analysis grouped the same code differently.\n' >> "$BODY" +fi printf '\nSee the full change in [CodeBoarding](%s).\n' "$WEBVIEW_URL" >> "$BODY" # The diagram compares against the merge base, so commits landed on the base # branch since this PR forked are excluded. Say so rather than hide it. @@ -58,7 +56,7 @@ fi # The machine-readable line: what a reader of the comment (the web platform's dashboard, an # agent) needs without parsing the prose or the diagram. An HTML comment renders as nothing. # Keep it one line, `key=value` pairs, values without spaces, so a regex over it stays trivial. - printf '\n' \ - "$PLATFORM_URL" "$N_CHANGED" "$VERDICT_UNCHANGED" "${HEAD_SHA:-}" + printf '\n' \ + "$PLATFORM_URL" "$N_CHANGED" "$ANALYSED_FILES_CHANGED" "${HEAD_SHA:-}" } >> "$BODY" echo "path=$BODY" >> "$GITHUB_OUTPUT" diff --git a/scripts/action/render-review.sh b/scripts/action/render-review.sh index 0b591d0..d6a1d3c 100755 --- a/scripts/action/render-review.sh +++ b/scripts/action/render-review.sh @@ -16,10 +16,11 @@ META="${RUNNER_TEMP}/diagram_meta.json" DIFF="$(python3 "$ACTION_PATH/scripts/diff_to_mermaid.py" --base "$BASE_ANALYSIS" --head "$HEAD_ANALYSIS" --out "$DIAGRAM_OUT" --direction LR --render-depth 1)" printf '%s' "$DIFF" > "$META" -read -r N_CHANGED TRUNCATED RENDERED EMPTY < <(jq -r '[.n_changed, .truncated, .rendered, .empty] | @tsv' "$META") +read -r N_CHANGED TRUNCATED RENDERED EMPTY ANALYSED_FILES_CHANGED < <(jq -r '[.n_changed, .truncated, .rendered, .empty, (.analysed_files_changed // "unknown")] | @tsv' "$META") [ "$RENDERED" = true ] || [ "$EMPTY" = true ] || { echo "::error::The architecture diff is too large to render."; exit 1; } { echo "diagram_md=$DIAGRAM_OUT" echo "n_changed=$N_CHANGED" echo "truncated=$TRUNCATED" + echo "analysed_files_changed=$ANALYSED_FILES_CHANGED" } >> "$GITHUB_OUTPUT" diff --git a/scripts/diff_to_mermaid.py b/scripts/diff_to_mermaid.py index 835cbcf..4d8283d 100644 --- a/scripts/diff_to_mermaid.py +++ b/scripts/diff_to_mermaid.py @@ -15,6 +15,11 @@ ``git show``), and a relation whose ``(src, dst)`` is unchanged but whose label text changed is reported as ``modified`` (the original only did added/deleted). +It also counts the analysed files that differ between the two sides, from the whole-file +content hash the engine records for every file a component owns. Zero means no byte of +analysed code changed, whatever the component diff says: a component can still read as +changed when the analysis grouped the same code differently. + Self-contained stdlib. """ @@ -144,14 +149,39 @@ def _has_method_changes(base: dict, current: dict) -> bool: ) -def _analysis_changes(base: dict, current: dict) -> tuple[set[str], set[str]]: - """Return changed method keys and changed files without method-level detail.""" +def _content_changed(before: dict | None, after: dict | None) -> bool: + if before is None or after is None: + return before != after + return before.get("content_hash") != after.get("content_hash") + + +def _changed_files(base: dict, current: dict) -> set[str]: + base_files = base.get("files") or {} + current_files = current.get("files") or {} + return { + path + for path in set(base_files) | set(current_files) + if _content_changed(base_files.get(path), current_files.get(path)) + } - def content_changed(before: dict | None, after: dict | None) -> bool: - if before is None or after is None: - return before != after - return before.get("content_hash") != after.get("content_hash") +def analysed_files_changed(base: dict, current: dict) -> int | None: + """How many analysed files were added, removed or edited between the two analyses. + + None when either side cannot vouch for it: no file index, or a file indexed without a hash, + which would make an edit to it indistinguishable from no edit. + """ + base_files = base.get("files") or {} + current_files = current.get("files") or {} + if not base_files or not current_files: + return None + if any(not (entry or {}).get("content_hash") for entry in [*base_files.values(), *current_files.values()]): + return None + return len(_changed_files(base, current)) + + +def _analysis_changes(base: dict, current: dict) -> tuple[set[str], set[str]]: + """Return changed method keys and changed files without method-level detail.""" base_index = base.get("methods_index") or {} current_index = current.get("methods_index") or {} changed_methods: set[str] = set() @@ -159,20 +189,12 @@ def content_changed(before: dict | None, after: dict | None) -> bool: for key in set(base_index) | set(current_index): before = base_index.get(key) after = current_index.get(key) - if not content_changed(before, after): + if not _content_changed(before, after): continue record = after or before or {} changed_methods.add(key) member_files.add(record.get("file_path") or key.partition("|")[0]) - - base_files = base.get("files") or {} - current_files = current.get("files") or {} - changed_files = { - path - for path in set(base_files) | set(current_files) - if content_changed(base_files.get(path), current_files.get(path)) - } - return changed_methods, changed_files - member_files + return changed_methods, _changed_files(base, current) - member_files def _owns_analysis_change( @@ -666,7 +688,8 @@ def main() -> int: p.add_argument("--rank-spacing", type=int, default=None, help="Space between ranks") args = p.parse_args() - diff = build_diff(load_analysis(args.base), load_analysis(args.head)) + base, head = load_analysis(args.base), load_analysis(args.head) + diff = build_diff(base, head) mermaid, meta = render_mermaid( diff, direction=args.direction, @@ -681,6 +704,7 @@ def main() -> int: args.out.write_text(mermaid if mermaid is not None else "", encoding="utf-8") meta["rendered"] = mermaid is not None + meta["analysed_files_changed"] = analysed_files_changed(base, head) # Machine-readable summary on stdout for the action to consume. print(json.dumps(meta)) return 0 diff --git a/tests/test_diff_to_mermaid.py b/tests/test_diff_to_mermaid.py index beec89d..0624787 100644 --- a/tests/test_diff_to_mermaid.py +++ b/tests/test_diff_to_mermaid.py @@ -1,5 +1,6 @@ """Unit tests for scripts/diff_to_mermaid.py — diff logic + Mermaid rendering.""" +import io import json import re import sys @@ -36,6 +37,42 @@ def linkstyle_indices_in_range(text): return all(i < n_edges for i in idxs) +class TestAnalysedFilesChanged(unittest.TestCase): + @staticmethod + def analysis(files, components=None): + return { + "files": {path: {"content_hash": digest} for path, digest in files.items()}, + "components": components or [], + } + + def test_the_same_code_grouped_differently_changes_no_file(self): + base = self.analysis({"a.py": "h1", "b.py": "h2"}, [comp("A", {"a.py": ["f"]}), comp("B", {"b.py": ["g"]})]) + head = self.analysis({"a.py": "h1", "b.py": "h2"}, [comp("A", {"a.py": ["f"], "b.py": ["g"]})]) + self.assertEqual(dm.analysed_files_changed(base, head), 0) + + def test_every_edited_added_or_removed_file_counts(self): + base = self.analysis({"a.py": "h1", "b.py": "h2", "gone.py": "h3"}) + head = self.analysis({"a.py": "h1", "b.py": "h2-edited", "new.py": "h4"}) + self.assertEqual(dm.analysed_files_changed(base, head), 3) + + def test_an_analysis_that_cannot_vouch_for_its_files_gives_no_count(self): + full = self.analysis({"a.py": "h1"}) + self.assertIsNone(dm.analysed_files_changed({"components": []}, full)) + self.assertIsNone(dm.analysed_files_changed(full, self.analysis({"a.py": ""}))) + + def test_the_count_rides_on_the_machine_readable_summary(self): + loaded = {"base.json": self.analysis({"a.py": "h1"}), "head.json": self.analysis({"a.py": "h2"})} + with tempfile.TemporaryDirectory() as tmp: + argv = ["diff_to_mermaid.py", "--base", "base.json", "--head", "head.json", "--out", f"{tmp}/diagram.md"] + with ( + patch.object(sys, "argv", argv), + patch.object(dm, "load_analysis", side_effect=lambda path: loaded[str(path)]), + patch("sys.stdout", new_callable=io.StringIO) as stdout, + ): + dm.main() + self.assertEqual(json.loads(stdout.getvalue())["analysed_files_changed"], 1) + + class TestDiff(unittest.TestCase): def test_core_loader_projects_global_relation_into_rendered_mermaid(self): root_a = SimpleNamespace(component_id="1") diff --git a/tests/test_review_artifact_unchanged.py b/tests/test_review_artifact_metadata.py similarity index 60% rename from tests/test_review_artifact_unchanged.py rename to tests/test_review_artifact_metadata.py index e1739ea..6b43526 100644 --- a/tests/test_review_artifact_unchanged.py +++ b/tests/test_review_artifact_metadata.py @@ -1,4 +1,4 @@ -"""The artifact relays the engine's early-exit verdict, and reads a pre-flag analysis as not unchanged.""" +"""The review artifact's metadata records how many analysed files changed, as the render step counted them.""" import json import os @@ -11,9 +11,9 @@ BUILD_ARTIFACT = ROOT / "scripts" / "action" / "build-review-artifact.sh" -def _build(root: Path, analysis: str) -> tuple[dict, dict]: +def _build(root: Path, **extra: str) -> tuple[dict, dict]: head = root / "head.json" - head.write_text(analysis, encoding="utf-8") + head.write_text('{"components": ["head"]}', encoding="utf-8") base = root / "base.json" base.write_text('{"components": ["base"]}', encoding="utf-8") output = root / "github-output" @@ -37,6 +37,7 @@ def _build(root: Path, analysis: str) -> tuple[dict, dict]: "PR_NUMBER": "81", "SEED_SOURCE": "pr-chain", "CHAIN_DEPTH": "2", + **extra, }, capture_output=True, text=True, @@ -51,23 +52,18 @@ def _build(root: Path, analysis: str) -> tuple[dict, dict]: return metadata, outputs -class ReviewArtifactUnchangedTests(unittest.TestCase): - def test_the_early_exit_flag_is_relayed_to_the_metadata_and_the_comment_step(self) -> None: +class ReviewArtifactMetadataTests(unittest.TestCase): + def test_the_analysed_file_count_is_recorded_as_counted(self) -> None: with tempfile.TemporaryDirectory() as tmp: - metadata, outputs = _build(Path(tmp), '{"metadata": {"structure_unchanged": true}, "components": []}') + metadata, outputs = _build(Path(tmp), ANALYSED_FILES_CHANGED="0") # A string, like every other `--arg` field the webview reads from this file. - self.assertEqual(metadata["structure_unchanged"], "true") - self.assertEqual(outputs["unchanged"], "true") + self.assertEqual(metadata["analysed_files_changed"], "0") + self.assertEqual(set(outputs), {"artifact_dir"}) - def test_a_re_detailed_run_and_a_pre_flag_analysis_both_read_as_not_unchanged(self) -> None: + def test_a_count_the_render_step_could_not_make_is_recorded_as_unknown(self) -> None: with tempfile.TemporaryDirectory() as tmp: - metadata, outputs = _build(Path(tmp), '{"metadata": {"structure_unchanged": false}, "components": []}') - self.assertEqual(metadata["structure_unchanged"], "false") - self.assertEqual(outputs["unchanged"], "false") - with tempfile.TemporaryDirectory() as tmp: - metadata, outputs = _build(Path(tmp), '{"components": ["head"]}') - self.assertEqual(metadata["structure_unchanged"], "false") - self.assertEqual(outputs["unchanged"], "false") + metadata, _outputs = _build(Path(tmp)) + self.assertEqual(metadata["analysed_files_changed"], "unknown") if __name__ == "__main__": diff --git a/tests/test_review_comment.py b/tests/test_review_comment.py index 5445cf8..709f693 100644 --- a/tests/test_review_comment.py +++ b/tests/test_review_comment.py @@ -54,28 +54,32 @@ def test_the_machine_readable_line_ends_the_body(self) -> None: last = body.rstrip("\n").splitlines()[-1] self.assertEqual( last, - "", + "", ) # The status regex the web platform uses must still find the status line, not the marker. match = re.search(r"\*\*Status:\*\*\s*(\d+)\s+changed\s+components?", body) self.assertIsNotNone(match) self.assertEqual(match.group(1) if match else None, "3") - def test_the_engines_early_exit_is_said_in_the_status_and_the_marker(self) -> None: + def test_no_analysed_file_changed_is_said_in_the_status_and_the_marker(self) -> None: with tempfile.TemporaryDirectory() as tmp: - body = _build(Path(tmp), N_CHANGED="0", UNCHANGED="true") - self.assertIn("**Status:** 0 changed components (nothing analysed changed)\n", body) - self.assertIn("changed=0 unchanged=true head=abc123", body) + body = _build(Path(tmp), N_CHANGED="0", ANALYSED_FILES_CHANGED="0") + self.assertIn("**Status:** 0 changed components (no analysed file changed)\n", body) + self.assertNotIn("grouped the same code", body) + self.assertIn("changed=0 analysed_files_changed=0 head=abc123", body) - def test_the_early_exit_alone_is_not_the_verdict(self) -> None: - # A run seeded from the pull request's previous head takes the early exit for a docs-only - # push on top of real changes, and a body-only edit keeps the clusters while moving method - # hashes. In both the diff is not zero, and the comment must not call it unchanged. + def test_components_that_differ_with_no_file_changed_are_called_regrouping(self) -> None: with tempfile.TemporaryDirectory() as tmp: - body = _build(Path(tmp), N_CHANGED="3", UNCHANGED="true") - self.assertIn("**Status:** 3 changed components\n", body) - self.assertNotIn("nothing analysed changed", body) - self.assertIn("changed=3 unchanged=false head=abc123", body) + body = _build(Path(tmp), N_CHANGED="3", ANALYSED_FILES_CHANGED="0") + self.assertIn("**Status:** 3 changed components (no analysed file changed)\n", body) + self.assertIn("grouped the same code differently", body) + self.assertIn("changed=3 analysed_files_changed=0 head=abc123", body) + + def test_a_changed_analysed_file_gets_no_verdict_even_at_zero_components(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + body = _build(Path(tmp), N_CHANGED="0", ANALYSED_FILES_CHANGED="2") + self.assertIn("**Status:** 0 changed components\n", body) + self.assertIn("changed=0 analysed_files_changed=2 head=abc123", body) if __name__ == "__main__":