From 6bf27f00cc185501a42a6654fd624c43d533ba75 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Wed, 19 Aug 2026 00:04:08 +0200 Subject: [PATCH 1/2] docs(sync): say why the no-op commit guard filters two field names health_report.json rewrites a timestamp on every run, so the existing -I filter is what stops committing the health report from turning every sync into a commit. That was incidental before and load-bearing now. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/action/deliver-sync.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scripts/action/deliver-sync.sh b/scripts/action/deliver-sync.sh index f647855..bbc454c 100755 --- a/scripts/action/deliver-sync.sh +++ b/scripts/action/deliver-sync.sh @@ -86,6 +86,9 @@ if [ "$remote_sha" != "$BASE_SHA" ]; then exit 0 fi +# The -I filters keep a run that changed nothing from committing: analysis.json +# carries generated_at and health_report.json carries timestamp, both rewritten +# every run. Dropping either filter turns every sync into a commit. if git diff --cached --quiet || git diff --cached --quiet -I '"generated_at"' -I '"timestamp"'; then git reset -q close_stale_pr From e12d41cb427f60d80dbe625fb208b772f3e961b8 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Wed, 19 Aug 2026 00:04:09 +0200 Subject: [PATCH 2/2] fix: stop discarding the health report The engine runs health checks on every analysis, full or incremental, and writes health_report.json beside the graph it produced. Nothing kept it: sync deleted the file instead of installing the fresh one, and review never uploaded it. The only surviving copy was inside the Actions cache, which has no download API, so neither the extension nor the webview could read health information at all. It has been that way since the v2 rewrite in 474d40a, which removed the file as a v1 leftover without noticing that the engine still produced it. Sync now installs the report it just generated, and removes a stale one when a run produced none, matching how the other optional artifacts are handled. Review ships it in the artifact next to the analyses. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 8 +++-- docs/COMMIT_STRATEGY.md | 10 ++++-- scripts/action/build-review-artifact.sh | 5 +++ scripts/action/install-sync.sh | 21 ++++++++---- tests/test_action_cache.py | 26 ++++++++++++++- tests/test_action_sync.py | 44 +++++++++++++++++++++++++ 6 files changed, 100 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 3746fa6..e8310a6 100644 --- a/README.md +++ b/README.md @@ -60,9 +60,10 @@ The uploaded artifact holds both sides of the comparison, so a reader can reprod |---|---| | `analysis.json` | the head analysis, at `head_sha` | | `base_analysis.json` | the analysis it was compared against, at `merge_base_sha` | -| `metadata.json` | both SHAs, `merge_base_resolved`, `seed_source`, `chain_depth`, PR number | +| `health_report.json` | the head's health findings, when the engine produced any | +| `metadata.json` | which commits those graphs describe — see [the field list](docs/COMMIT_STRATEGY.md#names) | -Artifacts are kept 30 days. Reading the default branch's committed baseline instead of `base_analysis.json` would drift from the merge base in exactly the way described above. +The action requests 30-day retention; a repository or organisation policy can shorten it, so treat an artifact's own `expired` flag as the truth rather than any fixed window. Reading the default branch's committed baseline instead of `base_analysis.json` would drift from the merge base in exactly the way described above. ### Reused analysis @@ -127,8 +128,9 @@ Sync mode commits only Core's persisted incremental-analysis state under `.codeb - `static_analysis.pkl` - `static_analysis.sha` - `codeboarding_version.json` when emitted by Core +- `health/health_report.json` -It does **not** render or commit architecture Markdown. Existing v1-generated `.codeboarding/*.md` and `docs/development/architecture.md` files carrying CodeBoarding's generated badge, plus `.codeboarding/health/health_report.json`, are removed on the first v2 sync. Hand-written Markdown and user-authored CodeBoarding configuration are preserved. +It does **not** render or commit architecture Markdown. Existing v1-generated `.codeboarding/*.md` and `docs/development/architecture.md` files carrying CodeBoarding's generated badge are removed on the first v2 sync. Hand-written Markdown and user-authored CodeBoarding configuration, including `health/health_config.json` and `health/.healthignore`, are preserved. Create `.github/workflows/codeboarding-sync.yml`: diff --git a/docs/COMMIT_STRATEGY.md b/docs/COMMIT_STRATEGY.md index f0b37f3..48acb8a 100644 --- a/docs/COMMIT_STRATEGY.md +++ b/docs/COMMIT_STRATEGY.md @@ -27,10 +27,13 @@ branch, so generated files cannot conflict during a merge. | `static_analysis.pkl` | LSP/CFG cache and the cluster baseline incremental needs | | `static_analysis.sha` | the warm-start gate for the pickle | | `codeboarding_version.json` | when Core emits it | +| `health/health_report.json` | the warnings, read without regenerating them | -Everything else generated is removed on sync: v1 architecture Markdown and -`health/health_report.json`. Hand-written Markdown and user configuration -(`.codeboardingignore`, health config) are preserved. +The engine writes a health report on every run, full or incremental, so sync +installs the fresh one and drops a stale one when a run produced none. v1 +architecture Markdown is removed. Hand-written Markdown and user configuration +(`.codeboardingignore`, `health/health_config.json`, `health/.healthignore`) are +preserved. **Delivery (`sync_strategy`).** The committed set is identical either way; only how it reaches the branch differs. `push` fast-forwards it directly. @@ -100,6 +103,7 @@ lists a pull request's artifacts and takes the newest. codeboarding-review--/ analysis.json the head analysis, at metadata.head_sha base_analysis.json what it was compared against, at metadata.merge_base_sha + health_report.json the head's health findings, when the engine wrote one metadata.json which commits those graphs describe | `metadata.json` field | Meaning | diff --git a/scripts/action/build-review-artifact.sh b/scripts/action/build-review-artifact.sh index 014dc5a..5c85c98 100755 --- a/scripts/action/build-review-artifact.sh +++ b/scripts/action/build-review-artifact.sh @@ -8,6 +8,11 @@ cp "$ANALYSIS_PATH" "${RUNNER_TEMP}/cb-review-artifact/analysis.json" # branch's committed baseline instead would drift from the merge base exactly as # the review itself used to. cp "$BASE_ANALYSIS_PATH" "${RUNNER_TEMP}/cb-review-artifact/base_analysis.json" +# The engine writes this next to the analysis it came from. Ship it when present +# so a pull request's warnings are readable without rerunning the analysis. +HEALTH_REPORT="$(dirname "$ANALYSIS_PATH")/health/health_report.json" +[ ! -f "$HEALTH_REPORT" ] || cp "$HEALTH_REPORT" "${RUNNER_TEMP}/cb-review-artifact/health_report.json" + # 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: diff --git a/scripts/action/install-sync.sh b/scripts/action/install-sync.sh index b5b7725..47c81c3 100755 --- a/scripts/action/install-sync.sh +++ b/scripts/action/install-sync.sh @@ -40,7 +40,20 @@ for name in "${artifacts[@]}"; do printf '%s\n' "$target" done -# Remove identifiable v1 Markdown and its generated health report. +# The engine writes a health report on every run, full or incremental. Install it +# beside the analysis so the extension and webview can read warnings without +# regenerating, and remove a stale one when a run produced none. +health_source="$ANALYSIS_DIR/health/health_report.json" +health_target="$output/health/health_report.json" +if [ -f "$health_source" ]; then + mkdir -p "$output/health" + cp "$health_source" "$health_target" +elif [ -e "$health_target" ]; then + rm -f "$health_target" +fi +printf '%s\n' "$health_target" + +# Remove identifiable v1 Markdown. marker='https://img.shields.io/badge/Generated%20by-CodeBoarding' for legacy in "$output"/*.md "$CHECKOUT_DIR/docs/development/architecture.md"; do [ -f "$legacy" ] || continue @@ -48,10 +61,4 @@ for legacy in "$output"/*.md "$CHECKOUT_DIR/docs/development/architecture.md"; d rm -f "$legacy" printf '%s\n' "$legacy" done -legacy="$output/health/health_report.json" -if [ -e "$legacy" ]; then - rm -f "$legacy" - printf '%s\n' "$legacy" -fi - echo "installed=$installed" >> "$GITHUB_OUTPUT" diff --git a/tests/test_action_cache.py b/tests/test_action_cache.py index f7a7fa3..e1d2f71 100644 --- a/tests/test_action_cache.py +++ b/tests/test_action_cache.py @@ -337,7 +337,7 @@ def setUp(self) -> None: def tearDown(self) -> None: self.temp_dir.cleanup() - def test_it_ships_both_graphs_and_the_commit_they_describe(self) -> None: + def _build(self) -> subprocess.CompletedProcess: output = self.root / "github-output" result = subprocess.run( [str(ROOT / "scripts" / "action" / "build-review-artifact.sh")], @@ -361,6 +361,10 @@ def test_it_ships_both_graphs_and_the_commit_they_describe(self) -> None: check=False, ) self.assertEqual(result.returncode, 0, result.stderr or result.stdout) + return result + + def test_it_ships_both_graphs_and_the_commit_they_describe(self) -> None: + self._build() artifact = self.root / "cb-review-artifact" self.assertEqual(json.loads((artifact / "analysis.json").read_text())["components"], ["head"]) @@ -379,6 +383,26 @@ def test_it_ships_both_graphs_and_the_commit_they_describe(self) -> None: self.assertEqual(metadata["seed_source"], "pr-chain") +class ReviewHealthArtifactTests(ReviewArtifactTests): + """The engine writes a health report beside every analysis it produces.""" + + def test_it_ships_the_health_report_when_the_engine_wrote_one(self) -> None: + (self.root / "health").mkdir() + (self.root / "health" / "health_report.json").write_text('{"overall_score": 0.9}', encoding="utf-8") + + self._build() + + report = self.root / "cb-review-artifact" / "health_report.json" + self.assertEqual(report.read_text(encoding="utf-8"), '{"overall_score": 0.9}') + + def test_it_still_builds_when_no_health_report_was_written(self) -> None: + self._build() + + artifact = self.root / "cb-review-artifact" + self.assertTrue((artifact / "analysis.json").is_file()) + self.assertFalse((artifact / "health_report.json").exists()) + + class CachePathParityTests(unittest.TestCase): """actions/cache derives its lookup version from the path strings, so a save under a different path than the restore can never be found again.""" diff --git a/tests/test_action_sync.py b/tests/test_action_sync.py index 073f494..b88e39f 100644 --- a/tests/test_action_sync.py +++ b/tests/test_action_sync.py @@ -244,6 +244,50 @@ def test_installs_core_manifest_and_preserves_user_configuration(self) -> None: self.assertEqual(result.returncode, 0, result.stderr or result.stdout) self.assertEqual(architecture.read_text(encoding="utf-8"), "hand-written architecture\n") + def test_installs_the_health_report_the_engine_produced(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + checkout = root / "checkout" + analysis = root / "analysis" + fake_core = root / "core" + static_analyzer = fake_core / "static_analyzer" + for directory in (checkout / ".codeboarding", analysis / "health", static_analyzer): + directory.mkdir(parents=True) + (fake_core / "utils.py").write_text( + "ANALYSIS_FILENAME = 'analysis.json'\nFINGERPRINT_FILENAME = 'fingerprint.json'\n", + encoding="utf-8", + ) + (static_analyzer / "__init__.py").touch() + (static_analyzer / "analysis_cache.py").write_text( + "STATIC_ANALYSIS_PKL = 'static_analysis.pkl'\nSTATIC_ANALYSIS_SHA = 'static_analysis.sha'\n", + encoding="utf-8", + ) + (analysis / "analysis.json").write_text("{}\n", encoding="utf-8") + (analysis / "health" / "health_report.json").write_text('{"overall_score": 1.0}', encoding="utf-8") + + result = subprocess.run( + [str(INSTALL_SYNC)], + cwd=checkout, + env={ + "PATH": os.environ["PATH"], + "PYTHONPATH": str(fake_core), + "ACTION_PATH": str(ROOT), + "ANALYSIS_DIR": str(analysis), + "CHECKOUT_DIR": str(checkout), + "GITHUB_OUTPUT": str(root / "github-output"), + }, + capture_output=True, + text=True, + check=False, + ) + + self.assertEqual(result.returncode, 0, result.stderr or result.stdout) + installed = checkout / ".codeboarding" / "health" / "health_report.json" + self.assertEqual(installed.read_text(encoding="utf-8"), '{"overall_score": 1.0}') + # Printed paths are what the delivery step stages, so an installed + # file that is never printed is silently left out of the commit. + self.assertIn(str(installed), result.stdout.splitlines()) + def test_empty_review_is_successful(self) -> None: with tempfile.TemporaryDirectory() as tmp: root = Path(tmp)