Drop the .loop scratch state from the tree - #133
Merged
Merged
Conversation
The /polish loop's working files -- polish-seen.md and polish-state.md -- rode into main with #127. They are per-run agent bookkeeping, not project artifacts: nothing in the tree, the build, or CI reads them, and they go stale the moment the branch they describe lands. Remove both and ignore .loop/ alongside .claude/ so a later run cannot commit them again.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved blocking issues were identified.
Pull request overview
Removes stale .loop scratch state and prevents future scratch files from being committed.
Changes:
- Deletes obsolete polish state and findings files.
- Adds
.loop/to.gitignore.
File summaries
| File | Description |
|---|---|
.loop/polish-state.md |
Removes stale run state. |
.loop/polish-seen.md |
Removes stale findings ledger. |
.gitignore |
Ignores future .loop/ scratch files. |
Review details
- Files reviewed: 2/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
zaoxing
added a commit
that referenced
this pull request
Sep 11, 2026
The /polish loop's working files are per-run agent bookkeeping, not project artifacts: nothing in the tree, the build, or CI reads them, and they go stale once the branch they describe has landed. #133 removes them from main; dropping them here too so a merge of this branch cannot carry them back in.
zaoxing
added a commit
that referenced
this pull request
Sep 11, 2026
The /polish loop's working files are per-run agent bookkeeping, not project artifacts: nothing in the tree, the build, or CI reads them, and they go stale once the branch they describe has landed. #133 removes them from main; dropping them here too so a merge of this branch cannot carry them back in.
zaoxing
added a commit
that referenced
this pull request
Sep 11, 2026
The /polish loop's working files are per-run agent bookkeeping, not project artifacts: nothing in the tree, the build, or CI reads them, and they go stale once the branch they describe has landed. #133 removes them from main; dropping them here too so a merge of this branch cannot carry them back in.
zaoxing
added a commit
that referenced
this pull request
Sep 11, 2026
The /polish loop's working files are per-run agent bookkeeping, not project artifacts: nothing in the tree, the build, or CI reads them, and they go stale once the branch they describe has landed. #133 removes them from main; dropping them here too so a merge of this branch cannot carry them back in.
zaoxing
added a commit
that referenced
this pull request
Sep 11, 2026
The /polish loop's working files are per-run agent bookkeeping, not project artifacts: nothing in the tree, the build, or CI reads them, and they go stale once the branch they describe has landed. #133 removes them from main; dropping them here too so a merge of this branch cannot carry them back in.
zaoxing
added a commit
that referenced
this pull request
Sep 22, 2026
* [Docs] Add DMI-configurator design plan
Design doc for DMI-configurator: a configuration tool that turns a model
descriptor plus visual selections into validated DMI runtime YAML.
YAML does not replace the existing configuration mechanisms. It is added in
front of them via a loader/compiler pair:
config.yaml -> load_config() -> DMIConfig -> compile_config()
-> CompiledDMIConfig -> existing DMI runtime
load_config() knows nothing about the runtime; compile_config() knows nothing
about YAML.
Includes an appendix verifying the design against the current code
(CaptureSchedule, the hook catalog, HookSpec.layer_no, select_hook_specs, and
the attach_model integration point), and the open schema decisions that must be
settled before Phase 1.
* [Feat] DMI-configurator: structured configuration, YAML, and web UI
Implements phases 1-7 of docs/dmi-configurator-plan.md. YAML does not replace
DMI's existing configuration mechanisms; it sits in front of them:
config.yaml -> load_config() -> DMIConfig -> compile_config()
-> CompiledDMIConfig -> existing DMI runtime
load_config() knows nothing about the runtime; compile_config() knows nothing
about YAML.
New package dmi.configuration:
schema.py DMIConfig, ObservationConfig, LayerSelection, descriptors
manifest.py descriptor load/validate; the only place HF naming meets
ModelShapeConfig naming
catalog_adapter.py projects HOOK_DEFS into labels, UI groups, availability
architecture.py structural metadata driving the diagram
validation.py issues addressed to the control that caused them
yaml.py canonical serialization; parse(dump(c)) == normalize(c)
compatibility.py to/from the existing comma-separated selection string
compiler.py DMIConfig + ModelContext -> CompiledDMIConfig
The hook catalog stays authoritative: availability mirrors the suppression
already in select_hook_specs rather than restating it, and every catalog hook
is reachable from the UI even if it has no presentation entry.
Layer selection is structured, never encoded into hook syntax. It compiles to
a new additive spec filter, dmi.hooks.selection.filter_by_layers, following the
same convention as filter_by_pp_rank/filter_by_tp_rank. Global hooks
(layer_no == -1) are never dropped by a range.
dmi.ui serves a localhost FastAPI app over vanilla HTML/CSS/JS/SVG -- no React,
no Node build. Every endpoint delegates to dmi.configuration, so UI validity is
DMI validity. The browser gets no filesystem access: Save writes only to the
server-side path named on the command line.
dmi ui MODEL_DESCRIPTOR [--config CONFIG] [--host] [--port]
The web dependencies are an optional [ui] extra; a normal install pulls in no
web framework.
Runtime policy (phase 8) is carried and serialized but not connected to runtime
behaviour, and the UI says so explicitly rather than implying an effect it does
not have.
Tests: 141 new CPU tests covering descriptors, schema, validation, YAML
round-tripping against golden files, the legacy bridge, compilation, and the
HTTP contract.
* [Feat] Derive model descriptors from framework configs
Descriptors were hand-typed, which was both tedious and unsafe: a wrong
num_layers lets the UI offer layers the model does not have, validation passes,
and filter_by_layers then silently keeps fewer layers than requested. The
framework already knows the model, so read it from there.
dmi.configuration.introspect derives a descriptor from a Hugging-Face-shaped
config. It reuses make_model_shape_from_hf_config, which already extracts 8 of
the 9 topology fields, and adds the layer count (num_hidden_layers / n_layer /
num_layers) plus identity. Reading a config.json needs no transformers install:
the extractor uses getattr, so parsed JSON in a namespace works. transformers
is imported lazily and only to resolve a bare model id.
Two entry points:
dmi describe-model ./Qwen3-8B --output qwen3-8b.yaml
dmi ui ./Qwen3-8B # also config.json, a model id, or a descriptor
dmi ui now resolves any of those sources, so a descriptor file is an override
rather than a required input. Encoder-decoder configs are refused rather than
mis-rendered as decoders.
A descriptor file still earns its place: the configurator runs where the model
is not loaded, so it stays the portable design-time record. The runtime never
reads it -- compile_config takes its shape from the adapter's live
detect_model_shape(model).
Examples: qwen3-8b.yaml and qwen3-30b-a3b.yaml carried numbers I could not
verify and are gone. The MoE case moves to tests/data/moe-decoder.model.yaml,
labelled synthetic rather than claiming to be a real model. llama3-8b.yaml
stays as the one worked example, and a test now pins it against what
describe-model produces for a real Llama 3 config.
Tests: 23 new CPU tests covering extraction across HF naming variants
(llama/gpt2), MoE fields, head_dim emission, identity from directory vs
config.json, and every rejection path.
* [Fix] Make configuration parsing strict and CI actually run the suite
Five review findings, all about failures that were silent.
CI was red on every PR touching this feature. The workflow installs
`--no-deps -e .` plus a hand-picked list, so requirements.txt is never
consulted and pyyaml was absent: nine test modules failed to collect and
`make check` exited 2. Adding pyyaml fixes it. Verified by blocking every
non-CI dependency locally, which reproduces the failure exactly, then
restoring only pyyaml.
The same workflow never installed the [ui] extra, so
tests/test_configurator_api.py skipped itself rather than failing -- all 41
tests of the HTTP contract, the whole backend, never ran on any PR. fastapi,
uvicorn and httpx are now installed so they do.
Parsing was strict for `schedule` and lax everywhere else, which made a typo
dangerous rather than loud: `observations: {layer: ...}` (missing the plural)
parsed as "no layer range", `_validate_layers` returned early because layers
was None, and a capture meant for 8 layers of a 32-layer model ran on all 32
with no issue reported at any severity. Every section now refuses unknown
keys through one helper.
`version` was defaulted to the current version when absent, which is the
guess that version dispatch exists to avoid -- an unversioned v2 file written
by a newer tool would have been read as v1. It is now required.
A YAML path that does not exist fell through to Hugging Face id resolution
and blamed a missing `transformers` install for what is almost always a shell
typo. A .yaml/.yml suffix is a file path, so a missing one now says so.
The "Other observations" fallback node hardcoded layer scope, so an
unassigned *global* hook would have been drawn inside the "Transformer layer
x N" group -- implying per-layer capture and that the layer range applied to
it. Leftovers now split by scope. The two new scope tests fail against the
old code, confirming they are not vacuous.
* [Fix] Address review comments on the configurator
Three items from the PR review.
The typed-schema and compilation snippets in the plan still showed a
`runtime: RuntimeConfig` field on DMIConfig and CompiledDMIConfig. The
implementation deliberately has no runtime block -- the appendix says so and
explains why -- so the snippets were telling readers the opposite of the
decision. The appendix note about RuntimeConfig overlapping RingConfig stays;
it is the record of that decision, not a stale snippet.
python-multipart is dropped from the [ui] extra. The backend accepts JSON
only: the browser reads a configuration file itself and posts its contents,
so there is no multipart/form-data surface. Verified no UploadFile or Form
usage anywhere under dmi/ui, and the API suite passes without it installed.
A non-integer `version` is now a malformed document rather than an
unsupported one. YAML hands back the string "1" for a quoted value, and
"version '1' is not supported by this build" would send the reader looking
for a build that reads it instead of at the quotes.
* DMI-configurator: apply layer ranges at runtime, estimate payload cost, and make it launchable (#121)
* [Docs] Review the optimization policy capture plan against current runtime
Holds the proposed policy/resolver design against the code on main. Records
three blocking findings: CaptureSchedule is defined and exported but no adapter
enforces it, the transport already blocks losslessly in prepare_step and has no
drop path (inverting the proposal's pressure assumptions), and per-layer
capture selection does not exist. Adds six design-level findings and a
dependency-ordered resequencing of the delivery phases.
* [Feat] Apply configurator layer ranges and estimate payload cost
Two gaps in the configurator, both about the UI telling the truth.
Layer selection was authored but never applied. attach_model had no way to
receive a range, so a range picked in the UI changed nothing at runtime --
the same defect class as the policy control, except that one is disclaimed
and this one was not. attach_model now takes a keyword `layers` argument and
applies filter_by_layers between apply_hook_selection and the PP/TP filters,
driven from a DMIConfig by configuration.attach_config.
The parameter is the range rather than a CompiledDMIConfig as the original
design assumed. Handing attach_model compile_config's spec list would leave
every deselected hook live: HookPoint.enabled defaults to True and only
apply_hook_selection walks the unselected specs to turn them off, so
attach_model has to own selection. Keeping the parameter primitive also keeps
configuration depending on the runtime rather than the reverse.
Payload estimation was out of scope for the MVP but needs no policy resolver,
so it lands now as configuration.estimate plus a UI panel. It reuses
compute_hook_shape and plan_step's align_up(_, 16) rounding, so the figure
shown and the figure prepare_step enforces cannot drift. It reports the peak
single step against min(payload, pinned) -- the constraint that actually
binds, since exceeding it silently drops the adapter to eager CPU-direct
dispatch -- and names the worst rank, because sharded hooks divide by tp_size
while unsharded hooks sit on rank 0 alone and PP_FIRST/PP_LAST skew the
stages. Workload and ring sizes stay UI-local and out of the YAML, keeping
DMIConfig free of a runtime block.
Also: visible focus rings (the architecture nodes carry tabindex but had no
focus style), focus restored across the SVG re-render that node activation
triggers, and a prefers-reduced-motion guard.
65 new CPU tests. The 5 pre-existing fsync failures in tests/test_capture_*
are untouched by this branch.
* [Feat] Make the configurator easy to launch
Running it took a PYTHONPATH prefix, a module path, and a descriptor path you
had to know, then opening a browser by hand. Now:
make ui
from a checkout, with nothing installed. Override with MODEL=./my-model.
- MODEL is optional. With none given, dmi ui uses the descriptor in the
current directory when there is exactly one. Several, or none, and it lists
what it found or points at describe-model rather than guessing.
- The browser opens once the port actually accepts a connection, polled rather
than slept, so a slow first import does not open a dead page. --no-browser
opts out.
- The default port walks forward to the next free one, so a second
configurator does not die on address-in-use. An explicit --port is never
moved: a named port that is busy should fail loudly.
- The startup banner is flushed, since it carries the URL and a piped or
make-wrapped stdout otherwise held it until exit. It now also reports the
model geometry.
- README documents the configurator for the first time.
The repo-specific default model lives in the Makefile, not in descriptor
discovery: the shipped CLI stays generic, and the example descriptor does not
match the *.model.yaml convention anyway.
30 new CPU tests. The port and browser tests drive the logic through injected
seams rather than binding real sockets, which a restricted CI refuses; real
socket behaviour was verified separately.
* [Test] Cover the architecture diagram's unassigned-hook fallback
`architecture_payload` documents that "extending HOOK_DEFS can never make
an observation unreachable from the UI". That guarantee rests entirely on
the trailing "Other observations" node, and nothing exercised it: every
catalog hook is currently assigned to a real block, so the fallback branch
never ran under test and could rot until the day someone adds a hook.
Adds two tests -- every catalog hook is reachable from some node, and a
hook with no assigned block lands in "Other observations". Removing the
`leftovers` branch fails the second one.
* [Fix] Reject a model id that is not a single path segment
`model.id` becomes a filename stem: the configurator writes
`<id>.dmi.yaml` beside the model, and `dmi.ui.app` documents that the
browser cannot cause a write outside the directory the user named on the
command line.
Descriptors derived from a framework config are already safe -- `_slug`
reduces `Qwen/Qwen3-8B` to `qwen3-8b`. A hand-written descriptor YAML is a
documented input too, and reached the same code unchecked: `id:
meta-llama/Llama-3-8B` resolved into a non-existent subdirectory (save
fails with a 500), and `id: ../../etc/pwned` resolved outside the base
entirely, quietly breaking the guarantee above.
Validates in `ModelIdentity.__post_init__` rather than at the save site, so
every consumer is covered. `descriptor_from_hf_config` now wraps the
resulting ValueError as `DescriptorError` for consistency with the rest of
that path.
* [Fix] Address deep-review findings across the configurator
Runtime wiring:
- HuggingFaceAdapter.attach_model now accepts and forwards the layers
keyword, so attach_config no longer raises TypeError on the only
shipped concrete adapter. Covered by tests that drive the real HF
adapter instead of a BackendAdapter stub.
- compile_config filters layers with the pure hook_belongs_to_layers
predicate instead of filter_by_layers, which disabled HookPoints on a
live model and raised on unbound authoring-time specs.
Estimation:
- bytes_per_request divides the whole-batch step total by batch_size in
both tensor conventions; batched shapes carry the leading batch
dimension too, so HF-mode figures were batch_size times too high.
- aggregate_prefill_step_bytes renamed to aggregate_peak_step_bytes: it
sums each rank's peak over the enabled phases, which is a decode
figure whenever prefill capture is off.
UI server:
- Relaunching without --config reloads the default save path instead of
starting blank and letting Save clobber the authored file.
- Loopback binds reject non-loopback Host headers, closing the DNS
rebinding path to the file-writing save endpoint.
- The browser opener uses a wall-clock deadline; the old loop counted
iterations while sleeping twice per step, stretching the stated
timeout to roughly double, and its Event was never set.
UI client:
- The decode rate field parses as a float, so 0.5 steps/s no longer
truncates to 0 and silently drops the sustained estimate.
- The Middle layer preset clamps its start, so 1-2 layer models cannot
produce an inverted range the server rejects.
Imports:
- dmi.hooks resolves its re-exports lazily and introspect defers the
model-shape extractor import, keeping torch off the descriptor,
validation, and CLI paths as designed.
Claude-Session: https://claude.ai/code/session_01J4TK3xEGqqjJuum1GRn6Ns
* [Fix] Report the decode step from the rank the estimate names
decode_step_bytes took a maximum across every rank while peak_step_rank and
bytes_per_request came from the rank selected by peak step. Under pipeline
parallelism those are not the same stage -- a first stage carrying many
layers versus a last stage carrying final_logits -- so one result could
describe two different ranks, and the per-request figure could be computed
from a rank the report never named.
All three now come from the worst rank.
* [Fix] Restore estimate request sequencing and let the rate field hold a float
Two fixes that went missing when the concurrent deep-review commit superseded
an overlapping local one.
Estimate responses are applied in order again. The endpoint walks every rank,
so a slow earlier request could resolve after a newer one and paint figures
for a workload the user had already changed away from. Requests are stamped
and stale replies dropped. Verified by delaying the first request behind the
second: the newer figures survive.
The decode-rate field kept `step="1"` while its handler had moved to
parseFloat, so the JS fix could not take effect: a number input with an
integer step treats 0.5 as invalid, `value` reads back empty, parseFloat
returns NaN and the handler bails, leaving the rate at 0 and the sustained
estimate blank. With step="any" the field validates and 0.5 steps/s now
yields 1.00 MiB/s and 84.4 GiB/day.
* [Fix] Address review comments on the follow-up branch
attach_config no longer passes layers= unconditionally. Adapters are a public
extension point, and one that overrides attach_model without the keyword --
as the shipped Hugging Face adapter did until this branch fixed it -- raised
TypeError for every configuration, including the majority that set no layer
range and need nothing from the argument. The keyword is now passed only when
a range is present. A config that does set one still fails loudly on such an
adapter, which is correct: the range would not be honoured, and silently
dropping it is what this wiring exists to prevent.
The estimate endpoint validates the ring block. A truthy non-mapping (a list,
a string, a bare number) reached .get and surfaced as a 500 AttributeError
rather than a 400, and payload_bytes was tested for truthiness, so 0 silently
skipped the ring-fit result the caller asked for instead of being rejected.
The type check now runs on the raw value, before the `or {}` coercion that
was hiding the falsy cases.
The README and plan claimed `make ui` runs "with nothing installed". The
server hard-requires fastapi and uvicorn and raises RuntimeError without
them; only DMI itself is optional. Both now say so.
Tests: the zero-ring case uses 0 rather than -5, which is what actually
exercised the truthiness bug, with the negative case kept separately and the
non-mapping cases parametrized.
* [Docs] Drop a duplicated install line from the configurator plan
The install command now appears once, up front, where it belongs -- the
"installed" variant only needs to show the command that differs.
* [Fix] Stop the estimate promising a sampling reduction nothing delivers
The payload estimate divided its per-request and sustained figures by
step_stride, and applied request_stride as an assumption, but no shipped
adapter enforces CaptureSchedule: should_capture_step and
should_capture_request have no callers outside dmi.config, and nothing in
dmi.adapters reads CompiledDMIConfig.schedule. So the UI quantified a
reduction the capture never performs. Someone setting stride=4 and sizing
storage from the panel under-provisioned by 4x; at stride=1000 the figure was
a thousandth of reality.
This was worse than the plain no-op it replaced. Before the estimator existed
the unenforced schedule was merely invisible; with it, the UI put a confident
wrong number on screen.
The estimate now reports the full volume and says why, and the Capture panel
carries the same kind of disclaimer the System objective control already had
-- that one disclaimed only the policy radios, leaving the stride and warmup
fields looking functional. Prefill and decode toggles are unaffected: they
gate which shapes fire, which the estimator models directly.
Separately, a layer range outside the model resolved to an empty set and
collapsed per-layer hooks to zero bytes with nothing said. Zero reads as
"free" rather than "selects nothing", so an empty selection and a clipped
range now both explain themselves.
When schedule enforcement lands, apply the stride again and drop the warning;
they are commented as a pair so the next reader finds both.
Reproduced first in tests/test_estimate_runtime_fidelity.py: 9 of its 14
tests failed against the old code, the other 5 being negative controls that
passed throughout. One existing test in test_configuration_estimate.py
asserted the buggy division and is corrected in place, with the reason
recorded in its docstring rather than silently rewritten.
Verified in the browser as well: stride 1, 4 and 1000 now all report
20.0 MiB/s and 1.65 TiB/day, the warning names the offending stride, an
out-of-range selection explains its 0 B, and a clipped range reports
"2 of 21 requested layers exist". No console errors.
* [Fix] Drop stale responses in refreshOutput too
refreshOutput fired two requests per edit and rendered whatever resolved,
with no way to tell a late reply from the current one. Under a slow response
the YAML preview and Issues tab could end up describing a configuration the
user had already changed away from. refreshEstimate got this guard earlier;
this is the same fix on the other render path, and a stale return also skips
the trailing refreshEstimate call, since the newer refreshOutput makes it.
Reproduced in a real browser before fixing: with the first request delayed
1400ms behind two later edits, the stride input read 99 while the preview
showed step_stride: 1 -- a response two edits old painting last. After the
fix the same harness reports 99, and a trace confirms the late reply resolves
and renders nothing.
The harness was then shown to discriminate: with the guard removed the same
script on an equally settled page reproduces the stale paint, and with it
restored it passes. That check mattered, because an earlier "green" run of
mine was invalid -- it executed against the pre-reload page, so it exercised
the unfixed code and reported a failure that was an artifact of the harness
rather than the fix.
tests/test_ui_client_invariants.py adds committed guards for both render
paths. They assert the mechanism is present rather than that it works, which
is weaker than a unit test and deliberately so: the plan gates the static UI
on "no build process" and rules out a Node toolchain, so there is no JS
runner in CI to execute app.js against. The file says as much, and says to
replace them if a runner ever lands. A coverage test derives which paths need
a guard from whether they can overlap -- scheduled or called directly -- so
boot is exempt because it is only ever registered on DOMContentLoaded, and a
new fetch-and-render path cannot be added unguarded without failing.
---------
Co-authored-by: Claude <noreply@anthropic.com>
* [Fix] Address Copilot review: strict section parsing, save error translation, doc snippet
* Close all 14 review findings on the configurator
Blocking:
- Non-loopback binds now mint a per-launch token demanded on every mutating
endpoint (the file-writing API is no longer open to the network).
- Save refreshes the server's reload state, so a refresh restores what was
saved and a second save cannot overwrite it with the launch config.
- Save runs model-aware validation before the write; invalid configurations
get a 400 naming the issues instead of a successful persist.
- The capture schedule is enforced: the adapter driver consults
should_capture_step/should_capture_request before every step (StepContext
carries the phase; HF reports it), so capture_decode=false actually stops
decode capture and strides thin the capture.
- compile_config refuses requested hook types absent from the model's spec
list, so "valid" means executable (pos_embed/mlp_post on a Llama can no
longer compile to an empty selection).
- attach_config diagnoses an adapter that cannot accept `layers` with a
ConfigurationError naming the remedy (pinned vLLM integration update is
a separate-repo follow-up).
Major:
- YAML boundary is type-strict: exact ints (no 2.9-truncation, no bools),
real booleans (no "false" strings), unknown keys in layers/policy refused.
- Descriptor topology requires exact positive integers; head_dim is
validated; num_layers 1.5 is refused at the boundary instead of exploding
in range().
- Introspect refuses known non-decoder model families (BERT, ViT, T5, ...)
instead of labeling every non-encoder-decoder config decoder_transformer.
- The frontend preserves policy absence when loading policy-less YAML.
- Copy serializes the current state instead of the possibly-stale preview.
- Estimator volume figures sum every rank (TP/PP splitting no longer deflates
bytes_per_request); peak stays per-rank for ring sizing.
- Estimator dtype accounting matches the runtime: packed token/topk ids are
int32, topk weights float32; batched token ids int64.
- Workload gains cache_max_len so StaticCache prefill attention can be
sized; without it the estimate says which cache it assumes.
CPU suite: 1373 passed.
* [Fix] Skip API suite without an HTTP transport; flag layer ranges on Packed/vLLM
Copilot's third pass on PR 122 noted that fastapi.testclient needs httpx,
which the [ui] extra does not carry. With Starlette 1.6 the import raises
RuntimeError (it wants httpx2 now) at collection, so the whole API contract
suite errored instead of skipping like every other optional-dependency suite.
Both TestClient-using modules now skip unless httpx2 or httpx is importable;
the transport stays out of [ui] on purpose (the UI never makes HTTP requests)
and pyproject says so. A child-process test pins the skip-not-error behaviour.
Review finding 3919326699: the pinned vLLM integration's attach_model has no
`layers` keyword, so attach_config refuses a ranged configuration on that
backend at launch. The estimator -- where the UI learns which backend is
targeted -- now warns about this on Packed (vLLM) with a layer range, and the
attach_config error links the tracking issue
(ProjectDMX/DMI-vLLM-Integration#20).
CPU suite: 1372 passed; the 5 remaining failures are the /proc/self/fd fsync
tests, which are Linux-only and identical to main.
* Fix the schedule-gate hole the review found, plus three minor findings
The high-severity one: refusing a step in before_forward only skipped
plan/commit -- the model's HookPoints still dispatched producers during that
step's forward, writing unreserved bytes into the ring and desyncing the
task/meta FIFO so later captured records were misattributed or dropped. The
driver now disarms the transport (capture_step=False) before refusing and
re-arms it on a committed step; HookPoint.forward checks the flag first and
returns without dispatching. CPU-level test drives a real HookPoint through
both flag states with the producer call recorded.
- Sustained rate and per-day volume now divide by request_stride as well
(the driver's request gate drops whole requests, matching per_request).
- The estimator states its enforcement assumption when strides or phase
flags are configured: an adapter reporting no phase / non-numeric request
ids applies only part of the schedule.
- Architecture refusal matches real HF spellings: "BertForMaskedLM"
(no underscores) now trips the masked-LM check; unknown-key TypeError
guard for head_dim covers bools exactly.
- Nits: constant-time token comparison in the UI middleware, isdecimal for
scheduler-id gating, and the never-fires compiler check now runs after
selection (shape suppression) but before layer filtering so its message
cannot blame the layer range for an absent hook.
CPU suite: 1377 passed.
* Land the schedule-gate producer fix that a stash had swept aside
Companion to 30f0e8a, which committed only four of the six files before a
concurrent stash took the rest of the working tree. This carries the core of
the fix: the transport gains a capture_step flag that the driver disarms
before refusing a schedule-refused step and re-arms on a committed one, and
HookPoint.forward checks it before dispatching -- so refused steps no longer
launch unreserved producers or desync the task/meta FIFO. Also lands here:
request_stride in the sustained rate, the estimator's enforcement
assumption, the BertForMaskedLM spelling fix, and the regression tests for
all of it.
CPU suite: 1385 passed.
* Export the layer-range API through the v1 integration facade
DMI-vLLM-Integration#21 accepts attach_model(..., layers=LayerSelection(...)),
and it imports everything through dmi.api.v1 — which did not carry the
layer-range pieces. filter_by_layers, hook_belongs_to_layers, and
LayerSelection are now facade exports alongside the rest of the selection
API, the exactness test and integration-api-v1.md cover them, and a pin
test locks the three to their real implementations.
The submodule pin bump and the estimator's vLLM layer-range warning
removal land together after DMI-vLLM-Integration#21 merges.
* Close the independent-review findings: HTTP hardening, atomic save, ring-fit task cap, gate disclosures
Eight independent reviewers audited this PR; every confirmed finding is
addressed here.
HTTP boundary (was raw 500s on malformed input):
- /api/config/parse rejects a non-string yaml value with 400.
- /api/estimate: Workload requires exact ints/finite floats (float 8192.0
exploded as TypeError in the byte arithmetic; bools read as 1), the ring
block requires integer payload/pinned bytes and OverflowError is caught,
and pinned_bytes without payload_bytes is a 400 instead of a silent
no-op. Non-ASCII X-DMI-Token headers compare as bytes (was TypeError ->
500) and /api/config is token-gated on network binds (it names a server
path).
- The loopback bind refuses mutating requests carrying a FOREIGN Origin:
TrustedHostMiddleware stops DNS rebinding, not plain CSRF -- a web page
can send Host: 127.0.0.1 legitimately, and sendBeacon can send
application/json without a preflight. Foreign Origin is the one signal
a browser always gives.
Data safety:
- save_config/save_descriptor write to a temp file and os.replace: a
mid-write ENOSPC used to destroy the previous configuration.
- The descriptor parser now matches the config parser's strictness:
exact-integer schema_version (true/1.0 are refused), unknown keys
refused in the model section and at the root, a bad model.id raises
DescriptorError instead of bare ValueError, and num_experts without
top_k is refused (the routing observations could never fire).
Estimator fidelity:
- check_ring_fit gains task_entries: prepare_step refuses on the task
ring too, so a wide full-preset model "fit" on bytes while every real
step returned OVERSIZED. /api/estimate accepts ring.task_entries and
names the actual breach in the detail text.
- The PP layer split now matches vLLM's get_pp_indices (remainder on the
FIRST stages), so the peak rank is the rank the runtime actually
pressures on non-divisible layer counts.
- Batched final_logits counts logits_to_keep=1 (what HF generate()
materializes) instead of every prefill row -- the old number was
~1000x the reality and set the ring-fit verdict.
Schedule gate:
- The no-phase tail (warmup/stride for adapters that report no phase) is
now pinned by tests, the docstring documents it and the request-gate
units, and an unrecognized phase value captures-as-unreported instead
of crashing the driver and silently disarming all later steps.
- catalog_adapter marks final_logits unavailable when vocab_size is 0
(compute_hook_shape returns [] for it; validation/estimate/compiler all
certified it before).
Frontend:
- Input handlers guard on an initialized flag: listeners bind before the
model fetch resolves, and a keystroke in that window crashed on null
state (and a mid-applyState crash discarded a loaded file).
- Copy carries a stale-response stamp like the other async paths.
- Tabs expose aria-selected and panels are role=tabpanel.
Docs: capture-policy-review's "schedule enforced by no shipped adapter"
finding is marked resolved (it landed in this PR after the doc was
written); the PR body no longer claims the change is additive-only nor
lists #121 as an open follow-up; the pyproject httpx comment matches
reality (it is a core dependency).
CPU suite: 1414 passed.
* Restore the clickhouse-live job the branch accidentally deleted
c00afa4, while adding pyyaml/fastapi to the CPU job's install, removed the
entire clickhouse-live job (its services, its fetch-depth: 0, its skip
gate) from python-checks.yml — so the manual ClickHouse suites this branch
carries ran on nobody's CI again, which is the exact regression that job
was created to prevent. The failure only surfaced once the configuration
pipeline started importing yaml at module level: the live job's install
never had pyyaml, and collection of the new suites died with
ModuleNotFoundError.
The job is restored from main verbatim, with pyyaml and the [ui] deps
added to its install so the configurator suites collect and run there too.
Verified locally against the CI selection: 30 collected, 0 skipped.
* Close the Samfisheryu review: one runtime-apply path, honest packed estimates, fail-closed detection
Every finding was reproduced as a test before any source changed
(tests/test_pr122_round3_review_findings.py). The review was written
against c19eba5; 3dc68ca had already closed three of the fifteen, and
those stay in the suite as regression pins rather than being dropped.
Blockers:
- attach_config now installs the configuration's CaptureSchedule on
adapter.engine.config, which is the only place _schedule_allows reads
it. Before, the documented path (load_config -> attach_config) applied
WHERE to capture and silently dropped WHEN: a YAML declaring
capture_prefill: false or step_stride: 17 attached hooks and captured
everything. An adapter with no engine still attaches under the default
schedule and raises for one that asks for anything, matching the rule
the `layers` keyword already follows.
- Attachment has one owner. BackendAdapter.attach_model records the
owning adapter on the model and HuggingFaceAdapter.detach_model
releases only its own marker; generate_with_monitoring (and the greedy
loop, same hazard) refuses a model somebody else attached instead of
building a second adapter over the shared transport -- which left the
forward with one caller's reservation and this call's producers, then
detached the first attachment on the way out.
- Packed estimates no longer divide by step_stride/request_stride. The
pinned vLLM integrations build MonitoringEngine(config=None) and call
commit_step directly, so no schedule reaches the gate and production
captures every step; dividing reported a reduction the shipped runtime
does not perform, on the UI's default convention. The figures now say
so. Batched (Hugging Face) still divides -- that driver does gate.
Correctness and honesty:
- compile_config refuses a layer range that removes every spec of a
selected hook, naming the live layer span. A stale descriptor claiming
32 layers against a model exposing 16 used to compile layers 20-25 to
an empty spec list that installed nothing and reported nothing.
- Architecture detection fails closed: an allowlist of causal decoder
families replaces the non-decoder blacklist, which could only refuse
what someone had thought to list (dinov2, modernbert and
bert-generation all escaped it and were emitted as
decoder_transformer). A config with no model_type is refused too --
geometry that parses says nothing about causality. Hand-written
descriptors remain the escape hatch and skip detection entirely.
- /api/validate and /api/config/save label their verdict design-time and
name the descriptor as its authority; the UI's "ready to run" wording
follows. Validation cannot certify runtime-readiness from a portable
descriptor: a decoder Llama exposes no pos_embed spec however the
descriptor marks it.
- A strided estimate is disclosed as a long-run average bounded by the
peak step, and offsets/warmups warn that they shift which steps are
captured rather than being silently absent from the arithmetic.
Boundaries:
- Saves serialize the write and the state update under one lock, so a
200 names one committed revision. Racing saves used to leave the file
and the served config disagreeing until restart.
- /api/config/parse and load_config share a loader that rejects
duplicate mapping keys: PyYAML's last-wins would let a merge conflict
change capture scope in silence.
- observations.hooks is a typed list[str] -- [q, 1, true, null] no
longer becomes ["q","1","True","None"].
- Workload requires a real bool for packed ("false" was truthy and read
as the vLLM convention) and a non-bool number for the decode rate.
- An explicit --port outside 1-65535 is refused before the descriptor is
read; port 0 used to advertise a URL naming no bound port.
Two findings are answered with disclosure rather than the reviewer's
first option, both stated in the estimate output: VLLM_PP_LAYER_PARTITION
on the serving host overrides the partition this estimator cannot see,
and the per-request figure is exposed as a long-run average instead of
evaluating the predicates for a stated request window.
Existing tests that pinned stride division on packed workloads were
split by convention rather than deleted, and the packed counter-case is
pinned alongside them.
CPU suite: 1473 passed. The 5 remaining failures are pre-existing and
Linux-only (capture-storage fsync tests read /proc/self/fd), verified
failing identically on a pristine 3dc68ca.
* Close the remaining review findings: compile disclosure, CLI error narrowing, selection taxonomy
- A non-default capture schedule now disables CUDA-graph compilation for
the generate() call, mirroring the capacity-overflow path: the gate is a
Python branch in HookPoint.forward and replay bakes it at the first
decode trace, so a refusal (or capture) at step one would apply to every
later step. Default schedules are unaffected. Pinned by driving
generate_with_monitoring with a stubbed adapter and inspecting the
kwargs the model receives.
- cli.main no longer catches bare RuntimeError -- that class carries
genuine bugs (RecursionError is one) and swallowed them into one stderr
line. The optional-dependency failure gets its own UIDependencyError;
internal raise sites updated.
- compile_config re-raises an unknown hook selection as
ConfigurationError instead of bare ValueError, so callers funneling on
the taxonomy (the UI's 400 mapping) see a 400, not a 500.
CPU suite: 1588 passed (after the main merge).
* Close the round-4 follow-up findings: packed honesty, PP rule, ownership, boundaries
Samfisheryu's follow-up review kept CHANGES_REQUESTED on eight residuals;
all are fixed here with tests, RED first.
Ownership:
- attach_config refuses a second adapter on an already-owned model BEFORE
any mutation (no schedule install, no attach call), naming the current
owner; same-adapter reconfiguration is allowed; detach by a non-owner
leaves the marker (HF adapter path pinned).
Packed honesty (the vLLM runtime executes no schedule):
- capture_prefill/capture_decode no longer shrink packed volume, peak, or
sustained figures -- the runtime captures both phases regardless. The
authored intent stays visible as an explicit warning; batched figures
are unchanged.
- vLLM PP partition rule ported from 0.27.1's get_pp_indices (remainder
backward from the second-to-last stage, never the last; cited cases
[1,2,1], [2,3,3,2], 32/40 verified) plus an explicit, validated
pp_layer_counts override for VLLM_PP_LAYER_PARTITION deployments.
- Offset/warmup disclosure no longer depends on a stride being set.
Boundaries:
- Ring: only absent/None defaults pinned_bytes; every explicit value is
validated (exact non-bool int, >= 0), and check_ring_fit refuses
negatives itself.
- Workload.cache_max_len requires an exact integer.
- YAML: unhashable mapping keys raise ConstructorError (a 400), and the
descriptor file path uses the same strict loader as configs.
- Introspect: any model_type containing "encoder" fails closed instead of
riding a decoder family prefix (qwen2_audio_encoder); decoder+head
configs still accepted.
CPU suite: 1679 passed.
* Disclose the two known ceilings the follow-up review named
- The per-request token-range index records every driven step including
schedule-refused ones; it is the attempted-traffic index, not the
captured-traffic index.
- The _accepts_layers probe cannot distinguish forwarding **kwargs from
tolerating them; the backstop is the v1 contract plus compile-time
live-hook rejection, stated where the probe lives.
* Add Workload.sliding_window for sliding-window attention estimates
Attention-weight hooks (pattern, attn_scores) attend at most window KV
positions on sliding-window models, so they are shaped against
min(context, window) instead of the full context. Only kv_dim is
affected, and only those shapes consume it. Exact-int >= 1 validation,
assumption text when set, no-op when absent or above context.
FP8 KV-cache quantization was investigated and deliberately not modeled:
hooks capture forward activations in model dtype, so cache-store
quantization changes zero hook payload bytes.
* Revert "Add Workload.sliding_window for sliding-window attention estimates"
The knob's core semantic was incorrect in the unsafe direction:
compute_hook_shape returns full [heads, q_len, kv_dim] for
pattern/attn_scores with no window input, plan_step reserves exactly
that, and these hooks fire meaningfully only in eager mode -- where SDPA
materializes the full scores matrix and masks it rather than shrinking
it. Capping kv_dim by the window made the estimate report less than the
runtime reserves (up to context/window under-count), and check_ring_fit
would bless a ring the runtime overflows. The FP8 non-modeling analysis
in the original message stands; the window half is withdrawn until a
backend materializes band-shaped scores.
* Frontend review round: guards, keyboard paths, live regions, curl-only banner
- Save disables itself for the request round trip (re-armed in finally).
- Layer rail ticks are keyboard-operable checkboxes sharing one picking
function with the click path; architecture nodes already had this.
- Estimate panel marks itself loading with aria-busy, cleared on either
guarded outcome.
- Status chip is a polite live region; ring meter exposes value semantics.
- Token-gated binds get a persistent curl-only banner revealed on 401
instead of per-panel curl-hint errors.
* Simplify the config / descriptor write helpers
- _reject_unknown gains an error= kwarg so the config half (ConfigurationError)
and the descriptor half (DescriptorError) share one body. As a bonus
this stops the int-YAML-key AttributeError the descriptor half had -- the
manifest copy already used repr() but the config half still did not.
- save_config and save_descriptor share _write_text_atomic(path, text, what,
error): the only delta is which exception class wraps an OSError.
CPU suite: 1685 passed.
* Record the configurability boundary on PR #122
The review round surfaced three rules that deserve a written rationale
rather than a thread of replies: the configuration artifact is strict
end-to-end; the runtime surface is enforced at the smallest possible
layer (the adapter driver's before_forward) rather than scattered
across every emit point; and the HTTP surface trusts the loopback bind
but refuses the network without a per-launch X-DMI-Token. The
one-owner attach invariant is the same rule applied to a model.
The alternatives weighed (stricter loopback defaults, TLS/OAuth on the
local tool, rejecting packed configs that name a schedule, hooking the
gate at every emit point, an inline JSON or frontmatter idiom) all
get a sentence here so the next round does not relitigate them.
* Finish the shrink round: UI half and matching tests
The yaml.py/manifest.py simplification shipped first without its test
updates, which broke CI: the old atomic-save test raised ENOSPC from
dump_config, and the new save_config evaluates dump_config before
_write_text_atomic's try-block, so the OSError escaped unwrapped. The
test now patches Path.write_text on the .tmp sibling instead, which is
the real I/O boundary the atomic write protects.
Rest of the round:
- UI_DEPENDENCY_MESSAGE moves to errors.py beside UIDependencyError, so
app.py and server.py quote one string.
- The auth banner reuses .notice plus one margin-top override instead
of duplicating seven properties.
- Copy calls a shared serializeCurrentState() rather than reissuing the
/api/config/serialize POST shape refreshOutput already builds.
- The two test-side brace matchers (_function_body /
_plain_function_body) collapse into one with an optional async prefix.
CPU suite: 1685 passed.
* Pin third_party/vllm-integration to main (layer-range attach)
DMI-vLLM-Integration PR #21 merged (squash 23717cb): attach_model
accepts layers=LayerSelection(...), applies it to the local specs and
the model-wide candidate-rank sets, and stays importable on DMI builds
without the layer-range facade. The pin moves old-main 29f26c3 ->
23717cb, picking up both the V2-runner promotion (#19) and the layer
range (#21).
* Drop the packed-backend layer-range refusal warning
The warning told the UI that attach_config refuses a ranged
configuration on Packed (vLLM) until the integration accepted ranges --
that is PR #21, now merged and pinned. The estimator test flips to pin
the absence of the warning (RED against the stale block), and
attach_config's fail-closed guard stays for genuinely older
integration checkouts, with its comment updated to name them.
CPU suite: 1689 passed.
* Document the live verification of the configurator runtime attach on vLLM
The recipe for proving a saved .dmi.yaml artifact drives a real vLLM
0.27.1 model through dmi.configuration.attach_config existed only in
the verification session: build the CI-style env (vllm 0.27.1 + DMI
--no-deps + integration --no-deps), subclass the V1 DMXGPUWorker to
suppress the self-attach (V2 refuses subclasses), run the engine core
in-process, and assert ownership, ranged spec layers, disabled
out-of-range hook points, and a completed generate. Also correct the
install step for shared checkouts: the native build emits ABI-suffixed
extensions per interpreter, so 'make clean' is destructive there.
* Add hf extra for transformers-backed model resolution
dmi ui/describe-model needed transformers installed by hand to resolve a
model directory or bare HF model id; pip install -e ".[hf]" makes that
reproducible, and the missing-transformers error now names it.
* Mention the hf extra in the README configurator section
Keep the top-level entry point consistent with the configurator plan doc.
* fix: count skipped metadata steps in capture schedules
* Drop the .loop scratch state from the tree
The /polish loop's working files are per-run agent bookkeeping, not project
artifacts: nothing in the tree, the build, or CI reads them, and they go
stale once the branch they describe has landed. #133 removes them from
main; dropping them here too so a merge of this branch cannot carry them
back in.
---------
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Alan Liu <zaoxing@users.noreply.github.com>
Co-authored-by: Samfisheryu <ynnpersonal@gmail.com>
zaoxing
added a commit
that referenced
this pull request
Sep 22, 2026
…#132) * Pin the record encoder's cell and payload-slice refusals Findings T2 (records.py:531 `_validate_cell`) and T3 (records.py:444-476 `_validate_payload_slices`) from the src/dmi polish round: both guards were dead to the CPU gate. A verifier put `return` at the head of each method and the whole suite stayed green, and a line trace confirmed 455/460/464/471/476 and 494-497/512-518/520-525 never executed. Every refusal test also asserts `transport.events == []`, which is the load-bearing part. The native side does re-check INT32 range (`CopyLiteralRecordValue` in native/csrc/bindings.cpp), but only at `push_record_descriptors` -- AFTER `reserve_record` -- so a late native throw leaves an unmatched ring reservation. What these tests protect is that the Python guard exists and fires FIRST. The residual the native check does not cover at all: `True` and `3.7` are silently coerced by `py::cast<int64_t>`, and a `list` is accepted as INT64_ARRAY. Two positive controls are deliberate, not filler: the one-open-plus-bounded slice case is what executes line 471's bounded arithmetic (it alone kills a `> 1` -> `> 2` mutant), and the exact-bound case pins the boundary as inclusive (it alone kills `>` -> `>=`). Mutation-checked against throwaway copies of src: guard -> `return` fails exactly the 4 T3 tests and exactly the 6 T2 tests; `> 1` -> `> 2` fails 1; `>` -> `>=` fails 2. No src file changed. * Cover the hook selection and PP/TP rank filters Finding T1: `select_hook_specs`, `apply_hook_selection`, `hook_belongs_to_pp_rank`, `hook_belongs_to_tp_rank`, `filter_by_pp_rank` and `filter_by_tp_rank` (hooks/selection.py:113-228) had no behavioral test at all. A line tracer over the full `-m cpu` run showed only their `def` lines execute; `select_hook_specs -> return []`, which disables every hook on every model, left the suite green. Not dead code: adapters/base.py:168-170 calls all three on every `attach_model`. Not covered elsewhere either -- the vLLM tests that do assert on this surface live under third_party/, which pyproject.toml's `norecursedirs` excludes, so they never run in this repo. The `and spec.hook_type not in unavailable` clause is load-bearing rather than defensive: model_shape.py:55 reads `getattr(cfg, "num_experts", 0)`, which is 0 for Mixtral-style configs that spell it `num_local_experts`, and `intermediate_dim` falls back to 0 for any non-gpt2 config without `intermediate_size`/`ffn_dim`. The stub hook point initialises `enabled` to a sentinel rather than True, so "left untouched" is provable instead of coincidental. Mutation-checked: 14 mutants, all killed -- including both always-True and always-False variants of each rank predicate, `filter_by_pp_rank -> return specs` and `-> return []`, dropping `enabled = False`, dropping the warning, and each of the three RuntimeErrors. No src file changed. * Pin the publisher lease's quarantine-on-unknown-outcome Finding T6: the `except BaseException -> _quarantine_locked()` handlers of `acquire_publisher_lease` and `renew_publisher_lease` (clickhouse_catalog.py:686-702) were untested. Reducing both to a bare `return self._leases.acquire(holder)` / `.renew()` left the CPU suite at its baseline. The existing lease-failure tests all raise taxonomy errors, which take the `except (CaptureStorageError, ValueError): raise` arm instead, and the covered quarantine at line 469 is the watermark-INSERT path. What the gap allows, measured: with the handlers removed, a writer whose term-2 claim row LANDED but whose connection dropped before the outcome was known keeps its stale term-1 identity and publishes -- `watermarks = [1]`. That is the split-brain publish the quarantine exists to prevent. The renewal test drives `renew_publisher_lease()` directly and says why in its docstring: `_serial` is an RLock, so the renew inside `publish_snapshot` is re-entrant within that method's own `except BaseException` at 468-470, and routing through `publish_snapshot` lets the outer handler mask the inner one being tested. Third test is a negative control -- a taxonomy failure must NOT quarantine -- so an over-broad "quarantine on everything" fix cannot pass this file. Mutation-checked: the handler removal fails both quarantine tests at `assert writer.publisher_lease is None`; dropping the taxonomy arm fails the control. Pure fakes, no server, runs under -m cpu. No src file changed. * Pin the reference capture sink's payload and durability guards Finding T5: the sink's CPU-tensor, contiguity, metadata-dtype and metadata-shape guards (record_adapter.py:261-268) plus the `persisted != target` durability check (295-298) were all unexercised -- four guards, not the two originally reported. Deleting 261-268 wholesale, or replacing the durability raise with `pass`, left the CPU suite green; only reducing `_validate_snapshot` to `return None` failed anything, which is `_validate_losses` being covered while the accounting line is not. Why the guards exist, since a reference-format producer cannot trip them: `CaptureRecordFormat.encode` builds both cells from one `CaptureMetadata` and already raises on dtype drift. These protect against a FOREIGN producer writing the same layout name, or an encoder bug. Nothing else in the stack cross-checks -- native/csrc/reference_python_capture_sink.cpp validates only slice-internal consistency and never compares the metadata JSON to the slice. Telling detail: the only existing proof this cross-check matters lives in a test-only stand-in target in the gpu e2e suite, never in _CapturePackTarget. The durability patch is applied after `_attach()`, not after construction: `_attach` re-reads `snapshot()` against its baseline, so an earlier patch trips "must remain empty before attachment" instead of reaching line 295. Mutation-checked by rebinding the guards at runtime: all 5 new tests fail, the 6 pre-existing ones stay green -- independently reproducing the finding. No src file changed. * Cover the IDENTITY, PREFIX_STRIP and CHUNKED CPU-direct fallbacks Finding T7: `RingTransport._record_cpu_tensor`'s three remaining transport branches (transport/ring.py:422-443) never executed; this file covered only SEQ_PREFIX_PACK and SEGMENTED_PACK. An unconditional `raise AssertionError` at the head of all three left the full suite green. This is the fallback that must byte-match the CUDA producer, so the clamps are contractual, not defensive: producer.cu's `record_producer_prefix_kernel` does `if (rows < 0) rows = 0;` and `record_producer_chunked_kernel` does `if (bytes < 0) bytes = 0; if (bytes > input_chunk) bytes = input_chunk;`. Without the CHUNKED clamp, `counts=[6,2]` on an 8-byte 2-chunk source yields [0,1,2,3,4,5,4,5] -- chunk 1's bytes duplicated into the record. Deliberately NOT covered: the `min(byte_source.numel(), ...)` in PREFIX_STRIP. A verifier showed it is an equivalent mutant -- Python slicing already clamps, so removing it is byte-identical for every input, and the surviving `max(0, ...)` carries the semantics. A test claiming to pin it would assert nothing, which is the fake coverage this round exists to remove. The -1 parametrization covers `max(0, ...)` instead. `_entry()`'s signature is generalized to `**spec_args` because TransportSpec.__post_init__ rejects `feature_bytes` for these three types; both pre-existing tests call it by keyword and still pass. Mutation-checked: the three-branch AssertionError now fails 8 cases (was green); removing both clamps fails the -1 and [6,2] cases. No src changed. * Pin the batched FINAL_LOGITS cap and the degenerate-config shapes Finding T8: hooks/specs.py:280 never executed, so no CPU test computed a batched FINAL_LOGITS shape at all -- not merely the over-cap case. The degenerate returns at 256/259/261/263 and the unknown-type return at 284 were equally untouched; setting `logits_q = 999999` and changing all five `return []` sites to `return [1234]` both left the suite green. The cap is reachable, not theoretical: adapters/huggingface/adapter.py:354 does `int(model_inputs.get("logits_to_keep", 0))` with no bound against q_len, and it reaches `compute_hook_shape` unmodified via the public `logits_to_keep` kwarg. Without `min(q_len, logits_to_keep)` the meta declares a larger shape than the tensor actually pushed, and the drain thread expects more bytes than the ring holds. The MoE positives are marked plain `cpu` on purpose: the only existing tests covering them are in test_moe_v1_routing_hooks.py under `pytest.mark.framework_fork`, which skips without the Transformers fork -- so those shapes had no coverage in the gate. Why the neighbouring tests missed this: test_tp_shapes.py:131 drives only the packed path (batch=0) and asserts just `shape[-1]`, and test_integration_api_v1.py:275 compares `v1.compute_hook_shape` against `specs.compute_hook_shape` -- the same object, so it can never catch a body mutation. `_VOCAB` is read from the `_cfg()` helper rather than hardcoded. Mutation-checked: the combined mutant fails exactly the 6 covering tests; dropping the `min()` fails the cap test. No src file changed. * Make the record-runtime switch a tested transaction Finding T9: `create_record_runtime`'s double-activation guard (engine.py:271-272) and its entire `except BaseException` rollback (341-361) never executed. Deleting the guard and sabotaging the except body both left the suite green; the two existing failure tests raise BEFORE the `try` at line 315. The original finding's scenario was wrong, and the third test is why. On the `create_record`/`init`/`start` paths the state reset is pure redundancy -- lines 323-324 already null both fields before `switched = True`, and `_record_mode = True` is only reached at 338 after `start()` returns, so `_record_mode=True` with `_ring_transport=None` is unreachable there. Only `_rt.activate()` raising reaches the handler with all three fields naming the new record ring, and it is also the only path that shows the second `deactivate()`; the naive `assert deactivated == [True]` fails there. Mutation table, each against a throwaway copy of src: delete the guard (271-272) -> 1 failed (test 1) sabotage `except BaseException` -> 4 failed (tests 2a-c, 3) delete the state reset (353-356) -> 1 failed (test 3 ONLY) delete `record_engine.stop()` -> 3 failed delete rollback `deactivate()` -> 1 failed (test 3 ONLY) The two rows marked ONLY are the point: without the activate-failure case both mutants survive, and the coverage would have proved nothing about the reset. No src file changed. * Pin the footer cache's byte-budget eviction Finding T10: `CaptureReader`'s byte-budget eviction disjunct (reader.py:348-351) was untested -- the file covered count-based eviction and oversized-entry rejection only. Deleting `or self._footer_cache_bytes + entry.wire_bytes > self._footer_cache_bytes_limit` left the CPU suite at baseline, while deleting the whole eviction body or the admission guard each failed an existing test, localizing the gap to that disjunct. The branch is live, not dead code behind the oversized-entry guard: that guard rejects only an entry larger than the ENTIRE budget, whereas this is cumulative overflow across several individually-admissible entries. With the shipped defaults (128 packs / 64 MiB) it fires once mean footer size passes roughly 512 KiB, reachable for packs near `max_pack_records = 10_000`. The test asserts `_footer_cache_limit >= 3` BEFORE lowering the byte limit, so count eviction cannot be what it observes -- otherwise it would silently re-test the neighbouring case. Footer size comes from the existing `_footer_read_bytes(sealed)` helper rather than a hardcoded constant. Mutation-checked: the disjunct's removal fails it with `assert 3 == 2`. No src file changed. * Drive plan_step and attach_model from the CPU gate Findings from round 2: `BackendAdapter.plan_step` (base.py:259) and `attach_model` (base.py:167) were never entered ONCE by the 1508-test gate. A verifier proved it with the strongest possible probe — an unconditional `raise` at the head of each function survives the whole suite. `plan_step` was invisible because the suite's only adapter stub (tests/test_adapter_protocol.py:111) overrides it, and that override turns out to be incidental: it exists to record call order, not because anything forces it. `PlanningAdapter` here deliberately overrides neither, which is the point of the file. `attach_model` had no call site in tests/ at all -- only a mention in a docstring. Two tests carry most of the weight: test_plan_step_rounds_each_hook_up_to_16_bytes_independently -- three 6-byte hooks must total 48, NOT 18. Alignment is per-hook rather than per-sum, and this is the only case that kills `align_up_py(nbytes, 16)` -> `nbytes`. Note 48 is also not align_up(18)=32. test_attach_model_runs_selection_then_pp_then_tp_filters_and_installs -- at tp_rank=1, is_pp_first=False the survivors must be exactly {Q, MLP_POST, PATTERN}. round 1 pinned each filter in isolation; nothing pinned the three-call WIRING, so deleting a call from the chain was free. The stub hook point starts `enabled` and `_ring_payload` at sentinel strings rather than True/None, so "left alone" is distinguishable from "installed" and the not-installed assertions cannot pass vacuously. 12 mutants killed, re-verified after the file was restyled: both unconditional raises, the alignment mutant, deleting each of the three chain calls, dropping the empty-shape skip, ignoring actual_q_len, ignoring the per-spec dtype override, unconditional needs_eager, skipping install_ring_hooks, and publishing unfiltered specs. No src file changed. * Pin the device gate, replay arity, and producer drift refusals Three round-2 findings in records.py, each mutation-confirmed against the 1508 baseline and each killed by exactly one of these tests: :308 -- no test exercised a SUCCESSFUL device-gated bind_hook. The negative branch was pinned (`if True:` is caught); the accepted-gate branch was not. The reserved flag is RecordReservationItem.needs_reclaim, and only when true does reserve_record register a pending task reclaim (drain_thread.cpp:278-281); the gated IDENTITY kernel returns before copying AND before publishing when the gate blocks (producer.cu:429-430), so this flag is the ring's only handling of that case. The test asserts ("reserve", ((16, True),)) against the ungated (16, False) already pinned at line 155. :347 -- prepare_replay's metadata/plan arity guard never fired. Without it the body publishes 2 descriptors while _reservation_items reserves 3, breaking push_record_descriptors' documented ordering contract (transport/ring.py:391). :433,:438 -- _validate_entry_output's dtype and input-shape drift refusals never fired. Realistic input, since a captured plan's entry outlives the output it was derived from. Note the existing test_payload_slice_dtype_drift_is_refused pins a DIFFERENT guard (:463) and does not reach these. A verifier correction is recorded here because it changes what the first test means: the original finding said the missing flag lets a record carry stale payload. It is one step off -- an unpublished gated task with needs_reclaim=True leaves an unresolved pending reclaim, which ring_engine_py.cu:511-514 reports as "record flush found incomplete producer reclaims". The untested property is the safety flag itself. Every refusal test asserts `transport.events == []`, pinning refusal before reservation. Kill checks were re-derived independently by monkeypatch simulation rather than taken on trust. No src file changed. * Pin resolve_hook_selection's misconfiguration refusals Round-2 finding, selection.py:101-107: the unknown-token and empty-result refusals were unexercised, though the path is on the attach line (adapters/base.py:168 -> selection.py:151 -> :118) and the only existing reference was a success assertion at tests/test_adapter_protocol.py:385. The mutant that makes the stakes clear is stronger than the one first filed: `_HOOK_SELECTIONS.get(token, _ALL_HOOK_TYPES)`, i.e. a typo'd `hook_selection=` silently selects EVERY hook instead of raising. The suite stayed green. The test asserts the message names the offending token AND lists the available names -- two assertions beyond the obvious, because `"Available:" in message` alone survives a mutant that emits the label with an empty list. It also pins propagation through select_hook_specs, so the attach-path caller is covered rather than just the leaf. Seven mutants killed, each only by this test. No src file changed. * Pin hook_row_basis's row-scaling contract Round-2 finding, specs.py:121-127. The original filing understated it: replacing the return value survives, but so does putting `raise AssertionError` in the BODY -- so this public v1 API is never CALLED by the CPU gate at all, not merely under-asserted. The only existing reference is the facade-identity assert at tests/test_integration_api_v1.py:175, which compares the same object to itself and therefore can never catch a body change. Its only non-test callers are in the third_party vLLM adapter, which `norecursedirs` excludes from collection. Pins the docstring's contract -- "Logit-shaped payloads are request-scaled; every other registered shape class is token-scaled" -- plus the unknown-type refusal. No src file changed. * Cover install_ring_hooks' binding and unbound-spec refusal Round-2 finding, dispatch.py:41-49: no CPU test invoked install_ring_hooks. The strongest possible mutation proves it -- `for spec in specs:` -> `for spec in []:`, making the function a total no-op, left the gate at 1508 passed. The only reference in the tree was the facade-identity line at tests/test_integration_api_v1.py:176. It needs no CUDA, which is why the gap was closable: install_ring_hooks only assigns three attributes on `spec.module` and never touches torch.ops.ring (that is dispatch_producer, a different function), so a plain nn.Identity() drives it. `_ring_hook_id` carries spec.layer_no, and per-layer reassembly buckets on that field (storage/internals.py:_reassemble_per_layer, key index 3) -- so the stuck-id mutant would collapse every layer into one. That is why the test asserts the ids are [0, 1, 2] rather than merely present. Four mutants killed: the no-op loop, `_ring_hook_id = 0`, the removed guard (AttributeError on None instead of the named RuntimeError), and `_ring_payload = None`. No src file changed. * Pin the footer cache's recency-on-hit Round-2 finding, reader.py:333 -- and the line number is the finding. It was originally filed at :342, which a verifier REFUTED: :342 sits inside the double-checked-lock block and is unreachable single-threaded (an AssertionError there survives the whole gate), so the cited mutation could not produce the cited LRU consequence. The real hit-path `move_to_end` is :333, where `-> pass` survives both the full gate and all 141 tests in test_capture_storage.py + test_capture_summary.py. Distinct from test_the_footer_cache_evicts_to_stay_inside_its_byte_budget, committed earlier in this run: that test never re-reads a cached pack, so no hit-path recency is observable through it. Without :333 the cache degrades from LRU to FIFO and the hot pack's footer is re-fetched cold on every read -- the exact cost the cache exists to avoid. The test asserts `_footer_cache_bytes_limit >= sum(footer_sizes)` up front, so if the default byte budget ever shrinks it fails loudly instead of silently degrading into a re-test of byte eviction. Correcting the verifier on one point: it reported that no existing test kills `popitem(last=True)` or `:355 -> pass`. Both ARE already covered -- the first by the byte-budget test from earlier in this run, the second by six tests including test_estimate_predicts_reads_exactly_without_coalescing. Line 333 is the genuinely uncovered one. `_RecordingStore` gains a `reads` list additively; `ranges` and every assertion on it are untouched. No src file changed. * Pin next_auto_group_id's monotonic group prefixes Round-2 finding, engine.py:473-474: mutating `self._auto_batch_group_id += 1` to `pass` left the gate at 1508 passed. No test referenced the counter on a real engine -- tests/test_hf_eos_strip.py:47 and tests/test_e2e_correctness_vs_hf.py:33 both REIMPLEMENT it (in a stand-in fake and in a docstring), so neither exercises engine.py, and the only real caller is adapters/huggingface/adapter.py:398, reached solely from CUDA-gated e2e tests that are not cpu-marked. A verifier refuted the assumption that this needs hardware: MonitoringEngine(enable_ring_transport=False) constructs with no ring, no native host and no CUDA, and the counter returns [0, 1, 2] -- a construction pattern cpu tests already use (tests/test_monitoring_engine_shutdown.py:22). Why a stuck counter matters: adapter.py:393-399 bumps the group on every prefill or batch-size change and mints per-request ids as f"{group}:{i}", so two successive generate() calls would both emit "0:0", "0:1", ... and the offload table is MergeTree with no dedup (native/csrc/clickhouse_client.cpp:389), so those rows collide in one catalog namespace. Pins the documented contract "returns engine-scoped integers starting at zero" (docs/integration-api-v1.md:242). No src file changed. * Record the src/dmi polish run: ledger, numbers, and lessons Two rounds over src/dmi. The state file carries the per-finding ledger with verdicts and dispositions; lessons.md carries what transfers to the next run -- chiefly that this repo has three separate ways for a mutation harness to silently test the pristine tree, and that a surviving mutant is indistinguishable from one that never loaded unless the harness is proven first against a known-covered mutant. * Refuse a database name that can escape its backtick quoting `database` was the one identifier this class stored raw and then interpolated exactly like `table`: `_build_select_sql` renders `FROM {_backtick(db)}.{_backtick(table)}` into every prefilled statement. A name closing its own quoting rewrote the statement -- `default`.`other` -- commented out the intended table, the WHERE and the ORDER BY, so `prefix_get` silently read a different table while `table="off`load"` had always been refused. Deliberately NOT `_validate_ident`. That is the COLUMN rule, `[A-Za-z_][A-Za-z0-9_]*`, and it would reject `my-analytics-db`, `9lives` and `défaut` -- all legal for a backquoted ClickHouse database, all working today, and all permitted by the documented `database: str` signature. The regression test pins those four names rendering unchanged, so a later "tighten it to match the columns" change fails loudly instead of breaking deployments. Refusing rather than escaping is also deliberate: ClickHouse honours more than one escape convention inside backquoted identifiers, so a wrong escape would silently address a DIFFERENT database instead of failing -- the same class of bug this guards against. Backtick, backslash and control characters are refused; everything else is passed through as before. Scope note, since it argues against overrating this: `database` is never data- or request-influenced in-tree (the only production constructor, storage/internals.py:_default_reader, leaves it at the "default" literal), and docs/integration-api-v1.md:1206 already disclaims `custom_select`'s text check as "not a security boundary". This closes the shape without claiming the class is hardened against a hostile caller. Red -> green verified against a reverted copy of src: the injection test fails before the change, the four legal names pass both before and after. * Accept every documented eos_token_id spelling in the greedy loop `generate_greedy_with_monitoring` compared the sampled token against `eos_token_id` with `!=` and the finished sequence with `==`. Both are correct only for a scalar: torch returns a plain Python `bool` for `tensor != list` -- no broadcast, no error -- so `.long()` raised AttributeError: 'bool' object has no attribute 'long' on the first decode step past `min_new_tokens`, and `.nonzero()` would have raised on the same bool afterwards. The list form is not exotic. It is what `generation_config.eos_token_id` holds for Qwen2.5/Qwen3 and Llama-3, it is what HF's own `generate()` takes, and line 508 forwards this very argument into `HuggingFaceAdapter`, whose docstring says it "Accepts ``int``, ``list[int]``, or ``torch.Tensor``". So the function crashed on a value its own callee documents accepting. A multi-element TENSOR did not crash but was worse in kind: it broadcast against the batch dimension, comparing request i against eos id i. Fixed by normalising once, where `device` is bound, into a 1-D int64 tensor, then using `torch.isin` at both sites -- elementwise over the batch for every spelling. int64 because argmax yields int64, so both sides compare in one dtype. No behaviour change for a scalar `eos_token_id`: `isin` against a one-element tensor is identical to the old `!=`, and the in-repo callers (benchmarks/bench_hf_transport.py, tests/hf_reference_runner.py) all pass `int(tokenizer.eos_token_id)`. Only the currently-broken list and multi-element-tensor inputs change, and nothing can depend on those. The three GPU tests assert the list form against the SCALAR form on the same scripted model rather than against a hand-computed sequence, so they pin equivalence instead of transcribing the loop. Red -> green verified against a reverted copy of src: all three fail before the change. The docstring now states the accepted spellings. * Bound the eager safety net by staging, not payload alone The eager path admitted a tensor into the ring on `transport_bytes <= available_capacity()` or `<= payload_cap()`. Both are payload-only: the ring's real per-step ceiling is min(payload, staging), which is what RingCapacities.effective_bytes publishes, what native prepare_step and reserve_record use (`effective_cap = std::min(pcap, scap)`), and what the HF adapter already warns about ("Effective capacity is staging-limited"). So the safety net accepted exactly the tensor prepare_step had just rejected. Measured against the real extension with payload=64 MiB, staging=1 MiB: 0.5 MiB: avail 67108864 -> reserve 66584576 -> after flush 67108864 1.0 MiB: avail 67108864 -> reserve 66060288 -> after flush 67108864 2.0 MiB: avail 67108864 -> reserve 65011712 -> after flush 65011712 The drain assembles each flush batch per WHOLE entry and breaks when the entry does not fit staging (drain_thread.cpp:350,434 -- no split or partial path, and PinnedStaging is a fixed cudaHostAlloc), so the 2 MiB capture is never delivered AND its payload reservation is never released: ring capacity shrinks for the life of the process while flush_and_wait() returns success and exits 0. Both branches carried the flaw, and which one fires depends on ring occupancy -- the OVERSIZED path force-flushes first, so a step's first hook takes the available_capacity branch and a later hook in the same step takes the payload_cap one. Fixing only one would have left the other live, which is why there is a test per branch plus a control proving the ring is still used when staging covers the tensor. Anything above the effective cap now falls to the existing cpu_direct branch: slower, but it is currently lost outright, so no correct caller depends on the old behaviour. `_FakeEagerRingEngine` gains `staging_cap()`, defaulting to the payload capacity exactly as RingConfig does when pinned staging is left at 0, so the two pre-existing tests are unaffected. Red -> green: both new refusal tests fail before the change; 20 passed after. * Give the eager decode step its attention mask Prefill was handed `attention_mask`; the decode kwargs were not. HF builds an all-ones causal mask over the whole cache when none is supplied, so every decode step let a left-padded row attend to its own pad positions' K/V and the greedy tokens diverged from model.generate(). Verified previously on a real model (tiny 4-layer Llama, fp32, sdpa, one left-padded row of 5 pads + 3 real against one full row): hf generate : [[66, 80, 80, 80, 80, 80], [36, 67, 63, 21, 107, 63]] greedy loop : [[66, 80, 107, 82, 52, 52], [36, 67, 63, 21, 107, 63]] The padded row diverged from the third generated token on; the unpadded row was identical, and an unpadded control batch matched generate() exactly. Cause isolated by injecting one variable at a time: supplying the grown mask alone restored exact parity, correcting position_ids alone changed nothing. So the mask is the fix and position_ids is left as it was. The mask grows with the cache -- the caller's prompt columns unchanged, then all-ones for the generated positions, which are all real. No behaviour change for unpadded or right-padded input, where the added columns and the implied all-ones mask agree; only the currently-wrong left-padded case moves. Not fixed, and now stated in the docstring: the compiled path (`cuda_graphs=True`) still passes no mask. A per-step-growing mask changes shape every step and would defeat CUDA-graph capture, and the StaticCache alternative (a fixed max_cache_len mask mutated in place) could not be verified here without real weights. Left-padded batches are therefore correct on the eager path only. The adaptor's before_forward_manual keeps receiving the caller's prompt mask, not the grown one: it derives prefill KV offsets from the prompt, which is what it wants, and that call is unchanged by this commit. Tests pin the mechanism with a recording stub -- every decode step receives a mask, its width tracks the cache, prompt padding survives and generated columns are admitted -- so they need no weights. All three fail before the change. * Disarm the hook points when detaching a model `install_ring_hooks` was the only thing that ever wrote `_ring_hook_type` / `_ring_payload`, and nothing cleared them. `detach_model` restored the prepare wrapper and emptied `transport._active_specs`, but the HookPoints are structural members of the model and outlived the call still armed. That matters because both `generate_with_monitoring` and `generate_greedy_with_monitoring` call `detach_model` from a `finally`, so every monitored generate ends with the model armed. The next ordinary forward on it -- an eval pass, a perplexity computation, another library calling the same object -- then launches producer kernels with no `prepare_step` reservation and an empty metadata FIFO. `HookPoint.forward` gates only on `enabled`, `_ring_hook_type is not None` and `x.is_cuda`; `_active_transport` stays set until `engine.close()`, so `g_active_engine` is non-null and the native producer does not early-return; and `hook_no_notify` is documented "No condition gating. Space is guaranteed by the pre-forward capacity check in Python" -- a check that did not run. Measured before this change on a real model: 27 producer dispatches and 29,638 bytes written into the payload ring after detach, while `available_capacity()` still reported only the previous generate's bytes outstanding. The strays are worse than lost -- `do_post_processing` pops one TensorMeta per task in FIFO order, so they consume the NEXT monitored step's metas and every later payload is paired with the wrong one. `uninstall_ring_hooks` is the exact inverse of the install: it clears `_ring_hook_type` (the condition `HookPoint.forward` actually returns on) and drops the payload reference so a detached model stops pinning the ring buffer. `enabled` is deliberately left alone -- that carries the hook SELECTION, which `apply_hook_selection` owns and a re-attach reuses. Unlike install it tolerates an unbound spec, because it runs from teardown where raising would displace whatever the caller was already handling. The cost, stated rather than hidden: `_ring_hook_type` is a plain int precisely so torch.compile bakes it as a compile-time constant, so clearing it invalidates a traced decode graph and re-attaching costs a recompile. That is the price of not corrupting the ring on the next ordinary forward. A recompile-free variant would need the gate to move device-side; that is a larger change and is not attempted here. Tests pin the disarm, the pre-existing teardown that must survive it, the prepare-wrapper restore, and idempotence (it runs from a `finally`, so a second call after an error path must not raise). All CPU -- no CUDA needed. * Refuse duplicate capture chunks instead of comparing tensors The three `_reassemble_*` helpers grouped a request's chunks by `(request, layer)` and then called `sorted(chunks)` on `(start_token, tensor)` pairs. With no sort key, a tie on start token makes Python fall through to comparing the TENSORS: * a normal payload raises "Boolean value of Tensor with more than one value is ambiguous" -- an error naming nothing about the duplicate, the request, the layer or the shard, so the reader cannot act on it; and * a ONE-ELEMENT payload compares fine and is silently concatenated, merging two captures into a single wrong tensor of double the token count. The silent case is the dangerous one and it is why a stable tiebreak (`key=lambda c: c[0]`) would have been the wrong fix: it makes that silent merge universal instead of removing it. The collision is production-shaped, not hypothetical. `filter_by_tp_rank` deliberately keeps TP-sharded hooks on every rank, `resolve_shard_rank()` returns `ctx.tp_rank` for them, and the repo writes all ranks to one table with `shard_rank` telling the rows apart -- tests/hf_compare_runner.py says so outright ("Production writes all ranks to one table with shard_rank distinguishing per-rank rows"), and the repo's own comparison tooling (tests/hf_comparator.py) picks one shard_rank before grouping, which is exactly the filter internals.py lacked. Because `get_internal` reassembles every field eagerly, one colliding sharded act made even the complete, unsharded hidden_states unreachable. Reassembly here concatenates along the TOKEN axis, and two shards are the same tokens rather than more of them, so merging is not a correct answer at any alignment: refusing is. `_ordered_chunks` now sorts by an explicit key -- so tensors are never compared under any input -- and raises a named RuntimeError identifying the act, the request and the duplicated start tokens, and pointing at shard_rank and shared model_ids as the two causes. RuntimeError rather than a new type so callers already catching the torch error keep working. Not changed: grouping still ignores shard_rank, so this does not silently make TP runs readable. That would be a contract change (docs/integration-api-v1.md disclaims cross-shard reassembly) and is left for a deliberate decision. Tests cover both shapes at the per-layer and non-layered sites -- including the one-element case that previously returned a wrong answer instead of raising -- plus controls proving out-of-order chunks still reassemble in token order and a single-shard run is still readable. * Describe MonitoringConfig's storage fields in the v1 contract The v1 doc said "MonitoringConfig currently contains only this schedule", which stopped being true when `storage_backend` and `capture_sink_config` were added. Both are exported through `dmi.api.v1`, both are acted on by `MonitoringEngine`, and two combinations raise at construction -- so a caller following only this document could not know the fields existed, let alone that setting them can refuse. Verified against engine.py:120-133 rather than transcribed: "native" without a host engine raises, and "capture"/"none" WITH one raises. The fields are documented elsewhere (docs/capture-storage-design.md, docs/benchmarks.md), so the two documents disagreed and the v1 contract was the stale one. * Drop the .loop scratch state from the tree The /polish loop's working files are per-run agent bookkeeping, not project artifacts: nothing in the tree, the build, or CI reads them, and they go stale once the branch they describe has landed. #133 removes them from main; dropping them here too so a merge of this branch cannot carry them back in. * Give each decode row its pad-aware RoPE position and stop regrowing the mask The eager decode step sent every row torch.full((B,1), Pmax+step+1): a k-left-padded row overshot its true position by k+1, and even an unpadded row by 1 (the CUDA-graph path starts cache_position at Pmax, so the two paths disagreed by one every step). Prefill is pad-aware (mask.cumsum(-1)-1), so a row's first decode token belongs at its real-token count -- exactly what HF derives from the grown decode mask via decode_mask.cumsum(-1)[:, -1] - 1. Compute next_pos from the mask once and send next_pos + step per row. While there, build the widest decode mask once before the loop and slice it per step instead of a per-step torch.cat, which was O(T^2) over the decode; values, dtype and device are identical. * Let readers select a shard_rank when TP ranks collide The duplicate-chunk refusal tells the caller to 'select a single shard_rank before reading', but neither get_internal nor LazyInternal had any such parameter -- the prescribed remedy was unreachable, so every TP>1 capture was a hard dead end. Give both an optional shard_rank that filters rows by key[4] before reassembly; the default None keeps the refusal-on-collision behavior, whose message is now an instruction the API can actually follow. * Cache the effective ring ceiling instead of querying caps per forward HookPoint.forward recomputed min(payload_cap(), staging_cap()) on every eager invocation -- two pybind crossings per hook forward on getters the native header documents as not to be called per-step, computed even on the strip cpu-direct branch that never uses the value, and the fourth open-coded spelling of RingCapacities.effective_bytes. Name the formula once as engine.effective_ring_bytes, cache the result on the hook the first time the eager ring branch needs it, and clear the cache on (un)install since a re-attach may bind a different engine. * Build the duplicate-start diagnostic in one pass The refusal named exactly the duplicated starts, but found them with starts.count(s) per start: a long chunked capture paid O(n^2) to build an error message that only names the repeats. One frequency map now feeds the same sorted list, so the diagnostic is linear plus the sort. The new case pins that only the colliding starts are named, sorted -- the property the O(n^2) set comprehension provided and the fix must keep. * Cache the ring ceiling on the transport, not per hook The per-hook cache only moved the burst: the first eager forward still answered payload_cap()/staging_cap() once for EVERY active hook, and the native header documents those getters as startup-only. The min now lives on the RingTransport (engine-lifetime constants, one transport per engine) and every hook reads it; a re-arm builds a fresh transport, so a new engine's caps are read rather than the old min. The install/uninstall cache resets go with it -- there is no per-hook cache left to clear. The eager-cap test now drives the real RingTransport (counting engine behind it), so the cache under test is the production one, and pins that a second arming reads the second engine instead of reusing the first. * Stop promising right-padded correctness in the greedy loop The note said right-padded batches were unaffected, but this loop always takes the last prompt position's logits -- a pad under right padding -- and the compiled decode step runs with no attention mask, so the pad's K/V stays visible to decode. Right-padded prompts are unsupported on both paths; only unpadded is unaffected, and left-padded only on the eager path. * Simplify the plumbing around the transport cap cache - ring.py imports effective_ring_bytes at module level like every other consumer; the function-level import bought nothing (dmi.engine has no transport import to cycle through). - The eager-cap test uses the real RingTransport with submit_cpu_direct monkeypatched on the instance, instead of a wrapper class that forwarded every attribute through __getattr__. - The chunked-schema fake computes its effective_cap once in __init__ rather than recomputing it behind a property. No behavior change; 41 passed, 20 skipped (native backend absent). * Guard the greedy-EOS GPU tests with require_cuda() The three `@pytest.mark.gpu` tests reached `_run()`, which calls `.cuda()` and builds tensors with `device="cuda"`, behind no guard at all. Markers in this repo are declarative only -- there is no conftest.py anywhere, and `addopts` deselects `manual` and nothing else -- so `make test-all` (plain `pytest -q`) collects and runs them on a CPU-only host, where they die with `RuntimeError: No CUDA GPUs are available` before reaching an assertion. tests/_requirements.py states the contract these missed: every GPU test should "fail closed with a precise reason, never with an import error or an opaque crash". The sibling test_hf_greedy_decode_mask.py already honours it. Guarded per-test rather than with a module-level `pytestmark = [pytest.mark.gpu, require_cuda()]`: this module is mixed, and also holds three `@pytest.mark.cpu` tests that must keep running on a host with no GPU. Verified both directions with CUDA_VISIBLE_DEVICES="" and without: without CUDA: 3 failed -> 3 skipped "no CUDA device available", 3 cpu pass with CUDA: 6 passed -- the tests still run, they are not disabled * Join TP shards on request with merge_shards=True Under tensor parallelism a sharded hook is written once per rank, each rank holding a different slice of the SAME tokens. Reassembly concatenates along the token axis, so it cannot merge those rows; it refuses the collision by name and the caller picks one `shard_rank`, getting that rank's slice rather than the model's tensor. Nothing reconstructed the whole thing. `merge_shards=True` does, joining in ascending rank order along the axis TP actually split. The axis is derived from the row layout rather than passed in: an attention matrix row is [heads, q, kv] and splits on dim 0, while q, k, v, z and mlp_post are [tokens, features] and split on the trailing axis. `_shard_axis` mirrors what `reassembly.segment_manager` already encodes, so the layout stays one decision rather than two that can drift apart. Opt-in, so the refusal added earlier in this PR remains the default for callers who do not know their data is sharded. `shard_rank` and `merge_shards` are mutually exclusive -- they are opposite answers to the same question -- and passing both raises rather than letting one quietly win. Two things are still refused under merge_shards, because merging them would be wrong rather than merely unsupported: * two rows from the SAME rank, which is a duplicate capture (separate runs sharing one model_id), not a TP split -- joining them would fabricate a tensor wider than the model ever produced; and * rank sets that differ across start tokens, where some token is missing a rank's rows, so the merged tensors would disagree on width. docs/integration-api-v1.md said v1 "does not reconstruct TP-sharded fields across shard_rank", which this makes false; it now documents both routes and the duplicate case that survives. Nine tests. Two mutations confirm they bite: dropping the rank sort fails the arrival-order test, and collapsing the axis to -1 fails the attention test. cpu suite 1219 -> 1228 passed, 0 failed. * Fix the TP shard join axis, and refuse k/v where GQA may replicate Two defects in the merge_shards join added a commit ago, both found by review and both reproduced by a failing test before being fixed. `_shard_axis` returned -1 for everything except attention matrices, on the claim that q/k/v/z/mlp_post are stored [tokens, features]. compute_hook_shape (dmi/hooks/specs.py) says otherwise: q, k, v and batched z are [tokens, heads, head_dim] -- three-dimensional, with TP splitting HEADS in the MIDDLE. Joining on the trailing axis glued head_dim together instead: a 4-head x head_dim-3 model over TP=2 returned [tokens, 2, 6] where the model's tensor is [tokens, 4, 3] -- the same element count, so nothing raised, but every head was a splice of two different heads. The axis is now derived from where the TOKEN axis sits, which is what reassembly.segment_manager already encodes: attention rows are [heads, q, kv] and join on dim 0; every other row is token-major and joins on dim 1, the axis immediately after tokens. Dim 1 is the trailing axis anyway for the two-dimensional packed z and mlp_post, so one rule covers both layouts -- and unlike -1 it stays correct when a head axis is present. k and v are now refused outright. `kv_heads = max(1, num_kv_heads // tp)` means a GQA model with fewer KV heads than ranks gives every rank the SAME kv head, and the per-rank shapes are identical whether the heads were split or replicated. The row key carries neither num_kv_heads nor tp_size, so nothing in the reader can tell the two apart; merging a replicated K would return duplicated heads the model never produced. Refusing is the only answer that cannot be silently wrong -- the caller selects a shard_rank. Six tests, each RED before the change: q [tokens, heads, head_dim] joins on heads, not head_dim three-dimensional z likewise two-dimensional mlp_post still joins on its only feature axis (guard) k and v are refused, naming replication and the way forward a database name carrying '%' is refused (below) Separately, the database guard this PR added missed the OTHER way out of backtick quoting. clickhouse-driver substitutes parameters as `query % escape_params(params)`, and escape_params quotes with ' escaping only ' and \ -- never a backtick. So `%(model_id)s` inside the database name is replaced AFTER validation by the parameter value, in identifier position: FROM `analytics'x`.`secret_db`.`other_table` -- '`.`offload` reads another database with the remainder commented out, driven by an ordinary prefix_get key. '%' is now refused, which also turns a bare '%' from an opaque per-query "unsupported format character" into an error at the point the name is supplied. Docs corrected in both places that repeated the wrong axis rule, and the v1 facade gap (neither shard_rank nor merge_shards is forwarded by dmi.api.v1.make_lazy_internal, so TP runs hit the refusal with no remedy) is now stated rather than implied. cpu suite 1804 -> 1810 passed, 0 failed. * Withdraw merge_shards; keep the database '%' guard it uncovered Reverts the TP shard-joining feature added in 5a0f645 and repaired in 243a4c8, restoring src/dmi/storage/internals.py to its state before either. This PR's own `shard_rank` selection and the duplicate-collision refusal are untouched. Withdrawn rather than kept-and-fixed, for reasons that outlived the bug: * It is unreachable from the surface it was documented on. This document tells integrations to import only from `dmi.api.v1`, whose `make_lazy_internal()` forwards neither `merge_shards` nor `shard_rank`, and which does not export `get_internal` at all. On the supported surface the feature did nothing. * What it did deliver was partial. `k` and `v` had to be refused, and that is not a gap a later commit closes: `kv_heads = max(1, num_kv_heads // tp)` means a GQA model with fewer KV heads than ranks REPLICATES them, the per-rank shapes are identical either way, and the row key records neither num_kv_heads nor tp_size -- so the reader cannot distinguish a replicated K from a split one. * The axis rule was wrong on its first outing, in a way that returned a plausible tensor rather than raising: q, k, v and batched z are [tokens, heads, head_dim], so joining on the trailing axis spliced head_dim instead of joining heads. It took dedicated review to catch. Those are design questions -- where shard geometry comes from, and whether the v1 facade should forward these arguments -- not defects to patch inside a correctness PR. The work is preserved on a branch and can return as its own change once they are settled. KEPT, because it belongs to this PR rather than to the withdrawn feature: the '%' refusal in `_validate_db_ident`, which closes a hole in this PR's own database guard. clickhouse-driver substitutes parameters as `query % escape_params(params)`, and escape_params quotes with ' escaping only ' and \\ -- never a backtick. So a `%(model_id)s` in the database name is replaced AFTER validation by the parameter value, in identifier position: FROM `analytics'x`.`secret_db`.`other_table` -- '`.`offload` which reads another database with the remainder commented out, driven by an ordinary prefix_get key -- the exact statement rewrite the guard was added to prevent, reached without ever putting a backtick in the name. Refusing '%' also turns a bare '%' from an opaque "unsupported format character" on every query into an error where the name is supplied. The TP paragraph in docs/integration-api-v1.md returns to its previous wording, which is accurate again, plus a note that reading a TP run now raises rather than mis-concatenating and that `shard_rank` is not yet reachable through the v1 facade. cpu suite 1796 passed, 0 failed. --------- Co-authored-by: Alan Liu <zaoxing@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Removes
.loop/polish-seen.mdand.loop/polish-state.md, and adds.loop/to.gitignore.Why
These are the
/polishloop's per-run working files — a findings ledger and aseen-set keyed to branch baselines (
92c2225/7eac029/a4d3dcb). They areagent bookkeeping, not project artifacts, and they went stale the moment those
branches landed. They rode into
mainwith the #127 merge (dac94dc)unintentionally.
Verified nothing depends on them before removing:
git grep -F .loop -- . ':!.loop/' ':!third_party/'onmainreturns nothing..github/.tests/test_live_loop_fixes.pyis deliberately left alone — despite the name itis unrelated to this tooling, and pins regressions for the E2E verification
loop's quorum DDL and
E2E_RING_PAYLOAD_BYTESfixes.The
.gitignoreentry sits next to the existing.claude/line so a later runcannot commit the directory again.
🤖 Generated with Claude Code