fix(core): serialize runtime NVS diagnostics through stdio

[vincentkoc]

Problem

The normal-runtime nvs_connect_diag and nvs_session_diag records use ROM
output alongside standard application logging. On the pinned ESP-IDF build,
standard Log V1 uses vprintf and USB Serial/JTAG VFS serialization, while ROM
printing takes a separate output path.

A live collector rejected a record with an unknown two-byte prefix. Those
prefix bytes were not retained, so this change does not claim to identify that
specific fragment's source.

Changes

• Emit each runtime record with one printf call, preserving its exact bare
  numeric grammar and existing call position after NVS operations.
• Keep node-state locking, return values, persistence, cleanup, and retry
  behavior unchanged. No mutex, flush, prefix, or parser change is introduced.
• Leave low-level NVS-I/O and initialization diagnostics on ROM output.
• Clarify the output paths and retain their limits in the Tab5 README.

Stacked on #51, base
e166026c7616e1c7eaeea1c023585d6c2748c02e. No SDK, dependency, memory-placement,
or configuration changes.

Validation

• The actual session-loader contention fixture fails on the original source
  at the output-order assertion and passes with this change. A condition
  variable arranges the competing stdout log; no timing sleeps are used.
• All three focused NVS tests pass, including original session error/clear
  behavior, synthetic-canary exclusion, and the unchanged SDK I/O,
  alignment, and concurrent first-failure cases.
• Fresh P2 review found no actionable findings; the scoped diff check passed.
• All five native builds passed in
  CI run 34497105780, attempt 1.
  The component job also executed the focused NVS tests.
• The Tab5 firmware archive was downloaded once and verified against its API
  digest, all 11 internal checksums, 10 manifest file records, and four image
  offsets. Compiled merge 1e909d48f3d7d623ff9ac12f09ad8658248de054
  has parents e166026c7616e1c7eaeea1c023585d6c2748c02e and
  6c23b60bfccf907d6fd64084610269ed7c7ba2e6, with the source head's tree.
• SDK and tracked-patch provenance, generated configuration, and the parsed
  dependency lock match the preceding d9bbea660bcfcd571df1bba81a9d121d594e97e9
  candidate. The SDK remains 362a1776ec212788fda95f75b733bfdde3a0c394
  plus the recorded patches, not pristine ESP-IDF 5.5.5.

Firmware artifact 10160619136, 2,898,606 bytes, SHA256:
4c93ac1345fe76ee9ff576c44581fc819b7e4afce64069e36c7bfe39f0047b49.

The contention test models ROM's independent write path and delegates stdio
output to real vprintf. It is host boundary proof, not execution of physical
USB or ROM firmware. This repair does not guarantee atomic output against
remaining ROM writers. No new firmware flashing, reconnect qualification,
Talk, or provider proof is claimed here.

Landing

Accept the narrow runtime-stdio correction with its deterministic contention
proof and unchanged numeric contract. Land after
#51 using a merge commit and
the exact cumulative tree, preserving the accepted encryption fallback and
earlier repairs. This is not a fix for every observed serial framing failure,
nor new voice or physical USB qualification. No release is authorized.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: blocked before merge. Reviewed September 12, 2026, 12:14 PM ET / 16:14 UTC (Revision 3).

ClawSweeper review

What this changes

Routes runtime nonvolatile-storage diagnostics through standard output and carries stacked Tab5 camera, display-memory, allocation-diagnostic, packaging, CI, and regression-test changes.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

⛔ Blocked before merge - 5 items remain

The runtime diagnostic correction remains useful and is absent from main. The draft blocker is resolved, but the retargeted cumulative branch still lacks an accepted predecessor repair and currently has merge conflicts.

Priority: P2
Reviewed head: 6c23b60bfccf907d6fd64084610269ed7c7ba2e6

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	The focused stdio repair is well supported, but the current cumulative patch retains a concrete display-compatibility defect.
Proof confidence	🌊 off-meta tidepool	Not applicable: The MEMBER-authored PR is exempt from the ordinary contributor proof gate. Its actual-session-loader fixture exercises host vprintf contention, and native CI supports compilation; neither establishes physical USB framing, camera qualification, or encrypted-display operation.
Patch quality	🦐 gold shrimp (3/6)	1 actionable review finding remain.

Verification

Check	Result	Evidence
Real behavior	Not applicable	Not applicable: The MEMBER-authored PR is exempt from the ordinary contributor proof gate. Its actual-session-loader fixture exercises host vprintf contention, and native CI supports compilation; neither establishes physical USB framing, camera qualification, or encrypted-display operation.
Evidence reviewed	8 items	Pinned scope and current landing state: The verified merge-base-to-head delta contains 21 files. GitHub still reports head 6c23b60 against main 6b63951, draft=false, mergeable=false, and mergeable_state=dirty. No current pinned test merge is available. Main still uses ROM output: Current main's session diagnostic helper still calls esp_rom_printf with the original numeric grammar. The proposed helper uses printf; the connection diagnostic makes the same substitution after releasing the node state lock. Bounded contention evidence: The fixture executes the actual session loader, arranges a competing stdout writer with a condition variable, delegates the new output path to real vprintf, and asserts intact ordered records. The captured PR body reports original-failure/patched-pass results and five successful native builds, while explicitly excluding physical USB qualification and attribution of the lost prefix bytes. Tests were inspected, not executed during this read-only review.
Findings	1 actionable finding	[P1] Carry the accepted encrypted-display fallback into the landing tree
Security	None	None.

How this fits together

The ESP node firmware loads saved connection credentials and reports numeric diagnostic records to its console. The Tab5 board adapter also manages display buffers, camera processing, and transport-start diagnostics.

flowchart TD
  A[Saved connection attempts] --> B[Nonvolatile storage operations]
  B --> C[Numeric runtime diagnostics]
  C --> D[Standard output serialization]
  E[Normal application logs] --> D
  D --> F[Device console collector]

Loading

Before merge

• Carry the accepted encrypted-display fallback into the landing tree (P1) - In the cumulative comparison against main, this changes ST7121 buffers to PSRAM unconditionally while retaining software rotation. On flash-encrypted devices, the pinned SDK rejects external SRM buffers, so display rotation fails. The accepted repair in fix(tab5): place ST7121 display buffers in PSRAM #51 already preserves the original allocation flags when encryption is active and supplies both-state tests. Integrate that repair before landing this retargeted stack; the current head does not contain it.
• Resolve merge risk (P2) - Landing the current cumulative display change without the accepted fallback would break rotation for flash-encrypted ST7121 setups.
• Resolve merge risk (P1) - The branch conflicts with current main, so preservation of accepted predecessor repairs and current-main changes remains unverified in a merged tree.
• Resolve merge risk (P1) - The contention fixture establishes host stdout ordering, not physical USB framing or the origin of the collector's discarded prefix.
• Complete next step (P2) - Integrate the repaired predecessor stack, resolve conflicts with current main, and verify the cumulative tree preserves the encryption fallback and earlier accepted repairs before merge.

Findings

• [P1] Carry the accepted encrypted-display fallback into the landing tree — examples/m5stack-tab5-room-node/components/tab5_room_board/tab5_room_board.c:253-254

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Cumulative line growth	Production/tooling +443, tests +1465, docs +111, CI +16 net lines	The 21-file comparison includes documented predecessor work, so its scope substantially exceeds the final stdio correction.

Merge-risk options

Maintainer options:

1. Integrate the repaired predecessor (recommended)
   Land or integrate the accepted predecessor stack, preserve its encrypted-display fallback and tests, and validate the resulting cumulative tree.

Technical review

Best possible solution:

Retain the narrow stdio correction on the repaired cumulative stack, preserving encrypted-display allocation and the documented limits of console serialization.

Do we have a high-confidence way to reproduce the issue?

Yes for the bounded output-ordering mechanism: the actual-loader fixture provides a deterministic contention path, with original-failure/patched-pass results reported in the captured body. The physical collector fragment's origin remains unknown.

Is this the best way to solve the issue?

Yes for replacing runtime ROM output with one printf call per record; the cumulative landing must also incorporate the already accepted encrypted-display repair.

Full review comments:

• [P1] Carry the accepted encrypted-display fallback into the landing tree — examples/m5stack-tab5-room-node/components/tab5_room_board/tab5_room_board.c:253-254
  In the cumulative comparison against main, this changes ST7121 buffers to PSRAM unconditionally while retaining software rotation. On flash-encrypted devices, the pinned SDK rejects external SRM buffers, so display rotation fails. The accepted repair in fix(tab5): place ST7121 display buffers in PSRAM #51 already preserves the original allocation flags when encryption is active and supplies both-state tests. Integrate that repair before landing this retargeted stack; the current head does not contain it.
  Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against 6b63951f0ac5.

Labels

Label changes:

• add merge-risk: 🚨 compatibility: Unconditional PSRAM display buffers conflict with the pinned SDK's encrypted-device rotation contract.
• add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
• add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The MEMBER-authored PR is exempt from the ordinary contributor proof gate. Its actual-session-loader fixture exercises host vprintf contention, and native CI supports compilation; neither establishes physical USB framing, camera qualification, or encrypted-display operation.
• remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.
• remove status: 👀 ready for maintainer look: Current PR status label is status: ⏳ waiting on author.

Label justifications:

• P2: This is a bounded runtime-diagnostic improvement with a specific board-compatibility blocker in its cumulative stack.
• merge-risk: 🚨 compatibility: Unconditional PSRAM display buffers conflict with the pinned SDK's encrypted-device rotation contract.
• rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
• status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The MEMBER-authored PR is exempt from the ordinary contributor proof gate. Its actual-session-loader fixture exercises host vprintf contention, and native CI supports compilation; neither establishes physical USB framing, camera qualification, or encrypted-display operation.

Evidence

What I checked:

• Pinned scope and current landing state: The verified merge-base-to-head delta contains 21 files. GitHub still reports head 6c23b60 against main 6b63951, draft=false, mergeable=false, and mergeable_state=dirty. No current pinned test merge is available. (6c23b60bfccf)
• Main still uses ROM output: Current main's session diagnostic helper still calls esp_rom_printf with the original numeric grammar. The proposed helper uses printf; the connection diagnostic makes the same substitution after releasing the node state lock. (components/esp-openclaw-node/src/esp_openclaw_node_persisted_session.c:60, 6b63951f0ac5)
• Bounded contention evidence: The fixture executes the actual session loader, arranges a competing stdout writer with a condition variable, delegates the new output path to real vprintf, and asserts intact ordered records. The captured PR body reports original-failure/patched-pass results and five successful native builds, while explicitly excluding physical USB qualification and attribution of the lost prefix bytes. Tests were inspected, not executed during this read-only review. (scripts/tests/fixtures/test_nvs_session.c:135, 6c23b60bfccf)
• Introduced display compatibility trigger: The cumulative introduced delta changes ST7121 from DMA allocation to unconditional PSRAM allocation while retaining software rotation. The reviewed head has no runtime encryption predicate around these flags. (examples/m5stack-tab5-room-node/components/tab5_room_board/tab5_room_board.c:254, 6c23b60bfccf)
• Pinned SDK rejects encrypted external rotation buffers: The target directly requires esp_driver_ppa, enables software rotation, and identifies this SDK revision in its Tab5 README. At this exact dependency revision, ppa_check_buffer_alignment returns false for SRM external buffers when flash encryption is enabled; ppa_do_scale_rotate_mirror propagates alignment rejection as ESP_ERR_INVALID_ARG. (components/esp_driver_ppa/src/ppa_core.c:622, 362a1776ec21)
• Accepted repair already exists in the preceding PR: fix(tab5): place ST7121 display buffers in PSRAM #51 remains open. Its repair commit samples esp_flash_encryption_enabled, preserves DMA allocation for encrypted devices, adds bootloader_support, and covers both encryption states. The captured maintainer disposition and this PR's landing section explicitly require preserving that repair. (examples/m5stack-tab5-room-node/components/tab5_room_board/tab5_room_board.c, 0160b153db80)

Likely related people:

• unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)
• steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Preserve the accepted encryption fallback, build dependency, and both-state regressions when integrating the predecessor stack.
• Validate the conflict-resolved cumulative tree against current main with focused regressions and native CI.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)

• reviewed 2026-09-10T15:42:17.365Z sha 6c23b60 :: needs maintainer review before merge. :: none
• reviewed 2026-09-10T15:57:20.622Z sha 6c23b60 :: needs changes before merge. :: none