Skip to content

fix(ci): drop a dead eslint directive and re-equal the warning ceiling - #7753

Merged
chenmingwei23 merged 1 commit into
mainfrom
fix/frontend-lint-warning-ceiling
Sep 2, 2026
Merged

fix(ci): drop a dead eslint directive and re-equal the warning ceiling#7753
chenmingwei23 merged 1 commit into
mainfrom
fix/frontend-lint-warning-ceiling

Conversation

@bolichen97

@bolichen97 bolichen97 commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Problem / Motivation

Two pieces of frontend lint debt on main, both invisible to the gate that is supposed
to catch them.

1. A dead eslint directive.

website/src/apps/issue-radar/components/crew-ghost-sprite.gen.mjs
  173:5  warning  Unused eslint-disable directive (no problems were reported from 'no-eval')

2. The ceiling sits above the measured count. On current main the gate reads
--max-warnings 599 while cd website && npx eslint src/ measures 598 — one free
slot. That is exactly the state the gate's own comment forbids:

The ceiling must EQUAL the measured count, not sit above it: slack is silent
admission
, and a warning that lands inside it never surfaces again.

A free slot means the next real warning lands without ever failing a check.

Why it matters

The ratchet only works while the ceiling tracks the count. With slack in it, the check
reports green through the first regressions and the ratchet silently stops being one —
and the ceiling is the only thing standing between the tree and unbounded warning
growth, since none of these rules is an error.

The dead directive is the smaller half, but it is also the more misleading one: it reads
as a deliberate, load-bearing suppression, and it suppresses nothing.

What changed (motivation → approach → change)

The directive was never functional. Every rule block in website/eslint.config.js is
scoped to src/**/*.{ts,tsx}, so no rule is enabled for a .mjs file — no-eval
included — and the directive could never suppress anything. Replaced it with a plain
comment that keeps the intent (why eval is safe in that generator: program is a
template literal built in the same file and handed to the Playwright page so the browser
context can call drawCrewGhost — nothing remote or user-supplied) and records the
condition under which the directive should come back.

Then re-measured and set the ceiling to the count, 599 → 597, which is the
direction the comment mandates. It is not a raise: it is lower than both the old ceiling
and the count before this change. 597 is measured, not derived — --max-warnings 597
exits 0 and --max-warnings 596 fails.

The ceiling comment loses its transient half. The paragraph above the run: line
narrated a specific earlier 604/603/599 episode, complete with a commit SHA and three PR
numbers. Those numbers are wrong the moment the ceiling moves — which this PR does — and
the root AGENTS.md comment rule forbids that shape outright ("NOT a task log: no PR/CR
numbers … or commit SHAs"). The durable half, the EQUAL-the-count instruction and how to
re-measure, stays verbatim.

Rejected alternative. Adding a src/**/*.mjs rule block would make the directive
used and also drop the warning — and would extend lint coverage to a file currently
linted by nothing. But it brings the generator's four console calls in as no-console
warnings, so it needs the ceiling raised, which the comment forbids. Probed and
reverted.

Tests

test/test_eslint_warning_ceiling.py already pins the two things that can be pinned
without running eslint: that ci.yml declares exactly ONE --max-warnings literal, and
that no prose transcribes its value. Neither can measure the count, and the module's own
docstring says so — "Only running eslint can measure that, so it is not pinned here --
the gate itself is that test." That gate, npx eslint src/ --max-warnings 597, is the
assertion for this change: with the ceiling at the count, the next added warning fails
it.

Manual verification

Ran the frontend job's gates verbatim from ci.yml:

origin/main this branch
npx eslint src/ 598 problems (0 errors, 598 warnings) 597 problems (0 errors, 597 warnings)
npx eslint src/ --max-warnings 597 would fail (598 > 597) pass (rc=0)
npx eslint src/ --max-warnings 596 would fail fails (ceiling has no slack)
npx tsc -b pass pass
npx jscpd . pass (0 clones) pass (0 clones)

.github/workflows/ci.yml parses as YAML, and
test/test_eslint_warning_ceiling.py test/test_security_posture.py are green (49 passed)
— including TestGateSideLogRedactorSpelling, whose shard-3 red on the previous head was
the inherited baseline-redactor census cascade and is cured by the rebase, with no code
change.

The generator is not itself generated — its header documents running it by hand — so
nothing regenerates the removed line back.

Screenshots / video

Why no screenshot: the only website/src change is a comment inside
crew-ghost-sprite.gen.mjs, a hand-run Node generator that is never bundled or
rendered. No component, layout, style, or user-visible string changes, and the committed
sprite sheet it produces is byte-identical.

Related Issues

no linked issue: lint-count drift against the gate's own ceiling rule; there is no
tracked issue for it.

Pattern harvest

Rule candidate: lint
Pattern: an eslint-disable directive in a file that no config block covers is silently
dead — it suppresses nothing, and the unused-directive warning it produces is charged to
the --max-warnings ratchet instead of failing where it was written. Every rule block in
website/eslint.config.js is scoped to src/**/*.{ts,tsx}, so any directive in a
src/**/*.{js,mjs,cjs} file is in this class. Promoting unused disable directives from
warn to error (linterOptions.reportUnusedDisableDirectives) would surface the next
one where it is written rather than months later as an unattributable ratchet overflow.

Second, narrower candidate for whoever owns the gate: the ceiling and the count can only
drift apart silently, because a lower count never fails anything. A CI step asserting
equality — rather than <= — would catch each slot at the moment it opens. Both are
additions to the gate rather than parts of this fix, so both are deferred here.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

@bolichen97
bolichen97 requested a review from a team September 1, 2026 22:46
@bolichen97
bolichen97 requested a review from a team as a code owner September 1, 2026 22:46
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

UX-level review of 998c4f87c9941216ff2e6553bd9558380820b6de — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

No user-facing surface in this PR — it changes a CI eslint ceiling and a comment in a build-time sprite generation script. No UI, strings, or screenshots to review.

UX-Verdict: PASS

No user-facing surface: the diff touches only a CI warning ceiling and a build-script comment — nothing a product user ever sees.

[UX-REVIEWED] 998c4f8

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 998c4f87c9941216ff2e6553bd9558380820b6de — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Both hunks verified against the description; the numbers reconcile (598 measured on main, minus the unused-directive warning = 597, ceiling set to 597 with a one-lower failure probe). The rejected alternative (bringing .mjs under a rule block) was considered and reasonably declined within the ratchet's own no-raise constraint, and the replacement comment records the re-add condition. Nothing undocumented in the diff, no contract or reversibility exposure — this is a two-line ratchet tightening that restores the gate's stated invariant.

Design-Verdict: PASS

Ratchet slack removed and a dead suppression replaced with its rationale — measured, falsified one-lower, no design-level concerns.

[DESIGN-REVIEWED] 998c4f8

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 998c4f87c9941216ff2e6553bd9558380820b6de and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 998c4f8

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 998c4f87c9941216ff2e6553bd9558380820b6de: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 998c4f87c9941216ff2e6553bd9558380820b6de — this comment is updated in place on each push.

Review details

No findings.

The single candidate rests on an unresolved conditional — whether the head tree measures 598 warnings (making the new --max-warnings 597 ceiling one too low). I confirmed the flat config in eslint.config.js scopes every rule block to src/**/*.{ts,tsx}, so the removed // eslint-disable-next-line no-eval on the .mjs file was indeed an unused-directive warning and its removal drops the count by exactly 1. But the base measured count is not statically knowable: the base ceiling of 599 does not prove the base measured 599 (the repo's own history documents ceiling/measured drift), and this environment has no network to install eslint and measure. Outcome (c) therefore stays a "if the count is 598, CI fails," which the falsification bar rejects — and the only outcome even in the bad case is a self-announcing red CI check on the very gate being edited, not a shipping defect.

[OPUS-REVIEWED] 998c4f8

Verdict parsed from the review's SHA-scoped output markers for commit 998c4f87c9941216ff2e6553bd9558380820b6de.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 998c4f87c9941216ff2e6553bd9558380820b6de: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 998c4f87c9941216ff2e6553bd9558380820b6de — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All verification is done. The change is a pure subtraction with every claim checked against the repo: the eslint config truly scopes every rule block to src/**/*.{ts,tsx} (so the directive was dead), the ceiling-equals-count invariant is written in ci.yml itself, the deleted narration paragraph is exactly the shape AGENTS.md's comment rule forbids, and a grep for sibling dead directives in the linted tree found zero.

First-Principles-Verdict: PASS

Every item deletes something — a dead suppression, two slots of silent admission, a stale task-log paragraph — and each deletion is mandated by an invariant the repo already wrote down.

What this change ships

Intent: close the two-warning slack in the frontend lint ratchet and remove the dead directive that produced part of it. FIX.

  1. Lint gate ceiling drops 599 → 597, equal to the measured count — justified (the gate's own comment mandates EQUAL, not above).
  2. Dead no-eval disable directive deleted from the sprite generator — justified; verified dead (every rule block in website/eslint.config.js is scoped to src/**/*.{ts,tsx}, so no rule reaches a .mjs file).
  3. Directive replaced by a rationale comment stating why eval is safe and when to re-add the directive — justified (states constraints the code can't show, per the AGENTS.md comment rule).
  4. ci.yml ceiling comment loses its 604/603/599 episode narration with a SHA and three PR numbers — declared; deletion mandated by AGENTS.md ("no PR/CR numbers … or commit SHAs").

No new surface, key, flag, or behavior branch ships, so there is nothing to consumer-count. Siblings of the root cause: grepped eslint-disable across website/src/**/*.{js,mjs,cjs} — 0 remaining in the linted tree. Transcriptions of the new ceiling value outside ci.yml: 0. The two cause-level gate hardenings (unused-directive-as-error, ceiling-equality assert) are named and deferred in the description itself; both are additions to the gate and correctly not part of this fix.

[FIRST-PRINCIPLES-REVIEWED] 998c4f8

@bolichen97
bolichen97 force-pushed the fix/frontend-lint-warning-ceiling branch from 1b36813 to 2ad6856 Compare September 1, 2026 23:22
@bolichen97 bolichen97 changed the title fix(ci): drop the unused no-eval directive that put main over its lint ceiling fix(ci): drop a dead eslint directive and re-equal the warning ceiling Sep 1, 2026
@github-actions github-actions Bot added merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 2, 2026
Two separate pieces of lint debt on `main`, both invisible to the gate:

1. `crew-ghost-sprite.gen.mjs` in `website/src/apps/issue-radar/`
   carries an `eslint-disable-next-line no-eval` that ESLint reports as
   unused. It always was: every rule block in `website/eslint.config.js`
   is scoped to `src/**/*.{ts,tsx}`, so no rule is enabled for a `.mjs`
   file and the directive could never suppress anything.

2. The ceiling sits above the measured count: `--max-warnings 599`
   against 598 measured. That is precisely the state the gate's own
   comment forbids -- "The ceiling must EQUAL the measured count, not
   sit above it: slack is silent admission, and a warning that lands
   inside it never surfaces again" -- and the free slot admits the next
   real warning without ever failing a check.

Replaced the dead directive with a plain comment that keeps the intent,
why `eval` is safe in that hand-run generator, and records when the
directive should come back. Then re-measured and set the ceiling to the
count: 597, with `--max-warnings 597` exiting 0 and one lower failing,
so the gate carries no slack.

Also drops the transient half of the ceiling comment, which narrated an
earlier episode by commit SHA and PR number. Those numbers go wrong as
soon as the ceiling moves, which this change does, and AGENTS.md forbids
that shape in a comment. The EQUAL-the-count instruction and the command
that re-measures it stay verbatim.

Rejected alternative: adding a `src/**/*.mjs` rule block would make the
directive used and also drop the warning, and would extend lint coverage
to a file currently linted by nothing -- but it brings the generator's
four `console` calls in as `no-console` warnings, so it needs the
ceiling raised.
@bolichen97
bolichen97 force-pushed the fix/frontend-lint-warning-ceiling branch from 2ad6856 to 998c4f8 Compare September 2, 2026 05:35
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 2, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Subtractions — shrink the replacement comment in crew-ghost-sprite.gen.mjs

Disposition: fixed.

Shrink the replacement comment in crew-ghost-sprite.gen.mjs to the eval-safety rationale plus the re-add condition: the middle sentences ("That warning consumed a slot… so it blocked every PR") are past-tense incident narration the root AGENTS.md comment rule forbids ("NOT a task log", no "previously" narration) — and the PR's own description says #7722 already cleared that blockage, so the sentence is stale on arrival.

Legitimate and proportional, so it is fixed in the single commit. The rule is explicit
and this comment was in breach of it: a comment must state current behaviour in the
present tense and is "NOT a task log". The incident sentences are gone. What is left is
the two things a future reader of that line actually needs:

  1. Why eval is safe hereprogram is the drawing source built as a template
    literal in the same file and handed to the Playwright page so the browser context can
    call drawCrewGhost; nothing there is remote or user-supplied.
  2. Why no directive belongs there, and when one would — every rule block in
    eslint.config.js is scoped to src/**/*.{ts,tsx}, so no rule is enabled for a
    .mjs file and a directive is charged to the ceiling instead of suppressing
    anything; re-add it if a config block ever brings .mjs under a preset that turns
    no-eval on.

Both clauses are present-tense conditions that stay true independently of any past
episode, which is the property the narration lacked.

No test was added, because no test pins comment prose and inventing one would pin the
wording rather than the behaviour. The change this PR actually asserts is unaffected:
npx eslint src/ --max-warnings 597 exits 0 and 596 fails, so the ceiling still
carries zero slack.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Suggestion 1 — the one-line equality assertion belongs in this ci.yml hunk, not a follow-up

Disposition: rebutted (the concern is real; the prescribed remedy is rejected).

The comment-only invariant ("ceiling must EQUAL the count") has now drifted twice (#7722 and here); the one-line equality assertion your own Pattern harvest names belongs in this ci.yml hunk, not a follow-up.

The concern is correct and is exactly what this PR fixes for the current value. The
ceiling and the count can only ever drift apart silently, because a count below the
ceiling fails nothing. That is why the slack existed to be found.

But there is no "one-line equality assertion" available, and the repo already recorded
why.
The value cannot be asserted without running eslint, so nothing in the Python test
suite can hold it — test/test_eslint_warning_ceiling.py's own module docstring says so
verbatim:

Only running eslint can measure that, so it is not pinned here -- the gate itself is
that test.

So the remedy is not a line, it is a new gate: a second --max-warnings <n-1> run in
the frontend job that must fail, doubling the eslint pass on every PR, plus the
must-fail-inverted plumbing to interpret it. And it has a consequence the suggestion does
not price in: with equality enforced, every future warning reduction turns main
instantly red
until the ceiling is bumped in the same commit. The ratchet this PR is
defending exists to make burning warnings down cheap; an equality gate makes each
burn-down a two-file coordinated change or a broken build. That trade may still be worth
making — it is a gate-design decision for whoever owns the gate, not a rider on a
two-file fix that removes one dead directive and re-equals one number.

The First Principles lane reached the same conclusion independently on this same head:

The deeper causes (promote unused directives to error; assert ceiling==count) are named and deferred in the description; both would be additions, so deferral is the right call here.

The deferral is now stated explicitly in the PR body's Pattern harvest rather than left
implicit, so the next person to touch the gate inherits the reasoning instead of
rediscovering it.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Suggestion 2 — set linterOptions.reportUnusedDisableDirectives: 'error' in website/eslint.config.js

Disposition: rebutted as out of scope for this change, not as unsound.

linterOptions.reportUnusedDisableDirectives: 'error' in website/eslint.config.js would fail the next dead directive at its source instead of charging the ratchet — a one-line change that closes the class this PR fixed one instance of.

Correcting the record first: the promotion would be green today, not red. Measured on
the rebased tree, npx eslint src/ --max-warnings 0 reports 597 warnings and
grep -c "Unused eslint-disable" over that output is 0 — after this PR there are no
unused directives left anywhere in the linted tree. The two i18n scans are unaffected
because they use their own configs: website/eslint.i18n.config.js already sets
reportUnusedDisableDirectives: 'off', and website/scripts/check-i18n-strings.mjs runs
with allowInlineConfig: false. So the suggestion carries no hidden breakage.

It is still declined here, for scope. It changes the severity contract for all of
website/src — promoting a warn-class report to error-class across the whole tree — which
is a lint-policy decision belonging to whoever owns that gate. In this PR it buys nothing
measurable: there is no second dead directive for it to catch, so it would land as an
unexercised policy change in a third file, inside a change whose entire purpose is one
dead directive plus one re-equalled number. Design's verdict on this head is PASS and the
item is advisory with no [BLOCK-MERGE] marker, so nothing here is being waived past a
block.

The mechanism is recorded as the named rule candidate in the PR body's Pattern harvest, so
the next person to look at the class has the diagnosis rather than the symptom. The First
Principles lane independently agreed on the same head that both deeper causes belong
outside this change:

The deeper causes (promote unused directives to error; assert ceiling==count) are named and deferred in the description; both would be additions, so deferral is the right call here.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 2, 2026
@chenmingwei23
chenmingwei23 enabled auto-merge (squash) September 2, 2026 07:07

@chenmingwei23 chenmingwei23 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tier 1 auto-approve: chore/build (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: CI plumbing - lowers eslint --max-warnings ceiling to the measured count and removes a dead eslint-disable directive in a .mjs file (no rule enabled there); no runtime impact.

@chenmingwei23
chenmingwei23 merged commit 4593b7d into main Sep 2, 2026
111 of 119 checks passed
@chenmingwei23
chenmingwei23 deleted the fix/frontend-lint-warning-ceiling branch September 2, 2026 07:08

@iamwhatever iamwhatever left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tier 1 auto-approve: chore / build tooling (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: CI plumbing only -- removes an inert eslint-disable-next-line no-eval from a .mjs build-time generator (eslint.config.js scopes every rule to src/**/*.{ts,tsx}, so the directive itself was charged against the ceiling) and lowers --max-warnings 599 -> 597 to re-equal the measured count. The eval(program) call is unchanged and the ceiling moves down, not up.

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.

3 participants