Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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: 94 additions & 0 deletions .github/workflows/qwen-autofix.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2944,6 +2944,51 @@ jobs:
echo '--- feedback.md ---'
cat "${WORKDIR}/feedback.md"

# The agent below runs for up to 80 minutes and the verification gate adds
# more, but nothing reaches the PR thread until "Push and report" at the
# very end: a maintainer who just engaged takeover sees silence and cannot
# tell a working round from a stuck one. The agent's output already
# streams live to the Actions log, so publish that link up front.
# Upserted by marker so one status comment per PR is EDITED each round
# (edits notify nobody) rather than stacking a new comment against a
# 100-round cap. Runs after prepare so a revalidated-away stale duplicate
# never announces a round it will not run. Best-effort: a status post that
# fails warns and continues — it must never cost the round.
- name: 'Post autofix status comment'
id: 'post_status'
if: |-
${{ steps.prepare.outputs.stale != 'true' && needs.route.outputs.dry_run != 'true' }}
env:
GITHUB_TOKEN: '${{ secrets.CI_DEV_BOT_PAT }}'
EFFECTIVE_ROUND: '${{ steps.prepare.outputs.effective_round }}'
RUN_URL: '${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}'
run: |-
set -uo pipefail
MARKER='<!-- autofix-status -->'
ROUND_DISPLAY="${EFFECTIVE_ROUND:-${ROUND}}"
BODY="$(printf '%s\n\n🔄 **AutoFix is working on this PR** — round %s/%s. [Watch live progress](%s); this round posts its report here when it finishes.\n\n<details>\n<summary>中文说明</summary>\n\n🔄 **AutoFix 正在处理此 PR** —— 第 %s/%s 轮。[查看实时进度](%s);本轮结束后会在此发布报告。\n\n</details>' \
"${MARKER}" "${ROUND_DISPLAY}" "${MAX_ROUNDS}" "${RUN_URL}" \
"${ROUND_DISPLAY}" "${MAX_ROUNDS}" "${RUN_URL}")"
STATUS_ID="$(gh api "repos/${REPO}/issues/${PR}/comments" --paginate |

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.

[P2] --paginate fetches all PR comments on every round

The paginated scan pulls every comment on the PR to find the status marker. On a heavily-iterated PR (100 rounds × multiple comments per round), this can mean hundreds of API calls per round just to locate one comment.

No server-side search-by-body exists in the GitHub REST API, so this is the pragmatic choice. Two possible mitigations for the future:

  1. Cache in repo variable/dispatch state: persist the comment_id across rounds so only round 1 needs the scan.
  2. Use search/issue-comments with repo:X in:body autofix-status: the search API supports body filtering, though it has its own rate limits and index lag.

Not blocking — just flagging for awareness on high-round PRs.

jq -rs --arg m "${MARKER}" --arg ab "${AUTOFIX_BOT}" \
'[ .[][] | select((.user.login // "") == $ab)
| select((.body // "") | contains($m)) ] | last | .id // empty')" ||

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.

[P2] contains($m) is substring match — consider startswith($m)

The marker <!-- autofix-status --> is always written at position 0 in the comment body (the printf template starts with ${MARKER}). Using contains($m) technically matches if the marker appears anywhere in the body, which could false-positive on a comment that quotes or references the marker text.

startswith($m) is more precise and costs nothing extra:

| select((.body // "") | startswith($m))

Extremely unlikely to matter in practice given how unique the marker is, but a one-character change for strict correctness.

STATUS_ID=''
if [[ -n "${STATUS_ID}" ]]; then
gh api --method PATCH "repos/${REPO}/issues/comments/${STATUS_ID}" \
-f body="${BODY}" > /dev/null ||
echo "::warning::Failed to update the autofix status comment on PR #${PR}; continuing."
else
STATUS_ID="$(gh api "repos/${REPO}/issues/${PR}/comments" \
-f body="${BODY}" --jq '.id')" ||
{
STATUS_ID=''
echo "::warning::Failed to post the autofix status comment on PR #${PR}; continuing."
}
fi
Comment on lines +2972 to +2988

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] Both "Post" and "Finalize" steps independently fetch all PR comments (gh api --paginate) and scan for the same marker-based comment ID. On a heavily-managed PR with hundreds of comments across up to 100 rounds, this paginated scan runs twice per round for no additional benefit — the Post step already has the answer when its shell exits.

The workflow already passes step outputs via $GITHUB_OUTPUT extensively (37 existing uses). Consider giving this step an id: and writing STATUS_ID to $GITHUB_OUTPUT in both branches (existing-comment PATCH and new-comment POST). The Finalize step can then check ${{ steps.post_status.outputs.comment_id }} first, falling back to the full scan only when that output is empty (which covers the always() case where Post was skipped).

Concrete cost: two paginated API round-trips per round on every managed PR.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Implemented — and taken one step further than suggested.

You are right that the two scans are redundant: the announcement either finds the id or creates it, so it now writes comment_id to $GITHUB_OUTPUT (the POST branch captures a freshly created id via --jq '.id'), and the finalize consumes it.

Where I deviated: the fallback scan is removed outright, not kept. An empty id means this round never announced — its step was skipped, or the post itself failed. In that case no comment claims this round is working, so there is nothing to flip: a previous round's comment is already in a terminal state, and the next round's announcement re-PATCHes it regardless. Keeping a fallback would add a second code path that only ever runs when there is nothing for it to do. So the finalize got shorter, not just cheaper — one scan per round instead of two, and less code.

Pinned by the existing test: comment_id handoff present in both branches, and --paginate asserted absent from the finalize. Mutation-verified — cutting the handoff, or reintroducing a scan in the finalize, each turns that test red.

中文说明

已实现,并且比建议更进一步。

两次扫描确实冗余:公告步骤要么找到 id、要么刚创建了它,因此现在把 comment_id 写入 $GITHUB_OUTPUT(POST 分支用 --jq '.id' 捕获新建 id),finalize 直接消费。

偏离之处:fallback 扫描被彻底删除,而非保留。 id 为空意味着本轮从未公告过(步骤被跳过,或发布本身失败)。此时没有任何评论声称本轮在运行,也就无可翻转:上一轮的评论已是终态,而下一轮的公告无论如何都会重新 PATCH 它。保留 fallback 只会多出一条"仅在无事可做时才执行"的代码路径。所以 finalize 变得更短,而不只是更省 —— 每轮一次扫描而非两次,代码也更少。

已由现有测试钉住:两个分支的 comment_id 交接,以及断言 finalize 中不含 --paginate。变异验证:切断交接、或在 finalize 中重新引入扫描,都会让该测试变红。

# Hand the id to the finalize step so it does not repeat this scan.
echo "comment_id=${STATUS_ID}" >> "${GITHUB_OUTPUT}"

- name: 'Triage and address'
id: 'address'
# Skipped entirely for a stale duplicate target (see the live-watermark
Expand Down Expand Up @@ -3667,3 +3712,52 @@ jobs:
} > "${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

# Flip the status comment out of "working" so a finished round never
# leaves a live-looking line behind. PATCH-only on purpose: a round that
# never posted a status (stale duplicate, dry run) must not gain one here.
# The verdict stays in the round report this job already posts; this only
# records that the round ended, and keeps the run link reachable.
# Gated on 'stale' for the same reason the announcement is: the per-PR
# concurrency group serialises duplicate address jobs, so the discarded
# one runs AFTER the real round already finalised. Ungated, it would
# overwrite that round's "finished" with its own "ended without
# publishing" and report a successful round as a failed one. An empty
# 'stale' (prepare itself crashed) still finalises — that IS this job's
# round, and it is exactly the case that must not stay "working".
- name: 'Finalize autofix status comment'
if: |-
${{ always() && steps.prepare.outputs.stale != 'true' && needs.route.outputs.dry_run != 'true' }}
env:
GITHUB_TOKEN: '${{ secrets.CI_DEV_BOT_PAT }}'
EFFECTIVE_ROUND: '${{ steps.prepare.outputs.effective_round }}'
OUTCOME: '${{ steps.verify.outputs.outcome }}'
RUN_URL: '${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}'
# The id the announcement wrote. Empty means this round never
# announced (its step was skipped, or the post itself failed) — then
# no comment claims this round is working, so there is nothing to
# flip and no reason to scan for one. A previous round's comment is
# already terminal, and the next round's announcement re-PATCHes it.
STATUS_ID: '${{ steps.post_status.outputs.comment_id }}'
run: |-
set -uo pipefail
MARKER='<!-- autofix-status -->'
if [[ -z "${STATUS_ID}" ]]; then
echo "This round posted no status comment on PR #${PR}; nothing to finalize."
exit 0
fi
ROUND_DISPLAY="${EFFECTIVE_ROUND:-${ROUND}}"
# 'fixed'/'noop' are the two outcomes that published a round report;
# anything else means the round stopped before publishing one.
if [[ "${OUTCOME:-}" == 'fixed' || "${OUTCOME:-}" == 'noop' ]]; then

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: OUTCOME=fixed/noop only proves that verification produced a publishable result; it does not prove that the preceding Push and report step actually pushed or posted its comment. For example, a transient git push or gh pr comment failure leaves OUTCOME=fixed, while Report dry-run / failure deliberately suppresses its handoff for fixed/noop. This finalizer then rewrites the live status to “finished / report below” even though no report exists (and possibly no fix was pushed). Please key the success wording on the Push and report step outcome or an explicit published=true output, and add a regression case for the publish-failure path.

EN="$(printf '✅ **AutoFix round %s finished** — [view run](%s). See this round'"'"'s report below.' "${ROUND_DISPLAY}" "${RUN_URL}")"
ZH="$(printf '✅ **AutoFix 第 %s 轮已完成** —— [查看运行](%s)。本轮报告见下方。' "${ROUND_DISPLAY}" "${RUN_URL}")"
else
EN="$(printf '⚠️ **AutoFix round %s ended without publishing a report** — [view run](%s).' "${ROUND_DISPLAY}" "${RUN_URL}")"
ZH="$(printf '⚠️ **AutoFix 第 %s 轮结束但未发布报告** —— [查看运行](%s)。' "${ROUND_DISPLAY}" "${RUN_URL}")"
fi
BODY="$(printf '%s\n\n%s\n\n<details>\n<summary>中文说明</summary>\n\n%s\n\n</details>' \
"${MARKER}" "${EN}" "${ZH}")"
gh api --method PATCH "repos/${REPO}/issues/comments/${STATUS_ID}" \
-f body="${BODY}" > /dev/null ||
echo "::warning::Failed to finalize the autofix status comment on PR #${PR}; continuing."
87 changes: 85 additions & 2 deletions scripts/tests/qwen-autofix-workflow.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,15 @@ const triageAndAddressStep =
)?.[0] ?? '';
const prepareBranchAndFeedbackStep =
workflow.match(
/- name: 'Prepare branch and feedback'[\s\S]*?(?=\n[ ]{6}- name: 'Triage and address')/,
/- name: 'Prepare branch and feedback'[\s\S]*?(?=\n[ ]{6}- name: 'Post autofix status comment')/,
)?.[0] ?? '';
const postStatusCommentStep =
workflow.match(
/- name: 'Post autofix status comment'[\s\S]*?(?=\n[ ]{6}- name: 'Triage and address')/,
)?.[0] ?? '';
const finalizeStatusCommentStep =
workflow.match(
/- name: 'Finalize autofix status comment'[\s\S]*?(?=\n[ ]{6}- name: '|$)/,
)?.[0] ?? '';
const resetAutofixWorkspaceSteps =
workflow.match(
Expand Down Expand Up @@ -293,9 +301,12 @@ describe('qwen-autofix workflow', () => {
// discards itself — no agent run, no marker, no comment.
expect(prepareBranchAndFeedbackStep).toContain('LIVE_EVAL_WM');
expect(prepareBranchAndFeedbackStep).toContain('stale duplicate target');
// Four gates, and both status-comment steps are among them: a discarded
// duplicate must neither announce a round it will never run nor rewrite
// the status the real round already finalised.
expect(
workflow.split("steps.prepare.outputs.stale != 'true'").length - 1,
).toBe(2);
).toBe(4);
expect(reviewScanJob).toContain(
'capture("^review-address \\\\((?<pr>[0-9]+),")',
);
Expand Down Expand Up @@ -4809,6 +4820,78 @@ describe('qwen-autofix workflow', () => {
).toBe(0);
});

it('announces a working round up front and closes the same status comment', () => {
// The whole point: the live run link reaches the thread BEFORE the
// 80-minute agent step, not after it. Without this the PR is silent from
// takeover until "Push and report", so a working round and a stuck one
// look identical.
expect(postStatusCommentStep.length).toBeGreaterThan(0);
expect(postStatusCommentStep).toContain('<!-- autofix-status -->');
expect(postStatusCommentStep).toContain(
'actions/runs/${{ github.run_id }}',
);
expect(postStatusCommentStep).toContain('Watch live progress');
// Announced only for a round that will really run, and never on a dry run.
expect(postStatusCommentStep).toContain(
"steps.prepare.outputs.stale != 'true'",
);
expect(postStatusCommentStep).toContain(
"needs.route.outputs.dry_run != 'true'",
);
// One comment per PR, EDITED each round: a new comment per round would
// stack up to MAX_ROUNDS of them on a managed PR.
expect(postStatusCommentStep).toContain('--method PATCH');
expect(postStatusCommentStep).toContain('contains($m)');
// Best-effort — a failed status post warns and continues, never costs a round.
expect(postStatusCommentStep).toContain('set -uo pipefail');
expect(postStatusCommentStep).toContain('continuing.');
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
expect(finalizeStatusCommentStep).toContain('set -uo pipefail');
expect(finalizeStatusCommentStep).toContain('continuing.');
// Repository convention for anything posted verbatim as a PR comment.
expect(postStatusCommentStep).toContain('<summary>中文说明</summary>');

// Runs on every ending (including a crashed agent) so no finished round
// leaves a live-looking "working" line behind.
expect(finalizeStatusCommentStep.length).toBeGreaterThan(0);
expect(finalizeStatusCommentStep).toContain('always()');
// ...but NOT for a discarded duplicate. The per-PR concurrency group runs
// it after the real round already finalised, so an ungated finalize would
// overwrite that round's "finished" with its own "ended without
// publishing" — reporting a successful round as a failed one.
expect(finalizeStatusCommentStep).toContain(
"steps.prepare.outputs.stale != 'true'",
);
expect(finalizeStatusCommentStep).toContain(
"needs.route.outputs.dry_run != 'true'",
);
expect(finalizeStatusCommentStep).toContain('<!-- autofix-status -->');
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
expect(finalizeStatusCommentStep).toContain('--method PATCH');
expect(finalizeStatusCommentStep).toContain('<summary>中文说明</summary>');
// PATCH-ONLY: a round that never announced (stale duplicate, dry run) must
// not gain a status comment at the end.
expect(finalizeStatusCommentStep).toContain('nothing to finalize');
expect(finalizeStatusCommentStep).not.toContain(
'gh api "repos/${REPO}/issues/${PR}/comments" -f body=',
);
// The announcement hands over the id it just wrote (both branches), so the
// finalize never repeats the paginated comment scan — one scan per round,
// not two, on a PR that can accumulate hundreds of comments over 100 rounds.
expect(postStatusCommentStep).toContain("id: 'post_status'");
expect(postStatusCommentStep).toContain(
'echo "comment_id=${STATUS_ID}" >> "${GITHUB_OUTPUT}"',
);
expect(postStatusCommentStep).toContain("--jq '.id'");
expect(finalizeStatusCommentStep).toContain(
"STATUS_ID: '${{ steps.post_status.outputs.comment_id }}'",
);
expect(finalizeStatusCommentStep).not.toContain('--paginate');
// Tells a round that published a report from one that died before it.
expect(finalizeStatusCommentStep).toContain("== 'fixed'");
expect(finalizeStatusCommentStep).toContain(
'ended without publishing a report',
);
});

it('renders the whole managed fleet into the run summary', () => {
// Diagnosing a stall used to mean listing bot PRs, regexing each one's eval
// markers, and cross-checking checks and fork state by hand - so stalls
Expand Down
Loading