Repository navigation
fix: make enclosing TextRange end exclusive - #351
Conversation
text_range_from_message_range(start, end) receives the last message ordinal (inclusive) but copied it into TextRange.end, which is exclusive. get_enclosing_date_range_for_text_range correctly treats that end as exclusive, so the time range attached to an answer's entities and topics ended at the last source message's own timestamp and excluded it. Set TextRange.end one past the last message. Co-authored-by: wtfyd-agent[bot] <328989958+wtfyd-agent[bot]@users.noreply.github.com>
Bernhard Merkle (bmerkle)
left a comment
There was a problem hiding this comment.
Thanks — you're right that text_range_from_message_range stores an inclusive ordinal in the exclusive TextRange.end, so the range doesn't satisfy TextRange's documented contract. But I don't think moving end to last + 1 is the right fix, because of what this range is used for.
text_range_from_message_range / get_enclosing_text_range have a single consumer: get_enclosing_data_range_for_messages → RelevantKnowledge.time_range ("Time period during which this knowledge was gathered"), which is rendered into the answer prompt. With this change the end becomes the timestamp of the next, unrelated message, or None when the last source is the conversation's final message. With emails dated Jan 1, Jan 3, Mar 10, Mar 11:
| source messages | main | this PR |
|---|---|---|
| Jan 1, Jan 3 | Jan 1 .. Jan 3 | Jan 1 .. Mar 10 |
| Mar 10, Mar 11 | Mar 10 .. Mar 11 | Mar 10 .. None (open-ended) |
For the LLM that's a less accurate answer context: it claims the knowledge was gathered over a span that can include weeks with no relevant messages, and an open-ended end reads as "ongoing". main matches the TypeScript reference (message.ts), and convutils.get_time_range_for_conversation deliberately does the same for display, with a docstring explaining that its end is "inclusive-looking" and must not be passed to timestamp-index lookups.
Suggestion: keep the reported time span as first..last source timestamp, and fix the contract violation where it lives instead. For example, have get_enclosing_data_range_for_messages build the DateRange directly from the first and last source messages' timestamps, with a docstring like get_time_range_for_conversation's noting it's a display range with an inclusive end. Then get_enclosing_text_range can either keep its current behaviour with a docstring saying end is the last ordinal (inclusive), or return a correct exclusive TextRange that nothing converts to a date via "next message's timestamp". Either way, the tests that check every source message's timestamp is in dr would need to compare with <= on the end, since DateRange.__contains__ is half-open.
Happy to be told the half-open semantics matter for a consumer I've missed, but I couldn't find any other caller in src/ or tools/.
Keep TextRange.end exclusive, but build the prompt's time range directly from the first and last source messages' timestamps (an inclusive display bound, like get_time_range_for_conversation) instead of the timestamp of the message after the last source.
|
Thanks, agreed: moving the reported end to the next message made the prompt less accurate. Reworked along the lines you suggested (e7bbd78):
Renamed the PR to match and updated the description. Merged the latest |
Bernhard Merkle (bmerkle)
left a comment
There was a problem hiding this comment.
Thanks for the rework — this addresses my concern. TextRange.end is now correctly exclusive, and get_enclosing_data_range_for_messages reports first..last source timestamp directly, with a docstring warning not to use it for timestamp-index lookups. I checked the cases against main's get_enclosing_date_range_for_text_range (multiple sources, single source, last source = final message, missing start timestamp): the prompt's time_range is unchanged in all of them.
Verified locally at e7bbd785 merged onto current main (1ae9435): merges clean, full make green (894 passed, pyright clean on 3.12 and 3.15). Reverting only answers.py fails the 2 updated TextRange assertions and the 3 new containment cases.
Non-blocking nits:
get_enclosing_date_range_for_text_rangeno longer has a caller insrc/ortools/(only tests); fine to keep for TS parity, or drop it.get_enclosing_data_range_for_messagescould takemin/maxof the ordinals directly instead of building aTextRangeand subtracting 1 again.
LGTM.
Summary
text_range_from_message_range(start, end)receives the last message ordinal (inclusive) but copied it intoTextRange.end, which is documented as exclusive. SetTextRange.endone past the last message. Single-message (point) ranges are unchanged.get_enclosing_data_range_for_messages(the only consumer, feedingRelevantKnowledge.time_rangein the answer prompt) now builds itsDateRangedirectly from the first and last source messages' timestamps, rather than converting theTextRangevia the next message's timestamp. The reported span stays first..last source message, as onmainand in the TypeScript reference; its docstring notes that the end is an inclusive display bound, likeget_time_range_for_conversation, and must not be passed to timestamp-index lookups.get_enclosing_text_range([3, 1, 7]).endis now 8, andtext_range_from_message_range(2, 5).endis now 6. Also corrected a stale test comment that said the date-range function subtracts 1; it doesn't.Validation
get_enclosing_text_range(fails onmain), andget_enclosing_data_range_for_messagesspans exactly the first through the last source message's timestamp, including when the last source is the conversation's final message.