Fix R definition token scope, index cap, and typeless gist, and pin all three as a shared fixture - #265
Merged
Conversation
Three defects in pkg-r/R/definitions.R, each found while building the Python registry and each pinned here as a shared fixture rather than as an R-only test. expand_definitions() resolved each token against SQL that earlier expansions had already rewritten, so a definition whose compiled SQL named another table brought that table into scope for the tokens that followed. That defeats the check keeping a bare token bound to its own table. Every token now resolves against the query as written, and substitution happens afterwards. Closes #260. definition_index_text() capped on cumsum(nchar(index)) but joined the kept lines with newlines, so the rendered index could exceed cap_chars by one character per line beyond the first, and definitions_overflow() measured the same way and could report no overflow. Both measure the joined text. definition_gist() built its head with sprintf("(%s, %s)", kind, type), which returns character(0) for a definition data-dict inferred no type for, so c() dropped the element and the gist rendered without its "(kind)" prefix. The type is omitted and the kind kept. Note this is narrower than #259 claims: the description and the compiled SQL did survive, so the model was not left with a bare token. Closes #259. tests/shared/definition-rendering.json pins all three as behaviour both packages must agree on: which queries expand and to what, the gist text, and the index under a cap. Refusals pin the fact of refusal and a reason slug rather than the message, which is idiomatic to each language. The two index cases sit on the exact boundary, so they fail against the old cumsum rule rather than passing by accident. The R suite runs the cases; the Python suite checks the fixture's integrity until #261 lands its registry, at which point it runs them too. All 13 cases already pass against that registry unchanged.
|
Preview deployed to Connect ( Deployed from commit e3442c9. |
|
Preview deployed to Connect ( Deployed from commit e3442c9. |
jat255
added a commit
that referenced
this pull request
Sep 3, 2026
The fixture landed in #265 with only the R suite executing it, and tests/shared/README.md asks for both runners to land with a fixture. The registry exists here, so Python runs the same 13 cases from the same file: which queries expand and to what, the gist, and the index under a cap. Replaces the integrity-only test that stood in for this. Refusals assert the reason the fixture names, through a map to this package's message wording, which is what the R runner does with its own. Every case passes without touching the registry. Checked by perturbing one expansion, one gist and one overflow flag in the fixture and confirming the three matching cases fail.
Both call sites pass dictionary lists, where an absent type is NULL and length(def$type) == 0 already catches it. The is.na() arm was only reachable from registry rows, which never reach definition_gist(), so drop it rather than answer the same question two ways.
simonpcouch
approved these changes
Sep 3, 2026
jat255
added a commit
that referenced
this pull request
Sep 4, 2026
The fixture landed in #265 with only the R suite executing it, and tests/shared/README.md asks for both runners to land with a fixture. The registry exists here, so Python runs the same 13 cases from the same file: which queries expand and to what, the gist, and the index under a cap. Replaces the integrity-only test that stood in for this. Refusals assert the reason the fixture names, through a map to this package's message wording, which is what the R runner does with its own. Every case passes without touching the registry. Checked by perturbing one expansion, one gist and one overflow flag in the fixture and confirming the three matching cases fail.
jat255
added a commit
that referenced
this pull request
Sep 4, 2026
…rain (#261) * feat(py): the definitions registry, token expansion and the prompt index The registry surface commons owns whatever data-dict eventually provides: {{name}} and {{table::name}} expansion, the kind index for the prompt, grain metadata for call_metrics' mixed-grain guard, and the check that a definition's table is one the source exposes. Everything consumes ExportRecord and nothing reaches into an expression parser or a typed IR. That rule is what makes replacing this with a shared data-dict interface a deletion rather than a rewrite, and it is why the registry can be built and tested before the compiler exists. The dictionary's first-touch entry and its retrieval chunks now render a table's definitions, which the reader left as a marked seam. Both show compiled SQL rather than the authored expression, since the expression is in data-dict's language and the model writes SQL. Tests run the registry against tests/shared/definitions.json as well as against hand-built records: every definition data-dict produced expands to its own compiled SQL, and each kind is one the index groups. This is the first Python code to consume that contract. * fix(py): resolve tokens against the query as written, and complete the gist Four defects, each reproduced first. Token resolution ran against SQL that earlier expansions had already rewritten, so a definition whose compiled SQL named another table brought that table into scope for a later bare token. Every token now resolves against the query as written, and substitution happens afterwards. Retrieval chunks carried only the kind and type, so a retrieved definition named a token without saying what it expands to. They carry the same gist as the first-touch entry now, which is compiled SQL and notes, never the authored expression. An absent type reached the prompt as the word "None". data-dict omits the type when no single one is inferred, so it is left out rather than printed. pkg-r loses the entire gist in this case, filed as #259. The index cap counted line lengths but not the newlines joining them, so the result could exceed the cap. It measures the joined text. * test(py): run the shared definition-rendering cases against the registry The fixture landed in #265 with only the R suite executing it, and tests/shared/README.md asks for both runners to land with a fixture. The registry exists here, so Python runs the same 13 cases from the same file: which queries expand and to what, the gist, and the index under a cap. Replaces the integrity-only test that stood in for this. Refusals assert the reason the fixture names, through a map to this package's message wording, which is what the R runner does with its own. Every case passes without touching the registry. Checked by perturbing one expansion, one gist and one overflow flag in the fixture and confirming the three matching cases fail. * fix(py): flatten prose around newlines only, per the shared contract Self-review found the gist collapsing every whitespace run while the shared contract keeps authored whitespace; the fixture now pins the rule with a double-space case. Also from that pass: - pin the qualified-token refusals (table absent, unknown name) in tests/shared/definition-rendering.json, synced copy included - restore the fixture-integrity guards dropped with test_definition_rendering_fixture.py: both sides of the index cap and a typeless gist case - cover the qualified and legacy-dotted refusal paths and multi-source build_registry/for_source - build_registry: tolerate tables = None instead of crashing past the getattr default - correct stale docstrings: the compiler is not present yet, and the registry now runs the definitions.json records
|
Cleaned up 4 preview bundle(s) on https://dogfood.team.pct.posit.it: 367498, 367510, 367515, 367516 |
|
Cleaned up 4 preview bundle(s) on https://connect.staging.pct.posit.it: 2520, 2525, 2526, 2527 |
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.
This PR fixes three bugs in
pkg-r/R/definitions.Rfound while building the Python registry (#261), and adds shared fixtures so they can't regress. Closes #259 and #260.What was broken
Token scope leak in
expand_definitions(). Each token was resolved against SQL that earlier expansions had already rewritten. If one definition's compiled SQL mentioned another table, that table became visible to the tokens that followed — defeating the check that keeps a bare token bound to its own table.Off-by-newlines in
definition_index_text(). The cap was computed withcumsum(nchar(index)), but the kept lines were joined with newlines. The rendered index could exceed the cap, anddefinitions_overflow()could report no overflow.Vanishing prefix in
definition_gist(). The head was built withsprintf("(%s, %s)", kind, type), which returnscharacter(0)when data-dict inferred no type, so the(kind, type)prefix silently disappeared.Note on #259: it's narrower than the original report. The description and compiled SQL did survive; only the prefix was lost. I verified this against main and commented on the issue. The fix still matters — the kind is prompt content, and the two packages disagreed on it.
The fixtures
tests/shared/definition-rendering.jsonpins all three fixes: which queries expand and to what, the gist text, and the index under a cap. Refusals pin the fact of refusal and a reason slug, not the exact message, since wording is idiomatic to each language. The two index cases sit exactly on the character boundary, so they fail against the oldcumsumrule rather than passing by accident.The Python tests for these fixtures land in #261. This PR carries only the fixture integrity checks; #261 adds the runner that executes the cases. All 13 cases already pass against that registry with no Python change.
What changes for users
Three inputs now behave differently:
definitions_overflow()reportsTRUEwhere it reportedFALSE.(derived) ...instead of dropping the prefix entirely.All three move in the safe direction: refuse rather than expand, truncate rather than overflow, say the kind rather than drop it.
Blast radius
expand_definitions()expand_for_run_sql()→run_sqldefinition_index_text(),definitions_overflow()definition_gist()definitions_entry_text()(first touch),definition_context_chunks()(retrieval)Why it's safe
The full suite passes with no failures. The three new tests were verified by reverting
definitions.Rto main and re-running: exactly three failures, one per test — evidence the fixture bites rather than passing vacuously. The olddefinition_gist()was run directly against a typeless definition to establish what it actually returned, which is how the #259 correction above was found.No hand-written R tests were deleted. Files under
pkg-r/tests/testthat/fixtures/shared/are generated byscripts/sync-shared-fixtures.sh.