From 7a65300301d850cd297c767bc279005973c0d001 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Fri, 9 Oct 2026 23:11:58 +0200 Subject: [PATCH 1/2] feat: save the diagram to an analysis branch of its own sync_strategy: branch saves the analysis to an orphan branch in the same repository, codeboarding/analysis unless analysis_branch names another, one fast-forward commit per sync, and never writes to the target branch. Each commit carries CodeBoarding-Source and CodeBoarding-Config trailers, also recorded in .codeboarding/source.json. Reviews read their base from the branch after an exact artifact and a baseline committed at the merge base: an entry for the merge base is reused, an entry for an ancestor is caught up. Only entries made under this run's configuration count, so a different engine version or model is never reused as is. Sync continues from the tip under the same rule, replacing the generated state wholesale and keeping only the checkout's user config. Delivery refuses an existing branch that is not an analysis branch (no trailer, or anything besides .codeboarding/), builds on the tip it fetched rather than the one ls-remote saw, and the guard rejects an analysis_branch git cannot use before anything runs. The importable ruleset now restricts creating and updating the branch to its bypass actor, since sync and review load a pickle from it. target_branch's description now says it is the code branch sync analyzes, written only by push and pull_request. The README carries a prompt for moving an existing setup. Co-Authored-By: Claude Opus 5.5 --- README.md | 40 ++- action.yml | 16 +- docs/COMMIT_STRATEGY.md | 67 +++++ docs/analysis-branch-ruleset.json | 24 ++ scripts/action/analyze.sh | 104 ++++++- scripts/action/deliver-sync.sh | 77 ++++++ scripts/action/guard.sh | 15 +- tests/test_analysis_branch.py | 445 ++++++++++++++++++++++++++++++ 8 files changed, 781 insertions(+), 7 deletions(-) create mode 100644 docs/analysis-branch-ruleset.json create mode 100644 tests/test_analysis_branch.py diff --git a/README.md b/README.md index 922b666..80f6820 100644 --- a/README.md +++ b/README.md @@ -301,6 +301,41 @@ Generation is identical to direct push. Only delivery changes: the same commit i With the default `github.token`, the repository or organization must allow GitHub Actions to create pull requests. A GitHub App token or PAT can instead be passed as `github_token`. The same input is used for review comments and sync delivery. +### Save the diagram to a branch of its own + +Set `sync_strategy: branch` to keep the analysis off your code branches entirely: + +```yaml + - uses: CodeBoarding/CodeBoarding-action@v1 + with: + mode: sync + llm: hosted + target_branch: main + sync_strategy: branch +``` + +Here `target_branch` is the code branch sync analyzes, and it is only read. Each sync adds one commit to `codeboarding/analysis` in the same repository (set `analysis_branch` to change the name), an orphan branch that shares no history with `main`. It holds the same `.codeboarding/` files sync would otherwise commit to `main`, plus `.codeboarding/source.json` naming the commit they describe and the configuration that made them; the commit message carries both as `CodeBoarding-Source:` and `CodeBoarding-Config:` trailers. Pushes only ever fast-forward, `main` is never written, and no pull request is opened. Reviews read their base from the branch, and the web platform reads the latest diagram from it. The [analysis branch section](docs/COMMIT_STRATEGY.md#the-analysis-branch) covers what happens if the branch is deleted, and a ruleset you should import to protect it: sync and review load a pickle from it. + +**Moving an existing setup.** Nothing changes until you opt in: `push` and `pull_request` keep working as before. To switch, paste this into your coding agent: + +```text +Move this repository's CodeBoarding sync to sync_strategy: branch. +1. In the workflow that runs CodeBoarding/CodeBoarding-action with mode: sync, set + `sync_strategy: branch` in its `with:` block (replace push or pull_request). + Keep every other input. +2. Delete the generated files under .codeboarding/ from the default branch, keeping + the user configuration: .codeboarding/.codeboardingignore, + .codeboarding/health/health_config.json and .codeboarding/health/.healthignore. + Reviews prefer a baseline committed on the branch, so a stale one left there + would keep being used. +3. Remove any .gitattributes lines that mark .codeboarding/ files as + linguist-generated, if nothing else is left under .codeboarding/ for them. +4. Open a pull request with these changes. After it merges, close any open + pull request from the codeboarding/sync branch and delete that branch. +``` + +The first sync after the merge creates `codeboarding/analysis`, catching up from a saved analysis when there is one. + ## Inputs | Input | Mode | Default | Description | @@ -316,8 +351,9 @@ With the default `github.token`, the repository or organization must allow GitHu | `parsing_model` | both | empty | Parsing-only override for `model`. | | `depth_cap` | both | `2` | Positive integer maximum analysis depth, including full-analysis fallbacks. Changing it rebuilds incompatible state. | | `github_token` | both | `${{ github.token }}` | Token for comments and sync delivery. | -| `sync_strategy` | sync | `push` | `push` or `pull_request`. | -| `target_branch` | sync | event branch | Branch receiving the baseline or rolling PR. | +| `sync_strategy` | sync | `push` | Where sync saves the analysis: `push` (a commit on `target_branch`), `pull_request` (a rolling PR into it), or `branch` (commits on `analysis_branch`). | +| `analysis_branch` | both | `codeboarding/analysis` | Branch in this repository that `sync_strategy: branch` saves the analysis to; reviews read their base from it when it exists. | +| `target_branch` | sync | event branch | Code branch sync analyzes. With `push` or `pull_request` it also receives the analysis commit or rolling PR; with `branch` it is only read. | | `force_full` | sync | `false` | Ignore the committed baseline for this run. | | `warmstart_retention_days` | review | `1` | Days to keep the reusable analysis. Only the next run reads it. | diff --git a/action.yml b/action.yml index 3dbd9a6..9401b62 100644 --- a/action.yml +++ b/action.yml @@ -143,11 +143,15 @@ inputs: required: false default: ${{ github.token }} sync_strategy: - description: 'Sync delivery method: push or pull_request.' + description: 'Where sync saves the analysis: push (a commit on target_branch), pull_request (a rolling PR into target_branch), or branch (commits on analysis_branch; target_branch is never written).' required: false default: 'push' + analysis_branch: + description: 'Branch in this repository that sync_strategy branch saves the analysis to, one commit per sync. Reviews read their base analysis from it when it exists.' + required: false + default: 'codeboarding/analysis' target_branch: - description: 'Branch updated by sync mode. Defaults to the event branch.' + description: 'Code branch sync mode analyzes. With sync_strategy push or pull_request the analysis is also committed to it; with branch it is only read. Defaults to the event branch.' required: false default: '' force_full: @@ -223,6 +227,7 @@ runs: HEAD_AUTHOR_EMAIL: ${{ github.event.head_commit.author.email }} TARGET_BRANCH_INPUT: ${{ inputs.target_branch }} SYNC_STRATEGY: ${{ inputs.sync_strategy }} + ANALYSIS_BRANCH: ${{ inputs.analysis_branch }} COMMENT_BODY: ${{ github.event.comment.body }} AUTHOR_ASSOCIATION: ${{ github.event.comment.author_association }} ISSUE_PR_URL: ${{ github.event.issue.pull_request.url }} @@ -451,6 +456,8 @@ runs: CHECKOUT_DIR: ${{ github.workspace }}/.codeboarding-target STAGE_DIR: ${{ runner.temp }}/cb-state/${{ github.action }}/out FORCE_FULL: ${{ inputs.force_full }} + SYNC_STRATEGY: ${{ inputs.sync_strategy }} + ANALYSIS_BRANCH: ${{ inputs.analysis_branch }} CFG_HASH: ${{ steps.state.outputs.cfg_hash }} # Lets a branch without a usable committed baseline catch up from a saved analysis. ANCESTOR_LOOKUP: ${{ github.server_url == 'https://github.com' && steps.state.outputs.cfg_hash != '' }} @@ -475,6 +482,10 @@ runs: TARGET_BRANCH: ${{ steps.guard.outputs.target_branch }} SYNC_BRANCH_START_SHA: ${{ steps.guard.outputs.sync_branch_start_sha }} SYNC_STRATEGY: ${{ inputs.sync_strategy }} + ANALYSIS_BRANCH: ${{ inputs.analysis_branch }} + ENGINE_VERSION: ${{ steps.state.outputs.engine_version }} + # Recorded with each analysis-branch commit, so a reader can tell which configuration made it. + CFG_HASH: ${{ steps.state.outputs.cfg_hash }} GITHUB_TOKEN: ${{ inputs.github_token }} GH_TOKEN: ${{ inputs.github_token }} GH_ENTERPRISE_TOKEN: ${{ inputs.github_token }} @@ -550,6 +561,7 @@ runs: ANCESTOR_LOOKUP: ${{ github.server_url == 'https://github.com' && steps.state.outputs.cfg_hash != '' }} # For rewriting the progress comment while a base is built from scratch. PROGRESS_HEADER: ${{ steps.guard.outputs.comment_id }} + ANALYSIS_BRANCH: ${{ inputs.analysis_branch }} BASE_REF: ${{ steps.guard.outputs.base_ref }} REPOSITORY: ${{ github.repository }} GH_HOST: ${{ github.server_url }} diff --git a/docs/COMMIT_STRATEGY.md b/docs/COMMIT_STRATEGY.md index 52407b2..d028050 100644 --- a/docs/COMMIT_STRATEGY.md +++ b/docs/COMMIT_STRATEGY.md @@ -107,6 +107,7 @@ them: |---|---| | the published `codeboarding-base--` artifact with a compatible depth cap | none | | no usable artifact — check out the merge base, seed from a compatible baseline committed there, catch up | one incremental, full if Core requires it | +| the analysis branch (`sync_strategy: branch`): its commit for the merge base, else for the nearest of the merge base's last 100 first-parent ancestors, made under this configuration | none for the merge base itself (`reused`), one incremental otherwise (`incremental`) | | no compatible committed baseline either: the nearest `codeboarding-base--` artifact among the merge base's last 100 first-parent ancestors | one incremental from that commit to the merge base | | none within 100 commits either | full analysis directly, at the configured `depth_cap` | @@ -145,6 +146,72 @@ diffs against, recorded as a digest in `origin.json`. Two runs of the engine ove one commit need not name components identically, so a head descended from one base and a diagram drawn against another would report changes nobody made. +## The analysis branch + +`sync_strategy: branch` saves the analysis to a branch of its own in the same +repository, `codeboarding/analysis` unless `analysis_branch` names another. +`target_branch` is then the code branch sync analyzes; it is never written. + +**What lives where.** The branch is an orphan: it shares no history with the code. +Each sync adds one commit holding the same `.codeboarding/` files the `push` +strategy would commit to the target branch, plus `.codeboarding/source.json`: + +```json +{"schema": 1, "source_branch": "main", "source_sha": "", "generated_at": "", "engine_version": "", "config": ""} +``` + +The commit is `chore(codeboarding): diagram of main @` with two trailers: +`CodeBoarding-Source: ` and `CodeBoarding-Config: `, the same +configuration hash that names the base artifacts (engine version, provider, model, +depth cap). Engine output is never edited; which commit it describes and how it was +made live only in `source.json` and the trailers. The target branch is never +written, not even `.gitattributes`. The base artifacts are still published, named +for the analysed commit. + +**How a sync writes it.** It seeds from the branch tip when the tip was made under +this configuration, and runs incrementally. The generated files are replaced +wholesale; only the checkout's own `.codeboardingignore` and health configuration +are kept. The push is a fast-forward onto the tip it fetched, never forced. Sync +refuses to write to an existing branch that is not an analysis branch (its tip has +no `CodeBoarding-Source` trailer, or holds anything besides `.codeboarding/`), so +pointing `analysis_branch` at a code branch fails instead of emptying it. If the +target branch moved during the analysis, the result is dropped, as with `push`. If +another sync moved the analysis branch, it builds on that tip once. A push the +remote refuses while the tip did not move is a branch rule, and the run fails +saying so. + +The target branch is checked just before the push, not in the same transaction: +if it moves in that window, the branch can end on an analysis of the older commit. +Its trailer still names that commit, so no reader takes it for newer, and the run +queued for the newer commit replaces it. + +**How a review reads it.** After an exact artifact and a baseline committed at the +merge base, a review lists the newest 100 commits of the branch (fetched without +file contents, so the listing costs commit messages only) and matches their +trailers against the merge base's first-parent history, up to 100 commits deep. +Only entries made under this run's configuration count: an entry for the merge +base itself is reused as is, so nothing else would catch a different engine or +model. An entry for an ancestor is caught up incrementally. Entries found only +under another configuration make a full run's reason `incompatible`. Only then +does it look for ancestor artifacts. + +**If the branch is deleted**, the next sync creates it again as a new orphan, +seeding from a saved ancestor artifact when there is one and analyzing in full +otherwise. The history is lost; the current diagram is not. + +**Protecting it.** Sync and review load `static_analysis.pkl` from this branch, and +a pickle runs code when loaded, so whoever can write the branch can run code in +the sync and review workflows. Import +[`analysis-branch-ruleset.json`](analysis-branch-ruleset.json) under Settings, +Rules, Rulesets, New ruleset, Import a ruleset. It blocks creating, updating, +deleting and force-pushing `codeboarding/analysis` for everyone except its bypass +actor, GitHub Actions (integration `15368`), which is what the default +`github.token` pushes as. If sync pushes with a GitHub App token instead, such as +the CodeBoarding Review app (`4021464`), make that app the only bypass actor: +any workflow can use `github.token`, while only the workflows you give the app's +key can push as the app. Rulesets on a private repository need a paid GitHub plan +(Pro, Team or Enterprise); on Free they apply to public repositories only. + ## Trust boundary `static_analysis.pkl` is a Python pickle, so state derived from code the diff --git a/docs/analysis-branch-ruleset.json b/docs/analysis-branch-ruleset.json new file mode 100644 index 0000000..a4a81ac --- /dev/null +++ b/docs/analysis-branch-ruleset.json @@ -0,0 +1,24 @@ +{ + "name": "CodeBoarding analysis branch", + "target": "branch", + "enforcement": "active", + "conditions": { + "ref_name": { + "include": ["refs/heads/codeboarding/analysis"], + "exclude": [] + } + }, + "rules": [ + { "type": "creation" }, + { "type": "update", "parameters": { "update_allows_fetch_and_merge": false } }, + { "type": "deletion" }, + { "type": "non_fast_forward" } + ], + "bypass_actors": [ + { + "actor_id": 15368, + "actor_type": "Integration", + "bypass_mode": "always" + } + ] +} diff --git a/scripts/action/analyze.sh b/scripts/action/analyze.sh index 25a9b5e..c9d5e2e 100755 --- a/scripts/action/analyze.sh +++ b/scripts/action/analyze.sh @@ -126,6 +126,20 @@ analyze_sync() { REQUIRES_FULL=true if [ "$(printf '%s' "${FORCE_FULL:-false}" | tr '[:upper:]' '[:lower:]')" != true ]; then + # The analysis branch's tip is this branch's last analysis. Without a usable + # one, the run below seeds from a saved ancestor or analyzes in full, and + # delivery creates the branch again. + if [ "${SYNC_STRATEGY:-}" = branch ]; then + local tip_entry + tip_entry="$(analysis_branch_index "${REPOSITORY:-}" | awk '{print $1, $3; exit}')" + if [ -z "$tip_entry" ]; then + echo "::notice::$ANALYSIS_BRANCH has no analysis to continue from; this sync creates it." + elif ! usable_config "${tip_entry#* }"; then + echo "::notice::The analysis on $ANALYSIS_BRANCH was made with another engine version or settings; not continuing from it." + elif ! restore_analysis_branch "${REPOSITORY:-}" "${tip_entry%% *}" "$state" "$CHECKOUT_DIR"; then + echo "::notice::Could not read the analysis on $ANALYSIS_BRANCH; not continuing from it." + fi + fi if [ "$(depth_cap_from "$state/analysis.json")" = "$DEPTH_CAP" ]; then incremental "$CHECKOUT_DIR" "$state" fi @@ -260,6 +274,73 @@ keep_user_config() { done } +# sync_strategy: branch keeps one commit per sync on ANALYSIS_BRANCH, each with +# trailers naming the commit it analysed (CodeBoarding-Source) and the +# configuration it ran under (CodeBoarding-Config). Lists them as +# " ", newest first, at most +# ANALYSIS_BRANCH_DEPTH of them; an entry without a config has none to compare. +# Fetched without blobs into a scratch repository: the lookup needs messages, and +# a hundred pickles would cost more than it saves. +ANALYSIS_BRANCH_DEPTH="${ANALYSIS_BRANCH_DEPTH:-100}" +analysis_branch_index() { + local repository="$1" scratch="$RUNNER_TEMP/codeboarding-analysis-index.git" auth + [ -n "${ANALYSIS_BRANCH:-}" ] || return 0 + rm -rf "$scratch" + git init -q --bare "$scratch" + auth="$(printf 'x-access-token:%s' "${GIT_TOKEN:-}" | base64 -w0)" + git -C "$scratch" -c "http.extraheader=AUTHORIZATION: basic $auth" fetch -q --filter=blob:none \ + --depth="$ANALYSIS_BRANCH_DEPTH" "${GITHUB_SERVER_URL%/}/${repository}.git" "refs/heads/$ANALYSIS_BRANCH" 2>/dev/null || + return 0 + git -C "$scratch" log --format='%H %(trailers:key=CodeBoarding-Source,valueonly,separator=%x20) %(trailers:key=CodeBoarding-Config,valueonly,separator=%x20)' FETCH_HEAD | + awk 'NF >= 2 {print $1, $2, (NF >= 3 ? $3 : "-")}' +} +# An entry is only as good as the configuration it ran under: the artifact name +# pins it for saved analyses, the trailer does here. Without a configuration hash +# this run cannot tell, so it uses none, as it reuses no artifact either. +usable_config() { + [ -n "${CFG_HASH:-}" ] && [ "$1" = "$CFG_HASH" ] +} +# Replaces the generated state in $3 with an analysis-branch commit's, keeping the +# user configuration of checkout $4. source.json is provenance, not engine state. +restore_analysis_branch() { + local repository="$1" commit="$2" state="$3" config_from="$4" scratch="$RUNNER_TEMP/codeboarding-analysis-restore" + fetch_commit "$repository" "$commit" || return 1 + rm -rf "$scratch" + mkdir -p "$scratch" + git -C "$CHECKOUT_DIR" archive "$commit" .codeboarding | tar -x -C "$scratch" || return 1 + [ -f "$scratch/.codeboarding/analysis.json" ] || return 1 + rm -f "$scratch/.codeboarding/source.json" + rm -rf "$state" + cp -a "$scratch/.codeboarding" "$state" + keep_user_config "$config_from" "$state" +} +# Seeds $3 from the analysis branch's entry for $2, or for its nearest first-parent +# ancestor that has one under this configuration, keeping checkout $4's user +# configuration. Sets BRANCH_SOURCE to the commit it describes and BRANCH_DISTANCE +# to how far below $2 that is; BRANCH_REASON=incompatible when the only entries +# found were made under another configuration. +BRANCH_SOURCE="" BRANCH_DISTANCE="" BRANCH_REASON="" +seed_from_analysis_branch() { + local repository="$1" tip="$2" state="$3" config_from="$4" index commit line entry="" distance=0 + BRANCH_SOURCE="" BRANCH_DISTANCE="" BRANCH_REASON="" + index="$(analysis_branch_index "$repository")" + [ -n "$index" ] || return 1 + fetch_commit "$repository" "$tip" "$(( CATCHUP_BOUND + 1 ))" || true + for commit in $(git -C "$CHECKOUT_DIR" rev-list --first-parent --max-count=$(( CATCHUP_BOUND + 1 )) "$tip" 2>/dev/null); do + while read -r line; do + if usable_config "${line#* }"; then + entry="${line%% *}" + break + fi + BRANCH_REASON=incompatible + done < <(awk -v source="$commit" '$2 == source {print $1, $3}' <<< "$index") + [ -z "$entry" ] || break + distance=$(( distance + 1 )) + done + [ -n "$entry" ] && restore_analysis_branch "$repository" "$entry" "$state" "$config_from" || return 1 + BRANCH_SOURCE="$commit" BRANCH_DISTANCE="$distance" BRANCH_REASON="" +} + # 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="" @@ -342,6 +423,26 @@ analyze_review() { elif [ -f "$base_state/analysis.json" ]; then full_cause=incompatible fi + # The analysis branch: its entry for the merge base is that commit's own + # analysis, and an entry for an ancestor is caught up like a committed one. + if [ "$REQUIRES_FULL" = true ] && + seed_from_analysis_branch "$REVIEW_BASE_REPO" "$REVIEW_BASE_SHA" "$base_state" "$base_checkout"; then + if [ "$(depth_cap_from "$base_state/analysis.json")" != "$DEPTH_CAP" ]; then + full_cause=incompatible + elif [ "$BRANCH_DISTANCE" -eq 0 ]; then + REQUIRES_FULL=false base_method=reused + else + incremental "$base_checkout" "$base_state" + if [ "$REQUIRES_FULL" = true ]; then + full_cause=incompatible + else + base_method=incremental base_from_sha="$BRANCH_SOURCE" + catchup_commits="$(catchup_count "$BRANCH_SOURCE" "$REVIEW_BASE_SHA")" + fi + fi + elif [ "$BRANCH_REASON" = incompatible ]; then + full_cause=incompatible + fi # Nothing at the merge base to grow from: catch up from the nearest saved # ancestor, and publish the result under the merge base's own name below. if [ "$REQUIRES_FULL" = true ] && seed_from_ancestor "$REVIEW_BASE_REPO" "$REVIEW_BASE_SHA" "$base_state" false "$base_checkout"; then @@ -400,7 +501,8 @@ analyze_review() { # under the same name every run, so normally only a run that produced one # publishes it. The exception is lifetime: a review artifact references a base # by id for its whole retention, so one about to expire is renewed rather than - # left dangling under a review that outlives it. + # left dangling under a review that outlives it. A base read from the baseline + # branch is published too: no artifact holds it yet. local publish_base=false if [ "$base_published" != true ] || [ "${RENEW_BASE:-false}" = true ]; then stage "$base_state" base diff --git a/scripts/action/deliver-sync.sh b/scripts/action/deliver-sync.sh index bbc454c..d503416 100755 --- a/scripts/action/deliver-sync.sh +++ b/scripts/action/deliver-sync.sh @@ -63,6 +63,83 @@ classify_push_failure() { exit 1 } +# sync_strategy: branch keeps the analysis on an orphan branch of its own, one +# fast-forward commit per sync, and never writes to the target branch. +deliver_to_analysis_branch() { + local branch="$ANALYSIS_BRANCH" tree="$RUNNER_TEMP/codeboarding-analysis-tree" + local index="$RUNNER_TEMP/codeboarding-analysis-index" git_dir files new_tree tip parent commit now + git_dir="$(git rev-parse --absolute-git-dir)" + rm -rf "$tree" "$index" + mkdir -p "$tree" + CHECKOUT_DIR="$tree" "$ACTION_PATH/scripts/action/install-sync.sh" > /dev/null + files="$(find "$tree/.codeboarding" -maxdepth 1 -type f | wc -l | tr -d ' ')" + # Engine output is never edited; which commit it describes, and under which + # configuration, lives here and in the commit's trailers only. + python3 -c 'import datetime,json,os,sys +json.dump({ + "schema": 1, + "source_branch": os.environ["TARGET_BRANCH"], + "source_sha": sys.argv[2], + "generated_at": datetime.datetime.now(datetime.timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ"), + "engine_version": os.environ.get("ENGINE_VERSION", ""), + "config": os.environ.get("CFG_HASH", ""), +}, open(sys.argv[1], "w"), indent=2)' "$tree/.codeboarding/source.json" "$BASE_SHA" + GIT_INDEX_FILE="$index" git --git-dir="$git_dir" --work-tree="$tree" -C "$tree" add -A -f .codeboarding + new_tree="$(GIT_INDEX_FILE="$index" git --git-dir="$git_dir" write-tree)" + git config user.name 'codeboarding-review[bot]' + git config user.email 'codeboarding-review[bot]@users.noreply.github.com' + local trailers=(-m "CodeBoarding-Source: $BASE_SHA") + [ -z "${CFG_HASH:-}" ] || trailers=(-m "CodeBoarding-Source: $BASE_SHA +CodeBoarding-Config: $CFG_HASH") + + # Two tries: a concurrent sync that moved the branch for an older commit is + # built on top of once. A second move means a newer run is handling it. + for _ in 1 2; do + git fetch -q "$REMOTE" "$TARGET_BRANCH" + if [ "$(git rev-parse FETCH_HEAD)" != "$BASE_SHA" ]; then + emit_result "$files" false "$BASE_SHA" + echo "::notice::$TARGET_BRANCH advanced during analysis; a newer run should update $branch." + exit 0 + fi + tip="" parent=() + if [ -n "$(git ls-remote "$REMOTE" "refs/heads/$branch")" ]; then + git fetch -q --depth=1 "$REMOTE" "refs/heads/$branch" + # The parent is what was fetched, not what ls-remote saw: the branch may move in between. + tip="$(git rev-parse FETCH_HEAD)" + # Building on any other branch would leave it holding nothing but .codeboarding/. + if [ -z "$(git log -1 --format='%(trailers:key=CodeBoarding-Source,valueonly)' "$tip" | tr -d '[:space:]')" ] || + [ "$(git ls-tree --name-only "$tip")" != .codeboarding ]; then + echo "::error::$branch already exists and is not a CodeBoarding analysis branch, so sync will not write to it. Set analysis_branch to a branch name that is not in use." + exit 1 + fi + parent=(-p "$tip") + if [ "$(git log -1 --format='%(trailers:key=CodeBoarding-Source,valueonly)' "$tip" | tr -d '[:space:]')" = "$BASE_SHA" ] && + git diff --quiet -I '"generated_at"' -I '"timestamp"' "$tip" "$new_tree"; then + emit_result "$files" false "$BASE_SHA" + echo "::notice::$branch already holds this analysis of $TARGET_BRANCH @${BASE_SHA:0:7}." + exit 0 + fi + fi + commit="$(git commit-tree "$new_tree" ${parent[@]+"${parent[@]}"} \ + -m "chore(codeboarding): diagram of $TARGET_BRANCH @${BASE_SHA:0:7}" "${trailers[@]}")" + # Never forced: the parent is the tip just read, so this only ever fast-forwards. + if git push -q "$REMOTE" "$commit:refs/heads/$branch"; then + emit_result "$files" true "$BASE_SHA" + echo "analysis_branch_sha=$commit" >> "$GITHUB_OUTPUT" + exit 0 + fi + now="$(git ls-remote "$REMOTE" "refs/heads/$branch" | awk '{print $1; exit}')" + if [ "$now" = "$tip" ]; then + echo "::error::GitHub refused the push to $branch, most likely because a branch rule protects it. Add the identity sync pushes with (the CodeBoarding app, or GitHub Actions for the default token) as a bypass actor for $branch in the repository's rulesets, or set sync_strategy: push." + exit 1 + fi + done + emit_result "$files" false "$BASE_SHA" + echo "::notice::Another sync keeps updating $branch; leaving it to that run." + exit 0 +} +[ "$SYNC_STRATEGY" != branch ] || deliver_to_analysis_branch + "$ACTION_PATH/scripts/action/install-sync.sh" > "$GENERATED_PATHS" stage_paths=() while IFS= read -r path; do diff --git a/scripts/action/guard.sh b/scripts/action/guard.sh index 84d9261..b469f0e 100755 --- a/scripts/action/guard.sh +++ b/scripts/action/guard.sh @@ -9,6 +9,11 @@ case "$MODE" in *) fail "mode must be review or sync." ;; esac printf 'mode=%s\nskip=false\nevent=%s\n' "$MODE" "$EVENT" >> "$GITHUB_OUTPUT" +# Both modes read the analysis branch, so a name git cannot use fails here, before +# anything is analyzed, rather than reading as a missing branch and costing a full run. +if [ -n "${ANALYSIS_BRANCH:-}" ] && ! git check-ref-format "refs/heads/$ANALYSIS_BRANCH"; then + fail "analysis_branch '$ANALYSIS_BRANCH' is not a valid branch name." +fi if [ "$MODE" = sync ]; then case "$EVENT" in push|workflow_dispatch|schedule) ;; @@ -16,8 +21,8 @@ if [ "$MODE" = sync ]; then esac [ "$REF_TYPE" != tag ] || skip "Sync mode ignores tag pushes." case "$SYNC_STRATEGY" in - push|pull_request) ;; - *) fail "sync_strategy must be push or pull_request." ;; + push|pull_request|branch) ;; + *) fail "sync_strategy must be push, pull_request or branch." ;; esac case "$HEAD_AUTHOR_EMAIL" in codeboarding-review\[bot\]@users.noreply.github.com|codeboarding\[bot\]@users.noreply.github.com) @@ -27,6 +32,12 @@ if [ "$MODE" = sync ]; then target_branch="${TARGET_BRANCH_INPUT:-$REF_NAME}" [ -n "$target_branch" ] || fail "target_branch is required for this event." [ "$SYNC_STRATEGY" != pull_request ] || [ "$target_branch" != codeboarding/sync ] || fail "target_branch must differ from codeboarding/sync." + if [ "$SYNC_STRATEGY" = branch ]; then + [ -n "${ANALYSIS_BRANCH:-}" ] || fail "analysis_branch is required with sync_strategy: branch." + # The analysis branch holds only analysis; a workflow that also fires on it must not analyze it. + [ "$REF_NAME" != "$ANALYSIS_BRANCH" ] || skip "Ignoring a push to the analysis branch $ANALYSIS_BRANCH." + [ "$target_branch" != "$ANALYSIS_BRANCH" ] || fail "target_branch must differ from analysis_branch." + fi sync_branch_start_sha="" if [ "$SYNC_STRATEGY" = pull_request ]; then sync_branch_start_sha="$(gh api "repos/$REPOSITORY/branches/codeboarding%2Fsync" --jq '.commit.sha' 2>/dev/null || true)" diff --git a/tests/test_analysis_branch.py b/tests/test_analysis_branch.py new file mode 100644 index 0000000..92a912f --- /dev/null +++ b/tests/test_analysis_branch.py @@ -0,0 +1,445 @@ +"""sync_strategy: branch saves the analysis to an orphan branch, and reviews read their base from it.""" + +from __future__ import annotations + +import json +import os +import subprocess +import tempfile +import unittest +from pathlib import Path + +from test_action_state import ANALYZE, ENGINE_STUB + +ROOT = Path(__file__).resolve().parent.parent +DELIVER = ROOT / "scripts" / "action" / "deliver-sync.sh" +BRANCH = "codeboarding/analysis" + + +def git(cwd: Path, *args: str) -> str: + return subprocess.run( + ["git", "-c", "user.name=T", "-c", "user.email=t@example.com", "-c", "commit.gpgsign=false", *args], + cwd=str(cwd), + capture_output=True, + text=True, + check=True, + ).stdout.strip() + + +class AnalysisBranchDeliveryTests(unittest.TestCase): + """deliver-sync.sh against a real local remote.""" + + def setUp(self) -> None: + self.temp_dir = tempfile.TemporaryDirectory() + self.root = Path(self.temp_dir.name) + self.remote = self.root / "owner" / "repo.git" + self.remote.mkdir(parents=True) + git(self.remote, "init", "-q", "--bare", "-b", "main") + self.checkout = self.root / "checkout" + git(self.root, "clone", "-q", str(self.remote), str(self.checkout)) + (self.checkout / "app.py").write_text("print('hi')\n", encoding="utf-8") + self._push_code("initial") + self.analysis = self.root / "analysis" + self.analysis.mkdir() + self._analysis("first") + self.core = self.root / "core" + (self.core / "static_analyzer").mkdir(parents=True) + (self.core / "utils.py").write_text( + "ANALYSIS_FILENAME = 'analysis.json'\nFINGERPRINT_FILENAME = 'fingerprint.json'\n", encoding="utf-8" + ) + (self.core / "static_analyzer" / "__init__.py").touch() + (self.core / "static_analyzer" / "analysis_cache.py").write_text( + "STATIC_ANALYSIS_PKL = 'static_analysis.pkl'\nSTATIC_ANALYSIS_SHA = 'static_analysis.sha'\n", + encoding="utf-8", + ) + + def tearDown(self) -> None: + self.temp_dir.cleanup() + + def _push_code(self, message: str) -> str: + (self.checkout / f"{message}.py").write_text("pass\n", encoding="utf-8") + git(self.checkout, "add", "-A") + git(self.checkout, "commit", "-q", "-m", message) + git(self.checkout, "push", "-q", "origin", "HEAD:main") + return git(self.checkout, "rev-parse", "HEAD") + + def _analysis(self, content: str) -> None: + for name in ("analysis.json", "fingerprint.json", "static_analysis.pkl"): + (self.analysis / name).write_text(f"{content} {name}\n", encoding="utf-8") + + def _deliver(self, expect_ok: bool = True) -> tuple[subprocess.CompletedProcess, dict[str, str]]: + output = self.root / "github-output" + output.write_text("", encoding="utf-8") + result = subprocess.run( + [str(DELIVER)], + env={ + "PATH": os.environ["PATH"], + "PYTHONPATH": str(self.core), + "ACTION_PATH": str(ROOT), + "ANALYSIS_DIR": str(self.analysis), + "CHECKOUT_DIR": str(self.checkout), + "GITHUB_OUTPUT": str(output), + "RUNNER_TEMP": str(self.root), + "GITHUB_SERVER_URL": str(self.root), + "GITHUB_TOKEN": "unused", + "GH_HOST": "github.com", + "REPOSITORY": "owner/repo", + "TARGET_BRANCH": "main", + "SYNC_STRATEGY": "branch", + "ANALYSIS_BRANCH": BRANCH, + "ENGINE_VERSION": "0.14.5", + "CFG_HASH": "cfg", + }, + capture_output=True, + text=True, + check=False, + ) + if expect_ok: + self.assertEqual(result.returncode, 0, result.stderr or result.stdout) + values = dict(line.split("=", 1) for line in output.read_text(encoding="utf-8").splitlines() if "=" in line) + return result, values + + def _branch_log(self) -> list[str]: + return git(self.remote, "log", "--format=%H %P", BRANCH).splitlines() + + def test_the_first_sync_creates_an_orphan_branch_with_its_provenance(self) -> None: + main_before = git(self.remote, "rev-parse", "main") + + _result, values = self._deliver() + + (only,) = self._branch_log() + self.assertEqual(only.split(), [values["analysis_branch_sha"]], "the branch must have no parent") + self.assertEqual(values["committed"], "true") + self.assertEqual(values["baseline_sha"], main_before, "artifacts are named for the analysed commit") + files = set(git(self.remote, "ls-tree", "-r", "--name-only", BRANCH).splitlines()) + self.assertEqual( + files, + { + ".codeboarding/analysis.json", + ".codeboarding/fingerprint.json", + ".codeboarding/static_analysis.pkl", + ".codeboarding/source.json", + }, + ) + source = json.loads(git(self.remote, "show", f"{BRANCH}:.codeboarding/source.json")) + self.assertEqual(source["schema"], 1) + self.assertEqual(source["source_branch"], "main") + self.assertEqual(source["source_sha"], main_before) + self.assertEqual(source["engine_version"], "0.14.5") + self.assertEqual(source["config"], "cfg") + self.assertRegex(source["generated_at"], r"^\d{4}-\d\d-\d\dT\d\d:\d\d:\d\dZ$") + message = git(self.remote, "log", "-1", "--format=%B", BRANCH) + self.assertEqual( + message, + f"chore(codeboarding): diagram of main @{main_before[:7]}\n\n" + f"CodeBoarding-Source: {main_before}\nCodeBoarding-Config: cfg", + ) + # The default branch is never written in this strategy. + self.assertEqual(git(self.remote, "rev-parse", "main"), main_before) + + def test_the_next_sync_appends_a_fast_forward_commit(self) -> None: + self._deliver() + first = git(self.remote, "rev-parse", BRANCH) + new_main = self._push_code("feature") + self._analysis("second") + + _result, values = self._deliver() + + log = self._branch_log() + self.assertEqual(len(log), 2) + self.assertEqual(log[0].split(), [values["analysis_branch_sha"], first]) + self.assertIn(f"CodeBoarding-Source: {new_main}", git(self.remote, "log", "-1", "--format=%B", BRANCH)) + + def test_a_rerun_on_the_same_commit_adds_nothing(self) -> None: + self._deliver() + + _result, values = self._deliver() + + self.assertEqual(values["committed"], "false") + self.assertEqual(len(self._branch_log()), 1) + + def test_a_push_refused_by_a_branch_rule_says_how_to_fix_it(self) -> None: + hook = self.remote / "hooks" / "pre-receive" + hook.write_text( + "#!/bin/sh\nwhile read old new ref; do\n" + f' [ "$ref" != refs/heads/{BRANCH} ] || {{ echo "GH013: Repository rule violations found"; exit 1; }}\n' + "done\n", + encoding="utf-8", + ) + hook.chmod(0o755) + + result, _values = self._deliver(expect_ok=False) + + self.assertNotEqual(result.returncode, 0) + self.assertIn(f"GitHub refused the push to {BRANCH}", result.stdout) + self.assertIn("bypass actor", result.stdout) + self.assertIn("sync_strategy: push", result.stdout) + + def test_a_deleted_branch_is_recreated_as_a_new_orphan(self) -> None: + self._deliver() + git(self.remote, "branch", "-D", BRANCH) + self._push_code("later") + + self._deliver() + + (only,) = self._branch_log() + self.assertEqual(len(only.split()), 1, "a recreated branch starts a new history") + + def test_an_existing_branch_that_is_not_an_analysis_branch_is_never_written(self) -> None: + # analysis_branch pointed at a code branch: building on it would leave it + # holding nothing but .codeboarding/. + git(self.checkout, "push", "-q", "origin", f"main:refs/heads/{BRANCH}") + before = git(self.remote, "rev-parse", BRANCH) + + result, values = self._deliver(expect_ok=False) + + self.assertNotEqual(result.returncode, 0) + self.assertIn(f"{BRANCH} already exists and is not a CodeBoarding analysis branch", result.stdout) + self.assertEqual(git(self.remote, "rev-parse", BRANCH), before) + self.assertNotIn("analysis_branch_sha", values) + + def test_a_target_that_moved_during_analysis_keeps_the_branch_unchanged(self) -> None: + self._deliver() + before = git(self.remote, "rev-parse", BRANCH) + other = self.root / "other" + git(self.root, "clone", "-q", str(self.remote), str(other)) + (other / "x.py").write_text("pass\n", encoding="utf-8") + git(other, "add", "-A") + git(other, "commit", "-q", "-m", "x") + git(other, "push", "-q", "origin", "HEAD:main") + self._analysis("stale") + + _result, values = self._deliver() + + self.assertEqual(values["committed"], "false") + self.assertEqual(git(self.remote, "rev-parse", BRANCH), before) + + +class AnalysisBranchReadTests(unittest.TestCase): + """Reviews and sync read the branch: commits c0..c4 on main, branch entries for c1 and c3 + made under configuration `cfg`.""" + + def setUp(self) -> None: + self.temp_dir = tempfile.TemporaryDirectory() + self.root = Path(self.temp_dir.name) + work = self.root / "work" + work.mkdir() + git(work, "init", "-q", "-b", "main") + self.shas = [] + for index in range(5): + (work / f"f{index}.py").write_text("pass\n", encoding="utf-8") + git(work, "add", "-A") + git(work, "commit", "-q", "-m", f"c{index}") + self.shas.append(git(work, "rev-parse", "HEAD")) + git(work, "checkout", "-q", "--orphan", BRANCH) + git(work, "rm", "-rq", "--cached", ".") + for path in work.glob("f*.py"): + path.unlink() + board = work / ".codeboarding" + board.mkdir() + for source in (self.shas[1], self.shas[3]): + (board / "analysis.json").write_text( + json.dumps({"metadata": {"depth_cap": 2}, "components": [source]}), encoding="utf-8" + ) + (board / "static_analysis.pkl").write_text("pickle", encoding="utf-8") + (board / "source.json").write_text(json.dumps({"schema": 1, "source_sha": source}), encoding="utf-8") + git(work, "add", "-A") + git( + work, + "commit", + "-q", + "-m", + f"chore(codeboarding): diagram of main @{source[:7]}", + "-m", + f"CodeBoarding-Source: {source}\nCodeBoarding-Config: cfg", + ) + git(work, "checkout", "-q", "main") + bare = self.root / "origin.git" + git(self.root, "clone", "-q", "--bare", str(work), str(bare)) + git(bare, "config", "uploadpack.allowAnySHA1InWant", "true") + git(bare, "config", "uploadpack.allowFilter", "true") + self.checkout = self.root / "checkout" + git(self.root, "clone", "-q", "--depth=1", "--branch", "main", f"file://{bare}", str(self.checkout)) + + self.bin_dir = self.root / "bin" + self.bin_dir.mkdir() + (self.bin_dir / "codeboarding").write_text(ENGINE_STUB, encoding="utf-8") + (self.bin_dir / "codeboarding").chmod(0o755) + self.engine_log = self.root / "engine.log" + self.engine_log.write_text("", encoding="utf-8") + self.runner = self.root / "runner" + self.runner.mkdir() + self.output = self.root / "github-output" + + def tearDown(self) -> None: + self.temp_dir.cleanup() + + def _analyze(self, **extra: str) -> dict[str, str]: + self.output.write_text("", encoding="utf-8") + result = subprocess.run( + [str(ANALYZE)], + env={ + "PATH": f"{self.bin_dir}:{os.environ['PATH']}", + "GITHUB_OUTPUT": str(self.output), + "RUNNER_TEMP": str(self.runner), + "CB_ENGINE_LOG": str(self.engine_log), + "ACTION_PATH": str(ROOT), + "ANALYSIS_KIND": "review", + "CHECKOUT_DIR": str(self.checkout), + "REVIEW_HEAD_SHA": "head-sha", + "REVIEW_BASE_REPO": "origin", + "REPOSITORY": "origin", + "GITHUB_SERVER_URL": f"file://{self.root}", + "PR_NUMBER": "42", + "ENGINE_VERSION": "0.14.5", + "CFG_HASH": "cfg", + "ANALYSIS_BRANCH": BRANCH, + "BASE_DIR": str(self.root / "state" / "base"), + "WARMSTART_DIR": str(self.root / "state" / "warmstart"), + "STAGE_DIR": str(self.root / "state" / "out"), + "DEPTH_CAP": "2", + **extra, + }, + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(result.returncode, 0, result.stderr or result.stdout) + return dict(line.split("=", 1) for line in self.output.read_text(encoding="utf-8").splitlines() if "=" in line) + + def _modes(self) -> list[str]: + return [json.loads(line)["mode"] for line in self.engine_log.read_text().splitlines()] + + def test_a_branch_entry_for_the_merge_base_is_reused(self) -> None: + values = self._analyze(REVIEW_BASE_SHA=self.shas[3]) + + self.assertEqual(values["base_analysis_method"], "reused") + self.assertEqual(values["base_analysis_reason"], f"{self.shas[3][:7]} already has a saved analysis") + self.assertEqual(self._modes(), ["incremental"], "only the head is analyzed") + base = json.loads(Path(values["base_analysis_path"]).read_text()) + self.assertEqual(base["components"], [self.shas[3]]) + self.assertFalse(Path(values["base_analysis_path"]).with_name("source.json").exists()) + # No artifact holds it yet, so it is published under the merge base's name. + self.assertEqual(values["publish_base"], "true") + + def test_the_nearest_branch_entry_below_the_merge_base_is_caught_up(self) -> None: + values = self._analyze(REVIEW_BASE_SHA=self.shas[4]) + + self.assertEqual(values["base_analysis_method"], "incremental") + self.assertEqual( + values["base_analysis_reason"], + f"updated the analysis of {self.shas[3][:7]} to {self.shas[4][:7]}, 1 commit caught up", + ) + self.assertEqual(self._modes(), ["incremental", "incremental"]) + + def test_an_entry_made_under_another_configuration_is_never_reused(self) -> None: + # Another engine version or model: reusing it as is would diff an old + # configuration's base against a new head. + values = self._analyze(REVIEW_BASE_SHA=self.shas[3], CFG_HASH="othercfg") + + self.assertEqual(values["base_analysis_method"], "full") + self.assertEqual( + values["base_analysis_reason"], + "the existing analysis was incompatible or could not be updated incrementally", + ) + self.assertEqual(self._modes(), ["full", "incremental"]) + + def test_without_a_configuration_hash_no_entry_is_trusted(self) -> None: + values = self._analyze(REVIEW_BASE_SHA=self.shas[3], CFG_HASH="") + + self.assertEqual(values["base_analysis_method"], "full") + + def test_without_the_branch_the_base_is_a_full_analysis(self) -> None: + values = self._analyze(REVIEW_BASE_SHA=self.shas[4], ANALYSIS_BRANCH="codeboarding/none") + + self.assertEqual(values["base_analysis_method"], "full") + self.assertEqual(values["base_analysis_reason"], "no usable analysis was available") + self.assertEqual(self._modes(), ["full", "incremental"]) + + def test_sync_continues_from_the_branch_tip(self) -> None: + self._analyze(ANALYSIS_KIND="sync", SYNC_STRATEGY="branch", FORCE_FULL="false") + + self.assertEqual(self._modes(), ["incremental"]) + + def test_sync_replaces_the_committed_state_with_the_branch_tip(self) -> None: + # Switching from push: the checkout still holds the old committed baseline. + # Only its user configuration may survive; generated files come from the tip. + board = self.checkout / ".codeboarding" + board.mkdir() + (board / "analysis.json").write_text(json.dumps({"metadata": {"depth_cap": 2}, "old": True})) + (board / "static_analysis.pkl").write_text("old pickle") + (board / "static_analysis.sha").write_text("stale\n") + (board / ".codeboardingignore").write_text("docs/\n") + + values = self._analyze(ANALYSIS_KIND="sync", SYNC_STRATEGY="branch", FORCE_FULL="false") + + state = Path(values["analysis_dir"]) + self.assertEqual((state / "static_analysis.pkl").read_text(), "pickle", "the tip's engine state") + self.assertFalse((state / "static_analysis.sha").exists(), "a generated file from the old baseline") + self.assertFalse((state / "source.json").exists()) + self.assertEqual((state / ".codeboardingignore").read_text(), "docs/\n") + + def test_sync_does_not_continue_from_a_tip_made_under_another_configuration(self) -> None: + self._analyze(ANALYSIS_KIND="sync", SYNC_STRATEGY="branch", FORCE_FULL="false", CFG_HASH="othercfg") + + self.assertEqual(self._modes(), ["full"]) + + def test_sync_without_the_branch_analyzes_from_scratch(self) -> None: + self._analyze( + ANALYSIS_KIND="sync", SYNC_STRATEGY="branch", FORCE_FULL="false", ANALYSIS_BRANCH="codeboarding/none" + ) + + self.assertEqual(self._modes(), ["full"]) + + +class AnalysisBranchGuardTests(unittest.TestCase): + def _guard(self, **extra: str) -> tuple[subprocess.CompletedProcess, str]: + with tempfile.TemporaryDirectory() as tmp: + output = Path(tmp) / "github-output" + output.write_text("", encoding="utf-8") + result = subprocess.run( + [str(ROOT / "scripts" / "action" / "guard.sh")], + env={ + "PATH": os.environ["PATH"], + "GITHUB_OUTPUT": str(output), + "MODE": "sync", + "EVENT": "push", + "REF_NAME": "main", + "REF_TYPE": "branch", + "HEAD_AUTHOR_EMAIL": "dev@example.com", + "SYNC_STRATEGY": "branch", + "ANALYSIS_BRANCH": BRANCH, + "REPOSITORY": "owner/repo", + **extra, + }, + capture_output=True, + text=True, + check=False, + ) + return result, output.read_text(encoding="utf-8") + + def test_the_branch_strategy_is_accepted(self) -> None: + result, values = self._guard() + self.assertEqual(result.returncode, 0, result.stdout) + self.assertIn("target_branch=main", values) + + def test_a_push_to_the_analysis_branch_itself_is_ignored(self) -> None: + result, values = self._guard(REF_NAME=BRANCH) + self.assertEqual(result.returncode, 0, result.stdout) + self.assertIn("skip=true", values) + + def test_a_branch_name_git_cannot_use_fails_before_anything_runs(self) -> None: + for mode in ("sync", "review"): + for name in ("codeboarding/a..b", "codeboarding/with space", "codeboarding/trailing."): + result, _values = self._guard(MODE=mode, ANALYSIS_BRANCH=name) + self.assertNotEqual(result.returncode, 0, (mode, name)) + self.assertIn("is not a valid branch name", result.stdout) + + def test_target_branch_must_differ_from_the_analysis_branch(self) -> None: + result, _values = self._guard(TARGET_BRANCH_INPUT=BRANCH) + self.assertNotEqual(result.returncode, 0) + self.assertIn("target_branch must differ from analysis_branch", result.stdout) + + +if __name__ == "__main__": + unittest.main() From 608b37dd482e8b4b595b2ee08f387b3a8b3f8eb4 Mon Sep 17 00:00:00 2001 From: ivanmilevtues Date: Sat, 10 Oct 2026 02:56:49 +0200 Subject: [PATCH 2/2] feat: name sync inputs for what they do, and move base timings to telemetry target_branch becomes synced_branch, sync_strategy becomes save_baseline_to (synced_branch | pull_request | baseline_branch), and the unreleased analysis_branch becomes baseline_branch with default codeboarding/baseline. The released names keep working through old_api_migrator.sh, the only reader of deprecated inputs. The base_analysis_method output and the review comment's base line and timers are removed. Engine runs carry CODEBOARDING_RUN_ID tagged base, head or sync, so the engine's own telemetry shows how each base was obtained. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/codeboarding-sync.yml | 10 +- README.md | 117 +++++++++++++++--- action.yml | 75 ++++++----- docs/COMMIT_STRATEGY.md | 56 +++++---- ...eset.json => baseline-branch-ruleset.json} | 4 +- scripts/action/analyze.sh | 74 +++++------ scripts/action/build-review-artifact.sh | 5 +- scripts/action/build-review-comment.sh | 34 +---- scripts/action/deliver-sync.sh | 56 ++++----- scripts/action/fetch-state.sh | 4 - scripts/action/guard.sh | 34 ++--- scripts/action/old_api_migrator.sh | 32 +++++ scripts/action/sync-summary.sh | 2 +- tests/test_action_inputs.py | 8 ++ tests/test_action_state.py | 22 ++-- tests/test_action_sync.py | 4 +- ...ysis_branch.py => test_baseline_branch.py} | 69 ++++++----- tests/test_old_api_migrator.py | 77 ++++++++++++ tests/test_review_artifact_metadata.py | 8 +- tests/test_review_comment.py | 73 ----------- 20 files changed, 441 insertions(+), 323 deletions(-) rename docs/{analysis-branch-ruleset.json => baseline-branch-ruleset.json} (81%) create mode 100755 scripts/action/old_api_migrator.sh rename tests/{test_analysis_branch.py => test_baseline_branch.py} (88%) create mode 100644 tests/test_old_api_migrator.py diff --git a/.github/workflows/codeboarding-sync.yml b/.github/workflows/codeboarding-sync.yml index 2660515..c947eca 100644 --- a/.github/workflows/codeboarding-sync.yml +++ b/.github/workflows/codeboarding-sync.yml @@ -41,12 +41,12 @@ on: type: boolean required: false default: false - sync_strategy: - description: 'Deliver directly to the target branch or open/update a rolling baseline PR.' + save_baseline_to: + description: 'Commit to main, open/update a rolling baseline PR, or commit to codeboarding/baseline.' type: choice - options: [push, pull_request] + options: [synced_branch, pull_request, baseline_branch] required: false - default: push + default: synced_branch # No workflow-level permissions: the single job below requests only what it # needs (least privilege), so the default token starts with none. @@ -144,7 +144,7 @@ jobs: # CodeBoarding's own repositories run on the CodeBoarding plan. llm: license license_key: ${{ secrets.CODEBOARDING_LICENSE }} - sync_strategy: ${{ inputs.sync_strategy || 'push' }} + save_baseline_to: ${{ inputs.save_baseline_to || 'synced_branch' }} force_full: ${{ inputs.force_full || false }} # App token authenticates the baseline push so the commit is attributed # to the CodeBoarding App (logo avatar). Falls back to the workflow token, diff --git a/README.md b/README.md index 80f6820..2bea5c9 100644 --- a/README.md +++ b/README.md @@ -272,15 +272,17 @@ jobs: with: mode: sync llm: hosted - target_branch: main + synced_branch: main force_full: ${{ inputs.force_full || false }} ``` -The first run, `force_full: true`, or an incompatible baseline causes a full analysis. Otherwise sync asks Core for an incremental update. If the generated state is unchanged, no commit is created. If the target advances while analysis is running, the stale result is not rebased onto code it did not analyze; the newer push run is allowed to produce the current baseline. +`synced_branch` is the code branch sync keeps an up-to-date analysis of; it defaults to the branch that triggered the run. `save_baseline_to` chooses where that analysis is saved: `synced_branch` (the default, a commit on that branch), `pull_request` (a rolling PR into it) or `baseline_branch` (a branch of its own, see below). + +The first run, `force_full: true`, or an incompatible baseline causes a full analysis. Otherwise sync asks Core for an incremental update. If the generated state is unchanged, no commit is created. If the synced branch advances while analysis is running, the stale result is not rebased onto code it did not analyze; the newer push run is allowed to produce the current baseline. ### Protected branches -Set `sync_strategy: pull_request` and grant `pull-requests: write`: +Set `save_baseline_to: pull_request` and grant `pull-requests: write`: ```yaml permissions: @@ -293,36 +295,39 @@ permissions: with: mode: sync llm: hosted - target_branch: main - sync_strategy: pull_request + synced_branch: main + save_baseline_to: pull_request ``` -Generation is identical to direct push. Only delivery changes: the same commit is force-with-lease pushed to the machine-owned `codeboarding/sync` branch and one rolling PR is opened into `target_branch`. When there is no longer a generated diff, an obsolete rolling PR is closed. +Generation is identical to direct push. Only delivery changes: the same commit is force-with-lease pushed to the machine-owned `codeboarding/sync` branch and one rolling PR is opened into `synced_branch`. When there is no longer a generated diff, an obsolete rolling PR is closed. With the default `github.token`, the repository or organization must allow GitHub Actions to create pull requests. A GitHub App token or PAT can instead be passed as `github_token`. The same input is used for review comments and sync delivery. ### Save the diagram to a branch of its own -Set `sync_strategy: branch` to keep the analysis off your code branches entirely: +Set `save_baseline_to: baseline_branch` to keep the analysis off your code branches entirely: ```yaml - uses: CodeBoarding/CodeBoarding-action@v1 with: mode: sync llm: hosted - target_branch: main - sync_strategy: branch + synced_branch: main + save_baseline_to: baseline_branch ``` -Here `target_branch` is the code branch sync analyzes, and it is only read. Each sync adds one commit to `codeboarding/analysis` in the same repository (set `analysis_branch` to change the name), an orphan branch that shares no history with `main`. It holds the same `.codeboarding/` files sync would otherwise commit to `main`, plus `.codeboarding/source.json` naming the commit they describe and the configuration that made them; the commit message carries both as `CodeBoarding-Source:` and `CodeBoarding-Config:` trailers. Pushes only ever fast-forward, `main` is never written, and no pull request is opened. Reviews read their base from the branch, and the web platform reads the latest diagram from it. The [analysis branch section](docs/COMMIT_STRATEGY.md#the-analysis-branch) covers what happens if the branch is deleted, and a ruleset you should import to protect it: sync and review load a pickle from it. +`main` is then only read. Each sync adds one commit to `codeboarding/baseline` in the same repository (set `baseline_branch` to change the name), an orphan branch that shares no history with `main`. It holds the same `.codeboarding/` files sync would otherwise commit to `main`, plus `.codeboarding/source.json` naming the commit they describe and the configuration that made them; the commit message carries both as `CodeBoarding-Source:` and `CodeBoarding-Config:` trailers. Pushes only ever fast-forward, `main` is never written, and no pull request is opened. Reviews read their base from the branch, and the web platform reads the latest diagram from it. The [baseline branch section](docs/COMMIT_STRATEGY.md#the-baseline-branch) covers what happens if the branch is deleted, and a ruleset you should import to protect it: sync and review load a pickle from it. + +No permission beyond the `contents: write` every sync already needs: the first sync creates the branch with an ordinary push. If you renamed it with `baseline_branch`, set the same name in your review workflow too, since reviews read it — or keep both jobs in one workflow, below. -**Moving an existing setup.** Nothing changes until you opt in: `push` and `pull_request` keep working as before. To switch, paste this into your coding agent: +**Moving an existing setup.** Nothing changes until you opt in: the default keeps committing to the synced branch. To switch, paste this into your coding agent: ```text -Move this repository's CodeBoarding sync to sync_strategy: branch. +Move this repository's CodeBoarding sync to save_baseline_to: baseline_branch. 1. In the workflow that runs CodeBoarding/CodeBoarding-action with mode: sync, set - `sync_strategy: branch` in its `with:` block (replace push or pull_request). - Keep every other input. + `save_baseline_to: baseline_branch` in its `with:` block, replacing any + sync_strategy. Rename target_branch to synced_branch if it is set. Keep every + other input. 2. Delete the generated files under .codeboarding/ from the default branch, keeping the user configuration: .codeboarding/.codeboardingignore, .codeboarding/health/health_config.json and .codeboarding/health/.healthignore. @@ -334,7 +339,81 @@ Move this repository's CodeBoarding sync to sync_strategy: branch. pull request from the codeboarding/sync branch and delete that branch. ``` -The first sync after the merge creates `codeboarding/analysis`, catching up from a saved analysis when there is one. +The first sync after the merge creates `codeboarding/baseline`, catching up from a saved analysis when there is one. + +### Review and sync in one workflow + +The two workflows above can be one file with two jobs. Each job keeps its own permissions, and settings both modes read live in one `env:` block, so review always looks for the baseline where sync saves it: + +```yaml +name: CodeBoarding + +on: + pull_request: + types: [opened, reopened, synchronize] + issue_comment: + types: [created] + push: + branches: [main] # the synced branch; `on:` cannot read env + workflow_dispatch: + inputs: + force_full: + description: Rebuild without the saved baseline + type: boolean + default: false + +env: + SYNCED_BRANCH: main + BASELINE_BRANCH: codeboarding/baseline + +permissions: {} + +# Reviews queue per pull request, syncs per branch. +concurrency: + group: codeboarding-${{ github.event.pull_request.number || github.event.issue.number || github.ref_name }} + cancel-in-progress: false + +jobs: + review: + if: > + (github.event_name == 'pull_request' && + github.event.pull_request.head.repo.full_name == github.repository) || + (github.event_name == 'issue_comment' && github.event.issue.pull_request != null && + startsWith(github.event.comment.body, '/codeboarding') && + contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association)) + runs-on: ubuntu-latest + timeout-minutes: 60 + permissions: + contents: read + actions: read + pull-requests: write + issues: write + id-token: write + steps: + - uses: CodeBoarding/CodeBoarding-action@v1 + with: + llm: hosted + baseline_branch: ${{ env.BASELINE_BRANCH }} + + sync: + if: github.event_name == 'push' || github.event_name == 'workflow_dispatch' + runs-on: ubuntu-latest + timeout-minutes: 60 + permissions: + contents: write + id-token: write + steps: + - uses: CodeBoarding/CodeBoarding-action@v1 + with: + mode: sync + llm: hosted + synced_branch: ${{ env.SYNCED_BRANCH }} + save_baseline_to: baseline_branch + baseline_branch: ${{ env.BASELINE_BRANCH }} + force_full: ${{ inputs.force_full || false }} +``` + +With `save_baseline_to: baseline_branch` sync never commits to `main`, so the `push` trigger needs no `paths-ignore`. Existing two-file setups keep working unchanged; this is only a different way to call the same action. ## Inputs @@ -351,12 +430,14 @@ The first sync after the merge creates `codeboarding/analysis`, catching up from | `parsing_model` | both | empty | Parsing-only override for `model`. | | `depth_cap` | both | `2` | Positive integer maximum analysis depth, including full-analysis fallbacks. Changing it rebuilds incompatible state. | | `github_token` | both | `${{ github.token }}` | Token for comments and sync delivery. | -| `sync_strategy` | sync | `push` | Where sync saves the analysis: `push` (a commit on `target_branch`), `pull_request` (a rolling PR into it), or `branch` (commits on `analysis_branch`). | -| `analysis_branch` | both | `codeboarding/analysis` | Branch in this repository that `sync_strategy: branch` saves the analysis to; reviews read their base from it when it exists. | -| `target_branch` | sync | event branch | Code branch sync analyzes. With `push` or `pull_request` it also receives the analysis commit or rolling PR; with `branch` it is only read. | +| `synced_branch` | sync | event branch | Code branch sync keeps an up-to-date analysis of. | +| `save_baseline_to` | sync | `synced_branch` | Where sync saves the analysis: `synced_branch` (a commit on it), `pull_request` (a rolling PR into it), or `baseline_branch` (a commit on `baseline_branch`; the synced branch is never written). | +| `baseline_branch` | both | `codeboarding/baseline` | Branch `save_baseline_to: baseline_branch` saves the analysis to; reviews read their base from it when it exists. | | `force_full` | sync | `false` | Ignore the committed baseline for this run. | | `warmstart_retention_days` | review | `1` | Days to keep the reusable analysis. Only the next run reads it. | +`target_branch` and `sync_strategy` are deprecated names for `synced_branch` and `save_baseline_to` (`push` is `synced_branch`, `pull_request` is `pull_request`). They still work, with a warning; setting a value under both names fails. + The `/codeboarding` command, comment heading, Mermaid direction (`LR`), hosted webview URL, rolling sync branch, commit message, and CodeBoarding 0.14.5 version are intentionally fixed rather than exposed as configuration. Review mode needs no sync workflow or committed `.codeboarding` directory. If no diff --git a/action.yml b/action.yml index 9401b62..171c267 100644 --- a/action.yml +++ b/action.yml @@ -142,18 +142,18 @@ inputs: description: 'Token used for comments and sync delivery.' required: false default: ${{ github.token }} - sync_strategy: - description: 'Where sync saves the analysis: push (a commit on target_branch), pull_request (a rolling PR into target_branch), or branch (commits on analysis_branch; target_branch is never written).' + synced_branch: + description: 'Code branch that sync keeps an up-to-date analysis of. Defaults to the event branch.' required: false - default: 'push' - analysis_branch: - description: 'Branch in this repository that sync_strategy branch saves the analysis to, one commit per sync. Reviews read their base analysis from it when it exists.' + default: '' + save_baseline_to: + description: 'Where sync saves the analysis: synced_branch (a commit on synced_branch), pull_request (a rolling PR into synced_branch), or baseline_branch (a commit on baseline_branch; synced_branch is never written).' required: false - default: 'codeboarding/analysis' - target_branch: - description: 'Code branch sync mode analyzes. With sync_strategy push or pull_request the analysis is also committed to it; with branch it is only read. Defaults to the event branch.' + default: 'synced_branch' + baseline_branch: + description: 'Branch that save_baseline_to: baseline_branch stores the analysis on, one commit per sync. Reviews read their base analysis from it when it exists.' required: false - default: '' + default: 'codeboarding/baseline' force_full: description: 'Run a full sync analysis instead of reusing the committed baseline.' required: false @@ -162,6 +162,17 @@ inputs: description: 'Days to keep the reusable analysis. Only the next run reads it.' required: false default: '1' + # Deprecated inputs. scripts/action/old_api_migrator.sh is the only reader. + target_branch: + description: 'Deprecated: use synced_branch.' + deprecationMessage: 'target_branch is deprecated; use synced_branch.' + required: false + default: '' + sync_strategy: + description: 'Deprecated: use save_baseline_to (push is synced_branch, pull_request is pull_request).' + deprecationMessage: 'sync_strategy is deprecated; use save_baseline_to.' + required: false + default: '' outputs: llm_tier: description: 'Resolved credential tier: hosted, license, byok, or byok+license.' @@ -187,9 +198,6 @@ 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 }} @@ -203,15 +211,25 @@ outputs: description: 'Whether sync delivered a baseline commit.' value: ${{ steps.sync_commit.outputs.committed }} sync_pr_url: - description: 'Rolling sync PR URL when sync_strategy is pull_request.' + description: 'Rolling sync PR URL when save_baseline_to is pull_request.' value: ${{ steps.sync_commit.outputs.sync_pr_url }} sync_pr_number: - description: 'Rolling sync PR number when sync_strategy is pull_request.' + description: 'Rolling sync PR number when save_baseline_to is pull_request.' value: ${{ steps.sync_commit.outputs.sync_pr_number }} runs: using: 'composite' steps: + - name: Translate deprecated inputs + id: inputs + shell: bash + env: + SYNCED_BRANCH: ${{ inputs.synced_branch }} + SAVE_BASELINE_TO: ${{ inputs.save_baseline_to }} + OLD_TARGET_BRANCH: ${{ inputs.target_branch }} + OLD_SYNC_STRATEGY: ${{ inputs.sync_strategy }} + run: "$GITHUB_ACTION_PATH/scripts/action/old_api_migrator.sh" + - name: Resolve event id: guard shell: bash @@ -225,9 +243,9 @@ runs: REF_TYPE: ${{ github.ref_type }} EVENT_SHA: ${{ github.sha }} HEAD_AUTHOR_EMAIL: ${{ github.event.head_commit.author.email }} - TARGET_BRANCH_INPUT: ${{ inputs.target_branch }} - SYNC_STRATEGY: ${{ inputs.sync_strategy }} - ANALYSIS_BRANCH: ${{ inputs.analysis_branch }} + SYNCED_BRANCH_INPUT: ${{ steps.inputs.outputs.synced_branch }} + SAVE_BASELINE_TO: ${{ steps.inputs.outputs.save_baseline_to }} + BASELINE_BRANCH: ${{ inputs.baseline_branch }} COMMENT_BODY: ${{ github.event.comment.body }} AUTHOR_ASSOCIATION: ${{ github.event.comment.author_association }} ISSUE_PR_URL: ${{ github.event.issue.pull_request.url }} @@ -456,8 +474,8 @@ runs: CHECKOUT_DIR: ${{ github.workspace }}/.codeboarding-target STAGE_DIR: ${{ runner.temp }}/cb-state/${{ github.action }}/out FORCE_FULL: ${{ inputs.force_full }} - SYNC_STRATEGY: ${{ inputs.sync_strategy }} - ANALYSIS_BRANCH: ${{ inputs.analysis_branch }} + SAVE_BASELINE_TO: ${{ steps.inputs.outputs.save_baseline_to }} + BASELINE_BRANCH: ${{ inputs.baseline_branch }} CFG_HASH: ${{ steps.state.outputs.cfg_hash }} # Lets a branch without a usable committed baseline catch up from a saved analysis. ANCESTOR_LOOKUP: ${{ github.server_url == 'https://github.com' && steps.state.outputs.cfg_hash != '' }} @@ -479,12 +497,12 @@ runs: ACTION_PATH: ${{ github.action_path }} ANALYSIS_DIR: ${{ steps.sync_analyze.outputs.analysis_dir }} CHECKOUT_DIR: ${{ github.workspace }}/.codeboarding-target - TARGET_BRANCH: ${{ steps.guard.outputs.target_branch }} + SYNCED_BRANCH: ${{ steps.guard.outputs.synced_branch }} SYNC_BRANCH_START_SHA: ${{ steps.guard.outputs.sync_branch_start_sha }} - SYNC_STRATEGY: ${{ inputs.sync_strategy }} - ANALYSIS_BRANCH: ${{ inputs.analysis_branch }} + SAVE_BASELINE_TO: ${{ steps.inputs.outputs.save_baseline_to }} + BASELINE_BRANCH: ${{ inputs.baseline_branch }} ENGINE_VERSION: ${{ steps.state.outputs.engine_version }} - # Recorded with each analysis-branch commit, so a reader can tell which configuration made it. + # Recorded with each baseline-branch commit, so a reader can tell which configuration made it. CFG_HASH: ${{ steps.state.outputs.cfg_hash }} GITHUB_TOKEN: ${{ inputs.github_token }} GH_TOKEN: ${{ inputs.github_token }} @@ -537,7 +555,7 @@ runs: MODE: ${{ steps.sync_analyze.outputs.analysis_mode }} COMMITTED: ${{ steps.sync_commit.outputs.committed }} FILES: ${{ steps.sync_commit.outputs.files_written }} - STRATEGY: ${{ inputs.sync_strategy }} + SAVE_BASELINE_TO: ${{ steps.inputs.outputs.save_baseline_to }} PR_URL: ${{ steps.sync_commit.outputs.sync_pr_url }} run: "$GITHUB_ACTION_PATH/scripts/action/sync-summary.sh" @@ -554,14 +572,13 @@ 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 }} # Lets a branch without a usable committed baseline catch up from a saved analysis. ANCESTOR_LOOKUP: ${{ github.server_url == 'https://github.com' && steps.state.outputs.cfg_hash != '' }} # For rewriting the progress comment while a base is built from scratch. PROGRESS_HEADER: ${{ steps.guard.outputs.comment_id }} - ANALYSIS_BRANCH: ${{ inputs.analysis_branch }} + BASELINE_BRANCH: ${{ inputs.baseline_branch }} BASE_REF: ${{ steps.guard.outputs.base_ref }} REPOSITORY: ${{ github.repository }} GH_HOST: ${{ github.server_url }} @@ -640,8 +657,6 @@ runs: 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 @@ -673,10 +688,6 @@ runs: HEAD_SHA: ${{ steps.guard.outputs.head_sha }} # Analysed files whose content hash differs between base and head, from the render step. ANALYSED_FILES_CHANGED: ${{ steps.review_render.outputs.analysed_files_changed }} - 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 d028050..bf801c2 100644 --- a/docs/COMMIT_STRATEGY.md +++ b/docs/COMMIT_STRATEGY.md @@ -3,11 +3,12 @@ Where CodeBoarding keeps analysis state, and how a review run finds the two graphs it compares. -## Three stores +## Where analysis is stored | Store | Holds | Lifetime | Who can read it | |---|---|---|---| -| **Git** (`sync` commits) | `.codeboarding/` on the target branch | forever | anyone with repo read | +| **Git, synced branch** (`save_baseline_to: synced_branch` or `pull_request`) | `.codeboarding/` on the synced branch | forever | anyone with repo read | +| **Git, baseline branch** (`save_baseline_to: baseline_branch`) | one commit per sync on `codeboarding/baseline` | forever | anyone with repo read | | **Workflow artifacts** | every analysis this action reuses or publishes | a retention window | any run with `actions: read`, plus humans | | ~~Actions cache~~ | — | — | not used | @@ -74,8 +75,6 @@ never fires. | `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, committed or saved, 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 @@ -107,7 +106,7 @@ them: |---|---| | the published `codeboarding-base--` artifact with a compatible depth cap | none | | no usable artifact — check out the merge base, seed from a compatible baseline committed there, catch up | one incremental, full if Core requires it | -| the analysis branch (`sync_strategy: branch`): its commit for the merge base, else for the nearest of the merge base's last 100 first-parent ancestors, made under this configuration | none for the merge base itself (`reused`), one incremental otherwise (`incremental`) | +| the baseline branch (`save_baseline_to: baseline_branch`): its commit for the merge base, else for the nearest of the merge base's last 100 first-parent ancestors, made under this configuration | none for the merge base itself (`reused`), one incremental otherwise (`incremental`) | | no compatible committed baseline either: the nearest `codeboarding-base--` artifact among the merge base's last 100 first-parent ancestors | one incremental from that commit to the merge base | | none within 100 commits either | full analysis directly, at the configured `depth_cap` | @@ -115,9 +114,16 @@ A trusted run that computed the base publishes it, so the next pull request forking from that commit gets the first row; that includes a base caught up from an ancestor. 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. +`base_analysis_reason`. The review comment does not: how the base was obtained is +for us, not the reader. While a base is computed, the progress comment says so in +two steps, with the elapsed time and the reason. + +**Telemetry.** Every engine run carries `CODEBOARDING_RUN_ID=gh---`, +the role being `base`, `head` or `sync`, so the engine's own `analysis_started` / +`analysis_completed` events (mode, duration, tokens) can be read per run. A review +with no `base` events reused its base; `base` events say whether it was caught up +or built in full. Where a caught-up base started from is only in +`base_analysis_reason`. The ancestor lookup walks the merge base's first-parent history, deepening the shallow checkout to 101 commits, then pages through the repository's artifacts @@ -146,25 +152,26 @@ diffs against, recorded as a digest in `origin.json`. Two runs of the engine ove one commit need not name components identically, so a head descended from one base and a diagram drawn against another would report changes nobody made. -## The analysis branch +## The baseline branch -`sync_strategy: branch` saves the analysis to a branch of its own in the same -repository, `codeboarding/analysis` unless `analysis_branch` names another. -`target_branch` is then the code branch sync analyzes; it is never written. +`save_baseline_to: baseline_branch` saves the analysis to a branch of its own in +the same repository, `codeboarding/baseline` unless `baseline_branch` names +another. The synced branch is then only read. **What lives where.** The branch is an orphan: it shares no history with the code. -Each sync adds one commit holding the same `.codeboarding/` files the `push` -strategy would commit to the target branch, plus `.codeboarding/source.json`: +Each sync adds one commit holding the same `.codeboarding/` files +`save_baseline_to: synced_branch` would commit to the synced branch, plus +`.codeboarding/source.json`: ```json -{"schema": 1, "source_branch": "main", "source_sha": "", "generated_at": "", "engine_version": "", "config": ""} +{"schema": 1, "synced_branch": "main", "source_sha": "", "generated_at": "", "engine_version": "", "config": ""} ``` The commit is `chore(codeboarding): diagram of main @` with two trailers: `CodeBoarding-Source: ` and `CodeBoarding-Config: `, the same configuration hash that names the base artifacts (engine version, provider, model, depth cap). Engine output is never edited; which commit it describes and how it was -made live only in `source.json` and the trailers. The target branch is never +made live only in `source.json` and the trailers. The synced branch is never written, not even `.gitattributes`. The base artifacts are still published, named for the analysed commit. @@ -172,15 +179,15 @@ for the analysed commit. this configuration, and runs incrementally. The generated files are replaced wholesale; only the checkout's own `.codeboardingignore` and health configuration are kept. The push is a fast-forward onto the tip it fetched, never forced. Sync -refuses to write to an existing branch that is not an analysis branch (its tip has +refuses to write to an existing branch that is not a baseline branch (its tip has no `CodeBoarding-Source` trailer, or holds anything besides `.codeboarding/`), so -pointing `analysis_branch` at a code branch fails instead of emptying it. If the -target branch moved during the analysis, the result is dropped, as with `push`. If -another sync moved the analysis branch, it builds on that tip once. A push the +pointing `baseline_branch` at a code branch fails instead of emptying it. If the +synced branch moved during the analysis, the result is dropped, as when committing +to it. If another sync moved the baseline branch, it builds on that tip once. A push the remote refuses while the tip did not move is a branch rule, and the run fails saying so. -The target branch is checked just before the push, not in the same transaction: +The synced branch is checked just before the push, not in the same transaction: if it moves in that window, the branch can end on an analysis of the older commit. Its trailer still names that commit, so no reader takes it for newer, and the run queued for the newer commit replaces it. @@ -202,14 +209,15 @@ otherwise. The history is lost; the current diagram is not. **Protecting it.** Sync and review load `static_analysis.pkl` from this branch, and a pickle runs code when loaded, so whoever can write the branch can run code in the sync and review workflows. Import -[`analysis-branch-ruleset.json`](analysis-branch-ruleset.json) under Settings, +[`baseline-branch-ruleset.json`](baseline-branch-ruleset.json) under Settings, Rules, Rulesets, New ruleset, Import a ruleset. It blocks creating, updating, -deleting and force-pushing `codeboarding/analysis` for everyone except its bypass +deleting and force-pushing `codeboarding/baseline` for everyone except its bypass actor, GitHub Actions (integration `15368`), which is what the default `github.token` pushes as. If sync pushes with a GitHub App token instead, such as the CodeBoarding Review app (`4021464`), make that app the only bypass actor: any workflow can use `github.token`, while only the workflows you give the app's -key can push as the app. Rulesets on a private repository need a paid GitHub plan +key can push as the app. Creation is covered too, so the bypass actor must be the +identity sync pushes as before the first sync, or that sync fails on the rule. Rulesets on a private repository need a paid GitHub plan (Pro, Team or Enterprise); on Free they apply to public repositories only. ## Trust boundary diff --git a/docs/analysis-branch-ruleset.json b/docs/baseline-branch-ruleset.json similarity index 81% rename from docs/analysis-branch-ruleset.json rename to docs/baseline-branch-ruleset.json index a4a81ac..a96dfb7 100644 --- a/docs/analysis-branch-ruleset.json +++ b/docs/baseline-branch-ruleset.json @@ -1,10 +1,10 @@ { - "name": "CodeBoarding analysis branch", + "name": "CodeBoarding baseline branch", "target": "branch", "enforcement": "active", "conditions": { "ref_name": { - "include": ["refs/heads/codeboarding/analysis"], + "include": ["refs/heads/codeboarding/baseline"], "exclude": [] } }, diff --git a/scripts/action/analyze.sh b/scripts/action/analyze.sh index c9d5e2e..fc6b1ce 100755 --- a/scripts/action/analyze.sh +++ b/scripts/action/analyze.sh @@ -12,9 +12,16 @@ parse_output() { REQUIRES_FULL="$(awk -F= '$1 == "requires_full_analysis" {print $2; exit}' <<< "$output")" ANALYSIS_PATH="$(awk -F= '$1 == "analysis_path" {print substr($0, index($0, "=") + 1); exit}' <<< "$output")" } +# Tags the engine's own telemetry with this run and the analysis it belongs to +# (base, head or sync), so its events can be read per run: a review with no base +# events reused its base, one with base events caught it up or built it in full. +ENGINE_ROLE="" +engine_run_id() { + [ -z "${GITHUB_RUN_ID:-}" ] || printf 'gh-%s-%s-%s' "$GITHUB_RUN_ID" "${GITHUB_RUN_ATTEMPT:-1}" "$ENGINE_ROLE" +} incremental() { local checkout="$1" output_dir="$2" output - output="$(python3 "$ACTION_PATH/scripts/analyze_repository.py" incremental \ + output="$(CODEBOARDING_RUN_ID="$(engine_run_id)" python3 "$ACTION_PATH/scripts/analyze_repository.py" incremental \ --checkout "$checkout" --output-dir "$output_dir")" parse_output "$output" if [ "$ANALYSIS_MODE" != incremental ] || { [ "$REQUIRES_FULL" != true ] && [ ! -f "$ANALYSIS_PATH" ]; }; then @@ -24,7 +31,7 @@ incremental() { } full() { local checkout="$1" output_dir="$2" depth="$3" output - output="$(python3 "$ACTION_PATH/scripts/analyze_repository.py" full \ + output="$(CODEBOARDING_RUN_ID="$(engine_run_id)" python3 "$ACTION_PATH/scripts/analyze_repository.py" full \ --checkout "$checkout" --output-dir "$output_dir" --depth-cap "$depth")" parse_output "$output" if [ "$ANALYSIS_MODE" != full ] || [ ! -f "$ANALYSIS_PATH" ]; then @@ -123,21 +130,22 @@ analyze_sync() { local work="$RUNNER_TEMP/codeboarding-sync" state="$RUNNER_TEMP/codeboarding-sync/analysis" rm -rf "$work" seed_state "$CHECKOUT_DIR" "$state" + ENGINE_ROLE=sync REQUIRES_FULL=true if [ "$(printf '%s' "${FORCE_FULL:-false}" | tr '[:upper:]' '[:lower:]')" != true ]; then - # The analysis branch's tip is this branch's last analysis. Without a usable + # The baseline branch's tip is this branch's last analysis. Without a usable # one, the run below seeds from a saved ancestor or analyzes in full, and # delivery creates the branch again. - if [ "${SYNC_STRATEGY:-}" = branch ]; then + if [ "${SAVE_BASELINE_TO:-}" = baseline_branch ]; then local tip_entry - tip_entry="$(analysis_branch_index "${REPOSITORY:-}" | awk '{print $1, $3; exit}')" + tip_entry="$(baseline_branch_index "${REPOSITORY:-}" | awk '{print $1, $3; exit}')" if [ -z "$tip_entry" ]; then - echo "::notice::$ANALYSIS_BRANCH has no analysis to continue from; this sync creates it." + echo "::notice::$BASELINE_BRANCH has no analysis to continue from; this sync creates it." elif ! usable_config "${tip_entry#* }"; then - echo "::notice::The analysis on $ANALYSIS_BRANCH was made with another engine version or settings; not continuing from it." - elif ! restore_analysis_branch "${REPOSITORY:-}" "${tip_entry%% *}" "$state" "$CHECKOUT_DIR"; then - echo "::notice::Could not read the analysis on $ANALYSIS_BRANCH; not continuing from it." + echo "::notice::The analysis on $BASELINE_BRANCH was made with another engine version or settings; not continuing from it." + elif ! restore_baseline_branch "${REPOSITORY:-}" "${tip_entry%% *}" "$state" "$CHECKOUT_DIR"; then + echo "::notice::Could not read the analysis on $BASELINE_BRANCH; not continuing from it." fi fi if [ "$(depth_cap_from "$state/analysis.json")" = "$DEPTH_CAP" ]; then @@ -274,22 +282,22 @@ keep_user_config() { done } -# sync_strategy: branch keeps one commit per sync on ANALYSIS_BRANCH, each with +# save_baseline_to: baseline_branch keeps one commit per sync on BASELINE_BRANCH, each with # trailers naming the commit it analysed (CodeBoarding-Source) and the # configuration it ran under (CodeBoarding-Config). Lists them as # " ", newest first, at most -# ANALYSIS_BRANCH_DEPTH of them; an entry without a config has none to compare. +# BASELINE_BRANCH_DEPTH of them; an entry without a config has none to compare. # Fetched without blobs into a scratch repository: the lookup needs messages, and # a hundred pickles would cost more than it saves. -ANALYSIS_BRANCH_DEPTH="${ANALYSIS_BRANCH_DEPTH:-100}" -analysis_branch_index() { - local repository="$1" scratch="$RUNNER_TEMP/codeboarding-analysis-index.git" auth - [ -n "${ANALYSIS_BRANCH:-}" ] || return 0 +BASELINE_BRANCH_DEPTH="${BASELINE_BRANCH_DEPTH:-100}" +baseline_branch_index() { + local repository="$1" scratch="$RUNNER_TEMP/codeboarding-baseline-index.git" auth + [ -n "${BASELINE_BRANCH:-}" ] || return 0 rm -rf "$scratch" git init -q --bare "$scratch" auth="$(printf 'x-access-token:%s' "${GIT_TOKEN:-}" | base64 -w0)" git -C "$scratch" -c "http.extraheader=AUTHORIZATION: basic $auth" fetch -q --filter=blob:none \ - --depth="$ANALYSIS_BRANCH_DEPTH" "${GITHUB_SERVER_URL%/}/${repository}.git" "refs/heads/$ANALYSIS_BRANCH" 2>/dev/null || + --depth="$BASELINE_BRANCH_DEPTH" "${GITHUB_SERVER_URL%/}/${repository}.git" "refs/heads/$BASELINE_BRANCH" 2>/dev/null || return 0 git -C "$scratch" log --format='%H %(trailers:key=CodeBoarding-Source,valueonly,separator=%x20) %(trailers:key=CodeBoarding-Config,valueonly,separator=%x20)' FETCH_HEAD | awk 'NF >= 2 {print $1, $2, (NF >= 3 ? $3 : "-")}' @@ -300,10 +308,10 @@ analysis_branch_index() { usable_config() { [ -n "${CFG_HASH:-}" ] && [ "$1" = "$CFG_HASH" ] } -# Replaces the generated state in $3 with an analysis-branch commit's, keeping the +# Replaces the generated state in $3 with a baseline-branch commit's, keeping the # user configuration of checkout $4. source.json is provenance, not engine state. -restore_analysis_branch() { - local repository="$1" commit="$2" state="$3" config_from="$4" scratch="$RUNNER_TEMP/codeboarding-analysis-restore" +restore_baseline_branch() { + local repository="$1" commit="$2" state="$3" config_from="$4" scratch="$RUNNER_TEMP/codeboarding-baseline-restore" fetch_commit "$repository" "$commit" || return 1 rm -rf "$scratch" mkdir -p "$scratch" @@ -314,16 +322,16 @@ restore_analysis_branch() { cp -a "$scratch/.codeboarding" "$state" keep_user_config "$config_from" "$state" } -# Seeds $3 from the analysis branch's entry for $2, or for its nearest first-parent +# Seeds $3 from the baseline branch's entry for $2, or for its nearest first-parent # ancestor that has one under this configuration, keeping checkout $4's user # configuration. Sets BRANCH_SOURCE to the commit it describes and BRANCH_DISTANCE # to how far below $2 that is; BRANCH_REASON=incompatible when the only entries # found were made under another configuration. BRANCH_SOURCE="" BRANCH_DISTANCE="" BRANCH_REASON="" -seed_from_analysis_branch() { +seed_from_baseline_branch() { local repository="$1" tip="$2" state="$3" config_from="$4" index commit line entry="" distance=0 BRANCH_SOURCE="" BRANCH_DISTANCE="" BRANCH_REASON="" - index="$(analysis_branch_index "$repository")" + index="$(baseline_branch_index "$repository")" [ -n "$index" ] || return 1 fetch_commit "$repository" "$tip" "$(( CATCHUP_BOUND + 1 ))" || true for commit in $(git -C "$CHECKOUT_DIR" rev-list --first-parent --max-count=$(( CATCHUP_BOUND + 1 )) "$tip" 2>/dev/null); do @@ -337,7 +345,7 @@ seed_from_analysis_branch() { [ -z "$entry" ] || break distance=$(( distance + 1 )) done - [ -n "$entry" ] && restore_analysis_branch "$repository" "$entry" "$state" "$config_from" || return 1 + [ -n "$entry" ] && restore_baseline_branch "$repository" "$entry" "$state" "$config_from" || return 1 BRANCH_SOURCE="$commit" BRANCH_DISTANCE="$distance" BRANCH_REASON="" } @@ -394,8 +402,9 @@ analyze_review() { # A published base graph is this merge base's own analysis, named for it, so it # needs no engine run at all. Without one, the merge base is checked out and # analyzed from whatever baseline the repository committed there. Each path - # records how the base analysis was obtained, for the comment and the review - # artifact: reused, incremental or full. + # records how the base analysis was obtained, for the review artifact: reused, + # incremental or full. + ENGINE_ROLE=base 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 @@ -423,10 +432,10 @@ analyze_review() { elif [ -f "$base_state/analysis.json" ]; then full_cause=incompatible fi - # The analysis branch: its entry for the merge base is that commit's own + # The baseline branch: its entry for the merge base is that commit's own # analysis, and an entry for an ancestor is caught up like a committed one. if [ "$REQUIRES_FULL" = true ] && - seed_from_analysis_branch "$REVIEW_BASE_REPO" "$REVIEW_BASE_SHA" "$base_state" "$base_checkout"; then + seed_from_baseline_branch "$REVIEW_BASE_REPO" "$REVIEW_BASE_SHA" "$base_state" "$base_checkout"; then if [ "$(depth_cap_from "$base_state/analysis.json")" != "$DEPTH_CAP" ]; then full_cause=incompatible elif [ "$BRANCH_DISTANCE" -eq 0 ]; then @@ -464,8 +473,6 @@ analyze_review() { 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" @@ -487,13 +494,11 @@ analyze_review() { fi rm -f "$head_state/origin.json" - local head_started - head_started="$(date +%s)" + ENGINE_ROLE=head 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 @@ -511,9 +516,8 @@ analyze_review() { 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" + printf 'base_analysis_method=%s\nbase_analysis_reason=%s\n' \ + "$base_method" "$(base_reason "$base_method" "$full_cause" "$base_from_sha" "$catchup_commits")" >> "$GITHUB_OUTPUT" } # Why the base analysis was obtained the way it was, in words. Whichever way, the diff --git a/scripts/action/build-review-artifact.sh b/scripts/action/build-review-artifact.sh index 9db59e4..b8ce994 100755 --- a/scripts/action/build-review-artifact.sh +++ b/scripts/action/build-review-artifact.sh @@ -40,14 +40,11 @@ jq -n \ --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, - base_analysis_method: $base_analysis_method, base_analysis_reason: $base_analysis_reason, - base_seconds: $base_seconds, head_seconds: $head_seconds}' \ + base_analysis_method: $base_analysis_method, base_analysis_reason: $base_analysis_reason}' \ > "${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 4e092a1..96743a6 100755 --- a/scripts/action/build-review-comment.sh +++ b/scripts/action/build-review-comment.sh @@ -45,33 +45,10 @@ 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' - [ -z "$BASE_LINE" ] || printf '\n%s\n' "$BASE_LINE" - printf '\n' + printf '\n\n' if [ -n "$ARTIFACT_URL" ]; then printf '[download artifacts](%s) · ' "$ARTIFACT_URL" fi @@ -79,12 +56,7 @@ 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. - # 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" + printf '\n' \ + "$PLATFORM_URL" "$N_CHANGED" "$ANALYSED_FILES_CHANGED" "${HEAD_SHA:-}" } >> "$BODY" echo "path=$BODY" >> "$GITHUB_OUTPUT" diff --git a/scripts/action/deliver-sync.sh b/scripts/action/deliver-sync.sh index d503416..a359578 100755 --- a/scripts/action/deliver-sync.sh +++ b/scripts/action/deliver-sync.sh @@ -1,5 +1,5 @@ #!/usr/bin/env bash -# Installs generated state and delivers it by direct push or a rolling sync PR. +# Installs generated state and delivers it to the synced branch, a rolling sync PR, or the baseline branch. set -euo pipefail cd "$CHECKOUT_DIR" SYNC_BRANCH=codeboarding/sync @@ -28,12 +28,12 @@ emit_result() { "$1" "$2" "$3" "$BASE_SHA" >> "$GITHUB_OUTPUT" } close_stale_pr() { - [ "$SYNC_STRATEGY" = pull_request ] || return 0 - git fetch "$REMOTE" "$TARGET_BRANCH" + [ "$SAVE_BASELINE_TO" = pull_request ] || return 0 + git fetch "$REMOTE" "$SYNCED_BRANCH" [ "$(git rev-parse FETCH_HEAD)" = "$BASE_SHA" ] || return 0 local number sync_sha number="$(gh api --method GET "repos/$REPOSITORY/pulls" \ - -f state=open -f base="$TARGET_BRANCH" \ + -f state=open -f base="$SYNCED_BRANCH" \ -f head="${REPOSITORY%%/*}:$SYNC_BRANCH" --jq '.[0].number // empty')" if [ -n "$number" ]; then sync_sha="$(git ls-remote "$REMOTE" "refs/heads/$SYNC_BRANCH" | awk '{print $1; exit}')" @@ -63,11 +63,11 @@ classify_push_failure() { exit 1 } -# sync_strategy: branch keeps the analysis on an orphan branch of its own, one -# fast-forward commit per sync, and never writes to the target branch. -deliver_to_analysis_branch() { - local branch="$ANALYSIS_BRANCH" tree="$RUNNER_TEMP/codeboarding-analysis-tree" - local index="$RUNNER_TEMP/codeboarding-analysis-index" git_dir files new_tree tip parent commit now +# save_baseline_to: baseline_branch keeps the analysis on an orphan branch of its +# own, one fast-forward commit per sync, and never writes to the synced branch. +deliver_to_baseline_branch() { + local branch="$BASELINE_BRANCH" tree="$RUNNER_TEMP/codeboarding-baseline-tree" + local index="$RUNNER_TEMP/codeboarding-baseline-index" git_dir files new_tree tip parent commit now git_dir="$(git rev-parse --absolute-git-dir)" rm -rf "$tree" "$index" mkdir -p "$tree" @@ -78,7 +78,7 @@ deliver_to_analysis_branch() { python3 -c 'import datetime,json,os,sys json.dump({ "schema": 1, - "source_branch": os.environ["TARGET_BRANCH"], + "synced_branch": os.environ["SYNCED_BRANCH"], "source_sha": sys.argv[2], "generated_at": datetime.datetime.now(datetime.timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ"), "engine_version": os.environ.get("ENGINE_VERSION", ""), @@ -95,10 +95,10 @@ CodeBoarding-Config: $CFG_HASH") # Two tries: a concurrent sync that moved the branch for an older commit is # built on top of once. A second move means a newer run is handling it. for _ in 1 2; do - git fetch -q "$REMOTE" "$TARGET_BRANCH" + git fetch -q "$REMOTE" "$SYNCED_BRANCH" if [ "$(git rev-parse FETCH_HEAD)" != "$BASE_SHA" ]; then emit_result "$files" false "$BASE_SHA" - echo "::notice::$TARGET_BRANCH advanced during analysis; a newer run should update $branch." + echo "::notice::$SYNCED_BRANCH advanced during analysis; a newer run should update $branch." exit 0 fi tip="" parent=() @@ -109,28 +109,28 @@ CodeBoarding-Config: $CFG_HASH") # Building on any other branch would leave it holding nothing but .codeboarding/. if [ -z "$(git log -1 --format='%(trailers:key=CodeBoarding-Source,valueonly)' "$tip" | tr -d '[:space:]')" ] || [ "$(git ls-tree --name-only "$tip")" != .codeboarding ]; then - echo "::error::$branch already exists and is not a CodeBoarding analysis branch, so sync will not write to it. Set analysis_branch to a branch name that is not in use." + echo "::error::$branch already exists and is not a CodeBoarding baseline branch, so sync will not write to it. Set baseline_branch to a branch name that is not in use." exit 1 fi parent=(-p "$tip") if [ "$(git log -1 --format='%(trailers:key=CodeBoarding-Source,valueonly)' "$tip" | tr -d '[:space:]')" = "$BASE_SHA" ] && git diff --quiet -I '"generated_at"' -I '"timestamp"' "$tip" "$new_tree"; then emit_result "$files" false "$BASE_SHA" - echo "::notice::$branch already holds this analysis of $TARGET_BRANCH @${BASE_SHA:0:7}." + echo "::notice::$branch already holds this analysis of $SYNCED_BRANCH @${BASE_SHA:0:7}." exit 0 fi fi commit="$(git commit-tree "$new_tree" ${parent[@]+"${parent[@]}"} \ - -m "chore(codeboarding): diagram of $TARGET_BRANCH @${BASE_SHA:0:7}" "${trailers[@]}")" + -m "chore(codeboarding): diagram of $SYNCED_BRANCH @${BASE_SHA:0:7}" "${trailers[@]}")" # Never forced: the parent is the tip just read, so this only ever fast-forwards. if git push -q "$REMOTE" "$commit:refs/heads/$branch"; then emit_result "$files" true "$BASE_SHA" - echo "analysis_branch_sha=$commit" >> "$GITHUB_OUTPUT" + echo "baseline_branch_sha=$commit" >> "$GITHUB_OUTPUT" exit 0 fi now="$(git ls-remote "$REMOTE" "refs/heads/$branch" | awk '{print $1; exit}')" if [ "$now" = "$tip" ]; then - echo "::error::GitHub refused the push to $branch, most likely because a branch rule protects it. Add the identity sync pushes with (the CodeBoarding app, or GitHub Actions for the default token) as a bypass actor for $branch in the repository's rulesets, or set sync_strategy: push." + echo "::error::GitHub refused the push to $branch, most likely because a branch rule protects it. Add the identity sync pushes with (the CodeBoarding app, or GitHub Actions for the default token) as a bypass actor for $branch in the repository's rulesets, or set save_baseline_to: synced_branch." exit 1 fi done @@ -138,7 +138,7 @@ CodeBoarding-Config: $CFG_HASH") echo "::notice::Another sync keeps updating $branch; leaving it to that run." exit 0 } -[ "$SYNC_STRATEGY" != branch ] || deliver_to_analysis_branch +[ "$SAVE_BASELINE_TO" != baseline_branch ] || deliver_to_baseline_branch "$ACTION_PATH/scripts/action/install-sync.sh" > "$GENERATED_PATHS" stage_paths=() @@ -155,11 +155,11 @@ files_written="$(find "$CHECKOUT_DIR/.codeboarding" -maxdepth 1 -type f \ \( -name analysis.json -o -name fingerprint.json -o -name static_analysis.pkl -o -name static_analysis.sha \ -o -name codeboarding_version.json \) | wc -l)" -git fetch "$REMOTE" "$TARGET_BRANCH" +git fetch "$REMOTE" "$SYNCED_BRANCH" remote_sha="$(git rev-parse FETCH_HEAD)" if [ "$remote_sha" != "$BASE_SHA" ]; then emit_result "$files_written" false "$BASE_SHA" - echo "::notice::$TARGET_BRANCH advanced during analysis; a newer run should update its baseline." + echo "::notice::$SYNCED_BRANCH advanced during analysis; a newer run should update its baseline." exit 0 fi @@ -176,9 +176,9 @@ fi git commit -m 'chore(codeboarding): sync analysis baseline' >/dev/null -if [ "$SYNC_STRATEGY" = push ]; then - if ! git push "$REMOTE" "HEAD:refs/heads/$TARGET_BRANCH"; then - classify_push_failure "$BASE_SHA" "$TARGET_BRANCH" +if [ "$SAVE_BASELINE_TO" = synced_branch ]; then + if ! git push "$REMOTE" "HEAD:refs/heads/$SYNCED_BRANCH"; then + classify_push_failure "$BASE_SHA" "$SYNCED_BRANCH" fi emit_result "$files_written" true "$(git rev-parse HEAD)" exit 0 @@ -186,8 +186,8 @@ fi pr_json="$(gh api --method GET "repos/$REPOSITORY/pulls" \ -f state=open -f head="${REPOSITORY%%/*}:$SYNC_BRANCH" --jq '.[0] // empty')" -if [ -n "$pr_json" ] && [ "$(jq -r .base.ref <<< "$pr_json")" != "$TARGET_BRANCH" ]; then - echo "::error::$SYNC_BRANCH already has an open PR into $(jq -r .base.ref <<< "$pr_json"); close it before changing target_branch." +if [ -n "$pr_json" ] && [ "$(jq -r .base.ref <<< "$pr_json")" != "$SYNCED_BRANCH" ]; then + echo "::error::$SYNC_BRANCH already has an open PR into $(jq -r .base.ref <<< "$pr_json"); close it before changing synced_branch." exit 1 fi @@ -198,11 +198,11 @@ if ! git push "--force-with-lease=refs/heads/$SYNC_BRANCH:$old_sync_sha" \ fi if [ -z "$pr_json" ]; then - gh pr create --repo "$REPOSITORY" --base "$TARGET_BRANCH" --head "$SYNC_BRANCH" \ + gh pr create --repo "$REPOSITORY" --base "$SYNCED_BRANCH" --head "$SYNC_BRANCH" \ --title 'chore(codeboarding): sync analysis baseline' \ - --body "Updates the versioned CodeBoarding analysis for \`$TARGET_BRANCH\`." + --body "Updates the versioned CodeBoarding analysis for \`$SYNCED_BRANCH\`." pr_json="$(gh api --method GET "repos/$REPOSITORY/pulls" \ - -f state=open -f base="$TARGET_BRANCH" \ + -f state=open -f base="$SYNCED_BRANCH" \ -f head="${REPOSITORY%%/*}:$SYNC_BRANCH" --jq '.[0]')" fi diff --git a/scripts/action/fetch-state.sh b/scripts/action/fetch-state.sh index e90dacd..cf16a1e 100755 --- a/scripts/action/fetch-state.sh +++ b/scripts/action/fetch-state.sh @@ -4,10 +4,6 @@ # 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/guard.sh b/scripts/action/guard.sh index b469f0e..8e1bf6c 100755 --- a/scripts/action/guard.sh +++ b/scripts/action/guard.sh @@ -9,10 +9,10 @@ case "$MODE" in *) fail "mode must be review or sync." ;; esac printf 'mode=%s\nskip=false\nevent=%s\n' "$MODE" "$EVENT" >> "$GITHUB_OUTPUT" -# Both modes read the analysis branch, so a name git cannot use fails here, before +# Both modes read the baseline branch, so a name git cannot use fails here, before # anything is analyzed, rather than reading as a missing branch and costing a full run. -if [ -n "${ANALYSIS_BRANCH:-}" ] && ! git check-ref-format "refs/heads/$ANALYSIS_BRANCH"; then - fail "analysis_branch '$ANALYSIS_BRANCH' is not a valid branch name." +if [ -n "${BASELINE_BRANCH:-}" ] && ! git check-ref-format "refs/heads/$BASELINE_BRANCH"; then + fail "baseline_branch '$BASELINE_BRANCH' is not a valid branch name." fi if [ "$MODE" = sync ]; then case "$EVENT" in @@ -20,33 +20,33 @@ if [ "$MODE" = sync ]; then *) skip "Sync mode ignores $EVENT events." ;; esac [ "$REF_TYPE" != tag ] || skip "Sync mode ignores tag pushes." - case "$SYNC_STRATEGY" in - push|pull_request|branch) ;; - *) fail "sync_strategy must be push, pull_request or branch." ;; + case "$SAVE_BASELINE_TO" in + synced_branch|pull_request|baseline_branch) ;; + *) fail "save_baseline_to must be synced_branch, pull_request or baseline_branch." ;; esac case "$HEAD_AUTHOR_EMAIL" in codeboarding-review\[bot\]@users.noreply.github.com|codeboarding\[bot\]@users.noreply.github.com) [ "$EVENT" != push ] || skip "Ignoring CodeBoarding's own baseline commit." ;; esac - target_branch="${TARGET_BRANCH_INPUT:-$REF_NAME}" - [ -n "$target_branch" ] || fail "target_branch is required for this event." - [ "$SYNC_STRATEGY" != pull_request ] || [ "$target_branch" != codeboarding/sync ] || fail "target_branch must differ from codeboarding/sync." - if [ "$SYNC_STRATEGY" = branch ]; then - [ -n "${ANALYSIS_BRANCH:-}" ] || fail "analysis_branch is required with sync_strategy: branch." - # The analysis branch holds only analysis; a workflow that also fires on it must not analyze it. - [ "$REF_NAME" != "$ANALYSIS_BRANCH" ] || skip "Ignoring a push to the analysis branch $ANALYSIS_BRANCH." - [ "$target_branch" != "$ANALYSIS_BRANCH" ] || fail "target_branch must differ from analysis_branch." + synced_branch="${SYNCED_BRANCH_INPUT:-$REF_NAME}" + [ -n "$synced_branch" ] || fail "synced_branch is required for this event." + [ "$SAVE_BASELINE_TO" != pull_request ] || [ "$synced_branch" != codeboarding/sync ] || fail "synced_branch must differ from codeboarding/sync." + if [ "$SAVE_BASELINE_TO" = baseline_branch ]; then + [ -n "${BASELINE_BRANCH:-}" ] || fail "baseline_branch is required with save_baseline_to: baseline_branch." + # The baseline branch holds only analysis; a workflow that also fires on it must not analyze it. + [ "$REF_NAME" != "$BASELINE_BRANCH" ] || skip "Ignoring a push to the baseline branch $BASELINE_BRANCH." + [ "$synced_branch" != "$BASELINE_BRANCH" ] || fail "synced_branch must differ from baseline_branch." fi sync_branch_start_sha="" - if [ "$SYNC_STRATEGY" = pull_request ]; then + if [ "$SAVE_BASELINE_TO" = pull_request ]; then sync_branch_start_sha="$(gh api "repos/$REPOSITORY/branches/codeboarding%2Fsync" --jq '.commit.sha' 2>/dev/null || true)" fi { - echo "target_branch=$target_branch" + echo "synced_branch=$synced_branch" echo "sync_branch_start_sha=$sync_branch_start_sha" echo "checkout_repo=$REPOSITORY" - echo "checkout_ref=$target_branch" + echo "checkout_ref=$synced_branch" } >> "$GITHUB_OUTPUT" exit 0 fi diff --git a/scripts/action/old_api_migrator.sh b/scripts/action/old_api_migrator.sh new file mode 100755 index 0000000..5d03cff --- /dev/null +++ b/scripts/action/old_api_migrator.sh @@ -0,0 +1,32 @@ +#!/usr/bin/env bash +# Translates deprecated inputs into the current ones, and is the only place that +# knows they exist. Every later step reads this step's outputs, never the inputs +# themselves, so no other script carries backward-compatibility logic. +# +# target_branch -> synced_branch +# sync_strategy -> save_baseline_to (push -> synced_branch, pull_request -> pull_request) +# +# Delete a mapping, and its input in action.yml, once no supported workflow sets it. +set -euo pipefail +fail() { echo "::error::$1"; exit 1; } + +synced_branch="${SYNCED_BRANCH:-}" +if [ -n "${OLD_TARGET_BRANCH:-}" ]; then + [ -z "$synced_branch" ] || fail "Set synced_branch only; target_branch is its deprecated name." + echo "::warning::target_branch is deprecated; rename it to synced_branch." + synced_branch="$OLD_TARGET_BRANCH" +fi + +save_baseline_to="${SAVE_BASELINE_TO:-}" +if [ -n "${OLD_SYNC_STRATEGY:-}" ]; then + # save_baseline_to has a default, so only a value other than it is a conflict. + [ "$save_baseline_to" = synced_branch ] || fail "Set save_baseline_to only; sync_strategy is its deprecated name." + case "$OLD_SYNC_STRATEGY" in + push) save_baseline_to=synced_branch ;; + pull_request) save_baseline_to=pull_request ;; + *) fail "sync_strategy is deprecated and only accepts push or pull_request; use save_baseline_to." ;; + esac + echo "::warning::sync_strategy is deprecated; use save_baseline_to: $save_baseline_to." +fi + +printf 'synced_branch=%s\nsave_baseline_to=%s\n' "$synced_branch" "$save_baseline_to" >> "$GITHUB_OUTPUT" diff --git a/scripts/action/sync-summary.sh b/scripts/action/sync-summary.sh index 287b9af..7233825 100755 --- a/scripts/action/sync-summary.sh +++ b/scripts/action/sync-summary.sh @@ -6,7 +6,7 @@ set -euo pipefail echo "- Analysis: ${MODE}" echo "- Analysis artifacts: ${FILES:-0}" echo "- Delivered: ${COMMITTED:-false}" - echo "- Strategy: ${STRATEGY}" + echo "- Saved to: ${SAVE_BASELINE_TO}" if [ -n "${PR_URL:-}" ]; then echo "- Sync PR: ${PR_URL}" fi diff --git a/tests/test_action_inputs.py b/tests/test_action_inputs.py index 4757bc7..233987e 100644 --- a/tests/test_action_inputs.py +++ b/tests/test_action_inputs.py @@ -86,6 +86,14 @@ def test_the_inferred_credential_inputs_are_gone(self) -> None: self.assertNotIn(stale, self.inputs) self.assertNotIn(f"inputs.{stale}", ACTION) + def test_deprecated_inputs_are_read_only_by_the_migrator(self) -> None: + """Backward compatibility lives in one script; every other step reads its outputs.""" + for old, new in (("target_branch", "synced_branch"), ("sync_strategy", "save_baseline_to")): + self.assertIn("deprecationMessage:", self.inputs[old]) + self.assertEqual(ACTION.count(f"inputs.{old} }}}}"), 1, f"only the migrator may read {old}") + self.assertEqual(ACTION.count(f"inputs.{new} }}}}"), 1, f"only the migrator may read {new}") + self.assertLess(ACTION.index("- name: Translate deprecated inputs"), ACTION.index("- name: Resolve event")) + def test_credentials_resolve_before_the_checkout_and_the_engine_install(self) -> None: """Fail-fast is positional: preflight is worth little after a minute of setup.""" preflight = ACTION.index("- name: Check LLM configuration") diff --git a/tests/test_action_state.py b/tests/test_action_state.py index 70d07ed..5275992 100644 --- a/tests/test_action_state.py +++ b/tests/test_action_state.py @@ -26,6 +26,7 @@ "mode": argv[0], "checkout": argv[argv.index("--local") + 1], "depth": argv[argv.index("--depth-cap") + 1] if "--depth-cap" in argv else None, + "run_id": os.environ.get("CODEBOARDING_RUN_ID", ""), }) + "\\n") analysis = os.path.join(output, "analysis.json") metadata = json.load(open(analysis))["metadata"] if os.path.isfile(analysis) else {} @@ -286,6 +287,16 @@ def test_compatible_committed_baseline_runs_incrementally(self) -> None: self._analyze(REVIEW_BASE_SHA=sha, DEPTH_CAP="4") self.assertEqual([c["mode"] for c in self._engine_calls()], ["incremental", "incremental"]) + def test_engine_runs_are_tagged_with_the_run_and_the_analysis_they_belong_to(self) -> None: + sha = self._commit_base(cap=4) + self._analyze(REVIEW_BASE_SHA=sha, DEPTH_CAP="4", GITHUB_RUN_ID="991", GITHUB_RUN_ATTEMPT="2") + self.assertEqual([c["run_id"] for c in self._engine_calls()], ["gh-991-2-base", "gh-991-2-head"]) + + def test_a_reused_base_leaves_only_head_engine_runs(self) -> None: + _state(self.base_dir) + self._analyze(GITHUB_RUN_ID="991", GITHUB_RUN_ATTEMPT="1") + self.assertEqual([c["run_id"] for c in self._engine_calls()], ["gh-991-1-head"]) + def test_legacy_committed_depth_is_not_inherited(self) -> None: sha = self._commit_base(legacy=True) self._analyze(REVIEW_BASE_SHA=sha, DEPTH_CAP="4") @@ -360,22 +371,17 @@ def _merge(self, branch: str, files: dict[str, str], *, bot: bool = False) -> st return self._git("rev-parse", "HEAD") def _provenance(self, values: dict[str, str]) -> dict[str, str]: - keys = ("base_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} + return {key: values[key] for key in ("base_analysis_method", "base_analysis_reason")} def test_a_saved_base_is_reused(self) -> None: _state(self.base_dir) - values = self._analyze(BASE_FETCH_SECONDS="7") + values = self._analyze() 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() @@ -456,7 +462,7 @@ def test_merged_pull_requests_count_as_commits_to_catch_up(self) -> None: ) 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 + # save_baseline_to: 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"}) diff --git a/tests/test_action_sync.py b/tests/test_action_sync.py index b0b87aa..e2e0cb1 100644 --- a/tests/test_action_sync.py +++ b/tests/test_action_sync.py @@ -392,8 +392,8 @@ def test_it_publishes_the_analysis_under_both_commits(self) -> None: "GITHUB_TOKEN": "unused", "GH_HOST": "github.com", "REPOSITORY": "owner/repo", - "TARGET_BRANCH": "main", - "SYNC_STRATEGY": "push", + "SYNCED_BRANCH": "main", + "SAVE_BASELINE_TO": "synced_branch", }, capture_output=True, text=True, diff --git a/tests/test_analysis_branch.py b/tests/test_baseline_branch.py similarity index 88% rename from tests/test_analysis_branch.py rename to tests/test_baseline_branch.py index 92a912f..96c8aee 100644 --- a/tests/test_analysis_branch.py +++ b/tests/test_baseline_branch.py @@ -1,4 +1,4 @@ -"""sync_strategy: branch saves the analysis to an orphan branch, and reviews read their base from it.""" +"""save_baseline_to: baseline_branch saves the analysis to an orphan branch, and reviews read their base from it.""" from __future__ import annotations @@ -13,7 +13,7 @@ ROOT = Path(__file__).resolve().parent.parent DELIVER = ROOT / "scripts" / "action" / "deliver-sync.sh" -BRANCH = "codeboarding/analysis" +BRANCH = "codeboarding/baseline" def git(cwd: Path, *args: str) -> str: @@ -26,7 +26,7 @@ def git(cwd: Path, *args: str) -> str: ).stdout.strip() -class AnalysisBranchDeliveryTests(unittest.TestCase): +class BaselineBranchDeliveryTests(unittest.TestCase): """deliver-sync.sh against a real local remote.""" def setUp(self) -> None: @@ -84,9 +84,9 @@ def _deliver(self, expect_ok: bool = True) -> tuple[subprocess.CompletedProcess, "GITHUB_TOKEN": "unused", "GH_HOST": "github.com", "REPOSITORY": "owner/repo", - "TARGET_BRANCH": "main", - "SYNC_STRATEGY": "branch", - "ANALYSIS_BRANCH": BRANCH, + "SYNCED_BRANCH": "main", + "SAVE_BASELINE_TO": "baseline_branch", + "BASELINE_BRANCH": BRANCH, "ENGINE_VERSION": "0.14.5", "CFG_HASH": "cfg", }, @@ -108,7 +108,7 @@ def test_the_first_sync_creates_an_orphan_branch_with_its_provenance(self) -> No _result, values = self._deliver() (only,) = self._branch_log() - self.assertEqual(only.split(), [values["analysis_branch_sha"]], "the branch must have no parent") + self.assertEqual(only.split(), [values["baseline_branch_sha"]], "the branch must have no parent") self.assertEqual(values["committed"], "true") self.assertEqual(values["baseline_sha"], main_before, "artifacts are named for the analysed commit") files = set(git(self.remote, "ls-tree", "-r", "--name-only", BRANCH).splitlines()) @@ -123,7 +123,7 @@ def test_the_first_sync_creates_an_orphan_branch_with_its_provenance(self) -> No ) source = json.loads(git(self.remote, "show", f"{BRANCH}:.codeboarding/source.json")) self.assertEqual(source["schema"], 1) - self.assertEqual(source["source_branch"], "main") + self.assertEqual(source["synced_branch"], "main") self.assertEqual(source["source_sha"], main_before) self.assertEqual(source["engine_version"], "0.14.5") self.assertEqual(source["config"], "cfg") @@ -134,7 +134,7 @@ def test_the_first_sync_creates_an_orphan_branch_with_its_provenance(self) -> No f"chore(codeboarding): diagram of main @{main_before[:7]}\n\n" f"CodeBoarding-Source: {main_before}\nCodeBoarding-Config: cfg", ) - # The default branch is never written in this strategy. + # The synced branch is never written when the baseline has a branch of its own. self.assertEqual(git(self.remote, "rev-parse", "main"), main_before) def test_the_next_sync_appends_a_fast_forward_commit(self) -> None: @@ -147,7 +147,7 @@ def test_the_next_sync_appends_a_fast_forward_commit(self) -> None: log = self._branch_log() self.assertEqual(len(log), 2) - self.assertEqual(log[0].split(), [values["analysis_branch_sha"], first]) + self.assertEqual(log[0].split(), [values["baseline_branch_sha"], first]) self.assertIn(f"CodeBoarding-Source: {new_main}", git(self.remote, "log", "-1", "--format=%B", BRANCH)) def test_a_rerun_on_the_same_commit_adds_nothing(self) -> None: @@ -173,7 +173,7 @@ def test_a_push_refused_by_a_branch_rule_says_how_to_fix_it(self) -> None: self.assertNotEqual(result.returncode, 0) self.assertIn(f"GitHub refused the push to {BRANCH}", result.stdout) self.assertIn("bypass actor", result.stdout) - self.assertIn("sync_strategy: push", result.stdout) + self.assertIn("save_baseline_to: synced_branch", result.stdout) def test_a_deleted_branch_is_recreated_as_a_new_orphan(self) -> None: self._deliver() @@ -185,8 +185,8 @@ def test_a_deleted_branch_is_recreated_as_a_new_orphan(self) -> None: (only,) = self._branch_log() self.assertEqual(len(only.split()), 1, "a recreated branch starts a new history") - def test_an_existing_branch_that_is_not_an_analysis_branch_is_never_written(self) -> None: - # analysis_branch pointed at a code branch: building on it would leave it + def test_an_existing_branch_that_is_not_a_baseline_branch_is_never_written(self) -> None: + # baseline_branch pointed at a code branch: building on it would leave it # holding nothing but .codeboarding/. git(self.checkout, "push", "-q", "origin", f"main:refs/heads/{BRANCH}") before = git(self.remote, "rev-parse", BRANCH) @@ -194,11 +194,11 @@ def test_an_existing_branch_that_is_not_an_analysis_branch_is_never_written(self result, values = self._deliver(expect_ok=False) self.assertNotEqual(result.returncode, 0) - self.assertIn(f"{BRANCH} already exists and is not a CodeBoarding analysis branch", result.stdout) + self.assertIn(f"{BRANCH} already exists and is not a CodeBoarding baseline branch", result.stdout) self.assertEqual(git(self.remote, "rev-parse", BRANCH), before) - self.assertNotIn("analysis_branch_sha", values) + self.assertNotIn("baseline_branch_sha", values) - def test_a_target_that_moved_during_analysis_keeps_the_branch_unchanged(self) -> None: + def test_a_synced_branch_that_moved_during_analysis_keeps_the_branch_unchanged(self) -> None: self._deliver() before = git(self.remote, "rev-parse", BRANCH) other = self.root / "other" @@ -215,7 +215,7 @@ def test_a_target_that_moved_during_analysis_keeps_the_branch_unchanged(self) -> self.assertEqual(git(self.remote, "rev-parse", BRANCH), before) -class AnalysisBranchReadTests(unittest.TestCase): +class BaselineBranchReadTests(unittest.TestCase): """Reviews and sync read the branch: commits c0..c4 on main, branch entries for c1 and c3 made under configuration `cfg`.""" @@ -293,7 +293,7 @@ def _analyze(self, **extra: str) -> dict[str, str]: "PR_NUMBER": "42", "ENGINE_VERSION": "0.14.5", "CFG_HASH": "cfg", - "ANALYSIS_BRANCH": BRANCH, + "BASELINE_BRANCH": BRANCH, "BASE_DIR": str(self.root / "state" / "base"), "WARMSTART_DIR": str(self.root / "state" / "warmstart"), "STAGE_DIR": str(self.root / "state" / "out"), @@ -350,14 +350,14 @@ def test_without_a_configuration_hash_no_entry_is_trusted(self) -> None: self.assertEqual(values["base_analysis_method"], "full") def test_without_the_branch_the_base_is_a_full_analysis(self) -> None: - values = self._analyze(REVIEW_BASE_SHA=self.shas[4], ANALYSIS_BRANCH="codeboarding/none") + values = self._analyze(REVIEW_BASE_SHA=self.shas[4], BASELINE_BRANCH="codeboarding/none") self.assertEqual(values["base_analysis_method"], "full") self.assertEqual(values["base_analysis_reason"], "no usable analysis was available") self.assertEqual(self._modes(), ["full", "incremental"]) def test_sync_continues_from_the_branch_tip(self) -> None: - self._analyze(ANALYSIS_KIND="sync", SYNC_STRATEGY="branch", FORCE_FULL="false") + self._analyze(ANALYSIS_KIND="sync", SAVE_BASELINE_TO="baseline_branch", FORCE_FULL="false") self.assertEqual(self._modes(), ["incremental"]) @@ -371,7 +371,7 @@ def test_sync_replaces_the_committed_state_with_the_branch_tip(self) -> None: (board / "static_analysis.sha").write_text("stale\n") (board / ".codeboardingignore").write_text("docs/\n") - values = self._analyze(ANALYSIS_KIND="sync", SYNC_STRATEGY="branch", FORCE_FULL="false") + values = self._analyze(ANALYSIS_KIND="sync", SAVE_BASELINE_TO="baseline_branch", FORCE_FULL="false") state = Path(values["analysis_dir"]) self.assertEqual((state / "static_analysis.pkl").read_text(), "pickle", "the tip's engine state") @@ -380,19 +380,22 @@ def test_sync_replaces_the_committed_state_with_the_branch_tip(self) -> None: self.assertEqual((state / ".codeboardingignore").read_text(), "docs/\n") def test_sync_does_not_continue_from_a_tip_made_under_another_configuration(self) -> None: - self._analyze(ANALYSIS_KIND="sync", SYNC_STRATEGY="branch", FORCE_FULL="false", CFG_HASH="othercfg") + self._analyze(ANALYSIS_KIND="sync", SAVE_BASELINE_TO="baseline_branch", FORCE_FULL="false", CFG_HASH="othercfg") self.assertEqual(self._modes(), ["full"]) def test_sync_without_the_branch_analyzes_from_scratch(self) -> None: self._analyze( - ANALYSIS_KIND="sync", SYNC_STRATEGY="branch", FORCE_FULL="false", ANALYSIS_BRANCH="codeboarding/none" + ANALYSIS_KIND="sync", + SAVE_BASELINE_TO="baseline_branch", + FORCE_FULL="false", + BASELINE_BRANCH="codeboarding/none", ) self.assertEqual(self._modes(), ["full"]) -class AnalysisBranchGuardTests(unittest.TestCase): +class BaselineBranchGuardTests(unittest.TestCase): def _guard(self, **extra: str) -> tuple[subprocess.CompletedProcess, str]: with tempfile.TemporaryDirectory() as tmp: output = Path(tmp) / "github-output" @@ -407,8 +410,8 @@ def _guard(self, **extra: str) -> tuple[subprocess.CompletedProcess, str]: "REF_NAME": "main", "REF_TYPE": "branch", "HEAD_AUTHOR_EMAIL": "dev@example.com", - "SYNC_STRATEGY": "branch", - "ANALYSIS_BRANCH": BRANCH, + "SAVE_BASELINE_TO": "baseline_branch", + "BASELINE_BRANCH": BRANCH, "REPOSITORY": "owner/repo", **extra, }, @@ -418,12 +421,12 @@ def _guard(self, **extra: str) -> tuple[subprocess.CompletedProcess, str]: ) return result, output.read_text(encoding="utf-8") - def test_the_branch_strategy_is_accepted(self) -> None: + def test_the_baseline_branch_destination_is_accepted(self) -> None: result, values = self._guard() self.assertEqual(result.returncode, 0, result.stdout) - self.assertIn("target_branch=main", values) + self.assertIn("synced_branch=main", values) - def test_a_push_to_the_analysis_branch_itself_is_ignored(self) -> None: + def test_a_push_to_the_baseline_branch_itself_is_ignored(self) -> None: result, values = self._guard(REF_NAME=BRANCH) self.assertEqual(result.returncode, 0, result.stdout) self.assertIn("skip=true", values) @@ -431,14 +434,14 @@ def test_a_push_to_the_analysis_branch_itself_is_ignored(self) -> None: def test_a_branch_name_git_cannot_use_fails_before_anything_runs(self) -> None: for mode in ("sync", "review"): for name in ("codeboarding/a..b", "codeboarding/with space", "codeboarding/trailing."): - result, _values = self._guard(MODE=mode, ANALYSIS_BRANCH=name) + result, _values = self._guard(MODE=mode, BASELINE_BRANCH=name) self.assertNotEqual(result.returncode, 0, (mode, name)) self.assertIn("is not a valid branch name", result.stdout) - def test_target_branch_must_differ_from_the_analysis_branch(self) -> None: - result, _values = self._guard(TARGET_BRANCH_INPUT=BRANCH) + def test_synced_branch_must_differ_from_the_baseline_branch(self) -> None: + result, _values = self._guard(SYNCED_BRANCH_INPUT=BRANCH) self.assertNotEqual(result.returncode, 0) - self.assertIn("target_branch must differ from analysis_branch", result.stdout) + self.assertIn("synced_branch must differ from baseline_branch", result.stdout) if __name__ == "__main__": diff --git a/tests/test_old_api_migrator.py b/tests/test_old_api_migrator.py new file mode 100644 index 0000000..5d7e390 --- /dev/null +++ b/tests/test_old_api_migrator.py @@ -0,0 +1,77 @@ +"""Deprecated inputs keep working, translated in one place, and never silently lose to the new ones.""" + +from __future__ import annotations + +import os +import subprocess +import tempfile +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parent.parent +MIGRATOR = ROOT / "scripts" / "action" / "old_api_migrator.sh" + + +def migrate(**inputs: str) -> tuple[subprocess.CompletedProcess, dict[str, str]]: + """Runs the migrator with action.yml's defaults, overridden by `inputs`.""" + with tempfile.TemporaryDirectory() as tmp: + output = Path(tmp) / "github-output" + output.write_text("", encoding="utf-8") + result = subprocess.run( + [str(MIGRATOR)], + env={ + "PATH": os.environ["PATH"], + "GITHUB_OUTPUT": str(output), + "SYNCED_BRANCH": "", + "SAVE_BASELINE_TO": "synced_branch", + "OLD_TARGET_BRANCH": "", + "OLD_SYNC_STRATEGY": "", + **inputs, + }, + capture_output=True, + text=True, + check=False, + ) + values = dict(line.split("=", 1) for line in output.read_text(encoding="utf-8").splitlines()) + return result, values + + +class OldApiMigratorTests(unittest.TestCase): + def test_current_inputs_pass_through_without_warnings(self) -> None: + result, values = migrate(SYNCED_BRANCH="main", SAVE_BASELINE_TO="baseline_branch") + self.assertEqual(result.returncode, 0, result.stdout) + self.assertEqual(values, {"synced_branch": "main", "save_baseline_to": "baseline_branch"}) + self.assertNotIn("::warning::", result.stdout) + + def test_target_branch_becomes_synced_branch(self) -> None: + result, values = migrate(OLD_TARGET_BRANCH="develop") + self.assertEqual(result.returncode, 0, result.stdout) + self.assertEqual(values["synced_branch"], "develop") + self.assertIn("::warning::target_branch is deprecated", result.stdout) + + def test_sync_strategy_values_map_onto_save_baseline_to(self) -> None: + for old, new in (("push", "synced_branch"), ("pull_request", "pull_request")): + with self.subTest(old=old): + result, values = migrate(OLD_SYNC_STRATEGY=old) + self.assertEqual(result.returncode, 0, result.stdout) + self.assertEqual(values["save_baseline_to"], new) + self.assertIn("::warning::sync_strategy is deprecated", result.stdout) + + def test_a_value_sync_strategy_never_accepted_fails(self) -> None: + result, _values = migrate(OLD_SYNC_STRATEGY="branch") + self.assertNotEqual(result.returncode, 0) + self.assertIn("use save_baseline_to", result.stdout) + + def test_setting_an_input_under_both_names_fails(self) -> None: + for inputs in ( + {"SYNCED_BRANCH": "main", "OLD_TARGET_BRANCH": "main"}, + {"SAVE_BASELINE_TO": "baseline_branch", "OLD_SYNC_STRATEGY": "push"}, + ): + with self.subTest(inputs=inputs): + result, _values = migrate(**inputs) + self.assertNotEqual(result.returncode, 0, result.stdout) + self.assertIn("deprecated name", result.stdout) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_review_artifact_metadata.py b/tests/test_review_artifact_metadata.py index 7a79060..b630201 100644 --- a/tests/test_review_artifact_metadata.py +++ b/tests/test_review_artifact_metadata.py @@ -71,26 +71,22 @@ def test_how_the_base_was_obtained_is_recorded_as_strings(self) -> None: 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")}, + {key: metadata[key] for key in metadata if key.startswith("base_")}, { "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"): + for key in ("base_analysis_method", "base_analysis_reason"): self.assertEqual(metadata[key], "", key) diff --git a/tests/test_review_comment.py b/tests/test_review_comment.py index ca77256..709f693 100644 --- a/tests/test_review_comment.py +++ b/tests/test_review_comment.py @@ -82,78 +82,5 @@ 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()