ci(pr): run tests across feature sets, not just default

[bdimitrov-netzine]

Problem

The PR `tests` job ran `cargo test` with default features only, while `feature-build-matrix.yml` merely compiled the non-default feature sets. Tests (and alternate code branches) gated behind non-default features were therefore compiled but never executed anywhere in CI.

Change

Convert the single `tests` job into a matrix:

leg	what it covers
`default`	full workspace, default features (command unchanged)
`dev-single-node-setup`	the `types` / `primary` / `orchestrator` unit tests gated on this feature — single-authority committee, sole-member consensus, and epoch-transition-mode — plus the alternate single-node production branches those crates take
`archive-replay`	evm-crate-local feature not reachable from the `rayls-network` bin, so the feature-build matrix can't cover it; `cargo test` here compile-checks the alternate `reth_env`/`chainspec`/block-execution paths and re-runs the evm suite through the feature-gated `test_utils` branches

Newly-executed gated tests: `committee_allows_single_authority`, `single_validator_commits_a_subdag`, `test_decide_node_mode_sole_member_without_override_returns_inactive`, `test_decide_node_mode_observer_sole_member_stays_observer`.

Notes

• Each leg uses its own `rust-cache` shared-key so feature-divergent `target/` dirs don't clobber one another; `fail-fast: false` so one failing set doesn't mask the others.
• `redb` was intentionally not added — we ship on reth-libmdbx.
• `faucet` / dev e2e remain uncovered (they belong in `nightly-e2e.yml`) — out of scope here.

[github-actions]

Claude finished @bdimitrov-netzine's task in 1m 45s —— View job

---

PR Review

• Gather context and understand the request
• Read CLAUDE.md for repo-specific guidelines
• Review .github/workflows/pr.yaml changes
• Provide comprehensive review feedback

---

This is a well-motivated, clean change that closes a genuine gap: feature-gated tests were being compiled by feature-build-matrix.yml but never actually executed anywhere in CI. The design decisions (separate cache keys, fail-fast: false, preserving the pr-tests cache key for the default leg) are all sound. A few things worth discussing:

---

Security / pattern: run: ${{ matrix.cmd }}

File: .github/workflows/pr.yaml, line 133

- name: Run tests
  run: ${{ matrix.cmd }}

The GitHub Actions security hardening guide explicitly warns against injecting ${{ expression }} values directly into a run: shell block, because the value is interpolated before the shell sees it — enabling shell injection if the input is attacker-controlled. In this case the matrix is statically hardcoded, so there is no practical injection risk today.

The concern is forward-looking: this pattern makes it easy for a future maintainer to add a matrix leg that inadvertently references a context value derived from user input (e.g. a PR title, branch name). Prefer the safer pattern:

env:
  TEST_CMD: ${{ matrix.cmd }}
run: $TEST_CMD

Or, better, decompose cmd into its constituent parts so the run: block is a fixed template:

matrix:
  include:
    - name: default
      packages: "--workspace --exclude rayls-execution-faucet --exclude e2e-tests"
      features: ""
    - name: dev-single-node-setup
      packages: "-p rayls-infrastructure-types -p rayls-consensus-primary -p rayls-middleware-orchestrator"
      features: "--features dev-single-node-setup"
    - name: archive-replay
      packages: "-p rayls-execution-evm"
      features: "--features archive-replay"

run: cargo test ${{ matrix.packages }} ${{ matrix.features }} -- --quiet --test-threads 1

This makes the shared skeleton visible, reduces repetition, and keeps the run: block injection-safe. Fix this →

---

Feature legs run with default features enabled

Lines 91 and 100 don't pass --no-default-features:

cmd: cargo test -p ... --features dev-single-node-setup -- ...
cmd: cargo test -p rayls-execution-evm --features archive-replay -- ...

This means each leg tests default features + the named feature. This is likely intentional (test the feature in the environment it will actually be used in), but it's worth a deliberate call:

• If a test is gated on #[cfg(feature = "dev-single-node-setup")] and some default feature causes a conflicting code path, the combination might behave differently from --no-default-features --features dev-single-node-setup.
• For archive-replay, which gates no dedicated tests and is valued for compile-checking alternate code paths, running without defaults would be a stronger check — it would ensure the feature compiles standalone.

Neither is necessarily wrong, but making the choice explicit (even just a comment "intentionally WITH defaults") would help future maintainers understand why.

---

dev-single-node-setup leg scope: worker and bridge

The comment in the workflow says:

orchestrator's variant transitively enables it in primary/worker/bridge/types

But the -p flags only include rayls-infrastructure-types, rayls-consensus-primary, and rayls-middleware-orchestrator — not worker or bridge. Transitive feature-enabling during compilation is correct (Cargo's feature graph handles it), but tests in worker and bridge are not collected or executed unless those crates are listed with -p.

If worker and bridge have tests that exercise dev-single-node-setup-gated branches, they should be added to the -p list. If they don't, a short comment noting this would prevent future confusion. The PR description only mentions the four named tests, all in types/primary/orchestrator, so this may already be the complete picture.

---

Minor: CI wall-clock time

Each of the three legs serializes test execution with --test-threads 1. This triples the serialized test time compared to the original single job. This is a deliberate correctness trade-off (the comment in the pre-existing code explains the rationale), but it's worth monitoring. If the matrix legs become slow, parallelizing at the test level (or splitting the default leg's --test-threads) would be the lever to pull.

---

Summary

The change is correct, well-commented, and fills a real gap. The main action item is the run: ${{ matrix.cmd }} pattern — it works today but is fragile over time. The feature/scope questions above are worth a quick sanity check before merging but may already be intentional. No blockers.

[github-actions]

Security Scan - Code

Severity: HIGH, CRITICAL

No vulnerabilities found

View scan results

Report Summary

┌─────────────────────────────────────────────┬────────────┬─────────────────┬───────────────────┐
│                   Target                    │    Type    │ Vulnerabilities │ Misconfigurations │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ Cargo.lock                                  │   cargo    │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ crates/testing/fuzz-targets/Cargo.lock      │   cargo    │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ crates/testing/fuzz-targets/fuzz/Cargo.lock │   cargo    │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/state-sum/Cargo.lock                    │   cargo    │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/tps/package-lock.json                   │    npm     │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ rayls-contracts/package-lock.json           │    npm     │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/chaos-network/Dockerfile                │ dockerfile │        -        │         0         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/docker-network/Dockerfile               │ dockerfile │        -        │         0         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/docker-replay/Dockerfile                │ dockerfile │        -        │         0         │
└─────────────────────────────────────────────┴────────────┴─────────────────┴───────────────────┘
Legend:
- '-': Not scanned
- '0': Clean (no security findings detected)

[github-actions]

Claude finished @bdimitrov-netzine's task in 1m 46s —— View job

---

PR Review

• Gather context and understand the request
• Read CLAUDE.md for repo-specific guidelines
• Review .github/workflows/pr.yaml changes
• Provide comprehensive review feedback

---

This PR closes a real gap — feature-gated tests were compiled by feature-build-matrix.yml but never executed in CI. The two-commit structure is clean: the matrix conversion first, then the gate job for branch protection. Overall design is sound. A few items worth discussing:

---

run: ${{ matrix.cmd }} — shell injection pattern

Line 129

- name: Run tests
  run: ${{ matrix.cmd }}

The matrix is statically hardcoded so there is no injection risk today. The concern is forward-looking: this pattern normalises inlining expressions into run: blocks, making it easy for a future maintainer to accidentally introduce a matrix leg that references attacker-influenced context (e.g. a branch name or PR title). The safer equivalent:

env:
  TEST_CMD: ${{ matrix.cmd }}
run: $TEST_CMD

Or decompose cmd into packages + features fields so the run block stays a fixed template — this also makes the shared structure explicit and eliminates repetition. Fix this →

---

tests-gate design — correct but worth a comment on cancelled

Lines 134–146

tests-gate:
  needs: tests
  if: always()
  steps:
    - name: Verify all test legs passed
      run: |
        if [[ "${{ needs.tests.result }}" != "success" ]]; then
          exit 1
        fi

The gate correctly handles failure and skipped. One case worth being explicit about: if the parent workflow run is cancelled (e.g. a force-push while tests are running), needs.tests.result will be cancelled, and the gate will exit 1 — which means branch protection remains unsatisfied and the PR can't be merged until a clean run completes. This is the correct behaviour, but it can surprise people who cancel a run and then find the PR gated. A one-line comment would document the intent:

# cancelled → exit 1 is intentional: a cancelled run must not satisfy the gate
if [[ "${{ needs.tests.result }}" != "success" ]]; then

The inline ${{ needs.tests.result }} expression is safe here — the value is always one of four fixed strings from GitHub infrastructure, not user input.

---

dev-single-node-setup leg: worker and bridge tests won't execute

Lines 89–91

- name: dev-single-node-setup
  cmd: cargo test -p rayls-infrastructure-types -p rayls-consensus-primary -p rayls-middleware-orchestrator --features dev-single-node-setup -- ...

The inline comment says "orchestrator's variant transitively enables it in primary/worker/bridge/types", which is correct for Cargo's feature graph during compilation. But worker and bridge are not listed with -p, so their tests are not collected or executed in this leg. The PR description only names four tests (all in types/primary/orchestrator), so this may already be the full picture — but if there are dev-single-node-setup-gated tests in worker or bridge, they'll be silently skipped. Worth a quick grep to confirm: grep -r 'cfg(feature = "dev-single-node-setup")' crates/*/worker crates/*/bridge. If nothing comes back, a short comment like # worker/bridge have no feature-gated tests removes the ambiguity.

---

Feature legs run with default features enabled

Lines 91, 96 — neither non-default leg passes --no-default-features:

cmd: cargo test ... --features dev-single-node-setup ...
cmd: cargo test ... --features archive-replay ...

Testing with defaults enabled is likely correct for dev-single-node-setup (test it as it ships). For archive-replay, which gates alternate reth_env/chainspec code paths, running without defaults would be a stronger compile-check of the isolated feature surface. Neither choice is wrong, but making it deliberate (even a brief comment like # intentionally WITH default features) helps future maintainers understand the choice. Suggest improvement →

---

Summary

The change is correct and fills a genuine CI gap. The tests-gate pattern for branch protection is the right approach for matrix jobs. Two actionable items before merging:

1. run: ${{ matrix.cmd }} — mitigate the forward-looking injection pattern with the env-var or decomposed-fields approach.
2. dev-single-node-setup scope — confirm worker/bridge have no feature-gated tests, or add them to the -p list.

The --no-default-features question and the cancelled-gate comment are minor and can be addressed in a follow-up if preferred. No blockers.