# Review, master (staff check in, spec 0007), 2026-11-03

**Reviewed by**: Claude Opus 5.5 (author on Composer 2.5)
**Scope**: 11 files, uncommitted
**Verdict**: Blocked

## Summary
This change adds the staff Check in page on the school app: an access helper, nav tiles, a page, an AJAX dispatcher, a large engine for mint and consume, page JS for GPS, phone, and liveness, CSS, and an in memory engine test. The server side is careful and follows spec 0007 closely: the client never picks `staff_id` or `location_id`, CSRF plus same domain run on every POST, uploads are checked and never stored, the lock and replay rules match the spec, and rendering is XSS safe. The headline problem is that the page JS does not parse, so the page does nothing in a browser. There is also a spec level dead end when `allow_checkout` is 0, consume does no transaction, and the tests miss most of the consume fail branches.

## Blockers
### 🔴 Page JS has a syntax error, the whole script fails to load, `app/admin/scripts/staff_attendance_checkin.js:684`
**Problem**: In `paintChallenges` the guard `if (!liveEl) { return;` is never closed. The next two lines declare `var keys` twice (a leftover `['neutral'].concat(...)` line sits above the real one). `node --check` fails with `SyntaxError: Unexpected token ')'` at line 1101. Because the whole file is one IIFE, nothing runs: no GPS watch, no tab handlers, no phone submit, no liveness.
**Why it matters**: The Check in page is dead on every device. No staff member can clock in or out. The engine test still passes because it only greps the JS for strings like `watchPosition`, so the suite gives a false green.
**Suggested fix**: Close the `if` block right after `return;` and delete the stale `var keys = ['neutral']...` line. Add `node --check` (or an equivalent parse check) to the JS string tests in `staff_attendance_checkin_engine_test.php` so a broken file cannot pass.

## Major
### 🟠 With `allow_checkout` set to 0 a staff member can check in only once, ever, `admin/includes/staff_attendance_checkin_engine.php:1562`
**Problem**: Check in refuses when the staff member has any day with `check_out_at IS NULL` (AC-9). When the school turns check out off (spec 0003 allows `allow_checkout = 0`), the Out tab is hidden and consume refuses `check_out` with `checkout_off`. So the first day row never closes, and every later check in fails with `already_in` and the message "Check out first", which the person cannot do.
**Why it matters**: Any school that turns check out off gets one day of attendance per staff member and then a permanent lockout. The code matches the letter of AC-9, so this is a spec gap, not an author mistake, but it will bite on the first school that uses the setting.
**Suggested fix**: Route this to /architect to amend spec 0007. A likely rule is that the open day check only applies when `allow_checkout` is 1, or only to an open row on today's `attend_date`, with the unique `(staff_id, attend_date)` key still preventing a second row on the same date. Then add a test for check in with `allow_checkout = 0` and an open day from yesterday.

### 🟠 Consume has no transaction, and the token is not reread under the lock, `admin/includes/staff_attendance_checkin_engine.php:1458`
**Problem**: AC-15 says to hold the lock through the day write and "release after COMMIT or rollback". The engine never calls `begin_transaction` or `commit`. The day INSERT or UPDATE, `sac_set_consumed`, and the success attempt are three separate autocommit writes. The return value of `sac_set_consumed` is ignored, so a second request with the same token that loaded the row before the first one finished will still run detect, HEAD, and verify, and then write its own fail attempt and possibly overwrite nothing silently. The token row is loaded at line 1420, before `GET_LOCK`, and never reloaded after the lock is held.
**Why it matters**: If PHP dies or the DB call fails after the day write but before `consumed_at` is set, the day exists but replay can never find it, so a retry after a lost reply shows `already_in` instead of the success card (breaks AC-11 replay). Two taps on Send with the same token both call Node, doubling the face service load and logging a confusing second attempt.
**Suggested fix**: After `GET_LOCK` succeeds, reload the token by hash and rerun the `consumed_at` and replay check. Wrap the day write, `sac_set_consumed`, and the success attempt in one transaction, commit before `RELEASE_LOCK`, and roll back on error. Treat a zero row `sac_set_consumed` as "already used" and go to replay.

### 🟠 Consume fail branches and several mint fails have no tests, `admin/includes/staff_attendance_checkin_engine_test.php:200`
**Problem**: The suite covers the happy check in, `already_in` from an open day, `no_checkin`, `already_out`, `too_soon`, `checkout_off`, replay, `token_used`, and `lock_busy`. It does not cover: `token_expired`, `invalid_image`, `token_bad` for an unknown intent or unknown hash, `no_face`, `many_faces`, `photo_missing` (bad key, HEAD missing, presign fail), each `saf_verify` typed status (`not_configured`, `timeout`, `unavailable`, `no_face`, `match_error`), `no_match`, `outside_location` at mint and at consume, `not_enrolled`, `inactive_staff`, the duplicate key `already_in` path (the `insert_day_errno` hook exists in the engine but no test sets it), a successful check out and its `location_name` snapshot, and `is_late`. The "boundary" fence test uses distance 0 against radius 0, and the test labelled "distance equal to radius is inside" actually uses distance 0 against radius 50, so the real `distance_m == radius_m` case from AC-17 is not tested. The test also passes with broken page JS (see the blocker).
**Why it matters**: These are the security and error branches the spec cares most about (generic messages, `consumed_at` rules, no leak of who failed). A regression in any of them would ship silently.
**Suggested fix**: Hand these cases to /test. Use a point offset by a known distance and set `radius_m` to that computed distance for the boundary case. Add a parse check for the JS.

## Minor
### 🟡 Config blocked mint and consume write attempts that can fatal when tables are missing, `admin/includes/staff_attendance_checkin_engine.php:1194`
**Problem**: When `sac_action_block` fails, mint logs a `not_configured` attempt and consume does the same through `sac_consume_fail`. If the block reason is "The attendance tables are missing.", `sac_log_attempt` prepares an INSERT on a missing table. Under PHP 8.1+ default mysqli strict reporting that throws, and the request becomes the fatal retry screen instead of the AC-13 message. AC-6 also does not list a config attempt in the mint order.
**Suggested fix**: Return the block message without logging when tables are missing, or skip logging for any config block.

### 🟡 Consume does not recheck activation or that the enrollment still matches the token phone, `admin/includes/staff_attendance_checkin_engine.php:1484`
**Problem**: `sac_enrollment_for_staff` only checks `status = 'current'` and `current_staff_id = staff_id`. It does not require `current_phone = phone_normalized` or that the phone equals `token.phone_normalized`, and `staff.activation` is not checked at consume. A staff member deactivated, or reenrolled with a new phone, inside the 10 minute window still clocks.
**Suggested fix**: Match the full AC-6 current enrollment rule at consume and recheck `activation = 'activated'`, failing with the generic line.

### 🟡 Mint logs a token insert failure as `not_enrolled`, `admin/includes/staff_attendance_checkin_engine.php:1280`
**Problem**: A DB failure inserting the token is logged with `reason_code` `not_enrolled` and `staff_id` NULL. That hides a server fault as a user fault in the later report.
**Suggested fix**: Use a distinct code such as `match_error` or a new `token_error` string, keep the generic user message.

### 🟡 Every mint and consume rebuilds the full page config, `admin/includes/staff_attendance_checkin_engine.php:1151`
**Problem**: `sac_action_block` calls `sac_page_config`, which runs six `SHOW TABLES`, reads settings, loads all active locations, resolves the school, reads config, and for staff looks up the autofill enrollment. Mint then reads settings and locations again, and consume reads settings again. `sac_days_for_staff` at line 1623 loads every day row for the staff only to test whether any exist.
**Suggested fix**: Give the block check a lean path (config flags plus one tables check plus settings) and reuse its settings in mint and consume. Replace the all days read with `SELECT 1 ... LIMIT 1`.

### 🟡 Clock time is taken before the lock wait and the Node calls, `admin/includes/staff_attendance_checkin_engine.php:1446`
**Problem**: `$now` is computed before `GET_LOCK` (up to 40 seconds) plus detect and verify. `check_in_at`, `is_late`, and the `too_soon` math use that earlier time. A person who arrives at 07:59:50 behind a busy lock is recorded on time even if the write lands after 08:00. The skew is small but it is in the person's favour and not visible.
**Suggested fix**: Take the expiry check time early, but take a fresh `$now` after verify for the day write and `consumed_at`, keeping the two equal for replay.

### 🟡 Success timer is not cleared on tab switch or reset, `app/admin/scripts/staff_attendance_checkin.js:494`
**Problem**: `successTimer` is only cleared inside `showSuccess`. If the attendant taps a tab during the 4 second card, a new GPS watch starts, and when the old timer fires it calls `startGps()` again, wiping a good fix or a phone the next person has started typing.
**Suggested fix**: Clear `successTimer` in `switchTab`, `resetToGps`, and `startGps`.

### 🟡 Mint has no busy guard and no network error handling, `app/admin/scripts/staff_attendance_checkin.js:500`
**Problem**: A double tap on Continue mints two tokens and calls `openSheet` twice. A network failure rejects the `fetch` with no `.catch`, so the phone step shows nothing and the error is unhandled. In `runCapture`, a network failure on consume is a `TypeError` that `failMessage` maps to "The face check did not load", which is misleading.
**Suggested fix**: Disable Continue while a mint is in flight, add a `.catch` that calls `failRestart` with a plain network message, and give network errors their own message in `failMessage`.

### 🟡 Oversized POST bodies skip the AJAX block entirely, `admin/includes/staff_attendance_checkin_ajax.php:120`
**Problem**: When a body exceeds `post_max_size`, PHP empties `$_POST` and `$_FILES`, so `isset($_POST['action'])` is false and the request falls through to the HTML page. JS then shows "Could not read the server reply." AC-8 wants host limits to give the JPEG message and an `invalid_image` attempt. The 720 pixel shrink makes this unlikely, so it is minor.
**Suggested fix**: Detect a POST with an empty `$_POST` and a nonzero `CONTENT_LENGTH` above `post_max_size`, and answer the JPEG message as JSON.

## Nits
- ⚪ `app/admin/partials/zap-nav-menu.php:538`, the `!in_array($table_name, ['admin','director','backend'])` test is always true once `$table_name === 'staff'`; drop it.
- ⚪ `admin/includes/staff_attendance_checkin_engine.php:1380`, `$held` and `unset($held)` are dead code.
- ⚪ `admin/includes/staff_attendance_checkin_engine.php:1266`, test hooks (`token_raw`, `now`, `mem`, `staff`) live in production paths; safe today because `sac_ajax_ctx` never sets them, but worth a one line note.
- ⚪ `admin/includes/staff_attendance_checkin_engine.php:1137`, when the school is missing from the switch list the banner shows only that sentence and drops the store or matching sentence; confirm against spec 0006 AC-14.
- ⚪ `app/admin/staff_attendance_checkin.php:52`, the page also adds the enrollments app and page classes to reuse their styles; a shared class name would be clearer.

## Strengths
- Identity and place are decided only on the server: the client never sends `staff_id`, `location_id`, or a face result, and success JSON carries no `staff_id`, `face_score`, late flag, or portrait URL.
- Upload handling is tight: `is_uploaded_file`, size cap, JPEG magic sniff, bytes passed straight to `saf_detect` and `saf_verify`, never stored; the presigned reference URL is short lived and never logged.
- The AJAX guard order is right (401 dead session, then CSRF plus `request_is_same_domain`, then role, all 403), uses `csrf_token_is_valid` and not `check_csrf_token`, and JSON uses the hex flags.
- JS renders server text with `textContent`, and the one `innerHTML` path escapes allowlisted challenge keys, so there is no XSS path.
- Generic messages are applied consistently for the AC-12 codes, the lock name uses the sanitized `db_name`, `lock_busy` and unknown token correctly leave `consumed_at` NULL, and the lock is released in `finally`.
- The in memory `$ctx['mem']` fixture lets the engine run without a database, which keeps the test safe to run anywhere.

## Test coverage
`php admin/includes/staff_attendance_checkin_engine_test.php` passes, and `php -l` is clean on all new PHP files. Covered: phone normalize, mask name, fence tie, accuracy equal and over max, invalid phone and duplicate phone attempt shape, mint JSON and `mint_ok` / `ok_mint`, happy check in with detect, HEAD, verify order, `already_in`, `no_checkin`, `already_out`, `too_soon` at the exact minute, `checkout_off`, replay ignoring intent, `token_used`, `lock_busy`, lock name, and string checks for page, AJAX, nav, and access. Not covered: all consume fail branches after the token load (expiry, image, face count, photo, verify statuses, `no_match`, consume time `outside_location`), the duplicate key path, a successful check out, `is_late`, mint `not_enrolled` / `inactive_staff` / `outside_location`, a true `distance_m == radius_m` boundary, and any parse check of the page JS, which is broken today while the suite reports green. The enrollments test was updated for five nav adds, which matches the new nav.