diff --git a/api/agent.py b/api/agent.py index e3f3a2d..52bb79d 100644 --- a/api/agent.py +++ b/api/agent.py @@ -31,7 +31,8 @@ import tools.metrics as metrics import tools.retrieval as retrieval import stub from config import settings -from contracts import ContractViolation, QuestionClass +from contracts import (ContractViolation, QuestionClass, documented_limits, + procedure_identity) from guardrails import enforce_contract log = logging.getLogger("agent") @@ -96,22 +97,39 @@ revision, effective date. If nothing relevant was retrieved, say so. PROCEDURAL_PROMPT = _BASE + """\ You are IDENTIFYING a controlled procedure, not explaining it. -Give the procedure number, revision, effective date, title, the authorising -role, and where the controlled copy is. Quote prerequisites word for word into -prerequisites_verbatim. Write no steps, no paraphrase of steps, no summary of -what the procedure involves, and no advice about what to do first. An interlock -exists because someone assessed a hazard; a bypass procedure you reconstructed -is a safety document nobody approved. +Say which procedure governs the action and that the operator should work from +the controlled copy. Do NOT state the document number, revision, effective +date, title or authorising role in your JSON fields: every one of those is +attached from the confirmed document header, because naming the wrong revision +of a procedure is the failure this system exists to prevent. -If no controlled procedure was retrieved, say so and stop. +Quote prerequisites word for word into prerequisites_verbatim. Write no steps, +no paraphrase of steps, no summary of what the procedure involves, and no +advice about what to do first. An interlock exists because someone assessed a +hazard; a bypass procedure you reconstructed is a safety document nobody +approved. + +You will not be shown the step sections at all - retrieval withholds them. + +Two different situations, which must not be confused: + + * Retrieval returned NOTHING. Say so plainly and stop. + * Retrieval returned a document that is marked draft, demo, uncontrolled, + superseded or not for plant use. IDENTIFY IT ANYWAY and quote its + prerequisites, and state that marking plainly in your answer. The marking + is information the operator needs, not a reason to withhold what was found + - "nothing was retrieved" when something was is a worse answer than either. """ ADVISORY_PROMPT = _BASE + """\ You are presenting EVIDENCE, not a recommendation. Report what has actually been done: the rates used, how many operations that is -drawn from, the outcomes, when alarms occurred, and the documented limits with -their citations. Then defer explicitly to a competent person. +drawn from, the outcomes, when alarms occurred, and the documented limits. For +each limit, set source_file to the source_file of the extract you read it from, +copied exactly. A limit whose source_file does not match a retrieved extract is +discarded, because a limit attributed to the wrong document is worse than one +not stated. Then defer explicitly to a competent person. Do not state a recommended value. Do not say what is best, optimal or safest. Do not offer a range as a disguised recommendation. "Best" depends on equipment @@ -266,16 +284,24 @@ _SCHEMA_HINTS: dict[QuestionClass, str] = { '{"answer": str} - the figures, the window, and what the rows show' ), QuestionClass.REFERENCE: '{"answer": str}', + # `procedure` is now attached ENTIRELY from evidence and config by + # contracts.procedure_identity() - identity from the confirmed header, + # controlled_copy_location from settings. The model supplies the prose and + # the verbatim prerequisites only. Asking it for identity fields it had + # never been shown is how it came to return "" for all of them. QuestionClass.PROCEDURAL: ( - '{"answer": str, "procedure": {"doc_number": str, "title": str, ' - '"revision": str, "effective_date": "YYYY-MM-DD", ' - '"authorising_role": str, "controlled_copy_location": str}, ' - '"prerequisites_verbatim": [str]}' + '{"answer": str, "prerequisites_verbatim": [str]}' ), + # documented_limits carries source_file, not a citation: the Citation is + # attached from the evidence in contracts.documented_limits(). An empty [] + # here told the model nothing about the shape, so it invented + # {"description", "citation": "WRPS-DEMO-003, Section 4"} and every + # advisory answer failed its contract. QuestionClass.ADVISORY: ( '{"answer": str, "evidence": [{"description": str, "metric": str, ' '"value": number, "unit": str, "sample_size": int}], ' - '"documented_limits": [], "deferral": str}' + '"documented_limits": [{"description": str, "value": number|str, ' + '"unit": str, "source_file": str}], "deferral": str}' ), } @@ -384,7 +410,7 @@ def _assemble( ) elif klass is QuestionClass.PROCEDURAL: payload.update( - procedure=generated.get("procedure"), + procedure=procedure_identity(evidence), prerequisites_verbatim=generated.get("prerequisites_verbatim", []), steps_provided=False, citations=evidence.get("citations", []), @@ -392,7 +418,7 @@ def _assemble( elif klass is QuestionClass.ADVISORY: payload.update( evidence=generated.get("evidence", []), - documented_limits=generated.get("documented_limits", []), + documented_limits=documented_limits(evidence, generated), recommendation_given=False, deferral=generated.get("deferral", ""), citations=evidence.get("citations", []), diff --git a/api/classifier.py b/api/classifier.py index 3e8b789..914d8c0 100644 --- a/api/classifier.py +++ b/api/classifier.py @@ -64,16 +64,85 @@ Return exactly: "missing_context": []} """ -FEW_SHOT: list[tuple[str, str]] = [ - ("Why did the wet well high level alarm come up 6 times last week?", "historical"), - ("What does the level signal fault alarm on the wet well mean?", "reference"), - ("How do I lift the interlock on Pump 02?", "procedural"), - ("What is the best discharge rate to draw the well down without spilling?", "advisory"), +# Each reply is the COMPLETE payload the system prompt asks for, not just the +# label. A few-shot reply is a shape the model copies: when these carried +# {"question_class": ...} alone, the model returned that and nothing else, so +# `confidence` was absent, defaulted to 0.0, fell below the threshold, and +# EVERY non-procedural question was downgraded to UNCLEAR. The bug is invisible +# until a real model runs - the stub supplies its own confidence - so keep +# these in step with SYSTEM_PROMPT whenever that changes. +# +# The confidences are deliberately varied and the traps carry `alternatives`, +# because a constant here teaches the model to emit that constant and the tie +# rule in apply_safety_rules() only has something to work with if the model +# populates the runners-up. +FEW_SHOT: list[tuple[str, dict[str, object]]] = [ + ( + "Why did the wet well high level alarm come up 6 times last week?", + {"question_class": "historical", "confidence": 0.95, + "alternatives": {"reference": 0.03}, + "entities": {"equipment": ["WW-101"], "tags": [], + "time_expression": "last week"}, + "missing_context": []}, + ), + ( + "What does the level signal fault alarm on the wet well mean?", + {"question_class": "reference", "confidence": 0.94, + "alternatives": {"historical": 0.04}, + "entities": {"equipment": ["WW-101"], "tags": [], + "time_expression": None}, + "missing_context": []}, + ), + ( + "How do I lift the interlock on Pump 02?", + {"question_class": "procedural", "confidence": 0.96, + "alternatives": {"reference": 0.02}, + "entities": {"equipment": ["PU-302"], "tags": [], + "time_expression": None}, + "missing_context": []}, + ), + ( + "What is the best discharge rate to draw the well down without spilling?", + {"question_class": "advisory", "confidence": 0.93, + "alternatives": {"historical": 0.05}, + "entities": {"equipment": ["WW-101"], "tags": [], + "time_expression": None}, + "missing_context": []}, + ), # Traps, drawn from the misclassification cases in eval/testset.jsonl. - ("What rate have we been running at, and what should we use tonight?", "advisory"), - ("How many times did Pump 03 trip, and how do I reset it?", "procedural"), - ("What is the high level alarm setpoint?", "reference"), - ("What was the high level alarm setpoint changed to in July?", "historical"), + # These show BOTH halves of the answer: the restrictive class wins, and the + # class it beat is named in alternatives rather than silently dropped. + ( + "What rate have we been running at, and what should we use tonight?", + {"question_class": "advisory", "confidence": 0.88, + "alternatives": {"historical": 0.62}, + "entities": {"equipment": [], "tags": [], "time_expression": "tonight"}, + "missing_context": []}, + ), + ( + "How many times did Pump 03 trip, and how do I reset it?", + {"question_class": "procedural", "confidence": 0.87, + "alternatives": {"historical": 0.64}, + "entities": {"equipment": ["PU-303"], "tags": [], + "time_expression": None}, + "missing_context": ["time_expression"]}, + ), + ( + "What is the high level alarm setpoint?", + {"question_class": "reference", "confidence": 0.92, + "alternatives": {"historical": 0.06}, + "entities": {"equipment": ["WW-101"], "tags": [], + "time_expression": None}, + "missing_context": []}, + ), + ( + "What was the high level alarm setpoint changed to in July?", + {"question_class": "historical", "confidence": 0.90, + "alternatives": {"reference": 0.11}, + "entities": {"equipment": ["WW-101"], "tags": [], + "time_expression": "July"}, + "missing_context": []}, + ), ] @@ -147,11 +216,9 @@ def classify(question: str, *, client, trace=None) -> Classification: """ cfg = settings() messages = [{"role": "system", "content": SYSTEM_PROMPT}] - for example, label in FEW_SHOT: + for example, reply in FEW_SHOT: messages.append({"role": "user", "content": example}) - messages.append( - {"role": "assistant", "content": json.dumps({"question_class": label})} - ) + messages.append({"role": "assistant", "content": json.dumps(reply)}) messages.append({"role": "user", "content": question}) response = client.chat.completions.create( diff --git a/api/contracts.py b/api/contracts.py index 1e1d49f..fef736a 100644 --- a/api/contracts.py +++ b/api/contracts.py @@ -22,6 +22,8 @@ from typing import Any, Literal from pydantic import BaseModel, Field, model_validator +from config import settings + class QuestionClass(str, Enum): """Assigned by the classifier before any generation happens. @@ -204,6 +206,55 @@ class ProcedureIdentity(BaseModel): controlled_copy_location: str +def procedure_identity(evidence: dict[str, Any]) -> dict[str, Any] | None: + """Which controlled document this is. Built ENTIRELY from evidence. + + Every field here was at some point the model's to supply, and every one of + them failed: + + * doc_number, revision, effective_date - the wrong-revision hazard. The + model returned effective_date "", which is neither a date nor None, so + ProceduralAnswer rejected the answer twice and /ask returned 422. + * title and authorising_role - returned "" whenever the header chunk was + not among the retrieved sections, which under the old similarity-ranked + find_procedure() was most of the time. + * controlled_copy_location - a site fact, the same for every document, + and the one field where an invented value sends a person to a place + that does not exist. + + So none of it is the model's any more. The header fields come from the + confirmed header via doc_chunks (migration 007), the copy location from + settings. The model writes the prose and quotes the prerequisites; it does + not get a say in which document an operator is pointed at. + + Returns None when nothing was retrieved. ProceduralAnswer.check() then + requires the prose to say so - a procedure asserted with no evidence behind + it is refused there, not papered over here. + """ + citations = evidence.get("citations") or [] + if not citations: + return None + + top = citations[0] + return { + "doc_number": top["doc_number"], + "revision": top["revision"], + "effective_date": top["effective_date"], + "title": top.get("title") or top["source_file"], + "authorising_role": _authorising_role(evidence), + "controlled_copy_location": settings().controlled_copy_location, + } + + +def _authorising_role(evidence: dict[str, Any]) -> str | None: + """From the confirmed header, denormalised onto every chunk by ingest.""" + for chunk in evidence.get("chunks") or []: + role = chunk.get("authorising_role") + if role: + return role + return None + + class ProceduralAnswer(BaseAnswer): """Locate and cite. Never paraphrase, never reconstruct, never instruct. @@ -285,6 +336,37 @@ class DocumentedLimit(BaseModel): citation: Citation +def documented_limits( + evidence: dict[str, Any], generated: dict[str, Any] +) -> list[dict[str, Any]]: + """Attach the real Citation to each limit the model described. + + Same rule as procedure_identity(): the model says what the limit IS, the + evidence says which controlled document states it. Previously the model + supplied `citation` itself and returned the string "WRPS-DEMO-003, Section + 4" where a Citation object was required, so every advisory answer failed + its contract twice and /ask returned 422. + + A limit whose source_file matches nothing that was actually retrieved is + DROPPED, not attached to the nearest citation. A documented limit carries + the authority of the document behind it; pointing it at the wrong document + is worse than not stating it. + """ + by_source = {c["source_file"]: c for c in evidence.get("citations") or []} + limits = [] + for limit in generated.get("documented_limits") or []: + citation = by_source.get(limit.get("source_file")) + if citation is None: + continue + limits.append({ + "description": limit.get("description", ""), + "value": limit.get("value"), + "unit": limit.get("unit"), + "citation": citation, + }) + return limits + + class AdvisoryAnswer(BaseAnswer): """'Best' depends on equipment condition and concurrent operations this system cannot see. A number presented as an answer gets typed into a diff --git a/api/tests/test_model_contract_shapes.py b/api/tests/test_model_contract_shapes.py new file mode 100644 index 0000000..45a4c37 --- /dev/null +++ b/api/tests/test_model_contract_shapes.py @@ -0,0 +1,161 @@ +"""Three defects that only appear once a real model runs, pinned in Python. + +All three were invisible under NO_LLM_STUB and surfaced within minutes of the +first live Azure OpenAI call on 2026-08-27. None of them needs an API key or a +database to test, which is the whole argument for putting them here. +""" + +import json + +import pytest + +from classifier import FEW_SHOT +from config import settings +from contracts import QuestionClass, documented_limits, procedure_identity + +# --------------------------------------------------------------------------- +# 1. The classifier few-shot must model the WHOLE reply shape +# --------------------------------------------------------------------------- + +# Read by classify(); an absent key silently becomes a default, and for +# `confidence` that default is 0.0 - below every sane threshold. +REQUIRED_KEYS = {"question_class", "confidence", "alternatives", + "entities", "missing_context"} + + +def test_every_few_shot_reply_carries_the_full_payload(): + """The bug: replies were {"question_class": ...} alone. + + A few-shot reply is a shape the model copies. Omitting `confidence` taught + it to omit `confidence`, so every question scored 0.0 and downgraded to + UNCLEAR. + """ + for question, reply in FEW_SHOT: + missing = REQUIRED_KEYS - reply.keys() + assert not missing, f"{question!r} omits {sorted(missing)}" + + +def test_few_shot_confidences_are_usable_and_not_constant(): + confidences = [r["confidence"] for _, r in FEW_SHOT] + assert all(0.0 < c <= 1.0 for c in confidences) + # A constant teaches the model to emit that constant. + assert len(set(confidences)) > 1 + + +def test_few_shot_replies_are_json_serialisable(): + for _, reply in FEW_SHOT: + json.loads(json.dumps(reply)) + + +def test_few_shot_classes_are_real_classes(): + for _, reply in FEW_SHOT: + QuestionClass(reply["question_class"]) + for name in reply["alternatives"]: + QuestionClass(name) + + +# --------------------------------------------------------------------------- +# 3. Procedure identity comes from evidence, never from the model +# --------------------------------------------------------------------------- + +CITATION = { + "doc_number": "WRPS-DEMO-001", + "title": "Temporary Bypass of Pump Motor Protection Interlock", + "revision": "0", + "effective_date": "2026-01-01", + "page": 1, + "section_title": "2. Prerequisites", + "source_file": "procedures/DEMO-interlock-bypass.md", + "superseded": False, +} + +# authorising_role is denormalised onto every chunk of the document by ingest. +CHUNKS = [{"authorising_role": "Station Maintenance Supervisor"}] + + +def test_identity_ignores_the_model_entirely(): + """The wrong-revision hazard. procedure_identity takes no `generated` + argument at all now: there is no path by which a model can name a revision + an operator does not hold.""" + identity = procedure_identity({"citations": [CITATION], "chunks": CHUNKS}) + assert identity["doc_number"] == "WRPS-DEMO-001" + assert identity["revision"] == "0" + # The bug: the model returned "", which is neither a date nor None, so + # ProceduralAnswer rejected it twice and /ask returned 422. + assert identity["effective_date"] == "2026-01-01" + + +def test_title_comes_from_the_confirmed_header(): + """Was "" whenever the header chunk was not retrieved - which under the old + similarity-ranked find_procedure() was most of the time.""" + identity = procedure_identity({"citations": [CITATION], "chunks": CHUNKS}) + assert identity["title"] == "Temporary Bypass of Pump Motor Protection Interlock" + + +def test_authorising_role_comes_from_the_chunks(): + identity = procedure_identity({"citations": [CITATION], "chunks": CHUNKS}) + assert identity["authorising_role"] == "Station Maintenance Supervisor" + + +def test_controlled_copy_location_comes_from_settings(): + """Never the model's: an invented location sends somebody to a place that + does not exist.""" + identity = procedure_identity({"citations": [CITATION], "chunks": CHUNKS}) + assert identity["controlled_copy_location"] == settings().controlled_copy_location + assert identity["controlled_copy_location"] + + +def test_title_falls_back_to_source_file_on_a_pre_007_row(): + stale = {**CITATION, "title": None} + identity = procedure_identity({"citations": [stale], "chunks": [{}]}) + assert identity["title"] == "procedures/DEMO-interlock-bypass.md" + + +def test_no_evidence_means_no_procedure(): + """Never assert a procedure with nothing retrieved behind it.""" + assert procedure_identity({"citations": [], "chunks": []}) is None + + +# --------------------------------------------------------------------------- +# 4. Documented limits are cited from evidence, never by the model +# --------------------------------------------------------------------------- + +DESIGN_CITATION = { + "doc_number": "WRPS-DEMO-003", + "title": "Design Basis", + "revision": "0", + "effective_date": "2026-01-01", + "page": 1, + "section_title": "4. Hydraulics", + "source_file": "design/DEMO-design-basis.md", + "superseded": False, +} + + +def test_limit_gets_the_real_citation_object(): + """The bug: the model returned citation as the string + "WRPS-DEMO-003, Section 4" where a Citation was required, so every advisory + answer failed its contract twice and /ask returned 422.""" + generated = {"documented_limits": [ + {"description": "Spill weir crest", "value": 6000, "unit": "mm", + "source_file": "design/DEMO-design-basis.md", + "citation": "WRPS-DEMO-003, Section 4"}, + ]} + limits = documented_limits({"citations": [DESIGN_CITATION]}, generated) + assert limits[0]["citation"] == DESIGN_CITATION + assert limits[0]["value"] == 6000 + assert limits[0]["unit"] == "mm" + + +def test_limit_with_an_unretrieved_source_is_dropped(): + """Never attach a limit to the nearest citation. A limit carries the + authority of the document behind it.""" + generated = {"documented_limits": [ + {"description": "Invented limit", "value": 42, + "source_file": "design/NOT-RETRIEVED.md"}, + ]} + assert documented_limits({"citations": [DESIGN_CITATION]}, generated) == [] + + +def test_no_limits_is_an_empty_list_not_none(): + assert documented_limits({"citations": [DESIGN_CITATION]}, {}) == []