diff --git a/.github/workflows/.size-baseline b/.github/workflows/.size-baseline index aeeb7d4a2e6..8f9ca4f5e15 100644 --- a/.github/workflows/.size-baseline +++ b/.github/workflows/.size-baseline @@ -37,7 +37,7 @@ 5942 qwen-autofix-fork-signal.yml 454547 qwen-autofix.yml 7061 qwen-ci-flaky-rerun.yml -193208 qwen-code-pr-review.yml +205562 qwen-code-pr-review.yml 79041 qwen-fleet-shepherd.yml 22680 qwen-issue-followup-bot.yml 5760 qwen-pr-safety-precheck.yml diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 092174deb5b..41b08b689a2 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -364,18 +364,33 @@ jobs: exit 0 fi # Fail closed: any API error or non-write permission denies the run. + # But ask more than once before a transient GitHub error becomes the + # denial: a 503 ("No server is currently available") here has + # silently dropped real /resolve requests — the commenter had write, + # no job started, and nothing on the PR said why. api_error_file="$(mktemp)" - if ! permission="$(gh api "repos/${GITHUB_REPOSITORY}/collaborators/${principal}/permission" --jq '.permission' 2>"$api_error_file")"; then + permission='' + attempt=0 + while :; do + attempt=$((attempt + 1)) + if permission="$(gh api "repos/${GITHUB_REPOSITORY}/collaborators/${principal}/permission" --jq '.permission' 2>"$api_error_file")"; then + break + fi api_error="$(cat "$api_error_file")" - rm -f "$api_error_file" api_error="${api_error:-unknown error}" api_error="${api_error//$'\r'/ }" api_error="${api_error//$'\n'/ }" + if [ "$attempt" -lt 3 ] && grep -qiE 'HTTP 5[0-9][0-9]|No server is currently available|connection reset|timed out|unexpected EOF' "$api_error_file"; then + echo "Permission API call for ${principal} failed transiently (attempt ${attempt} of 3): ${api_error}; retrying." + sleep $((attempt * 5)) + continue + fi + rm -f "$api_error_file" echo "::error::Permission API call failed for ${principal}: ${api_error}" echo "Failed to check permission for ${principal} (API error: ${api_error}); denying." >> "$GITHUB_STEP_SUMMARY" echo "should_review=false" >> "$GITHUB_OUTPUT" exit 0 - fi + done rm -f "$api_error_file" case "$permission" in admin|maintain|write) @@ -2593,11 +2608,11 @@ jobs: pr_json="${WORKDIR}/pr.json" gh pr view "$PR_NUMBER" \ --repo "$REPO" \ - --json state,isDraft,headRefName,headRefOid,headRepository,headRepositoryOwner,baseRefName,url,title \ + --json state,headRefName,headRefOid,headRepository,headRepositoryOwner,maintainerCanModify,baseRefName,url,title \ > "$pr_json" state="$(jq -r '.state' "$pr_json")" - is_draft="$(jq -r '.isDraft' "$pr_json")" + maintainer_can_modify="$(jq -r '.maintainerCanModify' "$pr_json")" head_ref="$(jq -r '.headRefName' "$pr_json")" head_sha="$(jq -r '.headRefOid' "$pr_json")" head_repo_owner="$(jq -r '.headRepositoryOwner.login // ""' "$pr_json")" @@ -2619,14 +2634,25 @@ jobs: if [ "$state" != "OPEN" ]; then finish_without_agent skip "PR #${PR_NUMBER} is ${state}." fi - if [ "$is_draft" = "true" ]; then - finish_without_agent skip "PR #${PR_NUMBER} is draft." - fi + # Drafts are not refused. /resolve is an explicit request by someone + # with write permission, and a draft with conflicts is exactly where + # resolving them is cheapest; the old gate only turned such requests + # into a skip comment (182 of them). The automatic review lane keeps + # its own draft gate — this is about the command only. + # # A deleted head repository makes headRepository null, so head_repo would # be "/" or "owner/" and the push URL malformed. Bail before the fetch. if [ -z "$head_repo_owner" ] || [ -z "$head_repo_name" ]; then finish_without_agent unsupported "PR #${PR_NUMBER}'s head repository was deleted; cannot push a resolution back." fi + # A fork PR is pushable only through "Allow edits by maintainers". + # When it is off, the agent run is wasted: the resolution is produced + # and then rejected at the push (10 runs so far). Say so up front. + # (GitHub also refuses maintainer edits on organization-owned forks + # even when the box is ticked; that case still surfaces at the push.) + if [ "$head_repo" != "$REPO" ] && [ "$maintainer_can_modify" != "true" ]; then + finish_without_agent unsupported "PR #${PR_NUMBER} comes from the fork \`${head_repo}\` with **Allow edits by maintainers** off, so a resolution could not be pushed back to it. Enable that option on the PR and re-run /resolve, or merge \`${base_ref}\` into \`${head_ref}\` locally." + fi # Fetch the PR head through refs/pull/N/head — the base repo mirrors it # for both same-repo and fork PRs — into a synthetic local tracking ref so @@ -3199,6 +3225,12 @@ jobs: HEAD_REF: '${{ needs.resolve-pr.outputs.head_ref }}' HEAD_SHA: '${{ needs.resolve-pr.outputs.head_sha }}' HEAD_REPO: '${{ needs.resolve-pr.outputs.head_repo }}' + # The replay merges the base branch again; without this the step's + # `set -u` aborts on the first ${BASE_REF} — a test pins env coverage. + BASE_REF: '${{ needs.resolve-pr.outputs.base_ref }}' + # Test hook for the replay's fetch URL; empty in production, where + # the replay fetches https://github.com/.git. + RESOLVE_ORIGIN_URL: '' OUTCOME: '${{ steps.verify.outputs.outcome }}' FAILURE_KIND: '${{ steps.verify.outputs.failure_kind }}' RESOLVE_OUTCOME: '${{ needs.resolve-pr.outputs.agent_outcome }}' @@ -3246,61 +3278,189 @@ jobs: } push_fail_reason='' + replayed_on='' + push_log="${WORKDIR}/push.log" + + # Push the resolved branch back to the PR head. For a fork PR the head + # lives in the contributor's repository (HEAD_REPO), reachable only via + # "Allow edits by maintainers"; for an in-repo PR HEAD_REPO == REPO, so + # the same push works. The token is passed inline so it never lands in + # .git/config (origin keeps its credential-free URL). git redacts the + # token from its own output; push.log is only grepped to classify the + # failure and is never echoed into the PR comment. + # SECURITY: the push token carries the `workflow` scope, so a conflict + # the agent resolves inside a .github/workflows/** file is pushed to the + # (possibly fork) head branch and then runs in that repo's Actions. This + # is bounded by the no-token agent, the scope guard (only base-changed + # files), and write+ maintainer authorization, and it lands in the + # contributor's own CI context — not this repo's. The push runs on a + # runner the agent never touched (this job), from a checkout whose + # refs came from GitHub and whose only agent-supplied input is the + # verified bundle, so nothing the agent could plant reaches it. + push_resolution() { + git push --no-verify \ + --force-with-lease="refs/heads/${HEAD_REF}:${HEAD_SHA}" \ + "https://x-access-token:${PUSH_TOKEN}@github.com/${HEAD_REPO}.git" \ + "HEAD:refs/heads/${HEAD_REF}" 2> "$push_log" + } + + # Classify in priority order: + # 1. workflow_scope — resolving merges the base branch in, which + # carries its .github/workflows/** changes; GitHub rejects any PAT + # without the `workflow` scope from updating workflow files. Anchor + # on GitHub's server phrase "refusing to allow ... workflow" — NOT a + # loose `workflow.*scope`, which the attacker-controlled branch name + # in git's `! [remote rejected] HEAD -> ` echo could trip + # (e.g. a branch named `fix-workflow-scope`), producing a comment + # that tells maintainers to grant the bot the workflow scope. + # 2. moved — the head branch advanced, so force-with-lease declined. + # Tested BEFORE permission: git echoes the destination ref into + # the same log (`HEAD -> (stale info)`), so a branch + # named e.g. fix/permission-prompt or fix-403-error would match a + # permission substring and the replay would never run. A genuine + # permission denial never carries a lease-decline phrase. The + # same echo cuts the other way: `force-with-lease` and + # `non-fast-forward` are legal branch-name substrings, so match + # git's parenthesised reason — `(stale info)`, `(fetch first)`, + # `(non-fast-forward)` — not the bare words, and anchor it to + # end-of-line: git prints the reason last on the rejection line, + # so a branch merely containing `(non-fast-forward)` still ends + # with the real reason and does not match. + # 3. permission — 403, or a 404 "not found" (GitHub hides repos a token + # can't see) which is an access problem in practice. + classify_push_failure() { + if grep -qiE 'refusing to allow.*workflow' "$push_log"; then + push_fail_reason='workflow_scope' + elif grep -qiE '\((stale info|fetch first|non-fast-forward)\)$' "$push_log"; then + push_fail_reason='moved' + elif grep -qiE '403|permission|not authorized|forbidden|protected branch|cannot be updated|not found|does not exist' "$push_log"; then + push_fail_reason='permission' + else + push_fail_reason='other' + fi + } + + # The head moved while the agent worked. That lost 17 of the 294 + # resolutions ever produced — the largest single loss after the agent + # itself — and nearly all such pushes are unrelated to the conflict + # (a CI fix, a review nit). So: fetch the new head, redo the merge on + # top of it, and wherever it still conflicts take the agent's version + # of that file IF the new head has not touched the file since the SHA + # the agent resolved from. Any other conflict gives up and reports + # "moved" exactly as before, with the original resolution attached. + # The replayed tree is then held to the same structural checks + # 'Resolution check' applied to the first merge — no markers, merges + # cleanly, only base-changed files — before it is pushed with a lease + # on the NEW head. The agent is never re-run. + replay_give_up() { + echo "Replay gave up: $1" + git merge --abort > /dev/null 2>&1 || true + git checkout -q -f "qwen-resolve/pr-${PR_NUMBER}" > /dev/null 2>&1 || true + } + replay_on_moved_head() { + local resolved_commit new_ref new_sha file + local markers merge_base base_changed replay_changed out_of_scope + # The replay commits (`git commit -C` below reuses the agent's + # author and message; the committer comes from the environment). + # The publish job's fresh checkout carries no committer identity — + # supply it here, never from config. + export GIT_COMMITTER_NAME='qwen-code-dev-bot' + export GIT_COMMITTER_EMAIL='qwen-code-dev-bot@users.noreply.github.com' + # Conflicted filenames are literal paths, not glob patterns: a file + # named `spec[1].md` would otherwise widen the per-file diff, + # checkout, and rm pathspecs to its sibling `spec1.md`. + export GIT_LITERAL_PATHSPECS=1 + resolved_commit="$(git rev-parse HEAD)" + new_ref="refs/remotes/origin/qwen-resolve/pr-${PR_NUMBER}/moved" + # Fetch by URL, not by remote name: the replay must not depend on + # the checkout's remote configuration, and RESOLVE_ORIGIN_URL + # exists so the fixture test can point this at a local bare + # repository. + git fetch "${RESOLVE_ORIGIN_URL:-https://github.com/${REPO}.git}" "+refs/pull/${PR_NUMBER}/head:${new_ref}" || { replay_give_up "could not fetch the new head."; return 1; } + new_sha="$(git rev-parse "$new_ref")" + if [ "$new_sha" = "$HEAD_SHA" ]; then + replay_give_up "the PR head is still ${HEAD_SHA}; the push was declined for another reason." + return 1 + fi + git checkout -q -B "qwen-resolve/pr-${PR_NUMBER}-replay" "$new_ref" || { replay_give_up "could not check out ${new_sha}."; return 1; } + if ! git merge --no-commit --no-ff "origin/${BASE_REF}" > /dev/null 2>&1; then + while IFS= read -r -d '' file; do + if git diff --quiet "$HEAD_SHA" "$new_sha" -- "$file"; then + # A deletion IS the agent's version of the file: checkout + # fails on a path absent from the resolved commit. + if git cat-file -e "${resolved_commit}:${file}" 2>/dev/null; then + git checkout "$resolved_commit" -- "$file" || { replay_give_up "could not take the agent's version of ${file}."; return 1; } + else + git rm -q -- "$file" || { replay_give_up "could not take the agent's deletion of ${file}."; return 1; } + fi + else + replay_give_up "${file} still conflicts and the new head changed it since ${HEAD_SHA}." + return 1 + fi + done < <(git diff --name-only --diff-filter=U -z) + fi + if git ls-files -u | grep -q .; then + replay_give_up "unresolved paths remain." + return 1 + fi + if git diff --cached --quiet; then + replay_give_up "merging ${BASE_REF} into ${new_sha} changes nothing." + return 1 + fi + git commit -q -C "$resolved_commit" || { replay_give_up "could not commit the replay."; return 1; } + markers="$(git diff --name-only -z --diff-filter=ACMRT "$new_sha" HEAD | + xargs -0 -r grep -InE -e '^(<<<<<<<|>>>>>>>) ' -- || true)" + if [ -n "$markers" ]; then + replay_give_up "conflict markers after replay." + return 1 + fi + if ! git merge-tree --write-tree "origin/${BASE_REF}" HEAD > /dev/null 2>&1; then + replay_give_up "the replay still conflicts with ${BASE_REF}." + return 1 + fi + merge_base="$(git merge-base "origin/${BASE_REF}" "$new_sha")" + base_changed="$(git diff --name-only -z "${merge_base}" "origin/${BASE_REF}" | sort -zu | tr '\0' '\n')" + replay_changed="$(git diff --name-only -z "$new_sha" HEAD | sort -zu | tr '\0' '\n')" + out_of_scope="$(comm -23 <(printf '%s\n' "$replay_changed") <(printf '%s\n' "$base_changed"))" + if [ -n "$out_of_scope" ]; then + replay_give_up "files outside the base-changed set: $(printf '%s ' $out_of_scope)" + return 1 + fi + replayed_on="$new_sha" + HEAD_SHA="$new_sha" + echo "Head moved to ${new_sha} while resolving; replayed the resolution on top of it." + return 0 + } + if [ "$OUTCOME" = "fixed" ] && [ "$DRY_RUN" != "true" ]; then if [ -z "${PUSH_TOKEN}" ]; then echo "::error::CI_DEV_BOT_PAT is required to push conflict fixes." exit 1 fi - # Push the resolved branch back to the PR head. For a fork PR the head - # lives in the contributor's repository (HEAD_REPO), reachable only via - # "Allow edits by maintainers"; for an in-repo PR HEAD_REPO == REPO, so - # the same push works. The token is passed inline so it never lands in - # .git/config (origin keeps its credential-free URL). git redacts the - # token from its own output; push.log is only grepped to classify the - # failure and is never echoed into the PR comment. - # SECURITY: the push token carries the `workflow` scope, so a conflict - # the agent resolves inside a .github/workflows/** file is pushed to the - # (possibly fork) head branch and then runs in that repo's Actions. This - # is bounded by the no-token agent, the scope guard (only base-changed - # files), and write+ maintainer authorization, and it lands in the - # contributor's own CI context — not this repo's. The push runs on a - # runner the agent never touched (this job), from a checkout whose - # refs came from GitHub and whose only agent-supplied input is the - # verified bundle, so nothing the agent could plant reaches it. - push_log="${WORKDIR}/push.log" - if git push --no-verify \ - --force-with-lease="refs/heads/${HEAD_REF}:${HEAD_SHA}" \ - "https://x-access-token:${PUSH_TOKEN}@github.com/${HEAD_REPO}.git" \ - "HEAD:refs/heads/${HEAD_REF}" 2> "$push_log"; then + if push_resolution; then : else - push_failed=true - # Classify in priority order: - # 1. workflow_scope — resolving merges the base branch in, which - # carries its .github/workflows/** changes; GitHub rejects any PAT - # without the `workflow` scope from updating workflow files. Anchor - # on GitHub's server phrase "refusing to allow ... workflow" — NOT a - # loose `workflow.*scope`, which the attacker-controlled branch name - # in git's `! [remote rejected] HEAD -> ` echo could trip - # (e.g. a branch named `fix-workflow-scope`), producing a comment - # that tells maintainers to grant the bot the workflow scope. - # 2. permission — 403, or a 404 "not found" (GitHub hides repos a token - # can't see) which is an access problem in practice. - # 3. moved — the head branch advanced, so force-with-lease declined. - if grep -qiE 'refusing to allow.*workflow' "$push_log"; then - push_fail_reason='workflow_scope' - elif grep -qiE '403|permission|not authorized|forbidden|protected branch|cannot be updated|not found|does not exist' "$push_log"; then - push_fail_reason='permission' - elif grep -qiE 'stale info|force-with-lease|non-fast-forward|fetch first' "$push_log"; then - push_fail_reason='moved' + classify_push_failure + if [ "$push_fail_reason" = "moved" ] && replay_on_moved_head && push_resolution; then + # The run artifact uploaded earlier describes the ORIGINAL + # resolution; what was just pushed is its replay onto the new + # head. Record the pushed tree so the upload step after this + # one preserves it beside the original (the replay-success + # comment points at it). + mkdir -p "${WORKDIR}/pushed" + git rev-parse HEAD > "${WORKDIR}/pushed/pushed.sha" || true + git diff "origin/${BASE_REF}...HEAD" > "${WORKDIR}/pushed/pushed.diff" || true else - push_fail_reason='other' + push_failed=true + # A replay that reached its own push leaves that push's log + # behind; classify whatever is there now. + classify_push_failure + echo "::error::Push to ${HEAD_REPO} failed (reason=${push_fail_reason})." + # Echo the git error for diagnosis with the token scrubbed. git already + # redacts the URL password to ***; this is belt-and-suspenders. + echo '--- git push stderr (token redacted) ---' + sed -E 's#x-access-token:[^@]*@#x-access-token:***@#g' "$push_log" || true fi - echo "::error::Push to ${HEAD_REPO} failed (reason=${push_fail_reason})." - # Echo the git error for diagnosis with the token scrubbed. git already - # redacts the URL password to ***; this is belt-and-suspenders. - echo '--- git push stderr (token redacted) ---' - sed -E 's#x-access-token:[^@]*@#x-access-token:***@#g' "$push_log" || true fi fi @@ -3325,6 +3485,8 @@ jobs: esac elif [ "$DRY_RUN" = "true" ]; then echo "Qwen Code resolved the merge conflicts in dry-run mode. No branch update was pushed." + elif [ -n "$replayed_on" ]; then + echo "Qwen Code resolved the merge conflicts and pushed the branch update. The head branch moved to \`${replayed_on}\` while the conflicts were being resolved; the resolution was replayed on top of it and re-checked before the push — where a file still conflicted and the new commits had not touched it, the replay took the agent's version. The pushed branch is that replay: its diff against \`${BASE_REF}\` is attached as the \`qwen-resolve-pr-${PR_NUMBER}-pushed\` artifact on the [workflow run](${RUN_URL}); the \`qwen-resolve-pr-${PR_NUMBER}-attempt-${AGENT_RUN_ATTEMPT}\` artifact describes the original resolution it was replayed from." else echo "Qwen Code resolved the merge conflicts and pushed the branch update." fi @@ -3378,3 +3540,15 @@ jobs: if [ "$OUTCOME" != "fixed" ] && [ "$OUTCOME" != "noop" ]; then exit 1 fi + + # Only populated when a moved head was replayed: the tree that was + # actually pushed, beside the attempt-suffixed run artifact that holds + # the original resolution. always() so it still runs when 'Report + # result' exits 1; the job-level gate already carries the decision check. + - name: 'Upload pushed tree' + if: '${{ always() }}' + uses: 'actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a' # v7.0.1 + with: + name: 'qwen-resolve-pr-${{ needs.resolve-pr.outputs.pr_number }}-pushed' + path: '${{ env.WORKDIR }}/pushed/' + if-no-files-found: 'ignore' diff --git a/scripts/tests/qwen-resolve-workflow.test.js b/scripts/tests/qwen-resolve-workflow.test.js index 4835e84ed8d..eb61e0c97bb 100644 --- a/scripts/tests/qwen-resolve-workflow.test.js +++ b/scripts/tests/qwen-resolve-workflow.test.js @@ -49,11 +49,31 @@ function step(section, name) { return match?.[0] ?? ''; } +// The behavioural tests below execute shell sliced out of the workflow's +// `run: |-` blocks; the index guards and the 10-space dedent live in one +// place so a drifted helper cannot silently extract a wrong range. +function extractBlock(source, startMarker, endMarker, options = {}) { + const { includeStart = true, includeEnd = true } = options; + const start = source.indexOf(startMarker); + const end = source.indexOf( + endMarker, + start === -1 ? 0 : start + startMarker.length, + ); + expect(start).toBeGreaterThan(-1); + expect(end).toBeGreaterThan(start); + const sliceStart = includeStart ? start : start + startMarker.length; + return source + .slice(sliceStart, includeEnd ? end + endMarker.length : end) + .replace(/^ {10}/gm, ''); +} + function reviewGhWrapper(runStep) { - const start = runStep.indexOf('cat > "$proxy_bin/gh" <<\'QWEN_GH_WRAPPER\''); - const bodyStart = runStep.indexOf('\n', start) + 1; - const end = runStep.indexOf('\n QWEN_GH_WRAPPER', bodyStart); - return runStep.slice(bodyStart, end).replace(/^ {10}/gm, ''); + return extractBlock( + runStep, + 'cat > "$proxy_bin/gh" <<\'QWEN_GH_WRAPPER\'\n', + '\n QWEN_GH_WRAPPER', + { includeStart: false, includeEnd: false }, + ); } function runReviewGhWrapper( @@ -1477,3 +1497,856 @@ describe('qwen resolve workflow', () => { expect(publishJob).toContain('Allow edits by maintainers'); }); }); + +// A /resolve request is lost after the agent succeeded whenever the push is +// declined, and before the agent even runs whenever a preflight refuses it. +// The suite below pins the recovery paths added for the largest of those +// losses (2026-06-25..08-27, 839 requests): the head moving during the run +// (17 resolutions lost), fork PRs the bot cannot push to (10 agent runs +// wasted), transient permission-API errors (silent denials), and the draft +// gate (182 skip comments on explicit requests). +describe('qwen resolve workflow: recovering requests that used to be lost', () => { + const workflow = readFileSync( + path.join(repoRoot, '.github/workflows/qwen-code-pr-review.yml'), + 'utf8', + ); + const resolveJob = job(workflow, 'resolve-pr'); + const publishJob = job(workflow, 'publish-resolution'); + const authorizeJob = job(workflow, 'authorize'); + const prepareStep = step(resolveJob, 'Prepare pull request branch'); + const reportStep = step(publishJob, 'Report result'); + const authorizeStep = step(authorizeJob, 'Check principal write permission'); + + // The replay functions, dedented out of the `run: |-` block so a real git + // fixture can exercise them exactly as the runner will. + function replayFunctions() { + return extractBlock( + reportStep, + 'replay_give_up() {', + '\n if [ "$OUTCOME" = "fixed" ]', + { includeEnd: false }, + ); + } + + const gitEnv = { + ...process.env, + GIT_AUTHOR_NAME: 'fixture', + GIT_AUTHOR_EMAIL: 'fixture@example.com', + GIT_COMMITTER_NAME: 'fixture', + GIT_COMMITTER_EMAIL: 'fixture@example.com', + GIT_CONFIG_GLOBAL: '/dev/null', + GIT_CONFIG_NOSYSTEM: '1', + }; + function git(cwd, ...args) { + const result = spawnSync('git', args, { + cwd, + env: gitEnv, + encoding: 'utf8', + }); + if (result.status !== 0) { + throw new Error( + `git ${args.join(' ')} failed in ${cwd}:\n${result.stdout}\n${result.stderr}`, + ); + } + return result.stdout.trim(); + } + + // Builds: a bare origin with `main` and refs/pull/1/head; a runner checkout + // on branch qwen-resolve/pr-1 holding the agent's resolution of a.txt (the + // conflicted file) merged on top of the ORIGINAL head; and a contributor + // clone that can move the head. Returns the paths and the original head SHA. + function makeFixture(resolveWith = 'edit', options = {}) { + const { conflicted = 'a.txt', sibling = null } = options; + const root = mkdtempSync(path.join(tmpdir(), 'qwen-resolve-replay-')); + const origin = path.join(root, 'origin.git'); + const contributor = path.join(root, 'contributor'); + const runner = path.join(root, 'runner'); + git(root, 'init', '-q', '--bare', origin); + git(root, 'init', '-q', '-b', 'main', contributor); + writeFileSync(path.join(contributor, conflicted), 'shared line\n'); + writeFileSync(path.join(contributor, 'b.txt'), 'b original\n'); + if (sibling) { + writeFileSync(path.join(contributor, sibling), 'sibling original\n'); + } + git(contributor, 'add', '.'); + git(contributor, 'commit', '-q', '-m', 'chore: seed'); + git(contributor, 'checkout', '-q', '-b', 'feature'); + writeFileSync(path.join(contributor, conflicted), 'feature side\n'); + git(contributor, 'commit', '-q', '-am', 'feat: feature side'); + const originalHead = git(contributor, 'rev-parse', 'HEAD'); + git(contributor, 'checkout', '-q', 'main'); + writeFileSync(path.join(contributor, conflicted), 'main side\n'); + writeFileSync(path.join(contributor, 'c.txt'), 'c from main\n'); + git(contributor, 'add', '.'); + git(contributor, 'commit', '-q', '-m', 'feat: main side'); + git(contributor, 'remote', 'add', 'origin', origin); + git( + contributor, + 'push', + '-q', + 'origin', + 'main', + 'feature:refs/pull/1/head', + ); + + git(root, 'clone', '-q', origin, runner); + git( + runner, + 'fetch', + '-q', + 'origin', + '+refs/pull/1/head:refs/remotes/origin/qwen-resolve/pr-1/head', + '+refs/heads/main:refs/remotes/origin/main', + ); + git( + runner, + 'checkout', + '-q', + '-B', + 'qwen-resolve/pr-1', + 'refs/remotes/origin/qwen-resolve/pr-1/head', + ); + // The agent's merge: a.txt conflicts, resolved to a line neither side had. + spawnSync('git', ['merge', '--no-commit', 'origin/main'], { + cwd: runner, + env: gitEnv, + }); + if (resolveWith === 'delete') { + // Deleting the conflicted file IS the agent's resolution of it. + // Literal pathspecs: a glob-named file must not take its wildcard + // siblings with it. + spawnSync('git', ['rm', '-q', '--', conflicted], { + cwd: runner, + env: { ...gitEnv, GIT_LITERAL_PATHSPECS: '1' }, + }); + } else { + writeFileSync(path.join(runner, conflicted), 'resolved by agent\n'); + spawnSync('git', ['add', '--', conflicted], { + cwd: runner, + env: { ...gitEnv, GIT_LITERAL_PATHSPECS: '1' }, + }); + } + git(runner, 'commit', '-q', '-m', 'fix: resolve merge conflicts with main'); + const resolvedCommit = git(runner, 'rev-parse', 'HEAD'); + return { root, origin, contributor, runner, originalHead, resolvedCommit }; + } + + function moveHead(fixture, file, content) { + git(fixture.contributor, 'checkout', '-q', 'feature'); + writeFileSync(path.join(fixture.contributor, file), content); + git(fixture.contributor, 'add', file); + git( + fixture.contributor, + 'commit', + '-q', + '-m', + `chore: move head (${file})`, + ); + git( + fixture.contributor, + 'push', + '-q', + 'origin', + 'feature:refs/pull/1/head', + ); + return git(fixture.contributor, 'rev-parse', 'HEAD'); + } + + function runReplay(fixture) { + // The production step has already scrubbed the workspace before the + // replay runs (#10428): no .git/config — so no `origin` remote and no + // user.name/email — and no global config. Reproduce that state so the + // replay must carry its own identity and fetch by URL. + const replayEnv = { ...gitEnv }; + for (const key of [ + 'GIT_AUTHOR_NAME', + 'GIT_AUTHOR_EMAIL', + 'GIT_COMMITTER_NAME', + 'GIT_COMMITTER_EMAIL', + ]) { + delete replayEnv[key]; + } + const script = [ + 'set -euo pipefail', + 'rm -f .git/config', + replayFunctions(), + 'replayed_on=""', + 'if replay_on_moved_head; then rc=0; else rc=$?; fi', + 'echo "rc=$rc"', + 'echo "HEAD_SHA=$HEAD_SHA"', + 'echo "replayed_on=$replayed_on"', + 'echo "head=$(git rev-parse HEAD)"', + 'echo "branch=$(git rev-parse --abbrev-ref HEAD)"', + 'echo "status=$(git status --porcelain | tr "\\n" ";")"', + ].join('\n'); + const result = spawnSync('bash', ['-c', script], { + cwd: fixture.runner, + env: { + ...replayEnv, + PR_NUMBER: '1', + HEAD_SHA: fixture.originalHead, + BASE_REF: 'main', + REPO: 'QwenLM/qwen-code', + // The replay fetches by URL (the scrub removes the `origin` remote + // with .git/config); point it at the fixture's bare repository. + RESOLVE_ORIGIN_URL: fixture.origin, + }, + encoding: 'utf8', + }); + const out = Object.fromEntries( + result.stdout + .split('\n') + .filter((line) => + /^(rc|HEAD_SHA|replayed_on|head|branch|status)=/.test(line), + ) + .map((line) => line.split(/=(.*)/s).slice(0, 2)), + ); + return { ...out, stdout: result.stdout, stderr: result.stderr }; + } + + it('replays the resolution when the head moved in files the conflict did not touch', () => { + const fixture = makeFixture(); + try { + const newHead = moveHead( + fixture, + 'b.txt', + 'b changed after the agent started\n', + ); + const out = runReplay(fixture); + expect(out.rc, out.stdout + out.stderr).toBe('0'); + // The lease for the second push is taken on the NEW head. + expect(out.HEAD_SHA).toBe(newHead); + expect(out.replayed_on).toBe(newHead); + expect(out.status).toBe(''); + // The replayed commit sits on the new head, merges the base, keeps the + // agent's resolution and the contributor's new change, and reuses the + // agent's commit message (CI rejects git's default merge message). + const parents = git( + fixture.runner, + 'log', + '-1', + '--format=%P', + out.head, + ).split(' '); + expect(parents).toContain(newHead); + expect(parents).toContain( + git(fixture.runner, 'rev-parse', 'origin/main'), + ); + expect(readFileSync(path.join(fixture.runner, 'a.txt'), 'utf8')).toBe( + 'resolved by agent\n', + ); + expect(readFileSync(path.join(fixture.runner, 'b.txt'), 'utf8')).toBe( + 'b changed after the agent started\n', + ); + expect(readFileSync(path.join(fixture.runner, 'c.txt'), 'utf8')).toBe( + 'c from main\n', + ); + expect(git(fixture.runner, 'log', '-1', '--format=%s', out.head)).toBe( + 'fix: resolve merge conflicts with main', + ); + // The scrub removed user.name/email with .git/config; the replay must + // bring the bot identity itself (the author is reused from the agent's + // commit by -C). + expect( + git(fixture.runner, 'log', '-1', '--format=%cn <%ce>', out.head), + ).toBe('qwen-code-dev-bot '); + // Only base-changed files differ from the new head: the scope guard holds. + expect( + git(fixture.runner, 'diff', '--name-only', newHead, out.head) + .split('\n') + .sort(), + ).toEqual(['a.txt', 'c.txt']); + } finally { + rmSync(fixture.root, { recursive: true, force: true }); + } + }); + + it('gives up, and restores the original resolution, when the new head touched a conflicted file', () => { + const fixture = makeFixture(); + try { + moveHead(fixture, 'a.txt', 'feature side, edited again\n'); + const out = runReplay(fixture); + expect(out.rc, out.stdout + out.stderr).toBe('1'); + expect(out.stdout).toContain( + 'Replay gave up: a.txt still conflicts and the new head changed it', + ); + // Nothing was taken from the agent's merge for a file the contributor + // rewrote, the lease SHA is untouched, and the checkout is back on the + // original resolution with a clean tree — the artifact and the "moved" + // comment describe exactly what exists. + expect(out.HEAD_SHA).toBe(fixture.originalHead); + expect(out.replayed_on).toBe(''); + expect(out.branch).toBe('qwen-resolve/pr-1'); + expect(out.head).toBe(fixture.resolvedCommit); + expect(out.status).toBe(''); + } finally { + rmSync(fixture.root, { recursive: true, force: true }); + } + }); + + it('gives up when the head did not move at all (the push failed for another reason)', () => { + const fixture = makeFixture(); + try { + const out = runReplay(fixture); + expect(out.rc, out.stdout + out.stderr).toBe('1'); + expect(out.stdout).toContain('the push was declined for another reason'); + expect(out.head).toBe(fixture.resolvedCommit); + expect(out.branch).toBe('qwen-resolve/pr-1'); + } finally { + rmSync(fixture.root, { recursive: true, force: true }); + } + }); + + it('wires the replay into the push path with a lease on the new head, and never re-runs the agent', () => { + expect(reportStep).toContain( + '[ "$push_fail_reason" = "moved" ] && replay_on_moved_head && push_resolution', + ); + // The lease still names ${HEAD_SHA}; the replay moves that variable to the + // new head instead of pushing with a bare --force. + expect(reportStep).toContain( + '--force-with-lease="refs/heads/${HEAD_REF}:${HEAD_SHA}"', + ); + expect(reportStep).toContain('HEAD_SHA="$new_sha"'); + expect(reportStep).not.toContain('--force '); + // The replay re-applies the structural checks of 'Resolution check'. + const replay = replayFunctions(); + expect(replay).toContain("grep -InE -e '^(<<<<<<<|>>>>>>>) '"); + expect(replay).toContain( + 'git merge-tree --write-tree "origin/${BASE_REF}" HEAD', + ); + expect(replay).toContain('comm -23 <(printf'); + expect(replay).not.toContain('qwen-code-action'); + // The credentialed steps remove the workspace .git/config — and with it + // the `origin` remote — before they run, so the replay must fetch by URL. + expect(replay).toContain( + 'git fetch "${RESOLVE_ORIGIN_URL:-https://github.com/${REPO}.git}"', + ); + expect(replay).not.toContain('git fetch origin'); + // And the comment says a replay happened, and where the pushed tree is: + // the first artifact holds the ORIGINAL resolution (uploaded before the + // replay ran), so a replay records what it pushed for a second upload. + expect(reportStep).toContain('the resolution was replayed on top of it'); + expect(reportStep).toContain( + 'git diff "origin/${BASE_REF}...HEAD" > "${WORKDIR}/pushed/pushed.diff"', + ); + expect(reportStep).toContain('qwen-resolve-pr-${PR_NUMBER}-pushed'); + const pushedUpload = step(publishJob, 'Upload pushed tree'); + expect(pushedUpload).toContain( + "name: 'qwen-resolve-pr-${{ needs.resolve-pr.outputs.pr_number }}-pushed'", + ); + expect(pushedUpload).toContain("path: '${{ env.WORKDIR }}/pushed/'"); + // always() (not the resolve-pr prepare gate, which has no meaning here); + // quote style is the workflow author's choice, so match either. + expect(pushedUpload).toMatch(/if:\s*['"]\$\{\{ always\(\) \}\}['"]/); + expect(pushedUpload).not.toContain('steps.prepare.outputs.decision'); + expect(publishJob.indexOf("- name: 'Upload pushed tree'")).toBeGreaterThan( + publishJob.indexOf("- name: 'Report result'"), + ); + }); + + it('defines every uppercase variable the resolve-pr and publish run blocks expand', () => { + // The steps run under `set -u`; a ${NAME} the env block does not define + // aborts the step at first use. The replay was shipped once with BASE_REF + // missing from 'Report result' — the fixture test injected it, so nothing + // noticed. Cover every run block of the job, not just that one. + const builtins = new Set([ + 'GITHUB_OUTPUT', + 'GITHUB_REPOSITORY', + 'GITHUB_STEP_SUMMARY', + 'GITHUB_ENV', + 'GITHUB_PATH', + 'RUNNER_TEMP', + 'HOME', + 'PATH', + 'RANDOM', + 'PIPESTATUS', + 'BASH_REMATCH', + ]); + for (const jobText of [resolveJob, publishJob]) { + const jobEnv = new Set( + [...jobText.matchAll(/^ {6}([A-Z][A-Z0-9_]*): /gm)].map((m) => m[1]), + ); + const stepBlocks = jobText.split(/\n {6}- name: /).slice(1); + for (const block of stepBlocks) { + if (!/\n {8}run: \|-?\n/.test(block)) { + continue; + } + const name = block.slice(0, block.indexOf('\n')); + const stepEnv = new Set( + [...block.matchAll(/^ {10}([A-Z][A-Z0-9_]*): /gm)].map((m) => m[1]), + ); + const run = block.slice(block.search(/\n {8}run: \|-?\n/)); + const assigned = new Set( + [...run.matchAll(/(?:^|[\s;{(])([A-Z][A-Z0-9_]*)=/gm)].map( + (m) => m[1], + ), + ); + const referenced = new Set( + [...run.matchAll(/\$\{?([A-Z][A-Z0-9_]+)\b/g)] + .map((m) => m[1]) + .filter((v) => !v.startsWith('GITHUB_')), + ); + const missing = [...referenced].filter( + (v) => + !jobEnv.has(v) && + !stepEnv.has(v) && + !assigned.has(v) && + !builtins.has(v), + ); + expect( + missing, + `step ${name} expands undefined: ${missing.join(', ')}`, + ).toEqual([]); + } + } + // And the one that shipped missing is now there (the replay lives in the + // publish job, whose refs come from the agent job's outputs). + expect(step(publishJob, 'Report result')).toContain( + "BASE_REF: '${{ needs.resolve-pr.outputs.base_ref }}'", + ); + }); + + it('refuses fork PRs the bot cannot push to before spending an agent run', () => { + expect(prepareStep).toContain('maintainerCanModify'); + expect(prepareStep).toContain( + '[ "$head_repo" != "$REPO" ] && [ "$maintainer_can_modify" != "true" ]', + ); + expect(prepareStep).toContain('**Allow edits by maintainers** off'); + // Same-repo PRs report maintainerCanModify=false too; the fork check must + // be conjoined with the head-repo comparison or every in-repo PR is refused. + expect(prepareStep).not.toMatch( + /\n\s+if \[ "\$maintainer_can_modify" != "true" \]; then/, + ); + }); + + it('no longer refuses draft PRs', () => { + // /resolve is an explicit request by a writer; a draft with conflicts is + // where resolving is cheapest. The automatic review lane keeps its gate. + expect(prepareStep).not.toContain('is draft.'); + expect(prepareStep).not.toContain('isDraft'); + expect(job(workflow, 'delay-automatic-review')).toContain('is draft.'); + }); + + it('retries a transient permission-API error before denying, and still fails closed', () => { + expect(authorizeJob).toContain( + 'failed transiently (attempt ${attempt} of 3)', + ); + expect(authorizeJob).toContain('No server is currently available'); + // The strings above stay put if the loop itself regresses: the bound, + // the backoff, and the retry branch are pinned by the behavioural tests + // below, and their YAML anchors here. + expect(authorizeJob).toContain('[ "$attempt" -lt 3 ]'); + expect(authorizeJob).toContain('sleep $((attempt * 5))'); + // After the retries the original fail-closed denial is unchanged. + expect(authorizeJob).toContain( + '::error::Permission API call failed for ${principal}: ${api_error}', + ); + expect(authorizeJob).toContain( + 'echo "should_review=false" >> "$GITHUB_OUTPUT"', + ); + }); + + // --- classify_push_failure: order pin + behaviour against real push logs --- + + it('tests the lease-decline signature before the permission patterns', () => { + // git echoes the destination branch into the push log; the permission + // patterns are substrings of plausible branch names, the moved patterns + // are server phrases a branch name cannot contain. Order is the guard. + const scope = reportStep.indexOf("push_fail_reason='workflow_scope'"); + const moved = reportStep.indexOf("push_fail_reason='moved'"); + const permission = reportStep.indexOf("push_fail_reason='permission'"); + expect(scope).toBeGreaterThan(-1); + expect(scope).toBeLessThan(moved); + expect(moved).toBeLessThan(permission); + }); + + function classifyFunction() { + return extractBlock( + reportStep, + 'classify_push_failure() {', + '\n }', + ); + } + + function classifyScript(lines, scriptLines, extraEnv = {}) { + const dir = mkdtempSync(path.join(tmpdir(), 'qwen-classify-')); + try { + const pushLog = path.join(dir, 'push.log'); + writeFileSync(pushLog, `${lines.join('\n')}\n`); + return spawnSync( + 'bash', + [ + '-c', + ['set -euo pipefail', classifyFunction(), ...scriptLines].join('\n'), + ], + { + encoding: 'utf8', + env: { ...process.env, push_log: pushLog, ...extraEnv }, + }, + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + } + + function classifyLog(lines) { + const result = classifyScript(lines, [ + 'classify_push_failure', + 'printf "reason=%s\\n" "$push_fail_reason"', + ]); + expect(result.status, result.stdout + result.stderr).toBe(0); + return result.stdout.match(/^reason=(.*)$/m)?.[1]; + } + + const staleInfoLog = (branch) => [ + 'To github.com:contributor/repo.git', + ` ! [remote rejected] HEAD -> ${branch} (stale info)`, + "error: failed to push some refs to 'github.com/contributor/repo.git'", + ]; + + it('classifies a moved head by its lease decline even when the branch name echoes a permission substring', () => { + expect(classifyLog(staleInfoLog('feature'))).toBe('moved'); + expect(classifyLog(staleInfoLog('fix/permission-prompt'))).toBe('moved'); + expect(classifyLog(staleInfoLog('fix-403-error'))).toBe('moved'); + // A branch that merely CONTAINS a moved-reason phrase — even parenthesised, + // which refnames legally allow — is not a lease decline: git prints the + // real reason last on the line, and the moved arm anchors to end-of-line. + // These also pin the parenthesised patterns against a bare-word + // regression, which every one of these branch names would trip. + expect( + classifyLog([ + ' ! [remote rejected] HEAD -> x(non-fast-forward)y (protected branch hook declined)', + ]), + ).toBe('permission'); + expect( + classifyLog([ + ' ! [remote rejected] HEAD -> fix/non-fast-forward (cannot be updated)', + ]), + ).toBe('permission'); + expect( + classifyLog([ + ' ! [remote rejected] HEAD -> fix/non-fast-forward-retry (pre-receive hook declined)', + "fatal: unable to access 'https://github.com/contributor/repo.git/': The requested URL returned error: 403", + ]), + ).toBe('permission'); + // Genuine access problems still classify as permission, the workflow-scope + // arm keeps its priority over both, and unknown failures stay 'other'. + expect( + classifyLog([ + "fatal: unable to access 'https://github.com/contributor/repo.git/': The requested URL returned error: 403", + ]), + ).toBe('permission'); + expect( + classifyLog([ + 'remote: refusing to allow an OAuth App to create or update workflow `.github/workflows/ci.yml` without `workflow` scope', + ]), + ).toBe('workflow_scope'); + expect(classifyLog(['error: something else entirely'])).toBe('other'); + }); + + it('re-classifies from the current push log when the replay push fails for a new reason', () => { + // After a failed replay push, classify_push_failure runs a second time; + // the reported reason must come from the second push's log, not the stale + // 'moved' from the first. + const result = classifyScript( + staleInfoLog('feature'), + [ + 'classify_push_failure', + 'printf "first=%s\\n" "$push_fail_reason"', + 'printf "%s\\n" "$SECOND_LOG" > "$push_log"', + 'classify_push_failure', + 'printf "second=%s\\n" "$push_fail_reason"', + ], + { + SECOND_LOG: + "fatal: unable to access 'https://github.com/contributor/repo.git/': The requested URL returned error: 403", + }, + ); + expect(result.status, result.stdout + result.stderr).toBe(0); + expect(result.stdout).toContain('first=moved'); + expect(result.stdout).toContain('second=permission'); + }); + + it('says in the replay comment what the run artifact does and does not describe', () => { + // The artifact's pr.diff is computed before the replay runs; the pushed + // tree is the replay. The comment must say so, and must not claim files + // were taken from the agent's merge when the replay merged clean. + const armStart = reportStep.indexOf('elif [ -n "$replayed_on" ]; then'); + expect(armStart).toBeGreaterThan(-1); + const arm = reportStep.slice( + armStart, + reportStep.indexOf('\n else', armStart), + ); + expect(arm).toContain('the resolution was replayed on top of it'); + expect(arm).toContain( + 'where a file still conflicted and the new commits had not touched it', + ); + expect(arm).toContain( + 'describes the original resolution it was replayed from', + ); + }); + + // --- replay fixtures: the empty-merge give-up and deletion resolutions --- + + // The contributor merges the base into their PR branch while the agent + // runs; the replay's re-merge of the base then changes nothing, so the + // lane must give up cleanly instead of committing an unchanged tree. + function mergeMainIntoHead(fixture) { + git(fixture.contributor, 'checkout', '-q', 'feature'); + const merge = spawnSync('git', ['merge', '--no-edit', 'main'], { + cwd: fixture.contributor, + env: gitEnv, + encoding: 'utf8', + }); + // a.txt still conflicts between feature and main; the contributor keeps + // their side and completes the merge themselves. + expect(merge.status).not.toBe(0); + writeFileSync( + path.join(fixture.contributor, 'a.txt'), + 'feature side, merged by the contributor\n', + ); + git(fixture.contributor, 'add', 'a.txt'); + git(fixture.contributor, 'commit', '-q', '--no-edit'); + git( + fixture.contributor, + 'push', + '-q', + 'origin', + 'feature:refs/pull/1/head', + ); + return git(fixture.contributor, 'rev-parse', 'HEAD'); + } + + it('gives up cleanly when the new head already merged the base itself', () => { + const fixture = makeFixture(); + try { + const newHead = mergeMainIntoHead(fixture); + expect(newHead).not.toBe(fixture.originalHead); + const out = runReplay(fixture); + expect(out.rc, out.stdout + out.stderr).toBe('1'); + expect(out.stdout).toContain('changes nothing'); + expect(out.HEAD_SHA).toBe(fixture.originalHead); + expect(out.replayed_on).toBe(''); + expect(out.branch).toBe('qwen-resolve/pr-1'); + expect(out.head).toBe(fixture.resolvedCommit); + expect(out.status).toBe(''); + } finally { + rmSync(fixture.root, { recursive: true, force: true }); + } + }); + + it('replays a resolution that deleted the conflicted file', () => { + const fixture = makeFixture('delete'); + try { + const newHead = moveHead( + fixture, + 'b.txt', + 'b changed after the agent started\n', + ); + const out = runReplay(fixture); + expect(out.rc, out.stdout + out.stderr).toBe('0'); + expect(out.HEAD_SHA).toBe(newHead); + expect(out.replayed_on).toBe(newHead); + // The agent's version of a.txt IS its deletion: the pushed tree must + // not have the file back, and the scope guard still holds. + expect(existsSync(path.join(fixture.runner, 'a.txt'))).toBe(false); + expect( + git(fixture.runner, 'ls-tree', '--name-only', out.head), + ).not.toContain('a.txt'); + expect( + git(fixture.runner, 'diff', '--name-only', newHead, out.head) + .split('\n') + .sort(), + ).toEqual(['a.txt', 'c.txt']); + } finally { + rmSync(fixture.root, { recursive: true, force: true }); + } + }); + + // Conflicted filenames legally carry glob characters; the replay's per-file + // diff/checkout/rm pathspecs must stay literal (GIT_LITERAL_PATHSPECS) or a + // file named `a[1].txt` widens every one of them to its sibling `a1.txt`. + it('replays when the head moved only in a glob sibling of the conflicted file', () => { + const fixture = makeFixture('edit', { + conflicted: 'a[1].txt', + sibling: 'a1.txt', + }); + try { + const newHead = moveHead( + fixture, + 'a1.txt', + 'sibling edited by the new head\n', + ); + const out = runReplay(fixture); + expect(out.rc, out.stdout + out.stderr).toBe('0'); + expect(out.HEAD_SHA).toBe(newHead); + expect(out.replayed_on).toBe(newHead); + // Both files intact: the agent's resolution of the conflicted file and + // the new head's edit of its wildcard sibling. + expect(readFileSync(path.join(fixture.runner, 'a[1].txt'), 'utf8')).toBe( + 'resolved by agent\n', + ); + expect(readFileSync(path.join(fixture.runner, 'a1.txt'), 'utf8')).toBe( + 'sibling edited by the new head\n', + ); + } finally { + rmSync(fixture.root, { recursive: true, force: true }); + } + }); + + it('keeps a glob sibling alive when a deletion resolution is replayed', () => { + const fixture = makeFixture('delete', { + conflicted: 'a[1].txt', + sibling: 'a1.txt', + }); + try { + const newHead = moveHead( + fixture, + 'a1.txt', + 'sibling edited by the new head\n', + ); + const out = runReplay(fixture); + expect(out.rc, out.stdout + out.stderr).toBe('0'); + expect(out.HEAD_SHA).toBe(newHead); + expect(out.replayed_on).toBe(newHead); + // The agent deleted a[1].txt; `git rm` must not have staged its + // wildcard sibling along with it. + expect(existsSync(path.join(fixture.runner, 'a[1].txt'))).toBe(false); + expect(readFileSync(path.join(fixture.runner, 'a1.txt'), 'utf8')).toBe( + 'sibling edited by the new head\n', + ); + expect( + git(fixture.runner, 'ls-tree', '--name-only', out.head).split('\n'), + ).toContain('a1.txt'); + } finally { + rmSync(fixture.root, { recursive: true, force: true }); + } + }); + + // --- authorize retry loop: behaviour against a scripted gh --- + + function authorizeRetryBlock() { + return extractBlock( + authorizeStep, + 'api_error_file="$(mktemp)"', + '\n esac', + ); + } + + function runAuthorizeRetry(plan) { + const tempDir = mkdtempSync(path.join(tmpdir(), 'qwen-authorize-retry-')); + try { + const binDir = path.join(tempDir, 'bin'); + mkdirSync(binDir); + const planFile = path.join(tempDir, 'plan.txt'); + const callLog = path.join(tempDir, 'calls.log'); + const sleepLog = path.join(tempDir, 'sleeps.log'); + const outputFile = path.join(tempDir, 'output'); + const summaryFile = path.join(tempDir, 'summary'); + writeFileSync(planFile, `${plan.join('\n')}\n`); + writeFileSync(callLog, ''); + writeFileSync(sleepLog, ''); + writeFileSync(outputFile, ''); + writeFileSync(summaryFile, ''); + writeFileSync( + path.join(binDir, 'gh'), + [ + '#!/usr/bin/env bash', + 'set -euo pipefail', + 'printf "%s\\n" "$*" >> "$AUTHORIZE_CALL_LOG"', + 'n="$(cat "$AUTHORIZE_CALL_COUNT" 2>/dev/null || echo 0)"', + 'n=$((n + 1))', + 'printf "%s\\n" "$n" > "$AUTHORIZE_CALL_COUNT"', + 'line="$(sed -n "${n}p" "$AUTHORIZE_PLAN_FILE")"', + 'case "$line" in', + ' ok:*) printf "%s\\n" "${line#ok:}" ;;', + ' fail:*) printf "%s\\n" "${line#fail:}" >&2; exit 1 ;;', + 'esac', + ].join('\n'), + ); + chmodSync(path.join(binDir, 'gh'), 0o755); + const result = spawnSync( + 'bash', + [ + '-c', + [ + 'set -euo pipefail', + 'sleep() { printf "%s\\n" "$1" >> "$AUTHORIZE_SLEEP_LOG"; }', + 'principal=commenter', + 'GITHUB_REPOSITORY=owner/repo', + authorizeRetryBlock(), + ].join('\n'), + ], + { + encoding: 'utf8', + env: { + ...process.env, + PATH: `${binDir}${path.delimiter}${process.env.PATH}`, + AUTHORIZE_PLAN_FILE: planFile, + AUTHORIZE_CALL_COUNT: path.join(tempDir, 'count'), + AUTHORIZE_CALL_LOG: callLog, + AUTHORIZE_SLEEP_LOG: sleepLog, + GITHUB_OUTPUT: outputFile, + GITHUB_STEP_SUMMARY: summaryFile, + }, + }, + ); + return { + ...result, + output: readFileSync(outputFile, 'utf8'), + summary: readFileSync(summaryFile, 'utf8'), + calls: readFileSync(callLog, 'utf8').split('\n').filter(Boolean), + sleeps: readFileSync(sleepLog, 'utf8').split('\n').filter(Boolean), + }; + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } + } + + const TRANSIENT_503 = 'fail:HTTP 503: No server is currently available'; + + it('retries a transient permission-API error twice, then allows the writer', () => { + const out = runAuthorizeRetry([TRANSIENT_503, TRANSIENT_503, 'ok:write']); + expect(out.status, out.stdout + out.stderr).toBe(0); + expect(out.output).toContain('should_review=true'); + expect(out.calls).toHaveLength(3); + for (const call of out.calls) { + expect(call).toBe( + 'api repos/owner/repo/collaborators/commenter/permission --jq .permission', + ); + } + expect(out.sleeps).toEqual(['5', '10']); + expect(out.stdout).toContain('failed transiently (attempt 1 of 3)'); + expect(out.stdout).toContain('failed transiently (attempt 2 of 3)'); + }); + + it('denies after three transient failures — fail closed', () => { + const out = runAuthorizeRetry([ + TRANSIENT_503, + TRANSIENT_503, + TRANSIENT_503, + ]); + expect(out.status, out.stdout + out.stderr).toBe(0); + expect(out.output).toContain('should_review=false'); + expect(out.output).not.toContain('should_review=true'); + expect(out.calls).toHaveLength(3); + expect(out.stdout).toContain( + '::error::Permission API call failed for commenter', + ); + expect(out.summary).toContain( + 'Failed to check permission for commenter (API error:', + ); + }); + + it('denies a non-transient permission-API error on the first attempt', () => { + const out = runAuthorizeRetry(['fail:HTTP 404: Not Found']); + expect(out.status, out.stdout + out.stderr).toBe(0); + expect(out.output).toContain('should_review=false'); + expect(out.output).not.toContain('should_review=true'); + expect(out.calls).toHaveLength(1); + expect(out.sleeps).toEqual([]); + expect(out.summary).toContain( + 'Failed to check permission for commenter (API error:', + ); + }); +});