fix(value_compare): keep non-finite numeric slots out of the comparator (#1227)

[robotrocketscience]

Closes #1227. Found while measuring #1175's lock-consistency proposal against the live store — the measurement script died partway through the scan.

The defect

float() does not raise on overflow, it saturates to infinity. The exponent branch of _NUMERIC_RE matches abbreviated git commit SHAs — 592e701 is a hex string that happens to hold one e between digits — so parsed as scientific notation it yields inf. The extractor's except ValueError never fires, because nothing was raised. _format_number then narrowed with a bare int(x):

def _format_number(x: float) -> str:
    if x == int(x):        # OverflowError on inf, ValueError on nan

Confirmed on the live 44,584-belief store: three beliefs carry such a literal (592e701, 1e124732), and driving the exact shipped call path over them gives

ce869c3e12c7 : returned None
4e2b3977fdb1 : returned '6d849282e0ee17ab'     # detector works
51ebe7a7e1f0 : *** OverflowError ***

Reachable through aelf search when AELF_SHOW_CONFLICTS=1 (cli.py:915/922/932/938). That flag defaults to "0", so this was latent rather than a live break — which is why it survived to be found by a scan rather than by a user.

Fixed at both ends, deliberately

The extractor drops it. An overflowed SHA is a parse artifact, not a measurement. Admitting it as a slot manufactures a comparison against a value no belief asserts — and #1175's data shows junk slots are not hypothetical there, where a single version-string lock accounts for 45% of all detected conflicts.

The formatter guards its own narrowing anyway. Defence in depth for any caller that reaches the comparator by another route.

Either fix alone leaves the other path live, so both are pinned by tests that fail when only the other is applied:

mutation	result
revert the extractor guard only	4 failed — test_extractor_drops_the_non_finite_slot[…]
revert the formatter guard only	3 failed — test_format_number_survives_a_non_finite_input[…]
(control)	14 passed

Two controls keep the tests honest. test_the_literal_really_does_overflow asserts the premise, so that if float() ever stopped saturating the rest of the file could not pass vacuously against a finite value. test_a_finite_neighbour_is_still_extracted asserts an ordinary numeric in the same sentence still extracts — without it, a "fix" that dropped every numeric slot would pass everything else in the file. test_a_real_conflict_is_still_detected_alongside_a_sha covers the same risk end to end.

Verified against the store that crashed

The whole-store scan now completes: 44,559 beliefs, 5,552 slot-conflicts, no exception. One fewer than the format-guard-only count, which is the extractor correctly declining the false slot.

Full suite: 6538 passed, 69 skipped, 71 xfailed.

Why it was worth doing before #1175

#1175's build-first item proposes promoting _slot_conflict_preextracted from that env-gated display path into retrieve_with_tiers — the injection path that runs on every UserPromptSubmit. There the hook's never-raise contract would most likely have absorbed the exception, which is worse than a crash: retrieval degrades silently.

Not in this PR

The extractor still admits a bare \d+e\d+ token as a numeric slot when it does not overflow, so a short SHA can still become a finite false slot. Narrowing that pattern changes what counts as a numeric across the whole comparator and has a wider blast radius than this crash fix; it deserves its own decision. Noted in the changelog entry rather than left implicit.

Summary by Sourcery

Prevent non-finite numeric values extracted from belief text from crashing contradiction detection and ensure they are safely formatted across the comparator path.

Bug Fixes:

• Exclude non-finite numeric values (e.g., overflowed scientific notation from SHA-like literals) from extracted numeric slots so they never enter conflict detection.
• Handle non-finite numeric inputs in number formatting without raising exceptions, returning a string representation instead.
• Ensure contradiction detection and SHA-bearing beliefs can be scanned end-to-end without raising OverflowError while still detecting genuine numeric conflicts.

Documentation:

• Document the SHA-induced overflow crash in the v4 changelog and describe the dual fix and its limitations around finite false numeric slots.

Tests:

• Add targeted tests to assert overflowing literals are non-finite, are not admitted as numeric slots, are still neighbored by valid numerics, that non-finite values are safely formatted, and that conflict detection continues to work and detect real disagreements alongside SHA-shaped literals.

Summary by CodeRabbit

• Bug Fixes

  • Fixed crashes in contradiction detection caused by oversized numeric and SHA-like values.
  • Improved handling of infinity and NaN values so valid neighboring numbers continue to be processed.
  • Numeric conflict detection now remains reliable when unusual numeric literals are present.

• Documentation

  • Added release notes documenting the fix.

[sourcery-ai]

Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[coderabbitai]

Warning

Review limit reached

@robotrocketscience, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 39 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 3cbd574d-7124-476c-90ff-2393a814941b

📥 Commits

Reviewing files that changed from the base of the PR and between 2a044be and a163040.

📒 Files selected for processing (3)

• CHANGELOG/v4.md
• src/aelfrice/value_compare.py
• tests/test_value_compare_nonfinite_1227.py

📝 Walkthrough

Walkthrough

The value comparison logic now rejects non-finite extracted numbers and safely formats infinity and NaN. Regression tests cover overflowed literals, numeric extraction, formatter behavior, conflict detection, and preservation of genuine conflicts. The v4.2.0 changelog documents the fix.

Changes

Non-finite numeric handling

Layer / File(s)	Summary
Numeric extraction and formatting safeguards src/aelfrice/value_compare.py	Adds finite-value validation during extraction and safe formatting for non-finite comparator inputs.
Regression coverage and release note tests/test_value_compare_nonfinite_1227.py, CHANGELOG/v4.md	Adds coverage for overflow literals, extraction, formatting, and conflict detection, and documents the v4.2.0 fix.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the primary fix: excluding non-finite numeric slots from the value comparator.
Description check	✅ Passed	The description thoroughly explains the defect, scope, linked issue, implementation, tests, verification results, and known limitation.
Linked Issues check	✅ Passed	The changes satisfy issue #1227 by guarding non-finite formatting, excluding non-finite extracted slots, and preserving conflict detection behavior.
Out of Scope Changes check	✅ Passed	The code, tests, and changelog changes directly support issue #1227 and introduce no unrelated functionality.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/issue-1227-nonfinite-slot-format

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[sourcery-ai]

Reviewer's Guide

Prevent non-finite numeric values (inf, -inf, nan) from reaching the comparator, and harden the numeric formatter and tests so SHA-shaped literals cannot crash contradiction detection while preserving legitimate numeric extraction and conflict detection.

Sequence diagram for non-finite numeric handling in value comparator

sequenceDiagram
    actor User
    participant aelf_search as aelf_search_cli
    participant Comparator as _slot_conflict_preextracted
    participant Extractor as _extract_numerics
    participant Formatter as _format_number
    participant Math as math_isfinite

    User->>aelf_search: run_search_with_conflicts
    aelf_search->>Comparator: _slot_conflict_preextracted(belief_text)
    Comparator->>Extractor: _extract_numerics(belief_text)

    loop numeric_matches
        Extractor->>Math: math_isfinite(value)
        alt [non_finite_value]
            Math-->>Extractor: False
            Extractor-->>Extractor: continue  # drop slot
        else [finite_value]
            Math-->>Extractor: True
            Extractor-->>Comparator: add NumericSlot(key, value)
        end
    end

    loop numeric_slots
        Comparator->>Formatter: _format_number(x)
        Formatter->>Math: math_isfinite(x)
        alt [non_finite_input]
            Math-->>Formatter: False
            Formatter-->>Comparator: return f"{x:g}"
        else [finite_input]
            Math-->>Formatter: True
            Formatter-->>Comparator: return str(int(x)) or f"{x:g}"
        end
    end

    Comparator-->>aelf_search: slot_conflict_results
    aelf_search-->>User: show_conflicts_without_crash

Loading

File-Level Changes

Change	Details	Files
Numeric extractor now filters out non-finite numeric slots produced by overflowed scientific-notation parses (e.g., SHA-like literals).	Import math and use float() to parse numeric matches, then skip any value where math.isfinite(value) is false. Ensure seen-slot de-duplication and downstream consumers only ever see finite numeric values.	src/aelfrice/value_compare.py
Numeric formatter is hardened to avoid exceptions on non-finite inputs, returning a string representation instead of narrowing with int().	Add a math.isfinite(x) guard in _format_number to short-circuit non-finite values. Return f"{x:g}" for non-finite inputs instead of calling int(x), preventing OverflowError/ValueError. Leave finite-path formatting behaviour unchanged (integer narrowing via int(x) or general g-format).	src/aelfrice/value_compare.py
Changelog documents the SHA-induced crash and the two-part fix, plus explicitly calls out what is and is not changed in numeric-slot extraction behaviour.	Add a detailed v4 changelog bullet describing the overflow behaviour, the two guards (extractor and formatter), and the rationale ahead of [R&D] Belief-logic alternates — conflict detection, lock consistency, injection provenance #1175. Note that finite \d+e\d+ tokens still count as numeric slots and that narrowing the numeric pattern is deferred.	CHANGELOG/v4.md
New tests pin the non-finite handling contract end-to-end, covering extraction, formatting, and conflict detection with SHA-bearing beliefs plus controls to prevent over-broad fixes.	Introduce test_value_compare_nonfinite_1227.py to assert overflow of specific literals and that extract_values() never emits non-finite numeric slots. Add controls verifying neighbouring finite numerics are still extracted and that _format_number() safely formats inf/-inf/nan. Test that find_conflicts() and _slot_conflict_preextracted() do not raise for SHA-bearing beliefs, and that genuine numeric conflicts are still detected alongside SHAs.	tests/test_value_compare_nonfinite_1227.py

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#1227	Modify value_compare._format_number so that it does not raise on non-finite float values (inf, -inf, nan) and instead returns a string representation for them.	✅
#1227	Prevent the contradiction/slot-conflict detection path (e.g., find_conflicts via _slot_conflict_preextracted) from crashing when beliefs contain git SHA-like literals that parse as non-finite numeric values.	✅

Possibly linked issues

• fix(value_compare): _format_number raises on non-finite slot values (git SHAs parse as scientific notation) #1227: PR implements the issue’s proposed fix: guarding _format_number and excluding non-finite numeric slots from extraction, with tests.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[robotrocketscience]

[claim:review:Setr:2026-07-31T03:21:47Z]

[robotrocketscience]

Review: approve

Verified independently rather than taken from the body.

The mutation table reproduces exactly. Checked out the head commit and ran the file three ways:

mutation	result
control	14 passed
delete only if not math.isfinite(value): continue from _extract_numerics	4 failed — test_extractor_drops_the_non_finite_slot[592e701, 1e124732, 4e999, -3e400]
delete only if not math.isfinite(x): return f"{x:g}" from _format_number	3 failed — test_format_number_survives_a_non_finite_input[inf, -inf, nan]

So both halves carry a distinguishing assert, and neither is covered incidentally by the other. That is the property that matters here, because a two-site fix is exactly the shape where one site can be dead weight and nothing notices.

The latency claim holds. find_conflicts reaches _format_number from two places, not one. Besides cli.py under AELF_SHOW_CONFLICTS, relationship_detector.analyze calls extract_values + find_conflicts behind use_value_comparison. That parameter defaults to False and no caller under src/ passes True — classify only threads it through — so the second route was equally cold and the "latent, not a live break" framing survives the wider check. Worth having on the record, since #988's auto-relationship path is where a reader would expect this to have been live, and is_auto_relationship_detection_enabled being default-off is a separate reason rather than the same one.

Blast radius of the extractor guard is nil. float() is called at exactly one site in value_compare.py (line 212), and _format_number at exactly two (296/297, both inside find_conflicts). Dropping non-finite values can only discard a magnitude above ~1.8e308, which no belief asserts as a measurement, so the guard cannot suppress a real conflict — and test_a_finite_neighbour_is_still_extracted plus test_a_real_conflict_is_still_detected_alongside_a_sha are the right two controls for that risk, at the unit and end-to-end levels respectively.

test_the_literal_really_does_overflow is the detail I'd have most expected to be missing. Without it the whole file passes vacuously the day a CPython release makes float("4e999") raise instead of saturate.

Two signed commits, FF on github/main, 22 checks green, CodeRabbit returned no actionable comments, discretion grep on added lines clean, CHANGELOG entry insert-only. Adding ready-to-merge.

One thing to route, not a blocker

The deferred half is real and currently has no home: the extractor still admits a bare \d+e\d+ token as a numeric slot when it happens not to overflow, so a short SHA like 4e56 becomes a finite false slot keyed off whatever word precedes it. Same root cause, no crash, and it manufactures exactly the phantom disagreements #1175's measurement was trying to count — where one version-string lock already accounts for 45% of detected conflicts, a second source of junk slots is not noise-level. It is recorded in the changelog under "Not covered," which is the right call for this PR but is not a queue entry.

I did not file it: no open issue covers it (searched), but the surface belongs to #1175, which is sitting at attn:decisions-needed. @robotrocketscience — its own bug issue, or a line item under #1175?

[robotrocketscience]

[release:review:Setr:2026-07-31T03:24:46Z]

[robotrocketscience]

Correction to my previous comment: the ready-to-merge label did not take, and I have removed it. Between the review and the label, #1230 landed on main and this branch went CONFLICTING, so the merge-train never fired — a conflicting PR produces no merge ref and therefore runs zero pull_request workflows, which reads like the bot ignoring the label rather than declining it.

The approval stands. Nothing about the fix changed; the conflict is packaging only.

Scope, checked with git merge-tree against the merge base — exactly one file:

changed in both
  CHANGELOG/v4.md

Both sides insert a new bullet at the top of the same ### Fixed block under [Unreleased]; src/aelfrice/value_compare.py and the test file are untouched by main.

Resolve it insert-only. Keep both bullets, add yours, delete nothing and reorder nothing. Re-sorting the [Unreleased] block re-adds lines that were already merged, and the discretion gate reads those as newly-introduced content — a known way to turn a one-line changelog conflict into a blocked push. Then rebase on github/main and re-add ready-to-merge; under the concurrency-1 merge-train a promptly-relabelled rebase is FF when its slot comes up.

Flagging attn:merge-conflict so this shows in the conflict section rather than sitting in the review queue as already-reviewed work.

[robotrocketscience]

[claim:review:Kulili:2026-07-31T03:45:58Z]

[robotrocketscience]

Review — approved, merging

Verified the diagnosis, the fix, and the mutation table independently. All
three hold. Two additions below, neither blocking.

Verified

• The claimed mutation table reproduces exactly. Reverting the extractor
  guard alone → 4 failed (test_extractor_drops_the_non_finite_slot[…]);
  reverting the formatter guard alone → 3 failed
  (test_format_number_survives_a_non_finite_input[…]); reverting both → 7
  failed; control → 14 passed. The "each half is pinned against the other"
  claim is the real reason to accept a two-site fix, and it is true here
  rather than asserted.
• The reachability argument is right, and I checked the whole surface, not
  just the named path. NumericSlot has exactly one construction site
  (value_compare.py:219) and _format_number exactly one pair of callers
  (296-297). Every route into find_conflicts today resolves through
  extract_values → _extract_numerics, including the
  _slot_conflict_preextracted hot path, whose locked_pairs the CLI builds
  from extract_values at cli.py:889-892. So the extractor guard alone
  closes every live path and the formatter guard is genuinely
  defence-in-depth. The PR says exactly that; it is not overclaimed in either
  direction.
• No sibling instances of this bug class. x == int(x) at
  value_compare.py:359 is the only narrowing of a float onto int in
  src/, and no other module runs float() over regex-matched belief text.
  This is a one-off, not the first of several.
• CI green on 3.12 and 3.13. Discretion grep on added lines clean. No
  unresolved review threads.

The residual case is reachable, and worse at the display layer than the note implies

The "Not covered" section is correct to scope out narrowing \d+e\d+, and
correct that a short SHA can still become a finite false slot. Confirming it
is reachable end to end so nobody has to re-derive it:

>>> a = extract_values('deploy pinned at commit 592e70')
>>> b = extract_values('deploy pinned at commit 111e22')
>>> find_conflicts(a, b)
(SlotConflict(kind='numeric', key='commit',
  value_a='5920000000000000012547316263131302473387185120223312302370762886496124928',
  value_b='1109999999999999970639872'),)

Two six-character SHAs under the same key produce a reported numeric
conflict. The part not in the note: because x == int(x) is True for large
finite floats, _format_number narrows 5.92e+72 through str(int(x)) and
renders a 73-digit integer in the user-facing conflict message. So the
residual is not only a false positive, it is a false positive that prints
binary float noise as if it were an asserted value.

That branch is the one this PR just guarded, which is why it is worth
recording here rather than leaving it to be rediscovered. Still out of scope
— the fix is to narrow the pattern, exactly as the PR says, and that decision
has a wider blast radius. Worth carrying into whatever issue picks it up
that the display path degrades too, not just the comparison.

Note

_format_number's non-finite branch returns f"{x:g}" → "inf" / "nan",
and is currently unreachable given the single construction site above. That
is the intended shape of a defence-in-depth guard, and vulture passes, so
nothing to do — noting it only so a future dead-code sweep does not read it
as an oversight and delete the half of the fix that has no live caller.

Adding ready-to-merge.

[github-actions]

merge-train: merged a163040 → main via FF push.

[robotrocketscience]

[release:review:Kulili:2026-07-31T03:49:26Z]