Skip to content

custom_usermod improvements#5403

Merged
netmindz merged 7 commits intowled:mainfrom
willmmiles:custom-usermods-better
Mar 8, 2026
Merged

custom_usermod improvements#5403
netmindz merged 7 commits intowled:mainfrom
willmmiles:custom-usermods-better

Conversation

@willmmiles
Copy link
Member

@willmmiles willmmiles commented Mar 5, 2026

Some small upgrades on the custom_usermods integration:

  • Formalization of the dynamic array system, in a way that should be more broadly compatible (eg. v5)
  • Support lib_deps syntax in custom_usermods.
  • Add an example of an out-of-tree usermod to platformio_override.sample.ini
  • Some minor cleanup

Note that lib_deps syntax uses newlines as delimiters instead of spaces as delimiters. For backwards compatibility, this implementation treats cases that don't look exactly like lib_deps style references as if they're space delimited usermod lists that should come from this repo. If we're up for breaking compatibility, the code would be much simpler if custom_usermods was always newline delimited.

Summary by CodeRabbit

  • New Features

    • Support for external usermods via Git URLs and expanded custom_usermods handling.
    • Dynamic registration of usermods enabling accurate mod counting and a public mod-count API.
  • Build System

    • Platform-aware injection of a linker fragment during post-build to support dynamic sections across targets.
  • Improvements

    • Clearer validation and error messages for usermod packaging and build settings.
    • More robust resolution and ordering of usermod dependencies.

willmmiles and others added 5 commits February 28, 2026 22:47
Use a custom linker section to hold dynamic arrays such as the
usermod list.  This provides an extensible solution for wider use
in the future (such as FX or Bus drivers), as well as IDF v5
compatibility.
Break out the dynamic array macros to a re-usable component and fix
the implementation.
Expand the parsing of custom_usermods to accept the same syntax as
lib_deps.  This allows external usermods to appear on the
custom_usermods lines.  Also includes comment handling.

Co-Authored-By: Claude <noreply@anthropic.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 5, 2026

Walkthrough

Implements a linker-section based dynamic-array mechanism for usermod registration (headers, linker fragment, and post-build script), replaces .dtors registration with dynarray macros across usermod code, and overhauls PlatformIO usermod parsing and ELF-based validation to support external/custom usermods.

Changes

Cohort / File(s) Summary
Dynamic array core
wled00/dynarray.h, tools/dynarray_espressif32.ld, pio-scripts/dynarray.py
Adds dynarray macros and conditional section name; introduces ESP32 linker fragment defining .dynarray placement; post-build script injects platform-specific linker script into LINKFLAGS, with ordering special-case for espressif32.
Usermod registration & usage
wled00/fcn_declare.h, wled00/um_manager.cpp
Replaces REGISTER_USERMOD/.dtors mechanism with DYNARRAY_MEMBER-based registration; updates all usermod iteration sites to use DYNARRAY_BEGIN/END and adds getModCount() and explicit nullptr return in lookup.
PlatformIO scripts — loading
pio-scripts/load_usermods.py, platformio_override.sample.ini, platformio.ini
Adds complex parsing for custom_usermods (URL/SSH/name/wildcard handling), collects external URLs/symlinks into lib_deps, updates sample overrides to demonstrate external custom_usermods, and adds post:pio-scripts/dynarray.py to extra_scripts.
PlatformIO scripts — validation
pio-scripts/validate_modules.py
Removes prior map-file approach and LibBuilderBase dependency; adds ELF-based symbol attribution using nm, introduces _get_nm_path, check_elf_modules, and dynamic USERMODS_SECTION/DYNARRAY_SECTION constants; updates counting and error messaging.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • netmindz
  • softhack007
🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.63% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'custom_usermod improvements' is vague and generic, using non-descriptive language that doesn't convey the specific scope or main technical changes of the changeset. Consider a more specific title that highlights a key change, such as 'Formalize dynamic array system for usermods' or 'Support external usermods with lib_deps syntax'.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (1)
wled00/um_manager.cpp (1)

7-13: Update stale registration comments to match dynarray behavior.

This comment block still describes .dtors as the general mechanism, but non-ESP8266 targets now use .dynarray.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wled00/um_manager.cpp` around lines 7 - 13, The comment above
DECLARE_DYNARRAY(Usermod*, usermods) is outdated about using the '.dtors'
segment; update it to describe current dynarray placement by mentioning that
non-ESP8266 targets use the '.dynarray' section (while ESP8266 may still rely on
'.dtors'), explain zero-length arrays get addresses but consume no flash and the
numeric suffix ensures ordering via the linker, and keep reference to
DECLARE_DYNARRAY and the usermods symbol so future readers know this macro now
maps to the '.dynarray' mechanism on modern targets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pio-scripts/dynarray.py`:
- Around line 14-16: The code naively calls linkflags.index("memory.ld") and
will raise if LINKFLAGS doesn't contain that exact token or contains a path
variant; update the logic around the linkflags/list mutation (the variables
linkflags, linker_script and the env.Replace(LINKFLAGS=linkflags) call) to:
ensure linkflags is a list, search for an element that endswith or contains
"memory.ld" (e.g., any(f for f in linkflags if "memory.ld" in f) ), and if found
insert linker_script after that element; if not found, append the linker_script
instead; finally call env.Replace(LINKFLAGS=linkflags).

In `@pio-scripts/load_usermods.py`:
- Around line 77-78: The current code truncates filenames at the first dot by
doing name.split('.')[0], which breaks names like "my.mod" and prevents
is_wled_module() from matching; change the return to preserve the full base name
without the final extension (e.g. use Path(name).stem or name.rsplit('.', 1)[0])
and keep the .strip() and None fallback, updating the expression that
assigns/returns name accordingly.

In `@platformio_override.sample.ini`:
- Line 554: The sample config currently references the external usermod repo
using a moving branch ref
"https://github.com/wled/wled-usermod-example.git#main"; change that to a fixed
tag or commit hash (for example replace "#main" with "#v1.0.0" or
"#<commit-hash>") so the external usermod URL is pinned and sample builds are
reproducible; update the URL string accordingly wherever
"https://github.com/wled/wled-usermod-example.git#main" appears.

In `@wled00/dynarray.h`:
- Line 17: DYNARRAY_MEMBER currently emits namespace-scope symbols; make them
internal by adding static to the declaration so symbols get internal linkage and
match DECLARE_DYNARRAY's static array markers—update the DYNARRAY_MEMBER macro
(the macro name DYNARRAY_MEMBER and its expansion that defines member_name in
the DYNARRAY_SECTION) to prepend static to the generated symbol declaration
while keeping the section attribute and used attribute unchanged.

In `@wled00/um_manager.cpp`:
- Around line 51-54: The serial debug block in readFromConfig() unconditionally
dereferences *x and always logs, which can access invalid memory when the
dynarray is empty and creates noisy serial output; remove the unconditional
dereference and noisy prints by guarding access with the usermods count (use
getCount() or check DYNARRAY_BEGIN(usermods) != DYNARRAY_END(usermods)) before
casting/reading *x, and wrap the remaining debug Serial.printf_P calls behind a
debug/build flag (or only run them when count>0) so they don't run in normal
builds; adjust references to DYNARRAY_BEGIN(usermods), usermods, x and
readFromConfig() accordingly.

---

Nitpick comments:
In `@wled00/um_manager.cpp`:
- Around line 7-13: The comment above DECLARE_DYNARRAY(Usermod*, usermods) is
outdated about using the '.dtors' segment; update it to describe current
dynarray placement by mentioning that non-ESP8266 targets use the '.dynarray'
section (while ESP8266 may still rely on '.dtors'), explain zero-length arrays
get addresses but consume no flash and the numeric suffix ensures ordering via
the linker, and keep reference to DECLARE_DYNARRAY and the usermods symbol so
future readers know this macro now maps to the '.dynarray' mechanism on modern
targets.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 37b8449e-f0fe-46d1-a5be-59981f5b4465

📥 Commits

Reviewing files that changed from the base of the PR and between b07cd45 and 210b4d8.

📒 Files selected for processing (9)
  • pio-scripts/dynarray.py
  • pio-scripts/load_usermods.py
  • pio-scripts/validate_modules.py
  • platformio.ini
  • platformio_override.sample.ini
  • tools/dynarray_espressif32.ld
  • wled00/dynarray.h
  • wled00/fcn_declare.h
  • wled00/um_manager.cpp

@netmindz
Copy link
Member

netmindz commented Mar 5, 2026

Using a consistent field for usermods will be a big improvement.

Have you tested against the V5 branch?

@willmmiles
Copy link
Member Author

Using a consistent field for usermods will be a big improvement.

Have you tested against the V5 branch?

I just tested with my own v5 WIP (it's IMO quite a bit cleaner, but shares no history, so not really a good candidate to merge .. I'll still post it up shortly though). The custom_usermods improvements work in that they build and run, but the mapfile validation checks are incompatible with -flto (it alters the information about what variable came from which file), so that'll still need to be fixed to use the Tasmota platform. I'll see what I can do.

@netmindz
Copy link
Member

netmindz commented Mar 7, 2026

I'll give this a test when I get a moment

When LTO is enabled, the map file no longer provides a positive
indication of whether a given object file has contributed to the final
binary.  Instead use nm to parse the debug data in the .elf file.

Co-Authored-By: Claude <noreply@anthropic.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pio-scripts/load_usermods.py`:
- Around line 70-80: _predict_dep_name() currently only extracts a basename for
GitHub/GitLab/Bitbucket hosts and returns None for other URL hosts, causing
entries like symlink://, file:// and self-hosted URLs to be skipped; change the
logic so that after parsing the URL (urlparse(entry)) you always derive a
candidate name from the path (e.g., use
Path(parsed.path.rstrip('/')).name.strip()) when parsed.netloc is not one of
('github.com','gitlab.com','bitbucket.com') and still strip a trailing ".git" if
present; ensure the function returns that basename (not None) so entries are
added to _custom_usermod_names and is_wled_module() will treat them as WLED
modules.

In `@pio-scripts/validate_modules.py`:
- Around line 43-50: The call to subprocess.run for nm (using nm_path) doesn't
check result.returncode, so when nm exits non‑zero the code still consumes
result.stdout; update the logic after subprocess.run to check result.returncode
and treat any non-zero as a failure: log a warning including the exit code (and
stderr if available) and return the conservative set ({Path(b.build_dir).name
for b in module_lib_builders}) instead of proceeding to parse nm_output; keep
the existing except block for timeouts/OS errors and reference nm_path,
result.returncode, result.stderr, nm_output, and module_lib_builders when
implementing the check.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 6f207ee6-63dc-4eca-a407-6c768da58a3b

📥 Commits

Reviewing files that changed from the base of the PR and between 210b4d8 and 7f44396.

📒 Files selected for processing (3)
  • pio-scripts/load_usermods.py
  • pio-scripts/validate_modules.py
  • wled00/um_manager.cpp

Comment on lines +70 to +80
if _URL_SCHEME_RE.match(entry):
parsed = urlparse(entry)
if parsed.netloc in ('github.com', 'gitlab.com', 'bitbucket.com'):
parts = [p for p in parsed.path.split('/') if p]
if len(parts) >= 2:
name = parts[1]
else:
name = Path(parsed.path.rstrip('/')).name.strip()
if name.endswith('.git'):
name = name[:-4]
return name or None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

fd load_usermods.py -x wc -l {}

Repository: wled/WLED

Length of output: 87


🏁 Script executed:

cat -n pio-scripts/load_usermods.py | head -150

Repository: wled/WLED

Length of output: 6777


🏁 Script executed:

cat -n pio-scripts/load_usermods.py | sed -n '60,130p'

Repository: wled/WLED

Length of output: 3194


🏁 Script executed:

python3 << 'EOF'
from urllib.parse import urlparse
from pathlib import Path

test_urls = [
    "symlink:///path/to/usermod_name",
    "file:///var/lib/repos/my_usermod",
    "https://git.example.com/user/repo.git",
    "git://self-hosted.org/usermods/cool-mod",
    "https://github.com/user/my-usermod.git",
]

for url in test_urls:
    parsed = urlparse(url)
    # Current logic (lines 70-80) - only github/gitlab/bitbucket
    if parsed.netloc in ('github.com', 'gitlab.com', 'bitbucket.com'):
        parts = [p for p in parsed.path.split('/') if p]
        if len(parts) >= 2:
            current_name = parts[1]
        else:
            current_name = Path(parsed.path.rstrip('/')).name.strip()
    else:
        current_name = None
    
    # Proposed logic (all schemes)
    name = Path(parsed.path.rstrip('/')).name.strip()
    if name.endswith('.git'):
        name = name[:-4]
    proposed_name = name or None
    
    print(f"URL: {url}")
    print(f"  Current: {current_name}")
    print(f"  Proposed: {proposed_name}")
    print()
EOF

Repository: wled/WLED

Length of output: 463


🏁 Script executed:

cat -n pio-scripts/load_usermods.py | sed -n '140,210p'

Repository: wled/WLED

Length of output: 3592


🏁 Script executed:

# Check if there's any validation or fallback that might handle unpredicted names
rg "library\.json" pio-scripts/ -A 3 -B 3

Repository: wled/WLED

Length of output: 2087


🏁 Script executed:

# Check validate_modules.py to see how it uses WLED_MODULES
cat -n pio-scripts/validate_modules.py | head -60

Repository: wled/WLED

Length of output: 3076


🏁 Script executed:

# Let's verify the proposed fix more carefully - ensure it handles all current code paths
cat -n pio-scripts/load_usermods.py | sed -n '70,93p'

Repository: wled/WLED

Length of output: 1197


🏁 Script executed:

# Check if there are any tests or examples of custom_usermods usage
fd -t f "platformio.ini" -exec grep -l "custom_usermods" {} \;

Repository: wled/WLED

Length of output: 303


🏁 Script executed:

fd platformio.ini -type f

Repository: wled/WLED

Length of output: 223


🏁 Script executed:

# Check for any issues or PRs related to custom_usermods and name prediction
git log --oneline --all -- "pio-scripts/load_usermods.py" | head -20

Repository: wled/WLED

Length of output: 940


🏁 Script executed:

# Let's verify the fix handles edge cases correctly
python3 << 'EOF'
from urllib.parse import urlparse
from pathlib import Path

test_cases = [
    "symlink:///path/to/usermod_name",
    "symlink:///path/to/usermod_name/",
    "file:///var/lib/repos/my_usermod",
    "file:///var/lib/repos/my_usermod.git",
    "https://git.example.com/user/repo.git",
    "git://self-hosted.org/usermods/cool-mod",
    "https://github.com/user/my-usermod.git",
    "symlink:///",  # edge: empty path
    "file://",      # edge: no path
]

print("Testing proposed fix logic:")
for url in test_cases:
    parsed = urlparse(url)
    name = Path(parsed.path.rstrip('/')).name.strip()
    if name.endswith('.git'):
        name = name[:-4]
    result = name or None
    print(f"{url:50} → {result}")
EOF

Repository: wled/WLED

Length of output: 640


🏁 Script executed:

# Let's check how PlatformIO resolves library names from deps to confirm behavior
rg "dep\.name" pio-scripts/load_usermods.py -B 2 -A 2

Repository: wled/WLED

Length of output: 195


🏁 Script executed:

# Check if there's any fallback mechanism when a library can't be recognized by name
rg "library\.json.*name" pio-scripts/ -A 2 -B 2

Repository: wled/WLED

Length of output: 483


🏁 Script executed:

# Verify the suggested fix doesn't have any syntax issues by testing it more thoroughly
python3 << 'EOF'
from urllib.parse import urlparse
from pathlib import Path

# Current code (lines 70-80)
def current_logic(entry):
    parsed = urlparse(entry)
    if parsed.netloc in ('github.com', 'gitlab.com', 'bitbucket.com'):
        parts = [p for p in parsed.path.split('/') if p]
        if len(parts) >= 2:
            name = parts[1]
        else:
            name = Path(parsed.path.rstrip('/')).name.strip()
        if name.endswith('.git'):
            name = name[:-4]
        return name or None
    return None  # Falls through for other schemes

# Proposed fix (restructured)
def proposed_logic(entry):
    parsed = urlparse(entry)
    if parsed.netloc in ('github.com', 'gitlab.com', 'bitbucket.com'):
        parts = [p for p in parsed.path.split('/') if p]
        if len(parts) >= 2:
            name = parts[1]
        else:
            name = Path(parsed.path.rstrip('/')).name.strip()
    else:
        name = Path(parsed.path.rstrip('/')).name.strip()
    if name.endswith('.git'):
        name = name[:-4]
    return name or None

test_cases = [
    "https://github.com/user/my-usermod.git",  # Should work in both
    "symlink:///path/to/usermod",              # Should fail in current, work in proposed
    "file:///path/my_usermod.git",             # Should fail in current, work in proposed
]

for url in test_cases:
    curr = current_logic(url)
    prop = proposed_logic(url)
    status = "✓" if curr == prop or (curr is None and prop is not None) else "✗"
    print(f"{status} {url:50} current={curr:15} proposed={prop}")
EOF

Repository: wled/WLED

Length of output: 307


🏁 Script executed:

# Fix the test to handle None values
python3 << 'EOF'
from urllib.parse import urlparse
from pathlib import Path

# Current code (lines 70-80)
def current_logic(entry):
    parsed = urlparse(entry)
    if parsed.netloc in ('github.com', 'gitlab.com', 'bitbucket.com'):
        parts = [p for p in parsed.path.split('/') if p]
        if len(parts) >= 2:
            name = parts[1]
        else:
            name = Path(parsed.path.rstrip('/')).name.strip()
        if name.endswith('.git'):
            name = name[:-4]
        return name or None
    return None  # Falls through for other schemes

# Proposed fix (restructured)
def proposed_logic(entry):
    parsed = urlparse(entry)
    if parsed.netloc in ('github.com', 'gitlab.com', 'bitbucket.com'):
        parts = [p for p in parsed.path.split('/') if p]
        if len(parts) >= 2:
            name = parts[1]
        else:
            name = Path(parsed.path.rstrip('/')).name.strip()
    else:
        name = Path(parsed.path.rstrip('/')).name.strip()
    if name.endswith('.git'):
        name = name[:-4]
    return name or None

test_cases = [
    "https://github.com/user/my-usermod.git",
    "symlink:///path/to/usermod",
    "file:///path/my_usermod.git",
]

print("Comparison of current vs proposed logic:")
for url in test_cases:
    curr = current_logic(url)
    prop = proposed_logic(url)
    improved = "✓" if curr != prop else "="
    print(f"{improved} {url:50}")
    print(f"   current:  {str(curr):20} proposed: {str(prop)}")
EOF

Repository: wled/WLED

Length of output: 414


🏁 Script executed:

# Now verify the impact: does is_wled_module rely on this name matching?
# Check how PlatformIO assigns names to library dependencies
rg "dep\.name" pio-scripts/ -B 3 -A 3

Repository: wled/WLED

Length of output: 492


Generic URL dependencies bypass WLED-module classification.

When _URL_SCHEME_RE matches a URL with a hostname outside GitHub/GitLab/Bitbucket, _predict_dep_name() returns None, so the entry never enters _custom_usermod_names. This causes is_wled_module() to fail for symlink://, file://, and self-hosted URLs, even though they're passed to PlatformIO. Such usermods then miss include-path setup, the -g flag, and libArchive=false validation.

Extract basename for any URL scheme to ensure these external usermods receive the same WLED-specific treatment:

Suggested fix
   if _URL_SCHEME_RE.match(entry):
     parsed = urlparse(entry)
     if parsed.netloc in ('github.com', 'gitlab.com', 'bitbucket.com'):
       parts = [p for p in parsed.path.split('/') if p]
       if len(parts) >= 2:
         name = parts[1]
       else:
         name = Path(parsed.path.rstrip('/')).name.strip()
-      if name.endswith('.git'):
-        name = name[:-4]
-      return name or None
+    else:
+      name = Path(parsed.path.rstrip('/')).name.strip()
+    if name.endswith('.git'):
+      name = name[:-4]
+    return name or None

Also applies to: 96-102, 114-125

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pio-scripts/load_usermods.py` around lines 70 - 80, _predict_dep_name()
currently only extracts a basename for GitHub/GitLab/Bitbucket hosts and returns
None for other URL hosts, causing entries like symlink://, file:// and
self-hosted URLs to be skipped; change the logic so that after parsing the URL
(urlparse(entry)) you always derive a candidate name from the path (e.g., use
Path(parsed.path.rstrip('/')).name.strip()) when parsed.netloc is not one of
('github.com','gitlab.com','bitbucket.com') and still strip a trailing ".git" if
present; ensure the function returns that basename (not None) so entries are
added to _custom_usermod_names and is_wled_module() will treat them as WLED
modules.

Comment on lines +43 to +50
result = subprocess.run(
[nm_path, "--defined-only", "-l", str(elf_path)],
capture_output=True, text=True, errors="ignore", timeout=120,
)
nm_output = result.stdout
except (subprocess.TimeoutExpired, FileNotFoundError, OSError) as e:
secho(f"WARNING: nm failed ({e}); skipping per-module validation", fg="yellow", err=True)
return {Path(b.build_dir).name for b in module_lib_builders} # conservative pass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -name "validate_modules.py" -type f

Repository: wled/WLED

Length of output: 86


🏁 Script executed:

cat -n pio-scripts/validate_modules.py | head -100

Repository: wled/WLED

Length of output: 5057


🏁 Script executed:

# Look for the check_elf_modules function
rg -n "def check_elf_modules" pio-scripts/validate_modules.py -A 50

Repository: wled/WLED

Length of output: 2785


🏁 Script executed:

# Verify subprocess.run() behavior by checking Python documentation/behavior
python3 << 'EOF'
import subprocess
import sys

# Check: does subprocess.run() raise on non-zero exit by default?
print("subprocess.run() behavior with check=False (default):")
print("- Does NOT raise exception on non-zero exit code")
print("- Returns CompletedProcess object with returncode attribute")
print("- stdout/stderr available regardless of exit code")
print()

# Show what happens with an nm-like scenario
result = subprocess.run(['sh', '-c', 'exit 5'], capture_output=True, text=True)
print(f"Example: exit code 5 with check=False")
print(f"  returncode: {result.returncode}")
print(f"  exception raised: No")
print(f"  stdout available: {repr(result.stdout)}")
EOF

Repository: wled/WLED

Length of output: 366


🏁 Script executed:

# Check if there are other subprocess calls in the file for pattern comparison
rg -n "subprocess\." pio-scripts/validate_modules.py

Repository: wled/WLED

Length of output: 164


Check nm exit code before consuming output.

subprocess.run() without check=True doesn't raise on non-zero exit—it returns CompletedProcess with the exit code in returncode. If nm fails but doesn't crash, the code consumes potentially empty or invalid stdout and proceeds to process it, leading to an empty confirmed set rather than the conservative pass. This creates asymmetric error handling: spawn/timeout failures return all modules, but tool execution failures return no modules.

Suggested fix
         result = subprocess.run(
             [nm_path, "--defined-only", "-l", str(elf_path)],
             capture_output=True, text=True, errors="ignore", timeout=120,
         )
+        if result.returncode != 0:
+            secho(
+                f"WARNING: nm failed with exit code {result.returncode}; skipping per-module validation",
+                fg="yellow",
+                err=True,
+            )
+            return {Path(b.build_dir).name for b in module_lib_builders}
         nm_output = result.stdout
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
result = subprocess.run(
[nm_path, "--defined-only", "-l", str(elf_path)],
capture_output=True, text=True, errors="ignore", timeout=120,
)
nm_output = result.stdout
except (subprocess.TimeoutExpired, FileNotFoundError, OSError) as e:
secho(f"WARNING: nm failed ({e}); skipping per-module validation", fg="yellow", err=True)
return {Path(b.build_dir).name for b in module_lib_builders} # conservative pass
result = subprocess.run(
[nm_path, "--defined-only", "-l", str(elf_path)],
capture_output=True, text=True, errors="ignore", timeout=120,
)
if result.returncode != 0:
secho(
f"WARNING: nm failed with exit code {result.returncode}; skipping per-module validation",
fg="yellow",
err=True,
)
return {Path(b.build_dir).name for b in module_lib_builders}
nm_output = result.stdout
except (subprocess.TimeoutExpired, FileNotFoundError, OSError) as e:
secho(f"WARNING: nm failed ({e}); skipping per-module validation", fg="yellow", err=True)
return {Path(b.build_dir).name for b in module_lib_builders} # conservative pass
🧰 Tools
🪛 Ruff (0.15.4)

[error] 43-43: subprocess call: check for execution of untrusted input

(S603)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pio-scripts/validate_modules.py` around lines 43 - 50, The call to
subprocess.run for nm (using nm_path) doesn't check result.returncode, so when
nm exits non‑zero the code still consumes result.stdout; update the logic after
subprocess.run to check result.returncode and treat any non-zero as a failure:
log a warning including the exit code (and stderr if available) and return the
conservative set ({Path(b.build_dir).name for b in module_lib_builders}) instead
of proceeding to parse nm_output; keep the existing except block for timeouts/OS
errors and reference nm_path, result.returncode, result.stderr, nm_output, and
module_lib_builders when implementing the check.

@netmindz netmindz merged commit 5beafe0 into wled:main Mar 8, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants