diff --git a/bin/gh-prs-merge.ts b/bin/gh-prs-merge.ts index eef33e9..19e793f 100755 --- a/bin/gh-prs-merge.ts +++ b/bin/gh-prs-merge.ts @@ -15,6 +15,7 @@ import { csv, integer, parseArgs, UsageError } from '../src/args.ts'; import { isMain } from '../src/is-main.ts'; import { Gh } from '../src/gh.ts'; +import { resolveLockfileConflicts } from '../src/lockfile-conflicts.ts'; import { defaults, render, renderSummary, sweep, type MergeOptions } from '../src/prs-merge.ts'; const USAGE = `Usage: @@ -74,11 +75,19 @@ Fixing (--fix): parked PRs are polled together and they are all running their suites at the same time anyway. + lockfile-only conflict mergeStateStatus=DIRTY where every conflicting + file is bun.lock(b), pnpm-lock.yaml or + package-lock.json: merges the base in a blobless + clone under ~/.cache, takes the base's lockfile, + rebuilds it (lockfile-only, no scripts), pushes a + merge commit (never a force push) and parks it. + Same-repo PRs only; forks stay FIXME + Never repaired: - mergeStateStatus=DIRTY is GitHub's settled verdict that the branch - conflicts with its base. update-branch cannot merge over a conflict, so it - is not attempted; the PR is reported once as FIXME and left alone. - Resolving a conflict means choosing between two authors' intent. A check + Any other DIRTY PR. update-branch cannot merge over a conflict, so it is + not attempted; the PR is reported once as FIXME, naming the conflicting + files, and left alone. Resolving a conflict in code means choosing + between two authors' intent. A check that ran and failed is a result, not an obstacle. `; @@ -104,6 +113,7 @@ async function main(argv: string[]): Promise { fixWaitMs: integer(parsed.values, '--fix-wait', defaults.fixWaitMs / 1000, { max: 86_400 }) * 1000, pollMs: defaults.pollMs, + resolveLockfiles: (pr) => resolveLockfileConflicts(pr), }; if (options.orgs.length === 0 && options.users.length === 0) { diff --git a/package.json b/package.json index fd5f855..0e5ed66 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@profullstack/cli-tools", - "version": "0.68.4", + "version": "0.69.0", "private": true, "description": "Local command-line tools, in TypeScript, exposed on PATH.", "type": "module", diff --git a/src/lockfile-conflicts.ts b/src/lockfile-conflicts.ts new file mode 100644 index 0000000..eff8e53 --- /dev/null +++ b/src/lockfile-conflicts.ts @@ -0,0 +1,141 @@ +import { mkdir, mkdtemp, rm } from 'node:fs/promises'; +import { homedir } from 'node:os'; +import { basename, dirname, join } from 'node:path'; +import { run, type RunResult } from './exec.ts'; +import { parsePullRequestUrl, type PullRequest } from './gh.ts'; + +/** + * Lockfiles a package manager can rebuild from the manifests beside it. + * + * A conflict in one of these is not two authors disagreeing: each side bumped + * a dependency, and the merged package.json already says what both wanted. Take + * the base's lockfile and let the tool that owns it re-resolve. Lockfile-only + * and no scripts, so nothing from the branch is executed. + * + * yarn.lock is absent on purpose: classic yarn has no lockfile-only mode, and + * guessing which yarn a repo uses is worse than a FIXME. + */ +export const LOCKFILES: Readonly> = { + 'bun.lock': ['bun', ['install', '--lockfile-only', '--ignore-scripts']], + 'bun.lockb': ['bun', ['install', '--lockfile-only', '--ignore-scripts']], + 'pnpm-lock.yaml': ['pnpm', ['install', '--lockfile-only', '--ignore-scripts']], + 'package-lock.json': [ + 'npm', + ['install', '--package-lock-only', '--ignore-scripts', '--no-audit', '--no-fund'], + ], +}; + +export const isLockfile = (path: string): boolean => basename(path) in LOCKFILES; + +export type LockfileOutcome = + | { kind: 'resolved'; files: string[] } + | { kind: 'manual'; reason: string }; + +/** Resolves a DIRTY PR when every conflicting file is a lockfile. */ +export type LockfileResolver = (pr: PullRequest) => Promise; + +const tail = (result: RunResult): string => + (result.stderr.trim() || result.stdout.trim()).replace(/\s+/g, ' ').slice(-300); + +/** + * Merge the base into the head in a throwaway blobless clone, rebuild the + * conflicting lockfiles, and push the merge commit. Never a force push: if the + * branch moved underneath us the push is refused and the PR stays a FIXME. + * + * The clone lives under ~/.cache, not os.tmpdir(): on these boxes /tmp is a + * quota'd tmpfs and a repo checkout is exactly what fills it. + */ +export async function resolveLockfileConflicts( + pr: PullRequest, + exec: typeof run = run, +): Promise { + const parsed = parsePullRequestUrl(pr.url); + if (!parsed) return { kind: 'manual', reason: `cannot parse ${pr.url}` }; + + const env = { ...process.env, GH_PAGER: 'cat', GIT_TERMINAL_PROMPT: '0' }; + const view = await exec( + 'gh', + ['pr', 'view', pr.url, '--json', 'headRefName,baseRefName,isCrossRepository'], + { env }, + ); + if (view.code !== 0) return { kind: 'manual', reason: `gh pr view failed: ${tail(view)}` }; + + const refs = JSON.parse(view.stdout) as { + headRefName: string; + baseRefName: string; + isCrossRepository: boolean; + }; + // A fork's branch is not ours to push to, and its manifests are not ours to trust. + if (refs.isCrossRepository) return { kind: 'manual', reason: 'PR is from a fork' }; + + const root = join(process.env.XDG_CACHE_HOME || join(homedir(), '.cache'), 'gh-prs-merge'); + await mkdir(root, { recursive: true }); + const dir = await mkdtemp(join(root, `${parsed.slug.replace('/', '-')}-${parsed.number}-`)); + + const git = (args: string[], cwd = dir) => + exec('git', ['-c', 'advice.mergeConflict=false', ...args], { env, cwd, timeoutMs: 600_000 }); + + try { + const clone = await exec( + 'gh', + ['repo', 'clone', parsed.slug, dir, '--', '--filter=blob:none', '--branch', refs.headRefName], + { env, timeoutMs: 600_000 }, + ); + if (clone.code !== 0) return { kind: 'manual', reason: `clone failed: ${tail(clone)}` }; + + const head = await git(['rev-parse', 'HEAD']); + if (head.stdout.trim() !== pr.headRefOid) { + return { kind: 'manual', reason: 'head moved since it was judged' }; + } + + const fetch = await git(['fetch', '--quiet', 'origin', refs.baseRefName]); + if (fetch.code !== 0) return { kind: 'manual', reason: `fetch failed: ${tail(fetch)}` }; + + const message = `Merge ${refs.baseRefName} into ${refs.headRefName}`; + const merge = await git(['merge', '--no-ff', '--no-commit', '-m', message, 'FETCH_HEAD']); + + const unmerged = await git(['diff', '--name-only', '--diff-filter=U']); + const conflicted = unmerged.stdout.split('\n').filter(Boolean); + + if (merge.code !== 0 && conflicted.length === 0) { + return { kind: 'manual', reason: `git merge failed: ${tail(merge)}` }; + } + + const other = conflicted.filter((f) => !isLockfile(f)); + if (other.length > 0) { + return { kind: 'manual', reason: `conflicts outside lockfiles: ${other.join(', ')}` }; + } + + for (const file of conflicted) { + // Merging base into head, "theirs" is the base: the lockfile main already + // resolved, which the branch's own changes are then applied on top of. + const take = await git(['checkout', '--theirs', '--', file]); + if (take.code !== 0) return { kind: 'manual', reason: `checkout ${file}: ${tail(take)}` }; + + const [bin, args] = LOCKFILES[basename(file)]!; + const regen = await exec(bin, [...args], { + env, + cwd: join(dir, dirname(file)), + timeoutMs: 600_000, + }); + if (regen.code !== 0) { + return { kind: 'manual', reason: `${bin} could not rebuild ${file}: ${tail(regen)}` }; + } + + const add = await git(['add', '--', file]); + if (add.code !== 0) return { kind: 'manual', reason: `git add ${file}: ${tail(add)}` }; + } + + const body = + conflicted.length > 0 ? `\n\nRegenerated ${conflicted.join(', ')} by gh-prs-merge --fix.` : ''; + const commit = await git(['commit', '--no-verify', '-m', `${message}${body}`]); + if (commit.code !== 0) return { kind: 'manual', reason: `commit failed: ${tail(commit)}` }; + + const push = await git(['push', '--quiet', 'origin', `HEAD:refs/heads/${refs.headRefName}`]); + if (push.code !== 0) return { kind: 'manual', reason: `push refused: ${tail(push)}` }; + + return { kind: 'resolved', files: conflicted }; + } finally { + await rm(dir, { recursive: true, force: true }); + } +} diff --git a/src/prs-merge.ts b/src/prs-merge.ts index ef07d74..b5e6716 100644 --- a/src/prs-merge.ts +++ b/src/prs-merge.ts @@ -1,5 +1,6 @@ import { Gh, RateLimitError, type Check, type MergeState, type PullRequest, type WorkflowRun } from './gh.ts'; import { sleep } from './exec.ts'; +import type { LockfileResolver } from './lockfile-conflicts.ts'; export interface MergeOptions { orgs: string[]; @@ -12,6 +13,11 @@ export interface MergeOptions { fixWaitMs: number; /** Poll interval while waiting on running checks. Shortened by tests. */ pollMs: number; + /** + * With --fix, rebuilds a DIRTY PR whose only conflicts are lockfiles. Left + * out (as in tests), every DIRTY PR is a FIXME. + */ + resolveLockfiles?: LockfileResolver; } export interface Summary { @@ -363,6 +369,41 @@ export async function sweep( continue; } + if (pr.mergeStateStatus === 'DIRTY' && options.resolveLockfiles) { + // Both sides bumped dependencies and only the lockfile collided. That + // is not a disagreement: the merged manifests say what both wanted. + emit({ kind: 'fixing', url, text: `${reason}; trying a lockfile-only conflict resolution` }); + let outcome: Awaited>; + try { + outcome = await options.resolveLockfiles(pr); + } catch (error) { + outcome = { + kind: 'manual', + reason: error instanceof Error ? error.message : String(error), + }; + } + + if (outcome.kind === 'resolved') { + emit({ + kind: 'fixing', + url, + text: `merged the base in and regenerated ${outcome.files.join(', ') || 'nothing'}; parked for the second pass`, + }); + summary.fixed += 1; + parked.push({ url, title: pr.title, repaired: true, awaitingChecks: true }); + continue; + } + + emit({ + kind: 'fixme', + url, + text: `${reason}; the branch conflicts with its base and needs resolving by hand (${outcome.reason})`, + title: pr.title, + }); + summary.skipped += 1; + continue; + } + if (pr.mergeStateStatus === 'DIRTY') { // A real conflict. Say so once — as FIXME, not as a second SKIP line // repeating it — and leave it alone: choosing between two authors' diff --git a/test/lockfile-conflicts.test.ts b/test/lockfile-conflicts.test.ts new file mode 100644 index 0000000..ac1789f --- /dev/null +++ b/test/lockfile-conflicts.test.ts @@ -0,0 +1,105 @@ +import { describe, expect, it } from 'vitest'; +import type { RunResult } from '../src/exec.ts'; +import type { PullRequest } from '../src/gh.ts'; +import { isLockfile, resolveLockfileConflicts } from '../src/lockfile-conflicts.ts'; + +const pr: PullRequest = { + url: 'https://github.com/acme/repo/pull/7', + title: 't', + state: 'OPEN', + isDraft: false, + mergeable: 'CONFLICTING', + mergeStateStatus: 'DIRTY', + headRefOid: 'deadbeef', +}; + +/** A scripted gh/git/bun; `conflicted` is what `git diff --diff-filter=U` reports. */ +function stubExec(options: { + conflicted: string[]; + fork?: boolean; + head?: string; + pushFails?: boolean; +}) { + const calls: string[] = []; + const ok = (stdout = ''): RunResult => ({ code: 0, stdout, stderr: '' }); + const exec = async (file: string, args: readonly string[]): Promise => { + const argv = args.filter((a, i) => !(a === '-c' || args[i - 1] === '-c')); + calls.push(`${file} ${argv.join(' ')}`); + if (file === 'gh' && argv[0] === 'pr') { + return ok( + JSON.stringify({ + headRefName: 'feat', + baseRefName: 'main', + isCrossRepository: !!options.fork, + }), + ); + } + if (file === 'git' && argv[0] === 'rev-parse') return ok(`${options.head ?? 'deadbeef'}\n`); + if (file === 'git' && argv[0] === 'merge') { + return options.conflicted.length ? { code: 1, stdout: 'CONFLICT', stderr: '' } : ok(); + } + if (file === 'git' && argv[0] === 'diff') return ok(options.conflicted.join('\n')); + if (file === 'git' && argv[0] === 'push' && options.pushFails) { + return { code: 1, stdout: '', stderr: '! [rejected] (fetch first)' }; + } + return ok(); + }; + return { exec, calls }; +} + +describe('lockfile conflict resolution', () => { + it('recognises lockfiles anywhere in the tree, and nothing else', () => { + expect(isLockfile('bun.lock')).toBe(true); + expect(isLockfile('apps/web/pnpm-lock.yaml')).toBe(true); + expect(isLockfile('package-lock.json')).toBe(true); + expect(isLockfile('yarn.lock')).toBe(false); + expect(isLockfile('package.json')).toBe(false); + }); + + it('takes the base lockfile, rebuilds it lockfile-only, and pushes without force', async () => { + const { exec, calls } = stubExec({ conflicted: ['bun.lock'] }); + const outcome = await resolveLockfileConflicts(pr, exec); + + expect(outcome).toEqual({ kind: 'resolved', files: ['bun.lock'] }); + expect(calls).toContain('git checkout --theirs -- bun.lock'); + expect(calls).toContain('bun install --lockfile-only --ignore-scripts'); + const push = calls.find((c) => c.startsWith('git push'))!; + expect(push).toContain('HEAD:refs/heads/feat'); + expect(push).not.toMatch(/--force|\+HEAD/); + }); + + it('declines when a code file conflicts too, before rebuilding anything', async () => { + const { exec, calls } = stubExec({ conflicted: ['bun.lock', 'src/index.ts'] }); + const outcome = await resolveLockfileConflicts(pr, exec); + + expect(outcome).toEqual({ + kind: 'manual', + reason: 'conflicts outside lockfiles: src/index.ts', + }); + expect(calls.some((c) => c.startsWith('bun '))).toBe(false); + expect(calls.some((c) => c.startsWith('git push'))).toBe(false); + }); + + it('never touches a fork', async () => { + const { exec, calls } = stubExec({ conflicted: ['bun.lock'], fork: true }); + expect(await resolveLockfileConflicts(pr, exec)).toEqual({ + kind: 'manual', + reason: 'PR is from a fork', + }); + expect(calls.some((c) => c.includes('clone'))).toBe(false); + }); + + it('declines when the head moved after the PR was judged', async () => { + const { exec, calls } = stubExec({ conflicted: ['bun.lock'], head: 'cafe' }); + const outcome = await resolveLockfileConflicts(pr, exec); + expect(outcome.kind).toBe('manual'); + expect(calls.some((c) => c.startsWith('git push'))).toBe(false); + }); + + it('reports a refused push instead of claiming the repair', async () => { + const { exec } = stubExec({ conflicted: ['bun.lock'], pushFails: true }); + const outcome = await resolveLockfileConflicts(pr, exec); + expect(outcome.kind).toBe('manual'); + expect(outcome.kind === 'manual' && outcome.reason).toContain('push refused'); + }); +}); diff --git a/test/prs-merge.test.ts b/test/prs-merge.test.ts index 50dbe5b..c95e007 100644 --- a/test/prs-merge.test.ts +++ b/test/prs-merge.test.ts @@ -475,6 +475,65 @@ describe('sweep', () => { expect(lines.filter((l) => l.kind === 'skip')).toHaveLength(0); }); + it('resolves a lockfile-only conflict with --fix, then merges once CI is green', async () => { + const { gh } = stubGh({ + steps: [{ mergeable: 'CONFLICTING', mergeStateStatus: 'DIRTY' }, {}], + }); + const seen: string[] = []; + const { summary, text } = await runSweep( + gh, + baseOptions({ + fix: true, + resolveLockfiles: async (pr) => { + seen.push(pr.url); + return { kind: 'resolved', files: ['bun.lock'] }; + }, + }), + ); + + expect(seen).toEqual([PR_URL]); + expect(text).toContain('regenerated bun.lock'); + expect(summary.fixed).toBe(1); + expect(summary.merged).toBe(1); + expect(summary.skipped).toBe(0); + }); + + it('keeps a code conflict a FIXME and names why the resolver declined', async () => { + const { gh, calls } = stubGh({ + steps: [{ mergeable: 'CONFLICTING', mergeStateStatus: 'DIRTY' }], + }); + const { summary, text } = await runSweep( + gh, + baseOptions({ + fix: true, + resolveLockfiles: async () => ({ + kind: 'manual', + reason: 'conflicts outside lockfiles: src/index.ts', + }), + }), + ); + + expect(text).toContain('needs resolving by hand (conflicts outside lockfiles: src/index.ts)'); + expect(summary.fixed).toBe(0); + expect(summary.skipped).toBe(1); + expect(calls.some((c) => c.startsWith('pr merge'))).toBe(false); + }); + + it('never calls the lockfile resolver without --fix', async () => { + const { gh } = stubGh({ steps: [{ mergeable: 'CONFLICTING', mergeStateStatus: 'DIRTY' }] }); + let called = false; + await runSweep( + gh, + baseOptions({ + resolveLockfiles: async () => { + called = true; + return { kind: 'resolved', files: [] }; + }, + }), + ); + expect(called).toBe(false); + }); + it('merges a PR GitHub calls UNSTABLE when every check is green', async () => { // The false skip this sweep kept producing: MERGEABLE, two green checks, // and an aggregate state that only meant a non-required context was not.