Skip to content

Commit ca628f6

Browse files
committed
Treat a reply cut off by the token limit as no answer
ask_about_branch warned about stop_reason max_tokens and then parsed the reply anyway. The answer format asks for the verdict lines first with nothing before them, so a truncated reply still carries a parseable verdict, and read_answer reads it as a real one: - **Likely affected**: No - **Confidence**: high - **Reasoning**: The vulnerable memcpy in <- cut off here parses as (False, high), and decide_unsure turns that into NOT_AFFECTED. So a branch could be cleared on an answer the model never finished, against this module s own rule that no answer means AFFECTED. Thinking tokens share the MAX_ANSWER_TOKENS budget, so the bigger the fix, the more likely it is It now returns None, which decide_unsure already treats as no answer and flags the branch for review. Three tests: the same text parses as a confident no when the reply is complete, so the guard is what changes the outcome; the truncated one is no answer; and the branch ends up AFFECTED Reported by the security review on this pull request
1 parent 816690f commit ca628f6

2 files changed

Lines changed: 78 additions & 4 deletions

File tree

util/backport/src/engine/consult_ai.py

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -333,12 +333,16 @@ def ask_about_branch(
333333
print(f"[ai] call failed for {branch}: {exc}", file=sys.stderr)
334334
return None
335335
if reply.stop_reason == "max_tokens":
336-
# Thinking tokens ate the budget, so the answer may be cut off mid-sentence
336+
# Thinking tokens ate the budget, so the answer is cut off. The verdict lines come
337+
# first by design, so a truncated reply usually still carries a parseable "No" -
338+
# and parsing it would clear a branch on an answer the model never finished.
339+
# Treated as no answer instead, which leaves the branch flagged
337340
print(
338-
f"[ai] reply for {branch} hit the token limit and may be cut short, "
339-
"raise MAX_ANSWER_TOKENS in src/util/config.py",
341+
f"[ai] reply for {branch} hit the token limit and was cut short, "
342+
"leaving the branch flagged. Raise MAX_ANSWER_TOKENS in src/util/config.py",
340343
file=sys.stderr,
341344
)
345+
return None
342346
raw = "".join(b.text for b in reply.content if hasattr(b, "text"))
343347
return read_answer(raw.strip())
344348

util/backport/testing/test_engine.py

Lines changed: 71 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,13 @@
1010
"""
1111

1212
import datetime
13+
import io
1314
import subprocess
1415
import sys
1516
import unittest
17+
from contextlib import redirect_stderr
1618
from pathlib import Path
17-
from typing import Any, Dict, List, Optional, Sequence
19+
from typing import Any, ClassVar, Dict, List, Optional, Sequence
1820
from unittest import mock
1921

2022
sys.path.insert(0, str(Path(__file__).resolve().parent.parent / "src"))
@@ -318,6 +320,74 @@ def test_confidence_matches_whole_words(self):
318320
self.assertEqual(consult_ai.read_answer("**Confidence**: high")[1], "high")
319321

320322

323+
class ATruncatedReplyIsNoAnswer(unittest.TestCase):
324+
# The answer format puts the verdict first, so a reply cut off by the token limit
325+
# still carries a parseable "No". Reading it would clear a branch on an answer the
326+
# model never finished, which is the one failure that ships a vulnerability
327+
328+
# A real truncated reply: verdict present, reasoning stops mid-sentence
329+
CUT_OFF = (
330+
"- **Likely affected**: No\n"
331+
"- **Confidence**: high\n"
332+
"- **Reasoning**: The vulnerable memcpy in"
333+
)
334+
335+
def ask(self, stop_reason):
336+
"""ask_about_branch against a stubbed Bedrock reply, as its return value"""
337+
338+
class Block:
339+
text = self.CUT_OFF
340+
341+
class Reply:
342+
content: ClassVar[list] = [Block()]
343+
344+
Reply.stop_reason = stop_reason
345+
346+
class Stream:
347+
def __enter__(inner):
348+
return inner
349+
350+
def __exit__(inner, *exc):
351+
return False
352+
353+
def get_final_message(inner):
354+
return Reply()
355+
356+
class Messages:
357+
def stream(inner, **kwargs):
358+
return Stream()
359+
360+
class Client:
361+
messages = Messages()
362+
363+
with mock.patch.multiple(
364+
consult_ai,
365+
ai_client=lambda: Client(),
366+
build_prompt=lambda *a, **k: "prompt",
367+
load_model_config=lambda: {"opus": "a-model"},
368+
branch_ref=lambda branch: f"upstream/{branch}",
369+
), redirect_stderr(io.StringIO()):
370+
return consult_ai.ask_about_branch(FIX_SHA, BRANCH, SRC_FILES, BUG_COMMITS)
371+
372+
def test_the_same_text_parses_as_a_confident_no_when_complete(self):
373+
# Establishes that the guard is what changes the outcome, not the text
374+
self.assertEqual(self.ask("end_turn"), (False, "high"))
375+
376+
def test_hitting_the_token_limit_is_no_answer(self):
377+
self.assertIsNone(self.ask("max_tokens"))
378+
379+
def test_so_the_branch_stays_flagged(self):
380+
# No answer is what decide_unsure turns into AFFECTED, so check the whole path
381+
verdicts = {BRANCH: config.UNSURE}
382+
decided_by = {}
383+
with mock.patch.object(consult_ai, "ask_about_branch", lambda *a, **k: None):
384+
consult_ai.decide_unsure(
385+
FIX_SHA, SRC_FILES, BUG_COMMITS, verdicts, decided_by
386+
)
387+
self.assertEqual(verdicts[BRANCH], config.AFFECTED)
388+
self.assertIn("flagged for review", decided_by[BRANCH])
389+
390+
321391
# --- Test Doubles For The Verdict Layer ---
322392

323393
# Stand-in names for one fix and one release branch. classify_branches only ever

0 commit comments

Comments
 (0)