Remove inappropriate usermod environment changes#5720
Conversation
Catch any incorrect debug prints for all targets.
WalkthroughUsermod build flags were updated in PlatformIO config and usermod override files, and the usermods GitHub Actions workflow now builds a per-usermod, per-environment matrix from override samples or the shared override. ChangesUsermod build and workflow
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🤖 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 `@platformio.ini`:
- Line 717: The generic [env:usermods] configuration is leaking a touch pin
default into every custom_usermods build. Remove TOUCH_CS from the shared
usermods env and move it into the specific environment that actually needs that
wiring, using the relevant env block in platformio.ini so other usermods can
define their own pin settings.
🪄 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
Run ID: 2daaed02-dbe9-41a8-99ea-1b1c63bc2710
📒 Files selected for processing (2)
platformio.iniusermods/platformio_override.usermods.ini
| extends = env:esp32dev | ||
| build_flags = ${common.build_flags} ${esp32_idf_V4.build_flags} -D WLED_RELEASE_NAME=\"ESP32_USERMODS\" | ||
| -DTOUCH_CS=9 -DWLED_USE_SD_SPI ;; help a few usermods that require special flags to compile | ||
| -DTOUCH_CS=9 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== platformio.ini line count =="
wc -l platformio.ini
echo
echo "== around referenced line 717 =="
nl -ba platformio.ini | sed -n '690,740p'
echo
echo "== all TOUCH_CS occurrences =="
rg -n "TOUCH_CS|custom_usermods|\\[env:usermods\\]|\\[env:" platformio.ini
echo
echo "== env headers around usermods =="
python3 - <<'PY'
from pathlib import Path
text = Path('platformio.ini').read_text().splitlines()
for i, line in enumerate(text, 1):
if line.strip().startswith('[env:') or 'TOUCH_CS' in line or 'custom_usermods' in line:
start = max(1, i-3)
end = min(len(text), i+3)
print(f"\n--- lines {start}-{end} ---")
for j in range(start, end+1):
print(f"{j:4d}: {text[j-1]}")
PYRepository: wled/WLED
Length of output: 272
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== platformio.ini context around TOUCH_CS =="
nl -ba platformio.ini | sed -n '700,730p'
echo
echo "== all env headers and custom_usermods references =="
rg -n "^\[env:|custom_usermods|TOUCH_CS" platformio.ini
echo
echo "== nearby override envs if present =="
python3 - <<'PY'
from pathlib import Path
lines = Path('platformio.ini').read_text().splitlines()
for i, line in enumerate(lines, 1):
if line.strip().startswith('[env:') and ('usermods' in line.lower() or 'override' in line.lower() or 'touch' in line.lower()):
start = max(1, i-2)
end = min(len(lines), i+18)
print(f"\n--- {i}: {line} ---")
for j in range(start, end+1):
print(f"{j:4d}: {lines[j-1]}")
PYRepository: wled/WLED
Length of output: 232
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lines 700-730 with line numbers =="
awk 'NR>=700 && NR<=730 { printf "%4d: %s\n", NR, $0 }' platformio.ini
echo
echo "== all occurrences of TOUCH_CS, custom_usermods, and relevant env headers =="
python3 - <<'PY'
from pathlib import Path
lines = Path('platformio.ini').read_text().splitlines()
for i, line in enumerate(lines, 1):
if 'TOUCH_CS' in line or 'custom_usermods' in line or line.strip().startswith('[env:'):
print(f"{i:4d}: {line}")
PY
echo
echo "== locate [env:usermods] block =="
python3 - <<'PY'
from pathlib import Path
lines = Path('platformio.ini').read_text().splitlines()
start = None
for i, line in enumerate(lines, 1):
if line.strip() == '[env:usermods]':
start = i
break
if start is None:
print("missing [env:usermods]")
raise SystemExit(0)
end = len(lines)
for j in range(start + 1, len(lines) + 1):
if lines[j-1].startswith('[env:') and j != start:
end = j - 1
break
for j in range(start, min(end, start + 40) + 1):
print(f"{j:4d}: {lines[j-1]}")
PYRepository: wled/WLED
Length of output: 5366
Keep [env:usermods] generic.
TOUCH_CS=9 in [env:usermods] forces every custom_usermods = * build to inherit the same touch pin. Move it to the specific env that needs it so usermods with different wiring don't pick up the wrong default.
🤖 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 `@platformio.ini` at line 717, The generic [env:usermods] configuration is
leaking a touch pin default into every custom_usermods build. Remove TOUCH_CS
from the shared usermods env and move it into the specific environment that
actually needs that wiring, using the relevant env block in platformio.ini so
other usermods can define their own pin settings.
Source: Path instructions
There was a problem hiding this comment.
@willmmiles -DTOUCH_CS=9 is actually an older "workaround" for making TFT_eSPI based usermods compile in the CI workflow. Not sure if we should keep this flag?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "TOUCH_CS" --type=cpp --type=ini -g '*.ini' -g '*.h' -g '*.cpp' . || true
rg -n "TOUCH_CS" .Length of output: 331
@softhack007, I searched the repo and TOUCH_CS appears only in platformio.ini (Line 717) — it's not referenced anywhere in the WLED core .cpp/.h sources. That confirms it's purely a compile-time define consumed by an external library's config (likely TFT_eSPI's User_Setup.h/pin-select macros), not something WLED itself depends on.
Given that, and consistent with this PR's stated goal of keeping the shared [env:usermods] environment free of usermod-specific flags:
- If there's currently no usermod in-tree that still needs
TOUCH_CS=9to compile in CI, it's probably safe to drop it entirely from[env:usermods]. - If some TFT_eSPI-based usermod does still need it, it should move into that usermod's own
platformio_override.ini.sample(the same pattern being introduced forWLED_USE_SD_SPI/SD_Card andPWM_fanin this PR), rather than leaking a pin default into everycustom_usermods = *build.
I'll leave the final call to you/willmmiles since you have more context on which usermods currently rely on this workaround in CI.
🧠 Learnings used
Learnt from: netmindz
Repo: wled/WLED PR: 5311
File: platformio.ini:395-395
Timestamp: 2026-01-31T18:19:28.320Z
Learning: In platformio.ini for WLED, understand that usermods can be pulled as out-of-tree dependencies via lib_deps (external repositories). The custom_usermods setting controls only in-tree usermods; setting custom_usermods = (empty) disables in-tree usermods but does not disable out-of-tree usermods included through library dependencies.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: platformio.ini:486-491
Timestamp: 2026-02-10T13:52:36.924Z
Learning: In PlatformIO projects, prefer using build_unflags in the PlatformIO configuration to remove inherited -D defines rather than using GCC's -U option. The -U approach is order-dependent and may be unreliable if the build command order is not guaranteed by PlatformIO. Use build_unflags in platformio.ini to explicitly remove inherited build flags for more robust and maintainable builds.
Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:788-850
Timestamp: 2026-03-31T13:31:01.488Z
Learning: For PlatformIO projects using `platform = pioarduino/platform-espressif32` with dual frameworks (`framework = arduino, espidf`), do not require an explicit `board_build.sdkconfig = sdkconfig.<env_name>` setting. `sdkconfig.<env_name>` (e.g., `sdkconfig.esp32s3_matter_wifi`) is a per-environment build artifact generated by pioarduino from merged board defaults plus the project-root `sdkconfig.defaults`. The project-root `sdkconfig.defaults` is automatically loaded by ESP-IDF’s CMake build, so the absence of `board_build.sdkconfig` should not be flagged as a missing configuration.
Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:788-850
Timestamp: 2026-03-31T13:31:01.488Z
Learning: For PlatformIO projects using `pioarduino/platform-espressif32` with dual framework (`framework = arduino, espidf`), treat `sdkconfig.<env_name>` (e.g., `sdkconfig.esp32s3_matter_wifi`) as a per-environment build artifact generated in the project root. PlatformIO will create it from `sdkconfig.defaults` on first build if it doesn’t exist, and will reuse the existing file on subsequent builds. In code review, do not recommend setting `board_build.sdkconfig = sdkconfig.<env_name>` because that file is an output, not required input. Only use `board_build.cmake_extra_args = -DSDKCONFIG_DEFAULTS=...` to select different defaults files when different environments truly need different SDK config defaults. As a sanity check, if `sdkconfig.<env_name>` is deleted and a rebuild regenerates it with the same content, that indicates `sdkconfig.defaults` is the correct source of truth.
Learnt from: netmindz
Repo: wled/WLED PR: 5543
File: platformio.ini:133-136
Timestamp: 2026-04-26T09:03:12.551Z
Learning: For WLED platformio.ini files: do not mark the audioreactive usermod as “unsupported” for ESP8266 build environments. While ESP8266 can’t process audio locally, it may still receive audioreactive sync data over the network (e.g., from an ESP32 or other source), so the outdated readme note about “ESP8266 is not supported” should be interpreted as only local audio processing. Reviewers should allow audioreactive to be included in ESP8266 environments in platformio.ini when network sync-receive is being used.
Learnt from: softhack007
Repo: wled/WLED PR: 5692
File: platformio.ini:798-799
Timestamp: 2026-06-20T15:08:04.789Z
Learning: For PlatformIO INI files, validate variable interpolation syntax as `${section_name.variable}` where `section_name` is the exact literal section header text (including any `env:` prefix for build environments like `[env:some_name]`). This refers to INI section data, not OS/system environment variables. Therefore, `${env:some_name.variable}` is correct for values in `[env:some_name]` (e.g., `${env:esp32S3_wroom2_32MB.build_unflags}` references `build_unflags` from that `[env:esp32S3_wroom2_32MB]` section); do not flag it as incorrect interpolation.
There was a problem hiding this comment.
I don't think it's actually necessary. TFT_eSPI generates warnings to indicate that the touch subsystem is disabled without it, but that's normal and expected if you don't actually have a touch screen!
I'm almost done the PR for the TFT_eSPI shim anyways. It's been another one where CodeRabbit has been pretty good at finding issues before I push. ;)
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@willmmiles I agree to the principle. The changes were meant to solve a problem with the CI scripts that trigger for usermod changes, especially PR #5679. I've added a custom platformio_override.ini.sample to the SD_Card PR. The CI build used that, but then it still ran the "standard" usermod build targets, in addition to the ones listed inside the usermod folder. I was (wrongly?) assuming this is intended behaviour. The ultimate goal should be that all CI builds are passing, unless there are real errors in the code. So it seems that our usermod-specific CI magic needs to be fixed, too? 
|
Yeah, I'd figured. Generally I'd expect that changes necessary to support a specific PR would be pushed on the PR branch instead of directly to main, though? On the off chance there's some dev with a different opinion .. ;)
In the specific case of #5679, I think the PR is in error (and I'll comment there). IMO it's poor practice to pass module settings using Nevertheless I believe there will be a use case for mandatory environment changes (cf. discusson on #5649) -- so yes, I agree that we probably want to add an exclusion to the CI for modules with specialized environment overrides. I'll add that here. |
Well, in this case i've pushed a change to main, because the overall CI build appeared to have an issue.
👍 |
|
@willmmiles in general, let's use the next team TelCo to discuss our policy for "pushing to main". Right now we still have a very stable My suggestion for the future would be:
that will move us more towards "trunk-based development" -> to be discussed. |
The usermod CI previously ran two parallel tracks: a "plain" build (get_usermod_envs + build) and a separate "custom env" build (get_custom_build_envs + build_custom) for usermods needing bespoke PlatformIO settings. This duplicated most of the gather/build logic. Merge them into a single get_usermod_envs + build pipeline. For each changed usermod, if it supplies a sample ini, use it; otherwise build with the reference platformio_override.usermods.ini. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/usermods.yml (1)
16-20: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAdd a least-privilege
permissions:block to this workflow. This job only needs read access, so setpermissions: { contents: read }at the workflow or job level.🤖 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/usermods.yml around lines 16 - 20, Add a least-privilege permissions block to the usermods workflow so the get_usermod_envs job only has read access; update the workflow-level or job-level configuration in usermods.yml to set permissions to contents: read, keeping the existing pull_request fork-only condition and job definition unchanged.Source: Path instructions
🤖 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/usermods.yml:
- Around line 29-30: The shell step in the usermods workflow directly
interpolates github.event.pull_request.base.sha inside the run script, which
should be avoided. Update the workflow so the base SHA is passed into the step
via an env variable and then referenced from the script, keeping the git diff
logic in the same changed assignment but removing any direct github.event.*
expansion from the run block.
- Around line 98-109: The shell steps in the usermods workflow are using
matrix.usermod and matrix.env directly in run commands, which can allow shell
injection; update the workflow so these matrix values are passed through env
variables and always quoted before use. In the same job, tighten the usermod/env
handling around the sample copy and pio run steps, and add validation for the
extracted env name in the Build firmware step using a safe pattern such as
^[A-Za-z0-9_-]+$ before invoking pio.
---
Outside diff comments:
In @.github/workflows/usermods.yml:
- Around line 16-20: Add a least-privilege permissions block to the usermods
workflow so the get_usermod_envs job only has read access; update the
workflow-level or job-level configuration in usermods.yml to set permissions to
contents: read, keeping the existing pull_request fork-only condition and job
definition 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
Run ID: 4bbe2e12-4a24-4e68-a40e-ee99add9bc3e
📒 Files selected for processing (1)
.github/workflows/usermods.yml
| sample="usermods/${{ matrix.usermod }}/platformio_override.ini.sample" | ||
| if [ -f "$sample" ]; then | ||
| cp -v "$sample" platformio_override.ini | ||
| else | ||
| samples=$(find usermods/ -name "platformio_override.ini.sample" | sort) | ||
| cp -v usermods/platformio_override.usermods.ini platformio_override.ini | ||
| echo >> platformio_override.ini | ||
| echo "custom_usermods = ${{ matrix.usermod }}" >> platformio_override.ini | ||
| fi | ||
| cat platformio_override.ini | ||
|
|
||
| result='[]' | ||
| for sample in $samples; do | ||
| usermod=$(dirname "$sample" | xargs basename) | ||
| # Skip usermods known to be incompatible (same list as get_usermod_envs) | ||
| case "$usermod" in PWM_fan|BME68X_v2|pixels_dice_tray) continue ;; esac | ||
| envs=$(grep -E '^\[env:[^]]+\]' "$sample" | sed 's/^\[env:\(.*\)\]$/\1/') | ||
| for env in $envs; do | ||
| result=$(echo "$result" | jq --arg u "$usermod" --arg e "$env" '. + [{usermod: $u, env: $e}]') | ||
| done | ||
| done | ||
| echo "matrix=$(echo "$result" | jq -c '.')" >> $GITHUB_OUTPUT | ||
| outputs: | ||
| matrix: ${{ steps.custom_envs.outputs.matrix }} | ||
|
|
||
|
|
||
| build_custom: | ||
| name: Build Custom Env (${{ matrix.usermod }} / ${{ matrix.env }}) | ||
| runs-on: ubuntu-latest | ||
| needs: get_custom_build_envs | ||
| if: needs.get_custom_build_envs.outputs.matrix != '[]' | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| include: ${{ fromJSON(needs.get_custom_build_envs.outputs.matrix) }} | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Set up Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version-file: '.nvmrc' | ||
| cache: 'npm' | ||
| - run: npm ci | ||
| - name: Cache PlatformIO | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: | | ||
| ~/.platformio/.cache | ||
| ~/.buildcache | ||
| build_output | ||
| key: pio-${{ runner.os }}-${{ matrix.env }}-${{ hashFiles('platformio.ini', 'pio-scripts/output_bins.py') }}-${{ hashFiles('wled00/**', 'usermods/**') }} | ||
| restore-keys: pio-${{ runner.os }}-${{ matrix.env }}-${{ hashFiles('platformio.ini', 'pio-scripts/output_bins.py') }}- | ||
| - name: Set up Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: '3.12' | ||
| cache: 'pip' | ||
| - name: Install PlatformIO | ||
| run: pip install -r requirements.txt | ||
| - name: Apply custom build environment | ||
| run: cp -v "usermods/${{ matrix.usermod }}/platformio_override.ini.sample" platformio_override.ini | ||
| - name: Build firmware | ||
| run: pio run -e ${{ matrix.env }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
wc -l .github/workflows/usermods.yml
cat -n .github/workflows/usermods.yml | sed -n '1,180p'Repository: wled/WLED
Length of output: 5058
🏁 Script executed:
set -euo pipefail
grep -nE '^(on:|permissions:|- name:|run:|matrix:|grep -oE|custom_usermods|pio run -e)' .github/workflows/usermods.ymlRepository: wled/WLED
Length of output: 153
🏁 Script executed:
set -euo pipefail
wc -l .github/workflows/usermods.yml
cat -n .github/workflows/usermods.yml | sed -n '1,180p'
grep -nE '^(on:|permissions:|- name:|run:|matrix:|grep -oE|custom_usermods|pio run -e)' .github/workflows/usermods.ymlRepository: wled/WLED
Length of output: 5064
Quote the matrix values before using them in shell
matrix.usermod and matrix.env are interpolated directly into run: scripts. A fork can supply shell metacharacters through a changed usermod path or platformio_override.ini.sample, which turns into command execution on the runner. Move both values into env: and quote them, and restrict extracted env names to a safe pattern like ^[A-Za-z0-9_-]+$.
🤖 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/usermods.yml around lines 98 - 109, The shell steps in the
usermods workflow are using matrix.usermod and matrix.env directly in run
commands, which can allow shell injection; update the workflow so these matrix
values are passed through env variables and always quoted before use. In the
same job, tighten the usermod/env handling around the sample copy and pio run
steps, and add validation for the extracted env name in the Build firmware step
using a safe pattern such as ^[A-Za-z0-9_-]+$ before invoking pio.
Source: Path instructions
There was a problem hiding this comment.
Is this new to the PR, or was the previous logic also vulnerable?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cd /tmp
gh pr view 5720 --repo wled/WLED --json baseRefOid,headRefOid,baseRefName,headRefName
gh api repos/wled/WLED/contents/.github/workflows/usermods.yml?ref=$(gh pr view 5720 --repo wled/WLED --json baseRefOid --jq .baseRefOid) --jq .content | base64 -d > usermods_base.yml
grep -nE 'matrix\.(usermod|env|environment)' usermods_base.ymlLength of output: 1655
@willmmiles Good question — I checked the base commit (bc2c80d, pre-PR main) and the same pattern already existed there:
88: echo "custom_usermods = ${{ matrix.usermod }}" >> platformio_override.ini
92: run: pio run -e ${{ matrix.environment }}
...
171: run: cp -v "usermods/${{ matrix.usermod }}/platformio_override.ini.sample" platformio_override.ini
173: run: pio run -e ${{ matrix.env }}
So this isn't new to your PR — the previous logic (both the build and build_custom jobs) already interpolated matrix.usermod/matrix.environment/matrix.env directly into run: steps without quoting or env: indirection. Your change just renamed matrix.environment to matrix.env and merged the two jobs, carrying the same pattern forward.
That said, worth noting the practical exposure is bounded here: this workflow only runs for pull_request events where head.repo.full_name != github.repository (i.e., fork PRs), and the usermod/env values are derived from directory names and [env:...] section names in files under usermods/, not from arbitrary free-text PR input — so exploitability would require a crafted usermod directory name or ini section name in the fork's own changed files. Still, hardening this (passing values via env: and validating with ^[A-Za-z0-9_-]+$) is a reasonable follow-up given it touches fork-triggered CI.
Since this predates your PR, I won't hold up this PR on it — happy to open a separate tracking issue if you'd like.
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5480
File: .github/copilot-instructions.md:73-73
Timestamp: 2026-04-05T23:41:30.189Z
Learning: When reviewing the repository’s `.github/` directory (any files under `.github/`), treat the literal filesystem path segment `.github/` as **mandatory lowercase**. If static analysis tools suggest changing `.github/workflows/...` (or similar `.github/...` paths) from `github` to `GitHub`, mark that suggestion as a false positive and do **not** recommend changing the casing in filesystem paths, code, docs, or references to those paths.
There was a problem hiding this comment.
I think it's still worth to check if the vulnerability gap can be closed with a few changes. Even when it predates this PR (so no regression).
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
No objections from me! The changes seem reasonably achievable. I just wanted to be sure it wasn't something I'd done inadvertently - I'm not an expert in Github Actions - and I would like to move quickly on this PR as I consider it important.
FWIW, CodeRabbit has been doing this to me a lot lately. I've been using to check some of my in-progress work before opening a PR, and it keeps finding bugs like this around areas I've been working that have always been there but could really use attention. Makes for better code, but it sure seems like a drag on getting things done... ;)
There was a problem hiding this comment.
Yep, I was hoping that CodeRabbit would start to highlight such improvement opportunities with all the rules I've added to its database.
Especially for code robustness and "cyber security", I think we've collected a lot of technical debt that might "fall onto our feet" one day. I think it's good to spend more time if that makes our codebase better - even at the price of "reducing feature delivery speed" (not sure if my wording makes sense in english ;) ).
I've experienced this in my day job, too: "clean up the module while you work on it" does pay back in terms of quality, even when it's sometimes conflicting with the intuition of "don't fix stuff that's not (proven to be) broken".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
usermods/PWM_fan/platformio_override.ini.sample (1)
7-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated env blocks risk drift from the shared override.
These four
[env:usermods_esp32*]blocks are byte-identical tousermods/platformio_override.usermods.ini. Since the workflow copies this sample verbatim instead of merging it with the shared file, any future change to the shared partitions/build flags (e.g., swappingWLED_DEBUGfor something else) won't automatically propagate here, silently diverging PWM_fan's CI build from the rest.🤖 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 `@usermods/PWM_fan/platformio_override.ini.sample` around lines 7 - 33, The four [env:usermods_esp32*] entries in PWM_fan/platformio_override.ini.sample are duplicated from the shared PlatformIO override, so they can drift out of sync. Update this sample to avoid hardcoding the same partition and build_flags settings in each env block; instead, reference the shared usermods/platformio_override.usermods.ini configuration or centralize the common values so changes to the shared override propagate automatically. Use the env:usermods_esp32, env:usermods_esp32c3, env:usermods_esp32s2, and env:usermods_esp32s3 blocks as the place to remove the duplicated definitions.
🤖 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 `@usermods/PWM_fan/platformio_override.ini.sample`:
- Around line 7-33: The four [env:usermods_esp32*] entries in
PWM_fan/platformio_override.ini.sample are duplicated from the shared PlatformIO
override, so they can drift out of sync. Update this sample to avoid hardcoding
the same partition and build_flags settings in each env block; instead,
reference the shared usermods/platformio_override.usermods.ini configuration or
centralize the common values so changes to the shared override propagate
automatically. Use the env:usermods_esp32, env:usermods_esp32c3,
env:usermods_esp32s2, and env:usermods_esp32s3 blocks as the place to remove the
duplicated definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 83e682e9-89a3-4055-884b-cb489be58c5e
📒 Files selected for processing (2)
.github/workflows/usermods.ymlusermods/PWM_fan/platformio_override.ini.sample
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/usermods.yml
Avoid interpolating GitHub Actions variables directly in to scripts. H/t @coderabbitai
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/build.yml (1)
79-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsistent with usermods.yml pattern; consider hoisting to job-level env.
The
BUILD_ENVenv block is duplicated identically across the "Build firmware" and "Get artifact name from bin filename" steps. Since both steps reference the same value derived frommatrix.environment, moving it to a job-levelenv:would eliminate the duplication.♻️ Optional refactor
build: name: Build Environments runs-on: ubuntu-latest needs: get_default_envs strategy: fail-fast: false matrix: environment: ${{ fromJSON(needs.get_default_envs.outputs.environments) }} + env: + BUILD_ENV: ${{ matrix.environment }} steps: ... - name: Build firmware - env: - BUILD_ENV: ${{ matrix.environment }} run: pio run -e "$BUILD_ENV" - name: Get artifact name from bin filename id: artifact_name - env: - BUILD_ENV: ${{ matrix.environment }} run: |🤖 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/build.yml around lines 79 - 85, The BUILD_ENV environment variable is duplicated across the Build firmware and Get artifact name from bin filename steps in the workflow. Hoist BUILD_ENV to the job-level env for the job that contains these steps, keeping the value sourced from matrix.environment, and remove the repeated step-level env blocks so both steps inherit the same setting.
🤖 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/build.yml:
- Around line 79-85: The BUILD_ENV environment variable is duplicated across the
Build firmware and Get artifact name from bin filename steps in the workflow.
Hoist BUILD_ENV to the job-level env for the job that contains these steps,
keeping the value sourced from matrix.environment, and remove the repeated
step-level env blocks so both steps inherit the same setting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 99f11f04-f882-4e3b-8a48-4a8e534d581b
📒 Files selected for processing (2)
.github/workflows/build.yml.github/workflows/usermods.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/usermods.yml
2 participants
The global usermod test build environments should not be carrying build flags specific to any usermod. If a usermod requires special build flags for test building, it should supply a
platformio_override.ini.samplewith an appropriate setup (see #5649).Summary by CodeRabbit