fix(room): advance directory pages past skipped entries

[steipete]

dir.list can repeat files on later pages when it skips an entry whose metadata cannot be read. Its page token used offset + added, even though skipped entries also advance the directory iterator. Continue from the actual iterator position, excluding the lookahead entry that belongs to the next page.

The regression calls the registered production handler against a real temporary directory. It injects EIO for two entries in the filesystem’s actual iteration order and checks that page sizes 1, 2 and 5 return every readable entry exactly once. Directory iteration, JSON and the remaining filesystem operations are real. Documentation retains the existing limitation that pagination is not a snapshot of a changing directory.

Validation on a remote Linux host:

python3 components/esp-openclaw-room-node/tests/run_file_host_tests.py \
  --cjson-dir /tmp/esp-sweep-deps/cJSON \
  --mbedtls-dir /tmp/esp-sweep-deps/mbedtls
python3 components/esp-openclaw-room-node/tests/test_room_audio_port_compat.py
CJSON_DIR=/tmp/esp-sweep-deps/cJSON GIT_DISCOVERY_ACROSS_FILESYSTEM=1 \
  /tmp/esp-sweep-python/bin/python -m unittest discover -s scripts/tests -v

• Before the production fix, the new pagination scenario aborts at assert(!returned[index]) because a file is returned twice.
• After the fix, all five registered file-command scenarios pass with ASan/UBSan and strict compiler warnings; the audio/display-port compatibility test passes.
• Python: 103 tests, OK with two configured-vendor-source skips. The first broad attempt lacked CJSON_DIR and encountered Git’s filesystem-boundary discovery diagnostic; both were resolved in the test environment before rerunning.
• Host dependencies: cJSON v1.7.19 and mbedTLS 3.6.4. No physical SD card, ESP-IDF VFS or device execution is claimed.
• Independent Codex autoreview: scoped-clean at P0–P2, no actionable findings.

[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: needs maintainer review before merge. Reviewed September 23, 2026, 3:49 AM ET / 07:49 UTC.

ClawSweeper review

What this changes

Corrects room-node directory pagination after skipped entries and adds fault-injection coverage, documentation, and a changelog entry.

Merge readiness

✅ Ready for maintainer review

This PR remains useful: current main still has the pagination defect. The focused correction and production-handler filesystem evidence support it, with no actionable correctness findings.

Priority: P2
Reviewed head: c8e0e59e1c82749352865e17b642f4fdb475d273

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	A narrow, maintainable correction with relevant production-path fault-injection evidence and no identified blockers.
Proof confidence	🐚 platinum hermit (4/6)	Sufficient (terminal): The supplied Linux before/after execution report exercises the registered production directory handler with real filesystem iteration and JSON, injecting two metadata failures and observing complete, duplicate-free pagination afterward. The inspected harness matches that claim; device-specific storage behavior is outside its scope. No stored-data contract changes.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): The supplied Linux before/after execution report exercises the registered production directory handler with real filesystem iteration and JSON, injecting two metadata failures and observing complete, duplicate-free pagination afterward. The inspected harness matches that claim; device-specific storage behavior is outside its scope. No stored-data contract changes.
Evidence reviewed	6 items	Introduced change: The pinned base-to-head diff changes the continuation calculation from offset plus returned entries to visited entries minus the lookahead entry; the remaining changes are regression coverage and documentation. Current-main defect remains: Main increments seen before metadata inspection, skips unreadable entries, and still returns offset + added. Those counters diverge after a skip, causing later pages to revisit entries. The PR preserves the existing numeric token format and path validation. Production-path proof: The complete supplied PR body, captured under sourceRevision 22256b4156932db185fa7d755b3e3671223e6603702c583e49d34210c46e5ef4, reports a remote Linux before/after run: duplicate-entry assertion failure before the fix and five passing scenarios afterward under ASan/UBSan. The inspected harness compiles production room_files.c, captures the registered handler, uses real cJSON and filesystem iteration, injects EIO for two entries, and verifies all three readable files appear exactly once with page sizes 1, 2, and 5. This review did not execute the harness; physical SD-card and ESP-IDF VFS coverage are expressly unclaimed.
Findings	None	None.
Security	None	None.

How this fits together

The room-node file service exposes a board-approved storage directory through registered commands. Directory-list requests produce bounded JSON pages and a continuation token for subsequent requests.

flowchart LR
  A[Directory request and page token] --> B[Validate approved storage path]
  B --> C[Visit directory entries]
  C --> D[Skip unreadable metadata]
  C --> E[Collect bounded result page]
  D --> C
  E --> F[Return entries and iterator token]

Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production and test delta	Production +2/-1; tests and harness +100/-2	Production growth is limited to the corrected cursor expression and explanatory comment, with focused regression coverage.

Technical review

Best possible solution:

Retain the existing listing API while making continuation tokens reflect every visited entry and preserving the documented non-snapshot behavior.

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

Yes: current-main source establishes the counter mismatch when metadata reads fail during a paginated listing, and the contributor reports a failing-before regression. This read-only review did not execute it.

Is this the best way to solve the issue?

Yes: using the actual iterator position fixes the mismatch without changing the command contract or introducing a parallel pagination mechanism.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against 9d905d30e549.

Labels

Label changes:

• add P2: Repeated directory entries after metadata failures are a bounded correctness defect in optional storage commands.
• add proof: sufficient: Contributor real behavior proof is sufficient. The supplied Linux before/after execution report exercises the registered production directory handler with real filesystem iteration and JSON, injecting two metadata failures and observing complete, duplicate-free pagination afterward. The inspected harness matches that claim; device-specific storage behavior is outside its scope. No stored-data contract changes.
• 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 supplied Linux before/after execution report exercises the registered production directory handler with real filesystem iteration and JSON, injecting two metadata failures and observing complete, duplicate-free pagination afterward. The inspected harness matches that claim; device-specific storage behavior is outside its scope. No stored-data contract changes.

Label justifications:

• P2: Repeated directory entries after metadata failures are a bounded correctness defect in optional storage commands.
• 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 supplied Linux before/after execution report exercises the registered production directory handler with real filesystem iteration and JSON, injecting two metadata failures and observing complete, duplicate-free pagination afterward. The inspected harness matches that claim; device-specific storage behavior is outside its scope. No stored-data contract changes.
• proof: sufficient: Contributor real behavior proof is sufficient. The supplied Linux before/after execution report exercises the registered production directory handler with real filesystem iteration and JSON, injecting two metadata failures and observing complete, duplicate-free pagination afterward. The inspected harness matches that claim; device-specific storage behavior is outside its scope. No stored-data contract changes.

Evidence

What I checked:

• Introduced change: The pinned base-to-head diff changes the continuation calculation from offset plus returned entries to visited entries minus the lookahead entry; the remaining changes are regression coverage and documentation. (components/esp-openclaw-room-node/room_files.c:259, c8e0e59e1c82)
• Current-main defect remains: Main increments seen before metadata inspection, skips unreadable entries, and still returns offset + added. Those counters diverge after a skip, causing later pages to revisit entries. The PR preserves the existing numeric token format and path validation. (components/esp-openclaw-room-node/room_files.c:259, 9d905d30e549)
• Production-path proof: The complete supplied PR body, captured under sourceRevision 22256b4156932db185fa7d755b3e3671223e6603702c583e49d34210c46e5ef4, reports a remote Linux before/after run: duplicate-entry assertion failure before the fix and five passing scenarios afterward under ASan/UBSan. The inspected harness compiles production room_files.c, captures the registered handler, uses real cJSON and filesystem iteration, injects EIO for two entries, and verifies all three readable files appear exactly once with page sizes 1, 2, and 5. This review did not execute the harness; physical SD-card and ESP-IDF VFS coverage are expressly unclaimed. (components/esp-openclaw-room-node/tests/test_room_files.c:56, c8e0e59e1c82)
• Existing host-harness precedent: Merged fix(room): bound file parent traversal and preflight #31 established this registered-command harness for a distinct file-write defect, using real JSON, crypto, and temporary filesystem operations. It does not fix directory pagination. (components/esp-openclaw-room-node/tests/run_file_host_tests.py, 31b2cf08cabc)
• Feature-history routing: GitHub commit metadata identifies steipete on the earlier shared-room-runtime extraction, including addition of room_files.c. Local file history also identifies the subsequent file-parent repair by Sebastien Tardif. Older blame and follow-history traversal could not complete because a historical object was unavailable; no exact introducing-line attribution is asserted. (components/esp-openclaw-room-node/room_files.c, 17837cee4852)
• Release and replacement check: The GitHub releases endpoint returned no releases. The inspected pull-request listing found no separate pagination replacement, and the live PR remains open and unmerged against the pinned main revision. No shipped fix is established.

Likely related people:

• steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• SebTardif: 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.

[steipete]

The pagination regression and component-test firmware build passed on c8e0e59. The two failed room-node jobs expose dependency drift already present in main: Tab5 resolves ESP Video 2.5.0 while its qualified CSI patch requires 2.4.1, and LVGL 9.6 moved Canvas’s directly included private header.

Both failures were reproduced independently on the remote build host and are repaired in #59. Its final candidate built the Tab5 firmware, passed camera object/provenance verification, all 20 camera compatibility tests, four SDIO allocator tests, and all three real LVGL 9.6 UI host scenarios. Please review/land #59 first; this PR remains awaiting review and the build repair. The pagination change itself has red/green registered-handler proof and scoped-clean P0–P2 autoreview.

[steipete]

#59 is now merged as e4988f4, with all five firmware jobs and CodeQL green. This branch includes the repaired main at c3caae1. Its diff against main remains only the pagination fix, regression tests and documentation; the merge preserved all changelog entries.

Independent autoreview of the updated complete diff is scoped-clean through P2. Fresh CI is running on this exact head before landing.

[steipete]

Merged as b9ea051 after all five firmware jobs and CodeQL passed on exact head c3caae1: https://github.com/openclaw/esp-openclaw-node/actions/runs/35837703448.

Remote red/green proof used python3 components/esp-openclaw-room-node/tests/run_file_host_tests.py --cjson-dir /tmp/esp-sweep-deps/cJSON --mbedtls-dir /tmp/esp-sweep-deps/mbedtls: the new pagination case failed with a duplicate-entry assertion before the production fix; all five scenarios passed afterward under ASan/UBSan. The audio/display-port compatibility test passed. The Python suite ran 103 tests successfully with two configured-vendor-source skips; the camera and SDIO source suites subsequently passed during #59’s configured build proof. Autoreview of this final diff against repaired main was scoped-clean through P2.

The original room-build failures were resolved in #59 before this final CI run. No physical SD-card/VFS or device execution is claimed. Main is pulled and clean at the merge commit.