From dba8d0759fca9c881b3ddebb4ed0afbf72f18195 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Wed, 7 Oct 2026 17:11:15 +0200 Subject: [PATCH 1/2] feat: catch up the review base from the nearest saved ancestor With no saved base for the merge base and no usable baseline committed there, a review analyzed the merge base from scratch, even when a saved analysis of a commit a few steps below it was sitting in the artifact store. find-ancestor-base.sh lists the repository's artifacts once, keeps the base analyses for this configuration that a run on the repository's own code produced (the same provenance rule as fetch-state.sh), and walks the merge base's first-parent history up to 100 commits, deepening the shallow checkout. The first hit seeds an incremental catch-up to the merge base, which is then published under the merge base's own name. Order: exact artifact, committed at the merge base, nearest ancestor, full. base_source=ancestor reports it, with base_from_sha and catchup_commits. A compatible ancestor that exists only beyond the bound gives base_reason=too_far_behind; a saved analysis of the merge base under another configuration gives incompatible. Sync uses the same lookup when the branch has no usable committed baseline, so the first sync after the setup pull request merges catches up from the base that pull request's review saved. FORCE_FULL is now lowercased with tr, which also runs on the bash 3.2 macOS ships. Co-Authored-By: Claude Opus 5.5 --- action.yml | 11 +- docs/COMMIT_STRATEGY.md | 20 ++- scripts/action/analyze.sh | 69 ++++++-- scripts/action/find-ancestor-base.sh | 81 ++++++++++ tests/test_action_state.py | 231 +++++++++++++++++++++++++++ 5 files changed, 397 insertions(+), 15 deletions(-) create mode 100755 scripts/action/find-ancestor-base.sh diff --git a/action.yml b/action.yml index 420559f..6465540 100644 --- a/action.yml +++ b/action.yml @@ -184,7 +184,7 @@ outputs: description: 'Which state the review head analysis grew from: pr-chain or base.' value: ${{ steps.review_analyze.outputs.seed_source }} base_source: - description: 'How the review base was obtained: saved, committed, or computed.' + description: 'How the review base was obtained: saved, ancestor, committed, or computed.' value: ${{ steps.review_analyze.outputs.base_source }} merge_base_sha: description: 'Merge base used as the review comparison baseline.' @@ -451,6 +451,13 @@ runs: CHECKOUT_DIR: ${{ github.workspace }}/.codeboarding-target STAGE_DIR: ${{ runner.temp }}/cb-state/${{ github.action }}/out FORCE_FULL: ${{ inputs.force_full }} + CFG_HASH: ${{ steps.state.outputs.cfg_hash }} + # Lets a branch without a usable committed baseline catch up from a saved analysis. + ANCESTOR_LOOKUP: ${{ github.server_url == 'https://github.com' && steps.state.outputs.cfg_hash != '' }} + GIT_TOKEN: ${{ inputs.github_token }} + REPOSITORY: ${{ github.repository }} + GH_HOST: ${{ github.server_url }} + GITHUB_SERVER_URL: ${{ github.server_url }} DEPTH_CAP: ${{ inputs.depth_cap }} MODEL: ${{ inputs.model }} AGENT_MODEL_INPUT: ${{ inputs.agent_model }} @@ -539,6 +546,8 @@ runs: BASE_FETCH_SECONDS: ${{ steps.fetch_base.outputs.seconds }} WARMSTART_DIR: ${{ runner.temp }}/cb-state/${{ github.action }}/warmstart RENEW_BASE: ${{ steps.fetch_base.outputs.renew }} + # Lets a branch without a usable committed baseline catch up from a saved analysis. + ANCESTOR_LOOKUP: ${{ github.server_url == 'https://github.com' && steps.state.outputs.cfg_hash != '' }} # For rewriting the progress comment while a base is built from scratch. PROGRESS_HEADER: ${{ steps.guard.outputs.comment_id }} BASE_REF: ${{ steps.guard.outputs.base_ref }} diff --git a/docs/COMMIT_STRATEGY.md b/docs/COMMIT_STRATEGY.md index d188966..5e5959e 100644 --- a/docs/COMMIT_STRATEGY.md +++ b/docs/COMMIT_STRATEGY.md @@ -72,8 +72,8 @@ never fires. | `base_sha` | string | the base branch tip when the event fired — *not* what was compared against | | `kind` | string | always `review`, so a reader can tell this artifact from a base or warm-start bundle | | `analysed_files_changed` | string | analysed files whose content hash differs between base and head; `unknown` when the analyses cannot say | -| `base_source` | string | how this run obtained the base graph: `saved` (the artifact for the merge base), `committed` (`.codeboarding/` committed at the merge base, caught up), or `computed` (full analysis in this run) | -| `base_reason` | string | only with `computed`: `no_baseline` (nothing to seed from) or `incompatible` (a candidate existed but its depth cap differed, or the engine demanded a full run); empty otherwise | +| `base_source` | string | how this run obtained the base graph: `saved` (the artifact for the merge base), `committed` (`.codeboarding/` committed at the merge base, caught up), `ancestor` (the artifact of an older base-branch commit, caught up), or `computed` (full analysis in this run) | +| `base_reason` | string | only with `computed`: `no_baseline` (nothing to seed from), `incompatible` (a candidate existed but its depth cap or configuration differed, or the engine demanded a full run) or `too_far_behind` (the nearest saved ancestor is beyond the catch-up bound); empty otherwise | | `base_from_sha` | string | the commit whose saved analysis seeded the base; the merge base for `saved`, empty for `computed` or when it lies beyond the fetched history | | `catchup_commits` | string | first-parent commits from `base_from_sha` to the merge base that change anything outside `.codeboarding/`; `0` when exact, empty when unknown | | `base_seconds` | string | wall time spent obtaining the base, the artifact lookup included | @@ -109,14 +109,24 @@ them: |---|---| | the published `codeboarding-base--` artifact with a compatible depth cap | none | | no usable artifact — check out the merge base, seed from a compatible baseline committed there, catch up | one incremental, full if Core requires it | -| no compatible committed baseline either | full analysis directly, at the configured `depth_cap` | +| no compatible committed baseline either: the nearest `codeboarding-base--` artifact among the merge base's last 100 first-parent ancestors | one incremental from that commit to the merge base | +| none within 100 commits either | full analysis directly, at the configured `depth_cap` | A trusted run that computed the base publishes it, so the next pull request -forking from that commit gets the first row. The rows are `base_source` `saved`, -`committed` and `computed` in the review metadata, and the review comment says +forking from that commit gets the first row; that includes a base caught up from +an ancestor. The rows are `base_source` `saved`, `committed`, `ancestor` and +`computed` in the review metadata, and the review comment says which one ran, with measured times. While a base is computed, the progress comment says so in two steps, with the elapsed time and the reason. +The ancestor lookup lists the repository's artifacts once (up to 1,000, newest +first), keeps those named for this configuration and produced by a run on the +repository's own code, and walks the merge base's first-parent history, deepening +the shallow checkout to 101 commits. Sync uses the same lookup when the branch has +no usable committed baseline, so the first sync after the setup pull request +merges catches up from the base that pull request's review saved, instead of +analyzing from scratch. + The configuration hash includes `depth_cap`. The workflow input controls depth for both fresh and fallback analyses; stored legacy depth values never override it. diff --git a/scripts/action/analyze.sh b/scripts/action/analyze.sh index 7283373..355bdfe 100755 --- a/scripts/action/analyze.sh +++ b/scripts/action/analyze.sh @@ -124,14 +124,24 @@ analyze_sync() { rm -rf "$work" seed_state "$CHECKOUT_DIR" "$state" - if [ "${FORCE_FULL,,}" = true ] || [ "$(depth_cap_from "$state/analysis.json")" != "$DEPTH_CAP" ]; then - full "$CHECKOUT_DIR" "$state" "$DEPTH_CAP" - else - incremental "$CHECKOUT_DIR" "$state" - if [ "$REQUIRES_FULL" = true ]; then - full "$CHECKOUT_DIR" "$state" "$DEPTH_CAP" + REQUIRES_FULL=true + if [ "$(printf '%s' "${FORCE_FULL:-false}" | tr '[:upper:]' '[:lower:]')" != true ]; then + if [ "$(depth_cap_from "$state/analysis.json")" = "$DEPTH_CAP" ]; then + incremental "$CHECKOUT_DIR" "$state" + fi + # A branch with no usable committed baseline, such as the first sync after the + # setup pull request merged, catches up from that pull request's saved base. + if [ "$REQUIRES_FULL" = true ] && + seed_from_ancestor "${REPOSITORY:-}" "$(git -C "$CHECKOUT_DIR" rev-parse HEAD 2>/dev/null || true)" "$state" true; then + incremental "$CHECKOUT_DIR" "$state" + [ "$REQUIRES_FULL" = true ] || + echo "::notice::Caught up from the saved analysis of $ANCESTOR_SHA instead of analyzing from scratch." fi fi + unset GIT_TOKEN + if [ "$REQUIRES_FULL" = true ]; then + full "$CHECKOUT_DIR" "$state" "$DEPTH_CAP" + fi # Sync already computes the graph every review of this branch compares against, # so publish it instead of making the first pull request recompute it. stage "$state" base @@ -140,8 +150,9 @@ analyze_sync() { } # How far below the merge base this run looks for the commit a saved analysis -# describes. Past it, a catch-up count is reported as unknown. -CATCHUP_BOUND=100 +# describes, and for a saved ancestor to catch up from. Past it, a catch-up count +# is unknown and an ancestor counts as too far behind. It is also the fetch depth. +CATCHUP_BOUND="${CATCHUP_BOUND:-100}" # A depth above 1 also deepens a commit the shallow checkout already holds. fetch_commit() { @@ -209,6 +220,35 @@ catchup_count() { echo "$count" } +# Replaces $3 with the saved analysis of the nearest first-parent ancestor of $2 +# (or of $2 itself when $4 is true). Sets ANCESTOR_SHA, or when there is none to +# use, ANCESTOR_REASON: too_far_behind or incompatible. +ANCESTOR_SHA="" ANCESTOR_REASON="" +seed_from_ancestor() { + local repository="$1" tip="$2" state="$3" include_tip="$4" found dest="$RUNNER_TEMP/codeboarding-ancestor" + ANCESTOR_SHA="" ANCESTOR_REASON="" + [ "${ANCESTOR_LOOKUP:-false}" = true ] && [ -n "${CFG_HASH:-}" ] && [ -n "${REPOSITORY:-}" ] && [ -n "$tip" ] || return 1 + fetch_commit "$repository" "$tip" "$(( CATCHUP_BOUND + 1 ))" || true + found="$(GH_TOKEN="${GIT_TOKEN:-}" GH_ENTERPRISE_TOKEN="${GIT_TOKEN:-}" TIP_SHA="$tip" DEST="$dest" \ + INCLUDE_TIP="$include_tip" CATCHUP_BOUND="$CATCHUP_BOUND" \ + "$ACTION_PATH/scripts/action/find-ancestor-base.sh" || true)" + ANCESTOR_SHA="$(awk -F= '$1 == "ancestor_sha" {print $2; exit}' <<< "$found")" + if [ -z "$ANCESTOR_SHA" ]; then + if grep -qx 'other_cfg_at_tip=true' <<< "$found"; then + ANCESTOR_REASON=incompatible + elif grep -qx 'too_far_behind=true' <<< "$found"; then + ANCESTOR_REASON=too_far_behind + fi + return 1 + fi + if [ "$(depth_cap_from "$dest/analysis.json")" != "$DEPTH_CAP" ]; then + ANCESTOR_SHA="" ANCESTOR_REASON=incompatible + return 1 + fi + rm -rf "$state" + cp -a "$dest" "$state" +} + # Rewrites the sticky progress comment while the base is built from scratch. A # fork's read-only token makes every call fail, which costs nothing. PROGRESS_PID="" @@ -289,9 +329,20 @@ analyze_review() { elif [ -f "$base_state/analysis.json" ]; then base_reason=incompatible fi + # Nothing at the merge base to grow from: catch up from the nearest saved + # ancestor, and publish the result under the merge base's own name below. + if [ "$REQUIRES_FULL" = true ] && seed_from_ancestor "$REVIEW_BASE_REPO" "$REVIEW_BASE_SHA" "$base_state" false; then + incremental "$base_checkout" "$base_state" + if [ "$REQUIRES_FULL" = true ]; then + base_reason=incompatible + else + base_source=ancestor base_from_sha="$ANCESTOR_SHA" + catchup_commits="$(catchup_count "$ANCESTOR_SHA" "$REVIEW_BASE_SHA")" + fi + fi if [ "$REQUIRES_FULL" = true ]; then base_source=computed - base_reason="${base_reason:-no_baseline}" + base_reason="${base_reason:-${ANCESTOR_REASON:-no_baseline}}" trap progress_stop EXIT progress_start "$base_started" full "$base_checkout" "$base_state" "$DEPTH_CAP" diff --git a/scripts/action/find-ancestor-base.sh b/scripts/action/find-ancestor-base.sh new file mode 100755 index 0000000..4e91cca --- /dev/null +++ b/scripts/action/find-ancestor-base.sh @@ -0,0 +1,81 @@ +#!/usr/bin/env bash +# Finds the nearest first-parent ancestor of TIP_SHA with a saved base analysis +# under this configuration and downloads it into DEST, so a run without an exact +# base can catch up from it instead of analyzing from scratch. Best effort. +# +# Prints key=value lines on stdout and everything else on stderr: +# ancestor_sha the commit whose analysis is now in DEST, or empty +# too_far_behind true when a compatible ancestor exists only beyond the bound +# other_cfg_at_tip true when TIP_SHA has a saved analysis under another configuration +# +# Needs the tip's first-parent history in CHECKOUT_DIR, at least CATCHUP_BOUND deep. +set -euo pipefail +: "${REPOSITORY:?}" "${CFG_HASH:?}" "${TIP_SHA:?}" "${CHECKOUT_DIR:?}" "${DEST:?}" +BOUND="${CATCHUP_BOUND:-100}" +rm -rf "$DEST" + +api() { gh api -H 'Accept: application/vnd.github+json' "$@"; } +export GH_HOST="${GH_HOST:-github.com}" +GH_HOST="${GH_HOST#*://}" + +ancestor="" too_far=false other_cfg=false +report() { + printf 'ancestor_sha=%s\ntoo_far_behind=%s\nother_cfg_at_tip=%s\n' "$ancestor" "$too_far" "$other_cfg" +} +trap report EXIT + +# One listing of every artifact, newest first. The name filter on this endpoint is +# exact, so a prefix needs the whole list. The provenance rule is fetch-state.sh's: +# only artifacts from a run on this repository's own code, since a fork's workflow +# can upload under any name and its bytes would reach a pickle loader. +trusted='.artifacts[]? + | select(.expired == false) + | select(.workflow_run != null) + | select(.workflow_run.head_repository_id == .workflow_run.repository_id) + | select(.name | startswith("codeboarding-base-")) + | .name' +names="" page=1 +while [ "$page" -le "${MAX_PAGES:-10}" ]; do + if ! listing="$(api "repos/$REPOSITORY/actions/artifacts?per_page=100&page=$page" 2>/dev/null)"; then + echo "::warning::Could not list artifacts in $REPOSITORY; not looking for an older saved analysis." >&2 + exit 0 + fi + names="$names$(jq -r "$trusted" <<< "$listing" 2>/dev/null || true)"$'\n' + returned="$(jq -r '.artifacts | length' <<< "$listing" 2>/dev/null || echo 0)" + [ "${returned:-0}" -eq 100 ] || break + page=$(( page + 1 )) +done + +prefix="codeboarding-base-$CFG_HASH-" +saved="$(grep "^$prefix" <<< "$names" | cut -c$(( ${#prefix} + 1 ))- || true)" +if grep "^codeboarding-base-.*-$TIP_SHA\$" <<< "$names" | grep -vq "^$prefix"; then + other_cfg=true +fi +[ -n "$saved" ] || exit 0 + +# Distance 0 is the tip itself, which a review already looked up by its exact +# name; sync has no such lookup and passes INCLUDE_TIP=true. +distance=0 +for commit in $(git -C "$CHECKOUT_DIR" rev-list --first-parent --max-count=$(( BOUND + 1 )) "$TIP_SHA" 2>/dev/null); do + if { [ "$distance" -gt 0 ] || [ "${INCLUDE_TIP:-false}" = true ]; } && grep -qx "$commit" <<< "$saved"; then + ancestor="$commit" + break + fi + distance=$(( distance + 1 )) +done + +if [ -z "$ancestor" ]; then + # Saved analyses of other branches are no reason to call this one far behind, + # so ask whether the newest one is an ancestor at all. + newest="$(head -n 1 <<< "$saved")" + if [ "$(api "repos/$REPOSITORY/compare/$newest...$TIP_SHA" --jq .status 2>/dev/null || true)" = ahead ]; then + too_far=true + fi + exit 0 +fi + +# Downloaded by its exact name, through the same checks as every other lookup. +if ! GITHUB_OUTPUT="" ARTIFACT_NAME="$prefix$ancestor" DEST="$DEST" \ + "$(dirname "$0")/fetch-state.sh" >&2 || [ ! -f "$DEST/analysis.json" ]; then + ancestor="" +fi diff --git a/tests/test_action_state.py b/tests/test_action_state.py index 8008eef..36965c3 100644 --- a/tests/test_action_state.py +++ b/tests/test_action_state.py @@ -639,6 +639,237 @@ def test_analysis_is_staged_for_publication(self) -> None: self.assertFalse((self.stage_dir / "base").exists(), "a published base needs no republishing") +GH_STUB = """#!/usr/bin/env python3 +\"\"\"gh stand-in: serves an artifact listing, one zip and a compare status from a JSON config.\"\"\" +import json, os, sys +from urllib.parse import parse_qs, urlparse + +config = json.load(open(os.environ["CB_GH_CONFIG"])) +with open(os.environ["CB_GH_LOG"], "a") as log: + log.write(" ".join(sys.argv[1:]) + "\\n") +path = next(a for a in sys.argv[1:] if a.startswith("repos/")) +url = urlparse(path) +query = parse_qs(url.query) +if url.path.endswith("/zip"): + sys.stdout.buffer.write(open(config["zip"], "rb").read()) +elif "/compare/" in url.path: + print(config.get("compare", "diverged")) +elif url.path.endswith("/actions/artifacts"): + artifacts = config["artifacts"] + if "name" in query: + artifacts = [a for a in artifacts if a["name"] == query["name"][0]] + page = int(query.get("page", ["1"])[0]) + print(json.dumps({"artifacts": artifacts if page == 1 else []})) +""" + + +class AncestorSeedTests(unittest.TestCase): + """With no saved base for the merge base and nothing committed there, a review + catches up from the nearest saved ancestor on the base branch's first-parent + history instead of analyzing the merge base from scratch.""" + + def setUp(self) -> None: + import zipfile + + self.temp_dir = tempfile.TemporaryDirectory() + self.root = Path(self.temp_dir.name) + self.bin_dir = self.root / "bin" + self.bin_dir.mkdir() + for name, body in (("codeboarding", ENGINE_STUB), ("gh", GH_STUB)): + (self.bin_dir / name).write_text(body, encoding="utf-8") + (self.bin_dir / name).chmod(0o755) + self.engine_log = self.root / "engine.log" + self.engine_log.write_text("", encoding="utf-8") + self.gh_log = self.root / "gh.log" + self.gh_log.write_text("", encoding="utf-8") + self.gh_config = self.root / "gh.json" + self.output = self.root / "github-output" + self.runner_temp = self.root / "runner" + self.runner_temp.mkdir() + self.stage_dir = self.root / "state" / "out" + self.origin = self.root / "origin" + self.origin.mkdir() + self.bundle = self.root / "bundle.zip" + with zipfile.ZipFile(self.bundle, "w") as archive: + archive.writestr("analysis.json", json.dumps({"metadata": {"depth_cap": 2}, "components": []})) + archive.writestr("static_analysis.pkl", "pickle") + archive.writestr("metadata.json", json.dumps({"kind": "base", "merge_base_sha": "ancestor"})) + + def tearDown(self) -> None: + self.temp_dir.cleanup() + + def _history(self, commits: int) -> list[str]: + """A base branch of `commits` code commits, oldest first, in origin/ (the checkout).""" + git = ["git", "-C", str(self.origin), "-c", "user.name=T", "-c", "user.email=t@example.com"] + subprocess.run([*git, "init", "-q"], check=True) + shas = [] + for index in range(commits): + (self.origin / f"file{index}.py").write_text("pass\n", encoding="utf-8") + subprocess.run([*git, "add", "-A"], check=True) + subprocess.run([*git, "-c", "commit.gpgsign=false", "commit", "-q", "-m", f"c{index}"], check=True) + shas.append(subprocess.check_output([*git, "rev-parse", "HEAD"], text=True).strip()) + return shas + + @staticmethod + def _artifact(name: str, *, fork: bool = False) -> dict: + return { + "id": abs(hash(name)) % 100000, + "name": name, + "expired": False, + "created_at": "2026-10-01T00:00:00Z", + "expires_at": "2027-01-01T00:00:00Z", + "workflow_run": {"id": 1, "repository_id": 1, "head_repository_id": 2 if fork else 1}, + } + + def _serve(self, artifacts: list[dict], compare: str = "diverged") -> None: + self.gh_config.write_text( + json.dumps({"artifacts": artifacts, "zip": str(self.bundle), "compare": compare}), encoding="utf-8" + ) + + def _analyze(self, checkout: Path, **extra: str) -> dict[str, str]: + self.output.write_text("", encoding="utf-8") + result = subprocess.run( + [str(ANALYZE)], + env={ + "PATH": f"{self.bin_dir}:{os.environ['PATH']}", + "GITHUB_OUTPUT": str(self.output), + "RUNNER_TEMP": str(self.runner_temp), + "CB_ENGINE_LOG": str(self.engine_log), + "CB_GH_CONFIG": str(self.gh_config), + "CB_GH_LOG": str(self.gh_log), + "ACTION_PATH": str(ROOT), + "ANALYSIS_KIND": "review", + "CHECKOUT_DIR": str(checkout), + "REVIEW_HEAD_SHA": "head-sha", + "REVIEW_BASE_REPO": "origin", + "REPOSITORY": "owner/repo", + "GITHUB_SERVER_URL": f"file://{self.root}", + "PR_NUMBER": "42", + "ENGINE_VERSION": "0.14.5", + "CFG_HASH": "cfg", + "ANCESTOR_LOOKUP": "true", + "BASE_DIR": str(self.root / "state" / "base"), + "WARMSTART_DIR": str(self.root / "state" / "warmstart"), + "STAGE_DIR": str(self.stage_dir), + "DEPTH_CAP": "2", + **extra, + }, + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(result.returncode, 0, result.stderr or result.stdout) + values: dict[str, str] = {} + for line in self.output.read_text(encoding="utf-8").splitlines(): + key, _, value = line.partition("=") + values[key] = value + return values + + def _modes(self) -> list[str]: + return [json.loads(line)["mode"] for line in self.engine_log.read_text().splitlines()] + + def test_it_catches_up_from_the_nearest_saved_ancestor(self) -> None: + shas = self._history(5) + merge_base = shas[-1] + # Two saved ancestors: the nearer one wins. + self._serve( + [self._artifact(f"codeboarding-base-cfg-{shas[0]}"), self._artifact(f"codeboarding-base-cfg-{shas[2]}")] + ) + + values = self._analyze(self.origin, REVIEW_BASE_SHA=merge_base) + + self.assertEqual(values["base_source"], "ancestor") + self.assertEqual(values["base_reason"], "") + self.assertEqual(values["base_from_sha"], shas[2]) + self.assertEqual(values["catchup_commits"], "2") + self.assertEqual(self._modes(), ["incremental", "incremental"]) + # Published under the merge base's own name, so the next review hits it exactly. + self.assertEqual(values["publish_base"], "true") + staged = json.loads((self.stage_dir / "base" / "metadata.json").read_text()) + self.assertEqual(staged["merge_base_sha"], merge_base) + + def test_an_ancestor_beyond_the_bound_means_too_far_behind(self) -> None: + shas = self._history(5) + self._serve([self._artifact(f"codeboarding-base-cfg-{shas[0]}")], compare="ahead") + + values = self._analyze(self.origin, REVIEW_BASE_SHA=shas[-1], CATCHUP_BOUND="2") + + self.assertEqual(values["base_source"], "computed") + self.assertEqual(values["base_reason"], "too_far_behind") + self.assertEqual(self._modes(), ["full", "incremental"]) + + def test_a_saved_analysis_off_this_history_is_not_called_far_behind(self) -> None: + shas = self._history(2) + self._serve([self._artifact("codeboarding-base-cfg-" + "e" * 40)], compare="diverged") + + values = self._analyze(self.origin, REVIEW_BASE_SHA=shas[-1]) + + self.assertEqual(values["base_reason"], "no_baseline") + + def test_an_ancestor_saved_by_a_run_on_forked_code_is_never_read(self) -> None: + shas = self._history(3) + self._serve([self._artifact(f"codeboarding-base-cfg-{shas[0]}", fork=True)]) + + values = self._analyze(self.origin, REVIEW_BASE_SHA=shas[-1]) + + self.assertEqual(values["base_source"], "computed") + self.assertEqual(values["base_reason"], "no_baseline") + self.assertNotIn("/zip", self.gh_log.read_text(), "a fork's bundle was downloaded") + + def test_another_configuration_saved_at_the_merge_base_is_incompatible(self) -> None: + shas = self._history(2) + self._serve([self._artifact(f"codeboarding-base-othercfg-{shas[-1]}")]) + + values = self._analyze(self.origin, REVIEW_BASE_SHA=shas[-1]) + + self.assertEqual(values["base_source"], "computed") + self.assertEqual(values["base_reason"], "incompatible") + + def test_a_shallow_checkout_is_deepened_to_find_the_ancestor(self) -> None: + shas = self._history(4) + bare = self.root / "origin.git" + subprocess.run(["git", "clone", "-q", "--bare", str(self.origin), str(bare)], check=True) + subprocess.run(["git", "-C", str(bare), "config", "uploadpack.allowAnySHA1InWant", "true"], check=True) + checkout = self.root / "shallow" + subprocess.run(["git", "clone", "-q", "--depth=1", f"file://{bare}", str(checkout)], check=True) + self._serve([self._artifact(f"codeboarding-base-cfg-{shas[0]}")]) + + values = self._analyze(checkout, REVIEW_BASE_SHA=shas[-1]) + + self.assertEqual(values["base_source"], "ancestor") + self.assertEqual(values["base_from_sha"], shas[0]) + self.assertEqual(values["catchup_commits"], "3") + + def test_without_the_lookup_nothing_is_listed(self) -> None: + # GHES has no artifact store, so the action turns the lookup off there. + shas = self._history(2) + self._serve([self._artifact(f"codeboarding-base-cfg-{shas[0]}")]) + + values = self._analyze(self.origin, REVIEW_BASE_SHA=shas[-1], ANCESTOR_LOOKUP="false") + + self.assertEqual(values["base_source"], "computed") + self.assertEqual(self.gh_log.read_text(), "") + + def test_a_first_sync_catches_up_from_the_setup_reviews_base(self) -> None: + # Merging the setup pull request leaves no committed baseline, but its + # preview review saved the base at its merge base, the new tip's parent. + shas = self._history(3) + self._serve([self._artifact(f"codeboarding-base-cfg-{shas[1]}")]) + + self._analyze(self.origin, ANALYSIS_KIND="sync", FORCE_FULL="false") + + self.assertEqual(self._modes(), ["incremental"]) + + def test_a_forced_sync_never_seeds(self) -> None: + shas = self._history(2) + self._serve([self._artifact(f"codeboarding-base-cfg-{shas[0]}")]) + + self._analyze(self.origin, ANALYSIS_KIND="sync", FORCE_FULL="True") + + self.assertEqual(self._modes(), ["full"]) + self.assertEqual(self.gh_log.read_text(), "") + + class ReviewArtifactTests(unittest.TestCase): """The artifact is the only channel a reader outside the run can use: cache entries have no download API, so whatever the webview needs must ship here.""" From 32fdc4295553c703d9c208c68b8f3363cbf3b6dc Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Wed, 7 Oct 2026 17:31:33 +0200 Subject: [PATCH 2/2] fix: page the ancestor lookup until it finds one, and keep the merge base's config Review follow-ups on the ancestor seed: - The lookup walks first-parent history first, then pages through the artifact listing newest first and stops at the first page holding a walked commit, instead of stopping after 10 pages. A busy repository's bases sat past page 10 and read as no_baseline. The cap is 50 pages; in practice it is one or two calls, and only on a run that would otherwise analyze from scratch. - too_far_behind needs a walk that reached the bound and a saved analysis confirmed (by compare) to be an ancestor, checking up to five rather than only the newest. A failed deepen or another branch's analysis no longer produces it. - Seeding from an ancestor keeps the merge base's own .codeboardingignore and health configuration instead of the ancestor's copies. Co-Authored-By: Claude Opus 5.5 --- docs/COMMIT_STRATEGY.md | 12 ++-- scripts/action/analyze.sh | 25 ++++++-- scripts/action/find-ancestor-base.sh | 75 ++++++++++++++---------- tests/test_action_state.py | 86 ++++++++++++++++++++++++++-- 4 files changed, 153 insertions(+), 45 deletions(-) diff --git a/docs/COMMIT_STRATEGY.md b/docs/COMMIT_STRATEGY.md index 5e5959e..b107ed9 100644 --- a/docs/COMMIT_STRATEGY.md +++ b/docs/COMMIT_STRATEGY.md @@ -119,10 +119,14 @@ an ancestor. The rows are `base_source` `saved`, `committed`, `ancestor` and which one ran, with measured times. While a base is computed, the progress comment says so in two steps, with the elapsed time and the reason. -The ancestor lookup lists the repository's artifacts once (up to 1,000, newest -first), keeps those named for this configuration and produced by a run on the -repository's own code, and walks the merge base's first-parent history, deepening -the shallow checkout to 101 commits. Sync uses the same lookup when the branch has +The ancestor lookup walks the merge base's first-parent history, deepening the +shallow checkout to 101 commits, then pages through the repository's artifacts +newest first, keeping those named for this configuration and produced by a run on +the repository's own code. It stops at the first page holding one of the walked +commits (usually the first; at most 50 pages) and takes the nearest commit seen. +The merge base's own `.codeboardingignore` and health configuration replace the +seed's. `too_far_behind` is reported only when the walk reached its bound and a +saved analysis is confirmed to be an ancestor beyond it. Sync uses the same lookup when the branch has no usable committed baseline, so the first sync after the setup pull request merges catches up from the base that pull request's review saved, instead of analyzing from scratch. diff --git a/scripts/action/analyze.sh b/scripts/action/analyze.sh index 355bdfe..f662ca0 100755 --- a/scripts/action/analyze.sh +++ b/scripts/action/analyze.sh @@ -132,7 +132,7 @@ analyze_sync() { # A branch with no usable committed baseline, such as the first sync after the # setup pull request merged, catches up from that pull request's saved base. if [ "$REQUIRES_FULL" = true ] && - seed_from_ancestor "${REPOSITORY:-}" "$(git -C "$CHECKOUT_DIR" rev-parse HEAD 2>/dev/null || true)" "$state" true; then + seed_from_ancestor "${REPOSITORY:-}" "$(git -C "$CHECKOUT_DIR" rev-parse HEAD 2>/dev/null || true)" "$state" true "$CHECKOUT_DIR"; then incremental "$CHECKOUT_DIR" "$state" [ "$REQUIRES_FULL" = true ] || echo "::notice::Caught up from the saved analysis of $ANCESTOR_SHA instead of analyzing from scratch." @@ -221,11 +221,12 @@ catchup_count() { } # Replaces $3 with the saved analysis of the nearest first-parent ancestor of $2 -# (or of $2 itself when $4 is true). Sets ANCESTOR_SHA, or when there is none to -# use, ANCESTOR_REASON: too_far_behind or incompatible. +# (or of $2 itself when $4 is true), keeping the user-authored configuration of +# checkout $5. Sets ANCESTOR_SHA, or when there is none to use, ANCESTOR_REASON: +# too_far_behind or incompatible. ANCESTOR_SHA="" ANCESTOR_REASON="" seed_from_ancestor() { - local repository="$1" tip="$2" state="$3" include_tip="$4" found dest="$RUNNER_TEMP/codeboarding-ancestor" + local repository="$1" tip="$2" state="$3" include_tip="$4" config_from="$5" found dest="$RUNNER_TEMP/codeboarding-ancestor" ANCESTOR_SHA="" ANCESTOR_REASON="" [ "${ANCESTOR_LOOKUP:-false}" = true ] && [ -n "${CFG_HASH:-}" ] && [ -n "${REPOSITORY:-}" ] && [ -n "$tip" ] || return 1 fetch_commit "$repository" "$tip" "$(( CATCHUP_BOUND + 1 ))" || true @@ -247,6 +248,20 @@ seed_from_ancestor() { fi rm -rf "$state" cp -a "$dest" "$state" + keep_user_config "$config_from" "$state" +} +# What the user writes under .codeboarding/ belongs to the commit being analysed, +# not to whichever analysis seeded it. +USER_CONFIG=(.codeboardingignore health/health_config.json health/.healthignore) +keep_user_config() { + local checkout="$1" state="$2" file + for file in "${USER_CONFIG[@]}"; do + rm -f "${state:?}/$file" + [ ! -f "$checkout/.codeboarding/$file" ] || { + mkdir -p "$(dirname "$state/$file")" + cp "$checkout/.codeboarding/$file" "$state/$file" + } + done } # Rewrites the sticky progress comment while the base is built from scratch. A @@ -331,7 +346,7 @@ analyze_review() { fi # Nothing at the merge base to grow from: catch up from the nearest saved # ancestor, and publish the result under the merge base's own name below. - if [ "$REQUIRES_FULL" = true ] && seed_from_ancestor "$REVIEW_BASE_REPO" "$REVIEW_BASE_SHA" "$base_state" false; then + if [ "$REQUIRES_FULL" = true ] && seed_from_ancestor "$REVIEW_BASE_REPO" "$REVIEW_BASE_SHA" "$base_state" false "$base_checkout"; then incremental "$base_checkout" "$base_state" if [ "$REQUIRES_FULL" = true ]; then base_reason=incompatible diff --git a/scripts/action/find-ancestor-base.sh b/scripts/action/find-ancestor-base.sh index 4e91cca..2ce2663 100755 --- a/scripts/action/find-ancestor-base.sh +++ b/scripts/action/find-ancestor-base.sh @@ -24,52 +24,67 @@ report() { } trap report EXIT -# One listing of every artifact, newest first. The name filter on this endpoint is -# exact, so a prefix needs the whole list. The provenance rule is fetch-state.sh's: -# only artifacts from a run on this repository's own code, since a fork's workflow -# can upload under any name and its bytes would reach a pickle loader. +# The merge base's first-parent history, nearest first. Distance 0 is the tip +# itself, which a review already looked up by its exact name; sync has no such +# lookup and passes INCLUDE_TIP=true. +walk="$(git -C "$CHECKOUT_DIR" rev-list --first-parent --max-count=$(( BOUND + 1 )) "$TIP_SHA" 2>/dev/null || true)" +[ "${INCLUDE_TIP:-false}" = true ] || walk="$(tail -n +2 <<< "$walk")" +walked="$(grep -c . <<< "$walk" || true)" +nearest() { + local commit + for commit in $walk; do + if grep -qx "$commit" <<< "$saved"; then + echo "$commit" + return 0 + fi + done +} + +# The artifact listing, newest first. Its name filter is exact, so a prefix needs +# the pages themselves; paging stops at the first page holding a saved ancestor. +# Bases are published as their commits are synced or reviewed, so the nearest one +# is nearly always the newest: one or two calls in practice, MAX_PAGES (50, 5,000 +# artifacts) at worst, against GITHUB_TOKEN's 1,000 requests an hour per +# repository, and only on a run that would otherwise analyze from scratch. The +# provenance rule is fetch-state.sh's: only artifacts from a run on this +# repository's own code, since a fork's workflow can upload under any name and +# its bytes would reach a pickle loader. trusted='.artifacts[]? | select(.expired == false) | select(.workflow_run != null) | select(.workflow_run.head_repository_id == .workflow_run.repository_id) | select(.name | startswith("codeboarding-base-")) | .name' -names="" page=1 -while [ "$page" -le "${MAX_PAGES:-10}" ]; do +prefix="codeboarding-base-$CFG_HASH-" +saved="" page=1 +while [ "$page" -le "${MAX_PAGES:-50}" ]; do if ! listing="$(api "repos/$REPOSITORY/actions/artifacts?per_page=100&page=$page" 2>/dev/null)"; then echo "::warning::Could not list artifacts in $REPOSITORY; not looking for an older saved analysis." >&2 exit 0 fi - names="$names$(jq -r "$trusted" <<< "$listing" 2>/dev/null || true)"$'\n' + names="$(jq -r "$trusted" <<< "$listing" 2>/dev/null || true)" + saved="$saved$(grep "^$prefix" <<< "$names" | cut -c$(( ${#prefix} + 1 ))- || true)"$'\n' + if grep "^codeboarding-base-.*-$TIP_SHA\$" <<< "$names" | grep -vq "^$prefix"; then + other_cfg=true + fi + ancestor="$(nearest)" + [ -z "$ancestor" ] || break returned="$(jq -r '.artifacts | length' <<< "$listing" 2>/dev/null || echo 0)" [ "${returned:-0}" -eq 100 ] || break page=$(( page + 1 )) done -prefix="codeboarding-base-$CFG_HASH-" -saved="$(grep "^$prefix" <<< "$names" | cut -c$(( ${#prefix} + 1 ))- || true)" -if grep "^codeboarding-base-.*-$TIP_SHA\$" <<< "$names" | grep -vq "^$prefix"; then - other_cfg=true -fi -[ -n "$saved" ] || exit 0 - -# Distance 0 is the tip itself, which a review already looked up by its exact -# name; sync has no such lookup and passes INCLUDE_TIP=true. -distance=0 -for commit in $(git -C "$CHECKOUT_DIR" rev-list --first-parent --max-count=$(( BOUND + 1 )) "$TIP_SHA" 2>/dev/null); do - if { [ "$distance" -gt 0 ] || [ "${INCLUDE_TIP:-false}" = true ]; } && grep -qx "$commit" <<< "$saved"; then - ancestor="$commit" - break - fi - distance=$(( distance + 1 )) -done - if [ -z "$ancestor" ]; then - # Saved analyses of other branches are no reason to call this one far behind, - # so ask whether the newest one is an ancestor at all. - newest="$(head -n 1 <<< "$saved")" - if [ "$(api "repos/$REPOSITORY/compare/$newest...$TIP_SHA" --jq .status 2>/dev/null || true)" = ahead ]; then - too_far=true + # Too far behind only when the walk reached the bound, so a short or failed + # deepen is never mistaken for distance, and a saved analysis is confirmed to + # be an ancestor: others may belong to other branches. + if [ "$walked" -ge "$BOUND" ]; then + for candidate in $(grep . <<< "$saved" | head -n 5); do + if [ "$(api "repos/$REPOSITORY/compare/$candidate...$TIP_SHA" --jq .status 2>/dev/null || true)" = ahead ]; then + too_far=true + break + fi + done fi exit 0 fi diff --git a/tests/test_action_state.py b/tests/test_action_state.py index 36965c3..d13d183 100644 --- a/tests/test_action_state.py +++ b/tests/test_action_state.py @@ -653,13 +653,19 @@ def test_analysis_is_staged_for_publication(self) -> None: if url.path.endswith("/zip"): sys.stdout.buffer.write(open(config["zip"], "rb").read()) elif "/compare/" in url.path: - print(config.get("compare", "diverged")) + compare = config.get("compare", "diverged") + base = url.path.split("/compare/")[1].split("...")[0] + print(compare.get(base, "diverged") if isinstance(compare, dict) else compare) elif url.path.endswith("/actions/artifacts"): artifacts = config["artifacts"] if "name" in query: artifacts = [a for a in artifacts if a["name"] == query["name"][0]] page = int(query.get("page", ["1"])[0]) - print(json.dumps({"artifacts": artifacts if page == 1 else []})) + if "name" not in query and "pages" in config: + pages = config["pages"] + print(json.dumps({"artifacts": pages[page - 1] if page <= len(pages) else []})) + else: + print(json.dumps({"artifacts": artifacts if page == 1 else []})) """ @@ -721,10 +727,11 @@ def _artifact(name: str, *, fork: bool = False) -> dict: "workflow_run": {"id": 1, "repository_id": 1, "head_repository_id": 2 if fork else 1}, } - def _serve(self, artifacts: list[dict], compare: str = "diverged") -> None: - self.gh_config.write_text( - json.dumps({"artifacts": artifacts, "zip": str(self.bundle), "compare": compare}), encoding="utf-8" - ) + def _serve(self, artifacts: list[dict], compare: str | dict = "diverged", pages: list | None = None) -> None: + config = {"artifacts": artifacts, "zip": str(self.bundle), "compare": compare} + if pages is not None: + config["pages"] = pages + self.gh_config.write_text(json.dumps(config), encoding="utf-8") def _analyze(self, checkout: Path, **extra: str) -> dict[str, str]: self.output.write_text("", encoding="utf-8") @@ -806,6 +813,73 @@ def test_a_saved_analysis_off_this_history_is_not_called_far_behind(self) -> Non self.assertEqual(values["base_reason"], "no_baseline") + def test_a_busy_artifact_store_is_paged_until_a_saved_ancestor_appears(self) -> None: + shas = self._history(3) + noise = [self._artifact(f"codeboarding-review-{i}-1") for i in range(100)] + found = self._artifact(f"codeboarding-base-cfg-{shas[0]}") + later = [self._artifact(f"codeboarding-warmstart-cfg-pr{i}") for i in range(100)] + self._serve([found], pages=[noise] * 11 + [noise[:99] + [found], later]) + + values = self._analyze(self.origin, REVIEW_BASE_SHA=shas[-1]) + + self.assertEqual(values["base_source"], "ancestor") + self.assertEqual(values["base_from_sha"], shas[0]) + listings = [ + line + for line in self.gh_log.read_text().splitlines() + if "per_page=100&page=" in line and "name=" not in line + ] + self.assertEqual(len(listings), 12, "paging stops at the page holding the ancestor") + + def test_a_newer_analysis_of_another_branch_does_not_hide_one_beyond_the_bound(self) -> None: + shas = self._history(5) + other = "e" * 40 + self._serve( + [self._artifact(f"codeboarding-base-cfg-{other}"), self._artifact(f"codeboarding-base-cfg-{shas[0]}")], + compare={other: "diverged", shas[0]: "ahead"}, + ) + + values = self._analyze(self.origin, REVIEW_BASE_SHA=shas[-1], CATCHUP_BOUND="2") + + self.assertEqual(values["base_reason"], "too_far_behind") + + def test_a_walk_cut_short_is_never_called_too_far_behind(self) -> None: + # The deepen fails, so the shallow checkout walks one commit: that says + # nothing about distance. + shas = self._history(4) + bare = self.root / "origin.git" + subprocess.run(["git", "clone", "-q", "--bare", str(self.origin), str(bare)], check=True) + checkout = self.root / "shallow" + subprocess.run(["git", "clone", "-q", "--depth=1", f"file://{bare}", str(checkout)], check=True) + self._serve([self._artifact(f"codeboarding-base-cfg-{shas[0]}")], compare="ahead") + + values = self._analyze(checkout, REVIEW_BASE_SHA=shas[-1], GITHUB_SERVER_URL=f"file://{self.root}/missing") + + self.assertEqual(values["base_source"], "computed") + self.assertEqual(values["base_reason"], "no_baseline") + + def test_the_merge_bases_own_configuration_survives_the_seed(self) -> None: + shas = self._history(2) + (self.origin / ".codeboarding").mkdir() + (self.origin / ".codeboarding" / ".codeboardingignore").write_text("docs/\n", encoding="utf-8") + git = ["git", "-C", str(self.origin), "-c", "user.name=T", "-c", "user.email=t@example.com"] + subprocess.run([*git, "add", "-A"], check=True) + subprocess.run([*git, "-c", "commit.gpgsign=false", "commit", "-q", "-m", "ignore docs"], check=True) + merge_base = subprocess.check_output([*git, "rev-parse", "HEAD"], text=True).strip() + import zipfile + + with zipfile.ZipFile(self.bundle, "a") as archive: + archive.writestr(".codeboardingignore", "stale/\n") + archive.writestr("health/health_config.json", "{}") + self._serve([self._artifact(f"codeboarding-base-cfg-{shas[0]}")]) + + values = self._analyze(self.origin, REVIEW_BASE_SHA=merge_base) + + self.assertEqual(values["base_source"], "ancestor") + staged = self.stage_dir / "base" + self.assertEqual((staged / ".codeboardingignore").read_text(), "docs/\n") + self.assertFalse((staged / "health" / "health_config.json").exists(), "the merge base has none") + def test_an_ancestor_saved_by_a_run_on_forked_code_is_never_read(self) -> None: shas = self._history(3) self._serve([self._artifact(f"codeboarding-base-cfg-{shas[0]}", fork=True)])