fix(determinism): seed the enum member index in sorted order (#1365 AC4) - #1391
fix(determinism): seed the enum member index in sorted order (#1365 AC4)#1391robotrocketscience wants to merge 2 commits into
Conversation
Reviewer's GuideEnsures deterministic ordering of enum value extraction across different PYTHONHASHSEED values by sorting enum group members when building the reverse lookup index, and adds a subprocess-based regression test that verifies stability of the ordering across multiple hash seeds. Sequence diagram for subprocess-based hash seed regression testsequenceDiagram
participant Pytest
participant SubprocessRunner
participant PythonInterp as PythonInterpreter
participant ValueCompare as value_compare
Pytest->>SubprocessRunner: spawn_interpreters(PYTHONHASHSEED values)
loop for each hash seed
SubprocessRunner->>PythonInterp: start_python_with_env(PYTHONHASHSEED)
PythonInterp->>ValueCompare: import value_compare
ValueCompare-->>PythonInterp: build _ENUM_MEMBER_INDEX with sorted(group)
PythonInterp->>ValueCompare: _extract_enums(input_text)
ValueCompare-->>PythonInterp: ValueSlots(enum ordering)
PythonInterp-->>SubprocessRunner: return enum ordering
end
SubprocessRunner-->>Pytest: collected_orderings
Pytest->>Pytest: assert all_orderings_equal()
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 44 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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. Comment |
robotrocketscience
added
the
attn:review
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
assert only.count("'") >= 6heuristic in the stability test is a bit brittle; consider emitting a structured format (e.g., JSON) from the subprocess and asserting on the actual list length (≥3 members) instead of inferring it from quote counts. - In the subprocess call you fully override
envwith justPYTHONHASHSEED,PATH, andHOME; if you only need to controlPYTHONHASHSEED, it may be safer to start fromos.environ.copy()and override that key to avoid surprising issues from missing locale or platform-specific environment variables.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `assert only.count("'") >= 6` heuristic in the stability test is a bit brittle; consider emitting a structured format (e.g., JSON) from the subprocess and asserting on the actual list length (≥3 members) instead of inferring it from quote counts.
- In the subprocess call you fully override `env` with just `PYTHONHASHSEED`, `PATH`, and `HOME`; if you only need to control `PYTHONHASHSEED`, it may be safer to start from `os.environ.copy()` and override that key to avoid surprising issues from missing locale or platform-specific environment variables.
## Individual Comments
### Comment 1
<location path="tests/test_value_compare.py" line_range="332-333" />
<code_context>
+ )
+ # The scenario must actually exercise the multi-member case, or the
+ # assertion above is satisfied by a one-element list.
+ (only,) = orderings
+ assert only.count("'") >= 6, f"scenario stopped matching >=3 members: {only}"
</code_context>
<issue_to_address>
**suggestion (testing):** Use structured parsing instead of quote-counting to assert multi-member matches
This guard currently relies on counting single quotes in the string representation, which is brittle and tied to formatting details (quote style, list formatting, or member names containing quotes). Instead, parse the list and assert on its length directly, e.g.:
```python
import ast
(only,) = orderings
members = ast.literal_eval(only)
assert len(members) >= 3, f"scenario stopped matching >=3 members: {members!r}"
```
This makes the test more robust and clearly checks the number of members rather than their string formatting.
Suggested implementation:
```python
assert len(orderings) == 1, (
"enum extraction order varies with PYTHONHASHSEED; "
f"{len(orderings)} orderings across {len(seeds)} seeds:\n "
+ "\n ".join(sorted(orderings))
)
# The scenario must actually exercise the multi-member case, or the
# assertion above is satisfied by a one-element list.
(only,) = orderings
members = ast.literal_eval(only)
assert len(members) >= 3, f"scenario stopped matching >=3 members: {members!r}"
```
You also need to ensure `ast` is imported at the top of `tests/test_value_compare.py`, e.g. add `import ast` alongside the other imports. If the file already has an `ast` import, no further changes are required.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
[claim:review:Idnn:2026-08-06T04:31:52Z] |
Review — reproduced from scratch, correct. Driving it to merge.I re-derived the defect and the fix rather than reading the body forward. The nondeterminism is real, and the magnitude reproducesTwelve The five, verbatim: Your "5 across 8 seeds" is consistent with this — 5 is the number of orderings
The test does what it claimsReverting The subprocess-per-seed design is the right call and worth stating plainly: Asserting the orderings agree rather than pinning one literal is also right: ScopeFixing the index rather than sorting the output is the root-cause fix — sorting Good catch on the #1370 overlap, and on noting it there rather than silently State
Nothing blocking. |
2eee497 to
aef4b1e
Compare
|
Overlap notice — this collides with #1393 (§8 of #1370). Both branches fix the same defect: This is content-scoped duplicate work that the claim mutex cannot catch — #1370 bundles the fix as one of four determinism sections ( Proposed resolution: this PR takes the fix, #1393 drops its §8 commit. This one is narrower and easier to review in isolation. #1393's other two commits (store.py tiebreak, graph_spectral.py seeding) are independent and unaffected. One thing worth porting across if you take it. #1393's test for this is out-of-process: it spawns six children under Deferring to whoever reviews first. I will drop the commit from #1393 on confirmation. |
robotrocketscience
added
the
ready-to-merge
|
merge-train: blocked branch is not fast-forward on The |
github-actions
Bot
removed
the
ready-to-merge
ENUM_VOCAB's groups are frozensets. Iterating them directly to build _ENUM_MEMBER_INDEX seeds the dict in string-hash order, which PYTHONHASHSEED varies per process; _extract_enums walks that dict and appends matches as it finds them, so the enum half of ValueSlots came out in a different order in different processes. Measured before the fix: five distinct orderings across eight seeds for one input where three members of one group match. Latent for the conflict set, which is order-independent -- which is why it has survived. Not latent for #1365's rendered annotation, which names which lock a belief conflicts with and so is decided by that order. The regression test is subprocess-driven across four seeds, because one interpreter has one hash seed and an in-process assertion structurally cannot observe this. Mutation-checked: reverting the sort yields four orderings across four seeds and the test goes red.
The multi-member guard asserted `only.count("'") >= 6` against the repr
of the child process's output. It works, but it is a proxy for the thing
it means, and it breaks silently on a member name containing an
apostrophe or on any change to list formatting.
`ast.literal_eval` on the printed list gives the members exactly, so the
assertion is `len(members) >= 3` — which is what the comment above it
already said it was checking. Re-verified that the guard still
discriminates: reverting `sorted(group)` still turns it red.
Refs #1365.
aef4b1e to
29d7870
Compare
|
Superseded by #1393, which ships the identical Independently verified before closing: #1393's Keeping two copies of a one-line change to the same comprehension would only guarantee a merge conflict for whichever landed second. #1365 AC4 is therefore satisfied by #1393 rather than by this PR. Recorded on #1365, which stays open for AC1-AC3 (the |
robotrocketscience
added
the
ready-to-merge
|
merge-train: blocked FF push to The |
github-actions
Bot
removed
the
ready-to-merge
|
[release:review:Idnn:2026-08-06T04:45:57Z] |
1 participant
Part of #1365 (AC4). Does not close it — this is the prerequisite the issue names: "Sort the extracted slot tuples before this becomes load-bearing."
The defect
ENUM_VOCAB's groups are frozensets. Building_ENUM_MEMBER_INDEXby iterating them directly (value_compare.py:141-146) seeds the dict in string-hash order, whichPYTHONHASHSEEDvaries per process._extract_enumsthen walks that dict and appends matches in the order it finds them — so the enum half ofValueSlots, not merely the index, came out in a different order in different processes.Measured before the fix, on
"the run is nondeterministic and stochastic and also non-deterministic"(three members of one group match):group_idwas never affected — it is alreadysorted(group)[0].Why it has survived
The conflict set is order-independent, so every current consumer is blind to it. It stops being latent the moment #1365's annotation renders which lock a belief conflicts with:
lock_conflict_annotationsreturns the first conflicting lock, and "first" is decided by this order. The agent would be shown a different lock id on different runs against an identical store.The fix
for member in sorted(group)in the innermost comprehension clause. Root cause, not the symptom — sorting the output tuple would leave the index itself still randomised for any future consumer that walks it.Verification
tests/test_lock_consistency_1175.py,test_slot_conflict.py,test_value_compare.py,test_value_compare_nonfinite_1227.py→ 81 passed.The regression test is subprocess-driven across four
PYTHONHASHSEEDvalues, because one interpreter has one hash seed and an in-process assertion structurally cannot observe this defect. It carries an explicittimeout(60)per the convention inpyproject.toml:131-133— four interpreter starts are not unit-sized work. It asserts the orderings agree rather than pinning one literal, so it keeps its meaning when a vocabulary entry is added, plus a guard that the scenario still matches ≥3 members (otherwise a one-element list satisfies it vacuously).Overlap
This is also the
_ENUM_MEMBER_INDEXitem in #1370. Noted on that issue so its owner drops it; the rest of #1370 (spla.eigshseeding, theretrieval.py:2923pack loop,_ORDER_BY_BM25) is untouched here.Rollback
Revert the single commit. No data, schema, config or default changes.
Summary by Sourcery
Tests: