diff --git a/README.md b/README.md index 7c53bf6..8f13b94 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 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. + ## 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..a371995 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 @@ -594,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 @@ -622,6 +625,9 @@ 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 }} + # 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 0dc5e06..acdf52a 100755 --- a/scripts/action/build-review-artifact.sh +++ b/scripts/action/build-review-artifact.sh @@ -36,9 +36,11 @@ jq -n \ --arg chain_depth "$CHAIN_DEPTH" \ --arg base_artifact "$BASE_ARTIFACT_NAME" \ --arg base_artifact_id "$BASE_ARTIFACT_ID" \ + --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}' \ + base_artifact: $base_artifact, base_artifact_id: $base_artifact_id, + analysed_files_changed: $analysed_files_changed}' \ > "${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..96743a6 100755 --- a/scripts/action/build-review-comment.sh +++ b/scripts/action/build-review-comment.sh @@ -14,9 +14,22 @@ 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. +# 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}" +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. @@ -40,5 +53,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" "$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_metadata.py b/tests/test_review_artifact_metadata.py new file mode 100644 index 0000000..6b43526 --- /dev/null +++ b/tests/test_review_artifact_metadata.py @@ -0,0 +1,70 @@ +"""The review artifact's metadata records how many analysed files changed, as the render step counted them.""" + +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, **extra: str) -> tuple[dict, dict]: + head = root / "head.json" + head.write_text('{"components": ["head"]}', 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", + **extra, + }, + 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 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), ANALYSED_FILES_CHANGED="0") + # A string, like every other `--arg` field the webview reads from this file. + self.assertEqual(metadata["analysed_files_changed"], "0") + self.assertEqual(set(outputs), {"artifact_dir"}) + + 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)) + self.assertEqual(metadata["analysed_files_changed"], "unknown") + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_review_comment.py b/tests/test_review_comment.py new file mode 100644 index 0000000..709f693 --- /dev/null +++ b/tests/test_review_comment.py @@ -0,0 +1,86 @@ +"""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)) + 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_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", 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_components_that_differ_with_no_file_changed_are_called_regrouping(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + 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__": + unittest.main()