PR #1555 (format coverage + virtual line numbering) merged with twelve
inline polish comments from Copilot + gemini-code-assist that weren't
load-bearing enough to block the original ship but are real cleanups.
This PR addresses them.
Twelve items in scope; one item (drawer ID delimiter — Copilot #13) is
deferred to its own dedicated PR because it's a breaking schema change
that requires migration design beyond the scope of a polish PR.
## Behavioral fixes (5 items, RED-tested first)
1. **FileNotFoundError vs broken symlink (Copilot #8).** ``extract_text``
previously mapped every ``FileNotFoundError`` from ``stat()`` to
``SKIP_BROKEN_SYMLINK``. That's misleading for the common case of a
regular file deleted between scan and extract. Now distinguishes:
``SKIP_BROKEN_SYMLINK`` only when ``p.is_symlink()`` is true;
``SKIP_UNREADABLE`` otherwise.
2. **``file_already_mined`` extract_mode scoping (Copilot #11, #12).**
Both call sites in ``mine_formats`` and ``_file_chunks_locked`` now
pass ``extract_mode="format"``. Previously the format miner could
falsely treat drawers from project / convo miner on the same source
file as "already mined" (and vice versa). Scopes idempotency to the
correct drawer subset.
3. **Sentinel skip for transient missing-dep statuses (Copilot #14).**
New ``_TRANSIENT_MISSING_DEP_STATUSES`` set + ``_register_skip_sentinel_if_appropriate``
helper. Skip variants like ``SKIP_NO_MARKITDOWN`` /
``SKIP_NO_STRIPRTF`` / ``SKIP_MISSING_FORMAT_DEPS`` /
``SKIP_NETWORK_TIMEOUT`` no longer write the "already-mined" sentinel.
Otherwise installing the missing extra later wouldn't trigger a re-mine.
4. **Outer ``except Exception`` in ``mine_formats`` (Gemini #5).** The
outer try around the loop previously caught only ``KeyboardInterrupt``,
leaving any setup-time error (e.g., ``scan_formats`` raising) to
propagate as a bare traceback. Now catches ``Exception`` defensively,
logs it, prints a partial-progress summary, and lets the ``finally``
PID-cleanup run. Mirrors miner.py's belt-and-suspenders pattern.
5. **Thread user's ``chunk_size`` / ``chunk_overlap`` / ``min_chunk_size``
through to ``chunk_text`` (Gemini #3).** ``MempalaceConfig`` was loaded
only to validate readability; users who tuned their config saw no
effect in format-mode mining. Now properly threaded.
## Trivial cleanups (5 items)
6. **Path expanduser in ``extract_text`` (Copilot #7).** ``Path(path)`` →
``Path(path).expanduser()`` so CLI inputs like ``~/docs/file.pdf``
resolve correctly.
7. **Path expanduser+resolve in ``scan_formats`` (Copilot #9).** Same
fix; ``~/docs`` and relative paths now work consistently.
8. **Use resolved ``format_path`` in ``mine_formats`` (Copilot #10).**
``scan_formats(format_dir)`` → ``scan_formats(format_path)`` so the
already-resolved path is used.
9. **``render_with_line_numbers`` type annotation (Copilot #15).**
``text: "str | None"`` reflects the documented + tested ``None``
handling.
10. **Test + docs claims (Copilot #16, #17, #18).** Stale framings
removed:
- ``docs/format-coverage.md`` — 14 fringe cases + "see the file for
the current test inventory" (no more frozen test count).
- ``tests/test_line_numbers.py`` — drops "proposed for mempalace
3.3.6" + "run from the proposal directory" references.
- ``tests/test_format_miner.py`` — drops "MarkItDown is mocked
throughout" (live integration tests exist) + proposal-directory
framing.
## Module-level hoists (enables clean test patching)
- ``MempalaceConfig`` (from ``.config``) hoisted from lazy local import
to module-level so tests can patch ``mempalace.format_miner.MempalaceConfig``.
- ``chunk_text`` (from ``.miner``) hoisted similarly.
Both follow the pattern PR #1565 used for ``compute_hallways_for_wing``.
## Complexity refactor
Extracted ``_print_mine_summary`` from ``mine_formats`` so the orchestrator
stays under the project's ``max-complexity = 25`` ceiling (per
``pyproject.toml [tool.ruff.lint.mccabe]``). Behavior unchanged; pure
extraction.
## Out of scope (intentionally deferred)
- **Drawer ID delimiter collision (Copilot #13)** — ``f"{source_file}{chunk_index}"``
can theoretically collide (``"/path/a1" + "23"`` == ``"/path/a" + "123"``).
Fixing this is a breaking schema change to drawer IDs and requires a
migration plan; will land as its own PR after design.
- The four bot comments that were ALREADY addressed by amendment #3
before the PR #1555 merge (``_SKIP_DIRS`` dedup, ``scan_formats``
symlink skip, ``source_mtime`` tracking, hall+entities metadata) —
no action needed; verified during audit.
## Tests (RED-first)
Six new RED-first tests in ``tests/test_format_miner.py``:
test_extract_text_nonexistent_regular_file_returns_unreadable_not_broken_symlink
test_mine_formats_passes_extract_mode_format_to_file_already_mined
test_mine_formats_does_not_write_sentinel_for_skip_no_markitdown
test_mine_formats_does_not_write_sentinel_for_skip_missing_format_deps
test_mine_formats_catches_unexpected_exception_and_prints_summary
test_mine_formats_threads_chunk_size_from_user_config
All six RED before this commit (failures correctly identified the bugs
they're targeting), all six GREEN after.
One existing test (``test_mine_formats_continues_after_per_file_error``)
updated to patch the new module-level binding
``mempalace.format_miner.chunk_text`` instead of the old
``mempalace.miner.chunk_text`` source location, and to accept the
``**kwargs`` the call now passes through. Behavior unchanged.
## Verification
pytest -q (full mempalace suite)
→ 2065 passed, 3 skipped, 0 regressions
ruff check mempalace/format_miner.py mempalace/searcher.py tests/
→ All checks passed!
ruff format --check ...
→ 4 files already formatted (pinned 0.15.9)
mine_formats complexity
→ ≤ 25 (under the project ceiling)
Addresses PR #1555 review (Igor) — two bugs that block this PR from
merging cleanly:
1. ``mempalace[extract]`` extras did not pull MarkItDown's per-format
sub-dependencies. A real PDF after ``pip install mempalace[extract]``
would raise ``MissingDependencyException`` asking for ``markitdown[pdf]``
— the code then routed that exception through the generic
``except Exception`` and surfaced it as ``SKIP_EXTRACTION_ERROR``,
stripping the actionable signal from the user.
2. ``test_fringe_broken_symlink`` and ``test_scan_formats_skips_symlinks``
called ``Path.symlink_to()`` unguarded, which raises ``OSError``
(``WinError 1314``) on Windows test environments without
``SeCreateSymbolicLinkPrivilege`` — surfacing as hard test failures
before any product code ran.
## What this commit does
1. ``mempalace/format_miner.py`` — adds a new
``ExtractionStatus.SKIP_MISSING_FORMAT_DEPS`` enum member and a
matching catch in ``extract_text`` that fires BEFORE the generic
``except Exception`` block. The catch matches by exception type name
(``type(exc).__name__ == "MissingDependencyException"``) so the static
import surface doesn't change — MarkItDown stays an optional
dependency.
2. ``pyproject.toml`` — changes ``[extract]`` to include MarkItDown's
per-format sub-extras:
"markitdown[docx,pdf,pptx,xlsx]>=0.1.5; python_version >= '3.10'"
These pull ``pdfminer-six``, ``pdfplumber``, ``mammoth``, ``lxml``,
``python-pptx``, ``openpyxl``, ``pandas`` — the deps each per-format
converter actually needs at runtime. Verified against MarkItDown
0.1.5's PyPI metadata (Provides-Extra includes ``pdf``, ``docx``,
``pptx``, ``xlsx`` among others). ``.epub`` is handled by base
markitdown (``EpubConverter`` uses ``beautifulsoup4``, a base
requirement, so no ``[epub]`` extra is needed — and none exists in
0.1.5). ``.rtf`` is covered by the existing ``striprtf`` entry.
``markitdown[all]`` was NOT used because it pulls audio
(``pydub``, ``speechrecognition``), YouTube, and Azure deps that
mempalace does not claim support for and that would bloat the
install for users.
3. ``tests/test_format_miner.py`` — adds a ``_make_symlink_or_skip``
helper near the top of the file that wraps ``Path.symlink_to()`` in
try/except ``OSError`` and calls ``pytest.skip(...)`` on failure.
Refactors both existing symlink tests
(``test_fringe_broken_symlink``, ``test_scan_formats_skips_symlinks``)
to use the helper. Behavior is unchanged on macOS/Linux; on Windows
without symlink privileges the tests skip cleanly instead of
spuriously failing.
## Tests (RED-first)
Two new RED-first tests:
test_extract_text_missing_format_dep_returns_distinct_status
Patches ``_extract_via_markitdown`` to raise a fake exception
with ``__name__ == "MissingDependencyException"``. Asserts the
dispatcher returns ``SKIP_MISSING_FORMAT_DEPS``, not
``SKIP_EXTRACTION_ERROR``.
test_pyproject_extract_extra_pulls_markitdown_format_subdeps
Parses ``pyproject.toml`` (via ``tomllib`` 3.11+ or ``tomli`` on
3.9/3.10 — the latter is already a base dependency under the
``python_version < '3.11'`` marker). Asserts ``[extract]``
includes ``pdf``, ``docx``, ``pptx``, ``xlsx`` inside SOME
``markitdown[...]`` bracketed group.
Both RED before this commit. Both GREEN after.
Also updates ``test_extraction_status_enum_has_all_documented_codes`` to
include the new ``SKIP_MISSING_FORMAT_DEPS`` code in the documented set,
so the enum-completeness doc-test stays honest.
## Verification
pytest tests/test_format_miner.py::test_extract_text_missing_format_dep_returns_distinct_status
tests/test_format_miner.py::test_pyproject_extract_extra_pulls_markitdown_format_subdeps
→ 2 passed (RED before, GREEN after)
pytest tests/test_format_miner.py::test_fringe_broken_symlink
tests/test_format_miner.py::test_scan_formats_skips_symlinks
→ 2 passed on macOS (Windows behaviour validated by CI test-windows)
pytest tests/test_format_miner.py
→ 64 passed, 2 skipped, 0 regressions
pytest -q (full mempalace suite)
→ 2002 passed, 3 skipped, 0 regressions
ruff check mempalace/format_miner.py tests/test_format_miner.py
→ All checks passed!
ruff format --check ...
→ 2 files already formatted (pinned 0.15.9)
## Why no OrbStack run this time
OrbStack runs Linux containers, which have unprivileged symlinks. It
cannot reproduce the Windows ``WinError 1314`` failure mode this
amendment fixes. The only authoritative gate for the symlink fix is
GitHub CI's ``test-windows`` job. For the enum + extras changes, both
are pure-Python / install-time concerns the CI test-linux matrix covers
identically to OrbStack. The ``tomli`` fallback in the new pyproject
test was verified against the base dependency declaration in
``pyproject.toml:32``.
Third amendment to PR #1555. Closes the remaining parity gaps between
format_miner and the existing project miner that Aya's palace audit
surfaced after amendment #2.
## What was broken
A smoke test against the just-mined v6 palace confirmed:
WING: wing_aya
ROOM: documents 2904 drawers ← ALL in one room
Tunnels for wing_aya: 0 ← no cross-wing links
Cause: format_miner hardcoded `room = "documents"` for every drawer and
never called `_compute_topic_tunnels_for_wing()` post-mine. miner.py
calls `detect_room()` per drawer and computes tunnels after the loop.
format_miner did neither.
## What this commit changes
1. Module-level imports of `detect_room`, `load_config`, and
`_compute_topic_tunnels_for_wing` from `.miner`. Module-level (not
lazy) so test seams `patch("mempalace.format_miner.detect_room", ...)`
work — lazy imports inside a function don't expose attributes on the
module object.
2. `mine_formats()` loads the project's `mempalace.yaml` via
`load_config()` to get the rooms list. Falls back to a single
"documents" room if no config exists.
3. The hardcoded `room = "documents"` line is replaced with:
room = detect_room(filepath, text, rooms, format_path)
Mirrors miner.py:904 exactly — folder-match → filename-match →
content-keyword scoring → fallback "general".
4. After the per-file loop completes (in an `else` branch on the outer
try, so it does NOT run on KeyboardInterrupt), call
`_compute_topic_tunnels_for_wing(wing)` in try/except. Exact mirror
of miner.py:1241-1249. Tunnel-compute failures must never fail a mine.
5. Import `sys` at module level (used by the tunnel-compute error path
for `print(..., file=sys.stderr)`).
## Tests
5 new RED-first tests in `tests/test_format_miner.py`:
- test_mine_formats_calls_load_config_for_rooms
- test_mine_formats_calls_detect_room_per_file
- test_mine_formats_uses_detected_room_in_drawer_metadata
- test_mine_formats_calls_compute_topic_tunnels_after_loop
- test_mine_formats_tunnel_failure_does_not_crash_mine
All 5 failed against pre-commit code with AttributeError on missing
module attributes (proof the bug existed). All 5 pass after the
implementation. Total format_miner test count: 62. Total proposal test
count (format_miner + line_numbers): 83. Full mempalace test suite:
2002 passed, 1 skipped (no regressions).
## Verification
ruff check mempalace/format_miner.py tests/test_format_miner.py
→ All checks passed!
ruff format --check ...
→ already formatted (pinned ruff 0.15.9)
pytest tests/test_format_miner.py tests/test_line_numbers.py
→ 83 passed
pytest -q (full suite)
→ 2002 passed, 1 skipped
## Not in this commit (deferred)
Within-wing hallway primitives are a separate architectural addition
(separate PR — being designed). The current `_compute_topic_tunnels_for_wing`
matches miner.py exactly but inherits miner.py's gap: it computes
tunnels from raw topic words rather than from hallway primitives. That
refactor lands in its own PR after the hallway primitive ships.
Six follow-up fixes addressing gemini-code-assist review on PR #1555.
All medium-priority parity gaps between format_miner.py and the
existing miner.py / convo_miner.py patterns.
1. Reuse palace.SKIP_DIRS instead of duplicating it locally.
Replaces the local _SKIP_DIRS set with the shared constant so the
format miner's directory-skip set stays in sync automatically.
2. scan_formats now explicitly skips symlinks. Prevents circular
links, following symlinks to /dev/urandom, and processing the same
file via multiple paths. Mirrors miner.py:1068 exactly.
3. mine_formats loads MempalaceConfig at start. The current chunker
(miner.chunk_text) uses module-level CHUNK_SIZE constants and
doesn't accept overrides, so the loaded config values aren't yet
threaded into chunking; the load is wired so future chunk_text
refactors that accept per-call sizing pick this up automatically.
Honest scope note: this gives format_miner the same config-load
stance as miner.py without overreaching what chunk_text supports.
4. source_mtime tracking + check_mtime=True. Each drawer's metadata
now carries source_mtime, and file_already_mined is called with
check_mtime=True so an edited PDF/DOCX is correctly re-mined on
the next pass. Mirrors miner.py semantics exactly. Previously,
updated documents would have been silently skipped.
5. Per-file try/except wrap + KeyboardInterrupt handling + PID
cleanup. One malformed file in a mine of thousands no longer
crashes the whole orchestrator — the error is logged, a counter
bumps, and the loop continues with the next file. KeyboardInterrupt
still produces a clean summary on Ctrl-C. _cleanup_mine_pid_file
is called in a finally block so hook-spawned mines don't leak a
stale PID file on exit.
6. hall + entities metadata on every drawer. Format-mined drawers
now carry the same hall (via miner.detect_hall) and entities (via
miner._extract_entities_for_metadata) tags as project-mined
drawers. Matches the search-result quality the other miners ship.
Tests: 5 new tests covering the new behaviors:
- test_scan_formats_skips_symlinks
- test_mine_formats_uses_check_mtime_true
- test_mine_formats_records_source_mtime_in_drawer_metadata
- test_mine_formats_records_hall_in_drawer_metadata
- test_mine_formats_continues_after_per_file_error
Total: 78 tests passing (was 73). Full mempalace test suite green
modulo one pre-existing flaky test (test_chroma_close_palace_..., a
sqlite-lock-timing flake unrelated to this change — passes 4/4 in
isolation). ruff check + ruff format clean on pinned 0.15.9.
Coverage on mempalace/format_miner.py: 84% (clears the 80% gate).
Refs: PR #1555 review comments from gemini-code-assist[bot]
Two additive features, both following the same read-time-transform pattern:
1. Virtual line numbering — new render_with_line_numbers() and
extract_line_range() in mempalace/searcher.py. Closet pointers like
2026-01-18:L55-L72 resolve to drawer slices rendered with [55] through
[72] line prefixes without modifying any stored content. Lines already
prefixed with [<digits>] pass through unchanged. Pure functions, no I/O.
2. Format coverage (mempalace mine --mode extract) — new mempalace/format_miner.py
reads binary office formats and files drawers via the lock + purge +
upsert pattern convo_miner uses. Source files never modified.
Per-format transformer routing: MarkItDown 0.1.5 does not actually
convert .rtf (returns raw control codes unchanged, verified live), so
.rtf is routed to striprtf which does convert. Other formats stay on
MarkItDown:
.pdf .docx .pptx .xlsx .epub -> MarkItDown
.rtf -> striprtf
13 fringe cases handled with dedicated ExtractionStatus codes:
SKIP_NO_MARKITDOWN, SKIP_NO_STRIPRTF, SKIP_TOO_LARGE, SKIP_CLOUD_ONLY,
SKIP_ENCRYPTED, SKIP_EMPTY, SKIP_PERMISSION, SKIP_BROKEN_SYMLINK,
SKIP_UNRECOGNIZED, SKIP_EXTRACTION_ERROR, SKIP_NETWORK_TIMEOUT,
SKIP_UNREADABLE, plus the encoding fallback handled internally.
Drawers carry ingest_mode=extract + extract_mode=format so they are
distinguishable from project / convo drawers in the palace.
Both transformers are optional extras: pip install mempalace[extract].
MarkItDown requires Python >= 3.10 (env marker ensures it only installs
where supported; Python 3.9 users still get RTF coverage via striprtf).
Tests: 94 new (21 line-numbering + 73 format-miner, including 9
mine_formats orchestrator tests). Full mempalace suite still green
(1992 passed locally). Coverage 86% on mempalace/format_miner.py.
ruff check + ruff format clean on the pinned 0.15.9.
Live verification: mempalace mine --mode extract on a directory with 2
RTFs + 1 PDF produced 90 drawers correctly; mempalace search found
content from both file types in the resulting wing.
Documented limits (out of scope for 3.3.6): custom PDF parsers, OCR on
scanned PDFs, DRM-locked files, pathological corrupt files. These get
reported via skip codes and skipped.
Refs: docs/format-coverage.md, docs/virtual-line-numbering.md
Igor Lins e Silva
mvalentsev
Milla J