Cover max_path_cost_factor, salvaged from an abandoned branch - #7
Merged
Conversation
While verifying that main supersedes cursor/critical-bug-fixes-6f68 (an unmerged March 2026 branch fixing the same defect class as v0.8.0) it turned out that max_path_cost_factor appears in no test at all, Python or C++. The 0.8.0 fix at src/flow_policy.cpp:149-158 - which skips the relative bound while best_path_cost_ is still the INT64_MAX sentinel, and drops it when the product would not fit - was shipped untested. The abandoned branch had a test for this, but not one that can be ported: it builds a graph with cost INT64_MAX-5, which main now rejects at construction (the 2**62 total-cost ceiling). These cover the same concern with graphs main accepts: the sentinel path before any best cost exists, a factor large enough to overflow the product, and the bound still functionally excluding a too-expensive alternative. Scope is stated in the docstring: these are coverage, not detectors of the original UB. Run against 0.7.2 they pass, because an out-of-range float->int conversion is undefined rather than reliably wrong. UBSan is what discriminates fixed from unfixed; this gives it something to instrument on a path it previously never reached. 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: 85512e3c82
ℹ️ 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".
Codex review on #7 was right on both counts, and the first led somewhere worse than reported. 1. `make sanitize-test` never ran the Python tests. The target builds a sanitized extension and then invokes ctest, so the tests added in the previous commit were never instrumented. The claim in their docstring that UBSan discriminated fixed from unfixed was simply false. 2. Those tests could not reach the sentinel branch anyway. best_path_cost_ is updated from the sentinel at src/flow_policy.cpp:135, before the gate runs, so a reachable destination always leaves it set. Only an unreachable destination keeps INT64_MAX live into the multiply-and-cast. Following (1) turned up the larger problem: UBSan findings were never fatal. Nothing set -fno-sanitize-recover or UBSAN_OPTIONS, so UBSan printed "runtime error: ... is outside the range of representable values" and let the process continue to exit 0. ctest reported success. Undefined behaviour was being detected and then discarded, which is indistinguishable from not checking. SAN_FLAGS now carries -fno-sanitize-recover=undefined. The full sanitized suite passes with it, so nothing else in the codebase was relying on that leniency. Adds FlowPolicyCostBounds.MaxPathCostFactor_{UnreachableDst_KeepsSentinelOutOfTheCast, HugeProduct_DropsBoundInsteadOfWrapping} in C++, where ctest actually runs them. Both need min_flow_count >= 1: seeding is what calls get_path_bundle, and without it the gate is never reached at all. Verified by temporarily reverting the 0.8.0 guard to its pre-fix form: fixed code -> 156/156 pass under fatal UBSan pre-fix code -> both new tests fail, at src/flow_policy.cpp with the exact out-of-range diagnostic The Python tests keep their functional value and their docstring now states plainly that they are coverage only, and points at the C++ tests for detection. 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.
Fallout from cleaning up the abandoned
cursor/*branches. Before deletingcursor/critical-bug-fixes-6f68— 6 unmerged commits from March 2026 fixing the samedefect class as v0.8.0 — I checked that
maingenuinely supersedes it rather thanassuming my own later work had covered it. It does, with one exception.
max_path_cost_factorappears in no test at all, Python or C++. The 0.8.0 fix atsrc/flow_policy.cpp:149-158 — skip the relative
bound while
best_path_cost_is still theINT64_MAXsentinel, drop it when the productwould not fit — shipped untested.
The abandoned branch had a test for exactly this, but it cannot be ported: it builds a graph
with cost
INT64_MAX - 5, whichmainnow rejects at construction under the 2^62 total-costceiling. (That rejection is itself why the branch's other overflow tests are unnecessary —
mainprevents the situation where the branch saturated through it.)These three cover the same concern with graphs
mainaccepts:best_cost * factoroverflows int64 — a wrapped negative boundwould reject the shortest path itself, so a successful placement is what distinguishes the
guard from the bug
Scope, stated plainly in the docstring: these are coverage, not detectors of the original
UB. Run against 0.7.2 they pass, because an out-of-range float→int conversion is undefined
rather than reliably wrong — in practice it saturates to something that still admits the path.
UBSan (
make sanitize-test) is what discriminates fixed from unfixed; this gives it somethingto instrument on a path it previously never reached. Standalone, they guard the functional
behaviour of the bound during future refactors.
447 tests pass, up from 444.
🤖 Generated with Claude Code