Fix the Parameter Group Version Check so it can run on pull requests (backport to release/9.1) - #11951
Open
sensei-hacker wants to merge 8 commits into
Open
Fix the Parameter Group Version Check so it can run on pull requests (backport to release/9.1)#11951sensei-hacker wants to merge 8 commits into
sensei-hacker wants to merge 8 commits into
Conversation
The check has failed on every pull request since it was added:
- check-pg-versions.sh declares `local companion` in the top-level loop
that builds the file list. Outside a function bash rejects `local`,
and with `set -e` the script aborts right there with "local: can only
be used in a function", before any file is checked.
- The workflow then inlines the multi-line script output into the
JavaScript source of the github-script step as a single-quoted string
literal, which fails to parse ("SyntaxError: Invalid or unexpected
token") and fails the job.
Drop the `local`, and hand the output to the script through an
environment variable instead of the source text.
Verified locally against a change that adds a field to
telemetryConfig_t: with the PG version bump the script reports "No PG
version issues detected" and exits 0, without the bump it reports the
struct, the unchanged version and the recommended increment and exits 1.
Reuse the CI fixes from iNavFlight#11885 and cover scalar, array and conditional registrations with regression fixtures.
The workflow README still described the shell implementation and its local-testing snippet did not mention python3 or the regression fixtures.
Two commits in this same PR chain drifted apart: d42addb added a workflow guard requiring '### ' in the checker's stdout to treat exit code 1 as a normal detection, but 5efd794 rewrote the checker in Python, which prints "PG version issue: ..." with no '###' anywhere. As a result the workflow hard-fails (exit 2, raw stdout dump) instead of posting the intended PR comment on every genuine detection - the one case this tooling exists for. Verified by running the workflow's own guard logic against the checker's real "issue found" output before and after this fix. Same fix applied to the PR comment step's JS output filter, which keyed on the same stale '###' marker. Also decode git subprocess output with errors='replace' instead of the default strict UTF-8, since a single non-ASCII byte (e.g. a smart quote in a comment) anywhere in a touched .c/.h file would otherwise raise an uncaught UnicodeDecodeError and hard-fail the check with a message that doesn't name the offending file. Added a regression fixture that runs the workflow's actual guard logic (read from the workflow YAML, not duplicated) against a real detected issue, so the two can't silently diverge again.
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 QodoBackport robust parameter group version checks to release/9.1
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1. Fork detections cannot post warnings
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
|
||
| def check(base, head): | ||
| base = git('merge-base', base, head).strip() | ||
| changed = [path for path in git('diff', '--name-only', base + '..' + head).splitlines() if path.endswith(('.c', '.h'))] |
There was a problem hiding this comment.
2. Renamed settings structs skip checks 🐞 Bug ≡ Correctness
check() builds changed with git diff --name-only, then loads both revisions using each resulting path rather than preserving the old and new names of a rename. When a persisted struct's file is renamed while its layout changes, the old definition never enters old_structs, so the intersection at the comparison loop silently excludes it.
Agent Prompt
## Issue description
The checker loses the base-side definition when a parameter-group struct is modified as part of a detected file rename. Because comparison requires the alias in both structure maps, this allows a persisted layout change to pass without a version increase.
## Fix Focus Areas
- .github/scripts/check-pg-versions.py[147-163]
- .github/scripts/test-check-pg-versions.py[53-67]
## Recommended Fix
Collect both old and new paths for changed files, such as by disabling rename collapsing with `git diff --no-renames --name-only`, and load each path from whichever tree contains it. Add a fixture that renames a registered struct header, changes its layout without increasing the version, and expects exit code 1.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| script: | | ||
| // Use the captured output from the previous step | ||
| const output = '${{ steps.pg_check.outputs.output }}'; | ||
| const output = process.env.PG_CHECK_OUTPUT || ''; |
There was a problem hiding this comment.
1. Fork detections cannot post warnings 🐞 Bug ☼ Reliability
The repaired output handoff now reaches github.rest.issues from a workflow triggered directly by pull_request, whose token is read-only for fork-originated runs. When a fork changes a persisted layout without a version bump, the create or update request reaches the catch handler and fails the job without posting the diagnostic comment.
Agent Prompt
## Issue description
The checker executes untrusted pull-request code and then tries to post its result using the same pull-request workflow token. Fork runs cannot obtain the required write access, so genuine detections fail during comment creation instead of publishing the warning.
## Fix Focus Areas
- .github/workflows/pg-version-check.yml[37-143]
- .github/workflows/pr-test-builds.yml[1-36]
## Recommended Fix
Keep checking in the unprivileged `pull_request` workflow, upload the checker result and validated pull-request number as artifacts, and add a privileged `workflow_run` consumer that downloads those artifacts and posts or updates the comment. Follow the repository's existing `pr-test-builds.yml` pattern and do not execute pull-request code in the privileged workflow.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
The check previously only triggered on PRs targeting maintenance-9.x and maintenance-10.x, so bugfixes landing directly on release/9.1 that touch a PG struct got no automated version-bump check at all. Add release/9.1 to the trigger's branch list and update the workflow README's documented scope to match.
|
RAM / Flash usage vs. base commit
See RAM/flash optimization guide for techniques to reduce usage. |
|
Test firmware build ready — commit Download firmware for PR #11951 245 targets built. Find your board's
|
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.
Summary
Backport of #11885 onto
release/9.1- cherry-picks its 6 commits fixing the "Parameter Group Version Check" CI job, since this is a CI/tooling fix rather than a breaking change and should land as early as possible so it flows forward intomaintenance-10.xandmasternaturally (same rationale as #11946 backporting #11875).The original bug:
check-pg-versions.shdeclaredlocal companionoutside a function, so underset -ethe script aborted withlocal: can only be used in a functionbefore checking anything - breaking this CI job on every PR that touches a.c/.hfile undersrc/. #11885 replaces the bash implementation with a Python checker (check-pg-versions.py) that also fixes several correctness gaps: cross-file struct/registration matching,#if/#ifdefconditional-variant awareness, and comparing againstgit merge-baseinstead of the branch tip so unrelated base-branch commits landing after the PR opened aren't misattributed as PR changes.One additional commit was needed to make this actually work correctly, found by the mandatory
inav-code-reviewpass (see below) - not a release/9.1-specific divergence, but a real bug in the original 6 commits:Fix PG checker output-format mismatch and non-UTF-8 file handling- two commits within Fix the Parameter Group Version Check so it can run on pull requests #11885's own chain drifted apart:d42addb88fadded a workflow guard requiring'### 'in the checker's stdout to treat a detection (exit 1) as normal, but5efd794951rewrote the checker in Python, which prints"PG version issue: ..."with no###anywhere. As shipped, the workflow would hard-fail (exit 2, raw stdout dump, no PR comment) on every genuine detection - the one case this tooling exists for. Fixed both the bash guard and the PR-comment JS's output filter to match the actual output format, and decoded git subprocess output witherrors='replace'so a stray non-ASCII byte in an unrelated comment can't raise an uncaughtUnicodeDecodeErrorand hard-fail the check with a message that doesn't name the file. Added a regression fixture that runs the workflow's own guard logic (read live from the YAML, not duplicated) against a real detected issue, so the two can't silently diverge again.release/9.1's own copy ofpg-version-check.yml(likemaintenance-10.x's, prior to #11885) previously only triggered on PRs targetingmaintenance-9.x/maintenance-10.x, so bugfixes landing directly onrelease/9.1that touch a PG struct got no automated version-bump check at all. Addedrelease/9.1to the trigger's branch list (and updated.github/workflows/README.md's documented scope to match), so this backport now also givesrelease/9.1its own PR coverage, not just a clean forward-merge path.Changes
.github/scripts/check-pg-versions.shas a thin wrapper around a new.github/scripts/check-pg-versions.py, which fixes thelocal-outside-a-function crash and adds cross-file/conditional-variant PG version checking.github/scripts/test-check-pg-versions.pywith 17 fixture cases (16 from the original PR plus 1 new regression fixture added in this backport).github/workflows/pg-version-check.ymland.github/workflows/README.mdaccordinglyrelease/9.1to the check's trigger branches, so it now runs onrelease/9.1PRs too, not justmaintenance-9.x/maintenance-10.xTesting
test-check-pg-versions.py) on this branch: all passrelease/9.1's pre-fixcheck-pg-versions.sh:local: can only be used in a functionorigin/<base>,GITHUB_BASE_REF/GITHUB_HEAD_REFenv, runcheck-pg-versions.sh) against a real change tosrc/main/fc/stats.h/stats.c(statsConfig_t): correctly flagged the missing version bump, and correctly passed once the version was bumpedinav-code-reviewpass caught that this previously hard-failed on any real detection.github/workflows/pg-version-check.yml's YAML after addingrelease/9.1to the trigger branches; re-ran the full 17-fixture suite (unaffected, since the trigger-branch list isn't exercised by the fixtures)Code Review
Reviewed with the
inav-code-reviewagent, which verified by direct execution (not just reading the diff) that the workflow's output-parsing guard and PR-comment JS filter were never updated to match the Python checker's output format, and that git subprocess output was decoded with strict UTF-8. Both fixed in this PR's additional commit above. The agent also flagged a MINOR gap (attribute-decorated struct declarations liketypedef struct PG_PACKED { ... }aren't recognized by the struct-detection regex) which isn't hit by any currently-registered PG struct; left as-is per the agent's own assessment that this is a latent gap rather than an active issue, worth a doc note rather than a code change.Related
Backport of #11885 for
release/9.1(original PR targetsmaintenance-10.x; a maintainer should confirm this is later reconciled/isn't otherwise superseded once both branches converge). Author of the original 6 commits is Raffi1202.https://claude.ai/code/session_01YbtjVLXNSuAxcW81c1SuiY