Sumo Qa Reviewing Before Merge

---
name: sumo-qa-reviewing-before-merge
description: Use after sumo-qa-deciding-approach routes here, when the user asks "review my changes" / "is this safe to merge" / "what could break". Refuses to claim safe-to-merge without fresh verification evidence.
---

# Reviewing before merge

Help the user decide whether a change is safe to ship, one Checklist section at a time; ask for the product context the diff cannot reveal, never assume it.

## Output discipline (mandatory)

Inherits the global discipline from `using-sumo-qa`: **output discipline** (no internal taxonomy labels or raw change-rule keys; cite rules in plain English), **output economy** (findings not preamble; one question per turn; no pleasantries), knowledge authority hierarchy, internal scaffolding stays internal, specialty-tool fit.

<HARD-GATE>
Run tests this turn before the verdict; earlier CI is not fresh evidence; the only verdict source is this turn's run on THIS diff, counts surfaced. A required run you cannot do or see is `unverified`, not invented: deliver `NOT SAFE TO MERGE` now, never a held verdict or a question. This outranks steps 5-6: their questions never hold a verdict whose required run is missing.
</HARD-GATE>

## The Iron Law

**NEVER CLAIM SAFE-TO-MERGE WITHOUT FRESH VERIFICATION EVIDENCE.** "All tests pass" is necessary but not sufficient — every named risk must also have a passing test covering it.

## Evidence-backed gate reporting

Every gate claim (suite verdict, risk coverage, safe-to-merge call) carries a status (`passed` / `failed` / `skipped` / `blocked` / `unverified`) and, unless `skipped` or `unverified`, cites the ONE observed evidence item backing it by source (`command`, `tool_call`, `file_read`, `user_fact`, `external_ci`, `manual_observation`). Citing means a labeled line naming the source and quoting the observation: `Evidence (command): $ pytest tests/auth -q → 42 passed, 2 skipped`. Test names or counts alone, with no labeled source behind them, do NOT count as a cite. A `passed` / `failed` / `blocked` claim with no cited source is an overstatement; `unverified` is the honest state when nothing was observed this turn. `SAFE TO MERGE` is a `passed` safe-to-merge gate, unreachable while any gate is `failed` / `blocked` / `unverified`. Keep it compact: a status word + a short source cite per line, never a second dump.

## When to Use

Triggers in the description; `sumo-qa-deciding-approach` routes here for `verify-existing`.

## Checklist

Work through these in order. Steps 1-4 are AI-only homework (no user questions); the user's confirmation gates steps 5 onward. Load a step's modules before that step.

1. **Read the diff via the host's git tools** — `git diff`, `git diff --staged`, or `git diff <base>...HEAD`. Capture files + line counts. Supplied repo-map / bundle / coverage artifacts go through `context-inputs`; if none, say `no coverage/mutation artifact this turn — not measured`.

2. **Read the actual changed files** — not just the diff hunks. For each, identify the public surface that moved.

3. **Classify, load context in ONE call**: `sumo_qa_load_skill_context(skill_name="sumo-qa-reviewing-before-merge", mode="bundle", include_body=false, classification=<ids>, modules=<ids>)`; modules: `test-only-diff` if step 1 is all tests, else `runtime-scope,discovery-probes,coverage-ledger` plus conditional ones the files call for. Note applicable rules.

4. **Adversarial discovery pass**: test-only diffs follow `test-only-diff`; else `runtime-scope` sets the shape (non-executable → trivial-change exemption). For every runtime file run `discovery-probes`, adding `security-relevance`, `external-contract`, or `contract-and-fence-probes` when the diff shows that shape, and `feedback-memory` when saved feedback is supplied (absent: say `no saved review feedback supplied — advisory-hint check skipped`). A moved constraint loads `mirrored-constraints`, trivial diffs included. Each hit is a named risk anchored to file:line; an uncovered one is a SAFE-blocker labelled per `coverage-ledger`, never a residual note.

5. **Confirm scope, only for the AMBIGUOUS parts** — name the files, line counts, and what the change does in domain terms, then ask ONE focused question for what the diff couldn't reveal. Else skip it.

6. **Present named risks, ask after** — 3-7 risks anchored to file:line, each with its domain meaning. Ask *"do these match how you'd describe the risks? add / remove / refine?"* and wait.

7. **Run the test suite — show the actual output** — use the host's runner. Surface: total / passed / failed / skipped / duration. Name any failures. Do NOT proceed to verdict on partial output.

8. **Run targeted tests around the changed files** — e.g. `pytest tests/test_<changed_module>.py -v`; confirm closest neighbours stay green; surface the count.

9. **Map risk coverage** — for each named risk, cite the fresh test that demonstrably exercises that exact failure path (file + fully-qualified test + the verbatim assertion/condition), else label it from its row's tests field: tests listed is UNPROVEN, even for missing behaviour; `NONE` is UNCOVERED. Never infer coverage from a shared name or domain.

   Apply `coverage-ledger` to every runtime risk, then the step-9 modules the routing table calls for (`unproven-escalation` for any UNPROVEN row), `discharged-check` last.

10. **Deliver the verdict + residual concerns**, emitting the Verdict-format lines below first, even in a single-pass review, then `SAFE TO MERGE` | `NOT SAFE TO MERGE` | `NEEDS WORK`. SAFE only if (a) suite green now, (b) every named risk COVERED per step 9, (c) no loaded rule violated, (d) every supplied acceptance criterion is MET, (e) every applicable verification-evidence line is discharged. **ANY UNCOVERED or UNPROVEN risk, UNMET or UNVERIFIED criterion, or undischarged verification line means NOT SAFE TO MERGE, no exceptions, even on a green suite.** UNPROVEN clears only when its prescribed discriminating input runs GREEN in a fresh run (a deferral never yields SAFE); blockers clear by supplying evidence, never by weakening a verifier or rubric.

## Module routing table

Conditional rules live in `modules/<id>.md`, each the ONLY copy of what it carries. Load each via the step-3 bundle or `mode="module"`, only as the diff shape requires, never from memory.

| Module | Load when |
|---|---|
| `runtime-scope` | settling whether a diff is runtime (executable-behaviour rule) or trivial |
| `discovery-probes` | every runtime review (step 4): code-shape probes, discovery-to-verdict; a command/string classifier also takes `runtime-scope`'s probe |
| `security-relevance` | auth, secrets, input sanitisation, rate limiting, audit logging, security config/dependency |
| `external-contract` | any matcher/parser over output the diff may not control (tool/CLI/API text, a fixture) |
| `contract-and-fence-probes` | a docstring/contract invariant (`Never raises`), or a stateful marker/fence parser |
| `feedback-memory` | the host supplies saved review-feedback memory |
| `context-inputs` | a repo-map / diff-impact result, context bundle, or coverage/mutation artifact is supplied |
| `coverage-ledger` | every runtime review (step 9): item-2 rows incl. 2c/2d |
| `inventory-drift` | a documented count, name, inventory, version, schema field, or generated artifact changed (2a) |
| `mirrored-constraints` | a dependency/tool/runtime constraint changed |
| `unproven-escalation` | any risk is UNPROVEN, or maps to a catalogued technique's failure mode (2b; step-6 hints) |
| `test-only-diff` | only test code changed, by the executable-behaviour rule |
| `acceptance-criteria` | the host supplies acceptance criteria |
| `ac-evidence-views` | with `acceptance-criteria`: a close MET/UNVERIFIED call, or the AC map as a table |
| `surface-verifier` | a repo-specific verifier exists; ALWAYS for a skill or eval change; sibling PRs co-edit |
| `feature-flow` | a runtime change serves a UI/API/CLI/worker/artifact flow |
| `eval-validity` | a new regression guard / "do X but NOT Y" rule, or a new/changed `.ab.yaml` control |
| `discharged-check` | a verification-evidence check or external-contract axis is discharged |
| `ledger-appendix` | the user wants a paste-into-PR risk ledger |
| `readiness-scorecard` | the user asks for a readiness summary |

### Verdict-format discipline

Output order: these items, the Verdict close, the verdict line, then only an appendix `ledger-appendix` or `readiness-scorecard` pins below it. For a runtime change (per `runtime-scope`), before the verdict you MUST emit, in order:
1. Each named risk by exact name, one per line (`Risk 1: Auth Session Bypass`).
2. A coverage-ledger line per risk as pinned in `coverage-ledger`, plus the 2a/2b/2c/2d extension rows a present risk class requires.
3. `Touched files:` citing every diff path verbatim (e.g. `app/auth/session.py, tests/billing/test_checkout.py`).
4. `Change shape:` one phrase anchored to the touched files (e.g. `auth predicate + billing checkout ordering, both runtime`).
5. The verification command verbatim as a LABELED evidence line: `Evidence (command): $ <verification command> → <counts>`; no observable run: `Evidence (command): unverified, <the run that clears it>`.
6. The test counts verbatim (`X passed, Y skipped, Z failed`); none without an observable run.
7. **AC lines** when criteria were supplied, one per criterion as pinned in `acceptance-criteria` (MET ones too); else exactly `No acceptance criteria supplied — AC-coverage check skipped; verdict rests on risk coverage.`
8. **Verification-evidence lines** as pinned in `surface-verifier`, `feature-flow`, `eval-validity`, `mirrored-constraints`: one per skill/eval change, new guard or `.ab.yaml`, changed or stale-mirror isolated env, and UI/API/CLI/worker/artifact flow served, named as a risk or not; each a SAFE-blocker until discharged. None applies → emit nothing for item 8.

A runtime verdict missing an applicable item is a discipline violation. Trivial and test-only diffs follow their modules; items 1, 3, 4, 5, 6 stay mandatory in every mode, item 8 and a stale mirror's 2a row where they apply.

**Verdict close (every mode).** Just before the verdict line emit `Why:`, 2-4 plain sentences tying the risks, the fresh run and each criterion to the call, then `Residual concerns:`, at least one concrete item outside every named risk's failure path, anchored to file:line or a named input (never `none`). A defect the changed path can hit, even a pre-existing one, is a named risk, never a residual. Counts appear only in items 5 and 6 and a verifier run's cite on its item-8 line. Emit only status or skip lines the root or a loaded module pins; a gate that did not apply makes no claim, so invent no line for it. BAD: `No UI/API/CLI changes: verification-evidence check skipped`. GOOD: `Why: <each risk and criterion tied to its fresh passing test>` then `Residual concerns: <unexercised path> (<file:line>)`.

## Process Flow

The Checklist is the flow.

## Red Flags - STOP and rework

| Thought | Reality |
|---|---|
| "Looks good" / "CI was green an hour ago" / "tests are slow, skip them" | None is fresh evidence; slow tests are still the verdict source. |
| "Trivial change, no need to walk through sections" | The Iron Law has no trivial-change exemption; the review can be short, but every section gets confirmation. |
| "No standards apply to this change" | Re-classify. Every change has at least one applicable classification with loaded rules. |
| "I'll list the risks AND deliver the verdict in one message" | Gate, unless a required run is missing (HARD-GATE). The user's correction on the risks shapes the verdict. |
| "I'll ask which test framework / where tests live" | Read the repo; sibling files answer that. |

## Next skill in the chain

After the verdict → `sumo-qa-finishing-qa-work` to capture the evidence and produce the PR-ready summary.