Skip to content

Commit dc81d72

Browse files
authored
fix(review): require a real installation before acting-autonomy scans include a repo (#5023) (#5052)
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.
1 parent 0943bb3 commit dc81d72

10 files changed

Lines changed: 123 additions & 18 deletions

src/queue/processors.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -878,10 +878,20 @@ export async function fanOutAgentRegateSweepJobs(
878878
// outcome for just this repo (it gets picked up again next tick) instead of rejecting.
879879
try {
880880
const settings = await resolveRepositorySettings(env, repoFullName);
881+
// #sweep-requires-installation: isAgentConfigured resolves the OPERATOR'S global-default autonomy
882+
// (e.g. a self-host `.gittensory.yml` settings.autonomy block meant for the repos this instance
883+
// actually operates on) for ANY repoFullName, regardless of whether the GitHub App is installed
884+
// there. A repo that merely has a local `repositories` row (a stray subnet-registry row, say) with
885+
// no real `installationId` would otherwise inherit that global default and look "agent-configured"
886+
// purely by existing — even though no installation token exists to act on it, and it was never
887+
// intentionally onboarded. Require a real installation before the autonomy-based path can make a
888+
// repo eligible; the explicit allowlist path is untouched (GITTENSORY_REVIEW_REPOS is a deliberate,
889+
// operator-typed signal independent of installation state, e.g. reviewing ahead of a pending install).
890+
const hasInstallation = typeof repo.installationId === "number";
881891
if (
882892
!(
883893
isConvergenceRepoAllowed(env, repoFullName) ||
884-
isAgentConfigured(settings.autonomy)
894+
(hasInstallation && isAgentConfigured(settings.autonomy))
885895
)
886896
)
887897
return { kind: "ineligible" };

src/review/maintainer-recap-wire.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,10 @@ async function recapScanRepos(env: Env): Promise<string[]> {
103103
const configured: string[] = [];
104104
for (const repo of repos) {
105105
try {
106+
// #sweep-requires-installation: a repo with no real GitHub App installation must never be treated as
107+
// agent-configured purely because it resolves the operator's global-default autonomy by merely having
108+
// a local row -- mirrors fanOutAgentRegateSweepJobs's own guard.
109+
if (typeof repo.installationId !== "number") continue;
106110
const settings = await resolveRepositorySettings(env, repo.fullName);
107111
if (isAgentConfigured(settings.autonomy)) configured.push(repo.fullName);
108112
} catch {

src/review/ops-wire.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,10 @@ async function opsScanRepos(env: Env): Promise<string[]> {
155155
const configured: string[] = [];
156156
for (const repo of repos) {
157157
try {
158+
// #sweep-requires-installation: a repo with no real GitHub App installation must never be treated as
159+
// agent-configured purely because it resolves the operator's global-default autonomy by merely having
160+
// a local row -- mirrors fanOutAgentRegateSweepJobs's own guard.
161+
if (typeof repo.installationId !== "number") continue;
158162
const settings = await resolveRepositorySettings(env, repo.fullName);
159163
if (isAgentConfigured(settings.autonomy)) configured.push(repo.fullName);
160164
} catch {

src/review/pr-reconciliation.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,12 @@ async function watchedRepos(env: Env): Promise<Array<{ fullName: string; install
4444
for (const repo of byKey.values()) {
4545
try {
4646
const settings = await resolveRepositorySettings(env, repo.fullName);
47-
if (isConvergenceRepoAllowed(env, repo.fullName) || isAgentConfigured(settings.autonomy)) configured.push(repo);
47+
// #sweep-requires-installation: isAgentConfigured resolves the operator's global-default autonomy for
48+
// ANY repoFullName -- a repo that merely has a local row (no real GitHub App installation) would
49+
// otherwise inherit that default and look "agent-configured" purely by existing. Require a real
50+
// installation before the autonomy-based path counts; the explicit allowlist stays untouched.
51+
const hasInstallation = typeof repo.installationId === "number";
52+
if (isConvergenceRepoAllowed(env, repo.fullName) || (hasInstallation && isAgentConfigured(settings.autonomy))) configured.push(repo);
4853
} catch {
4954
/* a settings blip on one repo must not abort the whole reconciliation scan */
5055
}

src/review/selftune-wire.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,10 @@ async function selfTuneRepos(env: Env): Promise<string[]> {
106106
const configured: string[] = [];
107107
for (const repo of repos) {
108108
try {
109+
// #sweep-requires-installation: a repo with no real GitHub App installation must never be treated as
110+
// agent-configured purely because it resolves the operator's global-default autonomy by merely having
111+
// a local row -- mirrors fanOutAgentRegateSweepJobs's own guard.
112+
if (typeof repo.installationId !== "number") continue;
109113
const settings = await resolveRepositorySettings(env, repo.fullName);
110114
if (!isAgentConfigured(settings.autonomy)) continue;
111115
const manifest = await loadRepoFocusManifest(env, repo.fullName).catch(() => null);

src/review/sweep-watchdog.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,11 @@ async function watchedRepos(env: Env): Promise<Array<{ fullName: string; install
5757
for (const repo of byKey.values()) {
5858
try {
5959
const settings = await resolveRepositorySettings(env, repo.fullName);
60-
if (isConvergenceRepoAllowed(env, repo.fullName) || isAgentConfigured(settings.autonomy)) configured.push(repo);
60+
// #sweep-requires-installation: mirrors fanOutAgentRegateSweepJobs's own guard -- a repo with no real
61+
// GitHub App installation must never be treated as agent-configured purely because it resolves the
62+
// operator's global-default autonomy by merely having a local row.
63+
const hasInstallation = typeof repo.installationId === "number";
64+
if (isConvergenceRepoAllowed(env, repo.fullName) || (hasInstallation && isAgentConfigured(settings.autonomy))) configured.push(repo);
6165
} catch {
6266
/* a settings blip on one repo must not abort the whole watchdog scan */
6367
}

test/unit/maintainer-recap-wire.test.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,14 @@ function poisonDbPrepare(env: Env, pattern: RegExp): void {
2929
}
3030

3131
// Mark a repo registered so recapScanRepos picks it up (mirrors ops-wire.test.ts's seedRegisteredRepo).
32-
async function seedRegisteredRepo(env: Env, fullName: string): Promise<void> {
32+
async function seedRegisteredRepo(env: Env, fullName: string, installationId?: number): Promise<void> {
3333
const [owner, name] = fullName.split("/");
34+
// installationId is only needed by tests that rely on the acting-autonomy selection path
35+
// (#sweep-requires-installation): a repo with no real installation is never treated as agent-configured,
36+
// regardless of its resolved autonomy settings.
3437
await (env.DB as unknown as { prepare: (s: string) => { bind: (...v: unknown[]) => { run: () => Promise<unknown> } } })
35-
.prepare("INSERT INTO repositories (full_name, owner, name, is_installed, is_registered) VALUES (?, ?, ?, 1, 1)")
36-
.bind(fullName, owner, name)
38+
.prepare("INSERT INTO repositories (full_name, owner, name, is_installed, is_registered, installation_id) VALUES (?, ?, ?, 1, 1, ?)")
39+
.bind(fullName, owner, name, installationId ?? null)
3740
.run();
3841
}
3942

@@ -231,7 +234,7 @@ describe("runMaintainerRecapJob — cross-repo digest (#1963, #2248)", () => {
231234

232235
it("prefers agent-configured repos over the full registered set when at least one is configured", async () => {
233236
const env = createTestEnv({ DISCORD_WEBHOOK_URL: HOOK });
234-
await seedRegisteredRepo(env, "owner/configured");
237+
await seedRegisteredRepo(env, "owner/configured", 9402);
235238
await seedMergedPr(env, "owner/configured", 1);
236239
await upsertRepositorySettings(env, { repoFullName: "owner/configured", autonomy: { merge: "auto" } });
237240
await seedRegisteredRepo(env, "owner/unconfigured");

test/unit/ops-wire.test.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,19 @@ async function seedRegisteredRepo(env: Env, fullName: string): Promise<void> {
173173
.run();
174174
}
175175

176+
// A registered repo with a REAL installation + acting-autonomy settings, so opsScanRepos's "prefer
177+
// agent-configured repos" branch picks it up (#sweep-requires-installation: installation_id is required
178+
// alongside the autonomy row, matching the real upsertRepositoryFromGitHub invariant).
179+
async function seedAgentConfiguredRepo(env: Env, fullName: string, installationId: number): Promise<void> {
180+
const [owner, name] = fullName.split("/");
181+
await env.DB.prepare("INSERT INTO repositories (full_name, owner, name, is_installed, is_registered, installation_id) VALUES (?, ?, ?, 1, 1, ?)")
182+
.bind(fullName, owner, name, installationId)
183+
.run();
184+
await env.DB.prepare("INSERT INTO repository_settings (repo_full_name, autonomy_json) VALUES (?, ?)")
185+
.bind(fullName, JSON.stringify({ review: "auto" }))
186+
.run();
187+
}
188+
176189
// Seed a gate-block ledger anomaly: blocked PRs that later MERGED (false positives) over the min sample.
177190
async function seedGateFalsePositiveAnomaly(env: Env, repoFullName: string): Promise<void> {
178191
for (let i = 1; i <= 6; i += 1) {
@@ -228,6 +241,21 @@ describe("runOpsAlerts — cron path over gittensory's outcome data", () => {
228241
expect(errors.mock.calls.map((c) => String(c[0])).some((line) => line.includes("ops_anomaly\""))).toBe(false);
229242
});
230243

244+
it("REGRESSION (#sweep-requires-installation): prefers the agent-configured repo and never scans an uninstalled registered repo when a configured one exists", async () => {
245+
const env = createTestEnv();
246+
await seedAgentConfiguredRepo(env, "owner/configured", 9501);
247+
await seedGateFalsePositiveAnomaly(env, "owner/configured");
248+
// Registered, but no real installation -- must not count as "agent-configured" merely by resolving the
249+
// operator's global-default autonomy, and must be excluded from the scan once a configured repo exists.
250+
await seedRegisteredRepo(env, "owner/no-install");
251+
await seedGateFalsePositiveAnomaly(env, "owner/no-install");
252+
253+
const found = await runOpsAlerts(env);
254+
255+
expect(found["owner/configured"]?.some((a) => /gate false-positive spike/.test(a))).toBe(true);
256+
expect(found["owner/no-install"]).toBeUndefined();
257+
});
258+
231259
it("detects and reports a review burst end-to-end (a PR published far more review surfaces than normal in the window)", async () => {
232260
setSelfHostedMetricsMode(true); // keep the repo label so the counter assertion can target the exact series
233261
const env = createTestEnv();

test/unit/queue.test.ts

Lines changed: 32 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -888,9 +888,11 @@ describe("queue processors", () => {
888888
},
889889
} as unknown as Queue,
890890
});
891-
await upsertRepositoryFromGitHub(env, { name: "agent-a", full_name: "owner/agent-a", private: false, owner: { login: "owner" } });
892-
await upsertRepositoryFromGitHub(env, { name: "agent-b", full_name: "owner/agent-b", private: false, owner: { login: "owner" } });
893-
await upsertRepositoryFromGitHub(env, { name: "plain-repo", full_name: "owner/plain-repo", private: false, owner: { login: "owner" } });
891+
// #sweep-requires-installation: acting-autonomy eligibility also requires a real installation now — give
892+
// the repos that SHOULD be swept a real installationId, mirroring an actually-installed repo.
893+
await upsertRepositoryFromGitHub(env, { name: "agent-a", full_name: "owner/agent-a", private: false, owner: { login: "owner" } }, 9201);
894+
await upsertRepositoryFromGitHub(env, { name: "agent-b", full_name: "owner/agent-b", private: false, owner: { login: "owner" } }, 9202);
895+
await upsertRepositoryFromGitHub(env, { name: "plain-repo", full_name: "owner/plain-repo", private: false, owner: { login: "owner" } }, 9203);
894896
await upsertRepositorySettings(env, { repoFullName: "owner/agent-a", autonomy: { label: "auto" } });
895897
await upsertRepositorySettings(env, { repoFullName: "owner/agent-b", autonomy: { merge: "auto_with_approval" } });
896898
await upsertRepositorySettings(env, { repoFullName: "owner/plain-repo", autonomy: { review: "observe" } }); // non-acting → not configured
@@ -908,6 +910,27 @@ describe("queue processors", () => {
908910
expect(JSON.parse(fanout?.metadata_json ?? "{}")).toMatchObject({ repoCount: 2, requestedBy: "schedule" });
909911
});
910912

913+
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 () => {
914+
const sent: import("../../src/types").JobMessage[] = [];
915+
const env = createTestEnv({
916+
GITTENSORY_REVIEW_REPOS: "",
917+
JOBS: { async send(m: import("../../src/types").JobMessage) { sent.push(m); } } as unknown as Queue,
918+
});
919+
// A repo that merely EXISTS locally (e.g. a stray row from an unrelated sync) with a real installation and
920+
// acting autonomy — sweep-eligible.
921+
await upsertRepositoryFromGitHub(env, { name: "installed-repo", full_name: "owner/installed-repo", private: false, owner: { login: "owner" } }, 9301);
922+
await upsertRepositorySettings(env, { repoFullName: "owner/installed-repo", autonomy: { merge: "auto" } });
923+
// Same acting autonomy, but NO installationId (never upserted with one) — this is exactly the shape a
924+
// stray repositories row takes: it can still resolve a permissive global-default autonomy policy, but
925+
// there is no GitHub App installation on it at all, so it must never be treated as agent-configured.
926+
await upsertRepositoryFromGitHub(env, { name: "uninstalled-repo", full_name: "owner/uninstalled-repo", private: false, owner: { login: "owner" } });
927+
await upsertRepositorySettings(env, { repoFullName: "owner/uninstalled-repo", autonomy: { merge: "auto" } });
928+
929+
await processJob(env, { type: "agent-regate-sweep", requestedBy: "schedule" });
930+
931+
expect(sent.map((m) => (m.type === "agent-regate-sweep" ? m.repoFullName : null))).toEqual(["owner/installed-repo"]);
932+
});
933+
911934
it("agent re-gate sweep ALSO fans out to allowlisted repos regardless of autonomy mode (#sweep-all-modes)", async () => {
912935
const sent: import("../../src/types").JobMessage[] = [];
913936
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", () => {
931954
GITTENSORY_REVIEW_REPOS: "",
932955
JOBS: { async send(m: import("../../src/types").JobMessage) { sent.push(m); } } as unknown as Queue,
933956
});
934-
await upsertRepositoryFromGitHub(env, { name: "agent-a", full_name: "owner/agent-a", private: false, owner: { login: "owner" } });
935-
await upsertRepositoryFromGitHub(env, { name: "agent-b", full_name: "owner/agent-b", private: false, owner: { login: "owner" } });
957+
await upsertRepositoryFromGitHub(env, { name: "agent-a", full_name: "owner/agent-a", private: false, owner: { login: "owner" } }, 9204);
958+
await upsertRepositoryFromGitHub(env, { name: "agent-b", full_name: "owner/agent-b", private: false, owner: { login: "owner" } }, 9205);
936959
await upsertRepositorySettings(env, { repoFullName: "owner/agent-a", autonomy: { label: "auto" } });
937960
await upsertRepositorySettings(env, { repoFullName: "owner/agent-b", autonomy: { label: "auto" } });
938961
const realResolve = repositorySettingsModule.resolveRepositorySettings;
@@ -964,8 +987,8 @@ describe("queue processors", () => {
964987
},
965988
} as unknown as Queue,
966989
});
967-
await upsertRepositoryFromGitHub(env, { name: "agent-a", full_name: "owner/agent-a", private: false, owner: { login: "owner" } });
968-
await upsertRepositoryFromGitHub(env, { name: "agent-b", full_name: "owner/agent-b", private: false, owner: { login: "owner" } });
990+
await upsertRepositoryFromGitHub(env, { name: "agent-a", full_name: "owner/agent-a", private: false, owner: { login: "owner" } }, 9206);
991+
await upsertRepositoryFromGitHub(env, { name: "agent-b", full_name: "owner/agent-b", private: false, owner: { login: "owner" } }, 9207);
969992
await upsertRepositorySettings(env, { repoFullName: "owner/agent-a", autonomy: { label: "auto" } });
970993
await upsertRepositorySettings(env, { repoFullName: "owner/agent-b", autonomy: { label: "auto" } });
971994
const errors = vi.spyOn(console, "error").mockImplementation(() => undefined);
@@ -988,8 +1011,8 @@ describe("queue processors", () => {
9881011
JOBS: { async send(m: import("../../src/types").JobMessage) { sent.push(m); } } as unknown as Queue,
9891012
});
9901013
const repoNames = ["r1", "r2", "r3", "r4", "r5", "r6"];
991-
for (const name of repoNames) {
992-
await upsertRepositoryFromGitHub(env, { name, full_name: `owner/${name}`, private: false, owner: { login: "owner" } });
1014+
for (const [index, name] of repoNames.entries()) {
1015+
await upsertRepositoryFromGitHub(env, { name, full_name: `owner/${name}`, private: false, owner: { login: "owner" } }, 9300 + index);
9931016
await upsertRepositorySettings(env, { repoFullName: `owner/${name}`, autonomy: { label: "auto" } });
9941017
}
9951018
const { mapWithConcurrencyLimit: realMapWithConcurrencyLimit } =

test/unit/selftune-wiring.test.ts

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,11 @@ function poisonDbPrepare(env: Env, pattern: RegExp): void {
3030

3131
async function seedRegisteredRepo(env: Env, fullName: string, autonomyJson: string): Promise<void> {
3232
const [owner, name] = fullName.split("/");
33-
await env.DB.prepare("INSERT INTO repositories (full_name, owner, name, is_installed, is_registered) VALUES (?, ?, ?, 1, 1)")
34-
.bind(fullName, owner, name)
33+
// installation_id is required alongside is_installed=1 (#sweep-requires-installation): selfTuneRepos now
34+
// requires a real installation before isAgentConfigured counts, matching the real upsertRepositoryFromGitHub
35+
// invariant (isInstalled is only ever true alongside a real installationId).
36+
await env.DB.prepare("INSERT INTO repositories (full_name, owner, name, is_installed, is_registered, installation_id) VALUES (?, ?, ?, 1, 1, ?)")
37+
.bind(fullName, owner, name, 9401)
3538
.run();
3639
// Opt the repo into the acting-autonomy surface so isAgentConfigured(settings.autonomy) is true (selfTuneRepos filter).
3740
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)", () => {
273276
expect((await listOverrideAudit(env as never, "owner/opted-out")).length).toBe(0);
274277
});
275278

279+
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 () => {
280+
const owner = "owner";
281+
const name = "no-install";
282+
const env = createTestEnv({ GITTENSORY_REVIEW_SELFTUNE: "true" });
283+
await env.DB.prepare("INSERT INTO repositories (full_name, owner, name, is_installed, is_registered) VALUES (?, ?, ?, 0, 1)")
284+
.bind(`${owner}/${name}`, owner, name)
285+
.run();
286+
await env.DB.prepare("INSERT INTO repository_settings (repo_full_name, autonomy_json) VALUES (?, ?)")
287+
.bind(`${owner}/${name}`, ACTING_AUTONOMY)
288+
.run();
289+
await seedRecommendationOutcomes(env, `${owner}/${name}`, 5, 10); // would otherwise be a clear tightening signal
290+
291+
await runSelfTune(env);
292+
293+
expect(await loadShadowOverride(env as never, `${owner}/${name}`)).toBeNull();
294+
});
295+
276296
it("unset review.selftune (the default) does not change today's behavior — an agent-configured repo still tunes normally", async () => {
277297
const env = createTestEnv({ GITTENSORY_REVIEW_SELFTUNE: "true" });
278298
await seedRegisteredRepo(env, "owner/repo", ACTING_AUTONOMY);

0 commit comments

Comments
 (0)