Skip to content

fix(hashing): op.Directory hashing now uses FileHasher cache (ITL-511)#215

Merged
eywalker merged 11 commits into
mainfrom
eywalker/itl-511-verify-opdirectory-hashing-uses-filehasher-cache-document
Jul 8, 2026
Merged

fix(hashing): op.Directory hashing now uses FileHasher cache (ITL-511)#215
eywalker merged 11 commits into
mainfrom
eywalker/itl-511-verify-opdirectory-hashing-uses-filehasher-cache-document

Conversation

@kurodo3

@kurodo3 kurodo3 Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • BasicDirectoryHasher._hash_dir() was calling hash_utils.hash_file() directly, bypassing the injected FileContentHasherProtocol entirely. This meant op.Directory hashing always recomputed every file hash from scratch, making the file hash cache dead weight for directory-heavy workloads.
  • enable_file_hash_caching() only patched FileHandler; DirectoryHandler was untouched.

Changes

  • BasicDirectoryHasher: file_hasher: FileContentHasherProtocol is now a required first argument; _hash_dir() delegates to it for per-file leaves
  • v0.1.json: wires {"_ref": "file_hasher"} into BasicDirectoryHasher config so the default context injects the same FileHasher instance
  • enable_file_hash_caching(): also patches DirectoryHandler using the same CachedFileHasher instance as FileHandler — a file cached via op.File is a cache hit during directory traversal
  • New tests in test_directory_hash_caching.py covering cache write/hit/invalidation and the enable_file_hash_caching() wiring
  • Updated test_directory_handler.py: all BasicDirectoryHasher() calls now pass file_hasher=FileHasher()
  • Updated test_hash_cachers.py: fixture restores DirectoryHandler; 2 new tests for DirectoryHandler patching
  • New doc docs/concepts/file-hash-caching.md: explains the caching architecture, activation, directory behaviour, storage, and trade-offs

Test plan

  • uv run pytest tests/test_hashing/ -v — all hashing tests pass
  • uv run pytest tests/ -q — full suite passes (4160 passed, 56 skipped)

Fixes ITL-511

kurodo3 Bot and others added 7 commits July 8, 2026 00:14
…file cache (ITL-511)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Document the file_hasher injection requirement for BasicDirectoryHasher
in the v0.1 context changelog.
…ryHasher in tests

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Wire the same CachedFileHasher into DirectoryHandler so that op.Directory
traversal consults the per-file cache. A file cached via op.File hashing is
a cache hit during directory traversal.
@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/orcapod/contexts/__init__.py 88.88% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@eywalker eywalker requested a review from Copilot July 8, 2026 01:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes op.Directory hashing so it reuses the injected FileContentHasherProtocol (and therefore the file-hash cache) instead of re-hashing file leaves from scratch, and extends enable_file_hash_caching() to patch both FileHandler and DirectoryHandler with the same cached hasher instance.

Changes:

  • BasicDirectoryHasher now requires a file_hasher and delegates file-leaf hashing to it during Merkle traversal.
  • Default context wiring updated (v0.1.json + builtin handler fallback) so directories receive the same FileHasher instance as files.
  • enable_file_hash_caching() now patches DirectoryHandler too, sharing one CachedFileHasher instance across file + directory hashing; adds tests + documentation.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/orcapod/hashing/directory_hashers.py Use injected file_hasher for file leaves; make file_hasher required.
src/orcapod/hashing/semantic_hashing/builtin_handlers.py Fallback BasicDirectoryHasher construction now passes file_hasher.
src/orcapod/contexts/__init__.py enable_file_hash_caching() now also patches DirectoryHandler with the same cached hasher instance.
src/orcapod/contexts/data/v0.1.json Inject file_hasher reference into BasicDirectoryHasher config; add changelog entry.
tests/test_hashing/test_directory_handler.py Update all BasicDirectoryHasher() call sites to pass file_hasher=FileHasher().
tests/test_hashing/test_directory_hash_caching.py New integration tests for directory hashing cache behavior + wiring.
tests/test_hashing/test_hash_cachers.py Extend cache-enablement tests to include DirectoryHandler patching and shared instance assertions.
docs/concepts/file-hash-caching.md New user documentation describing caching and activation.
superpowers/specs/2026-07-08-itl-511-directory-hash-caching-design.md Design/spec for the change set.
superpowers/plans/2026-07-08-itl-511-directory-hash-caching.md Implementation plan/checklist for the work.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/orcapod/contexts/__init__.py
Comment thread docs/concepts/file-hash-caching.md Outdated
To inspect cached entries using the SQLite command-line tool:

```bash
sqlite3 ~/.orcapod/file_hash_cache.db "SELECT path, method FROM file_hashes LIMIT 10;"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — corrected table name from file_hashes to file_hash_cache and updated the SELECT columns to valid schema columns (path, size, cached_at) — the actual schema has no method column.

Comment thread docs/concepts/file-hash-caching.md Outdated
Comment on lines +3 to +6
Orcapod can cache the SHA-256 digest of files so that re-hashing the same
unchanged file — across pipeline runs or between `op.File` and `op.Directory`
accesses — skips the disk read entirely. This page explains how the cache
works, how to turn it on, and when it helps.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — changed 'SHA-256 digest' to 'content hash (SHA-256 by default, configurable via the algorithm argument)' so it's accurate for any configured algorithm.

Comment thread docs/concepts/file-hash-caching.md Outdated
called the file hasher directly, bypassing the per-file cache even when
caching was enabled.

After ITL-511, the `DirectoryHandler`'s `BasicDirectoryHasher` uses the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do NOT mention Linear issue numbers anywhere in the documentation -- these issue numbers are NOT helpful.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — removed the ITL-511 reference from the directory hashing section. The paragraph now reads as timeless documentation without any internal issue numbers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up fix: also removed the issue number from the v0.1.json changelog entry we added in this PR. The entry now reads as plain prose without any Linear reference.

kurodo3 Bot added 2 commits July 8, 2026 01:51
…hema

- enable_file_hash_caching(): add explicit RuntimeError when no
  DirectoryHandler is registered (mirrors existing FileHandler guard)
- docs: fix SQLite table name in inspect/clear examples (file_hashes ->
  file_hash_cache); fix SELECT columns (method -> size, cached_at);
  describe FileHasher as returning a content hash (SHA-256 by default)
  rather than implying SHA-256 is the only algorithm
@kurodo3

kurodo3 Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Review round summary

Four issues fixed in a single commit (fix(contexts): guard DirectoryHandler None; fix doc table name and schema), plus a follow-up commit to remove the internal issue number from the docs:

Comment Fix
Missing None guard for DirectoryHandler in enable_file_hash_caching() Added explicit RuntimeError check after registry.get_handler_for_type(Directory), matching the existing FileHandler guard
SQL inspect example used wrong table name file_hashes Corrected to file_hash_cache; also fixed column names to match the actual schema (path, size, cached_at — there is no method column)
SQL delete example used wrong table name file_hashes Corrected to file_hash_cache
FileHasher described as always returning a "SHA-256 digest" Changed to "content hash (SHA-256 by default, configurable via the algorithm argument)"
Internal issue number (ITL-511) referenced in user-facing docs Removed; the directory hashing section is now written as timeless documentation

All 507 hashing tests still pass after the changes.

@kurodo3

kurodo3 Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Review round summary

Removed the remaining issue number reference from src/orcapod/contexts/data/v0.1.json.

The changelog entry added by this PR previously ended with (ITL-511). That suffix has been removed so the entry reads as plain prose, consistent with eywalker's feedback to keep issue numbers out of documentation.

All 507 hashing tests still pass.

@eywalker eywalker merged commit 9c6e124 into main Jul 8, 2026
11 checks passed
@eywalker eywalker deleted the eywalker/itl-511-verify-opdirectory-hashing-uses-filehasher-cache-document branch July 8, 2026 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants