chore(ci): escalate substantial query-engine changes to review tier 4 by wrn14897 · Pull Request #2871 · hyperdxio/hyperdx

wrn14897 · 2026-08-12T01:52:07Z

Why

The PR triage classifier has no critical-path coverage for packages/common-utils — the SQL rendering + execution engine that every chart, search, and alert query flows through. A substantial change there can silently alter query semantics product-wide, yet #2859 (664 prod lines moving multi-series metric merge computation into ClickHouse) tiered on generic size/cross-layer rules as Tier 3.

What

Adds a fourth critical band, QUERY_ENGINE_CRITICAL_PATTERNS:

packages/common-utils/src/core/renderChartConfig.*
packages/common-utils/src/core/builderToRawSql.*
packages/common-utils/src/clickhouse/

using the same total-churn escalation mechanism as the infra band, but with a 150-line bar (QUERY_ENGINE_CRITICAL_MIN_LINES).

Why 150 and not infra's 30

Calibrated against 20 recent merged PRs touching these files (prod-only churn):

Bar	PRs that would flip to Tier 4
≥30 (infra bar)	10/20 — including routine Tier 2 chart fixes (#2759, #2613, #2422, #2487)
≥150	Only engine-level rewrites: #2246 (Increase aggFn), #2634 (builder→raw SQL), #2859

These files are among the hottest in the repo — routine chart fixes graze them weekly — so the bar is set high enough that only changes warranting a domain expert escalate.

Also

Fixed a stale test fixture path (src/renderChartConfig.ts → src/core/renderChartConfig.ts; the file moved after the test was written)
Updated Tier 4 description text and comment triggers/context lines

Verification

node --test .github/scripts/__tests__/pr-triage-classify.test.js — 112 tests pass (10 new)
End-to-end replay of [HDX-5077] Move multi-series metric merge computation to ClickHouse #2859's actual file list now yields Tier 4:

Query rendering engine substantially modified — 664 lines (bar: 150). Every chart, search, and alert query flows through this code

No changeset: CI/internal tooling only.

The SQL rendering + execution pipeline in common-utils (renderChartConfig, builderToRawSql, clickhouse/) was absent from every critical-path band, so PRs like #2859 — 664 prod lines rewriting how multi-series metric merges are computed — tiered on generic size/cross-layer rules as Tier 3. Add a fourth critical band with the same total-churn mechanism as the infra band but a 150-line bar: calibrated against 20 recent merged PRs, infra's 30-line bar would have escalated half of them (including routine Tier 2 chart fixes), while 150 catches only engine-level rewrites (#2246, #2634, #2859). Also fixes a stale fixture path (renderChartConfig.ts moved to core/).

changeset-bot · 2026-08-12T01:52:10Z

⚠️ No Changeset found

Latest commit: 6f9eeb1

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

vercel · 2026-08-12T01:52:13Z

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

Project	Deployment	Actions	Updated (UTC)
hyperdx-oss	Ready	Preview	Aug 12, 2026 2:50pm
hyperdx-storybook	Ready	Preview	Aug 12, 2026 2:50pm

github-actions · 2026-08-12T01:52:34Z

🟢 Tier 1 — Trivial

Docs, images, lock files, a dependency bump, or an automated release. No functional code changes detected.

Why this tier:

All files are docs / images / lock files

Review process: Auto-merge once CI passes. No human review required.
SLA: Resolves automatically.

Stats

Production files changed: 0
Production lines changed: 0 (+ 111 in test files, excluded from tier calculation)
Branch: warren/revisit-review-tier-ranking
Author: wrn14897

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

greptile-apps · 2026-08-12T01:54:53Z

Greptile Summary

The PR adds a dedicated query-engine critical-path band that escalates at 150 lines of aggregate production churn.

Classifies the SQL rendering and ClickHouse execution paths as query-engine-critical.
Integrates the new band into tier computation and generated review explanations.
Adds coverage for threshold boundaries, aggregation, test-file exclusion, and representative historical PR shapes.
Corrects the moved renderChartConfig.ts fixture path.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
.github/scripts/pr-triage-classify.js	Adds query-engine path detection, aggregate churn escalation, exported signals, and corresponding Tier 4 comment text without an eligible blocking issue.
.github/scripts/tests/pr-triage-classify.test.js	Extends classifier coverage for path matching, threshold boundaries, aggregation, exclusions, and generated explanations.

_{Reviews (2): Last reviewed commit: "Merge branch 'main' into warren/revisit-..." | Re-trigger Greptile}

github-actions · 2026-08-12T01:57:33Z

Deep Review

Scope: Single commit touching .github/scripts/pr-triage-classify.js (+69/−13 prod) and .github/scripts/__tests__/pr-triage-classify.test.js. Pure CI-triage tooling; no application, security, API, or data-layer surface. (The supplied base SHA was ~2049 commits stale, so review was scoped to the PR's actual single commit, HEAD^..HEAD.)

Intent: Add a fourth query-engine critical band that escalates substantial changes to packages/common-utils query-rendering/execution files to review Tier 4 once total churn reaches QUERY_ENGINE_CRITICAL_MIN_LINES (150), mirroring the existing infra band.

✅ No critical issues found. The new band reuses the proven infra-band mechanism, is additive-only (it can push PRs up to Tier 4 but never down-tiers, since criticalFiles only grows), the >= 150 boundary is correct and tested at 149/150, band-exclusivity guards are present, and all three regex patterns were verified against real files on disk (packages/common-utils/src/core/renderChartConfig.ts, builderToRawSql.ts, and packages/common-utils/src/clickhouse/). Test coverage for the new behavior is strong: boundary, cross-file aggregation, test-file exclusion, exclusivity, and both buildTierComment branches are all asserted.

🟡 P2 -- recommended

.github/scripts/pr-triage-classify.js:282 -- The query-engine band escalates purely on churn volume with no small-but-dangerous safeguard, so a compact semantic change to renderChartConfig (e.g. ~40 lines that alter query filtering or aggregation) stays Tier 2/3 and skips the domain-expert review this band exists to trigger; unlike the security band (any size) and the core band (graze guard), a sub-150-line engine edit gets no escalation at all.
- Fix: Accept the churn-only bar as the documented calibration trade-off, or add a narrow keyword/path signal (e.g. changes to aggregation or filter construction) so a high-risk small edit can still escalate.

🔵 P3 nitpicks (3)

.github/scripts/pr-triage-classify.js:58 -- The ^packages/common-utils/src/clickhouse/ pattern matches the entire ClickHouse client directory (browser.ts, node.ts, index.ts), which is broader than the "SQL rendering + execution pipeline" framing in the adjacent comment.
- Fix: Confirm the whole clickhouse/ directory is the intended scope, and adjust the comment or the pattern so they agree.
.github/scripts/pr-triage-classify.js:285 -- Four critical bands now repeat the same filter → churn → escalate → trigger → context shape, so a future fifth band requires coordinated edits across ~8 sites (pattern const, threshold const, helper, computeSignals filter, criticalFiles spread, two buildTierComment blocks, module.exports, and destructure defaults).
- Fix: When a fifth band is next added, extract the threshold-based bands into a small table-driven helper rather than copying the block again.
.github/scripts/__tests__/pr-triage-classify.test.js:207 -- The non-index.ts ClickHouse paths (clickhouse/browser.ts, clickhouse/node.ts) and a builderToRawSql-only escalation are matched by the patterns but never asserted directly, so a future narrowing of the regex could regress them silently.
- Fix: Add a case asserting isQueryEngineCriticalFile for clickhouse/browser.ts/node.ts and a builderToRawSql-only churn escalation.

Reviewers (7): correctness, adversarial, testing, maintainability, project-standards, agent-native, learnings-researcher.

Testing gaps: New-behavior coverage is thorough; the only untested matched paths are the non-index.ts ClickHouse files and an isolated builderToRawSql escalation (see P3).

github-actions · 2026-08-12T01:59:45Z

E2E Test Results

✅ All tests passed • 280 passed • 1 skipped • 939s

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

Tests ran across 4 shards in parallel.

View full report →

github-actions Bot added the review/tier-1 Trivial — auto-merge candidate once CI passes label Aug 12, 2026

teeohhem approved these changes Aug 12, 2026

View reviewed changes

wrn14897 added the automerge label Aug 12, 2026

Merge branch 'main' into warren/revisit-review-tier-ranking

6f9eeb1

vercel Bot deployed to Preview – hyperdx-storybook August 12, 2026 14:47 View deployment

vercel Bot deployed to Preview – hyperdx-oss August 12, 2026 14:50 View deployment

kodiakhq Bot merged commit fce671b into main Aug 12, 2026
27 checks passed

kodiakhq Bot deleted the warren/revisit-review-tier-ranking branch August 12, 2026 14:52

Navigation Menu

Uh oh!

chore(ci): escalate substantial query-engine changes to review tier 4 - #2871

chore(ci): escalate substantial query-engine changes to review tier 4#2871
kodiakhq[bot] merged 2 commits into
mainfrom
warren/revisit-review-tier-ranking

wrn14897 commented Aug 12, 2026

Uh oh!

changeset-bot Bot commented Aug 12, 2026 •

edited

Loading

Uh oh!

vercel Bot commented Aug 12, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 12, 2026

Uh oh!

greptile-apps Bot commented Aug 12, 2026 •

edited

Loading

Important Files Changed

Uh oh!

github-actions Bot commented Aug 12, 2026

Uh oh!

github-actions Bot commented Aug 12, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

wrn14897 commented Aug 12, 2026

Why

What

Why 150 and not infra's 30

Also

Verification

Uh oh!

changeset-bot Bot commented Aug 12, 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 Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 12, 2026

🟢 Tier 1 — Trivial

Uh oh!

greptile-apps Bot commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Uh oh!

github-actions Bot commented Aug 12, 2026

Deep Review

Deep Review

🟡 P2 -- recommended

Uh oh!

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

Uh oh!

E2E Test Results

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

changeset-bot Bot commented Aug 12, 2026 •

edited

Loading

vercel Bot commented Aug 12, 2026 •

edited

Loading

greptile-apps Bot commented Aug 12, 2026 •

edited

Loading

github-actions Bot commented Aug 12, 2026 •

edited

Loading