pre-push-review

Pre-Push Review

Run the same review the automated review-pr-opened GitHub Action will run — catch issues before CI does. The #1 recurring pattern on amplifier-resolve: stale mock patch targets after module splits. When a monolithic module is split into a package, tests that patch("old.module._X") or from old.module import _X must become patch("new.module.submodule._X"). Patching a re-export is silently ineffective — the call inside the real module uses its own local binding, not the re-export.

Success artifact: A list of Required / Advisory findings that match what CI would flag, returned before the branch is pushed.

Inputs

• ``: Branch name to review (defaults to current branch). Optionally include the repo path.

Steps

1. Orient on the diff

git diff origin/main...HEAD --name-only  # files changed
git diff origin/main...HEAD              # full diff for orientation

Identify which source modules changed and whether any module splits occurred (a .py file deleted and a same-name / directory created).

Success criteria: You know which files changed and whether module splits happened.

---

2. Check stale patch targets (the #1 issue)

For every split module, grep ALL test files for references to the old path:

# Replace <old_module> with the split module name (e.g. cli, routes.instances)
grep -rn "patch.*amplifier_resolve\.<old_module>\." tests/
grep -rn "from amplifier_resolve\.<old_module> import _" tests/

Rules:

• patch("a.b.symbol") is correct ONLY if symbol ACTUALLY LIVES in a.b — not just re-exported there
• Patching a re-export binds the mock in the wrong namespace; the production code path is untouched
• Correct target: patch("the.module.that.calls.symbol") — where it is IMPORTED, not defined

Success criteria: Every stale patch target identified with exact file:line references.

---

3. Check dead imports with noqa suppressors

grep -rn "# noqa: F401" src/

For each hit: is the import legitimately kept (backward-compat re-export, TYPE_CHECKING guard)? If not, it is silencing a real lint warning — flag it.

Success criteria: Every # noqa: F401 is justified or flagged.

---

4. Check fixture env-var safety

grep -rn "os\.environ\[" tests/
grep -rn "os\.environ\.pop" tests/

Manual os.environ[key] = value in fixtures leaks the env var if the test raises before teardown. The correct pattern is monkeypatch.setenv(key, value) — pytest handles cleanup.

Success criteria: No unguarded os.environ mutations in test fixtures.

---

5. Full review using pr-review.md criteria

Fetch the review criteria, then delegate a full review.

Fetch: https://raw.githubusercontent.com/microsoft/amplifier-app-actions/main/context/pr-review.md

Delegate to superpowers:code-reviewer with:

• The pr-review.md content as the review instructions
• The branch name and what the PR is supposed to do
• Explicit instruction to read FULL files, not just the diff
• Explicit instruction to grep for all callers/tests of any changed interface

Key pr-review.md criteria:

• Read full files, not just diffs — the diff orients; the full file is the review
• Verify through code, not proxies — PR descriptions and agent summaries are hypotheses
• Five checks: Necessity, Layer fit, Pattern, Correctness, Calibration
• Patch target correctness: for every unittest.mock.patch() in changed test files, verify the target is the module where the symbol actually lives after any rename/split

Execution: Delegate to superpowers:code-reviewer, model_role=critique

Success criteria: All findings returned with file:line references.

---

6. Report

Group findings by severity:

## Pre-Push Review: <branch>

### Required (will trigger CI CHANGES_REQUESTED)
1. `file:line` — description + exact fix

### Advisory (should fix, won't block merge)
1. `file:line` — description

### Verdict: READY TO PUSH  /  NEEDS FIXES FIRST

Success criteria: User receives actionable list; verdict is explicit.