fix(screening): address code-review findings on Rentometer fallback chain - #378
Merged
Conversation
…hain Multi-agent review of #376 (Rentometer adapter) surfaced 9 issues, fixed here: - Rentometer's address-based lookup could never fire for HUD/USDA sources: _PipelineView (the adapter used for those sources) never carried address/city/state, so the whole feature silently degraded to HUD FMR for 2 of 4 property source types. _adapt_source_to_pipeline now populates them from the source model. - The DB-write cache-on-read in _get_monthly_rent assumed pipeline_property always has a `.pk`, which _PipelineView (no persisted row) doesn't have — an AttributeError there was silently swallowed, discarding a successfully fetched rent. Added a _cache_rent() helper that only writes when a pk exists. - That same DB write also silently violated screening_preview()'s documented "without saving" contract. screen_property() and the eval helpers now take a cache_rent flag; screening_preview() passes cache_rent=False. - Both `except Exception: pass` blocks in _get_monthly_rent logged nothing, making a real bug (schema drift, broken import) look identical to "no data available" (AGENTS.md rule #10). Now logged, and RentometerClient._get() classifies timeouts/5xx/malformed JSON into RentometerError instead of leaking raw requests exceptions. - _extract_zip's ZIP regex matched the first 5-digit token in an address, which could be a house number. Anchored to the end of the string instead. - rentometer.py's hand-rolled, unbounded _rent_cache dict is replaced with django.core.cache (matching walkscore.py/schools.py in the same package), fixing both the multi-worker cache-miss problem and the memory-growth issue. - _parse_rent no longer round-trips through binary float before producing a Decimal. - Fixed a stale "future lookups" docstring on the bedrooms param (it's used today, for the cache key). Added tests for the fallback chain (Rentometer success, Rentometer- fails-then-HUD-succeeds, cache_rent=False, HUD/USDA _PipelineView non-crash), the ZIP/house-number regex regression, _parse_rent edge cases, and RentometerClient._get()'s error classification. Full existing suite plus new tests: 913 passed (4 pre-existing, unrelated ATTOM live-API failures — missing test credentials). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CI on this branch caught three issues in the previous commit's fixes: - _cache_rent() referenced PipelineProperty.objects.filter(...) but PipelineProperty is only imported under `if TYPE_CHECKING:` in this module — a NameError on every real invocation, silently swallowed by the (now-logged) except block. This bug predates this branch: the original inline code had the same gap, which is exactly why the previous commit's new fallback-chain tests (the first to actually exercise a successful rent lookup) caught it. Added a local `from core.models import PipelineProperty` inside _cache_rent, matching this file's existing lazy-import convention for model access. - CodeQL flagged the new cache-key hash (MD5 over address/city/state/ zip) and the new log statements (interpolating the raw zip_code) as PII handling issues. Switched the cache key to SHA-256, and swapped the logged zip_code for pipeline_property's pk (an opaque internal id, not location data). Verified: the two previously-failing E2E tests now pass, full rentometer/screening suite (61 tests) green, ruff/mypy clean. Co-Authored-By: Claude Sonnet 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
2 participants
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.
Summary
A multi-agent code review of #376 (Rentometer API adapter) surfaced 9 findings. This fixes all of them:
_PipelineView(the adapter used for those two source types) never carriedaddress/city/state, so the guard in_get_monthly_rent()always failed and the feature's whole point (replacing HUD FMR with real comps) silently didn't apply to half the source types._adapt_source_to_pipeline()now populates them.AttributeErrorcould discard a fetched rent — the DB-write cache-on-read assumedpipeline_property.pkalways exists, which_PipelineViewdoesn't have. Added a_cache_rent()helper that only writes when apkis present.screening_preview()'s "without saving" contract was silently violated —_get_monthly_rent()wrote to the DB as a side effect even in preview mode.screen_property()/eval helpers now take acache_rentflag; the preview view passescache_rent=False.except Exception: pass— both catch blocks in_get_monthly_rent()logged nothing, making a real bug indistinguishable from "no data" (violates AGENTS.md rule Set up Copilot instructions #10). Now logged.RentometerClient._get()also classifies timeouts/5xx/malformed JSON intoRentometerErrorinstead of leaking rawrequestsexceptions past every catch point._extract_zip()'s fallback grabbed the first 5-digit token in an address; anchored to the end of the string instead.rentometer.py's module-level_rent_cachedict is replaced withdjango.core.cache, matching the existing pattern inwalkscore.py/schools.pyin the same package (fixes both the multi-worker cache-miss problem and unbounded memory growth)._parse_rent()float round-trip — now goes straight toDecimalinstead of through binary float.bedroomsparam said "future lookups" but is already used today for the cache key.Changes
core/services/screening.pycore/integrations/market/rentometer.pycore/views/__init__.py(screening_previewpassescache_rent=False)tests/test_rentometer.py,tests/e2e/test_rentometer_e2e.py(new coverage)Test plan
cache_rent=False, HUD/USDA_PipelineViewdoesn't crash), ZIP/house-number regex regression,_parse_rentedge cases,RentometerClient._get()error classificationruff check/ruff format --check/mypyclean🤖 Generated with Claude Code