Repository navigation
fix(delta): persist raw_imports in a deterministic order (follow-up to #3227) - #3234
Merged
Merged
Conversation
…#3227) #3220 persisted raw_imports with `json.dumps(sorted(...))`. That raised TypeError('<' not supported between tuple and str) on the mixed str/(module, entity) list, and #3227 fixed it by dropping the sort entirely. The order is not irrelevant. A rehydrated file's raw_imports is a set (state_rehydrator._json_import_set; test_state_rehydrator.py:132 asserts it), so persisting it in iteration order makes the column hash-seed dependent: seed 0: ["m/n.py", "missing_pkg", "src/lib.py", "z/y.py", ...] seed 1: ["m/n.py", "missing_pkg", ["src/lib.py","helper"], "a/b.py", ...] seed 2: ["src/util.py", ["src/lib.py","helper"], "src/lib.py", ...] That order is the order the resolver walks the imports, which is the order `_publish_edges` emits rows, which is the order #3183 assigns AUTOINCREMENT ids in. Edge VALUES are safe -- weight/import_statements/entity_imports accumulate with `+=`, and addition is commutative -- but row order and ids are not, in a change whose purpose is delta parity. `_ordered_raw_imports` sorts in two groups so a str is never compared to a tuple, which is what broke the original sorted(). Verified stable across PYTHONHASHSEED 0/1/2/99/12345. Scope note: no production path actually emits a tuple today. galaxyscope.py:734 builds raw_imports from `raw_imports.add(clean_module)` (strings only), and both it and :2048 would themselves raise on a tuple. The (module, entity) shape is live only in network_risk_sensor's "Level 2" branch and the test_edge_data fixture, so the tuple handling stays as defensive support -- the reachable bug is the ordering one, over a rehydrated set of plain strings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
CodeQL note: `gitgalaxy.core.state_rehydrator` was imported both as a module (`import ... as sr`) and with `from ... import StateRehydrator`. Only one symbol was needed from the module form, so it joins the existing `from` import and the alias goes away entirely -- rather than the suggested `StateRehydrator = sr.StateRehydrator` rebinding, which keeps both import forms and adds a shadowing assignment. Co-Authored-By: Claude Opus 5 (1M context) <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
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 #3227 / #3220. Small, self-contained.
The problem
#3220 persisted
raw_importsasjson.dumps(sorted(...)).sorted()over the mixedstr/(module, entity)list raisedTypeError: '<' not supported between instances of 'tuple' and 'str', and #3227 fixed it by dropping the sort entirely, with the comment "Order is irrelevant -- it round-trips into a set."The round-trip is fine. The order isn't irrelevant.
A rehydrated file's
raw_importsis a set (state_rehydrator._json_import_set;test_state_rehydrator.py:132already assertsisinstance(..., set)), so persisting it in iteration order makes the stored column depend onPYTHONHASHSEED. Measured on the exact expression atrecord_keeper.py:1333:Why it matters here specifically
That order is the order the resolver walks a file's imports → the order
_publish_edgesemits rows → the order #3183 assignsedge_dataAUTOINCREMENT ids in.To be precise about the blast radius, because it is narrower than it first looks:
weight,import_statementsandentity_importsaccumulate with+=inbuild_dependency_graph, and addition is commutative. No number changes.It is also the bug class this repo just swept in #3212/#3223 ("sort every set you serialise";
unresolved_calls = list(set)was one of them).The fix
Two sort groups, so a
stris never compared to atuple— which is exactly what broke the originalsorted(). Verified stable acrossPYTHONHASHSEED0/1/2/99/12345 (five seeds → one distinct output).Scope note worth recording
No production path emits a tuple today.
galaxyscope.py:734buildsraw_importsfromraw_imports.add(clean_module)— strings only — and both it and:2048would themselves raise on a tuple (sorted(), then.strip()). The(module, entity)shape is live only innetwork_risk_sensor's documented "Level 2 Tuple (Entity Import)" branch and in thetest_edge_data.pyfixture.So the
TypeError#3227 hit was reachable from the fixture, not from a scan. The tuple handling stays as defensive support for the sensor's API; the reachable bug is the ordering one, over a rehydrated set of plain strings. Flagging it in case the intended end state is for tuples to reachram_cache— if so, those twogalaxyscope.pycall sites need the same treatment and that's a separate issue.Verification
Three regression tests in
test_state_rehydrator.py: ordering is stable over 50 reshuffles of the same set, the ordering round-trips back through_json_import_setwith tuples restored as tuples, and the empty/Nonecases.🤖 Generated with Claude Code