Skip to content

Fix #226: [Model] AcyclicPartition#723

Merged
isPANN merged 8 commits intomainfrom
issue-226
Mar 21, 2026
Merged

Fix #226: [Model] AcyclicPartition#723
isPANN merged 8 commits intomainfrom
issue-226

Conversation

@GiggleLiu
Copy link
Copy Markdown
Contributor

Summary

Add the implementation plan for AcyclicPartition from issue #226 and prepare the branch for execution.

Fixes #226

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 21, 2026

Codecov Report

❌ Patch coverage is 99.67846% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 97.63%. Comparing base (a4e05bd) to head (862156d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/models/graph/acyclic_partition.rs 99.35% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #723      +/-   ##
==========================================
+ Coverage   97.62%   97.63%   +0.01%     
==========================================
  Files         403      405       +2     
  Lines       50112    50423     +311     
==========================================
+ Hits        48923    49233     +310     
- Misses       1189     1190       +1     

☔ 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 AcyclicPartition graph model with validation, quotient-DAG semantics, canonical example specs, and explicit size-field registration in src/models/graph/acyclic_partition.rs.
  • Registered the model in the library surface and added focused model tests covering construction, satisfiability semantics, invalid inputs, canonical solutions, serde, and size-field reporting.
  • Added pred create AcyclicPartition CLI support, example-driven CLI coverage, and inspect coverage for the new model.
  • Added the AcyclicPartition paper entry in docs/paper/reductions.typ.

Deviations from Plan

  • Explicit ProblemSizeFieldEntry registration was added after pred inspect exposed that size fields are not inferred automatically for new models.

Open Questions

  • None.

@GiggleLiu
Copy link
Copy Markdown
Contributor Author

Agentic Review Report

Structural Check

Structural Review: model AcyclicPartition

Structural Completeness

# Check Status
1 Model file exists PASS — src/models/graph/acyclic_partition.rs
2 inventory::submit! present PASS — schema and size-field entries are registered in src/models/graph/acyclic_partition.rs
3 Serialize / Deserialize on struct PASS — AcyclicPartition derives both
4 Problem trait impl PASS — impl Problem for AcyclicPartition<W> present
5 SatisfactionProblem impl PASS — impl SatisfactionProblem for AcyclicPartition<W> present
6 Test link from model file PASS — #[cfg(test)] #[path = "../../unit_tests/models/graph/acyclic_partition.rs"] present
7 Test file exists PASS — src/unit_tests/models/graph/acyclic_partition.rs
8 Test file has >= 3 tests PASS — 14 tests
9 Registered in graph/mod.rs PASS — module + re-export added
10 Re-exported in models/mod.rs PASS
11 declare_variants! entry exists PASS — default sat AcyclicPartition<i32> => "num_vertices^num_vertices"
12 CLI problem lookup support PASS — registry-backed lookup works through existing problem_name.rs flow; no manual alias table entry required
13 CLI create support PASS — problemreductions-cli/src/commands/create.rs adds explicit constructor path
14 Canonical model example registered PASS — canonical example spec is authored in src/models/graph/acyclic_partition.rs and collected from src/models/graph/mod.rs
15 Paper display-name entry PASS — docs/paper/reductions.typ
16 Paper problem-def block PASS — docs/paper/reductions.typ
17 Blacklisted auto-generated files absent PASS

Build Status

  • make test: PASS
  • make clippy: PASS

Semantic Review

  • evaluate() correctness: OK for well-formed instances. Manual review plus the added tests confirm the implementation enforces config length, label range, per-part weight caps, total crossing cost, and quotient-DAG acyclicity.
  • dims() correctness: OK — vec![num_vertices; num_vertices] matches the issue’s [n; n] configuration space.
  • Size getter consistency: OK — num_vertices() and num_arcs() are present and match the declared size fields / complexity string.
  • Input validation: ISSUE — the model derives Deserialize directly and the constructor only validates vector lengths, not the issue’s positive-weight / positive-cost domain. As a result, malformed or mathematically invalid JSON instances are accepted by pred solve. Confirmed reproductions returned evaluation: true for negative weights/costs and only No solution found for length-mismatched payloads. Relevant code: src/models/graph/acyclic_partition.rs:44-61, src/models/graph/acyclic_partition.rs:183-236.
  • Deterministic whitelist note: the generated packet reported Whitelist: fail, but manual audit shows that is tooling drift rather than a PR defect. New model PRs now legitimately touch problemreductions-cli/src/* and src/lib.rs for registration/help surface changes, and the whitelist script has not been updated accordingly.

Issue Compliance

# Check Status
1 Problem name matches issue OK
2 Mathematical definition matches ISSUE — solver/load path accepts invalid negative or malformed instance data that the issue defines out of scope (Z+, positive bounds)
3 Problem type (sat) matches OK
4 Type parameters match OK
5 Configuration space matches OK
6 Feasibility check matches ISSUE — feasibility logic is correct for well-formed instances, but deserialization does not enforce the issue’s input invariants
7 Objective / metric matches OK
8 Complexity matches OK

Summary

  • 17/17 structural checks passed after manual audit.
  • 6/8 issue-compliance checks passed fully.
  • ISSUE — invalid AcyclicPartition instances deserialize without validation and can yield incorrect satisfiable answers or misleading unsat-style errors.
  • Note — the packet’s whitelist failure is a tooling false positive, not a product issue in this PR.

Quality Check

Quality Review

Design Principles

  • DRY: OK — the core feasibility logic is centralized in is_valid_acyclic_partition() and reused by both evaluate() and is_valid_solution().
  • KISS: OK — the dense-label quotient-graph check is straightforward and appropriate for the problem size.
  • HC/LC: ISSUE — instance validation is split between CLI-only checks in problemreductions-cli/src/commands/create.rs:2982-3015 and an unchecked model type in src/models/graph/acyclic_partition.rs:44-61. JSON/API consumers bypass the invariants entirely, so correctness depends on which entry point created the instance.

HCI (if CLI/MCP changed)

  • Error messages: OK — pred create reports missing required flags and arc-cost length mismatches clearly.
  • Discoverability: OK — AcyclicPartition appears in pred list, pred show, and pred create --help.
  • Consistency: OK — the create flags and help text follow existing directed-graph model patterns.
  • Least surprise: ISSUE — pred create rejects negative weights/costs, but pred solve accepts equivalent hand-authored invalid JSON and may even report evaluation: true. That inconsistency is user-visible and surprising.
  • Feedback: OK — create/solve/inspect all report concrete outputs for the happy path.

Test Quality

  • Naive test detection: ISSUE
    • The model tests cover constructor panics and happy-path serialization, but there are no deserialization rejection tests or CLI tests for invalid JSON payloads. That gap allowed the invalid-instance bug above to slip through. Relevant coverage: src/unit_tests/models/graph/acyclic_partition.rs:98-123, src/unit_tests/models/graph/acyclic_partition.rs:199-209, problemreductions-cli/tests/cli_tests.rs:1954-2045.

Issues

Critical (Must Fix)

  • src/models/graph/acyclic_partition.rs:44AcyclicPartition derives Deserialize without validation, and new() does not enforce nonnegative / positive weights, arc costs, or bounds. Reproductions from the current worktree:
    • Solving {"vertex_weights":[-5,1],"arc_costs":[1],"weight_bound":0,"cost_bound":1,...} returned evaluation: true.
    • Solving {"vertex_weights":[1,1],"arc_costs":[-5],"weight_bound":2,"cost_bound":-4,...} returned evaluation: true.
    • Length-mismatched payloads deserialize and only fail later as Error: No solution found instead of being rejected as malformed input.

Important (Should Fix)

  • src/unit_tests/models/graph/acyclic_partition.rs:199 and problemreductions-cli/tests/cli_tests.rs:1954 — the new tests exercise only valid serialization / solve flows. Add rejection tests for malformed JSON and negative values so the deserialization hole cannot regress silently.

Minor (Nice to Have)

  • None.

Summary

  • ISSUE — validation lives only in the pred create path, while the model type itself accepts invalid data through deserialization.
  • ISSUE — the user-facing behavior is inconsistent: invalid instances are rejected in create but accepted in solve.
  • ISSUE — missing invalid-input tests left that behavior uncovered.

Agentic Feature Tests

Feature Test Report: problem-reductions

Date: 2026-03-21 16:44:49
Project type: CLI
Features tested: AcyclicPartition
Profile: ephemeral
Use Case: Discover the new AcyclicPartition model from the CLI, inspect its schema, create the canonical example instance, and solve it end-to-end.
Expected Outcome: AcyclicPartition is discoverable and usable through the documented CLI flow without surprises.
Verdict: fail
Critical Issues: 1

Summary

Feature Discoverable Setup Works Expected Outcome Met Doc Quality
AcyclicPartition yes yes yes partial good

Per-Feature Details

AcyclicPartition

  • Role: Downstream CLI user familiar with pred but new to this model
  • Use Case: Find the model in the catalog, inspect its schema, create the canonical example, solve it, and inspect the saved instance.
  • What they tried: Ran pred list, pred show AcyclicPartition, pred create --help, pred create --example AcyclicPartition/i32 -o <tmp>, pred solve <tmp> --solver brute-force, and pred inspect <tmp>.
  • Discoverability: Good. The model appears in pred list, pred show, and pred create --help.
  • Setup: No extra setup beyond building the CLI from the workspace.
  • Functionality: Happy-path commands all succeeded. pred create --example wrote a valid instance, pred solve found a satisfying partition, and pred inspect reported the expected size fields.
  • Expected vs Actual Outcome: Mostly matched. Normal usage is smooth, but invalid hand-authored JSON is not rejected during load.
  • Blocked steps: None.
  • Friction points: Invalid JSON instances with negative weights/costs or malformed vector lengths are accepted by pred solve; negatives can even produce satisfiable answers instead of a validation error.
  • Doc suggestions: None for the happy path. The main gap is implementation-side validation, not user-facing documentation.

Expected vs Actual Outcome

The expected downstream flow succeeded for valid example data. The outcome was only partial because malformed AcyclicPartition JSON is currently accepted instead of being rejected during deserialization.

Issues Found

  • Critical: pred solve accepts invalid AcyclicPartition JSON because the model derives Deserialize without validation. Reproductions:
    • Negative weights: solving {"vertex_weights":[-5,1],"arc_costs":[1],"weight_bound":0,"cost_bound":1,...} returned evaluation: true.
    • Negative costs/bounds: solving {"vertex_weights":[1,1],"arc_costs":[-5],"weight_bound":2,"cost_bound":-4,...} returned evaluation: true.
    • Length mismatches: solving payloads with too-short vertex_weights or arc_costs loaded successfully and only failed later as Error: No solution found, instead of rejecting malformed input.

Suggestions

  • Add validated deserialization for AcyclicPartition, analogous to other directed-graph models that use try_new or unchecked DTOs plus explicit validation.
  • Reuse the same validation in the constructor/setters and deserialization path so JSON load, API construction, and CLI behavior stay consistent.
  • Add CLI or model tests covering invalid JSON payloads for negative values and mismatched vector lengths.

Generated by review-pipeline

@isPANN isPANN mentioned this pull request Mar 21, 2026
3 tasks
The G&J definition specifies weights, costs, and bounds in Z+ (positive
integers). Updated CLI validation from `< 0` (non-negative) to `<= 0`
(positive) to match the mathematical definition.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@isPANN isPANN merged commit b400550 into main Mar 21, 2026
5 checks passed
@GiggleLiu GiggleLiu deleted the issue-226 branch April 12, 2026 00:51
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] AcyclicPartition

2 participants