Skip to content

ci(qwen-resolve): run the /resolve job on a hosted runner #5862

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yiliang114 merged 1 commit into from
Jun 25, 2026
+17 −14
Merged

ci(qwen-resolve): run the /resolve job on a hosted runner #5862

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 17 additions & 14 deletions .github/workflows/qwen-code-pr-review.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -621,13 +621,14 @@ jobs:
startsWith(github.event.comment.body, '@qwen-code /resolve ') ||
startsWith(github.event.comment.body, format('@qwen-code /resolve{0}', '\n'))))
)
# Prefer the self-hosted ECS pool, but honor the MAINTAINER_ECS_RUNNER_DISABLED
# kill-switch (like the review path): if ECS is toggled off, fall back to an
# ephemeral hosted runner so /resolve still works. NOTE: this job's verification
# gate runs the untrusted PR's build/typecheck/lint/test; on the reused ECS
# workspace the cleanup step below keeps stale per-run artifacts from leaking
# between PRs (hosted runners are ephemeral, so they need no such cleanup).
runs-on: "${{ vars.MAINTAINER_ECS_RUNNER_DISABLED != 'true' && fromJSON('[\"self-hosted\", \"linux\", \"x64\", \"ecs-qwen\"]') || fromJSON('[\"ubuntu-latest\"]') }}"
# Pinned to an ephemeral hosted runner. The conflict-resolution agent step
# runs with `sandbox: true`, which on Linux needs docker or podman to launch

@qwen-code-ci-bot qwen-code-ci-bot Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The comment explains why ECS can't be used here, but doesn't note that this intentionally diverges from the MAINTAINER_ECS_RUNNER_DISABLED kill-switch pattern still used by three other jobs in this file (lines 86, 128, 206). A maintainer doing a consistency pass across the file might "re-unify" the pattern, silently re-breaking /resolve.

Consider adding one line to the comment block:

    # Unlike the ack, review-config, and authorize jobs, this one must
    # NEVER use the ECS runner regardless of MAINTAINER_ECS_RUNNER_DISABLED.

— qwen3.7-max via Qwen Code /review

# the sandbox; the self-hosted ECS pool ships no container runtime, so routing
# this job there fails the agent before it starts (exit 44, "failed to
# determine command for sandbox"). Hosted runners ship docker and are
# ephemeral, which also suits running the untrusted PR's build/typecheck/
# lint/test in the verification gate below.
runs-on: 'ubuntu-latest'

@qwen-code-ci-bot qwen-code-ci-bot Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The existing test suite in scripts/tests/qwen-resolve-workflow.test.js asserts security controls (persist-credentials: false, "sandbox": true, etc.) but never checks the runner type. A future edit that re-adds the ECS conditional (MAINTAINER_ECS_RUNNER_DISABLED pattern used by 3 other jobs) would pass all existing tests and only fail at runtime with exit 44.

Consider adding an assertion to the pins the core security controls on resolve-pr test:

// sandbox: true needs docker; hosted runners ship it, ECS does not.
expect(resolveJob).toContain("runs-on: 'ubuntu-latest'");
expect(resolveJob).not.toContain('ecs-qwen');
expect(resolveJob).not.toContain('MAINTAINER_ECS_RUNNER_DISABLED');

— qwen3.7-max via Qwen Code /review

timeout-minutes: 90
concurrency:
group: 'qwen-resolve-${{ github.event.issue.number || github.event.inputs.pr_number }}'
Expand All @@ -644,10 +645,10 @@ jobs:
WORKDIR: '/tmp/qwen-resolve'
DRY_RUN: '${{ github.event.inputs.dry_run || false }}'
steps:
# Self-hosted runners reuse the workspace and /tmp across jobs. A prior
# /resolve can leave stale ${WORKDIR} reports (failure.md, no-action.md, ...)
# that the verification gate would misread as this run's outcome, plus stale
# git worktrees that trip the checkout. Clean defensively; never fail the job.
# Defensive cleanup. Hosted runners start clean so this is normally a no-op,
# but a stale ${WORKDIR} report (failure.md, no-action.md, ...) or leftover
# git worktree would make the verification gate or checkout misread this
# run's outcome. Clean before anything else; never fail the job.
- name: 'Clean stale resolve workspace'
run: |-
set -uo pipefail
Expand Down Expand Up @@ -1133,9 +1134,11 @@ jobs:
push_failed=true
echo "::error::Push rejected; the branch was updated concurrently. Re-run /resolve."
fi
# Scrub the PAT from .git/config: this job runs on a self-hosted ECS
# runner where .git/config persists between jobs, so a later job could
# otherwise read the token. Runs regardless of push outcome.
# Scrub the PAT from .git/config defensively. Hosted runners are
# ephemeral so .git/config does not persist, but scrubbing the token
# as soon as the push is done keeps it out of the workspace that the

@qwen-code-ci-bot qwen-code-ci-bot Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The comment says scrubbing "keeps it out of the workspace that the untrusted PR's build/lint/test step runs in," but the verification gate (line ~898) executes before this step ever writes the PAT to .git/config (line 1130). The untrusted code has already finished by the time the token appears on disk — step ordering is the real protection, not scrubbing.

The scrub is still good defense-in-depth, but the comment could mislead a future maintainer into thinking it's safe to reorder steps (e.g., moving verification after push for fresher results), which would actually expose the PAT.

Suggested change
# as soon as the push is done keeps it out of the workspace that the
# Scrub the PAT from .git/config defensively. Hosted runners are
# ephemeral so .git/config does not persist across jobs. The real
# guarantee is that the verification gate (untrusted PR code) runs
# BEFORE this step writes the token; do NOT reorder push before
# verify. Runs regardless of push outcome.

— qwen3.7-max via Qwen Code /review

# untrusted PR's build/lint/test step runs in. Runs regardless of
# push outcome.
git remote set-url origin "https://github.com/${REPO}.git"
fi

Expand Down
Loading