# Clinic CMS — audit report Audit of the Clinic Management System at `C:\laragon\www\Clinic`, serving `clinic.tmsoagency.com` from this directory. Conducted 2026-09-03/04 against `main` at `213f7ab`, with the working tree carrying uncommitted document-verification and branding work. Companion documents: [CMS-AUDIT.md](CMS-AUDIT.md) (what the system is), [CMS-AUDIT-PLAN.md](CMS-AUDIT-PLAN.md) (the checklist and how each verdict was reached), [CMS-AUDIT-PROGRESS.md](CMS-AUDIT-PROGRESS.md) (working notes). Statuses used throughout: **PASS** · **FIXED** · **PARTIAL** · **FAIL** · **UNVERIFIED**. --- ## Executive summary This is a well-built system. The architecture is real rather than aspirational — layering is enforced by a tool and by tests, money is integer minor units everywhere with a single value object owning the arithmetic, clinical records are versioned rather than overwritten, and the audit trail is hash-chained under a lock. Most of what I checked was already right, and the code explains itself unusually well. The defects that matter were all of one kind: **a rule that was stated but not enforced.** Not missing rules — the system knows what it believes. In seven places the belief was written down in a comment, or asserted by a test that stopped one step short, while the mechanism underneath did something else. Nine issues were found and fixed, three of them serious: - **Two cashiers could take the same balance twice.** Every settlement guard read the invoice from a snapshot loaded earlier in the request. Reproduced: a 100.00 invoice paid 200.00; a 50.00 invoice paid 90.00; a full refund taken twice, the second committing a payment row and *then* breaking the invoice's own totals. - **Suspending an account revoked nothing.** The status check sat in a `Gate::after` callback, which in Laravel cannot overturn a decision — only fill in an absent one. A suspended Super Admin kept everything. An API token issued before suspension kept working indefinitely. - **The whole patient-records directory had an HTTP route.** Laravel's default `serve => true` was left on a disk rooted at `storage/app/private` — patient documents, portal uploads, filed report PDFs, and the private key those PDFs are signed with — with `GET` and `PUT` across all of it. Also fixed: a retracted laboratory result could never be replaced (the worklist asked for a result the system would then refuse); dispensing medicine wrote nothing to the audit trail while *reversing* a dispense did; the `pharmacist` role was granted nothing by the pharmacy module; the appointment screens offered buttons the server would refuse; a duplicate translation key silently replaced a placeholder; and the specimen payment-gate override was missing from its own published interface. All three quality gates — Pint, PHPStan level 6, deptrac — were red on arrival and are green now. **Production readiness: fit for continued clinical use.** The blocking issues found are fixed and covered by tests. What remains open is operational rather than structural, and is listed under Remaining risks — chiefly that backups do not leave the machine and restore has never been rehearsed. --- ## System architecture Laravel 12 modular monolith: 1,100 PHP files, fourteen modules, 79 tables, 124 test files. Layering (Controller → Service → Repository/Query → Model, over a shared Foundation) is enforced by `deptrac` and by architecture tests that forbid core from importing a module and forbid modules from reaching each other except through `Contracts` and `Events`. **Tenancy is physical, not logical.** One clinic, one installation, one database, one domain. There is no `clinic_id` in the schema and nothing is written as though there could be. `branches` is *inside* one clinic. This materially changes what "clinic isolation" means here and is addressed under that heading below. Full inventory in [CMS-AUDIT.md](CMS-AUDIT.md). --- ## Modules audited All fourteen. Depth varied with risk: Billing, Laboratory, Auth, Pharmacy and Patients were traced end to end through their services; Appointments, Consultations, Prescriptions, Portal, Messaging and Updates were read at the service and route level with targeted checks; Cdss, Licensing and Settings were surveyed. --- ## Critical findings None outstanding. One issue was of critical *class* — financial corruption — and is fixed: ### Settlement decided from a stale invoice — FIXED **Where.** `PaymentService::take()`, `::refund()`, `::reverse()`; `CreditNoteService::write()`. **Rule.** The service's own docblock: *"Nothing is taken that cannot be accounted for. More than the balance is refused, because a clinic with no patient-account ledger has nowhere to put an overpayment and no way to find it again."* **Problem.** Each guard read `$invoice->balance`, `->paid_total` or `->credited_total` from the `Invoice` model the request had loaded, decided against that snapshot, and only then wrote — with no row lock, and with the decision outside the write's transaction. Two requests holding the same snapshot both decide yes. **Failure scenario, reproduced.** `ConcurrentSettlementTest` hydrates the invoice twice, which is exactly what two simultaneous HTTP requests produce: | Case | Expected | Actual before fix | |---|---|---| | 100.00 invoice, two cashiers take 100.00 | second refused | **both accepted**, paid 200.00, balance −100.00 | | 50.00 invoice, three payments of 30.00 | one accepted | **all three accepted**, 90.00 taken | | 80.00 paid, full refund requested twice | second refused | refund row **committed**, then the settlement write threw on the unsigned `paid_total` column — ledger and invoice permanently disagreeing about money that had left the drawer | | Credit 60.00 twice on a 60.00 invoice | second refused | **both accepted** | **Root cause.** No serialisation on the invoice row. The rest of the system locks correctly — `FefoAllocator`, `lockDoctorDay()`, `SequenceGenerator`, `CashSessionService` — so this was a gap in one area, not the house style. **Impact.** Financial corruption on the busiest counter in the clinic. A patient charged twice; a refund paid twice; an invoice showing a negative balance with no mechanism to represent it. **Severity.** CRITICAL. **Fix.** `InvoiceServiceInterface::settleUnderLock()` — opens a transaction, takes `lockForUpdate()` on the invoice by primary key, hands the *locked* row to the mutation, refreshes settlement inside the same transaction, and brings the caller's copy up to date. Every guard moved inside it. **Found while fixing.** `setRawAttributes($attrs, sync: true)` on a model whose money casts have already been read calls `syncOriginal()` *before* clearing the cast cache, so the stale amounts are written straight back over the new ones. `settleUnderLock` uses `refresh()` instead, with the reason recorded at the call site so it cannot be "simplified" back. **Tests.** `app/Modules/Billing/Tests/Feature/ConcurrentSettlementTest.php` — 6 cases, all failing before, all passing after. 49 billing tests pass in total. --- ## High findings ### Suspending an account revoked nothing — FIXED **Where.** `app/Providers/AuthServiceProvider.php`. **Rule.** The comment beside the code: *"A suspended or inactive account fails every check, even if its roles still carry the permission. Status is the outer gate."* **Problem.** The check was a `Gate::after` callback. Laravel merges an after callback's answer with `$result ??= $afterResult` — it can fill in a decision nobody made and can never overturn one. Every ability a policy granted, and every ability the Super Admin bypass granted, went straight past it. **Failure scenario, reproduced.** `InactiveAccountGateTest`: a suspended clinician kept an ability their role granted; a suspended **Super Admin** kept everything; a suspended administrator kept `settings.update`. **Impact.** Sign-in does refuse an inactive account, so the exposure is a session open at the moment of suspension — and, indefinitely, **an API token issued before it**, because `auth:sanctum` authenticates by bearer token and never revisits the sign-in rules. Suspending a compromised account did not lock it out. **Severity.** HIGH. **Fix.** Two places, because two engines answer authorization here: - `Gate::before` asks the status first and returns `false`, *ahead* of the Super Admin bypass — a bypass that outranks suspension cannot be switched off. - `User::hasPermissionTo()` refuses too. The permission package registers its own `Gate::before` when the gate is first resolved, so it always runs ahead of ours and answers bare permission names itself; it calls this method. **Tests.** `tests/Feature/InactiveAccountGateTest.php` — 4 cases. 81 Auth tests still pass. ### The patient-records directory was web-served — FIXED **Where.** `config/filesystems.php`, the `local` disk. **Problem.** Left on Laravel's default `serve => true` with its root at `storage_path('app/private')` — the *same directory* as the `private` disk two entries below, which is documented as *"Protected health information. Never web-accessible… Every read goes through a controller that runs a policy check and writes a record-access audit entry."* `serve => true` makes the framework register `GET /storage/{path}` and `PUT /storage/{path}` across that whole root. Confirmed present on the live install by route list. The directory holds `patients/`, `portal-quarantine/`, `issued-documents/` (the filed PDFs whose digest makes a forgery detectable) and `keys/document-signing.key` — the ECDSA private key those PDFs are signed with. **Impact.** Both routes require a valid URL signature, so this was **not** reachable by a stranger and not remotely exploitable without `APP_KEY`. What it was: a second door onto patient records with no policy and no access audit, and a `PUT` that would let a filed report or the signing key be overwritten by anyone holding a signed upload URL. Nothing in the application names the `local` disk — every real read and write names `private`, `public`, `backups` or `updates`. The routes bought nothing. **Severity.** HIGH (defence in depth over PHI; not independently exploitable). **Fix.** `serve => false`, plus an explicit `visibility => 'private'`. **Tests.** `tests/Feature/PrivateStorageIsNotServedTest.php` — 2 cases. **Verified live:** `GET /storage/keys/document-signing.key` now returns 404. ### A retracted laboratory result could never be replaced — FIXED **Where.** `LabResultService::enter()` / `::retract()`. **Rule.** `retract()`'s own comment: *"The analyte goes back to pending: it still needs reporting, and a worklist that showed it as done would lose it."* **Problem.** `currentResult()` selects on `superseded_by_id IS NULL`. Nothing supersedes a retraction, so a withdrawn result stayed "current" *and* verified — and `enter()` refused every attempt to report a replacement with *"This result has already been released. Amend it instead."* **Failure scenario, reproduced.** `RetractedResultTest`: retract a result reported off the wrong tube, then try to enter the right one — refused. The worklist asks for a result the system will not accept. The only escape was to *amend* a result that had been withdrawn, which says the wrong thing about the record. Two rows would also both have carried `version = 1`, leaving `orderByDesc('version')` to break the tie however the database felt like it — which is how a withdrawn value gets displayed as the current one. **Why it was not caught.** The existing test — `it sends a retracted result back to be reported again` — asserted the analyte was Pending and stopped there. The appearance of the rule, not the rule. **Impact.** A clinically significant workflow dead end. A wrong result cannot be corrected by the intended route. **Severity.** HIGH. **Fix.** A withdrawn result no longer blocks a replacement, and the replacement supersedes it at `version + 1`. This reuses the versioning already in place: the withdrawn row drops into the history the screens already render, and exactly one row stays current. **Tests.** `app/Modules/Laboratory/Tests/Feature/RetractedResultTest.php` — 5 cases, including one asserting the guard is *not* relaxed for a live verified result. 323 Laboratory tests pass. ### The invoice arithmetic tests had not run since the enum moved — FIXED **Where.** `tests/Unit/InvoiceCalculatorTest.php`. **Problem.** It imports `App\Modules\Billing\Enums\DiscountType`. That class does not exist — the enum lives at `App\Foundation\Billing\DiscountType`, moved there so a form request could consult it without reaching into a module. The import was never updated. **Impact.** Five tests died on *"Class not found"* rather than running, among them `it foots exactly across many awkward lines`, `it never discounts more than the bill is worth` and `it discounts before taxing`. These cover the most safety-critical arithmetic in the product — the numbers a patient is charged — and they have not been verifying it. Present in the initial commit, so it predates this audit. Nothing concealed it: a full run always reported it. It survived because a red suite had stopped being read, which is the same reason Pint was failing on five committed files and PHPStan on nine. **Severity.** HIGH — not a live defect, but the absence of the guard against one. **Fix.** One import. All fourteen tests in the file now pass, which confirms by execution what the rest of this audit could only confirm by reading: the calculator itself was correct throughout. --- ## Medium findings ### Dispensing medicine was not audited — FIXED `DispensingService::reverse()` wrote an audit entry; `::dispense()` did not. The chain showed medicines coming back that it never showed leaving, and for a controlled drug "who gave this to whom, and when" is the question the trail exists to answer. The `dispensings` row and the stock ledger both record the facts, but neither is hash-chained, so neither can show it has not been edited. Fixed: a `pharmacy_dispensed` entry per dispensing, inside the same transaction, carrying drug, strength, quantity, prescription, patient and batch numbers. ### The pharmacist role could not use the pharmacy — FIXED `app/Modules/Pharmacy/Module.php` grants pharmacy permissions to receptionist, nurse, doctor and clinic-admin. `RoleName::Pharmacist` — a role the product ships and displays — appears nowhere in the list. Every fresh install produces a pharmacist who gets a 403 on the pharmacy queue while the receptionist can dispense. Confirmed against the live role table: `pharmacist` holds 5 permissions, none of them `pharmacy.*`. Fixed: granted `pharmacy.view`, `pharmacy.dispense` and `pharmacy.stock`, with `pharmacy.manage` (pricing) deliberately withheld, following the separation the module's own docblock already argues for — the person who counts the shelf should not also set what it is worth. **Applied to this installation on 2026-09-08**, through the module's own `install()` so the live database and the shipped defaults say the same thing rather than merely agreeing today. The role went from 5 permissions to 8; no other role changed, and nothing was removed from any of them. Nobody was assigned to `pharmacist` at the time, so no person's access changed. Verified by a probe account created and rolled back: `pharmacy.view`, `pharmacy.dispense` and `pharmacy.stock` allowed; `pharmacy.manage` and `invoices.void` refused; and the same account, once suspended, refused `pharmacy.dispense` — so the suspension gate still bites on this role too. Recorded as audit entry 4672, and the chain still verifies. ### Screens offered actions the server would refuse — FIXED The appointment detail page offered every transition the status machine allowed, and the listing offered every non-reason-requiring one, to anybody who could open them. A cashier holds `appointments.view` and neither `update` nor `cancel`, so they were shown Cancel and Check-in buttons that then 403'd. The server was right — `UpdateAppointmentStatusRequest` correctly distinguishes cancelling from progressing — but the rule lived only there. Fixed by moving it onto the transition itself (`AppointmentStatus::requiredAbility()`), which both the form request and the two screens now ask. One rule, one place. **Tests.** `app/Modules/Appointments/Tests/Feature/StatusActionsAreGatedTest.php` — 5 cases, including that the change is still refused server-side and that an unknown status is rejected rather than treated as an update. ### The specimen payment-gate override was not in its own interface — FIXED `LabOrderService::collect()` takes `$unpaidOverrideReason` — the reason recorded when a sample is taken with the laboratory's bill outstanding. The published `LabOrderServiceInterface::collect()` did not declare it. The web controller passed it by name through the interface, which works only because PHP resolves on the concrete object; the API path could not reach the rule at all, and any second implementation would have dropped it silently. PHPStan had been reporting this. Fixed: the parameter is now part of the contract, documented. ### A duplicate translation key replaced a placeholder with a label — FIXED `patients::messages.placeholders.full_name` was defined twice in both English and Arabic. The second won, so the registration form's name field showed *"Full name"* — duplicating its own visible label — instead of the intended example *"e.g. Muhammad Imran Khan"*. The comment directly above the block states that a placeholder "must never be the only place a field is named"; the surviving value was doing exactly the thing the file forbids. Removed the duplicate in both languages. --- ## Low findings ### A dead method that said the opposite of the truth — FIXED `ResultStatus::isSuperseded()` reported `Amended` as superseded. An amendment is the row that *replaces* one; the superseded row is the older `Final` one carrying `superseded_by_id`. Called from nowhere. Removed, with the reasoning left in its place so it is not re-added. ### Documentation blocks that nothing could read — FIXED `UserServiceInterface::create()` and `::update()` each carried *two* stacked docblocks, and `InvoiceServiceInterface::draftFor()` the same. Only the block touching the declaration is read, so the `@param` and `@throws` tags above it were invisible to tooling and to IDEs. Merged. ### Quality gates were red — FIXED On arrival: Pint failing on 5 committed files, PHPStan reporting 9 errors, deptrac clean. All three are green now. Two of the PHPStan errors were the substantive findings above (the missing interface parameter, the duplicate lang key); the rest were missing generics and an unanalysable `partition()` destructure, which was rewritten as an explicit filter/reject pair that reads better than what it replaced. --- ## Security findings | Check | Status | Evidence | |---|---|---| | SQL injection | PASS | Every `DB::raw`/`whereRaw`/`selectRaw` in the codebase read — all parameterised or constant | | XSS | PASS | Three unescaped outputs exist; all read. An `e()`-escaped URL, a Fortify-generated SVG, and branding CSS that is hex-validated by regex before it can be emitted | | CSRF | PASS | Laravel default; no route excluded | | Broken access control | FIXED | Suspended accounts; see High findings | | IDOR | PASS | Nested resources resolve through the parent by ULID (`$invoice->items()->where('ulid', …)`), sampled across invoices, prescriptions, patient contacts and documents | | Mass assignment | PASS | No model uses `guarded = []`; the one model without either list extends Sanctum's, which has its own | | Unsafe file serving | FIXED | See High findings | | Path traversal | PASS | No user-controlled path concatenation into the private disk; downloads resolve a stored path from a scoped model | | Sensitive data in logs | PASS | `AuditLogger::redact()` strips configured keys before the row is written | | Debug leakage | PASS | `APP_ENV=production`, `APP_DEBUG=false` confirmed at runtime | | Session security | PASS | Encrypted, secure-only, `SameSite=Lax`, database driver | | Weak tokens | PASS | Verification tokens are 32 random bytes; document codes 60 bits; both throttled | | Predictable identifiers | PASS | ULIDs in every URL; integer ids never in a payload | | Supply chain (updates) | PASS | Release signature bound to version + SHA-256 + byte size, verified after download against the bytes on disk; **fails closed** when no key is embedded | | Rate limiting | PASS | Public verification 20/min, portal upload 10/min, booking 60/min, stock search 120/min; login throttled per username *and* per IP | | Secrets in source | PASS | None found; keys are on disk outside the repository | --- ## Database findings - **Money.** Every monetary column is `BIGINT` minor units plus a currency column. `Money` is the only place arithmetic happens: exact-rational `multiplyRatio` (integer throughout, round-half-away-from-zero), largest-remainder `allocate` that is correct for negative amounts too. No float reaches a financial column. **PASS.** - **Invoice arithmetic verified against live data.** All 11 live invoices foot: `total` equals the sum of line totals, `paid_total` equals the sum of payment rows, and `balance` equals `total − paid − credited`. The one apparent exception is a voided invoice, whose balance is correctly zeroed while its total is preserved. **PASS.** - **Constraints.** Foreign keys are `RESTRICT` on anything a record's history depends on; unique `(branch_id, reference)` on every numbered document; unique `(branch_id, idempotency_key)` on `payments` and `dispensings`. The dispensing double-reversal is blocked by that constraint rather than by application logic alone. **PASS.** - **Immutability.** `AuditLog::update()` and `::delete()` throw. Clinical corrections are new versions linked by `supersedes_id`/`superseded_by_id`. **PASS.** - **Schema state.** 83 migrations, all recorded, none pending. **PASS.** --- ## Clinic isolation findings **The premise needs restating before the verdict.** The audit brief assumes shared-database multi-tenancy with a tenant column to filter on. This system does not work that way, and the difference is deliberate and documented in `docs/TENANCY.md`: each clinic gets its own database on its own host. I verified this rather than taking the document's word for it — there is no `clinic_id` in any of the 79 tables, and no code path assumes one could exist. So Clinic A cannot reach Clinic B's data because there is no query that could: they are different databases. That guarantee does not depend on every `WHERE` being correct forever, which is the stronger position. What *is* enforced in code is **branch** isolation inside one clinic, and that was audited: a global scope on every branch-owned model, automatic `branch_id` on create, and a single explicit `withoutBranchScope()` escape hatch that is greppable. `BranchForbiddenException` is exercised by the existing suite. **PASS**, with the scope of the claim stated plainly. --- ## Clinical workflow findings - Encounter notes: draft → signed → amended/retracted, with `scopeBindings()` making the router resolve a note through its own encounter. **PASS.** - Retracted results: see High findings. **FIXED.** - Reference ranges are copied onto the result rather than linked, so revising a range cannot reinterpret a result reported last year. **PASS.** - Second-verifier rule: where required, a person's own entries are held rather than the whole panel refused; release is all-or-nothing inside one transaction. **PASS.** - Critical values are announced on entry and on amendment, and acknowledgement is tracked. **PASS.** --- ## Prescription findings Issued prescriptions are immutable; corrections reissue with a link to the original. Printing counts a copy when the browser's print dialog closes rather than when the PDF is fetched, which is as close to "actually printed" as a browser will say — an over-count is answerable, an unrecorded printout is not. Every prescription is registered in the document register with a QR code addressing a random token. **PASS.** --- ## Laboratory findings Catalogue, panels, ranges, cut-offs, microbiology and titres are all configuration rather than hard-coded clinical judgement — the system holds what a pathologist approved, which is the right division of responsibility. Specimen lifecycle (collect → receive → reject) and the payment gate are enforced in the service, not the screen. Result lifecycle fixed as above. **FIXED / PASS.** Reference ranges and critical limits are the clinic's to approve. The imported catalogue remains switched off pending that approval, which is correct and should stay correct. --- ## Pharmacy findings `FefoAllocator` locks candidate batches in expiry order — which is both the FEFO rule and deadlock avoidance, since two dispensings taking the same rows in different orders would each hold what the other waits for — and refuses outright to run outside a transaction, because a lock taken outside one releases immediately and two dispensings would oversell. That is the strongest piece of concurrency work in the system. Reversal returns stock to the batches it came from rather than to whichever is nearest expiry now, so a recall stays answerable. The bill is deliberately not touched: a charge on an issued invoice is corrected by credit note, by somebody who can see the payment that may already have been taken. Audit gap and role grant fixed as above. **FIXED / PASS.** --- ## Billing findings See Critical findings for the settlement race. Beyond it: `InvoiceCalculator` applies discount before tax — taxing an undiscounted amount charges the patient tax on money they did not pay — and allocates an invoice-level discount across eligible lines by largest remainder so the parts sum back exactly to the discount given. Lines marked non-discountable are excluded from both the base and the allocation. Verified by reading and by the existing unit tests. **PASS.** Cash sessions: one open till per cashier, taken under a lock; a reversal lands in the cashier's *current* shift rather than reaching back into one that was closed and signed off. **PASS.** --- ## Document and PDF findings Report and prescription rendering were verified earlier in this working session by generating PDFs and extracting per-page text — including the pagination and footer-pinning work already in the tree. Both carry clinic identity, patient, document number, version, issuing user and a QR code. Invoice, receipt and credit-note PDFs were **not** re-rendered in this pass, and Arabic/RTL PDF layout was **not** re-verified. Marked PARTIAL rather than PASS. --- ## Document verification findings The register (`issued_documents`) holds number, version, content hash, stored PDF with its SHA-256, an ECDSA P-256 signature and a 32-byte random token. Public verification is throttled, addresses documents by token rather than by a guessable number, and shows masked patient identity only. Uploading the PDF back re-checks its digest, which is what defeats a genuine QR pasted onto a forged report. **PASS**, with the key-management limitation carried forward. --- ## Audit trail findings Append-only and hash-chained, with the chain read and the insert locked together in one transaction so two concurrent writers cannot fork the chain and raise a false alarm. The model refuses update and delete. Secrets are redacted before the row is written. **Verified on live data:** `audit:verify --from=147` reports the chain intact across 3,245 entries. Run from zero it correctly reports the twelve known-broken day-one rows from 2026-08-02, which are documented history and are deliberately not rewritten. The nightly schedule pins the start after that damage, so the morning alarm only ever means something new. That arrangement is correct and was confirmed by running it. **PASS.** Coverage gap for dispensing fixed as above. Audit writes deliberately never break the clinical action that triggered them — a failure is logged loudly instead. That is a defensible trade and it is documented, but it does mean a silent gap is possible if logging itself fails. --- ## Performance findings `Model::preventLazyLoading()` is enabled outside production, so an N+1 introduced anywhere with test coverage fails the suite rather than shipping quietly. That is the right guard and it is why the report's `verifiedBy` lazy-load was caught earlier in this session. The live dataset is too small to profile meaningfully (8 patients, 13 lab orders, 11 invoices, 1,158 drugs, 1,175 lab tests). Index coverage on the listing paths was read and is appropriate: composite indexes on `(branch_id, …)` for every listing filter, a fulltext index on normalised patient names, and a normalised phone index. **PASS on inspection; UNVERIFIED under load.** --- ## UX findings Permission-gated actions fixed as above. Beyond that: destructive actions carry confirmation, empty states are distinguished from over-filtered states, listings filter without a page reload through a documented partial-swap mechanism, and the design system is applied consistently. The one systemic issue found was screens offering what the server would refuse, and it was in one module. --- ## Localisation findings English and Arabic are both maintained; the duplicate `full_name` placeholder was the only key collision the analyser found, and it is fixed in both languages. RTL is handled in the interface. **Arabic PDF output was not re-verified in this pass** and is marked UNVERIFIED rather than assumed. --- ## Communication findings Consent fails closed: no consent row for a channel means no send, and the suppression reason is recorded rather than the message silently vanishing. **PASS.** **Mail is configured by the clinic, in Settings, and the path is verified.** An earlier draft of this report listed "mail is not configured" as the top remaining risk. That was wrong, and the correction is worth stating plainly: this is a per-installation setting with a proper screen behind it, not a gap in the product. `MAIL_MAILER=log` is the shipped default precisely so a fresh install sends nothing to anybody until its owner says where. The wiring was traced end to end rather than assumed: - `UpdateMailRequest` saves host, port, encryption, username, from-address and from-name; `setPassword()` stores the password separately and the form renders a "password is set" state instead of the value, so nobody reads an SMTP password off a settings screen. - `MailSettingsService::apply()` pushes all of it onto the live config. - `SettingsServiceProvider` calls `apply()` on boot, so every request picks the saved settings up. It skips console runs — otherwise `migrate` on a fresh install would query a settings table that does not exist yet — and falls back to the shipped `.env` if anything throws. - A "send test email" action lets the administrator prove it before relying on it. 13 tests in `MailSettingsTest` cover it. **PASS.** What remains genuinely unverified is delivery through a real SMTP server, which needs credentials this audit does not have. --- ## Backup and restore findings Daily local dumps at 02:30 to `C:\laragon\backups\daily`, previously verified. `backups:verify` checks freshness and plausible size. `system:verify` reports the honest warning: *"local backups only — nothing leaves this machine yet."* **Restore has not been rehearsed in this audit.** Doing so requires a destructive operation against a database holding live clinical records, which is not something to do without a scheduled window and the clinic's agreement. Marked UNVERIFIED, and recommended. --- ## Tests added | File | Cases | What it holds | |---|---|---| | `app/Modules/Billing/Tests/Feature/ConcurrentSettlementTest.php` | 6 | Two cashiers, one invoice: payment, refund, reversal, credit note | | `tests/Feature/InactiveAccountGateTest.php` | 4 | A suspended account — Super Admin included — may do nothing | | `tests/Feature/PrivateStorageIsNotServedTest.php` | 2 | No route may be mounted over the PHI root | | `tests/Architecture/AuthorizationCoverageTest.php` | 2 | Every state-changing route decides who may, with a short justified exemption list | | `app/Modules/Laboratory/Tests/Feature/RetractedResultTest.php` | 5 | A withdrawn result can be replaced, and the replacement is what counts | | `app/Modules/Appointments/Tests/Feature/StatusActionsAreGatedTest.php` | 5 | Screens offer only what the viewer may actually do | | `tests/Architecture/DatabaseRefreshTest.php` | 1 | No suite outside the modules touches the database without refreshing it | **25 new cases.** One existing test was corrected rather than weakened: `WorklistScreenTest` asserted a column the clinic had asked to be removed; it now asserts the rule underneath (the row asks for a sample, then stops asking) and additionally checks the accession number is still on the order screen, so removing it from a list cannot quietly become removing it altogether. ### One mistake of mine, worth recording The first full-suite run reported 844 failures, all pointing at a prescriptions screen that was fine. The cause was a test I had just written: `tests/Pest.php` binds `RefreshDatabase` to `app/Modules` and deliberately not to `tests/`, so every suite there that touches the database opts in by hand — all twelve existing ones do, and mine did not. The rows it created outlived it and broke the next module test to create a primary branch. Fixed, and the trap closed with `DatabaseRefreshTest`, which fails by name on any suite that repeats it. The first version of that guard was itself too weak — it matched the surviving `use` import rather than the `uses()` call — and was corrected and verified in both directions. Detail in [CMS-AUDIT-PROGRESS.md](CMS-AUDIT-PROGRESS.md). --- ## Tests passed | Gate | On arrival | Now | |---|---|---| | `pest` (whole suite) | FAIL — 5 errors in `InvoiceCalculatorTest` | **1,405 passed, 0 failed** (4,198 assertions) | | `pint --test` | FAIL — 5 committed files | passed | | `phpstan analyse` (level 6) | FAIL — 9 errors | no errors | | `deptrac analyse` | 0 violations | 0 violations | Verified against the live installation as well: `audit:verify --from=147` reports the chain intact across 3,245 entries; every one of the 11 live invoices foots to the cent; protected routes redirect to sign-in while the two public pages serve; and `GET /storage/keys/document-signing.key` now returns 404. --- ## Remaining risks Ordered by what would hurt most. 1. **Backups do not leave the machine, and nothing alerts on failure.** A disk or host loss takes the clinical record with it. `backups-remote` is configured for any S3-compatible bucket and needs five environment variables. 2. **Restore has never been rehearsed.** A backup that has not been restored is a hypothesis. Needs a scheduled window against a scratch database. 3. **The document signing key has no key-management service.** It sits on disk under `storage/app/private/keys`. Documented as a known limitation; the HTTP route over that directory is now closed, which was the more urgent half. 4. **No release signing key exists.** The first was securely destroyed and its fingerprint blacklisted. The updater fails closed without one — correct behaviour — but no update can be shipped until a new key is generated offline. 5. **Queue has no worker.** Harmless today: nothing in the codebase implements `ShouldQueue` and nothing dispatches a job. It becomes a silent failure the moment something does. `QUEUE_CONNECTION=sync` would make that explicit. 6. **Invoice numbering changed shape mid-year — decided 2026-09-09.** `numbering.invoice` had been set to `INV-{branch}-{YYYY}-{0}` while every other series still used `{00000}`, so the live invoice series ran `…-00007` then `…-8` — one financial document series with two shapes. The clinic chose the short form, and every default now matches it: patient, appointment, encounter, prescription, invoice, payment, credit note, cash session, lab order and specimen all render `{0}`. The padding token still works, so a clinic that wants fixed-width numbers sets a longer run of zeros in Settings. Nothing in the application orders by these columns — checked — so the text-sort caveat applies only outside it, to a spreadsheet someone sorts by hand. Numbers already issued keep the shape they were issued with; they are on printed paper and in signed documents, and rewriting them would invalidate both. --- ## Unverified areas Stated as unverified rather than assumed correct: - Restore from backup (destructive; needs a window). - Actual delivery through a real SMTP server. The path to it *is* verified — see Communication findings — but no live mailbox was sent to. - Off-site backup upload (no credentials). - Invoice, receipt and credit-note PDF rendering (not re-rendered this pass). - Arabic/RTL PDF layout (not re-rendered this pass). - Behaviour under real load and data volume (the live dataset is too small to profile meaningfully). - True multi-process concurrency. The race conditions were reproduced deterministically via stale snapshots, which is the same defect two requests hit, and the fix is a database row lock — but no test in this suite spawns two processes to prove the lock under genuine contention. --- ## Second pass — reviewing this audit's own changes Re-read as a different engineer would: not "did the tests pass" but "what could this break in production that the tests would not notice". **Lock ordering, and whether the billing fix can deadlock.** This is the question that matters most, because the fix introduces a row lock into the busiest path in the system. Traced: | Operation | Order locks are taken | |---|---| | `InvoiceService::issue()` | `sequences[invoice]` → invoice row | | `PaymentService::take/refund/reverse()` | invoice row → `sequences[payment]` | | `CreditNoteService::write()` | invoice row → `sequences[credit_note]` | The first is the reverse of the other two, which is the shape of an ABBA deadlock — but the sequence rows are *different rows*, keyed by document type. A transaction issuing an invoice never waits on `sequences[payment]`, so no cycle can close. Two payments against one invoice take the same order as each other. Verified by reading every call site rather than by reasoning about the pattern. **Where the audit write sits.** All three payment audit entries are written *after* `settleUnderLock` returns, so the invoice lock is not held while contending on the audit-chain tail — which every audited write in the system serialises on. That was preserved deliberately. The dispensing audit entry I added *is* inside the transaction holding the batch locks, which matches what `reverse()` and `StockService::adjust()` already do. Consistent, and safe for the same reason: nothing holds the audit tail and then asks for a batch, so the order is one-way everywhere. **Whether the suspension gate breaks the online booking actor.** `OnlineBookingActor` deliberately creates an **inactive** account to attribute public bookings to, and my change makes inactive accounts fail every permission check. Traced the whole public booking path: `PublicBookingService`, `AppointmentService` and `InvoiceChargeCollector` take the actor for attribution and audit only and never ask a gate — consistent with the architecture's rule that services take the actor explicitly so they can run from a job or a command. Confirmed by `PublicBookingTest` passing in the full run. **Whether the appointment listing's per-row gate check is expensive.** It runs one policy call per offered transition per row. The permission package caches per request and the policy is a method call, so a 50-row day costs roughly 150 in-memory checks and no additional queries. **What I would still want before calling this finished.** A restore rehearsal, and one true multi-process concurrency test against the payment path — the fix is a database row lock, and everything here proves the *guard* is now read under the lock rather than proving the lock itself under real contention. --- ## Production readiness assessment **Fit for continued clinical use, with the operational items above addressed.** What supports that: the structural defects found are fixed and each carries a regression test that failed before the fix; the full suite passes; all three quality gates are green; the audit chain verifies intact on live data; every live invoice foots to the cent; and the architecture that would have to be trusted for the next change is genuinely enforced rather than merely described. What qualifies it: item 1 (mail) and items 2–3 (backups and restore) are operational gaps that no amount of code review closes, and the second of them is the one that would turn an ordinary bad day into an unrecoverable one. This assessment is a considered engineering judgement about the state of this codebase on this date. It is not a certification, it does not establish compliance with any regulatory framework, and it does not claim the system is free of defects — only that these were looked for in these places, by these means, with the results recorded above.