Skip to content

Commit ac97dee

Browse files
RealDiligentRealDiligent
andauthored
fix(review): re-sync the two public-surface redaction term lists (#8445)
unified-comment-bridge.ts documents PRIVATE_FORBIDDEN_TERMS as mirroring advisory.ts's CHECK_RUN_FORBIDDEN_TERMS, but the two had drifted in both directions: the Nit path was missing bare 'raw trust', mnemonic, seed phrase, cohort, miner-originated, human-originated and rankings, so those reached public PR comments unredacted even though the check-run path scrubs them; the check-run path was missing likely_duplicate and reviewability<digit>. Both regexes are now the identical union, and the Nit drop-guard gains cohort and mnemonic. Adds a behavioral parity test that drives one string per term through BOTH sanitizers via their real public entry points, so a future one-sided edit fails -- verified it fails when either list is reverted. Closes #8322 Co-authored-by: RealDiligent <nft.gold.eth@gmail.com>
1 parent 5a5a4f1 commit ac97dee

3 files changed

Lines changed: 69 additions & 3 deletions

File tree

src/review/unified-comment-bridge.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,8 +71,8 @@ export { splitAiReviewNits } from "./ai-notes";
7171
// src/signals/engine.ts containsPrivatePublicTerm (drop if still present). Kept inline so this module
7272
// stays a pure, dependency-light renderer-mapping seam.
7373
const PRIVATE_FORBIDDEN_TERMS =
74-
/\b(?:rewards?|payouts?|farming|estimated\s+scores?|raw\s+trust\s+scores?|trust\s+scores?|score\s+estimates?|reward\s+estimates?|wallets?|hotkeys?|coldkeys?|reviewability|scoreability|private\s+signals?|likely_duplicate|reviewability\s*\d)\b/gi;
75-
const PRIVATE_DROP_TERMS = /\b(?:reward|payout|farming|wallet|hotkey|trust score|raw trust|estimated score|scoreability|likely_duplicate|reviewability\s*\d)\b/i;
74+
/\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;
75+
const PRIVATE_DROP_TERMS = /\b(?:reward|payout|farming|wallet|hotkey|trust score|raw trust|estimated score|scoreability|cohort|mnemonic|likely_duplicate|reviewability\s*\d)\b/i;
7676

7777
/** Scrub forbidden terms from a contributor-facing Nit; return null to DROP it if it still leaks after
7878
* scrubbing (fail-safe: never publish a line that names private rubric/scoring/reward internals). */

src/rules/advisory.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -379,7 +379,7 @@ export function buildIssueAdvisory(repo: RepositoryRecord | null, issue: IssueRe
379379
// for the same leak class (#7074) -- `raw\s+trust\s+scores?` stays ahead of bare `raw\s+trust` so the compound
380380
// still matches first.
381381
const CHECK_RUN_FORBIDDEN_TERMS =
382-
/\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;
382+
/\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;
383383

384384
function sanitizeForCheckRun(text: string): string {
385385
return text.replace(CHECK_RUN_FORBIDDEN_TERMS, "[context]").replace(/\s+/g, " ").trim();

test/unit/unified-comment-bridge.test.ts

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
verdictToRecommendation,
1414
visualFindingsFromFindings,
1515
} from "../../src/review/unified-comment-bridge";
16+
import { buildCheckRunAnnotations } from "../../src/rules/advisory";
1617
import { VISUAL_REGRESSION_FINDING_CODE, VISUAL_UNRELATED_ISSUE_FINDING_CODE } from "../../src/review/visual/visual-findings";
1718
import { PR_PANEL_COMMENT_MARKER as MARKER_FROM_COMMENTS } from "../../src/github/comments";
1819
import { deriveUnifiedStatus, type MergeReadiness, type UnifiedCollapsible, type UnifiedCommentStatus } from "../../src/review/unified-comment";
@@ -1421,6 +1422,71 @@ describe("buildDualReviewNotes — public-safe Nit scrub (privacy-critical, gate
14211422
}
14221423
});
14231424

1425+
it("REGRESSION (#8322): every forbidden term is redacted by BOTH public-surface sanitizers (check-run text and PR-comment Nits)", () => {
1426+
// The two lists had drifted: advisory.ts scrubbed mnemonic/seed phrase/cohort/miner-originated/
1427+
// human-originated/rankings/bare "raw trust" while the Nit path did not (so those leaked into public
1428+
// PR comments), and the Nit path scrubbed likely_duplicate/reviewability<digit> while the check-run
1429+
// path did not. This drives one representative string per term through BOTH sanitizers via their real
1430+
// public entry points, so a future one-sided edit to either regex fails here. It is deliberately a
1431+
// behavioral test, not a string comparison of the two regex sources.
1432+
const forbiddenTerms = [
1433+
"reward", "rewards", "payout", "payouts", "farming", "estimated score", "raw trust scores",
1434+
"raw trust", "trust score", "score estimates", "reward estimates", "wallet", "hotkey", "coldkey",
1435+
"mnemonic", "seed phrase", "cohort", "miner-originated", "human_originated", "rankings",
1436+
"reviewability", "scoreability", "private signals", "likely_duplicate", "reviewability3",
1437+
];
1438+
1439+
for (const term of forbiddenTerms) {
1440+
// (1) PR-comment Nit path — publicSafeNit scrubs to "[context]" or drops the Nit entirely.
1441+
const reviews = buildDualReviewNotes({
1442+
aiReview: { notes: "Reviewer assessment." },
1443+
warnings: [{ code: "w", severity: "warning", title: `Concern about ${term} here`, detail: "...", action: "n/a" }],
1444+
recommendation: "manual_review",
1445+
verdict: "manual",
1446+
});
1447+
const nit = reviews[0]?.notes?.nits?.[0] ?? "";
1448+
expect(nit.toLowerCase(), `Nit path must not leak "${term}"`).not.toContain(term.toLowerCase());
1449+
1450+
// (2) Check-run annotation path — sanitizeForCheckRun scrubs title and message.
1451+
const advisory = {
1452+
id: "a1",
1453+
targetType: "pull_request" as const,
1454+
targetKey: "o/r#1",
1455+
repoFullName: "o/r",
1456+
pullNumber: 1,
1457+
headSha: "sha",
1458+
conclusion: "neutral" as const,
1459+
severity: "warning" as const,
1460+
title: "LoopOver advisory",
1461+
summary: "s",
1462+
findings: [
1463+
{
1464+
code: "missing_test_evidence",
1465+
title: `Concern about ${term} here`,
1466+
severity: "warning" as const,
1467+
detail: `Detail naming ${term} explicitly.`,
1468+
// publicText is what actually reaches an annotation (advisory.ts skips findings without it),
1469+
// so the term must live here or the check-run leg would assert against an EMPTY list.
1470+
publicText: `Detail naming ${term} explicitly.`,
1471+
},
1472+
],
1473+
generatedAt: "2026-07-24T00:00:00.000Z",
1474+
};
1475+
const { annotations } = buildCheckRunAnnotations(
1476+
advisory,
1477+
{
1478+
files: [{ repoFullName: "o/r", pullNumber: 1, path: "src/a.ts", additions: 1, deletions: 0, changes: 1, payload: {} }],
1479+
collisions: { repoFullName: "o/r", generatedAt: "2026-07-24T00:00:00.000Z", summary: { clusterCount: 0, highRiskCount: 0, itemsReviewed: 0 }, clusters: [] },
1480+
pullNumber: 1,
1481+
},
1482+
"standard",
1483+
);
1484+
// Non-vacuity guard: an empty annotation list would make the assertion below trivially true.
1485+
expect(annotations.length, `check-run leg must actually produce an annotation for "${term}"`).toBeGreaterThan(0);
1486+
expect(JSON.stringify(annotations).toLowerCase(), `check-run path must not leak "${term}"`).not.toContain(term.toLowerCase());
1487+
}
1488+
});
1489+
14241490
it("excludes a visual_regression_finding warning from Nits (#4111 — it renders in its own 'Visual findings' collapsible instead)", () => {
14251491
const reviews = buildDualReviewNotes({
14261492
aiReview: { notes: "Reviewer assessment." },

0 commit comments

Comments
 (0)