diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index 15082a15b19..adaadfd8c94 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -2402,7 +2402,7 @@ jobs: | select((.created_at // "") > $wm) | select((.user.login // "") != $ab) | select(((.author_association // "") | IN($trust[])) or (.user.login // "") == $rb) - | "- \(.path // "?"):\(.line // "?") @\(.user.login): \(.body // "" | gsub("\r"; ""))"' \ + | "- [rc:\(.id)] \(.path // "?"):\(.line // "?") @\(.user.login): \(.body // "" | gsub("\r"; ""))"' \ "${WORKDIR}/rc.json" echo echo "## Issue-level comments" @@ -2631,7 +2631,7 @@ jobs: if git rev-parse --verify "${BRANCH}" > /dev/null 2>&1; then git diff "origin/main...${BRANCH}" > "${WORKDIR}/pr.diff" || true fi - for f in feedback.md address-summary.md no-action.md failure.md handoff.md pr.diff; do + for f in feedback.md address-summary.md no-action.md failure.md handoff.md resolved-comments.txt pr.diff; do if [[ -f "${WORKDIR}/${f}" ]]; then echo "=============== ${f} ===============" cat "${WORKDIR}/${f}" @@ -2704,6 +2704,52 @@ jobs: else git push --no-verify origin "${BRANCH}" fi + # Resolve the review threads whose findings the agent actually + # IMPLEMENTED, so a human re-reviewing sees only what is still open + # instead of re-reading every thread to work out what was handled. + # The agent cannot do this itself - its sandbox carries no token - + # so it records the inline-comment ids it implemented and this step, + # which already holds the PAT, maps each to its thread. Findings it + # DECLINED or deferred are deliberately left open. Best-effort + # throughout: a resolve failure must never fail a good push. + if [[ -s "${WORKDIR}/resolved-comments.txt" ]]; then + # first-100 page cap: threads beyond this page are not resolved + THREADS_RAW="$(gh api graphql -f owner="${REPO%%/*}" -f name="${REPO##*/}" -F pr="${PR}" -f query=' + query($owner:String!,$name:String!,$pr:Int!){ + repository(owner:$owner,name:$name){ + pullRequest(number:$pr){ + reviewThreads(first:100){nodes{id isResolved comments(first:100){nodes{databaseId}}} pageInfo{hasNextPage}} + } + } + }' --jq '(.data.repository.pullRequest.reviewThreads // {nodes:[]})' 2> /dev/null || echo '{"nodes":[]}')" + THREADS_JSON="$(jq '.nodes' <<< "${THREADS_RAW}")" + if [[ "$(jq -r '.pageInfo.hasNextPage // false' <<< "${THREADS_RAW}")" == "true" ]]; then + echo "::warning::PR has more than 100 review threads; threads past the first page will not be resolved" + fi + RESOLVED_N=0 + while IFS= read -r rc_id || [[ -n "${rc_id}" ]]; do + rc_id="${rc_id%$'\r'}" + rc_id="${rc_id#rc:}" + [[ "${rc_id}" =~ ^[0-9]+$ ]] || continue + thread_id="$(jq -r --argjson id "${rc_id}" \ + 'map(select(.isResolved | not) + | select(any(.comments.nodes[]; .databaseId == $id))) + | .[0].id // ""' <<< "${THREADS_JSON}")" + if [[ -z "${thread_id}" ]]; then + echo "::warning::comment ${rc_id} matched no open review thread" + continue + fi + if gh api graphql -f threadId="${thread_id}" -f query=' + mutation($threadId:ID!){ + resolveReviewThread(input:{threadId:$threadId}){thread{isResolved}} + }' > /dev/null 2>&1; then + RESOLVED_N=$(( RESOLVED_N + 1 )) + else + echo "::warning::could not resolve the review thread for comment ${rc_id}" + fi + done < "${WORKDIR}/resolved-comments.txt" + echo "๐Ÿงต resolved ${RESOLVED_N} review thread(s) the agent implemented" + fi { echo "๐Ÿค– Addressed the latest review feedback (round ${NEXT_ROUND}/${MAX_ROUNDS}). What changed, and what I pushed back on:" echo diff --git a/.qwen/skills/autofix/SKILL.md b/.qwen/skills/autofix/SKILL.md index 3477534e6f8..6a265932fe7 100644 --- a/.qwen/skills/autofix/SKILL.md +++ b/.qwen/skills/autofix/SKILL.md @@ -167,6 +167,13 @@ Finish with exactly one outcome: the regenerated schema, if a settings source changed), commit once only after they pass, then write `/address-summary.md` with each feedback point, decision, changes, conflict notes, and verification results (bilingual per - Shared Rules). + Shared Rules). Also write `/resolved-comments.txt`: one inline + comment id per line โ€” the `rc:` handle shown in `feedback.md` โ€” for each + finding you IMPLEMENTED. The workflow resolves exactly those review threads + after the push, so a human re-reviewing sees only what is still open. List an + id ONLY when you actually made the change: a finding you declined or deferred + must stay unresolved so its recorded reason gets read. Omit the file (or + leave it empty) when you implemented nothing that came from an inline + comment. - No change: write `/no-action.md` (bilingual per Shared Rules). - Cannot confidently proceed: write `/failure.md` and do not commit. diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index 47e87728a68..739e38a2bbe 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -3406,6 +3406,89 @@ describe('qwen-autofix workflow', () => { expect(failed.stdout).toContain('Bad credentials'); }); + it('resolves only the review threads whose findings it implemented', () => { + // A human re-reviewing should see what is still OPEN, not re-read every + // thread to work out what the bot handled. The agent cannot resolve threads + // itself (its sandbox carries no token), so it records the inline-comment + // ids it implemented and the push step maps each to its thread. + const lines = workflow.split('\n'); + const i = lines.findIndex((l) => + l.includes('resolved-comments.txt" ]]; then'), + ); + const j = lines.findIndex( + (l, k) => k > i && l.trim().startsWith('echo "๐Ÿงต resolved'), + ); + expect(i).toBeGreaterThan(-1); + const block = lines.slice(i, j + 2).join('\n'); + // feedback.md must carry the handle the agent echoes back. + expect(workflow).toContain('- [rc:\\(.id)]'); + + const dir = mkdtempSync(join(tmpdir(), 'resolve-')); + const bin = join(dir, 'bin'); + mkdirSync(bin); + const resolvedLog = join(dir, 'resolved.log'); + writeFileSync(resolvedLog, ''); + writeFileSync( + join(dir, 'threads.json'), + JSON.stringify({ + nodes: [ + { + id: 'T_open_1', + isResolved: false, + comments: { nodes: [{ databaseId: 111 }] }, + }, + { + id: 'T_open_2', + isResolved: false, + comments: { nodes: [{ databaseId: 222 }] }, + }, + { + id: 'T_done', + isResolved: true, + comments: { nodes: [{ databaseId: 333 }] }, + }, + ], + pageInfo: { hasNextPage: false }, + }), + ); + writeFileSync( + join(bin, 'gh'), + [ + '#!/usr/bin/env bash', + 'if [[ "$*" == *mutation* ]]; then', + ' for a in "$@"; do [[ "$a" == threadId=* ]] && printf "%s\\n" "${a#threadId=}" >> "$RESOLVED_LOG"; done', + ' exit 0', + 'fi', + 'cat "$THREADS_FIXTURE"', + ].join('\n'), + ); + chmodSync(join(bin, 'gh'), 0o755); + // 111 was implemented; 333's thread is already resolved; 999 matches + // nothing. 222 was DECLINED, so it is deliberately absent and must stay open. + writeFileSync(join(dir, 'resolved-comments.txt'), 'rc:111\r\n333\n999\n'); + const out = execFileSync('bash', ['-c', `set -euo pipefail\n${block}`], { + env: { + ...process.env, + PATH: `${bin}:${process.env.PATH}`, + WORKDIR: dir, + REPO: 'QwenLM/qwen-code', + PR: '7308', + RESOLVED_LOG: resolvedLog, + THREADS_FIXTURE: join(dir, 'threads.json'), + }, + encoding: 'utf8', + }); + const resolved = readFileSync(resolvedLog, 'utf8') + .trim() + .split('\n') + .filter(Boolean); + expect(resolved).toEqual(['T_open_1']); + expect(resolved).not.toContain('T_open_2'); // declined stays open + expect(resolved).not.toContain('T_done'); // already resolved + expect(out).toContain('resolved 1 review thread'); + rmSync(dir, { recursive: true, force: true }); + }); + it('replays the handoff decision and terminal-round transitions under bash', () => { // The agent step is bounded below the 120-minute job timeout so a runaway // agent fails the STEP, not the job, leaving the always() report step time to