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/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); }); 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 },