# Review, master (uncommitted), 2026-10-27

**Reviewed by**: Deepseek V4.1 Flash (author on composer 2.5)
**Scope**: 10 files, uncommitted
**Verdict**: Approve with nits

## Summary

This slice adds a silent PHP client for the hosted Node face service, with config from `ask_zap.php`, typed results for detect and verify, and removal of the Settings Face group. The engine follows AC-1 to AC-11 closely: the guard order, TLS and redirect options, status mapping, and payload checks all match the spec. The settings surface keeps the spec 0003 auth chain and stops writing the school percent while still returning it in JSON. No blockers or majors. The findings are cleanup and hardening: one helper returns the secret key, the payload is encoded twice, a dead percent validator remains, the AC-10 SQL guard is a brittle substring check, the validation tests point at the live origin, the public origin has no key set, and a restore failure hides skipped schools.

## Minor

### 🟡 Config helper returns the key string, `admin/includes/staff_attendance_face_engine.php:151`
**Problem**: `saf_ready_config` returns `key`, the trimmed `face_api_key`. AC-2 says the returned array never includes the key string. `saf_config` is clean, but this global helper hands the secret to any later caller.
**Why it matters**: A future page or log line that dumps the helper result puts the Bearer secret in HTML or in the PHP log, which AC-8 forbids.
**Suggested fix**: Keep the key in a local value inside `saf_detect` and `saf_verify`, or pass it to `saf_post_json` separately so no array that someone may print contains it. At minimum add a comment that this helper must never be returned from a page or written to a log.

### 🟡 Payload is encoded twice, `admin/includes/staff_attendance_face_engine.php:376`
**Problem**: `saf_post_json` calls `saf_body_size_ok($payload)`, which json_encodes the payload, then it encodes the same payload again for the request body. For two large images this holds two copies of a string that can reach 9 MB.
**Why it matters**: The spec's Consequences section already warns that two 5 MB images plus base64 use tens of MB. Doubling the encoded copy raises peak memory for no gain, and this host runs PHP on shared cPanel.
**Suggested fix**: Encode once, run the `strlen` check on that string, and pass the same string to `CURLOPT_POSTFIELDS`. `json_encode` is deterministic here, so one encode is enough.

### 🟡 Dead percent validator remains, `admin/includes/staff_attendance_settings_engine.php:142`
**Problem**: `sas_parse_percent` is no longer called, but it still carries the old percent rule and builds a `face_match_threshold` value. AC-10 says save and reset must not validate the percent and must not write the column.
**Why it matters**: Dead code that matches the removed behavior invites someone to wire it back into save. Neither test script would catch that, because both only check that the save path does not call it today.
**Suggested fix**: Delete the function. Leave a short comment where save reads the posted fields that a posted percent is ignored by design.

### 🟡 The AC-10 SQL guard is a substring match, `admin/includes/staff_attendance_settings_engine_test.php:41`
**Problem**: Both test scripts assert that the engine source does not contain `face_match_threshold = ?`. A reformatted UPDATE, or one written as `face_match_threshold=?`, would still write the column and the test would pass. This is the only automated proof that save and reset leave the school percent alone.
**Why it matters**: AC-10's main data integrity claim is that the stored percent is never rewritten. A brittle check can pass while the column is updated, or fail after a harmless format change.
**Suggested fix**: Assert against the exact prepared SQL string used by `sas_update_row`, or drive that function with a small fake mysqli and inspect the SQL it was asked to prepare. Keep one such test, not two copies.

### 🟡 Validation tests point at the live origin, `admin/includes/staff_attendance_face_engine_test.php:305`
**Problem**: The block that proves the `bad_image` cases (PNG, oversize, both sides, http url) is built with the production origin. Those paths return before cURL, so no call is made today. If a guard regresses, the test sends test bytes to the live face service and waits on the network instead of failing cleanly.
**Why it matters**: The file header promises no live Node. A biometric test that can silently become a live POST is a poor gate, and it would post to a service that currently has no key.
**Suggested fix**: Use an origin that cannot resolve or a reserved test host, or inject the transport and assert the HTTP attempt count is zero. Keep the production host out of the test file.

### 🟡 No key set for a public origin, `private/includes/ask_zap.php:13`
**Problem**: `face_api_url` is present and is a public `https` origin. `face_api_timeout_seconds` and `face_api_key` are absent, so `saf_config` returns `has_key` false and no `Authorization` header is sent. The bearer box in `verify.md` is still unchecked.
**Why it matters**: Anyone who can reach that origin can post portraits to it, and this app cannot be told apart from any other caller. The spec lists this as a follow up before production, but the live `/check verify` pass already calls the public service.
**Suggested fix**: Set `face_api_key` in `ask_zap.php` and on the Node host before any caller (enrollment or check in) ships, then run the remaining live box. Do not put the value in a spec, a commit message, or a doc.

### 🟡 A restore failure hides the schools that were skipped, `admin/includes/staff_attendance_settings_engine.php:335`
**Problem**: When `select_db` back to the session database fails, the branch that was just saved is reported `ok` and the loop breaks. The remaining checked schools are never attempted and never appear in `branches[]`. The page only shows notes for rows with a failing status, so the admin sees a plain success toast.
**Why it matters**: An admin who ticked four schools can be told the save worked while only one or two were touched.
**Suggested fix**: Keep the saved branch as `ok`, and add a row for every skipped school with status `error` and a message that the session database could not be restored. Or return a top level flag and toast it. The engine test already pins the current behavior, so extend it.

## Nits

- ⚪ `app/admin/partials/zap-nav-menu.php:172`, the Settings tile subtitle still says "face match" while the page no longer has a face control. Spec 0003 AC-2 still mandates that subtitle, so leave the code and let `/sync` line the wording up after 0005 ships.
- ⚪ `admin/includes/staff_attendance_face_engine.php:264`, `saf_parse_count` accepts only digit strings. AC-7 says an integer or a numeric string then `intval`, so `"3.0"` would be rejected. Node returns an int, so the strict check is safe, but it is narrower than the contract.
- ⚪ `admin/includes/staff_attendance_face_engine.php:171`, the User Agent is set both as a header and through `CURLOPT_USERAGENT`. Pick one so the request has a single source of truth.
- ⚪ `admin/includes/staff_attendance_face_engine.php:224`, the `trim($url) === ''` check is unreachable because the whitespace regex above already rejected empty and whitespace only urls. Remove it or trim first.
- ⚪ `admin/includes/staff_attendance_settings_ajax.php:19`, this JSON response does not send `Cache-Control: no-store` and `Pragma: no-cache` the way the photos dispatcher does, though the body carries a CSRF token and school policy. Match the other dispatcher.
- ⚪ `admin/includes/staff_attendance_face_engine_test.php:360`, the settings assertions (Face group gone, JS has no percent) are duplicated in `staff_attendance_settings_engine_test.php`. Keep them in the settings test so a failure points at one file.

## Strengths

- The status mapping is careful and matches AC-6 and AC-7: 3xx, 401, 403, 429, 5xx, empty body, bad JSON, and transport errno become `unavailable`; detect 400 is `error`; verify 400 is `no_face`; `match` must be a real PHP boolean; `similarity` is finite, range checked, and rounded to four places.
- `CURLOPT_SSL_VERIFYPEER` true, `CURLOPT_SSL_VERIFYHOST` 2, and `CURLOPT_FOLLOWLOCATION` false are all set, so a redirect cannot carry the Bearer header. The engine never logs, never echoes, and never reads the request superglobals.
- Every value check runs before the POST: `array_key_exists` plus null for each side, JPEG magic, the 5 MB cap, the 2048 url limit, and the 9 MB body guard all return `bad_image` with `http_status` 0.
- AC-10 is done cleanly: the Face group and percent input are gone, the page JS stops sending the percent, the UPDATE list omits the column, and the JSON still maps the stored percent from `SELECT id = 1` so old JS keeps working.
- The settings surface keeps the spec 0003 auth order: session 401, CSRF and same origin 403, shared role helper 403, with extra school ids resolved from `$school_config['schools']` only and the session database restored after each attempt.

## Test coverage

`staff_attendance_face_engine_test.php` runs as a PHP script, uses config fixtures in a temp `DOCUMENT_ROOT`, and covers the origin rejects, timeout defaults, key omission, side rules, JPEG magic, size guards, and the detect and verify parse fixtures. On the intended path it makes no live call. `security_origin_test.php` covers the rewritten same origin helper for scheme, port, and the Origin fallback, which matters because `security.php` is shared by many desktop pages. `staff_attendance_settings_engine_test.php` covers the restore failure path and the public percent mapping.

Gaps: no test calls `saf_verify` for its two side pairs or payload keys, the detect missing or invalid `count` case is not asserted, the AC-10 SQL claim rests on a substring, and two live boxes in `verify.md` are still unchecked (bearer on the live POST, and teacher plus dead session on Settings). Those are `/check verify` items, not missing test infrastructure.