fix(degree): bound DNF expansion so render_plan_graph can't hang#13
Merged
Conversation
Issue 4 from the MCP tester report: render_plan_graph returned a generic "Tool execution failed" for sample / calc-ready / plan_index>=2 selectors while shortest/longest worked. The picker is correct and panic-guarded, and all the tester's calls shared one (successful) cached analysis — so the failure was a client-side timeout on a *hang* during per-plan rendering, not a panic. Root cause: build_edges_from_courses calls parse_to_dnf on each in-plan course's prerequisite string, and parse_dnf_recursive expands an AND-of-ORs by cartesian product. A pathological scrape like (A|B) & (C|D) & ... (~30 groups) is 2^30 paths and effectively hangs. The offending course appears only in the electives a sample/calc-ready plan draws, which is why the extremes rendered fine. Fix: cap the DNF expansion at MAX_DNF_PATHS (1024) inside parse_dnf_recursive — bounding both the AND cartesian product and the OR accumulation. Every kept path is still complete (one course per AND group) and valid; we just stop enumerating further alternatives. This protects all three parse_to_dnf callers (render, plan_export, course_graph), not just the render path. No realistic prerequisite has anywhere near 1024 genuinely distinct satisfying paths, so existing behavior is unchanged. Tests: a 2^30-path AND-of-ORs and a 5000-way OR now return bounded and fast (would hang unbounded); a render of a plan whose course carries a pathological prerequisites_raw now completes promptly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second phase of the MCP tester feedback fixes (independent of #12 — touches different files).
Issue 4 —
render_plan_graph"Tool execution failed" on sample /plan_index≥2shortest/longestrendered fine; every other selector failed with a generic error. Investigation showed:pick_by_category/plan_index) is correct and unit-tested, andrender_plan_graphis panic-guarded (catch_unwind) — so a panic would surface as JSON, not "Tool execution failed".max_plans=None) → the analysis ran once and succeeded (shortest rendered) → the failure is in per-plan rendering, and the generic message is a client-side timeout on a hang.Root cause:
build_edges_from_coursescallsparse_to_dnfon each in-plan course's prerequisite string, andparse_dnf_recursiveexpands an AND-of-ORs by cartesian product. A pathological(A|B) & (C|D) & …(~30 groups) is 2³⁰ paths and effectively hangs. That course only appears in the electives a sample/calc-ready plan draws — which is exactly why the extremes worked.Fix
Cap the DNF expansion at
MAX_DNF_PATHS = 1024insideparse_dnf_recursive, bounding both the AND cartesian product and the OR accumulation. Every kept path is still complete (one course per AND group) and valid; we just stop enumerating further alternatives. This protects all threeparse_to_dnfcallers (render,plan_export,course_graph), not just the render path. No realistic prerequisite has near 1024 genuinely distinct satisfying paths, so existing behavior is unchanged (all prior parser tests still pass).The plan's optional defense-in-depth render wall-clock guard was not needed: the DNF cap removes the only superlinear step, and the rest of the render is O(nodes+edges), bounded by plan size.
Verification
render_plan_graphof a plan whose course carries a pathologicalprerequisites_rawcompletes in 0.09s.cargo fmt --check,cargo clippy --all-targets --all-features -D warnings,cargo doc -D warnings,cargo test --all-featuresall green.🤖 Generated with Claude Code