From d8c58e054a641bf7d8f0036c7bf6a49a91da236b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 16 Jul 2026 09:34:52 -0700 Subject: [PATCH] fix(config): migrate badgeEnabled/publicQualityMetrics off the DB Batch A (#6442, merged as #6557) deliberately excluded these two fields because loadPublicRepoBadge/loadPublicRepoQualityMetrics read them via a raw getRepositorySettings call that bypasses the manifest overlay -- a perf tradeoff for two unauthenticated, high-frequency public routes. Per maintainer direction, finish the migration instead: both routes now read resolveRepositorySettings, accepting the manifest-cache lookup (and occasional cold-cache GitHub fetch) so .loopover.yml is honored here like every other settings.* field. Drops the two columns (migration 0158), removes them from the dashboard/ internal write paths and the maintainer settings panel, and updates the tests that seeded them via the DB to seed the focus manifest instead. Each live repo's current effective value (false) was already backfilled into its private .loopover.yml config before this drop. Part of #6442, epic #6440. --- .loopover.yml.example | 6 +++-- CONTRIBUTING.md | 10 +++++---- apps/loopover-ui/content/docs/github-app.mdx | 3 ++- apps/loopover-ui/content/docs/tuning.mdx | 3 ++- .../app-panels/gate-ramp-control.test.tsx | 2 -- .../site/app-panels/maintainer-settings.tsx | 6 ++--- .../lib/maintainer-settings-editable.test.ts | 6 ++--- .../src/lib/maintainer-settings-editable.ts | 4 ---- config/examples/loopover.full.yml | 6 +++-- ...159_drop_badge_quality_metrics_columns.sql | 10 +++++++++ src/api/routes.ts | 18 +++++---------- src/db/repositories.ts | 15 ++++++------- src/db/schema.ts | 2 -- test/integration/api.test.ts | 22 ++++++++++--------- ...public-quality-metrics-route-error.test.ts | 14 +++++------- test/integration/routes-errors.test.ts | 3 +-- test/unit/focus-manifest.test.ts | 14 ++++++------ 17 files changed, 71 insertions(+), 73 deletions(-) create mode 100644 migrations/0159_drop_badge_quality_metrics_columns.sql diff --git a/.loopover.yml.example b/.loopover.yml.example index 03a5a2a899..3eaeb0f79a 100644 --- a/.loopover.yml.example +++ b/.loopover.yml.example @@ -924,10 +924,12 @@ settings: # Bool. Default: true. backfillEnabled: true - # Render a README status badge for the repo. Bool. Default: false. + # Render a README status badge for the repo. Bool. Default: false. Config-as-code only (Batch A + # follow-up, loopover#6442) -- no DB column or dashboard toggle. badgeEnabled: false - # Publish a public per-repo review-quality page. Bool. Default: false. + # Publish a public per-repo review-quality page. Bool. Default: false. Config-as-code only (Batch A + # follow-up, loopover#6442) -- no DB column or dashboard toggle. publicQualityMetrics: false # Per-repo kill-switch: when true, the agent does nothing on this repo. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 18027a34a7..752e5ee1c2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -284,10 +284,12 @@ Public GitHub surfaces: Config as code (`.loopover.yml`) — every repository setting is controllable from the config file: -- **`settings:`** is a partial of the repository settings: any behaviour a maintainer can toggle in the - dashboard can be set here as code — `commentMode`, `publicAudienceMode`, `publicSurface`, `checkRunMode`, - `reviewCheckMode`, the gate-blocker modes, `autoLabelEnabled`, `gittensorLabel`, `requireLinkedIssue`, - `backfillEnabled`, etc. +- **`settings:`** is a partial of the repository settings. Any behaviour a maintainer can toggle in the + dashboard can also be set here as code — `reviewCheckMode`, the gate-blocker modes, `autoLabelEnabled`, + `gittensorLabel`, `requireLinkedIssue`, etc. A subset is config-as-code **only** (no DB column, no + dashboard toggle — this file is their sole source): `commentMode`, `publicAudienceMode`, + `publicSignalLevel`, `checkRunMode`, `checkRunDetailLevel`, `publicSurface`, `includeMaintainerAuthors`, + `backfillEnabled`, `badgeEnabled`, `publicQualityMetrics`, `regateSweepOrderMode`. - **`gate:`** is a friendly typed alias for the gate subset — `enabled` (on/off), `linkedIssue`, `duplicates`, `readiness: { mode, minScore }` (each `off | advisory | block`). - **`review:`** customizes the public review-panel CONTENT: `footer: { text }` (custom lead copy — the diff --git a/apps/loopover-ui/content/docs/github-app.mdx b/apps/loopover-ui/content/docs/github-app.mdx index 5edead8245..d6ced9b2b0 100644 --- a/apps/loopover-ui/content/docs/github-app.mdx +++ b/apps/loopover-ui/content/docs/github-app.mdx @@ -141,7 +141,8 @@ gates every author identically, regardless of config. { const payload = buildMaintainerSettingsSavePayload(SETTINGS); expect(Object.keys(payload).sort()).toEqual([...MAINTAINER_SETTINGS_EDITABLE_KEYS].sort()); expect(payload.linkedIssueGateMode).toBe("advisory"); - expect(payload.badgeEnabled).toBe(false); + expect(payload.autoLabelEnabled).toBe(true); }); it("buildMaintainerSettingsSavePayload merges a partial patch over the base settings", () => { @@ -47,7 +45,7 @@ describe("maintainer-settings-editable (#2218)", () => { expect(payload.duplicatePrGateMode).toBe("block"); // Untouched fields pass through unchanged. expect(payload.qualityGateMode).toBe("advisory"); - expect(payload.badgeEnabled).toBe(false); + expect(payload.autoLabelEnabled).toBe(true); }); it("an empty patch object is a no-op (same as omitting it)", () => { diff --git a/apps/loopover-ui/src/lib/maintainer-settings-editable.ts b/apps/loopover-ui/src/lib/maintainer-settings-editable.ts index 83d3c6bfab..761bdcd3d0 100644 --- a/apps/loopover-ui/src/lib/maintainer-settings-editable.ts +++ b/apps/loopover-ui/src/lib/maintainer-settings-editable.ts @@ -36,8 +36,6 @@ export type MaintainerSettingsEditable = { // #6443: gittensorLabel/createMissingLabel removed -- no longer DB-backed, config-as-code only via // .loopover.yml's settings: block now (the dashboard can no longer write them). requireLinkedIssue: boolean; - badgeEnabled: boolean; - publicQualityMetrics: boolean; commandAuthorization: CommandAuthorization; autonomy: Partial>; autoMaintain: { requireApprovals: number; mergeMethod: AutoMergeMethod }; @@ -61,8 +59,6 @@ export const MAINTAINER_SETTINGS_EDITABLE_KEYS: Array; async function loadPublicRepoBadge(env: Env, owner: string, repo: string): Promise { const repository = await getRepository(env, `${owner}/${repo}`); if (!repository || repository.isPrivate || !repository.isInstalled) return null; - // Intentionally the raw DB row, not resolveRepositorySettings: this is an unauthenticated, high-frequency - // public route (a README-embedded badge image), so it deliberately trades honoring a yml-only `badgeEnabled` - // override for avoiding a manifest-cache lookup (and a possible cold-cache GitHub fetch) on every image load. - // `badgeEnabled` is normally set via the dashboard/API, which persists straight to this same DB row (#2912). - const settings = await getRepositorySettings(env, repository.fullName); + // badgeEnabled has no DB column anymore (Batch A follow-up, loopover#6442) -- config-as-code only, so + // this must read the resolved (manifest-overlaid) settings instead of the old raw-DB-row shortcut. An + // accepted perf tradeoff (a manifest-cache lookup, occasionally a cold-cache GitHub fetch, on this + // unauthenticated high-frequency README-badge route) in exchange for `.loopover.yml` being honored here. + const settings = await resolveRepositorySettings(env, repository.fullName); if (!settings.badgeEnabled) return null; const pullRequests = await listPullRequests(env, repository.fullName); return buildPublicRepoQuality(pullRequests); @@ -339,7 +339,7 @@ async function loadPublicRepoBadge(env: Env, owner: string, repo: string): Promi async function loadPublicRepoQualityMetrics(env: Env, owner: string, repo: string) { const repository = await getRepository(env, `${owner}/${repo}`); if (!repository || repository.isPrivate || !repository.isInstalled) return null; - const settings = await getRepositorySettings(env, repository.fullName); + const settings = await resolveRepositorySettings(env, repository.fullName); if (!settings.publicQualityMetrics) return null; return loadPublicQualityMetrics(env, repository.fullName); } @@ -683,8 +683,6 @@ const repositorySettingsSchema = z.object({ // #6443: gittensorLabel/blacklistLabel/createMissingLabel/contributorBlacklist removed -- no longer // DB-backed, config-as-code only via .loopover.yml's settings: block now. requireLinkedIssue: z.boolean().default(false), - badgeEnabled: z.boolean().default(false), - publicQualityMetrics: z.boolean().default(false), commandAuthorization: z .object({ default: z.array(z.enum(["maintainer", "collaborator", "pr_author", "confirmed_miner"])).max(4).optional(), @@ -721,8 +719,6 @@ const maintainerSettingsSchema = z autoLabelEnabled: z.boolean(), closeOwnerAuthors: z.boolean(), requireLinkedIssue: z.boolean(), - badgeEnabled: z.boolean(), - publicQualityMetrics: z.boolean(), agentPaused: z.boolean(), agentDryRun: z.boolean(), requireFreshRebaseWindowMinutes: z.number().int().positive().nullable(), @@ -4231,8 +4227,6 @@ export function createApp() { closeOwnerAuthors: parsed.data.closeOwnerAuthors, autoLabelEnabled: parsed.data.autoLabelEnabled, requireLinkedIssue: parsed.data.requireLinkedIssue, - badgeEnabled: parsed.data.badgeEnabled, - publicQualityMetrics: parsed.data.publicQualityMetrics, commandAuthorization: normalizeCommandAuthorizationPolicy(parsed.data.commandAuthorization).policy, }), ); diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 0ac73b5496..5fc5082950 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -705,8 +705,9 @@ export async function getRepositorySettings(env: Env, fullName: string): Promise includeMaintainerAuthors: false, requireLinkedIssue: row.requireLinkedIssue, backfillEnabled: true, - badgeEnabled: row.badgeEnabled, - publicQualityMetrics: row.publicQualityMetrics, + // Config-as-code only (Batch A follow-up, loopover#6442): no DB column anymore, see migration 0158. + badgeEnabled: false, + publicQualityMetrics: false, agentPaused: row.agentPaused, agentDryRun: row.agentDryRun, commandAuthorization: parseCommandAuthorizationPolicy(row.commandAuthorizationJson), @@ -830,8 +831,10 @@ export async function upsertRepositorySettings(env: Env, settings: Partial { // Installed + opted in, with assessed merged PRs. await upsertRepositoryFromGitHub(env, { name: "badged", full_name: "acme/badged", private: false, owner: { login: "acme" }, default_branch: "main" }, 555); - await upsertRepositorySettings(env, { repoFullName: "acme/badged", badgeEnabled: true }); + await upsertRepoFocusManifest(env, "acme/badged", { settings: { badgeEnabled: true } }); await upsertPullRequestFromGitHub(env, "acme/badged", { number: 1, title: "Feature", state: "merged", created_at: "2026-06-01T00:00:00Z", merged_at: "2026-06-01T04:00:00Z", labels: [] }); await upsertPullRequestFromGitHub(env, "acme/badged", { number: 2, title: "Slop", state: "merged", created_at: "2026-06-02T00:00:00Z", merged_at: "2026-06-02T06:00:00Z", labels: [] }); await updatePullRequestSlopAssessment(env, "acme/badged", 1, { slopRisk: 0, slopBand: "clean" }); @@ -310,7 +310,7 @@ describe("api routes", () => { // Private repos stay unavailable even when installed and explicitly opted in. await upsertRepositoryFromGitHub(env, { name: "private", full_name: "acme/private", private: true, owner: { login: "acme" }, default_branch: "main" }, 558); - await upsertRepositorySettings(env, { repoFullName: "acme/private", badgeEnabled: true }); + await upsertRepoFocusManifest(env, "acme/private", { settings: { badgeEnabled: true } }); await upsertPullRequestFromGitHub(env, "acme/private", { number: 1, title: "Secret", state: "merged", created_at: "2026-06-03T00:00:00Z", merged_at: "2026-06-03T02:00:00Z", labels: [] }); await updatePullRequestSlopAssessment(env, "acme/private", 1, { slopRisk: 0, slopBand: "clean" }); const privateSvg = await app.request("/v1/public/repos/acme/private/badge.svg", {}, env); @@ -322,7 +322,7 @@ describe("api routes", () => { // Opted in but NOT installed → unavailable. await upsertRepositoryFromGitHub(env, { name: "uninstalled", full_name: "acme/uninstalled", private: false, owner: { login: "acme" }, default_branch: "main" }); - await upsertRepositorySettings(env, { repoFullName: "acme/uninstalled", badgeEnabled: true }); + await upsertRepoFocusManifest(env, "acme/uninstalled", { settings: { badgeEnabled: true } }); const notInstalled = await app.request("/v1/public/repos/acme/uninstalled/badge.svg", {}, env); expect(notInstalled.status).toBe(404); @@ -337,7 +337,7 @@ describe("api routes", () => { const env = createTestEnv(); await upsertRepositoryFromGitHub(env, { name: "quality", full_name: "acme/quality", private: false, owner: { login: "acme" }, default_branch: "main" }, 560); - await upsertRepositorySettings(env, { repoFullName: "acme/quality", publicQualityMetrics: true }); + await upsertRepoFocusManifest(env, "acme/quality", { settings: { publicQualityMetrics: true } }); await upsertPullRequestFromGitHub(env, "acme/quality", { number: 1, title: "Merged", state: "merged", created_at: "2026-06-01T00:00:00Z", merged_at: "2026-06-02T00:00:00Z", labels: [] }); await upsertPullRequestFromGitHub(env, "acme/quality", { number: 2, title: "Merged too", state: "merged", created_at: "2026-06-01T01:00:00Z", merged_at: "2026-06-02T01:00:00Z", labels: [] }); await upsertPullRequestFromGitHub(env, "acme/quality", { number: 3, title: "Closed", state: "closed", created_at: "2026-06-03T00:00:00Z", labels: [] }); @@ -371,13 +371,13 @@ describe("api routes", () => { // Private repos stay unavailable even when installed and explicitly opted in. await upsertRepositoryFromGitHub(env, { name: "quality-private", full_name: "acme/quality-private", private: true, owner: { login: "acme" }, default_branch: "main" }, 562); - await upsertRepositorySettings(env, { repoFullName: "acme/quality-private", publicQualityMetrics: true }); + await upsertRepoFocusManifest(env, "acme/quality-private", { settings: { publicQualityMetrics: true } }); const privateRes = await app.request("/v1/public/repos/acme/quality-private/quality", {}, env); expect(privateRes.status).toBe(404); // Opted in but NOT installed → unavailable. await upsertRepositoryFromGitHub(env, { name: "quality-uninstalled", full_name: "acme/quality-uninstalled", private: false, owner: { login: "acme" }, default_branch: "main" }); - await upsertRepositorySettings(env, { repoFullName: "acme/quality-uninstalled", publicQualityMetrics: true }); + await upsertRepoFocusManifest(env, "acme/quality-uninstalled", { settings: { publicQualityMetrics: true } }); const notInstalled = await app.request("/v1/public/repos/acme/quality-uninstalled/quality", {}, env); expect(notInstalled.status).toBe(404); @@ -387,7 +387,7 @@ describe("api routes", () => { await expect(unknown.json()).resolves.toMatchObject({ error: "not_found" }); }); - it("persists the publicQualityMetrics opt-in through the settings write endpoint (#2568)", async () => { + it("REGRESSION (Batch A follow-up, loopover#6442): ignores a publicQualityMetrics opt-in posted to the settings write endpoint (config-as-code only now)", async () => { const app = createApp(); const env = createTestEnv(); const response = await app.request( @@ -396,10 +396,11 @@ describe("api routes", () => { env, ); expect(response.status).toBe(200); - await expect(response.json()).resolves.toMatchObject({ repoFullName: "acme/quality", publicQualityMetrics: true }); + // publicQualityMetrics has no DB column anymore -- only .loopover.yml's settings: block can opt a repo in. + await expect(response.json()).resolves.toMatchObject({ repoFullName: "acme/quality", publicQualityMetrics: false }); }); - it("persists the badgeEnabled opt-in through the settings write endpoint (#541)", async () => { + it("REGRESSION (Batch A follow-up, loopover#6442): ignores a badgeEnabled opt-in posted to the settings write endpoint (config-as-code only now)", async () => { const app = createApp(); const env = createTestEnv(); const response = await app.request( @@ -408,7 +409,8 @@ describe("api routes", () => { env, ); expect(response.status).toBe(200); - await expect(response.json()).resolves.toMatchObject({ repoFullName: "acme/badged", badgeEnabled: true }); + // badgeEnabled has no DB column anymore -- only .loopover.yml's settings: block can opt a repo in. + await expect(response.json()).resolves.toMatchObject({ repoFullName: "acme/badged", badgeEnabled: false }); }); it("downgrades qualityGateMode: block to advisory through the internal settings write endpoint too (#2267)", async () => { diff --git a/test/integration/public-quality-metrics-route-error.test.ts b/test/integration/public-quality-metrics-route-error.test.ts index 4e230f2484..74c8d5ec59 100644 --- a/test/integration/public-quality-metrics-route-error.test.ts +++ b/test/integration/public-quality-metrics-route-error.test.ts @@ -7,19 +7,17 @@ vi.mock("../../src/services/public-quality-metrics", () => ({ import { createApp } from "../../src/api/routes"; import { createTestEnv } from "../helpers/d1"; -import { upsertRepositoryFromGitHub, upsertRepositorySettings } from "../../src/db/repositories"; +import { upsertRepositoryFromGitHub } from "../../src/db/repositories"; +import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; describe("GET /v1/public/repos/:owner/:repo/quality — error path", () => { it("returns 503 when quality metrics computation throws", async () => { const env = createTestEnv(); await upsertRepositoryFromGitHub(env, { name: "quality", full_name: "acme/quality", private: false, owner: { login: "acme" }, default_branch: "main" }, 560); - // NOTE: publicQualityMetrics intentionally stays DB-backed here (not moved to the focus manifest) - // because the route under test (`loadPublicRepoQualityMetrics` in src/api/routes.ts) reads - // `getRepositorySettings` directly -- the same deliberate raw-DB-row bypass documented on the sibling - // `loadPublicRepoBadge` helper -- and never consults `resolveRepositorySettings`/the manifest overlay. - // Moving this field to `upsertRepoFocusManifest` would make the route see publicQualityMetrics=false - // (404) instead of true (503 via the mocked throw), which is a real behavior difference, not a wiring bug. - await upsertRepositorySettings(env, { repoFullName: "acme/quality", publicQualityMetrics: true }); + // publicQualityMetrics has no DB column anymore (Batch A follow-up, loopover#6442) -- config-as-code + // only. loadPublicRepoQualityMetrics now reads resolveRepositorySettings (manifest-aware), so opt-in + // must go through the manifest, not a DB row. + await upsertRepoFocusManifest(env, "acme/quality", { settings: { publicQualityMetrics: true } }); const res = await createApp().request("/v1/public/repos/acme/quality/quality", {}, env); expect(res.status).toBe(503); diff --git a/test/integration/routes-errors.test.ts b/test/integration/routes-errors.test.ts index 685bd45342..c4a0b5eea0 100644 --- a/test/integration/routes-errors.test.ts +++ b/test/integration/routes-errors.test.ts @@ -1185,13 +1185,12 @@ describe("api route guards and error branches", () => { headers: internalHeaders(env), body: JSON.stringify({ gatePack: "oss-anti-slop", - badgeEnabled: true, }), }, env, ); expect(updated.status).toBe(200); - await expect(updated.json()).resolves.toMatchObject({ gatePack: "oss-anti-slop", badgeEnabled: true }); + await expect(updated.json()).resolves.toMatchObject({ gatePack: "oss-anti-slop" }); }); it("exposes and clears self-tune overrides for operators, rejecting unauthorized callers (#6168)", async () => { diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index db91fb34f2..4915f15a22 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -3039,16 +3039,16 @@ describe("parseFocusManifest settings override + resolveEffectiveSettings", () = expect(eff.linkedIssueGateMode).toBe("block"); // gate: wins over settings: }); - it("wires settings.badgeEnabled into the manifest parser and lets it override the DB value (#2555)", () => { + it("wires settings.badgeEnabled into the manifest parser and lets it override the built-in default (#2555, Batch A follow-up loopover#6442)", () => { const parsedTrue = parseFocusManifest({ settings: { badgeEnabled: true } }); expect(parsedTrue.settings.badgeEnabled).toBe(true); expect(parsedTrue.warnings).toEqual([]); const parsedFalse = parseFocusManifest({ settings: { badgeEnabled: false } }); expect(parsedFalse.settings.badgeEnabled).toBe(false); - const db = { badgeEnabled: false } as unknown as RepositorySettings; - const eff = resolveEffectiveSettings(db, parseFocusManifest({ settings: { badgeEnabled: true } })); - expect(eff.badgeEnabled).toBe(true); // settings: override wins over the DB-stored value + const base = { badgeEnabled: false } as unknown as RepositorySettings; + const eff = resolveEffectiveSettings(base, parseFocusManifest({ settings: { badgeEnabled: true } })); + expect(eff.badgeEnabled).toBe(true); // settings: override wins over the built-in default (no DB column anymore) }); it("wires settings.includeMaintainerAuthors into the manifest parser and resolver (#2052)", () => { @@ -3073,15 +3073,15 @@ describe("parseFocusManifest settings override + resolveEffectiveSettings", () = expect(reparsed.settings.includeMaintainerAuthors).toBe(true); }); - it("wires settings.publicQualityMetrics into the manifest parser and lets it override the DB value (#2568)", () => { + it("wires settings.publicQualityMetrics into the manifest parser and lets it override the built-in default (#2568, Batch A follow-up loopover#6442)", () => { const parsedTrue = parseFocusManifest({ settings: { publicQualityMetrics: true } }); expect(parsedTrue.settings.publicQualityMetrics).toBe(true); expect(parsedTrue.warnings).toEqual([]); const parsedFalse = parseFocusManifest({ settings: { publicQualityMetrics: false } }); expect(parsedFalse.settings.publicQualityMetrics).toBe(false); - const db = { publicQualityMetrics: false } as unknown as RepositorySettings; - const eff = resolveEffectiveSettings(db, parseFocusManifest({ settings: { publicQualityMetrics: true } })); + const base = { publicQualityMetrics: false } as unknown as RepositorySettings; + const eff = resolveEffectiveSettings(base, parseFocusManifest({ settings: { publicQualityMetrics: true } })); expect(eff.publicQualityMetrics).toBe(true); });