Repository navigation
Improve Wiki Graph archive reads and chapter state export - #87
Conversation
Summary by CodeRabbit
WalkthroughThis PR replaces chapter "stage" tracking with a computed "state" model (readiness per source/reading-graph/reading-summary/knowledge-graph) surfaced across CLI output, facade APIs, and help docs. It adds a Sequence Diagram(s)sequenceDiagram
participant CLI
participant ArchiveView
participant Document
participant Stdout
CLI->>ArchiveView: search/list request (--all)
loop until nextCursor is null
ArchiveView->>Document: compute chapter state / query results
Document-->>ArchiveView: readiness data / result page
ArchiveView-->>CLI: page + nextCursor
CLI->>Stdout: stream or accumulate page
end
CLI->>Stdout: write final merged/streamed output
Related PRs: None identified. Suggested labels: cli, facade, documentation, refactor Suggested reviewers: oomol-lab maintainers familiar with archive facade and CLI layers 🐰 A chapter's stage has grown up wise, 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/facade/archive-view.ts (1)
3557-3573: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winAvoid a DB round trip per chapter in these list/search paths.
createChapterStatecallsdocument.serials.getById(...), andReadonlySerialStore.getByIdis a direct SQL query, solistArchiveObjects,listArchiveCollection,findChapters,findChaptersLexical, andchapter-staterendering all fan out into repeated serial lookups. Batch-load or reuse serial states for the current result set, especially under--all.🤖 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 `@src/facade/archive-view.ts` around lines 3557 - 3573, `createChapterState` is doing a direct `document.serials.getById(...)` lookup for every chapter, which causes repeated DB round trips in the list/search and chapter-state paths. Update the archive listing flow in `archive-view.ts` so serial state is fetched once per result set and reused by `createChapterState`, or batch-load the needed serials before rendering `listArchiveObjects`, `listArchiveCollection`, `findChapters`, `findChaptersLexical`, and chapter-state output.src/cli/help.ts (1)
198-202: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale "stage" wording next to renamed "state" URI.
This
chapterobject's verb entry still says "Inspect one chapter stage and artifacts." forwkg://book.wikg/chapter/12/state get, while the new dedicatedchapter-stateentry a few lines below (line 239) calls the same command "Inspect aggregate chapter state." Given this PR replaces the stage-based model with a computed state model throughout, this leftover "stage" wording is inconsistent terminology in the same help object list.✏️ Proposed fix
{ command: "wikigraph wkg://book.wikg/chapter/12/state get", - note: "Inspect one chapter stage and artifacts.", + note: "Inspect one chapter's artifact readiness.", verb: "get", },🤖 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 `@src/cli/help.ts` around lines 198 - 202, The help text for the chapter command list still uses the old “stage” terminology in the `chapter` entry for `wkg://book.wikg/chapter/12/state get`, which is inconsistent with the renamed computed state model. Update the `note` string in the affected help object within `help.ts` so it matches the wording used by the `chapter-state` entry and refers to “state” instead of “stage,” keeping the terminology consistent across the command list.
🧹 Nitpick comments (1)
src/cli/archive.ts (1)
952-982: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider a safeguard against a non-advancing cursor.
writeAllFindHitsloops untilpage.nextCursor === nullwith no bound and no check that the cursor actually advances. A backend returning a repeating/stale non-nullnextCursorwould spin indefinitely (and, for non-jsonl, accumulate unbounded pages in memory). A simple guard—track the previous cursor and break if unchanged, or cap iterations—would harden this.🤖 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 `@src/cli/archive.ts` around lines 952 - 982, writeAllFindHits currently trusts page.nextCursor to advance, which can cause an infinite loop and unbounded page accumulation if the backend returns the same non-null cursor repeatedly. Update the cursor loop in writeAllFindHits to track the previous cursor (or similar safeguard) and stop if nextCursor does not change, while preserving the existing jsonl and merged-page write behavior.
🤖 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 `@src/cli/archive.ts`:
- Around line 1632-1638: The state-page text path is flattening page.state in
createPageObject(), so formatStatePageText() never sees the dedicated state
shape and falls back to formatPlainObject() for wkg://chapter/N/state. Fix this
by either preserving the nested state object in createPageObject() for the
"state" case or updating formatStatePageText() to recognize the flattened shape,
and add a text-format test covering the state URI to ensure it renders through
the state block formatter.
In `@src/cli/io.ts`:
- Around line 45-56: The broken-pipe handling in the IO promise flow falls
through to reject after calling process.exit(0), which makes the success path
fragile in tests and mocked environments. Update the catch/reject branch in the
function that uses isBrokenPipeError so that the EPIPE case returns immediately
after process.exit(0), ensuring reject(error) is never reached on that path and
the control flow is explicit.
In `@src/document/fragments.ts`:
- Line 154: The fragment directory snapshot cache in Fragments is being reused
after writes, so newly written fragment files can be missed by listFragmentIds()
and getFragment() on the same instance. Update the write paths in Fragments,
especially `#commitDraft`(), writeTextStream(), and any flow that calls
`#peekNextFragmentId`(), to invalidate or clear `#fileContents` immediately after
fragment_*.json is written. Make sure the cache is repopulated on the next read
so the in-memory snapshot always reflects the latest fragment set.
In `@src/facade/archive.ts`:
- Around line 83-91: The fire-and-forget close path in Archive.close() can թող
unhandled rejections from the lazy `#file` promise or file.close(). Update
Archive.close() to attach a rejection handler to the this.#file.then(...) chain,
using the existing `#file` field and close() method so any open/close failure is
swallowed or logged instead of becoming unhandled.
- Around line 513-532: The byte reads in readCompressedArchiveEntryBuffer ignore
bytesRead, so truncated archives can silently return zero-filled buffers and
corrupt entry data. Update this helper to read the local header and compressed
payload in a loop until the requested byte count is satisfied, or throw on EOF,
and use a small readExact-style helper inside readCompressedArchiveEntryBuffer
to make the behavior robust.
In `@src/facade/wikg-coordinator.ts`:
- Around line 1042-1052: The root prefix is not treated as a conflict in lock
path matching, so empty-directory reads can bypass write protection. Update lock
handling in lockPathsConflict and/or lockPathContains so that a normalized empty
prefix from normalizeEntryDirectoryPrefix("") conflicts with every non-empty
path, ensuring listFileContents("") blocks concurrent writes under the archive
root. Use the existing lockPathsConflict and lockPathContains helpers to apply
the root-prefix special case consistently.
- Around line 553-560: Refresh the entry-source cache after the SQLite
materialization path runs so database.db is no longer treated as archive-backed
after the workspace overlay is created or replaced. Update the cache in the code
path around acquireSqliteLease and observeDirtyEntry, using the relevant state
in WikgCoordinator/#entrySourceByPath so later reads and listing use the
materialized workspace file instead of stale archive bytes.
---
Outside diff comments:
In `@src/cli/help.ts`:
- Around line 198-202: The help text for the chapter command list still uses the
old “stage” terminology in the `chapter` entry for
`wkg://book.wikg/chapter/12/state get`, which is inconsistent with the renamed
computed state model. Update the `note` string in the affected help object
within `help.ts` so it matches the wording used by the `chapter-state` entry and
refers to “state” instead of “stage,” keeping the terminology consistent across
the command list.
In `@src/facade/archive-view.ts`:
- Around line 3557-3573: `createChapterState` is doing a direct
`document.serials.getById(...)` lookup for every chapter, which causes repeated
DB round trips in the list/search and chapter-state paths. Update the archive
listing flow in `archive-view.ts` so serial state is fetched once per result set
and reused by `createChapterState`, or batch-load the needed serials before
rendering `listArchiveObjects`, `listArchiveCollection`, `findChapters`,
`findChaptersLexical`, and chapter-state output.
---
Nitpick comments:
In `@src/cli/archive.ts`:
- Around line 952-982: writeAllFindHits currently trusts page.nextCursor to
advance, which can cause an infinite loop and unbounded page accumulation if the
backend returns the same non-null cursor repeatedly. Update the cursor loop in
writeAllFindHits to track the previous cursor (or similar safeguard) and stop if
nextCursor does not change, while preserving the existing jsonl and merged-page
write behavior.
🪄 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: cb55d747-19f2-45e4-bb4a-2ab4118652b9
📒 Files selected for processing (29)
data/help/commands/archive/list.jinjadata/help/commands/archive/search.jinjadata/help/commands/maintenance/chapter/list.jinjadata/help/commands/root.jinjadata/help/topics/ai.jinjadata/help/topics/command.jinjadata/help/topics/troubleshoot.jinjadata/help/topics/uri.jinjadocs/en/cli.mddocs/zh-CN/cli.mdsrc/cli/archive.tssrc/cli/args.tssrc/cli/help.tssrc/cli/io.tssrc/cli/json.tssrc/document/document.tssrc/document/fragments.tssrc/document/stores.tssrc/facade/archive-view.tssrc/facade/archive.tssrc/facade/chapter-build.tssrc/facade/chapter.tssrc/facade/spine-digest-file.tssrc/facade/wikg-coordinator.tstest/cli/archive.test.tstest/cli/args.test.tstest/facade/archive-view.test.tstest/facade/archive.test.tstest/facade/spine-digest-file.test.ts
💤 Files with no reviewable changes (1)
- data/help/topics/command.jinja
| case "state": { | ||
| const { id: _id, ...rest } = page; | ||
| if ("state" in page) { | ||
| return { ...page.state, uri: toWikiGraphUri(page.id) }; | ||
| } | ||
|
|
||
| return { ...rest, uri: toWikiGraphUri(page.id) }; | ||
| return { uri: toWikiGraphUri(page.id), value: page.value }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm formatStateBlock is only reachable via object.state and that the state page object is flattened.
rg -nP -C3 'formatStateBlock|formatStatePageText' src/cli/archive.ts
rg -nP -C4 '"state"\s*:' test/cli/archive.test.ts | rg -n 'format:\s*"text"' -C6 || echo "no text-format state-page assertion found"Repository: oomol-lab/wikigraph
Length of output: 1112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the state-page object construction and the surrounding text formatter.
sed -n '1590,1655p' src/cli/archive.ts
printf '\n----\n'
sed -n '1820,1868p' src/cli/archive.ts
printf '\n----\n'
# Look for tests covering the state page in text mode.
rg -n -C4 'wkg://chapter/.*/state|format:\s*"text"|formatStatePageText|formatStateBlock' test srcRepository: oomol-lab/wikigraph
Length of output: 22771
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect plain-object formatting and the relevant test expectations around state pages.
sed -n '1810,1868p' src/cli/archive.ts
printf '\n----\n'
sed -n '1410,1440p' test/cli/archive.test.ts
printf '\n----\n'
sed -n '1,120p' test/cli/archive.test.ts | sed -n '1,120p'Repository: oomol-lab/wikigraph
Length of output: 5660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the helpers used by both plain-object and state formatting.
sed -n '1760,1835p' src/cli/archive.ts
printf '\n----\n'
sed -n '1,40p' test/cli/archive.test.ts
printf '\n----\n'
sed -n '290,320p' test/cli/archive.test.tsRepository: oomol-lab/wikigraph
Length of output: 3898
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect value formatting to compare fallback plain-object output with state formatting.
rg -n -C3 'function formatPlainValue|function formatStateEntries|function comparePlainObjectKeys' src/cli/archive.ts
sed -n '1710,1815p' src/cli/archive.tsRepository: oomol-lab/wikigraph
Length of output: 3741
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the specific tests around state pages and the text/JSON expectations.
sed -n '540,590p' test/cli/archive.test.ts
printf '\n----\n'
sed -n '1408,1440p' test/cli/archive.test.ts
printf '\n----\n'
sed -n '150,220p' src/cli/archive.tsRepository: oomol-lab/wikigraph
Length of output: 4472
State-page text output never reaches the dedicated block formatter. createPageObject() flattens page.state, so formatStatePageText() falls back to formatPlainObject() for wkg://chapter/N/state. If this route should render as a state block, keep the nested state object here or teach the formatter to read the flattened shape; add a text-format test for the state URI.
🤖 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 `@src/cli/archive.ts` around lines 1632 - 1638, The state-page text path is
flattening page.state in createPageObject(), so formatStatePageText() never sees
the dedicated state shape and falls back to formatPlainObject() for
wkg://chapter/N/state. Fix this by either preserving the nested state object in
createPageObject() for the "state" case or updating formatStatePageText() to
recognize the flattened shape, and add a text-format test covering the state URI
to ensure it renders through the state block formatter.
| if (isBrokenPipeError(error)) { | ||
| process.exit(0); | ||
| } | ||
|
|
||
| reject(error); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| function isBrokenPipeError(error: Error): boolean { | ||
| return (error as NodeJS.ErrnoException).code === "EPIPE"; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add return after process.exit(0) to avoid falling through to reject.
process.exit() terminates the process synchronously and normally never returns control to the following line, so reject(error) at Line 49 is dead code in production. However, this makes the function fragile under any environment where process.exit is intercepted/mocked (e.g., unit tests spying on process.exit) — execution would then fall through and reject the promise despite the intended "exit(0)" success path. Add an explicit return; to make the control flow unambiguous and testable.
🔧 Proposed fix
if (isBrokenPipeError(error)) {
process.exit(0);
+ return;
}
reject(error);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (isBrokenPipeError(error)) { | |
| process.exit(0); | |
| } | |
| reject(error); | |
| }); | |
| }); | |
| } | |
| function isBrokenPipeError(error: Error): boolean { | |
| return (error as NodeJS.ErrnoException).code === "EPIPE"; | |
| } | |
| if (isBrokenPipeError(error)) { | |
| process.exit(0); | |
| return; | |
| } | |
| reject(error); | |
| }); | |
| }); | |
| } | |
| function isBrokenPipeError(error: Error): boolean { | |
| return (error as NodeJS.ErrnoException).code === "EPIPE"; | |
| } |
🤖 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 `@src/cli/io.ts` around lines 45 - 56, The broken-pipe handling in the IO
promise flow falls through to reject after calling process.exit(0), which makes
the success path fragile in tests and mocked environments. Update the
catch/reject branch in the function that uses isBrokenPipeError so that the
EPIPE case returns immediately after process.exit(0), ensuring reject(error) is
never reached on that path and the control flow is explicit.
| #draftOpen = false; | ||
| readonly #documentPath: string; | ||
| readonly #fileAccess: FragmentFileAccess; | ||
| #fileContents: Promise<ReadonlyMap<string, Uint8Array>> | undefined; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Invalidate the batch cache after fragment writes.
Line 350 memoizes a directory snapshot. In write flows, #peekNextFragmentId() can populate this cache before #commitDraft() or the empty writeTextStream() branch writes fragment_*.json, so later listFragmentIds()/getFragment() on the same instance can miss the fragment just written.
Proposed fix
await this.#writer.write(
this.#getFragmentPath(fragmentId),
JSON.stringify(
{
sentences: [],
summary: "",
},
undefined,
2,
),
);
+ this.#fileContents = undefined;
this.#nextFragmentId = fragmentId + 1;
return;
@@
await this.#writer.write(
this.#getFragmentPath(fragmentId),
JSON.stringify(
{
sentences,
summary,
},
undefined,
2,
),
);
+ this.#fileContents = undefined;
this.#nextFragmentId = fragmentId + 1;Also applies to: 191-200, 212-215, 345-351
🤖 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 `@src/document/fragments.ts` at line 154, The fragment directory snapshot cache
in Fragments is being reused after writes, so newly written fragment files can
be missed by listFragmentIds() and getFragment() on the same instance. Update
the write paths in Fragments, especially `#commitDraft`(), writeTextStream(), and
any flow that calls `#peekNextFragmentId`(), to invalidate or clear `#fileContents`
immediately after fragment_*.json is written. Make sure the cache is repopulated
on the next read so the in-memory snapshot always reflects the latest fragment
set.
| public close(): void { | ||
| this.#zipFile.close(); | ||
| if (this.#file !== undefined) { | ||
| void this.#file.then(async (file) => { | ||
| await file.close(); | ||
| }); | ||
| this.#file = undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Fire-and-forget file close can surface an unhandled rejection.
void this.#file.then(...) has no rejection handler. If the lazily-opened #file promise rejects (open failed) or file.close() rejects, the rejection goes unhandled, which recent Node versions treat as fatal. Attach a .catch.
🛡️ Proposed fix
public close(): void {
this.#zipFile.close();
if (this.#file !== undefined) {
- void this.#file.then(async (file) => {
- await file.close();
- });
+ void this.#file
+ .then(async (file) => {
+ await file.close();
+ })
+ .catch(() => undefined);
this.#file = undefined;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public close(): void { | |
| this.#zipFile.close(); | |
| if (this.#file !== undefined) { | |
| void this.#file.then(async (file) => { | |
| await file.close(); | |
| }); | |
| this.#file = undefined; | |
| } | |
| } | |
| public close(): void { | |
| this.#zipFile.close(); | |
| if (this.#file !== undefined) { | |
| void this.#file | |
| .then(async (file) => { | |
| await file.close(); | |
| }) | |
| .catch(() => undefined); | |
| this.#file = undefined; | |
| } | |
| } |
🤖 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 `@src/facade/archive.ts` around lines 83 - 91, The fire-and-forget close path
in Archive.close() can թող unhandled rejections from the lazy `#file` promise or
file.close(). Update Archive.close() to attach a rejection handler to the
this.#file.then(...) chain, using the existing `#file` field and close() method so
any open/close failure is swallowed or logged instead of becoming unhandled.
| async function readCompressedArchiveEntryBuffer( | ||
| file: FileHandle, | ||
| entry: Entry, | ||
| ): Promise<Buffer> { | ||
| const header = Buffer.alloc(30); | ||
|
|
||
| await file.read(header, 0, header.length, entry.relativeOffsetOfLocalHeader); | ||
| if (header.readUInt32LE(0) !== 0x04034b50) { | ||
| throw new Error(`Invalid ZIP local file header: ${entry.fileName}`); | ||
| } | ||
|
|
||
| const fileNameLength = header.readUInt16LE(26); | ||
| const extraFieldLength = header.readUInt16LE(28); | ||
| const dataOffset = | ||
| entry.relativeOffsetOfLocalHeader + 30 + fileNameLength + extraFieldLength; | ||
| const compressed = Buffer.alloc(entry.compressedSize); | ||
|
|
||
| await file.read(compressed, 0, compressed.length, dataOffset); | ||
| return compressed; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Node.js FileHandle.read can return fewer bytes than requested partial read
💡 Result:
Yes, Node.js FileHandle.read can return fewer bytes than requested [1][2]. This is expected behavior under several conditions, most notably when the end-of-file (EOF) is reached [1][3]. Key details regarding partial reads include: 1. End-of-File (EOF): If the number of bytes available in the file (starting from the specified position) is less than the requested length, the operation will return only the available bytes [1][2]. The operation successfully completes, and the returned object includes a bytesRead property indicating the actual number of bytes read [1][2]. If no more data can be read because the EOF has already been reached, bytesRead will be zero [1][2]. 2. Concurrent File Modification: If the file is modified concurrently by another process or operation, the amount of data available to read may change, potentially resulting in fewer bytes than initially expected [1][3]. 3. Implementation and API Nuances: Historically, there have been bugs where specific overloads of FileHandle.read incorrectly handled length or options, leading to unexpected partial or zero-byte reads [4][5]. For instance, older versions of Node.js had issues where certain method signatures would ignore length parameters or fail to default correctly when options were omitted [6][4][5]. These issues have been addressed in recent Node.js releases [4][5]. To handle this correctly in your application, you should always check the bytesRead property of the returned object rather than assuming the entire requested buffer was filled [1][2]. If your application requires a specific number of bytes, you may need to implement a loop that continues reading from the file at the updated position until the desired amount of data is retrieved [1][2].
Citations:
- 1: https://nodejs.org/api/fs.html
- 2: https://nodejs.org/dist/latest/docs/api/fs.html
- 3: https://nodejs.org/docs/latest-v23.x/api/fs.html
- 4: Two types of
filehandle.readreturn different values nodejs/node#41558 - 5: filehandle.read(buffer) can't read file when options are omitted nodejs/node#47183
- 6: fs: fix
lengthoption being ignored duringread()nodejs/node#40906
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and read the surrounding sections.
ast-grep outline src/facade/archive.ts --view expanded || true
echo '--- lines 1-180 ---'
sed -n '1,180p' src/facade/archive.ts
echo '--- lines 480-560 ---'
sed -n '480,560p' src/facade/archive.ts
echo '--- search for file.read usage ---'
rg -n "file\.read\(" src/facade/archive.ts src -g '*.ts' || true
echo '--- search for readExact / similar helpers ---'
rg -n "readExact|bytesRead|inflateRaw|compressedSize|relativeOffsetOfLocalHeader" src/facade/archive.ts src -g '*.ts' || trueRepository: oomol-lab/wikigraph
Length of output: 10404
Read the requested bytes in a loop here. FileHandle.read() can return fewer bytes than requested, but both reads ignore bytesRead. On a truncated archive that leaves compressed zero-filled, which can silently corrupt stored entries and make deflated entries fail during inflate. A small readExact() helper would make this path robust.
🤖 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 `@src/facade/archive.ts` around lines 513 - 532, The byte reads in
readCompressedArchiveEntryBuffer ignore bytesRead, so truncated archives can
silently return zero-filled buffers and corrupt entry data. Update this helper
to read the local header and compressed payload in a loop until the requested
byte count is satisfied, or throw on EOF, and use a small readExact-style helper
inside readCompressedArchiveEntryBuffer to make the behavior robust.
| await acquireSqliteLease({ | ||
| archiveKey: this.#archiveKey, | ||
| entryPath: DATABASE_ENTRY_PATH, | ||
| ownerId: this.#session?.ownerId ?? this.#sqliteLeaseOwnerId, | ||
| ownerId: this.#sqliteLeaseOwnerId, | ||
| }); | ||
| this.#session?.observeDirtyEntry(DATABASE_ENTRY_PATH); | ||
| if (!this.#readonlyDatabase) { | ||
| this.#session?.observeDirtyEntry(DATABASE_ENTRY_PATH); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Refresh the entry-source cache after SQLite materialization.
If #entrySourceByPath was initialized before resolveDatabasePath() creates or replaces the database.db workspace overlay, the cache can still mark database.db as archive-backed. Later reads/listing from the same file store may return stale archive bytes instead of the materialized workspace database.
🛠 Proposed fix
await acquireSqliteLease({
archiveKey: this.#archiveKey,
entryPath: DATABASE_ENTRY_PATH,
ownerId: this.#sqliteLeaseOwnerId,
});
+ this.#entrySourceByPath?.set(DATABASE_ENTRY_PATH, {
+ kind: "workspace",
+ path: overlay.workspacePath,
+ });
if (!this.#readonlyDatabase) {
this.#session?.observeDirtyEntry(DATABASE_ENTRY_PATH);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await acquireSqliteLease({ | |
| archiveKey: this.#archiveKey, | |
| entryPath: DATABASE_ENTRY_PATH, | |
| ownerId: this.#session?.ownerId ?? this.#sqliteLeaseOwnerId, | |
| ownerId: this.#sqliteLeaseOwnerId, | |
| }); | |
| this.#session?.observeDirtyEntry(DATABASE_ENTRY_PATH); | |
| if (!this.#readonlyDatabase) { | |
| this.#session?.observeDirtyEntry(DATABASE_ENTRY_PATH); | |
| } | |
| await acquireSqliteLease({ | |
| archiveKey: this.#archiveKey, | |
| entryPath: DATABASE_ENTRY_PATH, | |
| ownerId: this.#sqliteLeaseOwnerId, | |
| }); | |
| this.#entrySourceByPath?.set(DATABASE_ENTRY_PATH, { | |
| kind: "workspace", | |
| path: overlay.workspacePath, | |
| }); | |
| if (!this.#readonlyDatabase) { | |
| this.#session?.observeDirtyEntry(DATABASE_ENTRY_PATH); | |
| } |
🤖 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 `@src/facade/wikg-coordinator.ts` around lines 553 - 560, Refresh the
entry-source cache after the SQLite materialization path runs so database.db is
no longer treated as archive-backed after the workspace overlay is created or
replaced. Update the cache in the code path around acquireSqliteLease and
observeDirtyEntry, using the relevant state in
WikgCoordinator/#entrySourceByPath so later reads and listing use the
materialized workspace file instead of stale archive bytes.
| function lockPathsConflict(requested: string, existing: string): boolean { | ||
| return ( | ||
| requested === existing || | ||
| lockPathContains(requested, existing) || | ||
| lockPathContains(existing, requested) | ||
| ); | ||
| } | ||
|
|
||
| function lockPathContains(parent: string, child: string): boolean { | ||
| return parent.endsWith("/") && child.startsWith(parent); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make the root prefix lock conflict with all entries.
normalizeEntryDirectoryPrefix("") returns "", but lockPathContains("", "foo") is false, so listFileContents("") does not conflict with writes under the archive root. That leaves root batched reads unprotected.
🔒 Proposed fix
function lockPathContains(parent: string, child: string): boolean {
- return parent.endsWith("/") && child.startsWith(parent);
+ return parent === "" || (parent.endsWith("/") && child.startsWith(parent));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function lockPathsConflict(requested: string, existing: string): boolean { | |
| return ( | |
| requested === existing || | |
| lockPathContains(requested, existing) || | |
| lockPathContains(existing, requested) | |
| ); | |
| } | |
| function lockPathContains(parent: string, child: string): boolean { | |
| return parent.endsWith("/") && child.startsWith(parent); | |
| } | |
| function lockPathsConflict(requested: string, existing: string): boolean { | |
| return ( | |
| requested === existing || | |
| lockPathContains(requested, existing) || | |
| lockPathContains(existing, requested) | |
| ); | |
| } | |
| function lockPathContains(parent: string, child: string): boolean { | |
| return parent === "" || (parent.endsWith("/") && child.startsWith(parent)); | |
| } |
🤖 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 `@src/facade/wikg-coordinator.ts` around lines 1042 - 1052, The root prefix is
not treated as a conflict in lock path matching, so empty-directory reads can
bypass write protection. Update lock handling in lockPathsConflict and/or
lockPathContains so that a normalized empty prefix from
normalizeEntryDirectoryPrefix("") conflicts with every non-empty path, ensuring
listFileContents("") blocks concurrent writes under the archive root. Use the
existing lockPathsConflict and lockPathContains helpers to apply the root-prefix
special case consistently.
1 participant
Summary
--allfor paged archivesearch/list, with--all --jsonlstreaming all result pages for agent-friendly exports.Notes
This also includes the earlier chapter readiness output cleanup commit so agents can list chapters and filter missing artifacts directly from
chapter list --all --jsonl.Verification
pnpm lintpnpm typecheckpnpm format:checkpnpm vitest run test/cli/args.test.ts test/cli/archive.test.ts test/facade/archive-view.test.ts test/cli/archive-chapter.test.tspnpm dev wkg://../.spinedigest/out/主义主义-哲学意识形态大全.wikg/chapter list --all --limit 2 --jsonl | head -n 6