Skip to content

fix: drop an unused binding to bring eslint warnings back under the ceiling - #7745

Closed
CrysisDeu wants to merge 1 commit into
mainfrom
fix/eslint-warning-ceiling
Closed

fix: drop an unused binding to bring eslint warnings back under the ceiling#7745
CrysisDeu wants to merge 1 commit into
mainfrom
fix/eslint-warning-ceiling

Conversation

@CrysisDeu

@CrysisDeu CrysisDeu commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Problem / Motivation

The Frontend Lint gate runs npx eslint src/ --max-warnings 603, but a clean checkout of main measures 604 warnings: an unused const s = await shell() binding (website/src/test/issueRadarNarrowViewport.test.ts:167) landed while a string of backend-only merges skipped the Frontend Lint job entirely, so main's own CI never surfaced it.

Why it matters

Every frontend-touching PR currently inherits a red Frontend Lint & Type Check regardless of its own diff (confirmed on PR #7731: its two changed src files carry zero warnings, yet the job fails at 604 > 603). Until this lands, no frontend PR can go green.

What changed (motivation → approach → change)

The gate's own comment says the ceiling must EQUAL the measured count — so fix the count, don't raise the ceiling. Drop the unused binding, keep the shell() call (its render is the test's setup). npx eslint src/ measures exactly 603 again.

Tests

No behavior change; the touched test file still passes (16/16). The lint gate itself is the pin.

Manual verification

npx eslint src/ on this branch: 604 → 603 warnings, 0 errors. Verified the 604 reproduces on pristine origin/main (7126fa1) before the fix.

Why no screenshot: test-only unused-binding removal; no rendered surface changes.

no linked issue: gate breakage found during PR #7731's babysit; fix is one line, filed directly.

Pattern harvest

  • Root-cause class: ratchet-gate blind spot — a count-pinned gate (--max-warnings 603) only runs when its path filter matches, so a violation that lands alongside frontend changes survives as long as every SUBSEQUENT merge is backend-only; the breakage then surfaces on the next unrelated frontend PR instead of the culprit.
  • Detection gap: main's own CI conclusion stayed green across the window because the Frontend Lint job was skipped, not passed — a skipped required-context reads the same as green in the branch-level rollup.
  • Harvest: when a frontend PR inherits a ratchet red its own files cannot explain, measure the pristine base first (npx eslint src/ on origin/main) before touching the branch; and consider running count-pinned ratchets on a low-frequency schedule against main so a skip window cannot hide a regression.

Rule candidate: a count-pinned ratchet gate (--max-warnings N, baseline ceilings) should also run on a scheduled workflow against main, not only on path-filtered PR triggers — a skip window must not be able to hide a regression until an unrelated PR inherits it.

…eiling

The Frontend Lint gate pins --max-warnings 603, but main's tree measures
604: an unused `const s = await shell()` binding landed while a string
of backend-only merges skipped the lint job, so nothing surfaced it.
Every frontend PR now inherits the red.

Keep the shell() call (its render is the setup), drop the binding the
test never reads. Warnings measure exactly 603 again — the ceiling
equals the count, per the gate's own comment.
@CrysisDeu
CrysisDeu requested a review from a team as a code owner September 1, 2026 22:26
@CrysisDeu
CrysisDeu requested a review from dwu96 September 1, 2026 22:26
@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 b85599221cb74bfcf54615453a1bde21b613c070 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

UX-Verdict: PASS

One-line test-only cleanup (unused binding removed); no user-facing surface, string, or rendered pixel changes in this PR.

[UX-REVIEWED] b855992

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] b855992

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

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

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

One-line removal of the exact warning that broke main's ratchet, following the gate's own "ceiling equals measured count" rule — correct shape, minimal blast radius.

Suggestions

  • The recurrence mechanism (a frontend warning merging while the path-filtered Frontend Lint job never ran) is untouched; file a follow-up to make the ratchet un-skippable for any website/-touching merge, or the 603 pin drifts again.

[DESIGN-REVIEWED] b855992

@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 b85599221cb74bfcf54615453a1bde21b613c070 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] b855992

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

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Status note for the merger: the backend reds here (test_security_posture.py gate-side redactor census, Windows(3) + 3.10(4) shards) are main-inherited — this branch is origin/main plus one frontend test line and cannot reach slack/gateway.py. The census fix is already in flight as #7756, which in turn inherits the eslint 604>603 red THIS PR fixes. Cross-inherited pair: merging either one first greens the other after a rebase/rerun; neither can show a fully green board until then.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 1, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Closing — contained byte-for-byte inside #7569

Verified relationship: functional overlap

Verified independently, not from the earlier finding: #7745's entire diff is 13 lines, one file, one hunk in website/src/test/issueRadarNarrowViewport.test.ts turning const s = await shell() into await shell(), and that exact hunk with the same index line (a67439310f..120a8d29e1) sits inside #7569's 194-file diff. git rev-parse on the file gives origin/main a67439310f (the binding is still there), pr/7569 120a8d29e1 and pr/7745 120a8d29e1 -- identical post-images. The purpose is identical on both sides too: remove that one @typescript-eslint/no-unused-vars warning from the count the frontend-lint --max-warnings gate measures. Not a stacked branch: one commit each (c733254 / b855992), different merge-bases (63a043a / 13b0d28), neither ref an ancestor of the other. The relationship is asymmetric -- #7569 landing leaves #7745 with a literally empty diff, #7745 landing leaves #7569 with 193 files and the ceiling change -- which fails the duplicate symmetric bar and meets functional overlap's 'one is a strict superset of the other'. It is not independent (merely co-located), because this is not one file touched twice for independent reasons; it is the same single-line edit made for the same reason producing the same bytes. the first adjudication's ruling and its nomination direction both hold: #7569 does not refuse the capability #7745 exists to deliver, it contains it and 598 more. But #7569 cannot land as it stands, so it is a REBASE rather than a KEEP: its ci.yml hunk deletes a --max-warnings 599 line that no longer exists (main reads 597), its eslint.config.js src/**/*.mjs block exists to make live an eslint-disable-next-line no-eval directive that merged #7753 (4593b7d) deleted from crew-ghost-sprite.gen.mjs, main's FindingRow.tsx:40 already carries the role="presentation" it re-suppresses, its replacement ceiling comment re-adds the SHA-and-PR narration #7753 stripped as AGENTS.md-forbidden, and both human reviews are DISMISSED with the Design lane asking that the ChatPage.tsx close() fix be split into its own fix: PR.

Carry this over first

This closure is about redundancy, and these items are the exception: they are not on main and not in the surviving PR, so they need a home before the topic is finished. Please don't let them go with the branch.

No code. The one line is already byte-identical inside #7569 (website/src/test/issueRadarNarrowViewport.test.ts, the test 'names the LIST in the Back control, not one item from it': const s = await shell() -> await shell(), both heads resolving to blob 120a8d29e1). Two non-code items to carry before closing #7745: (1) its Pattern-harvest rule candidate -- a count-pinned ratchet (--max-warnings N, baseline ceilings) should also run on a scheduled workflow against main, so a path-filter skip window cannot hide a regression until an unrelated PR inherits it. #7569's body argues the adjacent but different push: [main] post-merge job, so this belongs in whichever PR lands or in a follow-up issue. (2) A conditional: if #7569 is split per the Design lane's request and the split drops the test-file hunk, that one-line removal must be re-made PAIRED with --max-warnings 596, because on today's main the binding is counted inside the green 597 and removing it alone opens the slack the gate's own comment forbids.

Current state

Neither side has merged -- the issue/PR reference check and the open-PR list both show #7569 and #7745 OPEN, so superseded by work already on main does not apply between them. Landed work does however dissolve #7745's stated urgency: merged 4593b7d (#7753) re-measured the gate to --max-warnings 597 with the unused binding still present (git rev-parse 4593b7d:website/src/test/issueRadarNarrowViewport.test.ts = a67439310f, same as main today), so frontend-lint is green on main and #7745's premise that 'no frontend PR can go green' is stale. That is partial coverage of #7745's motivation, not of its diff: the binding itself is still on main, which is precisely why #7569 still carries the hunk.


From a repository-wide duplicate/overlap audit of every pull request open against main (2026-09-02, 330 PRs, one reviewer per PR). Each PR was read as its full merge-base diff plus its description and every comment and review, then compared against each candidate PR's own diff and against origin/main at 1a765b88ceb7. Findings that implied a closure were re-adjudicated independently, including an adversarial pass whose only job was to refute them; the reasoning above is what survived. If it is wrong, reopening costs nothing — please say so, and treat the reasoning rather than the outcome as the thing to correct.

@bolichen97 bolichen97 closed this Sep 2, 2026
@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Sep 2, 2026
@bolichen97
bolichen97 deleted the fix/eslint-warning-ceiling branch September 6, 2026 03:56
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.

2 participants