feat(core): ingest docx, pptx, and csv into document sidecar notes - #1529
feat(core): ingest docx, pptx, and csv into document sidecar notes#1529phernandez wants to merge 10 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Closes #1512. Documents other than PDF now get the same sidecar Markdown note a PDF gets (`report.docx.md` next to `report.docx`), through the existing parser-neutral document contract. - Add `ExtractedDocument` and a `DocumentExtractor` protocol; the new `RawDocumentRuntime` dispatches on the indexed media type. The PDF runtime and `build_raw_document_artifacts` keep their signatures and delegate, so Cloud's wiring is unchanged. - Extract the killable, byte-capped child-process mechanics from the pdf-inspector adapter into `bounded_process.py` and the rlimit helpers into `worker_limits.py`; both workers use them. - Add a markitdown worker that calls `DocxConverter` / `PptxConverter` directly. The sniffing `MarkItDown.convert()` loop is never used, so magika, URL converters, zip recursion, and the plain-text and ASCII-decode failures seen on real files are out of the picture. Picture links are dropped, keeping alt text only when descriptive. - Render CSV with the stdlib as a bounded preview (header, first N rows, row count) with strict UTF-8 decoding instead of a charset guess. - Add a local project-directory runtime (API-backed source resolver, checksum-as-generation reader, sidecar + run-note writer that refuses to overwrite hand-written or enriched notes) and `bm import document`. - Ship markitdown[docx,pptx] behind a `basic-memory[documents]` extra. Verified end to end in an isolated home: docx, pptx, csv, and an 87-page PDF each produced an indexed, searchable sidecar and a run note; a second run reported both as already current. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VPRRyMREwCHSLnY3TCBr7s Signed-off-by: phernandez <paul@basicmachines.co>
680064d to
1e055ab
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 680064d40b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- `bm import document` pins local API routing. The runtime reads the source and writes the sidecar in the local project directory, so a cloud-configured project must not route its index calls to a server that cannot see those files. - The local writer verifies an existing run note against the accepted sidecar checksum and rewrites it when the two disagree, so overlapping imports of the same source converge instead of freezing a mismatch. - CSV cells escape backslashes before pipes so `a\|b` survives the Markdown table round trip. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VPRRyMREwCHSLnY3TCBr7s Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58b425a6b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A raw sidecar whose source changed is rebuilt only while it is still the projection an earlier run wrote. The writer looks up that run's note, compares the recorded checksum with the note on disk (allowing only the permalink the indexer adds), and refuses with a conflict when the bytes differ, when the run note is missing, or when the note changes between the check and the replacing write. Note content is canonical; a raw note a person has annotated is never replaced silently. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VPRRyMREwCHSLnY3TCBr7s Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebd87bfc3c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Every checksum the local document writer records or compares is now the checksum of the raw projection as its run wrote it: the sidecar re-assembled without the permalink the indexer appends. An unchanged re-import therefore leaves the run note alone, and a later source change still recognizes the annotated sidecar as untouched and rebuilds it. This also makes the comparison independent of the platform newline convention the atomic writer uses, which is what failed the Windows unit job. CSV cells flatten CRLF and lone CR as well as LF, so a quoted multiline field cannot split a table row once note assembly normalizes carriage returns. The reader raises the process-wide field size limit to the configured `max_field_bytes` (never lowering it) and reports parser failures as `CsvExtractionError` instead of a bare `_csv.Error`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VPRRyMREwCHSLnY3TCBr7s Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review |
|
I reproduced one remaining Windows failure on f904444 locally by making the atomic writer persist CRLF bytes: test_writer_creates_the_sidecar_and_run_note_and_indexes_both still expects document_db_checksum to match the physical file, but the new return value is the normalized projection checksum. I am preparing a small follow-up that carries physical and projection checksums separately in a frozen AcceptedDocumentNote, preserving the run normalization while retaining the file checksum contract. The regression will exercise LF and CRLF creation, reuse, and refresh on every host. |
Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review |
Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a8ad7192f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a865e179cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review |
|
Heads-up for anyone else working this branch: I am taking the two open P1 threads (checksum-conditioned replacement primitive in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af3ffeabbd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review latest head 7245d5c. Source move/recreation now fails before writing when document ownership conflicts; real SQLite and Postgres regressions pass. Missing originating-run recovery intentionally remains fail-closed as documented and explained in the resolved thread. Concurrent acceptance races are maintainer-deferred to #1530. Please check remaining in-scope behavior. |
|
Standing down on the two P1 threads: they were resolved as deferred to #1530 while I was implementing, and that is a maintainer scope call. A candidate implementation (advisory note lock plus source-generation fence under it, with interleaving tests) is parked on |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7245d5c988
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review latest head 134abc8. Both post-index checksum and YAML-comment preservation findings are fixed using the complete persisted post-index text. Please check the remaining in-scope behavior. The maintainer-deferred concurrency races remain in #1530, and ambiguous/missing provenance still requires explicit recovery. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 134abc84c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review current head 134abc8. Replied to the provenance-field finding with the existing schema invariants and regression evidence: independently altered output IDs/paths are rejected during parsing. Coordinated manual ledger rewriting is outside this release; please avoid duplicating the schema validator and identify a normal importer path if any in-scope issue remains. Concurrency remains deferred to #1530. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 134abc84c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) from error | ||
| output = run.frontmatter.output | ||
| recorded_checksum = output.raw.checksum if output and output.raw else None | ||
| if recorded_checksum != document_markdown_checksum(markdown): |
There was a problem hiding this comment.
When an editor changes only the sidecar's newline convention, FileService.read_file_content() performs universal-newline translation before this checksum, so CRLF and LF files compare equal. If the source later changes, the normal importer refresh path overwrites the sidecar without reporting an edit, silently discarding the user's physical byte representation even though Markdown bytes are canonical state. Fresh evidence beyond the resolved YAML-formatting case is the newline-normalized text read feeding this comparison; store or compare the physical checksum when authorizing replacement.
AGENTS.md reference: AGENTS.md:L156-L160
Useful? React with 👍 / 👎.
| header = next(reader, None) | ||
| if header is None: |
There was a problem hiding this comment.
When a valid CSV begins with a blank record, csv.reader returns [] here, so the renderer fixes the table width at zero and _table_row() slices every subsequent record to zero cells. For example, \nname,age\nAda,37\n produces a sidecar containing only empty rows and loses all previewed values; skip leading empty records or reject a zero-width header instead of rendering the remainder with width zero.
Useful? React with 👍 / 👎.
Closes #1512.
Why
Word, PowerPoint, and CSV files now get the same sidecar Markdown note a PDF gets (
report.docx.mdnext toreport.docx), through the existing parser-neutral document contract. PDF stays on pdf-inspector; the evaluation in #1512 shows why markitdown's PDF path is not a replacement (5.5s vs 0.36s on an 87-page paper, glued words, no page markers).What Changed
ExtractedDocument+DocumentExtractorprotocol inraw_document.py;RawDocumentRuntimedispatches on the indexed media type.RawPdfDocumentRuntimeandbuild_raw_document_artifactskeep their signatures and delegate, so Cloud's wiring is untouched. Tags anddocument.kindcome from the extractor instead of being hardcoded topdf.bounded_process.py; the rlimit helpers move intoworker_limits.py. Both workers use them.markitdown_extractor.py/markitdown_worker.pycallDocxConverter/PptxConverterdirectly. The sniffingMarkItDown.convert()loop is never used, so magika, the URL converters, zip recursion, and the plain-text / ASCII-decode failures seen on real files are out of the picture. Picture links are dropped (alt text kept when it is descriptive), so no danglingor data URIs land in notes. Slides fill the page diagnostics for pptx.local_runtime.pyresolves the indexed source through the API, uses the file checksum as the generation marker, writes the sidecar anddocument-ingestion-runs/<run-id>.md, indexes both, no-ops on an identical raw projection, and refuses to overwrite a hand-written or enriched note at the sidecar path.bm import document <path> [--project]. PDF works through the same command, which closes the localbm import pdfgap from Parse PDFs into searchable LiteParse sidecar notes #1006.basic-memory[documents]=markitdown[docx,pptx](also in the dev group). New install weight is roughly lxml, Pillow, mammoth, python-pptx; onnxruntime and numpy are already present via fastembed.Implementation Details
The local CLI forces local routing because the source and sidecar live in the local project directory. Raw sidecars are checked against their originating run before reuse or replacement; edited or enriched content is refused. This prevents an unchanged import from recording user annotations as generated content and authorizing their loss on a later refresh.
AcceptedDocumentNotecarries separate physical-file and normalized-projection checksums. Filesystem checks retain the actual LF/CRLF bytes. Run provenance hashes the complete persisted post-index text with normalized native line endings, including the indexed permalink. It does not parse/reassemble YAML, so authored comments and formatting remain significant. The physical checksum is also read after indexing, consistently across creation, reuse and refresh. CSV previews flatten CRLF, CR and LF, support large fields through a raised parser ceiling, enforce per-import UTF-8 field byte limits on headers and all rows, and report parser errors asCsvExtractionError. Local source reads and generation rechecks allocate at most the reader ceiling plus one byte (25 MiB by default); extractors retain their stricter format-specific limits.Generated document entities now derive their initial UUID from the source identity during indexing, so run-ledger references resolve to the actual sidecar. Existing entities retain their IDs. The local writer refuses imports when that derived identity is already indexed at another sidecar path, or an existing sidecar belongs to a different source entity. This preserves canonical bytes when a source is moved alone or deleted and recreated; automatic identity migration is outside this PR.
Testing
Final provenance follow-up: 138 focused tests passed; 3 real indexing/identity integration cases passed on SQLite and Postgres;
just fast-checkpassed. YAML-comment preservation and the post-index physical checksum failed before the fix. The SQLite integration additionally proves real creation/reuse/refresh.Final ownership follow-up: 137 document-ingestion/CLI/identity tests passed on SQLite; all 3 real identity integration cases passed on Postgres;
just fast-checkpassed.just fast-check: passed, including typecheck.uv run pytest tests/markdown tests/document_ingestion tests/cli/test_import_document.py test-int/test_local_document_identity.py --no-cov -q: 463 passed.BASIC_MEMORY_TEST_POSTGRES=1 uv run pytest test-int/test_local_document_identity.py --no-cov -q: 1 passed; proves the ledger ID resolves through the real indexing/API path.uv run pytest tests/document_ingestion tests/cli/test_import_document.py --no-cov -q: 132 passed on the final checksum follow-up, including LF/CRLF creation, reuse and refresh; annotation preservation; real mdformat reuse; per-import CSV field bounds; and bounded local source reads.just doctor: passed.just package-check: passed.A coverage-enabled local rerun hit a Pydantic
BasicMemoryConfigfixture initialization error; the same focused suite passes without coverage. Hosted CI covers the supported platform matrix.Original implementation validation: 114 tests across
tests/document_ingestionandtests/cli/test_import_document.py, 100% line coverage on every touched module indocument_ingestion/, including the real markitdown worker on generated docx/pptx and the CLI wiring.Every unit-test directory, the top-level unit files,
test-int(excluding benchmark/slow/live), andpluginspass when run per directory.End to end in an isolated home: docx, pptx, csv, and an 87-page PDF each produced an indexed sidecar and run note, found by
bm tool search-notes; a second run reported both notes as already current.Risks / Follow-ups
Maintainer-approved follow-up: #1530 tracks concurrent sidecar edits/creation and stale-import acceptance. Current checksum and generation checks are preflight checks, not atomic compare-and-swap. These two races are deferred; ordinary source-change refresh remains enabled.
A missing or mismatched originating run note now requires explicit recovery (for example, moving the sidecar aside before importing). Rewriting that checksum from current content cannot distinguish an interrupted import from user edits, so reuse refuses instead of silently accepting ambiguous provenance.
The run note records the complete accepted post-index text checksum with normalized native line endings. It may differ from the physical file checksum for CRLF files; YAML representation and the indexed permalink are included, not discarded.
docx form-style layout tables lose structure in markitdown's output; xlsx is deferred until there is a header strategy for title rows.
Only tested locally on macOS; Windows falls back to the parent's wall-clock deadline as the only worker ceiling, same as pdf-inspector.
🤖 Generated with Claude Code
https://claude.ai/code/session_01VPRRyMREwCHSLnY3TCBr7s