Skip to content

Fix #297: [Model] DisjointConnectingPaths#746

Merged
isPANN merged 5 commits intomainfrom
issue-297
Mar 22, 2026
Merged

Fix #297: [Model] DisjointConnectingPaths#746
isPANN merged 5 commits intomainfrom
issue-297

Conversation

@GiggleLiu
Copy link
Copy Markdown
Contributor

Summary

  • add the implementation plan for the DisjointConnectingPaths model
  • map the issue requirements onto the add-model workflow

Fixes #297

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 22, 2026

Codecov Report

❌ Patch coverage is 98.23789% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.62%. Comparing base (6356007) to head (4870b41).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/models/graph/disjoint_connecting_paths.rs 97.36% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #746      +/-   ##
==========================================
+ Coverage   97.61%   97.62%   +0.01%     
==========================================
  Files         435      437       +2     
  Lines       53770    53997     +227     
==========================================
+ Hits        52485    52715     +230     
+ Misses       1285     1282       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@GiggleLiu
Copy link
Copy Markdown
Contributor Author

Implementation Summary

Changes

  • Added the new DisjointConnectingPaths<SimpleGraph> model, including deterministic edge-order encoding, constructor validation, canonical example-db fixture, and focused unit tests.
  • Wired the model into the graph/model exports and prelude so it is discoverable through the registry and example exporters.
  • Added CLI creation support via pred create DisjointConnectingPaths --graph ... --terminal-pairs ..., including parser validation and targeted CLI tests.
  • Documented the model in docs/paper/reductions.typ and added the Robertson--Seymour / Kawarabayashi--Kobayashi--Reed bibliography entries used by the paper entry.

Deviations from Plan

  • The plan mentioned a manual alias update in problem_name.rs, but that was unnecessary: catalog alias lookup is already case-insensitive over canonical names, so DisjointConnectingPaths is discoverable without extra hand-written mapping.

Open Questions

  • None.

@GiggleLiu
Copy link
Copy Markdown
Contributor Author

GiggleLiu commented Mar 22, 2026

Agentic Review Report

Structural Check

Structural Review: model DisjointConnectingPaths

Structural Completeness

# Check Status
1 Deterministic whitelist FAIL — problemreductions-cli/src/cli.rs, problemreductions-cli/src/commands/create.rs, and src/lib.rs are outside the expected whitelist for a model PR
2 Deterministic completeness PASS
3 Model file exists PASS — src/models/graph/disjoint_connecting_paths.rs
4 inventory::submit! present PASS
5 Serialize/Deserialize derive present PASS
6 Problem trait impl present PASS
7 SatisfactionProblem impl present PASS
8 Test link present PASS — #[cfg(test)] + #[path = ...] present
9 Test file exists PASS — src/unit_tests/models/graph/disjoint_connecting_paths.rs
10 Test file has >= 3 test functions PASS — 10 test functions
11 Registered in src/models/graph/mod.rs PASS
12 Re-exported in src/models/mod.rs PASS
13 declare_variants! entry exists PASS
14 CLI alias resolution PASS — generic catalog-based resolve_alias() covers canonical names without a hand-written alias arm
15 CLI create support PASS
16 Canonical model example registered PASS — added via graph::canonical_model_example_specs()
17 Paper display-name entry PASS
18 Paper problem-def block PASS
19 Blacklisted autogenerated files absent PASS

Build Status

  • make test: PASS
  • make clippy: PASS

Semantic Review

  • evaluate() correctness: OK — selected edges must decompose into exactly k path components whose endpoints match the requested terminal pairs; cycles, branching, reused terminals, wrong-length configs, and non-binary entries are rejected.
  • dims() correctness: OK — vec![2; num_edges()] matches the edge-subset encoding described in the issue.
  • Size getter consistency: OK — num_vertices(), num_edges(), and num_pairs() align with the schema and complexity metadata.
  • Weight handling: OK / N/A — this is an unweighted satisfaction model.

Issue Compliance

# Check Status
1 Problem name matches issue OK
2 Mathematical definition matches OK
3 Problem type (sat) matches OK
4 Type parameters match OK — SimpleGraph
5 Configuration space matches OK — one binary variable per edge
6 Feasibility check matches OK
7 Objective function matches OK — boolean satisfiability
8 Complexity matches ISSUE — the code uses the requested 2^num_edges complexity string, but the paper text cites @karp1972 for NP-completeness instead of the linked issue’s approved ND40 / Knuth-Karp-Lynch trail

Summary

  • 18/19 structural checks passed
  • 7/8 issue-compliance checks passed
  • FAIL — deterministic whitelist violation: problemreductions-cli/src/cli.rs, problemreductions-cli/src/commands/create.rs, and src/lib.rs are outside the expected model-PR whitelist.
  • ISSUE — docs/paper/reductions.typ:973-975 documents pred solve disjoint-connecting-paths.json, but the model has no reduction path to ILP, so the documented command fails unless --solver brute-force is added.
  • ISSUE — docs/paper/reductions.typ:969 does not match the linked issue’s approved NP-completeness references.

Quality Check

Quality Review

Design Principles

  • DRY: OK — the implementation follows existing model/create-command patterns without obvious copy-paste debt.
  • KISS: OK — the model uses a single edge-subset validator with straightforward graph checks.
  • HC/LC: OK — graph semantics stay in the model, while CLI parsing stays in create.rs.

HCI (CLI changed)

  • Error messages: OK — invalid terminal-pair inputs return specific messages plus usage guidance.
  • Discoverability: OK — pred list, pred show, and pred create --example all surface the new model cleanly.
  • Consistency: OK — --terminal-pairs fits the existing graph-model CLI patterns.
  • Least surprise: ISSUE — the paper’s documented command pred solve disjoint-connecting-paths.json fails by default because DisjointConnectingPaths has no ILP path; users must infer --solver brute-force from the error instead of from the docs.
  • Feedback: OK — the CLI reports both the missing path and the brute-force workaround clearly.

Test Quality

  • Naive test detection: ISSUE
    • The added tests cover model validation and CLI JSON creation, but nothing exercises the documented end-user solve flow on the canonical example. That gap let the broken paper command ship without a guardrail.

Issues

Critical (Must Fix)

  • None.

Important (Should Fix)

  • docs/paper/reductions.typ:973-975 documents a default pred solve command that fails on the canonical example. Confirmed locally: pred path DisjointConnectingPaths ILP returns No reduction path from DisjointConnectingPaths to ILP, and pred solve <example> exits with Try --solver brute-force.

Minor (Nice to Have)

  • docs/paper/reductions.typ:969 uses an NP-completeness citation that does not match the linked issue’s approved references.
  • No automated test currently protects the documented solve flow for the new model.

Summary

  • ISSUE — the paper’s solve snippet is not reproducible as written.
  • ISSUE — tests do not cover the documented solve flow that regressed.
  • ISSUE — the paper’s hardness citation diverges from the linked issue review.

Agentic Feature Tests

Feature Test Report: problem-reductions

Date: 2026-03-22
Project type: CLI tool / Rust workspace
Features tested: DisjointConnectingPaths model discovery, inspection, creation, solving, and evaluation
Profile: ephemeral read-only review run
Use Case: A downstream CLI user tries to discover the new model from the catalog, create the canonical example, solve it, and confirm the documented commands work as written.
Expected Outcome: pred list, pred show, pred create --example, pred create, pred solve, and pred evaluate should work end-to-end for DisjointConnectingPaths.
Verdict: fail
Critical Issues: 1

Summary

Feature Discoverable Setup Works Expected Outcome Met Doc Quality
DisjointConnectingPaths CLI flow yes yes partial no broken solve snippet

Per-Feature Details

DisjointConnectingPaths CLI flow

  • Role: downstream CLI user validating a newly added model
  • Use Case: discover the model, inspect its metadata, create the canonical example, solve it, and verify the witness config
  • What they tried: pred list, pred show DisjointConnectingPaths, pred show disjointconnectingpaths, pred create --example DisjointConnectingPaths, pred create DisjointConnectingPaths --graph ... --terminal-pairs ..., pred path DisjointConnectingPaths ILP, pred solve <example>, pred solve <example> --solver brute-force, and pred evaluate <example> --config 1,0,1,0,1,0,1
  • Discoverability: Good. The model appears in pred list, pred show works, and lowercase canonical lookup also works.
  • Setup: Good. make cli succeeded and the installed pred binary ran without extra setup.
  • Functionality: Partial. Listing, showing, creating, brute-force solving, and evaluating all worked. Default solving failed because there is no reduction path from DisjointConnectingPaths to ILP.
  • Expected vs Actual Outcome: Expected the documented paper command pred solve disjoint-connecting-paths.json to work. Actual result: exit status 1 with Error: No reduction path from DisjointConnectingPaths to ILP or ILP solver found no solution. Try --solver brute-force.
  • Blocked steps: None.
  • Friction points: The paper suggests a command that does not work unless the user adds --solver brute-force.
  • Doc suggestions: Change the paper snippet to pred solve disjoint-connecting-paths.json --solver brute-force, or explicitly explain that the model currently has no ILP path.

Expected vs Actual Outcome

The catalog and creation flows matched expectations. The documented solve flow did not: the canonical example only solves with --solver brute-force, so the current paper snippet is not reproducible.

Issues Found

  • Confirmed, important: the documented default solve command fails because DisjointConnectingPaths has no ILP reduction path.

Suggestions

  • Update the paper command block to use --solver brute-force.
  • Add a CLI/documentation smoke test that exercises the canonical example with the documented commands.

Generated by review-pipeline

isPANN and others added 2 commits March 22, 2026 16:05
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@isPANN isPANN mentioned this pull request Mar 22, 2026
3 tasks
@isPANN isPANN merged commit adfe9ae into main Mar 22, 2026
5 checks passed
@GiggleLiu GiggleLiu deleted the issue-297 branch April 12, 2026 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Model] DisjointConnectingPaths

2 participants