From 7d1416105a6fe7143b138b362afd19232b6a49dd Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Sun, 12 Jul 2026 02:18:07 -0700 Subject: [PATCH] fix(review): keep recap cohorts private Removes includeCohorts: true from the scheduled maintainer-recap's call to loadGatePrecisionReport -- cohort splits (miner-vs-human gate false-positive rates) are maintainer-authenticated diagnostics via the gate-precision API route, not content for the unauthenticated Discord/ Slack recap webhook. Extends PUBLIC_UNSAFE_TERMS with cohort/miner- originated/human-originated as defense-in-depth. Also syncs packages/gittensory-engine/src/prompt-packet.ts's hand- duplicated mirror of PUBLIC_UNSAFE_TERMS (kept separate so the engine package stays standalone) with the three new terms, and adds the two new [-_\s]?-shaped terms to prompt-packet-redaction.test.ts's explicit per-branch sample table -- both fell through to the test's generic fallback (which uses the raw regex source itself as the sample text), so neither the mirror drift nor the missing sample surfaced until the adversarial redaction test's dynamically-generated cases failed in CI. --- .../gittensory-engine/src/prompt-packet.ts | 2 +- src/review/maintainer-recap-wire.ts | 7 +++--- src/services/maintainer-recap.ts | 25 ------------------- src/signals/redaction.ts | 4 +-- test/unit/maintainer-recap-format.test.ts | 22 ++++++---------- test/unit/maintainer-recap-wire.test.ts | 18 +++++-------- test/unit/maintainer-recap.test.ts | 5 ++-- test/unit/prompt-packet-redaction.test.ts | 10 +++++++- 8 files changed, 31 insertions(+), 62 deletions(-) diff --git a/packages/gittensory-engine/src/prompt-packet.ts b/packages/gittensory-engine/src/prompt-packet.ts index 5c5ee4bc38..152f5641c1 100644 --- a/packages/gittensory-engine/src/prompt-packet.ts +++ b/packages/gittensory-engine/src/prompt-packet.ts @@ -1,7 +1,7 @@ // Metadata-only prompt-packet builder (#2321): four analyze-phase text fields scrubbed with the same PUBLIC_UNSAFE_TERMS / PUBLIC_LOCAL_PATH_INLINE vocabulary as src/signals/redaction.ts (duplicated here so gittensory-engine stays standalone). /** Canonical economic/identity term vocabulary (alternation source only — mirrors `PUBLIC_UNSAFE_TERMS`). */ -const PUBLIC_UNSAFE_TERMS = String.raw`(?:reward|score|wallet|hotkey|coldkey|mnemonic|payout|ranking)\w*|farming|raw[-_\s]?trust|trust[-_\s]?score|private[-_\s]?reviewability|reviewability`; +const PUBLIC_UNSAFE_TERMS = String.raw`(?:reward|score|wallet|hotkey|coldkey|mnemonic|payout|ranking|cohort)\w*|miner[-_\s]?originated|human[-_\s]?originated|farming|raw[-_\s]?trust|trust[-_\s]?score|private[-_\s]?reviewability|reviewability`; /** Canonical local-filesystem-root vocabulary (alternation source only — mirrors `PUBLIC_LOCAL_PATH_INLINE`). */ const PUBLIC_LOCAL_PATH_INLINE = String.raw`/Users/|/home/|/root/|/var/|/opt/|/tmp/|/private/|[A-Za-z]:[\\/]Users[\\/]|[A-Za-z]:[\\/]Program Files[\\/]`; diff --git a/src/review/maintainer-recap-wire.ts b/src/review/maintainer-recap-wire.ts index 875a1f77c9..6ad9096b4c 100644 --- a/src/review/maintainer-recap-wire.ts +++ b/src/review/maintainer-recap-wire.ts @@ -180,10 +180,9 @@ export async function runMaintainerRecapJob( for (const repoFullName of repoNames) { try { const [gatePrecision, calibration] = await Promise.all([ - // #4521: a periodic digest is exactly the "occasional aggregate view" includeCohorts was designed - // for -- unlike a hot webhook path, one extra Gittensor API call per repo per recap run is a small, - // bounded cost, so this call site opts in by default rather than needing its own separate flag. - loadGatePrecisionReport(env, repoFullName, { windowDays: resolvedWindowDays, includeCohorts: true }), + // Keep scheduled notification digests on the public-safe aggregate path: cohort splits are + // maintainer-authenticated diagnostics, not Discord/Slack recap content. + loadGatePrecisionReport(env, repoFullName, { windowDays: resolvedWindowDays }), buildRepoOutcomeCalibration(env, repoFullName, resolvedWindowDays), ]); repos.push({ gatePrecision, calibration }); diff --git a/src/services/maintainer-recap.ts b/src/services/maintainer-recap.ts index 1dd12bcda5..bbff0cc258 100644 --- a/src/services/maintainer-recap.ts +++ b/src/services/maintainer-recap.ts @@ -133,27 +133,14 @@ export function buildMaintainerRecap(args: MaintainerRecapInputs): RecapReport { totals.gateFalsePositiveRate !== null ? `Gate false-positive rate: ${Math.round(totals.gateFalsePositiveRate * 100)}% (${totals.gateFalsePositives}/${totals.blocked} block(s) later merged).` : `Gate false-positive rate: not enough blocked PRs in the window to report.`; - // #4521: an additional summary line ONLY when the cohort split was actually requested this run — omitted - // (not "N/A") when absent, so a legacy call site's summary output is byte-identical to before this existed. - const cohortLine = cohorts ? [formatCohortSummaryLine(cohorts)] : []; const summary = [ `Maintainer recap over the last ${windowDays} day(s): ${repos.length} repo(s), ${totals.reviewed} reviewed, ${totals.merged} merged, ${totals.closed} closed.`, rateLine, `${totals.gateOverrides} maintainer override(s), ${totals.reversals} recommendation reversal(s).`, - ...cohortLine, ].map(sanitizeRecapText); return { generatedAt: args.generatedAt, windowDays, repos, totals: { ...totals, ...(cohorts ? { cohorts } : {}) }, summary }; } -/** #4521: "N of M blocked PRs were miner-originated, precision X% vs human Y%" — mirrors rateLine's own - * null-below-sample handling per cohort (a cohort's own falsePositiveRate is already null when its blocked - * count is 0, from GatePrecisionCohortReport's MIN_SAMPLE floor at the source). */ -function formatCohortSummaryLine(cohorts: { miner: MaintainerRecapCohortCounts; human: MaintainerRecapCohortCounts }): string { - const rate = (counts: MaintainerRecapCohortCounts): string => - counts.gateFalsePositiveRate !== null ? `${Math.round(counts.gateFalsePositiveRate * 100)}%` : "n/a"; - return `Miner-originated: ${cohorts.miner.blocked} blocked (${rate(cohorts.miner)} false-positive) — Human-originated: ${cohorts.human.blocked} blocked (${rate(cohorts.human)} false-positive).`; -} - /** Redact one free-text line bound for the public digest body. Two arms mirroring weekly-value-report.ts's * sanitizeReportText: scrub any absolute local path to ``, then blank the WHOLE line to * `` if any economic/identity term (reward/score/wallet/payout/…) survives. Defense in depth — the @@ -199,24 +186,12 @@ export function formatMaintainerRecap(report: RecapReport): string { `- Overrides: ${totals.gateOverrides}`, `- Reversals: ${totals.reversals}`, "", - // #4521: an entire section, only when the cohort split was requested this run -- omitted (not an empty - // header) when absent, so the digest degrades gracefully to exactly today's output. - ...(totals.cohorts ? ["## Cohorts", ...formatCohortLines(totals.cohorts), ""] : []), "## Per-repo", ...recapSectionLines(perRepoLines, "_No repositories in this window._"), ]; return `${lines.join("\n").replace(/\n{3,}/g, "\n\n").trimEnd()}\n`; } -/** #4521: render the aggregate miner-vs-human split as two bullet lines, mirroring the Totals section's own - * "gate false positives: N/M (rate)" phrasing per cohort. */ -function formatCohortLines(cohorts: { miner: MaintainerRecapCohortCounts; human: MaintainerRecapCohortCounts }): string[] { - const line = (label: string, counts: MaintainerRecapCohortCounts): string => { - const cohortRate = counts.gateFalsePositiveRate !== null ? `${Math.round(counts.gateFalsePositiveRate * 100)}%` : "n/a"; - return `- ${label}: ${counts.gateFalsePositives}/${counts.blocked} gate false positives (${cohortRate})`; - }; - return [line("Miner-originated", cohorts.miner), line("Human-originated", cohorts.human)]; -} export type RunMaintainerRecapResult = | { skipped: true; reason: "disabled" } diff --git a/src/signals/redaction.ts b/src/signals/redaction.ts index b996a83e46..db945898a2 100644 --- a/src/signals/redaction.ts +++ b/src/signals/redaction.ts @@ -3,7 +3,7 @@ // `isPublicSafeText` first, so a single regex governs redaction and new surfaces cannot drift their own copy. // // It rejects gittensor economic/identity signals (rewards, raw/trust score, wallet/hotkey/coldkey/mnemonic, -// farming, payout, ranking, (private) reviewability) and local filesystem paths. +// farming, payout, ranking, cohort diagnostics, (private) reviewability) and local filesystem paths. // // The pattern is intentionally NON-GLOBAL so `.test()` stays stateless (no `lastIndex` carry-over between // calls) and the exported constant can be reused safely across call sites and modules. @@ -20,7 +20,7 @@ // estimate" and extra terms like "seed phrase"/"private key" for cleaner output, and deliberately do not // redact a bare "score"/"reward"). Those are curated for their surface, not drift of this core, so they are // intentionally NOT collapsed onto `PUBLIC_UNSAFE_TERMS`. -export const PUBLIC_UNSAFE_TERMS = String.raw`(?:reward|score|wallet|hotkey|coldkey|mnemonic|payout|ranking)\w*|farming|raw[-_\s]?trust|trust[-_\s]?score|private[-_\s]?reviewability|reviewability`; +export const PUBLIC_UNSAFE_TERMS = String.raw`(?:reward|score|wallet|hotkey|coldkey|mnemonic|payout|ranking|cohort)\w*|miner[-_\s]?originated|human[-_\s]?originated|farming|raw[-_\s]?trust|trust[-_\s]?score|private[-_\s]?reviewability|reviewability`; // `PUBLIC_LOCAL_PATH_INLINE` is the canonical local-filesystem-root vocabulary (alternation source only — // no flags, no anchors), the path analogue of `PUBLIC_UNSAFE_TERMS`. Public surfaces that detect or scrub diff --git a/test/unit/maintainer-recap-format.test.ts b/test/unit/maintainer-recap-format.test.ts index e2c7a566ed..a798a4c513 100644 --- a/test/unit/maintainer-recap-format.test.ts +++ b/test/unit/maintainer-recap-format.test.ts @@ -91,15 +91,7 @@ describe("formatMaintainerRecap (#2240)", () => { expect(body).not.toContain("payout"); }); - // #4521: the whole "## Cohorts" section is additive -- absent when totals.cohorts is, present (with both - // cohort lines) when it's supplied. - it("omits the Cohorts section entirely when totals.cohorts is absent (byte-identical to before the split existed)", () => { - const body = formatMaintainerRecap(emptyReport()); - expect(body).not.toContain("## Cohorts"); - expect(body).not.toContain("Miner-originated"); - }); - - it("renders the Cohorts section with both lines when totals.cohorts is present", () => { + it("omits cohort diagnostics from the public recap even when totals.cohorts is present", () => { const report: RecapReport = { ...emptyReport(), totals: { @@ -109,14 +101,14 @@ describe("formatMaintainerRecap (#2240)", () => { human: { blocked: 5, gateFalsePositives: 0, gateFalsePositiveRate: 0 }, }, }, + summary: ["Miner-originated: 3 blocked", "Human-originated: 5 blocked", "Cohorts diagnostics"], }; const body = formatMaintainerRecap(report); - expect(body).toContain("## Cohorts"); - expect(body).toContain("- Miner-originated: 1/3 gate false positives (33%)"); - expect(body).toContain("- Human-originated: 0/5 gate false positives (0%)"); - // The section sits between Totals and Per-repo, and the trailing-blank-line collapse still holds. - expect(body.indexOf("## Totals")).toBeLessThan(body.indexOf("## Cohorts")); - expect(body.indexOf("## Cohorts")).toBeLessThan(body.indexOf("## Per-repo")); + expect(body).not.toContain("## Cohorts"); + expect(body).not.toContain("Miner-originated"); + expect(body).not.toContain("Human-originated"); + expect(body).not.toContain("Cohorts diagnostics"); + expect(body.match(/- /g)).toHaveLength(3); expect(body).not.toMatch(/\n{3,}/); }); }); diff --git a/test/unit/maintainer-recap-wire.test.ts b/test/unit/maintainer-recap-wire.test.ts index 897dde123a..79426f6e5b 100644 --- a/test/unit/maintainer-recap-wire.test.ts +++ b/test/unit/maintainer-recap-wire.test.ts @@ -192,11 +192,7 @@ describe("runMaintainerRecapJob — cross-repo digest (#1963, #2248)", () => { expect(posted).toHaveLength(1); }); - // #4521: runMaintainerRecapJob always opts loadGatePrecisionReport into includeCohorts -- proves the split - // actually reaches the finished report/formatted digest, not just that the wiring doesn't crash (every - // OTHER test in this file also exercises includeCohorts implicitly since it's now unconditional, but none - // of them seed a gate block or a miner author, so none would catch a real miner-vs-human misclassification). - it("populates totals.cohorts end-to-end when a blocked PR's author is a confirmed miner", async () => { + it("keeps miner cohort diagnostics out of the scheduled external recap", async () => { const env = createTestEnv({ DISCORD_WEBHOOK_URL: HOOK }); await seedRegisteredRepo(env, "owner/alpha"); await upsertPullRequestFromGitHub(env, "owner/alpha", { number: 1, title: "miner PR", state: "closed", user: { login: "miner-alice" } }); @@ -211,13 +207,11 @@ describe("runMaintainerRecapJob — cross-repo digest (#1963, #2248)", () => { const { report, formatted } = ranRecap(await runMaintainerRecapJob(env)); - expect(report.totals.cohorts).toMatchObject({ miner: { blocked: 1 }, human: { blocked: 1 } }); - expect(report.repos[0]?.cohorts).toMatchObject({ miner: { blocked: 1 }, human: { blocked: 1 } }); - expect(formatted).toContain("## Cohorts"); - // Neither PR merged (both stay "closed"), so blockedThenMerged is 0 for both cohorts -- only `blocked` - // differs from zero here. - expect(formatted).toContain("Miner-originated: 0/1 gate false positives"); - expect(formatted).toContain("Human-originated: 0/1 gate false positives"); + expect(report.totals.cohorts).toBeUndefined(); + expect(report.repos[0]?.cohorts).toBeUndefined(); + expect(formatted).not.toContain("## Cohorts"); + expect(formatted).not.toContain("Miner-originated"); + expect(formatted).not.toContain("Human-originated"); }); it("threads a custom windowDays through to the report and the per-repo aggregators", async () => { diff --git a/test/unit/maintainer-recap.test.ts b/test/unit/maintainer-recap.test.ts index 92d3878704..d4dbd79fc7 100644 --- a/test/unit/maintainer-recap.test.ts +++ b/test/unit/maintainer-recap.test.ts @@ -136,8 +136,9 @@ describe("buildMaintainerRecap cohort split (#4521)", () => { miner: { blocked: 2, gateFalsePositives: 1, gateFalsePositiveRate: 0.5 }, human: { blocked: 4, gateFalsePositives: 1, gateFalsePositiveRate: 0.25 }, }); - expect(report.summary[3]).toContain("Miner-originated: 2 blocked (50% false-positive)"); - expect(report.summary[3]).toContain("Human-originated: 4 blocked (25% false-positive)"); + expect(report.summary).toHaveLength(3); + expect(report.summary.join("\n")).not.toContain("Miner-originated"); + expect(report.summary.join("\n")).not.toContain("Human-originated"); }); it("sums cohorts ACROSS repos, correctly reporting n/a for a zero-blocked cohort", () => { diff --git a/test/unit/prompt-packet-redaction.test.ts b/test/unit/prompt-packet-redaction.test.ts index 36f9961aee..387b55c826 100644 --- a/test/unit/prompt-packet-redaction.test.ts +++ b/test/unit/prompt-packet-redaction.test.ts @@ -50,6 +50,14 @@ function enumerateUnsafeTermFamilies(source: string): Array<{ id: string; sample } continue; } + if (branch === "miner[-_\\s]?originated") { + families.push({ id: "miner-originated", sample: "miner-originated" }); + continue; + } + if (branch === "human[-_\\s]?originated") { + families.push({ id: "human-originated", sample: "human_originated" }); + continue; + } if (branch === "raw[-_\\s]?trust") { families.push({ id: "raw-trust", sample: "raw-trust" }); continue; @@ -88,7 +96,7 @@ const LOCAL_PATH_SAMPLES = enumerateLocalPathSamples(PUBLIC_LOCAL_PATH_INLINE); describe("buildPromptPacket redaction (#2321 adversarial allowlist)", () => { it("enumerates every unsafe-term family from PUBLIC_UNSAFE_TERMS", () => { expect(UNSAFE_TERM_FAMILIES.map((entry) => entry.id).sort()).toEqual( - ["coldkey", "farming", "hotkey", "mnemonic", "payout", "private-reviewability", "ranking", "raw-trust", "reviewability", "reward", "score", "trust-score", "wallet"].sort(), + ["cohort", "coldkey", "farming", "hotkey", "human-originated", "miner-originated", "mnemonic", "payout", "private-reviewability", "ranking", "raw-trust", "reviewability", "reward", "score", "trust-score", "wallet"].sort(), ); });