Skip to content

fix: three gates that could report success without checking - #191

Merged
kevintseng merged 4 commits into
mainfrom
fix/ci-verdict-and-matrix
Aug 23, 2026
Merged

fix: three gates that could report success without checking#191
kevintseng merged 4 commits into
mainfrom
fix/ci-verdict-and-matrix

Conversation

@kevintseng

@kevintseng kevintseng commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Three gates that could report success without checking anything. Each was found
by the previous one — the chain starts with a poll I watched land on PR #190.

1. wait-for-checks.mjs could call a partial matrix green

The script exists because gh pr checks' exit code cannot be trusted as a
verdict. It carried the same hole one level down: it asked "is pending zero?"
and returned 0 when the answer was yes. That is sound only if every check has
registered, and GitHub registers them in batches.

Observed, on PR #190:

2026-08-23T05:42:17Z pass=0 pending=1 fail=0 not-passed=[Analyze (javascript-typescript) [pending/IN_PROGRESS]]
2026-08-23T05:42:48Z pass=0 pending=12 fail=0 not-passed=[... twelve rows ...]

Thirty seconds where only one of thirteen legs existed. Had that row been
pass at that instant, the script would have printed
RESULT pass=1 fail=0 pending=0 exit=0.

A PASS now also requires the set of check names to be identical across two
consecutive polls. A FAIL is still immediate. Keyed on the name set, not the
count, because concurrency.cancel-in-progress can swap one run's legs for
another's at the same cardinality. Cost: one extra poll interval on a green run.

2. check-doc-claims checked the parent and ignored the child

It verified every memesh <word> names a registered command — capturing only
the first word. So memesh kg backfill read as kg, a real command, and
passed. The real name is kg backfill-relations.

The gate found a live one the moment it was added:
docs/api/API_REFERENCE.md:803 told readers the evidence edges are drawn by
memesh kg backfill. Fixed in the same commit.

Per-parent sets, not one flat set — patterns is registered twice, top-level
and under dream. Zero mentions fails, so the extraction cannot go quiet
and keep reporting "all resolve".

3. C3 counted a filename written in a comment as a caller

C3 finds gate-like scripts nothing runs, by counting basename occurrences. It
counted raw text.

This had already happened, been diagnosed, and been worked around instead of
fixed.
measure-injection-tokens.mjs opens with "Companion to
measure-work-topology-baseline.mjs" — one sentence, enough to make the
companion look called. Its correct entry was reported stale and pruned, and the
explanation was written into the other entry's reason field as a warning
not to delete the comment:

"…this file's header names measure-work-topology-baseline.mjs, and C3 counts
any basename reference — which is why that script stopped appearing as a hit
and its entry was pruned."

It happened again in this branch the moment a test header named
wait-for-checks.mjs, and the audit invited me to prune that entry too.

Comments now come out before counting. Over-stripping is the safe direction: a
lost reference makes a script look uncalled, which fails loudly and gets
triaged; a reference wrongly kept hides a finding silently. Consequences, all
correct: wait-for-checks.mjs is a hit again and its entry stands;
measure-work-topology-baseline.mjs surfaced and carries its own entry; the
obsolete note is gone.

The test's fixtures use invented filenames — the first version used the real
ones, and the strings holding them counted as references in exactly the
detector the test protects.

4. Reviewing the above found three more

The first version of #2 scanned single-backtick spans only, in a hand-listed
set of documents. Measured: that missed 19 distinct nested commands living in
fenced code blocks
— more than the 25 it saw — and the fenced ones are what an
agent copies out of llms-install.md and runs. It also never opened
skills/memesh-review/SKILL.md, which was not on the list. Now whole-document,
every tracked .md, derived from git ls-files instead of a list that drifts:
89 mentions across 31 files.

The widened gate then found a second dead command. memesh dream review,
in the 4.5.1 release notes:

$ git log -S "dream review" -- src/transports/cli/cli.ts
(no output — the string has never been in that file)

Not a retired command in frozen history: one that has never existed, published,
telling readers to run it. The passage means memesh dream show. Fixed.

That also settled a design question the honest way. The first version scanned
only [Unreleased] and the current version's section, reasoning that older
sections are frozen history which may name since-retired commands. The
reasoning was sound and the rule was wrong — it would have walked straight past
this. The CHANGELOG is read in full now.

A zero-row poll bypassed the settle bookkeeping. wait-for-checks.mjs
short-circuited on an empty check list with its own early return, and that
return never touched prevNamesKey/stablePolls — so "13 green, zero rows, the
same 13 green" counted as two agreeing polls and settled. A response that lost
every row is the clearest possible evidence the list is not holding still.
Fixed by deleting the early return rather than patching it: evaluatePoll owns
the empty case now (and must — two consecutive empty polls would otherwise
settle on an empty list). The script is 6 lines shorter and the case is finally
reachable from a test.

Two suspicions that measurement cleared

Reported because "I checked and it was fine" is worth as much as a finding:

  • Should the top-level check widen to all documents too? No. Measured: 59
    false positives, because memesh is the product name and prose says "memesh
    now…", "memesh has…". Its scope to llms-install.md + AGENTS.md — documents
    that are executed — is correct, not an oversight. The nested check does not
    have this problem: its parent word must be exactly config|kg|dream.
  • Does \s+ between parent and child wrongly match across a newline? Two
    matches do span one, and both are legitimate wrapped commands in the
    CHANGELOG (memesh dream\n conflicts, memesh\n config set). Narrowing
    to [ \t]+ would lose two real mentions and catch nothing. Left as is.

Verification

node scripts/run-tests-isolated.mjs   exit=0   157 files / 2308 tests (was 155/2284 — both new files ran), no "Errors" line
npm run verify:release                exit=0   ✓ 89 nested mentions across 31 markdown files all resolve
npm run lint / typecheck              exit=0

Break-test — mutate the source, not the assertion; restore by writing the
original string back and confirming sha256 is unchanged. 15/15 KILLED:

target mutation
settle guard accept the first green poll as a verdict KILLED
settle guard set the threshold to 1 (guard inert) KILLED
settle guard make the name key order-dependent KILLED
settle guard let a zero-row poll count toward settling KILLED
nested gate a mistyped child in a doc KILLED
nested gate a mistyped child in the live CHANGELOG section KILLED
nested gate a mistyped child inside a fenced code block KILLED
nested gate a mistyped child in a frozen older CHANGELOG section KILLED
nested gate the backtick extraction stops matching KILLED
nested gate the parent extraction loses a parent KILLED
stripComments stop stripping block comments KILLED
stripComments stop stripping line comments KILLED
stripComments stop stripping YAML/shell comments KILLED
stripComments strip too much (unknown extension) KILLED
stripComments treat the // in a URL as a comment KILLED

Loop-closure check (/verify-loop-closure): C1 and C2 do not apply — the diff
has no write/read persistence pair and no mocks at all (enumerated, both scans
empty). C4's three replace() hits are all inside stripComments, applied at
one point rather than on two sides of a round trip. C3 is the one that applies
and is the table above. PASS.

Not done, and why

The CI matrix is unchanged. I proposed tiering Windows onto push: main
only, then measured the run and the proposal did not survive it:

12.3 min  Build & Test (windows-latest, Node 22)
10.7 min  Build & Test (windows-latest, Node 24)
 2.7 min  Build & Test (macos-latest, Node 24)
 2.5 min  Build & Test (ubuntu-latest, Node 22)   … every other leg ≤ 2.7 min

The legs run in parallel, so dropping one Windows leg from PRs frees a queue
slot and changes wall-clock by nothing — the PR still waits ~12 min for the
other one. The gap is GitHub's degraded Windows runners (~5x), already
root-caused in this repository; cache: 'npm' is on and the double build was
removed. There is no change here that buys time without dropping the Windows
coverage that has caught Windows-only bugs before.

`wait-for-checks.mjs` exists because `gh pr checks`' exit code could not be
trusted as a verdict. It then carried the same hole one level down: it asked
"is pending zero?" and returned 0 when the answer was yes.

That is only sound if every check has been registered, and GitHub registers
check runs in batches. Measured on PR #190 (2026-08-23): one poll returned a
single row, `Analyze (javascript-typescript)`, while the other twelve did not
exist yet. Had that row been green at that instant, thirteen legs would have
been reported green on one of them — and the whole point of this script is
that I read its exit code instead of looking.

A PASS now also requires the SET OF CHECK NAMES to be identical across two
consecutive polls. A FAIL is still returned immediately: a red leg is red
whether or not its siblings have registered. Keyed on the name set rather than
the count, because `concurrency.cancel-in-progress` can swap one run's legs for
another's at the same cardinality.

Cost: one extra poll interval on a green run. That is the price of the verdict
meaning what it says.

The decision is a pure function in scripts/lib/check-settling.mjs so both
directions can be pinned without a live PR — the same split, and the same
reason, as scripts/lib/published-version.mjs.

Break-tested: accept the first green poll / set the threshold to 1 / make the
name key order-dependent — 3/3 KILLED, restore verified by sha256.
`check-doc-claims` verified that every `memesh <word>` in the agent docs names
a registered command. It captured only the FIRST word, so `memesh kg backfill`
read as `kg` — a real command — and passed. The real name is
`kg backfill-relations`. A gate that checks the parent and ignores the child is
a gate for the half of the name that is hardly ever wrong.

The gate found a live one the moment it was added:
docs/api/API_REFERENCE.md:803 told readers the evidence edges are drawn by
`memesh kg backfill`, a command that does not exist. Fixed here.

Parents and their children are both derived from cli.ts. Per-parent sets, not
one flat set: `patterns` is registered twice — top-level and under `dream` —
so a flat set would have accepted `memesh config patterns`.

Scanned across the living documents plus the CHANGELOG sections still being
WRITTEN (`[Unreleased]` and the current version's). Older CHANGELOG sections
are frozen history and may legitimately name a command that has since been
retired; a typo is only cheap to fix while the entry is being written, which is
also when it was missed last time.

Zero mentions FAILS. Without that, a doc style change or a regex edit would
leave it printing "0 mentions all resolve" forever — the exact shape of gate it
was added to close.

Break-tested: a mistyped child in a doc / a mistyped child in the live
CHANGELOG section / the extraction stops matching / the parent extraction loses
a parent — 4/4 KILLED, restore verified by sha256.
C3 finds gate-like scripts that nothing runs, by counting how often their
basename appears across workflows, scripts, tests and package.json. It counted
raw text, so prose counted.

This had already happened, been diagnosed, and been worked around instead of
fixed. `measure-injection-tokens.mjs` opens with "Companion to
measure-work-topology-baseline.mjs", and that one sentence was enough to make
the companion look called — its correct C3 entry was then reported as stale and
pruned. Somebody noticed and wrote the explanation into the OTHER entry's
reason field, where it sat as a warning not to delete the comment. It happened
again in this branch the moment a test's header comment named
`wait-for-checks.mjs`, and the audit invited me to prune that entry too.

A detector for uncalled gates that any sentence can silence is not a detector.
Comments come out before counting. Over-stripping is the safe direction: a
reference lost this way makes a script look UNcalled, which fails the audit
loudly and gets triaged. A reference wrongly kept hides a finding silently.

Consequences, both correct:
  - `wait-for-checks.mjs` is a C3 hit again, and its NOT-A-GATE entry stands.
  - `measure-work-topology-baseline.mjs` surfaced and now carries its own
    entry, which the workaround note said it would need.
  - that obsolete note is gone from the other entry's reason.

The stripper is a pure function so the "strips too much" direction can be
pinned too, and the test's fixtures use INVENTED filenames — the first version
used the real ones, and the strings holding them counted as references in
exactly the detector the test protects.

Break-tested: stop stripping block comments / line comments / YAML comments /
strip too much / treat the `//` in a URL as a comment — 5/5 KILLED, restore
verified by sha256.
Reviewing my own branch before merging it, on the rule that a gate is not done
until it has been looked at the way the thing it replaced should have been.

**The nested-command gate was looking in the wrong half of the documents.**
It scanned single-backtick spans only, in a hand-listed set of files. Measured:
that missed 19 distinct nested commands living in FENCED code blocks — more
than the 25 it saw — and the fenced ones are exactly what an agent copies out
of llms-install.md and runs. It also never opened
skills/memesh-review/SKILL.md, which was not on the list.

Now whole-document, every tracked `.md`, derived from `git ls-files` rather
than a list that can drift. 89 mentions across 31 files.

**And the widened gate found one.** `memesh dream review`, in the 4.5.1 release
notes: `git log -S "dream review" -- src/transports/cli/cli.ts` returns nothing
for the file's entire history, so it is not a retired command in frozen
history — it is a command that has never existed, published, telling readers to
run it. The passage means `memesh dream show`, whose description is "…ALL
observations, tags, source — so you can review the whole thing". Fixed.

That finding also settled a design question. The first version deliberately
scanned only `[Unreleased]` and the current version's section, reasoning that
older sections are frozen history that may name since-retired commands. The
reasoning was sound and the rule was wrong: it would have walked straight past
this. The CHANGELOG is now read in full. If a nested subcommand is ever
legitimately retired, its announcement will fail this gate — that is the moment
to decide what to do, not a reason to stop looking now.

**A zero-row poll bypassed the settle bookkeeping.** `wait-for-checks.mjs`
short-circuited on an empty check list with its own early return, and that
return did not touch `prevNamesKey`/`stablePolls` — so "13 green, then zero
rows, then the same 13 green" counted as two consecutive agreeing polls and
settled. A response that lost every row is the clearest possible evidence that
the list is NOT holding still.

Fixed by deleting the early return rather than patching it: `evaluatePoll` owns
the empty case now (and must, or two consecutive empty polls would settle on an
empty list). The script is 6 lines shorter and the case is finally reachable
from a test.

Break-tested, 10/10 KILLED — including the two new coverage directions (a
mistyped child inside a fenced block; a mistyped child in a FROZEN older
CHANGELOG section) and "let a zero-row poll count toward settling".
@kevintseng
kevintseng merged commit f277918 into main Aug 23, 2026
13 checks passed
@kevintseng
kevintseng deleted the fix/ci-verdict-and-matrix branch August 23, 2026 07:20
kevintseng added a commit that referenced this pull request Aug 23, 2026
docs(changelog): record what PR #191 changed
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