Cover the checkpoint bounds that shipped without tests - #210
Merged
Conversation
`checkpoint-retention-and-lazy-load` landed its implementation in #190 — lazy checkpoint reads and a `maxCheckpointSessions` bound separate from `maxProcesses` — and its test tasks 4.1, 4.2, 4.3 and 4.6 stayed open. The behaviour has been in `main` since, asserted by nothing. Each test asserts the property the task names, not merely that the code runs: - 4.1, no payload reads on load: two sessions are saved, both checkpoint files are then overwritten on disk with unparseable content, and `load()` still succeeds and lists both references. Only the later `loadCheckpoint()` touches the corrupt content, resolving to `undefined` rather than throwing. A test that merely counted reads would pass against an implementation that read them and discarded the result. - 4.2, the bound evicts: three sessions under `maxCheckpointSessions: 2`, asserting the oldest file is gone from disk while the two newest still resolve — and that all three *processes* are retained, which is what proves the two limits are independent rather than one limit applied twice. - 4.3, retention by recency and not by state: a `completed` process newer than a `failed` one, both within the bound, both keeping their checkpoint. Evicting by state would silently withdraw a rollback the product offers, since `canRollback` covers completed and failed runs. - 4.6, rollback after a lazy restore: a `completed` process restored through the same `loadCheckpoint()` indirection `restore()` uses in production, asserting the delta and rollback outcome match the eager path. Task 4.5 stays unchecked, deliberately. `restore()` still reads every referenced checkpoint, so a test asserting "no reads except an interrupted session with no delta" would fail against the real implementation, and a test that dropped the read-count claim would record the task as done while proving something weaker than it asks. It is outstanding until 2.3's remaining half lands. Tests only — no behaviour changes, so no changeset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
VeryComplexAndLongName
added a commit
that referenced
this pull request
Sep 4, 2026
`npm audit` was the last step of the `quality` job, and every other job in the workflow declares `needs: quality`. So an outage at npm's advisory service skipped the merge gate, the extension integration suite and the browser suite on pull requests that had nothing to do with dependencies. Observed twice in a row on #210: 04:59:17 npm run verify ok, 263 tests passed 04:59:18 npm audit started 05:06:19 npm warn audit 503 Service Unavailable - POST registry.npmjs.org/-/npm/v1/security/audits/quick ##[error]The operation was canceled. Seven minutes of retries against a failing endpoint, then the 10-minute job limit, with all of the job's actual work already finished and green. The third attempt, once the registry recovered, passed the same job in 7m12s — still most of the allowance spent inside one network call. The audit now runs in its own job that declares no `needs` and that nothing declares `needs` on, so its result decides only its own check. The move alone would not have been enough. `npm audit` exits non-zero both when it finds a high-severity advisory and when it cannot reach the service, and those are not the same claim: an audit that did not run says nothing about these dependencies, in either direction. Isolating the step without separating the two would have left the same red check on an unrelated pull request, just in a narrower place. So the report is read rather than the exit status trusted: - a high or critical advisory fails the check; - an error response, unparseable output, a missing report, or absent vulnerability counts does not fail it, and says in a workflow warning that the audit was not carried out; - `timeout` bounds the command itself and not just the job, because a job timeout is a red check — the outcome this exists to prevent — so the command has to give up first for its result to be interpretable at all. Absent counts are treated as unknown, never as zero, for the same reason `AuditEntry.usage` distinguishes unreported from free: a check that passes without having checked anything must say so rather than look clean. Verified rather than reasoned about, since the original failure was in the interpretation of an exit status: the interpreter was run against nine inputs — clean, high, moderate-only, a 503 error object, plain-text diagnostics, missing counts, an empty file, a missing file, and no argument — asserting each exit status; the workflow was parsed to confirm `quality` no longer audits, `dependency-audit` gates on nothing, and every other job's `needs` is unchanged; and the real audit was run end to end against this repository's lockfile. No changeset: CI configuration, nothing published changes. Co-authored-by: Claude Opus 5 (1M context) <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.
Why
checkpoint-retention-and-lazy-loadshipped its implementation in #190 —lazy checkpoint reads, and a
maxCheckpointSessionsbound separate frommaxProcesses— and left test tasks 4.1, 4.2, 4.3 and 4.6 open. Thebehaviour has been in
mainsince, asserted by nothing.What each test actually proves
load()is called.load()still succeeds and lists both references; only the laterloadCheckpoint()touches the corrupt content, resolving toundefinedrather than throwing.maxCheckpointSessions: 2— the oldest file is gone from disk, the two newest still resolve, and all three processes are retained. That last part is what proves the two limits are independent.completedprocess newer than afailedone, both inside the bound, both keeping their checkpoint. Evicting by state would silently withdraw a rollback the product offers, sincecanRollbackcovers completed and failed runs.completedprocess restored through the sameloadCheckpoint()indirectionrestore()uses in production; delta and rollback outcome match the eager path.4.1 is written that way on purpose: a test that merely counted reads would
pass against an implementation that read every payload and threw the
result away. Corrupting the files first makes the claim unfakeable.
What stays open, and why
Task 4.5 remains unchecked.
restore()still reads every referencedcheckpoint, so a test asserting "no reads except an interrupted session
with no delta" would fail against the real implementation — and one that
dropped the read-count claim would mark the task done while proving
something weaker than it asks. It waits on 2.3's remaining half
(deferring
rollback/describeDeltato read lazily).Scope
Tests and task bookkeeping only. No behaviour changes, so no changeset.
🤖 Generated with Claude Code