Merge 29e735e5d1 into a049b2dac6

.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

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');
+  });
+});

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 = <n>;\` 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);