diff --git a/codeframe/cli/app.py b/codeframe/cli/app.py index 406d59ce..d03aa6a1 100644 --- a/codeframe/cli/app.py +++ b/codeframe/cli/app.py @@ -1713,6 +1713,7 @@ def prd_stress_test( from codeframe.core.llm_resolution import resolve_llm_settings, create_provider from codeframe.cli.validators import require_api_key_for_provider from codeframe.core.prd_stress_test import ( + StressTestError, stress_test_prd, resolve_ambiguities_into_prd, ) @@ -1749,7 +1750,13 @@ def prd_stress_test( # Run stress test console.print(f"[dim]Recursively decomposing (max depth: {max_depth})...[/dim]") - result = stress_test_prd(record.content, provider, max_depth=max_depth) + try: + result = stress_test_prd(record.content, provider, max_depth=max_depth) + except StressTestError as e: + # extract_goals now raises rather than returning [] (#927). Without this + # the CLI shows a traceback where every other failure here is a red line. + console.print(f"[red]Error:[/red] {e}") + raise typer.Exit(1) # Show ambiguity report if result.ambiguities: @@ -1791,7 +1798,11 @@ def prd_stress_test( ) # Re-run stress test on updated PRD to reflect resolved ambiguities console.print("[dim]Re-analyzing updated PRD...[/dim]") - result = stress_test_prd(new_record.content, provider, max_depth=max_depth) + try: + result = stress_test_prd(new_record.content, provider, max_depth=max_depth) + except StressTestError as e: + console.print(f"[red]Error:[/red] {e}") + raise typer.Exit(1) else: console.print("[yellow]Warning:[/yellow] Failed to create new PRD version.") @@ -1806,6 +1817,11 @@ def prd_stress_test( # Summary node_count = _count_nodes(result.tree) console.print(f"\n[bold]Summary:[/bold] {len(result.tree)} goals, {node_count} nodes, {len(result.ambiguities)} ambiguities") + if result.partial: + console.print( + "[yellow]Warning:[/yellow] LLM call budget exhausted — the walk is " + "partial. Ambiguities listed are real, but others may be unreported." + ) if result.ambiguities and not interactive: console.print("[dim]Tip: Run with --interactive to resolve ambiguities and update the PRD[/dim]") diff --git a/codeframe/core/llm_json.py b/codeframe/core/llm_json.py new file mode 100644 index 00000000..470a2c37 --- /dev/null +++ b/codeframe/core/llm_json.py @@ -0,0 +1,62 @@ +"""Parsing JSON out of an LLM response (#927). + +A **leaf module**: stdlib only, so every consumer can converge on it. + +Models routinely wrap JSON in a markdown fence, and local / OpenAI-compatible +providers do it more often than Anthropic. Four call sites in this repo each +grew their own fence stripper, in two subtly different shapes — and +``prd_stress_test`` grew none at all, so it did a raw ``json.loads``, swallowed +the failure, and reported "No ambiguities found — PRD is well-specified" after +the user had paid for the call. + +The lesson in that bug is the reason this module exists: a parser that returns a +falsy default on failure turns a provider quirk into a clean bill of health. +``parse_json_response`` raises instead, and callers decide what to do about it. +""" + +from __future__ import annotations + +import json +import re +from typing import Any + +__all__ = ["LLMJsonError", "parse_json_response", "strip_code_fence"] + +#: A fenced block, with or without a language tag, anywhere in the response. +#: Non-greedy so the *first* complete block wins when a model emits several. +_FENCE_RE = re.compile(r"```[ \t]*[A-Za-z0-9_+-]*[ \t]*\r?\n(.*?)```", re.DOTALL) + + +class LLMJsonError(ValueError): + """An LLM response could not be parsed as JSON.""" + + +def strip_code_fence(content: str) -> str: + """Return the contents of the first markdown fence, or the input unchanged. + + Tolerates prose around the block ("Sure! Here you go:"), which local models + add routinely, and a fence with no language tag. + """ + match = _FENCE_RE.search(content) + return match.group(1).strip() if match else content.strip() + + +def parse_json_response(content: str, *, what: str = "response") -> Any: + """Parse an LLM response as JSON, stripping any markdown fence. + + Raises: + LLMJsonError: If the content is empty or is not JSON once unfenced. + Deliberately an exception rather than a falsy default — the caller + must not be able to mistake a parse failure for an empty result. + """ + if not content or not content.strip(): + raise LLMJsonError(f"Empty {what} — nothing to parse") + + stripped = strip_code_fence(content) + try: + return json.loads(stripped) + except (json.JSONDecodeError, TypeError) as exc: + preview = stripped[:200].replace("\n", " ") + raise LLMJsonError( + f"Could not parse {what} as JSON: {exc}. Content began: {preview!r}" + ) from exc diff --git a/codeframe/core/prd_stress_test.py b/codeframe/core/prd_stress_test.py index c71bea50..28e75312 100644 --- a/codeframe/core/prd_stress_test.py +++ b/codeframe/core/prd_stress_test.py @@ -9,14 +9,14 @@ """ import asyncio -import json import logging import uuid from dataclasses import dataclass from enum import Enum -from typing import AsyncGenerator, Literal, Optional +from typing import AsyncGenerator, Callable, Literal, Optional from codeframe.adapters.llm.base import Purpose +from codeframe.core.llm_json import LLMJsonError, parse_json_response logger = logging.getLogger(__name__) @@ -65,6 +65,9 @@ class StressTestResult: ambiguities: list[Ambiguity] tech_spec_markdown: str ambiguity_report: str + # True when the call budget ran out mid-walk: the ambiguities found are + # real, but absence of others is not evidence of their absence (#927). + partial: bool = False # --------------------------------------------------------------------------- @@ -120,6 +123,67 @@ class StressTestResult: # --------------------------------------------------------------------------- +#: A model can return an arbitrarily long ``children`` list. The depth cap alone +#: does not bound the walk: breadth multiplies at every level, and each node is +#: one paid LLM call (#927). +MAX_CHILDREN_PER_NODE = 12 + +#: Total classification calls for one stress-test run. Depth 10 (the API +#: maximum) with even modest breadth is thousands of calls; this is the ceiling +#: the user's bill actually cares about. +MAX_LLM_CALLS = 200 + + +class StressTestError(RuntimeError): + """The stress test could not produce a usable result. + + Raised rather than returning an empty result: the CLI and web UI report + "No ambiguities found — PRD is well-specified" for an empty run, so a + silent failure reads to the user as a passing grade (#927). + """ + + +@dataclass +class _Budget: + """Bounds one stress-test walk: total calls, plus caller cancellation. + + ``is_cancelled`` is polled before every call so a disconnected SSE client + stops paying for work nobody will read (#927). + """ + + max_calls: int = MAX_LLM_CALLS + is_cancelled: Optional[Callable[[], bool]] = None + spent: int = 0 + exhausted: bool = False + cancelled: bool = False + + def take(self) -> bool: + """Claim one LLM call. False means stop walking.""" + if self.is_cancelled is not None and self.is_cancelled(): + self.cancelled = True + return False + if self.spent >= self.max_calls: + self.exhausted = True + return False + self.spent += 1 + return True + + @property + def stopped_early(self) -> bool: + return self.exhausted or self.cancelled + + +def _cap_children(children: list[dict]) -> list[dict]: + """Truncate a model-supplied children list to a sane breadth.""" + if len(children) <= MAX_CHILDREN_PER_NODE: + return children + logger.warning( + "Model returned %d children; keeping the first %d", + len(children), MAX_CHILDREN_PER_NODE, + ) + return children[:MAX_CHILDREN_PER_NODE] + + def extract_goals(prd_content: str, provider) -> list[str]: """Extract high-level deliverable goals from a PRD.""" response = provider.complete( @@ -130,13 +194,26 @@ def extract_goals(prd_content: str, provider) -> list[str]: temperature=0.0, ) try: - goals = json.loads(response.content) - if isinstance(goals, list): - return [str(g) for g in goals] - logger.warning("Goal extraction returned non-list: %s", type(goals).__name__) - except (json.JSONDecodeError, TypeError) as exc: - logger.warning("Failed to parse goal extraction response: %s", exc) - return [] + goals = parse_json_response(response.content, what="goal extraction") + except LLMJsonError as exc: + # Never return [] here. The caller reads an empty list as "no goals to + # analyse" and reports "No ambiguities found — PRD is well-specified", + # so a provider that fenced its JSON produced a clean bill of health + # after the user had paid for the call (#927). + raise StressTestError(f"Could not read the model's goal list: {exc}") from exc + + if not isinstance(goals, list): + raise StressTestError( + f"Goal extraction returned {type(goals).__name__}, expected a list" + ) + + extracted = [str(g) for g in goals if str(g).strip()] + if not extracted: + raise StressTestError( + "The model returned no goals for this PRD. That is not a " + "well-specified PRD — it is an unusable response." + ) + return extracted def classify_and_decompose( @@ -170,11 +247,21 @@ def classify_and_decompose( ) try: - data = json.loads(response.content) - except (json.JSONDecodeError, TypeError) as exc: + data = parse_json_response(response.content, what=f"classification of {title!r}") + except LLMJsonError as exc: + # A single unparseable node degrades to a leaf rather than failing the + # whole run — unlike goal extraction, where an empty result is + # indistinguishable from success (#927). logger.warning("Failed to parse classification for '%s': %s", title, exc) return Classification.ATOMIC, [], None, "Low" + if not isinstance(data, dict): + logger.warning( + "Classification for '%s' returned %s, expected an object", + title, type(data).__name__, + ) + return Classification.ATOMIC, [], None, "Low" + raw_cls = data.get("classification", "atomic").lower() try: cls = Classification(raw_cls) @@ -184,10 +271,10 @@ def classify_and_decompose( complexity = data.get("complexity_hint", "Low") raw_children = data.get("children", []) if cls == Classification.COMPOSITE else [] # Validate children are dicts with expected keys - children = [ + children = _cap_children([ c for c in raw_children if isinstance(c, dict) and ("title" in c or "description" in c) - ] + ]) ambiguity = None if cls == Classification.AMBIGUOUS: @@ -214,10 +301,19 @@ def recursive_decompose( max_depth: int, ambiguities: list[Ambiguity], provider, + budget: Optional["_Budget"] = None, ) -> DecompositionNode: - """Recursively decompose a goal, collecting ambiguities along the way.""" - # Force leaf at max depth - if depth >= max_depth: + """Recursively decompose a goal, collecting ambiguities along the way. + + ``budget`` bounds the total number of LLM calls and lets a disconnected + caller stop the walk. A run that stops early returns the partial tree built + so far rather than raising — the ambiguities already found are real (#927). + """ + if budget is None: + budget = _Budget() + + # Force leaf at max depth, when the call budget is spent, or on cancellation + if depth >= max_depth or not budget.take(): return DecompositionNode( id=str(uuid.uuid4()), title=title, @@ -249,8 +345,11 @@ def recursive_decompose( max_depth, ambiguities, provider, + budget, ) children.append(child_node) + if budget.stopped_early: + break return DecompositionNode( id=str(uuid.uuid4()), @@ -402,7 +501,14 @@ def stress_test_prd( tree: list[DecompositionNode] = [] ambiguities: list[Ambiguity] = [] + # One budget for the whole run, matching the streaming path. Letting each + # goal default to its own ``_Budget()`` would bound the walk at + # ``len(goals) × MAX_LLM_CALLS`` instead of the documented total (#927). + budget = _Budget() + for goal in goals: + if budget.stopped_early: + break node = recursive_decompose( title=goal, description=goal, @@ -412,6 +518,7 @@ def stress_test_prd( max_depth=max_depth, ambiguities=ambiguities, provider=provider, + budget=budget, ) tree.append(node) @@ -435,11 +542,15 @@ def stress_test_prd( ambiguities=ambiguities, tech_spec_markdown=tech_spec, ambiguity_report=amb_report, + partial=budget.stopped_early, ) async def stress_test_prd_stream( - prd_content: str, provider, max_depth: int = 3 + prd_content: str, + provider, + max_depth: int = 3, + is_cancelled: Optional[Callable[[], bool]] = None, ) -> AsyncGenerator[dict, None]: """Async streaming variant of :func:`stress_test_prd`. @@ -465,7 +576,15 @@ async def stress_test_prd_stream( ambiguities: list[Ambiguity] = [] tree: list[DecompositionNode] = [] + # One budget for the whole run: the total-call ceiling and the + # cancellation check both live inside the recursion, so a disconnected + # client stops paying at the next node rather than at the next + # top-level goal (#927). + budget = _Budget(is_cancelled=is_cancelled) + for goal in goals: + if budget.stopped_early: + break node = await asyncio.to_thread( recursive_decompose, goal, # title @@ -476,6 +595,7 @@ async def stress_test_prd_stream( max_depth, ambiguities, provider, + budget, ) tree.append(node) yield { @@ -493,6 +613,9 @@ async def stress_test_prd_stream( "ambiguities": [ambiguity_to_dict(a) for a in ambiguities], "tech_spec_markdown": tech_spec, "ambiguity_report": amb_report, + # Honest about a truncated walk: the ambiguities found are real, + # but absence of others is not evidence of their absence (#927). + "partial": budget.stopped_early, } except Exception as exc: # noqa: BLE001 — surface any failure to the client logger.warning("Stress test stream failed: %s", exc, exc_info=True) diff --git a/codeframe/ui/routers/prd_v2.py b/codeframe/ui/routers/prd_v2.py index 251ebf6b..8443a580 100644 --- a/codeframe/ui/routers/prd_v2.py +++ b/codeframe/ui/routers/prd_v2.py @@ -287,15 +287,54 @@ async def _stress_test_event_stream( yield _sse({"type": "error", "message": str(exc)}) return - async for event in stress_test_prd_stream( - record.content, provider, max_depth=max_depth, - ): - # If the browser has gone away, stop iterating the core generator so its - # next (blocking, billable) LLM call is never made. - if request is not None and await request.is_disconnected(): - logger.info("Client disconnected from stress-test stream; aborting") - break - yield _sse(event) + # Latched so the *recursion* can see it, not just this loop. Breaking here + # only stopped between top-level goals, while the decomposition inside one + # goal kept issuing billable calls (#927). + disconnected = False + + async def _watch_disconnect() -> None: + # A whole top-level goal decomposes inside one `asyncio.to_thread`, so + # the per-event check below cannot fire during that walk — the loop is + # not running the generator's frame. This polls on the loop alongside + # the worker thread, so the recursion sees the latch at its next node + # rather than only at its next goal. + # ponytail: 1s poll, not a disconnect callback — a classification call + # takes seconds, so finer granularity saves nothing. + # + # Sleeps *before* its first poll deliberately. The loop below already + # checks on every event, so an immediate poll here would only duplicate + # that one — and the duplicate is observable, because it advances any + # caller whose disconnect signal is stateful rather than idempotent. + nonlocal disconnected + while not disconnected: + await asyncio.sleep(1.0) + if disconnected: + return + if await request.is_disconnected(): + logger.info("Client disconnected from stress-test stream; aborting") + disconnected = True + return + + watcher = asyncio.create_task(_watch_disconnect()) if request is not None else None + try: + async for event in stress_test_prd_stream( + record.content, provider, max_depth=max_depth, + is_cancelled=lambda: disconnected, + ): + # Still checked per event as well as on the poll interval: a stream + # that produces events faster than the poll would otherwise run to + # completion for a client that has already gone away. + if disconnected or ( + request is not None and await request.is_disconnected() + ): + logger.info("Client disconnected from stress-test stream; aborting") + disconnected = True + break + yield _sse(event) + finally: + if watcher is not None: + disconnected = True # stops the poll loop on the normal path too + watcher.cancel() @router.get("/stress-test") diff --git a/tests/core/test_cli_validators.py b/tests/core/test_cli_validators.py index 53603ec1..8cd0d960 100644 --- a/tests/core/test_cli_validators.py +++ b/tests/core/test_cli_validators.py @@ -315,18 +315,24 @@ def test_stress_test_default_provider_requires_anthropic_key( def test_stress_test_ollama_provider_skips_anthropic_key( self, workspace_with_prd, isolated_keys, monkeypatch ): - from types import SimpleNamespace - from codeframe.adapters.llm import OpenAIProvider from codeframe.cli.app import app from codeframe.core import prd_stress_test + from codeframe.core.prd_stress_test import StressTestResult seen_providers = [] def fake_stress_test(content, provider, max_depth=3): seen_providers.append(provider) - return SimpleNamespace( - ambiguities=[], tree=[], tech_spec_markdown="# Spec" + # The real dataclass, not a SimpleNamespace: this stub only cares + # about which provider was resolved, so it must not also encode a + # guess at the result's shape that goes stale when a field is added. + return StressTestResult( + prd_title="Sample PRD", + tree=[], + ambiguities=[], + tech_spec_markdown="# Spec", + ambiguity_report="", ) monkeypatch.setattr( diff --git a/tests/core/test_prd_stress_test.py b/tests/core/test_prd_stress_test.py index f80b31c6..21edc332 100644 --- a/tests/core/test_prd_stress_test.py +++ b/tests/core/test_prd_stress_test.py @@ -189,16 +189,21 @@ def test_extracts_goals_from_prd(self, sample_prd, mock_provider): assert goals == ["User Authentication", "Invoice Management", "PDF Export"] mock_provider.complete.assert_called_once() - def test_empty_prd_returns_empty(self, mock_provider): - from codeframe.core.prd_stress_test import extract_goals + def test_empty_prd_raises_rather_than_reporting_success(self, mock_provider): + """An empty goal list used to return []. The caller reports + "No ambiguities found — PRD is well-specified" for an empty run, so an + empty PRD came back as a passing grade (#927).""" + import pytest + + from codeframe.core.prd_stress_test import StressTestError, extract_goals mock_provider.complete.side_effect = None resp = MagicMock() resp.content = json.dumps([]) mock_provider.complete.return_value = resp - goals = extract_goals("", mock_provider) - assert goals == [] + with pytest.raises(StressTestError): + extract_goals("", mock_provider) class TestClassifyAndDecompose: @@ -604,16 +609,30 @@ def _provider_returning(content: str): class TestExtractGoalsErrorPaths: - def test_invalid_json_returns_empty(self): - from codeframe.core.prd_stress_test import extract_goals + def test_invalid_json_raises(self): + """Silently returning [] made a fenced-JSON provider look like a + well-specified PRD (#927).""" + import pytest + + from codeframe.core.prd_stress_test import StressTestError, extract_goals + + with pytest.raises(StressTestError): + extract_goals("PRD", _provider_returning("not json at all")) + + def test_non_list_json_raises(self): + import pytest + + from codeframe.core.prd_stress_test import StressTestError, extract_goals - assert extract_goals("PRD", _provider_returning("not json at all")) == [] + with pytest.raises(StressTestError): + extract_goals("PRD", _provider_returning('{"a": 1}')) - def test_non_list_json_returns_empty(self): + def test_a_fenced_list_is_parsed(self): + """The actual trigger: every OpenAI-compatible provider fences JSON.""" from codeframe.core.prd_stress_test import extract_goals - # Valid JSON, but an object rather than a list → treated as no goals. - assert extract_goals("PRD", _provider_returning('{"a": 1}')) == [] + fenced = "```json\n[\"Ship auth\"]\n```" + assert extract_goals("PRD", _provider_returning(fenced)) == ["Ship auth"] def test_list_of_non_strings_is_stringified(self): from codeframe.core.prd_stress_test import extract_goals diff --git a/tests/core/test_stress_test_json_927.py b/tests/core/test_stress_test_json_927.py new file mode 100644 index 00000000..20e76a1a --- /dev/null +++ b/tests/core/test_stress_test_json_927.py @@ -0,0 +1,169 @@ +"""Stress-test JSON parsing silently reported a clean bill of health (#927 / P1.9). + +``extract_goals`` did a raw ``json.loads`` and returned ``[]`` on failure; +``classify_and_decompose`` fell back to ``(ATOMIC, [], None, "Low")``. Neither +stripped ```json fences — although *every* sibling LLM-JSON consumer in this +repo does, in two subtly different ways. + +Fenced JSON is routine on the OpenAI-compatible and local providers this command +explicitly supports, so the failure mode was a silent false pass: the CLI and web +UI reported "No ambiguities found — PRD is well-specified" **after the user paid +for the call**. + +``recursive_decompose`` additionally walked an unbounded model-supplied +``children`` list with only a depth cap (API-settable to 10) and no cancellation +when the SSE client went away. +""" + +from __future__ import annotations + +import json + +import pytest + +pytestmark = pytest.mark.v2 + +_FENCE = "```" + + +def _fenced(payload, lang: str = "json") -> str: + return f"{_FENCE}{lang}\n{json.dumps(payload)}\n{_FENCE}" + + +# --------------------------------------------------------------------------- +# 1. The shared helper +# --------------------------------------------------------------------------- + + +class TestFenceStripping: + def test_strips_a_json_fence(self): + from codeframe.core.llm_json import parse_json_response + + assert parse_json_response(_fenced(["a", "b"])) == ["a", "b"] + + def test_strips_a_bare_fence(self): + from codeframe.core.llm_json import parse_json_response + + assert parse_json_response(_fenced({"k": 1}, lang="")) == {"k": 1} + + def test_accepts_unfenced_json(self): + from codeframe.core.llm_json import parse_json_response + + assert parse_json_response('["a"]') == ["a"] + + def test_tolerates_prose_around_the_fence(self): + """Local models routinely add 'Here is the JSON:' before the block.""" + from codeframe.core.llm_json import parse_json_response + + raw = f"Sure! Here you go:\n{_fenced(['a'])}\nHope that helps." + + assert parse_json_response(raw) == ["a"] + + def test_raises_on_unparseable_content(self): + """Silence is what produced the false pass — this must be loud.""" + from codeframe.core.llm_json import LLMJsonError, parse_json_response + + with pytest.raises(LLMJsonError): + parse_json_response("I'm sorry, I can't do that.") + + def test_raises_on_empty_content(self): + from codeframe.core.llm_json import LLMJsonError, parse_json_response + + with pytest.raises(LLMJsonError): + parse_json_response("") + + +# --------------------------------------------------------------------------- +# 2. Goal extraction +# --------------------------------------------------------------------------- + + +class _Provider: + def __init__(self, content: str): + self._content = content + self.calls = 0 + + def complete(self, **kwargs): + self.calls += 1 + + class _R: + content = self._content + + return _R() + + +class TestExtractGoals: + def test_a_fenced_response_yields_goals(self): + """AC1. The headline case on every OpenAI-compatible provider.""" + from codeframe.core.prd_stress_test import extract_goals + + provider = _Provider(_fenced(["Ship auth", "Ship billing"])) + + assert extract_goals("prd", provider) == ["Ship auth", "Ship billing"] + + def test_an_unfenced_response_still_works(self): + from codeframe.core.prd_stress_test import extract_goals + + provider = _Provider(json.dumps(["Ship auth"])) + + assert extract_goals("prd", provider) == ["Ship auth"] + + def test_zero_goals_raises_rather_than_reporting_a_clean_prd(self): + """AC2. Returning [] made the caller print + 'No ambiguities found — PRD is well-specified'.""" + from codeframe.core.prd_stress_test import StressTestError, extract_goals + + with pytest.raises(StressTestError): + extract_goals("prd", _Provider("not json at all")) + + def test_an_empty_list_also_raises(self): + """A well-formed empty list is the same false pass by another route.""" + from codeframe.core.prd_stress_test import StressTestError, extract_goals + + with pytest.raises(StressTestError): + extract_goals("prd", _Provider("[]")) + + +# --------------------------------------------------------------------------- +# 3. Bounded recursion +# --------------------------------------------------------------------------- + + +class TestRecursionIsBounded: + def test_a_per_node_children_cap_exists(self): + from codeframe.core import prd_stress_test + + assert prd_stress_test.MAX_CHILDREN_PER_NODE > 0 + + def test_a_total_call_budget_exists(self): + from codeframe.core import prd_stress_test + + assert prd_stress_test.MAX_LLM_CALLS > 0 + + def test_the_budget_stops_the_walk(self): + """AC3. A model returning children forever must terminate.""" + from codeframe.core.prd_stress_test import _Budget + + budget = _Budget(max_calls=3) + + assert budget.take() and budget.take() and budget.take() + assert not budget.take() + assert budget.exhausted + + def test_a_cancelled_walk_stops(self): + """AC4. A disconnected SSE client must stop the work it is paying for.""" + from codeframe.core.prd_stress_test import _Budget + + budget = _Budget(max_calls=100, is_cancelled=lambda: True) + + assert not budget.take() + + def test_children_are_truncated_not_dropped(self): + from codeframe.core import prd_stress_test + + children = [{"title": str(i)} for i in range(1000)] + + capped = prd_stress_test._cap_children(children) + + assert len(capped) == prd_stress_test.MAX_CHILDREN_PER_NODE + assert capped[0]["title"] == "0" diff --git a/tests/core/test_stress_test_review_1040.py b/tests/core/test_stress_test_review_1040.py new file mode 100644 index 00000000..190d2ce7 --- /dev/null +++ b/tests/core/test_stress_test_review_1040.py @@ -0,0 +1,196 @@ +"""Regression tests for the three gaps found in cross-family review of #927. + +Each one is a place where the PR's stated acceptance criteria and the actual +behaviour of a surface disagreed: + +1. ``extract_goals`` raises, but the CLI never caught it — AC2 says both + surfaces report the failure, and a Typer traceback is not a report. +2. ``stress_test_prd`` let every goal default to its own ``_Budget()``, so the + sync path was bounded per-goal, not per-run as AC3 requires. +3. The SSE route only sampled ``is_disconnected()`` between yielded events, so + the cancellation latch could not fire inside a goal's walk — AC4. +""" + +import asyncio +import time + +import pytest + +from codeframe.core.prd_stress_test import ( + MAX_LLM_CALLS, + StressTestError, + stress_test_prd, +) + +pytestmark = pytest.mark.v2 + + +class _Response: + def __init__(self, content): + self.content = content + + +class _CountingProvider: + """Returns goals once, then an endlessly-composite tree, counting calls.""" + + def __init__(self, goals, children_per_node=3): + self._goals = goals + self._children = children_per_node + self.calls = 0 + + def complete(self, **kwargs): + self.calls += 1 + if self.calls == 1: + import json + + return _Response(json.dumps(self._goals)) + children = [ + {"title": f"child-{self.calls}-{i}", "description": "d"} + for i in range(self._children) + ] + import json + + return _Response( + json.dumps({ + "classification": "composite", + "children": children, + "complexity_hint": "Low", + }) + ) + + +class TestSyncBudgetIsPerRun: + def test_the_budget_spans_all_goals_not_each_goal(self): + """Four goals must share one MAX_LLM_CALLS ceiling, not get one each. + + Before the fix this ran to ``goals × MAX_LLM_CALLS`` calls. + """ + provider = _CountingProvider(["g1", "g2", "g3", "g4"]) + + result = stress_test_prd("# PRD\nbody", provider, max_depth=10) + + # 1 goal-extraction call + at most MAX_LLM_CALLS classification calls. + assert provider.calls <= MAX_LLM_CALLS + 1 + assert result.partial is True + + def test_a_walk_that_fits_the_budget_is_not_partial(self): + """The flag must mean something — a small run stays non-partial.""" + + class _AtomicProvider: + def __init__(self): + self.calls = 0 + + def complete(self, **kwargs): + self.calls += 1 + import json + + if self.calls == 1: + return _Response(json.dumps(["only goal"])) + return _Response( + json.dumps({"classification": "atomic", "complexity_hint": "Low"}) + ) + + result = stress_test_prd("# PRD\nbody", _AtomicProvider(), max_depth=3) + assert result.partial is False + + +class TestCliReportsTheFailure: + def test_stress_test_command_exits_cleanly_on_unparseable_goals(self, tmp_path, monkeypatch): + """A StressTestError must become a red line + exit 1, not a traceback.""" + from typer.testing import CliRunner + + from codeframe.cli.app import app + from codeframe.core import prd as prd_module + from codeframe.core.workspace import create_or_load_workspace + + repo = tmp_path / "repo" + repo.mkdir() + workspace = create_or_load_workspace(repo, tech_stack="python") + prd_module.store(workspace, "# Demo PRD\nSome goals.", title="Demo PRD") + + monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-ant-test") + + def _boom(*args, **kwargs): + raise StressTestError("Could not read the model's goal list: no JSON found") + + monkeypatch.setattr("codeframe.core.prd_stress_test.stress_test_prd", _boom) + monkeypatch.setattr( + "codeframe.core.llm_resolution.create_provider", lambda *a, **k: object() + ) + + result = CliRunner().invoke( + app, ["prd", "stress-test", "--workspace", str(repo)] + ) + + assert result.exit_code == 1 + assert "Could not read the model's goal list" in result.output + assert not isinstance(result.exception, StressTestError) + + +class TestDisconnectLatchFiresDuringAGoal: + """Drives the real ``_stress_test_event_stream`` with a stand-in core + generator that decomposes one long 'goal' in a worker thread, polling + ``is_cancelled`` as the real recursion does. The route must flip the latch + while that thread is still running — not only between yielded events. + """ + + @pytest.mark.asyncio + async def test_the_latch_flips_mid_goal(self, monkeypatch, tmp_path): + from codeframe.core import prd as prd_module + from codeframe.core.workspace import create_or_load_workspace + from codeframe.ui.routers import prd_v2 + + repo = tmp_path / "repo" + repo.mkdir() + workspace = create_or_load_workspace(repo, tech_stack="python") + prd_module.store(workspace, "# Demo PRD\nSome goals.", title="Demo PRD") + + monkeypatch.setattr(prd_v2, "_resolve_llm_provider", lambda ws: object()) + + observed = {"cancelled_at_node": None} + + async def _fake_stream(content, provider, max_depth=3, is_cancelled=None): + yield {"type": "goals_extracted", "goals": ["one long goal"]} + + def _walk(): + # Stands in for recursive_decompose: many nodes, one thread, + # no yields back to the loop until the whole goal is done. + for node in range(200): + if is_cancelled is not None and is_cancelled(): + return node + time.sleep(0.01) + return None + + observed["cancelled_at_node"] = await asyncio.to_thread(_walk) + yield {"type": "goal_analyzed", "goal": "one long goal"} + + monkeypatch.setattr( + "codeframe.core.prd_stress_test.stress_test_prd_stream", _fake_stream + ) + + class _Request: + """Stays connected past the startup poll and the first yielded + event, then drops — so the only thing that can notice is the + concurrent poller running while the goal decomposes. + """ + + def __init__(self): + self.polls = 0 + + async def is_disconnected(self): + self.polls += 1 + return self.polls >= 3 + + frames = [] + async for frame in prd_v2._stress_test_event_stream( + workspace, max_depth=3, request=_Request() + ): + frames.append(frame) + + node = observed["cancelled_at_node"] + assert node is not None, ( + "the walk ran to completion — the latch never fired inside the goal" + ) + assert node < 200 + # The goal never finished, so its goal_analyzed frame was never sent. + assert not any("goal_analyzed" in f for f in frames)