From 9b7fb2c31e4420764dc4a11d6ae1e88878d82e79 Mon Sep 17 00:00:00 2001 From: Ray Morris Date: Mon, 14 Sep 2026 17:03:34 -0500 Subject: [PATCH 1/3] Fix PG checker output-format mismatch and non-UTF-8 file handling Two commits in this same PR chain drifted apart: d42addb88f added a workflow guard requiring '### ' in the checker's stdout to treat exit code 1 as a normal detection, but 5efd794951 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. --- .github/scripts/check-pg-versions.py | 2 +- .github/scripts/test-check-pg-versions.py | 30 ++++++++++++++++++++++- .github/workflows/pg-version-check.yml | 4 +-- 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/.github/scripts/check-pg-versions.py b/.github/scripts/check-pg-versions.py index b6964d05d6d..2122215491a 100644 --- a/.github/scripts/check-pg-versions.py +++ b/.github/scripts/check-pg-versions.py @@ -14,7 +14,7 @@ def git(*args, allow_missing=False): - result = subprocess.run(['git', *args], capture_output=True, text=True) + result = subprocess.run(['git', *args], capture_output=True, text=True, encoding='utf-8', errors='replace') if result.returncode and not (allow_missing and result.returncode == 1): raise RuntimeError(result.stderr.strip() or 'git command failed') return result.stdout diff --git a/.github/scripts/test-check-pg-versions.py b/.github/scripts/test-check-pg-versions.py index 888bd851d12..548cfabdf4d 100644 --- a/.github/scripts/test-check-pg-versions.py +++ b/.github/scripts/test-check-pg-versions.py @@ -1,5 +1,6 @@ -import subprocess,tempfile,pathlib,os +import re,subprocess,tempfile,pathlib,os script=str(pathlib.Path(__file__).with_name('check-pg-versions.sh').resolve()) +workflow=pathlib.Path(__file__).parents[1]/'workflows'/'pg-version-check.yml' cases=[('unchanged',False,False,[1],[1],0),('missing bump',True,False,[1],[1],1),('bumped',True,False,[1],[2],0),('array missing',True,True,[4],[4],1),('array bumped',True,True,[4],[5],0),('conditional bumped',True,True,[4,1],[5,2],0),('conditional partial',True,True,[4,1],[5,1],1),('conditional decreased',True,True,[4,1],[3,2],1)] for label,changed,array,old,new,expected in cases: with tempfile.TemporaryDirectory() as d: @@ -64,3 +65,30 @@ def git(*a): return subprocess.run(['git','-c','user.name=CI Test','-c','user.em result=subprocess.run(['bash',script],cwd=d,capture_output=True,text=True,env=env) print('advanced base uses merge-base','exit',result.returncode,'expected',0) assert result.returncode==0,result.stdout+result.stderr + +# The workflow's own "Run PG version check script" step re-parses the checker's stdout +# with a bash guard and (on the next step) a JS filter, both keyed on a literal string. +# Run that guard for real, against the checker's real "issue found" output, so the two +# can't silently drift apart the way they did across two commits in this same PR chain +# (one added a '^### ' guard for the old bash script's Markdown headings, a later one +# rewrote the checker in Python with no '###' anywhere in its output). +run_block=re.search(r"- name: Run PG version check script\n(?:.*\n)*? run: \|\n((?:( {10}.*)?\n)+)",workflow.read_text()) +assert run_block,'could not find the "Run PG version check script" step in ' + str(workflow) +guard=run_block[1] +assert 'check-pg-versions.sh' in guard and 'exit_code' in guard,'unexpected step contents:\n' + guard +with tempfile.TemporaryDirectory() as d: + def git(*a): return subprocess.run(['git','-c','user.name=CI Test','-c','user.email=ci@example.invalid',*a],cwd=d,check=True,capture_output=True,text=True).stdout.strip() + p=pathlib.Path(d);git('init') + (p/'config.h').write_text('typedef struct config_s {\n int old;\n} config_t;\n') + (p/'config.c').write_text('PG_REGISTER_WITH_RESET_FN(config_t, config, PG_CONFIG, 1);\n') + git('add','.');git('commit','-m','base');base=git('rev-parse','HEAD') + (p/'config.h').write_text('typedef struct config_s {\n int old;\n int added;\n} config_t;\n') + git('add','.');git('commit','--allow-empty','-m','head, missing version bump') + git('update-ref','refs/remotes/origin/test-base',base) + wrapper='#!/bin/bash\nset -e\ncd ' + d + '\n' + guard.replace('.github/scripts/check-pg-versions.sh', script) + outputs=str(p/'github_output') + env=dict(os.environ,GITHUB_BASE_REF='test-base',GITHUB_HEAD_REF='feature',GITHUB_OUTPUT=outputs) + result=subprocess.run(['bash','-c',wrapper],capture_output=True,text=True,env=env) + print('workflow guard accepts a real detected issue','exit',result.returncode,'expected',0) + assert result.returncode==0,'the workflow step would hard-fail instead of posting a comment:\n'+result.stdout+result.stderr + assert 'exit_code=1' in pathlib.Path(outputs).read_text(),'workflow step did not record the issue for the comment step' diff --git a/.github/workflows/pg-version-check.yml b/.github/workflows/pg-version-check.yml index c9caac7eaaf..660ab5d7053 100644 --- a/.github/workflows/pg-version-check.yml +++ b/.github/workflows/pg-version-check.yml @@ -42,7 +42,7 @@ jobs: # The output is captured and encoded to be passed between steps. output=$(bash .github/scripts/check-pg-versions.sh 2>&1) exit_code=$? - if [ "$exit_code" -gt 1 ] || { [ "$exit_code" -eq 1 ] && ! grep -q '^### ' <<< "$output"; }; then + if [ "$exit_code" -gt 1 ] || { [ "$exit_code" -eq 1 ] && ! grep -q '^PG version issue:' <<< "$output"; }; then printf '%s\n' "$output" exit 2 fi @@ -74,7 +74,7 @@ jobs: let issues = []; for (const line of lines) { - if (line.includes('###')) { + if (line.includes('PG version issue:')) { capturing = true; } if (capturing) { From 54f2b3206803041cb97ae554215554ae5b58294a Mon Sep 17 00:00:00 2001 From: Ray Morris Date: Thu, 17 Sep 2026 11:20:37 -0500 Subject: [PATCH 2/3] Fix PG checker missing renamed+layout-changed structs git diff --name-only collapses renames to the new path, so a persisted struct whose header was renamed and its layout changed never loaded its old definition and was silently excluded from the version-bump check. Use --no-renames so both old and new paths are collected, and add a rename regression fixture. --- .github/scripts/check-pg-versions.py | 2 +- .github/scripts/test-check-pg-versions.py | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/.github/scripts/check-pg-versions.py b/.github/scripts/check-pg-versions.py index 2122215491a..448093dda06 100644 --- a/.github/scripts/check-pg-versions.py +++ b/.github/scripts/check-pg-versions.py @@ -146,7 +146,7 @@ def layout(lines, values): 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'))] + changed = [path for path in git('diff', '--no-renames', '--name-only', base + '..' + head).splitlines() if path.endswith(('.c', '.h'))] if not changed: print('No C/H files changed') return 0 diff --git a/.github/scripts/test-check-pg-versions.py b/.github/scripts/test-check-pg-versions.py index 548cfabdf4d..62632277914 100644 --- a/.github/scripts/test-check-pg-versions.py +++ b/.github/scripts/test-check-pg-versions.py @@ -50,6 +50,23 @@ def registration(v): assert result.returncode==expected,result.stdout+result.stderr +# A renamed + layout-changed struct header must still be checked: the old path's +# definition is otherwise lost when git diff collapses the rename to the new name. +with tempfile.TemporaryDirectory() as d: + def git(*a): return subprocess.run(['git','-c','user.name=CI Test','-c','user.email=ci@example.invalid',*a],cwd=d,check=True,capture_output=True,text=True).stdout.strip() + p=pathlib.Path(d);git('init') + (p/'config.h').write_text('typedef struct config_s {\n int old;\n} config_t;\n') + (p/'config.c').write_text('PG_REGISTER_WITH_RESET_FN(config_t, config, PG_CONFIG, 1);\n') + git('add','.');git('commit','-m','base') + git('mv','config.h','renamed_config.h') + (p/'renamed_config.h').write_text('typedef struct config_s {\n int old;\n int added;\n} config_t;\n') + git('add','.');git('commit','-m','head') + env={k:v for k,v in os.environ.items() if k not in ('GITHUB_BASE_REF','GITHUB_HEAD_REF')} + result=subprocess.run(['bash',script],cwd=d,capture_output=True,text=True,env=env) + print('renamed header missing bump','exit',result.returncode,'expected',1) + assert result.returncode==1,result.stdout+result.stderr + + # Advancing the base branch must not make changes outside the PR look like removals. with tempfile.TemporaryDirectory() as d: def git(*a): return subprocess.run(['git','-c','user.name=CI Test','-c','user.email=ci@example.invalid',*a],cwd=d,check=True,capture_output=True,text=True).stdout.strip() From 413cdd252de26dffb1dd6c08a98119393d74af53 Mon Sep 17 00:00:00 2001 From: Ray Morris Date: Thu, 17 Sep 2026 11:51:20 -0500 Subject: [PATCH 3/3] Post PG version warnings from a workflow_run job for fork PRs The pull_request workflow's GITHUB_TOKEN is read-only for fork-originated runs, so its github-script comment step failed with 403 and hard-failed the job instead of warning. Move comment posting to a privileged workflow_run consumer: the check workflow uploads the checker output and PR number as artifacts, and the consumer downloads them and posts/updates the comment (mirroring pr-test-builds.yml). --- .../workflows/pg-version-check-comment.yml | 122 ++++++++++++++++++ .github/workflows/pg-version-check.yml | 99 ++------------ 2 files changed, 136 insertions(+), 85 deletions(-) create mode 100644 .github/workflows/pg-version-check-comment.yml diff --git a/.github/workflows/pg-version-check-comment.yml b/.github/workflows/pg-version-check-comment.yml new file mode 100644 index 00000000000..5481b22e580 --- /dev/null +++ b/.github/workflows/pg-version-check-comment.yml @@ -0,0 +1,122 @@ +name: Parameter Group Version Check Comment + +# Posts the PG version warning comment from a privileged context so it also works +# for PRs opened from forks, whose pull_request GITHUB_TOKEN is read-only. The +# unprivileged "Parameter Group Version Check" workflow uploads the checker output +# and PR number as artifacts; this workflow only downloads them — it never executes +# pull-request code. +on: + workflow_run: + workflows: ["Parameter Group Version Check"] + types: [completed] + +jobs: + comment: + runs-on: ubuntu-latest + # Only act on pull_request-triggered runs that succeeded (a checker error fails + # the check job, so its conclusion is not 'success'). + if: > + github.event.workflow_run.event == 'pull_request' && + github.event.workflow_run.conclusion == 'success' + permissions: + actions: read + issues: write + pull-requests: write + + steps: + - name: Download PG check result + uses: actions/download-artifact@v4 + with: + name: pg-check-result + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ secrets.GITHUB_TOKEN }} + + - name: Post or update comment + uses: actions/github-script@v7 + with: + script: | + const fs = require('fs'); + const prNumber = Number(fs.readFileSync('pr_number.txt', 'utf8').trim()); + const output = fs.readFileSync('pg_output.txt', 'utf8'); + + if (!output.includes('PG version issue:')) { + console.log('No PG version issues to report; nothing to do.'); + return; + } + + let issuesContent = ''; + + try { + // Extract issues from output (everything after the warning line) + const lines = output.split('\n'); + let capturing = false; + let issues = []; + + for (const line of lines) { + if (line.includes('PG version issue:')) { + capturing = true; + } + if (capturing) { + issues.push(line); + } + } + + issuesContent = issues.join('\n'); + } catch (err) { + console.log('Error capturing issues:', err); + issuesContent = '*Unable to extract detailed issues*'; + } + + const commentBody = '## ⚠️ Parameter Group Version Check\n\n' + + 'The following parameter groups may need version increments:\n\n' + + issuesContent + '\n\n' + + '**Why this matters:**\n' + + 'Modifying PG struct fields without incrementing the version can cause settings corruption when users flash new firmware. The `pgLoad()` function validates versions and will use defaults if there\'s a mismatch, preventing corruption.\n\n' + + '**When to increment the version:**\n' + + '- ✅ Adding/removing fields\n' + + '- ✅ Changing field types or sizes\n' + + '- ✅ Reordering fields\n' + + '- ✅ Adding/removing packing attributes\n' + + '- ❌ Only changing default values in `PG_RESET_TEMPLATE`\n' + + '- ❌ Only changing comments\n\n' + + '**Reference:**\n' + + '- [Parameter Group Documentation](../docs/development/parameter_groups/)\n' + + '- Example: [PR #11236](https://github.com/iNavFlight/inav/pull/11236) (field removal requiring version increment)\n\n' + + '---\n' + + '*This is an automated check. False positives are possible. If you believe the version increment is not needed, please explain in a comment.*'; + + try { + // Check if we already commented + const { data: comments } = await github.rest.issues.listComments({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + }); + + const botComment = comments.find(comment => + comment.user.login === 'github-actions[bot]' && + comment.body.includes('Parameter Group Version Check') + ); + + if (botComment) { + // Update existing comment + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: botComment.id, + body: commentBody + }); + console.log('Updated existing PG version check comment'); + } else { + // Post new comment + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + body: commentBody + }); + console.log('Posted new PG version check comment'); + } + } catch (err) { + core.setFailed(`Failed to post comment: ${err}`); + } diff --git a/.github/workflows/pg-version-check.yml b/.github/workflows/pg-version-check.yml index 660ab5d7053..bf0d3887670 100644 --- a/.github/workflows/pg-version-check.yml +++ b/.github/workflows/pg-version-check.yml @@ -19,7 +19,6 @@ jobs: runs-on: ubuntu-latest permissions: contents: read - pull-requests: write steps: - name: Checkout PR code @@ -54,90 +53,20 @@ jobs: GITHUB_BASE_REF: ${{ github.base_ref }} GITHUB_HEAD_REF: ${{ github.head_ref }} - - name: Post comment if issues found - if: steps.pg_check.outputs.exit_code == '1' - uses: actions/github-script@v7 + - name: Stage PG check result for the comment workflow env: - # Passed through the environment: inlining the multi-line script output - # into the JavaScript source breaks the string literal (SyntaxError). + # Multi-line output travels through the environment so shell + # metacharacters in it are not reinterpreted. PG_CHECK_OUTPUT: ${{ steps.pg_check.outputs.output }} - with: - script: | - // Use the captured output from the previous step - const output = process.env.PG_CHECK_OUTPUT || ''; - let issuesContent = ''; - - try { - // Extract issues from output (everything after the warning line) - const lines = output.split('\n'); - let capturing = false; - let issues = []; - - for (const line of lines) { - if (line.includes('PG version issue:')) { - capturing = true; - } - if (capturing) { - issues.push(line); - } - } - - issuesContent = issues.join('\n'); - } catch (err) { - console.log('Error capturing issues:', err); - issuesContent = '*Unable to extract detailed issues*'; - } - - const commentBody = '## ⚠️ Parameter Group Version Check\n\n' + - 'The following parameter groups may need version increments:\n\n' + - issuesContent + '\n\n' + - '**Why this matters:**\n' + - 'Modifying PG struct fields without incrementing the version can cause settings corruption when users flash new firmware. The `pgLoad()` function validates versions and will use defaults if there\'s a mismatch, preventing corruption.\n\n' + - '**When to increment the version:**\n' + - '- ✅ Adding/removing fields\n' + - '- ✅ Changing field types or sizes\n' + - '- ✅ Reordering fields\n' + - '- ✅ Adding/removing packing attributes\n' + - '- ❌ Only changing default values in `PG_RESET_TEMPLATE`\n' + - '- ❌ Only changing comments\n\n' + - '**Reference:**\n' + - '- [Parameter Group Documentation](../docs/development/parameter_groups/)\n' + - '- Example: [PR #11236](https://github.com/iNavFlight/inav/pull/11236) (field removal requiring version increment)\n\n' + - '---\n' + - '*This is an automated check. False positives are possible. If you believe the version increment is not needed, please explain in a comment.*'; - - try { - // Check if we already commented - const { data: comments } = await github.rest.issues.listComments({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.issue.number, - }); - - const botComment = comments.find(comment => - comment.user.login === 'github-actions[bot]' && - comment.body.includes('Parameter Group Version Check') - ); + run: | + echo "${{ github.event.pull_request.number }}" > pr_number.txt + printf '%s\n' "$PG_CHECK_OUTPUT" > pg_output.txt - if (botComment) { - // Update existing comment - await github.rest.issues.updateComment({ - owner: context.repo.owner, - repo: context.repo.repo, - comment_id: botComment.id, - body: commentBody - }); - console.log('Updated existing PG version check comment'); - } else { - // Post new comment - await github.rest.issues.createComment({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.issue.number, - body: commentBody - }); - console.log('Posted new PG version check comment'); - } - } catch (err) { - core.setFailed(`Failed to post comment: ${err}`); - } + - name: Upload PG check result + uses: actions/upload-artifact@v4 + with: + name: pg-check-result + path: | + pr_number.txt + pg_output.txt + retention-days: 1