Add defmt-avr checks - #94
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe PR expands AVR testing infrastructure to validate the new Changes
Sequence Diagram(s)sequenceDiagram
participant GHA as GitHub Actions
participant Build as Cargo Build
participant Size as AVR Size Analysis
participant Objdump as Objdump Symbol Extraction
participant Sim as Simulator
GHA->>Build: Build baseline (no defmt)
Build-->>Size: Generate ELF binary
GHA->>Build: Build with defmt feature
Build-->>Size: Generate ELF binary
Size->>Size: Compare program sizes<br/>(delta must = 0)
Size->>Objdump: Extract panic symbols<br/>(baseline)
Objdump-->>Objdump: Normalize symbol names<br/>(strip Rust hashes)
Objdump->>Objdump: Extract panic symbols<br/>(defmt variant)
Objdump->>Objdump: Compare symbol sets<br/>(fail if new symbols)
GHA->>Sim: Run smoke test<br/>(with defmt enabled)
Sim-->>Sim: Execute example,<br/>check completion marker
Sim->>GHA: Report results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 security issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- The new defmt smoke logic duplicates configuration data already present in CONFIGS (e.g., slice/stream tiny cases); consider reusing or deriving DEFMT_SMOKE_CONFIGS from CONFIGS to avoid drift between the two lists.
- run_panic_checker now hardcodes the asm_file path instead of using the value returned from _save_assembly_output, while _collect_objdump_output still saves a file; it might be clearer and less error-prone to have _collect_objdump_output return both the filtered output and the actual asm path, so all consumers use the same source of truth.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new defmt smoke logic duplicates configuration data already present in CONFIGS (e.g., slice/stream tiny cases); consider reusing or deriving DEFMT_SMOKE_CONFIGS from CONFIGS to avoid drift between the two lists.
- run_panic_checker now hardcodes the asm_file path instead of using the value returned from _save_assembly_output, while _collect_objdump_output still saves a file; it might be clearer and less error-prone to have _collect_objdump_output return both the filtered output and the actual asm path, so all consumers use the same source of truth.
## Individual Comments
### Comment 1
<location path="avr_demo/run_suite.py" line_range="169" />
<code_context>
subprocess.run(cmd, check=True, text=True, timeout=180)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 2
<location path="avr_demo/run_suite.py" line_range="220-225" />
<code_context>
result = subprocess.run(
cmd,
capture_output=True,
text=True,
timeout=180,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new defmt feature for AVR examples and adds a comprehensive smoke test suite to run_suite.py. The new suite automates binary size comparisons, panic symbol analysis, and simulator-based smoke tests to ensure the defmt feature doesn't introduce regressions. Feedback focuses on correcting the build output path for the dev profile and improving error handling to prevent script crashes when tools are missing or execution fails.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 678875b202
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
avr_demo/run_suite.py (3)
489-519: Recommended: dedupepanic_patternsbetween the two scanners.The list at lines 491–505 is a strict subset of the one in
_analyze_panic_patterns(lines 441–459), withunreachable_unchecked,panic!,unwrap\(\),expect\(intentionally dropped because they don't appear as linker-visible symbol names. Hoisting the shared core into a module-level constant (and extending it locally where needed) keeps the two scanners from drifting apart over time.♻️ Proposed structure
+# Linker-visible panic symbols common to both scanners. +_PANIC_SYMBOL_PATTERNS = ( + r'panic_fmt', + r'panic_const', + r'panic_nounwind', + r'panic_impl', + r'assert_failed', + r'unwrap_failed', + r'expect_failed', + r'slice_end_index_len_fail', + r'slice_start_index_len_fail', + r'slice_index_len_fail', + r'panic_for_nonpositive_argument', + r'panic_bounds_check', + r'core::panicking::', +) +# Source-form patterns only meaningful when scanning disassembly text. +_PANIC_SOURCE_PATTERNS = _PANIC_SYMBOL_PATTERNS + ( + r'unreachable_unchecked', + r'panic!', + r'unwrap\(\)', + r'expect\(', +)Then have
_analyze_panic_patternsuse_PANIC_SOURCE_PATTERNSand_collect_panic_symbolsuse_PANIC_SYMBOL_PATTERNS.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@avr_demo/run_suite.py` around lines 489 - 519, The two scanners duplicate panic pattern lists; extract the shared core into a module-level constant (e.g. _PANIC_CORE_PATTERNS) and replace the literal lists in _analyze_panic_patterns and _collect_panic_symbols with derived constants: _PANIC_SOURCE_PATTERNS = _PANIC_CORE_PATTERNS + [extra source-only patterns] and _PANIC_SYMBOL_PATTERNS = _PANIC_CORE_PATTERNS (or + any symbol-only patterns), update references in the functions (_analyze_panic_patterns, _collect_panic_symbols) to use these constants, and run tests to ensure behavior is unchanged.
260-263: Minor: prefer unpacking over list concatenation (RUF005).Aligns with the static-analysis hint and avoids constructing an intermediate list.
♻️ Proposed change
- defmt_feature_str = ",".join(base_features + ["defmt"]) + defmt_feature_str = ",".join([*base_features, "defmt"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@avr_demo/run_suite.py` around lines 260 - 263, The code builds defmt_feature_str by concatenating base_features + ["defmt"] which creates an intermediate list; instead, use sequence unpacking so join receives a tuple (e.g. join((*base_features, "defmt"))) to avoid the temporary list. Update the for-loop where label, example, base_features are iterated and replace the base_features + ["defmt"] expression used to create defmt_feature_str with an unpacked sequence containing base_features and "defmt".
152-171: Capture build output so failures are diagnosable.
subprocess.run(..., check=True, text=True, timeout=180)withoutcapture_output=Truemeans that when the build fails and theCalledProcessErroris caught at line 283,str(e)only shows the exit status — the actual cargo/rustc error is lost. Capturing and surfacing it (or echoing on failure) saves a lot of CI debugging time.♻️ Proposed change
- subprocess.run(cmd, check=True, text=True, timeout=180) + result = subprocess.run(cmd, text=True, capture_output=True, timeout=180) + if result.returncode != 0: + sys.stderr.write(result.stdout) + sys.stderr.write(result.stderr) + raise subprocess.CalledProcessError( + result.returncode, cmd, output=result.stdout, stderr=result.stderr + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@avr_demo/run_suite.py` around lines 152 - 171, The build invocation in _cargo_build_example currently calls subprocess.run(..., check=True, text=True, timeout=180) without capturing output, so failing builds lose cargo/rustc diagnostics; modify the subprocess.run call to include capture_output=True (or stdout=PIPE/stderr=PIPE) and keep text=True, and then when a CalledProcessError is caught by the caller, surface the captured output by including e.stdout and e.stderr (or printing/logging them) in the error message or log so CI failures include the full build output for diagnosis..github/workflows/avr_tests.yaml (1)
73-80: Heads-up: matrix size doubles to 32 combinations.The existing matrix (
example × int_type × profile × pico_size = 2×2×2×2 = 16) becomes 32 with the newdefmtdimension. CI minutes for this job will roughly double. If that becomes a problem, you could trim todefmt: [true]only for one representative(int_type, pico_size, profile)slice viainclude/exclude, since the orthogonal coverage of defmt vs everything else is rarely necessary for a smoke check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/avr_tests.yaml around lines 73 - 80, The workflow matrix added a new dimension defmt which doubles the total combinations from 16 to 32 and will double CI minutes; update the matrix declaration so defmt does not multiply across all combinations — either set defmt: [true] only, or keep defmt: [false, true] but add an include block that lists one representative combination (e.g., example: test_picojson, int_type: int32, profile: release, pico_size: pico-tiny, defmt: true) to run with defmt and avoid running defmt=true for every example/int_type/profile/pico_size tuple; modify the strategy.matrix keys (matrix, defmt, include/exclude) to implement this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/avr_tests.yaml:
- Around line 73-80: The workflow matrix added a new dimension defmt which
doubles the total combinations from 16 to 32 and will double CI minutes; update
the matrix declaration so defmt does not multiply across all combinations —
either set defmt: [true] only, or keep defmt: [false, true] but add an include
block that lists one representative combination (e.g., example: test_picojson,
int_type: int32, profile: release, pico_size: pico-tiny, defmt: true) to run
with defmt and avoid running defmt=true for every
example/int_type/profile/pico_size tuple; modify the strategy.matrix keys
(matrix, defmt, include/exclude) to implement this change.
In `@avr_demo/run_suite.py`:
- Around line 489-519: The two scanners duplicate panic pattern lists; extract
the shared core into a module-level constant (e.g. _PANIC_CORE_PATTERNS) and
replace the literal lists in _analyze_panic_patterns and _collect_panic_symbols
with derived constants: _PANIC_SOURCE_PATTERNS = _PANIC_CORE_PATTERNS + [extra
source-only patterns] and _PANIC_SYMBOL_PATTERNS = _PANIC_CORE_PATTERNS (or +
any symbol-only patterns), update references in the functions
(_analyze_panic_patterns, _collect_panic_symbols) to use these constants, and
run tests to ensure behavior is unchanged.
- Around line 260-263: The code builds defmt_feature_str by concatenating
base_features + ["defmt"] which creates an intermediate list; instead, use
sequence unpacking so join receives a tuple (e.g. join((*base_features,
"defmt"))) to avoid the temporary list. Update the for-loop where label,
example, base_features are iterated and replace the base_features + ["defmt"]
expression used to create defmt_feature_str with an unpacked sequence containing
base_features and "defmt".
- Around line 152-171: The build invocation in _cargo_build_example currently
calls subprocess.run(..., check=True, text=True, timeout=180) without capturing
output, so failing builds lose cargo/rustc diagnostics; modify the
subprocess.run call to include capture_output=True (or stdout=PIPE/stderr=PIPE)
and keep text=True, and then when a CalledProcessError is caught by the caller,
surface the captured output by including e.stdout and e.stderr (or
printing/logging them) in the error message or log so CI failures include the
full build output for diagnosis.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c6e1d16-a0c6-496b-bec3-a5a6d6347503
📒 Files selected for processing (3)
.github/workflows/avr_tests.yamlavr_demo/Cargo.tomlavr_demo/run_suite.py
1 participant
Smoketest defmt on AVR builds #37
Summary by Sourcery
Add AVR defmt smoke checks and integrate them into local tooling and CI to ensure enabling defmt does not regress binary size or panic behavior.
New Features:
Enhancements:
CI:
Summary by CodeRabbit
Release Notes
Tests
defmtfeature behavior across the buildChores