From 010bedfc882bfe7ca5bfa09471a74db44926d614 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=98=93=E8=89=AF?= <1204183885@qq.com> Date: Tue, 7 Jul 2026 20:27:13 +0800 Subject: [PATCH] fix(autofix): report review handoff failures (#6415) * fix(autofix): report review handoff failures * fix(autofix): distinguish terminal review handoffs * fix(autofix): harden review handoff reporting * test(autofix): cover agent failure handoff marker * fix(autofix): avoid hanging on destroyed log stream * fix(autofix): harden handoff failure reporting * fix(autofix): detect split loop guard output * fix(autofix): kill timed-out qwen process group --- .github/workflows/qwen-autofix.yml | 36 ++++- .qwen/skills/autofix/scripts/run-agent.mjs | 126 ++++++++++++++-- scripts/tests/qwen-autofix-workflow.test.js | 157 +++++++++++++++++++- 3 files changed, 302 insertions(+), 17 deletions(-) diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index 53474e159e..861a537c48 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -1281,7 +1281,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 pr.diff; do + for f in feedback.md address-summary.md no-action.md failure.md handoff.md pr.diff; do if [[ -f "${WORKDIR}/${f}" ]]; then echo "=============== ${f} ===============" cat "${WORKDIR}/${f}" @@ -1382,6 +1382,8 @@ jobs: OUTCOME: '${{ steps.verify.outputs.outcome }}' CONFLICT: '${{ steps.prepare.outputs.conflict }}' DRY_RUN: '${{ needs.route.outputs.dry_run }}' + GITHUB_TOKEN: '${{ secrets.CI_DEV_BOT_PAT }}' + NEWEST: '${{ steps.prepare.outputs.newest }}' run: |- SUFFIX='' [[ "${DRY_RUN}" == "true" ]] && SUFFIX=' (dry-run, nothing pushed)' @@ -1389,7 +1391,7 @@ jobs: echo "### PR #${PR} (issue #${ISSUE}) — outcome=${OUTCOME:-unknown}${SUFFIX}" echo "- Base conflict: ${CONFLICT:-unknown}" echo - for f in address-summary.md no-action.md failure.md; do + for f in address-summary.md no-action.md failure.md handoff.md; do if [[ -s "${WORKDIR}/${f}" ]]; then echo "**${f}:**" cat "${WORKDIR}/${f}" @@ -1397,3 +1399,33 @@ jobs: fi done } >> "${GITHUB_STEP_SUMMARY}" + + if [[ "${DRY_RUN}" != "true" && "${OUTCOME:-unknown}" == "failed" && -n "${NEWEST:-}" && -n "${GITHUB_TOKEN:-}" && -s "${WORKDIR}/handoff.md" ]]; then + api_error_file="$(mktemp)" + if ! bot_actor="$(GH_TOKEN="${GITHUB_TOKEN}" gh api user --jq '.login' 2>"${api_error_file}")"; then + api_error="$(tr '\r\n' ' ' < "${api_error_file}")" + rm -f "${api_error_file}" + echo "::error::Failed to verify CI_DEV_BOT_PAT identity with gh api user: ${api_error:-unknown error}." + exit 1 + fi + rm -f "${api_error_file}" + echo "CI_DEV_BOT_PAT authenticates as ${bot_actor}" + if [[ "${bot_actor}" != "${AUTOFIX_BOT}" ]]; then + echo "::error::CI_DEV_BOT_PAT authenticates as ${bot_actor}; expected ${AUTOFIX_BOT}." + exit 1 + fi + { + echo "🤖 Could not address the latest review feedback automatically." + echo + echo "The feedback was evaluated, but AutoFix failed before producing a verified commit. A human should take over this PR." + echo + echo "Run: ${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/actions/runs/${GITHUB_RUN_ID}" + echo + echo "**Failure:**" + head -c 1500 "${WORKDIR}/failure.md" | sed 's///g' + echo + echo + echo "" + } > "${WORKDIR}/report.md" + gh pr comment "${PR}" --repo "${REPO}" --body-file "${WORKDIR}/report.md" || echo "::warning::Failed to post handoff comment on PR #${PR}" + fi diff --git a/.qwen/skills/autofix/scripts/run-agent.mjs b/.qwen/skills/autofix/scripts/run-agent.mjs index 5ea66360af..066445f111 100755 --- a/.qwen/skills/autofix/scripts/run-agent.mjs +++ b/.qwen/skills/autofix/scripts/run-agent.mjs @@ -1,7 +1,8 @@ #!/usr/bin/env node -import { spawnSync } from 'node:child_process'; +import { spawn } from 'node:child_process'; import { + createWriteStream, existsSync, mkdirSync, readFileSync, @@ -17,7 +18,7 @@ const skillPath = resolve( '..', 'SKILL.md', ); -const QWEN_TIMEOUT_MS = 50 * 60 * 1000; +const QWEN_TIMEOUT_MS = Number(process.env.QWEN_TIMEOUT_MS) || 50 * 60 * 1000; const specs = { 'assess-candidates': { inputs: ['candidates.json'], @@ -66,6 +67,87 @@ function writeFailure(workdir, message) { ); } +function writeHandoff(workdir, message) { + mkdirSync(workdir, { recursive: true }); + writeFileSync(file(workdir, 'handoff.md'), `${message}\n`); +} + +function isLoopGuardOutput(output) { + return ( + output.includes('turn_tool_call_cap') || + output.includes('Loop detection halted the run') + ); +} + +function killQwen(child, signal) { + try { + process.kill(-child.pid, signal); + } catch { + child.kill(signal); + } +} + +function runQwen(options, prompt) { + mkdirSync(options.workdir, { recursive: true }); + const log = createWriteStream(file(options.workdir, 'agent.log'), { + flags: 'w', + }); + log.on('error', () => {}); + let outputTail = ''; + let loopDetected = false; + let settled = false; + let timedOut = false; + let timer; + let killTimer; + + return new Promise((resolve) => { + const child = spawn(options.qwenBin, ['--yolo', '--prompt', prompt], { + stdio: ['inherit', 'pipe', 'pipe'], + detached: true, + }); + + const finish = (result) => { + if (settled) return; + settled = true; + clearTimeout(timer); + clearTimeout(killTimer); + const payload = { + ...result, + timedOut, + loopDetected: loopDetected || isLoopGuardOutput(outputTail), + }; + if (log.destroyed) { + resolve(payload); + } else { + log.end(() => resolve(payload)); + } + }; + + const record = (chunk, stream) => { + const text = chunk.toString('utf8'); + outputTail = (outputTail + text).slice(-20_000); + if (!loopDetected && isLoopGuardOutput(outputTail)) loopDetected = true; + log.write(chunk); + stream.write(chunk); + }; + + child.stdout.on('data', (chunk) => record(chunk, process.stdout)); + child.stderr.on('data', (chunk) => record(chunk, process.stderr)); + child.on('error', (error) => finish({ error, status: null, signal: null })); + child.on('close', (status, signal) => + finish({ error: null, status, signal }), + ); + + timer = setTimeout(() => { + timedOut = true; + killQwen(child, 'SIGTERM'); + killTimer = setTimeout(() => { + if (!settled) killQwen(child, 'SIGKILL'); + }, 10_000); + }, QWEN_TIMEOUT_MS); + }); +} + function promptFor(options, spec) { const skill = readFileSync(skillPath, 'utf8') .replace(/\r\n/g, '\n') @@ -123,22 +205,36 @@ if (missingInputs.length > 0) { ); } -const result = spawnSync(options.qwenBin, ['--yolo', '--prompt', prompt], { - stdio: 'inherit', - timeout: QWEN_TIMEOUT_MS, -}); +const result = await runQwen(options, prompt); if (result.error || result.signal || result.status !== 0) { const detail = result.error ? result.error.message - : result.signal - ? `signal ${result.signal}` - : `status ${String(result.status)}`; + : result.timedOut + ? `timeout (${QWEN_TIMEOUT_MS}ms)` + : result.signal + ? `signal ${result.signal}` + : `status ${String(result.status)}`; if (!existsSync(file(options.workdir, 'failure.md'))) { - writeFailure( - options.workdir, - `Qwen failed during ${options.mode}: ${detail}.`, - ); + if (result.loopDetected) { + writeFailure( + options.workdir, + `Qwen hit the tool-call loop guard during ${options.mode}. A human should take over this feedback batch.`, + ); + writeHandoff( + options.workdir, + 'Qwen hit the tool-call loop guard; a human should take over this feedback batch.', + ); + } else { + writeFailure( + options.workdir, + `Qwen failed during ${options.mode}: ${detail}.`, + ); + } } else { + writeHandoff( + options.workdir, + 'The agent wrote failure.md before qwen exited; a human should take over this feedback batch.', + ); console.error( `Qwen failed during ${options.mode}: ${detail}; preserving agent-written failure.md.`, ); @@ -148,6 +244,10 @@ if (result.error || result.signal || result.status !== 0) { if (existsSync(file(options.workdir, 'failure.md'))) { const content = readFileSync(file(options.workdir, 'failure.md'), 'utf8'); + writeHandoff( + options.workdir, + 'The agent wrote failure.md; a human should take over this feedback batch.', + ); console.error(`Autofix agent wrote failure.md:\n${content}`); process.exit(0); } diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index efdec99a33..907633d769 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -7,6 +7,7 @@ import { execFileSync, spawnSync } from 'node:child_process'; import { chmodSync, + existsSync, mkdtempSync, readFileSync, rmSync, @@ -895,6 +896,109 @@ describe('qwen-autofix workflow', () => { ); }); + it('posts a human-handoff marker when review addressing reaches a terminal handoff', () => { + expect(reviewAddressReportStep).toContain( + "GITHUB_TOKEN: '${{ secrets.CI_DEV_BOT_PAT }}'", + ); + expect(reviewAddressReportStep).toContain( + "NEWEST: '${{ steps.prepare.outputs.newest }}'", + ); + expect(reviewAddressReportStep).toContain('"${DRY_RUN}" != "true"'); + expect(reviewAddressReportStep).toContain('-s "${WORKDIR}/handoff.md"'); + expect(reviewAddressReportStep).toContain( + '', + ); + expect(reviewAddressReportStep).toContain( + 'Could not address the latest review feedback automatically', + ); + expect(reviewAddressReportStep).toContain('gh pr comment "${PR}"'); + expect(reviewAddressReportStep).toContain( + 'GH_TOKEN="${GITHUB_TOKEN}" gh api user --jq \'.login\'', + ); + expect(reviewAddressReportStep).toContain( + 'CI_DEV_BOT_PAT authenticates as ${bot_actor}', + ); + expect(reviewAddressReportStep).toContain( + '::warning::Failed to post handoff comment on PR #${PR}', + ); + expect(reviewAddressReportStep).toContain('human should take over'); + expect(reviewAddressReportStep).toContain("sed 's///g'"); + }); + + it('writes agent output to a log and marks loop guard failures for handoff', () => { + withRunnerDir((dir) => { + writeFileSync(join(dir, 'feedback.md'), 'feedback\n'); + const stub = writeQwenStub(dir, [ + "process.stderr.write('turn_tool_call_cap: too many tool calls\\n');", + 'process.exit(1);', + ]); + + const result = runAddressReview(dir, stub); + + expect(result.status).not.toBe(0); + expect(readFileSync(join(dir, 'agent.log'), 'utf8')).toContain( + 'turn_tool_call_cap', + ); + expect(readFileSync(join(dir, 'failure.md'), 'utf8')).toContain( + 'Qwen hit the tool-call loop guard', + ); + expect(readFileSync(join(dir, 'handoff.md'), 'utf8')).toContain( + 'human should take over', + ); + }); + }); + + it('handles agent log stream errors without crashing immediately', () => { + expect(readFileSync(autofixRunnerScriptPath, 'utf8')).toContain( + "log.on('error', () => {});", + ); + expect(readFileSync(autofixRunnerScriptPath, 'utf8')).toContain( + 'if (log.destroyed)', + ); + }); + + it('detects loop guard output before it falls out of the log tail', () => { + withRunnerDir((dir) => { + writeFileSync(join(dir, 'feedback.md'), 'feedback\n'); + const stub = writeQwenStub(dir, [ + "process.stderr.write('Loop detection halted the run\\n');", + "process.stdout.write('x'.repeat(21_000));", + 'process.exit(1);', + ]); + + const result = runAddressReview(dir, stub); + + expect(result.status).not.toBe(0); + expect(readFileSync(join(dir, 'failure.md'), 'utf8')).toContain( + 'Qwen hit the tool-call loop guard', + ); + expect(readFileSync(join(dir, 'handoff.md'), 'utf8')).toContain( + 'human should take over', + ); + }); + }); + + it('does not mark generic qwen subprocess failures for handoff', () => { + withRunnerDir((dir) => { + writeFileSync(join(dir, 'feedback.md'), 'feedback\n'); + const stub = writeQwenStub(dir, [ + "process.stderr.write('temporary upstream error\\n');", + 'process.exit(1);', + ]); + + const result = runAddressReview(dir, stub); + + expect(result.status).not.toBe(0); + expect(readFileSync(join(dir, 'agent.log'), 'utf8')).toContain( + 'temporary upstream error', + ); + expect(readFileSync(join(dir, 'failure.md'), 'utf8')).toContain( + 'Qwen failed during address-review', + ); + expect(existsSync(join(dir, 'handoff.md'))).toBe(false); + }); + }); + it('preserves agent-written failure details when the qwen subprocess fails', () => { withRunnerDir((dir) => { writeFileSync(join(dir, 'candidates.json'), '[]\n'); @@ -909,14 +1013,60 @@ describe('qwen-autofix workflow', () => { expect(readFileSync(join(dir, 'failure.md'), 'utf8')).toContain( 'agent detail', ); + expect(readFileSync(join(dir, 'handoff.md'), 'utf8')).toContain( + 'human should take over', + ); }); }); it('bounds qwen subprocess runtime', () => { const runner = readFileSync(autofixRunnerScriptPath, 'utf8'); - expect(runner).toContain('const QWEN_TIMEOUT_MS = 50 * 60 * 1000'); - expect(runner).toContain('timeout: QWEN_TIMEOUT_MS'); + expect(runner).toContain('50 * 60 * 1000'); + expect(runner).toContain('setTimeout(() =>'); + expect(runner).toContain("killQwen(child, 'SIGKILL')"); + expect(runner).toContain('}, QWEN_TIMEOUT_MS)'); + }); + + it('kills qwen subprocess descendants on timeout', () => { + withRunnerDir((dir) => { + writeFileSync(join(dir, 'feedback.md'), 'feedback\n'); + const stub = writeQwenStub(dir, [ + "import { spawn } from 'node:child_process';", + "spawn(process.execPath, ['-e', 'setTimeout(() => {}, 3000)'], {", + " stdio: ['ignore', 'inherit', 'inherit'],", + '});', + 'setTimeout(() => process.exit(0), 3000);', + ]); + + const result = spawnSync( + process.execPath, + [ + autofixRunnerScriptPath, + '--mode', + 'address-review', + '--pr', + '5678', + '--issue', + '1234', + '--workdir', + dir, + '--qwen-bin', + stub, + ], + { + encoding: 'utf8', + env: { ...process.env, QWEN_TIMEOUT_MS: '100' }, + timeout: 2000, + }, + ); + + expect(result.error).toBeUndefined(); + expect(result.status).not.toBe(0); + expect(readFileSync(join(dir, 'failure.md'), 'utf8')).toContain( + 'timeout (100ms)', + ); + }); }); it('reports external qwen subprocess signals without calling them timeouts', () => { @@ -976,6 +1126,9 @@ describe('qwen-autofix workflow', () => { expect(readFileSync(join(dir, 'failure.md'), 'utf8')).toContain( 'cannot proceed', ); + expect(readFileSync(join(dir, 'handoff.md'), 'utf8')).toContain( + 'human should take over', + ); }); });