godot_build.py: hold presubmit.py/presubmit_test.py out of f5/integration — they're never in the playtest chain nf-bxrv ← Beads

closed priority 2 task unassigned

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

Aedan, verbatim (2026-08-18), reacting to a dev's comment on PR #1073 ("Pushed fine, but the f5 integration hits a fresh presubmit.py conflict (new conflict text now that my branch carries main's new checks). Resolving in the f5 worktree again"):

> "I wonder why a tooling only PR is spending any cycles at all navigating f5 > issues, I thought the f5 script was meant to be skipped for non-godot PRs"

Context

The mechanism Aedan is remembering is real and does exactly what he expects — godot_inert in tools/nflib/godot_build.py:113, wired into rebuild_f5.py's hold_out_inert (#599, criterion set on #604: *"if the code change doesn't affect the Godot build, it doesn't need to go in F5"*). A branch classified inert never enters the f5/integration merge loop at all — it cannot conflict, by construction.

PR #1073 is exactly the kind of PR that criterion is meant to hold out: no games/**, no shared/**. Its files are CLAUDE.md, notes/*.md, presubmit.py, presubmit_test.py, and four files under tools/. Everything in that list is already inert **except presubmit.py and presubmit_test.py** — GODOT_INERT_PREFIXES/SUFFIXES cover .md, design_docs/, notes/, .claude/, tools/, but there is no root-file carve-out for the CI gate script, so those two files alone were enough to classify the whole branch as build-affecting, merge it into f5/integration, and produce the presubmit.py conflict the dev reported.

This is not a gap in the mechanism — it's a *documented, deliberate* choice, made without presubmit.py-specific justification. godot_build_test.py:164's test_a_ROOT_python_tool_is_NOT_inert lumps presubmit.py in with f5.py, probe.py and run_probes.py under one rationale: *"f5.py is the thing that performs the playtest, so a branch changing it changes what happens when Aedan presses F5."* That's a real reason for f5.py (arguably) and for probe.py/run_probes.py (they run the probe suite against the built game). It says nothing about presubmit.py, and grepping every caller confirms why: presubmit.py is invoked only from merge_pr.run_presubmit and dev.py check/pr/push (tools/pr/merge_pr.py:370, tools/nflib/devcli.py:512). Neither f5.py nor rebuild_f5.py's own verify() ever shells out to it. It is also never a compile candidate — SDK Compile globs are .cs-only, Ninefold.csproj has no <Compile Remove> for it (it doesn't need one), and there is no presubmit.py.import sidecar anywhere in the repo (find . -name "*.py.import" — none), confirming Godot's importer never touches it either. By the same "does the code change affect the Godot build" criterion #604 set, presubmit.py is provably inert — it's the one root .py file in the lumped-together test case that has no execution path into anything ./f5.py builds, imports or shows, and it is also the single file most PRs touch (every tooling PR that adds or changes a gate check edits it), which is why it is the recurring conflict source rather than a one-off.

Prescription

Extend godot_inert (tools/nflib/godot_build.py) with a new exact-path set, alongside the existing GODOT_INERT_PREFIXES / _SUFFIXES / _SEGMENTS (B10 — reuse the existing mechanism rather than inventing a second one):

```python GODOT_INERT_PATHS = frozenset({"presubmit.py", "presubmit_test.py"}) ```

- Check it first in godot_inert(), same shape as the other checks: if path in GODOT_INERT_PATHS: return True. - Add it to __all__. - Extend the big comment block above GODOT_INERT_SUFFIXES (the one that derives every entry from something checked in) with an entry for these two files, citing: never invoked by f5.py/rebuild_f5.py, never a <Compile> candidate, no .py.import sidecar. Explicitly note *why this doesn't widen to f5.py/probe.py/run_probes.py/dev.py* — those have a real execution path into what a playtest exercises (or in f5.py's case, an existing test asserting the opposite deliberately) — so a future reader doesn't read this change as license to blanket-exempt every root .py file (B7: this closes the presubmit.py failure mode specifically, not a whole category by accident). - Update godot_build_test.py:164's test_a_ROOT_python_tool_is_NOT_inert — drop presubmit.py from that list (keep f5.py, probe.py, run_probes.py, whose rationale still holds) — and add a new test asserting godot_inert("presubmit.py") and godot_inert("presubmit_test.py") are both True, with a docstring citing this issue and the "never invoked by the playtest chain" evidence above so the next reader doesn't have to re-derive it. - Update notes/f5.md's "Held out" bullet (the `*.md anywhere, design_docs/, notes/, .claude/, tools/, installer/, art_dump/, and any tests/ directory` line) to add presubmit.py/presubmit_test.py, and note the f5.py/probe.py/run_probes.py root scripts remain build-visible on purpose, so the "root scripts stay in" framing doesn't read as a blanket rule that got quietly broken.

No changes needed to BuildPremiseTest in godot_build_test.py — that suite guards the .cs-compile and .gdignore premises behind the prefix/segment lists; an exact-path set for two .py files that were never compile candidates in the first place doesn't touch what it asserts.

Test plan

- tools/nflib/godot_build_test.py's GodotInertTest — the edited test_a_ROOT_python_tool_is_NOT_inert plus the new inert-path test, both via the python lane (./presubmit.py --all / `python -m unittest tools.nflib.godot_build_test`). - No probe or C# test needed — this is a pure-Python path predicate with no Godot-facing surface.

PR shape

One PR — a two-string addition to a data set plus its tests and one notes update, no behaviour split needed (E3).

Non-goals

- Widening this to f5.py, probe.py, run_probes.py, dev.py, or any other root .py/.sh file. Those have a stated (even if only partially examined) execution-path rationale for staying build-visible; re-litigating them is a separate question, not this one. - Touching GODOT_INERT_SUFFIXES to add a blanket .py/.sh rule — that would also inert files like games/tonapse/samples/gen_sampling_midi.py and, more importantly, run_tests.sh, which rebuild_f5.py's own verify() actually executes out of the integration worktree (tools/pr/rebuild_f5.py:1396) — a real behavioural difference from presubmit.py, which nothing in the f5 chain runs. Worth a separate, more careful pass if it recurs; not folded in here.

Open questions (each with a recommended default)

- **Should presubmit_test.py really ride along, or is presubmit.py alone enough to kill the observed conflict?** Recommended default: include both — they're edited together whenever a gate check gains a test, and the same "never in the playtest chain" argument applies to both with equal force.

Dependencies

None.

Comments

No comments.

Add a comment