From a9038a17699d8265cc6dedb9381372969f410eff Mon Sep 17 00:00:00 2001 From: wenshao Date: Tue, 7 Apr 2026 14:40:16 +0800 Subject: [PATCH] =?UTF-8?q?fix(review):=205=20issues=20=E2=80=94=20CI=20se?= =?UTF-8?q?curity,=20incremental+comment,=20doc=20accuracy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. CI config auto-discovery: read from base branch for PR reviews (PR branch is untrusted, malicious PR could inject commands) 2. Incremental early-exit: don't block --comment on unchanged PR — allow posting comments from previous review findings 3. Doc: review summary not always posted (Comment verdict skips it) 4. Doc: cross-repo reviews skip report persistence 5. Doc: clarify "Agents 1-4 findings verified" (not all — reverse audit findings skip verification) Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/users/features/code-review.md | 9 +++++---- packages/core/src/skills/bundled/review/SKILL.md | 4 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/docs/users/features/code-review.md b/docs/users/features/code-review.md index e497c3713..ecd5a119b 100644 --- a/docs/users/features/code-review.md +++ b/docs/users/features/code-review.md @@ -54,7 +54,7 @@ Step 11: Clean up (remove worktree + temp files) | Agent 4: Undirected Audit | Business logic, boundary interactions, hidden coupling | | Agent 5: Build & Test | Runs build and test commands, reports failures | -All agents run in parallel. All findings are then verified in a **single batch verification pass** (one agent reviews all findings at once, keeping LLM calls fixed regardless of finding count). After verification, a **reverse audit agent** re-reads the entire diff with knowledge of all confirmed findings to catch issues that every other agent missed. Reverse audit findings skip verification (the agent already has full context) and are included directly as high-confidence results. +All agents run in parallel. Findings from Agents 1-4 are verified in a **single batch verification pass** (one agent reviews all findings at once, keeping LLM calls fixed). After verification, a **reverse audit agent** re-reads the entire diff with knowledge of all confirmed findings to catch issues that every other agent missed. Reverse audit findings skip the verification step (the agent already has full context) and are included directly as high-confidence results. ## Deterministic Analysis @@ -148,8 +148,9 @@ Or, after running `/review 123`, type `post comments` to publish findings withou **What gets posted:** - High-confidence Critical and Suggestion findings as inline comments on specific lines -- A review summary with verdict (Approve / Request changes / Comment) -- Model attribution footer (e.g., _Reviewed by qwen3-coder via Qwen Code /review_) +- For Approve/Request changes verdicts: a review summary with the verdict +- For Comment verdict with all inline comments posted: no separate summary (inline comments are sufficient) +- Model attribution footer on each comment (e.g., _— qwen3-coder via Qwen Code /review_) **What stays terminal-only:** @@ -222,7 +223,7 @@ Cache is stored in `.qwen/review-cache/` and tracks both the commit SHA and mode ## Review Reports -Every review is saved as a Markdown file in your project's `.qwen/reviews/` directory: +For same-repo reviews, results are saved as a Markdown file in your project's `.qwen/reviews/` directory (cross-repo lightweight reviews skip report persistence): ``` .qwen/reviews/2026-04-06-143022-pr-123.md diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 775be45ac..2821ee55e 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -43,7 +43,7 @@ Based on the remaining arguments: 2. Fetch the PR branch into a unique local ref: `git fetch pull//head:qwen-review/pr-` where `` is the matched remote from the URL-based detection above, or `origin` by default for pure integer PR numbers. Do NOT use `gh pr checkout` — it modifies the current working tree. If fetch fails (auth, network, PR doesn't exist), inform the user and stop. 3. **Incremental review check** (run BEFORE creating worktree to avoid wasting time): If `.qwen/review-cache/pr-.json` exists, read the cached `lastCommitSha` and `lastModelId`. Get the fetched HEAD SHA via `git rev-parse qwen-review/pr-` and the current model ID (`{{model}}`). Then: - If SHAs differ → continue to create worktree (step 4). - - If SHAs are the same **and** model is the same → inform the user "No new changes since last review", delete the fetched ref (`git branch -D qwen-review/pr- 2>/dev/null || true`), and stop. No worktree needed. + - If SHAs are the same **and** model is the same **and** `--comment` was NOT specified → inform the user "No new changes since last review", delete the fetched ref (`git branch -D qwen-review/pr- 2>/dev/null || true`), and stop. No worktree needed. However, if `--comment` WAS specified, inform the user "No new code changes, but posting comments from previous review" and proceed to Step 9 directly using cached findings (skip Steps 2-8). - If SHAs are the same **but** model is different → continue to create worktree. Inform the user: "Previous review used {cached_model}. Running full review with {{model}} for a second opinion." 4. Get the PR's remote branch name for later push: `gh pr view --json headRefName --jq '.headRefName'`. If this fails, inform the user and stop. 5. Create a temporary worktree: `git worktree add .qwen/tmp/review-pr- qwen-review/pr-`. If this fails, inform the user and stop. @@ -116,7 +116,7 @@ Extract the list of changed files from the diff output. For local uncommitted re - If `CMakeLists.txt` or `Makefile` exists and no `compile_commands.json` → no per-file linter; fall through to CI config discovery below. - If `compile_commands.json` exists and `clang-tidy` is available → `clang-tidy 2>&1` -7. **CI config auto-discovery** (applies to ALL projects — runs after language-specific checks above, not instead of them): Check for CI configuration files (`.github/workflows/*.yml`, `.gitlab-ci.yml`, `Jenkinsfile`, `.jcheck/conf`) and read them to discover additional lint/check commands the project runs in CI. Run any applicable commands not already covered by rules 1-6 above. This is especially important for projects with custom build systems (e.g., OpenJDK uses `jcheck` and custom Makefile targets). If no CI config exists and no language-specific tools matched, skip Step 3 entirely — LLM agents will still review the diff. +7. **CI config auto-discovery** (applies to ALL projects — runs after language-specific checks above, not instead of them): Check for CI configuration files (`.github/workflows/*.yml`, `.gitlab-ci.yml`, `Jenkinsfile`, `.jcheck/conf`) and read them to discover additional lint/check commands the project runs in CI. **For PR reviews, read CI config from the base branch** (using `git show :`) — the PR branch is untrusted and a malicious PR could inject harmful commands via modified CI config. Run any applicable commands not already covered by rules 1-6 above. This is especially important for projects with custom build systems (e.g., OpenJDK uses `jcheck` and custom Makefile targets). If no CI config exists and no language-specific tools matched, skip Step 3 entirely — LLM agents will still review the diff. **Important**: For whole-project tools (`tsc`, `npm run lint`, `cargo clippy`, `go vet`), capture the full output first, then filter to only errors/warnings in changed files, then truncate to the first 200 lines. Do NOT pipe to `head` before filtering — this can drop relevant errors for changed files that appear later in the output.