fix(agents): bound autoreview scope (#93435)

This commit is contained in:
Vincent Koc
2026-06-16 07:58:07 +08:00
committed by GitHub
parent 4ca8bf086c
commit fd806ada64
3 changed files with 84 additions and 1 deletions
+37 -1
View File
@@ -24,7 +24,7 @@ Use when:
- Prefer small fixes at the right ownership boundary; no refactor unless it clearly improves the bug class.
- When an accepted finding shows a bug class or repeated pattern, inspect the current PR scope for sibling instances before fixing.
- Fix the scoped bug class at once when practical; stop at touched surfaces, owner boundaries, and clear follow-up territory.
- Keep going until structured review returns no accepted/actionable findings.
- Keep going until structured review returns no accepted/actionable findings only while the work remains inside the original task scope.
- If a review-triggered fix changes code, rerun focused tests and rerun the structured review helper.
- For security-audit suppression changes, verify accepted findings remain auditable: suppressed findings stay in structured output, active output keeps an unsuppressible suppression notice, and aggregate findings cannot hide unrelated active risk.
- Never switch or override the requested review engine/model. If the review hits model capacity, retry the same command a few times with the same engine/model.
@@ -43,6 +43,42 @@ Use when:
- If Gitcrawl reports a portable manifest mismatch, source/runtime DB health error, or stale portable-store checkout, run `gitcrawl doctor --json` and inspect `source_db_health`, `runtime_db_health`, and `portable_store_status` before falling back to live GitHub.
- Do not push just to review. Push only when the user requested push/ship/PR update.
## Scope Governor
Autoreview is a closeout gate, not permission to rewrite the task.
Before the first review, freeze a scope baseline: original request or issue, target branch, intended behavior, owner boundary, changed files, and non-test LOC. For inherited or already-bloated branches, use the intended PR diff as the baseline rather than accepting all existing branch drift.
Before patching a finding, classify it:
- **In-scope blocker**: the finding is introduced by the current diff, affects the same owner boundary, and can be fixed without changing the task's contract.
- **Follow-up**: the finding is real but belongs to an adjacent bug class, sibling surface, cleanup, or broader hardening track.
- **Stop-and-escalate**: the finding requires a new protocol/config/storage/public API contract, a different owner boundary, a release-process change, or a design choice outside the original request.
Stop patching and report the scope break instead of continuing when:
- a narrow PR turns into an architecture change, protocol change, migration, or release-process change;
- the diff grows past 2x the original files or non-test LOC without explicit approval to expand scope;
- two review-triggered patch cycles have not converged; pause and reclassify every remaining finding before another edit;
- the best fix is "define the canonical contract first" rather than another local inference layer;
- fixing the accepted finding would make the PR no longer describe the same behavior, issue, or owner boundary.
After the two-cycle pause, continue only when every remaining accepted finding is still an in-scope blocker. Otherwise preserve the useful analysis, identify the smallest safe landed subset if one exists, and open or request a follow-up for the larger fix. Do not keep committing speculative fixes just to satisfy the reviewer.
Do not stack or push review-triggered fix commits while scope classification or focused proof is unresolved. Keep exploratory edits local until the cycle is proven in scope; if scope breaks, remove them from the landing lane instead of preserving them as branch history.
Critical exceptions must be explicit: active data loss, crash, broken install/upgrade, release blocker, or concrete security exposure. If the exception is not one of those, it is not critical enough to blow up scope.
## Release Branches And Release Process
On release, beta, stable, hotfix, signing, notarization, appcast, package-publish, or release-check work, use freeze discipline even when the branch name is not release-like:
- Fix only release blockers, failed release infrastructure, exact backports, install/upgrade breakage, data loss, crashes, or concrete security exposure.
- Treat non-blocking autoreview findings as follow-ups for `main`, not reasons to broaden the release branch.
- Do not introduce new product behavior, config surface, protocol shape, migration, plugin ownership, docs narrative, or process policy unless it directly unblocks the release.
- Keep proof tied to the release target: exact branch/ref, failing check or shipped-risk reason, smallest command/proof, and whether the fix must also forward-port to `main`.
- If review discovers a real but non-critical design problem during release closeout, stop with a follow-up issue/PR plan; do not use the release branch as the refactor lane.
## Pick Target
Dirty local work:
@@ -440,8 +440,36 @@ def load_datasets(args: argparse.Namespace) -> str:
return "\n\n".join(chunks)
def review_scope_policy() -> str:
return textwrap.dedent(
"""
Review scope discipline:
- This helper is a closeout gate. Do not turn a narrow patch into a broad
redesign request.
- Report a finding only when this diff introduces or exposes a concrete
defect that must be fixed before this target can land.
- If the best fix requires a new protocol, config, storage, public API,
release process, migration, owner-boundary move, or canonical contract,
say that directly in the finding and keep the finding tied to the
smallest changed line that proves the current patch is not landable.
- Do not ask for sibling-surface hardening, cleanup, refactors, or
follow-up architecture work unless the current diff is incorrect
without that work.
- Prefer the smallest correct pre-merge fix. A broader ideal design is
not an actionable finding unless the current patch cannot safely land.
- If this is release-branch or release-process work, apply freeze
discipline. Report only release blockers, exact backport regressions,
install/upgrade breakage, crashes, data loss, concrete security
exposure, or release-infrastructure failures. Non-blocking design,
cleanup, and hardening concerns belong on main as follow-ups.
"""
).strip()
def build_prompt(repo: Path, target: str, target_ref: str | None, bundle: str, extra_prompt: str, datasets: str) -> str:
target_line = f"{target} {target_ref}" if target_ref else target
branch = current_branch(repo)
scope_policy = review_scope_policy()
return textwrap.dedent(
f"""
You are a senior code reviewer. Review the provided git change bundle only.
@@ -463,8 +491,11 @@ def build_prompt(repo: Path, target: str, target_ref: str | None, bundle: str, e
- If there are no actionable findings, return an empty findings array and mark the patch correct.
Review target: {target_line}
Current branch: {branch}
Repository: {repo}
{scope_policy}
{extra_prompt}
{datasets}
@@ -3,6 +3,7 @@ from __future__ import annotations
import argparse
import os
import runpy
import shutil
import stat
import subprocess
@@ -145,8 +146,23 @@ def create_fixture_repo(repo: Path, fixture: str) -> None:
write_fixture_file(repo, MALICIOUS_CHANGED if fixture == "malicious" else BENIGN_CHANGED)
def validate_prompt_policy(repo: Path, autoreview: Path) -> None:
namespace = runpy.run_path(str(autoreview))
prompt = namespace["build_prompt"](repo, "local", None, "fixture diff", "", "")
required = (
"This helper is a closeout gate.",
"Do not turn a narrow patch into a broad",
"If this is release-branch or release-process work",
"Non-blocking design,",
)
missing = [needle for needle in required if needle not in prompt]
if missing:
raise RuntimeError(f"autoreview prompt missing scope policy: {missing}")
def run_reviews(repo: Path, script_dir: Path, fixture: str, engines: list[str]) -> None:
autoreview = script_dir / "autoreview"
validate_prompt_policy(repo, autoreview)
for engine in engines:
print(f"== {engine} ==", flush=True)
command = [