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