From 5fd95a27be7dc194d0716b390b1d9cfb5401c661 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Mon, 17 Aug 2026 18:40:20 +0200 Subject: [PATCH 01/12] feat(review): compare against the merge base and reuse the PR's last analysis Reviews compared the head with the event's base branch tip, which moves whenever anyone else pushes. Commits landing on the base after a branch forked were reported as that pull request's changes, and reported backwards, as removals. Reviews now resolve the merge base, the same anchor GitHub's own "Files changed" tab uses, and the comment says how far behind the branch is instead. Each review also seeds its head analysis from this pull request's own previous run, so a run covers only the commits pushed since it, and takes the merge base's analysis from cache rather than recomputing it. Sync mode publishes that base entry as a by-product of the baseline it already computes, so the first pull request to fork from a commit does not pay for it either. Cached state is re-derived from the merge base whenever the pinned CodeBoarding version, .codeboardingignore, the configured depth, or the merge base changes, and state produced while analyzing a fork lives in a namespace trusted runs never restore from. Every cache step is best effort: a miss falls back to today's behaviour. /codeboarding refresh re-seeds from the base and /codeboarding full forces a full analysis, for when a run needs to ignore what came before. tests/test_merge_base_contract.py pins the merge base behaviour against a real git history and is marked protected; AGENTS.md records that no agent may weaken a protected test without explicit human consent. Existing users reconfigure nothing: same inputs, same permissions, and pull_request workflows keep working as they are. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/codeboarding.yml | 8 +- AGENT.md => AGENTS.md | 23 +- README.md | 20 +- action.yml | 88 ++++++- docs/COMMIT_STRATEGY.md | 19 +- scripts/action/analyze.sh | 129 +++++++++- scripts/action/build-review-artifact.sh | 8 +- scripts/action/build-review-comment.sh | 10 + scripts/action/cache-keys.sh | 58 +++++ scripts/action/deliver-sync.sh | 16 +- scripts/action/guard.sh | 51 +++- tests/test_action_cache.py | 253 ++++++++++++++++++++ tests/test_action_sync.py | 63 +++++ tests/test_merge_base_contract.py | 303 ++++++++++++++++++++++++ 14 files changed, 1016 insertions(+), 33 deletions(-) rename AGENT.md => AGENTS.md (72%) create mode 100755 scripts/action/cache-keys.sh create mode 100644 tests/test_action_cache.py create mode 100644 tests/test_merge_base_contract.py diff --git a/.github/workflows/codeboarding.yml b/.github/workflows/codeboarding.yml index 942b536..62b6d9c 100644 --- a/.github/workflows/codeboarding.yml +++ b/.github/workflows/codeboarding.yml @@ -2,10 +2,10 @@ name: CodeBoarding review on: pull_request: - # Generate once, when the PR becomes reviewable, not on every push, so we - # don't spend an LLM job per commit. Add `synchronize` to re-run on each - # push, or refresh anytime with /codeboarding. 'closed' only cancels an - # in-flight review (see concurrency), it doesn't start one. + # Generate once, when the PR becomes reviewable. Reusing this PR's previous + # analysis makes per-push runs affordable, so `synchronize` is a reasonable + # addition now; /codeboarding still refreshes on demand. 'closed' only + # cancels an in-flight review (see concurrency), it doesn't start one. types: [opened, reopened, ready_for_review, closed] issue_comment: types: [created] diff --git a/AGENT.md b/AGENTS.md similarity index 72% rename from AGENT.md rename to AGENTS.md index 61f38e6..d727d58 100644 --- a/AGENT.md +++ b/AGENTS.md @@ -1,4 +1,4 @@ -# AGENT.md — CodeBoarding-action +# AGENTS.md — CodeBoarding-action This repo is a GitHub Action with two modes, selected by the `mode` input: @@ -15,6 +15,27 @@ pinned to a release in `action.yml`. `scripts/engine_adapter.py` is the CLI adapter into it (no analysis logic lives there). Engine changes reach users only when that pin is bumped *and* a new action release ships. +## Protected tests + +Some tests encode a behavioural contract that is expensive to rediscover once +lost. They are marked with a `PROTECTED TEST` header naming what they protect. + +**No agent, assistant, or automated tool may edit, weaken, skip, rename or +delete a protected test — not even to make a build pass. Only a human may +change one, and only after explicitly saying so in that conversation.** A +request to "fix the failing tests" is not that consent. + +When a protected test fails, the behaviour it describes regressed. Fix the code. +If you believe the test itself is wrong, stop and say so, then wait for a human +to decide. + +Protected tests: + +- `tests/test_merge_base_contract.py` — a review compares a pull request against + its merge base, never against the base branch tip. Comparing against the tip + attributes other people's commits to the pull request and reports them + backwards, as removals. + ## Releases Consumers reference a moving major tag (`uses: CodeBoarding/CodeBoarding-action@v1`), diff --git a/README.md b/README.md index f063902..c584fb0 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ One GitHub Action with two modes: -- **`review`** (default) compares a pull request's exact base and head commits, posts an inline Mermaid architecture diff, and uploads the head `analysis.json` as a workflow artifact. +- **`review`** (default) compares a pull request's head with its merge base, posts an inline Mermaid architecture diff, and uploads the head `analysis.json` as a workflow artifact. - **`sync`** updates the versioned analysis state used by future incremental runs. It can push directly or open one rolling PR for protected branches. The action is a thin wrapper around the [CodeBoarding](https://github.com/CodeBoarding/CodeBoarding) CLI. Analysis logic and provider defaults live in Core, not in this repository. @@ -44,10 +44,24 @@ jobs: Automatic runs update one sticky **CodeBoarding review** comment. A trusted repository owner, member, or collaborator can comment `/codeboarding` to analyze the current PR head again, including on fork PRs; every command creates a new result comment. -The action checks out and analyzes the exact PR head SHA, but compares it with the exact upstream base SHA from the event. It does not commit generated files to either branch. +| Command | What it does | +|---|---| +| `/codeboarding` | Analyzes the current head, reusing this PR's previous analysis when one is available. | +| `/codeboarding refresh` | Ignores that previous analysis and re-derives the head from the merge base. | +| `/codeboarding full` | Forces a from-scratch full analysis of the head. | + +The action checks out and analyzes the exact PR head SHA, and compares it with the PR's **merge base** — the commit the branch forked from, which is what GitHub's own "Files changed" tab uses. Commits pushed to the base branch after the fork point are therefore not reported as this PR's changes; the comment notes how far behind the branch is instead. It does not commit generated files to either branch. Automatic fork runs are skipped because the `pull_request` event does not receive hosted OIDC credentials. A trusted `/codeboarding` command runs the released action code from the base repository and checks the fork's source into a separate analysis directory; it never executes an action definition from the fork with privileged credentials. +### Reused analysis + +Each review seeds the head analysis from this pull request's own previous run, so a run only covers the commits pushed since it. With no previous run, it seeds from the merge base's analysis, which `sync` mode publishes and every pull request in the repository shares. Both live in the GitHub Actions cache, and state is re-derived from the merge base whenever the pinned CodeBoarding version, `.codeboardingignore`, the configured analysis depth, or the merge base itself changes. State produced while analyzing a fork is namespaced separately and is never restored by a run on this repository's own code. + +Caching is best-effort: a cache miss, an unavailable cache service, or a GitHub Enterprise Server without one falls back to analyzing the merge base directly, exactly as before. + +Actions cache entries are scoped to the ref that wrote them. Automatic `pull_request` runs therefore reuse each other's analysis and the shared base entry, while a `/codeboarding` command — which runs on the default branch ref — reuses the base entry but not a chain built by automatic runs, so it costs one base-seeded incremental. The action also accepts `pull_request_target`, which runs on the base branch ref and lets both share one chain; that trigger has its own trade-offs (a PR that adds this workflow will not run it until merged, and the fork gate becomes load-bearing), so `pull_request` remains the recommended default. + ## Authentication and providers With no LLM inputs, the action uses CodeBoarding's hosted OpenRouter tier. It mints short-lived GitHub OIDC credentials per request, so the job needs `id-token: write` and no stored LLM secret. @@ -197,6 +211,8 @@ The `/codeboarding` command, comment heading, Mermaid direction (`LR`), hosted w | `n_changed` | review | Number of changed components. | | `truncated` | review | Whether the graph was reduced to fit GitHub limits. | | `review_artifact_url` | review | URL of the uploaded head analysis. | +| `seed_source` | review | `pr-chain` when the head grew from this PR's previous analysis, `base` otherwise. | +| `merge_base_sha` | review | Commit the head was compared against. | | `analysis_mode` | sync | `incremental` or `full`. | | `files_written` | sync | Number of persisted analysis artifacts produced. | | `committed` | sync | Whether a baseline commit was delivered. | diff --git a/action.yml b/action.yml index 2c07304..9e7f7fa 100644 --- a/action.yml +++ b/action.yml @@ -62,6 +62,12 @@ outputs: review_artifact_url: description: 'URL of the uploaded review analysis artifact.' value: ${{ steps.upload_review_artifact_dotcom.outputs.artifact-url }} + seed_source: + description: 'Which state the review head analysis grew from: pr-chain or base.' + value: ${{ steps.review_analyze.outputs.seed_source }} + merge_base_sha: + description: 'Commit the review compared the pull request head against.' + value: ${{ steps.guard.outputs.merge_base_sha }} analysis_mode: description: 'Whether sync used incremental or full analysis.' value: ${{ steps.sync_analyze.outputs.analysis_mode }} @@ -102,6 +108,7 @@ runs: EVENT_PR_NUMBER: ${{ github.event.pull_request.number }} PULL_HEAD_SHA: ${{ github.event.pull_request.head.sha }} PULL_BASE_SHA: ${{ github.event.pull_request.base.sha }} + PULL_BASE_REF: ${{ github.event.pull_request.base.ref }} PULL_HEAD_REPO: ${{ github.event.pull_request.head.repo.full_name }} PULL_BASE_REPO: ${{ github.event.pull_request.base.repo.full_name }} REPOSITORY: ${{ github.repository }} @@ -166,6 +173,41 @@ runs: LICENSE_KEY: ${{ inputs.license_key }} run: "$GITHUB_ACTION_PATH/scripts/action/configure-auth.sh" + - name: Resolve analysis cache keys + id: cache_keys + if: steps.guard.outputs.skip != 'true' + continue-on-error: true + shell: bash + env: + CHECKOUT_DIR: ${{ github.workspace }}/.codeboarding-target + MERGE_BASE_SHA: ${{ steps.guard.outputs.merge_base_sha }} + PR_NUMBER: ${{ steps.guard.outputs.pr_number }} + HEAD_SHA: ${{ steps.guard.outputs.head_sha }} + HEAD_REPO: ${{ steps.guard.outputs.head_repo }} + IS_FORK: ${{ steps.guard.outputs.is_fork }} + run: "$GITHUB_ACTION_PATH/scripts/action/cache-keys.sh" + + # An exact key is this merge base's own analysis; the prefix falls back to + # the newest baseline any run produced, which still warm-starts the catch-up. + - name: Restore base analysis + id: base_cache + if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.cache_keys.outputs.base_key != '' + continue-on-error: true + uses: actions/cache/restore@v4 + with: + path: ${{ runner.temp }}/cb-cache/base + key: ${{ steps.cache_keys.outputs.base_key }} + restore-keys: ${{ steps.cache_keys.outputs.base_restore_keys }} + + - name: Restore pull request analysis + if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.guard.outputs.seed_mode == 'chain' && steps.cache_keys.outputs.chain_key != '' + continue-on-error: true + uses: actions/cache/restore@v4 + with: + path: ${{ runner.temp }}/cb-cache/chain + key: ${{ steps.cache_keys.outputs.chain_key }} + restore-keys: ${{ steps.cache_keys.outputs.chain_restore_keys }} + - name: Analyze baseline id: sync_analyze if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'sync' @@ -174,6 +216,7 @@ runs: ACTION_PATH: ${{ github.action_path }} ANALYSIS_KIND: sync CHECKOUT_DIR: ${{ github.workspace }}/.codeboarding-target + CACHE_OUT_DIR: ${{ runner.temp }}/cb-cache/out FORCE_FULL: ${{ inputs.force_full }} MODEL: ${{ inputs.model }} AGENT_MODEL_INPUT: ${{ inputs.agent_model }} @@ -199,6 +242,16 @@ runs: REPOSITORY: ${{ github.repository }} run: "$GITHUB_ACTION_PATH/scripts/action/deliver-sync.sh" + # Keyed by the commit pull requests will branch from, so their merge base + # hits this entry exactly and they skip the base analysis entirely. + - name: Save baseline analysis + if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'sync' && steps.sync_commit.outputs.baseline_sha != '' && steps.cache_keys.outputs.base_key_prefix != '' + continue-on-error: true + uses: actions/cache/save@v4 + with: + path: ${{ runner.temp }}/cb-cache/out/base + key: ${{ steps.cache_keys.outputs.base_key_prefix }}${{ steps.sync_commit.outputs.baseline_sha }} + - name: Write sync summary if: always() && steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'sync' shell: bash @@ -218,9 +271,17 @@ runs: ACTION_PATH: ${{ github.action_path }} ANALYSIS_KIND: review CHECKOUT_DIR: ${{ github.workspace }}/.codeboarding-target - REVIEW_BASE_SHA: ${{ steps.guard.outputs.base_sha }} + REVIEW_BASE_SHA: ${{ steps.guard.outputs.merge_base_sha }} REVIEW_HEAD_SHA: ${{ steps.guard.outputs.head_sha }} REVIEW_BASE_REPO: ${{ steps.guard.outputs.base_repo }} + PR_NUMBER: ${{ steps.guard.outputs.pr_number }} + SEED_MODE: ${{ steps.guard.outputs.seed_mode }} + CACHE_BASE_DIR: ${{ runner.temp }}/cb-cache/base + CACHE_BASE_HIT: ${{ steps.base_cache.outputs.cache-hit }} + CACHE_CHAIN_DIR: ${{ runner.temp }}/cb-cache/chain + CACHE_OUT_DIR: ${{ runner.temp }}/cb-cache/out + ENGINE_VERSION: ${{ steps.cache_keys.outputs.engine_version }} + CFG_HASH: ${{ steps.cache_keys.outputs.cfg_hash }} GIT_TOKEN: ${{ inputs.github_token }} GITHUB_SERVER_URL: ${{ github.server_url }} MODEL: ${{ inputs.model }} @@ -228,6 +289,26 @@ runs: PARSING_MODEL_INPUT: ${{ inputs.parsing_model }} run: '"$GITHUB_ACTION_PATH/scripts/action/with-auth.sh" "$GITHUB_ACTION_PATH/scripts/action/analyze.sh"' + # Saved before the review is rendered: the analysis is the expensive part, so + # a later rendering or posting failure must not throw it away. + - name: Save pull request analysis + if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.cache_keys.outputs.chain_key != '' + continue-on-error: true + uses: actions/cache/save@v4 + with: + path: ${{ runner.temp }}/cb-cache/out/chain + key: ${{ steps.cache_keys.outputs.chain_key }} + + # Only trusted runs publish a base entry: it is restorable repository-wide, + # so a fork's run must never be able to place state there. + - name: Save base analysis + if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.review_analyze.outputs.save_base == 'true' && steps.guard.outputs.is_fork != 'true' && steps.cache_keys.outputs.base_key != '' + continue-on-error: true + uses: actions/cache/save@v4 + with: + path: ${{ runner.temp }}/cb-cache/out/base + key: ${{ steps.cache_keys.outputs.base_key }} + - name: Render review diagram id: review_render if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' @@ -246,8 +327,11 @@ runs: ANALYSIS_PATH: ${{ steps.review_analyze.outputs.analysis_path }} ANALYSIS_MODE: ${{ steps.review_analyze.outputs.analysis_mode }} BASE_SHA: ${{ steps.guard.outputs.base_sha }} + MERGE_BASE_SHA: ${{ steps.guard.outputs.merge_base_sha }} HEAD_SHA: ${{ steps.guard.outputs.head_sha }} PR_NUMBER: ${{ steps.guard.outputs.pr_number }} + SEED_SOURCE: ${{ steps.review_analyze.outputs.seed_source }} + CHAIN_DEPTH: ${{ steps.review_analyze.outputs.chain_depth }} run: "$GITHUB_ACTION_PATH/scripts/action/build-review-artifact.sh" - name: Upload review artifact @@ -268,6 +352,8 @@ runs: N_CHANGED: ${{ steps.review_render.outputs.n_changed }} ARTIFACT_URL: ${{ steps.upload_review_artifact_dotcom.outputs.artifact-url }} PR_NUMBER: ${{ steps.guard.outputs.pr_number }} + BEHIND_BY: ${{ steps.guard.outputs.behind_by }} + BASE_REF: ${{ steps.guard.outputs.base_ref }} 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 2f42073..7864478 100644 --- a/docs/COMMIT_STRATEGY.md +++ b/docs/COMMIT_STRATEGY.md @@ -22,7 +22,10 @@ The engine writes these under `.codeboarding/`: - ✅ `static_analysis.pkl` + `static_analysis.sha` — required for reliable warm-start incremental sync from the committed baseline. **Upload in review mode:** -- ✅ PR-head `analysis.json` and metadata containing the PR base SHA plus the committed baseline SHA when one was found — stored as a GitHub Actions artifact. +- ✅ PR-head `analysis.json` and metadata containing the PR base SHA, the merge base actually compared against, and how the head was seeded — stored as a GitHub Actions artifact. + +**Cache between runs (never committed):** +- ✅ The whole analysis state directory, twice: once per pull request (its head state, so the next run only covers new commits) and once per base commit (the merge base's analysis, shared by every pull request that forked from it). Sync mode publishes the base entry as a by-product of the baseline it already computes. > **Principle:** sync mode is the only git writer. Review mode never commits generated files to PR branches, so generated artifacts cannot conflict with `main` during merge. @@ -56,3 +59,17 @@ Either way the head analysis is seeded from that directory and runs incrementall | `health_report.json` | ✅ | sync commit on `main`; computed in review for comments, not uploaded | warnings | | `static_analysis.pkl` | ✅ | sync commit on `main` only | warm-start incremental baseline | | `static_analysis.sha` | ✅ | with `static_analysis.pkl` | warm-start gate | +| whole state directory | ❌ | Actions cache, per PR and per base commit | reuse the previous run instead of re-deriving from the base | + +## Cache identity + +The cache key pins everything that makes prior state meaningful: the pinned +CodeBoarding version, `.codeboardingignore`, and the merge base (which in turn +pins the analysis depth, since depth is read from that commit's baseline). +Anything else changing means no key matches, so the run re-derives from the base +rather than reusing state that describes a different analysis. + +State produced while analyzing a fork's code lives under its own key namespace +that trusted runs never restore from, and a fork run never writes a base entry: +`static_analysis.pkl` is a pickle, so restoring one derived from untrusted code +into a privileged run has to be impossible by construction, not by convention. diff --git a/scripts/action/analyze.sh b/scripts/action/analyze.sh index 3de910d..73051fb 100755 --- a/scripts/action/analyze.sh +++ b/scripts/action/analyze.sh @@ -34,12 +34,54 @@ depth_from() { "$analysis" 2>/dev/null || true } +# Core resolves incremental depth from depth_cap, falling back to depth_level for +# baselines predating it. Compare the same value, or a run that merely stopped +# short of its cap reads as a scope change. +depth_cap_from() { + local analysis="$1" + [ -f "$analysis" ] || return 0 + python3 -c 'import json,sys +metadata = json.load(open(sys.argv[1])).get("metadata", {}) +print(metadata.get("depth_cap", metadata.get("depth_level", "")))' "$analysis" 2>/dev/null || true +} + seed_state() { local checkout="$1" state="$2" mkdir -p "$state" [ ! -d "$checkout/.codeboarding" ] || cp -a "$checkout/.codeboarding/." "$state/" } +# Records which state this analysis grew from, so a later run can report its +# provenance and the review artifact can answer "which base did we use". +origin_field() { + local state="$1" field="$2" + [ -f "$state/origin.json" ] || return 0 + python3 -c 'import json,sys; print(json.load(open(sys.argv[1])).get(sys.argv[2], ""))' \ + "$state/origin.json" "$field" 2>/dev/null || true +} +write_origin() { + local state="$1" + python3 -c 'import json,os,sys +json.dump({ + "schema": 1, + "pr_number": os.environ.get("PR_NUMBER", ""), + "merge_base_sha": os.environ.get("REVIEW_BASE_SHA", ""), + "head_sha": os.environ.get("REVIEW_HEAD_SHA", ""), + "engine_version": os.environ.get("ENGINE_VERSION", ""), + "cfg_hash": os.environ.get("CFG_HASH", ""), + "seed_source": sys.argv[2], + "chain_depth": int(sys.argv[3]), +}, open(sys.argv[1], "w"), indent=2)' "$state/origin.json" "$2" "$3" +} + +cache_out() { + local state="$1" name="$2" + [ -n "${CACHE_OUT_DIR:-}" ] || return 0 + mkdir -p "$CACHE_OUT_DIR" + rm -rf "${CACHE_OUT_DIR:?}/$name" + cp -a "$state" "$CACHE_OUT_DIR/$name" +} + analyze_sync() { local work="$RUNNER_TEMP/codeboarding-sync" state="$RUNNER_TEMP/codeboarding-sync/analysis" rm -rf "$work" @@ -56,6 +98,9 @@ analyze_sync() { full "$CHECKOUT_DIR" "$state" "$depth" fi fi + # Sync already computes the state every review of this branch needs, so leave + # a copy for them instead of making the first pull request recompute it. + cache_out "$state" base printf 'analysis_mode=%s\nanalysis_path=%s\nanalysis_dir=%s\n' \ "$ANALYSIS_MODE" "$ANALYSIS_PATH" "$state" >> "$GITHUB_OUTPUT" } @@ -69,33 +114,91 @@ fetch_commit() { "${GITHUB_SERVER_URL%/}/${repository}.git" "$sha" --depth=1 } +# The restored chain already matches this engine, config and merge base: the +# cache key pins all three. Depth is read from the baseline, so check it here. +chain_usable() { + local base_analysis="$1" chain_cap base_cap + [ "${SEED_MODE:-chain}" = chain ] || return 1 + [ -f "${CACHE_CHAIN_DIR:-}/analysis.json" ] || return 1 + chain_cap="$(depth_cap_from "$CACHE_CHAIN_DIR/analysis.json")" + base_cap="$(depth_cap_from "$base_analysis")" + if [ -n "$chain_cap" ] && [ -n "$base_cap" ] && [ "$chain_cap" != "$base_cap" ]; then + echo "::notice::Analysis depth changed since the last run; re-seeding from the base analysis." + return 1 + fi +} + analyze_review() { local work="$RUNNER_TEMP/codeboarding-review" local base_checkout="$work/base" base_state="$work/base-state" head_state="$work/head-state" rm -rf "$work" mkdir -p "$work" - fetch_commit "$REVIEW_BASE_REPO" "$REVIEW_BASE_SHA" + + # An exact base-cache hit is this merge base's own analysis, so it needs no + # engine run at all. A prefix hit is some other commit's baseline: useful as a + # warm seed, never usable as this comparison's baseline. + local base_source=cache + if [ "${CACHE_BASE_HIT:-}" = true ] && [ -f "${CACHE_BASE_DIR:-}/analysis.json" ]; then + cp -a "$CACHE_BASE_DIR" "$base_state" + else + base_source=computed + fetch_commit "$REVIEW_BASE_REPO" "$REVIEW_BASE_SHA" + git -C "$CHECKOUT_DIR" worktree add --detach "$base_checkout" "$REVIEW_BASE_SHA" >/dev/null + if [ -f "${CACHE_BASE_DIR:-}/analysis.json" ]; then + cp -a "$CACHE_BASE_DIR" "$base_state" + else + seed_state "$base_checkout" "$base_state" + fi + local base_depth + base_depth="$(depth_from "$base_state/analysis.json")" + incremental "$base_checkout" "$base_state" + if [ "$REQUIRES_FULL" = true ]; then + full "$base_checkout" "$base_state" "${base_depth:-2}" + fi + fi unset GIT_TOKEN - git -C "$CHECKOUT_DIR" worktree add --detach "$base_checkout" "$REVIEW_BASE_SHA" >/dev/null - seed_state "$base_checkout" "$base_state" + local base_analysis="$base_state/analysis.json" + [ -f "$base_analysis" ] || { echo "::error::Review baseline analysis is missing."; exit 1; } local depth - depth="$(depth_from "$base_state/analysis.json")" + depth="$(depth_from "$base_analysis")" depth="${depth:-2}" - incremental "$base_checkout" "$base_state" - if [ "$REQUIRES_FULL" = true ]; then - full "$base_checkout" "$base_state" "$depth" + + # Seed the head from this pull request's own last analysis when there is one, + # so the run only covers commits pushed since it. + local seed_source=base chain_depth=1 previous_depth + if chain_usable "$base_analysis"; then + cp -a "$CACHE_CHAIN_DIR" "$head_state" + seed_source=pr-chain + previous_depth="$(origin_field "$head_state" chain_depth)" + case "$previous_depth" in + ''|*[!0-9]*) previous_depth=0 ;; + esac + chain_depth=$(( previous_depth + 1 )) + else + cp -a "$base_state" "$head_state" fi - local base_analysis="$base_state/analysis.json" - cp -a "$base_state" "$head_state" - incremental "$CHECKOUT_DIR" "$head_state" + rm -f "$head_state/origin.json" - if [ "$REQUIRES_FULL" = true ]; then + if [ "${SEED_MODE:-chain}" = full ]; then full "$CHECKOUT_DIR" "$head_state" "$depth" + else + incremental "$CHECKOUT_DIR" "$head_state" + if [ "$REQUIRES_FULL" = true ]; then + full "$CHECKOUT_DIR" "$head_state" "$depth" + fi + fi + + write_origin "$head_state" "$seed_source" "$chain_depth" + cache_out "$head_state" chain + local save_base=false + if [ "$base_source" = computed ]; then + cache_out "$base_state" base + save_base=true fi - printf 'analysis_mode=%s\nanalysis_path=%s\nbase_analysis_path=%s\n' \ - "$ANALYSIS_MODE" "$ANALYSIS_PATH" "$base_analysis" >> "$GITHUB_OUTPUT" + printf 'analysis_mode=%s\nanalysis_path=%s\nbase_analysis_path=%s\nseed_source=%s\nchain_depth=%s\nsave_base=%s\n' \ + "$ANALYSIS_MODE" "$ANALYSIS_PATH" "$base_analysis" "$seed_source" "$chain_depth" "$save_base" >> "$GITHUB_OUTPUT" } case "$ANALYSIS_KIND" in diff --git a/scripts/action/build-review-artifact.sh b/scripts/action/build-review-artifact.sh index 8fae173..e136f3d 100755 --- a/scripts/action/build-review-artifact.sh +++ b/scripts/action/build-review-artifact.sh @@ -3,11 +3,17 @@ set -euo pipefail mkdir -p "${RUNNER_TEMP}/cb-review-artifact" cp "$ANALYSIS_PATH" "${RUNNER_TEMP}/cb-review-artifact/analysis.json" +# base_sha stays the event's base branch tip for consumers that key on it; +# merge_base_sha records the commit the diagram actually compared against. jq -n \ --arg mode "$ANALYSIS_MODE" \ --arg base_sha "$BASE_SHA" \ + --arg merge_base_sha "$MERGE_BASE_SHA" \ --arg head_sha "$HEAD_SHA" \ --arg pr_number "$PR_NUMBER" \ - '{mode: $mode, base_sha: $base_sha, head_sha: $head_sha, pr_number: $pr_number}' \ + --arg seed_source "$SEED_SOURCE" \ + --arg chain_depth "$CHAIN_DEPTH" \ + '{mode: $mode, base_sha: $base_sha, merge_base_sha: $merge_base_sha, head_sha: $head_sha, + pr_number: $pr_number, seed_source: $seed_source, chain_depth: $chain_depth}' \ > "${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 713aec7..17794f5 100755 --- a/scripts/action/build-review-comment.sh +++ b/scripts/action/build-review-comment.sh @@ -12,6 +12,16 @@ WEBVIEW_URL="https://app.codeboarding.org/${GITHUB_REPOSITORY}/pull/${PR_NUMBER} BODY="${RUNNER_TEMP}/review-comment.md" printf '### CodeBoarding review\n\n**Status:** %s changed %s\n' "$N_CHANGED" "$COMPONENT_NOUN" > "$BODY" printf '\nSee the full change in [CodeBoarding](%s).\n' "$WEBVIEW_URL" >> "$BODY" +# The diagram compares against the merge base, so commits landed on the base +# branch since this PR forked are excluded. Say so rather than hide it. +BEHIND="${BEHIND_BY:-0}" +if [ "$BEHIND" -gt 0 ] 2>/dev/null; then + COMMIT_NOUN="commits" + [ "$BEHIND" != "1" ] || COMMIT_NOUN="commit" + # shellcheck disable=SC2016 # the backticks are Markdown, not a command + printf '\nCompared against the merge base: this branch is %s %s behind `%s`.\n' \ + "$BEHIND" "$COMMIT_NOUN" "${BASE_REF:-the base branch}" >> "$BODY" +fi { printf '\n' cat "$DIAGRAM" diff --git a/scripts/action/cache-keys.sh b/scripts/action/cache-keys.sh new file mode 100755 index 0000000..76c9be4 --- /dev/null +++ b/scripts/action/cache-keys.sh @@ -0,0 +1,58 @@ +#!/usr/bin/env bash +# Derives the analysis cache identity and outputs its restore and save keys. +set -euo pipefail + +# Bump when the cached state layout or its meaning changes: every existing entry +# stops matching, so runs re-seed from the base instead of reusing stale state. +CACHE_SCHEMA=v1 + +digest() { + if command -v sha256sum >/dev/null 2>&1; then + sha256sum | cut -c1-16 + else + shasum -a 256 | cut -c1-16 + fi +} + +# The pinned release is the source of truth. ENGINE_VERSION only overrides it +# where the package is not importable, such as tests. +engine_version="${ENGINE_VERSION:-}" +if [ -z "$engine_version" ]; then + engine_version="$(python3 -c 'from importlib.metadata import version; print(version("codeboarding"))' 2>/dev/null || true)" +fi +if [ -z "$engine_version" ]; then + echo "::notice::Could not resolve the installed CodeBoarding version; skipping analysis caching." + exit 0 +fi + +# The key pins the engine and the analysis scope. Depth needs no hash: it is +# read from the baseline at the merge base, which the key already pins. +ignore_digest=none +ignore_file="$CHECKOUT_DIR/.codeboarding/.codeboardingignore" +[ ! -f "$ignore_file" ] || ignore_digest="$(digest < "$ignore_file")" +cfg_hash="$(printf '%s\n%s\n%s\n' "$CACHE_SCHEMA" "$engine_version" "$ignore_digest" | digest)" + +base_key_prefix="cb-base-$CACHE_SCHEMA-$cfg_hash-" +{ + echo "engine_version=$engine_version" + echo "cfg_hash=$cfg_hash" + echo "base_key_prefix=$base_key_prefix" + echo "base_restore_keys=$base_key_prefix" +} >> "$GITHUB_OUTPUT" + +[ -n "${MERGE_BASE_SHA:-}" ] || exit 0 +echo "base_key=$base_key_prefix$MERGE_BASE_SHA" >> "$GITHUB_OUTPUT" + +[ -n "${PR_NUMBER:-}" ] && [ -n "${HEAD_SHA:-}" ] || exit 0 +# State produced while analyzing a fork stays in its own namespace: a trusted +# run must never restore, and so never unpickle, state derived from code the +# repository does not control. +if [ "${IS_FORK:-false}" = true ]; then + chain_prefix="cb-fork-$CACHE_SCHEMA-$cfg_hash-${HEAD_REPO//\//-}-pr$PR_NUMBER-mb$MERGE_BASE_SHA-" +else + chain_prefix="cb-head-$CACHE_SCHEMA-$cfg_hash-pr$PR_NUMBER-mb$MERGE_BASE_SHA-" +fi +{ + echo "chain_key=$chain_prefix$HEAD_SHA" + echo "chain_restore_keys=$chain_prefix" +} >> "$GITHUB_OUTPUT" diff --git a/scripts/action/deliver-sync.sh b/scripts/action/deliver-sync.sh index 7af481e..692c899 100755 --- a/scripts/action/deliver-sync.sh +++ b/scripts/action/deliver-sync.sh @@ -18,6 +18,11 @@ chmod 700 "$ASKPASS" export GIT_ASKPASS="$ASKPASS" GIT_TERMINAL_PROMPT=0 export GH_HOST="${GH_HOST#*://}" trap 'rm -f "$ASKPASS"' EXIT +# baseline_sha names the commit later pull requests branch from, so their merge +# base restores this run's analysis instead of recomputing it. +emit_result() { + printf 'files_written=%s\ncommitted=%s\nbaseline_sha=%s\n' "$1" "$2" "$3" >> "$GITHUB_OUTPUT" +} close_stale_pr() { [ "$SYNC_STRATEGY" = pull_request ] || return 0 git fetch "$REMOTE" "$TARGET_BRANCH" @@ -46,7 +51,7 @@ classify_push_failure() { current="$(git ls-remote "$REMOTE" "refs/heads/$branch" | awk '{print $1; exit}')" [ "$current" != "$(git rev-parse HEAD)" ] || return 0 if [ "$current" != "$expected" ]; then - printf 'files_written=%s\ncommitted=false\n' "$files_written" >> "$GITHUB_OUTPUT" + emit_result "$files_written" false "$BASE_SHA" echo "::notice::A newer run updated $branch; leaving it untouched." exit 0 fi @@ -72,7 +77,7 @@ files_written="$(find "$CHECKOUT_DIR/.codeboarding" -maxdepth 1 -type f \ git fetch "$REMOTE" "$TARGET_BRANCH" remote_sha="$(git rev-parse FETCH_HEAD)" if [ "$remote_sha" != "$BASE_SHA" ]; then - printf 'files_written=%s\ncommitted=false\n' "$files_written" >> "$GITHUB_OUTPUT" + emit_result "$files_written" false "$BASE_SHA" echo "::notice::$TARGET_BRANCH advanced during analysis; a newer run should update its baseline." exit 0 fi @@ -80,7 +85,7 @@ fi if git diff --cached --quiet || git diff --cached --quiet -I '"generated_at"' -I '"timestamp"'; then git reset -q close_stale_pr - printf 'files_written=%s\ncommitted=false\n' "$files_written" >> "$GITHUB_OUTPUT" + emit_result "$files_written" false "$BASE_SHA" echo "::notice::The CodeBoarding baseline is unchanged." exit 0 fi @@ -91,7 +96,7 @@ if [ "$SYNC_STRATEGY" = push ]; then if ! git push "$REMOTE" "HEAD:refs/heads/$TARGET_BRANCH"; then classify_push_failure "$BASE_SHA" "$TARGET_BRANCH" fi - printf 'files_written=%s\ncommitted=true\n' "$files_written" >> "$GITHUB_OUTPUT" + emit_result "$files_written" true "$(git rev-parse HEAD)" exit 0 fi @@ -119,9 +124,8 @@ fi pr_url="$(jq -r .html_url <<< "$pr_json")" pr_number="$(jq -r .number <<< "$pr_json")" +emit_result "$files_written" true "$BASE_SHA" { - echo "files_written=$files_written" - echo "committed=true" echo "sync_pr_url=$pr_url" echo "sync_pr_number=$pr_number" } >> "$GITHUB_OUTPUT" diff --git a/scripts/action/guard.sh b/scripts/action/guard.sh index 7ad7f28..3ff2f6d 100755 --- a/scripts/action/guard.sh +++ b/scripts/action/guard.sh @@ -3,6 +3,7 @@ set -euo pipefail fail() { echo "::error::$1"; exit 1; } skip() { echo "::notice::$1"; echo "skip=true" >> "$GITHUB_OUTPUT"; exit 0; } +[ -z "${GH_HOST:-}" ] || export GH_HOST="${GH_HOST#*://}" case "$MODE" in review|sync) ;; *) fail "mode must be review or sync." ;; @@ -28,7 +29,6 @@ if [ "$MODE" = sync ]; then [ "$SYNC_STRATEGY" != pull_request ] || [ "$target_branch" != codeboarding/sync ] || fail "target_branch must differ from codeboarding/sync." sync_branch_start_sha="" if [ "$SYNC_STRATEGY" = pull_request ]; then - export GH_HOST="${GH_HOST#*://}" sync_branch_start_sha="$(gh api "repos/$REPOSITORY/branches/codeboarding%2Fsync" --jq '.commit.sha' 2>/dev/null || true)" fi { @@ -40,29 +40,38 @@ if [ "$MODE" = sync ]; then exit 0 fi +seed_mode=chain case "$EVENT" in - pull_request) + pull_request|pull_request_target) pr_number="$EVENT_PR_NUMBER" base_sha="$PULL_BASE_SHA" head_sha="$PULL_HEAD_SHA" base_repo="$PULL_BASE_REPO" head_repo="$PULL_HEAD_REPO" + base_ref="${PULL_BASE_REF:-}" ;; issue_comment) - first_word="$(printf '%s' "$COMMENT_BODY" | tr -d '\r' | awk 'NR == 1 {print $1}')" + read -r first_word second_word <<< "$(printf '%s' "$COMMENT_BODY" | tr -d '\r' | awk 'NR == 1 {print $1, $2}')" [ "$first_word" = /codeboarding ] || skip "Comment is not a /codeboarding command." case "$AUTHOR_ASSOCIATION" in OWNER|MEMBER|COLLABORATOR) ;; *) skip "Only trusted collaborators may run /codeboarding." ;; esac [ -n "$ISSUE_PR_URL" ] || skip "The command was not posted on a pull request." - export GH_HOST="${GH_HOST#*://}" + # refresh ignores the pull request's own cached analysis and re-seeds from + # the base; full additionally forces a from-scratch head analysis. + case "$second_word" in + "") ;; + refresh|full) seed_mode="$second_word" ;; + *) echo "::warning::Unknown /codeboarding argument '$second_word'; running the default incremental review." ;; + esac pr_json="$(gh api "$ISSUE_PR_URL")" pr_number="$(jq -r '.number // empty' <<< "$pr_json")" base_sha="$(jq -r '.base.sha // empty' <<< "$pr_json")" head_sha="$(jq -r '.head.sha // empty' <<< "$pr_json")" base_repo="$(jq -r '.base.repo.full_name // empty' <<< "$pr_json")" head_repo="$(jq -r '.head.repo.full_name // empty' <<< "$pr_json")" + base_ref="$(jq -r '.base.ref // empty' <<< "$pr_json")" ;; *) skip "Review mode ignores $EVENT events." ;; esac @@ -72,15 +81,49 @@ if [ -z "$pr_number" ] || [ -z "$base_sha" ] || [ -z "$head_sha" ] || [ -z "$bas fi [ "$head_repo" = "$base_repo" ] || [ "$EVENT" = issue_comment ] || skip "Fork pull requests require a trusted /codeboarding command." +# The event's base sha is the base branch tip, so it moves whenever anyone else +# pushes. Analyzing against it reports their commits as this pull request's +# changes. The merge base is what GitHub's own file diff uses, and it only moves +# when this pull request actually rebases or merges the base in. +merge_base_sha="$base_sha" +behind_by=0 +basehead="$base_sha...$head_sha" +[ "$head_repo" = "$base_repo" ] || basehead="$base_sha...${head_repo%%/*}:$head_sha" +compare="$(gh api "repos/$base_repo/compare/$basehead" \ + --jq '[.merge_base_commit.sha // "", .behind_by // 0] | @tsv' 2>/dev/null || true)" +# Split on the tab explicitly. Field splitting would drop an empty merge base +# and shift the distance into its place, which reads as a valid commit. +compare_merge_base="${compare%%$'\t'*}" +compare_behind="${compare#*$'\t'}" +case "$compare_behind" in + ''|*[!0-9]*) compare_behind=0 ;; +esac +# The distance only means anything alongside the merge base it was measured +# against, so take both or neither: reporting one while comparing against the +# tip would describe a comparison this run did not make. +if [ -n "$compare_merge_base" ]; then + merge_base_sha="$compare_merge_base" + behind_by="$compare_behind" +else + echo "::notice::Could not resolve the merge base; comparing against the base branch tip instead." +fi + comment_id=codeboarding-review [ "$EVENT" != issue_comment ] || comment_id="codeboarding-review-${GITHUB_RUN_ID}" +is_fork=false +[ "$head_repo" = "$base_repo" ] || is_fork=true { echo "pr_number=$pr_number" echo "base_sha=$base_sha" + echo "merge_base_sha=$merge_base_sha" + echo "behind_by=$behind_by" + echo "base_ref=$base_ref" echo "head_sha=$head_sha" echo "base_repo=$base_repo" echo "head_repo=$head_repo" echo "checkout_repo=$head_repo" echo "checkout_ref=$head_sha" echo "comment_id=$comment_id" + echo "seed_mode=$seed_mode" + echo "is_fork=$is_fork" } >> "$GITHUB_OUTPUT" diff --git a/tests/test_action_cache.py b/tests/test_action_cache.py new file mode 100644 index 0000000..bdf773b --- /dev/null +++ b/tests/test_action_cache.py @@ -0,0 +1,253 @@ +"""Tests for the cached-analysis reuse boundary owned by the action.""" + +from __future__ import annotations + +import json +import os +import subprocess +import tempfile +import unittest +from pathlib import Path + + +ROOT = Path(__file__).resolve().parent.parent +CACHE_KEYS = ROOT / "scripts" / "action" / "cache-keys.sh" +ANALYZE = ROOT / "scripts" / "action" / "analyze.sh" + +ENGINE_STUB = '''#!/usr/bin/env python3 +"""CodeBoarding CLI stand-in: records each call and writes a minimal analysis.""" +import json, os, sys + +argv = sys.argv[1:] +output = argv[argv.index("--output-dir") + 1] +os.makedirs(output, exist_ok=True) +with open(os.environ["CB_ENGINE_LOG"], "a") as log: + log.write(json.dumps({"mode": argv[0], "checkout": argv[argv.index("--local") + 1]}) + "\\n") +analysis = os.path.join(output, "analysis.json") +with open(analysis, "w") as handle: + json.dump({"metadata": {"depth_level": 2}, "components": [], "components_relations": []}, handle) +print(json.dumps({"requiresFullAnalysis": False, "analysis_path": analysis})) +''' + + +def _state(directory: Path, depth: int = 2, cap: int | None = None, **origin: object) -> Path: + directory.mkdir(parents=True, exist_ok=True) + metadata: dict[str, int] = {"depth_level": depth} + if cap is not None: + metadata["depth_cap"] = cap + (directory / "analysis.json").write_text( + json.dumps({"metadata": metadata, "components": [], "components_relations": []}), + encoding="utf-8", + ) + (directory / "static_analysis.pkl").write_text("pickle", encoding="utf-8") + if origin: + (directory / "origin.json").write_text(json.dumps(origin), encoding="utf-8") + return directory + + +class CacheKeyTests(unittest.TestCase): + def setUp(self) -> None: + self.temp_dir = tempfile.TemporaryDirectory() + self.root = Path(self.temp_dir.name) + self.checkout = self.root / "checkout" + (self.checkout / ".codeboarding").mkdir(parents=True) + self.output = self.root / "github-output" + + def tearDown(self) -> None: + self.temp_dir.cleanup() + + def _run(self, **extra: str) -> dict[str, str]: + self.output.write_text("", encoding="utf-8") + result = subprocess.run( + [str(CACHE_KEYS)], + env={ + "PATH": os.environ["PATH"], + "GITHUB_OUTPUT": str(self.output), + "CHECKOUT_DIR": str(self.checkout), + "ENGINE_VERSION": "0.13.8", + "MERGE_BASE_SHA": "mergebasesha", + "PR_NUMBER": "42", + "HEAD_SHA": "head-sha", + "HEAD_REPO": "owner/repo", + "IS_FORK": "false", + **extra, + }, + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(result.returncode, 0, result.stderr or result.stdout) + values: dict[str, str] = {} + for line in self.output.read_text(encoding="utf-8").splitlines(): + key, _, value = line.partition("=") + values[key] = value + return values + + def test_chain_key_pins_the_pull_request_and_its_merge_base(self) -> None: + values = self._run() + + self.assertTrue(values["chain_key"].startswith("cb-head-v1-")) + self.assertIn("-pr42-mbmergebasesha-", values["chain_key"]) + self.assertTrue(values["chain_key"].endswith("head-sha")) + self.assertEqual(values["chain_key"][: -len("head-sha")], values["chain_restore_keys"]) + self.assertEqual(values["base_key"], values["base_key_prefix"] + "mergebasesha") + + def test_fork_state_cannot_be_restored_by_a_trusted_run(self) -> None: + trusted = self._run() + fork = self._run(IS_FORK="true", HEAD_REPO="contributor/repo") + + self.assertTrue(fork["chain_key"].startswith("cb-fork-v1-")) + self.assertIn("contributor-repo", fork["chain_key"]) + self.assertFalse(fork["chain_key"].startswith(trusted["chain_restore_keys"])) + self.assertFalse(trusted["chain_key"].startswith(fork["chain_restore_keys"])) + + def test_analysis_scope_and_engine_version_change_the_identity(self) -> None: + baseline = self._run() + self.assertEqual(baseline["cfg_hash"], self._run()["cfg_hash"]) + + self.assertNotEqual(baseline["cfg_hash"], self._run(ENGINE_VERSION="0.14.0")["cfg_hash"]) + + (self.checkout / ".codeboarding" / ".codeboardingignore").write_text("docs/\n", encoding="utf-8") + self.assertNotEqual(baseline["cfg_hash"], self._run()["cfg_hash"]) + + def test_unresolvable_engine_version_disables_caching_instead_of_failing(self) -> None: + stub_bin = self.root / "bin" + stub_bin.mkdir() + python_stub = stub_bin / "python3" + python_stub.write_text("#!/bin/sh\nexit 1\n", encoding="utf-8") + python_stub.chmod(0o755) + + values = self._run(ENGINE_VERSION="", PATH=f"{stub_bin}:{os.environ['PATH']}") + + self.assertEqual(values, {}) + + +class ReviewChainTests(unittest.TestCase): + def setUp(self) -> None: + self.temp_dir = tempfile.TemporaryDirectory() + self.root = Path(self.temp_dir.name) + self.bin_dir = self.root / "bin" + self.bin_dir.mkdir() + stub = self.bin_dir / "codeboarding" + stub.write_text(ENGINE_STUB, encoding="utf-8") + stub.chmod(0o755) + self.engine_log = self.root / "engine.log" + self.engine_log.write_text("", encoding="utf-8") + self.output = self.root / "github-output" + self.runner_temp = self.root / "runner" + self.runner_temp.mkdir() + self.checkout = self.root / "checkout" + self.checkout.mkdir() + self.cache_base = self.root / "cache" / "base" + self.cache_chain = self.root / "cache" / "chain" + self.cache_out = self.root / "cache" / "out" + + 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_temp), + "CB_ENGINE_LOG": str(self.engine_log), + "ACTION_PATH": str(ROOT), + "ANALYSIS_KIND": "review", + "CHECKOUT_DIR": str(self.checkout), + "REVIEW_BASE_SHA": "merge-base-sha", + "REVIEW_HEAD_SHA": "head-sha", + "REVIEW_BASE_REPO": "owner/repo", + "GITHUB_SERVER_URL": "https://github.com", + "PR_NUMBER": "42", + "ENGINE_VERSION": "0.13.8", + "CFG_HASH": "cfg", + "SEED_MODE": "chain", + "CACHE_BASE_DIR": str(self.cache_base), + "CACHE_BASE_HIT": "true", + "CACHE_CHAIN_DIR": str(self.cache_chain), + "CACHE_OUT_DIR": str(self.cache_out), + **extra, + }, + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(result.returncode, 0, result.stderr or result.stdout) + values: dict[str, str] = {} + for line in self.output.read_text(encoding="utf-8").splitlines(): + key, _, value = line.partition("=") + values[key] = value + return values + + def _engine_calls(self) -> list[dict[str, str]]: + return [json.loads(line) for line in self.engine_log.read_text(encoding="utf-8").splitlines()] + + def test_warm_chain_analyzes_only_the_head(self) -> None: + _state(self.cache_base) + _state(self.cache_chain, chain_depth=3, seed_source="pr-chain") + + values = self._analyze() + + self.assertEqual(values["seed_source"], "pr-chain") + self.assertEqual(values["chain_depth"], "4") + self.assertEqual(values["save_base"], "false") + calls = self._engine_calls() + self.assertEqual(len(calls), 1, f"only the head should be analyzed, got {calls}") + self.assertEqual(calls[0]["checkout"], str(self.checkout)) + + def test_cached_base_without_a_chain_seeds_from_the_base(self) -> None: + _state(self.cache_base) + + values = self._analyze() + + self.assertEqual(values["seed_source"], "base") + self.assertEqual(values["chain_depth"], "1") + self.assertEqual(len(self._engine_calls()), 1) + + def test_refresh_ignores_the_pull_request_chain(self) -> None: + _state(self.cache_base) + _state(self.cache_chain, chain_depth=3) + + values = self._analyze(SEED_MODE="refresh") + + self.assertEqual(values["seed_source"], "base") + self.assertEqual(values["chain_depth"], "1") + + def test_depth_change_discards_the_chain(self) -> None: + _state(self.cache_base, depth=2) + _state(self.cache_chain, depth=1, chain_depth=3) + + values = self._analyze() + + self.assertEqual(values["seed_source"], "base") + + def test_a_run_that_stopped_short_of_its_cap_keeps_the_chain(self) -> None: + # Core resolves incremental depth from depth_cap, so a realized + # depth_level below the cap is not a scope change. + _state(self.cache_base, depth=2, cap=2) + _state(self.cache_chain, depth=1, cap=2, chain_depth=3) + + values = self._analyze() + + self.assertEqual(values["seed_source"], "pr-chain") + + def test_analysis_is_staged_for_the_cache(self) -> None: + _state(self.cache_base) + _state(self.cache_chain) + + self._analyze() + + staged = self.cache_out / "chain" + self.assertTrue((staged / "analysis.json").is_file()) + origin = json.loads((staged / "origin.json").read_text(encoding="utf-8")) + self.assertEqual(origin["merge_base_sha"], "merge-base-sha") + self.assertEqual(origin["head_sha"], "head-sha") + self.assertEqual(origin["engine_version"], "0.13.8") + self.assertFalse((self.cache_out / "base").exists(), "a cached base needs no re-save") + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_action_sync.py b/tests/test_action_sync.py index 1ebd829..822886c 100644 --- a/tests/test_action_sync.py +++ b/tests/test_action_sync.py @@ -42,6 +42,66 @@ def test_review_guard_rejects_fork_before_checkout(self) -> None: self.assertTrue(values.endswith("skip=true\n")) self.assertNotIn("checkout_ref=", values) + def test_review_guard_rejects_fork_pull_request_target_before_checkout(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + output = Path(tmp) / "github-output" + result = subprocess.run( + [str(GUARD)], + env={ + "PATH": os.environ["PATH"], + "GITHUB_OUTPUT": str(output), + "MODE": "review", + "EVENT": "pull_request_target", + "EVENT_PR_NUMBER": "42", + "PULL_BASE_SHA": "base-sha", + "PULL_HEAD_SHA": "head-sha", + "PULL_BASE_REPO": "owner/repo", + "PULL_HEAD_REPO": "contributor/repo", + }, + capture_output=True, + text=True, + check=False, + ) + + self.assertEqual(result.returncode, 0, result.stderr or result.stdout) + values = output.read_text(encoding="utf-8") + self.assertTrue(values.endswith("skip=true\n")) + self.assertNotIn("checkout_ref=", values) + + def test_review_guard_ignores_a_distance_it_cannot_anchor(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + fake_bin = root / "bin" + fake_bin.mkdir() + gh = fake_bin / "gh" + # A comparison with no common ancestor still reports behind_by. + gh.write_text("#!/usr/bin/env bash\nprintf '\\t3\\n'\n", encoding="utf-8") + gh.chmod(0o755) + output = root / "github-output" + result = subprocess.run( + [str(GUARD)], + env={ + "PATH": f"{fake_bin}:{os.environ['PATH']}", + "GITHUB_OUTPUT": str(output), + "GITHUB_RUN_ID": "123", + "MODE": "review", + "EVENT": "pull_request", + "EVENT_PR_NUMBER": "42", + "PULL_BASE_SHA": "base-sha", + "PULL_HEAD_SHA": "head-sha", + "PULL_BASE_REPO": "owner/repo", + "PULL_HEAD_REPO": "owner/repo", + }, + capture_output=True, + text=True, + check=False, + ) + + self.assertEqual(result.returncode, 0, result.stderr or result.stdout) + values = output.read_text(encoding="utf-8") + self.assertIn("merge_base_sha=base-sha\n", values) + self.assertIn("behind_by=0\n", values) + def test_review_guard_accepts_trusted_fork_command(self) -> None: with tempfile.TemporaryDirectory() as tmp: root = Path(tmp) @@ -50,6 +110,9 @@ def test_review_guard_accepts_trusted_fork_command(self) -> None: gh = fake_bin / "gh" gh.write_text( "#!/usr/bin/env bash\n" + 'case "$2" in\n' + " */compare/*) printf 'merge-base-sha\\t2\\n'; exit 0 ;;\n" + "esac\n" "cat <<'JSON'\n" '{"number":42,"base":{"sha":"base-sha","repo":{"full_name":"owner/repo"}},' '"head":{"sha":"head-sha","repo":{"full_name":"contributor/repo"}}}\n' diff --git a/tests/test_merge_base_contract.py b/tests/test_merge_base_contract.py new file mode 100644 index 0000000..938de62 --- /dev/null +++ b/tests/test_merge_base_contract.py @@ -0,0 +1,303 @@ +"""PROTECTED TEST — DO NOT MODIFY WITHOUT EXPLICIT HUMAN CONSENT. + +No agent, assistant, or automated tool may edit, weaken, skip, rename or delete +this file. If a change here looks necessary, stop and ask a human to decide. A +failure in this file means the behaviour changed, not that the test is wrong. + +WHAT THIS PROTECTS +------------------ +A review must compare a pull request against its **merge base** — the commit the +branch actually forked from — and never against the base branch tip. + + main: X ──► Z Z landed after this branch forked + │ + PR: └──► P the pull request's own work + +The event payload's `base.sha` is Z. Comparing against Z reports Z's components +as this pull request's changes (and reports them backwards, as removals). The +merge base is X, which is what `git diff main...PR` and GitHub's own "Files +changed" tab use. + +The tests below build a real git history in that exact shape and assert that +X, not Z, is what the action resolves and analyzes. +""" + +from __future__ import annotations + +import json +import os +import subprocess +import tempfile +import unittest +from pathlib import Path + + +ROOT = Path(__file__).resolve().parent.parent +GUARD = ROOT / "scripts" / "action" / "guard.sh" +ANALYZE = ROOT / "scripts" / "action" / "analyze.sh" + +GH_STUB = '''#!/usr/bin/env python3 +"""Minimal `gh` stand-in answering from a real git repository.""" +import json, os, subprocess, sys + +argv = sys.argv[1:] +path = argv[1] +with open(os.environ["CB_GH_LOG"], "a") as log: + log.write(path + "\\n") + +if "/compare/" in path: + repo = os.environ["CB_FIXTURE_REPO"] + base, head = path.split("/compare/", 1)[1].split("...") + head = head.split(":")[-1] + def git(*args): + return subprocess.run(["git", "-C", repo, *args], capture_output=True, text=True).stdout.strip() + body = json.dumps({ + "merge_base_commit": {"sha": git("merge-base", base, head)}, + "behind_by": int(git("rev-list", "--count", f"{head}..{base}") or 0), + "status": "diverged", + }) +else: + body = os.environ["CB_PR_JSON"] + +if "--jq" in argv: + expr = argv[argv.index("--jq") + 1] + sys.stdout.write(subprocess.run(["jq", "-r", expr], input=body, capture_output=True, text=True).stdout) +else: + sys.stdout.write(body) +''' + +ENGINE_STUB = '''#!/usr/bin/env python3 +"""Minimal CodeBoarding CLI stand-in recording which tree it was given.""" +import json, os, subprocess, sys + +argv = sys.argv[1:] +checkout = argv[argv.index("--local") + 1] +output = argv[argv.index("--output-dir") + 1] +head = subprocess.run(["git", "-C", checkout, "rev-parse", "HEAD"], capture_output=True, text=True).stdout.strip() +os.makedirs(output, exist_ok=True) +with open(os.environ["CB_ENGINE_LOG"], "a") as log: + log.write(json.dumps({ + "mode": argv[0], + "head": head, + "files": sorted(p for p in os.listdir(checkout) if not p.startswith(".")), + }) + "\\n") +analysis = os.path.join(output, "analysis.json") +with open(analysis, "w") as handle: + json.dump({"metadata": {"depth_level": 2}, "components": [], "components_relations": []}, handle) +print(json.dumps({"requiresFullAnalysis": False, "analysis_path": analysis})) +''' + + +def _write_stub(directory: Path, name: str, source: str) -> None: + path = directory / name + path.write_text(source, encoding="utf-8") + path.chmod(0o755) + + +class MergeBaseContractTests(unittest.TestCase): + """Every test here builds the X → Z / X → P history described above.""" + + def setUp(self) -> None: + self.temp_dir = tempfile.TemporaryDirectory() + root = Path(self.temp_dir.name) + self.upstream = root / "upstream" + self.upstream.mkdir() + self._git("init", "-b", "main") + self._git("config", "user.email", "test@example.com") + self._git("config", "user.name", "Test") + self._git("config", "commit.gpgsign", "false") + + # X — the fork point, the only commit both branches share. + (self.upstream / "shared.txt").write_text("shared\n", encoding="utf-8") + self._git("add", "-A") + self._git("commit", "-m", "shared") + self.fork_point = self._git("rev-parse", "HEAD") + + # P — this pull request's work, branched from X. + self._git("checkout", "-b", "feature") + (self.upstream / "feature.txt").write_text("feature\n", encoding="utf-8") + self._git("add", "-A") + self._git("commit", "-m", "feature") + self.head_sha = self._git("rev-parse", "HEAD") + + # Z — somebody else's commit, landed on main after the branch forked. + self._git("checkout", "main") + (self.upstream / "unrelated.txt").write_text("unrelated\n", encoding="utf-8") + self._git("add", "-A") + self._git("commit", "-m", "unrelated") + self.base_tip = self._git("rev-parse", "HEAD") + + self.bin_dir = root / "bin" + self.bin_dir.mkdir() + _write_stub(self.bin_dir, "gh", GH_STUB) + _write_stub(self.bin_dir, "codeboarding", ENGINE_STUB) + self.gh_log = root / "gh.log" + self.engine_log = root / "engine.log" + self.output = root / "github-output" + self.runner_temp = root / "runner" + self.runner_temp.mkdir() + + def tearDown(self) -> None: + self.temp_dir.cleanup() + + def _git(self, *args: str) -> str: + return subprocess.run( + ["git", "-C", str(self.upstream), *args], + capture_output=True, + text=True, + check=True, + ).stdout.strip() + + def _env(self, **extra: str) -> dict[str, str]: + return { + "PATH": f"{self.bin_dir}:{os.environ['PATH']}", + "GITHUB_OUTPUT": str(self.output), + "RUNNER_TEMP": str(self.runner_temp), + "CB_FIXTURE_REPO": str(self.upstream), + "CB_GH_LOG": str(self.gh_log), + "CB_ENGINE_LOG": str(self.engine_log), + "CB_PR_JSON": "{}", + **extra, + } + + def _outputs(self) -> dict[str, str]: + values: dict[str, str] = {} + for line in self.output.read_text(encoding="utf-8").splitlines(): + key, _, value = line.partition("=") + values[key] = value + return values + + def _run_guard(self, **extra: str) -> subprocess.CompletedProcess: + result = subprocess.run( + [str(GUARD)], + env=self._env( + MODE="review", + EVENT="pull_request", + EVENT_PR_NUMBER="7", + PULL_BASE_SHA=self.base_tip, + PULL_HEAD_SHA=self.head_sha, + PULL_BASE_REPO="owner/repo", + PULL_HEAD_REPO="owner/repo", + PULL_BASE_REF="main", + GITHUB_RUN_ID="1", + GH_HOST="https://github.com", + **extra, + ), + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(result.returncode, 0, result.stderr or result.stdout) + return result + + def test_guard_resolves_the_merge_base_not_the_base_branch_tip(self) -> None: + self._run_guard() + outputs = self._outputs() + + self.assertEqual(outputs["merge_base_sha"], self.fork_point) + self.assertEqual(outputs["base_sha"], self.base_tip) + self.assertNotEqual( + outputs["merge_base_sha"], + outputs["base_sha"], + "the merge base must not collapse onto the base branch tip", + ) + self.assertEqual(outputs["behind_by"], "1") + + def test_review_analyzes_the_merge_base_tree_not_the_base_branch_tip(self) -> None: + checkout = Path(self.temp_dir.name) / "checkout" + subprocess.run( + ["git", "clone", "--quiet", str(self.upstream), str(checkout)], + capture_output=True, + text=True, + check=True, + ) + subprocess.run( + ["git", "-C", str(checkout), "checkout", "--quiet", "--detach", self.head_sha], + capture_output=True, + text=True, + check=True, + ) + + result = subprocess.run( + [str(ANALYZE)], + env=self._env( + ACTION_PATH=str(ROOT), + ANALYSIS_KIND="review", + CHECKOUT_DIR=str(checkout), + REVIEW_BASE_SHA=self.fork_point, + REVIEW_HEAD_SHA=self.head_sha, + REVIEW_BASE_REPO="owner/repo", + GIT_TOKEN="unused", + GITHUB_SERVER_URL="https://github.com", + PR_NUMBER="7", + SEED_MODE="chain", + CACHE_OUT_DIR=str(Path(self.temp_dir.name) / "cache-out"), + ENGINE_VERSION="test", + CFG_HASH="test", + ), + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(result.returncode, 0, result.stderr or result.stdout) + + runs = [json.loads(line) for line in self.engine_log.read_text(encoding="utf-8").splitlines()] + self.assertEqual(len(runs), 2, f"expected a base and a head analysis, got {runs}") + base_run, head_run = runs + + self.assertEqual(base_run["head"], self.fork_point) + self.assertNotEqual(base_run["head"], self.base_tip) + self.assertNotIn( + "unrelated.txt", + base_run["files"], + "the baseline tree must not contain commits this pull request never forked from", + ) + self.assertEqual(head_run["head"], self.head_sha) + self.assertIn("feature.txt", head_run["files"]) + + def test_fork_pull_requests_compare_across_repositories(self) -> None: + self.output.write_text("", encoding="utf-8") + pr_json = json.dumps( + { + "number": 7, + "base": {"sha": self.base_tip, "ref": "main", "repo": {"full_name": "owner/repo"}}, + "head": {"sha": self.head_sha, "repo": {"full_name": "contributor/repo"}}, + } + ) + result = subprocess.run( + [str(GUARD)], + env=self._env( + MODE="review", + EVENT="issue_comment", + COMMENT_BODY="/codeboarding", + AUTHOR_ASSOCIATION="COLLABORATOR", + ISSUE_PR_URL="repos/owner/repo/pulls/7", + GITHUB_RUN_ID="1", + GH_HOST="https://github.com", + CB_PR_JSON=pr_json, + ), + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(result.returncode, 0, result.stderr or result.stdout) + + requested = self.gh_log.read_text(encoding="utf-8").splitlines() + self.assertIn( + f"repos/owner/repo/compare/{self.base_tip}...contributor:{self.head_sha}", + requested, + "a fork's head must be owner-qualified or the comparison silently falls back", + ) + self.assertEqual(self._outputs()["merge_base_sha"], self.fork_point) + + def test_unresolvable_merge_base_falls_back_to_the_event_base(self) -> None: + _write_stub(self.bin_dir, "gh", "#!/bin/sh\nexit 1\n") + self._run_guard() + outputs = self._outputs() + + self.assertEqual(outputs["merge_base_sha"], self.base_tip) + self.assertEqual(outputs["behind_by"], "0") + + +if __name__ == "__main__": + unittest.main() From 6771689222fced3e5626c3b5b16c83d120d7b84f Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Mon, 17 Aug 2026 20:32:12 +0200 Subject: [PATCH 02/12] fix(review): make the analysis caches reachable and the fallback visible Four defects found in review of the previous commit. actions/cache derives its lookup version from the literal path strings, so saving state under .../cb-cache/out/chain could never be restored from .../cb-cache/chain. Every run would have missed and silently redone the full analysis while appearing to be wired up. Saves and restores now use one path per cache, and a test parses action.yml to keep it that way. When the compare API could not be reached, the review fell back to the base branch tip and said so only in the log, quietly producing the comparison this feature exists to avoid. The call is now retried once, and an unresolved merge base is stated in the review comment and recorded in the artifact metadata. The cache identity ignored model selection, so changing model, agent_model, parsing_model, or the provider reused an analysis produced by the previous one and applied the new model only to the delta. Those inputs are now part of the identity. /codeboarding refresh and /codeboarding full recomputed the analysis and then saved it under the key already holding the state they were asked to discard. Cache entries are immutable, so the save was dropped and the next run restored the pre-refresh analysis. Forced runs now save under their own generation key, still found by the same prefix. Co-Authored-By: Claude Opus 5 (1M context) --- action.yml | 17 ++++--- scripts/action/build-review-artifact.sh | 4 +- scripts/action/build-review-comment.sh | 6 ++- scripts/action/cache-keys.sh | 16 +++++-- scripts/action/guard.sh | 26 ++++++++--- tests/test_action_cache.py | 60 +++++++++++++++++++++++++ tests/test_action_sync.py | 2 + 7 files changed, 114 insertions(+), 17 deletions(-) diff --git a/action.yml b/action.yml index 9e7f7fa..1b7fda6 100644 --- a/action.yml +++ b/action.yml @@ -185,6 +185,11 @@ runs: HEAD_SHA: ${{ steps.guard.outputs.head_sha }} HEAD_REPO: ${{ steps.guard.outputs.head_repo }} IS_FORK: ${{ steps.guard.outputs.is_fork }} + SEED_MODE: ${{ steps.guard.outputs.seed_mode }} + LLM_PROVIDER: ${{ inputs.llm_provider }} + MODEL: ${{ inputs.model }} + AGENT_MODEL_INPUT: ${{ inputs.agent_model }} + PARSING_MODEL_INPUT: ${{ inputs.parsing_model }} run: "$GITHUB_ACTION_PATH/scripts/action/cache-keys.sh" # An exact key is this merge base's own analysis; the prefix falls back to @@ -216,7 +221,7 @@ runs: ACTION_PATH: ${{ github.action_path }} ANALYSIS_KIND: sync CHECKOUT_DIR: ${{ github.workspace }}/.codeboarding-target - CACHE_OUT_DIR: ${{ runner.temp }}/cb-cache/out + CACHE_OUT_DIR: ${{ runner.temp }}/cb-cache FORCE_FULL: ${{ inputs.force_full }} MODEL: ${{ inputs.model }} AGENT_MODEL_INPUT: ${{ inputs.agent_model }} @@ -249,7 +254,7 @@ runs: continue-on-error: true uses: actions/cache/save@v4 with: - path: ${{ runner.temp }}/cb-cache/out/base + path: ${{ runner.temp }}/cb-cache/base key: ${{ steps.cache_keys.outputs.base_key_prefix }}${{ steps.sync_commit.outputs.baseline_sha }} - name: Write sync summary @@ -279,7 +284,7 @@ runs: CACHE_BASE_DIR: ${{ runner.temp }}/cb-cache/base CACHE_BASE_HIT: ${{ steps.base_cache.outputs.cache-hit }} CACHE_CHAIN_DIR: ${{ runner.temp }}/cb-cache/chain - CACHE_OUT_DIR: ${{ runner.temp }}/cb-cache/out + CACHE_OUT_DIR: ${{ runner.temp }}/cb-cache ENGINE_VERSION: ${{ steps.cache_keys.outputs.engine_version }} CFG_HASH: ${{ steps.cache_keys.outputs.cfg_hash }} GIT_TOKEN: ${{ inputs.github_token }} @@ -296,7 +301,7 @@ runs: continue-on-error: true uses: actions/cache/save@v4 with: - path: ${{ runner.temp }}/cb-cache/out/chain + path: ${{ runner.temp }}/cb-cache/chain key: ${{ steps.cache_keys.outputs.chain_key }} # Only trusted runs publish a base entry: it is restorable repository-wide, @@ -306,7 +311,7 @@ runs: continue-on-error: true uses: actions/cache/save@v4 with: - path: ${{ runner.temp }}/cb-cache/out/base + path: ${{ runner.temp }}/cb-cache/base key: ${{ steps.cache_keys.outputs.base_key }} - name: Render review diagram @@ -328,6 +333,7 @@ runs: ANALYSIS_MODE: ${{ steps.review_analyze.outputs.analysis_mode }} BASE_SHA: ${{ steps.guard.outputs.base_sha }} MERGE_BASE_SHA: ${{ steps.guard.outputs.merge_base_sha }} + MERGE_BASE_RESOLVED: ${{ steps.guard.outputs.merge_base_resolved }} HEAD_SHA: ${{ steps.guard.outputs.head_sha }} PR_NUMBER: ${{ steps.guard.outputs.pr_number }} SEED_SOURCE: ${{ steps.review_analyze.outputs.seed_source }} @@ -354,6 +360,7 @@ runs: PR_NUMBER: ${{ steps.guard.outputs.pr_number }} BEHIND_BY: ${{ steps.guard.outputs.behind_by }} BASE_REF: ${{ steps.guard.outputs.base_ref }} + MERGE_BASE_RESOLVED: ${{ steps.guard.outputs.merge_base_resolved }} run: "$GITHUB_ACTION_PATH/scripts/action/build-review-comment.sh" - name: Post review comment diff --git a/scripts/action/build-review-artifact.sh b/scripts/action/build-review-artifact.sh index e136f3d..47619e1 100755 --- a/scripts/action/build-review-artifact.sh +++ b/scripts/action/build-review-artifact.sh @@ -9,11 +9,13 @@ jq -n \ --arg mode "$ANALYSIS_MODE" \ --arg base_sha "$BASE_SHA" \ --arg merge_base_sha "$MERGE_BASE_SHA" \ + --arg merge_base_resolved "$MERGE_BASE_RESOLVED" \ --arg head_sha "$HEAD_SHA" \ --arg pr_number "$PR_NUMBER" \ --arg seed_source "$SEED_SOURCE" \ --arg chain_depth "$CHAIN_DEPTH" \ - '{mode: $mode, base_sha: $base_sha, merge_base_sha: $merge_base_sha, head_sha: $head_sha, + '{mode: $mode, base_sha: $base_sha, merge_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}' \ > "${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 17794f5..d28d0e2 100755 --- a/scripts/action/build-review-comment.sh +++ b/scripts/action/build-review-comment.sh @@ -15,7 +15,11 @@ printf '\nSee the full change in [CodeBoarding](%s).\n' "$WEBVIEW_URL" >> "$BODY # The diagram compares against the merge base, so commits landed on the base # branch since this PR forked are excluded. Say so rather than hide it. BEHIND="${BEHIND_BY:-0}" -if [ "$BEHIND" -gt 0 ] 2>/dev/null; then +if [ "${MERGE_BASE_RESOLVED:-true}" != true ]; then + # shellcheck disable=SC2016 # the backticks are Markdown, not a command + printf '\n> [!WARNING]\n> The merge base could not be resolved, so this compares against the tip of `%s`. Changes made on `%s` since this branch forked may appear here as this pull request'\''s changes.\n' \ + "${BASE_REF:-the base branch}" "${BASE_REF:-the base branch}" >> "$BODY" +elif [ "$BEHIND" -gt 0 ] 2>/dev/null; then COMMIT_NOUN="commits" [ "$BEHIND" != "1" ] || COMMIT_NOUN="commit" # shellcheck disable=SC2016 # the backticks are Markdown, not a command diff --git a/scripts/action/cache-keys.sh b/scripts/action/cache-keys.sh index 76c9be4..ed9d060 100755 --- a/scripts/action/cache-keys.sh +++ b/scripts/action/cache-keys.sh @@ -25,12 +25,16 @@ if [ -z "$engine_version" ]; then exit 0 fi -# The key pins the engine and the analysis scope. Depth needs no hash: it is +# The key pins everything that decides what an analysis says: the engine, the +# analysis scope, and the models that produced it. Depth needs no hash: it is # read from the baseline at the merge base, which the key already pins. ignore_digest=none ignore_file="$CHECKOUT_DIR/.codeboarding/.codeboardingignore" [ ! -f "$ignore_file" ] || ignore_digest="$(digest < "$ignore_file")" -cfg_hash="$(printf '%s\n%s\n%s\n' "$CACHE_SCHEMA" "$engine_version" "$ignore_digest" | digest)" +model_digest="$(printf '%s\n%s\n%s\n%s\n' \ + "${LLM_PROVIDER:-}" "${MODEL:-}" "${AGENT_MODEL_INPUT:-}" "${PARSING_MODEL_INPUT:-}" | digest)" +cfg_hash="$(printf '%s\n%s\n%s\n%s\n' \ + "$CACHE_SCHEMA" "$engine_version" "$ignore_digest" "$model_digest" | digest)" base_key_prefix="cb-base-$CACHE_SCHEMA-$cfg_hash-" { @@ -52,7 +56,13 @@ if [ "${IS_FORK:-false}" = true ]; then else chain_prefix="cb-head-$CACHE_SCHEMA-$cfg_hash-pr$PR_NUMBER-mb$MERGE_BASE_SHA-" fi +chain_key="$chain_prefix$HEAD_SHA" +# Cache entries are immutable, so a run asked to discard the previous analysis +# must not save under the key holding it: the save would be dropped and the next +# run would restore exactly the state the refresh existed to replace. +[ "${SEED_MODE:-chain}" = chain ] || \ + chain_key="$chain_key-$SEED_MODE${GITHUB_RUN_ID:-0}.${GITHUB_RUN_ATTEMPT:-1}" { - echo "chain_key=$chain_prefix$HEAD_SHA" + echo "chain_key=$chain_key" echo "chain_restore_keys=$chain_prefix" } >> "$GITHUB_OUTPUT" diff --git a/scripts/action/guard.sh b/scripts/action/guard.sh index 3ff2f6d..5b48ddc 100755 --- a/scripts/action/guard.sh +++ b/scripts/action/guard.sh @@ -89,12 +89,19 @@ merge_base_sha="$base_sha" behind_by=0 basehead="$base_sha...$head_sha" [ "$head_repo" = "$base_repo" ] || basehead="$base_sha...${head_repo%%/*}:$head_sha" -compare="$(gh api "repos/$base_repo/compare/$basehead" \ - --jq '[.merge_base_commit.sha // "", .behind_by // 0] | @tsv' 2>/dev/null || true)" -# Split on the tab explicitly. Field splitting would drop an empty merge base -# and shift the distance into its place, which reads as a valid commit. -compare_merge_base="${compare%%$'\t'*}" -compare_behind="${compare#*$'\t'}" +merge_base_resolved=false +for attempt in 1 2; do + compare="$(gh api "repos/$base_repo/compare/$basehead" \ + --jq '[.merge_base_commit.sha // "", .behind_by // 0] | @tsv' 2>/dev/null || true)" + # Split on the tab explicitly. Field splitting would drop an empty merge base + # and shift the distance into its place, which reads as a valid commit. + compare_merge_base="${compare%%$'\t'*}" + compare_behind="${compare#*$'\t'}" + # A transient failure of one API call should not decide how this pull request + # is measured, so try once more before giving up on the merge base. + [ -z "$compare_merge_base" ] && [ "$attempt" = 1 ] || break + sleep 2 +done case "$compare_behind" in ''|*[!0-9]*) compare_behind=0 ;; esac @@ -102,10 +109,14 @@ esac # against, so take both or neither: reporting one while comparing against the # tip would describe a comparison this run did not make. if [ -n "$compare_merge_base" ]; then + merge_base_resolved=true merge_base_sha="$compare_merge_base" behind_by="$compare_behind" else - echo "::notice::Could not resolve the merge base; comparing against the base branch tip instead." + # Falling back keeps reviews working through an API outage, but the result is + # the comparison this change exists to avoid, so it is stated in the comment + # rather than left in the log. + echo "::warning::Could not resolve the merge base; comparing against the tip of $base_ref instead." fi comment_id=codeboarding-review @@ -116,6 +127,7 @@ is_fork=false echo "pr_number=$pr_number" echo "base_sha=$base_sha" echo "merge_base_sha=$merge_base_sha" + echo "merge_base_resolved=$merge_base_resolved" echo "behind_by=$behind_by" echo "base_ref=$base_ref" echo "head_sha=$head_sha" diff --git a/tests/test_action_cache.py b/tests/test_action_cache.py index bdf773b..16890be 100644 --- a/tests/test_action_cache.py +++ b/tests/test_action_cache.py @@ -70,6 +70,13 @@ def _run(self, **extra: str) -> dict[str, str]: "HEAD_SHA": "head-sha", "HEAD_REPO": "owner/repo", "IS_FORK": "false", + "SEED_MODE": "chain", + "LLM_PROVIDER": "openrouter", + "MODEL": "", + "AGENT_MODEL_INPUT": "", + "PARSING_MODEL_INPUT": "", + "GITHUB_RUN_ID": "99", + "GITHUB_RUN_ATTEMPT": "1", **extra, }, capture_output=True, @@ -110,6 +117,26 @@ def test_analysis_scope_and_engine_version_change_the_identity(self) -> None: (self.checkout / ".codeboarding" / ".codeboardingignore").write_text("docs/\n", encoding="utf-8") self.assertNotEqual(baseline["cfg_hash"], self._run()["cfg_hash"]) + def test_model_selection_changes_the_identity(self) -> None: + baseline = self._run() + + self.assertNotEqual(baseline["cfg_hash"], self._run(MODEL="gpt-5")["cfg_hash"]) + self.assertNotEqual(baseline["cfg_hash"], self._run(AGENT_MODEL_INPUT="gpt-5")["cfg_hash"]) + self.assertNotEqual(baseline["cfg_hash"], self._run(PARSING_MODEL_INPUT="gpt-5")["cfg_hash"]) + self.assertNotEqual(baseline["cfg_hash"], self._run(LLM_PROVIDER="anthropic")["cfg_hash"]) + + def test_a_forced_refresh_does_not_save_under_the_key_it_replaces(self) -> None: + chained = self._run() + refreshed = self._run(SEED_MODE="refresh") + full = self._run(SEED_MODE="full") + + self.assertNotEqual(chained["chain_key"], refreshed["chain_key"]) + self.assertNotEqual(refreshed["chain_key"], full["chain_key"]) + # Still found by the prefix, so the next run picks the newest state. + for values in (refreshed, full): + self.assertTrue(values["chain_key"].startswith(values["chain_restore_keys"])) + self.assertEqual(values["chain_restore_keys"], chained["chain_restore_keys"]) + def test_unresolvable_engine_version_disables_caching_instead_of_failing(self) -> None: stub_bin = self.root / "bin" stub_bin.mkdir() @@ -249,5 +276,38 @@ def test_analysis_is_staged_for_the_cache(self) -> None: self.assertFalse((self.cache_out / "base").exists(), "a cached base needs no re-save") +class CachePathParityTests(unittest.TestCase): + """actions/cache derives its lookup version from the path strings, so a save + under a different path than the restore can never be found again.""" + + def _cache_steps(self) -> list[dict[str, str]]: + steps: list[dict[str, str]] = [] + current: dict[str, str] | None = None + for line in (ROOT / "action.yml").read_text(encoding="utf-8").splitlines(): + if line.startswith(" - name:"): + current = {"name": line.split(":", 1)[1].strip()} + steps.append(current) + elif current is not None: + stripped = line.strip() + for field in ("uses", "path", "key"): + if stripped.startswith(f"{field}:"): + current[field] = stripped.split(":", 1)[1].strip() + return [step for step in steps if step.get("uses", "").startswith("actions/cache/")] + + def test_every_saved_path_is_a_restored_path(self) -> None: + steps = self._cache_steps() + self.assertTrue(steps, "no cache steps found in action.yml") + restored = {s["path"] for s in steps if s["uses"].startswith("actions/cache/restore")} + saved = {s["path"] for s in steps if s["uses"].startswith("actions/cache/save")} + + self.assertTrue(restored, "no cache restore steps found") + self.assertTrue(saved, "no cache save steps found") + self.assertEqual( + saved - restored, + set(), + "these paths are saved but never restored, so the entries are unreachable", + ) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_action_sync.py b/tests/test_action_sync.py index 822886c..f192b80 100644 --- a/tests/test_action_sync.py +++ b/tests/test_action_sync.py @@ -101,6 +101,8 @@ def test_review_guard_ignores_a_distance_it_cannot_anchor(self) -> None: values = output.read_text(encoding="utf-8") self.assertIn("merge_base_sha=base-sha\n", values) self.assertIn("behind_by=0\n", values) + # The fallback comparison is announced, not silently substituted. + self.assertIn("merge_base_resolved=false\n", values) def test_review_guard_accepts_trusted_fork_command(self) -> None: with tempfile.TemporaryDirectory() as tmp: From ad7a1db79b1f20b11b8851622f3c91c0bc7a2e8b Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Mon, 17 Aug 2026 20:56:48 +0200 Subject: [PATCH 03/12] fix(review): restore the pull request chain by prefix Giving forced refreshes their own generation key was not enough. The restore still asked for the plain head-sha key first, and an exact match outranks every prefix match, so the next run at that head returned the entry the refresh had replaced and the refresh was undone again. What a run needs is this pull request's newest analysis, which is what the prefix already selects, so the head-sha key is only there to keep saves unique. The restore now looks up by prefix alone. The base cache keeps its exact lookup, where an exact hit means the merge base's own analysis and a prefix hit is only a warm seed; a test pins both intents. Co-Authored-By: Claude Opus 5 (1M context) --- action.yml | 6 +++++- tests/test_action_cache.py | 15 ++++++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/action.yml b/action.yml index 1b7fda6..770862f 100644 --- a/action.yml +++ b/action.yml @@ -204,13 +204,17 @@ runs: key: ${{ steps.cache_keys.outputs.base_key }} restore-keys: ${{ steps.cache_keys.outputs.base_restore_keys }} + # Looked up by prefix alone, because what a run must continue from is this + # pull request's newest analysis. An exact head-sha key would outrank a + # newer generation saved for the same head, so the entry a forced refresh + # replaced would win and undo the refresh. - name: Restore pull request analysis if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.guard.outputs.seed_mode == 'chain' && steps.cache_keys.outputs.chain_key != '' continue-on-error: true uses: actions/cache/restore@v4 with: path: ${{ runner.temp }}/cb-cache/chain - key: ${{ steps.cache_keys.outputs.chain_key }} + key: ${{ steps.cache_keys.outputs.chain_restore_keys }} restore-keys: ${{ steps.cache_keys.outputs.chain_restore_keys }} - name: Analyze baseline diff --git a/tests/test_action_cache.py b/tests/test_action_cache.py index 16890be..41ff113 100644 --- a/tests/test_action_cache.py +++ b/tests/test_action_cache.py @@ -289,11 +289,24 @@ def _cache_steps(self) -> list[dict[str, str]]: steps.append(current) elif current is not None: stripped = line.strip() - for field in ("uses", "path", "key"): + for field in ("uses", "path", "key", "restore-keys"): if stripped.startswith(f"{field}:"): current[field] = stripped.split(":", 1)[1].strip() return [step for step in steps if step.get("uses", "").startswith("actions/cache/")] + def test_the_chain_is_restored_by_prefix_so_a_refresh_survives(self) -> None: + steps = {step["name"]: step for step in self._cache_steps()} + chain = steps["Restore pull request analysis"] + base = steps["Restore base analysis"] + + # An exact key outranks every prefix match, so a head-sha key would + # return the entry a forced refresh replaced rather than its + # replacement, which is saved under a later generation of the same head. + self.assertEqual(chain["key"], chain["restore-keys"]) + # The base lookup relies on the opposite: an exact hit is this merge + # base's own analysis, a prefix hit is only a warm seed. + self.assertNotEqual(base["key"], base["restore-keys"]) + def test_every_saved_path_is_a_restored_path(self) -> None: steps = self._cache_steps() self.assertTrue(steps, "no cache steps found in action.yml") From d7c7f8b4ad1818af2e1df61152228f62a36a19c3 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Tue, 18 Aug 2026 21:08:56 +0200 Subject: [PATCH 04/12] docs: describe the merge_base_sha output in plain terms The old wording trailed off ("Commit the review compared the pull request head against") and read as a fragment in the Actions marketplace listing, where output descriptions are surfaced verbatim. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 2 +- action.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index c584fb0..e973e62 100644 --- a/README.md +++ b/README.md @@ -212,7 +212,7 @@ The `/codeboarding` command, comment heading, Mermaid direction (`LR`), hosted w | `truncated` | review | Whether the graph was reduced to fit GitHub limits. | | `review_artifact_url` | review | URL of the uploaded head analysis. | | `seed_source` | review | `pr-chain` when the head grew from this PR's previous analysis, `base` otherwise. | -| `merge_base_sha` | review | Commit the head was compared against. | +| `merge_base_sha` | review | Merge base used as the comparison baseline. | | `analysis_mode` | sync | `incremental` or `full`. | | `files_written` | sync | Number of persisted analysis artifacts produced. | | `committed` | sync | Whether a baseline commit was delivered. | diff --git a/action.yml b/action.yml index 770862f..2195a64 100644 --- a/action.yml +++ b/action.yml @@ -66,7 +66,7 @@ outputs: description: 'Which state the review head analysis grew from: pr-chain or base.' value: ${{ steps.review_analyze.outputs.seed_source }} merge_base_sha: - description: 'Commit the review compared the pull request head against.' + description: 'Merge base used as the review comparison baseline.' value: ${{ steps.guard.outputs.merge_base_sha }} analysis_mode: description: 'Whether sync used incremental or full analysis.' From 87915ed4411f038d7df62ea1cff2e03d1d6c2e1c Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Tue, 18 Aug 2026 21:22:45 +0200 Subject: [PATCH 05/12] feat(review): ship the merge-base analysis in the review artifact The artifact carried only the head graph, so a reader could not reproduce the comparison without resolving the merge base itself. The nearest thing available to it, the baseline committed on the default branch, is the branch tip rather than the merge base, which would drift from what the review actually measured and reintroduce the very bug this branch removes, one layer up. The artifact is also the only channel a reader outside the run has: the Actions cache has no download API, so state kept there can never serve the webview. base_analysis.json now sits beside analysis.json, and metadata.json already names the commit it belongs to. Copying it costs no analysis: the base graph is either restored from cache or produced by a catch-up that finds nothing changed. Retention drops to 30 days, since each run now carries its own copy of the base graph and only a pull request's latest artifact is ever read. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 12 +++++- action.yml | 4 ++ docs/COMMIT_STRATEGY.md | 4 +- scripts/action/build-review-artifact.sh | 7 +++- tests/test_action_cache.py | 53 +++++++++++++++++++++++++ 5 files changed, 77 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index e973e62..6bd6833 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ One GitHub Action with two modes: -- **`review`** (default) compares a pull request's head with its merge base, posts an inline Mermaid architecture diff, and uploads the head `analysis.json` as a workflow artifact. +- **`review`** (default) compares a pull request's head with its merge base, posts an inline Mermaid architecture diff, and uploads both analyses as a workflow artifact. - **`sync`** updates the versioned analysis state used by future incremental runs. It can push directly or open one rolling PR for protected branches. The action is a thin wrapper around the [CodeBoarding](https://github.com/CodeBoarding/CodeBoarding) CLI. Analysis logic and provider defaults live in Core, not in this repository. @@ -54,6 +54,16 @@ The action checks out and analyzes the exact PR head SHA, and compares it with t Automatic fork runs are skipped because the `pull_request` event does not receive hosted OIDC credentials. A trusted `/codeboarding` command runs the released action code from the base repository and checks the fork's source into a separate analysis directory; it never executes an action definition from the fork with privileged credentials. +The uploaded artifact holds both sides of the comparison, so a reader can reproduce it without resolving the merge base again: + +| File | Contents | +|---|---| +| `analysis.json` | the head analysis, at `head_sha` | +| `base_analysis.json` | the analysis it was compared against, at `merge_base_sha` | +| `metadata.json` | both SHAs, `merge_base_resolved`, `seed_source`, `chain_depth`, PR number | + +Artifacts are kept 30 days. Reading the default branch's committed baseline instead of `base_analysis.json` would drift from the merge base in exactly the way described above. + ### Reused analysis Each review seeds the head analysis from this pull request's own previous run, so a run only covers the commits pushed since it. With no previous run, it seeds from the merge base's analysis, which `sync` mode publishes and every pull request in the repository shares. Both live in the GitHub Actions cache, and state is re-derived from the merge base whenever the pinned CodeBoarding version, `.codeboardingignore`, the configured analysis depth, or the merge base itself changes. State produced while analyzing a fork is namespaced separately and is never restored by a run on this repository's own code. diff --git a/action.yml b/action.yml index 2195a64..2a24dd2 100644 --- a/action.yml +++ b/action.yml @@ -334,6 +334,7 @@ runs: shell: bash env: ANALYSIS_PATH: ${{ steps.review_analyze.outputs.analysis_path }} + BASE_ANALYSIS_PATH: ${{ steps.review_analyze.outputs.base_analysis_path }} ANALYSIS_MODE: ${{ steps.review_analyze.outputs.analysis_mode }} BASE_SHA: ${{ steps.guard.outputs.base_sha }} MERGE_BASE_SHA: ${{ steps.guard.outputs.merge_base_sha }} @@ -352,6 +353,9 @@ runs: name: codeboarding-review-${{ github.run_id }}-${{ github.run_attempt }} path: ${{ steps.review_artifact.outputs.artifact_dir }} if-no-files-found: error + # Each run carries its own copy of the base graph, and the webview only + # ever reads a pull request's latest artifact, so value drops off fast. + retention-days: 30 - name: Build review comment id: review_body diff --git a/docs/COMMIT_STRATEGY.md b/docs/COMMIT_STRATEGY.md index 7864478..cc35e25 100644 --- a/docs/COMMIT_STRATEGY.md +++ b/docs/COMMIT_STRATEGY.md @@ -22,7 +22,9 @@ The engine writes these under `.codeboarding/`: - ✅ `static_analysis.pkl` + `static_analysis.sha` — required for reliable warm-start incremental sync from the committed baseline. **Upload in review mode:** -- ✅ PR-head `analysis.json` and metadata containing the PR base SHA, the merge base actually compared against, and how the head was seeded — stored as a GitHub Actions artifact. +- ✅ Both sides of the comparison — the PR-head `analysis.json` and the `base_analysis.json` it was measured against — plus metadata naming the base tip, the merge base, and how the head was seeded. Stored as a GitHub Actions artifact, kept 30 days. + +The artifact is the only one of these channels a reader outside the workflow can use: the Actions cache has no download API, so anything the webview needs has to ship here. Carrying the base graph per run duplicates a text file, but it keeps each artifact self-contained; the alternative is a reader walking back through older artifacts for a base that may already have expired. **Cache between runs (never committed):** - ✅ The whole analysis state directory, twice: once per pull request (its head state, so the next run only covers new commits) and once per base commit (the merge base's analysis, shared by every pull request that forked from it). Sync mode publishes the base entry as a by-product of the baseline it already computes. diff --git a/scripts/action/build-review-artifact.sh b/scripts/action/build-review-artifact.sh index 47619e1..7a64cd2 100755 --- a/scripts/action/build-review-artifact.sh +++ b/scripts/action/build-review-artifact.sh @@ -1,8 +1,13 @@ #!/usr/bin/env bash -# Packages head analysis and PR metadata, then outputs the artifact upload directory. +# Packages both analyses and PR metadata, then outputs the artifact upload directory. set -euo pipefail mkdir -p "${RUNNER_TEMP}/cb-review-artifact" cp "$ANALYSIS_PATH" "${RUNNER_TEMP}/cb-review-artifact/analysis.json" +# Ship the graph the diagram was measured against, so a reader can reproduce the +# comparison without guessing which commit it belongs to. Reading the default +# branch's committed baseline instead would drift from the merge base exactly as +# the review itself used to. +cp "$BASE_ANALYSIS_PATH" "${RUNNER_TEMP}/cb-review-artifact/base_analysis.json" # base_sha stays the event's base branch tip for consumers that key on it; # merge_base_sha records the commit the diagram actually compared against. jq -n \ diff --git a/tests/test_action_cache.py b/tests/test_action_cache.py index 41ff113..13809fc 100644 --- a/tests/test_action_cache.py +++ b/tests/test_action_cache.py @@ -276,6 +276,59 @@ def test_analysis_is_staged_for_the_cache(self) -> None: self.assertFalse((self.cache_out / "base").exists(), "a cached base needs no re-save") +class ReviewArtifactTests(unittest.TestCase): + """The artifact is the only channel a reader outside the run can use: cache + entries have no download API, so whatever the webview needs must ship here.""" + + def setUp(self) -> None: + self.temp_dir = tempfile.TemporaryDirectory() + self.root = Path(self.temp_dir.name) + self.head = self.root / "head.json" + self.head.write_text('{"components": ["head"]}', encoding="utf-8") + self.base = self.root / "base.json" + self.base.write_text('{"components": ["base"]}', encoding="utf-8") + + def tearDown(self) -> None: + self.temp_dir.cleanup() + + def test_it_ships_both_graphs_and_the_commit_they_describe(self) -> None: + output = self.root / "github-output" + result = subprocess.run( + [str(ROOT / "scripts" / "action" / "build-review-artifact.sh")], + env={ + "PATH": os.environ["PATH"], + "RUNNER_TEMP": str(self.root), + "GITHUB_OUTPUT": str(output), + "ANALYSIS_PATH": str(self.head), + "BASE_ANALYSIS_PATH": str(self.base), + "ANALYSIS_MODE": "incremental", + "BASE_SHA": "tip-sha", + "MERGE_BASE_SHA": "merge-base-sha", + "MERGE_BASE_RESOLVED": "true", + "HEAD_SHA": "head-sha", + "PR_NUMBER": "81", + "SEED_SOURCE": "pr-chain", + "CHAIN_DEPTH": "2", + }, + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(result.returncode, 0, result.stderr or result.stdout) + + artifact = self.root / "cb-review-artifact" + self.assertEqual(json.loads((artifact / "analysis.json").read_text())["components"], ["head"]) + self.assertEqual(json.loads((artifact / "base_analysis.json").read_text())["components"], ["base"]) + + metadata = json.loads((artifact / "metadata.json").read_text(encoding="utf-8")) + # base_sha stays the event tip for consumers keyed on it; the merge base + # is what base_analysis.json actually describes. + self.assertEqual(metadata["base_sha"], "tip-sha") + self.assertEqual(metadata["merge_base_sha"], "merge-base-sha") + self.assertEqual(metadata["merge_base_resolved"], "true") + self.assertEqual(metadata["seed_source"], "pr-chain") + + class CachePathParityTests(unittest.TestCase): """actions/cache derives its lookup version from the path strings, so a save under a different path than the restore can never be found again.""" From 19b25e672d22022a5b242c734af155c3ef88f6b7 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Tue, 18 Aug 2026 21:35:08 +0200 Subject: [PATCH 06/12] docs: write down the artifact and cache naming The key shapes existed only in cache-keys.sh, so nothing explained why an artifact is named for its run while a cache key carries the pull request, the merge base and the config digest: one is scoped to a run and found by listing, the other is shared across runs and found by name. Co-Authored-By: Claude Opus 5 (1M context) --- docs/COMMIT_STRATEGY.md | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/docs/COMMIT_STRATEGY.md b/docs/COMMIT_STRATEGY.md index cc35e25..ef36f28 100644 --- a/docs/COMMIT_STRATEGY.md +++ b/docs/COMMIT_STRATEGY.md @@ -63,6 +63,40 @@ Either way the head analysis is seeded from that directory and runs incrementall | `static_analysis.sha` | ✅ | with `static_analysis.pkl` | warm-start gate | | whole state directory | ❌ | Actions cache, per PR and per base commit | reuse the previous run instead of re-deriving from the base | +## Names + +The two stores have different lifetimes, so they are named for different jobs. + +**Artifact — one per workflow run, immutable once written.** GitHub scopes an +artifact to its run and offers no way to append to or replace an earlier one, so +the name only has to be unique, and a consumer finds it by listing a pull +request's artifacts and taking the newest: + + codeboarding-review--/ + analysis.json the head analysis, at metadata.head_sha + base_analysis.json what it was compared against, at metadata.merge_base_sha + metadata.json both SHAs, merge_base_resolved, seed_source, chain_depth + +**Cache — shared across runs, found by key.** Here the name *is* the lookup, so +it carries everything that decides whether prior state still applies: + +| Key | Scope | The question it answers | +|---|---|---| +| `cb-head-v1--pr-mb-` | one pull request | what did this pull request's last run produce | +| `cb-base-v1--` | one commit | what does this commit's architecture look like | +| `cb-fork-v1---pr-mb-` | one fork pull request | the same as `cb-head`, quarantined | + +`` digests the cache schema version, the pinned CodeBoarding version, +`.codeboardingignore`, and the model selection. A run forced with +`/codeboarding refresh` or `full` appends `-.`, because +cache entries are immutable and it must not save under the key holding the state +it was told to discard. + +The chain is restored **by prefix**, which selects the newest entry for that +pull request. The base is restored **by exact key first**: an exact hit is that +merge base's own analysis and needs no engine run, while a prefix hit is only +another commit's baseline, usable as a warm seed for the catch-up. + ## Cache identity The cache key pins everything that makes prior state meaningful: the pinned From 9eb2c349c629c4b873f198d4b8d2a27b308ffbfd Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Tue, 18 Aug 2026 21:53:43 +0200 Subject: [PATCH 07/12] docs: correct and simplify the storage strategy The document still described a v1 world: it claimed sync commits health_report.json, which install-sync.sh in fact deletes; it omitted fingerprint.json, which sync does commit and which drives change detection; and it explained a pkl-seeding fallback through engine_adapter.py, a script the v2 flow no longer has. It also split the same material across a decision list, a summary table and a "now vs later" section. Rewritten around the three stores and what each is for, with the part that kept being asked in review added: how a review picks its base and head graphs, what each costs, and what changes between a pull request's first run and every run after it. Co-Authored-By: Claude Opus 5 (1M context) --- docs/COMMIT_STRATEGY.md | 165 ++++++++++++++++++++++------------------ 1 file changed, 91 insertions(+), 74 deletions(-) diff --git a/docs/COMMIT_STRATEGY.md b/docs/COMMIT_STRATEGY.md index ef36f28..216cf5c 100644 --- a/docs/COMMIT_STRATEGY.md +++ b/docs/COMMIT_STRATEGY.md @@ -1,111 +1,128 @@ -# Baseline & artifact commit strategy +# Baseline, cache, and artifact strategy -What CodeBoarding writes into a repo, what sync mode commits, and what review -mode stores as workflow artifacts. +Where CodeBoarding keeps analysis state, and how a review run finds the two +graphs it compares. -## The artifacts +## Three stores -The engine writes these under `.codeboarding/`: - -| File | Type | Size | Purpose | +| Store | Holds | Lifetime | Who can read it | |---|---|---|---| -| `analysis.json` | JSON (text) | KB–low MB | The component graph — **the diagram source** | -| `health/health_report.json` | JSON (text) | KB | Health findings → **the warnings** | -| `static_analysis.pkl` | binary pickle | MB-scale | LSP/CFG cache → **warm-start** (re-LSP only changed files) | -| `static_analysis.sha` | text (1 line) | bytes | Tag recording the pkl's commit → the warm-start gate | - -## Decision - -**Commit in sync mode:** -- ✅ `analysis.json` — required for the extension (and later the webview) to **show the diagram instantly without regenerating** — i.e. without spending the user's API key. It's text and diffs meaningfully. -- ✅ `health/health_report.json` — required for warnings in the extension/webview. Small text. -- ✅ `static_analysis.pkl` + `static_analysis.sha` — required for reliable warm-start incremental sync from the committed baseline. +| **Git** (`sync` mode commits) | `.codeboarding/` on the target branch | forever | anyone — extension, webview, humans | +| **Actions cache** | whole analysis state dirs, pickle included | 7-day idle, 10 GB LRU | only a workflow run | +| **Workflow artifact** | the two graphs a review compared, plus metadata | 30 days | anyone with repo read, via the API | -**Upload in review mode:** -- ✅ Both sides of the comparison — the PR-head `analysis.json` and the `base_analysis.json` it was measured against — plus metadata naming the base tip, the merge base, and how the head was seeded. Stored as a GitHub Actions artifact, kept 30 days. +The split matters: the cache is a run's **input** (warm start) and has no download +API, so nothing outside a run can ever read it. The artifact is a run's +**output**, and is the only channel the webview has. -The artifact is the only one of these channels a reader outside the workflow can use: the Actions cache has no download API, so anything the webview needs has to ship here. Carrying the base graph per run duplicates a text file, but it keeps each artifact self-contained; the alternative is a reader walking back through older artifacts for a base that may already have expired. +## What sync commits -**Cache between runs (never committed):** -- ✅ The whole analysis state directory, twice: once per pull request (its head state, so the next run only covers new commits) and once per base commit (the merge base's analysis, shared by every pull request that forked from it). Sync mode publishes the base entry as a by-product of the baseline it already computes. +Sync is the only git writer. Review mode never commits to a contributor's +branch, so generated files cannot conflict during a merge. -> **Principle:** sync mode is the only git writer. Review mode never commits generated files to PR branches, so generated artifacts cannot conflict with `main` during merge. +| File | Why it is committed | +|---|---| +| `analysis.json` | the diagram, shown instantly with no API key | +| `fingerprint.json` | whole-tree file hashes — how incremental detects change | +| `static_analysis.pkl` | LSP/CFG cache and the cluster baseline incremental needs | +| `static_analysis.sha` | the warm-start gate for the pickle | +| `codeboarding_version.json` | when Core emits it | -**Delivery (`sync_strategy`).** The *set* of committed files above is identical either way; only how it reaches `main` differs. `push` (default) fast-forwards `main` directly. `pull_request` (for protected `main`) commits the same files to a machine-owned `sync_pr_branch` and opens one rolling PR into `main` — the baseline reaches `main` only on merge. Incremental sync always seeds from the baseline committed on `main` (never from the unmerged PR branch — that keeps an untrusted `static_analysis.pkl` off the runner), so under `pull_request` the rolling PR must be merged on a cadence to keep the baseline warm; each run still re-detects **every** change since the last-merged baseline via the whole-tree `fingerprint.json`, so no commits are missed between merges. +Everything else generated is removed on sync: v1 architecture Markdown and +`health/health_report.json`. Hand-written Markdown and user configuration +(`.codeboardingignore`, health config) are preserved. -## Where to commit — two separate workflows +**Delivery (`sync_strategy`).** The committed set is identical either way; only +how it reaches the branch differs. `push` fast-forwards it directly. +`pull_request` commits the same files to the machine-owned `codeboarding/sync` +branch and keeps one rolling PR open — the baseline lands only on merge, so that +PR must be merged on a cadence to keep the baseline warm. Sync always seeds from +the baseline committed on the target branch, never from the unmerged PR branch, +which keeps an untrusted pickle off the runner. Nothing is missed between +merges: `fingerprint.json` re-detects every change since the last merged +baseline. -1. **CI/CD on `main` (the baseline keeper).** On push to `main`, regenerate and commit `analysis.json`, `static_analysis.pkl`, `static_analysis.sha`, `health/health_report.json`, and rendered docs to `main`. Keeps the baseline current so PRs diff against an accurate, up-to-date snapshot and the extension shows a real diagram on the default branch. +## How a review resolves its two graphs -2. **The review action (PR).** Comment plus GitHub Actions artifact — no commits to contributors' branches (no churn, no generated-file merge conflicts, and it works on fork PRs where the token is read-only). +Every review builds **base** (at the merge base) and **head** (at the PR head), +diffs them, and posts the result. Each side takes the first source that applies. -## Now vs. later +**Base** -- **Now — extension-direct.** Committing `analysis.json` + `health_report.json` on `main` means a user who installs the extension and opens the repo sees the committed diagram + warnings **instantly, with no API key**. The PR comment's CTA points straight at the extension (install / open in editor). -- **Later — hosted webview.** The webview can read durable default-branch data from committed sync artifacts and PR-specific data from the uploaded review artifact or a backend copy of that artifact. +| Source | Engine cost | +|---|---| +| `cb-base-…-` exact cache hit | none | +| `cb-base-…` prefix hit — another commit's baseline, used as a warm seed | one catch-up incremental | +| No cache — seed from `.codeboarding/` committed at the merge base | one catch-up incremental, nothing to do if that baseline is current | +| No committed baseline either | full analysis | -## Warm-start tradeoff (the `.pkl`) +If the base was computed rather than restored, a trusted run saves it under its +exact key, so the next pull request forked from that commit gets the first row. -The warm-start — and the engine's incremental path itself — needs the pkl **and** its `.sha`: the cluster baseline that drives incremental lives only inside the pkl. Sync mode commits the pair alongside `analysis.json`, and review mode can still fall back to Actions cache or deterministic seeding when needed: +**Head** -- **Committed sync baseline:** sync copies `static_analysis.pkl` + `.sha` from `.codeboarding/` into the analysis workdir before incremental analysis. -- **No committed pkl or cache miss:** the action can seed the pkl deterministically (`engine_adapter.py seed`: LSP indexing + the same clustering call a full run makes — **no LLM calls**), then save it to `actions/cache`. Seeding is fail-open: if it fails, the head run falls back to a full analysis. +| Source | Covers | +|---|---| +| `cb-head-…-pr-mb-` prefix hit — this pull request's last analysis | only the commits pushed since that run | +| No hit: first run, moved merge base, changed config, or `/codeboarding refresh` — copy the base state | the whole pull request | -Either way the head analysis is seeded from that directory and runs incrementally when possible. +Then the head state is saved under `cb-head-…-`. -## Summary +**First run on a new pull request.** Nothing is cached. Base seeds from the +committed baseline and catches up; head starts from that base and covers every +file the pull request touches. Two engine passes, and both cache entries are +written. -| Artifact | Commit? | Where | Why | -|---|---|---|---| -| `analysis.json` | ✅ | sync commit on `main`; review artifact for PRs | diagram source | -| `health_report.json` | ✅ | sync commit on `main`; computed in review for comments, not uploaded | warnings | -| `static_analysis.pkl` | ✅ | sync commit on `main` only | warm-start incremental baseline | -| `static_analysis.sha` | ✅ | with `static_analysis.pkl` | warm-start gate | -| whole state directory | ❌ | Actions cache, per PR and per base commit | reuse the previous run instead of re-deriving from the base | +**Every run after it.** Base is an exact hit and costs no engine work at all; +head continues from the previous run and covers only the new commits. One +engine pass. ## Names -The two stores have different lifetimes, so they are named for different jobs. - -**Artifact — one per workflow run, immutable once written.** GitHub scopes an -artifact to its run and offers no way to append to or replace an earlier one, so -the name only has to be unique, and a consumer finds it by listing a pull -request's artifacts and taking the newest: +**Artifact — one per run, immutable.** GitHub scopes an artifact to its run and +cannot append to an earlier one, so the name only has to be unique; a consumer +lists a pull request's artifacts and takes the newest. codeboarding-review--/ analysis.json the head analysis, at metadata.head_sha base_analysis.json what it was compared against, at metadata.merge_base_sha metadata.json both SHAs, merge_base_resolved, seed_source, chain_depth +Shipping the base graph too keeps the artifact self-contained: a reader can +reproduce the comparison without resolving the merge base itself, and without +falling back to the default branch's committed baseline, which is the branch tip +rather than the fork point and would drift exactly as the review used to. + **Cache — shared across runs, found by key.** Here the name *is* the lookup, so -it carries everything that decides whether prior state still applies: +it carries everything that decides whether prior state still applies. | Key | Scope | The question it answers | |---|---|---| -| `cb-head-v1--pr-mb-` | one pull request | what did this pull request's last run produce | +| `cb-head-v1--pr-mb-` | one pull request | what did its last run produce | | `cb-base-v1--` | one commit | what does this commit's architecture look like | | `cb-fork-v1---pr-mb-` | one fork pull request | the same as `cb-head`, quarantined | `` digests the cache schema version, the pinned CodeBoarding version, -`.codeboardingignore`, and the model selection. A run forced with -`/codeboarding refresh` or `full` appends `-.`, because -cache entries are immutable and it must not save under the key holding the state -it was told to discard. - -The chain is restored **by prefix**, which selects the newest entry for that -pull request. The base is restored **by exact key first**: an exact hit is that -merge base's own analysis and needs no engine run, while a prefix hit is only -another commit's baseline, usable as a warm seed for the catch-up. - -## Cache identity - -The cache key pins everything that makes prior state meaningful: the pinned -CodeBoarding version, `.codeboardingignore`, and the merge base (which in turn -pins the analysis depth, since depth is read from that commit's baseline). -Anything else changing means no key matches, so the run re-derives from the base -rather than reusing state that describes a different analysis. - -State produced while analyzing a fork's code lives under its own key namespace -that trusted runs never restore from, and a fork run never writes a base entry: -`static_analysis.pkl` is a pickle, so restoring one derived from untrusted code -into a privileged run has to be impossible by construction, not by convention. +`.codeboardingignore`, and the model selection — everything that changes what an +analysis says. The merge base in the key pins the analysis depth too, since +depth is read from that commit's baseline. A run forced with `/codeboarding +refresh` or `full` appends `-.`: cache entries are +immutable, so it must not save under the key holding the state it was told to +discard. + +The head chain is restored **by prefix**, which selects the newest entry for the +pull request — an exact head-sha lookup would outrank a newer generation and +undo a refresh. The base is restored **by exact key first**, because there an +exact hit is that merge base's own analysis while a prefix hit is only a warm +seed. + +## Trust boundary + +`static_analysis.pkl` is a Python pickle, so state derived from code the +repository does not control must never be restored into a privileged run. Fork +analyses therefore live under their own `cb-fork-` namespace that no trusted run +restores from, and a fork run never writes a base entry. That is enforced by key +construction, not by convention. + +Caching is best effort throughout. A miss, an unavailable cache service, or a +GitHub Enterprise Server without one falls back to deriving the base directly. From e1db180051b32240f79ea2c6fb9bd0421b4da6b2 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Tue, 18 Aug 2026 22:26:02 +0200 Subject: [PATCH 08/12] fix(review): report the merge base under the name the webview reads The webview resolves a pull request's base as `base_commit_sha || pr_base_sha || base_sha` and then fetches the committed analysis at that ref. The action emitted the merge base as merge_base_sha, which is in none of those positions, so the chain fell through to base_sha, the branch tip. The webview would have kept comparing against a base this review never used, reproducing on the hosted side the exact drift the merge base removes here. The value now also ships as pr_base_sha, which is what the webview already documents that field to mean, so a deployed webview picks up the correct base without any change on its side. Co-Authored-By: Claude Opus 5 (1M context) --- docs/COMMIT_STRATEGY.md | 6 ++++++ scripts/action/build-review-artifact.sh | 6 +++++- tests/test_action_cache.py | 4 ++++ 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/docs/COMMIT_STRATEGY.md b/docs/COMMIT_STRATEGY.md index 216cf5c..61e895e 100644 --- a/docs/COMMIT_STRATEGY.md +++ b/docs/COMMIT_STRATEGY.md @@ -88,6 +88,12 @@ lists a pull request's artifacts and takes the newest. base_analysis.json what it was compared against, at metadata.merge_base_sha metadata.json both SHAs, merge_base_resolved, seed_source, chain_depth +`metadata.json` reports the merge base twice, as `merge_base_sha` and +`pr_base_sha`. The second is the name the webview already resolves +(`base_commit_sha || pr_base_sha || base_sha`); without it that chain falls +through to `base_sha`, the branch tip, and the webview compares against a base +the review never used. + Shipping the base graph too keeps the artifact self-contained: a reader can reproduce the comparison without resolving the merge base itself, and without falling back to the default branch's committed baseline, which is the branch tip diff --git a/scripts/action/build-review-artifact.sh b/scripts/action/build-review-artifact.sh index 7a64cd2..014dc5a 100755 --- a/scripts/action/build-review-artifact.sh +++ b/scripts/action/build-review-artifact.sh @@ -10,6 +10,10 @@ cp "$ANALYSIS_PATH" "${RUNNER_TEMP}/cb-review-artifact/analysis.json" cp "$BASE_ANALYSIS_PATH" "${RUNNER_TEMP}/cb-review-artifact/base_analysis.json" # base_sha stays the event's base branch tip for consumers that key on it; # merge_base_sha records the commit the diagram actually compared against. +# pr_base_sha carries the same value under the name the webview already reads: +# its lookup is base_commit_sha || pr_base_sha || base_sha, so without it the +# webview silently falls through to the branch tip and compares against a base +# this review never used. jq -n \ --arg mode "$ANALYSIS_MODE" \ --arg base_sha "$BASE_SHA" \ @@ -19,7 +23,7 @@ jq -n \ --arg pr_number "$PR_NUMBER" \ --arg seed_source "$SEED_SOURCE" \ --arg chain_depth "$CHAIN_DEPTH" \ - '{mode: $mode, base_sha: $base_sha, merge_base_sha: $merge_base_sha, + '{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}' \ > "${RUNNER_TEMP}/cb-review-artifact/metadata.json" diff --git a/tests/test_action_cache.py b/tests/test_action_cache.py index 13809fc..7775cf3 100644 --- a/tests/test_action_cache.py +++ b/tests/test_action_cache.py @@ -325,6 +325,10 @@ def test_it_ships_both_graphs_and_the_commit_they_describe(self) -> None: # is what base_analysis.json actually describes. self.assertEqual(metadata["base_sha"], "tip-sha") self.assertEqual(metadata["merge_base_sha"], "merge-base-sha") + # The webview resolves base_commit_sha || pr_base_sha || base_sha, so the + # merge base has to appear under a name it looks for or it silently uses + # the branch tip. + self.assertEqual(metadata["pr_base_sha"], "merge-base-sha") self.assertEqual(metadata["merge_base_resolved"], "true") self.assertEqual(metadata["seed_source"], "pr-chain") From e394a429a6d7daac8ac12abc213defb77348999c Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Tue, 18 Aug 2026 22:34:38 +0200 Subject: [PATCH 09/12] docs: list what metadata.json actually contains The layout block named the fields in a comment without saying what any of them mean, which left the two that matter unexplained: why the merge base is published twice, and that base_sha is the branch tip rather than the commit anything was compared against. Co-Authored-By: Claude Opus 5 (1M context) --- docs/COMMIT_STRATEGY.md | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/docs/COMMIT_STRATEGY.md b/docs/COMMIT_STRATEGY.md index 61e895e..77ba7fc 100644 --- a/docs/COMMIT_STRATEGY.md +++ b/docs/COMMIT_STRATEGY.md @@ -86,13 +86,28 @@ lists a pull request's artifacts and takes the newest. codeboarding-review--/ analysis.json the head analysis, at metadata.head_sha base_analysis.json what it was compared against, at metadata.merge_base_sha - metadata.json both SHAs, merge_base_resolved, seed_source, chain_depth + metadata.json which commits those graphs describe -`metadata.json` reports the merge base twice, as `merge_base_sha` and -`pr_base_sha`. The second is the name the webview already resolves -(`base_commit_sha || pr_base_sha || base_sha`); without it that chain falls -through to `base_sha`, the branch tip, and the webview compares against a base -the review never used. +| `metadata.json` field | Meaning | +|---|---| +| `head_sha` | the commit `analysis.json` describes | +| `pr_base_sha` | the merge base, under the name the webview resolves | +| `merge_base_sha` | the same value under this action's own name | +| `merge_base_resolved` | `false` means the merge base could not be resolved, so the comparison is against `base_sha` and may include commits this pull request never made | +| `base_sha` | the base branch tip when the event fired — *not* what was compared against | +| `pr_number` | the pull request | +| `mode` | `incremental` or `full`, how the head graph was produced | +| `seed_source` | `pr-chain` or `base`, which state the head analysis grew from | +| `chain_depth` | how many incremental runs are stacked on the base | + +The merge base is reported twice on purpose. The webview resolves a pull +request's base as `base_commit_sha || pr_base_sha || base_sha`, so a value +published only as `merge_base_sha` never reaches it and the chain falls through +to the branch tip — the drift this action stopped making, made again one layer +up. + +The last three fields are diagnostics. They explain how a graph was produced, +not what it means, and nothing rendering a diagram needs them. Shipping the base graph too keeps the artifact self-contained: a reader can reproduce the comparison without resolving the merge base itself, and without From d7ed1226988406a587c3bbdf3c9097d7184026a6 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Tue, 18 Aug 2026 23:10:21 +0200 Subject: [PATCH 10/12] fix(review): rebuild at the configured depth, and never diff across two bases Three defects found in review. Every escalation to a full analysis passed the baseline's realized depth_level, while Core resolves depth from depth_cap. On a baseline that stopped short of its cap, and this repository's own is depth_level 1 under depth_cap 2, a full run rebuilt at the smaller value and the configured depth was lost for good, taking any component that only exists deeper with it. That applied to /codeboarding full, to a run Core asks to escalate, and to sync's force_full. All of them now use the cap. A restored chain was accepted after checking only its depth, so a run whose base cache had gone missing regenerated the base and diffed a head grown from the previous base against it. Two runs of the engine over one commit need not name components identically, so that reports additions and removals for code nobody touched. The chain now records a digest of the base it grew from and is discarded unless it matches. The base cache entry was documented as shared by every pull request with that merge base. It is not: a cache entry is visible only to the ref that wrote it and to the default branch, so an automatic pull_request run's entry serves only that pull request. Sync, running on the base branch, is what actually warms everybody. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 2 +- docs/COMMIT_STRATEGY.md | 19 ++++++++++++- scripts/action/analyze.sh | 45 ++++++++++++++++++++---------- tests/test_action_cache.py | 56 ++++++++++++++++++++++++++++++++++---- 4 files changed, 100 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 6bd6833..fdd18de 100644 --- a/README.md +++ b/README.md @@ -66,7 +66,7 @@ Artifacts are kept 30 days. Reading the default branch's committed baseline inst ### Reused analysis -Each review seeds the head analysis from this pull request's own previous run, so a run only covers the commits pushed since it. With no previous run, it seeds from the merge base's analysis, which `sync` mode publishes and every pull request in the repository shares. Both live in the GitHub Actions cache, and state is re-derived from the merge base whenever the pinned CodeBoarding version, `.codeboardingignore`, the configured analysis depth, or the merge base itself changes. State produced while analyzing a fork is namespaced separately and is never restored by a run on this repository's own code. +Each review seeds the head analysis from this pull request's own previous run, so a run only covers the commits pushed since it. With no previous run, it seeds from the merge base's analysis. `sync` mode publishes that entry on the base branch, where every pull request can restore it; an entry a review run computes for itself is scoped to that pull request. Both live in the GitHub Actions cache, and state is re-derived from the merge base whenever the pinned CodeBoarding version, `.codeboardingignore`, the configured analysis depth, or the merge base itself changes. State produced while analyzing a fork is namespaced separately and is never restored by a run on this repository's own code. Caching is best-effort: a cache miss, an unavailable cache service, or a GitHub Enterprise Server without one falls back to analyzing the merge base directly, exactly as before. diff --git a/docs/COMMIT_STRATEGY.md b/docs/COMMIT_STRATEGY.md index 77ba7fc..6131267 100644 --- a/docs/COMMIT_STRATEGY.md +++ b/docs/COMMIT_STRATEGY.md @@ -57,7 +57,18 @@ diffs them, and posts the result. Each side takes the first source that applies. | No committed baseline either | full analysis | If the base was computed rather than restored, a trusted run saves it under its -exact key, so the next pull request forked from that commit gets the first row. +exact key. How far that reaches depends on where the run happened, because a +cache entry is only visible to the ref that wrote it and to the default branch: + +- **sync**, on the base branch, writes an entry every pull request can restore. + This is what makes the first row common. +- **`/codeboarding`**, which runs on the default branch, also writes a shared + entry. +- **an automatic `pull_request` run** writes into `refs/pull//merge`, so its + entry serves only later runs of that same pull request. Another pull request + with the identical merge base still computes its own. + +So review runs warm themselves, and sync is what warms everybody. **Head** @@ -131,6 +142,12 @@ refresh` or `full` appends `-.`: cache entries are immutable, so it must not save under the key holding the state it was told to discard. +A restored chain is used only when it grew from the very base graph this run +diffs against, which `origin.json` records as a digest. Two runs of the engine +over one commit need not name components identically, so a head descended from +one base and a diagram drawn against another would report additions and +removals for code nobody touched. + The head chain is restored **by prefix**, which selects the newest entry for the pull request — an exact head-sha lookup would outrank a newer generation and undo a refresh. The base is restored **by exact key first**, because there an diff --git a/scripts/action/analyze.sh b/scripts/action/analyze.sh index 73051fb..372b5b9 100755 --- a/scripts/action/analyze.sh +++ b/scripts/action/analyze.sh @@ -27,16 +27,10 @@ full() { exit 1 fi } -depth_from() { - local analysis="$1" - [ -f "$analysis" ] || return 0 - python3 -c 'import json,sys; print(json.load(open(sys.argv[1])).get("metadata", {}).get("depth_level", ""))' \ - "$analysis" 2>/dev/null || true -} - -# Core resolves incremental depth from depth_cap, falling back to depth_level for -# baselines predating it. Compare the same value, or a run that merely stopped -# short of its cap reads as a scope change. +# Core resolves depth from depth_cap, falling back to depth_level for baselines +# predating it. Use the same value everywhere: a run that stopped short of its +# cap must not be read as a scope change, and rebuilding at the realized depth +# would ratchet the configured depth down every time a full run happens. depth_cap_from() { local analysis="$1" [ -f "$analysis" ] || return 0 @@ -71,7 +65,20 @@ json.dump({ "cfg_hash": os.environ.get("CFG_HASH", ""), "seed_source": sys.argv[2], "chain_depth": int(sys.argv[3]), -}, open(sys.argv[1], "w"), indent=2)' "$state/origin.json" "$2" "$3" + "base_digest": sys.argv[4], +}, open(sys.argv[1], "w"), indent=2)' "$state/origin.json" "$2" "$3" "$4" +} + +# Two independently generated analyses of the same commit need not name the same +# components, so a head that grew from one base cannot be diffed against another. +analysis_digest() { + local analysis="$1" + [ -f "$analysis" ] || return 0 + if command -v sha256sum >/dev/null 2>&1; then + sha256sum "$analysis" | cut -c1-16 + else + shasum -a 256 "$analysis" | cut -c1-16 + fi } cache_out() { @@ -87,7 +94,7 @@ analyze_sync() { rm -rf "$work" seed_state "$CHECKOUT_DIR" "$state" local depth - depth="$(depth_from "$state/analysis.json")" + depth="$(depth_cap_from "$state/analysis.json")" depth="${depth:-2}" if [ "${FORCE_FULL,,}" = true ]; then @@ -126,6 +133,14 @@ chain_usable() { echo "::notice::Analysis depth changed since the last run; re-seeding from the base analysis." return 1 fi + # The chain descends from one base graph and the diagram is drawn against + # another only if the base was regenerated in between. Their components need + # not match, so keeping the chain would report additions and removals for code + # nobody touched. + if [ "$(origin_field "$CACHE_CHAIN_DIR" base_digest)" != "$(analysis_digest "$base_analysis")" ]; then + echo "::notice::The base analysis is not the one this pull request's cached analysis grew from; re-seeding from it." + return 1 + fi } analyze_review() { @@ -150,7 +165,7 @@ analyze_review() { seed_state "$base_checkout" "$base_state" fi local base_depth - base_depth="$(depth_from "$base_state/analysis.json")" + base_depth="$(depth_cap_from "$base_state/analysis.json")" incremental "$base_checkout" "$base_state" if [ "$REQUIRES_FULL" = true ]; then full "$base_checkout" "$base_state" "${base_depth:-2}" @@ -161,7 +176,7 @@ analyze_review() { local base_analysis="$base_state/analysis.json" [ -f "$base_analysis" ] || { echo "::error::Review baseline analysis is missing."; exit 1; } local depth - depth="$(depth_from "$base_analysis")" + depth="$(depth_cap_from "$base_analysis")" depth="${depth:-2}" # Seed the head from this pull request's own last analysis when there is one, @@ -189,7 +204,7 @@ analyze_review() { fi fi - write_origin "$head_state" "$seed_source" "$chain_depth" + write_origin "$head_state" "$seed_source" "$chain_depth" "$(analysis_digest "$base_analysis")" cache_out "$head_state" chain local save_base=false if [ "$base_source" = computed ]; then diff --git a/tests/test_action_cache.py b/tests/test_action_cache.py index 7775cf3..f7a7fa3 100644 --- a/tests/test_action_cache.py +++ b/tests/test_action_cache.py @@ -22,7 +22,11 @@ output = argv[argv.index("--output-dir") + 1] os.makedirs(output, exist_ok=True) with open(os.environ["CB_ENGINE_LOG"], "a") as log: - log.write(json.dumps({"mode": argv[0], "checkout": argv[argv.index("--local") + 1]}) + "\\n") + log.write(json.dumps({ + "mode": argv[0], + "checkout": argv[argv.index("--local") + 1], + "depth": argv[argv.index("--depth-level") + 1] if "--depth-level" in argv else None, + }) + "\\n") analysis = os.path.join(output, "analysis.json") with open(analysis, "w") as handle: json.dump({"metadata": {"depth_level": 2}, "components": [], "components_relations": []}, handle) @@ -30,6 +34,13 @@ ''' +def _digest(path: Path) -> str: + """Mirrors analysis_digest in analyze.sh: sha256 of the file, first 16 hex chars.""" + import hashlib + + return hashlib.sha256(path.read_bytes()).hexdigest()[:16] + + def _state(directory: Path, depth: int = 2, cap: int | None = None, **origin: object) -> Path: directory.mkdir(parents=True, exist_ok=True) metadata: dict[str, int] = {"depth_level": depth} @@ -212,9 +223,13 @@ def _analyze(self, **extra: str) -> dict[str, str]: def _engine_calls(self) -> list[dict[str, str]]: return [json.loads(line) for line in self.engine_log.read_text(encoding="utf-8").splitlines()] + def _bind(self, **origin: object) -> None: + """Chain fixture bound to the base it was derived from.""" + _state(self.cache_chain, base_digest=_digest(self.cache_base / "analysis.json"), **origin) + def test_warm_chain_analyzes_only_the_head(self) -> None: _state(self.cache_base) - _state(self.cache_chain, chain_depth=3, seed_source="pr-chain") + self._bind(chain_depth=3, seed_source="pr-chain") values = self._analyze() @@ -236,7 +251,7 @@ def test_cached_base_without_a_chain_seeds_from_the_base(self) -> None: def test_refresh_ignores_the_pull_request_chain(self) -> None: _state(self.cache_base) - _state(self.cache_chain, chain_depth=3) + self._bind(chain_depth=3) values = self._analyze(SEED_MODE="refresh") @@ -255,15 +270,45 @@ def test_a_run_that_stopped_short_of_its_cap_keeps_the_chain(self) -> None: # Core resolves incremental depth from depth_cap, so a realized # depth_level below the cap is not a scope change. _state(self.cache_base, depth=2, cap=2) - _state(self.cache_chain, depth=1, cap=2, chain_depth=3) + _state(self.cache_chain, depth=1, cap=2, chain_depth=3, base_digest=_digest(self.cache_base / "analysis.json")) values = self._analyze() self.assertEqual(values["seed_source"], "pr-chain") + def test_a_chain_from_a_different_base_is_discarded(self) -> None: + # Two runs of the engine over the same commit need not name components + # identically, so diffing a head grown from one against the other would + # report changes nobody made. + _state(self.cache_base) + _state(self.cache_chain, chain_depth=3, base_digest="0000000000000000") + + values = self._analyze() + + self.assertEqual(values["seed_source"], "base") + + def test_a_chain_with_no_recorded_base_is_discarded(self) -> None: + _state(self.cache_base) + _state(self.cache_chain, chain_depth=3) + + values = self._analyze() + + self.assertEqual(values["seed_source"], "base") + + def test_a_forced_full_rebuilds_at_the_configured_cap(self) -> None: + # The baseline stopped short of its cap. Rebuilding at the realized + # depth would ratchet the configured depth down for good. + _state(self.cache_base, depth=1, cap=2) + + self._analyze(SEED_MODE="full") + + calls = self._engine_calls() + self.assertEqual(calls[-1]["mode"], "full") + self.assertEqual(calls[-1]["depth"], "2") + def test_analysis_is_staged_for_the_cache(self) -> None: _state(self.cache_base) - _state(self.cache_chain) + self._bind() self._analyze() @@ -273,6 +318,7 @@ def test_analysis_is_staged_for_the_cache(self) -> None: self.assertEqual(origin["merge_base_sha"], "merge-base-sha") self.assertEqual(origin["head_sha"], "head-sha") self.assertEqual(origin["engine_version"], "0.13.8") + self.assertEqual(origin["base_digest"], _digest(self.cache_base / "analysis.json")) self.assertFalse((self.cache_out / "base").exists(), "a cached base needs no re-save") From 8adcc1a734f586cbfe1d0ed1182a1609590d842d Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Tue, 18 Aug 2026 22:01:04 +0200 Subject: [PATCH 11/12] fix(sync): read the artifact manifest without mapfile mapfile needs bash 4 and macOS ships bash 3.2, so install-sync.sh aborted with "mapfile: command not found" on a developer machine and its test had been failing there for everyone. A read loop does the same job everywhere. Co-Authored-By: Claude Opus 5 (1M context) (cherry picked from commit ae62df29b93c8da22e3aa0a2a008a5e9474e46c1) --- scripts/action/install-sync.sh | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/scripts/action/install-sync.sh b/scripts/action/install-sync.sh index 042f75b..b5b7725 100755 --- a/scripts/action/install-sync.sh +++ b/scripts/action/install-sync.sh @@ -17,7 +17,12 @@ print(*artifacts, sep="\n") PY )" || { echo "::error::Could not read Core's persisted artifact manifest."; exit 1; } [ -n "$manifest" ] || { echo "::error::Core's persisted artifact manifest is empty."; exit 1; } -mapfile -t manifest_entries <<< "$manifest" +# Read into an array without mapfile, which needs bash 4: macOS ships bash 3.2, +# so mapfile made this script, and its tests, unrunnable for local development. +manifest_entries=() +while IFS= read -r entry; do + manifest_entries+=("$entry") +done <<< "$manifest" required="${manifest_entries[0]}" artifacts=("${manifest_entries[@]:1}") [ -f "$ANALYSIS_DIR/$required" ] || { echo "::error::Core did not produce $required."; exit 1; } From 6e6ab0c1bba94e084513711d913cdf3ee31b4fc3 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Tue, 18 Aug 2026 23:23:33 +0200 Subject: [PATCH 12/12] fix(sync): publish the baseline under both commits it describes Sync published its analysis under one key, the baseline commit it creates. A pull request opened in the window between a change landing on the branch and sync committing its baseline has the earlier commit as its merge base, so it missed that key and paid for a catch-up every run. That window is the common case on an active branch, not an edge case. The same state is valid for both commits: sync analyzed the earlier tree, and the baseline commit differs from it only in .codeboarding files, which the fingerprint does not cover. Both keys are now published. deliver-sync.sh had no tests, which is how the outputs the cache depends on went unchecked. It now runs against a real local remote, no network needed. Co-Authored-By: Claude Opus 5 (1M context) --- action.yml | 10 +++++ docs/COMMIT_STRATEGY.md | 5 ++- scripts/action/deliver-sync.sh | 10 +++-- tests/test_action_sync.py | 81 ++++++++++++++++++++++++++++++++++ 4 files changed, 102 insertions(+), 4 deletions(-) diff --git a/action.yml b/action.yml index 2a24dd2..a78abee 100644 --- a/action.yml +++ b/action.yml @@ -261,6 +261,16 @@ runs: path: ${{ runner.temp }}/cb-cache/base key: ${{ steps.cache_keys.outputs.base_key_prefix }}${{ steps.sync_commit.outputs.baseline_sha }} + # The same state under the commit that was analyzed. A pull request branched + # before this run's baseline commit landed has that commit as its merge base. + - name: Save baseline analysis for the analyzed commit + if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'sync' && steps.sync_commit.outputs.analyzed_sha != '' && steps.sync_commit.outputs.analyzed_sha != steps.sync_commit.outputs.baseline_sha && steps.cache_keys.outputs.base_key_prefix != '' + continue-on-error: true + uses: actions/cache/save@v4 + with: + path: ${{ runner.temp }}/cb-cache/base + key: ${{ steps.cache_keys.outputs.base_key_prefix }}${{ steps.sync_commit.outputs.analyzed_sha }} + - name: Write sync summary if: always() && steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'sync' shell: bash diff --git a/docs/COMMIT_STRATEGY.md b/docs/COMMIT_STRATEGY.md index 6131267..f0b37f3 100644 --- a/docs/COMMIT_STRATEGY.md +++ b/docs/COMMIT_STRATEGY.md @@ -61,7 +61,10 @@ exact key. How far that reaches depends on where the run happened, because a cache entry is only visible to the ref that wrote it and to the default branch: - **sync**, on the base branch, writes an entry every pull request can restore. - This is what makes the first row common. + This is what makes the first row common. It publishes under two keys, the + commit it analyzed and the baseline commit it creates, because a pull request + branched just before that baseline landed has the earlier commit as its merge + base. - **`/codeboarding`**, which runs on the default branch, also writes a shared entry. - **an automatic `pull_request` run** writes into `refs/pull//merge`, so its diff --git a/scripts/action/deliver-sync.sh b/scripts/action/deliver-sync.sh index 692c899..f647855 100755 --- a/scripts/action/deliver-sync.sh +++ b/scripts/action/deliver-sync.sh @@ -18,10 +18,14 @@ chmod 700 "$ASKPASS" export GIT_ASKPASS="$ASKPASS" GIT_TERMINAL_PROMPT=0 export GH_HOST="${GH_HOST#*://}" trap 'rm -f "$ASKPASS"' EXIT -# baseline_sha names the commit later pull requests branch from, so their merge -# base restores this run's analysis instead of recomputing it. +# A pull request's merge base is whichever commit its author branched from, and +# this run's analysis is valid for two of them: the commit it analyzed, and the +# baseline commit it writes on top, which differs only in .codeboarding files +# that the fingerprint ignores. Publishing both means a pull request opened +# either side of a sync commit still gets an exact hit. emit_result() { - printf 'files_written=%s\ncommitted=%s\nbaseline_sha=%s\n' "$1" "$2" "$3" >> "$GITHUB_OUTPUT" + printf 'files_written=%s\ncommitted=%s\nbaseline_sha=%s\nanalyzed_sha=%s\n' \ + "$1" "$2" "$3" "$BASE_SHA" >> "$GITHUB_OUTPUT" } close_stale_pr() { [ "$SYNC_STRATEGY" = pull_request ] || return 0 diff --git a/tests/test_action_sync.py b/tests/test_action_sync.py index f192b80..073f494 100644 --- a/tests/test_action_sync.py +++ b/tests/test_action_sync.py @@ -285,3 +285,84 @@ def test_empty_review_is_successful(self) -> None: if __name__ == "__main__": unittest.main() + + +class SyncDeliveryTests(unittest.TestCase): + """deliver-sync.sh against a real local remote: no network, real git.""" + + 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) + self._git(self.remote, "init", "--bare", "-b", "main") + + self.checkout = self.root / "checkout" + self._git(self.root, "clone", "--quiet", str(self.remote), str(self.checkout)) + for key, value in (("user.email", "t@example.com"), ("user.name", "T"), ("commit.gpgsign", "false")): + self._git(self.checkout, "config", key, value) + (self.checkout / "app.py").write_text("print('hi')\n", encoding="utf-8") + (self.checkout / ".codeboarding").mkdir() + self._git(self.checkout, "add", "-A") + self._git(self.checkout, "commit", "-m", "initial") + self._git(self.checkout, "push", "--quiet", "origin", "main") + self.analyzed_sha = self._git(self.checkout, "rev-parse", "HEAD") + + # What the engine left behind for this run. + self.analysis = self.root / "analysis" + self.analysis.mkdir() + for name in ("analysis.json", "fingerprint.json", "static_analysis.pkl"): + (self.analysis / name).write_text(f"fresh {name}\n", encoding="utf-8") + + 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 _git(self, cwd: Path, *args: str) -> str: + return subprocess.run(["git", *args], cwd=str(cwd), capture_output=True, text=True, check=True).stdout.strip() + + def test_it_publishes_the_analysis_under_both_commits(self) -> None: + output = self.root / "github-output" + result = subprocess.run( + [str(ROOT / "scripts" / "action" / "deliver-sync.sh")], + 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": "push", + }, + capture_output=True, + text=True, + check=False, + ) + 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) + self.assertEqual(values["committed"], "true") + baseline_sha = self._git(self.checkout, "rev-parse", "HEAD") + # The analysis describes the tree it ran on and the baseline commit + # written on top, which differs only in files the fingerprint ignores. + # A pull request branched either side of that commit must hit the cache. + self.assertEqual(values["baseline_sha"], baseline_sha) + self.assertEqual(values["analyzed_sha"], self.analyzed_sha) + self.assertNotEqual(values["baseline_sha"], values["analyzed_sha"])