perf(ci): cut pipeline wall clock from ~7:20 to ~3:00

[pastaghost]

Result

Measured, not projected. Baseline is run 33043154603 on develop; final is run 33046944618 on this branch.

	baseline	this branch
total wall clock	440s	183s
jobs	11	7
emulator compiles per run	2	1
845 MB artifact round-trip	every run	publish-dispatch only

Per job (final run): lint-format 8s · secret-scan 21s · static-analysis 94s · build-and-test 155s · build-arm-firmware 81s · python-dylib-tests 58s.

The critical path used to be a single serial chain — static-analysis 192s → build-emulator 175s → unit-tests 54s → report 9s. It is now just gate ~20s → build-and-test 155s, with cppcheck running alongside.

Why there was so much to reclaim

Layer-level timings from the baseline build log — the firmware compile was never the expensive part:

layer	time
pip install rlp eth-keys eth-utils pycryptodome	36.8s
FROM kktech/firmware:v15 (base pull)	34.4s
apk add python3-dev gcc musl-dev	~8s
make -j — the actual firmware compile	9.6s

The emulator image was compiled twice per run, an 845 MB tarball was moved between two runners to run a 1s suite (unit-tests spent 49s on transfer for 1s of testing), and cppcheck ran single-threaded while every build job waited on it.

What changed

Structural

• Merge build-emulator + unit-tests + python-integration-tests into one build-and-test. Image built once, both suites run against it in place.
• Fold generate-test-report into build-and-test — every input was already on the runner.
• Fold check-submodules into lint-format — a 3s grep costing a full runner spin-up in the needs fan-in of every downstream job.

Ungating

• static-analysis no longer gates builds. cppcheck is advisory; it still fails the run, it just no longer delays it. lint-format and secret-scan stay in the gate.

Caching

• type=gha cache on the python-keepkey deps stage only. Cold 75s → warm 15s.
• Base image mirrored to GHCR with Docker Hub fallback. Final run logs base image: GHCR mirror.
• Base-image tar cache deleted — it was ref-scoped, 13 copies held 8.4 GB of the 10 GB quota and evicted each other. Those caches have been purged (repo now 2 caches / 19 MB).
• pip + protoc caching in python-dylib-tests.

Parallelism

• cppcheck --force -j8. Measured locally, identical findings: -j1 77s → -j4 31s. --force kept deliberately — dropping it is ~5x faster again but stops the combinatorial #ifdef sweep.
• clang-format via xargs -P, installed from PyPI rather than the LLVM apt repo. lint-format 21s → 8s, and it now also does the submodule check.
• Submodule init --jobs 4, with --recursive scoped to python-keepkey only — trezor-firmware declares 9 nested submodules this build never uses.

Waste

• concurrency group cancels superseded PR runs; pushes to develop/master/release are never cancelled.
• Docs-only commits skip the workflow.
• docker compose down -v dropped; healthcheck interval 3s → 1s.

Two bugs found and fixed in review of this branch's own CI runs

Both were introduced by this branch and caught by inspecting real runs. They are the reason for commits 3 and 4.

1. The layer cache made things slower. First attempt put cache-to: mode=max on the whole image. .dockerignore excludes only **/.git and bin, so COPY ./ /kkemu ships essentially the entire context — and it is invalidated by every commit, so it can never hit. The run spent 103.9s in exporting to GitHub Actions Cache to save 36.8s of pip. Fixed by splitting the Dockerfile into a deps stage and caching that target alone.

2. The pipeline went green while silently losing every integration test result. Run 33046518184 uploaded no python results and no screenshots. The tests were fine — 379 passed, 58 skipped, 310 PNGs — but make xunit runs as root and leaves a root-owned test-reports/firmware-unit/; docker cp chmods what it extracts into, could not, and failed outright. || echo "WARN: ..." swallowed it. When the suites were separate jobs the integration job always started with a pristine test-reports/. Fixed by chowning the bind mount, and by gating extraction so lost evidence fails the run instead of warning.

Verification

Artifacts from the final run, checked against baseline:

artifact	this branch	baseline
junit.xml	437 tests / 58 skipped / 0 failures	437 / 58
junit-screenshots.xml	85 / 8 skipped	85 / 8
screenshot PNGs	310	~310
test-report.pdf	present	present
dylib junit	4 / 0 failures	4

Also: every job resolves an identical submodule set to develop (checked programmatically); actionlint reports the same 5 pre-existing findings, zero new.

For the reviewer

Two decisions worth your explicit attention:

1. paths-ignore skips the workflow entirely on docs-only commits rather than reporting a neutral check. Safe today because develop is unprotected and master's only required context is ci/circleci: emulator-build-test — a job with no .circleci/ config in this repo, so it can never report. If you want CI jobs to become required checks, this needs rethinking first, or docs PRs will become unmergeable.
2. static-analysis no longer gates builds, so compute is spent before cppcheck's verdict is known. Deliberate — it was the pole.

Not done, deliberately

• Collapsing the two pytest phases — Phase 1 costs 3.3s, and merging it would run all 437 tests under KEEPKEY_SCREENSHOT=1 instead of the 85-test SECTIONS filter, changing the 90-day screenshot artifact and the report input.
• cppcheck --cppcheck-build-dir caching — ~85s → ~30s, but that job is off the critical path now, so it buys billed minutes in exchange for incremental-analysis cache risk on a security tool. Better as its own change.
• Ungating python-dylib-tests — no wall-clock gain, and the gate stops a lint-failing commit spending 10x-billed macOS minutes.

[Copilot]

Pull request overview

This PR restructures the GitHub Actions CI workflow to significantly reduce wall-clock time by removing redundant Docker builds/artifact shuttling, increasing parallelism, and introducing better caching/mirroring for large base images.

Changes:

• Consolidates emulator build + firmware unit tests + python integration tests (and report generation) into a single build-and-test job to avoid rebuilding and re-uploading/loading large Docker images.
• Adds base-image mirroring to GHCR (with Docker Hub fallback) and introduces a dedicated mirror-base-image workflow to populate the mirror.
• Improves CI efficiency via higher parallelism (cppcheck -j8, clang-format fan-out), concurrency cancellation for PRs, and docs-only workflow skipping.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.

File	Description
.github/workflows/ci.yml	Major CI pipeline restructure (job consolidation, caching/mirroring, parallelism, concurrency, docs-only skipping).
.github/workflows/mirror-base-image.yml	New manual workflow to mirror the Docker Hub base image into GHCR for faster pulls.
scripts/emulator/docker-compose.yml	Compose tweaks to support single-build reuse in CI and faster healthcheck polling.
.gitignore	Ignores cppcheck output files generated by CI runs.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

.github/workflows/ci.yml:213

• Severity: high. This step is intended to keep running after cppcheck returns non-zero so it can cat cppcheck_report.txt (for annotations) and build the summary counts, but under GitHub Actions’ default bash -e a failing cppcheck command will terminate the step immediately. As written, the later CPPCHECK_RC=$?/summary logic won’t run on the runs where cppcheck finds issues, reducing diagnostics. Capture the exit code via an || list (or temporarily disable -e) so the remainder of the step executes and can emit annotations + a clear error.

            --inconclusive \
            --force \
            -j8 \
            --inline-suppr \
            --suppressions-list=.cppcheck-suppressions \

[pastaghost]

Round 2 reported no new inline comments, but carried one suppressed finding in the review body (ci.yml:213, severity high, "previously missed"). Recording here since a suppressed finding has no thread to resolve.

It was correct, and it is a third instance of the bash -e pattern the other two round-1 findings covered. cppcheck runs with --error-exitcode=1, so it returns non-zero whenever it finds anything; run: steps execute under bash -e; so cppcheck ...; CPPCHECK_RC=$? terminated the step at the cppcheck line on exactly the runs that have findings. The cat cppcheck_report.txt that emits the ::warning file= annotations, the per-severity summary written to GITHUB_STEP_SUMMARY, and the explanatory ::error:: all never ran — leaving a bare red step with no indication of what cppcheck found.

Fixed in 6e8647d with || CPPCHECK_RC=$?. Note this one is pre-existing on develop rather than introduced by this PR; picked up here because the step was already being modified to add -j8.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

.github/workflows/ci.yml:241

• Severity: medium. The cppcheck summary counts use grep -c '\b...' but grep (without -P) treats \b as a backspace escape, not a word boundary, so these counters will likely stay at 0 and the step summary becomes misleading. Match the actual ::{severity}: tokens from the template using fixed-string grep (or add -P).

          # Count issues by severity
          ERRORS=$(grep -c '\berror:' cppcheck_report.txt 2>/dev/null || true)
          WARNINGS=$(grep -c '\bwarning:' cppcheck_report.txt 2>/dev/null || true)
          STYLE=$(grep -c '\bstyle:' cppcheck_report.txt 2>/dev/null || true)
          PERF=$(grep -c '\bperformance:' cppcheck_report.txt 2>/dev/null || true)
          PORT=$(grep -c '\bportability:' cppcheck_report.txt 2>/dev/null || true)

.github/workflows/ci.yml:385

• Severity: medium. The unit-test extraction suppresses cp errors and never verifies that any *.xml actually made it into test-reports/firmware-unit/. If make xunit stops producing XML (or the glob doesn’t match), the job can still proceed and generate a misleading PDF/report with "0 tests". Consider gating on at least one copied XML (while still preserving the original test exit code).

            -c "mkdir -p /kkemu/test-reports/firmware-unit && \
                make xunit; RC=\$?; \
                cp -r unittests/*.xml /kkemu/test-reports/firmware-unit/ 2>/dev/null; \
                exit \$RC" || UNIT_RC=$?

[pastaghost]

Round 3: no new inline comments, but two more suppressed findings in the body. Recording both here since suppressed findings have no thread to resolve. One fixed, one declined with evidence.

ci.yml:385 — unit XML extraction unverified (medium). Fixed in 21ed963.

Correct, and it is the same silent-evidence-loss mode that already bit this branch: an earlier run went green having discarded 437 test results because a docker cp failed into || echo WARN. The unit side had the same shape — cp -r unittests/*.xml ... 2>/dev/null discards the error, so a build that stops emitting XML would produce a report reading "0 tests" with nothing objecting. Now gated: missing XML is treated as an infrastructure failure distinct from a test failure, and fails at the point of loss.

ci.yml:241 — grep -c '\berror:' counters (medium). Declined; the premise does not hold.

The claim is that grep without -P treats \b as a backspace escape rather than a word boundary, so the counters stay at 0. That is true of some grep implementations, but \b is a GNU extension supported in BRE, and ubuntu-latest runs GNU grep. Verified in a Linux container against the actual annotation format rather than reasoned about:

$ docker run --rm ubuntu:24.04 ...
grep (GNU grep) 3.11
--- \b word-boundary counts (GNU grep, BRE) ---
  error        1
  warning      1
  style        1
  performance  1
  portability  1
--- sanity: does \b actually anchor? ---
  matches in "xerror: nope": 0

All five counters match, and the negative control confirms \b is anchoring rather than being taken literally. The counters work as written; changing them would be churn against a false premise.

Worth noting the summary genuinely does read zero on this branch — but because cppcheck is clean here, not because the counting is broken.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

.github/workflows/mirror-base-image.yml:45

• Severity: low. This comment claims the image is mirrored under both the bare name (no tag) and its tag, but the script only tags/pushes a single reference (DEST includes the tag). That mismatch can confuse maintainers who expect an additional :latest/untagged push.

Recommendation: reword the comment to match what the script actually does (mirror the tagged image), or add a second tag/push if you truly want both.

          # Mirror under the bare image name (no tag) plus its tag, so the
          # destination reads ghcr.io/<owner>/firmware:<tag>.

.github/workflows/ci.yml:148

• Severity: low. grep -v '.pb.' treats . as a regex wildcard, so it will exclude any path containing any character + pb + any character (not just literal .pb.). That can unintentionally skip files from the formatting check.

Recommendation: use fixed-string matching (or escape the dots) to exclude only generated protobuf artifacts.

               | grep -v generated | grep -v '.pb.' \
               | xargs -P "$(nproc)" -I{} sh -c '

[Copilot]

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

.github/workflows/ci.yml:148

• Severity: medium. The find expression -name '*.c' -o -name '*.h' relies on the implicit -print, which (due to -o short-circuiting) only prints the right-hand branch; this ends up skipping *.c files. Also, lib/transport/src does not exist in this repo, so lib/transport/*.c currently isn't checked at all (errors are just suppressed). Wrap the name tests in parentheses and include lib/transport so both C and header files are formatting-checked.

          if ! find include/keepkey lib/firmware lib/board lib/transport/src \
                    -name '*.c' -o -name '*.h' 2>/dev/null \
               | grep -v generated | grep -vF '.pb.' \
               | xargs -P "$(nproc)" -I{} sh -c '

[pastaghost]

Round 5 also carried a suppressed finding at ci.yml:148 with two claims. One holds, one does not, and its recommendation is deliberately not taken.

Holds — lib/transport/src does not exist. The transport sources sit directly in lib/transport/. Because find's stderr went to /dev/null, the bad path contributed nothing and said nothing. Dropped in f37e513, and stderr is no longer suppressed so a future bad path is loud rather than quietly narrowing coverage.

Does not hold — the -o short-circuit claim. The finding says the implicit -print only applies to the right-hand branch, skipping *.c files. GNU find applies it to the whole expression. Verified under the same findutils CI uses:

find (GNU findutils) 4.9.0
current expression:      77 c    120 h

77 .c files were being checked all along. Parenthesised anyway so the intent is explicit to a reader; the resulting file set is identical at 77/120.

Recommendation declined — adding lib/transport would turn CI red. That directory holds the vendored nanopb runtime. Checked against this repo's .clang-format with clang-format 20.1.8:

  CLEAN  lib/transport/pb_common.c
  DIRTY  lib/transport/pb_decode.c
  CLEAN  lib/transport/pb_encode.c

Including it would fail the formatting gate immediately, and reformatting vendored upstream sources to house style makes future nanopb updates needlessly painful. The exclusion is now commented so it reads as deliberate rather than accidental.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

.github/workflows/ci.yml:945

• Severity: high. publish-emulator no longer depends on static-analysis, so a workflow_dispatch publish can push DockerHub images even when cppcheck fails (the overall run will be red, but publishing can still complete). If cppcheck is meant to “still fail the run”, it should also block publishing.

  publish-emulator:
    needs: [build-and-test, python-dylib-tests, build-arm-firmware]
    if: >-

[pastaghost]

Round 6 suppressed finding (ci.yml:945, severity high) — correct, and a real regression introduced by this branch. Fixed in c54bbcc.

Taking static-analysis out of the build gate was deliberate: a cppcheck finding does not mean the firmware is wrong to compile, and as the slowest Stage 1 job it was serialising everything behind itself. But publish-emulator used to inherit that gate transitively, through build jobs that each depended on static-analysis. Rewriting its needs onto the merged build-and-test dropped the dependency altogether — so a workflow_dispatch publish could push kktech/kkemu images to DockerHub from a commit with cppcheck red. The run would be red; the push would still complete.

static-analysis is now listed explicitly on publish-emulator. That path is manual and already slow, so the gate costs nothing there — the speed trade was only ever meant to apply to compiling, not to publishing artifacts other things consume.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (4)

Previously missed (3) — in code that hasn't changed since the last review.

.github/workflows/ci.yml:333

• Severity: medium. This workflow describes the GHCR mirror as optional (it falls back to Docker Hub), but the preceding docker/login-action step will currently fail the entire job if GHCR login errors, preventing the fallback from running. Making the login non-fatal keeps the mirror truly optional.

This issue also appears on line 634 of the same file.

      - name: Log in to GHCR
        uses: docker/login-action@v4
        with:
          registry: ghcr.io
          username: ${{ github.actor }}

.github/workflows/ci.yml:645

• Severity: low. In build-arm-firmware this job sets up Buildx and exports RESOLVED_BASE (and includes a long comment about buildx behavior), but the job never uses buildx nor RESOLVED_BASE afterwards—only docker pull/tag and docker run ${{ env.BASE_IMAGE }}. This adds a bit of overhead and makes the step comments misleading for this job; consider dropping Buildx + RESOLVED_BASE here and simplifying the Pull base image comment accordingly.

      - name: Set up Docker Buildx
        uses: docker/setup-buildx-action@v4

      - name: Pull base image
        run: |

.github/workflows/ci.yml:814

• Severity: low. The protoc download is skipped when /tmp/protoc.zip exists, but the cache can restore an empty or partial file; -f only checks existence, so this can lead to an unzip failure with no retry download. Using -s (non-empty) makes the step resilient to a bad cache entry.

          if [ ! -f /tmp/protoc.zip ]; then
            curl -sSL -fL -o /tmp/protoc.zip \
              "https://github.com/protocolbuffers/protobuf/releases/download/v${PROTOC_VERSION}/${PROTOC_ASSET}"

.github/workflows/ci.yml:638

• Severity: medium. Same as build-and-test: GHCR login failing will currently fail the whole job before the mirror fallback can run. If the mirror is intended to be optional, the login step should be non-fatal.

      - name: Log in to GHCR
        uses: docker/login-action@v4
        with:
          registry: ghcr.io
          username: ${{ github.actor }}

[pastaghost]

Round 7 carried four suppressed findings, all valid, all fixed in 0f2a16c.

• GHCR login was fatal (medium, both build jobs). The sharpest of the four. The mirror is optional by design — CI warns and falls back to Docker Hub — but docker/login-action failing aborts the job before the fallback it exists to enable can run. A GHCR outage, a revoked package grant, or a token hiccup would have taken CI down despite the fallback being right there. Now continue-on-error: true; a failed login just means the mirror pull is unauthenticated, which takes the fallback path.
• build-arm-firmware set up Buildx and exported RESOLVED_BASE, using neither (low). Introduced by this PR and my own sloppiness — the pull-with-fallback step was applied to both build jobs identically and buildx came along for the ride. That job runs the toolchain via docker run and needs only the daemon-local tag. Removed, along with the buildx commentary that did not apply to it.
• protoc cache checked existence, not content (low). A cache entry can restore empty or truncated; -f would skip the download and hand unzip a bad file with no retry. Now -s.

For the record on where this loop stands: rounds 2 through 7 have produced zero new inline comments — every finding has been previously missed, in code unchanged since the prior round. That pattern is a reviewer surfacing pre-existing issues incrementally rather than a fix loop churning, and it is worth knowing the review effort level here is Lite.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

.github/workflows/ci.yml:536

• This workflow mostly uses actions/upload-artifact@v7, but this step still uses @v4. Mixing major versions can lead to subtle behavior differences; standardize on one version (v7 seems to be the intent here).

This issue also appears on line 583 of the same file.

      - name: Upload OLED screenshots
        uses: actions/upload-artifact@v4
        if: always()

.github/workflows/ci.yml:585

• This step still uses actions/upload-artifact@v4 while the rest of the workflow uses @v7. Standardizing avoids inconsistent artifact handling across jobs/steps.

      - name: Upload test report
        uses: actions/upload-artifact@v4
        if: always()

[Copilot]

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.