fix: bound historical context snapshots - #58
Merged
Conversation
Owner
Author
|
/agentic_review |
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✨ Finishing Touches🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #58 +/- ##
========================================
Coverage 99.71% 99.72%
========================================
Files 38 38
Lines 6418 6546 +128
Branches 363 376 +13
========================================
+ Hits 6400 6528 +128
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. Unbounded citation ID set
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
IceCodeNew
force-pushed
the
codex/bound-llm-context
branch
from
July 17, 2026 03:15
aa686c8 to
56297ac
Compare
Owner
Author
|
/agentic_review |
IceCodeNew
marked this pull request as ready for review
July 17, 2026 03:24
PR Summary by QodoBound LLM historical context snapshots by source and size
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
Comment on lines
277
to
291
| historical_context = self._state.recent_context_documents(now, self._settings.history_hours) | ||
| historical_context_payload, historical_context_characters = _bounded_context_history( | ||
| historical_context, | ||
| max_documents=self._settings.llm_history_max_documents, | ||
| max_characters=self._settings.llm_history_max_characters, | ||
| ) | ||
| _LOGGER.debug( | ||
| "Historical context bounded: input_documents=%d selected_documents=%d serialized_characters=%d", | ||
| len(historical_context), | ||
| len(historical_context_payload), | ||
| historical_context_characters, | ||
| ) | ||
| reference_context = _unique_documents((*historical_context, *context)) | ||
| active_warnings = self._state.active_warnings(now, self._settings.warning_retention_hours) | ||
| if not unpublished_articles and not context and not active_warnings: |
There was a problem hiding this comment.
1. Unbounded citation id set 🐞 Bug ≡ Correctness
BriefingService._run still builds valid_source_ids from the full unbounded historical_context via reference_context, even though only historical_context_payload is sent to the LLM. parse_result therefore can accept model citations to source IDs that were not present in the LLM input, weakening citation integrity when history is bounded.
Agent Prompt
## Issue description
`BriefingService._run()` sends a bounded `recent_context_documents` payload, but still constructs `reference_context`/`valid_source_ids` from the *unbounded* `historical_context`. Output validation in `parse_result()` only checks that citations are within `valid_source_ids`, so citations to sources omitted from the prompt can pass.
## Issue Context
The PR intentionally drops historical snapshots from the payload when exceeding document/character budgets; validation should reflect what the model actually saw.
## Fix Focus Areas
- weather_briefing/service.py[277-329]
### Implementation sketch
- Have `_bounded_context_history(...)` also return the selected `SourceDocument` objects (or at least the set of selected `source_id`s).
- Build `reference_context` (and thus `valid_source_ids`) from `context` plus the *selected* historical documents/IDs, not from all of `historical_context`.
- Keep renderer context consistent with the same bounded set so that citations map to sources actually provided.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+585
to
+626
| changes_by_source: dict[str, list[tuple[int, SourceDocument]]] = {} | ||
| for index, document in enumerate(documents): | ||
| source_changes = changes_by_source.setdefault(document.id, []) | ||
| previous_document = source_changes[-1][1] if source_changes else None | ||
| if previous_document is None or _context_document_value(previous_document) != _context_document_value(document): | ||
| source_changes.append((index, document)) | ||
|
|
||
| source_changes = sorted(changes_by_source.values(), key=lambda changes: changes[-1][0], reverse=True) | ||
| prioritized: list[tuple[int, _HistoricalContextCandidate]] = [] | ||
| selected_indexes: set[int] = set() | ||
|
|
||
| def add( | ||
| snapshot: tuple[int, SourceDocument], | ||
| role: Literal["latest", "retention_baseline", "recent_change"], | ||
| ) -> None: | ||
| if snapshot[0] not in selected_indexes: | ||
| prioritized.append((snapshot[0], _HistoricalContextCandidate(snapshot[1], role))) | ||
| selected_indexes.add(snapshot[0]) | ||
|
|
||
| for changes in source_changes: | ||
| add(changes[-1], "latest") | ||
| for changes in source_changes: | ||
| add(changes[0], "retention_baseline") | ||
|
|
||
| depth = 2 | ||
| while len(prioritized) < max_documents: | ||
| added_change = False | ||
| for changes in source_changes: | ||
| if len(changes) > depth: | ||
| add(changes[-depth], "recent_change") | ||
| added_change = True | ||
| if len(prioritized) == max_documents: | ||
| break | ||
| if not added_change: | ||
| break | ||
| depth += 1 | ||
|
|
||
| return tuple(candidate for _, candidate in prioritized[:max_documents]) | ||
|
|
||
|
|
||
| def _context_document_value(document: SourceDocument) -> tuple[str, str, str]: | ||
| return document.name, document.url, document.content |
There was a problem hiding this comment.
2. Dedupe includes timestamps 🐞 Bug ≡ Correctness
_context_history_candidates treats snapshots as different when (name, url, content) differs, but weather/air-quality documents embed observation/update timestamps in content. This causes otherwise identical hourly snapshots to be treated as changes whenever the timestamp changes, undermining the intended “collapse consecutive identical values” behavior.
Agent Prompt
## Issue description
The dedupe logic compares `document.content` verbatim, but several generated documents include volatile timestamp lines. As a result, dedupe will fail to collapse semantically identical snapshots when only the observation/update time changes.
## Issue Context
The design doc states snapshots should be folded by source on consecutive identical values before selecting latest/baseline/recent changes.
## Fix Focus Areas
- weather_briefing/service.py[585-626]
- weather_briefing/weather_context.py[729-744]
- weather_briefing/air_quality.py[93-112]
### Implementation sketch
- Change `_context_document_value()` to compute a *stable* comparison key (e.g., strip a leading `更新时间：...` / `观测时间：...` line, or otherwise remove known volatile prefixes).
- Prefer comparing semantic fields (or a normalized content body) rather than the fully rendered content string.
- Add/extend tests to cover two snapshots with identical measurements but different timestamps being collapsed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+562
to
+575
| def _bounded_context_history( | ||
| documents: tuple[SourceDocument, ...], | ||
| *, | ||
| max_documents: int, | ||
| max_characters: int, | ||
| ) -> tuple[list[dict[str, object]], int]: | ||
| candidates = _context_history_candidates(documents, max_documents) | ||
| selected: list[dict[str, object]] = [] | ||
| serialized_characters = len("[]") | ||
| for candidate in candidates: | ||
| entry = _serialize_context_document(candidate.document, history_role=candidate.role) | ||
| candidate_payload = [*selected, entry] | ||
| candidate_characters = len(json.dumps(candidate_payload, ensure_ascii=False, separators=(",", ":"))) | ||
| if candidate_characters <= max_characters: |
There was a problem hiding this comment.
3. Budget json mismatch 🐞 Bug ☼ Reliability
_bounded_context_history enforces LLM_HISTORY_MAX_CHARACTERS using a compact JSON serialization
(no spaces via separators=(',', ':')), but the actual LLM request serializes the payload with
default separators that add spaces. This can make the real recent_context_documents JSON exceed
the configured character budget.
Agent Prompt
## Issue description
Character budgeting uses a different JSON serialization format than the actual LLM request, so the enforced budget is not the budget actually sent over the wire.
## Issue Context
Budgeting aims to prevent LLM context-length/timeouts; even small systematic undercounting can matter when near the limit.
## Fix Focus Areas
- weather_briefing/service.py[562-578]
- weather_briefing/llm.py[152-165]
### Implementation sketch
- Use the *same* `json.dumps(..., ensure_ascii=False, separators=(",", ":"))` (or a shared helper) both for:
- computing `candidate_characters` in `_bounded_context_history`, and
- serializing `payload` in `AnyLLMStructuredProvider.summarize()`.
- Optionally adjust the DEBUG size log to use the same serialization to keep metrics consistent.
ⓘ 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 56297ac |
This was referenced Jul 17, 2026
This was referenced Jul 20, 2026
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
HISTORY_HOURSRoot cause
Hourly API snapshots were all copied into every LLM request for the full 48-hour retention window. The incident already carried 16 historical snapshots; without a bound this category continued growing.
Validation
prek run --all-filesuv run --with pytest --with pytest-cov -- pytest --cov --cov-branch --cov-report=xml(612 passed)