From 632b36132738c972aec9b45e1564ff4e079d626a Mon Sep 17 00:00:00 2001 From: Jean-Philippe Sirois Date: Wed, 5 Aug 2026 14:29:22 -0300 Subject: [PATCH] feat(gate): let an approved migration clear the schema-drift check The gate blocks a pull request that changes the schema so a person validates the migration. Site now records that validation (Site#3289) and reports it as schemaChange.approved. The gate reads it: a change nobody approved still blocks, an approved one passes. An absent field reads as unapproved, so a Site deployment that predates it keeps blocking rather than dropping the gate for every repo. The check's message named the setting that disables the gate, because that was the only way out when it was written. It now names the tool that clears it, and says an approval does not change a check that has already reported. Co-Authored-By: Claude --- src/gate/schema-change.test.ts | 22 ++++++++++++++++++++++ src/gate/schema-change.ts | 20 ++++++++++++++++---- src/main.ts | 6 +++++- src/reporters/site-api.ts | 6 ++++++ 4 files changed, 49 insertions(+), 5 deletions(-) diff --git a/src/gate/schema-change.test.ts b/src/gate/schema-change.test.ts index 0b31abe..68cc0fa 100644 --- a/src/gate/schema-change.test.ts +++ b/src/gate/schema-change.test.ts @@ -25,6 +25,28 @@ describe("gateSchemaChange", () => { expect(gateSchemaChange({ changed: false })).toBeNull(); }); + it("passes once someone has approved the migration", () => { + expect(gateSchemaChange({ changed: true, approved: true })).toBeNull(); + }); + + it("still blocks a schema change nobody has approved", () => { + expect(gateSchemaChange({ changed: true, approved: false })?.conclusion).toBe( + "failure", + ); + }); + + // An API that predates the field sends no `approved` at all. Reading that as + // approved would drop the gate for every repo on an older deployment. + it("blocks when the API sends no approval field", () => { + expect(gateSchemaChange({ changed: true })?.conclusion).toBe("failure"); + }); + + it("names the tool that clears the gate", () => { + expect(gateSchemaChange({ changed: true })!.message).toContain( + "approve_schema_change", + ); + }); + it("passes when the API returned no schema-change signal", () => { expect(gateSchemaChange(null)).toBeNull(); expect(gateSchemaChange(undefined)).toBeNull(); diff --git a/src/gate/schema-change.ts b/src/gate/schema-change.ts index 8d6de8f..213c078 100644 --- a/src/gate/schema-change.ts +++ b/src/gate/schema-change.ts @@ -11,6 +11,13 @@ import { resolveVerdict } from "./policy.ts"; /** The schema diff the API returns on a run (`runMetadata.schemaChange`). */ export interface SchemaChangeSignal { changed: boolean; + /** + * Whether someone validated this pull request's migration and the schema has + * not moved since (Site#3289). Absent on a Site deployment that predates the + * field, and read as unapproved — reading an absent field as approved would + * drop the gate for every repo on an older API. + */ + approved?: boolean; } export interface SchemaGateResult { @@ -20,20 +27,25 @@ export interface SchemaGateResult { } const MESSAGE = - "This PR changes the database schema — validate the migration before merge. " + - "To stop schema changes from blocking this repo, set the schema-drift check " + - "to warn or off in CI settings."; + "This PR changes the database schema. Review the migration, then call " + + "approve_schema_change({ runId, reason }) and re-run CI; approving does not " + + "change a check that has already reported. To stop schema changes blocking " + + "this repo, set the schema-drift check to warn or off in CI settings."; /** * Decide the schema-change gate from the API's schema diff and the repo policy. * Returns the check outcome when the PR changes the schema and the policy - * surfaces it, or `null` when there is no change or the policy is `off`. + * surfaces it, or `null` when there is no change, the migration is approved, or + * the policy is `off`. */ export function gateSchemaChange( schemaChange: SchemaChangeSignal | null | undefined, config: RepoPolicyConfig = {}, ): SchemaGateResult | null { if (!schemaChange?.changed) return null; + // An approved migration has already had the human eyeball this gate exists to + // force, so it stops blocking without the repo having to soften the policy. + if (schemaChange.approved) return null; const { conclusion, surfaced } = resolveVerdict( { condition: "schema-drift", verdictClass: "finding" }, config, diff --git a/src/main.ts b/src/main.ts index c3b9dab..04d26b6 100644 --- a/src/main.ts +++ b/src/main.ts @@ -289,8 +289,12 @@ async function runInCI( regressedCount: reportContext.comparison?.regressed.length ?? 0, newQueryCount: reportContext.comparison?.newQueries.length ?? 0, indexedNewQueryCount: eligible.length, + // An approved migration is not drift the roll-up should count: a + // person has already validated it, so the condition is satisfied + // rather than softened. schemaChanged: - reportContext.runMetadata?.schemaChange?.changed === true, + reportContext.runMetadata?.schemaChange?.changed === true && + reportContext.runMetadata.schemaChange.approved !== true, untestedDataAccessFileCount: reportContext.testPresenceVerdict?.dataAccessFiles.length ?? 0, regressionThreshold: config.regressionThreshold, diff --git a/src/reporters/site-api.ts b/src/reporters/site-api.ts index 9edb222..f567171 100644 --- a/src/reporters/site-api.ts +++ b/src/reporters/site-api.ts @@ -166,6 +166,12 @@ export interface CiRunMetadata { changed: boolean; operations: Op[]; changes?: NamedSchemaChange[]; + /** + * Whether someone validated this pull request's migration and the schema + * has not moved since (Site#3289). Absent on a Site API that predates the + * field, and read as unapproved so the gate keeps blocking. + */ + approved?: boolean; } | null; }