Skip to content

feat: catch up the review base from the nearest saved ancestor - #140

Open
Svilen-Stefanov wants to merge 1 commit into
feat/base-provenancefrom
feat/ancestor-base-seed
Open

Svilen-Stefanov wants to merge 1 commit into
feat/base-provenancefrom
feat/ancestor-base-seed

Conversation

@Svilen-Stefanov

@Svilen-Stefanov Svilen-Stefanov commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

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 through GET /repos/{repo}/actions/artifacts?per_page=100 newest first, keeping non-expired codeboarding-base-<cfg>-* artifacts produced by a run on the repository's own code (the same head_repository_id == repository_id rule as fetch-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 .codeboardingignore and health configuration. It downloads it through fetch-state.sh, so the exact-name lookup and provenance check run again on the one it uses.
  • Review order is now: exact artifact, committed at the merge base, nearest ancestor (bound: 100 first-parent commits), full. The ancestor seeds an incremental catch-up to the merge base (["incremental", "incremental"] with the head), and the caught-up base is published as the exact codeboarding-base-<cfg>-<merge_base> artifact, so later PRs forking there hit it directly. Fork runs still never publish it.
  • Reporting, in feat: report how the review base analysis was obtained #139's terms: an ancestor catch-up is base_analysis_method=incremental, with the reason naming the ancestor and the commits caught up. Nothing new is added to the vocabulary. A full run's reason is the existing analysis was incompatible… when the merge base has a saved analysis under another configuration, and no usable analysis was available otherwise; there is no separate "too far behind" reason (proving it took compare API calls that only changed the wording).
  • Sync uses the same lookup when the branch has no usable committed baseline (including the tip itself), so the first sync after the setup PR merges catches up from the base the setup PR's preview review saved instead of running full. The token is unset before the engine runs. force_full still skips all seeding.
  • FORCE_FULL is lowercased with tr instead of ${,,}, so analyze.sh and its tests also run on macOS's bash 3.2.
  • Docs: base-resolution table and the lookup in docs/COMMIT_STRATEGY.md.

Since the first review

Rebased onto #139's new reporting (base_analysis_method / base_analysis_reason), squashed to one commit. The ancestor path reports incremental instead of adding ancestor; too_far_behind and its compare-API confirmation are removed. The bound stays at 100 first-parent commits (see the thread on #139).

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:

<sub>Base: incremental in 41 s (updated the analysis of 9f8e7d6 to a1b2c3d, 4 commits caught up) · changes 2 m 39 s</sub>

How it was tested

New tests in tests/test_action_state.py (AncestorSeedTests), against real git histories and a stubbed gh: nearest ancestor found within the bound (and the nearer of two wins, published under the merge base's name, reported incremental with its reason), an ancestor beyond the bound (not used, full), a saved analysis off this history (not used, full), 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, a failed deepen falling back to full, and the merge base's config surviving the seed.

python -m unittest discover -s tests: 219 tests, all pass locally (macOS). Black 25.9.0 via the pre-commit hook; shellcheck on all action scripts.

🤖 Generated with Claude Code

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-07T15:34:47.046176Z 32fdc42 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@codeboarding-review

codeboarding-review Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

CodeBoarding review

Status: 0 changed components (no analysed file changed)

See the full change in CodeBoarding.

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;
Loading

Base: incremental in 41 s (updated the analysis of a7675d5 to 6b46ac3, 3 commits caught up) · changes 8 s

download artifacts · run 37991972206

@ivanmilevtues ivanmilevtues left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 walks the merge base's first-parent history up to
100 commits, deepening the shallow checkout, then pages through the
artifact listing newest first, keeping base analyses for this
configuration that a run on the repository's own code produced (the same
provenance rule as fetch-state.sh). It stops at the first page holding a
walked commit (50 pages at most; one or two calls in practice, and only on
a run that would otherwise analyze from scratch). The nearest 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. Seeding keeps the merge base's own
.codeboardingignore and health configuration.

Reported as base_analysis_method=incremental, the reason naming the
ancestor and the commits caught up. A saved analysis of the merge base
under another configuration makes a full run's reason "incompatible";
anything else is "no usable analysis was available". There is no separate
"too far behind" reason: proving it took compare API calls that only
changed the wording.

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>
@Svilen-Stefanov
Svilen-Stefanov force-pushed the feat/ancestor-base-seed branch from 32fdc42 to 3ca8dc1 Compare October 9, 2026 21:12
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

@Svilen-Stefanov

Copy link
Copy Markdown
Contributor Author

Rebased onto the reworked #139 and squashed to one commit (3ca8dc1). With the new vocabulary this PR adds nothing to the reporting: an ancestor catch-up is just incremental with the ancestor named in the reason. I also dropped too_far_behind and the compare-API calls that existed only to word it. Ready for another look whenever you have time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants