From 868ee9922f7f7f275880d987d663cd297ffc8ac6 Mon Sep 17 00:00:00 2001 From: Raphael Vigee Date: Mon, 31 Aug 2026 12:30:06 +0200 Subject: [PATCH 1/3] chore(orca): drop the default worktree tabs `defaultTabs` opened a `claude` tab, wrapped in `devenv shell`, in every new Orca worktree. It is gone; `scripts.setup` is untouched, so a new worktree still gets `gen` (which it cannot build without) and a `codegraph` index. What goes with it is the wrapping. Anything that opens a shell or an agent in a worktree now has to enter the devenv shell itself, and the failure mode when it does not is the quiet one rather than the loud one: missing `cargo`/`buf`/`gen`/`lint` fails immediately and visibly, but an ambient `rustc` with no `RUSTC_WRAPPER` builds perfectly well and simply bypasses kache. The header comment keeps the command and the reason so neither has to be rediscovered. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NQ8kRpuBjgxsSf15RjLDZz --- orca.yaml | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/orca.yaml b/orca.yaml index fe1005f01..6070badc2 100644 --- a/orca.yaml +++ b/orca.yaml @@ -5,24 +5,21 @@ # the team through git — the settings-pane equivalent is "local settings", # stored per-machine, so a teammate's worktrees would not get any of this. # -# Orca prompts to trust this file the first time it appears; neither the hook -# nor these tabs run until that is accepted. - -# Terminal tabs opened in every new worktree. +# Orca prompts to trust this file the first time it appears; the hook does not +# run until that is accepted. +# +# There are deliberately no `defaultTabs`. Whatever opens a shell or an agent +# in a new worktree has to enter the devenv shell itself — all development in +# this repo happens inside it (see CLAUDE.md), and a process started outside it +# gets a different, usually absent, toolchain: no `cargo`, no `buf`, no +# `gen`/`tst`/`lint`/`e2e` scripts, no `kache`. The failure worth knowing about +# is the quiet one — an ambient `rustc` with no `RUSTC_WRAPPER` builds fine and +# silently bypasses the build cache. So: # -# Wrapped in `devenv shell`, not bare `claude`: all development in this repo -# happens inside the devenv shell (see CLAUDE.md), and an agent started outside -# it gets a different — usually absent — toolchain. It would find no `cargo`, no -# `buf`, no `gen`/`tst`/`lint`/`e2e` scripts, and no `kache`, so builds would -# either fail outright or silently bypass the build cache by running an ambient -# rustc with no `RUSTC_WRAPPER`. Wrapping the agent is what makes "just run -# `lint`" work in a fresh worktree. +# devenv shell -- claude --permission-mode auto # # The `--` matters: it ends devenv's own option parsing, so `--permission-mode` # reaches claude instead of being eaten as a devenv flag. -defaultTabs: - - title: claude - command: devenv shell -- claude --permission-mode auto scripts: setup: | From 3f3da396ae04eb3112a79bf35bb7ba7b3b0e51fa Mon Sep 17 00:00:00 2001 From: Raphael Vigee Date: Wed, 2 Sep 2026 12:14:44 +0200 Subject: [PATCH 2/3] ci: skip stacked PRs unless they carry `ci/force-ci` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A PR stacked on another PR's branch is not the change that is about to land. Everything under it has to merge first, and `master` is squash-only, so the layer above always gets rebased once its base lands — that force-push is the run whose result anyone acts on. Building every layer on every push multiplied a deep stack's CI cost by its depth to produce verdicts about a tree nobody would ever merge. A new `gate` job decides once: a push, or a PR targeting the default branch, builds; anything else builds only with the `ci/force-ci` label. `gen`, `govet` and `abi` hang off it and every other job is transitively behind those, so a skipped PR costs one ~15s runner. The stack stays mergeable with no ruleset change. Required status checks live in the `master` ruleset, whose condition is `~DEFAULT_BRANCH`, so they apply to exactly the PRs the gate builds; GitHub asks nothing of a PR that does not target a protected branch. That argument has one seam, and `edited` in the trigger list is what closes it. A job skipped by an `if:` concludes `skipped`, which branch protection reads as a *pass* — so a child PR that became master-targeting without a new run would be mergeable carrying skipped checks over content nothing ever built. GitHub's automatic retarget is an `edited` event with `changes.base`, which triggers a run whose base is now master. `gh stack sync` (required anyway) is the second path. Neither is trusted alone. Cost of the two extra trigger types, taken deliberately rather than glossed: `labeled` and `edited` fire on things that are not code changes, and the gate cannot suppress those without concluding "skip" on a master-targeting PR, which would replace its green required checks with skipped ones. So a label or a body edit on a master-targeting PR re-runs CI, and with `cancel-in-progress` doing either mid-run restarts it. `gh stack submit` pushes and rewrites bodies seconds apart, so those collapse into one run. The gate writes its reason to stdout and to a `::notice` annotation, and deliberately not to the run-page step summary — that belongs to the `summary` job, which already reports the verdict there. The two surfaces it does write are the ones a non-browser reader can reach: `$GITHUB_STEP_SUMMARY` renders on the run page and nowhere else, so an agent reading `gh pr checks` — eleven `skipping` rows and exit 0 — could not reach it, while the annotation is in the check-run API. `summary` branches on `needs.gate.result` before its outputs, so a cancelled gate is not reported as "this PR is stacked". `tests/ci_gate.rs` freezes the structure rather than today's job list: every job must be reachable from `gate`, a job that needs `gate` must also read its verdict, `labeled`/`edited` must stay in the trigger types, the gate must emit an annotation, `summary` must consult `gate.result`, and no `branches:` filter may return to `pull_request:` (that one skips at the workflow level, which publishes no checks at all — an empty list that reads like a pass, #240). Each guard was mutation-tested. Reviewed by `product-vision`; two of its recommendations are left to the user rather than taken here. The label name `ci/force-ci` is the user's, and disagrees with the incumbent bare `perf-test`. And full-skip was chosen over a reduced stacked-PR matrix (lint + test on linux/amd64, ~1/3 the spend, real green checks and pre-merge platform signal): a break that only shows on darwin/arm64 now surfaces only once the layers below land. That per-target trade is recorded in CLAUDE.md next to the escape hatch. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NQ8kRpuBjgxsSf15RjLDZz --- .github/workflows/heph.yml | 184 ++++++++++++++++-- CLAUDE.md | 5 +- tests/ci_gate.rs | 385 +++++++++++++++++++++++++++++++++++++ 3 files changed, 556 insertions(+), 18 deletions(-) create mode 100644 tests/ci_gate.rs diff --git a/.github/workflows/heph.yml b/.github/workflows/heph.yml index b19b3c892..df76cfd91 100644 --- a/.github/workflows/heph.yml +++ b/.github/workflows/heph.yml @@ -3,17 +3,41 @@ name: CI on: push: branches: ["master"] - # Every pull request, whatever it targets. Deliberately unfiltered: a - # `branches:` filter here matches the PR's *base*, so restricting it to - # master silently gives zero CI to any PR stacked on another branch — no - # runs queued, no failed checks, nothing to notice. The upper layers of a - # stack are exactly the changes that most want testing before the layer - # below them merges. + # Every pull request, whatever it targets. Deliberately unfiltered *here*: a + # `branches:` filter matches the PR's *base*, and a workflow that never + # triggers publishes no checks at all — a stacked PR would show an empty + # check list, which reads like a pass and cannot be overridden with a label. + # The stacked-PR decision is made by the `gate` job below instead, where it + # is visible, explainable, and opt-outable. + # + # Naming `types:` replaces the default set, so the three default events are + # spelled out and two more are added. Both additions exist for the gate: + # + # - `labeled`, so adding `ci/force-ci` starts a run by itself rather than + # needing a push to notice the label. (It also makes `perf-test` + # self-triggering, which it previously was not.) + # - `edited`, so the base-branch change GitHub performs *itself* when a + # stack's base merges — retargeting the child at master — starts a run. + # Nothing else reports that: a retarget fires no `synchronize`, so + # without this the child would sit at `master` still carrying the + # `skipped` checks from its stacked days, which branch protection reads + # as success. That is code reaching master with nothing ever built. + # + # Both fire on things that are not code changes — any label, any title or + # body edit — and the gate cannot suppress those without concluding "skip" + # on a master-targeting PR, which would replace its green required checks + # with skipped ones. Re-running is the safe direction, so the cost is real + # and taken deliberately: **a label or a body edit on a master-targeting PR + # runs CI again, and because this workflow cancels in-progress PR runs, doing + # either mid-run kills that run and restarts it from zero.** In practice the + # dominant case is `gh stack submit`, which pushes and rewrites PR bodies + # seconds apart — same concurrency group, so those collapse into one run. # # The ABI guard diffs against `github.base_ref` rather than a hardcoded - # master, so it keeps working here and reports the ABI delta for that layer - # alone. + # master, so it keeps working on a stacked PR and reports the ABI delta for + # that layer alone. pull_request: + types: [opened, synchronize, reopened, labeled, edited] # PR: cancel superseded runs so a new push doesn't leave the old run burning # runners (and showing up as a noisy "cancelled"). master: every commit gets @@ -42,10 +66,97 @@ env: CARGO_INCREMENTAL: "0" jobs: + # Which pull requests get a build. + # + # A stacked PR — one whose base is another PR's branch rather than master — + # is not the change that is about to land. Everything under it has to merge + # first, and `master` is squash-only, so the layer above always gets rebased + # (`gh stack sync`) once its base lands; that force-push is a `synchronize` + # event on a PR whose base is now master, and *that* run is the one whose + # result anyone acts on. Building every layer on every push multiplied a + # deep stack's CI cost by its depth to produce results that were, at best, + # about a tree nobody would ever merge. + # + # So: a push, or a PR targeting the default branch, builds. Anything else + # builds only when the PR carries the `ci/force-ci` label — for the times a + # mid-stack change is worth testing on its own (a big refactor low in the + # stack, a platform-specific change, a flake hunt). + # + # This does not let untested code reach master. Required status checks live + # in the `master` repository ruleset, whose condition is `~DEFAULT_BRANCH` — + # it applies to exactly the PRs whose base is master, which is exactly the + # set this gate lets through. A stacked PR merges with skipped checks + # because GitHub asks nothing of a PR that does not target a protected + # branch. The stack stays mergeable bottom-up with no manual override and no + # ruleset change. + # + # That argument has one seam, and it is the reason `edited` is in the + # trigger list above. A skipped job's check run concludes `skipped`, which + # branch protection treats as a pass — so a child PR that *became* + # master-targeting without a new run would be mergeable carrying skipped + # checks over content nothing ever built. Two independent things close it: + # GitHub's own retarget is an `edited` event with `changes.base`, which + # triggers a run whose base is now master; and `gh stack sync` — required + # anyway, since `master` is squash-only and the child still carries its + # base's commits — force-pushes, which is a `synchronize`. Either one alone + # is sufficient; neither is trusted alone. + gate: + name: Gate + runs-on: ubuntu-latest + timeout-minutes: 5 + outputs: + run: ${{ steps.decide.outputs.run }} + reason: ${{ steps.decide.outputs.reason }} + steps: + - name: Decide + id: decide + env: + EVENT: ${{ github.event_name }} + BASE: ${{ github.event.pull_request.base.ref }} + DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} + FORCED: ${{ contains(github.event.pull_request.labels.*.name, 'ci/force-ci') }} + run: | + set -euo pipefail + if [ "$EVENT" != "pull_request" ]; then + run=true + why="event is '$EVENT', not a pull request." + elif [ "$BASE" = "$DEFAULT_BRANCH" ]; then + run=true + why="this PR targets '$DEFAULT_BRANCH'." + elif [ "$FORCED" = "true" ]; then + run=true + why="this PR carries the 'ci/force-ci' label." + else + run=false + why="this PR targets '$BASE', not '$DEFAULT_BRANCH' — it is stacked on another branch, so it cannot merge until the layer below it does, and it will get a full run as soon as it is rebased onto '$DEFAULT_BRANCH'. Add the 'ci/force-ci' label to build it anyway; 'perf-test' alone will not, because perfbench needs the build this gate skipped." + fi + echo "run=$run" >> "$GITHUB_OUTPUT" + echo "reason=$why" >> "$GITHUB_OUTPUT" + + # Stdout and an annotation, deliberately not a step summary. The + # run-page summary belongs to the `summary` job, which already + # reports the verdict there and would otherwise be preceded by a + # near-duplicate of itself. These two surfaces are the ones a + # non-browser reader can reach: stdout lands in `gh run view --log`, + # and the annotation is in the check-run API, so it survives into + # `gh run view` and the PR's checks page. That matters because + # someone — or something — reading `gh pr checks` sees eleven + # `skipping` rows and an exit code of 0, and needs the reason to be + # reachable without opening a tab. + if [ "$run" = "true" ]; then + echo "Running CI: $why" + echo "::notice title=CI running::$why" + else + echo "Skipping CI: $why" + echo "::notice title=CI skipped::$why" + fi + gen: name: Codegen runs-on: ubuntu-latest timeout-minutes: 10 + needs: gate + if: needs.gate.outputs.run == 'true' outputs: version: ${{ steps.version.outputs.version }} repo_artifact_id: ${{ steps.upload_repo.outputs.artifact-id }} @@ -91,12 +202,15 @@ jobs: # into the plugin (`HEPH_GOVET_SHA256_*`), which then verifies the asset it # downloads — no checksum for a user to configure, and a tampered asset fails # the build closed. That baking is the only reason `build` needs this job; it - # deliberately depends on nothing itself (no codegen, no devenv — just the Go - # sources), so it runs concurrently with `gen` and adds no critical path. + # needs no codegen and no devenv of its own (just the Go sources), so it runs + # concurrently with `gen` and adds no critical path. `gate` is the sole + # exception, and it is seconds. govet: name: Build heph-govet runs-on: ubuntu-latest timeout-minutes: 15 + needs: gate + if: needs.gate.outputs.run == 'true' outputs: sha256_linux_amd64: ${{ steps.build.outputs.sha256_linux_amd64 }} sha256_linux_arm64: ${{ steps.build.outputs.sha256_linux_arm64 }} @@ -631,8 +745,11 @@ jobs: abi: name: ABI guard # PR-only: it diffs the branch against the PR base. Pure git + bash, so it - # skips the devenv toolchain and runs in seconds. - if: github.event_name == 'pull_request' + # skips the devenv toolchain and runs in seconds — but it still reports on + # a layer nobody can merge yet, so it rides the same gate as everything + # else rather than being a lone check on a skipped stacked PR. + needs: gate + if: github.event_name == 'pull_request' && needs.gate.outputs.run == 'true' runs-on: ubuntu-latest timeout-minutes: 5 steps: @@ -1120,10 +1237,12 @@ jobs: # thresholds are still being proven out; an unlabeled PR skips this job # entirely rather than running it just to fail on the missing label — # cheaper, and the `if:` below is the single source of truth for - # whether perf runs at all. Adding the label to an already-open PR does - # NOT retrigger this job on its own (`pull_request` only fires on - # `opened`/`synchronize`/`reopened`, not `labeled`) — push a commit or - # re-run the job after labeling. + # whether perf runs at all. Labelling an already-open PR does start a + # run on its own — `labeled` is in this workflow's `pull_request` types + # (added for the gate's `ci/force-ci`), so no push or manual re-run is + # needed. On a *stacked* PR it still gets nothing: this job needs + # `build`, which the gate skipped, so `perf-test` there wants + # `ci/force-ci` alongside it. # - `push` to `master`: nothing is blocked at this point (the merge # already happened) — this is continuous monitoring, and a regression # instead files a GitHub issue. @@ -1184,15 +1303,46 @@ jobs: }, }); + # `always()`, so this is the one job that still speaks when the gate skipped + # everything — a run whose every check is "skipped" and whose summary is + # empty is indistinguishable from a broken workflow. It restates the gate's + # reason instead. summary: name: Summary - needs: [gen, upload_artifacts] + needs: [gate, gen, upload_artifacts] if: always() runs-on: ubuntu-latest timeout-minutes: 5 steps: - name: Generate summary + # The gate's reason carries a branch name, so it arrives through `env:` + # rather than a `${{ }}` spliced into the script. Ref names may contain + # `$`, `"` and backticks, and this one is only trusted because a PR's + # base is always a branch of *this* repo — one `env:` is cheaper than + # relying on that staying true. + env: + GATE_RUN: ${{ needs.gate.outputs.run }} + GATE_REASON: ${{ needs.gate.outputs.reason }} + GATE_RESULT: ${{ needs.gate.result }} run: | + # `needs.gate.result` first, and only then its outputs. `always()` + # means this job also runs when `gate` was cancelled or failed, and + # a cancelled gate has empty outputs — so branching on `run` alone + # would explain a broken workflow as "this PR is stacked", which is + # a confident, wrong, and unfalsifiable answer from the one job that + # exists to answer the question. + if [ "$GATE_RESULT" != "success" ]; then + echo "# Build Summary" >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY + echo "**CI did not run** — the \`Gate\` job concluded \`$GATE_RESULT\`, so nothing downstream of it started. This is not the stacked-PR skip; see the \`Gate\` job." >> $GITHUB_STEP_SUMMARY + exit 0 + fi + if [ "$GATE_RUN" != "true" ]; then + echo "# Build Summary" >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY + echo "**CI skipped** — $GATE_REASON" >> $GITHUB_STEP_SUMMARY + exit 0 + fi echo "# Build Summary" >> $GITHUB_STEP_SUMMARY echo "" >> $GITHUB_STEP_SUMMARY echo "**Version:** \`${{needs.gen.outputs.version}}\`" >> $GITHUB_STEP_SUMMARY diff --git a/CLAUDE.md b/CLAUDE.md index 0b21f3dbb..9134764ce 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -125,7 +125,10 @@ Plain `gh stack submit` opens an editor for PR titles — pass `--auto` from a s - **A red check on a stacked PR is not necessarily its own.** Before debugging, check the base: `gh pr checks `. Same job red there → not your bug; say so on your PR and fix it in the base, not in yours. This has already cost real time — a stacked PR reddened on a flake inherited from its base, and the fix for it lived in a third PR entirely. - **Don't fold a fix for the base into your stack.** It muddies the revert line — the fix disappears if your PR is reverted, and it lands bundled with an unrelated change. Fix the base in the base, or in its own PR. - **After resolving a stack conflict, diff against the lower branch and re-run the *lower* PR's tests.** For each conflicted file, `git diff -- ` and confirm every remaining difference is deliberately yours. A resolution can compile, pass your tests, and still revert the change below you: git applies an upper-PR copy of a moved code block cleanly *above* the conflict region and marks only the code below, so taking the upper side verbatim silently dropped a lower PR's `sort`/`dedup` and put a `HashSet` seed back into a def hash. -- **Stacked PRs do get CI.** `pull_request:` in `.github/workflows/heph.yml` is deliberately unfiltered — a `branches: ["master"]` filter matches the PR's *base*, so stacked PRs got zero runs and an empty check list that reads like a pass (fixed in #240). Don't re-add the filter. +- **A stacked PR does not build until it reaches the bottom.** The `gate` job in `.github/workflows/heph.yml` runs CI for a push, or for a PR whose base is `master`; a PR based on another branch is skipped — every check reports "skipped" and the `Summary` job says why. Add the **`ci/force-ci`** label to build one anyway (labelling starts the run on its own; no push needed). This does not weaken the merge gate: required status checks live in the `master` ruleset, whose condition is `~DEFAULT_BRANCH`, so they apply to exactly the PRs the gate builds. A stacked PR merges with skipped checks because nothing is required of it, and its code still cannot reach `master` without first becoming a `master`-targeting PR, which builds. +- **What the skip costs you: an upper layer's break surfaces late.** A change that only fails on `darwin/arm64`, or only under `--no-default-features`, sits undetected at layer 3 until the two below it land. On a deep stack that serializes debugging into one cycle per layer. `ci/force-ci` is the answer when a layer is worth testing on its own — a large refactor low in the stack, anything platform-specific, a flake hunt. Use it rather than assuming green-when-it-gets-there. +- **Sync after the base merges.** GitHub retargets the child at `master` itself, and that retarget *is* an `edited` event, so it now starts a full run — but the tree it builds still carries the base's commits. `gh stack sync` force-pushes the rebased branch (`synchronize` → another run), and that is the run whose result means anything. You need the rebase regardless, since `master` is squash-only. +- **Don't add a `branches:` filter to `pull_request:`.** It matches the PR's *base*, so stacked PRs got zero runs and an empty check list that reads like a pass (fixed in #240) — invisible, and not overridable by a label. That is why the skip lives in `gate` instead. `tests/ci_gate.rs` guards this, plus the rule that every job hangs off `gate`. ## Review Board diff --git a/tests/ci_gate.rs b/tests/ci_gate.rs new file mode 100644 index 000000000..33206c8f8 --- /dev/null +++ b/tests/ci_gate.rs @@ -0,0 +1,385 @@ +//! Guards on the CI gate — which pull requests get a build at all. +//! +//! `heph.yml` deliberately does not build a stacked PR (one whose base is +//! another PR's branch rather than the default branch) unless it carries the +//! `ci/force-ci` label. That saves a deep stack's CI cost times its depth, and +//! it is safe for one reason and one reason only: the `master` repository +//! ruleset — where the required status checks live — has condition +//! `~DEFAULT_BRANCH`, so it applies to exactly the PRs whose base is master, +//! which is exactly the set the gate lets through. Nothing reaches master +//! without first being a PR that targets master, and those always build. +//! +//! Every assertion below protects a way that reasoning can be broken silently, +//! in the direction where nothing goes red: +//! +//! - a new job added with no `needs:` builds on every stacked PR forever, +//! and no check ever complains; +//! - dropping `labeled` from the trigger list leaves `ci/force-ci` inert — +//! the label is accepted, and simply never starts a run; +//! - dropping `edited` reopens the one seam in the argument above. A job +//! skipped by an `if:` concludes `skipped`, which branch protection reads +//! as a pass. GitHub retargets a child PR at master by itself when the +//! base merges, and that retarget fires no `synchronize` — so without +//! `edited` the child sits at `master` carrying the skipped checks from +//! its stacked days, mergeable over content nothing ever built; +//! - a `branches:` filter on `pull_request:` would skip stacked PRs at the +//! *workflow* level, which publishes no checks at all rather than skipped +//! ones. An empty check list reads like a pass and no label can override +//! it. That has already shipped once (#240). +//! +//! What this cannot cover: whether the ruleset still targets only the default +//! branch. That lives in repo settings, outside this tree. If required checks +//! are ever extended to non-default branches, stacked PRs become unmergeable +//! and the gate has to go with it. +//! +//! Cheap on purpose: one file read and some string work, no build. + +use std::collections::{HashMap, HashSet}; +use std::path::Path; + +fn workflow() -> String { + let path = Path::new(env!("CARGO_MANIFEST_DIR")).join(".github/workflows/heph.yml"); + std::fs::read_to_string(&path).expect("read .github/workflows/heph.yml") +} + +/// `(job id, job body)` for every top-level job, in file order. +/// +/// A job's *leading* comment block lands in the body of the job above it, so +/// content assertions run over [`without_comments`]. +fn workflow_jobs(workflow: &str) -> Vec<(&str, String)> { + let (_, body) = workflow + .split_once("\njobs:\n") + .expect("the workflow has a top-level `jobs:` key"); + + let mut jobs: Vec<(&str, Vec<&str>)> = Vec::new(); + for line in body.lines() { + let header = line + .strip_prefix(" ") + .filter(|rest| !rest.starts_with([' ', '#', '-'])) + .and_then(|rest| rest.strip_suffix(':')) + .filter(|id| { + !id.is_empty() + && id + .chars() + .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '_') + }); + match header { + Some(id) => jobs.push((id, Vec::new())), + None => { + if let Some((_, lines)) = jobs.last_mut() { + lines.push(line); + } + } + } + } + jobs.into_iter() + .map(|(id, lines)| (id, lines.join("\n"))) + .collect() +} + +/// The `needs:` of a job body, tokenised. +/// +/// Tokenised rather than substring-matched: one job id can be a prefix of +/// another, and a `contains` check would report the short one present when +/// only the long one is. +/// +/// Only the inline `[a, b]` form is understood. A `needs:` written as a YAML +/// block list parses as empty, which reads as an orphan and fails +/// `every_job_is_behind_the_gate` — noisy, but closed. Leave it that way: the +/// alternative is a parser that can silently mis-attribute a dependency. +fn job_needs(body: &str) -> Vec<&str> { + body.lines() + .map(str::trim) + .find(|line| line.starts_with("needs:")) + .map(|line| { + line.strip_prefix("needs:") + .unwrap_or(line) + .trim() + .trim_matches(['[', ']']) + .split(',') + .map(str::trim) + .filter(|entry| !entry.is_empty()) + .collect() + }) + .unwrap_or_default() +} + +/// `text` with whole-line YAML comments removed. +/// +/// A content assertion must not be satisfiable by prose — least of all here, +/// where the surrounding comments spell out the very strings being asserted. +fn without_comments(text: &str) -> String { + text.lines() + .filter(|line| !line.trim_start().starts_with('#')) + .collect::>() + .join("\n") +} + +/// The `on:` block — everything from the top-level `on:` key to the next +/// top-level key. +fn trigger_block(workflow: &str) -> String { + let (_, rest) = workflow + .split_once("\non:\n") + .expect("the workflow has a top-level `on:` key"); + rest.lines() + .take_while(|line| line.is_empty() || line.starts_with([' ', '#'])) + .collect::>() + .join("\n") +} + +/// The `pull_request:` sub-block of `on:`. +fn pull_request_trigger(workflow: &str) -> String { + let block = trigger_block(workflow); + let (_, rest) = block + .split_once("\n pull_request:") + .expect("the workflow triggers on `pull_request`"); + rest.lines() + .take_while(|line| line.is_empty() || line.starts_with(" ") || line.starts_with(" #")) + .collect::>() + .join("\n") +} + +/// Every job must be reachable from `gate` through `needs:`. +/// +/// This is the whole safety property, stated once. A job that is not +/// downstream of `gate` runs on every stacked PR — burning three platforms' +/// worth of runners on a tree nobody can merge — and there is no check +/// anywhere that turns red to say so. The failure mode of the *feature* is +/// silent excess, so the guard has to be structural rather than a spot check +/// on the jobs that happen to exist today. +#[test] +fn every_job_is_behind_the_gate() { + let workflow = workflow(); + let jobs = workflow_jobs(&workflow); + assert!( + jobs.iter().any(|(id, _)| *id == "gate"), + "`heph.yml` has no `gate` job. Every other assertion here is about \ + what hangs off it." + ); + + let needs: HashMap<&str, Vec<&str>> = jobs + .iter() + .map(|(id, body)| (*id, job_needs(body))) + .collect(); + + let mut reaches_gate: HashSet<&str> = HashSet::new(); + // Fixed point rather than recursion: `needs:` is a DAG, so repeating the + // sweep until it stops growing settles it in at most `jobs.len()` passes. + loop { + let before = reaches_gate.len(); + for (id, deps) in &needs { + if deps + .iter() + .any(|dep| *dep == "gate" || reaches_gate.contains(dep)) + { + reaches_gate.insert(id); + } + } + if reaches_gate.len() == before { + break; + } + } + + let orphans: Vec<&str> = jobs + .iter() + .map(|(id, _)| *id) + .filter(|id| *id != "gate" && !reaches_gate.contains(id)) + .collect(); + assert!( + orphans.is_empty(), + "these jobs are not downstream of `gate`, so they run even on a \ + stacked PR that CI is supposed to skip entirely: {orphans:?}. Give \ + each one `needs: gate` (plus `if: needs.gate.outputs.run == 'true'` \ + if it is a root job), or make it depend on something that already is." + ); +} + +/// The two root jobs must consult the gate's verdict, not merely wait for it. +/// +/// `needs: gate` alone is worse than nothing: `gate` always succeeds, so a +/// root job that only depends on it runs unconditionally while *looking* +/// gated — and `every_job_is_behind_the_gate` above would still pass. +/// +/// Scoped to jobs whose `needs` is *exactly* `[gate]`, which is what a root +/// job looks like. A job with `needs: [gate, x]` is exempt on purpose: `x` is +/// itself downstream of the gate, so the job is already gated transitively and +/// an `if:` would be redundant. The exemption is a consequence of the +/// reachability rule above, not a hole in it. +#[test] +fn the_root_jobs_check_the_gates_verdict() { + let workflow = workflow(); + for (id, body) in workflow_jobs(&workflow) { + if job_needs(&body) != ["gate"] { + continue; + } + let body = without_comments(&body); + assert!( + body.contains("needs.gate.outputs.run == 'true'"), + "the `{id}` job depends on `gate` but never reads \ + `needs.gate.outputs.run`. `gate` always succeeds, so `{id}` runs \ + on every stacked PR while looking gated." + ); + } +} + +/// The gate decides on the PR's base and the `ci/force-ci` label — both. +/// +/// Drop the base comparison and every PR builds; drop the label and the escape +/// hatch is gone, with nothing to signal it but a stacked PR that stays +/// stubbornly unbuilt. +#[test] +fn the_gate_decides_on_the_base_branch_and_the_force_label() { + let workflow = workflow(); + let (_, body) = workflow_jobs(&workflow) + .into_iter() + .find(|(id, _)| *id == "gate") + .expect("`heph.yml` has a `gate` job"); + let body = without_comments(&body); + + assert!( + body.contains("default_branch"), + "the `gate` job never mentions `default_branch`. Without comparing the \ + PR's base to it, the gate cannot tell a stacked PR from the one that \ + is about to land." + ); + assert!( + body.contains("github.event.pull_request.base.ref"), + "the `gate` job never reads the PR's base ref, so its base comparison \ + cannot be against the PR's actual base." + ); + assert!( + body.contains("ci/force-ci"), + "the `gate` job never mentions `ci/force-ci`, so there is no way to \ + build a mid-stack PR on demand." + ); + assert!( + body.contains("$GITHUB_OUTPUT"), + "the `gate` job publishes no output, so `needs.gate.outputs.run` is \ + empty everywhere and every gated job is skipped — including on \ + master." + ); +} + +/// Labelling a PR, and retargeting one, must each be able to start a run. +/// +/// `labeled`: without it, labelling a stacked PR does nothing at all — the +/// label sticks, no run is queued, and the only way to build the PR is to push +/// to it, which is precisely what someone reaching for the label is avoiding. +/// +/// `edited`: without it, GitHub's automatic retarget of a child PR onto master +/// (when the base merges) queues nothing, and the child keeps the `skipped` +/// checks it earned while stacked. Branch protection reads `skipped` as a +/// pass, so the child becomes mergeable into master over content that was +/// never built. This is the single assertion the gate's safety argument rests +/// on that is not enforced anywhere else in the tree. +#[test] +fn a_label_or_a_retarget_can_start_a_run() { + let workflow = workflow(); + let trigger = without_comments(&pull_request_trigger(&workflow)); + let types = trigger + .lines() + .map(str::trim) + .find(|line| line.starts_with("types:")) + .unwrap_or_else(|| { + panic!( + "`on.pull_request` declares no `types:`, so it uses the default \ + (opened/synchronize/reopened) and the `ci/force-ci` label \ + cannot start a run" + ) + }); + assert!( + types.contains("labeled"), + "`on.pull_request.types` omits `labeled`, so adding `ci/force-ci` to a \ + stacked PR queues nothing. Found: {types}" + ); + assert!( + types.contains("edited"), + "`on.pull_request.types` omits `edited`, so GitHub's automatic \ + retarget of a stacked PR onto master queues no run and the PR stays \ + mergeable on its stale `skipped` checks. Found: {types}" + ); + for required in ["opened", "synchronize", "reopened"] { + assert!( + types.contains(required), + "`on.pull_request.types` omits `{required}`. Naming any `types:` \ + replaces the default set, so the omitted event now gets no CI at \ + all. Found: {types}" + ); + } +} + +/// The gate's reason has to be readable without a browser. +/// +/// The `summary` job reports the verdict to `$GITHUB_STEP_SUMMARY`, which +/// renders on the run page and nowhere else — it is not in +/// `gh run view --log`, not in `gh pr checks --json`. An agent staring at +/// eleven `skipping` rows and an exit code of 0 has no way to reach it, so the +/// gate carries the reason on two surfaces that do survive into `gh`: stdout, +/// and a `::notice` annotation in the check-run API. The annotation is the one +/// asserted here — stdout is a substring of it either way, and the annotation +/// is the surface reachable without resolving a run id. +#[test] +fn the_gates_reason_reaches_a_cli_reader() { + let workflow = workflow(); + let (_, body) = workflow_jobs(&workflow) + .into_iter() + .find(|(id, _)| *id == "gate") + .expect("`heph.yml` has a `gate` job"); + let body = without_comments(&body); + + assert!( + body.contains("::notice title=CI skipped::"), + "the `gate` job emits no annotation when it skips. The run-page step \ + summary is invisible to `gh`, so the reason has to reach the \ + check-run API too." + ); +} + +/// `summary` must distinguish "the gate skipped" from "the gate broke". +/// +/// It is `always()`, so it also runs when `gate` was cancelled (the PR +/// concurrency group cancels superseded runs constantly) or failed outright. +/// A cancelled gate has empty outputs, so a summary that branches on +/// `outputs.run` alone explains a broken workflow as "this PR is stacked" — +/// a confident wrong answer from the one job whose entire purpose is to +/// answer that question. +#[test] +fn the_summary_tells_a_skip_apart_from_a_broken_gate() { + let workflow = workflow(); + let (_, body) = workflow_jobs(&workflow) + .into_iter() + .find(|(id, _)| *id == "summary") + .expect("`heph.yml` has a `summary` job"); + let body = without_comments(&body); + + assert!( + body.contains("needs.gate.result"), + "the `summary` job never reads `needs.gate.result`, so it cannot tell \ + a deliberate stacked-PR skip from a gate that was cancelled or \ + failed — and it will report the former for both." + ); +} + +/// The stacked-PR decision belongs to `gate`, never to a `branches:` filter. +/// +/// A `branches:` filter on `pull_request:` matches the PR's *base*, so +/// restricting it to master skips stacked PRs at the workflow level. That does +/// not produce skipped checks — it produces *no* checks, an empty list that +/// reads like a pass, that `ci/force-ci` cannot override, and that nothing in +/// the run explains. Fixed once already in #240; the gate is the supported way +/// to say the same thing. +#[test] +fn the_pull_request_trigger_is_unfiltered() { + let workflow = workflow(); + let trigger = without_comments(&pull_request_trigger(&workflow)); + assert!( + !trigger + .lines() + .map(str::trim) + .any(|line| line.starts_with("branches:") || line.starts_with("branches-ignore:")), + "`on.pull_request` carries a branch filter. It matches the PR's base, \ + so a stacked PR gets no workflow run and therefore no checks at all — \ + an empty check list, not a skipped one, with no way to override it. \ + Skip stacked PRs in the `gate` job instead:\n{trigger}" + ); +} From 7cc1872dc78b42923c435f5af47136749dff8e52 Mon Sep 17 00:00:00 2001 From: Raphael Vigee Date: Wed, 2 Sep 2026 12:14:44 +0200 Subject: [PATCH 3/3] ci: namespace the perfbench opt-in label as `ci/perf-test` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Renamed in place on the repo (`gh label edit`), so it keeps its id and stays applied to the PRs already carrying it — #435 and #437 both still have it. With `ci/force-ci` from the commit below, both CI labels now share one namespace; a lone `pr/` prefix on one of them was a typo waiting to happen. The functional reference is `perfbench`'s `if:`; the rest are comments here and in `perf.yml`. Not touched: the `perf-test` *skill* in `.claude/skills/` and the references to it in `.claude/agents/` and `.cargo/config.toml`. That is samply profiling, unrelated to the label beyond sharing a name. One transition window worth knowing about. A `pull_request` run resolves its workflow from the merge of head into base, so an open PR carrying the label gets the renamed `if:` as soon as this lands on master, with no rebase needed. Before it lands, those PRs hold a label the workflow on their own base does not match — perfbench will not fire for them in that window. It is an opt-in check on two PRs, so the cost is a re-label or a rebase, not a lost signal. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NQ8kRpuBjgxsSf15RjLDZz --- .github/workflows/heph.yml | 10 +++++----- .github/workflows/perf.yml | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/heph.yml b/.github/workflows/heph.yml index df76cfd91..ab99906df 100644 --- a/.github/workflows/heph.yml +++ b/.github/workflows/heph.yml @@ -14,7 +14,7 @@ on: # spelled out and two more are added. Both additions exist for the gate: # # - `labeled`, so adding `ci/force-ci` starts a run by itself rather than - # needing a push to notice the label. (It also makes `perf-test` + # needing a push to notice the label. (It also makes `ci/perf-test` # self-triggering, which it previously was not.) # - `edited`, so the base-branch change GitHub performs *itself* when a # stack's base merges — retargeting the child at master — starts a run. @@ -128,7 +128,7 @@ jobs: why="this PR carries the 'ci/force-ci' label." else run=false - why="this PR targets '$BASE', not '$DEFAULT_BRANCH' — it is stacked on another branch, so it cannot merge until the layer below it does, and it will get a full run as soon as it is rebased onto '$DEFAULT_BRANCH'. Add the 'ci/force-ci' label to build it anyway; 'perf-test' alone will not, because perfbench needs the build this gate skipped." + why="this PR targets '$BASE', not '$DEFAULT_BRANCH' — it is stacked on another branch, so it cannot merge until the layer below it does, and it will get a full run as soon as it is rebased onto '$DEFAULT_BRANCH'. Add the 'ci/force-ci' label to build it anyway; 'ci/perf-test' alone will not, because perfbench needs the build this gate skipped." fi echo "run=$run" >> "$GITHUB_OUTPUT" echo "reason=$why" >> "$GITHUB_OUTPUT" @@ -1232,7 +1232,7 @@ jobs: # Perf-regression check (own workflow: .github/workflows/perf.yml) — # generates one synthetic corpus, times a fixed scenario set against N and # N-1, and reports a regression verdict. Two triggers, two roles: - # - `pull_request`, labeled `perf-test`: this is the GATE — a real + # - `pull_request`, labeled `ci/perf-test`: this is the GATE — a real # regression fails the check for real. Opt-in while the harness and its # thresholds are still being proven out; an unlabeled PR skips this job # entirely rather than running it just to fail on the missing label — @@ -1241,7 +1241,7 @@ jobs: # run on its own — `labeled` is in this workflow's `pull_request` types # (added for the gate's `ci/force-ci`), so no push or manual re-run is # needed. On a *stacked* PR it still gets nothing: this job needs - # `build`, which the gate skipped, so `perf-test` there wants + # `build`, which the gate skipped, so `ci/perf-test` there wants # `ci/force-ci` alongside it. # - `push` to `master`: nothing is blocked at this point (the merge # already happened) — this is continuous monitoring, and a regression @@ -1258,7 +1258,7 @@ jobs: perfbench: name: Perf if: | - (github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'perf-test')) || + (github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'ci/perf-test')) || (github.event_name == 'push' && github.ref == 'refs/heads/master') needs: [build] uses: ./.github/workflows/perf.yml diff --git a/.github/workflows/perf.yml b/.github/workflows/perf.yml index cd831b180..3056eff27 100644 --- a/.github/workflows/perf.yml +++ b/.github/workflows/perf.yml @@ -128,7 +128,7 @@ jobs: arch: arm64 runs-on: macos-latest steps: - # The `perf-test` label opt-in for `pull_request` runs is enforced by + # The `ci/perf-test` label opt-in for `pull_request` runs is enforced by # the caller (`heph.yml`'s `perfbench` job `if:`) — an unlabeled PR # never invokes this workflow at all, so there is no in-job gate step # here to fail past.