diff --git a/docs/superpowers/reviews/2026-08-21-plan1-review.md b/docs/superpowers/reviews/2026-08-21-plan1-review.md
new file mode 100644
index 0000000..8995e77
--- /dev/null
+++ b/docs/superpowers/reviews/2026-08-21-plan1-review.md
@@ -0,0 +1,322 @@
+# Plan 1 review findings — BYOD Wi-Fi access model
+
+**Date:** 2026-08-21
+**Reviewed:** the six commits implementing
+`docs/superpowers/plans/2026-08-21-byod-wifi-access-model.md`
+(`central-unifi` `a74818b`..`a8d7b34`, `dashboard-shell` `f3de4aa`..HEAD)
+**Verdict:** CHANGES NEEDED. Nothing needs redesigning; the list below needs fixing.
+**Suite at time of review:** 48 passed in `tests/Feature/Unifi/`; full suite
+6293 passed / 10 failed, all in the unrelated pre-existing `Logs\ScriptRunsTest`.
+
+Recorded here because these findings outlived the session that produced them and
+every one of them is about code that is already committed.
+
+---
+
+## 0. The request IP is trusted, and the app has no opinion of its own
+
+`dashboard-shell/bootstrap/app.php:14` — `$middleware->trustProxies(at: '*')`.
+Laravel routes `'*'` to `setTrustedProxyIpAddressesToTheCallingIp()`, so **the
+immediate peer is unconditionally trusted as a proxy** and its `X-Forwarded-For`
+becomes `$request->ip()`. `docker/nginx/default.conf` sets no `real_ip_header` or
+`X-Forwarded-For` of its own, so a client-supplied header reaches PHP verbatim.
+
+`DeviceResolver::resolve()` reads `$request->ip()` and derives the MAC from it, so
+if a client can deliver a forged header the resolver identifies a device of the
+attacker's choosing. The AP check does not help: `ap` is also a request parameter,
+so an attacker forges both.
+
+### What is NOT established
+
+An earlier session demonstrated forgery by sending `X-Forwarded-For` **directly to
+nginx on `:80`**. That path is not available to a real client: the Wi-Fi VLAN is
+firewalled off from the server and can reach only Traefik. So that demonstration
+does **not** show a live exploit, and it should not be cited as one.
+
+### What IS established: the legitimate value arrives correctly
+
+Confirmed by the operator on 2026-08-21 from the production Logged-in Users list,
+served through Traefik: sessions show **real per-client addresses** —
+`10.22.5.254` and `10.22.5.154` for on-site staff, and a public address for an
+off-site phone. So `TrustProxies` plus Traefik does yield the true client IP, and
+UniFi can map an on-site `10.x` address to a MAC.
+
+That settles two things:
+
+- **The IP → MAC approach is viable.** This was the operator's original question and
+  the answer is yes, with production evidence.
+- **Spec §E had the release-gate risk backwards.** It called "whether TrustProxies
+  yields the true client IP" the thing that gates release, fearing a fail-closed
+  outage where nobody can get online. That fear is unfounded — it demonstrably
+  works. Remove it as a blocker.
+
+Note the off-site public address: a portal request only ever originates on-site, so
+a request whose IP is public will correctly find no UniFi client and refuse. That is
+the right behaviour, not a gap.
+
+### The open question, which is narrower than it first appeared
+
+**A legitimate value arriving correctly and a forged value being rejected are
+different properties.** The evidence above establishes the first. It does not
+establish the second, because nothing in that list would look wrong if Traefik were
+also passing through client-supplied headers.
+
+So one question remains, and it is a one-line check
+
+Whether a client can deliver a forged header depends entirely on Traefik's
+`forwardedHeaders` for the entry point, on the Traefik host (162.223.102.234 —
+a separate LXC, not reachable from the dev box):
+
+- `forwardedHeaders.insecure: true`, or `trustedIPs` covering client ranges
+  → client headers are passed through → **the resolver's guarantee is void**
+- unset (Traefik's default) → Traefik replaces client-supplied forwarded headers
+  → the design works as intended
+
+**Treat this as the release gate.** Narrowing `trustProxies` from `*` to the
+Traefik network is worth doing either way as defence in depth: as written, the
+application delegates the entire question to whoever happens to be talking to it.
+
+### The consequence that was missed, if it is forgeable
+
+Not a quota bypass — quotas key on `user_id`, so an attacker cannot exceed their
+own limit. It is worse in a different way. `updateOrCreate` writes a row keyed
+`(attacker_user_id, victim_mac)`, making the attacker the legitimate **owner** of a
+session pointing at somebody else's device. `disconnectSession` then passes its
+owner check and runs `kickClient` + `unauthorizeGuest` on the victim's MAC.
+
+The spec declared `disconnectSession` "already correct and not being changed". That
+was true of the function and false of its inputs — the one component held out of
+scope as safe is what turns this into an eviction primitive. Worth remembering as a
+general lesson about scoping something out on the strength of its internals.
+
+Also note the three distinct refusal reasons (`no_client` / `ap_mismatch` / success)
+form an oracle: iterating candidate IPs enumerates the live client table with MAC
+and AP. They were added for operability and double as reconnaissance.
+
+**Nothing in the six commits is defective on this point.** `DeviceResolver` is a
+correct implementation of "derive the MAC from the source IP" sitting on a platform
+assumption. Spec §E named this exact dependency as the release gate and got the
+direction backwards — it worried `TrustProxies` would be too strict (a visible
+outage) and never considered it being too permissive (silent fail-open). The
+blind-spot note in `b59b5bf` reasons carefully and still only considers the strict
+direction.
+
+**Exposure is wider than this feature.** Every consumer of `$request->ip()` in the
+shell inherits it — session client IP, the Logged-in Users list, anything keyed on
+IP. File against `dashboard-shell`, not `central-unifi`.
+
+### What would not help
+
+Assessed and rejected, all reducing to trusting the same value:
+
+- **Re-checking the client's reported IP** — circular; the record was *selected* by
+  that IP, so the check cannot fail.
+- **Requiring the client to be unauthorized** — breaks the renewal path and with it
+  the spec's headline promise, while still leaving first-time connectors targetable.
+- **Association recency** — narrows harvesting to a race the attacker wins by
+  polling, and wrongly refuses a device revisiting after a long window expires.
+
+The durable fix is to stop making the request IP load-bearing: have the controller
+vouch for the MAC (a signed redirect / HMAC over `mac, ap, timestamp`). That
+survives any proxy misconfiguration. Whether this controller supports it is a
+question for the hardware session — **add it as a fourth item to spec §E**, ranked
+above the health line in Plan 2.
+
+---
+
+## 1. The reconciler would revoke every guest authorization on the whole site
+
+`src/Console/ReconcilePortal.php` — `array_diff($authorized, $live)` then
+`unauthorizeGuest` on each, where `$authorized` comes from `getAuthorizedGuestMacs()`
+reading **site-wide** `/stat/guest`. Nothing scopes it to authorizations this portal
+created.
+
+A second guest SSID, UniFi's own voucher portal, front-desk visitors, contractors,
+a gym rental, or anything an admin authorized by hand: all revoked on the first
+scheduled run, and again every run. Inversely, a device an admin deliberately
+un-authorized in the UniFi UI gets re-authorized — the dashboard silently
+overriding the network admin, forever.
+
+Fix: only revoke a MAC that appears in `unifi_portal_sessions` at all, or filter by
+the portal's SSID/VLAN. **This is why "not scheduled" is currently load-bearing
+rather than merely cautious.**
+
+Unverifiable without hardware, and worth adding to §E: what window does
+`GET /stat/guest` return with no `within`, and does the controller rewrite `end`
+when an authorization is revoked? If not, job 3 silently stops re-asserting.
+
+## 2. The pre-DHCP retry cannot succeed, and costs a worker 1.5s per failure
+
+`DeviceResolver` retries three times, but `findClientByIp()` → `getActiveClients()`
+is `Cache::remember('unifi:clients', 30s)`. All three attempts read one snapshot.
+
+Worse than a no-op: the first attempt **warms** the cache with a snapshot lacking
+the connecting device, so "try again" stays refused for the rest of the TTL, for
+that pupil and everyone else. Meanwhile each failure holds a PHP worker for 1.5s of
+`usleep` — the exact cost the comment claims to bound, with none of the benefit. A
+class of 25 arriving together is 25 workers asleep on one stale array.
+
+`UnifiApiClient`'s own comment justifies the cache on the grounds that "the portal
+calls this once per connection attempt". The resolver calls it three times.
+
+Fix: `Cache::forget('unifi:clients')` between retries, or an uncached read path.
+
+## 3a. Device type should come from the User-Agent, not from UniFi
+
+Raised by the operator on 2026-08-21, from the same production screenshot: the
+Logged-in Users list already labels sessions "Safari · iPhone" and
+"Chrome · macOS", and does it well.
+
+That is parsed from the **User-Agent**, by a hand-rolled private method
+`ActiveSessionController::device(string $ua)` — no library involved. Its OS table
+already distinguishes `iPhone`, `iPad`, `Android` from `Windows`, `macOS`,
+`Linux`, `CrOS`, which is precisely the phone-versus-laptop distinction the
+no-phones-for-students policy needs.
+
+**This is a better source than UniFi's fingerprint for this purpose**, and it
+removes a dependency the spec could never verify:
+
+- **Available at authorization time.** A portal request comes *from the device*, so
+  its User-Agent is the device's own. UniFi's `dev_cat` is inferred from observed
+  traffic and does not exist yet at that moment — the reason §C had to push capture
+  into the reconciler at all.
+- **Known to be populated.** `unifi_client_snapshots` has zero rows on dev, so
+  whether `dev_cat` is ever filled in this environment was never established. The
+  User-Agent is always present.
+- **It fixes finding 3 with a value that is actually there.** Replacing the three
+  always-NULL `$request->input(...)` writes with the parsed label gives the denial
+  page real device names, which is the single case that list exists for.
+
+Recommendation: extract `device()` into a shared support class (e.g.
+`App\Support\UserAgent::describe()`) and have both the Logged-in Users controller
+and the portal call it. Small refactor, one owner for the parsing, no duplication.
+
+**The spoofing caveat stands and does not weaken this.** A mobile browser's
+"Request Desktop Site" toggle changes the User-Agent, so this is sound for
+*reporting* and unsound for *enforcement* — which is exactly decision 6
+(report-only). Keep UniFi's `dev_cat` capture in the reconciler as a corroborating
+second opinion: it is not client-controlled, so a disagreement between the two is
+itself a useful signal if enforcement is ever wanted.
+
+## 3. Every portal visit blanks the fingerprint the reconciler filled in
+
+`PortalController` writes `device_hostname`, `device_os` and `device_type` from
+`$request->input('hostname'/'os'/'dev_cat')`. UniFi's redirect sends `id`, `ap`,
+`t`, `url`, `ssid` — the controller's own comment says so — so all three are
+written NULL on every authorization.
+
+The exact mirror of the bug `captureFingerprints()` goes out of its way to prevent.
+Consequence: the denial page's device list always reads "Unknown device", so a pupil
+at their limit chooses between two unlabelled MACs — the single case that list
+exists for. Plan 2's Wi-Fi Devices page inherits it.
+
+`$device->client` already carries UniFi's `hostname`/`os_name`/`dev_cat` and is
+never read. Use it. These are also the last request-supplied values written to a
+session, so `?hostname=Principal's Laptop` is shown to admins as fact.
+
+## 4. Windows slide, contradicting the spec
+
+`expires_at => now()->addMinutes($minutes)` on the renewal branch grants a fresh
+full window on every visit. With `session_minutes` set to a term, anyone who
+bookmarks the callback never ages out — which is what §C's fixed-window rule exists
+to prevent. Either preserve `expires_at` on renewal, or amend the spec and say why
+in a comment; today there is no comment where a reader would ask.
+
+## 5. Two routes to a wrong quota
+
+No unique index on `(user_id, mac_address)` (confirmed: only PRIMARY,
+`(user_id,is_active)`, `(mac_address)`) and no lock around read-then-authorize.
+
+- **Over quota:** two devices of a 1-device pupil hit the callback concurrently,
+  both read zero live sessions, both are authorized. The reconciler cannot correct
+  it — both rows are live and both authorized.
+- **Phantom slots:** two concurrent renewals of the same MAC each miss the other and
+  insert twice. One phone, two live rows, permanently. The reconciler's
+  `keyBy(mac)` silently collapses the pair so neither direction sees it, and
+  self-disconnect deactivates only the row that was clicked.
+
+Fix: unique index plus `lockForUpdate` in a transaction. Consider a reconciler step
+trimming a user's live rows to their quota, newest first — nothing currently
+enforces the quota against our own rows.
+
+## 6. Denial copy contradicts itself on the commonest path
+
+Controller says "Still connecting — wait a moment and try again"; `PortalDenied.vue`
+then prints "Please contact the school office or IT for help getting online". Two
+instructions in one view, on the most frequent denial. `resolveFailure` is passed
+and never declared as a prop.
+
+---
+
+## Tests that pass for the wrong reason
+
+1. **The headline quota-leak regression test** asserts `component('Unifi/PortalDenied')`
+   and a live count — but four branches render that component. It passes with no
+   role and no mapping at all, via "No WiFi access configured". Fix: assert the
+   `reason` string.
+2. **The proxy-outage test** sets `X-Forwarded-For` and `REMOTE_ADDR` to the same
+   value, so its condition holds by construction; mutating the production
+   condition to `($forwarded)` alone still passes. (Note: under
+   `trustProxies(at: '*')`, `$ip === $raw` only holds when no XFF was sent, so
+   `proxy_untrusted` is unreachable in this environment — a consequence of #0.)
+3. **The reconciler re-assert test** constrains neither MAC nor minutes. A sign
+   error in `now()->diffInMinutes($expires_at, false)` would clamp every
+   re-assertion to one minute and the test would not notice — and Carbon 3.13.1
+   changed that method's return type and default `$absolute`.
+4. **The `sort_order` precedence test** inserts the low-`sort_order` row first, so
+   replacing `orderBy('sort_order')` with `all()` still passes. It also never
+   asserts the user holds `parent`.
+
+Untested: the unreachable-controller guard — the fail-safe between a network blip
+and mass revocation. Worth a test now, and more so once #1 is fixed.
+
+---
+
+## Smaller notes
+
+- `PortalDeniedTest`'s docblock explains why a device "must look CONNECTED, or the
+  controller treats the session as stale, frees the slot and authorises" —
+  documenting behaviour this work **deleted**. Most confidently-worded comment in
+  the suite and now false. The dead `isClientConnected` stub goes with it.
+- `ResolvedDevice::$client` is never read (see #3). `$macParameterDisagreed` is
+  asserted in a test and consumed nowhere — surface it or drop it and keep the log.
+- The disagreement test asserts the flag, not that `Log::warning` fired, which is
+  what §B actually asks for.
+- Unused `use ...DB;` in two test files, copied from the plan.
+- Three resolver tests burn 1.5s of real `usleep` each; injecting the pause would
+  pay for itself.
+- `UnifiApiClient::isClientConnected()` is now referenced nowhere in `src/`.
+  Correct per spec (display-only, Plan 2) — say so in a comment or the next
+  dead-code sweep deletes it.
+
+## Release sequencing — not a finding, but it changes the plan
+
+`Pages/Unifi/Portal.vue` still offers "Default (catch-all)", which now 422s, and
+offers **no `role` option**. Wi-Fi quotas are therefore *unconfigurable* today:
+the reachable match types are `ou`/`group`/`email_domain`. Spec §F step 3
+("configure `role` → student / staff") cannot be carried out until Plan 2, which
+makes **Plan 2 a hard prerequisite for release rather than a follow-up**.
+
+---
+
+## What is good and should not be disturbed
+
+- **`scopeLive`** is the right shape in the right place: the model owns the
+  definition of a consumed slot, a NULL expiry is not live, and the *why* is in the
+  docblock. Leak 2 is fixed at the query level and does not depend on the
+  unscheduled reconciler. The most valuable thing in the six commits, and untouched
+  by #0.
+- **`?mac=` really is dead.** Verified end to end: with a supplied MAC present,
+  `authorizeGuest` receives the derived one and no other row is written. No
+  fallback anywhere; an absent `?ap=` fails closed as `ap_mismatch` rather than
+  skipping the check. The fail-closed discipline is genuine and thorough — it is
+  the input that betrays it, not the logic.
+- **`DeviceResolver` split out of the controller** — four named refusal reasons, no
+  quota knowledge, no Inertia. When the platform is fixed this is the right object
+  in the right place, and where an eventual HMAC check belongs.
+- **`captureFingerprints()`' fill-only rule** and its reasoning. #3 is the
+  controller violating this rule, not this code being wrong.
+- **The Mockery declaration-order docblock** in the reconciler test will save the
+  next person an hour.
+- **Removing `default` while `unifi_vlan_mappings` was empty**, and recording in
+  code why doing it later would be worse.