From da628ae0a166d05076647143acc04de16e42ab64 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Fri, 4 Sep 2026 12:12:26 -0600 Subject: [PATCH 1/3] feat: pin dictionary prose as retrievable context chunks The dictionary already turns its prose into retrieval chunks in both packages, but nothing held the two to the same answer. Add a dictionary_context_chunks section to the shared context_layer fixture and a runner in each suite, driving each package's own reader from the same parsed shape. The fixture exposed one divergence: R named its table chunks after their tables, because vapply() over names() carries them, and those names rode into the layer's documents. Chunks are text, so drop the names. Neither call site read them. Governed definitions carry no case. The two packages compile them at different constructors, so they disagree on whether a dictionary that is not attached to a source has any; definitions.json pins the chunk text. Python needs no new chunking function: the dictionary owns it as DataDictionary.context_chunks(). What was missing was the guard for a source with no dictionary at all, which is what _dictionary_chunks() adds and what folding source prose into a layer needs next. --- pkg-py/src/commons/_context_layer.py | 15 ++++ pkg-py/tests/test_context_layer.py | 18 +++- pkg-r/R/context-layer.R | 4 +- .../fixtures/shared/context_layer.json | 85 ++++++++++++++++++- pkg-r/tests/testthat/test-context-layer.R | 19 +++++ tests/shared/context_layer.json | 85 ++++++++++++++++++- 6 files changed, 222 insertions(+), 4 deletions(-) diff --git a/pkg-py/src/commons/_context_layer.py b/pkg-py/src/commons/_context_layer.py index a9251a30..07386987 100644 --- a/pkg-py/src/commons/_context_layer.py +++ b/pkg-py/src/commons/_context_layer.py @@ -12,11 +12,15 @@ import re import threading from collections.abc import Iterable +from typing import TYPE_CHECKING from raghilda.chunker import MarkdownChunker from raghilda.document import MarkdownDocument from raghilda.store import DuckDBStore +if TYPE_CHECKING: + from ._data_dictionary import DataDictionary + __all__ = ["ContextLayer", "context_layer"] # Frontmatter carries file metadata (e.g. provenance) meant for maintainers, @@ -111,6 +115,17 @@ def search(self, query: str, top_k: int = 3) -> list[str]: ] +def _dictionary_chunks(dictionary: DataDictionary | None) -> list[str]: + """The dictionary's retrievable prose, or nothing when there is none. + + A source without a dictionary is ordinary, so the absence is handled + here rather than by every caller. + """ + if dictionary is None: + return [] + return dictionary.context_chunks() + + def context_layer( files: Iterable[str | os.PathLike[str]] = (), ) -> ContextLayer: diff --git a/pkg-py/tests/test_context_layer.py b/pkg-py/tests/test_context_layer.py index 3e816d13..4be2dcc9 100644 --- a/pkg-py/tests/test_context_layer.py +++ b/pkg-py/tests/test_context_layer.py @@ -5,7 +5,8 @@ from raghilda.store import DuckDBStore from commons import ContextLayer, context_layer -from commons._context_layer import strip_frontmatter +from commons._context_layer import _dictionary_chunks, strip_frontmatter +from commons._data_dictionary import DataDictionary from ._shared import load_shared_fixture @@ -232,3 +233,18 @@ def counting_create(*args, **kwargs): assert builds == 1 assert all(result == results[0] for result in results) + + +# An empty case list would make the parametrized test below vacuously pass. +def test_the_dictionary_chunk_fixture_is_not_empty(): + assert SHARED["dictionary_context_chunks"]["cases"] + + +@pytest.mark.parametrize( + "case", SHARED["dictionary_context_chunks"]["cases"], ids=lambda c: c["name"] +) +def test_dictionary_context_chunks_shared_cases(case): + spec = case["dictionary"] + dictionary = None if spec is None else DataDictionary.model_validate(spec) + + assert _dictionary_chunks(dictionary) == case["expected"] diff --git a/pkg-r/R/context-layer.R b/pkg-r/R/context-layer.R index a0dab84d..581c31d4 100644 --- a/pkg-r/R/context-layer.R +++ b/pkg-r/R/context-layer.R @@ -97,7 +97,9 @@ dictionary_context_chunks <- function(dictionary) { glossary, definition_context_chunks(dictionary) ) - chunks[nzchar(chunks)] + # vapply() names the table chunks after their tables; the chunks are just + # text, and the names would ride along into the layer's documents. + unname(chunks[nzchar(chunks)]) } # Frontmatter carries file metadata (e.g. provenance) meant for maintainers, diff --git a/pkg-r/tests/testthat/fixtures/shared/context_layer.json b/pkg-r/tests/testthat/fixtures/shared/context_layer.json index c9cf2f7c..e560a4a0 100644 --- a/pkg-r/tests/testthat/fixtures/shared/context_layer.json +++ b/pkg-r/tests/testthat/fixtures/shared/context_layer.json @@ -1,5 +1,5 @@ { - "description": "The context layer's text handling: what frontmatter is stripped before indexing. Shared by pkg-r and pkg-py. The source is tests/shared/context_layer.json; the copy under pkg-r/tests/testthat/fixtures/shared/ is generated by scripts/sync-shared-fixtures.sh. Edit the source and re-run that script. Retrieval ranking is deliberately absent: the two BM25 engines score differently, and that difference is implementation detail.", + "description": "The context layer's text handling: what frontmatter is stripped before indexing, and how a data dictionary's prose becomes retrievable chunks. Shared by pkg-r and pkg-py. The source is tests/shared/context_layer.json; the copy under pkg-r/tests/testthat/fixtures/shared/ is generated by scripts/sync-shared-fixtures.sh. Edit the source and re-run that script. Retrieval ranking is deliberately absent: the two BM25 engines score differently, and that difference is implementation detail.", "strip_frontmatter": { "description": "Frontmatter carries file metadata meant for maintainers, not the model, so it is removed before the document reaches the store. Only a fence that opens on the very first line counts. A '---' in the body is a thematic break and must survive, or a document would lose everything above it.", "cases": [ @@ -49,5 +49,88 @@ "expected": "intro\n---\na: 1\n---\nbody" } ] + }, + "dictionary_context_chunks": { + "description": "Dictionary prose doubles as searchable context, cut at natural YAML boundaries (the dataset details, each table's prose, each glossary term) so retrieval returns coherent units rather than fragments. Order is dataset details, then tables in declaration order, then glossary terms, then governed definitions. A table with no prose contributes no chunk, and empty strings are dropped. Column-level content stays out: first touch owns it, and indexing it would pay for a second copy the agent already has. Governed definitions carry no case here, because the two implementations compile them at different constructors and so disagree on whether an unattached dictionary has any; definitions.json pins the chunk text itself.", + "cases": [ + { + "name": "details, tables, and glossary in order", + "dictionary": { + "details": "Orders from the retail system.", + "tables": { + "orders": { + "description": "One row per order.", + "details": "Excludes cancellations." + }, + "customers": { + "description": "One row per customer.", + "details": null + } + }, + "glossary": { + "AOV": "Average order value.", + "GMV": "Gross merchandise value." + } + }, + "expected": [ + "Orders from the retail system.", + "Table `orders`: One row per order.\n\nExcludes cancellations.", + "Table `customers`: One row per customer.", + "AOV: Average order value.", + "GMV: Gross merchandise value." + ] + }, + { + "name": "a table with no prose contributes no chunk", + "dictionary": { + "details": null, + "tables": { + "orders": { + "description": null, + "details": null + } + }, + "glossary": {} + }, + "expected": [] + }, + { + "name": "column-level prose is not indexed", + "dictionary": { + "details": null, + "tables": { + "orders": { + "description": "One row per order.", + "details": null, + "columns": { + "revenue": { + "description": "Line revenue, net of returns." + } + } + } + }, + "glossary": {} + }, + "expected": [ + "Table `orders`: One row per order." + ] + }, + { + "name": "an absent dictionary yields no chunks", + "dictionary": null, + "expected": [] + }, + { + "name": "details alone is enough", + "dictionary": { + "details": "Orders from the retail system.", + "tables": {}, + "glossary": {} + }, + "expected": [ + "Orders from the retail system." + ] + } + ] } } diff --git a/pkg-r/tests/testthat/test-context-layer.R b/pkg-r/tests/testthat/test-context-layer.R index 7ebec0ed..c3d1feab 100644 --- a/pkg-r/tests/testthat/test-context-layer.R +++ b/pkg-r/tests/testthat/test-context-layer.R @@ -10,6 +10,25 @@ test_that("strip_frontmatter matches the shared cases", { } }) +test_that("dictionary_context_chunks matches the shared cases", { + cases <- shared_fixture("context_layer")$dictionary_context_chunks$cases + # An empty list would make the loop below vacuously succeed. + expect_gt(length(cases), 0) + + for (case in cases) { + dictionary <- if (is.null(case$dictionary)) { + NULL + } else { + new_data_dictionary(case$dictionary) + } + expect_identical( + dictionary_context_chunks(dictionary), + as.character(unlist(case$expected)), + info = case$name + ) + } +}) + test_that("context_layer indexes files and finds relevant chunks", { path <- withr::local_tempfile(fileext = ".md") writeLines( diff --git a/tests/shared/context_layer.json b/tests/shared/context_layer.json index c9cf2f7c..e560a4a0 100644 --- a/tests/shared/context_layer.json +++ b/tests/shared/context_layer.json @@ -1,5 +1,5 @@ { - "description": "The context layer's text handling: what frontmatter is stripped before indexing. Shared by pkg-r and pkg-py. The source is tests/shared/context_layer.json; the copy under pkg-r/tests/testthat/fixtures/shared/ is generated by scripts/sync-shared-fixtures.sh. Edit the source and re-run that script. Retrieval ranking is deliberately absent: the two BM25 engines score differently, and that difference is implementation detail.", + "description": "The context layer's text handling: what frontmatter is stripped before indexing, and how a data dictionary's prose becomes retrievable chunks. Shared by pkg-r and pkg-py. The source is tests/shared/context_layer.json; the copy under pkg-r/tests/testthat/fixtures/shared/ is generated by scripts/sync-shared-fixtures.sh. Edit the source and re-run that script. Retrieval ranking is deliberately absent: the two BM25 engines score differently, and that difference is implementation detail.", "strip_frontmatter": { "description": "Frontmatter carries file metadata meant for maintainers, not the model, so it is removed before the document reaches the store. Only a fence that opens on the very first line counts. A '---' in the body is a thematic break and must survive, or a document would lose everything above it.", "cases": [ @@ -49,5 +49,88 @@ "expected": "intro\n---\na: 1\n---\nbody" } ] + }, + "dictionary_context_chunks": { + "description": "Dictionary prose doubles as searchable context, cut at natural YAML boundaries (the dataset details, each table's prose, each glossary term) so retrieval returns coherent units rather than fragments. Order is dataset details, then tables in declaration order, then glossary terms, then governed definitions. A table with no prose contributes no chunk, and empty strings are dropped. Column-level content stays out: first touch owns it, and indexing it would pay for a second copy the agent already has. Governed definitions carry no case here, because the two implementations compile them at different constructors and so disagree on whether an unattached dictionary has any; definitions.json pins the chunk text itself.", + "cases": [ + { + "name": "details, tables, and glossary in order", + "dictionary": { + "details": "Orders from the retail system.", + "tables": { + "orders": { + "description": "One row per order.", + "details": "Excludes cancellations." + }, + "customers": { + "description": "One row per customer.", + "details": null + } + }, + "glossary": { + "AOV": "Average order value.", + "GMV": "Gross merchandise value." + } + }, + "expected": [ + "Orders from the retail system.", + "Table `orders`: One row per order.\n\nExcludes cancellations.", + "Table `customers`: One row per customer.", + "AOV: Average order value.", + "GMV: Gross merchandise value." + ] + }, + { + "name": "a table with no prose contributes no chunk", + "dictionary": { + "details": null, + "tables": { + "orders": { + "description": null, + "details": null + } + }, + "glossary": {} + }, + "expected": [] + }, + { + "name": "column-level prose is not indexed", + "dictionary": { + "details": null, + "tables": { + "orders": { + "description": "One row per order.", + "details": null, + "columns": { + "revenue": { + "description": "Line revenue, net of returns." + } + } + } + }, + "glossary": {} + }, + "expected": [ + "Table `orders`: One row per order." + ] + }, + { + "name": "an absent dictionary yields no chunks", + "dictionary": null, + "expected": [] + }, + { + "name": "details alone is enough", + "dictionary": { + "details": "Orders from the retail system.", + "tables": {}, + "glossary": {} + }, + "expected": [ + "Orders from the retail system." + ] + } + ] } } From 7eb264a4def1de5ac4b2341e459eacd5bca70b68 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Sun, 6 Sep 2026 18:58:17 -0600 Subject: [PATCH 2/3] test: pin empty-string and sequence-shape chunk cases Review follow-ups. Restore the empty-fixture guard on the strip_frontmatter runner: tests/shared/README.md requires it of every runner that enumerates cases. Pin that empty strings are dropped from dictionary chunks, and pin table declaration order through the sequence shape that authored data-dict.yaml files use, rather than only the pre-keyed mapping. --- .../fixtures/shared/context_layer.json | 39 +++++++++++++++++++ pkg-r/tests/testthat/test-context-layer.R | 2 + tests/shared/context_layer.json | 39 +++++++++++++++++++ 3 files changed, 80 insertions(+) diff --git a/pkg-r/tests/testthat/fixtures/shared/context_layer.json b/pkg-r/tests/testthat/fixtures/shared/context_layer.json index e560a4a0..250fe39c 100644 --- a/pkg-r/tests/testthat/fixtures/shared/context_layer.json +++ b/pkg-r/tests/testthat/fixtures/shared/context_layer.json @@ -130,6 +130,45 @@ "expected": [ "Orders from the retail system." ] + }, + { + "name": "empty strings are dropped", + "dictionary": { + "details": "", + "tables": { + "orders": { + "description": "One row per order.", + "details": null + } + }, + "glossary": {} + }, + "expected": [ + "Table `orders`: One row per order." + ] + }, + { + "name": "tables as a sequence keep declaration order", + "dictionary": { + "details": null, + "tables": [ + { + "name": "orders", + "description": "One row per order.", + "details": null + }, + { + "name": "customers", + "description": "One row per customer.", + "details": null + } + ], + "glossary": {} + }, + "expected": [ + "Table `orders`: One row per order.", + "Table `customers`: One row per customer." + ] } ] } diff --git a/pkg-r/tests/testthat/test-context-layer.R b/pkg-r/tests/testthat/test-context-layer.R index c3d1feab..6c7c6c3f 100644 --- a/pkg-r/tests/testthat/test-context-layer.R +++ b/pkg-r/tests/testthat/test-context-layer.R @@ -1,5 +1,7 @@ test_that("strip_frontmatter matches the shared cases", { cases <- shared_fixture("context_layer")$strip_frontmatter$cases + # An empty list would make the loop below vacuously succeed. + expect_gt(length(cases), 0) for (case in cases) { expect_identical( diff --git a/tests/shared/context_layer.json b/tests/shared/context_layer.json index e560a4a0..250fe39c 100644 --- a/tests/shared/context_layer.json +++ b/tests/shared/context_layer.json @@ -130,6 +130,45 @@ "expected": [ "Orders from the retail system." ] + }, + { + "name": "empty strings are dropped", + "dictionary": { + "details": "", + "tables": { + "orders": { + "description": "One row per order.", + "details": null + } + }, + "glossary": {} + }, + "expected": [ + "Table `orders`: One row per order." + ] + }, + { + "name": "tables as a sequence keep declaration order", + "dictionary": { + "details": null, + "tables": [ + { + "name": "orders", + "description": "One row per order.", + "details": null + }, + { + "name": "customers", + "description": "One row per customer.", + "details": null + } + ], + "glossary": {} + }, + "expected": [ + "Table `orders`: One row per order.", + "Table `customers`: One row per customer." + ] } ] } From 6cbba98e27823abb192a0b515dff0d1c6d0ae111 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Sun, 6 Sep 2026 19:06:56 -0600 Subject: [PATCH 3/3] test: drop the empty-fixture guards from the R runners Simon pushed back on the strip_frontmatter runner's guard on #240, and the dictionary_context_chunks runner re-added the same pattern; drop it from both so the file stays consistent with the reviewed state. The Python suite keeps its standalone not-empty tests, the shape #240 approved. --- pkg-r/tests/testthat/test-context-layer.R | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pkg-r/tests/testthat/test-context-layer.R b/pkg-r/tests/testthat/test-context-layer.R index 6c7c6c3f..2b5560fc 100644 --- a/pkg-r/tests/testthat/test-context-layer.R +++ b/pkg-r/tests/testthat/test-context-layer.R @@ -1,7 +1,5 @@ test_that("strip_frontmatter matches the shared cases", { cases <- shared_fixture("context_layer")$strip_frontmatter$cases - # An empty list would make the loop below vacuously succeed. - expect_gt(length(cases), 0) for (case in cases) { expect_identical( @@ -14,8 +12,6 @@ test_that("strip_frontmatter matches the shared cases", { test_that("dictionary_context_chunks matches the shared cases", { cases <- shared_fixture("context_layer")$dictionary_context_chunks$cases - # An empty list would make the loop below vacuously succeed. - expect_gt(length(cases), 0) for (case in cases) { dictionary <- if (is.null(case$dictionary)) {