Fix size-diff baseline comparison: compare against the PR's merge-base commit, not the branch tip - #11835
Merged
sensei-hacker merged 3 commits intoAug 29, 2026
Conversation
Contributor
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
PR Summary by QodoCompare size diffs against merge-base baselines
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. Publish script is unavailable
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+113
to
+117
| bash .github/scripts/publish-size-baseline.sh \ | ||
| iNavFlight/pr-test-builds \ | ||
| "${BRANCH}" \ | ||
| "${COMMIT_SHA}" \ | ||
| size-report.json |
There was a problem hiding this comment.
1. Publish script is unavailable 🐞 Bug ≡ Correctness
The publish-baseline job invokes .github/scripts/publish-size-baseline.sh without first checking out the repository, so every eligible nightly run fails with “No such file or directory” and publishes no baseline. Because the job declares only actions: read, adding checkout also requires contents: read.
Agent Prompt
## Issue description
The publish job invokes a repository-local script without checking out the repository, so baseline publishing fails.
## Issue Context
The separate PR-comment job checks out trusted default-branch content before using local scripts. The publish job also needs `contents: read` in its explicit permissions.
## Fix Focus Areas
- .github/workflows/ci-size-report.yml[48-50]
- .github/workflows/ci-size-report.yml[82-98]
- .github/workflows/ci-size-report.yml[113-117]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| MERGE_BASE_SHA=${4:?usage: fetch-size-baseline.sh <main-repo> <builds-repo> <base-ref> <merge-base-sha> <out-dir>} | ||
| OUT_DIR=${5:?usage: fetch-size-baseline.sh <main-repo> <builds-repo> <base-ref> <merge-base-sha> <out-dir>} | ||
|
|
||
| MAX_WALK=30 |
There was a problem hiding this comment.
4. Ancestor search misses baselines 🐞 Bug ≡ Correctness
The fallback stops after 30 commits even though publishing retains 50 baselines per branch, and retained baselines are counted per nightly build rather than per intervening commit. A merge-base can therefore have an available retained ancestor more than 30 commits back, yet fetch-size-baseline.sh reports found=false and suppresses the size comparison.
Agent Prompt
## Issue description
The fixed 30-commit ancestry limit can terminate before reaching a retained per-commit baseline.
## Issue Context
Retention counts published baselines, not commits, and the nightly workflow is path-filtered, so more than 30 commits may separate two baseline-producing pushes. Make the search bound consistent with actual retained history or traverse until an appropriate retained boundary.
## Fix Focus Areas
- .github/scripts/fetch-size-baseline.sh[43-43]
- .github/scripts/fetch-size-baseline.sh[95-109]
- .github/scripts/publish-size-baseline.sh[35-36]
- .github/workflows/nightly-build.yml[5-20]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
Author
|
Code review pass complete: the review flagged one CRITICAL issue (pruning's branch-grouping jq capture lacked the (?m) flag — verified: per-commit baselines would all collapse into one bucket and pruning would wipe the less-active branches' baselines) plus robustness items. All fixed in |
|
RAM / Flash usage vs. base branch — commit
See RAM/flash optimization guide for techniques to reduce usage. |
|
Test firmware build ready — commit Download firmware for PR #11835 247 targets built. Find your board's
|
The size-diff workflow stored one baseline per branch, clobbered on every push, so PRs were compared against the base branch's LATEST tip. When the base advances after a PR forks, the delta includes unrelated changes (PR iNavFlight#11785 showed a stale +9,788 B RAM figure that actually belonged to iNavFlight#11438; the PR's real marginal RAM was ~120 B). Publish per-commit baselines alongside the branch-tip pointer: - publish-baseline now stores size-baseline-<COMMIT_SHA> (primary) in addition to size-baseline-<BRANCH> (backward-compat latest-tip pointer), and prunes per-commit baselines to the newest 50 per branch (plus a global cap) so iNavFlight/pr-test-builds can't grow unbounded. Per-commit releases carry a machine-readable branch marker in their notes so pruning can group them. Resolve the PR's true base commit on the comparison side: - pr-comment computes the merge-base of PR head and base ref via the compare API (verified to resolve fork-PR head SHAs), fetches the exact per-commit baseline, and falls back to the nearest ancestor commit that has one by walking the base branch's first-parent chain. The branch-tip baseline is deliberately NOT a fallback — comparing against it is the stale-delta bug this fixes. All new SHA/ref inputs are regex-validated before use in shell commands or release tags. - The comment header now names the baseline commit used ("vs. base commit `abcd123`"), with a note when a nearest-available baseline was used; the graceful "no size baseline available" path is unchanged. Also fixes two pre-existing size-diff-comment tests whose notable-delta expectations predated NOISE_THRESHOLD_BYTES being raised to 256.
Pruning is housekeeping after the baseline publish; a prune failure must not fail the nightly publish job (the baseline itself already landed). Warn loudly instead.
…, header consistency - publish-size-baseline: jq capture of the notes' `branch:` line needed the (?m) flag — without it, ^/$ anchored to the whole (multi-line) string, the capture never matched, and every per-commit baseline collapsed into the '?' bucket, so pruning treated all branches as one and deleted every baseline of the less-active branches (destructive data loss). Pruning now also tracks an explicit set of all tags instead of relying on awk's create-on-reference side effect. - fetch-size-baseline: the exact merge-base baseline is now retried as the first step of the ancestor walk, so a transient download failure on the exact commit degrades to a nearest-ancestor baseline instead of skipping the exact commit for that run. - size-diff-comment: only name the baseline commit in the header when a baseline report actually exists (no contradictory header), and reword the no-baseline note for accuracy during rollout (pre-existing PRs' merge-bases never advance; rebase refreshes them).
sensei-hacker
force-pushed
the
fix-size-baseline-commit-matching
branch
from
August 29, 2026 01:48
d4d541e to
6a0fb56
Compare
Member
Author
|
Retargeted to |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The RAM/flash size-diff workflow compared PRs against the base branch's latest tip baseline, so when the base advanced after a PR forked, the delta included unrelated changes. Concrete case: PR #11785 showed a stale +9,788 B RAM figure that actually belonged to #11438 (merged to base after the PR forked); the PR's real marginal RAM was ~120 B.
This PR keys the comparison to the PR's true base commit instead.
Changes
publish-size-baseline.sh(new):publish-baselinenow storessize-baseline-<COMMIT_SHA>(primary) iniNavFlight/pr-test-buildsin addition to the existingsize-baseline-<BRANCH>latest-tip pointer (kept for backward compatibility). Per-commit baselines are pruned to the newest 50 per branch (branch tracked via a machine-readablebranch:line in the release notes) plus a global cap of 300, so the companion repo can't grow unbounded. All inputs (branch, SHA, report) are regex-validated before use.fetch-size-baseline.sh(new):pr-commentcomputes the PR's merge-base via the compare API (compare/{base_ref}...{head_sha}— verified to resolve fork-PR head SHAs), fetches the exact per-commit baseline, and if absent walks the base branch's first-parent chain to the nearest nightly-built ancestor. The branch-tip baseline is deliberately not a fallback — comparing against it is the stale-delta bug being fixed. New SHA/ref inputs follow the existing regex-validation discipline.size-diff-comment.js: comment header now names the baseline used ("vs. base commit `abcd123`"), with a note when a nearest-available baseline was used. The graceful "No size baseline is available yet…" path is unchanged.size-diff-comment.test.js: tests for the new header/fallback note; also fixes two pre-existing tests whose notable-delta expectations predatedNOISE_THRESHOLD_BYTESbeing raised to 256..github/workflows/README.md: documents the new behavior.No
ci.ymlchange needed: the PR head SHA comes from theworkflow_runcontext, and the base ref is already carried in thebase-refartifact.Testing
node --test .github/scripts/size-diff-comment.test.js— 23/23 pass (19 original + 4 new; the 2 pre-existing threshold-drift failures are fixed by this PR).bash -non both new scripts; workflow YAML parses cleanly.fetch-size-baseline.shsmoke-tested end-to-end against the live repos (read-only): validates inputs, lists existing per-commit baseline tags, walks merge-base ancestors, and reportsfound=falsegracefully when no baseline exists.Code Review
Reviewed with the inav-code-review agent: one CRITICAL finding (pruning branch-grouping jq capture missing the (?m) flag, which would have collapsed all branches into one prune bucket) plus robustness items — all addressed in d4d541e (see review comment); 23/23 tests pass and the prune selection is verified against multi-branch fixtures.
Related
review-pr11785-terrain-agl-ram(stale +9,788 B baseline finding)