fix: wire schema drift errors and constrain notification triggers - #35
Conversation
… constrain trigger_event [W1,W2]
W1: the control layer's SCHEMA_DRIFT chain (error_kinds -> evaluator ->
policies -> actuator) runs every 60s but control/recorder.py only records
an error kind when error_type is not None. Of 4 parse-failure sites, only
crawl4ai_channel.py passed error_type correctly. Wire the other 3:
- rss_channel.py: both collect() and fetch()'s feedparser bozo-flag branch
now pass error_type via a new _bozo_error_type() helper, using feedparser's
real bozo_exception class name (verified empirically: SAXParseException
across malformed markup, truncated declarations, and encoding mismatches),
falling back to "ParseError" when none is attached.
- cli_channel.py: the json.JSONDecodeError branch now passes
error_type=type(exc).__name__ ("JSONDecodeError", already mapped).
- error_kinds.py: added "SAXParseException" -> ErrorKind.SCHEMA_DRIFT since
it wasn't in the map yet (minimal extension of the existing taxonomy).
W2: NotificationRule.trigger_event was a free-text str but
dispatch_notifications() only ever queries/fires "on_new_record" (its
default and only caller-supplied value) -- a rule saved with any other
value became permanently, silently inert with no producer. Constrained
trigger_event to Literal["on_new_record"] in NotificationRuleCreate/Update
(backend/schemas/notification.py) so the API rejects an unsupported value
loudly instead of persisting a dead rule. DB column stays String (no
migration) -- this is a validation-layer guard only.
|
✅ Health: 9.4 📋 At a glance 🚨 Change risk: 9.5/10 (high)
🔎 More signals (1)🔥 Hotspots touched (2)
👀 Suggested reviewers @xujinghua 📊 Full report · ⭐ Star Repowise · 📥 Install bot · Last updated 2026-07-19 03:22 UTC |
|
Warning Review limit reached
Next review available in: 32 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCLI and RSS channel failures now preserve structured error types for schema-drift classification. Channel tests were reorganized and expanded across execution, allowlists, parsing, HTTP statuses, and entry mapping. Notification schemas now restrict trigger events to the supported value. ChangesChannel error classification
Notification schema validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves error classification and schema validation across channels and notifications. Specifically, it ensures that JSON decode errors in the CLI channel and parse exceptions (like SAXParseException) in the RSS channel propagate their specific error types to trigger the SCHEMA_DRIFT pipeline instead of being silently dropped. It also constrains the notification rule trigger_event to Literal["on_new_record"] to prevent inert rules. The unit tests have been thoroughly reorganized and expanded to cover these changes. Review feedback suggests ensuring bozo_exception is an instance of Exception before checking its type, and using an 'or' fallback when retrieving the exception message to avoid displaying None if the attribute is explicitly set to None.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| bozo_exception = getattr(parsed, "bozo_exception", None) | ||
| if bozo_exception is not None: | ||
| return type(bozo_exception).__name__ |
There was a problem hiding this comment.
To ensure robust error classification and prevent unexpected type names (e.g., if bozo_exception is mocked as a string or set to a non-exception type), it is safer to explicitly verify that bozo_exception is an instance of Exception before accessing its class name.
| bozo_exception = getattr(parsed, "bozo_exception", None) | |
| if bozo_exception is not None: | |
| return type(bozo_exception).__name__ | |
| bozo_exception = getattr(parsed, "bozo_exception", None) | |
| if isinstance(bozo_exception, Exception): | |
| return type(bozo_exception).__name__ |
| if parsed.bozo and not parsed.entries: | ||
| return ChannelResult.fail( | ||
| f"Failed to parse feed: {getattr(parsed, 'bozo_exception', 'unknown error')}" | ||
| f"Failed to parse feed: {getattr(parsed, 'bozo_exception', 'unknown error')}", |
There was a problem hiding this comment.
If parsed.bozo_exception is explicitly set to None, getattr(parsed, 'bozo_exception', 'unknown error') will return None rather than the default 'unknown error', resulting in a confusing error message like Failed to parse feed: None. Using or ensures we fall back to 'unknown error' in this case.
| f"Failed to parse feed: {getattr(parsed, 'bozo_exception', 'unknown error')}", | |
| f"Failed to parse feed: {getattr(parsed, 'bozo_exception', None) or 'unknown error'}", |
| if parsed.bozo and not parsed.entries: | ||
| raise ChannelFetchError( | ||
| f"Failed to parse feed: {getattr(parsed, 'bozo_exception', 'unknown error')}" | ||
| f"Failed to parse feed: {getattr(parsed, 'bozo_exception', 'unknown error')}", |
There was a problem hiding this comment.
If parsed.bozo_exception is explicitly set to None, getattr(parsed, 'bozo_exception', 'unknown error') will return None rather than the default 'unknown error', resulting in a confusing error message like Failed to parse feed: None. Using or ensures we fall back to 'unknown error' in this case.
| f"Failed to parse feed: {getattr(parsed, 'bozo_exception', 'unknown error')}", | |
| f"Failed to parse feed: {getattr(parsed, 'bozo_exception', None) or 'unknown error'}", |
![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 `@tests/unit/channels/test_cli_channel_allowlist.py`:
- Around line 91-108: Update
test_allowlist_rejection_permanent_through_fetch_seam to assert that
effective_error_type(excinfo.value) equals the expected permanent allowlist
rejection type, rather than only checking is_retryable(...) is False. Retain the
existing fetch invocation and use the established error-taxonomy symbol for the
exact type.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 274bb22e-1705-4eed-bade-1ca57c45cedf
📒 Files selected for processing (15)
backend/channels/cli_channel.pybackend/channels/rss_channel.pybackend/control/error_kinds.pybackend/schemas/notification.pytests/unit/channels/test_cli_channel.pytests/unit/channels/test_cli_channel_allowlist.pytests/unit/channels/test_cli_channel_execution.pytests/unit/channels/test_rss_channel.pytests/unit/channels/test_rss_channel_entries.pytests/unit/channels/test_rss_channel_errors.pytests/unit/channels/test_rss_channel_schema_drift.pytests/unit/channels/test_rss_fetch.pytests/unit/channels/test_rss_fetch_statuses.pytests/unit/control/test_error_kinds.pytests/unit/test_schemas_notification.py
| @pytest.mark.asyncio | ||
| async def test_allowlist_rejection_permanent_through_fetch_seam(channel): | ||
| """The fetch seam preserves the permanent allowlist rejection.""" | ||
| from backend.channels.base import ChannelFetchError, FetchContext | ||
| from backend.pipeline.error_taxonomy import effective_error_type, is_retryable | ||
|
|
||
| with _allow(): | ||
| with pytest.raises(ChannelFetchError) as excinfo: | ||
| await channel.fetch( | ||
| FetchContext( | ||
| config={ | ||
| "binary": sys.executable, | ||
| "command": ["-c", "print(1)"], | ||
| }, | ||
| params={}, | ||
| ) | ||
| ) | ||
| assert is_retryable(effective_error_type(excinfo.value)) is False |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert that the fetch seam preserves the exact error type.
is_retryable(...) is False also passes for None or unknown types, so this test would miss the regression it claims to cover.
Proposed fix
- assert is_retryable(effective_error_type(excinfo.value)) is False
+ error_type = effective_error_type(excinfo.value)
+ assert error_type == "BinaryNotAllowedError"
+ assert is_retryable(error_type) is False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.mark.asyncio | |
| async def test_allowlist_rejection_permanent_through_fetch_seam(channel): | |
| """The fetch seam preserves the permanent allowlist rejection.""" | |
| from backend.channels.base import ChannelFetchError, FetchContext | |
| from backend.pipeline.error_taxonomy import effective_error_type, is_retryable | |
| with _allow(): | |
| with pytest.raises(ChannelFetchError) as excinfo: | |
| await channel.fetch( | |
| FetchContext( | |
| config={ | |
| "binary": sys.executable, | |
| "command": ["-c", "print(1)"], | |
| }, | |
| params={}, | |
| ) | |
| ) | |
| assert is_retryable(effective_error_type(excinfo.value)) is False | |
| `@pytest.mark.asyncio` | |
| async def test_allowlist_rejection_permanent_through_fetch_seam(channel): | |
| """The fetch seam preserves the permanent allowlist rejection.""" | |
| from backend.channels.base import ChannelFetchError, FetchContext | |
| from backend.pipeline.error_taxonomy import effective_error_type, is_retryable | |
| with _allow(): | |
| with pytest.raises(ChannelFetchError) as excinfo: | |
| await channel.fetch( | |
| FetchContext( | |
| config={ | |
| "binary": sys.executable, | |
| "command": ["-c", "print(1)"], | |
| }, | |
| params={}, | |
| ) | |
| ) | |
| error_type = effective_error_type(excinfo.value) | |
| assert error_type == "BinaryNotAllowedError" | |
| assert is_retryable(error_type) is False |
🤖 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/unit/channels/test_cli_channel_allowlist.py` around lines 91 - 108,
Update test_allowlist_rejection_permanent_through_fetch_seam to assert that
effective_error_type(excinfo.value) equals the expected permanent allowlist
rejection type, rather than only checking is_retryable(...) is False. Retain the
existing fetch invocation and use the established error-taxonomy symbol for the
exact type.
…ft-notify # Conflicts: # tests/unit/channels/test_cli_channel.py
1 participant
What changed
error_typevalues from RSS bozo failures and CLI JSON decode failuresSAXParseExceptiontoSCHEMA_DRIFTon_new_recordtriggerWhy
The control recorder can only emit schema-drift events when a structured error type reaches it. Several parse failure paths discarded that signal. Notification dispatch currently produces only
on_new_record, so accepting other new trigger strings creates inert rules.Scope
This PR prevents new unsupported
trigger_eventvalues through the create/update schemas. It does not migrate or reject already-persisted invalid rows at read time.HTTP 200 responses containing HTML or an empty body can still produce
bozo=Falsewith zero entries infeedparser; that separate detection gap is tracked as W1b rather than being folded into this wiring fix.Validation
1823 passed, 11 skipped(90.27%coverage)73 passedRuntimeWarningpromoted to an errorgit diff --checkpassed