Skip to content

Give a skipped readback criterion its own state, and count criteria instead of declaring 12 - #435

Merged
TennyZhuang merged 2 commits into
mainfrom
fix/readback-skip-state
Aug 9, 2026
Merged

Give a skipped readback criterion its own state, and count criteria instead of declaring 12#435
TennyZhuang merged 2 commits into
mainfrom
fix/readback-skip-state

Conversation

@TennyZhuang

Copy link
Copy Markdown
Contributor

The readback criteria set is the last instrument in this job still running on two
states.

When build_assets_legacy is absent, two criteria fall into an else that prints
(no build_assets_legacy; nothing to compare the copy against) — and the run goes
green. A criterion silently not executing is exactly the shape exit 2 was
introduced elsewhere to prevent; this is the one place it had not been applied.

Latent today, permanent later. The table still exists, so the branch is not taken.
It becomes permanent the moment task #114 drops the table — after which the deploy log
prints a reassuring sentence forever while checking nothing.

Change

check() increments evaluated
skip() new — prints SKIP <criterion>: <reason>, increments skipped
else reports both criteria as SKIP instead of one parenthetical
end evaluated N criteria, M skipped, … failed; warns when M > 0

Skips are loud, not red. A skip can be legitimate, and failing the deploy on one
would hold production hostage to a comparison source we intend to delete. What it must
not be is quiet — a permanently skipped criterion is itself the event worth seeing, and
it now says so on every deploy instead of waiting for someone to remember.

Counted, not declared. This gate has been described in writing as "12 criteria"; a
number kept in prose stays at 12 while the code drifts. Same reason the workflow lint
reports N of M YAML files with both figures counted.

Why this is not bundled into #114

That was the earlier plan and I now think it was wrong. #114's deletion is irreversible,
needs its own explicit authorisation, and may be executed under time pressure — a fix
riding along with it is a fix riding the least careful moment.

Done separately, while the table still exists, it is also verifiable now:

legacy present  -> evaluated=2 skipped=0 fail=0
legacy absent   -> evaluated=0 skipped=2 fail=0   <- previously printed green, silently
real mismatch   -> fail=1                          <- still fails

Once the else branch is the only reachable one, that comparison is no longer possible.

Verification

  • three branches exercised as above
  • workflow lint: exit 0
  • deploy job parses, environment: gate intact, 16 steps
  • full worker suite: 403 passed, 35 files

Credit

The design — giving the skipped state a name — is @sentinel's. Counting the criteria
rather than asserting the total is @gogo's.

Rhea Rafferty and others added 2 commits August 9, 2026 11:08
…nstead of declaring 12

The criteria set was the last instrument in this job still running on two states.
When build_assets_legacy is absent the value-for-value comparison and the
row-survival check fall into an `else` that prints "(no build_assets_legacy;
nothing to compare the copy against)" and the run goes green. That is a criterion
silently not executing - exactly the shape `exit 2` was introduced elsewhere to
prevent, in the one place it had not been applied.

It is latent rather than active: the table still exists, so the branch is not
taken today. It becomes permanent the moment task #114 drops the table, at which
point the deploy log keeps printing a reassuring sentence forever while checking
nothing.

  check()  now increments `evaluated`
  skip()   new; prints `SKIP <criterion>: <reason>` and increments `skipped`
  else     reports both criteria as SKIP rather than one parenthetical
  end      prints `evaluated N criteria, M skipped, ... failed`, and warns on M>0

Skips are loud, not red. A skip can be legitimate, and failing the deploy on one
would hold production hostage to a comparison source we intend to delete. What it
must not be is quiet: a permanently skipped criterion is itself the event worth
seeing, and it announces itself on every deploy rather than waiting for someone
to remember that a deletion elsewhere retired a check here.

Counted, not declared, for the same reason the workflow lint reports "N of M YAML
files" with both figures counted: this gate has been described in writing as "12
criteria", and a number kept in prose stays at 12 while the code drifts. Today
the declared number happens to be right, and "happens to be right" is how it goes
wrong later.

Deliberately NOT bundled into #114's deletion, which was the earlier plan. That
deletion is irreversible, needs its own explicit authorisation, and may well be
executed under time pressure - a fix riding along with it is a fix riding the
least careful moment. Done separately, while the table still exists, the change
is verifiable now: both branches were exercised (legacy present -> 2 evaluated,
0 skipped; legacy absent -> 0 evaluated, 2 skipped; a real mismatch -> still
fails), which is not possible once the else branch is the only reachable one.

Design is Sentinel's (give the skipped state a name); counting rather than
asserting the total is Gogo's.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…measurement

Sentinel caught that this PR fixed the confusion in one direction and left it
standing in the other. `PRAGMA foreign_key_check` being refused by D1 was
reported as `FAIL foreign_key_check did not run`, which sends someone looking
for a foreign-key violation that was never measured.

The blocking is deliberate and stays: the comment above that branch reasons that
a refusal means D1 changed what it permits, and that is worth stopping for. What
changes is what it says it is.

  ok           ran, correct
  FAIL         ran, wrong answer          -> blocks, counted as evaluated
  CANNOT RUN   did not run, and that is itself the anomaly
                                          -> blocks, NOT counted as evaluated
  SKIP         did not run, acceptably    -> does not block

`cannot_run` sits between `skip` and a failing `check` on purpose. Reporting it as
a skip would let a substrate change through; reporting it as a failure asserts a
violation nobody observed. It is the same distinction the rest of this job now
makes - could-not-run is not a result - applied to the one place that had it
backwards.

The summary line reports the count separately: "evaluated N criteria, M skipped,
K could not run, ... failed". A criterion that stops being measurable shows up as
K rising rather than as a fabricated verdict.

Exercised: pragma clean -> evaluated=1 fail=0; pragma finds orphans ->
evaluated=1 fail=1; pragma refused -> evaluated=0 unrunnable=1 fail=1, and the
three print as distinguishable lines.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@TennyZhuang
TennyZhuang merged commit 781dcfa into main Aug 9, 2026
7 checks passed
@TennyZhuang
TennyZhuang deleted the fix/readback-skip-state branch August 9, 2026 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant