api-compat-review

Files (4)

• SKILL.md
• DETECTION-PLAYBOOK.md · references/
• QUALITY-FILTER.md · references/
• REVIEW-VOICE.md · references/

SKILL.md (first lines)

# API Compatibility Review

A specialised lens layered on [`pr-review`](https://github.com/priyank766/OpenSource-SKILL/blob/main/skills/pr-review/SKILL.md). Run the general
procedure first; run this when the diff touches anything a downstream consumer can observe.

**Read first:** [`DETECTION-PLAYBOOK.md`](references/DETECTION-PLAYBOOK.md) ·
[`QUALITY-FILTER.md`](references/QUALITY-FILTER.md) ·
[`REVIEW-VOICE.md`](references/REVIEW-VOICE.md)

> **Where the time goes.** Finding the flaw is the review. The playbook above is the
> general flaw-hunting procedure; this file is the domain-specific lens on top of it.
> Scoring and writing the comment are the last ten minutes, not the work.

**Severity floor: 7.5** — lower than the suite default of 8.0. The asymmetry justifies it.
A noisy comment costs one maintainer thirty seconds. A missed break reaches every
downstream user simultaneously the moment the release publishes, and cannot be un-shipped:
registries are append-only, a yanked version is still in someone's lockfile, and the fix
arrives as *another* release those users must now also adopt. Under that asymmetry, a
false positive is cheap and a false negative is not.

---

## The governing question

> **What breaks for someone who upgrades without reading the changelog?**

Not "is this change reasonable" — most breaks are reasonable. Not "is the new API better" —
it usually is. The only question is what happens to the person who bumps a caret range,
runs their suite, and gets a red build or, worse, a green one with different behaviour.

Everything below exists to answer that one question with evidence rather than intuition.

---

## Why this exists

Compatibility damage is invisible at review time and expensive afterwards. A removed
keyword argument produces a clean diff, a passing CI run, and — three weeks after release —
a stream of issues that all look different and all have the same cause. The maintainer
pays for it in triage, not in review.

### What each role gets out of it

| Role | Time this gives back |
| :--- | :--- |
| **Maintainer** | The surface diff is computed before merge, not reconstructed from bug reports. One pass over `apidiff`/`cargo-semver-checks` output replaces the weeks of issue triage that follow an unnoticed break, plus the emergency patch release and the apology thread. |
| **Contributor** | You find out your change is breaking while you can still design around it, instead of after a maintainer closes the PR with "this can't land before 3.0". |
| **Project owner** | A written, checkable definition of what the project promises. Version numbers start meaning something, and "is this a minor or a major?" stops being decided by whoever cuts the release. |

---

## Procedure

```mermaid
flowchart TD
    A[Start] --> B[1. Establish the public surface]
    B --> C[2. Diff base vs head across that surface]
    C --> D[3. Classify each change with the taxonomy]
    D --> E{Breaking?}
    E -->|"no, internal only"| F[Drop it silently]
    E -->|"yes"| G[4. Measure blast radius]
    G --> H[5. Check for a migration path]
    H --> I{Shim, changelog, docs present?}
    I -->|"all present"| J[Score the deprecation window]
    I -->|"any missing"| K[Finding at 9.0 or above]
    J --> L[6. Frame in review voice]
    K --> L
    L --> M[7. Get explicit approval, then post]
```

---

## 1. Establish the public surface first

A change is breaking only relative to a contract. Find the contract before judging the
diff; otherwise you will flag internal churn and miss a real break in a file that did not
look public.

```bash
rg -n "semver|semantic version|stability|public API|breaking" README.md docs/ CONTRIBUTING.md
ls docs/versioning.md docs/stability.md POLICY.md 2>/dev/null
cat CHANGELOG.md | head -60        # how have breaks been handled before?
git tag --sort=-v:refname | head   # does this project even bump majors?
```

Then read the language's own marker of intent:

| Ecosystem | Where "public" is declared |
| :--- | :--- |
| Python | `__all__`, absence of a leading `_`, what `docs/` imports, `py.typed` |
| JS/TS | `package.json` `exports` map, generated `.d.ts`, `@public` / `@internal` TSDoc tags |
| Go | Exported identifiers outside any `internal/` directory — the compiler enforces this one |
| Rust | `pub` vs `pub(crate)`, `#[doc(hidden)]`, `#[non_exhaustive]` on enums and structs |
| Java | `public` modifier plus `module-info.java` `exports`, `@Internal` annotations |
| C/C++ | Installed headers only; anything under `src/` or `detail/` is not surface |

`#[non_exhaustive]`, `@internal`, and `internal/` are the project telling you in advance
which changes it reserves the right to make. Respect them — flagging a change the project
explicitly reserved is a false positive, and a costly one for your credibility.

**No stated policy?** Assume anything importable, documented, or shown in a README example
is public, and say so in the review rather than silently assuming it:

> There's no stability policy I could find, so I'm treating anything reachable from
> `import foo` as public — `foo.Client.retries` is used in the quickstart, which is why I
> think dropping it needs a shim. If that's not the intent, a line in the README would
> settle it for future PRs too.

---

## 2. Detection procedure

Start with the raw diff, restricted to the paths you established as public.

```bash
git diff upstream/main...HEAD -- src/mypkg/ 'src/**/*.pyi' docs/api/
git diff upstream/main...HEAD -- ':!**/internal/**' ':!**/_*.py' ':!**/*_test.go'
```

From priyank766/OpenSource-SKILL, Apache-2.0.

More from priyank766/OpenSource-SKILL