Preserve proved boundaries in mixed-stop captured histories - #882
Conversation
bradhilton
left a comment
There was a problem hiding this comment.
McCarthy: CHANGES REQUIRED — one P2.
Reviewed 3afdc48d7f9201fda897a866acd80b0da575b8c8 against 077abf6fa7a3a51f9ff255c37f8886c6c8256082. The runtime/test files are byte-identical to frozen overlay 83af514b332d539080e3ceba87b123ca094c0806, so this finding applies to both.
P2: terminal synthetic-tail construction duplicates already-sampled tool closing markup. The newly admitted terminal synthetic_stop case obtains bounds from marked content parts and passes bounds[1] as content_end (source). For a tool call those bounds end after name/arguments, before closing markup. _tokenize_exact_projected_chat_history then appends that whole renderer tail after the complete captured output (source). If the model sampled the closer but not EOS, the closer is appended twice.
Concrete public-path reproduction: use the PR's test_length_boundary_ends_before_next_assistant_tool_prefix history, changing only its terminal sampled output from <tool>lookup{} to <tool>lookup{}</tool>; keep finish_reason='tool_calls' and no sampled EOS. Head produces:
turn 0answer§turn 1<tool>lookup{}</tool></tool>§
The extra seven tokens are unsampled ASSISTANT|OUTPUT. The earlier length boundary and captured next prompt are valid. Base has one closer (it omits the final synthetic EOS, which is separate existing behavior); the duplication is introduced here. A no-duplicate assertion passes 2/2 on base and fails 1/2 on head, with the mismatched-next-prompt negative passing on both.
Please prove the full sampled endpoint or its overlap with the renderer tail before appending only the remaining tail, or decline the exact path when that proof is unavailable. Preserve the current next-prompt/projection/unique-stop checks. Add full and partially sampled tool-closing suffix cases alongside the existing unsampled-closer case.
Evidence/reproduction: /home/brad/.local/share/mccarthy/art882-review-20260910/test_no_duplicate_closer.py, no-duplicate-{head,base}.log; run with the desired ART src on PYTHONPATH using the existing no-sync environment.
Other validation passed: 234 tokenization tests; independent reruns of the frozen 75 value / 86 LOO histories, with hashed raw captures and metadata, network connections forbidden, sampled IDs/logprobs/STOP and first-occurrence selections preserved, and only histories 4/3 and 27/11 changing as reported. Ruff/format/diff checks clean. These fixtures do not cover the sampled-closer case above. No source edits, GPU/inference, merge or deployment.
bradhilton
left a comment
There was a problem hiding this comment.
Minsky review — CHANGES REQUIRED at 3afdc48d7f9201fda897a866acd80b0da575b8c8 (base 077abf6f). I concur with McCarthy's P2 and reproduced it independently; the same hold applies to the byte-identical frozen overlay 83af514b.
Reproduction (own probe, PR's test_length_boundary_ends_before_next_assistant_tool_prefix history, only the terminal sampled output varied, finish_reason="tool_calls", no sampled EOS):
| terminal sampled output | base 077abf6f tokens |
head 3afdc48d tokens |
head unsampled OUTPUT tokens |
|---|---|---|---|
<tool>lookup{} (closer unsampled) |
33 | 41 | 8 |
<tool>lookup{}</tool> (closer fully sampled) |
40 | 48 | 8 |
<tool>lookup{}</to (closer partially sampled) |
37 | 45 | 8 |
At head the renderer tail (</tool>§, 8 tokens) is appended after the complete captured output in all three cases, so the fully sampled case renders …</tool></tool>§ and the partial case …</to</tool>§. Sampled counts and logprobs are intact (20/27/24), but 7 unsampled ASSISTANT|OUTPUT closer tokens are duplicated in the sampled-closer rows. Root cause as McCarthy describes: in the newly admitted terminal synthetic_stop branch, content_end comes from the marked tool name/argument bounds (bounds[1]), which end before any closing markup, while _tokenize_exact_projected_chat_history appends the whole tail from that point after the full captured output. Base does not duplicate (it also omits the synthetic EOS on this history, which is separate pre-existing behaviour).
Fix shape: prove the overlap between the captured output's suffix and the renderer segment from content_end to the boundary (or the sampled endpoint itself), append only the remaining tail, and decline the exact path when that proof is unavailable, keeping the existing next-prompt/projection/unique-STOP guards. Add regressions for fully and partially sampled closers next to the existing unsampled-closer case.
Otherwise verified at this head: tests/unit/trajectories 463 passed (the full test_tokenize.py 234 included); Ruff check and format clean. Negative control of the head's test module against 077abf6f: the two terminal-synthetic-stop cases and the matched tool-prefix case fail on base while the mismatched negative passes, as intended. The value-fix half (terminal synthetic stop no longer vetoing an earlier proven length boundary) and the LOO boundary choice (next assistant span start before content-part bounds) both read correctly and are not the source of the P2; the P2 is the tail construction for sampled closers in the newly admitted terminal case.
Draft and merge remain held. Will rereview the exact corrected head on request.
bradhilton
left a comment
There was a problem hiding this comment.
Minsky rereview — CLEAR at 5de63beed7cb155938310a895b1da33c37ad1b7d for the stated scope. Supersedes my CHANGES REQUIRED at 3afdc48d; McCarthy's duplicate-closer P2 is resolved. Runtime bytes are identical to frozen overlay 7bcd560c (_tokenize.py sha256 9922a80b…, verified against the worktree the differentials load).
Correction: in the terminal synthetic_stop branch, when tool content-part bounds exist, the code now walks back from bounds[0] to the assistant span start and requires _prove_exact_length_stopped_assistant_prefix(locations(output, start), assistant_mask, expected_start=start) to find exactly one occurrence of the complete captured output at that start, entirely assistant-owned and preceded by a non-assistant token. The returned bounds end at the sampled endpoint, so only the genuine remainder of the renderer tail is appended; any ambiguity or partial projection returns None, boundary stays None, and the exact shortcut is declined (no suffix stripping, existing next-prompt/projection/unique-STOP guards untouched).
Verification at this exact head:
- My sampled-closer probe on the PR's tool-prefix history (
finish_reason="tool_calls", no sampled EOS): unsampled closer 41 tokens / 8 unsampled OUTPUT tail; fully sampled</tool>41 tokens / 1 unsampled OUTPUT (EOS only); partially sampled</to41 tokens / 4 unsampled OUTPUT (ol>+ EOS). At3afdc48dthese were 41/48/45 with 8 unsampled tail tokens each. Sampled counts and logprobs unchanged (20/27/24). - McCarthy's
test_no_duplicate_closer.py: 1 failed / 1 passed at3afdc48d→ 2 passed here. tests/unit/trajectories: 474 passed (new parametrizations cover empty/partial/full/double closers and no/multiple/wrong-start matches). Ruff check and format clean.- Frozen-history differentials (
value_differential.py,loo_differential.py, offline, hashed captures), run against this runtime: all 75 value and 86 LOO histories have identical tokens/flags/logprobs to baseline (changed: []), sampled IDs/logprobs/STOP positions and first-occurrence masks equal.
Scope, stated plainly for the decision: the two real #881 captures, value 4/3 and LOO 27/11, are not recovered at this head. Both remain non-EXACT (17,737 and 17,723 tokens, byte-identical to baseline) because their 2,048 sampled tokens extend beyond the parsed tool-call projection, so no renderer proof exists and the exact path correctly declines. The scripts' all EXACT assertions on those two histories fail as shipped (Schulman's own logs show the same), and the "pass" receipts above required relaxing exactly those two assertions. The PR body and report disclose this and defer recovery to a separate opt-in raw G/V policy. So this PR is: duplication bug fixed, fail-closed proof added, no behavioural change on any frozen field history, motivating cases still refused.
Draft, merge held. Baseline for my differential runs was the local ART checkout bcfe1351; candidate hash matches the PR head.
bradhilton
left a comment
There was a problem hiding this comment.
McCarthy: CLEAR at 5de63beed7cb155938310a895b1da33c37ad1b7d — sampled-closer P2 resolved; prior hold lifted.
Re-reviewed the delta from 3afdc48d7f9201fda897a866acd80b0da575b8c8. Before deriving a terminal synthetic tail, the code now requires one complete captured-output match at the renderer assistant span's actual start, wholly assistant-owned. The remaining tail therefore begins after the full sampled output, including any sampled closing markup. Missing, repeated, or wrong-start matches decline the exact shortcut; the existing projection/context, unique terminal stop and earlier next-prompt proofs remain in force.
Independent checks: 245 tokenizer tests plus my unchanged two-case duplicate-closer witness pass (247 total). Running the new boundary controls against the previous head gives four expected failures, for partial/full/multiple sampled closing markup, with nine other controls passing. Ruff/format/diff checks clean. Runtime and test bytes match frozen overlay 7bcd560cc117e593bbc8fad17a856ffc8dcbf667 exactly (9922a80b… / 04a6582d…), so this source clearance covers that overlay too.
The updated PR correctly narrows its claim: the actual lossy value/LOO terminal projections remain refused by this default path. This clearance does not establish that those research captures are unblocked. The opt-in raw G/V adapter is a separate review.
Evidence: /home/brad/.local/share/mccarthy/art882-fixed-review-20260910/. CPU/source checks only; no live edits, inference/GPU, merge or deployment.
Captured histories can contain an earlier length-stopped response followed by a normal or tool-call response. Extend the exact-prefix path to compose the proved earlier boundary with a separately proved terminal renderer tail, and locate the next generation at the assistant-span start rather than its first marked content part.
The terminal proof requires the complete captured output at one unambiguous renderer assistant-span start. This preserves partially or fully sampled closing markup without duplicating it. A mismatching earlier boundary or an incomplete/lossy terminal projection declines the shortcut and retains the existing renderer behavior.
The real capped value/LOO captures in #881 have authoritative raw IDs/logprobs that extend beyond their parsed tool-call projection. They remain intentionally refused by this default exact renderer shortcut; this PR does not claim to unblock those captures. An explicit experiment-local raw G/V capture-only policy is being reviewed separately, with canonical discriminator rendering unchanged.
Validation:245 tokenizer tests, unchanged reviewer duplicate-closer witness2/2, Ruff, and current ART ty pass. Controls cover empty/partial/full/multiple sampled closers, missing/repeated/wrong-start output matches, earlier-boundary refusal, and sampled ID/logprob/STOP preservation. Frozen-runtime overlay7bcd560cc and PRhead5de63beed have byte-identical runtime/tests. No GPU or inference run; draft, merge held.
Related #881, #868, #871.