Skip to content

Fix/health endpoint abstraction, conditional TLS config, and git hook guard#84

Merged
DaTiC0 merged 3 commits into
feat/workflow-hardeningfrom
copilot/sub-pr-83
Mar 7, 2026
Merged

Fix/health endpoint abstraction, conditional TLS config, and git hook guard#84
DaTiC0 merged 3 commits into
feat/workflow-hardeningfrom
copilot/sub-pr-83

Conversation

Copilot AI commented Mar 7, 2026

Copy link
Copy Markdown
Contributor

Summary

  • /health endpoint accessed mqtt.connected directly — fragile against internal attribute changes or unset state. Added is_mqtt_connected() helper in notifications.py; health route now calls the helper instead, and the unused mqtt import was removed from routes.py.
  • MQTT_TLS_VERSION was set unconditionally to ssl.PROTOCOL_TLSv1_2 regardless of whether TLS was enabled. Now conditional on MQTT_TLS_ENABLED.
  • install_git_hooks.sh would fail chmod +x on a fresh clone without .githooks/pre-commit. Added existence guard.
# notifications.py — new helper
def is_mqtt_connected():
    """Return True if the MQTT client is currently connected."""
    return bool(getattr(mqtt, 'connected', False))

# routes.py — uses helper only; unused mqtt import removed
from notifications import is_mqtt_connected

# config.py — TLS version now conditional
MQTT_TLS_VERSION = ssl.PROTOCOL_TLSv1_2 if MQTT_TLS_ENABLED else None
# install_git_hooks.sh — guard before chmod
if [ -f ".githooks/pre-commit" ]; then
    chmod +x .githooks/pre-commit
fi

Validation

  • Local checks passed (make test)
  • Runtime check done where relevant (make health)
  • CI checks are green

AI-Assisted Review (if applicable)

  • I used AI assistance for parts of this change
  • I manually reviewed all AI-generated code
  • I verified no secrets/credentials were added

Risk & Rollback

  • Risk level: Low
  • Rollback plan: Revert commit c1fc041; no data or schema changes involved.

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

…ok guard

Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>
Copilot AI changed the title [WIP] Add runtime health endpoint and workflow safeguards Fix/health endpoint abstraction, conditional TLS config, and git hook guard Mar 7, 2026
@DaTiC0
DaTiC0 requested a review from Copilot March 7, 2026 14:39
@DaTiC0
DaTiC0 marked this pull request as ready for review March 7, 2026 14:41
@DaTiC0

DaTiC0 commented Mar 7, 2026

Copy link
Copy Markdown
Owner

@copilot DeepSource: Python

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves robustness around the app’s health check, MQTT TLS configuration, and developer tooling setup by centralizing MQTT connection status checks, avoiding TLS settings when TLS is disabled, and preventing git-hook install errors on fresh clones.

Changes:

  • Added is_mqtt_connected() helper in notifications.py and updated /health route to use it.
  • Made MQTT_TLS_VERSION conditional on MQTT_TLS_ENABLED.
  • Guarded chmod +x in scripts/install_git_hooks.sh so it doesn’t fail when the hook file is missing.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
scripts/install_git_hooks.sh Adds existence check before chmod’ing the pre-commit hook.
routes.py Switches /health to use the new MQTT connection helper.
notifications.py Introduces is_mqtt_connected() helper around MQTT client state.
config.py Makes TLS version configuration conditional on TLS being enabled.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread routes.py Outdated
from models import Client
from my_oauth import get_current_user, oauth
from notifications import mqtt
from notifications import mqtt, is_mqtt_connected

Copilot AI Mar 7, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mqtt is imported but no longer used in this module after switching the /health route to is_mqtt_connected(). Dropping the unused import will avoid lint/tooling warnings and makes it clearer what this file depends on.

Suggested change
from notifications import mqtt, is_mqtt_connected
from notifications import is_mqtt_connected

Copilot uses AI. Check for mistakes.
Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>

Copilot AI commented Mar 7, 2026

Copy link
Copy Markdown
Contributor Author

@copilot DeepSource: Python

Fixed in c1fc041. DeepSource flagged F401 'notifications.mqtt' imported but unused in routes.py — after the refactor to use is_mqtt_connected(), mqtt itself was no longer referenced. Removed the unused import.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • app.deepsource.com
    • Triggering command: /home/REDACTED/work/_temp/ghcca-node/node/bin/node /home/REDACTED/work/_temp/ghcca-node/node/bin/node --enable-source-maps /home/REDACTED/work/_temp/copilot-developer-action-main/dist/index.js (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@DaTiC0
DaTiC0 merged commit 166ac78 into feat/workflow-hardening Mar 7, 2026
1 check failed
@DaTiC0
DaTiC0 deleted the copilot/sub-pr-83 branch March 7, 2026 14:49
DaTiC0 added a commit that referenced this pull request Mar 7, 2026
* Add health checks, workflow guards, and secure repo tooling

* Document safe recovery for accidental main commits

* Fix/health endpoint abstraction, conditional TLS config, and git hook guard (#84)

* Initial plan

* Address review feedback: MQTT helper, conditional TLS version, git hook guard

Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>

* Remove unused mqtt import from routes.py (fixes DeepSource F401)

Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>

* Fix incorrect exit code 5 handling in Makefile unittest fallback (#87)

* Initial plan

* Fix incorrect exit code 5 handling in unittest block of Makefile

Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>

* Use ssl.PROTOCOL_TLS_CLIENT for forward-compatible TLS negotiation (#86)

* Initial plan

* Use ssl.PROTOCOL_TLS_CLIENT instead of ssl.PROTOCOL_TLSv1_2 for TLS version negotiation

Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>

* Defensive parsing of MQTT_BROKER_PORT env var (#85)

* Initial plan

* Add _get_int_env helper for safe MQTT_BROKER_PORT parsing

Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>

---------

Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants