Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
154 changes: 138 additions & 16 deletions .github/scripts/autofix-push-and-report.sh
Original file line number Diff line number Diff line change
Expand Up @@ -118,17 +118,73 @@ fi
# replies — silence in still-open threads was a no-op-only gap.
resolve_and_reply_threads() {
CAN_RESOLVE_THREADS='false'
# Round-report observability (#10106): a guard refusing round after
# round reads, on the PR, exactly like resolution working — 0/90 on
# #9729 stayed invisible for days. Each refusing guard records its
# name, and the counters feed one host-authored line in the round
# report; the ::warning:: lines below reach only the run log.
RESOLUTION_GUARD=''
RESOLUTION_SELECTED_N=0
CONFIRMED_RESOLVED_N=0
RESOLVED_BY_OTHERS_N=0
ALREADY_RESOLVED_N=0
if [[ -s "${WORKDIR}/resolved-comments.txt" ]]; then
LOCAL_PUSHED_HEAD="$(git rev-parse HEAD)"
if [[ "${PUSH_RACE_MERGED}" == 'true' ]]; then
echo "::warning::skipping review-thread resolution because the pushed head includes commits merged after deterministic verification"
elif [[ -z "${VERIFIED_HEAD}" || "${LOCAL_PUSHED_HEAD}" != "${VERIFIED_HEAD}" ]]; then
echo "::warning::skipping review-thread resolution because the pushed head is not the exact deterministically verified commit"
elif LIVE_PR_HEAD="$(gh pr view "${PR}" --repo "${REPO}" --json headRefOid --jq '.headRefOid // ""' 2> /dev/null)" &&
[[ -n "${LIVE_PR_HEAD}" && "${LIVE_PR_HEAD}" == "${VERIFIED_HEAD}" ]]; then
CAN_RESOLVE_THREADS='true'
else
echo "::warning::skipping review-thread resolution because the live PR head could not be proven equal to the deterministically verified commit"
# One spelling of the id grammar (optional rc: prefix, optional
# trailing CR, digits only, deduplicated), shared by the counter
# and the resolve loop below.
RESOLVED_IDS="$(sed 's/\r$//; s/^rc://' "${WORKDIR}/resolved-comments.txt" | grep -E '^[0-9]+$' | sort -u || true)"
RESOLUTION_SELECTED_N="$(grep -c . <<< "${RESOLVED_IDS}" || true)"
# Nothing selected resolves nothing, so the head guards below have
# no resolution to gate — a malformed file (zero valid ids) must
# not spend this PAT-bearing step's reads on a proof nothing needs.
if [[ "${RESOLUTION_SELECTED_N}" -gt 0 ]]; then
LOCAL_PUSHED_HEAD="$(git rev-parse HEAD)"
if [[ "${PUSH_RACE_MERGED}" == 'true' ]]; then
RESOLUTION_GUARD='salvage merge'
echo "::warning::skipping review-thread resolution because the pushed head includes commits merged after deterministic verification"
elif [[ -z "${VERIFIED_HEAD}" ]]; then
RESOLUTION_GUARD='missing verified_head'
echo "::warning::skipping review-thread resolution because this round recorded no deterministically verified commit"
elif [[ "${LOCAL_PUSHED_HEAD}" != "${VERIFIED_HEAD}" ]]; then
RESOLUTION_GUARD='verified_head mismatch'
echo "::warning::skipping review-thread resolution because the pushed head is not the exact deterministically verified commit"
else
# The PR read model is eventually consistent: a headRefOid read
# seconds after this round's OWN push routinely still returns the
# previous head — on #9729 every pushed round tripped this guard
# that way, silently, for days (#10106). Give propagation a
# bounded window before declaring drift; the per-mutation guards
# below stay single-shot, because once the head was observed
# equal a later mismatch means it actually moved. A round that
# pushed nothing has no push to propagate — a mismatched head
# there moves only further away, never back — so one read
# decides.
# The delay knob exists for tests; anything but a single digit
# (e.g. a GITHUB_ENV plant stalling this PAT-bearing step) falls
# back to the default.
[[ "${LIVE_HEAD_RETRY_DELAY:-}" =~ ^[0-9]$ ]] || LIVE_HEAD_RETRY_DELAY=5
LIVE_HEAD_ATTEMPTS=5
[[ "${ROUND_PUSHED:-}" == 'true' ]] || LIVE_HEAD_ATTEMPTS=1
LIVE_HEAD_EVER_READ='false'
for (( live_head_attempt = 1; live_head_attempt <= LIVE_HEAD_ATTEMPTS; live_head_attempt++ )); do
LIVE_PR_HEAD="$(gh pr view "${PR}" --repo "${REPO}" --json headRefOid --jq '.headRefOid // ""' 2> /dev/null)" || LIVE_PR_HEAD=''
if [[ -n "${LIVE_PR_HEAD}" ]]; then
LIVE_HEAD_EVER_READ='true'
if [[ "${LIVE_PR_HEAD}" == "${VERIFIED_HEAD}" ]]; then
CAN_RESOLVE_THREADS='true'
break
fi
fi
[[ "${live_head_attempt}" == "${LIVE_HEAD_ATTEMPTS}" ]] || sleep "${LIVE_HEAD_RETRY_DELAY}"
done
if [[ "${CAN_RESOLVE_THREADS}" != 'true' ]]; then
# drift: a head was read but never matched; unreadable: no read
# returned any head (auth/API health, not a contributor push).
RESOLUTION_GUARD='live-head drift'
[[ "${LIVE_HEAD_EVER_READ}" == 'true' ]] || RESOLUTION_GUARD='live-head unreadable'
echo "::warning::skipping review-thread resolution because the live PR head could not be proven equal to the deterministically verified commit"
fi
fi
fi
fi
# Resolve the review threads whose findings the agent actually
Expand Down Expand Up @@ -170,34 +226,63 @@ resolve_and_reply_threads() {
fi
fi
if [[ "${CAN_RESOLVE_THREADS}" == 'true' ]]; then
CONFIRMED_RESOLVED_N=0
read_thread_guard() {
gh api graphql -f owner="${REPO%%/*}" -f name="${REPO##*/}" -F pr="${PR}" -f threadId="${1}" -f query='
query($owner:String!,$name:String!,$pr:Int!,$threadId:ID!){
repository(owner:$owner,name:$name){pullRequest(number:$pr){headRefOid}}
node(id:$threadId){... on PullRequestReviewThread{isResolved}}
}' --jq '[.data.repository.pullRequest.headRefOid // "", .data.node.isResolved] | @tsv'
}
# A thread can carry more than one selected id — the feedback
# renderer lists a reply under a Critical root as its own finding —
# so the loop tracks the threads it already handled and counts
# THREADS, not ids: a second id of a handled thread must not
# re-resolve it nor read the loop's own resolution as "another
# actor".
SEEN_THREAD_IDS=''
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
# A file with no valid ids normalizes to an empty list; the
# here-string still yields one empty iteration.
[[ -n "${rc_id}" ]] || continue
thread_id="$(jq -r --argjson id "${rc_id}" \
'map(select(.isResolved | not)
| select(any(.comments.nodes[]; .databaseId == $id)))
| .[0].id // ""' <<< "${THREADS_JSON}")"
thread_open='true'
if [[ -z "${thread_id}" ]]; then
echo "::warning::comment ${rc_id} matched no open review thread"
thread_open='false'
# The open-thread filter above hides an id whose thread was
# resolved BEFORE the fetch; find it anyway, or it stays in
# the residual count every round — the SKILL has the agent
# re-list a still-holding fix, so the count never converges.
thread_id="$(jq -r --argjson id "${rc_id}" \
'map(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
fi
if grep -qxF "${thread_id}" <<< "${SEEN_THREAD_IDS}"; then
RESOLUTION_SELECTED_N=$(( RESOLUTION_SELECTED_N - 1 ))
continue
fi
SEEN_THREAD_IDS="${SEEN_THREAD_IDS}${thread_id}"$'\n'
if [[ "${thread_open}" == 'false' ]]; then
ALREADY_RESOLVED_N=$(( ALREADY_RESOLVED_N + 1 ))
continue
fi
if ! IFS=$'\t' read -r LIVE_PR_HEAD THREAD_IS_RESOLVED < <(read_thread_guard "${thread_id}" 2> /dev/null) ||
[[ -z "${LIVE_PR_HEAD}" || "${LIVE_PR_HEAD}" != "${VERIFIED_HEAD}" ]]; then
RESOLUTION_GUARD='live-head drift'
echo "::warning::stopping review-thread resolution because the live PR head moved before resolving comment ${rc_id}"
break
elif [[ "${THREAD_IS_RESOLVED}" == 'true' ]]; then
RESOLVED_BY_OTHERS_N=$(( RESOLVED_BY_OTHERS_N + 1 ))
echo "::warning::comment ${rc_id} was resolved by another actor before this round could resolve it"
continue
elif [[ "${THREAD_IS_RESOLVED}" != 'false' ]]; then
RESOLUTION_GUARD='thread state unproven'
echo "::warning::stopping review-thread resolution because the state of comment ${rc_id} could not be proven"
break
fi
Expand All @@ -220,12 +305,39 @@ resolve_and_reply_threads() {
elif [[ "${POST_GUARD_OK}" == 'true' && "${LIVE_PR_HEAD}" == "${VERIFIED_HEAD}" && "${THREAD_IS_RESOLVED}" == 'false' && "${RESOLVE_SUCCEEDED}" == 'false' ]]; then
echo "::warning::could not resolve the review thread for comment ${rc_id}"
else
RESOLUTION_GUARD='mutation post-check ambiguous'
echo "::warning::the live PR head or thread state could not be proven after resolving comment ${rc_id}; stopping review-thread resolution"
break
fi
done < "${WORKDIR}/resolved-comments.txt"
done <<< "${RESOLVED_IDS}"
echo "🧵 confirmed ${CONFIRMED_RESOLVED_N} selected review thread(s) resolved while the verified head remained live"
fi
# One host-authored line for the round report (#10106): name the
# refusing guard and count the threads left behind. All text is fixed
# host strings plus counts — nothing agent-controlled.
RESOLUTION_NOTE=''
if [[ "${RESOLUTION_SELECTED_N}" -gt 0 ]]; then
RESOLUTION_LEFT_N=$(( RESOLUTION_SELECTED_N - CONFIRMED_RESOLVED_N - RESOLVED_BY_OTHERS_N - ALREADY_RESOLVED_N ))
if [[ -n "${RESOLUTION_GUARD}" ]]; then
RESOLUTION_PHASE='skipped'
RESOLUTION_PHASE_ZH='被跳过'
if [[ "${CAN_RESOLVE_THREADS}" == 'true' ]]; then
RESOLUTION_PHASE='stopped early'
RESOLUTION_PHASE_ZH='提前中止'
fi
RESOLUTION_NOTE="⚠️ Review-thread resolution ${RESOLUTION_PHASE} — guard: \`${RESOLUTION_GUARD}\`; resolved ${CONFIRMED_RESOLVED_N} of ${RESOLUTION_SELECTED_N} selected thread(s), ${RESOLUTION_LEFT_N} left for a later round. · 评审线程关闭${RESOLUTION_PHASE_ZH}——守卫:\`${RESOLUTION_GUARD}\`;选中 ${RESOLUTION_SELECTED_N} 条,本轮关闭 ${CONFIRMED_RESOLVED_N} 条,其余 ${RESOLUTION_LEFT_N} 条留待后续轮次。"
elif [[ "${RESOLUTION_LEFT_N}" -gt 0 ]]; then
RESOLUTION_DETAIL='details in the run log'
RESOLUTION_DETAIL_ZH='详见运行日志'
if [[ "${THREADS_FETCH_OK:-true}" != 'true' ]]; then
RESOLUTION_DETAIL='thread fetch incomplete; details in the run log'
RESOLUTION_DETAIL_ZH='线程拉取不完整,详见运行日志'
fi
RESOLUTION_NOTE="🧵 Resolved ${CONFIRMED_RESOLVED_N} of ${RESOLUTION_SELECTED_N} selected review thread(s); ${RESOLUTION_LEFT_N} not resolved by this round (${RESOLUTION_DETAIL}). · 选中评审线程 ${RESOLUTION_SELECTED_N} 条,已关闭 ${CONFIRMED_RESOLVED_N} 条;其余 ${RESOLUTION_LEFT_N} 条本轮未关闭(${RESOLUTION_DETAIL_ZH})。"
else
RESOLUTION_NOTE="🧵 Resolved all ${RESOLUTION_SELECTED_N} selected review thread(s). · 已关闭全部选中的 ${RESOLUTION_SELECTED_N} 条评审线程。"
fi
fi
# The mirror of the resolve above: a finding the agent did NOT
# resolve keeps its thread open, and this answers it IN that thread.
# Full rationale → qwen-autofix.md#af-132
Expand Down Expand Up @@ -448,6 +560,7 @@ if [[ "${OUTCOME}" == "fixed" ]]; then
PUSH_RACE_MERGED='true'
fi
done
ROUND_PUSHED='true'
resolve_and_reply_threads
# Best-effort: verified out-of-footprint findings persist into
# the per-PR tracking issue (script content from expression
Expand Down Expand Up @@ -484,6 +597,10 @@ if [[ "${OUTCOME}" == "fixed" ]]; then
echo
echo "⚠️ The branch received new commits while this round ran; they were merged into this push, but this round's verification predates that merge — re-check anything that landed mid-run. · 本轮运行期间分支收到了新的提交;本次推送已将其合并,但本轮验证在合并之前完成——请复查运行期间落地的改动。"
fi
if [[ -n "${RESOLUTION_NOTE}" ]]; then
echo
echo "${RESOLUTION_NOTE}"
fi
echo
echo "Re-review when you have a moment. After round ${MAX_ROUNDS} this bot stops and leaves the PR for a human. · 有空请复审;第 ${MAX_ROUNDS} 轮后本 bot 停止并将 PR 交给人工。"
echo
Expand All @@ -507,6 +624,7 @@ else
# No push happened, so the verified head is the unchanged
# origin head; resolution's own live-head guards still apply.
PUSH_RACE_MERGED='false'
ROUND_PUSHED='false'
resolve_and_reply_threads
# Best-effort: verified out-of-footprint findings persist into
# the per-PR tracking issue (script content from expression
Expand All @@ -524,6 +642,10 @@ else
fi
echo
echo "Base-conflict check · 基分支冲突检查: $([[ "${CONFLICT}" == "true" ]] && echo 'conflicts with main (no review fix needed, but a rebase/merge is required before merge). · 与 main 有冲突(无需评审修复,但合并前需 rebase/merge)。' || echo 'no conflict with main. · 与 main 无冲突。')"
if [[ -n "${RESOLUTION_NOTE}" ]]; then
echo
echo "${RESOLUTION_NOTE}"
fi
echo
echo "---"
echo "🧠 Handled by **Qwen Code** · model/模型 \`${MODEL_DISPLAY}\`"
Expand Down
5 changes: 5 additions & 0 deletions docs/design/autofix-resolve-fixed-review-threads.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,16 @@ If a pre-mutation condition is unknown or false, or a post-mutation condition is

The verified code push and normal round report still succeed. Replies for findings deliberately left open may continue after a successful push because they do not assert that a thread is fixed.

### Round-report observability

Fail closed must not mean fail silent (issue #10106: 0 of 90 threads resolved on PR #9729 across ~16 rounds, with every refusal visible only as run-log warnings). Every guard that refuses records a stable name — `salvage merge`, `missing verified_head`, `verified_head mismatch`, `live-head drift` or `live-head unreadable` for the up-front skips; `live-head drift`, `thread state unproven`, `mutation post-check ambiguous` for a mid-list stop — and the round report posted on the PR carries one host-authored line naming that guard and counting the selected, resolved, and left-behind threads. Rounds with no refusing guard report the positive count (including per-thread misses and an incomplete thread fetch), so a healthy mechanism is visible by the same line whose numbers go wrong when it dies. The counts are thread-accurate: two selected ids that sit in one thread count once, and an id whose thread was already resolved before the fetch is subtracted from the residual instead of being reported left behind — a re-listed still-holding fix must not keep the residual above zero forever. The line is composed exclusively from fixed host strings and counters; no agent-authored content enters it.

## Design decisions

- **Fail closed for resolution:** an unresolved thread is recoverable; an incorrectly resolved thread can hide a real defect.
- **Skip resolution after race merge:** rerunning the full deterministic gate inside the PAT-bearing publish step would duplicate expensive logic and run branch-controlled scripts with credentials in scope. A later review round can safely resolve the thread.
- **Query live PR state immediately before mutation:** workflow concurrency cannot prevent direct contributor pushes.
- **Bounded wait for the initial live-head equality:** the PR read model is eventually consistent, and a `headRefOid` read seconds after this round's own push routinely still returns the previous head — on PR #9729 every pushed round declared drift on that single stale read and silently skipped (issue #10106). The initial equality check retries briefly before declaring drift, and names the refusal `live-head unreadable` rather than `live-head drift` when no attempt returned any head, so an expired PAT or an API outage reads differently from a contributor push. The per-mutation guards stay single-shot: once the head has been observed equal, a later mismatch means it actually moved. The window runs only when the round pushed and selected at least one thread: its premise is THIS round's own push propagating, and a round that pushed nothing — or selected nothing to resolve — has no head proof to run.
- **Keep the existing model disposition contract:** semantic judgment remains with the agent, while exact commit identity is enforced deterministically by the host.
- **Do not add general CLI/core code:** this is Autofix workflow orchestration, not a reusable Qwen Code runtime feature.

Expand Down
Loading
Loading