diff --git a/.gittensory.yml.example b/.gittensory.yml.example index aaed6cc35d..a18e2df811 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -112,7 +112,10 @@ gate: # Quality / merge-readiness score gate (the PR-quality score). readiness: - # off | advisory | block. Default: advisory. + # off | advisory. Default: advisory. Informational only — this dimension can + # never hard-block a PR, so "block" is not an accepted value (a config that + # still says "block" is downgraded to "advisory" with a warning). For an + # enforceable quality floor, use mergeReadiness or manifestPolicy below. mode: advisory # At/above this score the quality dimension passes. # Number 0–100, or null for the engine default band. Default: null. diff --git a/src/api/routes.ts b/src/api/routes.ts index 3d386c5344..8a1be1d35c 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -238,7 +238,7 @@ import { isRagEnabled } from "../review/rag-wire"; import { getPublicStats, isPublicStatsEnabled } from "../review/public-stats"; import { buildMaintainerQualityDashboard, isMaintainerQualityDataStale } from "../services/maintainer-quality-dashboard"; import { MAX_LOCAL_SCORER_WARNING_CHARS, MAX_LOCAL_SCORER_WARNING_COUNT } from "../signals/local-scorer-diagnostics"; -import { compileFocusManifestPolicy, MAX_FOCUS_MANIFEST_BYTES } from "../signals/focus-manifest"; +import { compileFocusManifestPolicy, MAX_FOCUS_MANIFEST_BYTES, normalizeReadinessGateMode } from "../signals/focus-manifest"; import { resolveRepositorySettings } from "../settings/repository-settings"; import { loadPublicRepoFocusManifest, loadRepoFocusManifest, upsertRepoFocusManifest } from "../signals/focus-manifest-loader"; import { buildRepoOnboardingPackPreviewForRepo } from "../services/repo-onboarding-pack"; @@ -265,6 +265,7 @@ import type { PullRequestRecord, RepoSyncSegmentRecord, RepositoryRecord, + RepositorySettings, } from "../types"; import { errorMessage, nowIso } from "../utils/json"; @@ -686,6 +687,21 @@ const maintainerSettingsSchema = z }) .partial(); +/** Readiness/quality can never hard-block a PR (buildQualityGateWarning is always advisory-severity; + * isConfiguredGateBlocker has no branch for it) — downgrade a settings-write's `qualityGateMode: "block"` to + * `"advisory"` here too, mirroring the same downgrade `.gittensory.yml`'s `gate.readiness.mode` / + * `settings.qualityGateMode` already get in normalizeReadinessGateMode, so the dashboard/API save path can + * never persist a value that implies enforcement it doesn't have (#2267). Callers check for `undefined` + * (a PATCH-style save that didn't touch this field) before calling — this only handles the defined case, so + * the return type never needs `undefined` under `exactOptionalPropertyTypes`. The warnings array is scratch; + * these routes have no warnings-response protocol. */ +function downgradeQualityGateMode(mode: "off" | "advisory" | "block"): "off" | "advisory" { + /* v8 ignore next -- never null for a Zod-validated "off"|"advisory"|"block" input (normalizeReadinessGateMode's + "must be one of" branch only fires for a value outside that set); the fallback only satisfies the shared + parser's broader return type. */ + return (normalizeReadinessGateMode(mode, "qualityGateMode", []) as "off" | "advisory" | null) ?? "advisory"; +} + // Maintainer BYOK provider key. Write-only: the key is encrypted at rest and never returned. A loose // prefix check catches the common provider/key mismatch (e.g. pasting an OpenAI key under Anthropic) // without coupling to exact provider key formats: Anthropic keys start with `sk-ant-`; OpenAI keys @@ -2105,7 +2121,8 @@ export function createApp() { const parsed = maintainerSettingsSchema.safeParse(body); if (!parsed.success) return c.json({ error: "invalid_repository_settings", issues: parsed.error.issues }, 400); const current = await getRepositorySettings(c.env, fullName); - const changes = Object.fromEntries(Object.entries(parsed.data).filter(([, value]) => value !== undefined)); + const changes = Object.fromEntries(Object.entries(parsed.data).filter(([, value]) => value !== undefined)) as Partial; + if (changes.qualityGateMode !== undefined) changes.qualityGateMode = downgradeQualityGateMode(changes.qualityGateMode); const updated = await upsertRepositorySettings(c.env, { ...current, ...changes, repoFullName: fullName }); await recordAuditEvent(c.env, { eventType: "repo.settings_updated", @@ -3424,7 +3441,7 @@ export function createApp() { gatePack: parsed.data.gatePack, linkedIssueGateMode: parsed.data.linkedIssueGateMode, duplicatePrGateMode: parsed.data.duplicatePrGateMode, - qualityGateMode: parsed.data.qualityGateMode, + qualityGateMode: downgradeQualityGateMode(parsed.data.qualityGateMode), qualityGateMinScore: parsed.data.qualityGateMinScore, aiReviewMode: parsed.data.aiReviewMode, aiReviewByok: parsed.data.aiReviewByok, diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 1cf6fe245d..fb617217c5 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -357,6 +357,21 @@ function normalizeOptionalGateMode(value: JsonValue | undefined, field: string, return null; } +/** `gate.readiness.mode` (and its `settings.qualityGateMode` alias below) is documented and parsed as the shared + * off/advisory/block tri-state, but buildQualityGateWarning (src/rules/advisory.ts) always produces a + * warning-severity finding — never a blocker — and isConfiguredGateBlocker has no branch for it: readiness/ + * quality is intentionally informational-only and can never hard-block a PR. Without this, a maintainer who + * sets `mode: block` believes a real quality floor is enforced when the effective behavior is silently + * advisory-only (#2267). Downgrade "block" to "advisory" here, with a clear deprecation warning, so the parsed + * config always matches what the gate actually does. Exported so the settings-write API routes (the + * dashboard/API path for the SAME `qualityGateMode` field) can apply the identical downgrade before persisting. */ +export function normalizeReadinessGateMode(value: JsonValue | undefined, field: string, warnings: string[]): GateRuleMode | null { + const mode = normalizeOptionalGateMode(value, field, warnings); + if (mode !== "block") return mode; + warnings.push(`Manifest gate field "${field}" no longer accepts "block" — readiness/quality is informational-only and can never hard-block a PR; downgrading to "advisory". Use gate.manifestPolicy or another enforceable gate for a real quality floor.`); + return "advisory"; +} + function normalizeOptionalBoolean(value: JsonValue | undefined, field: string, warnings: string[]): boolean | null { if (value === undefined || value === null) return null; if (typeof value === "boolean") return value; @@ -422,7 +437,7 @@ function parseGateConfig(value: JsonValue | undefined, warnings: string[]): Focu pack: normalizeOptionalEnum(record.pack, "gate.pack", ["gittensor", "oss-anti-slop"] as const, warnings), linkedIssue: normalizeOptionalGateMode(record.linkedIssue, "gate.linkedIssue", warnings), duplicates: normalizeOptionalGateMode(record.duplicates, "gate.duplicates", warnings), - readinessMode: normalizeOptionalGateMode(readinessRecord?.mode, "gate.readiness.mode", warnings), + readinessMode: normalizeReadinessGateMode(readinessRecord?.mode, "gate.readiness.mode", warnings), readinessMinScore: normalizeOptionalScore(readinessRecord?.minScore, "gate.readiness.minScore", warnings), slopMode: normalizeOptionalGateMode(slopRecord?.mode, "gate.slop.mode", warnings), slopMinScore: normalizeOptionalScore(slopRecord?.minScore, "gate.slop.minScore", warnings), @@ -584,7 +599,10 @@ function parseSettingsOverride(value: JsonValue | undefined, warnings: string[]) if (duplicatePrGateMode !== null) out.duplicatePrGateMode = duplicatePrGateMode; const selfAuthoredLinkedIssueGateMode = normalizeOptionalGateMode(r.selfAuthoredLinkedIssueGateMode, "settings.selfAuthoredLinkedIssueGateMode", warnings); if (selfAuthoredLinkedIssueGateMode !== null) out.selfAuthoredLinkedIssueGateMode = selfAuthoredLinkedIssueGateMode; - const qualityGateMode = normalizeOptionalGateMode(r.qualityGateMode, "settings.qualityGateMode", warnings); + // Same tri-state field as gate.readiness.mode above (the friendly gate alias overlays onto it in + // resolveEffectiveSettings) — apply the identical "block" → "advisory" downgrade here too, so a maintainer + // setting `settings.qualityGateMode: block` directly hits the same deprecation warning (#2267). + const qualityGateMode = normalizeReadinessGateMode(r.qualityGateMode, "settings.qualityGateMode", warnings); if (qualityGateMode !== null) out.qualityGateMode = qualityGateMode; const qualityGateMinScore = normalizeOptionalScore(r.qualityGateMinScore, "settings.qualityGateMinScore", warnings); if (qualityGateMinScore !== null) out.qualityGateMinScore = qualityGateMinScore; @@ -983,6 +1001,14 @@ export function resolveEffectiveSettings( if (effective.requireLinkedIssue && effective.linkedIssueGateMode === "off") { effective.linkedIssueGateMode = "block"; } + // Readiness/quality can never hard-block a PR (buildQualityGateWarning is always advisory-severity; + // isConfiguredGateBlocker has no branch for it). The write-time guards (the settings.qualityGateMode / + // gate.readiness.mode parsers above, and the settings-write API routes) stop a NEW "block" value from being + // introduced, but a repo whose DB row already has quality_gate_mode = "block" from before those guards + // existed would still resolve to it here. Downgrade it at this single resolver too, so the EFFECTIVE settings + // the gate/review pipeline AND the settings-preview dashboard read (both call this function) can never carry + // a value that implies enforcement it doesn't have, regardless of when or where it was written (#2267). + if (effective.qualityGateMode === "block") effective.qualityGateMode = "advisory"; effective.contributorBlacklist = mergeContributorBlacklists(effective.contributorBlacklist ?? [], sharedContributorBlacklist); return effective; } diff --git a/test/integration/api.test.ts b/test/integration/api.test.ts index 9365fe18a4..df4dc4e74b 100644 --- a/test/integration/api.test.ts +++ b/test/integration/api.test.ts @@ -269,6 +269,20 @@ describe("api routes", () => { await expect(response.json()).resolves.toMatchObject({ repoFullName: "acme/badged", badgeEnabled: true }); }); + it("downgrades qualityGateMode: block to advisory through the internal settings write endpoint too (#2267)", async () => { + // Readiness/quality can never hard-block a PR — the internal full-settings write path (used by tooling, + // not just the maintainer dashboard) gets the identical downgrade so it can't persist "block" either. + const app = createApp(); + const env = createTestEnv(); + const response = await app.request( + "/v1/internal/repos/acme/readiness-block/settings", + { method: "POST", headers: { authorization: `Bearer ${env.INTERNAL_JOB_TOKEN}`, "content-type": "application/json" }, body: JSON.stringify({ qualityGateMode: "block" }) }, + env, + ); + expect(response.status).toBe(200); + await expect(response.json()).resolves.toMatchObject({ repoFullName: "acme/readiness-block", qualityGateMode: "advisory" }); + }); + it("rejects invalid public GitHub repo stats paths before calling GitHub", async () => { const app = createApp(); const env = createTestEnv(); @@ -2274,7 +2288,10 @@ describe("api routes", () => { method: "PUT", headers: ownerHeaders, // #773/#774/#776: the agent-layer config is settable here; the DB layer drops an unknown action class. - body: JSON.stringify({ gateCheckMode: "enabled", slopGateMode: "block", slopGateMinScore: 55, autonomy: { merge: "auto_with_approval", deploy: "auto" }, autoMaintain: { requireApprovals: 2, mergeMethod: "rebase" }, agentPaused: true, agentDryRun: true }), + // #2267: qualityGateMode: "block" is downgraded to "advisory" on write — readiness/quality can never + // hard-block a PR, so the dashboard/API save path can't persist a value implying enforcement it doesn't + // have. slopGateMode: "block" is a DIFFERENT, legitimately-blockable dimension and is left untouched. + body: JSON.stringify({ gateCheckMode: "enabled", slopGateMode: "block", slopGateMinScore: 55, qualityGateMode: "block", autonomy: { merge: "auto_with_approval", deploy: "auto" }, autoMaintain: { requireApprovals: 2, mergeMethod: "rebase" }, agentPaused: true, agentDryRun: true }), }, ownerEnv, ); @@ -2283,6 +2300,7 @@ describe("api routes", () => { gateCheckMode: "enabled", slopGateMode: "block", slopGateMinScore: 55, + qualityGateMode: "advisory", // #2267: downgraded, not persisted as "block" autonomy: { merge: "auto_with_approval" }, // unknown action class dropped by the DB normalizer autoMaintain: { requireApprovals: 2, mergeMethod: "rebase" }, agentPaused: true, // #776 kill-switch diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index bfc420ec20..311fdf0ad5 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -780,9 +780,11 @@ describe("public-safe invariant", () => { describe("parseFocusManifest gate config", () => { it("parses a full gate section including the readiness block", () => { - const m = parseFocusManifest({ gate: { linkedIssue: "block", duplicates: "advisory", readiness: { mode: "block", minScore: 70 } } }); + // readiness.mode uses "advisory" here (not "block") — readiness/quality can never hard-block (#2267); + // the block→advisory deprecation-downgrade behavior itself is covered separately below. + const m = parseFocusManifest({ gate: { linkedIssue: "block", duplicates: "advisory", readiness: { mode: "advisory", minScore: 70 } } }); expect(m.present).toBe(true); - expect(m.gate).toEqual({ present: true, enabled: null, pack: null, linkedIssue: "block", duplicates: "advisory", readinessMode: "block", readinessMinScore: 70, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, manifestPolicy: null, dryRun: null, firstTimeContributorGrace: null }); + expect(m.gate).toEqual({ present: true, enabled: null, pack: null, linkedIssue: "block", duplicates: "advisory", readinessMode: "advisory", readinessMinScore: 70, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, manifestPolicy: null, dryRun: null, firstTimeContributorGrace: null }); }); it("parses gate.mergeReadiness + gate.firstTimeContributorGrace, round-trips them, and warns on bad values (#822)", () => { @@ -892,6 +894,21 @@ describe("parseFocusManifest gate config", () => { expect(m.warnings.some((w) => /gate\.readiness\.mode/.test(w))).toBe(true); }); + it("downgrades gate.readiness.mode: block to advisory with a deprecation warning (#2267)", () => { + // readiness/quality is informational-only (buildQualityGateWarning always produces a warning-severity + // finding; isConfiguredGateBlocker has no branch for it) — a config that says "block" is downgraded + // rather than silently accepted, so the parsed config always matches what the gate actually does. + const m = parseFocusManifest({ gate: { readiness: { mode: "block" } } }); + expect(m.gate.readinessMode).toBe("advisory"); + expect(m.gate.present).toBe(true); + expect(m.warnings.some((w) => /gate\.readiness\.mode.*no longer accepts "block"/.test(w))).toBe(true); + // Genuinely invalid values still take the ORIGINAL "must be one of" warning path, unchanged. + const bad = parseFocusManifest({ gate: { readiness: { mode: "sometimes" } } }); + expect(bad.gate.readinessMode).toBeNull(); + expect(bad.warnings.some((w) => /gate\.readiness\.mode.*must be one of/.test(w))).toBe(true); + expect(bad.warnings.some((w) => /no longer accepts "block"/.test(w))).toBe(false); + }); + it("clamps and rounds the readiness minScore to 0-100", () => { expect(parseFocusManifest({ gate: { readiness: { minScore: 250 } } }).gate.readinessMinScore).toBe(100); expect(parseFocusManifest({ gate: { readiness: { minScore: -10 } } }).gate.readinessMinScore).toBe(0); @@ -915,9 +932,9 @@ describe("parseFocusManifest gate config", () => { }); it("parses the gate section from YAML content", () => { - const m = parseFocusManifestContent("gate:\n duplicates: block\n readiness:\n mode: block\n minScore: 80\n", "repo_file"); + const m = parseFocusManifestContent("gate:\n duplicates: block\n readiness:\n mode: advisory\n minScore: 80\n", "repo_file"); expect(m.gate.duplicates).toBe("block"); - expect(m.gate.readinessMode).toBe("block"); + expect(m.gate.readinessMode).toBe("advisory"); expect(m.gate.readinessMinScore).toBe(80); }); @@ -1072,6 +1089,20 @@ describe("parseFocusManifest settings override + resolveEffectiveSettings", () = expect(parseFocusManifest({ settings: { commentMode: "off" } }).present).toBe(true); }); + it("downgrades settings.qualityGateMode: block to advisory with a deprecation warning, same as gate.readiness.mode (#2267)", () => { + // The generic settings: override is the SAME dashboard/API-facing qualityGateMode field, read through a + // different manifest path than gate.readiness.mode — it must get the identical downgrade, not just a + // "must be one of" pass-through, or a maintainer using this path keeps the false-enforcement belief. + const m = parseFocusManifest({ settings: { qualityGateMode: "block" } }); + expect(m.settings.qualityGateMode).toBe("advisory"); + expect(m.warnings.some((w) => /settings\.qualityGateMode.*no longer accepts "block"/.test(w))).toBe(true); + // Genuinely invalid values still take the ORIGINAL "must be one of" warning path, unchanged. + const bad = parseFocusManifest({ settings: { qualityGateMode: "sometimes" } }); + expect(bad.settings.qualityGateMode).toBeUndefined(); + expect(bad.warnings.some((w) => /settings\.qualityGateMode.*must be one of/.test(w))).toBe(true); + expect(bad.warnings.some((w) => /no longer accepts "block"/.test(w))).toBe(false); + }); + it("round-trips settings through settingsOverrideToJson and serializes empty as null", () => { const original = parseFocusManifest({ settings: { commentMode: "all_prs", qualityGateMinScore: 40 } }); const reparsed = parseFocusManifest({ settings: settingsOverrideToJson(original.settings) }); @@ -1152,6 +1183,17 @@ describe("parseFocusManifest settings override + resolveEffectiveSettings", () = ); expect(eff.linkedIssueGateMode).toBe("block"); }); + + it("REGRESSION: downgrades a pre-existing DB qualityGateMode: block to advisory, even with no gate.readiness.mode override (#2267)", () => { + // Simulates a repo whose DB row already has quality_gate_mode = "block" from before the write-time guards + // (the settings.qualityGateMode parser, the settings-write API routes) existed — the dashboard/API path's + // "still survives" loophole this resolver-level guard closes for good, regardless of source or vintage. + const db = { qualityGateMode: "block" } as unknown as RepositorySettings; + expect(resolveEffectiveSettings(db, parseFocusManifest(null)).qualityGateMode).toBe("advisory"); + // A non-"block" value is untouched — the downgrade only ever fires for "block". + const dbAdvisory = { qualityGateMode: "advisory" } as unknown as RepositorySettings; + expect(resolveEffectiveSettings(dbAdvisory, parseFocusManifest(null)).qualityGateMode).toBe("advisory"); + }); }); describe("parseFocusManifest review config", () => { diff --git a/test/unit/gate-check-policy.test.ts b/test/unit/gate-check-policy.test.ts index 54b21fa7a3..64e74eee6f 100644 --- a/test/unit/gate-check-policy.test.ts +++ b/test/unit/gate-check-policy.test.ts @@ -50,12 +50,14 @@ describe(".gittensory.yml settings override (resolveEffectiveSettings)", () => { it("overlays the friendly gate: alias over DB settings (incl. gate.enabled -> gateCheckMode)", () => { const eff = resolveEffectiveSettings( settings({ gateCheckMode: "enabled", linkedIssueGateMode: "advisory", duplicatePrGateMode: "block", qualityGateMode: "off", qualityGateMinScore: 10 }), + // readiness.mode: "block" is downgraded to "advisory" at parse time (#2267) — readiness/quality can + // never hard-block, so this exercises the SAME downgrade flowing through resolveEffectiveSettings. parseFocusManifest({ gate: { enabled: false, linkedIssue: "block", duplicates: "off", readiness: { mode: "block", minScore: 70 } } }), ); expect(eff.gateCheckMode).toBe("off"); // gate.enabled: false disables from config expect(eff.linkedIssueGateMode).toBe("block"); expect(eff.duplicatePrGateMode).toBe("off"); - expect(eff.qualityGateMode).toBe("block"); + expect(eff.qualityGateMode).toBe("advisory"); expect(eff.qualityGateMinScore).toBe(70); });