Skip to content

Commit 94f3e63

Browse files
fix: /answer command sends swapped question/answer strings to the LLM (#2495)
* fix: /answer command sends swapped question/answer strings to the LLM * test(pr_reviewer): make /answer swap regression test behavioral Replace the inspect.getsource source-text assertion (brittle to formatting, and its runtime half only re-tested _get_user_answers, not the __init__ unpack where the bug was) with a behavioral test: drive the real PRReviewer.__init__ with external collaborators stubbed and assert the user's question and answer land in self.vars under the correct keys. Verified it fails on the swapped unpack and passes with the fix. Also adds the missing trailing newline. --------- Co-authored-by: naorpeled <me@naor.dev>
1 parent f226fee commit 94f3e63

2 files changed

Lines changed: 38 additions & 1 deletion

File tree

pr_agent/tools/pr_reviewer.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ def __init__(self, pr_url: str, is_answer: bool = False, is_auto: bool = False,
6464
self.ai_handler.main_pr_language = self.main_language
6565
self.patches_diff = None
6666
self.prediction = None
67-
answer_str, question_str = self._get_user_answers()
67+
question_str, answer_str = self._get_user_answers()
6868
self.pr_description, self.pr_description_files = (
6969
self.git_provider.get_pr_description(split_changes_walkthrough=True))
7070
if (self.pr_description_files and get_settings().get("config.is_auto_command", False) and

tests/unittest/test_pr_reviewer_core.py

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,3 +90,40 @@ def test_get_user_answers_collects_question_and_answer_from_issue_comments():
9090

9191
assert question == "Questions to better understand the PR:\n- Why?"
9292
assert answer == "/answer Because it fixes production."
93+
94+
95+
def test_init_maps_user_question_and_answer_to_correct_prompt_vars(monkeypatch):
96+
"""Behavioral regression for the swapped-unpacking bug (#2496).
97+
98+
The bug lived in ``PRReviewer.__init__``: ``_get_user_answers()`` returns
99+
``(question, answer)`` but the tuple was unpacked as ``answer, question``,
100+
so the review prompt rendered the user's answer under ``{{ question_str }}``
101+
and the question under ``{{ answer_str }}``. This drives the real ``__init__``
102+
(external collaborators stubbed) and asserts each value lands in ``self.vars``
103+
under the correct key — so it fails if the unpack is ever swapped again,
104+
regardless of how the line is formatted.
105+
"""
106+
from pr_agent.tools import pr_reviewer as pr_reviewer_module
107+
108+
provider = MagicMock()
109+
provider.is_supported.return_value = True
110+
provider.get_languages.return_value = {}
111+
provider.get_files.return_value = []
112+
provider.get_issue_comments.return_value = SimpleNamespace(reversed=[
113+
SimpleNamespace(body="Questions to better understand the PR:\n- Why?"),
114+
SimpleNamespace(body="/answer Because it fixes production."),
115+
])
116+
provider.get_pr_description.return_value = ("desc", [])
117+
118+
monkeypatch.setattr(pr_reviewer_module, "get_git_provider_with_context", lambda pr_url: provider)
119+
monkeypatch.setattr(pr_reviewer_module, "get_main_pr_language", lambda languages, files: "Python")
120+
monkeypatch.setattr(pr_reviewer_module, "TokenHandler", MagicMock())
121+
122+
reviewer = PRReviewer(
123+
"https://example/pr/1",
124+
is_answer=True,
125+
ai_handler=lambda: SimpleNamespace(main_pr_language=None),
126+
)
127+
128+
assert reviewer.vars["question_str"] == "Questions to better understand the PR:\n- Why?"
129+
assert reviewer.vars["answer_str"] == "/answer Because it fixes production."

0 commit comments

Comments
 (0)