From 8efdf749ad0e82acef04dc57cb2589d0eb4cb2bc Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Sat, 1 Aug 2026 00:04:30 +0800 Subject: [PATCH] fix(autofix): guard review thread resolution (#8231) Co-authored-by: Qwen-Coder --- .../run-autofix-review-verification.sh | 18 + .github/workflows/qwen-autofix.yml | 68 ++- .qwen/skills/autofix/SKILL.md | 12 +- .../autofix-resolve-fixed-review-threads.md | 95 +++++ scripts/tests/qwen-autofix-workflow.test.js | 388 +++++++++++++++--- 5 files changed, 517 insertions(+), 64 deletions(-) create mode 100644 docs/design/autofix-resolve-fixed-review-threads.md diff --git a/.github/scripts/run-autofix-review-verification.sh b/.github/scripts/run-autofix-review-verification.sh index 8a9a0c9bb6..b499dbfee9 100755 --- a/.github/scripts/run-autofix-review-verification.sh +++ b/.github/scripts/run-autofix-review-verification.sh @@ -68,6 +68,21 @@ run_check() { reject_fix "${label}" fi } +assert_verification_tree() { + if [[ "$(git rev-parse HEAD)" != "${VERIFICATION_HEAD}" ]]; then + reject_fix 'HEAD changed during deterministic verification' + fi + if [[ -n "$(git status --porcelain)" ]]; then + git status --short >> "${GATE_LOG}" + reject_fix 'workspace became dirty during deterministic verification' + fi +} + +if [[ -n "$(git status --porcelain)" ]]; then + git status --short >> "${GATE_LOG}" + reject_fix 'workspace is dirty before deterministic verification' +fi +VERIFICATION_HEAD="$(git rev-parse HEAD)" # Settings-schema freshness is a STRUCTURAL guard, checked BEFORE the # no-op/unchanged return: on a stale-schema PR the agent can wrongly @@ -87,6 +102,7 @@ run_check 'settings schema is stale on the agent-committed fix' \ CHANGED_FILES="$(git diff --name-only "origin/main...${BRANCH}")" run_check 'cross-package contract verification failed' \ bash "${RUNNER_TEMP}/check-autofix-contracts.sh" <<< "${CHANGED_FILES}" +assert_verification_tree if git diff --quiet "origin/${BRANCH}...${BRANCH}"; then # No new commit. That is only legitimate as a deliberate no-action. @@ -145,4 +161,6 @@ else npm run test --workspace "${p}" --if-present -- --changed origin/main --passWithNoTests done fi +assert_verification_tree +echo "verified_head=${VERIFICATION_HEAD}" >> "${GITHUB_OUTPUT}" echo "outcome=fixed" >> "${GITHUB_OUTPUT}" diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index 6f1a2054cb..e862c80022 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -3540,7 +3540,9 @@ jobs: "${WORKDIR}/gate-rejection.md" \ "${WORKDIR}/agent-api-error" \ "${WORKDIR}/agent-api-error-kind" \ - "${WORKDIR}/agent-timeout" + "${WORKDIR}/agent-timeout" \ + "${WORKDIR}/resolved-comments.txt" \ + "${WORKDIR}/comment-replies.json" rm -rf "${QWEN_HOME}" mkdir -p .qwen "${QWEN_HOME}" printf '%s\n' "${SETTINGS_JSON}" > .qwen/settings.json @@ -3568,20 +3570,27 @@ jobs: env: FIRST_OUTCOME: '${{ steps.verify.outputs.outcome }}' FIRST_COMMITTED: '${{ steps.verify.outputs.committed }}' + FIRST_VERIFIED_HEAD: '${{ steps.verify.outputs.verified_head }}' REPAIR_ATTEMPTED: '${{ steps.repair.outputs.attempted }}' REPAIR_OUTCOME: '${{ steps.verify_repair.outputs.outcome }}' REPAIR_COMMITTED: '${{ steps.verify_repair.outputs.committed }}' + REPAIR_VERIFIED_HEAD: '${{ steps.verify_repair.outputs.verified_head }}' run: |- OUTCOME="${FIRST_OUTCOME}" COMMITTED="${FIRST_COMMITTED}" + VERIFIED_HEAD="${FIRST_VERIFIED_HEAD}" if [[ "${REPAIR_ATTEMPTED}" == 'true' ]]; then OUTCOME="${REPAIR_OUTCOME}" COMMITTED="${REPAIR_COMMITTED:-${FIRST_COMMITTED}}" + VERIFIED_HEAD="${REPAIR_VERIFIED_HEAD}" fi echo "outcome=${OUTCOME}" >> "${GITHUB_OUTPUT}" if [[ -n "${COMMITTED}" ]]; then echo "committed=${COMMITTED}" >> "${GITHUB_OUTPUT}" fi + if [[ -n "${VERIFIED_HEAD}" ]]; then + echo "verified_head=${VERIFIED_HEAD}" >> "${GITHUB_OUTPUT}" + fi case "${OUTCOME}" in fixed|noop) ;; *) exit 1 ;; @@ -3628,6 +3637,7 @@ jobs: # variable (not a secret), already the agent's OPENAI_MODEL. MODEL: '${{ vars.QWEN_AUTOFIX_MODEL || vars.QWEN_PR_REVIEW_MODEL }}' CHECKED_OUT_HEAD: '${{ steps.prepare.outputs.checked_out_head }}' + VERIFIED_HEAD: '${{ steps.final_verify.outputs.verified_head }}' run: |- # The head the agent actually evaluated — captured in prepare before # any mutation, not the report-time remote head (which can move @@ -3717,6 +3727,20 @@ jobs: PUSH_RACE_MERGED='true' fi done + CAN_RESOLVE_THREADS='false' + 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" + fi + fi # Resolve the review threads whose findings the agent actually # IMPLEMENTED, so a human re-reviewing sees only what is still open # instead of re-reading every thread to work out what was handled. @@ -3745,8 +3769,15 @@ jobs: echo "::warning::PR has more than 100 review threads; threads past the first page will not be resolved or answered in-thread" fi fi - if [[ -s "${WORKDIR}/resolved-comments.txt" ]]; then - RESOLVED_N=0 + 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' + } while IFS= read -r rc_id || [[ -n "${rc_id}" ]]; do rc_id="${rc_id%$'\r'}" rc_id="${rc_id#rc:}" @@ -3759,16 +3790,41 @@ jobs: echo "::warning::comment ${rc_id} matched no open review thread" 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 + echo "::warning::stopping review-thread resolution because the live PR head moved before resolving comment ${rc_id}" + break + elif [[ "${THREAD_IS_RESOLVED}" == 'true' ]]; then + echo "::warning::comment ${rc_id} was resolved by another actor before this round could resolve it" + continue + elif [[ "${THREAD_IS_RESOLVED}" != 'false' ]]; then + echo "::warning::stopping review-thread resolution because the state of comment ${rc_id} could not be proven" + break + fi + RESOLVE_SUCCEEDED='false' if gh api graphql -f threadId="${thread_id}" -f query=' mutation($threadId:ID!){ resolveReviewThread(input:{threadId:$threadId}){thread{isResolved}} }' > /dev/null 2>&1; then - RESOLVED_N=$(( RESOLVED_N + 1 )) - else + RESOLVE_SUCCEEDED='true' + fi + POST_GUARD_OK='false' + if IFS=$'\t' read -r LIVE_PR_HEAD THREAD_IS_RESOLVED < <(read_thread_guard "${thread_id}" 2> /dev/null); then + POST_GUARD_OK='true' + fi + if [[ "${POST_GUARD_OK}" == 'true' && "${LIVE_PR_HEAD}" == "${VERIFIED_HEAD}" && "${THREAD_IS_RESOLVED}" == 'true' ]]; then + if [[ "${RESOLVE_SUCCEEDED}" != 'true' ]]; then + echo "::warning::comment ${rc_id} is resolved after an unsuccessful mutation command; another actor or a lost response may be responsible" + fi + CONFIRMED_RESOLVED_N=$(( CONFIRMED_RESOLVED_N + 1 )) + 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 + 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" - echo "🧵 resolved ${RESOLVED_N} review thread(s) the agent implemented" + echo "🧵 confirmed ${CONFIRMED_RESOLVED_N} selected review thread(s) resolved while the verified head remained live" 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. diff --git a/.qwen/skills/autofix/SKILL.md b/.qwen/skills/autofix/SKILL.md index 6fe549240a..193ac2d5ee 100644 --- a/.qwen/skills/autofix/SKILL.md +++ b/.qwen/skills/autofix/SKILL.md @@ -262,8 +262,16 @@ Finish with exactly one outcome: finding that is RESOLVED IN THE CODE. That is the test, not "did I edit a file this round": a finding you implemented now, and one an earlier commit already fixed that you re-verified still holds, are both resolved and both - belong here. The workflow resolves exactly those review threads - after the push, so a human re-reviewing sees only what is still open — an + belong here. After the push, the workflow resolves exactly those review + threads only while the live PR head is still the exact commit covered by + deterministic verification. The workflow checks the live head and thread + state around each mutation and stops resolving more threads if the result + cannot be proven. It does not automatically reopen a thread because GitHub + cannot atomically prove which actor resolved it. If uncertainty is detected, + remaining threads stay open for a later round. This minimizes the chance of + hiding a finding after unverified code lands, while acknowledging that GitHub + provides no atomic head-SHA precondition for the resolution mutation. A human + re-reviewing can focus on what is still open — an already-fixed Critical left open reads as an unaddressed Critical. A finding you declined, deferred, or escalated for a maintainer's decision must stay unresolved so its recorded diff --git a/docs/design/autofix-resolve-fixed-review-threads.md b/docs/design/autofix-resolve-fixed-review-threads.md new file mode 100644 index 0000000000..a05c1df839 --- /dev/null +++ b/docs/design/autofix-resolve-fixed-review-threads.md @@ -0,0 +1,95 @@ +# Autofix review-thread resolution hardening + +## Problem + +Qwen Autofix already lets the review-address agent identify inline review comments that are resolved in code. The credentialed host workflow maps those REST comment IDs to GitHub review threads and calls `resolveReviewThread` after pushing the fix. + +The current ordering is generally safe, but it does not prove that the live PR head being resolved is the exact commit covered by deterministic verification: + +- A rejected push may be salvaged by merging a newly moved remote head. The merged commit is pushed even though verification predates the merge. +- The PR author may push again after Autofix pushes and before the resolution mutation. +- A same-run repair can inherit `resolved-comments.txt` or `comment-replies.json` from the rejected first attempt. + +These gaps can mark a conversation resolved without evidence that the current PR head still contains the verified fix. + +## Current state + +The responsibilities are already separated correctly: + +- `.qwen/skills/autofix/SKILL.md` tells the agent how to classify findings and write `resolved-comments.txt` or `comment-replies.json`. +- `.github/scripts/run-autofix-review-verification.sh` independently runs deterministic build, typecheck, lint, and affected-package tests. +- `.github/workflows/qwen-autofix.yml` owns the GitHub PAT, pushes the branch, fetches review threads, and performs mutations. +- `scripts/tests/qwen-autofix-workflow.test.js` extracts and executes workflow shell blocks with stubbed GitHub responses. + +The GitHub mutation must remain in the trusted workflow. The agent must not receive GitHub credentials. + +## Proposed changes + +### Verification gate + +Require a clean tracked worktree and index before deterministic checks, capture the commit SHA, and require both the SHA and tracked state to remain unchanged after the structural checks and again after build, typecheck, lint, and tests. Then record that captured SHA as a step output named `verified_head`. Do not emit it for no-op or failed outcomes. This rejects persistent tracked changes or commits created by branch-controlled checks; it does not claim an immutable filesystem or detect a script that temporarily changes state and restores it within one command, which remains part of the existing CI trust model. + +### Final verification selection + +Propagate the selected verification SHA through the final verification step: + +- use the first verification SHA when no repair ran; +- use only the repair verification SHA when repair ran; +- never fall back to the first SHA for a successful repaired outcome. + +### Repair isolation + +Before invoking the repair agent, remove `resolved-comments.txt` and `comment-replies.json` together with the other prior-attempt artifacts. The repair attempt must explicitly regenerate its final dispositions. Missing files therefore fail closed: no thread is resolved or replied to. + +### Post-push resolution proof + +Before resolving any selected thread, require all of the following: + +1. `verified_head` is non-empty. +2. The push-race salvage did not create an unverified merge commit. +3. Local `HEAD` after the successful push equals `verified_head`. +4. A live `gh pr view` query succeeds. +5. The live PR `headRefOid` equals `verified_head` before each mutation. +6. The live PR `headRefOid` still equals `verified_head` immediately after each mutation. + +Before each mutation, a single GraphQL guard reads both the live `headRefOid` and the target thread's live `isResolved` state. A thread already resolved by another actor is skipped. After the mutation, the same guard verifies both values again. This post-check also runs when the mutation command returns an error, because a lost response does not prove that GitHub did not apply the mutation. + +If a pre-mutation condition is unknown or false, or a post-mutation condition is ambiguous, stop resolving additional conversations. A failed mutation whose post-guard proves the verified head is unchanged and the thread remains open is safe to warn and continue. The workflow does not call `unresolveReviewThread`: GitHub does not expose a compare-and-swap precondition or mutation attribution, so even a successful `resolveReviewThread` response cannot prove that another actor did not resolve the thread between the pre-guard and the mutation. Automatically reopening it could therefore undo another reviewer's action. An unsuccessful mutation command followed by a post-guard that confirms the verified head and resolved state is counted as an observed resolved state, without attributing it to Autofix; any ambiguous result stops the remaining mutations. + +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. + +## 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. +- **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. + +## Files affected + +- `.github/scripts/run-autofix-review-verification.sh` +- `.github/workflows/qwen-autofix.yml` +- `scripts/tests/qwen-autofix-workflow.test.js` +- `.qwen/skills/autofix/SKILL.md` for contract clarification + +## Scope boundaries + +Included: + +- exact verified/live head equality; +- push-race fail-closed behavior; +- repair-attempt disposition isolation; +- focused workflow contract and behavioral tests. + +Excluded: + +- GraphQL pagination beyond the existing first 100 threads; +- resolving arbitrary non-Autofix PR conversations; +- dismissing `CHANGES_REQUESTED` reviews; +- giving the model direct GitHub credentials; +- changing generic `/review` or CLI behavior. + +## Open questions + +None. The conservative behavior is deterministic before mutation: uncertainty prevents additional threads from being resolved. After a mutation, the workflow observes and reports state but never automatically unresolves it without atomic ownership evidence. diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index 2414f9368f..71d0f1ef51 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -6270,9 +6270,8 @@ describe('qwen-autofix workflow', () => { /rm -f \\\n([\s\S]*?)\n {10}rm -rf "\$\{QWEN_HOME\}"/, )?.[1] ?? ''; expect(repairCleanup).toContain('gate-rejection.md'); - expect(repairDeterministicRejectionStep).not.toContain( - '"${WORKDIR}/resolved-comments.txt"', - ); + expect(repairCleanup).toContain('"${WORKDIR}/resolved-comments.txt"'); + expect(repairCleanup).toContain('"${WORKDIR}/comment-replies.json"'); expect( repairDeterministicRejectionStep.match( /node "\$\{RUNNER_TEMP\}\/autofix-skill\/scripts\/run-agent\.mjs"/g, @@ -6312,6 +6311,42 @@ describe('qwen-autofix workflow', () => { expect(finalizeStatusCommentStep).toContain( "OUTCOME: '${{ steps.final_verify.outputs.outcome }}'", ); + const verificationHeadCapture = reviewVerificationRunner.indexOf( + 'VERIFICATION_HEAD="$(git rev-parse HEAD)"', + ); + const schemaCheck = reviewVerificationRunner.indexOf( + "run_check 'settings schema is stale on the agent-committed fix'", + ); + const contractCheck = reviewVerificationRunner.indexOf( + "run_check 'cross-package contract verification failed'", + ); + const noOpCheck = reviewVerificationRunner.indexOf( + 'if git diff --quiet "origin/${BRANCH}...${BRANCH}"; then', + ); + const buildCheck = reviewVerificationRunner.indexOf( + "run_check 'build failed on the agent-committed fix' npm run build", + ); + const assertions = [ + ...reviewVerificationRunner.matchAll(/^assert_verification_tree$/gm), + ].map((match) => match.index); + const verifiedHeadOutput = reviewVerificationRunner.indexOf( + 'echo "verified_head=${VERIFICATION_HEAD}" >> "${GITHUB_OUTPUT}"', + ); + expect(reviewVerificationRunner).toContain( + "reject_fix 'workspace is dirty before deterministic verification'", + ); + expect(verificationHeadCapture).toBeGreaterThan(-1); + expect(schemaCheck).toBeGreaterThan(verificationHeadCapture); + expect(contractCheck).toBeGreaterThan(schemaCheck); + expect(assertions).toHaveLength(2); + expect(assertions[0]).toBeGreaterThan(contractCheck); + expect(noOpCheck).toBeGreaterThan(assertions[0]); + expect(buildCheck).toBeGreaterThan(noOpCheck); + expect(assertions[1]).toBeGreaterThan(buildCheck); + expect(verifiedHeadOutput).toBeGreaterThan(assertions[1]); + expect(pushAndReportStep).toContain( + "VERIFIED_HEAD: '${{ steps.final_verify.outputs.verified_head }}'", + ); const body = finalizeVerificationStep .match(/ {8}run: \|-\n([\s\S]*)$/)?.[1] @@ -6329,9 +6364,11 @@ describe('qwen-autofix workflow', () => { GITHUB_OUTPUT: output, FIRST_OUTCOME: '', FIRST_COMMITTED: '', + FIRST_VERIFIED_HEAD: '', REPAIR_ATTEMPTED: '', REPAIR_OUTCOME: '', REPAIR_COMMITTED: '', + REPAIR_VERIFIED_HEAD: '', ...env, }, }); @@ -6340,9 +6377,11 @@ describe('qwen-autofix workflow', () => { return { status: result.status, written }; }; - expect(run({ FIRST_OUTCOME: 'fixed' })).toMatchObject({ + expect( + run({ FIRST_OUTCOME: 'fixed', FIRST_VERIFIED_HEAD: 'first-sha' }), + ).toMatchObject({ status: 0, - written: expect.stringContaining('outcome=fixed'), + written: expect.stringContaining('verified_head=first-sha'), }); expect(run({ FIRST_OUTCOME: 'noop' })).toMatchObject({ status: 0, @@ -6352,25 +6391,28 @@ describe('qwen-autofix workflow', () => { run({ FIRST_OUTCOME: 'failed', FIRST_COMMITTED: 'true', + FIRST_VERIFIED_HEAD: 'stale-first-sha', REPAIR_ATTEMPTED: 'true', REPAIR_OUTCOME: 'fixed', REPAIR_COMMITTED: 'true', + REPAIR_VERIFIED_HEAD: 'repair-sha', }), ).toMatchObject({ status: 0, - written: expect.stringContaining('outcome=fixed'), + written: expect.stringContaining('verified_head=repair-sha'), }); - expect( - run({ - FIRST_OUTCOME: 'failed', - FIRST_COMMITTED: 'true', - REPAIR_ATTEMPTED: 'true', - REPAIR_OUTCOME: 'fixed', - }), - ).toMatchObject({ + const repairedWithoutVerifiedHead = run({ + FIRST_OUTCOME: 'failed', + FIRST_COMMITTED: 'true', + FIRST_VERIFIED_HEAD: 'stale-first-sha', + REPAIR_ATTEMPTED: 'true', + REPAIR_OUTCOME: 'fixed', + }); + expect(repairedWithoutVerifiedHead).toMatchObject({ status: 0, written: expect.stringContaining('committed=true'), }); + expect(repairedWithoutVerifiedHead.written).not.toContain('verified_head='); expect( run({ FIRST_OUTCOME: 'failed', @@ -7849,13 +7891,12 @@ describe('qwen-autofix workflow', () => { // itself (its sandbox carries no token), so it records the inline-comment // ids it implemented and the push step maps each to its thread. const lines = workflow.split('\n'); - const i = lines.findIndex((l) => - l.includes('resolved-comments.txt" ]]; then'), - ); + const i = lines.findIndex((l) => l.includes("CAN_RESOLVE_THREADS='false'")); const j = lines.findIndex( - (l, k) => k > i && l.trim().startsWith('echo "🧵 resolved'), + (l, k) => k > i && l.trim().startsWith('echo "🧵 confirmed'), ); expect(i).toBeGreaterThan(-1); + expect(j).toBeGreaterThan(i); const block = lines.slice(i, j + 2).join('\n'); // feedback.md must carry the handle the agent echoes back. expect(workflow).toContain('- [rc:\\(.id)]'); @@ -7869,58 +7910,293 @@ describe('qwen-autofix workflow', () => { join(bin, 'gh'), [ '#!/usr/bin/env bash', - 'for a in "$@"; do [[ "$a" == threadId=* ]] && printf "%s\\n" "${a#threadId=}" >> "$RESOLVED_LOG"; done', - 'exit 0', + 'if [[ "$1 $2" == "pr view" ]]; then', + ' saw_json=false; saw_head_field=false; saw_jq=false; saw_head_filter=false', + ' for a in "$@"; do', + ' [[ "$a" == --json ]] && saw_json=true', + ' [[ "$a" == headRefOid ]] && saw_head_field=true', + ' [[ "$a" == --jq ]] && saw_jq=true', + ' [[ "$a" == \'.headRefOid // ""\' ]] && saw_head_filter=true', + ' done', + ' [[ "$saw_json $saw_head_field $saw_jq $saw_head_filter" == "true true true true" ]] || exit 2', + ' [[ "${LIVE_HEAD_EXIT:-0}" == 0 ]] || exit "${LIVE_HEAD_EXIT}"', + ' count="$(cat "$HEAD_READ_COUNT")"', + ' count=$((count + 1))', + ' printf "%s" "$count" > "$HEAD_READ_COUNT"', + ' if [[ -n "${LIVE_HEAD_SEQUENCE:-}" ]]; then', + ' value="$(cut -d, -f"$count" <<< "$LIVE_HEAD_SEQUENCE")"', + ' printf "%s" "${value:-${LIVE_HEAD:-}}"', + ' else', + ' printf "%s" "${LIVE_HEAD:-}"', + ' fi', + ' exit 0', + 'fi', + 'query=""; thread_id=""', + 'for a in "$@"; do', + ' [[ "$a" == query=* ]] && query="${a#query=}"', + ' [[ "$a" == threadId=* ]] && thread_id="${a#threadId=}"', + 'done', + 'if [[ "$query" == *"reviewThreads(first:100)"* ]]; then', + ' printf \'%s\' "$THREADS_RAW_STUB"', + ' exit 0', + 'fi', + 'if [[ "$query" == *PullRequestReviewThread* ]]; then', + ' saw_jq=false; saw_guard_filter=false', + ' for a in "$@"; do', + ' [[ "$a" == --jq ]] && saw_jq=true', + ' [[ "$a" == \'[.data.repository.pullRequest.headRefOid // "", .data.node.isResolved] | @tsv\' ]] && saw_guard_filter=true', + ' done', + ' [[ "$saw_jq $saw_guard_filter" == "true true" ]] || exit 2', + ' count="$(cat "$HEAD_READ_COUNT")"', + ' count=$((count + 1))', + ' printf "%s" "$count" > "$HEAD_READ_COUNT"', + ' value="$(cut -d, -f"$count" <<< "${LIVE_HEAD_SEQUENCE:-}")"', + ' head="${value:-${LIVE_HEAD:-}}"', + ' resolved=false', + ' if [[ "${UNKNOWN_THREAD_STATE:-}" == "$thread_id" ]]; then resolved=null; elif grep -qxF "$thread_id" "$THREAD_STATE_FILE" || [[ "${OTHER_ACTOR_RESOLVED:-}" == "$thread_id" ]]; then resolved=true; fi', + ' printf "%s\\t%s\\n" "$head" "$resolved"', + ' exit 0', + 'fi', + 'if [[ "$query" == *resolveReviewThread* ]]; then', + ' printf "resolve:%s\\n" "$thread_id" >> "$RESOLVED_LOG"', + ' if [[ "${RESOLVE_APPLIES:-true}" == true ]]; then', + ' grep -qxF "$thread_id" "$THREAD_STATE_FILE" || printf "%s\\n" "$thread_id" >> "$THREAD_STATE_FILE"', + ' fi', + ' [[ "${RESOLVE_EXIT:-0}" == 0 ]] || exit "${RESOLVE_EXIT}"', + ' exit 0', + 'fi', + 'exit 1', ].join('\n'), ); chmodSync(join(bin, 'gh'), 0o755); // 111 was implemented; 333's thread is already resolved; 999 matches // nothing. 222 was DECLINED, so it is deliberately absent and must stay open. writeFileSync(join(dir, 'resolved-comments.txt'), 'rc:111\r\n333\n999\n'); - const out = execFileSync('bash', ['-c', `set -euo pipefail\n${block}`], { - env: { - ...process.env, - PATH: `${bin}:${process.env.PATH}`, - WORKDIR: dir, - REPO: 'QwenLM/qwen-code', - PR: '7308', - RESOLVED_LOG: resolvedLog, - // The threads fetch is hoisted above the resolve block (shared with the - // reply block), so the test supplies the mapped threads directly. - THREADS_JSON: JSON.stringify([ - { - id: 'T_open_1', - isResolved: false, - comments: { nodes: [{ databaseId: 111 }] }, - }, - { - id: 'T_open_2', - isResolved: false, - comments: { nodes: [{ databaseId: 222 }] }, - }, - { - id: 'T_done', - isResolved: true, - comments: { nodes: [{ databaseId: 333 }] }, - }, - ]), - }, + const localHead = execFileSync('git', ['rev-parse', 'HEAD'], { encoding: 'utf8', + }).trim(); + const threadsRaw = JSON.stringify({ + nodes: [ + { + id: 'T_open_1', + isResolved: false, + comments: { nodes: [{ databaseId: 111 }] }, + }, + { + id: 'T_open_2', + isResolved: false, + comments: { nodes: [{ databaseId: 222 }] }, + }, + { + id: 'T_open_3', + isResolved: false, + comments: { nodes: [{ databaseId: 444 }] }, + }, + { + id: 'T_done', + isResolved: true, + comments: { nodes: [{ databaseId: 333 }] }, + }, + ], + pageInfo: { hasNextPage: false }, }); - const resolved = readFileSync(resolvedLog, 'utf8') - .trim() - .split('\n') - .filter(Boolean); - expect(resolved).toEqual(['T_open_1']); - expect(resolved).not.toContain('T_open_2'); // declined stays open - expect(resolved).not.toContain('T_done'); // already resolved - expect(out).toContain('resolved 1 review thread'); + const headReadCount = join(dir, 'head-read-count'); + const threadStateFile = join(dir, 'thread-state'); + const runResolve = (env = {}) => { + writeFileSync(resolvedLog, ''); + writeFileSync(headReadCount, '0'); + writeFileSync(threadStateFile, ''); + const result = spawnSync('bash', ['-c', `set -euo pipefail\n${block}`], { + env: { + ...process.env, + PATH: `${bin}:${process.env.PATH}`, + WORKDIR: dir, + REPO: 'QwenLM/qwen-code', + PR: '7308', + RESOLVED_LOG: resolvedLog, + HEAD_READ_COUNT: headReadCount, + THREAD_STATE_FILE: threadStateFile, + THREADS_RAW_STUB: threadsRaw, + VERIFIED_HEAD: localHead, + LIVE_HEAD: localHead, + PUSH_RACE_MERGED: 'false', + ...env, + }, + encoding: 'utf8', + }); + return { + status: result.status, + out: `${result.stdout}${result.stderr}`, + resolved: readFileSync(resolvedLog, 'utf8') + .trim() + .split('\n') + .filter(Boolean), + }; + }; + + expect(block).toContain( + 'gh api graphql -f owner="${REPO%%/*}" -f name="${REPO##*/}" -F pr="${PR}" -f threadId="${1}"', + ); + expect(block).toContain('pullRequest(number:$pr){headRefOid}'); + expect(block).toContain( + 'node(id:$threadId){... on PullRequestReviewThread{isResolved}}', + ); + + const matching = runResolve(); + expect(matching.status).toBe(0); + expect(matching.resolved).toEqual(['resolve:T_open_1']); + expect(matching.resolved).not.toContain('resolve:T_open_2'); // declined stays open + expect(matching.resolved).not.toContain('resolve:T_done'); // already resolved + expect(matching.out).toContain( + 'confirmed 1 selected review thread(s) resolved while the verified head remained live', + ); + + const movedBeforeMutation = runResolve({ + LIVE_HEAD_SEQUENCE: `${localHead},new-contributor-head`, + }); + expect(movedBeforeMutation.status).toBe(0); + expect(movedBeforeMutation.resolved).toEqual([]); + expect(movedBeforeMutation.out).toContain( + 'live PR head moved before resolving comment 111', + ); + + const movedDuringMutation = runResolve({ + LIVE_HEAD_SEQUENCE: `${localHead},${localHead},new-contributor-head`, + }); + expect(movedDuringMutation.status).toBe(0); + expect(movedDuringMutation.resolved).toEqual(['resolve:T_open_1']); + expect(movedDuringMutation.out).toContain( + 'live PR head or thread state could not be proven after resolving comment 111', + ); + expect(movedDuringMutation.out).toContain( + 'confirmed 0 selected review thread(s) resolved', + ); + + const lostMutationResponse = runResolve({ RESOLVE_EXIT: '1' }); + expect(lostMutationResponse.status).toBe(0); + expect(lostMutationResponse.resolved).toEqual(['resolve:T_open_1']); + expect(lostMutationResponse.out).toContain( + 'another actor or a lost response may be responsible', + ); + expect(lostMutationResponse.out).toContain( + 'confirmed 1 selected review thread(s) resolved', + ); + + writeFileSync(join(dir, 'resolved-comments.txt'), 'rc:111\nrc:444\n'); + const lostResponseThenDrift = runResolve({ + RESOLVE_EXIT: '1', + LIVE_HEAD_SEQUENCE: `${localHead},${localHead},${localHead},new-contributor-head`, + }); + expect(lostResponseThenDrift.status).toBe(0); + expect(lostResponseThenDrift.resolved).toEqual(['resolve:T_open_1']); + expect(lostResponseThenDrift.out).toContain( + 'another actor or a lost response may be responsible', + ); + expect(lostResponseThenDrift.out).toContain( + 'live PR head moved before resolving comment 444', + ); + writeFileSync(join(dir, 'resolved-comments.txt'), 'rc:111\r\n333\n999\n'); + + writeFileSync(join(dir, 'resolved-comments.txt'), 'rc:111\nrc:444\n'); + const failedMutation = runResolve({ + RESOLVE_APPLIES: 'false', + RESOLVE_EXIT: '1', + }); + expect(failedMutation.status).toBe(0); + expect(failedMutation.resolved).toEqual([ + 'resolve:T_open_1', + 'resolve:T_open_3', + ]); + expect(failedMutation.out).toContain( + 'could not resolve the review thread for comment 111', + ); + expect(failedMutation.out).toContain( + 'could not resolve the review thread for comment 444', + ); + expect(failedMutation.out).toContain( + 'confirmed 0 selected review thread(s) resolved', + ); + + const falseSuccess = runResolve({ + RESOLVE_APPLIES: 'false', + RESOLVE_EXIT: '0', + }); + expect(falseSuccess.status).toBe(0); + expect(falseSuccess.resolved).toEqual(['resolve:T_open_1']); + expect(falseSuccess.out).toContain( + 'live PR head or thread state could not be proven after resolving comment 111', + ); + expect(falseSuccess.out).not.toContain('resolve:T_open_3'); + expect(falseSuccess.out).toContain( + 'confirmed 0 selected review thread(s) resolved', + ); + writeFileSync(join(dir, 'resolved-comments.txt'), 'rc:111\r\n333\n999\n'); + + const ambiguousMutation = runResolve({ + RESOLVE_EXIT: '1', + LIVE_HEAD_SEQUENCE: `${localHead},${localHead},new-contributor-head`, + }); + expect(ambiguousMutation.status).toBe(0); + expect(ambiguousMutation.resolved).toEqual(['resolve:T_open_1']); + expect(ambiguousMutation.out).toContain( + 'live PR head or thread state could not be proven after resolving comment 111', + ); + expect(ambiguousMutation.out).toContain( + 'confirmed 0 selected review thread(s) resolved', + ); + + const resolvedByOtherActor = runResolve({ + OTHER_ACTOR_RESOLVED: 'T_open_1', + }); + expect(resolvedByOtherActor.status).toBe(0); + expect(resolvedByOtherActor.resolved).toEqual([]); + expect(resolvedByOtherActor.out).toContain('was resolved by another actor'); + + writeFileSync(join(dir, 'resolved-comments.txt'), 'rc:111\nrc:444\n'); + const unknownThreadState = runResolve({ + UNKNOWN_THREAD_STATE: 'T_open_1', + }); + expect(unknownThreadState.status).toBe(0); + expect(unknownThreadState.resolved).toEqual([]); + expect(unknownThreadState.out).toContain( + 'state of comment 111 could not be proven', + ); + expect(unknownThreadState.out).not.toContain('resolve:T_open_3'); + + const movedBeforeSecondMutation = runResolve({ + LIVE_HEAD_SEQUENCE: `${localHead},${localHead},${localHead},new-contributor-head`, + }); + expect(movedBeforeSecondMutation.status).toBe(0); + expect(movedBeforeSecondMutation.resolved).toEqual(['resolve:T_open_1']); + expect(movedBeforeSecondMutation.out).toContain( + 'live PR head moved before resolving comment 444', + ); + expect(movedBeforeSecondMutation.out).toContain( + 'confirmed 1 selected review thread(s) resolved', + ); + writeFileSync(join(dir, 'resolved-comments.txt'), 'rc:111\r\n333\n999\n'); + + for (const env of [ + { LIVE_HEAD: 'new-contributor-head' }, + { LIVE_HEAD_EXIT: '1' }, + { VERIFIED_HEAD: 'different-verified-head' }, + { PUSH_RACE_MERGED: 'true' }, + ]) { + const uncertain = runResolve(env); + expect(uncertain.status).toBe(0); + expect(uncertain.resolved).toEqual([]); + expect(uncertain.out).toContain('skipping review-thread resolution'); + expect(uncertain.out).not.toContain( + 'confirmed 1 selected review thread(s) resolved', + ); + } // The SKILL keys resolution on the FINDING being fixed, not on "did I edit // a file this round" — an earlier commit's fix that still holds resolves // too, or a fixed Critical sits open and reads as unaddressed (#7731). const skill = readAutofixSkill(); expect(skill).toContain('RESOLVED IN THE CODE'); expect(skill).toMatch(/already fixed that you re-verified still holds/); + expect(skill).toContain('live PR head is still the exact commit'); expect(skill).toContain('comment-replies.json'); rmSync(dir, { recursive: true, force: true }); });