Skip to content

fix(cli-gate): name it.skip / it.only / it.each blocks instead of collapsing them onto (top-level) - #12577

Open
os-litant wants to merge 1 commit into
mainfrom
claude/issue-12545-callback-site-modifier-unwrap
Open

fix(cli-gate): name it.skip / it.only / it.each blocks instead of collapsing them onto (top-level)#12577
os-litant wants to merge 1 commit into
mainfrom
claude/issue-12545-callback-site-modifier-unwrap

Conversation

@os-litant

@os-litantos-litant commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Fixes#12545

callbackSiteName() required a bare-identifier callee, so it('x', fn) was named but it.skip('x', fn) (a PropertyAccessExpression) and it.each(table)('x', fn) (a CallExpression) returned null. enclosingFunctionName() then fell out of its loop to '(top-level)', collapsing every skipped / only / table-driven block in one file onto a single site key — the same collision #12531 closed for the identifier branch, one modifier over.

Measured on origin/main @ aa4591971 before touching anything:

callcallee nodebeforeafter
it('x', fn)Identifierit("x")it("x") (unmoved)
it.skip('x', fn)PropertyAccess(top-level)it.skip("x")
it.only('x', fn)PropertyAccess(top-level)it.only("x")
describe.skip('x', fn)PropertyAccess(top-level)describe.skip("x")
it.each([…])('x', fn)CallExpression(top-level)it.each("x")
describe.each([…])('x', fn)CallExpression(top-level)describe.each("x")
it.skip.each([…])('x', fn)CallExpression(top-level)it.skip.each("x")
promise.then(cb)PropertyAccess(top-level)(top-level) (refused)
rows.map(cb)PropertyAccess(top-level)(top-level) (refused)

The unwrap is targeted, and it is gated on ROSTERS rather than on node kind

A blanket "property access whose base is an identifier" rule was measured, not assumed, and it is much worse than the defect. Against the gate's own live population (packages/cli/test/**, 98 files), that rule matches:

  • 209 call sites across 87 distinct calleesliterals.filter, child.on, entries.map, warnings.filter, server.close, Array.from, vi.fn, run.then, rows.map
  • against this roster's 22it.each(…) ×20 and describe.each(…) ×2, which is the entire modifier population of that directory.

Those 87 are not test blocks, their callee is a worse site name than the enclosing block the walk already reaches, and naming them would move the key of every site nested inside one. Re-keying is a data migration wearing a bug fix's clothes.

So the gate is two rosters, TEST_BLOCK_ROOTS × TEST_BLOCK_MODIFIERS:

  • rootsit, test, describe. ⭐ A roster on this side is load-bearing, not tidiness: this directory calls it.name, it.type, it.next and it.prevVersion on ordinary loop variables that happen to be spelled it. Gating on the root alone would name a callback site after one of those.
  • modifiersskip / only / todo / fails, concurrent / sequential / shuffle, each / for, skipIf / runIf. Every segment after the root must be on the roster, so it.skip.each(…) is named through the whole chain and a.b.c(cb) never is. each/for and skipIf/runIf curry, and are unwrapped exactly one call deeper.
  • extend is deliberately absent: test.extend(fixtures) builds a new test function, not a block, and its product is bound to a name the identifier branch already reads.

The title is read from the outer call's arguments, so it.each(['from the table'])('from the block', fn) is named for the block, never for a string sitting in the table. That is pinned.

⭐ Key stability — the proof, stated rather than left to be noticed

The identifier branch is reached first and unchanged, so plain it(…) naming is byte-identical. That matters because DELIBERATE_REROUTE holds two keys this function produced, carrying it("…") titles verbatim, and DELIBERATE holds two more from the variable-declaration branch — all four pinned in both directions, so a moved key exits 1.

The green live-tree run below IS the proof that no existing key moved, and it says so in its own words:

✓ check:cli-test-child-env: 35 spawner source(s) among 98 under packages/cli/test/**; …
(0 baselined in 0 file(s), ⛔ SHRINK-ONLY; 2 deliberate site(s) and 2 deliberate reroute(s) still pinned).

And the pin was proven LOUD rather than assumed to be, because a green that cannot fail proves nothing. Reverse verification on the committed tree — predicted direction RED:

  • mutated one DELIBERATE_REROUTE key (NODE_ENV=development (explicit)NODE_ENV=deliberately-moved-key (explicit));
  • mutation confirmed on disk, not by the editor's exit code: anchor occurrences 1 → 0, mutant 0 → 1, blob 2210188b…ec97c08b…;
  • gate on the mutated tree: exit 1, reporting the now-unclassified site by the very name this function produces[it("NODE_ENV=development (explicit): the gate stays OPEN — unaffected by the production default")];
  • restored with git checkout HEAD -- naming an ABSOLUTE path, and the restore was proven by state: blob back to 2210188b…, git diff HEAD empty.

⛔ No registry entry was edited. Had one gone stale, that would have been the finding.

Self-test: 132 → 152 cases, extend-never-weaken

Exactly one pre-existing case changed, and it is the one that pinned the defect being fixed:

'an it.skip() callee is a property access, not an identifier, so it still reads (top-level)''an it.skip() block is named, not (top-level)'

That is a strengthening — a specific name replaces a fallback — and what the old case's comment was really protecting (the refusal of promise.then / rows.map) is now pinned outright, as six explicit refusal cases rather than as a side effect. Every other change is additive; the full deletion list in the diff is that one case plus the two implementation lines and the docblock limit it described.

Added: each modifier form; both curried forms; the chained form; the table-string trap; two sibling it.each blocks getting distinct names; an end-to-end audit() case where one registry entry silences one it.each block and leaves its sibling a finding; the six refusals; and —

the negative pin, which is the one that matters: 'a plain it() block STILL yields exactly it("x") — the identifier branch did not move'. Without it nothing in the suite distinguishes "the modifiers now work" from "everything got renamed".

Rule 5 — the second implementation, MEASURED, not swept

scripts/check-durability-degradation-log-level.mjs:1695enclosingFunctionName(node), used at :2451 and :2493. ⛔ Not touched here. What I read:

It does NOT have this card's defect — and not because it handles modifiers, but because it has no callback-naming branch at all. Its four shapes are function declaration, method declaration, variable-bound arrow/function-expression, and named function expression. It never names it('x', fn)either, so there is no identifier branch for a modifier unwrap to extend. The claim comment's guess that "it takes no sourceFile argument, so it may not have the same branches" is correct.

Its fallback is undefined, not '(top-level)', and the two call sites diverge:

  • :2493declaredPropagationFor — no defect, structurally.if (!fnName) return globalPropagation; short-circuits, so no key is ever built. That is a closed door, not a shared key.
  • :2451readInventionKey (:2827) — the same collision class, latent. It keys a shrink-only baseline on f.file and f.fn joined by ::, substituting a fixed placeholder — the word anonymous wrapped in angle brackets — whenever f.fn is undefined. So two anonymous-callback read-seams in one file share one key, and a single baseline entry would classify both.

Measured live:scripts/durability-read-invention.baseline.json holds 1 entry, with 0 anonymous fn and 0 files carrying more than one entry. ⇒ latent, exactly as this card was. Filed unlabelled as its own card: #12576.

Falsifications of the dispatch order (reported, not reconciled)

  • ⚠️main had moved. The order pins the merged base at 1b7e3d2ce; origin/main was aa4591971 when I cut the branch (and 9bed0b0fe by the time gates ran). Re-verified the defect on aa4591971 rather than trusting the description — it reproduced exactly.
  • ⚠️The card's "24 such spellings" is a textual count including 2 prose mentions.grep finds 24 under packages/cli/test/**, but two are inside docblocks (it.each` below against silently iterating nothing, it.each` over an empty). The AST count is 22. Fittingly, the suite's own section (6) exists to make exactly this point — "prose is not code, which is what an AST buys over a text scan".
  • Ruling 2's literal spelling contradicts its own headline, and I followed the headline. "Accept a callee that is a property access whose base is an identifier" admits promise.then(cb) and rows.map(cb)promise and rowsare identifiers — which is precisely the blanket widening the same ruling forbids two sentences later, and which the issue body names as the reason a blanket fix is worse than the defect. The issue body's own precise spelling ("it / test / describe followed by a modifier") is the one that is self-consistent, so the implementation is roster-gated. The 209-vs-22 measurement above is what decided it. Flagging rather than silently choosing a side.
  • ✅ All five line anchors handed down (:521, :1695, :2451, :2493, and the 132-case count) were correct. Surface measured as claimed: git grep -ln "callbackSiteName" → exactly one file.
  • ✅ The 9 unreachable packages/spec families printed by dispatch-gates are the known [finding] 9 packages/spec gate families declare path populations the tree no longer has — a quiet green on every card, printed but never recorded #12514; not re-filed.
  • ⚠️Measured on this body itself, twice. GitHub's sanitizer silently ate every short angle-bracket fragment on creation (a path placeholder and two spellings of the anonymous key literal) — inside backticks, and the eaten 'anonymous' left the text asserting the fallback was an empty string, which is false. The first repair moved the literal into a fenced code block and that was eaten too, so a fence is no protection either. Both are now spelled in prose. ⛔ Worth knowing before quoting any angle-bracketed literal in a GitHub body.

Changeset — rule applied, and which one

No changeset. AGENTS.md line 1108: "Pure bug fixes do not require a changeset." This is a repo-internal CI gate script under scripts/, published by nothing and user-visible in no way; the change alters how a gate names sites in its own registry keys, not what it refuses. skip-changeset applied — verified it is a real mechanism in this repo (.github/workflows/pr-automation.yml:294 reads it live, not from the event payload) rather than assumed.

Verification

typecheck is NOT APPLICABLE to a .mjs gate script — the root tsconfig.json sets no allowJs, so tsc --listFiles returns 0 hits for this file. ⛔ Recorded as not-applicable, never as a green.

All gates below run on the final commit 605beb0b3, exit codes captured before any pipe (cmd > log 2>&1; code=$?), under os-verify-lock (VERDICT command-exit 0 · held the lock 73s · waited 1s):

gateexit
check:cli-test-child-env (self-test 152 pass + live tree)0
check:agent-test-spelling0
check:bash32-floor0
check:cli-command-ids0
check:cross-package-test-inputs0
check:entry-guard0
check:parse-guard0
check:pnpm-filter-targets0
check:nul-bytes0
check:pm-dispatch-gates (convention-triggered)0
scripts/check-ci-filter-parity.mjs0
scripts/check-cross-package-test-inputs.mjs0
scripts/pm/bare-root-worklist.mjs --self-test (convention-triggered)0

Families derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (10 path-derived + the 2 convention-triggered the order named). Control-character self-scan beyond the gate: grep -naP '[\x00-\x08\x0b\x0c\x0e-\x1f\x7f]' → no hits.


Generated by Claude Code

…lapsing them onto (top-level)
callbackSiteName() required a bare-identifier callee, so `it('x', fn)` was named
but `it.skip('x', fn)` (a property access) and `it.each(table)('x', fn)` (a call)
returned null. enclosingFunctionName() then fell out of its loop to
'(top-level)', collapsing every skipped/only/table-driven block in one file onto
a single site key -- the same collision #12531 closed for the identifier branch,
one modifier over.
The unwrap is TARGETED, gated on two rosters rather than on node kind: the roots
it/test/describe, and the vitest modifiers that keep a chain a test block. A
blanket "property access whose base is an identifier" rule matches 209 call sites
across 87 distinct callees under packages/cli/test (literals.filter, child.on,
run.then, rows.map, vi.fn, ...) against this roster's 22, and naming those would
MOVE the key of every site nested inside one. The modifier roster is also what
refuses it.name / it.type / it.next -- ordinary loop variables in this directory
that happen to be spelled `it`.
The identifier branch is byte-identical, which DELIBERATE_REROUTE's two it("...")
keys pin in both directions: a green live-tree run is the proof that no existing
key moved.
Self-test 132 -> 152 cases. One pre-existing case changed: the one that pinned
it.skip AS (top-level), retargeted to the name it now produces; what its comment
was protecting (the refusal of promise.then / rows.map) is now pinned outright.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/mskip-changesetPR has no user-facing published change; bypasses the changeset gate

Projects

None yet

2 participants

@os-litant@claude