Skip to content

Commit 708d39b

Browse files
committed
test(email): cover render_needs_you_list, rewrite_triage_answer, finalize_answer
Addresses review: the answer-rewriting logic that now decides what every triage reply says had no coverage. Adds section ordering, ref numbering, per-item classifier reasons, sender-address handling, the lead-paragraph extraction, and the finalize_answer/process_query dedup that stops an append-style guard firing twice on one turn. Also fixes the pylint failure (unused finalize_answer default-hook argument), collapses a doubled comment on _ITEM_LINE_EMAIL_RE, corrects two comments that still claimed NEEDS_YOU_CAP is 5, and documents why rewrite_triage_answer keys on tool presence rather than parsing intent.
1 parent 67eaa26 commit 708d39b

5 files changed

Lines changed: 212 additions & 12 deletions

File tree

hub/agents/email/python/gaia_agent_email/answer_grounding.py

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -311,12 +311,10 @@ def strip_scaffolding_leaks(text: str) -> str:
311311
# "...scheduling meetings: 4. Tomasz ... 5. Tomasz ...".
312312
_INLINE_NUMBERED_ITEM_RE = re.compile(r"(?<=\S)[ \t]+(?=\d{1,3}\.[ \t]+\S)")
313313

314-
# A bare address on an item line. The sender is already named beside it, so
315-
# this renders as the address twice -- once as text, once as a mailto: link
316-
# the markdown renderer expands.
317314
# Any bare address on an item line, however the model punctuated around it.
318-
# An explicit mailto: link goes too — the markdown renderer expands a bare
319-
# address into one anyway, which is the duplication being removed.
315+
# The sender is already named beside it, so a bare address renders twice --
316+
# once as text, once as the mailto: link the markdown renderer expands. An
317+
# explicit mailto: link goes too, for the same reason.
320318
_ITEM_LINE_EMAIL_RE = re.compile(
321319
r"[ \t]*\[?<?(?:mailto:)?[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}>?\]?"
322320
r"(?:\((?:mailto:)?[^)]*\))?"
@@ -450,6 +448,15 @@ def rewrite_triage_answer(
450448
and asking the chat model to do it produced invented numbering, dropped
451449
items, merged sections, and once no list at all. So the chat model keeps
452450
the opening sentence and this renders the rest.
451+
452+
Deliberately keyed on tool PRESENCE, not on parsing the user's question:
453+
any turn that calls ``pre_scan_inbox`` gets the authoritative list, even
454+
for a narrower ask ("how many urgent emails do I have?"). A hand-
455+
summarized partial view is exactly the failure mode this function
456+
replaces, and ``pre_scan_inbox`` only ever runs when the model judged
457+
the question worth a scan in the first place — so a rewrite here is
458+
never wrong, only sometimes more complete than the question strictly
459+
asked for.
453460
"""
454461
prescan = last_tool_payload(conversation, "pre_scan_inbox")
455462
if not prescan:

hub/agents/email/python/tests/test_answer_grounding.py

Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
from __future__ import annotations
3030

3131
import json
32+
import re
3233
import sys
3334
import time
3435
from pathlib import Path
@@ -58,6 +59,9 @@
5859
find_unlicensed_cross_mailbox_claim,
5960
find_unqualified_negative_claim,
6061
ground_final_answer,
62+
normalize_triage_list,
63+
render_needs_you_list,
64+
rewrite_triage_answer,
6165
strip_scaffolding_leaks,
6266
tools_called_this_turn,
6367
)
@@ -435,6 +439,150 @@ def test_leaves_clean_text_unchanged_apart_from_whitespace_trim(self):
435439
assert strip_scaffolding_leaks(clean) == clean
436440

437441

442+
# ---------------------------------------------------------------------------
443+
# render_needs_you_list / rewrite_triage_answer — the triage list is rendered
444+
# from the scan's own needs_you view, never retyped by the model (#2789
445+
# follow-up: three consecutive live runs invented numbering, dropped items,
446+
# or merged sections when the model was asked to write the list itself).
447+
# ---------------------------------------------------------------------------
448+
449+
450+
def _needs_you_row(ref: int, kind: str, **overrides) -> dict:
451+
base = {
452+
"ref": ref,
453+
"kind": kind,
454+
"message_id": f"m{ref}",
455+
"thread_id": f"t{ref}",
456+
"sender": "Someone <someone@example.com>",
457+
"subject": f"subject {ref}",
458+
"age_seconds": 3600,
459+
"why": "",
460+
"detail": [],
461+
"due_hint": None,
462+
"mailbox": None,
463+
}
464+
base.update(overrides)
465+
return base
466+
467+
468+
class TestRenderNeedsYouList:
469+
def test_empty_needs_you_renders_nothing(self):
470+
assert render_needs_you_list(_prescan_envelope(needs_you=[])) == ""
471+
assert render_needs_you_list({}) == ""
472+
473+
def test_sections_appear_in_display_order_with_ascending_refs(self):
474+
envelope = _prescan_envelope(
475+
needs_you=[
476+
_needs_you_row(1, "waiting_on_you"),
477+
_needs_you_row(2, "needs_response"),
478+
_needs_you_row(3, "meeting_request"),
479+
_needs_you_row(4, "needs_review"),
480+
]
481+
)
482+
rendered = render_needs_you_list(envelope)
483+
# Every section header appears, in _TRIAGE_SECTIONS order.
484+
for heading in (
485+
"Waiting on your reply",
486+
"Needs a response",
487+
"Meetings to decide",
488+
"Needs a manual look",
489+
):
490+
assert heading in rendered
491+
assert rendered.index("Waiting on your reply") < rendered.index("Needs a response")
492+
assert rendered.index("Needs a response") < rendered.index("Meetings to decide")
493+
assert rendered.index("Meetings to decide") < rendered.index("Needs a manual look")
494+
# Refs ascend in the order they appear on the page, unbroken.
495+
seen_refs = [int(tok) for tok in re.findall(r"^(\d+)\.", rendered, re.MULTILINE)]
496+
assert seen_refs == [1, 2, 3, 4]
497+
498+
def test_urgent_and_waiting_on_you_share_one_reply_section(self):
499+
# Both kinds render as REPLY (verbForKind, tui/cards/emailprescan.go) —
500+
# a separate heading per kind would split one verb into two sections.
501+
envelope = _prescan_envelope(
502+
needs_you=[
503+
_needs_you_row(1, "urgent"),
504+
_needs_you_row(2, "waiting_on_you"),
505+
]
506+
)
507+
rendered = render_needs_you_list(envelope)
508+
assert rendered.count("Waiting on your reply") == 1
509+
510+
def test_each_item_carries_its_classifier_reason(self):
511+
envelope = _prescan_envelope(
512+
needs_you=[
513+
_needs_you_row(1, "needs_response", why="flagged as phishing"),
514+
]
515+
)
516+
assert "flagged as phishing" in render_needs_you_list(envelope)
517+
518+
def test_sender_address_is_dropped_when_a_display_name_exists(self):
519+
envelope = _prescan_envelope(
520+
needs_you=[
521+
_needs_you_row(1, "waiting_on_you", sender="Jane Doe <jane@example.com>")
522+
]
523+
)
524+
rendered = render_needs_you_list(envelope)
525+
assert "Jane Doe" in rendered
526+
assert "jane@example.com" not in rendered
527+
528+
def test_address_only_sender_is_kept_but_not_autolinkable(self):
529+
envelope = _prescan_envelope(
530+
needs_you=[_needs_you_row(1, "waiting_on_you", sender="jane@example.com")]
531+
)
532+
rendered = render_needs_you_list(envelope)
533+
assert "`jane@example.com`" in rendered
534+
535+
536+
class TestRewriteTriageAnswer:
537+
def test_no_pre_scan_tool_call_leaves_the_answer_untouched(self):
538+
text = "You have no new mail today."
539+
assert rewrite_triage_answer(text, conversation=[]) == text
540+
541+
def test_pre_scan_with_no_needs_you_items_leaves_the_answer_untouched(self):
542+
conversation = [_tool_entry("pre_scan_inbox", _prescan_envelope(needs_you=[]))]
543+
text = "Your inbox is clear."
544+
assert rewrite_triage_answer(text, conversation) == text
545+
546+
def test_the_models_own_list_is_discarded_in_favor_of_the_rendered_one(self):
547+
envelope = _prescan_envelope(
548+
needs_you=[_needs_you_row(1, "waiting_on_you", subject="Re: Q3 roadmap")]
549+
)
550+
conversation = [_tool_entry("pre_scan_inbox", envelope)]
551+
model_answer = (
552+
"Here's your inbox — 1 item needs attention.\n\n"
553+
"### Stuff\n- **9.** a row the model invented\n- **3.** a duplicate"
554+
)
555+
out = rewrite_triage_answer(model_answer, conversation)
556+
assert "the model invented" not in out
557+
assert "1." in out and "Re: Q3 roadmap" in out
558+
# The opening sentence survives; only the list is replaced.
559+
assert out.startswith("Here's your inbox — 1 item needs attention.")
560+
561+
562+
class TestNormalizeTriageList:
563+
def test_ordinary_prose_is_untouched(self):
564+
prose = "We looked at 5. Then we stopped."
565+
assert normalize_triage_list(prose) == prose
566+
567+
def test_run_on_items_are_split_onto_their_own_line(self):
568+
# normalize_triage_list only activates once the text already contains
569+
# a line-start numbered item (the signal that this IS a triage list);
570+
# item 3 provides that, item 4/5 are the run-on fragment being fixed.
571+
text = (
572+
"3. Carol: Q3 roadmap\n"
573+
"These emails propose scheduling: 4. Alice: 30 minutes? 5. Bob: Design review"
574+
)
575+
out = normalize_triage_list(text)
576+
assert "\n4." in out
577+
assert "\n5." in out
578+
579+
def test_duplicated_address_is_stripped_from_a_numbered_line(self):
580+
text = "1. Tomasz Testingiewicz tomasz.t@outlook.com - Re: Partnership intro"
581+
out = normalize_triage_list(text)
582+
assert "tomasz.t@outlook.com" not in out
583+
assert "Tomasz Testingiewicz" in out
584+
585+
438586
class TestDecodeStrayUnicodeEscapes:
439587
def test_decodes_known_escapes(self):
440588
assert decode_stray_unicode_escapes("a\\u2013b\\u2019c") == "a\u2013b\u2019c"
@@ -1080,6 +1228,50 @@ def test_grounded_answer_passes_through_unchanged(self, tmp_path):
10801228
finally:
10811229
agent.close_db()
10821230

1231+
def test_finalize_answer_grounds_the_text_the_loop_will_emit(self, tmp_path):
1232+
# #2789: grounding used to run only on process_query's return value,
1233+
# which the SSE/TUI stream never re-reads. finalize_answer is the
1234+
# hook the agent LOOP calls before it emits anything, so this must
1235+
# ground on its own, with no process_query involved at all.
1236+
agent = _build_agent(tmp_path)
1237+
try:
1238+
conversation = [{"role": "user", "content": "archive it"}]
1239+
out = agent.finalize_answer("The message has been archived.", conversation)
1240+
assert out == UNGROUNDED_SUCCESS_FALLBACK
1241+
assert agent._grounded_answer == UNGROUNDED_SUCCESS_FALLBACK
1242+
finally:
1243+
agent.close_db()
1244+
1245+
def test_process_query_does_not_re_ground_what_finalize_answer_already_did(
1246+
self, tmp_path
1247+
):
1248+
# An append-style guard (attention-card) makes a double-fire visible: if
1249+
# process_query's fallback re-ran ground_final_answer on text
1250+
# finalize_answer already corrected, the correction would appear twice.
1251+
_store_attention_view(items=[_attention_item("action_item")], scanned=7)
1252+
agent = _build_agent(tmp_path)
1253+
try:
1254+
conversation = [{"role": "user", "content": "anything need my attention?"}]
1255+
grounded_by_loop = agent.finalize_answer(
1256+
"No urgent or actionable items found.", conversation
1257+
)
1258+
assert grounded_by_loop.count("attention card") == 1
1259+
1260+
canned = {
1261+
"status": "success",
1262+
"result": grounded_by_loop,
1263+
"conversation": conversation,
1264+
"steps_taken": 1,
1265+
}
1266+
with patch(
1267+
"gaia.agents.base.agent.Agent.process_query", return_value=canned
1268+
):
1269+
out = agent.process_query("anything need my attention?")
1270+
assert out["result"] == grounded_by_loop
1271+
assert out["result"].count("attention card") == 1
1272+
finally:
1273+
agent.close_db()
1274+
10831275
def test_attention_card_contradiction_is_appended_by_the_real_override(
10841276
self, tmp_path
10851277
):

src/gaia/agents/base/agent.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3325,7 +3325,7 @@ def _console_cancelled(self) -> bool:
33253325
cancelled = getattr(self.console, "cancelled", None)
33263326
return cancelled is not None and cancelled.is_set()
33273327

3328-
def finalize_answer(self, answer: str, conversation: Any) -> str:
3328+
def finalize_answer(self, answer: str, _conversation: Any) -> str:
33293329
"""Last chance to correct the final answer, BEFORE it is emitted.
33303330
33313331
Runs ahead of ``console.print_final_answer``, so a subclass's

tui/internal/ui/cards/emailprescan_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -107,9 +107,9 @@ func TestPreScanCapsHitShowsNofM(t *testing.T) {
107107
t.Logf("\n%s", plain(out))
108108

109109
assertWidth(t, out, width80)
110-
// needs_you is capped at 5 server-side while needs_you_total (40)
111-
// reports the true pre-cap count -- the header must read "5 of 40"
112-
// rather than a bare count that implies the list is everything.
110+
// The fixture ships fewer needs_you rows (5) than needs_you_total (40)
111+
// reports -- the header must read "5 of 40" rather than a bare count
112+
// that implies the list is everything.
113113
assertContains(t, out, "NEEDS A REPLY", "5 of 40")
114114
}
115115

tui/internal/ui/cards/testdata_test.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,9 +46,10 @@ const populatedPreScan = `{
4646
}
4747
}`
4848

49-
// capsHitPreScan: needs_you is at its server-side cap of 5 while
50-
// needs_you_total reports the real pre-cap count, so the header must read
51-
// "5 of 40".
49+
// capsHitPreScan: needs_you carries fewer rows (5) than needs_you_total (40)
50+
// reports as the real pre-cap count -- NEEDS_YOU_CAP is 10 server-side, so
51+
// this fixture demonstrates the header reading "N of M" honestly whenever
52+
// N < M, without needing to hit the cap exactly.
5253
const capsHitPreScan = `{
5354
"kind": "email_pre_scan",
5455
"urgent": [], "actionable": [], "informational_count": 4,

0 commit comments

Comments
 (0)