fix(room): bound file parent traversal and preflight

[SebTardif]

file.write could walk past its parent path's terminating NUL when the configured storage root disappeared after command registration. Parent creation now starts at the root boundary, safely recreating the root before any missing descendants. The related preflight traversal now checks the existing root itself, so createParents: true can validate a new nested path without creating directories or files.

This retains @SebTardif's reported fix and contributor credit, replaces the standalone walk helper with the bounded production traversal, and adds registered-command regression coverage. No public API, dependency version, or release changes.

Validation:

• Before: the registered handler triggered an ASan stack-buffer-overflow after root loss; a new-directory preflight returned PARENT_NOT_FOUND; nested root recovery returned WRITE_ERROR.
• After: run_file_host_tests.py passes all four scenarios with ASan/UBSan and patterned stack initialization. It compiles the complete production file-command source with real cJSON and mbedTLS, uses a synthetic Node registration boundary, and executes real temporary filesystem writes. It verifies content/SHA-256, mutation-free preflight, missing-parent errors, and symlink rejection.
• Existing file-validation, metrics, PCM gain, and audio-port compatibility checks pass. The SDK-backed script suite passed 103 tests; its two component-dependent groups also passed when run from the configured Tab5 project; actionlint and git diff --check pass. Independent autoreview is clean through P2.
• The new registered-command suite is wired into CI after the component-test firmware build supplies its dependencies. All five native firmware builds passed: Unity test app, ESP32 node, ESP-BOX-3, Waveshare S3, and Tab5 P4. The Tab5 build used the documented exact SDK base and verified SDK/SDIO/camera patches. The real-header host suites, 49 room lifecycle cases, and three UI profiles passed. All five exact-head CI builds passed at 07638b667b83c23b757861e694bea19f5ca19acd. Physical SD-card removal has not been tested.

[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 15, 2026, 11:42 PM ET / September 16, 2026, 03:42 UTC (Revision 19).

ClawSweeper review

What this changes

The PR bounds room-node file parent traversal, permits mutation-free preflight beneath an existing root, and adds registered-command filesystem coverage to CI.

Merge readiness

⛔ Blocked before merge - 1 item remains

This remains a useful fix: current main retains both traversal defects, and no replacement was identified. The updated production-handler validation addresses the earlier proof request; no blocking patch defect was found.

Priority: P2
Reviewed head: 07638b667b83c23b757861e694bea19f5ca19acd

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	A focused repair with relevant production-handler validation and no blocking findings.
Proof confidence	🐚 platinum hermit (4/6)	Sufficient (terminal): The captured before/after results exercise production room_files.c through registered file.write using real JSON, crypto, and host filesystem operations, showing root recovery, preserved content/hash, mutation-free preflight, and symlink rejection under sanitizers. This satisfies the prior production-handler request without claiming physical SD-card qualification.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): The captured before/after results exercise production room_files.c through registered file.write using real JSON, crypto, and host filesystem operations, showing root recovery, preserved content/hash, mutation-free preflight, and symlink rejection under sanitizers. This satisfies the prior production-handler request without claiming physical SD-card qualification.
Evidence reviewed	7 items	Current main still contains the defects: The default-branch implementation starts traversal one byte beyond the configured-root boundary and excludes the root itself from the preflight ancestor search. A direct child of a disappeared root therefore starts the former walk beyond its terminating NUL; missing immediate parents fail preflight despite an existing root. Introduced fix and containment checks: The pinned patch starts at the root boundary and allows preflight to inspect that root. Inspection through final writes confirmed that normalization, symlink rejection, canonical containment, overwrite handling, and preflight mutation suppression remain present. Production-path proof addresses prior feedback: The captured PR body, sourceRevision a4defa4d56cc05c36a1e11e374b6aa125474e04181ee3f9f5e569a87cc387419, reports before-fix ASan overflow and preflight/root-recovery failures, followed by four passing after-fix scenarios. The inspected harness registers the actual file.write handler, passes JSON through production parsing and crypto, removes a real directory, and verifies recreated directories, file bytes, SHA-256, and absence of preflight mutation. Registration is synthetic; filesystem operations are real. This addresses the previous review's specific request to replace extracted-loop evidence with registered-handler coverage.
Findings	None	None.
Security	None	None.

How this fits together

Room nodes expose file commands when their board supplies an approved storage root. Incoming file-write requests pass through path validation and optional parent creation before returning a preflight result or writing decoded content.

flowchart TD
  A[Board storage root] --> C[File command handler]
  B[File write request] --> C
  C --> D[Validate path and parents]
  D --> E{Preflight only}
  E -->|Yes| F[Return validation result]
  E -->|No| G[Create parents and write file]

Loading

Before merge

• Resolve merge risk (P1) - The reported host validation does not establish ESP-IDF VFS behavior or physical SD-card removal recovery.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production and test delta	Production +3/-3; test code +205/-0	The runtime repair adds no net production lines; the added harness covers four filesystem scenarios.

Merge-risk options

Maintainer options:

1. Decide the mitigation before merge
   Keep the bounded traversal in the existing file-command owner, with registered-handler regression coverage protecting root recovery and mutation-free preflight.
2. Pause or close
   Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Keep the bounded traversal in the existing file-command owner, with registered-handler regression coverage protecting root recovery and mutation-free preflight.

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

Yes: register an existing root, remove it, then request a direct-child write with createParents; current-main source proves the out-of-bounds starting position. The PR reports the corresponding sanitizer failure, but this review did not execute it.

Is this the best way to solve the issue?

Yes: correcting the two boundary conditions in the existing implementation is a narrow repair, and the new harness exercises the actual handler rather than duplicating its traversal.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against 7b8b32db7f12.

Labels

Label changes:

• add proof: sufficient: Contributor real behavior proof is sufficient. The captured before/after results exercise production room_files.c through registered file.write using real JSON, crypto, and host filesystem operations, showing root recovery, preserved content/hash, mutation-free preflight, and symlink rejection under sanitizers. This satisfies the prior production-handler request without claiming physical SD-card qualification.
• add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
• add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The captured before/after results exercise production room_files.c through registered file.write using real JSON, crypto, and host filesystem operations, showing root recovery, preserved content/hash, mutation-free preflight, and symlink rejection under sanitizers. This satisfies the prior production-handler request without claiming physical SD-card qualification.
• remove status: 📣 needs proof: Current PR status label is status: 👀 ready for maintainer look.
• remove rating: 🦐 gold shrimp: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.

Label justifications:

• P2: This repairs bounded file-write and preflight failures on storage-enabled room nodes without evidence of a widespread current outage.
• 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 (terminal): The captured before/after results exercise production room_files.c through registered file.write using real JSON, crypto, and host filesystem operations, showing root recovery, preserved content/hash, mutation-free preflight, and symlink rejection under sanitizers. This satisfies the prior production-handler request without claiming physical SD-card qualification.
• proof: sufficient: Contributor real behavior proof is sufficient. The captured before/after results exercise production room_files.c through registered file.write using real JSON, crypto, and host filesystem operations, showing root recovery, preserved content/hash, mutation-free preflight, and symlink rejection under sanitizers. This satisfies the prior production-handler request without claiming physical SD-card qualification.

Evidence

What I checked:

• Current main still contains the defects: The default-branch implementation starts traversal one byte beyond the configured-root boundary and excludes the root itself from the preflight ancestor search. A direct child of a disappeared root therefore starts the former walk beyond its terminating NUL; missing immediate parents fail preflight despite an existing root. (components/esp-openclaw-room-node/room_files.c:401, 7b8b32db7f12)
• Introduced fix and containment checks: The pinned patch starts at the root boundary and allows preflight to inspect that root. Inspection through final writes confirmed that normalization, symlink rejection, canonical containment, overwrite handling, and preflight mutation suppression remain present. (components/esp-openclaw-room-node/room_files.c:401, 07638b667b83)
• Production-path proof addresses prior feedback: The captured PR body, sourceRevision a4defa4d56cc05c36a1e11e374b6aa125474e04181ee3f9f5e569a87cc387419, reports before-fix ASan overflow and preflight/root-recovery failures, followed by four passing after-fix scenarios. The inspected harness registers the actual file.write handler, passes JSON through production parsing and crypto, removes a real directory, and verifies recreated directories, file bytes, SHA-256, and absence of preflight mutation. Registration is synthetic; filesystem operations are real. This addresses the previous review's specific request to replace extracted-loop evidence with registered-handler coverage. (components/esp-openclaw-room-node/tests/test_room_files.c:93, 07638b667b83)
• CI and dependency boundary: The added CI command runs after the component build. The runner compiles the complete file-command source using existing cJSON and ESP-IDF mbedTLS sources; these are affirmative dependency signals for the harness. The patch adds no dependency version, download, action reference, permission, or credential change. (.github/workflows/ci.yml:119, 07638b667b83)
• Feature-history routing: GitHub's path history on pinned main identifies steipete's shared-room-runtime extraction. The supplied merged feat(tab5): add reusable room runtime and Tab5 node #19 establishes related room-runtime work, not a fix for this defect. Local git log --follow could not complete because an older object was unavailable; GitHub commit metadata provided the routing fallback. Exact semantic introduction is not claimed. (components/esp-openclaw-room-node/room_files.c, 17837cee4852)
• Related-work and release checks: The bounded all-state pull-request listing returned only this PR for file, parent, or storage titles. The supplied related Talk redirect PR describes a separate failure. GitHub's latest-release endpoint returned 404, so no shipped-release claim is made; pinned main demonstrably remains unfixed.

Likely related people:

• steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

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 (18 earlier review cycles; latest 8 shown)

• reviewed 2026-09-02T22:00:01.885Z sha b769f01 :: needs real behavior proof before merge. :: none
• reviewed 2026-09-03T08:53:51.210Z sha b769f01 :: needs real behavior proof before merge. :: none
• reviewed 2026-09-03T15:54:46.838Z sha b769f01 :: needs real behavior proof before merge. :: none
• reviewed 2026-09-03T16:06:19.820Z sha b769f01 :: needs real behavior proof before merge. :: none
• reviewed 2026-09-04T02:52:42.247Z sha b769f01 :: needs real behavior proof before merge. :: none
• reviewed 2026-09-04T12:00:05.556Z sha b769f01 :: needs real behavior proof before merge. :: none
• reviewed 2026-09-05T17:59:07.064Z sha b769f01 :: needs real behavior proof before merge. :: none
• reviewed 2026-09-07T14:38:54.466Z sha b769f01 :: needs real behavior proof before merge. :: none