Mikegrier/probes encoded row contract - #89
Merged
Merged
Conversation
…nd re-plan M2 M2 built a report oracle that reads the RENDERED TEXT and checks the prose half against the NDJSON half. Its charter said that had to be so: "checking state would miss precisely this defect class -- in the original finding the state was consistent and the two renderings of it were not." Re-checked against the code, that sentence is wrong about its own evidence, and both defects it rests on were defects in the encoded row. **The alarm beside the agreeing verdict.** The alarm is a `writeln!` into the prose and the row has no key for it, while `cross_check` IS a key and read `agree`. So the row certified a clean agreeing measurement on a host where the probe had detected its own bug. A survey mining it would have been wrong with no way to know. The prose alarm was not the defect -- it was the only trace that the row was, which is why a human found it and no instrument did. **`efficiency classes: [0]` against `"efficiency_classes":1`.** Both halves were correct derivations of one consistent value. Note how it was repaired: the row now publishes the set. The fix was to change what the row publishes; the prose comparison was how a reviewer noticed, not the repair. **A correction made while writing this up, and it strengthened the case.** The first draft said the alarm's condition is unpublished so the verdict can still read `agree`. Checked before committing: the second half is no longer true -- `cross_check` now pushes `SummaryMissing` onto `parse_incomplete`, which forces the verdict away from `agree`. Writing a superseding decision against code as it used to be would have been the exact defect class this branch spent fifteen review rounds on. What the check found instead is live and larger. The row publishes `not_compared`, `parse_incomplete` and `enumeration_anomalies` as COUNTS, where the prose prints each entry's text. A survey reading `"parse_incomplete":1` cannot tell a probe self-bug from a self-contradicting core record from an unmeasured topology. The row is impoverished relative to the prose: the artifact that gets mined carries less than the artifact that gets read. So the decision recorded here is that they are not peers. The row must be CORRECT, machine-enforced. The prose must be ACCURATE AND READABLE, enforced by review. Nothing is required to hold between them. Counted, for what the other reading cost: of 38 top-level functions in report_oracle.rs, ten are correspondence rules and four are comparison helpers -- twenty-three exist only to extract values back out of rendered text. That is a parser for a format this crate writes, and it behaved like one: a large share of PR #88's review rounds were defects in the reader rather than the thing read. M3 queues the work in dependency order: publish each diagnostic as itself rather than as a count (M3.1); relocate the three surviving correspondences onto the observation as invariants checked before rendering (M3.2); emit the row from a typed value through one writer, which makes injection and field-order defects unrepresentable and subsumes the typed-banner question (M3.3); retire the prose-against-row family and its extraction layer, keeping only a well-formedness check on the row (M3.4); re-aim the corpus and fact accounting at the row's fields (M3.5); and split DESIGN-NOTES.md, now 88 KiB and XL, into Tier 1 and Tier 2 (M3.6). M2.18 is dissolved into M3.3 rather than answered alone -- it was the smallest instance of the same question. M2.17 is re-scoped by M3.5 and should follow it, since crossing prose shapes that are about to stop being checked aims at the retiring half. The nine remaining M2 items are unaffected. Supersedence markers are adjacent to both affected decisions, and the earlier one keeps everything about it that still holds -- what the oracle admits, the acceptance half, and the failure mode that looks like success. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ount
Completed item: M3.1: Publish each diagnostic as itself, not as a count.
The row published `not_compared`, `parse_incomplete` and `enumeration_anomalies`
as `.len()`, so it said a run was in doubt without saying why. A survey reading
`"parse_incomplete":1` could not tell `partitioning_summary_missing` -- this
probe detecting a bug in ITSELF -- from `contradictory_cores` or `not_measured`.
Four categorically different facts, one cardinality, and only the prose
separated them.
That is the gap recorded in DESIGN-NOTES.md -> #d-encoded-row-is-the-contract:
the row was impoverished relative to the prose, which is backwards given that
the row is what a fleet survey mines and what the designs rest on.
**The vocabulary is now three enums** in `topology::diagnostic` -- 21
`ParseIncomplete`, 6 `NotCompared`, 3 `Disagreement` variants, one per condition
that `cross_check` can report. Each carries its own data, renders its own
sentence through `Display`, and names itself through `code()`. `CrossCheck`'s
three `Vec<String>` became `Vec<Diagnostic>`, and all 30 push sites construct a
variant rather than formatting a string.
One definition, so the code and the sentence cannot drift apart -- where a
parallel `(code, String)` pair could be updated on one side only.
**The code is the contract; the sentence is not.** Changing a `code` is a
breaking change to the row, exactly as renaming a field would be. The `Display`
text reaches only the prose, where a reader is the consumer and rewording is
harmless. That is what lets the prose be written for a human: before, a test or
a survey wanting to know which condition fired had to match on English, which is
a reason not to improve it. 34 assertions did exactly that and now match on the
variant.
**The wire format changed, deliberately and not additively.**
`"parse_incomplete":1` is now `"parse_incomplete":["partitioning_summary_missing"]`.
The count is still available as the list's length, so nothing is lost, and
publishing both would be a restatement that can drift. Same correction, and the
same argument, as the `efficiency_classes` count-to-list change that preceded
it. Marked `!` for that reason: any consumer reading those three fields as
numbers must change.
The prose is byte-identical. The renderer's loops write `{entry}` and `Display`
emits the sentences that were there before.
**Which conditions are listed is deliberately not compared against the prose,
and needs no rule.** The code and the sentence come from one variant, so there
is no second implementation for them to disagree through: the correspondence
holds by construction, which is stronger than a check. What remains checkable,
and is checked, is that both renderings list the same NUMBER.
That was found rather than assumed. Converting the accounting instrument, a
mutation that swapped one code for another went unnoticed on the
`verdict incomplete` corpus shape -- because the oracle reads the length, and a
one-element list rewritten to a different one-element list has the same length.
`corruptions` now APPENDS a code instead of substituting one, so the length
differs by one whatever the host rendered.
`ndjson_count` was retired rather than left unused: with the three fields now
lists, it had no remaining consumer, and a dead reader is a second grammar for a
format that no longer has two.
Verified. 249 lib tests plus 14 ignored, 9 integration tests and doctests, on
both `x86_64-pc-windows-msvc` and `i686-pc-windows-msvc`. Workspace
`cargo check --all-targets` clean in debug and release, `cargo doc` clean under
all three rustdoc deny lints, clippy --all-targets --all-features clean, fmt
clean, encoding clean over 635 files. A real `probe-topology` run emits
`"not_compared":[],"parse_incomplete":[],"enumeration_anomalies":[]`.
The four new tests were each observed to fail, with the mutation injected on its
own line and reverted: renaming `PartitioningSummaryMissing`'s code reddens only
`the_row_names_the_probes_own_bug_when_it_detects_one`; making the row keep only
the first condition reddens both list tests, through the bound oracle's count
rule; mislabelling `TrailingBytes` reddens only
`an_anomaly_reaches_the_row_as_its_kind`.
Swept the claim this change falsified: DESIGN-NOTES.md stated in two places that
the row publishes `parse_incomplete` as a count, and M3's checklist preamble
stated it as a present gap. All three corrected in this commit rather than left
to be found later. The design-session record keeps its present tense, being a
dated account of that session.
`AnomalyKind` is `#[non_exhaustive]`, so `anomaly_code` needs a catch-all and a
variant added upstream lands in it. That is stated rather than hidden:
`unclassified` means "this probe's vocabulary is older than the crate's", which
is more useful to a survey than a code invented here that pretends to name the
new kind. Deliberately not a compile error -- owning the row's vocabulary means
this crate decides when a new upstream kind earns a code.
M3.3 picks up what this left: the row carries a condition's code but not the
values its variant holds, because the row is still a positional `concat!`
template where a nested per-entry object has to be hand-assembled. Queued there
rather than dropped, and the blocker is the row's shape, not the want of a
consumer.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nate
A review of the M3.1 change set found two tests that had stopped testing what
they are named for, and three stale statements.
**The substring-to-variant conversion dropped what the substring discriminated.**
`both_absent_counts_are_named_together_rather_than_one_standing_for_the_pair`
became `matches!(c, MeasuredButCountsAbsent { .. })`, which holds when `absent`
names only ONE of the two counts -- exactly what the test forbids. Measured: with
`absent` truncated to its first entry, the whole suite stayed green at 249
passed, so the test could not fail for its own property and was fully subsumed by
the loop above it.
The same conversion took the discrimination out of
`a_measured_topology_reporting_no_processors_or_groups_blocks_agreement`: its
loop labels had BEEN the assertion (`c.contains(label)`) and became captions for
the failure message, so neither iteration checked that the correct count was
named.
Both now assert the variant's payload, and both were observed to fail:
truncating `absent` reddens the both-absent test, and swapping the two names in
`cross_check` reddens both.
The general lesson, recorded in the checklist: converting an assertion from a
substring to a variant DROPS whatever the substring discriminated inside the
payload. The variant is the weaker claim unless the payload comes with it.
**Swept the class rather than the reported instance**, which found one more --
`an_empty_core_or_package_record_blocks_agreement`, where both arms raise
`RelationsWithoutProcessors` and the variant match cannot separate an empty CORE
record from an empty PACKAGE one. That gap PREDATES this branch: `Display`
renders `{cores} core(s) and {packages} package(s) cover no processors` as one
sentence, so the old `"core(s) and"` and `"package(s) cover no processors"`
substrings both matched whichever field was set. Fixed here because it is the
same class as the two above, and verified the same way: swapping `cores` and
`packages` at the construction site reddens it.
Reviewed the other thirty-odd conversions against this: the rest map to distinct
variants whose identity is the whole claim, and the one that did carry a payload
distinction -- `contains("none")` for a missing NUMA node -- kept it as
`HighestNumaNode { parsed: None, .. }`.
**Three stale statements**, each made false by the commit before this one and
each verified against the code:
- Both `PLANS.md` trackers still said the row publishes its diagnostic lists as
bare counts, in the present tense. They were written before M3.1 landed and
were not swept with DESIGN-NOTES.md and CHECKLIST.md, so they contradicted the
code and three sibling documents.
- `report_oracle.rs` carried the deleted `ndjson_count`'s summary line orphaned
above `ndjson_list_len`'s own, so rustdoc rendered a two-sentence summary whose
first half described a function that no longer exists.
- DESIGN-NOTES.md and CHECKLIST.md M3.3 both said the row interpolates
"eighteen values" positionally. Counted: the template has seventeen `{}`
placeholders and takes seventeen arguments -- `reason` is a literal. Since the
sentence's point is the hazard of a miscounted placeholder, the placeholder
count is the one it should quote. The session record's "eighteen keys" is
correct and unchanged.
Verified: 249 lib tests plus 14 ignored, 9 integration tests and doctests.
clippy --all-targets --all-features clean, fmt clean, cargo doc clean under all
three rustdoc deny lints, encoding clean over 636 files.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A review found the list M3.1 built a vocabulary for and never wired.
`Disagreement::code()` had ZERO production callers: the row carried
`cross_check`, `not_compared`, `parse_incomplete` and `enumeration_anomalies`,
and no `disagreements` field at all -- while the prose listed each disagreement
with both readings.
So a survey could see `"cross_check":"disagree"` and could not tell WHICH
counter contradicted the parse, or whether several did. That is the same shape
as the defect this milestone came from, in the one list nobody had noticed was
missing, and it breaks the rule M3.1 itself established: a renderer may not tell
a reader something the row cannot tell a survey.
The gap predates the milestone -- `disagreements` was never in the row, not even
as a count -- but M3.1 is what made it a stated violation rather than an
omission, and M3.1 converted all three lists while wiring two.
**Why every instrument here missed it, which is the durable finding.** The fact
accounting enumerates the ROW's keys and requires each to be classified, so it
asks "does anything read this key?" and never "does the prose state a fact the
row omits?". A fact with no key is outside the set of things it can have an
opinion about. The mutation sweep has the same orientation: it perturbs code the
row's construction reaches. Between them they survived 41 review rounds and a
zero-survivor sweep on this exact omission. The reviewer found it by reading the
enum and asking who called `code()`.
Recorded in DESIGN-NOTES.md as a limit of what the instruments cover -- the rule
is a convention, not a checked property, until the second enumeration exists --
and queued as M3.5, which owns the accounting.
The row now publishes `"disagreements":[...]`, and two rules read it:
- The DISAGREE arm counts them against the prose's bare `- ` lines, symmetric
with the two tagged kinds it already counts. That arm labels each kind, which
is why it can separate all three where the INCOMPLETE arm can only sum.
- The agreeing-verdict alarm covers it, and more directly than the other three:
a non-empty `disagreements` makes the verdict `Disagree` by construction, so
an agreeing row publishing one contradicts `CrossCheck::verdict` itself rather
than a rule about what `agree` implies.
Marked `!` for the same reason as M3.1: the row gained a key, and a consumer
asserting on its shape must change.
Two existing tests went red on the way, both correctly. An oracle fixture listed
a disagreement in prose with no `disagreements` in its row, which is now the
violation it looks like; and the accounting had nothing reading the key on an
INCOMPLETE shape, which the same `AtValueUnlessAgreeing("[]")` classification
`enumeration_anomalies` already carries resolves -- at `[]` there is no second
rendering, and under `agree` a non-empty list is a violation in its own right.
Sabotage-verified: with the row publishing an empty list unconditionally,
`the_row_names_which_counter_disagreed` fails through the bound oracle.
**Swept the placeholder count this change falsified, and stopped quoting it.**
DESIGN-NOTES.md and CHECKLIST.md said the template interpolates "eighteen
values"; a review corrected that to seventeen this afternoon; adding
`disagreements` made it eighteen again within the hour. Both now state the
coupling instead -- a field's name and its value are related only by counting
positions -- which is the hazard the sentence is actually about and cannot rot.
That is M2.14's first authoring rule, learned for the third time on the same
sentence.
Verified: 251 lib tests plus 14 ignored, 9 integration tests and doctests, on
both `x86_64-pc-windows-msvc` and `i686-pc-windows-msvc`. clippy --all-targets
--all-features clean, fmt clean, cargo doc clean under all three rustdoc deny
lints, encoding clean over 636 files. A real `probe-topology` run emits
`"cross_check":"agree","disagreements":[],"not_compared":[]`.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ff the text Completed item: M3.2: Assert the surviving correspondences as invariants on the observation, before rendering. The correspondences worth keeping were never really about rendering. They relate a STATE to the verdict, and the report is only where that relation first became visible. `topology::invariant` states them as predicates over `Observation`, so they run whether or not anything was rendered and no parser stands between the rule and the values it reads. **The first attempt was wrong, in a way worth recording because it is not obvious.** It had four rules, two of which read `CrossCheck`'s lists: "a non-empty `parse_incomplete` forbids `agree`", and the same for `disagreements`. `CrossCheck::verdict` is a pure function of those three lists, so both are restatements of its definition -- they cannot fail for any input, and three of the tests written for them asserted acceptance under violation-sounding names, which is the exact defect this branch has been fixing all day. Worse than useless: a list-reading rule **cannot catch the defect this component exists because of.** That defect was a state -- a named partitioning level with no summary -- that `cross_check` had no branch for. A missing branch leaves the list EMPTY, so a list-reading rule sees nothing and the verdict it produces is `Agree` legitimately. The evidence survives only in the observation. So every rule now reads the observation. `blocking_states` names ten states that forbid an agreeing verdict -- the summary-missing bug, recorded anomalies, an unmeasured topology, no caches, no packages, no cores, a self-contradicting core record, a cache level numbered 0, enumerations that did not agree, a bracket that did not hold -- each computed from the observation, each with a push site in `cross_check` that it does not consult. Plus the two counter rules: `agree` asserts every check was MADE, so a counter that reported failure or disagreed cannot sit beside it. `check` takes the VERDICT rather than deriving it. That is not a convenience: `cross_check` is what makes these hold, so no observation can violate them while it is correct, and a derived version would leave every branch reachable only by editing the source -- a green run carrying no information. Supplying the verdict lets a test hand over the answer a broken `cross_check` would give. Bound at two places, covering different things. `observe` covers every observation this crate MEASURES, rendered or not, which is what the item asked for. `report` covers every observation anyone RENDERS, which on the test side is most of them, since the suite builds observations by hand rather than measuring a machine. Never in `cross_check`: `assert_holds` asks it for the verdict, so the assertion would recurse. Sabotage-verified on the push site that matters. Deleting the `PartitioningSummaryMissing` push from `cross_check` -- the fix for the originating defect -- reddens four tests, two of them new: the invariant's own `every_blocking_state_is_one_the_real_cross_check_already_reports`, and `the_row_names_the_probes_own_bug_when_it_detects_one` through `assert_holds` at the renderer binding. The other two are pre-existing verdict tests, so the invariant is not the sole detector for THIS push site; its value is the nine others, several of which have no dedicated test. `every_blocking_state_is_one_the_real_cross_check_already_reports` is the test that keeps this a postcondition rather than a second opinion: for each state, the verdict the crate actually draws must ALREADY be something other than `agree`. If one ever fails, the answer is to fix `cross_check`, not to relax the invariant. Verified: 260 lib tests plus 14 ignored, 9 integration tests and doctests. clippy --all-targets --all-features clean, fmt clean, encoding clean over 636 files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ting at the row Completed items: M3.4, M3.5 Completed item: M3.4: Retire the prose-against-row correspondences and the parsers that serve only them. Completed item: M3.5: Re-aim the shape corpus and the fact accounting at the row. **Committed together because they cannot be separated.** The fact-accounting instrument is built entirely on `report_oracle::check` and the `Correspondence` variants, so deleting the correspondences leaves it measuring nothing and the suite red between the two items. Splitting would produce a commit that does not pass, which the checklist rules call the worse defect. `report_oracle` read the report's PROSE, extracted values back out of the rendered sentences, and compared them against the NDJSON row. DESIGN-NOTES.md -> #d-encoded-row-is-the-contract retired that: the row is a machine contract and the prose is for a reader, they carry different obligations, and nothing is required to hold between them. Measured. `report_oracle.rs` 79,394 -> 8,874 bytes; its tests 105,299 -> 5,598; the integration instrument 71,007 -> 25,689. All eight prose correspondences and all twenty-three extraction helpers are gone -- a parser for a format this crate itself writes, which behaved like one: a multi-byte panic, a substring matching inside an opaque `io::Error`, a `trim_matches` collapsing `[[0]]` and `[0]`. What survives is the writer's own output read back, which is the one thing no amount of typing upstream can do for itself: exactly one machine-readable line, brackets balanced, no repeated top-level key. The balance check is string-aware because a failed discovery's `io::Error` is interpolated into a string value and an OS message is free to contain a bracket -- a naive counter would report every such host as malformed. The repeated-key check is there because that malformation survives a consumer's parse and silently changes what it reads. **The key-set check is deliberately not here.** Asserting it needs a list of expected keys, and a list written here is a census -- this component re-corrected the same census three times in one day. M3.3 makes the row a typed value, at which point the set is derivable from the type. Moved there rather than approximated now. **The accounting now enumerates in the direction the rule lives in.** It no longer asks "which prose facts does the oracle read"; it asks, for every state `topology::invariant` knows forbids agreement, whether the row publishes a condition for it -- and holds the prose's diagnostic line count against the row's condition count across the corpus. That is the enumeration whose absence let `disagreements` reach the prose and not the row at all, surviving 41 review rounds and a zero-survivor mutation sweep. Sabotage-verified against exactly that: dropping `disagreements` from the row reddens `the_row_lists_a_condition_for_every_diagnostic_the_prose_lists`. **One asymmetry, found by the instrument rather than reasoned.** Counting all four lists against prose lines failed on the anomaly shapes: the prose folds every anomaly into ONE sentence while the row lists one code per anomaly, so three anomalies read as two dropped entries. `enumeration_anomalies` counts on its own axis, checked against the OBSERVATION -- one published code per anomaly recorded -- which is the artifact the row owes fidelity to. Checking it against the number inside that sentence would be the prose-reading this milestone retired. Both publication rules carry a corpus guard, because both skip a shape in no blocking state and a drifted all-healthy corpus would leave them green while checking nothing. The corpus itself, its shape builders and the real-host render are unchanged. Marked `!`: `report_oracle`'s public surface is replaced. `Correspondence`, `check`'s return type, `architecture_in_banner` and the extraction helpers are gone, and `check` now returns `Vec<RowDefect>`. Verified: 184 lib tests plus 14 ignored, 9 integration tests and doctests. clippy --all-targets --all-features clean, fmt clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…riter Completed item: M3.3: Emit the row from a typed value through one writer. The row was built by interpolating every value positionally into a `concat!` template. Two defect classes follow from that construction, and both are closed by replacing it rather than by checking it. **Injection.** Measured on PR #88: an `io::Error` whose text contained `{` was selected as the report's machine-readable row, so a reader checked the caller's text instead of the probe's. A `Value::Text` is escaped by the writer, so no string it carries can end the string it is in, let alone start a new row. **Field order and labelling.** A field's name and its value were related only by counting positions, so a reordered argument or a miscounted placeholder yielded mislabelled data that still parses and that nothing downstream can detect. Here a name and its value are one pair, moved together or not at all. `crate::row` holds the value model and the single writer. The escaping is deliberately total -- quote, backslash, newline, carriage return, tab, and every control character below a space -- because the text it carries is whatever the OS said, and each of those alone would end something. **Each diagnostic publishes its DATA**, not only its code: `{"code":"contradictory_cores","count":3}`. That is what M3.1 had to leave behind, and the blocker it named was exactly this template. Anomalies carry `source` and `offset` as well as kind, because the same kind at the same offset across a fleet is a different finding from the same kind scattered, and neither is visible from a count. `report_unmeasured` goes through the same writer, and it is the shape that most needed it: the only renderer that interpolates caller text. The error now reaches the row as a `discovery_error` field, so a survey can group failures by cause rather than parsing the prose sentence -- which is the rule M3.1 established, applied to the one fact this renderer has. **The key-set check M3.4 deferred to here now exists, derived.** `Row::keys` reads the value, and a test asserts that the reader and the writer agree. M3.4 declined to assert a key set because a list written beside the check is a census -- this component re-corrected the same one three times in a day and the last correction was falsified within the hour. A derived set cannot rot. **Two silent behaviour changes were caught by checking the old code rather than trusting the rewrite.** `PartitioningCache` has FIVE variants, not the four a rewrite naturally reaches for -- `NoUniqueOutermost` was missing -- and `SummaryMissing` publishes its LEVEL rather than `null`. The second matters precisely because that arm is the report telling a reader this probe has a bug, and which level went unchecked is what they need. Both recovered from `git show HEAD:...` before the conversion was believed. Sabotage-verified: removing the quote escape reddens three row tests, including the brace-injection one. The clean row is byte-identical to what the template produced, confirmed against a real `probe-topology` run. `list_codes` and `list_span_end` are shared from `report_oracle` rather than copied into the two instruments that need them. The diagnostic entries are objects now, so reading them means finding each `code` member rather than splitting on commas -- and a comma-splitting sabotage inside the accounting broke silently when this landed, which is the argument for one definition: a sabotage that no longer sabotages leaves the rule it guards unguarded while still passing. Marked `!`: every diagnostic list's entries change from strings to objects, and the unmeasured row gains a field. Verified: 202 lib tests plus 14 ignored, 9 integration tests and doctests, on both `x86_64-pc-windows-msvc` and `i686-pc-windows-msvc`. Workspace `cargo check --all-targets --release` clean, `cargo doc` clean under all three rustdoc deny lints, clippy --all-targets --all-features clean, fmt clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…t matrix M3.5 left one site where the test matrix obtained structured data by reading sentences: a rule that filtered rendered lines by prefix, counted them, and compared that number against the row's condition count. A wider sweep found it had a unit-test twin in `src/tests.rs` that the first pass missed. Both are replaced by the same claim stated against `cross_check`: the row's condition codes must EQUAL the cross-check's, in order. Strictly stronger. A count catches only a dropped entry; this catches a drop, a reorder and a substitution. It also covers all three lists, which the prose count could not -- under INCOMPLETE the renderer gives `not_compared` and `parse_incomplete` the same bare `- ` prefix, so only their total was recoverable from prose at all. Be clear about what it is, since the replacement is easy to over-read: the row is BUILT from these lists, so this is the writer checked against its input, not an independent reading of the machine. That is the check worth having -- the writer is the one thing no amount of typing upstream can check for itself -- but it is narrower than "the report is correct" and the comments say so. **The ordering half was vacuous, and the guard is what found it.** Reversing the row's `parse_incomplete` order reddened nothing: every corpus shape varied one dimension, so each landed at most one entry per list, and a one-element list has no order to get wrong. The first version of the guard summed the three lists and passed while the sabotage still did nothing -- one entry in each of two lists is two conditions and no order to get wrong. Corrected to measure the largest SINGLE list, and a `several conditions at once, in one list` shape added to the corpus. The reorder now reddens `the_row_lists_exactly_the_conditions_the_cross_check_found`. That sequence is the point of the guard: a corpus cannot report the shape it does not reach, so the rule looked strong for as long as nothing asked it to prove it. Swept for the claim rather than asserting it. What still touches rendered text: selecting the row line, and positional containment -- the banner is the first line, the banner is one line, there is exactly one row-shaped line. None reads prose for its content, and no test parses prose into a value. The 58 `contains(...)` assertions on prose remain and are deliberate: they test the prose renderer on its own terms, which the decision permits. They extract nothing. Verified: 202 lib tests plus 14 ignored, 9 integration tests and doctests, on both `x86_64-pc-windows-msvc` and `i686-pc-windows-msvc`. clippy --all-targets --all-features clean, fmt clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Completed item: M3.6: Split DESIGN-NOTES.md into Tier 1 and Tier 2. DESIGN-NOTES.md was 92,467 bytes -- XL on the repository's byte scale, where the default answer is to split unless the document is indivisible. It is not: it carried current decisions and a large block of how-we-got-here reasoning, which is exactly the Tier 1 / Tier 2 fracture. The fracture taken is the correspondence-oracle investigation: the two defects a pull-request review found after twenty-eight rounds of per-artifact review and a zero-surviving-mutant sweep, why each instrument was structurally incapable of seeing them, the sparse-matrix-and-oracle split that followed, and the oracle's own design, failure modes and mutation evidence. It is Tier 2 on both tests. It records how a decision was reached rather than stating one, and the decision it reached has since been superseded by #d-encoded-row-is-the-contract. Moving it also resolved latent drift rather than only relocating bytes. The block cites the `Correspondence` enum, the fact-accounting instrument and the prose correspondence rules -- none of which survived M3.4 and M3.5. As history those sentences are accurate; sitting in Tier 1 they described deleted code as though it were current. **The result is still XL, and that is stated rather than implied away.** DESIGN-NOTES.md is 67,807 bytes, over the 64 KiB threshold. The split was made at the one unambiguous fracture rather than trimmed until it hit a number: a cut sized to a byte budget yields two documents that are each individually incomprehensible, which is worse than one large coherent document. Why it stays over: the remaining bulk is neither current decisions nor rationale, but FINDINGS -- measurements about Windows that are this crate's actual product. They do not belong in a rationale file, and filing them as decisions is what keeps Tier 1 large. Whether they want a tier of their own is a structural choice about this component's documentation scheme, so it is raised as an open question on the checklist rather than taken unilaterally. Pure relocation, verified byte-for-byte against the pre-split file: 439 lines, identical. No reflow, no rewording, no other work in this commit -- the two headings missing a blank line above them were inherited and are left for a follow-up, because a split that also tidies is a split whose blame cannot be traced. The two moved anchors are kept in Tier 1 beside a pointer, so an existing link to `DESIGN-NOTES.md#d-correspondence-failures` still lands somewhere that says where the content went. Every in-repo reference was repointed at the content itself: the superseding decision's two links, the design-session record's two, and the crate's PLANS.md row. Split-Source: crates/windows-platform-probes/DESIGN-NOTES.md Split-Into: crates/windows-platform-probes/DESIGN-RATIONALE.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A review of the branch found eight issues. The first is a real defect, reachable
through a public renderer, and the rest are claims that outlived the code.
**`report_oracle::keys` was not escape-aware, and a correct report crashed the
probe.** `balanced` tracks `in_string`/`escaped`; `keys` walked quote to quote
with `find('"')` and took `\"` for a terminator. The row legitimately carries
caller text -- `report_unmeasured` publishes a failed discovery's `io::Error` as
`discovery_error` -- and an OS message may contain a quote. When it does, the
reader's idea of where strings begin and end shifts, text INSIDE the value is
emitted as top-level keys, and two equal ones are reported as a repeated key.
Measured, not reasoned: `report_unmeasured` given `io::Error::other("q\":1,\"q\":1,\"q")`
renders a row that parses cleanly with four keys, and `assert_corresponds`
panicked FROM INSIDE THE RENDERER -- `report_oracle.rs:295`, reached through
`report_unmeasured`'s own binding. Under `cfg(test)` or `oracle-in-renderer`
that is a correct report crashing the probe, which is the failure mode the
containment work exists to prevent. The writer was never at fault; the reader
disagreed with it.
`keys` now shares the state machine through `string_end`, and two tests cover
both directions -- an escaped quote inside a value, and a value ending in an
escaped backslash, which getting wrong the other way would silently drop every
key that follows.
The same hazard does not reach `list_codes` or `list_span_end`: those scan the
diagnostic lists, whose entries carry only codes minted in this crate and
numbers. No caller text reaches them.
**A completeness guard that was documented and did not exist.**
`perturbations()` said "the interesting part is that NONE of them is missing --
which the count assertion below states as a relation". There was no such
assertion. The table happened to match `blocking_states`' ten states, which is
the condition under which nobody notices. `every_blocking_state_has_a_perturbation`
now derives the reached set by applying every perturbation and compares it
against the table's labels in both directions, so a state added to the invariant
with no entry fails rather than going unexercised. Derived rather than counted,
because a census needs correcting every time a state is added.
**A test whose name asserted the opposite of its assertion.**
`a_defect_is_reported_once_per_repeated_key_rather_than_per_occurrence` claimed
de-duplication the code does not do while asserting the per-occurrence behaviour
its own failure message describes. The assertion was right; the name and comment
were the defect. Renamed to `..._once_per_extra_rendering_of_a_key`.
**Four statements that outlived the code they described**, each verified against
the tree before correcting:
- DESIGN-NOTES.md said the three fields "publish arrays of condition codes" with
a bare-code example. True at M3.1; M3.3 gave each entry its data three commits
later on the same branch, so the published form is an object. Tier 1 is where
a reader goes for the wire format, so it is the one place that must be right.
- CHECKLIST M3.5 said the instrument "holds the prose's diagnostic line count
against the row's condition count", and cited a sabotage against
`the_row_lists_a_condition_for_every_diagnostic_the_prose_lists`. Both were
invalidated by the follow-up commit recorded further down the same item, which
removed the prose count and renamed the test -- so the item disagreed with
itself and its evidence could not be re-run as written. The sabotage was
re-run against the current name to confirm it still reddens.
- DESIGN-RATIONALE.md said the oracle section "still describes what is in the
tree and still holds", then listed three rules M3.4 deleted. True when
written, carried through the Tier 1/Tier 2 split unchanged.
- Three same-file anchor links in DESIGN-RATIONALE.md still pointed at
`#d-encoded-row-is-the-contract`, which the split left in DESIGN-NOTES.md.
**One doc comment attached to the wrong item.** Two comments had merged onto
`codes_of`, so the paragraph warning against asserting on the struct sat above
the helper that reads the struct. Moved to `row_codes`, which had none.
Verified: 205 lib tests plus 14 ignored, 9 integration tests and doctests, on
both `x86_64-pc-windows-msvc` and `i686-pc-windows-msvc`. clippy --all-targets
--all-features clean, fmt clean.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A review of the completed M3 found no wrong behaviour and four weak
instruments -- tests and guards asserting a property they could not fail
to satisfy. Each is recorded with what it was measured to miss.
1. The row's key set was unenforced. M3.3 claimed the check was "derived"
because Row::keys reads the value and a test holds reader against
writer -- but both read the same Row, so the test proved only that the
writer is self-consistent. Measured: deleting .with("packages", ...)
from the renderer left the entire suite green. Declared
MEASURED_ROW_KEYS / UNMEASURED_ROW_KEYS as the contract instead. This
is not the census M3.4 feared: a count is derivable from the thing it
counts, so restating it invites drift; a schema is not derivable from
the row, which is why writing it down buys something.
2. The well-formedness oracle accepted invalid JSON. balanced() counted
bracket depth only, so {"a":1,} and {"a":1] both passed a check whose
name promised a consumer could parse the row. Replaced with
malformation(), a typed delimiter stack that also checks separator
placement; RowDefect::Unbalanced becomes Malformed { what, row } so
the defect names itself.
3. A sabotage asserted only that it had sabotaged. The publication
accounting's sabotage stripped a condition from the report and then
asserted the condition was absent -- a fact about the string edit, not
about the rule, and it would have passed with the rule deleted. The
rule is now publication_holds(observation, text), and the sabotage
asserts it rejects the stripped report and accepts the original.
4. A completeness guard compared a table against itself. blocking_states
returned strings and the guard derived both sides from that one table.
Added a BlockingState enum with ALL, so the guard holds the table
against the type's variants.
Sabotage-verified, each against the instrument it was meant to strengthen:
the dropped field reddens the key-set contract; a writer emitting a leading
separator produces a balanced-but-invalid row that the old check passed and
the new one rejects.
Completed item: M3.7: Make four instruments as strong as their names claim
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The same defect as the previous commit's fourth finding, one level up, and introduced by the fix for it. The doc on ALL claimed "an exhaustive list the compiler checks: adding a variant without adding it there fails to build". That is not what the compiler checks. The match in described() IS exhaustive-checked, which is what made the claim look right, but it forces a new variant to acquire an ARM, never an ENTRY in a separate array. Measured rather than read: a new variant plus the described() arm the match demands compiled cleanly and left all ten invariant tests green, reached by none of them. ALL is the list the completeness guard iterates, so a variant missing from it is a blocking state nothing tests -- the exact failure the guard was added to prevent, reintroduced by the shape of its own fix. The guard's reverse loop is not a substitute. It catches a state blocking_states produces and ALL omits, but only once some perturbation reaches it -- and a state with no perturbation entry is precisely what the test exists to catch, so it is circular in the case that matters. Fixed by declaring the enum, ALL and described() from one list through a macro, so a variant that is not in the list does not exist. The claim is now true rather than deleted. Corrected the same false claim where it was restated in the guard's own comment. Sabotage-verified in both directions: the original sabotage is now inexpressible, there being no second place to omit the variant from; and its reachable equivalent, a new state in the list with no perturbation entry, reddens every_blocking_state_has_a_perturbation where before the whole suite stayed green. Three rounds on one guard -- strings, a hand-written ALL, generation. Each earlier fix moved the census somewhere harder to see rather than removing it. Completed item: M3.8: Make `BlockingState::ALL` exhaustive by construction rather than by assertion Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…m bite M2.14 said "write two authoring rules into the repository instructions". Measurement says that would have been worse than useless, so the item is re-planned into two sub-steps before any of it is implemented. Both proposed rules already exist. CONTRACT INTEGRITY rule 1 in .github/copilot-instructions.md is "Prefer a derived fact to a restated one", and beneath it "verify the binding by sabotage: change the definition and confirm the consumer's BEHAVIOR changes". Those are the two rules M2.14 proposed. Writing them again would add a second copy of a rule, which is the exact defect that section forbids and the exact mechanism, restatement drift, that it exists to prevent. Statement is also demonstrably not the gap. Both rules were in force yesterday and both were violated: 09da7e9 claims "Sabotage-verified, EACH against the instrument it was meant to strengthen" and then names two sabotages for four fixes. The one that got none is the completeness guard, found broken by a review an hour later (M3.8). A rule cited in the commit that breaks it will not be repaired by a third copy of itself. This repository has solved the problem once already, and not with a rule: the sabotage-harness CI job records that the harness "accumulated fixes over eleven review rounds and thirteen of the later defects were introduced by earlier fixes, because every verification was a one-off command that was then discarded and nothing re-checked an earlier guarantee". That is M3.8's story verbatim, and the answer then was a ratchet. So M2.14 becomes M2.14.1, give this crate the sabotage.json it lacks while two sibling crates have one, and M2.14.2, add to rule 1 the single thing this branch learned that it does not already say -- sabotage the CLAIM that a weakness was removed, not the symptom -- plus a pointer to the mechanism. The M2.14 ID is preserved because COMPLETED-CHECKLIST.md is append-only and two archived entries cite it. Completed item: M2.14 (re-plan only): Re-plan M2.14 from "write two authoring rules" into M2.14.1 and M2.14.2 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
M2.14.1. tools/run-sabotage.ps1 exists, has its own tests and runs in CI; windows-placement-probe and windows-waitable-queues each carry a sabotage.json. This crate carried none, so every sabotage run while building M3 was ad-hoc PowerShell discarded on the spot -- which is why a git checkout destroyed uncommitted work twice and a .Replace pattern silently matched two sites once. Seven entries, swept green: six caught, one survives, all behaving as declared. Each is a defect that was actually run by hand during M3, so this is transcription of verifications that were already made and then thrown away. Not wired into CI, matching both sibling manifests and the sabotage-harness job's note that a sweep rebuilds a crate per entry and is deliberately an occasional instrument. The control is the entry that matters most. It rewords a prose line to carry the same fact and must SURVIVE, which turns this component's central decision -- the row is the machine contract, the prose is for a reader -- from a sentence into a measurement. If it is ever reported as caught, a test has started reading the prose again and that test is the defect, not the entry. The harness found a defect in the manifest that the authoring script missed, which is the lesson worth keeping. The escape-aware key reader entry anchored on a line that the script judged unique by whole-line equality; the harness matches by substring and found it twice, because the same match arm appears in malformation at a deeper indent and the shallower line is a substring of the deeper one. The script's check was a second, weaker implementation of the harness's rule -- the exact defect class this manifest exists to catch. The anchor was widened to the function signature and the harness left as the only authority. Completed item: M2.14.1: Give `windows-platform-probes` a sabotage manifest, so "observed to fail" is a recorded artifact rather than a habit Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
M2.14.2. Two paragraphs added to CONTRACT INTEGRITY rule 1. Neither restates the rule, which is what the original M2.14 wording would have done and why that item was re-planned first. The new fact is about CLAIMS. Rule 1 already says to prefer a derived fact over a restated one, and to verify the binding by sabotage. It does not warn that an author may believe they derived a fact when they only MOVED the restatement -- and such an author will sabotage the old site, watch it go red, and record the class as closed. Measured on one guard in windows-platform-probes three times in succession (strings, a hand-written ALL, generation), each fix relocating the census somewhere harder to see while its commit message claimed the class was closed, and each caught by a review rather than by its author. The second paragraph points at the mechanism rather than describing it: the manifest, run-sabotage.ps1, README-sabotage.md for the format and for why a manifest with no control can only tell you the tests are sensitive. It also states that the harness is the authority on patch-site uniqueness, because re-implementing that check beside it is the same defect -- as the previous commit measured. Completed item: M2.14.2: Add to CONTRACT INTEGRITY rule 1 the one thing this branch learned that it does NOT already say, and a pointer to the mechanism Completed item: M2.14: Make the two authoring rules this branch earned actually bite Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
M3.9. The oracle's question is "could a consumer read this row", and a
consumer uses a JSON parser. Anything hand-written here is a second
opinion about what JSON is, and a second opinion is a thing that can
disagree -- so malformation now calls serde_json and the scanner is gone.
Measured, and the measurement is why this happened at all. The
hand-written check had already been through a review, which strengthened
it after finding it accepted {"a":1,}. A generated test -- 1807
single-character corruptions of a real row, judged against serde_json --
then found 159 MORE disagreements, every single one a false accept: 129
stray backslashes forming invalid escapes, 10 missing ':', 13 ',' where a
':' belonged, 3 the reverse, 3 missing values, 1 string following a
number. The review had found one instance of a class with 160 members.
Closing the last ~26 required tracking whether an object expects a name
or a value next, which is a JSON parser; so the choice was to write one
or to depend on one.
The agreement test was deleted in the same commit, deliberately. With the
parse delegated it would compare serde_json against serde_json -- green
by construction, and exactly the tautology this milestone keeps deleting.
What replaced it asks a question that is still open: not whether the
verdict is right, but whether it REACHES the caller, which is a property
of `check` that no parser guarantees. It found a real boundary while
being written: 8 corruptions destroy the leading brace, and those are
Missing rather than Malformed -- not a row at all, which for a survey
asking "did this host report a row" is the right answer and a different
one. Both branches are asserted.
Three tests stopped asserting the defect's wording. The message is
serde_json's now, so this crate does not own it; pinning it would let a
dependency's patch release redden tests about unclosed delimiters, which
would be a false finding about this crate.
A parse does NOT subsume RepeatedKey, which is why that check stays hand
-written: serde_json accepts a duplicated key and silently keeps the
last, precisely the malformation that survives a consumer's parse and
changes what it reads.
BREAKING CHANGE: report_oracle is now gated on cfg(any(test, feature =
"oracle-in-renderer")), and RowDefect::Malformed carries a String rather
than a &'static str. Every caller was already behind that gate; stating
it is what keeps the parser out of a shipping probe. Verified by
inspecting the binaries, per the precedent in that feature's own comment:
the default probe-topology.exe contains no serde_json, no oracle panic
string and no parser message, while the --all-features one contains all
three.
Swept the class rather than the reported site: fixed the module's stale
claim that the key-set check was deferred, and the checklist's claim that
the delimiter stack was the answer. Sabotage manifest re-swept, 7 of 7
still as declared. cargo doc verified in BOTH feature shapes -- the
no-features build caught an intra-doc link the gate had broken.
Completed item: M3.9: Decide the row's well-formedness by a real parse, and delete the hand-written one
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
M3.10. M3.9 removed one of six scanners in the oracle; this removes the rest. The module hand-writes no string walking at all now -- every byte-level decision about quotes, escapes and delimiters is serde_json's. Two of the four were provably unsafe on an argument nothing enforced. list_codes found "code":" and took the next quote as the end; list_span_end counted brackets with no notion of being inside a string. Both were safe only because every code is a &'static str from an enum and no caller text reaches a diagnostic list -- true, load-bearing, and guarded by no test. keys had already proved the class reachable: it made assert_corresponds panic from inside report_unmeasured on a quoted discovery_error. Parsing makes the argument unnecessary rather than merely correct, which is the difference between a property and a hope. keys needed a MapAccess visitor rather than a parsed map, and the reason is a contract. It must return the row's names in ORDER and WITH DUPLICATES, and serde_json::Map sorts while silently keeping the last of a repeated key -- which would delete the evidence for RowDefect::RepeatedKey, the one malformation that survives a consumer's parse. The visitor reads each name as the parser reads it, so both properties survive while every scanning decision stays serde_json's. That reasoning is now a sabotage entry rather than a comment: replacing the visitor with the obvious Map one-liner is caught. list_span_end was deleted, not moved. Its only caller was a sabotage doing text surgery on a list; that sabotage now parses, empties the lists and re-renders. It is the third implementation, after one that split on commas -- which sliced entries in half once they became objects -- and one that used this helper. A sabotage that hand-parses is a sabotage that can quietly stop sabotaging while still passing, leaving the rule it guards unguarded. The stale manifest entry is itself the evidence. After the change the sweep reported MANIFEST STALE: pattern found 0 times for the escape-awareness entry, because the code that could hold that defect is gone. Replaced with the parsed-map entry; 7 of 7 behave as declared. serde joins serde_json as an optional dependency behind the same oracle-in-renderer gate. Default build re-verified by binary inspection: neither appears on a normal dependency edge and probe-topology.exe carries no parser string. cargo doc checked in both feature shapes. Completed item: M3.10: Delete the last hand-written string scanners in the oracle Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
All ten M3 items are complete, so the group migrates to COMPLETED-CHECKLIST.md and the active checklist is left holding only M4 and M5. Both PLANS.md entries said "M3.1 has landed", written when M3.1 was the only item; ten had landed since, including two that reversed decisions the same paragraph described. The re-scoping notes did NOT go to the archive. M4's preamble said "M3's preamble says which M3 item gates each of them", and the notes it meant -- how M3.2 re-scoped M2.4, how M3.1 and M3.3 gated M2.5, what evidence M3.4 took away from M2.15, how M3.5 re-scoped M2.17 -- are instructions for work that is still open. A pending instruction does not belong in an append-only file nobody may edit to correct it, so they moved into M4's preamble and M3's record carries a pointer rather than a second copy. Also corrected three statements the closure falsified: M4's heading and preamble spoke of M3 in the present tense, M5's said its items could be pulled ahead of "M3 or M4", and M2.14.1's body still said this crate carries no sabotage manifest -- true when written, false since the item it describes was completed. End-of-milestone gate, all clean: cargo check --all-targets on the DEFAULT workspace (not --workspace, so the deliberately excluded members stay excluded) in debug and release, zero compiler diagnostics in either; the in-scope crate's tests unfiltered so doctests run, 213 passed 0 failed, with Doc-tests reporting 0 -- confirmed real rather than a harness quirk, the crate has no doc code blocks at all; encoding check 642 files clean; both markdown files verified 7-bit ASCII and free of dangling anchors. git fetch reports origin/main at 3f31e92, already contained in HEAD, so there was nothing to integrate and no ON-GIT-MERGE-OR-REBASE trigger. Not pushed, at the engineer's instruction. Completed item: M3 (milestone closure): archive the completed group, update both PLANS.md entries, and relocate the still-live re-scoping notes into M4 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two of the dated formats already required one -- `## Resolved <... +-hh:mm>` and the completed-item stamp -- while `## Moved` did not. Worth noting that the correct form was present and then drifted: the first heading in this workspace's COMPLETED-CHECKLIST.md is a full `## Moved 2026-09-09 19:00:17 -04:00`, and the four entries after it are bare dates. Framed as a presentation concern rather than a correctness one, because that is what it is. A bare local date is not wrong -- the event happened when it happened -- it is just ambiguous in a way a few characters remove. An offset does not stop a machine with a misconfigured zone from writing a misleading date; it makes that date convertible afterwards, which is the whole of the benefit. The worked example is this session. A laptop reporting -04:00 while its owner sat in -07:00 ran three hours fast, so timestamps taken late in the evening carried the next day's date. Every affected commit stayed unambiguous because git stores the instant with its offset, so 2026-09-13T01:14:28-04:00 is exactly 2026-09-12 22:14 -07:00. One bare date in a checklist was the only thing that had to be re-derived from elsewhere, and it is corrected here. The rule explicitly does NOT ask anyone to go back and repair bare dates already written, or to rewrite history to relabel an offset. Instants are correct either way, and rewriting changes every hash -- on this branch two tracked files cite commit hashes, one of them a split's mandatory `Split from ... at <hash>` provenance line, and both would dangle. An archive heading upgraded in the course of writing this was reverted for the same reason: COMPLETED-CHECKLIST.md is append-only, and a heading that is merely imprecise is not grounds to edit it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… falsified A whole-branch review found no defects in the code and two clusters of stale claims, both written early on this branch and falsified by its own later commits. This is the drift class the surrounding prose describes, landing in the file that is least allowed to carry it. Tier 1 DESIGN-NOTES, section D-encoded-row-is-the-contract: - It said "Nothing here enforces that rule yet" and pointed at CHECKLIST.md for the queued M3.5. M3.5 landed on this branch, so the rule IS enforced -- by every_state_that_blocks_agreement_reaches_the_row and the shared publication_holds predicate -- and CHECKLIST.md no longer contains the item, which is archived. This was the costly one: it instructed the next reader not to assume the instruments cover the rule, when they do. - It said the row "is built today" by interpolating positionally through a concat! template. That template was deleted three commits later; the row goes through Row::with and src/row.rs is the typed writer the sentence described as hypothetical. The only concat! left in topology_report.rs is inside a comment about the old design, verified by grep before saying so. - It counted report_oracle.rs's functions in the present tense. That file no longer extracts text at all. Restated as what the design COST, with no count of the current file -- a count of a live file is a census, and this branch has already corrected several. Cargo.toml: the dev-dependency comment asserted the opposite of what Cargo does. It claimed cfg(test) builds compile report_oracle WITHOUT oracle-in-renderer. They do not: the self dev-dependency is an edge from the package to itself, so under resolver 2 the feature unifies onto the package in any build that includes dev-dependencies. Measured with a compile_error! probe in both directions -- cargo check --tests succeeds under not(feature) and fails under feature, while a plain --lib build has it off. Consequently the matching serde dev-dependency was dead weight and is removed; all 213 tests pass without it. Also repointed src/topology/invariant.rs at COMPLETED-CHECKLIST.md for the M3.2 sabotage evidence, which the archive commit had left pointing at a file that no longer holds the item, and named the sabotage.json entry that now re-runs that evidence instead of asserting it. Two claims written while fixing this were themselves wrong and were caught by checking rather than asserting: the replacement text said report_oracle "is now five functions" (it is six, so the count was dropped rather than corrected), and the concat! claim needed the comment-versus-code distinction above. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…es it A whole-branch review found the module header's central claim false for two push sites. It says "a push site deleted from `cross_check` leaves the state visible here and fires the rule; that is the whole design" -- an unqualified claim, and NUMA was not covered by any rule, so deleting either NUMA push fired nothing here. The exclusion had a rationale, and the rationale was half right. `GetNumaHighestNodeNumber` reports the largest node NUMBER rather than a count, so there is no enumerated quantity to hold it to: nodes 0 and 2 are a valid sparse topology, and comparing it against `numa_domains` would manufacture a violation on hardware reporting itself correctly. That rules out ONE comparison. It does not rule out the comparison `cross_check` actually makes, which is highest-against-highest, and excluding NUMA on the strength of it left the claim overstated rather than the code wrong. Measured, and the measurement is why this is a `fix` and not a `docs`: deleting the NUMA disagreement push was CAUGHT -- by two direct `cross_check` unit tests, not by the invariant layer. So this was never a hole in the suite. It was a hole in the layer whose whole stated purpose is to catch exactly that deletion, which is worse in one specific way: a reader who trusts the header would believe the layer covers a push site it does not. Closed by adding the rule rather than by narrowing the claim. `agree` is reachable only through both NUMA branches declining to fire, so beside `agree` the counter must have been readable and equal -- `AgreedWithoutComparingCounter` reuses the existing variant for the unreadable case, and a new `AgreedDespiteNumaMismatch` carries the optional parse for the mismatch, because folding an `Option<u32>` into the count-shaped variant would have meant inventing a `usize` for an absent parse. Sabotage-verified after the change: the same deletion now reddens `every_numa_branch_in_the_real_cross_check_is_one_this_module_forbids`, which pairs each branch against the real `cross_check` the way the other states were already paired. The sparse-topology test is kept and its comment now says why the two rules differ, since it is the case that distinguishes them. Also narrowed the sabotage control's `why`, which claimed "no test may read the prose" and declared any test catching a prose reword to be the defect. False: about 35 tests assert report text with `contains`, because the human report is a deliverable worth covering. The real rule is that no test may derive a MACHINE-CONTRACT requirement from the wording. As written it would have told a future reader to delete legitimate coverage. Manifest re-swept, 7 of 7 as declared. 216 tests pass on x86_64 and i686. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…they imply First mutation sweep of the two modules M3 created. row.rs came back clean on its first run -- 25 tested, 19 caught, 6 unviable, no survivors -- which review could not have established. topology/invariant.rs gave up three survivors, of two different kinds. **The two that were a real gap: `online_processors > 0` relaxed to `>= 0` in blocking_states.** The guard says a machine WITH processors and no packages is a finding; relaxing it accuses a topology that reported nothing of hiding packages it never claimed. The condition exists twice on purpose. cross_check has its own copy, and blocking_states must recompute rather than read cross_check's output, or it restates verdict() and goes blind to a deleted push site -- the whole reason the module exists. Independence is the design; AGREEMENT is the property, and the property was untested. The test that names this boundary, a_topology_with_no_processors_at_all_is_not_accused_of_hiding_packages, asserts on cross_check, so it covered one copy and left the other open. Five review rounds across four models did not find this; the sweep did, because a reviewer reasons about what code claims and a sweep asks what nothing notices. Closed by holding the two copies against each other over a corpus that spans the boundary, rather than by pinning one case: a guard whose condition drifts on either side is now caught wherever it drifts. A companion test asserts the corpus reaches BOTH answers, since a correspondence over shapes that all land on one side passes vacuously. **The third is equivalent and no test can kill it: `assert_holds` -> `()`.** It derives the verdict from observation.cross_check(), and check()'s rules hold for every such pair by construction -- they are reachable by code change, not by data, which is exactly why check() takes the verdict as a PARAMETER so a test can supply what a broken cross_check would give. assert_holds has no such seam, so nothing distinguishes it from (). The argument is now recorded at the function rather than left for the next sweep to rediscover, matching the assert_corresponds precedent from PR #88. Deliberately NOT added to sabotage.json: a `survives` entry means "this change is not a defect", and removing an assertion IS a defect -- one that happens to be undetectable. Labelling it `survives` would assert the opposite of the truth. The processor guard IS added to the manifest, so the pairing is re-run in two minutes rather than rediscovered in a three-minute sweep. 8 of 8 behave as declared. Re-swept after the fix: 23 caught, 1 missed, and the one is the equivalent mutant above. Sweep results recorded in DESIGN-RATIONALE.md, which claimed three sweeps and now states five. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A whole-branch review found the row's own payloads unchecked, and a sweep of the module put a number on it: 26 mutants, 9 caught, 12 MISSED. 46%. This is the field-labelling defect row.rs exists to make unrepresentable, reappearing one level down. row.rs pairs a name with its value so position cannot mislabel them, and MEASURED_ROW_KEYS closes the top-level key set. Then published() hand-pairs names with values INSIDE each entry, and nothing was watching. What that left open, measured rather than reasoned: - Rewriting the count helper to `*value * 7 + 1` -- every count in every parse_incomplete entry wrong -- left 218 tests passing. Reproduced before touching anything. - NotCompared::code could be replaced wholesale with "" or "xyzzy". Its own doc says "Changing one of these is a breaking change to the NDJSON row"; not one of the six was pinned anywhere. - Every arm of published_anomaly was deletable, losing source labelling and every payload field. - Two arms of anomaly_code were deletable, so a real buffer overrun would publish as `unclassified` -- whose documented meaning is "this probe's vocabulary is older than the crate". A survey would mis-attribute it rather than see it. The tests that LOOKED like they covered this could not. They built their expectation from code() and compared it against a row the writer had built from code(), so both sides moved together -- the tautology class this branch deletes elsewhere, in the one place it mattered most. Closed with goldens, written as literals on purpose. A predicate over values is defined once and asked, never restated; a code and a field name are not predicates but a SCHEMA, and a schema is not derivable from the thing that emits it. Writing it twice is how a golden works. Completeness is compiler-checked where the enum belongs to this crate: the expectation is an exhaustive match, so a variant added without a golden does not compile. AnomalyKind and Source are #[non_exhaustive] upstream, so those use explicit instances plus a direct assertion that a named kind does NOT fall through to the catch-all. Sabotage-verified: the count rewrite that left 218 green now reddens. Re-swept: 12 missed becomes 2. The two that remain are both Display impls -- the PROSE rendering, which under d-encoded-row-is-the-contract is a review obligation rather than a machine-checked one. Left open deliberately and recorded as such, not closed in passing: whether prose deserves machine coverage is a decision about where that line sits. Also corrected DESIGN-RATIONALE.md, which said five sweeps covering "the two modules M3 created". M3 created THREE, and the unswept one was the largest at 629 lines and the only one with survivors. My own claim from commit 6f57dd5, wrong within the hour -- the same class as b54c3a0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nothing The last two mutation survivors were both `Display` impls. Blanking either left the suite green, and a reader would have got ` - ` with nothing after the dash. Both obvious answers were wrong. Pinning the sentences would make the prose machine-checked, which d-encoded-row-is-the-contract deliberately does not do -- the row is the machine contract and the prose is a review obligation. Leaving them ships a blank line. The engineer named a third: a blank is perhaps wrong, perhaps it should be called out. That is better than either, because it separates two things the survivors had conflated. **A diagnostic's WORDING is a review obligation; its PRESENCE is not.** Presence is structural -- the renderer knows it holds an entry to print -- so it can be machine-checked without any sentence being pinned. So the renderer routes every diagnostic through `described`, which substitutes an explicit BUG IN THIS PROBE line for a rendering that is empty or blank, and a test asserts no diagnostic renders as that line. That pins THAT each entry describes itself without pinning WHAT it says. A blank was the worst available answer on its own terms, separately from the sweep: it is indistinguishable from a rendering bug, from a finding with genuinely nothing to say, and from a stray newline, so a reader cannot tell whether the probe found something it failed to describe. Both improvements fall out of the one branch. The reader gets a stated defect instead of a blank, and the mutants become catchable, because blanking a Display now produces the callout the test forbids. Verified: diagnostic.rs swept to ZERO survivors, from 12 two commits ago and 2 before this one -- including the two mutants `described` itself introduced, so the fix carries its own coverage. Whitespace is treated as blank, since a reader cannot tell it from emptiness on the page, and a test pins both. The sabotage manifest's prose control STILL survives, which is the check that matters here: `described` did not quietly move the line by making wording machine-checked. 8 of 8 behave as declared. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nderer, and unbreak the default doc build Three findings from the review of 40ea4f3, two of them mine from that commit. **`published_codes` discarded every payload field**, so the only end-to-end renderer test would have stayed green if `topology_report` dropped or reshaped `parsed`, `counter`, `level` or an anomaly's metadata. The per-variant tests cover `published()` in isolation; nothing covered it THROUGH the renderer. `the_row_carries_each_diagnostic_entry_whole_and_not_only_its_code` now compares each rendered entry, for all four diagnostic lists, across every corpus shape. The expectation is derived from `published()` rather than written out, so it cannot drift from the publisher; what it pins is that the renderer carries that value through unchanged. **The first sabotage of it was wrong, and the correction is the useful part.** Deleting `offset` from `published_anomaly` left the new test green -- correctly, because both sides derive from that function, so sabotaging the PUBLISHER moves them together. The defect the reviewer described is in the RENDERER, and sabotaging there -- mapping the anomalies back to bare codes, the superseded M3.1 form -- reddens it along with four neighbours. A test that derives its expectation can only be sabotage-verified at the layer it actually constrains. **The `RowDefect::RepeatedKey` intra-doc link broke `cargo doc`.** `row` is always built, `report_oracle` is compiled only under `cfg(any(test, feature = "oracle-in-renderer"))`, so the link cannot resolve in a default documentation build and emitted a broken-intra-doc-link warning. Demoted to plain code text, with the reason recorded beside it. Confirmed by running `cargo doc -p windows-platform-probes --lib`, which is now clean. That is the same feature-gating trap as b68a124 in a third guise -- module, statement, and now doc link. Everything local compiles with the feature ON because of this crate's dev-dependency on itself, so each variant has to be caught by a build that does not. **The design-session key list** read as a current-schema list missing `disagreements`, which was a fair reading of how d523f2b introduced it: a sentence pointing at `MEASURED_ROW_KEYS` as the authority, followed by a colon and the historical eighteen. The framing now says plainly that the list is the eighteen as they stood at that session and is not reproduced as current. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
One critical publication invariant issue and three moderate row-contract issues remain, along with a checklist nit.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
crates/windows-platform-probes/src/row.rs:241
- This intra-doc link points at
report_oracle, but that module is compiled only undertestororacle-in-renderer; a normal/default rustdoc build therefore has nocrate::report_oracle::RowDefectto resolve and reports a broken link. Keep the type name as an unlinked code reference, or make the referenced API available in default documentation; the analogous isolated default-doc jobs in.github/workflows/ci.ymlshow this configuration is intentionally checked.
crates/windows-platform-probes/CHECKLIST.md:13
- The milestone preamble says every carried item has its own post-M3 re-scoping note, but M2.5 still carries the old instruction to put this correspondence in the deleted M2.1 oracle (rather than in the surviving observation/renderer design). Re-scope that item before marking the milestone ready; otherwise an executor following this checklist is sent to a component M3 explicitly removed.
These were written under M2 and were blocked on M3, which is **now complete and archived** in
[COMPLETED-CHECKLIST.md](COMPLETED-CHECKLIST.md). They are unblocked, and each was RE-SCOPED rather
than merely delayed -- so each item below carries its own re-scoping note, in the item, where
somebody executing the list will actually meet it.
crates/windows-platform-probes/src/row.rs:171
Value::Objectis public and documents an ordered set of members, but unlikeRow::withit does not reject duplicate names. A caller can therefore construct a nested object with the same key twice; the writer emits ambiguous JSON, while the oracle's duplicate-key check only examines top-level members. Reject repeated names in this loop so every object in the machine contract has the same uniqueness guarantee.
for (at, (name, value)) in members.iter().enumerate() {
if at > 0 {
out.push(',');
}
write_escaped(out, name);
crates/windows-platform-probes/src/row.rs:82
- These shapes validate only the outer list and, for diagnostics, the presence of a string
code; they do not validate the member fields that are part of the row contract. Consequently, droppingdomainsfrom a cache object or droppingcount/level/other payloads from a diagnostic object would still pass the renderer's shape assertion, while the standalone publisher goldens do not exercise the renderer boundary. Add recursive member schemas or an independent rendered-row check for the exact payload fields before treating this as the complete machine contract.
/// by, so an entry without one is unmineable even though it is valid JSON.
ListOfCoded,
/// An object whose every member is a number.
- Files reviewed: 23/25 changed files
- Comments generated: 1
- Review effort level: Lite
Owner
Author
Round 9 disposition: the five suppressed commentsContinuing the ledger. All five addressed -- three were already fixed in b68a124 before this review landed, two were new and are fixed in 399cc39. The two inline comments are answered and resolved in their threads. Already fixed when the review arrivedBoth New, and both mineThe That is the same feature-gating trap in a third guise: module, statement, and now doc link. Everything local compiles with the feature ON because this crate dev-depends on itself, so each variant has to be caught by a build that does not.
My first sabotage of it was wrong, and that is the useful part. Deleting The general lesson, now in the commit message: a test that derives its expectation can only be sabotage-verified at the layer it actually constrains. Picking the wrong layer produces a false all-clear that looks exactly like a real one. Design-session key listFair reading, and my fault. d523f2b introduced a sentence pointing at Unrelated: a CI flake, diagnosed not patched
|
… and a row key A review read `codes_for(State::EnumerationAnomalies)` as naming the row KEY `enumeration_anomalies`, concluded that `publication_holds` could not find it because `DIAGNOSTIC_LISTS` deliberately excludes that list, and reported the assertion as failing for the `anomalies` corpus shape. It does not fail. The string is a condition CODE carried inside `parse_incomplete`: `cross_check` pushes `ParseIncomplete::EnumerationAnomalies` when the observation records any, and that variant's code is this same word. So `publication_holds` finds it while scanning the three diagnostic lists, and `every_state_that_blocks_agreement_reaches_the_row` exercises exactly that shape -- all 11 tests in the target are green, before and after this commit. The collision is real even though the conclusion was not: one word names both a row key and a condition code, and a careful reader took it for the wrong one. Noted at the arm, since the next reader has the same chance of being misled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Nested duplicate-key enforcement and complete nested cache schema validation remain unresolved.
Review details
Suppressed comments (2)
crates/windows-platform-probes/src/row.rs:107
Value::Objectis public and is documented as a set, but it accepts duplicate names andwriteemits both of them.Row::withonly rejects duplicate top-level members; a caller can still render a nested object such as{"code":"a","code":"b"}, which JSON parsers accept while silently retaining one value, so the duplicate-key integrity guarantee does not cover the public value API. Enforce uniqueness for every object (or hide this construction behind a checked constructor) before treating the row as unambiguous.
/// An ordered list.
List(Vec<Value>),
/// An ordered set of named members.
Object(Vec<(&'static str, Value)>),
crates/windows-platform-probes/src/topology_report.rs:84
ListOfObjectsonly checks that eachcacheselement is an object, so[{}]or[{"level":"L1","domains":[]}]satisfiesMEASURED_ROW_SHAPESeven though the renderer's cache contract publishes numericlevelanddomainsmembers (topology_report.rs:819-822). There is no independent topology-report assertion for those nested fields, so a regression can change the mined cache payload while the row-shape oracle stays green; add a nested-object schema or a golden that rejects missing/wrongly typed members.
"caches" => Shape::ListOfObjects,
- Files reviewed: 23/25 changed files
- Comments generated: 0 new
- Review effort level: Lite
…not just the first
Two gaps a review found in last round's work, both measured before being fixed
and both the same shape of defect: a guarantee stated for the ARTIFACT and
enforced only for its outermost level.
**Duplicate keys were rejected at the top level only.** `Row::with` refused a
repeat among the row's own members, so a nested entry could still render
`{"code":"a","code":"b"}` -- measured, exactly that string -- which
`serde_json` and `JSON.parse` both accept while silently keeping one value. The
oracle could not report it either, because `keys` reads top-level names by
design. `Row::with` now walks the whole value tree.
Uniqueness is per OBJECT, not across the row: every diagnostic entry carries
its own `code`, and a rule forbidding that would reject every real report. A
sibling-objects control pins that, so the check cannot be tightened into
nonsense later.
**`caches` promised only "a list of objects", so `[{}]` satisfied it.** The
renderer publishes numeric `level` and `domains`; nothing checked either.
Measured: an empty cache object produced zero violations. (`[{}]` is also,
exactly, the bogus shape a design session claimed the row emitted and which was
corrected earlier on this branch. It was never a real rendering -- but it was
reachable through the checker.)
`Shape::ListOfObjectsWith(&[(name, shape)])` replaces both `ListOfObjects` and
`ListOfCoded`, which generalises rather than adding a special case: the coded
lists become `ListOfObjectsWith(CODED)` and `caches` names its two numeric
members. Extra members stay legal -- what a schema owes a consumer is that the
fields it PROMISES are present and typed, and forbidding additions would make
every new field a breaking change to the checker rather than to the contract.
The checker is now recursive and reports WHERE rather than only THAT:
`caches[0].level should be Number but is "2"` instead of one sentence about the
whole list.
Sabotage-verified, each against its own claim:
- A nested duplicate now panics in the writer, naming the member; the
sibling control still renders both `code`s.
- Publishing `cache.level` through `.to_string()` fails 19 library tests
naming `caches[0].level`, against 0 before.
The `caches` test takes its schema FROM `MEASURED_ROW_SHAPES` by filtering for
the key, rather than restating the shape beside it. Written out, it would have
pinned a shape of its own and passed while the declared schema said something
else -- the copy checking the copy, which is the defect this branch keeps
finding. The first draft did exactly that, with a doc comment claiming it was
derived.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
Round 10 disposition: both suppressed commentsBoth fixed in 0c4aced. Both were genuine gaps in last round's work, and both are the same shape of defect: a guarantee stated for the artifact and enforced only for its outermost level.
|
| sabotage | before | after |
|---|---|---|
| nested duplicate key | rendered silently | writer panics, naming the member |
cache.level via .to_string() |
0 failures | 19 library tests, naming caches[0].level |
One note against myself: the caches test now takes its schema from MEASURED_ROW_SHAPES by filtering for the key. My first draft hand-wrote the shape while its doc comment claimed it was derived -- the copy checking the copy, which is the exact defect this branch keeps finding. Caught it on re-reading before commit.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate findings remain in nested schema validation and IoRing handover timing coverage.
Review details
Suppressed comments (6)
Previously missed (1) — in code that hasn't changed since the last review.
crates/windows-ioring-sys/tests/handover.rs:540
attached.elapsed()is evaluated beforedrain_to_empty, soto_polledis actually attach-to-before-poll (nearly zero), while the failure text claims attach-to-polled. This removes the timing evidence the new escalation diagnostics are meant to provide; captureattached.elapsed()immediately after the non-blocking drain and store that value.
crates/windows-ioring-sys/tests/handover.rs:121
- This ordinary test can issue up to
4 * (8 + 16 + 32 + 64)unbuffered 1 MiB reads before failing, and it hard-fails when the host completes every read beforecompletion_event()attaches. That outcome is a device/scheduling property, not evidence that handover strands anything, so a fast legitimate device can make the normal suite slow and red. Move this timing-dependent reachability probe to an explicitly run/ignored test or make the fixture deterministically hold an I/O in flight.
const DIRECT_WIDTHS: [usize; 4] = [8, 16, 32, 64];
crates/windows-platform-probes/COMPLETED-CHECKLIST.md:500
- This newly added archive heading uses only a bare date, but the same PR adds the repository rule that newly written dates must include a time and UTC offset. Please record the actual completion timestamp here with its offset so this archive entry follows the rule it introduces.
## Moved 2026-09-12 -- M2 completes: the report oracle, its fact set and its shape corpus
crates/windows-platform-probes/COMPLETED-CHECKLIST.md:527
- This newly added M3 archive heading also omits the time and UTC offset required by the timestamp rule added in this PR. Unlike the M2 entry, there is no nearby timestamp that can be reused; please record the actual completion timestamp here rather than leaving a bare local date.
## Moved 2026-09-12 -- M3: the encoded row became the contract, and the prose stopped being checked
crates/windows-platform-probes/src/row.rs:81
- These shapes validate only the outer container and, for diagnostics, that a string
codeexists. The renderer-bound schema therefore accepts{}or{"level":"1"}as a cache entry and accepts a diagnostic object with all variant payloads omitted;ObjectOfNumberslikewise cannot detect missing or extra policy fields. The new row contract does not enforce the nested data the renderer promises to publish; add nested schemas or explicitly document that those members are unconstrained.
///
/// **Recursive, because "a list of objects" was not a contract.** That is
/// what this replaced, and `[{}]` satisfied it -- so `caches` could stop
/// publishing `level` and `domains` with the shape oracle green, and a
/// diagnostic entry could lose the `code` a survey groups by. Reported by a
crates/windows-platform-probes/src/topology_report.rs:84
Shape::ListOfObjectsonly validates the outer array/object shell. The renderer treats each cache object as{"level": number, "domains": number}(lines 819-822), but this schema still passes if either member is deleted or changed to a string. The current cache tests only assert the writer's sample output, so a renderer regression here can reach the row unchecked; add a nested cache-entry shape and exercise it through the renderer.
"reason" => Shape::Text,
- Files reviewed: 23/25 changed files
- Comments generated: 0 new
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
A moderate compile-blocking issue and two documentation nits remain unresolved.
Review details
Suppressed comments (3)
crates/windows-platform-probes/DESIGN-NOTES.md:1033
- This retired decision is explicitly superseded by the encoded-row decision, but its status is only stated below the preserved anchors as “Moved to Tier 2.” Put a
**Superseded by [...]**marker directly beneath the heading (before the anchors or other prose) so readers cannot mistake this historical section for a current decision.
**Moved to Tier 2: [DESIGN-RATIONALE.md](DESIGN-RATIONALE.md).** The anchors
crates/windows-platform-probes/DESIGN-NOTES.md:1177
- This inline-code file reference is not clickable, unlike the linked reference to the same file three lines later. Use a relative Markdown link here as well so readers can open the historical source directly from the design note.
Counted in `src/report_oracle.rs` **as it stood before this decision**: of 38
crates/windows-platform-probes/src/topology/invariant.rs:310
Observation::coherenceis a non-CopyCoherencevalue, so matchingobservation.coherencethrough&Observationattempts to move it out of a borrowed struct and fails to compile. Match by reference here, asCrossCheckalready does forself.coherence.
match observation.coherence {
- Files reviewed: 23/25 changed files
- Comments generated: 0 new
- Review effort level: Lite
…ts it
Two documentation findings, both correct against the repository's own rules,
and one rejection.
**The retired correspondence-oracle decision stated its supersedence twelve
lines into the body.** The rule is explicit that the marker goes adjacent to
the title "even when the fuller explanation already appears later in the
decision's body", precisely so a reader cannot take a superseded section for a
current one by reading the top of it. Added beneath the anchors, matching the
shape the buffered-report decision already uses, and the fuller explanation
lower down is left as it was.
Swept rather than fixing the reported site alone: this file has exactly two
superseded decisions and both now carry an adjacent marker.
**An inline-code file reference was not clickable**, three lines above a linked
reference to the same file. Linked. Swept: no unlinked `src/...` references
remain in the file.
**Rejected: `match observation.coherence` "fails to compile".** It does not,
and this is the third round reporting this shape -- twice against
`diagnostic.rs`, now against `invariant.rs`. Matching a place expression behind
a shared reference is a move only when a PATTERN BINDING moves a non-`Copy`
value; all three arms here are unit-like or `{ .. }` and bind nothing.
`cargo check --all-targets` is clean across the workspace and has been on every
commit these lines have existed.
Left as it is rather than adding a no-op `&` to satisfy the claim -- but a note
now sits at the match, because three rounds spent on one false positive is
itself evidence the code should say why it is correct.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
Round 11 disposition: the three suppressed commentsTwo fixed in c1927a2, one rejected with evidence. Supersedence marker -- fixedCorrect, and against this repository's own rule, which says the marker goes adjacent to the title "even when the fuller explanation already appears later in the decision's body" -- precisely so a reader cannot take a superseded section for a current one by reading the top of it. The explanation was twelve lines down. Added beneath the anchors, matching the shape the buffered-report decision already uses rather than introducing a second convention. Swept: this file has exactly two superseded decisions, and both now carry an adjacent marker. All three anchors verified still present. Unlinked file reference -- fixedCorrect, and slightly embarrassing: it sat three lines above a linked reference to the same file. Linked. Swept: no unlinked
|
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate findings and documentation/comment nits remain.
Review details
Suppressed comments (8)
crates/windows-ioring-sys/tests/handover.rs:542
already_queuedis measured only aftercompletion_event()has returned, anddrain_to_emptyitself runs before the comparison. A read can therefore finish between the attach and this poll, soalready_queued < widthdoes not prove that any read was outstanding when the event was attached. The test can setcaught_in_flightand pass its final guard while exercising only the already-completed/backlog path; establish the pending count immediately before the attach (or otherwise synchronize the precondition with the attach boundary).
if already_queued < width {
caught_in_flight = true;
crates/windows-platform-probes/CHECKLIST.md:31
- These newly added historical dates omit the time and offset, even though this PR adds the repository rule that dates written to tracked documents must be UTC or offset-bearing. Please use the actual offset-bearing timestamp, or describe this as a relative milestone event without a date.
above was rewritten on 2026-09-13 to match**: it still asked for "the same correspondence
crates/windows-platform-probes/CHECKLIST.md:55
- These newly added historical dates omit the time and offset, even though this PR adds the repository rule that dates written to tracked documents must be UTC or offset-bearing. Please use the actual offset-bearing timestamp, or describe this as a relative milestone event without a date.
`every_numa_counter_branch_...` were narrowed on 2026-09-13 to stop overstating the coverage,
crates/windows-platform-probes/CHECKLIST.md:237
- These newly added completion links use bare local dates, despite the timestamp rule added by this PR requiring UTC or an explicit offset for dates written to repository documents. Please update each completion stamp with its actual offset-bearing timestamp before merging.
- [x] **M2.14** -- Make the two authoring rules this branch earned actually bite. -> [completed 2026-09-13](COMPLETED-CHECKLIST.md#m214)
- [x] **M2.14.1** -- Give this crate a sabotage manifest, so "observed to fail" is a recorded artifact rather than a habit. -> [completed 2026-09-13](COMPLETED-CHECKLIST.md#m2141)
- [x] **M2.14.2** -- Add to CONTRACT INTEGRITY rule 1 the one thing this branch learned that it does NOT already say. -> [completed 2026-09-13](COMPLETED-CHECKLIST.md#m2142)
crates/windows-platform-probes/CHECKLIST.md:73
- This new M4 placement says the item is unblocked by M3.1/M3.3, but its acceptance text below still requires expressing the relation in the deleted M2.1 prose oracle and says the work belongs to M2. M3 made the encoded row the contract, so the item is not actually re-scoped as the milestone header claims; update its acceptance criteria to the current observation/row design before it is executed.
Gated by M3.1 and M3.3, both landed: establishing that the middle of three discoveries agreed
produces a new FACT, which M3.1 says must reach the row rather than only the banner, and M3.3
changed how the banner is built. Written before those, it would have been written into machinery
crates/windows-platform-probes/COMPLETED-CHECKLIST.md:527
- This newly archived
Movedheading uses a bare date, unlike the adjacent timestamped archive entries. Include the UTC time or an explicit offset in new archive headings so the recorded completion instant is unambiguous.
## Moved 2026-09-12 -- M3: the encoded row became the contract, and the prose stopped being checked
crates/windows-platform-probes/src/report_oracle.rs:235
keysinvokesdeserialize_mapbut never callsDeserializer::end().serde_jsonstops after the first value, sokeys(r#"{\"reason\":\"x\"}garbage"#)returns the object's keys even thoughcheck/malformationreject the same row. This makes the public key reader accept a malformed artifact and lets callers disagree about whether the row is readable; finish the deserializer and reject trailing tokens.
let mut reader = serde_json::Deserializer::from_str(row);
serde::Deserializer::deserialize_map(&mut reader, TopLevelNames).unwrap_or_default()
crates/windows-platform-probes/src/topology.rs:618
- The newly added
MeasuredButCountsAbsententry makes the preceding explanation stale: a zero counter is now added toparse_incomplete, andparse_in_doubt()is true for any non-emptyparse_incomplete. As written, the comment incorrectly says this case is onlynot_comparedand leavesparse_in_doubtfalse, which can mislead future changes to this guard.
if self.topology_was_measured && !absent.is_empty() {
check
.parse_incomplete
.push(ParseIncomplete::MeasuredButCountsAbsent { absent });
- Files reviewed: 23/25 changed files
- Comments generated: 0 new
- Review effort level: Lite
…eview read it backwards `already_queued < width` sets `caught_in_flight`, and a review argued it does not prove a read was outstanding at the attach: a read finishing between the attach and the poll would, on that reading, let the test pass while exercising only the already-completed path. The direction is what settles it, and it runs the other way. `drain_to_empty` loops until `try_pop` reports the queue EMPTY -- no cap, no early exit -- so `already_queued` is the total observed at a moment strictly after the attach. A read finishing in that window is therefore COUNTED, which pushes `already_queued` toward `width` and makes the branch LESS likely to be taken. The already-completed case the finding warns about is exactly the case where all `width` are drained and the flag is never set. So the error this can make is a false NEGATIVE, never a false positive. And completion is monotonic: a read still outstanding at the later poll was outstanding at the earlier attach. The escalation over widths and attempts, and the final assertion, exist for the false negatives. The one real assumption is that `try_pop` reports emptiness truthfully -- a ring claiming empty while holding completions would forge this precondition. That is a defect in the crate under test rather than in the test, and `contract.assert_quiescent()` is what would catch it. Written down because it is the only way this argument fails. Comment only; no behaviour change. Typed `chore` because `tools/check-commit-scope.ps1` flagged it riding along with unrelated `windows-platform-probes` work. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`keys` called `deserialize_map` and never `Deserializer::end()`. `serde_json`
stops at the end of the first value and does not care what follows, so two
public readers disagreed about the same artifact. Measured before fixing:
keys(r#"{"reason":"x-probe-topology"}garbage"#) -> ["reason"]
check(...) -> Malformed { what: "trailing characters at line 1 column 30" }
A caller reading keys directly was told a malformed row was readable. Reported
by a review. `keys` now returns nothing when tokens follow the object, with a
control asserting the same row WITHOUT the garbage still reads normally.
The sabotage manifest anchored on the two lines this changed, so its entry was
rewritten and re-verified: all eight anchors resolve, and entry 3 still
compiles and is CAUGHT (nine tests fail) rather than becoming unviable. The
explanatory comment moved above the block so the anchor stays pure code -- a
substitution that has to skip a comment is one that silently stops matching
when the comment is edited.
**A stale motivation in `cross_check`, written in the present tense.** The
paragraph over the `MeasuredButCountsAbsent` push said a zero parse beside a
failed read "is filed as `not_compared` -- leaving `parse_in_doubt` false". That
described the gap the push CLOSES, sitting directly above the push that closes
it, so it read as current behaviour. Put in the past tense, with the effect on
`parse_in_doubt` stated.
**M2.5's acceptance criteria still named the deleted M2.1 prose oracle**, the
same defect M2.4 carried and the same fix: the relation belongs in M3.2's
invariant set and the row schema, checked through the renderer binding. Swept --
no remaining item ASKS for the retired machinery.
Two newly added prose dates lost their bare form in favour of relative wording
("when M3 was archived", "in the same review round that reported this"), which
is both offset-free and more durable than either date form.
Two further findings were REJECTED. The stub links `[completed 2026-09-13]` use
a bare date because that IS the stub format the repository rules specify; and
`COMPLETED-CHECKLIST.md`'s `## Moved 2026-09-12` heading predates the timestamp
rule, which says in terms that existing headings "are fine and are not worth
going back to change" -- and editing it would breach that file's append-only
invariant. The same heading was raised and answered two rounds ago.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
Round 12 disposition: the eight suppressed commentsFour fixed, four rejected. Every one verified by running it, not by reading. The in-flight precondition -- REJECTED, and the direction is what settles itThis was flagged to me as the important one, so I worked it through carefully. The concern: a read finishing between the attach and the poll lets the test pass while exercising only the already-completed path. It runs the other way.
So the error this can make is a false negative, never a false positive -- and completion is monotonic, so a read still outstanding at the later poll was outstanding at the earlier attach. The escalation over widths and attempts, and the final assertion, exist precisely for the false negatives. The one genuine assumption is that
|
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
A moderate test failure remains, and the broad contract and invariant changes warrant final human review.
Review details
Suppressed comments (1)
crates/windows-platform-probes/tests/a_real_report_agrees_with_itself.rs:484
serde_json::Valueuses its default sorted map representation here, soToString::to_string()reorders object members before this comparison. For example,Disagreement::published()renderscode, parsed, counter, while the parsed value is serialized ascode, counter, parsed; the disagreement shapes inshapes()therefore make this test fail even when the payload is correct. Compare parsedValues (or canonicalize both sides through the same serializer) instead of comparing order-sensitive strings.
let parsed: serde_json::Value = serde_json::from_str(row).expect("the row parses");
parsed[key]
.as_array()
.unwrap_or_else(|| panic!("`{key}` should be a list in:\n{row}"))
.iter()
.map(ToString::to_string)
.collect()
- Files reviewed: 23/25 changed files
- Comments generated: 0 new
- Review effort level: Lite
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.
No description provided.