Fix size report stale baseline#10935
Open
NomDeTom wants to merge 9 commits into
Open
Conversation
Contributor
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (27)
Build artifacts expire on 2026-08-06. Updated for |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR expands firmware size collection and reporting, adds a local size-check script and PlatformIO targets, and updates CI to produce size-status output and post a commit status from the generated report. ChangesFirmware size tracking and reporting
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
bin/collect_sizes.py (1)
69-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication: repeated int/bool guard.
The
isinstance(x, int) and not isinstance(x, bool)check is repeated forram_bytes,max_ram_bytes, andmax_flash_bytes, and the same pattern recurs insize_report.pyandplatformio-custom.py. Could be extracted into a small shared helper (e.g.is_plain_int(v)), but this is purely cosmetic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/collect_sizes.py` around lines 69 - 79, The repeated int-and-not-bool validation for ram_bytes, max_ram_bytes, and max_flash_bytes should be factored into a shared helper to reduce duplication. Add a small reusable helper such as is_plain_int(v) and use it in the collect_sizes flow, then apply the same helper consistently anywhere the same guard appears in size_report.py and platformio-custom.py, keeping the existing behavior unchanged..github/workflows/main_matrix.yml (2)
355-389: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUnguarded template expansion of baseline sha/date into the shell script (zizmor: template-injection).
${{ steps.baseline-develop.outputs.sha }},.date, and the master equivalents are interpolated directly into therun:block rather than passed throughenv:. In practice these values come fromgh run list --json headSha,createdAt(a commit SHA and an API-generated timestamp), so the immediate exploitability is low, but this is still the pattern that lets attacker-influenced strings be spliced into the generated script verbatim before the shell ever sees them. Passing them viaenv:avoids the class of issue entirely and is best practice regardless of current risk.🔒 Proposed fix: route values through env vars
- name: Generate size comparison report if: github.event_name == 'pull_request' id: report + env: + BASELINE_DEVELOP_SHA: ${{ steps.baseline-develop.outputs.sha }} + BASELINE_DEVELOP_DATE: ${{ steps.baseline-develop.outputs.date }} + BASELINE_MASTER_SHA: ${{ steps.baseline-master.outputs.sha }} + BASELINE_MASTER_DATE: ${{ steps.baseline-master.outputs.date }} run: | ... - if [ -n "${{ steps.baseline-develop.outputs.sha }}" ]; then - echo "_Baseline \`develop\` = ${{ steps.baseline-develop.outputs.sha }} (${{ steps.baseline-develop.outputs.date }})_ " + if [ -n "$BASELINE_DEVELOP_SHA" ]; then + echo "_Baseline \`develop\` = $BASELINE_DEVELOP_SHA ($BASELINE_DEVELOP_DATE)_ " fi - if [ -n "${{ steps.baseline-master.outputs.sha }}" ]; then - echo "_Baseline \`master\` = ${{ steps.baseline-master.outputs.sha }} (${{ steps.baseline-master.outputs.date }})_ " + if [ -n "$BASELINE_MASTER_SHA" ]; then + echo "_Baseline \`master\` = $BASELINE_MASTER_SHA ($BASELINE_MASTER_DATE)_ " fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/main_matrix.yml around lines 355 - 389, The Generate size comparison report step is interpolating baseline SHA/date expressions directly inside the shell script, which should be avoided as template-injection risk. Move the baseline values for the develop and master outputs into env variables on the workflow step, then reference those env vars inside the size-report generation logic in the same step. Update the Generate size comparison report block so the shell script uses the env-backed values instead of direct `${{ steps.baseline-* }}` expansion.Source: Linters/SAST tools
293-320: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDevelop/master baseline scan blocks are near-identical, duplicated logic.
Both blocks implement the same "scan up to 20 completed CI runs, download first non-expired
firmware-sizes-*artifact" loop, differing only by branch name and output filenames. This same logic is a third time re-implemented inbin/check-size.sh'sfetch_baseline(). Consider extracting this into a small reusable script (e.g.bin/fetch-ci-baseline.sh <branch> <outfile>) invoked from both workflow steps and fromcheck-size.sh, to avoid the three implementations drifting apart over time.Also applies to: 329-353
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/main_matrix.yml around lines 293 - 320, The develop/master baseline scan logic is duplicated across the workflow steps and the `fetch_baseline()` path in `bin/check-size.sh`, so it should be extracted into one reusable helper. Create a small shared script (for example, a baseline-fetch helper that takes branch and output filename arguments) and have the workflow step that uses the current `gh run list`/artifact download loop, the corresponding master step, and `fetch_baseline()` call that shared implementation instead of maintaining separate copies. Use the existing baseline-scan block and `fetch_baseline()` as the primary places to refactor so the artifact selection and download behavior stays consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/firmware-size-status.yml:
- Around line 21-37: The workflow uses mutable third-party action tags in the
Download size report and Set size status steps, which violates the supply-chain
policy. Update the actions/download-artifact and actions/github-script
references in this workflow to pinned commit SHAs, keeping the same step names
and behavior while replacing the tag-based versions with immutable revisions.
In `@bin/check-size.sh`:
- Around line 101-115: The detect_repo() helper is accepting non-GitHub remotes
as if they were valid owner/repo values because the sed fallback leaves
unmatched URLs unchanged. Update detect_repo() to only return when the GitHub
regex actually matches, using the existing detect_repo, git remote get-url, and
repo parsing logic to verify the substitution succeeded before echoing the
result; otherwise continue checking remotes and ultimately return 1 so callers
get the intended “could not determine repo” path.
In `@bin/platformio-custom.py`:
- Around line 283-293: The repo owner parsing in the git remote-URL lookup only
handles subprocess.CalledProcessError, so it can still crash when git is missing
or the URL shape is unexpected. Update the try/except around the
subprocess.check_output flow to also fall back to "unknown" for
FileNotFoundError and any parsing failures from r_owner indexing in the
repo_owner assignment, keeping the behavior consistent with the existing
fallback path.
- Line 135: The check_paths list in platformio_custom.py only includes
bleota-c3.bin, so the other BLE-OTA artifacts expected by
device-install_test.ps1 are missing from the manifest and size report. Update
the check_paths collection in the relevant manifest/size-report logic to include
bleota-s3.bin and bleota.bin alongside the existing BLE-OTA filename entry,
using the same pattern as the other binary names in that section.
---
Nitpick comments:
In @.github/workflows/main_matrix.yml:
- Around line 355-389: The Generate size comparison report step is interpolating
baseline SHA/date expressions directly inside the shell script, which should be
avoided as template-injection risk. Move the baseline values for the develop and
master outputs into env variables on the workflow step, then reference those env
vars inside the size-report generation logic in the same step. Update the
Generate size comparison report block so the shell script uses the env-backed
values instead of direct `${{ steps.baseline-* }}` expansion.
- Around line 293-320: The develop/master baseline scan logic is duplicated
across the workflow steps and the `fetch_baseline()` path in
`bin/check-size.sh`, so it should be extracted into one reusable helper. Create
a small shared script (for example, a baseline-fetch helper that takes branch
and output filename arguments) and have the workflow step that uses the current
`gh run list`/artifact download loop, the corresponding master step, and
`fetch_baseline()` call that shared implementation instead of maintaining
separate copies. Use the existing baseline-scan block and `fetch_baseline()` as
the primary places to refactor so the artifact selection and download behavior
stays consistent.
In `@bin/collect_sizes.py`:
- Around line 69-79: The repeated int-and-not-bool validation for ram_bytes,
max_ram_bytes, and max_flash_bytes should be factored into a shared helper to
reduce duplication. Add a small reusable helper such as is_plain_int(v) and use
it in the collect_sizes flow, then apply the same helper consistently anywhere
the same guard appears in size_report.py and platformio-custom.py, keeping the
existing behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e44abee-916e-49d3-a26b-65eb909fa4c7
📒 Files selected for processing (7)
.github/workflows/firmware-size-status.yml.github/workflows/main_matrix.ymlbin/check-size.shbin/collect_sizes.pybin/platformio-custom.pybin/size_report.pybin/test_size_scripts.py
Contributor
Firmware Size Report42 targets | no baseline available yet
Show 37 more target(s)
Size budgets
Budgets live in Updated for 9ad0f0e This comment may be deprecated in favor of the |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
bin/fetch-ci-baseline.sh (1)
22-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo validation of required positional arguments.
If any of
$1-$4is omitted by the caller,REPO/BRANCH/OUTFILE/DOWNLOAD_DIRsilently become empty strings, leading to crypticgherrors rather than a clear usage message.♻️ Suggested guard
REPO=$1 BRANCH=$2 OUTFILE=$3 DOWNLOAD_DIR=$4 + +if [ -z "$REPO" ] || [ -z "$BRANCH" ] || [ -z "$OUTFILE" ] || [ -z "$DOWNLOAD_DIR" ]; then + echo "Usage: $0 <repo> <branch> <outfile> <download-dir>" >&2 + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/fetch-ci-baseline.sh` around lines 22 - 25, Add explicit validation for the required positional arguments in the fetch-ci-baseline.sh script right after REPO, BRANCH, OUTFILE, and DOWNLOAD_DIR are assigned from $1-$4. If any value is missing or empty, stop early with a clear usage/error message instead of letting later gh commands fail with unclear errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/fetch-ci-baseline.sh`:
- Line 20: The CI baseline fetch script relies on pipeline behavior in fetch
logic, but `set -e` alone won’t surface failures from the `gh api ... | head -1`
pipeline. Update the shell setup in `fetch-ci-baseline.sh` to enable pipe
failure handling so API/auth/rate-limit/network errors in the `gh api` call are
not mistaken for a missing artifact, and keep the existing loop and baseline
lookup behavior in sync with that error handling.
- Line 36: The baseline fetch logic in the gh run download path can pull stale
firmware-sizes artifacts from an earlier attempt when a run is re-run. Update
the download flow in fetch-ci-baseline.sh to select the latest attempt for the
given run ID or use an attempt-specific artifact API URL instead of relying on
gh run download --name, so reruns always fetch the correct baseline.
---
Nitpick comments:
In `@bin/fetch-ci-baseline.sh`:
- Around line 22-25: Add explicit validation for the required positional
arguments in the fetch-ci-baseline.sh script right after REPO, BRANCH, OUTFILE,
and DOWNLOAD_DIR are assigned from $1-$4. If any value is missing or empty, stop
early with a clear usage/error message instead of letting later gh commands fail
with unclear errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f9f6196-e5e4-4a9d-ab17-1a0171249976
📒 Files selected for processing (5)
.github/workflows/firmware-size-status.yml.github/workflows/main_matrix.ymlbin/check-size.shbin/fetch-ci-baseline.shbin/platformio-custom.py
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/firmware-size-status.yml
- bin/check-size.sh
- .github/workflows/main_matrix.yml
- bin/platformio-custom.py
The size-report baseline was selected with `gh run list --status success --limit 1`, so whenever develop (or master) CI stayed red for a while the baseline silently froze at the last all-green run. The "vs develop" deltas then measured all of develop's drift since that point instead of the PR, producing large, uniform, identical-across-PRs size changes. Pick the newest *completed* run that still has a firmware-sizes artifact regardless of the run's overall conclusion (the artifact is uploaded independently of the rest of CI), and surface the chosen baseline's SHA and date in the report so staleness is visible. Also fall back to the firmware .uf2/.hex in collect_sizes.py for targets that emit no .bin (nRF52, some RP2040) so they are no longer silently dropped, and warn when a target yields no size-bearing artifact. Adds tests for both. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
NomDeTom
force-pushed
the
fix-size-report-stale-baseline
branch
from
8a3f198 to
7537dc7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
bin/collect_sizes.py (1)
66-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated int/bool validation could be extracted into a helper.
The
isinstance(x, int) and not isinstance(x, bool)check is duplicated three times forram_bytes,max_ram_bytes, andmax_flash_bytes(and again at Line 58 for the flash fallback). A small helper would reduce repetition.♻️ Optional refactor
+def _as_valid_int(value): + return value if isinstance(value, int) and not isinstance(value, bool) else None + + def collect_sizes(manifest_dir): ... - ram_bytes = data.get("ram_bytes") - if isinstance(ram_bytes, int) and not isinstance(ram_bytes, bool): - entry["ram_bytes"] = ram_bytes - max_ram_bytes = data.get("max_ram_bytes") - if isinstance(max_ram_bytes, int) and not isinstance(max_ram_bytes, bool): - entry["max_ram_bytes"] = max_ram_bytes - max_flash_bytes = data.get("max_flash_bytes") - if isinstance(max_flash_bytes, int) and not isinstance(max_flash_bytes, bool): - entry["max_flash_bytes"] = max_flash_bytes + for key in ("ram_bytes", "max_ram_bytes", "max_flash_bytes"): + val = _as_valid_int(data.get(key)) + if val is not None: + entry[key] = val🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/collect_sizes.py` around lines 66 - 74, The int/bool validation is duplicated in the size collection logic, so extract the repeated isinstance(value, int) and not isinstance(value, bool) check into a small helper and reuse it for ram_bytes, max_ram_bytes, max_flash_bytes, and the flash fallback in collect_sizes.py. Update the existing assignment flow to call the helper from the same location where the fields are read from data, keeping the current behavior unchanged while reducing repetition.bin/test_size_scripts.py (1)
180-180: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused
stdoutunpacked variable.Ruff (RUF059) flags
stdoutas unused in both new tests.🧹 Proposed fix
- rc, stdout, stderr = run_script("collect_sizes.py", [tmpdir, outfile]) + rc, _stdout, stderr = run_script("collect_sizes.py", [tmpdir, outfile])Apply to both Line 180 and Line 204.
Also applies to: 204-204
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/test_size_scripts.py` at line 180, The test helper call in the size script tests is unpacking an unused stdout value, which triggers Ruff RUF059. Update both test cases that call run_script in bin/test_size_scripts.py to stop binding stdout when it is not used, while keeping rc and stderr intact. Use the run_script call sites in the collect_sizes.py test functions to locate and adjust both unpacking assignments consistently.Source: Linters/SAST tools
.github/workflows/main_matrix.yml (1)
342-351: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winInterpolate baseline sha/date via env vars instead of directly in the
run:block.
${{ steps.baseline-develop.outputs.sha }}/.dateand the master equivalents are expanded directly into the shell heredoc, which is the classic template-injection pattern zizmor flags. The practical risk is low here since these values originate frombin/fetch-ci-baseline.shreading real commit SHAs/dates offdevelop/master(not attacker-controlled PR input), but routing them throughenv:is a cheap, standard mitigation and keeps this consistent with treating all${{ }}expansions inrun:blocks uniformly.🔒 Proposed fix
- name: Generate size comparison report if: github.event_name == 'pull_request' id: report + env: + BASELINE_DEVELOP_SHA: ${{ steps.baseline-develop.outputs.sha }} + BASELINE_DEVELOP_DATE: ${{ steps.baseline-develop.outputs.date }} + BASELINE_MASTER_SHA: ${{ steps.baseline-master.outputs.sha }} + BASELINE_MASTER_DATE: ${{ steps.baseline-master.outputs.date }} run: | ... - if [ -n "${{ steps.baseline-develop.outputs.sha }}" ]; then - echo "_Baseline \`develop\` = ${{ steps.baseline-develop.outputs.sha }} (${{ steps.baseline-develop.outputs.date }})_ " + if [ -n "$BASELINE_DEVELOP_SHA" ]; then + echo "_Baseline \`develop\` = $BASELINE_DEVELOP_SHA ($BASELINE_DEVELOP_DATE)_ " fi - if [ -n "${{ steps.baseline-master.outputs.sha }}" ]; then - echo "_Baseline \`master\` = ${{ steps.baseline-master.outputs.sha }} (${{ steps.baseline-master.outputs.date }})_ " + if [ -n "$BASELINE_MASTER_SHA" ]; then + echo "_Baseline \`master\` = $BASELINE_MASTER_SHA ($BASELINE_MASTER_DATE)_ " fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/main_matrix.yml around lines 342 - 351, The shell block in the workflow comment step is interpolating baseline SHA/date values directly inside the run script, so move the `steps.baseline-develop` and `steps.baseline-master` outputs into `env:` first and reference those environment variables inside the script. Update the comment-generation step in `main_matrix.yml` to use the env-backed values instead of inline `${{ }}` expansions in the heredoc, keeping the existing `fetch-ci-baseline.sh` outputs and the comment formatting intact.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/main_matrix.yml:
- Around line 342-351: The shell block in the workflow comment step is
interpolating baseline SHA/date values directly inside the run script, so move
the `steps.baseline-develop` and `steps.baseline-master` outputs into `env:`
first and reference those environment variables inside the script. Update the
comment-generation step in `main_matrix.yml` to use the env-backed values
instead of inline `${{ }}` expansions in the heredoc, keeping the existing
`fetch-ci-baseline.sh` outputs and the comment formatting intact.
In `@bin/collect_sizes.py`:
- Around line 66-74: The int/bool validation is duplicated in the size
collection logic, so extract the repeated isinstance(value, int) and not
isinstance(value, bool) check into a small helper and reuse it for ram_bytes,
max_ram_bytes, max_flash_bytes, and the flash fallback in collect_sizes.py.
Update the existing assignment flow to call the helper from the same location
where the fields are read from data, keeping the current behavior unchanged
while reducing repetition.
In `@bin/test_size_scripts.py`:
- Line 180: The test helper call in the size script tests is unpacking an unused
stdout value, which triggers Ruff RUF059. Update both test cases that call
run_script in bin/test_size_scripts.py to stop binding stdout when it is not
used, while keeping rc and stderr intact. Use the run_script call sites in the
collect_sizes.py test functions to locate and adjust both unpacking assignments
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eeb47602-6170-48b4-9c3e-063438894ad5
📒 Files selected for processing (8)
.github/workflows/firmware-size-status.yml.github/workflows/main_matrix.ymlbin/check-size.shbin/collect_sizes.pybin/fetch-ci-baseline.shbin/platformio-custom.pybin/size_report.pybin/test_size_scripts.py
🚧 Files skipped from review as they are similar to previous changes (2)
- bin/fetch-ci-baseline.sh
- bin/check-size.sh
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
2 participants
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.
This pull request makes several improvements to the CI workflow for firmware size reporting. The main changes focus on making the firmware size baseline selection more robust, adding a new commit status for firmware size, and improving the output and reporting of size changes for pull requests.
CI Workflow Improvements:
.github/workflows/firmware-size-status.ymlthat posts afirmware-sizecommit status to pull requests, providing an at-a-glance description of firmware size changes. This status is informational only and does not enforce pass/fail..github/workflows/main_matrix.ymlfor bothdevelopandmasterbranches to select the newest completed CI run with a valid (non-expired)firmware-sizesartifact, regardless of overall CI success. This prevents stale baselines when the main branch is red for a while. [1] [2]Reporting and Artifact Changes:
size-status.txtfor use in the new commit status check. [1] [2]developandmaster, and added a note that this comment may be deprecated in favor of the new commit status check.These changes collectively make firmware size tracking more reliable and visible in the GitHub UI, while reducing the risk of stale or misleading size comparisons.
🤝 Attestations
Summary by CodeRabbit
flash_kind), a one-line status summary (--status-out), and a new--format textview..bin, otherwise uses supported fallbacks).