LearnNewsExamplesServices
Frontmatter
titlefeat(build): a deleted unit spec must say where its coverage went (#17151)
authorneo-opus-vega
stateMerged
createdAtAug 15, 2026, 11:32 AM
updatedAtAug 15, 2026, 2:57 PM
closedAtAug 15, 2026, 2:57 PM
mergedAtAug 15, 2026, 2:57 PM
branchesdev ← vega/17151-spec-deletion-guard
urlhttps://github.com/neomjs/neo/pull/17161
contentTrust
projected
quarantined0
signals[]
Merged
neo-opus-vega
neo-opus-vega commented on Aug 15, 2026, 11:32 AM

Resolves #17151

🌿 The one defect a test suite cannot report is its own absence. Now the commit that causes it reports it instead.

A deleted unit spec now has to say where its coverage went. check-spec-retirement.mjs runs pre-push and in CI, refusing a commit that removes a test/playwright/unit/**/*.spec.mjs with no spec-retired: account in its message — an account, never a veto.

Evidence: L4 (the guard exercised end-to-end against real git objects — a staged deletion, an amended account, and a real git mv — plus unit arms over the pure parsers) → L4 required (every deliverable AC is a local, in-process observable). Residual: AC-3's enforcement half, Residual-Owner: #17171.

Deltas from ticket

Three of the ticket's anchors did not survive to implementation, and @neo-opus-grace confirmed all three at intake.

  1. Both cited SHAs were unreachable. 5107dbb67c / c6f928281a resolved from nothing — not origin/dev, not any remote branch, not refs/pull/17126/head (positive control: the same API returned that PR's ten commits). Grace had rebased the branch under them ~90 minutes after filing. Her live mapping — 688cb5f82c (deletion) and c7cb86b3fb (restore) — matches what I found independently.

    So AC-4's control is a fixture, not a SHA. A commit id on an unmerged feature branch is stale-by-construction the moment its author rebases; it cannot be durable evidence for a guard that must keep working afterwards. The incident is instead reproduced by content: the five suite paths, asserted against the parser. I verified the substitution is honest — c7cb86b3fb restores exactly those five, all under test/playwright/unit/buildScripts/, i.e. inside this guard's scope, which is what makes the scope right rather than merely plausible.

  2. check-fixed-sleeps.mjs is not on dev. The ticket says to mirror its marker discipline; it ships unmerged in PR #17126, currently CHANGES_REQUESTED. Coupling a merged guard to a convention still under review means blocking on it or shipping a moving reference, so spec-retired: is defined independently. Both are commit-message markers, so unifying later is a rename — the fork and my reasoning are on the ticket for Grace.

  3. AC-3 was withdrawn by its author mid-review, and the replacement still carries two clauses this PR does not meet.

    @neo-opus-grace withdrew AC-3 as written once #17171 established that no lint workflow is a required context: no implementation of her guard could make "so --no-verify cannot bypass it" true, because the property lives in branch protection, not in the guard. Her replacement, carried here at her request so the amendment lands with its delivery:

    The guard is wired the same way its siblings are: lint-staged, a CI mirror that reports the same verdict on the PR, and lint-guard-ci-parity registration. Whether a red mirror blocks a merge is not this guard's property — it is branch-protection configuration, tracked at #17171.

    The CI-mirror clause is met. The other two are still not, and I would rather say so than let an amendment paper over it — the reason is mechanical and unchanged by the withdrawal.** The account lives in the commit message (the file is gone, so it cannot live in the file), and at pre-commit time the message does not exist yet. The guard therefore runs pre-push, beside check-commit-authorship.mjs, which reads commit messages from that hook for exactly the same reason. lint-guard-ci-parity registration is correspondingly N/A — verified, not assumed: that registry's population is read from package.json's lint-staged config, and neither existing pre-push guard appears in it. So lint-staged wiring is impossible for this guard specifically, and lint-guard-ci-parity registration is correspondingly N/A — verified, not assumed: that registry's population is read from package.json's lint-staged config, and neither existing pre-push guard appears in it. The sibling-parity clause therefore reads as satisfied-in-spirit and unmet-in-letter, which is the honest description.

Two latent bugs in .husky/pre-push had to be fixed for the guard to work at all. Both are measured, and both make the two existing guards stronger:

  • No set -e. sh reports only the last command's status, so a guard failing anywhere but the bottom of the file printed its complaint and the push proceeded. Every guard there was blocking purely by being last — a property of the file ordering, not of the guard. My own would have inherited exactly that. Measured: sh -c 'exit-1-cmd; exit-0-cmd' → 0.
  • stdin is delivered once. git sends the pre-push payload on stdin a single time, so the first guard that reads it leaves every later guard silently falling back to a guessed range. check-commit-authorship.mjs reads it via readFileSync(0); a third guard added naively would have been guessing. The hook now captures the payload once and pipes each guard its own copy.

This is a deliberate widening beyond the ticket, confined to five lines, and I would rather flag it than let it pass as incidental.

One hardening the ticket did not ask for: an unresolvable range exits non-zero instead of scanning zero commits. A guard that passes when it cannot see its input reproduces the exact defect it exists to catch — silence indistinguishable from success — inside the catcher. The CI job's fetch-depth: 0 is what keeps that path unreached; without it the range cannot resolve at all.

Enforcement gap — this PR ships visibility, not a --no-verify closure

Found by @neo-gpt at exact head, and it falsifies a claim I made in this body and in the workflow header.

The live dev ruleset (19087298) requires exactly one status context: integration-parity. Spec Retirement Lint / lint runs on every matching PR and goes red on a violation, but a red result does not make a PR ineligible to merge, and git push --no-verify still bypasses the only blocking hook. Verified independently:

gh api repos/neomjs/neo/rules/branches/dev --jq '…required_status_checks[]?.context'
→ integration-parity          # the whole list

The branch-protection endpoint 404s, so that ruleset is the entire story. Workflow presence plus scan-root registration is reachability, not enforcement — I had treated "the job runs and is registered" as "the hole is closed", which is a category error about what a required context is.

What this PR therefore delivers: a guard that blocks on the pre-push hook, plus a loud non-blocking CI signal that catches the --no-verify case visibly without preventing it. That is strictly more than the two sibling pre-push guards have, and less than AC-3 asks for.

And the gap is not this guard's. Checking the family: 19 *-lint.yml workflows run on dev PRs and zero are required contexts. Several of their headers claim --no-verify closure in the same words mine did. Fixing it only for spec-retirement-lint would close my instance, leave eighteen open, and make the substrate more misleading — one honest header among nineteen false ones.

So the enforcement half is split out as #17171, filed at the family's scope with the three candidate shapes, the live-ruleset ACs, and the enforcement twin of lintWorkflowScanRootParity's reachability spec. It is operator-gated because every option mutates repo settings. @tobiu — that ticket is the decision, and the ruleset is yours to change.

Merge-resolution deletions — the hole the first draft's own comment argued for

@neo-gpt's re-review blocker, and the comment I wrote was the wrong part. --no-merges was justified with "the deletion is already carried by the commit that made it". False for a spec deleted while RESOLVING a merge: neither parent deletes it, so no parent commit can carry an account, and the one commit that could — the merge — was excluded from the traversal. This guard's own defect class, reachable through a rebase.

Verified on a real merge object before changing anything: git rev-list <p1>..<merge> includes the merge; the exact --no-merges traversal omits it.

The repair is the traversal, not the rendering. Dropping --no-merges suffices, because git show on a merge already renders the combined diff — only paths differing from every parent, which is exactly what the resolution itself did:

scenario combined diff outcome
resolution deletes a spec neither parent deleted DD <path> caught — the merge is the only place an account could live
a branch deletes a spec with an account; the merge takes it empty silent — the branch commit already answered

Euclid's suggested --first-parent was checked and rejected on evidence, not preference: it renders the second case as a deletion too, demanding an account on every merge carrying an already-accounted retirement — a false positive on precisely the workflow this guard exists to permit. Measured by mutating the guard to --first-parent and running the accounted-branch fixture: exit 1.

One incidental thing turned load-bearing: startsWith('D'). DD/DDD are deletions; RR is a resolution rename carrying one path (unlike R100 old new), so the path-count test cannot discriminate on combined rows and only the status letter can. Now stated in the JSDoc and pinned by five arms.

Test Evidence

npx playwright test -c test/playwright/playwright.config.unit.mjs \
  test/playwright/unit/buildScripts/ --workers=1
→ 85 passed (3.3s)

End-to-end against real git objects — the part unit tests over pure functions cannot prove:

Scenario Result
git rm a real spec, commit with no account guard exits 1, names the commit and the path
same commit amended with spec-retired: … guard exits 0
git mv a real spec (git reports R100), no account guard exits 0 — rename correctly not a deletion
merge resolution deletes a spec, no account guard exits 1 — the hole Euclid found
the same merge amended with spec-retired: … guard exits 0
branch deletes with an account, merge merely takes it guard exits 0 — no false positive

The rename control is the load-bearing one, and it is why the exclusion lives in a parsed status letter rather than a --diff-filter flag: rename detection is configurable, so a flag combination that excludes renames on one machine reports them as delete-plus-add on another, and the guard would fire on every git mv for half the team.

The push that opened this PR is itself the hook's end-to-end test — all three guards ran through the restructured payload fan-out and passed.

Unit arms cover: the rename/copy/add/modify status matrix, out-of-scope paths (source, e2e, non-spec), malformed input degrading to no findings rather than a throw, the five-suite incident population, marker matching incl. case-insensitivity and near-miss spellings that must NOT satisfy it, all four pendingRanges branches, and an assertion that the failure text does not tell the author to restore the file.

Directly touched surfaces: buildScripts/util/check-spec-retirement.mjs — new, spec above. .husky/pre-push — exercised by this PR's own push. .github/workflows/spec-retirement-lint.yml — new; lint-guard-ci-parity ran green on it in the commit hook.

Post-Merge Validation

One residual, owned elsewhere: AC-3's enforcement half is not delivered here and is carried by #17171 — the repo-wide finding that nineteen lint workflows run on dev PRs and none is a required status context. That needs a repo-admin ruleset mutation, which no PR can perform. #17171 also carries the spec that reads the live ruleset, so the gap cannot silently reopen.

Everything else is a local observable and each is armed above, including the two ACs the ticket framed as needing an incident replay — reproduced by content rather than by a commit id, which is the durable form and the one that survives the next rebase.

The first PR that legitimately retires a spec will exercise the account path in anger; that is the guard working, not a validation debt.

Commits

  • f40c97aa2a — the guard, its unit arms, the CI mirror, and the two .husky/pre-push fixes.
  • 832063e0ee — scan-root registration + both workflow triggers, after CI caught the omission.
  • 67db298a4c — @neo-gpt's re-review blocker: merges are scanned, and the combined diff is what makes that safe. Red-proved on one commit — --no-merges exits 0, the repair exits 1.
  • e3f8442364 — @neo-gpt's two exact-head blockers: the account grammar tightened from a substring test to a line-anchored one requiring content, and the enforcement claim narrowed to what the live ruleset actually supports. Guard behaviour is unchanged by the second; only the claims about it are.

Evolution

The ticket prescribed lint-staged wiring and a SHA-pinned control, and neither survived contact — not because the ticket was careless but because both were pinned to things that move: a hook phase that cannot see commit messages, and commit ids on a branch that got rebased ninety minutes later. Grace's own conclusion on the second is the durable one and it shaped AC-4 here: an incident receipt should cite content-addressable evidence — file sets, path anchors, counts — rather than branch-local commit identity.

Related: #17124 Refs #17126

Authored by Vega (Claude Opus 5, Claude Code). Session 5cd926fa-77e1-4309-8bbf-ca563ab07403.

tobiu
tobiu APPROVED reviewed on Aug 15, 2026, 2:57 PM

No review body provided.