# Myco:community Pr Review

> Use this skill when reviewing or merging any community PR in unifi-mcp — even if the user just says "take a look at this PR" or "can we merge this." Covers the quality gate checklist (f-string logger ban, Ruff lint, validator registration, doc site update ordering), the fork-edit model for trusted contributors, org-fork push limitations, dual-subagent review, PR body standards, live smoke tests, mutating cycles, the unresponsive-first-time-contributor fork-edit exception, and the close-and-redirect pattern. Also covers community infrastructure setup (.github/ health files, issue routing, bug report template design) and evidence-first bug triage protocol. Apply this skill before approving any externally-authored PR, before running the merge command, when auditing merged PRs retroactively, and when setting up community engagement infrastructure.

- Skill: `sirkirby/myco-community-pr-review` (Agent Skill)
- Install (CLI): `npx skillmds@latest add sirkirby/myco-community-pr-review`
- Raw SKILL.md: https://api.skillmd.com/api/skills/sirkirby/myco-community-pr-review/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: AI & ML
- Author: sirkirby (https://skillmd.com/u/sirkirby)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/sirkirby/myco-community-pr-review

---


# Community PR Review and Merge

Community PRs go through a fixed quality checklist before merge. For trusted contributors
(level99 has 7+ merged PRs), the maintainer commits fixes directly to the contributor's fork
branch rather than requesting round-trip revisions — this preserves attribution while eliminating
latency. An exception exists for first-time contributors who are historically unresponsive: when the fix is
trivial (ruff format, simple doc change), apply fork-edit rather than request changes. This skill documents
the full workflow from first look to merge, including technical validation for PRs that touch UniFi API tool implementations.

## Prerequisites

- PR is open and CI workflow state is understood (see Step 0b for first-time contributor gotcha)
- You have push access to the contributor's fork (needed for the fork-edit model)
- `AGENTS.md` is current — it is the canonical source for hard bans
- For first-time contributors: check prior PR history to assess responsiveness (72+ hour silence threshold for fork-edit exception eligibility)
- For PRs touching tool implementations or API handlers: a **live UniFi controller** must be reachable to run smoke tests
- **Makefile three-layer generation pipeline:** `make generate` regenerates committed artifacts, `make check-generated` checks for drift, `make pre-commit` runs the full chain (format → generate → lint → test → drift checks). Run `make pre-commit` before opening any PR that modifies generated artifacts.

---

## Step 0b — First-Time Contributor CI Auth Gate (Critical Gotcha)

When a first-time contributor opens a PR from a fork, GitHub queues all CI workflows with status
`action_required` silently. The workflows do NOT run automatically. No error is shown to the
contributor — they see a blank CI box in the PR.

**You must manually authorize the workflow run** in the GitHub UI:

1. Go to the PR Actions tab
2. Scroll to the pending workflow(s)
3. Click "Approve and run" on each queued workflow

Do this before asking the contributor to make changes or before running your own review tests.
Without this step, you will review against stale code or outdated test results, and the
contributor will believe their PR is broken when it's just the CI gate.

This gotcha affects every first-time contributor, including level99 on their first PR.

---

## Subagent Decomposition (For Complex PRs)

For PRs with significant code changes or security implications, split the review across two
subagents rather than doing a single-pass review:

1. **Code review subagent** — correctness, security, quality gates (Gates 1–4 below)
2. **Test coverage subagent** — test completeness, coverage gaps, test pattern compliance

Before dispatching either, check out the branch locally and run `git log origin/main..HEAD`
to enumerate commits. This gives both subagents a shared commit list for scoped analysis.

The dual-subagent split was established for complex ACL PRs — it catches both code correctness
issues and test coverage gaps that a single-pass review misses.

---

## Step 1 — Run the Quality Gate Checklist

First, classify the PR type (Gate 0), then work through the applicable gates in order.

### Gate 0: PR Type Classification — Routes the Checklist

Determine whether this is a **feature addition PR** or a **governance/structural refactor PR**
before running any other gate.

**Feature addition PR** — adds new tools, managers, or capabilities
→ Apply the **design-fit criterion first**: does the new tool or manager belong in this codebase,
or does it duplicate existing functionality / violate the intended scope? A PR that passes all
mechanical gates but adds a tool that's outside the project's design intent should be redirected
(Principle #6) rather than merged. Only after confirming design fit should you run Gates 1–4.

**Precedent — decline generic accessors in favor of typed tools:** A generic section-and-dictionary settings accessor (arbitrary read/write across controller settings) was declined as a transport shortcut — it bypasses per-field validation, redaction, and field-symmetry governance. Prefer a typed tool per setting.

**Governance/structural refactor PR** — reorganizes field definitions, introduces shared Pydantic
models, changes base class hierarchy, or implements a field-symmetry sub-issue
→ Run the structural-correctness path instead:

1. **Pydantic inheritance correct?** — If a shared base model is introduced, does it accurately
   represent the common fields? Are subclass fields genuinely distinct from the base?
2. **Field coverage complete?** — Does the shared model cover all field variants used by the
   resource's create, read, list, and update surfaces? No field should be accessible via
   list/read but absent from the shared model.
3. **Type symmetry correct?** — Field types in the shared model must be compatible with both
   read-surface output and create/update input. Name-match alone is insufficient — a field can
   appear in both surfaces but fail silently if types diverge (e.g., `source_macs: list[str]`
   returned by list tools vs. `source_macs: str` accepted by create). This is enforced by the
   field-symmetry CI gate (the field-symmetry pattern, formalised in the field-symmetry milestone
   and rolled out across phases).
4. **No field leakage?** — Fields belonging to one resource variant must not silently appear
   on another through inheritance.
5. **Matches issue spec?** — Compare against the linked GitHub issue. Every scoped item should
   be implemented; nothing out of scope should be added.
6. **Pattern symmetric with AGENTS.md rule?** — The implementation must align with the
   field-symmetry governance rule in `AGENTS.md`, not diverge from it.

Gates 1–4 (f-string logger, Ruff lint, validator registry, doc site, shared validator blast radius) still
apply to governance PRs for any new or modified tool/manager files — but the structural
questions above are the primary gate.

**Why the split matters:** The ACL shared-field-model pilot (level99) was reviewed with
the feature-addition checklist. The structural questions were asked only because the reviewer
recognized the PR type. A structural refactor can pass Gates 1–3 cleanly while still violating
inheritance structure, field coverage, or type symmetry — the feature-addition checklist gives
false confidence on governance PRs.

---

### Gate 1: Lint Checks and F-String Logger — Hard Blocker

**Primary targets:** All files the PR touches. F-strings specifically in `*_manager.py` files.

#### 1A: Ruff Lint Enforcement

Run the full lint gate that CI enforces:

```bash
make lint
```

This invokes Ruff on all modified files and fails on any violations. Do NOT merge a PR with
lint failures. The project uses Ruff as the canonical linter; violations are hard blockers.

If the PR introduces lint errors, request fixes via fork-edit (trusted contributors) or a
review comment (first-time contributors).

**Cross-package import check:** When a PR adds a new cross-package import (e.g., `unifi-network`
importing from `unifi-core`), verify the dependency is declared in `pyproject.toml`. PRs that add
cross-package imports without updating `pyproject.toml` silently skip the `check-pin-alignment` CI
gate, which enforces version floor consistency across packages. If the import is new and no
`pyproject.toml` change is present, flag it as a potential blocker and request an explicit floor
bump to the imported package's minimum version.

#### 1B: F-String Logger — Hard Blocker

**Primary target:** Every `*_manager.py` file the PR touches.

Scan for f-string logger calls and replace any hits with `%s`-style lazy formatting:

```python
# BLOCKED
logger.info(f"Found {count} devices on {network}")

# REQUIRED
logger.info("Found %s devices on %s", count, network)
```

**Why the manager layer is the blind spot:** Tool files (`*_tools.py`) tend to get this right
because they're reviewed more often. Manager files (`*_manager.py`) are where f-string loggers
keep appearing. In past reviews, the tool layer used `%s` correctly but the manager layer
introduced dozens of f-string calls. Always check manager files explicitly.

**Implicit concatenation is invisible to grep:** Adjacent string literals cannot
be reliably caught by automated scripts. This survived a large automated migration in the past
and was only caught by manual review. Scan manually for this pattern when logger calls span lines.

**Full-payload logging promoted to INFO level:** A `logger.debug` call that dumps a full JSON
payload is acceptable at DEBUG level but becomes production noise and data-exposure risk if
promoted to `logger.info`. Watch for this in manager files. All full-payload log calls must use `logger.debug`.

```python
# BLOCKED: full payload at INFO level
logger.info("Firewall policy response: %s", json.dumps(response))

# REQUIRED: full payload at DEBUG level only
logger.debug("Firewall policy response: %s", json.dumps(response))
```

**No-issue-refs ban extends to test docstrings and comment strings:** Hardcoded `#NNN` GitHub
issue or PR numbers embedded in test docstrings, pytest parameterize IDs, or inline comment
strings are banned under the same no-issue-refs rule as source code. These literals survive into
the package wheel and create dangling stale references that are invisible to the f-string logger
scanner. When the PR touches test files, scan them manually for `#\d+` embedded in string
literals and docstrings.

**Why this is a hard ban:** F-string loggers eagerly evaluate all arguments even when the log
level is suppressed. On deployments with debug logging disabled, this creates unnecessary overhead
on every suppressed call.

---

### Gate 2: Pydantic Model Wiring — Silent Failure Risk

**Target:** Any PR introducing a new tool or manager for a domain that has create/update tools.

New domains must define a pydantic model in `packages/unifi-core/src/unifi_core/<server>/models/<domain>.py` and wire it into the tool layer. A domain without a model silently bypasses field validation — unknown or read-only fields pass through unchecked.

Check that each new domain has a corresponding model with `MUTABLE_FIELDS` / `READ_ONLY_FIELDS` frozensets and `to_controller_create` / `to_controller_update` helpers. Verify the tool layer calls `to_controller_update(fields)` (not a raw dict pass-through) for update tools.

**The `to_controller_update` gotcha:** When a model exists but the tool bypasses it and passes raw caller args directly to the manager, the model's field validation is silently skipped. Confirm the tool calls `to_controller_update(fields)` and passes the result, not the original dict.

**Redaction policy must be threaded, not hardcoded:** When a new resource-type PR introduces a resolver or route that returns sensitive fields, verify `redact_sensitive=True` is passed through the call chain from route to serializer as a parameter — not hardcoded as a local default. A resolver that hardcodes redaction behavior instead of accepting it as an argument silently diverges from callers that need the opposite setting. Check this whenever Gate 2 or Gate 3.5 involves a new resource-type PR.

---

### Gate 3: Doc Site Update — Ordering Gate

**Target:** Any PR that adds, renames, or removes tools.

The doc site must be updated as part of the same PR — not as a follow-up. The ordering matters:
the doc site should be updated *after* the tool code is finalized but *before* merge, so the
published docs stay in sync with the merged code at every point in history.

**Note:** `docs/index.html` is a static marketing site HTML file, not a generated artifact.
Marketing site changes must be made and reviewed manually, separate from the tool documentation sweep.

**Static-docs checklist (hand-maintained, not covered by the generator or Gate 3.5's `make check-generated`):**
When a PR adds a new tool, these five files require a manual edit each and are easy to miss because
no CI gate catches them — a past PR missed all five and required a follow-up commit:
- `README.md`
- `docs/ARCHITECTURE.md`
- `apps/network/README.md` (or the relevant app's README)
- `apps/network/docs/tools.md` (or the relevant app's tools doc)
- `plugins/unifi-network/skills/unifi-network/SKILL.md` (or the relevant app's plugin skill file)

---

### Gate 3.5: New-Tool Registration Completeness — Generated Artifact Drift

**Target:** Any PR that adds a new tool function or modifies tool dispatch registration.

A new tool may pass Gates 1–3 while still causing silent failures if it bypasses required registration layers. Verify all three before approving:

1. **Serializer projection** — Confirm the tool's response flows through the project's serialization/projection layer. A tool that returns raw internal objects instead of projected response models silently exposes unexpected types to MCP clients.

2. **Dispatch/action registration** — If the tool category participates in the project's action dispatch layer (for routing tool calls), confirm the new tool is registered. A tool missing from the dispatcher is silently unreachable via that path even when correctly listed in the manifest. **Dispatch-override translator required for mismatched arg names:** when the tool's argument name differs from the manager's kwarg name (e.g., tool exposes `update_data` but the manager method expects `entry_data`), the dispatch registration must include an explicit translator/mapping — otherwise the dispatcher passes the wrong kwarg and the call fails or silently drops the argument.

3. **Generated artifact regen** — Verify `make generate` was run on the PR branch and all committed generated artifacts (manifest, REST docs, schema files) match. Run `make check-generated` locally to confirm no drift. Artifact drift is a silent correctness failure — it does not fail lint but causes behavioral divergence between the code and what clients discover.

**Note:** `make pre-commit` runs the full chain (format → generate → lint → test → drift checks). If the contributor did not run it, `make check-generated` will surface the gap immediately.

---

### Gate 4: Shared Pydantic Model Defaults — Blast Radius Check

**Target:** Any PR that modifies a shared `<Domain>Base` pydantic model in `packages/unifi-core`.

If a PR adds non-`None` defaults to mutable fields on a shared base model, every update tool that uses the model will silently inject those defaults when the caller omits the field — a data-loss bug with blast radius across all tools that import the model.

**Hard blocker pattern:**

```python
# DANGEROUS — non-None default on shared base model field
# (illustrative; substitute the actual <Domain>Base class name from the PR diff)
class <DomainBase>(BaseModel):
    create_allow_respond: bool = False   # silently overwrites on update
    schedule: dict = {"mode": "ALWAYS"}  # silently overwrites on update
```

**The rule:** Non-`None` defaults belong only in create-specific subclasses or create-specific
code paths. Shared base model fields must use `= None`. Any PR that adds non-`None` defaults to
a shared base model field is a **hard blocker**.

Check for it whenever the PR diff touches a `<Domain>Base` class
that both create and update tools inherit from.

---

## Step 1.5 — API-Touching PRs: Live Validation Requirements

For any PR that modifies UniFi MCP tool implementations, fixes API integration bugs, or adds new
handlers, apply additional technical validation before merge. The UniFi controller is stateful
and mock-based tests do not catch real-world edge cases.

### Smoke Test Coverage (All API-Touching PRs)

Run `scripts/live_smoke.py` against a live controller — not a mock — before opening or approving
the PR.

```bash
# Run read-only + preview smoke tests against the network server
# NOTE: The API server is 'unifi-api-server' (not 'unifi-api')
python scripts/live_smoke.py --server unifi-api-server --phase safe

# Run all servers (network + protect + access)
python scripts/live_smoke.py --server all --phase safe
```

**What "passing" means:** each tool returns a structurally valid response; no unexpected errors
or stack traces; list tools return at least the expected schema shape even with no controller objects.

**Independent maintainer smoke is mandatory — contributor's run does not substitute:** Even when a
contributor has already run live smoke tests and embedded results in the PR body, the maintainer
must run a full independent validation suite. Contributor runs confirm the code works in their
environment; maintainer runs confirm it against the project's reference hardware and catch
environment-specific divergence that the contributor cannot reproduce. This is a non-optional
standing rule.

**Gotcha:** A tool already broken before the PR is still your responsibility to flag. Don't
silently skip known-broken tools — note them explicitly in the PR description.

**Gotcha:** Mock tests give false confidence. Response shapes differ between controller versions,
and some fields only appear when specific configuration exists.

**Gotcha:** HA/shadow mode transient failures are environment issues, not code bugs. If live smoke
tests fail with "resource temporarily unavailable" or "sync in progress," verify the HA cluster has
stabilized. Retry after 30–60 seconds. Do not block merge on HA transient failures.

**Gotcha — MCP plugin invokes the published PyPI package, not local branch code.** When you run
tool calls through the MCP plugin in Claude Code (or any MCP client), the plugin loads the
**installed (published) package** — not your local working tree or the PR branch. A fix that
only exists on a branch will not be exercised via the plugin until it is published. Use
`scripts/live_smoke.py` directly (not the plugin) when verifying branch-local changes, and
confirm the fix reaches production by running the plugin after the release tag is pushed.

**Gotcha — worktree default starts the MCP server in eager registration mode, hiding lazy-mode code paths.** A validation probe initially reported reviewed code as unreachable because the worktree registers all tools eagerly by default, masking lazy-registration-only paths. Check registration mode before concluding a code path is dead.

**Gotcha — enum-hint PRs: harness defaults miss the change entirely.** When a PR adds enum-value hints or restricts accepted values for a tool parameter, `scripts/live_smoke.py` invokes each tool with default arguments only and exercises none of the constrained paths. Run a targeted script that explicitly passes each new enum value and asserts the response shape is correct (or fails predictably for invalid values).

**Gotcha — hardware-gated tools: deferral requires four conditions.** Some tools succeed only with specific attached hardware. Before blocking merge on a live test failure, confirm all four: (a) the failing tool targets optional hardware (e.g., Access controller), (b) the test environment lacks that hardware, (c) the failure message indicates absence ("device not found" / "hardware unavailable"), not a logic or auth error, and (d) the same tool passes on an environment with the hardware attached. If all four hold, document the deferral explicitly in the PR description.

**Gotcha — HTTP 401 on uiprotect bootstrap path is benign noise.** Live smoke runs against a controller with Protect enabled emit HTTP 401 on the uiprotect bootstrap path during library startup. This is the library's startup probe and is expected — not an authentication failure. Do not block merge or file a bug on a 401 from this specific path.

**Gotcha — live_smoke.py report JSON has no response payloads; it cannot verify field-population fixes.** `scripts/live_smoke.py` writes only per-tool summaries (count, bytes, duration, status) to `live-smoke-results/*.json` — never the actual response payload. When a PR changes what fields get populated in a response (e.g. populating event `mac`/`ip`/`msg` from v2 system-log records), harness "ok" status proves nothing about the fix. Write a small in-process script that calls the tool directly and diffs the payload against a `git checkout origin/main` run before/after the change.

**Discovery — Pydantic field description changes don't auto-propagate to the MCP manifest / GraphQL SDL / OpenAPI.** Editing a field's `description=` only changes the source comment. Run `make generate` and verify the new wording reaches the generated artifact before claiming a client-visible docs fix is complete.

**Gotcha — `make pre-commit` / bare `uv lock` or `uv sync` silently drop workspace deps.** Running `make pre-commit`, or `uv lock`/`uv sync` without flags, re-syncs the root `.venv` without the full workspace package set — dropping deps like `mcp`. The next `uv run python scripts/live_smoke.py` then dies with `ModuleNotFoundError: No module named 'mcp'`. Run `uv sync --all-packages` before any `scripts/live_smoke.py` invocation that follows a `make` target or a lock/sync operation.

### API Family Boundary Check (V2 vs. Integration)

**Target:** Any PR that adds or modifies tools exposing UniFi API identifiers.

The UniFi API has two distinct identifier families: **V2 ObjectIDs** (UUID format, newer controllers)
and **Integration UUIDs** (legacy format). Tools must not mix identifier families in the same resource
surface — doing so creates silent data-loss bugs when downstream integrations receive mismatched ID types.

**Family boundary rule:** Each tool's input and output must be consistently rooted in one identifier
family. All nested references must match the primary resource's ID family.

If the PR introduces mixed families, request fixes before merge. This is a **hard blocker**.

### Mutating Cycle Tests (Create/Update/Delete PRs)

For any PR that touches create, update, or delete handlers, run a full mutating cycle:

1. **Create** — create the resource via the tool; capture the returned ID.
2. **Partial update** — update only a subset of fields.
3. **Verify field preservation** — read back and confirm fields you did NOT update are unchanged.
4. **Delete** — remove the resource and confirm it is gone (expect 204 or equivalent).

**Why field preservation matters:** The UniFi API silently drops fields not included in a
PUT/PATCH body. The verify step is the only reliable way to catch silent field zeroing.

### New-Parameter Coverage (Read-Tool PRs that Add Optional Params)

`scripts/live_smoke.py` calls every tool with default values only — it exercises none of the new
code paths. A targeted in-process pass is required: build a small Python script under `scripts/`
that calls each new parameter with at least one non-default value and asserts on the response shape.
Delete the script after verification — it's one-shot validation, not a durable harness.

### Docker Compose Verification (Shape/Description/Default Changes)

For PRs that change tool descriptions, response shapes, or parameter schemas, verify both discovery paths
a real LLM client uses:

1. **Pre-loaded path** (`unifi_load_tools` then `list_tools`) — confirms full JSON schema reaches MCP clients
2. **Lazy-execute path** (`unifi_execute(tool, arguments)`) — confirms description text carries new param
   semantics (schemas do not appear at the lazy-discovery tier). `unifi_execute` auto-promotes the tool
   into the loaded set after first call.

### "Fully Additive" Claim Audit

When a PR body claims "fully additive," diff every response shape against `main`. Any field that
disappears from the default path is a breaking change. Gate the narrowing behind an opt-in flag, or
explicitly document it as an intentional breaking change.

### Cross-Platform Validation (Windows-Targeting PRs)

For any PR that adds or modifies Windows-specific behavior (PowerShell prereq scripts, `.exe` executable detection, CRLF line endings), validate the following from macOS before approving:

1. **Plugin bundle `.ps1` presence** — unzip the plugin bundle and confirm `.ps1` scripts are included: `unzip -l <bundle>.zip | grep '\.ps1'`. Missing scripts mean Windows prereq flows will silently fail to load.
2. **Stdio MCP auth probe** — invoke the MCP server via stdio with intentionally bogus credentials and confirm the error response is human-readable (not a generic opaque "MCP error"). Windows users hit the stdio auth path first; an opaque error leaves them with no recovery action.
3. **Prereq script CRLF and `.exe` detection** — read the modified Windows prereq scripts and confirm CRLF handling and `.exe` suffix detection are correct. These regressions are invisible to macOS test runs.

You cannot execute `.ps1` scripts directly from macOS, but these three checks catch the most common Windows regression classes without requiring Windows hardware.

---

## Step 1.5b — AI-Bot vs Human Contributor Handling

### Mass-Submission / Spam Signal Check

Before judging a PR on its own merits, check the contributor's overall GitHub PR history
(`gh api "search/issues?q=author:<user>+type:pr"`) for a mass-submission pattern — hundreds of
near-identical PRs opened across unrelated repos. A high mass-submission ratio is a spam/low-trust
signal that should raise scrutiny, but it does not by itself block a well-formed PR — still run the
full gate checklist on the PR's own content.

### AI-Bot PRs: Close in Favor of In-House Work

When an AI-Bot submits a fix to an area with parallel in-house work, close it with:

```
Thank you for the contribution. We're already working on this area in-house
(see issue #NNN). Our version includes tests, live smoke validation, and full
audit coverage. We'll proceed with the in-house approach rather than merging
parallel work. Closed in favor of issue #NNN.
```

There is no goodwill concern — a bot is a tool whose value is the idea, not the effort.

### Human Contributors: Merge or Encourage Forward

- **If the PR is salvageable,** use the fork-edit model (Step 2) to integrate their work
- **If the PR is unsalvageable,** use Principle #6 (close-and-redirect) but include meaningful credit
- Always acknowledge the effort, even if you don't merge the code

---

## Step 1b — Post Feedback With the Right Review Type

**Copilot review threads are not approval gates.** "Open Copilot reviews" shown on a GitHub PR are review threads/comments, not a blocking review state — they do not by themselves prevent merge the way a human REQUEST_CHANGES review does. Do not treat an unresolved Copilot thread as a hard blocker unless its content identifies a real issue; verify against the actual PR review state (`gh pr view --json reviews`), not the presence of Copilot comments.

**PR head can drift between pre-submit refresh and GitHub accepting the review.** Refreshing the PR head immediately before posting a formal review (REQUEST_CHANGES or otherwise) does not guarantee the review lands against that exact commit — a contributor can push a new commit in the narrow window between your refresh and GitHub accepting the review call, silently attaching your review to a stale diff. Re-check the PR head SHA right before submitting, and re-verify after submission that the review's `commit_id` matches what you expect.

When you find merge blockers in Step 1, submit your GitHub review as **`request-changes`**, not
as `comment`. This prevents accidental merge and signals mandatory work clearly.

Structure your review body with explicit sections:

```
## Hard Blockers (must fix before merge)
- [ ] Replace f-string loggers in modified manager files
- [ ] Register new tool in validator registry

## Minor Items (nice-to-have)
- Consider renaming X for consistency with Y
```

Hard blockers are items from Gates 1–4. Use the `comment` review type only when you have zero
hard blockers and are leaving suggestions.

**Wording preference:** Write maintainer PR comments in first person ("I ran the live smoke tests
and found...") rather than passive/impersonal phrasing — this reads as direct maintainer accountability
rather than a generated report.

---

## Hard Blockers (must fix before merge)

See Gates 1–4 above for detailed checklists. Summary:
- F-string loggers in any modified file (Gate 1B)
- Ruff lint violations (Gate 1A)
- Missing Pydantic model wiring for new domains (Gate 2)
- Doc site not updated for tool additions/removals (Gate 3)
- Non-`None` defaults on shared base model fields (Gate 4)
- Mixed API identifier families (Step 1.5)

## Minor Items (nice-to-have)

- Naming alignment with existing conventions
- Test coverage for edge cases not on the happy path
- PR body prose (not blocking, but worth noting)

---

## Step 2 — Apply Fixes (Fork-Edit Model)

If you found gaps in Step 1, don't request changes — fix them directly on the contributor's
fork branch. This is the established model for trusted contributors and for unresponsive first-time contributors with trivial fixes.

```bash
# Add the contributor's fork as a remote (one-time setup)
git remote add <contributor> https://github.com/<contributor>/unifi-mcp.git

# Verify and update the remote URL if stale (e.g., after the unifi-network-mcp → unifi-mcp
# repo rename, or if the fork was recreated since the last PR)
git remote get-url <contributor>
git remote set-url <contributor> https://github.com/<contributor>/unifi-mcp.git  # if URL is wrong

# Fetch and check out their branch
git fetch <contributor>
git checkout -b review/<pr-branch> <contributor>/<pr-branch>

# Make your fixes, commit with attribution context, then push back
git push <contributor> HEAD:<pr-branch>
```

**Tip:** `gh pr checkout <PR-number>` resolves the PR's head ref directly without requiring a named remote. Use this when the remote URL may be stale — it bypasses the rename/recreate gotcha entirely.

### Rebase Before Push — When Branch Is Behind Main

**"N commits behind main" is a branch-protection gate, not proof of a real conflict.** GitHub's
branch-protection banner ("This branch is out-of-date with the base branch") fires purely on commit
count and can be satisfied by a clean, non-conflicting rebase. Do not assume actual file-overlap
conflicts exist until you've checked.

First, trial the rebase on a throwaway local ref so you don't touch the contributor's fork or
force-push until you know whether real conflicts exist:

```bash
# From the checked-out review branch, trial on a disposable ref:
git fetch origin main
git checkout -b trial/<pr-branch> review/<pr-branch>
git rebase origin/main
# Clean rebase (no conflicts) -> gate-blocked-but-clean; proceed to push below.
# Conflicts reported -> resolve them on this throwaway ref first, or hand back to the
# contributor if the conflict touches logic you shouldn't resolve unilaterally.
```

When another PR lands on `main` after the contributor's branch point, the contributor's branch must be rebased before merge to avoid a merge commit. Do this maintainer-side rather than requesting a contributor round-trip:

```bash
# Once the trial rebase is confirmed clean (or conflicts are resolved), repeat on the real review branch:
git checkout review/<pr-branch>
git rebase origin/main
# Resolve any conflicts, then push back with --force-with-lease:
git push --force-with-lease <contributor> HEAD:<pr-branch>
```

`--force-with-lease` is required for the post-rebase push — a plain `git push` will be rejected as non-fast-forward, and `--force` risks overwriting any commits the contributor added between your fetch and push. `--force-with-lease` fails safely if the remote has moved since your last fetch.

**Trusted contributor definition:** Level99 qualifies (7+ merged PRs). For first-time or
low-history contributors, prefer review comments so they learn the patterns.

### Unresponsive First-Time Contributor — Fork-Edit Exception

When a first-time contributor is **historically unresponsive** (72+ hours no response) AND the fix
is **trivial and mechanical** (ruff format, logger replacement, simple doc fix), apply fork-edit
instead of requesting changes.

**Substantive-gap exception:** When the unresolved blockers are substantive rather than trivial
(e.g., missing platform parity, unaddressed architecture feedback), do not fork-edit even if the
contributor is unresponsive. Close the PR, credit the contributor, and open a linked in-house
issue instead (Principle #6) — fork-edit is reserved for trivial/mechanical fixes only.

### Org Forks — Push Limitation

**The fork-edit model only works for personal forks.** Org forks block `git push` back even when
"Allow edits from maintainers" is checked — that checkbox is scoped to personal accounts only.

| Fork type | Can push fixes? | Action |
|-----------|----------------|--------|
| Personal fork | ✅ Yes | Fork-edit model |
| Org fork | ❌ No | Merge PR as-is, then commit cleanup to `main` in a follow-up |

---

## Step 3 — Verify PR Body Standards

Before merging, confirm the PR body includes: **What changed**, **Why**, and **Testing notes**
(including live smoke test output for API-touching PRs).

### API-Touching PR Body: Minimum Requirements

1. **Tool summary** — List every tool fixed or added, grouped by category.
2. **Embedded live test output** — Paste raw terminal output (not a prose summary) in a `<details>` block.
3. **Issue references** — `#N` format in both the commit message and the PR body for reliable auto-close.

**Gotcha — post-merge PR body edits do not trigger GitHub autoclose.** If issue references were missing at merge time and you add `#N` to the PR body afterward, GitHub does NOT retroactively close the linked issue — autoclose only fires from the merge event itself. For retroactive linking, post an explicit closing comment on each affected issue and close it manually.

**Gotcha — closing keywords only auto-close issues, never other PRs.** `Closes #NNN` in a merge commit or PR body auto-closes issues but does NOT auto-close superseded pull requests, even when the merged PR consolidates or replaces them (e.g., a consolidated Dependabot PR listing `Closes #444`, `Closes #445` for redundant bot PRs). Close superseded PRs manually.

### When a PR surfaces broader scope (Principle #5)

If reviewing a PR uncovers a pattern warranting a wider architectural fix, open a separate GitHub
issue and link it in the PR body. Use Principle #5 when: **the PR itself is salvageable** but the
idea it surfaces is too big to carry in this PR.

### When the PR itself is unsalvageable — Close-and-Redirect (Principle #6)

1. **Extract valid proposals** — open a new GitHub issue capturing genuine good ideas
2. **Implement in-house** — if high-value, plan to implement on `main`
3. **Credit the contributor** — close with a comment acknowledging them and linking the new issue

### Principle #5 vs. Principle #6 — Decision Matrix

| Situation | Principle | Action |
|-----------|-----------|--------|
| PR is good; it just surfaced a bigger idea | **#5** | Keep PR → merge; open separate issue |
| PR has too many concerns to fix cleanly | **#6** | Close PR → extract ideas; implement in-house |
| PR has mechanical gaps (logger, registry) | Fork-edit | Fix directly on fork; don't close |
| Contributor is first-time/low-history | Review comments | Request changes; don't close unless clearly out of scope |

---

## Step 4 — Merge

**Gotcha — `mergeStateStatus: BLOCKED`:** GitHub's `mergeable` field and `mergeStateStatus` are
independent. A PR can show `mergeable: MERGEABLE` while `mergeStateStatus` is still `BLOCKED`
(e.g., required approvals missing, required status checks pending, branch protection rules active).
Always confirm both before running the merge command:

```bash
gh pr view <PR-number> --json mergeStateStatus,reviewDecision,mergeable
```

`mergeStateStatus: BLOCKED` means the PR cannot be merged regardless of `mergeable`. Resolve
the blocking condition (get missing approvals, wait for pending checks) before proceeding.

**Admin-bypass precedent:** When the sole block is a missing-approval requirement that cannot be satisfied because the maintainer authored the PR (GitHub blocks self-approval) and all CI checks are green, `gh pr merge <PR-number> --squash --admin` is an accepted owner-bypass. Use it only when the block is purely the self-approval gate, not an unresolved review concern.

```bash
# Squash-merge is the repo standard — one clean commit per PR on main
gh pr merge <PR-number> --squash
```

Squash-and-merge is the default: it keeps `main` linear with a single commit per PR rather
than carrying every intermediate or force-push commit from the contributor's fork branch.

**Merge strategy override:** The squash default can be overridden on explicit user instruction.
If the user asks for a merge commit (`--merge`) to preserve the contributor's individual commits,
apply it without hesitation — do not silently revert to the default. Acknowledge the override explicitly.

---

## Sequential PR Artifact Conflicts

When two community PRs both modify the same generated artifacts (GraphQL schema, REST docs, or
manifest files), the second PR becomes conflicting after the first is merged.

Resolution:
1. Review both PRs before deciding merge order — earlier detection minimizes rebase cycles.
2. After merging the first PR, rebase the second onto updated `main`.
3. Regenerate the affected artifacts on the rebased branch.
4. Push the regenerated artifacts and wait for CI to re-run.
5. Proceed with the standard review checklist on the rebased branch.

Do not merge both PRs in rapid succession without first checking for shared artifact dependencies. Release-batch check: if an open PR touches the same package as a diff just merged, fold it into the same release batch instead of tagging early; if not, tag now and start the next batch with that PR.

---

## Post-Merge Audit Pattern

If a PR was merged without running this checklist, run a retroactive audit:

```bash
git diff --name-only <merge-commit>^1 <merge-commit>
```

Then run Gates 1–4 against those files. If gaps are found, open a follow-up PR immediately.

---

## Community Infrastructure Setup

Set up `.github/` infrastructure when the project goes public or when a new issue category
emerges. This is the upstream foundation that makes PR triage scalable.

The standard layout is: `.github/ISSUE_TEMPLATE/` (structured forms), `.github/config.yml`
(routing rules), `CONTRIBUTING.md` (contribution guidelines including the evidence-first doctrine),
and `SUPPORT.md` (support channels: Discussions for questions, tracker for confirmed bugs only).

### config.yml Routing

Route sensitive and support traffic away from Issues before it arrives:

```yaml
# .github/config.yml
blank_issues_enabled: false
contact_links:
  - name: Security Vulnerability
    url: https://github.com/ORG/REPO/security/advisories/new
    about: Report a security vulnerability via GitHub Security Advisories
  - name: Usage Question / Support
    url: https://github.com/ORG/REPO/discussions
    about: Ask questions and get help in GitHub Discussions
```

`blank_issues_enabled: false` forces all issues through form templates, preventing unstructured
filings. Security reports go to Advisories (private by default); support questions go to Discussions
so the issue tracker stays actionable.

---

## Bug Report Template Design

The bug report form collects hardware-specific context that prevents multi-comment follow-up cycles.
Because unifi-mcp behavior varies by controller firmware, hardware SKU, and install method,
collecting this context at first filing is essential.

### Required Fields (never make optional)

| Field | Type | Why required |
|-------|------|--------------|
| Controller hardware | Dropdown | API behavior varies by hardware family |
| UniFi OS version | Text | Firmware version determines which API fields are present |
| Install method | Text | Determines whether aiounifi version is pinned vs. flexible |
| Client OS | Text | Needed for install-method-specific issues |

**Controller hardware dropdown options:** UDM Pro, UDM Pro Max, UDM SE, UDM (base), UDR / UDR7,
UCG Max / UCG Ultra / UCG Fiber, Cloud Key Gen2 

…(truncated)