From ba5a539136e79972969c94a2807f9f788135f548 Mon Sep 17 00:00:00 2001 From: mvalentsev Date: Thu, 13 Aug 2026 16:19:52 +0500 Subject: [PATCH] fix(ingest): finish the non-regular-file guards left out of 3.7.1 (#2221) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #2221 fix reached develop through #2228 at an earlier revision of the branch, so three guards and their tests did not come with it. One of the three is a regression the gate that did land introduced. sweep_directory: the new gate probes the file type with f.stat() inside a try, and its except OSError printed SKIP and continued. A dangling symlink, a symlink loop and a file unlinked between rglob and the gate all raise there. Before the gate existed each of them reached sweep(), raised, and was appended to failures — so the gate turned "could not read this transcript" into a silent skip and a successful exit. A probe that FAILS is an error, not a benign file type: log it, print WARNING, book it in failures. A probe that succeeds and reports a non-regular file still skips silently. _parse_gradle: the is_file() gate sat in front of the try whose except OSError the parser already had, so a manifest under a directory with r but no x raised PermissionError out of a call that used to answer "no manifest name". The gate moves inside that try. _collect_manifest_names stats with os.path.isfile, which reports instead of raising, matching the parsers it guards. split_file: the type gate in main() covers the files the glob listed, but split_file builds its output names itself, so a pre-existing FIFO at one of them wedged write_text in the kernel waiting for a reader. Output names that are anything but a regular file are skipped. That gate asks os.path.lexists, not os.path.exists. exists() follows the link, so a DANGLING symlink at an output name reads as "nothing there" and the write goes through it, creating the target — a chunk landing wherever the link points rather than in the output directory. Measured: the two calls differ on that one case and agree on every other (regular file, symlink to a file, missing name, FIFO, symlink to FIFO, directory). test_gather_origin_samples_survives_an_unreadable_directory broke under root rather than passing vacuously: CAP_DAC_OVERRIDE walks into the 0o444 directory, the walled-off file stays readable, and the count assertion sees two samples instead of one. It now carries the same needs_unprivileged_posix gate as the three new permission tests. miner._read_text_no_follow: comment fix only. F_SETLEASE on a FIFO fails EINVAL, not ENXIO — measured on Linux 6.18 / glibc 2.39. The code branches on EAGAIN and is unaffected. --- CHANGELOG.md | 6 ++ mempalace/miner.py | 2 +- mempalace/project_scanner.py | 19 ++-- mempalace/split_mega_files.py | 12 +++ mempalace/sweeper.py | 9 +- tests/test_non_regular_file_guards.py | 141 +++++++++++++++++++++++++- 6 files changed, 179 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e04a193f1b..d19f39525f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), ## [Unreleased] +### Bug Fixes + +- **`sweep` books a failed `stat` as a failure again.** The non-regular-file gate added in 3.7.1 reads `stat.S_ISREG(f.stat().st_mode)` inside a `try`, and its `except OSError` printed `SKIP` and moved on. A dangling symlink, a symlink loop and a file unlinked between `rglob` and the gate all raise there, and before the gate existed every one of them reached `sweep()` and was booked in `failures` — so `sweep` went from reporting a transcript it could not read to reporting success. A failed probe is now an error, not a benign file type: it is logged, printed as `WARNING`, and appended to `failures`, while a probe that succeeds and says "not regular" still skips silently. (#2221) +- **`mempalace init` no longer tracebacks on a directory it cannot enter.** `_parse_gradle`'s `is_file()` gate sat in front of the `try` that the parser's own `except OSError` provides, so a manifest under a directory with `r` but no `x` raised `PermissionError` out of a call that used to answer "no manifest name". The gate moved inside that `try`, and `_collect_manifest_names` stats through `os.path.isfile`, which reports rather than raises. (#2221) +- **`split` no longer blocks on a FIFO at its own output name, nor writes through a broken link.** The type gate in `main` covers the files the glob listed; `split_file` builds its output names itself, so a pre-existing named pipe at one of them wedged `write_text` in the kernel waiting for a reader. The check asks about the link itself rather than its target, because a dangling symlink at an output name reads as "nothing there" and `write_text` would create the target — landing a chunk outside the output directory. Output names that are anything but a regular file are now skipped with a `SKIP` line. (#2221) + --- ## [3.7.1] — 2026-08-12 diff --git a/mempalace/miner.py b/mempalace/miner.py index 707efde562..a12f00d2c4 100644 --- a/mempalace/miner.py +++ b/mempalace/miner.py @@ -84,7 +84,7 @@ def _read_text_no_follow(filepath: Path, root: Path) -> Optional[tuple[str, floa # A reader that breaks a write lease gets EAGAIN when it passes # O_NONBLOCK, where a blocking open waits out lease-break-time # and succeeds. The kernel grants leases on regular files only - # (F_SETLEASE on a pipe gives ENXIO), so re-check the type and + # (F_SETLEASE on a pipe fails EINVAL), so re-check the type and # then read it the way this code did before the flag existed; # dropping it would silently lose a file that used to be mined. if exc.errno != errno.EAGAIN or not stat.S_ISREG(os.lstat(filepath).st_mode): diff --git a/mempalace/project_scanner.py b/mempalace/project_scanner.py index dd77c45240..ddf20b829e 100644 --- a/mempalace/project_scanner.py +++ b/mempalace/project_scanner.py @@ -188,13 +188,15 @@ def _parse_pom(path: Path) -> Optional[str]: def _parse_gradle_root_project_name(path: Path) -> Optional[str]: - # ``_parse_gradle`` reaches this with a SIBLING path it constructs itself - # (``build.gradle`` next to ``settings.gradle``), which the manifest walk - # never vetted. Opening a FIFO for reading blocks in the kernel until a - # writer appears; ``is_file()`` stats instead and never blocks. - if not path.is_file(): - return None try: + # Reached two ways: with the walk-vetted manifest path, and from + # ``_parse_gradle`` with a ``settings.gradle`` sibling it builds next + # to a vetted ``build.gradle`` — that one no walk ever saw. Opening a + # FIFO for reading blocks in the kernel until a writer appears; + # ``is_file()`` stats instead. It goes inside this ``try``, which + # already absorbs the PermissionError an unsearchable parent raises. + if not path.is_file(): + return None text = path.read_text(encoding="utf-8", errors="replace") except OSError: return None @@ -431,7 +433,10 @@ def _collect_manifest_names(repo_root: Path) -> list[tuple[str, str, Path]]: # Every parser below opens the path. A FIFO named # ``package.json`` would park that open in the kernel until a # writer appears; ``is_file()`` stats instead and never blocks. - if not manifest_path.is_file(): + # ``os.path.isfile`` rather than ``Path.is_file``: each parser + # swallows OSError itself, so the gate must swallow it too — an + # unsearchable directory used to yield "no name", not a traceback. + if not os.path.isfile(manifest_path): continue name = parser(manifest_path) if name: diff --git a/mempalace/split_mega_files.py b/mempalace/split_mega_files.py index d876c7722a..a18c8ce04d 100644 --- a/mempalace/split_mega_files.py +++ b/mempalace/split_mega_files.py @@ -224,6 +224,18 @@ def split_file(filepath, output_dir, dry_run=False): if dry_run: print(f" [{i + 1}/{len(boundaries) - 1}] {name} ({len(chunk)} lines)") else: + # The gate in ``main`` covers the files the glob listed; this name + # is built here, so nothing has vetted it. Opening a pre-existing + # FIFO for writing blocks in the kernel until a reader appears. + # ``lexists`` rather than ``exists``: the latter follows the link + # and so answers False for a DANGLING symlink, and writing through + # one creates the target instead — a chunk landing wherever the + # link points, outside the output directory entirely. + # ``os.path`` rather than ``Path``: neither call can raise, so a + # write that fails still fails at ``write_text`` as it always did. + if os.path.lexists(out_path) and not os.path.isfile(out_path): + print(f" SKIP: {name} (not a regular file)") + continue out_path.write_text("".join(chunk), encoding="utf-8") print(f" + {name} ({len(chunk)} lines)") diff --git a/mempalace/sweeper.py b/mempalace/sweeper.py index 715e41d2ab..45feb6c465 100644 --- a/mempalace/sweeper.py +++ b/mempalace/sweeper.py @@ -356,7 +356,14 @@ def sweep_directory(dir_path: str, palace_path: str) -> dict: try: regular = stat.S_ISREG(f.stat().st_mode) except OSError as exc: - print(f" SKIP: {f.name} (stat error: {exc.strerror or exc})", file=sys.stderr) + # A stat that FAILS is a real error, not a benign type. A dangling + # symlink, a symlink loop and a file unlinked between rglob and + # here all land here, and every one of them used to reach ``open`` + # and be booked below. Keep booking them, or ``sweep`` reports + # success on a transcript it could not read. + logger.error("sweeper: stat failed on %s: %s", f, exc) + print(f" WARNING: stat failed on {f}: {exc}", file=sys.stderr) + failures.append({"file": str(f), "error": str(exc)}) continue if not regular: print(f" SKIP: {f.name} (not a regular file)", file=sys.stderr) diff --git a/tests/test_non_regular_file_guards.py b/tests/test_non_regular_file_guards.py index e982d9da71..e928f6c8d7 100644 --- a/tests/test_non_regular_file_guards.py +++ b/tests/test_non_regular_file_guards.py @@ -40,10 +40,11 @@ from mempalace.llm_refine import collect_corpus_text from mempalace.miner import _read_text_no_follow, load_config, mine, scan_project from mempalace.normalize import _read_transcript_file -from mempalace.project_scanner import _collect_manifest_names +from mempalace.project_scanner import _collect_manifest_names, _parse_gradle from mempalace.repair import _copy_file_no_follow, _open_regular_file_no_follow from mempalace.room_detector_local import detect_rooms_local from mempalace.split_mega_files import main as split_main +from mempalace.split_mega_files import split_file from mempalace.sweeper import parse_claude_jsonl, sweep_directory # ``os.mkfifo`` and ``SIGALRM`` are both POSIX-only. Windows has no FIFO in @@ -54,6 +55,17 @@ reason="requires POSIX FIFOs and SIGALRM", ) +# Root holds CAP_DAC_OVERRIDE and walks straight into a directory with no +# ``x`` bit, so the file each test walls off stays readable and the assertion +# below breaks: the state these tests need cannot be built as root, they do +# not merely pass vacuously there. ``tests/test_backups.py`` gates the same +# way and additionally excludes Windows, which it has to because it carries +# no ``posix_only``; every use here already sits under ``posix_only``. +needs_unprivileged_posix = pytest.mark.skipif( + hasattr(os, "geteuid") and os.geteuid() == 0, + reason="directory permission bits do not gate root", +) + TIMEOUT_SECONDS = 10.0 @@ -356,6 +368,104 @@ def test_split_mega_files_skips_fifo(tmp_path, capsys, monkeypatch): assert "real.txt" in out +@posix_only +def test_split_file_skips_a_fifo_at_its_own_output_name(tmp_path, capsys): + """The walk gate covers the source; the output name is built here. + + ``split_file`` synthesises each per-session filename from the transcript + and writes it into the source directory, so nothing has vetted that path. + A pre-existing FIFO sitting at one of those names turned the write into a + blocking open — the same hang, in the one path the discovery gate cannot + reach. + """ + session = "Claude Code v1.0\n" + "content line\n" * 14 + "\n" * 5 + source = write_regular(tmp_path, "real.txt", session * 2) + planned = split_file(str(source), None, dry_run=True) + assert len(planned) >= 2, "fixture must produce at least two output files" + blocked = Path(planned[0]) + os.mkfifo(blocked) + + with hard_timeout(TIMEOUT_SECONDS, "split_file writing over a FIFO output"): + written = split_file(str(source), None, dry_run=False) + + out = capsys.readouterr().out + assert f"SKIP: {blocked.name} (not a regular file)" in out + assert blocked not in written + # The pipe must cost only its own chunk: every other session still lands. + assert len(written) == len(planned) - 1 + assert all(path.is_file() for path in written) + + +@posix_only +def test_split_file_skips_a_dangling_symlink_at_its_own_output_name(tmp_path, capsys): + """A broken link at an output name must not redirect the write. + + ``os.path.exists`` follows the link and answers False for a dangling one, + so the type gate would wave it through — and ``write_text`` then CREATES + the target, landing a chunk wherever the link points instead of in the + output directory. The gate has to ask about the link itself. + """ + session = "Claude Code v1.0\n" + "content line\n" * 14 + "\n" * 5 + source = write_regular(tmp_path, "real.txt", session * 2) + planned = split_file(str(source), None, dry_run=True) + assert len(planned) >= 2, "fixture must produce at least two output files" + blocked = Path(planned[0]) + outside = tmp_path / "outside" / "victim.txt" + outside.parent.mkdir() + os.symlink(outside, blocked) + assert not outside.exists(), "the link must dangle before the run" + + with hard_timeout(TIMEOUT_SECONDS, "split_file writing over a dangling symlink"): + written = split_file(str(source), None, dry_run=False) + + out = capsys.readouterr().out + assert f"SKIP: {blocked.name} (not a regular file)" in out + assert blocked not in written + assert not outside.exists(), "a chunk was written through the link, outside the output dir" + assert len(written) == len(planned) - 1 + + +@posix_only +@needs_unprivileged_posix +def test_collect_manifest_names_survives_an_unreadable_directory(tmp_path): + """The type gate must not turn a skipped manifest into a crash. + + ``os.walk`` lists the children of a directory with ``r`` but no ``x``, + and stating one of them raises ``PermissionError``. Each parser already + swallowed that through its own ``except OSError``, so the gate in front + of them has to swallow it too — otherwise ``mempalace init`` gains a + traceback where it used to report no manifest name. + """ + repo = tmp_path / "repo" + repo.mkdir() + (repo / "package.json").write_text('{"name": "inner"}', encoding="utf-8") + os.chmod(repo, 0o444) + try: + with hard_timeout(TIMEOUT_SECONDS, "_collect_manifest_names over an unreadable dir"): + found = _collect_manifest_names(repo) + finally: + os.chmod(repo, 0o755) + assert found == [] + + +@posix_only +@needs_unprivileged_posix +def test_parse_gradle_survives_an_unreadable_directory(tmp_path): + """The sibling ``settings.gradle`` is stat'd inside the parser's own try.""" + repo = tmp_path / "repo" + repo.mkdir() + build = repo / "build.gradle" + build.write_text("plugins { id 'java' }\n", encoding="utf-8") + os.chmod(repo, 0o444) + try: + with hard_timeout(TIMEOUT_SECONDS, "_parse_gradle over an unreadable dir"): + name = _parse_gradle(build) + finally: + os.chmod(repo, 0o755) + # Falls back to the directory name, exactly as it did before the gate. + assert name == "repo" + + @posix_only def test_format_miner_extract_text_does_not_block_on_fifo(tmp_path): """``mine --mode extract`` was already immune — its zero-size gate fires @@ -584,6 +694,34 @@ def test_sweep_directory_skips_a_fifo_without_booking_a_failure(tmp_path, capsys assert "SKIP: piped.jsonl (not a regular file)" in capsys.readouterr().err +@posix_only +def test_sweep_directory_still_books_a_stat_failure_as_a_failure(tmp_path, capsys): + """A pipe is nothing to sweep; a stat that FAILS is a real error. + + The type gate has to tell those apart. A dangling symlink, a symlink loop + and a file unlinked between ``rglob`` and the gate all raise from + ``stat`` — and every one of them used to reach ``open`` inside ``sweep`` + and be booked. Swallowing them would flip ``mempalace sweep`` from exit 2 + to exit 0 on a transcript it could not read. + """ + convos = tmp_path / "convos" + convos.mkdir() + write_regular( + convos, + "real.jsonl", + '{"type": "user", "sessionId": "s1", "uuid": "u1", ' + '"timestamp": "2026-01-01T00:00:00Z", ' + '"message": {"role": "user", "content": "hello"}}\n', + ) + os.symlink(convos / "gone.jsonl", convos / "dangling.jsonl") + with hard_timeout(TIMEOUT_SECONDS, "sweep_directory over a dangling symlink"): + result = sweep_directory(str(convos), str(tmp_path / "palace")) + # ``cli.cmd_sweep`` turns a non-empty ``failures`` into ``sys.exit(2)``. + assert [Path(entry["file"]).name for entry in result["failures"]] == ["dangling.jsonl"] + assert result["files_succeeded"] == 1 + assert "stat failed" in capsys.readouterr().err + + # ───────────────────────────────────────────────────────────────────────── # O_NONBLOCK must not drop a regular file the blocking open would have read # ───────────────────────────────────────────────────────────────────────── @@ -645,6 +783,7 @@ def _fake_open(path, flags, *args, **kwargs): @posix_only +@needs_unprivileged_posix def test_gather_origin_samples_survives_an_unreadable_directory(tmp_path): """The type gate must not turn a skipped file into a crash.