# Review, master (attendance settings, spec 0003), 2026-10-27

**Reviewed by**: sonnet (claude-sonnet-5-5) (author on composer 2.5)
**Scope**: 9 files (11 on disk, 2 generated min.css excluded), uncommitted on master
**Verdict**: Approve with nits

## Summary
Adds the Settings page, AJAX dispatcher (`save` and `reset`), and engine for the 0001 settings row. It also adds the shared `staff_attendance_can_access` helper and splits the nav into a Staff attendance group with Locations and Settings tiles. The code follows spec 0003 closely and the AGENTS.md rules: the engine is pure, prepared statements are used throughout, and CSRF, session and role gates run in the right order. The extra-school path restores the session database in `finally`. No blockers or majors. The findings are small validation and error-masking gaps.

## Minor
### 🟡 Digit regexes accept a trailing newline, `admin/includes/staff_attendance_settings_engine.php:144,157,172`
**Problem**: `preg_match('/^...$/')` without the `D` modifier lets `$` match before a final `\n`. I confirmed `"50\n"` passes the percent pattern. The same applies to the GPS and minutes patterns. AC-7 says strings that do not match are refused.
**Why it matters**: Low impact because `(int)` cast gives the right number and nothing is stored raw. It is still a deviation from the "refuse, do not clamp" contract.
**Suggested fix**: Add the `D` flag or use `\z` in the percent, GPS and minutes patterns (the late-after pattern is safe because the value is trimmed).

### 🟡 Any DB error is reported as "tables are missing", `admin/includes/staff_attendance_settings_engine.php:108-111,137-139`
**Problem**: `sas_select_row` returns `missing_tables` whenever `prepare` fails. `sas_get` returns `missing_tables` for every `Throwable`. A dropped connection or a permissions error looks the same as a missing 0001 migration.
**Why it matters**: The admin sees the wrong message, and the Save and Reset buttons are hidden on the page. That makes a transient fault look like a schema problem.
**Suggested fix**: Only map to `missing_tables` when `sas_table_exists` is false (already checked first). Return a generic "Could not read settings." for other failures.

### 🟡 Restore of the session database is sanitized and unchecked, `admin/includes/staff_attendance_settings_engine.php:233-246`
**Problem**: `$home_db` goes through `preg_replace('/[^a-zA-Z0-9_]/', '', ...)` before the restore `select_db`, and the return value is ignored. A tenant database name with a hyphen or dot would be restored to a different (likely nonexistent) name, leaving mysqli on no database for the rest of the request.
**Why it matters**: AC-10 and the invariants require mysqli to be back on the session database after every extra write. The current school is written first, so no wrong-tenant write follows in this request. It still violates the invariant if names are ever not `[A-Za-z0-9_]`.
**Suggested fix**: Do not sanitize `$home_db`, since it comes from the server session, not the client. Check the return of the restore `select_db`, and fail loudly or log if it returns false.

## Nits
- ⚪ `admin/includes/staff_attendance_settings_engine.php:53`, `SHOW TABLES LIKE` treats `_` as a wildcard. Harmless for a fixed table name. `information_schema` or a `SELECT 1 ... LIMIT 0` probe would be exact.
- ⚪ `admin/includes/staff_attendance_settings_ajax.php:175`, `global $info, $table_name, $mysqli, $security;` at file scope is a no-op. Remove it, or keep it only if this file may be included from inside a function.
- ⚪ `app/admin/scripts/staff-attendance-settings.js:243-245`, a server error shows both a toast and the inline `sas-save-error`. Pick one, or accept it as intentional.
- ⚪ `app/admin/staff_attendance_locations.php:47`, the diff only reorders the `onclick` and `href` attributes on the back link. It is noise, so revert it or leave it.
- ⚪ `request_is_same_domain()` fails when no Referer is sent (`security.php:231`). Spec 0003 requires it, but a WebView or referrer-policy setup that strips Referer would give a 403 on save. `/check verify` should exercise this in the real WebView.

## Strengths
- The access helper is a single shared source, and `sal_can_access` stays a one-line wrapper as the spec asks.
- AJAX ordering is right: dead session returns 401, then CSRF 403, then role 403. Off-list school ids abort before any write. `check_csrf_token()` is avoided.
- `finally` restore, SELECT-before-UPDATE with no `affected_rows`, `UPDATE` only and never `INSERT`, and prepared statements everywhere. The engine never echoes and never reads superglobals.
- Checkout switch and hidden-minutes naming is handled so only one control carries `min_checkout_minutes`. The flatpickr SRI hashes match jsDelivr (I recomputed both). The native time input still works if the script fails.
- CSS is scoped under `.staff-attendance-settings-page` with the correct html/body hide for `page-title-clear`. Output escaping is correct (`htmlspecialchars`, JSON hex flags, `esc()` in the flash HTML).

## Test coverage
TESTS = none-by-design. The gate is `/check verify`. Points it should exercise: the trailing-newline case above, flatpickr binding to an `input type=time` (value posts `HH:MM`), a missing Referer in the WebView, and the extra-school `missing_tables` flash with the session database restored.