From 29e735e5d138e55f4223b9e78b0d603916340cd8 Mon Sep 17 00:00:00 2001 From: ReidenXerx Date: Thu, 6 Aug 2026 22:59:15 +0300 Subject: [PATCH] ci: fail a PR whose SCHEMA_BUMP collides with the base branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up, and the reviewer's own framing is the argument: a literal pin cannot structurally close this class. Both branches assert `toBe(N)`, both are green, and the collision exists only in the RELATION between them — which no single-branch test can see. The ledger records ten entries and four exact clashes, every one caught by hand at merge time and one only after it shipped. Failure is silent, which is why it recurs. `PARSE_CACHE_VERSION` is the sole invalidator for the durable ParsedFile store, so two divergent capture schemas sharing one version string means one side's parse-time change replays pre-change ParsedFiles on every incremental analyze. Nothing throws; the graph is just missing edges — the confident-empty answer this series exists to remove. Only a branch that MODIFIES the parse cache is claiming a version, so only that branch has to prove the claim is unique; anything else passes untouched. The first version of this check required a strict increase unconditionally, which would have failed every PR that does not touch the file — including this one. It failed on its own branch, which is how that got caught. Being open at the same time is not the collision. 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 merges and base becomes 46. That is the "RE-CHECK AGAINST origin/main IMMEDIATELY BEFORE MERGING" step the ledger already demands, now automatic instead of remembered. Three outcomes, deliberately distinct: pass, conflict (exit 1, naming the number to move to and the two other files to update), and unreachable base ref (exit 2), so a misconfigured shallow checkout cannot masquerade as a pass — the failure mode a naive version of this check would have. Tested against real git state rather than mocks, because the thing under test IS a comparison between branches: a temp repo with a base at 45, then an untouched branch, 46, 45-with-the-file-edited, 44, an unreachable ref, and a renamed declaration that must fail loudly rather than silently matching nothing. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci-quality.yml | 24 ++++ .../unit/parse-cache-version-check.test.ts | 131 ++++++++++++++++++ scripts/check-parse-cache-version.mjs | 110 +++++++++++++++ 3 files changed, 265 insertions(+) create mode 100644 gitnexus/test/unit/parse-cache-version-check.test.ts create mode 100644 scripts/check-parse-cache-version.mjs diff --git a/.github/workflows/ci-quality.yml b/.github/workflows/ci-quality.yml index 182c620d2..9c68269a4 100644 --- a/.github/workflows/ci-quality.yml +++ b/.github/workflows/ci-quality.yml @@ -22,6 +22,30 @@ jobs: - run: npm ci - 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);