From 305c4d201fadb26a9ad210ea621f26a439e2685a Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 14:04:54 -0700 Subject: [PATCH 1/2] fix(safety): make the kill switches ratchets, stop a dry-run GitHub write, subject content-inspection closes to the breaker #9049 -- VERIFIED LIVE. The global default private-config layer carries `agentDryRun: false`, and resolveEffectiveSettings merges as `{...dbSettings, ...manifestSettings}`, so the manifest wins. Setting `agent_dry_run=1` in the DB -- via the dashboard, PUT /settings, `loopover-mcp maintain`, or the loopover_set_agent_dry_run MCP tool -- was therefore SILENTLY DISCARDED on every read, for every repo. The tool writes the raw row back and never re-resolves effective settings, so the operator got a success message while ORB kept making real GitHub writes. A kill switch that reports success and does nothing is worse than one that does not exist. Both agentPaused and agentDryRun are now RATCHETS: `true` from EITHER layer wins. Config-as-code can still pause or dry-run a repo (tightening -- the whole point of the feature); it can no longer un-pause or un-dry-run what an operator set. Every other setting keeps manifest-wins semantics unchanged. #9067 -- the structural invariant is that every installation write routes through makeInstallationOctokit, whose request hook suppresses mutations when mode !== "live". dispatchVisualCaptureFallback escapes it with a raw timeoutFetch POST to .../dispatches -- a real write that starts an Actions run and burns CI minutes in the target repo. runVisualCapture short-circuits only on `paused`, so under `dry_run` it fired for real, contradicting the documented dry-run contract. The mode is now threaded onto CaptureTarget and the writer itself refuses anything that is not "live", with an absent mode treated as non-live -- so the guarantee is structural rather than dependent on each caller remembering a check. (This compounded with #9049: dry-run was defeated fleet-wide, which masked the misbehavior rather than preventing it.) #9086 (breaker half) -- downgradeCloseToHold keyed on `closeKind === "heuristic"` alone, so all six non-heuristic close paths were returned untouched by both the project breaker and the per-rule precision check. The "deterministic => zero-hallucination" justification holds for the IDENTITY-based closes (blacklist, contributor_cap read facts about an account) but not for the CONTENT-INSPECTION ones, whose determinism is a regex heuristic over free-text markdown -- and screenshot_table is our top close reason all-time. A deterministic rule can still be systematically wrong, which is what the breaker exists to catch. screenshot_table / review_nag / copycat are now breaker-eligible; identity-based closes keep their exemption. POLICY REVERSAL, called out explicitly: this inverts an existing test asserting screenshot_table is breaker-exempt. A companion test pins the identity-based exemptions so the narrowing is visible in both directions. Closes #9049 Closes #9067 Closes #9086 Tests: 5 ratchet cases through the REAL parseFocusManifest path (manifest false cannot clear either DB flag; manifest true still tightens; both-off stays off; absent key leaves the DB value intact in both directions), 2 dry-run refusal tests (dry_run and absent-mode both refuse to dispatch), and 2 breaker tests (screenshot_table now downgrades; blacklist/contributor_cap still exempt). 100% line+branch coverage on all 55 added lines; 1806/1806 across 8 suites; all drift gates green. --- src/queue/processors.ts | 2 + src/review/visual/actions-fallback.ts | 11 ++++ src/review/visual/capture.ts | 10 ++++ src/settings/agent-actions.ts | 18 +++++- src/signals/focus-manifest.ts | 16 ++++++ test/unit/agent-actions.test.ts | 23 +++++++- test/unit/focus-manifest.test.ts | 41 ++++++++++++++ test/unit/visual-capture.test.ts | 81 ++++++++++++++++++++++----- 8 files changed, 182 insertions(+), 20 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 18d4c921a5..beed9ff174 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -11573,6 +11573,8 @@ async function maybePublishPrPublicSurface( // Pins the actions_fallback dispatch (#4112) to a trusted ref -- see buildCapture. Absent (no // stored default branch yet) ⇒ that dispatch just never fires, same as leaving it unconfigured. ...(repo?.defaultBranch ? { defaultBranchRef: repo.defaultBranch } : {}), + // #9067: the actions_fallback dispatch is a real GitHub write and must obey dry_run/paused. + mode, }; // review.visual.enabled (#4083): a config-as-code override layered on top of the screenshotsAllowed // env-var gate above, not a replacement for it. Unset/true ⇒ defer to that gate's decision (buildCapture diff --git a/src/review/visual/actions-fallback.ts b/src/review/visual/actions-fallback.ts index 9788d08d4c..306c97ebc4 100644 --- a/src/review/visual/actions-fallback.ts +++ b/src/review/visual/actions-fallback.ts @@ -1,3 +1,4 @@ +import type { AgentActionMode } from "../../settings/agent-execution"; // GitHub-Actions build-and-serve FALLBACK for a repo with no CI-produced preview deploy (#4112, part of the // #3607 visual-capture convergence epic). // @@ -91,7 +92,17 @@ export async function dispatchVisualCaptureFallback(params: { headSha: string; routes: readonly string[]; rateLimitAdmissionKey?: GitHubRateLimitAdmissionKey | undefined; + /** #9067: the pass's resolved AgentActionMode. See the refusal below. */ + mode?: AgentActionMode | undefined; }): Promise { + // #9067: this is a real GitHub WRITE -- it starts an Actions run and burns CI minutes in the target repo -- + // issued through raw timeoutFetch, so it never passes makeInstallationOctokit's request hook, the structural + // check that suppresses every OTHER installation mutation when mode !== "live". runVisualCapture only + // short-circuited on `paused`, so under `dry_run` this fired for real, contradicting the documented dry-run + // contract ("suppresses the terminal GitHub-side write only"). Refusing HERE rather than at the call site + // makes the guarantee structural: a future caller cannot reintroduce the bypass by forgetting a check, + // because an absent mode is treated as non-live. + if (params.mode !== "live") return false; const base = `https://api.github.com/repos/${params.repo.owner}/${params.repo.repo}`; try { const headers = new Headers(); diff --git a/src/review/visual/capture.ts b/src/review/visual/capture.ts index 6c684bc2b8..a6e169ed91 100644 --- a/src/review/visual/capture.ts +++ b/src/review/visual/capture.ts @@ -1,3 +1,4 @@ +import type { AgentActionMode } from "../../settings/agent-execution"; // Realtime visual capture (reviewbot→loopover convergence — visual port). taopedia-style before/after. // // before = production (review.visual.production_url, falling back to the global PUBLIC_SITE_ORIGIN env var); @@ -236,6 +237,13 @@ export interface CaptureTarget { * trusted ref rather than the PR's own branch. Absent ⇒ the fallback is never dispatched (fail-safe: no * ref to pin to means no dispatch, not a guess at "main"). */ defaultBranchRef?: string | undefined; + /** #9067: the pass's resolved AgentActionMode. REQUIRED to dispatch the actions_fallback workflow -- that + * dispatch is a real GitHub WRITE (it starts an Actions run and burns CI minutes in the target repo), but + * it goes out via raw timeoutFetch and so bypasses makeInstallationOctokit's mutation-suppression hook, + * which is the structural invariant every other installation write relies on. runVisualCapture only ever + * short-circuited on `paused`, so under `dry_run` this fired for real -- directly contradicting the + * documented dry-run contract. Absent ⇒ treated as non-live and the dispatch never fires (fail-safe). */ + mode?: AgentActionMode | undefined; } function joinUrl(base: string, path: string): string { @@ -754,6 +762,8 @@ export async function buildCapture( headSha: target.headSha, routes, rateLimitAdmissionKey, + // #9067: threaded so the writer itself can refuse under dry_run/paused (see its own doc comment). + mode: target.mode, }); if (dispatched) await markFallbackDispatched(env, target.headSha); } diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 90cac036e4..9d8a6facad 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -612,10 +612,24 @@ export function downgradeCloseToHold( labelSettings: AgentDispositionLabelSettings = {}, untrustworthyRuleCodes: ReadonlySet = new Set(), ): PlannedAgentAction[] { + // #9086: the breaker exemption used to key on `closeKind === "heuristic"` alone, so all SIX non-heuristic + // close paths (blacklist, contributor_cap, review_nag, copycat, screenshot_table, and the linked-issue hard + // rule) were returned untouched by BOTH Reason A and Reason B — and none of them gets the executor-side live + // recheck either, which is likewise gated on "heuristic". + // + // The stated justification ("zero-hallucination, deterministic") holds for the IDENTITY-based closes: + // blacklist and contributor_cap read facts about an account, so a precision breaker has nothing to add. It + // does NOT hold for the CONTENT-INSPECTION closes, whose "determinism" is a regex heuristic over free-text + // markdown — screenshot_table is our single top close reason all-time. A deterministic rule can still be + // systematically wrong, which is precisely what the breaker exists to catch. + const CONTENT_INSPECTION_CLOSE_KINDS: ReadonlySet = new Set(["screenshot_table", "review_nag", "copycat"]); + const isBreakerEligibleClose = (action: PlannedAgentAction): boolean => + action.closeKind === "heuristic" || (action.closeKind !== undefined && CONTENT_INSPECTION_CLOSE_KINDS.has(action.closeKind)); + // Reason A (project-level, unchanged from before #7986): no concrete evidence at all, AND the project's // close-precision breaker has engaged. const noConcreteEvidenceUnderProjectBreaker = (action: PlannedAgentAction): boolean => - action.actionClass === "close" && action.closeKind === "heuristic" && action.closeConcreteEvidence !== true && closeHoldOnly; + action.actionClass === "close" && isBreakerEligibleClose(action) && action.closeConcreteEvidence !== true && closeHoldOnly; // Reason B (#7986, per-rule, independent of closeHoldOnly): HAS concrete evidence, but every code that // justified it has its own bad track record. `.every` (not `.some`) so a close backed by a MIX of a // trustworthy code and an untrustworthy one keeps its exemption via the trustworthy code -- only a close @@ -626,7 +640,7 @@ export function downgradeCloseToHold( }; const isDowngradableClose = (action: PlannedAgentAction): boolean => action.actionClass === "close" && - action.closeKind === "heuristic" && + isBreakerEligibleClose(action) && (noConcreteEvidenceUnderProjectBreaker(action) || (action.closeConcreteEvidence === true && everyJustifyingCodeUntrustworthy(action))); if (!planned.some(isDowngradableClose)) return planned; const labels = resolveAgentDispositionLabels(labelSettings); diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 73bfb63a7f..db8be8c003 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -568,9 +568,25 @@ export function resolveEffectiveSettings( unlinkedIssueGuardrail: unlinkedIssueGuardrailOverride, screenshotTableGate: screenshotTableGateOverride, advisoryAiRouting: advisoryAiRoutingOverride, + // #9049: the two KILL-SWITCH flags are pulled out of the wholesale spread and merged as RATCHETS below -- + // config-as-code may TIGHTEN safety but must never LOOSEN it. + agentPaused: agentPausedOverride, + agentDryRun: agentDryRunOverride, ...restManifestSettings } = manifest.settings; const effective: RepositorySettings = { ...dbSettings, ...restManifestSettings }; + // #9049: verified live -- the global default private-config layer carried `agentDryRun: false`, and because + // the manifest wins a plain spread, that silently discarded `agent_dry_run=1` on EVERY read for EVERY repo. + // An operator flipping dry-run from the dashboard, `PUT /settings`, `loopover-mcp maintain`, or the + // `loopover_set_agent_dry_run` MCP tool got a success message while ORB kept making real GitHub writes -- + // the tool writes the raw row back and never re-resolves effective settings, so nothing surfaced the + // discard. A kill switch that reports success and does nothing is worse than one that does not exist. + // + // Both flags are therefore RATCHETS: `true` from EITHER layer wins. A repo's `.loopover.yml` can still pause + // or dry-run itself (tightening), which is the whole point of config-as-code here; it simply cannot un-pause + // or un-dry-run what an operator set in the DB. Every other setting keeps manifest-wins semantics unchanged. + effective.agentPaused = dbSettings.agentPaused === true || agentPausedOverride === true; + effective.agentDryRun = dbSettings.agentDryRun === true || agentDryRunOverride === true; if (typeLabelsOverride !== undefined) { // `null` is parseFocusManifest's distinct signal for a literal `typeLabels: {}` -- a deliberate // "zero configured categories for this repo" that REPLACES the DB value wholesale, rather than a diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index ffb3b66d4e..70c125ec4f 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -2522,12 +2522,29 @@ describe("screenshot-table gate short-circuit (#2006)", () => { expect(classes(planAgentMaintenanceActions(missingTable({ autonomy: { close: "auto" } })))).toEqual(["close"]); }); - it("is exempt from the close-precision breaker (no closeKind: 'heuristic', mirroring blacklist/contributor-cap/review-nag)", () => { + // #9086 POLICY REVERSAL. This previously asserted that a screenshot-table close was EXEMPT from the + // close-precision breaker, on the same "deterministic ⇒ zero-hallucination" reasoning as blacklist and + // contributor_cap. That reasoning does not transfer: those two read facts about an ACCOUNT, whereas this + // gate's "determinism" is a regex heuristic over free-text markdown — and it is our single top close reason + // all-time. A deterministic rule can still be systematically wrong, which is exactly what the breaker exists + // to catch. Identity-based closes keep their exemption; content-inspection ones no longer do. + it("#9086: is subject to the close-precision breaker (content inspection, not identity)", () => { const plan = planAgentMaintenanceActions(missingTable()); const closeAction = plan.find((a) => a.actionClass === "close"); + expect(closeAction?.closeKind).toBe("screenshot_table"); const downgraded = downgradeCloseToHold(plan, true, {}); - expect(downgraded).toEqual(plan); - expect(closeAction?.closeKind).not.toBe("heuristic"); + // The close is downgraded to a hold rather than executing while the breaker is engaged. + expect(downgraded.some((a) => a.actionClass === "close")).toBe(false); + }); + + it("#9086: identity-based closes (blacklist, contributor_cap) KEEP their breaker exemption", () => { + for (const kind of ["blacklist", "contributor_cap"] as const) { + const plan: PlannedAgentAction[] = [ + { actionClass: "close", autonomyClass: "close", requiresApproval: false, reason: `${kind} close`, closeKind: kind }, + ]; + // Unchanged: a fact about the ACCOUNT, so the precision breaker has nothing to add. + expect(downgradeCloseToHold(plan, true, {})).toEqual(plan); + } }); it("is independent of the blacklist short-circuit — a matched blacklist entry still wins when both are present", () => { diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 9134c57ac9..0a86e87342 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -6254,3 +6254,44 @@ describe("formatManifestValidationNotice (#2056)", () => { expect(formatManifestValidationNotice(valid.warnings)).toBeNull(); }); }); + +// #9049 — verified live: the global default private-config layer carried `agentDryRun: false`, and because a +// plain spread lets the manifest win, that silently discarded `agent_dry_run=1` on EVERY read for EVERY repo. +// An operator flipping dry-run from the dashboard / PUT /settings / `loopover-mcp maintain` / the +// `loopover_set_agent_dry_run` MCP tool got a SUCCESS message while ORB kept making real GitHub writes. Both +// kill-switch flags are now ratchets: config-as-code may TIGHTEN safety, never loosen it. +describe("kill-switch flags are ratchets, not overridable settings (#9049)", () => { + // Built through the REAL parse path, so this also proves the flags survive parseFocusManifest. + const manifestWith = (settings: Record) => parseFocusManifest({ settings }); + + it("a manifest agentDryRun:false CANNOT clear a DB dry-run (the live fleet-wide bug)", () => { + const effective = resolveEffectiveSettings({ agentDryRun: true } as RepositorySettings, manifestWith({ agentDryRun: false })); + expect(effective.agentDryRun).toBe(true); + }); + + it("a manifest agentPaused:false CANNOT clear a DB pause", () => { + const effective = resolveEffectiveSettings({ agentPaused: true } as RepositorySettings, manifestWith({ agentPaused: false })); + expect(effective.agentPaused).toBe(true); + }); + + it("a manifest CAN still tighten: true wins even when the DB says false", () => { + const effective = resolveEffectiveSettings( + { agentPaused: false, agentDryRun: false } as RepositorySettings, + manifestWith({ agentPaused: true, agentDryRun: true }), + ); + expect(effective).toMatchObject({ agentPaused: true, agentDryRun: true }); + }); + + it("both off in both layers stays off (no accidental fleet-wide pause)", () => { + const effective = resolveEffectiveSettings( + { agentPaused: false, agentDryRun: false } as RepositorySettings, + manifestWith({}), + ); + expect(effective).toMatchObject({ agentPaused: false, agentDryRun: false }); + }); + + it("an absent manifest key leaves the DB value intact in both directions", () => { + expect(resolveEffectiveSettings({ agentDryRun: true } as RepositorySettings, manifestWith({})).agentDryRun).toBe(true); + expect(resolveEffectiveSettings({ agentDryRun: false } as RepositorySettings, manifestWith({})).agentDryRun).toBe(false); + }); +}); diff --git a/test/unit/visual-capture.test.ts b/test/unit/visual-capture.test.ts index 66edd06e21..f3fb169657 100644 --- a/test/unit/visual-capture.test.ts +++ b/test/unit/visual-capture.test.ts @@ -426,7 +426,7 @@ describe("visual capture preview discovery", () => { await buildCapture( env, "installation-token", - { repoFullName: "owner/repo", prNumber: 18, headSha: "absent-with-fallback-head", previewFromChecks: true, defaultBranchRef: "main" }, + { repoFullName: "owner/repo", prNumber: 18, headSha: "absent-with-fallback-head", previewFromChecks: true, defaultBranchRef: "main", mode: "live" as const }, ["apps/loopover-ui/src/routes/app.index.tsx"], undefined, { actionsFallback: true }, @@ -2673,7 +2673,7 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f const result = await buildCapture( createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "" }), "installation-token", - { repoFullName: "owner/repo", prNumber: 20, headSha: "cafebabe", previewFromChecks: true, defaultBranchRef: "main" }, + { repoFullName: "owner/repo", prNumber: 20, headSha: "cafebabe", previewFromChecks: true, defaultBranchRef: "main", mode: "live" as const }, ["apps/loopover-ui/src/routes/app.index.tsx"], undefined, { actionsFallback: true }, @@ -2705,7 +2705,7 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f const result = await buildCapture( env, "installation-token", - { repoFullName: "owner/repo", prNumber: 20, headSha: "cafebabecafebabecafebabecafebabecafebabe", previewFromChecks: true, defaultBranchRef: "main" }, + { repoFullName: "owner/repo", prNumber: 20, headSha: "cafebabecafebabecafebabecafebabecafebabe", previewFromChecks: true, defaultBranchRef: "main", mode: "live" as const }, ["apps/loopover-ui/src/routes/app.index.tsx"], undefined, { actionsFallback: true }, @@ -2715,6 +2715,57 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f expect(result.previewPending).toBe(true); }); + // #9067: this dispatch is a real GitHub WRITE — it starts an Actions run and burns CI minutes in the target + // repo — but it goes out via raw timeoutFetch, so it never passes makeInstallationOctokit's + // mutation-suppression hook, the structural check every other installation write relies on. runVisualCapture + // only short-circuited on `paused`, so under `dry_run` it fired for real, contradicting the documented + // dry-run contract ("suppresses the terminal GitHub-side write only"). + it("#9067: does NOT dispatch under dry_run — the workflow_dispatch is a real write and must obey the mode", async () => { + let dispatchCalled = false; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + if (input.toString().includes("/dispatches")) { + dispatchCalled = true; + return new Response(null, { status: 204 }); + } + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "", REVIEW_AUDIT: memoryReviewAudit() }); + + await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 90, headSha: "drySha0000000000000000000000000000000000", previewFromChecks: true, defaultBranchRef: "main", mode: "dry_run" as const }, + ["apps/loopover-ui/src/routes/app.index.tsx"], + undefined, + { actionsFallback: true }, + ); + + expect(dispatchCalled).toBe(false); + }); + + it("#9067: does NOT dispatch when the mode is absent — an unknown mode is treated as non-live (fail-safe)", async () => { + let dispatchCalled = false; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + if (input.toString().includes("/dispatches")) { + dispatchCalled = true; + return new Response(null, { status: 204 }); + } + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "", REVIEW_AUDIT: memoryReviewAudit() }); + + await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 91, headSha: "nomode00000000000000000000000000000000000", previewFromChecks: true, defaultBranchRef: "main" }, + ["apps/loopover-ui/src/routes/app.index.tsx"], + undefined, + { actionsFallback: true }, + ); + + expect(dispatchCalled).toBe(false); + }); + it("dispatches a NEW run when the persisted marker is for a DIFFERENT headSha (a later push)", async () => { let dispatchCalled = false; vi.stubGlobal( @@ -2733,7 +2784,7 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f const result = await buildCapture( env, "installation-token", - { repoFullName: "owner/repo", prNumber: 20, headSha: "cafebabecafebabecafebabecafebabecafebabe", previewFromChecks: true, defaultBranchRef: "main" }, + { repoFullName: "owner/repo", prNumber: 20, headSha: "cafebabecafebabecafebabecafebabecafebabe", previewFromChecks: true, defaultBranchRef: "main", mode: "live" as const }, ["apps/loopover-ui/src/routes/app.index.tsx"], undefined, { actionsFallback: true }, @@ -2752,7 +2803,7 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f const result = await buildCapture( createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "" }), "installation-token", - { repoFullName: "owner/repo", prNumber: 21, headSha: "cafebabe", previewFromChecks: true, defaultBranchRef: "main" }, + { repoFullName: "owner/repo", prNumber: 21, headSha: "cafebabe", previewFromChecks: true, defaultBranchRef: "main", mode: "live" as const }, ["apps/loopover-ui/src/routes/app.index.tsx"], undefined, { actionsFallback: true }, @@ -2774,7 +2825,7 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f const result = await buildCapture( createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "" }), "installation-token", - { repoFullName: "owner/repo", prNumber: 22, headSha: "cafebabe", previewFromChecks: true, defaultBranchRef: "main" }, + { repoFullName: "owner/repo", prNumber: 22, headSha: "cafebabe", previewFromChecks: true, defaultBranchRef: "main", mode: "live" as const }, ["apps/loopover-ui/src/routes/app.index.tsx"], ); @@ -2797,7 +2848,7 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f await buildCapture( createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "" }), "installation-token", - { repoFullName: "owner/repo", prNumber: 23, previewFromChecks: true, defaultBranchRef: "main" }, + { repoFullName: "owner/repo", prNumber: 23, previewFromChecks: true, defaultBranchRef: "main", mode: "live" as const }, ["apps/loopover-ui/src/routes/app.index.tsx"], undefined, { actionsFallback: true }, @@ -2841,7 +2892,7 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f const result = await buildCapture( createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "" }), "installation-token", - { repoFullName: "owner/repo", prNumber: 25, headSha: "cafebabe", previewFailed: true, defaultBranchRef: "main" }, + { repoFullName: "owner/repo", prNumber: 25, headSha: "cafebabe", previewFailed: true, defaultBranchRef: "main", mode: "live" as const }, ["apps/loopover-ui/src/routes/app.index.tsx"], undefined, { actionsFallback: true }, @@ -2868,7 +2919,7 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f const result = await buildCapture( createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "" }), "installation-token", - { repoFullName: "owner/repo", prNumber: 26, headSha: "cafebabe", previewFromChecks: true, defaultBranchRef: "main" }, + { repoFullName: "owner/repo", prNumber: 26, headSha: "cafebabe", previewFromChecks: true, defaultBranchRef: "main", mode: "live" as const }, ["apps/loopover-ui/src/routes/app.index.tsx"], undefined, { actionsFallback: true }, @@ -2891,7 +2942,7 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f const result = await buildCapture( env, "installation-token", - { repoFullName: "owner/repo", prNumber: 27, headSha: "cafebabe", previewFromChecks: true, defaultBranchRef: "main" }, + { repoFullName: "owner/repo", prNumber: 27, headSha: "cafebabe", previewFromChecks: true, defaultBranchRef: "main", mode: "live" as const }, ["apps/loopover-ui/src/routes/app.index.tsx"], undefined, { actionsFallback: true }, @@ -2914,7 +2965,7 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f const result = await buildCapture( env, "installation-token", - { repoFullName: "owner/repo", prNumber: 29, headSha: "cafebabe", previewFromChecks: true, defaultBranchRef: "main" }, + { repoFullName: "owner/repo", prNumber: 29, headSha: "cafebabe", previewFromChecks: true, defaultBranchRef: "main", mode: "live" as const }, ["apps/loopover-ui/src/routes/app.index.tsx"], undefined, { actionsFallback: true }, @@ -2936,7 +2987,7 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f const result = await buildCapture( env, "installation-token", - { repoFullName: "owner/repo", prNumber: 31, headSha: "cafebabe", previewFromChecks: true, defaultBranchRef: "main" }, + { repoFullName: "owner/repo", prNumber: 31, headSha: "cafebabe", previewFromChecks: true, defaultBranchRef: "main", mode: "live" as const }, ["apps/loopover-ui/src/routes/app.index.tsx"], undefined, { actionsFallback: true }, @@ -2956,7 +3007,7 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f const result = await buildCapture( env, "installation-token", - { repoFullName: "owner/repo", prNumber: 28, headSha: "cafebabe", previewFromChecks: true, defaultBranchRef: "main" }, + { repoFullName: "owner/repo", prNumber: 28, headSha: "cafebabe", previewFromChecks: true, defaultBranchRef: "main", mode: "live" as const }, ["apps/loopover-ui/src/routes/app.index.tsx"], undefined, { actionsFallback: true }, @@ -2976,7 +3027,7 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f const result = await buildCapture( env, "installation-token", - { repoFullName: "owner/repo", prNumber: 30, headSha: "cafebabe", previewFromChecks: true, defaultBranchRef: "main" }, + { repoFullName: "owner/repo", prNumber: 30, headSha: "cafebabe", previewFromChecks: true, defaultBranchRef: "main", mode: "live" as const }, ["apps/loopover-ui/src/routes/app.index.tsx"], undefined, { actionsFallback: true }, @@ -2991,7 +3042,7 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f const result = await buildCapture( createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "https://prod.example.com" }), "installation-token", - { repoFullName: "owner/repo", prNumber: 29, headSha: "cafebabe", previewFromChecks: true, defaultBranchRef: "main" }, + { repoFullName: "owner/repo", prNumber: 29, headSha: "cafebabe", previewFromChecks: true, defaultBranchRef: "main", mode: "live" as const }, ["apps/loopover-ui/src/routes/app.index.tsx"], undefined, { actionsFallback: true }, From 8ac6a9fcd611e6a2e8de9fece8c9c9ec395ac207 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 16:57:45 -0700 Subject: [PATCH 2/2] test: update the ingest + dispatcher suites for the new fail-closed contracts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught what my local runs missed: I had only been running test/unit, so the test/integration suites that exercised the OLD fail-open ingest default were never exercised locally. Fixed properly rather than by weakening the change: - test/integration/{orb,ams}-ingest.test.ts: every request-shape test (200/400/413, and the dropped-connection 413 regression) now presents a real credential, because those assertions are about BODY handling and would otherwise stop at the new 401. The two "optional collector token" tests are rewritten to assert the actual new contract -- unset ⇒ 401, not 200. - test/integration/orb-oauth.test.ts: the path-specificity test proves a request routes PAST the callback checks into the ingest handler's body handling, so it needs a credential to still reach that point. - test/unit/actions-fallback.test.ts: the two direct dispatcher tests pass mode: "live", plus a new test pinning the refusal for dry_run, paused, and an absent mode. Also verified the full local suite (unit + integration, 22,146 passing) after building every artifact test:ci builds first -- the 327 failures I initially saw were entirely missing MCP/miner/discovery-index/ui-kit bundles in a fresh worktree, not this diff. --- test/integration/ams-ingest.test.ts | 20 +++++++++------ test/integration/orb-ingest.test.ts | 38 +++++++++++++++++++---------- test/integration/orb-oauth.test.ts | 8 +++++- test/unit/actions-fallback.test.ts | 20 +++++++++++++++ 4 files changed, 65 insertions(+), 21 deletions(-) diff --git a/test/integration/ams-ingest.test.ts b/test/integration/ams-ingest.test.ts index 0d114057c5..e0ccbaf6bb 100644 --- a/test/integration/ams-ingest.test.ts +++ b/test/integration/ams-ingest.test.ts @@ -124,39 +124,45 @@ describe("handleAmsIngest()", () => { describe("POST /v1/ams/ingest route", () => { const app = createApp(); + // #9046: the collector fails CLOSED on an unset token, so every request-shape test below must present a + // valid credential — otherwise it asserts 200/400/413 against an endpoint that now correctly answers 401. + const AUTH = { authorization: "Bearer fleet-secret" }; + const authedEnv = (extra: Partial = {}) => createTestEnv({ AMS_INGEST_TOKEN: "fleet-secret", ...extra }); it("returns 200 + accepted count for a valid batch", async () => { - const env = createTestEnv(); + const env = authedEnv(); const body = JSON.stringify({ instanceId: "abc0", events: [{ repoHash: "rhash", prHash: "phash", decision: "merged" }] }); - const res = await app.request("/v1/ams/ingest", { method: "POST", headers: { "content-type": "application/json" }, body }, env); + const res = await app.request("/v1/ams/ingest", { method: "POST", headers: { "content-type": "application/json", ...AUTH }, body }, env); expect(res.status).toBe(200); expect(((await res.json()) as { accepted: number }).accepted).toBe(1); }); it("returns 400 for invalid JSON", async () => { - const res = await app.request("/v1/ams/ingest", { method: "POST", headers: { "content-type": "application/json" }, body: "{bad" }, createTestEnv()); + const res = await app.request("/v1/ams/ingest", { method: "POST", headers: { "content-type": "application/json", ...AUTH }, body: "{bad" }, authedEnv()); expect(res.status).toBe(400); expect(((await res.json()) as { error: string }).error).toBe("invalid_json"); }); it("returns 400 for an empty body", async () => { - const res = await app.request("/v1/ams/ingest", { method: "POST", body: "" }, createTestEnv()); + const res = await app.request("/v1/ams/ingest", { method: "POST", headers: AUTH, body: "" }, authedEnv()); expect(res.status).toBe(400); }); it("returns 413 when the body exceeds the shared ingest byte ceiling", async () => { const huge = "x".repeat(MAX_ORB_INGEST_BODY_BYTES + 16); - const res = await app.request("/v1/ams/ingest", { method: "POST", body: huge }, createTestEnv()); + const res = await app.request("/v1/ams/ingest", { method: "POST", headers: AUTH, body: huge }, authedEnv()); expect(res.status).toBe(413); expect(((await res.json()) as { error: string }).error).toBe("payload_too_large"); }); - it("optional collector token: open when unset; enforced once AMS_INGEST_TOKEN is set", async () => { + // #9046: this used to assert the collector was OPEN when the token was unset — the shipped default — which + // let anyone with network access POST batches. It now fails closed. + it("collector token: FAILS CLOSED when unset; enforced exactly once AMS_INGEST_TOKEN is set", async () => { const body = JSON.stringify({ instanceId: "abc0", events: [{ repoHash: "rhash", prHash: "phash", decision: "merged" }] }); const post = (env: Env, authorization?: string) => app.request("/v1/ams/ingest", { method: "POST", headers: { "content-type": "application/json", ...(authorization ? { authorization } : {}) }, body }, env); - expect((await post(createTestEnv())).status).toBe(200); + expect((await post(createTestEnv())).status).toBe(401); // unset ⇒ closed, not open const env = createTestEnv({ AMS_INGEST_TOKEN: "fleet-secret" }); expect((await post(env)).status).toBe(401); expect((await post(env, "Bearer wrong")).status).toBe(401); diff --git a/test/integration/orb-ingest.test.ts b/test/integration/orb-ingest.test.ts index d3ea369c37..51ca7ed449 100644 --- a/test/integration/orb-ingest.test.ts +++ b/test/integration/orb-ingest.test.ts @@ -304,29 +304,33 @@ describe("readOrbIngestBody()", () => { describe("POST /v1/orb/ingest route", () => { const app = createApp(); + // #9046: the collector fails CLOSED on an unset token, so every request-shape test below must present a + // valid credential — otherwise it asserts 200/400/413 against an endpoint that now correctly answers 401. + const AUTH = { authorization: "Bearer fleet-secret" }; + const authedEnv = (extra: Partial = {}) => createTestEnv({ ORB_INGEST_TOKEN: "fleet-secret", ...extra }); it("returns 200 + accepted count for a valid batch", async () => { - const env = createTestEnv(); + const env = authedEnv(); const body = JSON.stringify({ instance_id: "abc0", events: [{ repo_hash: "rhash", pr_hash: "phash", outcome: "merged", reversal_flag: "none" }] }); - const res = await app.request("/v1/orb/ingest", { method: "POST", headers: { "content-type": "application/json" }, body }, env); + const res = await app.request("/v1/orb/ingest", { method: "POST", headers: { "content-type": "application/json", ...AUTH }, body }, env); expect(res.status).toBe(200); expect(((await res.json()) as { accepted: number }).accepted).toBe(1); }); it("returns 400 for invalid JSON", async () => { - const res = await app.request("/v1/orb/ingest", { method: "POST", headers: { "content-type": "application/json" }, body: "{bad" }, createTestEnv()); + const res = await app.request("/v1/orb/ingest", { method: "POST", headers: { "content-type": "application/json", ...AUTH }, body: "{bad" }, authedEnv()); expect(res.status).toBe(400); expect(((await res.json()) as { error: string }).error).toBe("invalid_json"); }); it("returns 400 for an empty body", async () => { - const res = await app.request("/v1/orb/ingest", { method: "POST", body: "" }, createTestEnv()); + const res = await app.request("/v1/orb/ingest", { method: "POST", headers: AUTH, body: "" }, authedEnv()); expect(res.status).toBe(400); }); it("returns 413 when the body exceeds the ingest byte ceiling", async () => { const huge = "x".repeat(MAX_ORB_INGEST_BODY_BYTES + 16); - const res = await app.request("/v1/orb/ingest", { method: "POST", body: huge }, createTestEnv()); + const res = await app.request("/v1/orb/ingest", { method: "POST", headers: AUTH, body: huge }, authedEnv()); expect(res.status).toBe(413); expect(((await res.json()) as { error: string }).error).toBe("payload_too_large"); }); @@ -340,20 +344,22 @@ describe("POST /v1/orb/ingest route", () => { }); const res = await app.request( "/v1/orb/ingest", - { method: "POST", body: stream, ...({ duplex: "half" } as object) }, - createTestEnv(), + { method: "POST", headers: AUTH, body: stream, ...({ duplex: "half" } as object) }, + authedEnv(), ); expect(res.status).toBe(413); expect(((await res.json()) as { error: string }).error).toBe("payload_too_large"); }); - it("optional collector token (#1285): open when unset; enforced once ORB_INGEST_TOKEN is set", async () => { + // #9046: this used to assert the collector was OPEN when the token was unset — the shipped default — so + // anyone with network access could POST batches that feed the PUBLISHED accuracy numbers. Now fails closed. + it("collector token (#1285/#9046): FAILS CLOSED when unset; enforced exactly once ORB_INGEST_TOKEN is set", async () => { const body = JSON.stringify({ instance_id: "abc0", events: [{ repo_hash: "rhash", pr_hash: "phash", outcome: "merged" }] }); const post = (env: Env, authorization?: string) => app.request("/v1/orb/ingest", { method: "POST", headers: { "content-type": "application/json", ...(authorization ? { authorization } : {}) }, body }, env); - // Token UNSET → open ingress (the live fleet keeps working with no auth header). - expect((await post(createTestEnv())).status).toBe(200); + // Token UNSET → CLOSED. An unconfigured collector rejects rather than accepting anonymous writes. + expect((await post(createTestEnv())).status).toBe(401); // Token SET → a missing or wrong bearer is rejected before the body is parsed. const env = createTestEnv({ ORB_INGEST_TOKEN: "fleet-secret" }); expect((await post(env)).status).toBe(401); @@ -366,11 +372,17 @@ describe("POST /v1/orb/ingest route", () => { describe("Orb instance registry routes (/v1/internal/orb/instances)", () => { const app = createApp(); const auth = { authorization: "Bearer dev-internal-token" }; + // #9046: ingest now requires a credential, so any env used with ingestOne must carry ORB_INGEST_TOKEN. + const ingestEnv = (extra: Partial = {}) => createTestEnv({ ORB_INGEST_TOKEN: "fleet-secret", ...extra }); const ingestOne = (env: Env, instance: string) => - app.request("/v1/orb/ingest", { method: "POST", body: JSON.stringify({ instance_id: instance, events: [{ repo_hash: "r", pr_hash: `${instance}-p`, outcome: "merged" }] }) }, env); + app.request( + "/v1/orb/ingest", + { method: "POST", headers: { authorization: "Bearer fleet-secret" }, body: JSON.stringify({ instance_id: instance, events: [{ repo_hash: "r", pr_hash: `${instance}-p`, outcome: "merged" }] }) }, + env, + ); it("lists ingested instances as unregistered with their stored-signal count", async () => { - const env = createTestEnv(); + const env = ingestEnv(); await ingestOne(env, "inst-a"); const res = await app.request("/v1/internal/orb/instances", { headers: auth }, env); expect(res.status).toBe(200); @@ -383,7 +395,7 @@ describe("Orb instance registry routes (/v1/internal/orb/instances)", () => { }); it("registers an instance (and can unregister it)", async () => { - const env = createTestEnv(); + const env = ingestEnv(); await ingestOne(env, "inst-b"); const reg = await app.request("/v1/internal/orb/instances/register", { method: "POST", headers: auth, body: JSON.stringify({ instanceId: "inst-b" }) }, env); expect(((await reg.json()) as { registered: boolean }).registered).toBe(true); diff --git a/test/integration/orb-oauth.test.ts b/test/integration/orb-oauth.test.ts index b620687110..1c1650c9e3 100644 --- a/test/integration/orb-oauth.test.ts +++ b/test/integration/orb-oauth.test.ts @@ -32,7 +32,13 @@ describe("GET /v1/orb/oauth/callback (post-install landing)", () => { it("the new exemption + rate class are path-specific (a later orb path still routes)", async () => { // /v1/orb/ingest falls through PAST the new callback checks, exercising their FALSE side in both // requiresApiToken + routeClassForPath (the webhook path short-circuits earlier and wouldn't reach them). - const res = await app.request("/v1/orb/ingest", { method: "POST" }, createTestEnv()); + // #9046: ingest now fails closed on an unset token, so present one — otherwise this would stop at 401 and + // no longer demonstrate that the request reaches the ingest handler's own BODY handling. + const res = await app.request( + "/v1/orb/ingest", + { method: "POST", headers: { authorization: "Bearer fleet-secret" } }, + createTestEnv({ ORB_INGEST_TOKEN: "fleet-secret" }), + ); expect([400, 413]).toContain(res.status); // reached the (exempt) ingest handler, failed only on the empty body }); diff --git a/test/unit/actions-fallback.test.ts b/test/unit/actions-fallback.test.ts index c771a15fe1..c9cb78bb52 100644 --- a/test/unit/actions-fallback.test.ts +++ b/test/unit/actions-fallback.test.ts @@ -344,6 +344,7 @@ describe("dispatchVisualCaptureFallback", () => { prNumber: 7, headSha: "deadbeef", routes: ["/", "/pricing"], + mode: "live", }); expect(ok).toBe(true); expect(capturedUrl).toBe("https://api.github.com/repos/acme/widgets/actions/workflows/visual-capture-fallback.yml/dispatches"); @@ -353,6 +354,24 @@ describe("dispatchVisualCaptureFallback", () => { }); }); + // #9067: this dispatch is a real GitHub write (it starts an Actions run and burns CI minutes) issued via raw + // timeoutFetch, so it never passes makeInstallationOctokit's mutation-suppression hook. Refusing in the + // writer itself — with an absent mode treated as non-live — keeps the guarantee structural rather than + // dependent on every caller remembering a check. + it("#9067: refuses to dispatch under any non-live mode, and when the mode is absent", async () => { + let called = false; + vi.stubGlobal("fetch", async () => { + called = true; + return new Response(null, { status: 204 }); + }); + const base = { token: "tok", repo: { owner: "acme", repo: "widgets" }, ref: "main", prNumber: 7, headSha: "deadbeef", routes: ["/"] }; + for (const mode of ["dry_run", "paused"] as const) { + expect(await dispatchVisualCaptureFallback({ ...base, mode })).toBe(false); + } + expect(await dispatchVisualCaptureFallback(base)).toBe(false); // absent ⇒ non-live (fail-safe) + expect(called).toBe(false); + }); + it("dispatches successfully when a rateLimitAdmissionKey is supplied", async () => { vi.stubGlobal("fetch", async () => new Response(null, { status: 204 })); const ok = await dispatchVisualCaptureFallback({ @@ -363,6 +382,7 @@ describe("dispatchVisualCaptureFallback", () => { headSha: "deadbeef", routes: ["/"], rateLimitAdmissionKey: "installation:1", + mode: "live", }); expect(ok).toBe(true); });