diff --git a/.gstack/pregate.json b/.gstack/pregate.json index 608163b2f..1246f74c7 100644 --- a/.gstack/pregate.json +++ b/.gstack/pregate.json @@ -39,7 +39,7 @@ { "id": "module-size", "when": ["lib/**/*.ts", "bin/**", "scripts/**/*.ts", "browse/src/**/*.ts", "test/helpers/**/*.ts", "test/module-size-ratchet.test.ts", "test/fixtures/module-size-ratchet.json"], "command": "bun test test/module-size-ratchet.test.ts", "fix": "split the module or function the failure names; it may not grow past its recorded size" }, { "id": "goldens", "when": ["SKILL.md", "*/SKILL.md", "*/sections/*.md", "SKILL.md.tmpl", "*/SKILL.md.tmpl", "*/sections/*.tmpl", "*/sections/manifest.json", "*/templates/*.tmpl", "hosts/**", "model-overlays/**", "scripts/gen-skill-docs.ts", "scripts/resolvers/**", "ship/**", "scripts/gen-ship-goldens.ts", "test/host-config.test.ts", "test/fixtures/golden/**"], "command": "bun test test/host-config.test.ts", "fix": "when the render change is intended, run bun scripts/gen-ship-goldens.ts and commit the updated goldens" }, { "id": "doc-caps", "when": ["CLAUDE.md", "test/claude-md-size.test.ts"], "command": "bun test test/claude-md-size.test.ts", "fix": "move contributor-only detail from CLAUDE.md into docs/ and leave a one-line pointer" }, - { "id": "conflict-markers", "when": ["**/*.ts", "**/*.tsx", "**/*.js", "**/*.mjs", "**/*.cjs", "**/*.sh", "**/*.json", "**/*.yml", "**/*.yaml", "**/*.tmpl", "bin/**"], "command": "bun --no-env-file --config=/dev/null scripts/check-conflict-markers.ts", "fix": "finish the merge in each file it names and delete the marker lines; a fixture that shows markers on purpose declares gstack-conflict-markers: fixture" } + { "id": "conflict-markers", "when": ["**/*.ts", "**/*.tsx", "**/*.js", "**/*.mjs", "**/*.cjs", "**/*.sh", "**/*.json", "**/*.yml", "**/*.yaml", "**/*.tmpl", "bin/**"], "command": "bun scripts/check-conflict-markers.ts", "fix": "finish the merge in each file it names, delete the marker lines and commit; a fixture that shows markers on purpose declares itself as docs/pregate.md describes" } ], "strays": { "allow": ["modules/loadouts.json", "modules/loadouts.schema.json"] } } diff --git a/changelog.d/gsta258-l6-conflict-markers.md b/changelog.d/gsta258-l6-conflict-markers.md index 85ca4f682..4b87b93ad 100644 --- a/changelog.d/gsta258-l6-conflict-markers.md +++ b/changelog.d/gsta258-l6-conflict-markers.md @@ -1,4 +1,4 @@ -- Pushes from /ship now stop on leftover merge conflict markers. A new `conflict-markers` guard reads every line the branch adds and refuses one that starts with `<<<<<<<` or `>>>>>>>`, or is exactly `=======`, even inside a template literal that still parses and passes its tests (GST-1190, seen on #3927). Markdown code blocks that show a conflict are skipped, and a fixture that shows markers on purpose declares `gstack-conflict-markers: fixture`. +- Pushes from /ship now stop on leftover merge conflict markers. A new `conflict-markers` guard reads every line the branch adds, in the commit being pushed and in the working tree, and refuses one that starts with `<<<<<<<` or `>>>>>>>`, or is exactly `=======`, even inside a template literal that still parses and passes its tests (GST-1190, seen on #3927). Markdown code blocks that show git output (an unlabeled, `text` or `diff` fence) are skipped, while a conflict inside a `bash` or `ts` fence is still refused. A fixture that shows markers on purpose declares `gstack-conflict-markers: fixture` on a line of its own near the top. ### Tests -- `test/conflict-markers.test.ts`: a marker inside a template literal is refused (it failed before the guard existed); a Markdown code block that shows a marker passes while the same lines outside a fence are refused; unchanged lines, declared fixtures and uncommitted files are handled. +- `test/conflict-markers.test.ts`: a marker inside a template literal is refused (it failed before the guard existed); a Markdown code block that shows a marker passes while the same lines outside a fence or in a bash fence are refused; unchanged lines, declared fixtures, untracked files, quoted file names, git prefix settings and a committed conflict hidden by an unstaged repair are handled. diff --git a/docs/pregate.md b/docs/pregate.md index b69bde8df..0192eb467 100644 --- a/docs/pregate.md +++ b/docs/pregate.md @@ -1365,13 +1365,18 @@ gstack declares its own list in `.gstack/pregate.json`: `typecheck`, `doc-caps` and `conflict-markers`. Generated SKILL.md freshness is the `regen` check's job. `conflict-markers` runs `scripts/check-conflict-markers.ts` on a change to code, templates or config, and reads every line the branch adds -against its merge base with `origin/main`. A line starting with `<<<<<<<` or -`>>>>>>>` (then a space or the line end), or a line that is exactly `=======`, -stops the push, even inside a template literal the parser accepts. In Markdown -it skips fenced code blocks and counts `=======` only between an unfenced -`<<<<<<<` and its close, so a code block that shows a conflict and a heading -underline both pass. A fixture that shows markers on purpose carries a line -with `gstack-conflict-markers: fixture`. Other +against its merge base with `origin/main`, both in the committed HEAD (what the +push publishes) and in the working tree's tracked files. A line starting with +`<<<<<<<` or `>>>>>>>` (then a space or the line end), or a line that is exactly +`=======`, stops the push, even inside a template literal the parser accepts. +In Markdown (`.md`, `.mdx`, `.markdown`, `.md.tmpl`) it skips fences whose info +string is empty, `text`, `txt`, `plaintext`, `diff` or `console`, the fences a +doc uses to show git output, and counts `=======` only between a `<<<<<<<` and +its close, so such a code block and a heading underline both pass; a conflict +inside a `bash` or `ts` fence is still refused. A fixture that shows markers on +purpose puts a line that is only `gstack-conflict-markers: fixture` (a comment +leader such as `#` or `//` is fine) in its first five lines; the phrase +anywhere else exempts nothing. Other repos add their own `guards` the same way; the key needs no newer gstack than the one that already reads `guards`. diff --git a/scripts/check-conflict-markers.ts b/scripts/check-conflict-markers.ts index 6b7c016ff..4389ef0e6 100755 --- a/scripts/check-conflict-markers.ts +++ b/scripts/check-conflict-markers.ts @@ -4,72 +4,102 @@ * * bun scripts/check-conflict-markers.ts [--base ] * - * Reads every line the working tree adds against the merge base with (default origin/main): committed, - * staged, unstaged and untracked, deleted files skipped. A line starting with `<<<<<<<` or `>>>>>>>` followed - * by a space or the line end, or a line that is exactly `=======`, is a marker. This catches a marker the - * parser accepts, such as one inside a template literal (#3927). Markdown files skip lines inside a ``` or ~~~ - * fence, and count `=======` only between an unfenced `<<<<<<<` and its close, so a setext heading underline - * and a code block that shows a conflict both pass. A file with a line containing - * `gstack-conflict-markers: fixture` is skipped. Prints one CONFLICT_MARKER: : per hit - * and exits 1, or CONFLICT_MARKERS: none and exits 0; a git failure exits 2. + * Reads every line the branch adds against its merge base with (default origin/main) twice: in the + * committed HEAD, which is what a push publishes, and in the working tree's tracked files (deleted files are + * skipped; untracked files are never pushed, so they are not read). A line starting with `<<<<<<<` or + * `>>>>>>>` followed by a space or the line end, or a line that is exactly `=======`, is a marker. This + * catches a marker the parser accepts, such as one inside a template literal (#3927). Markdown files + * (`.md`, `.mdx`, `.markdown`, `.md.tmpl`) skip lines inside a ``` or ~~~ fence whose info string is empty, + * text, txt, plaintext, diff or console, the fences a doc uses to show git output; a conflict inside a bash, + * ts or other code fence is still refused. They count `=======` only between a `<<<<<<<` and its close, so a + * setext heading underline passes. A file whose first five lines include one that is only the declaration + * `gstack-conflict-markers: fixture` (after an optional comment leader such as `#`, `//` or `)?\s*$/; +const GIT = ['-c', 'core.quotePath=false', '-c', 'diff.noprefix=false', '-c', 'diff.mnemonicPrefix=false']; -function git(args: string[]): string { - const r = spawnSync('git', args, { encoding: 'utf8', maxBuffer: 256 * 1024 * 1024, timeout: 120_000 }); - if (r.status !== 0) { console.error(`check-conflict-markers: git ${args.join(' ')} failed: ${(r.stderr || r.error?.message || '').trim()}`); process.exit(2); } - return r.stdout; +function git(args: string[], allowFail = false): string | null { + const r = spawnSync('git', [...GIT, ...args], { encoding: 'utf8', maxBuffer: 256 * 1024 * 1024, timeout: 120_000 }); + if (r.status === 0) return r.stdout; + if (allowFail) return null; + console.error(`check-conflict-markers: git ${args.join(' ')} failed: ${(r.stderr || r.error?.message || '').trim()}`); + process.exit(2); } -const i = process.argv.indexOf('--base'); -const base = i > 0 ? process.argv[i + 1]! : 'origin/main'; -const root = git(['rev-parse', '--show-toplevel']).trim(); -process.chdir(root); -const fork = git(['merge-base', base, 'HEAD']).trim(); - -const added = new Map>(); -let file = ''; -for (const line of git(['diff', '-U0', '--no-color', '--no-ext-diff', '--no-renames', '--diff-filter=AM', fork, '--']).split('\n')) { - if (line.startsWith('+++ ')) { file = line.startsWith('+++ b/') ? line.slice(6) : ''; continue; } - const hunk = /^@@ -\S+ \+(\d+)(?:,(\d+))? @@/.exec(line); - if (!hunk || !file) continue; - const start = Number(hunk[1]), count = hunk[2] === undefined ? 1 : Number(hunk[2]); - const set = added.get(file) ?? new Set(); - for (let n = start; n < start + count; n++) set.add(n); - added.set(file, set); +/** The added line numbers per path, from a zero-context diff whose headers name paths with fixed a/ and b/ prefixes. */ +function addedLines(diff: string): Map> { + const added = new Map>(); + let file = '', header = false; + for (const line of diff.split('\n')) { + if (line.startsWith('diff --git ')) { header = true; file = ''; continue; } + if (header && line.startsWith('+++ ')) { file = line.startsWith('+++ b/') ? line.slice(6).replace(/\t$/, '') : ''; header = false; continue; } + if (header) continue; + const hunk = /^@@ -\S+ \+(\d+)(?:,(\d+))? @@/.exec(line); + if (!hunk || !file) continue; + const start = Number(hunk[1]), count = hunk[2] === undefined ? 1 : Number(hunk[2]); + const set = added.get(file) ?? new Set(); + for (let n = start; n < start + count; n++) set.add(n); + added.set(file, set); + } + return added; } -for (const f of git(['ls-files', '--others', '--exclude-standard', '-z']).split('\0').filter(Boolean)) added.set(f, new Set([Infinity])); -const hits: string[] = []; -for (const [f, lines] of [...added].sort(([a], [b]) => a.localeCompare(b))) { - let text: string; - try { text = fs.readFileSync(path.join(root, f), 'utf8'); } catch { continue; } - if (text.includes('\0') || text.includes(FIXTURE)) continue; +const seen = new Set(); +function scan(f: string, text: string, lines: Set, where: string, hits: Set): void { + const all = text.split('\n'); + if (text.includes('\0') || all.slice(0, 5).some(l => DECLARATION.test(l))) return; const md = MARKDOWN.test(f); - let fence = '', open = false; - text.split('\n').forEach((raw, idx) => { + let fence = '', shown = false, open = false; + all.forEach((raw, idx) => { const l = raw.replace(/\r$/, ''); if (md) { const m = FENCE.exec(l); - if (m && (!fence || (m[1]![0] === fence[0] && m[1]!.length >= fence.length))) { fence = fence ? '' : m[1]!; return; } - if (fence) return; + if (m && !fence) { fence = m[1]!; shown = SHOWN.test(m[2]!); return; } + if (m && !m[2] && m[1]![0] === fence[0] && m[1]!.length >= fence.length) { fence = ''; shown = false; return; } + if (shown) return; } const marker = OPEN.test(l) ? '<'.repeat(7) : CLOSE.test(l) ? '>'.repeat(7) : MID.test(l) && (!md || open) ? '='.repeat(7) : ''; if (OPEN.test(l)) open = true; if (CLOSE.test(l)) open = false; - if (marker && (lines.has(Infinity) || lines.has(idx + 1))) hits.push(`CONFLICT_MARKER: ${f}:${idx + 1} ${marker}`); + if (!marker || !lines.has(idx + 1) || seen.has(`${f}:${idx + 1}`)) return; + seen.add(`${f}:${idx + 1}`); + hits.add(`CONFLICT_MARKER: ${f}:${idx + 1} ${marker}${where}`); }); } -if (hits.length) { - console.log(hits.join('\n')); - console.log(`fix: finish the merge in each file above (keep the right side, delete the marker lines), or add a line with "${FIXTURE}" to a fixture that shows markers on purpose`); + +const i = process.argv.indexOf('--base'); +const base = i > 0 ? process.argv[i + 1]! : 'origin/main'; +const root = git(['rev-parse', '--show-toplevel'])!.trim(); +process.chdir(root); +const fork = git(['merge-base', base, 'HEAD'])!.trim(); +const DIFF = ['diff', '-U0', '--no-color', '--no-ext-diff', '--no-renames', '--src-prefix=a/', '--dst-prefix=b/', '--diff-filter=AM']; + +const hits = new Set(); +const committed = addedLines(git([...DIFF, fork, 'HEAD', '--'])!); +for (const [f, lines] of committed) { + const text = git(['show', `HEAD:${f}`], true); + if (text !== null) scan(f, text, lines, '', hits); +} +const working = addedLines(git([...DIFF, fork, '--'])!); +for (const [f, lines] of working) { + let text: string; + try { text = fs.readFileSync(path.join(root, f), 'utf8'); } catch { continue; } + scan(f, text, lines, ' (working tree)', hits); +} +const files = new Set([...committed.keys(), ...working.keys()]).size; +if (hits.size) { + console.log([...hits].sort().join('\n')); + console.log('fix: finish the merge in each file above (keep the right side, delete the marker lines) and commit; a fixture that shows markers on purpose declares the fixture line documented in docs/pregate.md#prepush in its first five lines'); process.exit(1); } -console.log(`CONFLICT_MARKERS: none (${added.size} changed file(s) against ${fork.slice(0, 12)})`); +console.log(`CONFLICT_MARKERS: none (${files} changed file(s) against ${fork.slice(0, 12)})`); diff --git a/test/conflict-markers.test.ts b/test/conflict-markers.test.ts index 32b6808a7..ee05a48f1 100644 --- a/test/conflict-markers.test.ts +++ b/test/conflict-markers.test.ts @@ -3,8 +3,9 @@ import { spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; +import { bunIsolationArgs } from '../lib/bun-isolation'; -const SCRIPT = path.resolve(import.meta.dir, '..', 'scripts', 'check-conflict-markers.ts'); +const SCRIPT = path.resolve(import.meta.dir, '..', 'scripts/check-conflict-markers.ts'); const OPEN = '<'.repeat(7), MID = '='.repeat(7), CLOSE = '>'.repeat(7); const dirs: string[] = []; afterAll(() => { for (const d of dirs) fs.rmSync(d, { recursive: true, force: true }); }); @@ -12,35 +13,39 @@ afterAll(() => { for (const d of dirs) fs.rmSync(d, { recursive: true, force: tr function repo(base: Record = { 'README.md': 'base\n' }) { const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-markers-')); dirs.push(root); - const git = (...args: string[]) => spawnSync('git', args, { cwd: root, encoding: 'utf8', timeout: 30_000 }); - git('init', '-q', '-b', 'main'); - git('config', 'user.email', 'test@example.com'); - git('config', 'user.name', 'test'); + const git = (...args: string[]) => { + const r = spawnSync('git', ['-c', 'commit.gpgsign=false', '-c', 'core.hooksPath=/dev/null', '-c', 'user.email=test@example.com', '-c', 'user.name=test', ...args], { cwd: root, encoding: 'utf8', timeout: 30_000 }); + if (r.status !== 0) throw new Error(`git ${args.join(' ')}: ${r.stderr}`); + return r.stdout; + }; + git('init', '-q'); + git('symbolic-ref', 'HEAD', 'refs/heads/main'); const write = (files: Record) => { for (const [p, c] of Object.entries(files)) { fs.mkdirSync(path.dirname(path.join(root, p)), { recursive: true }); fs.writeFileSync(path.join(root, p), c); } }; write(base); git('add', '-A'); git('commit', '-q', '-m', 'base'); git('checkout', '-q', '-b', 'feat'); const change = (files: Record) => { write(files); git('add', '-A'); git('commit', '-q', '-m', 'change'); }; - const run = () => spawnSync('bun', ['--no-env-file', '--config=/dev/null', SCRIPT, '--base', 'main'], { cwd: root, encoding: 'utf8', timeout: 60_000 }); - return { root, change, write, run }; + const run = (env: Record = {}) => spawnSync('bun', [...bunIsolationArgs(), SCRIPT, '--base', 'main'], { cwd: root, encoding: 'utf8', timeout: 60_000, env: { ...process.env, ...env } }); + return { root, git, change, write, run }; } describe('check-conflict-markers (GST-1190: #3927 kept markers inside a template literal and its tests passed)', () => { // Value: protects=a merge that leaves conflict markers in a changed file is refused before the push, even where the - // markers sit inside a string the parser accepts; fails_when=a marker line in a template literal passes, or a markdown - // code block that shows a marker is refused; why_new=GSTA-234 #3927; seam=a real git repository and the script + // markers sit inside a string the parser accepts; fails_when=a marker line in a template literal passes, a markdown + // code block that shows a marker is refused, or a git display setting hides a file; why_new=GSTA-234 #3927; + // seam=a real git repository and the script test('a marker inside a template literal is refused', () => { const r = repo(); r.change({ 'lib/prompt.ts': `export const PROMPT = \`\n${OPEN} HEAD\nkeep this\n${MID}\nor this\n${CLOSE} origin/main\n\`;\n` }); const out = r.run(); expect(out.status).toBe(1); - expect(out.stdout).toContain(`CONFLICT_MARKER: lib/prompt.ts:2 ${OPEN}`); - expect(out.stdout).toContain(`CONFLICT_MARKER: lib/prompt.ts:4 ${MID}`); - expect(out.stdout).toContain(`CONFLICT_MARKER: lib/prompt.ts:6 ${CLOSE}`); + expect(out.stdout).toContain(`CONFLICT_MARKER: lib/prompt.ts:2 ${OPEN}\n`); + expect(out.stdout).toContain(`CONFLICT_MARKER: lib/prompt.ts:4 ${MID}\n`); + expect(out.stdout).toContain(`CONFLICT_MARKER: lib/prompt.ts:6 ${CLOSE}\n`); }); - test('a markdown code block that shows a marker is not refused; the same lines outside a fence are', () => { + test('a markdown code block that shows a marker is not refused; the same lines outside a fence, or in a bash fence, are', () => { const r = repo(); const block = `${OPEN} HEAD\nours\n${MID}\ntheirs\n${CLOSE} main\n`; r.change({ 'docs/merging.md': `# Merging\n\nGit writes this:\n\n\`\`\`\n${block}\`\`\`\n\nA heading\n${MID}\n` }); @@ -51,14 +56,31 @@ describe('check-conflict-markers (GST-1190: #3927 kept markers inside a template expect(bare.status).toBe(1); expect(bare.stdout).toContain(`CONFLICT_MARKER: docs/merging.md:3 ${OPEN}`); expect(bare.stdout).toContain(`CONFLICT_MARKER: docs/merging.md:5 ${MID}`); + r.change({ 'skill/sections/x.md': `# X\n\n\`\`\`bash\n${OPEN} HEAD\necho a\n${MID}\necho b\n${CLOSE} feat\n\`\`\`\n` }); + expect(r.run().stdout).toContain(`CONFLICT_MARKER: skill/sections/x.md:4 ${OPEN}`); }); - test('only changed lines count, and a fixture that declares its markers is skipped', () => { + test('only changed lines count; a fixture declares itself on a line of its own, a mention elsewhere does not exempt a file', () => { const r = repo({ 'old.txt': `${OPEN} kept on main\n` }); - r.change({ 'old.txt': `${OPEN} kept on main\nnew line\n`, 'test/fixtures/merge.txt': `gstack-conflict-markers: fixture\n${OPEN} HEAD\n${CLOSE} main\n` }); + r.change({ 'old.txt': `${OPEN} kept on main\nnew line\n`, 'test/fixtures/merge.txt': `# gstack-conflict-markers: fixture\n${OPEN} HEAD\n${CLOSE} main\n` }); expect(r.run()).toMatchObject({ status: 0 }); - r.write({ 'src/wip.js': `${CLOSE} uncommitted\n` }); - const dirty = r.run(); - expect([dirty.status, dirty.stdout]).toEqual([1, expect.stringContaining(`CONFLICT_MARKER: src/wip.js:1 ${CLOSE}`)]); + r.change({ 'cfg.json': `{"fix": "declare gstack-conflict-markers: fixture"}\n${OPEN} HEAD\n` }); + expect(r.run().stdout).toContain(`CONFLICT_MARKER: cfg.json:2 ${OPEN}`); + }); + + test('the pushed commit is read, not only the working tree; untracked files are not; git display settings change nothing', () => { + const r = repo(); + r.change({ 'src/a b/ünï.ts': `const s = \`\n${CLOSE} theirs\n\`;\n` }); + for (const env of [{}, { GIT_CONFIG_COUNT: '1', GIT_CONFIG_KEY_0: 'diff.noprefix', GIT_CONFIG_VALUE_0: 'true' }, { GIT_CONFIG_COUNT: '1', GIT_CONFIG_KEY_0: 'diff.mnemonicPrefix', GIT_CONFIG_VALUE_0: 'true' }]) { + expect(r.run(env).stdout, JSON.stringify(env)).toContain(`CONFLICT_MARKER: src/a b/ünï.ts:2 ${CLOSE}`); + } + r.write({ 'src/a b/ünï.ts': 'const s = 1;\n', 'merge.ts.orig': `${OPEN} HEAD\n` }); + const repaired = r.run(); + expect([repaired.status, repaired.stdout]).toEqual([1, expect.stringContaining(`CONFLICT_MARKER: src/a b/ünï.ts:2 ${CLOSE}`)]); + expect(repaired.stdout).not.toContain('merge.ts.orig'); + r.write({ 'README.md': `base\n${MID}\n` }); + r.git('add', '-A'); r.git('commit', '-q', '-m', 'repair'); + r.write({ 'lib/wip.ts': `${CLOSE} uncommitted\n` }); r.git('add', 'lib/wip.ts'); r.git('commit', '-q', '-m', 'wip'); r.write({ 'lib/wip.ts': `ok\n${OPEN} x\n` }); + expect(r.run().stdout).toContain(`CONFLICT_MARKER: lib/wip.ts:2 ${OPEN} (working tree)`); }); });