[FIX] Resolve statement enhancement context through the chunk store - #531
Open
noel-improv wants to merge 4 commits into
Open
[FIX] Resolve statement enhancement context through the chunk store#531noel-improv wants to merge 4 commits into
noel-improv wants to merge 4 commits into
Conversation
… chunk store The post-processor read node.node.metadata['chunk']['value'] directly, so it returned nothing useful once chunk text moved out of the graph. The broad except swallowed the KeyError and handed back the original node, which made the failure silent. It now takes an optional graph_store, resolves the chunk store from it the way the traversal retriever and KeywordVSSProvider do, and fetches the text a node does not carry. One get_batch covers a whole call rather than one fetch per node. A node with no statement or no chunk text from either source is returned unchanged instead of raising into the except, and the model is not called. Without a graph_store the behaviour is what it was.
…s not exist Both said the placeholder text would be populated by StatementGraphRetriever. No such class exists in the repo. Nothing populates these nodes because nothing needs to: the caller reads the id off the metadata and discards the node, then builds its results from a graph query.
S3ChunkStoreFactory needs a graph store only to attach the in-graph fallback, but the post-processor gated the whole resolution on one, so a caller passing none got no chunk store at all. Every construction site in the repo and in the docs passes none, which left the fix inert for the case it was written for. Resolve the store when S3_CHUNK_STORE is set or a graph store is given, and cover the four combinations plus a misconfigured URI, which now fails at construction rather than as unenhanced statements at query time.
…ing a query Reading the chunk store was the only path in the post-processor that could propagate: every other failure returns the node unchanged, but an S3 or graph error from the batch read took the whole query down. It now degrades the same way as the rest. Also in this pass: - 'chunk' set to None rather than absent no longer raises. One accessor now answers for the chunk metadata, its id and its text. - An enhanced node keeps its metadata, id and exclusion lists instead of being rebuilt from four keys, so it no longer drops what retrievers attach and no longer emits 'source': None for a node that had no source. - Statements sharing a chunk cost one fetch instead of one per statement. dict.fromkeys rather than a set, so the request order stays reproducible. - Empty chunk text is treated as absent and resolved from the store, matching what both stores count as a hit. - enhance_statement, which is public, resolves context from the store it holds when called without a batch map. After a batch has run and missed, it does not ask again per node.
noel-improv
marked this pull request as ready for review
September 9, 2026 23:21
noel-improv
force-pushed
the
fix/statement-enhancement-chunk-store
branch
from
September 11, 2026 00:24
5bb28ba to
58e9d7e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
StatementEnhancementPostProcessorread chunk text straight off the node:Moving chunk text out of the graph is the point of the ChunkStore work, so this path stops working once the text leaves. The traversal retrievers and
KeywordVSSProvideralready resolve throughChunkStore.get_batch(); this one did not.The failure was also not reported. Reading the key directly raises
KeyError, which the broadexceptaround the call caught before returning the original node, so a failed enhancement could not be distinguished from a successful one.Related issue: #505.
Changes
StatementEnhancementPostProcessortakes an optionalgraph_storeand resolves aChunkStorefrom it, followingtraversal_based_base_retriever.py:125andkeyword_vss_provider.py:53.get_batchper call rather than one fetch per node.chunk_cosine_search.pyanddeprecated/statement_cosine_seach.pyboth stated that their placeholder text would be populated by aStatementGraphRetriever, which does not exist in the repository. Those nodes are not populated because they do not need to be: the caller reads the id from the metadata and discards the node.Behaviour is unchanged when no
graph_storeis passed, and the class is only ever constructed by callers, so there are no internal construction sites to update.Testing
pytest)27 new tests in
tests/unit/retrieval/post_processors/test_statement_enhancement.py, covering text carried on the node, text resolved from the store, a store miss, absent chunk metadata, absent statement, and a response with no match. 820 pass acrosstests/unit/retrievalandtests/unit/storage.One pre-existing failure is unrelated and reproduces on a clean tree:
test_integ_dependency_compatibility.pyerrors withNo module named pipin this venv.Checklist
Draft: the second read path in the related issue,
get_chunks_queryinchunk_utils.pyfeeding the deprecated semantic-guided retriever, is not covered here. Whether that path is connected to the ChunkStore or removed is a team decision, so this PR covers only the first path.