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")