From 73acbaafca25dd4d60118cff7799526576172483 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 24 Jun 2026 05:20:21 -0700 Subject: [PATCH] fix(gate): unconditional secret-leak block; don't let inconclusive AI bury blockers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two AI-safety gaps in the gate: - The concrete-credential secret-leak scan only ran when GITTENSORY_REVIEW_SAFETY was ON and the repo was allowlisted, so a registered-but-not-allowlisted repo got zero secret-leak gate while the engine still auto-merged/closed. A real-format committed credential (github_token, aws_access_key, …) is a leak on any repo, so the scan now runs unconditionally. secretLeakFinding already filters to HARD_SECRET_KINDS, so it never mis-fires on config/workflow content; the prompt-injection defang and AI review stay flag-gated. - evaluateGateCheck returned a neutral "held for human" and dropped every configured deterministic blocker the moment an AI review was inconclusive, so a contributor who forced a non-JSON / budget-exhausted AI result could bury a PR's real violations (secret_leak, duplicate, missing-issue, slop, quality) in a hold. The inconclusive hold is now evaluated AFTER deterministic blockers and only when none fired — a real violation still blocks; a clean PR with an inconclusive AI is still held (never auto-closed on a model hiccup). --- src/queue/processors.ts | 21 +++++++++------------ src/review/safety.ts | 5 +++-- src/rules/advisory.ts | 28 +++++++++++++++------------- test/unit/gate-check-policy.test.ts | 14 ++++++++++++++ test/unit/safety-wiring.test.ts | 5 +++-- test/unit/safety.test.ts | 11 ++++++----- 6 files changed, 50 insertions(+), 34 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 4c9285a188..1bbdf928f3 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -172,7 +172,7 @@ import { loadRepoFocusManifest } from "../signals/focus-manifest-loader"; import { resolveRepositorySettings } from "../settings/repository-settings"; import type { LocalBranchAnalysisInput } from "../signals/local-branch"; import { runGittensoryAiReview } from "../services/ai-review"; -import { isSafetyEnabled, secretLeakFinding } from "../review/safety"; +import { secretLeakFinding } from "../review/safety"; import { buildReviewGroundingText, checkSummaryText as checkFailureSummaryText, isGroundingEnabled } from "../review/grounding-wire"; import { buildReviewRagContext, isRagEnabled } from "../review/rag-wire"; import { indexRepo, reindexChangedPaths } from "../review/rag-index"; @@ -2030,10 +2030,10 @@ export async function maybeAddSecretLeakFinding( files: Awaited> | null; }, ): Promise { - // Per-repo cutover gate (GITTENSORY_REVIEW_REPOS): the secret-leak scan activates for THIS repo only when - // it is allowlisted AND the global safety flag is ON. Empty/unset allowlist → no-op for every repo (the - // advisory is byte-identical to today) regardless of GITTENSORY_REVIEW_SAFETY. - if (!isSafetyEnabled(env) || !isConvergenceRepoAllowed(env, args.repoFullName)) return; + // UNCONDITIONAL (#audit-3.4): a CONCRETE, real-format committed credential (github_token, aws_access_key, …) + // is unambiguously a leak regardless of which repo it lands in, so the secret-leak hard block runs for every + // repo — NOT only the safety-flag-on / allowlisted ones. secretLeakFinding already filters to HARD_SECRET_KINDS + // (the weak heuristics that false-positive on config/workflow content are dropped), so this never mis-fires. try { const files = args.files ?? (await listPullRequestFiles(env, args.repoFullName, args.pullNumber)); const finding = secretLeakFinding(buildSecretScanDiff(files)); @@ -2408,17 +2408,14 @@ async function maybePublishPrPublicSurface( }); } - // Safety secrets-scan (convergence, flag-gated by GITTENSORY_REVIEW_SAFETY). Scans the diff and, on a hit, - // appends a critical `secret_leak` blocker BEFORE the gate evaluates. When the scan will actually run - // (safety flag ON + repo allowlisted), pass the shared resolved files so it scans the REAL diff (FIX B); - // otherwise pass the already-loaded files (or null) and let the scan early-return. Flag-OFF (default) is an - // immediate no-op → the advisory/gate is byte-identical. - const safetyWillRun = isSafetyEnabled(env) && isConvergenceRepoAllowed(env, repoFullName); + // Secrets-scan (#audit-3.4): always scans the REAL resolved diff and, on a CONCRETE credential hit, appends a + // critical `secret_leak` hard blocker BEFORE the gate evaluates — unconditionally, since a committed token is + // a leak on any repo. getReviewFiles() is memoized, so this reuses the already-loaded diff when present. await maybeAddSecretLeakFinding(env, { advisory, repoFullName, pullNumber: pr.number, - files: safetyWillRun ? await getReviewFiles() : reviewFiles, + files: await getReviewFiles(), }); // First-time-contributor grace (#552): compute the author's complete per-repo PR history diff --git a/src/review/safety.ts b/src/review/safety.ts index 929e41e24e..986cd93f83 100644 --- a/src/review/safety.ts +++ b/src/review/safety.ts @@ -71,8 +71,9 @@ export function defangReviewInput(input: SafetyReviewInput): { * (see rules/advisory.ts) so a leaked secret holds the PR. Only CONCRETE credential formats * ({@link HARD_SECRET_KINDS}) qualify — the weak `seed_or_mnemonic` / `bittensor_key` heuristics are ignored * here because they false-positive on legitimate config/workflow content (e.g. `coldkey:` / `hotkey =` lines - * in *.toml, .github/workflows/**, or wrangler/workers config). Callers MUST gate this on - * {@link isSafetyEnabled} — when OFF, no finding is produced so the advisory/gate is unchanged. + * in *.toml, .github/workflows/**, or wrangler/workers config). This is UNCONDITIONAL (#audit-3.4): a concrete, + * real-format committed credential is a leak on any repo, so the caller runs it regardless of the safety flag / + * review allowlist (unlike the prompt-injection defang, which stays flag-gated). */ export function secretLeakFinding(diff: string): AdvisoryFinding | null { // Scan ONLY additions — the secrets THIS change introduces. A token on a removed/context line is not being diff --git a/src/rules/advisory.ts b/src/rules/advisory.ts index 49be0fe099..ae4d7d9a07 100644 --- a/src/rules/advisory.ts +++ b/src/rules/advisory.ts @@ -359,19 +359,6 @@ export function evaluateGateCheck(advisoryResult: Advisory, policy: GateCheckPol warnings, }; } - // Fail-CLOSED AI hold (#ai-fail-closed): the block-mode AI review could not return a usable verdict, so the - // gate is HELD (neutral) for a human rather than passed automatically. NEVER a failure → a contributor PR is - // never auto-CLOSED because an AI model hiccupped; it re-evaluates on the next update. - if (advisoryResult.findings.some((finding) => finding.code === "ai_review_inconclusive")) { - return { - enabled: true, - conclusion: "neutral", - title: "Gittensory Gate — held for human review", - summary: "The AI review could not be completed for this change, so the gate is held for a human reviewer rather than passed automatically. It re-evaluates on the next update.", - blockers: [], - warnings, - }; - } // Merge-readiness composite (#551): when set, escalate every sub-gate to its mode so they roll into one // pass/fail. When off, this is a no-op and each sub-gate keeps its own mode. const effective = applyMergeReadinessGate(policy); @@ -404,6 +391,21 @@ export function evaluateGateCheck(advisoryResult: Advisory, policy: GateCheckPol }; } if (blockers.length === 0) { + // Fail-CLOSED AI hold (#ai-fail-closed, #audit-3.5): with NO deterministic blocker, a block-mode AI review + // that could not return a usable verdict HOLDS the gate (neutral) for a human rather than passing + // automatically — NEVER a failure, so a contributor PR is never auto-CLOSED because a model hiccupped. This + // is evaluated AFTER the deterministic blockers above, so a real violation (secret_leak, duplicate, + // missing-issue, slop, quality) still blocks: an inconclusive AI can no longer bury a blocked PR in a hold. + if (advisoryResult.findings.some((finding) => finding.code === "ai_review_inconclusive")) { + return { + enabled: true, + conclusion: "neutral", + title: "Gittensory Gate — held for human review", + summary: "The AI review could not be completed for this change, so the gate is held for a human reviewer rather than passed automatically. It re-evaluates on the next update.", + blockers: [], + warnings, + }; + } return { enabled: true, conclusion: "success", diff --git a/test/unit/gate-check-policy.test.ts b/test/unit/gate-check-policy.test.ts index 6f36fd6acc..e762c7766b 100644 --- a/test/unit/gate-check-policy.test.ts +++ b/test/unit/gate-check-policy.test.ts @@ -110,6 +110,20 @@ describe("AI fail-closed hold (#ai-fail-closed)", () => { expect(result.conclusion).toBe("neutral"); expect(result.blockers).toEqual([]); }); + + it("a deterministic hard blocker (secret_leak) still FAILS even when the AI review is inconclusive (#audit-3.5)", () => { + const adv: Advisory = { + ...missingIssueAdvisory(), + findings: [ + { code: "secret_leak", title: "Possible leaked secret", severity: "critical", detail: "a committed token", action: "remove and rotate it" }, + { code: "ai_review_inconclusive", title: "AI review could not be completed", severity: "warning", detail: "no usable verdict", action: "held for human" }, + ], + }; + const result = evaluateGateCheck(adv, gateCheckPolicy(settings(), null, true)); + // An inconclusive AI can no longer bury a real violation in a "held" state — the secret_leak still hard-blocks. + expect(result.conclusion).toBe("failure"); + expect(result.blockers.map((blocker) => blocker.code)).toContain("secret_leak"); + }); }); describe("policy pack (#692)", () => { diff --git a/test/unit/safety-wiring.test.ts b/test/unit/safety-wiring.test.ts index f2a90d296b..a78663334a 100644 --- a/test/unit/safety-wiring.test.ts +++ b/test/unit/safety-wiring.test.ts @@ -255,7 +255,7 @@ describe("secret-leak finding in the advisory build", () => { expect(out).toContain("### ok.ts (added) +2/-1\n@@\n+const a = 1;"); }); - it("FLAG-OFF (default): no secret_leak finding is produced — the advisory is unchanged", async () => { + it("FLAG-OFF: a concrete leaked secret STILL produces the secret_leak finding (unconditional, #audit-3.4)", async () => { const env = createTestEnv({ GITTENSORY_REVIEW_SAFETY: "false" }); const adv = advisory(); const files = [ @@ -278,7 +278,8 @@ describe("secret-leak finding in the advisory build", () => { pullNumber: 7, files, }); - expect(adv.findings).toEqual([]); + // The concrete-credential hard block does not depend on GITTENSORY_REVIEW_SAFETY. + expect(adv.findings.map((f) => f.code)).toContain("secret_leak"); }); it("FLAG-ON + files=null: lazily loads the changed files from D1 and still finds the leaked secret", async () => { diff --git a/test/unit/safety.test.ts b/test/unit/safety.test.ts index 720c834384..92b9afd2c8 100644 --- a/test/unit/safety.test.ts +++ b/test/unit/safety.test.ts @@ -130,7 +130,7 @@ describe("GITTENSORY_REVIEW_SAFETY secrets-scan wired into the review FINALIZE p expect(seen.conclusion).toBe("failure"); }); - it("FLAG-OFF (default): the SAME leaked secret produces NO blocker — the finalized gate is byte-identical (not failed on the secret)", async () => { + it("FLAG-OFF: a leaked secret STILL fails the gate — the concrete-credential block is unconditional (#audit-3.4)", async () => { const env = createTestEnv({ GITTENSORY_REVIEW_SAFETY: "false", GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await seedGateEnabledRepo(env); await seedLeakedSecretFile(env); @@ -141,11 +141,12 @@ describe("GITTENSORY_REVIEW_SAFETY secrets-scan wired into the review FINALIZE p } finally { vi.unstubAllGlobals(); } - // No secret_leak finding is produced flag-OFF → the gate is not driven to failure by the (ignored) secret. - expect(seen.conclusion).not.toBe("failure"); + // A real-format committed credential is a leak on any repo, so the secret_leak hard block fires regardless + // of GITTENSORY_REVIEW_SAFETY (only the prompt-injection defang / AI review remain flag-gated). + expect(seen.conclusion).toBe("failure"); }); - it("UNSET behaves identically to explicit-false (the flag-OFF branch is the default — no new branch taken)", async () => { + it("UNSET (default): a leaked secret also fails the gate — the secret-leak block does not depend on the flag", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); // GITTENSORY_REVIEW_SAFETY unset await seedGateEnabledRepo(env); await seedLeakedSecretFile(env); @@ -156,6 +157,6 @@ describe("GITTENSORY_REVIEW_SAFETY secrets-scan wired into the review FINALIZE p } finally { vi.unstubAllGlobals(); } - expect(seen.conclusion).not.toBe("failure"); + expect(seen.conclusion).toBe("failure"); }); });