fix(prd): parse fenced stress-test JSON and bound the walk (#927) - #1040
Conversation
**The silent false pass.** extract_goals did a raw json.loads and returned [] on failure; the caller reads an empty goal list as "nothing 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 paid for the call*. Fenced JSON is routine on the OpenAI-compatible and local providers this command explicitly supports. Every sibling LLM-JSON consumer in this repo already strips fences — in two subtly different shapes, across four call sites. This one had none. That divergence is the bug, so the fix is a shared leaf module: core/llm_json.py, whose parse_json_response *raises* rather than returning a falsy default. A parser that degrades quietly is how a provider quirk becomes a passing grade. extract_goals now raises StressTestError on unparseable output, a non-list, or zero usable goals. An empty PRD is not a well-specified PRD. The SSE stream's existing blanket handler turns that into an error event, so both surfaces report it. classify_and_decompose keeps degrading a single unparseable node to a leaf — there, unlike goal extraction, an empty result is not indistinguishable from success. **Bounded recursion.** The walk followed a model-supplied children list with only a depth cap (API-settable to 10); breadth multiplies at every level and each node is one paid call. Adds MAX_CHILDREN_PER_NODE (12) and a per-run _Budget with MAX_LLM_CALLS (200), returning the partial tree with `partial: true` rather than raising — the ambiguities already found are real. **Cancellation.** The router broke out of the loop on disconnect, which only stopped between *top-level goals* while the recursion inside one kept issuing calls. The disconnect is now latched into a flag the budget polls before every call, so an abandoned stream stops at the next node. Three existing tests asserted the silent-empty behaviour and now assert the raise; a fourth pins the fenced-response case that triggered all of this.
|
Claude Code is working… I'll analyze this and get back to you. |
|
Claude finished @frankbria's task in 5m 48s —— View job GLM precision review — complete
3 defects found (all minor) — gaps between the PR's stated ACs and the behavior of the surfaces it claims to cover:
All three are strict improvements over pre-PR; none crash the happy path or lose data. Details and failure scenarios are in the 3 inline threads + the summary comment. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
GLM precision review — 3 defects (all minor)Scoped to concrete defects only (logic errors, unhandled error paths, billing/bounding gaps). Style, architecture, naming, and test coverage are covered by the general review.
All three are strict improvements over the pre-PR state and none lose data or crash the happy path; they are gaps between the PR's stated ACs and the behavior of the surfaces it claims to cover. Details + concrete failure scenarios are in the inline threads. Not approving or requesting changes. |
All three were places where a stated acceptance criterion covered a surface the code did not actually reach. - CLI: `extract_goals` raises StressTestError, but `prd stress-test` never caught it, so AC2's "both surfaces report it" produced a Typer traceback on the CLI while the web stream got a clean error event. Now a red line + exit 1, matching every other failure in that command. - Sync path: `stress_test_prd` let each goal default to its own `_Budget()`, bounding the walk at `goals x MAX_LLM_CALLS` rather than the documented per-run total AC3 asks for. One budget now spans the run, and `StressTestResult.partial` reports a truncated walk the way the stream already did — the CLI warns instead of presenting a partial tree as complete. - SSE route: a whole top-level goal decomposes inside one `asyncio.to_thread`, so a latch updated only between yielded events stayed False for that entire billable walk. A concurrent poller now sets it while the worker thread runs, so the recursion stops at its next node rather than its next goal (AC4). The per-event check is kept alongside it — a stream faster than the poll interval would otherwise run to completion for a client already gone. Tests: tests/core/test_stress_test_review_1040.py, one per finding. Verified they fail (4/4) with the fixes reverted.
|
Claude finished @frankbria's task in 3m 50s —— View job GLM precision review — re-run on synchronizeReviewed the current PR state (
Scoped to concrete defects only (logic errors, unhandled error paths, billing/bounding gaps, races). Not repeating the 3 already-resolved findings. ✅ GLM review: no defects found.What I verified on each fix (reading the real code, not the diff alone):
Notes (not defects)
The |
Review triage — GLM precision review (3 findings)All three verified against the code before acting. All three are real, and all three are fixed in
One thing worth flaggingFix 3 initially broke an existing test — Verification
Note on the red
|
Phase 11 — Demo verification (#927)Live runs against the branch. The only thing stubbed is the LLM provider, which reproduces the real provider behaviours that caused the bug (markdown-fenced JSON, prose around the block, an unbounded AC1 — both parse sites share a fence-stripping helperNot AC2 — zero goals raises, never "PRD is well-specified"All three raise. None reaches the clean-bill-of-health report. AC3 — total call budget + per-node children capRunaway provider: 4 goals, every node claims 50 children, And the flag means something — a walk that fits reports AC4 — a disconnected SSE client stops decomposition workThe real route generator, provider calls sleeping 0.2s each like real ones, client dropping at t=1.0s. Pre-fix route vs. this branch, same script:
Pre-fix the abandoned stream ran to completion and billed the full walk. On this branch it stops 1 second in, mid-goal — Acceptance criteria → outcome evidence
Mutation check (phase-07a)
While validating fix 3, the change initially broke Suite
|
|
Claude finished @frankbria's task in 1m 21s —— View job GLM precision review — synchronize (merge of
|
| Surface | Verdict | Detail |
|---|---|---|
Per-run budget (sync) prd_stress_test.py:507 |
✓ | One _Budget() created before the goal loop, threaded into every recursive_decompose call (:521), break on stopped_early (:510), partial=budget.stopped_early reported (:545). Ceiling is MAX_LLM_CALLS, not goals × MAX_LLM_CALLS. |
Per-run budget (stream) prd_stress_test.py:583 |
✓ | _Budget(is_cancelled=is_cancelled) shared across the whole run; budget.take() checked at the top of every node (:316). |
Cancellation latch prd_v2.py:293–330 |
✓ | _watch_disconnect (1s poll) and the per-event check (:320) both flip the same disconnected the budget's is_cancelled=lambda: disconnected reads. budget.take() runs at the top of every recursive_decompose node → a disconnect stops the walk at the next node. watcher.cancel() runs in finally (:328). The per-event check is retained, so a stream faster than the poll still aborts. |
extract_goals raises prd_stress_test.py:196–216 |
✓ | LLMJsonError → StressTestError; non-list → StressTestError; empty list → StressTestError. Never returns []. |
The three previously-resolved findings remain fixed and the merge introduced no new concrete defects. Not approving or requesting changes.
…927) CI caught two things the local run raced past. - `_watch_disconnect` polled immediately on start, duplicating the check the event loop already does per event. For a caller whose disconnect signal is stateful rather than idempotent that duplicate is observable: it advanced the signal a step early and aborted the stream before its first frame, breaking `test_aborts_when_client_disconnects`. The watcher now sleeps before its first poll, so the per-event check stays the only one on the fast path and the watcher covers exactly the gap it was added for — the seconds spent inside one goal's `asyncio.to_thread` walk. - `test_stress_test_ollama_provider_skips_anthropic_key` stubbed the result as a hand-rolled `SimpleNamespace`, which went stale when `StressTestResult` gained `partial`. Replaced with the real dataclass: that test is about which provider gets resolved, so it should not also encode a guess at the result's shape. Verified stable across 3 consecutive runs of the affected suites.
|
Claude finished @frankbria's task in 3m 5s —— View job GLM precision review — synchronize (
|
| Concern | Verdict | Detail |
|---|---|---|
| Detection gap from the delayed first poll | ✓ none | At t=0 the client has just connected, so the old immediate poll was False in production — moving it to t=1 s loses nothing real. The per-event check (prd_v2.py:327–329) still runs on every yielded frame, so the fast path is unchanged. The watcher's sole job remains covering the seconds spent inside one goal's asyncio.to_thread walk, where the async for is suspended and the per-event check can't fire — it still wakes every 1 s alongside the worker thread and flips the same disconnected the budget's is_cancelled=lambda: disconnected reads. |
| New loop ordering / control flow | ✓ correct | while not disconnected: → sleep → if disconnected: return → if is_disconnected(): set + return. The post-sleep disconnected guard is sound: if the per-event check broke the loop or finally ran during the 1 s sleep, the watcher exits without a spurious poll/log. There is no path that exits the watcher without setting disconnected while the stream is still active and the client gone. |
| AC4 (abandoned stream stops at next node) | ✓ still holds | budget.take() runs at the top of every recursive_decompose node (prd_stress_test.py:316) and short-circuits on is_cancelled(). The watcher setting disconnected=True at the next 1 s boundary is therefore observed at the next node — not the next goal. The ceiling on orphaned within-goal calls after a disconnect is now ~the remainder of one classification call, not 200. |
| Cancellation / exception paths | ✓ no new issue | watcher.cancel() in finally (prd_v2.py:337) interrupts asyncio.sleep; CancelledError is special-cased by asyncio, so no "exception never retrieved". asyncio is imported (prd_v2.py:17). The fire-and-forget is_disconnected() raising would only kill the watcher task (logged), not the stream — pre-existing, unchanged, and not introduced here. |
Test stub (test_cli_validators.py) |
✓ correct | StressTestResult.partial defaults to False (prd_stress_test.py:70), so the new explicit construction (prd_title/tree/ambiguities/tech_spec_markdown/ambiguity_report) is valid and no longer goes stale when a field is added. The remaining SimpleNamespace uses in that file (lines 185–189, 248–256) stub batch/provider shapes in unrelated tests, not StressTestResult — correctly left alone. |
Why the immediate poll had to go (confirms the commit's rationale). TestStressTestDisconnect::test_aborts_when_client_disconnects drives a stateful FakeRequest.is_disconnected() (test_prd_stress_test_router.py:201–209: return self.calls > 1 — False on the first call, True thereafter). With the old immediate poll, the watcher's t=0 check and the per-event check both consumed that counter, so the exact call at which the abort tripped depended on whether the watcher ran before or after the first async for iteration — a genuine event-loop scheduling race (CI vs local), not a stale test. Sleeping first removes the redundant poll entirely; the per-event check is the only signal source on the fast path, the watcher covers only the within-goal gap it was added for, and the test is deterministic (per-event call #1 on goals_extracted → False → yield; call #2 on the next event → True → break before yielding). The per-event check is retained, so a stream faster than the 1 s poll still aborts — the property the prior review flagged as load-bearing.
The three previously-resolved findings remain fixed and this synchronize introduces no new concrete defects. Not approving or requesting changes.
|
Claude finished @frankbria's task in 2m 13s —— View job GLM precision review — synchronize (
|
| Surface | Verdict | Detail |
|---|---|---|
Per-run budget (sync) prd_stress_test.py:507 |
✓ | One _Budget() created before the goal loop (:507), threaded into every recursive_decompose (:521), break on stopped_early (:510), partial=budget.stopped_early reported (:545). spent starts at 0 and take() blocks when spent >= 200, so the ceiling is exactly MAX_LLM_CALLS (200 classify calls + 1 extraction), not goals × 200. |
Per-run budget (stream) prd_stress_test.py:583 |
✓ | _Budget(is_cancelled=is_cancelled) shared across the whole run; budget.take() checked at the top of every node (:316). Same accounting. |
Budget call accounting prd_stress_test.py:160–169, 316 |
✓ | take() is invoked exactly once per non-max-depth node, immediately before the single classify_and_decompose call. depth >= max_depth short-circuits the or so leaf-at-depth consumes nothing. No double-decrement, no off-by-one: call 200 sets spent=200; call 201 hits spent >= max_calls → exhausted=True, no call. |
extract_goals raise paths prd_stress_test.py:196–216 |
✓ | LLMJsonError → StressTestError; non-list → StressTestError; empty-after-strip → StressTestError. Never returns []. The stream's blanket except (:620) turns it into an error event; the CLI wraps both call sites (cli/app.py:1755, :1803). |
Cancellation latch prd_v2.py:293–337 |
✓ | _watch_disconnect (sleep-first, 1s poll) and the per-event check (:327–329) both flip the same disconnected the budget's is_cancelled=lambda: disconnected reads. budget.take() runs at the top of every recursive_decompose node → a disconnect stops the walk at the next node, not the next goal. disconnected is a bool written on the loop thread and read inside the worker thread via the closure; CPython bool read/write is atomic, so no torn-read race. watcher.cancel() runs in finally on every exit path. The per-event check is retained, so a stream faster than the 1s poll still aborts. |
The three previously-resolved findings remain fixed and this synchronize introduces no new concrete defects. Not approving or requesting changes.
|
Claude finished @frankbria's task in 3m 35s —— View job GLM precision review — synchronize (
|
| Surface | Verdict | Detail |
|---|---|---|
Budget call accounting prd_stress_test.py:160–169, 316 |
✓ | take() is invoked exactly once per non-max-depth node, immediately before the single classify_and_decompose call. depth >= max_depth short-circuits the or (:316) so a leaf at depth consumes nothing. Call 200 sets spent=200; call 201 hits spent >= max_calls → exhausted=True, no call. No double-decrement, no off-by-one. Total ceiling is 1 (extraction) + 200 (classification) = 201. |
Per-run budget (sync) prd_stress_test.py:507 |
✓ | One _Budget() created before the goal loop (:507), threaded into every recursive_decompose (:521) and into the recursive call (:348). break on stopped_early (:510); partial=budget.stopped_early reported (:545). Ceiling is MAX_LLM_CALLS, not goals × MAX_LLM_CALLS. |
Per-run budget (stream) prd_stress_test.py:583 |
✓ | _Budget(is_cancelled=is_cancelled) shared across the whole run; budget.take() checked at the top of every node (:316). Same accounting; is_cancelled reads the router's disconnected latch. |
extract_goals raise paths prd_stress_test.py:196–216 |
✓ | LLMJsonError → StressTestError; non-list → StressTestError; empty-after-strip → StressTestError. Never returns []. The stream's blanket except (:620) turns it into an error event; the CLI wraps both call sites (cli/app.py:1755, :1803). |
Cancellation latch prd_v2.py:293–337 |
✓ | _watch_disconnect (sleep-first, 1s poll) and the per-event check (:327–329) both flip the same disconnected the budget's is_cancelled=lambda: disconnected reads. budget.take() runs at the top of every recursive_decompose node → a disconnect stops the walk at the next node, not the next goal. disconnected is a monotonic bool written on the loop thread and read inside the worker thread via the closure; CPython bool read/write is atomic under the GIL, so no torn-read. watcher.cancel() runs in finally on every exit path (:337); the watcher is only created when request is not None, so the request.is_disconnected() reference inside it is always bound. |
StressTestResult.partial contract prd_stress_test.py:70 |
✓ | Added at the end with default False; both construction sites (prd_stress_test.py:539 keyword, test_cli_validators.py:330 keyword-omitting-partial) are valid. The streaming complete event gaining a partial key is a plain dict, not a Pydantic model, so no schema break. |
The three previously-resolved findings remain fixed and this synchronize introduces no new concrete defects. Not approving or requesting changes.
1 participant
Closes #927.
The silent false pass
extract_goalsdid a rawjson.loadsand returned[]on failure. The caller reads an empty goal list as "nothing to analyse" and reports "No ambiguities found — PRD is well-specified".So a provider that wrapped its JSON in a markdown fence produced a clean bill of health after the user paid for the call. Fenced JSON is routine on the OpenAI-compatible and local providers this command explicitly supports.
The divergence was the bug
Every sibling LLM-JSON consumer in this repo already strips fences — in two subtly different shapes, across four call sites (
executor.py×2,prd_discovery.py×2). This one had none.So the fix is a shared leaf module,
core/llm_json.py, whoseparse_json_responseraises rather than returning a falsy default. That is the whole lesson of the bug: a parser that degrades quietly turns a provider quirk into a passing grade.It also tolerates prose around the block ("Sure! Here you go:"), which local models add routinely, and a fence with no language tag.
Failing loudly (AC2)
extract_goalsnow raisesStressTestErroron unparseable output, a non-list, or zero usable goals. An empty PRD is not a well-specified PRD. The SSE stream's existing blanket handler turns that into anerrorevent, so both surfaces report it.classify_and_decomposekeeps degrading a single unparseable node to a leaf — there, unlike goal extraction, an empty result is not indistinguishable from success.Bounded recursion (AC3)
The walk followed a model-supplied
childrenlist with only a depth cap, API-settable to 10. Breadth multiplies at every level and each node is one paid call.MAX_CHILDREN_PER_NODE = 12— truncates, and logs what it droppedMAX_LLM_CALLS = 200— a per-run budgetpartial: truerather than raising; the ambiguities already found are real, but their absence elsewhere is not evidenceCancellation (AC4)
The router broke out of its loop on disconnect, which only stopped between top-level goals — the recursion inside one goal kept issuing billable calls. The disconnect is now latched into a flag the budget polls before every call, so an abandoned stream stops at the next node.
Testing
tests/core/test_stress_test_json_927.py— 15 tests across all four ACs.Three existing tests asserted the silent-empty behaviour and now assert the raise; a fourth pins the fenced-response case that triggered all of this.
PRD suites: 251 passed. Full gate: 4945 passed,
ruffclean.Scope note
The four existing hand-rolled fence strippers in
executor.pyandprd_discovery.pyare left in place. Converting them is mechanical but touches two unrelated subsystems; the shared module now exists for them to adopt, and this PR stays scoped to the parse sites the issue names.