From 7a091f5975f22d3f772b083a4be9c742275c78d4 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Wed, 7 Oct 2026 17:05:38 +0200 Subject: [PATCH 1/3] feat: report how the review base was built A review's base comes from a saved artifact, a baseline committed at the merge base, or a full analysis in this run, and nothing said which. The slow path is the last one, and a reader had no way to tell why a run took ten minutes instead of two. analyze.sh now records base_source (saved, committed, computed), base_reason (no_baseline, incompatible), base_from_sha, catchup_commits, base_seconds and head_seconds. They go into the review metadata.json as strings, onto the comment's machine-readable marker, and into one "Base:" line in the comment. While a base is computed, the sticky progress comment is rewritten into two steps with the elapsed time and the reason, never an estimate. Co-Authored-By: Claude Opus 5.5 --- action.yml | 22 ++++ docs/COMMIT_STRATEGY.md | 15 ++- scripts/action/analyze.sh | 104 ++++++++++++++-- scripts/action/build-review-artifact.sh | 13 +- scripts/action/build-review-comment.sh | 48 +++++++- scripts/action/fetch-state.sh | 4 + scripts/action/post-progress.sh | 55 +++++++++ tests/test_action_state.py | 156 ++++++++++++++++++++++++ tests/test_progress_comment.py | 84 +++++++++++++ tests/test_review_artifact_metadata.py | 40 ++++++ tests/test_review_comment.py | 66 ++++++++++ 11 files changed, 593 insertions(+), 14 deletions(-) create mode 100755 scripts/action/post-progress.sh create mode 100644 tests/test_progress_comment.py diff --git a/action.yml b/action.yml index ab7f483..dc805e0 100644 --- a/action.yml +++ b/action.yml @@ -183,6 +183,9 @@ outputs: seed_source: 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.' + value: ${{ steps.review_analyze.outputs.base_source }} merge_base_sha: description: 'Merge base used as the review comparison baseline.' value: ${{ steps.guard.outputs.merge_base_sha }} @@ -533,8 +536,14 @@ runs: REVIEW_BASE_REPO: ${{ steps.guard.outputs.base_repo }} PR_NUMBER: ${{ steps.guard.outputs.pr_number }} BASE_DIR: ${{ runner.temp }}/cb-state/${{ github.action }}/base + BASE_FETCH_SECONDS: ${{ steps.fetch_base.outputs.seconds }} WARMSTART_DIR: ${{ runner.temp }}/cb-state/${{ github.action }}/warmstart RENEW_BASE: ${{ steps.fetch_base.outputs.renew }} + # 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 }} + REPOSITORY: ${{ github.repository }} + GH_HOST: ${{ github.server_url }} STAGE_DIR: ${{ runner.temp }}/cb-state/${{ github.action }}/out ENGINE_VERSION: ${{ steps.state.outputs.engine_version }} CFG_HASH: ${{ steps.state.outputs.cfg_hash }} @@ -608,6 +617,12 @@ runs: 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 }} + BASE_SOURCE: ${{ steps.review_analyze.outputs.base_source }} + BASE_REASON: ${{ steps.review_analyze.outputs.base_reason }} + BASE_FROM_SHA: ${{ steps.review_analyze.outputs.base_from_sha }} + CATCHUP_COMMITS: ${{ steps.review_analyze.outputs.catchup_commits }} + BASE_SECONDS: ${{ steps.review_analyze.outputs.base_seconds }} + HEAD_SECONDS: ${{ steps.review_analyze.outputs.head_seconds }} run: "$GITHUB_ACTION_PATH/scripts/action/build-review-artifact.sh" - name: Upload review artifact @@ -639,6 +654,13 @@ runs: 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 }} + MERGE_BASE_SHA: ${{ steps.guard.outputs.merge_base_sha }} + BASE_SOURCE: ${{ steps.review_analyze.outputs.base_source }} + BASE_REASON: ${{ steps.review_analyze.outputs.base_reason }} + BASE_FROM_SHA: ${{ steps.review_analyze.outputs.base_from_sha }} + CATCHUP_COMMITS: ${{ steps.review_analyze.outputs.catchup_commits }} + BASE_SECONDS: ${{ steps.review_analyze.outputs.base_seconds }} + HEAD_SECONDS: ${{ steps.review_analyze.outputs.head_seconds }} run: "$GITHUB_ACTION_PATH/scripts/action/build-review-comment.sh" - name: Post review comment diff --git a/docs/COMMIT_STRATEGY.md b/docs/COMMIT_STRATEGY.md index 2849dfb..d188966 100644 --- a/docs/COMMIT_STRATEGY.md +++ b/docs/COMMIT_STRATEGY.md @@ -70,7 +70,15 @@ never fires. | `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 | +| `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_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 | +| `head_seconds` | string | wall time of the head analysis | +| `pr_number`, `mode`, `seed_source`, `chain_depth` | string | provenance; nothing rendering a diagram needs them. `mode` is the head's engine mode; `base_source` answers for the base | **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 @@ -104,7 +112,10 @@ them: | no compatible committed baseline 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. +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 +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 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 5f246a0..228f6b1 100755 --- a/scripts/action/analyze.sh +++ b/scripts/action/analyze.sh @@ -139,13 +139,73 @@ analyze_sync() { "$ANALYSIS_MODE" "$ANALYSIS_PATH" "$state" >> "$GITHUB_OUTPUT" } +# 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 + +# A depth above 1 also deepens a commit the shallow checkout already holds. fetch_commit() { - local repository="$1" sha="$2" - git -C "$CHECKOUT_DIR" cat-file -e "$sha^{commit}" 2>/dev/null && return 0 + local repository="$1" sha="$2" depth="${3:-1}" + if git -C "$CHECKOUT_DIR" cat-file -e "$sha^{commit}" 2>/dev/null; then + [ "$depth" -gt 1 ] && [ "$(git -C "$CHECKOUT_DIR" rev-parse --is-shallow-repository)" = true ] || return 0 + fi local auth - auth="$(printf 'x-access-token:%s' "$GIT_TOKEN" | base64 -w0)" + auth="$(printf 'x-access-token:%s' "${GIT_TOKEN:-}" | base64 -w0)" git -C "$CHECKOUT_DIR" -c "http.extraheader=AUTHORIZATION: basic $auth" fetch \ - "${GITHUB_SERVER_URL%/}/${repository}.git" "$sha" --depth=1 + "${GITHUB_SERVER_URL%/}/${repository}.git" "$sha" --depth="$depth" +} + +# The commit a baseline committed at $1 describes. Sync writes it in a commit of +# its own on top of the analysed one, so that commit's parent is the answer. +# Empty when nothing within the fetched history wrote it. +baseline_commit() { + local tip="$1" writer shallow + writer="$(git -C "$CHECKOUT_DIR" log --first-parent -1 --format=%H "$tip" -- .codeboarding/analysis.json 2>/dev/null || true)" + [ -n "$writer" ] || return 0 + # A shallow boundary looks like it added every file, so it proves nothing. + shallow="$(git -C "$CHECKOUT_DIR" rev-parse --git-path shallow)" + case "$shallow" in /*) ;; *) shallow="$CHECKOUT_DIR/$shallow" ;; esac + if [ -f "$shallow" ] && grep -qx "$writer" "$shallow"; then + return 0 + fi + if [ -z "$(code_paths_changed "$writer")" ] && git -C "$CHECKOUT_DIR" rev-parse -q --verify "$writer^1" >/dev/null; then + git -C "$CHECKOUT_DIR" rev-parse "$writer^1" + else + echo "$writer" + fi +} +code_paths_changed() { + git -C "$CHECKOUT_DIR" diff-tree --no-commit-id --name-only -r "$1" -- . ':(exclude).codeboarding' 2>/dev/null || true +} +# First-parent commits from $1 to $2 that change anything outside .codeboarding/: +# a sync commit changes nothing the analysis reads, so it is nothing to catch up. +catchup_count() { + local from="$1" to="$2" commit count=0 + for commit in $(git -C "$CHECKOUT_DIR" rev-list --first-parent "$from..$to" 2>/dev/null); do + [ -z "$(code_paths_changed "$commit")" ] || count=$(( count + 1 )) + done + echo "$count" +} + +# 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="" +progress() { + GH_TOKEN="${GIT_TOKEN:-}" GH_ENTERPRISE_TOKEN="${GIT_TOKEN:-}" BASE_REASON="$base_reason" \ + "$ACTION_PATH/scripts/action/post-progress.sh" "$@" >/dev/null 2>&1 || true +} +progress_start() { + local started="$1" + progress base 0 + ( while sleep 60; do progress base "$(( $(date +%s) - started ))"; done ) >/dev/null 2>&1 & + PROGRESS_PID=$! +} +progress_stop() { + [ -n "$PROGRESS_PID" ] || return 0 + pkill -P "$PROGRESS_PID" 2>/dev/null || true + kill "$PROGRESS_PID" 2>/dev/null || true + wait "$PROGRESS_PID" 2>/dev/null || true + PROGRESS_PID="" } # The artifact name pins configuration; verify the stored cap and lineage too. @@ -174,24 +234,47 @@ analyze_review() { # A published base graph is this merge base's own analysis, named for it, so it # needs no engine run at all. Without one, the merge base is checked out and - # analyzed from whatever baseline the repository committed there. - local base_source=published + # analyzed from whatever baseline the repository committed there. Each path + # records how the base was obtained, for the comment and the review artifact. + local base_started base_source=saved base_reason="" base_from_sha="" catchup_commits="" + base_started="$(date +%s)" if [ "$(depth_cap_from "${BASE_DIR:-}/analysis.json")" = "$DEPTH_CAP" ]; then mkdir -p "$base_state" cp -a "$BASE_DIR/." "$base_state/" + base_from_sha="$REVIEW_BASE_SHA" catchup_commits=0 else - base_source=computed + # A bundle under this exact name that the run cannot use was made with another cap. + [ ! -f "${BASE_DIR:-}/analysis.json" ] || base_reason=incompatible fetch_commit "$REVIEW_BASE_REPO" "$REVIEW_BASE_SHA" git -C "$CHECKOUT_DIR" worktree add --detach "$base_checkout" "$REVIEW_BASE_SHA" >/dev/null seed_state "$base_checkout" "$base_state" REQUIRES_FULL=true if [ "$(depth_cap_from "$base_state/analysis.json")" = "$DEPTH_CAP" ]; then incremental "$base_checkout" "$base_state" + if [ "$REQUIRES_FULL" = true ]; then + base_reason=incompatible + else + base_source=committed + fetch_commit "$REVIEW_BASE_REPO" "$REVIEW_BASE_SHA" "$(( CATCHUP_BOUND + 1 ))" || true + base_from_sha="$(baseline_commit "$REVIEW_BASE_SHA")" + [ -z "$base_from_sha" ] || catchup_commits="$(catchup_count "$base_from_sha" "$REVIEW_BASE_SHA")" + fi + elif [ -f "$base_state/analysis.json" ]; then + base_reason=incompatible fi if [ "$REQUIRES_FULL" = true ]; then + base_source=computed + base_reason="${base_reason:-no_baseline}" + trap progress_stop EXIT + progress_start "$base_started" full "$base_checkout" "$base_state" "$DEPTH_CAP" + progress_stop + progress head "$(( $(date +%s) - base_started ))" fi fi + [ "$base_source" = computed ] || base_reason="" + # The lookup in the step before this one is part of obtaining the base too. + local base_seconds=$(( $(date +%s) - base_started + ${BASE_FETCH_SECONDS:-0} )) unset GIT_TOKEN local base_analysis="$base_state/analysis.json" @@ -213,10 +296,13 @@ analyze_review() { fi rm -f "$head_state/origin.json" + local head_started + head_started="$(date +%s)" incremental "$CHECKOUT_DIR" "$head_state" if [ "$REQUIRES_FULL" = true ]; then full "$CHECKOUT_DIR" "$head_state" "$DEPTH_CAP" fi + local head_seconds=$(( $(date +%s) - head_started )) write_origin "$head_state" "$seed_source" "$chain_depth" "$(analysis_digest "$base_analysis")" stage "$head_state" warmstart @@ -226,13 +312,15 @@ analyze_review() { # by id for its whole retention, so one about to expire is renewed rather than # left dangling under a review that outlives it. local publish_base=false - if [ "$base_source" = computed ] || [ "${RENEW_BASE:-false}" = true ]; then + if [ "$base_source" != saved ] || [ "${RENEW_BASE:-false}" = true ]; then stage "$base_state" base publish_base=true fi printf 'analysis_mode=%s\nanalysis_path=%s\nbase_analysis_path=%s\nseed_source=%s\nchain_depth=%s\npublish_base=%s\n' \ "$ANALYSIS_MODE" "$ANALYSIS_PATH" "$base_analysis" "$seed_source" "$chain_depth" "$publish_base" >> "$GITHUB_OUTPUT" + printf 'base_source=%s\nbase_reason=%s\nbase_from_sha=%s\ncatchup_commits=%s\nbase_seconds=%s\nhead_seconds=%s\n' \ + "$base_source" "$base_reason" "$base_from_sha" "$catchup_commits" "$base_seconds" "$head_seconds" >> "$GITHUB_OUTPUT" } case "$ANALYSIS_KIND" in diff --git a/scripts/action/build-review-artifact.sh b/scripts/action/build-review-artifact.sh index acdf52a..aa14c95 100755 --- a/scripts/action/build-review-artifact.sh +++ b/scripts/action/build-review-artifact.sh @@ -23,7 +23,8 @@ HEALTH_REPORT="$(dirname "$ANALYSIS_PATH")/health/health_report.json" # pr_base_sha carries the same value under the name the webview already reads: # its lookup is base_commit_sha || pr_base_sha || base_sha, so without it the # webview silently falls through to the branch tip and compares against a base -# this review never used. +# this review never used. The base_* fields say how this run obtained the base +# graph; all strings, empty when not applicable. jq -n \ --arg kind review \ --arg mode "$ANALYSIS_MODE" \ @@ -37,10 +38,18 @@ jq -n \ --arg base_artifact "$BASE_ARTIFACT_NAME" \ --arg base_artifact_id "$BASE_ARTIFACT_ID" \ --arg analysed_files_changed "${ANALYSED_FILES_CHANGED:-unknown}" \ + --arg base_source "${BASE_SOURCE:-}" \ + --arg base_reason "${BASE_REASON:-}" \ + --arg base_from_sha "${BASE_FROM_SHA:-}" \ + --arg catchup_commits "${CATCHUP_COMMITS:-}" \ + --arg base_seconds "${BASE_SECONDS:-}" \ + --arg head_seconds "${HEAD_SECONDS:-}" \ '{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, - analysed_files_changed: $analysed_files_changed}' \ + analysed_files_changed: $analysed_files_changed, + base_source: $base_source, base_reason: $base_reason, base_from_sha: $base_from_sha, + catchup_commits: $catchup_commits, base_seconds: $base_seconds, head_seconds: $head_seconds}' \ > "${RUNNER_TEMP}/cb-review-artifact/metadata.json" echo "artifact_dir=${RUNNER_TEMP}/cb-review-artifact" >> "$GITHUB_OUTPUT" diff --git a/scripts/action/build-review-comment.sh b/scripts/action/build-review-comment.sh index 96743a6..a1869fa 100755 --- a/scripts/action/build-review-comment.sh +++ b/scripts/action/build-review-comment.sh @@ -45,6 +45,44 @@ elif [ "$BEHIND" -gt 0 ] 2>/dev/null; then printf '\nCompared against the merge base: this branch is %s %s behind `%s`.\n' \ "$BEHIND" "$COMMIT_NOUN" "${BASE_REF:-the base branch}" >> "$BODY" fi +# How the base was obtained, with measured times only: an estimate would be wrong for +# exactly the slow runs it is meant to explain. +duration() { + local seconds="${1:-0}" + case "$seconds" in ''|*[!0-9]*) seconds=0 ;; esac + if [ "$seconds" -ge 60 ]; then + printf '%s m %s s' "$(( seconds / 60 ))" "$(( seconds % 60 ))" + else + printf '%s s' "$seconds" + fi +} +BASE_SOURCE="${BASE_SOURCE:-}" +BASE_BRANCH="${BASE_REF:-the base branch}" +BASE_AT="${BASE_FROM_SHA:-${MERGE_BASE_SHA:-}}" +CHANGES="changes $(duration "${HEAD_SECONDS:-}")" +BASE_LINE="" +case "$BASE_SOURCE" in + computed) + case "${BASE_REASON:-}" in + incompatible) WHY="saved diagram incompatible" ;; + too_far_behind) WHY="saved diagram too far behind" ;; + *) WHY="no saved diagram" ;; + esac + BASE_LINE="Base: built from scratch (${WHY}), $(duration "${BASE_SECONDS:-}") · ${CHANGES}" + ;; + saved | committed | ancestor) + CATCHUP="${CATCHUP_COMMITS:-}" + case "$CATCHUP" in ''|*[!0-9]*) CATCHUP=0 ;; esac + if [ "$CATCHUP" -gt 0 ]; then + COMMIT_NOUN="commits" + [ "$CATCHUP" != 1 ] || COMMIT_NOUN="commit" + BASE_LINE="Base: caught up ${CATCHUP} ${COMMIT_NOUN} from ${BASE_BRANCH} @${BASE_AT:0:7}, $(duration "${BASE_SECONDS:-}") · ${CHANGES}" + else + BASE_LINE="Base: saved diagram of ${BASE_BRANCH} @${BASE_AT:0:7} · ${CHANGES}" + fi + ;; +esac +[ -z "$BASE_LINE" ] || printf '\n%s\n' "$BASE_LINE" >> "$BODY" { printf '\n' cat "$DIAGRAM" @@ -56,7 +94,13 @@ fi # 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 '\n' \ - "$PLATFORM_URL" "$N_CHANGED" "$ANALYSED_FILES_CHANGED" "${HEAD_SHA:-}" + BASE_KEYS="" + if [ -n "$BASE_SOURCE" ]; then + BASE_KEYS=" base=${BASE_SOURCE}" + [ -z "${BASE_REASON:-}" ] || BASE_KEYS="${BASE_KEYS} base_reason=${BASE_REASON}" + BASE_KEYS="${BASE_KEYS} base_seconds=${BASE_SECONDS:-} head_seconds=${HEAD_SECONDS:-}" + fi + printf '\n' \ + "$PLATFORM_URL" "$N_CHANGED" "$ANALYSED_FILES_CHANGED" "${HEAD_SHA:-}" "$BASE_KEYS" } >> "$BODY" echo "path=$BODY" >> "$GITHUB_OUTPUT" diff --git a/scripts/action/fetch-state.sh b/scripts/action/fetch-state.sh index cf16a1e..e90dacd 100755 --- a/scripts/action/fetch-state.sh +++ b/scripts/action/fetch-state.sh @@ -4,6 +4,10 @@ # leave the directory absent, and the caller derives from the base instead. set -euo pipefail [ -n "${ARTIFACT_NAME:-}" ] || exit 0 +# A review reports how long obtaining its base took, and this lookup is part of it. +started="$(date +%s)" +report_seconds() { [ -z "${GITHUB_OUTPUT:-}" ] || echo "seconds=$(( $(date +%s) - started ))" >> "$GITHUB_OUTPUT"; } +trap report_seconds EXIT # Clear first, on every path. These destinations are fixed, so a second use of # the action in one job would otherwise inherit the first one's files and treat diff --git a/scripts/action/post-progress.sh b/scripts/action/post-progress.sh new file mode 100755 index 0000000..746fcbe --- /dev/null +++ b/scripts/action/post-progress.sh @@ -0,0 +1,55 @@ +#!/usr/bin/env bash +# Rewrites the review's sticky progress comment into two steps while the base is +# built from scratch: that is the slow path, and the reader should know why. +# Usage: post-progress.sh base|head . Best effort throughout. +set -euo pipefail +step="$1" elapsed="$2" +[ -n "${PROGRESS_HEADER:-}" ] && [ -n "${PR_NUMBER:-}" ] && [ -n "${REPOSITORY:-}" ] || exit 0 +export GH_HOST="${GH_HOST:-github.com}" +GH_HOST="${GH_HOST#*://}" + +# The sticky-comment action finds its comment by this line, so it must survive the edit. +marker="" +id_file="${RUNNER_TEMP:?}/codeboarding-progress-comment-${PROGRESS_HEADER}" +id="$(cat "$id_file" 2>/dev/null || true)" +if [ -z "$id" ]; then + id="$(gh api --paginate "repos/$REPOSITORY/issues/$PR_NUMBER/comments?per_page=100" \ + --jq ".[] | select((.body // \"\") | startswith(\"### CodeBoarding review\") and contains(\"$marker\")) | .id" | + tail -n 1)" + [ -n "$id" ] || exit 0 + echo "$id" > "$id_file" +fi + +if [ -n "${BASE_REF:-}" ]; then + branch="\`$BASE_REF\`" +else + branch="the base branch" +fi +sha7="${REVIEW_BASE_SHA:0:7}" +case "${BASE_REASON:-}" in + incompatible) why="The saved diagram of $branch was made by a different engine version or settings, so this review builds a new one." ;; + too_far_behind) why="The nearest saved diagram of $branch is too far behind this pull request's base, so this review builds one from scratch." ;; + *) why="$branch has no saved diagram yet, so this review builds one first. Once a diagram of $branch is saved, reviews start from it and skip this step." ;; +esac +minutes=$(( elapsed / 60 )) +if [ "$step" = base ]; then + running="running for $minutes min" + [ "$minutes" -gt 0 ] || running="running for less than a minute" + first="1. ⏳ Building the diagram of $branch @$sha7 from scratch · $running" + second="2. Analysing this PR's changes" +else + took="$(( elapsed % 60 )) s" + [ "$minutes" -eq 0 ] || took="$minutes m $took" + first="1. ✅ Built the diagram of $branch @$sha7 from scratch in $took" + second="2. ⏳ Analysing this PR's changes" +fi + +platform="https://app.codeboarding.org/$REPOSITORY/pull/$PR_NUMBER?utm_source=github&utm_medium=pr_comment&utm_campaign=gh_action" +run_url="${GITHUB_SERVER_URL:-https://github.com}/$REPOSITORY/actions/runs/${GITHUB_RUN_ID:-}" +body="$(printf '%s\n\n%s\n %s\n%s\n\n%s\n\n%s\n%s' \ + '### CodeBoarding review · analyzing…' \ + "$first" "$why" "$second" \ + "Open it in [CodeBoarding]($platform) meanwhile: the files, comments and review are there already, and the diff appears when the run finishes." \ + "run [${GITHUB_RUN_ID:-}]($run_url) · attempt ${GITHUB_RUN_ATTEMPT:-1}" \ + "$marker")" +gh api -X PATCH "repos/$REPOSITORY/issues/comments/$id" -f body="$body" >/dev/null diff --git a/tests/test_action_state.py b/tests/test_action_state.py index 7b5a8aa..1e53262 100644 --- a/tests/test_action_state.py +++ b/tests/test_action_state.py @@ -312,6 +312,162 @@ def test_base_and_head_fallbacks_keep_configured_depth(self) -> None: self.assertEqual([c["mode"] for c in calls], ["incremental", "full", "incremental", "full"]) self.assertEqual([c["depth"] for c in calls if c["mode"] == "full"], ["4", "4"]) + # How the base was obtained is reported, not just used: the review comment and + # the webview explain a slow run by it. + + def _git(self, *args: str) -> str: + return subprocess.run( + [ + "git", + "-C", + str(self.checkout), + "-c", + "user.name=Test", + "-c", + "user.email=test@example.com", + "-c", + "commit.gpgsign=false", + *args, + ], + check=True, + capture_output=True, + text=True, + ).stdout.strip() + + def _commit(self, message: str, files: dict[str, str]) -> str: + for name, content in files.items(): + (self.checkout / name).parent.mkdir(parents=True, exist_ok=True) + (self.checkout / name).write_text(content, encoding="utf-8") + self._git("add", "-A") + self._git("commit", "-q", "-m", message) + return self._git("rev-parse", "HEAD") + + def _sync_history(self) -> tuple[str, str]: + """An analysed commit with a sync commit on top that writes only .codeboarding/.""" + self._git("init", "-q") + analysed = self._commit("feat: code", {"code.py": "pass\n"}) + _state(self.checkout / ".codeboarding", cap=2) + sync = self._commit("chore(codeboarding): sync analysis baseline", {}) + return analysed, sync + + def _provenance(self, values: dict[str, str]) -> dict[str, str]: + keys = ("base_source", "base_reason", "base_from_sha", "catchup_commits") + for key in ("base_seconds", "head_seconds"): + self.assertRegex(values[key], r"^[0-9]+$", key) + return {key: values[key] for key in keys} + + def test_a_saved_base_reports_itself_as_exact(self) -> None: + _state(self.base_dir) + + values = self._analyze(BASE_FETCH_SECONDS="7") + + self.assertEqual( + self._provenance(values), + {"base_source": "saved", "base_reason": "", "base_from_sha": "merge-base-sha", "catchup_commits": "0"}, + ) + # The download happened in the step before; it is still time spent on the base. + self.assertGreaterEqual(int(values["base_seconds"]), 7) + + def test_a_base_with_nothing_to_seed_it_is_computed_for_lack_of_a_baseline(self) -> None: + sha = self._commit_base() + + values = self._analyze(REVIEW_BASE_SHA=sha) + + self.assertEqual( + self._provenance(values), + {"base_source": "computed", "base_reason": "no_baseline", "base_from_sha": "", "catchup_commits": ""}, + ) + + def test_a_committed_baseline_with_another_depth_is_incompatible(self) -> None: + sha = self._commit_base(legacy=True) + + values = self._analyze(REVIEW_BASE_SHA=sha) + + self.assertEqual(values["base_source"], "computed") + self.assertEqual(values["base_reason"], "incompatible") + + def test_a_saved_base_with_another_depth_is_incompatible(self) -> None: + sha = self._commit_base() + _state(self.base_dir, cap=1) + + values = self._analyze(REVIEW_BASE_SHA=sha) + + self.assertEqual(values["base_source"], "computed") + self.assertEqual(values["base_reason"], "incompatible") + + def test_an_engine_that_demands_a_full_run_makes_the_baseline_incompatible(self) -> None: + sha = self._commit_base(cap=2) + + values = self._analyze(REVIEW_BASE_SHA=sha, CB_REQUIRE_FULL="true") + + self.assertEqual(values["base_source"], "computed") + self.assertEqual(values["base_reason"], "incompatible") + + def test_a_baseline_committed_at_the_merge_base_needs_no_catching_up(self) -> None: + # The sync commit changes only .codeboarding/, which the analysis ignores, + # so a merge base on it is exactly the commit the baseline describes. + analysed, sync = self._sync_history() + + values = self._analyze(REVIEW_BASE_SHA=sync) + + self.assertEqual( + self._provenance(values), + {"base_source": "committed", "base_reason": "", "base_from_sha": analysed, "catchup_commits": "0"}, + ) + self.assertEqual(values["publish_base"], "true") + + def test_a_committed_baseline_counts_the_commits_it_caught_up(self) -> None: + analysed, _sync = self._sync_history() + self._commit("feat: more", {"more.py": "pass\n"}) + merge_base = self._commit("feat: again", {"code.py": "print()\n"}) + + values = self._analyze(REVIEW_BASE_SHA=merge_base) + + self.assertEqual(values["base_source"], "committed") + self.assertEqual(values["base_from_sha"], analysed) + self.assertEqual(values["catchup_commits"], "2") + + def _progress_stub(self) -> Path: + calls = self.root / "gh-calls" + gh = self.bin_dir / "gh" + gh.write_text( + "#!/usr/bin/env bash\n" + f'printf "%s\\n----\\n" "$*" >> "{calls}"\n' + 'case "$*" in *"/comments?per_page"*) echo 77 ;; esac\n', + encoding="utf-8", + ) + gh.chmod(0o755) + return calls + + def test_building_the_base_from_scratch_rewrites_the_progress_comment(self) -> None: + calls = self._progress_stub() + sha = self._commit_base() + + self._analyze( + REVIEW_BASE_SHA=sha, + PROGRESS_HEADER="codeboarding-review", + REPOSITORY="owner/repo", + BASE_REF="develop", + GIT_TOKEN="token", + ) + + patches = [call for call in calls.read_text().split("\n----\n") if "PATCH" in call] + self.assertGreaterEqual(len(patches), 2, calls.read_text()) + self.assertIn("repos/owner/repo/issues/comments/77", patches[0]) + self.assertIn(f"Building the diagram of `develop` @{sha[:7]} from scratch", patches[0]) + self.assertIn("`develop` has no saved diagram yet", patches[0]) + self.assertIn("2. ⏳ Analysing this PR's changes", patches[-1]) + # The sticky-comment action finds its comment by this line on the final write. + self.assertIn("", patches[-1]) + + def test_a_saved_base_leaves_the_progress_comment_alone(self) -> None: + calls = self._progress_stub() + _state(self.base_dir) + + self._analyze(PROGRESS_HEADER="codeboarding-review", REPOSITORY="owner/repo", GIT_TOKEN="token") + + self.assertFalse(calls.exists()) + def test_invalid_depth_fails_before_analysis(self) -> None: result = subprocess.run( [str(ANALYZE)], diff --git a/tests/test_progress_comment.py b/tests/test_progress_comment.py new file mode 100644 index 0000000..f96c2bf --- /dev/null +++ b/tests/test_progress_comment.py @@ -0,0 +1,84 @@ +"""The progress comment while a base is built from scratch: two steps, elapsed time only.""" + +import os +import subprocess +import tempfile +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +POST_PROGRESS = ROOT / "scripts" / "action" / "post-progress.sh" + + +class ProgressCommentTests(unittest.TestCase): + def setUp(self) -> None: + self.temp_dir = tempfile.TemporaryDirectory() + self.root = Path(self.temp_dir.name) + self.bin_dir = self.root / "bin" + self.bin_dir.mkdir() + self.calls = self.root / "calls" + + def tearDown(self) -> None: + self.temp_dir.cleanup() + + def _post(self, step: str, elapsed: int, comment_id: str = "77", **extra: str) -> list[str]: + gh = self.bin_dir / "gh" + gh.write_text( + "#!/usr/bin/env bash\n" + f'printf "%s\\n----\\n" "$*" >> "{self.calls}"\n' + f'case "$*" in *"/comments?per_page"*) echo "{comment_id}" ;; esac\n', + encoding="utf-8", + ) + gh.chmod(0o755) + subprocess.run( + [str(POST_PROGRESS), step, str(elapsed)], + env={ + "PATH": f"{self.bin_dir}:{os.environ['PATH']}", + "RUNNER_TEMP": str(self.root), + "PROGRESS_HEADER": "codeboarding-review", + "PR_NUMBER": "9", + "REPOSITORY": "owner/repo", + "BASE_REF": "main", + "REVIEW_BASE_SHA": "a1b2c3d4e5f6", + "GITHUB_RUN_ID": "55", + **extra, + }, + check=True, + capture_output=True, + text=True, + ) + return [c for c in self.calls.read_text().split("\n----\n") if "PATCH" in c] if self.calls.exists() else [] + + def test_the_base_step_shows_elapsed_minutes_and_the_reason(self) -> None: + (patch,) = self._post("base", 125, BASE_REASON="no_baseline") + self.assertIn("1. ⏳ Building the diagram of `main` @a1b2c3d from scratch · running for 2 min\n", patch) + self.assertIn(" `main` has no saved diagram yet, so this review builds one first.", patch) + self.assertIn("\n2. Analysing this PR's changes\n", patch) + self.assertNotIn("estimate", patch.lower()) + + def test_the_first_minute_does_not_read_as_zero(self) -> None: + (patch,) = self._post("base", 0) + self.assertIn("running for less than a minute", patch) + + def test_an_incompatible_base_says_why(self) -> None: + (patch,) = self._post("base", 60, BASE_REASON="incompatible") + self.assertIn("The saved diagram of `main` was made by a different engine version or settings", patch) + + def test_the_head_step_reports_the_measured_base_time(self) -> None: + (patch,) = self._post("head", 534) + self.assertIn("1. ✅ Built the diagram of `main` @a1b2c3d from scratch in 8 m 54 s\n", patch) + self.assertIn("2. ⏳ Analysing this PR's changes", patch) + self.assertTrue(patch.rstrip().endswith("")) + + def test_the_comment_is_looked_up_once(self) -> None: + self._post("base", 0) + self._post("base", 60) + lookups = [c for c in self.calls.read_text().split("\n----\n") if "/comments?per_page" in c] + self.assertEqual(len(lookups), 1) + + def test_no_progress_comment_means_no_edit(self) -> None: + self.assertEqual(self._post("base", 0, comment_id=""), []) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_review_artifact_metadata.py b/tests/test_review_artifact_metadata.py index 6b43526..a9bd90c 100644 --- a/tests/test_review_artifact_metadata.py +++ b/tests/test_review_artifact_metadata.py @@ -65,6 +65,46 @@ def test_a_count_the_render_step_could_not_make_is_recorded_as_unknown(self) -> metadata, _outputs = _build(Path(tmp)) self.assertEqual(metadata["analysed_files_changed"], "unknown") + def test_how_the_base_was_obtained_is_recorded_as_strings(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + metadata, _outputs = _build( + Path(tmp), + BASE_SOURCE="committed", + BASE_REASON="", + BASE_FROM_SHA="abc", + CATCHUP_COMMITS="3", + BASE_SECONDS="41", + HEAD_SECONDS="159", + ) + self.assertEqual( + {key: metadata[key] for key in metadata if key.startswith("base_") or key.endswith("_seconds")} + | {"catchup_commits": metadata["catchup_commits"]}, + { + "base_sha": "tip-sha", + "base_artifact": "codeboarding-base-cfg-mergebasesha", + "base_artifact_id": "4242", + "base_source": "committed", + "base_reason": "", + "base_from_sha": "abc", + "catchup_commits": "3", + "base_seconds": "41", + "head_seconds": "159", + }, + ) + + def test_an_older_run_without_provenance_records_empty_strings(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + metadata, _outputs = _build(Path(tmp)) + for key in ( + "base_source", + "base_reason", + "base_from_sha", + "catchup_commits", + "base_seconds", + "head_seconds", + ): + self.assertEqual(metadata[key], "", key) + if __name__ == "__main__": unittest.main() diff --git a/tests/test_review_comment.py b/tests/test_review_comment.py index 709f693..f88a67a 100644 --- a/tests/test_review_comment.py +++ b/tests/test_review_comment.py @@ -82,5 +82,71 @@ def test_a_changed_analysed_file_gets_no_verdict_even_at_zero_components(self) - self.assertIn("changed=0 analysed_files_changed=2 head=abc123", body) +class BaseLineTests(unittest.TestCase): + """One line saying how the base was obtained, with measured times and never an estimate.""" + + def _base_line(self, **extra: str) -> str: + with tempfile.TemporaryDirectory() as tmp: + body = _build(Path(tmp), BASE_REF="main", MERGE_BASE_SHA="f00dfeed" * 5, **extra) + lines = [line for line in body.splitlines() if line.startswith("Base: ")] + self.assertEqual(len(lines), 1, body) + return lines[0] + + def test_a_computed_base_says_why_and_how_long(self) -> None: + line = self._base_line( + BASE_SOURCE="computed", BASE_REASON="no_baseline", BASE_SECONDS="534", HEAD_SECONDS="192" + ) + self.assertEqual(line, "Base: built from scratch (no saved diagram), 8 m 54 s · changes 3 m 12 s") + + def test_an_incompatible_base_says_so(self) -> None: + line = self._base_line(BASE_SOURCE="computed", BASE_REASON="incompatible", BASE_SECONDS="60", HEAD_SECONDS="5") + self.assertEqual( + line, "Base: built from scratch (saved diagram incompatible), 1 m 0 s · changes 5 s" + ) + + def test_a_saved_base_names_the_commit(self) -> None: + line = self._base_line( + BASE_SOURCE="saved", BASE_FROM_SHA="a1b2c3d4e5", CATCHUP_COMMITS="0", BASE_SECONDS="3", HEAD_SECONDS="159" + ) + self.assertEqual(line, "Base: saved diagram of main @a1b2c3d · changes 2 m 39 s") + + def test_a_committed_base_at_the_merge_base_reads_as_saved(self) -> None: + line = self._base_line(BASE_SOURCE="committed", BASE_FROM_SHA="", CATCHUP_COMMITS="", HEAD_SECONDS="4") + self.assertEqual(line, "Base: saved diagram of main @f00dfee · changes 4 s") + + def test_a_caught_up_base_counts_the_commits(self) -> None: + line = self._base_line( + BASE_SOURCE="committed", + BASE_FROM_SHA="a1b2c3d4e5", + CATCHUP_COMMITS="4", + BASE_SECONDS="41", + HEAD_SECONDS="159", + ) + self.assertEqual(line, "Base: caught up 4 commits from main @a1b2c3d, 41 s · changes 2 m 39 s") + + def test_the_marker_carries_the_base_after_the_existing_keys(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + body = _build( + Path(tmp), BASE_SOURCE="computed", BASE_REASON="no_baseline", BASE_SECONDS="534", HEAD_SECONDS="192" + ) + self.assertTrue( + body.rstrip("\n").endswith( + "head=abc123 base=computed base_reason=no_baseline base_seconds=534 head_seconds=192 -->" + ), + body, + ) + + def test_the_marker_omits_an_empty_reason(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + body = _build(Path(tmp), BASE_SOURCE="saved", BASE_REASON="", BASE_SECONDS="2", HEAD_SECONDS="9") + self.assertIn("head=abc123 base=saved base_seconds=2 head_seconds=9 -->", body) + + def test_without_a_base_source_there_is_no_line_and_no_keys(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + body = _build(Path(tmp)) + self.assertNotIn("Base:", body) + self.assertNotIn(" base=", body) + + if __name__ == "__main__": unittest.main() From 74d36cee6e53f0550700e48a9fd459ce0e729d3a Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Wed, 7 Oct 2026 17:29:13 +0200 Subject: [PATCH 2/3] fix: count merges as catch-up and never call an unknown base exact Review follow-ups on the base provenance: - code_paths_changed diffs against the first parent, so a --no-ff merge on the first-parent chain counts as the change it brought in; diff-tree printed nothing for merges and catch-up read 0. .gitattributes is not code either. - base_from_sha for a committed baseline comes only from a commit sync made (its bot committer, nothing but .codeboarding/): pushed directly, or on the second-parent side of a merged sync pull request. Anything else (a squash, a hand edit) leaves it empty rather than guessing. - An empty sha or count is unknown, not 0: the comment then says "saved diagram of , caught up" without a sha, and an ancestor never reads as "saved". - The progress ticker stops through a stop file and is waited for, and post-progress.sh checks the file right before editing, so a stale "running" edit cannot land after step 2. Co-Authored-By: Claude Opus 5.5 --- action.yml | 1 - scripts/action/analyze.sh | 61 ++++++++++++++++------ scripts/action/build-review-comment.sh | 26 +++++++--- scripts/action/post-progress.sh | 5 ++ tests/test_action_state.py | 72 ++++++++++++++++++++++++-- tests/test_progress_comment.py | 7 +++ tests/test_review_comment.py | 17 +++++- 7 files changed, 157 insertions(+), 32 deletions(-) diff --git a/action.yml b/action.yml index dc805e0..420559f 100644 --- a/action.yml +++ b/action.yml @@ -654,7 +654,6 @@ runs: 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 }} - MERGE_BASE_SHA: ${{ steps.guard.outputs.merge_base_sha }} BASE_SOURCE: ${{ steps.review_analyze.outputs.base_source }} BASE_REASON: ${{ steps.review_analyze.outputs.base_reason }} BASE_FROM_SHA: ${{ steps.review_analyze.outputs.base_from_sha }} diff --git a/scripts/action/analyze.sh b/scripts/action/analyze.sh index 228f6b1..7283373 100755 --- a/scripts/action/analyze.sh +++ b/scripts/action/analyze.sh @@ -155,12 +155,24 @@ fetch_commit() { "${GITHUB_SERVER_URL%/}/${repository}.git" "$sha" --depth="$depth" } -# The commit a baseline committed at $1 describes. Sync writes it in a commit of -# its own on top of the analysed one, so that commit's parent is the answer. -# Empty when nothing within the fetched history wrote it. +# The commit a baseline committed at $1 describes, or empty when that cannot be +# told. Sync writes the baseline in a commit of its own on top of the analysed +# commit, pushed straight to the branch or merged in from its pull request's +# branch. A writer sync did not make (a squash, a rebase, a hand edit) says +# nothing about which commit was analysed. baseline_commit() { - local tip="$1" writer shallow - writer="$(git -C "$CHECKOUT_DIR" log --first-parent -1 --format=%H "$tip" -- .codeboarding/analysis.json 2>/dev/null || true)" + local writer + writer="$(baseline_writer "$1")" + if [ -n "$writer" ] && git -C "$CHECKOUT_DIR" rev-parse -q --verify "$writer^2" >/dev/null; then + writer="$(baseline_writer "$writer^2")" + fi + [ -n "$writer" ] && is_sync_commit "$writer" || return 0 + git -C "$CHECKOUT_DIR" rev-parse -q --verify "$writer^1" || true +} +# The newest first-parent commit at or below $1 that wrote the baseline. +baseline_writer() { + local writer shallow + writer="$(git -C "$CHECKOUT_DIR" log --first-parent -1 --format=%H "$1" -- .codeboarding/analysis.json 2>/dev/null || true)" [ -n "$writer" ] || return 0 # A shallow boundary looks like it added every file, so it proves nothing. shallow="$(git -C "$CHECKOUT_DIR" rev-parse --git-path shallow)" @@ -168,17 +180,27 @@ baseline_commit() { if [ -f "$shallow" ] && grep -qx "$writer" "$shallow"; then return 0 fi - if [ -z "$(code_paths_changed "$writer")" ] && git -C "$CHECKOUT_DIR" rev-parse -q --verify "$writer^1" >/dev/null; then - git -C "$CHECKOUT_DIR" rev-parse "$writer^1" - else - echo "$writer" - fi + echo "$writer" } +is_sync_commit() { + case "$(git -C "$CHECKOUT_DIR" log -1 --format=%ce "$1")" in + 'codeboarding-review[bot]@users.noreply.github.com' | 'codeboarding[bot]@users.noreply.github.com') ;; + *) return 1 ;; + esac + [ -z "$(code_paths_changed "$1")" ] +} +# Against the first parent, so a merge counts as the change it brought in. The +# baseline and the attributes line sync may add are not code. code_paths_changed() { - git -C "$CHECKOUT_DIR" diff-tree --no-commit-id --name-only -r "$1" -- . ':(exclude).codeboarding' 2>/dev/null || true + local exclude=(-- . ':(exclude).codeboarding' ':(exclude).gitattributes') + if git -C "$CHECKOUT_DIR" rev-parse -q --verify "$1^1" >/dev/null; then + git -C "$CHECKOUT_DIR" diff --name-only "$1^1" "$1" "${exclude[@]}" 2>/dev/null || true + else + git -C "$CHECKOUT_DIR" diff-tree --root --no-commit-id --name-only -r "$1" "${exclude[@]}" 2>/dev/null || true + fi } -# First-parent commits from $1 to $2 that change anything outside .codeboarding/: -# a sync commit changes nothing the analysis reads, so it is nothing to catch up. +# First-parent commits from $1 to $2 that change code: a sync commit changes +# nothing the analysis reads, so it is nothing to catch up. catchup_count() { local from="$1" to="$2" commit count=0 for commit in $(git -C "$CHECKOUT_DIR" rev-list --first-parent "$from..$to" 2>/dev/null); do @@ -190,20 +212,25 @@ catchup_count() { # 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="" +PROGRESS_STOP="${RUNNER_TEMP:-}/codeboarding-progress-stop" progress() { GH_TOKEN="${GIT_TOKEN:-}" GH_ENTERPRISE_TOKEN="${GIT_TOKEN:-}" BASE_REASON="$base_reason" \ - "$ACTION_PATH/scripts/action/post-progress.sh" "$@" >/dev/null 2>&1 || true + PROGRESS_STOP_FILE="$PROGRESS_STOP" "$ACTION_PATH/scripts/action/post-progress.sh" "$@" >/dev/null 2>&1 || true } progress_start() { local started="$1" + rm -f "$PROGRESS_STOP" progress base 0 - ( while sleep 60; do progress base "$(( $(date +%s) - started ))"; done ) >/dev/null 2>&1 & + ( while sleep 60 && [ ! -e "$PROGRESS_STOP" ]; do progress base "$(( $(date +%s) - started ))"; done ) >/dev/null 2>&1 & PROGRESS_PID=$! } +# Stops the ticker and waits for it, so no "still running" edit can land after the +# next one. Only its sleep is killed: an edit in flight finishes or, having seen +# the stop file, never starts. progress_stop() { [ -n "$PROGRESS_PID" ] || return 0 - pkill -P "$PROGRESS_PID" 2>/dev/null || true - kill "$PROGRESS_PID" 2>/dev/null || true + touch "$PROGRESS_STOP" + pkill -x sleep -P "$PROGRESS_PID" 2>/dev/null || true wait "$PROGRESS_PID" 2>/dev/null || true PROGRESS_PID="" } diff --git a/scripts/action/build-review-comment.sh b/scripts/action/build-review-comment.sh index a1869fa..51e2ea0 100755 --- a/scripts/action/build-review-comment.sh +++ b/scripts/action/build-review-comment.sh @@ -58,9 +58,14 @@ duration() { } BASE_SOURCE="${BASE_SOURCE:-}" BASE_BRANCH="${BASE_REF:-the base branch}" -BASE_AT="${BASE_FROM_SHA:-${MERGE_BASE_SHA:-}}" +BASE_FROM="${BASE_FROM_SHA:-}" +CATCHUP="${CATCHUP_COMMITS:-}" +case "$CATCHUP" in *[!0-9]*) CATCHUP="" ;; esac CHANGES="changes $(duration "${HEAD_SECONDS:-}")" +TOOK="$(duration "${BASE_SECONDS:-}")" BASE_LINE="" +# An empty sha or count is unknown, not zero: the line then says it caught up +# without claiming the base was exact. case "$BASE_SOURCE" in computed) case "${BASE_REASON:-}" in @@ -68,17 +73,22 @@ case "$BASE_SOURCE" in too_far_behind) WHY="saved diagram too far behind" ;; *) WHY="no saved diagram" ;; esac - BASE_LINE="Base: built from scratch (${WHY}), $(duration "${BASE_SECONDS:-}") · ${CHANGES}" + BASE_LINE="Base: built from scratch (${WHY}), ${TOOK} · ${CHANGES}" ;; - saved | committed | ancestor) - CATCHUP="${CATCHUP_COMMITS:-}" - case "$CATCHUP" in ''|*[!0-9]*) CATCHUP=0 ;; esac - if [ "$CATCHUP" -gt 0 ]; then + saved) + BASE_LINE="Base: saved diagram of ${BASE_BRANCH} @${BASE_FROM:0:7} · ${CHANGES}" + ;; + committed | ancestor) + if [ -z "$BASE_FROM" ] || [ -z "$CATCHUP" ]; then + BASE_LINE="Base: saved diagram of ${BASE_BRANCH}, caught up, ${TOOK} · ${CHANGES}" + elif [ "$CATCHUP" -gt 0 ]; then COMMIT_NOUN="commits" [ "$CATCHUP" != 1 ] || COMMIT_NOUN="commit" - BASE_LINE="Base: caught up ${CATCHUP} ${COMMIT_NOUN} from ${BASE_BRANCH} @${BASE_AT:0:7}, $(duration "${BASE_SECONDS:-}") · ${CHANGES}" + BASE_LINE="Base: caught up ${CATCHUP} ${COMMIT_NOUN} from ${BASE_BRANCH} @${BASE_FROM:0:7}, ${TOOK} · ${CHANGES}" + elif [ "$BASE_SOURCE" = ancestor ]; then + BASE_LINE="Base: caught up from ${BASE_BRANCH} @${BASE_FROM:0:7}, ${TOOK} · ${CHANGES}" else - BASE_LINE="Base: saved diagram of ${BASE_BRANCH} @${BASE_AT:0:7} · ${CHANGES}" + BASE_LINE="Base: saved diagram of ${BASE_BRANCH} @${BASE_FROM:0:7} · ${CHANGES}" fi ;; esac diff --git a/scripts/action/post-progress.sh b/scripts/action/post-progress.sh index 746fcbe..ce5471d 100755 --- a/scripts/action/post-progress.sh +++ b/scripts/action/post-progress.sh @@ -52,4 +52,9 @@ body="$(printf '%s\n\n%s\n %s\n%s\n\n%s\n\n%s\n%s' \ "Open it in [CodeBoarding]($platform) meanwhile: the files, comments and review are there already, and the diff appears when the run finishes." \ "run [${GITHUB_RUN_ID:-}]($run_url) · attempt ${GITHUB_RUN_ATTEMPT:-1}" \ "$marker")" +# Checked last: the ticker may have been stopped while this ran, and a stale +# "running" edit must not land on top of the next step. +if [ "$step" = base ] && [ -n "${PROGRESS_STOP_FILE:-}" ] && [ -e "$PROGRESS_STOP_FILE" ]; then + exit 0 +fi gh api -X PATCH "repos/$REPOSITORY/issues/comments/$id" -f body="$body" >/dev/null diff --git a/tests/test_action_state.py b/tests/test_action_state.py index 1e53262..8008eef 100644 --- a/tests/test_action_state.py +++ b/tests/test_action_state.py @@ -334,22 +334,31 @@ def _git(self, *args: str) -> str: text=True, ).stdout.strip() - def _commit(self, message: str, files: dict[str, str]) -> str: + def _commit(self, message: str, files: dict[str, str], *, bot: bool = False) -> str: for name, content in files.items(): (self.checkout / name).parent.mkdir(parents=True, exist_ok=True) (self.checkout / name).write_text(content, encoding="utf-8") self._git("add", "-A") - self._git("commit", "-q", "-m", message) + committer = ("-c", "user.email=codeboarding-review[bot]@users.noreply.github.com") if bot else () + self._git(*committer, "commit", "-q", "-m", message) return self._git("rev-parse", "HEAD") def _sync_history(self) -> tuple[str, str]: """An analysed commit with a sync commit on top that writes only .codeboarding/.""" - self._git("init", "-q") + self._git("init", "-q", "-b", "main") analysed = self._commit("feat: code", {"code.py": "pass\n"}) _state(self.checkout / ".codeboarding", cap=2) - sync = self._commit("chore(codeboarding): sync analysis baseline", {}) + sync = self._commit("chore(codeboarding): sync analysis baseline", {}, bot=True) return analysed, sync + def _merge(self, branch: str, files: dict[str, str], *, bot: bool = False) -> str: + """A commit on `branch` merged into main with --no-ff.""" + self._git("checkout", "-q", "-b", branch) + self._commit(f"work on {branch}", files, bot=bot) + self._git("checkout", "-q", "main") + self._git("merge", "-q", "--no-ff", "-m", f"Merge {branch}", branch) + return self._git("rev-parse", "HEAD") + def _provenance(self, values: dict[str, str]) -> dict[str, str]: keys = ("base_source", "base_reason", "base_from_sha", "catchup_commits") for key in ("base_seconds", "head_seconds"): @@ -427,6 +436,61 @@ def test_a_committed_baseline_counts_the_commits_it_caught_up(self) -> None: self.assertEqual(values["base_from_sha"], analysed) self.assertEqual(values["catchup_commits"], "2") + def test_merged_pull_requests_count_as_commits_to_catch_up(self) -> None: + # Merged with --no-ff, the first-parent chain is merge commits only, and + # each one brings code in even though it changes nothing against itself. + analysed, _sync = self._sync_history() + self._merge("feature-a", {"a.py": "pass\n"}) + merge_base = self._merge("feature-b", {"b.py": "pass\n"}) + + values = self._analyze(REVIEW_BASE_SHA=merge_base) + + self.assertEqual(values["base_from_sha"], analysed) + self.assertEqual(values["catchup_commits"], "2") + + def test_a_merged_sync_pull_request_describes_its_own_parent(self) -> None: + # sync_strategy: pull_request. The sync commit sits on codeboarding/sync on + # top of the analysed commit; main moved on before the merge. + self._git("init", "-q", "-b", "main") + analysed = self._commit("feat: code", {"code.py": "pass\n"}) + self._git("checkout", "-q", "-b", "codeboarding/sync") + _state(self.checkout / ".codeboarding", cap=2) + self._commit("chore(codeboarding): sync analysis baseline", {}, bot=True) + self._git("checkout", "-q", "main") + self._commit("feat: meanwhile", {"later.py": "pass\n"}) + self._git("merge", "-q", "--no-ff", "-m", "Merge codeboarding/sync", "codeboarding/sync") + merge_base = self._git("rev-parse", "HEAD") + + values = self._analyze(REVIEW_BASE_SHA=merge_base) + + self.assertEqual(values["base_source"], "committed") + self.assertEqual(values["base_from_sha"], analysed) + self.assertEqual(values["catchup_commits"], "1") + + def test_a_baseline_sync_did_not_write_is_of_unknown_origin(self) -> None: + # A squash or a hand edit: its parent is not known to be what was analysed. + self._git("init", "-q", "-b", "main") + self._commit("feat: code", {"code.py": "pass\n"}) + _state(self.checkout / ".codeboarding", cap=2) + merge_base = self._commit("chore(codeboarding): sync analysis baseline (#7)", {}) + + values = self._analyze(REVIEW_BASE_SHA=merge_base) + + self.assertEqual(values["base_source"], "committed") + self.assertEqual(values["base_from_sha"], "") + self.assertEqual(values["catchup_commits"], "") + + def test_an_attributes_line_is_not_code_to_catch_up(self) -> None: + analysed, _sync = self._sync_history() + merge_base = self._commit( + "chore: attributes", {".gitattributes": ".codeboarding/analysis.json linguist-generated=true\n"} + ) + + values = self._analyze(REVIEW_BASE_SHA=merge_base) + + self.assertEqual(values["base_from_sha"], analysed) + self.assertEqual(values["catchup_commits"], "0") + def _progress_stub(self) -> Path: calls = self.root / "gh-calls" gh = self.bin_dir / "gh" diff --git a/tests/test_progress_comment.py b/tests/test_progress_comment.py index f96c2bf..034dd72 100644 --- a/tests/test_progress_comment.py +++ b/tests/test_progress_comment.py @@ -76,6 +76,13 @@ def test_the_comment_is_looked_up_once(self) -> None: lookups = [c for c in self.calls.read_text().split("\n----\n") if "/comments?per_page" in c] self.assertEqual(len(lookups), 1) + def test_a_stopped_ticker_never_edits_after_the_next_step(self) -> None: + stop = self.root / "stop" + stop.touch() + self.assertEqual(self._post("base", 120, PROGRESS_STOP_FILE=str(stop)), []) + # The step-2 edit is the one that follows the stop, so it still lands. + self.assertEqual(len(self._post("head", 120, PROGRESS_STOP_FILE=str(stop))), 1) + def test_no_progress_comment_means_no_edit(self) -> None: self.assertEqual(self._post("base", 0, comment_id=""), []) diff --git a/tests/test_review_comment.py b/tests/test_review_comment.py index f88a67a..9df2135 100644 --- a/tests/test_review_comment.py +++ b/tests/test_review_comment.py @@ -111,8 +111,21 @@ def test_a_saved_base_names_the_commit(self) -> None: self.assertEqual(line, "Base: saved diagram of main @a1b2c3d · changes 2 m 39 s") def test_a_committed_base_at_the_merge_base_reads_as_saved(self) -> None: - line = self._base_line(BASE_SOURCE="committed", BASE_FROM_SHA="", CATCHUP_COMMITS="", HEAD_SECONDS="4") - self.assertEqual(line, "Base: saved diagram of main @f00dfee · changes 4 s") + line = self._base_line(BASE_SOURCE="committed", BASE_FROM_SHA="a1b2c3d4", CATCHUP_COMMITS="0", HEAD_SECONDS="4") + self.assertEqual(line, "Base: saved diagram of main @a1b2c3d · changes 4 s") + + def test_an_unknown_catch_up_does_not_claim_an_exact_base(self) -> None: + for sha, count in (("", ""), ("a1b2c3d4", ""), ("", "3")): + line = self._base_line( + BASE_SOURCE="committed", BASE_FROM_SHA=sha, CATCHUP_COMMITS=count, BASE_SECONDS="41", HEAD_SECONDS="4" + ) + self.assertEqual(line, "Base: saved diagram of main, caught up, 41 s · changes 4 s") + + def test_an_ancestor_never_reads_as_saved(self) -> None: + line = self._base_line( + BASE_SOURCE="ancestor", BASE_FROM_SHA="a1b2c3d4", CATCHUP_COMMITS="0", BASE_SECONDS="41", HEAD_SECONDS="4" + ) + self.assertEqual(line, "Base: caught up from main @a1b2c3d, 41 s · changes 4 s") def test_a_caught_up_base_counts_the_commits(self) -> None: line = self._base_line( From 6b46ac320faed2ae77f5998bccace51b4d56f383 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Fri, 9 Oct 2026 23:03:47 +0200 Subject: [PATCH 3/3] refactor: report the base analysis as reused, incremental or full Review feedback: base_source/base_reason/base_from_sha/catchup_commits used "base" for too many things. A review compares the base commit's analysis with the head's; what matters is how the base analysis was obtained. - base_analysis_method: reused | incremental | full (replaces base_source). A committed baseline with nothing to catch up is reused. - base_analysis_reason: the method in words; the starting commit and the number of commits caught up are detail in it, not fields of their own. - base_seconds / head_seconds unchanged. - The comment's Base: line moves under the diagram, above the run links, and the marker carries base_analysis_method instead of base/base_reason. - Drops the too_far_behind and ancestor wording, which belong to the ancestor lookup in the next PR. Co-Authored-By: Claude Opus 5.5 --- action.yml | 18 ++--- docs/COMMIT_STRATEGY.md | 15 ++-- scripts/action/analyze.sh | 60 +++++++++++---- scripts/action/build-review-artifact.sh | 14 ++-- scripts/action/build-review-comment.sh | 60 +++++---------- scripts/action/post-progress.sh | 3 +- tests/test_action_state.py | 85 ++++++++++++--------- tests/test_progress_comment.py | 4 +- tests/test_review_artifact_metadata.py | 24 ++---- tests/test_review_comment.py | 98 ++++++++++++------------- 10 files changed, 188 insertions(+), 193 deletions(-) diff --git a/action.yml b/action.yml index 420559f..7ca8012 100644 --- a/action.yml +++ b/action.yml @@ -183,9 +183,9 @@ outputs: seed_source: 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.' - value: ${{ steps.review_analyze.outputs.base_source }} + base_analysis_method: + description: 'How the analysis of the review base commit was obtained: reused, incremental, or full.' + value: ${{ steps.review_analyze.outputs.base_analysis_method }} merge_base_sha: description: 'Merge base used as the review comparison baseline.' value: ${{ steps.guard.outputs.merge_base_sha }} @@ -617,10 +617,8 @@ runs: 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 }} - BASE_SOURCE: ${{ steps.review_analyze.outputs.base_source }} - BASE_REASON: ${{ steps.review_analyze.outputs.base_reason }} - BASE_FROM_SHA: ${{ steps.review_analyze.outputs.base_from_sha }} - CATCHUP_COMMITS: ${{ steps.review_analyze.outputs.catchup_commits }} + BASE_ANALYSIS_METHOD: ${{ steps.review_analyze.outputs.base_analysis_method }} + BASE_ANALYSIS_REASON: ${{ steps.review_analyze.outputs.base_analysis_reason }} BASE_SECONDS: ${{ steps.review_analyze.outputs.base_seconds }} HEAD_SECONDS: ${{ steps.review_analyze.outputs.head_seconds }} run: "$GITHUB_ACTION_PATH/scripts/action/build-review-artifact.sh" @@ -654,10 +652,8 @@ runs: 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 }} - BASE_SOURCE: ${{ steps.review_analyze.outputs.base_source }} - BASE_REASON: ${{ steps.review_analyze.outputs.base_reason }} - BASE_FROM_SHA: ${{ steps.review_analyze.outputs.base_from_sha }} - CATCHUP_COMMITS: ${{ steps.review_analyze.outputs.catchup_commits }} + BASE_ANALYSIS_METHOD: ${{ steps.review_analyze.outputs.base_analysis_method }} + BASE_ANALYSIS_REASON: ${{ steps.review_analyze.outputs.base_analysis_reason }} BASE_SECONDS: ${{ steps.review_analyze.outputs.base_seconds }} HEAD_SECONDS: ${{ steps.review_analyze.outputs.head_seconds }} run: "$GITHUB_ACTION_PATH/scripts/action/build-review-comment.sh" diff --git a/docs/COMMIT_STRATEGY.md b/docs/COMMIT_STRATEGY.md index d188966..9f9fdcf 100644 --- a/docs/COMMIT_STRATEGY.md +++ b/docs/COMMIT_STRATEGY.md @@ -72,13 +72,11 @@ 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_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_analysis_method` | string | how this run obtained the analysis of the merge base: `reused` (the merge base already had one: its saved artifact, or a committed baseline with nothing to catch up), `incremental` (an earlier commit's analysis updated to the merge base) or `full` (analyzed from scratch in this run). Whichever, the base graph is the merge base's own analysis | +| `base_analysis_reason` | string | the method in words, e.g. `updated the analysis of 9f8e7d6 to a1b2c3d, 4 commits caught up`; where an incremental run started and how far it caught up are detail here, present when known | | `base_seconds` | string | wall time spent obtaining the base, the artifact lookup included | | `head_seconds` | string | wall time of the head analysis | -| `pr_number`, `mode`, `seed_source`, `chain_depth` | string | provenance; nothing rendering a diagram needs them. `mode` is the head's engine mode; `base_source` answers for the base | +| `pr_number`, `mode`, `seed_source`, `chain_depth` | string | provenance; nothing rendering a diagram needs them. `mode` is the head's engine mode; `base_analysis_method` answers for the base | **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 @@ -112,9 +110,10 @@ them: | no compatible committed baseline 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 -which one ran, with measured times. While a base is computed, the progress +forking from that commit gets the first row. The review metadata reports the +row as `base_analysis_method` (`reused`, `incremental` or `full`) with a +`base_analysis_reason`, and the review comment repeats both under the diagram, +with measured times. While a base is computed, the progress comment says so in two steps, with the elapsed time and the reason. The configuration hash includes `depth_cap`. The workflow input controls depth diff --git a/scripts/action/analyze.sh b/scripts/action/analyze.sh index 7283373..a7048e5 100755 --- a/scripts/action/analyze.sh +++ b/scripts/action/analyze.sh @@ -214,7 +214,7 @@ catchup_count() { PROGRESS_PID="" PROGRESS_STOP="${RUNNER_TEMP:-}/codeboarding-progress-stop" progress() { - GH_TOKEN="${GIT_TOKEN:-}" GH_ENTERPRISE_TOKEN="${GIT_TOKEN:-}" BASE_REASON="$base_reason" \ + GH_TOKEN="${GIT_TOKEN:-}" GH_ENTERPRISE_TOKEN="${GIT_TOKEN:-}" FULL_CAUSE="$full_cause" \ PROGRESS_STOP_FILE="$PROGRESS_STOP" "$ACTION_PATH/scripts/action/post-progress.sh" "$@" >/dev/null 2>&1 || true } progress_start() { @@ -262,16 +262,17 @@ analyze_review() { # A published base graph is this merge base's own analysis, named for it, so it # needs no engine run at all. Without one, the merge base is checked out and # analyzed from whatever baseline the repository committed there. Each path - # records how the base was obtained, for the comment and the review artifact. - local base_started base_source=saved base_reason="" base_from_sha="" catchup_commits="" + # records how the base analysis was obtained, for the comment and the review + # artifact: reused, incremental or full. + local base_started base_method=reused base_published=true full_cause="" base_from_sha="" catchup_commits="" base_started="$(date +%s)" if [ "$(depth_cap_from "${BASE_DIR:-}/analysis.json")" = "$DEPTH_CAP" ]; then mkdir -p "$base_state" cp -a "$BASE_DIR/." "$base_state/" - base_from_sha="$REVIEW_BASE_SHA" catchup_commits=0 else + base_published=false # A bundle under this exact name that the run cannot use was made with another cap. - [ ! -f "${BASE_DIR:-}/analysis.json" ] || base_reason=incompatible + [ ! -f "${BASE_DIR:-}/analysis.json" ] || full_cause=incompatible fetch_commit "$REVIEW_BASE_REPO" "$REVIEW_BASE_SHA" git -C "$CHECKOUT_DIR" worktree add --detach "$base_checkout" "$REVIEW_BASE_SHA" >/dev/null seed_state "$base_checkout" "$base_state" @@ -279,19 +280,20 @@ analyze_review() { if [ "$(depth_cap_from "$base_state/analysis.json")" = "$DEPTH_CAP" ]; then incremental "$base_checkout" "$base_state" if [ "$REQUIRES_FULL" = true ]; then - base_reason=incompatible + full_cause=incompatible else - base_source=committed fetch_commit "$REVIEW_BASE_REPO" "$REVIEW_BASE_SHA" "$(( CATCHUP_BOUND + 1 ))" || true base_from_sha="$(baseline_commit "$REVIEW_BASE_SHA")" [ -z "$base_from_sha" ] || catchup_commits="$(catchup_count "$base_from_sha" "$REVIEW_BASE_SHA")" + # Nothing to catch up means the committed analysis already describes this code. + [ "$catchup_commits" = 0 ] || base_method=incremental fi elif [ -f "$base_state/analysis.json" ]; then - base_reason=incompatible + full_cause=incompatible fi if [ "$REQUIRES_FULL" = true ]; then - base_source=computed - base_reason="${base_reason:-no_baseline}" + base_method=full + full_cause="${full_cause:-no_baseline}" trap progress_stop EXIT progress_start "$base_started" full "$base_checkout" "$base_state" "$DEPTH_CAP" @@ -299,7 +301,6 @@ analyze_review() { progress head "$(( $(date +%s) - base_started ))" fi fi - [ "$base_source" = computed ] || base_reason="" # The lookup in the step before this one is part of obtaining the base too. local base_seconds=$(( $(date +%s) - base_started + ${BASE_FETCH_SECONDS:-0} )) unset GIT_TOKEN @@ -339,15 +340,46 @@ analyze_review() { # by id for its whole retention, so one about to expire is renewed rather than # left dangling under a review that outlives it. local publish_base=false - if [ "$base_source" != saved ] || [ "${RENEW_BASE:-false}" = true ]; then + if [ "$base_published" != true ] || [ "${RENEW_BASE:-false}" = true ]; then stage "$base_state" base publish_base=true fi printf 'analysis_mode=%s\nanalysis_path=%s\nbase_analysis_path=%s\nseed_source=%s\nchain_depth=%s\npublish_base=%s\n' \ "$ANALYSIS_MODE" "$ANALYSIS_PATH" "$base_analysis" "$seed_source" "$chain_depth" "$publish_base" >> "$GITHUB_OUTPUT" - printf 'base_source=%s\nbase_reason=%s\nbase_from_sha=%s\ncatchup_commits=%s\nbase_seconds=%s\nhead_seconds=%s\n' \ - "$base_source" "$base_reason" "$base_from_sha" "$catchup_commits" "$base_seconds" "$head_seconds" >> "$GITHUB_OUTPUT" + printf 'base_analysis_method=%s\nbase_analysis_reason=%s\nbase_seconds=%s\nhead_seconds=%s\n' \ + "$base_method" "$(base_reason "$base_method" "$full_cause" "$base_from_sha" "$catchup_commits")" \ + "$base_seconds" "$head_seconds" >> "$GITHUB_OUTPUT" +} + +# Why the base analysis was obtained the way it was, in words. Whichever way, the +# result is an analysis of the merge base itself, so where an incremental run +# started is detail for this sentence, not a field of its own. +base_reason() { + local method="$1" cause="$2" from="$3" count="$4" base="${REVIEW_BASE_SHA:0:7}" reason + case "$method" in + reused) reason="$base already has a saved analysis" ;; + incremental) + if [ -z "$from" ]; then + reason="updated an existing analysis to $base" + else + reason="updated the analysis of ${from:0:7} to $base" + case "$count" in + '') ;; + 1) reason="$reason, 1 commit caught up" ;; + *) reason="$reason, $count commits caught up" ;; + esac + fi + ;; + full) + if [ "$cause" = incompatible ]; then + reason="the existing analysis was incompatible or could not be updated incrementally" + else + reason="no usable analysis was available" + fi + ;; + esac + echo "$reason" } case "$ANALYSIS_KIND" in diff --git a/scripts/action/build-review-artifact.sh b/scripts/action/build-review-artifact.sh index aa14c95..9db59e4 100755 --- a/scripts/action/build-review-artifact.sh +++ b/scripts/action/build-review-artifact.sh @@ -23,8 +23,8 @@ HEALTH_REPORT="$(dirname "$ANALYSIS_PATH")/health/health_report.json" # pr_base_sha carries the same value under the name the webview already reads: # its lookup is base_commit_sha || pr_base_sha || base_sha, so without it the # webview silently falls through to the branch tip and compares against a base -# this review never used. The base_* fields say how this run obtained the base -# graph; all strings, empty when not applicable. +# this review never used. base_analysis_method and base_analysis_reason say how +# this run obtained the base analysis; all strings, empty from an older run. jq -n \ --arg kind review \ --arg mode "$ANALYSIS_MODE" \ @@ -38,10 +38,8 @@ jq -n \ --arg base_artifact "$BASE_ARTIFACT_NAME" \ --arg base_artifact_id "$BASE_ARTIFACT_ID" \ --arg analysed_files_changed "${ANALYSED_FILES_CHANGED:-unknown}" \ - --arg base_source "${BASE_SOURCE:-}" \ - --arg base_reason "${BASE_REASON:-}" \ - --arg base_from_sha "${BASE_FROM_SHA:-}" \ - --arg catchup_commits "${CATCHUP_COMMITS:-}" \ + --arg base_analysis_method "${BASE_ANALYSIS_METHOD:-}" \ + --arg base_analysis_reason "${BASE_ANALYSIS_REASON:-}" \ --arg base_seconds "${BASE_SECONDS:-}" \ --arg head_seconds "${HEAD_SECONDS:-}" \ '{kind: $kind, mode: $mode, base_sha: $base_sha, merge_base_sha: $merge_base_sha, pr_base_sha: $merge_base_sha, @@ -49,7 +47,7 @@ jq -n \ pr_number: $pr_number, seed_source: $seed_source, chain_depth: $chain_depth, base_artifact: $base_artifact, base_artifact_id: $base_artifact_id, analysed_files_changed: $analysed_files_changed, - base_source: $base_source, base_reason: $base_reason, base_from_sha: $base_from_sha, - catchup_commits: $catchup_commits, base_seconds: $base_seconds, head_seconds: $head_seconds}' \ + base_analysis_method: $base_analysis_method, base_analysis_reason: $base_analysis_reason, + base_seconds: $base_seconds, head_seconds: $head_seconds}' \ > "${RUNNER_TEMP}/cb-review-artifact/metadata.json" echo "artifact_dir=${RUNNER_TEMP}/cb-review-artifact" >> "$GITHUB_OUTPUT" diff --git a/scripts/action/build-review-comment.sh b/scripts/action/build-review-comment.sh index 51e2ea0..4e092a1 100755 --- a/scripts/action/build-review-comment.sh +++ b/scripts/action/build-review-comment.sh @@ -45,8 +45,9 @@ elif [ "$BEHIND" -gt 0 ] 2>/dev/null; then printf '\nCompared against the merge base: this branch is %s %s behind `%s`.\n' \ "$BEHIND" "$COMMIT_NOUN" "${BASE_REF:-the base branch}" >> "$BODY" fi -# How the base was obtained, with measured times only: an estimate would be wrong for -# exactly the slow runs it is meant to explain. +# How the base analysis was obtained, with measured times only: an estimate would be +# wrong for exactly the slow runs it is meant to explain. Supporting detail, so it +# sits under the diagram with the run links. duration() { local seconds="${1:-0}" case "$seconds" in ''|*[!0-9]*) seconds=0 ;; esac @@ -56,47 +57,21 @@ duration() { printf '%s s' "$seconds" fi } -BASE_SOURCE="${BASE_SOURCE:-}" -BASE_BRANCH="${BASE_REF:-the base branch}" -BASE_FROM="${BASE_FROM_SHA:-}" -CATCHUP="${CATCHUP_COMMITS:-}" -case "$CATCHUP" in *[!0-9]*) CATCHUP="" ;; esac -CHANGES="changes $(duration "${HEAD_SECONDS:-}")" -TOOK="$(duration "${BASE_SECONDS:-}")" +BASE_METHOD="${BASE_ANALYSIS_METHOD:-}" BASE_LINE="" -# An empty sha or count is unknown, not zero: the line then says it caught up -# without claiming the base was exact. -case "$BASE_SOURCE" in - computed) - case "${BASE_REASON:-}" in - incompatible) WHY="saved diagram incompatible" ;; - too_far_behind) WHY="saved diagram too far behind" ;; - *) WHY="no saved diagram" ;; - esac - BASE_LINE="Base: built from scratch (${WHY}), ${TOOK} · ${CHANGES}" - ;; - saved) - BASE_LINE="Base: saved diagram of ${BASE_BRANCH} @${BASE_FROM:0:7} · ${CHANGES}" - ;; - committed | ancestor) - if [ -z "$BASE_FROM" ] || [ -z "$CATCHUP" ]; then - BASE_LINE="Base: saved diagram of ${BASE_BRANCH}, caught up, ${TOOK} · ${CHANGES}" - elif [ "$CATCHUP" -gt 0 ]; then - COMMIT_NOUN="commits" - [ "$CATCHUP" != 1 ] || COMMIT_NOUN="commit" - BASE_LINE="Base: caught up ${CATCHUP} ${COMMIT_NOUN} from ${BASE_BRANCH} @${BASE_FROM:0:7}, ${TOOK} · ${CHANGES}" - elif [ "$BASE_SOURCE" = ancestor ]; then - BASE_LINE="Base: caught up from ${BASE_BRANCH} @${BASE_FROM:0:7}, ${TOOK} · ${CHANGES}" - else - BASE_LINE="Base: saved diagram of ${BASE_BRANCH} @${BASE_FROM:0:7} · ${CHANGES}" - fi - ;; -esac -[ -z "$BASE_LINE" ] || printf '\n%s\n' "$BASE_LINE" >> "$BODY" +if [ -n "$BASE_METHOD" ]; then + BASE_LINE="Base: ${BASE_METHOD}" + # A reused analysis had nothing to catch up, so its time is not worth a figure. + [ "$BASE_METHOD" = reused ] || BASE_LINE="${BASE_LINE} in $(duration "${BASE_SECONDS:-}")" + [ -z "${BASE_ANALYSIS_REASON:-}" ] || BASE_LINE="${BASE_LINE} (${BASE_ANALYSIS_REASON})" + BASE_LINE="${BASE_LINE} · changes $(duration "${HEAD_SECONDS:-}")" +fi { printf '\n' cat "$DIAGRAM" - printf '\n\n' + printf '\n' + [ -z "$BASE_LINE" ] || printf '\n%s\n' "$BASE_LINE" + printf '\n' if [ -n "$ARTIFACT_URL" ]; then printf '[download artifacts](%s) · ' "$ARTIFACT_URL" fi @@ -104,11 +79,10 @@ esac # 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. + # The reason is prose, so it stays out of here: it is in the review artifact's metadata. BASE_KEYS="" - if [ -n "$BASE_SOURCE" ]; then - BASE_KEYS=" base=${BASE_SOURCE}" - [ -z "${BASE_REASON:-}" ] || BASE_KEYS="${BASE_KEYS} base_reason=${BASE_REASON}" - BASE_KEYS="${BASE_KEYS} base_seconds=${BASE_SECONDS:-} head_seconds=${HEAD_SECONDS:-}" + if [ -n "$BASE_METHOD" ]; then + BASE_KEYS=" base_analysis_method=${BASE_METHOD} base_seconds=${BASE_SECONDS:-} head_seconds=${HEAD_SECONDS:-}" fi printf '\n' \ "$PLATFORM_URL" "$N_CHANGED" "$ANALYSED_FILES_CHANGED" "${HEAD_SHA:-}" "$BASE_KEYS" diff --git a/scripts/action/post-progress.sh b/scripts/action/post-progress.sh index ce5471d..7ccb210 100755 --- a/scripts/action/post-progress.sh +++ b/scripts/action/post-progress.sh @@ -26,9 +26,8 @@ else branch="the base branch" fi sha7="${REVIEW_BASE_SHA:0:7}" -case "${BASE_REASON:-}" in +case "${FULL_CAUSE:-}" in incompatible) why="The saved diagram of $branch was made by a different engine version or settings, so this review builds a new one." ;; - too_far_behind) why="The nearest saved diagram of $branch is too far behind this pull request's base, so this review builds one from scratch." ;; *) why="$branch has no saved diagram yet, so this review builds one first. Once a diagram of $branch is saved, reviews start from it and skip this step." ;; esac minutes=$(( elapsed / 60 )) diff --git a/tests/test_action_state.py b/tests/test_action_state.py index 8008eef..de351fd 100644 --- a/tests/test_action_state.py +++ b/tests/test_action_state.py @@ -360,69 +360,70 @@ def _merge(self, branch: str, files: dict[str, str], *, bot: bool = False) -> st return self._git("rev-parse", "HEAD") def _provenance(self, values: dict[str, str]) -> dict[str, str]: - keys = ("base_source", "base_reason", "base_from_sha", "catchup_commits") + keys = ("base_analysis_method", "base_analysis_reason") for key in ("base_seconds", "head_seconds"): self.assertRegex(values[key], r"^[0-9]+$", key) return {key: values[key] for key in keys} - def test_a_saved_base_reports_itself_as_exact(self) -> None: + def test_a_saved_base_is_reused(self) -> None: _state(self.base_dir) values = self._analyze(BASE_FETCH_SECONDS="7") self.assertEqual( self._provenance(values), - {"base_source": "saved", "base_reason": "", "base_from_sha": "merge-base-sha", "catchup_commits": "0"}, + {"base_analysis_method": "reused", "base_analysis_reason": "merge-b already has a saved analysis"}, ) # The download happened in the step before; it is still time spent on the base. self.assertGreaterEqual(int(values["base_seconds"]), 7) - def test_a_base_with_nothing_to_seed_it_is_computed_for_lack_of_a_baseline(self) -> None: + def test_a_base_with_nothing_to_seed_it_is_a_full_analysis(self) -> None: sha = self._commit_base() values = self._analyze(REVIEW_BASE_SHA=sha) self.assertEqual( self._provenance(values), - {"base_source": "computed", "base_reason": "no_baseline", "base_from_sha": "", "catchup_commits": ""}, + {"base_analysis_method": "full", "base_analysis_reason": "no usable analysis was available"}, + ) + + def _assert_incompatible(self, values: dict[str, str]) -> None: + self.assertEqual( + self._provenance(values), + { + "base_analysis_method": "full", + "base_analysis_reason": "the existing analysis was incompatible or could not be updated incrementally", + }, ) def test_a_committed_baseline_with_another_depth_is_incompatible(self) -> None: sha = self._commit_base(legacy=True) - values = self._analyze(REVIEW_BASE_SHA=sha) - - self.assertEqual(values["base_source"], "computed") - self.assertEqual(values["base_reason"], "incompatible") + self._assert_incompatible(self._analyze(REVIEW_BASE_SHA=sha)) def test_a_saved_base_with_another_depth_is_incompatible(self) -> None: sha = self._commit_base() _state(self.base_dir, cap=1) - values = self._analyze(REVIEW_BASE_SHA=sha) - - self.assertEqual(values["base_source"], "computed") - self.assertEqual(values["base_reason"], "incompatible") + self._assert_incompatible(self._analyze(REVIEW_BASE_SHA=sha)) def test_an_engine_that_demands_a_full_run_makes_the_baseline_incompatible(self) -> None: sha = self._commit_base(cap=2) - values = self._analyze(REVIEW_BASE_SHA=sha, CB_REQUIRE_FULL="true") + self._assert_incompatible(self._analyze(REVIEW_BASE_SHA=sha, CB_REQUIRE_FULL="true")) - self.assertEqual(values["base_source"], "computed") - self.assertEqual(values["base_reason"], "incompatible") - - def test_a_baseline_committed_at_the_merge_base_needs_no_catching_up(self) -> None: + def test_a_baseline_committed_at_the_merge_base_is_reused(self) -> None: # The sync commit changes only .codeboarding/, which the analysis ignores, # so a merge base on it is exactly the commit the baseline describes. - analysed, sync = self._sync_history() + _analysed, sync = self._sync_history() values = self._analyze(REVIEW_BASE_SHA=sync) self.assertEqual( self._provenance(values), - {"base_source": "committed", "base_reason": "", "base_from_sha": analysed, "catchup_commits": "0"}, + {"base_analysis_method": "reused", "base_analysis_reason": f"{sync[:7]} already has a saved analysis"}, ) + # Not under the merge base's name yet, so this run publishes it. self.assertEqual(values["publish_base"], "true") def test_a_committed_baseline_counts_the_commits_it_caught_up(self) -> None: @@ -432,9 +433,13 @@ def test_a_committed_baseline_counts_the_commits_it_caught_up(self) -> None: values = self._analyze(REVIEW_BASE_SHA=merge_base) - self.assertEqual(values["base_source"], "committed") - self.assertEqual(values["base_from_sha"], analysed) - self.assertEqual(values["catchup_commits"], "2") + self.assertEqual( + self._provenance(values), + { + "base_analysis_method": "incremental", + "base_analysis_reason": f"updated the analysis of {analysed[:7]} to {merge_base[:7]}, 2 commits caught up", + }, + ) def test_merged_pull_requests_count_as_commits_to_catch_up(self) -> None: # Merged with --no-ff, the first-parent chain is merge commits only, and @@ -445,8 +450,10 @@ def test_merged_pull_requests_count_as_commits_to_catch_up(self) -> None: values = self._analyze(REVIEW_BASE_SHA=merge_base) - self.assertEqual(values["base_from_sha"], analysed) - self.assertEqual(values["catchup_commits"], "2") + self.assertEqual( + values["base_analysis_reason"], + f"updated the analysis of {analysed[:7]} to {merge_base[:7]}, 2 commits caught up", + ) def test_a_merged_sync_pull_request_describes_its_own_parent(self) -> None: # sync_strategy: pull_request. The sync commit sits on codeboarding/sync on @@ -463,12 +470,17 @@ def test_a_merged_sync_pull_request_describes_its_own_parent(self) -> None: values = self._analyze(REVIEW_BASE_SHA=merge_base) - self.assertEqual(values["base_source"], "committed") - self.assertEqual(values["base_from_sha"], analysed) - self.assertEqual(values["catchup_commits"], "1") + self.assertEqual( + self._provenance(values), + { + "base_analysis_method": "incremental", + "base_analysis_reason": f"updated the analysis of {analysed[:7]} to {merge_base[:7]}, 1 commit caught up", + }, + ) def test_a_baseline_sync_did_not_write_is_of_unknown_origin(self) -> None: - # A squash or a hand edit: its parent is not known to be what was analysed. + # A squash or a hand edit: its parent is not known to be what was analysed, + # so the reason names no starting commit and claims nothing was exact. self._git("init", "-q", "-b", "main") self._commit("feat: code", {"code.py": "pass\n"}) _state(self.checkout / ".codeboarding", cap=2) @@ -476,20 +488,23 @@ def test_a_baseline_sync_did_not_write_is_of_unknown_origin(self) -> None: values = self._analyze(REVIEW_BASE_SHA=merge_base) - self.assertEqual(values["base_source"], "committed") - self.assertEqual(values["base_from_sha"], "") - self.assertEqual(values["catchup_commits"], "") + self.assertEqual( + self._provenance(values), + { + "base_analysis_method": "incremental", + "base_analysis_reason": f"updated an existing analysis to {merge_base[:7]}", + }, + ) def test_an_attributes_line_is_not_code_to_catch_up(self) -> None: - analysed, _sync = self._sync_history() + _analysed, _sync = self._sync_history() merge_base = self._commit( "chore: attributes", {".gitattributes": ".codeboarding/analysis.json linguist-generated=true\n"} ) values = self._analyze(REVIEW_BASE_SHA=merge_base) - self.assertEqual(values["base_from_sha"], analysed) - self.assertEqual(values["catchup_commits"], "0") + self.assertEqual(values["base_analysis_method"], "reused") def _progress_stub(self) -> Path: calls = self.root / "gh-calls" diff --git a/tests/test_progress_comment.py b/tests/test_progress_comment.py index 034dd72..8994422 100644 --- a/tests/test_progress_comment.py +++ b/tests/test_progress_comment.py @@ -50,7 +50,7 @@ def _post(self, step: str, elapsed: int, comment_id: str = "77", **extra: str) - return [c for c in self.calls.read_text().split("\n----\n") if "PATCH" in c] if self.calls.exists() else [] def test_the_base_step_shows_elapsed_minutes_and_the_reason(self) -> None: - (patch,) = self._post("base", 125, BASE_REASON="no_baseline") + (patch,) = self._post("base", 125, FULL_CAUSE="no_baseline") self.assertIn("1. ⏳ Building the diagram of `main` @a1b2c3d from scratch · running for 2 min\n", patch) self.assertIn(" `main` has no saved diagram yet, so this review builds one first.", patch) self.assertIn("\n2. Analysing this PR's changes\n", patch) @@ -61,7 +61,7 @@ def test_the_first_minute_does_not_read_as_zero(self) -> None: self.assertIn("running for less than a minute", patch) def test_an_incompatible_base_says_why(self) -> None: - (patch,) = self._post("base", 60, BASE_REASON="incompatible") + (patch,) = self._post("base", 60, FULL_CAUSE="incompatible") self.assertIn("The saved diagram of `main` was made by a different engine version or settings", patch) def test_the_head_step_reports_the_measured_base_time(self) -> None: diff --git a/tests/test_review_artifact_metadata.py b/tests/test_review_artifact_metadata.py index a9bd90c..7a79060 100644 --- a/tests/test_review_artifact_metadata.py +++ b/tests/test_review_artifact_metadata.py @@ -69,24 +69,19 @@ def test_how_the_base_was_obtained_is_recorded_as_strings(self) -> None: with tempfile.TemporaryDirectory() as tmp: metadata, _outputs = _build( Path(tmp), - BASE_SOURCE="committed", - BASE_REASON="", - BASE_FROM_SHA="abc", - CATCHUP_COMMITS="3", + BASE_ANALYSIS_METHOD="incremental", + BASE_ANALYSIS_REASON="updated the analysis of 9f8e7d6 to a1b2c3d, 3 commits caught up", BASE_SECONDS="41", HEAD_SECONDS="159", ) self.assertEqual( - {key: metadata[key] for key in metadata if key.startswith("base_") or key.endswith("_seconds")} - | {"catchup_commits": metadata["catchup_commits"]}, + {key: metadata[key] for key in metadata if key.startswith("base_") or key.endswith("_seconds")}, { "base_sha": "tip-sha", "base_artifact": "codeboarding-base-cfg-mergebasesha", "base_artifact_id": "4242", - "base_source": "committed", - "base_reason": "", - "base_from_sha": "abc", - "catchup_commits": "3", + "base_analysis_method": "incremental", + "base_analysis_reason": "updated the analysis of 9f8e7d6 to a1b2c3d, 3 commits caught up", "base_seconds": "41", "head_seconds": "159", }, @@ -95,14 +90,7 @@ def test_how_the_base_was_obtained_is_recorded_as_strings(self) -> None: def test_an_older_run_without_provenance_records_empty_strings(self) -> None: with tempfile.TemporaryDirectory() as tmp: metadata, _outputs = _build(Path(tmp)) - for key in ( - "base_source", - "base_reason", - "base_from_sha", - "catchup_commits", - "base_seconds", - "head_seconds", - ): + for key in ("base_analysis_method", "base_analysis_reason", "base_seconds", "head_seconds"): self.assertEqual(metadata[key], "", key) diff --git a/tests/test_review_comment.py b/tests/test_review_comment.py index 9df2135..ca77256 100644 --- a/tests/test_review_comment.py +++ b/tests/test_review_comment.py @@ -83,82 +83,76 @@ def test_a_changed_analysed_file_gets_no_verdict_even_at_zero_components(self) - class BaseLineTests(unittest.TestCase): - """One line saying how the base was obtained, with measured times and never an estimate.""" + """One line under the diagram saying how the base analysis was obtained, with measured times only.""" - def _base_line(self, **extra: str) -> str: + def _body(self, **extra: str) -> str: with tempfile.TemporaryDirectory() as tmp: - body = _build(Path(tmp), BASE_REF="main", MERGE_BASE_SHA="f00dfeed" * 5, **extra) + return _build(Path(tmp), BASE_REF="main", MERGE_BASE_SHA="f00dfeed" * 5, **extra) + + def _base_line(self, **extra: str) -> str: + body = self._body(**extra) lines = [line for line in body.splitlines() if line.startswith("Base: ")] self.assertEqual(len(lines), 1, body) return lines[0] - def test_a_computed_base_says_why_and_how_long(self) -> None: + def test_a_full_analysis_says_why_and_how_long(self) -> None: line = self._base_line( - BASE_SOURCE="computed", BASE_REASON="no_baseline", BASE_SECONDS="534", HEAD_SECONDS="192" + BASE_ANALYSIS_METHOD="full", + BASE_ANALYSIS_REASON="no usable analysis was available", + BASE_SECONDS="534", + HEAD_SECONDS="192", ) - self.assertEqual(line, "Base: built from scratch (no saved diagram), 8 m 54 s · changes 3 m 12 s") - - def test_an_incompatible_base_says_so(self) -> None: - line = self._base_line(BASE_SOURCE="computed", BASE_REASON="incompatible", BASE_SECONDS="60", HEAD_SECONDS="5") self.assertEqual( - line, "Base: built from scratch (saved diagram incompatible), 1 m 0 s · changes 5 s" - ) - - def test_a_saved_base_names_the_commit(self) -> None: - line = self._base_line( - BASE_SOURCE="saved", BASE_FROM_SHA="a1b2c3d4e5", CATCHUP_COMMITS="0", BASE_SECONDS="3", HEAD_SECONDS="159" + line, "Base: full in 8 m 54 s (no usable analysis was available) · changes 3 m 12 s" ) - self.assertEqual(line, "Base: saved diagram of main @a1b2c3d · changes 2 m 39 s") - - def test_a_committed_base_at_the_merge_base_reads_as_saved(self) -> None: - line = self._base_line(BASE_SOURCE="committed", BASE_FROM_SHA="a1b2c3d4", CATCHUP_COMMITS="0", HEAD_SECONDS="4") - self.assertEqual(line, "Base: saved diagram of main @a1b2c3d · changes 4 s") - - def test_an_unknown_catch_up_does_not_claim_an_exact_base(self) -> None: - for sha, count in (("", ""), ("a1b2c3d4", ""), ("", "3")): - line = self._base_line( - BASE_SOURCE="committed", BASE_FROM_SHA=sha, CATCHUP_COMMITS=count, BASE_SECONDS="41", HEAD_SECONDS="4" - ) - self.assertEqual(line, "Base: saved diagram of main, caught up, 41 s · changes 4 s") - def test_an_ancestor_never_reads_as_saved(self) -> None: + def test_a_reused_analysis_has_no_base_time(self) -> None: line = self._base_line( - BASE_SOURCE="ancestor", BASE_FROM_SHA="a1b2c3d4", CATCHUP_COMMITS="0", BASE_SECONDS="41", HEAD_SECONDS="4" + BASE_ANALYSIS_METHOD="reused", + BASE_ANALYSIS_REASON="a1b2c3d already has a saved analysis", + BASE_SECONDS="3", + HEAD_SECONDS="159", ) - self.assertEqual(line, "Base: caught up from main @a1b2c3d, 41 s · changes 4 s") + self.assertEqual(line, "Base: reused (a1b2c3d already has a saved analysis) · changes 2 m 39 s") - def test_a_caught_up_base_counts_the_commits(self) -> None: + def test_an_incremental_analysis_carries_where_it_started(self) -> None: line = self._base_line( - BASE_SOURCE="committed", - BASE_FROM_SHA="a1b2c3d4e5", - CATCHUP_COMMITS="4", + BASE_ANALYSIS_METHOD="incremental", + BASE_ANALYSIS_REASON="updated the analysis of 9f8e7d6 to a1b2c3d, 4 commits caught up", BASE_SECONDS="41", HEAD_SECONDS="159", ) - self.assertEqual(line, "Base: caught up 4 commits from main @a1b2c3d, 41 s · changes 2 m 39 s") + self.assertEqual( + line, + "Base: incremental in 41 s (updated the analysis of 9f8e7d6 to a1b2c3d, 4 commits caught up)" + " · changes 2 m 39 s", + ) - def test_the_marker_carries_the_base_after_the_existing_keys(self) -> None: - with tempfile.TemporaryDirectory() as tmp: - body = _build( - Path(tmp), BASE_SOURCE="computed", BASE_REASON="no_baseline", BASE_SECONDS="534", HEAD_SECONDS="192" - ) + def test_the_base_line_sits_under_the_diagram_above_the_run_links(self) -> None: + body = self._body(BASE_ANALYSIS_METHOD="reused", BASE_ANALYSIS_REASON="r", HEAD_SECONDS="4") + diagram = body.index("```mermaid") + base = body.index("Base: ") + footer = body.index("run [1234]") + self.assertLess(diagram, base) + self.assertLess(base, footer) + self.assertIn("· changes 4 s\n\nrun [1234]", body) + + def test_the_marker_carries_the_method_after_the_existing_keys(self) -> None: + body = self._body( + BASE_ANALYSIS_METHOD="full", + BASE_ANALYSIS_REASON="no usable analysis was available", + BASE_SECONDS="534", + HEAD_SECONDS="192", + ) self.assertTrue( - body.rstrip("\n").endswith( - "head=abc123 base=computed base_reason=no_baseline base_seconds=534 head_seconds=192 -->" - ), + body.rstrip("\n").endswith("head=abc123 base_analysis_method=full base_seconds=534 head_seconds=192 -->"), body, ) - def test_the_marker_omits_an_empty_reason(self) -> None: - with tempfile.TemporaryDirectory() as tmp: - body = _build(Path(tmp), BASE_SOURCE="saved", BASE_REASON="", BASE_SECONDS="2", HEAD_SECONDS="9") - self.assertIn("head=abc123 base=saved base_seconds=2 head_seconds=9 -->", body) - - def test_without_a_base_source_there_is_no_line_and_no_keys(self) -> None: - with tempfile.TemporaryDirectory() as tmp: - body = _build(Path(tmp)) + def test_without_a_method_there_is_no_line_and_no_keys(self) -> None: + body = self._body() self.assertNotIn("Base:", body) - self.assertNotIn(" base=", body) + self.assertNotIn("base_analysis_method=", body) if __name__ == "__main__":