GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — ci: point at the new warnings when an ESLint ceiling is exceeded by teeohhem · Pull Request #3607 · ClickHouse/ClickStack

Conversation

teeohhem commented Nov 10, 2026

Copy link

Copy Markdown

Contributor

Summary

When a PR pushes a package over its --max-warnings ceiling, CI fails without saying which warning is new: the "too many warnings" line is buried in hundreds of old warnings, under a summary that reads "0 errors". Pre-commit runs eslint --quiet, so warnings never show up before CI.

scripts/ci/eslint-warnings.mjs wraps the ESLint CLI to fix both:

CI (package lint scripts): when over the ceiling, lists the warnings on lines the PR changed and annotates them inline. If a type change makes a rule fire on untouched code, it lists warnings in the changed files instead. When under the ceiling, it prints the number to lower it to.
Pre-commit (lint-staged): replaces eslint --fix --quiet. Errors still block the commit. Warnings block only on lines the commit changes, so editing a file with old warnings doesn't block it.

Ceilings are unchanged.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          ci: point at the new warnings when an ESLint ceiling is exceeded

4bff623

Each package lints with `--max-warnings N`, set at or near the current
count. When a PR adds one warning, ESLint prints "too many warnings" inside
hundreds of pre-existing ones and the summary still says "0 errors", so
the failure is hard to diagnose. Pre-commit runs eslint with `--quiet`, so
warnings never surface before CI.

Add scripts/ci/eslint-warnings.mjs, a wrapper around the ESLint CLI:

- Ceiling mode (package `lint` scripts): same output and exit code as
  before, plus, when over the ceiling, a summary listing the warnings on
  lines changed since the merge-base with the PR base, and GitHub error
  annotations on those lines. Falls back to warnings in changed files when
  a type change makes a type-aware rule fire on untouched code. When under
  the ceiling, prints the count to lower it to.
- Staged mode (lint-staged): replaces `eslint --fix --quiet`. Fails on
  errors as before, and on warnings only when they sit on lines the commit
  changes, so editing a file with old warnings doesn't block the commit.

teeohhem requested a review from a team as a code owner

November 10, 2026 17:20

changeset-bot Bot commented Nov 10, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

⚠️ No Changeset found

Latest commit: c451b85

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vercel Bot commented Nov 10, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments

Project	Deployment	Actions	Updated
hyperdx-oss	Ignored	Preview	Nov 10, 2026 5:24pm UTC
hyperdx-storybook	Ignored	Preview	Nov 10, 2026 5:24pm UTC

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

teeohhem requested a review from brandon-pereira

November 10, 2026 17:20

github-actions Bot added the review/tier-3 Standard — full human review required label

Nov 10, 2026

github-actions Bot commented Nov 10, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

🟡 Tier 3 — Standard

Introduces new logic, modifies core functionality, or touches areas with non-trivial risk.

Why this tier:

Diff size: 302 production lines changed (Tier 2 max: < 250)
Cross-layer change: touches frontend (packages/app) + backend (packages/api) + shared utils (packages/common-utils)

Additional context: touches background tasks or the delivery pipeline lightly (6 lines, under the 30-line bar for Tier 4), 1 file(s) in private internal-tooling packages, excluded from the line count

Review process: Full human review — logic, architecture, edge cases.
SLA: First-pass feedback within 1 business day.

Stats

Production files changed: 8
Production lines changed: 302 (+ 115 in test files, excluded from tier calculation)
Branch: claude/lint-warning-ceiling
Author: teeohhem

To override this classification, remove the review/tier-3 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot commented Nov 10, 2026

Copy link

Copy Markdown

Contributor

Duplicate code analysis

No new duplicate code introduced by this PR.

jscpd compared this branch against origin/main
across app, api and common-utils. Only new clones are listed; the codebase
currently holds 110 in total (1.21%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vercel Bot deployed to Preview – hyperdx-oss

November 10, 2026 17:21

View deployment

github-actions Bot reviewed

Nov 10, 2026

View reviewed changes

Comment thread

scripts/ci/eslint-warnings.mjs

		results = JSON.parse(readFileSync(outFile, 'utf8'));
		} catch {
		// ESLint crashed (bad config, bad flag) and has already said why.
		process.exit(status \|\| 2);

github-actions Bot Nov 10, 2026

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

🔵 minor — process.exit inside try/finally skips the temp-dir cleanup

When ESLint crashes, process.exit(status || 2) exits before the finally block's rmSync(tmp) runs, so a /tmp/eslint-warnings-* directory is left behind on every crash. Call rmSync before exiting, or return a sentinel value and exit from main after the finally has run.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

scripts/ci/eslint-warnings.mjs

		if (staged) {
		const changed = changedLines(root, true);
		const fresh = changed
		? warnings.filter(w => changed.addedLines.get(w.filePath)?.has(w.line))

github-actions Bot Nov 10, 2026

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

🔵 minor — Staged-mode filter repeats newWarnings' line match and has no test

warnings.filter(w => changed.addedLines.get(w.filePath)?.has(w.line)) copies the onLines predicate in newWarnings. The staged branch also has no test: nothing covers onlyErrorsAnd, which keeps warnings by object identity via .raw, or the rule that errors still block. Pull the predicate into an exported helper (e.g. warningsOnLines) that both modes use. Add a test that runs onlyErrorsAnd on the results fixture and checks that errors are kept, warnings on unchanged lines are dropped, and warningCount is recomputed.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

github-actions Bot commented Nov 10, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.

🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.

Anything about surrounding code, or asking you to widen the change (hoist a helper,
dedupe with another file, fix other call sites): never fix here, whatever the
severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

4 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 4 minor

None could be anchored to changed lines; they are listed below.

Findings outside the changed lines

4 minor

🔵 scripts/ci/eslint-warnings.mjs:414 — Fallback 'files' scope annotates pre-existing warnings as "New ESLint warning" errors → When newWarnings falls back to scope: 'files', every old warning in every touched file gets an ::error ... title=New ESLint warning annotation on the PR diff (for example, editing one line of a large packages/app file with dozens of old warnings). Only annotate when scope === 'lines', or use ::warning with a title like "ESLint warning in changed file" for the fallback.
🔵 scripts/ci/eslint-warnings.mjs:389 — Staged mode diffs against HEAD, so resolving a merge conflict is blocked by warnings that came from the other branch → When you resolve a conflicted git merge main and run git commit, the pre-commit hook runs and git diff HEAD counts every line brought in from main as added. Warnings that main already allows under its ceiling now block the commit, which the old --quiet hook never did. Skip the warning gate when .git/MERGE_HEAD (or CHERRY_PICK_HEAD/REBASE_HEAD) exists, or diff against the merge-base instead of HEAD.
🔵 scripts/ci/eslint-warnings.mjs:436 — process.exit inside the try skips the finally, so the temp dir is never removed → process.exit() does not run finally blocks, so the eslint-warnings-* tmpdir leaks every time ESLint crashes. It also contradicts the comment at line 538 about process.exit dropping piped stdout. Remove the dir before exiting, or return a sentinel and let main set process.exitCode.
🔵 scripts/ci/eslint-warnings.mjs:473 — The staged-mode line filter repeats newWarnings, and the staged path has no tests → Line 474 reimplements the onLines filter from newWarnings (line 355) inside main. Because of that, the staged decision (which warnings block a commit, and the silent pass when changedLines returns undefined) and onlyErrorsAnd are never exercised by eslint-warnings.test.mjs. Extract a tested helper such as warningsOnLines(warnings, addedLines), use it in both places, and add a test for staged filtering plus onlyErrorsAnd's recomputed counts.

_{Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          ci: register scripts/ci tests as knip entries

c451b85

Knip only reads the first file after `node --test`, so it flagged the
new eslint-warnings test as unused.

github-actions Bot commented Nov 10, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

E2E Test Results

✅ All tests passed • 465 passed • 1 skipped • 1324s

Status	Count
✅ Passed	465
❌ Failed	0
⚠️ Flaky	1
⏭️ Skipped	1

Tests ran across 4 shards in parallel.

View full report →

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot commented Nov 10, 2026

Copy link

Copy Markdown

Contributor

Deep Review

✅ No critical issues found.

This is a well-constructed CI/tooling change. The core safety property holds: in ceiling mode the over-limit branch returns 1 unconditionally regardless of whether git can resolve a base, and lint errors propagate on every path (hasErrors is checked in staged, under-ceiling, and over-ceiling returns), so the lint gate cannot falsely pass. git-resolution failures degrade gracefully (ceiling mode still fails; staged mode falls back to blocking on errors only, which is acceptable for a best-effort pre-commit hook).

🟡 P2 — recommended

scripts/ci/__tests__/eslint-warnings.test.mjs:1 — The new tests cover only the pure helpers (parseArgs, parseAddedLines, collectWarnings, newWarnings), leaving the safety-critical pass/fail decision in main() and the count-recomputation in onlyErrorsAnd with no coverage.
- Fix: Extract the ceiling/staged pass-fail decision into a pure function and add tests for the over-ceiling, under-ceiling, errors-present, and staged fresh-warning cases.

🔵 P3 nitpicks (2)

.github/workflows/main.yml:57 — The run: step is a plain multi-line YAML scalar that folds into a single node --test ratchet.test.mjs eslint-warnings.test.mjs command; it is correct but reads as two separate commands and is easy to break on a future edit.
- Fix: Use a block scalar (run: |) with one explicit node --test line naming both files, or keep both files on a single line.
scripts/ci/eslint-warnings.mjs:129 — changedLines hardcodes origin/main as the base when GITHUB_BASE_REF is unset, so non-PR runs silently compare against that branch.
- Fix: Add a short comment noting the non-PR fallback target so the default is discoverable.

Reviewers (5): correctness, adversarial, testing, maintainability, project-standards.

Testing gaps: main() gate decision and onlyErrorsAnd count recomputation are untested; changedLines git-failure fallback paths are not exercised.

_{No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

teeohhem added the automerge label

Nov 10, 2026

This branch was successfully deployed

1 active (outdated) deployment

Preview – hyperdx-oss — 4bff6237 Deployed Nov 10, 2026 by vercel[bot]

View deployment

Reviewers

github-actions[bot]

github-actions[bot] left review comments

brandon-pereira

Awaiting requested review from brandon-pereira

At least 1 approving review is required to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

automerge review/tier-3

Standard — full human review required

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

1 participant

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

teeohhem commented Nov 10, 2026

Summary

Uh oh!

changeset-bot Bot commented Nov 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

vercel Bot commented Nov 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Nov 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🟡 Tier 3 — Standard

Uh oh!

github-actions Bot commented Nov 10, 2026

Duplicate code analysis

Uh oh!

github-actions Bot Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Nov 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review

Findings outside the changed lines

Uh oh!

github-actions Bot commented Nov 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

E2E Test Results

Uh oh!

github-actions Bot commented Nov 10, 2026

Deep Review

🟡 P2 — recommended

Uh oh!

This branch was successfully deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

changeset-bot Bot commented Nov 10, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

vercel Bot commented Nov 10, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot commented Nov 10, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot commented Nov 10, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot commented Nov 10, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.