fix(node): validate JSON sends and trace invoke replies

[vincentkoc]

Problem

The shared JSON sender treats every nonnegative SDK return as success. The
pinned WebSocket client can return zero when a nonempty write fails, so the
existing invoke warning and Gateway-request failure callback can be suppressed.
A short return must not count as a complete JSON send either.

Separately, a bounded camera trial reached the existing pipeline release
marker but delivered no invoke result before the Gateway timeout. That marker
precedes handler payload construction and does not establish handler return
or transmission. This change does not claim the zero-return defect caused
that trial's timeout.

Stacked on #53.

Changes

• Require a positive SDK return equal to the complete serialized JSON length.
  Negative, zero, and short results use existing failure and cleanup paths,
  including non-invoke Gateway requests.
• Add private, stack-local diagnostics only for validated exact camera.snap
  and device.info invocations. At most nine fixed numeric records cover
  handler return, envelope construction, serialization, SDK send, and local
  completion after frees.
• Preserve RPC envelopes, ownership, frees, SDK timeouts, allocation policy,
  drivers, retries, and public configuration. INFO follows existing logging
  controls; no global verbosity or WebRTC logging change.
• Document field availability and limits. No IDs, payloads, dynamic command
  names, session material, remote error text, or per-fragment tracing.
• Execute the focused host fixture in the existing component CI job after
  its cJSON source is configured.

Validation

• The original source compiles with the fixture, then fails exactly the zero-
  and short-send error-callback assertions; negative and full returns pass.
• Six focused methods pass with ASan/UBSan and -Wall -Wextra -Werror.
  The fixture executes the actual protocol functions with real cJSON and
  bounded transport/handler stubs, covering full/negative/zero/short sends,
  unchanged envelopes, serialization allocation failure, blocked-send
  marker ordering, exact cleanup, quiet logging, and synthetic secret
  exclusion. No production test exports.
• Actionlint and staged git diff --check pass.
• Fresh P2 autoreview and independent source review found no actionable
  P0-P2 issues in the frozen five-file change.
• All five native jobs passed in
  CI run 34537672287,
  attempt 1, for head 8e824973093329e5efe6034a6bc15e41f7146e52.
  The component job also executes the focused reply fixture.
• The Tab5 artifact was downloaded and verified against GitHub's SHA256
  78db4028765f59628c95a0ddd6cb9cd862a8b7d2e871d0388d19926528cc15f2.
  It compiled synthetic merge
  5ec58db4c226a0b7cf1b078fc7a452444fd9099e (app version 5ec58db);
  the merge parents and tree match the intended base and head.
• All 11 internal checksums, 10 manifest file entries, and four flash-image
  mappings verified. Generated configuration, parsed dependency lock,
  SDK/component patches, and protected partition ranges match the preceding
  verified PR53 artifact. The SDK build remains
  v5.5.5-648-g362a1776ec2-dirty with the existing tracked patches, not a
  pristine release tag.

Limits

SDK acceptance and local completion are not Gateway acknowledgement or
end-to-end delivery. Envelope construction returning does not certify every
child allocation; alloc_failed records an absent serialized buffer, not
an independently localized heap fault. The existing timeout is not an
aggregate RPC deadline.

The verified 5ec58db candidate was subsequently flashed with four writes and
independent readbacks passing. One camera trial recorded an SDK return of zero
for a 69,546-byte serialized response; the diagnostic correctly reported a
failed send. A later keyless trial on the same image accepted all 82,374 bytes
in 6,718 ms, and the Gateway returned a decoded 630x1120 JPEG of 61,562 bytes.
The larger successful response rules out a universal 64 KiB send ceiling; it
does not explain or repair the earlier intermittent zero return.

Recognizable camera output was observed, but color remained uncalibrated.
There is no successful voice qualification or established cause for the
earlier timeout/disconnect. No SDK or dependency change is part of this repair.

Maintainer Disposition

Accept the complete-length send contract and bounded diagnostics independently
of unresolved transport intermittency and voice qualification. Land after
#53 with a merge commit and
the verified cumulative tree, preserving signed history and predecessor repairs.
The physical observations identify their historical image, not a new live test
of the cumulative landing tree. 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:16 PM ET / 16:16 UTC (Revision 3).

ClawSweeper review

What this changes

The branch validates complete JSON sends, traces camera and device-information replies, and carries preceding Tab5 camera, allocation-diagnostic, and firmware-packaging repairs.

Merge readiness

⛔ Blocked before merge - 3 items remain

The correction remains necessary on main, and no concrete introduced defect was found. New device observations address the previous hardware-proof concern; this maintainer-authored stack remains open.

Priority: P2
Reviewed head: 8e824973093329e5efe6034a6bc15e41f7146e52

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	A sound repair with focused regression coverage and relevant physical observations; stack integration remains a separate landing prerequisite.
Proof confidence	🐚 platinum hermit (4/6)	Sufficient (live_output): The supplied body records the real Tab5 camera-to-Gateway path on historical firmware 5ec58db: a zero return was classified as failure, followed by full acceptance of 82,374 bytes and a decoded JPEG. This supports the changed protocol behavior and resolves the earlier hardware-proof concern; it does not qualify a newly integrated landing tree.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (live_output): The supplied body records the real Tab5 camera-to-Gateway path on historical firmware 5ec58db: a zero return was classified as failure, followed by full acceptance of 82,374 bytes and a decoded JPEG. This supports the changed protocol behavior and resolves the earlier hardware-proof concern; it does not qualify a newly integrated landing tree.
Evidence reviewed	11 items	Main still contains the reported defect: The default-branch JSON sender still returns written >= 0, accepting zero and short writes. The reviewed branch instead requires a positive return equal to the serialized length. Affirmative WebSocket dependency boundary: The Tab5 manifest selects third_party/esp-protocols for its WebSocket client; .gitmodules identifies espressif/esp-protocols, and the gitlink pins b7f3de500a4fd4b58e4248093067058959833260. SDK confirms zero-return failure: The pinned send implementation treats zero for a nonempty transport write as failure and returns that zero unchanged; successful completion returns the accumulated byte count.
Findings	None	None.
Security	None	None.

How this fits together

The ESP node receives Gateway commands, runs device handlers, and sends JSON results over WebSocket. The stacked Tab5 changes also affect camera capture, image conversion, and firmware build verification.

flowchart TD
  A[Gateway command] --> B[Device handler]
  B --> C[JSON result envelope]
  C --> D[WebSocket send]
  D --> E{Complete positive byte count}
  E -->|Yes| F[Local send success]
  E -->|No| G[Existing failure handling]
  B --> H[Bounded diagnostic records]
  D --> H

Loading

Before merge

• Resolve merge risk (P1) - The current stack conflicts with main, and no pinned current test-merge result is available to establish the final combined behavior.
• Resolve merge risk (P2) - The cumulative camera timeout intentionally makes cameras needing more than two seconds per frame return unavailable; the predecessor disposition accepts this tradeoff, but fresh-device and upgrade latency qualification remains unestablished.
• Complete next step (P2) - Land fix(tab5): bound camera dequeue waits #53 first, resolve this branch's conflicts while preserving signed history and accepted predecessor repairs, then verify the cumulative merge tree and native checks before the requested merge commit.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Cumulative stack scope	26 files across 10 commits	The pinned introduction delta includes predecessor repairs beyond the final five-file send-validation change.
Production and test growth	Production/build/patch +622/-41 lines; tests +2180/-15 lines	Production growth is explained by bounded diagnostics and guarded camera compatibility work, with substantial regression coverage.

Merge-risk options

Maintainer options:

1. Preserve the accepted camera policy (recommended)
   Carry the predecessor's explicit two-second timeout acceptance through stack integration and retain hardware upgrade qualification as a separate release gate.

Technical review

Best possible solution:

Land complete-send validation with the accepted predecessor repairs preserved, while keeping device-upgrade qualification and release approval separate from source landing.

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

Yes: current main accepts zero or short SDK returns as success, and the pinned SDK explicitly returns zero on a failed nonempty write. This was established from source, without executing a current-main reproduction.

Is this the best way to solve the issue?

Yes: comparing the returned byte count with the complete serialized length reuses existing failure paths, while stack-local numeric diagnostics preserve envelopes, ownership, and logging controls.

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: The cumulative branch changes camera waits from indefinite to two seconds, so previously slow captures can now fail under an explicitly accepted policy.
• add proof: sufficient: Contributor real behavior proof is sufficient. The supplied body records the real Tab5 camera-to-Gateway path on historical firmware 5ec58db: a zero return was classified as failure, followed by full acceptance of 82,374 bytes and a decoded JPEG. This supports the changed protocol behavior and resolves the earlier hardware-proof concern; it does not qualify a newly integrated landing tree.

Label justifications:

• P2: This repairs a concrete send-result classification defect and adds bounded device diagnostics without evidence of an urgent fleet-wide regression.
• merge-risk: 🚨 compatibility: The cumulative branch changes camera waits from indefinite to two seconds, so previously slow captures can now fail under an explicitly accepted policy.
• rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The supplied body records the real Tab5 camera-to-Gateway path on historical firmware 5ec58db: a zero return was classified as failure, followed by full acceptance of 82,374 bytes and a decoded JPEG. This supports the changed protocol behavior and resolves the earlier hardware-proof concern; it does not qualify a newly integrated landing tree.
• proof: sufficient: Contributor real behavior proof is sufficient. The supplied body records the real Tab5 camera-to-Gateway path on historical firmware 5ec58db: a zero return was classified as failure, followed by full acceptance of 82,374 bytes and a decoded JPEG. This supports the changed protocol behavior and resolves the earlier hardware-proof concern; it does not qualify a newly integrated landing tree.

Evidence

What I checked:

• Main still contains the reported defect: The default-branch JSON sender still returns written >= 0, accepting zero and short writes. The reviewed branch instead requires a positive return equal to the serialized length. (components/esp-openclaw-node/src/esp_openclaw_node_protocol.c:42, 6b63951f0ac5)
• Affirmative WebSocket dependency boundary: The Tab5 manifest selects third_party/esp-protocols for its WebSocket client; .gitmodules identifies espressif/esp-protocols, and the gitlink pins b7f3de500a4fd4b58e4248093067058959833260. (examples/m5stack-tab5-room-node/main/idf_component.yml:27, 8e8249730933)
• SDK confirms zero-return failure: The pinned send implementation treats zero for a nonempty transport write as failure and returns that zero unchanged; successful completion returns the accumulated byte count. (components/esp_websocket_client/esp_websocket_client.c:705, b7f3de500a4f)
• Focused regression coverage: Six test methods exercise actual protocol functions with real cJSON and stubbed handlers/transport: complete, negative, zero and short sends; failure callbacks; serialization failure; cleanup ordering; bounded diagnostics; and quiet logging. Tests were inspected, not executed during this read-only review. (scripts/tests/test_node_reply_diagnostics.py:52, 8e8249730933)
• Updated physical observations and review continuity: The complete supplied body, captured under sourceRevision 644639a4202d0d97248e2a215b57ee2601bba795bc1246de9d7729ada2fef64a, reports flashing historical candidate 5ec58db with independent readbacks. A 69,546-byte response returned zero and was classified as failed; a later 82,374-byte response completed and the Gateway returned a decoded 630x1120 JPEG. This supersedes the previous review's unexercised-hardware statement, without proving a newly composed landing tree or explaining intermittent transport failures. (8e8249730933)
• Stack scope and accepted compatibility policy: The verified merge-base delta contains 26 files and includes the preceding camera-timeout repair. fix(tab5): bound camera dequeue waits #53 explicitly accepts a two-second dequeue allowance and existing unavailable-result cleanup, including the compatibility consequence for slower cameras. This PR requests landing afterward with a merge commit preserving signed history; neither disposition authorizes release. (examples/m5stack-tab5-room-node/components/tab5_room_board/tab5_room_board.c:760, 8e8249730933)

Likely related people:

• unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)
• unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)

Rank-up moves

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

• Verify the cumulative tree and native checks after integrating the predecessor and resolving current-main conflicts, preserving signed history and accepted repairs.

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-10T22:31:20.595Z sha 8e82497 :: needs changes before merge. :: none
• reviewed 2026-09-10T23:00:23.799Z sha 8e82497 :: blocked before merge. :: none