Skip to content

fix(types): JSON-quote string-shaped built-in terms inside containers - #2015

Open
bharadwaj-pendyala wants to merge 1 commit into
dottxt-ai:mainfrom
bharadwaj-pendyala:fix/json-quote-builtin-terms-in-containers
Open

bharadwaj-pendyala wants to merge 1 commit into
dottxt-ai:mainfrom
bharadwaj-pendyala:fix/json-quote-builtin-terms-in-containers

Conversation

@bharadwaj-pendyala

@bharadwaj-pendyala bharadwaj-pendyala commented Aug 17, 2026

Copy link
Copy Markdown

Closes #1962. Supersedes #1961, which fixed date/time/datetime only.

Problem

list[types.email] generates [a@b.com]. That is not JSON.

import re
import outlines.types as types
from outlines.types.dsl import to_regex, python_types_to_terms

pattern = to_regex(python_types_to_terms(list[types.email]))
re.fullmatch(pattern, '[a@b.com]')      # matches, and json.loads rejects it
re.fullmatch(pattern, '["a@b.com"]')    # no match, valid JSON rejected

_ensure_json_quoted quotes a bare Regex only when called with quote_regex=True, which happens for Dict keys and nowhere else (src/outlines/types/dsl.py:884 before this change). So every built-in term reaching a list item, a tuple item or a dict value came back unquoted. Same for dict[str, types.email], which generates {"k":a@b.com}.

The quiet case is worse than the loud one. json.loads reads a lone control character between brackets as whitespace, so list[types.newline] generates text a parser accepts as an empty array:

json.loads('[\n]')     # []

The caller asked for one element, the generation produced one, the parse yields zero, and nothing raises.

Solution

Quote every built-in outlines.types term except the ones JSON reads as scalars: integer, number, boolean, digit, latitude, longitude, plus string, whose pattern carries the quotes already.

Listing the exceptions rather than the terms to quote is a deliberate choice, and I would rather state it than leave it open. An allowlist has to be extended every time a term is added and fails open when someone forgets, which is how the missing quotes arrived. A denylist fails the other way: a new term gets quoted like the rest, and if it turns out to need more than quoting that is a visible bug instead of silent invalid JSON. @ErenAta16 argued the same case on the issue and did the work of separating the escaping treatments; @Sanjays2402 spotted the gap during review of #1961.

Terms are matched on their pattern, not by identity. Identity looks tighter but breaks under pickle, copy.deepcopy and any reconstruction, which would silently restore the old output. Matching the pattern also means a Regex(r"\w") the caller wrote is treated as types.char and quoted, since nothing distinguishes them; test_regex_colliding_with_a_builtin_is_quoted pins that. Any other user-supplied Regex keeps its current bare behaviour, because nothing records whether it stands for a JSON string or a number.

The outlines.types.locale.us terms (zip_code, phone_number) are included. They have the same bug and live in a submodule.

24 built-in terms are now quoted. 17 are fully correct. The rest need escaping on top of quoting and are out of scope here:

Term Why quoting is not enough
newline, whitespace admit raw control characters
sentence, paragraph .* admits the backslash and a bare quote, so they want json.dumps, not a hand-written escape list
datetime its separator is \s, so a tab passes
email the quoted-local-part branch admits a "

For newline and whitespace quoting still helps, because it converts the silent bad parse into a raised error:

json.loads('[\n]')      # [] on main
json.loads('["\n"]')    # Invalid control character at: line 1 column 3

isbn is quoted with the rest and nothing observable changes: its four $ anchors sit inside lookaheads, so once the term is embedded in \[(...)\] the alternation is unsatisfiable and list[types.isbn] matches nothing either way. Dropping the anchors changes what bare isbn accepts, so it needs its own call.

Scope

Left out on purpose:

  • Escaping for the six terms above, and the isbn anchors. Each needs a different treatment and sentence/paragraph want json.dumps rather than the mechanism used here.
  • latitude and longitude emit a leading + that JSON rejects, and boolean emits True rather than true. Those are separate bugs from the quoting one and the denylist keeps all three bare either way.
  • _ensure_json_quoted still does not recurse into Sequence, Optional or the quantifier nodes, so list[optional(types.email)] stays unquoted. That predates this change and test_ensure_json_quoted_sequence_passthrough freezes it.

Validation

Base commit 7d06847.

  • pytest tests/types : 389 passed, up from 346 on 7d06847. 43 tests added; 34 of them fail against the 7d06847 source and pass here, and the 9 that pass on both pin the scalar and user-supplied-Regex behaviour that must not change.
  • ruff check --config=pyproject.toml src/outlines/types/dsl.py tests/types/test_dsl.py (0.9.1, the pinned pre-commit version): all checks passed.
  • mypy --allow-redefinition src/outlines/types/dsl.py (1.14.1): no issues found.
  • test_container_of_builtin_generates_parseable_json runs the generated regex against a real sample for 13 terms and asserts json.loads accepts the quoted form and the regex rejects the bare one, so the tests check the output rather than restating the wrapper.
  • Verified by hand on 7d06847 and on this branch: pickle.loads(pickle.dumps(list[types.email])) and copy.deepcopy(types.email) both quote correctly here and did not under an identity check.

The rest of the suite needs transformers, which I could not install locally; tests/test_exceptions.py and tests/test_cache.py report 2 failures and 1 error on 7d06847 unchanged, from a missing async pytest plugin rather than this change.

AI disclosure

This change was AI-assisted, per CONTRIBUTING.md. I reviewed every line, ran every command quoted above myself, and can explain the whole diff.

@bharadwaj-pendyala

Copy link
Copy Markdown
Author

The empty checks on this PR are the first-time-contributor workflow-approval gate, not a problem with the branch. Worth saying here so nobody reads the blank list as a red CI.

$ gh api repos/dottxt-ai/outlines/commits/6beef9d7/check-runs --jq .total_count
0

Both workflows on head 6beef9d came back conclusion=action_required and registered zero check-runs, so the PR page has nothing to display. @ErenAta16 hit the same thing elsewhere and pointed out the part that matters: pushing another commit does not clear it, it just queues a second run in the same state. A maintainer approving the run is the only thing that moves it.

Since CI cannot speak for the branch yet, here is what it does locally. On main:

list[date]:  bare '[2026-08-23]' -> True,  quoted '["2026-08-23"]' -> False
list[email]: bare '[a@b.com]'    -> True,  quoted '["a@b.com"]'    -> False

The generator accepts the string JSON rejects and rejects the string JSON accepts. On this branch, both flip:

list[date]:     bare -> False, quoted -> True
list[time]:     bare -> False, quoted -> True
list[datetime]: bare -> False, quoted -> True
list[email]:    bare -> False, quoted -> True

One scope note, since #1960 has come up as a candidate for a combined dsl.py PR. The date/time/datetime case in #1960 is the same _ensure_json_quoted mechanism as #1962, and the scalar denylist here covers it, which the list[date] line above shows directly. So #1960 needs nothing further once this lands. #1942 (bool as True/False) and #1971 (Optional[T] as None) are separate code paths and are untouched by this diff, so they still want their own fix.

Happy to rebase whenever, though per the above that will not start CI on its own.

@bharadwaj-pendyala

Copy link
Copy Markdown
Author

Correcting an omission in the description above, since it matters for how this gets reviewed.

@feiiiiii5 opened #1975 on 1 August with the same title and the same goal, sixteen days before this PR. I reviewed that branch on 10 August, found three terms falling through its allowlist, and feiiiiii5 fixed all three the next day in 052726b1. Then I opened this PR without linking #1975 or saying that I had reviewed it. That was wrong of me and a maintainer should not have to discover it from the timeline.

The "allowlist fails open" argument in my description above is weaker than I wrote it. 052726b1 added test_every_builtin_regex_term_is_classified, which walks dir(types) and asserts nothing is left unclassified. I ran it on pr1975:

tests/types/test_dsl.py::test_every_builtin_regex_term_is_classified  PASSED
tests/types/test_dsl.py  82 passed in 2.30s

That closes the hole I said an allowlist could not close, at least for terms exported from outlines.types.

Where the two branches actually differ. Same probe on main at 7d06847, on #1975 at 052726b1, and on this branch at 6beef9d. Each row is whether list[term] matches the quoted form:

                                     main    #1975   #2015
list[types.email]                    False   True    True
list[locale.us.zip_code]             False   False   True
list[locale.us.phone_number]         False   False   True
list[copy.deepcopy(types.email)]     False   False   True
list[pickle roundtrip of email]      False   False   True
list[Regex(types.email.pattern)]     False   False   True

The first five are coverage this branch has and #1975 does not. dir(types) does not reach the outlines.types.locale.us submodule, and identity matching does not survive deepcopy or pickle.

The sixth row is the cost of getting the other five, and it goes the other way. Matching on the pattern string means a Regex a user wrote themselves gets quoted when it happens to be spelled the same as a built-in. #1975 pins the opposite behaviour on purpose in test_e2e_user_regex_term_still_bare_in_containers, and I would not call that test wrong. Nothing in a Regex records whether it stands for a JSON string, so both readings are defensible and it is a maintainer's call rather than mine.

What I would suggest. #1975 has been running CI since 1 August and this branch has never run any, since both workflows on 6beef9d came back action_required with zero check-runs. If the maintainers prefer #1975, the locale submodule and the reconstruction cases are each a few lines on that branch and I am happy to send them to @feiiiiii5 or open them as a follow-up, and I will close this PR. If the denylist and pattern matching are the wanted shape, then this one needs the workflow approved before it can be judged on anything except reading.

Either outcome is fine by me. I would rather that be decided in the open than have two branches sitting here doing the same job.

list[types.email] generated [a@b.com], which does not parse as JSON.
_ensure_json_quoted only quoted a bare Regex for Dict keys, so every
built-in term reaching a list item, tuple item or dict value came back
unquoted.

Quote every built-in outlines.types term except the JSON scalars
(integer, number, boolean, digit, latitude, longitude) and string, which
carries its own quotes. Terms are matched on their pattern, so a copy or
a rebuilt term is treated the same as the module-level one, and the
outlines.types.locale.us terms are covered as well.

@ErenAta16 ErenAta16 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your 24 August comment gave away more than it had to. The retraction is wrong, and
since it is the argument a maintainer would weigh when choosing between the two
branches, it is worth putting straight.

You credited test_every_builtin_regex_term_is_classified with closing the hole.
It does not, because of what it asserts:

assert not builtins - (set(BARE_TERMS) | set(QUOTED_TERMS) | {"string"})

That is a membership check. It proves no built-in is left unclassified. It says
nothing about whether a term landed in the right tuple, so a string-shaped term
placed in BARE_TERMS passes it exactly as cleanly as a correct one. No
completeness test over dir(types) can catch a wrong classification, only a
missing one.

That distinction is not academic here. I called _ensure_json_quoted on every
built-in Regex across main, #1975 and this branch:

                                              main   #1975   #2015
integer number boolean digit lat long         bare   bare    bare
whitespace newline sentence paragraph         bare   bare    quoted
date time datetime                            bare   bare    quoted
email uuid4 ipv4 isbn semver slug hex e164    bare   quoted  quoted

Seven terms are classified bare on #1975 while passing its own completeness test.
Four of them are the control-character group, which that branch leaves bare
deliberately and documents in
test_e2e_control_char_builtin_terms_left_bare_in_containers, so that part is a
judgement call rather than a miss, and I have said so on #1975. The temporal row
is not:

              bare              quoted
2024-01-01    JSONDecodeError   ['2024-01-01']
12:30:00      JSONDecodeError   ['12:30:00']

So your original framing was right, just for a different reason than the one you
gave. An allowlist does not fail by leaving a term out, that test does close that
door. It fails by putting one on the wrong side, and nothing on that branch checks
for it.

This also changes the shape of the offer at the end of your comment. Handing
#1975 the locale submodule and the reconstruction cases is a few lines, as you
say, but whoever takes that route also has to move date, time and datetime
across, and that is #1960 rather than a tidy-up. Worth naming, because "a few
lines of follow-up" and "and it closes a second open issue" are different asks.

Your reachability rows and this classification row are independent, incidentally.
Yours is about which objects get recognised as a built-in, mine is about what
happens once one is. A branch can be right on either and wrong on the other, so a
maintainer picking between these two is choosing on both axes, not one.

On the omission you corrected: noted, and I would not have raised it. Saying it
yourself before anyone found it is the reason this thread is easy to read.

@bharadwaj-pendyala

Copy link
Copy Markdown
Author

You're right, I retracted too much. tests/types/test_dsl.py:1299 asserts not builtins - (set(BARE_TERMS) | set(QUOTED_TERMS) | {"string"}), so the test only requires a term in one tuple. On pr1975, date, time and datetime are in BARE_TERMS at src/outlines/types/dsl.py:889-891, and test_every_builtin_regex_term_is_classified passes. list[date] still generates unquoted, and json.loads('[2024-01-01]') raises JSONDecodeError: Expecting ',' delimiter: line 1 column 6.

The test closes the gap I claimed it left, and dir(types) doesn't show which tuple owns a term. The locale submodule and reconstruction cases belong in #1960, not #1975.

@bharadwaj-pendyala

Copy link
Copy Markdown
Author

@RobinPicard CI has never run here: both check suites on d3120fb are at action_required, waiting on workflow approval for a first-time contributor.

#1975 was closed in favor of this one on Sep 2, so it's the only open branch covering the date/time/datetime family. It closes #1960 alongside #1962 since the denylist quotes all three.

@ErenAta16 ErenAta16 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming the CI reading independently, since it is the kind of claim a
maintainer should not have to take on trust before spending a click on it.

On d3120fb:

check-suites: 2
  GitHub Actions  status=completed  conclusion=action_required  latest_check_runs_count=0
  GitHub Actions  status=completed  conclusion=action_required  latest_check_runs_count=0
check-runs total: 0

action_required with zero runs is the first-time-contributor workflow gate, not
a failure and not something a push can clear. Nothing has executed on this branch,
so there is no green or red to read yet, and no amount of rebasing changes that.

Two other things worth having in one place, both checked rather than recalled.

#1975 was closed unmerged on 2 September, so this is now the only open branch
covering the string-shaped terms. The row is no longer a choice between two.

And the coverage is what the description claims. I called _ensure_json_quoted
on every built-in Regex in outlines.types at this head:

quoted (15)  whitespace newline sentence paragraph date time datetime
             email uuid4 ipv4 isbn semver slug hex_color e164
bare   (6)   integer number boolean digit latitude longitude

The six left bare are the number-shaped ones, which is correct, and the temporal
row is quoted, which is what #1960 asks for. #1960 and #1962 are both still open,
so approving the workflow here is the step that lets one branch close two issues.

@RobinPicard the only thing needed to move this is the workflow approval. Until
that happens neither the author nor a reviewer can tell you anything about this
branch that was not read by hand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Built-in Regex terms are not JSON-quoted inside containers (list[types.email] generates [a@b.com])

2 participants