Improv = REST over serial: push device-model config (incl. eth-only)

[ewowi]

Summary

Reworks how the web installer applies a device-model's catalog defaults — "Improv = REST over serial". The installer now pushes the whole config over the serial port it already owns during provisioning, as REST operations, applied on the device by the same core the HTTP API uses. This works identically on the deployed HTTPS installer and the local preview, and on WiFi and Ethernet-only firmware.

The problem this fixes

The deployed installer (https://moonmodules.org/projectMM/install/) often left a flashed device unconfigured:

• The original push (installer POSTs to /api/control) is blocked on the deployed HTTPS site — a browser won't let an HTTPS page POST to an http:// device (mixed-content).
• The pull/handoff that replaced it (device fetches the catalog when opened with ?deviceModel=) only ran if the user opened that exact link — easy to miss.

The fix

During provisioning the installer holds the USB serial port, so it pushes the config over serial: SET_DEVICE_MODEL then APPLY_OP (one /api/modules or /api/control op per Improv frame). The device routes each op to the same apply-core the HTTP handlers call, so a REST call over the network and an APPLY_OP over serial execute identical code. No HTTP, no mixed-content, no browser handoff — so preview and deployed behave the same. The device-side catalog fetch + the whole ?deviceModel= handoff are deleted.

Eth-only too

The Improv listener was bundled with WiFi (compiled out on MM_NO_WIFI). This decouples the serial transport from WiFi: the listener now runs on every ESP32 target — the WiFi-provisioning RPCs (WIFI_SETTINGS, GET_WIFI_NETWORKS) are #ifdef-guarded out on eth, the vendor RPCs (SET_DEVICE_MODEL, SET_TX_POWER, APPLY_OP) stay, and GET_CURRENT_STATE reports from the Ethernet link. So an eth-only device (e.g. the P4) gets serial config injection too.

What landed

• One apply-core (applyAddModule / applySetControl / applyClearChildren / applyOp) extracted out of the TcpConnection handlers; HTTP handlers are now thin wrappers. HTTP and Improv-serial share it.
• APPLY_OP (0xFC) vendor RPC — one REST op as JSON per frame, chunked + reassembled for a rare long value, single-buffered, acked. Applied on the main loop (never the Improv task).
• eth-only Improv — listener runs on MM_NO_WIFI; hasImprov true on every ESP32 target.
• Installer: pushDefaultsOverSerial on both the WiFi-provision path and the already-online/eth path (an eth device is online at boot, which previously skipped the push — that was the bug). TX-power cap still sent before provisioning.
• Honest success message — reports from defaultsApplied (did the push run?), not the checkbox intent.
• Deletions: device-side handoff (consumePendingDeviceModelParam, catalog fetch), the HTTP fan-out, the Inject button. The device no longer reaches out to Pages.
• Rename: addProvisionedDevice param + "Your devices" state field board → deviceModel (reads old board as a fallback — no migration).
• New test unit_HttpServerModule_apply — host-tests the apply-core directly (no socket), incl. robustness (bad input → typed result, never crash).
• Docs (Improv/System/install README/architecture), the saved plan (docs/history/plans/), the new CLAUDE.md plan-storage rule, and backlog cleanup (the shipped "general data injector" item is APPLY_OP now).

Verification

• Host gates green: spec, desktop build (0 warnings), 423 unit tests (×2, no flake), scenarios, platform boundary, check_devices, check_firmwares, KPI.
• ESP32 builds clean under -Werror: esp32, esp32s3-n16r8, esp32p4-eth (the eth-Improv path).
• Hardware, end-to-end through the real (preview) installer:
  • S3 (WiFi/Improv) — flashed + provisioned, came up fully configured (8×8, AudioSpectrum + RandomMap) via the serial push.
  • P4 (Ethernet-only) — the new capability: flashed + applied the testbench defaults (Grid 8×8, Layer[NetworkReceive], brightness 255, ParlioLed) over serial, no crash. Also verified at the wire level (hand-built APPLY_OP frame → op applied + device acked).

🤖 Generated with Claude Code

---

Commit 2 — Hardening (CodeRabbit fixes + frame/reassembly tests)

A follow-up commit (3a5d3ce) processes the CodeRabbit review of commit 1 and adds the test coverage that makes config-push-over-serial provable without hardware.

CodeRabbit findings — all 8 dispatched: 🐇1 split OpResult::NotFound→ModuleNotFound/ControlNotFound (distinct 404s restored); 🐇2 a failed applyOp now logs over serial + surfaces in provision_status; 🐇3 APPLY_OP pacing 30→120 ms (ack-loop backlogged); 🐇4 the flagged perf "regression" is observed.* telemetry, not a bound (accepted); 🐇5 OpResult::AlreadyExists restores the "already exists" note; 🐇6 documented the serial parent vs HTTP parent_id key difference; 🐇7 NUL-guard confirmed correct (no action); 🐇8 plan date format → ISO YYYYMMDD.

New test coverage:

• src/core/ImprovOpReassembler.h (new) — extracted the APPLY_OP chunk-reassembly + out-of-order/duplicate sequence guard out of the ESP32 handler into a pure, desktop-testable core state machine. test/unit/core/unit_ImprovOpReassembler.cpp (12 cases) pins every path.
• docs/install/improv-frame.js (new) — pure frame builders extracted from the orchestrator so node:test can import them. test/python/test_improv_frame.py + test/js/improv-frame.test.mjs assert the same golden vector so the device C++, Python, and JS frame builders provably agree.
• .github/workflows/test.yml (new) — first PR-level test gate: pytest + node --test.

Also: restored 18 historical plans under the corrected Plan-YYYYMMDD naming; carried the "Improv = REST over serial" lesson into decisions.md.

Reviewer (👾, pre-merge over the whole branch): merge-ready, no blockers — both extractions justified for testability, the OpResult split is clean, the branch is a net deletion with strong new coverage, and it touches no render hot path.

Summary by CodeRabbit

• New Features

  • Added REST-over-serial device configuration delivery during provisioning, eliminating mixed-content HTTPS limitations.
  • Implemented multi-frame operation support for serial provisioning.

• Bug Fixes

  • Resolved web installer defaults application by moving from HTTP to serial-based delivery.

• Documentation

  • Added comprehensive release planning documents for future features.
  • Updated provisioning and Improv protocol documentation.

• Tests

  • Added Python and JavaScript test suites for Improv frame format validation.
  • Introduced HTTP server apply-core operation unit tests.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c00546bd-8554-4f36-88be-fe3ebecee93c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• ✅ Review completed - (🔄 Check again to review again)

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 48.21% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately reflects the main change: shifting from HTTP-based to serial-based (REST over serial) device-model configuration push, with explicit mention of Ethernet-only device support.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch next-iteration

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/platform/esp32/platform_esp32_improv.cpp`:
- Around line 397-461: The improvHandleApplyOp function does not validate that
chunks arrive in the expected sequence order, which can cause data corruption
from retried or out-of-order chunks. Add a field to track the expected next
sequence number in the g_improv structure, then validate that the incoming seq
matches the expected value before processing; if it doesn't match, reset the
reassembly buffer and send an error. Additionally, add braces around the
single-statement if block where opLen is reset when seq equals zero to satisfy
static analysis requirements.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5dfffaaf-24f4-4638-89b8-e8cd76411963

📥 Commits

Reviewing files that changed from the base of the PR and between 0da3cbb and b375d59.

📒 Files selected for processing (26)

• CLAUDE.md
• docs/architecture.md
• docs/backlog/backlog.md
• docs/history/plans/Plan-20262006 - Improv-as-REST: push device-model config over serial.md
• docs/install/README.md
• docs/install/devices.js
• docs/install/index.html
• docs/install/install-orchestrator.js
• docs/moonmodules/core/ImprovProvisioningModule.md
• docs/moonmodules/core/SystemModule.md
• src/core/HttpServerModule.cpp
• src/core/HttpServerModule.h
• src/core/ImprovProvisioningModule.h
• src/main.cpp
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_config.h
• src/platform/esp32/platform_esp32_improv.cpp
• src/platform/platform.h
• src/ui/app.js
• src/ui/style.css
• test/CMakeLists.txt
• test/scenarios/light/scenario_Driver_mutation.json
• test/scenarios/light/scenario_modifier_swap.json
• test/scenarios/light/scenario_perf_full.json
• test/scenarios/light/scenario_perf_light.json
• test/unit/core/unit_HttpServerModule_apply.cpp

[ewowi]

@coderabbitai, can you do a thorough analysis of commit b375d59 and report findings in a prompt for agents

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch commit details
gh api repos/MoonModules/projectMM/commits/b375d59dca72e9364df0a90364f048830ea4ed14 \
  --jq '{sha: .sha, message: .commit.message, author: .commit.author.name, date: .commit.author.date, files: [.files[] | {filename: .filename, status: .status, additions: .additions, deletions: .deletions, patch: .patch}]}'

Length of output: 190315

---

---

@ewowi Here is a thorough analysis of commit b375d59 formatted as a prompt for agents.

---

Agent Prompt: Code-review findings for commit b375d59 — "Improv = REST over serial"

Context

This commit reworks device-model provisioning in projectMM (ESP32 firmware + web installer). The installer previously POSTed device defaults to the device over HTTP, which fails from an HTTPS installer page (mixed-content). The fix: push the whole config over the serial port the installer already owns during provisioning, using a new APPLY_OP (0xFC) Improv vendor RPC. The device routes each op through extracted transport-free apply-core methods shared by the HTTP API. The device-side ?deviceModel= handoff and all HTTP fan-out helpers are deleted. Improv now runs on Ethernet-only builds too.

Verification reported: 423 passing unit tests, clean ESP32 builds, end-to-end hardware test on WiFi (S3) and Ethernet-only (P4).

---

Finding 1 — HTTP API behavior change: unified 404 loses error specificity

File: src/core/HttpServerModule.cpp — handleSetControl

Before: Two distinct 404 responses:

• {"error":"module not found"} when the module name wasn't in the tree
• {"error":"control not found"} when the module existed but the control didn't

After (the extracted core merges both into OpResult::NotFound): A single 404 response:

• {"error":"module or control not found"}

Impact: Any client (MoonDeck, tests, scripts) that distinguished the two 404 cases by message text will silently lose that specificity. The behavior is semantically correct but the error body changed.

Suggested fix (optional): Split OpResult::NotFound into OpResult::ModuleNotFound and OpResult::ControlNotFound in the enum, and have handleSetControl map them to distinct messages. The apply-core can still return the right variant without a TcpConnection.

---

Finding 2 — applyOp result is silently dropped in ImprovProvisioningModule::loop()

File: src/core/ImprovProvisioningModule.h, loop() method

void loop() override {
    if (pendingOpReady_.load(std::memory_order_acquire) && httpServerModule_) {
        httpServerModule_->applyOp(pendingOp_);   // result currently informational; ack already sent
        std::memset(pendingOp_, 0, sizeof(pendingOp_));
        pendingOpReady_.store(false, std::memory_order_release);
    }

The Improv task already sent a success ack to the installer before the op was applied (the ack confirms frame receipt, not application). The OpResult is discarded, so if an op fails (e.g., UnknownType, OutOfRange, NotFound), there is no error signal — the installer believes it succeeded, the device is silently misconfigured.

Suggested fix: Log the OpResult via ESP_LOGW / the status buffer when it is not Ok, so a serial monitor shows which op failed and why. Example:

auto result = httpServerModule_->applyOp(pendingOp_);
if (result != HttpServerModule::OpResult::Ok) {
    ESP_LOGW("ImprovProv", "APPLY_OP failed: op=%s result=%d", pendingOp_, (int)result);
}

---

Finding 3 — No retry on APPLY_OP busy-reject; 30 ms pacing is not ack-based

File: docs/install/install-orchestrator.js, sendApplyOpFrame()

// Small pace so the device's main loop consumes the op (clears its single buffer)
// before the next op's frame arrives — the device refuses a new op while busy.
await new Promise(r => setTimeout(r, 30));

The installer never reads acks back ("Web Serial duplex read while we hold the writer is awkward"). If the device returns error 0x82 (buffer busy), the installer has no way to know and does not retry. The 30 ms pause assumes the device's main loop always consumes an op within 30 ms, which may not hold under a heavy render tick (e.g., 342 µs×1000 iterations worst-case is ~342 ms for a full pass at scale, though each tick is much shorter in practice).

Risk: On a loaded device (large grid, many modules), a burst of APPLY_OP frames during provisioning could silently lose ops.

Suggested fix options (pick one):

1. Increase the inter-op delay conservatively (e.g., 100–150 ms) to match worst-case tick latency seen in scenarios.
2. Implement a read-back ack loop with a short timeout + one retry on error 0x82.

---

Finding 4 — Performance regressions in scenario_perf_full.json (pc-macos upper bounds)

File: test/scenarios/light/scenario_perf_full.json

Step	Old upper tick_us	New upper tick_us	Change
measure-light-64	4	5	+25%
measure-light-128	17	41	+141%
measure-heavy-32	16	59	+269%
measure-heavy-64	71	342	+382%
measure-heavy-128	343	420	+22%

And in scenario_modifier_swap.json: upper tick_us 13 → 20 (+54%).

These are recorded macOS benchmark samples, likely reflecting measurement variance on a loaded CI/dev machine rather than a genuine regression introduced by this commit (the C++ changes touch module management paths, not the render hot path). However the measure-heavy-64 jump (71 → 342 µs, 4.8×) is large enough to warrant confirmation.

Suggested action: Confirm these updated bounds represent a noisy re-run on the same machine rather than a code-induced slowdown. If any benchmark is run on dedicated hardware, re-run measure-heavy-64 in isolation. If it is indeed noise, add a comment in the scenario JSON explaining how upper bounds are updated.

---

Finding 5 — applyAddModule idempotent response body changed (minor)

File: src/core/HttpServerModule.cpp, handleAddModule

Before: Returned {"ok":true,"note":"already exists"} when the module id already existed.
After: Returns {"ok":true} (no note field).

Any client checking for that note to distinguish "created now" vs "already there" will silently stop seeing the distinction. This is unlikely to matter in practice but is a subtle API behavior change. Worth documenting in the API changelog or a comment if MoonDeck relies on that note.

---

Finding 6 — applyOp serial shape uses parent, HTTP uses parent_id

Files: src/core/HttpServerModule.cpp applyOp() vs HTTP handler body

The serial APPLY_OP op for "add" uses {"op":"add","type":"...","id":"...","parent":"..."} (key: parent), while the HTTP /api/modules body uses {"type":"...","id":"...","parent_id":"..."} (key: parent_id). The applyOp dispatcher reads parent, and handleAddModule reads parent_id and passes it to the same applyAddModule(). So the two transports pass through the same core but parse different JSON keys.

This is intentional (documented in comments) and correct, but it means the serial op schema and the HTTP API schema are not interchangeable — a tool that re-uses HTTP payloads as APPLY_OP frames would need to rename parent_id → parent. This is worth calling out explicitly in ImprovProvisioningModule.md or the applyOp header comment to avoid future confusion.

---

Finding 7 — pendingOp_ buffer overflow check is off-by-one (edge case)

File: src/platform/esp32/platform_esp32_improv.cpp, improvHandleApplyOp()

if (g_improv.opLen + chunkLen >= g_improv.opOutLen) {
    // drop
}
std::memcpy(g_improv.opOut + g_improv.opLen, chunk, chunkLen);
g_improv.opLen += chunkLen;
if (last) {
    g_improv.opOut[g_improv.opLen] = 0;  // NUL terminator

The guard >= opOutLen correctly leaves one byte for the NUL. This is fine. ✅ (No action needed — noted for completeness.)

---

Finding 8 — CLAUDE.md date format is non-standard (YYYYDDMM)

File: CLAUDE.md

The naming convention for saved plans is Plan-YYYYDDMM - <title>.md with an explicit note that it is day-before-month (DDMM not MMDD), e.g. Plan-20262006 for 2026-06-20. This is intentional but unusual and easy to get wrong. The saved plan Plan-20262006 - Improv-as-REST: push device-model config over serial.md is correctly named.

Suggested clarification: Add an example or note to CLAUDE.md that explicitly warns "DDMM not MMDD" at the point of the rule, not just in the example, to prevent future agents from using the more common ISO YYYYMMDD order.

---

Summary table

#	Severity	File(s)	Issue
1	Medium	HttpServerModule.cpp	Merged 404 loses module-vs-control error specificity
2	Medium	ImprovProvisioningModule.h	applyOp result dropped — silent failures on bad ops
3	Low–Medium	install-orchestrator.js	No retry on busy-reject; 30 ms pacing not ack-based
4	Low	scenario_perf_full.json	Large upper-bound jumps (4.8×) need confirmation
5	Low	HttpServerModule.cpp	already exists note removed from add response
6	Low (doc)	HttpServerModule.cpp / .md	parent vs parent_id key discrepancy not documented in spec
7	None	platform_esp32_improv.cpp	Off-by-one guard is correct ✅
8	Low (doc)	CLAUDE.md	YYYYDDMM date convention easy to misread

[coderabbitai]

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/platform/esp32/platform_esp32_improv.cpp (1)

434-445: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Reject non-binary last flag values before reassembly.

Line 435 is parsed as uint8_t, and Line 444 implicitly treats any non-zero as true. That accepts malformed frames and can finalize partial payloads early.

Suggested fix

     uint8_t seq = payload[1];
     uint8_t last = payload[2];
+    if (last > 1) {
+        improvSendError(static_cast<improv::Error>(IMPROV_ERROR_INVALID_OP));
+        return;
+    }
     const uint8_t* chunk = payload + 3;
     size_t chunkLen = static_cast<size_t>(len) - 3;
@@
-    switch (reasm.feed(seq, last, chunk, chunkLen)) {
+    switch (reasm.feed(seq, last == 1, chunk, chunkLen)) {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/platform/esp32/platform_esp32_improv.cpp` around lines 434 - 445, The
`last` variable is parsed as `uint8_t` from the payload but should only contain
binary values (0 or 1) since it represents a flag. Add validation after line 435
where `last` is extracted from `payload[2]` to check that it is either 0 or 1,
and return an error or reject the frame if it contains any other value before
passing it to the `reasm.feed()` method on line 444.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/test.yml:
- Around line 14-29: The path filters in the .github/workflows/test.yml workflow
are missing entries for firmware-side frame contract files, allowing changes to
device C++ frame and dispatch code to skip CI checks. Add path patterns for the
C++ firmware/device frame contract files to both the pull_request and push
sections' paths filters, ensuring that any modifications to the device-side wire
protocol definitions trigger the test workflow and prevent cross-language
contract drift from bypassing validation.
- Around line 38-41: Replace the version tag references with specific commit
SHAs to mitigate supply-chain risks. For the actions/checkout action, replace
`@v4` with a specific commit SHA. For the astral-sh/setup-uv action, replace `@v3`
with a specific commit SHA. You can find the correct commit SHAs by visiting the
respective action repositories on GitHub and selecting a release or specific
commit. This ensures your workflows use immutable commits instead of moving tags
that could be compromised.

In `@docs/history/plans/Plan-20260522` - Stream -api-state JSON (fix fixed-buffer
overflow).md:
- Around line 1-16: Add a new "Lessons" section to the plan document after the
verification section to capture key insights about buffer management decisions.
Document when streaming JSON responses is justified versus when fixed-size
buffers are sufficient, with a concrete example such as "Once a JSON response
exceeds 4 KB, streaming is simpler than guessing a larger static size." This
section should serve as a reference for future buffer-management decisions in
the codebase.

In `@docs/history/plans/Plan-20260522` - UI rewrite to ui-spec.md baseline (item
12).md:
- Around line 1-252: Plan-11 lacks a Lessons or Learnings section to document
non-obvious insights for future UI work. Add a new section between the "Risks
and mitigations" section and the "Verification" section titled "Lessons learned"
or "Insights for future work" (~10-20 lines) that captures key learnings such as
the complexity of localStorage key migration patterns, why design decisions like
one-root-visible were chosen, non-obvious patterns around WS state-push conflict
avoidance, and why preserving existing patterns like the dragTs cooldown despite
a full rewrite is important. This section mirrors the retrospective lens used in
Plan-09 and ensures archival completeness for future reference.

In `@docs/install/improv-frame.js`:
- Around line 43-50: The `buildImprovFrame` function assigns the payload length
directly to `frame[8]` without validating that it fits in a single byte. Add a
guard clause at the beginning of `buildImprovFrame` to check if the payload
length exceeds 255, and throw an error with a descriptive message if it does.
This prevents truncation of the length value and ensures only valid wire frames
are created.

---

Outside diff comments:
In `@src/platform/esp32/platform_esp32_improv.cpp`:
- Around line 434-445: The `last` variable is parsed as `uint8_t` from the
payload but should only contain binary values (0 or 1) since it represents a
flag. Add validation after line 435 where `last` is extracted from `payload[2]`
to check that it is either 0 or 1, and return an error or reject the frame if it
contains any other value before passing it to the `reasm.feed()` method on line
444.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 51114825-68f5-46cd-8a4a-1a620e1d8b96

📥 Commits

Reviewing files that changed from the base of the PR and between b375d59 and 3a5d3ce.

📒 Files selected for processing (38)

• .github/workflows/test.yml
• CLAUDE.md
• docs/backlog/backlog.md
• docs/history/plans/Plan-20260519 - Core Pipeline on Desktop — Lights on Panel via ArtNet.md
• docs/history/plans/Plan-20260519 - ESP32 Deployment.md
• docs/history/plans/Plan-20260519 - HTTP Server + WebSocket + Web UI (Items 5+6a).md
• docs/history/plans/Plan-20260519 - Noise Effect + Mirror Modifier + MappingLUT.md
• docs/history/plans/Plan-20260520 - 3D WebGL Preview (Item 5b).md
• docs/history/plans/Plan-20260520 - Adaptive Memory Allocation & Memory Scenario Testing.md
• docs/history/plans/Plan-20260520 - Live Scenario Testing (Item 8).md
• docs/history/plans/Plan-20260520 - SystemModule + NetworkModule (Items 9+10).md
• docs/history/plans/Plan-20260521 - Control-list-driven JSON persistence (item 11).md
• docs/history/plans/Plan-20260521 - FilesystemModule + flash partition scheme (attempted, abandoned).md
• docs/history/plans/Plan-20260522 - Nest child module cards inside their parent card's box.md
• docs/history/plans/Plan-20260522 - Replace a module with another type.md
• docs/history/plans/Plan-20260522 - Side-nav (hamburger + footer), MoonLight logo + favicon, repo URL rename.md
• docs/history/plans/Plan-20260522 - Stream -api-state JSON (fix fixed-buffer overflow).md
• docs/history/plans/Plan-20260522 - UI rewrite to ui-spec.md baseline (item 12).md
• docs/history/plans/Plan-20260523 - Top-level shape change to Layouts, Layers, Drivers.md
• docs/history/plans/Plan-20260524 - Release 1.0 distribution: web installer + GitHub Releases.md
• docs/history/plans/Plan-20260525 - Release-channel picker + first-boot WiFi provisioning.md
• docs/history/plans/Plan-20260620 - Improv-as-REST: push device-model config over serial.md
• docs/history/plans/Plan-20260621 - Improv frame-contract unit tests (pytest + node-test).md
• docs/install/improv-frame.js
• docs/install/install-orchestrator.js
• docs/moonmodules/core/ImprovProvisioningModule.md
• docs/testing.md
• src/core/HttpServerModule.cpp
• src/core/HttpServerModule.h
• src/core/ImprovOpReassembler.h
• src/core/ImprovProvisioningModule.h
• src/platform/esp32/platform_esp32_improv.cpp
• test/CMakeLists.txt
• test/js/improv-frame.test.mjs
• test/python/test_improv_frame.py
• test/scenarios/light/scenario_perf_full.json
• test/unit/core/unit_HttpServerModule_apply.cpp
• test/unit/core/unit_ImprovOpReassembler.cpp

💤 Files with no reviewable changes (1)

• docs/history/plans/Plan-20260620 - Improv-as-REST: push device-model config over serial.md

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n .github/workflows/test.yml | sed -n '30,60p'

Repository: MoonModules/projectMM

Length of output: 1153

---

Pin GitHub Actions to commit SHAs to mitigate supply-chain risks.

Using moving tags like @v4 and @v3 exposes your workflows to potential compromises of upstream action repositories. Pin each action to a specific commit SHA instead.

This applies to:

• Line 38: actions/checkout@v4
• Line 41: astral-sh/setup-uv@v3
• Line 51: actions/checkout@v4

🧰 Tools

🪛 zizmor (1.25.2)

[error] 38-38: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)

(unpinned-uses)

---

[error] 41-41: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)

(unpinned-uses)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/test.yml around lines 38 - 41, Replace the version tag
references with specific commit SHAs to mitigate supply-chain risks. For the
actions/checkout action, replace `@v4` with a specific commit SHA. For the
astral-sh/setup-uv action, replace `@v3` with a specific commit SHA. You can find
the correct commit SHAs by visiting the respective action repositories on GitHub
and selecting a release or specific commit. This ensures your workflows use
immutable commits instead of moving tags that could be compromised.

Source: Linters/SAST tools

[coderabbitai]

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Plan-15 captures a critical bug fix and design shift.

The root cause is clear (line 7: fixed 4 KB buffer overflows with malformed JSON), and the solution (streaming JsonSink with socket/buffer modes, lines 20–26) is well-motivated. The verification (lines 31–34) confirms the fix works at scale (7 KB valid JSON).

Optional enhancement: Consider adding a brief "Lessons" section to capture insights about when streaming is justified vs. when fixed buffers suffice. For example: "Once a JSON response exceeds 4 KB, streaming is simpler than guessing a larger static size." This would help future buffer-management decisions. (Not required given the fix's success.)

Also applies to: 35-41

🧰 Tools

🪛 LanguageTool

[style] ~9-~9: Consider an alternative for the overused word “exactly”.
Context: ...char json[4096]` stack buffer. This is exactly the failure the plan-12 spec note predi...

(EXACTLY_PRECISELY)

---

[style] ~16-~16: ‘in a heap’ might be wordy. Consider a shorter alternative.
Context: ...ate`. - Buffer mode — bytes collect in a heap buffer that doubles on demand. Used by ...

(EN_WORDINESS_PREMIUM_IN_A_HEAP)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/history/plans/Plan-20260522` - Stream -api-state JSON (fix fixed-buffer
overflow).md around lines 1 - 16, Add a new "Lessons" section to the plan
document after the verification section to capture key insights about buffer
management decisions. Document when streaming JSON responses is justified versus
when fixed-size buffers are sufficient, with a concrete example such as "Once a
JSON response exceeds 4 KB, streaming is simpler than guessing a larger static
size." This section should serve as a reference for future buffer-management
decisions in the codebase.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff

Plan-11 is comprehensive but lacks a Lessons section.

This is the largest plan in the batch (252 lines, covering major UI rewrite). The structure is strong: scope, decisions locked, engine additions, UI rewrite, tests, docs, verification. The risk-mitigations section (lines 215–224) is valuable.

Suggestion: Add a "Lessons" or "Learnings" section at the end (before or after verification, ~10–20 lines) to capture non-obvious insights, such as:

• The complexity of localStorage migration across keys
• Why one-root-visible was preferred to all-roots (user expectations, not technical constraint)
• Insights about WS state-push pushback handling
• Why dragTs cooldown persists despite full rewrite (the pattern worked, keep it)

Plan-09 does this excellently (lines 54–88); Plan-11 would benefit from the same retrospective lens so future UI work learns from gotchas encountered. Not blocking, but recommended for archival completeness.

🧰 Tools

🪛 LanguageTool

[style] ~109-~109: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...o allow drop, add .drag-over class. - On drop: compute delta from indices (sou...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/history/plans/Plan-20260522` - UI rewrite to ui-spec.md baseline (item
12).md around lines 1 - 252, Plan-11 lacks a Lessons or Learnings section to
document non-obvious insights for future UI work. Add a new section between the
"Risks and mitigations" section and the "Verification" section titled "Lessons
learned" or "Insights for future work" (~10-20 lines) that captures key
learnings such as the complexity of localStorage key migration patterns, why
design decisions like one-root-visible were chosen, non-obvious patterns around
WS state-push conflict avoidance, and why preserving existing patterns like the
dragTs cooldown despite a full rewrite is important. This section mirrors the
retrospective lens used in Plan-09 and ensures archival completeness for future
reference.