fix(ci): propagate Playwright exit code through tee pipe by faultless-casey · Pull Request #57 · fluffyx/github-workflows

faultless-casey · 2026-05-03T05:09:44Z

Summary

Fix 1: Propagate Playwright/audit exit codes through `tee` pipes (6 files)

cmd | tee file returns tee's exit code (always 0), masking failures from the left side. set -e alone doesn't help — bash evaluates the pipeline exit code. set -o pipefail fixes this.

Playwright (false-green test runs):

ci-e2e.yml — the original, reported in fluffyx/billiedoby-scheduling PR #194
ci-svelte-e2e.yml — same pattern, same fix

Audit tools (silent security bypasses):

ci-gem.yml, ci-rails.yml, ci-rails-svelte.yml, ci-svelte.yml — bundler-audit/pnpm audit/bin/audit-frontend piped to tee with continue-on-error: true. The follow-up "Classify audit result" step checks outcome == 'failure', but outcome was always success because tee won the pipeline. Real vulnerabilities silently passed CI.

Fix 2: Scope e2e setup to the matrix suite only (`ci-e2e.yml`)

Each shard runs one suite (cd "$SUITE" && npx playwright test), but three setup steps looped over all frontend*/ directories. On billiedoby-scheduling (two suites × 2 shards = 4 jobs), this wasted ~46s/shard because the unused suite's pnpm install triggered prepare scripts for private git-branch packages.

Changed: SUITE hoisted to job-level env; the three for dir in frontend*/ loops in Install dependencies, Install Playwright browsers, and Generate GraphQL types replaced with direct pnpm --dir "$SUITE" calls.

Blast radius (Fix 1)

After landing, repos pinned to @main will start correctly failing when Playwright has failures or audit tools find real vulnerabilities. Expect previously-hidden failures to surface.

Reproducer

fluffyx/billiedoby-scheduling PR #194 (commit 47e46cc8): shard 1 showed 1 failed, 8 did not run, 8 passed but the GitHub Actions job was green.

Test plan

Consumer repo with always-failing e2e spec → job is now red
Revert always-fail spec → job goes green
Shard with zero tests → fails with "No tests were run in this shard"
Consumer repo with a known vuln in Gemfile.lock → audit job is now red
On a multi-suite consumer (e.g. billiedoby-scheduling): grep run log for ==> Installing — each shard should only show its own suite
Confirm wall-clock time of slowest e2e shard drops ~30–50s

🤖 Generated with Claude Code

Summary by CodeRabbit

Chores
- Improved CI reliability by enabling pipeline failure propagation for test and audit steps that pipe output.
- Reduced E2E runtime and cache scope by running frontend setup and browser installs only for the active test suite and narrowing cache keys to the suite and lockfile.
- Added explicit caching of the package manager store to speed repeated runs.

Note

Propagate exit codes through `tee` pipes in CI workflows using `pipefail`

Adds set -o pipefail to audit and test steps across multiple CI workflows so that failures in commands piped to tee are correctly surfaced instead of being masked by tee's exit code.
Refactors ci-e2e.yml to declare SUITE at job level, use explicit pnpm store caching keyed by suite, install Playwright only for the selected suite, and conditionally wait for dev readiness via bin/wait-for-e2e "$SUITE" with a fallback to polling localhost:${DEV_PORT:-4100}.
Documents the optional bin/wait-for-e2e script in README.md and notes the hardcoded DEV_PORT fallback in TODOS.md.

^{Macroscope summarized ab05aad.}

Without pipefail, `cmd | tee file` returns tee's exit status (always 0), so a Playwright run with failures silently exits 0 and the job goes green. Adding `set -o pipefail` makes the pipeline return Playwright's exit code. The grep-based "no tests ran" guard is preserved for the zero-test case. Fixes false-green seen on fluffyx/billiedoby-scheduling PR #194 (shard 1 reported 1 failed but the GH job was marked successful). Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

coderabbitai · 2026-05-03T05:09:54Z

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

Multiple GitHub Actions workflows were modified. Several steps now enable set -o pipefail so failures in commands piped to tee propagate (ci-gem.yml, ci-rails.yml, ci-rails-svelte.yml, ci-svelte.yml, and ci-svelte-e2e.yml). The ci-e2e workflow sets a SUITE env var from the matrix, scopes frontend setup and Playwright browser install to pnpm --dir "$SUITE", narrows Playwright cache keys to the suite and its lockfile, adds explicit pnpm store caching, and enables set -o pipefail when running Playwright tests.

Suggested reviewers

CharlieHelps
macroscopeapp

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately reflects the main change: adding `set -o pipefail` to propagate command failures through tee pipes in CI workflows, which is the primary fix across all modified files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch casey/fix-e2e-exit-code

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

macroscopeapp · 2026-05-03T05:11:01Z

Approvability

Verdict: Approved

CI/CD workflow changes only - adds set -o pipefail to ensure command exit codes propagate correctly through tee pipes, plus cache and env var reorganization in e2e workflow. No production runtime impact.

^{You can customize Macroscope's approvability policy. Learn more.}

…2e steps Five more instances of the same bug fixed in #57: a pipe to tee swallows the left-side exit code, causing audit tools and Playwright to report success even when they fail. The audit steps (ci-gem, ci-rails, ci-rails-svelte, ci-svelte) were especially broken: the continue-on-error + outcome == 'failure' pattern depended on the audit command's exit code propagating, but tee always wins the pipeline. Real vulnerabilities would silently pass. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

Dismissing prior approval to re-evaluate eddf401

Each e2e shard only runs one suite but previously installed dependencies, Playwright browsers, and ran codegen for every frontend*/ directory. On billiedoby-scheduling (frontend + frontend-book, 2 shards), this wasted ~46s per shard because the unused suite's install triggered prepare scripts for private git-branch packages. Hoist SUITE to job-level env so all steps can reference it, then replace the three for-dir-in-frontend*/ loops with direct pnpm --dir "$SUITE" calls. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

Dismissing prior approval to re-evaluate bd089ce

Both cache keys previously hashed all frontend*/pnpm-lock.yaml files, causing spurious cache misses when the non-tested suite's lockfile changed. Scope them to matrix.suite so each shard only invalidates on its own deps. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

Dismissing prior approval to re-evaluate 9346057

…cripts setup-node's implicit pnpm cache wasn't persisting the store across runs (visible in PR #194 logs — prepare ran on every install for git-ref deps like @fluffyx/fx-glass and @fluffyx/fx-core-svelte, costing ~46s/shard). Replace the implicit cache: pnpm on setup-node with an explicit cache step keyed on all frontend*/pnpm-lock.yaml files. The pnpm store is content- addressable so both suites share it naturally; cross-suite key means a store warm for frontend also benefits frontend-book shards and vice versa. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

Dismissing prior approval to re-evaluate 7a8a12a

coderabbitai

🧹 Nitpick comments (1)

.github/workflows/ci-e2e.yml (1)
188-189: ⚡ Quick win

Include architecture in Playwright cache key.

Line 188 currently keys cache by OS + suite + lockfile, but Playwright browser artifacts are architecture-specific. Add runner.arch to avoid cross-arch cache collisions.
Proposed change
-          key: playwright-${{ runner.os }}-${{ matrix.suite }}-${{ hashFiles(format('{0}/pnpm-lock.yaml', matrix.suite)) }}
-          restore-keys: playwright-${{ runner.os }}-${{ matrix.suite }}-
+          key: playwright-${{ runner.os }}-${{ runner.arch }}-${{ matrix.suite }}-${{ hashFiles(format('{0}/pnpm-lock.yaml', matrix.suite)) }}
+          restore-keys: playwright-${{ runner.os }}-${{ runner.arch }}-${{ matrix.suite }}-

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4799a07d-6c15-48d2-a438-4da3a219e53a

📥 Commits

Reviewing files that changed from the base of the PR and between 9346057 and 7a8a12a.

📒 Files selected for processing (1)

.github/workflows/ci-e2e.yml

Cross-suite key caused spurious invalidations: a lockfile bump in one suite would bust the store cache for the other. Per-suite key means each shard is independently warm/cold on its own lockfile, consistent with all other suite-scoped caches in this job. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

Dismissing prior approval to re-evaluate 1d18d0c

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

Dismissing prior approval to re-evaluate ec78f60

…-aware Per-suite install broke consumer repos where bin/dev-e2e starts both Vite servers in parallel (billiedoby-scheduling frontend-book shards timed out at "Wait for dev server" because frontend/node_modules was never installed). Revert install and codegen back to the for-dir-in-frontend*/ loop. The pnpm store cache makes this fast on warm runs — prepare scripts on git-ref packages are skipped when the store is populated. Switch pnpm store cache key to union over all lockfiles so all four shards share one cache entry (each shard installs both frontends, so both lockfiles contribute to the same store). Fix latent bug in wait-for-server: frontend-book shards now poll book.lvh.me instead of app.lvh.me, matching the host their Playwright config targets. Playwright install and cache remain scoped to $SUITE — only the suite under test needs browsers, and suites can pin different versions. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

Replace hardcoded consumer-specific hostnames (app.lvh.me, book.lvh.me) with the bin/wait-for-e2e convention. The workflow calls the consumer's script with $SUITE as $1 if it exists, otherwise falls back to polling localhost:$DEV_PORT. This follows the repo's existing pattern — every other consumer-specific concern is already a bin/ script. Consumer repos with multi-frontend Caddy setups (like billiedoby) provide their own bin/wait-for-e2e with the hostname mapping. Single-frontend repos need no script — the fallback works. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

Dismissing prior approval to re-evaluate ab05aad

github-actions Bot requested a review from CharlieHelps May 3, 2026 05:09

coderabbitai Bot approved these changes May 3, 2026

View reviewed changes