diff --git a/app/app.py b/app/app.py index 7d00949..4273b41 100644 --- a/app/app.py +++ b/app/app.py @@ -609,8 +609,19 @@ def create_app(config=None): from app.discord_bot import start_bot start_bot(flask_app=app) - except Exception as e: - app.logger.warning('Could not start Discord bot: %s', e) + except Exception: # the site must come up even if the bot cannot + # With the message alone, the two ways this fails — a bad token + # and a broken import in discord_bot — read identically, and + # neither is diagnosable from one line. Notifications are down + # either way, so the traceback is the whole value of the log. + # Error, not warning: a club that receives no reminders has lost + # a feature, and the old level put that next to the deprecation + # notices. + app.logger.error( + 'Could not start the Discord bot. The site is up; no notification ' + 'will be sent until this is fixed.', + exc_info=True, + ) return app diff --git a/app/logging_config.py b/app/logging_config.py index 82397e3..a126cf9 100644 --- a/app/logging_config.py +++ b/app/logging_config.py @@ -88,8 +88,12 @@ class SensitiveDataFilter(logging.Filter): """ try: rendered = record.getMessage() - except Exception: + except Exception: # noqa: BLE001 — see below; this one cannot log its own failure # A malformed format string must not lose the record entirely. + # Nor can it be logged: this runs inside a filter, and logging + # from here re-enters the same filter on the new record. The + # traceback BLE001 normally asks for is the one thing this + # handler must not produce, hence the waiver. return True for pattern, replacement in self.SENSITIVE_PATTERNS: diff --git a/app/services/notifications.py b/app/services/notifications.py index 72166f2..496fb1d 100644 --- a/app/services/notifications.py +++ b/app/services/notifications.py @@ -4,10 +4,15 @@ Extracted from app/routes/users.py, where it sat between two route definitions and pulled `requests`, `logging` and the Discord bot into a module whose subject is HTTP handlers (ARCH-003). -Every failure here is swallowed and logged on purpose: a notification that -does not reach Discord must not roll back the session it was announcing. -That is the one place in this codebase where `except Exception` is the -right answer rather than an oversight. +Failures here are swallowed and logged on purpose: a notification that does +not reach Discord must not roll back the session it was announcing. That is +a property of the caller — a Flask request whose work is already committed — +not of the failure, which is why the breadth is argued for at each of the +two boundaries below rather than assumed (ARCH-008 / QUA-004). + +The webhook branch is narrower than it was: `requests.RequestException` +covers every way an HTTP call can fail, and anything else coming out of it +is a defect worth seeing. """ import logging @@ -61,56 +66,62 @@ def send_discord_notification( points=points, request_id=request_id, ) - except Exception as e: - logger.warning(f"Failed to send Discord DM: {e}") + except Exception: # noqa: BLE001 — the request that booked the meeting is already committed + logger.warning('Failed to hand the One on One DM to the bot', exc_info=True) - if DISCORD_WEBHOOK_URL: - try: + if not DISCORD_WEBHOOK_URL: + return + + # A bare snowflake here means "DM this person instead", and only when the + # coach has no id of their own. Queueing it cannot raise (see _enqueue). + if DISCORD_WEBHOOK_URL.isdigit(): + if not coach_discord_id: from app.discord_bot import send_one_on_one_dm - if DISCORD_WEBHOOK_URL.isdigit() and not coach_discord_id: - send_one_on_one_dm( - coach_name=coach_name, - coach_discord_id=DISCORD_WEBHOOK_URL, - player_name=player_name, - team_name=team_name, - date_str=date_str, - start_time=start_time_str, - end_time=end_time_str, - points=points, - ) - elif not DISCORD_WEBHOOK_URL.isdigit(): - embed = { - "embeds": [ - { - "title": "One on One Request", - "color": 3447003, - "fields": [ - {"name": "Player", "value": player_name, "inline": True}, - { - "name": "Team", - "value": team_name or "Unknown Team", - "inline": True, - }, - {"name": "Date", "value": date_str, "inline": True}, - { - "name": "Time", - "value": f"{start_time_str} - {end_time_str}", - "inline": True, - }, - { - "name": "Discussion Points", - "value": points or "No specific points provided", - "inline": False, - }, - ], - "footer": { - "text": f"Coach: {coach_name}" - + (f" (Discord: {coach_discord})" if coach_discord else ""), - }, - } - ], - } - requests.post(DISCORD_WEBHOOK_URL, json=embed, timeout=5) - except Exception as e: - logger.warning(f"Failed to send Discord notification: {e}") + send_one_on_one_dm( + coach_name=coach_name, + coach_discord_id=DISCORD_WEBHOOK_URL, + player_name=player_name, + team_name=team_name, + date_str=date_str, + start_time=start_time_str, + end_time=end_time_str, + points=points, + ) + return + + embed = { + "embeds": [ + { + "title": "One on One Request", + "color": 3447003, + "fields": [ + {"name": "Player", "value": player_name, "inline": True}, + { + "name": "Team", + "value": team_name or "Unknown Team", + "inline": True, + }, + {"name": "Date", "value": date_str, "inline": True}, + { + "name": "Time", + "value": f"{start_time_str} - {end_time_str}", + "inline": True, + }, + { + "name": "Discussion Points", + "value": points or "No specific points provided", + "inline": False, + }, + ], + "footer": { + "text": f"Coach: {coach_name}" + + (f" (Discord: {coach_discord})" if coach_discord else ""), + }, + } + ], + } + try: + requests.post(DISCORD_WEBHOOK_URL, json=embed, timeout=5) + except requests.RequestException as exc: + logger.warning('Failed to post the One on One webhook: %s', exc) diff --git a/app/supporting_scripts/backup.py b/app/supporting_scripts/backup.py index 1126226..53d7475 100644 --- a/app/supporting_scripts/backup.py +++ b/app/supporting_scripts/backup.py @@ -249,7 +249,11 @@ def backup_documents(): try: shutil.make_archive(archive_basename, 'zip', DOCUMENTS_DIR) - except Exception as exc: + except Exception as exc: # noqa: BLE001 — a failed document archive must not lose the dump + # This runs after the database dump has already succeeded. Letting + # anything through here would abort the script with a traceback and + # take the one part that worked down with it. Reported to stdout, in + # the format the rest of this script uses; it has no logger. print(f'[ERROR] Document backup failed: {exc}') return None diff --git a/app/supporting_scripts/security_scan.py b/app/supporting_scripts/security_scan.py index dc8afa9..32632c2 100644 --- a/app/supporting_scripts/security_scan.py +++ b/app/supporting_scripts/security_scan.py @@ -332,10 +332,15 @@ def check_flask_config(): else: print('[OK] DEBUG mode: disabled') - except Exception as e: + except Exception as e: # noqa: BLE001 — any failure to load the app is a failed check # Returning all_ok (still True) here meant that failing to load the # application at all was counted as a passing check — the most # important section of the report silently never ran. + # + # The breadth is the point: this section's question is "does the + # application load with a safe configuration", and every way of not + # loading answers it the same way. Reported on stdout because this + # script is read by a CI job, not by a log collector. print(f'[FAIL] Cannot check Flask config: {e}') return False diff --git a/pyproject.toml b/pyproject.toml index a954859..101fff0 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -45,7 +45,14 @@ exclude = [".venv", "venv", "migrations", "docs"] # I isort; enabled in its own commit, for the same reason the # formatting got one — the diff is churn and must not hide # behind a behavioural change -select = ["E4", "E7", "E9", "F", "B", "C4", "RET", "SIM", "UP", "I"] +# BLE blind `except Exception`. Added with QUA-004/ARCH-008, and +# the point of it is what happens next: the rule cannot be +# satisfied by narrowing every catch, because a handful of +# boundaries genuinely need the breadth. It can only be +# satisfied by writing down which ones and why, in a +# `# noqa: BLE001 — reason` on the line. That turns a count +# that drifts back up into a decision someone has to defend. +select = ["E4", "E7", "E9", "F", "B", "C4", "RET", "SIM", "UP", "I", "BLE"] # Forcing a ternary reads worse than the if/else it replaces in the one place # it fires (evaluations.py, choosing a sort direction).