From 9304454a76e96aced4cf56ddec38c85377159c41 Mon Sep 17 00:00:00 2001 From: Day Dreamer <87640074+HugoXK@users.noreply.github.com> Date: Sat, 31 Oct 2026 13:22:29 +0800 Subject: [PATCH] Fail CI when generate would rewrite the patch series (#1890) check proves the committed series applies and reproduces core/. It does not prove the series is the one generate writes: #1883 left a split where generate rewrites 0001/0002/0005/0008/0012 and folds 0018 away, check stayed green on that tree, and #1889 had to regenerate the series by hand to get back to canonical. Nothing in CI could see the difference, so the same drift can land again. The generator gains `canonical`: it regenerates into a scratch copy of patches/ds-bot/ and fails, naming each M/D/A file, when the committed series is not what generate writes. The working tree is never touched. `.github/workflows/test.yml` and `npm run verify` run it beside `check`, and the test pins both halves - a drifted series fails the gate, a clean tree passes it and prints no diff - so a gate that was removed or skipped cannot hide behind silence. The generator also stops trusting `git ls-files` and `git write-tree` when this directory is not a repository root: a copy of the tree inside another checkout answered with the outer repository's index and produced a phantom drift, and the applier now matches that rule. --- .github/scripts/ds-bot-patches.test.mjs | 44 +++++++++++ .github/workflows/test.yml | 2 + .../1890-generate-canonical-gate.fixed.md | 1 + docs/subsystems/ds-bot-core.md | 1 + package.json | 2 +- scripts/ds-bot-patches.mjs | 75 +++++++++++++++---- 6 files changed, 109 insertions(+), 16 deletions(-) create mode 100644 changelog.d/1890-generate-canonical-gate.fixed.md diff --git a/.github/scripts/ds-bot-patches.test.mjs b/.github/scripts/ds-bot-patches.test.mjs index bf715adf6..a1909a790 100644 --- a/.github/scripts/ds-bot-patches.test.mjs +++ b/.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 }) } + }) +} diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index cb4042045..e4c3d7e41 100644 --- a/.github/workflows/test.yml +++ b/.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) diff --git a/changelog.d/1890-generate-canonical-gate.fixed.md b/changelog.d/1890-generate-canonical-gate.fixed.md new file mode 100644 index 000000000..d81c49c0a --- /dev/null +++ b/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)) diff --git a/docs/subsystems/ds-bot-core.md b/docs/subsystems/ds-bot-core.md index a95d32a23..a8f3faa93 100644 --- a/docs/subsystems/ds-bot-core.md +++ b/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`. diff --git a/package.json b/package.json index 9dfba3784..e49d35720 100644 --- a/package.json +++ b/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", diff --git a/scripts/ds-bot-patches.mjs b/scripts/ds-bot-patches.mjs index 4faa4d37b..36ba0d01c 100644 --- a/scripts/ds-bot-patches.mjs +++ b/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 ` 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') }