diff --git a/.agents/skills/autoreview/SKILL.md b/.agents/skills/autoreview/SKILL.md index e0745bde198d..a2ad388eef2f 100644 --- a/.agents/skills/autoreview/SKILL.md +++ b/.agents/skills/autoreview/SKILL.md @@ -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: diff --git a/.agents/skills/autoreview/scripts/autoreview b/.agents/skills/autoreview/scripts/autoreview index a5ea1d56d66c..7e5f142c18a1 100755 --- a/.agents/skills/autoreview/scripts/autoreview +++ b/.agents/skills/autoreview/scripts/autoreview @@ -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} diff --git a/.agents/skills/autoreview/scripts/test-review-harness.py b/.agents/skills/autoreview/scripts/test-review-harness.py index 7f1baaa805ec..364568e9b68c 100644 --- a/.agents/skills/autoreview/scripts/test-review-harness.py +++ b/.agents/skills/autoreview/scripts/test-review-harness.py @@ -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 = [