Certify max-flow optimality instead of trusting recorded values - #6
Merged
Conversation
The 0.7.2 under-reporting bug (fixed in 0.8.0) survived ~400 tests because the suite could not reach it, not because it checked too loosely. Measured: none of the shared fixtures expose it, at any src/dst pair (0 of 78 combinations), so no amount of assertion strengthening would have caught it. The binding constraint was input generation - every test graph had near-uniform costs, and the defect needs dispersed costs to trigger. Add an oracle-free optimality certificate and generate graphs in the region where such defects live: - conftest: certify_max_flow checks feasibility, conservation, that the reported min-cut genuinely separates src from dst, and tightness. By weak duality those four together *prove* the flow is maximum, so no external solver and no stored expected value is needed. Masked runs are supported, and maximal=False covers EQUAL_BALANCED and shortest_path=True, where only the first two properties hold. - test_maxflow_certificate: seeded sweeps over full-duplex graphs with dispersed costs, plus certification of every shared fixture across all src/dst pairs. The seed was chosen so its corpus trips the 0.7.2 defect within the first few graphs; both sweeps were confirmed to fail against 0.7.2 and pass against 0.8.0. Includes a guard that the certificate rejects a known non-maximal flow, so it cannot decay into an assertion that always passes. - conftest: assert_valid_min_cut previously checked only that edge ids were unique and in range, both of which an empty cut satisfies vacuously - so it was guaranteed to pass on exactly the output the bug produced. It now takes an optional total_flow and asserts duality. Wired into the four call sites that compute a true maximum; the EQUAL_BALANCED and shortest_path sites are annotated with why duality does not apply there. - test_review_regressions: the batch/serial parity test generated uniform costs and compared two paths that both call calc_max_flow, so it could only ever prove consistency, never correctness. Costs are now dispersed and results certified; this makes the test fail against 0.7.2, which it did not before. 444 Python tests pass (from 433), suite 2.4s -> 3.8s. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd4db563fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The sweep supplied only node_mask, so it never reached calc_max_flow's edge_mask branches -- including the two inside the 0.8.0 residual completion phase, whose forward and reverse arc loops consult edge_mask independently (src/max_flow.cpp:190, :202) and compute_min_cut's own edge-mask path. The certificate helper's edge_mask handling was likewise unexercised despite the test claiming coverage for masked-out edges. Rotate through node-only, edge-only, and combined masking, and assert both branches were actually reached so the sweep cannot silently degrade into an unmasked one. This is prospective branch coverage, not retroactive detection: the masked sweep passes against 0.7.2 either way, since the under-reporting defect is not mask-specific. Verified separately that the masked-edge assertion does fire on a violating input, so it is not another check that cannot fail. Reported by Codex review on #6. Co-Authored-By: Claude Opus 5 <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
1 participant
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.
Follow-up to the 0.8.0 max-flow fix. That release fixed the defect; this closes the
detection gap that let it survive ~400 tests.
Why the old suite missed it
I measured rather than assumed, and the intuitive answer turned out to be wrong.
It was not weak assertions. None of the shared fixtures expose the bug — not at the
pair each test queries, and not at any other pair. 0 of 78 (fixture, src, dst)
combinations disagree with an independent oracle under 0.7.2. Strengthening every
assertion in the suite would have caught exactly nothing.
It was input generation. Every test graph had near-uniform costs (mostly all-
1,occasionally
1,1,2,2). The defect needs dispersed costs — randomized sweeps find0/2000 failures at spread ≤
1..2. The suite faithfully modeled its intended domain(uniform-cost Clos fabrics), and that domain is precisely the region where the bug
cannot occur.
Worth noting: the bug lived in thoroughly covered code — the tier loop ran in all 78
passing cases. Line coverage was never going to find this.
What this adds
An optimality certificate instead of an oracle.
certify_max_flowchecks fourproperties: feasibility, conservation, that the reported min-cut genuinely separates
source from sink, and tightness. By weak duality those together prove the flow is
maximum — so there's no external solver and no recorded expected value to drift.
I deliberately did not add
networkxas a dev dependency. Measured, it adds zerodetection over the certificate for this defect class, while costing a dependency across
14 CI jobs. The certificate is also faster, and reaches configurations no external solver
models — masked runs and
EQUAL_BALANCED, which is exactly where the next defect of thiskind would hide.
Generation in the trigger region. Seeded sweeps over full-duplex graphs with dispersed
costs. Since a fixed seed makes detection deterministic, the corpus has to be shown to
have teeth — the seed was chosen so it trips the 0.7.2 defect within the first few graphs.
An arbitrary seed needed ~5750 graphs to hit the same defect, so this is a cheaper corpus,
not a narrower one.
A guard against decay.
test_certificate_rejects_a_non_maximal_flowproves thecertificate rejects a genuinely non-maximal result, reached through the public API. Without
it the certificate could quietly become an assertion that always passes — which is exactly
how the old helper failed.
The vacuous helper.
assert_valid_min_cutchecked only that edge ids were unique andin range. An empty list satisfies both trivially, so the one helper positioned to catch this
was guaranteed to pass on the
flow=2.0, min_cut=[]output the bug produced. It nowasserts duality when given the total. Honest accounting: this would not have caught the
bug (0/78), and it's included because ~7 max-flow tests currently assert nothing that can
fail — not because it's the fix.
A consistency-vs-correctness fix. The batch/serial parity test generated uniform costs
and compared two paths that both call
calc_max_flow— it could only prove they agreed,never that they were right. Costs are now dispersed and results certified. It now fails
against 0.7.2, which it did not before.
Validation
Every item was run against both versions:
assert_valid_min_cut444 Python tests pass, up from 433. Suite 2.4s → 3.8s.
🤖 Generated with Claude Code