feat: add staging preprocessor and watcher for cleaner, denser mining - #2011
feat: add staging preprocessor and watcher for cleaner, denser mining#2011jjcav84 wants to merge 7 commits into
Conversation
fatkobra
left a comment
There was a problem hiding this comment.
Blocking: post-mine verification is fail-open and is not tied to the batch being committed. search ... || true discards the command's exit status; an error or blank stdout does not match the negative-result regex and is therefore treated as success. The function also returns success when it cannot obtain a usable sample. Even a genuine search hit can come from an older, unrelated drawer that happens to contain the sampled text.
The script then archives the originals and deletes staging based on that result. Please require a successful machine-readable search response and verify that returned IDs or source_file values belong to the current batch manifest before destructive cleanup. Add regressions for search exit 1, blank output, an unusable sample, and a pre-existing unrelated matching drawer.
|
On top of the verify problem already raised: the archive step keys everything by basename, so two files with the same name in different subdirectories collide, and I ran the script as-is (real
|
…ory preservation Addresses review on PR MemPalace#2011: - verify_mined is now fail-closed and tied to the current batch. It uses machine-readable mempalace search --json and a batch manifest to confirm that any search hit source_file belongs to the batch and matches the sampled file before destructive cleanup. - Added tools/verify_mined.py to extract a snippet, run a scoped JSON search, and return non-zero for: search exit 1, blank output, unusable sample, unrelated/pre-existing matching drawers, and sources outside the manifest. - archive_files now derives archive names from the original file's path relative to staging (preserving subdirectories), and refuses to overwrite an existing archive path. This prevents projA/notes.md and projB/notes.md from colliding silently. - preprocess_staging.py now preserves directory structure under processed/ so same-named files in different subdirectories do not overwrite each other before mining. - Added --json and --source-file options to mempalace search so the watcher can consume a machine-readable response. - Added tests/test_staging_watcher.py with regressions for all of the above. Resolves: MemPalace#2011 (review)
4c9f13d to
acf5632
Compare
…ory preservation Addresses review on PR MemPalace#2011: - verify_mined is now fail-closed and tied to the current batch. It uses machine-readable mempalace search --json and a batch manifest to confirm that any search hit source_file belongs to the batch and matches the sampled file before destructive cleanup. - Added tools/verify_mined.py to extract a snippet, run a scoped JSON search, and return non-zero for: search exit 1, blank output, unusable sample, unrelated/pre-existing matching drawers, and sources outside the manifest. - archive_files now derives archive names from the original file's path relative to staging (preserving subdirectories), and refuses to overwrite an existing archive path. This prevents projA/notes.md and projB/notes.md from colliding silently. - preprocess_staging.py now preserves directory structure under processed/ so same-named files in different subdirectories do not overwrite each other before mining. - Added --json and --source-file options to mempalace search so the watcher can consume a machine-readable response. - Added tests/test_staging_watcher.py with regressions for all of the above. Resolves: MemPalace#2011 (review)
…ory preservation Addresses review on PR MemPalace#2011: - verify_mined is now fail-closed and tied to the current batch. It uses machine-readable mempalace search --json and a batch manifest to confirm that any search hit source_file belongs to the batch and matches the sampled file before destructive cleanup. - Added tools/verify_mined.py to extract a snippet, run a scoped JSON search, and return non-zero for: search exit 1, blank output, unusable sample, unrelated/pre-existing matching drawers, and sources outside the manifest. - archive_files now derives archive names from the original file's path relative to staging (preserving subdirectories), and refuses to overwrite an existing archive path. This prevents projA/notes.md and projB/notes.md from colliding silently. - preprocess_staging.py now preserves directory structure under processed/ so same-named files in different subdirectories do not overwrite each other before mining. - Added --json and --source-file options to mempalace search so the watcher can consume a machine-readable response. - Added tests/test_staging_watcher.py with regressions for all of the above. Resolves: MemPalace#2011 (review)
a4eb281 to
3f22dc4
Compare
…ory preservation Addresses review on PR MemPalace#2011: - verify_mined is now fail-closed and tied to the current batch. It uses machine-readable mempalace search --json and a batch manifest to confirm that any search hit source_file belongs to the batch and matches the sampled file before destructive cleanup. - Added tools/verify_mined.py to extract a snippet, run a scoped JSON search, and return non-zero for: search exit 1, blank output, unusable sample, unrelated/pre-existing matching drawers, and sources outside the manifest. - archive_files now derives archive names from the original file's path relative to staging (preserving subdirectories), and refuses to overwrite an existing archive path. This prevents projA/notes.md and projB/notes.md from colliding silently. - preprocess_staging.py now preserves directory structure under processed/ so same-named files in different subdirectories do not overwrite each other before mining. - Added --json and --source-file options to mempalace search so the watcher can consume a machine-readable response. - Added tests/test_staging_watcher.py with regressions for all of the above. Resolves: MemPalace#2011 (review)
|
@fatkobra I rebased this onto the latest develop and resolved the conflict in mempalace/searcher.py. The branch now keeps the upstream stop_words/functools changes while retaining the fail-closed batch verification, manifest-scoped search, and subdirectory-preserving archive logic from the review response. Tests pass: test_staging_watcher.py + test_preprocess_staging.py (38 passed) and test_searcher.py + test_hybrid_search.py (77 passed). Could you take another look when you have a moment? |
fatkobra
left a comment
There was a problem hiding this comment.
Thanks for the rebase and follow-up. The exact-source JSON verification now fails on search errors, blank/malformed output, and unrelated source hits, and preserving relative paths resolves the basename-collision issue from the earlier review.
- Verification still checks only one randomly selected file.
The watcher mines with --max-chunks-per-file 500, while the miner treats a file exceeding that cap as a successful-command skip. A batch can therefore contain:
- file A, which mines successfully; and
- file B, which is skipped by the chunk cap.
If shuf selects A, verification succeeds and both originals are archived and removed from staging even though B was never mined.
The preprocessor has a similar partial-success path: it records processing exceptions in stats["errors"], but its CLI still exits successfully.
Please require a machine-readable per-file outcome for the complete manifest. Every processed source should be confirmed as mined/current or explicitly accepted under a documented non-mining status. Preprocessing errors and chunk-cap skips should fail the batch before cleanup. Please add a regression where A succeeds, B is skipped, A is selected for verification, and staging is nevertheless retained.
- Archive failure is not a cleanup gate.
process_batch() calls archive_files and then unconditionally calls clear_staging. Because the script does not use set -e, an explicit archive failure does not prevent deletion. Individual mkdir, checksum, gzip, and manifest writes are also not consistently checked.
Please make archive completion a required gate before cleanup. Prefer building the archive in a temporary directory, checking every compression/write result, validating the archive, and atomically renaming it into place. At minimum, any archive error must return from process_batch without clearing staging.
Please add a failure-injection regression—such as a failing gzip or unwritable archive directory—and prove that every original remains in staging.
- The retry path can recursively preprocess stale output.
Only top-level files inside processed/ are deleted, and the recursive scan excludes only files whose immediate parent is named processed. After a failed batch, a retained file such as processed/projA/notes.md is therefore processed into processed/processed/projA/notes.md on the next attempt.
Please remove and recreate the complete processed tree before each non-dry run, and exclude every descendant of processed/ from the source scan. Add a regression that runs preprocessing twice against nested input and verifies that no processed/processed/... output or duplicate is produced.
Once complete-batch verification, fail-closed archiving, and retry cleanup are covered, my original concern will be resolved.
…ory preservation Addresses review on PR MemPalace#2011: - verify_mined is now fail-closed and tied to the current batch. It uses machine-readable mempalace search --json and a batch manifest to confirm that any search hit source_file belongs to the batch and matches the sampled file before destructive cleanup. - Added tools/verify_mined.py to extract a snippet, run a scoped JSON search, and return non-zero for: search exit 1, blank output, unusable sample, unrelated/pre-existing matching drawers, and sources outside the manifest. - archive_files now derives archive names from the original file's path relative to staging (preserving subdirectories), and refuses to overwrite an existing archive path. This prevents projA/notes.md and projB/notes.md from colliding silently. - preprocess_staging.py now preserves directory structure under processed/ so same-named files in different subdirectories do not overwrite each other before mining. - Added --json and --source-file options to mempalace search so the watcher can consume a machine-readable response. - Added tests/test_staging_watcher.py with regressions for all of the above. Resolves: MemPalace#2011 (review)
3f22dc4 to
a1c54d6
Compare
…ory preservation Addresses review on PR MemPalace#2011: - verify_mined is now fail-closed and tied to the current batch. It uses machine-readable mempalace search --json and a batch manifest to confirm that any search hit source_file belongs to the batch and matches the sampled file before destructive cleanup. - Added tools/verify_mined.py to extract a snippet, run a scoped JSON search, and return non-zero for: search exit 1, blank output, unusable sample, unrelated/pre-existing matching drawers, and sources outside the manifest. - archive_files now derives archive names from the original file's path relative to staging (preserving subdirectories), and refuses to overwrite an existing archive path. This prevents projA/notes.md and projB/notes.md from colliding silently. - preprocess_staging.py now preserves directory structure under processed/ so same-named files in different subdirectories do not overwrite each other before mining. - Added --json and --source-file options to mempalace search so the watcher can consume a machine-readable response. - Added tests/test_staging_watcher.py with regressions for all of the above. Resolves: MemPalace#2011 (review)
a1c54d6 to
b3d198d
Compare
|
Hi maintainers — this PR is rebased onto the latest and the staging tests pass. Ready for review. |
|
@fatkobra addressed the three review points in this push:
|
|
@fatkobra added the requested end-to-end staging regression:
Full suite on |
|
@fatkobra all points from the latest |
fatkobra
left a comment
There was a problem hiding this comment.
Thanks — I reviewed the revised pipeline. My previous concerns around full
manifest verification, archive gating, and recursive processed-file cleanup
appear to be addressed.
I found two remaining blocking races around determining and claiming the
batch.
1. “Stable” currently means only that the file count did not change
wait_for_stable() compares the number of files across debounce intervals.
An existing file can continue to be appended to, truncated, or replaced
without changing the file count. The debounce can therefore declare the
staging directory stable and begin preprocessing while a producer is still
writing one of the files.
That does not satisfy the documented “no writes for the debounce period”
contract.
Please detect changes to the files themselves. A batch fingerprint could
include, at minimum:
- relative path;
- size;
- modification time;
- and optionally an inode or content hash where needed.
The debounce period should reset whenever any fingerprint component changes.
Please add a regression in which one existing file grows repeatedly while
the number of files remains constant, and verify that processing does not
begin until the file has remained unchanged for the complete debounce
period.
2. Files arriving after the manifest snapshot can be archived or deleted
without being processed
The processed manifest describes the files handled by preprocessing and
mining.
The later archive phase rescans the current staging tree rather than
archiving only that exact claimed batch. A file arriving after preprocessing
but before the archive scan can therefore be included in the archive even
though it was never processed or mined.
There is a second, more destructive window: a file arriving after the
archive is created but before clear_staging() can be removed by cleanup
without having been processed or included in the archive.
Please claim one immutable batch before destructive work begins.
A robust approach would be to atomically move the stable batch into a private
work directory and process, archive, and delete only that directory. New
arrivals would remain in the public staging directory for the next batch.
Alternatively, every later phase must operate only on the exact snapshot
captured at claim time and verify that each file is still the same file
before archiving or deleting it.
Please add deterministic regressions that inject:
- a new file after manifest creation but before archiving;
- a new file after archiving but before cleanup.
In both cases, the late file must remain available for the next run and must
never be silently archived as processed or deleted.
Once the batch is atomically claimed and content stability is verified, the
pipeline will have a much stronger no-data-loss contract.
|
Hi @fatkobra — the two race/blocking issues should now be addressed:
Regressions added:
Verification:
Could you re-review when you have a moment? |
fatkobra
left a comment
There was a problem hiding this comment.
Thanks — I reviewed 374c738.
The content-based debounce and late-file protections address most of my
previous review:
- file growth, truncation, replacement, and additions change the stability
fingerprint; - late files are excluded from preprocessing and archiving;
- archive and cleanup are limited to snapshot members;
- modified snapshot members are retained rather than deleted.
One blocking race remains: the claimed batch is still a manifest of mutable
live paths, not an immutable batch.
claim_batch() records path, size, mtime, and hash, but
_read_batch_snapshot() intentionally keeps only the relative path.
preprocess_directory() then reads each file from the live staging tree
without verifying its recorded identity before or after the read.
A producer can therefore modify or replace a claimed file after the snapshot
is written, or while preprocessing reads it. The changed or partial content
can be mined as part of this batch. A later archive identity check may fail
and preserve the source file, but it does not undo content already written to
the palace.
Please make preprocessing consume immutable claimed bytes. For example:
- atomically move the stable input set into a private batch/work directory; or
- copy each claimed file into a private work directory, verify the copy
against the recorded hash, and preprocess only the verified copy.
Please add a deterministic regression that:
- claims a source file;
- modifies or replaces it after claim but before or during preprocessing;
- verifies that the changed content is not mined as part of that batch;
- verifies that the live source remains available for a later stable run.
The archive and cleanup identity checks should remain, but they occur too
late to protect the preprocessing and mining stage.
My original late-file deletion concern is substantially improved, but the
promised immutable batch claim is not complete yet.
…ory preservation Addresses review on PR MemPalace#2011: - verify_mined is now fail-closed and tied to the current batch. It uses machine-readable mempalace search --json and a batch manifest to confirm that any search hit source_file belongs to the batch and matches the sampled file before destructive cleanup. - Added tools/verify_mined.py to extract a snippet, run a scoped JSON search, and return non-zero for: search exit 1, blank output, unusable sample, unrelated/pre-existing matching drawers, and sources outside the manifest. - archive_files now derives archive names from the original file's path relative to staging (preserving subdirectories), and refuses to overwrite an existing archive path. This prevents projA/notes.md and projB/notes.md from colliding silently. - preprocess_staging.py now preserves directory structure under processed/ so same-named files in different subdirectories do not overwrite each other before mining. - Added --json and --source-file options to mempalace search so the watcher can consume a machine-readable response. - Added tests/test_staging_watcher.py with regressions for all of the above. Resolves: MemPalace#2011 (review)
fe03fdc to
e0ecd3d
Compare
Adds two optional tools for users who stage files before mining: - tools/preprocess_staging.py: Strips boilerplate (license headers, system prompts, IDE metadata, tool confirmations) and splits files exceeding a line limit (default 4000) into numbered parts. Writes cleaned output to a processed/ subdirectory alongside mempalace.yaml so wing routing is preserved. - tools/staging_watcher.sh: Bash watcher that runs the full pipeline: preprocess → mine → verify (search sample) → compress → gzip → archive. Archives originals with sha256 manifests before clearing staging. Includes palace-lock awareness to avoid spawning concurrent mines. - tests/test_preprocess_staging.py: 29 tests covering skip logic, block stripping, license detection, dedup, blank-line collapse, file splitting, and directory-level preprocessing. These are opt-in tools, not core mining changes. The verbatim principle is preserved: originals are gzipped and archived with checksums before any preprocessing occurs. The preprocessor only touches staging copies. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ory preservation Addresses review on PR MemPalace#2011: - verify_mined is now fail-closed and tied to the current batch. It uses machine-readable mempalace search --json and a batch manifest to confirm that any search hit source_file belongs to the batch and matches the sampled file before destructive cleanup. - Added tools/verify_mined.py to extract a snippet, run a scoped JSON search, and return non-zero for: search exit 1, blank output, unusable sample, unrelated/pre-existing matching drawers, and sources outside the manifest. - archive_files now derives archive names from the original file's path relative to staging (preserving subdirectories), and refuses to overwrite an existing archive path. This prevents projA/notes.md and projB/notes.md from colliding silently. - preprocess_staging.py now preserves directory structure under processed/ so same-named files in different subdirectories do not overwrite each other before mining. - Added --json and --source-file options to mempalace search so the watcher can consume a machine-readable response. - Added tests/test_staging_watcher.py with regressions for all of the above. Resolves: MemPalace#2011 (review)
…ve processed cleanup
wait_for_stable() now uses a SHA-256 fingerprint of the staging tree (path, size, mtime) instead of a raw file count. A file that grows, is truncated, or is replaced while the debounce runs resets the timer. process_batch() now claims an immutable batch snapshot before any destructive work. The snapshot feeds: - preprocess (optional --batch-snapshot, so late arrivals are not preprocessed in the current run); - archive_files (only snapshot files are archived, and each is verified against its recorded size/mtime/sha256 before gzip); - clear_staging (only unchanged snapshot files are deleted, so late or modified files survive for the next batch). Adds regression coverage: - fingerprint changes on file growth/addition and is stable when idle; - archive ignores late files and skips modified files; - clear_staging leaves late/modified files in place. Full test suite: 4346 passed. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
- Verify each claimed file against the sha256 recorded in the batch snapshot. - Copy claimed files into a private .batch_work directory before reading. - Skip files that have been modified or replaced after the batch was claimed. - Add deterministic regression test for post-claim source mutation.
e0ecd3d to
15e479d
Compare
|
@fatkobra the immutable batch claim you asked for is in commit
Full test suite passes (4520 passed, 31 skipped). Could you re-review? |
|
Hi @fatkobra — the immutable batch claim fix from your last review is in commit 38b4708 (pushed Aug 22). |
|
@fatkobra — the 15 Aug blocker should be resolved in commit 38b4708 (pushed 22 Aug). Preprocessing now copies claimed files into a private .batch_work directory and verifies the copy against the sha256 recorded in the batch snapshot. Files modified or replaced after the batch was claimed are skipped and left in staging for the next run. The regression test is test_preprocess_staging.py::test_batch_snapshot_rejects_modified_source. Could you take another look when you have a moment? |
fatkobra
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. The branch now verifies the full manifest, preserves relative paths, rebuilds processed output, gates cleanup on archiving, and preprocesses from a hash-checked work copy.
I still cannot approve because four data-integrity issues remain.
1. Verification does not prove the current source version was mined
verify_one() accepts any result with the same source_file. If version A was previously indexed and version B at the same path is skipped—for example because it exceeds --max-chunks-per-file 500—A can satisfy B’s verification query. B is then archived and removed even though it was never indexed.
Please tie verification to the exact source fingerprint or a machine-readable per-file mine outcome. Skips must fail the batch, and “already current” must require matching stored metadata.
Add a regression with indexed version A and oversized version B sharing the verification snippet; B must remain in staging.
2. .batch_work is still watcher input
The preprocessor creates staging/.batch_work/..., but discovery, snapshots, and cleanup do not consistently exclude or remove it. After a successful batch, the watcher can treat its own work copies as a new batch and produce a phantom retry/failure.
Please move private work outside the watched tree or exclude all .batch_work descendants everywhere, with explicit cleanup on success, failure, and restart.
Add a regression proving that after a successful batch, count_files is zero and the next watcher iteration does nothing.
3. The claimed bytes are not used through archive and deletion
Preprocessing reads .batch_work, but archiving and cleanup return to the live public path. A producer can replace that path:
- after the hash check but before
gzip, so different bytes are archived under the old digest; or - after the cleanup check but before
rm, so new unclaimed input is deleted.
A changed file may also be skipped during archive while the batch still succeeds because another file archived successfully.
Please claim stable files into one private per-batch tree and use those paths for preprocessing, verification provenance, archiving, and deletion. New arrivals must remain in public staging for the next batch.
Add deterministic replacement tests around archive and unlink. The archive must match the claimed digest, and late replacements must survive.
4. Aggregate hashing fails open without sha256sum
file_sha256() supports shasum, but fingerprint_staging() hardcodes sha256sum. On systems without it, an empty digest can be compared repeatedly and a changing tree may be treated as stable.
Please use one checked hashing helper for both file and aggregate hashes. Missing commands, failed pipelines, or empty output must stop batch claiming.
Add tests for a shasum-only environment and for no available SHA-256 command.
Once these four cases are covered, I will be comfortable approving.
Summary
tools/preprocess_staging.py— Optional preprocessor that strips boilerplate (license headers, system prompts, IDE metadata, tool confirmation noise) and splits files exceeding a line limit (default 4000) into numbered parts. Writes cleaned output to aprocessed/subdirectory somempalace.yamlwing routing is preserved.tools/staging_watcher.sh— Bash watcher that runs the full ingest pipeline:preprocess → mine → verify → compress → gzip → archive. Archives originals with sha256 manifests before clearing staging. Includes palace-lock awareness to avoid spawning concurrent mines.tests/test_preprocess_staging.py— 29 tests covering skip logic, block stripping, license detection, dedup, blank-line collapse, file splitting, and directory-level preprocessing.Design notes
These are opt-in tools, not core mining changes. The verbatim principle is preserved:
The preprocessor is useful for users who stage files from multiple sources (session transcripts, code dumps, configs) before mining. It reduces wasted chunks on license headers, agent XML tags, and tool confirmations that add no searchable value.
Test plan
pytest tests/test_preprocess_staging.py -v)ruff checkpassesruff format --checkpassesGenerated with Devin