From d16baf917e533612e1050db2f0760af0fa13e630 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Thu, 20 Aug 2026 00:08:17 +0200 Subject: [PATCH] fix(review): publish state, not scratch, and a boolean that is one Two reports from the webview, both correct. Every metadata value was a JSON string, because jq --arg produces one. So merge_base_resolved shipped as "false", which is truthy in a consumer, and the caveat banner keyed on it never fired from the day it was written. It is now a real boolean; the rest stay strings, which is what readers already parse, and the types are written down so the next field does not repeat it. Published bundles carried the engine's working area: a run log, two empty lock files, all inflated by nobody and paid for on every fetch. On a small repository the log alone was a fifth of the bundle, and it grows with the analysis. Logs and lock files are now stripped when a bundle is staged. Health configuration stays, because a run seeded from a bundle reads it. Co-Authored-By: Claude Opus 5 (1M context) --- docs/COMMIT_STRATEGY.md | 30 ++++++++++++++++--------- scripts/action/analyze.sh | 5 +++++ scripts/action/build-review-artifact.sh | 2 +- tests/test_action_state.py | 21 ++++++++++++++++- 4 files changed, 46 insertions(+), 12 deletions(-) diff --git a/docs/COMMIT_STRATEGY.md b/docs/COMMIT_STRATEGY.md index a7913f0..38edb80 100644 --- a/docs/COMMIT_STRATEGY.md +++ b/docs/COMMIT_STRATEGY.md @@ -31,6 +31,11 @@ cache was free. Retention is the lever — see below. | `codeboarding-base--` | the merge base's own analysis | 30 days, renewed while still referenced | any later review forking from that commit | | `codeboarding-warmstart--pr` | the working directory: graph, pickle, fingerprint, gate | **1 day**, configurable | only the next run of that pull request | +Bundles carry analysis state, not the engine's scratch: run logs and lock files +are stripped before publication, since no reader inflates them and every fetch +pays for them. Health configuration stays, because a run seeded from a bundle +reads it. + The base graph is published only by the run that *computed* it, so it is written about once per merge base rather than once per run — with two exceptions. A review artifact references a base by id for its whole retention, so a base about @@ -51,16 +56,21 @@ the payload. `metadata.json` in the review artifact names the base artifact so a reader can fetch it without reconstructing the name: -| Field | Meaning | -|---|---| -| `head_sha` | the commit `analysis.json` describes | -| `pr_base_sha` | the merge base, under the name the webview resolves | -| `merge_base_sha` | the same value under this action's own name | -| `base_artifact` | the artifact holding the graph that was compared against | -| `base_artifact_id` | **which one**, since two artifacts can share that name and disagree: the engine is not deterministic, and a sync run publishes bases for the same commit | -| `merge_base_resolved` | `false` means the merge base could not be resolved, so the comparison is against `base_sha` | -| `base_sha` | the base branch tip when the event fired — *not* what was compared against | -| `pr_number`, `mode`, `seed_source`, `chain_depth` | provenance; nothing rendering a diagram needs them | +Types are part of the contract, not an accident of how the file is written: +`merge_base_resolved` is a JSON **boolean**, everything else is a string. A +string `"false"` is truthy in most consumers, so a caveat keyed on it silently +never fires. + +| Field | Type | Meaning | +|---|---|---| +| `head_sha` | string | the commit `analysis.json` describes | +| `pr_base_sha` | string | the merge base, under the name the webview resolves | +| `merge_base_sha` | string | the same value under this action's own name | +| `base_artifact` | string | the artifact holding the graph that was compared against | +| `base_artifact_id` | string | **which one**, since two artifacts can share that name and disagree: the engine is not deterministic, and a sync run publishes bases for the same commit | +| `merge_base_resolved` | **boolean** | `false` means the merge base could not be resolved, so the comparison is against `base_sha` | +| `base_sha` | string | the base branch tip when the event fired — *not* what was compared against | +| `pr_number`, `mode`, `seed_source`, `chain_depth` | string | provenance; nothing rendering a diagram needs them | **A sync run** publishes the base graph under both the commit it analyzed and the baseline commit it writes on top, because a pull request opened either side of diff --git a/scripts/action/analyze.sh b/scripts/action/analyze.sh index 74ad967..6841e96 100755 --- a/scripts/action/analyze.sh +++ b/scripts/action/analyze.sh @@ -90,6 +90,11 @@ stage() { rm -rf "${STAGE_DIR:?}/$kind" mkdir -p "$STAGE_DIR" cp -a "$state" "$STAGE_DIR/$kind" + # Run logs and lock files are the engine's scratch, not analysis state. Nobody + # inflates them, and every fetch pays for them: they were a fifth of a bundle. + # Health config stays, because a run seeded from this bundle reads it. + rm -rf "$STAGE_DIR/$kind/logs" + find "$STAGE_DIR/$kind" -name '*.lock' -delete # Say what this bundle is. Without it a base bundle is an analysis.json and # nothing else, which unpacks exactly like a head artifact and would be # rendered as one by a reader that fetched the wrong name. Written at staging diff --git a/scripts/action/build-review-artifact.sh b/scripts/action/build-review-artifact.sh index a7777d7..0dc5e06 100755 --- a/scripts/action/build-review-artifact.sh +++ b/scripts/action/build-review-artifact.sh @@ -29,7 +29,7 @@ jq -n \ --arg mode "$ANALYSIS_MODE" \ --arg base_sha "$BASE_SHA" \ --arg merge_base_sha "$MERGE_BASE_SHA" \ - --arg merge_base_resolved "$MERGE_BASE_RESOLVED" \ + --argjson merge_base_resolved "${MERGE_BASE_RESOLVED:-false}" \ --arg head_sha "$HEAD_SHA" \ --arg pr_number "$PR_NUMBER" \ --arg seed_source "$SEED_SOURCE" \ diff --git a/tests/test_action_state.py b/tests/test_action_state.py index 622a445..e26a695 100644 --- a/tests/test_action_state.py +++ b/tests/test_action_state.py @@ -51,6 +51,12 @@ def _state(directory: Path, depth: int = 2, cap: int | None = None, **origin: ob encoding="utf-8", ) (directory / "static_analysis.pkl").write_text("pickle", encoding="utf-8") + (directory / "static_analysis.lock").write_text("", encoding="utf-8") + (directory / "logs").mkdir(exist_ok=True) + (directory / "logs" / "run.log").write_text("noise\n", encoding="utf-8") + (directory / "static_analysis.lock").write_text("", encoding="utf-8") + (directory / "logs").mkdir(exist_ok=True) + (directory / "logs" / "run.log").write_text("noise\n", encoding="utf-8") if origin: (directory / "origin.json").write_text(json.dumps(origin), encoding="utf-8") return directory @@ -305,6 +311,17 @@ def test_a_bundle_never_inherits_the_label_of_its_seed(self) -> None: self.assertEqual(json.loads((self.stage_dir / "warmstart" / "metadata.json").read_text())["kind"], "warmstart") + def test_scratch_files_are_not_published(self) -> None: + # Run logs and lock files are the engine's working area. No reader + # inflates them and every fetch pays for them. + _state(self.base_dir) + self._analyze() + + staged = self.stage_dir / "warmstart" + self.assertTrue((staged / "analysis.json").is_file()) + self.assertFalse((staged / "logs").exists()) + self.assertEqual(list(staged.glob("*.lock")), []) + def test_analysis_is_staged_for_publication(self) -> None: _state(self.base_dir) self._bind() @@ -390,7 +407,9 @@ def test_it_ships_both_graphs_and_the_commit_they_describe(self) -> None: # merge base has to appear under a name it looks for or it silently uses # the branch tip. self.assertEqual(metadata["pr_base_sha"], "merge-base-sha") - self.assertEqual(metadata["merge_base_resolved"], "true") + # A JSON string, which "false" also is, is truthy in a consumer: this + # has to be a real boolean or a caveat banner never fires. + self.assertIs(metadata["merge_base_resolved"], True) self.assertEqual(metadata["seed_source"], "pr-chain")