refactor(cli): improve index jobs and query coverage UX - #234
Merged
Conversation
Summary by CodeRabbit
WalkthroughThis change adds Sequence Diagram(s)sequenceDiagram
participant CLI
participant ArchiveCommand
participant ArchiveIndex
participant Retrieval
participant Output
CLI->>ArchiveCommand: Parse --skip-unindexed
ArchiveCommand->>ArchiveIndex: Resolve queryable chapters
ArchiveCommand->>Retrieval: Execute scoped query
Retrieval-->>Output: Return paginated results
Output-->>CLI: Emit indexed-only warning and results
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
test/cli/archive/object.test.ts (1)
231-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a JSON assertion for
help.summaryCoverage. The JSON test does not check this field or its explanation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/cli/archive/object.test.ts` around lines 231 - 234, Add a JSON-output assertion in the relevant archive CLI test to verify help.summaryCoverage and its explanatory text, alongside the existing summary coverage assertions. Use the established archiveMockState JSON output and preserve the current text-output checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/args/parse.ts`:
- Around line 172-174: Update the argument parser configuration around the
"skip-unindexed" option so it is accepted only by query parsers and explicitly
rejected by non-query commands such as gc. Add negative tests covering
representative non-query commands and verify they fail when passed
--skip-unindexed.
In `@packages/cli/src/commands/archive-output/write/evidence.ts`:
- Line 71: Emit the JSONL skip-unindexed warning only on the first page of each
--all command. In packages/cli/src/commands/archive-output/write/evidence.ts:71,
pass the first-page state into writeEvidenceWithoutContinuation; update
writeFindHitsWithoutContinuation in
packages/cli/src/commands/archive-output/write/find.ts:110-121 and
writeListWithoutContinuation in
packages/cli/src/commands/archive-output/write/list.ts:102-113 to accept and
honor a suppression flag for subsequent JSONL pages.
In `@packages/cli/src/commands/queue/worker.ts`:
- Around line 359-366: Update the index-fts flow in the worker to pass
chapterTitles, summarySentences, chunks, and mentions from the chapter snapshot
to createFtsIndexArtifactInput alongside sentences, preserving all available FTS
row kinds. In test/cli/queue.test.ts:1049, extend the assertion to verify the
available non-source rows.
In `@packages/core/src/retrieval/index-artifact/output.ts`:
- Around line 23-36: The embedding metadata contracts are inconsistent across
createEmbeddingIndexArtifactInput, createOutputManifest, readNumberMetadata, and
manifestSchema. Make identity optional when the provider has no identity,
enforce nonnegative integer dimensions while reading and writing metadata, and
add a round-trip test covering a provider without identity.
- Around line 70-90: Update the stream-writing flow around createWriteStream and
closeWritableStream to apply backpressure: await the stream’s drain event
whenever write() returns false before emitting the next record. Add error
propagation for the stream so failures during writes or closing reject the
operation, while preserving manifest, lexical-row, segment output, and
guaranteed cleanup.
In `@packages/core/src/retrieval/query/continuation-cursor/payload.ts`:
- Line 125: Update getPayloadOptionalBoolean and its callers in the cursor
payload parsing flow so the boolean is assigned to the requested key, preserving
skipUnindexed and preventing it from overwriting backlinks. Add round-trip
coverage for search, evidence, and related cursors to verify skipUnindexed
survives persistence and resumption.
In `@packages/core/src/runtime/jobs/jobs.ts`:
- Around line 344-356: Make the queued-state check and paused-to-queued
transition atomic in the resume flow around requireBuildJobById and the
transition helper, so concurrent callers cannot both observe paused and cause
one to fail. Prefer keeping both operations on the same database transaction;
alternatively, make the transition helper treat an already-queued job as an
idempotent no-op without adding another event. Add a regression test covering
concurrent resume calls.
In `@packages/core/src/storage/schema-upgrade/index.ts`:
- Around line 335-367: Replace the all-stream textCache in the row-processing
loop with state for only the current stream, reloading when the stream name or
chapterId changes. Represent a loaded-but-missing stream explicitly so
subsequent rows for that stream are skipped without rereading it, while
preserving sentence extraction for available text and allowing previously held
buffers to be released.
In `@packages/core/src/utils/text-word-count.ts`:
- Around line 2-23: Update countTextWords and WORD_CHARACTER_PATTERN so Unicode
combining Mark characters continue an already active word without starting one
when inWord is false; preserve existing Han-character and separator behavior.
Add regression coverage for decomposed text such as e\u0301clair, verifying it
counts as one word and prevents incorrect words_count updates.
---
Nitpick comments:
In `@test/cli/archive/object.test.ts`:
- Around line 231-234: Add a JSON-output assertion in the relevant archive CLI
test to verify help.summaryCoverage and its explanatory text, alongside the
existing summary coverage assertions. Use the established archiveMockState JSON
output and preserve the current text-output checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3eb74a27-b7b8-4328-a281-fae85fc67464
📒 Files selected for processing (82)
packages/cli/src/args/archive-index.test.tspackages/cli/src/args/archive.test.tspackages/cli/src/args/archive.tspackages/cli/src/args/help.test.tspackages/cli/src/args/parse.tspackages/cli/src/args/types.tspackages/cli/src/args/uri/archive-objects.tspackages/cli/src/commands/archive-command/chapter.tspackages/cli/src/commands/archive-command/index.tspackages/cli/src/commands/archive-command/inspect.tspackages/cli/src/commands/archive-command/run/next.tspackages/cli/src/commands/archive-command/run/options.tspackages/cli/src/commands/archive-command/search-index.tspackages/cli/src/commands/archive-output/object/cursor.tspackages/cli/src/commands/archive-output/object/objects.tspackages/cli/src/commands/archive-output/object/types.tspackages/cli/src/commands/archive-output/write/evidence.tspackages/cli/src/commands/archive-output/write/find.tspackages/cli/src/commands/archive-output/write/list.tspackages/cli/src/commands/archive-output/write/warnings.tspackages/cli/src/commands/queue/output.tspackages/cli/src/commands/queue/worker.tspackages/core/data/help/commands/library.jinjapackages/core/data/help/commands/predicate.jinjapackages/core/data/help/commands/uri.jinjapackages/core/data/help/topics/library.jinjapackages/core/data/help/topics/readiness.jinjapackages/core/data/help/topics/uri.jinjapackages/core/src/api/chapter/generate.tspackages/core/src/api/chapter/manage.tspackages/core/src/document/directory/core.tspackages/core/src/document/directory/index-artifact-invalidation.test.tspackages/core/src/document/directory/serial-cleanup.tspackages/core/src/document/directory/types.tspackages/core/src/document/fragments/sentence.tspackages/core/src/document/text-streams/sentence.tspackages/core/src/graph/knowledge-build/commit.tspackages/core/src/graph/reading-build/artifact.tspackages/core/src/index.tspackages/core/src/library/query.tspackages/core/src/library/search-index.tspackages/core/src/library/search-query.tspackages/core/src/retrieval/index-artifact/build.test.tspackages/core/src/retrieval/index-artifact/index.tspackages/core/src/retrieval/index-artifact/output.tspackages/core/src/retrieval/query/archive-view/evidence.tspackages/core/src/retrieval/query/archive-view/index-state.test.tspackages/core/src/retrieval/query/archive-view/index-state.tspackages/core/src/retrieval/query/archive-view/index.tspackages/core/src/retrieval/query/archive-view/related/core.tspackages/core/src/retrieval/query/archive-view/related/entity.tspackages/core/src/retrieval/query/archive-view/related/query.tspackages/core/src/retrieval/query/archive-view/search/core.tspackages/core/src/retrieval/query/archive-view/search/hydration.tspackages/core/src/retrieval/query/archive-view/source-evidence/core.tspackages/core/src/retrieval/query/archive-view/text-streams.tspackages/core/src/retrieval/query/archive-view/types.tspackages/core/src/retrieval/query/continuation-cursor/payload.tspackages/core/src/retrieval/query/continuation-cursor/types.tspackages/core/src/retrieval/query/index.tspackages/core/src/retrieval/query/search-cache/sessions.tspackages/core/src/retrieval/query/search-cache/store.tspackages/core/src/runtime/jobs/jobs.tspackages/core/src/storage/schema-upgrade/index.tspackages/core/src/text/reader/segment/intl-segmenter.tspackages/core/src/text/serial/generation.tspackages/core/src/text/serial/source.tspackages/core/src/text/source/epub/content.tspackages/core/src/utils/text-word-count.test.tspackages/core/src/utils/text-word-count.tstest/cli/archive/chapter.test.tstest/cli/archive/evidence-pack.test.tstest/cli/archive/mock.tstest/cli/archive/object.test.tstest/cli/archive/query.test.tstest/cli/archive/related.test.tstest/cli/queue.test.tstest/core/api/build-queue.test.tstest/core/api/knowledge-graph-build.test.tstest/core/library/query.test.tstest/core/retrieval/query/search-cache.test.tstest/core/storage/schema-upgrade/index.test.ts
💤 Files with no reviewable changes (1)
- packages/core/src/text/serial/generation.ts
Moskize91
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/retrieval/index-artifact/output.ts`:
- Around line 407-445: Update writeIndexArtifactOutput so its stream error
listener remains installed after settle rejects a failed write, rather than
being removed by cleanup; keep that listener through closeWritableStream
completion and route stream errors through the existing rejection path. Add a
regression test using a failing Writable to verify the write callback error is
rejected without an unhandled stream error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aee6b311-719f-4fc2-af3a-56219b23898d
📒 Files selected for processing (17)
packages/cli/src/args/gc.test.tspackages/cli/src/args/parse.tspackages/cli/src/commands/archive-output/write/evidence.tspackages/cli/src/commands/archive-output/write/find.tspackages/cli/src/commands/archive-output/write/list.tspackages/cli/src/commands/queue/worker.tspackages/core/src/retrieval/index-artifact/build.test.tspackages/core/src/retrieval/index-artifact/output.tspackages/core/src/retrieval/query/continuation-cursor/payload.test.tspackages/core/src/retrieval/query/continuation-cursor/payload.tspackages/core/src/runtime/jobs/jobs.tspackages/core/src/storage/schema-upgrade/index.tspackages/core/src/utils/text-word-count.test.tspackages/core/src/utils/text-word-count.tstest/cli/archive/object.test.tstest/cli/queue.test.tstest/core/api/build-queue.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/cli/src/commands/archive-output/write/find.ts
- packages/core/src/runtime/jobs/jobs.ts
- test/cli/archive/object.test.ts
- packages/cli/src/commands/archive-output/write/evidence.ts
- test/cli/queue.test.ts
- packages/core/src/retrieval/query/continuation-cursor/payload.ts
- packages/cli/src/commands/archive-output/write/list.ts
- packages/core/src/storage/schema-upgrade/index.ts
- packages/cli/src/commands/queue/worker.ts
- packages/core/src/utils/text-word-count.ts
- packages/core/src/utils/text-word-count.test.ts
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/src/retrieval/index-artifact/build.test.ts (1)
325-357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the regression assertion explicit.
The test relies on the trailing
await new Promise((resolve) => setImmediate(resolve))to surface an unhandled"error"event. Whether that failure is reported depends on the test runner configuration for unhandled errors. The test can pass even if the implementation removes its"error"listener too early.Add a direct assertion instead. For example, record
this.listenerCount("error")inside the fixture at emit time and assert it is greater than zero.♻️ Proposed fixture change
class CallbackThenStreamErrorWritable extends Writable { readonly `#error` = new Error("write failed"); + public errorListenerCountAtEmit = -1; public override _final(callback: (error?: Error | null) => void): void { setTimeout(() => callback(), 10); } public override _write( _chunk: unknown, _encoding: BufferEncoding, callback: (error?: Error | null) => void, ): void { callback(this.#error); setImmediate(() => { + this.errorListenerCountAtEmit = this.listenerCount("error"); this.emit("error", this.#error); }); } }Then assert in the test:
await new Promise((resolve) => setImmediate(resolve)); expect(stream.errorListenerCountAtEmit).toBeGreaterThan(0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/retrieval/index-artifact/build.test.ts` around lines 325 - 357, Update CallbackThenStreamErrorWritable to record this.listenerCount("error") immediately before emitting the delayed error, expose that recorded count to the test, and replace the trailing timing-only wait with an assertion that errorListenerCountAtEmit is greater than zero after writeIndexArtifactOutputToStream rejects.packages/core/src/retrieval/index-artifact/output.ts (1)
455-479: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueBound the wait for
"close".
closePromisenever resolves if the stream does not emit"close". AWritablecreated withemitClose: falseproduces an unresolved await here.writeIndexArtifactOutputToStreamis exported, so a caller can pass such a stream and the queue worker then hangs without an error.Consider resolving the close wait when the stream reports
closedordestroyedafterend()completes, instead of requiring the event.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/retrieval/index-artifact/output.ts` around lines 455 - 479, Update writeIndexArtifactOutputToStream’s close-wait logic so completion does not depend solely on the "close" event: after stream.end() completes, resolve the wait when the stream reports closed or destroyed, including streams configured with emitClose: false. Preserve error propagation through streamErrors.race and the existing early handling for already-destroyed streams.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/retrieval/index-artifact/build.test.ts`:
- Around line 325-357: Update CallbackThenStreamErrorWritable to record
this.listenerCount("error") immediately before emitting the delayed error,
expose that recorded count to the test, and replace the trailing timing-only
wait with an assertion that errorListenerCountAtEmit is greater than zero after
writeIndexArtifactOutputToStream rejects.
In `@packages/core/src/retrieval/index-artifact/output.ts`:
- Around line 455-479: Update writeIndexArtifactOutputToStream’s close-wait
logic so completion does not depend solely on the "close" event: after
stream.end() completes, resolve the wait when the stream reports closed or
destroyed, including streams configured with emitClose: false. Preserve error
propagation through streamErrors.race and the existing early handling for
already-destroyed streams.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d8d7ccc-bf85-425b-9830-6afae881777d
📒 Files selected for processing (2)
packages/core/src/retrieval/index-artifact/build.test.tspackages/core/src/retrieval/index-artifact/output.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Notes