fix(dsl): handle citation contexts without source text - #2488
Conversation
ErenAta16
left a comment
There was a problem hiding this comment.
Correct fix, and in the right place.
Worth spelling out why the guard is needed at all given there's already one two lines up: if info.context is None: return self covers "no validation context at all", but not "context supplied without a context key". In that second case text_chunks comes back None from the .get() and flows into get_spans -> _get_span -> regex.search(pattern, None):
regex.search('(quote){e<=0}', None) -> TypeError: expected string or buffer
and even if that were survived, text_chunks[span[0]:span[1]] on the next line would fail on None too. So both downstream uses assume a string, and returning early is the right call rather than patching either one.
Returning self unchanged is also the behavior that matches the existing contract: no source text means there's nothing to validate the quotes against, so leaving substring_quotes as the model produced them is consistent with the info.context is None path right above.
Test covers the exact shape (non-empty citation list, context dict missing the key), which is what distinguishes this from the already-handled None case.
Heads up that #2492 is the same one-line fix, opened a day later and without a test. This one is the more complete of the two.
|
Correction to my review above, where I said this was "the more complete of the two". There are four PRs for #2459, not two: #2460 (Jul 18, by the issue author, with a test), #2471 (closed), this one, and #2492. My check only covered recently-created PRs and missed the earlier ones. Your version is still the only one carrying a CHANGELOG entry, and the fix and test are both right. But #2460 predates this by eight days and also has a test, so it has priority on ordering. Comparison table on #2460. |
|
Consolidated and shipped in #2495. Closing this focused patch as superseded; thank you for the contribution. |
What\n\nPrevents CitationMixin validation from crashing when validation_context is present but does not include the expected context source text.\n\nCloses #2459.\n\n## Changes\n\n- Return citations unchanged when validation context has no context key.\n- Add a regression test for a non-empty citation list and missing source text.\n- Document the fix in the changelog.\n\n## Testing\n\n- uv run pytest tests/coverage/test_dsl_small_coverage.py\n- uv run ruff check instructor/v2/dsl/citation.py tests/coverage/test_dsl_small_coverage.py\n- uv run ruff format --check instructor/v2/dsl/citation.py tests/coverage/test_dsl_small_coverage.py