diff --git a/.github/workflows/ci-quality.yml b/.github/workflows/ci-quality.yml index 1a2ac4728..83c64b7f3 100644 --- a/.github/workflows/ci-quality.yml +++ b/.github/workflows/ci-quality.yml @@ -24,6 +24,30 @@ jobs: - run: npm ci --ignore-scripts - run: npx prettier --check . + parse-cache-version: + # Guards a collision no single-branch test can see: two PRs claiming the + # same SCHEMA_BUMP both pass their own `toBe(N)` pin, then share one + # PARSE_CACHE_VERSION after merge and one side's parse-time capture change + # silently replays pre-change ParsedFiles forever. Ten ledger entries, four + # exact clashes, every one caught by hand at merge time until now. + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false + # Full history: the check reads the BASE branch's value, which a + # shallow clone does not contain. The script exits 2 (distinct from a + # real conflict) if the ref is unreachable, so a misconfiguration here + # cannot masquerade as a pass. + fetch-depth: 0 + - name: Fetch base branch + run: git fetch --no-tags --depth=1 origin "${GITHUB_BASE_REF:-main}:refs/remotes/origin/${GITHUB_BASE_REF:-main}" || git fetch --no-tags origin "${GITHUB_BASE_REF:-main}" + - name: Compare SCHEMA_BUMP against the base branch + env: + GITNEXUS_BASE_REF: origin/${{ github.base_ref || 'main' }} + run: node scripts/check-parse-cache-version.mjs + lint: runs-on: ubuntu-latest timeout-minutes: 10 diff --git a/gitnexus/test/unit/parse-cache-version-check.test.ts b/gitnexus/test/unit/parse-cache-version-check.test.ts new file mode 100644 index 000000000..ff5582fe3 --- /dev/null +++ b/gitnexus/test/unit/parse-cache-version-check.test.ts @@ -0,0 +1,131 @@ +/** + * The CI guard for concurrent `SCHEMA_BUMP` claims. + * + * The in-repo pin (`expect(...).toBe(N)`) cannot catch this and the ledger says + * so four separate times: both branches assert the same number, so both are + * green while they collide. The conflict exists only in the RELATION between + * two branches, so the check has to compare against the base — and therefore + * has to be tested against real git state rather than mocked. + * + * Failure here is silent in production, which is why it kept recurring: + * `PARSE_CACHE_VERSION` is the only invalidator for the durable ParsedFile + * store, so a shared version means one side's parse-time change replays + * pre-change ParsedFiles on every incremental analyze, and the graph is simply + * missing edges. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFileSync, spawnSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; + +const SCRIPT = path.resolve( + __dirname, + '..', + '..', + '..', + 'scripts', + 'check-parse-cache-version.mjs', +); +const CACHE_REL = path.join('gitnexus', 'src', 'storage', 'parse-cache.ts'); + +let repo: string; + +const git = (...args: string[]): void => { + execFileSync('git', args, { cwd: repo, stdio: 'ignore' }); +}; + +const writeBump = (n: number): void => { + const file = path.join(repo, CACHE_REL); + fs.mkdirSync(path.dirname(file), { recursive: true }); + fs.writeFileSync(file, `// fixture\nconst SCHEMA_BUMP = ${n};\nexport {};\n`, 'utf8'); +}; + +/** Run the check against `baseRef`, returning its exit code and stderr. */ +const runCheck = (baseRef: string): { code: number; out: string } => { + const res = spawnSync(process.execPath, [SCRIPT], { + cwd: repo, + encoding: 'utf8', + env: { ...process.env, GITNEXUS_BASE_REF: baseRef }, + }); + return { code: res.status ?? -1, out: `${res.stdout}${res.stderr}` }; +}; + +beforeAll(() => { + repo = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-cachever-')); + git('init', '-q', '--initial-branch=main'); + git('config', 'user.email', 't@t'); + git('config', 'user.name', 't'); + writeBump(45); + git('add', '-A'); + git('commit', '-qm', 'base at 45'); +}); + +afterAll(() => { + fs.rmSync(repo, { recursive: true, force: true }); +}); + +describe('check-parse-cache-version', () => { + // The case that matters most in practice, because it is the case for almost + // every PR — and the one this check got wrong first. Requiring a strict + // increase unconditionally would have failed every branch that does not touch + // the parse cache, including the branch that introduced the check. + it('passes a branch that does not touch the parse cache at all', () => { + // Byte-identical to base: no claim on any version, nothing to collide with. + const { code, out } = runCheck('main'); + expect(code).toBe(0); + expect(out).toContain('claims no version'); + }); + + it('passes when the branch raises the version', () => { + writeBump(46); + const { code, out } = runCheck('main'); + expect(code).toBe(0); + expect(out).toContain('OK'); + }); + + // The case the pin test is blind to, and the one that has actually happened + // four times. + it('fails when both sides claim the same number', () => { + // Same NUMBER but the file is modified, which is what makes it a claim. + // (A comment change is enough — the point is that this branch is editing + // the parse cache while leaving the version alone.) + fs.writeFileSync( + path.join(repo, CACHE_REL), + `// fixture, edited\nconst SCHEMA_BUMP = 45;\nexport {};\n`, + 'utf8', + ); + const { code, out } = runCheck('main'); + expect(code).toBe(1); + expect(out).toContain('on BOTH this branch'); + // The message must say what to do, not just that something is wrong. + expect(out).toContain('46 or higher'); + }); + + it('fails when the version goes backwards', () => { + writeBump(44); + const { code, out } = runCheck('main'); + expect(code).toBe(1); + expect(out).toMatch(/BACKWARDS/); + }); + + // A misconfigured CI checkout must not look like a pass. Exit 2 is distinct + // from both success and a real conflict so the two cannot be confused. + it('exits 2 — not 0 — when the base ref is unreachable', () => { + writeBump(46); + const { code, out } = runCheck('origin/nope'); + expect(code).toBe(2); + expect(out).toContain('fetch-depth'); + }); + + it('fails loudly if the declaration it parses ever moves', () => { + fs.writeFileSync( + path.join(repo, CACHE_REL), + '// renamed away\nconst SOMETHING_ELSE = 46;\nexport {};\n', + 'utf8', + ); + const { code, out } = runCheck('main'); + expect(code).not.toBe(0); + expect(out).toContain('do not delete the check'); + }); +}); diff --git a/scripts/check-parse-cache-version.mjs b/scripts/check-parse-cache-version.mjs new file mode 100644 index 000000000..ca6f61699 --- /dev/null +++ b/scripts/check-parse-cache-version.mjs @@ -0,0 +1,110 @@ +#!/usr/bin/env node +/** + * Fail a PR whose `SCHEMA_BUMP` is not STRICTLY GREATER than the base branch's. + * + * The in-repo pin test cannot catch this, and its ledger says so in four + * separate entries: both branches assert `toBe(N)`, so the assertion is green on + * each side while they claim the same number. The collision only exists in the + * relationship BETWEEN the two branches, which no single-branch test can see. + * + * The consequence is silent, which is why it keeps recurring. `PARSE_CACHE_VERSION` + * is the only invalidator for the durable ParsedFile store — byte-unchanged files + * skip tree-sitter entirely — so two divergent capture schemas sharing one version + * string means one side's parse-time change replays pre-change ParsedFiles forever. + * Nothing throws; the graph is simply missing edges, which is the confident-empty + * answer this repo has spent several PRs removing. + * + * Ten ledger entries, four of them exact clashes, every one caught by hand at + * merge time. This is that manual step, run on every PR instead of remembered. + */ +import { execFileSync } from 'node:child_process'; +import { readFileSync } from 'node:fs'; + +const BASE_REF = process.env.GITNEXUS_BASE_REF ?? 'origin/main'; +const CACHE_FILE = 'gitnexus/src/storage/parse-cache.ts'; +const PATTERN = /^const SCHEMA_BUMP = (\d+);/m; + +/** Parse `SCHEMA_BUMP` out of a parse-cache source string. */ +function readBump(source, origin) { + const match = PATTERN.exec(source); + if (match === null) { + throw new Error( + `Could not find \`const SCHEMA_BUMP = ;\` in ${origin}. If the declaration ` + + `moved or was renamed, update scripts/check-parse-cache-version.mjs to match — ` + + `do not delete the check.`, + ); + } + return Number(match[1]); +} + +function readBaseSource() { + try { + return execFileSync('git', ['show', `${BASE_REF}:${CACHE_FILE}`], { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'], + }); + } catch { + return null; + } +} + +const baseSource = readBaseSource(); +if (baseSource === null) { + // A shallow clone or a missing base ref is a CI-configuration problem, not a + // version conflict. Say so plainly rather than passing quietly (which would + // make the check look green while testing nothing) or failing the PR for it. + console.error( + `parse-cache version check: could not read ${CACHE_FILE} at ${BASE_REF}.\n` + + `Fetch the base branch before running this (actions/checkout needs ` + + `fetch-depth: 0, or an explicit \`git fetch origin main\`).`, + ); + process.exit(2); +} + +const headSource = readFileSync(CACHE_FILE, 'utf8'); +const base = readBump(baseSource, BASE_REF); +const head = readBump(headSource, 'the working tree'); + +// A branch that does not touch this file cannot collide with anything, and +// must not be asked to bump a version it has no reason to change. Only a branch +// that MODIFIES the parse cache is claiming a version, so only it has to prove +// the claim is unique. +// +// This is also why the check stays useful after both PRs are open: while base +// is still 45 two branches can both claim 46 and both pass, exactly as the +// ledger describes. What catches it is CI re-running once the first one merges +// and base becomes 46 — which is the "RE-CHECK AGAINST origin/main IMMEDIATELY +// BEFORE MERGING" step, now automatic instead of remembered. +if (headSource === baseSource) { + console.log( + `parse-cache version check: OK — ${CACHE_FILE} is unchanged from ${BASE_REF} ` + + `(SCHEMA_BUMP ${head}), so this branch claims no version.`, + ); + process.exit(0); +} + +if (head > base) { + console.log(`parse-cache version check: OK — SCHEMA_BUMP ${head} > ${BASE_REF} ${base}.`); + process.exit(0); +} + +const verdict = + head === base + ? `${CACHE_FILE} changed on this branch, but SCHEMA_BUMP is ${head} on BOTH this ` + + `branch and ${BASE_REF}.` + : `SCHEMA_BUMP is ${head} here but ${base} on ${BASE_REF} — it has gone BACKWARDS.`; + +console.error( + `parse-cache version check: FAILED\n\n` + + ` ${verdict}\n\n` + + ` Two divergent capture schemas would share one PARSE_CACHE_VERSION. The durable\n` + + ` ParsedFile store treats that string as its only invalidator, so one side's\n` + + ` parse-time change would replay pre-change ParsedFiles on every incremental\n` + + ` analyze. Nothing fails; the graph is just missing edges.\n\n` + + ` Fix: raise SCHEMA_BUMP in ${CACHE_FILE} to ${base + 1} or higher, move the pin in\n` + + ` gitnexus/test/unit/incremental-parse-cache.test.ts to match, and add a ledger\n` + + ` entry above the constant recording BOTH claimants.\n\n` + + ` The pin test cannot catch this on its own: both branches assert the same\n` + + ` number and both pass. That is why this check compares against ${BASE_REF}.`, +); +process.exit(1);