feat: add CoreS3 duplex Realtime voice and fluid face rendering - #634

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/host/modules/testing/module-structure.architecture.ts-373-374 (1)

373-374: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Test the draw boundary instead of the source expression.

Lines 373-374 require one exact bitwise expression and one helper absence. An equivalent numeric color-packing implementation fails even when drawTexture receives the same packed RGBA value. Assert the numeric argument at the drawing boundary with an XS-driven or instrumented rendering test.

As per path instructions, “Flag tests that read production source as text only to re-assert constants, configuration values, or implementation fragments.”

🤖 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/modules/testing/module-structure.architecture.ts` around lines
373 - 374, Replace the source-text assertions in the emoticon test with an
instrumented or XS-driven rendering test that observes the numeric color
argument passed to drawTexture. Validate the packed RGBA value at the draw
boundary rather than requiring the specific bitwise expression or checking for
colorString absence.

Sources: Coding guidelines, Path instructions

firmware/host/modules/audio/__tests__/audio-duplex-device/manifest.json-3-6 (1)

3-6: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add the ESP-IDF I2C master dependency for amp-readback.

amp-readback.c uses the esp_driver_i2c master APIs, but this manifest only declares esp_driver_i2s. Add an idf dependency on esp_driver_i2c so the ESP32 build links the required I2C master driver.

🤖 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/modules/audio/__tests__/audio-duplex-device/manifest.json`
around lines 3 - 6, Update the manifest’s amp-readback module configuration to
declare the ESP-IDF dependency on esp_driver_i2c alongside its existing
dependencies, ensuring the build links the I2C master APIs used by
amp-readback.c.

firmware/host/modules/conversation/chat-audioio/duplex-chat-audioio.js-288-291 (1)

288-291: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Guard sourcePreRollBytes against rounding to zero.

sourcePreRollBytes rounds preRollBytes from the transport sample rate down to the hardware rate. For small preRollBytes with a high inputSampleRate (for example preRollBytes = 2 at 48000 Hz), the expression rounds to 0. CircularByteHistory then throws RangeError('history capacity must be positive') inside the worker message handler, so the graceful ChatAudioIOBase.FAILED path above is bypassed. Clamp the value to at least one sample frame.

🛡️ Proposed clamp

-    const sourcePreRollBytes =
-      Math.round((preRollBytes * HARDWARE_SAMPLE_RATE) / this.inputSampleRate / PCM_BYTES_PER_SAMPLE) *
-      PCM_BYTES_PER_SAMPLE
+    const sourcePreRollBytes = Math.max(
+      PCM_BYTES_PER_SAMPLE,
+      Math.round((preRollBytes * HARDWARE_SAMPLE_RATE) / this.inputSampleRate / PCM_BYTES_PER_SAMPLE) *
+        PCM_BYTES_PER_SAMPLE,
+    )

🤖 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/modules/conversation/chat-audioio/duplex-chat-audioio.js`
around lines 288 - 291, Update the sourcePreRollBytes calculation in the duplex
audio initialization flow to clamp the rounded value to at least one PCM sample
frame before constructing CircularByteHistory. Preserve the existing sample-rate
conversion and byte alignment, ensuring small preRollBytes values cannot produce
zero capacity and bypass the ChatAudioIOBase.FAILED path.

firmware/host/modules/conversation/chat-audioio/manifest.json-15-23 (1)

15-23: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Document or include TLS roots for all wss:// Realtime endpoints.

providerID accepts any wss:// host, but chat-audioio only bundles ca35, ca109, ca222, ca233, and ca236. If the device can be configured for an OpenAI Realtime wss://api.openai.com/v1/realtime session or another trusted upstream, document the expected root. If it can connect to an arbitrary server-backed endpoint, add the operator-selected roots and explain why.

🤖 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/modules/conversation/chat-audioio/manifest.json` around lines
15 - 23, Update the manifest data roots for chat-audioio to cover every
supported wss:// Realtime endpoint: add the required OpenAI or other trusted
upstream root certificates, or document the expected roots and configuration
when operators select arbitrary server-backed endpoints. Keep the existing CA
entries and align the documentation with the accepted providerID behavior.

firmware/mods/examples/chat_audioio/mod.js-21-24 (1)

21-24: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Align the device smoke timeout with the host runner timeout.

DEFAULT_SMOKE_TIMEOUT_MS is 300000 ms. firmware/scripts/run-chat-smoke.mjs defaults its run timeout to 120000 ms. In a default run the host aborts first, so [ChatSmoke] FAIL reason=timeout never reaches the log and the device never runs its own cleanup path. Lower the device default below the host default, or document the required smokeTimeoutMs value for the runner.

🤖 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/mods/examples/chat_audioio/mod.js` around lines 21 - 24, Update
DEFAULT_SMOKE_TIMEOUT_MS in the chat audio smoke configuration to a value below
the host runner’s 120000 ms default, ensuring the device emits its timeout
failure and executes cleanup before the host aborts.

firmware/mods/examples/chat_audioio/mod.js-569-579 (1)

569-579: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Document or enforce autoStop for smoke runs.

The evaluator in firmware/scripts/lib/chat-smoke-log.mjs (lines 78 and 141) upgrades a PASS to passed only after onStateChanged: disconnected. This code stops the chat only when autoStop is true. If a smoke configuration enables autoStart without autoStop, every run fails with "chat did not disconnect within ... ms after PASS" even though the conversation succeeded. Stop the chat after a recorded PASS in smoke mode, or state the autoStop requirement where the smoke configuration is documented.

🤖 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/mods/examples/chat_audioio/mod.js` around lines 569 - 579, Ensure
smoke runs always disconnect after recording a PASS by updating the
response-complete handling around smokeResultRecorded and stopChat, rather than
depending solely on autoStop. Preserve the delayed AUTO_STOP_DELAY_MS timer
behavior where applicable, and update the related smoke configuration
documentation if autoStop remains a required setting.

firmware/scripts/capture-aec-waveforms.mjs-33-33 (1)

33-33: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Validate the capture timeout.

Number.parseInt returns NaN for a non-numeric STACKCHAN_AEC_CAPTURE_TIMEOUT_MS. setTimeout(fn, NaN) runs on the next tick, so the capture aborts right after the device is flashed and the failure reason reads "NaN ms以内に...". firmware/scripts/run-chat-smoke.mjs validates the same kind of value with positiveInteger. Apply the same validation here.

🐛 Proposed fix

-const timeoutMs = Number.parseInt(process.env.STACKCHAN_AEC_CAPTURE_TIMEOUT_MS ?? '120000', 10)
+const rawTimeoutMs = process.env.STACKCHAN_AEC_CAPTURE_TIMEOUT_MS ?? '120000'
+if (!/^[1-9][0-9]*$/.test(rawTimeoutMs)) {
+  console.error(`STACKCHAN_AEC_CAPTURE_TIMEOUT_MS は正の整数で指定してください: ${rawTimeoutMs}`)
+  process.exit(1)
+}
+const timeoutMs = Number.parseInt(rawTimeoutMs, 10)

🤖 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/capture-aec-waveforms.mjs` at line 33, Validate
STACKCHAN_AEC_CAPTURE_TIMEOUT_MS using the existing positiveInteger pattern from
run-chat-smoke.mjs before assigning timeoutMs. Ensure non-numeric, zero, and
negative values are rejected or replaced with the default so setTimeout always
receives a valid positive integer and the timeout message never contains NaN.

firmware/scripts/run-chat-smoke.mjs-25-29 (1)

25-29: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Resolve the default artifact directory against the firmware directory.

path.resolve applies the process cwd to the relative fallback. If a caller runs the script from the repository root, logs land in <repo>/dist/chat-smoke, outside firmware/dist/, where the firmware ignore rules do not apply. firmware/scripts/capture-aec-waveforms.mjs anchors its default output to firmwareDirectory. Use the same anchor.

🐛 Proposed fix

 const outputRoot = path.resolve(
   readOption(rawArguments, 'output') ??
     process.env.STACKCHAN_CHAT_SMOKE_OUTPUT ??
-    path.join('dist/chat-smoke', timestamp()),
+    path.join(firmwareDirectory, 'dist/chat-smoke', timestamp()),
 )

🤖 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/run-chat-smoke.mjs` around lines 25 - 29, Update the
outputRoot initialization to resolve the default artifact path against the
existing firmwareDirectory, matching capture-aec-waveforms.mjs, while preserving
explicitly supplied output options and environment values.

firmware/host/modules/audio/__tests__/audio-duplex-waveform/README.md-20-20 (1)

20-20: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Remove the account-specific setup command.

Line 20 names a path in one developer account. The documented capture workflow fails in other checkouts. Document a repository-supported setup prerequisite instead of a user home path.

🤖 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/modules/audio/__tests__/audio-duplex-waveform/README.md` at
line 20, Replace the account-specific source command in the
audio-duplex-waveform README capture workflow with the repository-supported
setup prerequisite, using a portable repository-relative instruction that works
across developer environments.

firmware/host/modules/audio/__tests__/audio-duplex-waveform/manifest.json-9-18 (1)

9-18: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Align captureSamples with the requested capture duration.

At 16 kHz, both manifests request longer capture windows than captureSamples allows, and main.js uses maxSamples: CAPTURE_SAMPLES for the diagnostics buffer. Increase the configured values to cover captureMs, or document this as intentional bounded capture.

• firmware/host/modules/audio/__tests__/audio-duplex-waveform/manifest.json: captureSamples = 16384 covers only 1024 ms; request is captureMs = 1200.
• firmware/host/modules/audio/__tests__/audio-duplex-waveform/manifest.doubletalk.json: captureSamples = 131072 covers only 8192 ms; request is captureMs = 8500.

🤖 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/modules/audio/__tests__/audio-duplex-waveform/manifest.json`
around lines 9 - 18, Increase captureSamples in
firmware/host/modules/audio/__tests__/audio-duplex-waveform/manifest.json (lines
9-18) to cover the configured 1200 ms captureMs, and increase it in
firmware/host/modules/audio/__tests__/audio-duplex-waveform/manifest.doubletalk.json
(lines 4-12) to cover the configured 8500 ms captureMs. Keep the diagnostics
buffer in main.js fully sized for each requested capture duration.

Source: Coding guidelines

firmware/scripts/prepare-openai-realtime.mjs-38-42 (1)

38-42: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Reject an option when its value is another option.

readOption() accepts --interactive as the value of a preceding bare --output. The generator then writes the manifest to a file named --interactive instead of stopping with a usage error. Reject missing detached values and detached values that start with --.

Proposed parser change

 function readOption(values, name) {
   const prefix = `--${name}=`
   const index = values.indexOf(`--${name}`)
-  if (index >= 0) return values[index + 1]
-  return values.find((value) => value.startsWith(prefix))?.slice(prefix.length)
+  if (index >= 0) {
+    const value = values[index + 1]
+    if (!value || value.startsWith('--')) {
+      throw new Error(`--${name} requires a value`)
+    }
+    return value
+  }
+
+  const argument = values.find((value) => value.startsWith(prefix))
+  if (!argument) return undefined
+  const value = argument.slice(prefix.length)
+  if (!value) throw new Error(`--${name} requires a value`)
+  return value
 }

🤖 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/prepare-openai-realtime.mjs` around lines 38 - 42, Update
readOption() so bare options require a following value: return no value when the
next argument is missing or starts with "--", while preserving the existing
inline --name=value parsing. Ensure callers treat the rejected result as a usage
error rather than using another option as a filename.

firmware/host/modules/conversation/chat.ts-14-17 (1)

14-17: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Add the missing constructor-option contract test.

The existing test covers endpoint forwarding through providerID, but it does not cover fallback precedence for providerID or endpoint. Add an injected-constructor behavior test that asserts explicit endpoint takes precedence over existing providerID, while existing providerID falls back when endpoint is absent.

🤖 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/modules/conversation/chat.ts` around lines 14 - 17, Extend the
ChatConfig constructor-option tests to cover fallback precedence: verify an
explicit endpoint is forwarded instead of an existing providerID, and verify
providerID is used when endpoint is absent. Use the existing
injected-constructor test setup and preserve the current endpoint-forwarding
assertion.

firmware/host/modules/ui/components/face/parts/dog/mouth.ts-26-41 (1)

26-41: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Derive the curve depth from the mouth height instead of the constant 20.

Lines 27-29 size the mask from maxHeight, but lines 40-41 place the control points at y + 20 regardless of maxHeight. With the defaults the curve fits. If a caller passes a smaller maxHeight, the curve extends past the mask and the native coverage writers discard the out-of-range pixels, so the mouth is silently clipped. Scale the control offset with mouthHeight, or clamp the mask height to the drawn extent.

🤖 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/modules/ui/components/face/parts/dog/mouth.ts` around lines 26
- 41, Update updateMask so the cubic-curve control-point depth is derived from
the current mouthHeight instead of the fixed 20 value, ensuring the drawn mouth
remains within the mask for smaller maxHeight values while preserving the
existing curve shape.

firmware/host/modules/ui/components/face/parts/gray16-mask-raster.c-287-295 (1)

287-295: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Combine the top and bottom coverage additively when both edges fall on one row.

When the eyelid closes, top and bottom converge, so topWhole equals bottomWhole. Line 290 sets that row's coverage to top - topWhole, and line 292 then calls stackchanGray16AddCoverage, which keeps the larger coverage instead of the sum. A fully closed eyelid therefore keeps one partially transparent row across the iris. Sum the two contributions for the shared row.

🐛 Proposed fix

-		if ((topWhole >= 0) && (topWhole < mask.height))
-			stackchanGray16SetCoverage(&mask, x, topWhole, top - topWhole);
-		if ((bottomWhole >= 0) && (bottomWhole < mask.height))
-			stackchanGray16AddCoverage(&mask, x, bottomWhole, 1 - (bottom - bottomWhole));
+		if (topWhole == bottomWhole) {
+			if ((topWhole >= 0) && (topWhole < mask.height))
+				stackchanGray16SetCoverage(
+					&mask, x, topWhole, (top - topWhole) + (1 - (bottom - bottomWhole)));
+		}
+		else {
+			if ((topWhole >= 0) && (topWhole < mask.height))
+				stackchanGray16SetCoverage(&mask, x, topWhole, top - topWhole);
+			if ((bottomWhole >= 0) && (bottomWhole < mask.height))
+				stackchanGray16AddCoverage(&mask, x, bottomWhole, 1 - (bottom - bottomWhole));
+		}

🤖 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/modules/ui/components/face/parts/gray16-mask-raster.c` around
lines 287 - 295, Update the coverage handling around stackchanGray16SetCoverage
and stackchanGray16AddCoverage so that when topWhole equals bottomWhole, the top
and bottom edge contributions are summed on that shared row rather than merged
by the existing max-preserving operation. Preserve the current behavior when the
edges fall on different rows.

firmware/host/modules/ui/components/face/parts/gray16-mask-raster.c-22-29 (1)

22-29: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Compute the buffer-size guard without signed overflow.

Line 27 multiplies mask->strideWidth * mask->height in int. Large dimensions overflow before the cast, so the guard passes and the coverage writers then index past byteLength. The per-pixel checks in stackchanGray16SetCoverage and stackchanGray16AddCoverage test width and height only, so they do not stop the out-of-range offset. Widen the arithmetic and reject values that cannot fit.

🛡️ Proposed fix

 	if (
 		(mask->width <= 0) ||
 		(mask->height <= 0) ||
 		(mask->strideWidth < mask->width) ||
 		(mask->strideWidth & 1) ||
-		(((xsUnsignedValue)(mask->strideWidth * mask->height) >> 1) > mask->byteLength)
+		(mask->strideWidth > 0x7FFF) ||
+		(mask->height > 0x7FFF) ||
+		((((uint32_t)mask->strideWidth * (uint32_t)mask->height) >> 1) > (uint32_t)mask->byteLength)
 	)
 		xsRangeError("invalid Gray16 mask");

🤖 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/modules/ui/components/face/parts/gray16-mask-raster.c` around
lines 22 - 29, Update the buffer-size guard in the mask validation block to
perform the stride-by-height multiplication in a sufficiently wide unsigned type
before shifting or comparing with byteLength, preventing signed overflow. Ensure
dimensions whose required buffer size cannot be represented or exceeds
mask->byteLength are rejected before stackchanGray16SetCoverage and
stackchanGray16AddCoverage can write.

firmware/host/modules/ui/components/face/__tests__/face-rendering/face-rendering.test.ts-479-479 (1)

479-479: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Update the stale assertion message.

The assertion now compares eyelid mask revisions. The message still says "eyelid outlines".

🐛 Proposed fix

-assert(blinkChanged, 'default FaceBehavior should change eyelid outlines for blinking')
+assert(blinkChanged, 'default FaceBehavior should change the eyelid mask for blinking')

🤖 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/modules/ui/components/face/__tests__/face-rendering/face-rendering.test.ts`
at line 479, Update the assertion message in the face-rendering test to refer to
eyelid mask revisions rather than eyelid outlines, while preserving the existing
blinkChanged assertion and its behavior.

firmware/benchmarks/face-rendering/manifest.json-5-9 (1)

5-9: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Remove the unused i2s decoder modules from the face-rendering manifests.

firmware/benchmarks/face-rendering/manifest.json and manifest.visual.json directly include sbc_decoder and dvi_adpcm_decode, but face-rendering sources only measure face/effects rendering and instrumentation counters, and host/modules/ui/manifest.json does not transitively reference them. Remove these entries unless a non-tracked dependency intentionally requires them.

🤖 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/benchmarks/face-rendering/manifest.json` around lines 5 - 9, Remove
the unused $(MODULES)/pins/i2s/sbc_decoder and
$(MODULES)/pins/i2s/dvi_adpcm_decode entries from the "*" dependency list in the
face-rendering manifest, and apply the same change to manifest.visual.json.
Preserve the instrumentation-control entry and verify no non-tracked dependency
requires the decoder modules.

firmware/host/modules/conversation/chat-audioio/openai-realtime-model.js (1)

12-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document or separate the two meanings of barrier index 1.

INPUT_ACK_BARRIER_INDEX and INPUT_RING_HEAD_BARRIER_INDEX are both 1. Line 130 stores message.sequence at index 1, and line 146 reads index 1 as the ring head. Today the two uses cannot overlap: the constructor always requests mode: 'shared-ring' at line 69, and pumpInputAudio calls sendAudio without a sequence, so line 129 skips the store. If a caller ever sends audio with a sequence while shared-ring mode is active, the store overwrites the ring head and pumpInputAudio computes a wrong available and offset.

Add a comment that states the two barrier layouts, or gate the ack store on the active transport mode.

♻️ Proposed clarification

-const INPUT_ACK_BARRIER_INDEX = 1
-const INPUT_RING_HEAD_BARRIER_INDEX = 1
+/*
+ * The barrier layout depends on the input transport mode.
+ * message transport: [reserved, ack sequence]
+ * shared-ring transport: [reserved, ring head, ring tail, gate]
+ * Index 1 therefore carries the ack sequence only in message transport.
+ */
+const INPUT_ACK_BARRIER_INDEX = 1
+const INPUT_RING_HEAD_BARRIER_INDEX = 1

🤖 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/modules/conversation/chat-audioio/openai-realtime-model.js`
around lines 12 - 18, Add an explanatory comment beside INPUT_ACK_BARRIER_INDEX
and INPUT_RING_HEAD_BARRIER_INDEX documenting that index 1 is an acknowledgement
sequence in direct transport but the ring head in shared-ring transport, and
that these layouts must not be mixed. Keep the existing shared-ring behavior
unchanged.

firmware/host/modules/testing/fakes/audio-in.ts (1)

36-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Retain the unread remainder on a partial buffer read.

When the queued chunk is larger than target, the fake copies the prefix and discards the rest. A real driver keeps the remaining bytes for the next read(). With the current fake, a test that queues a chunk larger than the caller's target cannot detect a lost-offset bug in Microphone.record. Push the remainder back to the front of the queue.

♻️ Proposed change

     const source = new Uint8Array(chunk)
     const byteLength = Math.min(source.byteLength, target.byteLength)
     target.set(source.subarray(0, byteLength))
+    if (byteLength < source.byteLength) {
+      chunks.unshift(source.slice(byteLength).buffer)
+    }
     return byteLength

🤖 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/modules/testing/fakes/audio-in.ts` around lines 36 - 47, Update
the read method to preserve partial-read data: when target is a Uint8Array and
byteLength is smaller than source.byteLength, push the unconsumed suffix back to
the front of chunks before returning. Keep the existing prefix copy and return
behavior unchanged, and do not alter numeric-target reads.

firmware/host/modules/conversation/manifest.json (1)

13-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Collapse the identical esp32 platform blocks with manifest include rather than comma-separated keys.

The Moddable manifest platforms object uses one key per platform, so esp32/m5stack_cores3,esp32/m5stackchan_cores3,esp32/stackchan_rt is not valid. Keep the three platform entries only if the intent is to target these specific subplatforms; otherwise, move the shared platform configuration to an included manifest.

🤖 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/modules/conversation/manifest.json` around lines 13 - 34,
Replace the duplicated platform entries in the platforms configuration with a
shared included manifest, using the manifest include mechanism rather than
comma-separated platform keys. Preserve the ChatAudioIO and ChatAudioIOBase
mappings for esp32/m5stack_cores3, esp32/m5stackchan_cores3, and
esp32/stackchan_rt, or retain separate valid entries if platform-specific
targeting is required.

firmware/host/modules/audio/__tests__/audio-duplex-acoustic/main.js (1)

111-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Restore a safe amplifier attenuation before the test ends.

The test arms the AW88298 at −60 dB and never restores a quiet level. The waveform test restores SAFE_HARDWARE_ATTENUATION_DB in stopSafely(). If a later firmware image or a reset starts playback, the device stays loud. Set the safe attenuation after the streams stop, and set it also on the failure path before the throw.

🤖 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/modules/audio/__tests__/audio-duplex-acoustic/main.js` around
lines 111 - 119, Update the duplex test cleanup around the Timer callback and
stopSafely() so the amplifier is set to SAFE_HARDWARE_ATTENUATION_DB after both
streams stop, before diagnostics or close; also apply the same attenuation on
the failure path before throwing. Reuse the existing safe-attenuation constant
and preserve the current shutdown and analysis flow.

firmware/host/modules/audio/__tests__/audio-duplex-waveform/main.js (1)

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

Extract the shared device audio test helpers. Both CoreS3 device tests carry their own copy of the probe generator and the AW88298 register packing. The root cause is a missing shared fixture module for these tests, so the two copies can drift from the production packing in firmware/host/modules/audio/platforms/m5stackchan-cores3/audio-duplex.js.

• firmware/host/modules/audio/__tests__/audio-duplex-waveform/main.js#L60-L83: import buildProbe, amplifierVolumeByte, and amplifierVolumeRegister from a shared helper module, and keep only the local PROBE_PEAK and attenuation arguments here.
• firmware/host/modules/audio/__tests__/audio-duplex-acoustic/main.js#L21-L50: remove the local amplifierVolumeRegister and buildProbe copies and import the same helper module, adding it to this test's manifest.

🤖 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/modules/audio/__tests__/audio-duplex-waveform/main.js` around
lines 60 - 83, Extract the shared audio test helpers into a fixture module based
on the production AW88298 packing in audio-duplex.js. In
firmware/host/modules/audio/__tests__/audio-duplex-waveform/main.js lines 60-83,
remove the local probe generator and import buildProbe, amplifierVolumeByte, and
amplifierVolumeRegister, retaining only local PROBE_PEAK and attenuation
arguments. In
firmware/host/modules/audio/__tests__/audio-duplex-acoustic/main.js lines 21-50,
remove the local amplifierVolumeRegister and buildProbe implementations, import
the shared helpers, and add the helper module to that test’s manifest.

firmware/scripts/capture-aec-waveforms.mjs (1)

130-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Import the shared xsbug decoder.

This function is identical to decodeXsbugLog in firmware/scripts/lib/chat-smoke-log.mjs. Two copies of the entity table will drift. Import the shared helper instead.

♻️ Proposed refactor

+import { decodeXsbugLog } from './lib/chat-smoke-log.mjs'
 import { startXsbugServer } from './lib/xsbug-log-server.js'

-function decodeXsbugLog(log) {
-  return Array.from(log.matchAll(/<log(?:\s[^>]*)?>([\s\S]*?)<\/log>/g), ([, text]) => text)
-    .join('')
-    .replaceAll('&amp;', '&')
-    .replaceAll('&`#10`;', '\n')
-    .replaceAll('&`#13`;', '\r')
-    .replaceAll('&`#34`;', '"')
-    .replaceAll('&`#39`;', "'")
-    .replaceAll('&lt;', '<')
-    .replaceAll('&gt;', '>')
-    .replaceAll('&quot;', '"')
-    .replaceAll('&apos;', "'")
-}

🤖 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/capture-aec-waveforms.mjs` around lines 130 - 142, Remove
the local decodeXsbugLog implementation and import the shared decodeXsbugLog
helper from firmware/scripts/lib/chat-smoke-log.mjs. Update all existing call
sites in capture-aec-waveforms.mjs to use the imported function without changing
decoding behavior.

firmware/host/modules/ui/components/effects/music-notes.ts (1)

85-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Extract the shared sprite-position computation.

onDraw and invalidateSprite both derive progress, rise, and the sprite top from #elapsed. The invalidated rectangle must match the drawn rectangle exactly. If one copy changes later, the note leaves residue on screen because the parent skin repaints only the invalidated region. A single private helper removes that coupling.

♻️ Proposed refactor

+  `#spriteTop`(): number {
+    const progress = this.#elapsed / FADE_DURATION_MS
+    const rise = Math.round(RISE_PIXELS * progress * (2 - progress))
+    return RISE_PIXELS - rise
+  }
+
   onDraw(port: PiuPort, x = 0, y = 0, width = SPRITE_SIZE, height = NOTE_HEIGHT) {
     if (this.#elapsed >= FADE_DURATION_MS) return
-    const progress = this.#elapsed / FADE_DURATION_MS
-    const rise = Math.round(RISE_PIXELS * progress * (2 - progress))
-    const spriteY = RISE_PIXELS - rise
+    const progress = this.#elapsed / FADE_DURATION_MS
+    const spriteY = this.#spriteTop()
     if (x >= SPRITE_SIZE || x + width <= 0 || y >= spriteY + SPRITE_SIZE || y + height <= spriteY) return
@@
   private invalidateSprite(port: PiuPort): void {
     if (this.#elapsed >= FADE_DURATION_MS) return
-    const progress = this.#elapsed / FADE_DURATION_MS
-    const rise = Math.round(RISE_PIXELS * progress * (2 - progress))
-    port.invalidate(0, RISE_PIXELS - rise, SPRITE_SIZE, SPRITE_SIZE)
+    port.invalidate(0, this.#spriteTop(), SPRITE_SIZE, SPRITE_SIZE)
   }

🤖 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/modules/ui/components/effects/music-notes.ts` around lines 85 -
109, Extract the shared progress, rise, and sprite-top calculation from onDraw
and invalidateSprite into a single private helper, then use its returned
position in both methods. Preserve the existing fade-duration early returns and
ensure invalidateSprite uses exactly the same sprite top and dimensions as
onDraw.

firmware/benchmarks/face-rendering/manifest.visual.json (1)

5-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Document why the visual benchmark includes the i2s decoder modules.

The visual benchmark does not reference sbc_decoder, dvi_adpcm_decode, AudioOut, AudioIn, or audio startup behavior in visual.ts or firmware/host/modules/ui/manifest.json. If these modules are required by a shared module/import path for this build target, add a document-level reason next to the entries; otherwise remove them to keep the benchmark build small.

🤖 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/benchmarks/face-rendering/manifest.visual.json` around lines 5 - 8,
Review the module entries in the visual benchmark manifest and either remove
sbc_decoder and dvi_adpcm_decode if they are unused, or retain them with a
document-level explanation of the shared import/build dependency requiring both
modules. Keep the benchmark manifest limited to modules necessary for this build
target.

firmware/host/modules/ui/components/face/parts/gray16-mask.ts (1)

32-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the extra Bitmap constructor argument.

commodetto/Bitmap is defined with constructor arguments width, height, format, buffer, and offset; this call passes this.bytes.byteLength as a sixth argument. Drop it to keep the constructor call aligned with the documented SDK API.

🤖 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/modules/ui/components/face/parts/gray16-mask.ts` around lines
32 - 39, Update the Bitmap constructor call in the gray16 mask initialization to
remove the trailing this.bytes.byteLength argument, retaining width, height,
format, buffer, and byteOffset in the documented five-argument order.

firmware/host/modules/ui/components/face/parts/__tests__/unit-steps.test.ts (1)

6-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add the clamping and non-finite cases.

quantizeUnit returns 0 for NaN and for negative input, and unitFromStep clamps steps outside 0..UNIT_OPEN_STEPS. Those branches drive every mask update, and the current test does not reach them.

♻️ Proposed additional assertions

   assert.equal(unitFromStep(quantizeUnit(0.5)), 0.5)
 })
+
+test('out-of-range and non-finite ratios clamp onto the unit scale', () => {
+  assert.equal(quantizeUnit(Number.NaN), 0)
+  assert.equal(quantizeUnit(-0.5), 0)
+  assert.equal(quantizeUnit(2), UNIT_OPEN_STEPS)
+  assert.equal(unitFromStep(-1), 0)
+  assert.equal(unitFromStep(UNIT_OPEN_STEPS + 1), 1)
+})

🤖 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/modules/ui/components/face/parts/__tests__/unit-steps.test.ts`
around lines 6 - 11, Extend the `animation ratios quantize onto the bounded unit
scale` test to cover `quantizeUnit` returning 0 for `NaN` and negative inputs,
and `unitFromStep` clamping steps below 0 and above `UNIT_OPEN_STEPS` to the
corresponding unit bounds.

firmware/host/modules/ui/state/face-state.architecture.ts (1)

89-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Narrow the allocation matchers so comments do not fail the test.

/\bnew\b/ matches the word new in a comment or string inside the extracted onFaceState block. /\.\.\./ also matches a rest parameter in a nested function. A future comment such as // reuse instead of new objects then fails the test with no regression present. Strip line comments from each block, or match the construction form new <Identifier>( and the object-spread form.

🤖 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/modules/ui/state/face-state.architecture.ts` around lines 89 -
92, Narrow the allocation checks in the block-validation loop around onFaceState
so comments and strings do not trigger failures. Replace the broad new and
spread patterns with matchers for actual constructor expressions and
object-spread syntax, or strip line comments before matching; preserve detection
of allocations while allowing nested rest parameters.

firmware/host/modules/ui/components/face/__tests__/face-rendering/face-rendering.test.ts (1)

132-145: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Document fillOutsideAperture as resetting the mask before drawing.

fillOutsideAperture starts with memset(..., 0xFF, ...), so it clears existing raster output, including fillCircle(4, 4, 2). Add a short API note or use clear()/separate mask semantics in the test so future code does not rely on an inferred reset behavior.

🤖 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/modules/ui/components/face/__tests__/face-rendering/face-rendering.test.ts`
around lines 132 - 145, Document in the test or API-facing definition of
fillOutsideAperture that it resets the mask before drawing, clearing prior
raster output such as fillCircle results. Add a concise note or explicitly
clear/use separate mask state before invoking fillOutsideAperture, without
relying on inferred reset behavior.

Fix all unresolved CodeRabbit comments on this PR:

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

firmware/host/modules/testing/module-structure.architecture.ts (1)

467-478: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace the source-text assertion with a behavior test.

Lines 468 and 472-477 require one exact implementation expression. A valid refactor can change that expression without changing behavior. Test processInputAudio with a distinct producer buffer. Assert that the queued resampled output derives from that buffer.

As per coding guidelines, tests must verify observable behavior or relational invariants and must not re-assert production source literals. As per path instructions, use behavior- or relational-invariant assertions instead of source-text checks.

🤖 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/modules/testing/module-structure.architecture.ts` around lines
467 - 478, Replace the source-text checks in the test for duplex ChatAudioIO
resampling with a behavioral test that invokes processInputAudio using a
producer buffer distinct from the microphone/source buffer. Assert that the
queued resampled output is derived from that producer buffer, preserving the
invariant that each input producer’s own samples are resampled and queued.

Sources: Coding guidelines, Path instructions

Fix all unresolved CodeRabbit comments on this PR:

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

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".

Fix all unresolved CodeRabbit comments on this PR:

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

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".