From b2c66d7296a62b24323e80bb5ab551e1cf17f532 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 16 Jul 2026 00:50:07 -0700 Subject: [PATCH] fix(review): close cohort/score gaps in public-comment sanitizers The two production public/private boundary sanitizers gating AI-generated and echoed free text before it reaches a real GitHub comment omitted "cohort" entirely and never redacted a bare "score", diverging from the canonical PUBLIC_UNSAFE_TERMS boundary in src/signals/redaction.ts. - src/queue-intelligence.ts: add "cohort", "miner-originated", "human-originated", and "raw trust" to FORBIDDEN_PUBLIC_COMMENT_WORDS, and add a separate word-boundary check for a bare "score" (kept out of the substring wordlist so it can't false-positive on words like "underscore"). This sanitizer gates ai-review/ai-summaries/ai-chat-qa/ enrichment-wire output before it is ever posted. - src/github/commands.ts and packages/loopover-engine/src/github/ sanitize-public-comment.ts (kept byte-identical): add the same bare "cohort"/miner-originated/human-originated/raw-trust redaction. A bare "score" is intentionally left alone here since this sanitizer is also reused by score-breakdown.ts's own "explain my score" copy, which legitimately says "score" throughout by design. Adds regression tests reproducing the audited leak sentence and an over-blocking guard proving ordinary words like "underscore" survive. --- .../src/github/sanitize-public-comment.ts | 10 +++++ src/github/commands.ts | 10 +++++ src/queue-intelligence.ts | 21 +++++++++ test/unit/github-commands.test.ts | 13 ++++++ .../predicted-gate-engine-coverage.test.ts | 16 +++++++ test/unit/queue-intelligence.test.ts | 43 +++++++++++++++++++ 6 files changed, 113 insertions(+) diff --git a/packages/loopover-engine/src/github/sanitize-public-comment.ts b/packages/loopover-engine/src/github/sanitize-public-comment.ts index 513571c82e..83fbb9d038 100644 --- a/packages/loopover-engine/src/github/sanitize-public-comment.ts +++ b/packages/loopover-engine/src/github/sanitize-public-comment.ts @@ -8,6 +8,16 @@ export function sanitizePublicComment(value: string): string { .replace(/\b(?:effective|projected|estimated) score(?: changes?)?\b(?:\s+from)?\s+[-+]?\d+(?:\.\d+)?\s*(?:->|→|to)\s*[-+]?\d+(?:\.\d+)?/gi, "private context") .replace(/\b(raw trust scores?|trust scores?|wallets?|hotkeys?|coldkeys?|seed phrases?|mnemonics?)\b/gi, "private context") .replace(/\b(public score estimates?|estimated scores?|score estimates?|estimated rewards?|rewards?|reward estimates?|payouts?|farming|scoreability|score previews?|projected score changes?)\b/gi, "private context") + // "cohort" and standalone miner-/human-originated / "raw trust" (not just the "raw trust score" compound) + // leaked through this sanitizer: no entry above catches a bare mention of any of these. A bare "score" is + // deliberately NOT added here the same way: unlike queue-intelligence.ts's sanitizePublicComment (which + // gates unconstrained AI-review/chat-qa output before it is ever posted), this function is also reused by + // src/services/score-breakdown.ts to render its own contributor-facing "explain my score" copy, which + // legitimately says "score" throughout by design -- redacting it there would gut that feature's own output, + // not close a leak. See src/signals/redaction.ts's note on agent-action-explanation-card.ts / + // miner-dashboard-recommendations.ts for the established precedent of a surface intentionally not + // redacting bare score/reward. + .replace(/\b(cohorts?|miner[-_\s]?originated|human[-_\s]?originated|raw trust)\b/gi, "private context") .replace(/\b(private reviewability|reviewability internals?)\b/gi, "private context") .replace(/\b(private rankings?|rankings?)\b/gi, "private context") .replace(/\b(?:open_pr_pressure|closed_pr_credibility|low_credibility|maintainer_lane|inactive_or_unknown_lane|issue_discovery_only|merged_pr_history_floor|issue_discovery_validity_floor)\b/gi, "private context") diff --git a/src/github/commands.ts b/src/github/commands.ts index e41861b146..ba1e843736 100644 --- a/src/github/commands.ts +++ b/src/github/commands.ts @@ -1848,6 +1848,16 @@ export function sanitizePublicComment(value: string): string { .replace(/\b(?:effective|projected|estimated) score(?: changes?)?\b(?:\s+from)?\s+[-+]?\d+(?:\.\d+)?\s*(?:->|→|to)\s*[-+]?\d+(?:\.\d+)?/gi, "private context") .replace(/\b(raw trust scores?|trust scores?|wallets?|hotkeys?|coldkeys?|seed phrases?|mnemonics?)\b/gi, "private context") .replace(/\b(public score estimates?|estimated scores?|score estimates?|estimated rewards?|rewards?|reward estimates?|payouts?|farming|scoreability|score previews?|projected score changes?)\b/gi, "private context") + // "cohort" and standalone miner-/human-originated / "raw trust" (not just the "raw trust score" compound) + // leaked through this sanitizer: no entry above catches a bare mention of any of these. A bare "score" is + // deliberately NOT added here the same way: unlike queue-intelligence.ts's sanitizePublicComment (which + // gates unconstrained AI-review/chat-qa output before it is ever posted), this function is also reused by + // src/services/score-breakdown.ts to render its own contributor-facing "explain my score" copy, which + // legitimately says "score" throughout by design -- redacting it there would gut that feature's own output, + // not close a leak. See src/signals/redaction.ts's note on agent-action-explanation-card.ts / + // miner-dashboard-recommendations.ts for the established precedent of a surface intentionally not + // redacting bare score/reward. + .replace(/\b(cohorts?|miner[-_\s]?originated|human[-_\s]?originated|raw trust)\b/gi, "private context") .replace(/\b(private reviewability|reviewability internals?)\b/gi, "private context") .replace(/\b(private rankings?|rankings?)\b/gi, "private context") .replace(/\b(?:open_pr_pressure|closed_pr_credibility|low_credibility|maintainer_lane|inactive_or_unknown_lane|issue_discovery_only|merged_pr_history_floor|issue_discovery_validity_floor)\b/gi, "private context") diff --git a/src/queue-intelligence.ts b/src/queue-intelligence.ts index d61ff32a67..a5123b247f 100644 --- a/src/queue-intelligence.ts +++ b/src/queue-intelligence.ts @@ -44,6 +44,7 @@ export const FORBIDDEN_PUBLIC_COMMENT_WORDS = [ "wallet", "hotkey", "raw trust score", + "raw trust", "trust score", "coldkey", "seed phrase", @@ -68,8 +69,24 @@ export const FORBIDDEN_PUBLIC_COMMENT_WORDS = [ "private ranking", "rankings", "ranking", + "cohort", + "miner-originated", + "miner originated", + "human-originated", + "human originated", ] as const; +// A bare "score" is checked separately from the substring list above (not folded in as another entry): +// FORBIDDEN_PUBLIC_COMMENT_WORDS is matched with a plain case-insensitive `.includes()`, and an unqualified +// "score" substring also matches ordinary English words that carry no gittensor meaning at all ("underscore", +// "outscore", "overscore"), which would cause safe comments to be dropped for no reason. It still must be +// caught -- the canonical public/private boundary (PUBLIC_UNSAFE_TERMS in src/signals/redaction.ts) treats any +// bare score/cohort mention as unsafe -- so it's matched here with the same `\bscore\w*\b` word-boundary shape +// the canonical pattern uses (catches "score"/"scores"/"scored"/"scorer", though not "scoring", which drops +// the trailing "e" and so isn't a literal "score" substring -- the canonical pattern shares this same limit) +// instead of a plain substring test. +const BARE_SCORE_TERM_PATTERN = /\bscore\w*\b/i; + function computeDaysSince(isoDateString: string, now: Date): number { // A malformed/empty timestamp -> NaN, which flows into computePrivateBurdenReductionScore and then the // analyzePRQueue sort comparator (`b.score - a.score`). NaN makes Array.sort non-deterministic, and even @@ -150,6 +167,10 @@ export function sanitizePublicComment(comment: string): string { throw new Error(`Public comment contains forbidden word: "${forbiddenWord}"`); } } + const bareScoreMatch = comment.match(BARE_SCORE_TERM_PATTERN); + if (bareScoreMatch) { + throw new Error(`Public comment contains forbidden word: "${bareScoreMatch[0]}"`); + } return comment; } diff --git a/test/unit/github-commands.test.ts b/test/unit/github-commands.test.ts index 64d3671e86..46f3e3856f 100644 --- a/test/unit/github-commands.test.ts +++ b/test/unit/github-commands.test.ts @@ -417,6 +417,19 @@ describe("GitHub mention commands", () => { expect(sanitizePublicComment("open_pr_pressure closed_pr_credibility low_credibility credibility updates")).not.toMatch(/open_pr_pressure|closed_pr_credibility|low_credibility|credibility/i); expect(sanitizePublicComment("Command: @loopover reviewability")).toContain("@loopover reviewability"); expect(sanitizePublicComment("private ranking, wallet, payout")).toBe("private context"); + // Regression: bare "cohort" and standalone miner-originated/human-originated/raw-trust were not redacted + // (only compound phrases like "raw trust score" were), unlike the canonical PUBLIC_UNSAFE_TERMS boundary + // (src/signals/redaction.ts) which treats all of these as unsafe -- so unconstrained free text mentioning + // them could reach a real public GitHub comment unredacted. A bare "score" is intentionally NOT added here + // (see the comment above sanitizePublicComment's cohort/originated replace call): this function is shared + // with src/services/score-breakdown.ts's own contributor-facing "explain my score" copy, which legitimately + // says "score" throughout by design. + expect( + sanitizePublicComment("This diff looks miner-originated and the resulting cohort standing would only shift modestly."), + ).not.toMatch(/miner-originated|cohort/i); + expect(sanitizePublicComment("This PR affects the cohort.")).toContain("private context"); + expect(sanitizePublicComment("This change is human originated.")).toContain("private context"); + expect(sanitizePublicComment("Raw trust is unaffected by this PR.")).toContain("private context"); }); it("redacts private score projection deltas from public command rerun guidance", () => { diff --git a/test/unit/predicted-gate-engine-coverage.test.ts b/test/unit/predicted-gate-engine-coverage.test.ts index 1e1488eccc..7627eed840 100644 --- a/test/unit/predicted-gate-engine-coverage.test.ts +++ b/test/unit/predicted-gate-engine-coverage.test.ts @@ -167,6 +167,22 @@ describe("predicted-gate engine module coverage (#2283)", () => { expect(sanitizePublicComment("open pr count 12 exceeds threshold 10")).toContain("private 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. + // A bare "score" is intentionally NOT redacted by this shared function (see the comment above its + // cohort/originated replace call in sanitize-public-comment.ts): it is also reused by + // src/services/score-breakdown.ts's own contributor-facing "explain my score" copy, which legitimately says + // "score" throughout by design. + it("redacts bare cohort and standalone miner-originated/human-originated/raw-trust mentions", () => { + expect( + sanitizePublicComment("This diff looks miner-originated and the resulting cohort standing would only shift modestly."), + ).not.toMatch(/miner-originated|cohort/i); + expect(sanitizePublicComment("This PR affects the cohort.")).toContain("private context"); + expect(sanitizePublicComment("This change is human originated.")).toContain("private context"); + expect(sanitizePublicComment("Raw trust is unaffected by this PR.")).toContain("private context"); + }); + it("exercises focus-manifest guidance branches", () => { const manifest: FocusManifest = { present: true, diff --git a/test/unit/queue-intelligence.test.ts b/test/unit/queue-intelligence.test.ts index 5ccac95adc..709c97c589 100644 --- a/test/unit/queue-intelligence.test.ts +++ b/test/unit/queue-intelligence.test.ts @@ -344,3 +344,46 @@ describe("sanitizePublicComment — sanitizer regression", () => { } }); }); + +describe("sanitizePublicComment — cohort/score public-boundary gap regression", () => { + // Audited failure scenario: unconstrained AI-review free text echoed straight through toPublicSafe() + // (src/services/ai-review.ts) reached a real public GitHub comment because this sanitizer's wordlist had no + // entry for "cohort" and no bare "score" entry (only qualified phrases like "estimated score"), unlike the + // canonical PUBLIC_UNSAFE_TERMS boundary (src/signals/redaction.ts) which treats both as unsafe. + it("throws on the audited leak sentence (miner-originated / cohort / bare score, none of them a listed compound phrase)", () => { + expect(() => + sanitizePublicComment( + "This diff looks miner-originated and the resulting cohort standing / score would only shift modestly.", + ), + ).toThrow(); + }); + + it("throws for a bare 'cohort' mention", () => { + expect(() => sanitizePublicComment("This PR affects the cohort differently.")).toThrow(/cohort/i); + }); + + it("throws for a bare 'score' mention that isn't part of any listed compound phrase", () => { + expect(() => sanitizePublicComment("The score looks good here.")).toThrow(/score/i); + // `\bscore\w*\b` mirrors the canonical PUBLIC_UNSAFE_TERMS `\w*` suffix shape (src/signals/redaction.ts), + // which only extends a bare match, so it catches "scores"/"scored" but not "scoring" (that drops the "e"). + expect(() => sanitizePublicComment("Multiple scores were affected by this change.")).toThrow(/scores/i); + }); + + it("throws for standalone miner-originated / human-originated / raw trust (not just the 'raw trust score' compound)", () => { + expect(() => sanitizePublicComment("This change is miner-originated.")).toThrow(/miner-originated/i); + expect(() => sanitizePublicComment("This change is human originated.")).toThrow(/human originated/i); + expect(() => sanitizePublicComment("Raw trust is unaffected by this PR.")).toThrow(/raw trust/i); + }); + + it("does NOT throw on ordinary English words that merely contain 'score' as a substring (no over-blocking)", () => { + // A plain `.includes("score")` check (rather than a word-boundary match) would misfire on these -- neither + // is a gittensor score reference, and dropping the whole public comment for a false positive is a real + // regression, not a safe default, since it silently swallows a legitimate comment instead of leaking it. + expect(sanitizePublicComment("Prefer the underscore-prefixed helper here.")).toBe( + "Prefer the underscore-prefixed helper here.", + ); + expect(sanitizePublicComment("This change would outscore the previous approach on readability alone.")).toBe( + "This change would outscore the previous approach on readability alone.", + ); + }); +});