feat(review): compare against the merge base and reuse the PR's last analysis - #81
Conversation
…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) <noreply@anthropic.com>
CodeBoarding reviewStatus: 0 changed components See the full change in CodeBoarding. Compared against the merge base: this branch is 2 commits behind graph LR
n_Visual_Rendering_Engine["Visual Rendering Engine"]
n_Structural_Diff_Engine["Structural Diff Engine"]
n_Interaction_Orchestrator["Interaction Orchestrator"]
n_Visual_Rendering_Engine -- "Returns rendering metadata and diagram artifacts" --> n_Interaction_Orchestrator
n_Structural_Diff_Engine -- "Provides annotated diff model for visualization" --> n_Visual_Rendering_Engine
n_Interaction_Orchestrator -- "Triggers structural comparison via CLI" --> n_Structural_Diff_Engine
classDef added fill:#1f883d,stroke:#0b5d23,color:#ffffff;
classDef modified fill:#bf8700,stroke:#7d4e00,color:#ffffff;
classDef deleted fill:#cf222e,stroke:#82071e,color:#ffffff,stroke-dasharray:5 3;
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fd95a27be
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| 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." |
There was a problem hiding this comment.
When the compare API is unavailable, rate-limited, or returns no merge base, this branch leaves merge_base_sha initialized to the event's moving base tip and continues the review. If the base advanced after the PR forked, the resulting diagram attributes unrelated base-branch commits to the PR—precisely the regression this merge-base change must prevent. Resolve the merge base locally or fail the review rather than silently producing an incorrect comparison.
AGENTS.md reference: AGENTS.md:L34-L37
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6771689, though not by failing the run.
You are right that the fallback was the problem: it silently produced exactly the comparison this change exists to prevent. Hard-failing is the wrong trade here, since a transient 503 would then block every review, and the compare endpoint returned 503 twice while this PR was being opened. Resolving locally is not cheap either: the head is checked out at fetch-depth: 1 and the base commit is fetched --depth=1, so the two tips share no history and git merge-base cannot answer without progressively deepening the fetch.
So the failure is now loud instead of silent:
- the compare call is retried once, which covers the transient case;
- an unresolved merge base sets
merge_base_resolved=false, which puts a> [!WARNING]block at the top of the review comment saying the comparison is against the tip of the base branch and that its commits may appear as this PR's changes; - the same flag is recorded in the artifact metadata, and the log line was raised from
::notice::to::warning::.
A reader can no longer be misled without being told.
| continue-on-error: true | ||
| uses: actions/cache/save@v4 | ||
| with: | ||
| path: ${{ runner.temp }}/cb-cache/out/chain |
There was a problem hiding this comment.
After any review saves this chain, the next run restores ${{ runner.temp }}/cb-cache/chain (action.yml:207), but this step saves ${{ runner.temp }}/cb-cache/out/chain. actions/cache includes the supplied paths in its cache version, so the restore lookup cannot match the saved entry even when the key is identical; the base cache has the same out/base versus base mismatch. Consequently neither advertised cache is ever reused and each run redoes the expensive analysis. Stage or save the state at the same paths used by the restore steps.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 6771689.
Verified against the toolkit rather than taken on trust: getCacheVersion in @actions/cache hashes the literal path strings (components.join("|") over paths + compression method + salt), so the save at .../cb-cache/out/chain could never be found by a restore at .../cb-cache/chain. Every run would have missed and redone the full analysis while the workflow looked correctly wired.
Saves and restores now use one path per cache (cb-cache/chain, cb-cache/base), including the sync-mode base save, which has to match the review-mode restore string exactly for reviews to pick it up.
CachePathParityTests in tests/test_action_cache.py parses action.yml and asserts every saved path is also a restored path. Mutation-checked: reintroducing out/chain on the save step fails it.
| 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)" | ||
|
|
There was a problem hiding this comment.
When a workflow changes model, agent_model, parsing_model, or the provider supplying their defaults, this hash remains unchanged because it only includes the schema, engine version, and ignore file. A warm PR therefore restores analysis produced with the old model and only applies the new model to the incremental delta, leaving a mixed, stale component graph; even /codeboarding refresh can reuse a base generated under the previous settings. Include the resolved analysis/parsing model configuration in the cache identity so configuration changes re-seed the analysis.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 6771689. llm_provider, model, agent_model, and parsing_model are now hashed into cfg_hash, so changing any of them produces no matching key and the analysis is re-seeded from the merge base rather than mixing a new model's delta into an old model's graph. Covered by test_model_selection_changes_the_identity.
| uses: actions/cache/save@v4 | ||
| with: | ||
| path: ${{ runner.temp }}/cb-cache/out/chain | ||
| key: ${{ steps.cache_keys.outputs.chain_key }} |
There was a problem hiding this comment.
When /codeboarding refresh or /codeboarding full runs for a head SHA that already has a chain cache, it deliberately recomputes the analysis but then saves under the same key as the existing entry. Actions caches are immutable, so this save cannot replace the old chain; the next run after another push restores the pre-refresh analysis rather than the PR's actual last analysis. Give forced refreshes a distinct generation key, or otherwise ensure subsequent prefix restores select the newly computed state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed in 6771689. Cache entries being immutable meant the save was dropped and the next run restored precisely the state the refresh was asked to discard.
/codeboarding refresh and /codeboarding full now save under <prefix><head_sha>-<mode><run_id>.<attempt>. The prefix is unchanged, so a later run still finds it, and since prefix restores select the most recently created entry, the refreshed analysis wins over the one it replaced. Covered by test_a_forced_refresh_does_not_save_under_the_key_it_replaces.
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) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6771689222
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| [ "${SEED_MODE:-chain}" = chain ] || \ | ||
| chain_key="$chain_key-$SEED_MODE${GITHUB_RUN_ID:-0}.${GITHUB_RUN_ATTEMPT:-1}" |
There was a problem hiding this comment.
When an unchanged head already has the normal $chain_prefix$HEAD_SHA entry, a forced refresh or full run saves the replacement only under this suffixed key. A subsequent plain /codeboarding requests the old unsuffixed key, and the exact cache match wins over the newer prefix match, so incremental analysis starts from the pre-refresh state and may return it unchanged. Fresh evidence beyond the earlier cache comment is that the new suffix logic changes only the forced run's save key while leaving the next normal run's exact lookup unchanged; the normal restore path must select the refreshed generation too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and my previous reply was wrong on this point. Fixed in ad7a1db.
I had claimed the prefix restore would select the refreshed generation. That only holds once the head moves. At the same head the restore still asked for <prefix><head_sha> as its exact key, and an exact match outranks every prefix match, so the pre-refresh entry came back and the refresh was undone exactly as you describe.
The head-sha key was never load-bearing for the lookup: what a run needs to continue from is this pull request's newest analysis, which is what the prefix selects on its own. The head sha only keeps saves unique. So the chain restore now passes the prefix as both key and restore-keys, and the generation suffix keeps forced runs from colliding with the entry they replace.
The base cache deliberately keeps the opposite semantics, since there an exact hit is the merge base's own analysis and a prefix hit is only a warm seed. test_the_chain_is_restored_by_prefix_so_a_refresh_survives pins both, and fails if the chain restore goes back to the head-sha key.
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) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad7a1db79b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| model_digest="$(printf '%s\n%s\n%s\n%s\n' \ | ||
| "${LLM_PROVIDER:-}" "${MODEL:-}" "${AGENT_MODEL_INPUT:-}" "${PARSING_MODEL_INPUT:-}" | digest)" |
There was a problem hiding this comment.
When a workflow changes an endpoint such as OLLAMA_BASE_URL, OLLAMA_HOST, or LITELLM_BASE_URL—settings explicitly supported by the action and consumed by with-auth.sh—this digest remains unchanged because it hashes only the provider and model inputs. A warm PR can therefore restore component state produced by a different server and incrementally analyze only the new delta with the current server, yielding a mixed, stale graph; even refresh can reuse the old base entry. Hash the selected provider's non-secret endpoint configuration as part of cfg_hash.
Useful? React with 👍 / 👎.
| 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}" |
There was a problem hiding this comment.
When the exact base key misses but the prefix restore finds an analysis from another commit whose configured depth differs, this branch copies that restored state and derives base_depth from it rather than from the target merge base's committed baseline. Core also resolves the incremental depth cap from that restored state, so the merge base is analyzed at the unrelated commit's depth and the resulting state is then saved under the exact merge-base key, making the wrong scope persistent. Read and compare the depth from base_checkout/.codeboarding/analysis.json before accepting a prefix seed, or prevent prefix matches across depth configurations.
Useful? React with 👍 / 👎.
ivanmilevtues
left a comment
ivanmilevtues
left a comment
There was a problem hiding this comment.
Seems good, i mainly took a look at the action.yml and skimmed through the rest.
| 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.' |
There was a problem hiding this comment.
i don't get this description at all.
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19b25e672d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…wo 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) <noreply@anthropic.com>
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) <noreply@anthropic.com> (cherry picked from commit ae62df2)
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) <noreply@anthropic.com>
2 participants
Two changes to review mode, plus the protected-test rule.
1. Compare against the merge base
Review compared the head with
pull_request.base.sha, the base branch tip, which moves whenever anyone else pushes. Whenmainadvanced after a branch forked, that commit's components showed up in the PR's diagram, and showed up backwards, as removals.The merge base (
X) is the anchorgit diff main...PRand GitHub's own "Files changed" tab use, so the diagram and the file diff now agree. The one thing tip-comparison gave you for free was noticing a stale branch, so the comment states the distance instead: "this branch is 3 commits behindmain".Resolved with one compare API call, which returns
merge_base_commit.shaandbehind_bytogether. It fails open to the previous behaviour with a notice.2. Reuse the pull request's last analysis
Every run rebuilt the base analysis from scratch and re-derived the head from it: two analyses per run, and the same base recomputed for every PR forked from it.
Now a run seeds the head from this PR's own previous analysis and covers only the commits pushed since it, and it restores the merge base's analysis instead of computing one. Sync mode publishes that base entry as a by-product of the baseline it already computes, so the first PR 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 itself changes. The first three sit in the key prefix, so a mismatch simply finds nothing to restore.State produced while analyzing a fork lives under a
cb-fork-*namespace that trusted runs never restore from, and a fork run never writes a base entry.static_analysis.pklis a pickle, so that boundary is enforced by key construction rather than convention.Every cache step is
continue-on-error. A miss, an unavailable cache service, or a GHES instance without one falls back to exactly today's behaviour.New commands:
/codeboarding refreshre-seeds from the base,/codeboarding fullforces a full analysis.3. Protected tests
tests/test_merge_base_contract.pybuilds a real git history in the shape above and asserts both that the guard resolvesXand that the base analysis actually runs againstX's tree. It carries aPROTECTED TESTheader, andAGENTS.md(renamed fromAGENT.md) records that no agent may edit, weaken, skip, or delete such a test without explicit human consent; "fix the failing tests" does not count.Compatibility
Nothing to reconfigure: the input set is unchanged, no new permissions are needed (Actions cache requires none), and
pull_requestworkflows keep working as they are.pull_request_targetis now also accepted for repos that want automatic runs and/codeboardingto share one cache scope, butpull_requeststays the documented default.Verification
test_installs_core_manifest_and_preserves_user_configuration, predates this branch and is macOS-only (/bin/bash3.2 has nomapfile); CI runs bash 5.generate_analysis_incrementalhas an explicit empty-delta path, so a re-run at an unchanged head is a valid cheap replay, and incremental depth resolves frommetadata.depth_cap, which is what the chain check compares.create-github-app-tokenwarnings.🤖 Generated with Claude Code