diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 50ee0f5164c..750c1f68e2d 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -1,25 +1,53 @@ -name: '🧐 Qwen Pull Request Review' +name: 'Qwen Pull Request Review' on: pull_request_target: - types: ['opened'] - pull_request_review_comment: - types: ['created'] - pull_request_review: - types: ['submitted'] + types: ['opened', 'reopened', 'ready_for_review', 'synchronize'] + issue_comment: + types: ['created', 'edited'] workflow_dispatch: inputs: pr_number: description: 'PR number to review' required: true type: 'number' + review_mode: + description: 'Run without posting comments, or publish a PR summary comment' + required: true + default: 'dry-run' + type: 'choice' + options: + - 'dry-run' + - 'comment' + additional_instructions: + description: 'Optional maintainer focus for this review' + required: false + type: 'string' jobs: review-pr: + # NOTE (intentional, current-phase safeguard): auto-triggers and the + # @qwen /review comment trigger are gated to OWNER/MEMBER/COLLABORATOR + # on purpose. This workflow runs under pull_request_target with + # repository secrets and a 5-30 min LLM deep review, so an open + # trigger would be a denial-of-wallet / abuse surface. External + # contributor PRs are still reviewable today — a maintainer comments + # `@qwen /review` on the PR. Broadening auto-trigger for community + # PRs (with per-author rate limiting) is deferred to a later phase, + # not dropped here. See docs/design/code-review/code-review-design.md. + # + # NOTE: contains() below is a substring match, so '@qwen /reviewer' + # (or any other suffix) also passes this gate. That's intentional: + # GHA expressions cannot anchor a regex here, and the + # 'Resolve PR context' shell step re-checks with a properly + # end-anchored awk/grep regex (@qwen /review($|[[:space:]])) and sets + # should_run_review=false on non-matches. The cost of the extra + # runner spin-up on a malformed mention is a few seconds and + # acceptable. if: |- github.event_name == 'workflow_dispatch' || (github.event_name == 'pull_request_target' && - github.event.action == 'opened' && + (github.event.action == 'opened' || github.event.action == 'reopened' || github.event.action == 'ready_for_review' || github.event.action == 'synchronize') && (github.event.pull_request.author_association == 'OWNER' || github.event.pull_request.author_association == 'MEMBER' || github.event.pull_request.author_association == 'COLLABORATOR')) || @@ -28,163 +56,593 @@ jobs: contains(github.event.comment.body, '@qwen /review') && (github.event.comment.author_association == 'OWNER' || github.event.comment.author_association == 'MEMBER' || - github.event.comment.author_association == 'COLLABORATOR')) || - (github.event_name == 'pull_request_review_comment' && - contains(github.event.comment.body, '@qwen /review') && - (github.event.comment.author_association == 'OWNER' || - github.event.comment.author_association == 'MEMBER' || - github.event.comment.author_association == 'COLLABORATOR')) || - (github.event_name == 'pull_request_review' && - contains(github.event.review.body, '@qwen /review') && - (github.event.review.author_association == 'OWNER' || - github.event.review.author_association == 'MEMBER' || - github.event.review.author_association == 'COLLABORATOR')) - timeout-minutes: 15 + github.event.comment.author_association == 'COLLABORATOR')) + concurrency: + group: 'qwen-pr-review-${{ github.event.issue.number || github.event.pull_request.number || github.event.inputs.pr_number }}' + cancel-in-progress: true + # 30 min was empirically too low: a full (no-cache) 9-agent deep + # review on a ~800-line PR exceeded it and the job was killed + # mid-review. Incremental (synchronize) reviews are far shorter, but + # opened/comment/dispatch run the full path. 60 min leaves headroom. + timeout-minutes: 60 runs-on: 'ubuntu-latest' permissions: contents: 'read' - id-token: 'write' + # 'write' is required even though we only use `gh pr comment`: the gh + # CLI calls the GraphQL addComment mutation, which is gated on + # pull-requests write (not issues write — that path is for issue + # comments on non-PR issues). Validated by an empirical + # workflow_dispatch run that failed with + # "GraphQL: Resource not accessible by integration (addComment)". pull-requests: 'write' issues: 'write' + env: + GITHUB_TOKEN: '${{ secrets.GITHUB_TOKEN }}' + OPENAI_MODEL: '${{ vars.QWEN_PR_REVIEW_MODEL }}' + QWEN_PR_REVIEW_MAX_CHANGED_LINES: "${{ vars.QWEN_PR_REVIEW_MAX_CHANGED_LINES || '1500' }}" steps: - - name: 'Checkout PR code' - uses: 'actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd' # v6.0.2 + # Security: pull_request_target runs with repository secrets, so it + # MUST check out trusted base code (main), never the PR head. + # workflow_dispatch is maintainer-triggered, so it runs the + # dispatched ref's own code — this is the pre-merge dry-run path. + - name: 'Checkout review code' + uses: 'actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683' # v4.2.2 with: token: '${{ secrets.GITHUB_TOKEN }}' + ref: "${{ github.event_name == 'workflow_dispatch' && github.ref || 'main' }}" fetch-depth: 0 - - name: 'Get PR details (pull_request_target & workflow_dispatch)' - id: 'get_pr' + - name: 'Resolve PR context' + id: 'pr' + env: + EVENT_NAME: '${{ github.event_name }}' + WORKFLOW_PR_NUMBER: '${{ github.event.inputs.pr_number }}' + WORKFLOW_REVIEW_MODE: '${{ github.event.inputs.review_mode }}' + WORKFLOW_ADDITIONAL_INSTRUCTIONS: '${{ github.event.inputs.additional_instructions }}' + run: |- + set -euo pipefail + + additional_instructions="" + case "$EVENT_NAME" in + workflow_dispatch) + pr_number="$WORKFLOW_PR_NUMBER" + review_mode="${WORKFLOW_REVIEW_MODE:-dry-run}" + additional_instructions="${WORKFLOW_ADDITIONAL_INSTRUCTIONS:-}" + comment_body="" + ;; + pull_request_target) + pr_number="$(jq -r '.pull_request.number' "$GITHUB_EVENT_PATH")" + review_mode="comment" + comment_body="" + ;; + issue_comment) + pr_number="$(jq -r '.issue.number' "$GITHUB_EVENT_PATH")" + review_mode="comment" + comment_body="$(jq -r '.comment.body // ""' "$GITHUB_EVENT_PATH")" + ;; + *) + echo "::error::Unsupported event: $EVENT_NAME" + exit 1 + ;; + esac + + if [ -z "$pr_number" ] || [ "$pr_number" = "null" ]; then + echo "::error::Could not resolve pull request number" + exit 1 + fi + + case "$review_mode" in + dry-run|comment) + ;; + *) + echo "::error::Unsupported review mode: $review_mode" + exit 1 + ;; + esac + + should_run_review="true" + + # Use grep -qE with end-boundary so '@qwen /review' does not match + # '@qwen /reviewer' or similar variants. + if [ "$EVENT_NAME" != "workflow_dispatch" ] && [ "$EVENT_NAME" != "pull_request_target" ]; then + if printf '%s' "$comment_body" | grep -qE '@qwen /review($|[[:space:]])'; then + additional_instructions="$( + printf '%s' "$comment_body" | + awk ' + BEGIN { found = 0 } + !found { + if (match($0, /@qwen \/review([[:space:]]|$)/)) { + found = 1 + rest = substr($0, RSTART + RLENGTH) + if (length(rest) > 0) print rest + } + next + } + { print } + ' | + sed 's/^[[:space:]]*//' + )" + else + should_run_review="false" + fi + fi + # Strip leading blank lines so the prompt doesn't start with an + # empty line when @qwen /review is on its own line. + additional_instructions="$(printf '%s' "$additional_instructions" | sed '/./,$!d')" + # Keep maintainer focus text from changing the slash-command flags. + additional_instructions="$(printf '%s' "$additional_instructions" | LC_ALL=C perl -0pe 's/(?> "$GITHUB_OUTPUT" + + - name: 'Check PR size' + id: 'size' if: |- - ${{ github.event_name == 'pull_request_target' || github.event_name == 'workflow_dispatch' }} + steps.pr.outputs.should_run_review == 'true' env: - GITHUB_TOKEN: '${{ secrets.GITHUB_TOKEN }}' + PR_NUMBER: '${{ steps.pr.outputs.number }}' + REVIEW_MODE: '${{ steps.pr.outputs.review_mode }}' + SHOULD_COMMENT: '${{ steps.pr.outputs.should_comment }}' run: |- - if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then - PR_NUMBER=${{ github.event.inputs.pr_number }} + set -euo pipefail + + write_output() { + echo "$1" >> "$GITHUB_OUTPUT" + } + + if [ -z "${OPENAI_MODEL:-}" ]; then + echo "::error::Repository variable QWEN_PR_REVIEW_MODEL is required for this workflow (maps to env var OPENAI_MODEL)." + exit 1 + fi + + if ! printf '%s' "$QWEN_PR_REVIEW_MAX_CHANGED_LINES" | grep -Eq '^[0-9]+$'; then + echo "::error::QWEN_PR_REVIEW_MAX_CHANGED_LINES must be an integer." + exit 1 + fi + + if [ "$QWEN_PR_REVIEW_MAX_CHANGED_LINES" -lt 100 ] || [ "$QWEN_PR_REVIEW_MAX_CHANGED_LINES" -gt 50000 ]; then + echo "::error::QWEN_PR_REVIEW_MAX_CHANGED_LINES must be between 100 and 50000 (got $QWEN_PR_REVIEW_MAX_CHANGED_LINES)." + exit 1 + fi + + pr_json="$(gh pr view "$PR_NUMBER" \ + --repo "$GITHUB_REPOSITORY" \ + --json additions,deletions,changedFiles,title,baseRefName,baseRefOid,headRefName,headRefOid)" + additions="$(jq -r '.additions' <<< "$pr_json")" + deletions="$(jq -r '.deletions' <<< "$pr_json")" + changed_files="$(jq -r '.changedFiles' <<< "$pr_json")" + title="$(jq -r '.title' <<< "$pr_json")" + base_ref="$(jq -r '.baseRefName' <<< "$pr_json")" + base_sha="$(jq -r '.baseRefOid // ""' <<< "$pr_json")" + head_ref="$(jq -r '.headRefName' <<< "$pr_json")" + head_sha="$(jq -r '.headRefOid // ""' <<< "$pr_json")" + changed_lines=$((additions + deletions)) + + # Reject empty head_sha / base_sha early. Both are queried via + # gh pr view; an empty value almost always means gh pr view + # partially failed. + if [ -z "$head_sha" ]; then + echo "::error::Could not resolve PR head SHA via gh pr view --json headRefOid." + exit 1 + fi + if [ -z "$base_sha" ]; then + echo "::error::Could not resolve PR base SHA via gh pr view --json baseRefOid." + exit 1 + fi + + # Compute the merge-base SHA, i.e., the commit where the PR's + # history forks from the base branch. The merge base is what the + # cache key must be scoped to, NOT baseRefOid. + # + # baseRefOid is the current tip of the base ref (e.g., main HEAD). + # That value can stay fixed across reviews even when the PR + # merges base into itself: if the author clicks "Update branch" + # while base hasn't moved, baseRefOid is unchanged but the PR's + # history now incorporates base's commits. A cache restored under + # baseRefOid would then let the bundled /review skill compute + # `git diff ..` across upstream commits the PR + # did not author. The merge base, in contrast, advances whenever + # base content enters the PR (Update branch, rebase onto newer + # base, or base retarget), which is exactly the boundary we + # want to invalidate the cache on. + # + # merge-base is best-effort: the compare endpoint can fail to + # resolve SHAs for fork PRs in the base repo's namespace. That + # must not abort the review (set -euo pipefail) — it only means + # the incremental cache can't be scoped this run, so we fall + # back to a full review rather than failing the job. + merge_base_sha="$(gh api \ + "repos/${GITHUB_REPOSITORY}/compare/${base_sha}...${head_sha}" \ + --jq '.merge_base_commit.sha // ""' 2>/dev/null || true)" + if [ -z "$merge_base_sha" ]; then + echo "::warning::Could not resolve PR merge base; incremental cache will fall back to a full review this run." + fi + + write_output "changed_lines=$changed_lines" + write_output "changed_files=$changed_files" + write_output "head_sha=$head_sha" + write_output "base_sha=$base_sha" + write_output "merge_base_sha=$merge_base_sha" + echo "Review target: PR #$PR_NUMBER" + echo "Review title: $title" + echo "Review branch: $base_ref ($base_sha) <- $head_ref ($head_sha)" + if [ -n "$merge_base_sha" ]; then + echo "Review merge base: $merge_base_sha" + else + echo "Review merge base: (unresolved; full review this run)" + fi + echo "Review scope: $changed_files files, +$additions/-$deletions ($changed_lines changed lines)" + + if [ "$changed_lines" -gt "$QWEN_PR_REVIEW_MAX_CHANGED_LINES" ]; then + write_output "should_review=false" + { + printf 'This PR changes %s lines across %s files, which is above the current automated review threshold of %s changed lines.\n\n' \ + "$changed_lines" "$changed_files" "$QWEN_PR_REVIEW_MAX_CHANGED_LINES" + printf 'Please consider splitting it into smaller, focused PRs before requesting a full Qwen Code review. Smaller PRs are easier to validate, easier to dogfood, and less likely to mix product direction, refactoring, and implementation details in one review.\n\n' + printf "_Qwen Code PR review did not run a detailed code review for this oversized changeset. Model configured for review: \`%s\`._\n" \ + "$OPENAI_MODEL" + } > qwen-pr-review-size-comment.md + if [ "$SHOULD_COMMENT" = "true" ]; then + gh pr comment "$PR_NUMBER" \ + --repo "$GITHUB_REPOSITORY" \ + --body-file qwen-pr-review-size-comment.md + else + { + printf '### Qwen PR review dry run\n\n' + cat qwen-pr-review-size-comment.md + printf "\n\nReview mode: \`%s\`; no PR comments were posted.\n" "$REVIEW_MODE" + } >> "$GITHUB_STEP_SUMMARY" + fi else - PR_NUMBER=${{ github.event.pull_request.number }} + write_output "should_review=true" fi - echo "pr_number=$PR_NUMBER" >> "$GITHUB_OUTPUT" - # Get PR details - PR_DATA=$(gh pr view $PR_NUMBER --json title,body,additions,deletions,changedFiles,baseRefName,headRefName) - echo "pr_data=$PR_DATA" >> "$GITHUB_OUTPUT" - # Get file changes - CHANGED_FILES=$(gh pr diff $PR_NUMBER --name-only) - echo "changed_files<> "$GITHUB_OUTPUT" - echo "$CHANGED_FILES" >> "$GITHUB_OUTPUT" - echo "EOF" >> "$GITHUB_OUTPUT" - - - name: 'Get PR details (issue_comment)' - id: 'get_pr_comment' + + # Restore the bundled /review skill's per-PR cache only on synchronize. + # The skill writes .qwen/review-cache/pr-.json with the last reviewed + # commit SHA so subsequent runs can scope review to the incremental + # diff. Cache is intentionally NOT restored on opened/reopened (full + # review) or comment / workflow_dispatch (manual re-review must not + # short-circuit on "No new changes since last review"). See + # docs/design/code-review/code-review-design.md §增量评审与缓存. + # + # Cache key uses merge_base_sha + head_sha (not baseRefOid). The + # merge base advances whenever base content enters the PR — Update + # branch, rebase onto newer base, or base retarget — which are + # exactly the boundaries that should invalidate the cache. Pure + # author pushes leave the merge base alone, so the restore-keys + # prefix `qwen-review---` keeps matching and + # bundled /review can still scope to the incremental diff. + - name: 'Restore review cache' + id: 'restore-cache' if: |- - ${{ github.event_name == 'issue_comment' }} + steps.size.outputs.should_review == 'true' && + github.event_name == 'pull_request_target' && + github.event.action == 'synchronize' + uses: 'actions/cache/restore@0057852bfaa89a56745cba8c7296529d2fc39830' # v4.3.0 + with: + path: '.qwen/review-cache' + key: 'qwen-review-${{ steps.pr.outputs.number }}-${{ steps.size.outputs.merge_base_sha }}-${{ steps.size.outputs.head_sha }}' + restore-keys: | + qwen-review-${{ steps.pr.outputs.number }}-${{ steps.size.outputs.merge_base_sha }}- + + # @qwen-code/qwen-code requires Node >=22; runner default is 20 + # (npm EBADENGINE). Pin Node 22 before installing/invoking qwen. + - name: 'Set up Node.js' + if: |- + steps.size.outputs.should_review == 'true' + uses: 'actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e' # v4 + with: + node-version: '22' + + # Cache the npm download cache so the global qwen-code install is + # offline-fast on subsequent runs (content-addressed; a stale key + # only misses the newest tarball, never serves wrong content). + - name: 'Cache npm for qwen-code install' + if: |- + steps.size.outputs.should_review == 'true' + uses: 'actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830' # v4.3.0 + with: + path: '~/.npm' + key: 'qwen-npm-global-${{ runner.os }}-v1' + restore-keys: | + qwen-npm-global-${{ runner.os }}- + + # Direct invocation instead of QwenLM/qwen-code-action. + # The action wraps the CLI in `QWEN_RESPONSE=$(qwen ...)` command + # substitution, which buffers ALL stdout until the process exits — + # so a slow or stuck bundled /review produced zero observable + # output for the whole job and could not be diagnosed. Calling + # qwen ourselves and piping through `tee` streams progress to the + # live job log in real time, and an explicit `timeout` makes a + # stall fail fast with a clear error instead of a silent job kill. + # settings.json mirrors what the action wrote (folder trust off + + # yolo + sandbox off) so the bundled /review skill's full tool set + # runs non-interactively. + - name: 'Run Qwen Code Review' + id: 'review' + if: |- + steps.size.outputs.should_review == 'true' env: GITHUB_TOKEN: '${{ secrets.GITHUB_TOKEN }}' - COMMENT_BODY: '${{ github.event.comment.body }}' + OPENAI_API_KEY: '${{ secrets.REVIEW_OPENAI_API_KEY }}' + OPENAI_BASE_URL: '${{ secrets.REVIEW_OPENAI_BASE_URL }}' + OPENAI_MODEL: '${{ vars.QWEN_PR_REVIEW_MODEL }}' + REVIEW_PROMPT: '${{ steps.pr.outputs.review_prompt }}' + SHOULD_COMMENT: '${{ steps.pr.outputs.should_comment }}' + run: |- + set -euo pipefail + + mkdir -p .qwen + cat > .qwen/settings.json <<'EOF' + { + "security": { "folderTrust": { "enabled": false } }, + "tools": { "approvalMode": "yolo" }, + "sandbox": false + } + EOF + + # Intentionally @latest: this workflow is still in the Phase 1-3 + # bundled-action rollout (see docs/design/code-review/roadmap.md) + # and we want every CI run to exercise the newest bundled /review + # skill while it stabilizes — pinning a version would freeze the + # in-progress skill iterations behind a manual bump PR. Once the + # workflow lands and the skill cadence slows, switch this to a + # pinned `@qwen-code/qwen-code@x.y.z` so the secret-bearing + # pull_request_target run can't be moved by a future package + # publish. Tracked as a follow-up; not blocking on this PR. + echo "::group::Install qwen-code" + npm install -g @qwen-code/qwen-code@latest + qwen --version + echo "::endgroup::" + + # CI-lightweight steering applies to EVERY CI run (comment and + # dry-run): the full bundled /review (9 agents incl. Agent 7 = + # npm ci + whole-monorepo build/test) cannot finish a heavy PR + # within the job timeout, and the repo's own CI already + # builds/tests every PR, so Agent 7 is redundant here. The + # extra "do not post" clause is added only for dry-run. + # Prompt-level steering only — the bundled SKILL.md is unchanged. + # + # Validation Evidence verdict: the exact section name, PRESENT / + # MISSING semantics, and the verbatim advisory closing line all + # live in .qwen/review-rules.md ("Validation And Dogfooding"). + # The bundled /review skill is expected to load that file, so the + # prompt only references it instead of restating the spec — that + # way edits to review-rules.md don't drift from the workflow. + prompt="${REVIEW_PROMPT} + + CI LIGHTWEIGHT MODE — follow these execution constraints, they + override the skill's defaults for this run: + 1. Do NOT run \`npm ci\`/install, build, or the test suite, and + do NOT launch Agent 7 (Build & Test). The repository's own + CI already builds and tests this PR; it is redundant here. + 2. Run Agents 1-5 (Correctness, Security, Code Quality, + Performance, Test Coverage) and ONE consolidated audit + instead of the three separate 6a/6b/6c personas. + 3. Skip the local linter/typecheck step (Step 3). + 4. Apply the \`## Validation Evidence\` requirement defined in + .qwen/review-rules.md (section 'Validation And Dogfooding') + exactly as written there — same section title, same PRESENT/ + MISSING verdict, same verbatim advisory closing line. Do not + paraphrase. + 5. Produce the complete code review." + if [ "${SHOULD_COMMENT:-}" != "true" ]; then + prompt="${prompt} + + DRY RUN: print the full review to stdout and do NOT post + anything to GitHub — no gh pr comment, no gh pr review, no + GitHub API writes, no inline comments. Output only; the + maintainer reads it from the CI log." + fi + + out=qwen-review-stream.jsonl + # Plain `qwen ... | tee` does NOT stream: qwen is a Node CLI and + # Node full-buffers stdout when it is a pipe, so nothing shows + # until exit. `--output-format stream-json --include-partial- + # messages` is qwen's purpose-built incremental event stream — + # each event is written as it happens, so `tee` shows real + # progress in the live job log. `timeout` bounds a stall. + echo "::group::Qwen /review (live stream-json events)" + set +e + timeout 50m qwen --yolo \ + --output-format stream-json --include-partial-messages \ + --prompt "$prompt" 2>&1 | tee "$out" + status=${PIPESTATUS[0]} + set -e + echo "::endgroup::" + + # Extract the final assistant text from the stream for a + # human-readable summary (downstream comment step / artifact). + # + # Intentional simplification while debugging: this loop keeps + # overwriting `txt` on every assistant/message event so the file + # ends up with the LAST text segment seen. With + # --include-partial-messages enabled, intermediate progress lines + # land in the live job log (via `tee` above), which is exactly + # what we want to verify the review is actually running. The + # parsed-summary file only needs the final segment for the PR + # comment. If multi-turn assistant output ever becomes the norm, + # switch to a stop_reason / terminal-event filter and concatenate + # — but that costs the live-progress visibility we rely on for + # now, so this is staying simple until the skill stabilizes. + node -e ' + const fs=require("fs"); + const lines=fs.readFileSync(process.argv[1],"utf8").split(/\r?\n/); + let txt=""; + for (const l of lines) { + if (!l.trim()) continue; + let e; try { e=JSON.parse(l); } catch { continue; } + const c=e?.message?.content; + if ((e.type==="assistant"||e.type==="message") && Array.isArray(c)) { + const t=c.filter(p=>p?.type==="text").map(p=>p.text).join(""); + if (t) txt=t; + } + } + fs.writeFileSync("qwen-review-summary.md", txt || "(no assistant text parsed; see raw stream in the job log)"); + ' "$out" || cp "$out" qwen-review-summary.md + + echo "::group::Parsed review summary" + cat qwen-review-summary.md + echo "::endgroup::" + + delimiter="QWEN_REVIEW_SUMMARY_$(openssl rand -hex 8)" + { + echo "summary<<$delimiter" + cat qwen-review-summary.md + echo "" + echo "$delimiter" + } >> "$GITHUB_OUTPUT" + + if [ "$status" -eq 124 ]; then + echo "::error::qwen /review exceeded the 50m step timeout." + exit 1 + fi + exit "$status" + + - name: 'Post dry-run summary' + if: |- + steps.review.outcome == 'success' && + steps.pr.outputs.should_comment != 'true' + env: + PR_NUMBER: '${{ steps.pr.outputs.number }}' run: |- - PR_NUMBER=${{ github.event.issue.number }} - echo "pr_number=$PR_NUMBER" >> "$GITHUB_OUTPUT" - # Extract additional instructions from comment - ADDITIONAL_INSTRUCTIONS=$(echo "$COMMENT_BODY" | sed 's/.*@qwen \/review//' | xargs) - echo "additional_instructions=$ADDITIONAL_INSTRUCTIONS" >> "$GITHUB_OUTPUT" - # Get PR details - PR_DATA=$(gh pr view $PR_NUMBER --json title,body,additions,deletions,changedFiles,baseRefName,headRefName) - echo "pr_data=$PR_DATA" >> "$GITHUB_OUTPUT" - # Get file changes - CHANGED_FILES=$(gh pr diff $PR_NUMBER --name-only) - echo "changed_files<> "$GITHUB_OUTPUT" - echo "$CHANGED_FILES" >> "$GITHUB_OUTPUT" - echo "EOF" >> "$GITHUB_OUTPUT" - - - name: 'Run Qwen PR Review' - uses: 'QwenLM/qwen-code-action@5fd6818d04d64e87d255ee4d5f77995e32fbf4c2' + { + printf '### Qwen PR review dry run\n\n' + printf 'Completed review for PR #%s without posting PR comments.\n' "$PR_NUMBER" + printf '\nReview logs are available in this workflow run.\n' + } >> "$GITHUB_STEP_SUMMARY" + + - name: 'Post review summary comment' + id: 'post-summary' + if: |- + steps.review.outcome == 'success' && + steps.pr.outputs.should_comment == 'true' env: - GITHUB_TOKEN: '${{ secrets.GITHUB_TOKEN }}' - PR_NUMBER: '${{ steps.get_pr.outputs.pr_number || steps.get_pr_comment.outputs.pr_number }}' - PR_DATA: '${{ steps.get_pr.outputs.pr_data || steps.get_pr_comment.outputs.pr_data }}' - CHANGED_FILES: '${{ steps.get_pr.outputs.changed_files || steps.get_pr_comment.outputs.changed_files }}' - ADDITIONAL_INSTRUCTIONS: '${{ steps.get_pr.outputs.additional_instructions || steps.get_pr_comment.outputs.additional_instructions }}' - REPOSITORY: '${{ github.repository }}' + PR_NUMBER: '${{ steps.pr.outputs.number }}' + REVIEW_SUMMARY: '${{ steps.review.outputs.summary }}' + run: |- + set -euo pipefail + + { + printf '## Qwen Code Review\n\n' + if [ -n "${REVIEW_SUMMARY:-}" ]; then + printf '%s\n' "${REVIEW_SUMMARY:0:60000}" + if [ "${#REVIEW_SUMMARY}" -gt 60000 ]; then + printf '\n\n_Review summary was truncated. See the workflow logs for the full output._\n' + fi + else + printf '_Qwen Code review completed, but no summary was captured. See the workflow logs for details._\n' + fi + # The single-quoted format string keeps literal backticks + # around %s and /review for the rendered Markdown; %s is a + # printf placeholder filled by the positional arg, so the + # single quotes are correct here. + # shellcheck disable=SC2016 + printf '\n\n---\n_Reviewed by `%s` via Qwen Code `/review` (automated). Reply `@qwen /review` to re-run._\n' "${OPENAI_MODEL:-unknown}" + } > qwen-pr-review-summary-comment.md + + gh pr comment "$PR_NUMBER" \ + --repo "$GITHUB_REPOSITORY" \ + --body-file qwen-pr-review-summary-comment.md + + # Check the exact cache key after the review comment is delivered. + # lookup-only avoids downloading/restoring cache contents here; it only + # prevents actions/cache/save from attempting to create a key that already + # exists, which can happen on reruns of opened/reopened/ready_for_review + # where the pre-review restore step intentionally did not run. + - name: 'Check review cache key' + id: 'cache-lookup' + if: |- + steps.review.outcome == 'success' && + steps.post-summary.outcome == 'success' && + github.event_name == 'pull_request_target' && + steps.size.outputs.head_sha != '' && + steps.size.outputs.merge_base_sha != '' + uses: 'actions/cache/restore@0057852bfaa89a56745cba8c7296529d2fc39830' # v4.3.0 with: - OPENAI_API_KEY: '${{ secrets.OPENAI_API_KEY }}' - OPENAI_BASE_URL: '${{ secrets.OPENAI_BASE_URL }}' - OPENAI_MODEL: '${{ secrets.OPENAI_MODEL }}' - settings_json: |- - { - "coreTools": [ - "run_shell_command", - "write_file" - ], - "sandbox": false - } - prompt: |- - You are an expert code reviewer. You have access to shell commands to gather PR information and perform the review. - - IMPORTANT: Use the available shell commands to gather information. Do not ask for information to be provided. - - Start by running these commands to gather the required data: - 1. Run: echo "$PR_DATA" to get PR details (JSON format) - 2. Run: echo "$CHANGED_FILES" to get the list of changed files - 3. Run: echo "$PR_NUMBER" to get the PR number - 4. Run: echo "$ADDITIONAL_INSTRUCTIONS" to see any specific review instructions from the user - 5. Run: gh pr diff $PR_NUMBER to see the full diff - 6. For any specific files, use: cat filename, head -50 filename, or tail -50 filename - - Additional Review Instructions: - If ADDITIONAL_INSTRUCTIONS contains text, prioritize those specific areas or focus points in your review. - Common instruction examples: "focus on security", "check performance", "review error handling", "check for breaking changes" - - Once you have the information, provide a comprehensive code review by: - 1. Writing your review to a file: write_file("review.md", "") - 2. Posting the review: gh pr comment $PR_NUMBER --body-file review.md --repo $REPOSITORY - - Review Areas: - - **Security**: Authentication, authorization, input validation, data sanitization - - **Performance**: Algorithms, database queries, caching, resource usage - - **Reliability**: Error handling, logging, testing coverage, edge cases - - **Maintainability**: Code structure, documentation, naming conventions - - **Functionality**: Logic correctness, requirements fulfillment - - Output Format: - Structure your review using this exact format with markdown: - - ## 📋 Review Summary - Provide a brief 2-3 sentence overview of the PR and overall assessment. - - ## 🔍 General Feedback - - List general observations about code quality - - Mention overall patterns or architectural decisions - - Highlight positive aspects of the implementation - - Note any recurring themes across files - - ## 🎯 Specific Feedback - Only include sections below that have actual issues. If there are no issues in a priority category, omit that entire section. - - ### 🔴 Critical - (Only include this section if there are critical issues) - Issues that must be addressed before merging (security vulnerabilities, breaking changes, major bugs): - - **File: `filename:line`** - Description of critical issue with specific recommendation - - ### 🟡 High - (Only include this section if there are high priority issues) - Important issues that should be addressed (performance problems, design flaws, significant bugs): - - **File: `filename:line`** - Description of high priority issue with suggested fix - - ### 🟢 Medium - (Only include this section if there are medium priority issues) - Improvements that would enhance code quality (style issues, minor optimizations, better practices): - - **File: `filename:line`** - Description of medium priority improvement - - ### 🔵 Low - (Only include this section if there are suggestions) - Nice-to-have improvements and suggestions (documentation, naming, minor refactoring): - - **File: `filename:line`** - Description of suggestion or enhancement - - **Note**: If no specific issues are found in any category, simply state "No specific issues identified in this review." - - ## ✅ Highlights - (Only include this section if there are positive aspects to highlight) - - Mention specific good practices or implementations - - Acknowledge well-written code sections - - Note improvements from previous versions + path: '.qwen/review-cache' + key: 'qwen-review-${{ steps.pr.outputs.number }}-${{ steps.size.outputs.merge_base_sha }}-${{ steps.size.outputs.head_sha }}' + lookup-only: true + + # Save cache only AFTER the review summary comment was successfully + # posted. Saving before publication would persist "this head was + # reviewed" state even if `gh pr comment` later failed (rate-limit, + # network, deleted PR, etc.) — the next synchronize would then + # restore the cache, bundled /review would short-circuit on + # "No new changes since last review" or scope to a tiny incremental + # diff, and the findings that never reached the PR would be lost. + # Gating on post-summary.outcome makes cache advancement track + # comment delivery, not just model success. The lookup step above skips + # duplicate exact-key saves on reruns. + - name: 'Save review cache' + if: |- + steps.review.outcome == 'success' && + steps.post-summary.outcome == 'success' && + github.event_name == 'pull_request_target' && + steps.size.outputs.head_sha != '' && + steps.size.outputs.merge_base_sha != '' && + steps.cache-lookup.outputs.cache-hit != 'true' + uses: 'actions/cache/save@0057852bfaa89a56745cba8c7296529d2fc39830' # v4.3.0 + with: + path: '.qwen/review-cache' + key: 'qwen-review-${{ steps.pr.outputs.number }}-${{ steps.size.outputs.merge_base_sha }}-${{ steps.size.outputs.head_sha }}' + + # Fires on either review-step failure OR post-summary failure, so a + # broken `gh pr comment` (permission gap, rate limit, transient + # network) still leaves a visible trace on the PR pointing reviewers + # at the workflow run. Without the post-summary branch a silent + # post-comment failure would look identical to "no review happened" + # from the PR's perspective. + - name: 'Post fallback comment on review failure' + if: |- + failure() && + (steps.review.conclusion == 'failure' || + steps.post-summary.conclusion == 'failure') && + steps.pr.outputs.should_comment == 'true' + env: + PR_NUMBER: '${{ steps.pr.outputs.number }}' + RUN_URL: '${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}' + run: |- + set -euo pipefail + + { + printf '_Qwen Code automated PR review did not complete successfully. See the workflow logs for details: %s_\n' "$RUN_URL" + printf "\nThis is an automated message; please retry by commenting \`@qwen /review\` once the underlying issue is resolved.\n" + } > qwen-pr-review-failure-comment.md + gh pr comment "$PR_NUMBER" \ + --repo "$GITHUB_REPOSITORY" \ + --body-file qwen-pr-review-failure-comment.md || true diff --git a/.gitignore b/.gitignore index 6ff1d950be2..912e9de603a 100644 --- a/.gitignore +++ b/.gitignore @@ -30,6 +30,7 @@ CLAUDE.md # Qwen Code Configs .qwen/* +!.qwen/review-rules.md !.qwen/commands/ !.qwen/commands/** !.qwen/skills/ @@ -93,4 +94,4 @@ tmp/ # code graph skills .venv -.codegraph \ No newline at end of file +.codegraph diff --git a/.qwen/review-rules.md b/.qwen/review-rules.md new file mode 100644 index 00000000000..5824cff01d9 --- /dev/null +++ b/.qwen/review-rules.md @@ -0,0 +1,87 @@ +# Qwen Code Review Rules + +These are the project-specific review criteria for Qwen Code. Bundled +`/review` loads this file and applies the rules below to its review +agents. Apply them conservatively: the goal is to reduce review noise +and route unclear PRs to maintainers, not to make final product +decisions on weak evidence. + +> Scope note: this file describes _what_ to evaluate and _how strong_ a +> finding is — review content only. It does NOT describe CI workflow +> mechanics (when a gate stops the pipeline, when a process comment is +> posted, how to re-trigger). When `/review` runs locally on +> uncommitted changes there is no CI gate; treat the rules below as +> review guidance, not as a process to enforce. + +## Finding Severity + +How strongly to weight each gate's findings. "blocking" means a failure +here is a high-priority, actionable finding the author should resolve +before the change is mergeable; "advisory" means flag it but it does not +by itself block. + +| Gate | Default severity | Override token (in this file) | +| ----------------------- | ---------------- | ---------------------------------- | +| Scope / PR Purity | blocking | `scope-gate: advisory` | +| Product Direction | blocking | `product-direction-gate: advisory` | +| Validation / Dogfooding | advisory | `validation-gate: blocking` | + +## Review Gates + +### Scope And PR Purity + +- Prefer small, focused PRs that can be reviewed and validated independently. +- Flag PRs that mix unrelated product changes, broad refactors, dependency + churn, formatting, and feature implementation in one changeset. +- Large implementation PRs should clearly separate planning/rationale from + mechanical code changes. If the rationale is missing, ask for it before + reviewing code details. + +### Product Direction + +- New features should fit Qwen Code's existing CLI/TUI-first developer workflow, + composable tool model, slash-command behavior, and repository conventions. +- Do not reward PRs that add popular external features only because another + tool has them. The author should explain why the feature belongs in Qwen Code + and how it fits existing interaction patterns. +- Ask for maintainer discussion when a PR changes core agent behavior, tool + permissions, authentication, model selection, sandboxing, telemetry, release + flow, or public CLI/SDK contracts without a clear design rationale. +- Prefer incremental extensions over rewrites unless the PR explains why the + existing design cannot support the change. + +### Validation And Dogfooding + +- Feature PRs and user-visible behavior changes should include reviewer-facing + validation evidence, not just "tested locally". +- Good evidence includes exact commands, prompts, inputs, observed output, + logs, JSON traces, before/after examples, screenshots, GIFs, or short videos. +- CLI-only changes can be validated with command transcripts and observed + output when they demonstrate the changed behavior. +- TUI, interactive, visual, or workflow changes should include a screenshot, + GIF, video, or equivalent before/after evidence whenever practical. +- Dogfooding notes should explain the quickest reviewer path to exercise the + feature and what result to expect. + +**Required review output — Validation Evidence verdict.** Every review of a +PR MUST include a section titled exactly `## Validation Evidence` with one +of: + +- `PRESENT` — name the concrete evidence found (commands / logs / JSON + trace / before-after / screenshot / GIF / recording / test report). +- `MISSING` — state what reviewer-facing evidence is absent and what the + author should add. For feature / user-visible / high-risk PRs treat this + as a blocking-severity finding; for docs-only / pure-refactor it is + advisory. + +End that section with this line verbatim: +`> This is an automated, advisory, comment-only review — it never approves or requests changes. After adding validation evidence, comment \`@qwen /review\` to re-run; editing the PR description alone does NOT re-trigger this review in the current phase.` + +### Functional Review + +- Once the gates pass, focus detailed code review on correctness, security, + maintainability, performance, test coverage, and compatibility with existing + Qwen Code conventions. +- Prefer high-signal findings with concrete impact. Avoid style preferences, + speculative best-practice commentary, and issues already covered by linters, + typecheckers, or existing PR comments. diff --git a/docs/design/code-review/code-review-design.md b/docs/design/code-review/code-review-design.md new file mode 100644 index 00000000000..e39610f83f5 --- /dev/null +++ b/docs/design/code-review/code-review-design.md @@ -0,0 +1,169 @@ +# Code Review 自动化设计(Phase 1-3) + +> 本文档只覆盖本 PR 实际交付的 **Phase 1-3**(bundled action 切换、增量评审 cache wiring、本设计文档)。 +> Design Gate / 历史 PR 感知 / Feature Readiness / Override / 轮次抑制 / GitHub App 等属于 Phase 4-7, +> 设计与实现随对应 PR 一起提交,路线见 `docs/design/code-review/roadmap.md`。 + +## 问题陈述 + +仓库当前的 AI PR review 跑在 `.github/workflows/qwen-code-pr-review.yml` 上,调用上游 `QwenLM/qwen-code-action` 触发 bundled review skill(`packages/core/src/skills/bundled/review/SKILL.md`)。bundled skill 本身已经做了 9 个并行 review agent、确定性 lint/typecheck、跨文件影响分析、批量 verification、迭代 reverse audit、模式聚合等工作,单次评审质量已经足够。 + +实际运行暴露三类持续问题,单靠 bundled skill 内部优化解决不了: + +1. **不收敛**:作者 push 新 commit 不会自动触发评审;手动 `@qwen /review` 每次都是全量重评,第一轮讨论过的小问题反复在后续轮次被 raise。bundled skill 有 `.qwen/review-cache/pr-.json` 做增量评审,但 GitHub Actions 每次都是全新 runner,cache 在 run 之间丢失,机制从未生效。 +2. **方向偏差**:`review-rules.md` 的 `Product Direction` gate 只是抽象规则,模型靠常识填空,碰到 framing 巧妙的方向漂移会站在作者一边。 +3. **历史决策遗忘**:仓库已有大量"by design 拒过"的 PR,AI review 不感知这些历史决策,新作者重复踩坑。 + +**本 PR(Phase 1-3)只解决问题 1 的基础设施部分**:把 review workflow 切到 bundled action、补齐跨 run 增量 cache wiring、并把整体设计沉淀成文档供后续阶段引用。问题 2、3 由 Phase 4(Design Gate)、Phase 5(历史感知)解决,不在本 PR 范围。 + +## 现状对比(仅 Phase 1-3 关心的维度) + +| 维度 | 改造前 | 本 PR 后 | +| ---------------------------- | ------------------------------ | -------------------------- | +| PR 打开 / reopened 自动评审 | ✅ | ✅ | +| `@qwen /review` 评论触发 | ✅ | ✅ | +| 作者 push 新 commit 自动评审 | ❌ 未监听 `synchronize` | ✅ 新增 synchronize 触发 | +| 增量评审(只评新 commit) | ⚠️ skill 内置但 cache 不持久 | ✅ 跨 run cache 持久化 | +| PR 体积 gate | ⚠️ | ✅ 1500 行可配 | +| 项目级 review 规则文件 | ❌ | ✅ `.qwen/review-rules.md` | +| 9-agent 深审 / reverse audit | ✅(bundled skill 内置,不动) | ✅ | + +> bundled skill 的 9-agent / 确定性 lint / reverse audit 等能力本设计不改动,详见 `packages/core/src/skills/bundled/review/SKILL.md`。 + +## 设计原则 + +**P1. review 工具无状态,状态在外部控制流。** +bundled `/review` skill 跑完一次就退出,不维护跨 run 状态。所有跨 run 状态(cache 等)由 workflow 层用 `actions/cache` / GitHub API 维护。skill 不变,可独立测试、可被任何 channel 调用。**这是 Phase 1-3 的核心原则。** + +**P5. 优先复用现有 design 文档,不写新"团队红线"清单。** +仓库已有 `docs/developers/roadmap.md` / `docs/developers/architecture.md` / `docs/design/*` / 历史 closed-unmerged PR 评论。这些是真实的"团队方向"记录,比新写 `anti-features.md` 更准、更有 cite 价值。Phase 4+ 的 anchor 全部复用它们。 + +> P2(每条判断必须 cite anchor)、P3(按轮次抑制非 critical)、P4(方向判断不进 `/review` deep 流程)属于 Phase 4-6,本 PR 不实现,详见 roadmap。 + +## 触发与权限 + +### 触发事件 + +| 事件 | 行为 | +| ------------------------------------------------------------------------------------------ | ------------------------------------------------------------------ | +| `pull_request_target.opened / reopened / ready_for_review` | 自动跑全量评审 | +| `pull_request_target.synchronize` | **新增**:作者 push 时自动跑**增量评审**(依赖 cache) | +| `issue_comment` / `pull_request_review_comment` / `pull_request_review` 含 `@qwen /review` | 评论触发,默认**强制重跑**,不因同 SHA cache 命中短路 | +| `workflow_dispatch` | 手动触发,可选 dry-run / comment 模式 + 自定义 focus,默认强制重跑 | + +### 权限与 fork 处理 + +- 所有触发都要求 actor 是 `OWNER / MEMBER / COLLABORATOR`,在 workflow `if:` 表达式实现。**这是当前阶段有意保留的安全闸**:本 workflow 在 `pull_request_target` 下带 secrets 运行且深审耗时长,开放触发等于 denial-of-wallet / 滥用面。外部贡献者的 PR 当前仍可评审 —— 由 maintainer 在 PR 下评论 `@qwen /review`。面向社区 PR 的更宽自动触发(配合按作者限流)推后到后续 Phase,本 PR 暂不放开。 +- **不设跨仓 (fork) 拒评 gate**:fork PR 同样进入评审流程。安全边界由 `pull_request_target` 的检出策略保证 —— 自动触发时 workflow 检出可信的 base(`main`)代码、不检出 PR head;只有 maintainer 手动 `workflow_dispatch` 才检出被 dispatch 的 ref。 +- fork PR 的 merge-base 可能无法由 compare 端点解析;该计算是**尽力而为、非致命**:解析失败只是这一轮无法增量、退回全量评审,不阻塞、不报错。 + +### 触发频率策略 + +`synchronize` 不做 debounce:每次 push 都触发,由 cache 保证后续运行只评增量、token 成本可控。push 过频出现 CI 拥塞时,靠已有的 `concurrency` cancel-in-progress 兜底。 + +评论触发和 `workflow_dispatch` **默认不 restore cache**:maintainer 可能在同一 commit 上追加新的 review focus,若 restored cache 的 `lastCommitSha` 与当前 head 一致,bundled skill 会按 "No new changes since last review" 直接退出,导致手动复核没真正执行。 + +## Workflow Review Pipeline(Phase 1-3 形态) + +| Stage | 触发动作 | 成本 | 失败处理 | +| ----- | ----------------------------------------------------------------------- | ---------------------------------------------------------------- | ----------------------------------------------------- | +| 0 | GitHub `if:`(event type / author_association / `@qwen /review`) | 0 | 静默不跑 | +| 1 | workflow shell step(env / model 配置校验、PR size gate、PR 元数据) | <5s | post process comment("PR too large" / 配置缺失) | +| 2 | bundled `/review` deep review(9-agent + reverse audit + verification) | 增量 ~5-15 min;全量可达 ~45-60 min(job `timeout-minutes: 60`) | post inline + summary review comment;失败发 fallback | + +> Phase 4 会在 Stage 1 与 Stage 2 之间插入一个独立的 Design Gate step;本 PR 不含该 step,Stage 1 通过即直接进 bundled `/review`。 + +## 增量评审与缓存(Phase 2 核心) + +### Bundled skill 已有机制 + +`packages/core/src/skills/bundled/review/SKILL.md` Step 1 已实现 incremental review: + +- worktree 创建后写 `.qwen/review-cache/pr-.json`,记 `lastCommitSha`、`lastModelId` +- 再跑同一 PR:SHA 相同 + model 相同 + 无 `--comment` → "No new changes",退出;SHA 不同 → 跑 `git diff ..HEAD` 增量评审;cache 缺失或 rebase 把 cached SHA 推没 → fallback 全量评 + warning + +### 缺失的 wiring(本 PR 补齐) + +`.qwen/review-cache/` 当前**没有跨 GitHub Actions run 持久化**,每次 runner 都是干净的,机制永远走 fallback 全量评分支。本 PR 在 review 步骤前后加 `actions/cache/restore` / `actions/cache/save`: + +- cache key 必须同时含 PR **merge base** 和 head SHA,不能用 `github.sha`,也不要用 baseRefOid。merge base 通过 `gh api repos///compare/...` 的 `merge_base_commit.sha` 获取。 +- merge-base 计算**尽力而为**:fork SHA 解析失败不 `exit 1`,退回全量评审 + warning。 +- 只有 `pull_request_target.synchronize` 在 review 前 restore cache 走增量;`opened/reopened/ready_for_review` 跑全量但成功后 save;comment / `workflow_dispatch` 默认不 restore。 +- save 必须在 PR review summary comment **发出之后**才执行,保存前用 `actions/cache/restore` 的 `lookup-only: true` 检查 exact key 是否已存在。 + +```yaml +- name: Restore previous review cache + if: github.event_name == 'pull_request_target' && github.event.action == 'synchronize' + uses: actions/cache/restore@v4 + with: + path: .qwen/review-cache + key: qwen-review-${{ steps.pr.outputs.number }}-${{ steps.size.outputs.merge_base_sha }}-${{ steps.size.outputs.head_sha }} + restore-keys: | + qwen-review-${{ steps.pr.outputs.number }}-${{ steps.size.outputs.merge_base_sha }}- + +# ... Run Qwen Code Review → Post review summary comment ... + +- name: Check review cache key + id: cache-lookup + if: steps.post-summary.outcome == 'success' + uses: actions/cache/restore@v4 + with: { path: .qwen/review-cache, key: qwen-review-${{ steps.pr.outputs.number }}-${{ steps.size.outputs.merge_base_sha }}-${{ steps.size.outputs.head_sha }}, lookup-only: true } + +- name: Save review cache + if: | + github.event_name == 'pull_request_target' && + steps.review.outcome == 'success' && + steps.post-summary.outcome == 'success' && + steps.cache-lookup.outputs.cache-hit != 'true' + uses: actions/cache/save@v4 + with: { path: .qwen/review-cache, key: qwen-review-${{ steps.pr.outputs.number }}-${{ steps.size.outputs.merge_base_sha }}-${{ steps.size.outputs.head_sha }} } +``` + +**为什么用 merge base 而非 baseRefOid**:merge base 是 PR 历史从 base 分叉的点,在 `Update branch from base` / `rebase` 到更新 base / PR retarget 时会前移 —— 这些恰是 cache 必须失效、必须走 full review 的边界。baseRefOid(base 当前 HEAD)做不到:base 没动但作者 Update branch 时 baseRefOid 不变,restore-keys 仍能 hit 旧 cache,bundled skill 用旧 `lastCommitSha` diff 新 head 会把 merge 引入的上游 commits 一起评。 + +**为什么 Save 必须在 publication 之后**:bundled `/review` step 成功只代表模型出了 summary,不代表 `gh pr comment` 真发出去了(可能 rate-limit / 网络 / PR 关闭失败)。若 Save 在发评论之前,cache 推进 → 下次 synchronize → bundled skill 看到 `lastCommitSha == HEAD` 就 "No new changes" 退出,那轮 findings 永远到不了 PR。Save 必须依赖 `post-summary.outcome == 'success'`。 + +### 路径冲突注意 + +bundled skill 在 worktree(`.qwen/tmp/review-pr-/`)里跑,cache 文件实际写在**主项目目录** `.qwen/review-cache/pr-.json`。`actions/cache` 的 `path` 指主项目目录,不是 worktree 内目录。 + +## 评审身份 + +所有 review 评论作者目前是 `github-actions[bot]`。独立 `qwen-code-review[bot]` 身份需要 org owner 注册 GitHub App,属 Phase 7,本 PR 不动;继续用默认 `GITHUB_TOKEN` 即可跑通本 PR 描述的全部能力。 + +## 配置位置 + +| 资产 | 位置 | 用途 | +| -------------------- | ------------------------------------------------------------------- | ----------------------------------- | +| Review workflow | `.github/workflows/qwen-code-pr-review.yml` | 触发、PR 解析、size gate、调 action | +| 项目级 review 规则 | `.qwen/review-rules.md` | reviewer 行为约束 | +| Bundled review skill | `packages/core/src/skills/bundled/review/SKILL.md` | 9-agent + 增量评审 | +| Cross-run cache | `actions/cache` key=`qwen-review---` | 增量评审持久化 | +| Model 配置 | `vars.QWEN_PR_REVIEW_MODEL` | 评审用模型 | +| 模型 endpoint / key | `secrets.REVIEW_OPENAI_BASE_URL` + `secrets.REVIEW_OPENAI_API_KEY` | 兼容 endpoint | + +## Testing Strategy + +GitHub Actions 的权限、cache、`pull_request_target` 默认分支语义无法被本地完整模拟。Phase 1-3 测试分层: + +1. **本地静态检查(必须)**:`actionlint .github/workflows/qwen-code-pr-review.yml`、`git diff --check`。 +2. **本地 container smoke(可选)**:`act + Colima` 验证 YAML glue / 环境变量 / shell 步骤;不作为 `pull_request_target` / cache / token 权限的最终验收。 +3. **真实 GitHub staging(必须)**:workflow 在 default branch 存在后,用 `gh workflow run ... --ref ` 跑 dry-run;`synchronize` + cache 行为必须在 staging 或 default-branch skeleton 上验证 —— 第二次 push 能 restore cache 并进入 incremental review。 + +## 风险与开放问题(Phase 1-3 相关) + +### R1. 增量 cache 在 rebase / force-push 下的 fallback + +`actions/cache` 的 `restore-keys` prefix match 可能 restore 一个对当前 head 已无意义的 cache。 + +**缓解**:cache key 含 merge base + head SHA 且只在 `synchronize` restore;bundled skill Step 1 已做 SHA validity 检查(`git diff ..HEAD` 失败 → fallback 全量),workflow 层不需额外处理。 + +### R2. Bundled skill 与本仓库的版本耦合 + +PR review workflow 用的是 `qwen-code-action` 内部 `npm install qwen-code@latest`,跟仓库 source 不是同一份;改 bundled skill 必须等下一个 release 才生效。 + +**缓解**:Phase 1-3 不修改 bundled skill;只做 workflow 层 wiring。Phase 4/5 的逻辑也优先作为 workflow helper 实现,不依赖 bundled skill release。 + +## Follow-up & 实施路线 + +后续 Phase 4-7(Design Gate / 历史感知 / 轮次抑制 / GitHub App)的范围、依赖、验收标准见 `docs/design/code-review/roadmap.md`。每个 Phase 作为独立 PR 推进,设计细节随对应实现 PR 一起提交,不在本 PR 提前沉淀。 diff --git a/docs/design/code-review/compare.md b/docs/design/code-review/compare.md new file mode 100644 index 00000000000..1e9e3f3bac2 --- /dev/null +++ b/docs/design/code-review/compare.md @@ -0,0 +1,88 @@ +# Code Review 自动化方案对比 + +跟同类 AI PR review 工具的能力对比,仅看本设计要关心的维度(触发、状态、文档锚定、身份)。 + +> 表中 "qwen-code 目标" 列描述的是完整 Phase 1-7 终态。**本 PR 只交付 Phase 1-3**(bundled action 切换 + 增量 cache + 本设计文档);标 `(Phase 4)` / `(Phase 5)` / `(Phase 6)` / `(Phase 7)` 的能力随对应独立 PR 实现。 + +## 工具范围 + +| 工具 | 形态 | 触发关键词 | 评审主体 | +| -------------------------- | ------------------------------------------------------ | -------------------------- | ------------------------------------- | +| qwen-code 当前 | GitHub Action + 内置 review skill | `@qwen /review` | `github-actions[bot]` | +| qwen-code 本设计目标 | GitHub Action + preflight gates + bundled review + App | `@qwen /review` | `qwen-code-review[bot]` (待 App 注册) | +| Claude Code GitHub | GitHub App + claude-code-action | `@claude` | `claude[bot]` | +| GitHub Copilot Code Review | GitHub 内置 | 自动 + `@copilot` (PR 内) | `Copilot` | +| CodeRabbit | GitHub App + 自家后端 | `@coderabbitai` + 评论命令 | `coderabbitai[bot]` | +| Cursor BugBot | GitHub App | 自动 + `@cursor` (PR 内) | `cursor[bot]` | +| Greptile | GitHub App + 自家后端 | `@greptileai` | `greptileai[bot]` | + +## 维度对比 + +### 触发与执行 + +| 维度 | qwen-code 当前 | qwen-code 目标 | Claude Code | Copilot Review | CodeRabbit | +| ------------------------ | -------------- | ------------------- | ----------- | -------------- | ---------- | +| PR opened 自动 | ✅ | ✅ | ✅ | ✅ | ✅ | +| push 后自动 | ❌ | ✅ | ✅ | ✅ | ✅ | +| `@mention /review` 触发 | ✅ | ✅ | ✅ | ✅ | ✅ | +| `workflow_dispatch` 手动 | ✅ | ✅ | ✅ | ❌ | ❌ | +| 跨 repo PR (fork) 评审 | ⚠️ 无隔离 | ✅(base 检出隔离) | ⚠️ 仅评论 | ✅ | ✅ | +| dry-run 模式 | ✅ | ✅ | ❌ | ❌ | ❌ | +| 大 PR 体积 gate | ✅ 1500 行 | ✅ | ❌ | ❌ | ⚠️ 不阻断 | +| 并发 cancel-in-progress | ✅ | ✅ | ✅ | ✅ | ✅ | + +### 状态与增量 + +| 维度 | qwen-code 当前 | qwen-code 目标 | Claude Code | Copilot Review | CodeRabbit | +| -------------------------- | ------------------------------ | -------------- | ----------- | -------------- | ---------- | +| 增量评审 (只评新 commit) | ⚠️ skill 支持但 cache 不持久化 | ✅ | ✅ | ✅ | ✅ | +| 跨 run cache 持久化 | ❌ | ✅ | 内部托管 | 内部托管 | 内部托管 | +| 历史评审 finding 去重 | ❌ | ✅ (Phase 6) | ✅ | ✅ | ✅ | +| 历史评论 reply chain 解析 | ✅ | ✅ | ✅ | ⚠️ | ✅ | +| "Already discussed" 抑制 | ✅ | ✅ | ✅ | ❌ | ✅ | +| 轮次感知的非 critical 抑制 | ❌ | ✅ (Phase 6) | ❌ | ❌ | ⚠️ 部分 | + +### 评审深度 + +| 维度 | qwen-code 当前 | qwen-code 目标 | Claude Code | Copilot Review | CodeRabbit | +| ----------------------------------------- | -------------- | -------------- | ----------- | -------------- | ---------- | +| 多 agent 并行评审 | ✅ 9 agent | ✅ | ⚠️ 单 agent | ❌ | ⚠️ 2-3 | +| 多人格 audit (attacker / oncall / 维护者) | ✅ | ✅ | ❌ | ❌ | ❌ | +| 确定性 lint/typecheck 集成 | ✅ | ✅ | ⚠️ 靠 hooks | ✅ | ✅ | +| 跨文件影响分析 | ✅ | ✅ | ⚠️ | ⚠️ | ✅ | +| 迭代 reverse audit | ✅ 最多 3 轮 | ✅ | ❌ | ❌ | ❌ | +| 批量 verification 防止假阳性 | ✅ | ✅ | ❌ | ❌ | ⚠️ | +| Low-confidence finding 不进 PR 评论 | ✅ | ✅ | ❌ | ❌ | ⚠️ | +| Build + test 自动跑 | ✅ | ✅ | ❌ (CI 跑) | ❌ | ❌ | + +### 文档锚定与方向控制(本设计独有能力) + +| 维度 | qwen-code 当前 | qwen-code 目标 | Claude Code | Copilot Review | CodeRabbit | +| -------------------------------------- | -------------------------- | -------------- | ---------------- | -------------- | ------------------ | +| 项目级 review 规则文件 | ✅ `.qwen/review-rules.md` | ✅ | `CLAUDE.md` 段落 | 仓库设置 | `.coderabbit.yaml` | +| 评审前置 gate 对照具体设计文档 | ❌ | ✅ (Phase 4) | ❌ | ❌ | ❌ | +| 评审前置 gate 对照 roadmap | ❌ | ✅ (Phase 4) | ❌ | ❌ | ❌ | +| 评审前置 gate 对照架构文档 | ❌ | ✅ (Phase 4) | ❌ | ❌ | ❌ | +| 评审规则对标其他工具 (Claude Code) | ❌ | ✅ (Phase 4) | n/a | ❌ | ❌ | +| Feature PR readiness / dogfooding gate | ⚠️ 仅规则文字 | ✅ (Phase 4) | ❌ | ❌ | ⚠️ 部分 | +| 历史 closed-unmerged PR 感知 | ❌ | ✅ (Phase 5) | ❌ | ❌ | ❌ | +| "by design 拒过"检测 | ❌ | ✅ (Phase 5) | ❌ | ❌ | ❌ | +| 历史 revert / regression 感知 | ❌ | ✅ (Phase 5) | ❌ | ❌ | ❌ | + +> 文档锚定与方向控制是本设计相对其他工具的**核心差异化能力**。其他工具靠模型常识 + 用户配置文件,本设计靠仓库已有的 design 文档 + 历史 PR 数据,每条 finding 必须 cite anchor。 + +### 身份与权限 + +| 维度 | qwen-code 当前 | qwen-code 目标 | Claude Code | Copilot Review | CodeRabbit | +| -------------------------------- | ------------------------ | ------------------------------- | ----------- | -------------- | ---------- | +| 评审主体身份独立 (`[bot]`) | ❌ `github-actions[bot]` | ✅ `qwen-code-review[bot]` (待) | ✅ | ✅ | ✅ | +| `@` 评论框补全 | ❌ | ✅ (待 App 装) | ✅ | ✅ | ✅ | +| 触发权限校验 | ✅ author_association | ✅ App installation | ✅ App | ✅ 内置 | ✅ App | +| 公开 App 可安装 | ❌ | 待 org owner | ✅ | ✅ | ✅ | +| OSS 仓库可独立 install | ❌ | ✅ (后) | ✅ | ✅ | ✅ | + +## 总结 + +本设计在**评审深度**维度已经比所有同类工具更深(9 agent + reverse audit + 跨文件 + 多人格)。**触发自动化**(push 自动评审 + 跨 run 增量 cache)由本 PR 的 Phase 1-2 补齐;**评审主体身份**(独立 `qwen-code-review[bot]`)仍落后于行业基线,由 Phase 7 的 GitHub App 集成补齐。 + +真正独有的差异化在**preflight 文档锚定与方向控制**:现有 design 文档 + 历史 PR 数据作为 anchor,每条 direction 类 finding 强制 cite,并在进入实现层 `/review` 前完成判断。这一块直接对应"`Catch up with Claude Code` + 在 preflight 层校验对齐情况"的 roadmap 目标。 diff --git a/docs/design/code-review/roadmap.md b/docs/design/code-review/roadmap.md new file mode 100644 index 00000000000..cd4abe78e44 --- /dev/null +++ b/docs/design/code-review/roadmap.md @@ -0,0 +1,74 @@ +# Code Review Roadmap + +按"先 wiring 后 logic、先 workflow 后 skill、能小则小"的原则分阶段实施。**本 PR 只交付 Phase 1-3**;Phase 4 起每个阶段作为独立 PR 推进,设计细节随对应实现 PR 一起提交。 + +## Phase 1:Bundled action 切换(本 PR) + +**范围**: + +- 把 PR review workflow 从外部 action 换成 `QwenLM/qwen-code-action`(pin SHA,调用 bundled review skill) +- 加 `.qwen/review-rules.md` 项目级规则 +- 加 `--output-format json` / `--channel=CI` / size gate / fallback comment +- `workflow_dispatch` 检出被 dispatch 的 ref(`pull_request_target` 仍锁 base),用于合并前 dry-run + +**不在此 Phase**:Design Gate / Direction Gate(Phase 4);**不设跨仓 fork 拒评 gate**(fork PR 同样评审,安全边界由 `pull_request_target` 的 base 检出策略保证)。 + +**状态**:本 PR。 + +## Phase 2:增量评审 wiring(本 PR) + +**范围**: + +- 触发列表加入 `pull_request_target.synchronize` +- PR context 解析记录 `baseRefOid`、`headRefOid` 和 **merge base SHA**(`gh api .../compare/...` 的 `merge_base_commit.sha`);merge-base 计算尽力而为、非致命 +- review step 前后加 `actions/cache/restore` + `actions/cache/save`,path 指向主项目目录 `.qwen/review-cache/` +- cache key `qwen-review---`,`restore-keys` 用 `qwen-review---` 前缀。**必须用 merge base 而非 baseRefOid**(理由见 `code-review-design.md`) +- 只有 `pull_request_target.synchronize` 在 review 前 restore cache;评论触发和 `workflow_dispatch` 默认强制重跑 +- **Save cache 必须在 `Post review summary comment` 之后执行**,依赖 `steps.post-summary.outcome == 'success'`,保存前用 `lookup-only: true` 检查 exact key + +**不在此 Phase**:bundled skill 内部不动(已支持 incremental);不引入 debounce;不加 `--incremental` / `--force` 语义。 + +**依赖**:Phase 1。**状态**:本 PR。 + +## Phase 3:Code Review 设计文档(本 PR) + +**范围**:`code-review-design.md`(Phase 1-3 主设计)、`roadmap.md`(本文件)、`compare.md`(对比表)。 + +**目的**:沉淀 Phase 1-3 设计;让 maintainer / 贡献者理解 review 自动化的基础架构与后续路线。Phase 4+ 的详细设计不在本 PR 提前沉淀,随对应 PR 提交。 + +**不在此 Phase**:不动任何 workflow / skill / 代码。 + +**依赖**:可与 Phase 2 并行。**状态**:本 PR。 + +--- + +## 后续阶段(独立 PR,设计随实现提交) + +- **Phase 4 — Design Gate preflight**:新增 workflow helper,在调用 bundled `/review` 前跑方向 / scope / 架构 / Claude Code 对标检查,输出 `PASS / ADVISORY_ONLY / BLOCK`;调整 `review-rules.md` 要求 cite anchor;加 Feature PR Readiness gate。依赖 Phase 3 合入。 +- **Phase 5 — 历史 PR / Issue 感知**:Design Gate 增加 4 类历史检测(同 issue 解决过 / 已有 PR 实现过 / by-design 拒过 → VIOLATION / 历史"坏"PR 信号),`gh search prs/issues` 实现。依赖 Phase 4。 +- **Phase 6 — 轮次抑制**:bundled skill 写 finding cache,对第 2 轮起的 `Suggestion` 同 file/line 抑制,`Critical` 永不抑制;加 `--force` 语义。需改上游 skill,依赖 release 节奏。 +- **Phase 7 — GitHub App 集成**:org owner 创建 `qwen-code-review` App,workflow 加 `actions/create-github-app-token`(带 `if: vars.APP_ID` 兜底)。技术 ready,行政阻塞,可与 Phase 4-6 并行。 + +``` +Phase 1-3 (本 PR) ── merge + │ + ├──────────► Phase 7 (App, async) + ▼ + Phase 4 ──► Phase 5 ──► Phase 6 +``` + +Phase 4/5 必须串行但不依赖 bundled skill release;Phase 6 依赖 release 节奏。 + +## 验收标准(Phase 1-3) + +- **P1**:workflow 合入 main 后,新 PR 触发 bundled action 评审;`.qwen/review-rules.md` 能被 bundled `/review` 加载并作为 review guidance 生效(dry-run 验证)。 +- **P2**:同一 PR 连续 push 两次,第二次从 cache restore,bundled skill 日志显示 "incremental review (last sha: ...)";同 SHA 下评论 `@qwen /review` 仍强制重跑;`Update branch from base`(merge base 前移)后 cache 不被 prefix-match 命中、走 full review;模拟 `gh pr comment` 失败,下次 synchronize 重跑而非 short-circuit。 +- **P3**:合入后任何后续 PR 都能 cite `docs/design/code-review/*`。 + +Phase 4-7 的验收标准随对应 PR 提交。 + +## 测试要求(Phase 1-3) + +- 必须:`actionlint .github/workflows/qwen-code-pr-review.yml`、`git diff --check`。 +- `act + Colima` 可作为 smoke,不作为最终验收。 +- 真实集成至少通过 `workflow_dispatch --ref` dry-run;`pull_request_target.synchronize` + cache restore 行为需要 staging / default-branch skeleton 验证。