fix(source-github): fix remaining NameError references to removed MessageRepresentationAirbyteTracedErrors#76080
Conversation
…sageRepresentationAirbyteTracedErrors Co-Authored-By: bot_apk <apk@cognition.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Co-Authored-By: bot_apk <apk@cognition.ai>
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksPR Slash CommandsAirbyte Maintainers (that's you!) can execute the following slash commands on your PR:
📚 Show Repo GuidanceHelpful Resources
|
|
|
Deploy preview for airbyte-docs ready! ✅ Preview Built with commit 50e1e31. |
|
↪️ Triggering Reason: CI is green (36 passed, 0 failed). Follow-up fix for remaining |
|
🔬
|
| Check | Result |
|---|---|
| Viability | ✅ Fix correctly replaces undefined class in 2 locations (streams.py:1814, test_stream.py:1616) |
| Safety | ✅ No suspicious code, straightforward 2-line fix |
| Breaking Change | ✅ NOT breaking — patch version bump (2.1.16 → 2.1.17), no schema/spec/state changes |
| Reversibility | ✅ Safely reversible, no state format changes |
Evidence
📊 Regression Test Results (PASSED)
Workflow: Connector Regression Test
Duration: ~22 minutes (11:44 → 12:06 UTC)
Conclusion: ✅ Success
All connector operations passed with the pre-release version:
| Operation | Result |
|---|---|
| SPEC | ✅ Passed |
| CHECK | ✅ Passed |
| DISCOVER | ✅ Passed |
| READ | ✅ Passed |
No regressions detected compared to the baseline version.
🔍 Root Cause Analysis
PR #76038 removed the MessageRepresentationAirbyteTracedErrors class but missed updating 2 references:
streams.py:1814—except MessageRepresentationAirbyteTracedErrorsinContributorActivity.read_recordstest_stream.py:1616—pytest.raises(MessageRepresentationAirbyteTracedErrors)in the corresponding test
This PR replaces both with AirbyteTracedException, which is already imported in both files.
🔄 Live Connection Testing (Pending Approval)
2 candidate connections with contributor_activity stream were identified (details in oncall issue).
Approval requested via Slack (#human-in-the-loop) but not yet received.
Live testing would provide additional confidence but is not strictly required — the regression tests already demonstrate the fix works correctly across all connector operations.
|
|
↪️ Triggering Reason: CI is green (36 passed, 0 failed). Prove-fix validated (regression tests passed). Next pipeline step is AI review. |
Reviewing PR for connector safety and quality.
|
AI PR Review ReportReview Action: NO ACTION (NOT ELIGIBLE)
📋 PR Details & EligibilityConnector & PR InfoConnector(s): Auto-Approve EligibilityEligible: No Review Action DetailsNO ACTION (NOT ELIGIBLE) - All gates pass but PR is not eligible for auto-approval because it contains functional code changes. No PR review submitted. Human review required.
🔍 Gate Evaluation DetailsGate-by-Gate Analysis
Test Coverage Evidence
📚 Evidence ConsultedEvidence
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if hasattr(e, "_exception") and hasattr(e._exception, "response"): | ||
| if e._exception.response.status_code == requests.codes.ACCEPTED: | ||
| yield AirbyteMessage( |
There was a problem hiding this comment.
🔴 Non-ACCEPTED AirbyteTracedException with _exception.response is silently swallowed
In ContributorActivity.read_records(), the else: raise e on line 1831 is paired with the outer if on line 1815, not the inner if on line 1816. This means when an AirbyteTracedException has a valid _exception.response (outer if is True) but the status code is not requests.codes.ACCEPTED (inner if is False), the exception silently falls through without being re-raised.
This is reachable: super().read_records() calls GithubStreamABC.read_records() (streams.py:128), which catches AirbyteTracedException and re-raises for 401 Unauthorized (line 171) and other unhandled status codes (line 188). These re-raised exceptions are then caught by ContributorActivity at line 1814, pass the outer hasattr check, fail the inner ACCEPTED check, and are silently swallowed — meaning authentication errors and unexpected HTTP failures for ContributorActivity are completely ignored.
(Refers to lines 1815-1832)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good observation about the else: raise e being paired with the outer if (line 1815) rather than the inner if (line 1816). The indentation confirms this — when _exception.response exists but the status code is not ACCEPTED, the exception does fall through without being re-raised.
However, this is pre-existing behavior that existed before this PR. This PR's scope is strictly limited to replacing the 2 missed MessageRepresentationAirbyteTracedErrors references with AirbyteTracedException — the same mechanical change that was already applied to the first occurrence in GithubStreamABC.read_records by #76038. No logic was changed.
This potential silent-swallowing issue could be worth filing as a separate issue to investigate, but fixing it here would expand the scope beyond the targeted bug fix for the NameError.
Anatolii Yatsuk (tolik0)
deleted the
devin/1775244830-fix-source-github-name-error
branch
3 participants
What
Resolves https://github.com/airbytehq/oncall/issues/11869:
PR #76038 replaced the deprecated
MessageRepresentationAirbyteTracedErrorswithAirbyteTracedExceptioninGithubStreamABC.read_recordsbut missed two other occurrences, causing aNameErrorat runtime in theContributorActivitystream.How
Replaced the remaining two references to the removed
MessageRepresentationAirbyteTracedErrors:source_github/streams.pyline 1814:except MessageRepresentationAirbyteTracedErrors→except AirbyteTracedExceptioninContributorActivity.read_records(the production error)unit_tests/test_stream.pyline 1616:pytest.raises(MessageRepresentationAirbyteTracedErrors)→pytest.raises(AirbyteTracedException)intest_stream_projects_v2_graphql_retryAirbyteTracedExceptionwas already imported in both files — no new imports needed.Review guide
source_github/streams.py— the production fix (2-line change: comment + except clause)unit_tests/test_stream.py— matching test fix (1-line change)metadata.yaml/pyproject.toml— patch version bump 2.1.16 → 2.1.17docs/integrations/sources/github.md— changelog entry for 2.1.17Human review checklist
AirbyteTracedExceptionis the correct replacement for theexceptclause inContributorActivity.read_records— specifically that the attributes accessed one(e.g.,e._exception,e._exception.response) are present onAirbyteTracedExceptioninstances thrown by the HTTP client. The parent classGithubStreamABC.read_recordsalready uses this same pattern after PR 76038.User Impact
Fixes
NameErrorcrash when syncing theContributorActivitystream, which was broken by the incomplete migration in v2.1.16.Can this PR be safely reverted and rolled back?
Link to Devin session: https://app.devin.ai/sessions/40d065ac191b4270a84d023c4ed3bd75