fix: ignore unknown resolved warning IDs - #67
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe briefing service now supplies an allowlist of active warning IDs to the LLM, preserves it during retries, and filters unknown resolved IDs after summarization instead of retrying. Prompts, tests, and warning lifecycle documentation describe the updated contract and behavior. ChangesResolved warning handling
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Owner
Author
|
/agentic_review |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #67 +/- ##
=======================================
Coverage 99.74% 99.74%
=======================================
Files 39 39
Lines 7144 7151 +7
Branches 411 411
=======================================
+ Hits 7126 7133 +7
Misses 13 13
Partials 5 5 ☔ View full report in Codecov by Harness. |
Code Review by Qodo
Context used✅ Compliance rules (platform):
31 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
IceCodeNew
force-pushed
the
codex/ignore-unknown-warning-resolutions
branch
from
July 18, 2026 06:15
ed78075 to
5416557
Compare
Owner
Author
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 5416557 |
IceCodeNew
marked this pull request as ready for review
July 18, 2026 06:19
PR Summary by QodoIgnore unknown resolved warning IDs and persist only active resolutions
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Comment on lines
+412
to
+423
| unknown_resolved_warning_ids = set(result.resolved_warning_ids) - active_warning_ids | ||
| if unknown_resolved_warning_ids: | ||
| _LOGGER.warning( | ||
| "Ignoring %d distinct resolved warning ID(s) that are not currently active", | ||
| len(unknown_resolved_warning_ids), | ||
| ) | ||
| result = replace( | ||
| result, | ||
| resolved_warning_ids=tuple( | ||
| warning_id for warning_id in result.resolved_warning_ids if warning_id in active_warning_ids | ||
| ), | ||
| ) |
There was a problem hiding this comment.
1. Raw_payload diverges after filtering 🐞 Bug ⚙ Maintainability
After filtering out unknown resolved warning IDs, the service updates only BriefingResult.resolved_warning_ids but leaves BriefingResult.raw_payload containing the original (unfiltered) resolved_warning_ids. This makes raw_payload inconsistent with the persisted/effective result and unsafe to treat as the canonical post-normalization representation.
Agent Prompt
## Issue description
`BriefingService._run()` filters unknown `resolved_warning_ids` using `dataclasses.replace`, but only updates the typed `resolved_warning_ids` field. `BriefingResult.raw_payload` (which is populated from the original structured LLM response) remains unchanged and can therefore disagree with `result.resolved_warning_ids` after normalization.
## Issue Context
`raw_payload` is part of `BriefingResult` and currently represents the exact parsed model output. Once the service applies post-processing, the object contains two conflicting representations of `resolved_warning_ids`.
## Fix Focus Areas
- weather_briefing/service.py[411-423]
- weather_briefing/llm.py[283-331]
- weather_briefing/models.py[205-218]
## Suggested fix
When filtering unknown IDs, also update `raw_payload["resolved_warning_ids"]` to match the filtered tuple (or explicitly document/rename `raw_payload` to indicate it is intentionally unnormalized and must not be used as a post-processed representation).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 5416557 |
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
1 participant
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
resolved_warning_idsRoot cause
The LLM could infer or recreate a warning ID from historical material even after that ID had left the active warning retention window. Validation treated the no-op unknown ID as a fatal contract error. The repair request did not expose the allowed warning IDs, so repeated responses could exhaust
LLM_MAX_ATTEMPTSand prevent the forecast from being delivered.Production symptom
A scheduled forecast run successfully fetched all configured RSS feeds and completed the primary weather provider's forecast, lifestyle-index, and air-quality requests. After the LLM contract-repair attempts were exhausted, the run failed because
resolved_warning_idscontained an ID that was not currently active. A subsequent Telegram200represented the operational failure alert sent by the exception handler; the user-facing forecast itself never reached the delivery call.This description intentionally omits the location, exact schedule time, feed contents and URLs, recipient details, and the model-generated warning ID.
Impact
Unknown warning IDs are now filtered without changing state, while IDs copied from the active warning allowlist still resolve their matching records. Logs report only the ignored count and do not expose model-generated identifiers.
Validation
uv run --with pytest --with pytest-cov -- pytest --cov --cov-branch --cov-report=xml(664 passed; line rate99.82%, branch rate99.39%)prek runweather_briefing/service.py: 100% statement and branch coverageSummary by CodeRabbit
Bug Fixes
Documentation