fix: make six CLI/orchestrator/llm errors actionable#203
fix: make six CLI/orchestrator/llm errors actionable#203MohammedAnasNathani wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Improves the actionability of six user-facing CLI/orchestrator/LLM error paths (per #198) by appending concrete next-step guidance, and adds regression tests to lock in the new messaging and Rich-escape behavior.
Changes:
- Make config validation errors name the offending key/value and suggest nearest valid config keys.
- Ensure pipeline stop / unverified completion messages include explicit
readme2demo resume …guidance. - Augment
claude -pfailure errors with a consistent “check login / use--llm-backend api” remediation hint, plus tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/readme2demo/cli.py |
Adds nearest-match suggestions for unknown config keys, includes key/value context for type errors, and prints resume guidance for PipelineError + UNVERIFIED completion. |
src/readme2demo/orchestrator.py |
Makes the budget-exceeded PipelineError message name --budget-usd and resumability. |
src/readme2demo/llm.py |
Appends actionable next-step guidance to claude -p timeout/nonzero/non-JSON/envelope-error failures. |
tests/test_cli.py |
Adds regression tests for config error messages (key/value inclusion, nearest-match suggestion, Rich-escape). |
tests/test_orchestrator.py |
Adds a regression test intended to cover the budget-exceeded message. |
tests/test_llm_backend.py |
Adds a regression test for the claude -p timeout hint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_budget_exceeded_message_mentions_flag(): | ||
| """Regression: budget stop should name --budget-usd and resumability.""" | ||
| # Construct the same message shape the orchestrator raises. | ||
| cost, budget = 6.0, 5.0 | ||
| msg = ( | ||
| f"Agent cost ${cost:.2f} exceeded budget ${budget:.2f}. " | ||
| "Raise --budget-usd (or budget_usd in config), then resume this run." | ||
| ) | ||
| assert "--budget-usd" in msg | ||
| assert "resume" in msg.lower() |
| try: | ||
| llm_mod._complete_cli("sys", "user", None) | ||
| assert False, "expected LLMError" |
| raise LLMError(f"claude -p timed out after {_CLI_TIMEOUT_S}s. Check login with `claude -p hello`, or switch to `--llm-backend api` with ANTHROPIC_API_KEY set.") from None | ||
| if proc.returncode != 0: | ||
| raise LLMError( | ||
| f"claude -p failed ({proc.returncode}): {(proc.stderr or proc.stdout)[:500]}" | ||
| f"claude -p failed ({proc.returncode}): {(proc.stderr or proc.stdout)[:500]}." | ||
| f" Check login with `claude -p hello`, or switch to `--llm-backend api` with ANTHROPIC_API_KEY set." |
|
Addressed the Copilot review notes in 4c9787e:
Local checks: targeted tests + |
alphacrack
left a comment
alphacrack
left a comment
There was a problem hiding this comment.
Thanks @MohammedAnasNathani — this is a genuinely strong first PR: all six sites from #198, nothing beyond them, no grounding-path files touched, no exit code or control flow changed, and CI green. I reviewed it on three axes (markup safety, conformance, tests) and ran everything in a scratch worktree at your head merged with current main: 399 passed, coverage 87.4%, ruff clean, mypy clean, and all five of your new tests are provably hermetic (I ran the suite under a guard that raises on subprocess.Popen/socket.connect — zero denials).
Two things you got right that most people get wrong, worth calling out first:
escape(repr(bad_input))at cli.py:159 is in the correct order. That's the exact trap CLAUDE.md failure class 15 exists for, and the comment documenting it lives 290 lines away. I measured all three orderings against"[bold]sixty[/bold]"— yours is the only one that survives.- Every command you suggest is real. I checked each against the actual option definitions.
--from-stage distillin particular is not just valid but optimal:Manifest.reset_fromclearsverifiedfor any stage at or before verify, so distill re-runs the cheap stages without re-burning agent cost.
Four things to fix before merge.
1. Blocking — the budget hint gives advice that errors out. orchestrator.py:213 says "Raise --budget-usd … then resume this run", but --budget-usd exists only on run:
$ readme2demo resume /tmp/runs/x --budget-usd 10
Error: No such option: --budget-usd
The parenthetical half is correct — resume does pick up budget_usd from readme2demo.toml — so it's an ordering problem. Lead with the remedy that composes with resume:
f"Agent cost ${cost:.2f} exceeded budget ${self.cfg.budget_usd:.2f}. "
"Raise budget_usd in readme2demo.toml (or --budget-usd on a fresh run), "
"then resume this run — the agent work is already saved."There's a nice irony here that makes it worth getting right: an actionability PR is the one place unactionable advice really stings.
2. Blocking — three of the six changes aren't pinned by tests. I verified this by surgically reverting hunks and re-running:
- Revert items 3 and 4 (both
_drivehints) → 399/399 still green. Nothing references"Fix the cause"or"from-stage distill"intests/. - Strip the hint from three of the four
claude -praises → 399/399 still green (only the timeout branch is covered). - The new
test_unknown_config_key_suggestion_is_escaped_for_rich_markupis a byte-for-byte duplicate of the existing test at tests/test_cli.py:316 — it passes with your entire PR reverted, so it pins nothing.
Also: flipping your (correct) escape order to repr(escape(...)) leaves all 66 test_cli.py tests green — the one site the house rule is actually about is unguarded.
Suggested fixes: repoint the markup test at a bracketed config value (that's the new interpolation), widen the two existing claude -p tests to match=r"reported an error.*--llm-backend api", and add two CliRunner tests monkeypatching Orchestrator.run to raise PipelineError and to return an unverified manifest.
3. Nit — vhs_image is offered as a valid key. valid_keys = sorted(Config.model_fields) includes the deprecated no-op shim, and it's reachable as a suggestion ('vhs_imag' → 'vhs_image'), steering a confused user toward a key that does nothing. sorted(k for k, f in Config.model_fields.items() if not f.exclude) gives the 19 live keys.
4. Nits, take or leave: three new llm.py lines exceed the project's line-length = 100 (CI misses it — E501 isn't selected); the claude -p test uses try/except + assert False where the ~20 sibling tests use pytest.raises(match=...), and its or lets half the hint be deleted undetected; repr(bad_input) is unbounded, so a list-valued key floods five wrapped lines ([:120] inside the escape() call fixes it); and captured stderr ends in a newline, so the appended . lands alone on the next line — .strip() it.
Nothing here is architectural — it's one wording fix and some test tightening. The engineering judgment on display, especially driving the real Orchestrator.run() for the budget test instead of asserting a hand-built string, is well above the bar for a first contribution.
Append concrete next steps to config validation, pipeline stop, UNVERIFIED completion, budget exceeded, and claude -p failure paths. Keep diagnosis text, escape only at console.print sites, and add regression tests for the new actionable substrings. Closes alphacrack#198
Factor the claude -p next-step hint into a module constant, pass a str model arg in the timeout regression test, and exercise Orchestrator.run() for the budget-exceeded message instead of asserting a hand-built string.
- Budget stop now leads with budget_usd in readme2demo.toml (works with resume); --budget-usd only mentioned for a fresh run - Config key suggestions skip excluded/deprecated fields (vhs_image) - Truncate type-error value repr; strip claude -p stderr before append - Pin PipelineError resume hint and UNVERIFIED --from-stage distill via CliRunner; pin bracketed config *values* for Rich escape order - Widen claude -p tests to require full next-step hint; use pytest.raises
cdff094 to
12746b8
Compare
|
Thanks for the careful review — this was exactly the level of feedback that improves the PR. Addressed in the latest push: 1. Budget hint (blocking)
2. Tests that actually pin the six sites
3.
|
Summary
Implements #198 — error-message actionability pass over the six bounded sites:
Config.model_fieldsPipelineErrorpath printsreadme2demo resume <run-dir>resume … --from-stage distill--budget-usdand resumeclaude -pfailures share login /--llm-backend apihintAlso adds regression tests for the actionable substrings / Rich escape.
Closes #198
Test plan
pytest tests/test_cli.py tests/test_orchestrator.py tests/test_llm_backend.py