Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
94 changes: 73 additions & 21 deletions .github/workflows/qwen-autofix.yml
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,12 @@ env:
# Hard cap on automated review-address rounds per PR. After this the bot stops
# and leaves the PR for a human.
MAX_ROUNDS: '5'
# An auth/access model error (401/402/403, "no access"/"does not exist")
# never self-heals - only a maintainer can fix the key - and every retry
# costs an agent run AND a PR comment. Cap those attempts far below
# MAX_ROUNDS so the actionable "check the model key" message lands in an
# hour instead of a day. Transient (429/5xx) errors keep the full budget.
API_AUTH_MAX_ROUNDS: '3'
# Upper bound on review targets emitted per scan (fan-out defense-in-depth;
# excess is logged and deferred to the next scan).
MAX_TARGETS_PER_SCAN: '10'
Expand Down Expand Up @@ -2500,7 +2506,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 agent-api-error agent-api-error-kind pr.diff; do
if [[ -f "${WORKDIR}/${f}" ]]; then
echo "=============== ${f} ==============="
cat "${WORKDIR}/${f}"
Expand Down Expand Up @@ -2712,6 +2718,30 @@ jobs:
for f in failure.md handoff.md address-summary.md no-action.md; do
if [[ -s "${WORKDIR}/${f}" ]]; then DETAIL_FILE="${WORKDIR}/${f}"; break; fi
done
# run-agent.mjs drops this marker when qwen died on a model-side
# [API Error: 4xx/5xx] (access denied, quota, a 5xx). The agent never
# evaluated the feedback, so this is treated as a retry (sentinel ts,
# no watermark advance) below — a model access/quota blip must not
# strand the PR the way a real evaluated handoff would.
API_ERROR_DETAIL=''
API_ERROR_KIND=''
if [[ -s "${WORKDIR}/agent-api-error" ]]; then
# First line only, comment-opener escaped (agent stdout can echo
# external PR-comment text and the marker regex spans '<!-- ... -->'
# happily), and capped so a long span can't bloat the headline.
# `cut -c` counts BYTES, so the cap can split a multi-byte
# character - and the classifier deliberately matches CJK renders,
# so a >200-byte Chinese error is a supported input, not a
# hypothetical. iconv -c drops the dangling bytes so the headline
# stays valid UTF-8; it EXITS 1 when it discards one, which under
# this step's `set -eo pipefail` would abort before the marker and
# the gh pr comment - hence the `|| true`, same as the sibling
# publish site below.
API_ERROR_DETAIL="$(head -n 1 "${WORKDIR}/agent-api-error" | sed 's/<!--/<!\\-\\-/g' | cut -c1-200 | iconv -f utf-8 -t utf-8 -c || true)"
fi
if [[ -s "${WORKDIR}/agent-api-error-kind" ]]; then
API_ERROR_KIND="$(head -n 1 "${WORKDIR}/agent-api-error-kind" | tr -cd 'a-z')"
fi
# If feedback was actually read (prepare ran), stamp its newest ts so
# the watermark advances and the same feedback is not re-selected next
# scan. If the crash happened before prepare, NEWEST is empty and the
Expand All @@ -2727,28 +2757,50 @@ jobs:
MARK_TS="${NEWEST:-${WATERMARK:-9999-12-31T23:59:59Z}}"
if [[ -n "${NEWEST:-}" ]]; then
MARK_ROUND="$(( ROUND + 1 ))"
if [[ -z "${DETAIL_FILE}" ]]; then
# Prepare ran (NEWEST is set) but the agent produced NO output
# at all — it crashed before writing any verdict (e.g. a staged
# runner that fails to boot). It evaluated NOTHING, so the
# watermark must NOT advance past this feedback: an advance makes
# the next scan see "nothing new" and never retry, stranding the
# PR on a transient crash (a deploy, an infra blip, a base-image
# bug fixed minutes later). Stamp the sentinel ts (excluded from
# EVAL_WM) so the feedback stays live and the next scan retries;
# the incremented round still bounds retries to MAX_ROUNDS before
# a terminal handoff, so a PERSISTENT crash cannot loop forever.
if [[ -z "${DETAIL_FILE}" || -n "${API_ERROR_DETAIL}" ]]; then
# Either the agent produced NO output (crashed before any verdict
# — a staged runner that fails to boot) OR it died on a model
# [API Error] (access/quota/5xx). In BOTH cases it evaluated
# NOTHING, so the watermark must NOT advance past this feedback:
# an advance makes the next scan see "nothing new" and never
# retry, stranding the PR on a transient failure (an infra blip,
# a quota reset minutes away, a model-access grant). Stamp the
# sentinel ts (excluded from EVAL_WM) so the feedback stays live
# and the next scan retries; the incremented round still bounds
# retries to MAX_ROUNDS before a terminal handoff.
MARK_TS='9999-12-31T23:59:59Z'
# Only promise a retry when one will actually happen: at
# MARK_ROUND == MAX_ROUNDS the next scan's round-cap gate skips
# the PR, and the cap-reached notice is takeover-only — so the
# final attempt must say so itself, or the maintainer waits for
# a retry that never comes. No Run log here: the report block
# below appends it to every handoff (avoid a duplicate URL).
if [[ "${MARK_ROUND}" -lt "${MAX_ROUNDS}" ]]; then
HEADLINE="🤖 AutoFix crashed before it could evaluate the feedback (attempt ${MARK_ROUND}/${MAX_ROUNDS}) — it will retry on the next scan."
# Cause-aware wording: a model error names the model issue and
# the operator fix; a bare crash points at a human. No Run log
# here — the report block below appends it (avoid a duplicate).
if [[ -n "${API_ERROR_DETAIL}" ]]; then
CAUSE="could not reach the model — ${API_ERROR_DETAIL}"
LAST_FIX="a maintainer should check the autofix model key/access, then re-arm"
else
CAUSE="crashed before it could evaluate the feedback"
LAST_FIX="a human should take over this PR"
fi
# Budget retries by CAUSE. A transient 429/5xx self-heals, so
# it keeps the full round budget. An auth/access error that
# only a maintainer can fix never self-heals, and every attempt
# costs an agent run AND a PR comment - on a takeover PR that
# is ~100 comments over ~17h before the useful message lands.
CAUSE_MAX="${MAX_ROUNDS}"
if [[ "${API_ERROR_KIND}" == 'auth' && "${API_AUTH_MAX_ROUNDS}" -lt "${MAX_ROUNDS}" ]]; then
CAUSE_MAX="${API_AUTH_MAX_ROUNDS}"
fi
Comment on lines +3068 to +3071

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 auth-specific reduced retry cap (API_AUTH_MAX_ROUNDS) has no workflow-level test. runMark and runHeadline pass API_ERROR_DETAIL but never API_ERROR_KIND or API_AUTH_MAX_ROUNDS, so the CAUSE_MAX branching, the MARK_ROUND="${MAX_ROUNDS}" terminal-round override, and the attempt N/3 headline denominator are unexercised by the bash-replay tests.

Concrete cost: a bug in CAUSE_MAX comparison, the override, or the headline template ships undetected — the auth cap silently falls back to the full 5-round budget, producing ~100 PR comments over ~17h instead of the intended ~6 over ~3h.

Suggested change
CAUSE_MAX="${MAX_ROUNDS}"
if [[ "${API_ERROR_KIND}" == 'auth' && "${API_AUTH_MAX_ROUNDS}" -lt "${MAX_ROUNDS}" ]]; then
CAUSE_MAX="${API_AUTH_MAX_ROUNDS}"
fi
# Add two cases to the runMark / runHeadline section:
# 1. runMark({ ROUND: '1', ..., API_ERROR_KIND: 'auth', API_AUTH_MAX_ROUNDS: '3' })
# → expect SENTINEL|2 (mid-budget retry, CAUSE_MAX=3)
# 2. runMark({ ROUND: '2', ..., API_ERROR_KIND: 'auth', API_AUTH_MAX_ROUNDS: '3' })
# → expect SENTINEL|5 (terminal auth cap, MARK_ROUND overridden to MAX_ROUNDS)
# Plus a runHeadline case asserting "attempt 3/3" in the output.

— qwen3.7-max via Qwen Code /review

# Only promise a retry when one will actually happen: at the
# cap the next scan's round gate skips the PR, and the
# cap-reached notice is takeover-only, so the final attempt
# must say so itself.
if [[ "${MARK_ROUND}" -lt "${CAUSE_MAX}" ]]; then
HEADLINE="🤖 AutoFix ${CAUSE} (attempt ${MARK_ROUND}/${CAUSE_MAX}) — it will retry on the next scan."
else
HEADLINE="🤖 AutoFix crashed before it could evaluate the feedback (attempt ${MARK_ROUND}/${MAX_ROUNDS}) — this was the last automatic attempt; a human should take over this PR."
HEADLINE="🤖 AutoFix ${CAUSE} (attempt ${MARK_ROUND}/${CAUSE_MAX}) — this was the last automatic attempt; ${LAST_FIX}."
# Stamp the terminal round so the scan's max-round gate skips
# this PR instead of re-handing-off every tick; the sentinel
# ts keeps the feedback live for a re-arm once the key is
# fixed. (No-op when CAUSE_MAX is already MAX_ROUNDS.)
MARK_ROUND="${MAX_ROUNDS}"
fi
else
HEADLINE="🤖 Could not address the latest feedback automatically (round ${MARK_ROUND}/${MAX_ROUNDS}). A human should take over this PR."
Expand Down
84 changes: 83 additions & 1 deletion .qwen/skills/autofix/scripts/run-agent.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,63 @@ function writeFailure(workdir, message) {
);
}

// Classify a model-side [API Error] render as recoverable, and by CAUSE:
// 'transient' - 429 / 5xx / rate-limit / overload / quota: self-heals on its
// own once the limit resets, so it earns the full retry budget.
// 'auth' - 401 / 402 / 403, or a render saying the model does not exist
// or the key has no access: ONLY a maintainer can fix it, so
// the workflow caps these retries low and then goes terminal
// with the operator fix (each attempt costs an agent run AND a
// PR comment; a hundred of them help nobody).
// '' - terminal: reproduces identically forever (a malformed 400).
//
// The status code is read from its POSITION in the render (`[API Error: <code>`)
// rather than matched anywhere in the text. Matching anywhere made permanent
// failures look retryable: `400 Invalid value for max_tokens: must be <= 512`
// matched a bare \b5\d\d\b, and `400 context length exceeded` matched a bare
// `exceeded` - both reproduce forever. `exceeded` therefore only counts as part
// of `quota`. The match is single-line ([^\]\n]) so a multi-line render cannot
// smuggle a newline into the marker or the PR-comment headline.
const TRANSIENT_API_ERROR =
/rate.?limit|quota|RESOURCE_EXHAUSTED|overloaded|temporarily|too many requests|速率限制|配额|服务(?:繁忙|不可用)/i;
// "does not exist or you do not have access to it" is the OpenAI-compatible
// render of the same condition a 403 reports - same root cause, same fix.
const AUTH_API_ERROR =
/api key|do not have access|does not exist|unauthorized|forbidden/i;

function classifyApiError(render) {
const code = render.match(/\[API Error:\s*(\d{3})\b/)?.[1];
if (code) {
const status = Number(code);
if (status === 429 || (status >= 500 && status <= 599)) return 'transient';
if (status === 401 || status === 402 || status === 403) return 'auth';
// Any other code (400, 404, ...) is permanent UNLESS the message itself
// names an access/existence problem.
return AUTH_API_ERROR.test(render) ? 'auth' : '';

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] AUTH_API_ERROR includes does not exist as a keyword, but this fires for 400 status codes too via the catch-all return AUTH_API_ERROR.test(render) ? 'auth' : '' on line 102. A 400 error whose message says "does not exist" in a non-access context — e.g. [API Error: 400 Tool 'web_search' does not exist] or [API Error: 400 Field 'temperature' does not exist in schema] — is classified as 'auth', triggering 3 auth-capped retries with the misleading "check the autofix model key/access" headline. A 400 Bad Request reproduces identically on every retry; each attempt costs an agent run and a PR comment.

The two existing 400 tests (max_tokens: must be <= 512 and context length exceeded: 40000 > 32768) don't carry "does not exist" so they don't expose this.

Failure scenario: model returns [API Error: 400 Tool 'web_search' does not exist] → classified auth → 3 wasted retries + 3 misleading PR comments → terminal after ~30 min instead of immediate.

Suggested change
// Any other code (400, 404, ...) is permanent UNLESS the message itself
// names an access/existence problem.
return AUTH_API_ERROR.test(render) ? 'auth' : '';
// 400 is always a malformed client request — it never self-heals by retry,
// regardless of what the message says. Route it terminal unconditionally
// so a "does not exist" in the body (a tool name, a field name) cannot
// trigger the auth/access retry path.
if (status === 400) return '';
return AUTH_API_ERROR.test(render) ? 'auth' : '';

— qwen3.7-max via Qwen Code /review

}
// Code-less render: fall back to the keyword arms.
if (AUTH_API_ERROR.test(render)) return 'auth';
return TRANSIENT_API_ERROR.test(render) ? 'transient' : '';
}

// Returns { error, kind }; error is '' when nothing recoverable was found.
// NOTE: the caller passes the last 20 KB of output, so an API error emitted
// early in a long run can scroll out and be classified terminal. That is the
// fail-safe direction (a missed retry, never a wrongly-retried permanent
// failure), but detection is best-effort rather than guaranteed.
function recoverableApiError(output) {
const wrapped = output.match(/\[API Error:[^\]\n]*\]/g) || [];
for (const render of wrapped.reverse()) {
const kind = classifyApiError(render);
if (kind) return { error: render, kind };
}
// Some quota errors are never wrapped in [API Error: ...] (e.g. Qwen OAuth
// quota returns early before formatting) - catch the known standalone form.
const oauth = output.match(/Qwen OAuth quota exceeded[^\n]*/i);
if (oauth) return { error: `[API Error: ${oauth[0]}]`, kind: 'transient' };
return { error: '', kind: '' };
}

function writeHandoff(workdir, message) {
mkdirSync(workdir, { recursive: true });
writeFileSync(file(workdir, 'handoff.md'), `${message}\n`);
Expand Down Expand Up @@ -111,10 +168,15 @@ function runQwen(options, prompt) {
settled = true;
clearTimeout(timer);
clearTimeout(killTimer);
const apiErrorInfo = recoverableApiError(outputTail);
const payload = {
...result,
timedOut,
loopDetected: loopDetected || isLoopGuardOutput(outputTail),
// A RECOVERABLE model error means qwen never evaluated the feedback —
// the workflow retries it rather than advancing the watermark.
apiError: apiErrorInfo.error,
apiErrorKind: apiErrorInfo.kind,
};
if (log.destroyed) {
resolve(payload);
Expand Down Expand Up @@ -227,8 +289,28 @@ if (result.error || result.signal || result.status !== 0) {
} else {
writeFailure(
options.workdir,
`Qwen failed during ${options.mode}: ${detail}.`,
`Qwen failed during ${options.mode}: ${detail}.${
result.apiError ? ` ${result.apiError}` : ''
}`,
);
// Only a BARE, un-evaluated API failure is retryable. A loop guard, a
// timeout, or an agent-written failure.md is a real verdict — leave
// those terminal even if an API-error string appears in the output tail.
// Signals the workflow to retry (sentinel ts) instead of advancing the
// watermark and stranding the PR.
if (result.apiError && !result.timedOut) {
writeFileSync(
file(options.workdir, 'agent-api-error'),
`${result.apiError}\n`,
);
// Cause class ("transient" | "auth") — the handoff step gives a
// self-healing transient error the full round budget, but caps an
// auth/access error that only a maintainer can fix.
writeFileSync(
file(options.workdir, 'agent-api-error-kind'),
`${result.apiErrorKind}\n`,
);
}
Comment thread
wenshao marked this conversation as resolved.
Resolved
}
} else {
writeHandoff(
Expand Down
Loading
Loading