From 40f32087a9d1b7b3db312b38e7d750c43f90f774 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 10:54:29 +1000 Subject: [PATCH] Take the facts back off the model Four faults, all surfaced within an hour of the first live Azure OpenAI call on 2026-08-27, all invisible under NO_LLM_STUB because the stub supplied the very fields that turned out to be missing. 1. The classifier few-shot showed eight replies of {"question_class": ...} alone. A few-shot reply is a shape the model copies, so it omitted confidence, which defaulted to 0.0, fell below the 0.7 threshold, and EVERY non-procedural question downgraded to UNCLEAR. The replies now carry the complete payload the system prompt asks for. Confidences are varied and the traps carry alternatives: a constant teaches the model to emit that constant, and the tie rule in apply_safety_rules only has something to work with if the runners-up are populated. 2. procedure{} was the one part of the procedural payload not assembled from evidence, contrary to _assemble's own stated rule. The model returned effective_date "" - neither a date nor None - so ProceduralAnswer rejected the answer, the single regeneration failed identically, and every procedural question returned 422. 3. title and authorising_role came back "" for the same reason: the model was asked for header fields it had never been shown. 4. documented_limits[].citation arrived as the string "WRPS-DEMO-003, Section 4" where a Citation was required, because the schema hint said only "documented_limits": [] and told the model nothing about the shape. procedure_identity now takes no `generated` argument at all: there is no path by which a model can name a revision an operator does not hold. documented_ limits attaches the real Citation by matching source_file against what was actually retrieved, and DROPS a limit matching nothing - a limit carries the authority of the document behind it, and misattributing one is worse than omitting it. Both live in contracts.py rather than agent.py because they are contract rules, and because agent.py imports langgraph, which would make the test suite unrunnable on a bare checkout. The prompt also now separates two things it was conflating: retrieval returning nothing (say so and stop) from retrieval returning a document marked draft, demo or superseded (identify it, quote it, and state the marking). Including the header chunk made the model read "NOT A CONTROLLED DOCUMENT" and answer "no controlled procedure was retrieved" while citing one. The marking is information the operator needs, not a reason to withhold what was found. Co-Authored-By: Claude Opus 5 --- api/agent.py | 60 ++++++--- api/classifier.py | 93 ++++++++++++-- api/contracts.py | 82 ++++++++++++ api/tests/test_model_contract_shapes.py | 161 ++++++++++++++++++++++++ 4 files changed, 366 insertions(+), 30 deletions(-) create mode 100644 api/tests/test_model_contract_shapes.py 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]}, {}) == []