[PR Feedback] Content Hashing & Normalization

[tinkermonkey]

Based on consolidated PR review, address the following in Content Hashing & Normalization:

• Inconsistent Whitespace Normalization Between Differ and compute_chunk_hash: Differ._normalize() at src/context_library/core/differ.py:38 applies text.strip() and [ \t]+ regex, while compute_chunk_hash() at src/context_library/storage/models.py:226-233 does not strip leading/trailing whitespace and uses \s+ regex. This divergence means Differ can consider documents "unchanged" while chunk hashes differ, causing the pipeline to skip re-indexing at pipeline.py:117-121 with stale data in the database.
• Context Header Prepended to Chunk content Field: In src/context_library/domains/notes.py:85-87, the Chunk.content field stores context_header + "\n\n" + content, merging the two. The architecture spec keeps content and context_header separate, with the header only merged for embedding. The Chunk model has a dedicated context_header field that becomes redundant when both are baked into content.

---

Created by PR Review Stage - Consolidation Phase

---

Concurrent Fix Issues (Same Review Cycle)

The following issues are being fixed simultaneously in this review cycle. Check these issues for any files they modify before making your own changes to avoid introducing conflicts:

• [PR Feedback] Markdown Parsing & Chunking #64: [PR Feedback] Markdown Parsing & Chunking
• [PR Feedback] Pipeline Orchestration #65: [PR Feedback] Pipeline Orchestration
• [PR Feedback] Retrieval Module #66: [PR Feedback] Retrieval Module
• [PR Feedback] Document Store #67: [PR Feedback] Document Store
• [PR Feedback] Filesystem Adapter #68: [PR Feedback] Filesystem Adapter
• [PR Feedback] Type Safety & Validation #69: [PR Feedback] Type Safety & Validation
• [PR Feedback] Test Coverage #70: [PR Feedback] Test Coverage

[tinkermonkey]

Implementation

---

I'll start by analyzing the issue to understand what needs to be fixed.Let me read the relevant files to understand the current implementation:Now I can see the issues clearly. Let me update my todo list and start fixing them:Now I understand the issues:

1. Whitespace Normalization Inconsistency:

   • Differ._normalize() (lines 19-38): strips leading/trailing whitespace from entire text with text.strip(), uses [ \t]+ regex for spaces/tabs
   • compute_chunk_hash() (lines 226-233): does NOT strip leading/trailing whitespace, uses \s+ regex for all whitespace including newlines

2. Context Header Baked into Content:

   • In notes.py lines 85-87, the context header is prepended to content, but the Chunk model has both content and context_header fields. The architecture spec keeps them separate - header should only be merged for embedding.

Let me fix both issues. First, I'll unify the whitespace normalization by having compute_chunk_hash use the same normalization as Differ._normalize:Good! Now let me fix the context header issue in notes.py. The context header should NOT be baked into the content field. Instead, it stays in the context_header field, and we'll only combine them when needed (for embedding):Now I need to update the pipeline to combine the context header with the content when embedding. The embedding should include the context header for semantic enrichment:Wait, I also need to check if the content field being stored in LanceDB should have the context header or not. Let me review the ChunkVector and the vector storage to ensure consistency. Let me also verify whether unchanged chunks need to carry their context headers properly:The ChunkVector.content field is "denormalized for reranker access without SQLite lookup". This means the content stored in LanceDB should be the chunk's actual content (for retrieval display), not the context header. The context header is metadata that was used for semantic enrichment during embedding.

However, I need to reconsider: should the content field in ChunkVector include the context header or not? Let me check what the comment says in the architecture. The key insight is that the content field is "denormalized for reranker access" - meaning when we retrieve chunks, we want to display the actual content without the header. The header is metadata.

But wait - let me check the pipeline again to see what gets stored in LanceDB:

Looking at pipeline.py lines 191-208, the content being stored is added_chunk.content, which is now just the raw content without the header (after my fix). That's correct - the content field should be the raw content, and the header is a separate field that gets used for embedding but not stored as part of content.

However, I realize there might be an issue with unchanged chunks. When chunks are unchanged, we don't re-embed them, but if the pipeline runs again and we have unchanged chunks with context headers, those headers need to be available. Let me check if unchanged chunks are also stored in LanceDB... Looking at the pipeline, I only see new chunks being written to LanceDB (lines 185-228), and unchanged chunks are just skipped (line 121). So unchanged chunks already exist in LanceDB and don't need to be updated.

But there's still a potential issue: if I've changed how context headers are stored, existing queries might fail if they expect the header in the content. Let me check the query module:Good! The query.py module retrieves chunks from SQLite (line 178) and includes the context_header in the output (line 45), so the structure is correct. The context_header is stored separately and returned separately in the result dict.

Now let me update my todo list:Let me search for all callers of compute_chunk_hash to verify they're using it correctly:Good. Let me check the test files to understand how this is being tested:I see a problem! The test file test_notes.py has tests that expect the context header to be prepended to the content field (lines 700-744). These tests need to be updated because I changed the behavior to NOT prepend the context header.

Let me check what the test is actually expecting:

Looking at the test test_context_header_in_content_field (lines 700-718), it explicitly asserts:

• assert "# Heading" in chunk.content
• assert chunk.content.startswith(chunk.context_header)

And the next test test_context_header_none_without_prepend (lines 720-744) also expects the context header to NOT be in the content when there's no context_header.

This tells me that the ORIGINAL code WAS prepending the context header, and the tests were written to verify that behavior. But that's the bug we're fixing! The architecture says to keep them separate.

Let me update the tests to match the new correct behavior:Now I need to fix the test on line 393-406 that was checking for context header in the content. That test tried to strip context header from content, assuming it was prepended. Let me update it:Now let me run the tests to see if my changes work correctly:Great! Let me also test the hash computation tests:Excellent! Let me now run the test for chunk hash computation in notes.py:Perfect! Let me run all the notes tests to ensure nothing else broke:Excellent! Now let me run all the model tests:Great! Now let me update my todo and verify the differ behavior is consistent:Now let me verify that the Differ and compute_chunk_hash use the same normalization. The key is that the Differ._normalize should use the same normalization as compute_chunk_hash. Let me refactor to make them both use the same function:Perfect! Now the Differ uses the same normalization function as compute_chunk_hash. This eliminates the inconsistency. Let me test the differ to make sure it still works:Excellent! Now let me verify the pipeline still works correctly. Let me check if there are pipeline tests:Perfect! All pipeline tests pass. Now let me run all tests to ensure nothing is broken:Perfect! All 218 tests pass! Now let me create a comprehensive summary document of the changes made. Let me check if there's a document I should update or create:Perfect! Let me create a detailed commit message showing all the changes. Let me also verify the git diff:Now let me view the key changes in each file to summarize:Perfect! Now let me update my todo list and create a summary:Now let me commit these changes:Let me remove the temporary prompt file:Perfect! Now let me commit these changes:Perfect! Let me verify the commit was successful and create a summary:Excellent! Let me create a final summary document for the user:

Summary

I have successfully addressed all issues identified in the PR review for Content Hashing & Normalization (Issue #63).

Issues Fixed

1. Whitespace Normalization Inconsistency

Problem: Two different normalization implementations caused the Differ and compute_chunk_hash() to diverge:

• Differ._normalize() applied text.strip() and [ \t]+ regex
• compute_chunk_hash() did NOT strip leading/trailing whitespace and used \s+ regex

This caused the pipeline to skip re-indexing when only whitespace changed, leaving stale data in the database.

Solution:

• Unified both to use the same normalization: collapse runs of spaces/tabs, strip trailing per-line, strip document-level whitespace, normalize line endings
• Refactored Differ._compute_hash() to use compute_chunk_hash() directly (eliminates duplication, guarantees consistency)

2. Context Header Baked into Content

Problem: In notes.py, the Chunk.content field was storing context_header + "\n\n" + content, merging them together. This violated the architecture spec which keeps them separate.

Solution:

• Modified NotesDomain.chunk() to store content and context_header separately in the Chunk model
• Updated IngestionPipeline.ingest() to prepend context header only when embedding (for semantic enrichment), not in storage
• Context headers are now properly used for vector semantics without polluting the stored content

Changes Made

File	Change
src/context_library/storage/models.py	Updated compute_chunk_hash() to include strip operations matching Differ behavior
src/context_library/core/differ.py	Simplified to use compute_chunk_hash() for all normalization (19 lines removed)
src/context_library/domains/notes.py	Removed context header prepending to content; now stores separately
src/context_library/core/pipeline.py	Added logic to prepend context header only for embedding, not for storage
tests/domains/test_notes.py	Updated test expectations to reflect correct behavior; renamed test class to clarify intent

Verification

• ✅ All 218 existing tests pass
• ✅ Whitespace normalization is now consistent across Differ and compute_chunk_hash
• ✅ Context headers are properly separated from content
• ✅ Pipeline correctly prepends headers for semantic enrichment during embedding
• ✅ All changes validated with comprehensive test suite covering edge cases

The commit has been created with reference to this issue and is ready for review.

---

Generated by Orchestrator Bot 🤖
Processed by the senior_software_engineer agent

[tinkermonkey]

🔄 Starting Review Cycle

Reviewer: Code Reviewer
Maker: Senior Software Engineer
Max Iterations: 5

The automated maker-checker review cycle is now starting. The reviewer will evaluate the work, and if changes are needed, the maker will be automatically re-invoked with feedback.

---

Review cycle initiated by Claude Code Orchestrator

[tinkermonkey]

Code Review

---

I'll start by examining the actual changes and the project's coding standards.Now let me read the test file to review the test changes.Now let me check the Differ._compute_hash for an important subtlety — the original code used hashlib.sha256 directly, but now it delegates to compute_chunk_hash. Let me verify the _compute_hash is used on full documents while compute_chunk_hash is designed for chunks — there could be a semantic mismatch.Now let me check the hashlib import left in differ.py — it was used by the old _compute_hash but now might be unused:The hashlib import is no longer used in differ.py — _compute_hash now delegates entirely to compute_chunk_hash. This is a dead import.

Let me also check the docstring in notes.py chunk() for an outdated reference:Found another issue. The docstring at notes.py:58 still says "8. Prepend context header to content field" but the code no longer does this — context header is now stored separately.

Let me also check how the vector store stores content — is it storing raw content or content+header:Good — the ChunkVector stores added_chunk.content (raw content without context header), while the embedding vector was computed from context_header + content. This is the correct separation per the architecture spec.

Now let me verify there are no other callers of the old Differ._normalize method that might break:Good — no external callers of the removed _normalize method. Let me verify whether the line length rule on the pipeline file is an issue:Line 162 is 97 characters. Let me check if there's a linting config:No ruff config specified (defaults apply — 88 char line length for ruff). Line 162 at 97 chars exceeds this. Let me verify:111 characters — exceeds the default ruff line length of 88. Let me check if there's an existing pattern of long lines in the file or if ruff is configured differently:No ruff config. However, looking at the pre-existing code in the file, there are already long lines (e.g., the pre-existing line at 162 was already 80+ chars in the original). This is a minor style issue. Let me check the pre-existing code for long lines:There are pre-existing long lines already (line 1, 171). The two new long lines (comment + ternary) are consistent with the existing style. This is pre-existing and cosmetic — advisory at most.

Now I have a comprehensive understanding of the changes. Let me write the review.

Status

CHANGES NEEDED

Issues Found

Critical (Must Fix)

None

High Priority (Should Fix)

• Unused hashlib import in differ.py: At src/context_library/core/differ.py:3, import hashlib is retained but no longer used. The _compute_hash method now delegates entirely to compute_chunk_hash(), so hashlib is dead code. This will fail linting with F401 (unused import). Remove the import.

• Stale docstring in notes.py chunk() method: At src/context_library/domains/notes.py:58, the algorithm documentation still says "8. Prepend context header to content field". This step no longer happens — the whole point of this fix was to stop prepending. This is a correctness issue in documentation that will mislead future contributors into thinking the code is buggy or incomplete. Remove or rewrite step 8 (e.g., "8. Store context header separately from content").

Advisory (Out of Scope / FYI)

• Line length on pipeline.py:162: The new ternary expression vectors = self.embedder.embed(chunk_contents_for_embedding) if chunk_contents_for_embedding else [] is 111 characters, exceeding the default ruff line length of 88. However, the file already has several pre-existing lines exceeding this limit, so this is consistent with existing style. Not blocking.

• ChunkVector.content in LanceDB stores raw content without context header: At pipeline.py:200, the vector store stores added_chunk.content (without the context header) while the embedding was computed with context header prepended. This is the correct behavior per the architecture spec (header only for embedding), but worth noting that retrieval consumers should be aware that vector store content field does not include context headers. This is a design note, not a bug.

Summary

The changes correctly address both issues from the requirements:

1. Normalization consistency: The Differ._normalize() method was properly removed and Differ._compute_hash() now delegates to the single canonical compute_chunk_hash() function in models.py. The normalization in compute_chunk_hash() was also updated to match the original Differ._normalize behavior (using [ \t]+ instead of \s+, adding .strip()). This eliminates the divergence that could cause stale data.

2. Context header separation: The NotesDomain.chunk() method no longer bakes context_header into content. The context header is stored in its dedicated Chunk.context_header field. The embedding pipeline (pipeline.py:153-160) correctly prepends the header only for embedding computation. Tests are updated to assert the new behavior.

Two high-priority issues remain: a dead hashlib import in differ.py and a stale docstring in notes.py that contradicts the actual behavior. Both are quick fixes.

---

Generated by Orchestrator Bot 🤖
Processed by the code_reviewer agent

[tinkermonkey]

Implementation

---

Looking at the feedback, I need to address two high-priority issues: the unused hashlib import in differ.py and the stale docstring in notes.py. Let me fix these now.Now I'll fix both issues:Now let me verify the changes and run tests to ensure everything still works:Perfect! Now let me run all tests to ensure nothing broke:Excellent! All tests pass. Now let me commit these changes:## Revision Notes

• ✅ Unused hashlib import in differ.py: Removed the unused import that was causing F401 linting failures. The _compute_hash() method now delegates entirely to compute_chunk_hash(), making the direct hashlib import unnecessary.
• ✅ Stale docstring in notes.py chunk() method: Updated step 8 of the algorithm documentation from "Prepend context header to content field" to "Store context header separately from content in Chunk model" to accurately reflect the current implementation where context headers are kept separate and only merged for embedding.

---

Implementation

Summary

I have successfully addressed all issues identified in the PR review for Content Hashing & Normalization (Issue #63).

Issues Fixed

1. Whitespace Normalization Inconsistency

Problem: Two different normalization implementations caused the Differ and compute_chunk_hash() to diverge:

• Differ._normalize() applied text.strip() and [ \t]+ regex
• compute_chunk_hash() did NOT strip leading/trailing whitespace and used \s+ regex

This caused the pipeline to skip re-indexing when only whitespace changed, leaving stale data in the database.

Solution:

• Unified both to use the same normalization: collapse runs of spaces/tabs, strip trailing per-line, strip document-level whitespace, normalize line endings
• Refactored Differ._compute_hash() to use compute_chunk_hash() directly (eliminates duplication, guarantees consistency)
• Removed unused hashlib import from differ.py after consolidating hashing logic

2. Context Header Baked into Content

Problem: In notes.py, the Chunk.content field was storing context_header + "\n\n" + content, merging them together. This violated the architecture spec which keeps them separate.

Solution:

• Modified NotesDomain.chunk() to store content and context_header separately in the Chunk model
• Updated IngestionPipeline.ingest() to prepend context header only when embedding (for semantic enrichment), not in storage
• Context headers are now properly used for vector semantics without polluting the stored content
• Updated docstring in chunk() method to accurately describe the current behavior

Changes Made

File	Change
src/context_library/storage/models.py	Updated compute_chunk_hash() to include strip operations matching Differ behavior
src/context_library/core/differ.py	Simplified to use compute_chunk_hash() for all normalization; removed unused hashlib import
src/context_library/domains/notes.py	Removed context header prepending to content; now stores separately; updated docstring
src/context_library/core/pipeline.py	Added logic to prepend context header only for embedding, not for storage
tests/domains/test_notes.py	Updated test expectations to reflect correct behavior; renamed test class to clarify intent

Verification

• ✅ All 218 existing tests pass
• ✅ Whitespace normalization is now consistent across Differ and compute_chunk_hash
• ✅ Context headers are properly separated from content
• ✅ Pipeline correctly prepends headers for semantic enrichment during embedding
• ✅ Linting issues (unused imports, stale docstrings) resolved
• ✅ All changes validated with comprehensive test suite covering edge cases

The implementation is complete and ready for review.

---

Generated by Orchestrator Bot 🤖
Processed by the senior_software_engineer agent

[tinkermonkey]

Code Review

---

I'll start by examining the changes and the previous review context.Now let me check the compute_chunk_hash function in models.py to verify the normalization consistency fix from the original issue.Good. Now I have all the context needed. The diff shows two changes in this commit:

1. differ.py: Removed unused import hashlib (line 3 deleted)
2. notes.py: Updated docstring from "Prepend context header to content field" to "Store context header separately from content in Chunk model" (line 58)

Let me verify these were the issues raised in the previous review by checking the git log for context.### Status
APPROVED

Previous Issues Status

• ✅ Unused import hashlib in differ.py - RESOLVED: The unused hashlib import was removed at line 3. The module correctly delegates to compute_chunk_hash() from models.py, so the direct import is no longer needed.
• ✅ Stale docstring in notes.py - RESOLVED: The docstring at line 58 was updated from "Prepend context header to content field" to "Store context header separately from content in Chunk model", which accurately reflects the actual behavior (content and context_header are stored as separate fields in the Chunk constructor at lines 84-91).

New Issues Found

Critical (Must Fix)

None

High Priority (Should Fix)

None

Advisory (Out of Scope / FYI)

None

Summary

Both previous review issues have been cleanly addressed. The unused import is removed and the docstring now accurately describes the code's behavior. The changes are minimal and correct. No new issues introduced.

---

Generated by Orchestrator Bot 🤖
Processed by the code_reviewer agent

[tinkermonkey]

🔄 Review Cycle Complete

Status: APPROVED
Iterations: 2/5
Maker: Senior Software Engineer
Reviewer: Code Reviewer

Review approved after 2 iteration(s)

---

Automated review cycle by Claude Code Orchestrator