Feat/workflow hardening#83
Conversation
Reviewer's GuideAdds a runtime /health endpoint integrated with MQTT status, introduces a standardized local development workflow with Makefile targets and git hooks, hardens MQTT configuration via environment-driven settings and TLS, and sets up contribution documentation plus CI and local tooling to enforce path safety and review hygiene. Sequence diagram for the new /health runtime checksequenceDiagram
actor Developer
participant Shell as LocalShell
participant HealthScript as health_check_sh
participant Flask as FlaskApp
participant MQTT as MQTTClient
Developer->>LocalShell: run make health
LocalShell->>HealthScript: bash scripts/health_check.sh
HealthScript->>Flask: HTTP GET /health
Flask->>MQTT: read connected attribute
MQTT-->>Flask: connected (true|false)
alt MQTT connected
Flask-->>HealthScript: 200 {status: ok, service: smart-google, mqtt_connected: true}
HealthScript-->>LocalShell: exit 0 (Health check passed)
else MQTT not connected
Flask-->>HealthScript: 503 {status: degraded, service: smart-google, mqtt_connected: false}
HealthScript-->>LocalShell: exit 1 (Health check failed)
end
LocalShell-->>Developer: show health result and HTTP body
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the project's development workflow, maintainability, and security posture. It establishes clear contribution guidelines, automates pre-commit checks to enforce code quality and prevent common issues, and introduces a centralized command interface for development tasks. Additionally, it enhances the application's observability with a new health check endpoint and makes critical configurations more flexible. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
/healthendpoint reaches intonotifications.mqtt.connecteddirectly; consider adding a small helper or interface on the MQTT client so the health check doesn’t depend on an internal attribute that might change or be unset. - In
config.py,MQTT_TLS_VERSION = ssl.PROTOCOL_TLSv1_2is now unconditional; if TLS is disabled orsslis not already imported in this module, this may be unnecessary or error-prone—consider importingsslexplicitly and only setting the version whenMQTT_TLS_ENABLEDis true or making it configurable. - The
scripts/install_git_hooks.shscript assumes.githooks/pre-commitexists and is chmod-safe; you may want to add a guard that checks for the file’s existence before changing permissions to avoid failing in fresh clones or forks without that hook.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `/health` endpoint reaches into `notifications.mqtt.connected` directly; consider adding a small helper or interface on the MQTT client so the health check doesn’t depend on an internal attribute that might change or be unset.
- In `config.py`, `MQTT_TLS_VERSION = ssl.PROTOCOL_TLSv1_2` is now unconditional; if TLS is disabled or `ssl` is not already imported in this module, this may be unnecessary or error-prone—consider importing `ssl` explicitly and only setting the version when `MQTT_TLS_ENABLED` is true or making it configurable.
- The `scripts/install_git_hooks.sh` script assumes `.githooks/pre-commit` exists and is chmod-safe; you may want to add a guard that checks for the file’s existence before changing permissions to avoid failing in fresh clones or forks without that hook.
## Individual Comments
### Comment 1
<location path="config.py" line_range="26" />
<code_context>
SQLALCHEMY_TRACK_MODIFICATIONS = False
MQTT_BROKER_URL = environ.get('MQTT_BROKER_URL')
- MQTT_BROKER_PORT = 11868
+ MQTT_BROKER_PORT = int(environ.get('MQTT_BROKER_PORT', 11868))
MQTT_USERNAME = environ.get('MQTT_USERNAME')
MQTT_PASSWORD = environ.get('MQTT_PASSWORD')
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider more defensive parsing of MQTT_BROKER_PORT to handle empty or invalid values.
If `MQTT_BROKER_PORT` is present but empty or non-numeric, `int(...)` will raise `ValueError` and prevent startup. Consider a small helper or try/except to fall back to the default or emit a clearer configuration error.
Suggested implementation:
```python
from dotenv import load_dotenv
from generate_service_account_file import generate_file
def _get_int_env(name, default):
"""
Safely parse an integer environment variable.
Returns the provided default if the variable is unset, empty, or invalid.
"""
raw_value = environ.get(name)
if raw_value is None or raw_value == "":
return default
try:
return int(raw_value)
except (TypeError, ValueError):
return default
```
```python
MQTT_BROKER_URL = environ.get('MQTT_BROKER_URL')
MQTT_BROKER_PORT = _get_int_env('MQTT_BROKER_PORT', 11868)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly hardens the development workflow by introducing a Makefile, git hooks for pre-commit checks, a pull request template, and comprehensive contributing guidelines. It also improves security by expanding the .gitignore to cover credentials and local artifacts, and makes the MQTT configuration more robust and environment-driven. I've added a couple of suggestions to further refine the test script logic in the Makefile and improve the TLS configuration for future-proofing.
|
@copilot Please address the comments from this code review: Overall Comments
Individual CommentsComment 1SQLALCHEMY_TRACK_MODIFICATIONS = False MQTT_BROKER_URL = environ.get('MQTT_BROKER_URL') - MQTT_BROKER_PORT = 11868 + MQTT_BROKER_PORT = int(environ.get('MQTT_BROKER_PORT', 11868)) MQTT_USERNAME = environ.get('MQTT_USERNAME') MQTT_PASSWORD = environ.get('MQTT_PASSWORD') **suggestion (bug_risk):** Consider more defensive parsing of MQTT_BROKER_PORT to handle empty or invalid values.If Suggested implementation: from dotenv import load_dotenv
from generate_service_account_file import generate_file
def _get_int_env(name, default):
"""
Safely parse an integer environment variable.
Returns the provided default if the variable is unset, empty, or invalid.
"""
raw_value = environ.get(name)
if raw_value is None or raw_value == "":
return default
try:
return int(raw_value)
except (TypeError, ValueError):
return default MQTT_BROKER_URL = environ.get('MQTT_BROKER_URL')
MQTT_BROKER_PORT = _get_int_env('MQTT_BROKER_PORT', 11868)</issue_to_address> |
… 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>
* 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>
* 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>
* 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>
DaTiC0
left a comment
DaTiC0
left a comment
There was a problem hiding this comment.
testing auto review and fixing
There was a problem hiding this comment.
Pull request overview
This PR hardens local development and CI workflows by adding a runtime health endpoint, introducing local helper tooling (scripts/Makefile/hooks), and enforcing a CI safeguard that blocks commits containing machine-specific absolute paths.
Changes:
- Add
/healthendpoint that reports MQTT connectivity and returns200/503accordingly. - Add local tooling: Makefile targets, health/path-scan scripts, and a pre-commit hook installer.
- Add CI workflow to scan tracked files for absolute local filesystem paths; expand docs/templates for contribution/PR hygiene.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/install_git_hooks.sh | Configures repo-local git hooks path and ensures pre-commit hook is executable. |
| scripts/health_check.sh | Adds a curl-based CLI health check helper. |
| scripts/check_absolute_paths.sh | Adds tracked-file scan for common absolute path patterns. |
| routes.py | Adds /health endpoint using centralized MQTT connectivity check. |
| notifications.py | Adds is_mqtt_connected() helper around Flask-MQTT client state. |
| config.py | Reads MQTT broker port from env and adds env-driven TLS enablement + TLS version selection. |
| README.md | Documents health check usage and recommended collaboration workflow. |
| Makefile | Adds common dev commands (run/test/health/path scan/hook install). |
| CONTRIBUTING.md | Adds contributor workflow + PR requirements + AI-assisted review policy. |
| .gitignore | Broadens ignoring of local DB/credential artifacts and .env.* files. |
| .github/workflows/path-safety.yml | Adds CI job that fails if absolute local paths are detected. |
| .github/pull_request_template.md | Adds PR checklist including validation and AI-assistance declarations. |
| .githooks/pre-commit | Adds pre-commit hook to run the absolute-path scan locally. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
|
|
||
| pattern='(/home/[[:alnum:]_.-]+/|/Users/[[:alnum:]_.-]+/|[A-Za-z]:\\\\Users\\\\)' |
There was a problem hiding this comment.
The Windows-path branch of the regex looks like it only matches doubled backslashes (e.g., C:\\Users\\...). Many files will contain Windows paths with single backslashes (C:\Users\...), so this scan can miss the very cases it’s meant to catch. Consider adjusting the pattern to match one-or-more backslashes between segments (and/or both escaped and unescaped variants).
| pattern='(/home/[[:alnum:]_.-]+/|/Users/[[:alnum:]_.-]+/|[A-Za-z]:\\\\Users\\\\)' | |
| pattern='(/home/[[:alnum:]_.-]+/|/Users/[[:alnum:]_.-]+/|[A-Za-z]:\\\\+Users\\\\+)' |
| MQTT_KEEPALIVE = 90 | ||
| MQTT_TLS_ENABLED = False # set TLS to disabled for testing purposes | ||
| MQTT_TLS_ENABLED = environ.get('MQTT_TLS_ENABLED', 'False').lower() in ('1', 'true', 'yes', 'on') | ||
| MQTT_TLS_VERSION = ssl.PROTOCOL_TLS_CLIENT if MQTT_TLS_ENABLED else None |
There was a problem hiding this comment.
ssl.PROTOCOL_TLS_CLIENT negotiates the highest supported TLS version (may include TLS 1.3) and does not strictly enforce TLS v1.2 as described in the PR. If you need to pin TLS 1.2, set the TLS version accordingly (e.g., use the TLSv1.2 protocol constant that Flask-MQTT/paho expects, or configure min/max TLS version via an SSLContext if supported).
| MQTT_TLS_VERSION = ssl.PROTOCOL_TLS_CLIENT if MQTT_TLS_ENABLED else None | |
| MQTT_TLS_VERSION = ssl.PROTOCOL_TLSv1_2 if MQTT_TLS_ENABLED else None |
Summary by Sourcery
Add runtime health endpoint, local tooling, and workflow safeguards to harden development and deployment practices.
New Features:
Enhancements:
CI:
Documentation:
Chores: