From 8c4f196d9e10c85a7facda15191abc3c90c8c15a Mon Sep 17 00:00:00 2001 From: ivanmilevtues Date: Mon, 19 Oct 2026 22:23:41 +0200 Subject: [PATCH 1/2] feat: preview picked pull requests and email the author from a setup PR's run Before a setup PR merges, its own run is the only one GitHub starts for us. That run now reviews up to three open pull requests the app lists in the setup PR's description (), each applied to the commit the map was built from and analysed incrementally from it, and uploads each as codeboarding-preview---pr. Nothing is posted on those pull requests. The sticky review comment is edited, and edits send no notification, so the run also posts one new comment on the setup PR when the first preview (or, without previews, the map) is ready. GitHub emails it to the PR's author, who is the person who asked for the map. Co-Authored-By: Claude Opus 5.5 --- action.yml | 163 ++++++++++++++- scripts/action/build-review-artifact.sh | 21 +- scripts/action/notify-setup-ready.sh | 56 ++++++ scripts/action/resolve-preview-prs.sh | 57 ++++++ scripts/action/review-preview-pr.sh | 61 ++++++ tests/test_setup_previews.py | 251 ++++++++++++++++++++++++ tests/test_setup_ready_notice.py | 82 ++++++++ tests/test_with_auth.py | 8 +- 8 files changed, 685 insertions(+), 14 deletions(-) create mode 100755 scripts/action/notify-setup-ready.sh create mode 100755 scripts/action/resolve-preview-prs.sh create mode 100755 scripts/action/review-preview-pr.sh create mode 100644 tests/test_setup_previews.py create mode 100644 tests/test_setup_ready_notice.py diff --git a/action.yml b/action.yml index 3de0b22..41e9b38 100644 --- a/action.yml +++ b/action.yml @@ -611,14 +611,11 @@ runs: MODEL: ${{ inputs.model }} AGENT_MODEL_INPUT: ${{ inputs.agent_model }} PARSING_MODEL_INPUT: ${{ inputs.parsing_model }} + # A setup PR's run goes on to preview other pull requests; the credentials stay until + # those finish and the step after them removes them. + KEEP_AUTH: ${{ github.event_name == 'pull_request' && github.head_ref == 'codeboarding/setup-action' }} run: '"$GITHUB_ACTION_PATH/scripts/action/with-auth.sh" "$GITHUB_ACTION_PATH/scripts/action/review-analyze-head.sh"' - # The head step removes the credentials; this covers a run that failed before it. - - name: Remove analysis credentials - if: always() && steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' - shell: bash - run: '"$GITHUB_ACTION_PATH/scripts/action/clear-auth.sh"' - # Published before the review is rendered: the analysis is the expensive part, # so a later rendering or posting failure must not throw it away. Only the # next run reads this, hence the short retention. @@ -724,6 +721,160 @@ runs: GITHUB_TOKEN: ${{ inputs.github_token }} path: ${{ steps.review_body.outputs.path }} + # Before a setup PR merges, its own run is the only one GitHub starts for us, so this is + # where a new user's open pull requests get their first review: the ones the app listed + # in the setup PR's description, each applied to the commit this run just analysed and + # analysed incrementally from it (see review-preview-pr.sh). Nothing is posted on them. + # The app finds each as codeboarding-preview---pr plus its metadata. + # Not codeboarding-review--...: the app takes the newest artifact with that prefix as + # the setup PR's own review, so a preview named that way would replace the setup PR's map. + # All best effort: the setup PR's map is already uploaded and must not fail because of them. + - name: Resolve pull requests to preview + id: preview_list + if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.review_comment.outcome == 'success' && github.event_name == 'pull_request' && github.head_ref == 'codeboarding/setup-action' + continue-on-error: true + shell: bash + env: + GH_TOKEN: ${{ inputs.github_token }} + GH_HOST: ${{ github.server_url }} + REPOSITORY: ${{ github.repository }} + PR_NUMBER: ${{ steps.guard.outputs.pr_number }} + HEAD_REF: ${{ github.head_ref }} + BASE_REF: ${{ steps.guard.outputs.base_ref }} + MAX_PREVIEWS: '3' + run: "$GITHUB_ACTION_PATH/scripts/action/resolve-preview-prs.sh" + + - name: Preview pull request 1 for the setup PR + id: preview_1 + if: steps.guard.outputs.mode == 'review' && steps.preview_list.outputs.count >= 1 + continue-on-error: true + shell: bash + env: + ACTION_PATH: ${{ github.action_path }} + CHECKOUT_DIR: ${{ github.workspace }}/.codeboarding-target + GIT_TOKEN: ${{ inputs.github_token }} + REPOSITORY: ${{ github.repository }} + SETUP_PR: ${{ steps.guard.outputs.pr_number }} + REVIEW_BASE_SHA: ${{ steps.guard.outputs.merge_base_sha }} + BASE_ANALYSIS_PATH: ${{ steps.review_baseline.outputs.base_analysis_path }} + # The setup PR's own review artifact already records where the base graph lives. + REVIEW_ARTIFACT_DIR: ${{ steps.review_artifact.outputs.artifact_dir }} + ENGINE_VERSION: ${{ steps.state.outputs.engine_version }} + CFG_HASH: ${{ steps.state.outputs.cfg_hash }} + DEPTH_CAP: ${{ inputs.depth_cap }} + MODEL: ${{ inputs.model }} + AGENT_MODEL_INPUT: ${{ inputs.agent_model }} + PARSING_MODEL_INPUT: ${{ inputs.parsing_model }} + KEEP_AUTH: 'true' + run: '"$GITHUB_ACTION_PATH/scripts/action/with-auth.sh" "$GITHUB_ACTION_PATH/scripts/action/review-preview-pr.sh" 1' + + - name: Upload preview 1 + id: upload_preview_1 + if: steps.preview_1.outputs.artifact_dir != '' && github.server_url == 'https://github.com' + continue-on-error: true + uses: actions/upload-artifact@v4 + with: + name: codeboarding-preview-${{ github.run_id }}-${{ github.run_attempt }}-pr${{ steps.preview_1.outputs.pr_number }} + path: ${{ steps.preview_1.outputs.artifact_dir }} + if-no-files-found: error + retention-days: 14 + + # The sticky review comment is an edit by now, and edits send no notification. One new + # comment on a setup PR emails its author: after the first preview when there is one, + # so the email links a coloured pull request, otherwise as soon as the map is posted. + - name: Tell the setup PR's author the map is ready + if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.review_comment.outcome == 'success' && github.event_name == 'pull_request' && github.head_ref == 'codeboarding/setup-action' + continue-on-error: true + shell: bash + env: + GH_TOKEN: ${{ inputs.github_token }} + GH_HOST: ${{ github.server_url }} + REPOSITORY: ${{ github.repository }} + PR_NUMBER: ${{ steps.guard.outputs.pr_number }} + PR_AUTHOR: ${{ github.event.pull_request.user.login }} + HEAD_REF: ${{ github.head_ref }} + BASE_REF: ${{ steps.guard.outputs.base_ref }} + FIRST_PR: ${{ steps.upload_preview_1.outcome == 'success' && steps.preview_1.outputs.pr_number || '' }} + FIRST_TITLE: ${{ steps.preview_1.outputs.title }} + PREVIEW_PRS: ${{ steps.preview_list.outputs.prs }} + run: "$GITHUB_ACTION_PATH/scripts/action/notify-setup-ready.sh" + + - name: Preview pull request 2 for the setup PR + id: preview_2 + if: steps.guard.outputs.mode == 'review' && steps.preview_list.outputs.count >= 2 + continue-on-error: true + shell: bash + env: + ACTION_PATH: ${{ github.action_path }} + CHECKOUT_DIR: ${{ github.workspace }}/.codeboarding-target + GIT_TOKEN: ${{ inputs.github_token }} + REPOSITORY: ${{ github.repository }} + SETUP_PR: ${{ steps.guard.outputs.pr_number }} + REVIEW_BASE_SHA: ${{ steps.guard.outputs.merge_base_sha }} + BASE_ANALYSIS_PATH: ${{ steps.review_baseline.outputs.base_analysis_path }} + # The setup PR's own review artifact already records where the base graph lives. + REVIEW_ARTIFACT_DIR: ${{ steps.review_artifact.outputs.artifact_dir }} + ENGINE_VERSION: ${{ steps.state.outputs.engine_version }} + CFG_HASH: ${{ steps.state.outputs.cfg_hash }} + DEPTH_CAP: ${{ inputs.depth_cap }} + MODEL: ${{ inputs.model }} + AGENT_MODEL_INPUT: ${{ inputs.agent_model }} + PARSING_MODEL_INPUT: ${{ inputs.parsing_model }} + KEEP_AUTH: 'true' + run: '"$GITHUB_ACTION_PATH/scripts/action/with-auth.sh" "$GITHUB_ACTION_PATH/scripts/action/review-preview-pr.sh" 2' + + - name: Upload preview 2 + id: upload_preview_2 + if: steps.preview_2.outputs.artifact_dir != '' && github.server_url == 'https://github.com' + continue-on-error: true + uses: actions/upload-artifact@v4 + with: + name: codeboarding-preview-${{ github.run_id }}-${{ github.run_attempt }}-pr${{ steps.preview_2.outputs.pr_number }} + path: ${{ steps.preview_2.outputs.artifact_dir }} + if-no-files-found: error + retention-days: 14 + + - name: Preview pull request 3 for the setup PR + id: preview_3 + if: steps.guard.outputs.mode == 'review' && steps.preview_list.outputs.count >= 3 + continue-on-error: true + shell: bash + env: + ACTION_PATH: ${{ github.action_path }} + CHECKOUT_DIR: ${{ github.workspace }}/.codeboarding-target + GIT_TOKEN: ${{ inputs.github_token }} + REPOSITORY: ${{ github.repository }} + SETUP_PR: ${{ steps.guard.outputs.pr_number }} + REVIEW_BASE_SHA: ${{ steps.guard.outputs.merge_base_sha }} + BASE_ANALYSIS_PATH: ${{ steps.review_baseline.outputs.base_analysis_path }} + # The setup PR's own review artifact already records where the base graph lives. + REVIEW_ARTIFACT_DIR: ${{ steps.review_artifact.outputs.artifact_dir }} + ENGINE_VERSION: ${{ steps.state.outputs.engine_version }} + CFG_HASH: ${{ steps.state.outputs.cfg_hash }} + DEPTH_CAP: ${{ inputs.depth_cap }} + MODEL: ${{ inputs.model }} + AGENT_MODEL_INPUT: ${{ inputs.agent_model }} + PARSING_MODEL_INPUT: ${{ inputs.parsing_model }} + run: '"$GITHUB_ACTION_PATH/scripts/action/with-auth.sh" "$GITHUB_ACTION_PATH/scripts/action/review-preview-pr.sh" 3' + + - name: Upload preview 3 + id: upload_preview_3 + if: steps.preview_3.outputs.artifact_dir != '' && github.server_url == 'https://github.com' + continue-on-error: true + uses: actions/upload-artifact@v4 + with: + name: codeboarding-preview-${{ github.run_id }}-${{ github.run_attempt }}-pr${{ steps.preview_3.outputs.pr_number }} + path: ${{ steps.preview_3.outputs.artifact_dir }} + if-no-files-found: error + retention-days: 14 + + # The head and preview steps remove the credentials themselves; this covers a run that + # failed before them, and a setup PR's run, which keeps them for its previews. + - name: Remove analysis credentials + if: always() && steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' + shell: bash + run: '"$GITHUB_ACTION_PATH/scripts/action/clear-auth.sh"' + # The same body, on the run page. Job summaries and logs are explicitly excluded from # the artifact storage allowance, and the cap is 1MiB per step against a diagram # measured in single-digit kilobytes, so this is free in both senses. It earns its diff --git a/scripts/action/build-review-artifact.sh b/scripts/action/build-review-artifact.sh index acdf52a..c53118b 100755 --- a/scripts/action/build-review-artifact.sh +++ b/scripts/action/build-review-artifact.sh @@ -1,8 +1,10 @@ #!/usr/bin/env bash # Packages both analyses and PR metadata, then outputs the artifact upload directory. set -euo pipefail -mkdir -p "${RUNNER_TEMP}/cb-review-artifact" -cp "$ANALYSIS_PATH" "${RUNNER_TEMP}/cb-review-artifact/analysis.json" +# A setup PR's previews of other pull requests are packaged the same way, each into its own directory. +dir="${ARTIFACT_DIR:-${RUNNER_TEMP}/cb-review-artifact}" +mkdir -p "$dir" +cp "$ANALYSIS_PATH" "$dir/analysis.json" # The base graph is normally published separately, named for the commit it # describes, so ten runs on one pull request store it once rather than ten times. # A fork review publishes nothing another run could read, so when it had to @@ -10,14 +12,16 @@ cp "$ANALYSIS_PATH" "${RUNNER_TEMP}/cb-review-artifact/analysis.json" # artifact would name a base that does not exist and no reader could reproduce # the comparison. if [ "${INLINE_BASE:-false}" = true ]; then - cp "$BASE_ANALYSIS_PATH" "${RUNNER_TEMP}/cb-review-artifact/base_analysis.json" + cp "$BASE_ANALYSIS_PATH" "$dir/base_analysis.json" BASE_ARTIFACT_NAME="" fi # The engine writes this next to the analysis it came from. Ship it when present # so a pull request's warnings are readable without rerunning the analysis. HEALTH_REPORT="$(dirname "$ANALYSIS_PATH")/health/health_report.json" -[ ! -f "$HEALTH_REPORT" ] || cp "$HEALTH_REPORT" "${RUNNER_TEMP}/cb-review-artifact/health_report.json" +[ ! -f "$HEALTH_REPORT" ] || cp "$HEALTH_REPORT" "$dir/health_report.json" +# basis=preview marks a review made inside a setup PR's run: the pull request's own changes +# laid over the commit that run analysed, not over its real merge base. setup_pr names that run's PR. # base_sha stays the event's base branch tip for consumers that key on it; # merge_base_sha records the commit the diagram actually compared against. # pr_base_sha carries the same value under the name the webview already reads: @@ -37,10 +41,13 @@ jq -n \ --arg base_artifact "$BASE_ARTIFACT_NAME" \ --arg base_artifact_id "$BASE_ARTIFACT_ID" \ --arg analysed_files_changed "${ANALYSED_FILES_CHANGED:-unknown}" \ + --arg basis "${BASIS:-}" \ + --arg setup_pr "${SETUP_PR:-}" \ '{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}' \ - > "${RUNNER_TEMP}/cb-review-artifact/metadata.json" -echo "artifact_dir=${RUNNER_TEMP}/cb-review-artifact" >> "$GITHUB_OUTPUT" + analysed_files_changed: $analysed_files_changed} + + (if $basis == "" then {} else {basis: $basis, setup_pr: $setup_pr} end)' \ + > "$dir/metadata.json" +echo "artifact_dir=$dir" >> "$GITHUB_OUTPUT" diff --git a/scripts/action/notify-setup-ready.sh b/scripts/action/notify-setup-ready.sh new file mode 100755 index 0000000..de20101 --- /dev/null +++ b/scripts/action/notify-setup-ready.sh @@ -0,0 +1,56 @@ +#!/usr/bin/env bash +# Tells the author of a CodeBoarding setup PR that its preview map is ready. +# +# The review comment is sticky: it is created when the run starts and edited when it +# ends, and GitHub sends no notification for an edit. A new comment does notify the +# PR's author, who for a setup PR is the person who asked for the map (the app opens +# it with their token), so this one comment is what turns "you can close this tab" +# into an email from GitHub. Failed runs need nothing here: GitHub already emails the +# person who triggered a failed run. +# +# When the setup PR asked for previews of other pull requests, this runs after the first +# one is ready (FIRST_PR) and links it, since a coloured pull request is the point; the +# rest of PREVIEW_PRS are named as still running. Posted once per PR, found again by its +# marker, so pushing new settings to the setup PR does not email again. Best effort throughout. +set -euo pipefail +[ "${HEAD_REF:-}" = "codeboarding/setup-action" ] || exit 0 +[ -n "${PR_NUMBER:-}" ] && [ -n "${REPOSITORY:-}" ] || exit 0 +export GH_HOST="${GH_HOST:-github.com}" +GH_HOST="${GH_HOST#*://}" + +marker="" +existing="$(gh api --paginate "repos/$REPOSITORY/issues/$PR_NUMBER/comments?per_page=100" \ + --jq ".[] | select((.body // \"\") | contains(\"$marker\")) | .id")" +[ -z "$existing" ] || exit 0 + +# A mention notifies even an author who unsubscribed from the thread. Bots are not mentioned. +greeting="Your" +case "${PR_AUTHOR:-}" in + "" | *"[bot]") ;; + *) greeting="@$PR_AUTHOR your" ;; +esac +if [ -n "${BASE_REF:-}" ]; then + branch="\`$BASE_REF\`" +else + branch="your default branch" +fi +link() { printf 'https://app.codeboarding.org/%s/pull/%s?utm_source=github&utm_medium=setup_ready&utm_campaign=gh_action' "$REPOSITORY" "$1"; } +if [ -n "${FIRST_PR:-}" ]; then + headline="$greeting CodeBoarding map of $branch is ready, and #$FIRST_PR is reviewed on it." + cta="**[Open #$FIRST_PR in CodeBoarding]($(link "$FIRST_PR"))**${FIRST_TITLE:+ ยท $FIRST_TITLE}" + rest="" + IFS=',' read -r -a all <<< "${PREVIEW_PRS:-}" + for pr in "${all[@]}"; do + if [ -n "$pr" ] && [ "$pr" != "$FIRST_PR" ]; then rest+="${rest:+, }#$pr"; fi + done + [ -z "$rest" ] || cta+=$'\n\n'"$rest follow in a few minutes. [Your map]($(link "$PR_NUMBER")) is open already." +else + headline="$greeting CodeBoarding map of $branch is ready." + cta="**[Open it in CodeBoarding]($(link "$PR_NUMBER"))**" +fi +body="$(printf '%s\n\n%s\n\n%s\n\n%s' \ + "$headline" \ + "$cta" \ + "This is a preview. Merge this PR to keep the map up to date and review every pull request." \ + "$marker")" +gh api -X POST "repos/$REPOSITORY/issues/$PR_NUMBER/comments" -f body="$body" >/dev/null diff --git a/scripts/action/resolve-preview-prs.sh b/scripts/action/resolve-preview-prs.sh new file mode 100755 index 0000000..b247aef --- /dev/null +++ b/scripts/action/resolve-preview-prs.sh @@ -0,0 +1,57 @@ +#!/usr/bin/env bash +# Reads which pull requests a CodeBoarding setup PR should preview, from a line the app +# writes into its description: +# +# +# +# Before the setup PR merges, its own run is the only one GitHub starts for us, so this is +# where a new user's open pull requests get their first review. The description is read +# now, after the map is built, not from the event payload, so a list changed while the map +# was building still counts. Each entry must be an open pull request from this repository +# into the same branch; anything else is skipped with a notice. At most MAX_PREVIEWS. +# +# Writes one JSON file per preview to $RUNNER_TEMP/codeboarding-preview/.json and +# outputs count= and prs= (comma-separated, in order). Best effort throughout. +set -euo pipefail +out="${RUNNER_TEMP:?}/codeboarding-preview" +rm -rf "$out" +mkdir -p "$out" +count=0 prs="" +finish() { printf 'count=%s\nprs=%s\n' "$count" "$prs" >> "${GITHUB_OUTPUT:-/dev/null}"; exit 0; } +[ "${HEAD_REF:-}" = "codeboarding/setup-action" ] || finish +[ -n "${PR_NUMBER:-}" ] && [ -n "${REPOSITORY:-}" ] && [ -n "${BASE_REF:-}" ] || finish +export GH_HOST="${GH_HOST:-github.com}" +GH_HOST="${GH_HOST#*://}" +max="${MAX_PREVIEWS:-3}" + +body="$(gh api "repos/$REPOSITORY/pulls/$PR_NUMBER" --jq '.body // ""' 2>/dev/null || true)" +line="$(grep -oE '' <<< "$body" | head -n 1 || true)" +[ -n "$line" ] || finish + +skip() { echo "::notice::Not previewing #$1: $2"; } +seen=" " +for pr in $(grep -oE '[0-9]+' <<< "${line#*preview}"); do + [ "$count" -lt "$max" ] || { skip "$pr" "at most $max pull requests are previewed."; continue; } + [ "$pr" != "$PR_NUMBER" ] || continue + case "$seen" in *" $pr "*) continue ;; esac + seen+="$pr " + if ! json="$(gh api "repos/$REPOSITORY/pulls/$pr" 2>/dev/null)"; then + skip "$pr" "it could not be read." + continue + fi + IFS=$'\t' read -r state head_repo base_ref base_sha head_sha < <(jq -r \ + '[.state, (.head.repo.full_name // ""), .base.ref, .base.sha, .head.sha] | @tsv' <<< "$json") + [ "$state" = open ] || { skip "$pr" "it is not open."; continue; } + [ "$head_repo" = "$REPOSITORY" ] || { skip "$pr" "it comes from a fork."; continue; } + [ "$base_ref" = "$BASE_REF" ] || { skip "$pr" "it targets $base_ref, not $BASE_REF."; continue; } + # The pull request's own changes are what it adds since its merge base, the same + # comparison GitHub's Files tab makes. + merge_base="$(gh api "repos/$REPOSITORY/compare/$base_sha...$head_sha" --jq '.merge_base_commit.sha // ""' 2>/dev/null || true)" + [ -n "$merge_base" ] || { skip "$pr" "its merge base could not be resolved."; continue; } + count=$(( count + 1 )) + jq -n --arg pr "$pr" --arg head_sha "$head_sha" --arg merge_base "$merge_base" \ + --arg title "$(jq -r '.title // ""' <<< "$json")" \ + '{pr_number: $pr, head_sha: $head_sha, merge_base_sha: $merge_base, title: $title}' > "$out/$count.json" + prs="${prs:+$prs,}$pr" +done +finish diff --git a/scripts/action/review-preview-pr.sh b/scripts/action/review-preview-pr.sh new file mode 100755 index 0000000..f56e736 --- /dev/null +++ b/scripts/action/review-preview-pr.sh @@ -0,0 +1,61 @@ +#!/usr/bin/env bash +# Reviews the n-th pull request a setup PR asked to preview (see resolve-preview-prs.sh). +# +# The run has one analysis to start from: the commit the setup PR is compared against. +# Reviewing a pull request at its own merge base would need a second full analysis of an +# older commit, so instead its own changes (merge base to head, as GitHub's Files tab shows +# them) are applied to that commit and analysed incrementally from the analysis in hand. +# Only the pull request's own work differs between the two, so nobody else's commits are +# reported as its changes. A pull request that does not apply cleanly there is skipped. +# +# Usage: review-preview-pr.sh . Outputs pr_number=, title= and artifact_dir= when a +# preview was built; nothing when there is no n-th entry or it was skipped. +set -euo pipefail +# shellcheck source=scripts/action/analysis-state.sh +source "$(dirname "$0")/analysis-state.sh" +n="$1" +dir="${RUNNER_TEMP:?}/codeboarding-preview" +entry="$dir/$n.json" +[ -f "$entry" ] || exit 0 +IFS=$'\t' read -r pr head_sha pr_merge_base < <(jq -r '[.pr_number, .head_sha, .merge_base_sha] | @tsv' "$entry") +title="$(jq -r '.title' "$entry")" +base="$REVIEW_BASE_SHA" + +fetch_commit "$REPOSITORY" "$base" >&2 +fetch_commit "$REPOSITORY" "$pr_merge_base" >&2 +fetch_commit "$REPOSITORY" "$head_sha" >&2 +tree="$dir/tree-$n" +rm -rf "$tree" +git -C "$CHECKOUT_DIR" worktree add --detach "$tree" "$base" >/dev/null 2>&1 +if ! git -C "$CHECKOUT_DIR" diff --binary "$pr_merge_base" "$head_sha" | + git -C "$tree" apply --3way --whitespace=nowarn >/dev/null 2>&1; then + echo "::notice::#$pr does not apply cleanly to ${base:0:7}, the commit this map describes; not previewing it." + exit 0 +fi + +state="$dir/state-$n" +rm -rf "$state" +cp -a "$(dirname "$BASE_ANALYSIS_PATH")" "$state" +rm -f "$state/origin.json" +unset GIT_TOKEN +result="$("$ACTION_PATH/scripts/action/analyze.sh" head "$tree" "$state")" +analysis_path="$(awk -F= '$1 == "analysis_path" {print substr($0, index($0, "=") + 1); exit}' <<< "$result")" +analysis_mode="$(awk -F= '$1 == "analysis_mode" {print $2; exit}' <<< "$result")" + +meta="$(python3 "$ACTION_PATH/scripts/diff_to_mermaid.py" --base "$BASE_ANALYSIS_PATH" --head "$analysis_path" \ + --out "$dir/diagram-$n.md" --direction LR --render-depth 1)" +analysed="$(jq -r '.analysed_files_changed // "unknown"' <<< "$meta")" + +# Point at the same base graph the setup PR's review does, inlined when it does. +review_meta="$REVIEW_ARTIFACT_DIR/metadata.json" +inline=false +[ ! -f "$REVIEW_ARTIFACT_DIR/base_analysis.json" ] || inline=true +# The packaging step writes artifact_dir= to this step's outputs. +BASE_ARTIFACT_NAME="$(jq -r '.base_artifact // ""' "$review_meta")" \ + BASE_ARTIFACT_ID="$(jq -r '.base_artifact_id // ""' "$review_meta")" INLINE_BASE="$inline" \ + ARTIFACT_DIR="$dir/artifact-$n" ANALYSIS_PATH="$analysis_path" ANALYSIS_MODE="$analysis_mode" \ + BASE_SHA="$base" MERGE_BASE_SHA="$base" MERGE_BASE_RESOLVED=true HEAD_SHA="$head_sha" \ + PR_NUMBER="$pr" SEED_SOURCE=base CHAIN_DEPTH=1 ANALYSED_FILES_CHANGED="$analysed" \ + BASIS=preview SETUP_PR="$SETUP_PR" \ + "$ACTION_PATH/scripts/action/build-review-artifact.sh" +printf 'pr_number=%s\ntitle=%s\n' "$pr" "${title//$'\n'/ }" >> "$GITHUB_OUTPUT" diff --git a/tests/test_setup_previews.py b/tests/test_setup_previews.py new file mode 100644 index 0000000..ab8a730 --- /dev/null +++ b/tests/test_setup_previews.py @@ -0,0 +1,251 @@ +"""A setup PR's run previews the pull requests the app listed in its description.""" + +from __future__ import annotations + +import json +import os +import shutil +import subprocess +import tempfile +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +RESOLVE = ROOT / "scripts" / "action" / "resolve-preview-prs.sh" +PREVIEW = ROOT / "scripts" / "action" / "review-preview-pr.sh" +NOTIFY = ROOT / "scripts" / "action" / "notify-setup-ready.sh" + +PULLS = { + "412": {"state": "open", "head": {"repo": {"full_name": "owner/repo"}, "sha": "h412"}, "base": {"ref": "main", "sha": "t"}, "title": "Retry failed webhooks"}, + "409": {"state": "open", "head": {"repo": {"full_name": "owner/repo"}, "sha": "h409"}, "base": {"ref": "main", "sha": "t"}, "title": "Split ledger"}, + "401": {"state": "open", "head": {"repo": {"full_name": "owner/repo"}, "sha": "h401"}, "base": {"ref": "main", "sha": "t"}, "title": "Bump SDK"}, + "400": {"state": "open", "head": {"repo": {"full_name": "owner/repo"}, "sha": "h400"}, "base": {"ref": "main", "sha": "t"}, "title": "Fourth"}, + "377": {"state": "open", "head": {"repo": {"full_name": "someone/fork"}, "sha": "h377"}, "base": {"ref": "main", "sha": "t"}, "title": "From a fork"}, + "350": {"state": "closed", "head": {"repo": {"full_name": "owner/repo"}, "sha": "h350"}, "base": {"ref": "main", "sha": "t"}, "title": "Closed"}, + "340": {"state": "open", "head": {"repo": {"full_name": "owner/repo"}, "sha": "h340"}, "base": {"ref": "release", "sha": "t"}, "title": "Other branch"}, +} + + +def git(cwd: Path, *args: str) -> str: + return subprocess.run( + ["git", "-c", "user.name=t", "-c", "user.email=t@t", *args], cwd=cwd, check=True, capture_output=True, text=True + ).stdout.strip() + + +class ResolvePreviewTests(unittest.TestCase): + def setUp(self) -> None: + self.temp_dir = tempfile.TemporaryDirectory() + self.root = Path(self.temp_dir.name) + (self.root / "bin").mkdir() + (self.root / "pulls.json").write_text(json.dumps(PULLS)) + self.calls = self.root / "calls" + gh = self.root / "bin" / "gh" + # pulls/418 is the setup PR; its body carries the list. compare returns a merge base per head. + gh.write_text( + "#!/usr/bin/env python3\n" + "import json, sys\n" + f"open({str(self.calls)!r}, 'a').write(' '.join(sys.argv[1:]) + '\\n')\n" + f"pulls = json.load(open({str(self.root / 'pulls.json')!r}))\n" + "path = sys.argv[2]\n" + "if path.endswith('/pulls/418'):\n" + f" print(open({str(self.root / 'body')!r}).read())\n" + "elif '/compare/' in path:\n" + " print('mb-' + path.rsplit('...', 1)[1])\n" + "else:\n" + " n = path.rsplit('/', 1)[1]\n" + " if n not in pulls: sys.exit(1)\n" + " print(json.dumps(pulls[n]))\n", + encoding="utf-8", + ) + gh.chmod(0o755) + + def tearDown(self) -> None: + self.temp_dir.cleanup() + + def _run(self, body: str, **extra: str) -> dict[str, str]: + (self.root / "body").write_text(body) + output = self.root / "output" + output.write_text("") + subprocess.run( + [str(RESOLVE)], + env={ + "PATH": f"{self.root / 'bin'}:{os.environ['PATH']}", + "RUNNER_TEMP": str(self.root), + "GITHUB_OUTPUT": str(output), + "REPOSITORY": "owner/repo", + "PR_NUMBER": "418", + "HEAD_REF": "codeboarding/setup-action", + "BASE_REF": "main", + **extra, + }, + check=True, + capture_output=True, + text=True, + ) + return dict(line.split("=", 1) for line in output.read_text().splitlines() if line) + + def _entry(self, n: int) -> dict: + return json.loads((self.root / "codeboarding-preview" / f"{n}.json").read_text()) + + def test_the_listed_pull_requests_are_previewed_in_order(self) -> None: + out = self._run("Adds CodeBoarding.\n\n\n") + self.assertEqual(out, {"count": "2", "prs": "412,409"}) + self.assertEqual(self._entry(1), {"pr_number": "412", "head_sha": "h412", "merge_base_sha": "mb-h412", "title": "Retry failed webhooks"}) + self.assertEqual(self._entry(2)["pr_number"], "409") + + def test_forks_closed_and_other_branches_are_skipped_and_the_list_is_capped(self) -> None: + out = self._run("") + self.assertEqual(out, {"count": "3", "prs": "412,409,401"}) + + def test_no_list_means_no_previews(self) -> None: + self.assertEqual(self._run("Adds CodeBoarding."), {"count": "0", "prs": ""}) + + def test_other_pull_requests_read_nothing(self) -> None: + self.assertEqual(self._run("", HEAD_REF="feat/x"), {"count": "0", "prs": ""}) + self.assertFalse(self.calls.exists()) + + +class ReviewPreviewTests(unittest.TestCase): + """Real git history: main moved on after the PR forked, and the preview applies only the PR's own work.""" + + def setUp(self) -> None: + self.temp_dir = tempfile.TemporaryDirectory() + self.root = Path(self.temp_dir.name) + repo = self.repo = self.root / "repo" + repo.mkdir() + git(repo, "init", "-q", "-b", "main") + (repo / "shared.txt").write_text("one\ntwo\nthree\n") + git(repo, "add", ".") + git(repo, "commit", "-qm", "X") + self.fork_point = git(repo, "rev-parse", "HEAD") + git(repo, "checkout", "-qb", "pr") + (repo / "webhooks.txt").write_text("retry\n") + git(repo, "add", ".") + git(repo, "commit", "-qm", "P") + self.pr_head = git(repo, "rev-parse", "HEAD") + git(repo, "checkout", "-q", "main") + (repo / "ledger.txt").write_text("landed after the fork\n") + git(repo, "add", ".") + git(repo, "commit", "-qm", "Z") + self.base = git(repo, "rev-parse", "HEAD") + + # The base analysis this run built, and the setup PR's own review artifact. + self.base_dir = self.root / "base-state" + self.base_dir.mkdir() + (self.base_dir / "analysis.json").write_text('{"components": []}') + self.review_dir = self.root / "review-artifact" + self.review_dir.mkdir() + (self.review_dir / "metadata.json").write_text(json.dumps({"base_artifact": "cb-base-Z", "base_artifact_id": "77"})) + + # The action tree with the engine and renderer stubbed; the packager is the real one. + action = self.action = self.root / "action" + (action / "scripts" / "action").mkdir(parents=True) + shutil.copy(ROOT / "scripts" / "action" / "build-review-artifact.sh", action / "scripts" / "action") + analyze = action / "scripts" / "action" / "analyze.sh" + analyze.write_text( + "#!/usr/bin/env bash\n" + 'ls "$2" | sort > "$3/seen-files"\n' + 'echo \'{"components": ["Webhooks"]}\' > "$3/analysis.json"\n' + 'printf "analysis_mode=incremental\\nanalysis_path=%s\\n" "$3/analysis.json"\n', + encoding="utf-8", + ) + analyze.chmod(0o755) + (action / "scripts" / "diff_to_mermaid.py").write_text('print(\'{"n_changed": 1, "analysed_files_changed": 1}\')\n') + + self.preview_dir = self.root / "codeboarding-preview" + self.preview_dir.mkdir() + + def tearDown(self) -> None: + self.temp_dir.cleanup() + + def _run(self, pr_head: str | None = None) -> dict[str, str]: + (self.preview_dir / "1.json").write_text(json.dumps( + {"pr_number": "412", "head_sha": pr_head or self.pr_head, "merge_base_sha": self.fork_point, "title": "Retry failed webhooks"})) + output = self.root / "output" + output.write_text("") + subprocess.run( + [str(PREVIEW), "1"], + env={ + "PATH": os.environ["PATH"], + "RUNNER_TEMP": str(self.root), + "GITHUB_OUTPUT": str(output), + "ACTION_PATH": str(self.action), + "CHECKOUT_DIR": str(self.repo), + "REPOSITORY": "owner/repo", + "SETUP_PR": "418", + "REVIEW_BASE_SHA": self.base, + "BASE_ANALYSIS_PATH": str(self.base_dir / "analysis.json"), + "REVIEW_ARTIFACT_DIR": str(self.review_dir), + }, + check=True, + capture_output=True, + text=True, + ) + return dict(line.split("=", 1) for line in output.read_text().splitlines() if line) + + def test_the_pull_request_is_applied_to_the_analysed_commit(self) -> None: + out = self._run() + self.assertEqual(out["pr_number"], "412") + seen = (self.preview_dir / "state-1" / "seen-files").read_text().split() + # Z's file is there because the base is Z; the PR's file because it was applied; nothing else. + self.assertIn("ledger.txt", seen) + self.assertIn("webhooks.txt", seen) + + def test_the_artifact_names_the_pull_request_and_the_comparison(self) -> None: + out = self._run() + meta = json.loads((Path(out["artifact_dir"]) / "metadata.json").read_text()) + self.assertEqual(meta["pr_number"], "412") + self.assertEqual(meta["head_sha"], self.pr_head) + self.assertEqual(meta["merge_base_sha"], self.base, "the commit the diagram compared against") + self.assertEqual(meta["pr_base_sha"], self.base) + self.assertEqual(meta["basis"], "preview") + self.assertEqual(meta["setup_pr"], "418") + self.assertEqual(meta["base_artifact"], "cb-base-Z") + self.assertEqual(meta["base_artifact_id"], "77") + self.assertFalse((Path(out["artifact_dir"]) / "base_analysis.json").exists()) + + def test_the_base_is_inlined_when_the_setup_review_inlined_it(self) -> None: + (self.review_dir / "base_analysis.json").write_text("{}") + out = self._run() + self.assertTrue((Path(out["artifact_dir"]) / "base_analysis.json").exists()) + + def test_a_pull_request_that_conflicts_with_the_analysed_commit_is_skipped(self) -> None: + git(self.repo, "checkout", "-qb", "clash", self.fork_point) + (self.repo / "ledger.txt").write_text("a different ledger\n") + git(self.repo, "add", ".") + git(self.repo, "commit", "-qm", "C") + clash = git(self.repo, "rev-parse", "HEAD") + git(self.repo, "checkout", "-q", "main") + self.assertEqual(self._run(pr_head=clash), {}) + + +class FirstPreviewNoticeTests(unittest.TestCase): + def test_the_email_links_the_first_reviewed_pull_request(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + (root / "bin").mkdir() + calls = root / "calls" + gh = root / "bin" / "gh" + gh.write_text(f'#!/usr/bin/env bash\nprintf "%s\\n----\\n" "$*" >> "{calls}"\n', encoding="utf-8") + gh.chmod(0o755) + subprocess.run( + [str(NOTIFY)], + env={ + "PATH": f"{root / 'bin'}:{os.environ['PATH']}", + "REPOSITORY": "owner/repo", "PR_NUMBER": "418", "PR_AUTHOR": "maria-k", + "HEAD_REF": "codeboarding/setup-action", "BASE_REF": "main", + "FIRST_PR": "412", "FIRST_TITLE": "Retry failed webhooks", "PREVIEW_PRS": "412,409,401", + }, + check=True, + capture_output=True, + text=True, + ) + (post,) = [c for c in calls.read_text().split("\n----\n") if c.startswith("api -X POST")] + self.assertIn("@maria-k your CodeBoarding map of `main` is ready, and #412 is reviewed on it.", post) + self.assertIn("[Open #412 in CodeBoarding](https://app.codeboarding.org/owner/repo/pull/412?", post) + self.assertIn("#409, #401 follow in a few minutes.", post) + self.assertIn("(https://app.codeboarding.org/owner/repo/pull/418?", post) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_setup_ready_notice.py b/tests/test_setup_ready_notice.py new file mode 100644 index 0000000..47fee63 --- /dev/null +++ b/tests/test_setup_ready_notice.py @@ -0,0 +1,82 @@ +"""The one new comment on a setup PR that makes GitHub email its author the map is ready.""" + +import os +import subprocess +import tempfile +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +NOTIFY = ROOT / "scripts" / "action" / "notify-setup-ready.sh" +MARKER = "" + + +class SetupReadyNoticeTests(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 _run(self, existing: str = "", **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"*) printf "%s" "{existing}" ;; esac\n', + encoding="utf-8", + ) + gh.chmod(0o755) + env = { + "PATH": f"{self.bin_dir}:{os.environ['PATH']}", + "REPOSITORY": "owner/repo", + "PR_NUMBER": "418", + "PR_AUTHOR": "maria-k", + "HEAD_REF": "codeboarding/setup-action", + "BASE_REF": "main", + **extra, + } + subprocess.run([str(NOTIFY)], env=env, check=True, capture_output=True, text=True) + if not self.calls.exists(): + return [] + return [c for c in self.calls.read_text().split("\n----\n") if c.startswith("api -X POST")] + + def test_a_setup_pr_gets_one_new_comment_that_mentions_its_author(self) -> None: + (post,) = self._run() + self.assertIn("repos/owner/repo/issues/418/comments", post) + self.assertIn("@maria-k your CodeBoarding map of `main` is ready.", post) + self.assertIn("https://app.codeboarding.org/owner/repo/pull/418?", post) + self.assertNotIn("run=", post) + self.assertIn(MARKER, post) + + def test_it_is_posted_once_per_pull_request(self) -> None: + self.assertEqual(self._run(existing="991"), []) + + def test_other_pull_requests_get_nothing(self) -> None: + self.assertEqual(self._run(HEAD_REF="feat/retry-webhooks"), []) + self.assertFalse(self.calls.exists(), "no API call at all outside the setup branch") + + def test_a_bot_author_is_not_mentioned(self) -> None: + (post,) = self._run(PR_AUTHOR="codeboarding-app[bot]") + self.assertIn("Your CodeBoarding map of `main` is ready.", post) + self.assertNotIn("@", post.split("body=", 1)[1].split("\n", 1)[0]) + + def test_the_step_runs_only_after_a_posted_review_on_the_setup_branch(self) -> None: + action = (ROOT / "action.yml").read_text(encoding="utf-8") + start = action.index(" - name: Tell the setup PR's author the map is ready") + step = action[start:action.index("\n - name:", start + 1)] + self.assertGreater(start, action.index(" - name: Post review comment")) + condition = next(line for line in step.splitlines() if line.strip().startswith("if:")) + self.assertIn("steps.review_comment.outcome == 'success'", condition) + self.assertIn("github.event_name == 'pull_request'", condition) + self.assertIn("github.head_ref == 'codeboarding/setup-action'", condition) + self.assertIn("continue-on-error: true", step, "a notification must never fail the review") + self.assertIn("notify-setup-ready.sh", step) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_with_auth.py b/tests/test_with_auth.py index 49cfdbc..5e9afb4 100644 --- a/tests/test_with_auth.py +++ b/tests/test_with_auth.py @@ -56,7 +56,13 @@ def test_every_step_but_the_last_of_a_mode_keeps_credentials(self) -> None: steps = [s for s in self._steps() if "with-auth.sh" in s and f"mode == '{mode}'" in s] self.assertTrue(steps, mode) for step in steps[:-1]: - self.assertIn("KEEP_AUTH: 'true'", step, f"{mode}: {step.splitlines()[0]} consumes the credentials") + # The head analysis keeps them only on a setup PR's run, which goes on to preview + # other pull requests; elsewhere it is the last analysis and removes them. + keeps = "KEEP_AUTH: 'true'" in step or ( + step.startswith("Analyze pull request") + and "KEEP_AUTH: ${{ github.event_name == 'pull_request' && github.head_ref == 'codeboarding/setup-action' }}" in step + ) + self.assertTrue(keeps, f"{mode}: {step.splitlines()[0]} consumes the credentials") self.assertNotIn("KEEP_AUTH", steps[-1], f"{mode}: the last analysis step must remove them") def test_kept_credentials_are_always_removed(self) -> None: From 02707d9d833ad06eaf4411ba39207a681596c898 Mon Sep 17 00:00:00 2001 From: ivanmilevtues Date: Mon, 19 Oct 2026 22:26:00 +0200 Subject: [PATCH 2/2] feat: name each preview artifact for its pull request and head The app attributes an artifact to a pull request per run, and every preview shares the setup PR's run, so the name carries the pull request and the head it reviewed, as the legacy codeboarding-pr-- names did. Co-Authored-By: Claude Opus 5.5 --- action.yml | 8 ++++---- scripts/action/review-preview-pr.sh | 4 ++-- tests/test_setup_previews.py | 1 + 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/action.yml b/action.yml index 41e9b38..f0f458f 100644 --- a/action.yml +++ b/action.yml @@ -725,7 +725,7 @@ runs: # where a new user's open pull requests get their first review: the ones the app listed # in the setup PR's description, each applied to the commit this run just analysed and # analysed incrementally from it (see review-preview-pr.sh). Nothing is posted on them. - # The app finds each as codeboarding-preview---pr plus its metadata. + # The app finds each as codeboarding-preview---pr-. # Not codeboarding-review--...: the app takes the newest artifact with that prefix as # the setup PR's own review, so a preview named that way would replace the setup PR's map. # All best effort: the setup PR's map is already uploaded and must not fail because of them. @@ -774,7 +774,7 @@ runs: continue-on-error: true uses: actions/upload-artifact@v4 with: - name: codeboarding-preview-${{ github.run_id }}-${{ github.run_attempt }}-pr${{ steps.preview_1.outputs.pr_number }} + name: codeboarding-preview-${{ github.run_id }}-${{ github.run_attempt }}-pr${{ steps.preview_1.outputs.pr_number }}-${{ steps.preview_1.outputs.head_sha }} path: ${{ steps.preview_1.outputs.artifact_dir }} if-no-files-found: error retention-days: 14 @@ -829,7 +829,7 @@ runs: continue-on-error: true uses: actions/upload-artifact@v4 with: - name: codeboarding-preview-${{ github.run_id }}-${{ github.run_attempt }}-pr${{ steps.preview_2.outputs.pr_number }} + name: codeboarding-preview-${{ github.run_id }}-${{ github.run_attempt }}-pr${{ steps.preview_2.outputs.pr_number }}-${{ steps.preview_2.outputs.head_sha }} path: ${{ steps.preview_2.outputs.artifact_dir }} if-no-files-found: error retention-days: 14 @@ -863,7 +863,7 @@ runs: continue-on-error: true uses: actions/upload-artifact@v4 with: - name: codeboarding-preview-${{ github.run_id }}-${{ github.run_attempt }}-pr${{ steps.preview_3.outputs.pr_number }} + name: codeboarding-preview-${{ github.run_id }}-${{ github.run_attempt }}-pr${{ steps.preview_3.outputs.pr_number }}-${{ steps.preview_3.outputs.head_sha }} path: ${{ steps.preview_3.outputs.artifact_dir }} if-no-files-found: error retention-days: 14 diff --git a/scripts/action/review-preview-pr.sh b/scripts/action/review-preview-pr.sh index f56e736..62f640a 100755 --- a/scripts/action/review-preview-pr.sh +++ b/scripts/action/review-preview-pr.sh @@ -9,7 +9,7 @@ # reported as its changes. A pull request that does not apply cleanly there is skipped. # # Usage: review-preview-pr.sh . Outputs pr_number=, title= and artifact_dir= when a -# preview was built; nothing when there is no n-th entry or it was skipped. +# preview was built, plus head_sha=; nothing when there is no n-th entry or it was skipped. set -euo pipefail # shellcheck source=scripts/action/analysis-state.sh source "$(dirname "$0")/analysis-state.sh" @@ -58,4 +58,4 @@ BASE_ARTIFACT_NAME="$(jq -r '.base_artifact // ""' "$review_meta")" \ PR_NUMBER="$pr" SEED_SOURCE=base CHAIN_DEPTH=1 ANALYSED_FILES_CHANGED="$analysed" \ BASIS=preview SETUP_PR="$SETUP_PR" \ "$ACTION_PATH/scripts/action/build-review-artifact.sh" -printf 'pr_number=%s\ntitle=%s\n' "$pr" "${title//$'\n'/ }" >> "$GITHUB_OUTPUT" +printf 'pr_number=%s\nhead_sha=%s\ntitle=%s\n' "$pr" "$head_sha" "${title//$'\n'/ }" >> "$GITHUB_OUTPUT" diff --git a/tests/test_setup_previews.py b/tests/test_setup_previews.py index ab8a730..aaa705d 100644 --- a/tests/test_setup_previews.py +++ b/tests/test_setup_previews.py @@ -186,6 +186,7 @@ def _run(self, pr_head: str | None = None) -> dict[str, str]: def test_the_pull_request_is_applied_to_the_analysed_commit(self) -> None: out = self._run() self.assertEqual(out["pr_number"], "412") + self.assertEqual(out["head_sha"], self.pr_head, "the artifact name carries it, so the app needs no download") seen = (self.preview_dir / "state-1" / "seen-files").read_text().split() # Z's file is there because the base is Z; the PR's file because it was applied; nothing else. self.assertIn("ledger.txt", seen)