From 5aab98e4d72b26a35787adfb79345ec585faf45a Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 26 Apr 2026 15:03:13 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20classifier=20robustness=20=E2=80=94=20sc?= =?UTF-8?q?hema=20in=20prompt,=20retries,=20kickoff=20fallback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The kickoff parse-and-confirm route was 500-ing intermittently because Hermes-3 + Featherless's response_format={"type":"json_object"} only guarantees JSON output, NOT a particular schema. The model was inventing its own field names (sceneTime, entities, settingDetails) instead of the KickoffParse fields, causing Pydantic validation to fail on both classify() retries. Three changes: 1. Include the Pydantic JSON schema in the system prompt so the model knows exactly which keys to produce. Affects every classify() call (kickoff parse, turn parse, scene-close detect, significance, state-update, scene summarize). Strip ```json fences if the model wraps its output. Bump retries 2 → 3 (model is stochastic; one extra attempt closes most of the remaining gap). 2. parse_kickoff() now passes a default empty KickoffParse so the route degrades to a fillable form instead of 500 when the classifier ultimately fails. The confirm form is the human-in-the-loop; an empty form is strictly better UX than a stack trace. 3. Tests updated: bumped canned-failure arrays from 2 → 3 entries to match the new attempt count; renamed kickoff test from "raises_when_classifier_fails_twice" to "falls_back_to_empty_when_classifier_fails" reflecting the new degraded-but-usable behavior. Verified live with all 3 sample bots (maya/eli/sam) — kickoff route returns 200 across multiple attempts. Full suite: 168 passed. --- chat/llm/classify.py | 31 +++++++++++++++++++++++----- chat/services/kickoff.py | 39 ++++++++++++++++++++++++++++++----- tests/test_classify.py | 2 +- tests/test_kickoff.py | 35 ++++++++++++++++++++----------- tests/test_per_pov_summary.py | 2 +- tests/test_scene_close.py | 2 +- tests/test_significance.py | 2 +- tests/test_state_update.py | 2 +- tests/test_turn_parse.py | 2 +- 9 files changed, 89 insertions(+), 28 deletions(-) diff --git a/chat/llm/classify.py b/chat/llm/classify.py index 66517c6..7074c3b 100644 --- a/chat/llm/classify.py +++ b/chat/llm/classify.py @@ -10,6 +10,18 @@ T = TypeVar("T", bound=BaseModel) REFUSAL_PATTERNS = ("i can't", "i cannot", "i'm sorry, but", "as an ai") +def _strip_json_fences(text: str) -> str: + """Strip ```json ... ``` markdown fences if the model wraps its JSON output.""" + s = text.strip() + if s.startswith("```"): + # Drop the first fence line (which may be ``` or ```json) + s = s.split("\n", 1)[1] if "\n" in s else s[3:] + # Drop the trailing fence + if s.rstrip().endswith("```"): + s = s.rstrip()[:-3] + return s.strip() + + async def classify( client: LLMClient, *, @@ -20,21 +32,30 @@ async def classify( default: T | None = None, timeout_s: float = 10.0, ) -> T: + schema_json = json.dumps(schema.model_json_schema(), indent=2) + schema_block = ( + f"\n\nRespond with a single JSON object matching this exact schema. " + f"Use these field names exactly; do not invent your own keys:\n```json\n{schema_json}\n```" + ) msgs = [ - Message(role="system", content=system + "\n\nRespond with JSON only matching the schema."), + Message(role="system", content=system + schema_block), Message(role="user", content=user), ] - for attempt in range(2): + for attempt in range(3): try: text = await asyncio.wait_for( client.generate(msgs, model=model, response_format={"type": "json_object"}), timeout=timeout_s, ) - if any(p in text.lower()[:80] for p in REFUSAL_PATTERNS) and not text.strip().startswith("{"): + cleaned = _strip_json_fences(text) + if any(p in cleaned.lower()[:80] for p in REFUSAL_PATTERNS) and not cleaned.lstrip().startswith("{"): raise ValueError("refusal-shaped response") - return schema.model_validate_json(text) + return schema.model_validate_json(cleaned) except (ValidationError, ValueError, json.JSONDecodeError, asyncio.TimeoutError): - msgs[0] = Message(role="system", content=system + "\n\nRespond with valid JSON ONLY. No prose.") + msgs[0] = Message( + role="system", + content=system + schema_block + "\n\nRespond with valid JSON ONLY. No prose, no markdown fences.", + ) continue if default is None: raise RuntimeError(f"classify failed for schema {schema.__name__} with no default") diff --git a/chat/services/kickoff.py b/chat/services/kickoff.py index 591b09c..a5540bf 100644 --- a/chat/services/kickoff.py +++ b/chat/services/kickoff.py @@ -85,6 +85,36 @@ def _build_user_prompt( ) +def _empty_activity() -> ActivityShape: + return ActivityShape( + posture="", + action_verb="", + action_interruptible=True, + action_required_attention="low", + action_expected_duration="brief", + ) + + +def _empty_kickoff_parse() -> KickoffParse: + """Default returned when the classifier can't produce a valid parse. + + The user gets a mostly-empty confirm form they can fill in by hand + instead of a 500. ``initial_time_iso`` is left as the current UTC. + """ + from datetime import datetime, timezone + + return KickoffParse( + container_name="", + container_type="", + container_properties={}, + you_activity=_empty_activity(), + bot_activity=_empty_activity(), + initial_time_iso=datetime.now(timezone.utc).isoformat(timespec="seconds"), + edge_seed_summary="", + edge_seed_knowledge_facts=[], + ) + + async def parse_kickoff( client: LLMClient, *, @@ -98,11 +128,9 @@ async def parse_kickoff( ) -> KickoffParse: """Parse authored kickoff prose into a structured ``KickoffParse``. - Internally calls :func:`chat.llm.classify.classify` with a labeled - user prompt. Raises ``RuntimeError`` if the classifier fails twice in - a row — no default is supplied at this layer, since the caller (T13's - confirm form) is responsible for showing an error and letting the - user edit. + Falls back to a mostly-empty default if the classifier fails — the + confirm-and-edit form is the human-in-the-loop, so a degraded form + that the user can fill in is preferable to a 500. """ user_prompt = _build_user_prompt( bot_name=bot_name, @@ -117,5 +145,6 @@ async def parse_kickoff( system=_SYSTEM_PROMPT, user=user_prompt, schema=KickoffParse, + default=_empty_kickoff_parse(), timeout_s=timeout_s, ) diff --git a/tests/test_classify.py b/tests/test_classify.py index d059eaa..c521c70 100644 --- a/tests/test_classify.py +++ b/tests/test_classify.py @@ -18,7 +18,7 @@ async def test_classify_parses_valid_json(): @pytest.mark.asyncio async def test_classify_falls_back_on_unparseable_after_retry(): - mock = MockLLMClient(canned=["nope", "still nope"]) + mock = MockLLMClient(canned=["nope", "still nope", "nope3"]) default = Verdict(score=1, reason="fallback") result = await classify(mock, model="m", system="x", user="y", schema=Verdict, default=default) assert result.reason == "fallback" diff --git a/tests/test_kickoff.py b/tests/test_kickoff.py index c3dfc14..4efa2a7 100644 --- a/tests/test_kickoff.py +++ b/tests/test_kickoff.py @@ -117,15 +117,26 @@ async def test_parse_kickoff_applies_activity_defaults_for_missing_fields(): @pytest.mark.asyncio -async def test_parse_kickoff_raises_when_classifier_fails_twice(): - mock = MockLLMClient(canned=["nope", "still nope"]) - with pytest.raises(RuntimeError): - await parse_kickoff( - mock, - model="m", - bot_name="BotA", - bot_persona="x", - initial_relationship_to_you="y", - kickoff_prose="z", - you_name="You", - ) +async def test_parse_kickoff_falls_back_to_empty_when_classifier_fails(): + """When the classifier fails three times, return an empty KickoffParse + instead of raising — the confirm form lets the user fill in by hand. + """ + mock = MockLLMClient(canned=["nope", "still nope", "still bad"]) + result = await parse_kickoff( + mock, + model="m", + bot_name="BotA", + bot_persona="x", + initial_relationship_to_you="y", + kickoff_prose="z", + you_name="You", + ) + assert isinstance(result, KickoffParse) + assert result.container_name == "" + assert result.container_type == "" + assert result.edge_seed_summary == "" + assert result.edge_seed_knowledge_facts == [] + # Activity defaults sane (action_interruptible defaults to True so the + # confirm form's checkbox is in a reasonable initial state). + assert result.you_activity.action_interruptible is True + assert result.bot_activity.action_interruptible is True diff --git a/tests/test_per_pov_summary.py b/tests/test_per_pov_summary.py index d6ce0f2..3e9d597 100644 --- a/tests/test_per_pov_summary.py +++ b/tests/test_per_pov_summary.py @@ -84,7 +84,7 @@ async def test_summarize_scene_default_on_failure(): """Two consecutive non-JSON returns trip the classifier's retry-then-default path; we should get the empty fallback rather than crashing the close flow.""" - mock = MockLLMClient(canned=["bad", "still bad"]) + mock = MockLLMClient(canned=["bad", "still bad", "bad3"]) result = await summarize_scene( mock, model="x", diff --git a/tests/test_scene_close.py b/tests/test_scene_close.py index 83a9701..41adf4b 100644 --- a/tests/test_scene_close.py +++ b/tests/test_scene_close.py @@ -58,7 +58,7 @@ async def test_detect_scene_close_default_on_failure(): """Two consecutive non-JSON returns trip the classifier's retry-then-default path; we should get the safe ``should_close=False`` fallback rather than crashing the turn flow.""" - mock = MockLLMClient(canned=["nope", "still nope"]) + mock = MockLLMClient(canned=["nope", "still nope", "nope3"]) decision = await detect_scene_close( mock, model="x", diff --git a/tests/test_significance.py b/tests/test_significance.py index e5a538c..97445ac 100644 --- a/tests/test_significance.py +++ b/tests/test_significance.py @@ -45,7 +45,7 @@ async def test_compute_significance_parses_score(): async def test_compute_significance_default_on_failure(): # Both attempts return non-JSON text; the classify wrapper falls back # to the SignificanceVerdict default (score=1, "fallback"). - mock = MockLLMClient(canned=["nope", "still nope"]) + mock = MockLLMClient(canned=["nope", "still nope", "nope3"]) score = await compute_significance( mock, model="x", diff --git a/tests/test_state_update.py b/tests/test_state_update.py index e7c19b3..8fad50a 100644 --- a/tests/test_state_update.py +++ b/tests/test_state_update.py @@ -62,7 +62,7 @@ async def test_compute_state_update_parses_classifier_output(): @pytest.mark.asyncio async def test_compute_state_update_returns_default_on_failure(): """Two malformed classifier responses -> default StateUpdate (zeros).""" - mock = MockLLMClient(canned=["nope", "still nope"]) + mock = MockLLMClient(canned=["nope", "still nope", "nope3"]) result = await compute_state_update( mock, model="x", diff --git a/tests/test_turn_parse.py b/tests/test_turn_parse.py index 3246bd4..026f9b1 100644 --- a/tests/test_turn_parse.py +++ b/tests/test_turn_parse.py @@ -74,7 +74,7 @@ async def test_parse_turn_empty_prose_short_circuits_without_classifier_call(): @pytest.mark.asyncio async def test_parse_turn_raises_when_classifier_fails_twice(): - mock = MockLLMClient(canned=["nope", "still nope"]) + mock = MockLLMClient(canned=["nope", "still nope", "nope3"]) with pytest.raises(RuntimeError): await parse_turn( mock,