From d93ea15e3735cf8227b997b8571b48ef5b370bfc Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 22:30:43 -0700 Subject: [PATCH] fix(mcp,selfhost): audit mutating MCP tools, dedupe predict_gate agreement, guard repo privacy Three related ORB gaps in one pass: - Every mutating MCP tool (agent pause, autonomy dial, private-config write, redeploy trigger) now records an audit_events row, mirroring the HTTP settings route's own audit shape. Previously none of them left any forensic trace of who flipped the autonomy dial or paused the agent. - computePredictedGateVerdict (backing predict_gate/explain_gate_disposition) now shares the sibling tools' per-actor rate limit, and the predicted-vs- live agreement report dedupes multiple predicted calls against the same real decision down to the latest one, so a contributor iterating on predict_gate can no longer inflate their own agreement sample. Gave predicted_gate_calls a 90-day retention window alongside the other append-only log tables. - PostHog no longer sends raw repo/PR/owner/SHA identifiers when reporting through the shared central key: they're HMAC'd with the same per-instance secret orb-collector.ts already uses. initPostHog now honors ORB_AIR_GAP and an explicit POSTHOG_DISABLED, and an empty POSTHOG_API_KEY means off instead of falling through to the central key. Self-hosted /metrics now pseudonymizes the repo label on the private-repo-scoped counters by default (with an explicit opt-out for an operator who wants raw labels on a verified private network), and observe() now routes through the same redaction path incr()/gaugeVector() already use. Fixed a scrubber bug where the vocabulary redaction pattern corrupted structured identifiers like repo names (e.g. "ossf/scorecard" -> "ossf/private context") by scoping vocabulary scrubbing away from identifier-shaped keys while keeping credential-shape scrubbing unconditional. Also fixed two stray NUL bytes in predicted-gate-agreement.ts's map-key join (pre-existing source corruption that made every past diff of that file render as binary) while touching the exact lines they sat on. Closes #9137 Closes #9138 Closes #9142 --- .../src/lib/selfhost-env-reference.ts | 10 ++ src/db/retention.ts | 4 + src/mcp/server.ts | 68 +++++++- src/review/predicted-gate-agreement.ts | Bin 9820 -> 11191 bytes src/selfhost/metrics.ts | 34 +++- src/selfhost/posthog.ts | 52 ++++++- src/selfhost/redaction-scrub.ts | 17 ++ src/server.ts | 18 ++- test/unit/mcp-admin-redeploy-tool.test.ts | 14 ++ test/unit/mcp-mutating-tools-audit.test.ts | 147 ++++++++++++++++++ test/unit/mcp-predict-gate.test.ts | 49 ++++++ test/unit/predicted-gate-agreement.test.ts | 62 +++++++- test/unit/retention.test.ts | 26 ++++ test/unit/selfhost-metrics.test.ts | 67 +++++++- test/unit/selfhost-posthog.test.ts | 106 +++++++++++++ test/unit/selfhost-redaction-scrub.test.ts | 89 +++++++++++ 16 files changed, 740 insertions(+), 23 deletions(-) create mode 100644 test/unit/mcp-mutating-tools-audit.test.ts create mode 100644 test/unit/selfhost-redaction-scrub.test.ts diff --git a/apps/loopover-ui/src/lib/selfhost-env-reference.ts b/apps/loopover-ui/src/lib/selfhost-env-reference.ts index f963f7bb77..d2455fabdf 100644 --- a/apps/loopover-ui/src/lib/selfhost-env-reference.ts +++ b/apps/loopover-ui/src/lib/selfhost-env-reference.ts @@ -281,6 +281,10 @@ export const SELFHOST_ENV_REFERENCE_ROWS: SelfHostEnvReferenceRow[] = [ name: "LOOPOVER_MCP_TOKEN", firstReference: "src/selfhost/preflight.ts", }, + { + name: "LOOPOVER_METRICS_REPO_LABELS", + firstReference: "src/server.ts", + }, { name: "LOOPOVER_REPO_CONFIG_DIR", firstReference: "src/server.ts", @@ -481,6 +485,10 @@ export const SELFHOST_ENV_REFERENCE_ROWS: SelfHostEnvReferenceRow[] = [ name: "POSTHOG_API_KEY", firstReference: "src/selfhost/otel.ts", }, + { + name: "POSTHOG_DISABLED", + firstReference: "src/selfhost/posthog.ts", + }, { name: "POSTHOG_ENVIRONMENT", firstReference: "src/selfhost/otel.ts", @@ -703,6 +711,7 @@ export const SELFHOST_ENV_REFERENCE_MARKDOWN = [ "| `LOOPOVER_ENABLE_PAGERDUTY` | `src/services/notify-pagerduty.ts` |", "| `LOOPOVER_ENABLE_UNSAFE_CODEX_REVIEWER` | `src/selfhost/ai.ts` |", "| `LOOPOVER_MCP_TOKEN` | `src/selfhost/preflight.ts` |", + "| `LOOPOVER_METRICS_REPO_LABELS` | `src/server.ts` |", "| `LOOPOVER_REPO_CONFIG_DIR` | `src/server.ts` |", "| `LOOPOVER_REVIEW_CONTINUOUS` | `src/queue/processors.ts` |", "| `LOOPOVER_REVIEW_RAG` | `src/selfhost/ai.ts` |", @@ -753,6 +762,7 @@ export const SELFHOST_ENV_REFERENCE_MARKDOWN = [ "| `PGVECTOR_ENABLED` | `src/server.ts` |", "| `PORT` | `src/server.ts` |", "| `POSTHOG_API_KEY` | `src/selfhost/otel.ts` |", + "| `POSTHOG_DISABLED` | `src/selfhost/posthog.ts` |", "| `POSTHOG_ENVIRONMENT` | `src/selfhost/otel.ts` |", "| `POSTHOG_HOST` | `src/selfhost/otel.ts` |", "| `POSTHOG_MIN_SEVERITY` | `src/selfhost/posthog.ts` |", diff --git a/src/db/retention.ts b/src/db/retention.ts index e1fda585b9..785753b60d 100644 --- a/src/db/retention.ts +++ b/src/db/retention.ts @@ -29,6 +29,10 @@ export const RETENTION_POLICY: readonly RetentionRule[] = [ { table: "webhook_events", column: "received_at", days: 90 }, // One row per outbound notification delivery (#8899); same append-only log shape as webhook_events. { table: "notification_deliveries", column: "created_at", days: 90 }, + // #9138: one row per loopover_predict_gate/explain_gate_disposition MCP call (unbounded, contributor- + // driven growth -- predicted-gate-calls.ts deliberately never dedups at write time, see that file's own + // header comment) -- same 90-day append-only-log window as audit_events/ai_usage_events above. + { table: "predicted_gate_calls", column: "created_at", days: 90 }, ]; export type PruneResult = { table: string; column: string; cutoff: string; deleted: number }; diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 41895c2e20..09006f4974 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -76,6 +76,7 @@ import { MAX_NOTIFICATION_DELIVERY_ID_LENGTH, MAX_NOTIFICATION_MARK_READ_IDS, markNotificationDeliveriesRead, + recordAuditEvent, recordProductUsageEvent, } from "../db/repositories"; import { decidePendingAgentAction } from "../services/agent-approval-queue"; @@ -4045,6 +4046,21 @@ export class LoopoverMcp { } const result = input.scope === "global" ? await functions.writeGlobal(input.content) : await functions.writeRepo(input.repoFullName!, input.content); + // #9137: rewrites the instance's private .loopover.yml FLEET-WIDE (global scope) or per-repo -- the + // sharpest unaudited write this issue calls out (previously left only a `.bak-` file on disk). + // A dedicated event type, not repo.settings_updated: this is the raw config FILE the focus-manifest + // loader reads, not the DB-backed RepositorySettings row. Audited on failure too, so "who attempted a + // write, and when" is answerable even when the write itself was rejected. + await recordAuditEvent(this.env, { + eventType: "config.private_write", + actor: this.identity.actor, + targetKey: input.scope === "global" ? "global" : input.repoFullName!, + outcome: result.ok ? "success" : "error", + detail: result.ok + ? `Wrote ${input.scope} config to ${result.path}${result.backupPath ? ` (backed up to ${result.backupPath})` : ""}.` + : `Failed to write ${input.scope} config: ${result.error}`, + metadata: { scope: input.scope, ...(input.repoFullName ? { repoFullName: input.repoFullName } : {}), ok: result.ok }, + }); if (!result.ok) { return { summary: `LoopOver admin config write failed: ${result.error}`, @@ -4088,6 +4104,18 @@ export class LoopoverMcp { } try { const result = await trigger(input.image); + // #9137: redeploys the instance -- audited regardless of outcome so "who triggered a redeploy, and + // when" is answerable even when the companion itself reports a failed run. + await recordAuditEvent(this.env, { + eventType: "instance.redeploy_triggered", + actor: this.identity.actor, + targetKey: input.image ?? "default", + outcome: result.ok ? "success" : "error", + detail: result.ok + ? `Redeploy completed successfully${input.image ? ` (${input.image})` : ""}.` + : `Redeploy failed (exit ${result.exitCode ?? "unknown"}): ${result.error ?? "see log"}.`, + metadata: { image: input.image ?? null, ok: result.ok, exitCode: result.exitCode ?? null }, + }); return { summary: result.ok ? `LoopOver redeploy: completed successfully${input.image ? ` (${input.image})` : ""}.` @@ -4097,9 +4125,18 @@ export class LoopoverMcp { } catch (error) { // A connection/protocol failure to the companion itself (socket missing, timeout, unauthorized) -- // distinct from a redeploy that ran and failed (handled above via result.ok === false). + const message = error instanceof Error ? error.message : String(error); + await recordAuditEvent(this.env, { + eventType: "instance.redeploy_triggered", + actor: this.identity.actor, + targetKey: input.image ?? "default", + outcome: "error", + detail: `Could not reach the host companion: ${message}`, + metadata: { image: input.image ?? null, ok: false, exitCode: null }, + }); return { - summary: `LoopOver redeploy trigger: could not reach the host companion: ${error instanceof Error ? error.message : String(error)}`, - data: { configured: true, ok: false, exitCode: null, error: error instanceof Error ? error.message : String(error) }, + summary: `LoopOver redeploy trigger: could not reach the host companion: ${message}`, + data: { configured: true, ok: false, exitCode: null, error: message }, }; } } @@ -4497,6 +4534,10 @@ export class LoopoverMcp { private async computePredictedGateVerdict( input: z.infer>, ): Promise<{ repoFullName: string; verdict: PredictedGateVerdict }> { + // #9138: shared by both loopover_predict_gate and loopover_explain_gate_disposition -- neither previously + // called enforceToolRateLimit, so the only ceiling was the shared /mcp route class (120/min), well above + // what's needed to flood predicted_gate_calls and drive the agreement metric toward 100%. + await this.enforceToolRateLimit("loopover_predict_gate"); this.requireContributorAccess(input.login); const repoFullName = `${input.owner}/${input.repo}`; await this.requireRepoAccess(repoFullName); @@ -4873,6 +4914,19 @@ export class LoopoverMcp { await this.requireRepoManageAccess(fullName); const current = await getRepositorySettings(this.env, fullName); const updated = await upsertRepositorySettings(this.env, { ...current, agentPaused: input.paused }); + // #9137: mirror PUT /v1/repos/:owner/:repo/settings' own audit (src/api/routes.ts) -- this is the kill + // switch, and was previously the only mutating write in this file with no audit_events row at all. + await recordAuditEvent(this.env, { + eventType: "repo.settings_updated", + actor: this.identity.actor, + targetKey: fullName, + outcome: "success", + detail: `Agent actions ${input.paused ? "paused" : "resumed"} for ${fullName} via MCP.`, + // input.paused (never undefined, per the Zod-validated shape), not updated.agentPaused (typed + // boolean | undefined for other RepositorySettings read paths) -- upsertRepositorySettings persists + // exactly this value, so recording the request avoids an always-false `?? fallback` for TS alone. + metadata: { repoFullName: fullName, fields: ["agentPaused"], agentPaused: input.paused }, + }); // #9018: a paused->live transition performs no catch-up by default. A PR that went GREEN during the pause // window (CI-completion passes plan-and-suppress the whole time) can be permanently stranded: if it was // ALSO already regated once before the pause, agent-sweep.ts's #never-endless-reregate rule excludes it @@ -4897,6 +4951,16 @@ export class LoopoverMcp { const current = await getRepositorySettings(this.env, fullName); const autonomy = { ...current.autonomy, [input.action]: input.level }; const updated = await upsertRepositorySettings(this.env, { ...current, autonomy }); + // #9137: same audit gap as setAgentPaused above -- the autonomy dial (e.g. merge: auto) is the other half + // of the kill-switch/autonomy pair this issue calls out by name. + await recordAuditEvent(this.env, { + eventType: "repo.settings_updated", + actor: this.identity.actor, + targetKey: fullName, + outcome: "success", + detail: `Set ${input.action} autonomy to ${input.level} for ${fullName} via MCP.`, + metadata: { repoFullName: fullName, fields: ["autonomy"], action: input.action, level: input.level }, + }); return { summary: `Set ${input.action} autonomy to ${input.level} for ${fullName}.`, data: { repoFullName: fullName, action: input.action, level: input.level, autonomy: updated.autonomy }, diff --git a/src/review/predicted-gate-agreement.ts b/src/review/predicted-gate-agreement.ts index cf768b9ae838f56cb23782f62d81a49088b713e4..4214e49373811edb84965321fd98cef053ae1da7 100644 GIT binary patch delta 1461 zcmb7E!A=uV6pgrR+|_7YkX*yUj%iDb8v(%(Y)A|gQb<@B18-*Tv=83Qo6LJdX+--C zroZ3^n6Pl+uNc3@^X3he=mOj|Z|=S4+;h(D&zQq`XBVDSPLiUYiW>LG> z-|Dp~(+($M%DJd!cj-u4)LS4+)5h#MlJ*rc8&VE58QJ5ZuuOfjG6jv06J0RIF%>K- zmMKq*A(PA!h03_*c~H?b)Dl??m=YTd3+9wFJ3v`L;RLveh%6v=0)S4Z5`r)AMvBit z)(Mj`G>AwVx0osOTpNH`jSs?<;8O4dBaIbqL#w;9druxg5W^}VW2I7b#+(~+11Ht` z&kX5tMui?z%P^Wo!n)z9j!VV(I${xX5fD}-9U3mqMJScDlQq;zLovr?4n|}moqIBl z>E`zKptlVl>0wPe(rFI&VoWUf>T7eQagVNkE%q0t%ga<5xC-f&61m};V68fOTnZ{~ zG_I_5{zF6|Gvkb7I@sLpRW5)~B#qP=#cnZZb6WbE1`&|zY{9fP#F$|A6zc2nl%ceY zVF`9~Cb+3}%$&QO`P@n?7RaXiB{MpK4sdRAvV0Pj(HYwlr-$%(Fe5}n3r?^$Y-y9x zUOA_kNSLPMI)H-ABm=B<7Gr0v$}|h^6i|<)w9YlGJ-$m^kr^M=cmJPN5BFDKA?d8s zd_2XjIlJ`Zn(IrPpA?*Rk;a$3e(zddf5mO5c&(X8nMRlg<3wi0F8!KZ1iUY)zvF&= z|A=rx(st1dpaGiP@fo}Cj}}?j=VAa4-(E*!oiZNNuE<~Cd>-g=o9ZTxw}6M2jaINW z3+M~N`hz|%O!)$4yT63>+fcegwk@BaU=7@eWJ0Z{^8uHa{tqZZyr9;xu(Sbr{P^yl z*Wf15G_wEh(=~IXb~UrY`_uraJKGjifhg}i7hmzgX?BZD0e!4!`}P7G7y9bW(&GK! D<}Upa delta 112 zcmdlUe#d8nvan`qVseH;d1A3bqC$FVUTJ1tszPCDYH>+sex5>Nadv8o4wr(KLSkOZ zDiHX+chAa$-qp@#aubOD;x+&HeIzOq2Un`hZM1 M)jFolQ#6#B0m+6Y5dZ)H diff --git a/src/selfhost/metrics.ts b/src/selfhost/metrics.ts index fca3d29bf5..6e4e23cefa 100644 --- a/src/selfhost/metrics.ts +++ b/src/selfhost/metrics.ts @@ -193,6 +193,23 @@ export function setSelfHostedMetricsMode(isSelfHosted: boolean): void { selfHostedMetricsMode = isSelfHosted; } +// #9142: self-host mode used to serve the RAW repo label unchanged on PRIVATE_REPO_LABEL_METRICS -- /metrics +// is commonly scraped by an operator's reverse proxy before any application auth, and every self-hosted +// instance calls setSelfHostedMetricsMode(true) unconditionally at boot (server.ts), so every self-host +// deployment shipped raw private repo names on an effectively-unauthenticated endpoint by default. The +// pseudonym scheme below (stable per-repo redacted-N label, the SAME one ALWAYS_REDACT_REPO_LABEL_METRICS +// already uses) is now the default instead -- an operator's own dashboards still get a stable per-repo series +// to group by, just not the real name. An operator who has verified /metrics never leaves their private +// network and wants real repo names can opt back in with LOOPOVER_METRICS_REPO_LABELS=raw. +let rawSelfHostedRepoLabels = false; + +/** Call ONCE at boot (self-host entrypoint only, alongside setSelfHostedMetricsMode) to opt back into raw + * (non-pseudonymized) repo labels on PRIVATE_REPO_LABEL_METRICS -- an explicit operator choice, never the + * default. Never called on the shared cloud worker. */ +export function setSelfHostedRawRepoLabels(allowRaw: boolean): void { + rawSelfHostedRepoLabels = allowRaw; +} + const PRIVATE_REPO_LABEL_METRICS = new Set([ "loopover_gate_decisions_total", "loopover_reviews_published_total", @@ -216,7 +233,11 @@ function redactedRepoLabel(repo: string): string { function publicLabelsForMetric(name: string, labels?: Labels): Labels | undefined { if (!labels || !("repo" in labels)) return labels; if (ALWAYS_REDACT_REPO_LABEL_METRICS.has(name)) return { ...labels, repo: redactedRepoLabel(labels.repo) }; - if (selfHostedMetricsMode || !PRIVATE_REPO_LABEL_METRICS.has(name)) return labels; + if (!PRIVATE_REPO_LABEL_METRICS.has(name)) return labels; + // #9142: self-host mode defaults to the SAME pseudonym scheme as ALWAYS_REDACT_REPO_LABEL_METRICS (not the + // cloud path's outright strip below -- a self-hosted operator legitimately wants a stable per-repo series + // on their OWN metrics), unless the operator explicitly opted into raw labels. + if (selfHostedMetricsMode) return rawSelfHostedRepoLabels ? labels : { ...labels, repo: redactedRepoLabel(labels.repo) }; const publicLabels = { ...labels }; delete publicLabels.repo; return Object.keys(publicLabels).length > 0 ? publicLabels : undefined; @@ -299,12 +320,17 @@ export function gaugeVector(name: string, sample: GaugeVectorSample): void { gaugeVectors.set(name, sample); } -/** Observe a value into a histogram (created on first use). `buckets` must be ascending upper bounds. */ +/** Observe a value into a histogram (created on first use). `buckets` must be ascending upper bounds. + * #9142: labels are routed through publicLabelsForMetric (the same redaction incr()/gaugeVector() already + * apply) BEFORE being used as the series key or stored on the histogram -- previously this was the one + * metric-recording path that bypassed redaction entirely, including the ALWAYS_REDACT set. No histogram + * carries a `repo` label today, so this changes nothing observable yet; it closes the gap before one does. */ export function observe(name: string, value: number, labels?: Labels, buckets: number[] = DEFAULT_BUCKETS): void { - const k = seriesKey(name, labels); + const publicLabels = publicLabelsForMetric(name, labels); + const k = seriesKey(name, publicLabels); let h = histograms.get(k); if (!h) { - h = { name, labels, buckets, counts: new Array(buckets.length).fill(0), sum: 0, count: 0 }; + h = { name, labels: publicLabels, buckets, counts: new Array(buckets.length).fill(0), sum: 0, count: 0 }; histograms.set(k, h); } // Cumulative bucketing: bump every bucket whose upper bound is >= the value. diff --git a/src/selfhost/posthog.ts b/src/selfhost/posthog.ts index 6af1ca1828..e9ef51a935 100644 --- a/src/selfhost/posthog.ts +++ b/src/selfhost/posthog.ts @@ -37,6 +37,9 @@ import { scrubString, SECRET_KEY, } from "./redaction-scrub"; +// #9142: the SAME HMAC primitive orb-collector.ts's exportOrbBatch uses to anonymize repo/PR identifiers +// before they leave the box -- reused here so a repo/PR hashes identically across both telemetry paths. +import { hmacAnonymize } from "../../packages/loopover-engine/src/telemetry/anonymize.js"; type PostHogNs = typeof import("posthog-node"); type PostHogClient = InstanceType; @@ -46,6 +49,16 @@ let client: PostHogClient | undefined; let active = false; let posthogEnvironment = "production"; let activeRelease: string | undefined; +// #9142: true when the resolved key came from the shared LOOPOVER_CENTRAL_POSTHOG_KEY fallback rather than an +// operator's own explicit POSTHOG_API_KEY -- gates identifier anonymization in operationalProperties below. +// An operator's own key is their own private analytics project; only the fleet-wide shared one needs it. +let usingCentralPostHogKey = false; +// #9142: the shared per-instance HMAC secret (same value + persistence as orb-collector.ts's +// getOrCreateAnonSecret/"orb:anon_secret") used to anonymize repo/PR/owner/SHA identifiers before they reach +// the vendor's PostHog project via the shared central key. Injected lazily via setPostHogAnonSecret once the +// self-host DB backend exists (server.ts) -- this module never touches D1 directly, matching every other +// lazily-injected selfhost dependency (setLocalManifestReader, etc.). +let centralKeyAnonSecret: string | undefined; /** No per-user identity is tracked by this sink (operational error events, not user analytics) -- every event * shares one anonymous, constant distinct id, mirroring src/mcp/telemetry.ts's identical MCP_TELEMETRY_DISTINCT_ID @@ -109,9 +122,16 @@ export function scrubPostHogEvent(event: PostHogEventMessage | null): PostHogEve return event; } +/** #9142: the OPERATIONAL_TAG_KEYS entries that name a specific private repo/PR/org -- the ones that must + * never leave the box in the clear when the resolved API key is the SHARED LOOPOVER_CENTRAL_POSTHOG_KEY. + * Mirrors orb-collector.ts's repo_hash/pr_hash treatment exactly, reusing the same HMAC primitive + secret. */ +const CENTRAL_KEY_IDENTIFYING_KEYS = new Set(["repo", "repository", "owner", "pull", "pullNumber", "pr", "head_sha"]); + /** Build the properties bag for a captured error/log: hashes any installation id, then tags the shared * operational key allowlist (./redaction-scrub's OPERATIONAL_TAG_KEYS) alongside whatever else the caller - * passed. */ + * passed. #9142: when the active key is the shared central one, the identifying subset above is HMAC'd with + * the instance's own anonymization secret instead of passed through raw -- fail CLOSED (the field is + * dropped, never sent raw) if that secret hasn't been injected yet via setPostHogAnonSecret. */ function operationalProperties(context: Record | undefined): Record { const safeContext = context ? hashedInstallationContext(context) : {}; const normalized: Record = @@ -121,7 +141,13 @@ function operationalProperties(context: Record | undefined): Re const properties: Record = {}; for (const key of OPERATIONAL_TAG_KEYS) { const value = normalized[key]; - if (typeof value === "string" || typeof value === "number") properties[key] = value; + if (typeof value !== "string" && typeof value !== "number") continue; + if (usingCentralPostHogKey && CENTRAL_KEY_IDENTIFYING_KEYS.has(key)) { + if (!centralKeyAnonSecret) continue; // fail closed -- never send the raw identifier + properties[key] = hmacAnonymize(String(value), centralKeyAnonSecret); + continue; + } + properties[key] = value; } const trace = currentOtelTraceIds(); if (trace) { @@ -135,13 +161,24 @@ function operationalProperties(context: Record | undefined): Re * the SAME var #6235's MCP telemetry reads (env-var decision, #8287). `env` is real process.env, matching * initSentry's identical NodeJS.ProcessEnv shape. */ export async function initPostHog(env: NodeJS.ProcessEnv): Promise { + // #9142: air-gapped/offline deployments and an explicit kill switch both take precedence over any key -- + // mirrors orb-collector.ts's own ORB_AIR_GAP short-circuit, since the central-key path reports to the same + // loopover-owned project ORB_AIR_GAP already promises never to reach. + if ((env.ORB_AIR_GAP ?? "").toLowerCase() === "true") return false; + if ((env.POSTHOG_DISABLED ?? "").toLowerCase() === "true") return false; // #8626: POSTHOG_API_KEY is the operator's own explicit config (highest precedence, unchanged); when it is // unset, fall back to LOOPOVER_CENTRAL_POSTHOG_KEY -- the fleet-wide key the hosted control-plane injects into // a tenant container (control-plane/src/container-driver.ts) so a hosted image reports to the loopover-owned // project without the operator setting anything. A hosted-injected fallback, never a silent override of an // operator's explicit POSTHOG_API_KEY. When neither is set, initPostHog stays a complete no-op (returns false). - const apiKey = processEnvString(env, "POSTHOG_API_KEY") ?? processEnvString(env, "LOOPOVER_CENTRAL_POSTHOG_KEY"); + const explicitKey = processEnvString(env, "POSTHOG_API_KEY"); + // #9142: an explicitly-set-but-EMPTY POSTHOG_API_KEY ("") is the operator's own "off", not "unset" -- it + // must NOT fall through to the shared central key (previously indistinguishable from unset once nonBlank + // collapsed both to undefined). A genuinely unset var (the property is simply absent) still falls through. + if (!explicitKey && env.POSTHOG_API_KEY !== undefined) return false; + const apiKey = explicitKey ?? processEnvString(env, "LOOPOVER_CENTRAL_POSTHOG_KEY"); if (!apiKey) return false; + usingCentralPostHogKey = !explicitKey; await loadNodeHasher(); const { PostHog } = await import("posthog-node"); posthogEnvironment = processEnvString(env, "POSTHOG_ENVIRONMENT") ?? "production"; @@ -411,11 +448,20 @@ export async function shutdownPostHog(): Promise { await client.shutdown().catch(() => undefined); } +/** #9142: inject the shared per-instance HMAC secret used to anonymize identifying fields when the active key + * is the shared central one -- see {@link centralKeyAnonSecret}'s own doc comment for the full rationale. + * Pass undefined to clear it (test-only; also covered by resetPostHogForTest). */ +export function setPostHogAnonSecret(secret: string | undefined): void { + centralKeyAnonSecret = secret; +} + /** Test-only: reset module state between cases. */ export function resetPostHogForTest(): void { client = undefined; active = false; posthogEnvironment = "production"; activeRelease = undefined; + usingCentralPostHogKey = false; + centralKeyAnonSecret = undefined; resetRedactionScrubForTest(); } diff --git a/src/selfhost/redaction-scrub.ts b/src/selfhost/redaction-scrub.ts index b154455ad5..8ecec9b4f7 100644 --- a/src/selfhost/redaction-scrub.ts +++ b/src/selfhost/redaction-scrub.ts @@ -186,9 +186,26 @@ export function scrubUrl(value: string): string { } } +/** #9142: OPERATIONAL_TAG_KEYS are structured identifiers (repo full names, PR/pull numbers, SHAs, model ids, + * ...) -- never free text a human wrote. PUBLIC_UNSAFE_SCRUB's bare `\b(reward|score|wallet|hotkey|...)\w*\b` + * match and PRIVATE_TEXT's phrase list both assume they're scanning prose; over an identifier like + * "ossf/scorecard" (`/` is a non-word char, so `\b` still anchors at the segment start) PUBLIC_UNSAFE_SCRUB + * corrupts the very label a reader groups events/metrics by, and two differently-named repos whose second + * segment happens to start with the same vocabulary word (scorecard, score-keeper, ...) collapse into one + * series once corrupted identically. */ +const STRUCTURED_IDENTIFIER_KEYS = new Set(OPERATIONAL_TAG_KEYS); + +/** Credential-shape scrubbers ONLY (no vocabulary/phrase matching) -- used for {@link STRUCTURED_IDENTIFIER_KEYS}. + * A leaked token/JWT/query secret that landed in an identifier-shaped field by mistake is still a real leak, + * so this stays unconditional even for a field whose vocabulary scrubbing is skipped. */ +function scrubStructuredIdentifierString(value: string): string { + return value.replace(QUERY_SECRET_VALUE, `$1${REDACTED}`).replace(SECRET_VALUE, REDACTED).replace(JWT_VALUE, REDACTED); +} + export function scrubStringField(key: string, value: string): string { if (isUrlKey(key)) return scrubUrl(value); if (isQueryKey(key)) return scrubQueryString(value); + if (STRUCTURED_IDENTIFIER_KEYS.has(key)) return scrubStructuredIdentifierString(value); return scrubString(value); } diff --git a/src/server.ts b/src/server.ts index 4fc4227141..c2b2632709 100644 --- a/src/server.ts +++ b/src/server.ts @@ -43,7 +43,7 @@ import { timingSafeStrEqual, } from "./selfhost/setup-wizard"; import { createOrbRelayRegistrationState, isOrbBrokerMode, registerOrbRelayTargetWithRetry } from "./orb/broker-client"; -import { exportOrbBatch } from "./selfhost/orb-collector"; +import { exportOrbBatch, getOrCreateAnonSecret } from "./selfhost/orb-collector"; import { createD1Adapter, nodeSqliteDriver } from "./selfhost/d1-adapter"; import { loadFileSecrets } from "./selfhost/load-file-secrets"; import { @@ -61,7 +61,7 @@ import { } from "./selfhost/health"; import { clockSkewSampleAgeSeconds, clockSkewSecondsSample } from "./selfhost/clock-skew"; import { d1DatabaseSizeBytesSample, d1SignalSnapshotsRowsPerKeySample, d1TableRowCountSamples, isD1SizeProbeEnabled, runD1SizeProbe } from "./selfhost/d1-size-probe"; -import { gauge, gaugeVector, incr, observe, renderMetrics, setSelfHostedMetricsMode } from "./selfhost/metrics"; +import { gauge, gaugeVector, incr, observe, renderMetrics, setSelfHostedMetricsMode, setSelfHostedRawRepoLabels } from "./selfhost/metrics"; import { delayToNextWallClockBoundaryMs } from "./selfhost/cron-alignment"; import { runSelfHostMigrations } from "./selfhost/migrate"; import { createPgAdapter, tuneGithubRateLimitObservationsAutovacuum, widenGithubIdColumnsToBigint } from "./selfhost/pg-adapter"; @@ -91,6 +91,7 @@ import { flushPostHog, initPostHog, installPostHogStructuredLogForwarding, + setPostHogAnonSecret, shutdownPostHog, } from "./selfhost/posthog"; import { @@ -350,9 +351,12 @@ async function main(): Promise { const startedAt = Date.now(); // This entrypoint IS the self-host runtime by definition (the cloud worker never imports server.ts), so the // /metrics endpoint it serves is the operator's own private scrape target, not a publicly reachable one -- - // stop redacting the `repo` label PRIVATE_REPO_LABEL_METRICS otherwise drops for every deployment - // (#terminal-outcome-audit). + // stop STRIPPING the `repo` label PRIVATE_REPO_LABEL_METRICS otherwise drops for every deployment + // (#terminal-outcome-audit). #9142: this now defaults to a PSEUDONYMIZED repo label (not the raw name) -- + // /metrics is commonly exposed by a reverse proxy before any application auth. An operator who has verified + // /metrics never leaves their private network and wants real repo names can opt in explicitly. setSelfHostedMetricsMode(true); + setSelfHostedRawRepoLabels((process.env.LOOPOVER_METRICS_REPO_LABELS ?? "").toLowerCase() === "raw"); // Container-private per-repo config (self-host): register the LOOPOVER_REPO_CONFIG_DIR reader so the focus- // manifest loader prefers a mounted `{owner}__{repo}.yml`, deep-merged over an optional root `.loopover.yml` // global default, over the public `.loopover.yml` (review policy stays private; see @@ -479,6 +483,12 @@ async function main(): Promise { backend: dbBackend, }), ); + // #9142: inject the same per-instance HMAC secret orb-collector.ts's exportOrbBatch already generates and + // persists (system_flags "orb:anon_secret") so posthog.ts can anonymize repo/PR identifiers before they + // reach the shared LOOPOVER_CENTRAL_POSTHOG_KEY project -- deferred until here (rather than inside + // initPostHog, called above before the DB backend exists) since it needs a real DB handle. A no-op cost + // when PostHog never activated; setPostHogAnonSecret itself no-ops when the active key isn't the central one. + if (posthogEnabled) setPostHogAnonSecret(await getOrCreateAnonSecret(backend.db)); // Data-safety advisory (#8): warn LOUDLY at boot if running on a single SQLite file with no acknowledged backup, // so an operator doesn't run with zero durability while /ready answers 200. const sqliteBackupOpts = { diff --git a/test/unit/mcp-admin-redeploy-tool.test.ts b/test/unit/mcp-admin-redeploy-tool.test.ts index b14e5055a0..12cfa4aa33 100644 --- a/test/unit/mcp-admin-redeploy-tool.test.ts +++ b/test/unit/mcp-admin-redeploy-tool.test.ts @@ -120,6 +120,20 @@ describe("MCP admin redeploy tool: trigger call (#7723)", () => { expect(result.structuredContent).toEqual({ configured: true, ok: false, exitCode: 1, error: "health check timed out", log: ["pulling...", "restarting..."] }); }); + // #9137: exitCode/error nullish fallbacks ("unknown"/"see log") in both the summary AND the audit-event + // detail -- the test above always supplies both fields, so this exercises the OTHER side of each `??`. + it("falls back to 'unknown'/'see log' in the summary when a failed run reports no exitCode/error", async () => { + setRedeployTrigger(vi.fn().mockResolvedValue({ ok: false, exitCode: null, log: [] })); + const client = await connect(createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" })); + + const result = await client.callTool({ name: "loopover_admin_trigger_redeploy", arguments: {} }); + + expect(result.isError).toBeFalsy(); + const text = (result.content as Array<{ type: string; text: string }>)[0]?.text ?? ""; + expect(text).toContain("exit unknown"); + expect(text).toContain("see log"); + }); + it("catches a connection/protocol failure to the companion itself and reports it as a normal tool result, distinct from a ran-but-failed redeploy", async () => { setRedeployTrigger(vi.fn().mockRejectedValue(new Error("connect ECONNREFUSED /run/loopover-redeploy.sock"))); const client = await connect(createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" })); diff --git a/test/unit/mcp-mutating-tools-audit.test.ts b/test/unit/mcp-mutating-tools-audit.test.ts new file mode 100644 index 0000000000..88bfde6ade --- /dev/null +++ b/test/unit/mcp-mutating-tools-audit.test.ts @@ -0,0 +1,147 @@ +import { Client } from "@modelcontextprotocol/sdk/client/index.js"; +import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { LoopoverMcp } from "../../src/mcp/server"; +import { setConfigAdminFunctions } from "../../src/mcp/private-config-admin-registry"; +import { setRedeployTrigger } from "../../src/mcp/redeploy-companion-registry"; +import type { AuthIdentity } from "../../src/auth/security"; +import { listAuditEventsByType, upsertRepositoryFromGitHub } from "../../src/db/repositories"; +import { createTestEnv } from "../helpers/d1"; + +const MCP_IDENTITY: AuthIdentity = { kind: "static", actor: "mcp" }; +const MCP_ADMIN_IDENTITY: AuthIdentity = { kind: "static", actor: "mcp-admin" }; +const SINCE = "2000-01-01T00:00:00.000Z"; + +async function connect(env: Env, identity: AuthIdentity = MCP_IDENTITY) { + const server = new LoopoverMcp(env, identity).createServer(); + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + await server.connect(serverTransport); + const client = new Client({ name: "mcp-mutating-tools-audit-test", version: "0.1.0" }, { capabilities: {} }); + await client.connect(clientTransport); + return client; +} + +afterEach(() => { + setConfigAdminFunctions(null); + setRedeployTrigger(null); +}); + +// #9137: every mutating MCP tool must leave a forensic `audit_events` row -- previously NONE of them did +// (`grep -n "recordAuditEvent" src/mcp/server.ts` returned zero hits). Table-driven across the four tools the +// issue calls out by name: the kill switch, the autonomy dial, the private-config writer, and the redeploy +// trigger. Assertions read back via listAuditEventsByType (an exact-eventType, unscoped read) rather than +// loopover_get_agent_audit_feed / listAgentAuditEvents, which is intentionally scoped to `agent.action.%` / +// `agent.pending_action.%` event types keyed by a `repo#pr` target -- none of these four events are PR-scoped. +describe("MCP mutating tools leave an audit_events row (#9137)", () => { + it("loopover_set_agent_paused records repo.settings_updated with the actor, repo, and changed field", async () => { + const env = createTestEnv(); + await upsertRepositoryFromGitHub(env, { name: "repo", full_name: "owner/repo", private: false, owner: { login: "owner" } }, 5); + const client = await connect(env); + + const result = await client.callTool({ name: "loopover_set_agent_paused", arguments: { owner: "owner", repo: "repo", paused: true } }); + expect(result.isError).toBeFalsy(); + + const events = await listAuditEventsByType(env, "repo.settings_updated", SINCE); + const own = events.filter((event) => event.targetKey === "owner/repo"); + expect(own).toHaveLength(1); + expect(own[0]?.metadata).toMatchObject({ repoFullName: "owner/repo", fields: ["agentPaused"], agentPaused: true }); + }); + + it("loopover_set_action_autonomy records repo.settings_updated with the action and level", async () => { + const env = createTestEnv(); + await upsertRepositoryFromGitHub(env, { name: "repo", full_name: "owner/repo", private: false, owner: { login: "owner" } }, 5); + const client = await connect(env); + + const result = await client.callTool({ name: "loopover_set_action_autonomy", arguments: { owner: "owner", repo: "repo", action: "merge", level: "auto" } }); + expect(result.isError).toBeFalsy(); + + const events = await listAuditEventsByType(env, "repo.settings_updated", SINCE); + const own = events.filter((event) => event.targetKey === "owner/repo"); + expect(own).toHaveLength(1); + expect(own[0]?.metadata).toMatchObject({ repoFullName: "owner/repo", fields: ["autonomy"], action: "merge", level: "auto" }); + }); + + it("loopover_admin_write_config records config.private_write on both a successful and a failed write", async () => { + const env = createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" }); + const writeGlobal = vi.fn().mockResolvedValueOnce({ ok: true, path: ".loopover.yml", backupPath: null }).mockResolvedValueOnce({ ok: false, error: "Content is empty." }); + setConfigAdminFunctions({ readGlobal: vi.fn(), readRepo: vi.fn(), writeGlobal, writeRepo: vi.fn(), listBackups: vi.fn() }); + const client = await connect(env, MCP_ADMIN_IDENTITY); + + const ok = await client.callTool({ name: "loopover_admin_write_config", arguments: { scope: "global", content: "gate:\n mode: advisory\n" } }); + expect(ok.isError).toBeFalsy(); + const failed = await client.callTool({ name: "loopover_admin_write_config", arguments: { scope: "global", content: "" } }); + expect(failed.isError).toBeFalsy(); // a rejected write is a normal tool result, not an MCP-level error + + const events = await listAuditEventsByType(env, "config.private_write", SINCE); + expect(events).toHaveLength(2); + expect(events.find((event) => event.metadata.ok === true)).toMatchObject({ targetKey: "global", metadata: { scope: "global", ok: true } }); + expect(events.find((event) => event.metadata.ok === false)).toMatchObject({ targetKey: "global", metadata: { scope: "global", ok: false } }); + }); + + it("loopover_admin_write_config records config.private_write scoped to the target repo, not 'global'", async () => { + const env = createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" }); + const writeRepo = vi.fn().mockResolvedValue({ ok: true, path: "loopover/.loopover.yml", backupPath: null }); + setConfigAdminFunctions({ readGlobal: vi.fn(), readRepo: vi.fn(), writeGlobal: vi.fn(), writeRepo, listBackups: vi.fn() }); + const client = await connect(env, MCP_ADMIN_IDENTITY); + + const result = await client.callTool({ name: "loopover_admin_write_config", arguments: { scope: "repo", repoFullName: "JSONbored/loopover", content: "gate:\n mode: advisory\n" } }); + expect(result.isError).toBeFalsy(); + + const events = await listAuditEventsByType(env, "config.private_write", SINCE); + const own = events.filter((event) => event.targetKey === "JSONbored/loopover"); + expect(own).toHaveLength(1); + expect(own[0]?.metadata).toMatchObject({ scope: "repo", repoFullName: "JSONbored/loopover", ok: true }); + }); + + it("loopover_admin_trigger_redeploy records instance.redeploy_triggered on success, a failed run, and a companion connection error", async () => { + const env = createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" }); + const client = await connect(env, MCP_ADMIN_IDENTITY); + + setRedeployTrigger(vi.fn().mockResolvedValue({ ok: true, exitCode: 0, log: [] })); + await client.callTool({ name: "loopover_admin_trigger_redeploy", arguments: { image: "ghcr.io/jsonbored/loopover-selfhost:v1" } }); + + setRedeployTrigger(vi.fn().mockResolvedValue({ ok: false, exitCode: 1, error: "health check timed out", log: [] })); + await client.callTool({ name: "loopover_admin_trigger_redeploy", arguments: {} }); + + setRedeployTrigger(vi.fn().mockRejectedValue(new Error("connect ECONNREFUSED /run/loopover-redeploy.sock"))); + await client.callTool({ name: "loopover_admin_trigger_redeploy", arguments: {} }); + + const events = await listAuditEventsByType(env, "instance.redeploy_triggered", SINCE); + expect(events).toHaveLength(3); + expect(events.find((event) => event.metadata.ok === true)).toMatchObject({ + targetKey: "ghcr.io/jsonbored/loopover-selfhost:v1", + metadata: { ok: true, exitCode: 0 }, + }); + expect(events.find((event) => event.metadata.ok === false && event.metadata.exitCode === 1)).toMatchObject({ + targetKey: "default", + metadata: { ok: false, exitCode: 1 }, + }); + expect(events.find((event) => event.metadata.ok === false && event.metadata.exitCode === null)).toMatchObject({ + targetKey: "default", + metadata: { ok: false, exitCode: null }, + }); + }); + + // listAuditEventsByType (used above) deliberately doesn't project `actor` -- read it back directly, the + // same raw-SQL pattern the rest of the suite already uses for audit_events assertions (e.g. + // test/unit/github-client.test.ts, test/unit/impact-map.test.ts). + it("records the caller's own identity actor on the audit row, not a generic placeholder", async () => { + const env = createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" }); + await upsertRepositoryFromGitHub(env, { name: "repo", full_name: "owner/repo", private: false, owner: { login: "owner" } }, 5); + setRedeployTrigger(vi.fn().mockResolvedValue({ ok: true, exitCode: 0, log: [] })); + + const pausedClient = await connect(env); // default static "mcp" identity + await pausedClient.callTool({ name: "loopover_set_agent_paused", arguments: { owner: "owner", repo: "repo", paused: true } }); + const adminClient = await connect(env, MCP_ADMIN_IDENTITY); + await adminClient.callTool({ name: "loopover_admin_trigger_redeploy", arguments: {} }); + + const pauseRow = await env.DB.prepare("SELECT actor FROM audit_events WHERE event_type = ? AND target_key = ?") + .bind("repo.settings_updated", "owner/repo") + .first<{ actor: string | null }>(); + expect(pauseRow?.actor).toBe("mcp"); + const redeployRow = await env.DB.prepare("SELECT actor FROM audit_events WHERE event_type = ?") + .bind("instance.redeploy_triggered") + .first<{ actor: string | null }>(); + expect(redeployRow?.actor).toBe("mcp-admin"); + }); +}); diff --git a/test/unit/mcp-predict-gate.test.ts b/test/unit/mcp-predict-gate.test.ts index 3fc8e963a8..61a5617259 100644 --- a/test/unit/mcp-predict-gate.test.ts +++ b/test/unit/mcp-predict-gate.test.ts @@ -275,6 +275,55 @@ testExpectations: }); }); + // #9138: predict_gate/explain_gate_disposition previously called neither enforceToolRateLimit, so the only + // ceiling was the shared /mcp route class (120/min) -- comfortably enough for a contributor to flood + // predicted_gate_calls and drive the agreement metric toward 100% (see predicted-gate-agreement.test.ts's + // dedup coverage for the other half of that fix). Mirrors mcp-output-schemas.test.ts's own + // mockRateLimiter helper for the sibling slop-oracle tools. + describe("shares the per-tool rate limit with the sibling slop-oracle tools (#9138)", () => { + function mockRateLimiter(status: number, body: Record = {}): NonNullable { + return { + idFromName: (name: string) => name as unknown as DurableObjectId, + get: () => ({ + async fetch() { + return Response.json(body, { status }); + }, + }), + } as unknown as NonNullable; + } + + it("skips the rate-limit when RATE_LIMITER is absent (test/local env)", async () => { + const client = await connect(createTestEnv()); + const result = await client.callTool({ name: "loopover_predict_gate", arguments: { login: "miner1", owner: "acme", repo: "widgets", title: "x" } }); + expect(result.isError).toBeFalsy(); + }); + + it("allows the call when the tool rate-limit returns 200", async () => { + const env = createTestEnv({ RATE_LIMITER: mockRateLimiter(200, { allowed: true, remaining: 19 }) }); + const client = await connect(env); + const result = await client.callTool({ name: "loopover_predict_gate", arguments: { login: "miner1", owner: "acme", repo: "widgets", title: "x" } }); + expect(result.isError).toBeFalsy(); + }); + + it("blocks loopover_predict_gate with a rate-limit error when the limiter returns 429", async () => { + const env = createTestEnv({ RATE_LIMITER: mockRateLimiter(429, { retryAfterSeconds: 42 }) }); + const client = await connect(env); + const result = await client.callTool({ name: "loopover_predict_gate", arguments: { login: "miner1", owner: "acme", repo: "widgets", title: "x" } }); + expect(result.isError).toBe(true); + const text = (result.content as Array<{ type: string; text: string }>)[0]?.text ?? ""; + expect(text).toMatch(/rate limit exceeded/i); + }); + + it("also blocks loopover_explain_gate_disposition -- both tools share computePredictedGateVerdict's rate limit", async () => { + const env = createTestEnv({ RATE_LIMITER: mockRateLimiter(429, { retryAfterSeconds: 42 }) }); + const client = await connect(env); + const result = await client.callTool({ name: "loopover_explain_gate_disposition", arguments: { login: "miner1", owner: "acme", repo: "widgets", title: "x" } }); + expect(result.isError).toBe(true); + const text = (result.content as Array<{ type: string; text: string }>)[0]?.text ?? ""; + expect(text).toMatch(/rate limit exceeded/i); + }); + }); + describe("personalized calibration wiring (#2349)", () => { async function seedLedgerRow(env: Env, opts: { login: string; agreed: boolean; pullNumber: number }) { const now = new Date().toISOString(); diff --git a/test/unit/predicted-gate-agreement.test.ts b/test/unit/predicted-gate-agreement.test.ts index 49fa7a1a4f..736948f36e 100644 --- a/test/unit/predicted-gate-agreement.test.ts +++ b/test/unit/predicted-gate-agreement.test.ts @@ -64,7 +64,12 @@ describe("computePredictedGateAgreement — predicted-vs-live gate agreement (#p expect(row).toMatchObject({ pairedSamples: 1, disagree: 1, unsafeDisagreements: 0 }); }); - it("pairs MULTIPLE predicted calls (a contributor iterating) against the SAME eventual real decision", async () => { + // #9138 core regression: N predictions before one real decision must yield EXACTLY ONE paired sample -- + // previously every one of them paired against the same eventual decision, inflating pairedSamples (and + // agreementRate) by a factor of N. Only the LATEST predicted call (the one closest to, and agreeing with, + // the real merge) is kept; the earlier "hold" guess is dropped from the aggregate, not double-counted as a + // second disagreement. + it("#9138: collapses MULTIPLE predicted calls before one real decision down to exactly ONE paired sample (the latest call)", async () => { const env = createTestEnv(); await seedPredicted(env, { login: "octocat", project: "owner/repo", action: "hold", createdAt: T0 }); await seedPredicted(env, { login: "octocat", project: "owner/repo", action: "merge", createdAt: hoursAfter(T0, 1) }); @@ -72,8 +77,59 @@ describe("computePredictedGateAgreement — predicted-vs-live gate agreement (#p const report = await computePredictedGateAgreement(env, { days: 90, nowMs: NOW }); const row = report.rows.find((r) => r.project === "owner/repo"); - // Both predicted calls pair to the one real decision: the first (hold) disagrees, the second (merge) agrees. - expect(row).toMatchObject({ pairedSamples: 2, bothMerge: 1, disagree: 1 }); + // Exactly one paired sample: the latest call ("merge") agrees with the real "merge" -- the earlier + // ("hold") call is dropped entirely from the aggregate, not counted as its own disagreement. + expect(row).toMatchObject({ pairedSamples: 1, bothMerge: 1, bothHold: 0, disagree: 0, agreementRate: 1 }); + }); + + it("#9138: collapses N (many more than 2) predictions before one real decision down to exactly ONE paired sample", async () => { + const env = createTestEnv(); + const N = 50; // representative of the issue's "500 calls in ~5 min" trigger; kept smaller here for test speed + for (let i = 0; i < N; i++) { + await seedPredicted(env, { login: "octocat", project: "owner/repo", action: "merge", createdAt: hoursAfter(T0, i / 3600) }); + } + await seedReal(env, { login: "octocat", project: "owner/repo", decision: "merge", createdAt: hoursAfter(T0, 1) }); + + const report = await computePredictedGateAgreement(env, { days: 90, nowMs: NOW }); + const row = report.rows.find((r) => r.project === "owner/repo"); + // Without the dedup this would report pairedSamples: N, bothMerge: N, agreementRate: 1 -- a contributor + // flooding predict_gate could not, by itself, manufacture N samples of "signal". + expect(row).toMatchObject({ pairedSamples: 1, bothMerge: 1, agreementRate: 1 }); + }); + + it("#9138: keeps the chronologically-latest predicted call as the pairing even when it isn't the last one READ from the DB", async () => { + const env = createTestEnv(); + // predicted_gate_calls carries no ORDER BY on the read side, so the read order is not guaranteed to match + // insertion (chronological) order -- insert the LATER-timestamped call first, so a naive "last one wins" + // read-order assumption would keep the WRONG (earlier) one. The dedup must compare `created_at`, not rely + // on read/array order, to correctly keep the chronologically-latest call either way. + await seedPredicted(env, { login: "octocat", project: "owner/repo", action: "merge", createdAt: hoursAfter(T0, 5) }); // later, inserted first + await seedPredicted(env, { login: "octocat", project: "owner/repo", action: "hold", createdAt: hoursAfter(T0, 1) }); // earlier, inserted second + await seedReal(env, { login: "octocat", project: "owner/repo", decision: "merge", createdAt: hoursAfter(T0, 6) }); + + const report = await computePredictedGateAgreement(env, { days: 90, nowMs: NOW }); + const row = report.rows.find((r) => r.project === "owner/repo"); + // The chronologically-latest call ("merge" at +5h) is kept -- not overwritten by the earlier ("hold" at + // +1h) call even if that one happens to be read/iterated after it. + expect(row).toMatchObject({ pairedSamples: 1, bothMerge: 1, bothHold: 0, disagree: 0 }); + }); + + it("#9138: two separate real decisions for the same login each get their own paired sample, still deduped independently", async () => { + const env = createTestEnv(); + // First round: two predictions, then a real 'hold'. + await seedPredicted(env, { login: "octocat", project: "owner/repo", action: "merge", createdAt: T0 }); + await seedPredicted(env, { login: "octocat", project: "owner/repo", action: "hold", createdAt: hoursAfter(T0, 1) }); + await seedReal(env, { login: "octocat", project: "owner/repo", decision: "hold", createdAt: hoursAfter(T0, 2), pullNumber: 1 }); + // Second round, later: two more predictions, then a real 'merge'. + await seedPredicted(env, { login: "octocat", project: "owner/repo", action: "hold", createdAt: hoursAfter(T0, 10) }); + await seedPredicted(env, { login: "octocat", project: "owner/repo", action: "merge", createdAt: hoursAfter(T0, 11) }); + await seedReal(env, { login: "octocat", project: "owner/repo", decision: "merge", createdAt: hoursAfter(T0, 12), pullNumber: 2 }); + + const report = await computePredictedGateAgreement(env, { days: 90, nowMs: NOW }); + const row = report.rows.find((r) => r.project === "owner/repo"); + // Two real decisions -> two paired samples (one per real decision, each deduped to its own latest call), + // both agreeing (latest call before the hold was "hold"; latest call before the merge was "merge"). + expect(row).toMatchObject({ pairedSamples: 2, bothHold: 1, bothMerge: 1, disagree: 0 }); }); it("does not pair a predicted call with no real decision at all (project absent from the report)", async () => { diff --git a/test/unit/retention.test.ts b/test/unit/retention.test.ts index e3716a7dbf..1df1409828 100644 --- a/test/unit/retention.test.ts +++ b/test/unit/retention.test.ts @@ -166,6 +166,32 @@ describe("pruneExpiredRecords", () => { const rows = await env.DB.prepare("SELECT id FROM notification_deliveries").all<{ id: string }>(); expect(rows.results.map((row) => row.id)).toEqual(["nd-recent"]); }); + + // #9138: predicted_gate_calls (src/review/predicted-gate-calls.ts) deliberately never dedups at write time -- + // "every call gets its own row" -- so it needed the same retention path as the other unbounded, contributor- + // driven append-only log tables above. + it("prunes predicted_gate_calls older than 90d and keeps recent rows (#9138)", async () => { + const env = createTestEnv(); + await env.DB.prepare( + `INSERT INTO predicted_gate_calls (id, login, project, predicted_action, conclusion, reason_code, created_at) + VALUES + ('pgc-old-1', 'octocat', 'owner/repo', 'merge', 'success', 'success', ?), + ('pgc-old-2', 'octocat', 'owner/repo', 'hold', 'action_required', 'missing_linked_issue', ?), + ('pgc-recent', 'octocat', 'owner/repo', 'merge', 'success', 'success', ?)`, + ) + .bind(daysAgo(100), daysAgo(95), daysAgo(1)) + .run(); + + expect(RETENTION_POLICY.some((rule) => rule.table === "predicted_gate_calls" && rule.column === "created_at" && rule.days === 90)).toBe(true); + + const results = await pruneExpiredRecords(env, { + nowMs: NOW, + policy: [{ table: "predicted_gate_calls", column: "created_at", days: 90 }], + }); + expect(results[0]?.deleted).toBe(2); + const rows = await env.DB.prepare("SELECT id FROM predicted_gate_calls").all<{ id: string }>(); + expect(rows.results.map((row) => row.id)).toEqual(["pgc-recent"]); + }); }); describe("dedupeSignalSnapshots", () => { diff --git a/test/unit/selfhost-metrics.test.ts b/test/unit/selfhost-metrics.test.ts index 0c0fddddc3..89305e4765 100644 --- a/test/unit/selfhost-metrics.test.ts +++ b/test/unit/selfhost-metrics.test.ts @@ -2,13 +2,28 @@ import { readFileSync, readdirSync } from "node:fs"; import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; import { backupAcknowledgedGaugeValue } from "../../src/selfhost/health"; -import { DEFAULT_METRIC_META, counterValue, gauge, gaugeVector, hitRatio, incr, observe, registerMetricMeta, renderMetrics, resetMetrics, setSelfHostedMetricsMode } from "../../src/selfhost/metrics"; +import { + DEFAULT_METRIC_META, + counterValue, + gauge, + gaugeVector, + hitRatio, + incr, + observe, + registerMetricMeta, + renderMetrics, + resetMetrics, + setSelfHostedMetricsMode, + setSelfHostedRawRepoLabels, +} from "../../src/selfhost/metrics"; afterEach(() => { resetMetrics(); - // setSelfHostedMetricsMode is a separate module-level flag from the counters/gauges resetMetrics() clears -- - // reset it explicitly so a test that turns it on can never leak into an unrelated later test. + // setSelfHostedMetricsMode/setSelfHostedRawRepoLabels are separate module-level flags from the + // counters/gauges resetMetrics() clears -- reset them explicitly so a test that turns either on can never + // leak into an unrelated later test. setSelfHostedMetricsMode(false); + setSelfHostedRawRepoLabels(false); }); describe("metrics registry (#982)", () => { @@ -136,15 +151,32 @@ describe("metrics registry (#982)", () => { expect(await renderMetrics()).toContain('debug_total{repo="public-owner/public-repo"} 1'); }); - // #terminal-outcome-audit: a self-hosted instance's /metrics is the operator's own private scrape target, not - // a publicly reachable one -- setSelfHostedMetricsMode(true) (called once at self-host boot) must stop - // redacting `repo` from these counters so an operator can actually slice their OWN dashboards by repo. - it("setSelfHostedMetricsMode(true) stops redacting the repo label on the cloud-private counters", async () => { + // #terminal-outcome-audit / #9142: a self-hosted instance's /metrics is the operator's own private scrape + // target, not a publicly reachable one, but /metrics is commonly exposed by a reverse proxy before any + // application auth -- setSelfHostedMetricsMode(true) (called unconditionally at self-host boot) must NOT + // serve the raw repo name by default. It now pseudonymizes (the SAME redacted-N scheme + // ALWAYS_REDACT_REPO_LABEL_METRICS already uses) so an operator's own dashboards still get a stable + // per-repo series to group by, without the real name leaving the instance by default. + it("setSelfHostedMetricsMode(true) pseudonymizes (not strips, not raw) the repo label on the private counters by default", async () => { setSelfHostedMetricsMode(true); incr("loopover_gate_decisions_total", { repo: "owner/repo", conclusion: "success" }); incr("loopover_reviews_published_total", { repo: "owner/repo" }); incr("loopover_ops_anomaly_total", { repo: "owner/repo", kind: "review_burst" }); + const out = await renderMetrics(); + expect(out).toContain('loopover_gate_decisions_total{conclusion="success",repo="redacted-1"} 1'); + expect(out).toContain('loopover_reviews_published_total{repo="redacted-1"} 1'); + expect(out).toContain('loopover_ops_anomaly_total{kind="review_burst",repo="redacted-1"} 1'); + expect(out).not.toContain("owner/repo"); + }); + + it("setSelfHostedRawRepoLabels(true) restores raw repo labels in self-hosted metrics mode (explicit operator opt-in)", async () => { + setSelfHostedMetricsMode(true); + setSelfHostedRawRepoLabels(true); + incr("loopover_gate_decisions_total", { repo: "owner/repo", conclusion: "success" }); + incr("loopover_reviews_published_total", { repo: "owner/repo" }); + incr("loopover_ops_anomaly_total", { repo: "owner/repo", kind: "review_burst" }); + const out = await renderMetrics(); expect(out).toContain('loopover_gate_decisions_total{conclusion="success",repo="owner/repo"} 1'); expect(out).toContain('loopover_reviews_published_total{repo="owner/repo"} 1'); @@ -152,6 +184,16 @@ describe("metrics registry (#982)", () => { expect(out).toContain('repo="owner/repo"'); }); + it("setSelfHostedRawRepoLabels(true) has no effect while self-hosted metrics mode is off (still strips)", async () => { + setSelfHostedRawRepoLabels(true); + incr("loopover_gate_decisions_total", { repo: "owner/repo", conclusion: "success" }); + + const out = await renderMetrics(); + expect(out).toContain('loopover_gate_decisions_total{conclusion="success"} 1'); + expect(out).not.toContain("owner/repo"); + expect(out).not.toContain('repo="'); + }); + it("setSelfHostedMetricsMode(false) (the default) still redacts — byte-identical to the cloud worker", async () => { setSelfHostedMetricsMode(false); incr("loopover_agent_disposition_total", { repo: "owner/repo", action_class: "hold", blocker_class: "none", autonomy_level: "auto" }); @@ -333,6 +375,17 @@ describe("histograms (observe)", () => { resetMetrics(); expect(await renderMetrics()).toBe("\n"); }); + + // #9142: observe() used to build its series key and store its labels WITHOUT routing through + // publicLabelsForMetric -- the one metric-recording path that bypassed redaction entirely, including the + // ALWAYS_REDACT_REPO_LABEL_METRICS set incr()/gaugeVector() always honor. Reuses an ALWAYS_REDACT name here + // (rather than a PRIVATE_REPO_LABEL_METRICS one) so the assertion holds regardless of selfHostedMetricsMode. + it("redacts a repo label on a histogram observation the same as incr()/gaugeVector() (#9142)", async () => { + observe("loopover_merge_train_deferred_total", 1, { repo: "owner/repo" }); + const out = await renderMetrics(); + expect(out).toContain('loopover_merge_train_deferred_total_bucket{le="+Inf",repo="redacted-1"}'); + expect(out).not.toContain("owner/repo"); + }); }); describe("hitRatio (#2090)", () => { diff --git a/test/unit/selfhost-posthog.test.ts b/test/unit/selfhost-posthog.test.ts index 501919926b..7e49a6e2c0 100644 --- a/test/unit/selfhost-posthog.test.ts +++ b/test/unit/selfhost-posthog.test.ts @@ -35,6 +35,7 @@ import { resetPostHogForTest, resolvePostHogRelease, scrubPostHogEvent, + setPostHogAnonSecret, shutdownPostHog, withPostHogMonitor, } from "../../src/selfhost/posthog"; @@ -96,6 +97,111 @@ describe("initPostHog", () => { expect(enabled).toBe(false); expect(mocks.PostHog).not.toHaveBeenCalled(); }); + + // #9142 + it("stays a no-op when ORB_AIR_GAP=true, even with a real key configured", async () => { + const enabled = await initPostHog({ ORB_AIR_GAP: "true", POSTHOG_API_KEY: "phc_test_key" } as unknown as NodeJS.ProcessEnv); + expect(enabled).toBe(false); + expect(mocks.PostHog).not.toHaveBeenCalled(); + }); + + it("stays a no-op when POSTHOG_DISABLED=true, even with a real key configured", async () => { + const enabled = await initPostHog({ POSTHOG_DISABLED: "true", POSTHOG_API_KEY: "phc_test_key" } as unknown as NodeJS.ProcessEnv); + expect(enabled).toBe(false); + expect(mocks.PostHog).not.toHaveBeenCalled(); + }); + + it("treats an explicitly EMPTY POSTHOG_API_KEY as OFF, not falling through to LOOPOVER_CENTRAL_POSTHOG_KEY", async () => { + const enabled = await initPostHog({ POSTHOG_API_KEY: "", LOOPOVER_CENTRAL_POSTHOG_KEY: "phc_central" } as unknown as NodeJS.ProcessEnv); + expect(enabled).toBe(false); + expect(mocks.PostHog).not.toHaveBeenCalled(); + }); + + it("still falls through to LOOPOVER_CENTRAL_POSTHOG_KEY when POSTHOG_API_KEY is genuinely unset (not just blank)", async () => { + const enabled = await initPostHog({ LOOPOVER_CENTRAL_POSTHOG_KEY: "phc_central" } as unknown as NodeJS.ProcessEnv); + expect(enabled).toBe(true); + expect(mocks.PostHog).toHaveBeenCalledWith("phc_central", expect.anything()); + }); +}); + +// #9142: repo/PR/owner/SHA identifiers must be HMAC'd, never sent in the clear, when the active key is the +// shared LOOPOVER_CENTRAL_POSTHOG_KEY -- an operator's own explicit POSTHOG_API_KEY is unaffected (their own +// private analytics project has no cross-tenant concern). +describe("central-key identifier anonymization (#9142)", () => { + it("hashes repo/pull/head_sha instead of sending them raw when using the central key, with the injected secret", async () => { + await initPostHog({ LOOPOVER_CENTRAL_POSTHOG_KEY: "phc_central" } as unknown as NodeJS.ProcessEnv); + setPostHogAnonSecret("a".repeat(64)); + capturePostHogError(new Error("boom"), { repo: "owner/repo", pull: 7, head_sha: "abc123" }); + const properties = lastCapturedProperties(); + expect(properties.repo).not.toBe("owner/repo"); + expect(properties.repo).toMatch(/^[0-9a-f]{24}$/); + expect(properties.pull).not.toBe(7); + expect(properties.pull).toMatch(/^[0-9a-f]{24}$/); + expect(properties.head_sha).not.toBe("abc123"); + expect(properties.head_sha).toMatch(/^[0-9a-f]{24}$/); + }); + + it("hashes the SAME repo identically across two calls (stable per-instance secret)", async () => { + await initPostHog({ LOOPOVER_CENTRAL_POSTHOG_KEY: "phc_central" } as unknown as NodeJS.ProcessEnv); + setPostHogAnonSecret("a".repeat(64)); + capturePostHogError(new Error("boom1"), { repo: "owner/repo" }); + const first = lastCapturedProperties().repo; + capturePostHogError(new Error("boom2"), { repo: "owner/repo" }); + const second = lastCapturedProperties().repo; + expect(first).toBe(second); + }); + + it("hashes a DIFFERENT repo to a DIFFERENT value", async () => { + await initPostHog({ LOOPOVER_CENTRAL_POSTHOG_KEY: "phc_central" } as unknown as NodeJS.ProcessEnv); + setPostHogAnonSecret("a".repeat(64)); + capturePostHogError(new Error("boom1"), { repo: "owner/repo-a" }); + const a = lastCapturedProperties().repo; + capturePostHogError(new Error("boom2"), { repo: "owner/repo-b" }); + const b = lastCapturedProperties().repo; + expect(a).not.toBe(b); + }); + + it("drops (never sends raw) an identifying field when using the central key but no secret has been injected yet (fail closed)", async () => { + await initPostHog({ LOOPOVER_CENTRAL_POSTHOG_KEY: "phc_central" } as unknown as NodeJS.ProcessEnv); + // setPostHogAnonSecret deliberately not called -- simulates the early-boot race before server.ts wires it. + capturePostHogError(new Error("boom"), { repo: "owner/repo" }); + const properties = lastCapturedProperties(); + expect(properties.repo).toBeUndefined(); + }); + + it("leaves repo/pull raw when the active key is the operator's OWN POSTHOG_API_KEY, never the central one", async () => { + await initPostHog({ POSTHOG_API_KEY: "phc_operator" } as unknown as NodeJS.ProcessEnv); + setPostHogAnonSecret("a".repeat(64)); // even if a secret happens to be injected, it must not apply here + capturePostHogError(new Error("boom"), { repo: "owner/repo", pull: 7 }); + const properties = lastCapturedProperties(); + expect(properties.repo).toBe("owner/repo"); + expect(properties.pull).toBe(7); + }); + + it("leaves repo/pull raw when POSTHOG_API_KEY is set even though LOOPOVER_CENTRAL_POSTHOG_KEY is also present (#8626 precedence)", async () => { + await initPostHog({ POSTHOG_API_KEY: "phc_operator", LOOPOVER_CENTRAL_POSTHOG_KEY: "phc_central" } as unknown as NodeJS.ProcessEnv); + setPostHogAnonSecret("a".repeat(64)); + capturePostHogError(new Error("boom"), { repo: "owner/repo" }); + expect(lastCapturedProperties().repo).toBe("owner/repo"); + }); + + it("also anonymizes on the capturePostHogAiGeneration success path (#9142 ai.ts context)", async () => { + await initPostHog({ LOOPOVER_CENTRAL_POSTHOG_KEY: "phc_central" } as unknown as NodeJS.ProcessEnv); + setPostHogAnonSecret("a".repeat(64)); + capturePostHogAiGeneration({ provider: "ollama", model: "llama3.1", requestKind: "review", latencyMs: 1000, isError: false, context: { repo: "owner/repo", pullNumber: 7 } }); + const call = mocks.capture.mock.calls[0]?.[0]; + expect(call.properties.repo).not.toBe("owner/repo"); + expect(call.properties.repo).toMatch(/^[0-9a-f]{24}$/); + }); + + it("resetPostHogForTest clears both the central-key flag and the injected secret", async () => { + await initPostHog({ LOOPOVER_CENTRAL_POSTHOG_KEY: "phc_central" } as unknown as NodeJS.ProcessEnv); + setPostHogAnonSecret("a".repeat(64)); + resetPostHogForTest(); + await initPostHog({ POSTHOG_API_KEY: "phc_operator" } as unknown as NodeJS.ProcessEnv); + capturePostHogError(new Error("boom"), { repo: "owner/repo" }); + expect(lastCapturedProperties().repo).toBe("owner/repo"); // operator key path, never hashed + }); }); describe("resolvePostHogRelease", () => { diff --git a/test/unit/selfhost-redaction-scrub.test.ts b/test/unit/selfhost-redaction-scrub.test.ts new file mode 100644 index 0000000000..8d74658054 --- /dev/null +++ b/test/unit/selfhost-redaction-scrub.test.ts @@ -0,0 +1,89 @@ +import { describe, expect, it } from "vitest"; +import { + OPERATIONAL_TAG_KEYS, + REDACTED, + scrubRecord, + scrubString, + scrubStringField, +} from "../../src/selfhost/redaction-scrub"; + +const fakeClassicAccessToken = (): string => `${"github" + "_pat_"}${"a".repeat(24)}`; + +describe("scrubStringField — structured identifiers skip vocabulary scrubbing (#9142)", () => { + // The sharpest, easiest-to-verify regression in the fix: PUBLIC_UNSAFE_SCRUB's bare `\b(reward|score| + // wallet|hotkey|...)\w*\b` match used to fire on the "score" segment inside "ossf/scorecard" (a repo full + // name, reached via scrubStringField's dispatch) and corrupt it into "ossf/private context" -- a real repo, + // reviewed on GitHub, whose label would silently corrupt on every single captured event. + it("REGRESSION (#9142): 'ossf/scorecard' survives scrubbing intact under the `repo` key", () => { + expect(scrubStringField("repo", "ossf/scorecard")).toBe("ossf/scorecard"); + }); + + it("still corrupts the SAME string under a non-identifier (free-text) key -- proves the bypass is key-scoped, not global", () => { + expect(scrubStringField("detail", "ossf/scorecard")).toBe("ossf/private context"); + expect(scrubString("ossf/scorecard")).toBe("ossf/private context"); + }); + + it.each(["reward", "wallet", "hotkey", "coldkey", "mnemonic", "payout", "ranking", "cohort"])( + "does not corrupt a repo name whose segment starts with the vocabulary word '%s'", + (word) => { + const repo = `some-org/${word}-analytics`; + expect(scrubStringField("repo", repo)).toBe(repo); + }, + ); + + it("two differently-named repos that both start with a vocabulary word no longer collapse to the same corrupted label", () => { + const a = scrubStringField("repo", "org-a/scorecard-tools"); + const b = scrubStringField("repo", "org-b/score-keeper"); + expect(a).not.toBe(b); + expect(a).toBe("org-a/scorecard-tools"); + expect(b).toBe("org-b/score-keeper"); + }); + + it("applies to every OPERATIONAL_TAG_KEYS entry, not just repo", () => { + for (const key of OPERATIONAL_TAG_KEYS) { + expect(scrubStringField(key, "cohort-alpha")).toBe("cohort-alpha"); + } + }); + + it("still redacts a credential-shaped value even under a structured-identifier key (credential scrubbing stays unconditional)", () => { + const token = fakeClassicAccessToken(); + expect(scrubStringField("repo", `owner/${token}`)).toBe(`owner/${REDACTED}`); + }); + + it("still redacts a JWT-shaped value under a structured-identifier key", () => { + const jwt = "eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.dozjgNryP4J3jVmNHl0w5N_XgL0n3I9PYb0nSYw"; + expect(scrubStringField("head_sha", jwt)).toBe(REDACTED); + }); + + it("still redacts a query-string secret parameter under a structured-identifier key", () => { + expect(scrubStringField("repo", "owner/repo?token=shhhhh")).toBe(`owner/repo?token=${REDACTED}`); + }); + + it("a key not in OPERATIONAL_TAG_KEYS still gets full vocabulary scrubbing (baseline, unaffected by this change)", () => { + expect(scrubStringField("message", "reward estimate leaked")).toContain("private context"); + }); + + it("still routes a URL-shaped key through scrubUrl (unaffected by the identifier bypass ordering)", () => { + const result = scrubStringField("targetUrl", "https://example.com/x?token=shh"); + expect(result).not.toContain("shh"); + expect(result).toContain("https://example.com/x?token="); + }); + + it("still routes a query-shaped key through scrubQueryString (unaffected by the identifier bypass ordering)", () => { + // URLSearchParams re-serialization percent-encodes REDACTED's brackets ("[" / "]") -- pre-existing + // scrubQueryString behavior, unrelated to and unaffected by this change; just confirming routing. + const result = scrubStringField("query", "token=shh&other=value"); + expect(result).not.toContain("shh"); + expect(result).toContain("other=value"); + }); +}); + +describe("scrubRecord — end-to-end via the structured-identifier keys (#9142)", () => { + it("leaves a repo full name intact when scrubbing a whole properties bag", () => { + const properties: Record = { repo: "ossf/scorecard", pull: 7, detail: "reward estimate leaked" }; + scrubRecord(properties, 0); + expect(properties.repo).toBe("ossf/scorecard"); + expect(properties.pull).toBe(7); + expect(properties.detail).toContain("private context"); + }); +});