# Review, master, 2026-10-30

**Reviewed by**: Deepseek V4.1 Flash (author on Composer 2.5)
**Scope**: 26 source files, uncommitted (plus 15 audio assets and `skills-lock.json`, not reviewed)
**Verdict**: Approve with nits

## Summary

This is slice 6, staff face enrollment, built on the new access helper, the `saf_*` face engine, and the `sap_*` storage helpers, and it retires the Photos page. The shape is right: the engine stays pure and injectable, the pending key protects the last good portrait, the duplicate phone path has a real rollback under `MYSQLI_REPORT_STRICT`, and `view` refuses a client supplied key. I ran all five `php admin/includes/*_test.php` scripts and they pass on this host. The findings below are behaviours that drift from spec 0006 and two places where the new tests cannot prove what their labels claim; none of them corrupt data.

## Minor

### 🟡 Enroll reports "not activated" for a staff id that is not in this school, `admin/includes/staff_attendance_enrollments_engine.php:1079`
**Problem**: `sae_enroll` calls `sae_require_activated($db, $ctx, $staff_id, true)`. With `$missing_as_inactive` true, a `staff_id` that has no row in the current school database returns `That staff member is not activated.`, not `That staff member was not found.`.
**Why it matters**: AC-9 separates the two cases explicitly ("a staff id not in this school is `That staff member was not found.`; staff in this school with `activation` not `activated` is `That staff member is not activated.`"). `mint` gets this right by passing false, so the same input now produces two different messages depending on where capture stopped, which will confuse support when a stale id is posted.
**Suggested fix**: Pass false in `sae_enroll`, or drop the flag and keep the missing row branch as `sae_msg_missing_staff()`. Add a test that asserts the not found message for a positively numbered staff id absent from the table, which the current file does not cover.

### 🟡 The tenant lock is still held while the duplicate phone path deletes the pending object, `admin/includes/staff_attendance_enrollments_engine.php:1114`
**Problem**: `sae_finish_duplicate_phone` is called from inside the `try` block, so the `finally` that runs `sae_release_lock` has not fired yet. That function writes the attempts row and then calls `sap_delete` on the pending key, which is a B2 network call, all while `GET_LOCK` is held.
**Why it matters**: AC-9 says the lock is "held for the DB write only" and released "before any B2 copy, HEAD, delete, or sweep". A slow or hanging B2 delete (the photos engine uses a 10 second cURL timeout) keeps the lock for the same staff member, so a second admin's enroll attempt blocks and then fails with `Please try again.` for a reason that has nothing to do with their request.
**Suggested fix**: Capture the duplicate outcome inside the transaction, release the lock, and only then write the attempts row and delete the pending object.

### 🟡 Re-testing inside the same second reports "not enrolled", `admin/includes/staff_attendance_enrollments_engine.php:1313`
**Problem**: The `tested_at` update uses `$stmt->affected_rows` and treats 0 as `That staff member is not enrolled.`. PHP's mysqli counts changed rows by default, so an `UPDATE` that matches the row but writes the same `Y-m-d H:i:s` value reports 0.
**Why it matters**: Tap Test, pass, tap Test again within the same second (a double tap, or a client retry) and a successful verify is reported as a failure that names the wrong cause. AC-13 asks the same statement to refresh `tested_at`, and this path does not fail the test, only the message.
**Suggested fix**: Use `$db->info` or compare against the row's previous `tested_at` before deciding the row is gone, or pick the primary key update as the proof that the row still exists and ignore the changed row count.

### 🟡 The browser PUT gives up after about four seconds, `app/admin/scripts/staff_attendance_enrollments.js:688`
**Problem**: `putUntilExpiry` sets `maxTries = 6` with delays of 400, 1000 and 2000 ms, so it stops long before the 120 second `expires_in` window it was given, then shows `The photo did not upload.`.
**Why it matters**: AC-16 says the PUT "retries that same `put_url` until `expires_in`". The spec also mentions "until PUT retries are exhausted", so the intent is ambiguous, but six tries in roughly four seconds is short for a school phone on weak wifi, and the cost of giving up is not a retry, it is a full recapture of the liveness sequence. `staff_attendance_photos_engine_test.php` asserts `var maxTries = 6`, so the test locks in whichever answer the author picks.
**Suggested fix**: Either loop with the existing backoff until the deadline, or amend AC-16 to name the cap so the two agree.

### 🟡 `confirm_face` retry branches never match, so a blip costs a recapture, `app/admin/scripts/staff_attendance_enrollments.js:975`
**Problem**: `afterConfirm` keeps the pending key for `timeout`, `unavailable` and `not_configured`, but `sae_confirm_face` can only return `school`, `config`, `staff`, `inactive`, `folder`, `missing`, `jpeg`, or `store` and `b2_timeout` from the HEAD. Every store failure therefore falls through to `faceCountFail`, which clears `pendingKey` and sends the sheet back to Start.
**Why it matters**: The uploaded object is still in the bucket and the pending key is still valid, so a transient B2 error throws away a good capture and forces the staff member through liveness again. The dead branches suggest the author expected a retry that cannot happen.
**Suggested fix**: Keep the pending key and offer Retry for `store` and `b2_timeout` (and drop the three branches that never fire), so only `missing` and `jpeg` reset to Start as AC-7 requires.

### 🟡 The shared same origin helper changed app wide, outside the spec, `admin/includes/security.php:301`
**Problem**: `request_is_same_domain` now compares scheme and port as well as host, and falls back to `HTTP_ORIGIN` when the referer is absent. Roughly forty desktop pages gate real writes on it, including `admin/manage_students.php` (`delete_student`, `change_password`) and `admin/grading.php`.
**Why it matters**: The enrollment page needs this relaxation (it posts with `referrerPolicy: 'origin'`), and the stricter comparison is an improvement, but it is a repo wide auth adjacent change bundled into a face enrollment slice with no acceptance criterion behind it. The risk is the scheme guess: `request_origin_scheme` reads `HTTPS`, then `SERVER_PORT` 443, then `REQUEST_SCHEME`, and returns `http` otherwise. On any host behind a TLS terminating proxy that leaves all three unset, every existing POST that passes CSRF would start failing with "Request cannot be processed". That is correct on this cPanel host and wrong anywhere the guess is wrong.
**Suggested fix**: Keep the change, but call it out in the spec or the commit message, and consider `X-Forwarded-Proto` for the proxy case. `admin/includes/security_origin_test.php` covers the direct cases well.

### 🟡 Consent code and consent naming survived the drop, `admin/includes/staff_attendance_enrollments_engine.php:69`
**Problem**: `sae_msg_consent`, `sae_consent_is_true` and `sae_status_label` (lines 69, 142, 114) are defined and never called, `app/admin/staff_attendance_enrollments.php` still ships `<form class="sae-consent">` with `sae-consent-error`, and the page JS still has `hideConsent`, `showConsentError` and `showEnrollForm`.
**Why it matters**: AC-8 and AC-9 remove the consent tick and say not to read posted consent. Dead consent helpers and consent named elements invite a later reader to wire up a control the spec deleted, and they make the "no consent" rule look unimplemented to anyone auditing it.
**Suggested fix**: Delete the three engine functions and rename the form and the three JS helpers to something like `sae-save`, `hideSave`, `showSaveError` and `showSaveForm`.

### 🟡 The nav order test cannot prove the order it claims, `admin/includes/staff_attendance_enrollments_engine_test.php:263`
**Problem**: The assertion is `strpos($pagePhp, "staff_attendance_enrollments_ajax.php") !== false` under the label `page includes ajax before authenticate via require`. It proves the filename appears, not that it appears before `includes/authenticate.php`.
**Why it matters**: That ordering is the whole point of AC-15 and the reason a dead session returns JSON 401 instead of an HTML login page. A misleading green tick here is worse than no test, because a later refactor that moves the include after `authenticate.php` would keep the test passing.
**Suggested fix**: Assert `strpos($page, 'enrollments_ajax.php') < strpos($page, 'authenticate.php')`, and do the same for the engine include.

### 🟡 The engine test bootstraps live MySQL and can abort without a report, `admin/includes/staff_attendance_enrollments_engine_test.php:399`
**Problem**: `sae_test_live_db()` does `require_once app/db_connect.php`, and that file `die()`s with the raw PDO error when the connection fails. The test then records `$live instanceof mysqli` as a code failure. Temporary tables make the run safe and it passes today, but the bootstrap is outside the test's control.
**Why it matters**: A database outage or a missing credential turns into either a silent `die()` with no failure list, or a failure that reads like a code defect. AC-17 only excuses live B2 and live Node, not live MySQL, so the dependency is allowed, but the failure mode should still be honest.
**Suggested fix**: Guard with `is_file`, connect inside a `try` and skip the live section with a clear "skipped: no database" line instead of a failure. The three test contexts that still pass `'consent' => true, 'phone' => '...'` (lines 423, 476, 528) should also be dropped, since the engine no longer reads either key and the tests never assert they are ignored.

### 🟡 `saf_strip_raw` misses one of the spec's image keys, `admin/includes/staff_attendance_face_engine.php:268`
**Problem**: The strip list is `image`, `referenceImage`, `capturedImage` and `referenceImageUrl`. AC-3 lists five names and the fifth reads as a duplicate of `capturedImage`, which I read as `capturedImageUrl`.
**Why it matters**: Low, because `raw` is discarded by the enrollment engine and nothing persists it. But a future caller that keeps `raw` for logging would carry a captured image URL through with it, and the spec's own typo means whoever fixes this should also fix the spec line.
**Suggested fix**: `unset` `capturedImageUrl` as well, and correct the AC-3 wording.

## Nits

- ⚪ `admin/includes/staff_attendance_photos_engine.php:760`, `sap_purge_other_keys` is now unreachable (the old Photos confirm path was its only caller) and duplicates `sae_sweep` with a different page cap. Two sweep implementations will drift; either delete it or have `sae_sweep` call it with a cap parameter. `sap_staff_options` and `sap_staff_id_from_key` lost their callers too and are now covered only by tests.
- ⚪ `admin/includes/staff_attendance_enrollments_engine.php:1180`, `sae_list` calls `sap_school_id` a second time via `sae_school_id_value` after `sae_config_flags` already ran it. Pass the value down.
- ⚪ `admin/includes/staff_attendance_enrollments_ajax.php:150`, `(int) ($_POST['staff_id'] ?? 0)` accepts `12abc` as staff 12. AC-7 and AC-18 say a non integer is `That staff member was not found.`.
- ⚪ `app/admin/scripts/staff-attendance-liveness.js:298`, `grabFrame` encodes every evidence frame at quality 0.9 and full camera resolution before the page re-encodes the chosen frame at 720 px and 0.85. Only the uploaded bytes need to meet AC-6, but the intermediate frames are far larger than they need to be on a phone.
- ⚪ `app/admin/scripts/staff-attendance-liveness.js:96`, audio clips are cache busted with a hardcoded `?v=1` while every other asset uses `get_v()` filemtime.
- ⚪ `admin/includes/staff_attendance_enrollments_engine.php:198`, `sae_call_sap` returns `sap_fail(sap_msg_store())` when a helper is missing, so a wiring mistake reads as a store outage rather than as a missing include.
- ⚪ `skills-lock.json` is untracked at the repo root while `.agents/` is gitignored. AGENTS.md says agent skills were declined, so a plain `git add -A` would commit a tooling lock file the project says it does not use.

## Strengths

- The engine is genuinely pure: no echo, no superglobals, `mysqli` plus `$ctx`, and every storage and matching call goes through `$ctx['sap']` and `$ctx['saf']`, which is what made the duplicate phone test under `MYSQLI_REPORT_STRICT` and the copy failure test possible without live B2 or Node.
- The copy and pending key handling matches AC-9 closely, including the two easy to miss branches: a failed row update deletes the new final object and leaves pending alone, and a copy or HEAD failure skips both the sweep and the pending delete because pending is still the live portrait.
- `sap_object_key_allowed` plus the exact pending name check closes the obvious ways to point `confirm_face`, `enroll`, `view` or `remove` at someone else's object, and the tests exercise the foreign prefix, the traversal case and the shared suffix trap (`-12.jpg` versus staff 2).
- The Photos retirement is complete: GET is a 302, any POST is a 410 after the 401 and 403 gates, nothing links to the page, the JS is gone, and the `.sap-sheet` rules the view sheet depends on were kept.
- All user text on the list is escaped in `esc()`, `list` JSON carries no `object_key` or `photo_object_key`, and nothing logs a presigned URL.

## Test coverage

There is no test runner by project convention, and that is fine here: this change adds five standalone scripts and I ran all five, all green. `staff_attendance_face_engine_test.php` is the strongest of them, using a temporary `ask_zap.php` to exercise the real config loader, image sides, parse fixtures and typed statuses without a network call. `security_origin_test.php` and `staff_attendance_settings_engine_test.php` are focused and behavioural.

The enrollment engine test does the hard part well: temporary tables let it prove the 1062 rollback, the attempts row shape, the retake reusing `enrollment_id`, the mint refusal, and the sweep suffix rule against a real MySQL. What it does not cover is the two behaviours flagged above (the missing staff message, and a re-test whose `tested_at` does not change), and roughly a dozen of its assertions are source greps rather than behaviour, including the ordering one called out in the Minor section.