Discriminate WG21 index refresh failures and propagate stale signal t…#76
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds typed WG21 refresh outcomes and categorized refresh failures, updates scheduler staleness handling, and expands protocol and integration tests for the new return shapes and error paths. ChangesWG21 Index Refresh Error Discrimination
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/paperscout/sources.py`:
- Around line 99-107: The code currently accepts any dict as a successful
response, including empty dicts which should be rejected. In the
isinstance(data, dict) check, add an additional condition to verify the dict is
not empty before returning it as successful. If the data is an empty dict, treat
it the same as a non-dict response by logging a warning with
FailureCategory.CONFIGURATION and returning None, FailureCategory.CONFIGURATION
instead, to prevent empty payloads from overwriting the cache.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e6c6db1-416a-4e62-a98f-ca48d5cb20cc
📒 Files selected for processing (8)
src/paperscout/errors.pysrc/paperscout/models.pysrc/paperscout/monitor.pysrc/paperscout/sources.pytests/test_callback_protocols.pytests/test_datasource_protocol.pytests/test_monitor.pytests/test_sources.py
Collaborator
Author
|
@coderabbitai full review again |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/test_sources.py (1)
264-272: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a malformed-JSON regression for the terminal config path.
The no-data test covers non-dict JSON, but not
resp.json()raisingValueError; that branch determines whether no-cache refresh becomesConfigurationErroror retryableIndexRefreshError.🧪 Proposed test
async def test_refresh_no_data_raises_configuration_error(self, fake_pool): index = WG21Index(fake_pool) mock_resp = _make_response(200, json_data=[1, 2, 3]) mock_client = _make_async_client(get_resp=mock_resp) with patch("paperscout.sources.httpx.AsyncClient") as mock_cls: mock_cls.return_value.__aenter__ = AsyncMock(return_value=mock_client) mock_cls.return_value.__aexit__ = AsyncMock(return_value=False) with pytest.raises(ConfigurationError, match="No index data available"): await index.refresh() + + async def test_refresh_malformed_json_raises_configuration_error(self, fake_pool): + index = WG21Index(fake_pool) + mock_resp = _make_response(200, json_data=SAMPLE_INDEX_DATA) + mock_resp.json = MagicMock(side_effect=ValueError("invalid json")) + mock_client = _make_async_client(get_resp=mock_resp) + with patch("paperscout.sources.httpx.AsyncClient") as mock_cls: + mock_cls.return_value.__aenter__ = AsyncMock(return_value=mock_client) + mock_cls.return_value.__aexit__ = AsyncMock(return_value=False) + with pytest.raises(ConfigurationError, match="No index data available"): + await index.refresh()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_sources.py` around lines 264 - 272, Add a regression test for malformed JSON handling in the WG21Index refresh flow. Currently, test_refresh_no_data_raises_configuration_error covers non-dict JSON responses, but not the case where resp.json() raises a ValueError due to malformed JSON. Create a new test (or extend the existing one) that mocks the async response object to raise a ValueError when .json() is called (using AsyncMock with side_effect), then verify that index.refresh() still raises ConfigurationError with an appropriate error message. This ensures the error path that determines whether no-cache refresh becomes ConfigurationError versus retryable IndexRefreshError is properly covered.tests/test_monitor.py (1)
812-827: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover stale fallback after the scheduler is already seeded.
This test currently hits the cold-start seed early return, so it does not exercise the normal poll completion path where
_advance_staleness_clock_if_ok()runs after diffing and notifications. Add a second stale poll or pre-seed the scheduler to cover that path.Example extension
assert "P2300R10" in wg21.papers assert "INDEX-STALE" in caplog.text assert scheduler._last_successful_poll is None prober.fetch.assert_called_once() + + prober.fetch.reset_mock() + with patch("paperscout.sources.httpx.AsyncClient") as mock_cls: + mock_cls.return_value.__aenter__ = AsyncMock(return_value=mock_client) + mock_cls.return_value.__aexit__ = AsyncMock(return_value=False) + await scheduler.poll_once() + assert scheduler._last_successful_poll is None + prober.fetch.assert_called_once()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_monitor.py` around lines 812 - 827, The test_poll_once_uses_stale_index test currently exits early during the cold-start seed phase, so it does not exercise the normal poll completion path where _advance_staleness_clock_if_ok() runs. To cover this path, add a second await scheduler.poll_once() call after the first one (before the assertions) to ensure the scheduler is already seeded and initialized, so the second poll attempt will go through the full completion path instead of taking the cold-start early return.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/paperscout/sources.py`:
- Around line 59-63: The code checks if cached data is not None but doesn't
validate that it contains actual entries. An empty dictionary from
`_cache.read_if_fresh()` will pass the `is not None` check and result in
`_parse_and_index(cached)` returning empty papers while still returning
`stale=False`, which incorrectly advances downstream staleness clocks. Modify
the condition that checks `if cached is not None` to also verify that cached is
not empty (has length greater than zero) before proceeding with
`_parse_and_index(cached)` and returning the IndexRefreshResult with
stale=False. This fix needs to be applied in both locations mentioned: the
cached read_if_fresh path and the also applies section around line 72-79.
- Around line 136-143: The except clause in the WG21 index fetching logic
currently catches both httpx.HTTPError and ValueError together and classifies
both as NETWORK failures, but ValueError from malformed JSON should be treated
as a CONFIGURATION failure instead. Split the exception handling into two
separate except clauses: keep httpx.HTTPError with FailureCategory.NETWORK
classification, and add a new except clause specifically for ValueError that
logs and returns FailureCategory.CONFIGURATION to properly surface parse
failures as terminal configuration errors rather than retryable network errors.
In `@tests/test_monitor.py`:
- Around line 872-874: The test for the fatal-path in scheduler.run_forever()
lacks a shutdown mechanism to prevent hanging if the expected ConfigurationError
path regresses. Replace the AsyncMock() for the patched asyncio.sleep with a
mock that raises an exception instead of returning immediately. This ensures
that if the code path ever reaches the sleep call (indicating the
ConfigurationError was not properly raised), the test will fail immediately with
an exception rather than entering an infinite loop.
---
Nitpick comments:
In `@tests/test_monitor.py`:
- Around line 812-827: The test_poll_once_uses_stale_index test currently exits
early during the cold-start seed phase, so it does not exercise the normal poll
completion path where _advance_staleness_clock_if_ok() runs. To cover this path,
add a second await scheduler.poll_once() call after the first one (before the
assertions) to ensure the scheduler is already seeded and initialized, so the
second poll attempt will go through the full completion path instead of taking
the cold-start early return.
In `@tests/test_sources.py`:
- Around line 264-272: Add a regression test for malformed JSON handling in the
WG21Index refresh flow. Currently,
test_refresh_no_data_raises_configuration_error covers non-dict JSON responses,
but not the case where resp.json() raises a ValueError due to malformed JSON.
Create a new test (or extend the existing one) that mocks the async response
object to raise a ValueError when .json() is called (using AsyncMock with
side_effect), then verify that index.refresh() still raises ConfigurationError
with an appropriate error message. This ensures the error path that determines
whether no-cache refresh becomes ConfigurationError versus retryable
IndexRefreshError is properly covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ed944b6-b15b-4e7a-8401-9bf606446a8b
📒 Files selected for processing (8)
src/paperscout/errors.pysrc/paperscout/models.pysrc/paperscout/monitor.pysrc/paperscout/sources.pytests/test_callback_protocols.pytests/test_datasource_protocol.pytests/test_monitor.pytests/test_sources.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/paperscout/sources.py`:
- Around line 60-70: The cache/download success path in `PaperScoutSource` can
still yield zero parsed papers because `_parse_and_index()` filters out non-dict
entries, so validate the parsed result before returning `IndexRefreshResult(...,
stale=False)` or calling `_cache.write()`. In `sources.py`, use the `cached` and
`raw` handling around `_download()`, `_parse_and_index()`, and `_cache.write()`
to only mark success and overwrite the persisted cache when the parsed paper set
is non-empty; otherwise keep the existing cache and treat the payload as
unusable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e034380-434c-46cf-8961-e206df405c49
📒 Files selected for processing (3)
src/paperscout/sources.pytests/test_monitor.pytests/test_sources.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_monitor.py
- tests/test_sources.py
…lid entry dicts, so len(cached) > 0 / if raw: was not enough to guarantee usable index data.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/paperscout/sources.py`:
- Around line 61-84: The current refresh flow in sources.py lets
_parse_and_index() mutate index metadata while testing cached/downloaded
payloads, which can leave self.papers out of sync if a candidate payload is
rejected or raises. Update the refresh logic around _parse_and_index(),
_download(), and the stale-cache fallback so parsing happens against local
temporary state first, then commit self.papers and the derived frontier metadata
only after a payload is accepted. Keep _max_rev, _max_p, and _sorted_p_nums
unchanged unless the new payload is successfully selected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0122bac0-43f0-4669-9817-9c1a2bb71602
📒 Files selected for processing (2)
src/paperscout/sources.pytests/test_sources.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_sources.py
…utated _max_rev, _max_p, and _sorted_p_nums before acceptance — leaving frontier metadata out of sync with self.papers when a payload was rejected.
- Added a second await scheduler.poll_once() after resetting the ISO prober mock, asserting _last_successful_poll stays None on the incremental poll path.
- Added test_refresh_malformed_json_raises_configuration_error asserting refresh() raises ConfigurationError("No index data available") when resp.json() raises ValueError with no cache.
wpak-ai
approved these changes
Jun 24, 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
2 participants
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
IndexRefreshErrorandIndexRefreshResultsoWG21Index.refresh()/fetch()distinguish transient download failures (TIMEOUT,RATE_LIMIT,NETWORK) from permanent ones (CONFIGURATION).IndexRefreshResult(papers, stale=True)on stale cache fallback instead of silently returning an empty or in-memory dict.ConfigurationErrorwhen no index data is available (e.g. non-dict JSON with no cache).Schedulerto unwrapIndexRefreshResult, logINDEX-STALE, skip advancing_last_successful_pollon stale index, and handleIndexRefreshErrorinrun_forever()(retry next cycle).Test plan
pytest— full suite (392 passed)ruff check/ruff format --checkmypyonprotocols,monitor,__main__test_sources.pyrun_forever()behavior intest_monitor.pyRelated Issue
Summary by CodeRabbit
Bug Fixes
Tests