Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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, `<!-- codeboarding: platform_url=… changed=… analysed_files_changed=… head=… -->`, 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
Expand Down
6 changes: 6 additions & 0 deletions action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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.

<sub>run [${{ github.run_id }}](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}) · attempt ${{ github.run_attempt }}</sub>

- name: Checkout analysis target
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion scripts/action/build-review-artifact.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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"
22 changes: 20 additions & 2 deletions scripts/action/build-review-comment.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -40,5 +53,10 @@ fi
printf '[download artifacts](%s) · ' "$ARTIFACT_URL"
fi
printf 'run [%s](%s)</sub>\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 '<!-- codeboarding: platform_url=%s changed=%s analysed_files_changed=%s head=%s -->\n' \
"$PLATFORM_URL" "$N_CHANGED" "$ANALYSED_FILES_CHANGED" "${HEAD_SHA:-}"
} >> "$BODY"
echo "path=$BODY" >> "$GITHUB_OUTPUT"
3 changes: 2 additions & 1 deletion scripts/action/render-review.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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"
58 changes: 41 additions & 17 deletions scripts/diff_to_mermaid.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
"""

Expand Down Expand Up @@ -144,35 +149,52 @@ 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
Comment on lines +176 to +177

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Distinguish empty file indexes from missing indexes

When a PR deletes the last analysed source file, the head analysis legitimately has "files": {}, but this condition treats that empty index as unavailable and returns None. The artifact and comment therefore report analysed_files_changed=unknown instead of counting the removed files; similarly, a repository with no analysed files on either side never receives the zero-change verdict. Check whether the files field is absent or invalid rather than whether its mapping is empty.

Useful? React with 👍 / 👎.

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()
member_files: set[str] = set()
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(
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand Down
37 changes: 37 additions & 0 deletions tests/test_diff_to_mermaid.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Unit tests for scripts/diff_to_mermaid.py — diff logic + Mermaid rendering."""

import io
import json
import re
import sys
Expand Down Expand Up @@ -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")
Expand Down
70 changes: 70 additions & 0 deletions tests/test_review_artifact_metadata.py
Original file line number Diff line number Diff line change
@@ -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()
Loading
Loading