Skip to content

test(cli): name a callback arrow's site after its call, so sibling it() blocks key apart - #12544

Merged
os-litant merged 1 commit into
mainfrom
claude/issue-12531-sitekey-enclosing-fn
Aug 26, 2026
Merged

test(cli): name a callback arrow's site after its call, so sibling it() blocks key apart#12544
os-litant merged 1 commit into
mainfrom
claude/issue-12531-sitekey-enclosing-fn

Conversation

@os-litant

Copy link
Copy Markdown
Collaborator

Fixes#12531

enclosingFunctionName() in scripts/check-cli-test-child-env.mjs recognised four
shapes — a function declaration, a method, and an arrow/function expression bound to a
variable or to a property. None of them is the one that dominates a test directory: an
arrow passed directly as a call argument. it('...', () => {}), beforeAll(() => {})
and describe(...) matched none, so the walk ran past the callback to the source file and
attributed the site to (top-level).

Why that was not cosmetic

siteKey(row) is file::fn, and the DELIBERATE registry is keyed by it. Two deliberate
bulk copies in two it() blocks of one file therefore both keyed to
packages/cli/test/a.e2e.test.ts::(top-level)one entry would have silenced both,
and the second would never have been reviewed as part of the PR that needed it. That is
exactly the carve-out-by-accident shape this gate's own header says the registry exists to
prevent.

The repair

An arrow whose parent is a CallExpression with an identifier callee is now named
after that callee plus its first string-literal argument, falling back to the callee alone
when there is no literal:

it('boots the child', ...) -> it("boots the child")
beforeAll(async () => ...) -> beforeAll

Double quotes deliberately: the product is a DELIBERATE key, and this file's string
literals are single-quoted, so a key pasted into that registry needs no escaping.

⚠️ Latent today, not live

Both current DELIBERATE entries name real functions (childEnv, leakedEnv), so
nothing was mis-keyed and this repairs no live carve-out. The --list census is
byte-identical across the change (diff of before/after: no output) and the gate's own
verdict line is unchanged: 35 spawner source(s) among 96 ... 2 deliberate site(s) still pinned. What is closed is the next entry's problem.

Both directions, measured — not read off the new key strings

The self-test now drives the real audit() classification path with a synthesised
one-entry registry over a fixture holding two it() blocks. Reverse-verified by ablation:
the fix was committed, then the two lines that make it do anything were removed, the
mutation was confirmed on disk (git hash-object moved off the HEAD blob; the deleted
text greps to 0 hits), the self-test was re-run, and the file was restored with
git checkout HEAD -- on an absolute path and proven restored (hash back to the HEAD blob,
git diff HEAD empty). No build or dist/ is involved — this is a Node script executed
from source, with no package exports resolution and no vitest alias — so there is no
rebuild leg to report.

Before the fix, one registry entry keyed ...::(top-level) silenced BOTH blocks:

✗ ...and a (top-level) entry silences NEITHER -- before this fix that ONE entry silenced BOTH
-- {"deliberate":["(top-level)","(top-level)"],"findings":[]}

After the fix, that same entry silences neither, and a per-block entry silences exactly
its own block:

{"deliberate":[],"findings":["it(\"first\")","it(\"second\")"]}
{"deliberate":["it(\"first\")"],"findings":["it(\"second\")"]}

The ablation reddened 10 of 92 cases. The other 4 new cases are pins on unchanged
behaviour (a named helper inside an it() block still wins; a variable-bound arrow still
wins; a top-level copy is still (top-level); a property-access callee is still
(top-level)) and stayed green in both legs — so the battery is not merely "everything
reds".

Rule 1 and rule 2: one change, not a fork

Rule 2 (#11595, merged as 5d4d93af5) has no registry, so it cannot mis-key — but it
names sites in its messages from the same walk, and (top-level) was as unhelpful
there. Both rules read enclosingFunctionName, so this is one change, and a case pins it:
an env-less spawn inside it('spawns a probe', ...) is now reported as
[it("spawns a probe")]. No fork.

Named limits, pinned rather than discovered

  • The callee must be an identifier. it.skip('...') and it.each(table)('...') call
    through a property access or through another call and still read (top-level). That is
    a choice, not an oversight: widening to a property-access callee would also capture
    promise.then(...) and rows.map(...), whose callee is a worse site name than the
    enclosing test block the walk already reaches. Measured: 24 such spellings exist under
    packages/cli/test/** today, none of them at a bulk-copy or env-less site. Reported as
    an out-of-scope finding rather than folded in.
  • A title's whitespace is collapsed so the name stays one line, but it is never
    truncated
    — two long titles sharing a prefix would collide again, which is the defect
    being closed.

audit() takes an optional registry

Defaulting to DELIBERATE; every caller but the self-test uses that default. The property
under test — one entry silences one site — cannot be asserted against the live
registry, whose two entries sit in two different files and name real functions. A
synthesised registry is what lets the collision be shown gone through the real
classification path.

Self-test: 78 → 92 cases, none weakened

Every deletion in the diff is structural — the audit() signature, the
DELIBERATE-to-registry swap, and one prose line replaced by two. Zero t(...) cases
removed or altered
; git diff | grep for removed lines returns exactly those three.

Gates

Union re-derived from the real changeset with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (which reads the merge base itself) and run at
8b1a9c6db — 10 path-matched families plus the 2 convention-triggered ones the path
derivation cannot name (this edits a gate script), plus check:nul-bytes. All 13 exit 0,
each read from the gate's own verdict line with the exit code captured before any pipe:

  • check:cli-test-child-env✓ ... self-test: 92 cases pass / ✓ ... 35 spawner source(s) among 96 ... 2 deliberate site(s) still pinned
  • check:agent-test-spelling, check:bash32-floor, check:cli-command-ids,
    check:cross-package-test-inputs, check:entry-guard, check:parse-guard,
    check:pnpm-filter-targets, check-ci-filter-parity.mjs,
    check-cross-package-test-inputs.mjs, check:nul-bytes — all green
  • scripts/pm/bare-root-worklist.mjs --self-testOK self-test: 46 live row(s) ... none stale, none missing, none contradicted (no population declaration was moved, so no
    ledger row changes)
  • check:pm-dispatch-gates✓ dispatch-gates self-test: 719 cases pass

Repo-wide pnpm lint (eslint . --no-inline-config) ran in full — exit 0 in 59s — so
no narrowing is claimed. Control-character self-scan of the changed file: no match.

No changeset — skip-changeset

The diff is one CI gate script under scripts/, published in no package and visible to no
customer, so it releases nothing. This matches the sibling PR #12530, which touched this
same file and shipped no changeset. ⚠️ The dispatch asked for the latency statement to
live in the changeset; with no changeset to write, it is carried verbatim in substance
in the commit message and in the "Latent today, not live" section above. Flagged rather
than silently re-homed.


Generated by Claude Code

…() blocks key apart
`enclosingFunctionName()` recognised four shapes -- a function declaration, a
method, and an arrow/function expression bound to a variable or a property --
and none of them is the one that dominates a test directory: an arrow passed
DIRECTLY as a call argument. `it('...', () => {})`, `beforeAll(() => {})` and
`describe(...)` matched none, so the walk ran past the callback to the source
file and attributed the site to `(top-level)`.
That was never merely an ugly label. `siteKey(row)` is `file::fn` and the
DELIBERATE registry is keyed by it, so two deliberate bulk copies in two `it()`
blocks of ONE file both keyed to `<file>::(top-level)` -- one entry would have
silenced BOTH, and the second would never have been reviewed as part of the PR
that needed it. That is the carve-out-by-accident shape the gate's own header
says the registry exists to prevent.
Latent, not live: both current DELIBERATE entries name real functions
(`childEnv`, `leakedEnv`), so nothing was mis-keyed and the live census is
byte-identical across this change. What is closed is the NEXT entry's problem.
An arrow whose parent is a call with an IDENTIFIER callee is now named after
that callee plus its first string-literal argument, falling back to the callee
alone when there is no literal. Rule 2 reads the same walk and has no registry
to mis-key, so it gets the better site in its MESSAGES from the same change.
`audit()` takes an optional registry, defaulting to DELIBERATE. The property
under test -- ONE entry silences ONE site -- cannot be asserted against a live
registry whose two entries sit in two different files, so the self-test
synthesises one and drives the real classification path with it.
Self-test: 78 -> 92 cases.
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

1 participant

@os-litant