From a3042125d82729fca3333189b3eeee18c06049ef Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 29 Aug 2026 01:59:42 +0800 Subject: [PATCH 01/12] fix(ci): run /resolve without the container sandbox and pin its CLI version Every /resolve since 2026-08-15 has failed before the agent looked at the conflict: 0 of 81 agent runs pushed a resolution, against 84% of the 273 runs before that date. Three causes, none of them the model: - 14 runs: `@qwen-code/qwen-code@latest` pointed at an unresolvable 0.21.12 and the action's npm install died on `notarget`. - 24 runs: #9252 made the settings block take effect for the first time, `"sandbox": true` with it; the CLI hopped into the versioned ghcr image, which lags the npm release, and died on `manifest unknown` (#9898). - 40 runs: with the image present the sandboxed agent produced no output until the 120-minute job timeout; the merge had not even started. No sandboxed /resolve run has ever pushed a resolution and every pushed one ran outside the container, so pin tools.sandbox to false, pin the CLI to 0.21.10 (the last version with a measured record on this job), give the agent step a 75-minute timeout (p99 is 70), and when the step ends without outcome=success report it as an infrastructure failure that a re-run will repeat, instead of the generic "did not complete successfully" that read as the model giving up and drew 13 days of re-runs. The two contract tests that pinned sandbox: true now pin false; new tests pin the exact-semver version input, the step timeout mirrored into the comment, and the infra-failure wording. --- .github/scripts/qwen-triage-workflow.test.mjs | 44 +++++++--- .github/workflows/qwen-code-pr-review.yml | 84 +++++++++++++++---- scripts/tests/qwen-resolve-workflow.test.js | 60 ++++++++++++- 3 files changed, 157 insertions(+), 31 deletions(-) diff --git a/.github/scripts/qwen-triage-workflow.test.mjs b/.github/scripts/qwen-triage-workflow.test.mjs index ad2a9cf44ea..820a3ecc00c 100644 --- a/.github/scripts/qwen-triage-workflow.test.mjs +++ b/.github/scripts/qwen-triage-workflow.test.mjs @@ -241,10 +241,15 @@ describe('qwen-triage: agent tool/permission settings', () => { // The same settings_json → settings bug survived in two more workflows after // the triage fix. An unknown action input is dropped without error, so the -// /resolve agent ran every time with no turn cap, no tool allowlist, and no -// sandbox — on a runner pool its runs-on comment chose specifically because -// `sandbox: true` needs docker — and the follow-up bot ran uncapped too. -// These blocks were therefore never validated by anything; parse them here. +// /resolve agent ran every time with no turn cap and no tool allowlist, and +// the follow-up bot ran uncapped too. These blocks were therefore never +// validated by anything; parse them here. +// +// The sandbox key is pinned to FALSE, not true. The fix that made the block +// take effect (#9252) also switched the container on, and /resolve went from +// 84% pushed to 0 of 81 in a row: the versioned ghcr image lags the npm +// release (#9898) so the agent died at startup, or it hung silently to the job +// timeout. No sandboxed /resolve run has ever pushed a resolution. describe('qwen-code-pr-review.yml resolve-pr: agent settings', () => { it('passes `settings:` (not the silently-dropped `settings_json:`)', () => { assertSettingsContract(resolveConflictsStep, 'resolve_conflicts'); @@ -275,22 +280,37 @@ describe('qwen-code-pr-review.yml resolve-pr: agent settings', () => { ]) { assert.ok(core.includes(t), `tools.core must include ${t}`); } - // The runs-on comment pins this job to hosted runners because the sandbox - // needs docker; dropping the key would pay that routing cost for nothing. + // Explicit false, not absent: absent means "whatever the CLI defaults to + // or QWEN_SANDBOX says", and the one time this flipped on it took the + // command down for 13 days (see the describe comment). assert.equal( settings.tools?.sandbox, - true, - 'tools.sandbox must stay true — the runs-on routing depends on it', + false, + 'tools.sandbox must stay false — every sandboxed /resolve run has failed; re-enable only with a dry-run that finishes inside the container', + ); + }); + + it('pins the CLI version instead of following `latest`', () => { + // `latest` ties the job to the npm release pipeline of the moment: on + // 2026-08-15 the dist-tag pointed at an unresolvable 0.21.12 and every + // /resolve died on `npm error notarget` before the agent started. + const version = resolveConflictsStep.with?.qwen_cli_version; + assert.match( + String(version), + /^\d+\.\d+\.\d+$/, + 'qwen_cli_version must be an exact semver, not a dist-tag', ); }); - it('keeps resolve-pr on hosted runners (sandbox: true needs docker)', () => { - // The routing half of the sandbox coupling: the ECS pool ships no - // container runtime, so an ECS-routed sandboxed agent dies at startup. + it('keeps resolve-pr on an ephemeral hosted runner', () => { + // This job merges the base branch and force-pushes to the PR head; a fresh + // runner is the cheapest guarantee that nothing from an earlier attempt + // is carried into the credentialed push. (It used to be pinned for docker + // as well; the agent no longer runs sandboxed, so that half is moot.) assert.equal( resolvePrJob['runs-on'], 'ubuntu-latest', - 'resolve-pr must stay on hosted runners — sandbox: true needs docker, absent on the ECS pool', + 'resolve-pr must stay on an ephemeral hosted runner', ); }); }); diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 0c5e046b9a0..1b4f7882071 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -2384,7 +2384,8 @@ jobs: # `true`. The variable is daemon state, and autofix's preflight is what # checks it up front, failing the job in seconds instead of at agent # startup. Anything moving a sandboxed job onto the pool should copy that - # preflight — see #9556. + # preflight — see #9556. (Moot for now: the agent step below runs with + # tools.sandbox false, for the reasons recorded on its settings block.) runs-on: 'ubuntu-latest' timeout-minutes: 120 # Shared with qwen-autofix.yml's review-address job — see the rationale @@ -2594,6 +2595,14 @@ jobs: if: "steps.prepare.outputs.decision == 'run'" id: 'resolve_conflicts' uses: 'QwenLM/qwen-code-action@05f81718976f3b7da657422e6e8e4d372b8621d7' + # A pushed resolution takes 6 minutes at the median, 41 at p95 and 70 + # at p99 (294 runs, request to result comment); only two ever took + # longer than 75. The 120-minute job timeout has therefore never ended + # a healthy run, only hung ones — and a hang billed the full 120 + # minutes 40 times in 2026-08. 75 keeps the p99 tail and cuts that + # bill by a third. Keep AGENT_TIMEOUT_MINUTES on 'Report result' in + # step with this value; a test pins the pair. + timeout-minutes: 75 env: PR_NUMBER: '${{ steps.resolve.outputs.pr_number }}' BASE_REF: '${{ steps.prepare.outputs.base_ref }}' @@ -2602,14 +2611,37 @@ jobs: OPENAI_API_KEY: '${{ secrets.OPENAI_API_KEY }}' OPENAI_BASE_URL: '${{ secrets.OPENAI_BASE_URL }}' OPENAI_MODEL: '${{ vars.QWEN_PR_REVIEW_MODEL }}' + # Pinned, not `latest`. The action installs the CLI from npm at run + # time, so `latest` makes every /resolve depend on the release + # pipeline of the moment: on 2026-08-15 the dist-tag moved to + # 0.21.12 before the tarball was resolvable and 14 runs in a row + # died on `npm error notarget`. 0.21.10 is the last version with a + # measured record on this job (16 of 19 requests pushed on + # 2026-08-12). Bump it on purpose, the way `uses:` above is bumped, + # after a dry-run /resolve on the new version. + qwen_cli_version: '0.21.10' # The input name is `settings` — this action version has no # `settings_json` input, and an unknown input is silently dropped. - # That is exactly what happened to this block: every /resolve run so - # far ignored it, and the agent ran without the turn cap, toolset - # allowlist, or the sandbox the runs-on comment above assumes. + # That is exactly what happened to this block until #9252: every + # /resolve run before it ignored the block, and the agent ran without + # the turn cap or toolset allowlist. # tools.core specifiers (e.g. `run_shell_command(git add)`) are advisory: # the permission manager keys on the tool name and drops the parenthesised - # command. Real containment = sandbox + write authorization + no agent token. + # command. Real containment = write authorization + no agent token + + # the scope guard in 'Resolution check' + an ephemeral runner. + # + # tools.sandbox is deliberately false. The day #9252 made this block + # take effect, `sandbox: true` took effect with it, and /resolve went + # from 84% pushed (273 agent runs, 2026-06-25..08-14) to 0 of 81 + # (2026-08-15..08-27). Inside the container the agent either died at + # startup — the versioned `ghcr.io/qwenlm/qwen-code:` image lags + # the npm release (#9898: `manifest unknown` on 0.21.13, 0.22.0 and + # 0.22.1; 24 runs) — or produced no output at all until the job + # timeout (40 runs; `git status` afterwards showed the merge had not + # even started). No sandboxed /resolve run has ever pushed a + # resolution; every pushed one ran without the container. Turning the + # sandbox back on needs the same evidence the other way: a dry-run + # with `qwen_debug` that shows the agent finishing inside it. settings: |- { "model": { @@ -2634,7 +2666,7 @@ jobs: "run_shell_command(mkdir)", "run_shell_command(pwd)" ], - "sandbox": true + "sandbox": false } } prompt: |- @@ -2690,8 +2722,12 @@ jobs: # and the merge-tree check below would misreport "still has conflicts". # Surface the real cause instead. if [ "$RESOLVE_OUTCOME" != "success" ]; then - echo "The conflict-resolution agent step did not succeed (outcome=${RESOLVE_OUTCOME}): API/quota/infrastructure error or cancellation. Check its logs." + echo "The conflict-resolution agent step did not succeed (outcome=${RESOLVE_OUTCOME}): CLI install, model endpoint or infrastructure error, its step timeout, or cancellation. Check its logs." echo "outcome=failed" >> "$GITHUB_OUTPUT" + # Distinguish "never reached a verdict" from "reached a wrong one": + # 'Report result' words the comment differently and does not invite + # a re-run, because a re-run repeats an infrastructure failure. + echo "failure_kind=infra" >> "$GITHUB_OUTPUT" exit 1 fi @@ -2834,6 +2870,10 @@ jobs: HEAD_SHA: '${{ steps.prepare.outputs.head_sha }}' HEAD_REPO: '${{ steps.prepare.outputs.head_repo }}' OUTCOME: '${{ steps.verify.outputs.outcome }}' + FAILURE_KIND: '${{ steps.verify.outputs.failure_kind }}' + RESOLVE_OUTCOME: '${{ steps.resolve_conflicts.outcome }}' + # Mirrors timeout-minutes on 'Resolve conflicts'; a test pins the pair. + AGENT_TIMEOUT_MINUTES: '75' DRY_RUN: '${{ env.DRY_RUN }}' RUN_URL: '${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}' run: |- @@ -2957,16 +2997,26 @@ jobs: append_safe_file "${WORKDIR}/no-action.md" ;; *) - echo "Qwen Code attempted to resolve merge conflicts but the run did not complete successfully." - echo - for file in failure.md address-summary.md no-action.md; do - if [ -s "${WORKDIR}/${file}" ]; then - echo "### ${file}" - append_safe_file "${WORKDIR}/${file}" - echo - fi - done - echo "Check the [workflow run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}) for full logs." + if [ "$FAILURE_KIND" = "infra" ]; then + # The agent never reached a verdict: outcome=failure (CLI + # install, model endpoint, crash) or outcome=cancelled (its + # step timeout). Say which, and do not invite a re-run: the + # generic text below reads as "the model gave up", and 13 + # days of re-runs against one broken install (#9898) is what + # this branch exists to stop. + echo "Qwen Code could not run conflict resolution on this PR: the agent step ended with \`outcome=${RESOLVE_OUTCOME}\` before producing a result — a CLI install, model endpoint or infrastructure failure, or the step's ${AGENT_TIMEOUT_MINUTES}-minute timeout. This is not a verdict on the conflict. Requesting /resolve again will fail the same way until the cause shown in the [workflow run](${RUN_URL}) is fixed." + else + echo "Qwen Code attempted to resolve merge conflicts but the run did not complete successfully." + echo + for file in failure.md address-summary.md no-action.md; do + if [ -s "${WORKDIR}/${file}" ]; then + echo "### ${file}" + append_safe_file "${WORKDIR}/${file}" + echo + fi + done + echo "Check the [workflow run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}) for full logs." + fi ;; esac } > "${WORKDIR}/report.md" diff --git a/scripts/tests/qwen-resolve-workflow.test.js b/scripts/tests/qwen-resolve-workflow.test.js index b4c541bc283..5f0d31ab4fa 100644 --- a/scripts/tests/qwen-resolve-workflow.test.js +++ b/scripts/tests/qwen-resolve-workflow.test.js @@ -787,8 +787,14 @@ describe('qwen resolve workflow', () => { expect(resolveJob).toContain('persist-credentials: false'); // The resolution check carries no writable GitHub token (defense in depth). expect(resolveJob).toContain("GITHUB_TOKEN: ''"); - // The agent runs sandboxed. - expect(resolveJob).toContain('"sandbox": true'); + // The agent runs WITHOUT the container sandbox — measured, not assumed: + // the first day `sandbox: true` took effect (#9252, 2026-08-16) /resolve + // went from 84% pushed to 0 of 81, dying on a missing versioned image or + // hanging to the job timeout. Containment is the no-token agent, the scope + // guard and the ephemeral runner. Flipping this back needs a dry-run that + // shows the agent finishing inside the container. + expect(resolveJob).toContain('"sandbox": false'); + expect(resolveJob).not.toContain('"sandbox": true'); // Concurrent /resolve runs must not interleave on the credentialed push. expect(resolveJob).toContain('cancel-in-progress: false'); }); @@ -805,6 +811,56 @@ describe('qwen resolve workflow', () => { expect(agentStep).not.toContain('CI_DEV_BOT_PAT'); }); + it('pins the CLI version and bounds the agent step', () => { + const agentStep = resolveJob.slice( + resolveJob.indexOf("- name: 'Resolve conflicts'"), + resolveJob.indexOf("- name: 'Resolution check'"), + ); + // `latest` ties every run to the npm release pipeline of the moment: the + // 2026-08-15 dist-tag pointed at an unresolvable 0.21.12 and 14 runs died + // on `npm error notarget` before the agent started. + expect(agentStep).toMatch(/qwen_cli_version: '\d+\.\d+\.\d+'/); + expect(agentStep).not.toContain("qwen_cli_version: 'latest'"); + // A hung agent must not bill the whole 120-minute job; the step timeout + // and the number quoted in the failure comment must agree. + const stepTimeout = agentStep.match(/^\s+timeout-minutes: (\d+)$/m); + expect(stepTimeout).not.toBeNull(); + const reportStep = resolveJob.slice( + resolveJob.indexOf("- name: 'Report result'"), + ); + expect(reportStep).toContain(`AGENT_TIMEOUT_MINUTES: '${stepTimeout[1]}'`); + expect(Number(stepTimeout[1])).toBeLessThan(120); + }); + + it('reports an agent that never ran as an infrastructure failure, not a verdict', () => { + // outcome != success on the agent step means no resolution was attempted + // (install/model/infra error, step timeout, cancellation). The comment + // must say so and must not invite a re-run, which repeats the failure. + expect(resolveJob).toContain( + 'echo "failure_kind=infra" >> "$GITHUB_OUTPUT"', + ); + expect(resolveJob).toContain( + "FAILURE_KIND: '${{ steps.verify.outputs.failure_kind }}'", + ); + expect(resolveJob).toContain( + "RESOLVE_OUTCOME: '${{ steps.resolve_conflicts.outcome }}'", + ); + const infraIdx = resolveJob.indexOf( + 'Qwen Code could not run conflict resolution on this PR', + ); + expect(infraIdx).toBeGreaterThan(-1); + const infraLine = resolveJob.slice( + infraIdx, + resolveJob.indexOf('\n', infraIdx), + ); + expect(infraLine).toContain('This is not a verdict on the conflict'); + expect(infraLine).not.toContain('Re-run /resolve'); + // The generic wording survives for the cases where the agent did run. + expect(resolveJob).toContain( + 'Qwen Code attempted to resolve merge conflicts but the run did not complete successfully.', + ); + }); + it('supports dry-run and workflow_dispatch', () => { expect(workflow).toContain('github.event.inputs.dry_run'); expect(workflow).toContain('in dry-run mode'); From a2aba41d0397d81865cf808b21d5d41c8bc57b84 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 29 Aug 2026 07:06:29 +0800 Subject: [PATCH 02/12] ci: record the resolve-pr growth in .size-baseline and drop two stale sandbox mentions The settings-block rationale for tools.sandbox=false and the version pin grow qwen-code-pr-review.yml by ~7.5 KB, past the 4096-byte ratchet; the growth is the incident record itself, so bump the baseline. Two comments on the scope guard still named the sandbox as part of the containment story; align them with the settings block (write authorization + no agent token + ephemeral runner). --- .github/workflows/.size-baseline | 2 +- .github/workflows/qwen-code-pr-review.yml | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/.github/workflows/.size-baseline b/.github/workflows/.size-baseline index 2d9baebaaf8..3ac81a49cc1 100644 --- a/.github/workflows/.size-baseline +++ b/.github/workflows/.size-baseline @@ -36,7 +36,7 @@ 5942 qwen-autofix-fork-signal.yml 426924 qwen-autofix.yml 7061 qwen-ci-flaky-rerun.yml -165876 qwen-code-pr-review.yml +173427 qwen-code-pr-review.yml 79041 qwen-fleet-shepherd.yml 20525 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 1b4f7882071..f42a25a9fc7 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -2791,13 +2791,14 @@ jobs: # itself changed (conflict resolutions live in those same files). # Anything edited outside that set is out of scope — a prompt-injection # symptom — so fail closed and list the offending files. Granularity is - # deliberately file-level, not per-hunk: real containment is sandbox + - # write authorization + no agent token; this is one defense-in-depth layer. + # deliberately file-level, not per-hunk: real containment is write + # authorization + no agent token + an ephemeral runner; this is one + # defense-in-depth layer. merge_base="$(git merge-base "origin/${BASE_REF}" "$HEAD_FETCH_REF")" # -z/sort -zu mirrors the conflict-marker check above so unusual # filenames in the diff are handled consistently. ponytail: tr back to # newlines because bash vars can't hold NUL — a filename containing a - # literal newline still splits, but that's covered by the sandbox + + # literal newline still splits, but that's covered by the # write-authorization + no-token containment this guard backstops. base_changed="$(git diff --name-only -z "${merge_base}" "origin/${BASE_REF}" | sort -zu | tr '\0' '\n')" agent_changed="$(git diff --name-only -z "$HEAD_FETCH_REF" HEAD | sort -zu | tr '\0' '\n')" From 1896f37a38fe88231ea7b97de94597512f4b9a1e Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Sat, 29 Aug 2026 01:59:10 +0000 Subject: [PATCH 03/12] fix(ci): scrub agent-planted runner state before the resolve-pr credentialed push MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The no-sandbox /resolve agent runs --yolo before 'Resolution check' and 'Report result' in the same job: anything it appends to $GITHUB_ENV reaches every later step, and it writes the workspace. A prompt-injected agent could therefore plant BASH_ENV/LD_* hijacks, PATH shims, git-config url rewrites or a pre-push hook that the workflow-scope push token then walks through (measured by review probes; the pre-fix shape fires all three channels). Harden both post-agent steps: pin BASH_ENV/LD_* empty at the step boundary (they execute before any body line runs), pin PATH, drop git's env channels, ignore host/global config, remove the agent-writable local .git/config, and run the push hookless (--no-verify, core.hooksPath=/dev/null). Pin the follow-up bot's CLI version too — it was the one sibling still following npm `latest` through the 2026-08-15 notarget incident class. Tests: witness assertions for every hardening layer (each one red when its layer is removed), arm-slice pins for the infra/generic report bodies and the failure_kind=infra write, and the step-vs-job timeout pair now reads the job ceiling instead of duplicating it. Bump the size baseline for the deliberate workflow growth. --- .github/scripts/qwen-triage-workflow.test.mjs | 14 ++ .github/workflows/.size-baseline | 2 +- .github/workflows/qwen-code-pr-review.yml | 69 +++++++++- .github/workflows/qwen-issue-followup-bot.yml | 6 + scripts/tests/qwen-resolve-workflow.test.js | 127 +++++++++++++++--- 5 files changed, 197 insertions(+), 21 deletions(-) diff --git a/.github/scripts/qwen-triage-workflow.test.mjs b/.github/scripts/qwen-triage-workflow.test.mjs index 820a3ecc00c..62404780c2b 100644 --- a/.github/scripts/qwen-triage-workflow.test.mjs +++ b/.github/scripts/qwen-triage-workflow.test.mjs @@ -347,6 +347,20 @@ describe('qwen-issue-followup-bot.yml: agent settings', () => { 'tools.sandbox must stay false — the ECS pool has no container runtime', ); }); + + it('pins the CLI version instead of following `latest`', () => { + // The action installs the CLI from npm at run time; `latest` couples every + // scheduled run to the release pipeline of the moment — on 2026-08-15 the + // dist-tag pointed at an unresolvable 0.21.12 and every consumer following + // it died on `npm error notarget`. resolve-pr pins for the same reason; + // this must not be the one sibling that keeps following the tag. + const version = followupStep.with?.qwen_cli_version; + assert.match( + String(version), + /^\d+\.\d+\.\d+$/, + 'qwen_cli_version must be an exact semver, not a dist-tag', + ); + }); }); describe('qwen-triage: fork-PR runner routing', () => { diff --git a/.github/workflows/.size-baseline b/.github/workflows/.size-baseline index 3ac81a49cc1..83e1d9b96f7 100644 --- a/.github/workflows/.size-baseline +++ b/.github/workflows/.size-baseline @@ -36,7 +36,7 @@ 5942 qwen-autofix-fork-signal.yml 426924 qwen-autofix.yml 7061 qwen-ci-flaky-rerun.yml -173427 qwen-code-pr-review.yml +177091 qwen-code-pr-review.yml 79041 qwen-fleet-shepherd.yml 20525 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 f42a25a9fc7..97f84b73ea6 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -2711,12 +2711,41 @@ jobs: # value keeps a writable GITHUB_TOKEN out of the workspace as defense in # depth, mirroring the no-token contract of the agent step above. GITHUB_TOKEN: '' + # The agent step above runs --yolo without a sandbox and can append + # arbitrary KEY=VALUE pairs to $GITHUB_ENV; the runner exports them to + # every later step. Pin the execution-hijack ones to empty HERE: bash + # sources BASH_ENV and the loader honours LD_* at process startup, + # before any line of the run body can unset them. + BASH_ENV: '' + LD_PRELOAD: '' + LD_AUDIT: '' + LD_LIBRARY_PATH: '' BASE_REF: '${{ steps.prepare.outputs.base_ref }}' HEAD_FETCH_REF: '${{ steps.prepare.outputs.head_fetch_ref }}' RESOLVE_OUTCOME: '${{ steps.resolve_conflicts.outcome }}' run: |- set -euo pipefail + # SECURITY: this step runs AFTER the prompt-injectable agent step, on a + # workspace that step wrote — it must not trust inherited runner state. + # A planted PATH shim could falsify every check below, and git config + # planted on host, global or LOCAL scope reaches the credentialed push + # in 'Report result' (a url rewrite redirects the token URL, hooks run + # with it in argv). Pin PATH, ignore host/global config, and remove the + # local .git/config — the agent writes the workspace and that file is + # the highest-precedence scope, so GIT_CONFIG_GLOBAL=/dev/null alone + # does not cover it. Every git call below uses explicit refs, so + # nothing legitimate is lost. Full rationale → qwen-autofix.md#af-014. + PATH=/usr/bin:/bin + unset GIT_CONFIG_PARAMETERS GIT_ALLOW_PROTOCOL GIT_PROXY_COMMAND \ + GIT_SSL_NO_VERIFY GIT_SSL_CAINFO GIT_EXEC_PATH GIT_DIR GIT_WORK_TREE \ + GIT_COMMON_DIR GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES \ + GIT_SHALLOW_FILE GIT_ASKPASS GIT_SSH GIT_SSH_COMMAND + export GIT_CONFIG_COUNT=0 + export GIT_CONFIG_SYSTEM=/dev/null + export GIT_CONFIG_GLOBAL=/dev/null + rm -f .git/config + # The agent step runs under always(); if it failed at the infrastructure # level (API timeout, model quota, action crash) the branch is unmodified # and the merge-tree check below would misreport "still has conflicts". @@ -2864,6 +2893,12 @@ jobs: - name: 'Report result' if: "${{ always() && steps.prepare.outputs.decision == 'run' }}" env: + # The BASH_ENV/LD_* pins mirror 'Resolution check': the agent step can + # plant them through $GITHUB_ENV, and they execute at process startup. + BASH_ENV: '' + LD_PRELOAD: '' + LD_AUDIT: '' + LD_LIBRARY_PATH: '' GH_TOKEN: '${{ secrets.CI_DEV_BOT_PAT }}' PUSH_TOKEN: '${{ secrets.CI_DEV_BOT_PAT }}' PR_NUMBER: '${{ steps.resolve.outputs.pr_number }}' @@ -2879,6 +2914,28 @@ jobs: RUN_URL: '${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}' run: |- set -euo pipefail + + # SECURITY: this step runs AFTER the prompt-injectable agent step and + # carries the push token — it must not execute or route through + # anything that step can plant. The BASH_ENV/LD_* startup channels are + # pinned empty in the step env above; here: resolve binaries through a + # pinned PATH, drop git's env channels and every config scope (a url + # rewrite redirects the token URL, hooks run with it in argv), and the + # proxy/gh vars that could route the push or the PAT comment to a + # listener. Full rationale → qwen-autofix.md#af-014. + PATH=/usr/bin:/bin + unset GIT_CONFIG_PARAMETERS GIT_ALLOW_PROTOCOL GIT_PROXY_COMMAND \ + GIT_SSL_NO_VERIFY GIT_SSL_CAINFO GIT_EXEC_PATH GIT_DIR GIT_WORK_TREE \ + GIT_COMMON_DIR GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES \ + GIT_SHALLOW_FILE GIT_ASKPASS GIT_SSH GIT_SSH_COMMAND \ + HTTP_PROXY HTTPS_PROXY http_proxy https_proxy ALL_PROXY NO_PROXY \ + GH_HOST GH_ENTERPRISE_TOKEN GH_CONFIG_DIR + export GIT_CONFIG_COUNT=0 + export GIT_TERMINAL_PROMPT=0 + export GIT_CONFIG_SYSTEM=/dev/null + export GIT_CONFIG_GLOBAL=/dev/null + rm -f .git/config + push_failed=false # Budget for one agent-written section of the comment. The old 2000 @@ -2925,11 +2982,15 @@ jobs: # 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 sandboxed 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. + # 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 must also not + # inherit state the agent can plant: the scrub at the top of this step + # pins PATH and removes every git config scope, and git runs hookless + # below, so a prompt-injected agent cannot redirect or hook the step + # that carries the token. push_log="${WORKDIR}/push.log" - if git push \ + if git -c core.hooksPath=/dev/null 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 diff --git a/.github/workflows/qwen-issue-followup-bot.yml b/.github/workflows/qwen-issue-followup-bot.yml index 9235346872e..166e01d2658 100644 --- a/.github/workflows/qwen-issue-followup-bot.yml +++ b/.github/workflows/qwen-issue-followup-bot.yml @@ -297,6 +297,12 @@ jobs: OPENAI_API_KEY: '${{ secrets.OPENAI_API_KEY }}' OPENAI_BASE_URL: '${{ secrets.OPENAI_BASE_URL }}' OPENAI_MODEL: '${{ vars.QWEN_PR_REVIEW_MODEL }}' + # Pinned, not `latest`: the action installs the CLI from npm at run + # time, and every consumer following the dist-tag died on + # `npm error notarget` when it moved to an unresolvable version on + # 2026-08-15. resolve-pr pins the same version for the same reason; + # an org-level QWEN_CLI_VERSION variable would still override this. + qwen_cli_version: '0.21.10' # The input name is `settings` — this action version has no # `settings_json` input, and an unknown input is silently dropped, # which is what happened to this block until the rename. diff --git a/scripts/tests/qwen-resolve-workflow.test.js b/scripts/tests/qwen-resolve-workflow.test.js index 5f0d31ab4fa..69fd15284a8 100644 --- a/scripts/tests/qwen-resolve-workflow.test.js +++ b/scripts/tests/qwen-resolve-workflow.test.js @@ -811,6 +811,48 @@ describe('qwen resolve workflow', () => { expect(agentStep).not.toContain('CI_DEV_BOT_PAT'); }); + it('hardens the post-agent steps against state the agent can plant', () => { + // The agent step runs --yolo without a sandbox BEFORE these steps in the + // same job: anything it appends to $GITHUB_ENV reaches every later step, + // and it writes the workspace. 'Resolution check' must not be falsifiable + // by planted shims or config, and the credentialed push in 'Report result' + // must not inherit an executable or routable channel to the token. + const verifyStep = step(resolveJob, 'Resolution check'); + const reportStep = step(resolveJob, 'Report result'); + + for (const postAgentStep of [verifyStep, reportStep]) { + // BASH_ENV and LD_* execute at process startup, before any body line + // runs — pin them empty at the step boundary, not merely unset in the + // body. + expect(postAgentStep).toContain("BASH_ENV: ''"); + expect(postAgentStep).toContain("LD_PRELOAD: ''"); + expect(postAgentStep).toContain("LD_AUDIT: ''"); + expect(postAgentStep).toContain("LD_LIBRARY_PATH: ''"); + // A planted PATH shim must not resolve git for the guard or the push. + expect(postAgentStep).toContain('PATH=/usr/bin:/bin'); + // Host and global config are agent-reachable runner surfaces (url + // rewrites redirect the token URL); GIT_CONFIG_COUNT injects config + // through env. None of it may reach the git calls. + expect(postAgentStep).toContain('export GIT_CONFIG_SYSTEM=/dev/null'); + expect(postAgentStep).toContain('export GIT_CONFIG_GLOBAL=/dev/null'); + expect(postAgentStep).toContain('export GIT_CONFIG_COUNT=0'); + // The LOCAL .git/config is the highest-precedence scope and the agent + // writes the workspace — hooks, filters, includes and url rewrites + // planted there survive GIT_CONFIG_GLOBAL=/dev/null, so it is removed. + expect(postAgentStep).toContain('rm -f .git/config'); + } + + // The credentialed push itself: hookless and verify-less, so a planted + // pre-push hook never receives the token URL. + expect(reportStep).toContain( + 'git -c core.hooksPath=/dev/null push --no-verify', + ); + // Proxy and gh-host vars could route the push or the PAT comment to a + // listener; they must be dropped from the inherited environment. + expect(reportStep).toContain('HTTP_PROXY HTTPS_PROXY'); + expect(reportStep).toContain('GH_HOST GH_ENTERPRISE_TOKEN GH_CONFIG_DIR'); + }); + it('pins the CLI version and bounds the agent step', () => { const agentStep = resolveJob.slice( resolveJob.indexOf("- name: 'Resolve conflicts'"), @@ -829,36 +871,89 @@ describe('qwen resolve workflow', () => { resolveJob.indexOf("- name: 'Report result'"), ); expect(reportStep).toContain(`AGENT_TIMEOUT_MINUTES: '${stepTimeout[1]}'`); - expect(Number(stepTimeout[1])).toBeLessThan(120); + // Read the ceiling instead of duplicating it as a literal: if the job's + // timeout-minutes drops to or below the step's, the job-level timeout + // cancels the whole job on a hung agent — the always()-gated 'Resolution + // check' and 'Report result' never start, so the failure posts neither + // failure_kind=infra nor a "not a verdict" comment. + const jobTimeout = resolveJob.match(/^ {4}timeout-minutes: (\d+)$/m); + expect(jobTimeout).not.toBeNull(); + expect(Number(stepTimeout[1])).toBeLessThan(Number(jobTimeout[1])); }); it('reports an agent that never ran as an infrastructure failure, not a verdict', () => { // outcome != success on the agent step means no resolution was attempted // (install/model/infra error, step timeout, cancellation). The comment // must say so and must not invite a re-run, which repeats the failure. - expect(resolveJob).toContain( - 'echo "failure_kind=infra" >> "$GITHUB_OUTPUT"', - ); - expect(resolveJob).toContain( + const verifyStep = step(resolveJob, 'Resolution check'); + const reportStep = step(resolveJob, 'Report result'); + expect(reportStep).toContain( "FAILURE_KIND: '${{ steps.verify.outputs.failure_kind }}'", ); - expect(resolveJob).toContain( + expect(reportStep).toContain( "RESOLVE_OUTCOME: '${{ steps.resolve_conflicts.outcome }}'", ); - const infraIdx = resolveJob.indexOf( - 'Qwen Code could not run conflict resolution on this PR', + + // The failure_kind=infra write is pinned to the never-ran block of + // 'Resolution check' — moving it into the failure.md branch is the exact + // regression this branch exists to stop (an infrastructure failure would + // then post the generic verdict wording and invite re-runs). + const neverRanStart = verifyStep.indexOf( + 'if [ "$RESOLVE_OUTCOME" != "success" ]; then', ); - expect(infraIdx).toBeGreaterThan(-1); - const infraLine = resolveJob.slice( - infraIdx, - resolveJob.indexOf('\n', infraIdx), + expect(neverRanStart).toBeGreaterThan(-1); + const neverRanArm = verifyStep.slice( + neverRanStart, + verifyStep.indexOf('exit 1', neverRanStart), ); - expect(infraLine).toContain('This is not a verdict on the conflict'); - expect(infraLine).not.toContain('Re-run /resolve'); - // The generic wording survives for the cases where the agent did run. - expect(resolveJob).toContain( + expect(neverRanArm).toContain( + 'echo "failure_kind=infra" >> "$GITHUB_OUTPUT"', + ); + const failureMdStart = verifyStep.indexOf( + 'if [ -s "${WORKDIR}/failure.md" ]; then', + ); + expect(failureMdStart).toBeGreaterThan(-1); + const failureMdArm = verifyStep.slice( + failureMdStart, + verifyStep.indexOf('exit 1', failureMdStart), + ); + expect(failureMdArm).not.toContain('failure_kind=infra'); + + // Pin the branch CONDITION literal, then slice the two arms of the `*)` + // case so a transposition of the bodies fails — unordered toContain kept + // both texts present in either arm. + const infraCond = 'if [ "$FAILURE_KIND" = "infra" ]; then'; + const infraStart = reportStep.indexOf(infraCond); + expect(infraStart).toBeGreaterThan(-1); + const infraArm = reportStep.slice( + infraStart, + reportStep.indexOf('else', infraStart), + ); + expect(infraArm).toContain( + 'Qwen Code could not run conflict resolution on this PR', + ); + expect(infraArm).toContain('This is not a verdict on the conflict'); + // The timeout rides as the env interpolation: hardcoding the number in + // the message lets the pair drift when the step timeout changes. + expect(infraArm).toContain('${AGENT_TIMEOUT_MINUTES}-minute timeout'); + expect(infraArm).not.toContain('Re-run /resolve'); + expect(infraArm).not.toContain( + 'Qwen Code attempted to resolve merge conflicts', + ); + const elseStart = reportStep.indexOf('else', infraStart); + expect(elseStart).toBeGreaterThan(-1); + // Line-anchored end on the `*)` case terminator: the else arm nests its + // own if/fi (the file-append loop), so a bare fi search truncates early. + const elseEnd = reportStep.slice(elseStart).search(/\n\s*;;/); + expect(elseEnd).toBeGreaterThan(-1); + const elseArm = reportStep.slice(elseStart, elseStart + elseEnd); + // Symmetric pin: the generic wording lives in the else arm only. + expect(elseArm).toContain( 'Qwen Code attempted to resolve merge conflicts but the run did not complete successfully.', ); + expect(elseArm).toContain('Check the [workflow run]('); + expect(elseArm).not.toContain('could not run conflict resolution'); + expect(elseArm).not.toContain('This is not a verdict'); }); it('supports dry-run and workflow_dispatch', () => { From f745f9db7126f9be3c3b9b8ff4228ab8420c4746 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 29 Aug 2026 11:11:00 +0800 Subject: [PATCH 04/12] fix(ci): invoke the /resolve agent directly with decoyed runner files, on top of the post-agent scrub MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Complements the scrub that 'Resolution check' and 'Report result' now open with (PATH pinned, git env channels dropped, every config scope ignored, .git/config removed, BASH_ENV/LD_* pinned empty): those close what the later steps can see, but a $GITHUB_ENV append made by the agent is applied by the runner to every later step's environment, where an unset list can only chase known names. So the agent is now invoked directly — the pinned CLI installed in its own step — with its invocation's $GITHUB_ENV, $GITHUB_PATH, $GITHUB_OUTPUT and $GITHUB_STEP_SUMMARY pointed at decoys that die with the step, inside a ::stop-commands:: window with a random resume token, the way the review lane runs its agent. Settings go to a per-run QWEN_HOME instead of the action input. On the scrubbed side: 'Show run artifacts' gets the same scrub, every post-agent git call goes through a wrapper that disables hooks, the fsmonitor command and credential helpers, and 'Report result' gives gh a fresh empty config dir instead of leaving ~/.config/gh in reach. The settings-block comment records the residual: a root-capable agent on a hosted runner can defeat any in-job measure; the structural answer is a downstream job for the credentialed push, tracked separately. Tests pin the direct invocation, the decoys, the stop-commands window, the scrub and wrapper on every post-agent step, the BASH_ENV/LD_* pins, the gh config dir and --no-verify; the size baseline moves with the growth. --- .github/scripts/qwen-triage-workflow.test.mjs | 123 +++++++++++++--- .github/workflows/.size-baseline | 2 +- .github/workflows/qwen-code-pr-review.yml | 137 ++++++++++++++---- scripts/tests/qwen-resolve-workflow.test.js | 13 +- 4 files changed, 223 insertions(+), 52 deletions(-) diff --git a/.github/scripts/qwen-triage-workflow.test.mjs b/.github/scripts/qwen-triage-workflow.test.mjs index 62404780c2b..1cdae454761 100644 --- a/.github/scripts/qwen-triage-workflow.test.mjs +++ b/.github/scripts/qwen-triage-workflow.test.mjs @@ -251,15 +251,40 @@ describe('qwen-triage: agent tool/permission settings', () => { // release (#9898) so the agent died at startup, or it hung silently to the job // timeout. No sandboxed /resolve run has ever pushed a resolution. describe('qwen-code-pr-review.yml resolve-pr: agent settings', () => { - it('passes `settings:` (not the silently-dropped `settings_json:`)', () => { - assertSettingsContract(resolveConflictsStep, 'resolve_conflicts'); + // The agent is a direct `qwen` invocation (no qwen-code-action), so the + // settings travel as a step env value that the run block writes to a + // per-run QWEN_HOME — nothing can be silently dropped by an input name. + const resolveSettings = (label) => { + assert.ok(resolveConflictsStep, `${label} must keep its agent step`); + assert.equal( + resolveConflictsStep.uses, + undefined, + `${label} must invoke qwen directly, not through an action (its $GITHUB_ENV/$GITHUB_PATH cannot be decoyed)`, + ); + const raw = resolveConflictsStep.env?.QWEN_SETTINGS; + assert.ok(typeof raw === 'string', `${label} must carry QWEN_SETTINGS`); + assert.ok( + resolveConflictsStep.run.includes('> "$QWEN_HOME/settings.json"'), + `${label} must write QWEN_SETTINGS to the per-run QWEN_HOME`, + ); + const settings = JSON.parse(raw); + for (const key of ['coreTools', 'maxSessionTurns', 'sandbox']) { + assert.equal( + settings[key], + undefined, + `${label}: v1 top-level \`${key}\` is a legacy key — use the v2 shape`, + ); + } + return settings; + }; + + it('writes its settings to a per-run QWEN_HOME instead of an action input', () => { + resolveSettings('resolve_conflicts'); + assert.equal(resolveConflictsStep.with, undefined); }); it('settings is valid JSON pinning the turn cap, allowlist, and sandbox', () => { - const settings = assertSettingsContract( - resolveConflictsStep, - 'resolve_conflicts', - ); + const settings = resolveSettings('resolve_conflicts'); assert.equal( settings.model?.maxSessionTurns, 400, @@ -294,12 +319,78 @@ describe('qwen-code-pr-review.yml resolve-pr: agent settings', () => { // `latest` ties the job to the npm release pipeline of the moment: on // 2026-08-15 the dist-tag pointed at an unresolvable 0.21.12 and every // /resolve died on `npm error notarget` before the agent started. - const version = resolveConflictsStep.with?.qwen_cli_version; + const install = resolvePrJob.steps.find( + (s) => s.name === 'Install Qwen CLI', + ); + assert.ok(install, 'resolve-pr must install the CLI in its own step'); assert.match( - String(version), + String(install.env?.QWEN_CLI_VERSION), /^\d+\.\d+\.\d+$/, - 'qwen_cli_version must be an exact semver, not a dist-tag', + 'QWEN_CLI_VERSION must be an exact semver, not a dist-tag', ); + assert.ok( + install.run.includes('@qwen-code/qwen-code@${QWEN_CLI_VERSION}'), + 'the install must use the pinned version', + ); + }); + + it('decoys the runner command files for the agent invocation', () => { + // The agent has arbitrary shell; whatever it appends to $GITHUB_ENV or + // $GITHUB_PATH would apply to the credentialed push step of the same + // job. The invocation must see invocation-scoped decoys, inside a + // parsing-off window, with no token in its environment. + const run = resolveConflictsStep.run; + for (const file of ['GITHUB_PATH', 'GITHUB_ENV', 'GITHUB_OUTPUT']) { + assert.ok( + run.includes(`${file}="$decoy_dir/`), + `${file} must point at the decoy for the qwen invocation`, + ); + } + assert.ok(run.includes('echo "::stop-commands::${stop_token}"')); + assert.ok(run.includes("printf '\\n::%s::\\n' \"$stop_token\"")); + assert.ok(run.includes('--approval-mode yolo')); + for (const key of Object.keys(resolveConflictsStep.env)) { + assert.ok( + !/TOKEN|PAT/.test(key), + `agent env must carry no credential, found ${key}`, + ); + } + }); + + it('hardens every step that runs after the agent', () => { + const steps = resolvePrJob.steps; + const agentIdx = steps.findIndex((s) => s.id === 'resolve_conflicts'); + const later = steps.slice(agentIdx + 1).filter((s) => s.run); + assert.ok(later.length >= 3, 'expected the post-agent run steps'); + for (const s of later) { + if (s.name === 'Report skipped request') { + // Only runs when the agent did not (decision != run). + continue; + } + for (const line of [ + 'PATH=/usr/bin:/bin', + 'export GIT_CONFIG_COUNT=0', + 'export GIT_CONFIG_SYSTEM=/dev/null', + 'export GIT_CONFIG_GLOBAL=/dev/null', + 'git() { command git -c core.hooksPath=/dev/null -c core.fsmonitor= -c credential.helper= "$@"; }', + ]) { + assert.ok( + s.run.includes(line), + `post-agent step '${s.name}' must open with the hardened tool environment: missing ${line}`, + ); + } + } + // The workspace's own .git/config is the highest-precedence scope and the + // agent writes the workspace: it is removed before any check runs. + const verify = steps.find((s) => s.id === 'verify'); + assert.ok(verify.run.includes('rm -f .git/config')); + for (const key of ['BASH_ENV', 'LD_PRELOAD', 'LD_AUDIT', 'LD_LIBRARY_PATH']) { + assert.equal(verify.env[key], '', `verify must pin ${key} empty`); + } + const report = steps.find((s) => s.name === 'Report result'); + assert.ok(report.run.includes('rm -f .git/config')); + assert.ok(report.run.includes('export GH_CONFIG_DIR')); + assert.ok(report.run.includes('push --no-verify')); }); it('keeps resolve-pr on an ephemeral hosted runner', () => { @@ -347,20 +438,6 @@ describe('qwen-issue-followup-bot.yml: agent settings', () => { 'tools.sandbox must stay false — the ECS pool has no container runtime', ); }); - - it('pins the CLI version instead of following `latest`', () => { - // The action installs the CLI from npm at run time; `latest` couples every - // scheduled run to the release pipeline of the moment — on 2026-08-15 the - // dist-tag pointed at an unresolvable 0.21.12 and every consumer following - // it died on `npm error notarget`. resolve-pr pins for the same reason; - // this must not be the one sibling that keeps following the tag. - const version = followupStep.with?.qwen_cli_version; - assert.match( - String(version), - /^\d+\.\d+\.\d+$/, - 'qwen_cli_version must be an exact semver, not a dist-tag', - ); - }); }); describe('qwen-triage: fork-PR runner routing', () => { diff --git a/.github/workflows/.size-baseline b/.github/workflows/.size-baseline index 83e1d9b96f7..5a97c602a07 100644 --- a/.github/workflows/.size-baseline +++ b/.github/workflows/.size-baseline @@ -36,7 +36,7 @@ 5942 qwen-autofix-fork-signal.yml 426924 qwen-autofix.yml 7061 qwen-ci-flaky-rerun.yml -177091 qwen-code-pr-review.yml +181780 qwen-code-pr-review.yml 79041 qwen-fleet-shepherd.yml 20525 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 97f84b73ea6..aa4674c35d7 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -2591,10 +2591,31 @@ jobs: write_output decision run decision_written=1 + - name: 'Install Qwen CLI' + if: "steps.prepare.outputs.decision == 'run'" + env: + # Pinned, not `latest`. The CLI is installed from npm at run time, + # so `latest` makes every /resolve depend on the release pipeline + # of the moment: on 2026-08-15 the dist-tag moved to 0.21.12 + # before the tarball was resolvable and 14 runs in a row died on + # `npm error notarget`. 0.21.10 is the last version with a + # measured record on this job (16 of 19 requests pushed on + # 2026-08-12). Bump it on purpose, after a dry-run /resolve on the + # new version. + QWEN_CLI_VERSION: '0.21.10' + run: |- + set -euo pipefail + npm install -g --registry=https://registry.npmjs.org "@qwen-code/qwen-code@${QWEN_CLI_VERSION}" + qwen --version + + # The agent is invoked directly rather than through qwen-code-action so + # that its process environment can be controlled: the action runs the + # CLI with the runner's real $GITHUB_ENV / $GITHUB_PATH, and anything + # appended there is applied to every later step of this job — the + # credentialed push included. See the decoy note in the run block. - name: 'Resolve conflicts' if: "steps.prepare.outputs.decision == 'run'" id: 'resolve_conflicts' - uses: 'QwenLM/qwen-code-action@05f81718976f3b7da657422e6e8e4d372b8621d7' # A pushed resolution takes 6 minutes at the median, 41 at p95 and 70 # at p99 (294 runs, request to result comment); only two ever took # longer than 75. The 120-minute job timeout has therefore never ended @@ -2604,31 +2625,28 @@ jobs: # step with this value; a test pins the pair. timeout-minutes: 75 env: - PR_NUMBER: '${{ steps.resolve.outputs.pr_number }}' - BASE_REF: '${{ steps.prepare.outputs.base_ref }}' - HEAD_REF: '${{ steps.prepare.outputs.head_ref }}' - with: OPENAI_API_KEY: '${{ secrets.OPENAI_API_KEY }}' OPENAI_BASE_URL: '${{ secrets.OPENAI_BASE_URL }}' OPENAI_MODEL: '${{ vars.QWEN_PR_REVIEW_MODEL }}' - # Pinned, not `latest`. The action installs the CLI from npm at run - # time, so `latest` makes every /resolve depend on the release - # pipeline of the moment: on 2026-08-15 the dist-tag moved to - # 0.21.12 before the tarball was resolvable and 14 runs in a row - # died on `npm error notarget`. 0.21.10 is the last version with a - # measured record on this job (16 of 19 requests pushed on - # 2026-08-12). Bump it on purpose, the way `uses:` above is bumped, - # after a dry-run /resolve on the new version. - qwen_cli_version: '0.21.10' - # The input name is `settings` — this action version has no - # `settings_json` input, and an unknown input is silently dropped. - # That is exactly what happened to this block until #9252: every - # /resolve run before it ignored the block, and the agent ran without - # the turn cap or toolset allowlist. - # tools.core specifiers (e.g. `run_shell_command(git add)`) are advisory: - # the permission manager keys on the tool name and drops the parenthesised - # command. Real containment = write authorization + no agent token + - # the scope guard in 'Resolution check' + an ephemeral runner. + PR_NUMBER: '${{ steps.resolve.outputs.pr_number }}' + BASE_REF: '${{ steps.prepare.outputs.base_ref }}' + HEAD_REF: '${{ steps.prepare.outputs.head_ref }}' + # Written to a per-run QWEN_HOME by the run block below. Until + # #9252 this block was passed to the action under the wrong input + # name and silently dropped, so every /resolve ran without the turn + # cap or toolset allowlist; it now takes effect on every run. + # tools.core specifiers (e.g. `run_shell_command(git add)`) are + # advisory: the permission manager keys on the tool name and drops + # the parenthesised command, so the agent has arbitrary shell in + # this checkout. Containment therefore lives OUTSIDE the agent: no + # token in its environment, runner command files decoyed for its + # invocation, every later step scrubbed of planted runner state + # (PATH, git config scopes, hooks — see 'Resolution check'), the + # scope guard, write authorization, and an ephemeral runner. A + # root-capable agent on a hosted runner (passwordless sudo) can in + # principle defeat any in-job measure; the structural answer to + # that is moving the credentialed push into a downstream job, which + # is tracked separately. # # tools.sandbox is deliberately false. The day #9252 made this block # take effect, `sandbox: true` took effect with it, and /resolve went @@ -2641,8 +2659,8 @@ jobs: # even started). No sandboxed /resolve run has ever pushed a # resolution; every pushed one ran without the container. Turning the # sandbox back on needs the same evidence the other way: a dry-run - # with `qwen_debug` that shows the agent finishing inside it. - settings: |- + # whose logs show the agent finishing inside it. + QWEN_SETTINGS: |- { "model": { "maxSessionTurns": 400 @@ -2669,7 +2687,7 @@ jobs: "sandbox": false } } - prompt: |- + PROMPT: |- ## Role You are resolving merge conflicts for PR #${{ steps.resolve.outputs.pr_number }} in this repository. The pull request branch is already checked out. You only need git to resolve the text conflicts; dependencies are not installed. Read ${{ env.WORKDIR }}/context.md first. @@ -2698,6 +2716,54 @@ jobs: End with the project's collapsed Chinese translation: `
` / `中文说明` / the translated body / `
`. - If there is no longer a conflict, do not commit. Write `${{ env.WORKDIR }}/no-action.md` explaining what changed. - If you cannot confidently resolve the conflict, do not commit. Write `${{ env.WORKDIR }}/failure.md` with the blocker and what you learned. + run: |- + set -euo pipefail + # Per-run agent home: the settings above and nothing inherited from + # the runner user's ~/.qwen. + QWEN_HOME="${RUNNER_TEMP:?}/qwen-resolve-home" + rm -rf "$QWEN_HOME" + mkdir -p "$QWEN_HOME" + printf '%s\n' "$QWEN_SETTINGS" > "$QWEN_HOME/settings.json" + export QWEN_HOME + MODEL_ARGS=() + if [ -n "${OPENAI_MODEL:-}" ]; then + MODEL_ARGS=(--model "$OPENAI_MODEL") + fi + # The agent runs no-sandbox with arbitrary shell and, on a fork PR, + # reads attacker-authored files. The runner applies whatever a step + # appends to $GITHUB_PATH / $GITHUB_ENV to every LATER step — the + # credentialed push included — so every runner command file is + # pointed at an invocation-scoped decoy that dies with this step, + # the way 'Run review' does. Command parsing is switched off around + # the agent for the same reason: a `::set-output`-style line in its + # output must stay text. The token is random per attempt so no + # agent output can guess it and resume parsing early. The steps + # after this one scrub what a decoy cannot reach (the workspace's + # .git/config, planted PATH shims) on their side. + decoy_dir="$(mktemp -d "${RUNNER_TEMP}/qwen-resolve-decoy.XXXXXX")" + stop_token="qwen-resolve-stop-$(date +%s%N)-${RANDOM}${RANDOM}" + echo "::stop-commands::${stop_token}" + set +e + GITHUB_PATH="$decoy_dir/github-path" \ + GITHUB_ENV="$decoy_dir/github-env" \ + GITHUB_OUTPUT="$decoy_dir/github-output" \ + GITHUB_STEP_SUMMARY="$decoy_dir/github-step-summary" \ + qwen \ + --auth-type openai \ + --approval-mode yolo \ + "${MODEL_ARGS[@]}" \ + --prompt "$PROMPT" \ + --output-format stream-json \ + > "${WORKDIR}/agent.stream-json.log" 2> "${WORKDIR}/agent.stderr.log" + status=$? + # Diagnostics stay inside the parsing-off window: agent output is + # untrusted text. The full logs travel with the run artifact. + echo "qwen exited with ${status}; last stderr lines:" + tail -n 40 "${WORKDIR}/agent.stderr.log" || true + printf '\n::%s::\n' "$stop_token" + set -e + rm -rf "$decoy_dir" + exit "$status" - name: 'Resolution check' id: 'verify' @@ -2745,6 +2811,11 @@ jobs: export GIT_CONFIG_SYSTEM=/dev/null export GIT_CONFIG_GLOBAL=/dev/null rm -f .git/config + # Belt and braces for the config removal: even a git call that + # slips in before it cannot run a hook, an fsmonitor command or a + # credential helper. `command` also bypasses a shell function of + # the same name should one ever reach this process. + git() { command git -c core.hooksPath=/dev/null -c core.fsmonitor= -c credential.helper= "$@"; } # The agent step runs under always(); if it failed at the infrastructure # level (API timeout, model quota, action crash) the branch is unmodified @@ -2850,6 +2921,14 @@ jobs: env: BASE_REF: '${{ steps.prepare.outputs.base_ref }}' run: |- + # Runs after the agent: same scrub as 'Resolution check' (which has + # already removed the workspace .git/config by now). + PATH=/usr/bin:/bin + export GIT_CONFIG_COUNT=0 + export GIT_CONFIG_SYSTEM=/dev/null + export GIT_CONFIG_GLOBAL=/dev/null + git() { command git -c core.hooksPath=/dev/null -c core.fsmonitor= -c credential.helper= "$@"; } + git status --short || true git diff "origin/${BASE_REF}...HEAD" > "${WORKDIR}/pr.diff" || true for file in context.md address-summary.md no-action.md failure.md pr.diff; do @@ -2935,6 +3014,12 @@ jobs: export GIT_CONFIG_SYSTEM=/dev/null export GIT_CONFIG_GLOBAL=/dev/null rm -f .git/config + # gh reads ~/.config/gh unless told otherwise, and the agent can write + # there (http_unix_socket, a host override): give it a fresh, empty + # config dir rather than leaving the default in reach. + GH_CONFIG_DIR="$(mktemp -d "${RUNNER_TEMP:?}/qwen-resolve-gh.XXXXXX")" + export GH_CONFIG_DIR + git() { command git -c core.hooksPath=/dev/null -c core.fsmonitor= -c credential.helper= "$@"; } push_failed=false diff --git a/scripts/tests/qwen-resolve-workflow.test.js b/scripts/tests/qwen-resolve-workflow.test.js index 69fd15284a8..b2eb4837b92 100644 --- a/scripts/tests/qwen-resolve-workflow.test.js +++ b/scripts/tests/qwen-resolve-workflow.test.js @@ -861,8 +861,17 @@ describe('qwen resolve workflow', () => { // `latest` ties every run to the npm release pipeline of the moment: the // 2026-08-15 dist-tag pointed at an unresolvable 0.21.12 and 14 runs died // on `npm error notarget` before the agent started. - expect(agentStep).toMatch(/qwen_cli_version: '\d+\.\d+\.\d+'/); - expect(agentStep).not.toContain("qwen_cli_version: 'latest'"); + const installStep = step(resolveJob, 'Install Qwen CLI'); + expect(installStep).toMatch(/QWEN_CLI_VERSION: '\d+\.\d+\.\d+'/); + expect(installStep).not.toContain("QWEN_CLI_VERSION: 'latest'"); + expect(installStep).toContain('@qwen-code/qwen-code@${QWEN_CLI_VERSION}'); + // Direct invocation, not the action: the action runs the CLI with the + // runner's real $GITHUB_ENV/$GITHUB_PATH, which the scrub in the later + // steps cannot fully undo (see 'hardens the post-agent steps'). + expect(agentStep).not.toContain('qwen-code-action'); + expect(agentStep).toContain('GITHUB_ENV="$decoy_dir/github-env"'); + expect(agentStep).toContain('GITHUB_PATH="$decoy_dir/github-path"'); + expect(agentStep).toContain('::stop-commands::'); // A hung agent must not bill the whole 120-minute job; the step timeout // and the number quoted in the failure comment must agree. const stepTimeout = agentStep.match(/^\s+timeout-minutes: (\d+)$/m); From 91678f0faf231a67c4c4dfbbde7298147aaef1fa Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 29 Aug 2026 15:04:25 +0800 Subject: [PATCH 05/12] chore: drop an unrelated lint-staged reformat picked up by the merge commit --- packages/core/src/agents/team/teamHelpers.test.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/core/src/agents/team/teamHelpers.test.ts b/packages/core/src/agents/team/teamHelpers.test.ts index d26396f1d3e..64ee4680ac9 100644 --- a/packages/core/src/agents/team/teamHelpers.test.ts +++ b/packages/core/src/agents/team/teamHelpers.test.ts @@ -58,9 +58,7 @@ let rmMockOverride: // otherwise the real readFile runs. vi.mock('node:fs/promises', async (importOriginal) => { const original = await importOriginal(); - type ReadFileHook = ( - ...args: Parameters - ) => unknown; + type ReadFileHook = (...args: Parameters) => unknown; let readFileHook: ReadFileHook | undefined; return { ...original, From c5a45e55f80d7d52c7fd37ac4c367bec3fd5c651 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 29 Aug 2026 13:18:19 +0000 Subject: [PATCH 06/12] fix(ci): close the resolve-pr containment bypasses from review round 4 (#10428) Truncate the real runner command files after the agent (decoys only mask the invocation env), wrap the invocation in GNU timeout so the resume/truncation survive a hung agent, remove a fork-PR-committed workspace settings file that outranks the per-run settings, refuse a gitfile-replaced .git before the checks and the credentialed push, give 'Show run artifacts' the full scrub with step-env pins, pin absolute shells on every post-agent step, drop the dead agent env keys, and pin each of these plus the exit-code propagation, scrub ordering, and both version pins in the contract tests. --- .github/scripts/qwen-triage-workflow.test.mjs | 113 +++++++++++++++++- .github/workflows/.size-baseline | 2 +- .github/workflows/qwen-code-pr-review.yml | 112 +++++++++++++---- .github/workflows/qwen-issue-followup-bot.yml | 5 +- scripts/tests/qwen-resolve-workflow.test.js | 66 +++++++++- scripts/tests/review-artifact-upload.test.js | 8 ++ 6 files changed, 275 insertions(+), 31 deletions(-) diff --git a/.github/scripts/qwen-triage-workflow.test.mjs b/.github/scripts/qwen-triage-workflow.test.mjs index bb3a21a94ee..26e9c5c3cd3 100644 --- a/.github/scripts/qwen-triage-workflow.test.mjs +++ b/.github/scripts/qwen-triage-workflow.test.mjs @@ -267,6 +267,18 @@ describe('qwen-code-pr-review.yml resolve-pr: agent settings', () => { resolveConflictsStep.run.includes('> "$QWEN_HOME/settings.json"'), `${label} must write QWEN_SETTINGS to the per-run QWEN_HOME`, ); + assert.ok( + resolveConflictsStep.run.includes('export QWEN_HOME'), + `${label} must export QWEN_HOME so the qwen process reads the per-run settings`, + ); + // A fork PR can commit .qwen/settings.json (the workspace layer), which + // the CLI merges ABOVE the user layer written above — an attacker file + // would override tools.sandbox and maxSessionTurns. The run block must + // remove it; out-setting it from the user layer cannot work. + assert.ok( + resolveConflictsStep.run.includes('rm -f .qwen/settings.json'), + `${label} must remove a fork-PR-committed workspace settings file`, + ); const settings = JSON.parse(raw); for (const key of ['coreTools', 'maxSessionTurns', 'sandbox']) { assert.equal( @@ -340,12 +352,43 @@ describe('qwen-code-pr-review.yml resolve-pr: agent settings', () => { // job. The invocation must see invocation-scoped decoys, inside a // parsing-off window, with no token in its environment. const run = resolveConflictsStep.run; - for (const file of ['GITHUB_PATH', 'GITHUB_ENV', 'GITHUB_OUTPUT']) { + for (const file of [ + 'GITHUB_PATH', + 'GITHUB_ENV', + 'GITHUB_OUTPUT', + 'GITHUB_STEP_SUMMARY', + ]) { assert.ok( run.includes(`${file}="$decoy_dir/`), `${file} must point at the decoy for the qwen invocation`, ); } + // The decoys only mask the invocation's env: the REAL runner command + // files stay discoverable by the agent (the step's parent-shell + // /proc//environ is same-uid readable, $RUNNER_TEMP is enumerable), + // and anything planted there applies to every later step — shell lookup + // included. The step must therefore truncate all four after the agent, + // on every exit path. + const statusIdx = run.indexOf('status=$?'); + assert.ok(statusIdx > -1, 'the run block must capture the qwen exit status'); + for (const file of [ + 'GITHUB_ENV', + 'GITHUB_PATH', + 'GITHUB_OUTPUT', + 'GITHUB_STEP_SUMMARY', + ]) { + const trunc = `: > "\${${file}:?}" || true`; + const idx = run.indexOf(trunc); + assert.ok(idx > -1, `${file} must be truncated after the qwen invocation`); + assert.ok( + idx > statusIdx, + `${file} truncation must run on the exit path, after status=$?`, + ); + } + // A hung agent must end inside the script (GNU timeout), not by the + // runner killing the process tree — only then are the resume and the + // truncation above reached (mirrors 'Run review'). + assert.ok(run.includes('timeout --kill-after=10s')); assert.ok(run.includes('echo "::stop-commands::${stop_token}"')); assert.ok(run.includes("printf '\\n::%s::\\n' \"$stop_token\"")); assert.ok(run.includes('--approval-mode yolo')); @@ -355,6 +398,16 @@ describe('qwen-code-pr-review.yml resolve-pr: agent settings', () => { `agent env must carry no credential, found ${key}`, ); } + // Dead keys: the run block reads none of them (PROMPT interpolates + // pr_number directly) and the CLI consumes no such env vars — they only + // clutter the env surface this step's containment comment says to audit. + for (const key of ['PR_NUMBER', 'BASE_REF', 'HEAD_REF']) { + assert.equal( + resolveConflictsStep.env[key], + undefined, + `agent env must not carry dead key ${key} (keep the copies on 'Report result' / 'Report skipped request')`, + ); + } }); it('hardens every step that runs after the agent', () => { @@ -369,6 +422,7 @@ describe('qwen-code-pr-review.yml resolve-pr: agent settings', () => { } for (const line of [ 'PATH=/usr/bin:/bin', + 'unset GIT_CONFIG_PARAMETERS', 'export GIT_CONFIG_COUNT=0', 'export GIT_CONFIG_SYSTEM=/dev/null', 'export GIT_CONFIG_GLOBAL=/dev/null', @@ -379,16 +433,52 @@ describe('qwen-code-pr-review.yml resolve-pr: agent settings', () => { `post-agent step '${s.name}' must open with the hardened tool environment: missing ${line}`, ); } + // BASH_ENV/LD_* execute at process startup, before any body line runs — + // every post-agent run step pins them empty at the step boundary. + for (const key of [ + 'BASH_ENV', + 'LD_PRELOAD', + 'LD_AUDIT', + 'LD_LIBRARY_PATH', + ]) { + assert.equal(s.env?.[key], '', `'${s.name}' must pin ${key} empty`); + } + // A planted $GITHUB_PATH prepend hijacks the runner's shell lookup for + // every later step; pin the interpreter absolutely so no shim can stand + // in for it (the truncation in the agent step closes the same channel + // at the source). + assert.match( + String(s.shell), + /^\/bin\/bash/, + `'${s.name}' must pin an absolute shell`, + ); } // The workspace's own .git/config is the highest-precedence scope and the - // agent writes the workspace: it is removed before any check runs. + // agent writes the workspace: it is removed before any check runs. A + // gitfile-replaced .git would make the rm a no-op and let a + // url.*.insteadOf rewrite the push URL — both guarded steps refuse it. const verify = steps.find((s) => s.id === 'verify'); assert.ok(verify.run.includes('rm -f .git/config')); - for (const key of ['BASH_ENV', 'LD_PRELOAD', 'LD_AUDIT', 'LD_LIBRARY_PATH']) { - assert.equal(verify.env[key], '', `verify must pin ${key} empty`); - } + const verifyGuard = verify.run.indexOf('[ ! -d .git ]'); + assert.ok( + verifyGuard > -1, + 'verify must refuse a gitfile-replaced .git directory', + ); + assert.ok( + verifyGuard < verify.run.indexOf('git ls-files -u'), + 'the .git directory guard must run before the first git use', + ); const report = steps.find((s) => s.name === 'Report result'); assert.ok(report.run.includes('rm -f .git/config')); + const reportGuard = report.run.indexOf('[ ! -d .git ]'); + assert.ok( + reportGuard > -1, + 'report must re-assert the .git directory guard before the push', + ); + assert.ok( + reportGuard < report.run.indexOf('push --no-verify'), + 'the .git directory guard must run before the credentialed push', + ); assert.ok(report.run.includes('export GH_CONFIG_DIR')); assert.ok(report.run.includes('push --no-verify')); }); @@ -411,6 +501,19 @@ describe('qwen-issue-followup-bot.yml: agent settings', () => { assertSettingsContract(followupStep, 'the follow-up step'); }); + it('pins the CLI version instead of following `latest`', () => { + // Unknown `with:` inputs are dropped without error, and the 2026-08-15 + // notarget outage is why the pin exists: without a contract test a + // future rename silently reverts the bot to the dist-tag with nothing + // red at merge time. resolve-pr's equivalent pin has two tests; this one + // keeps the follow-up bot's covered too. + assert.match( + String(followupStep.with?.qwen_cli_version), + /^\d+\.\d+\.\d+$/, + 'the follow-up bot must pin an exact CLI version, not a dist-tag', + ); + }); + it('settings is valid JSON pinning the turn cap and gh allowlist', () => { const settings = assertSettingsContract(followupStep, 'the follow-up step'); assert.equal( diff --git a/.github/workflows/.size-baseline b/.github/workflows/.size-baseline index eae5c6e3e99..63f640ea592 100644 --- a/.github/workflows/.size-baseline +++ b/.github/workflows/.size-baseline @@ -36,7 +36,7 @@ 5942 qwen-autofix-fork-signal.yml 454547 qwen-autofix.yml 7061 qwen-ci-flaky-rerun.yml -185337 qwen-code-pr-review.yml +189756 qwen-code-pr-review.yml 79041 qwen-fleet-shepherd.yml 20525 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 7c07c982c4b..adc69e3480e 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -1447,13 +1447,20 @@ jobs: # The agent runs no-sandbox with the full job environment, and # the runner applies whatever it appends to $GITHUB_PATH / # $GITHUB_ENV to every LATER step (prepending the path file, - # shell lookup included). Point both at invocation-scoped decoys - # so appends die with PROXY_BIN no matter how the step exits — - # SIGKILL, set -e abort, chmod 444 on the real files, or an - # EXIT-trap bypass all become irrelevant because the real - # command files are never in the agent's environment. + # shell lookup included). Point all four runner command files at + # invocation-scoped decoys so appends die with PROXY_BIN no matter + # how the step exits — SIGKILL, set -e abort, chmod 444 on the + # real files, or an EXIT-trap bypass all become irrelevant because + # the real command files are never in the agent's environment, and + # a decoyed $GITHUB_STEP_SUMMARY cannot plant attacker text into + # the run page. The prefix only reaches the qwen invocation, so + # the step's own later writes still hit the real files. This + # shield is copied inline in 'Resolve conflicts': keep the decoy + # set aligned, edit both copies together. GITHUB_PATH="$PROXY_BIN/decoy.github-path" \ GITHUB_ENV="$PROXY_BIN/decoy.github-env" \ + GITHUB_OUTPUT="$PROXY_BIN/decoy.github-output" \ + GITHUB_STEP_SUMMARY="$PROXY_BIN/decoy.github-step-summary" \ timeout --kill-after=10s "${attempt_timeout}s" qwen \ --auth-type openai \ --approval-mode yolo \ @@ -2686,9 +2693,6 @@ jobs: OPENAI_API_KEY: '${{ secrets.OPENAI_API_KEY }}' OPENAI_BASE_URL: '${{ secrets.OPENAI_BASE_URL }}' OPENAI_MODEL: '${{ vars.QWEN_PR_REVIEW_MODEL }}' - PR_NUMBER: '${{ steps.resolve.outputs.pr_number }}' - BASE_REF: '${{ steps.prepare.outputs.base_ref }}' - HEAD_REF: '${{ steps.prepare.outputs.head_ref }}' # Written to a per-run QWEN_HOME by the run block below. Until # #9252 this block was passed to the action under the wrong input # name and silently dropped, so every /resolve ran without the turn @@ -2700,11 +2704,16 @@ jobs: # token in its environment, runner command files decoyed for its # invocation, every later step scrubbed of planted runner state # (PATH, git config scopes, hooks — see 'Resolution check'), the - # scope guard, write authorization, and an ephemeral runner. A - # root-capable agent on a hosted runner (passwordless sudo) can in - # principle defeat any in-job measure; the structural answer to - # that is moving the credentialed push into a downstream job, which - # is tracked separately. + # scope guard, write authorization, and an ephemeral runner. Two + # residuals stay acknowledged: a root-capable agent on a hosted + # runner (passwordless sudo) can in principle defeat any in-job + # measure, and even a same-uid process the agent leaves running (a + # detached poller) can read later steps' /proc//environ and + # /proc//cmdline for as long as the push token lives there — + # no privilege needed, and no in-job scrub reaches a live process. + # The structural answer to both is moving the credentialed push + # into a downstream job that never ran the agent, which is tracked + # separately. # # tools.sandbox is deliberately false. The day #9252 made this block # take effect, `sandbox: true` took effect with it, and /resolve went @@ -2783,6 +2792,14 @@ jobs: mkdir -p "$QWEN_HOME" printf '%s\n' "$QWEN_SETTINGS" > "$QWEN_HOME/settings.json" export QWEN_HOME + # A fork PR can commit its own .qwen/settings.json (the workspace + # layer), which the CLI merges ABOVE the user layer written above — + # an attacker-authored tools.sandbox or maxSessionTurns would + # outrank these settings. Remove the file; it is git-ignored here, + # so it can only arrive via a forced add on a fork branch. Do not + # rm -rf .qwen/: commands/, skills/ and agents/ under it can be + # legitimate tracked content. + rm -f .qwen/settings.json MODEL_ARGS=() if [ -n "${OPENAI_MODEL:-}" ]; then MODEL_ARGS=(--model "$OPENAI_MODEL") @@ -2791,22 +2808,29 @@ jobs: # reads attacker-authored files. The runner applies whatever a step # appends to $GITHUB_PATH / $GITHUB_ENV to every LATER step — the # credentialed push included — so every runner command file is - # pointed at an invocation-scoped decoy that dies with this step, - # the way 'Run review' does. Command parsing is switched off around - # the agent for the same reason: a `::set-output`-style line in its - # output must stay text. The token is random per attempt so no - # agent output can guess it and resume parsing early. The steps - # after this one scrub what a decoy cannot reach (the workspace's - # .git/config, planted PATH shims) on their side. + # pointed at an invocation-scoped decoy that dies with this step. + # This shield is copied inline in 'Run review' (run_review_once): + # keep the decoy set aligned, edit both copies together. Command + # parsing is switched off around the agent for the same reason: a + # `::set-output`-style line in its output must stay text. The token + # is random per attempt so no agent output can guess it and resume + # parsing early. The steps after this one scrub what a decoy cannot + # reach (the workspace's .git/config, planted PATH shims) on their + # side. decoy_dir="$(mktemp -d "${RUNNER_TEMP}/qwen-resolve-decoy.XXXXXX")" stop_token="qwen-resolve-stop-$(date +%s%N)-${RANDOM}${RANDOM}" echo "::stop-commands::${stop_token}" set +e + # GNU timeout (mirroring 'Run review') so a hung agent ends inside + # THIS shell: the runner-side timeout-minutes kills the whole + # process tree before the resume below and the command-file + # truncation can run. 4440s sits just under the 75-minute step + # timeout, which stays the outer backstop. GITHUB_PATH="$decoy_dir/github-path" \ GITHUB_ENV="$decoy_dir/github-env" \ GITHUB_OUTPUT="$decoy_dir/github-output" \ GITHUB_STEP_SUMMARY="$decoy_dir/github-step-summary" \ - qwen \ + timeout --kill-after=10s 4440s qwen \ --auth-type openai \ --approval-mode yolo \ "${MODEL_ARGS[@]}" \ @@ -2814,6 +2838,17 @@ jobs: --output-format stream-json \ > "${WORKDIR}/agent.stream-json.log" 2> "${WORKDIR}/agent.stderr.log" status=$? + # The decoys only mask THIS invocation's environment: the real + # runner command files stay discoverable by the agent (this step's + # parent-shell /proc//environ is same-uid readable, + # $RUNNER_TEMP is enumerable), and anything planted there applies to + # every later step — shell lookup included. This step writes nothing + # to them and downstream reads only runner-tracked values, so + # truncate all four on every exit path before the step ends. + : > "${GITHUB_ENV:?}" || true + : > "${GITHUB_PATH:?}" || true + : > "${GITHUB_OUTPUT:?}" || true + : > "${GITHUB_STEP_SUMMARY:?}" || true # Diagnostics stay inside the parsing-off window: agent output is # untrusted text. The full logs travel with the run artifact. echo "qwen exited with ${status}; last stderr lines:" @@ -2826,6 +2861,7 @@ jobs: - name: 'Resolution check' id: 'verify' if: "${{ always() && steps.prepare.outputs.decision == 'run' }}" + shell: '/bin/bash --noprofile --norc -eo pipefail {0}' # Refs are passed as env vars and only referenced as "$BASE_REF" / # "$HEAD_FETCH_REF" inside the script. They must never be inlined as # ${{ ... }}: branch names legally contain `$(...)`/backticks, so textual @@ -2868,6 +2904,18 @@ jobs: export GIT_CONFIG_COUNT=0 export GIT_CONFIG_SYSTEM=/dev/null export GIT_CONFIG_GLOBAL=/dev/null + # Fail closed if the agent replaced the .git DIRECTORY with a gitfile + # (`gitdir: `): git would then read the attacker's local + # config — the rm below exits 0 deleting nothing, and a + # url.*.insteadOf planted there rewrites the push URL in 'Report + # result' with the token in the request path. Test the directory + # itself, never `git rev-parse --git-dir`: that would run git + # against the attacker's config. + if [ ! -d .git ]; then + echo ".git is not a directory — workspace tampered after the agent step; refusing." + echo "outcome=failed" >> "$GITHUB_OUTPUT" + exit 1 + fi rm -f .git/config # Belt and braces for the config removal: even a git call that # slips in before it cannot run a hook, an fsmonitor command or a @@ -2976,12 +3024,25 @@ jobs: - name: 'Show run artifacts' if: "${{ always() && steps.prepare.outputs.decision == 'run' }}" + shell: '/bin/bash --noprofile --norc -eo pipefail {0}' env: + # The BASH_ENV/LD_* pins mirror 'Resolution check': they execute at + # process startup, before any body line can unset them. + # GIT_EXTERNAL_DIFF is exec'd by the content `git diff` below. + BASH_ENV: '' + LD_PRELOAD: '' + LD_AUDIT: '' + LD_LIBRARY_PATH: '' + GIT_EXTERNAL_DIFF: '' BASE_REF: '${{ steps.prepare.outputs.base_ref }}' run: |- # Runs after the agent: same scrub as 'Resolution check' (which has # already removed the workspace .git/config by now). PATH=/usr/bin:/bin + unset GIT_CONFIG_PARAMETERS GIT_ALLOW_PROTOCOL GIT_PROXY_COMMAND \ + GIT_SSL_NO_VERIFY GIT_SSL_CAINFO GIT_EXEC_PATH GIT_DIR GIT_WORK_TREE \ + GIT_COMMON_DIR GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES \ + GIT_SHALLOW_FILE GIT_ASKPASS GIT_SSH GIT_SSH_COMMAND export GIT_CONFIG_COUNT=0 export GIT_CONFIG_SYSTEM=/dev/null export GIT_CONFIG_GLOBAL=/dev/null @@ -3029,6 +3090,7 @@ jobs: - name: 'Report result' if: "${{ always() && steps.prepare.outputs.decision == 'run' }}" + shell: '/bin/bash --noprofile --norc -eo pipefail {0}' env: # The BASH_ENV/LD_* pins mirror 'Resolution check': the agent step can # plant them through $GITHUB_ENV, and they execute at process startup. @@ -3111,6 +3173,14 @@ jobs: push_fail_reason='' if [ "$OUTCOME" = "fixed" ] && [ "$DRY_RUN" != "true" ]; then + # Re-assert the .git directory guard (see 'Resolution check'): a + # gitfile planted after that step would let a url.*.insteadOf in + # the attacker's config rewrite this push URL and carry the token + # to the attacker host. + if [ ! -d .git ]; then + echo ".git is not a directory — workspace tampered after the agent step; refusing to push." + exit 1 + fi if [ -z "${PUSH_TOKEN}" ]; then echo "::error::CI_DEV_BOT_PAT is required to push conflict fixes." exit 1 diff --git a/.github/workflows/qwen-issue-followup-bot.yml b/.github/workflows/qwen-issue-followup-bot.yml index 166e01d2658..9043742133f 100644 --- a/.github/workflows/qwen-issue-followup-bot.yml +++ b/.github/workflows/qwen-issue-followup-bot.yml @@ -300,8 +300,9 @@ jobs: # Pinned, not `latest`: the action installs the CLI from npm at run # time, and every consumer following the dist-tag died on # `npm error notarget` when it moved to an unresolvable version on - # 2026-08-15. resolve-pr pins the same version for the same reason; - # an org-level QWEN_CLI_VERSION variable would still override this. + # 2026-08-15. resolve-pr pins the same version for the same reason. + # The action wires this input as step-level env, which masks any + # inherited org/repo QWEN_CLI_VERSION variable — the pin wins. qwen_cli_version: '0.21.10' # The input name is `settings` — this action version has no # `settings_json` input, and an unknown input is silently dropped, diff --git a/scripts/tests/qwen-resolve-workflow.test.js b/scripts/tests/qwen-resolve-workflow.test.js index b2eb4837b92..cbac841b2d1 100644 --- a/scripts/tests/qwen-resolve-workflow.test.js +++ b/scripts/tests/qwen-resolve-workflow.test.js @@ -819,8 +819,9 @@ describe('qwen resolve workflow', () => { // must not inherit an executable or routable channel to the token. const verifyStep = step(resolveJob, 'Resolution check'); const reportStep = step(resolveJob, 'Report result'); + const showStep = step(resolveJob, 'Show run artifacts'); - for (const postAgentStep of [verifyStep, reportStep]) { + for (const postAgentStep of [verifyStep, reportStep, showStep]) { // BASH_ENV and LD_* execute at process startup, before any body line // runs — pin them empty at the step boundary, not merely unset in the // body. @@ -828,19 +829,54 @@ describe('qwen resolve workflow', () => { expect(postAgentStep).toContain("LD_PRELOAD: ''"); expect(postAgentStep).toContain("LD_AUDIT: ''"); expect(postAgentStep).toContain("LD_LIBRARY_PATH: ''"); - // A planted PATH shim must not resolve git for the guard or the push. + // A planted PATH shim must not resolve git for the guard or the push, + // and a planted PATH prepend must not stand in for the step shell. expect(postAgentStep).toContain('PATH=/usr/bin:/bin'); + expect(postAgentStep).toContain( + "shell: '/bin/bash --noprofile --norc -eo pipefail {0}'", + ); // Host and global config are agent-reachable runner surfaces (url // rewrites redirect the token URL); GIT_CONFIG_COUNT injects config // through env. None of it may reach the git calls. expect(postAgentStep).toContain('export GIT_CONFIG_SYSTEM=/dev/null'); expect(postAgentStep).toContain('export GIT_CONFIG_GLOBAL=/dev/null'); expect(postAgentStep).toContain('export GIT_CONFIG_COUNT=0'); + // GIT_CONFIG_PARAMETERS injects config that survives GIT_CONFIG_COUNT=0 + // and the /dev/null scopes; GIT_EXTERNAL_DIFF and GIT_EXEC_PATH are + // exec channels a content diff can hit. All three steps must drop the + // whole list — the comment claims a 'same scrub', so pin it. + expect(postAgentStep).toContain('unset GIT_CONFIG_PARAMETERS'); + // The scrub must execute BEFORE each step's first git/gh use — + // relocating it below them re-exposes the checks and the push to + // planted state, the exact regression this PR exists to stop. + expect(postAgentStep.indexOf('PATH=/usr/bin:/bin')).toBeGreaterThan(-1); + } + expect(verifyStep.indexOf('PATH=/usr/bin:/bin')).toBeLessThan( + verifyStep.indexOf('git ls-files -u'), + ); + expect(reportStep.indexOf('PATH=/usr/bin:/bin')).toBeLessThan( + reportStep.indexOf('push --no-verify'), + ); + expect(reportStep.indexOf('PATH=/usr/bin:/bin')).toBeLessThan( + reportStep.indexOf('gh pr comment'), + ); + + for (const postAgentStep of [verifyStep, reportStep]) { // The LOCAL .git/config is the highest-precedence scope and the agent // writes the workspace — hooks, filters, includes and url rewrites // planted there survive GIT_CONFIG_GLOBAL=/dev/null, so it is removed. expect(postAgentStep).toContain('rm -f .git/config'); + // A gitfile-replaced .git (`gitdir: `) makes that rm a no-op and + // lets a url.*.insteadOf rewrite the push URL with the token in the + // request path — refuse it before any check and before the push. + expect(postAgentStep).toContain('[ ! -d .git ]'); } + expect(verifyStep.indexOf('[ ! -d .git ]')).toBeLessThan( + verifyStep.indexOf('git ls-files -u'), + ); + expect(reportStep.indexOf('[ ! -d .git ]')).toBeLessThan( + reportStep.indexOf('push --no-verify'), + ); // The credentialed push itself: hookless and verify-less, so a planted // pre-push hook never receives the token URL. @@ -872,6 +908,31 @@ describe('qwen resolve workflow', () => { expect(agentStep).toContain('GITHUB_ENV="$decoy_dir/github-env"'); expect(agentStep).toContain('GITHUB_PATH="$decoy_dir/github-path"'); expect(agentStep).toContain('::stop-commands::'); + // A hung agent must end inside the script (GNU timeout), not by the + // runner's timeout-minutes killing the process tree — only the + // script-internal timeout reaches the ::stop-commands:: resume and the + // command-file truncation (mirrors 'Run review'). The budget stays under + // the step timeout, which remains the outer backstop. + const timeoutCmd = agentStep.match(/timeout --kill-after=10s (\d+)s qwen/); + expect(timeoutCmd).not.toBeNull(); + // The failure_kind contract depends on the exit code surviving: + // swallowing it (`exit 0`, a trailing `|| true`) would let a crashed or + // turn-capped agent report success and misroute the comment. + expect(agentStep).toContain('status=$?'); + expect(agentStep).toMatch(/exit "\$status"\s*$/); + // The decoys only mask the invocation's env; the real runner command + // files stay agent-discoverable and are truncated on every exit path. + for (const file of [ + 'GITHUB_ENV', + 'GITHUB_PATH', + 'GITHUB_OUTPUT', + 'GITHUB_STEP_SUMMARY', + ]) { + expect(agentStep).toContain(`: > "\${${file}:?}" || true`); + } + expect(agentStep.indexOf('status=$?')).toBeLessThan( + agentStep.indexOf(': > "${GITHUB_ENV:?}"'), + ); // A hung agent must not bill the whole 120-minute job; the step timeout // and the number quoted in the failure comment must agree. const stepTimeout = agentStep.match(/^\s+timeout-minutes: (\d+)$/m); @@ -880,6 +941,7 @@ describe('qwen resolve workflow', () => { resolveJob.indexOf("- name: 'Report result'"), ); expect(reportStep).toContain(`AGENT_TIMEOUT_MINUTES: '${stepTimeout[1]}'`); + expect(Number(timeoutCmd[1])).toBeLessThan(Number(stepTimeout[1]) * 60); // Read the ceiling instead of duplicating it as a literal: if the job's // timeout-minutes drops to or below the step's, the job-level timeout // cancels the whole job on a hung agent — the always()-gated 'Resolution diff --git a/scripts/tests/review-artifact-upload.test.js b/scripts/tests/review-artifact-upload.test.js index 1ded268b99d..284e4f44710 100644 --- a/scripts/tests/review-artifact-upload.test.js +++ b/scripts/tests/review-artifact-upload.test.js @@ -303,6 +303,14 @@ describe('review artifact upload — naming contract', () => { expect(runStep).toBeDefined(); expect(runStep.run).toContain('GITHUB_PATH="$PROXY_BIN/decoy.github-path"'); expect(runStep.run).toContain('GITHUB_ENV="$PROXY_BIN/decoy.github-env"'); + // All four runner command files are decoyed — aligned with the copy in + // resolve-pr's 'Resolve conflicts'; edit the pins together with it. + expect(runStep.run).toContain( + 'GITHUB_OUTPUT="$PROXY_BIN/decoy.github-output"', + ); + expect(runStep.run).toContain( + 'GITHUB_STEP_SUMMARY="$PROXY_BIN/decoy.github-step-summary"', + ); expect(runStep.run).not.toContain(': > "$GITHUB_PATH"'); expect(stageStep.shell).toContain('/bin/bash'); expect(stageBlock).toContain('PATH=/usr/bin:/bin'); From af381c10ace27d2f916b925c50ff4b7fe7ca8883 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 30 Aug 2026 04:31:33 +0800 Subject: [PATCH 07/12] fix(ci): publish the /resolve resolution from a job that never ran the agent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review rounds 2-5 on this PR each demonstrated another entrance into the credentialed push from the same job the unsandboxed agent ran in: git config scopes and hooks, a gitfile-replaced .git, a moved ref, PATH shims, appends to the real $GITHUB_ENV (discoverable on disk despite the decoys), lowercase proxy variables, GIT_TRACE, a FIFO in place of a command file, BASH_FUNC_* exports, and finally a detached process reading /proc//environ of the push step — which no in-job scrub can reach. A denylist over that surface cannot be closed, so the push moves. resolve-pr now ends without credentials once the agent has run: a new 'Package resolution' step bundles the resolution (objects and one named ref, prerequisite = the head the agent resolved from) into the run artifact alongside the report files, and the job exposes decision, pr_number, the agent step's outcome and the prepare-time refs as outputs. The scrubs, the .git guards and the BASH_ENV/LD_* pins that tried to harden the old post-agent steps are gone with them. publish-resolution (new, needs resolve-pr, always() && decision == run) checks out fresh, downloads the artifact, fetches the base and PR head refs from GitHub, pins HEAD_FETCH_REF to the head the agent resolved from, `git bundle verify`s the bundle against that, imports it and requires the resolution to descend from that head — then runs the unchanged 'Resolution check' guards and 'Report result' push/comment. A missing artifact (agent job cancelled or crashed) is reported as an infrastructure failure. Both jobs hold the qwen-pr-head-write lock. Tests: the contract pins move to the publish job and now assert the structure itself — no secret in resolve-pr after the agent step, no agent in the publish job, the token-bearing step only there, matching artifact names, the verify/import/lineage order — and a fixture test drives the real 'Package resolution' and 'Resolution check' run blocks across a bare origin, an agent-side clone and a fresh publish-side clone: happy path, a bundle that does not descend from the head, a bundle with an unknown prerequisite, a no-op against a clean base and its refusal against a conflicting one, a missing artifact, and an out-of-scope file smuggled in the bundle. Dropping the lineage check, the bundle verify, the head pinning or the missing-artifact branch each reds it. --- .github/scripts/qwen-triage-workflow.test.mjs | 191 ++++--- .github/workflows/qwen-code-pr-review.yml | 399 ++++++------- scripts/tests/qwen-resolve-workflow.test.js | 524 ++++++++++++++---- 3 files changed, 759 insertions(+), 355 deletions(-) diff --git a/.github/scripts/qwen-triage-workflow.test.mjs b/.github/scripts/qwen-triage-workflow.test.mjs index 93b12505cef..ea5d68486c5 100644 --- a/.github/scripts/qwen-triage-workflow.test.mjs +++ b/.github/scripts/qwen-triage-workflow.test.mjs @@ -410,89 +410,154 @@ describe('qwen-code-pr-review.yml resolve-pr: agent settings', () => { } }); - it('hardens every step that runs after the agent', () => { + it('keeps every credential out of the agent job once the agent has run', () => { + // Structural containment: the agent runs --yolo without a sandbox and + // can leave anything on its runner (config scopes, hooks, moved refs, + // PATH shims, the real $GITHUB_ENV, a detached process reading /proc). + // Rather than chase each entrance with a scrub, nothing credentialed + // runs on that runner after the agent: the push and the result comment + // live in `publish-resolution`, whose runner never executed the agent. const steps = resolvePrJob.steps; const agentIdx = steps.findIndex((s) => s.id === 'resolve_conflicts'); - const later = steps.slice(agentIdx + 1).filter((s) => s.run); - assert.ok(later.length >= 3, 'expected the post-agent run steps'); - for (const s of later) { + assert.ok(agentIdx > -1); + const secretRef = (v) => + typeof v === 'string' && v.includes('${{ secrets.'); + for (const s of steps.slice(agentIdx + 1)) { if (s.name === 'Report skipped request') { - // Only runs when the agent did not (decision != run). + // The one exception: it runs only when the agent did NOT run. + assert.ok( + String(s.if).includes("steps.prepare.outputs.decision == 'skip'") && + !String(s.if).includes("decision == 'run'"), + 'Report skipped request must be gated on the agent not having run', + ); continue; } - for (const line of [ - 'PATH=/usr/bin:/bin', - 'unset GIT_CONFIG_PARAMETERS', - 'export GIT_CONFIG_COUNT=0', - 'export GIT_CONFIG_SYSTEM=/dev/null', - 'export GIT_CONFIG_GLOBAL=/dev/null', - 'git() { command git -c core.hooksPath=/dev/null -c core.fsmonitor= -c credential.helper= "$@"; }', - ]) { + for (const [key, value] of Object.entries(s.env ?? {})) { assert.ok( - s.run.includes(line), - `post-agent step '${s.name}' must open with the hardened tool environment: missing ${line}`, + !secretRef(value), + `'${s.name}' runs after the agent and must not carry a secret (${key})`, ); } - // BASH_ENV/LD_* execute at process startup, before any body line runs — - // every post-agent run step pins them empty at the step boundary. - for (const key of [ - 'BASH_ENV', - 'LD_PRELOAD', - 'LD_AUDIT', - 'LD_LIBRARY_PATH', - ]) { - assert.equal(s.env?.[key], '', `'${s.name}' must pin ${key} empty`); + for (const [key, value] of Object.entries(s.with ?? {})) { + assert.ok( + !secretRef(value), + `'${s.name}' runs after the agent and must not carry a secret (${key})`, + ); } - // A planted $GITHUB_PATH prepend hijacks the runner's shell lookup for - // every later step; pin the interpreter absolutely so no shim can stand - // in for it (the truncation in the agent step closes the same channel - // at the source). - assert.match( - String(s.shell), - /^\/bin\/bash/, - `'${s.name}' must pin an absolute shell`, - ); } - // The workspace's own .git/config is the highest-precedence scope and the - // agent writes the workspace: it is removed before any check runs. A - // gitfile-replaced .git would make the rm a no-op and let a - // url.*.insteadOf rewrite the push URL — both guarded steps refuse it. - const verify = steps.find((s) => s.id === 'verify'); - assert.ok(verify.run.includes('rm -f .git/config')); - const verifyGuard = verify.run.indexOf('[ ! -d .git ]'); + // What crosses the job boundary is a bundle and report files, verified + // on the other side; the verdict on the agent step crosses as an output. + const packageStep = steps.find((s) => s.name === 'Package resolution'); + assert.ok(packageStep, 'resolve-pr must package the resolution'); assert.ok( - verifyGuard > -1, - 'verify must refuse a gitfile-replaced .git directory', + packageStep.run.includes( + 'git bundle create "${WORKDIR}/resolution.bundle" "${HEAD_SHA}..refs/heads/qwen-resolve/pr-${PR_NUMBER}"', + ), + ); + assert.equal( + resolvePrJob.outputs.agent_outcome, + '${{ steps.resolve_conflicts.outcome }}', + ); + assert.equal( + resolvePrJob.outputs.decision, + '${{ steps.prepare.outputs.decision }}', + ); + }); + + it('publishes from a job that never ran the agent, from GitHub-fetched refs and a verified bundle', () => { + const publish = prReviewDoc.jobs['publish-resolution']; + assert.ok(publish, 'publish-resolution must exist'); + assert.deepEqual(publish.needs, ['resolve-pr']); + assert.equal( + publish.if, + "${{ always() && needs.resolve-pr.outputs.decision == 'run' }}", ); + assert.equal(publish['runs-on'], 'ubuntu-latest'); + assert.deepEqual(publish.permissions, { contents: 'read' }); + // The push lock is held here too: the push is what it serialises. + assert.equal( + publish.concurrency.group, + resolvePrJob.concurrency.group, + ); + assert.equal(publish.concurrency['cancel-in-progress'], false); + // No agent here, and the token-bearing step is here and nowhere else. + assert.ok(publish.steps.every((s) => s.id !== 'resolve_conflicts')); + assert.ok(publish.steps.every((s) => !(s.run ?? '').includes('qwen '))); + const report = publish.steps.find((s) => s.name === 'Report result'); + assert.equal(report.env.PUSH_TOKEN, '${{ secrets.CI_DEV_BOT_PAT }}'); + assert.ok(report.run.includes('x-access-token:${PUSH_TOKEN}')); assert.ok( - verifyGuard < verify.run.indexOf('git ls-files -u'), - 'the .git directory guard must run before the first git use', + resolvePrJob.steps.every( + (s) => !(s.run ?? '').includes('x-access-token:'), + ), + ); + // Fresh checkout, refs from GitHub, compared against the head the agent + // resolved from; the resolution enters only as a verified bundle that + // must descend from that head. + const checkout = publish.steps.find((s) => + s.uses?.startsWith('actions/checkout@'), + ); + assert.equal(checkout.with['persist-credentials'], false); + const verify = publish.steps.find((s) => s.id === 'verify'); + assert.equal( + verify.env.HEAD_SHA, + '${{ needs.resolve-pr.outputs.head_sha }}', ); - const report = steps.find((s) => s.name === 'Report result'); - assert.ok(report.run.includes('rm -f .git/config')); - const reportGuard = report.run.indexOf('[ ! -d .git ]'); + assert.equal( + verify.env.RESOLVE_OUTCOME, + '${{ needs.resolve-pr.outputs.agent_outcome }}', + ); + for (const line of [ + 'git fetch origin "+refs/pull/${PR_NUMBER}/head:${HEAD_FETCH_REF}"', + 'git update-ref "$HEAD_FETCH_REF" "$HEAD_SHA"', + 'git bundle verify "$bundle"', + 'git merge-base --is-ancestor "$HEAD_SHA" HEAD', + ]) { + assert.ok(verify.run.includes(line), `verify must contain: ${line}`); + } assert.ok( - reportGuard > -1, - 'report must re-assert the .git directory guard before the push', + verify.run.indexOf('git bundle verify') < + verify.run.indexOf('git fetch "$bundle"'), + 'the bundle must verify before it is fetched', ); assert.ok( - reportGuard < report.run.indexOf('push --no-verify'), - 'the .git directory guard must run before the credentialed push', + verify.run.indexOf('git merge-base --is-ancestor') < + verify.run.indexOf('git ls-files -u'), + 'lineage must be checked before any content check', + ); + // The artifact is the same one the agent job uploads; its absence is an + // infrastructure failure, not a silent job failure. + const download = publish.steps.find((s) => + s.uses?.startsWith('actions/download-artifact@'), + ); + assert.equal(download['continue-on-error'], true); + assert.equal( + download.with.name, + "qwen-resolve-pr-${{ needs.resolve-pr.outputs.pr_number }}", + ); + const upload = resolvePrJob.steps.find((s) => + s.uses?.startsWith('actions/upload-artifact@'), ); - assert.ok(report.run.includes('export GH_CONFIG_DIR')); - assert.ok(report.run.includes('push --no-verify')); - }); - - it('keeps resolve-pr on an ephemeral hosted runner', () => { - // This job merges the base branch and force-pushes to the PR head; a fresh - // runner is the cheapest guarantee that nothing from an earlier attempt - // is carried into the credentialed push. (It used to be pinned for docker - // as well; the agent no longer runs sandboxed, so that half is moot.) assert.equal( - resolvePrJob['runs-on'], - 'ubuntu-latest', - 'resolve-pr must stay on an ephemeral hosted runner', + upload.with.name, + "qwen-resolve-pr-${{ steps.resolve.outputs.pr_number }}", ); + assert.ok(verify.run.includes('failure_kind=infra')); + assert.ok(verify.run.includes('uploaded no run artifact')); + }); + + it('keeps both /resolve jobs on ephemeral hosted runners', () => { + // resolve-pr runs an unsandboxed agent; publish-resolution force-pushes + // to the PR head with a PAT. A fresh runner for each is the cheapest + // guarantee that nothing from an earlier attempt — or from the other + // job — is carried into either. + for (const name of ['resolve-pr', 'publish-resolution']) { + assert.equal( + prReviewDoc.jobs[name]['runs-on'], + 'ubuntu-latest', + `${name} must stay on an ephemeral hosted runner`, + ); + } }); }); diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index adc69e3480e..336cb94b72c 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -2469,6 +2469,17 @@ jobs: # conflict-resolution path can reach a writable ambient token. permissions: contents: 'read' + # Consumed by publish-resolution. All of these are written before the + # agent step runs (or by the runner about it), never by the agent. + outputs: + decision: '${{ steps.prepare.outputs.decision }}' + pr_number: '${{ steps.resolve.outputs.pr_number }}' + agent_outcome: '${{ steps.resolve_conflicts.outcome }}' + head_ref: '${{ steps.prepare.outputs.head_ref }}' + head_sha: '${{ steps.prepare.outputs.head_sha }}' + head_repo: '${{ steps.prepare.outputs.head_repo }}' + head_fetch_ref: '${{ steps.prepare.outputs.head_fetch_ref }}' + base_ref: '${{ steps.prepare.outputs.base_ref }}' env: REPO: '${{ github.repository }}' WORKDIR: '/tmp/qwen-resolve' @@ -2676,8 +2687,8 @@ jobs: # The agent is invoked directly rather than through qwen-code-action so # that its process environment can be controlled: the action runs the # CLI with the runner's real $GITHUB_ENV / $GITHUB_PATH, and anything - # appended there is applied to every later step of this job — the - # credentialed push included. See the decoy note in the run block. + # appended there is applied to every later step of this job. See the + # decoy note in the run block. - name: 'Resolve conflicts' if: "steps.prepare.outputs.decision == 'run'" id: 'resolve_conflicts' @@ -2700,20 +2711,19 @@ jobs: # tools.core specifiers (e.g. `run_shell_command(git add)`) are # advisory: the permission manager keys on the tool name and drops # the parenthesised command, so the agent has arbitrary shell in - # this checkout. Containment therefore lives OUTSIDE the agent: no - # token in its environment, runner command files decoyed for its - # invocation, every later step scrubbed of planted runner state - # (PATH, git config scopes, hooks — see 'Resolution check'), the - # scope guard, write authorization, and an ephemeral runner. Two - # residuals stay acknowledged: a root-capable agent on a hosted - # runner (passwordless sudo) can in principle defeat any in-job - # measure, and even a same-uid process the agent leaves running (a - # detached poller) can read later steps' /proc//environ and - # /proc//cmdline for as long as the push token lives there — - # no privilege needed, and no in-job scrub reaches a live process. - # The structural answer to both is moving the credentialed push - # into a downstream job that never ran the agent, which is tracked - # separately. + # this checkout. Containment therefore lives OUTSIDE the agent, and + # structurally: no token in this job after the agent starts — the + # push and the result comment run in `publish-resolution`, on a + # runner that never executed the agent, from a fresh checkout whose + # refs come from GitHub and whose only agent-supplied input is a + # verified git bundle. Whatever the agent leaves behind here — a + # config scope, a hook, a moved ref, a PATH shim, an append to the + # real $GITHUB_ENV, a detached process reading /proc — dies with + # this runner. (An in-job denylist was tried first and could not be + # closed: each entrance named produced the next one; see the review + # threads on #10428.) The remaining in-job measures — decoyed + # command files and the parsing-off window below — only keep the + # agent's OWN job's later steps (packaging, upload) honest. # # tools.sandbox is deliberately false. The day #9252 made this block # take effect, `sandbox: true` took effect with it, and /resolve went @@ -2814,9 +2824,8 @@ jobs: # parsing is switched off around the agent for the same reason: a # `::set-output`-style line in its output must stay text. The token # is random per attempt so no agent output can guess it and resume - # parsing early. The steps after this one scrub what a decoy cannot - # reach (the workspace's .git/config, planted PATH shims) on their - # side. + # parsing early. Nothing credentialed follows in this job; the + # publish job starts from a clean runner (see the settings block). decoy_dir="$(mktemp -d "${RUNNER_TEMP}/qwen-resolve-decoy.XXXXXX")" stop_token="qwen-resolve-stop-$(date +%s%N)-${RANDOM}${RANDOM}" echo "::stop-commands::${stop_token}" @@ -2858,10 +2867,130 @@ jobs: rm -rf "$decoy_dir" exit "$status" + # The agent job ends here. Everything the publish job needs travels + # through the run artifact: the resolution as a git bundle (objects and + # one named ref — no config, no hooks, no other refs) plus the agent's + # report files. The publish job fetches the base and head refs from + # GitHub itself and verifies the bundle against them before anything + # else happens, so this runner's state stays in this job. + - name: 'Package resolution' + if: "${{ always() && steps.prepare.outputs.decision == 'run' }}" + env: + HEAD_SHA: '${{ steps.prepare.outputs.head_sha }}' + PR_NUMBER: '${{ steps.resolve.outputs.pr_number }}' + run: |- + set -uo pipefail + if [ ! -d .git ]; then + echo "no .git directory after the agent step; nothing to package" + exit 0 + fi + resolved="$(git rev-parse --verify HEAD 2>/dev/null || true)" + if [ -z "$resolved" ] || [ "$resolved" = "$HEAD_SHA" ]; then + echo "branch unchanged (${resolved:-unreadable}); no bundle to package" + exit 0 + fi + git update-ref "refs/heads/qwen-resolve/pr-${PR_NUMBER}" "$resolved" + if git bundle create "${WORKDIR}/resolution.bundle" "${HEAD_SHA}..refs/heads/qwen-resolve/pr-${PR_NUMBER}"; then + printf '%s\n' "$resolved" > "${WORKDIR}/resolution.sha" + echo "packaged ${resolved} (on ${HEAD_SHA}) as resolution.bundle" + else + echo "::warning::could not package the resolution bundle; the publish job will report the run as failed." + fi + + - name: 'Show run artifacts' + if: "${{ always() && steps.prepare.outputs.decision == 'run' }}" + env: + BASE_REF: '${{ steps.prepare.outputs.base_ref }}' + run: |- + # Diagnostics only, in the agent's own runner. Nothing printed or + # written here is trusted downstream: the publish job recomputes the + # diff and every verdict from the bundle and GitHub's refs, so a + # planted diff driver or config can only mislead this log. + git status --short || true + git diff "origin/${BASE_REF}...HEAD" > "${WORKDIR}/pr.diff" || true + for file in context.md address-summary.md no-action.md failure.md pr.diff; do + if [ -f "${WORKDIR}/${file}" ]; then + echo "=============== ${file} ===============" + cat "${WORKDIR}/${file}" + echo + fi + done + + - name: 'Upload run artifacts' + if: "${{ always() && steps.prepare.outputs.decision == 'run' }}" + uses: 'actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a' # v7.0.1 + with: + name: 'qwen-resolve-pr-${{ steps.resolve.outputs.pr_number }}' + path: '${{ env.WORKDIR }}/' + if-no-files-found: 'ignore' + + - name: 'Report skipped request' + # always(): the prepare step's EXIT trap writes decision=failed when it + # crashes, but a bare if implicitly requires success() — so without + # always() this step is skipped on the very crash it must report. + if: "${{ always() && (steps.prepare.outputs.decision == 'skip' || steps.prepare.outputs.decision == 'unsupported' || steps.prepare.outputs.decision == 'failed') }}" + env: + GH_TOKEN: '${{ secrets.CI_DEV_BOT_PAT }}' + PR_NUMBER: '${{ steps.resolve.outputs.pr_number }}' + SKIP_REASON: '${{ steps.prepare.outputs.skip_reason }}' + run: |- + set -euo pipefail + { + echo "" + echo "Qwen Code did not run conflict resolution for this request." + echo + echo "${SKIP_REASON}" + } > "${WORKDIR}/report.md" + # Best-effort, matching 'Report result': a transient API error here must + # not abort the step under set -e and swallow the only failure signal. + gh pr comment "$PR_NUMBER" --repo "$REPO" --body-file "${WORKDIR}/report.md" || + echo "::warning::Resolve was skipped, but posting the skip-reason comment failed." + + + # Publishes what the agent job produced — verification, push and the result + # comment — on a runner that never executed the agent. It starts from a + # fresh checkout, fetches the base and head refs from GitHub itself, and + # takes from the agent job only the run artifact (a git bundle and the + # report files), each verified or sanitised before use. The push token lives + # only in this job. + publish-resolution: + needs: ['resolve-pr'] + if: "${{ always() && needs.resolve-pr.outputs.decision == 'run' }}" + runs-on: 'ubuntu-latest' + timeout-minutes: 20 + # Same group as resolve-pr and qwen-autofix.yml's review-address: the push + # is what the group serialises (see the note on resolve-pr). + concurrency: + group: 'qwen-pr-head-write-${{ github.event.issue.number || github.event.inputs.pr_number }}' + cancel-in-progress: false + permissions: + contents: 'read' + env: + REPO: '${{ github.repository }}' + WORKDIR: '/tmp/qwen-resolve' + DRY_RUN: '${{ github.event.inputs.dry_run || false }}' + PR_NUMBER: '${{ needs.resolve-pr.outputs.pr_number }}' + steps: + - name: 'Checkout base branch' + uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 + with: + ref: '${{ github.event.repository.default_branch }}' + fetch-depth: 0 + persist-credentials: false + + - name: 'Download run artifacts' + # A cancelled or crashed agent job may have uploaded nothing; the + # import in 'Resolution check' reports that as an infrastructure + # failure instead of letting this step end the job without a comment. + continue-on-error: true + uses: 'actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c' # v8.0.1 + with: + name: 'qwen-resolve-pr-${{ needs.resolve-pr.outputs.pr_number }}' + path: '/tmp/qwen-resolve' + - name: 'Resolution check' id: 'verify' - if: "${{ always() && steps.prepare.outputs.decision == 'run' }}" - shell: '/bin/bash --noprofile --norc -eo pipefail {0}' + if: "${{ always() }}" # Refs are passed as env vars and only referenced as "$BASE_REF" / # "$HEAD_FETCH_REF" inside the script. They must never be inlined as # ${{ ... }}: branch names legally contain `$(...)`/backticks, so textual @@ -2869,59 +2998,34 @@ jobs: env: # No PR code runs in this step — it only runs git checks — but the empty # value keeps a writable GITHUB_TOKEN out of the workspace as defense in - # depth, mirroring the no-token contract of the agent step above. + # depth, mirroring the no-token contract of the agent step. GITHUB_TOKEN: '' - # The agent step above runs --yolo without a sandbox and can append - # arbitrary KEY=VALUE pairs to $GITHUB_ENV; the runner exports them to - # every later step. Pin the execution-hijack ones to empty HERE: bash - # sources BASH_ENV and the loader honours LD_* at process startup, - # before any line of the run body can unset them. - BASH_ENV: '' - LD_PRELOAD: '' - LD_AUDIT: '' - LD_LIBRARY_PATH: '' - BASE_REF: '${{ steps.prepare.outputs.base_ref }}' - HEAD_FETCH_REF: '${{ steps.prepare.outputs.head_fetch_ref }}' - RESOLVE_OUTCOME: '${{ steps.resolve_conflicts.outcome }}' + BASE_REF: '${{ needs.resolve-pr.outputs.base_ref }}' + HEAD_FETCH_REF: '${{ needs.resolve-pr.outputs.head_fetch_ref }}' + HEAD_SHA: '${{ needs.resolve-pr.outputs.head_sha }}' + RESOLVE_OUTCOME: '${{ needs.resolve-pr.outputs.agent_outcome }}' run: |- set -euo pipefail - # SECURITY: this step runs AFTER the prompt-injectable agent step, on a - # workspace that step wrote — it must not trust inherited runner state. - # A planted PATH shim could falsify every check below, and git config - # planted on host, global or LOCAL scope reaches the credentialed push - # in 'Report result' (a url rewrite redirects the token URL, hooks run - # with it in argv). Pin PATH, ignore host/global config, and remove the - # local .git/config — the agent writes the workspace and that file is - # the highest-precedence scope, so GIT_CONFIG_GLOBAL=/dev/null alone - # does not cover it. Every git call below uses explicit refs, so - # nothing legitimate is lost. Full rationale → qwen-autofix.md#af-014. - PATH=/usr/bin:/bin - unset GIT_CONFIG_PARAMETERS GIT_ALLOW_PROTOCOL GIT_PROXY_COMMAND \ - GIT_SSL_NO_VERIFY GIT_SSL_CAINFO GIT_EXEC_PATH GIT_DIR GIT_WORK_TREE \ - GIT_COMMON_DIR GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES \ - GIT_SHALLOW_FILE GIT_ASKPASS GIT_SSH GIT_SSH_COMMAND - export GIT_CONFIG_COUNT=0 - export GIT_CONFIG_SYSTEM=/dev/null - export GIT_CONFIG_GLOBAL=/dev/null - # Fail closed if the agent replaced the .git DIRECTORY with a gitfile - # (`gitdir: `): git would then read the attacker's local - # config — the rm below exits 0 deleting nothing, and a - # url.*.insteadOf planted there rewrites the push URL in 'Report - # result' with the token in the request path. Test the directory - # itself, never `git rev-parse --git-dir`: that would run git - # against the attacker's config. - if [ ! -d .git ]; then - echo ".git is not a directory — workspace tampered after the agent step; refusing." + # This checkout never ran the agent: the refs it checks against come + # from GitHub, and the resolution arrives only as a bundle (below). + git fetch origin "+refs/pull/${PR_NUMBER}/head:${HEAD_FETCH_REF}" "+refs/heads/${BASE_REF}:refs/remotes/origin/${BASE_REF}" + # Every check compares against the head the agent resolved FROM — + # recorded by the agent job before its agent ran — not against the + # PR's current head. A head that moved meanwhile is the push's + # business (the lease declines and the moved-head comment follows), + # not a scope-guard failure that would blame the contributor's new + # commits on the agent. If the PR head no longer points at it, the + # commit is fetched by SHA. + if ! git cat-file -e "${HEAD_SHA}^{commit}" 2>/dev/null; then + git fetch origin "$HEAD_SHA" || true + fi + if ! git cat-file -e "${HEAD_SHA}^{commit}" 2>/dev/null; then + echo "The PR head the agent resolved from (${HEAD_SHA}) is no longer reachable on GitHub; refusing." echo "outcome=failed" >> "$GITHUB_OUTPUT" exit 1 fi - rm -f .git/config - # Belt and braces for the config removal: even a git call that - # slips in before it cannot run a hook, an fsmonitor command or a - # credential helper. `command` also bypasses a shell function of - # the same name should one ever reach this process. - git() { command git -c core.hooksPath=/dev/null -c core.fsmonitor= -c credential.helper= "$@"; } + git update-ref "$HEAD_FETCH_REF" "$HEAD_SHA" # The agent step runs under always(); if it failed at the infrastructure # level (API timeout, model quota, action crash) the branch is unmodified @@ -2937,6 +3041,37 @@ jobs: exit 1 fi + # Import the resolution. The artifact is attacker-influenced (the + # agent wrote it), so it is admitted as objects only: `git bundle + # verify` proves the bundle's prerequisites exist in THIS repository + # (the head fetched from GitHub above), and the imported commit must + # descend from the head the agent resolved from. No bundle means the + # agent changed nothing — a no-op or a written-up failure — and the + # checks below run against the unchanged head. + bundle="${WORKDIR}/resolution.bundle" + resolution_ref="refs/heads/qwen-resolve/pr-${PR_NUMBER}" + if [ -s "$bundle" ]; then + if ! git bundle verify "$bundle" > /dev/null; then + echo "The resolution bundle from the agent job does not verify against the PR head; refusing." + echo "outcome=failed" >> "$GITHUB_OUTPUT" + exit 1 + fi + git fetch "$bundle" "+${resolution_ref}:${resolution_ref}" + git checkout -q -B "qwen-resolve/pr-${PR_NUMBER}" "$resolution_ref" + if ! git merge-base --is-ancestor "$HEAD_SHA" HEAD; then + echo "The resolution does not descend from the PR head the agent resolved from (${HEAD_SHA}); refusing." + echo "outcome=failed" >> "$GITHUB_OUTPUT" + exit 1 + fi + elif [ -d "$WORKDIR" ]; then + git checkout -q -B "qwen-resolve/pr-${PR_NUMBER}" "$HEAD_FETCH_REF" + else + echo "The agent job uploaded no run artifact (cancelled or crashed before packaging); nothing to verify." + echo "outcome=failed" >> "$GITHUB_OUTPUT" + echo "failure_kind=infra" >> "$GITHUB_OUTPUT" + exit 1 + fi + if [ -s "${WORKDIR}/failure.md" ]; then echo "Agent reported failure:" cat "${WORKDIR}/failure.md" @@ -3022,124 +3157,25 @@ jobs: # in scope, and committed, the resolution is publishable. echo "outcome=fixed" >> "$GITHUB_OUTPUT" - - name: 'Show run artifacts' - if: "${{ always() && steps.prepare.outputs.decision == 'run' }}" - shell: '/bin/bash --noprofile --norc -eo pipefail {0}' - env: - # The BASH_ENV/LD_* pins mirror 'Resolution check': they execute at - # process startup, before any body line can unset them. - # GIT_EXTERNAL_DIFF is exec'd by the content `git diff` below. - BASH_ENV: '' - LD_PRELOAD: '' - LD_AUDIT: '' - LD_LIBRARY_PATH: '' - GIT_EXTERNAL_DIFF: '' - BASE_REF: '${{ steps.prepare.outputs.base_ref }}' - run: |- - # Runs after the agent: same scrub as 'Resolution check' (which has - # already removed the workspace .git/config by now). - PATH=/usr/bin:/bin - unset GIT_CONFIG_PARAMETERS GIT_ALLOW_PROTOCOL GIT_PROXY_COMMAND \ - GIT_SSL_NO_VERIFY GIT_SSL_CAINFO GIT_EXEC_PATH GIT_DIR GIT_WORK_TREE \ - GIT_COMMON_DIR GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES \ - GIT_SHALLOW_FILE GIT_ASKPASS GIT_SSH GIT_SSH_COMMAND - export GIT_CONFIG_COUNT=0 - export GIT_CONFIG_SYSTEM=/dev/null - export GIT_CONFIG_GLOBAL=/dev/null - git() { command git -c core.hooksPath=/dev/null -c core.fsmonitor= -c credential.helper= "$@"; } - - git status --short || true - git diff "origin/${BASE_REF}...HEAD" > "${WORKDIR}/pr.diff" || true - for file in context.md address-summary.md no-action.md failure.md pr.diff; do - if [ -f "${WORKDIR}/${file}" ]; then - echo "=============== ${file} ===============" - cat "${WORKDIR}/${file}" - echo - fi - done - - - name: 'Upload run artifacts' - if: "${{ always() && steps.prepare.outputs.decision == 'run' }}" - uses: 'actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a' # v7.0.1 - with: - name: 'qwen-resolve-pr-${{ steps.resolve.outputs.pr_number }}' - path: '${{ env.WORKDIR }}/' - if-no-files-found: 'ignore' - - - name: 'Report skipped request' - # always(): the prepare step's EXIT trap writes decision=failed when it - # crashes, but a bare if implicitly requires success() — so without - # always() this step is skipped on the very crash it must report. - if: "${{ always() && (steps.prepare.outputs.decision == 'skip' || steps.prepare.outputs.decision == 'unsupported' || steps.prepare.outputs.decision == 'failed') }}" - env: - GH_TOKEN: '${{ secrets.CI_DEV_BOT_PAT }}' - PR_NUMBER: '${{ steps.resolve.outputs.pr_number }}' - SKIP_REASON: '${{ steps.prepare.outputs.skip_reason }}' - run: |- - set -euo pipefail - { - echo "" - echo "Qwen Code did not run conflict resolution for this request." - echo - echo "${SKIP_REASON}" - } > "${WORKDIR}/report.md" - # Best-effort, matching 'Report result': a transient API error here must - # not abort the step under set -e and swallow the only failure signal. - gh pr comment "$PR_NUMBER" --repo "$REPO" --body-file "${WORKDIR}/report.md" || - echo "::warning::Resolve was skipped, but posting the skip-reason comment failed." - - name: 'Report result' - if: "${{ always() && steps.prepare.outputs.decision == 'run' }}" - shell: '/bin/bash --noprofile --norc -eo pipefail {0}' + if: "${{ always() }}" env: - # The BASH_ENV/LD_* pins mirror 'Resolution check': the agent step can - # plant them through $GITHUB_ENV, and they execute at process startup. - BASH_ENV: '' - LD_PRELOAD: '' - LD_AUDIT: '' - LD_LIBRARY_PATH: '' GH_TOKEN: '${{ secrets.CI_DEV_BOT_PAT }}' PUSH_TOKEN: '${{ secrets.CI_DEV_BOT_PAT }}' - PR_NUMBER: '${{ steps.resolve.outputs.pr_number }}' - HEAD_REF: '${{ steps.prepare.outputs.head_ref }}' - HEAD_SHA: '${{ steps.prepare.outputs.head_sha }}' - HEAD_REPO: '${{ steps.prepare.outputs.head_repo }}' + HEAD_REF: '${{ needs.resolve-pr.outputs.head_ref }}' + HEAD_SHA: '${{ needs.resolve-pr.outputs.head_sha }}' + HEAD_REPO: '${{ needs.resolve-pr.outputs.head_repo }}' OUTCOME: '${{ steps.verify.outputs.outcome }}' FAILURE_KIND: '${{ steps.verify.outputs.failure_kind }}' - RESOLVE_OUTCOME: '${{ steps.resolve_conflicts.outcome }}' + RESOLVE_OUTCOME: '${{ needs.resolve-pr.outputs.agent_outcome }}' # Mirrors timeout-minutes on 'Resolve conflicts'; a test pins the pair. AGENT_TIMEOUT_MINUTES: '75' DRY_RUN: '${{ env.DRY_RUN }}' RUN_URL: '${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}' run: |- set -euo pipefail - - # SECURITY: this step runs AFTER the prompt-injectable agent step and - # carries the push token — it must not execute or route through - # anything that step can plant. The BASH_ENV/LD_* startup channels are - # pinned empty in the step env above; here: resolve binaries through a - # pinned PATH, drop git's env channels and every config scope (a url - # rewrite redirects the token URL, hooks run with it in argv), and the - # proxy/gh vars that could route the push or the PAT comment to a - # listener. Full rationale → qwen-autofix.md#af-014. - PATH=/usr/bin:/bin - unset GIT_CONFIG_PARAMETERS GIT_ALLOW_PROTOCOL GIT_PROXY_COMMAND \ - GIT_SSL_NO_VERIFY GIT_SSL_CAINFO GIT_EXEC_PATH GIT_DIR GIT_WORK_TREE \ - GIT_COMMON_DIR GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES \ - GIT_SHALLOW_FILE GIT_ASKPASS GIT_SSH GIT_SSH_COMMAND \ - HTTP_PROXY HTTPS_PROXY http_proxy https_proxy ALL_PROXY NO_PROXY \ - GH_HOST GH_ENTERPRISE_TOKEN GH_CONFIG_DIR - export GIT_CONFIG_COUNT=0 export GIT_TERMINAL_PROMPT=0 - export GIT_CONFIG_SYSTEM=/dev/null - export GIT_CONFIG_GLOBAL=/dev/null - rm -f .git/config - # gh reads ~/.config/gh unless told otherwise, and the agent can write - # there (http_unix_socket, a host override): give it a fresh, empty - # config dir rather than leaving the default in reach. - GH_CONFIG_DIR="$(mktemp -d "${RUNNER_TEMP:?}/qwen-resolve-gh.XXXXXX")" - export GH_CONFIG_DIR - git() { command git -c core.hooksPath=/dev/null -c core.fsmonitor= -c credential.helper= "$@"; } + mkdir -p "${WORKDIR}" push_failed=false @@ -3173,14 +3209,6 @@ jobs: push_fail_reason='' if [ "$OUTCOME" = "fixed" ] && [ "$DRY_RUN" != "true" ]; then - # Re-assert the .git directory guard (see 'Resolution check'): a - # gitfile planted after that step would let a url.*.insteadOf in - # the attacker's config rewrite this push URL and carry the token - # to the attacker host. - if [ ! -d .git ]; then - echo ".git is not a directory — workspace tampered after the agent step; refusing to push." - exit 1 - fi if [ -z "${PUSH_TOKEN}" ]; then echo "::error::CI_DEV_BOT_PAT is required to push conflict fixes." exit 1 @@ -3197,13 +3225,12 @@ jobs: # (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 must also not - # inherit state the agent can plant: the scrub at the top of this step - # pins PATH and removes every git config scope, and git runs hookless - # below, so a prompt-injected agent cannot redirect or hook the step - # that carries the token. + # 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 -c core.hooksPath=/dev/null push --no-verify \ + 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 diff --git a/scripts/tests/qwen-resolve-workflow.test.js b/scripts/tests/qwen-resolve-workflow.test.js index cbac841b2d1..6c2bdf6a976 100644 --- a/scripts/tests/qwen-resolve-workflow.test.js +++ b/scripts/tests/qwen-resolve-workflow.test.js @@ -7,6 +7,7 @@ import { chmodSync, existsSync, + mkdirSync, mkdtempSync, readFileSync, rmSync, @@ -264,7 +265,8 @@ describe('qwen resolve workflow', () => { }); it('truncates an over-long report visibly, on a character boundary', () => { - const helper = resolveJob.match( + // The report is composed in the publish job. + const helper = job(workflow, 'publish-resolution').match( /(SUMMARY_MAX_BYTES=\d+\n[\s\S]*?append_safe_file\(\) \{[\s\S]*?\n {10}\})/, )?.[1]; expect(helper).toBeTruthy(); @@ -672,6 +674,9 @@ describe('qwen resolve workflow', () => { nextJob === -1 ? workflow.slice(resolveJobStart) : workflow.slice(resolveJobStart, resolveJobStart + 1 + nextJob); + // The credentialed half of /resolve — verification, push, result comment — + // lives in its own job on a runner that never executed the agent. + const publishJob = job(workflow, 'publish-resolution'); const reviewJob = job(workflow, 'review-pr'); const delayAutomaticReviewJob = job(workflow, 'delay-automatic-review'); const authorizeJob = job(workflow, 'authorize'); @@ -728,24 +733,28 @@ describe('qwen resolve workflow', () => { expect(authorizeStep).not.toContain('principal="$PR_AUTHOR"'); }); - it('keeps the authorization and scope guards on resolve-pr', () => { - // /resolve must require write+ permission before any credentialed push. + it('keeps the authorization and scope guards on the /resolve lane', () => { + // /resolve must require write+ permission before any credentialed push; + // the publish job inherits that gate through `needs`. expect(resolveJob).toContain( "needs.authorize.outputs.should_review == 'true'", ); + expect(publishJob).toContain("needs: ['resolve-pr']"); // Fork PRs are supported: the head is fetched through refs/pull/N/head and // the resolved branch is pushed back to the PR's head repository. expect(resolveJob).toContain('refs/pull/${PR_NUMBER}/head'); - expect(resolveJob).toContain('github.com/${HEAD_REPO}.git'); + expect(publishJob).toContain('refs/pull/${PR_NUMBER}/head'); + expect(publishJob).toContain('github.com/${HEAD_REPO}.git'); // Out-of-scope edits (prompt-injection symptom) fail closed. - expect(resolveJob).toContain( + expect(publishJob).toContain( 'Agent modified files outside the conflict set', ); - // The push only happens through the credentialed publish step, SHA-pinned: + // The push only happens through the credentialed publish job, SHA-pinned: // the bare flag would allow any force-push regardless of the remote's current // state, defeating the concurrent-update guard. - expect(resolveJob).toContain('--force-with-lease="refs/heads/'); - expect(resolveJob).toContain(':${HEAD_SHA}"'); + expect(publishJob).toContain('--force-with-lease="refs/heads/'); + expect(publishJob).toContain(':${HEAD_SHA}"'); + expect(resolveJob).not.toContain('--force-with-lease'); }); it('fetches the PR head into a collision-free local ref', () => { @@ -758,35 +767,46 @@ describe('qwen resolve workflow', () => { expect(resolveJob).not.toContain( '+refs/pull/${PR_NUMBER}/head:refs/remotes/origin/${head_ref}', ); - expect(resolveJob).toContain('HEAD_FETCH_REF:'); - expect(resolveJob).toContain( + // The publish job fetches the same ref from GitHub for itself and then + // points it at the head the agent resolved FROM, so the guards compare + // against that commit rather than a head that moved meanwhile. + expect(publishJob).toContain('HEAD_FETCH_REF:'); + expect(publishJob).toContain( + 'git fetch origin "+refs/pull/${PR_NUMBER}/head:${HEAD_FETCH_REF}"', + ); + expect(publishJob).toContain( + 'git update-ref "$HEAD_FETCH_REF" "$HEAD_SHA"', + ); + expect(publishJob).toContain( 'git diff --name-only -z --diff-filter=ACMRT "$HEAD_FETCH_REF" HEAD', ); }); - it('keeps the verification-gate failure checks on resolve-pr', () => { + it('keeps the verification-gate failure checks on the publish job', () => { // These guard against prompt-injection symptoms; a future edit that drops // any of them from the credentialed conflict-resolution path must fail here. - expect(resolveJob).toContain( + expect(publishJob).toContain( 'Leftover conflict markers found after resolution', ); - expect(resolveJob).toContain('Branch still has merge conflicts with'); - expect(resolveJob).toContain('The top commit is a default merge commit'); - expect(resolveJob).toContain( + expect(publishJob).toContain('Branch still has merge conflicts with'); + expect(publishJob).toContain('The top commit is a default merge commit'); + expect(publishJob).toContain( 'Branch unchanged and no no-action.md was written', ); - expect(resolveJob).toContain( + expect(publishJob).toContain( 'The conflict-resolution agent step did not succeed', ); - expect(resolveJob).toContain('address-summary.md is missing'); - expect(resolveJob).toContain('Unresolved index conflicts remain'); + expect(publishJob).toContain('address-summary.md is missing'); + expect(publishJob).toContain('Unresolved index conflicts remain'); }); - it('pins the core security controls on resolve-pr', () => { - // Checkout must not persist GITHUB_TOKEN into .git/config. + it('pins the core security controls on the /resolve lane', () => { + // Checkout must not persist GITHUB_TOKEN into .git/config, in either job. expect(resolveJob).toContain('persist-credentials: false'); + expect(publishJob).toContain('persist-credentials: false'); + expect(publishJob).not.toContain('persist-credentials: true'); // The resolution check carries no writable GitHub token (defense in depth). - expect(resolveJob).toContain("GITHUB_TOKEN: ''"); + expect(publishJob).toContain("GITHUB_TOKEN: ''"); // The agent runs WITHOUT the container sandbox — measured, not assumed: // the first day `sandbox: true` took effect (#9252, 2026-08-16) /resolve // went from 84% pushed to 0 of 81, dying on a missing versioned image or @@ -797,102 +817,131 @@ describe('qwen resolve workflow', () => { expect(resolveJob).not.toContain('"sandbox": true'); // Concurrent /resolve runs must not interleave on the credentialed push. expect(resolveJob).toContain('cancel-in-progress: false'); + expect(publishJob).toContain('cancel-in-progress: false'); }); - it('runs the agent without any GitHub credentials', () => { + it('runs the agent without any GitHub credentials, and nothing credentialed after it', () => { + const agentStart = resolveJob.indexOf("- name: 'Resolve conflicts'"); const agentStep = resolveJob.slice( - resolveJob.indexOf("- name: 'Resolve conflicts'"), - resolveJob.indexOf("- name: 'Resolution check'"), + agentStart, + resolveJob.indexOf("- name: 'Package resolution'"), ); expect(agentStep.length).toBeGreaterThan(0); expect(agentStep).not.toContain('GH_TOKEN'); expect(agentStep).not.toContain('GITHUB_TOKEN'); expect(agentStep).not.toContain('CI_BOT_PAT'); expect(agentStep).not.toContain('CI_DEV_BOT_PAT'); + // Once the agent has run, no step of its job may carry a secret: the + // runner is agent-written (config, hooks, refs, PATH, the real + // $GITHUB_ENV, live processes), and an in-job scrub cannot enumerate + // every entrance. 'Report skipped request' is the one exception, and it + // runs only when the agent did not (decision != run). + const afterAgent = resolveJob.slice( + resolveJob.indexOf("- name: 'Package resolution'"), + ); + const skipStart = afterAgent.indexOf("- name: 'Report skipped request'"); + expect(skipStart).toBeGreaterThan(-1); + expect(afterAgent.slice(0, skipStart)).not.toContain('secrets.'); + const skipStep = step(resolveJob, 'Report skipped request'); + expect(skipStep).toContain("steps.prepare.outputs.decision == 'skip'"); + expect(skipStep).not.toContain("decision == 'run'"); }); - it('hardens the post-agent steps against state the agent can plant', () => { - // The agent step runs --yolo without a sandbox BEFORE these steps in the - // same job: anything it appends to $GITHUB_ENV reaches every later step, - // and it writes the workspace. 'Resolution check' must not be falsifiable - // by planted shims or config, and the credentialed push in 'Report result' - // must not inherit an executable or routable channel to the token. - const verifyStep = step(resolveJob, 'Resolution check'); - const reportStep = step(resolveJob, 'Report result'); - const showStep = step(resolveJob, 'Show run artifacts'); - - for (const postAgentStep of [verifyStep, reportStep, showStep]) { - // BASH_ENV and LD_* execute at process startup, before any body line - // runs — pin them empty at the step boundary, not merely unset in the - // body. - expect(postAgentStep).toContain("BASH_ENV: ''"); - expect(postAgentStep).toContain("LD_PRELOAD: ''"); - expect(postAgentStep).toContain("LD_AUDIT: ''"); - expect(postAgentStep).toContain("LD_LIBRARY_PATH: ''"); - // A planted PATH shim must not resolve git for the guard or the push, - // and a planted PATH prepend must not stand in for the step shell. - expect(postAgentStep).toContain('PATH=/usr/bin:/bin'); - expect(postAgentStep).toContain( - "shell: '/bin/bash --noprofile --norc -eo pipefail {0}'", - ); - // Host and global config are agent-reachable runner surfaces (url - // rewrites redirect the token URL); GIT_CONFIG_COUNT injects config - // through env. None of it may reach the git calls. - expect(postAgentStep).toContain('export GIT_CONFIG_SYSTEM=/dev/null'); - expect(postAgentStep).toContain('export GIT_CONFIG_GLOBAL=/dev/null'); - expect(postAgentStep).toContain('export GIT_CONFIG_COUNT=0'); - // GIT_CONFIG_PARAMETERS injects config that survives GIT_CONFIG_COUNT=0 - // and the /dev/null scopes; GIT_EXTERNAL_DIFF and GIT_EXEC_PATH are - // exec channels a content diff can hit. All three steps must drop the - // whole list — the comment claims a 'same scrub', so pin it. - expect(postAgentStep).toContain('unset GIT_CONFIG_PARAMETERS'); - // The scrub must execute BEFORE each step's first git/gh use — - // relocating it below them re-exposes the checks and the push to - // planted state, the exact regression this PR exists to stop. - expect(postAgentStep.indexOf('PATH=/usr/bin:/bin')).toBeGreaterThan(-1); - } - expect(verifyStep.indexOf('PATH=/usr/bin:/bin')).toBeLessThan( + it('publishes from a job whose runner never executed the agent', () => { + // The structural containment: the agent runs --yolo without a sandbox and + // can leave anything behind on its runner — config scopes, hooks, moved + // refs, PATH shims, appends to the real $GITHUB_ENV, a detached process + // reading /proc. An in-job denylist could not be closed (review rounds + // 2–5 on #10428 each demonstrated the next entrance), so the credentialed + // half runs on a fresh runner and takes from the agent job only a verified + // git bundle and the report files. + expect(publishJob.length).toBeGreaterThan(0); + expect(publishJob).toContain("needs: ['resolve-pr']"); + expect(publishJob).toContain( + 'if: "${{ always() && needs.resolve-pr.outputs.decision == \'run\' }}"', + ); + // The token-bearing step lives in the publish job, and that job runs no + // agent: no `id: 'resolve_conflicts'`, no qwen invocation. + expect(publishJob).toContain('x-access-token:${PUSH_TOKEN}'); + expect(resolveJob).not.toContain('x-access-token:'); + expect(resolveJob).not.toContain('PUSH_TOKEN'); + expect(publishJob).not.toContain("id: 'resolve_conflicts'"); + expect(publishJob).not.toContain('qwen \\'); + expect(publishJob).not.toContain('Install Qwen CLI'); + // Its checkout is fresh and its refs come from GitHub, not from the + // agent's checkout: base and head are fetched again, and the guards + // compare against the head the agent resolved from. + expect(step(publishJob, 'Checkout base branch')).toContain( + 'persist-credentials: false', + ); + const verifyStep = step(publishJob, 'Resolution check'); + expect(verifyStep).toContain( + 'git fetch origin "+refs/pull/${PR_NUMBER}/head:${HEAD_FETCH_REF}" "+refs/heads/${BASE_REF}:refs/remotes/origin/${BASE_REF}"', + ); + expect(verifyStep).toContain( + "HEAD_SHA: '${{ needs.resolve-pr.outputs.head_sha }}'", + ); + expect(verifyStep).toContain( + 'git update-ref "$HEAD_FETCH_REF" "$HEAD_SHA"', + ); + // The resolution is admitted as objects only: a bundle that must verify + // against the head fetched from GitHub and descend from the head the + // agent resolved from; absence means the agent changed nothing. + expect(verifyStep).toContain('git bundle verify "$bundle"'); + expect(verifyStep).toContain( + 'git fetch "$bundle" "+${resolution_ref}:${resolution_ref}"', + ); + expect(verifyStep).toContain( + 'git merge-base --is-ancestor "$HEAD_SHA" HEAD', + ); + expect(verifyStep.indexOf('git bundle verify')).toBeLessThan( + verifyStep.indexOf('git fetch "$bundle"'), + ); + expect(verifyStep.indexOf('git merge-base --is-ancestor')).toBeLessThan( verifyStep.indexOf('git ls-files -u'), ); - expect(reportStep.indexOf('PATH=/usr/bin:/bin')).toBeLessThan( - reportStep.indexOf('push --no-verify'), + // A missing artifact (agent job cancelled or crashed before packaging) + // is reported as an infrastructure failure, not silently. + expect(verifyStep).toContain('uploaded no run artifact'); + expect(step(publishJob, 'Download run artifacts')).toContain( + 'continue-on-error: true', ); - expect(reportStep.indexOf('PATH=/usr/bin:/bin')).toBeLessThan( - reportStep.indexOf('gh pr comment'), + // The two halves agree on the artifact name and the bundle's shape. + const packageStep = step(resolveJob, 'Package resolution'); + expect(packageStep).toContain( + 'git bundle create "${WORKDIR}/resolution.bundle" "${HEAD_SHA}..refs/heads/qwen-resolve/pr-${PR_NUMBER}"', ); - - for (const postAgentStep of [verifyStep, reportStep]) { - // The LOCAL .git/config is the highest-precedence scope and the agent - // writes the workspace — hooks, filters, includes and url rewrites - // planted there survive GIT_CONFIG_GLOBAL=/dev/null, so it is removed. - expect(postAgentStep).toContain('rm -f .git/config'); - // A gitfile-replaced .git (`gitdir: `) makes that rm a no-op and - // lets a url.*.insteadOf rewrite the push URL with the token in the - // request path — refuse it before any check and before the push. - expect(postAgentStep).toContain('[ ! -d .git ]'); - } - expect(verifyStep.indexOf('[ ! -d .git ]')).toBeLessThan( - verifyStep.indexOf('git ls-files -u'), + expect(step(resolveJob, 'Upload run artifacts')).toContain( + "name: 'qwen-resolve-pr-${{ steps.resolve.outputs.pr_number }}'", ); - expect(reportStep.indexOf('[ ! -d .git ]')).toBeLessThan( - reportStep.indexOf('push --no-verify'), + expect(step(publishJob, 'Download run artifacts')).toContain( + "name: 'qwen-resolve-pr-${{ needs.resolve-pr.outputs.pr_number }}'", ); - - // The credentialed push itself: hookless and verify-less, so a planted - // pre-push hook never receives the token URL. - expect(reportStep).toContain( - 'git -c core.hooksPath=/dev/null push --no-verify', + expect(step(publishJob, 'Download run artifacts')).toContain( + "path: '/tmp/qwen-resolve'", ); - // Proxy and gh-host vars could route the push or the PAT comment to a - // listener; they must be dropped from the inherited environment. - expect(reportStep).toContain('HTTP_PROXY HTTPS_PROXY'); - expect(reportStep).toContain('GH_HOST GH_ENTERPRISE_TOKEN GH_CONFIG_DIR'); + expect(publishJob).toContain("WORKDIR: '/tmp/qwen-resolve'"); + // The verdict on the agent step crosses the job boundary as a job output. + expect(resolveJob).toContain( + "agent_outcome: '${{ steps.resolve_conflicts.outcome }}'", + ); + expect(verifyStep).toContain( + "RESOLVE_OUTCOME: '${{ needs.resolve-pr.outputs.agent_outcome }}'", + ); + // Both jobs hold the head-write lock: the push is what it serialises. + for (const text of [resolveJob, publishJob]) { + expect(text).toContain( + "group: 'qwen-pr-head-write-${{ github.event.issue.number || github.event.inputs.pr_number }}'", + ); + } + // The push itself is verify-less, so a hook could never receive the URL. + expect(step(publishJob, 'Report result')).toContain('git push --no-verify'); }); it('pins the CLI version and bounds the agent step', () => { const agentStep = resolveJob.slice( resolveJob.indexOf("- name: 'Resolve conflicts'"), - resolveJob.indexOf("- name: 'Resolution check'"), + resolveJob.indexOf("- name: 'Package resolution'"), ); // `latest` ties every run to the npm release pipeline of the moment: the // 2026-08-15 dist-tag pointed at an unresolvable 0.21.12 and 14 runs died @@ -919,7 +968,14 @@ describe('qwen resolve workflow', () => { // swallowing it (`exit 0`, a trailing `|| true`) would let a crashed or // turn-capped agent report success and misroute the comment. expect(agentStep).toContain('status=$?'); - expect(agentStep).toMatch(/exit "\$status"\s*$/); + // `exit "$status"` must be the last command of the run block: only + // comments and blank lines may follow it (the next step's YAML comment). + const exitAt = agentStep.lastIndexOf('exit "$status"'); + expect(exitAt).toBeGreaterThan(-1); + const tail = agentStep.slice(exitAt + 'exit "$status"'.length).split('\n'); + expect(tail.every((l) => l.trim() === '' || l.trim().startsWith('#'))).toBe( + true, + ); // The decoys only mask the invocation's env; the real runner command // files stay agent-discoverable and are truncated on every exit path. for (const file of [ @@ -937,9 +993,7 @@ describe('qwen resolve workflow', () => { // and the number quoted in the failure comment must agree. const stepTimeout = agentStep.match(/^\s+timeout-minutes: (\d+)$/m); expect(stepTimeout).not.toBeNull(); - const reportStep = resolveJob.slice( - resolveJob.indexOf("- name: 'Report result'"), - ); + const reportStep = step(publishJob, 'Report result'); expect(reportStep).toContain(`AGENT_TIMEOUT_MINUTES: '${stepTimeout[1]}'`); expect(Number(timeoutCmd[1])).toBeLessThan(Number(stepTimeout[1]) * 60); // Read the ceiling instead of duplicating it as a literal: if the job's @@ -956,13 +1010,13 @@ describe('qwen resolve workflow', () => { // outcome != success on the agent step means no resolution was attempted // (install/model/infra error, step timeout, cancellation). The comment // must say so and must not invite a re-run, which repeats the failure. - const verifyStep = step(resolveJob, 'Resolution check'); - const reportStep = step(resolveJob, 'Report result'); + const verifyStep = step(publishJob, 'Resolution check'); + const reportStep = step(publishJob, 'Report result'); expect(reportStep).toContain( "FAILURE_KIND: '${{ steps.verify.outputs.failure_kind }}'", ); expect(reportStep).toContain( - "RESOLVE_OUTCOME: '${{ steps.resolve_conflicts.outcome }}'", + "RESOLVE_OUTCOME: '${{ needs.resolve-pr.outputs.agent_outcome }}'", ); // The failure_kind=infra write is pinned to the never-ran block of @@ -1027,6 +1081,264 @@ describe('qwen resolve workflow', () => { expect(elseArm).not.toContain('This is not a verdict'); }); + it('packages the resolution in the agent job and verifies it on a fresh runner', () => { + // Drives the real run blocks — 'Package resolution' from resolve-pr and + // 'Resolution check' from publish-resolution — against a bare origin, an + // agent-side clone and a fresh publish-side clone, the way the two + // runners see them. The publish side never reads the agent's checkout. + const runBlock = (section, name) => { + const text = step(section, name); + const start = + text.indexOf('\n run: |-\n') + '\n run: |-\n'.length; + expect(start).toBeGreaterThan('\n run: |-\n'.length - 1); + return text.slice(start).replace(/^ {10}/gm, ''); + }; + const packageBlock = runBlock(resolveJob, 'Package resolution'); + const verifyBlock = runBlock(publishJob, 'Resolution check'); + expect(packageBlock).not.toContain('${{'); + expect(verifyBlock).not.toContain('${{'); + + 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', + }; + const git = (cwd, ...args) => { + const r = spawnSync('git', args, { cwd, env: gitEnv, encoding: 'utf8' }); + if (r.status !== 0) { + throw new Error( + `git ${args.join(' ')} in ${cwd}:\n${r.stdout}\n${r.stderr}`, + ); + } + return r.stdout.trim(); + }; + const root = mkdtempSync(path.join(tmpdir(), 'qwen-resolve-split-')); + try { + // Origin: main and a PR head that conflict on a.txt; main also adds c.txt. + const origin = path.join(root, 'origin.git'); + const seed = path.join(root, 'seed'); + git(root, 'init', '-q', '--bare', origin); + git(root, 'init', '-q', '-b', 'main', seed); + writeFileSync(path.join(seed, 'a.txt'), 'shared\n'); + git(seed, 'add', '.'); + git(seed, 'commit', '-q', '-m', 'chore: seed'); + git(seed, 'checkout', '-q', '-b', 'feature'); + writeFileSync(path.join(seed, 'a.txt'), 'feature side\n'); + git(seed, 'commit', '-q', '-am', 'feat: feature'); + const headSha = git(seed, 'rev-parse', 'HEAD'); + git(seed, 'checkout', '-q', 'main'); + writeFileSync(path.join(seed, 'a.txt'), 'main side\n'); + writeFileSync(path.join(seed, 'c.txt'), 'from main\n'); + git(seed, 'add', '.'); + git(seed, 'commit', '-q', '-m', 'feat: main'); + git(seed, 'remote', 'add', 'origin', origin); + git(seed, 'push', '-q', 'origin', 'main', 'feature:refs/pull/1/head'); + + // Agent side: the checkout the agent resolved in, plus its WORKDIR. + const agent = path.join(root, 'agent'); + git(root, 'clone', '-q', origin, agent); + git( + agent, + 'fetch', + '-q', + 'origin', + '+refs/pull/1/head:refs/remotes/origin/qwen-resolve/pr-1/head', + ); + git( + agent, + 'checkout', + '-q', + '-B', + 'qwen-resolve/pr-1', + 'refs/remotes/origin/qwen-resolve/pr-1/head', + ); + spawnSync('git', ['merge', '--no-commit', 'origin/main'], { + cwd: agent, + env: gitEnv, + }); + const resolveWith = (extraFile) => { + writeFileSync(path.join(agent, 'a.txt'), 'resolved by agent\n'); + git(agent, 'add', 'a.txt'); + if (extraFile) { + writeFileSync(path.join(agent, extraFile), 'planted\n'); + git(agent, 'add', extraFile); + } + git( + agent, + 'commit', + '-q', + '-m', + 'fix: resolve merge conflicts with main', + ); + }; + resolveWith(null); + const resolvedSha = git(agent, 'rev-parse', 'HEAD'); + + const pkg = (workdir, extraEnv = {}) => { + rmSync(workdir, { recursive: true, force: true }); + mkdirSync(workdir, { recursive: true }); + const r = spawnSync('bash', ['-c', packageBlock], { + cwd: agent, + env: { + ...gitEnv, + WORKDIR: workdir, + HEAD_SHA: headSha, + PR_NUMBER: '1', + ...extraEnv, + }, + encoding: 'utf8', + }); + return r; + }; + const verify = (workdir, extraEnv = {}) => { + const publish = path.join( + root, + `publish-${Math.random().toString(36).slice(2)}`, + ); + git(root, 'clone', '-q', origin, publish); + const out = path.join(publish, 'outputs.txt'); + writeFileSync(out, ''); + const r = spawnSync('bash', ['-c', verifyBlock], { + cwd: publish, + env: { + ...gitEnv, + WORKDIR: workdir, + PR_NUMBER: '1', + BASE_REF: 'main', + HEAD_FETCH_REF: 'refs/remotes/origin/qwen-resolve/pr-1/head', + HEAD_SHA: headSha, + RESOLVE_OUTCOME: 'success', + GITHUB_OUTPUT: out, + ...extraEnv, + }, + encoding: 'utf8', + }); + return { ...r, outputs: readFileSync(out, 'utf8'), publish }; + }; + + // (a) The happy path: the bundle imports, descends from the head, and + // every structural check passes on the publish side. + const w1 = path.join(root, 'w1'); + const p1 = pkg(w1); + expect(p1.status, p1.stdout + p1.stderr).toBe(0); + expect(existsSync(path.join(w1, 'resolution.bundle'))).toBe(true); + writeFileSync(path.join(w1, 'address-summary.md'), 'summary\n'); + const v1 = verify(w1); + expect(v1.status, v1.stdout + v1.stderr).toBe(0); + expect(v1.outputs).toContain('outcome=fixed'); + expect(git(v1.publish, 'rev-parse', 'HEAD')).toBe(resolvedSha); + expect( + git( + v1.publish, + 'rev-parse', + 'refs/remotes/origin/qwen-resolve/pr-1/head', + ), + ).toBe(headSha); + + // (b) A bundle whose commit does not descend from the head the agent + // resolved from — built on main alone, as a prompt-injected agent that + // abandoned the PR branch would produce — is refused before any + // content check runs. + const w2 = path.join(root, 'w2'); + mkdirSync(w2, { recursive: true }); + git(agent, 'checkout', '-q', '-B', 'stray', 'origin/main'); + writeFileSync(path.join(agent, 'a.txt'), 'stray\n'); + git(agent, 'commit', '-q', '-am', 'fix: stray'); + git(agent, 'update-ref', 'refs/heads/qwen-resolve/pr-1', 'stray'); + git( + agent, + 'bundle', + 'create', + path.join(w2, 'resolution.bundle'), + `${headSha}..refs/heads/qwen-resolve/pr-1`, + ); + writeFileSync(path.join(w2, 'address-summary.md'), 'summary\n'); + const v2 = verify(w2); + expect(v2.status).toBe(1); + expect(v2.stdout).toContain('does not descend from the PR head'); + expect(v2.outputs).toContain('outcome=failed'); + expect(v2.outputs).not.toContain('outcome=fixed'); + git(agent, 'checkout', '-q', 'qwen-resolve/pr-1'); + git(agent, 'reset', '-q', '--hard', resolvedSha); + + // (b2) A bundle whose prerequisite the publish runner has never seen + // (an agent-local commit) fails `git bundle verify` outright. + const w2b = path.join(root, 'w2b'); + mkdirSync(w2b, { recursive: true }); + git(agent, 'checkout', '-q', '-B', 'local-only', headSha); + writeFileSync(path.join(agent, 'z.txt'), 'local\n'); + git(agent, 'add', 'z.txt'); + git(agent, 'commit', '-q', '-m', 'chore: local only'); + const localOnly = git(agent, 'rev-parse', 'HEAD'); + writeFileSync(path.join(agent, 'a.txt'), 'on local\n'); + git(agent, 'commit', '-q', '-am', 'fix: on local'); + git(agent, 'update-ref', 'refs/heads/qwen-resolve/pr-1', 'local-only'); + git( + agent, + 'bundle', + 'create', + path.join(w2b, 'resolution.bundle'), + `${localOnly}..refs/heads/qwen-resolve/pr-1`, + ); + const v2b = verify(w2b); + expect(v2b.status).toBe(1); + expect(v2b.stdout).toContain('does not verify against the PR head'); + expect(v2b.outputs).toContain('outcome=failed'); + git(agent, 'checkout', '-q', '-B', 'qwen-resolve/pr-1', resolvedSha); + + // (c) No bundle (the agent changed nothing) plus no-action.md → noop. + const w3 = path.join(root, 'w3'); + const p3 = pkg(w3, { HEAD_SHA: resolvedSha }); + expect(p3.status).toBe(0); + expect(p3.stdout).toContain('branch unchanged'); + expect(existsSync(path.join(w3, 'resolution.bundle'))).toBe(false); + writeFileSync(path.join(w3, 'no-action.md'), 'nothing to do\n'); + // A no-op is only accepted when the head really merges cleanly: verify + // against a base the head does not conflict with (the merge-tree check + // runs first and refuses a no-action.md on a still-conflicting head). + git(seed, 'push', '-q', 'origin', `${headSha}:refs/heads/base2`); + const v3 = verify(w3, { BASE_REF: 'base2' }); + expect(v3.status, v3.stdout + v3.stderr).toBe(0); + expect(v3.outputs).toContain('outcome=noop'); + const v3c = verify(w3); + expect(v3c.status).toBe(1); + expect(v3c.stdout).toContain( + 'Branch still has merge conflicts with main', + ); + + // (d) No artifact at all (agent job died before packaging) → an + // infrastructure failure, never a silent job failure. + const v4 = verify(path.join(root, 'missing')); + expect(v4.status).toBe(1); + expect(v4.outputs).toContain('outcome=failed'); + expect(v4.outputs).toContain('failure_kind=infra'); + + // (e) The scope guard still bites on the imported bundle: an extra file + // outside the base-changed set travels in the bundle and is refused. + git(agent, 'reset', '-q', '--hard', headSha); + spawnSync('git', ['merge', '--no-commit', 'origin/main'], { + cwd: agent, + env: gitEnv, + }); + resolveWith('planted.txt'); + const w5 = path.join(root, 'w5'); + pkg(w5); + writeFileSync(path.join(w5, 'address-summary.md'), 'summary\n'); + const v5 = verify(w5); + expect(v5.status).toBe(1); + expect(v5.stdout).toContain( + 'Agent modified files outside the conflict set', + ); + expect(v5.outputs).toContain('outcome=failed'); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); + it('supports dry-run and workflow_dispatch', () => { expect(workflow).toContain('github.event.inputs.dry_run'); expect(workflow).toContain('in dry-run mode'); @@ -1039,10 +1351,10 @@ describe('qwen resolve workflow', () => { // changes; a token without the `workflow` scope is rejected, and that gets its // own actionable reason. A 403 (maintainer-edits off / org-owned fork / PAT // lacking push) and a stale force-with-lease are reported differently too. - expect(resolveJob).toContain("push_fail_reason='workflow_scope'"); - expect(resolveJob).toContain('grant that scope to the push bot'); - expect(resolveJob).toContain("push_fail_reason='permission'"); - expect(resolveJob).toContain("push_fail_reason='moved'"); - expect(resolveJob).toContain('Allow edits by maintainers'); + expect(publishJob).toContain("push_fail_reason='workflow_scope'"); + expect(publishJob).toContain('grant that scope to the push bot'); + expect(publishJob).toContain("push_fail_reason='permission'"); + expect(publishJob).toContain("push_fail_reason='moved'"); + expect(publishJob).toContain('Allow edits by maintainers'); }); }); From 1d7b829d7cea2b1276c7bd612328c2a8b6dc6971 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 30 Aug 2026 04:32:59 +0800 Subject: [PATCH 08/12] test(ci): pin the /resolve agent invocation wiring and ordering Round-5 suggestions: --prompt and --auth-type are asserted on the invocation; `set +e` must sit between the stop token and the invocation and `set -e` resume after the command-file truncation; the workspace .qwen/settings.json removal must precede the qwen invocation. --- .github/scripts/qwen-triage-workflow.test.mjs | 9 +++++++++ scripts/tests/qwen-resolve-workflow.test.js | 19 +++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/.github/scripts/qwen-triage-workflow.test.mjs b/.github/scripts/qwen-triage-workflow.test.mjs index ea5d68486c5..29fc5474684 100644 --- a/.github/scripts/qwen-triage-workflow.test.mjs +++ b/.github/scripts/qwen-triage-workflow.test.mjs @@ -389,6 +389,15 @@ describe('qwen-code-pr-review.yml resolve-pr: agent settings', () => { // runner killing the process tree — only then are the resume and the // truncation above reached (mirrors 'Run review'). assert.ok(run.includes('timeout --kill-after=10s')); + // The workspace-layer settings file is removed BEFORE qwen starts: a + // fork-committed .qwen/settings.json would otherwise outrank the + // per-run home written above for the whole run. + const settingsRm = run.indexOf('rm -f .qwen/settings.json'); + assert.ok(settingsRm > -1, 'the workspace settings file must be removed'); + assert.ok( + settingsRm < run.indexOf('timeout --kill-after=10s'), + 'the workspace settings removal must precede the qwen invocation', + ); assert.ok(run.includes('echo "::stop-commands::${stop_token}"')); assert.ok(run.includes("printf '\\n::%s::\\n' \"$stop_token\"")); assert.ok(run.includes('--approval-mode yolo')); diff --git a/scripts/tests/qwen-resolve-workflow.test.js b/scripts/tests/qwen-resolve-workflow.test.js index 6c2bdf6a976..c51ae7debe0 100644 --- a/scripts/tests/qwen-resolve-workflow.test.js +++ b/scripts/tests/qwen-resolve-workflow.test.js @@ -957,6 +957,25 @@ describe('qwen resolve workflow', () => { expect(agentStep).toContain('GITHUB_ENV="$decoy_dir/github-env"'); expect(agentStep).toContain('GITHUB_PATH="$decoy_dir/github-path"'); expect(agentStep).toContain('::stop-commands::'); + // The invocation wiring itself: dropping --prompt would start a yolo + // agent with no task, and the auth type selects the OpenAI-compatible + // endpoint the secrets are for. + expect(agentStep).toContain('--prompt "$PROMPT" \\'); + expect(agentStep).toContain('--auth-type openai \\'); + // errexit is off around the invocation only: the parsing-off window + // must be closed and the command files truncated on every exit path, + // so `set +e` sits between the stop token and the invocation and + // `set -e` resumes after the truncation block. + const stopIdx = agentStep.indexOf('::stop-commands::'); + const offIdx = agentStep.indexOf('\n set +e\n'); + const statusIdx = agentStep.indexOf('status=$?'); + const onIdx = agentStep.indexOf('\n set -e\n'); + expect(stopIdx).toBeGreaterThan(-1); + expect(offIdx).toBeGreaterThan(stopIdx); + expect(statusIdx).toBeGreaterThan(offIdx); + expect(onIdx).toBeGreaterThan( + agentStep.indexOf(': > "${GITHUB_STEP_SUMMARY:?}"'), + ); // A hung agent must end inside the script (GNU timeout), not by the // runner's timeout-minutes killing the process tree — only the // script-internal timeout reaches the ::stop-commands:: resume and the From 4fb0c4490a6eaf867dde0aec5a399993dfc22c03 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Sun, 30 Aug 2026 03:09:20 +0000 Subject: [PATCH 09/12] fix(ci): keep /resolve re-runs from republishing a stale resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-6 review findings: R6-2 — the run artifact name carried no run_attempt suffix on either side, so a "Re-run failed jobs" failed the second upload on the first attempt's same-name artifact and the publish job's by-name download picked up attempt 1's bundle — re-publishing the stale resolution as if it were the re-run's work. Suffix both sides (and the user-facing comment references) with the attempt, mirroring the sibling review lane's documented doctrine for the same upload-artifact version, and extend the artifact-name agreement pins in both contract suites. R6-1 — the two `if: "${{ always() }}"` lines added by the structural split are bare double-quoted strings, which the enforced yamllint quoted-strings rule rejects, failing the Test job. Flip them to single quotes, the form eight sibling workflows already use. --- .github/scripts/qwen-triage-workflow.test.mjs | 8 +++++--- .github/workflows/qwen-code-pr-review.yml | 19 +++++++++++-------- scripts/tests/qwen-resolve-workflow.test.js | 9 ++++++--- 3 files changed, 22 insertions(+), 14 deletions(-) diff --git a/.github/scripts/qwen-triage-workflow.test.mjs b/.github/scripts/qwen-triage-workflow.test.mjs index 29fc5474684..d21a56d37ae 100644 --- a/.github/scripts/qwen-triage-workflow.test.mjs +++ b/.github/scripts/qwen-triage-workflow.test.mjs @@ -535,21 +535,23 @@ describe('qwen-code-pr-review.yml resolve-pr: agent settings', () => { 'lineage must be checked before any content check', ); // The artifact is the same one the agent job uploads; its absence is an - // infrastructure failure, not a silent job failure. + // infrastructure failure, not a silent job failure. The name carries + // run_attempt on both sides: a re-run keeps the run_id, and without the + // suffix the re-run would republish attempt 1's stale bundle. const download = publish.steps.find((s) => s.uses?.startsWith('actions/download-artifact@'), ); assert.equal(download['continue-on-error'], true); assert.equal( download.with.name, - "qwen-resolve-pr-${{ needs.resolve-pr.outputs.pr_number }}", + 'qwen-resolve-pr-${{ needs.resolve-pr.outputs.pr_number }}-attempt-${{ github.run_attempt }}', ); const upload = resolvePrJob.steps.find((s) => s.uses?.startsWith('actions/upload-artifact@'), ); assert.equal( upload.with.name, - "qwen-resolve-pr-${{ steps.resolve.outputs.pr_number }}", + 'qwen-resolve-pr-${{ steps.resolve.outputs.pr_number }}-attempt-${{ github.run_attempt }}', ); assert.ok(verify.run.includes('failure_kind=infra')); assert.ok(verify.run.includes('uploaded no run artifact')); diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 336cb94b72c..60a19668b5a 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -2920,7 +2920,10 @@ jobs: if: "${{ always() && steps.prepare.outputs.decision == 'run' }}" uses: 'actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a' # v7.0.1 with: - name: 'qwen-resolve-pr-${{ steps.resolve.outputs.pr_number }}' + # The name carries run_attempt: a re-run keeps the run_id, and the + # publish job downloads by this exact name — without the suffix the + # re-run 409s on attempt 1's artifact and republishes the stale one. + name: 'qwen-resolve-pr-${{ steps.resolve.outputs.pr_number }}-attempt-${{ github.run_attempt }}' path: '${{ env.WORKDIR }}/' if-no-files-found: 'ignore' @@ -2985,12 +2988,12 @@ jobs: continue-on-error: true uses: 'actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c' # v8.0.1 with: - name: 'qwen-resolve-pr-${{ needs.resolve-pr.outputs.pr_number }}' + name: 'qwen-resolve-pr-${{ needs.resolve-pr.outputs.pr_number }}-attempt-${{ github.run_attempt }}' path: '/tmp/qwen-resolve' - name: 'Resolution check' id: 'verify' - if: "${{ always() }}" + if: '${{ always() }}' # Refs are passed as env vars and only referenced as "$BASE_REF" / # "$HEAD_FETCH_REF" inside the script. They must never be inlined as # ${{ ... }}: branch names legally contain `$(...)`/backticks, so textual @@ -3158,7 +3161,7 @@ jobs: echo "outcome=fixed" >> "$GITHUB_OUTPUT" - name: 'Report result' - if: "${{ always() }}" + if: '${{ always() }}' env: GH_TOKEN: '${{ secrets.CI_DEV_BOT_PAT }}' PUSH_TOKEN: '${{ secrets.CI_DEV_BOT_PAT }}' @@ -3273,16 +3276,16 @@ jobs: if [ "$push_failed" = "true" ]; then case "$push_fail_reason" in workflow_scope) - echo "Qwen Code resolved the merge conflicts, but could not push to \`${HEAD_REPO}\`: resolving merges the base branch in, which includes its \`.github/workflows/**\` changes, and GitHub blocks a token without the **\`workflow\`** scope from updating workflow files. A maintainer needs to grant that scope to the push bot (classic PAT: check \`workflow\`; fine-grained PAT: Workflows → Read and write), then re-run /resolve. The resolved diff is attached as the \`qwen-resolve-pr-${PR_NUMBER}\` artifact on the [workflow run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})." + echo "Qwen Code resolved the merge conflicts, but could not push to \`${HEAD_REPO}\`: resolving merges the base branch in, which includes its \`.github/workflows/**\` changes, and GitHub blocks a token without the **\`workflow\`** scope from updating workflow files. A maintainer needs to grant that scope to the push bot (classic PAT: check \`workflow\`; fine-grained PAT: Workflows → Read and write), then re-run /resolve. The resolved diff is attached as the \`qwen-resolve-pr-${PR_NUMBER}-attempt-${{ github.run_attempt }}\` artifact on the [workflow run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})." ;; permission) - echo "Qwen Code resolved the merge conflicts, but could not push to \`${HEAD_REPO}\`. For a fork PR this needs **Allow edits by maintainers** enabled, and GitHub blocks maintainer edits on forks owned by an organization. The resolved diff is attached as the \`qwen-resolve-pr-${PR_NUMBER}\` artifact on the [workflow run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})." + echo "Qwen Code resolved the merge conflicts, but could not push to \`${HEAD_REPO}\`. For a fork PR this needs **Allow edits by maintainers** enabled, and GitHub blocks maintainer edits on forks owned by an organization. The resolved diff is attached as the \`qwen-resolve-pr-${PR_NUMBER}-attempt-${{ github.run_attempt }}\` artifact on the [workflow run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})." ;; moved) - echo "Qwen Code resolved the merge conflicts, but the head branch changed while resolving, so the update was not pushed. Re-run /resolve. The resolved diff is attached as the \`qwen-resolve-pr-${PR_NUMBER}\` artifact on the [workflow run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})." + echo "Qwen Code resolved the merge conflicts, but the head branch changed while resolving, so the update was not pushed. Re-run /resolve. The resolved diff is attached as the \`qwen-resolve-pr-${PR_NUMBER}-attempt-${{ github.run_attempt }}\` artifact on the [workflow run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})." ;; *) - echo "Qwen Code resolved the merge conflicts, but pushing to \`${HEAD_REPO}\` failed. The resolved diff is attached as the \`qwen-resolve-pr-${PR_NUMBER}\` artifact on the [workflow run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})." + echo "Qwen Code resolved the merge conflicts, but pushing to \`${HEAD_REPO}\` failed. The resolved diff is attached as the \`qwen-resolve-pr-${PR_NUMBER}-attempt-${{ github.run_attempt }}\` artifact on the [workflow run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})." ;; esac elif [ "$DRY_RUN" = "true" ]; then diff --git a/scripts/tests/qwen-resolve-workflow.test.js b/scripts/tests/qwen-resolve-workflow.test.js index c51ae7debe0..00bfdac0edd 100644 --- a/scripts/tests/qwen-resolve-workflow.test.js +++ b/scripts/tests/qwen-resolve-workflow.test.js @@ -906,16 +906,19 @@ describe('qwen resolve workflow', () => { expect(step(publishJob, 'Download run artifacts')).toContain( 'continue-on-error: true', ); - // The two halves agree on the artifact name and the bundle's shape. + // The two halves agree on the artifact name and the bundle's shape. The + // name carries run_attempt on BOTH sides: a re-run keeps the run_id, and + // without the suffix the second attempt 409s on the first attempt's upload + // while the by-name download republishes the stale first-attempt bundle. const packageStep = step(resolveJob, 'Package resolution'); expect(packageStep).toContain( 'git bundle create "${WORKDIR}/resolution.bundle" "${HEAD_SHA}..refs/heads/qwen-resolve/pr-${PR_NUMBER}"', ); expect(step(resolveJob, 'Upload run artifacts')).toContain( - "name: 'qwen-resolve-pr-${{ steps.resolve.outputs.pr_number }}'", + "name: 'qwen-resolve-pr-${{ steps.resolve.outputs.pr_number }}-attempt-${{ github.run_attempt }}'", ); expect(step(publishJob, 'Download run artifacts')).toContain( - "name: 'qwen-resolve-pr-${{ needs.resolve-pr.outputs.pr_number }}'", + "name: 'qwen-resolve-pr-${{ needs.resolve-pr.outputs.pr_number }}-attempt-${{ github.run_attempt }}'", ); expect(step(publishJob, 'Download run artifacts')).toContain( "path: '/tmp/qwen-resolve'", From 06d174bd7f24835b79b73502b69fdc005b9fd03c Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Sun, 30 Aug 2026 07:30:21 +0000 Subject: [PATCH 10/12] fix(ci): let /resolve partial re-runs publish the agent's resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-7 review findings: R6-2 (fix-induced) — the run_attempt artifact-name suffix that fixed the stale-republish defect broke "Re-run failed jobs" the other way: the re-run executes only publish-resolution, whose attempt number has no artifact, so the by-name download missed the agent's upload, 'Resolution check' called a SUCCESSFUL agent an infrastructure failure, and 'Report result' posted a comment self-contradictory on every clause ("outcome=success ... CLI install, model endpoint or infrastructure failure ... Requesting /resolve again will fail the same way") while the resolution stayed unpublished — every further partial re-run repeated it. Thread the attempt that ran the agent across the job boundary (the runner provides GITHUB_RUN_ATTEMPT; the agent never writes it) and download by it, so the partial re-run picks up the artifact that actually exists and publishes it. For the residual case — the agent succeeded but the artifact is genuinely gone, upload failure or expiry — classify the lost artifact (failure_kind=artifact_missing) instead of the agent run, and word the comment accordingly: the agent finished, the result could not be published, and "Re-run all jobs" is the one recovery that re-produces the artifact. Fixture scenario (d) now drives the real 'Resolution check' block across both arms and reds on the pre-fix shape. R7-1 — the follow-up bot's qwen_cli_version pin is inert on its primary lane: the pinned action's 'Install Qwen Code' step skips when qwen is already on PATH (verified at the pinned SHA), always true on the ecs-qwen pool whose fleet CLI update-ecs-runner-qwen.yml maintains, so the comment's "the pin wins" falsely certified there. Rewrite it to state the real contract — fleet version authoritative on ecs-qwen, the pin governing the ubuntu-latest fallback lane — and say so in the contract test too. Also lands the round's mechanical items: .size-baseline records both workflows' exact new sizes (the ratchet passed only on its 4096-byte allowance); the unpinned gitfile guard in 'Package resolution' and GIT_TERMINAL_PROMPT=0 in 'Report result' get contract pins (mutation survivors M9/M10 of the sandboxed verification); a stale test cross-reference ('hardens the post-agent steps' no longer exists) and a garbled scope-guard comment repaired in passing. --- .github/scripts/qwen-triage-workflow.test.mjs | 31 +++++-- .github/workflows/.size-baseline | 4 +- .github/workflows/qwen-code-pr-review.yml | 43 ++++++++-- .github/workflows/qwen-issue-followup-bot.yml | 10 ++- scripts/tests/qwen-resolve-workflow.test.js | 86 ++++++++++++++++--- 5 files changed, 142 insertions(+), 32 deletions(-) diff --git a/.github/scripts/qwen-triage-workflow.test.mjs b/.github/scripts/qwen-triage-workflow.test.mjs index d21a56d37ae..dff1ec4d9ca 100644 --- a/.github/scripts/qwen-triage-workflow.test.mjs +++ b/.github/scripts/qwen-triage-workflow.test.mjs @@ -534,17 +534,32 @@ describe('qwen-code-pr-review.yml resolve-pr: agent settings', () => { verify.run.indexOf('git ls-files -u'), 'lineage must be checked before any content check', ); - // The artifact is the same one the agent job uploads; its absence is an - // infrastructure failure, not a silent job failure. The name carries + // The artifact is the same one the agent job uploads. The name carries // run_attempt on both sides: a re-run keeps the run_id, and without the - // suffix the re-run would republish attempt 1's stale bundle. + // suffix the re-run would republish attempt 1's stale bundle. The + // DOWNLOAD spells the attempt that RAN THE AGENT, not the publish job's + // own run_attempt: a partial "Re-run failed jobs" re-runs only the + // publish job, whose attempt number has no artifact. A missing artifact + // with a successful agent step is the lost artifact, never an infra + // failure of the agent run. const download = publish.steps.find((s) => s.uses?.startsWith('actions/download-artifact@'), ); assert.equal(download['continue-on-error'], true); assert.equal( download.with.name, - 'qwen-resolve-pr-${{ needs.resolve-pr.outputs.pr_number }}-attempt-${{ github.run_attempt }}', + 'qwen-resolve-pr-${{ needs.resolve-pr.outputs.pr_number }}-attempt-${{ needs.resolve-pr.outputs.agent_run_attempt }}', + ); + assert.equal( + resolvePrJob.outputs.agent_run_attempt, + '${{ steps.resolve.outputs.run_attempt }}', + ); + const resolveStep = resolvePrJob.steps.find((s) => s.id === 'resolve'); + assert.ok( + resolveStep.run.includes( + 'echo "run_attempt=${GITHUB_RUN_ATTEMPT}" >> "$GITHUB_OUTPUT"', + ), + 'the attempt that ran the agent must cross the job boundary', ); const upload = resolvePrJob.steps.find((s) => s.uses?.startsWith('actions/upload-artifact@'), @@ -554,7 +569,8 @@ describe('qwen-code-pr-review.yml resolve-pr: agent settings', () => { 'qwen-resolve-pr-${{ steps.resolve.outputs.pr_number }}-attempt-${{ github.run_attempt }}', ); assert.ok(verify.run.includes('failure_kind=infra')); - assert.ok(verify.run.includes('uploaded no run artifact')); + assert.ok(verify.run.includes('failure_kind=artifact_missing')); + assert.ok(verify.run.includes('its run artifact is missing')); }); it('keeps both /resolve jobs on ephemeral hosted runners', () => { @@ -582,7 +598,10 @@ describe('qwen-issue-followup-bot.yml: agent settings', () => { // notarget outage is why the pin exists: without a contract test a // future rename silently reverts the bot to the dist-tag with nothing // red at merge time. resolve-pr's equivalent pin has two tests; this one - // keeps the follow-up bot's covered too. + // keeps the follow-up bot's covered too. The pin governs runners without + // `qwen` on PATH — the action's install step skips when the CLI is + // already present — so it binds the ubuntu-latest fallback lane; on the + // ecs-qwen lane the fleet-installed CLI is authoritative. assert.match( String(followupStep.with?.qwen_cli_version), /^\d+\.\d+\.\d+$/, diff --git a/.github/workflows/.size-baseline b/.github/workflows/.size-baseline index be0618b2d5f..566d76ce326 100644 --- a/.github/workflows/.size-baseline +++ b/.github/workflows/.size-baseline @@ -37,9 +37,9 @@ 5942 qwen-autofix-fork-signal.yml 454547 qwen-autofix.yml 7061 qwen-ci-flaky-rerun.yml -189756 qwen-code-pr-review.yml +193207 qwen-code-pr-review.yml 79041 qwen-fleet-shepherd.yml -20525 qwen-issue-followup-bot.yml +21443 qwen-issue-followup-bot.yml 5760 qwen-pr-safety-precheck.yml 27648 qwen-triage-finalize.yml 350381 qwen-triage.yml diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 60a19668b5a..b7675b23590 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -2475,6 +2475,7 @@ jobs: decision: '${{ steps.prepare.outputs.decision }}' pr_number: '${{ steps.resolve.outputs.pr_number }}' agent_outcome: '${{ steps.resolve_conflicts.outcome }}' + agent_run_attempt: '${{ steps.resolve.outputs.run_attempt }}' head_ref: '${{ steps.prepare.outputs.head_ref }}' head_sha: '${{ steps.prepare.outputs.head_sha }}' head_repo: '${{ steps.prepare.outputs.head_repo }}' @@ -2526,6 +2527,10 @@ jobs: pr_number="$ISSUE_NUMBER" fi echo "pr_number=${pr_number}" >> "$GITHUB_OUTPUT" + # The attempt that runs the agent — publish-resolution downloads + # the run artifact by THIS number, not its own github.run_attempt + # (a partial "Re-run failed jobs" re-runs only the publish job). + echo "run_attempt=${GITHUB_RUN_ATTEMPT}" >> "$GITHUB_OUTPUT" - name: 'Checkout base branch' uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 @@ -2985,10 +2990,15 @@ jobs: # A cancelled or crashed agent job may have uploaded nothing; the # import in 'Resolution check' reports that as an infrastructure # failure instead of letting this step end the job without a comment. + # The name ends in the attempt that RAN THE AGENT, not this job's own + # github.run_attempt: a partial "Re-run failed jobs" re-runs only the + # publish job, and only the agent's attempt uploaded an artifact — + # downloading by this attempt's name would miss it and wedge the + # re-run on a contradictory failure comment. continue-on-error: true uses: 'actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c' # v8.0.1 with: - name: 'qwen-resolve-pr-${{ needs.resolve-pr.outputs.pr_number }}-attempt-${{ github.run_attempt }}' + name: 'qwen-resolve-pr-${{ needs.resolve-pr.outputs.pr_number }}-attempt-${{ needs.resolve-pr.outputs.agent_run_attempt }}' path: '/tmp/qwen-resolve' - name: 'Resolution check' @@ -3069,9 +3079,14 @@ jobs: elif [ -d "$WORKDIR" ]; then git checkout -q -B "qwen-resolve/pr-${PR_NUMBER}" "$HEAD_FETCH_REF" else - echo "The agent job uploaded no run artifact (cancelled or crashed before packaging); nothing to verify." + # Only reachable with RESOLVE_OUTCOME=success — any other outcome + # exits in the never-ran block above. The agent finished, but its + # artifact is gone (the upload step failed or it expired), so the + # result cannot be verified or published; classify the LOST + # ARTIFACT, never the agent run, and word the comment accordingly. + echo "The agent step succeeded, but its run artifact is missing (the upload failed or the artifact expired); nothing to verify." echo "outcome=failed" >> "$GITHUB_OUTPUT" - echo "failure_kind=infra" >> "$GITHUB_OUTPUT" + echo "failure_kind=artifact_missing" >> "$GITHUB_OUTPUT" exit 1 fi @@ -3140,7 +3155,7 @@ jobs: # defense-in-depth layer. merge_base="$(git merge-base "origin/${BASE_REF}" "$HEAD_FETCH_REF")" # -z/sort -zu mirrors the conflict-marker check above so unusual - # filenames in the diff are handled consistently. ponytail: tr back to + # filenames in the diff are handled consistently, then tr back to # newlines because bash vars can't hold NUL — a filename containing a # literal newline still splits, but that's covered by the # write-authorization + no-token containment this guard backstops. @@ -3171,6 +3186,10 @@ jobs: OUTCOME: '${{ steps.verify.outputs.outcome }}' FAILURE_KIND: '${{ steps.verify.outputs.failure_kind }}' RESOLVE_OUTCOME: '${{ needs.resolve-pr.outputs.agent_outcome }}' + # The attempt that ran the agent; the artifact name cited below + # carries it (this job's own github.run_attempt is one ahead on a + # partial "Re-run failed jobs"). + AGENT_RUN_ATTEMPT: '${{ needs.resolve-pr.outputs.agent_run_attempt }}' # Mirrors timeout-minutes on 'Resolve conflicts'; a test pins the pair. AGENT_TIMEOUT_MINUTES: '75' DRY_RUN: '${{ env.DRY_RUN }}' @@ -3276,16 +3295,16 @@ jobs: if [ "$push_failed" = "true" ]; then case "$push_fail_reason" in workflow_scope) - echo "Qwen Code resolved the merge conflicts, but could not push to \`${HEAD_REPO}\`: resolving merges the base branch in, which includes its \`.github/workflows/**\` changes, and GitHub blocks a token without the **\`workflow\`** scope from updating workflow files. A maintainer needs to grant that scope to the push bot (classic PAT: check \`workflow\`; fine-grained PAT: Workflows → Read and write), then re-run /resolve. The resolved diff is attached as the \`qwen-resolve-pr-${PR_NUMBER}-attempt-${{ github.run_attempt }}\` artifact on the [workflow run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})." + echo "Qwen Code resolved the merge conflicts, but could not push to \`${HEAD_REPO}\`: resolving merges the base branch in, which includes its \`.github/workflows/**\` changes, and GitHub blocks a token without the **\`workflow\`** scope from updating workflow files. A maintainer needs to grant that scope to the push bot (classic PAT: check \`workflow\`; fine-grained PAT: Workflows → Read and write), then re-run /resolve. The resolved diff is attached as the \`qwen-resolve-pr-${PR_NUMBER}-attempt-${AGENT_RUN_ATTEMPT}\` artifact on the [workflow run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})." ;; permission) - echo "Qwen Code resolved the merge conflicts, but could not push to \`${HEAD_REPO}\`. For a fork PR this needs **Allow edits by maintainers** enabled, and GitHub blocks maintainer edits on forks owned by an organization. The resolved diff is attached as the \`qwen-resolve-pr-${PR_NUMBER}-attempt-${{ github.run_attempt }}\` artifact on the [workflow run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})." + echo "Qwen Code resolved the merge conflicts, but could not push to \`${HEAD_REPO}\`. For a fork PR this needs **Allow edits by maintainers** enabled, and GitHub blocks maintainer edits on forks owned by an organization. The resolved diff is attached as the \`qwen-resolve-pr-${PR_NUMBER}-attempt-${AGENT_RUN_ATTEMPT}\` artifact on the [workflow run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})." ;; moved) - echo "Qwen Code resolved the merge conflicts, but the head branch changed while resolving, so the update was not pushed. Re-run /resolve. The resolved diff is attached as the \`qwen-resolve-pr-${PR_NUMBER}-attempt-${{ github.run_attempt }}\` artifact on the [workflow run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})." + echo "Qwen Code resolved the merge conflicts, but the head branch changed while resolving, so the update was not pushed. Re-run /resolve. The resolved diff is attached as the \`qwen-resolve-pr-${PR_NUMBER}-attempt-${AGENT_RUN_ATTEMPT}\` artifact on the [workflow run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})." ;; *) - echo "Qwen Code resolved the merge conflicts, but pushing to \`${HEAD_REPO}\` failed. The resolved diff is attached as the \`qwen-resolve-pr-${PR_NUMBER}-attempt-${{ github.run_attempt }}\` artifact on the [workflow run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})." + echo "Qwen Code resolved the merge conflicts, but pushing to \`${HEAD_REPO}\` failed. The resolved diff is attached as the \`qwen-resolve-pr-${PR_NUMBER}-attempt-${AGENT_RUN_ATTEMPT}\` artifact on the [workflow run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})." ;; esac elif [ "$DRY_RUN" = "true" ]; then @@ -3302,7 +3321,13 @@ jobs: append_safe_file "${WORKDIR}/no-action.md" ;; *) - if [ "$FAILURE_KIND" = "infra" ]; then + if [ "$FAILURE_KIND" = "artifact_missing" ]; then + # The agent SUCCEEDED: the infra wording below would blame + # a run that did not fail, and a partial re-run cannot + # bring the artifact back. Say the agent finished, and + # point at the one recovery that re-produces the artifact. + echo "Qwen Code's conflict-resolution agent finished successfully, but its run artifact never reached the publish job — the upload failed or the artifact expired — so the result could not be verified or published. This is not a verdict on the conflict. Use **Re-run all jobs** on the [workflow run](${RUN_URL}) to run the agent again and publish a fresh result; re-running only the failed jobs cannot recover the missing artifact." + elif [ "$FAILURE_KIND" = "infra" ]; then # The agent never reached a verdict: outcome=failure (CLI # install, model endpoint, crash) or outcome=cancelled (its # step timeout). Say which, and do not invite a re-run: the diff --git a/.github/workflows/qwen-issue-followup-bot.yml b/.github/workflows/qwen-issue-followup-bot.yml index 9043742133f..9f27e767374 100644 --- a/.github/workflows/qwen-issue-followup-bot.yml +++ b/.github/workflows/qwen-issue-followup-bot.yml @@ -301,8 +301,14 @@ jobs: # time, and every consumer following the dist-tag died on # `npm error notarget` when it moved to an unresolvable version on # 2026-08-15. resolve-pr pins the same version for the same reason. - # The action wires this input as step-level env, which masks any - # inherited org/repo QWEN_CLI_VERSION variable — the pin wins. + # The pin governs only runners WITHOUT `qwen` on PATH: the action's + # 'Install Qwen Code' step skips when the CLI is already present + # (verified in the pinned action SHA). On the ecs-qwen pool — this + # job's default lane — the fleet-installed CLI maintained at + # `@latest` by update-ecs-runner-qwen.yml is therefore + # authoritative; the pin binds the ubuntu-latest fallback lane, + # where the action wires this input as step-level env, masking any + # inherited org/repo QWEN_CLI_VERSION variable. qwen_cli_version: '0.21.10' # The input name is `settings` — this action version has no # `settings_json` input, and an unknown input is silently dropped, diff --git a/scripts/tests/qwen-resolve-workflow.test.js b/scripts/tests/qwen-resolve-workflow.test.js index 00bfdac0edd..2e32041372a 100644 --- a/scripts/tests/qwen-resolve-workflow.test.js +++ b/scripts/tests/qwen-resolve-workflow.test.js @@ -900,9 +900,12 @@ describe('qwen resolve workflow', () => { expect(verifyStep.indexOf('git merge-base --is-ancestor')).toBeLessThan( verifyStep.indexOf('git ls-files -u'), ); - // A missing artifact (agent job cancelled or crashed before packaging) - // is reported as an infrastructure failure, not silently. - expect(verifyStep).toContain('uploaded no run artifact'); + // A missing artifact is classified, never silent. This branch is only + // reachable with a SUCCESSFUL agent step (any other outcome exits in the + // never-ran block), so it classifies the LOST ARTIFACT — upload failed + // or expired — never the agent run itself. + expect(verifyStep).toContain('its run artifact is missing'); + expect(verifyStep).toContain('failure_kind=artifact_missing'); expect(step(publishJob, 'Download run artifacts')).toContain( 'continue-on-error: true', ); @@ -910,15 +913,29 @@ describe('qwen resolve workflow', () => { // name carries run_attempt on BOTH sides: a re-run keeps the run_id, and // without the suffix the second attempt 409s on the first attempt's upload // while the by-name download republishes the stale first-attempt bundle. + // The DOWNLOAD side spells the attempt that RAN THE AGENT, not the + // publish job's own github.run_attempt: a partial "Re-run failed jobs" + // re-runs only the publish job, whose attempt number has no artifact — + // the upload's does. The attempt crosses the job boundary as an output + // the agent never writes (the runner provides GITHUB_RUN_ATTEMPT). const packageStep = step(resolveJob, 'Package resolution'); expect(packageStep).toContain( 'git bundle create "${WORKDIR}/resolution.bundle" "${HEAD_SHA}..refs/heads/qwen-resolve/pr-${PR_NUMBER}"', ); + // A gitfile-replaced .git packages nothing: the guard fails closed + // before any bundle exists for the publish job to verify. + expect(packageStep).toContain('if [ ! -d .git ]; then'); + expect(step(resolveJob, 'Resolve pull request')).toContain( + 'echo "run_attempt=${GITHUB_RUN_ATTEMPT}" >> "$GITHUB_OUTPUT"', + ); + expect(resolveJob).toContain( + "agent_run_attempt: '${{ steps.resolve.outputs.run_attempt }}'", + ); expect(step(resolveJob, 'Upload run artifacts')).toContain( "name: 'qwen-resolve-pr-${{ steps.resolve.outputs.pr_number }}-attempt-${{ github.run_attempt }}'", ); expect(step(publishJob, 'Download run artifacts')).toContain( - "name: 'qwen-resolve-pr-${{ needs.resolve-pr.outputs.pr_number }}-attempt-${{ github.run_attempt }}'", + "name: 'qwen-resolve-pr-${{ needs.resolve-pr.outputs.pr_number }}-attempt-${{ needs.resolve-pr.outputs.agent_run_attempt }}'", ); expect(step(publishJob, 'Download run artifacts')).toContain( "path: '/tmp/qwen-resolve'", @@ -937,8 +954,18 @@ describe('qwen resolve workflow', () => { "group: 'qwen-pr-head-write-${{ github.event.issue.number || github.event.inputs.pr_number }}'", ); } - // The push itself is verify-less, so a hook could never receive the URL. - expect(step(publishJob, 'Report result')).toContain('git push --no-verify'); + // The push itself is verify-less, so a hook could never receive the URL, + // and it never answers a credential prompt against a rewritten one. + const reportStep = step(publishJob, 'Report result'); + expect(reportStep).toContain('git push --no-verify'); + expect(reportStep).toContain('export GIT_TERMINAL_PROMPT=0'); + expect(reportStep).toContain( + "AGENT_RUN_ATTEMPT: '${{ needs.resolve-pr.outputs.agent_run_attempt }}'", + ); + // The push-failure comments cite the artifact that actually exists — + // the agent's attempt, not this job's. + expect(reportStep).toContain('attempt-${AGENT_RUN_ATTEMPT}'); + expect(reportStep).not.toContain('attempt-${{ github.run_attempt }}'); }); it('pins the CLI version and bounds the agent step', () => { @@ -954,8 +981,10 @@ describe('qwen resolve workflow', () => { expect(installStep).not.toContain("QWEN_CLI_VERSION: 'latest'"); expect(installStep).toContain('@qwen-code/qwen-code@${QWEN_CLI_VERSION}'); // Direct invocation, not the action: the action runs the CLI with the - // runner's real $GITHUB_ENV/$GITHUB_PATH, which the scrub in the later - // steps cannot fully undo (see 'hardens the post-agent steps'). + // runner's real $GITHUB_ENV/$GITHUB_PATH; the direct call decoys them + // per invocation instead. Containment of everything else the agent + // plants lives in the publish split — see 'publishes from a job whose + // runner never executed the agent'. expect(agentStep).not.toContain('qwen-code-action'); expect(agentStep).toContain('GITHUB_ENV="$decoy_dir/github-env"'); expect(agentStep).toContain('GITHUB_PATH="$decoy_dir/github-path"'); @@ -1069,9 +1098,27 @@ describe('qwen resolve workflow', () => { // Pin the branch CONDITION literal, then slice the two arms of the `*)` // case so a transposition of the bodies fails — unordered toContain kept // both texts present in either arm. - const infraCond = 'if [ "$FAILURE_KIND" = "infra" ]; then'; + // The lost-artifact arm comes FIRST: the agent SUCCEEDED, so neither + // the infra wording (it blames a run that did not fail, and forbids the + // one re-run shape that re-produces the artifact) nor the verdict + // wording may post for it. + const missingCond = 'if [ "$FAILURE_KIND" = "artifact_missing" ]; then'; + const missingStart = reportStep.indexOf(missingCond); + expect(missingStart).toBeGreaterThan(-1); + const missingArm = reportStep.slice( + missingStart, + reportStep.indexOf('elif', missingStart), + ); + expect(missingArm).toContain('finished successfully'); + expect(missingArm).toContain('**Re-run all jobs**'); + expect(missingArm).toContain( + 're-running only the failed jobs cannot recover the missing artifact', + ); + expect(missingArm).not.toContain('will fail the same way'); + expect(missingArm).not.toContain('Requesting /resolve again'); + const infraCond = 'elif [ "$FAILURE_KIND" = "infra" ]; then'; const infraStart = reportStep.indexOf(infraCond); - expect(infraStart).toBeGreaterThan(-1); + expect(infraStart).toBeGreaterThan(missingStart); const infraArm = reportStep.slice( infraStart, reportStep.indexOf('else', infraStart), @@ -1332,12 +1379,25 @@ describe('qwen resolve workflow', () => { 'Branch still has merge conflicts with main', ); - // (d) No artifact at all (agent job died before packaging) → an - // infrastructure failure, never a silent job failure. + // (d) A SUCCESSFUL agent step whose artifact is gone (its upload + // failed or expired) — the shape a partial "Re-run failed jobs" used + // to hit when the download missed the artifact. Classify the lost + // artifact, never the agent run: the infra comment would blame a run + // that succeeded and forbid the re-run that re-produces the artifact. const v4 = verify(path.join(root, 'missing')); expect(v4.status).toBe(1); expect(v4.outputs).toContain('outcome=failed'); - expect(v4.outputs).toContain('failure_kind=infra'); + expect(v4.outputs).toContain('failure_kind=artifact_missing'); + expect(v4.outputs).not.toContain('failure_kind=infra'); + + // (d2) A non-success agent step keeps the infra contract whatever the + // artifact state: the never-ran block fires before the import. + const v4b = verify(path.join(root, 'missing'), { + RESOLVE_OUTCOME: 'failure', + }); + expect(v4b.status).toBe(1); + expect(v4b.outputs).toContain('outcome=failed'); + expect(v4b.outputs).toContain('failure_kind=infra'); // (e) The scope guard still bites on the imported bundle: an extra file // outside the base-changed set travels in the bundle and is refused. From a85176154b26ef2f21422ee095aec3fa97998af0 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Sun, 30 Aug 2026 21:56:08 +0000 Subject: [PATCH 11/12] fix(ci): bind the follow-up bot's CLI pin on its primary lane (#10428) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R7-1 (review rounds 7-10): the follow-up bot's qwen_cli_version pin was a dead switch on its primary lane. The job routes to the ecs-qwen pool by default, where `qwen` is always on PATH — the fleet-installed CLI that update-ecs-runner-qwen.yml maintains at `latest` — and the pinned action's 'Install Qwen Code' step skips unconditionally when `qwen` is already present (verified at the pinned action SHA). The bot therefore ran the fleet CLI, not 0.21.10: a broken npm publish propagating through the fleet update would run the agent despite the declared pin — the exact 2026-08-15 notarget outage class the pin exists to prevent. The previous round rewrote the comment to concede the dead pin; this makes the pin effective instead. Add a job-local install step before the action: `npm install --prefix` into $RUNNER_TEMP (not -g — a global install on the shared ECS machines would fight the fleet-maintenance contract) and prepend its bin dir to GITHUB_PATH. The pinned copy outranks the fleet CLI on ecs-qwen and is the only CLI on the ubuntu-latest fallback lane; the action's install then skips over the pinned copy on BOTH lanes. The action's qwen_cli_version input now carries the installed version from the step output, so the declared input can never drift from what runs (same ensure->wire shape as qwen-triage.yml). The contract test pins the whole mechanism: step presence, exact-version pin, job-local --prefix install, registry pin, PATH prepend, ordering before the action step, and the input wired from the step output. Mutation probes: step deletion, -g install, dropped PATH prepend, dist-tag version, reverted input literal, and reordered steps all red. Also records the exact sizes in .size-baseline: qwen-code-pr-review.yml 193208 (one byte merged in from #10537's runner rename) and the follow-up bot's grown file. --- .github/scripts/qwen-triage-workflow.test.mjs | 59 +++++++++++++++---- .github/workflows/.size-baseline | 4 +- .github/workflows/qwen-issue-followup-bot.yml | 50 ++++++++++++---- 3 files changed, 86 insertions(+), 27 deletions(-) diff --git a/.github/scripts/qwen-triage-workflow.test.mjs b/.github/scripts/qwen-triage-workflow.test.mjs index dff1ec4d9ca..9bd5f944570 100644 --- a/.github/scripts/qwen-triage-workflow.test.mjs +++ b/.github/scripts/qwen-triage-workflow.test.mjs @@ -100,7 +100,8 @@ const followupWorkflowPath = join( 'qwen-issue-followup-bot.yml', ); const followupDoc = parse(readFileSync(followupWorkflowPath, 'utf8')); -const followupStep = followupDoc.jobs['follow-up-issues'].steps.find( +const followupJob = followupDoc.jobs['follow-up-issues']; +const followupStep = followupJob.steps.find( (s) => s.name === 'Run Qwen issue follow-up', ); const ciWebShellJob = ciDoc.jobs.web_shell_e2e_smoke; @@ -593,19 +594,53 @@ describe('qwen-issue-followup-bot.yml: agent settings', () => { assertSettingsContract(followupStep, 'the follow-up step'); }); - it('pins the CLI version instead of following `latest`', () => { - // Unknown `with:` inputs are dropped without error, and the 2026-08-15 - // notarget outage is why the pin exists: without a contract test a - // future rename silently reverts the bot to the dist-tag with nothing - // red at merge time. resolve-pr's equivalent pin has two tests; this one - // keeps the follow-up bot's covered too. The pin governs runners without - // `qwen` on PATH — the action's install step skips when the CLI is - // already present — so it binds the ubuntu-latest fallback lane; on the - // ecs-qwen lane the fleet-installed CLI is authoritative. + it('installs the pinned CLI job-locally so the pin binds BOTH lanes', () => { + // The action's 'Install Qwen Code' step skips when `qwen` is already on + // PATH (verified at the pinned action SHA) — always true on the ecs-qwen + // pool this job routes to by default, whose fleet CLI + // update-ecs-runner-qwen.yml maintains at `latest`. An input-only pin + // never bound that primary lane, so the bot there ran whatever the fleet + // update last installed — the exact 2026-08-15 notarget outage class the + // pin exists to prevent. The job-local install is prepended to PATH, so + // the pinned copy outranks the fleet CLI on ecs-qwen and is the only CLI + // on the ubuntu-latest fallback lane; without a contract test a future + // edit silently reverts the bot to the fleet dist-tag with nothing red + // at merge time. + const install = followupJob.steps.find((s) => s.id === 'install_qwen'); + assert.ok(install, 'the pinned CLI install step must exist'); assert.match( - String(followupStep.with?.qwen_cli_version), + String(install.env?.QWEN_CLI_VERSION), /^\d+\.\d+\.\d+$/, - 'the follow-up bot must pin an exact CLI version, not a dist-tag', + 'the install must pin an exact CLI version, not a dist-tag', + ); + assert.ok( + install.run.includes('npm install --prefix') && + install.run.includes('"@qwen-code/qwen-code@${QWEN_CLI_VERSION}"'), + 'the install must be job-local (--prefix) and pinned to QWEN_CLI_VERSION', + ); + assert.ok( + install.run.includes('--registry=https://registry.npmjs.org'), + 'the install must pin the registry, not the runner npm mirror', + ); + assert.ok( + install.run.includes( + 'echo "${install_dir}/node_modules/.bin" >> "${GITHUB_PATH}"', + ), + 'the job-local bin dir must be prepended to PATH to outrank the fleet CLI', + ); + // PATH prepends apply only to LATER steps, and the action's own install + // skips on any `qwen` already on PATH: if the install stops preceding + // the action step, the fleet CLI wins on ecs-qwen again. + assert.ok( + followupJob.steps.indexOf(install) < + followupJob.steps.indexOf(followupStep), + 'the pinned install must run before the action step', + ); + // Single source of truth: the action input is the installed version, so + // it can never drift from what actually runs. + assert.equal( + String(followupStep.with?.qwen_cli_version), + '${{ steps.install_qwen.outputs.version }}', ); }); diff --git a/.github/workflows/.size-baseline b/.github/workflows/.size-baseline index 566d76ce326..aeeb7d4a2e6 100644 --- a/.github/workflows/.size-baseline +++ b/.github/workflows/.size-baseline @@ -37,9 +37,9 @@ 5942 qwen-autofix-fork-signal.yml 454547 qwen-autofix.yml 7061 qwen-ci-flaky-rerun.yml -193207 qwen-code-pr-review.yml +193208 qwen-code-pr-review.yml 79041 qwen-fleet-shepherd.yml -21443 qwen-issue-followup-bot.yml +22680 qwen-issue-followup-bot.yml 5760 qwen-pr-safety-precheck.yml 27648 qwen-triage-finalize.yml 350381 qwen-triage.yml diff --git a/.github/workflows/qwen-issue-followup-bot.yml b/.github/workflows/qwen-issue-followup-bot.yml index 9f27e767374..93f4e099e35 100644 --- a/.github/workflows/qwen-issue-followup-bot.yml +++ b/.github/workflows/qwen-issue-followup-bot.yml @@ -283,6 +283,38 @@ jobs: echo "scheduled_limit=${SCHEDULED_LIMIT_INPUT}" >> "${GITHUB_OUTPUT}" echo "Issue follow-up state: event=${EVENT_NAME} dispatch_dry=${DISPATCH_DRY_RUN} schedule_dry=${SCHEDULE_DRY_RUN} resolved_dry_run=${dry_run} scheduled_limit=${SCHEDULED_LIMIT_INPUT}" + - name: 'Install pinned Qwen CLI' + id: 'install_qwen' + env: + # Pinned, not `latest`: the CLI is installed from npm at run + # time, and every consumer following the dist-tag died on + # `npm error notarget` when it moved to an unresolvable version + # on 2026-08-15. resolve-pr pins the same version for the same + # reason; bump the two together. + QWEN_CLI_VERSION: '0.21.10' + run: |- + set -euo pipefail + # Job-local, not `npm install -g`: this job's default lane is + # the ecs-qwen pool, whose system-wide CLI + # update-ecs-runner-qwen.yml maintains at `latest` for the + # agent fleet — a global install here would fight that + # contract. The pinned copy is prepended to PATH instead, so it + # outranks the fleet CLI on ecs-qwen and is the only CLI on the + # ubuntu-latest fallback lane. The action's 'Install Qwen Code' + # step skips when `qwen` is already on PATH (verified at the + # pinned action SHA), so the agent runs THIS install on both + # lanes: an input-only pin never bound ecs-qwen, the exact + # outage class this step exists to prevent. + install_dir="${RUNNER_TEMP}/qwen-cli" + npm install --prefix "${install_dir}" --loglevel=error --no-audit \ + --registry=https://registry.npmjs.org \ + "@qwen-code/qwen-code@${QWEN_CLI_VERSION}" + echo "${install_dir}/node_modules/.bin" >> "${GITHUB_PATH}" + qwen_bin="${install_dir}/node_modules/.bin/qwen" + installed_version="$("${qwen_bin}" --version)" + echo "version=${installed_version}" >> "${GITHUB_OUTPUT}" + echo "Installed qwen ${installed_version} at ${install_dir}" + - name: 'Run Qwen issue follow-up' uses: 'QwenLM/qwen-code-action@05f81718976f3b7da657422e6e8e4d372b8621d7' env: @@ -297,19 +329,11 @@ jobs: OPENAI_API_KEY: '${{ secrets.OPENAI_API_KEY }}' OPENAI_BASE_URL: '${{ secrets.OPENAI_BASE_URL }}' OPENAI_MODEL: '${{ vars.QWEN_PR_REVIEW_MODEL }}' - # Pinned, not `latest`: the action installs the CLI from npm at run - # time, and every consumer following the dist-tag died on - # `npm error notarget` when it moved to an unresolvable version on - # 2026-08-15. resolve-pr pins the same version for the same reason. - # The pin governs only runners WITHOUT `qwen` on PATH: the action's - # 'Install Qwen Code' step skips when the CLI is already present - # (verified in the pinned action SHA). On the ecs-qwen pool — this - # job's default lane — the fleet-installed CLI maintained at - # `@latest` by update-ecs-runner-qwen.yml is therefore - # authoritative; the pin binds the ubuntu-latest fallback lane, - # where the action wires this input as step-level env, masking any - # inherited org/repo QWEN_CLI_VERSION variable. - qwen_cli_version: '0.21.10' + # The install step above already put the pinned CLI first on + # PATH on both lanes, and the action's own install skips when + # `qwen` is present — this declares the installed version to + # the action, so the input can never drift from what runs. + qwen_cli_version: '${{ steps.install_qwen.outputs.version }}' # The input name is `settings` — this action version has no # `settings_json` input, and an unknown input is silently dropped, # which is what happened to this block until the rename. From 15399f428a4636accb80f2f770736f1425654cf2 Mon Sep 17 00:00:00 2001 From: wenshao Date: Mon, 31 Aug 2026 13:34:38 +0800 Subject: [PATCH 12/12] fix(ci): give the /resolve publisher a per-run group so a completed resolution is never dropped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two-job split put publish-resolution in the shared qwen-pr-head-write- concurrency group. GitHub keeps only one PENDING job per group and replaces it when another same-group job is queued — cancel-in-progress:false protects only a RUNNING job. So after resolve-pr finished and uploaded a completed resolution, its publisher sat pending in that group, where a second /resolve, a later publisher, or an autofix writer could silently cancel it before the push or the result comment: a resolution that already succeeded, dropped with no response to the user — the exact silent-failure class this PR set exists to remove. (Reported by a reviewer on #10428.) Move the publisher to a per-run group (qwen-pr-publish-): one member for its whole life, so it can never be the replaced pending job. Two publishers that do run at once stay correct through the push, not the group — it is already force-with-lease pinned to the head the agent resolved from, so exactly one wins and the other reports "moved" cleanly. The expensive, wasteful-to-race work — the agent — stays in the shared head-write group on resolve-pr, so #7355 (resolve vs autofix agent runs) stays covered. Tests: a new 'cannot drop a completed resolution while it waits to publish' pins the per-run publisher group, the head-write agent group and the lease; the triage and publish-job contracts are updated to the new split. Reverting the publisher to the shared group reds all three. --- .github/scripts/qwen-triage-workflow.test.mjs | 25 ++++++++-- .github/workflows/qwen-code-pr-review.yml | 32 +++++++++--- scripts/tests/qwen-resolve-workflow.test.js | 49 ++++++++++++++++--- 3 files changed, 87 insertions(+), 19 deletions(-) diff --git a/.github/scripts/qwen-triage-workflow.test.mjs b/.github/scripts/qwen-triage-workflow.test.mjs index 9bd5f944570..fe5a6ebae8b 100644 --- a/.github/scripts/qwen-triage-workflow.test.mjs +++ b/.github/scripts/qwen-triage-workflow.test.mjs @@ -484,12 +484,27 @@ describe('qwen-code-pr-review.yml resolve-pr: agent settings', () => { ); assert.equal(publish['runs-on'], 'ubuntu-latest'); assert.deepEqual(publish.permissions, { contents: 'read' }); - // The push lock is held here too: the push is what it serialises. - assert.equal( - publish.concurrency.group, - resolvePrJob.concurrency.group, - ); + // The publisher uses a PER-RUN concurrency group, NOT the shared + // head-write one: it is reached only after resolve-pr uploaded a + // completed resolution, and GitHub replaces the single pending job in a + // group when another is queued, so sharing the head-write group would let + // a second /resolve or an autofix writer silently cancel this pending + // publisher and drop a resolution that already succeeded. A per-run group + // can never be replaced; competing publishers are made safe by the + // force-with-lease push, not by the group. + assert.match(publish.concurrency.group, /\$\{\{\s*github\.run_id\s*\}\}/); + assert.notEqual(publish.concurrency.group, resolvePrJob.concurrency.group); + assert.match(resolvePrJob.concurrency.group, /^qwen-pr-head-write-/); assert.equal(publish.concurrency['cancel-in-progress'], false); + // The atomic guard for two concurrent publishers is the lease, pinned to + // the head the agent resolved from. + const reportRun = publish.steps.find((s) => s.name === 'Report result').run; + assert.ok( + reportRun.includes( + '--force-with-lease="refs/heads/${HEAD_REF}:${HEAD_SHA}"', + ), + 'the publisher push must be force-with-lease pinned to HEAD_SHA', + ); // No agent here, and the token-bearing step is here and nowhere else. assert.ok(publish.steps.every((s) => s.id !== 'resolve_conflicts')); assert.ok(publish.steps.every((s) => !(s.run ?? '').includes('qwen '))); diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 98b2be83987..092174deb5b 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -2455,11 +2455,14 @@ jobs: timeout-minutes: 120 # Shared with qwen-autofix.yml's review-address job — see the rationale # there. GitHub concurrency groups are repository-scoped, so the identical - # prefix is what serialises the two workflows against each other; both - # merge the base branch and push to this PR's head, and racing them costs a - # whole agent run on the loser (observed on #7355). The prefix must stay - # byte-identical in both files; a test pins that, because renaming one side - # alone re-opens the race with nothing failing. + # prefix is what serialises the two workflows against each other; both run + # an expensive conflict-resolution agent against this PR's head, and + # racing them costs a whole agent run on the loser (observed on #7355). + # The prefix must stay byte-identical in both files; a test pins that, + # because renaming one side alone re-opens the race with nothing failing. + # Only the AGENT phase lives here; the credentialed push runs in + # publish-resolution under a per-run group (see the note there), so a + # completed resolution waiting to publish can never be replaced. concurrency: group: 'qwen-pr-head-write-${{ github.event.issue.number || github.event.inputs.pr_number }}' cancel-in-progress: false @@ -2966,10 +2969,23 @@ jobs: if: "${{ always() && needs.resolve-pr.outputs.decision == 'run' }}" runs-on: 'ubuntu-latest' timeout-minutes: 20 - # Same group as resolve-pr and qwen-autofix.yml's review-address: the push - # is what the group serialises (see the note on resolve-pr). + # A PER-RUN group, deliberately NOT the shared qwen-pr-head-write- one. + # This job is only reached after resolve-pr finished and uploaded a + # completed resolution; between that and this job starting it sits pending. + # GitHub keeps only one pending job per group and replaces it when another + # same-group job is queued (cancel-in-progress:false protects only a + # RUNNING job), so sharing the head-write group would let a second + # /resolve, a later publisher, or an autofix writer silently cancel this + # pending publisher — dropping a resolution that already succeeded, before + # it is ever pushed or reported. A per-run group has one member for life, + # so this job can never be replaced. Competing publishers stay safe not + # through the group but through the push itself: it is force-with-lease + # pinned to the head the agent resolved from, so of two concurrent pushes + # exactly one wins and the other reports "moved" cleanly (never a clobber, + # never a silent drop). The expensive, wasteful-to-race work — the agent — + # is what stays in the shared head-write group, on resolve-pr. concurrency: - group: 'qwen-pr-head-write-${{ github.event.issue.number || github.event.inputs.pr_number }}' + group: 'qwen-pr-publish-${{ github.run_id }}' cancel-in-progress: false permissions: contents: 'read' diff --git a/scripts/tests/qwen-resolve-workflow.test.js b/scripts/tests/qwen-resolve-workflow.test.js index 2e32041372a..4835e84ed8d 100644 --- a/scripts/tests/qwen-resolve-workflow.test.js +++ b/scripts/tests/qwen-resolve-workflow.test.js @@ -820,6 +820,37 @@ describe('qwen resolve workflow', () => { expect(publishJob).toContain('cancel-in-progress: false'); }); + it('cannot drop a completed resolution while it waits to publish', () => { + // GitHub keeps only one PENDING job per concurrency group and replaces it + // when another same-group job is queued; cancel-in-progress:false guards + // only a RUNNING job. If the publisher shared the head-write group it + // would sit pending after resolve-pr finished, where a second /resolve or + // an autofix writer could silently cancel it — dropping a resolution that + // already succeeded, before it is pushed or reported. So: + const groupOf = (jobText) => + jobText.match(/\n {4}concurrency:\n {6}group: '([^']*)'/)?.[1]; + const resolveGroup = groupOf(resolveJob); + const publishGroup = groupOf(publishJob); + // The agent phase keeps the shared head-write group (serialised with + // autofix — the expensive work that must not race). + expect(resolveGroup).toMatch(/^qwen-pr-head-write-/); + // The publisher uses a PER-RUN group: one member for its whole life, so + // it can never be the replaced pending job. + expect(publishGroup).toContain('${{ github.run_id }}'); + expect(publishGroup).not.toContain('qwen-pr-head-write-'); + expect(publishGroup).not.toBe(resolveGroup); + // Correctness for two publishers that DO run at once rests on the push, + // not the group: force-with-lease pinned to the head the agent resolved + // from means exactly one wins and the other reports "moved" — never a + // clobber, never a silent drop. + const reportStep = step(publishJob, 'Report result'); + expect(reportStep).toContain( + '--force-with-lease="refs/heads/${HEAD_REF}:${HEAD_SHA}"', + ); + // And the publisher is not itself cancel-on-supersede. + expect(publishJob).toContain('cancel-in-progress: false'); + }); + it('runs the agent without any GitHub credentials, and nothing credentialed after it', () => { const agentStart = resolveJob.indexOf("- name: 'Resolve conflicts'"); const agentStep = resolveJob.slice( @@ -948,12 +979,18 @@ describe('qwen resolve workflow', () => { expect(verifyStep).toContain( "RESOLVE_OUTCOME: '${{ needs.resolve-pr.outputs.agent_outcome }}'", ); - // Both jobs hold the head-write lock: the push is what it serialises. - for (const text of [resolveJob, publishJob]) { - expect(text).toContain( - "group: 'qwen-pr-head-write-${{ github.event.issue.number || github.event.inputs.pr_number }}'", - ); - } + // The agent phase holds the shared head-write lock (serialised with + // autofix); the publisher does NOT — it uses a per-run group so a + // completed resolution waiting to publish can never be replaced. See + // 'cannot drop a completed resolution while it waits to publish'. + expect(resolveJob).toContain( + "group: 'qwen-pr-head-write-${{ github.event.issue.number || github.event.inputs.pr_number }}'", + ); + expect(publishJob).toContain( + "group: 'qwen-pr-publish-${{ github.run_id }}'", + ); + const publishGroupLine = publishJob.match(/\n {6}group: '([^']*)'/)[1]; + expect(publishGroupLine).not.toContain('qwen-pr-head-write-'); // The push itself is verify-less, so a hook could never receive the URL, // and it never answers a credential prompt against a rewritten one. const reportStep = step(publishJob, 'Report result');