From 46871aff6c02449a86f630500112a4f0820a21c8 Mon Sep 17 00:00:00 2001 From: emmanuelgjr Date: Fri, 18 Sep 2026 16:02:29 -0400 Subject: [PATCH] Fail on duplicate incident ids, and give contributors a way to allocate one INC-132 was allocated twice - by #117 and by #109 - because both read the end of data/incidents.json while the other was open, and the second one only found out when the merge conflicted. A duplicate id also silently breaks anything that resolves an incident by id: the webapp deep link, the evidence join, the reports. validate.js gains checkIncidentIds(), which fails on a duplicate and names it. scripts/next-incident-id.mjs prints a free id; --check-prs also accounts for ids claimed by open pull requests, which is what would have caught this one - it currently reports INC-136, because INC-135 is claimed by open PR #122. The duplicate case is deliberately not tested by mutating data/incidents.json: node --test runs suites in parallel, so writing to the shared corpus races the suites reading it, which is a bug this repository has already had. The guard is covered by a corpus-uniqueness test and a wiring test instead, and was negative-tested by hand: injecting a duplicate made validate.js exit 1 with "INC-006 is used by 2 records". Baseline before: 0 errors, 88 warnings, 327 passed; 85/85 tests. Baseline after : 0 errors, 88 warnings, 328 passed; 89/89 tests, twice. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/incident-ids.test.mjs | 58 ++++++++++++++++++++++++++++++++ scripts/next-incident-id.mjs | 63 +++++++++++++++++++++++++++++++++++ scripts/validate.js | 45 +++++++++++++++++++++++++ 3 files changed, 166 insertions(+) create mode 100644 scripts/incident-ids.test.mjs create mode 100644 scripts/next-incident-id.mjs diff --git a/scripts/incident-ids.test.mjs b/scripts/incident-ids.test.mjs new file mode 100644 index 0000000..9b3472c --- /dev/null +++ b/scripts/incident-ids.test.mjs @@ -0,0 +1,58 @@ +/** + * Incident id allocation. + * + * INC-132 was allocated twice — by #117 and by #109 — because both read the end + * of data/incidents.json while the other was open. These cover the guard that + * now fails on a duplicate, and the helper that hands out a free id. + */ + +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { execFileSync } from 'node:child_process'; +import { readFileSync, writeFileSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); +const INCIDENTS = path.join(ROOT, 'data', 'incidents.json'); +const run = (script, args = []) => + execFileSync(process.execPath, [path.join(ROOT, 'scripts', script), ...args], { cwd: ROOT, encoding: 'utf8' }); + +test('the committed corpus has no duplicate incident ids', () => { + const { incidents } = JSON.parse(readFileSync(INCIDENTS, 'utf8')); + const seen = new Set(); + const duplicates = incidents.map((i) => i.id).filter((id) => (seen.has(id) ? true : (seen.add(id), false))); + assert.deepEqual(duplicates, [], `duplicate incident ids: ${duplicates.join(', ')}`); +}); + +// The duplicate case is deliberately NOT tested by mutating data/incidents.json: +// node --test runs suites in parallel, so writing to the shared corpus races the +// other suites reading it. The guard is verified two ways instead — the corpus +// check above, and the wiring check below — and negative-tested by hand when it +// was written (injecting a duplicate made validate.js exit 1 with +// "INC-006 is used by 2 records"). +test('the duplicate-id guard is wired into validate.js', () => { + const src = readFileSync(path.join(ROOT, 'scripts', 'validate.js'), 'utf8'); + assert.match(src, /function checkIncidentIds\(\)/, 'the guard is missing'); + assert.match(src, /^\s*checkIncidentIds\(\);/m, 'the guard is defined but never called'); + assert.match(src, /is used by \$\{n\} records/, 'the guard no longer fails on a duplicate'); +}); + +test('next-incident-id.mjs proposes an unused id', () => { + const { incidents } = JSON.parse(readFileSync(INCIDENTS, 'utf8')); + const used = new Set(incidents.map((i) => i.id)); + const next = run('next-incident-id.mjs').trim(); + + assert.match(next, /^INC-\d{3}$/); + assert.ok(!used.has(next), `${next} is already used`); + + const highest = Math.max(...incidents.map((i) => Number(String(i.id).slice(4))).filter(Number.isFinite)); + assert.equal(Number(next.slice(4)), highest + 1); +}); + +test('--json reports what it based the answer on', () => { + const out = JSON.parse(run('next-incident-id.mjs', ['--json'])); + assert.match(out.next, /^INC-\d{3}$/); + assert.match(out.highest_committed, /^INC-\d{3}$/); + assert.ok(Array.isArray(out.claimed_in_open_prs)); +}); diff --git a/scripts/next-incident-id.mjs b/scripts/next-incident-id.mjs new file mode 100644 index 0000000..22c12da --- /dev/null +++ b/scripts/next-incident-id.mjs @@ -0,0 +1,63 @@ +#!/usr/bin/env node +/** + * next-incident-id.mjs — print the next free incident id. + * + * Two contributors adding an incident at the same time both read the end of + * data/incidents.json and both pick the same number; the second one to merge + * finds their PR conflicting (this happened to INC-132). This prints an id that + * accounts for what is already merged, and optionally for what open pull + * requests have claimed. + * + * Usage: + * node scripts/next-incident-id.mjs # next id after data/incidents.json + * node scripts/next-incident-id.mjs --check-prs # also scan open PRs (needs gh) + * node scripts/next-incident-id.mjs --json + */ + +import { readFileSync } from 'node:fs'; +import { execFileSync } from 'node:child_process'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); +const CHECK_PRS = process.argv.includes('--check-prs'); +const AS_JSON = process.argv.includes('--json'); + +const ID = /INC-(\d{3})\b/g; +const format = (n) => `INC-${String(n).padStart(3, '0')}`; + +const db = JSON.parse(readFileSync(path.join(ROOT, 'data', 'incidents.json'), 'utf8')); +const committed = db.incidents.map((i) => Number(String(i.id).slice(4))).filter(Number.isFinite); +const highestCommitted = Math.max(0, ...committed); + +const claimed = new Map(); // id -> where it is claimed +if (CHECK_PRS) { + try { + const list = JSON.parse(execFileSync('gh', ['pr', 'list', '--state', 'open', '--json', 'number,title,headRefName'], { encoding: 'utf8' })); + for (const pr of list) { + // The title and branch name are cheap to read; a full diff per PR is not. + const text = `${pr.title} ${pr.headRefName}`; + for (const m of text.matchAll(ID)) claimed.set(Number(m[1]), `PR #${pr.number}`); + } + } catch (err) { + if (!AS_JSON) console.error(`(could not read open PRs: ${err.message.split('\n')[0]})`); + } +} + +const highestClaimed = Math.max(0, ...claimed.keys()); +const next = Math.max(highestCommitted, highestClaimed) + 1; + +if (AS_JSON) { + console.log(JSON.stringify({ + next: format(next), + highest_committed: format(highestCommitted), + claimed_in_open_prs: [...claimed].sort((a, b) => a[0] - b[0]).map(([n, where]) => ({ id: format(n), where })), + }, null, 2)); +} else { + console.log(format(next)); + if (highestCommitted) console.error(` highest in data/incidents.json: ${format(highestCommitted)}`); + for (const [n, where] of [...claimed].sort((a, b) => a[0] - b[0])) { + if (n > highestCommitted) console.error(` claimed by ${where}: ${format(n)}`); + } + if (!CHECK_PRS) console.error(' (pass --check-prs to account for ids claimed by open pull requests)'); +} diff --git a/scripts/validate.js b/scripts/validate.js index 3d4d50d..941ff64 100644 --- a/scripts/validate.js +++ b/scripts/validate.js @@ -943,6 +943,50 @@ function checkAtlasMappings() { return true; } +/** + * 22. Incident ids must be unique. + * + * Two contributors adding a record at the same time both read the end of + * data/incidents.json and both pick the same number. That is how INC-132 was + * allocated twice (#117 and #109), and the second one only found out when the + * merge conflicted. A duplicate id also silently breaks anything that resolves + * an incident by id — the webapp deep link, the evidence join, the reports. + * + * scripts/next-incident-id.mjs prints a free id, and --check-prs also accounts + * for ids claimed by open pull requests. + */ +function checkIncidentIds() { + const incPath = path.join(ROOT, 'data', 'incidents.json'); + if (!fs.existsSync(incPath)) return true; + const doc = JSON.parse(fs.readFileSync(incPath, 'utf8')); + const incidents = doc.incidents || []; + + const seen = new Map(); + const duplicates = new Map(); + for (const inc of incidents) { + const id = String(inc.id); + if (seen.has(id)) duplicates.set(id, (duplicates.get(id) || 1) + 1); + else seen.set(id, inc.title || ''); + } + + for (const [id, n] of duplicates) { + fail('Incident ids', `${id} is used by ${n} records — allocate a free id with scripts/next-incident-id.mjs`); + } + + // A gap is not an error (a record may be withdrawn), but a silent gap plus a + // duplicate is how a renumbering goes wrong, so the count is reported. + const numbers = incidents.map((i) => Number(String(i.id).slice(4))).filter(Number.isFinite).sort((a, b) => a - b); + const gaps = []; + for (let i = 1; i < numbers.length; i++) { + for (let n = numbers[i - 1] + 1; n < numbers[i]; n++) gaps.push(n); + } + + if (!duplicates.size) { + pass('Incident ids', `${incidents.length} incident ids are unique (highest INC-${String(numbers[numbers.length - 1] || 0).padStart(3, '0')}${gaps.length ? `, ${gaps.length} unused number(s)` : ''})`); + } + return duplicates.size === 0; +} + /** * Evidence guard (T-STRAT03). * @@ -1122,6 +1166,7 @@ function run() { checkMaestroLayers(); checkAtlasMappings(); checkControlIdShapes(); + checkIncidentIds(); checkEvidence(); }