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
This commit is contained in:
易良 2026-07-07 20:27:13 +08:00 committed by GitHub
parent df6be035e1
commit 010bedfc88
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 302 additions and 17 deletions

View file

@ -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 "<!-- autofix-eval ts=${NEWEST} acted=false round=${ROUND} -->"
} > "${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

View file

@ -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);
}

View file

@ -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(
'<!-- autofix-eval ts=${NEWEST} acted=false round=${ROUND} -->',
);
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',
);
});
});