feat: report how the review base analysis was obtained

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

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.

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

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"

scripts/action/build-review-comment.sh

@@ -45,18 +45,46 @@ elif [ "$BEHIND" -gt 0 ] 2>/dev/null; then
   printf '\n<sub>Compared against the merge base: this branch is %s %s behind `%s`.</sub>\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<sub>'
+  printf '\n'
+  [ -z "$BASE_LINE" ] || printf '\n<sub>%s</sub>\n' "$BASE_LINE"
+  printf '\n<sub>'
   if [ -n "$ARTIFACT_URL" ]; then
     printf '[download artifacts](%s) · ' "$ARTIFACT_URL"
   fi
   printf 'run [%s](%s)</sub>\n' "$GITHUB_RUN_ID" "$RUN_URL"
   # 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 '<!-- codeboarding: platform_url=%s changed=%s analysed_files_changed=%s head=%s -->\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 '<!-- codeboarding: platform_url=%s changed=%s analysed_files_changed=%s head=%s%s -->\n' \
+    "$PLATFORM_URL" "$N_CHANGED" "$ANALYSED_FILES_CHANGED" "${HEAD_SHA:-}" "$BASE_KEYS"
 } >> "$BODY"
 echo "path=$BODY" >> "$GITHUB_OUTPUT"

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