Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions packages/loopover-engine/src/github/sanitize-public-comment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
10 changes: 10 additions & 0 deletions src/github/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
21 changes: 21 additions & 0 deletions src/queue-intelligence.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export const FORBIDDEN_PUBLIC_COMMENT_WORDS = [
"wallet",
"hotkey",
"raw trust score",
"raw trust",
"trust score",
"coldkey",
"seed phrase",
Expand All @@ -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
Expand Down Expand Up @@ -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;
}

Expand Down
13 changes: 13 additions & 0 deletions test/unit/github-commands.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand Down
16 changes: 16 additions & 0 deletions test/unit/predicted-gate-engine-coverage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
43 changes: 43 additions & 0 deletions test/unit/queue-intelligence.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
);
});
});