add force notify option #349

joseph-sentry · 2025-07-24T20:37:59Z

No description provided.

joseph-sentry · 2025-07-24T20:38:08Z

seer-by-sentry · 2025-07-24T20:42:19Z

apps/worker/services/notification/notifiers/checks/base.py

+            if (
+                comparison.enriched_pull is None
+                or comparison.enriched_pull.provider_pull is None
+            ):
+                log.debug(


Potential bug: Bypassing `comparison.enriched_pull.provider_pull` validation with `force_notify=True` can lead to `AttributeError` when accessing `provider_pull` attributes (e.g., `["author"]`, `["head"]`) without null checks, causing notification process crashes.

Description: When "force_notify=True", the validation for comparison.enriched_pull.provider_pull being None is bypassed. This allows code execution to proceed even if provider_pull is None. Later, methods like get_upgrade_message in apps/worker/services/notification/notifiers/checks/base.py (e.g., line 59: author_username = comparison.enriched_pull.provider_pull["author"].get("username")) and _create_upgrade_message in apps/worker/services/notification/notifiers/comment/__init__.py (e.g., line 391) directly access provider_pull["author"] or provider_pull["head"] without null checks. If comparison.enriched_pull.provider_pull is None, attempting to access ["author"] or ["head"] will raise an AttributeError: 'NoneType' object has no attribute '__getitem__', leading to an unhandled exception and a crash in the notification process.

Suggested fix: Reinstate the comparison.enriched_pull.provider_pull is None validation when force_notify=True or add explicit null checks for comparison.enriched_pull.provider_pull before accessing its attributes in methods like get_upgrade_message and _create_upgrade_message.
_{severity: 0.9, confidence: 0.95}

_{Did we get this right? 👍 / 👎 to inform future reviews.}

seer-by-sentry · 2025-07-24T20:42:22Z

apps/worker/services/notification/notifiers/comment/__init__.py

+            for condition in self.notify_conditions:
+                condition_result = condition.check_condition(
+                    notifier=self, comparison=comparison
                )
-                return default_result.merge(side_effect_result)
+                if condition_result == False:


Potential bug: Bypassing `comparison.pull` and `comparison.enriched_pull.provider_pull` validation in the comment notifier with `force_notify=True` can cause `AttributeError` when accessing their attributes, leading to crashes.

Description: When "force_notify=True", the comment notifier bypasses validation checks like ComparisonHasPull (ensuring comparison.pull is not None) and PullRequestInProvider (ensuring comparison.enriched_pull.provider_pull is not None). This allows execution to reach build_message() (e.g., line 427) where comparison.pull.is_first_coverage_pull is accessed, or comparison.enriched_pull.provider_pull (line 428) is accessed directly. Similarly, pull.pullid (line 134) and comparison.enriched_pull.provider_pull["author"] in _create_upgrade_message() are accessed without null checks. If the bypassed objects are None, these accesses will raise an AttributeError (e.g., 'NoneType' object has no attribute 'is_first_coverage_pull'), leading to an unhandled exception and a crash in the notification process.

Suggested fix: Reinstate ComparisonHasPull and PullRequestInProvider validations in the comment notifier when force_notify=True, or add explicit null checks for comparison.pull and comparison.enriched_pull.provider_pull before accessing their attributes.
_{severity: 0.9, confidence: 0.95}

_{Did we get this right? 👍 / 👎 to inform future reviews.}

seer-by-sentry · 2025-07-24T20:42:24Z

apps/worker/services/notification/notifiers/checks/base.py

+        if not force_notify:
+            if comparison.pull is None or ():
+                log.debug(
+                    "Falling back to commit_status: Not a pull request",
+                    extra={
+                        "notifier": self.name,
+                        "repoid": comparison.head.commit.repoid,
+                        "notifier_title": self.title,
+                        "commit": comparison.head.commit,
+                    },
+                )
+                return NotificationResult(
+                    notification_attempted=False,
+                    notification_successful=None,
+                    explanation="no_pull_request",
+                    data_sent=None,
+                    data_received=None,
+                )
+            if (
+                comparison.enriched_pull is None
+                or comparison.enriched_pull.provider_pull is None
+            ):
+                log.debug(
+                    "Falling back to commit_status: Pull request not in provider",
+                    extra={
+                        "notifier": self.name,
+                        "repoid": comparison.head.commit.repoid,
+                        "notifier_title": self.title,
+                        "commit": comparison.head.commit,
+                    },
+                )
+                return NotificationResult(
+                    notification_attempted=False,
+                    notification_successful=None,
+                    explanation="pull_request_not_in_provider",
+                    data_sent=None,
+                    data_received=None,
+                )
+            if comparison.pull and comparison.pull.state != "open":
+                log.debug(
+                    "Falling back to commit_status: Pull request closed",
+                    extra={
+                        "notifier": self.name,
+                        "repoid": comparison.head.commit.repoid,
+                        "notifier_title": self.title,
+                        "commit": comparison.head.commit,
+                    },
+                )
+                return NotificationResult(
+                    notification_attempted=False,
+                    notification_successful=None,
+                    explanation="pull_request_closed",
+                    data_sent=None,
+                    data_received=None,
+                )
+            # Check branches config for this status before sending the check
+            if not self.can_we_set_this_status(comparison):
+                return NotificationResult(
+                    notification_attempted=False,
+                    notification_successful=None,
+                    explanation="not_fit_criteria",
+                    data_sent=None,
+                )
+            if not self.required_builds(comparison):
+                return NotificationResult(
+                    notification_attempted=False,
+                    notification_successful=None,
+                    explanation="need_more_builds",
+                    data_sent=None,
+                    data_received=None,
+                )
+            # Check for existing statuses for this commit. If so, retain
+            # statuses and don't do a check as well
+            statuses = comparison.get_existing_statuses()
+            status_title = self.get_status_external_name()
+            if statuses and statuses.get(status_title):
+                log.debug(
+                    "Falling back to commit_status: Status already exists for this commit",
+                    extra={
+                        "notifier": self.name,
+                        "repoid": comparison.head.commit.repoid,
+                        "notifier_title": self.title,
+                        "status_title": status_title,
+                        "commit": comparison.head.commit,
+                    },
+                )
+                return NotificationResult(
+                    notification_attempted=False,
+                    notification_successful=None,
+                    explanation="preexisting_commit_status",
+                    data_sent=None,
+                    data_received=None,
+                )
+
        payload = None
        try:
            with nullcontext():


Deep nesting of conditional logic makes the code hard to read and maintain. Consider extracting the validation logic into separate methods to improve readability and testability.

Suggested change

if not force_notify:

if comparison.pull is None or ():

log.debug(

"Falling back to commit_status: Not a pull request",

extra={

"notifier": self.name,

"repoid": comparison.head.commit.repoid,

"notifier_title": self.title,

"commit": comparison.head.commit,

},

)

return NotificationResult(

notification_attempted=False,

notification_successful=None,

explanation="no_pull_request",

data_sent=None,

data_received=None,

)

if (

comparison.enriched_pull is None

or comparison.enriched_pull.provider_pull is None

):

log.debug(

"Falling back to commit_status: Pull request not in provider",

extra={

"notifier": self.name,

"repoid": comparison.head.commit.repoid,

"notifier_title": self.title,

"commit": comparison.head.commit,

},

)

return NotificationResult(

notification_attempted=False,

notification_successful=None,

explanation="pull_request_not_in_provider",

data_sent=None,

data_received=None,

)

if comparison.pull and comparison.pull.state != "open":

log.debug(

"Falling back to commit_status: Pull request closed",

extra={

"notifier": self.name,

"repoid": comparison.head.commit.repoid,

"notifier_title": self.title,

"commit": comparison.head.commit,

},

)

return NotificationResult(

notification_attempted=False,

notification_successful=None,

explanation="pull_request_closed",

data_sent=None,

data_received=None,

)

# Check branches config for this status before sending the check

if not self.can_we_set_this_status(comparison):

return NotificationResult(

notification_attempted=False,

notification_successful=None,

explanation="not_fit_criteria",

data_sent=None,

)

if not self.required_builds(comparison):

return NotificationResult(

notification_attempted=False,

notification_successful=None,

explanation="need_more_builds",

data_sent=None,

data_received=None,

)

# Check for existing statuses for this commit. If so, retain

# statuses and don't do a check as well

statuses = comparison.get_existing_statuses()

status_title = self.get_status_external_name()

if statuses and statuses.get(status_title):

log.debug(

"Falling back to commit_status: Status already exists for this commit",

extra={

"notifier": self.name,

"repoid": comparison.head.commit.repoid,

"notifier_title": self.title,

"status_title": status_title,

"commit": comparison.head.commit,

},

)

return NotificationResult(

notification_attempted=False,

notification_successful=None,

explanation="preexisting_commit_status",

data_sent=None,

data_received=None,

)

payload = None

try:

with nullcontext():

if not force_notify:

validation_result = self._validate_notification_conditions(comparison)

if validation_result:

return validation_result

# Then implement:

def _validate_notification_conditions(self, comparison) -> NotificationResult | None:

# Extract all validation logic here

pass

_{Did we get this right? 👍 / 👎 to inform future reviews.}

seer-by-sentry · 2025-07-24T20:42:26Z

apps/worker/services/notification/notifiers/status/base.py

+        force_notify = getattr(comparison.context, "force_notify", False)
+
        repository_service = self.repository_service
        title = self.get_status_external_name()
        head_commit_sha = comparison.head.commit.commitid
        head_report = comparison.head.report
        state = payload["state"]
        message = payload["message"]
        url = payload["url"]
-        if self.status_already_exists(comparison, title, state, message):
+


The logic for handling force_notify in send_notification duplicates the check from maybe_send_notification. This creates a potential inconsistency where the behavior could differ between the two methods.

Suggested change

force_notify = getattr(comparison.context, "force_notify", False)

repository_service = self.repository_service

title = self.get_status_external_name()

head_commit_sha = comparison.head.commit.commitid

head_report = comparison.head.report

state = payload["state"]

message = payload["message"]

url = payload["url"]

if self.status_already_exists(comparison, title, state, message):

# Remove the duplicate force_notify check here since maybe_send_notification already handles it appropriately

_{Did we get this right? 👍 / 👎 to inform future reviews.}

seer-by-sentry · 2025-07-24T20:42:29Z

apps/worker/tasks/notify.py

+                }
+            else:
+                ci_results = None


When force_notify=True and CI results cannot be fetched, setting ci_results = None could lead to unexpected behavior downstream. Consider logging this decision more prominently or adding explicit handling for this case.

Suggested change

}

else:

ci_results = None

else:

log.warning(

"Force notify enabled: proceeding without CI results due to client error",

extra={"repoid": repoid, "commit": commitid}

)

ci_results = None

_{Did we get this right? 👍 / 👎 to inform future reviews.}

codecov-notifications · 2025-07-24T20:45:57Z

Codecov Report

❌ Patch coverage is 93.44262% with 4 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
apps/worker/tasks/notify.py	83.33%	3 Missing ⚠️
...ker/services/notification/notifiers/status/base.py	94.11%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

codecov · 2025-07-24T20:46:00Z

Codecov Report

❌ Patch coverage is 93.44262% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.29%. Comparing base (bf7d111) to head (c5854b4).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
apps/worker/tasks/notify.py	83.33%	3 Missing ⚠️
...ker/services/notification/notifiers/status/base.py	94.11%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #349      +/-   ##
==========================================
- Coverage   94.30%   94.29%   -0.01%     
==========================================
  Files        1247     1247              
  Lines       45835    45874      +39     
  Branches     1455     1455              
==========================================
+ Hits        43223    43258      +35     
- Misses       2307     2311       +4     
  Partials      305      305

Flag	Coverage Δ
apiunit	`96.42% <100.00%> (ø)`
workerintegration	`61.55% <59.32%> (-0.01%)`	⬇️
workerunit	`90.53% <93.22%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

seer-by-sentry bot reviewed Jul 24, 2025

View reviewed changes

joseph-sentry force-pushed the joseph/force-notify branch from 07ca5c6 to e0c23ba Compare July 25, 2025 15:44

joseph-sentry added 2 commits July 29, 2025 13:23

add force notify option

d0a421b

feat: add force_notify arg to task service

c5854b4

joseph-sentry force-pushed the joseph/force-notify branch from e0c23ba to c5854b4 Compare July 29, 2025 17:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

add force notify option #349

add force notify option #349

Uh oh!

joseph-sentry commented Jul 24, 2025

Uh oh!

joseph-sentry commented Jul 24, 2025

Uh oh!

seer-by-sentry bot Jul 24, 2025

Uh oh!

seer-by-sentry bot Jul 24, 2025

Uh oh!

seer-by-sentry bot Jul 24, 2025

Uh oh!

seer-by-sentry bot Jul 24, 2025

Uh oh!

seer-by-sentry bot Jul 24, 2025

Uh oh!

codecov-notifications bot commented Jul 24, 2025 •

edited

Loading

Uh oh!

codecov bot commented Jul 24, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-                }
-            else:
-                ci_results = None
+else:
+    log.warning(
+        "Force notify enabled: proceeding without CI results due to client error",
+        extra={"repoid": repoid, "commit": commitid}
+    )
+    ci_results = None

add force notify option #349

Are you sure you want to change the base?

add force notify option #349

Uh oh!

Conversation

joseph-sentry commented Jul 24, 2025

Uh oh!

joseph-sentry commented Jul 24, 2025

Uh oh!

seer-by-sentry bot Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

seer-by-sentry bot Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

seer-by-sentry bot Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

seer-by-sentry bot Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

seer-by-sentry bot Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

codecov-notifications bot commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codecov bot commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov-notifications bot commented Jul 24, 2025 •

edited

Loading

codecov bot commented Jul 24, 2025 •

edited

Loading