diff --git a/.agents/issue-index.md b/.agents/issue-index.md index 3b529d2b3..90c07ad1b 100644 --- a/.agents/issue-index.md +++ b/.agents/issue-index.md @@ -269,4 +269,7 @@ rather than merged. `scripts/check-agent-record.py` gates both. | [#986](https://github.com/mudler/vllm.cpp/issues/986) | `LTX25-DFR-PIPELINE` | `DFRPipeline` (`dfr_pipeline.py` + `dfr_layout.py` @ `fd4ded7f`) has no representation in this tree — `git grep -i dfr` over `src include tests examples docs` returns ZERO product hits, against 87 `ltx2` hits in `ltx2.cpp` alone as the positive control ([#604](https://github.com/mudler/vllm.cpp/issues/604)). It matters beyond its own feature because it is the **ONLY** upstream consumer of the temporal x2 latent upsampler this project already ships: [`ltx25-temporal-upsampler.md`](specs/ltx25-temporal-upsampler.md) section 7 records the operator as ported, loader-parsed and gated but *"not reachable from any shipped pipeline"*, and `docs/FEATURES.md` carries that as `Temporal x2 ups gated, UNDRIVEN`. No issue tracked that state — `undriven` returned zero hits across open and closed issues. DFR also needs the generated-keyframe-slot READBACK that [#920](https://github.com/mudler/vllm.cpp/issues/920) refused by name and left owed after it CLOSED, so the debt had a spec bullet and no open issue; it needs the LAYOUT and the EXTRACTION but **not** the standalone single-frame decode, because DFR never decodes its slots — it hands them to the spatial upsampler (`dfr_pipeline.py:348`) and feeds them back as `initial_keyframes` (`:364`). Spec [`ltx25-dfr-pipeline.md`](specs/ltx25-dfr-pipeline.md). Campaign [#644](https://github.com/mudler/vllm.cpp/issues/644) | feature | | [#995](https://github.com/mudler/vllm.cpp/issues/995) | `ENG-EXPERT-STREAM` | `check-env-doc` and `test_check_env_doc` were RED on `origin/main`, so every branch cut from it inherited a preflight failure its own diff did not cause: `VT_MOE_EXPERT_STREAM`, `VT_MOE_EXPERT_STREAM_SLOTS` and `VT_MOE_EXPERT_STREAM_SLOT_BYTES` are read from `src/vllm/model_executor/models/qwen3_5.cpp` (`:5145`, `:5195`, `:5189` @ `4496ef196`) and appeared in neither `docs/ENVIRONMENT.md` (`grep -c` returned **0**) nor `scripts/env-doc-allowlist.txt`. They arrived with the `ENG-EXPERT-STREAM` W4 wiring commit `3005447f8` ([#993](https://github.com/mudler/vllm.cpp/pull/993)). Found while gating [#986](https://github.com/mudler/vllm.cpp/issues/986) and proved pre-existing with a matched-arm check rather than asserted: the three sites are in a file that branch does not touch, and `git diff origin/main...HEAD \| grep '^+.*VT_MOE_EXPERT'` returns nothing. It is a PRE-FLIGHT gate, so it failed before every edit and presented to each author in turn as a red their own diff caused -- the shape [#965](https://github.com/mudler/vllm.cpp/issues/965) and [#968](https://github.com/mudler/vllm.cpp/issues/968) both took. The discoverer deliberately did NOT fix it in flow: documenting a knob means stating its default and when to touch it, and the expert-streamer's slot accounting belongs to the row that added it, so a plausible-sounding entry written by a passer-by is how `docs/ENVIRONMENT.md` stops being trustworthy. FIXED in [#997](https://github.com/mudler/vllm.cpp/pull/997) by DOCUMENTING all three in `docs/ENVIRONMENT.md`, not by allowlisting them: a knob that changes the host memory a MoE decode reserves and disables the default-on grouped-MoE path is a deployment surface, not a kernel-internal micro-tuning switch | bug | | [#987](https://github.com/mudler/vllm.cpp/issues/987) | `LTX25-RETAKE` | Two `ltx-2.5` refusal messages state reasons that are no longer true. (a) `src/vllm/multimodal/ltx2_video.cpp:1608 @ 0e1bee42f` says "nothing reads `ref_video_dir` at all", and MiniMax-H3 has always consumed the directory in full (`ReadReferenceClipChw`, `src/vllm/multimodal/minimax_h3_video.cpp:135 @ 0e1bee42f`, called at `:650`); [#975](https://github.com/mudler/vllm.cpp/issues/975) inherited the wider claim from this message. The claim that holds is narrower: the LTX-2.5 engine never reads the directory's CONTENTS. (b) `ltx2_video.cpp:1636-1638 @ 0e1bee42f` says "there is no AUDIO_VAE_ENCODER key filter", and `c2019b0e3` landed `Ltx2AudioVaeEncoderKeyRules()` (`include/vllm/model_executor/models/ltx2_audio_input.h:73 @ 0e1bee42f`) with a live call through `Ltx2EncodeAudioToLatent`. Both rewritten in the `WHAT IS *NOT* THE REASON` shape in the same flow, with one assertion tied to the LOCAL fact that the LTX side now reads the directory | bug | +| [#998](https://github.com/mudler/vllm.cpp/issues/998) | `GATE-PREFLIGHT-SKIP-REPORT` | `scripts/agent-preflight.sh` prints `All gates green.` while a guarded block never ran: the trailer block is gated on `git merge-base --is-ancestor origin/main HEAD`, and when that is false the two gates vanish from the report with no output. Fired three times in one session, twice because `origin/main` is a remote-tracking ref every worktree of the checkout shares and another worktree advanced it MID-RUN, so the `ok` count fell from 76 to 74 and the banner did not change. Same shape in the `Committed range` block, whose unresolvable-ref arm drops three more gates. A green banner over a block that never executed is a FALSE REPORT, which the file's own `audit-live-rows` comment already calls "the one unacceptable outcome", spec [`gate-preflight-skip-report.md`](specs/gate-preflight-skip-report.md) | bug | +| [#999](https://github.com/mudler/vllm.cpp/issues/999) | — | `scripts/check-commit-style.py` `validate_range` still raises `range base must be an ancestor of range head`, so the merge-base repair `GATE-FORK-ANCESTRY` (#773) applied to `check-commit-trailers.py` never reached it: the checker aborts before reading a commit on any branch cut before the last merge of `main`. Found auditing the guards for #998, and it is why that row reports a SKIP rather than dropping the guard and letting both checkers run. Owed by [`gate-preflight-skip-report.md`](specs/gate-preflight-skip-report.md) | bug | +| [#1000](https://github.com/mudler/vllm.cpp/issues/1000) | `ENG-EXPERT-STREAM` | `main` is RED on `check-env-doc` at `3ce1cf7c7`: `VT_MOE_EXPERT_STREAM`, `VT_MOE_EXPERT_STREAM_SLOTS` and `VT_MOE_EXPERT_STREAM_SLOT_BYTES` arrived with `3005447f8` (#993) and are neither in `docs/ENVIRONMENT.md` nor on `scripts/env-doc-allowlist.txt`, so `check-env-doc` and `test_check_env_doc` fail on every branch cut from current main. Measured in a clean worktree at that SHA with no local edits. Found running the preflight as the gate for #998, and NOT repaired there: choosing documented knob versus internal tuning switch for each var belongs to the row that added them | bug | | [#1023](https://github.com/mudler/vllm.cpp/issues/1023) | `ENG-EXPERT-STREAM` | The IQ1_S (ggml 19) / IQ1_XXXS (ggml 66) decode landed in [#946](https://github.com/mudler/vllm.cpp/pull/946) with every parameter EXCEPT the grid pinned only by self-consistency. The grid seal (FNV-1a digest + lane census) works and stops one table short: `ReferenceDotF64` (`tests/vt/test_ops_quant_dot.cpp:231-236`) and the G3 NMSE reference (`:915`) both decode the weight with `vt::cpu::BlockToFloat`, the function under test, so they are independent only in the SUMMATION. Three injected defects, each applied and compiled, left the suite green with an UNCHANGED assertion count: `kIq1sDelta` `0.125F`->`0.25F` (`cpu_quant_iq_tables.h:422`, affects BOTH encodings), the IQ1_S delta sign inverted in dequant AND vec_dot, and the IQ1_S scale read from `qh` bits 13-15 instead of 12-14 in both paths. Second defect, same PR: `gguf_dequant.cpp:107-116` lists no `case 19` and no `case 66`, so the expansion path throws `unsupported ggml type` for the two encodings the target checkpoints are 96.92 % made of, and `RouteGgufTensor` (`gguf_keep_quant.cpp:122`) sends a tensor there whenever `VT_CPU_REF` is on, keep-quant is off, K is ragged, or the role is not verbatim — a refusal to load on the reference lane. **ggml 18 (IQ3_XXS) carried the same omission**, pre-existing since the DeepSeek-V4 UD-IQ2_XXS port, and all three are one shared `switch` branch. Repaired by golden vectors whose EXPECTED values come from the ORACLES themselves (`ggml_get_type_traits(type)->to_float` built from `ggml-org/llama.cpp @ 237ad9b96` for 18/19 and `unslothai/llama.cpp @ 36fe8e1cc` for 66) over REAL checkpoint bytes, which is the first reference for this encoding family that is not this tree's own decoder. Also: the `2e-3` NMSE ceiling passed the doubled-delta defect that `6e-4` fails (measured 5.240e-4 unmutated, 6.967e-4 mutated on `iq1_s`), and on `iq1_xxxs` that defect moves the statistic the WRONG WAY (3.109e-4 -> 1.420e-4), so no ceiling can catch it and only the goldens can; and both new census cases claimed TOTAL coverage of 1702 records while summing to 864, F32's 838 tensors omitted. Spec [`expert-streaming.md`](specs/expert-streaming.md) | bug | diff --git a/.agents/specs/gate-preflight-skip-report.md b/.agents/specs/gate-preflight-skip-report.md new file mode 100644 index 000000000..7c1668208 --- /dev/null +++ b/.agents/specs/gate-preflight-skip-report.md @@ -0,0 +1,1008 @@ +# GATE-PREFLIGHT-SKIP-REPORT: a skipped block reports SKIP, and a run with a skip is not green + +**Row:** `GATE-PREFLIGHT-SKIP-REPORT` +**Issue:** [#998](https://github.com/mudler/vllm.cpp/issues/998) +**Base:** `origin/main` `3ce1cf7c7` +**Status:** ACTIVE, 2026-08-16 + +## 1. Scope + +`scripts/agent-preflight.sh` gains a third per-gate state and a pinned base +revision: + +- A block that cannot run prints `SKIP` with the reason that stopped it. +- The summary prints `All gates green.` only when nothing was skipped and + nothing failed. +- The two blocks that compare against `origin/main` resolve that ref once, at + the start of the run, and the run names the resolved SHA. +- An opt-in `--fail-on-skip` carries the third state into the exit status, for + the callers that can read only the exit status. The default is unchanged. + +**In scope.** The two range blocks in `scripts/agent-preflight.sh`, the new +`skip` reporting path, the summary, the `--fail-on-skip` flag and its one +production caller in `scripts/agent-ready.py`, and a new executable suite +`tests/scripts/test_agent_preflight_skip_report.py` that runs the script and +reads its report. + +**Out of scope.** Every checker the script calls. This change alters *what the +script reports about itself*, never *what any checker demands*. No checker +gains or loses an assertion. + +Also out of scope, and recorded under `## Owed`: `scripts/check-commit-style.py` +never got the merge base repair that `GATE-FORK-ANCESTRY` (#773) applied to +`scripts/check-commit-trailers.py`. See §4. + +## 2. Anchors + +Local governance script. There is no vLLM counterpart. + +| What | Where | +|---|---| +| The block that skips silently | `scripts/agent-preflight.sh`, `Commit trailers vs origin/main` | +| The block that shares the shape | `scripts/agent-preflight.sh`, `Committed range vs origin/main` | +| The banner a skip survives today | `scripts/agent-preflight.sh`, `echo "All gates green."` | +| The principle already settled in tree | `scripts/agent-preflight.sh`, the comment above `run "audit-live-rows"` | +| The in-tree precedent for an honest partial run | `scripts/agent-preflight.sh`, the `ROLE_ONLY` branch | +| The one consumer that read the exit status as a verdict | `scripts/agent-ready.py` `run_local_preflight` | +| The consumer that inherits that verdict | `scripts/agent-integration.py` `run_ready` | +| The consumer that requires exit 0 over five skips | `scripts/check-test-registration.py` `_preflight_execution_errors` | +| The checker that already merge-bases | `scripts/check-commit-trailers.py` `validate_range` | +| The checker that does not | `scripts/check-commit-style.py` `validate_range` | + +## 3. Design + +### 3.1 The defect + +The trailer block is guarded by +`git merge-base --is-ancestor origin/main HEAD`. When that guard is false the +block produces no output at all, and control reaches `echo "All gates green."` +with exit 0. Two gates, `commit-trailers` and `commit-style`, leave the report +without a word. + +The file already states the rule this violates, in the comment above +`run "audit-live-rows"`: + +> A skip would have to survive the "All gates green." banner below, and a green +> preflight that never verified the record is the one unacceptable outcome + +That comment argues the point for one block and then leaves the next two blocks +carrying the defect it names. The principle is settled. The reporting is not. + +### 3.2 Why the guard is false so often + +`origin/main` is a remote-tracking ref. Every linked worktree of one checkout +shares it. A fetch in any other worktree moves it, so the guard can be true when +the run starts and false when line 226 evaluates it. Three occurrences in one +session: + +| Occurrence | Branch | What the run printed | Why the guard was false | +|---|---|---|---| +| 1 | `row/FIX-SERVER-CONCURRENCY-931` | `74 ok, 0 FAIL, All gates green.` | the branch was five commits behind `main` | +| 2 | same, after merging `origin/main` | `All gates green.` again | `origin/main` advanced during the run | +| 3 | `row/BENCH-QWEN38-27B-FOUR-WAY` | `All gates green.` with 74 ok, not 76 | `main` moved during the run | + +Occurrence 3 is the proof that this is not a one-off. The count falls by exactly +two and the banner does not change, so nothing in the output distinguishes a run +that checked the trailers from a run that did not. + +### 3.3 The three parts of the fix + +**Pin the base once.** The script resolves `origin/main` to a SHA before the +first gate runs, stores it in `BASE_SHA`, and uses that SHA everywhere it used +the ref. A concurrent fetch can no longer change the answer mid-run. The run +prints the pinned SHA, so a reader knows what the range blocks compared against. +This alone removes occurrences 2 and 3, because the pinned SHA was an ancestor +at the moment it was captured. + +**Report a skip as a skip.** A `skip` function mirrors `run`. It prints a yellow +`SKIP` line, prints the reason on the following line, and appends the label to a +`skipped` array. `SKIP` is a third state. It is not `ok`, because nothing was +verified. It is not `FAIL`, because nothing was found wrong. + +**Never call a run with a skip green.** The summary prints the skipped labels +and the line `NOT a green preflight` whenever `skipped` is non-empty. The banner +`All gates green.` is reachable only when both arrays are empty. + +### 3.4 Exit code: 0 by default, 1 under `--fail-on-skip`, never the banner + +The run exits 0 when a block was skipped and nothing failed. Three things in the +tree settle this. + +1. `AGENTS.md` on gate design: "A gate that fires on ordinary work is the + defect, not the discipline." A branch behind `main` is ordinary. Every helper + worktree is behind `main` within minutes of being cut. A preflight that exits + 1 there would be red on most first runs of most sessions, and an agent would + read that red as a broken record rather than as a missing merge. +2. `AGENTS.md` on unknown state: "Unknown is not absence or success". A `SKIP` + line with a reason records the unknown. Exit 0 with a banner claims success. + Exit 0 without a banner, with the unknown named, is the accurate report. The + `audit-live-rows` comment objects to a skip *surviving the banner*, not to a + skip existing, and after this change no skip survives it. +3. Nothing enforces anything on this exit status by itself. `AGENTS.md`: hooks + and local runs are "bypassable convenience, not evidence". A hand-chained + `scripts/agent-preflight.sh && git push` is exactly that convenience, and CI + runs `check-commit-trailers.py` over the pull request range whatever this + script returned. + +Exit 1 keeps its meaning: a gate ran and failed. Widening exit 1 to cover "a +gate did not run" would merge two different facts into one signal, which is the +conflation this row exists to remove. + +**`--role-only` is not a precedent here, and the first version of this section +said it was.** The reviewer of this row rejected that argument and was right. +`--role-only` is a narrowing the caller *asked for*, so its caller knows exactly +what will not run before the run starts. A `SKIP` is imposed on a caller who +asked for a full run and finds out afterwards, and only if it reads the report. +The two cases share what an honest partial run should *print*, which is where +`--role-only` really is the model. They share nothing about what an unrequested +partial run should *return*. The three arguments above stand without it. + +### 3.4.1 Who reads the exit status + +§3.5 enumerated every conditional in the script and never asked the other +question, which is who consumes the number the script returns. There are five +consumers, and exactly one of them treated that number as a verdict about the +tree. + +The fifth was missing from the first version of this table, which listed four. +A census that stops when it has found the defect is not a census, and this row's +whole subject is a report that omits what it did not examine. + +The table carries a sixth row that is not a sixth consumer. This row's own suite +reads `returncode` too, and it is listed so that a reader who greps for that +field finds every match accounted for rather than one match unexplained. It +defines the contract instead of consuming it, and the verdict column says so. + +| Consumer | How it reads preflight | Verdict | +|---|---|---| +| `scripts/agent-ready.py` `run_local_preflight` | `returncode == 0`, nothing else | **The defect.** `AGENTS.md` names it the gate to run "before remote handoff". The `SKIP` lines, the `N gate(s) SKIPPED` summary and the missing banner are all invisible to it, so a behind branch reached `READY: local and live PR/CI evidence are green` with two gates never run, and an unresolvable base reached it with five. The word "green" printed over a trailer check that had not executed. Fixed by passing `--fail-on-skip`. | +| `scripts/agent-integration.py` `run_ready` | the exit status of `agent-ready.py` | Inherits the repair. It never calls preflight itself. | +| `scripts/check-test-registration.py` `_preflight_execution_errors` | requires `rc == 0` from an instrumented run | Reads the status as a fact about the *script's own execution*, never as a verdict about a tree. Its `git` shim fails every call, so `BASE_SHA` is empty and five gates skip on every run of it. This consumer is why the default cannot flip: making a skip exit 1 unconditionally turns `check-test-registration` red, and that checker is itself in `CHECKERS`. | +| `tests/scripts/test_agent_onboard.py` `:485` and `:500` | executes `scripts/agent-preflight.sh --role-only` and asserts on the exit status and the output | Unaffected, and named here to complete the census rather than because it is at risk. `--role-only` returns from the role block, above every record gate and both range blocks, so no `skip` can have been recorded by the time either call reads the status. Verified by reading the two call sites, not inferred from the flag's name. | +| a human at a shell, plus the `scripts/agent-preflight.sh passes` checkbox in `.github/pull_request_template.md` | reads the report | Unchanged, and the reason the default stays 0. | +| `tests/scripts/test_agent_preflight_skip_report.py`, through `Report.returncode` | asserts on the exit status in the `--fail-on-skip` and empty-range cases | Named for completeness only, and not a sixth consumer. It **defines** the contract the five above consume rather than depending on it, so it cannot be broken by a change to the exit status: a change there makes this suite red by construction, which is the point of it. | + +So the flag is opt-in because the population splits cleanly: one machine +consumer that must not read a skip as success, one instrumented consumer that +must keep reading exit 0, and a human who reads the report either way. A default +that satisfied the first would break the second. + +`--fail-on-skip` changes nothing else. It does not change what any gate demands, +what any block prints, or which blocks run. It fires only after the failure +summary, so a run that both failed and skipped still exits 1 with the failure +message, and it names itself in its own refusal so a reader is never left +guessing which flag turned an ordinary run red. + +### 3.5 Audit of every other conditional block + +Nothing enumerated these before. This is the full list, in file order. + +| Block | Guard | Verdict | +|---|---|---| +| argument loop | `case "$arg"` | Not a gate. An unknown argument exits 2. | +| role resolution | `if role_line=$(agent-role.py show)` | Reported. The failure arm prints `--` plus two lines of explanation, and appends `role-undeclared` when `REQUIRE_ROLE` is 1. Under `--no-require-role` the state is still printed, and the flag is a documented opt-out the caller chose. Unchanged. Named here because `--` is now the only state that is neither `ok`, `FAIL` nor `SKIP`, and the only one `All gates green.` can print over: the check ran and reported, so it is not a skip, and the caller asked for the exemption, so it is not a failure. That verdict is accepted rather than overlooked, and a reader who greps for the three states should not conclude this fourth mark is a gap. | +| read-only plus `--staged` | `[ "$STAGED" -eq 1 ] && grep role=read-only` | Adds a `FAIL`. Unchanged. | +| `--role-only` early exit | `[ "$ROLE_ONLY" -eq 1 ]` | Already honest, and the model for this fix. It names what did not run and refuses the banner. Unchanged. | +| per-checker dispatch | `case "$checker"` in the `CHECKERS` loop | Selects `--check` for two checkers. Every checker still runs. Not a skip. | +| `Committed range vs origin/main` | `git rev-parse --verify -q origin/main` and `rev-list --count -gt 0` | **Same shape, fixed here.** The unresolvable-ref arm drops three gates silently here, and two more in the trailer block below, so an unresolvable base costs **five** gates in one run and not the three this row first recorded. Measured on the reviewer's independent harness and on case 4 of §5. | +| `Commit trailers vs origin/main` | the same, plus `--is-ancestor` | **The reported defect, fixed here.** | +| `Staged change` | `[ "$STAGED" -eq 1 ]` | A mode the caller selects, documented in the usage text at the top of the file. Running without `--staged` is not a skipped gate, it is a different run. Unchanged. | +| digest print | `[ "$QUIET" -eq 0 ]` | Not a gate. Unchanged. | +| the `--fail-on-skip` exit | `[ "${#skipped[@]}" -ne 0 ] && [ "$FAIL_ON_SKIP" -eq 1 ]` | Added here. Not a gate and it runs nothing. It converts a report the caller cannot read into an exit status the caller can, after the failure summary, so a failing run keeps its own message. See §3.4.1. | + +So two blocks share the shape, and both are repaired in this change. + +### 3.6 An empty range is not a skip + +`rev-list --count "$BASE_SHA..HEAD" -gt 0` guards both range blocks. Two +different situations hide behind one guard today, and they get different +verdicts: + +| Situation | Verdict | Why | +|---|---|---| +| `origin/main` does not resolve | `SKIP` | The input is *unknown*. There are commits to check and the script cannot tell which. | +| `git rev-list --count` fails with the base resolved | `SKIP` | Also *unknown*, and the first version of this change did not say so. See below. | +| `git rev-list --count` succeeds and prints something that is not a count | `SKIP` | Also *unknown*. The exit status is not evidence about stdout. See below. | +| `git merge-base --is-ancestor` exits above 1 | `SKIP`, with the ancestry reason and not the behind-the-base reason | Also *unknown*. The query failed, so the run has no verdict on ancestry to report. | +| `HEAD` adds no commits over the pinned base | reported as an empty range, not a skip | The input is *empty*. A gate over zero commits has nothing to report, and calling that a skip would print `SKIP` on the ordinary session-start run of a freshly cut branch. | + +The empty-range case prints one line naming the pinned SHA, so the reader still +learns why no gate line followed the heading. It does not suppress the banner. + +**Two guards filed an unknown under the empty-range exemption.** Both were found +in the fresh review of this row and both are repaired here. + +`[ "$(git rev-list --count "${BASE_SHA}..HEAD" 2>/dev/null || echo 0)" -gt 0 ]` +maps a *failed* count onto the count `0`, which is the arm this section +deliberately exempts from `SKIP`. The two facts are opposites. An empty range +withholds nothing, and a count that could not be taken withholds everything. +`git checkout --orphan` reaches it with `origin/main` perfectly resolvable: the +base resolves, `HEAD` is unborn, `rev-list` exits 128, and three gates took the +exemption in silence while the banner printed. The count and its exit status are +now kept separately, and a non-zero status is its own `SKIP` arm. + +`git merge-base --is-ancestor` answers 1 for "the base is not an ancestor" and +128 for "that question cannot be asked here". Both took the arm whose reason +reads "is not an ancestor of HEAD, so this branch is behind it", which names a +cause that is not the cause and sends the reader to `git merge` for a tree that +has no commit to merge into. The status above 1 now reports the failed query. + +Both of those bugs failed in the honest direction. Neither produced a wrong +verdict about a tree. One filed an unknown as an exemption and the other filed +an unknown under the wrong name. + +### 3.6.1 The third bug, which this row itself introduced + +**An earlier version of this section claimed "Both bugs failed in the honest +direction", and by the time it was written that was already untrue.** The repair +for `|| echo 0` was spelled + +```sh +RANGE_COUNT="$(git rev-list --count "${BASE_SHA}..HEAD" 2>&1)" +RANGE_STATUS=$? +``` + +and folding stderr into the *value* reintroduced the exact defect this row +exists to remove, through a narrower door and in the **dishonest** direction. It +arrived in `05f6dc04e` and the fresh review of this row found it. + +`RANGE_STATUS` then catches only the case where git **fails**. Git can write to +stderr and still exit 0, and the value is the error text with the count after +it. `[ "$RANGE_COUNT" -gt 0 ]` does not evaluate false there. It **errors** with +status 2, bash prints `integer expression expected` on stderr, a `[` that errors +reads as false, and both range blocks fall through to their empty-range arm. Five +gates leave the report, nothing says so, and `All gates green.` prints with exit +0. That is occurrence 3 again, at five gates instead of two. + +A `.git/objects/info/alternates` naming a path that does not exist reaches it +with no shim at all: git prints `error: unable to normalize alternate object +path: ...` on nearly every object-reading command, writes its ordinary answer to +stdout, and exits 0. Measured on the shipped script at `23d28243f`: + +``` +Committed range vs origin/main 5adc7de9e: empty, HEAD adds no commits. +Commit trailers vs origin/main 5adc7de9e: empty, HEAD adds no commits. + +All gates green. rc=0 +``` + +72 `ok`, 0 `FAIL`, 0 `SKIP`, banner, exit 0. The pre-repair script at `ad8dcad8b` +reports 77 `ok` and runs all five gates on the identical scratch repository, +because it discarded stderr. So this is a regression and not an inherited +defect, and §7.9 records both runs. + +**The repair is two halves, because they cover different failures.** Neither +alone is enough, and the mutations in §7.9 show each half failing on its own. + +1. **Keep stderr out of the value**, with `2>/dev/null`. This is what makes the + value correct in the reproduced case, where the count was perfectly readable + on stdout all along. It is also what stops a git that merely *warns* from + costing five gates a `SKIP` they do not deserve, which validation alone would + do. The correct report for a noisy git with a readable count is the ordinary + run, not a skip. +2. **Validate the value** against `^[0-9]+$`, in one `case` predicate that both + range blocks read, so they cannot drift apart on what counts as unknown. This + is what makes an arm *exist* for a count that is not a count. Without it any + non-numeric value still reaches `-gt`, and its status-2 error is + indistinguishable from "zero commits". Inferring from exit 0 that stdout is a + decimal integer is the assumption that produced this defect, and the repair + should not keep making it. + +### 3.6.2 What `2>/dev/null` cost, and how the cost is paid + +The list above states the benefit of discarding stderr and stops there. The +redirect also had a **cost**, and this section records it, because a trade-off +written down as only its upside is the shape of an argument rather than the +shape of a measurement. + +`2>/dev/null` keeps stderr out of the value **and throws the message away**. An +unborn HEAD then reported + +``` +git rev-list --count ..HEAD exited 128 and printed [] on stdout, which is +not a commit count, so this run could not count the commits under judgement. +``` + +while git had already named the cause in one line, `fatal: ambiguous argument +'..HEAD': unknown revision or path not in the working tree`. Before the +repair for `|| echo 0`, that text reached the terminal by accident, because +nothing captured it. The repair captured the call and silenced it in the same +stroke. The report was still honest, and it lost the half a reader can act on. +This row exists for a report that is honest **and** actionable, so half of it is +not the goal met. + +**The rule is message-not-a-value, not no-message.** Both are satisfied at once +by capturing the text into its own variable, which nothing compares and no arm +reads: + +```sh +RANGE_ERROR="$(git rev-list --count "${BASE_SHA}..HEAD" 2>&1 >/dev/null)" +RANGE_COUNT="$(git rev-list --count "${BASE_SHA}..HEAD" 2>/dev/null)" +RANGE_STATUS=$? +``` + +Two details carry the correctness. `2>&1 >/dev/null` duplicates stderr onto the +capture **before** stdout is sent to `/dev/null`, so the message arrives alone +and the count can never enter it. The reverse order would capture the count and +discard the message, which is the defect wearing the fix as a disguise. And the +value call runs **second**, so `RANGE_STATUS` still describes the command the +value came from rather than the diagnostic call beside it. + +`ANCESTRY_ERROR` at the same site is the precedent this follows, so the two +queries beside each other now report the same way. The captured text prints +inside `RANGE_UNKNOWN`, which a run reaches only when the count is unusable, and +case 17 asserts that it is there. Two mutations pin it: dropping `${RANGE_ERROR}` +from the reason, and reverting the capture to a discard, each reddening case 17 +alone. + +**`ANCESTRY_ERROR` merges stderr the same way and is correct as it stands.** +Checked rather than assumed. Under the same broken-alternates repository +`git merge-base --is-ancestor` also writes to stderr and exits 0. The difference +is that `ANCESTRY_ERROR` is a **message and never a value**: nothing compares it, +and every ancestry arm is selected by `ANCESTRY_STATUS` alone. The only read of +it is inside the `ANCESTRY_UNKNOWN` string, which a run prints only when the +query failed, and where the stderr text is the useful part. It is left merged +deliberately, and the script now says so beside the call. + +## 4. Risks and decisions + +| Risk | Assessment | +|---|---| +| Weakening a checker to make something pass | Nothing is relaxed. No checker changes. The script reports strictly more than it did, and one previously reachable banner becomes unreachable. | +| `SKIP` becomes the new silence, ignored like a warning | It is loud by construction. It prints at the skip site with its reason, it prints again in the summary with a count, and it deletes the banner every reader greps for. A reader who greps `All gates green.` gets nothing. | +| The green banner never appears again on ordinary work | Measured against the three occurrences. After pinning, occurrences 2 and 3 print the banner, because the pinned SHA was an ancestor when captured. Only occurrence 1, a genuinely behind branch, reports `SKIP`, and that report is the correct one. | +| Exit 0 lets a behind branch land unchecked trailers | It does not, because CI runs `check-commit-trailers.py` on the pull request range independently of this script. Preflight is developer convenience, and `AGENTS.md` already says hooks and local runs are "bypassable convenience, not evidence". What was missing was an honest local report, not a second enforcement point. | +| Pinning hides a base that moved for a good reason | The pinned SHA is printed. A reader who wants the newer base fetches, merges, and reruns, which is the same repair the skip reason names. | +| The audit missed a block | §3.5 lists every conditional in the file, in file order, with a verdict for each. The suite asserts that both repaired blocks report, and the mutation cases in §5 fail if either regresses. | +| `check-commit-style.py` still refuses a non-ancestor base | Real, found during this work, and deliberately **not** fixed here. Removing the preflight guard would send `check-commit-style.py --range` a non-ancestor base, which raises at `validate_range`, so `commit-style` would go red on every branch behind `main`. That is the "gate that fires on ordinary work" defect, traded for this one. Repairing it means changing that checker's range semantics, which `AGENTS.md` says needs its own spec and its own red-before test. Listed under `## Owed`. | + +## 5. Tests + +`tests/scripts/test_agent_preflight_skip_report.py`, registered in the `SUITES` +array of `scripts/agent-preflight.sh` and in the record lane of +`.github/workflows/ci.yml`. + +The suite executes the real script. It cannot run a nested full preflight, +because preflight runs this very suite and would recurse without bound. The +existing dodge for that problem is `--role-only`, which is too narrow here, +because the blocks under test are the last two in the file. + +**The harness.** Copy `scripts/agent-preflight.sh` into a scratch git repository +and put a stub `python3` on `PATH` that exits 0. Every `run` then reports `ok` +without executing a checker, no suite re-enters this one, and the git-shaped +control flow under test runs for real against refs the test controls. +`scripts/agent-ready.py` is copied in beside it, so the cases that exercise the +handoff gate run the real script against the same scratch refs. + +**The stub records its argv**, appending each invocation to +`$VLLM_TEST_ARGV_LOG`. Without that the suite reads only the script's *report*, +and the report is silent about the SHA the five range checkers are handed. +Putting `--base origin/main` back on the three range gates, or +`--range origin/main..HEAD` back on the two trailer gates, then leaves every +other case in the file green while the heading names pinned SHA X and the +checkers judge whatever the moving ref points at, which is the exact defect this +row was filed for wearing the fix as a disguise. The script states the guarantee +in the comment above `BASE_REF` ("Every range block below compares against this +SHA and never against the ref"), and nothing detected its loss. +`scripts/check-test-registration.py` traces preflight the same way for the same +reason. + +RED-first cases, each failing on the unmodified script: + +1. `test_a_non_ancestor_base_reports_skip_and_no_green_banner` sets + `refs/remotes/origin/main` to a divergent commit. **RED before:** the run + prints `All gates green.`, exit 0, with two fewer `ok` lines than the + ancestor run and no mention of the trailer block. **After:** the run prints + `SKIP commit-trailers`, `SKIP commit-style`, a reason naming the pinned SHA, + a `2 gate(s) SKIPPED` summary, and no banner. +2. `test_the_ok_count_falls_only_with_a_reported_skip` runs the same script + twice, once with an ancestor base and once with a divergent base, and asserts + that the difference in `ok` count equals the number of `SKIP` lines. + **RED before:** the counts differ by two and there are zero `SKIP` lines, + which is occurrence 3 exactly. This is the case that makes a *silent* drop + impossible rather than merely unlikely. +3. `test_a_base_that_moves_mid_run_does_not_change_the_verdict` installs a stub + `python3` that advances `refs/remotes/origin/main` to a divergent commit on + its first call, which is what another worktree's fetch does. **RED before:** + the trailer block vanishes and the run still says green. **After:** the run + gates against the SHA pinned at the start, both trailer gates run, and the + banner is earned. +4. `test_an_unresolvable_base_reports_skip_for_both_range_blocks` deletes + `refs/remotes/origin/main`. **RED before:** five gates vanish and the run says + green. **After:** five `SKIP` lines and no banner. +5. `test_an_empty_range_is_not_a_skip` puts `HEAD` exactly at the pinned base. + Green both before and after, and it must stay green: it pins §3.6, so a fix + that reports `SKIP` for an empty range fails here. Without this case the + obvious over-correction passes every other case. +6. `test_an_ancestor_base_still_earns_the_banner` is the control. It must print + `All gates green.` and exit 0, so a fix that suppresses the banner + unconditionally cannot pass. +7. The pinned SHA has to appear in the output, asserted inside case 3 rather + than in a case of its own. +8. `test_the_suite_runs_in_preflight_and_in_ci` asserts this file's name appears + in the `SUITES` array and in `.github/workflows/ci.yml`, because a suite + wired into neither runs on no machine. + +Cases added by the fresh review of this row: + +9. `test_every_range_gate_is_handed_the_pinned_sha_and_never_the_ref` reads the + argv log after an ordinary run and asserts that each of the five gates that + take a base was invoked exactly once with one, that the invocation carries + the pinned SHA, and that it names no ref. It counts all five first, because + a log that recorded nothing satisfies every `assertNotIn` in the case. Green + before and after, like case 5: it is a regression guard, and its red-before + is the pair of mutations below. +10. `test_an_unborn_head_reports_skip_rather_than_an_empty_range` runs + `git checkout --orphan` with `origin/main` still resolvable. **RED before:** + the three range gates take the empty-range exemption and print + `empty, HEAD adds no commits`, and the trailer block skips with the reason + `is not an ancestor of HEAD`, which is not the reason. **After:** five + `SKIP` lines, no banner, and each reason names the git query that failed. +11. `test_the_flag_makes_a_skip_exit_1_and_the_default_still_exits_0` runs the + same behind-base tree twice, once plain and once with `--fail-on-skip`, and + asserts the reports are identical while the exit statuses differ. Both facts + live in one case because each is the other's justification. +12. `test_the_flag_does_not_fire_on_a_run_that_skipped_nothing` is the control + for the flag. A flag that reds an ordinary run gates nothing. +13. `test_a_skipped_preflight_stops_the_handoff_gate` runs the real + `scripts/agent-ready.py` against a scratch repo whose base is divergent. + **RED before:** preflight exits 0 over two skipped gates, `agent-ready` + reads that as success and walks straight past it. **After:** it stops at the + local preflight, relays the `SKIPPED` report to its own caller, and never + reaches the remote question. +14. `test_an_unskipped_preflight_lets_the_handoff_gate_continue` is the control + for case 13. The scratch repo has no `origin` remote, so a run that gets + past the local preflight refuses with `REMOTE_UNVERIFIED` instead. That + second refusal is the proof the first one did not fire. + +Cases added by the second fresh review of this row, both in +`StderrIsNotTheValueTests` and both red on `23d28243f`: + +15. `test_a_git_that_warns_and_exits_zero_still_runs_every_range_gate` breaks + `.git/objects/info/alternates` so git writes to stderr, prints the count on + stdout, and exits 0. It asserts a precondition first, and asserts all three + parts of it: exit 0, stderr non-empty, stdout exactly `1`. Without that, a + git that simply failed would satisfy the case through the arm + `test_an_unborn_head_...` already covers. **RED before:** the five range + gates report neither `ok` nor `SKIP`, `empty, HEAD adds no commits` prints + over both blocks, and the banner prints. **After:** all five run, nothing is + skipped, and the run keeps the banner it earned. It also compares the `ok` + count against a control run of the same repository, which is the same + invariant as case 2: a gate may leave the report only by saying that it did. +16. `test_a_count_that_is_not_a_number_reports_skip_rather_than_empty` puts a + `git` shim on `PATH` that answers `rev-list` with a non-numeric line and + exit 0, and forwards every other subcommand to the real program. Its + precondition asserts both halves of that, so a shim that broke `rev-parse` + could not pass as a shim that only broke the count. **RED before**, and red + against a repair that only discards stderr, which is why both halves of + §3.6.1 are in the change. No real git behaves this way. The case pins the + *arm*, not a git. + +Case added by the third fresh review of this row, in +`AnUnknownIsNotAnEmptyRangeTests` and red on `3fd734a5b`: + +17. `test_a_range_skip_carries_the_message_git_printed` runs the unborn-HEAD + scenario and asserts that the reason printed under each of the three range + gates **contains the line git wrote to stderr**. It reads the reason through + the new `skip_reason` harness helper, which returns the nine-column block + `skip` indents under one label, rather than matching against the whole + transcript: the same words printed anywhere else in a long report would + otherwise satisfy the assertion. Its precondition asserts that git really + fails and really writes a `fatal:` line for that exact command, so a git that + printed nothing could not pass the case by leaving nothing to carry. It + asserts only the three range gates, because `--is-ancestor` exits 128 on an + unborn HEAD and the two trailer gates take the ancestry arm, whose reason + carries a different message. **RED before:** the reason names exit 128 and an + empty value and no cause. **After:** it carries `fatal: ambiguous argument + ...` beside each gate. §3.6.2 records the trade-off this case pins. + +Mutation cases, each restoring the tree byte-for-byte afterwards: + +- Delete the `skipped` check from the summary. Cases 1 and 4 must go red. +- Replace `"$BASE_SHA"` with `origin/main` in the trailer guard. Case 3 must go + red. +- Make `skip` print `ok`. Cases 1, 2 and 4 must go red. +- Report `SKIP` for the empty range. Case 5 must go red. +- Replace `--base "$BASE_SHA"` with `--base origin/main` at all three range + gates. Case 9 must go red on all three. +- Replace `--range "${BASE_SHA}..HEAD"` with `--range "origin/main..HEAD"` at + both trailer gates. Case 9 must go red on both. +- Restore `2>&1` on the count, which is the regression itself. Case 15 must go + red and case 16 must stay green. +- Make the numeric predicate unmatchable, and separately delete the + `RANGE_NUMERIC` term from both range arms. Case 16 must go red for each and + case 15 must stay green. +- Drop `${RANGE_ERROR}` from the `RANGE_UNKNOWN` reason, and separately revert + the capture to `2>/dev/null` so there is no message to print. Case 17 must go + red for each, and cases 15 and 16 must stay green. + +The last two pairs are the evidence that the two halves of §3.6.1 are not +redundant. If either half covered the other, one of these mutations would leave +the suite green. + +The mutation harness restores from a **pristine copy taken before the first +mutation**, never with `git checkout --`. The first run of this row's matrix used +`git checkout -- scripts/agent-preflight.sh`, which reverts to the committed file +and therefore deleted the uncommitted repair under test. The first mutation was +then measured against the repair and the next two against its absence, so case 17 +reddened under mutations that have nothing to do with it and the result read as a +finding about the code. The per-mutation hash comparison is what exposed it. An +instrument that restores from `HEAD` injects exactly the defect it was built to +detect, and the cost of getting this wrong is a verdict, not an error message. + +## 6. Gates + +- `python3 tests/scripts/test_agent_preflight_skip_report.py`, with cases 1 + through 4 shown RED on the unmodified script. +- `bash scripts/agent-preflight.sh`, run on this branch, with the real per-block + counts recorded and the pinned SHA named. The run must not itself skip a + block, which is the same claim this row is about. +- `python3 scripts/check-test-registration.py` and + `python3 tests/scripts/test_check_gate_commands.py`, because the change edits + the script both of them read. `check-test-registration.py` is the sharper of + the two here: it executes preflight under a `git` shim that fails every call + and requires exit 0, so it fails if the default exit status for a skip ever + flips. +- `python3 tests/scripts/test_agent_gates.py`, because the change edits + `scripts/agent-ready.py`, which that suite loads. +- No CUDA, GPU, oracle, checkpoint or SACRED gate is implicated. The change + touches one shell script and one test file, reaches no forward pass, and loads + no weights. + +## 7. Evidence + +### 7.1 The defect reproduced on the unmodified script + +Scratch repository, stub `python3`, one `HEAD` and two settings of +`refs/remotes/origin/main`. The only difference between the two runs is which +commit that ref names. + +| `origin/main` | `ok` lines | `SKIP` lines | banner | exit | +|---|---:|---:|---|---:| +| an ancestor of `HEAD` | 76 | 0 | `All gates green.` | 0 | +| a divergent commit | 74 | 0 | `All gates green.` | 0 | + +Two gates left the report and every observable in the output stayed the same. +This is occurrence 3, at the same counts that were seen live. + +### 7.2 RED before + +`python3 tests/scripts/test_agent_preflight_skip_report.py` on the unmodified +script: `Ran 8 tests`, `FAILED (failures=11)`. Five of the eight are red, and +the three controls in `TheBannerStaysReachableTests` that must stay green +already are. The count case reports the defect in one line: + +``` +AssertionError: 2 != 0 : 2 gate(s) disappeared from the report and 0 were +reported as skipped. Every gate that stops running has to say so. +``` + +### 7.3 GREEN after + +Same suite on the repaired script: `Ran 8 tests`, `OK`. The first fresh review of +this row added six more cases, taking the suite to `Ran 14 tests`, `OK`. Their +own red-before is §7.7 and §7.8. The second fresh review added two more, so the +suite now reports `Ran 16 tests`, `OK`, and their red-before is §7.9. + +The same scratch comparison, rerun against the repaired script. The `ok` totals +are one higher than in §7.1 because the new suite joined `SUITES`: + +| `origin/main` | `ok` | `SKIP` | banner | exit | +|---|---:|---:|---|---:| +| an ancestor of `HEAD` | 77 | 0 | `All gates green.` | 0 | +| a divergent commit | 75 | 2 | none | 0 | + +The drop of two is now fully accounted for by two `SKIP` lines, and the run +prints the reason and the pinned SHA: + +``` +2 gate(s) SKIPPED: commit-trailers commit-style +NOT a green preflight: a skipped gate reported nothing about this tree. +``` + +### 7.4 Mutations + +Four mutations, each verified to have applied (`git diff --stat` changed) and to +still compile (`bash -n` returned 0), each restoring the file byte-for-byte +afterwards, confirmed by a sha256 match against the pre-mutation file. + +| Mutation | Cases turned red | +|---|---| +| the banner ignores the `skipped` array | 2: the non-ancestor case and the unresolvable-base case | +| the trailer guard reads `origin/main` again instead of `$BASE_SHA` | 1: the mid-run move case | +| `skip()` prints `ok` | 4: both skip cases, the count case, and the exit-code case | +| the empty range reports `SKIP` | 1: the empty-range case | + +### 7.5 The row's own preflight run + +`bash scripts/agent-preflight.sh --quiet` on this branch after the review +repairs, gated against `origin/main 332aed738a40050fd3fe677bc12ac1bcdc8e4bb5`, +which both range block headings name. + +| Block heading | `ok` | `FAIL` | `SKIP` | +|---|---:|---:|---:| +| `Session role:` | 1 | 0 | 0 | +| `Record gates:` | 25 | 1 | 0 | +| `Mutation suites:` | 44 | 1 | 0 | +| `Committed range vs origin/main 332aed738:` | 3 | 0 | 0 | +| `Commit trailers vs origin/main 332aed738:` | 2 | 0 | 0 | +| total | 75 | 2 | 0 | + +`SKIP` is 0, so this run did not itself skip a block, which is the same claim +the row is about. The trailer block reports two `ok` rather than two `SKIP` +because the branch merged `origin/main` first, which is the repair the skip +reason names. + +The two failures are `check-env-doc` and `test_check_env_doc`, and they are +**pre-existing on `main`**, not caused by this change. +`VT_MOE_EXPERT_STREAM`, `VT_MOE_EXPERT_STREAM_SLOTS` and +`VT_MOE_EXPERT_STREAM_SLOT_BYTES` arrived with `3005447f8` (#993) and are +neither documented nor allowlisted. Filed as +[#1000](https://github.com/mudler/vllm.cpp/issues/1000) and owned by +`ENG-EXPERT-STREAM`, and independently as +[#995](https://github.com/mudler/vllm.cpp/issues/995) against the same row by +another branch. The attribution here is measured rather than asserted: +`git diff origin/main...HEAD --stat` names six paths, none of them under `src/` +or `include/`, so this branch cannot have introduced an env var read from those +trees. + +### 7.5.1 The three reds measured under `ENOSPC` + +The first version of this section said that `test_cpu_x86_llamacpp_floor` failed +and that "two focused gates reported unrelated errors". It named one of the +three, quoted no `df`, no error text and no rerun output. That is thinner than +what this project asks of an attribution, and an unnamed gate cannot be checked +by a reader. The attribution stands. The evidence for it is below. + +The three, named, with what each printed in that run: + +| Gate | Symptom in the original `ENOSPC` run | +|---|---| +| `test_cpu_x86_llamacpp_floor` | failed. Load-dependent and pre-existing, [#618](https://github.com/mudler/vllm.cpp/issues/618) | +| `python3 scripts/check-test-registration.py` | `ERROR: CMake configure failed while proving required test registration`, then `ERROR: missing required test target test_device_selection in configured codemodel`, exit 1 | +| `python3 tests/scripts/test_check_test_registration.py` | 65 errors, exit 1 | + +**Why an out-of-disk instrument accuses the code here.** +`scripts/check-test-registration.py` does its work in a temporary directory at +`:300`, `:489` and `:737`, each under `tempfile.TemporaryDirectory`. `_configure` +at `:67-78` requests the CMake File API codemodel at `:72-74` and then runs a +real `cmake -S . -B ` at `:75-78`, and `_codemodel_targets` at `:81-110` +reads the reply back out of that same directory at `:91-97`, returning an empty +target map at `:93-94` when no `index-*.json` is there. When the write fails +there is no reply to read, the target list is empty, and the checker reports +precisely a **missing cmake target**: a verdict about the tree, phrased in the +vocabulary of the tree, produced by a full disk. + +Each fact above is anchored to the function that carries it, because the earlier +version of this paragraph cited `_configure_cmake` at `:72-91`, and that symbol +does not exist anywhere in the tree. The span also straddled two functions, so +half of what it claimed was in the other one. This is the section that was +rewritten precisely because its evidence was thin, and a reader who greps the +cited symbol and finds nothing cannot tell a wrong anchor from a wrong claim. +`grep -rn '_configure_cmake' .` now matches nothing. + +That mechanism is measured rather than argued. Deliberately reproduced on a +1 MiB `tmpfs` filled to zero bytes and pointed at by `TMPDIR`, on this tree, at +this SHA: + +``` +tmpfs 1.0M 1.0M 0 100% .../scratchpad/fullfs +$ TMPDIR= python3 scripts/check-test-registration.py # rc=1 +ERROR: CMake configure failed while proving required test registration +ERROR: missing required test target test_device_selection in configured codemodel +ERROR: vllm_cpp_add_test does not create an executable with its configured sources +$ TMPDIR= python3 tests/scripts/test_check_test_registration.py # rc=1 +Ran 52 tests -- FAILED (failures=12) +``` + +The checker's verdict reproduces word for word. The suite's *count* does not, +because a 1 MiB `tmpfs` is milder pressure than the whole filesystem at zero: +some temporary directories still get created, so cases fail rather than error. +The count is therefore not the evidence and is not offered as such. The +mechanism is, and it reproduces on demand. + +**Green on the identical tree with space available**, measured immediately +before this run, `TMPDIR` unset so both use `/tmp`: + +``` +$ df -h /tmp -> 447G size, 66G avail, 85% used +$ python3 scripts/check-test-registration.py # rc=0 +OK: required regression tests have executable + CTest registration and the +guard is wired into preflight/CI. +$ python3 tests/scripts/test_check_test_registration.py # rc=0 +Ran 52 tests -- OK +``` + +So a red measured under `ENOSPC` is not a result. Check free disk before +attributing any failure to the code, because a broken instrument does not report +as a broken instrument. It reports as a verdict about the code. + +### 7.6 `agent-ready.py` refuses a skip while a human preflight does not + +Both scripts copied into one scratch repository, one `HEAD`, `origin/main` set +to a divergent commit, stub `python3` on `PATH`. The only difference between the +two rows is which program asked. + +| Caller | Report | Exit | +|---|---|---:| +| `bash scripts/agent-preflight.sh --quiet` | `2 gate(s) SKIPPED: commit-trailers commit-style`, no banner | 0 | +| `python3 scripts/agent-ready.py` | the same report, then `READY FAILED: local preflight did not report every gate green.` | 1 | + +`agent-ready.py` never reached the remote question, which is the proof that it +stopped at the local one. On the unrepaired pair it walked straight past the +skip and refused with `REMOTE_UNVERIFIED: error: No such remote 'origin'` +instead, which is case 13's red-before in one line. + +### 7.7 Red-before for the review repairs + +The new cases run against the tree as it was before this repair, restored with +`git show HEAD:scripts/agent-preflight.sh` and +`git show HEAD:scripts/agent-ready.py` while the new suite stayed in place: +`Ran 14 tests`, `FAILED (failures=7)`. + +| Case | Red-before | +|---|---| +| 10, unborn `HEAD` | the three range gates never reported, and `empty, HEAD adds no commits` was printed over them | +| 10, the ancestry reason | `is not an ancestor of HEAD` printed for a query that failed with `fatal: Not a valid object name HEAD` | +| 11 and 12, `--fail-on-skip` | `unknown argument`, exit 2, and the harness precondition caught it as a run that reported nothing | +| 13, the handoff gate | `'READY FAILED: local preflight' not found in "REMOTE_UNVERIFIED: error: No such remote 'origin'"` | + +Cases 9 and 14 are green before and after by design. Case 9 is a regression +guard whose red-before is §7.8, and case 14 is the control that proves case 13 +is not a script that refuses everything. + +### 7.8 The two mutations the suite could not see before + +Both are the reviewer's, applied to the repaired script, each verified to have +applied (`git diff --stat` changed) and to still parse (`bash -n` returned 0), +each restored byte-for-byte afterwards and confirmed by a sha256 match. + +| Mutation | Sites | Result | +|---|---:|---| +| `--base "$BASE_SHA"` becomes `--base origin/main` | 3 | case 9 red on all three range gates, `FAILED (failures=3)` | +| `--range "${BASE_SHA}..HEAD"` becomes `--range "origin/main..HEAD"` | 2 | case 9 red on both trailer gates, `FAILED (failures=2)` | + +Before the argv log, both mutations left the suite at `Ran 8 tests`, `OK`. + +Restored: `sha256sum -c` reports `OK` for all three files, and the suite returns +to `Ran 14 tests`, `OK`. + +### 7.9 The regression this row introduced, and its repair + +**The defect on the shipped script**, `23d28243f`, reproduced end to end before +anything was edited. One scratch repository, stub `python3` on `PATH`, +`origin/main` an ancestor, `HEAD` one commit ahead, and a +`.git/objects/info/alternates` naming a path that does not exist. The only +difference between the rows is which script ran against it: + +| Script | `ok` | `FAIL` | `SKIP` | the five range gates | banner | exit | +|---|---:|---:|---:|---|---|---:| +| `ad8dcad8b`, before the `2>&1` | 77 | 0 | 0 | all five `ok` | `All gates green.` | 0 | +| `23d28243f`, as shipped | 72 | 0 | 0 | **absent from the report** | `All gates green.` | 0 | +| this repair | 77 | 0 | 0 | all five `ok` | `All gates green.` | 0 | + +The shipped script also wrote two lines to stderr that no reader of the report +sees: + +``` +agent-preflight.sh: line 320: [: error: unable to normalize alternate object path: ... +1: integer expression expected +``` + +`git log -S` places the `2>&1` in `05f6dc04e` and nowhere earlier, so the +regression is this row's own and not something it inherited. + +**RED before.** The two new cases against the script restored to `23d28243f` +with the new suite in place, the restore verified by `git diff --quiet` and the +file verified to parse with `bash -n`: + +``` +Ran 16 tests -- FAILED (failures=12) +``` + +The 12 are the two new cases and their ten `subTest` gates. The other 14 cases +were green, so the red is scoped to what this repair is about: + +``` +AssertionError: False is not true : commit-trailers did not run although the +count was readable on stdout: +AssertionError: 'empty, HEAD adds no commits' unexpectedly found in ... : +HEAD adds a commit and the run reported an empty range: +AssertionError: True is not false : five unknown gates printed green: +``` + +**GREEN after.** `Ran 16 tests`, `OK`. + +One correct report was red on the first green-after run, and the message was +repaired rather than the assertion. Rewrapping `RANGE_UNKNOWN` had split the +sentence `Unknown is not an empty range.` across a newline, which cases 10 and +16 assert verbatim. The behaviour was right and the prose was wrong. The line is +now kept unwrapped and the script says why beside it. + +**Mutations**, each verified to have applied by a diff against the pre-mutation +file rather than against `HEAD`, each verified to parse, each restored +byte-for-byte and confirmed by `sha256sum -c`: + +| Mutation | Sites | Result | +|---|---:|---| +| `2>/dev/null` becomes `2>&1`, the regression itself | 1 | case 15 red, `FAILED (failures=6)`. Case 16 stays green | +| the numeric `case` pattern is made unmatchable | 1 | case 16 red, `FAILED (failures=6)`. Case 15 stays green | +| the `RANGE_NUMERIC` term is deleted from both range arms | 2 | case 16 red, `FAILED (failures=6)`. Case 15 stays green | + +The last two rows are why the repair has two halves. Each half has a mutation +that only the other half's case detects, so neither is covering for the other. + +A first attempt at the second mutation was written with `sed` and a delimiter +that collided with the `|` in the pattern. `sed` exited non-zero, nothing was +edited, and the suite reported `Ran 16 tests`, `OK`. That `OK` was a mutation +that never applied wearing a passing test. The harness was rewritten to diff +against the saved pre-mutation copy and to refuse to report a suite result at +all when the pattern matched zero sites. Every result above ran under that +harness. + +### 7.10 Two corrections to the merge commit body + +`5b95e221e` is a merge and its body is not amendable, so the corrections live +here. The squash body will be the landed record. + +- The body says main brought "#986, #995 and #987". It brought **#986 and + #995**. `#987` was already in the merge base: `3ce1cf7c7`, this row's own + base, is `feat(LTX25-RETAKE) ... (#924, #987) (#992)`. Diffing + `.agents/issue-index.md` at the merge base against each parent shows main's + side adding exactly two rows. +- The body says "this branch's #998 and #999". There are **three**: #998, #999 + and #1000. The same diff shows all three on the branch side, and all three are + on `HEAD` at lines 272 through 274. + +Both are miscounts in a description of a union merge that was itself correct. +The merged file is unchanged by this correction. + +### 7.11 The row's own preflight run after the second review repair + +`bash scripts/agent-preflight.sh --quiet`, gated against +`origin/main 45b022cdc138ae15b77b0149093071353de8ad4e`, which both range block +headings name. `/tmp` had 65 GiB free at the start and 44 GiB at the end, so no +gate here ran out of disk and §7.5.1 does not apply to any red below. + +| Block heading | `ok` | `FAIL` | `SKIP` | +|---|---:|---:|---:| +| `Session role:` | 1 | 0 | 0 | +| `Record gates:` | 25 | 1 | 0 | +| `Mutation suites:` | 43 | 2 | 0 | +| `Committed range vs origin/main 45b022cdc:` | 3 | 0 | 0 | +| `Commit trailers vs origin/main 45b022cdc:` | 2 | 0 | 0 | +| total | 74 | 3 | 0 | + +**`SKIP` is 0**, so this run did not itself skip a block, which is the same +claim the row is about. The trailer block reports two `ok` rather than two +`SKIP` because the branch merged `origin/main` first, which is the repair the +skip reason names. + +`check-env-doc` and `test_check_env_doc` are green here. They were red on the +run before the merge, and they were proved red on **pristine** `332aed738` in a +detached worktree with no local edits, naming the same three +`VT_MOE_EXPERT_STREAM` variables. The incoming `45b022cdc` (#997) documents +them, so the merge cleared a red this branch never caused. + +The three remaining failures are pre-existing, and each is **measured** on a +tree this branch does not control rather than attributed: + +| Failure | Proof it is not this change | +|---|---| +| `check-agent-record` | `.agents/issue-index.md` lists issue #995 twice. Reproduced on a **pristine detached worktree at `origin/main` `45b022cdc`** with an empty `git status --porcelain`, printing the identical message. The branch side at `ca1f5ad59` had no duplicate row at all, so the merge inherited it. Branch `fix/issue-index-995-dup` already owns the repair | +| `test_agent_record` | Same cause, same pristine worktree, `Ran 74 tests`, `FAILED (failures=1)`, on the one case that reads that index | +| `test_cpu_x86_llamacpp_floor` | Load-dependent, [#618](https://github.com/mudler/vllm.cpp/issues/618). It reported `ok` in this branch's own pre-merge preflight run, and `Ran 10 tests`, `OK` in 80s on pristine `332aed738` earlier in the same session. It fails now with the box at load 126 to 180, and every failing assertion carries the harness's own reason: `waiting for quiet: 15s busy=110% builders=0 load=172.07`. The suite refuses to measure a contended box, which is the behaviour it is built for, so this red is the instrument declining to produce a number rather than a verdict about the tree | + +Disk was checked before any of these attributions, as §7.5.1 requires. + +### 7.12 The third review's two repairs + +The third fresh review returned `PASS` and raised two `LOW` observations. Both +are on this row's own theme, a report that is honest **and** actionable, so both +were taken. + +**The wrong anchor.** `grep -rn '_configure_cmake' .` matched exactly one line in +the tree, the §7.5.1 sentence that cited it. The functions that carry those facts +are `_configure` at `scripts/check-test-registration.py:67-78` and +`_codemodel_targets` at `:81-110`, so the cited span `:72-91` also straddled two +of them. §7.5.1 now anchors each fact to the function that carries it, and the +grep matches nothing. + +**The discarded message.** RED before, on `3fd734a5b`: + +``` +$ python3 tests/scripts/test_agent_preflight_skip_report.py +Ran 17 tests -- FAILED (failures=3) +FAIL: test_a_range_skip_carries_the_message_git_printed (gate='now-current range') +FAIL: test_a_range_skip_carries_the_message_git_printed (gate='doc-checkpoint range') +FAIL: test_a_range_skip_carries_the_message_git_printed (gate='issue-index append-only') +``` + +Three failures and one case, because the case reports one `subTest` per range +gate. Every other case stayed green, so the new assertion is the only thing this +red measures. GREEN after: `Ran 17 tests -- OK`. + +What the reader now gets, from the unborn-HEAD scenario reproduced by hand +against a scratch repository and the stub `python3`: + +``` + SKIP now-current range + git rev-list --count 78d72e768..HEAD exited 128 and + printed [] on stdout, which is not a commit count, so this run + could not count the commits under judgement. An unborn HEAD is one way to reach + this, and so is a git that writes an error to stderr and still exits 0. git + wrote this to stderr: + fatal: ambiguous argument '78d72e768..HEAD': unknown revision or path not in the working tree. +``` + +**The mutation matrix, rerun in full.** Every mutation is shown applied by a diff +against a pristine copy, the script is shown parsing before any result is read, +and each run's case count is asserted non-zero, because a suite that never ran +prints no failures and reads as a pass: + +| Mutation | Applied | Parses | Cases ran | Red | +|---|---|---|---:|---| +| `2>/dev/null` reverted to `2>&1` on the value | 1 line | yes | 17 | case 15 only, `failures=6` | +| numeric `case` pattern made unmatchable | 1 line | yes | 17 | case 16 only, `failures=6` | +| `RANGE_NUMERIC` dropped from both arms | 2 lines | yes | 17 | case 16 only, `failures=6` | +| `${RANGE_ERROR}` dropped from the reason | 1 line | yes | 17 | case 17 only, `failures=3` | +| `RANGE_ERROR` capture reverted to a discard | 1 line | yes | 17 | case 17 only, `failures=3` | + +The three prior mutations still redden exactly their own case, so the new +assertion is not coupled to them and they are not coupled to it. After each +mutation the script was restored and its sha256 compared against the +pre-mutation value `a7437384ea7fd9b60b3e8daf86885866a2eaeec3a3a967908261c21f91e60d88`, +matching every time. + +**The harness ate the tree once, and the hash check is why that is known.** The +first version of the matrix restored with `git checkout --`, which reverted to +the committed file and deleted the uncommitted repair under test. Mutations 2 and +3 were then measured against a script with no repair in it, and case 17 reddened +under both. Read at face value that is a finding about the code, and it is +entirely an artefact of the instrument. §5 records the rule that follows. + +**The row's own preflight run**, `bash scripts/agent-preflight.sh --quiet`, gated +against `origin/main 45b022cdc138ae15b77b0149093071353de8ad4e`, which both range +block headings name: + +| Block heading | `ok` | `FAIL` | `SKIP` | +|---|---:|---:|---:| +| `Session role:` | 1 | 0 | 0 | +| `Record gates:` | 25 | 1 | 0 | +| `Mutation suites:` | 44 | 1 | 0 | +| `Committed range vs origin/main 45b022cdc:` | 3 | 0 | 0 | +| `Commit trailers vs origin/main 45b022cdc:` | 2 | 0 | 0 | +| total | 75 | 2 | 0 | + +**`SKIP` is 0**, so this run skipped nothing, which is the claim the row is +about. The two failures are `check-agent-record` and `test_agent_record`, the +inherited duplicate `#995` row that §7.11 already measured on a pristine +worktree and that `fix/issue-index-995-dup` owns. `test_cpu_x86_llamacpp_floor` +is green in this run, one block up from where §7.11 recorded it red, which is +the load dependence [#618](https://github.com/mudler/vllm.cpp/issues/618) +describes rather than a change in the tree. + +Disk was checked before any attribution, as §7.5.1 requires: the root filesystem +held 49 GiB free before the run and 39 GiB after, at load 41 to 77. No gate here +ran out of space, so §7.5.1 does not apply to either red. + +## 8. Stop conditions + +- Stop if `All gates green.` can still print after any block was skipped. That + is the whole row. +- Stop if the `ok` count can fall without a matching `SKIP` line. A silent drop + is occurrence 3, and case 2 exists to make it impossible. +- Stop if an ordinary run on a branch level with `origin/main` stops printing + the banner. Making the banner unreachable is not the fix. +- Stop if repairing the report requires changing what any checker demands. +- Stop if the empty-range case starts reporting `SKIP`, because that turns the + most common session-start run into a false alarm. + +## Owed + +- [#999](https://github.com/mudler/vllm.cpp/issues/999): `scripts/check-commit-style.py` + `validate_range` still refuses a base that is not an ancestor of head, so it + never received the merge base repair that `GATE-FORK-ANCESTRY` (#773) applied + to `scripts/check-commit-trailers.py`. Found while auditing this script's + guards. Not fixed here, because it changes that checker's range semantics and + needs its own spec and red-before test. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0b8ccec10..98b9d1329 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -251,6 +251,14 @@ jobs: # whether a checker should catch the class; this closes the instance. run: | python3 tests/scripts/test_main_baseline.py + - name: Preflight never reports green over a block that did not run + # Registered in TWO places, like the baseline suite above: here and in + # the `SUITES` array of `scripts/agent-preflight.sh`. This lane is the + # one that matters more of the two, because the defect it pins (#998) + # is preflight lying about ITSELF, and a suite that only ever runs from + # the script it audits is checked by exactly the run it cannot trust. + run: | + python3 tests/scripts/test_agent_preflight_skip_report.py - name: Protocol prose matches the checkers that enforce it # PART tree-scoped, PART diff-scoped, and the two must be told apart on # the baseline lane (#274). `documentation-checkpoint` and diff --git a/scripts/agent-preflight.sh b/scripts/agent-preflight.sh index 836d77c36..5ca6fb58d 100755 --- a/scripts/agent-preflight.sh +++ b/scripts/agent-preflight.sh @@ -11,6 +11,7 @@ # scripts/agent-preflight.sh --quiet # gates only, no digest # scripts/agent-preflight.sh --no-require-role # tolerate an undeclared role # scripts/agent-preflight.sh --role-only # ONLY the role gate; NOT a preflight +# scripts/agent-preflight.sh --fail-on-skip # exit 1 when any gate was SKIPPED # # A session declares a role, and an UNDECLARED one is a failing gate by default: # the obligation used to live in prose and in an opt-in flag, and neither fired. @@ -26,6 +27,19 @@ # --no-require-role runs and so has not earned it. Neither mode is a superset of # the other; --role-only is narrower and stricter, and says so on stdout. # +# Every gate reports one of THREE states. `ok` ran and passed, `FAIL` ran and +# failed, `SKIP` did not run and says why. "All gates green." needs an empty +# FAIL list AND an empty SKIP list, because a banner over a block that never +# executed is a false report rather than a weaker gate (#998). +# +# The exit status carries only TWO of those states, so a SKIP still exits 0 and +# a reader who wants the third has to read the report. That is right for a human +# on a branch behind main and wrong for a program: `scripts/agent-ready.py` read +# preflight by exit code alone and called a run with two unexecuted trailer gates +# "green". --fail-on-skip is the opt-in for a machine consumer -- it exits 1 when +# anything was skipped, and it does not change what any gate demands or what a +# plain run reports. Ask for it when a skip must not read as success. +# # It never writes anything, so it is always safe to run. set -uo pipefail @@ -36,6 +50,12 @@ cd "$ROOT" STAGED=0 QUIET=0 ROLE_ONLY=0 +# OFF by default, and that default is the decision argued in the spec's §3.4: a +# branch behind origin/main is ordinary work, and a gate that fires on ordinary +# work is the defect. The flag exists because the exit status cannot carry the +# third state, so every consumer that reads only the return code reads a SKIP as +# success. There was exactly one such consumer (#998). +FAIL_ON_SKIP=0 # ON by default: an undeclared session is a FAILING gate. The mutation suite # anchors on THIS line (`^REQUIRE_ROLE=1$`) and refuses any line-anchored # assignment of zero, quoted or not, so a silent revert of the default goes red @@ -49,7 +69,8 @@ for arg in "$@"; do --require-role) REQUIRE_ROLE=1 ;; --no-require-role) REQUIRE_ROLE=0 ;; --role-only) ROLE_ONLY=1 ;; - -h|--help) sed -n '2,29p' "$0" | sed 's/^# \{0,1\}//'; exit 0 ;; + --fail-on-skip) FAIL_ON_SKIP=1 ;; + -h|--help) sed -n '2,43p' "$0" | sed 's/^# \{0,1\}//'; exit 0 ;; *) echo "unknown argument: $arg" >&2; exit 2 ;; esac done @@ -123,9 +144,16 @@ SUITES=( test_check_gate_commands test_gpu_lock_one_truth test_main_baseline + test_agent_preflight_skip_report ) failed=() +# A gate that did NOT run is a THIRD state (#998). It is not `ok`, because +# nothing was verified, and it is not `FAIL`, because nothing was found wrong. +# Reporting it as neither is what let this script print "All gates green." over +# a block that never executed: three times in one session, twice because +# origin/main moved MID-RUN. See BASE_SHA below and the two range blocks. +skipped=() run() { local label="$1" @@ -139,6 +167,117 @@ run() { fi } +# The same shape as run(), for a gate this run cannot execute. The REASON prints +# with it and is not optional: a reader who learns only that something did not +# happen cannot tell a stale checkout from a broken one. +skip() { + local label="$1" + shift + printf ' \033[33mSKIP\033[0m %s\n' "$label" + printf '%s\n' "$*" | sed 's/^/ /' + skipped+=("$label") +} + +# origin/main resolved ONCE, before any gate runs. Every range block below +# compares against this SHA and never against the ref. +# +# The ref is remote-tracking and therefore SHARED by every linked worktree of +# this checkout. A fetch in any other worktree moves it while this run is in +# flight, so `--is-ancestor origin/main HEAD` answered true at the top of the +# run and false by the time the trailer block asked. The trailer gates then +# vanished from a report that still said green, and the `ok` count fell from 76 +# to 74 with no other change in the output. Pinning removes that race. Printing +# the SHA tells the reader which revision the verdict is about. +# +# Resolving to a commit (`^{commit}`) rather than to whatever the ref names +# keeps a tag or an annotated object from reaching a gate as a base. +BASE_REF="origin/main" +BASE_SHA="$(git rev-parse --verify -q "${BASE_REF}^{commit}" 2>/dev/null || true)" +BASE_UNRESOLVED="${BASE_REF} does not resolve here, so this run cannot tell which +commits are new. Fetch the remote, or name the remote that carries the base +branch, then rerun. Unknown is not absence." + +# The other two questions about the pinned base, asked ONCE and beside the pin, +# with the git exit status kept rather than collapsed. +# +# `--is-ancestor` answers 1 for "no" and 128 for "that question cannot be asked +# here" -- an unborn HEAD (`git checkout --orphan`) is one way to reach it. Both +# used to take the same arm, so the report named a cause ("is not an ancestor of +# HEAD") that was not the cause. +# +# `rev-list --count` used to be spelled `|| echo 0`, which filed a FAILED count +# under the deliberate empty-range exemption of §3.6. An empty range withholds +# nothing and keeps the banner. A count that could not be taken withholds +# everything and must not. Mapping the second onto the first is the same +# unknown-as-success conflation this row exists to remove. +# +# The count keeps stderr OUT of its value, and the value is then validated. The +# repair for `|| echo 0` was first written as `2>&1`, which reintroduced the very +# defect through a narrower door: git can write to stderr AND exit 0, so the +# status catches nothing while the value carries the error text ahead of the +# number. `[ "$RANGE_COUNT" -gt 0 ]` does not evaluate false there, it ERRORS +# with status 2, which reads as false, and both range blocks fell through to +# their empty-range arm while "All gates green." printed over five dropped +# gates. A `.git/objects/info/alternates` naming a path that does not exist is +# one way to reach it: `error: unable to normalize alternate object path: ...` +# on stderr, the count on stdout, exit 0. +# +# Discarding stderr is what makes the VALUE right, and it is also what keeps a +# git that merely warns from costing five gates a SKIP it does not deserve. +# Validating the value is what makes an arm exist at all for a count that is not +# a count: without it, any non-numeric value reaches `-gt` and its status-2 error +# is indistinguishable from "zero commits". The two halves cover different +# failures, so both are here. +# +# Discarding stderr also had a COST, and the cost is paid separately rather than +# argued away. `2>/dev/null` threw the message away along with its influence on +# the value, so an unborn HEAD reported "exited 128 and printed [] on stdout" +# while git had already named the cause in one line. That report is honest and +# not actionable, and this row owes both. The rule is message-NOT-a-value, not +# no-message: the text goes into its own variable, which nothing compares and no +# arm reads, exactly as ANCESTRY_ERROR already did one line above. +ANCESTRY_ERROR="" +ANCESTRY_STATUS=0 +RANGE_COUNT="" +RANGE_ERROR="" +RANGE_STATUS=0 +if [ -n "$BASE_SHA" ]; then + # stderr stays merged HERE deliberately. ANCESTRY_ERROR is a MESSAGE and never + # a value: nothing compares it, and only ANCESTRY_STATUS selects an arm below. + # Checked rather than assumed -- under the broken-alternates case above this + # call also writes to stderr and exits 0, and every ancestry arm is unaffected. + ANCESTRY_ERROR="$(git merge-base --is-ancestor "$BASE_SHA" HEAD 2>&1)" + ANCESTRY_STATUS=$? + # The count is asked TWICE, for two different things, and the order matters + # twice over. `2>&1 >/dev/null` duplicates stderr onto the capture BEFORE + # stdout leaves for /dev/null, so this call yields the message alone and never + # the count. The value call runs second so RANGE_STATUS still describes the + # command the value came from. Nothing selects an arm from RANGE_ERROR. + RANGE_ERROR="$(git rev-list --count "${BASE_SHA}..HEAD" 2>&1 >/dev/null)" + RANGE_COUNT="$(git rev-list --count "${BASE_SHA}..HEAD" 2>/dev/null)" + RANGE_STATUS=$? +fi +# One predicate for "this run holds a usable commit count", so the two range +# blocks below cannot drift apart on what counts as unknown. +RANGE_NUMERIC=1 +case "$RANGE_COUNT" in + '' | *[!0-9]*) RANGE_NUMERIC=0 ;; +esac +ANCESTRY_UNKNOWN="git merge-base --is-ancestor ${BASE_SHA} HEAD exited +${ANCESTRY_STATUS}, so this run could not ask whether the base is behind HEAD: +${ANCESTRY_ERROR} +An unborn HEAD is one way to reach this. Unknown is not a verdict on ancestry." +# The closing sentence stays on ONE line deliberately. The suite asserts it +# verbatim, and a rewrap that splits it across a newline turns a correct report +# into a red gate rather than into a weaker one. +RANGE_UNKNOWN="git rev-list --count ${BASE_SHA}..HEAD exited ${RANGE_STATUS} and +printed [${RANGE_COUNT}] on stdout, which is not a commit count, so this run +could not count the commits under judgement. An unborn HEAD is one way to reach +this, and so is a git that writes an error to stderr and still exits 0. git +wrote this to stderr: +${RANGE_ERROR} +Unknown is not an empty range." + echo "Session role:" if role_line=$(python3 scripts/agent-role.py show 2>&1); then printf ' \033[32mok\033[0m %s\n' "$role_line" @@ -211,26 +350,70 @@ run "commit style suites" python3 -m unittest \ # unchecked for a whole series and PR #80 landed eight commits that reddened # documentation-checkpoint on main, where a diff-scoped range is never re-covered. # Gating this on --staged would reproduce that hole exactly. -if git rev-parse --verify -q origin/main >/dev/null 2>&1 && - [ "$(git rev-list --count origin/main..HEAD 2>/dev/null || echo 0)" -gt 0 ]; then - echo "Committed range vs origin/main:" +# +# An EMPTY range is not a skipped gate and does not print SKIP. A verdict over +# zero commits withholds nothing, and reporting it as a skip would fire on the +# ordinary session-start run of a freshly cut branch. An UNRESOLVABLE base is +# the other case entirely: there are commits to judge and this run cannot tell +# which, so it reports SKIP and forfeits the banner. +if [ -z "$BASE_SHA" ]; then + echo "Committed range vs ${BASE_REF}:" + skip "now-current range" "$BASE_UNRESOLVED" + skip "doc-checkpoint range" "$BASE_UNRESOLVED" + skip "issue-index append-only" "$BASE_UNRESOLVED" +elif [ "$RANGE_STATUS" -ne 0 ] || [ "$RANGE_NUMERIC" -eq 0 ]; then + echo "Committed range vs ${BASE_REF} ${BASE_SHA}:" + skip "now-current range" "$RANGE_UNKNOWN" + skip "doc-checkpoint range" "$RANGE_UNKNOWN" + skip "issue-index append-only" "$RANGE_UNKNOWN" +elif [ "$RANGE_COUNT" -gt 0 ]; then + echo "Committed range vs ${BASE_REF} ${BASE_SHA}:" run "now-current range" python3 scripts/check-now-current.py \ - --base origin/main --head HEAD + --base "$BASE_SHA" --head HEAD run "doc-checkpoint range" python3 scripts/check-doc-checkpoint.py \ - --base origin/main --head HEAD + --base "$BASE_SHA" --head HEAD run "issue-index append-only" python3 scripts/check-issue-index-append-only.py \ - --base origin/main --head HEAD + --base "$BASE_SHA" --head HEAD +else + echo "Committed range vs ${BASE_REF} ${BASE_SHA}: empty, HEAD adds no commits." fi # Trailer enforcement reads only committed Git objects. -if git rev-parse --verify -q origin/main >/dev/null 2>&1 && - git merge-base --is-ancestor origin/main HEAD && - [ "$(git rev-list --count origin/main..HEAD 2>/dev/null || echo 0)" -gt 0 ]; then - echo "Commit trailers vs origin/main:" +# +# The ancestry arm is the one #998 was filed for. It USED to be spelled as a +# silent `&&` in the condition, so a base that was not an ancestor deleted both +# gates from the report and the run still printed "All gates green.". The guard +# itself stays, because check-commit-style.py refuses a non-ancestor base at +# validate_range (#999 owes that repair, and until it lands, dropping the guard +# would turn every branch behind main RED instead of honest). What changes is +# that the skip now SAYS SO and costs the banner. +if [ -z "$BASE_SHA" ]; then + echo "Commit trailers vs ${BASE_REF}:" + skip "commit-trailers" "$BASE_UNRESOLVED" + skip "commit-style" "$BASE_UNRESOLVED" +elif [ "$ANCESTRY_STATUS" -gt 1 ]; then + echo "Commit trailers vs ${BASE_REF} ${BASE_SHA}:" + skip "commit-trailers" "$ANCESTRY_UNKNOWN" + skip "commit-style" "$ANCESTRY_UNKNOWN" +elif [ "$ANCESTRY_STATUS" -ne 0 ]; then + echo "Commit trailers vs ${BASE_REF} ${BASE_SHA}:" + TRAILER_BEHIND="${BASE_REF} ${BASE_SHA} is not an ancestor of HEAD, so this +branch is behind it and the trailer gates did NOT run. Merge ${BASE_REF} and +rerun. Neither gate reported anything about this tree." + skip "commit-trailers" "$TRAILER_BEHIND" + skip "commit-style" "$TRAILER_BEHIND" +elif [ "$RANGE_STATUS" -ne 0 ] || [ "$RANGE_NUMERIC" -eq 0 ]; then + echo "Commit trailers vs ${BASE_REF} ${BASE_SHA}:" + skip "commit-trailers" "$RANGE_UNKNOWN" + skip "commit-style" "$RANGE_UNKNOWN" +elif [ "$RANGE_COUNT" -gt 0 ]; then + echo "Commit trailers vs ${BASE_REF} ${BASE_SHA}:" run "commit-trailers" python3 scripts/check-commit-trailers.py \ - --range "origin/main..HEAD" + --range "${BASE_SHA}..HEAD" run "commit-style" python3 scripts/check-commit-style.py \ - --range "origin/main..HEAD" + --range "${BASE_SHA}..HEAD" +else + echo "Commit trailers vs ${BASE_REF} ${BASE_SHA}: empty, HEAD adds no commits." fi if [ "$STAGED" -eq 1 ]; then @@ -239,6 +422,15 @@ if [ "$STAGED" -eq 1 ]; then run "now-current --staged" python3 scripts/check-now-current.py --staged fi +# Printed BEFORE the failure summary and outside it, so a run that both failed +# and skipped reports both facts. They are different facts. +if [ "${#skipped[@]}" -ne 0 ]; then + echo + echo "${#skipped[@]} gate(s) SKIPPED: ${skipped[*]}" + echo "NOT a green preflight: a skipped gate reported nothing about this tree." + echo "Each reason is printed beside its SKIP above." +fi + if [ "${#failed[@]}" -ne 0 ]; then echo echo "${#failed[@]} gate(s) failed: ${failed[*]}" @@ -246,8 +438,27 @@ if [ "${#failed[@]}" -ne 0 ]; then exit 1 fi -echo -echo "All gates green." +# The opt-in, and the only thing that changes the exit status for a skip. The +# report above already says everything this line says. A caller reaches here +# because it reads the STATUS and not the report, which is what agent-ready.py +# did while printing the word "green" over two gates that never ran. +if [ "${#skipped[@]}" -ne 0 ] && [ "$FAIL_ON_SKIP" -eq 1 ]; then + echo + echo "--fail-on-skip: ${#skipped[@]} gate(s) did not run, so this run cannot" + echo "answer the question that was asked of it. Exit 1 for the caller that" + echo "reads only the status. Nothing here failed, and nothing here passed." + exit 1 +fi + +# Reachable ONLY when both arrays are empty. A skip used to survive this line, +# which made the banner a claim the run had not earned (#998). The DEFAULT exit +# status stays 0 for a skip: a branch behind origin/main is ordinary work, and +# exit 1 would merge "a gate did not run" into the signal that means "a gate ran +# and failed". A caller that cannot read the report asks for --fail-on-skip. +if [ "${#skipped[@]}" -eq 0 ]; then + echo + echo "All gates green." +fi if [ "$QUIET" -eq 0 ]; then echo diff --git a/scripts/agent-ready.py b/scripts/agent-ready.py index c184ac429..49451c9f2 100755 --- a/scripts/agent-ready.py +++ b/scripts/agent-ready.py @@ -128,8 +128,23 @@ def ready_errors(payload: dict[str, Any], expected: dict[str, str]) -> list[str] def run_local_preflight() -> bool: + """Run preflight, and refuse a run that SKIPPED a gate as well as one that failed. + + This reads preflight by exit status alone, which carries two of the three + states preflight reports. Without `--fail-on-skip` a run whose base does not + resolve, or whose branch is behind `origin/main`, exits 0 with up to five + gates never executed, and this function returned True for it. The line below + then printed the word "green" over a trailer check that had not run (#998). + + The flag is opt-in precisely so a human running preflight on a branch behind + `main` still gets exit 0. This caller is not that human. It is the documented + gate before a remote handoff, so an unknown here has to read as "not ready" + rather than as success. + """ + return subprocess.run( - [str(ROOT / "scripts/agent-preflight.sh"), "--quiet"], cwd=ROOT + [str(ROOT / "scripts/agent-preflight.sh"), "--quiet", "--fail-on-skip"], + cwd=ROOT, ).returncode == 0 @@ -138,7 +153,12 @@ def main() -> int: parser.add_argument("--pr-json", type=Path) args = parser.parse_args() if not run_local_preflight(): - print("READY FAILED: local preflight is red", file=sys.stderr) + print( + "READY FAILED: local preflight did not report every gate green. " + "A gate FAILED, or a gate was SKIPPED and reported nothing about " + "this tree. Its report above says which, and why.", + file=sys.stderr, + ) return 1 try: expected = local_expected() diff --git a/tests/scripts/test_agent_preflight_skip_report.py b/tests/scripts/test_agent_preflight_skip_report.py new file mode 100644 index 000000000..1abd930e4 --- /dev/null +++ b/tests/scripts/test_agent_preflight_skip_report.py @@ -0,0 +1,1000 @@ +#!/usr/bin/env python3 +"""`scripts/agent-preflight.sh` never reports green over a block that did not run. + +Row `GATE-PREFLIGHT-SKIP-REPORT`, issue #998, spec +`.agents/specs/gate-preflight-skip-report.md`. + +The defect this suite pins: the trailer block was guarded by +`git merge-base --is-ancestor origin/main HEAD`, and when that guard was false +the block emitted NOTHING and the run still printed `All gates green.` and +exited 0. Two gates left the report without a word. It fired three times in one +session, twice because `origin/main` is a remote-tracking ref that every +worktree of one checkout shares, so another worktree's fetch moved it BETWEEN +the top of the run and the guard. On the third the `ok` count fell from 76 to 74 +and the banner did not change. + +That last number is the whole problem in one line. Nothing in the output +distinguished a run that checked the trailers from a run that did not, so the +only reader who could catch it was one who read the script. + +## Why this suite executes the script instead of grepping it + +Every assertion here could be spelled as a text match against the script, and +every one of them would then be satisfied by a REWRITE while missing an +OVERRIDE on a later line. `tests/scripts/test_agent_onboard.py` already learned +that lesson about `REQUIRE_ROLE` and switched to executing the script. The same +reasoning applies harder here, because what is under test is control flow across +four `git` calls, not the value of a variable. + +## Why it can execute the script without recursing + +`scripts/agent-preflight.sh` runs this very suite from its `SUITES` array, so a +nested full run would recurse without bound. `--role-only` exists for that +problem and is too narrow here, because the blocks under test are the last two +in the file. + +So the run is pointed at a SCRATCH repository instead: the script is copied into +a temporary git repo and a stub `python3` that exits 0 is put on `PATH`. Every +`run` then reports `ok` without executing a checker, nothing re-enters this +suite, and the git-shaped control flow under test executes for real against refs +the test owns. `ROOT` in the script is derived from `BASH_SOURCE`, so the copy +makes the scratch repo the script's own tree. +""" + +from __future__ import annotations + +import os +import re +import shutil +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + + +ROOT = Path(__file__).resolve().parents[2] +PREFLIGHT = ROOT / "scripts/agent-preflight.sh" +READY = ROOT / "scripts/agent-ready.py" +CI = ROOT / ".github/workflows/ci.yml" +SUITE_NAME = Path(__file__).stem + +# Resolved BEFORE any test prepends its scratch `bin` to PATH, so the shim in +# `NON_NUMERIC_GIT` can forward to the real program rather than to itself. +GIT = shutil.which("git") + +ANSI = re.compile(r"\x1b\[[0-9;]*m") + +# Exits 0 for every checker and every suite. The script's `run()` reports `ok`. +# +# It also RECORDS its argv, one invocation per line, when the test asks for it. +# Without that, this suite pins the ancestry decision against the pinned SHA and +# says nothing about the SHA the five range checkers are actually handed: putting +# `--base origin/main` back on all three range gates, and `--range +# origin/main..HEAD` back on both trailer gates, parses and leaves every other +# case here green. The script's own comment above `BASE_REF` states the guarantee +# ("Every range block below compares against this SHA and never against the +# ref"), and nothing detected its loss. `scripts/check-test-registration.py` +# traces preflight the same way for the same reason. +INERT_PYTHON3 = """#!/bin/sh +if [ -n "${VLLM_TEST_ARGV_LOG:-}" ]; then + printf '%s\\n' "$*" >> "$VLLM_TEST_ARGV_LOG" +fi +exit 0 +""" + +# The five gates that take the pinned base as an argument, by the script name +# each one is invoked with. Every one of them must be handed `BASE_SHA`. +BASE_ARGUMENT_GATES = ( + "scripts/check-now-current.py", + "scripts/check-doc-checkpoint.py", + "scripts/check-issue-index-append-only.py", + "scripts/check-commit-trailers.py", + "scripts/check-commit-style.py", +) + +# Advances `refs/remotes/origin/main` on its FIRST call and then goes inert. +# This is what another worktree of the same checkout does when it fetches while +# a preflight is running, and it is occurrences 2 and 3 of #998 reproduced +# deterministically. `cd "$ROOT"` at the top of the script means the stub's cwd +# is the scratch repo, so the ref it moves is the one the script reads. +MOVING_PYTHON3 = """#!/bin/sh +if [ ! -e "$VLLM_TEST_MOVE_MARKER" ]; then + : > "$VLLM_TEST_MOVE_MARKER" + git update-ref refs/remotes/origin/main "$VLLM_TEST_MOVE_TO" +fi +exit 0 +""" + +# Answers `rev-list` with a line that is not a number and exits 0, and forwards +# every other subcommand to the real program. No git known to this suite behaves +# this way, and that is the point: the script must hold an arm for a count it +# cannot read, rather than inferring from exit 0 that stdout is a decimal +# integer. That inference is what produced the defect this class pins. +NON_NUMERIC_GIT = """#!/bin/sh +if [ "$1" = "rev-list" ]; then + echo "not-a-commit-count" + exit 0 +fi +exec {git} "$@" +""" + +# A path that does not exist, named as an alternate object database. Git prints +# `error: unable to normalize alternate object path: ...` to stderr on nearly +# every object-reading command, writes its ordinary answer to stdout, and exits +# 0. It is the cheapest reproduction of "stderr without a failure" and it needs +# no shim at all. +MISSING_ALTERNATE = "/nonexistent/vllm-cpp-preflight-alternate/objects" + + +class Report: + """One preflight run, parsed into the three states it can report.""" + + def __init__(self, returncode: int, text: str) -> None: + self.returncode = returncode + self.text = ANSI.sub("", text) + lines = self.text.splitlines() + self.ok = [line[7:] for line in lines if line.startswith(" ok ")] + self.fail = [line[7:] for line in lines if line.startswith(" FAIL ")] + self.skip = [line[7:] for line in lines if line.startswith(" SKIP ")] + self.headings = [line for line in lines if re.fullmatch(r"[A-Z][^\n]*:", line)] + + @property + def green(self) -> bool: + return "All gates green." in self.text + + def __str__(self) -> str: # shown verbatim on any failure below + return self.text + + +class PreflightHarness(unittest.TestCase): + """A scratch repo with a known ref topology and an inert `python3`.""" + + def setUp(self) -> None: + self.tmp = Path(tempfile.mkdtemp(prefix="preflight-skip-")) + self.addCleanup(shutil.rmtree, self.tmp, True) + (self.tmp / "scripts").mkdir() + (self.tmp / "bin").mkdir() + shutil.copy2(PREFLIGHT, self.tmp / "scripts" / PREFLIGHT.name) + self.script = self.tmp / "scripts" / PREFLIGHT.name + self.argv_log = self.tmp / "argv.log" + self.set_python3(INERT_PYTHON3) + + self.git("init", "--quiet", ".") + self.git("config", "user.email", "preflight@test.invalid") + self.git("config", "user.name", "preflight test") + self.git("config", "commit.gpgsign", "false") + (self.tmp / "a").write_text("a\n", encoding="utf-8") + self.git("add", "-A") + self.git("commit", "--quiet", "-m", "base") + self.base = self.rev("HEAD") + + # A divergent commit that HEAD does NOT contain. This is what + # `origin/main` looks like from a branch that is behind it. + self.git("checkout", "--quiet", "-b", "divergent") + (self.tmp / "b").write_text("b\n", encoding="utf-8") + self.git("add", "-A") + self.git("commit", "--quiet", "-m", "divergent") + self.divergent = self.rev("HEAD") + + self.git("checkout", "--quiet", "-B", "work", self.base) + (self.tmp / "c").write_text("c\n", encoding="utf-8") + self.git("add", "-A") + self.git("commit", "--quiet", "-m", "work") + self.head = self.rev("HEAD") + + self.set_origin_main(self.base) + + # -- scratch repo helpers ------------------------------------------------ + + def git(self, *args: str) -> str: + result = subprocess.run( + ["git", *args], cwd=self.tmp, capture_output=True, text=True, check=True + ) + return result.stdout.strip() + + def rev(self, revision: str) -> str: + return self.git("rev-parse", revision) + + def set_origin_main(self, oid: str) -> None: + self.git("update-ref", "refs/remotes/origin/main", oid) + + def unset_origin_main(self) -> None: + self.git("update-ref", "-d", "refs/remotes/origin/main") + + def set_python3(self, body: str) -> None: + stub = self.tmp / "bin" / "python3" + stub.write_text(body, encoding="utf-8") + stub.chmod(0o755) + + def environment(self, **env: str) -> dict[str, str]: + environment = dict(os.environ) + environment["PATH"] = f"{self.tmp / 'bin'}{os.pathsep}{environment['PATH']}" + # Truncated per run, so a log always describes exactly one invocation of + # the script even in the cases that run it twice. + self.argv_log.write_text("", encoding="utf-8") + environment["VLLM_TEST_ARGV_LOG"] = str(self.argv_log) + environment.update(env) + return environment + + def preflight(self, *args: str, **env: str) -> Report: + result = subprocess.run( + ["bash", str(self.script), "--quiet", *args], + cwd=self.tmp, + capture_output=True, + text=True, + check=False, + env=self.environment(**env), + ) + return Report(result.returncode, result.stdout + result.stderr) + + def base_arguments(self, gate: str) -> list[str]: + """Every recorded `python3` invocation of `gate` that carried a base.""" + + recorded = self.argv_log.read_text(encoding="utf-8").splitlines() + return [ + line + for line in recorded + if line.startswith(gate) and ("--base " in line or "--range " in line) + ] + + def skip_reason(self, report: Report, label: str) -> str: + """The indented reason block printed under one SKIP label. + + `report.skip` carries the labels only, so an assertion made against + `report.text` cannot tell a reason printed under the gate it explains + from the same words printed anywhere else in the run. This reads the + block `skip()` indents by nine columns, which is the reason and nothing + else. + """ + + lines = report.text.splitlines() + for index, line in enumerate(lines): + if not (line.startswith(" SKIP ") and line[7:].strip() == label): + continue + reason = [] + for following in lines[index + 1 :]: + if not following.startswith(" "): + break + reason.append(following[9:]) + return "\n".join(reason) + return "" + + # -- preconditions ------------------------------------------------------- + + def assert_ran_something(self, report: Report) -> None: + """A run that reported nothing at all asserts nothing at all. + + The stub `python3` makes every gate cheap, and a broken harness makes + every gate ABSENT. Both produce a short transcript with no `FAIL`, and + only this check tells them apart. `assertEqual([], report.fail)` on its + own is satisfied by a run that never started. + """ + + self.assertGreater( + len(report.ok), + 40, + f"harness precondition failed: only {len(report.ok)} gates reported " + f"`ok`, so this run asserts nothing.\n{report}", + ) + self.assertEqual([], report.fail, f"scratch run had failures:\n{report}") + + +class SkipIsReportedTests(PreflightHarness): + def test_a_non_ancestor_base_reports_skip_and_no_green_banner(self) -> None: + """RED before: prints `All gates green.` with the trailer block absent. + + Occurrence 1 of #998: a branch behind `main`. The two trailer gates were + not run, nothing said so, and the banner claimed otherwise. + """ + + self.set_origin_main(self.divergent) + report = self.preflight() + self.assert_ran_something(report) + + self.assertTrue( + any("commit-trailers" in line for line in report.skip), + f"the trailer block did not report a SKIP:\n{report}", + ) + self.assertTrue( + any("commit-style" in line for line in report.skip), + f"the style gate did not report a SKIP:\n{report}", + ) + self.assertFalse( + report.green, + f"a run that skipped {len(report.skip)} gate(s) still printed the " + f"green banner:\n{report}", + ) + self.assertIn( + "SKIPPED", + report.text, + f"the summary does not count the skipped gates:\n{report}", + ) + # The reason has to travel with the skip, or the reader learns only that + # something did not happen. + self.assertIn( + "ancestor", + report.text, + f"no reason printed beside the SKIP:\n{report}", + ) + + def test_the_ok_count_falls_only_with_a_reported_skip(self) -> None: + """RED before: the count falls by two and there are zero SKIP lines. + + This is occurrence 3 exactly, and it is the case that makes a SILENT + drop impossible rather than merely unlikely. Asserting the presence of a + SKIP line elsewhere does not cover it: a future block could vanish the + same way and every other case here would stay green. + """ + + self.set_origin_main(self.base) + ancestor = self.preflight() + self.assert_ran_something(ancestor) + self.assertEqual([], ancestor.skip, f"the control run skipped:\n{ancestor}") + + self.set_origin_main(self.divergent) + behind = self.preflight() + self.assert_ran_something(behind) + + missing = len(ancestor.ok) - len(behind.ok) + self.assertGreater( + missing, + 0, + "harness precondition failed: the two runs report the same number " + f"of gates, so there is no drop to account for.\n{behind}", + ) + self.assertEqual( + missing, + len(behind.skip), + f"{missing} gate(s) disappeared from the report and " + f"{len(behind.skip)} were reported as skipped. Every gate that " + f"stops running has to say so.\n{behind}", + ) + + def test_a_base_that_moves_mid_run_does_not_change_the_verdict(self) -> None: + """RED before: the trailer block vanishes and the run still says green. + + Occurrences 2 and 3. `origin/main` is shared by every worktree of one + checkout, so the guard was true when the run started and false when it + was evaluated. Pinning the base at the start is what fixes it, and the + run has to name the SHA it pinned. + """ + + marker = self.tmp / "moved" + self.set_python3(MOVING_PYTHON3) + self.set_origin_main(self.base) + report = self.preflight( + VLLM_TEST_MOVE_MARKER=str(marker), + VLLM_TEST_MOVE_TO=self.divergent, + ) + self.assert_ran_something(report) + + # PRECONDITION, asserted and never skipped: if the stub never moved the + # ref then this test reproduces nothing and must go red. + self.assertTrue( + marker.exists(), + "precondition failed: the stub python3 never ran, so origin/main " + f"never moved and this test asserts nothing.\n{report}", + ) + self.assertEqual( + self.divergent, + self.rev("refs/remotes/origin/main"), + f"precondition failed: origin/main did not move.\n{report}", + ) + + self.assertIn( + self.base, + report.text, + "the run does not name the base SHA it gated against, so a reader " + f"cannot tell which revision it compared to.\n{report}", + ) + self.assertEqual( + [], + report.skip, + "the base was pinned to an ancestor at the start of the run, so no " + f"block should have been skipped when the ref moved:\n{report}", + ) + self.assertTrue( + any("commit-trailers" in line for line in report.ok), + f"the trailer gate did not run against the pinned base:\n{report}", + ) + self.assertTrue(report.green, f"the run earned the banner and withheld it:\n{report}") + + def test_an_unresolvable_base_reports_skip_for_both_range_blocks(self) -> None: + """RED before: five gates vanish and the run says green. + + The same shape in the `Committed range` block. A checkout whose remote + is not named `origin`, or one that has never fetched, resolves no + `origin/main`. `AGENTS.md`: "Unknown is not absence or success." + """ + + self.unset_origin_main() + report = self.preflight() + self.assert_ran_something(report) + + for label in ( + "now-current range", + "doc-checkpoint range", + "issue-index append-only", + "commit-trailers", + "commit-style", + ): + with self.subTest(gate=label): + self.assertTrue( + any(label in line for line in report.skip), + f"{label} vanished without reporting a SKIP:\n{report}", + ) + self.assertFalse(report.green, f"five skipped gates still printed green:\n{report}") + + +class TheBannerStaysReachableTests(PreflightHarness): + def test_an_ancestor_base_still_earns_the_banner(self) -> None: + """The control. A fix that suppresses the banner always cannot pass.""" + + self.set_origin_main(self.base) + report = self.preflight() + self.assert_ran_something(report) + self.assertEqual([], report.skip, f"an ordinary run skipped a gate:\n{report}") + self.assertTrue(report.green, f"an ordinary run lost the banner:\n{report}") + self.assertEqual(0, report.returncode, f"an ordinary run exited non-zero:\n{report}") + for label in ("commit-trailers", "commit-style"): + with self.subTest(gate=label): + self.assertTrue( + any(label in line for line in report.ok), + f"{label} did not run on an ancestor base:\n{report}", + ) + + def test_an_empty_range_is_not_a_skip(self) -> None: + """An empty input set is not an unexamined one. + + `HEAD` level with the base means the range gates have zero commits to + read, and a verdict over zero commits is not withheld information. If + this reported `SKIP`, the ordinary session-start run of a freshly cut + branch would print a false alarm and lose the banner, and + `AGENTS.md` is explicit that a gate firing on ordinary work is the + defect. Without this case the obvious over-correction passes every + other test in this file. + """ + + self.set_origin_main(self.head) + report = self.preflight() + self.assert_ran_something(report) + self.assertEqual([], report.skip, f"an empty range reported a SKIP:\n{report}") + self.assertTrue(report.green, f"an empty range lost the banner:\n{report}") + self.assertEqual(0, report.returncode, f"an empty range exited non-zero:\n{report}") + + def test_a_skipped_run_exits_zero_and_a_failing_run_does_not(self) -> None: + """SKIP and FAIL are different facts and keep different exit codes. + + A branch behind `main` is ordinary work, so a skip does not fail the + run. Widening exit 1 to mean "a gate did not run" would merge the two + facts into one signal, which is the conflation this row removes. + """ + + self.set_origin_main(self.divergent) + skipped = self.preflight() + self.assert_ran_something(skipped) + self.assertNotEqual([], skipped.skip, f"nothing was skipped:\n{skipped}") + self.assertEqual( + 0, + skipped.returncode, + f"a skipped block failed the run:\n{skipped}", + ) + + # And a real failure still exits 1, so exit 0 above is a decision rather + # than a script that cannot fail. + self.set_python3("#!/bin/sh\nexit 1\n") + failing = self.preflight() + self.assertNotEqual( + 0, + failing.returncode, + f"every gate failed and the run exited 0:\n{failing}", + ) + self.assertFalse(failing.green, f"a failing run printed the banner:\n{failing}") + + +class ThePinnedBaseReachesTheCheckersTests(PreflightHarness): + def test_every_range_gate_is_handed_the_pinned_sha_and_never_the_ref(self) -> None: + """The pin is worth nothing if the checkers are still passed the ref. + + Every other case in this file reads the script's REPORT, so all of them + stay green when `--base "$BASE_SHA"` reverts to `--base origin/main` on + the three range gates or `--range "${BASE_SHA}..HEAD"` reverts to + `--range origin/main..HEAD` on the two trailer gates. The ancestry + decision would still be made against the pinned SHA, the heading would + still name it, and the five checkers would judge whatever the ref points + at when each one starts. With `origin/main` moving mid-run, that is a + report whose heading and whose verdict are about different revisions. + """ + + self.set_origin_main(self.base) + report = self.preflight() + self.assert_ran_something(report) + self.assertEqual([], report.skip, f"an ordinary run skipped a gate:\n{report}") + + recorded = {gate: self.base_arguments(gate) for gate in BASE_ARGUMENT_GATES} + + # PRECONDITION: a log that recorded nothing satisfies every `assertNotIn` + # below. Count first, and count ALL of them, so a gate that stops being + # invoked at all cannot pass as a gate invoked correctly. + self.assertEqual( + 5, + sum(len(lines) for lines in recorded.values()), + f"precondition failed: the stub recorded {recorded}, which is not " + f"one base-carrying invocation per gate.\n{report}", + ) + + for gate, lines in recorded.items(): + with self.subTest(gate=gate): + self.assertEqual( + 1, + len(lines), + f"{gate} was invoked with a base {len(lines)} time(s): {lines}", + ) + self.assertIn( + self.base, + lines[0], + f"{gate} was not handed the pinned SHA {self.base}: {lines[0]}", + ) + self.assertNotIn( + "origin/main", + lines[0], + f"{gate} was handed the moving ref instead of the pinned " + f"SHA: {lines[0]}", + ) + + +class AnUnknownIsNotAnEmptyRangeTests(PreflightHarness): + def test_an_unborn_head_reports_skip_rather_than_an_empty_range(self) -> None: + """RED before: three gates take the empty-range exemption and say nothing. + + `git rev-list --count` was spelled `|| echo 0`, which maps a FAILED + count onto the deliberate exemption for a range with no commits in it. + The two are opposite facts: an empty range withholds nothing, and a + count that could not be taken withholds everything. `git checkout + --orphan` reaches it with `origin/main` perfectly resolvable. + + The same run also pins the ancestry reason. `--is-ancestor` answers 1 + for "no" and 128 for "that question cannot be asked here", and both used + to take the arm that says the branch is behind `origin/main`. That names + a cause which is not the cause, and it sends the reader to `git merge` + for a tree that has no commit to merge into. + """ + + self.set_origin_main(self.base) + self.git("checkout", "--orphan", "unborn") + + # PRECONDITION: this reproduces nothing unless HEAD really is unborn + # while the base still resolves. + self.assertNotEqual( + 0, + subprocess.run( + ["git", "rev-parse", "--verify", "-q", "HEAD"], + cwd=self.tmp, capture_output=True, text=True, check=False, + ).returncode, + "precondition failed: HEAD still resolves, so the git queries under " + "test do not fail and this test asserts nothing.", + ) + + report = self.preflight() + self.assert_ran_something(report) + + for label in ( + "now-current range", + "doc-checkpoint range", + "issue-index append-only", + "commit-trailers", + "commit-style", + ): + with self.subTest(gate=label): + self.assertTrue( + any(label in line for line in report.skip), + f"{label} did not report a SKIP:\n{report}", + ) + self.assertFalse(report.green, f"five unknown gates printed green:\n{report}") + self.assertNotIn( + "empty, HEAD adds no commits", + report.text, + f"a failed count was reported as an empty range:\n{report}", + ) + self.assertNotIn( + "is not an ancestor of HEAD", + report.text, + "a failed ancestry query was reported as a branch behind the base, " + f"which names the wrong cause:\n{report}", + ) + self.assertIn( + "Unknown is not an empty range.", + report.text, + f"the range skip does not say what is unknown:\n{report}", + ) + self.assertIn( + "Unknown is not a verdict on ancestry.", + report.text, + f"the ancestry skip does not say what is unknown:\n{report}", + ) + + def test_a_range_skip_carries_the_message_git_printed(self) -> None: + """RED before: the reason named a status and showed an empty value. + + Discarding stderr is what keeps it out of the VALUE, and `2>/dev/null` + also threw the message away. The reader was told that the count + `exited 128 and printed [] on stdout` and never told why, although git + had already said why in one line. A skip that names no cause is honest + and not actionable, and this row owes both halves. + + The fix is the discipline the same block already uses two lines above + for `ANCESTRY_ERROR`: the message goes into its OWN variable, never into + the value that an arm is selected from. `2>&1 >/dev/null` is the + ordering that yields the message alone, because stderr is duplicated + onto the capture before stdout leaves for `/dev/null`. + """ + + self.set_origin_main(self.base) + self.git("checkout", "--orphan", "unborn") + + # PRECONDITION: git has to actually write a diagnostic to stderr for the + # exact command the script runs. If it prints nothing there is no + # message to carry and this case asserts nothing. + probe = subprocess.run( + ["git", "rev-list", "--count", f"{self.base}..HEAD"], + cwd=self.tmp, capture_output=True, text=True, check=False, + ) + self.assertNotEqual( + 0, + probe.returncode, + "precondition failed: the count succeeded, so no skip is reported " + "and this case asserts nothing.", + ) + printed = probe.stderr.strip().splitlines() + self.assertNotEqual( + [], + printed, + "precondition failed: git wrote nothing to stderr, so there is no " + "message for the reason to carry.", + ) + message = printed[0] + self.assertIn( + "fatal:", + message, + f"precondition failed: git's first stderr line is not a diagnostic: {message}", + ) + + report = self.preflight() + self.assert_ran_something(report) + + # The trailer gates take the ANCESTRY arm here (`--is-ancestor` exits + # 128 on an unborn HEAD), so `RANGE_UNKNOWN` reaches exactly these + # three. Asserting it on the other two would pass on the wrong message. + for label in ( + "now-current range", + "doc-checkpoint range", + "issue-index append-only", + ): + with self.subTest(gate=label): + reason = self.skip_reason(report, label) + self.assertNotEqual( + "", + reason, + f"{label} reported no SKIP with a reason:\n{report}", + ) + self.assertIn( + message, + reason, + f"{label} skipped without the message git printed, so the " + f"reader is told a status and not a cause:\n{report}", + ) + + +class StderrIsNotTheValueTests(PreflightHarness): + """A git that writes to stderr and exits 0 must not delete a range block. + + The `|| echo 0` repair above was first written as + `RANGE_COUNT="$(git rev-list --count ... 2>&1)"`, which reintroduced the + silence this row exists to remove, through a narrower door and in the + DISHONEST direction. Folding stderr into the value leaves `RANGE_STATUS` + catching only the case where git FAILS. When git exits 0 and also writes to + stderr, the status is 0 and the value is the error text with the number + after it, so `[ "$RANGE_COUNT" -gt 0 ]` does not evaluate false, it ERRORS + with status 2. A `[` that errors reads as false, both range blocks take + their empty-range arm, and the run prints `All gates green.` over five gates + that never executed. + + Neither case here is exotic. `test_an_unborn_head_...` above covers a git + that fails. Nothing covered a git that succeeds noisily, and the pre-repair + script handled it correctly because it discarded stderr. + """ + + def break_alternates(self) -> None: + alternates = self.tmp / ".git" / "objects" / "info" / "alternates" + alternates.parent.mkdir(parents=True, exist_ok=True) + alternates.write_text(f"{MISSING_ALTERNATE}\n", encoding="utf-8") + + def raw_git(self, *args: str) -> subprocess.CompletedProcess[str]: + """A git run the way the script runs it, through the scratch PATH.""" + + return subprocess.run( + ["git", *args], + cwd=self.tmp, + capture_output=True, + text=True, + check=False, + env=self.environment(), + ) + + def test_a_git_that_warns_and_exits_zero_still_runs_every_range_gate(self) -> None: + """RED before: five gates vanish, `empty` is printed, the banner prints. + + The count is perfectly readable on stdout here, so the correct report is + not a SKIP. It is the ordinary run, with all five gates executed. A + repair that answered this case with a SKIP would cost five gates to a + stderr line that changed no answer. + """ + + self.set_origin_main(self.base) + control = self.preflight() + self.assert_ran_something(control) + self.assertEqual([], control.skip, f"the control run skipped:\n{control}") + + self.break_alternates() + + # PRECONDITION, asserted and never assumed: this reproduces nothing + # unless git really does exit 0, write to stderr, and print the count. + probe = self.raw_git("rev-list", "--count", f"{self.base}..HEAD") + self.assertEqual( + 0, + probe.returncode, + "precondition failed: git did not exit 0, so this case is the " + f"already-covered failing-git case instead: {probe.stderr}", + ) + self.assertNotEqual( + "", + probe.stderr.strip(), + "precondition failed: git wrote nothing to stderr, so there is no " + "stderr for the value to absorb and this case asserts nothing.", + ) + self.assertEqual( + "1", + probe.stdout.strip(), + "precondition failed: stdout is not the count, so the repair under " + "test is not the one being exercised.", + ) + + report = self.preflight() + self.assert_ran_something(report) + + for label in ( + "now-current range", + "doc-checkpoint range", + "issue-index append-only", + "commit-trailers", + "commit-style", + ): + with self.subTest(gate=label): + self.assertTrue( + any(label in line for line in report.ok), + f"{label} did not run although the count was readable on " + f"stdout:\n{report}", + ) + self.assertEqual([], report.skip, f"a readable count reported a SKIP:\n{report}") + self.assertNotIn( + "empty, HEAD adds no commits", + report.text, + f"HEAD adds a commit and the run reported an empty range:\n{report}", + ) + # The same invariant as the ok-count case, against the same control: a + # gate may leave the report only by saying that it did. + self.assertEqual( + len(control.ok), + len(report.ok), + f"{len(control.ok) - len(report.ok)} gate(s) disappeared and " + f"{len(report.skip)} were reported as skipped:\n{report}", + ) + self.assertTrue(report.green, f"the run earned the banner and withheld it:\n{report}") + + def test_a_count_that_is_not_a_number_reports_skip_rather_than_empty(self) -> None: + """RED before, and red against a repair that only discards stderr. + + Keeping stderr out of the value fixes the case above and leaves this arm + missing: any value that is not a decimal integer still reaches `-gt`, + where the status-2 error is indistinguishable from "zero commits". The + exit status is not evidence about stdout, and reading it as evidence is + the assumption that produced this defect twice. + """ + + self.assertIsNotNone(GIT, "precondition failed: no git on PATH to forward to") + shim = self.tmp / "bin" / "git" + shim.write_text(NON_NUMERIC_GIT.format(git=GIT), encoding="utf-8") + shim.chmod(0o755) + self.set_origin_main(self.base) + + # PRECONDITION: the shim has to be the git the script finds, it has to + # exit 0, and it has to answer with something that is not a count. + probe = self.raw_git("rev-list", "--count", f"{self.base}..HEAD") + self.assertEqual(0, probe.returncode, f"precondition failed: {probe.stderr}") + self.assertEqual("not-a-commit-count", probe.stdout.strip()) + # And it must still forward everything else, or the run under test fails + # for a reason that has nothing to do with the count. + self.assertEqual(self.base, self.raw_git("rev-parse", "refs/remotes/origin/main").stdout.strip()) + + report = self.preflight() + self.assert_ran_something(report) + + for label in ( + "now-current range", + "doc-checkpoint range", + "issue-index append-only", + "commit-trailers", + "commit-style", + ): + with self.subTest(gate=label): + self.assertTrue( + any(label in line for line in report.skip), + f"{label} did not report a SKIP for a count that is not a " + f"count:\n{report}", + ) + self.assertFalse(report.green, f"five unknown gates printed green:\n{report}") + self.assertNotIn( + "empty, HEAD adds no commits", + report.text, + f"an unreadable count was reported as an empty range:\n{report}", + ) + self.assertIn( + "Unknown is not an empty range.", + report.text, + f"the range skip does not say what is unknown:\n{report}", + ) + + +class FailOnSkipTests(PreflightHarness): + """The third state has to reach a caller that can only read the exit status.""" + + def test_the_flag_makes_a_skip_exit_1_and_the_default_still_exits_0(self) -> None: + """Both facts in one case, because each is the other's justification. + + Exit 0 by default is the row's own §3.4 decision: a branch behind + `origin/main` is ordinary work. Exit 1 under the flag is what a program + needs, because the exit status carries two of the three states and a + program cannot read the report that carries the third. + """ + + self.set_origin_main(self.divergent) + + plain = self.preflight() + self.assert_ran_something(plain) + self.assertNotEqual([], plain.skip, f"nothing was skipped:\n{plain}") + self.assertEqual(0, plain.returncode, f"the default failed a skip:\n{plain}") + + strict = self.preflight("--fail-on-skip") + self.assert_ran_something(strict) + self.assertEqual( + plain.skip, + strict.skip, + "the flag changed WHAT was reported, and it must change only the " + f"exit status:\n{strict}", + ) + self.assertEqual( + 1, + strict.returncode, + f"--fail-on-skip exited 0 over {len(strict.skip)} skipped " + f"gate(s):\n{strict}", + ) + self.assertFalse(strict.green, f"a skipped run printed the banner:\n{strict}") + self.assertIn( + "--fail-on-skip", + strict.text, + f"the refusal does not name the flag that caused it:\n{strict}", + ) + + def test_the_flag_does_not_fire_on_a_run_that_skipped_nothing(self) -> None: + """A flag that reds an ordinary run is the defect, not the discipline.""" + + self.set_origin_main(self.base) + report = self.preflight("--fail-on-skip") + self.assert_ran_something(report) + self.assertEqual([], report.skip, f"an ordinary run skipped:\n{report}") + self.assertTrue(report.green, f"an ordinary run lost the banner:\n{report}") + self.assertEqual(0, report.returncode, f"an ordinary run exited 1:\n{report}") + + +class AgentReadyRefusesASkipTests(PreflightHarness): + """`scripts/agent-ready.py` is the one consumer that reads the exit status. + + It is `AGENTS.md`'s documented gate before a remote handoff, and it read + preflight by return code alone. So a branch behind `origin/main`, or any + checkout where `origin/main` does not resolve, reached + `READY: local and live PR/CI evidence are green` with two or five gates + never run. The word "green" was in the output over a trailer check that had + not executed, which is this row's own thesis failing one layer up. + + These cases run the real `agent-ready.py` against the scratch repo, so what + is proved is that the refusal REACHES it, not that a flag exists. + """ + + def setUp(self) -> None: + super().setUp() + shutil.copy2(READY, self.tmp / "scripts" / READY.name) + self.ready_script = self.tmp / "scripts" / READY.name + + def ready(self) -> subprocess.CompletedProcess[str]: + return subprocess.run( + [sys.executable, str(self.ready_script)], + cwd=self.tmp, + capture_output=True, + text=True, + check=False, + env=self.environment(), + ) + + def test_a_skipped_preflight_stops_the_handoff_gate(self) -> None: + """RED before: preflight exits 0 over two skipped gates and this passes.""" + + self.set_origin_main(self.divergent) + + # PRECONDITION: the same tree must actually make preflight skip, or the + # case below proves nothing about a skip. + report = self.preflight() + self.assert_ran_something(report) + self.assertNotEqual([], report.skip, f"nothing was skipped:\n{report}") + + result = self.ready() + output = result.stdout + result.stderr + self.assertNotEqual(0, result.returncode, f"agent-ready passed a skip:\n{output}") + self.assertIn( + "READY FAILED: local preflight", + result.stderr, + f"agent-ready did not stop at the local preflight:\n{output}", + ) + self.assertIn( + "SKIPPED", + result.stdout, + f"agent-ready did not relay the skip report to its caller:\n{output}", + ) + # It stopped at the preflight and never reached the remote question. If + # it had, the refusal below would be the one that appeared instead, and + # a skip would be indistinguishable from having no remote. + self.assertNotIn("REMOTE_UNVERIFIED", output) + self.assertNotIn("READY:", output) + + def test_an_unskipped_preflight_lets_the_handoff_gate_continue(self) -> None: + """The control. A flag that refuses every run gates nothing. + + The scratch repo has no `origin` remote, so a run that gets past the + local preflight fails at the remote question instead. That refusal is + the proof that the local one did not fire. + """ + + self.set_origin_main(self.base) + report = self.preflight() + self.assert_ran_something(report) + self.assertEqual([], report.skip, f"the control run skipped:\n{report}") + + result = self.ready() + output = result.stdout + result.stderr + self.assertIn( + "REMOTE_UNVERIFIED", + result.stderr, + f"agent-ready did not get past the local preflight:\n{output}", + ) + self.assertNotIn("READY FAILED: local preflight", output) + + +class RegistrationTests(unittest.TestCase): + def test_the_suite_runs_in_preflight_and_in_ci(self) -> None: + """A suite wired into neither surface runs on no machine. + + `test_main_baseline.py` shipped in neither and its 24 tests ran nowhere, + so both surfaces are asserted here and removing either is red. + """ + + preflight = PREFLIGHT.read_text(encoding="utf-8") + self.assertIn(SUITE_NAME, preflight, "not in the preflight SUITES array") + self.assertIn( + f"python3 tests/scripts/{SUITE_NAME}.py", + CI.read_text(encoding="utf-8"), + "CI does not run this suite", + ) + + +if __name__ == "__main__": + unittest.main()