Second review pass: remediation plan and fixes for F-19…F-44

.claude/agents/firmware-review.md

@@ -36,9 +36,10 @@ exhaustive list.
 - Shared containers read and written from different tasks without a lock (F-07).
 - Accessors that hand out raw pointers to mutex-protected state (F-06). In this repo the
   surviving ones are guarded by `CONFIG_THROTTLE_TESTS` and must stay that way.
-- **Lock ordering: `m_stateMutex` before `lvgl_port_lock`, never the reverse.** Any path
-  that takes them the other way is a deadlock, and it will present as a frozen screen with
-  a train still rolling.
+- **Lock ordering: `lvgl_port_lock` before `m_stateMutex`, never the reverse.** The UI holds
+  the LVGL lock while it reads the controller, so any controller path that calls the UI
+  (`updateUI()`, the power callback) while holding `m_stateMutex` is a deadlock, and it will
+  present as a frozen screen with a train still rolling. (Documented backwards until F-29.)
 
 **LVGL**
 - An LVGL call from a network task, timer, or the encoder task without `lvgl_port_lock`

.github/copilot-instructions.md

@@ -26,8 +26,9 @@ main/
   ├── hardware/       # HAL (encoders, I2C)
   ├── communication/  # ThrottleBackend port + transports (WiThrottle; orchestrator WebSocket)
   ├── model/          # Data (Loco, Throttle, Knob)
-  ├── controller/     # Business logic
-  └── ui/             # LVGL screens; colours from UiTheme.h, never raw hex
+  ├── controller/     # Business logic; SettingsWriter takes the UI's NVS writes
+  ├── ui/             # LVGL screens; colours from UiTheme.h, never raw hex
+  └── utils/          # CallbackSlot for any callback set on one task and called on another
 ```
 
 ### Class Structure
@@ -74,12 +75,16 @@ class MainScreen {
     ThrottleController* m_throttleController;  // Not owned!
 };
 
-// UI can be deleted/recreated without state loss
-delete g_mainScreen;
-g_mainScreen = new MainScreen();
-g_mainScreen->create(..., g_throttleController);
+// Config screens can be deleted/recreated without state loss
+m_settingsScreen = std::make_unique<SettingsScreen>(...);
+m_settingsScreen->create();
 ```
 
+**Exception: `MainScreen` is built once and never destroyed** (F-21) — re-show it with
+`show()`. Destroying it from a Back handler freed it under a task already waiting on the LVGL
+lock to repaint it. And no UI class registers on a client's connection callback: those single
+slots belong to the active backend, so screens poll on an LVGL timer.
+
 ### ⚠️ CRITICAL: LVGL Thread Safety
 **LVGL is NOT thread-safe!** Lock before UI access from network tasks:
 

.github/workflows/ci.yml

@@ -5,26 +5,37 @@ on:
     branches: [main]
   pull_request:
 
+# Read-only: the job checks out, builds and uploads an artifact, and needs no
+# more than that. Without this block the token gets the repository's default,
+# which may include write (F-43).
+permissions:
+  contents: read
+
 jobs:
   build:
     name: firmware build
     runs-on: ubuntu-latest
 
     steps:
-      - uses: actions/checkout@v7
+      # Actions are pinned by commit, not tag: a tag can be moved to other code
+      # after review (F-43). The comment says which release each commit is;
+      # bump both together.
+      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1  # v7.0.1
 
       # ESP-IDF version and target both come from dependencies.lock, which the
       # component manager writes. Keep them in step with it — a mismatch here
       # builds something the bench never runs.
       - name: Build firmware
-        uses: espressif/esp-idf-ci-action@v1
+        # `v1` is a branch in this repository, not a tag; this is its head as of
+        # 2026-09-25, which is what `@v1` was running.
+        uses: espressif/esp-idf-ci-action@9d38657f3d789ca759b2b37aaf5ceffbc42c4f0d  # v1
         with:
           esp_idf_version: v5.5.2
           target: esp32s3
           path: '.'
 
       - name: Upload firmware
-        uses: actions/upload-artifact@v7
+        uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a  # v7.0.1
         with:
           name: esp-layout-controller-firmware
           path: |

CLAUDE.md

@@ -19,8 +19,9 @@ and stop as soon as you know enough:
    diagrams per flow, per-layer reference, protocol wire formats.
 
 `docs/REVIEW_REMEDIATION_PLAN.md` is the record of a completed hardening pass (F-01…F-18,
-all Done). Read a finding when you are about to touch the code it covers — several of the
-"why is it written like this" answers live there and nowhere else.
+all Done); `docs/REVIEW_REMEDIATION_PLAN_2.md` is the second pass (F-19…F-44, in progress,
+with a per-finding status). Read a finding when you are about to touch the code it covers —
+several of the "why is it written like this" answers live there and nowhere else.
 
 **`.github/copilot-instructions.md` covers the same conventions for Copilot.** Where the
 two overlap they must be changed together; where they disagree, this file wins for Claude,
@@ -68,9 +69,11 @@ skipped (speed, direction — the next one will land), `-1` only for critical on
 updates. F-14 removed an infinite lock from a frequently-called path; do not reintroduce
 that shape.
 
-**3. Lock ordering is fixed: `m_stateMutex` before `lvgl_port_lock`, never the reverse.**
-`ThrottleController` guards its own state with a mutex separate from the LVGL port lock.
-Taking them in the other order deadlocks.
+**3. Lock ordering is fixed: `lvgl_port_lock` before `m_stateMutex`, never the reverse.**
+`ThrottleController` guards its own state with a mutex separate from the LVGL port lock. The
+UI takes the LVGL lock and then reads the controller — every event handler, every repaint —
+so the controller must never call out to the UI (`updateUI()`, the power callback) while
+holding its mutex. Documented the other way round until F-29.
 
 **4. State lives at the application layer, never in the UI.** `AppController` owns the
 clients and `ThrottleController`; `ThrottleController` owns the `Throttle` / `Knob` /
@@ -89,10 +92,12 @@ main/
 ├── communication/  ThrottleBackend (port), WiThrottleBackend, OrchestratorBackend,
 │                   WiFiManager, WiThrottleClient (TCP), JmriJsonClient (WebSocket),
 │                   OrchestratorClient (WebSocket control plane)
-├── controller/     AppController, ThrottleController, WiFiController, JmriConnectionController
-└── ui/             SettingsScreen (transport choice, device settings, status),
-                    JmriConfigScreen and OrchestratorConfigScreen (one per
-                    transport), MainScreen, components
+├── controller/     AppController, ThrottleController, WiFiController, JmriConnectionController,
+│                   SettingsWriter (the UI's NVS writes, off the LVGL task)
+├── ui/             SettingsScreen (transport choice, device settings, status),
+│                   JmriConfigScreen and OrchestratorConfigScreen (one per
+│                   transport), MainScreen, components
+└── utils/          CallbackSlot (cross-task callbacks), StackReport (diagnostics)
 ```
 
 Dependency direction is one way: `ui` → `controller` → `communication` / `model`. A model
@@ -177,6 +182,11 @@ Things that look like bugs or oversights and are not. One line each.
 - **`getThrottle` / `getKnob` bypass `m_stateMutex` and are guarded by
   `CONFIG_THROTTLE_TESTS`** — deliberately unsafe test-only accessors (F-06 removed the
   unguarded ones). They must never gain a non-test caller.
+- **`MainScreen` is built once and never destroyed** (F-21). Rebuilding it freed it under a
+  task already waiting on the LVGL lock to repaint it. Constraint 4 is about where state
+  lives, not a licence to rebuild this screen.
+- **No UI class registers on a client's connection callback.** Those are single slots owned by
+  the active backend; config screens poll on an LVGL timer instead (F-21, F-30).
 - **`JmriJsonClient` parses JSON by substring search** (`extractJsonString` /
   `extractJsonInt`) rather than with a parser. Adequate for the narrow JMRI subset it
   reads; **not** adequate for orchestrator payloads. `OrchestratorClient` uses cJSON and
@@ -190,8 +200,9 @@ Things that look like bugs or oversights and are not. One line each.
   (F-18), not an oversight.
 - **`json_port` is normally discovered from the WiThrottle `PW` message**, not configured,
   which is why it has a default and no prominent UI field.
-- **K1/K2 are disabled while WiThrottle is disconnected** (472a955) — a knob that still
-  turned would move a model that no longer tracks anything real.
+- **K1/K2 are disabled while the transport is disconnected** (472a955, F-25) — a knob that
+  still turned would move a model that no longer tracks anything real. Enforced in
+  `ThrottleController`, because the physical encoders never pass through the UI.
 - **`sdkconfig.tests` is tracked despite appearing in `.gitignore`** — it predates the
   ignore rule. `sdkconfig.test.defaults` is the file that actually matters.
 - **`CONFIG_WS_BUFFER_SIZE=16384` is not oversizing.** It sizes the WebSocket *handshake*
@@ -211,3 +222,9 @@ Things that look like bugs or oversights and are not. One line each.
   orchestrator's `locos` table stores no labels, so a WebSocket-connected device would show
   `F0`…`F28`. Being fixed orchestrator-side: labels become operator-authored per loco.
 - **No OTA.** See the flash budget above.
+- **The orchestrator link is cleartext, and whether to keep it so is undecided (F-36).**
+  The login, the 30-day session cookie and the control plane all cross the layout WiFi in
+  the clear. WiThrottle is no better: it has no authentication at all. TLS costs almost no
+  flash, because `esp_websocket_client` already links mbedTLS, but it does cost heap per
+  connection, and the orchestrator would have to serve it. The measured figures and the
+  options are in `docs/REVIEW_REMEDIATION_PLAN_2.md`.

dependencies.lock

@@ -50,6 +50,6 @@ direct_dependencies:
 - espressif/esp_websocket_client
 - idf
 - lvgl/lvgl
-manifest_hash: 58434bdfbf7f1a655631fa5921acdfd8740417f13c01f0b7cb73ef06db9d993a
+manifest_hash: 10a41b0c21730f80db071115f4d17bc807ad458d2d3d58864cffa67d5184d40f
 target: esp32s3
 version: 2.0.0