feat(cli): non-interactive prd generate, plus a retry cap so it cannot hang (#1114) - #1135
Conversation
…t hang (#1114) `cf prd generate` — the primary THINK entry point and the command the README leads with — could only be driven by a human at a TTY. So it could not be covered end to end, could not be demoed reproducibly, and #614's harness had to build an out-of-process LLM stand-in user (responder.py, 206 lines of pty and subprocess plumbing) to get through it. Two non-interactive modes, because they are not equivalent: --brief-file <path> Answers each question from a project brief, reading the question actually asked. This is the mode that works against a real model. --answers-file <path> A JSON array of answers consumed in order. Deterministic and ideal for CI, but a fixed list CANNOT resynchronise once the validator rejects one. That distinction is the whole issue. The report measured a canned list at 21 turns / 0 accepted / 0% coverage because the questions are AI-generated and often multi-part. I reproduced exactly that against a live model with --answers-file: five substantive answers, one rejected as off-topic, and the run ended at question 2 / 67% coverage. --brief-file on the same brief produced a complete PRD in 67 seconds with no TTY, exit 0. Also: - A retry cap (MAX_ANSWER_ATTEMPTS = 5) per question. Without it --answers-file reproduces the original hang with no TTY to interrupt it. Interactive runs are told what is happening and offered the pause they would otherwise have to discover; non-interactive runs stop and explain why a canned list cannot recover. - Running out of answers raises AnswersExhausted and reports which question and what coverage it stopped at, instead of an EOF traceback out of Prompt.ask. - --answers-file and --brief-file are rejected together, before any API call. 22 tests, including a full discovery run with no `input=` stream at all, and the always-rejecting validator scenario asserting it terminates rather than looping.
|
Claude finished @frankbria's task in 4m 53s —— View job Precision bug-hunting review — doneReviewed
2 findings, both minor:
Both posted as inline review comments with a failure scenario (and a suggestion block on the first). No critical/major defects, no security or data-loss issues, no breaking-contract changes found. Nothing else rose to a confirmable defect — e.g. an uncaught exception from the brief provider's |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…1114) CI caught two repo-wide rules I broke, both real: - read_text() without encoding= decodes with the locale encoding, so the same answers/brief file decodes differently across machines (#1029). - UnicodeDecodeError subclasses ValueError, not OSError, so it escaped the FileNotFoundError-only handlers as a traceback. A latin-1 answers file now reports "not valid UTF-8" like every other bad-input case. Two tests cover the non-UTF-8 paths. My mistake was running only tests/cli/ before pushing. Full suite now: 6347 passed, 49 skipped.
|
Claude finished @frankbria's task in 4m 33s —— View job Precision bug-hunting review — re-run on synchronizeReviewed the new commit
✅ No new defects found in the synchronize commit. The push is a correct, narrowly-scoped fix to the two file loaders:
I checked the candidates that looked alive on the diff and discarded each against real context:
Still-open (not re-posted)These two findings were posted inline on the prior commit and are not addressed by this push — the new commit didn't touch either code path. Flagging status only, not repeating the inline comments:
Nothing else rose to a confirmable defect. |
|
Reviewed the current diff (
Beyond those, I found one more significant issue that I don't think was covered yet: The In the discovery loop (app.py:1837-1849): attempts = 0
while True:
try:
attempts += 1
if attempts > MAX_ANSWER_ATTEMPTS:
...
answer = answers.ask(question["text"])
...
result = session.submit_answer(answer)
if result["accepted"]:
break
else:
console.print(f"[yellow]{result['feedback']}[/yellow]")
if result.get("follow_up"):
break # <-- exits the inner loop without ever re-checking attempts
# otherwise let user try again
The validator's own prompt (
The existing test for this ( Suggest tracking attempts per logical question rather than per outer-loop pass — e.g. don't reset Everything else looks solid: the |
…ore any call (#1114) Both from PR review, both real. 1. The retry-cap handler gave --brief-file runs an --answers-file message: it told them a "canned answer list cannot recover" and to "supply answers that address every part of the question". Brief mode has no list and takes a brief, so that advice is unactionable — and it contradicts this PR's own claim that --brief-file survives a rejection. Worse, can_desynchronise was added precisely to tell the two modes apart and then never used. It is now the branch: --answers-file is pointed at --brief-file, and --brief-file is told the brief probably does not cover the question, quoting it. 2. The input files were loaded after session.start_discovery(), which generates the opening question — a paid call taking minutes (#902). So a typo in a path cost a request before failing. This contradicted the comment three lines above the conflict check ("a flag conflict must not cost an API call"), which I had applied to the flags and not their contents. Both loads moved up; two tests assert create_provider is never called on a bad path. The brief-mode message test exercises the handler directly. Driving it through the CLI hung inside the discovery engine's own retry loop, which would have meant mocking the whole engine to assert a message string. Full suite: 6351 passed, 49 skipped.
|
Claude finished @frankbria's task in 3m 12s —— View job Precision bug-hunting review — re-run on synchronizeReviewed commit
✅ No new defects in
Still-open — the follow-up retry-cap gap (not re-posted, status only)This was raised in the prior review round (the
Suggested direction (from the prior comment): track attempts per logical question — don't reset when |
ReviewConfirmed the two issues flagged in the earlier automated reviews (mode-mismatch retry advice, and file validation happening after the paid
Nice fixes, and the encoding pin ( One finding still open: the retry cap doesn't fire on the failure mode this PR names as the root cause
if result.get("follow_up"):
console.print("\n[cyan]Let me ask differently:[/cyan]")
break # -> outer loop, attempts resets to 0
So as long as the validator keeps returning a follow-up instead of a flat rejection,
Suggest tracking attempts per logical question rather than per outer-loop pass — e.g. don't reset This finding was raised in a previous review comment on this PR (posted before the third commit) and, based on reading the current code, is still present in the latest commit — the third commit's fixes were scoped to the other two issues and didn't touch this loop. Everything else
No security concerns — this only reads local files supplied via CLI flags and calls the already-trusted LLM provider chain. |
1 participant
Closes #1114.
Two modes, because they are not equivalent
--brief-file <path>--answers-file <path>That distinction is the whole issue, and it is why shipping only
--answers-filewould not have closed it.
I reproduced the failure the issue describes
The report measured a canned list at 21 turns, 0 accepted, coverage stuck at
0%. I hit exactly that against a live model with
--answers-file— fivesubstantive, on-topic answers, one rejected as addressing a different question,
and the run ended stranded:
--brief-file, same brief, same model:A complete PRD without a TTY — AC #1 actually satisfied, not satisfied on
paper.
The retry cap is part of the same change, not a nice-to-have
Without it,
--answers-filereproduces the original hang with no TTY tointerrupt it — strictly worse than the interactive version.
MAX_ANSWER_ATTEMPTS = 5:and offers the
/pausea stuck user would otherwise have to discoverLoud exhaustion
Running out now raises
AnswersExhaustedand reports the question number andcoverage it stopped at. Before it was EOF inside
Prompt.ask— an opaquetraceback naming neither.
Acceptance criteria
--brief-file, verified end to end against a live modelcf prd generateend to end without a TTY (noinput=)scripts/quickstart-cleanroom/can dropresponder.py— see below22 tests, including the always-rejecting-validator scenario asserting it
terminates instead of looping.
ruffclean;tests/cli/: 574 passed.Judgment calls
responder.pyin this PR. The AC says the harness candrop it, and
--brief-fileis the supported replacement — it is the same ideain-process, minus 206 lines of pty/subprocess plumbing. But swapping the Launch: validate the 15-minute quick start from a clean machine + recorded demo #614
harness over changes what that walkthrough measures (a stand-in user driving
the real CLI vs. the CLI answering itself), and Launch: validate the 15-minute quick start from a clean machine + recorded demo #614 is someone's evidence
artifact. That swap deserves its own change with the walkthrough re-run, rather
than riding along here.
--answers-file, not one-per-line: real answers are proseand wrap. One format keeps the failure modes obvious.
--brief-fileuses the standard provider resolution chain, so--llm-provider/ config apply to the stand-in answerer too.Known limitations
--answers-fileagainst a real model is unreliable by nature, not bydefect. It is documented as such in
--helpand points at--brief-file. Itremains the right choice for CI, where the provider is deterministic.
--brief-filecosts one extra LLM call per question. That is inherent toanswering the question actually asked.