Skip to content

Conversation

@lihuanhuan
Copy link
Contributor

@lihuanhuan lihuanhuan commented Dec 10, 2025

Summary by CodeRabbit

  • Chores

    • Updated firmware build naming convention to include "btc" identifier in stable builds.
    • Enhanced signing workflow for intermediate boot updates in QA builds.
    • Adjusted QA firmware flag configuration.
  • Bug Fixes

    • Modified bootloader shutdown behavior in non-production builds.

✏️ Tip: You can customize this high-level summary in your review settings.

@revan-zhang
Copy link
Contributor

revan-zhang commented Dec 10, 2025

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@coderabbitai
Copy link

coderabbitai bot commented Dec 10, 2025

Walkthrough

The changes update firmware build naming conventions, adjust QA build configuration flags, and add conditional production-mode shutdown behavior for unknown bootloader scenarios. A new signing flow for intermediate boot updates is introduced in QA builds.

Changes

Cohort / File(s) Summary
CI/CD Workflow
.github/workflows/build-classic1s.yml
FIRMWARE_QA flag changed from 1 to 0 in QA build path. New conditional signing flow added: when building non-Bitcoin firmware in QA mode, executes SIGN_SCRIPT on legacy/intermediate_fw artifacts using QA keys and copies the signed binary; otherwise copies unsigned boot updates.
Firmware Build Naming
legacy/firmware/Makefile
NAME variable for non-Bitcoin builds updated from "...-Stable-$(BUILD_COMMIT)" to "...-Stable-btc-$(BUILD_COMMIT)", adding a "btc" label to stable build identifiers.
Bootloader Logic
legacy/firmware/bl_check.c
shutdown() call after unknown bootloader dialog now wrapped in #if PRODUCTION ... #endif`` guard, suppressing shutdown in non-production builds.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Workflow file: Verify the new signing conditional logic executes in the correct build path and that artifact copying works as intended
  • Build naming: Straightforward variable update; confirm no other build identifiers are affected

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the primary change: adding intermediate_fw bin files to classic1s build artifacts in the CI workflow.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 2aad007 and c410495.

📒 Files selected for processing (3)
  • .github/workflows/build-classic1s.yml (2 hunks)
  • legacy/firmware/Makefile (1 hunks)
  • legacy/firmware/bl_check.c (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
legacy/firmware/bl_check.c (1)
core/embed/trezorhal/common.c (1)
  • shutdown (42-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Style check
  • GitHub Check: Defs check
  • GitHub Check: Gen check
🔇 Additional comments (3)
legacy/firmware/Makefile (1)

10-14: Confirm new bitcoin‑only build name format

For BITCOIN_ONLY=1 you now emit classic1s.${FIRMWARE_BUILD_VERSION}-Stable-btc-${BUILD_COMMIT} while the non‑bitcoin name stays as ...-Stable-${BUILD_COMMIT}. Wildcard globs like classic*Stable*.bin still work, but please confirm no external tooling or release scripts rely on the old -Stable-${BUILD_COMMIT} format (for example by splitting on -Stable- and taking the tail as the hash).

.github/workflows/build-classic1s.yml (2)

98-113: QA signing and copying of intermediate_fw artifacts

The new block for non‑bitcoin builds:

  • Always copies classic1s_bootloader*Stable*.bin into the artifact dir.
  • For QA builds, signs ./legacy/intermediate_fw/classic1s_boot_update*.bin with QA keys via $SIGN_SCRIPT and copies classic1s_boot_update*qa.signed.bin.
  • For prod builds, copies the unsigned classic1s_boot_update*.bin.

This matches the existing firmware QA signing flow and should satisfy the “include intermediate_fw bin files in artifacts” goal. One thing to verify: for every non‑bitcoin matrix row, does cibuild always produce matching ./legacy/intermediate_fw/classic1s_boot_update*.bin files? If not, the cp with wildcards will fail and break the job.


72-79: Verify FIRMWARE_QA=0 semantics in QA builds

In the QA branch you export:

export FIRMWARE_QA=0
export PRODUCTION=0
export BOOTLOADER_QA=1

Setting FIRMWARE_QA to 0 appears counter-intuitive for a QA build. Please confirm how FIRMWARE_QA is consumed in C/Make code (search for #if FIRMWARE_QA, #ifdef FIRMWARE_QA, etc.) and verify that 0 is the intended value. If the flag uses standard semantics where non-zero means "enabled," this should be 1. If 0 is correct due to inverted logic or other reasons, consider clarifying this with a comment or renaming the flag to better reflect its semantics.


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

@lihuanhuan lihuanhuan merged commit ae331ad into OneKeyHQ:master Dec 10, 2025
5 of 8 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.

4 participants