Review & the gauntlet

You’ve written a change. Now you have to land it — and a change that breaks the build or slips an unreviewed decision past a reviewer doesn’t land. This lesson is the gate every change passes through: the CI gauntlet, the source-level guards that keep retired code gone, and the checklist a contributor runs on their own diff before asking anyone to look.

Prerequisites

• Add a transform/operator

→

Leads to

• Plan a change without breaking a boundary

You’ll be able to: run the local mirror of CI before you push, explain why clippy runs twice on purpose, say what a retire-gate is and what it protects, and apply the engine’s review checklist to your own diff.

The gauntlet

CI is defined in one file. It runs five jobs in parallel: a check job on Linux, the full suite on Windows and macOS (three first-class targets), a cross-compile type-check, and a dependency-license audit. Only the check job has an internal order — the rest are single steps.

clinker ·ci.yml doc @47d2e12

jobs:
  check:                       # ubuntu-latest, toolchain "1.91"
    steps:
      - run: cargo fmt --all --check
      - run: cargo clippy --workspace -- -D warnings
      - run: cargo clippy --workspace --all-targets -- -D warnings
      - run: cargo test --workspace
      - run: cargo check --benches --workspace
      - run: cargo check --features bench-alloc -p clinker-benchmarks
      - run: cargo test --benches -p clinker-benchmarks
  test-windows:  # windows-latest → cargo test --workspace
  test-macos:    # macos-latest   → cargo test --workspace
  cross-platform:# ubuntu → cargo check --target {windows,apple} for the no-C crates
  deny:          # EmbarkStudios/cargo-deny-action@v2

The order inside check is the order to internalize, because it’s the cheapest-failure-first order: formatting (instant) → lint → test → bench gates. Windows and macOS run the full suite on native runners specifically because ring (TLS) needs a native C toolchain that the Linux cross-check can’t link — so the platform #[cfg(windows)] / #[cfg(macos)] tests genuinely execute on real runners, not just type-check.

Why clippy runs twice

The two clippy lines aren’t a copy-paste mistake. The CI file comments the reason verbatim, and it’s a clever interaction worth understanding:

clinker ·50_TESTING_AND_COMMANDS.md ·CI intentionally runs both clippy passes: doc @47d2e12

# Two clippy passes. The first omits --all-targets on purpose: with
# test targets excluded, a pub(crate) item referenced only from
# #[cfg(test)] code still trips the dead-code lint, so the dead-code
# gate keeps working. The second adds --all-targets to lint test,
# bench, and example code that the first pass never compiles.
- run: cargo clippy --workspace -- -D warnings
- run: cargo clippy --workspace --all-targets -- -D warnings

Read it slowly. If clippy only ran with --all-targets, then a pub(crate) function used only from tests would look “used” — and dead production code that happens to have a test would never be flagged. By running once without test targets, the first pass sees that function as dead (nothing in the library uses it) and fails. The second pass then lints the test/bench/example code the first pass skipped. The design rule states it plainly:

clinker ·30_DESIGN_RULES.md ·Keep dead-code pressure intact. doc @47d2e12

Keep dead-code pressure intact. CI intentionally runs cargo clippy --workspace -- -D warnings without --all-targets before the all-targets pass so test-only pub(crate) items still fail as dead code.

The local mirror of the whole gauntlet (run this before you push):

cargo fmt --all --check
cargo clippy --workspace --locked --offline -- -D warnings
cargo clippy --workspace --all-targets --locked --offline -- -D warnings
ulimit -n 4096 && cargo test --workspace --locked --offline

Retire-gates: keeping removed code gone

When the project retires something — an old module name, a withdrawn diagnostic code — deleting it isn’t enough; someone could reintroduce it later. Clinker installs retire-gates: tiny source-text tests that fail CI if a retired identifier reappears.

clinker-plan ·lib.rs ·rename_gates test @47d2e12

mod rename_gates {
    //! Source-text gates asserting retired identifiers stay gone: the old
    //! `cxl_compile` module name and the retired `E100` diagnostic code
    //! must not reappear in the config, error, or plan sources.

    #[test]
    fn e100_diagnostic_code_is_absent() {
        for (name, src) in [
            ("config/mod.rs", include_str!("config/mod.rs")),
            ("error.rs", include_str!("error.rs")),
        ] {
            assert!(
                !src.contains("\"E100\""),
                "found \"E100\" string literal in {name} — E100 should be fully retired"
            );
        }
    }
}

This is a great pattern to recognize: a test whose subject is the source text itself, via include_str!. It encodes a decision (“E100 is gone, don’t bring it back”) as an executable guard. The design rule it enforces: “Do not add compatibility shims around retired config shapes or old module names.” If your change needs to revive a retired name, that’s not a code change — it’s a decision to escalate (the subject of the next lesson).

The review checklist — applied to your own diff

Before a change is reviewed by anyone else, the contributor runs the project’s checklist on their own diff. The point is to catch the things CI can’t: scope creep, hidden decisions, weakened tests.

clinker ·REVIEW.md ·Review Checklist doc @47d2e12

The checklist’s load-bearing items (quoted):

• PR maps to exactly one issue or one coherent sub-issue. No unrelated cleanup.
• No hidden public API, schema, dependency, auth, security, memory, or architecture change.
• Tests were not weakened to pass. (The mirror of lesson 5.1: a green suite earned by deleting an assertion is worse than a red one.)
• Regression tests exist for bug fixes.
• Follow-up work is captured as issues, not hidden TODO drift.
• Verification commands were run or skipped with a reason.

Notice how much of this is about what didn’t happen: no hidden schema change, no weakened test, no silent scope expansion. That’s the reviewer’s real job — and the contributor’s, first.

// quick check

Why does CI run `cargo clippy --workspace -- -D warnings` (no --all-targets) before the --all-targets pass?

• It's redundant; the second pass covers everything the first does
• Without test targets, a pub(crate) item used only from #[cfg(test)] code is correctly seen as dead and fails — the first pass keeps the dead-code gate honest; the second pass then lints test/bench/example code
• The first pass checks formatting and the second checks lints

Ready to plan a change safely? Skip ahead →

Run the gauntlet

✓ Checkpoint — review & the gauntlet

💡 Hint 1

Run both clippy passes locally and notice they are not the same command. Then run the rename_gates tests and read what they assert — they include_str! real source files and check a string is absent.

💡 Hint 2

To see a retire-gate fire: temporarily add the literal "E100" to error.rs and run cargo test -p clinker-plan rename_gates. The named panic tells you exactly which guard you tripped. Revert with git restore.

What the tour establishes

CI is five parallel jobs; the check job runs fmt → clippy (twice) → test → bench gates, with Windows/macOS running the full suite natively. Clippy runs twice on purpose: the no---all-targets pass keeps the dead-code gate honest for test-only pub(crate) items, the --all-targets pass lints test/bench/example code. Retire-gates (rename_gates in clinker-plan/src/lib.rs) are include_str!-based source-text tests that fail CI if a retired identifier (cxl_compile, E100) reappears, enforcing the “no compatibility shims for retired shapes” rule. The review checklist (REVIEW.md) catches what CI can’t: scope creep, hidden API/schema/auth changes, and weakened tests.

// quick check

Your change makes a test fail. Which response is consistent with the review checklist?

• Delete or weaken the failing assertion so the suite is green again
• Fix the code so the assertion passes, or — if the behavior change is intended — update the test deliberately and explain it; never weaken a test merely to pass
• Mark the test #[ignore] and open the PR

You should be able to:

• You can run the local CI mirror (fmt, both clippy passes, tests) before pushing
• You can explain the dead-code rationale for the two clippy passes
• You can explain what a retire-gate guards and name the design rule behind it
• You can apply the review checklist's 'no hidden change / no weakened test' items to a diff

Verify in the checkout:

cargo fmt --all --check
cargo clippy --workspace --offline -- -D warnings && cargo clippy --workspace --all-targets --offline -- -D warnings
cargo test -p clinker-plan --offline rename_gates
grep -n 'Two clippy passes' .github/workflows/ci.yml

You can build a change and land it. The final lesson steps back: how to plan a change so that it respects the engine’s architectural boundaries — and how to recognize a decision you must escalate rather than make alone.

Mark this lesson complete