Skip to content

Surface swallowed errors across pipeline and UI - #13

Open
devin-ai-integration[bot] wants to merge 2 commits into
mainfrom
devin/1786296072-error-handling
Open

Surface swallowed errors across pipeline and UI#13
devin-ai-integration[bot] wants to merge 2 commits into
mainfrom
devin/1786296072-error-handling

Conversation

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Summary

An audit of every except handler outside autotrader_isolated/ found ~107 broad handlers; most were legitimate fallbacks, but a large group turned real operational failures (unreadable CSV, missing model artifact, corrupt lock/marker JSON, bad timezone config) into empty frames, False, or partial results with no record anywhere. This PR keeps the resilience but makes the failure visible, and fixes two places where a failure was reported as success.

Two deliberate behaviour changes:

# scripts/update_bids.py — was: log and return the partial frame as if it succeeded
     if not skip_master:
-        try:
-            update_master.update_master_database()
-        except Exception as exc:
-            logger.error(f"Failed to run update_master after update_bids: {exc}")
+        update_master.update_master_database()
     return df, skipped_urls
 except Exception:
     logger.exception("Unexpected error in update_bids; attempting emergency snapshot before failing.")
     ...persist_dataframe(df, "Emergency save")...
-    return df, skipped_urls
+    raise

The emergency snapshot is still taken; the caller now learns the run failed. pages/3_ACTIVE_LISTINGS.py wraps the call and renders st.error/st.exception instead of continuing as if bids were refreshed.

# scripts/update_master.py — a failed restricted build no longer just prints
-except Exception as exc:
-    print(f"Restricted dataset build failed: {exc}")
+except Exception as exc:
+    raise RuntimeError("Restricted dataset build failed after master update.") from exc

This makes the daily job fail loudly (health report + Telegram failure alert) rather than leaving the restricted datasets that feed AI valuation silently stale.

Everything else is narrowing + reporting. A shared tuple centralises what a CSV read can legitimately raise:

# shared/csv_utils.py
CSV_READ_ERRORS: tuple[type[BaseException], ...] = (OSError, ValueError, csv.Error)

(pandas ParserError/EmptyDataError both derive from ValueError.) Handlers in shared/ (data_loader, csv_utils, telegram_alerts, scraper_health, curves, canonical_tagging, repair_pricing*, repair_review, repair_ai_classifier, manual_curve_evidence, missed_opportunities, auction_model, ui_helpers), scripts/ (scheduled_jobs, update_bids, update_master, extract_vehicle_details, ai_price_analysis, ai_listing_valuation, scrape_bid_history, scrape_autotrader_rego, process_curve_candidates, run_grays_pipeline_loop), ops/active_monitor.py, two Streamlit pages, and status_app.py now catch only what they expect and log/print the path plus type(exc).__name__: exc before falling back. Notable ones where silence was actively misleading: auction_model._try_load_models() (valuations silently drop to curve-only pricing), _load_alert_log/_load_alert_state (duplicate Telegram alerts get re-sent), _should_refresh() (stale-data marker), and _local_timezone() (coverage dates silently shift to UTC).

Two broad handlers were removed rather than narrowed — derive_auction_site() (pure string split) and record_snapshot()'s .iloc[0] lookup, which now uses an explicit matches.empty check. Also fixed update_bids() returning [] instead of a DataFrame on the empty-seed path.

tests/test_error_visibility.py asserts the new diagnostics are emitted (via caplog) for CSV/alert-log/state-file failures and that the restricted-build failure propagates.

Verified: 634 passed, ruff check . clean, readiness_smoke, governance_checks check, project_memory check (and --staged), check_commit_hygiene --staged all pass. project_memory/02_state/recent_changes.md updated.

Link to Devin session: https://app.devin.ai/sessions/86d7aa075214441192e549f6879fbad4
Requested by: @fallen-pc

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@fallen-pc fallen-pc self-assigned this Aug 9, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 06bc155384

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread tests/test_error_visibility.py Outdated

monkeypatch.setattr(update_master, "build_restricted_datasets", _boom)
monkeypatch.setattr(update_master, "_load_dataframe", lambda *_a, **_k: pd.DataFrame())
monkeypatch.setattr(update_master, "_write_master_outputs", lambda *_a, **_k: None, raising=False)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Isolate the update_master test from runtime CSVs

This monkeypatch creates _write_master_outputs, but update_master_database() never calls that helper; it writes to SOLD_FILE, REFERRED_FILE, and ACTIVE_FILE directly via _merge_preserving_history()/_atomic_write() before build_restricted_datasets() is reached. In a normal pytest run against the tracked CSV_data baseline, this test can rewrite the real runtime CSVs while only intending to assert the raised RuntimeError; redirect the file constants or write helpers to tmp_path instead.

AGENTS.md reference: AGENTS.md:L101-L102

Useful? React with 👍 / 👎.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants