diff --git a/.agents/issue-index.md b/.agents/issue-index.md index 519a50241..1407b5b19 100644 --- a/.agents/issue-index.md +++ b/.agents/issue-index.md @@ -479,4 +479,6 @@ rather than merged. `scripts/check-agent-record.py` gates both. | [#1439](https://github.com/mudler/vllm.cpp/issues/1439) | `LTX25-DEVICE-RESIDENCY` | `ltx2 video: a render through the ABI emits a phase table that SUMS to wall` asserts `CHECK(leaves >= 0.95 * wall)` (`tests/vllm/multimodal/test_ltx2_video.cpp:2854`) and is **RED on `origin/main`**, not on the branch that found it. MEASURED on one x86 box, one build directory, `CMAKE_BUILD_TYPE` empty as CI has it, with this lane's four files reverted so the binary IS main at `89261c955`: six in-suite runs read 94.32%, 95.20%, 93.74%, 94.20%, 94.69% and one `-tc` run 94.19% - **five of six red**, 93 cases / 3321 assertions / 1 failed, exit 1. The W0-live merge reads 93.82%, 93.68%, 94.34%, 94.62%, 94.39% in-suite (96 cases / 3555 assertions / 1 failed) and 94.12% with `VLLM_RENDER_PROGRESS=0`, so neither the new emitter nor its call site is the term; one `-tc` run passed at 95.40%. It is NOT box contention, and the run that disproves that is main's single green: it had `wall=0.579684s`, more than double every other run, because the box was LOADED - a slower render passes, since the un-named residue grows more slowly than the wall it is divided by. The residue is 4.80% to 6.32% of `wall` across all twelve runs (0.0128 s to 0.0278 s against a `wall` of 0.220 s to 0.580 s), so a 95% floor sits INSIDE the measurement's own range at the 64x64 / 9-frame FIXTURE scale and the case decides by coin flip, mostly red. The tolerance was argued for the 21.004 B render, where the same residue would be a far smaller fraction. NOT FIXED IN FLOW, deliberately: naming the un-named time, or bounding `unaccounted_seconds` beside the ratio so the assertion says the same thing at both scales, is a change to a gate's semantics and needs its own row, spec and red-first evidence per `AGENTS.md` "Changing the rules or a checker". Found while merging `origin/main` into `row/LTX25-RESIDENCY-W0-LIVE` after [#1419](https://github.com/mudler/vllm.cpp/pull/1419) was auto-closed by its base branch being deleted. Listed under `## Owed` in [`ltx25-device-residency.md`](specs/ltx25-device-residency.md) | bug | | [#1435](https://github.com/mudler/vllm.cpp/issues/1435) | `VT-MATMUL-FP8-BLOCK-CUDA` | **A CUDA build without CUTLASS headers segfaults on the block-wise FP8 path instead of refusing by name, and configure reports the feature ENABLED anyway.** Measured 2026-08-20 on `dgx:gpu0` (GB10, cc 12.1) in an `rc` lease at `63d87805c`. `src/vt/cuda/cuda_matmul_fp8_block_cutlass.cu` is the only TU that registers `kMatmulFp8BlockScaled` for `kCUDA` and it reaches `target_sources(vllm PRIVATE ... ${_FP8_CUTLASS_SOURCES})` only through TWO nested conditions in `CMakeLists.txt` -- the outer `if(VLLM_CPP_CUTLASS)` (one occurrence in the file) and, inside it, the `if(VT_CUTLASS_FP8_ARCHS)` that SETS `_FP8_CUTLASS_SOURCES` -- so with no CUTLASS headers the op is unregistered, a device tensor reaches the portable HOST kernel, and the process SIGSEGVs while the reference tier prints `correct but slow`. Anchored by SYMBOL rather than by line on purpose, and the reason is measured on this very row: the guard sat at `CMakeLists.txt:1695@63d87805c` -- the only line number in this row, sha-qualified so it stays resolvable -- and by the time this row was written that number was already stale, then rotted AGAIN by four lines when `origin/main` was merged into the row's own branch mid-review. Two rots inside one pull request. A bare head-relative number here would be permanent, because this index is append-only and no checker scans it, and it would resolve to plausible unrelated code rather than to nothing: at `63d87805c` that line was the guard, and two trees later it was an NCCL include directory. Resolve the anchors above by grepping the symbols; do not trust any number quoted for them. Reachable on a default build: `VLLM_CPP_CUTLASS_FETCH` defaults OFF (the single `option(VLLM_CPP_CUTLASS_FETCH ...)` line, `OFF` at its default argument) and CUTLASS is not a submodule. This contradicts the design note in `cuda_matmul_fp8_block_cutlass.cu`, which states an unsupported build `keeps refusing by name -- which is the honest answer and not the #960/#844 fall-through`: the refusal IS real on the model path (`RefuseUnrunnableQwen3_5DenseFp8Block`, plus the two `VT_CHECK`s) but a caller entering through the op registry, which is what the ported upstream test does, bypasses all three. Second, smaller defect in the same run: `vt_cuda_report_feature` printed `CUDA feature cutlass-fp8: ENABLED for [121a]` for a build whose `build.ninja` contains ZERO references to either FP8 CUTLASS TU, because the report runs on the arch intersection alone, before CUTLASS-header detection. `cmake/CudaArchFeatures.cmake` opens by naming this exact class as what the feature table exists to prevent. NOT fixed in flow: both are code changes on a CUDA path needing their own row, spec and hardware re-gate. Owned by row `VT-MATMUL-FP8-BLOCK-CUDA`, under `## Owed` in [`vt-matmul-fp8-block-cuda.md`](specs/vt-matmul-fp8-block-cuda.md) | bug | | [#1437](https://github.com/mudler/vllm.cpp/issues/1437) | `VT-MATMUL-FP8-BLOCK-CUDA` | **The mainloop-scaled CUTLASS block-wise FP8 GEMM throws `cutlass Invalid status` on upstream's own ported case, on the arch it targets.** FIRST on-hardware execution of #1189 M5 (`489a9a4c0`), measured 2026-08-20 on `dgx:gpu0` (GB10, driver 580.173.02, compute capability 12.1) in an `rc` lease at `63d87805c`, CUDA 13.0 Release, `-DVLLM_CPP_CUDA_ARCHITECTURES=121a -DVLLM_CPP_CUTLASS_FETCH=ON`. G2 (upstream's `test_w8a8_block_fp8_cutlass_matmul` ported whole: M=32, N=576, K=7168, block [128,128], bf16 out) and G7 both THROW from `gemm_op.can_implement(args)`, so CUTLASS rejects the configuration before any launch -- not a numerical disagreement and not a launch failure. G6, G8 and G9 PASS. That this was an EXECUTION and not a skip rests on the throw TEXT, not on a count: `vt cuda: matmul_fp8_block_scaled: cutlass Invalid status` is assembled by the `VT_CUTLASS_CHECK` macro in `src/vt/cuda/cuda_matmul_fp8_block_cutlass.cu`, and the message prefix `matmul_fp8_block_scaled: cutlass ` occurs in NO other translation unit (the per-tensor sibling `cuda_matmul_fp8_cutlass.cu` defines a same-named macro but writes a different message), in a TU compiled only when CUTLASS is found, so no host fallback can produce that string; the run also logged ZERO `[vt reference-tier]` lines, and artifact provenance was confirmed by `cuobjdump --list-elf` reporting `cuda_matmul_fp8_block_cutlass.cu.1.sm_121a.cubin`. Suite: 5 cases, 3 passed, 2 failed, **34 assertions, 0 failed** -- recorded, but attributed rather than leaned on, because **27** of the 34 are G6, which has no `HasCuda()` guard and prints identically on a GPU-less box (8 grid entries x 3 `CHECK` + 3 tile-config `CHECK`s); the device-only remainder is G8's 2 and G9's 5, and a pure skip of this file prints 27. NO shape has had its output compared with the CPU reference: G2 and G7 are the only two cases that make that comparison and BOTH threw before their first assertion -- G7 aborted at `Grid()[0]`, the same M=32 N=576 K=7168 case, so its other seven shapes, including the second ragged-N entry M=8 N=576 K=1024, were never attempted -- and G8 compares a device f32 out against a device bf16 out, which is the kernel against ITSELF. Hypothesis recorded in the issue and explicitly NOT isolated: N=576 is 4*128+64, a ragged final scale block, which the test header records as upstream's reason for choosing it; G8 passing makes the failure shape-dependent rather than universal. This moves the row from `unmeasured` to `measured and failing`, a worse position than the spec previously described. NO speed claim: the lease took no clock control and recorded no contention. Owned by row `VT-MATMUL-FP8-BLOCK-CUDA`, under `## Owed` in [`vt-matmul-fp8-block-cuda.md`](specs/vt-matmul-fp8-block-cuda.md) | measurement | +| [#1417](https://github.com/mudler/vllm.cpp/issues/1417) | `GATE-CONFLICT-MARKERS` | **Four record gates return rc 0 on a document carrying literal conflict markers.** Re-derived on a detached scratch worktree at `b537a5344`: five lines spliced into the `docs/STATUS.md` capability table (start marker, a duplicated keyed row, separator, the same row again, end marker) leave `scripts/check-public-doc-tables.py`, `scripts/check-agent-record.py`, `scripts/check-doc-checkpoint.py` and `scripts/check-issue-index-append-only.py` all at rc 0 with their normal OK messages. The two range-scoped checkers were run over a scratch COMMIT, because a working-tree mutation of a commit-reading checker returns 0 without ever reading the mutated bytes. Closed by one tree-scoped checker, `scripts/check-conflict-markers.py`, wired into `scripts/agent-preflight.sh` and the `agent-record` CI job: it refuses a line that starts with seven `<` or seven `>` and a space, and a line of exactly seven `=` only when a start marker opened a hunk above it. The separator stays conditional because a bare row of `=` is a legal setext heading underline, and five shipped files already carry lines of eight or more `=`. No allowlist: the checker builds its patterns from character repetition, so it and its suite carry no marker at column 0. Spec [gate-conflict-markers.md](specs/gate-conflict-markers.md) | bug | +| [#1448](https://github.com/mudler/vllm.cpp/issues/1448) | `GATE-CONFLICT-MARKERS` | **`scripts/check-pr-size.py`'s `classify_path` has no entry for a per-run `docs/bench-evidence//` directory, so its own suite is red on `main`.** `BENCH_EVIDENCE` matches exactly one path segment, and the ten files of `docs/bench-evidence/gdn-replayssm-w0-20260818/` (landed 2026-08-18) match none of it. Measured at `9ecaf1bb3`: sweeping every tracked path through `classify_path` leaves exactly 10 unclassified, and `test_every_tracked_and_current_change_path_is_classified` is RED in a detached worktree at that SHA. That suite is in no preflight `SUITES` entry, so preflight is green over it; it surfaces only through the checker-evidence contract, where it reads as `ERROR: HEAD checker/test pair failed for 'scripts/check-pr-size.py'` and looks like a defect in the change under review. Registering any NEW checker requires editing `CREATION_MUTATIONS`, so every future checker was blocked behind it. Fourth instance after [#856](https://github.com/mudler/vllm.cpp/issues/856), [#668](https://github.com/mudler/vllm.cpp/issues/668) and [#989](https://github.com/mudler/vllm.cpp/issues/989). FIXED in flow by a `BENCH_EVIDENCE_RUN` pattern restricted to the extensions the directory carries, excluding `.md` and `.json` because the `evidence` arm is tested before `public_document` and would silently reclassify `docs/bench-evidence/mxfp4-qwen/*`. Verified to move exactly those 10 paths and no others | bug | | [#1447](https://github.com/mudler/vllm.cpp/issues/1447) | `ENG-EXPERT-STREAM` | **`docs/USAGE.md` said EVERY Qwen3.8 decode figure came from the W0e C ABI harness; the 66.7 s/token streaming-off row of 16 August 2026 came from `vllm-server`, the same binary the section tells the reader to run.** Introduced by #1211 and fixed in the same flow: the sentence is scoped to the W0e and W0f runs and the exception is named, in `docs/USAGE.md` and in the spec paragraph that mirrors it. Provenance read at the source, `.agents/specs/expert-streaming.md:837` (server entry point) and `:905`. | record | diff --git a/.agents/specs/gate-conflict-markers.md b/.agents/specs/gate-conflict-markers.md new file mode 100644 index 000000000..771f86ba3 --- /dev/null +++ b/.agents/specs/gate-conflict-markers.md @@ -0,0 +1,334 @@ +# Spec — no record gate can see a conflict marker + +Issue: [#1417](https://github.com/mudler/vllm.cpp/issues/1417) +Row: `GATE-CONFLICT-MARKERS` (unplaced record/gate defect; the tracked tree is a +record surface, not a matrix row) +State: `ACTIVE` + +## Scope + +Splice a merge conflict into `docs/STATUS.md` and every record gate passes. The +measurement was re-derived for this spec on a detached scratch worktree at +`b537a5344`, not quoted from the issue. Five lines were inserted into the +capability table: a start marker, a duplicated keyed row, a separator, the same +row again, and an end marker. `git diff --stat` read +`docs/STATUS.md | 5 +++++`. + +| Checker | Scope | rc | Message | +|---|---|---|---| +| `scripts/check-public-doc-tables.py` | tree | 0 | `OK: docs/BENCHMARKS.md and docs/FEATURES.md are human-readable keyed tables...` | +| `scripts/check-agent-record.py` | tree | 0 | `agent record OK: ENGINE=168 MODEL=377 ...` | +| `scripts/check-doc-checkpoint.py` | range | 0 | `OK: public documents match the claims this change makes.` | +| `scripts/check-issue-index-append-only.py` | range | 0 | `OK: issue index append-only` | + +The two range-scoped checkers were run over a detached scratch **commit**, not +over the working tree. A working-tree mutation of a commit-reading checker +returns 0 because the checker never reads the mutated bytes, and that 0 looks +exactly like a gate that cannot detect the defect. + +This is not hypothetical. An earlier revision of the branch for +[#1414](https://github.com/mudler/vllm.cpp/issues/1414) carried a `docs/STATUS.md` +mangled by stale working-tree state from a pre-squash branch, together with an +unrelated spec file deleted outright. The full record gate set ran and reported +clean. A person caught it by reading `git diff --stat`. + +In scope: + +1. One new tree-scoped checker, `scripts/check-conflict-markers.py`. +2. Its registration in `scripts/agent-preflight.sh` and in + `.github/workflows/ci.yml`. +3. Its mutation suite, `tests/scripts/test_check_conflict_markers.py`. +4. The creation-mutation entry a new checker owes `scripts/check-pr-size.py` + and its suite, because the checker has no version at the merge base. +5. One in-flow repair the entry above uncovered + ([#1448](https://github.com/mudler/vllm.cpp/issues/1448)): + `classify_path` has no entry for a per-run + `docs/bench-evidence//` directory, so ten tracked paths are + unclassified and `test_check_pr_size.py`'s sweep case is red on + `origin/main`. That red blocks the checker-evidence contract of every change + that edits `scripts/check-pr-size.py`, which registering a new checker + requires, so this row could not prove its own contract while it stood. The + repair names the surface and does not widen a rule, and classification of + all 4811 tracked paths was captured before and after to prove that exactly + those ten move. + +Out of scope, and each for a stated reason: + +- **Any change to the four checkers above.** They measure what they were built + to measure. Widening one of them to also parse markdown well-formedness would + make the same gap reappear in the next checker that reads a file for its own + reason. The issue asks for one check in one place, and that is what this is. +- **Markdown well-formedness in general.** A half-merged table row is one shape + of a broken document. This row refuses the shape that a merge tool writes and + that nothing else writes, and it does not become a markdown validator. +- **The other half of the #1414 incident, the deleted spec file.** A deletion + leaves no bytes to grep. It needs a diff-scoped check, it has a different + design, and merging it into this row would give one checker two jobs. + +## Upstream anchors + +None. This is a repository checker. vLLM has no equivalent surface, so no +oracle applies and none is claimed. + +## Design + +`scripts/check-conflict-markers.py` reads every tracked path with +`git ls-files -z`, reads its working-tree bytes, and refuses three line shapes. +Working-tree bytes rather than a commit: the incident this row closes happened +in a working tree, and a checkout in CI carries the committed bytes anyway, so +one read covers both. + +### The rule + +A line is a **start marker** when it begins with seven `<` characters and a +space. A line is an **end marker** when it begins with seven `>` characters and +a space. Either one, anywhere in a tracked text file, fails the gate. + +A line that is exactly seven `=` characters and nothing else is a **separator**, +and it fails **only when it lies inside an open hunk**: a start marker appeared +on an earlier line of the same file and no end marker has appeared since. + +The separator rule is conditional because a bare row of `=` characters is legal +markdown. It is the setext heading underline, and it is a horizontal rule. A +gate that fires on ordinary work is the defect, not the discipline. Two +independent narrowings keep it off ordinary text: + +- **Exactly seven.** The tree carries lines that start with seven or more `=` + characters today, in `docs/bench-evidence/gdn-replayssm-w0-20260818/*.log`, + `tests/parity/goldens/tokenizer_deepseek_v2/corpus.txt` and + `tests/parity/goldens/tokenizer_qwen36/corpus.txt`. Every one of them is + longer than seven, so a `^=+` rule would have fired on five files on arrival. +- **Adjacency.** Even a line of exactly seven is silent unless a real marker + opened a hunk above it. + +The separator therefore adds no detection power over the two markers on its own, +and that is deliberate. Its job is to name the middle of the hunk in the report, +so the reader sees the whole conflict rather than its first line. + +The diff3 `|||||||` marker is deliberately absent. A diff3 conflict still +carries the start and end markers, so nothing escapes, and a line of seven `|` +characters is a plausible empty row in a repository whose records are wide +markdown tables. + +### No allowlist, and no self-exclusion + +The checker builds its own patterns from character repetition (`"<" * 7`) +instead of writing a marker literal. Its suite builds fixtures in a temporary +git repository. Neither file therefore contains a marker at the start of a line, +so neither needs an exclusion, and the repository needs no allowlist file for +any change to append itself to. `AGENTS.md` § *Records* forbids a surface that +every pull request must edit, and an allowlist is exactly that surface. + +### The cheap reject, and what it is not + +A marker line necessarily contains the marker as a substring, so a buffer +holding neither substring anywhere cannot hold one at the start of a line. The +per-line pass is skipped for such a buffer, which is nearly every file in the +tree. This is a **pure optimization**: it decides nothing the full scan would +decide differently. It is measured as such in `## Evidence`, where deleting it +leaves the whole suite green, and that green is the expected result rather than +a gap. The semantic guard is the adjacency test, and the case above proves it. + +The scan reads bytes and never decodes a whole file. Decoding 142 MB of tracked +text to find a marker in none of it cost more than reading it: 0.9 s of CPU +against 0.29 s. + +### The unmerged index, counted once + +`git ls-files` emits stages 1, 2 and 3 for a path with an unresolved merge, so +a live conflict names the same file three times. `tracked_paths` deduplicates +with `dict.fromkeys`, which keeps git's order. Without it the file is read three +times, its findings print three times, and `examined` over-counts by two per +conflicted path: `9 findings in 1 file; examined 3 tracked text files` for one +file. The verdict was 1 either way, so this never caused a miss, and the count +was wrong in exactly the state the gate exists for. + +`test_an_unmerged_index_is_counted_once` builds the state by making `git merge` +fail, not by writing markers into a file, and it asserts the three stage entries +before it asserts anything else. Only a real merge failure puts three stages in +the index, and a fixture that faked it would pass with the dedupe removed. + +### What is skipped, and counted + +Three classes are skipped, and each is counted and reported rather than dropped +in silence: + +- **Binary files.** A NUL byte in the first 8192 bytes decides it, which is + git's own heuristic. `tests/**/fixtures/*.bin` and the `.npy` goldens carry + arbitrary bytes, and a marker byte sequence inside a tensor dump is not a + conflict. +- **Symlinks.** Reading one follows it out of the repository or fails on a + broken link. The link is not text, and its target is examined on its own if + it is tracked. +- **Paths absent from the working tree.** A tracked file deleted in the working + tree has no bytes to read. + +The binary test reads the first 8192 bytes and stops there for a binary file, so +the 175 MB of tracked bytes are not all read. + +A file the run could not read is none of those three. It is not a skip, because +the run does not know what it holds, and it is not a finding, because an +`OSError` is not a merge conflict. It takes its own list, its own exit status of +2 and its own remedy. Filing it under the findings made an I/O error exit 1 and +print "Resolve the merge before committing", which named the wrong problem and +the wrong repair. + +### The report + +A clean run prints the counts on one line and exits 0: + + conflict markers: 0 findings in 4744 tracked text files (58 binary, 0 + symlink, 0 absent skipped, 4806 tracked paths) + +A run that examined zero text files exits 2 and says so. A gate that examined +nothing has not reported, and a `git ls-files` that returns nothing is a broken +invocation rather than a clean tree. + +A failing run names `path:line`, the shape, and the line, and it prints the +remedy: resolve the merge, or, for a document that quotes a marker on purpose, +indent the quoted marker so it does not start at column 0. + +### Where it runs + +Two registrations, which is this repository's convention for a checker that +must not depend on a human remembering it: + +- `CHECKERS` in `scripts/agent-preflight.sh`, so it runs before every commit and + every push, beside the four gates it complements. +- A step in the `agent-record` job of `.github/workflows/ci.yml`, so a pull + request is gated even when preflight was never run. + +`SUITES` in preflight and the same CI step carry the mutation suite. + +## Risks + +**A document that quotes a conflict marker at column 0.** A future guide about +resolving merges would fire this gate. No tracked file does today, measured with +a grep whose pattern was first proven against a positive control. The remedy is +in the failure message and costs two spaces of indentation, and it is per +instance and in the text, not a registry entry. Recorded here as the deliberate +cost of having no allowlist. + +**A conflict written with a marker length other than seven.** Git writes seven. +A hand-typed six or eight passes. The gate refuses what the tool produces, which +is the incident it exists to catch. + +**A `git` that fails.** `git ls-files` returning non-zero, or an invocation from +outside a repository, exits 2 with the git error attached. Unknown is not +absence. + +**The checker resolves its root from its own path.** Run from a linked worktree, +`scripts/check-conflict-markers.py` reads that worktree, because the script file +is physically inside it. Run by absolute path from the shared checkout, it reads +the shared checkout. `--root` makes the target explicit, and the report names +the resolved root so no run has to be trusted about which tree it read. + +**Cost.** The tree scan must stay cheap enough to sit in preflight. Measured in +`## Evidence`. + +**The unmerged index was NOT SEEN when this spec was first written, and is +recorded here as a miss rather than as a considered trade-off.** The first +version of this section reasoned about which files the scan reads and never +asked what `git ls-files` returns while a merge is in flight. A fresh review +found the triple count. It is repaired above, and the case that proves the +repair is falsifiable is named in `## Tests`. The general lesson is the one this +row already carries: a gate is measured by what it says it examined, and the +count is easiest to get wrong in the very state the gate exists for. + +## Tests + +`tests/scripts/test_check_conflict_markers.py`, registered in preflight's +`SUITES` and in CI. Every fixture case builds a temporary git repository and +runs the shipped checker against it with `--root`, so no case can pass on a +fixture the tree does not have, and no case writes into the repository. + +The red-before case is the first one: markers written into a scratch copy must +produce a NON-ZERO exit. + +- `test_a_full_conflict_hunk_is_refused` — start, separator and end spliced into + a tracked markdown file. Requires exit 1 and requires the report to name the + line number of each of the three. +- `test_a_lone_start_marker_is_refused` and + `test_a_lone_end_marker_is_refused` — half a conflict is still a conflict. +- `test_a_bare_separator_alone_is_not_refused` — a setext heading underline of + exactly seven `=` characters, in a file with no marker. Exit 0. This is the + case that keeps the gate off ordinary documents. +- `test_a_long_rule_of_equals_is_not_refused` — the shape the shipped evidence + logs and tokenizer corpora carry. +- `test_a_separator_outside_an_open_hunk_is_not_reported` — a marker-free file + whose separator follows other text is silent. +- `test_a_separator_after_the_hunk_closes_is_not_named` — a file that carries a + closed hunk AND a legal setext underline below it. The exit code is 1 either + way, so this case asserts the REPORT: lines 2, 4 and 6 are named and line 8 is + not. **Written because the first mutation run found the adjacency guard was + not load-bearing.** Dropping `open_at is not None` left every other case + green, because a file with no marker leaves the scan early and the guard never + decides anything there. The absence assertion carries its own positive control + in the same case: three named lines prove the match works before the fourth + claims a line is absent. +- `test_a_binary_file_with_marker_bytes_is_skipped` — exit 0, and the report + counts one binary skip. +- `test_a_symlink_is_skipped` and `test_a_tracked_path_absent_from_the_worktree_is_skipped`. +- `test_an_untracked_file_is_not_examined` — the gate reads what the repository + tracks. +- `test_crlf_line_endings_are_refused` — a conflict written by a tool on Windows. +- `test_a_tree_with_no_text_files_exits_two` — the vacuity floor. +- `test_an_unmerged_index_is_counted_once` — a real `git merge` conflict, with + the three stage entries asserted first. Reds without the dedupe. +- `test_an_unreadable_tracked_file_exits_two_and_names_no_merge` — a `chmod 000` + tracked file exits 2, is named, and does not draw the merge remedy. +- `test_two_paths_sharing_a_colon_prefix_count_as_two_files` — two tracked paths + that share everything before a colon. The old file count split each report + line on its first colon and collapsed them into one. +- `test_the_shipped_tree_is_clean` — runs against the real repository root, + requires exit 0, and requires the reported examined count to EQUAL git's own + tracked text set, derived at read time from + `git grep -I --name-only -e ''` plus the tracked files that have no lines at + all. The first version asserted `> 1000` against a real 3733, which is 27% of + the count and would have stayed green over a scan that collapsed to markdown + alone. Equality is strictly stronger and stores no number, so it is not the + drift lock `AGENTS.md` forbids: both sides are re-derived on every run. A file + that `.gitattributes` marks binary while carrying no NUL byte would separate + the two sets, and the failure message names that cause beside the other one. +- `test_the_checker_is_registered_in_preflight_and_ci` — the gate runs somewhere. + +## Gates + +Each command run bare, with its exit code echoed unpiped. `cmd | tail` reports +`tail`'s status, which is how this tree has produced false green verdicts. + +| Gate | Command | Expected | +|---|---|---| +| G1 red-before | the four record gates on a mangled scratch commit | all rc 0, which is the defect | +| G2 red-before | `python3 tests/scripts/test_check_conflict_markers.py` before the checker exists | non-zero | +| G3 green | `python3 scripts/check-conflict-markers.py` | rc 0, count reported | +| G4 green | `python3 scripts/check-conflict-markers.py --root ` | rc 1, naming the three lines | +| G5 green | `python3 tests/scripts/test_check_conflict_markers.py` | rc 0, `Ran N tests` with N greater than zero, `OK` | +| G6 mutation | the detection core deleted in a scratch copy | the suite goes RED | +| G7 unchanged | the four existing checkers on the clean tree | rc 0, byte-identical output before and after this change | +| G8 full | `scripts/agent-preflight.sh` | green | + +## Evidence + +Captured verbatim with exit codes in the pull request body: the red-before +transcript, the green-after transcript, the self-mutation result, the examined +count on a clean tree, and the wall time of the tree scan. + +## Stop conditions + +- Stop and report `NEEDS_DECISION` if the rule fires on any file of + `origin/main` as it stands. That would mean the rule is wrong, not the tree, + and widening it to pass is the one move the protocol forbids. +- Stop and report `NEEDS_DECISION` if closing the gap needs an allowlist file + that every change must append to. That is a lock, and a lock is not a gate. +- Stop if the suite can only go green by widening the marker set beyond the + three shapes #1417 names. +- Stop if the tree scan costs more than one second of CPU. The bound is CPU and + not wall time on purpose: this box runs several agent sessions at once, and + the same scan measured 1.17 s to 4.60 s of wall against 0.29 s of CPU. A wall + bound would report the other sessions, not this gate. + +## Now + +`ACTIVE`. The spec commits first, then the checker, its suite, and its two +registrations in one pull request. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1e21a21a3..31447ef44 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -345,6 +345,20 @@ jobs: python3 tests/scripts/test_check_prompt_contract.py python3 tests/scripts/test_agent_gates.py python3 tests/scripts/test_agent_pr_body.py + - name: No tracked file carries a merge conflict marker (#1417) + # Registered HERE and not only in preflight. The four record gates that + # missed this all return 0 on a `docs/STATUS.md` carrying a spliced + # conflict hunk, because each measures its own budget and none asks + # whether the file is well-formed. A person caught the real instance by + # reading `git diff --stat`. + # + # The bare `=======` separator is refused ONLY inside an open hunk. Five + # shipped files carry a rule of eight or more `=` characters, so an + # unconditional rule would have been red on arrival, and a gate that + # fires on ordinary work is the defect rather than the discipline. + run: | + python3 scripts/check-conflict-markers.py + python3 tests/scripts/test_check_conflict_markers.py - name: A citation that names a symbol still finds it (#1143, #1139) # The tree already carried 539 `path::Symbol` citations and had never # checked one. Line anchors are what rot: one 45-line insertion near the diff --git a/scripts/agent-preflight.sh b/scripts/agent-preflight.sh index a9e555837..94b0e2e9d 100755 --- a/scripts/agent-preflight.sh +++ b/scripts/agent-preflight.sh @@ -76,6 +76,12 @@ for arg in "$@"; do done CHECKERS=( + # FIRST deliberately. Every other gate reads a file for its own reason and + # measures its own budget, so a table row that is half one branch and half + # another satisfies all of them (#1417). This one asks whether a merge tool + # wrote into a tracked file at all, and a reader who sees it fail knows to + # stop reading the verdicts below. + check-conflict-markers check-prompt-contract check-agent-record check-release-binary-contract @@ -152,6 +158,7 @@ SUITES=( test_agent_preflight_skip_report test_agent_pr_body test_check_symbol_anchors + test_check_conflict_markers ) failed=() diff --git a/scripts/check-conflict-markers.py b/scripts/check-conflict-markers.py new file mode 100755 index 000000000..88b37ef1b --- /dev/null +++ b/scripts/check-conflict-markers.py @@ -0,0 +1,261 @@ +#!/usr/bin/env python3 +"""Refuse a tracked file that carries a literal merge conflict marker (#1417). + +Splice a conflict into `docs/STATUS.md` and every record gate passes. +`check-public-doc-tables.py` measures cell and row budgets, `check-agent-record.py` +measures counts and anchors, and neither asks whether the file is well-formed in +the first place, so a table row that is half one branch and half another +satisfies every budget it is measured against. Measured on a scratch commit at +`b537a5344`: all four record gates returned 0 with their normal OK messages. + +That is not hypothetical. An earlier revision of the branch for #1414 carried a +`docs/STATUS.md` mangled by stale working-tree state, the full record gate set +ran, and it reported clean. + +THE RULE, and why each half of it is shaped the way it is. + + * A line that starts with seven `<` and a space opens a hunk. A line that + starts with seven `>` and a space closes one. Either one fails, anywhere. + * A line that is EXACTLY seven `=` and nothing else fails ONLY inside an open + hunk. + +The separator is conditional because a bare row of `=` is legal markdown: it is +the setext heading underline and it is a horizontal rule. A `^=+` rule would +have fired on five shipped files on arrival -- three logs under +`docs/bench-evidence/gdn-replayssm-w0-20260818/` and two tokenizer corpora under +`tests/parity/goldens/` -- every one of which carries a rule longer than seven. +A gate that fires on ordinary work is the defect, not the discipline. So the +separator adds no detection power over the two markers, deliberately: its job is +to name the middle of the hunk in the report, so the reader sees the whole +conflict rather than its first line. + +The diff3 `|||||||` marker is absent on purpose. A diff3 conflict still carries +the start and end markers, so nothing escapes, and a line of seven `|` is a +plausible empty row in a repository whose records are wide markdown tables. + +NO ALLOWLIST, AND NO SELF-EXCLUSION. Every pattern below is built by character +repetition rather than written as a literal, so this file and its suite carry no +marker at the start of any line and need no exemption from the gate they +implement. An allowlist would be a file that every change must append to, and +AGENTS.md forbids that surface: if N concurrent pull requests edit file F, F is +a lock. A document that quotes a marker on purpose indents it, which is what the +failure message says. + + scripts/check-conflict-markers.py # the tree this script is in + scripts/check-conflict-markers.py --root PATH # an explicit tree + +Exit 0 clean, 1 with findings, 2 when the run could not examine anything. +""" + +from __future__ import annotations + +import argparse +import subprocess +import sys +from pathlib import Path + + +# Built, never written: see the docstring. `START`/`END` carry the trailing +# space that `git` writes before the branch label; `SEPARATOR` is the whole line. +# Bytes, not text: the scan reads 142 MB of tracked text, and decoding all of it +# to find a marker in none of it cost more than reading it. +START = b"<" * 7 + b" " +SEPARATOR = b"=" * 7 +END = b">" * 7 + b" " + +# git's own binary heuristic: a NUL byte in the first block. Reading only this +# much of a binary keeps the scan off the 175 MB of tracked tensor fixtures. +SNIFF_BYTES = 8192 + + +def tracked_paths(root: Path) -> list[str]: + """Every path in the index, once each, in `git`'s order. + + The index rather than a commit: the incident this gate closes happened in a + working tree, and a fresh clone's index matches its checkout anyway. + + DEDUPLICATED, and that is not a tidiness measure. An index holding an + unresolved merge carries stages 1, 2 and 3 for every conflicted path, so + `git ls-files` names such a path three times. Without this, the file is read + three times, its findings print three times, and `examined` over-counts by + two per conflicted path -- measured as `9 findings in 1 file; examined 3 + tracked text files` for a single real conflict. The verdict was still 1, so + this never caused a miss, but the count was wrong in exactly the state this + gate exists for, and a gate that cannot say how many things it examined has + not reported. + """ + result = subprocess.run( + ["git", "-C", str(root), "ls-files", "-z"], + capture_output=True, + check=False, + ) + if result.returncode != 0: + message = result.stderr.decode("utf-8", "replace").strip() + raise RuntimeError( + f"git ls-files exited {result.returncode} in {root}: {message}\n" + "Unknown is not absence: this run examined nothing." + ) + names = [ + name + for name in result.stdout.decode("utf-8", "surrogateescape").split("\0") + if name + ] + # `dict.fromkeys` rather than `set`, so git's order survives the dedupe and + # the report stays reproducible run to run. + return list(dict.fromkeys(names)) + + +def scan_text(data: bytes) -> list[tuple[int, str, str]]: + """Findings in one file, as `(line number, shape, line)`. + + A cheap whole-buffer reject first, and it is a PURE OPTIMIZATION: it decides + nothing the per-line pass would decide differently. A marker line contains + the marker as a substring, so a buffer holding neither substring anywhere + cannot hold one at the start of a line. The separator needs no test of its + own here, because it is only ever a finding inside a hunk and a hunk needs a + start marker in the same buffer. Almost every file takes this arm, which is + what holds the whole-tree scan at 0.29 s of CPU. + + Deleting this reject therefore leaves the suite green, and that green is the + expected result rather than a gap. The SEMANTIC guard in this function is + `open_at is not None`, and dropping it reds + `test_a_separator_after_the_hunk_closes_is_not_named`, which exists because + the first mutation run found nothing else could tell. + """ + if START not in data and END not in data: + return [] + findings: list[tuple[int, str, str]] = [] + open_at: int | None = None + for number, raw in enumerate(data.split(b"\n"), start=1): + # A conflict written on Windows arrives with a carriage return, and a + # separator compared without stripping it would never match. + line = raw.rstrip(b"\r") + if line.startswith(START): + shape = "conflict start marker" + open_at = number + elif line.startswith(END): + shape = "conflict end marker" + open_at = None + elif line == SEPARATOR and open_at is not None: + shape = f"conflict separator inside the hunk opened at line {open_at}" + else: + continue + findings.append((number, shape, line.decode("utf-8", "replace"))) + return findings + + +def scan_tree(root: Path) -> tuple[list[str], set[str], list[str], dict[str, int]]: + """Findings over every tracked text file, with the counts that were skipped. + + Returns the finding lines, the SET of paths they name, the unreadable-file + lines, and the counts. The offender set is collected here rather than parsed + back out of the report: splitting a report line on its first colon + mis-attributes any path that contains one. + """ + counts = {"tracked": 0, "examined": 0, "binary": 0, "symlink": 0, "absent": 0} + report: list[str] = [] + offenders: set[str] = set() + unreadable: list[str] = [] + for name in tracked_paths(root): + counts["tracked"] += 1 + path = root / name + # Checked before `is_file()`, which follows a symlink and would file a + # broken link under the wrong count. + if path.is_symlink(): + counts["symlink"] += 1 + continue + if not path.is_file(): + counts["absent"] += 1 + continue + try: + with path.open("rb") as handle: + head = handle.read(SNIFF_BYTES) + if b"\0" in head: + counts["binary"] += 1 + continue + data = head + handle.read() + except OSError as error: + # NOT a finding, and not a skip either. A file this run could not + # read is a file this run cannot call clean, so it takes its own + # list, its own exit status and its own remedy. Filing it under + # `report` made an I/O error exit 1 and print "Resolve the merge + # before committing", which is the wrong remedy for the wrong + # problem. + unreadable.append(f"{name}: unreadable: {error}") + continue + counts["examined"] += 1 + for number, shape, line in scan_text(data): + report.append(f"{name}:{number}: {shape}: {line[:80]}") + offenders.add(name) + return report, offenders, unreadable, counts + + +def summary(findings: int, files: int, counts: dict[str, int]) -> str: + plural = "file" if files == 1 else "files" + return ( + f"conflict markers: {findings} findings in {files} {plural}; " + f"examined {counts['examined']} tracked text files " + f"({counts['binary']} binary, {counts['symlink']} symlink, " + f"{counts['absent']} absent skipped, {counts['tracked']} tracked paths)" + ) + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument( + "--root", + default=None, + help=( + "the tree to examine. Defaults to the repository holding this " + "script, which from a linked worktree is that worktree and from the " + "shared checkout is the shared checkout." + ), + ) + args = parser.parse_args(argv) + root = Path(args.root).resolve() if args.root else Path(__file__).resolve().parents[1] + + # Printed first and always. A checker resolves its root from its own path, + # so a run whose root is not the tree the reader means can print OK about + # somebody else's checkout. + print(f"root: {root}") + try: + report, offenders, unreadable, counts = scan_tree(root) + except RuntimeError as error: + print(str(error)) + print(summary(0, 0, {"tracked": 0, "examined": 0, "binary": 0, "symlink": 0, "absent": 0})) + return 2 + + for line in report: + print(line) + for line in unreadable: + print(line) + print(summary(len(report), len(offenders), counts)) + + if counts["examined"] == 0: + print( + "A gate that examined nothing has not reported. Check --root, and " + "check that it names a git repository with tracked text files." + ) + return 2 + if unreadable: + # Ordered BEFORE the findings arm on purpose. A run that could not read + # part of the tree cannot say the tree is clean, and it must not say so + # by reporting only the part it managed to read. + print( + f"{len(unreadable)} tracked file(s) could not be read, so this run " + "cannot report on them. Unknown is not absence. Repair the " + "permissions or the checkout and rerun." + ) + return 2 + if report: + print( + "Resolve the merge before committing. A document that quotes a " + "conflict marker on purpose must indent it, so that it does not " + "start at column 0." + ) + return 1 + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/scripts/check-pr-size.py b/scripts/check-pr-size.py index 7dcef4689..33ec85bff 100755 --- a/scripts/check-pr-size.py +++ b/scripts/check-pr-size.py @@ -209,6 +209,33 @@ SYNC_RECORD = re.compile(r"\.agents/sync/[A-Za-z0-9_.-]+\.md\Z") HOOK = re.compile(r"\.githooks/(?:README\.md|[A-Za-z0-9_.-]+)\Z") BENCH_EVIDENCE = re.compile(r"(?:benchmarks/(?:demo|media)|docs/bench-evidence)/[A-Za-z0-9_.-]+\.(?:json|png|gif|mp4|log)\Z") +# A PER-RUN evidence directory: docs/bench-evidence// (#1448). +# AGENTS.md requires the exact build and run recipe beside a measurement, so one +# run arrives as a dated directory of logs, dumps and the scripts that produced +# them, not as one flat file. BENCH_EVIDENCE above matches exactly ONE path +# segment, so all ten files of the first such directory +# (`gdn-replayssm-w0-20260818`, landed 2026-08-18) were unclassified. +# `classify_path` FAILS CLOSED, and the sweep in +# tests/scripts/test_check_pr_size.py classifies every TRACKED path, so that +# suite went red on `main` and every PR touching this checker was refused +# through its own evidence contract. Fourth instance of the class after #856, +# #668 and #989, and repaired the same way: name the surface, do not widen a +# rule. +# +# The extension list is EXACTLY the set this directory carries, and it +# deliberately omits `.md` and `.json`. Those two already classify as +# public_document through DOC below, and the evidence arm is tested FIRST, so +# admitting them here would silently RECLASSIFY +# docs/bench-evidence/mxfp4-qwen/*.md and its golden .json. Preserving the class +# of a path that already had one matters more than making the directory +# uniform. +# +# `.sh` and `.cu` are evidence, not product: they are the recipe that produced +# the number. Nothing builds them, nothing installs them, and no entry point +# reaches them. +BENCH_EVIDENCE_RUN = re.compile( + r"docs/bench-evidence/[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+\.(?:txt|log|gz|sh|cu)\Z" +) STATE_MIGRATION_MANIFEST = ".agents/completed/state-migration-manifest.csv" STATE_MIGRATION_MANIFEST_ARCHIVE = re.compile( r"\.agents/completed/state-migration-manifest-" @@ -328,6 +355,16 @@ # checker as a module and calls into it, so the disabled stub fails at import # rather than quietly passing a reduced set of cases. "scripts/check-cuda-op-arch-gate.py": DISABLED_CREATION_CHECKER, + # GATE-CONFLICT-MARKERS (#1417). Created in this range, so there is no BASE + # version to mutate. The empty stub exits 0 and prints nothing, which fails + # 16 of the 21 cases in tests/scripts/test_check_conflict_markers.py -- + # measured, not asserted. The five survivors are the four OrdinaryTextTests + # cases, which assert only that an ordinary document exits 0 and are + # therefore satisfied by silence, and the registration case, which reads two + # files and never runs the checker at all. Every case that reads an exit + # code of 1 or an examined count out of the report goes red, which is what + # makes the stub a mutation rather than a weaker checker. + "scripts/check-conflict-markers.py": DISABLED_CREATION_CHECKER, } SELF_CHECKER = "scripts/check-pr-size.py" EVIDENCE_TIMEOUT_SECONDS = 120 @@ -417,6 +454,7 @@ def classify_path(path: str) -> str: or SPEC_EVIDENCE.fullmatch(path) or SYNC_RECORD.fullmatch(path) or BENCH_EVIDENCE.fullmatch(path) + or BENCH_EVIDENCE_RUN.fullmatch(path) ): return "evidence" if path in GOVERNANCE_SUPPORT_FILES: diff --git a/tests/scripts/test_check_conflict_markers.py b/tests/scripts/test_check_conflict_markers.py new file mode 100755 index 000000000..303fe14f1 --- /dev/null +++ b/tests/scripts/test_check_conflict_markers.py @@ -0,0 +1,414 @@ +#!/usr/bin/env python3 +"""Unit and mutation checks for scripts/check-conflict-markers.py (#1417). + +Four record gates return rc 0 on a document carrying literal conflict markers, +so a half-merged keyed row satisfies every budget it is measured against. The +checker under test is the one place that reads a tracked file and asks whether a +merge tool wrote into it. + +TWO DIRECTIONS IN EVERY CASE. A gate that refuses a marker is only half the +contract; the other half is that it stays silent on ordinary text. A bare row of +seven `=` characters is a setext heading underline and a horizontal rule, and +five shipped files already carry lines of eight or more `=`. So the separator +cases below assert exit 0 as hard as the marker cases assert exit 1. + +NO MARKER LITERAL LIVES IN THIS FILE. Every pattern is built by character +repetition, so this suite never contains a marker at the start of a line and +therefore never needs an exclusion from the checker it tests. That is the whole +reason the tree needs no allowlist: an allowlist is a file every change must +append to, and AGENTS.md forbids that surface. +""" + +from __future__ import annotations + +import os +import re +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[2] +CHECKER = ROOT / "scripts/check-conflict-markers.py" +PREFLIGHT = ROOT / "scripts/agent-preflight.sh" +CI = ROOT / ".github/workflows/ci.yml" + +# Built, never written. See the module docstring. +START = "<" * 7 +SEPARATOR = "=" * 7 +END = ">" * 7 + +EXAMINED = re.compile(r"examined ([0-9]+) tracked text files") + + +class CheckerCase(unittest.TestCase): + """Base case: build a scratch git repository and run the shipped checker.""" + + def run_checker(self, root: Path) -> subprocess.CompletedProcess[str]: + return subprocess.run( + [sys.executable, str(CHECKER), "--root", str(root)], + capture_output=True, + text=True, + check=False, + ) + + def scratch_repo(self, files: dict[str, bytes]) -> Path: + """A temporary repository whose index tracks `files`. + + The files are staged and never committed. `git ls-files` reads the + index, so a commit would add an identity requirement and prove nothing. + """ + root = Path(tempfile.mkdtemp(prefix="vllm-conflict-markers-")) + subprocess.run( + ["git", "init", "-q", str(root)], check=True, capture_output=True + ) + for name, content in files.items(): + path = root / name + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(content) + if files: + subprocess.run( + ["git", "-C", str(root), "add", "-A"], + check=True, + capture_output=True, + ) + return root + + def conflicted_repo(self, base: str, ours: str, theirs: str) -> Path: + """A repository whose index holds an UNRESOLVED merge. + + Built by making git produce the conflict, not by writing markers into a + file and pretending. Only a real `git merge` failure puts stages 1, 2 + and 3 in the index, which is the state the deduplication exists for. + """ + root = Path(tempfile.mkdtemp(prefix="vllm-conflict-merge-")) + git = ["git", "-C", str(root), "-c", "user.name=t", "-c", "user.email=t@x"] + subprocess.run(["git", "init", "-q", "-b", "main", str(root)], check=True, capture_output=True) + target = root / "docs" / "STATUS.md" + target.parent.mkdir(parents=True, exist_ok=True) + for content, message, branch in ( + (base, "base", None), + (theirs, "theirs", "other"), + ): + if branch is not None: + subprocess.run([*git, "checkout", "-q", "-b", branch], check=True, capture_output=True) + target.write_text(content) + subprocess.run([*git, "add", "-A"], check=True, capture_output=True) + subprocess.run([*git, "commit", "-q", "-m", message], check=True, capture_output=True) + subprocess.run([*git, "checkout", "-q", "main"], check=True, capture_output=True) + target.write_text(ours) + subprocess.run([*git, "add", "-A"], check=True, capture_output=True) + subprocess.run([*git, "commit", "-q", "-m", "ours"], check=True, capture_output=True) + merge = subprocess.run([*git, "merge", "other"], capture_output=True, text=True) + self.assertNotEqual(merge.returncode, 0, "the merge was supposed to CONFLICT") + stages = subprocess.run( + ["git", "-C", str(root), "ls-files", "-z"], capture_output=True, check=True + ).stdout.decode().split("\0") + # The precondition, asserted rather than assumed: without three entries + # for one path there is nothing here to deduplicate and the case would + # pass vacuously. + self.assertEqual( + [n for n in stages if n].count("docs/STATUS.md"), 3, stages + ) + return root + + def examined_count(self, output: str) -> int: + match = EXAMINED.search(output) + self.assertIsNotNone( + match, f"report names no examined count:\n{output}" + ) + assert match is not None + return int(match.group(1)) + + +class MarkerRefusalTests(CheckerCase): + """The red-before direction: a spliced marker must exit NON-ZERO.""" + + def test_a_full_conflict_hunk_is_refused(self) -> None: + body = ( + "| Capability | State |\n" + "|---|---|\n" + f"{START} HEAD\n" + "| a | ours |\n" + f"{SEPARATOR}\n" + "| a | theirs |\n" + f"{END} origin/main\n" + ) + root = self.scratch_repo({"docs/STATUS.md": body.encode()}) + result = self.run_checker(root) + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + # Each of the three lines named, by number, so the reader sees the hunk + # and not only its first line. + self.assertIn("docs/STATUS.md:3", result.stdout) + self.assertIn("docs/STATUS.md:5", result.stdout) + self.assertIn("docs/STATUS.md:7", result.stdout) + + def test_a_lone_start_marker_is_refused(self) -> None: + root = self.scratch_repo({"a.md": f"text\n{START} HEAD\nmore\n".encode()}) + result = self.run_checker(root) + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + self.assertIn("a.md:2", result.stdout) + + def test_a_lone_end_marker_is_refused(self) -> None: + root = self.scratch_repo({"a.md": f"text\n{END} branch\n".encode()}) + result = self.run_checker(root) + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + self.assertIn("a.md:2", result.stdout) + + def test_crlf_line_endings_are_refused(self) -> None: + body = f"text\r\n{START} HEAD\r\nours\r\n{SEPARATOR}\r\ntheirs\r\n{END} b\r\n" + root = self.scratch_repo({"a.md": body.encode()}) + result = self.run_checker(root) + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + self.assertIn("a.md:2", result.stdout) + # The separator too: a rstrip that forgets the carriage return would + # report the two markers and silently drop the middle of the hunk. + self.assertIn("a.md:4", result.stdout) + + def test_a_separator_after_the_hunk_closes_is_not_named(self) -> None: + """The adjacency guard, made load-bearing. + + Written because the first mutation run found it was not. Dropping + `open_at is not None` left every other case green: a file with no marker + exits the scan early, so the guard never decided anything there. It only + decides in a file that HAS a marker and also carries a legal rule of + seven `=` outside the hunk, and the observable is the REPORT rather than + the exit code, which is 1 either way. + """ + body = ( + "text\n" + f"{START} HEAD\n" + "ours\n" + f"{SEPARATOR}\n" + "theirs\n" + f"{END} other\n" + "A heading\n" + f"{SEPARATOR}\n" + ) + root = self.scratch_repo({"a.md": body.encode()}) + result = self.run_checker(root) + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + # Positive control first: the report does name lines, so a later + # assertion of absence is about this line and not about a broken match. + self.assertIn("a.md:2", result.stdout) + self.assertIn("a.md:4", result.stdout) + self.assertIn("a.md:6", result.stdout) + self.assertNotIn("a.md:8", result.stdout, result.stdout) + + def test_an_unmerged_index_is_counted_once(self) -> None: + """A live conflict must not triple-count the file it conflicts on. + + `git ls-files` emits stages 1, 2 and 3 for a conflicted path, so without + the dedupe in `tracked_paths` this reads `9 findings in 1 file; examined + 3 tracked text files` for one file. The verdict is 1 either way, so the + observable is the COUNT, which is wrong in precisely the state this gate + exists for. + """ + root = self.conflicted_repo( + base="| a | one |\n", ours="| a | ours |\n", theirs="| a | theirs |\n" + ) + result = self.run_checker(root) + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + self.assertIn("3 findings in 1 file;", result.stdout) + self.assertEqual(self.examined_count(result.stdout), 1) + self.assertIn("1 tracked paths", result.stdout) + + def test_two_paths_sharing_a_colon_prefix_count_as_two_files(self) -> None: + """The summary count, made falsifiable. + + The file count used to be derived by splitting each report line on its + FIRST colon, so two paths that share everything before a colon collapsed + into one. A colon is legal in a POSIX filename and git tracks it. + """ + body = f"{START} HEAD\n".encode() + root = self.scratch_repo({"weird:one.md": body, "weird:two.md": body}) + result = self.run_checker(root) + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + self.assertIn("2 findings in 2 files;", result.stdout) + + def test_the_failure_report_names_the_remedy(self) -> None: + root = self.scratch_repo({"a.md": f"{START} HEAD\n".encode()}) + result = self.run_checker(root) + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + self.assertIn("indent", result.stdout.lower()) + + +class OrdinaryTextTests(CheckerCase): + """The other direction: the gate must not fire on legal markdown.""" + + def test_a_bare_separator_alone_is_not_refused(self) -> None: + # A setext heading underline. Exactly seven `=`, no marker anywhere. + body = f"A heading\n{SEPARATOR}\n\nbody text\n" + root = self.scratch_repo({"a.md": body.encode()}) + result = self.run_checker(root) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + + def test_a_long_rule_of_equals_is_not_refused(self) -> None: + # The shape docs/bench-evidence/*.log and the tokenizer corpora carry. + body = "=" * 40 + "\nrun output\n" + "=" * 8 + "\n" + root = self.scratch_repo({"a.log": body.encode()}) + result = self.run_checker(root) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + + def test_a_separator_outside_an_open_hunk_is_not_reported(self) -> None: + # Separator, then text, then separator: no marker opened a hunk, so + # neither line is a finding and the file is clean. + body = f"one\n{SEPARATOR}\ntwo\n{SEPARATOR}\n" + root = self.scratch_repo({"a.md": body.encode()}) + result = self.run_checker(root) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + + def test_a_marker_that_is_not_at_column_zero_is_not_refused(self) -> None: + # The documented remedy for a document that quotes a marker on purpose. + body = f" {START} HEAD\n {SEPARATOR}\n {END} other\n" + root = self.scratch_repo({"guide.md": body.encode()}) + result = self.run_checker(root) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + + +class SkipTests(CheckerCase): + """What is not read, and the counts that say so.""" + + def test_a_binary_file_with_marker_bytes_is_skipped(self) -> None: + blob = b"\x00\x01\x02" + f"\n{START} HEAD\n".encode() + b"\x00" * 16 + root = self.scratch_repo({"t.bin": blob, "a.md": b"clean\n"}) + result = self.run_checker(root) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertIn("1 binary", result.stdout) + self.assertEqual(self.examined_count(result.stdout), 1) + + def test_a_symlink_is_skipped(self) -> None: + root = self.scratch_repo({"a.md": b"clean\n"}) + os.symlink("a.md", root / "link.md") + subprocess.run( + ["git", "-C", str(root), "add", "-A"], check=True, capture_output=True + ) + result = self.run_checker(root) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertIn("1 symlink", result.stdout) + + def test_a_tracked_path_absent_from_the_worktree_is_skipped(self) -> None: + root = self.scratch_repo({"a.md": b"clean\n", "gone.md": b"clean\n"}) + (root / "gone.md").unlink() + result = self.run_checker(root) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertIn("1 absent", result.stdout) + + def test_an_untracked_file_is_not_examined(self) -> None: + root = self.scratch_repo({"a.md": b"clean\n"}) + (root / "scratch.md").write_bytes(f"{START} HEAD\n".encode()) + result = self.run_checker(root) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertEqual(self.examined_count(result.stdout), 1) + + +class UnreadableTests(CheckerCase): + """A file this run could not read is not a file it may call clean.""" + + def test_an_unreadable_tracked_file_exits_two_and_names_no_merge(self) -> None: + root = self.scratch_repo({"a.md": b"clean\n", "locked.md": b"clean\n"}) + locked = root / "locked.md" + locked.chmod(0o000) + self.addCleanup(locked.chmod, 0o644) + result = self.run_checker(root) + # 2, not 1: the run cannot report on that file, and unknown is not + # absence. It used to be filed as a finding, which exited 1 and printed + # "Resolve the merge before committing" for an I/O error. + self.assertEqual(result.returncode, 2, result.stdout + result.stderr) + self.assertIn("locked.md: unreadable:", result.stdout) + self.assertIn("0 findings", result.stdout) + self.assertNotIn("Resolve the merge", result.stdout) + + +class ReportingTests(CheckerCase): + """A gate that cannot say how much it examined has not reported.""" + + def test_a_tree_with_no_text_files_exits_two(self) -> None: + root = self.scratch_repo({}) + result = self.run_checker(root) + self.assertEqual(result.returncode, 2, result.stdout + result.stderr) + self.assertEqual(self.examined_count(result.stdout + result.stderr), 0) + + def test_the_report_names_the_resolved_root(self) -> None: + root = self.scratch_repo({"a.md": b"clean\n"}) + result = self.run_checker(root) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + # A checker resolves its root from its own path, so a run from a linked + # worktree can read the shared checkout while printing OK. The report + # says which tree the verdict is about. + self.assertIn(str(root.resolve()), result.stdout) + + def test_the_shipped_tree_is_clean(self) -> None: + result = subprocess.run( + [sys.executable, str(CHECKER)], + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + examined = self.examined_count(result.stdout) + # EQUALITY against git's own idea of the tracked text set, DERIVED at + # read time. This replaces a `> 1000` floor, which was 27% of the real + # 3733 and would have stayed green over a scan that collapsed to + # markdown alone. A stored number is the drift lock AGENTS.md forbids; + # this stores nothing and re-derives both sides on every run. + # + # `git grep -e ''` matches every LINE, so a tracked file with no lines + # at all is absent from git's set while the checker reads it and counts + # it. Empty files are therefore added back here rather than being + # tolerated by a fuzzy comparison. The tree carried none when this was + # written, so the term is zero today and correct the day it is not. + text_set = set( + subprocess.run( + ["git", "-C", str(ROOT), "grep", "-I", "--name-only", "-e", ""], + capture_output=True, + text=True, + check=False, + ).stdout.split("\n") + ) - {""} + tracked = [ + n + for n in subprocess.run( + ["git", "-C", str(ROOT), "ls-files", "-z"], + capture_output=True, + check=True, + ).stdout.decode("utf-8", "surrogateescape").split("\0") + if n + ] + empty = { + n + for n in tracked + if not (ROOT / n).is_symlink() + and (ROOT / n).is_file() + and (ROOT / n).stat().st_size == 0 + } + self.assertEqual( + examined, + len(text_set | empty), + "the checker's text set no longer agrees with git's. A file marked " + "binary by .gitattributes but carrying no NUL byte is one way to " + "reach this, and so is a scan that stopped early.\n" + result.stdout, + ) + + +class RegistrationTests(unittest.TestCase): + """A gate registered nowhere runs nowhere.""" + + def test_the_checker_is_registered_in_preflight_and_ci(self) -> None: + preflight = PREFLIGHT.read_text() + self.assertIn("check-conflict-markers", preflight) + self.assertIn("test_check_conflict_markers", preflight) + self.assertIn("check-conflict-markers", CI.read_text()) + + +if __name__ == "__main__": + # unittest.main() alone would exit before this file could assert that + # anything ran. A suite that ran zero cases prints OK and proves nothing. + outcome = unittest.main(exit=False, verbosity=2) + ran = outcome.result.testsRun + print(f"cases run: {ran}") + if ran == 0: + print("FAIL: zero cases ran, so this suite reported nothing") + raise SystemExit(2) + raise SystemExit(0 if outcome.result.wasSuccessful() else 1) diff --git a/tests/scripts/test_check_pr_size.py b/tests/scripts/test_check_pr_size.py index 1b5257573..201c092c0 100755 --- a/tests/scripts/test_check_pr_size.py +++ b/tests/scripts/test_check_pr_size.py @@ -499,6 +499,14 @@ def test_every_created_checker_has_closed_bootstrap_evidence(self) -> None: # including the clean-tree case, which asserts a checked count at or # above the recorded floor and so cannot be satisfied by silence. "scripts/check-symbol-anchors.py", + # 2026-08-20: the conflict-marker gate (#1417). Created in the same + # range, so it has no BASE version to mutate. The disabled stub + # exits 0 and prints nothing, which fails 16 of its 21 cases -- + # measured. The five that survive assert only that an ordinary + # document exits 0, or read no checker at all, so silence satisfies + # them; every case that reads an exit code of 1 or an examined count + # goes red. + "scripts/check-conflict-markers.py", } self.assertEqual(set(checker.CREATION_MUTATIONS), expected) for path, mutation in checker.CREATION_MUTATIONS.items():