diff --git a/pkg-py/src/commons/_citations.py b/pkg-py/src/commons/_citations.py index d3d60846..c13dccbf 100644 --- a/pkg-py/src/commons/_citations.py +++ b/pkg-py/src/commons/_citations.py @@ -1,8 +1,9 @@ -"""Parsing a citation block and verifying its quote against a trusted corpus. +"""Citations: verifying a quote against a trusted corpus, and asking for one. The normalization rules and the matching verdicts are a cross-language contract -pinned by ``tests/shared/citations.json``; change that fixture, not just this -file. ``pkg-r/R/citations.R`` implements the same contract for R. +pinned by ``tests/shared/citations.json``, and where the citation request lands +by ``tests/shared/citation-request.json``; change those fixtures, not just this +file. ``pkg-r/R/citations.R`` implements the same contracts for R. """ from __future__ import annotations @@ -10,17 +11,27 @@ import html import re from collections.abc import Sequence -from dataclasses import dataclass +from dataclasses import dataclass, field from typing import Any, Literal, get_args +from chatlas import ContentToolResult, Turn +from chatlas.types import ContentText + +from ._prompt import read_prompt +from ._provenance import Tag + __all__ = [ "CitationDecision", + "CitationRequest", "CorpusEntry", "ParsedCitation", "citation_aside_html", + "citation_reminder_text", "match_citation", "normalize_citation", "parse_commons_citation", + "tool_result", + "turn_has_user_message", ] # The minimum length is a guard, not a tuning knob: a fragment this short can @@ -201,3 +212,66 @@ def citation_aside_html(quote: str, explanation: str, label: str, kind: str) -> # Ampersands first, so the entities this generates are not escaped again. def _escape_attr(text: str) -> str: return text.replace("&", "&").replace('"', """) + + +def tool_result(value: Any, tag: Tag | None = None) -> ContentToolResult: + """A tool result carrying the provenance tag of the output it holds. + + The tag is read back off ``extra`` when the turn is classified, so it is + set here rather than at the point a result is added to the conversation. + """ + return ContentToolResult(value=value, extra={"commons_tag": tag}) + + +def citation_reminder_text() -> str: + """The reminder text, from the prompt file both packages ship.""" + return read_prompt("citation-request.md") + + +@dataclass +class CitationRequest: + """Whether this user turn has carried the citation reminder yet. + + The citation contract lives in the system prompt; this is the nudge that + rides on the first tool result of a turn whose output has to be cited. + """ + + reminder: str = field(default_factory=citation_reminder_text) + requested: bool = False + + def add_request(self, result: ContentToolResult) -> ContentToolResult: + """Add the reminder to ``result``, unless this turn has asked already. + + An errored result passes through without spending the request: the + model is sent the error rather than the value, so a reminder added to + the value would never arrive. + """ + if self.requested or result.error is not None: + return result + self.requested = True + result.value = _with_reminder(result.value, self.reminder) + return result + + def reset(self) -> None: + """Start a new user turn, so the next eligible result asks again.""" + self.requested = False + + +def _with_reminder(value: Any, reminder: str) -> Any: + if isinstance(value, str): + return f"{value}\n\n{reminder}" + part = ContentText(text=reminder) + if isinstance(value, list): + return [*value, part] + # A tool result can hold something that is neither: it becomes the first + # part rather than being reformatted to make room for the reminder. + return [value, part] + + +def turn_has_user_message(turn: Turn) -> bool: + """Whether a turn asks something new, rather than continuing the tool loop. + + A turn of nothing but tool results is the same question still running, so + the reminder stays spent until the person says something. + """ + return any(not isinstance(content, ContentToolResult) for content in turn.contents) diff --git a/pkg-py/tests/test_citation_request.py b/pkg-py/tests/test_citation_request.py new file mode 100644 index 00000000..951b92b5 --- /dev/null +++ b/pkg-py/tests/test_citation_request.py @@ -0,0 +1,89 @@ +"""The citation-request tracker and the tag a tool result carries.""" + +from typing import Any + +from chatlas import ContentToolResult, UserTurn +from chatlas.types import ContentText + +from commons._citations import CitationRequest, tool_result, turn_has_user_message +from commons._prompt import read_prompt +from commons._provenance import Tag + +from ._shared import load_shared_fixture + + +def _fixture_value(spec: dict[str, Any]) -> Any: + if spec["kind"] == "text": + return spec["text"] + return [ContentText(text=part) for part in spec["parts"]] + + +def _value_shape(value: Any) -> dict[str, Any]: + if isinstance(value, str): + return {"kind": "text", "text": value} + return {"kind": "parts", "parts": [part.text for part in value]} + + +def test_tool_result_carries_its_provenance_tag(): + result = tool_result("6 rows", tag=Tag.B) + + assert isinstance(result, ContentToolResult) + assert result.value == "6 rows" + assert result.extra == {"commons_tag": Tag.B} + + +def test_shared_citation_request_cases(): + section = load_shared_fixture("citation-request")["requests"] + assert section["cases"] + + for case in section["cases"]: + tracker = CitationRequest(reminder=section["reminder"]) + for index, step in enumerate(case["steps"]): + if step["action"] == "reset": + tracker.reset() + continue + result = tracker.add_request(tool_result(_fixture_value(step["value"]), tag=Tag.B)) + assert _value_shape(result.value) == step["expected"], ( + f"{case['name']} step {index}" + ) + + +def test_a_value_that_is_neither_text_nor_parts_becomes_parts(): + frame = {"rows": 6} + result = CitationRequest(reminder="REMINDER").add_request( + tool_result(frame, tag=Tag.B) + ) + + assert result.value[0] == frame + assert result.value[1].text == "REMINDER" + + +def test_an_errored_result_does_not_spend_the_request(): + tracker = CitationRequest(reminder="REMINDER") + failed = tool_result("6 rows", tag=Tag.B) + failed.error = ValueError("boom") + + assert tracker.add_request(failed).value == "6 rows" + + result = tracker.add_request(tool_result("3 rows", tag=Tag.B)) + assert result.value == "3 rows\n\nREMINDER" + + +def test_shared_reset_cases(): + section = load_shared_fixture("citation-request")["resets"] + assert section["cases"] + + for case in section["cases"]: + contents = [ + ContentText(text="a question") + if kind == "text" + else tool_result("6 rows", tag=Tag.B) + for kind in case["contents"] + ] + turn = UserTurn(contents) + + assert turn_has_user_message(turn) is case["resets"], case["name"] + + +def test_the_reminder_defaults_to_the_shipped_prompt_text(): + assert CitationRequest().reminder == read_prompt("citation-request.md") diff --git a/pkg-r/tests/testthat/fixtures/shared/citation-request.json b/pkg-r/tests/testthat/fixtures/shared/citation-request.json new file mode 100644 index 00000000..8e486ac4 --- /dev/null +++ b/pkg-r/tests/testthat/fixtures/shared/citation-request.json @@ -0,0 +1,118 @@ +{ + "description": "How the citation request reaches the model. The citation contract itself lives in the system prompt; a short reminder rides along on the first tool result of a user turn whose output has to be cited. The source is tests/shared/citation-request.json; the copy under pkg-r/tests/testthat/fixtures/shared/ is generated by scripts/sync-shared.sh. Edit the source and re-run that script.", + "requests": { + "description": "Each case drives one tracker through a sequence of steps. `add` passes a tool result through the tracker and states the value it must come back with; `reset` is what a new user message does to the tracker. The reminder is a stand-in for the real text, which lives in prompts/citation-request.md, so these cases pin placement rather than wording. A value is either the text a tool returned or a list of content parts, and the reminder joins each shape in its own way.", + "reminder": "SHORT CITATION REMINDER", + "cases": [ + { + "name": "one reminder per user turn", + "steps": [ + { + "action": "add", + "value": { + "kind": "text", + "text": "6 rows" + }, + "expected": { + "kind": "text", + "text": "6 rows\n\nSHORT CITATION REMINDER" + } + }, + { + "action": "add", + "value": { + "kind": "text", + "text": "3 rows" + }, + "expected": { + "kind": "text", + "text": "3 rows" + } + }, + { + "action": "reset" + }, + { + "action": "add", + "value": { + "kind": "text", + "text": "2 rows" + }, + "expected": { + "kind": "text", + "text": "2 rows\n\nSHORT CITATION REMINDER" + } + } + ] + }, + { + "name": "content parts take the reminder as another part", + "steps": [ + { + "action": "add", + "value": { + "kind": "parts", + "parts": [ + "output" + ] + }, + "expected": { + "kind": "parts", + "parts": [ + "output", + "SHORT CITATION REMINDER" + ] + } + }, + { + "action": "add", + "value": { + "kind": "parts", + "parts": [ + "more output" + ] + }, + "expected": { + "kind": "parts", + "parts": [ + "more output" + ] + } + } + ] + } + ] + }, + "resets": { + "description": "Which turns clear the flag, so the next eligible tool result asks for a citation again. A turn that carries a real user message starts a new question; a turn of nothing but tool results is the same question still running, and an empty turn is neither.", + "cases": [ + { + "name": "a user message", + "contents": [ + "text" + ], + "resets": true + }, + { + "name": "tool results only", + "contents": [ + "tool_result" + ], + "resets": false + }, + { + "name": "a user message after tool results", + "contents": [ + "tool_result", + "text" + ], + "resets": true + }, + { + "name": "no contents", + "contents": [], + "resets": false + } + ] + } +} diff --git a/pkg-r/tests/testthat/test-citations.R b/pkg-r/tests/testthat/test-citations.R index c5db4b67..58fcce7f 100644 --- a/pkg-r/tests/testthat/test-citations.R +++ b/pkg-r/tests/testthat/test-citations.R @@ -270,36 +270,71 @@ test_that("dataset-level dictionary prose is citable", { ) }) -test_that("add_citation_request appends one reminder per user turn", { - tracker <- new.env(parent = emptyenv()) - tracker$reminder <- "SHORT CITATION REMINDER" - first <- tool_result("6 rows", title = "Ran SQL", tag = "B") - second <- tool_result("3 rows", title = "Ran SQL", tag = "B") - third <- tool_result("2 rows", title = "Ran SQL", tag = "B") - - first <- add_citation_request(first, tracker) - second <- add_citation_request(second, tracker) - tracker$requested <- FALSE - third <- add_citation_request(third, tracker) - - expect_match(first@value, "6 rows") - expect_match(first@value, "SHORT CITATION REMINDER", fixed = TRUE) - expect_equal(second@value, "3 rows") - expect_match(third@value, "SHORT CITATION REMINDER", fixed = TRUE) -}) +# The fixture states a tool result's value as {kind, text} or {kind, parts}; +# each package builds and reads back its own content objects from that. +fixture_tool_value <- function(spec) { + if (identical(spec$kind, "text")) { + return(spec$text) + } + lapply(spec$parts, function(part) ellmer::ContentText(text = part)) +} -test_that("add_citation_request appends ContentText to content lists", { - tracker <- new.env(parent = emptyenv()) - result <- tool_result( - list(ellmer::ContentText(text = "output")), - title = "Ran R code", - tag = "B" - ) +fixture_value_shape <- function(value) { + if (is.character(value)) { + return(list(kind = "text", text = value)) + } + list(kind = "parts", parts = lapply(value, function(part) part@text)) +} + +test_that("both packages place the citation request the same way", { + section <- shared_fixture("citation-request")$requests + expect_gt(length(section$cases), 0) + + for (case in section$cases) { + tracker <- new.env(parent = emptyenv()) + tracker$reminder <- section$reminder + + for (step in case$steps) { + if (identical(step$action, "reset")) { + tracker$requested <- FALSE + next + } + result <- add_citation_request( + tool_result( + fixture_tool_value(step$value), + title = "Ran SQL", + tag = "B" + ), + tracker + ) + expect_equal( + fixture_value_shape(result@value), + step$expected, + info = case$name + ) + } + } +}) - result <- add_citation_request(result, tracker) +test_that("both packages agree on which turns reset the citation request", { + section <- shared_fixture("citation-request")$resets + expect_gt(length(section$cases), 0) + + for (case in section$cases) { + contents <- lapply(case$contents, function(kind) { + if (identical(kind, "text")) { + ellmer::ContentText(text = "a question") + } else { + tool_result("6 rows", title = "Ran SQL", tag = "B") + } + }) - expect_length(result@value, 2) - expect_match(result@value[[2]]@text, "", fixed = TRUE) + expect_identical( + turn_has_user_message(ellmer::Turn("user", contents = contents)), + case$resets, + info = case$name + ) + } }) test_that("search_context requests a citation for fallback answers", { diff --git a/tests/shared/README.md b/tests/shared/README.md index b1ddabc8..aca4f1c2 100644 --- a/tests/shared/README.md +++ b/tests/shared/README.md @@ -25,6 +25,7 @@ The Python suite reads this directory directly. The R suite cannot. `testthat` n - **Span names and attributes.** `commons_conversation_turn`, `commons_agent_create`, `commons_data_source_create`, and friends. Also `gen_ai.conversation.id`, `commons.provenance.tag`, and the exact JSON shape of `commons.citation.candidates`. This contract lets the R trajectory reviewer read Python traces. Write it so that it survives the conversation-id ownership moving upstream to shinychat. - **The provenance and citation behavior.** The `derive_provenance_tag()` truth table, `normalize_citation()` input/output pairs, `match_citation()` verdicts including both guards (10-character minimum, only-the-quote-verifies), `parse_commons_citation()` well-formed and malformed bodies. Also the chunk-invariance cases of the streaming scanner. The scanner is a pure chunks-in/string-out function, so it is ideal fixture material. - **System prompt rendering.** The template lives in `prompts/`, not here, but what each renderer makes of it does: `prompt-render.json` holds the data a renderer receives and the prompt it must produce. A shared template pins the words, not the rendering. +- **Where the citation request lands.** `citation-request.json` pins the reminder to the first tool result of a user turn whose output has to be cited: which value shapes it joins and how, and which turns start a new request. The wording lives in `prompts/citation-request.md`, so the fixture uses a stand-in for it. - **The citation dialect and display copy.** The `` grammar and the `PROVENANCE_DISPLAY` strings, so that both UIs say the same words. - **The definitions interface.** `definition-export/` holds the shared fixtures: 14 data dictionaries in data-dict's YAML format, each declaring table-level `definitions` whose expressions use data-dict's expression language — 3 valid files (42 definitions) and 11 invalid ones — read by both commons implementations. `definitions.json` pins what both packages agree to produce from them, in three sections: - `export_records` — the expected export for each valid definition: its SQL translation, its inferred kind and type, and the columns and definitions it references. Generated from the data-dict binary at the pinned commit by `scripts/generate-definitions-fixture.sh`, which refuses to run against a binary built from anything else. Never hand-edit. diff --git a/tests/shared/citation-request.json b/tests/shared/citation-request.json new file mode 100644 index 00000000..8e486ac4 --- /dev/null +++ b/tests/shared/citation-request.json @@ -0,0 +1,118 @@ +{ + "description": "How the citation request reaches the model. The citation contract itself lives in the system prompt; a short reminder rides along on the first tool result of a user turn whose output has to be cited. The source is tests/shared/citation-request.json; the copy under pkg-r/tests/testthat/fixtures/shared/ is generated by scripts/sync-shared.sh. Edit the source and re-run that script.", + "requests": { + "description": "Each case drives one tracker through a sequence of steps. `add` passes a tool result through the tracker and states the value it must come back with; `reset` is what a new user message does to the tracker. The reminder is a stand-in for the real text, which lives in prompts/citation-request.md, so these cases pin placement rather than wording. A value is either the text a tool returned or a list of content parts, and the reminder joins each shape in its own way.", + "reminder": "SHORT CITATION REMINDER", + "cases": [ + { + "name": "one reminder per user turn", + "steps": [ + { + "action": "add", + "value": { + "kind": "text", + "text": "6 rows" + }, + "expected": { + "kind": "text", + "text": "6 rows\n\nSHORT CITATION REMINDER" + } + }, + { + "action": "add", + "value": { + "kind": "text", + "text": "3 rows" + }, + "expected": { + "kind": "text", + "text": "3 rows" + } + }, + { + "action": "reset" + }, + { + "action": "add", + "value": { + "kind": "text", + "text": "2 rows" + }, + "expected": { + "kind": "text", + "text": "2 rows\n\nSHORT CITATION REMINDER" + } + } + ] + }, + { + "name": "content parts take the reminder as another part", + "steps": [ + { + "action": "add", + "value": { + "kind": "parts", + "parts": [ + "output" + ] + }, + "expected": { + "kind": "parts", + "parts": [ + "output", + "SHORT CITATION REMINDER" + ] + } + }, + { + "action": "add", + "value": { + "kind": "parts", + "parts": [ + "more output" + ] + }, + "expected": { + "kind": "parts", + "parts": [ + "more output" + ] + } + } + ] + } + ] + }, + "resets": { + "description": "Which turns clear the flag, so the next eligible tool result asks for a citation again. A turn that carries a real user message starts a new question; a turn of nothing but tool results is the same question still running, and an empty turn is neither.", + "cases": [ + { + "name": "a user message", + "contents": [ + "text" + ], + "resets": true + }, + { + "name": "tool results only", + "contents": [ + "tool_result" + ], + "resets": false + }, + { + "name": "a user message after tool results", + "contents": [ + "tool_result", + "text" + ], + "resets": true + }, + { + "name": "no contents", + "contents": [], + "resets": false + } + ] + } +}