From db94bc1f749b030f0a1001dc1546d44ec5f04c7c Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 3 Jul 2026 01:28:18 -0700 Subject: [PATCH 1/4] fix(settings): default linked issue gate to advisory repository_settings.linked_issue_gate_mode's Drizzle schema default was "block", drifting from every other layer that already said "advisory" (the application fallback in src/db/repositories.ts, .gittensory.yml.example, the tuning docs, the settings API schema, and the gate-evaluation code's own comment). A missing linked issue must only be advisory unless a repo explicitly opts in, via an explicit linkedIssueGateMode: "block" or the "Require linked issue" toggle. The drift traces to two historical migrations: 0023 introduced the column with DEFAULT 'block', and 0025 later flipped any 'advisory' row back to 'block' for repos with an enabled gate. Migration 0101 backfills the rows those two migrations affected, conservatively: only a row that is currently 'block' AND has no explicit require_linked_issue opt-in is corrected, so a genuine maintainer opt-in is left untouched. Audited every other gate-mode/default setting listed for similar drift (duplicatePrGateMode, qualityGateMode, selfAuthoredLinkedIssueGateMode, slopGateMode, mergeReadinessGateMode, manifestPolicyGateMode, aiReviewMode, closeOwnerAuthors, agentDryRun, autonomyJson, and the label/PR-cap defaults) -- schema.ts, the application fallback, and the documented defaults already agree everywhere else; this was an isolated single-column drift. The separate deterministic linked-issue hard rule (closing a PR that links an INELIGIBLE issue -- owner-assigned, wrong label, etc.) shares no config with linkedIssueGateMode and is unaffected; a new regression test pins that isolation explicitly. --- .gittensory.yml.example | 10 +- apps/gittensory-ui/src/routes/docs.tuning.tsx | 10 +- ...101_fix_linked_issue_gate_mode_default.sql | 17 ++++ src/db/schema.ts | 7 +- test/unit/focus-manifest.test.ts | 16 ++++ test/unit/linked-issue-hard-rules.test.ts | 22 +++++ ...ration-0101-linked-issue-gate-mode.test.ts | 93 +++++++++++++++++++ ...ory-settings-linked-issue-defaults.test.ts | 39 ++++++++ 8 files changed, 207 insertions(+), 7 deletions(-) create mode 100644 migrations/0101_fix_linked_issue_gate_mode_default.sql create mode 100644 test/unit/migration-0101-linked-issue-gate-mode.test.ts create mode 100644 test/unit/repository-settings-linked-issue-defaults.test.ts diff --git a/.gittensory.yml.example b/.gittensory.yml.example index bc269a0bb0..7ef6ae3797 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -101,9 +101,13 @@ gate: # Default: gittensor. pack: gittensor - # Linked-issue gate. off | advisory | block. Default: advisory. - # (If the dashboard "Require linked issue" toggle is on but this is `off`, - # it is auto-promoted to `block`.) + # Linked-issue gate — what happens when a PR has NO linked issue at all. off | advisory | block. + # Default: advisory (a missing issue is surfaced in the review panel but never blocks the gate; issues + # aren't always available, e.g. small/self-evident fixes). Set `block` here, or turn on the dashboard + # "Require linked issue" toggle, to make a missing issue an explicit opt-in blocker. + # (If the dashboard toggle is on but this is still `off`, it is auto-promoted to `block`.) + # This is UNRELATED to closing a PR that links an INELIGIBLE issue (owner-assigned, wrong label, etc.) — + # that is a separate, deterministic rule, not this gate. linkedIssue: advisory # Duplicate-PR gate — detects duplicate/superseding PRs. diff --git a/apps/gittensory-ui/src/routes/docs.tuning.tsx b/apps/gittensory-ui/src/routes/docs.tuning.tsx index 07f4711540..59dafc096f 100644 --- a/apps/gittensory-ui/src/routes/docs.tuning.tsx +++ b/apps/gittensory-ui/src/routes/docs.tuning.tsx @@ -220,9 +220,13 @@ function Tuning() { block.
  • - gate.linkedIssue — linked-issue gate. Default advisory. If the - dashboard "Require linked issue" toggle is on but this is off, it is - auto-promoted to block. + gate.linkedIssue — what happens when a PR has no linked issue at all + . Default advisory (surfaced in the review panel, never blocks — issues + aren't always available). Set block, or turn on the dashboard "Require + linked issue" toggle, to make a missing issue an explicit opt-in blocker (if the toggle is + on but this is still off, it is auto-promoted to block). This is + unrelated to closing a PR that links an ineligible issue (owner-assigned, wrong + label, etc.) — that is a separate, deterministic rule, not this gate.
  • gate.readiness.mode — the PR-quality / merge-readiness score gate. Default{" "} diff --git a/migrations/0101_fix_linked_issue_gate_mode_default.sql b/migrations/0101_fix_linked_issue_gate_mode_default.sql new file mode 100644 index 0000000000..10bd1e6bb9 --- /dev/null +++ b/migrations/0101_fix_linked_issue_gate_mode_default.sql @@ -0,0 +1,17 @@ +-- #selfhost-linked-issue-gate-drift: repository_settings.linked_issue_gate_mode was persisted as 'block' +-- by migration 0023 (its initial ADD COLUMN default) and migration 0025 (a since-superseded "restore" +-- backfill that flipped any 'advisory' row to 'block' for repos with gate_check_mode='enabled'), even +-- though missing a linked issue is only ever supposed to be advisory unless a maintainer explicitly opts +-- into blocking. The application-level fallback (src/db/repositories.ts) and every documented default +-- (.gittensory.yml.example, docs.tuning.tsx, the settings API schema) already say 'advisory' -- only the +-- persisted column value drifted. +-- +-- Conservative: only flips a row that is CURRENTLY 'block' AND has require_linked_issue = 0. A row with +-- require_linked_issue = 1 is left untouched -- that boolean is an explicit maintainer opt-in that +-- resolveEffectiveSettings (src/signals/focus-manifest.ts) itself promotes to 'block' whenever the gate +-- would otherwise be 'off' (#797), so a 'block' value paired with require_linked_issue = 1 already reflects +-- an intentional choice, not the drift this migration exists to correct. +UPDATE repository_settings +SET linked_issue_gate_mode = 'advisory' +WHERE linked_issue_gate_mode = 'block' + AND require_linked_issue = 0; diff --git a/src/db/schema.ts b/src/db/schema.ts index 275e436e15..e54ef503a5 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -52,7 +52,12 @@ export const repositorySettings = sqliteTable("repository_settings", { checkRunDetailLevel: text("check_run_detail_level").notNull().default("minimal"), gateCheckMode: text("gate_check_mode").notNull().default("off"), gatePack: text("gate_pack").notNull().default("gittensor"), - linkedIssueGateMode: text("linked_issue_gate_mode").notNull().default("block"), + // Missing a linked issue is advisory-only by default -- issues aren't always available, so it only + // blocks when a repo explicitly opts in (linkedIssueGateMode: "block" or the requireLinkedIssue toggle; + // see resolveEffectiveSettings in signals/focus-manifest.ts). This default was "block" until #selfhost- + // linked-issue-gate-drift, which also backfills any row an earlier migration (0023/0025) persisted as + // "block" without an explicit opt-in (migrations/0101_fix_linked_issue_gate_mode_default.sql). + linkedIssueGateMode: text("linked_issue_gate_mode").notNull().default("advisory"), duplicatePrGateMode: text("duplicate_pr_gate_mode").notNull().default("block"), qualityGateMode: text("quality_gate_mode").notNull().default("advisory"), qualityGateMinScore: integer("quality_gate_min_score"), diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 4c93854a80..ab399e5b6a 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -1660,6 +1660,22 @@ describe("parseFocusManifest settings override + resolveEffectiveSettings", () = const dbAdvisory = { qualityGateMode: "advisory" } as unknown as RepositorySettings; expect(resolveEffectiveSettings(dbAdvisory, parseFocusManifest(null)).qualityGateMode).toBe("advisory"); }); + + it("REGRESSION: keeps missing-linked-issue advisory when the DB row already says advisory and nothing overrides it (#selfhost-linked-issue-gate-drift)", () => { + const db = { linkedIssueGateMode: "advisory", requireLinkedIssue: false } as unknown as RepositorySettings; + expect(resolveEffectiveSettings(db, parseFocusManifest(null)).linkedIssueGateMode).toBe("advisory"); + }); + + it("does NOT blanket-downgrade a DB linkedIssueGateMode: block to advisory -- unlike qualityGateMode, block is a legitimate opt-in here (#selfhost-linked-issue-gate-drift)", () => { + // Deliberately the OPPOSITE assertion from the qualityGateMode regression above: qualityGateMode can + // NEVER legitimately be "block" (isConfiguredGateBlocker has no branch for it), so resolveEffectiveSettings + // unconditionally downgrades it. linkedIssueGateMode CAN legitimately be "block" -- a maintainer may + // explicitly opt into it -- so migration 0101's data fix (conservative: only rows with + // require_linked_issue=0) is the correct place to correct historically-drifted rows, not a resolver-level + // downgrade that would also silently defeat a real, current opt-in. + const db = { linkedIssueGateMode: "block", requireLinkedIssue: false } as unknown as RepositorySettings; + expect(resolveEffectiveSettings(db, parseFocusManifest(null)).linkedIssueGateMode).toBe("block"); + }); }); describe("parseFocusManifest review config", () => { diff --git a/test/unit/linked-issue-hard-rules.test.ts b/test/unit/linked-issue-hard-rules.test.ts index 707c140e92..e9093d095c 100644 --- a/test/unit/linked-issue-hard-rules.test.ts +++ b/test/unit/linked-issue-hard-rules.test.ts @@ -9,6 +9,8 @@ import { type LinkedIssueFacts, type LinkedIssueHardRulesConfig, } from "../../src/review/linked-issue-hard-rules"; +import { parseFocusManifest, resolveEffectiveSettings } from "../../src/signals/focus-manifest"; +import type { RepositorySettings } from "../../src/types"; function config(overrides: Partial = {}): LinkedIssueHardRulesConfig { return { @@ -341,4 +343,24 @@ describe("resolveLinkedIssueHardRule (#1144 — overflow + orchestration)", () = expect(spy).toHaveBeenCalledWith(expect.anything(), "owner/repo", 7, "installation-token", "installation:143010787"); spy.mockRestore(); }); + + it("REGRESSION: an ineligible (owner-assigned) linked issue still violates the hard rule regardless of linkedIssueGateMode -- the two are fully independent (#selfhost-linked-issue-gate-drift)", () => { + // evaluateLinkedIssueHardRules's own input type (`{ issues, config, repoOwner }`) has no linkedIssueGateMode + // field at all -- it structurally cannot read it. This test pins the END-TO-END behavior: fixing + // linkedIssueGateMode's default to "advisory" (missing-issue is non-blocking by default) must never soften + // or bypass the hard rule for a linked issue that DOES exist but is ineligible (owner-assigned here). + const result = evaluateLinkedIssueHardRules({ + issues: [issue({ number: 9, assignees: ["jsonbored"] })], + config: config({ ownerAssignedClose: "block" }), + repoOwner: OWNER, + }); + expect(result.violated).toBe(true); + expect(result.reason).toContain("#9"); + + // The gate-mode side, evaluated completely separately: a repo with no explicit override now resolves + // linkedIssueGateMode to "advisory" (the fixed default) -- confirming the fix under test is live -- while + // the hard-rule violation above is computed independently and is unaffected by it either way. + const db = { linkedIssueGateMode: "advisory", requireLinkedIssue: false } as unknown as RepositorySettings; + expect(resolveEffectiveSettings(db, parseFocusManifest(null)).linkedIssueGateMode).toBe("advisory"); + }); }); diff --git a/test/unit/migration-0101-linked-issue-gate-mode.test.ts b/test/unit/migration-0101-linked-issue-gate-mode.test.ts new file mode 100644 index 0000000000..184aa8de32 --- /dev/null +++ b/test/unit/migration-0101-linked-issue-gate-mode.test.ts @@ -0,0 +1,93 @@ +import { readFileSync, readdirSync } from "node:fs"; +import { DatabaseSync } from "node:sqlite"; +import { describe, expect, it } from "vitest"; + +const MIGRATION_FILE = "0101_fix_linked_issue_gate_mode_default.sql"; + +// Replays every migrations/*.sql file BEFORE 0101 into a fresh in-memory DB (mirrors +// scripts/check-schema-drift.mjs's own "replay migrations into node:sqlite" approach), so the table shape +// this test inserts into is exactly what migration 0101 itself was written against -- not a guess. The +// TestD1Database helper (test/helpers/d1.ts) can't be reused here: it concatenates and applies EVERY +// migration (including 0101) up front, so the `repository_settings` table would already be empty-and-fixed +// by the time a test could insert a "bad state" row -- there would be nothing left for 0101 to correct. +function applyMigrationsBefore(cutoffFile: string): DatabaseSync { + const db = new DatabaseSync(":memory:"); + const files = readdirSync("migrations") + .filter((file) => file.endsWith(".sql") && file < cutoffFile) + .sort(); + for (const file of files) db.exec(readFileSync(`migrations/${file}`, "utf8")); + return db; +} + +function applyMigration(db: DatabaseSync, file: string): void { + db.exec(readFileSync(`migrations/${file}`, "utf8")); +} + +function insertRepositorySettingsRow( + db: DatabaseSync, + repoFullName: string, + linkedIssueGateMode: string, + requireLinkedIssue: 0 | 1, +): void { + db.prepare( + "INSERT INTO repository_settings (repo_full_name, linked_issue_gate_mode, require_linked_issue) VALUES (?, ?, ?)", + ).run(repoFullName, linkedIssueGateMode, requireLinkedIssue); +} + +function readLinkedIssueGateMode(db: DatabaseSync, repoFullName: string): string { + const row = db + .prepare("SELECT linked_issue_gate_mode FROM repository_settings WHERE repo_full_name = ?") + .get(repoFullName) as { linked_issue_gate_mode: string } | undefined; + if (!row) throw new Error(`no repository_settings row for ${repoFullName}`); + return row.linked_issue_gate_mode; +} + +describe("migration 0101: fix linked_issue_gate_mode default drift (#selfhost-linked-issue-gate-drift)", () => { + it("flips a 'block' row with no explicit require_linked_issue opt-in to 'advisory'", () => { + const db = applyMigrationsBefore(MIGRATION_FILE); + insertRepositorySettingsRow(db, "acme/drifted-repo", "block", 0); + + applyMigration(db, MIGRATION_FILE); + + expect(readLinkedIssueGateMode(db, "acme/drifted-repo")).toBe("advisory"); + }); + + it("leaves a 'block' row alone when require_linked_issue is an explicit maintainer opt-in", () => { + const db = applyMigrationsBefore(MIGRATION_FILE); + insertRepositorySettingsRow(db, "acme/explicit-opt-in", "block", 1); + + applyMigration(db, MIGRATION_FILE); + + expect(readLinkedIssueGateMode(db, "acme/explicit-opt-in")).toBe("block"); + }); + + it("leaves an already-advisory row unchanged", () => { + const db = applyMigrationsBefore(MIGRATION_FILE); + insertRepositorySettingsRow(db, "acme/already-advisory", "advisory", 0); + + applyMigration(db, MIGRATION_FILE); + + expect(readLinkedIssueGateMode(db, "acme/already-advisory")).toBe("advisory"); + }); + + it("leaves an 'off' row unchanged (not a drifted value at all)", () => { + const db = applyMigrationsBefore(MIGRATION_FILE); + insertRepositorySettingsRow(db, "acme/gate-off", "off", 0); + + applyMigration(db, MIGRATION_FILE); + + expect(readLinkedIssueGateMode(db, "acme/gate-off")).toBe("off"); + }); + + it("is idempotent -- running it a second time changes nothing further", () => { + const db = applyMigrationsBefore(MIGRATION_FILE); + insertRepositorySettingsRow(db, "acme/drifted-repo", "block", 0); + insertRepositorySettingsRow(db, "acme/explicit-opt-in", "block", 1); + + applyMigration(db, MIGRATION_FILE); + applyMigration(db, MIGRATION_FILE); + + expect(readLinkedIssueGateMode(db, "acme/drifted-repo")).toBe("advisory"); + expect(readLinkedIssueGateMode(db, "acme/explicit-opt-in")).toBe("block"); + }); +}); diff --git a/test/unit/repository-settings-linked-issue-defaults.test.ts b/test/unit/repository-settings-linked-issue-defaults.test.ts new file mode 100644 index 0000000000..3b3ac6591d --- /dev/null +++ b/test/unit/repository-settings-linked-issue-defaults.test.ts @@ -0,0 +1,39 @@ +import { describe, expect, it } from "vitest"; +import { getRepositorySettings, upsertRepositorySettings } from "../../src/db/repositories"; +import { createTestEnv } from "../helpers/d1"; + +// #selfhost-linked-issue-gate-drift: repository_settings.linked_issue_gate_mode was persisted as 'block' in +// production for repos that never explicitly opted into it (migrations/0101_fix_linked_issue_gate_mode_default.sql +// backfills the historically-drifted rows). These regression tests pin the two paths that must default to +// 'advisory' going forward: a brand-new row (no DB row yet) and an explicit upsert that omits the field. +describe("repository_settings: linked-issue gate defaults to advisory, not block (#selfhost-linked-issue-gate-drift)", () => { + it("getRepositorySettings returns advisory for a repo with no DB row at all", async () => { + const env = createTestEnv(); + const settings = await getRepositorySettings(env, "acme/brand-new-repo"); + expect(settings.linkedIssueGateMode).toBe("advisory"); + expect(settings.requireLinkedIssue).toBe(false); + }); + + it("upsertRepositorySettings persists advisory when the caller omits linkedIssueGateMode entirely", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/omits-gate-mode" }); + const settings = await getRepositorySettings(env, "acme/omits-gate-mode"); + expect(settings.linkedIssueGateMode).toBe("advisory"); + }); + + it("an explicit block opt-in is persisted and read back as block -- advisory-by-default does not clobber a real opt-in", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/explicit-block", linkedIssueGateMode: "block" }); + const settings = await getRepositorySettings(env, "acme/explicit-block"); + expect(settings.linkedIssueGateMode).toBe("block"); + }); + + it("re-upserting without specifying linkedIssueGateMode keeps the row at its previously-set value (upsert defaults only apply when the field is omitted from the settings object, not merged against the existing row)", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/round-trip", linkedIssueGateMode: "block" }); + const settings = await getRepositorySettings(env, "acme/round-trip"); + await upsertRepositorySettings(env, { ...settings, repoFullName: "acme/round-trip" }); + const after = await getRepositorySettings(env, "acme/round-trip"); + expect(after.linkedIssueGateMode).toBe("block"); + }); +}); From f089b0e78fc53187f2c1deca1f242bec9f46bf85 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 3 Jul 2026 02:47:53 -0700 Subject: [PATCH 2/4] fix(settings): narrow migration 0101 backfill to provably drifted rows require_linked_issue = 0 alone doesn't prove linked_issue_gate_mode = 'block' was default drift: the two fields are independently settable from the maintainer settings UI, so a maintainer can genuinely choose 'block' while leaving require_linked_issue off. Narrow the backfill to rows where updated_at still equals created_at -- the only rows that have never been written to since creation, and therefore can only hold a migration-era default, not a reviewed or reaffirmed choice. Renumbered to 0102 after a numbering collision with an already-merged migration. --- ...101_fix_linked_issue_gate_mode_default.sql | 17 --------- ...102_fix_linked_issue_gate_mode_default.sql | 29 +++++++++++++++ src/db/schema.ts | 2 +- ...ation-0102-linked-issue-gate-mode.test.ts} | 35 +++++++++++++++---- ...ory-settings-linked-issue-defaults.test.ts | 2 +- 5 files changed, 60 insertions(+), 25 deletions(-) delete mode 100644 migrations/0101_fix_linked_issue_gate_mode_default.sql create mode 100644 migrations/0102_fix_linked_issue_gate_mode_default.sql rename test/unit/{migration-0101-linked-issue-gate-mode.test.ts => migration-0102-linked-issue-gate-mode.test.ts} (66%) diff --git a/migrations/0101_fix_linked_issue_gate_mode_default.sql b/migrations/0101_fix_linked_issue_gate_mode_default.sql deleted file mode 100644 index 10bd1e6bb9..0000000000 --- a/migrations/0101_fix_linked_issue_gate_mode_default.sql +++ /dev/null @@ -1,17 +0,0 @@ --- #selfhost-linked-issue-gate-drift: repository_settings.linked_issue_gate_mode was persisted as 'block' --- by migration 0023 (its initial ADD COLUMN default) and migration 0025 (a since-superseded "restore" --- backfill that flipped any 'advisory' row to 'block' for repos with gate_check_mode='enabled'), even --- though missing a linked issue is only ever supposed to be advisory unless a maintainer explicitly opts --- into blocking. The application-level fallback (src/db/repositories.ts) and every documented default --- (.gittensory.yml.example, docs.tuning.tsx, the settings API schema) already say 'advisory' -- only the --- persisted column value drifted. --- --- Conservative: only flips a row that is CURRENTLY 'block' AND has require_linked_issue = 0. A row with --- require_linked_issue = 1 is left untouched -- that boolean is an explicit maintainer opt-in that --- resolveEffectiveSettings (src/signals/focus-manifest.ts) itself promotes to 'block' whenever the gate --- would otherwise be 'off' (#797), so a 'block' value paired with require_linked_issue = 1 already reflects --- an intentional choice, not the drift this migration exists to correct. -UPDATE repository_settings -SET linked_issue_gate_mode = 'advisory' -WHERE linked_issue_gate_mode = 'block' - AND require_linked_issue = 0; diff --git a/migrations/0102_fix_linked_issue_gate_mode_default.sql b/migrations/0102_fix_linked_issue_gate_mode_default.sql new file mode 100644 index 0000000000..3a8c6c2bf5 --- /dev/null +++ b/migrations/0102_fix_linked_issue_gate_mode_default.sql @@ -0,0 +1,29 @@ +-- #selfhost-linked-issue-gate-drift: repository_settings.linked_issue_gate_mode was persisted as 'block' +-- by migration 0023 (its initial ADD COLUMN default) and migration 0025 (a since-superseded "restore" +-- backfill that flipped any 'advisory' row to 'block' for repos with gate_check_mode='enabled'), even +-- though missing a linked issue is only ever supposed to be advisory unless a maintainer explicitly opts +-- into blocking. The application-level fallback (src/db/repositories.ts) and every documented default +-- (.gittensory.yml.example, docs.tuning.tsx, the settings API schema) already say 'advisory' -- only the +-- persisted column value drifted. +-- +-- #gate-review-2727: require_linked_issue = 0 alone does NOT prove drift. linkedIssueGateMode and +-- requireLinkedIssue are independently settable (the maintainer settings UI exposes them as a separate +-- dropdown and toggle; both PUT .../settings and the internal settings route accept linkedIssueGateMode on +-- its own), so a maintainer can genuinely choose 'block' while leaving requireLinkedIssue off. No column on +-- this row records which field a maintainer last touched, so per-field intent can't be recovered. +-- +-- What CAN be proven: updated_at is bumped on every write through upsertRepositorySettings +-- (src/db/repositories.ts) -- the only code path that ever changes this row after INSERT -- while +-- created_at is set once, at INSERT, and never touched again. updated_at = created_at therefore means this +-- row has never been written to since it was first created: no settings save, by anyone, has ever happened +-- for this repo. Its 'block' value can only be the byproduct of migration 0023's column default or 0025's +-- blanket flip -- provable drift, not inference from an unrelated field. A row with updated_at > created_at +-- has been through at least one real settings write since creation and is left alone, even though some of +-- those may also be untouched drift this migration can no longer safely reach -- a maintainer stuck with a +-- leftover 'block' default can flip it from the settings UI. Leaving that row alone is the safe failure +-- mode; silently downgrading a real 'block' opt-in to advisory is not. +UPDATE repository_settings +SET linked_issue_gate_mode = 'advisory' +WHERE linked_issue_gate_mode = 'block' + AND require_linked_issue = 0 + AND updated_at = created_at; diff --git a/src/db/schema.ts b/src/db/schema.ts index e54ef503a5..61321fad4d 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -56,7 +56,7 @@ export const repositorySettings = sqliteTable("repository_settings", { // blocks when a repo explicitly opts in (linkedIssueGateMode: "block" or the requireLinkedIssue toggle; // see resolveEffectiveSettings in signals/focus-manifest.ts). This default was "block" until #selfhost- // linked-issue-gate-drift, which also backfills any row an earlier migration (0023/0025) persisted as - // "block" without an explicit opt-in (migrations/0101_fix_linked_issue_gate_mode_default.sql). + // "block" without an explicit opt-in (migrations/0102_fix_linked_issue_gate_mode_default.sql). linkedIssueGateMode: text("linked_issue_gate_mode").notNull().default("advisory"), duplicatePrGateMode: text("duplicate_pr_gate_mode").notNull().default("block"), qualityGateMode: text("quality_gate_mode").notNull().default("advisory"), diff --git a/test/unit/migration-0101-linked-issue-gate-mode.test.ts b/test/unit/migration-0102-linked-issue-gate-mode.test.ts similarity index 66% rename from test/unit/migration-0101-linked-issue-gate-mode.test.ts rename to test/unit/migration-0102-linked-issue-gate-mode.test.ts index 184aa8de32..871d4329aa 100644 --- a/test/unit/migration-0101-linked-issue-gate-mode.test.ts +++ b/test/unit/migration-0102-linked-issue-gate-mode.test.ts @@ -2,11 +2,11 @@ import { readFileSync, readdirSync } from "node:fs"; import { DatabaseSync } from "node:sqlite"; import { describe, expect, it } from "vitest"; -const MIGRATION_FILE = "0101_fix_linked_issue_gate_mode_default.sql"; +const MIGRATION_FILE = "0102_fix_linked_issue_gate_mode_default.sql"; -// Replays every migrations/*.sql file BEFORE 0101 into a fresh in-memory DB (mirrors +// Replays every migrations/*.sql file BEFORE 0102 into a fresh in-memory DB (mirrors // scripts/check-schema-drift.mjs's own "replay migrations into node:sqlite" approach), so the table shape -// this test inserts into is exactly what migration 0101 itself was written against -- not a guess. The +// this test inserts into is exactly what migration 0102 itself was written against -- not a guess. The // TestD1Database helper (test/helpers/d1.ts) can't be reused here: it concatenates and applies EVERY // migration (including 0101) up front, so the `repository_settings` table would already be empty-and-fixed // by the time a test could insert a "bad state" row -- there would be nothing left for 0101 to correct. @@ -23,15 +23,25 @@ function applyMigration(db: DatabaseSync, file: string): void { db.exec(readFileSync(`migrations/${file}`, "utf8")); } +// created_at/updated_at are set explicitly (not left to the column's CURRENT_TIMESTAMP default) so a +// "touched since creation" row can be simulated deterministically: migration 0101 only flips a row whose +// updated_at still equals its created_at (see the migration's own header comment for why). function insertRepositorySettingsRow( db: DatabaseSync, repoFullName: string, linkedIssueGateMode: string, requireLinkedIssue: 0 | 1, + touchedSinceCreation = false, ): void { db.prepare( - "INSERT INTO repository_settings (repo_full_name, linked_issue_gate_mode, require_linked_issue) VALUES (?, ?, ?)", + "INSERT INTO repository_settings (repo_full_name, linked_issue_gate_mode, require_linked_issue, created_at, updated_at) VALUES (?, ?, ?, '2026-01-01T00:00:00.000Z', '2026-01-01T00:00:00.000Z')", ).run(repoFullName, linkedIssueGateMode, requireLinkedIssue); + if (touchedSinceCreation) { + db.prepare("UPDATE repository_settings SET updated_at = ? WHERE repo_full_name = ?").run( + "2026-02-01T00:00:00.000Z", + repoFullName, + ); + } } function readLinkedIssueGateMode(db: DatabaseSync, repoFullName: string): string { @@ -42,8 +52,8 @@ function readLinkedIssueGateMode(db: DatabaseSync, repoFullName: string): string return row.linked_issue_gate_mode; } -describe("migration 0101: fix linked_issue_gate_mode default drift (#selfhost-linked-issue-gate-drift)", () => { - it("flips a 'block' row with no explicit require_linked_issue opt-in to 'advisory'", () => { +describe("migration 0102: fix linked_issue_gate_mode default drift (#selfhost-linked-issue-gate-drift)", () => { + it("flips a 'block' row that has never been written to since creation to 'advisory'", () => { const db = applyMigrationsBefore(MIGRATION_FILE); insertRepositorySettingsRow(db, "acme/drifted-repo", "block", 0); @@ -61,6 +71,19 @@ describe("migration 0101: fix linked_issue_gate_mode default drift (#selfhost-li expect(readLinkedIssueGateMode(db, "acme/explicit-opt-in")).toBe("block"); }); + // #gate-review-2727: the exact scenario the reviewer flagged -- a maintainer who chose 'block' from the + // settings UI's "Linked issue" dropdown without also turning on the separate "Require a linked issue" + // toggle. require_linked_issue = 0 alone can't distinguish this from drift; updated_at > created_at can, + // because it proves a real settings write happened after the row was created. + it("leaves a 'block' row alone when it has been saved since creation, even with require_linked_issue = 0", () => { + const db = applyMigrationsBefore(MIGRATION_FILE); + insertRepositorySettingsRow(db, "acme/explicit-block-no-require", "block", 0, true); + + applyMigration(db, MIGRATION_FILE); + + expect(readLinkedIssueGateMode(db, "acme/explicit-block-no-require")).toBe("block"); + }); + it("leaves an already-advisory row unchanged", () => { const db = applyMigrationsBefore(MIGRATION_FILE); insertRepositorySettingsRow(db, "acme/already-advisory", "advisory", 0); diff --git a/test/unit/repository-settings-linked-issue-defaults.test.ts b/test/unit/repository-settings-linked-issue-defaults.test.ts index 3b3ac6591d..ad9fe373cf 100644 --- a/test/unit/repository-settings-linked-issue-defaults.test.ts +++ b/test/unit/repository-settings-linked-issue-defaults.test.ts @@ -3,7 +3,7 @@ import { getRepositorySettings, upsertRepositorySettings } from "../../src/db/re import { createTestEnv } from "../helpers/d1"; // #selfhost-linked-issue-gate-drift: repository_settings.linked_issue_gate_mode was persisted as 'block' in -// production for repos that never explicitly opted into it (migrations/0101_fix_linked_issue_gate_mode_default.sql +// production for repos that never explicitly opted into it (migrations/0102_fix_linked_issue_gate_mode_default.sql // backfills the historically-drifted rows). These regression tests pin the two paths that must default to // 'advisory' going forward: a brand-new row (no DB row yet) and an explicit upsert that omits the field. describe("repository_settings: linked-issue gate defaults to advisory, not block (#selfhost-linked-issue-gate-drift)", () => { From f817a6def35e0b9298c8b66f9d963bf41ed92e95 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 3 Jul 2026 03:09:05 -0700 Subject: [PATCH 3/4] fix(settings): also repair rows that predate the linked_issue_gate_mode column updated_at = created_at alone missed a real class of drifted rows: ones last written before migration 0023 added the column, which have updated_at > created_at but still could not possibly have chosen 'block' since the column didn't exist yet. Add a second, independently provable path -- updated_at at or before the instant 0023 deployed (proxied by its merge commit time, which is always at or before the real remote migration apply time) -- covering those rows without touching any row genuinely written after the column existed. Also fixes stale 0101 references left over from the migration renumbering. --- ...102_fix_linked_issue_gate_mode_default.sql | 33 ++++++++++----- test/unit/focus-manifest.test.ts | 6 +-- ...ration-0102-linked-issue-gate-mode.test.ts | 40 ++++++++++++++++--- 3 files changed, 62 insertions(+), 17 deletions(-) diff --git a/migrations/0102_fix_linked_issue_gate_mode_default.sql b/migrations/0102_fix_linked_issue_gate_mode_default.sql index 3a8c6c2bf5..03c82db185 100644 --- a/migrations/0102_fix_linked_issue_gate_mode_default.sql +++ b/migrations/0102_fix_linked_issue_gate_mode_default.sql @@ -6,24 +6,39 @@ -- (.gittensory.yml.example, docs.tuning.tsx, the settings API schema) already say 'advisory' -- only the -- persisted column value drifted. -- --- #gate-review-2727: require_linked_issue = 0 alone does NOT prove drift. linkedIssueGateMode and +-- #gate-review-2727 round 1: require_linked_issue = 0 alone does NOT prove drift. linkedIssueGateMode and -- requireLinkedIssue are independently settable (the maintainer settings UI exposes them as a separate -- dropdown and toggle; both PUT .../settings and the internal settings route accept linkedIssueGateMode on -- its own), so a maintainer can genuinely choose 'block' while leaving requireLinkedIssue off. No column on --- this row records which field a maintainer last touched, so per-field intent can't be recovered. +-- this row records which field a maintainer last touched, so per-field intent can't be recovered by itself. -- --- What CAN be proven: updated_at is bumped on every write through upsertRepositorySettings +-- What CAN be proven, path 1: updated_at is bumped on every write through upsertRepositorySettings -- (src/db/repositories.ts) -- the only code path that ever changes this row after INSERT -- while -- created_at is set once, at INSERT, and never touched again. updated_at = created_at therefore means this -- row has never been written to since it was first created: no settings save, by anyone, has ever happened -- for this repo. Its 'block' value can only be the byproduct of migration 0023's column default or 0025's --- blanket flip -- provable drift, not inference from an unrelated field. A row with updated_at > created_at --- has been through at least one real settings write since creation and is left alone, even though some of --- those may also be untouched drift this migration can no longer safely reach -- a maintainer stuck with a --- leftover 'block' default can flip it from the settings UI. Leaving that row alone is the safe failure --- mode; silently downgrading a real 'block' opt-in to advisory is not. +-- blanket flip -- provable drift, not inference from an unrelated field. +-- +-- #gate-review-2727 round 2: path 1 alone is too narrow -- a row last written to BEFORE migration 0023 ran +-- (updated_at > created_at, but that write predates 0023) has updated_at != created_at and was skipped, even +-- though that write could not possibly have set linked_issue_gate_mode: the column did not exist yet. That +-- is an equally provable drift signal, just anchored to a different fixed point. +-- +-- What CAN be proven, path 2: migration 0023 was deployed at commit 66e4dd6b (2026-06-05T14:01:39-06:00 = +-- 2026-06-05T20:01:39Z; this repo auto-deploys `wrangler d1 migrations apply --remote` on every merge to +-- main, so the merge commit time is a same-day, conservatively-early proxy for the real remote apply time -- +-- true apply is always slightly AFTER this timestamp, never before, so using it as an inclusive upper bound +-- can only under-select rows, never wrongly include a row genuinely written after the column existed). A row +-- whose updated_at is at or before that instant was last written before linked_issue_gate_mode existed at +-- all, so its current value is provably the column-add default (or 0025's later blanket flip), not a choice. +-- +-- A row with updated_at > created_at AND updated_at after that cutoff has been through at least one real +-- settings write since the column existed and is left alone, even though some of those may also be +-- untouched drift this migration can no longer safely reach -- a maintainer stuck with a leftover 'block' +-- default can flip it from the settings UI. Leaving that row alone is the safe failure mode; silently +-- downgrading a real 'block' opt-in to advisory is not. UPDATE repository_settings SET linked_issue_gate_mode = 'advisory' WHERE linked_issue_gate_mode = 'block' AND require_linked_issue = 0 - AND updated_at = created_at; + AND (updated_at = created_at OR updated_at <= '2026-06-05T20:01:39.000Z'); diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index ab399e5b6a..4b1b2e4e60 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -1670,9 +1670,9 @@ describe("parseFocusManifest settings override + resolveEffectiveSettings", () = // Deliberately the OPPOSITE assertion from the qualityGateMode regression above: qualityGateMode can // NEVER legitimately be "block" (isConfiguredGateBlocker has no branch for it), so resolveEffectiveSettings // unconditionally downgrades it. linkedIssueGateMode CAN legitimately be "block" -- a maintainer may - // explicitly opt into it -- so migration 0101's data fix (conservative: only rows with - // require_linked_issue=0) is the correct place to correct historically-drifted rows, not a resolver-level - // downgrade that would also silently defeat a real, current opt-in. + // explicitly opt into it -- so migration 0102's data fix (conservative: only provably-drifted rows) is + // the correct place to correct historically-drifted rows, not a resolver-level downgrade that would also + // silently defeat a real, current opt-in. const db = { linkedIssueGateMode: "block", requireLinkedIssue: false } as unknown as RepositorySettings; expect(resolveEffectiveSettings(db, parseFocusManifest(null)).linkedIssueGateMode).toBe("block"); }); diff --git a/test/unit/migration-0102-linked-issue-gate-mode.test.ts b/test/unit/migration-0102-linked-issue-gate-mode.test.ts index 871d4329aa..a363b3a5de 100644 --- a/test/unit/migration-0102-linked-issue-gate-mode.test.ts +++ b/test/unit/migration-0102-linked-issue-gate-mode.test.ts @@ -8,8 +8,8 @@ const MIGRATION_FILE = "0102_fix_linked_issue_gate_mode_default.sql"; // scripts/check-schema-drift.mjs's own "replay migrations into node:sqlite" approach), so the table shape // this test inserts into is exactly what migration 0102 itself was written against -- not a guess. The // TestD1Database helper (test/helpers/d1.ts) can't be reused here: it concatenates and applies EVERY -// migration (including 0101) up front, so the `repository_settings` table would already be empty-and-fixed -// by the time a test could insert a "bad state" row -- there would be nothing left for 0101 to correct. +// migration (including 0102) up front, so the `repository_settings` table would already be empty-and-fixed +// by the time a test could insert a "bad state" row -- there would be nothing left for 0102 to correct. function applyMigrationsBefore(cutoffFile: string): DatabaseSync { const db = new DatabaseSync(":memory:"); const files = readdirSync("migrations") @@ -24,8 +24,13 @@ function applyMigration(db: DatabaseSync, file: string): void { } // created_at/updated_at are set explicitly (not left to the column's CURRENT_TIMESTAMP default) so a -// "touched since creation" row can be simulated deterministically: migration 0101 only flips a row whose -// updated_at still equals its created_at (see the migration's own header comment for why). +// "touched since creation" row can be simulated deterministically. Migration 0102 flips a row whose +// updated_at still equals its created_at, OR whose updated_at is at/before the instant migration 0023 +// deployed (2026-06-05T20:01:39.000Z, see the migration's own header comment for why) -- so a "touched" +// fixture must land AFTER that cutoff to exercise the "genuinely left alone" path instead of accidentally +// also satisfying the pre-column-existence path. +const AFTER_0023_CUTOFF = "2026-07-01T00:00:00.000Z"; + function insertRepositorySettingsRow( db: DatabaseSync, repoFullName: string, @@ -38,12 +43,19 @@ function insertRepositorySettingsRow( ).run(repoFullName, linkedIssueGateMode, requireLinkedIssue); if (touchedSinceCreation) { db.prepare("UPDATE repository_settings SET updated_at = ? WHERE repo_full_name = ?").run( - "2026-02-01T00:00:00.000Z", + AFTER_0023_CUTOFF, repoFullName, ); } } +function applyMigrationsInRange(db: DatabaseSync, fromFileInclusive: string, throughFileInclusive: string): void { + const files = readdirSync("migrations") + .filter((file) => file.endsWith(".sql") && file >= fromFileInclusive && file <= throughFileInclusive) + .sort(); + for (const file of files) db.exec(readFileSync(`migrations/${file}`, "utf8")); +} + function readLinkedIssueGateMode(db: DatabaseSync, repoFullName: string): string { const row = db .prepare("SELECT linked_issue_gate_mode FROM repository_settings WHERE repo_full_name = ?") @@ -84,6 +96,24 @@ describe("migration 0102: fix linked_issue_gate_mode default drift (#selfhost-li expect(readLinkedIssueGateMode(db, "acme/explicit-block-no-require")).toBe("block"); }); + // #gate-review-2727 round 2: a row last written to BEFORE migration 0023 ran has updated_at != created_at, + // so it fails the "never touched" path -- but that write could not possibly have set + // linked_issue_gate_mode, because the column did not exist yet. Proves this via the REAL migration + // sequence (0023's ADD COLUMN DEFAULT 'block' backfills the pre-existing row, exactly like production), + // not a hand-inserted 'block' row. + it("repairs a row that predates the linked_issue_gate_mode column entirely, via the real migration sequence", () => { + const db = applyMigrationsBefore("0023_gate_quality_modes.sql"); + db.prepare("INSERT INTO repository_settings (repo_full_name, created_at, updated_at) VALUES (?, ?, ?)").run( + "acme/pre-column-repo", + "2026-01-01T00:00:00.000Z", + "2026-03-01T00:00:00.000Z", + ); + + applyMigrationsInRange(db, "0023_gate_quality_modes.sql", MIGRATION_FILE); + + expect(readLinkedIssueGateMode(db, "acme/pre-column-repo")).toBe("advisory"); + }); + it("leaves an already-advisory row unchanged", () => { const db = applyMigrationsBefore(MIGRATION_FILE); insertRepositorySettingsRow(db, "acme/already-advisory", "advisory", 0); From 5ec41a8309dfb1b551c3a9222ae63a781c9cb1c8 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 3 Jul 2026 03:32:15 -0700 Subject: [PATCH 4/4] fix(settings): anchor the linked-issue-gate backfill on the column's own deploy time The updated_at = created_at signal could not tell a genuinely untouched drifted row apart from a repo whose very first settings save explicitly chose 'block' (a single INSERT sets both timestamps to the same instant either way). Replace it with the single condition already partially in use for the pre-0023 case: updated_at at or before the instant migration 0023 deployed. Since upsertRepositorySettings is the only writer and always resolves an explicit value, any row untouched since before the column existed can only hold a migration-era default, while any write after that point -- first save or later -- reflects a real, resolvable choice. Also documents why the raw SQLite column-level default is intentionally left at 'block' (no ALTER COLUMN SET DEFAULT in SQLite, no live path ever reaches it) and pins that behavior with a regression test, and fixes a JSX whitespace bug in the tuning docs. --- apps/gittensory-ui/src/routes/docs.tuning.tsx | 2 +- ...102_fix_linked_issue_gate_mode_default.sql | 59 +++++++++++-------- src/db/schema.ts | 6 +- ...ration-0102-linked-issue-gate-mode.test.ts | 45 +++++++++----- test/unit/schema-timestamp-defaults.test.ts | 6 ++ 5 files changed, 79 insertions(+), 39 deletions(-) diff --git a/apps/gittensory-ui/src/routes/docs.tuning.tsx b/apps/gittensory-ui/src/routes/docs.tuning.tsx index 59dafc096f..27de1f1853 100644 --- a/apps/gittensory-ui/src/routes/docs.tuning.tsx +++ b/apps/gittensory-ui/src/routes/docs.tuning.tsx @@ -221,7 +221,7 @@ function Tuning() {
  • gate.linkedIssue — what happens when a PR has no linked issue at all - . Default advisory (surfaced in the review panel, never blocks — issues + {". "}Default advisory (surfaced in the review panel, never blocks — issues aren't always available). Set block, or turn on the dashboard "Require linked issue" toggle, to make a missing issue an explicit opt-in blocker (if the toggle is on but this is still off, it is auto-promoted to block). This is diff --git a/migrations/0102_fix_linked_issue_gate_mode_default.sql b/migrations/0102_fix_linked_issue_gate_mode_default.sql index 03c82db185..238a485557 100644 --- a/migrations/0102_fix_linked_issue_gate_mode_default.sql +++ b/migrations/0102_fix_linked_issue_gate_mode_default.sql @@ -12,33 +12,46 @@ -- its own), so a maintainer can genuinely choose 'block' while leaving requireLinkedIssue off. No column on -- this row records which field a maintainer last touched, so per-field intent can't be recovered by itself. -- --- What CAN be proven, path 1: updated_at is bumped on every write through upsertRepositorySettings --- (src/db/repositories.ts) -- the only code path that ever changes this row after INSERT -- while --- created_at is set once, at INSERT, and never touched again. updated_at = created_at therefore means this --- row has never been written to since it was first created: no settings save, by anyone, has ever happened --- for this repo. Its 'block' value can only be the byproduct of migration 0023's column default or 0025's --- blanket flip -- provable drift, not inference from an unrelated field. +-- #gate-review-2727 round 2: an earlier draft used `updated_at = created_at` ("never written to since +-- creation") as the drift signal, anchored implicitly on the row's OWN creation time. That missed rows last +-- written before migration 0023 even added the column (provably drift too, just not "never written") +-- AND, the opposite failure mode, could clobber a repo whose very FIRST-EVER settings save explicitly chose +-- 'block' (upsertRepositorySettings sets createdAt and updatedAt to the same instant on that single INSERT, +-- so a genuine first-save opt-in is indistinguishable from untouched drift under that condition alone). -- --- #gate-review-2727 round 2: path 1 alone is too narrow -- a row last written to BEFORE migration 0023 ran --- (updated_at > created_at, but that write predates 0023) has updated_at != created_at and was skipped, even --- though that write could not possibly have set linked_issue_gate_mode: the column did not exist yet. That --- is an equally provable drift signal, just anchored to a different fixed point. +-- What CAN be proven: updated_at is bumped on every write through upsertRepositorySettings +-- (src/db/repositories.ts) -- the ONLY code path that ever inserts or updates this row, always through +-- Drizzle, always with an explicit resolved value for this column (never omitted, so the raw column-level +-- default below never fires through it) -- so updated_at is exactly "the last instant any application code +-- touched this row, insert or update alike". linked_issue_gate_mode did not exist as a column, and therefore +-- could not have been part of ANY resolved settings value, before migration 0023 was deployed at commit +-- 66e4dd6b (2026-06-05T14:01:39-06:00 = 2026-06-05T20:01:39Z; this repo auto-deploys `wrangler d1 migrations +-- apply --remote` on every merge to main, so the merge commit time is a same-day, conservatively-early proxy +-- for the real remote apply time -- true apply is always slightly AFTER this timestamp, never before, so an +-- inclusive upper bound here can only under-select rows, never wrongly include one written after the column +-- existed). A row whose updated_at is at or before that instant has therefore NEVER been written to by any +-- application code since the column started existing, under ANY value -- its current 'block' can only be +-- the byproduct of migration 0023's column-add default or 0025's later blanket flip, never a maintainer's +-- resolved choice. That single condition subsumes the "never touched since creation" case too: a row with no +-- write at all still has its ORIGINAL creation timestamp as its updated_at, which for any row old enough to +-- predate 0023 is at-or-before the cutoff by construction. -- --- What CAN be proven, path 2: migration 0023 was deployed at commit 66e4dd6b (2026-06-05T14:01:39-06:00 = --- 2026-06-05T20:01:39Z; this repo auto-deploys `wrangler d1 migrations apply --remote` on every merge to --- main, so the merge commit time is a same-day, conservatively-early proxy for the real remote apply time -- --- true apply is always slightly AFTER this timestamp, never before, so using it as an inclusive upper bound --- can only under-select rows, never wrongly include a row genuinely written after the column existed). A row --- whose updated_at is at or before that instant was last written before linked_issue_gate_mode existed at --- all, so its current value is provably the column-add default (or 0025's later blanket flip), not a choice. --- --- A row with updated_at > created_at AND updated_at after that cutoff has been through at least one real --- settings write since the column existed and is left alone, even though some of those may also be --- untouched drift this migration can no longer safely reach -- a maintainer stuck with a leftover 'block' +-- A row with updated_at after that cutoff has been through at least one real settings write (its own +-- creation or a later update) since the column existed and is left alone, even though some of those may also +-- be untouched drift this migration can no longer safely reach -- a maintainer stuck with a leftover 'block' -- default can flip it from the settings UI. Leaving that row alone is the safe failure mode; silently --- downgrading a real 'block' opt-in to advisory is not. +-- downgrading a real 'block' opt-in -- first-save or later -- to advisory is not. +-- +-- The raw SQLite column-level DEFAULT on linked_issue_gate_mode itself (set to 'block' by migration 0023's +-- `ADD COLUMN ... DEFAULT 'block'`) is intentionally left as-is here rather than rebuilt to 'advisory': SQLite +-- has no ALTER COLUMN SET DEFAULT, so changing it requires a full create-copy-drop-rename table rebuild -- +-- an operation with no precedent anywhere in migrations/ and real operational risk for a table this central, +-- for a column-level default that no live code path ever reaches (upsertRepositorySettings is the only +-- writer, and it always resolves and supplies an explicit value; see +-- test/unit/schema-timestamp-defaults.test.ts, which pins that even an omitted-field Drizzle insert resolves +-- via the schema.ts default, never this raw column default). UPDATE repository_settings SET linked_issue_gate_mode = 'advisory' WHERE linked_issue_gate_mode = 'block' AND require_linked_issue = 0 - AND (updated_at = created_at OR updated_at <= '2026-06-05T20:01:39.000Z'); + AND updated_at <= '2026-06-05T20:01:39.000Z'; diff --git a/src/db/schema.ts b/src/db/schema.ts index 61321fad4d..a7e1ac01ed 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -56,7 +56,11 @@ export const repositorySettings = sqliteTable("repository_settings", { // blocks when a repo explicitly opts in (linkedIssueGateMode: "block" or the requireLinkedIssue toggle; // see resolveEffectiveSettings in signals/focus-manifest.ts). This default was "block" until #selfhost- // linked-issue-gate-drift, which also backfills any row an earlier migration (0023/0025) persisted as - // "block" without an explicit opt-in (migrations/0102_fix_linked_issue_gate_mode_default.sql). + // "block" without an explicit opt-in (migrations/0102_fix_linked_issue_gate_mode_default.sql). The raw + // SQLite column still has a DEFAULT 'block' from migration 0023 -- SQLite has no ALTER COLUMN SET DEFAULT, + // so fixing that requires a full table rebuild, not done here since no write path ever omits this field + // (see test/unit/schema-timestamp-defaults.test.ts). This .default("advisory") is what actually applies, + // client-side, on any insert that omits the field. linkedIssueGateMode: text("linked_issue_gate_mode").notNull().default("advisory"), duplicatePrGateMode: text("duplicate_pr_gate_mode").notNull().default("block"), qualityGateMode: text("quality_gate_mode").notNull().default("advisory"), diff --git a/test/unit/migration-0102-linked-issue-gate-mode.test.ts b/test/unit/migration-0102-linked-issue-gate-mode.test.ts index a363b3a5de..e619d089b7 100644 --- a/test/unit/migration-0102-linked-issue-gate-mode.test.ts +++ b/test/unit/migration-0102-linked-issue-gate-mode.test.ts @@ -24,11 +24,10 @@ function applyMigration(db: DatabaseSync, file: string): void { } // created_at/updated_at are set explicitly (not left to the column's CURRENT_TIMESTAMP default) so a -// "touched since creation" row can be simulated deterministically. Migration 0102 flips a row whose -// updated_at still equals its created_at, OR whose updated_at is at/before the instant migration 0023 -// deployed (2026-06-05T20:01:39.000Z, see the migration's own header comment for why) -- so a "touched" -// fixture must land AFTER that cutoff to exercise the "genuinely left alone" path instead of accidentally -// also satisfying the pre-column-existence path. +// "touched since creation" row can be simulated deterministically. Migration 0102 flips a row iff its +// updated_at is at/before the instant migration 0023 deployed (2026-06-05T20:01:39.000Z, see the migration's +// own header comment for why) -- so a "touched" fixture must land AFTER that cutoff to exercise the +// "genuinely left alone" path. const AFTER_0023_CUTOFF = "2026-07-01T00:00:00.000Z"; function insertRepositorySettingsRow( @@ -83,10 +82,11 @@ describe("migration 0102: fix linked_issue_gate_mode default drift (#selfhost-li expect(readLinkedIssueGateMode(db, "acme/explicit-opt-in")).toBe("block"); }); - // #gate-review-2727: the exact scenario the reviewer flagged -- a maintainer who chose 'block' from the - // settings UI's "Linked issue" dropdown without also turning on the separate "Require a linked issue" - // toggle. require_linked_issue = 0 alone can't distinguish this from drift; updated_at > created_at can, - // because it proves a real settings write happened after the row was created. + // #gate-review-2727 round 1: the exact scenario the reviewer flagged -- a maintainer who chose 'block' from + // the settings UI's "Linked issue" dropdown without also turning on the separate "Require a linked issue" + // toggle, THEN saved again later (e.g. touching an unrelated field). require_linked_issue = 0 alone can't + // distinguish this from drift; updated_at after the 0023 cutoff can, because it proves a real settings + // write happened after the column -- and therefore this field -- existed to have an opinion about. it("leaves a 'block' row alone when it has been saved since creation, even with require_linked_issue = 0", () => { const db = applyMigrationsBefore(MIGRATION_FILE); insertRepositorySettingsRow(db, "acme/explicit-block-no-require", "block", 0, true); @@ -96,11 +96,28 @@ describe("migration 0102: fix linked_issue_gate_mode default drift (#selfhost-li expect(readLinkedIssueGateMode(db, "acme/explicit-block-no-require")).toBe("block"); }); - // #gate-review-2727 round 2: a row last written to BEFORE migration 0023 ran has updated_at != created_at, - // so it fails the "never touched" path -- but that write could not possibly have set - // linked_issue_gate_mode, because the column did not exist yet. Proves this via the REAL migration - // sequence (0023's ADD COLUMN DEFAULT 'block' backfills the pre-existing row, exactly like production), - // not a hand-inserted 'block' row. + // #gate-review-2727 round 3: a NARROWER, more direct version of the same concern -- a repo whose very + // FIRST-EVER settings save (a single INSERT, so created_at = updated_at by construction) explicitly chose + // 'block'. An earlier draft used `updated_at = created_at` as its sole drift signal, which could not tell + // this apart from a genuinely untouched drifted row. Anchoring on the 0023 cutoff instead of on + // created_at/updated_at equality fixes this: this row's single write happened AFTER the column existed, so + // it is left alone regardless of whether it was ever touched again. + it("leaves a 'block' row alone when it was chosen on the repo's very first settings save, after the column existed", () => { + const db = applyMigrationsBefore(MIGRATION_FILE); + db.prepare( + "INSERT INTO repository_settings (repo_full_name, linked_issue_gate_mode, require_linked_issue, created_at, updated_at) VALUES (?, 'block', 0, ?, ?)", + ).run("acme/first-save-block", AFTER_0023_CUTOFF, AFTER_0023_CUTOFF); + + applyMigration(db, MIGRATION_FILE); + + expect(readLinkedIssueGateMode(db, "acme/first-save-block")).toBe("block"); + }); + + // #gate-review-2727 round 2: a row last written to BEFORE migration 0023 ran has updated_at > created_at + // (an old write, from before the column existed), which an earlier "never touched since creation" signal + // would have skipped -- but that write could not possibly have set linked_issue_gate_mode either way. + // Proves this via the REAL migration sequence (0023's ADD COLUMN DEFAULT 'block' backfills the pre-existing + // row, exactly like production), not a hand-inserted 'block' row. it("repairs a row that predates the linked_issue_gate_mode column entirely, via the real migration sequence", () => { const db = applyMigrationsBefore("0023_gate_quality_modes.sql"); db.prepare("INSERT INTO repository_settings (repo_full_name, created_at, updated_at) VALUES (?, ?, ?)").run( diff --git a/test/unit/schema-timestamp-defaults.test.ts b/test/unit/schema-timestamp-defaults.test.ts index 84486d8705..aeec02d8b6 100644 --- a/test/unit/schema-timestamp-defaults.test.ts +++ b/test/unit/schema-timestamp-defaults.test.ts @@ -26,6 +26,12 @@ describe("timestamp column defaults", () => { expect(row?.createdAt).toMatch(ISO); expect(row?.updatedAt).toMatch(ISO); expect(row?.createdAt).not.toBe("CURRENT_TIMESTAMP"); + // #gate-review-2727: the raw SQLite column-level DEFAULT for linked_issue_gate_mode is still 'block' + // (migration 0023 added it that way, and SQLite has no ALTER COLUMN SET DEFAULT to fix it without a full + // table rebuild -- see migrations/0102_fix_linked_issue_gate_mode_default.sql's header comment). This + // pins that the raw default never actually fires: Drizzle's schema.ts `.default("advisory")` is injected + // client-side into the generated INSERT whenever the field is omitted, same as createdAt/updatedAt above. + expect(row?.linkedIssueGateMode).toBe("advisory"); }); it("keeps orb relay pending coalesce keys wired through the drizzle schema", async () => {