From 0ea11612f68e4dd0e7112408eff2c8c8ea1459a2 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 13 Jul 2026 00:47:28 -0700 Subject: [PATCH] fix(miner): scope deny-hook synthesis proposals by forge host, not bare repoFullName deny_rule_proposals' PRIMARY KEY (repo_full_name, id) let two forge hosts (github.com vs. a GitHub Enterprise host, #4784) serving a same-named owner/repo share one proposal row (and its maintainer approval decision). Rebuild the constraint to PRIMARY KEY (api_base_url, repo_full_name, id). This file has no schema-version framework of its own (unlike the package's other local stores) -- it uses a raw DatabaseSync connection, no applySchemaMigrations. Follows governor-state.js's idempotent column-presence-gated rebuild convention instead of introducing a new framework dependency here for the first time. Uses INSERT OR IGNORE for the copy step, matching the fix already applied to every other #5563 migration in this epic: a legacy row with an already-invalid status value (this store's own CHECK-constrained schema already rejects those) is dropped, not a migration-aborting crash. Threads an optional apiBaseUrl through refreshProposals/listProposals/ setProposalStatus/resolveEffectiveRules. initDenyHookSynthesisStore has no real callers yet (feeds the consumption surface #2343 will eventually wire into evaluateDenyHooks; this store owns derivation + audit, not live hook interception), so there is no CLI surface or call site to thread through -- purely the storage-layer fix, mirroring governor_reputation_history's scaffold-only precedent. Closes #5563 (5th and final store: claim-ledger.js in #5576, portfolio-queue.js in #5583, run-state.js in #5585, governor-state.js in #5591, plus the claimNextBatch/migration hardening follow-up in #5594). --- .../lib/deny-hook-synthesis.d.ts | 15 +- .../lib/deny-hook-synthesis.js | 78 ++++++++-- test/unit/miner-deny-hook-synthesis.test.ts | 141 ++++++++++++++++++ 3 files changed, 217 insertions(+), 17 deletions(-) diff --git a/packages/gittensory-miner/lib/deny-hook-synthesis.d.ts b/packages/gittensory-miner/lib/deny-hook-synthesis.d.ts index b28600ddee..8e07457ced 100644 --- a/packages/gittensory-miner/lib/deny-hook-synthesis.d.ts +++ b/packages/gittensory-miner/lib/deny-hook-synthesis.d.ts @@ -74,10 +74,19 @@ export type DenyHookSynthesisStore = { repoFullName: string, history: unknown, config?: SynthesisConfig, + apiBaseUrl?: string, ): DenyRuleProposal[]; - listProposals(repoFullName: string): DenyRuleProposal[]; - setProposalStatus(repoFullName: string, proposalId: string, status: DenyRuleProposalStatus): void; - resolveEffectiveRules(repoFullName: string, options?: { includeDefaults?: boolean }): DenyRule[]; + listProposals(repoFullName: string, apiBaseUrl?: string): DenyRuleProposal[]; + setProposalStatus( + repoFullName: string, + proposalId: string, + status: DenyRuleProposalStatus, + apiBaseUrl?: string, + ): void; + resolveEffectiveRules( + repoFullName: string, + options?: { includeDefaults?: boolean; apiBaseUrl?: string }, + ): DenyRule[]; close(): void; }; diff --git a/packages/gittensory-miner/lib/deny-hook-synthesis.js b/packages/gittensory-miner/lib/deny-hook-synthesis.js index 4697a4f14e..0cc3b6f1e7 100644 --- a/packages/gittensory-miner/lib/deny-hook-synthesis.js +++ b/packages/gittensory-miner/lib/deny-hook-synthesis.js @@ -9,6 +9,7 @@ import { dirname, join } from "node:path"; import { createHash } from "node:crypto"; import { DatabaseSync } from "node:sqlite"; import { DEFAULT_DENY_RULES, evaluateDenyHooks } from "./deny-hooks.js"; +import { DEFAULT_FORGE_CONFIG } from "./forge-config.js"; const defaultDbFileName = "deny-hook-synthesis.sqlite3"; const PROPOSAL_STATUSES = Object.freeze(["proposed", "approved", "rejected"]); @@ -26,6 +27,14 @@ function normalizeRepoFullName(repoFullName) { return `${owner}/${repo}`; } +/** Optional forge host, scoping rows so two hosts serving the same owner/repo name never collide (#5563). + * Omitted/nullish → the github.com default, so every pre-existing single-forge caller is unaffected. */ +function normalizeApiBaseUrl(apiBaseUrl) { + if (apiBaseUrl === undefined || apiBaseUrl === null) return DEFAULT_FORGE_CONFIG.apiBaseUrl; + if (typeof apiBaseUrl !== "string" || !apiBaseUrl.trim()) throw new Error("invalid_api_base_url"); + return apiBaseUrl.trim(); +} + function normalizeOptionalStringArray(value) { if (value === undefined || value === null) return []; if (!Array.isArray(value)) return []; @@ -255,6 +264,42 @@ function rowToProposal(row) { }; } +// Rebuild deny_rule_proposals' (repo_full_name, id) PRIMARY KEY into a (api_base_url, repo_full_name, id) +// composite (#5563) -- two forge hosts serving a same-named owner/repo must not share one proposal row. SQLite +// cannot ALTER a PRIMARY KEY in place, so this rebuilds the table: create the new shape, copy every existing row +// with the pre-#4784 implicit single-forge default backfilled, drop the old table, rename the new one in. +// Guarded by a column-presence check (this module has no schema-version framework of its own, unlike the +// package's other local stores) so this only runs once per file. +function ensureDenyRuleProposalsForgeScope(db) { + const hasApiBaseUrlColumn = db + .prepare("PRAGMA table_info(deny_rule_proposals)") + .all() + .some((column) => column.name === "api_base_url"); + if (hasApiBaseUrlColumn) return; + db.exec(` + CREATE TABLE deny_rule_proposals_v2 ( + api_base_url TEXT NOT NULL, + repo_full_name TEXT NOT NULL, + id TEXT NOT NULL, + status TEXT NOT NULL CHECK (status IN ('proposed', 'approved', 'rejected')), + rule_json TEXT NOT NULL, + audit_json TEXT NOT NULL, + updated_at TEXT NOT NULL, + PRIMARY KEY (api_base_url, repo_full_name, id) + ) + `); + // OR IGNORE: a row this store's own read path already treats as unusable garbage (an unrecognized `status`, + // e.g. from a hand-edited or otherwise corrupted file) would violate the CHECK constraint above and abort the + // whole migration. Skipping it here is consistent with that same fail-closed posture, rather than turning one + // bad row into a permanently unmigratable file. + db.prepare( + `INSERT OR IGNORE INTO deny_rule_proposals_v2 (api_base_url, repo_full_name, id, status, rule_json, audit_json, updated_at) + SELECT ?, repo_full_name, id, status, rule_json, audit_json, updated_at FROM deny_rule_proposals`, + ).run(DEFAULT_FORGE_CONFIG.apiBaseUrl); + db.exec("DROP TABLE deny_rule_proposals"); + db.exec("ALTER TABLE deny_rule_proposals_v2 RENAME TO deny_rule_proposals"); +} + /** * Local SQLite store for synthesized deny-rule proposals. Refresh re-derives proposals from history while * preserving maintainer decisions on ids that still exist. @@ -276,40 +321,43 @@ export function initDenyHookSynthesisStore(dbPath = resolveDenyHookSynthesisDbPa PRIMARY KEY (repo_full_name, id) ) `); + ensureDenyRuleProposalsForgeScope(db); const upsertStatement = db.prepare(` - INSERT INTO deny_rule_proposals (repo_full_name, id, status, rule_json, audit_json, updated_at) - VALUES (?, ?, ?, ?, ?, ?) - ON CONFLICT(repo_full_name, id) DO UPDATE SET + INSERT INTO deny_rule_proposals (api_base_url, repo_full_name, id, status, rule_json, audit_json, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?) + ON CONFLICT(api_base_url, repo_full_name, id) DO UPDATE SET status = excluded.status, rule_json = excluded.rule_json, audit_json = excluded.audit_json, updated_at = excluded.updated_at `); const getStatusStatement = db.prepare( - "SELECT status FROM deny_rule_proposals WHERE repo_full_name = ? AND id = ?", + "SELECT status FROM deny_rule_proposals WHERE api_base_url = ? AND repo_full_name = ? AND id = ?", ); const listStatement = db.prepare( - "SELECT repo_full_name, id, status, rule_json, audit_json, updated_at FROM deny_rule_proposals WHERE repo_full_name = ? ORDER BY id ASC", + "SELECT repo_full_name, id, status, rule_json, audit_json, updated_at FROM deny_rule_proposals WHERE api_base_url = ? AND repo_full_name = ? ORDER BY id ASC", ); const setStatusStatement = db.prepare(` - UPDATE deny_rule_proposals SET status = ?, updated_at = ? WHERE repo_full_name = ? AND id = ? + UPDATE deny_rule_proposals SET status = ?, updated_at = ? WHERE api_base_url = ? AND repo_full_name = ? AND id = ? `); return { dbPath: resolvedPath, - refreshProposals(repoFullName, history, config = {}) { + refreshProposals(repoFullName, history, config = {}, apiBaseUrl) { + const forge = normalizeApiBaseUrl(apiBaseUrl); const repo = normalizeRepoFullName(repoFullName); const synthesized = synthesizeDenyRuleProposals(history, config); const updatedAt = new Date().toISOString(); db.exec("BEGIN IMMEDIATE"); try { for (const proposal of synthesized) { - const existing = getStatusStatement.get(repo, proposal.id); + const existing = getStatusStatement.get(forge, repo, proposal.id); const status = existing?.status && proposalStatusSet.has(existing.status) && existing.status !== "proposed" ? existing.status : "proposed"; upsertStatement.run( + forge, repo, proposal.id, status, @@ -323,20 +371,22 @@ export function initDenyHookSynthesisStore(dbPath = resolveDenyHookSynthesisDbPa db.exec("ROLLBACK"); throw error; } - return listStatement.all(repo).map(rowToProposal); + return listStatement.all(forge, repo).map(rowToProposal); }, - listProposals(repoFullName) { + listProposals(repoFullName, apiBaseUrl) { + const forge = normalizeApiBaseUrl(apiBaseUrl); const repo = normalizeRepoFullName(repoFullName); - return listStatement.all(repo).map(rowToProposal); + return listStatement.all(forge, repo).map(rowToProposal); }, - setProposalStatus(repoFullName, proposalId, status) { + setProposalStatus(repoFullName, proposalId, status, apiBaseUrl) { + const forge = normalizeApiBaseUrl(apiBaseUrl); const repo = normalizeRepoFullName(repoFullName); if (typeof proposalId !== "string" || !proposalId.trim()) throw new Error("invalid_proposal_id"); if (!proposalStatusSet.has(status)) throw new Error("invalid_proposal_status"); - setStatusStatement.run(status, new Date().toISOString(), repo, proposalId.trim()); + setStatusStatement.run(status, new Date().toISOString(), forge, repo, proposalId.trim()); }, resolveEffectiveRules(repoFullName, options = {}) { - const proposals = this.listProposals(repoFullName); + const proposals = this.listProposals(repoFullName, options.apiBaseUrl); return resolveEffectiveDenyRules({ includeDefaults: options.includeDefaults, approvedProposals: proposals, diff --git a/test/unit/miner-deny-hook-synthesis.test.ts b/test/unit/miner-deny-hook-synthesis.test.ts index 8e6db2feb9..a809cf59e9 100644 --- a/test/unit/miner-deny-hook-synthesis.test.ts +++ b/test/unit/miner-deny-hook-synthesis.test.ts @@ -1,6 +1,7 @@ import { mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; +import { DatabaseSync } from "node:sqlite"; import { afterEach, describe, expect, it } from "vitest"; import { DEFAULT_DENY_RULES, @@ -112,4 +113,144 @@ describe("initDenyHookSynthesisStore() (#4522)", () => { const effective = store.resolveEffectiveRules("acme/widgets"); expect(effective.length).toBe(DEFAULT_DENY_RULES.length + 1); }); + + describe("forge-scoping (#5563)", () => { + it("two forge hosts can each hold their own proposals for the same owner/repo without colliding", () => { + const store = tempStore(); + const history = [ + { blockerCodes: ["guardrail_hold"], changedPaths: ["CHANGELOG.md"] }, + { blockerCodes: ["guardrail_hold"], changedPaths: ["CHANGELOG.md"] }, + ]; + const ghRefreshed = store.refreshProposals("acme/widgets", history, {}, "https://api.github.com"); + const gheRefreshed = store.refreshProposals("acme/widgets", history, {}, "https://ghe.example.com/api/v3"); + expect(ghRefreshed).toHaveLength(1); + expect(gheRefreshed).toHaveLength(1); + // Same synthesized proposal id (derived from the path, not the host) on both hosts, but the rows are + // independent -- approving one host's proposal must not affect the other's. + expect(ghRefreshed[0]!.id).toBe(gheRefreshed[0]!.id); + store.setProposalStatus("acme/widgets", ghRefreshed[0]!.id, "approved", "https://api.github.com"); + expect(store.listProposals("acme/widgets", "https://api.github.com")[0]?.status).toBe("approved"); + expect(store.listProposals("acme/widgets", "https://ghe.example.com/api/v3")[0]?.status).toBe("proposed"); + }); + + it("defaults apiBaseUrl to the github.com default when omitted", () => { + const store = tempStore(); + const history = [ + { blockerCodes: ["guardrail_hold"], changedPaths: ["CHANGELOG.md"] }, + { blockerCodes: ["guardrail_hold"], changedPaths: ["CHANGELOG.md"] }, + ]; + store.refreshProposals("acme/widgets", history); + expect(store.listProposals("acme/widgets", "https://api.github.com")).toHaveLength(1); + }); + + it("resolveEffectiveRules threads options.apiBaseUrl through to listProposals", () => { + const store = tempStore(); + const history = [ + { blockerCodes: ["guardrail_hold"], changedPaths: ["CHANGELOG.md"] }, + { blockerCodes: ["guardrail_hold"], changedPaths: ["CHANGELOG.md"] }, + ]; + const refreshed = store.refreshProposals("acme/widgets", history, {}, "https://ghe.example.com/api/v3"); + store.setProposalStatus("acme/widgets", refreshed[0]!.id, "approved", "https://ghe.example.com/api/v3"); + + // The github.com host has no approved proposal -- effective rules stay at the static defaults. + expect(store.resolveEffectiveRules("acme/widgets").length).toBe(DEFAULT_DENY_RULES.length); + // The GHE host's approval is picked up when its apiBaseUrl is threaded through. + expect( + store.resolveEffectiveRules("acme/widgets", { apiBaseUrl: "https://ghe.example.com/api/v3" }).length, + ).toBe(DEFAULT_DENY_RULES.length + 1); + }); + + it("rejects a non-string or blank apiBaseUrl", () => { + const store = tempStore(); + expect(() => store.listProposals("acme/widgets", " ")).toThrow("invalid_api_base_url"); + expect(() => store.refreshProposals("acme/widgets", [], {}, 42 as never)).toThrow("invalid_api_base_url"); + expect(() => store.setProposalStatus("acme/widgets", "x", "approved", " ")).toThrow("invalid_api_base_url"); + }); + + it("migrates an existing pre-#5563 file, backfilling api_base_url and preserving every row", () => { + const dir = mkdtempSync(join(tmpdir(), "miner-deny-hook-synthesis-legacy-")); + tempDirs.push(dir); + const dbPath = join(dir, "legacy.sqlite3"); + const legacy = new DatabaseSync(dbPath); + legacy.exec(` + CREATE TABLE deny_rule_proposals ( + repo_full_name TEXT NOT NULL, + id TEXT NOT NULL, + status TEXT NOT NULL CHECK (status IN ('proposed', 'approved', 'rejected')), + rule_json TEXT NOT NULL, + audit_json TEXT NOT NULL, + updated_at TEXT NOT NULL, + PRIMARY KEY (repo_full_name, id) + ) + `); + legacy + .prepare( + "INSERT INTO deny_rule_proposals (repo_full_name, id, status, rule_json, audit_json, updated_at) VALUES (?, ?, ?, ?, ?, ?)", + ) + .run( + "acme/widgets", + "path:abc123", + "approved", + JSON.stringify({ matcher: "*", pathPattern: "**/changelog.md", reason: "legacy" }), + JSON.stringify({ kind: "path_history", synthesizedAt: "2026-01-01T00:00:00.000Z" }), + "2026-01-01T00:00:00.000Z", + ); + legacy.close(); + + const store = initDenyHookSynthesisStore(dbPath); + stores.push(store); + expect(store.listProposals("acme/widgets", "https://api.github.com")).toEqual([ + { + id: "path:abc123", + status: "approved", + rule: { matcher: "*", pathPattern: "**/changelog.md", reason: "legacy" }, + audit: { kind: "path_history", synthesizedAt: "2026-01-01T00:00:00.000Z" }, + }, + ]); + // The old bare (repo_full_name, id) collision is gone: a second host can now hold its own proposal state. + store.setProposalStatus("acme/widgets", "path:abc123", "rejected", "https://ghe.example.com/api/v3"); + expect(store.listProposals("acme/widgets", "https://api.github.com")[0]?.status).toBe("approved"); + }); + + it("REGRESSION: a legacy row violating the rebuilt table's status CHECK constraint is dropped, not a migration-aborting crash", () => { + const dir = mkdtempSync(join(tmpdir(), "miner-deny-hook-synthesis-legacy-corrupt-")); + tempDirs.push(dir); + const dbPath = join(dir, "legacy-corrupt.sqlite3"); + const legacy = new DatabaseSync(dbPath); + // No CHECK on status here, simulating a hand-edited or otherwise corrupted legacy file -- the real + // baseline schema always enforces the CHECK, so this can only arise from external tampering. + legacy.exec(` + CREATE TABLE deny_rule_proposals ( + repo_full_name TEXT NOT NULL, + id TEXT NOT NULL, + status TEXT NOT NULL, + rule_json TEXT NOT NULL, + audit_json TEXT NOT NULL, + updated_at TEXT NOT NULL, + PRIMARY KEY (repo_full_name, id) + ) + `); + legacy + .prepare( + "INSERT INTO deny_rule_proposals (repo_full_name, id, status, rule_json, audit_json, updated_at) VALUES (?, ?, ?, ?, ?, ?)", + ) + .run("acme/corrupt", "path:bad", "bogus", "{}", "{}", "2026-01-01T00:00:00.000Z"); + legacy + .prepare( + "INSERT INTO deny_rule_proposals (repo_full_name, id, status, rule_json, audit_json, updated_at) VALUES (?, ?, ?, ?, ?, ?)", + ) + .run("acme/widgets", "path:ok", "proposed", "{}", "{}", "2026-01-01T00:00:00.000Z"); + legacy.close(); + + let opened: ReturnType | undefined; + expect(() => { + opened = initDenyHookSynthesisStore(dbPath); + }).not.toThrow(); + const store = opened!; + stores.push(store); + // The corrupt row was dropped, not migrated -- only the valid row survived the rebuild. + expect(store.listProposals("acme/corrupt", "https://api.github.com")).toEqual([]); + expect(store.listProposals("acme/widgets", "https://api.github.com")).toHaveLength(1); + }); + }); });