lekss361/gendesign

Fork 0

fix(tradein-phase-c): house_type 'monolith'→'monolithic' + log 400 body #537

Merged

lekss361 merged 1 commit from fix/tradein-phase-c-house-type-mapping into main

2026-05-24 15:05:50 +00:00

lekss361 commented

2026-05-24 14:59:40 +00:00

Owner

Problem

Live test после merge PR #534 показал что Avito IMV API отвечает HTTP 400 (silent empty body) для house_type='monolith' и house_type='monolith_brick'. Наш pick_lot_params берёт mode() raw из listings.house_type — для большинства домов ЕКБ это именно 'monolith'. Все 5 тестовых вызовов fail'нулись с одинаковой ошибкой.

Verified live (через docker exec)

house_type	API status
`panel`	✅ 200
`brick`	✅ 200
`monolithic`	✅ 200
`block`	✅ 200
`monolith`	❌ 400
`monolith_brick`	❌ 400

Avito переименовал monolith → monolithic, убрал гибрид monolith_brick.

Fix

scripts/backfill-house-imv.py:

_HOUSE_TYPE_TO_IMV dict для нормализации raw → API-valid value
_map_house_type() helper с panel default
pick_lot_params использует helper вместо raw fallback

app/services/scrapers/avito_imv.py:

_raise_for_status_categorized() логирует resp.text[:200] перед raise_for_status() на >=400 status. Будущие contract changes Avito (PR #524 был один пример, этот — второй) теперь будут видны в логах сразу.

Roadmap

Follow-up к PR #534 (Phase C). После merge можно повторить script на failed houses (--only-status error) — все monolith dom'а должны зайти.

Verify after deploy

ssh gendesign 'docker cp /opt/gendesign/tradein-mvp/scripts/backfill-house-imv.py tradein-backend:/tmp/backfill.py'
ssh gendesign 'docker exec -e PYTHONPATH=/app tradein-backend python /tmp/backfill.py --batch 5 --delay 2 --only-status error'
# Expected: status=ok для большинства

## Problem Live test после merge PR #534 показал что Avito IMV API отвечает **HTTP 400** (silent empty body) для `house_type='monolith'` и `house_type='monolith_brick'`. Наш `pick_lot_params` берёт `mode()` raw из `listings.house_type` — для большинства домов ЕКБ это именно `'monolith'`. Все 5 тестовых вызовов fail'нулись с одинаковой ошибкой. ## Verified live (через docker exec) | house_type | API status | |---|---| | `panel` | ✅ 200 | | `brick` | ✅ 200 | | `monolithic` | ✅ 200 | | `block` | ✅ 200 | | `monolith` | ❌ 400 | | `monolith_brick` | ❌ 400 | Avito переименовал `monolith` → `monolithic`, убрал гибрид `monolith_brick`. ## Fix **`scripts/backfill-house-imv.py`:** - `_HOUSE_TYPE_TO_IMV` dict для нормализации raw → API-valid value - `_map_house_type()` helper с `panel` default - `pick_lot_params` использует helper вместо raw fallback **`app/services/scrapers/avito_imv.py`:** - `_raise_for_status_categorized()` логирует `resp.text[:200]` перед `raise_for_status()` на >=400 status. Будущие contract changes Avito (PR #524 был один пример, этот — второй) теперь будут видны в логах сразу. ## Roadmap Follow-up к PR #534 (Phase C). После merge можно повторить script на failed houses (`--only-status error`) — все monolith dom'а должны зайти. ## Verify after deploy ```bash ssh gendesign 'docker cp /opt/gendesign/tradein-mvp/scripts/backfill-house-imv.py tradein-backend:/tmp/backfill.py' ssh gendesign 'docker exec -e PYTHONPATH=/app tradein-backend python /tmp/backfill.py --batch 5 --delay 2 --only-status error' # Expected: status=ok для большинства ```

lekss361 added 1 commit 2026-05-24 14:59:41 +00:00

fix(tradein-phase-c): map house_type 'monolith'→'monolithic' + log 400 body c533d3affd

Avito IMV API rejects raw listings.house_type values 'monolith' and
'monolith_brick' with HTTP 400 silent body. Verified live: panel/brick/
monolithic/block/wood are valid; monolith/monolith_brick fail.

- backfill-house-imv.py: _HOUSE_TYPE_TO_IMV dict + _map_house_type() helper
  applied to median house_type from linked listings
- avito_imv.py: log response.text snippet on >=400 status before raising
  for future contract drift visibility

lekss361 commented

2026-05-24 15:05:41 +00:00

Author

Owner

Deep Code Review — verdict

Summary

Status: APPROVE (with mandatory follow-up)
Files: 2 (P1: 1, P2: 1)
Lines: +23 / -6
Scope: Phase C backfill script + diagnostic logging

Verified

scripts/backfill-house-imv.py: _HOUSE_TYPE_TO_IMV map matches PR body's verified table (panel/brick/monolithic/block/wood OK at API; monolith and monolith_brick get rerouted to monolithic).
_map_house_type() correctness: None/blank/unknown → panel default (matches prior or 'panel' fallback; no regression).
_raise_for_status_categorized(): warning log fires only on status >= 400 AFTER 401/403/5xx are siphoned off — so it covers the 400-class blind spot exactly as advertised. IMVAddressNotFoundError path doesn't go through HTTP 4xx so log won't be noisy on "address not found".
Shared module impact: log is on the common helper — any future contract drift on live estimator path (_imv_evaluate) will surface immediately. Net positive.
psycopg v3: no new SQL in this PR. pick_lot_params query unchanged (already uses CAST(... AS integer) from PR #534).

High (non-blocking, mandatory follow-up)

backend/app/services/estimator.py:111-119 — live estimator path is still broken for the SAME root cause

_IMV_HOUSE_TYPE_MAP: dict[str | None, str | None] = {
    "panel": "panel",
    "brick": "brick",
    "monolith": "monolith",            # ← still sends old value → API 400
    "monolith_brick": "monolith_brick", # ← still sends old value → API 400
    "monolithic": "monolith",           # ← inverse of script mapping → API 400
    ...
}

Live /api/v1/trade-in/estimate flow:

Frontend HouseType literal (tradein-mvp/frontend/src/types/trade-in.ts:3-9) sends "monolith" or "monolith_brick".
Pydantic TradeInEstimateInput.house_type (backend/app/schemas/trade_in.py:22) accepts only those legacy values — "monolithic" is rejected by frontend type and not in backend Literal.
estimator.py:755 runs _IMV_HOUSE_TYPE_MAP.get(target_house_type) and feeds result to evaluate_via_imv(house_type=...) → POST houseType: "monolith" to Avito → 400, same as your verified table.
_get_or_fetch_imv_cached catches and returns None gracefully — so users don't see an error, but IMV signal is silently dropped for every monolith building ("для большинства домов ЕКБ это именно monolith" — your PR body).

Consequence: backfill script will now successfully populate avito_imv_evaluations with house_type='monolithic', but live estimator cache lookups will miss because compute_imv_cache_key (avito_imv.py:145-160) hashes the literal house_type string. Script writes rows keyed on monolithic, live computes cache_key with monolith → no hit → fresh fetch → 400 → no IMV in user-facing estimate. The backfill cache effectively never serves the live path for monolith houses.

Follow-up PR scope (separate from this one — keep this PR's scope intact):

Update _IMV_HOUSE_TYPE_MAP so the OUTPUT is the new API value:

"monolith": "monolithic",
"monolithic": "monolithic",
"monolith_brick": "monolithic",

Update backend/tests/test_estimator_imv_integration.py:22 — asserts current broken mapping _IMV_HOUSE_TYPE_MAP["monolithic"] == "monolith", must become == "monolithic".
Update stale docstring comment backend/app/services/scrapers/avito_imv.py:269 (# panel/brick/monolith/monolith_brick/block/wood — same outdated list).
Consider invalidating stale cache rows: DELETE FROM avito_imv_evaluations WHERE house_type IN ('monolith', 'monolith_brick') — they were written via failed mapping anyway, so likely already-empty or non-existent.

Medium

backfill-house-imv.py:71 — _HOUSE_TYPE_TO_IMV covers panel/brick/monolith/monolithic/monolith_brick/block/wood but Pydantic schema also allows "other" (schemas/trade_in.py:22). With this PR "other" → default panel, same as old or "panel" fallback — functionally identical, but worth a # 'other' falls to panel default comment so reader knows it's deliberate, not a miss.
backfill-house-imv.py:62 — monolith_brick → monolithic: live verification only tested pure monolithic cells. Hybrid panels in EKB are usually closer to brick by structural class than to pure monolithic. The PR table doesn't claim IMV returns identical pricing for both, only that 400 stops. Worth a follow-up note: "verify recommended_price for monolith_brick houses matches comparable Yandex valuation within ±15% — currently we silently round them down to monolithic."

Low

avito_imv.py:336 — body_preview = (resp.text or "")[:200]: curl_cffi resp.text is a property that may be expensive to compute (decodes body each access). Cheap here since it's already buffered post-post(), but if future migration to streaming HTTP comes, this fires before resp body is fully streamed. Non-issue today.
avito_imv.py:269 — docstring # panel/brick/monolith/monolith_brick/block/wood is now misleading (left intentionally out of scope). Cleanup in follow-up.

Vault cross-check

Memory rule [feedback_check_persistence_gap_across_prs] — this script writes house_type in INSERT to house_imv_evaluations. The value written is the API-mapped one (monolithic), not the raw listing value. That's intentional (cache_key consistency) and matches PR #534 behavior.
Memory rule [feedback_verify_dependency_contract_in_review] — verified evaluate_via_imv does send houseType directly to Avito (no further mapping in scraper). Script fix takes effect end-to-end.

Test coverage

No unit test for _map_house_type() added. Trivial pure function, low risk, acceptable.
Existing test_estimator_imv_integration.py:22 will keep passing — but it codifies the broken estimator mapping. Follow-up PR must flip both code AND test together.

Complexity / blast radius

Risk: low. Script-only behavioral change. Live estimator path unchanged (already broken in the same way, gracefully degraded to None).
Reversibility: trivial. Revert one commit.
Merge window: anytime — admin-only manual script invocation.

Recommended next steps

Merge this PR (unblocks backfill on --only-status error houses).
Open follow-up PR: align estimator._IMV_HOUSE_TYPE_MAP with new API + flip test assertion + stale docstring on avito_imv.py:269 — before running the rerun command in your verify-after-deploy block (cache_key drift means rerunning script populates rows the live path won't see).
After both merged: live qa-tester smoke on a monolith building (Belinskogo 86 or similar) — expect IMV signal in response payload (currently always missing).

## Deep Code Review — verdict  ### Summary - Status: APPROVE (with mandatory follow-up) - Files: 2 (P1: 1, P2: 1) - Lines: +23 / -6 - Scope: Phase C backfill script + diagnostic logging ### Verified - `scripts/backfill-house-imv.py`: `_HOUSE_TYPE_TO_IMV` map matches PR body's verified table (panel/brick/monolithic/block/wood OK at API; `monolith` and `monolith_brick` get rerouted to `monolithic`). - `_map_house_type()` correctness: `None`/blank/unknown → `panel` default (matches prior `or 'panel'` fallback; no regression). - `_raise_for_status_categorized()`: warning log fires only on `status >= 400` AFTER 401/403/5xx are siphoned off — so it covers the 400-class blind spot exactly as advertised. `IMVAddressNotFoundError` path doesn't go through HTTP 4xx so log won't be noisy on "address not found". - Shared module impact: log is on the common helper — any future contract drift on live estimator path (`_imv_evaluate`) will surface immediately. Net positive. - psycopg v3: no new SQL in this PR. `pick_lot_params` query unchanged (already uses `CAST(... AS integer)` from PR #534). ### High (non-blocking, mandatory follow-up) **`backend/app/services/estimator.py:111-119` — live estimator path is still broken for the SAME root cause** ```python _IMV_HOUSE_TYPE_MAP: dict[str | None, str | None] = { "panel": "panel", "brick": "brick", "monolith": "monolith", # ← still sends old value → API 400 "monolith_brick": "monolith_brick", # ← still sends old value → API 400 "monolithic": "monolith", # ← inverse of script mapping → API 400 ... } ``` Live `/api/v1/trade-in/estimate` flow: 1. Frontend `HouseType` literal (`tradein-mvp/frontend/src/types/trade-in.ts:3-9`) sends `"monolith"` or `"monolith_brick"`. 2. Pydantic `TradeInEstimateInput.house_type` (`backend/app/schemas/trade_in.py:22`) accepts only those legacy values — `"monolithic"` is rejected by frontend type and not in backend Literal. 3. `estimator.py:755` runs `_IMV_HOUSE_TYPE_MAP.get(target_house_type)` and feeds result to `evaluate_via_imv(house_type=...)` → POST `houseType: "monolith"` to Avito → **400**, same as your verified table. 4. `_get_or_fetch_imv_cached` catches and returns `None` gracefully — so users don't see an error, but IMV signal is silently dropped for every monolith building ("для большинства домов ЕКБ это именно `monolith`" — your PR body). Consequence: backfill script will now successfully populate `avito_imv_evaluations` with `house_type='monolithic'`, but **live estimator cache lookups will miss** because `compute_imv_cache_key` (`avito_imv.py:145-160`) hashes the literal `house_type` string. Script writes rows keyed on `monolithic`, live computes cache_key with `monolith` → no hit → fresh fetch → 400 → no IMV in user-facing estimate. The backfill cache effectively never serves the live path for monolith houses. Follow-up PR scope (separate from this one — keep this PR's scope intact): - Update `_IMV_HOUSE_TYPE_MAP` so the OUTPUT is the new API value: ```python "monolith": "monolithic", "monolithic": "monolithic", "monolith_brick": "monolithic", ``` - Update `backend/tests/test_estimator_imv_integration.py:22` — asserts current broken mapping `_IMV_HOUSE_TYPE_MAP["monolithic"] == "monolith"`, must become `== "monolithic"`. - Update stale docstring comment `backend/app/services/scrapers/avito_imv.py:269` (`# panel/brick/monolith/monolith_brick/block/wood` — same outdated list). - Consider invalidating stale cache rows: `DELETE FROM avito_imv_evaluations WHERE house_type IN ('monolith', 'monolith_brick')` — they were written via failed mapping anyway, so likely already-empty or non-existent. ### Medium - `backfill-house-imv.py:71` — `_HOUSE_TYPE_TO_IMV` covers `panel/brick/monolith/monolithic/monolith_brick/block/wood` but Pydantic schema also allows `"other"` (`schemas/trade_in.py:22`). With this PR `"other"` → default `panel`, same as old `or "panel"` fallback — functionally identical, but worth a `# 'other' falls to panel default` comment so reader knows it's deliberate, not a miss. - `backfill-house-imv.py:62` — `monolith_brick` → `monolithic`: live verification only tested pure `monolithic` cells. Hybrid panels in EKB are usually closer to brick by structural class than to pure monolithic. The PR table doesn't claim IMV returns identical pricing for both, only that 400 stops. Worth a follow-up note: "verify recommended_price for monolith_brick houses matches comparable Yandex valuation within ±15% — currently we silently round them down to `monolithic`." ### Low - `avito_imv.py:336` — `body_preview = (resp.text or "")[:200]`: curl_cffi `resp.text` is a property that may be expensive to compute (decodes body each access). Cheap here since it's already buffered post-`post()`, but if future migration to streaming HTTP comes, this fires before resp body is fully streamed. Non-issue today. - `avito_imv.py:269` — docstring `# panel/brick/monolith/monolith_brick/block/wood` is now misleading (left intentionally out of scope). Cleanup in follow-up. ### Vault cross-check - Memory rule [feedback_check_persistence_gap_across_prs] — this script writes `house_type` in INSERT to `house_imv_evaluations`. The value written is the API-mapped one (`monolithic`), not the raw listing value. That's intentional (cache_key consistency) and matches PR #534 behavior. - Memory rule [feedback_verify_dependency_contract_in_review] — verified `evaluate_via_imv` does send `houseType` directly to Avito (no further mapping in scraper). Script fix takes effect end-to-end. ### Test coverage - No unit test for `_map_house_type()` added. Trivial pure function, low risk, acceptable. - Existing `test_estimator_imv_integration.py:22` will keep passing — but it codifies the broken estimator mapping. Follow-up PR must flip both code AND test together. ### Complexity / blast radius - Risk: **low**. Script-only behavioral change. Live estimator path unchanged (already broken in the same way, gracefully degraded to `None`). - Reversibility: **trivial**. Revert one commit. - Merge window: anytime — admin-only manual script invocation. ### Recommended next steps 1. Merge this PR (unblocks backfill on `--only-status error` houses). 2. Open follow-up PR: align `estimator._IMV_HOUSE_TYPE_MAP` with new API + flip test assertion + stale docstring on `avito_imv.py:269` — **before** running the rerun command in your verify-after-deploy block (cache_key drift means rerunning script populates rows the live path won't see). 3. After both merged: live `qa-tester` smoke on a monolith building (Belinskogo 86 or similar) — expect IMV signal in response payload (currently always missing).