Skip to content

fix(ci): skip release instead of erroring when both fixtures are cached - #2036

Merged
thymikee merged 4 commits into
mainfrom
claude/mystifying-brattain-9960d5
Aug 26, 2026
Merged

fix(ci): skip release instead of erroring when both fixtures are cached#2036
thymikee merged 4 commits into
mainfrom
claude/mystifying-brattain-9960d5

Conversation

@thymikee

Copy link
Copy Markdown
Member

Summary

Fixes #2034. Test App Build Cache has failed on every run since 2026-08-24 14:42 UTC, including on main.

#1996 changed the release matrix builder to filter to only entries that will actually build:

-            '{ include: [ {...iOS...}, {...Android...} ] }')"
+            '[ {...iOS...}, {...Android...} ] | { include: map(select(.build)) }')"

That's correct when at least one fixture needs building, but when both are already cached, map(select(.build)) yields include: []. GitHub Actions rejects an empty strategy.matrix at the workflow level, so the release job is never created and the whole run is marked failure. Both-cached is the steady state, which is why this fails on nearly every run rather than intermittently.

  • fingerprint now also publishes a has-work output (jq -r '.include | length > 0' over the matrix).
  • release gets if: needs.fingerprint.outputs.has-work == 'true', so a both-cached run skips release instead of erroring.

I checked whether a skipped release breaks anything downstream that needs: it: nothing does — consumers (setup-fixture-app / trusted-artifact.mjs) poll classifyProducerState, which reads the workflow run's overall status/conclusion, not individual job state. A workflow with a skipped job (and no failed jobs) reports success, so consumers correctly see the producer as done rather than failed.

Test plan

  • Added a regression test (test/ci/trusted-fixture-artifact.test.mjs) that executes the real fingerprint jq/shell step across all four cache combinations (both-cached, iOS-only, Android-only, neither), pinning that both-cached produces include: [] + has-work=false, and every other combination produces has-work=true. This is the combination that shipped broken and had no prior coverage.
  • Updated the existing producer test to account for the new has-work= output line in $GITHUB_OUTPUT.
  • pnpm run test:fixture-cache — all 12 tests pass.
  • actionlint .github/workflows/test-app-build-cache.yml — clean.
  • oxlint on the modified test file — clean.

After merge, a push to main with no native change should show Test App Build Cache green with release skipped rather than errored; a run where a fingerprint actually changed should still build and publish as before.

…ed (#2034)

map(select(.build)) yields an empty include list when both the iOS and
Android fingerprints already have a trusted artifact, and GitHub Actions
rejects an empty strategy.matrix at the workflow level -- so release was
never created and the run was marked failure on every push since #1996
merged. Publish has-work alongside matrix and gate release on it, so the
both-cached steady state now completes with release skipped instead of
erroring the whole workflow.
@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 2.44 MB 2.44 MB -1.6 kB
JS gzip 818.1 kB 817.6 kB -514 B
npm tarball 942.0 kB 941.5 kB -491 B
npm unpacked 3.27 MB 3.27 MB -1.6 kB

npm unpacked components

Component Base Current Diff
JS / dist source 2.60 MB 2.60 MB -1.6 kB
Apple runner source/project 570.8 kB 570.8 kB 0 B
macOS helper source 54.7 kB 54.7 kB 0 B
Android helper artifacts 0 B 0 B 0 B
Other package files 45.3 kB 45.3 kB 0 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 26.0 ms 25.9 ms -0.1 ms
CLI --help 79.3 ms 78.1 ms -1.2 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/sdk-batch-runner.js -77 B -27 B
dist/src/device-claim-conflict.js -39 B -9 B
dist/src/cli-help.js +9 B +4 B
dist/src/internal/daemon.js 0 B -1 B

Top changed packed files

Packed file Base Current Diff
dist/src/agent-browser-tool.js 11.4 kB 10.0 kB -1.4 kB
dist/src/screenshot-runtime.js 14.3 kB 14.1 kB -114 B
dist/src/sdk-batch-runner.js 78.5 kB 78.4 kB -77 B
dist/src/agent-browser-provider.js 17.8 kB 17.9 kB +43 B
dist/src/device-claim-conflict.js 90.1 kB 90.0 kB -39 B
dist/src/cli-help.js 89.4 kB 89.4 kB +9 B
dist/src/snapshot2.js 6.1 kB 6.1 kB +5 B

Reviewer feedback on #2036: the standalone four-case test duplicated the
harness above it and only two states are meaningful for this regression.
Reuse the same parsed workflow, temp dir, resolver stub, and Node stub;
keep neither-cached (both platforms, has-work=true) and both-cached
(empty matrix, has-work=false, release gated). Drops the single-cache
permutations, which exercise #1996's unchanged filtering rather than
this fix.
@thymikee

Copy link
Copy Markdown
Member Author

Re-reviewed 9eaea0bc1aa2edde686285e1c260cc436d83df4d. The production fix is correct, the duplicated harness is gone, all checks are green, and the live workflow proves fingerprint-success + release-skipped + overall success. One P2 regression gap remains: the reduced test covers matrix cardinalities 0 and 2, but not 1. A mistaken length > 1 would pass while suppressing a valid single-platform build. Add exactly one single-cache case to the reused harness, asserting a one-entry matrix and has-work=true; do not restore both permutations or the duplicate setup. Re-review after that head change.

Reduced coverage to 0-cached and 2-cached, leaving the 1-cached
cardinality unchecked -- a mistaken \`length > 1\` in the has-work
check would pass while wrongly suppressing a valid single-platform
build. Generalize the Node stub to report caching per artifact-name
suffix and add the iOS-cached case to the same reused harness.
@thymikee

Copy link
Copy Markdown
Member Author

Added exactly one single-cache case to the reused harness in fb49d11: the Node stub now takes a cachedPlatforms list keyed off the artifact-name suffix, so the same run/output-parsing helper exercises 0/1/2 cached cardinalities without restoring both permutations or duplicating setup. New assertion: iOS cached only → one-entry android matrix, has-work=true (would catch a length > 1 mistake, since that reads as has-work=false there). All 11 tests still pass.

🤖 Addressed by Claude Code

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Aug 25, 2026
@thymikee

Copy link
Copy Markdown
Member Author

Re-review at fb49d11cef196d8f49255aaa0f9694f999b34a42: the P2 is resolved without duplicating setup. The reused workflow harness now proves all three matrix cardinalities: 2 (neither cached), 1 (iOS cached -> Android-only and has-work=true), and 0 (both cached -> empty and has-work=false). No remaining code findings. The prior Android smoke red was unrelated automation-host/device timing (wait text "Alert result: cancelled" hit its 10s deadline); this PR does not touch Android runtime paths, adjacent/prior runs passed, and the rerun has cleared setup/cache/catalog steps and is currently executing the smoke. Code is ready for human review; CI rerun still in progress.

…lines

Thermo-nuclear review: matrix was already parsed out of its GITHUB_OUTPUT
line (prefix stripped, JSON-parsed), but hasWork returned the raw
"has-work=true" line, so assertions compared against a redundant
'has-work=true' string instead of the actual value. Slice the prefix
the same way matrix does.
@thymikee

Copy link
Copy Markdown
Member Author

Delta re-review at bea44642fd344c9f88802bc9dadd5dc0852a165d: clean. The 8-line test-only change strips the has-work= prefix before asserting the actual boolean string, matching the matrix parsing style; missing output still fails structurally, and the 2/1/0 cardinality coverage remains intact. Test App Build Cache is green with the release matrix skipped as intended. ready-for-human remains appropriate; current-head checks are still running, so this is code-review readiness, not yet merge-ready.

@thymikee

Copy link
Copy Markdown
Member Author

Current-head CI is now complete: all 19 non-skipped checks passed, and the sole skipped matrix.name is the intended no-work release path. The prior clean code verdict is now merge-ready.

@thymikee
thymikee merged commit b40debf into main Aug 26, 2026
20 checks passed
@thymikee
thymikee deleted the claude/mystifying-brattain-9960d5 branch August 26, 2026 05:49
@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-26 05:49 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test App Build Cache fails on every run when both fixture fingerprints are already cached

1 participant