fix: remediate every finding from the QA audit - #16
Merged
Conversation
The summary is the one artifact that leaves this tool and lands in somebody else's ticket, so it has to describe the session that ran. It named the target and the zone by reading them LIVE, and both are reachable between the run and the copy: the form unlocks the moment a session ends, while the summary stays copyable for every status but Idle and Connecting. A tester lining up the next run and then copying the last one got one session's verdict under another session's target name and zone - evidence pointing at the wrong application, which is worse than no evidence. The start snapshot already existed for the moment and the mode, because those two can change IN FLIGHT. The target and the zone cannot, which is why they were read live - and why the window that does exist was missed. They join the snapshot now, and four private readers (RequestedTargetPath, RequestedZone, RequestedMoment, RequestedMode) are what every report goes through, so a new one cannot pick the live value by accident. The diagnostics block and the history record are built inside the Start finally, before the form unlocks, so reading live was correct there today. They read the snapshot anyway: neither should depend on WHEN it is called. Two tests, each seen red with its half of the fix reverted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five inputs decide what a run does and none of them was recorded: the target's arguments, its working folder, and the three switches (scale-duration, scale-QPC, force). Repeating a session filled four fields and left the other five holding whatever the form had, so Start produced a third setup that was never run and never recorded, and nothing said so (rule 6). Scale-QPC is the expensive one - it decides whether the target's elapsed counters move at all. The five are additive and optional, so the schema stays 1: a file written before they existed still loads with the defaults, and a file written now still loads in a build that predates them. A zone or a speed the catalogues no longer offer still cannot be filled in - that part is unavoidable, since both are closed lists. What is fixed is the silence: the form used to keep the CURRENT value and claim to be the recorded session. It now says which of the two it could not fill, so the reader knows which box to set by hand, and everything that did load stays loaded. Two notes on shape, both load-bearing. The five snapshots are plain strings and bools rather than a setup type, and the note is nested ifs rather than a switch on a tuple, because this class sits exactly on CA1506 = 82 - the tuple version reddened the metrics gate, measured, not guessed. Correction to my own audit: the "session at a custom speed reads back as flow" case I reported is NOT reachable. The custom-speed box lives in a panel bound to IsRunning, so a session can only ever start on one of the five catalogue modes. What is real is a record that predates a change to either list. Four tests, each seen red with its half of the fix reverted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Load promises not to delete a broken history and keeps that promise. Append broke it one step later: it built its new list from Load's empty answer and wrote over the file, so the first session recorded after a downgrade - a newer build's schema this one will not read - wiped the whole log. The schema gate stopped this build from MISREADING the file and did nothing to stop it destroying it. Load now answers through a reader that also says whether there IS a file here it could not read. Load itself drops that half, because a reader has no use for it. A writer does: an unreadable file is still somebody's history, and overwriting it is a deletion however quietly it happens. Append sets such a file aside under a dated name and starts a fresh log, so the tester keeps their records, can see where they went, and still gets this session recorded. A move, not a copy-then-delete, so there is no window in which both or neither exists. It happens once - the next append finds an ordinary file and extends it. Remove already did the right thing by accident (an empty list removes nothing, so it never writes). Clear still deletes, which is correct: it is the one place the user asked for exactly that. Three tests, all seen red with the set-aside branch disabled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The calculator runs the engine as a child process and got its refusals as the
English stderr sentence - correct for a command line, wrong for a window
(rule 15). Exactly one of nine was translated. A Polish window answered a
mistyped date with
chrono calc: unrecognised date format '31.02.2026' (calc.analyze_unrecognized)
which is the wrong language, the name of a process the user is not supposed to
know runs, and a raw contract key. This is the RELEASE-002 class the protocol
side already closed - the calculator was outside it because it does not travel
over the protocol.
The engine names its own failures: every refusal ends in `(calc.something)`,
stable across rewordings. CalcErrorText reads that key and nothing else, and
maps calc.X to calc.err.X. Keying on the key, never on the prose, is the rule
the missing-calendar check has always followed.
The test that asserted the opposite had a sound reason - a refusal names a step
number or a year range that no key reproduces, and swallowing the sentence
would cost the tester which step. What was wrong was the conclusion, that the
sentence should therefore BE the message. Both halves are kept: the translation
is the message, the engine's sentence a quieter line beneath it, and nothing is
parsed out of prose. It is empty when it would only repeat the line above -
the duplicate-line shape rule 24 caught in the About window.
Three literals go too: two in the view model, and the timeout in CalcClient,
which now carries `(calc.timeout)` so the protocol library can name a failure
without knowing any language.
The guard could not have caught this. It scans for key-SHAPED literals, and a
calc key lives inside an English sentence, so the literal it finds is the whole
sentence - every calc.* key was invisible to it, while this file's header said
they "are not translation keys". A second scan reads them off the trailing
parenthesis, exactly as the GUI does, with a literal canary of nine.
Seen red: one translation removed reddens the new guard, naming the key.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A transport failure ended the stream in exactly the silence of an ordinary EOF. A client that sent an over-long line, or a byte that is not UTF-8, saw a session close and was told the CORE had stopped - a diagnosis pointing away from the mistake, from a tool whose rule 6 forbids exactly that. One unreadable COMMAND was already answered and survived; the stream failing under it was not. Ending the stream stays: a half-read line cannot be resynced onto, and the remainder would arrive as junk or, worse, as a different command than was sent. What changes is that it says which of the two happened first. Two keys, not one. An over-long line and bytes that are not text are separate mistakes with separate fixes, and both arrive as io::ErrorKind::InvalidData - so they are told apart by the message this crate itself wrote, never by guessing at the error. The reader takes its event sink as an argument now. That is not decoration: emit writes to the process stdout, so a test could only have asserted the key MAPPING, and would have passed over a loop that mapped the key correctly and then never sent it. Seen red exactly that way - with the emission removed and the mapping left in place, the new test fails on an empty event list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three messages that were correct and could not be acted on.
A Win32 failure was formatted with {e:?} - Rust's DEBUG rendering of
windows::core::Error - so a missing path answered with
`Error { code: HRESULT(0x80070003), message: "..." }`: a struct dump instead
of a sentence. Worse for an English-only CLI (rule 15), the message field is
written by Windows in the SYSTEM's language, so on this machine it printed
Polish. Measured, not supposed. The OS text goes, the code stays, and the five
codes a tester actually meets get a clause of our own - including
ERROR_BAD_EXE_FORMAT, which is what a corrupt or non-native binary comes back
as and reads as nothing at all on its own.
chrono core: CreateProcessW failed - the path does not exist (0x80070003)
A bare number that cannot be a date was answered "unrecognised date format" -
a sentence about SHAPE, on input whose shape was read perfectly. What failed
was the range. Someone pasting a microsecond timestamp, which is the ordinary
way to reach this, was sent to look at their formatting. The shape now decides
which refusal an input gets, and the number branch says which range it missed.
The time fields were checked out of order: the leap-second refusal ran first,
so 25:00:60 was answered "second 60 is a leap second" while the hour was 25 -
fix the second, run again, then hear about the hour. And the catch-all under
it said "time out of range" without naming which of the three. Each field is
now refused by name, left to right, and the leap second only ever answers for
input whose only fault it is.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ay when it cannot
Integer division truncates, so under a large multiplier every timeout below
M milliseconds came out as 0 - and the comment called that "collapsing to a
yield". Sleep(0) is not a short sleep. It gives up the rest of the time slice
and returns at once, so an application polling with
while (!done) Sleep(100)
stopped sleeping under x1000 and started spinning a core flat out, for the
length of the session, with nothing said about it anywhere. The tester pays
that cost and cannot see it - worse, it lands on the very thing this tool
exists to measure, so the load reads back as a property of their application.
The timer family has had a floor all along for the same reason
(scale_timer_period_ms clamps so a periodic timer never becomes a one-shot).
The wait family simply never got one. It has one now, in both shapes: Sleep
and SleepEx through scale_wait, and NtDelayExecution through
scale_delay_interval - a floor on one and not the other would be a floor on
neither, since the first two bottom out on the third. A caller's OWN zero
still passes through: the floor is about what the scaling produces, never
about overriding what the application asked for.
The floor is not free, and that is the other half. A wait held there did NOT
accelerate by the session's multiplier, so the hook counts it (Cov gains
waits_at_floor, layout version 4 -> 5) and the audit warns. Coverage first,
and name what is left over (rules 4 and 27) - the alternative was a silent
difference in behaviour, which is what rule 27 calls a breach rather than a
compromise.
Measured end to end, not assumed: psleep with its default 6000 ms at x1440
comes out at 4 ms and raises nothing, which is correct; the same probe with
`--args "100 2"` lands on the floor and the report says so. Harness 146 PASS
on x64 and x86 with the widened layout.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…f rewinding Untouchable rule 3 says the duration clock never goes backwards. It did. The three axes multiplied elapsed real time by M with WRAPPING arithmetic, so about 10.6 days into a session at the maximum multiplier the product overflows an i64 and the axis jumps backwards by centuries in a single step. Measured in the probe for this commit: with wrapping restored, dur_quit_at comes back as -1000000 - a negative monotonic clock - where saturating holds it at i64::MAX. That is the axis .NET Stopwatch, Java nanoTime and Python perf_counter all read, so it is the most visible of the three. The wall clock has had a clamp and a warning for this exact shape since R2-X2. The three axes rule 3 is actually about had neither. Saturating does not make the rule whole - a standing clock is not advancing either - and pretending otherwise would be the silence rule 6 forbids. What it does is turn an invisible rewind into a visible stop, and the stop is now detectable (dur_axis_at_range_end) and reported as its own session warning. Its own, not folded into the wall-clock one: the two limits are different (the FILETIME range against an i64 of elapsed ticks), neither implies the other, and a reader told about one when the other happened has been told the session did something it did not. Reachability, stated plainly: at x1440 - the largest speed anyone has documented using - this is about twenty years of session, so no real run meets it. At the maximum the product allows, ten days. It is fixed because the horizon here is decades (rule 28) and because a rewinding monotonic clock is the kind of wrong answer this tool exists to not produce. Harness 146 PASS. Probe seen red with wrapping restored. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three places where the interface stopped saying things it had promised to say. The diagnostics marker fell out of its own queue. It was a LINE, enqueued once at the first drop, and the trimming dequeues from the front - so after another cap's worth of lines the marker reached the front and went, and the flag guarding it was already set, so it never came back. A block that was a tail then read as complete again, which is exactly what the marker existed to prevent. A running count cannot age out, and it says HOW MANY lines went, which the marker never could. The dispatcher's last-resort handler went silent for the whole run after one exception. The reason was sound - a binding that throws on every heartbeat used to open one modal box per beat, and a window nobody can out-click is worse than the fault it reports (R2-N12) - but a flag set once per RUN also muted every LATER and UNRELATED failure. An early stumble in the preset list silenced a coverage failure an hour on, and the app went on looking healthy while every operation threw. It is per SIGNATURE now (type plus innermost frame): a repeating fault is still one box, a genuinely new one is still heard, and the set is capped at eight because past that the interface is not recoverable in any sense the boxes help with. The event channel was the one input in this tool with no bound - every other has one. A consumer that stopped draining grew it for the length of the session. Bounded at 4096 with Wait, not Drop: losing an event would lose a verdict or an `ended`, and back-pressure onto a core we started ourselves costs nothing worse than a paused reader. The trimming rule is now a pure function with its own tests, which needed InternalsVisibleTo on ChronoMock.Protocol - the same seam ChronoMock.App already uses, and better than widening a method to public so a test can reach it. Both new tests seen red with the marker-in-queue shape restored. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The idle watchdog armed a timer before each wait and let it run through the handling loop. The handler runs on the UI thread by design (no ConfigureAwait anywhere in this pump), so anything that parks that thread for the length of the window cancelled the token while events were arriving perfectly well - and CancelAfter does not un-cancel an already-cancelled source, so the next wait threw at once. The session was then reported as "core not responding": a statement about the CORE, made because of a stall in the INTERFACE, which is rule 4 exactly. It measures the gap between events now, with the clock restarted after handling rather than before waiting. Seen red with the timer version restored: three events and a handler slower than the window, and the old shape reports a hung core on a stream that was delivering the whole time. The handshake had a smaller version of the same fault. It read events until `ready` and DROPPED everything else, so an error sent instead of ready would have been swallowed, the caller would have sat out the whole timeout, and the session would have been reported as "no handshake from the core" while the core had said exactly what was wrong. The core emits ready first today, so this is depth rather than a live bug - it costs one match arm and removes a diagnosis that points away from the answer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The core names a CDP channel by context TYPE plus API - "page Date.now" - and emits one coverage event per context. An application with two pages therefore produced two identical rows with different counts, and nothing on the panel said which was which. The comment here claimed the name told them apart: true of the KIND of context, not of the context. Two rows a reader cannot attribute are two numbers they cannot explain, which this project treats as a defect rather than a cosmetic gap. Each CDP row carries its context now, the way the CLI carries a pid - and on every row, not only when there are several, because the list is built as the events arrive and the second context is not known when the first is rendered. The native branch is untouched: those rows are already laid out per process. Seen red with the tag removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two limits and a check, all on input this build does not write. eval's doc comment says it validates rather than trusting its caller, because it is public API over a struct with public fields. It checked the YEAR and nothing else - and below that check the month indexes the month-name tables and feeds `(month - 1)` arithmetic that has no answer for 0. Unreachable through every surface that ships (all of them parse the base), so this is the API keeping a promise it already made, on a horizon where a second consumer of this crate is likely. A catalogue file was read with read_to_string and no ceiling - the last unbounded read left in this tool, on the one catalogue outsiders are invited to write. Four megabytes, checked BEFORE the read, so a file that cannot be a catalogue never has to fit in memory to be rejected. Presets share it. A holiday list is capped at 2000. The cost of "is this a business day" is the holidays in force times three years, and a business-day walk pays it again on every year boundary, so the two multiply. Measured here: 20 000 holidays with +200000bd took 4.6 s in release and 1 min 55 s in debug, against 0.47 s for the same walk on a shipped calendar. The GUI's calc timeout would cut that off with a message and the CLI has no such ceiling, so the refusal belongs in the loader, where the file's author sees it. The shipped calendars hold fourteen. The calculator-key guard added earlier this branch caught the new calc.base_not_a_date before the translations existed, which is what it is for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ape the last three attributes The language guard was green and did not look. It matched Polish DIACRITICS in lines that BEGIN with a comment, so two whole classes were outside it: a Polish word that carries no diacritics, and a comment trailing code. Four real violations had accumulated under it - three uses of "plasterek" and a "jawne" dropped into an English sentence. It reads trailing comments now and carries a short word list that only grows, and both new classes were seen caught with a probe before the four were fixed. The first version of the trailing-comment split claimed in its own comment that a `//` inside a string literal was a harmless false positive "because the URLs in this codebase hold no Polish letters". Running it said otherwise immediately: a CDP probe builds a URL out of "ó".repeat(60), and site/i18n's note fields use "//" as a KEY with Polish values, because that file is the Polish half of the site. Three false alarms from an assumption written down as a fact. It counts quotes now. History appends are serialised across processes. The write was already atomic - a unique scratch name and a move - but the read-modify-write around it was not, so two portable instances finishing at once both read the same list, both appended, and the second won. Nothing failed and nothing retried: one session was simply missing. The existing concurrency test asserted "does not throw" and "not empty", both true of the broken version, so it could never have seen this; the new one counts. Seen red with the mutex skipped. Three interpolations in the site's head block were the only ones not escaped, sitting between neighbours that were. Nothing in the config reaches them today, which is why the inconsistency would have lasted until the first value that did. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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 the twenty findings from the QA audit in
docs/qa-audit.md(local, outside git). Thirteen slices, each with its own regression test seen red with its half of the fix reverted.What was wrong, in one line each
Evidence that named the wrong thing
Loadpromised not to delete it.Text the user could not act on
chrono calc:process prefix and the contract key in brackets, for eight refusals out of nine.chrono runprinted Rust'sDebugrendering of a Win32 error, whose message field Windows fills in the SYSTEM's language - so an English-only CLI printed Polish.Silence
Time semantics
Sleep(0)returns at once, so a polling loop stopped sleeping and started spinning a core - a cost charged to the application under test, invisible, and easily read back as a property of that application. It has a floor now, and a wait held there is reported, because it did not accelerate by the full multiplier.-1000000where saturating holds. Reachable at about ten days under the maximum multiplier, twenty years at x1440.Guards that were green and did not look
Verification
pwsh tools/gates.ps112/12Ctllayout change (4 -> 5)Notes for review
CTL_LAYOUT_VERSIONgoes 4 -> 5:Covgainswaits_at_floor, which moves every slot after the first. Hook and core ship together, and the version bump is what turns a mismatch into a refusal.SessionRecordgains five optional fields. The schema stays 1 - a file from before them still loads, and a file written now still loads in a build that predates them.SessionViewModelsits exactly on CA1506 = 82 and both alternatives reddened the metrics gate.IsRunning.🤖 Generated with Claude Code