fix(engine): remove duplicated injection backtrack accumulation (#1490)#1491
fix(engine): remove duplicated injection backtrack accumulation (#1490)#1491Anai-Guo wants to merge 1 commit into
Conversation
The step-injection stop-string handler applied the same two non-idempotent statements twice in a row, doubling both backtrack and backtrack_bytes whenever a stop string spans a chunk boundary. The doubled byte string then failed the endswith() guard in EngineInterpreter.grammar(), so the stop-string fragment was never removed and leaked into the output. Fixes guidance-ai#1490.
ErenAta16
left a comment
There was a problem hiding this comment.
This is the exact duplication I reported in #1490, and the removal is correct.
Traced the arithmetic to confirm the impact of the duplicate. With both pairs present, the two lines run twice on the same variables:
backtracked_bytes = backtrack_bytes_from_previous + backtracked_bytes
backtrack = previous_response_token_count + backtrack
backtracked_bytes = backtrack_bytes_from_previous + backtracked_bytes # again
backtrack = previous_response_token_count + backtrack # again
so backtracked_bytes ends up 2 * backtrack_bytes_from_previous + backtracked_bytes and backtrack ends up 2 * previous_response_token_count + backtrack. The injection backtrack is counted twice, which is what leaks the extra stop-string fragment bytes / over-backtracks past what was actually in the previous responses. Removing the second pair leaves each addition applied once, which is the intended single accumulation described by the comment right above it.
Confirmed current main still has all four lines, so the fix is against the live bug and doesn't touch anything else.
One thing worth considering (not blocking): there's no regression test here. I understand it's awkward to pin this down without driving a full step-injection + stop-string scenario through a real engine, so I wouldn't hold the fix on it. If a lightweight reproduction is feasible with models.Mock and an injection callback, a test asserting the emitted text doesn't carry the doubled backtrack would guard against the duplicate creeping back in on a future refactor. Either way the removal itself is correct.
Fixes #1490.
The bug
Engine.__call__'s step-injection stop-string handler runs the same two statements twice:Neither statement is idempotent — each re-adds its left operand on top of the previous result — so executing the pair twice doubles both values whenever
previous_response_token_count > 0, i.e. whenever a configured stop string spans a chunk boundary rather than sitting entirely inside the current response chunk.Why it matters
The doubled
backtracked_bytesbreaks the removal step inEngineInterpreter.grammar():The prompt only ever contained one copy of the fragment, so
endswith(backtrack_text)is false against the doubled string. The guard silently skips the removal, and the stop-string fragment leaks verbatim into the output instead of being replaced by the injected text. TheBacktrack(n_tokens=...)trace event is emitted with the doubled count as well.Note the failure is silent in both places: the
endswithguard has noelse, so nothing surfaces the mismatch.The fix
Delete the duplicated pair. When
previous_response_token_count == 0(stop string fully inside the current chunk)backtrack_bytes_from_previousisb""and the duplication was a no-op, which is why this survived: it only misbehaves on the cross-boundary path.Provenance
git logpoints at ddf9eed ([Feature] Monitor-guided Inference #1391), which added this block already duplicated:So this isn't a regression — the cross-boundary stop-string path has never worked since the feature landed. That also matches the absence of tests: I couldn't find any coverage referencing
step_stop_tokens/with_step_config/injection_backtrackoutside the engine itself, which is presumably how the duplicate went unnoticed. Happy to add a regression test for the cross-boundary case if you'd like — it needs a scriptednext_tokento force the stop string across an iteration boundary, so I left it out of this minimal fix rather than guess at the shape you'd want.Verification
Since the block is only reachable through step injection, I confirmed the normal generation path is untouched by running
models.Mockgeneration against the patched module (guidance0.3.1 ships the identical duplicate). The change removes two statements and adds none.🤖 Generated with Claude Code