rebuild_f5: fast verify when nothing Godot-visible changed; dev.py pr/merge run the rebuild nf-vvhx ← Beads

closed priority 2 task unassigned activesession:f5-fast-verify

Twinned from https://github.com/cfsh/ninefold/issues/984 by tools/beads/import_github.py's reconcile pass.

Problem

Loop step 4 tells every session to rebuild f5 after every dev.py pr and after every merge. For a tooling PR — the majority case; affected_probes.py's docstring cites the #847 measurement that 43% of merges are Godot-invisible and most of the rest touch only tools/ — that rebuild changes **nothing Aedan plays**, yet still pays the full verify phase in rebuild_f5.py: `dotnet build Ninefold.sln` + the whole xUnit suite + a headless Godot import. That is 10–12s on an idle box and minutes when several sessions compete — and tooling-heavy stretches are exactly when they do.

The result is that the decision "does *my* change need an f5 rebuild?" lives in each session's head, steered only by .md guidance. Sessions either pay the minutes pointlessly or quietly skip the step and drift.

What NOT to do: skip the rebuild for tooling PRs

The obvious reading of this issue — "only rebuild for games changes" — is wrong as an implementation, and rejecting it is part of the spec:

f5/integration is *defined* as origin/main + every open PR head. If a tooling merge advances main and nobody rebuilds:

* f5.py's warn_if_behind_main asks whether origin/main is an ancestor of the branch. It is not, so Aedan gets a false BEHIND main warning on every launch. * rebuild_f5.py --health (f5_status) counts *every* commit main has that f5 lacks, so it cries STALE-F5 forever — and poll.py surfaces that to the f5/integration owner as an outage. * The -f5 worktree's *tools* go stale, and f5.py itself lives on that branch.

Teaching all three consumers the Godot-visible distinction is strictly more surface (and more drift risk) than making the rebuild itself cheap. **Keep the rebuild unconditional; make the expensive part conditional.**

Design

Reuse design 005's classifier — nflib.godot_build.changed_paths + godot_inert — exactly as probe_gate's #637 fast-skip and rebuild_f5's own #599 hold-out already do. Do not write a new path classifier; the whole point of godot_build.py is that there is one, and BuildPremiseTest pins it to what Ninefold.csproj and .gdignore actually say.

T1 — rebuild_f5.py: fast verify when nothing Godot-visible changed

In integrate(), after the merge loop and the provenance stamp, immediately before verify(tree_path, runner):

```python visible = changed_paths(tree, "origin/f5/integration", "HEAD") ```

* Two shas, tree against tree — the same contract as affected_probes.py and probe_gate.sweep_input_changed, and for the same reason: three dots is merge-base-relative and would hide what the old integration carried and the new one does not. * Filter with godot_inert. If every changed path is inert → run the **fast verify**: dotnet build only, skipping the test suite and the headless import. Print the decision loudly, e.g. → Verify (fast): nothing Godot-visible changed since <short-sha> — build only. * Otherwise (any visible path, changed_paths returned None, or origin/f5/integration does not exist / is unreadable) → the full verify, unchanged. Every ambiguity fails toward verifying, mirroring affected_probes.py's unknown-diff rule: "I could not tell" must never become "don't check". * Add --full-verify to force the full phase regardless (the RUN_PROBE_GATE=1 idea, as a flag).

Why each piece of the split is sound:

* Headless import reads only what the Godot build can see — that is godot_inert's literal definition — so an unchanged visible tree cannot change its outcome. Safe to skip. * dotnet build is kept even on the fast path. Ninefold.sln includes tests/Ninefold.Tests.csproj, which cross-links tools/tests/*.cs and pure shared logic — all Godot-*inert* paths — so a compile-level semantic conflict between a fresh tooling commit on main and an open mixed branch is exactly what the diff check cannot rule out. The build is also what f5.py runs at Aedan's desk; keeping it guarantees his F5 never hits a compile error we chose not to look at. It is incremental in the reused -f5 worktree, so it is the cheap third of the phase. * The test suite is skipped on the fast path. In principle inert files can break tests, but every inert delta in the integration arrived through a presubmit-gated PR or presubmit-gated main; the only unverified object is the *combination*, a test-level (not compile-level) conflict between two green tooling changes with zero game-file movement. The blast radius is a test suite Aedan never runs during a playtest, and the next visible rebuild re-runs everything. If the reviewer or Aedan judges even that too loose, the fallback is "fast = build + tests, skip import only" — still saves the Godot launch — but implement the build-only version first.

Transitivity note for the docstring: the comparison is against the last *pushed* integration, whose own verify may itself have been fast. That is sound because each fast push asserted visible-tree equality with its predecessor, so equality chains back to the last fully-verified build — and the build check ran on every link.

Tests (in tools/tests/test_rebuild_f5.py, same seams the suite already uses — runner injection, scripted Git):

* Inert-only diff → run_tests.sh and the Godot import are not invoked; dotnet build is; push still happens; the fast-path line is printed. * One visible path among many inert → full verify. * changed_pathsNone (scripted git failure) → full verify. * No origin/f5/integration ref (first-ever build) → full verify. * --full-verify → full verify on an inert diff.

T2 — the frontier-changing verbs run the rebuild themselves

The manual loop-step-4 is the part that makes sessions *feel* the decision is theirs. Move it into the front door (design 006). f5/integration is main + every open PR head, so it changes on exactly three session-driven events, and each one's verb ends by invoking ./tools/pr/rebuild_f5.py — foreground, echoed like every other process a verb runs:

* dev.py pr — a new head enters the frontier. * dev.py merge — main advances, a head leaves. * dev.py push (new verb) — an existing head *moves*. This is the iterate-on-feedback / prototyping case ("move that thing a bit to the right", pre-review, Aedan playtesting each round via ./f5.py): today that loop is edit → commit → raw git push, which touches no verb, so hooking only pr and merge would leave every feedback round invisible to f5 — and worse, teach sessions that rebuilds are automatic right as the one remaining manual case becomes the common one. The verb: **gate, push, rebuild** — run the gate through devcli's existing one-spelling-of-it helper (presubmit.py --all, streamed), record the result in the presubmit ledger exactly as dev.py check does, push the current branch (refuse on main), then the same rebuild tail as the other two. notes/pair-programming.md's mechanics list ("commit, open PRs, …, rebuild f5") switches to naming dev.py push as the iterate step.

Why the gate is in the verb (this is presubmit disappearing behind the front door, same move as the rebuild):

* The presubmit ledger stays true for every head. _record_presubmit is what merge_pr.py --why reads; a head pushed with raw git push reports "presubmit: UNKNOWN — no run recorded" forever. A gating push verb is the only way dev.py status stays honest during iteration. * In a rev:off pairing worktree, merge_pr.py is not used (Aedan merges from the UI), so its --all never runs — push-time is the only automated gate on that path. * A red gate refuses the push (and therefore the rebuild), same posture as pr. Escape hatch: --ungated, loudly named, for vibing rounds where Aedan has said speed beats the suite; it skips gate *and* ledger record, so --why honestly reports no run.

⚠ Do not re-introduce path→suite routing to make the push gate cheaper. presubmit.py's plan() docstring records the #458 measurement and the agreed removal; the gate is --all or skipped-loudly, nothing between. And do not skip the rebuild's own verify because the gate just passed — the gate checks the branch tree, the verify checks the merged integration; they are different objects and both stay.

Note the fast verify (T1) deliberately does not fire for that prototyping loop — a game edit is Godot-visible, so each round pays the full verify, as it must: that build is the thing Aedan is about to play. If iteration latency under load turns out to matter, a --build-only escape for vibing rounds is a possible follow-up, but it is a policy call (publishing an f5 the tests never saw) — file it separately if wanted; do not fold it in here.

* Unconditional. No path inspection in devcli — the decision about what the rebuild costs lives in rebuild_f5.py (T1), in one place. After a tooling PR/merge the invocation is seconds; after a games one it was required anyway. * Lock contention is already handled: rebuild() stands down with exit 0 when another session holds the publish lock, precisely so callers chaining it don't escalate routine contention. A rebuild failure should be *reported* by the verb but must not turn a completed merge into a failed verb — the merge happened; say the rebuild didn't, and print the command to re-run. * Respect the existing separation: merge_pr.py itself stays rebuild-free (its _gate_worktree docstring explains the backgrounded-gate collision with <topic>-f5; the *foreground* invocation after merge_pr.py returns, from devcli, has no such overlap — note that in a comment). * Extend the devcli contract tests the same way the merge verb's merge_pr.py invocation is pinned today.

T3 — docs follow the code

* CLAUDE.md loop step 4 shrinks from an instruction to a description: `dev.py pr/merge` rebuild f5 for you; it is cheap when your change is Godot-inert. Do not grow the file — this should be a net wording swap. * notes/merging.md / notes/operating.md: same adjustment where they narrate the manual step; keep --health guidance as-is (its semantics are deliberately untouched by this issue). * notes/pair-programming.md: the iterate loop pushes with dev.py push, so every feedback round Aedan playtests is in f5 by construction. * Mind tools/tests/test_doc_pointers.py — pointer phrases in the touched docstrings are checked.

Non-goals

* No change to which *branches* enter the frontier — #599's hold-out already covers that and is untouched. * No change to f5_status / --health / warn_if_behind_main semantics — the design keeps their invariant ("f5 contains main") true instead of teaching them exceptions. * No probe-selection changes — design 005b already owns that side.

Suggested stacking

PR 1: T1 (rebuild_f5 fast verify + tests). PR 2: T2 + T3 (devcli invocations, contract tests, doc wording). PR 2 stacks on PR 1 so the auto-invocation never lands without the thing that makes it cheap.

Dependencies

None.

Comments

No comments.

Add a comment