fix(source-github): fix silent error swallowing in exception handlers

[devin-ai-integration]

What

Fixes three silent error-swallowing bugs in the source-github connector's exception handling logic.

Resolves https://github.com/airbytehq/oncall/issues/11907

Related: https://github.com/airbytehq/oncall/issues/11869

How

Bug 1: ContributorActivity.read_records() (streams.py ~line 1831)

The else: raise e was paired with the outer if hasattr(...) check, not the inner if status_code == ACCEPTED. When an exception had a valid _exception.response but a non-ACCEPTED status code, execution fell through without re-raising — silently swallowing the error. Fix: Added else: raise e to the inner if block.

Bug 2: GithubStreamABC.read_records() (streams.py ~line 140)

The guard condition used and instead of or:

# Before (broken): if _exception is missing, second hasattr raises AttributeError
if not hasattr(e, "_exception") and not hasattr(e._exception, "response"):
# After (fixed): short-circuits correctly
if not hasattr(e, "_exception") or not hasattr(e._exception, "response"):

Bug 3: Releases._extract_database_id_from_node_id() (streams.py ~line 830)

Replaced bare except Exception: with except (ValueError, struct.error, binascii.Error): — the specific exceptions that base64.urlsafe_b64decode and struct.unpack can raise. Unexpected exceptions (e.g. TypeError) now propagate instead of being silently swallowed.

Note: binascii.Error is a subclass of ValueError, so listing both is technically redundant — kept intentionally to document the distinct exception sources (base64 vs struct).

Review guide

1. source_github/streams.py — all three fixes, plus the import binascii addition
2. unit_tests/test_stream.py — six new tests covering each fix (re-raise behavior + regression for expected catches)
3. metadata.yaml / pyproject.toml — patch version bump 2.1.17 → 2.1.18
4. docs/integrations/sources/github.md — changelog entry

Recommended review checklist:

• Verify the else: raise e indentation in Bug 1 is correctly paired with the inner if (line ~1832). This is the highest-risk change since Python indentation determines control flow — confirm both the new inner else: raise e and the existing outer else: raise e are at the correct levels.
• Confirm or short-circuit logic in Bug 2 is correct
• Confirm (ValueError, struct.error, binascii.Error) covers all realistic failure modes of base64.urlsafe_b64decode + struct.unpack
• Bug 2 tests directly exercise the guard clause by constructing exceptions (a) without _exception and (b) with _exception but without .response — verify both paths through the or

Updates since last revision

• Rebased onto current master (post-PR fix(source-github): fix remaining NameError references to removed MessageRepresentationAirbyteTracedErrors #76080). Version bump is now 2.1.17 → 2.1.18. The MessageRepresentationAirbyteTracedErrors → AirbyteTracedException rename from fix(source-github): fix remaining NameError references to removed MessageRepresentationAirbyteTracedErrors #76080 is incorporated.
• Improved Bug 2 tests per review feedback (P1/P2):
  • test_github_stream_abc_read_records_reraises_when_no_exception_attr — now constructs a bare AirbyteTracedException with _exception deleted, patches HttpStream.read_records to raise it. Directly exercises the first branch of the or guard.
  • test_github_stream_abc_read_records_reraises_when_no_response_attr (renamed) — now constructs an AirbyteTracedException with _exception set to a plain Exception (no .response). Directly exercises the second branch of the or guard.

User Impact

Exceptions that were previously silently swallowed will now be properly raised, making sync failures visible to users instead of silently producing incomplete data.

Can this PR be safely reverted and rolled back?

• YES 💚

Link to Devin session: https://app.devin.ai/sessions/e7b3b318b64e4770a309c7aaadce8de9

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment and CI monitoring

[github-actions]

👋 Greetings, Airbyte Team Member!

Here are some helpful tips and reminders for your convenience.

💡 Show Tips and Tricks

PR Slash Commands

Airbyte Maintainers (that's you!) can execute the following slash commands on your PR:

• 🛠️ Quick Fixes
  • /format-fix - Fixes most formatting issues.
  • /bump-version - Bumps connector versions, scraping changelog description from the PR title.
    • Bump types: patch (default), minor, major, major_rc, rc, promote.
    • The rc type is a smart default: applies minor_rc if stable, or bumps the RC number if already RC.
    • The promote type strips the RC suffix to finalize a release.
    • Example: /bump-version type=rc or /bump-version type=minor
  • /bump-progressive-rollout-version - Alias for /bump-version type=rc. Bumps with an RC suffix and enables progressive rollout.
• ❇️ AI Testing and Review (internal link: AI-SDLC Docs):
  • /ai-prove-fix - Runs prerelease readiness checks, including testing against customer connections.
  • /ai-canary-prerelease - Rolls out prerelease to 5-10 connections for canary testing.
  • /ai-review - AI-powered PR review for connector safety and quality gates.
• 🚀 Connector Releases:
  • /publish-connectors-prerelease - Publishes pre-release connector builds (tagged as {version}-preview.{git-sha}) for all modified connectors in the PR.
• ☕️ JVM connectors:
  • /update-connector-cdk-version connector=<CONNECTOR_NAME> - Updates the specified connector to the latest CDK version.
    Example: /update-connector-cdk-version connector=destination-bigquery
• 🐍 Python connectors:
  • /poe connector source-example lock - Run the Poe lock task on the source-example connector, committing the results back to the branch.
  • /poe source example lock - Alias for /poe connector source-example lock.
  • /poe source example use-cdk-branch my/branch - Pin the source-example CDK reference to the branch name specified.
  • /poe source example use-cdk-latest - Update the source-example CDK dependency to the latest available version.
• ⚙️ Admin commands:
  • /force-merge reason="<REASON>" - Force merges the PR using admin privileges, bypassing CI checks. Requires a reason.
    Example: /force-merge reason="CI is flaky, tests pass locally"

📚 Show Repo Guidance

Helpful Resources

• Breaking Changes Guide - Breaking changes, migration guides, and upgrade deadlines
• Developing Connectors Locally
• Managing Connector Secrets
• On-Demand Regression Tests
• #connector-ci-issues
• #connector-publish-updates
• #connector-build-statuses

📝 Edit this welcome message.

[github-actions]

Deploy preview for airbyte-docs ready!

✅ Preview
https://airbyte-docs-bpwcfid35-airbyte-growth.vercel.app

Built with commit 85d707c.
This pull request is being automatically deployed with vercel-action

[github-actions]

source-github Connector Test Results

107 tests   103 ✅  21s ⏱️
  3 suites    4 💤
  3 files      0 ❌

Results for commit 85d707c.

♻️ This comment has been updated with latest results.

[Patrick Nilan (pnilan)]

API Source PR Review: fix(source-github): fix silent error swallowing in exception handlers

Connector: source-github (Custom Python)
Linked Issue: airbytehq/oncall#11907

Verdict: APPROVED WITH CHANGES

Total Issues: 4 (P0: 1, P1: 1, P2: 1, P3: 1)

---

Issue Analysis

The linked issue accurately identifies three distinct silent error-swallowing bugs in streams.py. All three root cause analyses are correct:

1. ContributorActivity — Indentation-level mismatch causes else: raise e to pair with the outer if instead of the inner if status_code == ACCEPTED. ✅ Correctly diagnosed.
2. GithubStreamABC — and instead of or prevents proper short-circuit evaluation. With and, if e lacks _exception, accessing e._exception in the second operand raises AttributeError. ✅ Correctly diagnosed.
3. Releases — Bare except Exception: catches everything including unexpected errors. ✅ Correctly diagnosed, low severity.

The PR's approach (targeted fixes at each site) is the right strategy. All three code fixes are logically correct.

---

Findings by Priority

P0 - Critical (Must fix before merge)

1. PR has merge conflicts — needs rebase

• Files: metadata.yaml, pyproject.toml, streams.py, docs/integrations/sources/github.md
• Category: Completeness
• Problem: The PR branch is based on a pre-#76080 master. GitHub reports mergeStateStatus: DIRTY, mergeable: CONFLICTING. Specific conflicts:
  • streams.py line ~1814-1815: PR branch still has except MessageRepresentationAirbyteTracedErrors as e: while master already has except AirbyteTracedException as e: (fixed by PR #76080). MessageRepresentationAirbyteTracedErrors no longer exists in the CDK, so the except clause on the PR branch would never match at runtime.
  • Version: Master is already at 2.1.17 (from PR #76080). The PR bumps 2.1.16 → 2.1.17, which conflicts. After rebase, the bump should be 2.1.17 → 2.1.18.
  • Changelog: Master already has a 2.1.17 entry for PR #76080. The PR's 2.1.17 changelog entry will conflict.
• Fix: Rebase onto current master. After rebase:
  1. The MessageRepresentationAirbyteTracedErrors reference will resolve to AirbyteTracedException
  2. Bump version to 2.1.18 in metadata.yaml and pyproject.toml
  3. Update changelog entry to 2.1.18

P1 - High Priority

2. Test for Bug 2 (and → or) may not directly exercise the guard clause

• File: unit_tests/test_stream.py — test_github_stream_abc_read_records_reraises_when_no_exception_attr
• Category: Test Quality
• Problem: This test mocks a 500 response and expects AirbyteTracedException to be raised. However, when the CDK wraps HTTP errors into AirbyteTracedException, it typically sets _exception on the exception object. If _exception IS set with a valid .response, the guard clause (if not hasattr(e, "_exception") or ...) evaluates to False and doesn't fire — the test then relies on the existing else: raise e at line ~188 (SERVER_ERROR for non-WorkflowRuns streams) to propagate, which is pre-existing behavior.
• Evidence: The test docstring says _"should re-raise when AirbyteTracedException has no exception attribute" — but a 500 mock response will likely produce an exception WITH _exception set.
• Suggestion: To directly test the or fix, construct an AirbyteTracedException without _exception and verify it propagates. For example, mock the stream's internal send method to raise a bare AirbyteTracedException() without the _exception attribute.

P2 - Medium Priority

3. Second Bug 2 test exercises pre-existing behavior, not the fix

• File: unit_tests/test_stream.py — test_github_stream_abc_read_records_reraises_for_unhandled_status
• Category: Test Quality
• Problem: This test uses status 422. With 422, the exception will have _exception and _exception.response set, so the guard clause passes through (evaluates to False). The 422 falls through all the if/elif checks and hits the final else: raise e at line ~188. This path already worked on master before the and → or fix. Valid as a regression test, but doesn't exercise the changed code.
• Suggestion: Consider adding a test where _exception exists but _exception.response is None or missing — this would exercise the second part of the or condition.

P3 - Low Priority

4. Redundant exception type in Bug 3 except tuple

• File: streams.py:828
• Category: Code Organization
• Problem: binascii.Error is a subclass of ValueError, so except (ValueError, struct.error, binascii.Error) is technically redundant. However, listing both explicitly documents the intended exception sources.
• Suggestion: No change needed — the explicit listing improves readability.

---

Positive Observations

1. All three fixes are logically correct — The else: raise e indentation in Bug 1, the or short-circuit in Bug 2, and the narrowed exception tuple in Bug 3 are all sound fixes.
2. Good test coverage for Bug 1 and Bug 3 — test_contributor_activity_reraises_non_accepted_status exercises the ContributorActivity path end-to-end, and the Releases tests cover both the "now propagates" and "still catches expected errors" directions.
3. Clean, minimal diff — Tightly scoped to the three reported issues with appropriate version bump, changelog, and docs.
4. Excellent PR description — The review guide with indentation analysis and recommended checklist is very thorough.

---

Review Checklist

• PR resolves all three instances from the linked issue
• All three code fixes are logically correct
• PR needs rebase — merge conflicts due to stale base (P0)
• Follows CDK patterns for custom Python connector
• Unit tests cover Bug 1 and Bug 3 well
• Bug 2 test doesn't directly exercise the and → or fix (P1)
• Error cases tested (401 for ContributorActivity, TypeError for Releases)
• No schema changes
• No breaking changes (behavioral change is intentional — errors now surface instead of being silently swallowed)
• Version bump appropriate (patch for bug fix)
• Version needs update after rebase (2.1.17 → 2.1.18)

[devin-ai-integration]

Thanks for the thorough review! I've addressed all findings:

P0 — Rebase + version conflicts: Rebased onto current master (which includes PR #76080). Version is now 2.1.18 in metadata.yaml, pyproject.toml, and changelog. The MessageRepresentationAirbyteTracedErrors → AirbyteTracedException rename from #76080 is now incorporated.

P1 — Test for Bug 2 doesn't exercise the guard clause: Rewrote test_github_stream_abc_read_records_reraises_when_no_exception_attr to directly construct an AirbyteTracedException without _exception attribute (by delattr-ing it after construction, since CDK sets _exception=None by default). The test patches HttpStream.read_records to raise this bare exception, which now directly exercises the first branch of the or condition.

P2 — Second Bug 2 test exercises pre-existing behavior: Rewrote test_github_stream_abc_read_records_reraises_for_unhandled_status → test_github_stream_abc_read_records_reraises_when_no_response_attr. Now constructs an AirbyteTracedException with _exception set to a plain Exception (no .response attribute), directly exercising the second branch of the or condition.

P3 — Redundant binascii.Error: Acknowledged — keeping it for explicit documentation of intended exception sources, as you suggested.

All 64 tests pass locally (excluding the pre-existing test_stream_projects_v2_graphql_retry failure on master).

Link to Devin session: https://app.devin.ai/sessions/e7b3b318b64e4770a309c7aaadce8de9