Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kodiakhq merged 2 commits into from
Aug 12, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
12c1aca
chore(ci): escalate substantial query-engine changes to review tier 4
wrn14897 Aug 12, 2026
6f9eeb1
Merge branch 'main' into warren/revisit-review-tier-ranking
kodiakhq[bot] Aug 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 109 additions & 2 deletions .github/scripts/__tests__/pr-triage-classify.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const assert = require('node:assert/strict');
const {
isTestFile, isTrivialFile, isCriticalFile,
isSecurityCriticalFile, isCoreCriticalFile, isInfraCriticalFile,
isQueryEngineCriticalFile,
isInternalToolingFile, isReleaseArtifactFile,
computeSignals, determineTier, buildTierComment,
} = require('../pr-triage-classify');
Expand Down Expand Up @@ -201,9 +202,34 @@ describe('critical-path bands', () => {
}
});

it('isCriticalFile still matches either list (path-level check)', () => {
it('classifies the SQL rendering + execution pipeline as query-engine-critical', () => {
for (const f of [
'packages/common-utils/src/core/renderChartConfig.ts',
'packages/common-utils/src/core/builderToRawSql.ts',
'packages/common-utils/src/clickhouse/index.ts',
]) {
assert.ok(isQueryEngineCriticalFile(f), `expected query-engine-critical: ${f}`);
assert.ok(!isSecurityCriticalFile(f), `expected not security-critical: ${f}`);
assert.ok(!isCoreCriticalFile(f), `expected not core-critical: ${f}`);
assert.ok(!isInfraCriticalFile(f), `expected not infra-critical: ${f}`);
}
});

it('does not sweep the rest of common-utils into the query-engine band', () => {
for (const f of [
'packages/common-utils/src/core/metadata.ts',
'packages/common-utils/src/core/utils.ts',
'packages/common-utils/src/queryParser.ts',
'packages/common-utils/src/types.ts',
]) {
assert.ok(!isQueryEngineCriticalFile(f), `expected not query-engine-critical: ${f}`);
}
});

it('isCriticalFile still matches any band (path-level check)', () => {
assert.ok(isCriticalFile('packages/api/src/middleware/auth.ts'));
assert.ok(isCriticalFile('docker/hyperdx/Dockerfile'));
assert.ok(isCriticalFile('packages/common-utils/src/core/renderChartConfig.ts'));
assert.ok(!isCriticalFile('packages/app/src/App.tsx'));
});
});
Expand Down Expand Up @@ -533,6 +559,55 @@ describe('determineTier', () => {
]), 4);
});

it('substantial query-engine rewrite escalates (PR #2859 pattern)', () => {
// Multi-series metric merge moved into ClickHouse: 664 prod lines across
// the rendering + execution pipeline. Tests and snapshots don't count.
assert.equal(classify('alice', 'feat/metrics-ch-side-merge', [
makeFile('packages/common-utils/src/core/renderChartConfig.ts', 313, 36),
makeFile('packages/common-utils/src/clickhouse/index.ts', 19, 289),
makeFile('packages/common-utils/src/core/builderToRawSql.ts', 5, 2),
makeFile('packages/common-utils/src/__tests__/renderChartConfig.test.ts', 174, 0),
makeFile('packages/common-utils/src/__tests__/__snapshots__/renderChartConfig.test.ts.snap', 245, 0),
makeFile('packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx', 6, 6),
makeFile('packages/cli/src/shared/tileQuery.ts', 2, 2),
makeFile('.changeset/metrics-ch-side-merge.md', 6, 0),
]), 4);
});

it('does NOT escalate a routine chart fix touching the query engine (PR #2759 pattern)', () => {
// Series-limit ranking fix: 68 lines in renderChartConfig, well under the
// 150-line bar. Half of all rendering-touching PRs look like this.
assert.equal(classify('alice', 'fix/ratio-series-limit', [
makeFile('packages/common-utils/src/core/renderChartConfig.ts', 48, 20),
]), 2);
});

it('escalates once query-engine churn reaches the bar', () => {
assert.equal(classify('alice', 'refactor/render-pipeline', [
makeFile('packages/common-utils/src/core/renderChartConfig.ts', 120, 30), // exactly 150
]), 4);
assert.equal(classify('alice', 'refactor/render-pipeline', [
makeFile('packages/common-utils/src/core/renderChartConfig.ts', 120, 29), // 149 — just under
]), 2);
});

it('aggregates query-engine churn across files', () => {
// No single file clears the bar, but the pipeline change as a whole does
assert.equal(classify('alice', 'refactor/query-exec', [
makeFile('packages/common-utils/src/core/renderChartConfig.ts', 60, 20),
makeFile('packages/common-utils/src/clickhouse/index.ts', 50, 15),
makeFile('packages/common-utils/src/core/builderToRawSql.ts', 5, 0),
]), 4);
});

it('query-engine test files never count toward the bar', () => {
assert.equal(classify('alice', 'test/render-coverage', [
makeFile('packages/common-utils/src/__tests__/renderChartConfig.test.ts', 400, 100),
makeFile('packages/common-utils/src/core/__tests__/builderToRawSql.test.ts', 200, 0),
makeFile('packages/common-utils/src/core/renderChartConfig.ts', 10, 5),
]), 2);
});

it('docs under a critical path never escalate', () => {
assert.equal(classify('alice', 'docs/collector-readme', [
makeFile('packages/otel-collector/README.md', 60, 20),
Expand Down Expand Up @@ -678,9 +753,11 @@ describe('determineTier', () => {
});

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

Expand Down Expand Up @@ -739,6 +816,9 @@ describe('buildTierComment', () => {
infraCriticalFiles: [],
infraCriticalLines: 0,
infraCriticalEscalates: false,
queryEngineCriticalFiles: [],
queryEngineCriticalLines: 0,
queryEngineCriticalEscalates: false,
internalToolingFiles: [],
isBotAuthor: false,
allFilesTrivial: false,
Expand Down Expand Up @@ -809,6 +889,33 @@ describe('buildTierComment', () => {
assert.ok(!body.includes('substantially modified'));
});

it('explains a substantial query-engine change as the Tier 4 trigger', () => {
const queryEngineCriticalFiles = [
makeFile('packages/common-utils/src/core/renderChartConfig.ts', 313, 36),
makeFile('packages/common-utils/src/clickhouse/index.ts', 19, 289),
];
const body = buildTierComment(4, makeSignals({
criticalFiles: queryEngineCriticalFiles,
queryEngineCriticalFiles,
queryEngineCriticalLines: 657,
queryEngineCriticalEscalates: true,
}));
assert.ok(body.includes('Query rendering engine substantially modified'));
assert.ok(body.includes('657 lines'));
assert.ok(body.includes('renderChartConfig.ts'));
assert.ok(!body.includes('Critical-path files'), 'no always-critical files were touched');
});

it('notes a light query-engine touch as context rather than a trigger', () => {
const body = buildTierComment(2, makeSignals({
queryEngineCriticalFiles: [makeFile('packages/common-utils/src/core/renderChartConfig.ts', 6, 2)],
queryEngineCriticalLines: 8,
queryEngineCriticalEscalates: false,
}));
assert.ok(body.includes('query rendering engine lightly'));
assert.ok(!body.includes('Query rendering engine substantially modified'));
});

it('explains an automated release PR', () => {
const body = buildTierComment(1, makeSignals({
isReleaseArtifactPR: true,
Expand Down
69 changes: 58 additions & 11 deletions .github/scripts/pr-triage-classify.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
'use strict';

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

// The SQL rendering + execution pipeline in common-utils. Every chart, search,
// and alert query flows through renderChartConfig, so a substantial change here
// can silently alter query semantics product-wide (e.g. #2859 moving
// multi-series metric merge computation into ClickHouse). It is also one of the
// hottest files in the repo — routine chart fixes graze it weekly — so it gets
// its own band with a bar high enough that only engine-level rewrites escalate.
const QUERY_ENGINE_CRITICAL_PATTERNS = [
/^packages\/common-utils\/src\/core\/renderChartConfig\./,
/^packages\/common-utils\/src\/core\/builderToRawSql\./,
/^packages\/common-utils\/src\/clickhouse\//,
];

// Docs and images carry no functional risk, so they never escalate a PR — not
// even under a critical path (e.g. packages/otel-collector/CHANGELOG.md).
const DOC_PATTERN = /\.(md|txt|png|jpg|jpeg|gif|svg|ico)$/i;
Expand Down Expand Up @@ -100,6 +115,14 @@ const TIER4_ESCALATION_LINES = 1000; // Tier 3 → 4 for very large diffs
// change spread thinly across several of them still escalates.
const INFRA_CRITICAL_MIN_LINES = 30;

// Query-engine paths use the same total-churn mechanism but a higher bar.
// Calibrated against 20 recent merged PRs touching these files: at infra's
// 30-line bar half of them escalated, including routine Tier 2 chart fixes
// (#2759, #2613, #2422); at 150 only substantial engine changes cleared it
// (#2246 Increase aggFn, #2634 builder→raw SQL, #2859 metric merge in CH) —
// exactly the changes that warrant a domain expert.
const QUERY_ENGINE_CRITICAL_MIN_LINES = 150;

// A PR only "grazes" a core-critical path when the touch AND the PR as a whole
// are tiny. Both bounds matter: across 600 merged PRs, a <=10-line core-critical
// touch inside a larger PR still drew substantive review comments 50% of the
Expand Down Expand Up @@ -165,7 +188,7 @@ const TIER_INFO = {
4: {
emoji: '🔴',
headline: 'Tier 4 — Critical',
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.',
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.',
process: 'Deep review from a domain expert. Synchronous walkthrough may be required.',
sla: 'Schedule synchronous review within 2 business days.',
},
Expand All @@ -179,10 +202,12 @@ const isDocFile = f => DOC_PATTERN.test(f);
const isSecurityCriticalFile = f => SECURITY_CRITICAL_PATTERNS.some(p => p.test(f));
const isCoreCriticalFile = f => CORE_CRITICAL_PATTERNS.some(p => p.test(f));
const isInfraCriticalFile = f => INFRA_CRITICAL_PATTERNS.some(p => p.test(f));
// Path-level check only — whether a core- or infra-critical file actually
// escalates the PR depends on diff size, resolved in computeSignals.
const isQueryEngineCriticalFile = f => QUERY_ENGINE_CRITICAL_PATTERNS.some(p => p.test(f));
// Path-level check only — whether a core-, infra- or query-engine-critical file
// actually escalates the PR depends on diff size, resolved in computeSignals.
const isCriticalFile = f =>
isSecurityCriticalFile(f) || isCoreCriticalFile(f) || isInfraCriticalFile(f);
isSecurityCriticalFile(f) || isCoreCriticalFile(f) || isInfraCriticalFile(f) ||
isQueryEngineCriticalFile(f);

const isInternalToolingFile = f => INTERNAL_TOOLING_PATTERNS.some(p => p.test(f));
const isReleaseArtifactFile = f => RELEASE_ARTIFACT_PATTERNS.some(p => p.test(f));
Expand Down Expand Up @@ -223,9 +248,16 @@ function computeSignals(pr, filesRes) {
!isSecurityCriticalFile(f.filename) &&
!isCoreCriticalFile(f.filename)
);
const queryEngineCriticalFiles = criticalCandidates.filter(f =>
isQueryEngineCriticalFile(f.filename) &&
!isSecurityCriticalFile(f.filename) &&
!isCoreCriticalFile(f.filename) &&
!isInfraCriticalFile(f.filename)
);

const coreCriticalLines = churn(coreCriticalFiles);
const infraCriticalLines = churn(infraCriticalFiles);
const queryEngineCriticalLines = churn(queryEngineCriticalFiles);

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

const infraCriticalEscalates = infraCriticalLines >= INFRA_CRITICAL_MIN_LINES;
const queryEngineCriticalEscalates =
queryEngineCriticalLines >= QUERY_ENGINE_CRITICAL_MIN_LINES;

const criticalFiles = [
...securityCriticalFiles,
...(grazesCoreCritical ? [] : coreCriticalFiles),
...(infraCriticalEscalates ? infraCriticalFiles : []),
...(queryEngineCriticalEscalates ? queryEngineCriticalFiles : []),
];

const isBotAuthor = BOT_AUTHORS.includes(author);
Expand Down Expand Up @@ -287,8 +322,10 @@ function computeSignals(pr, filesRes) {
author, branchName,
prodFiles, prodLines, testLines,
criticalFiles, securityCriticalFiles, coreCriticalFiles, infraCriticalFiles,
coreCriticalLines, infraCriticalLines, reviewableLines,
grazesCoreCritical, infraCriticalEscalates,
queryEngineCriticalFiles,
coreCriticalLines, infraCriticalLines, queryEngineCriticalLines,
reviewableLines,
grazesCoreCritical, infraCriticalEscalates, queryEngineCriticalEscalates,
internalToolingFiles,
isBotAuthor, allFilesTrivial, isReleaseArtifactPR,
touchesApiModels, touchesFrontend, touchesBackend, touchesSharedUtils,
Expand Down Expand Up @@ -339,8 +376,10 @@ function buildTierComment(tier, signals) {
isCrossLayer,
// Defaulted so older callers (and hand-built fixtures) keep working.
securityCriticalFiles = [], coreCriticalFiles = [], infraCriticalFiles = [],
coreCriticalLines = 0, infraCriticalLines = 0,
queryEngineCriticalFiles = [],
coreCriticalLines = 0, infraCriticalLines = 0, queryEngineCriticalLines = 0,
grazesCoreCritical = false, infraCriticalEscalates = false,
queryEngineCriticalEscalates = false,
internalToolingFiles = [], isReleaseArtifactPR = false, spansLayers = false,
} = signals;

Expand Down Expand Up @@ -391,6 +430,9 @@ function buildTierComment(tier, signals) {
if (infraCriticalEscalates && infraCriticalFiles.length > 0) {
triggers.push(`**Background tasks or delivery pipeline substantially modified** — ${infraCriticalLines} lines (bar: ${INFRA_CRITICAL_MIN_LINES}):\n${fileList(infraCriticalFiles)}`);
}
if (queryEngineCriticalEscalates && queryEngineCriticalFiles.length > 0) {
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)}`);
}
if (tier === 4 && prodLines > TIER4_ESCALATION_LINES && criticalFiles.length === 0) {
triggers.push(`**Large diff**: ${prodLines} production lines changed (threshold: ${TIER4_ESCALATION_LINES})`);
}
Expand Down Expand Up @@ -429,6 +471,9 @@ function buildTierComment(tier, signals) {
if (infraCriticalFiles.length > 0 && !infraCriticalEscalates) {
contextSignals.push(`touches background tasks or the delivery pipeline lightly (${infraCriticalLines} lines, under the ${INFRA_CRITICAL_MIN_LINES}-line bar for Tier 4)`);
}
if (queryEngineCriticalFiles.length > 0 && !queryEngineCriticalEscalates) {
contextSignals.push(`touches the query rendering engine lightly (${queryEngineCriticalLines} lines, under the ${QUERY_ENGINE_CRITICAL_MIN_LINES}-line bar for Tier 4)`);
}
if (spansLayers && !isCrossLayer) {
contextSignals.push(`spans packages but only ${prodLines} prod lines — under the ${CROSS_LAYER_MIN_LINES}-line cross-layer bar`);
}
Expand All @@ -445,9 +490,11 @@ module.exports = {
// Thresholds (exported for tests and documentation)
TIER2_MAX_LINES, INFRA_CRITICAL_MIN_LINES, CROSS_LAYER_MIN_LINES,
TIER4_ESCALATION_LINES, CORE_CRITICAL_GRAZE_LINES, GRAZE_MAX_PROD_LINES,
QUERY_ENGINE_CRITICAL_MIN_LINES,
// Pure functions
isTestFile, isTrivialFile, isDocFile, isCriticalFile,
isSecurityCriticalFile, isCoreCriticalFile, isInfraCriticalFile,
isQueryEngineCriticalFile,
isInternalToolingFile, isReleaseArtifactFile,
computeSignals, determineTier, buildTierComment,
};
Loading