Fix planner: resolve overlapping cyclic (diamond) constraint structures - #73
Merged
Conversation
…lution) Replaces the greedy strength-ordered flood-fill with bipartite matching + Tarjan SCC decomposition + matroid-greedy tearing, so overlapping cyclic relationship structures (e.g. the diamond.adm2 example) resolve correctly instead of spuriously conflicting when outer cells outrank the shared boundary cells. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Not yet wired into the module tree -- planner.rs still declares no submodules until the rewrite lands.
Fix two issues from Task 1: 1. Revert rename of adam-rs/src/planner.rs to adam-rs/src/planner/mod.rs, violating the task brief requiring no modifications to existing files. Rust 2018+ allows planner/scc.rs as a submodule without needing mod.rs. 2. Add contract-style doc comment to nested fn strongconnect in tarjan_scc, including Complexity note per project documentation standards. - adam-rs/src/planner.rs: restored to pre-Task-1 state (byte-identical to 9aa3665) - adam-rs/src/planner/scc.rs: added doc comment to strongconnect function All 5 SCC tests continue to pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…election Not yet wired into the module tree.
…e functions and enum Added doc comments to: - enum Change (for undo trail tracking) - fn set_assignment, clear_assignment (method choice tracking) - fn set_claim, clear_claim (cell claim tracking) - fn undo (backtracking implementation) All 6 tests verified passing.
Not yet wired into the module tree. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The Node enum's doc comment previously claimed that all relationships appear in the digraph, but fully self-referencing relationships (with neither plain inputs nor pure outputs) contribute no edges and don't appear. Reworded to accurately describe what THIS module guarantees, noting that the caller (Task 5) must account for that case when constructing the execution order. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Not yet wired into the module tree.
Every method in a relationship must now reference the same set of cells (the union of its inputs and outputs). Fixes three existing tests whose relationships had methods referencing inconsistent cell subsets -- including relationship_selected_at_most_once, whose old is_err() assertion turned out to depend on a spurious-conflict bug in the old flood-fill planner (same bug class the broader rewrite fixes), not a genuine unsatisfiability. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
build_digraph previously used matching::pure_outputs, which excludes self-referencing outputs (correct for its original conflict-detection purpose in matching.rs, but wrong for dependency-edge purposes). This meant a relationship whose method writes a self-referencing cell that another relationship reads as a plain input produced no edge between them, letting HashMap iteration order (randomly seeded per process) silently decide execution order -- and thus produce different, wrong results on different runs. Iterate all of a method's outputs instead of just its pure ones. Also update Node's doc comment to reflect the new invariant (every relationship with a valid method now contributes at least one edge) and flip purely_self_referencing_relationship_still_appears_as_a_node's assertion to match its own name, pinning down the exact single edge it now produces. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t-set uniqueness rule Assignment::try_assign previously tracked only pure_outputs (excluding self-referencing cells) as claimed, so release::resolve's strength-ordered loop never got a chance to reconsider which self-referencing method a relationship should use -- method choice always defaulted to index 0 regardless of cell strength. Claiming all outputs (self-referencing included) lets the existing release mechanism handle self-referencing method selection with no special-case logic, matching the classical algorithm's uniform "eliminate the method whose output is already determined" rule. Also adds a validation rule (no two methods in a relationship may share an identical output set) so that elimination is always unambiguous.
…+ SCC pipeline Wires up the new matching/digraph/release/scc modules. plan()'s public signature and Plan's shape are unchanged; forced_output_cells is kept verbatim (now importing pure_outputs from the matching module instead of a private copy). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The loop backfilling adj entries for fully self-referencing relationships was dead code and its comment was stale: build_digraph (since f56e642) draws an edge from every method's outputs, including self-referencing ones, and Sheet::add_relationship already rejects zero-output methods, so every relationship in assignment.chosen is guaranteed at least one edge without this loop's help. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tests Covers the collision pattern from begin/examples/diamond.adm2 (previously Error::Conflict, now resolves), a chain of two overlapping diamonds (exercising the cascade), and confirms a genuine algebraic loop with no external input still correctly reports Error::Conflict.
Companion to the design spec at docs/superpowers/specs/2026-08-04-cyclic-constraint-planner-design.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…d candidates Critical soundness bug found in final whole-branch review: try_assign's shared visited set was never rolled back when a candidate method's blocker displacement failed, even though the corresponding chosen/claimed mutations were correctly undone. A relationship poisoned by a failed displacement attempt stayed marked visited for the rest of the caller's candidate-method loop, so a later candidate encountering the same relationship as a blocker would wrongly treat it as already resolved and silently overwrite its still-valid claim -- producing a double-write that Assignment::solve reported as a successful, but unsound, assignment. Fixed by threading visited insertions through the same trail/undo mechanism already used for chosen/claimed, so a relationship's poisoning cannot outlive the specific candidate attempt that introduced it. Bundled in the same root-cause fix: the blocker-displacement path's old_outputs computation, which still used pure_outputs (excluding self-referencing cells) instead of the full output set when clearing a displaced blocker's previous claims -- a second, related unsoundness in the same code path. Also tightens complexity/determinism doc comments (add_relationship, release::resolve, Assignment::solve) and a minor style fix in digraph.rs (Node::Cell now uses a top-level CellId import).
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.
Summary
Replaces
adam-rs's greedy strength-ordered flood-fill planner with a bipartite/hypergraph matching + Tarjan SCC + strength-ordered release pipeline, fixing a real bug: a constraint graph shaped like two triangle relationships sharing two cells (seebegin/examples/diamond.adm2) was spuriously reported as unsolvable (Error::Conflict) whenever the two outer cells outranked the two shared cells in write-recency strength, even though a valid solution existed.adam-rs/src/planner/{scc,matching,digraph,release}.rs, each with one clear responsibility (generic Tarjan SCC; bipartite/hypergraph matching; dependency digraph + acyclicity check; greedy strength-ordered release).planner.rs'splan()is rewritten around this pipeline; its public API (Plan,plan()'s signature) is unchanged, and the existing test module was preserved and kept passing throughout.Sheet::add_relationshipvalidation (all methods in a relationship must reference the same set of cells).build_digraphwrongly excluded self-referencing outputs from dependency edges, causing non-deterministic wrong results for chains of self-referencing relationships — fixed by drawing edges from all of a method's outputs.Assignment::try_assign's claim-tracking excluded self-referencing outputs entirely, so the strength-ordered release mechanism never reconsidered self-referencing method choice — fixed by claiming all outputs, plus a second validation (no two methods in a relationship may share an identical output set).try_assign's backtracking search never rolled back itsvisitedset on a failed candidate, which could silently poison a blocker relationship and cause a later candidate to overwrite its claim — producing a silent double-write with no error. Fixed by threadingvisitedthrough the existing trail/undo mechanism, with a new regression test assertingchosen/claimedmutual consistency.Design and implementation history:
docs/superpowers/specs/2026-08-04-cyclic-constraint-planner-design.mddocs/superpowers/plans/2026-08-04-cyclic-constraint-planner.mdTest plan
self_ref_le_chain) resolves deterministically,chosen/claimedconsistency invariant🤖 Generated with Claude Code