project-review

project-review — holistic coherence audit of tesla-key-esp32

This project is an ESP-IDF 5.x C++ firmware for the ESP32 family — one source tree builds for esp32 / esp32s3 / esp32c3 / esp32c6 / esp32c5 (the four targets yoziru/tesla-ble supports, plus esp32c5 via prepare-tesla-ble-c5.sh + the conditional main/idf_component.yml). All five receive the repository-owned replay-rejection patch in patches/tesla-ble/ via root CMake. The firmware acts as a BLE↔HTTP proxy for a Tesla vehicle, API-compatible with TeslaBleHttpProxy, so it works as an evcc BLE vehicle. It is small but dense with non-local invariants: a one-line change in code often has to be mirrored in three docs, a Kconfig option, the partition table, and the web UI — and several rules only bite at runtime (BLE wake semantics, heap fragmentation, OTA rollback) where a static read won't catch them.

The goal of a review here is not just "find bugs in this file" but coherence: does the documentation describe the code that exists, does a feature appear everywhere it should, do the config/build/version all agree, and do the runtime invariants still hold.

How to run a review

Work in this order — it's what makes the review catch drift rather than just style:

1. Build the intended model from the docs first. Read .claude/CLAUDE.md (the always-needed essentials: API, command list, NVS table, partition offsets, link-state summary, invariants) and docs/ARCHITECTURE.md (the deep reference CLAUDE.md points to — telemetry fields, MQTT entities, full sleep/link-state + connection-failure semantics, pairing, OTA detail), README.md, docs/README.md, docs/SECURITY.md. CLAUDE.md and docs/ARCHITECTURE.md must agree with each other and with the code — drift between the slim summary and the deep reference is itself a finding. Note every concrete claim: endpoints, commands, NVS keys, partition offsets, flash size, version, defaults. These are your assertions to check.

2. Read the code and find where reality diverges. Walk the components below. For each doc claim, confirm the code matches. For each code feature, confirm the docs cover it. Drift goes both ways — an undocumented endpoint is as much a finding as a documented one that no longer exists.

3. Check config & build agree with both: sdkconfig.defaults, partitions.csv, main/idf_component.yml (library pin), main/Kconfig.projbuild, version.txt, .github/workflows/build.yml.

4. Re-derive the runtime invariants (wake/sleep, heap, OTA, concurrency) from the code — these are listed below and are the easiest things to silently break.

5. Verify before you assert (see Verification discipline). Separate confirmed bugs from hypotheses. Do not over-claim.

6. Audit the review tooling against the project (see Reviewing the skills). The skills (this one and every sibling under .claude/skills/) and the review subagents under .claude/agents/ are part of what drifts — confirm each still maps the project that exists before you trust it, and report any gap as a SKILL-DRIFT finding (correcting it in the same pass).

7. Write the report in the structure at the end.

8. Record the pass in the PR so the gates clear (no file marker). A clean review (no blocking findings) must tick the PR's /project-review checkbox and stamp it with the reviewed commit — the require-project-review.sh PreToolUse hook refuses every PR merge (gh pr merge and mcp__github__merge_pull_request) until that box is ticked and its stamped sha still matches the PR head:

   - [x] `/project-review` clean — merge gate @ <short-sha>    # <short-sha> = git rev-parse --short=12 HEAD
   

   A full review also audits the skills (step 6), and skill-audit ⊂ project-review, so also tick the sibling /skill-audit box in the same body to clear require-skill-audit.sh (which blocks opening a PR and every push to it). Edit the PR body with gh pr edit <pr> --body-file <file> (or the GitHub MCP update tool in web/remote). Any later commit changes the sha and re-stales both boxes, forcing a fresh review before the next merge. Don't tick either if findings block the merge — fix first.

Use parallel reads/Explore to cover the tree quickly, but reason about the cross-cutting links yourself — that's where the value is.

Project map (what to read)

| HA bridge | main/mqtt_ha.{cpp,hpp} | read-only MQTT discovery publish; its own tasks | | Storage | main/nvs_storage.{cpp,hpp} | NVS adapter; maps library keys ≤15 chars | | Diag log | main/diag_log.{cpp,hpp} | in-RAM console ring (GET /diag); static .bss buffer (heap budget!) | | Syslog | main/syslog.{cpp,hpp} | UDP RFC 5424 forwarder for the captured diag lines; server from NVS syslog_uri / CONFIG_TESLA_SYSLOG_SERVER (POST /set_syslog, empty = disabled); hard/transient send-failure split and the per-line RFC 5424 PRI (facility user, severity from each line's own esp_log level) in the host-tested logic/syslog_policy.hpp | | OTA | main/ota_update.{cpp,hpp} | pull-based self-update; dual-slot | | Provisioning | main/provisioning.{cpp,hpp} | captive setup portal when no WiFi | | Web UI | main/www/ (index.html markup + style.css + app.js, spliced by inline_assets.cmake) | compiled into the app binary as ONE self-contained page; live-updates from the /events WebSocket push (~2 s; WS-only, no interval poll — GET /status is retained only for curl/diagnostics + the post-OTA reboot probe) | | On-device indicators | main/display.{cpp,hpp} + main/display_font.h (generated by tools/display_sim.py) + main/led_status.{cpp,hpp} | ST7735 status panel (T-Dongle-C5/S3, CONFIG_TESLA_DISPLAY_ENABLED) + underside APA102 status LED (CONFIG_TESLA_LED_ENABLED, default off). Both are thin renderers — cache-only (never wake the car), no MQTT; the "what to show" decisions live in the host-tested logic/display_model.hpp / logic/led_status.hpp, both reading the shared logic/ui_state.hpp snapshot + logic/soc_gradient.hpp ramp. No-op stubs on boards without the hardware | | Pure logic (host-tested) | main/logic/*.hpp (vin, units, link_state, target, mcp, command_registry, status_model, vehicle_data, command_result, ui_state, display_model, soc_gradient, led_status, syslog_policy, ws_policy, active_window, charge_control, ble_phase, ble_row, ha_templates, http_body, heap_watchdog, connect_outcome) + test/test_logic.cpp | IDF-free logic the device and the host test share (exception: ble_row is a spec header — no firmware TU includes it; its consumer is the browser, held to it by the parity harness): VIN validation, imperial→metric units, the link_state() four-state machine + its /status/MQTT strings, per-target platform/OTA-suffix map, the ONE command registry both /api and /mcp dispatch from (names/kinds/shared arg bounds + tools/list row order) with the MCP JSON-RPC routing/version negotiation, the golden-pinned /status field contract (status_model over the vehicle_data result structs), the shared command result-string mapping, charging-current ACK/readback + active-cache freshness, and the on-device display presenter + status-LED ladder (both off the shared ui_state snapshot + soc_gradient ramp). Host-built + run by scripts/run-mock-tests.sh (CI logic-test gate) — the real local verification

Content truncated.