feat(access): ingest PDF, DOCX and the raw/_inbox doorway - #7
Merged
Conversation
Closes plan tasks 3.3, 3.4 and 3.7. 3.3 — A PDF's text is extracted per page and written under a `## p<N>` heading, so the fragment of `src://<id>#p12` is also the markdown anchor of the heading it points at. A page with no extractable text keeps its anchor: dropping it would shift every page number after it. pdfjs-dist rather than the pdf-parse this repo's stack.md had pencilled in — pdf-parse wraps an old fork of the same engine and returns the document as one string, so the page boundary, the only thing a citation needs, has to be recovered through a render hook. 3.4 — A DOCX's text and heading hierarchy, read with mammoth. Its own markdown writer is deprecated and escapes ordinary prose (`in two\.`), so this converts mammoth's HTML subset itself. No page anchor: the format records no pagination, and a synthetic `p<N>` would be a number that looks like provenance and points nowhere. That leaves a real gap against the shipped 5.4, which accepts only `p<N>` — recorded on the plan's 3.4 line rather than left implied by a comment. 3.7 — `raw/_inbox/` is scaffolded, gitignored and drained through the same path as an upload. It is not a source: nothing enumerates it, cites it, or reports it uncited. A file that cannot be ingested stays where it is with the reason reported — it is the user's only copy. Also from the two reviews on this branch: - The watcher ingests the file an event named, not the whole directory. chokidar's awaitWriteFinish stabilises one path; re-reading everything on that event picked up a neighbour still mid-copy, froze half of it as an immutable source and deleted the original. It also applies no stability check at all before `ready`, so the guarantee is ours: each file is observed twice before it is read. - The doorway is confined against the project, not against `raw/`, and registerSource confines before it creates anything. Rooting the assertion at `raw/` asserted nothing about `raw/` itself, so a symlink there landed bytes outside the project and still reported failure. - Size ceilings on both readers. A 551 KB DOCX declaring 166 MB of XML exhausted the heap, which V8 aborts rather than throws, so no try/catch downstream contained it — and the file survived to kill the next start too. The zip's declared sizes are checked before anything inflates. - The managed .gitignore block is rewritten rather than skipped, so a rule added later reaches a project scaffolded earlier. Opting in moves to a negation below the closing marker, which git honours and the tool never rewrites — something the file can state, unlike an edit inside the block that nothing could tell from a mistake. - chokidar's `error` event is handled. Unhandled, an EventEmitter error throws and takes the host process with it. - Table cells, multi-paragraph list items, nested ordered-list indents and emphasis runs carrying an edge space are all handled; the tests that covered them asserted markup mammoth never emits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016iMM93Wk43o44V5J2AxPgL
📝 WalkthroughWalkthroughThe access package adds PDF and DOCX ingestion, unified upload dispatch, and a ChangesSource ingestion
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ingestSource
participant uploadPdfSource
participant extractPdfPages
participant registerSource
Client->>ingestSource: submit PDF buffer
ingestSource->>uploadPdfSource: dispatch recognized format
uploadPdfSource->>extractPdfPages: extract page text
extractPdfPages-->>uploadPdfSource: return PdfPage[]
uploadPdfSource->>registerSource: persist original and text.md
registerSource-->>Client: return source id and page count
sequenceDiagram
participant FileSystem
participant watchInbox
participant drainInbox
participant ingestInboxEntry
FileSystem->>watchInbox: emit stable-file event
watchInbox->>drainInbox: queue serialized drain
drainInbox->>ingestInboxEntry: validate and ingest entry
ingestInboxEntry-->>drainInbox: return InboxOutcome
drainInbox-->>watchInbox: remove success or retain refusal
watchInbox-->>FileSystem: report outcome or error
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…e symlink privilege The repo already established this in paths.spec.ts: creating a symlink is a privilege a Windows account may not have, and that failure is a different thing from the containment behaviour under test. Windows is the only platform this product supports, so the suite has to run there for a developer who is not elevated. Deleting a directory chokidar is watching is the same kind of platform difference, and is guarded the same way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016iMM93Wk43o44V5J2AxPgL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
packages/access/src/ignore.ts (1)
21-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBuild the inbox rule from the
INBOXconstant.
manifest.tscentralizesINBOXbecause a rename must not miss one copy of the string. This rule is another copy, and it is the quietest one: a rename would leave the doorway git-visible with no compile error.♻️ Proposed refactor
+import { INBOX } from "./sources/manifest.js"; + const BODY = [ ... - "raw/_inbox/", + `raw/${INBOX}/`,🤖 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/access/src/ignore.ts` around lines 21 - 24, Update the ignore rule in the exported ignore configuration to derive the inbox path from the centralized INBOX constant in manifest.ts rather than duplicating the raw string. Preserve the existing trailing-slash ignore behavior and explanatory comments.packages/access/src/sources/docx.ts (1)
358-367: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winA failure in
writeSourceTextleaves a registered source with notext.md.
registerSourcecreatesraw/<id>/, writessource.docx, and writesmanifest.json. IfwriteSourceTextthen throws, the id is taken and the source has no extracted text. A retry of the same filename fails withTakenIdError, so the user cannot recover without deleting the directory by hand. The realistic trigger is a disk or permission error, not a malformed document, because extraction already completed.
uploadPdfSourceinpackages/access/src/sources/pdf.tsfollows the same sequence and carries the same gap. Consider a shared helper that removesraw/<id>/when the text write fails.♻️ Proposed cleanup on failure
export async function uploadDocxSource( projectRoot: string, name: string, content: Buffer, ): Promise<{ id: string }> { const markdown = await extractDocxMarkdown(content); const { id } = registerSource(projectRoot, { name, kind: "file", content }); - writeSourceText(projectRoot, id, markdown); + try { + writeSourceText(projectRoot, id, markdown); + } catch (err) { + // The id is frozen the moment the directory exists. A source with no + // `text.md` is not a source, so leave nothing behind to block a retry. + rmSync(join(projectRoot, "raw", id), { recursive: true, force: true }); + throw err; + } return { id }; }Add the imports:
+import { rmSync } from "node:fs"; +import { join } from "node:path"; import { registerSource } from "./register.js";🤖 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/access/src/sources/docx.ts` around lines 358 - 367, Update uploadDocxSource and the analogous uploadPdfSource flow so a failure in writeSourceText removes the newly registered raw/<id> directory before rethrowing the error. Prefer a shared cleanup helper for both sources, preserving successful uploads and allowing retries after disk or permission failures.packages/access/types/mammoth.d.ts (1)
22-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow the declaration to the API used by
sources/docx.ts.Remove the unused
extractRawTextdeclarations. Keepexport default mammoth;mammoth@1.12.0is CommonJS, so dynamic import exposes its exports through.default.Proposed narrowing
export function convertToHtml(input: ConvertInput): Promise<ConvertResult>; - export function extractRawText(input: ConvertInput): Promise<ConvertResult>; const mammoth: { convertToHtml: typeof convertToHtml; - extractRawText: typeof extractRawText; }; export default mammoth;🤖 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/access/types/mammoth.d.ts` around lines 22 - 29, In the mammoth module declaration, remove the extractRawText function declaration and its corresponding property from the default mammoth object. Keep convertToHtml and preserve the export default mammoth shape so the CommonJS dynamic-import contract remains represented through .default.packages/access/src/sources/pdf.ts (1)
129-138: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDrop the redundant buffer copy.
Bufferis already aUint8Array, andextractPdfPagescopies the bytes again at Line 81 before handing them to pdfjs. The copy at Line 134 therefore holds a third full image of the file in memory. At the 64 MiB ceiling this is 128 MiB of avoidable allocation per upload.♻️ Proposed refactor
- const pages = await extractPdfPages(new Uint8Array(content)); + const pages = await extractPdfPages(content);🤖 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/access/src/sources/pdf.ts` around lines 129 - 138, Update uploadPdfSource to pass the existing content Buffer directly to extractPdfPages instead of wrapping it in a new Uint8Array; preserve the remaining source registration, text writing, and return behavior unchanged.packages/access/src/sources/upload.ts (1)
44-48: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse one basename rule for recognition and id derivation.
extensionOfsplits on/and\.basenamefromnode:pathon POSIX does not treat\as a separator. If a Windows-style path reaches this function on a POSIX host (a test, or a CLI invoked with a path copied from Windows),formatresolves correctly butnamekeeps the directory components, andderiveIdproducesc-users-u-report.pdf. This is the same class of mismatch the comment on Lines 86-90 describes.Extract the basename once with the same separator rule and pass it to both.
♻️ Proposed refactor
-/** The lowercased extension of a filename, `.pdf`, or `""` when it has none. */ -function extensionOf(name: string): string { - const base = name.split(/[\\/]/).pop() ?? name; - const dot = base.lastIndexOf("."); +/** The last path component, under either separator. */ +function baseNameOf(name: string): string { + return name.split(/[\\/]/).pop() ?? name; +} + +/** The lowercased extension of a filename, `.pdf`, or `""` when it has none. */ +function extensionOf(name: string): string { + const base = baseNameOf(name); + const dot = base.lastIndexOf("."); return dot > 0 ? base.slice(dot).toLowerCase() : ""; }- const name = basename(rawName); + const name = baseNameOf(rawName);Also applies to: 86-94
🤖 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/access/src/sources/upload.ts` around lines 44 - 48, Unify basename extraction for format recognition and ID derivation in the upload flow. Reuse the separator-aware basename computed by extensionOf—or extract it through a shared helper—and pass that same basename to deriveId, ensuring Windows-style paths do not retain directory components while preserving existing extension detection.
🤖 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/access/package.json`:
- Around line 18-20: Align the repository’s root Node engine requirement with
pdfjs-dist@6.2.108 by raising the minimum from >=22 to >=22.13.0, while
preserving the existing >=24 allowance. Update the root package.json engines
configuration rather than changing the dependency selection.
In `@packages/access/src/sources/docx.ts`:
- Around line 341-348: Update extractDocxMarkdown and the
declaredUncompressedSize ZIP64 handling so ZIP64 sentinel values produce a
distinct refusal message explaining that the document uses ZIP64, rather than
interpolating Infinity as a byte count; retain the existing size-limit message
for finite declared sizes and add coverage for both ZIP64 sentinel paths.
- Around line 31-32: Update the numeric-entity replacements in htmlToMarkdown to
validate decoded values before calling String.fromCodePoint, covering both
decimal and hexadecimal entities. Preserve valid conversions, but leave entities
with values outside the Unicode range (above 0x10FFFF) as their original literal
text instead of throwing.
In `@packages/access/src/sources/inbox.ts`:
- Around line 321-327: Update the initial readiness promise in watchInbox to
settle on both watcher “ready” and “error” events. Use a shared one-time cleanup
callback that removes both listeners before resolving, preventing a fatal scan
error from leaving the watchInbox flow hanging.
In `@packages/access/tests/fixtures/documents.ts`:
- Line 1: Set the project’s Node.js minimum version to >=22.2.0 in the
engines.node configuration and update CI’s node-version setting from the broad
22 range to enforce that floor. Ensure all relevant version declarations align
so packages and CI do not run with Node.js versions where zlib.crc32 is
unavailable.
In `@packages/access/tests/sources-inbox.spec.ts`:
- Around line 30-34: Update tempProject to resolve the temporary root with
realpathSync before returning it, and import realpathSync from node:fs. Ensure
assertions comparing inboxPath(root) use the resolved path consistently,
including the additional assertion around lines 51–53.
---
Nitpick comments:
In `@packages/access/src/ignore.ts`:
- Around line 21-24: Update the ignore rule in the exported ignore configuration
to derive the inbox path from the centralized INBOX constant in manifest.ts
rather than duplicating the raw string. Preserve the existing trailing-slash
ignore behavior and explanatory comments.
In `@packages/access/src/sources/docx.ts`:
- Around line 358-367: Update uploadDocxSource and the analogous uploadPdfSource
flow so a failure in writeSourceText removes the newly registered raw/<id>
directory before rethrowing the error. Prefer a shared cleanup helper for both
sources, preserving successful uploads and allowing retries after disk or
permission failures.
In `@packages/access/src/sources/pdf.ts`:
- Around line 129-138: Update uploadPdfSource to pass the existing content
Buffer directly to extractPdfPages instead of wrapping it in a new Uint8Array;
preserve the remaining source registration, text writing, and return behavior
unchanged.
In `@packages/access/src/sources/upload.ts`:
- Around line 44-48: Unify basename extraction for format recognition and ID
derivation in the upload flow. Reuse the separator-aware basename computed by
extensionOf—or extract it through a shared helper—and pass that same basename to
deriveId, ensuring Windows-style paths do not retain directory components while
preserving existing extension detection.
In `@packages/access/types/mammoth.d.ts`:
- Around line 22-29: In the mammoth module declaration, remove the
extractRawText function declaration and its corresponding property from the
default mammoth object. Keep convertToHtml and preserve the export default
mammoth shape so the CommonJS dynamic-import contract remains represented
through .default.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ac0edf93-6b06-403b-ae84-e06d061a5581
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
docs/stack.mdpackages/access/package.jsonpackages/access/src/ignore.tspackages/access/src/index.tspackages/access/src/scaffold.tspackages/access/src/sources/docx.tspackages/access/src/sources/id.tspackages/access/src/sources/inbox.tspackages/access/src/sources/manifest.tspackages/access/src/sources/pdf.tspackages/access/src/sources/register.tspackages/access/src/sources/upload.tspackages/access/tests/fixtures/documents.tspackages/access/tests/ignore.spec.tspackages/access/tests/sources-docx.spec.tspackages/access/tests/sources-inbox.spec.tspackages/access/tests/sources-pdf.spec.tspackages/access/tests/sources-upload.spec.tspackages/access/tsconfig.jsonpackages/access/types/mammoth.d.tsplans/open-wiki.md
| @@ -0,0 +1,182 @@ | |||
| import { crc32 } from "node:zlib"; | |||
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find the declared Node floor and the CI Node version.
set -euo pipefail
fd -H -t f 'package.json' -E node_modules --exec rg -n -A3 '"engines"' {} \; -x echo '--- {}'
fd -H -t f -e yml -e yaml . .github --exec rg -n 'node-version|setup-node' {} \;
fd -H -t f '.nvmrc|.tool-versions' --exec cat {} \;Repository: protonspy/open-wiki
Length of output: 238
🌐 Web query:
Which Node.js version added the zlib.crc32 API?
💡 Result:
The zlib.crc32 API was added to Node.js in versions v22.2.0 and v20.15.0 [1][2][3].
Citations:
- 1: https://nodejs.org/api/zlib.html
- 2: https://nodejs.org/docs/latest-v24.x/api/zlib.html
- 3: https://nodejs.org/download/release/v23.6.1/docs/api/zlib.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- package engines ---'
cat -n package.json | sed -n '1,18p'
printf '%s\n' '--- CI Node configuration ---'
if [ -d .github ]; then
rg -n -C2 'setup-node|node-version|NODE_VERSION' .github || true
fi
printf '%s\n' '--- repository Node version files ---'
for f in .nvmrc .tool-versions; do
if [ -f "$f" ]; then
printf '%s\n' "--- $f"
cat -n "$f"
fi
done
printf '%s\n' '--- crc32 fixture usage ---'
rg -n -C3 'crc32|documents' packages/access/tests/fixtures/documents.tsRepository: protonspy/open-wiki
Length of output: 2719
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json
from pathlib import Path
p = json.loads(Path("package.json").read_text())
print("engines:", p.get("engines"))
for path in sorted(Path(".github").rglob("*")) if Path(".github").exists() else []:
if path.is_file() and path.suffix in {".yml", ".yaml"}:
text = path.read_text(errors="replace")
if "setup-node" in text or "node-version" in text:
print(f"--- {path}")
for i, line in enumerate(text.splitlines(), 1):
if "setup-node" in line or "node-version" in line:
print(f"{i}: {line}")
PYRepository: protonspy/open-wiki
Length of output: 530
Set the Node.js floor to >=22.2.0.
engines.node: ">=22" and CI node-version: 22 permit Node.js versions before 22.2.0, where zlib.crc32 is unavailable and DOCX fixtures fail.
🤖 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/access/tests/fixtures/documents.ts` at line 1, Set the project’s
Node.js minimum version to >=22.2.0 in the engines.node configuration and update
CI’s node-version setting from the broad 22 range to enforce that floor. Ensure
all relevant version declarations align so packages and CI do not run with
Node.js versions where zlib.crc32 is unavailable.
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/access/tests/sources-inbox.spec.ts`:
- Around line 52-58: In packages/access/tests/sources-inbox.spec.ts:52-58,
update trySymlink to suppress only the expected Windows symlink-privilege error
and rethrow all other filesystem errors; in
packages/access/tests/sources-inbox.spec.ts:350-356, apply the same narrowing to
the cleanup catch so only the known watched-directory deletion error is ignored,
while unexpected errors propagate.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d27f1a1e-2376-4938-a76f-1a4cedb9dc90
📒 Files selected for processing (1)
packages/access/tests/sources-inbox.spec.ts
- `watchInbox` no longer hangs when chokidar never emits `ready`. The event is not guaranteed after a failure in the initial scan, and a bare await on it means the desktop application never finishes opening the project. It now settles on ready or error, whichever comes first, with a bounded fallback. - A ZIP64 DOCX is reported as ZIP64. It was refused correctly and then described to the user as declaring "Infinity bytes of content". - A numeric character reference above the Unicode range no longer aborts the conversion. `String.fromCodePoint` throws there, and `htmlToMarkdown` is exported, so the HTML need not have come from mammoth. Out-of-range and surrogate-half references stay literal. - `engines.node` is `>=22.13.0`: pdfjs-dist 6.2.108 requires it, and `zlib.crc32` — which the DOCX fixture builds on — landed in 22.2.0. - The inbox tests resolve their temp root. `inboxPath` returns a real path, and `os.tmpdir()` is itself a symlink on macOS, so the assertion compared two spellings of the same directory. - The two platform skips catch only the codes that mean "this platform cannot do that". Catching everything turned an unexpected setup failure into a passing test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016iMM93Wk43o44V5J2AxPgL
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/access/src/sources/docx.ts`:
- Around line 331-341: Update the ZIP metadata validation around the EOCD
parsing and central-directory loop to reject ZIP64 whenever either EOCD
entry-count field, central-directory-size field, or offset uses its sentinel,
and reject any central-directory entry whose compressed or uncompressed size is
ZIP64_SENTINEL. Preserve the existing archive-size limit behavior, and add
coverage for an EOCD entry-count sentinel that would otherwise omit a later
oversized entry.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d8f6b354-71fa-48b7-bcd3-82620fa50172
📒 Files selected for processing (6)
package.jsonpackages/access/src/sources/docx.tspackages/access/src/sources/inbox.tspackages/access/tests/fixtures/documents.tspackages/access/tests/sources-docx.spec.tspackages/access/tests/sources-inbox.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/access/tests/sources-docx.spec.ts
- packages/access/tests/fixtures/documents.ts
- packages/access/src/sources/inbox.ts
Comment on lines
+331
to
+341
| if (offset === ZIP64_SENTINEL) return ZIP64; | ||
|
|
||
| let total = 0; | ||
| for (let n = 0; n < entries; n++) { | ||
| if (offset + 46 > zip.length) return null; | ||
| if (zip.readUInt32LE(offset) !== CENTRAL_SIGNATURE) return null; | ||
| const size = zip.readUInt32LE(offset + 24); | ||
| // A ZIP64 entry hides its real size in an extra field. Rather than parse | ||
| // that, say so: a document that needs ZIP64 is not one this reads, and | ||
| // "declares Infinity bytes" is not a sentence to show anybody. | ||
| if (size === ZIP64_SENTINEL) return ZIP64; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Reject all ZIP64 EOCD sentinel fields.
A ZIP64 archive can set the EOCD entry count to 0xffff while keeping offset below 0xffffffff. The loop then reads only 65,535 entries and can omit a later oversized entry from total. This bypasses the 64 MiB pre-extraction limit before Mammoth reads the archive.
Reject ZIP64 when any EOCD count or central-directory-size field uses its sentinel. Also reject a central-directory entry when its compressed or uncompressed size uses the ZIP64 sentinel. Add a test with a ZIP64 entry-count sentinel and an oversized omitted entry.
Proposed fix
+ const entriesOnThisDisk = zip.readUInt16LE(eocd + 8);
const entries = zip.readUInt16LE(eocd + 10);
+ const centralDirectorySize = zip.readUInt32LE(eocd + 12);
let offset = zip.readUInt32LE(eocd + 16);
- if (offset === ZIP64_SENTINEL) return ZIP64;
+ if (
+ entriesOnThisDisk === 0xffff ||
+ entries === 0xffff ||
+ centralDirectorySize === ZIP64_SENTINEL ||
+ offset === ZIP64_SENTINEL
+ ) {
+ return ZIP64;
+ }
let total = 0;
for (let n = 0; n < entries; n++) {
if (offset + 46 > zip.length) return null;
if (zip.readUInt32LE(offset) !== CENTRAL_SIGNATURE) return null;
+ const compressedSize = zip.readUInt32LE(offset + 20);
const size = zip.readUInt32LE(offset + 24);
- if (size === ZIP64_SENTINEL) return ZIP64;
+ if (compressedSize === ZIP64_SENTINEL || size === ZIP64_SENTINEL) return ZIP64;🤖 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/access/src/sources/docx.ts` around lines 331 - 341, Update the ZIP
metadata validation around the EOCD parsing and central-directory loop to reject
ZIP64 whenever either EOCD entry-count field, central-directory-size field, or
offset uses its sentinel, and reject any central-directory entry whose
compressed or uncompressed size is ZIP64_SENTINEL. Preserve the existing
archive-size limit behavior, and add coverage for an EOCD entry-count sentinel
that would otherwise omit a later oversized entry.
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.
Closes plan tasks 3.3, 3.4 and 3.7 of
plans/open-wiki.md.What changed
3.3 — Upload a PDF. Text is extracted one page at a time and written under a
## p<N>heading, so the fragment ofsrc://<id>#p12is also the markdown anchor of the heading it points at. A page with no extractable text keeps its anchor — dropping it would shift every page number after it.3.4 — Upload a DOCX. Text and heading hierarchy, via mammoth. Its own markdown writer is deprecated and escapes ordinary prose (
split in two\.), so the adapter converts mammoth's HTML subset itself. No page anchor is written: a DOCX records no pagination, and a syntheticp<N>would be a number that looks like provenance and points nowhere.3.7 — The
raw/_inbox/doorway. Scaffolded, gitignored, and drained through the same path as an upload. It is not a source: nothing enumerates it, cites it, or reports it uncited. A file that cannot be ingested stays where it is with the reason reported — it is the user's only copy of it.A shared
ingestSourcedispatcher recognises the format and picks the adapter; 3.5 will call the same door.One thing to decide, not fixed here
A DOCX has no page anchor, but the already-shipped 5.4 accepts only
p<N>for asrc://citation — so a DOCX source is citable only assrc://<id>#p1, which resolves to the source but to no place inside itstext.md. That is a real gap between 3.4 and 5.4. It is now recorded on the plan's 3.4 line rather than left implied by a comment in one file, and deferred to group 7 with the rest of provenance. The MVP path that matters end to end is markdown and PDF.From the reviews on this branch
code-reviewandsecurity-reviewwere run on the diff before this PR was opened. Both found real defects; all are fixed here:awaitWriteFinishstabilises the path an event names, but the drain re-read the whole directory — so a small file's event pulled in a large one still mid-copy, froze half of it as an immutable source, and deleted the user's only copy. Now an event ingests the file it named. chokidar also applies no stability check at all before it emitsready, so the guarantee is ours: each file is observed twice before it is read.raw/, not the project. That asserted nothing aboutraw/itself, so a symlink standing there landed bytes outside the project and still reported failure.inboxPathandregisterSourceboth confine against the project now, before anything is created.try/catchdownstream contained it, and because the file only leaves the inbox on success it killed every subsequent start too. Size ceilings on both readers, and the zip's declared sizes are checked before anything inflates.scaffoldstill created the new directory — soraw/_inbox/appeared unignored in exactly the projects that already existed. The block is rewritten now; opting in moves to a negation below the closing marker, which git honours and the tool never rewrites.errorevent crashed the host process. A node EventEmitter that emitserrorwith no listener throws.inbox.tsmade git treat the most concurrency-sensitive file on the branch as binary, with no visible diff.How it was verified
pnpm test); access coverage 94.6% against the 76% floorpnpm run typecheckandpnpm lintcleanscc validate— 0 findingsNote on scope
mainis not prettier-clean — CI runstypecheckandlintbut notformat:check, so ~30 pre-existing files reformat on anypnpm format. That churn was deliberately kept out of this diff. Worth a separate housekeeping PR plus aformat:checkstep in CI.🤖 Generated with Claude Code
https://claude.ai/code/session_016iMM93Wk43o44V5J2AxPgL
Summary by CodeRabbit
New Features
Bug Fixes
Documentation