Fail CI when generate would rewrite the patch series

.github/scripts/ds-bot-patches.test.mjs

@@ -234,3 +234,47 @@ if (gitAvailable) {
     }
   })
 }
+
+// #1890: `check` answers "does the committed series apply and reproduce core/", not "would a fresh
+// generate change anything", so a drifted split lands green — it did between #1883 and #1889, where
+// generate rewrote 0001/0002/0005/0008/0012 and deleted 0018 while check stayed ok. The gate is a
+// pair on purpose: the drifted case proves it catches the shape, and the clean case proves it runs at
+// all and says nothing but a pass line, so a gate removed or skipped cannot hide as silence.
+if (gitAvailable) {
+  const scratchRepo = () => {
+    const root = join(import.meta.dirname, '..', '..')
+    const dir = mkdtempSync(join(tmpdir(), 'ds-bot-canonical-'))
+    for (const entry of ['scripts', 'ds-bot', 'ds-bot.pin.json', 'patches', 'core', '.gitignore']) {
+      cpSync(join(root, entry), join(dir, entry), { recursive: true, verbatimSymlinks: true })
+    }
+    return dir
+  }
+  const runCanonical = dir => {
+    try {
+      const out = execFileSync(process.execPath, [join(dir, 'scripts', 'ds-bot-patches.mjs'), 'canonical'], { cwd: dir, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] })
+      return { code: 0, out }
+    } catch (error) { return { code: error.status ?? 1, out: `${error.stdout ?? ''}${error.stderr ?? ''}` } }
+  }
+
+  test('#1890: a drifted patch series fails the gate, naming what generate would rewrite', () => {
+    const dir = scratchRepo()
+    try {
+      writeFileSync(join(dir, 'patches', 'ds-bot', '0002-flybot-api-host.patch'), '\n# drifted by the test\n', { flag: 'a' })
+      const { code, out } = runCanonical(dir)
+      assert.notEqual(code, 0, 'a drifted series must not pass the gate')
+      assert.match(out, /not what generate writes/)
+      assert.match(out, /0002-flybot-api-host\.patch/)
+      assert.doesNotMatch(out, /^ok /m, 'a drifted series cannot print the pass line')
+    } finally { rmSync(dir, { recursive: true, force: true }) }
+  })
+
+  test('#1890: a clean tree passes the gate, and the gate prints no diff', () => {
+    const dir = scratchRepo()
+    try {
+      const { code, out } = runCanonical(dir)
+      assert.equal(code, 0, out)
+      assert.match(out, /^ok /m)
+      assert.doesNotMatch(out, /^(diff --git |--- a\/|\+\+\+ b\/|@@ )/m, 'a clean tree prints no diff')
+    } finally { rmSync(dir, { recursive: true, force: true }) }
+  })
+}

.github/workflows/test.yml

@@ -100,6 +100,8 @@ jobs:
           node-version: 24
       - name: ds-bot/ matches the pin, patches apply, core/ is ds-bot/ + patches (#34)
         run: node scripts/ds-bot-patches.mjs check
+      - name: generate is a no-op on the committed patch split (#1890)
+        run: node scripts/ds-bot-patches.mjs canonical
       - name: Pack and check core (ds-bot), plugins/social, plugins/tree, plugins/timeline, plugins/workflow
         run: node .github/scripts/check-pack.mjs
       - name: Typecheck the plugin API (types/*.d.ts, strict)

changelog.d/1890-generate-canonical-gate.fixed.md

@@ -0,0 +1 @@
+CI now runs `node scripts/ds-bot-patches.mjs canonical`, which fails when `generate` would rewrite `patches/ds-bot/`: `check` proves the committed series applies and reproduces `core/`, not that it is the split the generator writes, so a drifted series landed green between #1883 and #1889. `npm run verify` runs the same gate. ([#1890](https://github.com/ThinkFlowLab/FlyBot/issues/1890))

docs/subsystems/ds-bot-core.md

@@ -56,6 +56,7 @@ holds. Task bookkeeping does not imply runtime execution or tool permission.
 
 1. `ds-bot/` stays pristine; CI fails if it differs from the pin.
 2. `core/` == `ds-bot/` + `patches/ds-bot/files/` + `patches/ds-bot/*.patch`; regenerate after every `core/` change. A new file under `core/` (for example `test/new-chat.test.js`) is stored whole in `patches/ds-bot/files/` by `generate` and needs no `topics.json` entry; an upstream file edited for the first time must be listed in `patches/ds-bot/topics.json`. Otherwise `scripts/ds-bot-patches.mjs check` fails with “committed but not produced by the patches”. The format has no `index` lines or cross-hunk line numbers so parallel PRs merge cleanly (see `patches/ds-bot/README.md`).
+   `check` does not mean the split is the one `generate` writes: a series re-split between topics still applies and still reproduces `core/`. `scripts/ds-bot-patches.mjs canonical` is the other half — it regenerates into a scratch copy and fails when the committed `patches/ds-bot/` would change — and both `.github/workflows/test.yml` and `npm run verify` run it (#1890).
    `check` also runs on a machine without `git` (#978), through the script's own applier at `git apply`'s strictness: a hunk body must carry the counts its header declares, must match at the old-side line that header names (no search for nearby context — the loose applier that did let `#934`'s extra blank line and `#966`'s 662nd line through was only caught by CI), may not run past the end of the file, and honours `\ No newline at end of file`. There the pin prints `skipped pin (no git): the tree hash cannot be computed here` instead of passing, and `generate` still needs git because it asks git for the diff. CI keeps the git path as the reference, and a test builds the series with both appliers and compares the trees.
 3. `client.js` matches `core/src/client/`; `npm run build` + `--check`.
 

package.json

@@ -6,7 +6,7 @@
     "node": ">=22"
   },
   "scripts": {
-    "verify": "npm --prefix core run lint && node scripts/lint-plugins.mjs && node core/scripts/build-client.mjs --check && node scripts/ds-bot-patches.mjs check && npm --prefix core test && npm --prefix plugins/social test && npm --prefix plugins/tree test && npm --prefix plugins/timeline test && npm --prefix plugins/workflow test && node --test \".github/scripts/*.test.mjs\" && npm run lint:interop && npm run test:interop && npm run lint:live-e2e && npm run test:live-e2e",
+    "verify": "npm --prefix core run lint && node scripts/lint-plugins.mjs && node core/scripts/build-client.mjs --check && node scripts/ds-bot-patches.mjs check && node scripts/ds-bot-patches.mjs canonical && npm --prefix core test && npm --prefix plugins/social test && npm --prefix plugins/tree test && npm --prefix plugins/timeline test && npm --prefix plugins/workflow test && node --test \".github/scripts/*.test.mjs\" && npm run lint:interop && npm run test:interop && npm run lint:live-e2e && npm run test:live-e2e",
     "lint:interop": "node scripts/lint-interop.mjs",
     "test:interop": "node --import ./dev/interop/no-network.mjs --test \"dev/interop/test/*.test.mjs\"",
     "demo:interop": "node --import ./dev/interop/no-network.mjs dev/interop/demo.mjs",

scripts/ds-bot-patches.mjs

@@ -8,6 +8,11 @@
 //   node scripts/ds-bot-patches.mjs generate   rewrite patches/ds-bot/ from core/ vs ds-bot/ (after editing
 //                                              core/): *.patch for edits to upstream files, split by
 //                                              topics.json; files/ for files upstream does not have
+//   node scripts/ds-bot-patches.mjs canonical  CI gate (#1890): fail when generate would change
+//                                              patches/ds-bot/, i.e. when the committed split drifted.
+//                                              `check` cannot answer this — a re-split series still
+//                                              reproduces core/ — and that gap landed #1883..#1889 green.
+//                                              Changes nothing on disk: generate writes to a scratch copy.
 //
 // The format is chosen so that two branches editing the same core file merge without conflicts in
 // the generated output (see patches/ds-bot/README.md): no `index <hash>` lines, hunk headers that
@@ -26,7 +31,7 @@
 // git keeps the git path: CI is the reference, and the test compares both appliers on every patch.
 // `generate` needs git either way, because it asks git for the diff.
 import { execFileSync } from 'node:child_process'
-import { chmodSync, cpSync, existsSync, mkdtempSync, mkdirSync, readFileSync, readdirSync, rmSync, statSync, writeFileSync } from 'node:fs'
+import { chmodSync, cpSync, existsSync, mkdtempSync, mkdirSync, readFileSync, readdirSync, realpathSync, rmSync, statSync, writeFileSync } from 'node:fs'
 import { tmpdir } from 'node:os'
 import { dirname, join, relative } from 'node:path'
 import { fileURLToPath } from 'node:url'
@@ -39,13 +44,23 @@ const PATCHES = join(ROOT, 'patches', 'ds-bot')
 const FILES = join(PATCHES, 'files') // whole files FlyBot adds (or replaces, when binary): copied over ds-bot/ before the patches apply
 const git = (args, opts = {}) => execFileSync('git', args, { cwd: ROOT, encoding: 'utf8', maxBuffer: 1 << 28, ...opts })
 const hasGit = (() => { try { execFileSync('git', ['--version'], { stdio: 'ignore' }); return true } catch { return false } })()
+// `git ls-files` and `git write-tree` answer only when this directory is the root of its own
+// repository. A copy of the tree inside another checkout answers with the outer repository's index
+// and says nothing about these files (it produced a phantom drift on a copied tree while #1890 was
+// built), so the root has to be the repository's top level; anything else uses the walk below.
+const inRepo = hasGit && (() => {
+  try {
+    const top = execFileSync('git', ['rev-parse', '--show-toplevel'], { cwd: ROOT, encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }).trim()
+    return realpathSync(top) === realpathSync(ROOT)
+  } catch { return false }
+})()
 const series = () => readdirSync(PATCHES).filter(f => /^\d{4}-.*\.patch$/.test(f)).sort()
 const fail = (message) => { console.error(`ds-bot-patches: ${message}`); process.exit(1) }
 
 // The tracked files of a directory. git answers on a machine that has it; without git the walk below
 // answers, and the root .gitignore decides what is not in the repository. The walk is a list of what
 // is on disk, which is what `ls-files` reports too before a commit, apart from ignored files.
-const tracked = dir => (hasGit ? git(['ls-files', '-z', '--', dir]).split('\0').filter(Boolean).map(f => relative(dir, f)) : walkTracked(dir))
+const tracked = dir => (inRepo ? git(['ls-files', '-z', '--', dir]).split('\0').filter(Boolean).map(f => relative(dir, f)) : walkTracked(dir))
 
 const globToRegExp = glob => new RegExp(`^${glob.split('*').map(part => part.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')).join('[^/]*')}$`)
 
@@ -228,7 +243,7 @@ function build(out) {
     cpSync(join(FILES, file), join(ROOT, out, file))
   }
   for (const patch of series()) {
-    if (!hasGit) {
+    if (!inRepo) {
       try { applyPatchText(readFileSync(join(PATCHES, patch), 'utf8'), join(ROOT, out)) } catch (error) { fail(`${patch} does not apply cleanly:\n${error.message}`) }
       continue
     }
@@ -239,9 +254,9 @@ function build(out) {
 }
 
 function checkPin() {
-  if (!hasGit) {
+  if (!inRepo) {
     // Not a pass: the tree hash cannot be computed here, so say what was skipped (#978).
-    console.log(`skipped pin (no git): the tree hash cannot be computed here`)
+    console.log(hasGit ? 'skipped pin (not a git checkout at the root): the tree hash cannot be computed here' : 'skipped pin (no git): the tree hash cannot be computed here')
     return
   }
   // The tree of ds-bot/ as staged (equals HEAD in CI), so the check also works before a commit.
@@ -329,11 +344,14 @@ function diffReproduces(file, diff, hadUpstream) {
   } catch { return false } finally { rmSync(dir, { recursive: true, force: true }) }
 }
 
-function generate() {
+// `dest` is patches/ds-bot/ itself, or a scratch copy of it for `canonical` (#1890); `quiet` keeps
+// that gate's output down to its one pass line.
+function generate(dest = PATCHES, { quiet = false } = {}) {
   // The diff itself comes from git. Without git there is nothing to ask for it: the git-free applier
   // (#978) is a validator, not a diff writer.
   if (!hasGit) fail('generate needs git: it asks git for the diff of ds-bot/ against core/. Without git, edit core/ and patches/ds-bot/ together and run check.')
-  const topics = JSON.parse(readFileSync(join(PATCHES, 'topics.json'), 'utf8'))
+  const say = quiet ? () => {} : console.log
+  const topics = JSON.parse(readFileSync(join(dest, 'topics.json'), 'utf8'))
   const upstream = new Set(tracked(UPSTREAM))
   const core = new Set(tracked(CORE))
   const changed = [...new Set([...upstream, ...core])].sort().filter(f => !upstream.has(f) || !core.has(f) ||
@@ -356,29 +374,56 @@ function generate() {
     return fail(`the diff of ${f} does not reproduce core/${f} when applied to ds-bot/${f}`)
   }
   const texts = new Map(edited.map(f => [f, textOf(f)]))
-  rmSync(FILES, { recursive: true, force: true })
-  for (const old of series()) rmSync(join(PATCHES, old))
+  const filesOut = join(dest, 'files')
+  rmSync(filesOut, { recursive: true, force: true })
+  for (const old of readdirSync(dest)) if (/^\d{4}-.*\.patch$/.test(old)) rmSync(join(dest, old))
   for (const f of whole) {
-    mkdirSync(dirname(join(FILES, f)), { recursive: true })
-    cpSync(join(ROOT, CORE, f), join(FILES, f))
+    mkdirSync(dirname(join(filesOut, f)), { recursive: true })
+    cpSync(join(ROOT, CORE, f), join(filesOut, f))
   }
-  if (whole.length) console.log(`wrote patches/ds-bot/files/ (${whole.length} files)`)
+  if (whole.length) say(`wrote patches/ds-bot/files/ (${whole.length} files)`)
   for (const topic of Object.keys(topics)) {
     const files = edited.filter(f => topicOf(f) === topic)
     if (!files.length) continue
     let text = `FlyBot patch ${topic} on ${PIN.repo}@${PIN.commit.slice(0, 7)} (${UPSTREAM}/). Generated by scripts/ds-bot-patches.mjs; edits to upstream files only (files FlyBot adds are in files/).\n\n`
     for (const f of files) text += texts.get(f)
-    writeFileSync(join(PATCHES, `${topic}.patch`), text)
-    console.log(`wrote patches/ds-bot/${topic}.patch (${files.length} files)`)
+    writeFileSync(join(dest, `${topic}.patch`), text)
+    say(`wrote patches/ds-bot/${topic}.patch (${files.length} files)`)
   }
 }
 
+// The gate #1890 asked for: is the committed series what `generate` writes? It regenerates into a
+// scratch copy of patches/ds-bot/ — never into the working tree — and compares the two file for file,
+// so a drift that still reproduces core/ (a re-split topic, a patch folded away) fails here and not
+// in the merge queue. Only the files generate owns are compared; topics.json is hand-written.
+function canonical() {
+  if (!hasGit) fail('canonical needs git: it compares the committed series with what generate writes, and generate asks git for the diff')
+  const scratch = mkdtempSync(join(tmpdir(), 'ds-bot-canonical-'))
+  try {
+    const staged = join(scratch, 'patches')
+    cpSync(PATCHES, staged, { recursive: true })
+    generate(staged, { quiet: true })
+    const owned = dir => readdirTree(dir).filter(f => /^\d{4}-.*\.patch$/.test(f) || f.split(/[\\/]/)[0] === 'files')
+    const committed = new Set(owned(PATCHES))
+    const produced = new Set(owned(staged))
+    const drift = []
+    for (const file of [...committed].sort()) {
+      if (!produced.has(file)) drift.push(`D patches/ds-bot/${file}`)
+      else if (!readFileSync(join(PATCHES, file)).equals(readFileSync(join(staged, file)))) drift.push(`M patches/ds-bot/${file}`)
+    }
+    for (const file of [...produced].sort()) if (!committed.has(file)) drift.push(`A patches/ds-bot/${file}`)
+    if (drift.length) fail(`patches/ds-bot/ is not what generate writes — the committed series drifted:\n  ${drift.join('\n  ')}\nrun \`node scripts/ds-bot-patches.mjs generate\` and commit its output`)
+    console.log('ok   patches/ds-bot/ is what generate writes (generate is a no-op)')
+  } finally { rmSync(scratch, { recursive: true, force: true }) }
+}
+
 if (process.argv[1] && fileURLToPath(import.meta.url) === process.argv[1]) {
   const command = process.argv[2]
   if (!existsSync(PATCHES)) fail('patches/ds-bot/ is missing')
   if (command === 'apply') { build(CORE); console.log(`core/ rebuilt from ${UPSTREAM}/ + files/ + ${series().length} patches`) }
   else if (command === 'check') check()
   else if (command === 'generate') generate()
+  else if (command === 'canonical') canonical()
   else if (command === 'pin') checkPin()
-  else fail('usage: node scripts/ds-bot-patches.mjs apply|check|generate|pin')
+  else fail('usage: node scripts/ds-bot-patches.mjs apply|check|generate|canonical|pin')
 }