diff --git a/codeframe/cli/app.py b/codeframe/cli/app.py index 7156181c..b3d87d2d 100644 --- a/codeframe/cli/app.py +++ b/codeframe/cli/app.py @@ -57,6 +57,207 @@ #: wrote. Pointing only at `prd add` sent new users straight past Socratic #: discovery — the capability the product leads on. `cf` rather than #: `codeframe` because that is the name the README uses; both binaries work. +#: How many rejected attempts at one discovery question before the loop stops +#: and says something. Without a cap, a user who keeps missing a sub-clause of a +#: two-part question loops forever with coverage pinned at 0% and no hint that +#: they are stuck — measured at 21 turns / 0 accepted answers (#1114). +MAX_ANSWER_ATTEMPTS = 5 + + +class AnswersExhausted(Exception): + """A non-interactive discovery run ran out of answers (#1114). + + Distinct from EOF inside ``Prompt.ask``, which is what happened before and + surfaced as an opaque traceback with no indication of which question was + unanswered or how many had been consumed. + """ + + +#: Prompt for --brief-file. Deliberately instructs the stand-in to answer the +#: question actually asked, including every part of a multi-part one — that is +#: the difference between the 3-turn run and the 21-turn one (#1114). +_BRIEF_ANSWER_PROMPT = """\ +You are a product owner being interviewed about a project you want built. +Everything you know about it is in this brief: + +{brief} + +Answer this interview question: + +{question} + +Rules: +- Answer only what was asked. If the question has two parts, answer both. +- Use the brief. Where it is silent, give a plausible answer consistent with it. +- Two to four sentences of plain prose. No preamble, no bullet points. +""" + + +class _AnswerSource: + """Where discovery answers come from: a TTY, canned answers, or a brief. + + Keeping this behind one object is what stops ``if non_interactive`` from + being threaded through every branch of the discovery loop. + + The three modes are not equivalent. A canned list cannot survive a rejected + answer — the questions are AI-generated, so the list desynchronises and never + resynchronises. ``--brief-file`` reads each question and answers *that* + question, which is the only non-interactive mode that reliably finishes + against a real model (#1114). + """ + + def __init__( + self, + answers: Optional[list[str]] = None, + brief: Optional[str] = None, + provider=None, + ): + self._answers = answers + self._brief = brief + self._provider = provider + self._index = 0 + + @property + def interactive(self) -> bool: + return self._answers is None and self._brief is None + + @property + def consumed(self) -> int: + return self._index + + @property + def can_desynchronise(self) -> bool: + """True for a fixed list, which cannot recover from a rejection.""" + return self._answers is not None + + def ask(self, question_text: str = "") -> str: + if self._brief is not None: + return self._ask_brief(question_text) + if self._answers is not None: + return self._ask_canned() + # Imported here, as prd_generate does — rich.prompt is only needed on + # the interactive path. + from rich.prompt import Prompt + + return Prompt.ask("\nYour answer", default="").strip() + + def _ask_canned(self) -> str: + if self._index >= len(self._answers): + raise AnswersExhausted( + f"ran out of answers after {self._index} of {len(self._answers)}" + ) + answer = self._answers[self._index].strip() + self._index += 1 + console.print(f"\n[dim]Your answer (from --answers-file):[/dim] {escape(answer)}") + return answer + + def _ask_brief(self, question_text: str) -> str: + from codeframe.adapters.llm import Purpose + + self._index += 1 + response = self._provider.complete( + messages=[{ + "role": "user", + "content": _BRIEF_ANSWER_PROMPT.format( + brief=self._brief, question=question_text + ), + }], + purpose=Purpose.GENERATION, + max_tokens=600, + ) + answer = (response.content or "").strip() + console.print(f"\n[dim]Your answer (from --brief-file):[/dim] {escape(answer)}") + return answer + + +def _load_brief_file(path: Path) -> str: + """Read a plain-text project brief for --brief-file.""" + try: + brief = path.read_text(encoding="utf-8").strip() + except FileNotFoundError: + raise typer.BadParameter(f"brief file not found: {path}") + except UnicodeDecodeError as e: + raise typer.BadParameter(f"{path} is not valid UTF-8 ({e}).") + if not brief: + raise typer.BadParameter(f"{path} is empty.") + return brief + + +def _handle_answer_attempts_exhausted(session, answers: "_AnswerSource", question: dict) -> bool: + """React to MAX_ANSWER_ATTEMPTS rejections on one question (#1114). + + Returns True to keep trying, False when the caller should stop. + + Non-interactive runs stop: a canned list that has desynchronised will not + resynchronise by being fed more of itself. Interactive runs are told what is + happening and offered the pause they would otherwise have to discover. + """ + console.print( + f"\n[yellow]That question has been answered {MAX_ANSWER_ATTEMPTS} times " + f"without being accepted.[/yellow]" + ) + if answers.can_desynchronise: + console.print( + "[red]Error:[/red] giving up in non-interactive mode. The validator " + "rejects partial answers, and AI-generated questions often have two " + "parts — a canned --answers-file cannot recover once it " + "desynchronises. Use --brief-file, which answers the question " + "actually asked, or run interactively." + ) + raise typer.Exit(1) + if not answers.interactive: + # --brief-file: there is no list to desynchronise, so repeated rejection + # means the brief does not cover what is being asked. + console.print( + "[red]Error:[/red] giving up in non-interactive mode. The stand-in " + "answered from the brief five times without being accepted, which " + "usually means the brief does not cover what this question asks:\n\n" + f" {escape(question.get('text', ''))}\n\n" + "Extend the brief to cover it, or run interactively." + ) + raise typer.Exit(1) + + console.print( + "[dim]Questions often have two parts; make sure the answer covers all of " + "them. You can also type /pause to save and come back.[/dim]" + ) + if not typer.confirm("Keep trying this question?", default=True): + blocker_id = session.pause_discovery("Stuck on a question") + console.print("\n[green]✓[/green] Session paused") + console.print( + f"To resume: [cyan]cf prd generate --resume {blocker_id[:8]}[/cyan]" + ) + return False + return True + + +def _load_answers_file(path: Path) -> list[str]: + """Read a JSON array of answer strings. + + JSON rather than one-per-line because real answers are prose and wrap. A + single format keeps the failure modes obvious. + """ + try: + raw = json.loads(path.read_text(encoding="utf-8")) + except FileNotFoundError: + raise typer.BadParameter(f"answers file not found: {path}") + except UnicodeDecodeError as e: + # A strict read raises ValueError, not OSError, so it would otherwise + # escape as a traceback (#1029). + raise typer.BadParameter(f"{path} is not valid UTF-8 ({e}).") + except json.JSONDecodeError as e: + raise typer.BadParameter( + f"{path} is not valid JSON ({e}). Expected an array of answer strings." + ) + if not isinstance(raw, list) or not all(isinstance(a, str) for a in raw): + raise typer.BadParameter( + f"{path} must contain a JSON array of strings, one answer per question." + ) + if not raw: + raise typer.BadParameter(f"{path} contains no answers.") + return raw + + PRD_NEXT_STEPS = ( " cf prd generate Start AI-guided requirements discovery\n" " cf prd add Import a PRD you already have" @@ -1462,12 +1663,35 @@ def prd_generate( "--template", "-t", help="PRD template to use (standard, lean, enterprise, user-story-map, technical-spec)", ), + answers_file: Optional[Path] = typer.Option( + None, + "--answers-file", + help=( + "Run without a TTY, taking answers from a JSON array of strings. " + "Answers are consumed in order; the run fails loudly if they run out. " + "A fixed list cannot recover if the validator rejects one — prefer " + "--brief-file against a real model." + ), + ), + brief_file: Optional[Path] = typer.Option( + None, + "--brief-file", + help=( + "Run without a TTY by answering each question from a project brief. " + "Unlike --answers-file this reads the question actually asked, so it " + "survives a rejected answer." + ), + ), ) -> None: """Generate a PRD through AI-driven Socratic discovery. An AI product manager conducts an intelligent conversation to understand your project requirements, then generates a structured PRD. + Two non-interactive modes exist for CI and demos: --brief-file (answers each + question from a project brief; survives a rejected answer) and --answers-file + (a fixed list; cannot resynchronise once one is rejected). + The AI: - Asks context-sensitive follow-up questions - Validates that answers are substantive @@ -1520,6 +1744,20 @@ def prd_generate( console.print(f" {t.id} - {escape(t.name)}") raise typer.Exit(1) + if answers_file and brief_file: + # Checked before any work: a flag conflict must not cost an API call. + console.print( + "[red]Error:[/red] --answers-file and --brief-file are alternatives; " + "pass one." + ) + raise typer.Exit(2) + + # Same rule for the file contents. start_discovery() generates the opening + # question, which is a paid call taking minutes (#902) — a missing or + # malformed input file must not get that far. + canned = _load_answers_file(answers_file) if answers_file else None + brief = _load_brief_file(brief_file) if brief_file else None + console.print(f"[dim]Using template: {escape(template_obj.name)}[/dim]") try: @@ -1572,6 +1810,27 @@ def prd_generate( console.print("[dim]The AI will ask questions to understand your project.[/dim]") console.print("[dim]Type /help for available commands[/dim]\n") + brief_provider = None + if brief is not None: + # The stand-in answerer uses the same resolved provider chain as + # everything else, so --llm-provider/config apply to it too. + from codeframe.core.llm_resolution import ( + create_provider, + resolve_llm_settings, + ) + + brief_provider = create_provider(resolve_llm_settings(workspace.repo_path)) + answers = _AnswerSource(canned, brief, brief_provider) + if canned is not None: + console.print( + f"[dim]Non-interactive: {len(canned)} answer(s) " + f"from {answers_file}[/dim]\n" + ) + elif brief is not None: + console.print( + f"[dim]Non-interactive: answering from {brief_file}[/dim]\n" + ) + # Discovery loop while not session.is_complete(): question = session.get_current_question() @@ -1591,13 +1850,18 @@ def prd_generate( console.print(Panel(question["text"], title="Question", border_style="cyan")) # Get answer + attempts = 0 while True: try: - answer = Prompt.ask( - "\nYour answer", - default="", - ) - answer = answer.strip() + attempts += 1 + if attempts > MAX_ANSWER_ATTEMPTS: + if not _handle_answer_attempts_exhausted( + session, answers, question + ): + return + attempts = 1 + + answer = answers.ask(question["text"]) if not answer: console.print("[yellow]Please enter an answer[/yellow]") @@ -1642,6 +1906,19 @@ def prd_generate( break # Otherwise let user try again + except AnswersExhausted as e: + # Loud and specific, rather than an EOF traceback out of + # Prompt.ask that says nothing about where it stopped (#1114). + console.print( + f"\n[red]Error:[/red] --answers-file {e}.\n" + f"Discovery was still on question " + f"{question.get('question_number', '?')} at " + f"{progress.get('percentage', 0)}% coverage.\n" + "The questions are AI-generated, so a fixed list can " + "desynchronise if an answer is rejected. Add more answers, " + "or run interactively to finish the session." + ) + raise typer.Exit(1) except ValidationError as e: console.print(f"[yellow]{e}[/yellow]") except DiscoveryError as e: diff --git a/tests/cli/test_prd_generate_non_interactive_1114.py b/tests/cli/test_prd_generate_non_interactive_1114.py new file mode 100644 index 00000000..dbaae803 --- /dev/null +++ b/tests/cli/test_prd_generate_non_interactive_1114.py @@ -0,0 +1,417 @@ +"""#1114 — `cf prd generate` could only be driven by a human at a TTY. + +The primary THINK entry point, the command the README leads with, had no way to +run without someone typing. So it could not be covered end to end, could not be +demoed reproducibly, and #614's harness had to build an LLM-backed stand-in user +to get through it. + +A fixed answer list is not a sufficient answer on its own: the questions are +AI-generated and the validator rejects partial ones, so a single rejection +desynchronises the list permanently — measured at 21 turns, 0 accepted answers, +coverage stuck at 0%. That is why the retry cap here is part of the same change: +without it, `--answers-file` reproduces exactly that hang with no TTY to +interrupt it. +""" + +import json +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest +from typer.testing import CliRunner + +from codeframe.cli.app import ( + MAX_ANSWER_ATTEMPTS, + AnswersExhausted, + _AnswerSource, + _load_answers_file, + _handle_answer_attempts_exhausted, + _load_brief_file, + app, +) +from codeframe.core.workspace import create_or_load_workspace + +pytestmark = pytest.mark.v2 + +runner = CliRunner() + + +@pytest.fixture +def workspace_dir(tmp_path: Path) -> Path: + create_or_load_workspace(tmp_path) + return tmp_path + + +def _answers_file(tmp_path: Path, answers) -> Path: + path = tmp_path / "answers.json" + path.write_text(json.dumps(answers)) + return path + + +class TestTheAnswerSource: + """The seam that keeps `if non_interactive` out of every loop branch.""" + + def test_a_file_source_is_not_interactive(self): + assert not _AnswerSource(["a"]).interactive + + def test_no_answers_means_interactive(self): + assert _AnswerSource(None).interactive + + def test_answers_are_consumed_in_order(self): + source = _AnswerSource(["first", "second"]) + assert source.ask() == "first" + assert source.ask() == "second" + assert source.consumed == 2 + + def test_running_out_raises_a_specific_error(self): + """AC: fail loudly and specifically, not EOF inside Prompt.ask.""" + source = _AnswerSource(["only one"]) + source.ask() + with pytest.raises(AnswersExhausted) as exc: + source.ask() + assert "1 of 1" in str(exc.value) + + +class TestTheAnswersFileFormat: + def test_a_json_array_of_strings_loads(self, tmp_path): + path = _answers_file(tmp_path, ["a", "b"]) + assert _load_answers_file(path) == ["a", "b"] + + @pytest.mark.parametrize( + "content", ["not json at all", '{"a": 1}', "[1, 2, 3]", "[]"] + ) + def test_a_malformed_file_is_rejected_with_a_usable_message(self, tmp_path, content): + path = tmp_path / "answers.json" + path.write_text(content) + import typer + + with pytest.raises(typer.BadParameter): + _load_answers_file(path) + + def test_a_missing_file_is_rejected(self, tmp_path): + import typer + + with pytest.raises(typer.BadParameter): + _load_answers_file(tmp_path / "nope.json") + + +class TestTheRetryCapExists: + """AC: a retry cap per question, so a stuck run says something.""" + + def test_the_cap_is_small_enough_to_notice(self): + assert 1 < MAX_ANSWER_ATTEMPTS <= 10 + + +class TestEndToEndWithoutATTY: + """AC: a test drives `cf prd generate` end to end without a TTY.""" + + @pytest.fixture + def provider(self): + """A provider that accepts every answer and completes after three.""" + mock = MagicMock() + answered = [0] + + def complete(messages, **kwargs): + content = messages[0]["content"] if messages else "" + response = MagicMock() + lowered = content.lower() + + if "opening question" in lowered: + response.content = "What problem are you trying to solve?" + elif "assess the current coverage" in lowered: + ready = answered[0] >= 3 + response.content = json.dumps({ + "scores": { + "problem": 90, "users": 90, "features": 90, + "constraints": 90, "tech_stack": 90, + }, + "average": 90 if ready else 10, + "ready_for_prd": ready, + "weakest_category": "tech_stack", + "reasoning": "ok", + }) + elif "validate" in lowered or "adequate" in lowered: + answered[0] += 1 + response.content = json.dumps( + {"accepted": True, "feedback": "Good", "follow_up": None} + ) + elif "next question" in lowered: + response.content = "And who are the users?" + else: + response.content = "# Todo API\n\n## Problem\n\nThings." + return response + + mock.complete.side_effect = complete + return mock + + @patch("codeframe.core.llm_resolution.create_provider") + def test_it_runs_with_no_input_stream( + self, create_provider, provider, workspace_dir, tmp_path + ): + create_provider.return_value = provider + path = _answers_file(tmp_path, [ + "Developers lose track of todos across scattered notes, and I wanted " + "a self-hosted API after paying for three SaaS trackers.", + "Self-hosting developers comfortable with REST and the command line.", + "FastAPI and SQLite, deployed on a small VPS.", + "Sub-50ms creates, and filtering by completion status.", + ]) + + result = runner.invoke( + app, + ["prd", "generate", "-w", str(workspace_dir), "--answers-file", str(path)], + # No `input=`: nothing is available to read from stdin. + env={"ANTHROPIC_API_KEY": "test-key"}, + ) + + assert result.exit_code == 0, result.output + assert "Non-interactive" in result.output + + @patch("codeframe.core.llm_resolution.create_provider") + def test_running_out_of_answers_says_which_question( + self, create_provider, provider, workspace_dir, tmp_path + ): + """AC: fails loudly and specifically, not an EOF traceback.""" + create_provider.return_value = provider + path = _answers_file(tmp_path, ["Only one answer, and discovery needs more."]) + + result = runner.invoke( + app, + ["prd", "generate", "-w", str(workspace_dir), "--answers-file", str(path)], + env={"ANTHROPIC_API_KEY": "test-key"}, + ) + + assert result.exit_code == 1 + assert "ran out of answers" in result.output + assert "coverage" in result.output.lower() + # The old failure was an EOF traceback out of Prompt.ask. + assert "EOF" not in result.output + assert "Traceback" not in result.output + + +class TestARejectingValidatorDoesNotHang: + """The #1114 scenario: 21 turns, 0 accepted, coverage pinned at 0%.""" + + @pytest.fixture + def always_rejects(self): + mock = MagicMock() + + def complete(messages, **kwargs): + content = messages[0]["content"] if messages else "" + response = MagicMock() + lowered = content.lower() + if "opening question" in lowered: + response.content = "What problem are you solving, and what inspired it?" + elif "assess the current coverage" in lowered: + response.content = json.dumps({ + "scores": { + "problem": 0, "users": 0, "features": 0, + "constraints": 0, "tech_stack": 0, + }, + "average": 0, + "ready_for_prd": False, + "weakest_category": "problem", + "reasoning": "nothing yet", + }) + elif "validate" in lowered or "adequate" in lowered: + response.content = json.dumps({ + "accepted": False, + "feedback": "Omits the second part of the question.", + "follow_up": None, + }) + else: + response.content = "text" + return response + + mock.complete.side_effect = complete + return mock + + @patch("codeframe.core.llm_resolution.create_provider") + def test_it_stops_instead_of_looping_forever( + self, create_provider, always_rejects, workspace_dir, tmp_path + ): + create_provider.return_value = always_rejects + # Far more answers than the cap: without the cap this consumes all of + # them and then hangs or EOFs, which is the reported behaviour. + path = _answers_file(tmp_path, [f"answer {i}" for i in range(30)]) + + result = runner.invoke( + app, + ["prd", "generate", "-w", str(workspace_dir), "--answers-file", str(path)], + env={"ANTHROPIC_API_KEY": "test-key"}, + ) + + assert result.exit_code == 1 + assert str(MAX_ANSWER_ATTEMPTS) in result.output + assert "non-interactive" in result.output.lower() + + +class TestTheBriefBackedMode: + """--brief-file answers the question actually asked, so it survives rejection. + + This is the mode that works against a real model. A fixed list cannot: the + questions are AI-generated, so one rejection desynchronises it permanently. + Verified end to end against a live model — a complete PRD in 67s with no TTY. + """ + + def test_a_brief_source_is_not_interactive(self): + assert not _AnswerSource(None, "a brief", MagicMock()).interactive + + def test_only_a_fixed_list_can_desynchronise(self): + assert _AnswerSource(["a"]).can_desynchronise + assert not _AnswerSource(None, "brief", MagicMock()).can_desynchronise + assert not _AnswerSource(None).can_desynchronise + + def test_the_question_is_passed_to_the_model(self): + provider = MagicMock() + provider.complete.return_value = MagicMock(content="An answer.") + source = _AnswerSource(None, "The brief text", provider) + + assert source.ask("What problem are you solving?") == "An answer." + + prompt = provider.complete.call_args.kwargs["messages"][0]["content"] + assert "What problem are you solving?" in prompt + assert "The brief text" in prompt + + def test_the_prompt_demands_every_part_of_a_question(self): + """Multi-part questions are what desynchronised the canned list.""" + provider = MagicMock() + provider.complete.return_value = MagicMock(content="x") + _AnswerSource(None, "brief", provider).ask("Two-part question?") + + prompt = provider.complete.call_args.kwargs["messages"][0]["content"].lower() + assert "two parts" in prompt or "both" in prompt + + def test_it_never_runs_out(self): + """Unlike a canned list, there is no fixed supply to exhaust.""" + provider = MagicMock() + provider.complete.return_value = MagicMock(content="An answer.") + source = _AnswerSource(None, "brief", provider) + for _ in range(50): + assert source.ask("q") == "An answer." + + def test_a_missing_brief_is_rejected(self, tmp_path): + import typer + + with pytest.raises(typer.BadParameter): + _load_brief_file(tmp_path / "nope.md") + + def test_an_empty_brief_is_rejected(self, tmp_path): + import typer + + path = tmp_path / "brief.md" + path.write_text(" \n") + with pytest.raises(typer.BadParameter): + _load_brief_file(path) + + def test_the_two_modes_are_mutually_exclusive(self, workspace_dir, tmp_path): + answers = _answers_file(tmp_path, ["a"]) + brief = tmp_path / "brief.md" + brief.write_text("A brief.") + + result = runner.invoke( + app, + [ + "prd", "generate", "-w", str(workspace_dir), + "--answers-file", str(answers), + "--brief-file", str(brief), + ], + env={"ANTHROPIC_API_KEY": "test-key"}, + ) + assert result.exit_code != 0 + assert "alternatives" in result.output or "one" in result.output.lower() + + +class TestNonUtf8InputIsReported: + """A strict read raises ValueError, which a FileNotFoundError-only handler + would let escape as a traceback (the #1029 rule).""" + + def test_a_non_utf8_answers_file(self, tmp_path): + import typer + + path = tmp_path / "answers.json" + path.write_bytes(b'["caf\xe9 latte"]') # latin-1 + with pytest.raises(typer.BadParameter) as exc: + _load_answers_file(path) + assert "UTF-8" in str(exc.value) + + def test_a_non_utf8_brief_file(self, tmp_path): + import typer + + path = tmp_path / "brief.md" + path.write_bytes(b"caf\xe9 latte") + with pytest.raises(typer.BadParameter) as exc: + _load_brief_file(path) + assert "UTF-8" in str(exc.value) + + +class TestReviewFindings: + """Two findings from PR review, both real.""" + + def test_a_bad_input_file_costs_no_llm_call(self, workspace_dir, tmp_path): + """start_discovery() generates the opening question — a paid, slow call. + + Validation of the input file was happening after it, so a typo in a path + cost a request and minutes of wall time before failing. + """ + with patch("codeframe.core.llm_resolution.create_provider") as create_provider: + result = runner.invoke( + app, + [ + "prd", "generate", "-w", str(workspace_dir), + "--answers-file", str(tmp_path / "does-not-exist.json"), + ], + env={"ANTHROPIC_API_KEY": "test-key"}, + ) + + assert result.exit_code != 0 + create_provider.assert_not_called() + + def test_a_bad_brief_file_costs_no_llm_call(self, workspace_dir, tmp_path): + with patch("codeframe.core.llm_resolution.create_provider") as create_provider: + result = runner.invoke( + app, + [ + "prd", "generate", "-w", str(workspace_dir), + "--brief-file", str(tmp_path / "nope.md"), + ], + env={"ANTHROPIC_API_KEY": "test-key"}, + ) + + assert result.exit_code != 0 + create_provider.assert_not_called() + + def test_brief_mode_gets_brief_advice_not_answers_advice(self, capsys): + """The retry-cap message must match the mode. + + --brief-file has no canned list, so telling the user their "answer list + desynchronised" and to "supply answers" is unactionable. Exercised on the + handler directly: driving it through the CLI would mean mocking the whole + discovery engine, which is not what this finding is about. + """ + import typer + + source = _AnswerSource(None, "A brief.", MagicMock()) + question = {"text": "What problem, and what inspired it?"} + + with pytest.raises(typer.Exit): + _handle_answer_attempts_exhausted(MagicMock(), source, question) + + out = capsys.readouterr().out + assert "brief" in out.lower() + assert "--answers-file" not in out + assert "canned" not in out.lower() + # It should quote the question the brief failed to cover. + assert "What problem" in out + + def test_answers_file_mode_still_gets_answers_advice(self, capsys): + import typer + + source = _AnswerSource(["a", "b"]) + + with pytest.raises(typer.Exit): + _handle_answer_attempts_exhausted(MagicMock(), source, {"text": "q"}) + + out = capsys.readouterr().out + assert "--answers-file" in out + assert "--brief-file" in out, "it should point at the mode that recovers"