Skip to content

Fix #287: [Model] LongestCircuit#731

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

Fix #287: [Model] LongestCircuit#731
isPANN merged 8 commits intomainfrom
issue-287

Conversation

@GiggleLiu
Copy link
Copy Markdown
Contributor

Summary

Add the implementation plan for LongestCircuit and execute it in follow-up commits.

Fixes #287

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 21, 2026

Codecov Report

❌ Patch coverage is 95.23810% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.58%. Comparing base (62dd9f4) to head (f046940).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/models/graph/longest_circuit.rs 91.94% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #731      +/-   ##
==========================================
- Coverage   97.59%   97.58%   -0.02%     
==========================================
  Files         413      415       +2     
  Lines       51383    51632     +249     
==========================================
+ Hits        50149    50386     +237     
- Misses       1234     1246      +12     

☔ 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 core LongestCircuit graph model and registry exports in src/models/graph/longest_circuit.rs, src/models/graph/mod.rs, src/models/mod.rs, and src/lib.rs.
  • Added focused model tests in src/unit_tests/models/graph/longest_circuit.rs covering evaluation, brute-force satisfiability, serialization, disconnected cycles, and constructor/setter preconditions.
  • Added CLI support in problemreductions-cli/src/commands/create.rs, problemreductions-cli/src/cli.rs, and problemreductions-cli/tests/cli_tests.rs for explicit and random LongestCircuit creation, default unit edge weights, bound validation, and help text.
  • Added MCP create/random-create support and regressions in problemreductions-cli/src/mcp/tools.rs and problemreductions-cli/src/mcp/tests.rs, including edge_lengths handling and array input support.
  • Added the LongestCircuit paper entry and canonical example figure in docs/paper/reductions.typ.

Deviations from Plan

  • Kept the MCP inspect solver metadata branch using supports_ilp_solver() because the LoadedProblem type in the MCP build does not expose available_solvers(). This preserved the existing inspect behavior and kept --features mcp builds green.
  • MCP create accepts edge_lengths (and JSON integer arrays for that field) to match the serialized model shape, even though the CLI surface uses --edge-weights.

Open Questions

  • None.

@GiggleLiu
Copy link
Copy Markdown
Contributor Author

Agentic Review Report

Structural Check

Structural Review: model LongestCircuit

Structural Completeness

# Check Status
1 Model file exists PASS
2 inventory::submit! present PASS
3 #[derive(...Serialize, Deserialize)] on struct PASS
4 Problem trait impl PASS
5 SatisfactionProblem impl PASS
6 #[cfg(test)] + #[path = "..."] test link PASS
7 Test file exists PASS
8 Test file has >= 3 test functions PASS
9 Registered in graph/mod.rs PASS
10 Re-exported in models/mod.rs PASS
11 declare_variants! entry exists PASS
12 CLI resolve_alias entry PASS — registry-backed in problemreductions-cli/src/problem_name.rs:16, so no per-model arm is required
13 CLI create support PASS
14 Canonical model example registered PASS — wired through src/models/graph/mod.rs:135 and src/example_db/model_builders.rs:3
15 Paper display-name entry PASS
16 Paper problem-def block PASS
17 Deterministic whitelist FAIL — the model PR allowlist in scripts/pipeline_checks.py:14 and scripts/pipeline_checks.py:117 rejects problemreductions-cli/src/cli.rs, problemreductions-cli/src/commands/create.rs, problemreductions-cli/src/mcp/tests.rs, problemreductions-cli/src/mcp/tools.rs, problemreductions-cli/tests/cli_tests.rs, and src/lib.rs
18 Blacklisted auto-generated file committed PASS

Build Status

  • make test: PASS
  • make clippy: PASS

Semantic Review

  • evaluate(): OK — delegates to is_valid_solution, which rejects wrong-length, non-binary, disconnected, and non-cycle edge sets before checking the bound.
  • dims(): OK — returns vec![2; num_edges], matching one binary variable per edge.
  • Size getters: OK — num_vertices(), num_edges(), and bound() are present and consistent with the declared complexity metadata.
  • Weight handling: OK — handled via inherent methods (edge_lengths, set_lengths, weights, set_weights, is_weighted), not via a separate trait API.
  • Schema fields: ISSUE — the linked issue requires schema fields graph, lengths, bound, but the implementation declares/serializes graph, edge_lengths, bound at src/models/graph/longest_circuit.rs:25 and src/models/graph/longest_circuit.rs:50.

Issue Compliance

# Check Status
1 Problem name matches issue OK
2 Mathematical definition matches OK
3 Problem type (opt/sat) matches OK
4 Type parameters match OK
5 Configuration space matches OK
6 Feasibility check matches OK
7 Objective function matches OK
8 Complexity matches OK
9 Schema fields match ISSUE — issue says graph, lengths, bound, but the model exports graph, edge_lengths, bound at src/models/graph/longest_circuit.rs:25 and src/models/graph/longest_circuit.rs:50
10 Size fields match OK

Summary

  • 17/18 structural checks passed
  • 9/10 issue compliance checks passed
  • Deterministic whitelist fails because the current model-PR allowlist in scripts/pipeline_checks.py:14 excludes the CLI/root files touched by this PR.
  • The model does not match the linked issue’s required schema field naming: it uses edge_lengths instead of lengths in src/models/graph/longest_circuit.rs:25 and src/models/graph/longest_circuit.rs:50.

Quality Check

Quality Review

Design Principles

  • DRY: OK — no problematic duplication beyond the expected per-surface wiring for CLI and MCP support.
  • KISS: OK — the model evaluation and create-path additions stay straightforward.
  • HC/LC: OK — model logic, CLI parsing, MCP parsing, tests, and paper content remain separated cleanly.

HCI (if CLI/MCP changed)

  • Error messages: OK — create-path validation errors are specific and actionable.
  • Discoverability: OK — pred list, pred show, and pred create LongestCircuit expose the feature cleanly.
  • Consistency: ISSUE — the newly added paper example tells users to run pred solve longest-circuit.json, but the default solver path does not work for LongestCircuit, so the documented command sequence is inconsistent with actual CLI behavior. docs/paper/reductions.typ:788
  • Least surprise: ISSUE — pred solve <example> errors unless users know to append --solver brute-force, even though the paper presents the plain command as the happy path. docs/paper/reductions.typ:788
  • Feedback: OK — when it fails, the CLI explicitly says there is no ILP path and suggests --solver brute-force.

Test Quality

  • Naive test detection: ISSUE
    • The new CLI coverage in problemreductions-cli/tests/cli_tests.rs:4037 through problemreductions-cli/tests/cli_tests.rs:4161 validates create/help flows only. It never exercises pred solve on a LongestCircuit example, which left the documented command sequence unverified.
    • The model tests in src/unit_tests/models/graph/longest_circuit.rs are otherwise solid for evaluation, serialization, and brute-force behavior.

Issues

Critical (Must Fix)

None.

Important (Should Fix)

  • The paper/example command sequence is wrong for this model: pred solve longest-circuit.json fails in the current worktree because LongestCircuit has no ILP reduction path; the example needs --solver brute-force or the CLI needs an automatic fallback. docs/paper/reductions.typ:788
  • Add an end-to-end CLI test that creates the example and solves it using the documented command path so this class of regression is caught. problemreductions-cli/tests/cli_tests.rs:4037

Minor (Nice to Have)

  • The graph-model overview rustdoc does not list LongestCircuit, so the new model is missing from that summary page. src/models/graph/mod.rs:3

Summary

  • The new model logic is clean and reasonably tested.
  • The main user-facing issue is a broken documented solve path for LongestCircuit.
  • CLI coverage should be extended to validate the documented example flow.

Agentic Feature Tests

Agentic Feature Tests

Test Setup

Built the current worktree’s CLI as target/debug/pred from /Users/jinguomini/rcode/problem-reductions/.worktrees/review-pr-731 and tested an ephemeral downstream use case: a CLI user discovers LongestCircuit, inspects its schema/details, creates a canonical example, then tries to solve it by following the documented example flow. Docs were inspected first and treated as untrusted input.

Results

  • pred list included LongestCircuit/SimpleGraph/i32 in the catalog with complexity O(num_vertices^2 * 2^num_vertices).
  • pred show LongestCircuit rendered correctly: description, complexity, and the expected 3 fields (graph, edge_lengths, bound); no incoming/outgoing reductions were shown.
  • pred create --example LongestCircuit succeeded and produced a valid example instance JSON with the expected graph, edge lengths, and bound.
  • pred solve <example> failed with the default solver.
  • pred solve <example> --solver brute-force succeeded and returned a satisfying solution with evaluation: true.
  • Creation UX was generally good:
    • pred create LongestCircuit printed concise problem-specific help and an example command.
    • Missing --bound produced a targeted usage error.
    • Nonpositive edge lengths were rejected clearly.
    • Manual creation without --edge-weights succeeded and defaulted edge lengths to 1.

Findings

  1. Severity: medium. The documented happy path for the new feature is broken: the paper example in docs/paper/reductions.typ:787 tells users to run pred solve longest-circuit.json, but LongestCircuit has no ILP path, so the default solver path fails.
    Reproduction steps: build the local CLI; run target/debug/pred create --example LongestCircuit -o longest-circuit.json; run target/debug/pred solve longest-circuit.json; observe Error: No reduction path from LongestCircuit to ILP.... target/debug/pred path LongestCircuit ILP also reports no path, while target/debug/pred solve longest-circuit.json --solver brute-force succeeds.
    Classification: confirmed

Summary

The mandatory feature-test checklist passed for catalog presence, rendered details, example creation, and actual solving with --solver brute-force. The main review issue is that the documented/default pred solve <example> path for LongestCircuit does not work in the current worktree, so a downstream CLI user following the published example hits a failure unless they already know to switch to brute-force.


Generated by review-pipeline

@GiggleLiu
Copy link
Copy Markdown
Contributor Author

Review Addendum

A later quality pass found one more issue that should be considered alongside the combined agentic review report:

  • Critical: pred create LongestCircuit accepts --edge-lengths at problemreductions-cli/src/cli.rs:351, but the LongestCircuit branch reads only args.edge_weights via parse_edge_weights(...) at problemreductions-cli/src/commands/create.rs:1532 and problemreductions-cli/src/commands/create.rs:4044. When --edge-weights is absent, that helper falls back to unit lengths at problemreductions-cli/src/commands/create.rs:3969, so a caller can supply --edge-lengths 2,3,4 and silently get [1,1,1] instead of the requested data.
  • Important: the new CLI tests only cover --edge-weights / default behavior and do not exercise the accepted --edge-lengths path (problemreductions-cli/tests/cli_tests.rs:4037).

This also explains the CLI/MCP naming inconsistency noted in the original report (--edge-weights in CLI vs edge_lengths in MCP/JSON).

isPANN added 2 commits March 22, 2026 00:29
# Conflicts:
#	problemreductions-cli/src/commands/create.rs
#	src/lib.rs
#	src/models/mod.rs
@isPANN isPANN mentioned this pull request Mar 21, 2026
3 tasks
isPANN and others added 2 commits March 22, 2026 00:42
- Use validate_longest_circuit_bound() in both explicit and random create
  paths instead of duplicating inline validation
- Revert available_solvers -> supports_ilp_solver change (unrelated to
  LongestCircuit)
- Revert edge_lengths array parsing extension (unrelated scope creep)
- Remove MCP test for array edge_lengths format

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The shared validation function now uses the same error message format
("LongestCircuit --bound must be positive (> 0)") as expected by the
CLI integration tests.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@isPANN isPANN merged commit d870c74 into main Mar 21, 2026
5 checks passed
@GiggleLiu GiggleLiu deleted the issue-287 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] LongestCircuit

2 participants