diff --git a/CHANGELOG.md b/CHANGELOG.md index dd02870..e35822b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Fixed +- When another push lands on the pull request branch while a run commits its translations, the run now puts its commit on top of the newer one and pushes again, as signed commits already did. This is the common case in a monorepo where each app's job commits to the same branch: before, the job that pushed second failed after three identical retries and its translations were lost. If the newer commits changed one of the same translation files, or the branch was rewritten, the run skips its commit with a notice instead of overwriting anything. +- A failed push only suggests checking `permissions: contents: write` when GitHub actually refused the push for permissions. Other failures, such as a network error, show git's output without that hint. + ## [0.0.79] - 2026-10-08 ### Changed diff --git a/src/commands/translate.ts b/src/commands/translate.ts index 52f857c..8aba32f 100644 --- a/src/commands/translate.ts +++ b/src/commands/translate.ts @@ -22,7 +22,8 @@ import { BatchResult, MissingLocaleEntry } from '../utils/translation-utils.js'; -import { autoCommitChanges, buildMakemessagesCommand, MISSING_BRANCH_ERROR, MISSING_TOKEN_ERROR, type CommitResult } from '../utils/github.js'; +import { autoCommitChanges, buildMakemessagesCommand, MISSING_BRANCH_ERROR, MISSING_TOKEN_ERROR, PushPermissionError, type CommitResult } from '../utils/github.js'; +import { GitHubGraphQLError } from '../utils/github-graphql.js'; import { escapeAnnotationData } from '../utils/ci-context.js'; import { detectTargetChanges, type TargetChangeFile } from '../utils/target-changes.js'; import { resetUnreadableFiles, getUnreadableFiles } from '../utils/unreadable-files.js'; @@ -160,9 +161,15 @@ const COMMIT_FAILURE_FIXES: Record = { const NOT_A_PULL_REQUEST_WARNING = '::warning::Translations were not committed: this is not a pull request run. Only pull_request runs commit translations.'; const PUSH_PERMISSION_FIX = 'Check that the workflow grants `permissions: contents: write`. On a pull request from a fork, GITHUB_TOKEN is read-only.'; -function commitFailedAnnotation(reason: string): string { - const fix = COMMIT_FAILURE_FIXES[reason] ?? PUSH_PERMISSION_FIX; - return `::error::${escapeAnnotationData(`Translations were not committed: ${reason}. ${fix}`)}`; +function isPermissionFailure(error: Error): boolean { + if (error instanceof PushPermissionError) return true; + return error instanceof GitHubGraphQLError && (error.type === 'FORBIDDEN' || /\b403\b/.test(error.message)); +} + +function commitFailedAnnotation(error: Error): string { + const fix = isPermissionFailure(error) ? PUSH_PERMISSION_FIX : COMMIT_FAILURE_FIXES[error.message]; + const text = [`Translations were not committed: ${error.message}.`, fix].filter(Boolean).join(' '); + return `::error::${escapeAnnotationData(text)}`; } /** @@ -456,7 +463,7 @@ export async function translate(options: TranslationOptions = {}, deps: Translat } else if (err.message === MISSING_BRANCH_ERROR) { console.log(NOT_A_PULL_REQUEST_WARNING); } else { - console.log(commitFailedAnnotation(err.message)); + console.log(commitFailedAnnotation(err)); process.exitCode = 1; } } diff --git a/src/utils/github.ts b/src/utils/github.ts index dbbb3d0..b9432ab 100644 --- a/src/utils/github.ts +++ b/src/utils/github.ts @@ -34,6 +34,15 @@ const SYNC_SKIP_NOTICES: Record = { // A tip that keeps moving is a busy branch; stop chasing it and let a later run commit. const MAX_COMMITS_ON_NEWER_TIP = 2; +// A failed push is classified by what git printed, not by the branch's shape: +// a read-only token can still fetch a branch that moved. +const PERMISSION_DENIED_OUTPUT = /Permission to \S+ denied|returned error: 403|Authentication failed|could not read Username/; +const BRANCH_MOVED_OUTPUT = /\((fetch first|non-fast-forward|stale info)\)|cannot lock ref/; +const PUSH_ATTEMPTS = 3; + +export class PushPermissionError extends Error {} +class BranchMovedError extends Error {} + /** * Dependencies for the GitHub service */ @@ -367,26 +376,82 @@ ${buildExtractStep(options)} - name: Translate return new Promise(resolve => setTimeout(resolve, ms)); }, + /** + * A moved branch is replayed onto (never for an amend, which would carry the + * trigger commit along) or skipped; a permission failure fails at once; any + * other failure, such as the network, is retried. + */ async pushWithRetry( branchName: string, token: string, - forceWithLease: boolean = false, - maxRetries: number = 3 - ): Promise { + forceWithLease: boolean = false + ): Promise<'pushed' | SkippedCommit> { const { console: log } = this.deps; + let failedAttempts = 0; + let replays = 0; - for (let attempt = 1; attempt <= maxRetries; attempt++) { + for (;;) { try { this.pushToGitHub(branchName, token, forceWithLease); - return; + return 'pushed'; } catch (error) { - if (attempt === maxRetries) throw error; - log.log(`Push failed, retrying (${attempt}/${maxRetries})...`); + if (error instanceof PushPermissionError) throw error; + if (error instanceof BranchMovedError) { + if (forceWithLease || replays === MAX_COMMITS_ON_NEWER_TIP) return 'skipped-uncertain'; + replays++; + const outcome = await this.rebaseOntoMovedBranch(branchName); + if (outcome !== 'rebased') return outcome; + continue; + } + failedAttempts++; + if (failedAttempts === PUSH_ATTEMPTS) throw error; + log.log(`Push failed, retrying (${failedAttempts}/${PUSH_ATTEMPTS})...`); await this.sleep(2000); } } }, + /** + * After a push rejected because the branch moved: replay our translation + * commit onto the new tip when that is safe, the plain-git counterpart of + * findTipSafeToCommitOn. Safe means the branch only moved forward from our + * commit's parent (so nothing someone removed comes back) and the newer + * commits left our files alone. Anything else is a skip. + */ + async rebaseOntoMovedBranch(branchName: string): Promise<'rebased' | SkippedCommit> { + const { console: log } = this.deps; + const git = (args: string) => this.deps.exec(`git ${args}`, { stdio: 'pipe' }).toString().trim(); + // --no-renames lists a rename under both paths, so a renamed file of ours counts as touched. + const changedFiles = (from: string, to: string) => + git(`diff --name-only --no-renames ${from} ${to}`).split('\n').filter(Boolean); + + try { + git(`fetch --no-tags origin ${branchName}`); + git('merge-base --is-ancestor HEAD~1 FETCH_HEAD'); + } catch { + log.log('Could not confirm the branch only moved forward since the checkout.'); + return 'skipped-uncertain'; + } + + const theirs = new Set(changedFiles('HEAD~1', 'FETCH_HEAD')); + const overlap = changedFiles('HEAD~1', 'HEAD').find(file => theirs.has(file)); + if (overlap) { + log.log(`Newer commits on the branch changed ${overlap}.`); + return 'skipped-overlap'; + } + + // --autostash: translate leaves localhero.json (lastSyncedAt) modified and unstaged. + try { + git('rebase --autostash --onto FETCH_HEAD HEAD~1'); + } catch { + try { git('rebase --abort'); } catch { /* nothing to abort */ } + log.log('Could not replay the translation commit onto the newer commits.'); + return 'skipped-uncertain'; + } + log.log(`Branch moved during the run; replayed the translation commit onto ${git('rev-parse --short FETCH_HEAD')}.`); + return 'rebased'; + }, + /** * Push changes to GitHub using the provided token * @param branchName Branch to push to @@ -418,9 +483,22 @@ ${buildExtractStep(options)} - name: Translate } const pushCmd = forceWithLease - ? `git push --force-with-lease origin HEAD:${branchName}` - : `git push origin HEAD:${branchName}`; - exec(pushCmd, { stdio: 'inherit' }); + ? `git push --force-with-lease origin HEAD:${branchName} 2>&1` + : `git push origin HEAD:${branchName} 2>&1`; + const mask = (text: string) => text.split(token).join('***TOKEN***').trim(); + + try { + const output = mask(exec(pushCmd, { stdio: 'pipe' }).toString()); + if (output) log.log(output); + } catch (error: unknown) { + const err = error as Error & { stdout?: Buffer | string }; + const output = mask(err.stdout?.toString() ?? ''); + if (output) log.log(output); + const message = [mask(err.message), output].filter(Boolean).join('\n'); + if (PERMISSION_DENIED_OUTPUT.test(output)) throw new PushPermissionError(message); + if (BRANCH_MOVED_OUTPUT.test(output)) throw new BranchMovedError(message); + throw new Error(message); + } }, /** @@ -565,7 +643,11 @@ ${buildExtractStep(options)} - name: Translate this.commit(commitMessage, canAmend); const token = await this.getTokenForPush(); - await this.pushWithRetry(branchName, token, canAmend); + const outcome = await this.pushWithRetry(branchName, token, canAmend); + if (outcome !== 'pushed') { + log.log(`::warning::${SYNC_SKIP_NOTICES[outcome]}`); + return 'skipped'; + } if (canAmend) { log.log('✓ Commit amended and pushed to GitHub\n'); @@ -681,7 +763,11 @@ ${buildExtractStep(options)} - name: Translate this.commit(commitMessage); const token = await this.getTokenForPush(); - await this.pushWithRetry(branchName, token); + const outcome = await this.pushWithRetry(branchName, token); + if (outcome !== 'pushed') { + log.log(`::warning::${TRANSLATE_SKIP_NOTICES[outcome]}`); + return 'skipped'; + } log.log('Changes committed and pushed successfully.'); return 'new'; diff --git a/tests/commands/translate.test.js b/tests/commands/translate.test.js index a9ab88a..6a5b2ec 100644 --- a/tests/commands/translate.test.js +++ b/tests/commands/translate.test.js @@ -1,6 +1,8 @@ import { jest } from '@jest/globals'; import { translate } from '../../src/commands/translate.js'; import { ApiResponseError } from '../../src/types/index.js'; +import { PushPermissionError } from '../../src/utils/github.js'; +import { GitHubGraphQLError } from '../../src/utils/github-graphql.js'; describe('translate command', () => { let mockConsole; @@ -1440,8 +1442,8 @@ describe('translate command', () => { expect(process.exitCode).not.toBe(1); }); - it('fails the run and points at the push permission when the push is rejected', async () => { - gitUtils.autoCommitChanges.mockRejectedValue(new Error('Command failed: git push origin HEAD:feature\nremote: Permission denied')); + it('fails the run and points at the push permission when the push is denied', async () => { + gitUtils.autoCommitChanges.mockRejectedValue(new PushPermissionError('Command failed: git push origin HEAD:feature\nremote: Permission denied')); await translate({}, createTranslateDeps()); @@ -1452,6 +1454,28 @@ describe('translate command', () => { expect(process.exitCode).toBe(1); }); + it('points at the push permission when a signed commit is forbidden', async () => { + gitUtils.autoCommitChanges.mockRejectedValue(new GitHubGraphQLError('Resource not accessible by integration', 'FORBIDDEN')); + + await translate({}, createTranslateDeps()); + + expect(errorAnnotations()).toEqual([ + '::error::Translations were not committed: Resource not accessible by integration. ' + + 'Check that the workflow grants `permissions: contents: write`. On a pull request from a fork, GITHUB_TOKEN is read-only.' + ]); + }); + + it('fails the run without blaming permissions when the push failed for another reason', async () => { + gitUtils.autoCommitChanges.mockRejectedValue(new Error("Command failed: git push origin HEAD:feature\nfatal: unable to access: Could not resolve host: github.com")); + + await translate({}, createTranslateDeps()); + + expect(errorAnnotations()).toEqual([ + '::error::Translations were not committed: Command failed: git push origin HEAD:feature%0Afatal: unable to access: Could not resolve host: github.com.' + ]); + expect(process.exitCode).toBe(1); + }); + it.each([['skipped'], ['no-changes']])('passes when the commit result is %s', async (commitResult) => { gitUtils.autoCommitChanges.mockResolvedValue(commitResult); diff --git a/tests/utils/git-push-branch-moved.test.ts b/tests/utils/git-push-branch-moved.test.ts new file mode 100644 index 0000000..34237f8 --- /dev/null +++ b/tests/utils/git-push-branch-moved.test.ts @@ -0,0 +1,141 @@ +import { describe, it, expect, jest, beforeEach, afterEach } from '@jest/globals'; +import { execSync } from 'child_process'; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'fs'; +import os from 'os'; +import path from 'path'; +import { githubService } from '../../src/utils/github.js'; + +const BRANCH = 'feature'; +const GIT = '-c user.email=t@t -c user.name=t -c commit.gpgsign=false'; + +function git(cwd: string, args: string): string { + return execSync(`git ${GIT} ${args}`, { cwd, stdio: 'pipe' }).toString().trim(); +} + +function commitFile(cwd: string, file: string, content: string, message: string): void { + mkdirSync(path.dirname(path.join(cwd, file)), { recursive: true }); + writeFileSync(path.join(cwd, file), content); + git(cwd, `add ${file}`); + git(cwd, `commit -qm "${message}"`); +} + +function remoteLog(remote: string): string[] { + return git(remote, `log --format=%s ${BRANCH}`).split('\n'); +} + +describe('rebaseOntoMovedBranch against a real remote', () => { + let root: string; + let remote: string; + let sibling: string; + let ours: string; + let originalCwd: string; + + beforeEach(() => { + originalCwd = process.cwd(); + root = mkdtempSync(path.join(os.tmpdir(), 'push-moved-')); + remote = path.join(root, 'remote.git'); + sibling = path.join(root, 'sibling'); + ours = path.join(root, 'ours'); + + execSync(`git init -q --bare -b ${BRANCH} ${remote}`); + execSync(`git clone -q ${remote} ${sibling}`, { stdio: 'pipe' }); + commitFile(sibling, 'apps/editor/public/locale/sv.json', '{}\n', 'base'); + git(sibling, `push -q origin HEAD:${BRANCH}`); + execSync(`git clone -q ${remote} ${ours}`, { stdio: 'pipe' }); + + // In CI configureGitUser sets the identity before committing; here the rebase needs one too. + const gitEnv = { + ...process.env, + GIT_COMMITTER_NAME: 't', GIT_COMMITTER_EMAIL: 't@t', + GIT_CONFIG_COUNT: '1', GIT_CONFIG_KEY_0: 'commit.gpgsign', GIT_CONFIG_VALUE_0: 'false' + }; + githubService.setDependencies({ + exec: (cmd, options) => execSync(cmd, { ...options, env: gitEnv }), + env: {}, + console: { log: jest.fn(), warn: jest.fn(), error: jest.fn() } + }); + process.chdir(ours); + }); + + afterEach(() => { + process.chdir(originalCwd); + rmSync(root, { recursive: true, force: true }); + }); + + it('replays our commit onto a tip that moved forward without touching our files', async () => { + commitFile(sibling, 'apps/editor/public/locale/de.json', '{}\n', 'editor translations'); + git(sibling, `push -q origin HEAD:${BRANCH}`); + commitFile(ours, 'apps/portal/public/locales/sv.json', '{}\n', 'portal translations'); + + const outcome = await githubService.rebaseOntoMovedBranch(BRANCH); + git(ours, `push -q origin HEAD:${BRANCH}`); + + expect(outcome).toBe('rebased'); + expect(remoteLog(remote)).toEqual(['portal translations', 'editor translations', 'base']); + }); + + it('replays with the unstaged localhero.json change translate leaves behind, and keeps it', async () => { + commitFile(sibling, 'localhero.json', '{"lastSyncedAt": "old"}\n', 'config'); + git(sibling, `push -q origin HEAD:${BRANCH}`); + git(ours, `pull -q origin ${BRANCH}`); + commitFile(sibling, 'apps/editor/public/locale/de.json', '{}\n', 'editor translations'); + git(sibling, `push -q origin HEAD:${BRANCH}`); + commitFile(ours, 'apps/portal/public/locales/sv.json', '{}\n', 'portal translations'); + writeFileSync(path.join(ours, 'localhero.json'), '{"lastSyncedAt": "new"}\n'); + + const outcome = await githubService.rebaseOntoMovedBranch(BRANCH); + git(ours, `push -q origin HEAD:${BRANCH}`); + + expect(outcome).toBe('rebased'); + expect(remoteLog(remote)).toEqual(['portal translations', 'editor translations', 'config', 'base']); + expect(git(ours, 'status --porcelain')).toBe('M localhero.json'); + }); + + it('skips when the newer commits touched one of our files', async () => { + commitFile(sibling, 'apps/editor/public/locale/sv.json', '{"a": 1}\n', 'someone else edits sv'); + git(sibling, `push -q origin HEAD:${BRANCH}`); + commitFile(ours, 'apps/editor/public/locale/sv.json', '{"a": 2}\n', 'our sv'); + + const outcome = await githubService.rebaseOntoMovedBranch(BRANCH); + + expect(outcome).toBe('skipped-overlap'); + expect(git(ours, 'log -1 --format=%s')).toBe('our sv'); + expect(git(ours, 'status --porcelain')).toBe(''); + }); + + it('treats a renamed file as touched, so our edits are not carried to the new name', async () => { + git(sibling, 'mv apps/editor/public/locale/sv.json apps/editor/public/locale/sv-SE.json'); + git(sibling, 'commit -qm "rename sv"'); + git(sibling, `push -q origin HEAD:${BRANCH}`); + commitFile(ours, 'apps/editor/public/locale/sv.json', '{"a": 2}\n', 'our sv'); + + const outcome = await githubService.rebaseOntoMovedBranch(BRANCH); + + expect(outcome).toBe('skipped-overlap'); + }); + + it('skips rather than replay a commit someone removed with a force-push', async () => { + commitFile(sibling, 'app/code.js', 'removed later\n', 'code change'); + git(sibling, `push -q origin HEAD:${BRANCH}`); + git(ours, `pull -q origin ${BRANCH}`); + commitFile(ours, 'apps/portal/public/locales/sv.json', '{}\n', 'portal translations'); + git(sibling, 'reset -q --hard HEAD~1'); + commitFile(sibling, 'README.md', 'docs\n', 'replacement'); + git(sibling, `push -q --force origin HEAD:${BRANCH}`); + + const outcome = await githubService.rebaseOntoMovedBranch(BRANCH); + + expect(outcome).toBe('skipped-uncertain'); + expect(remoteLog(remote)).toEqual(['replacement', 'base']); + }); + + it('skips when the branch cannot be fetched', async () => { + commitFile(ours, 'apps/portal/public/locales/sv.json', '{}\n', 'portal translations'); + git(ours, 'remote set-url origin /nonexistent/remote.git'); + + const outcome = await githubService.rebaseOntoMovedBranch(BRANCH); + + expect(outcome).toBe('skipped-uncertain'); + expect(git(ours, 'log -1 --format=%s')).toBe('portal translations'); + }); +}); diff --git a/tests/utils/github.test.ts b/tests/utils/github.test.ts index 1b91c9c..c704aae 100644 --- a/tests/utils/github.test.ts +++ b/tests/utils/github.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect, jest, beforeEach, afterEach } from '@jest/globals'; import { parse } from 'yaml'; -import { githubService, createGitHubActionFile, autoCommitChanges, workflowExists, fetchActionToken } from '../../src/utils/github.js'; +import { githubService, createGitHubActionFile, autoCommitChanges, workflowExists, fetchActionToken, PushPermissionError } from '../../src/utils/github.js'; import { GitHubGraphQLError, StaleHeadError } from '../../src/utils/github-graphql.js'; describe('githubService', () => { @@ -411,7 +411,7 @@ describe('githubService', () => { expect(mockExec).toHaveBeenCalledWith('git add locales/**/*.json', { stdio: 'inherit' }); expect(mockExec).toHaveBeenCalledWith('git status --porcelain'); expect(mockExec).toHaveBeenCalledWith("git commit -m 'Update translations'", { stdio: 'inherit' }); - expect(mockExec).toHaveBeenCalledWith('git push origin HEAD:feature-branch', { stdio: 'inherit' }); + expect(mockExec).toHaveBeenCalledWith('git push origin HEAD:feature-branch 2>&1', { stdio: 'pipe' }); }); it('commits with enhanced message when translation summary provided', async () => { @@ -705,99 +705,161 @@ describe('githubService', () => { '⚠️ Warning: The Localhero GitHub App is not installed for this project. Using GITHUB_TOKEN instead, so checks on this commit will not run. Install the app to enable them.' ); }); + }); - it('retries push on failure and succeeds on second attempt', async () => { - mockEnv.GITHUB_ACTIONS = 'true'; - mockEnv.GITHUB_HEAD_REF = 'feature-branch'; - mockEnv.GITHUB_TOKEN = 'fake-token'; - mockEnv.GITHUB_REPOSITORY = 'owner/repo'; + describe('a failed plain git push', () => { + const PUSH = 'git push origin HEAD:feature-branch 2>&1'; + const AMEND_PUSH = 'git push --force-with-lease origin HEAD:feature-branch 2>&1'; + const FORBIDDEN_GIT_COMMAND = /^git (push --force |push -f|pull|reset|merge)/; + const OUTPUT = { + fetchFirst: ' ! [rejected] HEAD -> feature-branch (fetch first)\nhint: Updates were rejected because the remote contains work that you do not', + lockRef: " ! [remote rejected] HEAD -> feature-branch (cannot lock ref 'refs/heads/feature-branch': is at 7064da9 but expected a2da9bb)", + nonFastForward: ' ! [rejected] HEAD -> feature-branch (non-fast-forward)', + staleInfo: ' ! [rejected] HEAD -> feature-branch (stale info)', + forbidden: 'remote: Permission to owner/repo.git denied to github-actions[bot].\nfatal: unable to access \'https://github.com/owner/repo.git/\': The requested URL returned error: 403', + noCredentials: "fatal: could not read Username for 'https://github.com': No such device or address", + network: "fatal: unable to access 'https://github.com/owner/repo.git/': Could not resolve host: github.com", + signatureRule: 'remote: error: GH013: Repository rule violations found for refs/heads/feature-branch.\nremote: - Commits must have verified signatures.' + }; + + function rejectPush(output: string): never { + throw Object.assign(new Error(`Command failed: ${PUSH}`), { stdout: Buffer.from(output) }); + } - let pushAttempts = 0; + function stubGit(pushOutcomes: Array, extra: Record = {}) { mockExec.mockImplementation((cmd: string) => { if (cmd === 'git status --porcelain') return Buffer.from('M locales/en.json'); - if (cmd === 'git push origin HEAD:feature-branch') { - pushAttempts++; - if (pushAttempts === 1) throw new Error('Repository not found'); - return Buffer.from(''); + if (cmd in extra) return Buffer.from(extra[cmd]); + if (cmd === PUSH || cmd === AMEND_PUSH) { + const outcome = pushOutcomes.shift(); + if (outcome) rejectPush(outcome); + return Buffer.from('To https://github.com/owner/repo.git\n a2da9bb..b77e503 HEAD -> feature-branch'); } return Buffer.from(''); }); + } + + const commands = () => mockExec.mock.calls.map(([cmd]) => cmd as string); + const pushes = () => commands().filter(cmd => cmd.startsWith('git push')); + let rebase: jest.SpiedFunction; + beforeEach(() => { + mockEnv.GITHUB_ACTIONS = 'true'; + mockEnv.GITHUB_HEAD_REF = 'feature-branch'; + mockEnv.GITHUB_TOKEN = 'fake-token'; + mockEnv.GITHUB_REPOSITORY = 'owner/repo'; githubService.sleep = jest.fn().mockResolvedValue(undefined) as any; + rebase = jest.spyOn(githubService, 'rebaseOntoMovedBranch').mockResolvedValue('rebased'); + }); + + afterEach(() => { + rebase.mockRestore(); + }); + + it('prints the push output', async () => { + stubGit([null]); await githubService.autoCommitChanges('locales/**/*.json'); - expect(pushAttempts).toBe(2); - expect(mockConsole.log).toHaveBeenCalledWith('Push failed, retrying (1/3)...'); - expect(mockConsole.log).toHaveBeenCalledWith('Changes committed and pushed successfully.'); + expect(mockConsole.log).toHaveBeenCalledWith(expect.stringContaining('a2da9bb..b77e503 HEAD -> feature-branch')); }); - it('throws after all retry attempts exhausted', async () => { - mockEnv.GITHUB_ACTIONS = 'true'; - mockEnv.GITHUB_HEAD_REF = 'feature-branch'; - mockEnv.GITHUB_TOKEN = 'fake-token'; - mockEnv.GITHUB_REPOSITORY = 'owner/repo'; + it.each(['fetchFirst', 'lockRef', 'nonFastForward'] as const)( + 'replays the commit onto the moved branch and pushes again (%s)', + async (kind) => { + stubGit([OUTPUT[kind], null]); - mockExec.mockImplementation((cmd: string) => { - if (cmd === 'git status --porcelain') return Buffer.from('M locales/en.json'); - if (cmd === 'git push origin HEAD:feature-branch') throw new Error('Repository not found'); - return Buffer.from(''); - }); + const result = await githubService.autoCommitChanges('locales/**/*.json'); - githubService.sleep = jest.fn().mockResolvedValue(undefined) as any; + expect(result).toBe('new'); + expect(rebase).toHaveBeenCalledWith('feature-branch'); + expect(pushes()).toEqual([PUSH, PUSH]); + expect(commands().filter(cmd => FORBIDDEN_GIT_COMMAND.test(cmd))).toEqual([]); + } + ); + + it('skips with the branch-changed notice when the newer commits touched our files', async () => { + stubGit([OUTPUT.fetchFirst]); + rebase.mockResolvedValue('skipped-overlap'); - await expect(githubService.autoCommitChanges('locales/**/*.json')).rejects.toThrow('Repository not found'); - expect(mockConsole.log).toHaveBeenCalledWith('Push failed, retrying (1/3)...'); - expect(mockConsole.log).toHaveBeenCalledWith('Push failed, retrying (2/3)...'); + const result = await githubService.autoCommitChanges('locales/**/*.json'); + + expect(result).toBe('skipped'); + expect(mockConsole.log).toHaveBeenCalledWith( + '::warning::Branch changed during the run and touched these translation files; skipping commit. The new push triggers a fresh run.' + ); + expect(pushes()).toEqual([PUSH]); }); - }); - describe('plain git push when the branch moved during the run', () => { - const rejectNonFastForward = () => { - throw new Error('! [rejected] HEAD -> feature-branch (non-fast-forward)'); - }; - const REWRITING_GIT_COMMAND = /^git (fetch|pull|rebase|reset|merge)/; + it('stops chasing a branch that keeps moving', async () => { + stubGit([OUTPUT.fetchFirst, OUTPUT.fetchFirst, OUTPUT.fetchFirst]); - beforeEach(() => { - mockEnv.GITHUB_ACTIONS = 'true'; - mockEnv.GITHUB_HEAD_REF = 'feature-branch'; - mockEnv.GITHUB_TOKEN = 'fake-token'; - mockEnv.GITHUB_REPOSITORY = 'owner/repo'; - githubService.sleep = jest.fn().mockResolvedValue(undefined) as any; + const result = await githubService.autoCommitChanges('locales/**/*.json'); + + expect(result).toBe('skipped'); + expect(rebase).toHaveBeenCalledTimes(2); + expect(pushes()).toHaveLength(3); }); - it('fails without force-pushing, pulling or rebasing in the translate flow', async () => { - mockExec.mockImplementation((cmd: string) => { - if (cmd === 'git status --porcelain') return Buffer.from('M locales/en.json'); - if (cmd.startsWith('git push')) rejectNonFastForward(); - return Buffer.from(''); - }); + it.each(['forbidden', 'noCredentials'] as const)( + 'fails at once with a permission error, without looking at the branch (%s)', + async (kind) => { + stubGit([OUTPUT[kind]]); + + await expect(githubService.autoCommitChanges('locales/**/*.json')).rejects.toBeInstanceOf(PushPermissionError); + expect(rebase).not.toHaveBeenCalled(); + expect(pushes()).toEqual([PUSH]); + } + ); - await expect(githubService.autoCommitChanges('locales/**/*.json')).rejects.toThrow('non-fast-forward'); + it.each(['network', 'signatureRule'] as const)( + 'retries any other failure three times, then fails with git\'s output (%s)', + async (kind) => { + stubGit([OUTPUT[kind], OUTPUT[kind], OUTPUT[kind]]); - const commands = mockExec.mock.calls.map(([cmd]) => cmd as string); - expect(new Set(commands.filter(cmd => cmd.startsWith('git push')))).toEqual( - new Set(['git push origin HEAD:feature-branch']) - ); - expect(commands.filter(cmd => REWRITING_GIT_COMMAND.test(cmd))).toEqual([]); + const failure = githubService.autoCommitChanges('locales/**/*.json'); + + await expect(failure).rejects.toThrow(OUTPUT[kind].split('\n')[0]); + await expect(failure).rejects.not.toBeInstanceOf(PushPermissionError); + expect(pushes()).toHaveLength(3); + expect(mockConsole.log).toHaveBeenCalledWith('Push failed, retrying (2/3)...'); + expect(rebase).not.toHaveBeenCalled(); + } + ); + + it('recovers from a network failure and a moved branch in the same push', async () => { + stubGit([OUTPUT.network, OUTPUT.fetchFirst, null]); + + const result = await githubService.autoCommitChanges('locales/**/*.json'); + + expect(result).toBe('new'); + expect(pushes()).toHaveLength(3); }); - it('amends with a lease on the checked-out remote ref in the sync flow, never a plain force push', async () => { - mockExec.mockImplementation((cmd: string) => { - if (cmd === 'git status --porcelain') return Buffer.from('M locales/sv.json'); - if (cmd === 'git log -1 --format=%ae') return Buffer.from('hi@localhero.ai'); - if (cmd === 'git log -1 --format= -p -- localhero.json') return Buffer.from('+ "syncTriggerId": "sync_abc"'); - if (cmd.startsWith('git push')) rejectNonFastForward(); - return Buffer.from(''); + describe('amending the sync trigger commit', () => { + const amendable = { + 'git log -1 --format=%ae': 'hi@localhero.ai', + 'git log -1 --format= -p -- localhero.json': '+ "syncTriggerId": "sync_abc"' + }; + + it('skips without replaying when the branch moved, leaving the sync to be retried', async () => { + stubGit([OUTPUT.staleInfo], amendable); + + const result = await githubService.autoCommitSyncChanges(['locales/sv.json']); + + expect(result).toBe('skipped'); + expect(rebase).not.toHaveBeenCalled(); + expect(pushes()).toEqual([AMEND_PUSH]); + expect(mockConsole.log).toHaveBeenCalledWith( + '::warning::Branch changed during the sync; skipping commit to avoid overwriting newer work. Sync again from Localhero to commit the translations.' + ); }); - await expect(githubService.autoCommitSyncChanges(['locales/sv.json'])).rejects.toThrow('non-fast-forward'); + it('fails on a permission error even though nothing moved', async () => { + stubGit([OUTPUT.forbidden], amendable); - const commands = mockExec.mock.calls.map(([cmd]) => cmd as string); - expect(new Set(commands.filter(cmd => cmd.startsWith('git push')))).toEqual( - new Set(['git push --force-with-lease origin HEAD:feature-branch']) - ); - expect(commands.filter(cmd => REWRITING_GIT_COMMAND.test(cmd))).toEqual([]); + await expect(githubService.autoCommitSyncChanges(['locales/sv.json'])).rejects.toBeInstanceOf(PushPermissionError); + }); }); });