From 6df733dd3936d57394425cf0a7574bcb967681be Mon Sep 17 00:00:00 2001 From: bitfathers94 <237535319+bitfathers94@users.noreply.github.com> Date: Sun, 26 Jul 2026 00:40:06 +0000 Subject: [PATCH] fix(engine): sync gate-advisory CHECK_RUN_FORBIDDEN_TERMS with its host twin and diff regex bodies in CI The engine copy's CHECK_RUN_FORBIDDEN_TERMS regex had silently dropped the likely_duplicate and reviewability\s*\d alternatives its host twin in src/rules/advisory.ts carries, so sanitizeForCheckRun would leak those terms into a public check-run for any direct consumer of evaluateGateCheck. The existing guard (checkGateDecisionVersionBump / GATE_DECISION_CORE_MARKERS) only enforced function-name presence, never the regex body itself, so the drift went undetected despite both files' comments claiming byte-identical enforcement. - Restore the two missing alternatives so the two copies are byte-identical. - Add checkGateDecisionForbiddenTermsParity to scripts/check-engine-parity.ts, which extracts and diffs the actual regex literal of both twins and wires into runEngineParityChecks so a future body divergence fails CI immediately. - Cover the newly-redacted terms through sanitizeForCheckRun/evaluateGateCheck and the strengthened drift check's failure path (including a synthetic divergence fixture mirroring the marker-presence test pattern). Closes #8697 --- .../src/advisory/gate-advisory.ts | 10 +- scripts/check-engine-parity.ts | 64 +++++++++++ src/rules/advisory.ts | 8 +- test/unit/check-engine-parity-script.test.ts | 107 ++++++++++++++++++ test/unit/predicted-gate-engine.test.ts | 39 +++++++ 5 files changed, 220 insertions(+), 8 deletions(-) diff --git a/packages/loopover-engine/src/advisory/gate-advisory.ts b/packages/loopover-engine/src/advisory/gate-advisory.ts index aa54b56f07..3e5fe8a52b 100644 --- a/packages/loopover-engine/src/advisory/gate-advisory.ts +++ b/packages/loopover-engine/src/advisory/gate-advisory.ts @@ -27,11 +27,12 @@ import { REVIEW_THREAD_BLOCKER_CODE } from "../review/review-thread-findings.js" import { labelMatchesPattern } from "../scoring/label-match.js"; // Kept byte-identical with the GATE_DECISION_TWIN_PAIR copy in src/rules/advisory.ts -// (checkGateDecisionVersionBump enforces this). The mnemonics/seed-phrases/cohort/miner-|human-originated/ -// bare-raw-trust/bare-rankings terms were ported from sanitizePublicComment's own fix for the same leak class -// (#7074) -- `raw\s+trust\s+scores?` stays ahead of bare `raw\s+trust` so the compound still matches first. +// (checkGateDecisionForbiddenTermsParity now diffs the two regex bodies byte-for-byte, #8697). The +// mnemonics/seed-phrases/cohort/miner-|human-originated/bare-raw-trust/bare-rankings terms were ported from +// sanitizePublicComment's own fix for the same leak class (#7074) -- `raw\s+trust\s+scores?` stays ahead of +// bare `raw\s+trust` so the compound still matches first. const CHECK_RUN_FORBIDDEN_TERMS = - /\b(?:rewards?|payouts?|farming|estimated\s+scores?|raw\s+trust\s+scores?|raw\s+trust|trust\s+scores?|score\s+estimates?|reward\s+estimates?|wallets?|hotkeys?|coldkeys?|mnemonics?|seed\s?phrases?|cohorts?|miner[-_\s]?originated|human[-_\s]?originated|rankings?|reviewability|scoreability|private\s+signals?)\b/gi; + /\b(?:rewards?|payouts?|farming|estimated\s+scores?|raw\s+trust\s+scores?|raw\s+trust|trust\s+scores?|score\s+estimates?|reward\s+estimates?|wallets?|hotkeys?|coldkeys?|mnemonics?|seed\s?phrases?|cohorts?|miner[-_\s]?originated|human[-_\s]?originated|rankings?|reviewability|scoreability|private\s+signals?|likely_duplicate|reviewability\s*\d)\b/gi; function sanitizeForCheckRun(text: string): string { return text.replace(CHECK_RUN_FORBIDDEN_TERMS, "[context]").replace(/\s+/g, " ").trim(); @@ -707,6 +708,7 @@ function normalizeScore(value: number | null | undefined): number | null { /** @internal Exported for unit tests of advisory severity wiring. */ export const gateAdvisoryInternals = { advisory, + sanitizeForCheckRun, highestSeverity, conclusionForSeverity, buildSizeHoldFinding, diff --git a/scripts/check-engine-parity.ts b/scripts/check-engine-parity.ts index b5fcaf95d5..09a16f8402 100644 --- a/scripts/check-engine-parity.ts +++ b/scripts/check-engine-parity.ts @@ -352,6 +352,67 @@ export function checkGateDecisionTwinPresence({ return { failures, pairChecked: twin }; } +/** The `const` both gate-decision twins declare their shared check-run redaction regex on. */ +const FORBIDDEN_TERMS_CONST = "const CHECK_RUN_FORBIDDEN_TERMS ="; + +/** Pull the `CHECK_RUN_FORBIDDEN_TERMS` regex literal (source + flags) out of a twin file's raw text so the + * two copies can be compared by CONTENT, not just by the const name's presence (#8697). The regex body + * carries no forward slash, so the literal spans from the first `/` after the const to the next unescaped + * `/` and its trailing flags. Returns null when the const declaration or its regex literal isn't found. */ +export function extractForbiddenTermsRegex(text: string): string | null { + const constIndex = text.indexOf(FORBIDDEN_TERMS_CONST); + if (constIndex === -1) return null; + const afterConst = text.slice(constIndex + FORBIDDEN_TERMS_CONST.length); + const literal = afterConst.match(/\/((?:\\.|[^/\\])+)\/([a-z]*)/); + if (!literal) return null; + return `/${literal[1]}/${literal[2]}`; +} + +/** Content-level drift guard (#8697): both gate-decision twins hand-maintain a `CHECK_RUN_FORBIDDEN_TERMS` + * regex their own comments claim is "byte-identical", but no check ever diffed the bodies -- the engine copy + * had silently dropped `likely_duplicate|reviewability\s*\d`. `checkGateDecisionTwinPresence` only asserts + * the four function-name markers exist, so it never saw this. This diffs the two regex literals directly, so + * a future divergence in the body itself (not just a missing entrypoint) fails CI immediately. */ +export function checkGateDecisionForbiddenTermsParity({ + root, + readFile = defaultReadFile, + pair = GATE_DECISION_TWIN_PAIR, +}: { + root: string; + readFile?: EngineParityReadFile; + pair?: NamedTwinPair; +}): { failures: string[] } { + let hostText: string; + let engineText: string; + try { + hostText = readFile(root, pair.hostRelative); + engineText = readFile(root, pair.engineRelative); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + return { failures: [`Could not load ${pair.area} twin pair files for regex-body parity: ${message}`] }; + } + const failures: string[] = []; + const hostRegex = extractForbiddenTermsRegex(hostText); + const engineRegex = extractForbiddenTermsRegex(engineText); + if (hostRegex === null) { + failures.push(`${pair.hostRelative} is missing a CHECK_RUN_FORBIDDEN_TERMS regex literal to diff.`); + } + if (engineRegex === null) { + failures.push(`${pair.engineRelative} is missing a CHECK_RUN_FORBIDDEN_TERMS regex literal to diff.`); + } + if (hostRegex !== null && engineRegex !== null && hostRegex !== engineRegex) { + failures.push( + [ + "CHECK_RUN_FORBIDDEN_TERMS regex body has drifted between the gate-decision twins:", + ` • ${pair.hostRelative}: ${hostRegex}`, + ` • ${pair.engineRelative}: ${engineRegex}`, + ` Make ${pair.engineRelative}'s regex byte-identical to the host copy.`, + ].join("\n"), + ); + } + return { failures }; +} + export function parseEnginePackageVersion(text: string): string | null { try { const version = JSON.parse(text).version; @@ -635,6 +696,8 @@ export function runEngineParityChecks(options: { const namedTwinPresence = NAMED_TWIN_PAIRS.map(({ pair, markers }) => checkGateDecisionTwinPresence({ root: options.root, readFile, pair, markers }), ); + // Content-level guard on the gate-decision twins' shared redaction regex, beyond mere marker presence (#8697). + const forbiddenTermsParity = checkGateDecisionForbiddenTermsParity({ root: options.root, readFile }); const skew = checkEngineVersionSkew(options); const pinSync = checkMinerEngineVersionPinSync(options); let headEngineVersion = options.headEngineVersion; @@ -675,6 +738,7 @@ export function runEngineParityChecks(options: { failures: [ ...drift.failures, ...namedTwinPresence.flatMap((result) => result.failures), + ...forbiddenTermsParity.failures, ...versionBump.failures, ...skew.failures, ...pinSync.failures, diff --git a/src/rules/advisory.ts b/src/rules/advisory.ts index 7189af1d9d..cce3eaeffe 100644 --- a/src/rules/advisory.ts +++ b/src/rules/advisory.ts @@ -374,10 +374,10 @@ export function buildIssueAdvisory(repo: RepositoryRecord | null, issue: IssueRe } // Kept byte-identical with the GATE_DECISION_TWIN_PAIR copy in packages/loopover-engine/src/advisory/ -// gate-advisory.ts (checkGateDecisionVersionBump enforces this). The mnemonics/seed-phrases/cohort/ -// miner-|human-originated/bare-raw-trust/bare-rankings terms were ported from sanitizePublicComment's own fix -// for the same leak class (#7074) -- `raw\s+trust\s+scores?` stays ahead of bare `raw\s+trust` so the compound -// still matches first. +// gate-advisory.ts (checkGateDecisionForbiddenTermsParity now diffs the two regex bodies byte-for-byte, #8697). +// The mnemonics/seed-phrases/cohort/miner-|human-originated/bare-raw-trust/bare-rankings terms were ported from +// sanitizePublicComment's own fix for the same leak class (#7074) -- `raw\s+trust\s+scores?` stays ahead of +// bare `raw\s+trust` so the compound still matches first. const CHECK_RUN_FORBIDDEN_TERMS = /\b(?:rewards?|payouts?|farming|estimated\s+scores?|raw\s+trust\s+scores?|raw\s+trust|trust\s+scores?|score\s+estimates?|reward\s+estimates?|wallets?|hotkeys?|coldkeys?|mnemonics?|seed\s?phrases?|cohorts?|miner[-_\s]?originated|human[-_\s]?originated|rankings?|reviewability|scoreability|private\s+signals?|likely_duplicate|reviewability\s*\d)\b/gi; diff --git a/test/unit/check-engine-parity-script.test.ts b/test/unit/check-engine-parity-script.test.ts index 6831e4951a..e2c17c5f4a 100644 --- a/test/unit/check-engine-parity-script.test.ts +++ b/test/unit/check-engine-parity-script.test.ts @@ -5,6 +5,7 @@ import { join } from "node:path"; import { describe, expect, it, vi } from "vitest"; import { checkEngineParityDrift, + checkGateDecisionForbiddenTermsParity, checkGateDecisionTwinPresence, checkGateDecisionVersionBump, checkEngineVersionSkew, @@ -18,6 +19,7 @@ import { discoverEngineParityPairs, discoverGateDecisionTwinPair, enginePackageVersionIncreased, + extractForbiddenTermsRegex, GATE_DECISION_TWIN_PAIR, type EngineParityPair, isEngineStubPair, @@ -347,6 +349,111 @@ describe("check-engine-parity script", () => { }); }); + // #8697: strengthen the gate-decision twin guard to diff the CHECK_RUN_FORBIDDEN_TERMS regex BODY, not just + // the four function-name markers -- the engine copy had silently dropped `likely_duplicate|reviewability\s*\d` + // and nothing caught it. These mirror the marker-presence test pattern above with a synthetic divergence. + describe("CHECK_RUN_FORBIDDEN_TERMS regex-body parity (#8697)", () => { + const gateDecisionReadFile = (host: string, engine: string) => (_root: string, relativePath: string) => { + if (relativePath === GATE_DECISION_TWIN_PAIR.hostRelative) return host; + if (relativePath === GATE_DECISION_TWIN_PAIR.engineRelative) return engine; + throw new Error(`unexpected read: ${relativePath}`); + }; + + it("extracts the regex literal (source + flags) and returns null when it is absent", () => { + const withRegex = String.raw` +const CHECK_RUN_FORBIDDEN_TERMS = + /\b(?:rewards?|likely_duplicate|reviewability\s*\d)\b/gi; +`; + expect(extractForbiddenTermsRegex(withRegex)).toBe("/\\b(?:rewards?|likely_duplicate|reviewability\\s*\\d)\\b/gi"); + expect(extractForbiddenTermsRegex("export const OTHER = 1;\n")).toBeNull(); + expect(extractForbiddenTermsRegex("const CHECK_RUN_FORBIDDEN_TERMS = buildTerms(list);\n")).toBeNull(); + }); + + it("passes against the real repo now that both twins carry the same regex body", () => { + expect(checkGateDecisionForbiddenTermsParity({ root: process.cwd() }).failures).toEqual([]); + }); + + it("passes when the two synthetic regex bodies are byte-identical", () => { + const body = String.raw` +const CHECK_RUN_FORBIDDEN_TERMS = + /\b(?:rewards?|likely_duplicate|reviewability\s*\d)\b/gi; +`; + const result = checkGateDecisionForbiddenTermsParity({ root: "/fake", readFile: gateDecisionReadFile(body, body) }); + expect(result.failures).toEqual([]); + }); + + it("fails when the two regex bodies are made to diverge again (synthetic fixture)", () => { + const host = String.raw` +const CHECK_RUN_FORBIDDEN_TERMS = + /\b(?:rewards?|likely_duplicate|reviewability\s*\d)\b/gi; +`; + const engine = String.raw` +const CHECK_RUN_FORBIDDEN_TERMS = + /\b(?:rewards?)\b/gi; +`; + const result = checkGateDecisionForbiddenTermsParity({ root: "/fake", readFile: gateDecisionReadFile(host, engine) }); + expect(result.failures.some((failure) => failure.includes("regex body has drifted"))).toBe(true); + // runEngineParityChecks surfaces the same drift end-to-end. + const combined = runEngineParityChecks({ + root: "/fake", + readFile: (_root, relativePath) => { + if (relativePath === "packages/loopover-engine/package.json") return JSON.stringify({ version: "0.2.0" }); + if (relativePath === GATE_DECISION_TWIN_PAIR.hostRelative) return host; + if (relativePath === GATE_DECISION_TWIN_PAIR.engineRelative) return engine; + throw new Error(`unexpected read: ${relativePath}`); + }, + listDir: () => [], + resolveInstalled: () => "0.2.0", + readExpected: () => "0.2.0", + changedFiles: [], + headEngineVersion: "0.2.0", + }); + expect(combined.failures.some((failure) => failure.includes("regex body has drifted"))).toBe(true); + }); + + it("fails when either twin copy is missing its regex literal entirely", () => { + const withRegex = String.raw` +const CHECK_RUN_FORBIDDEN_TERMS = + /\b(?:rewards?)\b/gi; +`; + const withoutRegex = "const CHECK_RUN_FORBIDDEN_TERMS = buildForbiddenTerms();\n"; + + const engineMissing = checkGateDecisionForbiddenTermsParity({ + root: "/fake", + readFile: gateDecisionReadFile(withRegex, withoutRegex), + }); + expect( + engineMissing.failures.some((failure) => failure.includes(`${GATE_DECISION_TWIN_PAIR.engineRelative} is missing`)), + ).toBe(true); + + const hostMissing = checkGateDecisionForbiddenTermsParity({ + root: "/fake", + readFile: gateDecisionReadFile(withoutRegex, withRegex), + }); + expect( + hostMissing.failures.some((failure) => failure.includes(`${GATE_DECISION_TWIN_PAIR.hostRelative} is missing`)), + ).toBe(true); + }); + + it("reports a load failure when a twin file cannot be read (both Error and non-Error throws)", () => { + const errorThrow = checkGateDecisionForbiddenTermsParity({ + root: "/fake", + readFile: () => { + throw new Error("boom"); + }, + }); + expect(errorThrow.failures).toEqual(["Could not load gate-decision twin pair files for regex-body parity: boom"]); + + const stringThrow = checkGateDecisionForbiddenTermsParity({ + root: "/fake", + readFile: () => { + throw "kaboom"; + }, + }); + expect(stringThrow.failures).toEqual(["Could not load gate-decision twin pair files for regex-body parity: kaboom"]); + }); + }); + describe("named twin-pair coverage (#4605)", () => { it("registers the gate-decision, safe-url, diff-file-priority, shares-meaningful-file, and secret-detection pairs", () => { const areas = NAMED_TWIN_PAIRS.map(({ pair }) => pair.area); diff --git a/test/unit/predicted-gate-engine.test.ts b/test/unit/predicted-gate-engine.test.ts index d6a33fb165..3698e42710 100644 --- a/test/unit/predicted-gate-engine.test.ts +++ b/test/unit/predicted-gate-engine.test.ts @@ -187,6 +187,45 @@ describe("predicted-gate engine module coverage (#2283)", () => { expect(sanitizePublicComment("open pr count 12 exceeds threshold 10")).toContain("private context"); }); + // #8697: CHECK_RUN_FORBIDDEN_TERMS in gate-advisory.ts had silently drifted from its host twin, missing the + // `likely_duplicate` and `reviewability\s*\d` alternatives, so sanitizeForCheckRun leaked those terms. Prove + // the engine copy now redacts them, both directly and through evaluateGateCheck's rendered check-run text. + it("sanitizeForCheckRun redacts likely_duplicate and reviewability (#8697 twin drift)", () => { + expect(gateAdvisoryInternals.sanitizeForCheckRun("this is likely_duplicate of another PR")).toBe( + "this is [context] of another PR", + ); + const scrubbed = gateAdvisoryInternals.sanitizeForCheckRun("reviewability 87 too low"); + expect(scrubbed).not.toContain("reviewability"); + expect(scrubbed).toContain("[context]"); + + const evaluation = evaluateGateCheck( + { + id: "a", + targetType: "pull_request", + targetKey: "k", + repoFullName: REPO.fullName, + conclusion: "action_required", + severity: "critical", + title: "t", + summary: "s", + generatedAt: "2026-01-01T00:00:00.000Z", + findings: [ + { + code: "duplicate_pr_risk", + severity: "critical", + title: "This PR is likely_duplicate of #123", + detail: "overlaps an existing PR", + action: "reviewability 87 is below the floor", + }, + ], + }, + { duplicatePrGateMode: "block" }, + ); + expect(evaluation.conclusion).toBe("failure"); + expect(`${evaluation.title} ${evaluation.summary}`).not.toMatch(/likely_duplicate|reviewability/); + expect(evaluation.summary).toContain("[context]"); + }); + // Regression: this sanitizer's phrase list had no entry for bare "cohort" or standalone // miner-originated/human-originated/raw-trust (only compound phrases like "raw trust score"), unlike the // canonical PUBLIC_UNSAFE_TERMS boundary (src/signals/redaction.ts) which treats all of these as unsafe.