fix: ten review findings, each one a silent wrong answer - #136
fix: ten review findings, each one a silent wrong answer#136skishchampi wants to merge 10 commits into
Conversation
Every one of these returns a plausible result rather than an error. Each fix has a test that fails first. geoserver, four findings, and three of them defeat the module's own purpose: - The sweep discarded the point location. It stored `properties` and dropped `geometry`, so a POINT extractor returned attributes and no points. - A tile capped at the minimum span was ingested as complete. A cap means "there may be more", so the densest clusters were truncated in silence. Capped leaves are now reported and the sweep is marked PARTIAL. - The verification grid shifted by half the REGION instead of half a cell. A 4-degree box with 2-degree cells moved 2 degrees. It queried ground outside the region, left the leading edge untested, then reported saturation. - Saturation was claimed even when every verification tile failed. An empty `new` set proves nothing when the second pass asked no questions. `sweep` now reports failed and capped tiles through a `status` dict, and `verify` refuses to certify a pass with holes in it. http_client: the stdlib client ignored `json=`. A default install posted an empty body, and the server answered as though the caller had sent nothing. The fix goes in the shared client, not in each caller. aspnet: a Hindi save button arrives as numeric HTML entities, so the literal Devanagari hints never matched and a live WRITE control read as harmless. The label is decoded before classification. cdn_dashboard: a geo-fence 403 was read as an absent period. Outside the publisher's country every object 403s, so a blocked run produced a clean empty dataset. It now raises `GeoFenced`. An ordinary 403 still means absent. cdn_dashboard: `cryptography` was imported and declared nowhere. It is now the `crypto` extra, and it is in `all` and `dev`. aspnet_cascade: reseating reused the poisoned session, so the recovery path could not recover. It builds a new session, unless the caller injected one. The User-Agent is now a parameter, because some deployments answer 500 to this package's identifier.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f3f869084
ℹ️ 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".
| get it from a named extra rather than by guessing.""" | ||
| import pathlib | ||
|
|
||
| import tomllib |
There was a problem hiding this comment.
The repository supports Python 3.10 and the inspected .github/workflows/ci.yml runs the full suite on 3.10, but tomllib was only added to the standard library in Python 3.11. This new test therefore fails during import on the 3.10 CI job; use a compatible parser or avoid parsing TOML in this test.
Useful? React with 👍 / 👎.
| # it here rather than in each caller: every adapter shares this | ||
| # client, and the next one would repeat the bug. | ||
| body = json.dumps(kwargs["json"]).encode("utf-8") | ||
| headers.setdefault("Content-Type", "application/json") |
There was a problem hiding this comment.
When a stdlib-client caller supplies a JSON request with a differently cased header such as headers={"content-type": "application/vnd.api+json"}, this case-sensitive setdefault adds a second Content-Type. urllib.request.Request normalizes both names and the newly added application/json value overwrites the caller's media type, unlike the requests-backed path's case-insensitive behavior; check existing header names case-insensitively before adding the default.
Useful? React with 👍 / 👎.
The P1 is mine, and it would have broken CI rather than a crawl. My new test imported tomllib, which arrived in 3.11. This package supports 3.10, and the workflow runs the suite on 3.10, 3.11 and 3.12. The test now reads the extras with a regex. The P2: HTTP header names are case-insensitive and `setdefault` is not, so a caller who sent `content-type` in another case would have received a SECOND, conflicting header rather than keeping their own.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb86d2fda0
ℹ️ 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".
| step = start_span / 2 | ||
| shifted = (west + step, south + step, east + step, north + step) |
There was a problem hiding this comment.
When either bounding-box dimension is no larger than half of start_span, this unconditional shift moves the verification pass entirely outside the requested region. For example, the default start_span=2 changes (76, 12, 77, 13) into (77, 13, 78, 14); if that unrelated area returns no features, partial remains false and the method incorrectly reports saturated=True. Construct the offset grid within—or clipped around—the original bounds so every supported box actually receives an overlapping verification pass.
Useful? React with 👍 / 👎.
My own previous fix caused this. It shifted by half a cell unconditionally, so a box smaller than one cell moved clean off its own ground: (76,12,77,13) with a 2-degree cell became (77,13,78,14). Finding nothing there means nothing, and the method then reported saturation over ground the layer never claimed. The shift is now bounded per axis at half the region. A box can be wide and short, so the two axes are computed separately.
|
@codex review — fixed in the push above. The shift is now bounded per axis at half the region, so a box smaller than one cell keeps an overlapping verification pass. This defect was mine: the previous commit fixed the large-box case and broke the small-box one. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a54805439c
ℹ️ 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".
| self._factory = session_factory or ( | ||
| None if session is not None |
There was a problem hiding this comment.
When a caller provides both an authenticated session and a session_factory, this assignment retains the factory, so the next reset() replaces the injected session and discards its authentication despite the constructor's stated guarantee. Either reject the conflicting arguments or ensure that any supplied session forces _factory to None.
Useful? React with 👍 / 👎.
Review found the code and its own docstring disagreeing. The docstring promised that an injected session is never replaced. The code replaced it whenever a factory was also supplied. The contract now says what the code does, because the code is right. Passing a factory IS the caller saying that a rebuild is theirs to define, so it wins even beside an injected session: a caller whose session carries a login supplies a factory that can re-establish it. A session with no factory is still never replaced. A crawl in that state cannot recover from an expired session, and that is the caller's trade to make. Three combinations, three tests.
|
@codex review — 7f741ca. On this one I changed the CONTRACT, not the behaviour, and here is the reasoning. A caller passes A session with NO factory is still never replaced. Three combinations, three tests. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f741ca058
ℹ️ 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".
| None if session is not None | ||
| else lambda: make_session(rate_limit_sec=rate_limit_sec, | ||
| user_agent=user_agent)) | ||
| self.session = session or self._factory() |
There was a problem hiding this comment.
When an injected session has false truthiness (for example, a dict-like session or test double with __len__ == 0), the no-factory combination sets _factory to None and then evaluates self._factory(), raising TypeError; with both arguments, it prematurely uses the factory instead of the injected session. The documented contract distinguishes absence with session is None, so this assignment should do the same rather than using or.
Useful? React with 👍 / 👎.
| A session the caller injected is kept: replacing it would discard an | ||
| authentication this class did not create. |
There was a problem hiding this comment.
When both session and session_factory are supplied, reset() replaces the injected session at lines 143–144, but this method-level contract still says every injected session is kept. That directly contradicts the newly documented constructor contract and can mislead callers inspecting reset() into assuming their session survives; qualify this statement for the no-factory case.
Useful? React with 👍 / 👎.
`session or make_session()` discards any session object whose truthiness is false. A session that defines __len__ or __bool__ is ordinary, and the caller then runs against a client this package built without saying so. Review found one site. The idiom was in fourteen, across ten modules, so all fourteen now test `is not None`. Fixing the reported line alone would have left the same defect in nine other adapters. Also: `reset()` promised that an injected session is never replaced, which the class docstring had already corrected. The method now states all three cases.
|
Two of the three are fixed, and one is a standing decision. Falsey sessions: fixed, and wider than reported. The idiom was in fourteen sites across ten modules, so all fourteen now test reset() docstring: fixed. It contradicted the class docstring. It now states all three cases. Factory precedence beside an injected session: standing. Passing |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc2c9f3a21
ℹ️ 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".
| self.report_url = report_url | ||
| self.controls = controls | ||
| self.session = session or make_session(rate_limit_sec=rate_limit_sec) | ||
| self._factory = session_factory or ( |
There was a problem hiding this comment.
When session_factory is a callable object with false truthiness (for example, a stateful factory implementing __len__), this or treats the supplied factory as absent: alongside an injected session it sets _factory to None, so reset() never rebuilds, and without a session it silently substitutes the default client. The documented contract distinguishes absence with None, so select the factory using session_factory is not None just as the session assignment does.
Useful? React with 👍 / 👎.
The last commit fixed the session and left the same idiom on the factory one line above it. A factory is a callable OBJECT as often as a lambda, and an object that defines __len__ can be falsey. `or` then dropped the caller's factory and built the default client instead.
|
@codex review — the factory now uses Factory precedence beside an injected session stays as it is, for the reason given twice above. Passing a factory is the instruction for how to rebuild. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65c0a5431c
ℹ️ 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".
| # per axis, because a box can be wide and short. | ||
| step_x = min(start_span, east - west) / 2 | ||
| step_y = min(start_span, north - south) / 2 | ||
| shifted = (west + step_x, south + step_y, east + step_x, north + step_y) |
There was a problem hiding this comment.
For every positive offset, this moves the verification bounds east and north without adding boundary tiles on the west and south. For example, (76, 12, 80, 16) with start_span=2 produces tiles beginning at (77, 13), so a feature near (76.5, 12.5) that the first sweep missed is never queried by the verification pass; new can remain empty and saturated becomes true incorrectly. Build shifted interior cells while also covering or clipping the leading boundary strips to the original bounding box.
Useful? React with 👍 / 👎.
Shifting the box east and north offset the cell boundaries correctly. It also left the western and southern strips of the region with no second pass, while `saturated` was still reported for the whole region. A first-pass miss in those strips was never examined. The grid ORIGIN now moves backwards by half a cell instead. The boundaries still fall between the first pass's query points, and every part of the region lands inside some tile. A test asserts coverage at the four corners and the centre. Third defect in this method, and the third caught by review rather than by me. Each fix was right about the case reported and wrong about its neighbour: large boxes, then small boxes, then the leading strips.
|
@codex review — fixed. The grid ORIGIN now shifts backwards by half a cell, instead of the whole box shifting forwards. The cell boundaries still fall between the first pass's query points, and every part of the region gets a second pass. A test asserts coverage at all four corners and the centre. This is the third defect of mine in this method that the review caught. Each fix corrected the reported case and broke a neighbour: large boxes, then small boxes, then the leading strips. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0a1f69d7f
ℹ️ 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".
| # pass at all — and saturation is claimed for the WHOLE region. Starting | ||
| # half a cell before the region keeps the offset and covers every part | ||
| # of it. | ||
| shifted = (west - step_x, south - step_y, east, north) |
There was a problem hiding this comment.
When features exist immediately west or south of the requested bbox, the fresh backward shift queries them as part of the verification pass, and new = set(got) - known then counts those out-of-region IDs as first-pass misses. For example, verifying (76, 12, 80, 16) now sweeps down to (75, 11), so a feature at (75.5, 11.5) makes an otherwise complete in-region extraction report saturated=False. Keep the offset query layout, but clip/filter its returned features to the original bbox before computing pass2 and new.
Useful? React with 👍 / 👎.
The grid starts half a cell before the region, so it queries ground west and south of it. A feature there was never this extraction's to find. Counting it as a first-pass miss made a complete sweep report saturated=False. Pass-2 features are now filtered to the requested bbox before `pass2` and `new` are computed. The offset layout is unchanged, so coverage of the region stays. A feature with no point geometry cannot be placed. It is KEPT and counted, and the count is returned as `unlocatable`. Dropping it would hide a real miss. Keeping it in silence would hide the doubt, so the number travels with the verdict.
|
@codex review — a5f9b3f. Pass-2 features are filtered to the requested bbox before A feature with no point geometry cannot be placed. It is kept and counted, and the count returns as |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5f9b3f9be
ℹ️ 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".
| if geom.get("type") != "Point" or not isinstance(coords, (list, tuple)) or len(coords) < 2: | ||
| return None |
There was a problem hiding this comment.
When a point layer returns valid GeoJSON MultiPoint geometry, this strict type == "Point" check classifies it as unlocatable, so a MultiPoint lying wholly west or south of the requested bbox is retained from the backward-shifted tiles and counted in pass2/new, incorrectly making an otherwise complete extraction report saturated=False. Fresh evidence in this revision is that the new filter handles only scalar Points even though MultiPoints also provide placeable point coordinates; test their coordinates against the bbox (retaining them as unlocatable only when they truly cannot be evaluated).
Useful? React with 👍 / 👎.
A point layer serves MultiPoint as readily as Point. The strict type check made those features unplaceable, so an out-of-region MultiPoint still counted as a first-pass miss and a complete sweep still reported saturated=False. One point inside now makes the feature the region's. A feature whose points all lie outside is excluded. Any other geometry stays unplaceable: a polygon has no single answer here, and guessing one would be the invented geometry this module exists to refuse.
|
@codex review — MultiPoint is fixed. A point layer serves MultiPoint as readily as Point, and my strict The exclude-outside-region finding was raised again on the same hunk. It is fixed: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbf9e64680
ℹ️ 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".
| if any(m in body for m in GEO_FENCE_MARKERS): | ||
| raise GeoFenced( |
There was a problem hiding this comment.
When a 403 response contains one of the geo-fence markers, this branch now raises GeoFenced, but fetch() still tells callers that every 403 represents an absent object, cannot be distinguished, and returns None. Callers following that public contract may be surprised by the new exception, so document the split between ordinary 403 responses and geo-fenced responses.
Useful? React with 👍 / 👎.
The geo-fence raise landed in an earlier commit here, and this docstring still told callers that every 403 means an absent period, and that they should not try to tell them apart. It now names the one case that raises and why.
|
Geo-fence docstring: fixed. Exclude features outside the verified region: this is the third time on the same hunk, and it is fixed. Evidence rather than assertion:
If the finding survives against |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Ten findings from the automated review of #130, #131, #134 and #135. Every one
returns a plausible result rather than an error. Each fix has a test that fails
first.
geoserver — four, and three defeat the module's own purpose
propertiesand droppedgeometrymin_spanwas ingested as completesweep()now reports failed and capped tiles through astatusdict, the waythe LS paginator reports skipped offsets.
verify()refuses to declaresaturation when that dict is non-empty, and returns
partial,failed_tilesand
capped_tiles.The offset now moves half a CELL. A 4-degree box with 2-degree cells shifts 1
degree, verified against the emitted BBOX parameters.
http_client — the stdlib client ignored
json=A default install posted an EMPTY body, and the server answered as though the
caller had sent nothing. The OTP flow could not work without the
httpextra.The fix goes in the shared client rather than in each caller, so the next
adapter cannot repeat it. An explicit
data=still wins.aspnet — an entity-encoded label defeated write-button detection
A Hindi save button arrives as numeric HTML entities, so the literal Devanagari
hints never matched and a live WRITE control read as harmless. That is the
check standing between a crawler and an insert into a live government system.
cdn_dashboard — two
A geo-fence 403 was read as an absent period. Outside the publisher's country
every object 403s, so a blocked run produced a clean empty dataset. It now
raises
GeoFenced, and an ordinary 403 still means absent.cryptographywas imported and declared nowhere. It is now thecryptoextra,and it is in
allanddev. A test asserts both.aspnet_cascade — two
Reseating reused the poisoned session, so the recovery path could not recover.
It now builds a new session — unless the caller injected one, which is not this
class's to discard.
The User-Agent is a parameter now. Some deployments answer 500 to this
package's own identifier. There is no default: overriding it stays a deliberate
act, per the posture recorded for the archives adapter.
The one I refuted
The P1 on #135 asked for a shim on
otp_download_portal. That path is in norelease and no tag, and all six consumers pin a release. Evidence is in the
#135 thread.
Verification
1,440 passed, ruff clean.