fix(sessions): complete tail-read windows despite short positional reads#108655
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed July 16, 2026, 2:10 AM ET / 06:10 UTC. Summary PR surface: Source +1, Tests +84. Total +85 across 2 files. Reproducibility: yes. from source and deterministic short-read injection: current main performs one positional read, while the branch’s mocked production-path test caps reads at 16 bytes and exercises the failure contract. A real NFS or production-entrypoint run was not supplied. Review metrics: none identified. Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep the shared-helper substitution, move the short-read coverage into a focused companion test file, cover the preview path, and provide redacted terminal output invoking the real recent-message and preview entrypoints under capped positional reads. Do we have a high-confidence way to reproduce the issue? Yes from source and deterministic short-read injection: current main performs one positional read, while the branch’s mocked production-path test caps reads at 16 bytes and exercises the failure contract. A real NFS or production-entrypoint run was not supplied. Is this the best way to solve the issue? Yes for the implementation boundary: reusing the existing full-window helper is narrower and more maintainable than duplicating a loop. The branch still needs its tests relocated, preview coverage, and stronger entrypoint-level proof before merge. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 5f2828b50ada. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +1, Tests +84. Total +85 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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. How this review workflow works
|
164fe76 to
32775c1
Compare
Replace single-shot handle.read() calls in two session tail-read paths with readFileWindowFully so multibyte-range positional reads do not silently return incomplete tail data on network filesystems. - readRecentTranscriptTailLinesAsync: tail-window read for recent session messages now loops until the requested window fills - readLastMessagePreviewFromOpenTranscriptAsync: last-message preview tail read now loops until the requested window fills The same readFileWindowFully helper was introduced in openclaw#108253 and expanded with a sync variant in openclaw#108127 (both by sunlit-deng).
32775c1 to
031f724
Compare
|
Maintainer pass completed the short-read repair across every sibling path.
Node's file-read contract permits fewer bytes than requested, so each caller must use the returned byte count and continue until its window is filled or EOF is reached: https://nodejs.org/api/fs.html#fsreadfd-buffer-offset-length-position-callback |
|
Merged via squash.
|
…ads (openclaw#108655) * fix(sessions): complete tail-read windows despite short positional reads Replace single-shot handle.read() calls in two session tail-read paths with readFileWindowFully so multibyte-range positional reads do not silently return incomplete tail data on network filesystems. - readRecentTranscriptTailLinesAsync: tail-window read for recent session messages now loops until the requested window fills - readLastMessagePreviewFromOpenTranscriptAsync: last-message preview tail read now loops until the requested window fills The same readFileWindowFully helper was introduced in openclaw#108253 and expanded with a sync variant in openclaw#108127 (both by sunlit-deng). * fix(sessions): complete sync title preview reads --------- Co-authored-by: Peter Steinberger <[email protected]>
2 participants
What Problem This Solves
Fixes an issue where
readRecentSessionMessagesAsyncandreadLastMessagePreviewFromOpenTranscriptAsynccould silently return incomplete tail data when a positionalhandle.read()returned fewer bytes than requested — a documented POSIX behavior, common on network filesystems (NFS, FUSE) and container bind mounts.On
main, both paths callhandle.read(buffer, 0, readLen, readStart)once and usebytesReadas-is. When the read returns short, the remaining buffer bytes are garbage and the caller surfaces truncated messages ornullpreviews without any error.The same short-read anti-pattern was recently fixed by sunlit-deng in:
src/agents/bootstrap-files.ts(fix(agents): keep continuation bootstrap marker across short reads #108253) —readFileWindowFullyintroducedsrc/commands/sessions-tail.ts(fix(sessions): keep tail follow cursor aligned with bytes actually read #108127) — sync variantreadFileWindowFullySyncaddedWhy This Change Was Made
Both tail-read paths now call the existing
readFileWindowFullyhelper instead of a single-shothandle.read(). The helper loops until the requested buffer window fills or the file reaches EOF. This is a mechanical substitution — the readStart, readLen, and all downstream line-splitting and filtering logic are unchanged.User Impact
Session history reads and last-message previews return complete data on network filesystems. There is no API shape or configuration change, no additional I/O on local filesystems (the loop executes exactly once), and no change to byte/line caps.
Evidence
On current
main, a proof script capping eachhandle.read()at 16 bytes to simulate NFS reproduces the short read:With this change, the same short-read proxy produces the same result as an uncapped local read, validated by two new vitest tests:
The complete owning test file also passes, including the existing title-fields, usage, preview, and oversized-line coverage: