From 6d8c0435d319e63e4aec0941194d608b6ea4f5fc Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 11 Jul 2026 03:50:17 -0700 Subject: [PATCH] fix(review): require a real installation before acting-autonomy scans include a repo (#5023) isAgentConfigured resolves the operator's global-default autonomy for ANY repoFullName, regardless of whether the GitHub App is installed there. A repo with only a local `repositories` row (a stray subnet-registry entry, say) inherited that global default and looked "agent-configured" purely by existing, even with no installation token to act on it. Require a real installationId before the autonomy-based path counts, across every site that mirrors the regate sweep's selection: the regate sweep itself, PR reconciliation, the sweep watchdog, ops-alerts, selftune, and the maintainer recap. The explicit GITTENSORY_REVIEW_REPOS allowlist path is untouched -- it's a deliberate, operator-typed signal independent of installation state. --- src/queue/processors.ts | 12 +++++++- src/review/maintainer-recap-wire.ts | 4 +++ src/review/ops-wire.ts | 4 +++ src/review/pr-reconciliation.ts | 7 ++++- src/review/selftune-wire.ts | 4 +++ src/review/sweep-watchdog.ts | 6 +++- test/unit/maintainer-recap-wire.test.ts | 11 ++++--- test/unit/ops-wire.test.ts | 28 +++++++++++++++++ test/unit/queue.test.ts | 41 +++++++++++++++++++------ test/unit/selftune-wiring.test.ts | 24 +++++++++++++-- 10 files changed, 123 insertions(+), 18 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 3e58adf856..6afb265eb0 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -878,10 +878,20 @@ export async function fanOutAgentRegateSweepJobs( // outcome for just this repo (it gets picked up again next tick) instead of rejecting. try { const settings = await resolveRepositorySettings(env, repoFullName); + // #sweep-requires-installation: isAgentConfigured resolves the OPERATOR'S global-default autonomy + // (e.g. a self-host `.gittensory.yml` settings.autonomy block meant for the repos this instance + // actually operates on) for ANY repoFullName, regardless of whether the GitHub App is installed + // there. A repo that merely has a local `repositories` row (a stray subnet-registry row, say) with + // no real `installationId` would otherwise inherit that global default and look "agent-configured" + // purely by existing — even though no installation token exists to act on it, and it was never + // intentionally onboarded. Require a real installation before the autonomy-based path can make a + // repo eligible; the explicit allowlist path is untouched (GITTENSORY_REVIEW_REPOS is a deliberate, + // operator-typed signal independent of installation state, e.g. reviewing ahead of a pending install). + const hasInstallation = typeof repo.installationId === "number"; if ( !( isConvergenceRepoAllowed(env, repoFullName) || - isAgentConfigured(settings.autonomy) + (hasInstallation && isAgentConfigured(settings.autonomy)) ) ) return { kind: "ineligible" }; diff --git a/src/review/maintainer-recap-wire.ts b/src/review/maintainer-recap-wire.ts index 08f61e9387..875a1f77c9 100644 --- a/src/review/maintainer-recap-wire.ts +++ b/src/review/maintainer-recap-wire.ts @@ -103,6 +103,10 @@ async function recapScanRepos(env: Env): Promise { const configured: string[] = []; for (const repo of repos) { try { + // #sweep-requires-installation: a repo with no real GitHub App installation must never be treated as + // agent-configured purely because it resolves the operator's global-default autonomy by merely having + // a local row -- mirrors fanOutAgentRegateSweepJobs's own guard. + if (typeof repo.installationId !== "number") continue; const settings = await resolveRepositorySettings(env, repo.fullName); if (isAgentConfigured(settings.autonomy)) configured.push(repo.fullName); } catch { diff --git a/src/review/ops-wire.ts b/src/review/ops-wire.ts index e5c75ef864..0fe367341a 100644 --- a/src/review/ops-wire.ts +++ b/src/review/ops-wire.ts @@ -155,6 +155,10 @@ async function opsScanRepos(env: Env): Promise { const configured: string[] = []; for (const repo of repos) { try { + // #sweep-requires-installation: a repo with no real GitHub App installation must never be treated as + // agent-configured purely because it resolves the operator's global-default autonomy by merely having + // a local row -- mirrors fanOutAgentRegateSweepJobs's own guard. + if (typeof repo.installationId !== "number") continue; const settings = await resolveRepositorySettings(env, repo.fullName); if (isAgentConfigured(settings.autonomy)) configured.push(repo.fullName); } catch { diff --git a/src/review/pr-reconciliation.ts b/src/review/pr-reconciliation.ts index da9cced3f7..d5a9c6b7e1 100644 --- a/src/review/pr-reconciliation.ts +++ b/src/review/pr-reconciliation.ts @@ -44,7 +44,12 @@ async function watchedRepos(env: Env): Promise { const configured: string[] = []; for (const repo of repos) { try { + // #sweep-requires-installation: a repo with no real GitHub App installation must never be treated as + // agent-configured purely because it resolves the operator's global-default autonomy by merely having + // a local row -- mirrors fanOutAgentRegateSweepJobs's own guard. + if (typeof repo.installationId !== "number") continue; const settings = await resolveRepositorySettings(env, repo.fullName); if (!isAgentConfigured(settings.autonomy)) continue; const manifest = await loadRepoFocusManifest(env, repo.fullName).catch(() => null); diff --git a/src/review/sweep-watchdog.ts b/src/review/sweep-watchdog.ts index 46ba651ebd..e3681a2501 100644 --- a/src/review/sweep-watchdog.ts +++ b/src/review/sweep-watchdog.ts @@ -57,7 +57,11 @@ async function watchedRepos(env: Env): Promise { +async function seedRegisteredRepo(env: Env, fullName: string, installationId?: number): Promise { const [owner, name] = fullName.split("/"); + // installationId is only needed by tests that rely on the acting-autonomy selection path + // (#sweep-requires-installation): a repo with no real installation is never treated as agent-configured, + // regardless of its resolved autonomy settings. await (env.DB as unknown as { prepare: (s: string) => { bind: (...v: unknown[]) => { run: () => Promise } } }) - .prepare("INSERT INTO repositories (full_name, owner, name, is_installed, is_registered) VALUES (?, ?, ?, 1, 1)") - .bind(fullName, owner, name) + .prepare("INSERT INTO repositories (full_name, owner, name, is_installed, is_registered, installation_id) VALUES (?, ?, ?, 1, 1, ?)") + .bind(fullName, owner, name, installationId ?? null) .run(); } @@ -231,7 +234,7 @@ describe("runMaintainerRecapJob — cross-repo digest (#1963, #2248)", () => { it("prefers agent-configured repos over the full registered set when at least one is configured", async () => { const env = createTestEnv({ DISCORD_WEBHOOK_URL: HOOK }); - await seedRegisteredRepo(env, "owner/configured"); + await seedRegisteredRepo(env, "owner/configured", 9402); await seedMergedPr(env, "owner/configured", 1); await upsertRepositorySettings(env, { repoFullName: "owner/configured", autonomy: { merge: "auto" } }); await seedRegisteredRepo(env, "owner/unconfigured"); diff --git a/test/unit/ops-wire.test.ts b/test/unit/ops-wire.test.ts index 3714e38bda..36952aa473 100644 --- a/test/unit/ops-wire.test.ts +++ b/test/unit/ops-wire.test.ts @@ -173,6 +173,19 @@ async function seedRegisteredRepo(env: Env, fullName: string): Promise { .run(); } +// A registered repo with a REAL installation + acting-autonomy settings, so opsScanRepos's "prefer +// agent-configured repos" branch picks it up (#sweep-requires-installation: installation_id is required +// alongside the autonomy row, matching the real upsertRepositoryFromGitHub invariant). +async function seedAgentConfiguredRepo(env: Env, fullName: string, installationId: number): Promise { + const [owner, name] = fullName.split("/"); + await env.DB.prepare("INSERT INTO repositories (full_name, owner, name, is_installed, is_registered, installation_id) VALUES (?, ?, ?, 1, 1, ?)") + .bind(fullName, owner, name, installationId) + .run(); + await env.DB.prepare("INSERT INTO repository_settings (repo_full_name, autonomy_json) VALUES (?, ?)") + .bind(fullName, JSON.stringify({ review: "auto" })) + .run(); +} + // Seed a gate-block ledger anomaly: blocked PRs that later MERGED (false positives) over the min sample. async function seedGateFalsePositiveAnomaly(env: Env, repoFullName: string): Promise { for (let i = 1; i <= 6; i += 1) { @@ -228,6 +241,21 @@ describe("runOpsAlerts — cron path over gittensory's outcome data", () => { expect(errors.mock.calls.map((c) => String(c[0])).some((line) => line.includes("ops_anomaly\""))).toBe(false); }); + it("REGRESSION (#sweep-requires-installation): prefers the agent-configured repo and never scans an uninstalled registered repo when a configured one exists", async () => { + const env = createTestEnv(); + await seedAgentConfiguredRepo(env, "owner/configured", 9501); + await seedGateFalsePositiveAnomaly(env, "owner/configured"); + // Registered, but no real installation -- must not count as "agent-configured" merely by resolving the + // operator's global-default autonomy, and must be excluded from the scan once a configured repo exists. + await seedRegisteredRepo(env, "owner/no-install"); + await seedGateFalsePositiveAnomaly(env, "owner/no-install"); + + const found = await runOpsAlerts(env); + + expect(found["owner/configured"]?.some((a) => /gate false-positive spike/.test(a))).toBe(true); + expect(found["owner/no-install"]).toBeUndefined(); + }); + it("detects and reports a review burst end-to-end (a PR published far more review surfaces than normal in the window)", async () => { setSelfHostedMetricsMode(true); // keep the repo label so the counter assertion can target the exact series const env = createTestEnv(); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index b53b71786f..ca43d24b73 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -888,9 +888,11 @@ describe("queue processors", () => { }, } as unknown as Queue, }); - await upsertRepositoryFromGitHub(env, { name: "agent-a", full_name: "owner/agent-a", private: false, owner: { login: "owner" } }); - await upsertRepositoryFromGitHub(env, { name: "agent-b", full_name: "owner/agent-b", private: false, owner: { login: "owner" } }); - await upsertRepositoryFromGitHub(env, { name: "plain-repo", full_name: "owner/plain-repo", private: false, owner: { login: "owner" } }); + // #sweep-requires-installation: acting-autonomy eligibility also requires a real installation now — give + // the repos that SHOULD be swept a real installationId, mirroring an actually-installed repo. + await upsertRepositoryFromGitHub(env, { name: "agent-a", full_name: "owner/agent-a", private: false, owner: { login: "owner" } }, 9201); + await upsertRepositoryFromGitHub(env, { name: "agent-b", full_name: "owner/agent-b", private: false, owner: { login: "owner" } }, 9202); + await upsertRepositoryFromGitHub(env, { name: "plain-repo", full_name: "owner/plain-repo", private: false, owner: { login: "owner" } }, 9203); await upsertRepositorySettings(env, { repoFullName: "owner/agent-a", autonomy: { label: "auto" } }); await upsertRepositorySettings(env, { repoFullName: "owner/agent-b", autonomy: { merge: "auto_with_approval" } }); await upsertRepositorySettings(env, { repoFullName: "owner/plain-repo", autonomy: { review: "observe" } }); // non-acting → not configured @@ -908,6 +910,27 @@ describe("queue processors", () => { expect(JSON.parse(fanout?.metadata_json ?? "{}")).toMatchObject({ repoCount: 2, requestedBy: "schedule" }); }); + it("REGRESSION (#sweep-requires-installation): an acting-autonomy repo with NO real installation is excluded, even though it resolves the operator's global-default autonomy", async () => { + const sent: import("../../src/types").JobMessage[] = []; + const env = createTestEnv({ + GITTENSORY_REVIEW_REPOS: "", + JOBS: { async send(m: import("../../src/types").JobMessage) { sent.push(m); } } as unknown as Queue, + }); + // A repo that merely EXISTS locally (e.g. a stray row from an unrelated sync) with a real installation and + // acting autonomy — sweep-eligible. + await upsertRepositoryFromGitHub(env, { name: "installed-repo", full_name: "owner/installed-repo", private: false, owner: { login: "owner" } }, 9301); + await upsertRepositorySettings(env, { repoFullName: "owner/installed-repo", autonomy: { merge: "auto" } }); + // Same acting autonomy, but NO installationId (never upserted with one) — this is exactly the shape a + // stray repositories row takes: it can still resolve a permissive global-default autonomy policy, but + // there is no GitHub App installation on it at all, so it must never be treated as agent-configured. + await upsertRepositoryFromGitHub(env, { name: "uninstalled-repo", full_name: "owner/uninstalled-repo", private: false, owner: { login: "owner" } }); + await upsertRepositorySettings(env, { repoFullName: "owner/uninstalled-repo", autonomy: { merge: "auto" } }); + + await processJob(env, { type: "agent-regate-sweep", requestedBy: "schedule" }); + + expect(sent.map((m) => (m.type === "agent-regate-sweep" ? m.repoFullName : null))).toEqual(["owner/installed-repo"]); + }); + it("agent re-gate sweep ALSO fans out to allowlisted repos regardless of autonomy mode (#sweep-all-modes)", async () => { const sent: import("../../src/types").JobMessage[] = []; const env = createTestEnv({ GITTENSORY_REVIEW_REPOS: "owner/advisory-repo", JOBS: { async send(m: import("../../src/types").JobMessage) { sent.push(m); } } as unknown as Queue }); @@ -931,8 +954,8 @@ describe("queue processors", () => { GITTENSORY_REVIEW_REPOS: "", JOBS: { async send(m: import("../../src/types").JobMessage) { sent.push(m); } } as unknown as Queue, }); - await upsertRepositoryFromGitHub(env, { name: "agent-a", full_name: "owner/agent-a", private: false, owner: { login: "owner" } }); - await upsertRepositoryFromGitHub(env, { name: "agent-b", full_name: "owner/agent-b", private: false, owner: { login: "owner" } }); + await upsertRepositoryFromGitHub(env, { name: "agent-a", full_name: "owner/agent-a", private: false, owner: { login: "owner" } }, 9204); + await upsertRepositoryFromGitHub(env, { name: "agent-b", full_name: "owner/agent-b", private: false, owner: { login: "owner" } }, 9205); await upsertRepositorySettings(env, { repoFullName: "owner/agent-a", autonomy: { label: "auto" } }); await upsertRepositorySettings(env, { repoFullName: "owner/agent-b", autonomy: { label: "auto" } }); const realResolve = repositorySettingsModule.resolveRepositorySettings; @@ -964,8 +987,8 @@ describe("queue processors", () => { }, } as unknown as Queue, }); - await upsertRepositoryFromGitHub(env, { name: "agent-a", full_name: "owner/agent-a", private: false, owner: { login: "owner" } }); - await upsertRepositoryFromGitHub(env, { name: "agent-b", full_name: "owner/agent-b", private: false, owner: { login: "owner" } }); + await upsertRepositoryFromGitHub(env, { name: "agent-a", full_name: "owner/agent-a", private: false, owner: { login: "owner" } }, 9206); + await upsertRepositoryFromGitHub(env, { name: "agent-b", full_name: "owner/agent-b", private: false, owner: { login: "owner" } }, 9207); await upsertRepositorySettings(env, { repoFullName: "owner/agent-a", autonomy: { label: "auto" } }); await upsertRepositorySettings(env, { repoFullName: "owner/agent-b", autonomy: { label: "auto" } }); const errors = vi.spyOn(console, "error").mockImplementation(() => undefined); @@ -988,8 +1011,8 @@ describe("queue processors", () => { JOBS: { async send(m: import("../../src/types").JobMessage) { sent.push(m); } } as unknown as Queue, }); const repoNames = ["r1", "r2", "r3", "r4", "r5", "r6"]; - for (const name of repoNames) { - await upsertRepositoryFromGitHub(env, { name, full_name: `owner/${name}`, private: false, owner: { login: "owner" } }); + for (const [index, name] of repoNames.entries()) { + await upsertRepositoryFromGitHub(env, { name, full_name: `owner/${name}`, private: false, owner: { login: "owner" } }, 9300 + index); await upsertRepositorySettings(env, { repoFullName: `owner/${name}`, autonomy: { label: "auto" } }); } const { mapWithConcurrencyLimit: realMapWithConcurrencyLimit } = diff --git a/test/unit/selftune-wiring.test.ts b/test/unit/selftune-wiring.test.ts index 6db17c62d6..1b0c148823 100644 --- a/test/unit/selftune-wiring.test.ts +++ b/test/unit/selftune-wiring.test.ts @@ -30,8 +30,11 @@ function poisonDbPrepare(env: Env, pattern: RegExp): void { async function seedRegisteredRepo(env: Env, fullName: string, autonomyJson: string): Promise { const [owner, name] = fullName.split("/"); - await env.DB.prepare("INSERT INTO repositories (full_name, owner, name, is_installed, is_registered) VALUES (?, ?, ?, 1, 1)") - .bind(fullName, owner, name) + // installation_id is required alongside is_installed=1 (#sweep-requires-installation): selfTuneRepos now + // requires a real installation before isAgentConfigured counts, matching the real upsertRepositoryFromGitHub + // invariant (isInstalled is only ever true alongside a real installationId). + await env.DB.prepare("INSERT INTO repositories (full_name, owner, name, is_installed, is_registered, installation_id) VALUES (?, ?, ?, 1, 1, ?)") + .bind(fullName, owner, name, 9401) .run(); // Opt the repo into the acting-autonomy surface so isAgentConfigured(settings.autonomy) is true (selfTuneRepos filter). await env.DB.prepare("INSERT INTO repository_settings (repo_full_name, autonomy_json) VALUES (?, ?)") @@ -273,6 +276,23 @@ describe("selfTuneRepos — per-repo review.selftune FORCE-OFF (#4104)", () => { expect((await listOverrideAudit(env as never, "owner/opted-out")).length).toBe(0); }); + it("REGRESSION (#sweep-requires-installation): an acting-autonomy repo with NO real installation is excluded from the tuning pass, even though it resolves the operator's global-default autonomy", async () => { + const owner = "owner"; + const name = "no-install"; + const env = createTestEnv({ GITTENSORY_REVIEW_SELFTUNE: "true" }); + await env.DB.prepare("INSERT INTO repositories (full_name, owner, name, is_installed, is_registered) VALUES (?, ?, ?, 0, 1)") + .bind(`${owner}/${name}`, owner, name) + .run(); + await env.DB.prepare("INSERT INTO repository_settings (repo_full_name, autonomy_json) VALUES (?, ?)") + .bind(`${owner}/${name}`, ACTING_AUTONOMY) + .run(); + await seedRecommendationOutcomes(env, `${owner}/${name}`, 5, 10); // would otherwise be a clear tightening signal + + await runSelfTune(env); + + expect(await loadShadowOverride(env as never, `${owner}/${name}`)).toBeNull(); + }); + it("unset review.selftune (the default) does not change today's behavior — an agent-configured repo still tunes normally", async () => { const env = createTestEnv({ GITTENSORY_REVIEW_SELFTUNE: "true" }); await seedRegisteredRepo(env, "owner/repo", ACTING_AUTONOMY);