From c2b2121e0fb992b68f6062e90fc1887ed4fa75cb Mon Sep 17 00:00:00 2001 From: Carlo van Driesten Date: Wed, 11 Nov 2026 11:00:28 +0100 Subject: [PATCH 1/2] chore(fork): scale process cost to change risk Two scheduled jobs had been red on every run for a fortnight, and both alarms were false. `sync-mirror.yml` masked a `gh label create` failure with `|| true` and then hard-failed on the `gh issue create` that depended on the label. `fork-only-view.yml` failed its first stack invariant because the stack was one rebase behind upstream, returned early, and so silently stopped running the view-drift and standing-PR checks it exists for. Both are fixed at the root. The tracking issue is now matched by exact title, so no second object has to exist first, and it is the only alarm for an outstanding rebase. `verify_stack` gains `behind_is_notice`: a behind stack is reported as a warning and the remaining invariants are measured against the mirror the stack was built on, so a sync state can no longer disable a different detector. The rest reduces what every agent session costs. The always-loaded instructions drop from 6056 to 3941 words, with the rationale moved into PROCESS.md where it is read on demand. Review is now tiered: `forkctl track` classifies a diff as A (pufferlib, config, pufferl.py, models.py, setup.py, smoke goldens) or B (everything else), and only Track A owes a fresh-context adversarial review and the simulator suites. Nits are stated as a process bug, not a review comment. The PR template drops from 85 lines and 20 checkboxes to 56 and 7, by deleting everything forkctl check-commit already proves. --- .github/copilot-instructions.md | 723 ++++++++++----------------- .github/fork/PROCESS.md | 83 ++- .github/fork/forkctl.py | 137 ++++- .github/fork/test_forkctl.py | 221 +++++++- .github/pull_request_template.md | 82 +-- .github/workflows/fork-gates.yml | 68 +++ .github/workflows/fork-only-view.yml | 23 +- .github/workflows/sync-mirror.yml | 30 +- CONTRIBUTING.md | 26 +- 9 files changed, 791 insertions(+), 602 deletions(-) create mode 100644 .github/workflows/fork-gates.yml diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 0c941e295e..fa5a11a36d 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,90 +1,75 @@ # ASCS-eV/PufferDrive — fork contribution rules -**Audience:** every coding agent (Copilot CLI/IDE, Claude Code, Codex, …) and every human working in -`ASCS-eV/PufferDrive`. +**Audience:** every coding agent and every human working in `ASCS-eV/PufferDrive`. +**Fork-only:** this file must never reach `Emerge-Lab/PufferDrive`. -**This file is fork-only.** It does not exist upstream and must never reach `Emerge-Lab/PufferDrive`. - -**This file is the core.** It is loaded automatically in every session in this repository, so it -carries only what you need to avoid doing damage. The deep procedures — the ones you run rarely and -must not improvise — live in `.github/fork/PROCESS.md`, which is **not** auto-loaded. Each one is -signposted below. Read the signposted section before you run the procedure; do not reconstruct it -from memory. +This file is **rules and commands only**, because it is auto-loaded into every session here. Every +rationale, worked procedure and hazard story lives in `.github/fork/PROCESS.md`, which is **not** +auto-loaded. Read the signposted section there before running a procedure; do not improvise it. ## 0. Precedence 1. The human's explicit instruction in the current turn. -2. **This file** — fork branching, PR, and release process. -3. `.github/fork/PROCESS.md` — the same process in detail; never contradicts this file. +2. **This file** — fork branching, PR and release process. +3. `.github/fork/PROCESS.md` — the detail; never contradicts this file. 4. `AGENTS.md` / `CLAUDE.md` (upstream-owned) — code style, trust boundaries, guardrails. -Rules 2–3 and 4 do not overlap: they never redefine coding standards, and `AGENTS.md` never -defines fork process. Where `AGENTS.md` speaks about issues and PRs, it is describing -**upstream's** process; for PRs raised inside this fork, section 5 below applies instead. +They do not overlap: 2–3 never redefine coding standards, and `AGENTS.md` never defines fork +process. Where `AGENTS.md` speaks about issues and PRs it describes **upstream's** process; for PRs +raised inside this fork, section 5 applies. + +### 0.1 `forkctl` -### 0.1 `forkctl` — the checks live in code, not in this file +Every mechanical check below is implemented once in `.github/fork/forkctl.py` and tested in +`test_forkctl.py`. Stdlib-only, no shell; identical on Linux, macOS, WSL and Windows. -Every mechanical check in this document is implemented once, in `.github/fork/forkctl.py`, and is -covered by `.github/fork/test_forkctl.py`. It is stdlib-only and spawns no shell, so it behaves -identically on Linux, macOS, WSL and a Windows checkout, and there is nothing to install: +**The ones that gate a PR — `check-commit` and `mirror` — run in CI as the `Fork gates` check +(4.5).** You normally never type them. The table is for when you want the answer early, or need to +debug a red gate. ```bash -python .github/fork/forkctl.py --help +python .github/fork/forkctl.py --help # written below as `forkctl ` ``` -For brevity this file writes `forkctl `. Expand it to the line above, or alias it with -`alias forkctl='python .github/fork/forkctl.py'`. - -| Command | What it decides | Where it is written up | -| --- | --- | --- | -| `forkctl doctor` | is this clone set up, and is the process itself mid-change? | 4.1 | -| `forkctl check-commit [--chore]` | exactly one commit, carrying no fork-only content | 4.4, 4.5 | -| `forkctl check-stack` | the stack invariants, before any force-push | 3.3, 7 | -| `forkctl mirror --sha\|--subject` | gate 3, and building the upstream branch | 4.8, 6 | -| `forkctl status` | divergence report, stale commits, in-flight mirrors | 7, and *While the mirror PR is open* / *After upstream responds* in `PROCESS.md` | -| `forkctl delete-mirror ` | is it safe to delete this mirror branch? | *While the mirror PR is open* in `PROCESS.md` | -| `forkctl fork-only [-v]` | the canonical fork-only path list | 3.1 | - -Bare numbers are sections of this file; italics are headings in `.github/fork/PROCESS.md`. - -Each exits non-zero on failure and prints the fix, so they compose into any script and are safe in -front of a `&&`. - -**Do not retype these checks as shell one-liners.** The previous revision of this file did, and the -copies drifted: the fork-only file list existed in six places, and the `AGENTS.md` fork-block guard -never fired for an edit *between* the markers, because the changed lines carry no marker of their -own. One implementation, with tests that prove each guard fails when it should, is the point. - -### 0.2 When to open `.github/fork/PROCESS.md` - -| You are about to… | Read | +| Command | What it decides | +| --- | --- | +| `forkctl doctor` | is this clone set up, and is the process itself mid-change? (4.1) | +| `forkctl check-commit [--chore]` | exactly one commit, carrying no fork-only content (4.4) | +| `forkctl track` | Track A or B — how much review this diff owes (4.7) | +| `forkctl check-stack` | stack invariants, before any force-push (3.3, 7) | +| `forkctl check-view` | the view branch and its standing PR (2) | +| `forkctl mirror --sha\|--subject` | Gate 3, and building the upstream branch (4.8, 6) | +| `forkctl status` | divergence report, stale commits, in-flight mirrors (7) | +| `forkctl delete-mirror ` | is it safe to delete this mirror branch? (6) | +| `forkctl fork-only [-v]` | the canonical fork-only path list (3.1) | + +Each exits non-zero on failure and prints the fix. **Never retype these checks as shell one-liners** +— copies drift; one tested implementation is the point. + +### 0.2 When to open `PROCESS.md` + +| About to… | Read | | --- | --- | -| change a fork-only file on a `chore/` branch | *Changing fork-only content* | -| adversarially review someone's feature PR | *Adversarial review* | -| open, refresh, or retire an upstream PR | *Mirror PR workflow* | -| rebase the stack onto new upstream, or cut a release | *Syncing and releasing* | +| change a fork-only file on `chore/` | *Changing fork-only content* | +| adversarially review a Track A PR | *Adversarial review* | +| open, refresh or retire an upstream PR | *Mirror PR workflow* | +| rebase the stack, or cut a release | *Syncing and releasing* | | touch either standing draft PR | *The standing draft PRs* | -Writing a feature, running the gates, and opening a fork PR need **only this file**. That is the -common case, and it is why the detail was moved out. +Writing a feature, running the gates and opening a fork PR need **only this file**. ## 1. Why this fork exists -`ASCS-eV/PufferDrive` is a fork of `Emerge-Lab/PufferDrive`, consumed as a **git submodule** by -internal BMW projects. It has two jobs: - -- Provide a **stable, integrated branch** that internal consumers can pin. -- Prepare **small, well-tested, independently reviewable features** that we offer back upstream as - mirror PRs once we consider them ready. - -Everything below exists to keep those two jobs from fighting each other. +A fork of `Emerge-Lab/PufferDrive`, consumed as a **git submodule** by internal BMW projects. Two +jobs: a **stable integrated branch** consumers can pin, and **small, independently reviewable +features** offered back upstream. Everything below keeps those two from fighting each other. ## 2. Branch model | Branch | Role | Who writes | History | | --- | --- | --- | --- | | `3.0` | Verbatim mirror of `upstream/3.0`; base for every upstream PR | automation only | fast-forward only | -| `integration/3.0` | **Default branch.** Combined feature stack; what BMW consumes | feature PRs only | linear, rebased | +| `integration/3.0` | **Default branch.** Feature stack; what BMW consumes | feature PRs only | linear, rebased | | `feature/` | One feature, **exactly one commit** | contributor/agent | rebased onto `integration/3.0` | | `chore/` | Fork-only change, **exactly one commit** | contributor/agent | rebased onto `integration/3.0` | | `mirror/` | Upstream PR source | generated by cherry-pick | cut from `upstream/3.0` | @@ -93,18 +78,6 @@ Everything below exists to keep those two jobs from fighting each other. Remotes: `origin` = `ASCS-eV/PufferDrive`, `upstream` = `Emerge-Lab/PufferDrive`. -`integration/3.0` is the default branch because GitHub reads the PR template, the contributing -guidelines, and scheduled workflows **only** from the default branch — and none of those fork-only -files may exist on `3.0` without destroying the mirror. A welcome side effect: new PRs default to -the correct base. - -The same reasoning governs this file. A fresh `git clone` checks out the default branch, so the copy -of `.github/copilot-instructions.md`, `AGENTS.md`, and `CLAUDE.md` **on `integration/3.0` is the -process** as far as every agent — and every BMW submodule consumer — is concerned. A process change -that exists only in an open PR does not exist. That is precisely why fork-only changes are folded -into the base commit on the default branch (section 3.3) instead of being parked on a long-lived -branch, and why section 4.1 opens by checking whether the rules are mid-change. - ``` upstream/3.0 ──► 3.0 (mirror) └─► integration/3.0 = 3.0 + [fork scaffolding] + [feat A] + [feat B] + … @@ -112,251 +85,164 @@ upstream/3.0 ──► 3.0 (mirror) 3.0 ──► mirror/ (cherry-pick of that same 1 commit → upstream PR) ``` -`upstream/2.0` and `upstream/3.0` have **no common ancestor**. Never attempt to merge, rebase, or -cherry-pick between the `2.0` and `3.0` lines. +`integration/3.0` is the default branch because GitHub reads the PR template, the contributing +guidelines and scheduled workflows only from the default branch. The copy of this file **on +`integration/3.0` is the process**; a process change that exists only in an open PR does not exist. + +`upstream/2.0` and `upstream/3.0` have **no common ancestor**. Never merge, rebase or cherry-pick +between the `2.0` and `3.0` lines. ### Hard branch rules -- **Nothing reaches `integration/3.0` that has not cleared the merge gate in section 4.8** — green - CI, adversarial review, and proven upstream-readiness. The stack is a line of - already-contributable commits, not a staging area for work in progress. -- **Never commit to `3.0`.** It is a pure mirror; any local commit there breaks the clean base for - every mirror PR. -- **Never create a merge commit on `integration/3.0`.** The stack stays linear so that each feature - remains a single, cherry-pickable commit. +- **Nothing reaches `integration/3.0` that has not cleared the merge gate in 4.8.** +- **Never commit to `3.0`.** Any local commit there breaks the base for every mirror PR. +- **Never create a merge commit on `integration/3.0`.** Each feature stays one cherry-pickable commit. - **Never branch a feature off another unmerged feature.** Each feature must apply to pure `3.0`. -- `integration/3.0` is maintained by rebase and is therefore **force-pushed** (`--force-with-lease`). - Contributors re-sync with `git fetch origin && git reset --hard origin/integration/3.0` after - confirming `git status` is clean — **not** `git pull --rebase`, which replays your local copies of - commits that were rewritten or dropped during the rebase and silently resurrects them. Consumers - pin an immutable tag (section 7), not the branch tip. - -**How `integration/3.0` is protected, and the deliberate hole in it.** All 12 required checks are -enforced, and "require branches to be up to date" is on, so a PR cannot be merged green against a -stale base and land an untested combination. Admin enforcement is deliberately **off**: with it on, -GitHub rejects *any* direct ref update to the branch (verified — it returns -`8 of 12 required status checks are failing`), which would make the fixup flow in section 3.3 and -the stack rebase in section 7 impossible. Those two procedures bypass the gate by design, and the -invariant checks written into them are the compensating control. Do not force-push this branch -outside those two procedures. +- Re-sync with `git fetch origin && git reset --hard origin/integration/3.0` on a clean tree — + **not** `git pull --rebase`, which resurrects rewritten commits. Consumers pin a tag (7), not the tip. +- **Never force-push `integration/3.0` outside the two procedures in 3.3 and 7.** Admin enforcement + is deliberately off so those two can move the ref; their invariant checks are the only control. ### The two standing draft PRs -Two permanent **draft PRs target `3.0`**, and both must **never be merged, never be marked ready -for review, and never be committed to**: - -- **`integration/3.0` → `3.0`** — `DO NOT MERGE — integration stack CI`. A continuous CI verdict on - all combined features at once, which no individual feature PR can provide. It refreshes itself - whenever the stack is force-pushed. -- **`fork-only/3.0` → `3.0`** — `DO NOT MERGE — fork-only content view`. Its *Files changed* tab is - exactly what this fork adds and never sends upstream, with no features mixed in. Its branch is a - derived **view** that is republished **by hand** (section 3.3 and section 7); - `.github/workflows/fork-only-view.yml` only *detects* drift, it cannot publish. +Both target `3.0` and must **never be merged, marked ready, or committed to**: -Merging either would push fork-only content into the mirror and destroy the clean base that every -upstream PR is cut from. +- **`integration/3.0` → `3.0`** — continuous CI verdict on all features combined. Self-refreshing. +- **`fork-only/3.0` → `3.0`** — *Files changed* is exactly what this fork adds. Its branch is + republished **by hand** (3.3, 7); `fork-only-view.yml` only *detects* drift. -Both carry the `do-not-merge` label **alone**. Never give a standing PR `do-not-mirror` as well: the -two labels *together* are the in-flight-process-change signal that `forkctl doctor` queries, and a -permanent PR carrying both would tell every agent, forever, that the rules are mid-change. The rule -is general: **a standing artifact carries `do-not-merge` alone; both labels together mark a -`chore/` PR that is actively rewriting the process.** - -Rationale, and why the view branch cannot be automated: `.github/fork/PROCESS.md` → *The standing -draft PRs*. +Both carry `do-not-merge` **alone**. The two labels *together* are the in-flight-process-change +signal `forkctl doctor` queries. **A standing artifact carries `do-not-merge` alone; both labels +together mark a `chore/` PR actively rewriting the process.** ## 3. Fork-only content — never send this upstream -There are **two kinds**, and they are checked differently. Both must stay out of every feature -commit and every mirror PR. - ### 3.1 Whole files that exist only here -If any of these appears in a feature diff, the feature is contaminated: - -- `.github/copilot-instructions.md` (this file) -- `CONTRIBUTING.md` -- `.github/pull_request_template.md` -- `.github/workflows/sync-mirror.yml` -- `.github/workflows/fork-only-view.yml` -- anything under `.github/fork/` — the fork's own tooling and its tests +`.github/copilot-instructions.md`, `CONTRIBUTING.md`, `.github/pull_request_template.md`, +`.github/workflows/sync-mirror.yml`, `.github/workflows/fork-only-view.yml`, +`.github/workflows/fork-gates.yml`, and **anything under `.github/fork/`** (a prefix, not a list). -The last entry is a **prefix, not a list**: a new fork-only tool dropped into `.github/fork/` is -covered without editing anything. The authoritative definition lives in `.github/fork/forkctl.py` -and is printed by `forkctl fork-only -v`. Do not copy it into another file; a stale copy is a guard -that has silently stopped firing. +`forkctl fork-only -v` prints the authoritative list. Do not copy it elsewhere — a stale copy is a +guard that has silently stopped firing. -### 3.2 Fork-only edits inside files that also exist upstream +### 3.2 Fork-only edits inside files shared with upstream -These files are shared with upstream, so the **file** appearing in a diff is fine and often -correct — only the **fork-only hunk** must stay put: +The **file** in a diff is fine; only the **fork-only hunk** must stay put: | Shared file | Fork-only part | | --- | --- | | `AGENTS.md`, `CLAUDE.md` | the `` … `` block | -| `.github/workflows/ci.yml`, `.github/workflows/install.yml` | the `'integration/3.0'` entry in the **`pull_request`** `branches:` filter | +| `.github/workflows/ci.yml`, `install.yml` | the `'integration/3.0'` entry in the **`pull_request`** `branches:` filter | -A file-name check cannot catch 3.2 — only a content check can (section 4.5). Conversely, do not -treat a shared file as untouchable: a PR that rewrites the install steps of `ci.yml` and -`install.yml` is fully upstream-contributable, because it leaves the branch filter alone. +Do not treat a shared file as untouchable: rewriting the install steps of `ci.yml` is fully +upstream-contributable, because it leaves the branch filter alone. -Do **not** add `integration/3.0` to those workflows' `push:` filters. The standing draft PR -(section 2) already runs the full suite on every update of the branch, so a `push` trigger only -duplicates every job and doubles runner consumption. +Do **not** add `integration/3.0` to those workflows' `push:` filters — the standing draft PR already +runs the full suite on every update. That applies to the **test suites**; `fork-only-view.yml` +deliberately does use `push:`, and must not be "fixed" to match. -That reasoning is about the **test suites** specifically. `fork-only-view.yml` deliberately does use -a `push:` trigger on `integration/3.0`: it runs a handful of git commands rather than a test matrix, -and it exists precisely to react to that branch moving. Do not "fix" it to match the others. +### 3.3 Changing fork-only content -Fork-only content lives in **one dedicated commit** (`chore(fork): …`) sitting directly on top of -`3.0` at the base of the integration stack. That commit is never cherry-picked. +Fork-only content lives in **exactly one** `chore(fork):` commit at the base of the stack. +`forkctl check-stack` asserts there is one, so a second breaks every future sync. -### 3.3 Changing fork-only content +- branch `chore/` from `origin/integration/3.0`, **exactly one commit**; +- open as a **draft** titled `[do-not-mirror][do-not-merge] chore(fork): …`, with **both** the + `do-not-mirror` and `do-not-merge` labels; +- Gates 1 and 2 (4.8) apply; Gate 3 does not; +- **never merged with the button** — folded into the base commit by fixup, ending **closed, not + merged**. Merging would put a second `chore(fork):` commit on the stack and abort every sync. -Fork-only content lives in **one dedicated commit** (`chore(fork): …`) sitting directly on top of -`3.0` at the base of the integration stack. That commit is never cherry-picked, and there is exactly -one of it — `forkctl check-stack` asserts that, so a second one breaks every future sync. +The fold force-pushes the default branch: a maintainer action, never taken by an agent unasked (5). +**Procedure: `PROCESS.md` → *Changing fork-only content*. Read it before you start.** -Changing it therefore has its own shape: +**Keep the window short.** Until the fold lands, the default branch still serves the *old* rules. A +fork-only PR is a **review vehicle, not a delivery mechanism**. + +A change to a **shared** file (`ci.yml`, `AGENTS.md`, `.pre-commit-config.yaml`, any upstream-owned +source) is **not** fork-only work, even when the motive is fork-internal. It belongs in a normal +feature commit: `check-stack` rejects a fork base that touches an unregistered file, or a shared +file outside its fork-only hunk. -- branch `chore/` from `origin/integration/3.0`, **exactly one commit**; -- open the PR as a **draft** titled `[do-not-mirror][do-not-merge] chore(fork): …`, with **both** - the `do-not-mirror` and `do-not-merge` labels; -- Gates 1 and 2 (section 4.8) apply in full; Gate 3 does not; -- it is **never merged with the button** — it is folded into the base commit by fixup and ends - **closed, not merged**. - -Those are two independent axes and both must be marked, because they fail in different directions. -`do-not-mirror` means the change must never reach upstream; `do-not-merge` means the merge button -must never be pressed, because merging would put a *second* `chore(fork):` commit on the stack and -every future sync would abort until someone unpicked it by hand. - -The fold force-pushes the default branch, so it is a maintainer action: an agent must not perform it -unasked (section 5). - -**The full procedure — the fixup, the invariant re-check, and the atomic push that moves -`integration/3.0` and `fork-only/3.0` together — is in `.github/fork/PROCESS.md` → *Changing -fork-only content*. Read it before you start; do not reconstruct it from memory.** - -**Keep the window short.** Between opening a `chore/` PR and folding it in, the default branch -still serves the *old* rules, and every agent that clones the fork in that window follows them — see -section 2. A fork-only PR is a **review vehicle, not a delivery mechanism**: nothing in it is in -force until the force-push lands. So do not park one open across unrelated work, and when one has -been open a while, assume agents have already acted on the superseded text and check their output -against the new rules rather than assuming compliance. - -If a feature genuinely needs to change one of these files, make it a **separate** `chore(fork):` -commit — do not fold it into the feature commit. ## 4. Feature workflow ### 4.1 Start -One-time setup in a fresh clone — `upstream` is not configured automatically, and several commands -below (and Gate 3) fail without it: - ```bash git remote get-url upstream >/dev/null 2>&1 \ || git remote add upstream https://github.com/Emerge-Lab/PufferDrive.git gh repo set-default ASCS-eV/PufferDrive # else bare `gh pr …` can resolve to upstream uv venv && source .venv/bin/activate && uv pip install -e . -``` - -Then check the clone, and whether the process itself is mid-change: -```bash forkctl doctor -``` - -It verifies the remote and the virtualenv, and lists any open PR carrying **both** fork-only -labels. Such a PR is rewriting the rules you are about to follow: anything it reports supersedes -this file in the areas it touches, so read it first and rebase onto the result rather than racing -it. (The query ANDs both labels, so it matches process PRs and nothing else — the two standing PRs -of section 2 carry `do-not-merge` alone and are correctly excluded. That exclusion is the whole -reason a standing artifact must never be given the second label.) - -Then: - -```bash git fetch origin && git fetch upstream git switch -c feature/ origin/integration/3.0 ``` -`` is short, lowercase, hyphenated, and describes the change (`lane-heading-obs`, not -`fix-stuff`). +`doctor` lists any open PR carrying **both** fork-only labels. Such a PR is rewriting the rules you +are about to follow: read it first and rebase onto the result rather than racing it. -For fork-only work that will become a `chore(fork):` commit, name the branch `chore/` -instead. Everything else in section 4 applies unchanged, except Gate 3 (section 4.8), from which -such commits are exempt. +`` is short, lowercase, hyphenated, descriptive (`lane-heading-obs`, not `fix-stuff`). For +fork-only work use `chore/`; all of section 4 applies except Gate 3. ### 4.2 Scope -One feature = one reviewable idea. If the PR description needs the word "and" to list unrelated -changes, split it. Refactors that merely enable a feature go in their own earlier PR. +One feature = one reviewable idea. If the PR description needs "and" to list unrelated changes, +split it. Refactors that merely enable a feature go in their own earlier PR. ### 4.3 Build and test -Makefile targets assume a POSIX shell and `.venv/bin/python` — use Linux, macOS, or WSL. +**CI runs the suites. Do not run them locally first.** Every PR into `integration/3.0` already runs +unit, eval, C, notebook, smoke-train and smoke-perf on Linux, which is the environment that counts. +Re-running them on your laptop costs minutes, proves less, and on Windows cannot be done at all. -| Situation | Required before pushing | +Before pushing you owe exactly two things: + +| Always | Why it is not CI's job | | --- | --- | -| any change | `pre-commit run --all-files` | -| `.c` / `.h` touched | `make rebuild` (zero warnings — warnings are bugs) | -| Python / env / eval | `make test-unit`, `make test-eval` | -| C sim logic | `make test-c` | -| notebooks or their deps | `make test-notebooks` | -| **sim dynamics, rewards, observations, or training loop** | `make test-docker-smoke` (golden train/rollout smoke run) | -| perf-sensitive hot paths | `make -C tests/drive perf` | -| everything in the default suite, before marking ready | `make test` | +| `pre-commit run --all-files` | a formatting-only CI round trip is pure latency | +| `make rebuild` **if `.c`/`.h` changed** | a build break should not cost a 12-minute run; zero warnings — warnings are bugs | -`make test` is **not** everything: it runs `test-unit test-eval test-c test-notebooks` and -deliberately excludes `test-docker-smoke` (slow, needs Docker) and the perf target. If the table -above says you need those, run them explicitly — `make test` passing does not cover them. +Run a suite locally only to **debug a red check**, or when CI cannot run it at all: -**Regenerating smoke goldens.** If your change legitimately moves the golden train/rollout values -(observation layout, dynamics, rewards, RNG draw order), regenerate them with -`tests/smoke_tests/update_goldens.sh`, which runs inside the pinned QEMU image so the numbers match -CI. Review the diff, then stage both files **in the same single commit as the change that caused -them** — the rollout golden is gitignored and needs `-f`: +| Not covered by CI | Command | +| --- | --- | +| dynamics, rewards, observations, training loop | `make test-docker-smoke` | +| perf-sensitive hot paths | `make -C tests/drive perf` | +| anything needing a GPU or triton | no CI runner has one — say so in the PR | -```bash -tests/smoke_tests/update_goldens.sh -# tests/smoke_tests/data/ matches .gitignore, so BOTH goldens need -f even though -# both are already tracked — a plain `git add` on them exits 1. -git add -f tests/smoke_tests/data/drive_smoke_golden.json \ - tests/smoke_tests/data/drive_rollout_golden.json -``` +Makefile targets assume a POSIX shell and `.venv/bin/python` — use Linux, macOS or WSL. -Goldens are **base-dependent**: values regenerated on `integration/3.0` are not necessarily valid on -pure `upstream/3.0`. If Gate 3 (section 4.8) shows the smoke test failing on `mirror/`, -regenerate there and carry the corrected values back, rather than assuming our stack's numbers are -upstream's. +**New behaviour requires a new test.** A fix requires a test that fails before it and passes after. +"Existing tests still pass" is not sufficient for a feature PR. -**New behaviour requires a new test.** A bug fix requires a test that fails before the fix and -passes after it. "Existing tests still pass" is not sufficient evidence for a feature PR. +In the PR, list only what you ran that CI did not. Never claim a suite passed unless it ran to +completion. Do not paste CI's own output back into the PR — link the run instead. -Paste the commands you actually ran, and their outcome, into the PR's *How tested* section. Do not -claim a suite passed unless it ran to completion in this environment. +Smoke goldens are **base-dependent** and are regenerated only with +`tests/smoke_tests/update_goldens.sh`, staged `-f` into the same commit as the change that moved +them. Procedure and the Gate 3 consequence: `PROCESS.md` → *Smoke goldens*. ### 4.4 Shape the commit -Squash to **exactly one commit** before requesting review: - ```bash git fetch origin -git rebase origin/integration/3.0 # replay onto the current base git reset --soft origin/integration/3.0 # collapse the branch into the index git commit -F # or -m; never rely on an editor opening -forkctl check-commit ``` -If the rebase conflicts: resolve, `git add` the files, then -`GIT_EDITOR=true git rebase --continue` — plain `--continue` opens an editor and fails outright when -none is available. To back out, `git rebase --abort`. +Rebase only when you actually need the newer base, or when the merge button says the commit no +longer applies: `git rebase origin/integration/3.0`. **Being behind the base is not a defect** — +"Rebase and merge" replays the commit and refuses if it cannot, so there is no reason for every +open PR to rebase each time something else lands. -Use the non-interactive form above rather than `git rebase -i`: agents run without an editor, where -the interactive rebase either stalls or silently does nothing. +On conflict: resolve, `git add`, then `GIT_EDITOR=true git rebase --continue`; back out with +`git rebase --abort`. Never use `git rebase -i` — agents run without an editor, where it stalls or +silently does nothing. -Conventional Commits, imperative mood, ≤ 72-char subject: +Conventional Commits, imperative mood, **≤ 72-char subject** (count it before committing; this is +the single most common gate failure): ``` feat(drive): add lane-heading error to the observation vector @@ -366,289 +252,204 @@ would otherwise have to reverse-engineer. ``` Types: `feat`, `fix`, `perf`, `refactor`, `test`, `docs`, `build`, `ci`, `chore`. -Common scopes: `drive`, `obs`, `reward`, `train`, `eval`, `render`, `config`, `fork`. - +Scopes: `drive`, `obs`, `reward`, `train`, `eval`, `render`, `config`, `fork`. `chore(fork): …` marks a commit that **must never go upstream**. -Do not add AI/agent authorship trailers or "generated by" notes to commits, or to **fork** PR text. +No AI/agent authorship trailers or "generated by" notes in commits or **fork** PR text. The one +exception is the upstream disclosure required by section 6, which is **mandatory** on mirror PR +bodies — a disclosure to upstream, not an authorship trailer. -The single exception is the upstream authorship disclosure required by section 6, which is -**mandatory** on mirror PR bodies. That is a disclosure to upstream, not an authorship trailer, and -this rule does not license omitting it. +### 4.5 The gates run themselves -### 4.5 Verify upstream-cleanliness before opening the PR +**You do not run the gates, and you never paste their output into a PR.** The `Fork gates` check +runs `forkctl check-commit` and Gate 3 on every PR into `integration/3.0`, picking `--chore` and +the Gate 3 exemption off the commit subject itself. A failure is a red check with the fix in its +log, like any other test. -```bash -forkctl check-commit -``` - -It asserts exactly one commit, no fork-only **file** in the diff, and no fork-only **hunk** or -**block** touched in a shared file. It exits non-zero and names the offender; a check that prints -contamination and exits `0` is not a check. Changing the *rest* of a shared file is allowed and is -sometimes the whole point of the PR — only the fork-only parts of section 3.2 are protected. - -This is the cheap pre-flight check. The authoritative proof is Gate 3 in section 4.8. - -**`chore/` branches pass `--chore`** — their whole purpose is to change the files in section -3.1, so the fork-only checks are skipped and only the one-commit rule is enforced. The flag also -asserts the subject really is `chore(fork): …`, so it cannot be used to wave a feature through: +Run them locally only to get the answer before pushing, or to debug that red check: ```bash -forkctl check-commit --chore +forkctl check-commit # --chore on a chore/ branch +forkctl mirror --sha HEAD --no-publish # Gate 3, without moving mirror/ ``` +Drop `--no-publish` only when you actually want the branch, i.e. when opening an upstream PR (6). + ### 4.6 Open the fork PR -Base **`integration/3.0`** (the default, so it is pre-selected). Fill in -`.github/pull_request_template.md` completely and let CI finish. +Base **`integration/3.0`** (the default). Fill in `.github/pull_request_template.md`, let CI finish. +Opening the PR is not the end of your job — it must clear 4.8. -Opening the PR is **not** the end of your job — a PR that is merely open is not stacked. It must -clear section 4.8 first. +### 4.7 Review — two tracks -### 4.7 Adversarial review +Review cost scales with what the change can break. `forkctl track` decides from the changed paths; +do not argue it in prose. -Every feature PR is adversarially reviewed before it may be stacked. The reviewer's job is to -**try to break the change**, not to confirm that it looks reasonable. "LGTM" is not a review. +**Track A — sim-affecting.** Anything under `pufferlib/`, `config/`, `pufferl.py`, `models.py`, +`setup.py` or the smoke goldens, excluding `.md`. These change what BMW consumes. -**The reviewer is not the author.** When an agent wrote the change, the review runs as a *separate -task with fresh context* — a reviewer still holding the author's reasoning reproduces the author's -blind spots. +Track A requires an **adversarial review**: the reviewer's job is to **try to break the change**, +not to confirm it looks reasonable. "LGTM" is not a review. **The reviewer is not the author** — +when an agent wrote it, the review runs as a *separate task with fresh context*. Post the outcome as +a PR review listing what was probed and what was found, **including probes that found nothing**. +Every finding is fixed, or explicitly accepted by a human with the reason in the thread. +Checklist: `PROCESS.md` → *Adversarial review*. -Post the outcome as a PR review listing what was probed and what was found — **including the probes -that found nothing**, so the next reader knows the coverage. Each finding is either fixed or -explicitly accepted by a human, with the reason in the thread. +**Track B — everything else.** Docs, fork tooling, workflows, CI, build scripts, tests. One reviewer +reads the diff once and confirms three things: scope matches the stated idea, the claimed commands +were really run, and the change really is not sim-affecting. No fresh-context agent, no written +probe list, no checklist. A second opinion is allowed, not required. -**The checklist the reviewer works through is in `.github/fork/PROCESS.md` → *Adversarial review*.** -It covers test validity, breaking inputs, determinism, trust boundaries, layout compatibility, -performance, scope creep and upstream reception. Read it when reviewing; you do not need it when -authoring. -### 4.8 The merge gate — nothing stacks until it is proven +**Nits are a process bug, not a review comment.** If a formatter, linter or `forkctl` could have +caught it, say so once and fix the tool — do not hand-annotate instances. No review comments about +whitespace, import order, line length or wording preference: those belong to `pre-commit`. Authors +run `pre-commit run --all-files` before asking for review, so there is nothing of that kind to find. -`integration/3.0` is what BMW pins *and* the base that every later feature rebases onto, so a defect -merged here is multiplied across every downstream consumer and every subsequent PR. Merge a feature -PR **only** when all applicable gates are green: +### 4.8 The merge gate -**Gate 1 — CI is green.** Every required check passes *on the final commit*. Not "green apart from a -known flake", not "green before the last force-push". This is enforced by GitHub, not by good -intentions: the required set is configured as branch protection on `integration/3.0`, so the merge -button stays disabled until all of it is green. Confirm with: +`integration/3.0` is what BMW pins *and* the base every later feature rebases onto, so a defect here +is multiplied. Two of the three gates are checks on the PR; only one needs a person. ```bash gh pr checks --repo ASCS-eV/PufferDrive ``` -If a check is broken for reasons unrelated to the feature, fix that first as its own PR — a CI fix -is itself a perfectly good feature PR — instead of merging past it. +**Gate 1 — CI green** on the *final* commit. Not "green apart from a known flake", not "green before +the last force-push". Enforced by branch protection. If a check is broken for unrelated reasons, fix +that first as its own PR rather than merging past it. -**If the commit adds or changes a workflow, Gate 1 also requires that the workflow has actually -run.** The required checks validate the repository; they do not execute your workflow, so a green -board says nothing about it. Dispatch it on the PR ref and link the run: +**Gate 2 — review passed**, at the depth 4.7 requires for this track. The only gate a human owns. -```bash -gh workflow run .yml --ref -gh run list --workflow .yml --limit 1 --json databaseId,conclusion -``` +**Gate 3 — upstream-readiness**, proven by the `Fork gates` check (4.5): the commit cherry-picks +onto pure `upstream/3.0` and the resulting diff carries no fork-only file or marker. A conflict +means the feature is entangled with fork-only state — fix it on the feature branch. -Exercise each state it is supposed to distinguish, not just the happy path. This rule exists because -a fork-only workflow once cleared Gate 1 **and** Gate 2 and was still impossible: GitHub rejects any -`GITHUB_TOKEN` push that touches `.github/workflows/`, and nothing short of running it could have -revealed that. Assertions about a workflow are worth exactly as much as the runs behind them. +Two things the check cannot see, which stay on you: -**Gate 2 — adversarial review passed.** Section 4.7, findings written down and resolved. +- **A commit that adds or changes a workflow must also have *run* that workflow.** Required checks + validate the repository; they do not execute yours. Dispatch it, exercise each state it is meant + to distinguish, and link the run in the PR. -**Gate 3 — upstream-readiness is proven, not asserted.** Build the mirror branch *before* merging -and let the cherry-pick prove the commit is contributable as-is: + ```bash + gh workflow run .yml --ref + gh run list --workflow .yml --limit 1 --json databaseId,conclusion + ``` -```bash -git fetch upstream -forkctl mirror --sha -``` +- **If the change can move the smoke goldens**, run the relevant suites on the mirror branch, not + just diff it. Goldens that depend on an earlier feature upstream does not have are not + upstream-ready. + +*Opening* the upstream PR is a separate, human-approved step (5, 6) — the gate is only that we could +open it today. -It recreates `mirror/` from pure `upstream/3.0`, cherry-picks the commit, asserts the result -contains no fork-only file and adds no fork-only marker anywhere, prints the file list for review, -and returns you to the branch you started on. It exits non-zero on any of those, and leaves the -branch in place for inspection when it fails. - -Recreating `mirror/` every time is deliberate — section 7 requires re-proving this gate after -a rebase. If the cherry-pick conflicts, the feature is entangled with fork-only state: fix it on -the feature branch and re-run. - -**If the change can move the smoke goldens** (observation layout, dynamics, rewards, RNG order), -Gate 3 must also *run* the relevant suites from section 4.3 on the mirror branch, not just diff it. -Goldens generated on our stack are not evidence about pure upstream. A commit whose goldens depend -on an earlier feature that upstream does not have is **not** upstream-ready: it does not stack until -that dependency is resolved. - -Actually *opening* the upstream PR stays a separate, human-approved step (sections 5 and 6) — the -gate is only that we could open it today. Record the resulting `integration/3.0` SHA on the PR when -the feature is stacked, so section 6 can rebuild the mirror later from a known commit. - -**Exemption — `chore(fork):` commits.** Fork-only infrastructure cannot be upstream-ready by -definition, so Gate 3 does not apply, and section 4.5 is run with `--chore`, which skips the -fork-only checks — touching those files is the entire point. **Every other rule still holds**, in -particular "exactly one commit": section 3.3 cherry-picks a single SHA, so a two-commit chore branch -silently loses work. Gates 1 and 2 apply in full, and the reviewer additionally confirms the change -is genuinely fork-only and contains nothing upstream would have wanted. Such a PR is a draft titled -`[do-not-mirror][do-not-merge] …` and carries both the `do-not-mirror` and `do-not-merge` labels; -it is applied by fixup into the base -commit rather than merged (section 3.3), so it ends **closed, not merged** — the "Rebase and merge" -step below does not apply to it. - -Only once the applicable gates pass, **a human presses "Rebase and merge"** (never squash-merge a -branch that is already one commit, never create a merge commit). Squash and merge commits are -disabled on this repository so the wrong button cannot be pressed by accident. - -If a feature is good but not yet upstream-ready, it does **not** go on the stack. Leave the PR open -and say why. An unmergeable-but-correct feature is a smaller problem than a stack nobody can -contribute from. +**Exemption — `chore(fork):` commits:** Gate 3 does not apply; the check detects this from the +subject and skips it. Every other rule holds, in particular "exactly one commit": 3.3 cherry-picks a +single SHA, so a two-commit chore branch silently loses work. + +Only once the gates pass, **a human presses "Rebase and merge"**. Squash and merge commits are +disabled so the wrong button cannot be pressed. A feature that is good but not yet upstream-ready +does **not** go on the stack: leave the PR open and say why. ## 5. Agent rules for issues and PRs -- Agents **may** open, update, and comment on PRs and issues **in `ASCS-eV/PufferDrive`**. -- An agent **must not** adversarially review (section 4.7) a change it authored in the same session. - Start a fresh review task with no authoring context, or hand it to a human. -- Agents **must not** merge into `integration/3.0` on their own initiative. Report the gate status - from section 4.8 and let a human decide. -- **A human must verify the final PR title, body, and diff before an agent opens an _upstream_ PR.** - The agent may press the button; it may not decide that content is fit to send to another project. - Show the exact text and the diff, get explicit approval, then open it. That approval is **per PR, - in the current conversation**, and covers the `mirror/` content too. - Fork-internal PRs need no such approval — open them, fill the template honestly, and report the - gate status. The human gate on fork PRs is the **merge** button (above), not the open button. -- Never open a PR targeting the `3.0` mirror. The only PRs that may ever target it are the two - standing drafts from section 2 — the integration stack CI verdict and the fork-only view. Each is - opened **once, by a human**, and must never be merged. The integration draft refreshes itself - whenever `integration/3.0` moves; the fork-only view does **not** — its branch is republished by - hand (section 2), and a workflow fails until that happens. -- When a PR targets upstream, **upstream's `AGENTS.md` "Issue and PR Guidelines" apply**, with one - documented exception handled in section 6. Do not import them into fork PRs. -- Never force-push a branch you did not create. Never force-push `integration/3.0` without being - asked to. -- Never commit secrets, internal BMW identifiers, customer data, internal URLs, or proprietary +- Agents **may** open, update and comment on PRs and issues **in `ASCS-eV/PufferDrive`**. +- An agent **must not** review a change it authored in the same session. Use a fresh task with no + authoring context, or hand it to a human. +- Agents **must not** merge into `integration/3.0` on their own initiative. Report the 4.8 gate + status and let a human decide. +- **A human must verify the final title, body and diff before an agent opens an _upstream_ PR.** The + agent may press the button; it may not decide content is fit to send to another project. Approval + is **per PR, in the current conversation**, and covers the `mirror/` content. Fork-internal + PRs need no such approval — their human gate is the **merge** button. +- Never open a PR targeting `3.0`. The only ones that may are the two standing drafts (2), each + opened **once, by a human**, and never merged. +- Never force-push a branch you did not create. Never force-push `integration/3.0` unasked. +- Never commit secrets, internal BMW identifiers, customer data, internal URLs or proprietary scenario data. This fork is public. +- For PRs targeting upstream, **upstream's `AGENTS.md` "Issue and PR Guidelines" apply**, with the + exception in section 6. Do not import them into fork PRs. ## 6. Mirror PR workflow (fork → upstream) -Once a feature is stacked, offering it upstream is a separate, **human-approved** step (section 5). +Offering a stacked feature upstream is a separate, **human-approved** step (5). -Gate 3 already proved the commit cherry-picks cleanly, but do not assume the branch it produced -still exists — it is local-only. **Always rebuild the mirror from scratch**, and locate the commit -**by subject, not by the SHA recorded on the feature PR**: every sync rebase rewrites the stack, so -a recorded SHA stops resolving as soon as `3.0` advances. +**Always rebuild the mirror from scratch**, and locate the commit **by subject, never by a recorded +SHA** — every sync rebase rewrites the stack. ```bash git fetch origin && git fetch upstream forkctl mirror --subject "feat(scope): the exact commit subject" --stay ``` -If it aborts, the feature commit is contaminated or no longer applies — fix it on -`integration/3.0` and regenerate. Otherwise push `mirror/` to `origin` and open the upstream -PR from it **after a human has verified the title, body and diff**. - -Re-run the relevant suites from section 4.3 on the mirror branch: it is a different base from the -one CI tested, and "it passed on our stack" is not evidence that it passes on pure upstream. +If it aborts, the commit is contaminated or no longer applies: fix it on `integration/3.0` and +regenerate. Otherwise push `mirror/` to `origin` and open the upstream PR **after a human has +verified title, body and diff**. Re-run the 4.3 suites on the mirror branch — "it passed on our +stack" is not evidence about pure upstream. Three things an upstream PR gets wrong by default: -- **Structure is upstream's**: *What / Why / Notes*. Give them evidence, not our internal process - vocabulary — upstream does not know or care what our integration stack is. -- **Title style is upstream's, not ours.** They use imperative sentence case, not Conventional - Commits; a `fix(ci):` prefix marks the PR as foreign on sight. The *commit* keeps our - Conventional Commits subject — only the PR title is restyled. -- **Every upstream PR body must carry the AI-assistance disclosure** we use in place of upstream's - `AGENTIC_PR` canary. Omitting both the file and the disclosure is not an option: it would quietly - defeat a detection mechanism the maintainers put there on purpose. +- **Structure is upstream's**: *What / Why / Notes*. No internal process vocabulary. +- **Title style is upstream's**: imperative sentence case, not Conventional Commits. The *commit* + keeps our subject; only the PR title is restyled. +- **The body must carry the AI-assistance disclosure** we use in place of upstream's `AGENTIC_PR` + canary. Omitting both is not an option. -**The exact disclosure text, keeping an open PR fresh while upstream reviews it, the -branch-deletion hazard, and what to do after upstream merges, amends or rejects, are in -`.github/fork/PROCESS.md` → *Mirror PR workflow*. Read it before touching an upstream PR.** - -Two commands cover the routine half of that: +**Exact disclosure text, refreshing an open PR, the branch-deletion hazard, and what to do after +upstream merges, amends or rejects: `PROCESS.md` → *Mirror PR workflow*.** ```bash forkctl status # what is in flight, and which mirrors have gone stale forkctl delete-mirror # refuses while the upstream PR is still open ``` -## 7. Syncing and releasing -**Sync the mirror:** automated. `.github/workflows/sync-mirror.yml` fast-forwards `3.0` from -`upstream/3.0` daily and on demand (Actions → *Sync 3.0 mirror* → *Run workflow*). It refuses any -non-fast-forward, which is the alarm that someone committed to the mirror. `3.0` additionally blocks -force-pushes, deletions, and non-linear history. **Never push to `3.0` by hand.** +## 7. Syncing and releasing -When the mirror moves ahead of the stack, the same workflow opens (and keeps updated) a -`mirror-sync` tracking issue, and closes it once `integration/3.0` contains the mirror head. The -rebase itself stays manual — replaying features onto new upstream code can conflict and must be -re-tested. +`sync-mirror.yml` fast-forwards `3.0` daily and on demand. It refuses any non-fast-forward, which is +the alarm that someone committed to the mirror. **Never push to `3.0` by hand.** -**Rebasing the stack, and tagging a release, force-push or move refs on the branch BMW pins.** Both -are maintainer actions; an agent must not perform either unasked (section 5). The procedure, the -conflict policy, the explicit `--force-with-lease=:` rationale and the tagging steps are -in `.github/fork/PROCESS.md` → *Syncing and releasing*. +When the mirror moves ahead of the stack, that workflow opens and maintains an issue titled +`Rebase integration/3.0 onto upstream 3.0`, closing it once the stack contains the mirror head. +**That issue is the only alarm for an outstanding rebase.** No workflow fails while it is open: +being behind upstream is a normal window, not a defect. `forkctl check-view` reports it as a warning +and carries on, so view drift is still detected meanwhile. -**Hazard — never dispatch `update-smoke-golden.yml` against `3.0`.** This inherited upstream -workflow ends in `git push origin HEAD:` and triggers on pushes to `3.0` as well as -`workflow_dispatch`, so dispatching it there commits to the mirror and destroys the clean base every -upstream PR is cut from. No `forkctl` command can stop this — a workflow dispatch is not a git -operation — so it is the one destructive action guarded by nothing but this sentence. Regenerate -goldens with `tests/smoke_tests/update_goldens.sh` on your own branch instead (section 4.3). -Recovery is in `.github/fork/PROCESS.md` → *Syncing and releasing*. +**Rebasing the stack and tagging a release move refs on the branch BMW pins.** Both are maintainer +actions, never taken by an agent unasked (5). Procedure, conflict policy, the explicit +`--force-with-lease=:` rationale and the tagging steps: `PROCESS.md` → *Syncing and +releasing*. -**Health check — run it after every sync and before any release:** +**Hazard — never dispatch `update-smoke-golden.yml` against `3.0`.** It ends in +`git push origin HEAD:`, so dispatching it there commits to the mirror and destroys the clean +base every upstream PR is cut from. No `forkctl` command can stop this — a workflow dispatch is not +a git operation — so it is guarded by nothing but this sentence. Regenerate goldens on your own +branch instead (4.3). ```bash git fetch origin && git fetch upstream -forkctl status +forkctl status # after every sync, before any release ``` -It reports the total gap, the permanent fork-only floor, the feature gap, any commits upstream -already has (which the last sync should have dropped), and every in-flight mirror PR. - -**The total gap can never reach zero, and must not be driven to zero.** `integration/3.0` -permanently carries at least one `chore(fork):` commit, because the files in section 3.1 do not -exist upstream by design. An agent that reads a non-zero gap as a defect and tries to close it by -upstreaming that commit would leak the entire fork-only process into `Emerge-Lab/PufferDrive` — -precisely what Gate 3 exists to prevent. - -**The number that should trend to zero is the feature gap.** Zero means upstream has absorbed -everything we carry and the fork is pure scaffolding again; that is the steady state we are aiming -for, not an exceptional one. A feature gap that only grows means we are accumulating a private fork -rather than contributing — treat a persistently rising count as a process failure to discuss, not a -number to ignore. Every feature commit on the stack should be traceable to an open or pending mirror -PR (section 6); one that is not is either unfinished work or a change nobody intends to upstream, -and the second kind belongs in a `chore(fork):` commit where the guards can see it. -## 8. Definition of done +**The total gap can never reach zero**: the stack permanently carries the `chore(fork):` commit. An +agent that treats a non-zero gap as a defect and upstreams that commit would leak the entire +fork-only process into `Emerge-Lab/PufferDrive`. **The number that should trend to zero is the +feature gap**; a persistently rising one is a process failure to discuss, not a number to ignore. -A feature PR may be **stacked onto `integration/3.0`** only when **all** of the following hold. -Anything less stays an open PR. +## 8. Definition of done -Authoring: +A feature PR may be **stacked** only when all of the following hold: -- exactly one commit, Conventional Commits subject, rebased on current `integration/3.0` -- no fork-only file from section 3.1 and no fork-only hunk from section 3.2 in its diff +- **Gate 1** — every required check green on the final commit +- **Gate 2** — review at the depth 4.7 requires for this track, findings resolved +- **Gate 3** — the `Fork gates` check green *(it exempts `chore(fork):` itself)* - new or updated tests cover the new behaviour; a fix has a test that fails without it -- the suites from section 4.3 that apply were run to completion and are listed in the PR -- the PR template is filled in, including the upstream-readiness assessment +- the PR template is filled in, including the upstream disposition -Gates (section 4.8): - -- **Gate 1** — every required check green on the final commit -- **Gate 2** — adversarial review (section 4.7) posted, findings fixed or explicitly accepted -- **Gate 3** — `mirror/` cherry-picks onto pure `upstream/3.0` without conflict and its diff - is upstream-clean — i.e. the feature is genuinely contributable today - *(exempt for `chore(fork):` commits, which can never be upstream-ready)* +Gates 1 and 3 are checks: read them, do not reproduce them. One commit, a well-formed subject and a +clean fork-only boundary are all asserted by Gate 3's job, so there is nothing left to assert by +hand. Merged by a human with **Rebase and merge**, leaving the stack linear. -A `chore(fork):` PR has a different definition of done: exactly one commit, Gates 1 and 2 green, -Gate 3 and the section 4.5 fork-only checks exempt, all four `do-not-mirror` markers **and** both -`do-not-merge` markers present, and it is **folded into the base commit by fixup and the PR closed -unmerged** (section 3.3) — never merged with the button. - -The machine-checkable half of this list is two commands. Neither replaces Gates 1 and 2, which are -human and CI judgements: - -```bash -forkctl check-commit # authoring bullets 1-2 (--chore for a chore(fork): branch) -forkctl mirror --sha HEAD # Gate 3 (skip for chore(fork):) -``` +A `chore(fork):` PR instead: Gates 1 and 2 green, Gate 3 skipped by the check, all four +`do-not-mirror` markers and both `do-not-merge` markers present, and **folded into the base commit +by fixup and closed unmerged** (3.3). diff --git a/.github/fork/PROCESS.md b/.github/fork/PROCESS.md index 99b76e59c7..7e05927ede 100644 --- a/.github/fork/PROCESS.md +++ b/.github/fork/PROCESS.md @@ -12,7 +12,8 @@ Read the section you need, when you need it. The core tells you which one: | --- | --- | | merge or close one of the two standing draft PRs | [Standing PRs](#the-standing-draft-prs) | | change a fork-only file (`chore/`) | [Changing fork-only content](#changing-fork-only-content) | -| review someone else's feature PR | [Adversarial review](#adversarial-review) | +| review a Track A (sim-affecting) feature PR | [Adversarial review](#adversarial-review) | +| regenerate the smoke goldens | [Smoke goldens](#smoke-goldens) | | open, refresh, or retire an upstream PR | [Mirror PR workflow](#mirror-pr-workflow-fork--upstream) | | rebase the stack onto new upstream, or cut a release | [Syncing and releasing](#syncing-and-releasing) | @@ -61,10 +62,17 @@ stack as well. is a far worse trade than one extra line in two procedures that are already manual. - `.github/workflows/fork-only-view.yml` is therefore a **detector, not a publisher**, and needs no write access at all (`contents: read`). It re-derives the fork base, **fails if `fork-only/3.0` is - missing or stale** and prints the exact command to fix it, refuses to pass a stack that violates - the stack invariants in *Syncing and releasing*, and checks that the standing PR still has base `3.0`, draft status and - the `do-not-merge` label. It runs on pushes to `integration/3.0` **and to `fork-only/3.0`** (so - tampering with the view itself is caught), plus daily, plus on demand. Drift is loud, not silent. + missing or stale** and prints the exact command to fix it, and checks that the standing PR still + has base `3.0`, draft status and the `do-not-merge` label. It runs on pushes to `integration/3.0` + **and to `fork-only/3.0`** (so tampering with the view itself is caught), plus daily, plus on + demand. Drift is loud, not silent. +- **A stack that is behind upstream is not view drift, and does not fail this job.** It is the + normal window between upstream moving and the next rebase, and it is owned by `sync-mirror.yml`, + which tracks it in an issue. `forkctl check-view` reports it with `::warning::` and then measures + the remaining invariants against the mirror the stack was actually built on. This matters: before + the split, one outstanding rebase turned the job red *and* returned early, so the view-drift and + standing-PR checks — the only reason the job exists — silently stopped running for as long as the + rebase was outstanding. An alarm that disables the detector it guards is worse than no alarm. - **The workflow does not open the PR either; a human does, once** — this repository deliberately withholds PR-creation rights from `GITHUB_TOKEN` (`can_approve_pull_request_reviews: false`). Afterwards the workflow only *checks* it: it warns when the standing PR is missing, and **fails @@ -169,8 +177,15 @@ commit — do not fold it into the feature commit. ## Adversarial review -Every feature PR is adversarially reviewed before it may be stacked. The reviewer's job is to -**try to break the change**, not to confirm that it looks reasonable. "LGTM" is not a review. +**This applies to Track A only** — a change touching `pufferlib/`, `config/`, `pufferl.py`, +`models.py`, `setup.py` or the smoke goldens, as decided by `forkctl track`. Track B changes (docs, +fork tooling, workflows, CI, build scripts, tests) get one human read against the three questions in +core section 4.7, and none of this checklist. Spending a fresh-context agent and a written probe +list on a Markdown edit is the cost the tiering exists to remove; skipping them on a reward change +is the risk it exists to keep paying for. + +The reviewer's job is to **try to break the change**, not to confirm that it looks reasonable. +"LGTM" is not a review. **The reviewer is not the author.** When an agent wrote the change, the review runs as a *separate task with fresh context* — a reviewer still holding the author's reasoning reproduces the author's @@ -199,6 +214,43 @@ Post the outcome as a PR review listing what was probed and what was found — * that found nothing**, so the next reader knows the coverage. Each finding is either fixed or explicitly accepted by a human, with the reason in the thread. +**Do not spend a review comment on anything a tool owns.** Whitespace, import order, line length, +formatting and wording preference belong to `pre-commit`; commit shape and fork-only contamination +belong to `forkctl`. If one of those reaches review, the finding is "the tool did not catch this" — +report it once and fix the tool. Hand-annotating instances costs the author a round trip, costs an +agent a full context reload, and leaves the next change just as exposed. + +## Smoke goldens + +The golden train/rollout values are a *behavioural* fixture: they change only when observation +layout, dynamics, rewards or RNG draw order change. If your change legitimately moves them, +regenerate inside the pinned QEMU image so the numbers match CI, rather than on your own machine: + +```bash +tests/smoke_tests/update_goldens.sh +``` + +Review the diff, then stage both files **in the same single commit as the change that caused them**. +`tests/smoke_tests/data/` matches `.gitignore`, so **both** goldens need `-f` even though both are +already tracked — a plain `git add` on them exits 1: + +```bash +git add -f tests/smoke_tests/data/drive_smoke_golden.json \ + tests/smoke_tests/data/drive_rollout_golden.json +``` + +**Goldens are base-dependent.** Values regenerated on `integration/3.0` are not necessarily valid on +pure `upstream/3.0`: our stack may already carry a feature that moved them. So if Gate 3 shows the +smoke test failing on `mirror/`, regenerate *there* and carry the corrected values back — +never assume our stack's numbers are upstream's. + +A commit whose goldens depend on an earlier feature that upstream does not have is **not** +upstream-ready. It does not stack until that dependency is resolved, because Gate 3 would be a +promise we could not keep. + +Touching the goldens makes a change Track A by construction (`forkctl track` lists +`tests/smoke_tests/data/` as sim-affecting), so the full review above applies. + ## Mirror PR workflow (fork → upstream) Gate 3 (section 4.8) already proved this commit cherry-picks cleanly. Do **not** assume the branch @@ -434,10 +486,17 @@ drawing any conclusion from `git cherry`, which compares the stale patch rather non-fast-forward, which is the alarm that someone committed to the mirror. `3.0` additionally blocks force-pushes, deletions, and non-linear history. Do not push to it by hand. -When the mirror moves ahead of the stack, the same workflow opens (and keeps updated) a -`mirror-sync` tracking issue; it closes the issue automatically once `integration/3.0` contains the -mirror head. The rebase itself stays manual — replaying features onto new upstream code can -conflict and must be re-tested. +When the mirror moves ahead of the stack, the same workflow opens (and keeps updated) a tracking +issue titled `Rebase integration/3.0 onto upstream 3.0`, and closes it automatically once +`integration/3.0` contains the mirror head. It is matched by **exact title, not by a label**: a +label is a second object that must exist before the issue can be filed, and a masked +`gh label create ... || true` once failed silently while every scheduled run went red on the +`gh issue create` that depended on it. + +That issue is the **only** alarm for an outstanding rebase, and nothing fails while it is open. +Being behind upstream is the normal window between upstream moving and the next rebase, not a +defect, and a daily red cron for a known state trains everyone to ignore the board. The rebase +itself stays manual — replaying features onto new upstream code can conflict and must be re-tested. **Rebase the stack** (maintainers): @@ -521,7 +580,7 @@ and the second kind belongs in a `chore(fork):` commit where the guards can see `git push origin HEAD:`, and it triggers on pushes to `3.0` as well as `workflow_dispatch`. Never dispatch it against `3.0`: it would commit to the mirror and break the clean base for every upstream PR. If it ever does, `sync-mirror.yml` is the detector — it refuses the non-fast-forward -and the mirror-sync issue appears. Recover by resetting `3.0` to `upstream/3.0` (the branch +and the rebase tracking issue appears. Recover by resetting `3.0` to `upstream/3.0` (the branch protection on `3.0` blocks force-pushes, so this needs an admin and should be done deliberately). **Release for the BMW submodule:** after a green rebase, run the divergence report above and confirm diff --git a/.github/fork/forkctl.py b/.github/fork/forkctl.py index 5e88a0c825..9fe54afdab 100644 --- a/.github/fork/forkctl.py +++ b/.github/fork/forkctl.py @@ -39,6 +39,28 @@ FORK_COMMIT_PREFIX = "chore(fork): " +# Paths whose behaviour the submodule consumers depend on: the simulation, the +# policy, the training loop and the config that drives them. A commit touching +# any of them is Track A and pays the full review cost (section 4.7); anything +# else is Track B. Defined here, not in prose, so the two cannot drift apart. +# The list is deliberately coarse: over-classifying costs a review, while +# under-classifying ships unreviewed sim behaviour. +SIM_PATHS = ( + "pufferl.py", + "models.py", + "setup.py", +) + +SIM_PREFIXES = ( + "pufferlib/", + "config/", + "tests/smoke_tests/data/", +) + +# Documentation never changes shipped behaviour, so it stays Track B even under +# a Track A prefix. Everything else under those prefixes is Track A. +NON_SIM_SUFFIXES = (".md",) + # Files that exist only in this fork; none may appear in a feature diff. This # tuple is the single source of truth. The docs point here instead of repeating # the list, so adding a fork-only file cannot leave a stale copy behind. @@ -48,6 +70,7 @@ ".github/pull_request_template.md", ".github/workflows/sync-mirror.yml", ".github/workflows/fork-only-view.yml", + ".github/workflows/fork-gates.yml", ) # Anything under these prefixes is fork-only by construction, so a new fork-only @@ -734,6 +757,37 @@ def current_branch() -> str: return git("rev-parse", "HEAD") if branch == "HEAD" else branch +def is_sim_path(path: str) -> bool: + if path.endswith(NON_SIM_SUFFIXES): + return False + return path in SIM_PATHS or path.startswith(SIM_PREFIXES) + + +def track_of(base: str, head: str = "HEAD") -> tuple[str, list[str]]: + """Section 4.7: which review track this diff falls into, and why. + + Returns ("A", ) or ("B", []). + """ + sim = sorted(path for path in diff_files(base, head) if is_sim_path(path)) + return ("A" if sim else "B"), sim + + +def cmd_track(args: argparse.Namespace) -> int: + track, sim = track_of(args.base, args.head) + if args.quiet: + print(track) + return 0 + report = Report("track", args.format) + if track == "A": + report.info(f"Track A -- sim-affecting ({len(sim)} path(s)); full gates and adversarial review") + for path in sim: + report.info(f" {path}") + else: + report.info("Track B -- no sim-affecting path; CI, forkctl and a human read are the gate") + print(track, file=report.stream) + return 0 + + def cmd_fork_only(args: argparse.Namespace) -> int: for path in FORK_ONLY_PATHS: print(path) @@ -763,15 +817,13 @@ def cmd_check_commit(args: argparse.Namespace) -> int: f"git rebase {args.base} && git reset --soft {args.base} && git commit -F msg", ) - # One commit *ahead* is not the same as rebased onto the current base: a - # branch cut weeks ago is still one commit ahead of where it started. - if one_commit: - report.check( - git("rev-parse", "HEAD^") == git("rev-parse", args.base), - f"rebased directly onto {args.base}", - f"the commit is not parented on the current {args.base} tip", - f"git rebase {args.base}", - ) + # Being behind the base is not a defect. "Rebase and merge" replays the + # commit itself and refuses when it cannot, so the merge button already + # enforces linearity; failing here only forces every open PR to rebase and + # re-run its gates each time anything else lands. + if one_commit and git("rev-parse", "HEAD^") != git("rev-parse", args.base): + behind = len(git_lines("rev-list", f"HEAD^..{args.base}")) + report.warn(f"{behind} commit(s) behind {args.base}; rebase before merging if the button objects") subject = git("log", "-1", "--format=%s") if commits else "" problems = subject_problems(subject) @@ -814,8 +866,15 @@ def cmd_check_commit(args: argparse.Namespace) -> int: if args.chore: report.info("fork-only file and hunk checks: exempt (chore(fork) branch)") + report.info("review track: B (fork-only infrastructure is never sim-affecting)") return report.finish() + track, sim = track_of(args.base) + if track == "A": + report.info(f"review track: A -- sim-affecting, full gates and adversarial review ({len(sim)} path(s))") + else: + report.info("review track: B -- CI, forkctl and a human read are the gate (section 4.7)") + offenders = fork_only_files_in(args.base) report.check( not offenders, @@ -843,8 +902,14 @@ def cmd_check_commit(args: argparse.Namespace) -> int: return report.finish() -def verify_stack(report: Report, mirror_ref: str, stack_ref: str) -> str | None: - """Sections 3.3 and 7: stack invariants. Returns the fork base, or None.""" +def verify_stack(report: Report, mirror_ref: str, stack_ref: str, behind_is_notice: bool = False) -> str | None: + """Sections 3.3 and 7: stack invariants. Returns the fork base, or None. + + A stack that is behind the mirror is a *sync* state, not a malformed stack: + it is the normal window between upstream moving and the next rebase, and it + is owned and reported by `sync-mirror.yml`. Callers that police something + else pass `behind_is_notice` so that window does not mask their own checks. + """ if not require_refs(report, mirror_ref, stack_ref): return None @@ -852,7 +917,17 @@ def verify_stack(report: Report, mirror_ref: str, stack_ref: str) -> str | None: stack = git("rev-parse", stack_ref) contains = git_rc("merge-base", "--is-ancestor", mirror, stack) == 0 - if not report.check( + if not contains and behind_is_notice: + behind = len(git_lines("rev-list", f"{stack}..{mirror}")) + report.warn( + f"{stack_ref} is {behind} commit(s) behind {mirror_ref}; " + "the rebase is tracked by sync-mirror.yml, not by this check" + ) + # Measure against the mirror the stack was actually built on, so the + # remaining invariants still describe this stack rather than failing + # for the one reason that is already reported elsewhere. + mirror = git("merge-base", mirror, stack) + elif not report.check( contains, f"{stack_ref} contains the {mirror_ref} tip", f"{stack_ref} does not contain the {mirror_ref} tip", @@ -1025,13 +1100,16 @@ def resolve_feature_commit(report: Report, args: argparse.Namespace) -> str | No "Gate 3 proves the commit under review; use --subject to rebuild a stacked commit", ) return None - parented_on_tip = git_rc("rev-parse", "--verify", "--quiet", f"{sha}^") == 0 and git( - "rev-parse", f"{sha}^" - ) == git("rev-parse", args.stack) - if not parented_on_tip: + # The invariant is "one commit, and its parent is on the stack line" -- + # not "parented on the tip". A branch that is merely behind still has + # exactly one commit to prove, and failing it here would drag every open + # PR through a rebase each time something else lands (4.4). + parent_exists = git_rc("rev-parse", "--verify", "--quiet", f"{sha}^") == 0 + on_stack_line = parent_exists and git_rc("merge-base", "--is-ancestor", f"{sha}^", args.stack) == 0 + if not on_stack_line: report.fail( - f"{head} is not a single commit on top of the {args.stack} tip", - "squash to one commit and rebase onto the current stack tip (section 4.4)", + f"{head} is not a single commit on top of {args.stack}", + "squash to one commit branched from the stack (section 4.4)", ) return None return sha @@ -1126,6 +1204,8 @@ def cmd_mirror(args: argparse.Namespace) -> int: candidate = git("rev-parse", "HEAD") if report.failed: report.info(f"{branch} was left untouched; nothing was published") + elif args.no_publish: + report.info(f"checks only: {branch} was not moved") else: if ref_exists(branch) and git("rev-parse", branch) != candidate: report.info(f"{branch} moved from {git('rev-parse', branch)[:9]} to {candidate[:9]}") @@ -1218,9 +1298,15 @@ def cmd_delete_mirror(args: argparse.Namespace) -> int: def cmd_check_view(args: argparse.Namespace) -> int: - """Section 2: fork-only/3.0 is a derived view, republished by hand.""" + """Section 2: fork-only/3.0 is a derived view, republished by hand. + + Whether the stack is current with upstream is deliberately not this check's + business: that is reported once, by sync-mirror.yml. Failing on it here + would skip every assertion below and silently disable the drift detector + for as long as a rebase is outstanding. + """ report = Report("check-view", args.format) - base = verify_stack(report, args.mirror, args.stack) + base = verify_stack(report, args.mirror, args.stack, behind_is_notice=True) if base is None: return report.finish() report.ok(f"stack is sound; fork base is {base[:9]}") @@ -1488,6 +1574,13 @@ def add_fmt(p: argparse.ArgumentParser) -> None: add_fmt(p) p.set_defaults(func=cmd_check_commit) + p = sub.add_parser("track", help="review track for a diff (section 4.7)") + p.add_argument("--base", default=STACK_REF) + p.add_argument("--head", default="HEAD") + p.add_argument("--quiet", action="store_true", help="print only A or B") + add_fmt(p) + p.set_defaults(func=cmd_track) + p = sub.add_parser("check-stack", help="stack invariants (sections 3.3, 7)") p.add_argument("--mirror", default=MIRROR_REF) p.add_argument("--stack", default=STACK_REF) @@ -1514,7 +1607,9 @@ def add_fmt(p: argparse.ArgumentParser) -> None: p.add_argument("--upstream", default=UPSTREAM_REF) p.add_argument("--mirror", default=MIRROR_REF) p.add_argument("--stack", default=STACK_REF) - p.add_argument("--stay", action="store_true", help="stay on the mirror branch") + publish = p.add_mutually_exclusive_group() + publish.add_argument("--stay", action="store_true", help="stay on the mirror branch") + publish.add_argument("--no-publish", action="store_true", help="prove the gate without moving mirror/") add_fmt(p) p.set_defaults(func=cmd_mirror) diff --git a/.github/fork/test_forkctl.py b/.github/fork/test_forkctl.py index 54a270164a..66d5c77e1e 100644 --- a/.github/fork/test_forkctl.py +++ b/.github/fork/test_forkctl.py @@ -517,6 +517,46 @@ def test_rebuilds_an_existing_mirror_branch(self) -> None: ) self.assertEqual(code, 0, out) + def test_no_publish_proves_the_gate_without_moving_the_branch(self) -> None: + """CI proves Gate 3 on every PR; it must not leave mirror branches behind.""" + git(self.repo, "branch", "mirror/clean", "up") + before = git(self.repo, "rev-parse", "mirror/clean") + sha = self.stacked_feature("feat(x): clean work", {"clean.c": "x\n"}) + code, out = run( + "mirror", + "clean", + "--sha", + sha, + "--no-publish", + "--upstream", + "up", + "--mirror", + "mir", + "--stack", + "stack", + ) + self.assertEqual(code, 0, out) + self.assertIn("was not moved", out) + self.assertEqual(git(self.repo, "rev-parse", "mirror/clean"), before) + + def test_no_publish_still_fails_a_contaminated_commit(self) -> None: + sha = self.stacked_feature("feat(x): leak", {".github/fork/leaked.py": "secret\n"}) + code, out = run( + "mirror", + "clean", + "--sha", + sha, + "--no-publish", + "--upstream", + "up", + "--mirror", + "mir", + "--stack", + "stack", + ) + self.assertEqual(code, 1, out) + self.assertIn("fork-only file", out) + def test_refuses_to_run_with_a_dirty_tree(self) -> None: write(self.repo, "scratch.txt", "uncommitted\n") code, out = run( @@ -645,16 +685,30 @@ def test_a_duplicate_fork_block_is_rejected(self) -> None: self.assertEqual(code, 1, out) self.assertIn("marker structure", out) - def test_a_stale_base_is_rejected(self) -> None: - """One commit *ahead* is not the same as rebased onto the current tip.""" + def test_a_stale_base_is_a_notice_not_a_failure(self) -> None: + """Being behind the base does not block: "Rebase and merge" replays the + commit and refuses if it cannot, so failing here would only force every + open PR to rebase and re-run its gates each time something else lands.""" self.feature_branch() commit(self.repo, "feat(x): mine", {"mine.c": "x\n"}) git(self.repo, "switch", "-q", "stack") commit(self.repo, "feat(y): someone else landed first", {"theirs.c": "y\n"}) git(self.repo, "switch", "-q", "feature/x") code, out = run("check-commit", "--base", "stack") + self.assertEqual(code, 0, out) + self.assertIn("1 commit(s) behind", out) + + def test_being_behind_does_not_mask_a_real_failure(self) -> None: + """The downgrade must not turn a stale branch into a free pass.""" + self.feature_branch() + commit(self.repo, "feat(x): mine", {".github/copilot-instructions.md": "leaked\n"}) + git(self.repo, "switch", "-q", "stack") + commit(self.repo, "feat(y): someone else landed first", {"theirs.c": "y\n"}) + git(self.repo, "switch", "-q", "feature/x") + code, out = run("check-commit", "--base", "stack") self.assertEqual(code, 1, out) - self.assertIn("not parented on the current", out) + self.assertIn("1 commit(s) behind", out) + self.assertIn("fork-only file", out) def test_a_non_conventional_subject_is_rejected(self) -> None: self.feature_branch() @@ -692,13 +746,19 @@ def test_an_unregistered_file_in_the_fork_base_is_caught(self) -> None: self.assertEqual(code, 1, out) self.assertIn("unregistered", out) - def test_gate_three_refuses_an_arbitrary_commit(self) -> None: - """--sha must prove the commit being merged, not any commit that exists.""" - git(self.repo, "switch", "-q", "-c", "elsewhere", "up") - outsider = commit(self.repo, "feat(x): unrelated external commit", {"x.c": "x\n"}) - code, out = run("mirror", "arb", "--sha", outsider, "--upstream", "up", "--mirror", "mir", "--stack", "stack") - self.assertEqual(code, 1, out) - self.assertIn("not a single commit on top of", out) + def test_gate_three_accepts_a_branch_that_is_behind_the_stack_tip(self) -> None: + """Being behind is not a defect (4.4). Gate 3's question is whether the + commit cherry-picks onto pure upstream, which a stale base does not + change. An arbitrary commit is still refused -- by the rule that --sha + must be HEAD, covered by test_gate_three_refuses_a_commit_that_is_not_checked_out.""" + sha = self.stacked_feature("feat(x): cut before the tip moved", {"mine.c": "x\n"}) + git(self.repo, "switch", "-q", "stack") + commit(self.repo, "feat(y): landed first", {"theirs.c": "y\n"}) + git(self.repo, "switch", "-q", "feature/x") + code, out = run( + "mirror", "behind", "--sha", sha, "--no-publish", "--upstream", "up", "--mirror", "mir", "--stack", "stack" + ) + self.assertEqual(code, 0, out) def test_gate_three_refuses_a_commit_that_is_not_checked_out(self) -> None: git(self.repo, "switch", "-q", "-c", "elsewhere-2", "up") @@ -1231,6 +1291,134 @@ def test_a_crlf_fork_base_separator_is_accepted(self) -> None: self.assertIn(stripped, (upstream, upstream + separator)) +class TestBehindIsNotice(ForkRepoTest): + """A stack behind the mirror must not mask the checks that follow it. + + `check-view` exists to detect view drift and standing-PR tampering. Before + the split, one outstanding rebase failed the very first invariant and + returned early, so those detectors silently stopped running for as long as + the rebase was outstanding -- an alarm that disables the detector it guards. + """ + + def _verify(self, behind_is_notice: bool) -> tuple[bool, str, str | None]: + buf = io.StringIO() + report = forkctl.Report("test", stream=buf) + base = forkctl.verify_stack(report, "mir", "stack", behind_is_notice=behind_is_notice) + return report.failed, buf.getvalue(), base + + def _move_mirror_ahead(self) -> None: + git(self.repo, "switch", "-q", "mir") + commit(self.repo, "upstream: moved on", {"README.md": "newer\n"}) + git(self.repo, "switch", "-q", "stack") + + def test_behind_still_fails_by_default(self) -> None: + self._move_mirror_ahead() + failed, out, base = self._verify(behind_is_notice=False) + self.assertTrue(failed, out) + self.assertIsNone(base) + self.assertIn("does not contain", out) + + def test_behind_is_a_warning_and_the_rest_still_runs(self) -> None: + self._move_mirror_ahead() + failed, out, base = self._verify(behind_is_notice=True) + self.assertFalse(failed, out) + self.assertIn("1 commit(s) behind", out) + self.assertIn("sync-mirror.yml", out) + self.assertIn("exactly one chore(fork): commit", out) + self.assertEqual(base, self.base) + + def test_a_behind_stack_is_still_measured_for_real_violations(self) -> None: + """The fallback must not become a blanket pass.""" + self._move_mirror_ahead() + commit(self.repo, "chore(fork): second one", {"CONTRIBUTING.md": "again\n"}) + failed, out, base = self._verify(behind_is_notice=True) + self.assertTrue(failed, out) + self.assertIn("2 chore(fork): commits", out) + + def test_an_up_to_date_stack_warns_about_nothing(self) -> None: + failed, out, base = self._verify(behind_is_notice=True) + self.assertFalse(failed, out) + self.assertNotIn("behind", out) + self.assertEqual(base, self.base) + + +class TestTrack(ForkRepoTest): + """Review effort is tiered by what the change can break, not by its size.""" + + def test_a_simulator_change_is_track_a(self) -> None: + self.stacked_feature( + "feat(drive): touch the sim", + {"pufferlib/ocean/drive/drive.h": "float x;\n"}, + ) + code, out = run("track", "--base", "stack", "--head", "HEAD") + self.assertEqual(code, 0, out) + self.assertIn("Track A", out) + self.assertIn("pufferlib/ocean/drive/drive.h", out) + + def test_a_docs_only_change_is_track_b(self) -> None: + self.stacked_feature("docs: explain a thing", {"README.md": "clearer\n"}) + code, out = run("track", "--base", "stack", "--head", "HEAD") + self.assertEqual(code, 0, out) + self.assertIn("Track B", out) + + def test_fork_tooling_is_track_b(self) -> None: + self.stacked_feature( + "chore(fork): tweak the tooling", + {".github/fork/forkctl.py": "# changed\n"}, + ) + code, out = run("track", "--base", "stack", "--head", "HEAD") + self.assertEqual(code, 0, out) + self.assertIn("Track B", out) + + def test_a_mixed_change_is_track_a(self) -> None: + """One simulator path is enough; the tier is the maximum, not the average.""" + self.stacked_feature( + "feat(drive): sim plus docs", + {"README.md": "clearer\n", "config/puffer_drive.yaml": "lr: 0.1\n"}, + ) + code, out = run("track", "--base", "stack", "--head", "HEAD") + self.assertEqual(code, 0, out) + self.assertIn("Track A", out) + + def test_a_markdown_file_under_a_simulator_path_is_track_b(self) -> None: + """Documentation cannot change behaviour, wherever it lives.""" + self.stacked_feature( + "docs(drive): document the observation layout", + {"pufferlib/ocean/drive/README.md": "layout\n"}, + ) + code, out = run("track", "--base", "stack", "--head", "HEAD") + self.assertEqual(code, 0, out) + self.assertIn("Track B", out) + + def test_the_smoke_goldens_are_track_a(self) -> None: + self.stacked_feature( + "test(drive): refresh goldens", + {"tests/smoke_tests/data/drive_smoke_golden.json": "{}\n"}, + ) + code, out = run("track", "--base", "stack", "--head", "HEAD") + self.assertEqual(code, 0, out) + self.assertIn("Track A", out) + + def test_quiet_prints_only_the_track(self) -> None: + self.stacked_feature("docs: explain a thing", {"README.md": "clearer\n"}) + code, out = run("track", "--base", "stack", "--head", "HEAD", "--quiet") + self.assertEqual(code, 0, out) + self.assertEqual(out.strip(), "B") + + def test_check_commit_reports_the_track(self) -> None: + self.stacked_feature("feat(drive): touch the sim", {"pufferlib/drive.c": "int x;\n"}) + code, out = run("check-commit", "--base", "stack") + self.assertEqual(code, 0, out) + self.assertIn("review track: A", out) + + def test_chore_mode_reports_track_b(self) -> None: + self.branch_from_stack("chore/x") + commit(self.repo, "chore(fork): adjust the process", {"CONTRIBUTING.md": "new\n"}) + code, out = run("check-commit", "--base", "stack", "--chore") + self.assertEqual(code, 0, out) + self.assertIn("review track: B", out) + + class TestPureHelpers(unittest.TestCase): def test_fork_only_paths_are_matched_exactly(self) -> None: self.assertTrue(forkctl.is_fork_only_path("CONTRIBUTING.md")) @@ -1250,6 +1438,19 @@ def test_diff_headers_are_not_treated_as_content(self) -> None: self.assertEqual(forkctl.diff_content_lines(diff), ["-old line", "+new line"]) self.assertEqual(forkctl.diff_content_lines(diff, added_only=True), ["+new line"]) + def test_simulator_paths_are_matched_by_prefix(self) -> None: + self.assertTrue(forkctl.is_sim_path("pufferlib/ocean/drive/drive.h")) + self.assertTrue(forkctl.is_sim_path("pufferl.py")) + self.assertTrue(forkctl.is_sim_path("config/puffer_drive.yaml")) + self.assertFalse(forkctl.is_sim_path("tests/drive/test_obs.py")) + self.assertFalse(forkctl.is_sim_path(".github/workflows/ci.yml")) + + def test_a_lookalike_prefix_is_not_a_simulator_path(self) -> None: + """Prefixes are anchored at a directory boundary, names match exactly.""" + self.assertFalse(forkctl.is_sim_path("docs/pufferlib/notes.c")) + self.assertFalse(forkctl.is_sim_path("pufferl.py.bak")) + self.assertFalse(forkctl.is_sim_path("configs/other.yaml")) + if __name__ == "__main__": unittest.main(verbosity=2) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index ff8f41343a..017da4a0eb 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,85 +1,33 @@ - + ## What - + ## Why - + ## How tested - - -- [ ] `pre-commit run --all-files` -- [ ] `make rebuild` (required if `.c`/`.h` changed — zero new warnings) -- [ ] `make test-unit` / `make test-eval` -- [ ] `make test-c` (C sim logic) -- [ ] `make test-notebooks` (notebooks touched) -- [ ] `make test-docker-smoke` (dynamics, rewards, observations, or training loop touched) -- [ ] `make -C tests/drive perf` (hot paths touched) -- [ ] `make test` before marking ready (unit + eval + c + notebooks — does **not** include - docker-smoke or perf; those are the two boxes above) + New behaviour is covered by a new test / the fix has a test that fails without it: - - - -## Stack hygiene - - - -- [ ] `forkctl check-commit` passes (`--chore` on a fork-only branch) -- [ ] Conventional Commits subject, rebased on current `integration/3.0` - - - -## Merge gate - - - -- [ ] **Gate 1 — CI green** on the final commit (no "green except a flake") -- [ ] **Gate 2 — adversarial review** posted by someone other than the author; findings fixed or - explicitly accepted in the thread -- [ ] **Gate 3 — upstream-readiness proven**: `forkctl mirror --sha HEAD` passes, i.e. the - commit cherry-picks onto pure `upstream/3.0` and its diff is upstream-clean - - -Gate 3 result: - - + ## Upstream disposition -- [ ] Mirror PR to `Emerge-Lab/PufferDrive` requested now — needs a human to verify the final - title, body and diff first, and the body must carry the authorship disclosure from - `.github/fork/PROCESS.md` → *Agent authorship* -- [ ] Contributable, but we hold it for now, because: -- [ ] **Never upstream** — fork-only `chore(fork):` change, Gate 3 exempt - - +- [ ] Mirror PR requested now — a human must verify title, body and diff first, and the body + carries the disclosure from `.github/fork/PROCESS.md` → *Agent authorship* +- [ ] Contributable, held for now because: +- [ ] **Never upstream** — fork-only `chore(fork):` change + ## Risk / notes - + diff --git a/.github/workflows/fork-gates.yml b/.github/workflows/fork-gates.yml new file mode 100644 index 0000000000..0e4470c532 --- /dev/null +++ b/.github/workflows/fork-gates.yml @@ -0,0 +1,68 @@ +# Fork-only. Never cherry-pick this file upstream. +# +# Runs the mechanical merge gates so nobody has to run them by hand or paste +# their output into a pull request. Gate 1 is the rest of CI; this job is +# check-commit plus Gate 3, reported like any other check. Text output, not +# annotations: this log is what a reviewer reads instead of a pasted block. +name: Fork gates + +on: + pull_request: + branches: + - 'integration/3.0' + +permissions: + contents: read + +jobs: + gates: + name: Fork gates + runs-on: ubuntu-latest + steps: + - name: Checkout the pull request head + uses: actions/checkout@v4 + with: + # The merge ref would put a synthetic merge commit at HEAD, and every + # gate reasons about the real commit the branch carries. + ref: ${{ github.event.pull_request.head.sha }} + fetch-depth: 0 + + # Checking out a SHA leaves HEAD detached, which `mirror` refuses: it has + # to know which branch to return you to. + - name: Put HEAD on a branch + run: git switch -c gate-head + + - name: Fetch upstream + run: | + git remote add upstream https://github.com/Emerge-Lab/PufferDrive.git + git fetch --no-tags --quiet upstream 3.0:refs/remotes/upstream/3.0 + + - name: Identify the change + id: id + run: | + branch='${{ github.event.pull_request.head.ref }}' + echo "slug=${branch#*/}" >> "$GITHUB_OUTPUT" + case "$(git log -1 --format=%s)" in + 'chore(fork):'*) echo "chore=true" >> "$GITHUB_OUTPUT" ;; + *) echo "chore=false" >> "$GITHUB_OUTPUT" ;; + esac + + - name: Review track + run: python .github/fork/forkctl.py track + + - name: check-commit + run: | + if [ '${{ steps.id.outputs.chore }}' = 'true' ]; then + python .github/fork/forkctl.py check-commit --chore + else + python .github/fork/forkctl.py check-commit + fi + + # A chore(fork) commit is fork-only by definition, so it has no upstream + # form to prove. Section 4.8 exempts it. + # + # always(): an author should get both gate results from one run, not fix + # check-commit and only then discover Gate 3 is red too. + - name: 'Gate 3: upstream-readiness' + if: always() && steps.id.outputs.chore == 'false' + run: python .github/fork/forkctl.py mirror '${{ steps.id.outputs.slug }}' --sha HEAD --no-publish diff --git a/.github/workflows/fork-only-view.yml b/.github/workflows/fork-only-view.yml index c8b848d9a2..ff67d66694 100644 --- a/.github/workflows/fork-only-view.yml +++ b/.github/workflows/fork-only-view.yml @@ -2,26 +2,11 @@ name: Fork-only view # Fork-only. Verifies that the `fork-only/3.0` view branch still matches the single # `chore(fork):` commit on `integration/3.0`, and that its standing draft PR is intact. +# It detects drift; it cannot publish, because GITHUB_TOKEN may not push workflow files. +# A stack that is behind upstream is reported by sync-mirror.yml, not here. # -# It deliberately does NOT publish the branch. The fork base is made almost entirely of -# files under `.github/workflows/`, and GitHub blocks `GITHUB_TOKEN` from pushing any -# commit that creates or updates a workflow file — a platform rule no `permissions:` -# block can lift. Publishing therefore happens by hand, in the two procedures that -# already force-push with human credentials (see .github/fork/PROCESS.md). This job is -# the detector that makes drift loud, and it needs no write access at all. -# -# The checks themselves live in .github/fork/forkctl.py so that CI, humans and agents -# run the same tested code. This file only decides *when* to run them. -# -# Residual trust, stated plainly. A workflow cannot be a tamper-proof monitor of the -# same repository that stores it: on `push`, GitHub runs the workflow *definition* from -# the pushed ref, so a tampered `fork-only/3.0` can neuter or delete this file before the -# trusted checkout above ever executes. The backstop is the `schedule:` trigger, which -# GitHub always runs from the **default branch** (`integration/3.0`) regardless of what -# any other branch contains -- so tampering is caught within a day even if the push-time -# run was suppressed. `integration/3.0` policing itself is the irreducible core: it is -# branch-protected, and section 3.3 / section 7 are the only procedures permitted to -# force-push it. +# Why it never publishes, and what it does and does not prove about tampering: +# .github/fork/PROCESS.md -> The standing draft PRs. # # Never cherry-pick this workflow upstream. diff --git a/.github/workflows/sync-mirror.yml b/.github/workflows/sync-mirror.yml index dafbece6a7..f1f94ba2df 100644 --- a/.github/workflows/sync-mirror.yml +++ b/.github/workflows/sync-mirror.yml @@ -81,17 +81,18 @@ jobs: if: steps.stack.outputs.needs_rebase == 'true' env: GH_TOKEN: ${{ github.token }} + GH_REPO: ${{ github.repository }} UPSTREAM_SHA: ${{ steps.sync.outputs.upstream_sha }} BEHIND: ${{ steps.stack.outputs.behind }} TITLE: Rebase integration/3.0 onto upstream 3.0 run: | - gh label create mirror-sync --color FBCA04 \ - --description "Integration stack is behind the upstream mirror" 2>/dev/null || true - { echo "The \`3.0\` mirror has advanced to \`$UPSTREAM_SHA\`." echo "\`integration/3.0\` is missing **$BEHIND** upstream commit(s)." echo + echo "This issue is the *only* alarm for an outstanding rebase. No workflow fails" + echo "while it is open: being behind upstream is a normal window, not a defect." + echo echo '```bash' echo "git fetch origin" echo "git switch integration/3.0" @@ -112,21 +113,36 @@ jobs: echo "Closes automatically once \`integration/3.0\` contains the mirror head." } > issue_body.md - existing=$(gh issue list --label mirror-sync --state open \ - --search "$TITLE in:title" --json number --jq '.[0].number // empty') + # Found by exact title, never by a label. A label is a second object that has + # to exist before the issue can be filed, and `gh label create ... || true` hid + # its own failure here while every run went red on the create that needed it. + # + # No --search: that hits the search API, whose index is eventually consistent, + # so a freshly filed issue is invisible to it. A plain list is strongly + # consistent. GH_REPO is set explicitly rather than inferred from the checkout. + open_issues=$(gh issue list --state open --limit 100 --json number,title) + echo "Open issues: $open_issues" + existing=$(printf '%s' "$open_issues" \ + | jq -r "[.[] | select(.title == env.TITLE)][0].number // empty") if [ -n "$existing" ]; then gh issue edit "$existing" --body-file issue_body.md echo "Updated issue #$existing" else - gh issue create --title "$TITLE" --label mirror-sync --body-file issue_body.md + gh issue create --title "$TITLE" --body-file issue_body.md fi - name: Close the tracking issue when the stack is current if: steps.stack.outputs.needs_rebase == 'false' env: GH_TOKEN: ${{ github.token }} + GH_REPO: ${{ github.repository }} + TITLE: Rebase integration/3.0 onto upstream 3.0 run: | - for n in $(gh issue list --label mirror-sync --state open --json number --jq '.[].number'); do + open_issues=$(gh issue list --state open --limit 100 --json number,title) + echo "Open issues: $open_issues" + for n in $(printf '%s' "$open_issues" \ + | jq -r "[.[] | select(.title == env.TITLE)][].number"); do gh issue close "$n" --comment "\`integration/3.0\` now contains the mirror head." + echo "Closed issue #$n" done diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7b13447f46..521d36d6f7 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -44,7 +44,9 @@ never commit to it.** Like the PR above it carries `do-not-merge` **alone**: the *together* are reserved for a PR that is actively rewriting the process, and a permanent PR wearing both would signal that forever. -When upstream advances, a bot opens a `mirror-sync` issue asking a maintainer to rebase the stack. +When upstream advances, a bot opens an issue titled `Rebase integration/3.0 onto upstream 3.0` +asking a maintainer to rebase the stack. Nothing fails while it is open — being behind upstream is +a normal window, not a defect. ## Working on a feature @@ -84,10 +86,19 @@ Opening the PR is not the finish line. `integration/3.0` is what BMW pins *and* later feature rebases onto, so a defect merged there is multiplied. Three gates, all required: 1. **Green CI** on the final commit — not "green except a flake", not "green before the last push". -2. **Adversarial review** by someone other than the author, who tries to *break* the change: revert - the fix and prove the new test fails, hunt edge cases, check determinism, check observation and - config layout, check hot-path cost, check scope creep. "LGTM" is not a review. The findings go in - the PR thread, including the probes that found nothing. +2. **Adversarial review** by someone other than the author. How much review depends on what the + change can break — `forkctl track` decides: + - **Track A** (`pufferlib/`, `config/`, `pufferl.py`, `models.py`, `setup.py`, smoke goldens): + the reviewer tries to *break* the change — revert the fix and prove the new test fails, hunt + edge cases, check determinism, observation and config layout, hot-path cost, scope creep. + "LGTM" is not a review. Findings go in the thread, including the probes that found nothing. + - **Track B** (everything else — docs, tooling, workflows, CI, tests): one human read answering + three questions — does it do what it says, can it break anything outside its own files, is it + the simplest version. No written probe list. + + Either way, **nits are a process bug, not a review comment.** Formatting, import order, line + length and wording belong to `pre-commit`; commit shape and contamination belong to `forkctl`. + If one reaches review, fix the tool instead of annotating the instance. 3. **Proven upstream-readiness** — the commit is cherry-picked onto a branch cut from pure `upstream/3.0` *before* merging. It must apply without conflict and produce an upstream-clean diff. We stack only commits we could contribute today. (`chore(fork):` commits are exempt from @@ -121,6 +132,11 @@ table says so. If your change moves the smoke goldens, regenerate them with `tests/smoke_tests/update_goldens.sh` and stage both JSON files in the same commit (the rollout golden is gitignored and needs `git add -f`). +A **Track B** change — one `forkctl track` reports as touching no simulator path — needs only +`pre-commit`, plus whatever directly exercises what it changed (its own tests, a workflow +dispatch). Running the simulator suites to prove a Markdown edit is safe costs twelve minutes and +proves nothing; CI runs them anyway on the standing stack PR. + New behaviour needs a new test; a bug fix needs a test that fails without the fix. List the commands you ran in the PR. From 544c74987f960d7ffe53cba4d4ae2ae5dd31cf61 Mon Sep 17 00:00:00 2001 From: Carlo van Driesten Date: Wed, 11 Nov 2026 14:19:37 +0100 Subject: [PATCH 2/2] feat(drive): probe the feature path of the fork gates check --- probe_scratch.txt | 1 + 1 file changed, 1 insertion(+) create mode 100644 probe_scratch.txt diff --git a/probe_scratch.txt b/probe_scratch.txt new file mode 100644 index 0000000000..da0c4eb8d9 --- /dev/null +++ b/probe_scratch.txt @@ -0,0 +1 @@ +probe