diff --git a/action.yml b/action.yml
index ab7f483..7ca8012 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_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 }}
@@ -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,10 @@ 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_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"
- name: Upload review artifact
@@ -639,6 +652,10 @@ 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_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"
- name: Post review comment
diff --git a/docs/COMMIT_STRATEGY.md b/docs/COMMIT_STRATEGY.md
index 2849dfb..9f9fdcf 100644
--- a/docs/COMMIT_STRATEGY.md
+++ b/docs/COMMIT_STRATEGY.md
@@ -70,7 +70,13 @@ 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_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_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
@@ -104,7 +110,11 @@ 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 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
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..a7048e5 100755
--- a/scripts/action/analyze.sh
+++ b/scripts/action/analyze.sh
@@ -139,13 +139,100 @@ 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, 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 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)"
+ case "$shallow" in /*) ;; *) shallow="$CHECKOUT_DIR/$shallow" ;; esac
+ if [ -f "$shallow" ] && grep -qx "$writer" "$shallow"; then
+ return 0
+ 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() {
+ 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 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
+ [ -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_STOP="${RUNNER_TEMP:-}/codeboarding-progress-stop"
+progress() {
+ 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() {
+ local started="$1"
+ rm -f "$PROGRESS_STOP"
+ progress base 0
+ ( 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
+ touch "$PROGRESS_STOP"
+ pkill -x sleep -P "$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 +261,48 @@ 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 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/"
else
- base_source=computed
+ base_published=false
+ # A bundle under this exact name that the run cannot use was made with another cap.
+ [ ! -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"
REQUIRES_FULL=true
if [ "$(depth_cap_from "$base_state/analysis.json")" = "$DEPTH_CAP" ]; then
incremental "$base_checkout" "$base_state"
+ if [ "$REQUIRES_FULL" = true ]; then
+ full_cause=incompatible
+ else
+ 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
+ full_cause=incompatible
fi
if [ "$REQUIRES_FULL" = true ]; then
+ base_method=full
+ full_cause="${full_cause:-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
+ # 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 +324,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 +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" = computed ] || [ "${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_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 acdf52a..9db59e4 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. 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" \
@@ -37,10 +38,16 @@ 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_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,
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_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 96743a6..4e092a1 100755
--- a/scripts/action/build-review-comment.sh
+++ b/scripts/action/build-review-comment.sh
@@ -45,10 +45,33 @@ 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 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
+ if [ "$seconds" -ge 60 ]; then
+ printf '%s m %s s' "$(( seconds / 60 ))" "$(( seconds % 60 ))"
+ else
+ printf '%s s' "$seconds"
+ fi
+}
+BASE_METHOD="${BASE_ANALYSIS_METHOD:-}"
+BASE_LINE=""
+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
@@ -56,7 +79,12 @@ 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:-}"
+ # The reason is prose, so it stays out of here: it is in the review artifact's metadata.
+ BASE_KEYS=""
+ 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"
} >> "$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..7ccb210
--- /dev/null
+++ b/scripts/action/post-progress.sh
@@ -0,0 +1,59 @@
+#!/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 "${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." ;;
+ *) 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")"
+# 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 7b5a8aa..de351fd 100644
--- a/tests/test_action_state.py
+++ b/tests/test_action_state.py
@@ -312,6 +312,241 @@ 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], *, 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")
+ 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", "-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", {}, 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_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_is_reused(self) -> None:
+ _state(self.base_dir)
+
+ values = self._analyze(BASE_FETCH_SECONDS="7")
+
+ self.assertEqual(
+ self._provenance(values),
+ {"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_a_full_analysis(self) -> None:
+ sha = self._commit_base()
+
+ values = self._analyze(REVIEW_BASE_SHA=sha)
+
+ self.assertEqual(
+ self._provenance(values),
+ {"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)
+
+ 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)
+
+ 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)
+
+ self._assert_incompatible(self._analyze(REVIEW_BASE_SHA=sha, CB_REQUIRE_FULL="true"))
+
+ 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()
+
+ values = self._analyze(REVIEW_BASE_SHA=sync)
+
+ self.assertEqual(
+ self._provenance(values),
+ {"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:
+ 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(
+ 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
+ # 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_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
+ # 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(
+ 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,
+ # 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)
+ merge_base = self._commit("chore(codeboarding): sync analysis baseline (#7)", {})
+
+ values = self._analyze(REVIEW_BASE_SHA=merge_base)
+
+ 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()
+ 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_analysis_method"], "reused")
+
+ 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..8994422
--- /dev/null
+++ b/tests/test_progress_comment.py
@@ -0,0 +1,91 @@
+"""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, 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)
+ 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, 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:
+ (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_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=""), [])
+
+
+if __name__ == "__main__":
+ unittest.main()
diff --git a/tests/test_review_artifact_metadata.py b/tests/test_review_artifact_metadata.py
index 6b43526..7a79060 100644
--- a/tests/test_review_artifact_metadata.py
+++ b/tests/test_review_artifact_metadata.py
@@ -65,6 +65,34 @@ 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_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")},
+ {
+ "base_sha": "tip-sha",
+ "base_artifact": "codeboarding-base-cfg-mergebasesha",
+ "base_artifact_id": "4242",
+ "base_analysis_method": "incremental",
+ "base_analysis_reason": "updated the analysis of 9f8e7d6 to a1b2c3d, 3 commits caught up",
+ "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_analysis_method", "base_analysis_reason", "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..ca77256 100644
--- a/tests/test_review_comment.py
+++ b/tests/test_review_comment.py
@@ -82,5 +82,78 @@ 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 under the diagram saying how the base analysis was obtained, with measured times only."""
+
+ def _body(self, **extra: str) -> str:
+ with tempfile.TemporaryDirectory() as tmp:
+ 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_full_analysis_says_why_and_how_long(self) -> None:
+ line = self._base_line(
+ BASE_ANALYSIS_METHOD="full",
+ BASE_ANALYSIS_REASON="no usable analysis was available",
+ BASE_SECONDS="534",
+ HEAD_SECONDS="192",
+ )
+ self.assertEqual(
+ line, "Base: full in 8 m 54 s (no usable analysis was available) · changes 3 m 12 s"
+ )
+
+ def test_a_reused_analysis_has_no_base_time(self) -> None:
+ line = self._base_line(
+ BASE_ANALYSIS_METHOD="reused",
+ BASE_ANALYSIS_REASON="a1b2c3d already has a saved analysis",
+ BASE_SECONDS="3",
+ HEAD_SECONDS="159",
+ )
+ self.assertEqual(line, "Base: reused (a1b2c3d already has a saved analysis) · changes 2 m 39 s")
+
+ def test_an_incremental_analysis_carries_where_it_started(self) -> None:
+ line = self._base_line(
+ 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: incremental in 41 s (updated the analysis of 9f8e7d6 to a1b2c3d, 4 commits caught up)"
+ " · changes 2 m 39 s",
+ )
+
+ 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_analysis_method=full base_seconds=534 head_seconds=192 -->"),
+ body,
+ )
+
+ def test_without_a_method_there_is_no_line_and_no_keys(self) -> None:
+ body = self._body()
+ self.assertNotIn("Base:", body)
+ self.assertNotIn("base_analysis_method=", body)
+
+
if __name__ == "__main__":
unittest.main()