Review & the gauntlet

You’ve written a change. Now you have to land it. 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. This is a run-and-read lesson: you’ll run the real gauntlet commands and predict which gate catches which kind of mistake.

Prerequisites

• Add a transform/operator

→

Leads to

• Plan a change without breaking a boundary

What you’ll be able to do

• Run the local mirror of CI (fmt, both clippy passes, the test suite) before you push.
• Explain why clippy runs twice on purpose and what the no---all-targets pass protects.
• Name what a retire-gate is, what it guards, and the design rule behind it.
• Predict which CI gate fails for a given mistake (a format slip, a dead pub(crate) item, a revived retired identifier).
• Apply the engine’s review checklist to your own diff: the scope, hidden-change, and weakened-test items CI can’t catch.

New terms in this lesson (each is also expanded inline at the point you first need it):

• the gauntlet: the whole CI run a change must survive.
• CI gate: one pass/fail check that can block a merge.
• retire-gate: a test that keeps deleted code from creeping back.
• review checklist: what a contributor runs on their own diff before asking for review.

Predict first

✋ Predict first

You make three separate mistakes, each in its own commit: (1) you leave a line mis-indented; (2) you add a pub(crate) helper that's only ever called from a #[cfg(test)] test; (3) you reintroduce the retired "E100" diagnostic string into error.rs. The gauntlet runs four kinds of gate: fmt, clippy (two passes), the test suite, and a license audit. For each mistake, predict WHICH gate goes red first. Write your three answers before revealing.

Decide on your answer before you open the reveal — committing a guess is what makes it stick.

Reveal the answer

• (1) Mis-indented line → the fmt gate. cargo fmt --all --check is the first step in the check job precisely because it’s instant. It fails before clippy or the tests even start, so you fix formatting noise for free.
• (2) pub(crate) helper used only from a test → the first clippy pass. This is the subtle one. cargo clippy --workspace -- -D warnings (no --all-targets) does not compile the test targets, so nothing “uses” your helper. Clippy’s dead-code lint fires and -D warnings turns it into an error. The second, --all-targets pass would hide this, because it compiles the test and sees the call. That’s exactly why the first pass exists.
• (3) Revived "E100" string → the test gate. The retire-gate rename_gates is an ordinary #[test] that include_str!s error.rs and asserts "E100" is absent. It fails under cargo test, naming the guard you tripped.

The wrong mental model to drop: “clippy with --all-targets is strictly more thorough, so the first pass is redundant.” It is the opposite: the narrower first pass catches a class of dead code the broader pass conceals. More targets is not more dead-code pressure.

The gauntlet

CI is defined in one file. The gauntlet 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 @19acdcb4

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

Each step is one CI gate. 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 @19acdcb4

# 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 @19acdcb4

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.

Use → Modify → Create: drive the gauntlet

You learn the gauntlet by running it, then perturbing it, then predicting it from scratch. Each rung adds exactly one new demand.

Use: run the local mirror

The local mirror of the whole gauntlet reproduces the check job’s order on your machine. Run it before you push, and CI rarely surprises you:

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

The ulimit -n 4096 prefix is load-bearing on the test step (the default 1024-fd limit makes clinker-exec’s spill tests fail with “Too many open files,” the same prefix you met in the testing strategy lesson). A clean run prints, in order: no fmt diff, two clippy passes with 0 warnings, then the test result: ok summary. That clean transcript is the “all gates green” state every merge needs.

Modify: introduce one fault, predict the gate

Now perturb the codebase one fault at a time and predict which gate catches it before you re-run. Here are two perturbations grounded in the commands above:

# Perturbation A — drop a blank line or mis-indent a line in any committed .rs file.
#   Predict: which of the four steps fails, and is it before or after the tests run?

# Perturbation B — add this to a library module (NOT a test module):
#   pub(crate) fn only_used_by_a_test() -> i64 { 7 }
#   ...and call it solely from a #[cfg(test)] test in the same file.
#   Predict: does the FIRST clippy pass pass or fail? Does the SECOND?

💡 Hint 1

Perturbation A trips the very first step (cargo fmt --all --check). Fmt is the cheapest gate, so it fails before clippy or the tests run at all. Perturbation B is the dead-code case: the first, no---all-targets clippy pass sees the helper as unused and fails under -D warnings; the --all-targets pass compiles the test, sees the call, and would pass.

Show solution

A → cargo fmt --all --check, the first step, failing before tests. B → the first clippy pass fails, the second would pass. That is the entire reason both passes exist. Revert each with git restore once you’ve watched the gate fire. The lesson of B: a fault can be visible to a narrower check and invisible to a broader one, so the gauntlet runs both deliberately.

Create: a retire-gate, and predict it firing

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 @19acdcb4

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 pattern worth recognizing: 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 a decision to escalate, not a code change to make alone (the subject of the next lesson).

Your create-and-predict task: without running anything yet, predict the exact behavior of this one-line perturbation, then verify it.

# Add the literal string "E100" back into crates/clinker-plan/src/error.rs,
# then run: cargo test -p clinker-plan --offline rename_gates
# Predict — three parts:
#   1. Does this fail the fmt gate, a clippy gate, or the test gate?
#   2. What does the panic MESSAGE name?
#   3. Is reviving E100 a code fix, or a decision to escalate?

Show solution

1. The test gate. rename_gates is an ordinary #[test]; fmt and clippy don’t read string contents for retired identifiers, so neither would flag it. Only the source-text assertion does.
2. The panic prints found "E100" string literal in error.rs — E100 should be fully retired, naming the file and the retired identifier so you know exactly which guard you tripped.
3. It’s a decision to escalate. Reviving a retired name contradicts a recorded design rule; the right move is to raise it, not to quietly delete the gate. Revert with git restore.

The review checklist, applied to your own diff

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

clinker ·GITHUB_ISSUE_AGENT_WORKFLOW.md ·Review doc @19acdcb4

The load-bearing items the contributor self-checks:

• 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 the testing-strategy lesson: 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. CI proves the build is green; the checklist proves the diff is honest. A green gauntlet on a diff that quietly deleted an assertion is the most dangerous kind of green.

// 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 →

Retrieval checkpoint

✓ 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 — part of clinker’s GitHub-issue agent workflow — catches what CI can’t: scope creep, hidden API/schema/auth changes, and weakened tests.

// quick check

You add a pub(crate) helper and call it ONLY from a #[cfg(test)] test in the same file. Which gate goes red, and why?

• The --all-targets clippy pass, because it compiles the test and sees the helper is unused
• The first clippy pass (no --all-targets): it doesn't compile the test targets, so the helper looks dead and -D warnings turns the dead-code lint into an error
• The fmt gate, because new helpers must be formatted before they compile

// 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

// quick check

Spaced recall from the testing-strategy lesson: a golden-baseline test fails because your change altered one byte of a pipeline's output. You rerun with UPDATE_BASELINES=1 and the suite goes green. Why does the review checklist still flag this?

• It doesn't; regenerating the baseline is the correct fix and needs no review
• UPDATE_BASELINES=1 overwrites the committed golden with the new output, so the green is earned by accepting a diff, not by proving it's correct. That diff must be reviewed like any intentional change, never run reflexively to silence a red test
• UPDATE_BASELINES=1 deletes the baseline, so the test is skipped rather than passing

You should be able to:

• You can run the local CI mirror (fmt, both clippy passes, tests) before pushing
• You can predict which gate fails for a format slip, a dead pub(crate) item, and a revived retired identifier
• 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.

Want to build the engine's error type by hand: Display, source, and From? Skip ahead →

Mark this lesson complete