introduce prodtest error codes#6829
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduces a comprehensive numeric error-code system for prodtest commands: adds Sequence Diagram(s)sequenceDiagram
participant User as User/Test
participant CLI as rtl/cli
participant Cmd as prodtest command handler
participant Codes as prodtest_error_codes.h
User->>CLI: execute prodtest command
CLI->>Cmd: invoke handler
Cmd->>Codes: include & reference PRODTEST_ERR_* constant
Cmd->>CLI: cli_error(code: int, message)
CLI->>User: return ERROR <numeric_code> "<message>"
sequenceDiagram
participant Dev as Developer
participant Make as Makefile
participant Tool as core/tools/prodtest_error_codes.py
participant Header as prodtest_error_codes.h
participant CliH as rtl/inc/rtl/cli.h
participant Ver as version.h
participant JSON as error_codes.json
Dev->>Make: make gen (or gen_check)
Make->>Tool: python3 prodtest_error_codes.py
Tool->>Header: parse PRODTEST_ERR_* enum by module block
Tool->>CliH: parse CLI_ERROR_* framework code constants
Tool->>Ver: extract VERSION_MAJOR/MINOR/PATCH/BUILD
Tool->>JSON: generate or verify catalog structure
alt --check mode
Tool->>JSON: compare generated vs stored
Tool->>Make: exit non-zero if mismatch
else default mode
Tool->>JSON: write generated catalog
Tool->>Make: exit success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
| model | device_test | click_test | persistence_test |
|---|---|---|---|
| T2T1 |  test(all)  main(all)  |
 test(all)  main(all)  |
 test(all)  main(all)  |
| T3B1 |  test(all)  main(all)  |
 test(all)  main(all)  |
 test(all)  main(all)  |
| T3T1 |  test(all)  main(all)  |
 test(all)  main(all)  |
 test(all)  main(all)  |
| T3W1 |  test(all)  main(all)  |
 test(all)  main(all)  |
 test(all)  main(all)  |
Latest CI run: 27602807999
TychoVrahe
force-pushed
the
tychovrahe/prodtest/error_codes
branch
2 times, most recently
from
a98e657 to
5217521
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
core/embed/projects/prodtest/cmd/prodtest_ble.c (1)
206-221:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
returnaftercli_errorcauses double-response on failure paths.
prodtest_ble_erase_bonds_cmdhas twocli_errorcalls without a followingreturn:
- Line 207–208 (
PRODTEST_ERR_BLE_STATE_UNKNOWN): execution falls through; ifpeer_count == 0the function then callscli_ok("No bonds to erase."), emitting both an error and a success response to the host.- Lines 216–217 (
PRODTEST_ERR_BLE_BONDS_ERASE):cli_traceandcli_okare unconditionally reached after the error, so erase failures always also emit a success response.Every other
cli_errorsite in this file is followed byreturn. These two are the only exceptions and appear to be accidental omissions.🐛 Proposed fix
if (!state.state_known) { cli_error(cli, PRODTEST_ERR_BLE_STATE_UNKNOWN, "BLE state unknown."); + return; } if (state.peer_count == 0) { cli_ok(cli, "No bonds to erase."); return; } if (!prodtest_ble_erase_bonds(cli)) { cli_error(cli, PRODTEST_ERR_BLE_BONDS_ERASE, "Could not erase bonds."); + return; } cli_trace(cli, "Erased %d bonds.", state.peer_count); cli_ok(cli, "");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/embed/projects/prodtest/cmd/prodtest_ble.c` around lines 206 - 221, In prodtest_ble_erase_bonds_cmd, the two cli_error calls for state.state_known and prodtest_ble_erase_bonds failures must be followed by immediate returns to avoid emitting subsequent cli_ok/cli_trace responses; update the branches that call cli_error(PRODTEST_ERR_BLE_STATE_UNKNOWN, ...) and cli_error(PRODTEST_ERR_BLE_BONDS_ERASE, ...) to return after logging (same pattern used by other cli_error sites), ensuring state.state_known, state.peer_count, and prodtest_ble_erase_bonds are left unchanged otherwise.core/embed/rtl/cli.c (1)
161-191:⚠️ Potential issue | 🔴 Critical
CLI_ERRORis still referenced insyslog.cafter being removed by this PR.The PR removes the generic
CLI_ERRORmacro, andcore/embed/sys/dbg/syslog.c:289still calls:cli_error(cli, CLI_ERROR, "Failed to set log filter.");
CLI_ERRORis not defined incli.h(onlyCLI_ERROR_INVALID_CMDthroughCLI_ERROR_INVALID_CRCremain defined). This undefined identifier will cause a compilation failure.Replace
CLI_ERRORwithCLI_ERROR_FATALor another appropriate code fromcli.h:Proposed fix
if (!syslog_set_filter(filter, filter_len)) { - cli_error(cli, CLI_ERROR, "Failed to set log filter."); + cli_error(cli, CLI_ERROR_FATAL, "Failed to set log filter."); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/embed/rtl/cli.c` around lines 161 - 191, The call to cli_error(...) uses the removed identifier CLI_ERROR causing a compile error; update the invocation of cli_error (the call site that currently does cli_error(cli, CLI_ERROR, "Failed to set log filter.");) to use a valid code from cli.h such as CLI_ERROR_FATAL (or another appropriate CLI_ERROR_* constant), i.e. replace CLI_ERROR with CLI_ERROR_FATAL and keep the rest of the call unchanged so the signature cli_error(cli, int, const char*, ...) remains correct.core/embed/projects/prodtest/cmd/prodtest_tropic.c (1)
856-866:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
PRODTEST_ERR_TROPIC_PAIR_CERT_SIGNis semantically wrong for a key-invalidation failureThe operation at this site is
lt_pairing_key_invalidate()(invalidating the factory pairing key slot), but the assigned error code namePRODTEST_ERR_TROPIC_PAIR_CERT_SIGNdescribes certificate signing. Any host-side tooling or log triage that maps error code → description will receive a misleading signal.Since the enum is introduced fresh in this PR, the correct fix is to rename the enum member to reflect the actual operation (e.g.,
PRODTEST_ERR_TROPIC_PAIR_KEY_INVALIDATE) and update this call site. Per the PR rules, once the code ships, the integer value must be retired and a new code appended.🐛 Proposed fix (rename in `prodtest_error_codes.h` + update call site)
In
prodtest_error_codes.h, rename the enum member:- PRODTEST_ERR_TROPIC_PAIR_CERT_SIGN = <value>, + PRODTEST_ERR_TROPIC_PAIR_KEY_INVALIDATE = <value>,In
prodtest_tropic.c, line 861:- cli_error(cli, PRODTEST_ERR_TROPIC_PAIR_CERT_SIGN, + cli_error(cli, PRODTEST_ERR_TROPIC_PAIR_KEY_INVALIDATE, "`lt_pairing_key_invalidate()` failed for factory pairing key " "with error '%s'", lt_ret_verbose(ret));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/embed/projects/prodtest/cmd/prodtest_tropic.c` around lines 856 - 866, The error enum used for a failed key-invalidation is misnamed: replace the enum member PRODTEST_ERR_TROPIC_PAIR_CERT_SIGN with a new, semantically correct member (e.g., PRODTEST_ERR_TROPIC_PAIR_KEY_INVALIDATE) in the prodtest_error_codes.h enum (do not reuse the old integer value; append a new value per PR rules), then update the call site in prodtest_tropic.c that calls lt_pairing_key_invalidate(tropic_handle, TROPIC_FACTORY_PAIRING_KEY_SLOT) to pass the new PRODTEST_ERR_TROPIC_PAIR_KEY_INVALIDATE constant to cli_error instead of PRODTEST_ERR_TROPIC_PAIR_CERT_SIGN.core/embed/projects/prodtest/cmd/common.c (1)
500-519:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
return falseafter SN-retrieval errors allows cert-chain validation to pass with an empty device serial number.Both error paths that call
cli_error(lines 502–503 and 511–513) fall through to thememcmpcheck. When bothunit_properties_get_snandget_name_attributefail,device_sn_sizeandsubject_sn_sizeare both 0, sosubject_sn_size != device_sn_sizeis false andmemcmp(…, 0)returns 0 — the comparison passes silently andcheck_cert_chainreturnstrue. In production this is a security concern because a device whose serial number cannot be read will incorrectly validate its certificate chain.This is pre-existing but is made visible by the PR touching these exact
cli_errorcall sites.🛡️ Proposed fix
if (!unit_properties_get_sn(device_sn, sizeof(device_sn), &device_sn_size) || device_sn_size == 0) { cli_error(cli, PRODTEST_ERR_COMMON_CERT_CHAIN_DEVICE_SN, "check_device_cert_chain, device_sn not set."); + return false; } ... if (!get_name_attribute(&subject, OID_SERIAL_NUMBER, sizeof(OID_SERIAL_NUMBER), &subject_sn, &subject_sn_size)) { cli_error(cli, PRODTEST_ERR_COMMON_CERT_CHAIN_SERIAL_NUM, "check_device_cert_chain, serialNumber not set."); + return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/embed/projects/prodtest/cmd/common.c` around lines 500 - 519, The serial-number retrieval failure paths in check_device_cert_chain currently call cli_error but fall through, allowing both device_sn_size and subject_sn_size to be 0 and the memcmp to erroneously succeed; after the unit_properties_get_sn failure (the cli_error that logs PRODTEST_ERR_COMMON_CERT_CHAIN_DEVICE_SN) and after the get_name_attribute failure (the cli_error that logs PRODTEST_ERR_COMMON_CERT_CHAIN_SERIAL_NUM), immediately return false to abort validation — i.e., add a return false; after each cli_error so check_device_cert_chain exits on SN retrieval errors.
🧹 Nitpick comments (2)
core/tools/prodtest_error_codes.py (2)
30-32: 💤 Low valueRuff RUF001: ambiguous EN DASH
–in_MODULE_REregex character class.
[–-]is correct (the trailing-before]is treated as a literal hyphen), but the character class looks like an invalid range to both readers and the linter. An explicit escape clarifies intent:♻️ Suggested clarification
-_MODULE_RE = re.compile(r"//\s*===\s*(.+?)\s*\((\d+)\s*[–-]\s*(\d+)\)\s*") +_MODULE_RE = re.compile(r"//\s*===\s*(.+?)\s*\((\d+)\s*[–\-]\s*(\d+)\)\s*")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/tools/prodtest_error_codes.py` around lines 30 - 32, The regex in _MODULE_RE uses the character class [–-] which the linter flags as an ambiguous range; update _MODULE_RE to explicitly match an en dash or a hyphen (e.g., replace the character class with an alternation like (?:–|-) or use an escaped form such as [\u2013\-]) so the intent is unambiguous to readers and linters while keeping the rest of the pattern the same.
46-76: 💤 Low valueSilent
"module": nullwhen aPRODTEST_ERR_*entry appears before any module comment.
current_moduleis initialised toNoneand is only set when a// === … (range) ===marker is encountered. If a matching enum line appears before the first marker (e.g., due to a malformed header), the resulting JSON entry will have"module": nullwith no warning or error.A guard would make this detectable early:
♻️ Suggested guard
m = _ENUM_RE.match(line) if m: + if current_module is None: + print( + f"warning: enum entry {m.group(1)} has no preceding module marker", + file=sys.stderr, + ) errors.append( { "code": int(m.group(2)), "name": m.group(1), "module": current_module, } )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/tools/prodtest_error_codes.py` around lines 46 - 76, In parse_prodtest_header, add a guard inside the block handling _ENUM_RE to detect when current_module is still None (i.e., an enum appears before any module marker) and fail fast: raise a clear ValueError (or RuntimeError) that includes the matched enum name/code (from m.group(1)/m.group(2)) and the offending line content so the malformed header is detectable, rather than appending an entry with "module": None; reference parse_prodtest_header, current_module, and _ENUM_RE when implementing this check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/embed/projects/prodtest/cmd/common.c`:
- Around line 586-588: In check_device_cert_chain, fix the cli_error call that
reports der_read_item 7 by removing the spurious "\"%d.\"\" argument so the
format string and arguments align: ensure cli_error is called with the format
"check_device_cert_chain, der_read_item 7, cert %d." and a single cert_count
argument (use the existing PRODTEST_ERR_COMMON_CERT_CHAIN_READ_SIG_ALG constant
and keep der_read_item reference unchanged); this removes the undefined behavior
caused by passing a const char* where an int was expected.
In `@core/embed/projects/prodtest/cmd/prodtest_power_manager.c`:
- Around line 125-126: In prodtest_pm_charge_disable, the log/error messages are
copy-pasted for the opposite action; update the cli_trace and cli_error calls
(references: prodtest_pm_charge_disable, pm_charging_disable, cli_trace,
cli_error) to say "Disabling battery charging" and "Failed to disable battery
charging" respectively so the messages match the actual pm_charging_disable call
and behavior.
In `@core/embed/projects/prodtest/cmd/prodtest_secure_channel.c`:
- Around line 60-73: The function prodtest_secure_channel_handshake_2 currently
falls through after error reporting; add immediate returns after the two
cli_error calls so execution stops there: after the cli_error that reports
PRODTEST_ERR_SECURE_CHANNEL_INPUT_LEN (when input_length !=
SECURE_CHANNEL_INPUT_SIZE) return from the function instead of continuing to
call secure_channel_handshake_2, and after the cli_error that reports
PRODTEST_ERR_SECURE_CHANNEL_HANDSHAKE_FINISH (when
secure_channel_handshake_2(input) fails) return instead of falling through to
cli_ok; this ensures no further processing or an extra cli_ok() is emitted for
the same command.
---
Outside diff comments:
In `@core/embed/projects/prodtest/cmd/common.c`:
- Around line 500-519: The serial-number retrieval failure paths in
check_device_cert_chain currently call cli_error but fall through, allowing both
device_sn_size and subject_sn_size to be 0 and the memcmp to erroneously
succeed; after the unit_properties_get_sn failure (the cli_error that logs
PRODTEST_ERR_COMMON_CERT_CHAIN_DEVICE_SN) and after the get_name_attribute
failure (the cli_error that logs PRODTEST_ERR_COMMON_CERT_CHAIN_SERIAL_NUM),
immediately return false to abort validation — i.e., add a return false; after
each cli_error so check_device_cert_chain exits on SN retrieval errors.
In `@core/embed/projects/prodtest/cmd/prodtest_ble.c`:
- Around line 206-221: In prodtest_ble_erase_bonds_cmd, the two cli_error calls
for state.state_known and prodtest_ble_erase_bonds failures must be followed by
immediate returns to avoid emitting subsequent cli_ok/cli_trace responses;
update the branches that call cli_error(PRODTEST_ERR_BLE_STATE_UNKNOWN, ...) and
cli_error(PRODTEST_ERR_BLE_BONDS_ERASE, ...) to return after logging (same
pattern used by other cli_error sites), ensuring state.state_known,
state.peer_count, and prodtest_ble_erase_bonds are left unchanged otherwise.
In `@core/embed/projects/prodtest/cmd/prodtest_tropic.c`:
- Around line 856-866: The error enum used for a failed key-invalidation is
misnamed: replace the enum member PRODTEST_ERR_TROPIC_PAIR_CERT_SIGN with a new,
semantically correct member (e.g., PRODTEST_ERR_TROPIC_PAIR_KEY_INVALIDATE) in
the prodtest_error_codes.h enum (do not reuse the old integer value; append a
new value per PR rules), then update the call site in prodtest_tropic.c that
calls lt_pairing_key_invalidate(tropic_handle, TROPIC_FACTORY_PAIRING_KEY_SLOT)
to pass the new PRODTEST_ERR_TROPIC_PAIR_KEY_INVALIDATE constant to cli_error
instead of PRODTEST_ERR_TROPIC_PAIR_CERT_SIGN.
In `@core/embed/rtl/cli.c`:
- Around line 161-191: The call to cli_error(...) uses the removed identifier
CLI_ERROR causing a compile error; update the invocation of cli_error (the call
site that currently does cli_error(cli, CLI_ERROR, "Failed to set log
filter.");) to use a valid code from cli.h such as CLI_ERROR_FATAL (or another
appropriate CLI_ERROR_* constant), i.e. replace CLI_ERROR with CLI_ERROR_FATAL
and keep the rest of the call unchanged so the signature cli_error(cli, int,
const char*, ...) remains correct.
---
Nitpick comments:
In `@core/tools/prodtest_error_codes.py`:
- Around line 30-32: The regex in _MODULE_RE uses the character class [–-] which
the linter flags as an ambiguous range; update _MODULE_RE to explicitly match an
en dash or a hyphen (e.g., replace the character class with an alternation like
(?:–|-) or use an escaped form such as [\u2013\-]) so the intent is unambiguous
to readers and linters while keeping the rest of the pattern the same.
- Around line 46-76: In parse_prodtest_header, add a guard inside the block
handling _ENUM_RE to detect when current_module is still None (i.e., an enum
appears before any module marker) and fail fast: raise a clear ValueError (or
RuntimeError) that includes the matched enum name/code (from
m.group(1)/m.group(2)) and the offending line content so the malformed header is
detectable, rather than appending an entry with "module": None; reference
parse_prodtest_header, current_module, and _ENUM_RE when implementing this
check.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a7a47f6-82e1-4bc9-a61a-62e0bf317a38
📒 Files selected for processing (32)
Makefilecore/embed/projects/prodtest/.changelog.d/6829.addedcore/embed/projects/prodtest/cmd/common.ccore/embed/projects/prodtest/cmd/common.hcore/embed/projects/prodtest/cmd/prodtest_backup_ram.ccore/embed/projects/prodtest/cmd/prodtest_ble.ccore/embed/projects/prodtest/cmd/prodtest_bootloader.ccore/embed/projects/prodtest/cmd/prodtest_button.ccore/embed/projects/prodtest/cmd/prodtest_haptic.ccore/embed/projects/prodtest/cmd/prodtest_manufacturing_lock.ccore/embed/projects/prodtest/cmd/prodtest_nfc.ccore/embed/projects/prodtest/cmd/prodtest_nrf.ccore/embed/projects/prodtest/cmd/prodtest_optiga.ccore/embed/projects/prodtest/cmd/prodtest_otp_batch.ccore/embed/projects/prodtest/cmd/prodtest_otp_variant.ccore/embed/projects/prodtest/cmd/prodtest_power_manager.ccore/embed/projects/prodtest/cmd/prodtest_prodtest.ccore/embed/projects/prodtest/cmd/prodtest_rtc.ccore/embed/projects/prodtest/cmd/prodtest_sdcard.ccore/embed/projects/prodtest/cmd/prodtest_secrets.ccore/embed/projects/prodtest/cmd/prodtest_secure_channel.ccore/embed/projects/prodtest/cmd/prodtest_tamper.ccore/embed/projects/prodtest/cmd/prodtest_telemetry.ccore/embed/projects/prodtest/cmd/prodtest_touch.ccore/embed/projects/prodtest/cmd/prodtest_tropic.ccore/embed/projects/prodtest/cmd/prodtest_unit_test.ccore/embed/projects/prodtest/cmd/prodtest_wpc.ccore/embed/projects/prodtest/error_codes.jsoncore/embed/projects/prodtest/prodtest_error_codes.hcore/embed/rtl/cli.ccore/embed/rtl/inc/rtl/cli.hcore/tools/prodtest_error_codes.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/embed/projects/prodtest/cmd/common.c (1)
499-519:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBoth SN-check error paths fall through without returning, allowing cert-chain validation to silently succeed when neither SN can be retrieved.
When
unit_properties_get_snfails (line 499) andget_name_attributefails (line 508), bothcli_errorpaths continue without areturn false. At that pointdevice_sn_size == 0,subject_sn == NULL,subject_sn_size == 0. The subsequent guard:if (subject_sn_size != device_sn_size || // 0 != 0 → false memcmp(subject_sn, device_sn, device_sn_size) // memcmp(NULL, buf, 0)…is never taken, so the SN comparison trivially passes despite both lookups having failed.
memcmpwith a NULL pointer and size 0 is also technically UB in C (§7.1.4 requires valid pointers regardless of n). The net result is that the cert-chain check proceeds as if SN validation succeeded on an unprovisioned or misconfigured device.Even if this is intentional for unprovisioned devices, the silent success is dangerous; the two error codes (
PRODTEST_ERR_COMMON_CERT_CHAIN_DEVICE_SN/_SERIAL_NUM) emit an error but do not abort — conflicting signal to callers. At minimum, each failure branch shouldreturn falseto be consistent with every other error path in this function.🐛 Proposed fix
if (!unit_properties_get_sn(device_sn, sizeof(device_sn), &device_sn_size) || device_sn_size == 0) { cli_error(cli, PRODTEST_ERR_COMMON_CERT_CHAIN_DEVICE_SN, "check_device_cert_chain, device_sn not set."); + return false; } const uint8_t* subject_sn = NULL; size_t subject_sn_size = 0; if (!get_name_attribute(&subject, OID_SERIAL_NUMBER, sizeof(OID_SERIAL_NUMBER), &subject_sn, &subject_sn_size)) { cli_error(cli, PRODTEST_ERR_COMMON_CERT_CHAIN_SERIAL_NUM, "check_device_cert_chain, serialNumber not set."); + return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/embed/projects/prodtest/cmd/common.c` around lines 499 - 519, The error branches after unit_properties_get_sn and get_name_attribute call cli_error but do not abort, allowing check_device_cert_chain to continue with device_sn_size == 0 and subject_sn == NULL; add an immediate return false after each cli_error that logs PRODTEST_ERR_COMMON_CERT_CHAIN_DEVICE_SN and PRODTEST_ERR_COMMON_CERT_CHAIN_SERIAL_NUM respectively so failures in unit_properties_get_sn or get_name_attribute stop processing (this prevents the subsequent memcmp(NULL, ..., 0) UB and fixes the silent-success cert-chain validation in check_device_cert_chain).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@core/embed/projects/prodtest/cmd/common.c`:
- Around line 499-519: The error branches after unit_properties_get_sn and
get_name_attribute call cli_error but do not abort, allowing
check_device_cert_chain to continue with device_sn_size == 0 and subject_sn ==
NULL; add an immediate return false after each cli_error that logs
PRODTEST_ERR_COMMON_CERT_CHAIN_DEVICE_SN and
PRODTEST_ERR_COMMON_CERT_CHAIN_SERIAL_NUM respectively so failures in
unit_properties_get_sn or get_name_attribute stop processing (this prevents the
subsequent memcmp(NULL, ..., 0) UB and fixes the silent-success cert-chain
validation in check_device_cert_chain).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11517aa8-5ecc-47d9-977e-8ecd19358650
📒 Files selected for processing (3)
core/embed/projects/prodtest/cmd/common.ccore/embed/projects/prodtest/cmd/prodtest_power_manager.ccore/embed/projects/prodtest/cmd/prodtest_secure_channel.c
🚧 Files skipped from review as they are similar to previous changes (2)
- core/embed/projects/prodtest/cmd/prodtest_secure_channel.c
- core/embed/projects/prodtest/cmd/prodtest_power_manager.c
TychoVrahe
force-pushed
the
tychovrahe/prodtest/error_codes
branch
from
e6b8491 to
05aab3f
Compare
cepetr
requested changes
Jun 3, 2026
github-project-automation
Bot
moved this from 🔎 Needs review
to 🏃♀️ In progress
in Firmware
cepetr
requested changes
Jun 3, 2026
Contributor
Author
|
On request from production guys, prodtest error codes for each group start from xx10, the 0-9 are reserved. |
kopecdav
requested changes
Jun 9, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/embed/projects/prodtest/README.md (1)
44-46: 💤 Low valueConsider adding language identifiers to fenced code blocks.
The static analysis tool flags missing language specifiers for these code blocks. Adding
```shellor```consoleidentifiers would improve markdown rendering and syntax highlighting.Also applies to: 639-644, 673-678
🤖 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 `@core/embed/projects/prodtest/README.md` around lines 44 - 46, The fenced code blocks in README.md that contain console output like ERROR 11 "Expecting x-coordinate." (and the other blocks at the ranges you noted) lack a language identifier; update each opening triple-backtick to include a language specifier such as ```shell or ```console so the snippets render and highlight correctly — locate the code blocks around the shown ERROR line and the other reported ranges (639-644, 673-678) and replace ``` with ```shell or ```console as appropriate.Source: Linters/SAST tools
🤖 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 `@core/embed/projects/prodtest/README.md`:
- Around line 44-46: The fenced code blocks in README.md that contain console
output like ERROR 11 "Expecting x-coordinate." (and the other blocks at the
ranges you noted) lack a language identifier; update each opening
triple-backtick to include a language specifier such as ```shell or ```console
so the snippets render and highlight correctly — locate the code blocks around
the shown ERROR line and the other reported ranges (639-644, 673-678) and
replace ``` with ```shell or ```console as appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23ee7c3d-44ec-48eb-ad05-e466ece9e1d4
📒 Files selected for processing (3)
core/embed/projects/prodtest/README.mdcore/embed/projects/prodtest/error_codes.jsoncore/embed/rtl/inc/rtl/cli.h
💤 Files with no reviewable changes (1)
- core/embed/projects/prodtest/error_codes.json
🚧 Files skipped from review as they are similar to previous changes (1)
- core/embed/rtl/inc/rtl/cli.h
kopecdav
approved these changes
Jun 15, 2026
TychoVrahe
force-pushed
the
tychovrahe/prodtest/error_codes
branch
from
243a538 to
b66535e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Makefile (1)
202-210: ⚡ Quick winDeclare new generation targets as phony.
Please add
.PHONYentries forprodtest_error_codes,prodtest_error_codes_check,gen, andgen_checkto prevent accidental file-name shadowing from skipping recipes.🤖 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 `@Makefile` around lines 202 - 210, Add a .PHONY declaration in the Makefile to mark the new generation targets as phony targets. Declare prodtest_error_codes, prodtest_error_codes_check, gen, and gen_check as phony targets by adding them to a .PHONY line (either a new line or appended to an existing .PHONY declaration if one exists in the file). This prevents Make from treating these as file targets and ensures the recipes always execute even if files with those names exist.Source: Linters/SAST tools
🤖 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 `@core/embed/projects/prodtest/README.md`:
- Line 578: In the error description text on line 578 of the README.md file, fix
the spelling error where "occured" is misspelled. Change it to the correct
spelling "occurred" (with double 'r') in the user-facing error message that
describes the I/O error codes 14011-14015.
In `@core/tools/prodtest_error_codes.py`:
- Around line 178-181: The current code initializes framework_codes as an empty
list and only populates it if the --cli-header file exists, which allows the
program to continue with degraded output when the required file is missing. Fix
this by adding explicit error handling that raises an exception or exits the
program when args.cli_header does not exist, rather than silently continuing
with an empty framework_codes list. Remove or modify the conditional check to
ensure the parse_cli_header function is always called and any missing file will
cause the program to fail fast.
- Around line 52-76: The parse_prodtest_header() function is missing validation
to enforce catalog invariants when parsing error codes. When appending entries
to the errors list in the section where it matches _ENUM_RE, add checks to
ensure: (1) the error code (obtained from m.group(2)) falls within the current
module's range_start and range_end, and (2) the error code has not already been
added to the errors list (no duplicates). If either check fails, raise an
appropriate exception with a descriptive message to prevent invalid catalogs
from being silently generated and committed.
---
Nitpick comments:
In `@Makefile`:
- Around line 202-210: Add a .PHONY declaration in the Makefile to mark the new
generation targets as phony targets. Declare prodtest_error_codes,
prodtest_error_codes_check, gen, and gen_check as phony targets by adding them
to a .PHONY line (either a new line or appended to an existing .PHONY
declaration if one exists in the file). This prevents Make from treating these
as file targets and ensures the recipes always execute even if files with those
names exist.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 053068be-6e62-4b87-a105-8fa1cf48ecbe
📒 Files selected for processing (38)
Makefilecore/SConscript.prodtestcore/SConscript.prodtest_emucore/embed/projects/prodtest/.changelog.d/6829.addedcore/embed/projects/prodtest/README.mdcore/embed/projects/prodtest/build.rscore/embed/projects/prodtest/cmd/common.ccore/embed/projects/prodtest/cmd/common.hcore/embed/projects/prodtest/cmd/prodtest_backup_ram.ccore/embed/projects/prodtest/cmd/prodtest_ble.ccore/embed/projects/prodtest/cmd/prodtest_bootloader.ccore/embed/projects/prodtest/cmd/prodtest_button.ccore/embed/projects/prodtest/cmd/prodtest_haptic.ccore/embed/projects/prodtest/cmd/prodtest_manufacturing_lock.ccore/embed/projects/prodtest/cmd/prodtest_nfc.ccore/embed/projects/prodtest/cmd/prodtest_nrf.ccore/embed/projects/prodtest/cmd/prodtest_optiga.ccore/embed/projects/prodtest/cmd/prodtest_otp_batch.ccore/embed/projects/prodtest/cmd/prodtest_otp_variant.ccore/embed/projects/prodtest/cmd/prodtest_power_manager.ccore/embed/projects/prodtest/cmd/prodtest_prodtest.ccore/embed/projects/prodtest/cmd/prodtest_rtc.ccore/embed/projects/prodtest/cmd/prodtest_sdcard.ccore/embed/projects/prodtest/cmd/prodtest_secrets.ccore/embed/projects/prodtest/cmd/prodtest_secure_channel.ccore/embed/projects/prodtest/cmd/prodtest_syslog.ccore/embed/projects/prodtest/cmd/prodtest_tamper.ccore/embed/projects/prodtest/cmd/prodtest_telemetry.ccore/embed/projects/prodtest/cmd/prodtest_touch.ccore/embed/projects/prodtest/cmd/prodtest_tropic.ccore/embed/projects/prodtest/cmd/prodtest_unit_test.ccore/embed/projects/prodtest/cmd/prodtest_wpc.ccore/embed/projects/prodtest/error_codes.jsoncore/embed/projects/prodtest/prodtest_error_codes.hcore/embed/rtl/cli.ccore/embed/rtl/inc/rtl/cli.hcore/embed/sys/dbg/syslog.ccore/tools/prodtest_error_codes.py
💤 Files with no reviewable changes (1)
- core/embed/sys/dbg/syslog.c
✅ Files skipped from review due to trivial changes (4)
- core/embed/projects/prodtest/cmd/common.h
- core/embed/projects/prodtest/cmd/prodtest_prodtest.c
- core/embed/projects/prodtest/.changelog.d/6829.added
- core/embed/projects/prodtest/error_codes.json
🚧 Files skipped from review as they are similar to previous changes (28)
- core/SConscript.prodtest
- core/embed/projects/prodtest/build.rs
- core/embed/projects/prodtest/cmd/prodtest_telemetry.c
- core/embed/projects/prodtest/cmd/prodtest_bootloader.c
- core/embed/rtl/cli.c
- core/SConscript.prodtest_emu
- core/embed/projects/prodtest/cmd/prodtest_button.c
- core/embed/projects/prodtest/cmd/prodtest_secure_channel.c
- core/embed/projects/prodtest/cmd/prodtest_tamper.c
- core/embed/projects/prodtest/cmd/prodtest_rtc.c
- core/embed/rtl/inc/rtl/cli.h
- core/embed/projects/prodtest/cmd/prodtest_wpc.c
- core/embed/projects/prodtest/cmd/prodtest_backup_ram.c
- core/embed/projects/prodtest/cmd/prodtest_otp_batch.c
- core/embed/projects/prodtest/cmd/prodtest_power_manager.c
- core/embed/projects/prodtest/cmd/prodtest_unit_test.c
- core/embed/projects/prodtest/cmd/prodtest_touch.c
- core/embed/projects/prodtest/prodtest_error_codes.h
- core/embed/projects/prodtest/cmd/prodtest_nfc.c
- core/embed/projects/prodtest/cmd/prodtest_nrf.c
- core/embed/projects/prodtest/cmd/prodtest_ble.c
- core/embed/projects/prodtest/cmd/prodtest_sdcard.c
- core/embed/projects/prodtest/cmd/prodtest_manufacturing_lock.c
- core/embed/projects/prodtest/cmd/common.c
- core/embed/projects/prodtest/cmd/prodtest_secrets.c
- core/embed/projects/prodtest/cmd/prodtest_otp_variant.c
- core/embed/projects/prodtest/cmd/prodtest_optiga.c
- core/embed/projects/prodtest/cmd/prodtest_tropic.c
Comment on lines
+52
to
+76
| for line in path.read_text(encoding="utf-8").splitlines(): | ||
| m = _MODULE_RE.search(line) | ||
| if m: | ||
| # Extract the module name, stripping any trailing note after " — " | ||
| raw_name = m.group(1).strip() | ||
| name = raw_name.split(" — ")[0].strip() | ||
| range_start = int(m.group(2)) | ||
| range_end = int(m.group(3)) | ||
| current_module = name | ||
| modules.append( | ||
| {"module": name, "range_start": range_start, "range_end": range_end} | ||
| ) | ||
| continue | ||
|
|
||
| m = _ENUM_RE.match(line) | ||
| if m: | ||
| errors.append( | ||
| { | ||
| "code": int(m.group(2)), | ||
| "name": m.group(1), | ||
| "module": current_module, | ||
| } | ||
| ) | ||
|
|
||
| return modules, errors |
There was a problem hiding this comment.
Enforce catalog invariants during generation (range + uniqueness).
parse_prodtest_header() currently accepts enum entries without checking duplicate numeric codes or whether each code belongs to the active module range. That allows invalid catalogs to be generated and committed silently.
Suggested fix
def parse_prodtest_header(path: Path) -> tuple[list[dict], list[dict]]:
@@
- current_module: str | None = None
+ current_module: str | None = None
+ current_range: tuple[int, int] | None = None
+ seen_codes: set[int] = set()
@@
if m:
@@
range_start = int(m.group(2))
range_end = int(m.group(3))
current_module = name
+ current_range = (range_start, range_end)
modules.append(
{"module": name, "range_start": range_start, "range_end": range_end}
)
continue
@@
if m:
+ code = int(m.group(2))
+ if current_module is None or current_range is None:
+ raise ValueError(f"{path}: enum {m.group(1)} declared outside a module block")
+ if code in seen_codes:
+ raise ValueError(f"{path}: duplicate code {code} ({m.group(1)})")
+ if not (current_range[0] <= code <= current_range[1]):
+ raise ValueError(
+ f"{path}: code {code} ({m.group(1)}) out of range for module {current_module} "
+ f"[{current_range[0]}-{current_range[1]}]"
+ )
+ seen_codes.add(code)
errors.append(
{
- "code": int(m.group(2)),
+ "code": code,
"name": m.group(1),
"module": current_module,
}
)🤖 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 `@core/tools/prodtest_error_codes.py` around lines 52 - 76, The
parse_prodtest_header() function is missing validation to enforce catalog
invariants when parsing error codes. When appending entries to the errors list
in the section where it matches _ENUM_RE, add checks to ensure: (1) the error
code (obtained from m.group(2)) falls within the current module's range_start
and range_end, and (2) the error code has not already been added to the errors
list (no duplicates). If either check fails, raise an appropriate exception with
a descriptive message to prevent invalid catalogs from being silently generated
and committed.
Comment on lines
+178
to
+181
| framework_codes: list[dict] = [] | ||
| if args.cli_header.exists(): | ||
| framework_codes = parse_cli_header(args.cli_header) | ||
|
|
There was a problem hiding this comment.
Fail fast when --cli-header is missing to avoid partial catalogs.
On Line 179, missing cli.h currently yields an empty framework_codes list and still succeeds. For this rollout, that silently degrades the output contract.
🤖 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 `@core/tools/prodtest_error_codes.py` around lines 178 - 181, The current code
initializes framework_codes as an empty list and only populates it if the
--cli-header file exists, which allows the program to continue with degraded
output when the required file is missing. Fix this by adding explicit error
handling that raises an exception or exits the program when args.cli_header does
not exist, rather than silently continuing with an empty framework_codes list.
Remove or modify the conditional check to ensure the parse_cli_header function
is always called and any missing file will cause the program to fail fast.
[no changelog]
…_disable [no changelog]
…andshake_2 [no changelog]
…ject [no changelog]
TychoVrahe
force-pushed
the
tychovrahe/prodtest/error_codes
branch
from
b66535e to
81a15a0
Compare
This was referenced Jun 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cli_error()call site gets a unique code that survives refactoring, command addition/removal, and message editsERROR error "message"->ERROR <int> "message"Changes to
cli.h/cli.ccli_error()signature:const char* code->int codeCLI_ERROR_INVALID_CMDetc.) converted from strings to integers 1?8CLI_ERROR "error"macro removedRules (encoded as comments in the header)
DEPRECATEDBreaking change
Host-side test scripts must be updated - the
ERRORline now carries an integer instead of a string.New file:
prodtest_error_codes.hCentral
prodtest_error_tenum with 326 unique codes across 24 module blocks of 1000: