fix: redact location details from diagnostics

[IceCodeNew]

Summary

• omit location names from routine processing logs
• keep geocoding failures diagnostic using location IDs, provider operations, and exception types
• suppress underlying provider exceptions that can contain raw query text
• preserve the user-facing precision-reduction confirmation notice

Verification

• targeted CLI and geocoding tests: 132 passed
• full branch-coverage suite: 592 passed; 15 misses and 7 uncovered branches
• all prek hooks passed

[coderabbitai]

Warning

Review limit reached

@IceCodeNew, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 41 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9d7c30e9-9511-4092-bb76-073af70153ff

📥 Commits

Reviewing files that changed from the base of the PR and between 3c5e47f and 75c281c.

📒 Files selected for processing (5)

• docs/requirements.md
• tests/test_cli.py
• tests/test_geocoding.py
• weather_briefing/cli.py
• weather_briefing/geocoding.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/remove-location-log-details

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.64%. Comparing base (44248a3) to head (75c281c).
⚠️ Report is 3 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master      #49   +/-   ##
=======================================
  Coverage   99.64%   99.64%           
=======================================
  Files          38       38           
  Lines        6210     6253   +43     
  Branches      341      341           
=======================================
+ Hits         6188     6231   +43     
  Misses         15       15           
  Partials        7        7           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[IceCodeNew]

/agentic_review

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 27 rules

1. GeocodingError leaks httpx types 📘 Rule violation ⌂ Architecture ⭐ New

Description

GeocodingError now stores and propagates vendor-specific exception classes (e.g.,
httpx.ConnectError) via cause_type, which exposes provider details beyond adapter boundaries and
couples the rest of the application/tests to httpx. This violates the requirement to isolate
provider-specific implementations behind adapter interfaces.

Code

weather_briefing/geocoding.py[R24-28]

+    def __init__(self, message: str, *, cause_type: type[Exception] | None = None) -> None:
+        """Retain a safe exception class without preserving sensitive error text."""
+        super().__init__(message)
+        self.cause_type = cause_type
+

Relevance

⭐⭐⭐ High

Docs repeatedly enforce provider/SDK isolation boundaries; recent architecture work reinforces
avoiding vendor leakage (PRs #41/#21/#29).

PR-#41
PR-#21
PR-#29

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The checklist requires provider-specific types to remain behind adapter interfaces, but the new
GeocodingError API includes cause_type and is set to type(exc) from httpx exceptions; tests
further depend on httpx.ConnectError, demonstrating leakage beyond a provider boundary.

Rule 2141688: Isolate provider-specific implementations behind adapter interfaces
weather_briefing/geocoding.py[21-28]
weather_briefing/geocoding.py[202-206]
tests/test_geocoding.py[497-506]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`weather_briefing/geocoding.py` exposes provider-specific exception types (`httpx.*`) outside the provider/adapter boundary by storing them in `GeocodingError.cause_type` and asserting on them in tests.

## Issue Context
Compliance requires core/application logic to depend on provider-neutral interfaces and core-owned value types, not vendor SDK types.

## Fix Focus Areas
- weather_briefing/geocoding.py[21-28]
- weather_briefing/geocoding.py[202-206]
- tests/test_geocoding.py[497-506]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Fallback hides failure details ✓ Resolved 🐞 Bug ◔ Observability

Description

FallbackGeocodingProvider.geocode() and PrecisionReducingGeocodingProvider.geocode() always end
their final error with "(GeocodingError)" because they only ever capture GeocodingError instances,
and they raise from None so the most useful provider-level failure details (already sanitized to
include safe exception-type info) are hidden. This makes the hardest-to-debug path—when all
providers/precision-reduction attempts fail—significantly harder to troubleshoot.

Code

weather_briefing/geocoding.py[R294-296]

+        raise GeocodingError(
+            f"No geocoder could resolve location: {location.id} ({type(errors[-1]).__name__})"
+        ) from None

Relevance

⭐⭐⭐ High

Team previously preserved safe provider error details in wrappers despite from None (e.g.,
_safe_provider_error in messages).

PR-#19
PR-#29

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

In both wrappers, the collected error variables are constrained to GeocodingError only (errors
is a list[GeocodingError] and is only populated by catching GeocodingError, and last_error is
assigned only from caught GeocodingError instances), so formatting a suffix using
type(errors[-1]).__name__ or type(last_error).__name__ can never reflect the underlying failure
type and will always be GeocodingError. Additionally, both final raises use from None, which
suppresses exception context and, combined with not including str(errors[-1])/str(last_error) in
the final message, discards the provider-level GeocodingError messages that already preserve safe
diagnostics such as the underlying exception type name (e.g., (... ConnectError)).

weather_briefing/geocoding.py[276-297]
weather_briefing/geocoding.py[160-212]
tests/test_geocoding.py[671-685]
weather_briefing/geocoding.py[299-328]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Both `FallbackGeocodingProvider.geocode()` and `PrecisionReducingGeocodingProvider.geocode()` raise a final `GeocodingError` that appends a suffix based on `type(errors[-1]).__name__` / `type(last_error).__name__`, but those values are always `GeocodingError` because only `GeocodingError` instances are collected. The final raises also use `from None`, which suppresses the last/accumulated provider error messages that already contain safe diagnostics (like the underlying exception type name), making “all attempts failed” scenarios hard to troubleshoot.

## Issue Context
Provider-level errors were updated to avoid leaking raw location names while still preserving useful diagnostics by embedding the underlying exception type name in the `GeocodingError` message (e.g., `(... ConnectError)`), and provider implementations suppress exception chaining to avoid leaking provider exception messages. The fallback and precision-reduction wrappers currently discard these sanitized messages by not including `str(...)` details in their final error and by raising with `from None`, so callers cannot see what the last/each attempt failed with.

## Fix Focus Areas
- weather_briefing/geocoding.py[285-297]
- weather_briefing/geocoding.py[306-328]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit 75c281c

Results up to commit 4ea7cde

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Fallback hides failure details ✓ Resolved 🐞 Bug ◔ Observability

Description

FallbackGeocodingProvider.geocode() and PrecisionReducingGeocodingProvider.geocode() always end
their final error with "(GeocodingError)" because they only ever capture GeocodingError instances,
and they raise from None so the most useful provider-level failure details (already sanitized to
include safe exception-type info) are hidden. This makes the hardest-to-debug path—when all
providers/precision-reduction attempts fail—significantly harder to troubleshoot.

Code

weather_briefing/geocoding.py[R294-296]

+        raise GeocodingError(
+            f"No geocoder could resolve location: {location.id} ({type(errors[-1]).__name__})"
+        ) from None

Relevance

⭐⭐⭐ High

Team previously preserved safe provider error details in wrappers despite from None (e.g.,
_safe_provider_error in messages).

PR-#19
PR-#29

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

In both wrappers, the collected error variables are constrained to GeocodingError only (errors
is a list[GeocodingError] and is only populated by catching GeocodingError, and last_error is
assigned only from caught GeocodingError instances), so formatting a suffix using
type(errors[-1]).__name__ or type(last_error).__name__ can never reflect the underlying failure
type and will always be GeocodingError. Additionally, both final raises use from None, which
suppresses exception context and, combined with not including str(errors[-1])/str(last_error) in
the final message, discards the provider-level GeocodingError messages that already preserve safe
diagnostics such as the underlying exception type name (e.g., (... ConnectError)).

weather_briefing/geocoding.py[276-297]
weather_briefing/geocoding.py[160-212]
tests/test_geocoding.py[671-685]
weather_briefing/geocoding.py[299-328]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Both `FallbackGeocodingProvider.geocode()` and `PrecisionReducingGeocodingProvider.geocode()` raise a final `GeocodingError` that appends a suffix based on `type(errors[-1]).__name__` / `type(last_error).__name__`, but those values are always `GeocodingError` because only `GeocodingError` instances are collected. The final raises also use `from None`, which suppresses the last/accumulated provider error messages that already contain safe diagnostics (like the underlying exception type name), making “all attempts failed” scenarios hard to troubleshoot.

## Issue Context
Provider-level errors were updated to avoid leaking raw location names while still preserving useful diagnostics by embedding the underlying exception type name in the `GeocodingError` message (e.g., `(... ConnectError)`), and provider implementations suppress exception chaining to avoid leaking provider exception messages. The fallback and precision-reduction wrappers currently discard these sanitized messages by not including `str(...)` details in their final error and by raising with `from None`, so callers cannot see what the last/each attempt failed with.

## Fix Focus Areas
- weather_briefing/geocoding.py[285-297]
- weather_briefing/geocoding.py[306-328]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 75c281c

[qodo-code-review]

PR Summary by Qodo

Redact location names from logs and geocoding diagnostics

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Remove location display names from routine INFO processing logs.
• Make geocoding failures diagnostic via location IDs and safe exception types only.
• Update tests and logging requirements to enforce the new redaction rules.

Diagram

graph TD
  CLI["CLI run()"] --> LOG["Logger (INFO/DEBUG)"] --> OUT["Diagnostics output"]
  CLI --> PIPE["Location processing"] --> GEO["Geocoding pipeline"] --> ERR["GeocodingError (safe)" ]
  GEO --> PROV["Providers (Open-Meteo/Nominatim)"] --> ERR
  TESTS["Tests"] --> CLI --> GEO
  DOCS["Docs"] --> LOG

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Central log redaction filter/formatter

• ➕ Enforces redaction consistently without relying on each call site
• ➕ Can be extended to other sensitive fields (URLs, tokens) uniformly
• ➖ Hard to reliably redact arbitrary exception messages without losing useful context
• ➖ Risk of false negatives (missed patterns) or false positives (over-redaction)

2. Structured logging with explicit fields

• ➕ Keeps diagnostics useful via typed fields (location_id, provider, operation, error_type)
• ➕ Easier to audit: sensitive fields can be omitted at INFO by construction
• ➖ Larger refactor if current logging is primarily free-form strings
• ➖ May require downstream log consumers/formatters to adapt

Recommendation: The PR’s approach—removing names at the source of INFO logs and explicitly sanitizing geocoding exceptions (retaining only a safe cause_type)—is the most reliable privacy guarantee with minimal churn. A central redaction filter was considered but is inherently brittle for exception text, and full structured logging would be a broader architectural change than warranted for this fix.

Files changed (5) +104 / -25

Bug fix (2) +48 / -14

cli.pyRedact location names from INFO processing logs +2/-1

Redact location names from INFO processing logs

• Changes per-location processing logs to emit only the location ID at INFO. Moves display name logging to DEBUG to keep routine diagnostics non-sensitive.

weather_briefing/cli.py

geocoding.pySanitize GeocodingError to avoid leaking provider/query text +46/-13

Sanitize GeocodingError to avoid leaking provider/query text

• Extends GeocodingError to carry a safe cause_type and updates providers/wrappers to raise sanitized errors that reference location IDs instead of names. Suppresses exception chaining (from None) to prevent tracebacks from including raw provider exception messages that may embed query text.

weather_briefing/geocoding.py

Tests (2) +55 / -10

test_cli.pyAssert CLI INFO logs omit location names, DEBUG may include them +5/-3

Assert CLI INFO logs omit location names, DEBUG may include them

• Parameterizes the runtime-diagnostics-unavailable test over debug mode. Updates expectations so INFO output contains only the location ID and only includes the display name when debug is enabled.

tests/test_cli.py

test_geocoding.pyAdd privacy-focused geocoding error tests and update message matching +50/-7

Add privacy-focused geocoding error tests and update message matching

• Adds tests verifying fallback and precision-reducing providers preserve only a safe underlying exception type. Introduces a traceback-based test ensuring provider exceptions do not leak private location names, and updates existing tests to match new sanitized error messages.

tests/test_geocoding.py

Documentation (1) +1 / -1

requirements.mdDocument that INFO logs must omit location display names +1/-1

Document that INFO logs must omit location display names

• Tightens the logging requirements to forbid location display names in INFO logs while allowing them under explicit DEBUG. Clarifies that DEBUG logs containing names are location-context-bearing and should not be shared as non-sensitive diagnostics.

docs/requirements.md

[qodo-code-review]

1. Geocodingerror leaks httpx types 📘 Rule violation ⌂ Architecture

GeocodingError now stores and propagates vendor-specific exception classes (e.g.,
httpx.ConnectError) via cause_type, which exposes provider details beyond adapter boundaries and
couples the rest of the application/tests to httpx. This violates the requirement to isolate
provider-specific implementations behind adapter interfaces.

Agent Prompt

## Issue description
`weather_briefing/geocoding.py` exposes provider-specific exception types (`httpx.*`) outside the provider/adapter boundary by storing them in `GeocodingError.cause_type` and asserting on them in tests.

## Issue Context
Compliance requires core/application logic to depend on provider-neutral interfaces and core-owned value types, not vendor SDK types.

## Fix Focus Areas
- weather_briefing/geocoding.py[21-28]
- weather_briefing/geocoding.py[202-206]
- tests/test_geocoding.py[497-506]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[qodo-code-review]

Code review by qodo was updated up to the latest commit 75c281c