feat(firmware): add manifest preflight check (check:manifest) - #516

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

firmware/scripts/check-manifest.js (2)

56-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Early exit on spawn error skips remaining targets without a summary.

If mcconfig can't be spawned for one target (e.g., PATH not yet set up), the script exits immediately instead of recording it as a failure and continuing to check other targets, unlike the warning/exit-status path below which aggregates failures across all targets.

♻️ Optional: aggregate this failure like the others

-  if (result.error) {
-    console.error(`mcconfig could not be started: ${result.error.message}`)
-    console.error('Set up the Moddable SDK first, e.g. source "$HOME/.local/share/xs-dev-export.sh" or npm run setup.')
-    process.exit(1)
-  }
+  if (result.error) {
+    console.error('Set up the Moddable SDK first, e.g. source "$HOME/.local/share/xs-dev-export.sh" or npm run setup.')
+    failures.push(`mcconfig could not be started: ${result.error.message}`)
+    return failures
+  }

🤖 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 `@firmware/scripts/check-manifest.js` around lines 56 - 60, The spawn-error
path in check-manifest.js exits immediately inside the mcconfig check, which
prevents the script from continuing through the remaining targets and producing
the usual failure summary. Update the logic around the mcconfig spawn result to
treat result.error as a per-target failure: record it in the same aggregated
failure tracking used by the warning/exit-status path, keep iterating over the
other targets, and only exit after the loop with the collected summary. Use the
existing mcconfig checks and the failure aggregation flow in check-manifest.js
to keep behavior consistent.

---

11-18: 🧹 Nitpick | 🔵 Trivial

Six full debug builds (including ESP32 hardware targets) run sequentially in CI.

Each target does a real mcconfig -d ... -t build, which for ESP32 platforms means invoking the ESP-IDF toolchain. Sequential execution across 6 targets adds a non-trivial amount of wall-clock time to every PR before the heavier build matrix even starts. Consider whether these could run in parallel (e.g., Promise.all with worker processes) or whether build artifact caching is desirable to keep this preflight check "lightweight" as intended.

Also applies to: 84-99

🤖 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 `@firmware/scripts/check-manifest.js` around lines 11 - 18, The preflight
manifest check in check-manifest.js is running all six mcconfig -d builds
sequentially, which makes CI slower than intended. Update the target loop to run
builds concurrently where safe, using the targets array and the existing build
invocation logic to launch independent worker processes in parallel (for example
via Promise.all), or otherwise add caching to avoid repeated ESP32 toolchain
work. Keep the same target definitions but change the execution flow so the
builds no longer block one another.

firmware/host/app/runtime-audio.ts (1)

86-91: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Consider guarding against a throwing microphone.stop().

If #microphone?.stop() throws, the subsequent TTS handler resets (onPlayed/onDone → noop) are skipped, leaving stale handlers attached after a failed close. Since close() is a shutdown path, wrapping the microphone stop in try/finally (or reordering so TTS detachment always runs) would make cleanup best-effort rather than all-or-nothing.

♻️ Suggested defensive ordering

   close(): void {
-    this.#microphone?.stop()
-    this.#tts.onPlayed = noop
-    this.#tts.onDone = noop
+    try {
+      this.#microphone?.stop()
+    } finally {
+      this.#tts.onPlayed = noop
+      this.#tts.onDone = noop
+    }
   }

🤖 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 `@firmware/host/app/runtime-audio.ts` around lines 86 - 91, The close() cleanup
in RuntimeAudio can leave stale TTS handlers if microphone.stop() throws before
the handler resets run. Update the RuntimeAudio.close method to make the TTS
detachment (`this.#tts.onPlayed` and `this.#tts.onDone`) happen reliably even
when `#microphone?.stop()` fails, for example by using try/finally or reordering
the cleanup so the noop assignments always execute.

firmware/host/app/runtime-lighting.ts (1)

45-50: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

One failing LED aborts the rest of shutdown cleanup.

If any led.off() throws, the for...of loop stops and remaining LEDs are left in their prior state. For a shutdown path, consider making this best-effort per LED.

♻️ Suggested fix

   close(): void {
     for (const led of Object.values(this.#led)) {
-      led.off()
+      try {
+        led.off()
+      } catch {
+        // best-effort: continue turning off remaining LEDs
+      }
     }
   }

🤖 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 `@firmware/host/app/runtime-lighting.ts` around lines 45 - 50, The shutdown
cleanup in RuntimeLighting.close() should be best-effort per LED because a
single failing led.off() currently aborts the loop and leaves other LEDs
unchanged. Update the close() method to handle errors around each led.off() call
independently so every entry in this.#led is attempted, and preserve the
existing shutdown flow even if one LED fails. Use the RuntimeLighting.close()
and this.#led iteration as the main places to fix.

Fix all unresolved CodeRabbit comments on this PR:

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

Fix all unresolved CodeRabbit comments on this PR:

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