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..fdd18de 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 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. @@ -44,10 +44,34 @@ 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. +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. `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. + +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 +221,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 | 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 2c07304..a78abee 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: '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.' 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,50 @@ 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 }} + 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 + # 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 }} + + # 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_restore_keys }} + 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 +225,7 @@ runs: ACTION_PATH: ${{ github.action_path }} ANALYSIS_KIND: sync CHECKOUT_DIR: ${{ github.workspace }}/.codeboarding-target + CACHE_OUT_DIR: ${{ runner.temp }}/cb-cache FORCE_FULL: ${{ inputs.force_full }} MODEL: ${{ inputs.model }} AGENT_MODEL_INPUT: ${{ inputs.agent_model }} @@ -199,6 +251,26 @@ 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/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 @@ -218,9 +290,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 + 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 +308,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/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/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' @@ -244,10 +344,15 @@ 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 }} + 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 }} + CHAIN_DEPTH: ${{ steps.review_analyze.outputs.chain_depth }} run: "$GITHUB_ACTION_PATH/scripts/action/build-review-artifact.sh" - name: Upload review artifact @@ -258,6 +363,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 @@ -268,6 +376,9 @@ 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 }} + 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/docs/COMMIT_STRATEGY.md b/docs/COMMIT_STRATEGY.md index 2f42073..f0b37f3 100644 --- a/docs/COMMIT_STRATEGY.md +++ b/docs/COMMIT_STRATEGY.md @@ -1,58 +1,169 @@ -# 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 | -|---|---|---|---| -| `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. - -**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. - -> **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. - -**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. - -## Where to commit — two separate workflows - -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. - -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). - -## Now vs. later - -- **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. - -## Warm-start tradeoff (the `.pkl`) - -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: - -- **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. - -Either way the head analysis is seeded from that directory and runs incrementally when possible. - -## Summary - -| Artifact | Commit? | Where | Why | +| Store | Holds | Lifetime | Who can read it | |---|---|---|---| -| `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 | +| **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 | + +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. + +## What sync commits + +Sync is the only git writer. Review mode never commits to a contributor's +branch, so generated files cannot conflict during a 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 | + +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. + +**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. + +## How a review resolves its two graphs + +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. + +**Base** + +| 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 | + +If the base was computed rather than restored, a trusted run saves it under its +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. 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 + 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** + +| 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 | + +Then the head state is saved under `cb-head-…-`. + +**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. + +**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 + +**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 which commits those graphs describe + +| `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 +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. + +| Key | Scope | The question it answers | +|---|---|---| +| `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 — 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. + +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 +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. diff --git a/scripts/action/analyze.sh b/scripts/action/analyze.sh index 3de910d..372b5b9 100755 --- a/scripts/action/analyze.sh +++ b/scripts/action/analyze.sh @@ -27,11 +27,16 @@ full() { exit 1 fi } -depth_from() { +# 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 - python3 -c 'import json,sys; print(json.load(open(sys.argv[1])).get("metadata", {}).get("depth_level", ""))' \ - "$analysis" 2>/dev/null || true + 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() { @@ -40,12 +45,56 @@ seed_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]), + "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() { + 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" 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 @@ -56,6 +105,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 +121,99 @@ 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 + # 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() { 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_cap_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_cap_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" "$(analysis_digest "$base_analysis")" + 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..014dc5a 100755 --- a/scripts/action/build-review-artifact.sh +++ b/scripts/action/build-review-artifact.sh @@ -1,13 +1,30 @@ #!/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. +# 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" \ + --arg merge_base_sha "$MERGE_BASE_SHA" \ + --arg merge_base_resolved "$MERGE_BASE_RESOLVED" \ --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, 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" 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..d28d0e2 100755 --- a/scripts/action/build-review-comment.sh +++ b/scripts/action/build-review-comment.sh @@ -12,6 +12,20 @@ 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 [ "${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 + 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..ed9d060 --- /dev/null +++ b/scripts/action/cache-keys.sh @@ -0,0 +1,68 @@ +#!/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 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")" +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-" +{ + 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 +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_key" + echo "chain_restore_keys=$chain_prefix" +} >> "$GITHUB_OUTPUT" diff --git a/scripts/action/deliver-sync.sh b/scripts/action/deliver-sync.sh index 7af481e..f647855 100755 --- a/scripts/action/deliver-sync.sh +++ b/scripts/action/deliver-sync.sh @@ -18,6 +18,15 @@ chmod 700 "$ASKPASS" export GIT_ASKPASS="$ASKPASS" GIT_TERMINAL_PROMPT=0 export GH_HOST="${GH_HOST#*://}" trap 'rm -f "$ASKPASS"' EXIT +# 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\nanalyzed_sha=%s\n' \ + "$1" "$2" "$3" "$BASE_SHA" >> "$GITHUB_OUTPUT" +} close_stale_pr() { [ "$SYNC_STRATEGY" = pull_request ] || return 0 git fetch "$REMOTE" "$TARGET_BRANCH" @@ -46,7 +55,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 +81,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 +89,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 +100,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 +128,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..5b48ddc 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,61 @@ 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" +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 +# 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_resolved=true + merge_base_sha="$compare_merge_base" + behind_by="$compare_behind" +else + # 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 [ "$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 "merge_base_resolved=$merge_base_resolved" + 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/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; } diff --git a/tests/test_action_cache.py b/tests/test_action_cache.py new file mode 100644 index 0000000..f7a7fa3 --- /dev/null +++ b/tests/test_action_cache.py @@ -0,0 +1,429 @@ +"""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], + "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) +print(json.dumps({"requiresFullAnalysis": False, "analysis_path": analysis})) +''' + + +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} + 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", + "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, + 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_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() + 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 _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) + self._bind(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) + self._bind(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, 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) + self._bind() + + 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.assertEqual(origin["base_digest"], _digest(self.cache_base / "analysis.json")) + 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") + # 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") + + +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", "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") + 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 1ebd829..073f494 100644 --- a/tests/test_action_sync.py +++ b/tests/test_action_sync.py @@ -42,6 +42,68 @@ 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) + # 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: root = Path(tmp) @@ -50,6 +112,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' @@ -220,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"]) 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()