Skip to content

Commit fce671b

Browse files
authored
chore(ci): escalate substantial query-engine changes to review tier 4 (#2871)
## 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 #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.
1 parent b619603 commit fce671b

2 files changed

Lines changed: 167 additions & 13 deletions

File tree

‎.github/scripts/__tests__/pr-triage-classify.test.js‎

Lines changed: 109 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ const assert = require('node:assert/strict');
1010
const {
1111
isTestFile, isTrivialFile, isCriticalFile,
1212
isSecurityCriticalFile, isCoreCriticalFile, isInfraCriticalFile,
13+
isQueryEngineCriticalFile,
1314
isInternalToolingFile, isReleaseArtifactFile,
1415
computeSignals, determineTier, buildTierComment,
1516
} = require('../pr-triage-classify');
@@ -201,9 +202,34 @@ describe('critical-path bands', () => {
201202
}
202203
});
203204

204-
it('isCriticalFile still matches either list (path-level check)', () => {
205+
it('classifies the SQL rendering + execution pipeline as query-engine-critical', () => {
206+
for (const f of [
207+
'packages/common-utils/src/core/renderChartConfig.ts',
208+
'packages/common-utils/src/core/builderToRawSql.ts',
209+
'packages/common-utils/src/clickhouse/index.ts',
210+
]) {
211+
assert.ok(isQueryEngineCriticalFile(f), `expected query-engine-critical: ${f}`);
212+
assert.ok(!isSecurityCriticalFile(f), `expected not security-critical: ${f}`);
213+
assert.ok(!isCoreCriticalFile(f), `expected not core-critical: ${f}`);
214+
assert.ok(!isInfraCriticalFile(f), `expected not infra-critical: ${f}`);
215+
}
216+
});
217+
218+
it('does not sweep the rest of common-utils into the query-engine band', () => {
219+
for (const f of [
220+
'packages/common-utils/src/core/metadata.ts',
221+
'packages/common-utils/src/core/utils.ts',
222+
'packages/common-utils/src/queryParser.ts',
223+
'packages/common-utils/src/types.ts',
224+
]) {
225+
assert.ok(!isQueryEngineCriticalFile(f), `expected not query-engine-critical: ${f}`);
226+
}
227+
});
228+
229+
it('isCriticalFile still matches any band (path-level check)', () => {
205230
assert.ok(isCriticalFile('packages/api/src/middleware/auth.ts'));
206231
assert.ok(isCriticalFile('docker/hyperdx/Dockerfile'));
232+
assert.ok(isCriticalFile('packages/common-utils/src/core/renderChartConfig.ts'));
207233
assert.ok(!isCriticalFile('packages/app/src/App.tsx'));
208234
});
209235
});
@@ -533,6 +559,55 @@ describe('determineTier', () => {
533559
]), 4);
534560
});
535561

562+
it('substantial query-engine rewrite escalates (PR #2859 pattern)', () => {
563+
// Multi-series metric merge moved into ClickHouse: 664 prod lines across
564+
// the rendering + execution pipeline. Tests and snapshots don't count.
565+
assert.equal(classify('alice', 'feat/metrics-ch-side-merge', [
566+
makeFile('packages/common-utils/src/core/renderChartConfig.ts', 313, 36),
567+
makeFile('packages/common-utils/src/clickhouse/index.ts', 19, 289),
568+
makeFile('packages/common-utils/src/core/builderToRawSql.ts', 5, 2),
569+
makeFile('packages/common-utils/src/__tests__/renderChartConfig.test.ts', 174, 0),
570+
makeFile('packages/common-utils/src/__tests__/__snapshots__/renderChartConfig.test.ts.snap', 245, 0),
571+
makeFile('packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx', 6, 6),
572+
makeFile('packages/cli/src/shared/tileQuery.ts', 2, 2),
573+
makeFile('.changeset/metrics-ch-side-merge.md', 6, 0),
574+
]), 4);
575+
});
576+
577+
it('does NOT escalate a routine chart fix touching the query engine (PR #2759 pattern)', () => {
578+
// Series-limit ranking fix: 68 lines in renderChartConfig, well under the
579+
// 150-line bar. Half of all rendering-touching PRs look like this.
580+
assert.equal(classify('alice', 'fix/ratio-series-limit', [
581+
makeFile('packages/common-utils/src/core/renderChartConfig.ts', 48, 20),
582+
]), 2);
583+
});
584+
585+
it('escalates once query-engine churn reaches the bar', () => {
586+
assert.equal(classify('alice', 'refactor/render-pipeline', [
587+
makeFile('packages/common-utils/src/core/renderChartConfig.ts', 120, 30), // exactly 150
588+
]), 4);
589+
assert.equal(classify('alice', 'refactor/render-pipeline', [
590+
makeFile('packages/common-utils/src/core/renderChartConfig.ts', 120, 29), // 149 — just under
591+
]), 2);
592+
});
593+
594+
it('aggregates query-engine churn across files', () => {
595+
// No single file clears the bar, but the pipeline change as a whole does
596+
assert.equal(classify('alice', 'refactor/query-exec', [
597+
makeFile('packages/common-utils/src/core/renderChartConfig.ts', 60, 20),
598+
makeFile('packages/common-utils/src/clickhouse/index.ts', 50, 15),
599+
makeFile('packages/common-utils/src/core/builderToRawSql.ts', 5, 0),
600+
]), 4);
601+
});
602+
603+
it('query-engine test files never count toward the bar', () => {
604+
assert.equal(classify('alice', 'test/render-coverage', [
605+
makeFile('packages/common-utils/src/__tests__/renderChartConfig.test.ts', 400, 100),
606+
makeFile('packages/common-utils/src/core/__tests__/builderToRawSql.test.ts', 200, 0),
607+
makeFile('packages/common-utils/src/core/renderChartConfig.ts', 10, 5),
608+
]), 2);
609+
});
610+
536611
it('docs under a critical path never escalate', () => {
537612
assert.equal(classify('alice', 'docs/collector-readme', [
538613
makeFile('packages/otel-collector/README.md', 60, 20),
@@ -678,9 +753,11 @@ describe('determineTier', () => {
678753
});
679754

680755
it('cross-layer change below the line floor drops to Tier 2 (PR #2707 pattern)', () => {
756+
// renderChartConfig is query-engine-critical, but 8 lines is far under
757+
// the 150-line bar — a small cross-layer fix still tiers on size.
681758
assert.equal(classify('alice', 'fix/histogram-grouping', [
682759
makeFile('packages/app/src/Chart.tsx', 10, 4),
683-
makeFile('packages/common-utils/src/renderChartConfig.ts', 6, 2),
760+
makeFile('packages/common-utils/src/core/renderChartConfig.ts', 6, 2),
684761
]), 2); // 22 prod lines
685762
});
686763

@@ -739,6 +816,9 @@ describe('buildTierComment', () => {
739816
infraCriticalFiles: [],
740817
infraCriticalLines: 0,
741818
infraCriticalEscalates: false,
819+
queryEngineCriticalFiles: [],
820+
queryEngineCriticalLines: 0,
821+
queryEngineCriticalEscalates: false,
742822
internalToolingFiles: [],
743823
isBotAuthor: false,
744824
allFilesTrivial: false,
@@ -809,6 +889,33 @@ describe('buildTierComment', () => {
809889
assert.ok(!body.includes('substantially modified'));
810890
});
811891

892+
it('explains a substantial query-engine change as the Tier 4 trigger', () => {
893+
const queryEngineCriticalFiles = [
894+
makeFile('packages/common-utils/src/core/renderChartConfig.ts', 313, 36),
895+
makeFile('packages/common-utils/src/clickhouse/index.ts', 19, 289),
896+
];
897+
const body = buildTierComment(4, makeSignals({
898+
criticalFiles: queryEngineCriticalFiles,
899+
queryEngineCriticalFiles,
900+
queryEngineCriticalLines: 657,
901+
queryEngineCriticalEscalates: true,
902+
}));
903+
assert.ok(body.includes('Query rendering engine substantially modified'));
904+
assert.ok(body.includes('657 lines'));
905+
assert.ok(body.includes('renderChartConfig.ts'));
906+
assert.ok(!body.includes('Critical-path files'), 'no always-critical files were touched');
907+
});
908+
909+
it('notes a light query-engine touch as context rather than a trigger', () => {
910+
const body = buildTierComment(2, makeSignals({
911+
queryEngineCriticalFiles: [makeFile('packages/common-utils/src/core/renderChartConfig.ts', 6, 2)],
912+
queryEngineCriticalLines: 8,
913+
queryEngineCriticalEscalates: false,
914+
}));
915+
assert.ok(body.includes('query rendering engine lightly'));
916+
assert.ok(!body.includes('Query rendering engine substantially modified'));
917+
});
918+
812919
it('explains an automated release PR', () => {
813920
const body = buildTierComment(1, makeSignals({
814921
isReleaseArtifactPR: true,

‎.github/scripts/pr-triage-classify.js‎

Lines changed: 58 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,15 @@
11
'use strict';
22

33
// ── File classification patterns ─────────────────────────────────────────────
4-
// Critical paths come in three bands, ordered by how much the size of the diff
4+
// Critical paths come in four bands, ordered by how much the size of the diff
55
// should be allowed to soften them.
66
//
7-
// security — Tier 4 at any size, no escape hatch
8-
// core — Tier 4 unless the PR merely grazes it (see the graze bounds)
9-
// infra — Tier 4 only once substantially modified (INFRA_CRITICAL_MIN_LINES)
7+
// security — Tier 4 at any size, no escape hatch
8+
// core — Tier 4 unless the PR merely grazes it (see the graze bounds)
9+
// infra — Tier 4 only once substantially modified (INFRA_CRITICAL_MIN_LINES)
10+
// query-engine — Tier 4 only once substantially modified
11+
// (QUERY_ENGINE_CRITICAL_MIN_LINES, higher than infra's bar —
12+
// it is a hot path that routine chart fixes touch constantly)
1013
//
1114
// Without the bands, a three-line Dockerfile tweak carried the same "domain
1215
// expert sign-off" weight as an auth change.
@@ -43,6 +46,18 @@ const INFRA_CRITICAL_PATTERNS = [
4346
/^\.github\/workflows\/(main|release)\.yml$/,
4447
];
4548

49+
// The SQL rendering + execution pipeline in common-utils. Every chart, search,
50+
// and alert query flows through renderChartConfig, so a substantial change here
51+
// can silently alter query semantics product-wide (e.g. #2859 moving
52+
// multi-series metric merge computation into ClickHouse). It is also one of the
53+
// hottest files in the repo — routine chart fixes graze it weekly — so it gets
54+
// its own band with a bar high enough that only engine-level rewrites escalate.
55+
const QUERY_ENGINE_CRITICAL_PATTERNS = [
56+
/^packages\/common-utils\/src\/core\/renderChartConfig\./,
57+
/^packages\/common-utils\/src\/core\/builderToRawSql\./,
58+
/^packages\/common-utils\/src\/clickhouse\//,
59+
];
60+
4661
// Docs and images carry no functional risk, so they never escalate a PR — not
4762
// even under a critical path (e.g. packages/otel-collector/CHANGELOG.md).
4863
const DOC_PATTERN = /\.(md|txt|png|jpg|jpeg|gif|svg|ico)$/i;
@@ -100,6 +115,14 @@ const TIER4_ESCALATION_LINES = 1000; // Tier 3 → 4 for very large diffs
100115
// change spread thinly across several of them still escalates.
101116
const INFRA_CRITICAL_MIN_LINES = 30;
102117

118+
// Query-engine paths use the same total-churn mechanism but a higher bar.
119+
// Calibrated against 20 recent merged PRs touching these files: at infra's
120+
// 30-line bar half of them escalated, including routine Tier 2 chart fixes
121+
// (#2759, #2613, #2422); at 150 only substantial engine changes cleared it
122+
// (#2246 Increase aggFn, #2634 builder→raw SQL, #2859 metric merge in CH) —
123+
// exactly the changes that warrant a domain expert.
124+
const QUERY_ENGINE_CRITICAL_MIN_LINES = 150;
125+
103126
// A PR only "grazes" a core-critical path when the touch AND the PR as a whole
104127
// are tiny. Both bounds matter: across 600 merged PRs, a <=10-line core-critical
105128
// touch inside a larger PR still drew substantive review comments 50% of the
@@ -165,7 +188,7 @@ const TIER_INFO = {
165188
4: {
166189
emoji: '🔴',
167190
headline: 'Tier 4 — Critical',
168-
detail: 'Touches authentication, tenancy data models, the public API or shipped database config — or substantially changes background tasks, the OTel pipeline, image build, or release CI.',
191+
detail: 'Touches authentication, tenancy data models, the public API or shipped database config — or substantially changes the query rendering engine, background tasks, the OTel pipeline, image build, or release CI.',
169192
process: 'Deep review from a domain expert. Synchronous walkthrough may be required.',
170193
sla: 'Schedule synchronous review within 2 business days.',
171194
},
@@ -179,10 +202,12 @@ const isDocFile = f => DOC_PATTERN.test(f);
179202
const isSecurityCriticalFile = f => SECURITY_CRITICAL_PATTERNS.some(p => p.test(f));
180203
const isCoreCriticalFile = f => CORE_CRITICAL_PATTERNS.some(p => p.test(f));
181204
const isInfraCriticalFile = f => INFRA_CRITICAL_PATTERNS.some(p => p.test(f));
182-
// Path-level check only — whether a core- or infra-critical file actually
183-
// escalates the PR depends on diff size, resolved in computeSignals.
205+
const isQueryEngineCriticalFile = f => QUERY_ENGINE_CRITICAL_PATTERNS.some(p => p.test(f));
206+
// Path-level check only — whether a core-, infra- or query-engine-critical file
207+
// actually escalates the PR depends on diff size, resolved in computeSignals.
184208
const isCriticalFile = f =>
185-
isSecurityCriticalFile(f) || isCoreCriticalFile(f) || isInfraCriticalFile(f);
209+
isSecurityCriticalFile(f) || isCoreCriticalFile(f) || isInfraCriticalFile(f) ||
210+
isQueryEngineCriticalFile(f);
186211

187212
const isInternalToolingFile = f => INTERNAL_TOOLING_PATTERNS.some(p => p.test(f));
188213
const isReleaseArtifactFile = f => RELEASE_ARTIFACT_PATTERNS.some(p => p.test(f));
@@ -223,9 +248,16 @@ function computeSignals(pr, filesRes) {
223248
!isSecurityCriticalFile(f.filename) &&
224249
!isCoreCriticalFile(f.filename)
225250
);
251+
const queryEngineCriticalFiles = criticalCandidates.filter(f =>
252+
isQueryEngineCriticalFile(f.filename) &&
253+
!isSecurityCriticalFile(f.filename) &&
254+
!isCoreCriticalFile(f.filename) &&
255+
!isInfraCriticalFile(f.filename)
256+
);
226257

227258
const coreCriticalLines = churn(coreCriticalFiles);
228259
const infraCriticalLines = churn(infraCriticalFiles);
260+
const queryEngineCriticalLines = churn(queryEngineCriticalFiles);
229261

230262
// Everything a reviewer actually has to read. Unlike prodLines this keeps
231263
// workflows, Actions scripts and internal-tooling packages in: they are
@@ -247,11 +279,14 @@ function computeSignals(pr, filesRes) {
247279
reviewableLines <= GRAZE_MAX_PROD_LINES;
248280

249281
const infraCriticalEscalates = infraCriticalLines >= INFRA_CRITICAL_MIN_LINES;
282+
const queryEngineCriticalEscalates =
283+
queryEngineCriticalLines >= QUERY_ENGINE_CRITICAL_MIN_LINES;
250284

251285
const criticalFiles = [
252286
...securityCriticalFiles,
253287
...(grazesCoreCritical ? [] : coreCriticalFiles),
254288
...(infraCriticalEscalates ? infraCriticalFiles : []),
289+
...(queryEngineCriticalEscalates ? queryEngineCriticalFiles : []),
255290
];
256291

257292
const isBotAuthor = BOT_AUTHORS.includes(author);
@@ -287,8 +322,10 @@ function computeSignals(pr, filesRes) {
287322
author, branchName,
288323
prodFiles, prodLines, testLines,
289324
criticalFiles, securityCriticalFiles, coreCriticalFiles, infraCriticalFiles,
290-
coreCriticalLines, infraCriticalLines, reviewableLines,
291-
grazesCoreCritical, infraCriticalEscalates,
325+
queryEngineCriticalFiles,
326+
coreCriticalLines, infraCriticalLines, queryEngineCriticalLines,
327+
reviewableLines,
328+
grazesCoreCritical, infraCriticalEscalates, queryEngineCriticalEscalates,
292329
internalToolingFiles,
293330
isBotAuthor, allFilesTrivial, isReleaseArtifactPR,
294331
touchesApiModels, touchesFrontend, touchesBackend, touchesSharedUtils,
@@ -339,8 +376,10 @@ function buildTierComment(tier, signals) {
339376
isCrossLayer,
340377
// Defaulted so older callers (and hand-built fixtures) keep working.
341378
securityCriticalFiles = [], coreCriticalFiles = [], infraCriticalFiles = [],
342-
coreCriticalLines = 0, infraCriticalLines = 0,
379+
queryEngineCriticalFiles = [],
380+
coreCriticalLines = 0, infraCriticalLines = 0, queryEngineCriticalLines = 0,
343381
grazesCoreCritical = false, infraCriticalEscalates = false,
382+
queryEngineCriticalEscalates = false,
344383
internalToolingFiles = [], isReleaseArtifactPR = false, spansLayers = false,
345384
} = signals;
346385

@@ -391,6 +430,9 @@ function buildTierComment(tier, signals) {
391430
if (infraCriticalEscalates && infraCriticalFiles.length > 0) {
392431
triggers.push(`**Background tasks or delivery pipeline substantially modified** — ${infraCriticalLines} lines (bar: ${INFRA_CRITICAL_MIN_LINES}):\n${fileList(infraCriticalFiles)}`);
393432
}
433+
if (queryEngineCriticalEscalates && queryEngineCriticalFiles.length > 0) {
434+
triggers.push(`**Query rendering engine substantially modified** — ${queryEngineCriticalLines} lines (bar: ${QUERY_ENGINE_CRITICAL_MIN_LINES}). Every chart, search, and alert query flows through this code:\n${fileList(queryEngineCriticalFiles)}`);
435+
}
394436
if (tier === 4 && prodLines > TIER4_ESCALATION_LINES && criticalFiles.length === 0) {
395437
triggers.push(`**Large diff**: ${prodLines} production lines changed (threshold: ${TIER4_ESCALATION_LINES})`);
396438
}
@@ -429,6 +471,9 @@ function buildTierComment(tier, signals) {
429471
if (infraCriticalFiles.length > 0 && !infraCriticalEscalates) {
430472
contextSignals.push(`touches background tasks or the delivery pipeline lightly (${infraCriticalLines} lines, under the ${INFRA_CRITICAL_MIN_LINES}-line bar for Tier 4)`);
431473
}
474+
if (queryEngineCriticalFiles.length > 0 && !queryEngineCriticalEscalates) {
475+
contextSignals.push(`touches the query rendering engine lightly (${queryEngineCriticalLines} lines, under the ${QUERY_ENGINE_CRITICAL_MIN_LINES}-line bar for Tier 4)`);
476+
}
432477
if (spansLayers && !isCrossLayer) {
433478
contextSignals.push(`spans packages but only ${prodLines} prod lines — under the ${CROSS_LAYER_MIN_LINES}-line cross-layer bar`);
434479
}
@@ -445,9 +490,11 @@ module.exports = {
445490
// Thresholds (exported for tests and documentation)
446491
TIER2_MAX_LINES, INFRA_CRITICAL_MIN_LINES, CROSS_LAYER_MIN_LINES,
447492
TIER4_ESCALATION_LINES, CORE_CRITICAL_GRAZE_LINES, GRAZE_MAX_PROD_LINES,
493+
QUERY_ENGINE_CRITICAL_MIN_LINES,
448494
// Pure functions
449495
isTestFile, isTrivialFile, isDocFile, isCriticalFile,
450496
isSecurityCriticalFile, isCoreCriticalFile, isInfraCriticalFile,
497+
isQueryEngineCriticalFile,
451498
isInternalToolingFile, isReleaseArtifactFile,
452499
computeSignals, determineTier, buildTierComment,
453500
};

0 commit comments

Comments
 (0)