From c7e5e23faca3a03b01322b27a03f602dd17df5e5 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Sun, 16 Aug 2026 23:58:42 +0000 Subject: [PATCH] fix(#595): doc-checkpoint asks whether the registry moved, not whether a file did `FEATURE_SURFACE_PREFIXES` covered all of `src/vllm/model_executor/models/`, so any edit to any model translation unit classified as `feature_surface` and owed `docs/FEATURES.md`. That is the classify-by-directory trigger this file's own header says the 2026-08-11 rewrite removed, still standing for model files: > A one-line compile fix owed three public-doc edits, so this gate produced 16 > of the last 20 red CI runs, and it had accreted SIX hardcoded exact-path-set > escape hatches -- one per legitimate change it had blocked. > [...] Editing src/ alone owes nothing. The cost stopped being hypothetical on 2026-08-16. `e34d71379` (#1054) is a one-line lambda-capture change to `models/qwen3_5_weights.cpp` that alters no capability. The gate demanded the surface; the commit answered with prose in BENCHMARKS, FEATURES and STATUS; that prose crossed the `check-public-doc-tables` paragraph budgets; and because that checker also runs in the pre-push hook, every branch in the repository was blocked from pushing. That is #1055, re-filed by a second agent as #1062 with #1064 as a duplicate fix PR, alongside #1058 which is still open. Two shared-file gates in series, each individually defensible. What the project supports is what the registry registers, so `feature_surface` now keys off a change to the set of `REGISTER_VLLM_MODEL(...)` entries in the touched file, read through `blob()` like the existing `measurement_changes`. `scripts/check-supported-models.py` already gates `docs/FEATURES.md` against that same set, so the signal is authoritative rather than newly invented. This NARROWS a gate, which is the risky direction, so the polarity is pinned by test rather than asserted: adding an architecture, adding a whole new registered file, and removing an architecture each still owe `docs/FEATURES.md`, and the four `.agents/*-matrix.md` records keep their path trigger because editing one IS the claim. Red before, green after, and mutation-proven. `test_doc_checkpoint.py` goes 27 -> 32 cases. Before the implementation, `test_editing_a_registered_model_owes_nothing` failed with the FEATURES demand. After it, 32 pass. Because the three still-owes cases were green under the OLD path trigger too, they could not by themselves show the new trigger works, so `registration_changes` was mutated to return `[]`: that fails exactly those three and no others, and restoring returns 32 green. Does not close #595. A genuine new architecture still writes the shared `docs/FEATURES.md` table, so the lock that issue names survives; this removes the contention for every fix, refactor and port phase that changes no registration. #515 is the identical shape for `CMakeLists.txt` -> `docs/USAGE.md` and is untouched. Both are recorded under `## Owed` in the spec, together with the class this narrowing gives up: a capability change inside an already-registered model now goes undemanded. Refs #595 FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: ClaudeCode:claude-opus-5 [ClaudeCode] --- .agents/issue-index.md | 1 + .../specs/doc-checkpoint-feature-trigger.md | 139 ++++++++++++++++++ scripts/check-doc-checkpoint.py | 46 +++++- tests/scripts/test_doc_checkpoint.py | 67 ++++++++- 4 files changed, 248 insertions(+), 5 deletions(-) create mode 100644 .agents/specs/doc-checkpoint-feature-trigger.md diff --git a/.agents/issue-index.md b/.agents/issue-index.md index b501d993d..46639e3db 100644 --- a/.agents/issue-index.md +++ b/.agents/issue-index.md @@ -317,3 +317,4 @@ rather than merged. `scripts/check-agent-record.py` gates both. | [#1096](https://github.com/mudler/vllm.cpp/issues/1096) | `ROAD-V1-LTX25` | `KeyframeInterpolationPipeline` (`keyframe_interpolation.py` @ `fd4ded7f`) is absent with no `Ltx2UnportedPipelineFeature` marker, and its conditioning building block IS served: `Ltx2ConditionVideoByKeyframe` (`ltx2_conditioning.h:172`, `.cpp:530`) is reached from `include/vllm.h:935` through `vllm_c.cpp:1635,:1646` to `ltx2_video.cpp:2745-2751`, mutation-proven at [`ltx25-token-append.md`](specs/ltx25-token-append.md):270. Blocked on (a) no multi-keyframe request surface: the ABI carries two scalar slots, `first_frame` and `last_frame` (`include/vllm.h:934-935`), the engine request two paths and one blob (`video_engine.h:89-93`), and the indices are hard-coded (`latent_idx=0` at `ltx2_video.cpp:2699`, `frame_idx=frames-1` at `:2750`); the CLI exposes only `--first-frame` (`examples/ltx2_gen/main.cpp:269`) and the server only a first frame (`video_engine.cpp:365-372`), against upstream's repeatable `--image PATH FRAME_IDX STRENGTH [CRF]` (`utils/args.py:805-817`, expanded per keyframe at `utils/helpers.py:343-367`). `num_generated_keyframes` is a DIFFERENT feature (model-invented interior slots, `ltx2_video.cpp:1328-1354`) and must not be mistaken for it. (b) A per-sigma guided denoiser: ours is one struct per PHASE (`ltx2_pipeline.h:526-527`, assigned `ltx2_pipeline.cpp:1069-1070`) and audio-only, with `git grep "build_from_sigma|GuiderFactory|per_sigma"` returning 0 against a `sigma` control of ~10 lines in the same header; upstream resolves guiders per step from sigma (`utils/denoisers.py:304-361`, `ltx-core/components/guiders.py:294-342`). REJECTS the audit's "pure porting, no missing checkpoint": `--distilled-lora` is `required=True` on the parser this pipeline uses (`utils/args.py:1146`, selected at `keyframe_interpolation.py:301`, consumed `:111-122`) and stage 1 runs the full `-dev-` transformer; neither file is on the NAS. It needs no IC-LoRA, which is the half of that framing that holds | feature | | [#1097](https://github.com/mudler/vllm.cpp/issues/1097) | `ROAD-V1-LTX25` | `ltx2-gen --lora a --lora b` fuses `b`, DISCARDS `a` and exits 0. `SetExtra` (`examples/ltx2_gen/main.cpp:212-221`) overwrites an existing key in place, so N invocations of `--lora` (`:255-262`) leave exactly one `lora_path` extra. `docs/USAGE.md:809-813` published the opposite - "a second `--lora`" as one of three things that refuse by name - and is corrected in the change that filed this. The wider half, measured after filing: the refusal is unreachable from EVERY production entry point, not only the CLI. `ltx2_video.cpp:813` is the only `dit_options.loras.push_back` in the tree and runs at most once under `if (!lora_path.empty())`, so `options.loras.size()` is 0 or 1 for the CLI, for `vllm_video_engine_load` and for the server alike; `Ltx2ResolveLoraReferenceFactors`'s `> 1` branch (`ltx2_lora.cpp:243-248`) is reached only by `test_ltx2_lora.cpp:384,:492`. So it is correct code guarding a state nothing can construct yet, and the state it guards is what N-adapter fusion ([#932](https://github.com/mudler/vllm.cpp/issues/932)) introduces - which is where the reachability half belongs. Two fix shapes, neither chosen here: refuse the second `--lora` in the CLI, or accumulate and let the library refusal fire. Second defect in the same area and from the same landing: `ltx2_video.cpp:362-363` says "nine of these ten reach a reader" about `kKnownLoadExtras`, which now holds TWELVE entries (`:377-383`) after `lora_path` and `lora_strength` landed with #923; eleven of twelve reach a reader and `duration_head_path` is still the one that does not | bug | | [#1098](https://github.com/mudler/vllm.cpp/issues/1098) | `ROAD-V1-LTX25` | `README.md` cannot be corrected, and TWO gates each refuse the fix independently. The claims that are wrong right now: **"37 registered architectures"** four times (`README.md:11,:80,:253,:296`) where `docs/FEATURES.md` says **40** in three places, corrected two commits earlier in `9143196c7`; and **ZERO `LTX` occurrences** against a `minimax` control of 7, so the video-generation announcement names one of the two shipped video families. Blocker 1: `README.md` measures **29,989 chars against `MAX_README_CHARS = 30000`** (`scripts/check-readme-structure.py:47`), so the `LTX-2.5` matrix row (~130 chars) could only land by DELETING another architecture's row - the shared-file lock AGENTS.md forbids in its own words ("Limit an entry, not a shared file"), and the third instance after the two whole-file budgets [#364](https://github.com/mudler/vllm.cpp/issues/364) retired on that argument, `MAX_CHARS` in `check-now-current.py` and the `chars` key in `check-public-doc-tables.py`. The per-ENTRY caps beside it (`MAX_CELL_CHARS = 220`, `MAX_PARAGRAPH_CHARS = 900`) are what actually stop a landing page decaying into a status log. Blocker 2, which is the decisive one: `check-doc-checkpoint.py:346-354` refuses ANY README change that does not also touch a LANDING SOURCE (`:104-113` - `.agents/mission.md`, `CMakeLists.txt`, three `benchmarks/demo/*.json`, `examples/{cli,server}/main.cpp`), evaluated PER COMMIT ([#573](https://github.com/mudler/vllm.cpp/issues/573)) so splitting the edit out does not help, and its own comment calls the rule deliberate and directly tested. It has no arm for a README CORRECTION as against README CHURN, and a correction has a natural witness: the value disagrees with the projection that owns it. So a two-family paragraph was written, MEASURED to fit at 445 chars against the old 438 with 4 to spare, and then REVERTED unlanded; it is preserved verbatim in the issue thread rather than lost. Asked: whether the checkpoint gate should distinguish correction from churn, and whether the whole-file cap should exist at all. NOT asked: raise the constant, which is widening an assertion to keep a gate green | bug | +| [#595](https://github.com/mudler/vllm.cpp/issues/595) | — | `check-doc-checkpoint` keys `feature_surface` off the PATH `src/vllm/model_executor/models/`, so every edit to any model TU owes `docs/FEATURES.md` — the same classify-by-directory defect the file's own header says its rewrite removed for `src/`, `include/` and `tests/`. Measured cost: `e34d71379` (#1054) is a one-line lambda-capture change that alters no capability; the gate demanded the surface, the commit answered with prose, the prose crossed the `check-public-doc-tables` paragraph budgets, and because that checker also runs in the pre-push hook it blocked EVERY branch in the repository from pushing ([#1055](https://github.com/mudler/vllm.cpp/issues/1055), re-filed as [#1062](https://github.com/mudler/vllm.cpp/issues/1062) with a duplicate fix PR, plus [#1058](https://github.com/mudler/vllm.cpp/issues/1058) still open). The repair for the MSVC break the same commit caused ([#1068](https://github.com/mudler/vllm.cpp/issues/1068)) hit the identical demand. Narrowed here to a change in the set of `REGISTER_VLLM_MODEL(...)` registrations, which is what `check-supported-models.py` already gates the table against; adding, removing or renaming an architecture still owes the surface. The LOCK this issue names is NOT closed by that — a genuine new architecture still writes the shared table — so #595 stays open, listed under `## Owed` in [`doc-checkpoint-feature-trigger.md`](specs/doc-checkpoint-feature-trigger.md). Sibling shape for `CMakeLists.txt` -> `docs/USAGE.md` is [#515](https://github.com/mudler/vllm.cpp/issues/515) | bug | diff --git a/.agents/specs/doc-checkpoint-feature-trigger.md b/.agents/specs/doc-checkpoint-feature-trigger.md new file mode 100644 index 000000000..2bd9d821e --- /dev/null +++ b/.agents/specs/doc-checkpoint-feature-trigger.md @@ -0,0 +1,139 @@ +# doc-checkpoint: key `feature_surface` off registrations, not paths + +Issue: [#595](https://github.com/mudler/vllm.cpp/issues/595) +Row: none. This is a checker-semantics repair, not a roadmap item; #595 is +carried under `## Owed` below, which is the index's other admissible shape. + +## Now + +`IMPLEMENTING`. The trigger change and its tests are written; the gate evidence +below is captured on this branch. + +## Scope + +`scripts/check-doc-checkpoint.py:79` sets + +```python +FEATURE_SURFACE_PREFIXES = ("src/vllm/model_executor/models/",) +``` + +so **any** edit to any file under that directory classifies the change as +`feature_surface` and demands a `docs/FEATURES.md` edit. + +In scope: replace that path trigger with a content trigger — a change to the set +of `REGISTER_VLLM_MODEL(...)` registrations in the touched file. + +Out of scope, and explicitly NOT fixed here: + +- `FEATURE_SURFACE_FILES` (the four `.agents/*-matrix.md` records). Those are + claim surfaces; editing one **is** a claim change and keeps its path trigger. +- `USER_USAGE_FILES` / `CMakeLists.txt`, which is the identical shape reported + separately as [#515](https://github.com/mudler/vllm.cpp/issues/515). +- The lock that #595 names in full. A genuine new architecture still writes the + shared `docs/FEATURES.md` table. This change removes the contention for every + fix, refactor and port phase that changes no registration; it does not + relocate the obligation to a per-row surface, which is #595's larger ask. + +## Why + +The checker's own docstring already argues this: + +> A one-line compile fix owed three public-doc edits, so this gate produced 16 +> of the last 20 red CI runs, and it had accreted SIX hardcoded exact-path-set +> escape hatches -- one per legitimate change it had blocked. +> [...] Editing src/ alone owes nothing. + +The 2026-08-11 rewrite removed path classification for `src/`, `include/` and +`tests/` generally, but kept it for `models/`. So the failure the rewrite exists +to prevent still reproduces, restricted to model files. + +Measured cost on 2026-08-16. `e34d71379` (#1054) is a one-line lambda-capture +change to `models/qwen3_5_weights.cpp` that alters no capability. The gate +demanded `docs/FEATURES.md`; the change answered with prose in `BENCHMARKS.md`, +`FEATURES.md` and `STATUS.md`; that prose crossed the `check-public-doc-tables` +paragraph budgets, which reds `main` **and** runs in the pre-push hook, so every +branch in the repository was blocked from pushing. That is +[#1055](https://github.com/mudler/vllm.cpp/issues/1055) (fixed by #1057), +re-filed by a second agent as +[#1062](https://github.com/mudler/vllm.cpp/issues/1062) with #1064 as a +duplicate fix PR. [#1058](https://github.com/mudler/vllm.cpp/issues/1058) is a +third #1054 fallout and remains open. The repair for the MSVC break the same +commit introduced (#1068, PR #1069) hit the identical demand and had to argue an +exception in its commit body. + +Two shared-file gates in series, each individually defensible, took `main` down +and blocked every push. + +## Design + +`REGISTER_VLLM_MODEL(` is the registry's own entry point, and +`scripts/check-supported-models.py` already gates `docs/FEATURES.md` against +exactly that set, so the signal is authoritative and already load-bearing. + +Add, mirroring the existing `measurement_changes` shape so the trigger is +content-based and reads through `blob()`: + +```python +def registrations(text: str) -> set[str] +def registration_changes(paths, before, after) -> list[str] +``` + +`classify()` adds `feature_surface` when `registration_changes` is non-empty, +and no longer adds it from `FEATURE_SURFACE_PREFIXES`. + +Polarity, stated because it is the risk: this NARROWS a gate. Adding an +architecture, removing one, and renaming one all still owe `docs/FEATURES.md`, +because all three change the registration set. A file added with a registration +reads as a change from the empty set, and a deleted file reads as a change to +it. + +## Risks + +- **Narrowing lets a real claim change through.** A capability change inside an + already-registered model — a new quantized arm, a refusal that becomes a + render — changes no registration and would no longer be demanded. Accepted: + that class was never reliably caught either, since the gate could be satisfied + by any `FEATURES.md` edit including an unrelated one, and `docs/FEATURES.md` + row content is separately gated by `check-supported-models.py`. Recorded under + `## Owed`. +- **Regex vs the real parser.** `REGISTER_VLLM_MODEL` inside a comment or a + string would count. Accepted: the same false positive fails toward DEMANDING a + doc edit, which is the safe direction. + +## Tests + +`tests/scripts/test_doc_checkpoint.py`, red before the implementation: + +1. `test_editing_a_registered_model_owes_nothing` — a model file whose + registration set is unchanged demands nothing. **RED before**, because the + path trigger fires today. +2. `test_a_new_model_registration_owes_the_feature_surface` — adding a + `REGISTER_VLLM_MODEL` without touching `docs/FEATURES.md` is refused. +3. `test_removing_a_registration_owes_the_feature_surface` — deleting one is + refused. +4. `test_a_matrix_record_still_owes_the_feature_surface` — the + `.agents/*-matrix.md` path trigger is untouched. + +2, 3 and 4 are green before and after: they pin what must NOT be widened away. + +## Gates + +- `python3 tests/scripts/test_doc_checkpoint.py` +- `python3 scripts/check-doc-checkpoint.py --commit ` on this branch +- `documentation-checkpoint` and `agent-record` in CI + +## Owed + +- The capability-change-without-registration-change class above, which no gate + now covers. Owned by this row; tracked on #595. +- #595 itself stays open. This change does not relocate the obligation to a + per-row surface, which is what closes it. +- #515 is the identical shape for `CMakeLists.txt` -> `docs/USAGE.md` and is not + touched here. + +## Stop conditions + +Stop and report if removing the path trigger makes any existing test in +`tests/scripts/test_doc_checkpoint.py` green-by-absence rather than by intent — +that is the "never make a red gate green by deleting an assertion" failure, and +it means the trigger needs narrowing rather than replacing. diff --git a/scripts/check-doc-checkpoint.py b/scripts/check-doc-checkpoint.py index ad4a72815..36501d7a3 100755 --- a/scripts/check-doc-checkpoint.py +++ b/scripts/check-doc-checkpoint.py @@ -66,8 +66,9 @@ STATE_CELL = re.compile(r"`(" + "|".join(re.escape(s) for s in STATES) + r")`") ROW_ID = re.compile(r"^\|\s*`([A-Z0-9][A-Za-z0-9_.-]*)`") -# Support-surface triggers. These are about WHAT IS SUPPORTED, so they are still -# path-derived -- adding a model file genuinely changes the feature surface. +# Support-surface triggers. These four records ARE the claim, so editing one is +# path-derived. Model SOURCE is not: see registration_changes() (#595), which +# asks whether the registry set moved rather than whether a file was touched. FEATURE_SURFACE_FILES = frozenset( { ".agents/backend-matrix.md", @@ -78,6 +79,10 @@ ) FEATURE_SURFACE_PREFIXES = ("src/vllm/model_executor/models/",) +# The registry's own entry point. `feature_surface` keys off a change to the SET +# of these in a model TU, not off the path -- see registration_changes() (#595). +REGISTRATION = re.compile(r"REGISTER_VLLM_MODEL\(\s*([A-Za-z0-9_:\"]+)") + # Exact user-facing configuration/build/install entrypoints. Deliberately NOT # all of cmake/: toolchain internals do not change installation instructions. USER_USAGE_FILES = frozenset( @@ -269,6 +274,36 @@ def measurement_changes(paths: set[str], before: str, after: str) -> list[str]: ) +def registrations(text: str) -> set[str]: + """The architectures a model TU registers, by registry name.""" + return set(REGISTRATION.findall(text)) + + +def registration_changes(paths: set[str], before: str, after: str) -> list[str]: + """Model files whose set of REGISTER_VLLM_MODEL entries changed (#595). + + Keying `feature_surface` off the PATH made every edit under + `src/vllm/model_executor/models/` owe docs/FEATURES.md, which is the same + "classify by directory" defect this file's header says the rewrite removed + for src/, include/ and tests/. A one-line compile fix there owed a public + doc edit with nothing true to say; #1054 answered that demand with prose, + the prose crossed the check-public-doc-tables budgets, and because that + checker also runs pre-push it blocked EVERY branch in the repo (#1055). + + What the project supports is what the registry registers, which is the same + set scripts/check-supported-models.py already gates FEATURES.md against. So + adding, removing or renaming an architecture is a claim and still owes the + surface; editing the internals of one already registered is not. + """ + + return sorted( + path + for path in paths + if path.startswith(FEATURE_SURFACE_PREFIXES) + and registrations(blob(before, path)) != registrations(blob(after, path)) + ) + + def classify(paths: set[str], before: str, after: str) -> tuple[set[str], list[str]]: """Return (change classes, human-readable reasons).""" classes: set[str] = set() @@ -284,10 +319,15 @@ def classify(paths: set[str], before: str, after: str) -> tuple[set[str], list[s classes.add("lifecycle") reasons.extend(f"{path}: measurement recorded" for path in measured) + registered = registration_changes(paths, before, after) + if registered: + classes.add("feature_surface") + reasons.extend(f"{path}: model registration changed" for path in registered) + for path in sorted(paths): if path in PUBLIC_SURFACES: continue - if path in FEATURE_SURFACE_FILES or path.startswith(FEATURE_SURFACE_PREFIXES): + if path in FEATURE_SURFACE_FILES: classes.add("feature_surface") if path in USER_USAGE_FILES or path.startswith(USER_USAGE_PREFIXES): classes.add("user_usage") diff --git a/tests/scripts/test_doc_checkpoint.py b/tests/scripts/test_doc_checkpoint.py index a692ffeaf..8971cb378 100755 --- a/tests/scripts/test_doc_checkpoint.py +++ b/tests/scripts/test_doc_checkpoint.py @@ -335,8 +335,10 @@ def errors(self, paths): finally: checker.blob = original - def test_a_new_model_owes_the_feature_surface(self): - errors = self.errors(["src/vllm/model_executor/models/newmodel.cpp"]) + def test_a_matrix_record_still_owes_the_feature_surface(self): + # The four .agents/*-matrix.md records ARE claim surfaces, so they keep + # the path trigger. Pins what #595's fix must not widen away. + errors = self.errors([".agents/model-matrix.md"]) self.assertTrue(errors) self.assertIn("docs/FEATURES.md", errors[0]) @@ -392,5 +394,66 @@ def test_a_landing_source_permits_but_does_not_demand_readme(self): self.assertEqual(self.errors([".agents/mission.md"]), []) + + +class FeatureSurfaceTrigger(unittest.TestCase): + """#595: a model file owes docs/FEATURES.md when its REGISTRATIONS change. + + Editing the internals of an already-registered architecture is not a claim + about what the project supports. Keying on the path instead is what made a + one-line compile fix owe a public-doc edit, and #1054 answered that demand + with prose that crossed the check-public-doc-tables budgets and blocked + every push in the repository (#1055, #1058, #1062). + """ + + MODEL = "src/vllm/model_executor/models/somemodel.cpp" + REGISTERED = 'REGISTER_VLLM_MODEL("SomeForCausalLM", SomeModel);\nint f() { return 1; }\n' + EDITED = 'REGISTER_VLLM_MODEL("SomeForCausalLM", SomeModel);\nint f() { return 2; }\n' + ADDED = ( + 'REGISTER_VLLM_MODEL("SomeForCausalLM", SomeModel);\n' + 'REGISTER_VLLM_MODEL("OtherForCausalLM", OtherModel);\n' + ) + + def errors(self, paths, before_text, after_text): + original = checker.blob + + def fake(rev, path): + if path.startswith(".agents/specs/"): + return SPEC_WITH_NOW + return before_text if rev == "BEFORE" else after_text + + checker.blob = fake + try: + return checker.errors_for(set(paths), "BEFORE", "AFTER") + finally: + checker.blob = original + + def test_editing_a_registered_model_owes_nothing(self): + # RED before the #595 trigger change: the path alone fired. + errors = self.errors([self.MODEL], self.REGISTERED, self.EDITED) + self.assertEqual(errors, [], f"a registration-preserving edit demanded: {errors}") + + def test_a_new_model_registration_owes_the_feature_surface(self): + errors = self.errors([self.MODEL], self.REGISTERED, self.ADDED) + self.assertTrue(errors, "adding an architecture must still owe FEATURES.md") + self.assertIn("docs/FEATURES.md", errors[0]) + + def test_a_brand_new_model_file_owes_the_feature_surface(self): + errors = self.errors([self.MODEL], "", self.REGISTERED) + self.assertTrue(errors, "a new registered architecture must owe FEATURES.md") + self.assertIn("docs/FEATURES.md", errors[0]) + + def test_removing_a_registration_owes_the_feature_surface(self): + errors = self.errors([self.MODEL], self.ADDED, self.REGISTERED) + self.assertTrue(errors, "removing an architecture must still owe FEATURES.md") + self.assertIn("docs/FEATURES.md", errors[0]) + + def test_a_satisfied_registration_change_passes(self): + errors = self.errors( + [self.MODEL, "docs/FEATURES.md"], self.REGISTERED, self.ADDED + ) + self.assertEqual(errors, []) + + if __name__ == "__main__": unittest.main()