Give doctor a check model and a registry, and stop one throwing check taking the rest - #475
Merged
Conversation
…nting severity
The row was seven fields. No category, so nothing could ever select a
subsystem; no severity, so ordering findings meant re-deriving it wherever
someone needed an order; no evidence slot, so a check stated its conclusion
with the observation interpolated into prose or dropped. `skipped` carried its
reason as free text, which is a skip nothing can act on.
Severity is now a total function of status computed inside the one factory, and
absent from its parameter list -- passing one is a type error rather than a
discouraged habit. Two axes that can disagree make every consumer resolve the
disagreement; deriving at a chokepoint makes the inconsistency unrepresentable.
`skipped` maps to info rather than warning, because a check that could not run
reported nothing, and giving it a warning's weight is how a report starts
ranking its own blind spots above its findings.
Category is a required positional argument rather than a lookup keyed by id.
The lookup was simpler and wrong in a specific way: a check added later would
silently take a default. This makes omitting one fail to compile.
The instrument landed before the change, which is the part worth keeping. It
caught the very first edit -- and the failure was the snapshot's own, not the
report's: mkdtemp's random suffix was unnormalised, so it varied per run. A
snapshot that fails for reasons unrelated to what it guards is deleted within a
week, so the normaliser was fixed rather than the expectation regenerated
blindly.
Migrating the sixty-one call sites took two attempts. The first rewrote
positional calls into object literals, converted fifteen, and raised the error
count from sixty-one to two hundred and forty-two; it was reverted whole rather
than repaired.
Limit: evidence is `{}` on every row until the ticket that populates it, so the field exists and proves nothing yet
Ruled-out: deriving category from a table keyed by check id | it is less code and gives a new check a silent default, which is the failure mode this milestone exists to remove
Ruled-out: mapping `skipped` to `warning` | a check that could not run has reported nothing, and weighting a blind spot above a finding inverts what the report is for
Ruled-out: repairing the object-literal migration that broke | it had converted a quarter of the sites and quadrupled the error count, and a half-applied mechanical rewrite is harder to audit than a fresh one
Warn: `skipReason` is still optional on the skipped overload; until the sites are mapped a bare skip is constructible, and the type will not catch it
Blast: local
Undo: easy
Certainty: firm
Verified: 102 files and 2,254 cases pass; the text snapshot and the check-order snapshot are unchanged across the model change, every v1 JSON key is present with its type on every row, severity agrees with status on every row of a full run, and the two rows that deliberately clear needsAttention still do
Provenance: authored
Record-Id: r-checkmodel
CommitLore — record lintTrailers: clean — 3 commits in Active constraints for the paths this PR touchesLimits (105)
Ruled out (243)
Truncated: 99 lines omitted — the comment hit GitHub's 65000 character limit. Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR. |
runDoctor was a hand-written array of thirteen ad-hoc functions. A throw in any of them killed the command, so the user who most needs a diagnosis -- the one whose repository is in a state some check did not anticipate -- got a stack trace instead of the other twelve answers. Nothing was timed, so §10's budget was an assertion with no instrument, and there was no data structure a filter could ever read. Checks are registry entries now. A throw becomes one failed row carrying the message's first line as evidence, and the rest of the report still renders. Every row is stamped from a monotonic clock, because a wall clock can go backwards and a negative duration is worse than none. One ordering fact drove the design. `commit-msg-hook` consumes `hook-runtime`'s result but presents before it, so the dependency runs backwards against the report order and cannot be satisfied by running earlier entries and reading their output. Memoising the computation keeps "each check runs exactly once" true without reordering anything a user reads. That edge is deliberately not declared as a dependency: §2 admits only earlier entries, and declaring a backward edge to make the data look complete would put a lie in the structure the fix plan will later walk. Effect injection is not here. §2.3 names five effects and only the clock is injected; threading the rest through thirteen checks touches all of them, and a half-injected context is worse than none because a test that stubs it would then exercise an arbitrary subset. Split to #476 with that reasoning. Limit: the registry is data but nothing filters it yet, so the ordering guarantees are tested and unused until the --only ticket Ruled-out: declaring commit-msg-hook -> hook-runtime now | the edge points backwards against report order, and a declared dependency that the emission order cannot satisfy is a lie in the structure the fix plan walks Ruled-out: injecting git and spawn alongside the clock in this change | it rewrites every check while the text must stay byte-identical, and partial injection makes a stubbed-context test cover an arbitrary subset Ruled-out: letting a throwing check abort the report | the twelve remaining answers are worth more than the one missing, and the containment row says which subsystem went unreported rather than implying it passed Warn: containment turns a crash into a `fail`, so a check that throws on every run now reports a steady failure rather than an obvious one -- the evidence line is the only thing distinguishing them Blast: local Undo: easy Certainty: firm Verified: 102 files and 2,259 cases pass with the text and check-order snapshots unchanged; ids are unique and kebab-case, every category is populated, every declared dependency resolves to an earlier entry, every row carries a whole non-negative durationMs, and a check forced to throw yields one fail row with its first message line while the other twelve still render Provenance: authored Record-Id: r-registry463
CI disagreed with the local run on two rows, and the disagreement was the interpreter's path: nvm under a home directory here, hostedtoolcache under /opt on the runner. The report was identical; the snapshot was a record of where it was first generated. This is the second time this instrument has failed for a reason that has nothing to do with what it guards -- the first was mkdtemp's random suffix. Both are the same mistake in different clothes: normalising the values a reader cares about and leaving in the ones that vary per machine. A snapshot that cries wolf gets deleted, and this one has to survive twelve more tickets. Limit: normalisation is a list of known-variable things, so the next machine-specific value to appear in a detail string will fail once before it is added Ruled-out: dropping the two rows that mention the interpreter | they carry the commitlore.bin and commitlore.node pairing, which is exactly what #382 and #71 were about Warn: the node path is replaced by exact match, so a detail string that renders the interpreter differently -- relative, or through a symlink the check did not resolve -- would slip past and reintroduce the drift Blast: local Undo: easy Certainty: firm Verified: the regenerated snapshot contains no home directory, no nvm path and no hostedtoolcache path; thirteen cases pass locally Provenance: authored Record-Id: r-snapnode
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.
Closes #462 and #463. Tickets two and three of Doctor · Diagnostic Rigor. No observable behaviour change — text byte-identical,
--jsongains keys only.#462 — the row was seven fields
No category, so nothing could select a subsystem. No severity, so ordering findings meant re-deriving it wherever someone needed an order. No evidence slot.
skippedcarried its reason as free text — a skip nothing can act on.Severity cannot be passed. It is a total function of
statuscomputed inside the one factory and absent from its parameter list, so supplying one is a type error rather than a discouraged habit (ADR-0032 §3).skipped → info, notwarning: a check that could not run has reported nothing, and giving it a warning's weight is how a report starts ranking its own blind spots above its findings.Category is positional and required. A lookup keyed by id was simpler and wrong in a specific way — a check added later would silently take a default.
#463 — one throw killed the command
A throw in any of thirteen ad-hoc functions ended the run, so the user who most needs a diagnosis got a stack trace instead of the other twelve answers.
Every row is stamped from a monotonic clock — a wall clock can go backwards and a negative duration is worse than none.
The ordering fact that drove the design
commit-msg-hookconsumeshook-runtime's result but presents before it. The dependency runs backwards against report order and cannot be satisfied by running earlier entries and reading their output. Memoising keeps "each check runs exactly once" true without reordering anything a user reads.That edge is deliberately not declared: §2 admits only earlier entries, and declaring a backward edge to make the data look complete would put a lie into the structure the fix plan will later walk.
Scope reduced honestly
PRD §2.3 names five injectable effects; only the clock is injected. Threading the rest through thirteen checks touches all of them while the text must stay byte-identical, and a half-injected context is worse than none — a test that stubs it would exercise an arbitrary subset. Split to #476 with that reasoning.
The instrument caught its own defect first
The text snapshot landed before the model change and failed immediately — and the failure was the snapshot's own:
mkdtemp's random suffix was unnormalised. The report was identical. A snapshot that fails for reasons unrelated to what it guards is deleted within a week, so the normaliser was fixed rather than the expectation regenerated blindly.The first call-site migration attempt converted 15 of 61 sites and raised the error count from 61 to 242. Reverted whole rather than repaired.
Verification
102 files, 2,259 cases pass, text and check-order snapshots unchanged. Ids unique and kebab-case, every category populated, every declared dependency earlier, every row carrying a whole non-negative
durationMs, and a check forced to throw yielding onefailrow while the other twelve render.