Repository navigation
feat: catch up the review base from the nearest saved ancestor - #140
Svilen-Stefanov wants to merge 2 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
CodeBoarding reviewStatus: 0 changed components (no analysed file changed) See the full change in CodeBoarding. Base: caught up 2 commits from feat/base-provenance @a7675d5, 29 s · changes 10 s graph LR
n_action_scripts["action_scripts"]
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;
|
With no saved base for the merge base and no usable baseline committed there, a review analyzed the merge base from scratch, even when a saved analysis of a commit a few steps below it was sitting in the artifact store. find-ancestor-base.sh lists the repository's artifacts once, keeps the base analyses for this configuration that a run on the repository's own code produced (the same provenance rule as fetch-state.sh), and walks the merge base's first-parent history up to 100 commits, deepening the shallow checkout. The first hit seeds an incremental catch-up to the merge base, which is then published under the merge base's own name. Order: exact artifact, committed at the merge base, nearest ancestor, full. base_source=ancestor reports it, with base_from_sha and catchup_commits. A compatible ancestor that exists only beyond the bound gives base_reason=too_far_behind; a saved analysis of the merge base under another configuration gives incompatible. Sync uses the same lookup when the branch has no usable committed baseline, so the first sync after the setup pull request merges catches up from the base that pull request's review saved. FORCE_FULL is now lowercased with tr, which also runs on the bash 3.2 macOS ships. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…base's config Review follow-ups on the ancestor seed: - The lookup walks first-parent history first, then pages through the artifact listing newest first and stops at the first page holding a walked commit, instead of stopping after 10 pages. A busy repository's bases sat past page 10 and read as no_baseline. The cap is 50 pages; in practice it is one or two calls, and only on a run that would otherwise analyze from scratch. - too_far_behind needs a walk that reached the bound and a saved analysis confirmed (by compare) to be an ancestor, checking up to five rather than only the newest. A failed deepen or another branch's analysis no longer produces it. - Seeding from an ancestor keeps the merge base's own .codeboardingignore and health configuration instead of the ancestor's copies. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2012a60 to
32fdc42
Compare
ivanmilevtues
left a comment
There was a problem hiding this comment.
I think all my comments from #139 are actually for #140.
I think this split in particular is not really good, as it is a stack which defines its funcitonality in the base but doesn't really have it, so I got puzzled. I thought it defined the things which are defined in this PR.
I will proly re-review after we change the language in the base, as that might prily will change how things look here and I won't have to remap names and things
Stacked on #139 (
feat/base-provenance); review and merge that first.What changed and why
With no saved base for the merge base and no usable baseline committed there, a review analyzed the merge base from scratch, even when a saved analysis of a commit a few steps below it was already in the artifact store. That is the slowest path a review has, and the common one right after setup.
scripts/action/find-ancestor-base.sh(new): walks the merge base's first-parent history (git rev-list --first-parent, after deepening the shallow checkout to 101 commits), then pages throughGET /repos/{repo}/actions/artifacts?per_page=100newest first, keeping non-expiredcodeboarding-base-<cfg>-*artifacts produced by a run on the repository's own code (the samehead_repository_id == repository_idrule asfetch-state.sh). It stops at the first page holding a walked commit and takes the nearest one seen. Bases are published as commits are synced or reviewed, so that is one or two calls in practice and 50 pages at most, against GITHUB_TOKEN's 1,000 requests an hour per repository, and only on a run that would otherwise analyze from scratch. Seeding keeps the merge base's own.codeboardingignoreand health configuration. It downloads it throughfetch-state.sh, so the exact-name lookup and provenance check run again on the one it uses.["incremental", "incremental"]with the head), and the caught-up base is published as the exactcodeboarding-base-<cfg>-<merge_base>artifact, so later PRs forking there hit it directly. Fork runs still never publish it.base_source=ancestor,base_from_sha=<A>,catchup_commits=<N>.base_reason=too_far_behindonly when the walk reached the bound and one of up to five saved analyses is confirmed (compare API) to be an ancestor; a failed deepen or another branch's analysis givesno_baseline. A saved analysis of the merge base under another configuration reportsincompatible.force_fullstill skips all seeding.FORCE_FULLis lowercased withtrinstead of${,,}, soanalyze.shand its tests also run on macOS's bash 3.2.docs/COMMIT_STRATEGY.md.Why a committed baseline still ranks above an ancestor artifact
A baseline committed at the merge base is in the checkout already: no listing, no download. It is what sync writes for that branch, and sync publishes artifacts for the same two commits, so an ancestor artifact nearer than the committed baseline's own commit would be one of those, describing the same tree. Ranking it first saves the listing on every review of a repository that commits its baseline.
What it looks like
This PR changes nothing visible in the web platform UI by itself. On GitHub the comment can now say:
How it was tested
New tests in
tests/test_action_state.py(AncestorSeedTests), against real git histories and a stubbedgh: nearest ancestor found within the bound (and the nearer of two wins, published under the merge base's name), ancestor beyond the bound (too_far_behind), a saved analysis off this history (no_baseline, not far behind), an ancestor uploaded by a run on forked code (rejected, never downloaded), another configuration at the merge base (incompatible), a depth-1 clone deepened to find the ancestor, lookup disabled (no API calls), first sync after setup (incremental only), forced sync (no lookup), a busy store paged to page 12 and no further, another branch's newer analysis not hiding a real far ancestor, a cut-short walk never called too far behind, and the merge base's config surviving the seed.python -m unittest discover -s tests: 224 tests, all pass locally (macOS). Black 25.9.0 via the pre-commit hook; shellcheck on all action scripts.🤖 Generated with Claude Code