[world-local] Tidy stream chunk list code#1494
Conversation
Negative startIndex values (e.g. -3) resolve to n chunks before the known end of the stream. All world implementations (local, postgres, vercel) support this. Includes unit tests, e2e test, and doc updates. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Peter Wielander <mittgfu@gmail.com>
Replaces three separate tests (no startIndex, positive, negative) with a single describe block that iterates over startIndex cases. Fixes the negative startIndex test by waiting for workflow completion before connecting the reader — the backend resolves negative indices at connection time using knownChunkCount, which is 0 if the stream hasn't been fully written yet. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replaces three separate tests (no startIndex, positive, negative) with a single describe block that iterates over startIndex cases. Fixes the negative startIndex test by waiting for workflow completion before connecting the reader — the backend resolves negative indices at connection time using knownChunkCount, which is 0 if the stream hasn't been fully written yet. Signed-off-by: Peter Wielander <mittgfu@gmail.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Clarify that negative startIndex is for custom consumers, not WorkflowChatTransport - Add live-stream caveat for negative startIndex resolution timing - Add pagination limitation callout for live streams - Use explicit typeof guards for negative startIndex checks (world-local, world-postgres) - Add cost comment for EOF marker disk read in world-local Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Pranay Prakash <pranay.gp@gmail.com> Signed-off-by: Peter Wielander <mittgfu@gmail.com>
Signed-off-by: Peter Wielander <mittgfu@gmail.com>
Signed-off-by: Peter Wielander <mittgfu@gmail.com>
Signed-off-by: Peter Wielander <mittgfu@gmail.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Peter Wielander <mittgfu@gmail.com>
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
Clean refactoring — the listChunkFilesForStream helper is a correct, character-for-character extraction of the duplicated file-listing logic from getStreamChunks and getStreamInfo. The new unit tests are solid and cover pagination, in-progress streams, empty/nonexistent streams, invalid cursors, and single-chunk edge cases.
What I verified:
- The extracted helper logic is semantically identical to both original inline blocks (only cosmetic differences: a removed comment, variable renames
fileExtMap→extMap/chunkFiles→filesrestored via destructuring at call sites). - The helper is correctly defined outside
createStreamerand takestagas an explicit parameter rather than closing over it. - The helper uses its own local
tagSfx, distinct fromcreateStreamer'stagSuffix— no variable shadowing issues. - All downstream references to
chunkFilesandfileExtMapin both methods are preserved via destructuring aliases. - Test coverage is good:
getStreamChunkstests pagination, in-progress, nonexistent, and invalid cursor;getStreamInfotests completed, in-progress, nonexistent, and single-chunk. - 100 CI checks pass. However, the DCO check fails because the first commit ("c") lacks a
Signed-off-byline.
Two issues:
-
Blocking (DCO): The first commit
5d243a3has message "c" without aSigned-off-byline, failing the DCO check. This needs a rebase/amend to add the sign-off. -
Non-blocking (missed third duplicate):
readFromStream(~lines 472-498) contains an identical copy of the same file-listing logic that was not refactored. See inline comment.
| @@ -55,6 +55,43 @@ export function deserializeChunk(serialized: Buffer) { | |||
| return { eof, chunk }; | |||
There was a problem hiding this comment.
Non-blocking: readFromStream (around lines 472-498 in the current file) contains a third identical copy of the file-listing logic that this PR extracts into listChunkFilesForStream. Since the whole point of this PR is DRYing up this pattern, it would be worth updating readFromStream too:
const { files: chunkFiles, extMap: fileExtMap } =
await listChunkFilesForStream(chunksDir, name, tag);This would eliminate the last remaining duplicate. Fine to do in a follow-up if you prefer.
There was a problem hiding this comment.
Done — replaced the inline file-listing logic in readFromStream with listChunkFilesForStream(chunksDir, name, tag). All 41 streamer tests pass.
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
Updating my review — the DCO sign-off is a process issue for the author to fix, not a code blocker.
The refactoring is correct and the tests are solid. LGTM with one optional suggestion (see prior inline comment about the third duplicate in readFromStream).
Addresses review feedback: replaces the third inline copy of the file-listing logic in readFromStream with the extracted helper. Signed-off-by: Peter Wielander <peter@vercel.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ghost
mentioned this pull request
2 participants
No description provided.