Fix/remove conflict markers #13
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Leitfaden für ReviewerDieser PR entfernt verbleibende Konfliktmarker und redundanten Code, refaktoriert den One-Click-Installer für Konsistenz und Einfachheit, überarbeitet die README mit strukturiertem Markdown und optimiert API, Schema, gemeinsam genutzte Module und Tests durch die Eliminierung von Duplikaten sowie die Verbesserung von Validatoren und Formatierung. Änderungen auf Dateiebene
Tipps und BefehleInteraktion mit Sourcery
Anpassung Ihrer ErfahrungGreifen Sie auf Ihr Dashboard zu, um:
Hilfe erhalten
Original review guide in EnglishReviewer's GuideThis PR removes residual conflict markers and redundant code, refactors the one-click installer for consistency and simplicity, overhauls the README with structured markdown, and streamlines API, schema, shared modules, and tests by eliminating duplicates and improving validators and formatting. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hallo zusammen – ich habe eure Änderungen geprüft – hier ist etwas Feedback:
Blockierende Probleme:
- Ein potenzielles Autorisierungstoken wurde in einem Curl-Befehlsheader entdeckt, was die über Curl aufgerufene Ressource kompromittieren könnte. (Link)
Allgemeine Kommentare:
- Entfernt den verbleibenden Konfliktmarker '->>>>>> main' am Anfang von README.md, um Merge-Konflikte vollständig zu beheben.
- Überprüft, ob shared/db.py das Datenbankschema beim ersten Start immer noch initialisiert, da der SCHEMA-Ausführungsblock entfernt wurde.
- In install.sh solltet ihr in Betracht ziehen, die Unbound-Root-Hints nur dann neu herunterzuladen, wenn sie älter als ein Schwellenwert sind (z.B. 30 Tage), um unnötige Downloads bei jedem Lauf zu vermeiden.
Prompt für KI-Agenten
Please address the comments from this code review:
## Overall Comments
- Remove the leftover conflict marker '->>>>>> main' at the top of README.md to fully clear merge conflicts.
- Verify that shared/db.py still initializes the database schema on first run, since the SCHEMA execution block was removed.
- In install.sh, consider only redownloading the Unbound root hints if they're older than a threshold (e.g. 30 days) to avoid unnecessary downloads on every run.
## Individual Comments
### Comment 1
<location> `install.sh:102-104` </location>
<code_context>
-EOF
-
- # Run Pi-hole installer
+ if ! command -v pihole >/dev/null; then
curl -sSL https://install.pi-hole.net | bash /dev/stdin --unattended
fi
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Die Pi-hole-Installation überprüft nicht den erfolgreichen Abschluss, bevor fortgefahren wird.
Überprüft den Exit-Status des Installers, bevor ihr mit der Konfiguration fortfahrt, um Fehler zu vermeiden, falls die Installation fehlschlägt.
```suggestion
if ! command -v pihole >/dev/null; then
curl -sSL https://install.pi-hole.net | bash /dev/stdin --unattended
installer_status=$?
if [ $installer_status -ne 0 ]; then
error "Pi-hole installation failed (exit code $installer_status). Aborting."
exit 1
fi
fi
```
</issue_to_address>
### Comment 2
<location> `install.sh:131-133` </location>
<code_context>
- fi
-
- # Install Python dependencies
+ [[ -d .venv ]] || sudo -u "$INSTALL_USER" python3 -m venv .venv
sudo -u "$INSTALL_USER" .venv/bin/pip install -U pip
sudo -u "$INSTALL_USER" .venv/bin/pip install -r requirements.txt
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Die Erstellung der Virtualenv prüft nicht auf Fehler oder Python-Versionskompatibilität.
Überprüft den Exit-Status nach dem Versuch, die virtuelle Umgebung zu erstellen, und zeigt eine informative Fehlermeldung an, falls dies fehlschlägt.
```suggestion
if [[ ! -d .venv ]]; then
sudo -u "$INSTALL_USER" python3 -m venv .venv
if [[ $? -ne 0 ]]; then
echo "ERROR: Failed to create Python virtual environment (.venv)."
echo "Please ensure Python 3 is installed and supports venv."
exit 1
fi
fi
sudo -u "$INSTALL_USER" .venv/bin/pip install -U pip
sudo -u "$INSTALL_USER" .venv/bin/pip install -r requirements.txt
```
</issue_to_address>
### Comment 3
<location> `install.sh:162` </location>
<code_context>
-
- # Enable and start the service
+
+ cat > .env <<ENV
+SUITE_API_KEY=$SUITE_API_KEY
+SUITE_PORT=$PYTHON_SUITE_PORT
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Die Umgebungsdatei wird bedingungslos überschrieben, was Benutzeränderungen löschen könnte.
Bitte überprüft, ob .env existiert, bevor ihr sie überschreibt, und erwägt, den Benutzer aufzufordern oder die Datei zu sichern, um den Verlust benutzerdefinierter Einstellungen zu verhindern.
</issue_to_address>
### Comment 4
<location> `api/main.py:52` </location>
<code_context>
return os.getenv("SUITE_API_KEY", "")
def get_db() -> Generator[sqlite3.Connection, None, None]:
- """Database dependency."""
+ """FastAPI dependency to provide a DB connection."""
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Die Datenbankverbindung wird nach Gebrauch nicht geschlossen, was zu Ressourcenlecks führen kann.
Bitte aktualisiert get_db, um sicherzustellen, dass die Datenbankverbindung nach Gebrauch ordnungsgemäß geschlossen wird, z.B. durch die Verwendung eines Kontextmanagers.
</issue_to_address>
### Comment 5
<location> `api/schemas.py:60` </location>
<code_context>
-
- @field_validator('mac_address')
+
+ @field_validator("mac_address")
@classmethod
- def validate_mac_address(cls, v):
</code_context>
<issue_to_address>
**suggestion:** Die MAC-Adressvalidierung ist zu vereinfacht und könnte gültige Formate ablehnen.
Der aktuelle Validator unterstützt keine MAC-Adressen mit Bindestrichen oder Großbuchstaben. Die Verwendung eines Regex oder einer speziellen Bibliothek würde die Kompatibilität verbessern.
</issue_to_address>
### Comment 6
<location> `tests/test_api.py:84-90` </location>
<code_context>
+ assert isinstance(resp.json(), list)
def test_stats_endpoint(client, api_headers):
- """Test statistics endpoint."""
- response = client.get("/stats", headers=api_headers)
- assert response.status_code == 200
- data = response.json()
+ resp = client.get("/stats", headers=api_headers)
+ assert resp.status_code == 200
+ data = resp.json()
assert "total_dns_logs" in data
assert "total_devices" in data
assert "recent_queries" in data
- assert isinstance(data["total_dns_logs"], int)
- assert isinstance(data["total_devices"], int)
</code_context>
<issue_to_address>
**suggestion (testing):** Der Test könnte Typen und Werte für die Felder der Statistikantwort überprüfen.
Das Wiederherstellen der Typzusicherungen für diese Felder hilft, die Konsistenz des Antwortschemas zu überprüfen.
</issue_to_address>
### Comment 7
<location> `README.md:96` </location>
<code_context>
your-api-key
</code_context>
<issue_to_address>
**security (curl-auth-header):** Ein potenzielles Autorisierungstoken wurde in einem Curl-Befehlsheader entdeckt, was die über Curl aufgerufene Ressource kompromittieren könnte.
*Quelle: gitleaks*
</issue_to_address>
### Comment 8
<location> `api/schemas.py:66` </location>
<code_context>
@field_validator("mac_address")
@classmethod
def validate_mac_address(cls, v: Optional[str]) -> Optional[str]:
if v is None:
return v
# Simple MAC address validation (XX:XX:XX:XX:XX:XX)
if not (len(v) == 17 and v.count(":") == 5):
raise ValueError("Invalid MAC address format")
return v.lower()
</code_context>
<issue_to_address>
**suggestion (code-quality):** Vereinfacht den logischen Ausdruck unter Verwendung der De Morgan'schen Gesetze ([`de-morgan`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/de-morgan/))
```suggestion
if len(v) != 17 or v.count(":") != 5:
```
</issue_to_address>
### Comment 9
<location> `start_suite.py:71` </location>
<code_context>
def main() -> None:
"""Main application entry point."""
logger.info("Starting Pi-hole Suite...")
# Verify API key
api_key = os.environ.get("SUITE_API_KEY")
if not api_key:
logger.error("SUITE_API_KEY environment variable must be set")
logger.info("Generate one with: openssl rand -hex 16")
sys.exit(1)
# Initialize database
try:
conn = init_db()
logger.info("Database initialized successfully")
except Exception as e:
logger.error(f"Failed to initialize database: {e}")
sys.exit(1)
# Start core DNS monitoring
try:
dns_thread = threading.Thread(
target=dns_start,
args=(conn,),
daemon=True,
name="DNSMonitor",
)
dns_thread.start()
logger.info("DNS monitor started")
except Exception as e:
logger.error(f"Failed to start DNS monitor: {e}")
# Start optional demo allocator if enabled
if ENABLE_PYALLOC_DEMO:
try:
alloc_thread = threading.Thread(
target=alloc_start,
args=(conn,),
daemon=True,
name="AllocDemo",
)
alloc_thread.start()
logger.info("PyAlloc demo component started")
except Exception as e:
logger.warning(f"Failed to start PyAlloc demo: {e}")
logger.info(f"API Key: {api_key[:8]}...")
logger.info("Starting API server...")
try:
asyncio.run(run_api())
except KeyboardInterrupt:
logger.info("Shutting down...")
except Exception as e:
logger.error(f"Application error: {e}")
sys.exit(1)
</code_context>
<issue_to_address>
**issue (code-quality):** Duplizierten Code in eine Funktion extrahieren ([`extract-duplicate-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/de-morgan/))
</issue_to_address>Sourcery ist kostenlos für Open Source – wenn euch unsere Reviews gefallen, teilt sie gerne ✨
Original comment in English
Hey there - I've reviewed your changes - here's some feedback:
Blocking issues:
- Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource. (link)
General comments:
- Remove the leftover conflict marker '->>>>>> main' at the top of README.md to fully clear merge conflicts.
- Verify that shared/db.py still initializes the database schema on first run, since the SCHEMA execution block was removed.
- In install.sh, consider only redownloading the Unbound root hints if they're older than a threshold (e.g. 30 days) to avoid unnecessary downloads on every run.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Remove the leftover conflict marker '->>>>>> main' at the top of README.md to fully clear merge conflicts.
- Verify that shared/db.py still initializes the database schema on first run, since the SCHEMA execution block was removed.
- In install.sh, consider only redownloading the Unbound root hints if they're older than a threshold (e.g. 30 days) to avoid unnecessary downloads on every run.
## Individual Comments
### Comment 1
<location> `install.sh:102-104` </location>
<code_context>
-EOF
-
- # Run Pi-hole installer
+ if ! command -v pihole >/dev/null; then
curl -sSL https://install.pi-hole.net | bash /dev/stdin --unattended
fi
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Pi-hole installation does not verify successful install before proceeding.
Check the installer’s exit status before proceeding to configuration to prevent errors if installation fails.
```suggestion
if ! command -v pihole >/dev/null; then
curl -sSL https://install.pi-hole.net | bash /dev/stdin --unattended
installer_status=$?
if [ $installer_status -ne 0 ]; then
error "Pi-hole installation failed (exit code $installer_status). Aborting."
exit 1
fi
fi
```
</issue_to_address>
### Comment 2
<location> `install.sh:131-133` </location>
<code_context>
- fi
-
- # Install Python dependencies
+ [[ -d .venv ]] || sudo -u "$INSTALL_USER" python3 -m venv .venv
sudo -u "$INSTALL_USER" .venv/bin/pip install -U pip
sudo -u "$INSTALL_USER" .venv/bin/pip install -r requirements.txt
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Virtualenv creation does not check for errors or Python version compatibility.
Check the exit status after attempting to create the virtual environment and display an informative error if it fails.
```suggestion
if [[ ! -d .venv ]]; then
sudo -u "$INSTALL_USER" python3 -m venv .venv
if [[ $? -ne 0 ]]; then
echo "ERROR: Failed to create Python virtual environment (.venv)."
echo "Please ensure Python 3 is installed and supports venv."
exit 1
fi
fi
sudo -u "$INSTALL_USER" .venv/bin/pip install -U pip
sudo -u "$INSTALL_USER" .venv/bin/pip install -r requirements.txt
```
</issue_to_address>
### Comment 3
<location> `install.sh:162` </location>
<code_context>
-
- # Enable and start the service
+
+ cat > .env <<ENV
+SUITE_API_KEY=$SUITE_API_KEY
+SUITE_PORT=$PYTHON_SUITE_PORT
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Environment file is overwritten unconditionally, which may erase user changes.
Please check if .env exists before overwriting, and consider prompting the user or backing up the file to prevent loss of custom settings.
</issue_to_address>
### Comment 4
<location> `api/main.py:52` </location>
<code_context>
return os.getenv("SUITE_API_KEY", "")
def get_db() -> Generator[sqlite3.Connection, None, None]:
- """Database dependency."""
+ """FastAPI dependency to provide a DB connection."""
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Database connection is not closed after use, which may lead to resource leaks.
Please update get_db to ensure the database connection is properly closed after use, such as by using a context manager.
</issue_to_address>
### Comment 5
<location> `api/schemas.py:60` </location>
<code_context>
-
- @field_validator('mac_address')
+
+ @field_validator("mac_address")
@classmethod
- def validate_mac_address(cls, v):
</code_context>
<issue_to_address>
**suggestion:** MAC address validation is simplistic and may reject valid formats.
The current validator does not support MAC addresses with dashes or uppercase letters. Using a regex or a dedicated library would improve compatibility.
</issue_to_address>
### Comment 6
<location> `tests/test_api.py:84-90` </location>
<code_context>
+ assert isinstance(resp.json(), list)
def test_stats_endpoint(client, api_headers):
- """Test statistics endpoint."""
- response = client.get("/stats", headers=api_headers)
- assert response.status_code == 200
- data = response.json()
+ resp = client.get("/stats", headers=api_headers)
+ assert resp.status_code == 200
+ data = resp.json()
assert "total_dns_logs" in data
assert "total_devices" in data
assert "recent_queries" in data
- assert isinstance(data["total_dns_logs"], int)
- assert isinstance(data["total_devices"], int)
</code_context>
<issue_to_address>
**suggestion (testing):** Test could assert types and values for stats response fields.
Restoring the type assertions for these fields will help verify the response schema remains consistent.
</issue_to_address>
### Comment 7
<location> `README.md:96` </location>
<code_context>
your-api-key
</code_context>
<issue_to_address>
**security (curl-auth-header):** Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
*Source: gitleaks*
</issue_to_address>
### Comment 8
<location> `api/schemas.py:66` </location>
<code_context>
@field_validator("mac_address")
@classmethod
def validate_mac_address(cls, v: Optional[str]) -> Optional[str]:
if v is None:
return v
# Simple MAC address validation (XX:XX:XX:XX:XX:XX)
if not (len(v) == 17 and v.count(":") == 5):
raise ValueError("Invalid MAC address format")
return v.lower()
</code_context>
<issue_to_address>
**suggestion (code-quality):** Simplify logical expression using De Morgan identities ([`de-morgan`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/de-morgan/))
```suggestion
if len(v) != 17 or v.count(":") != 5:
```
</issue_to_address>
### Comment 9
<location> `start_suite.py:71` </location>
<code_context>
def main() -> None:
"""Main application entry point."""
logger.info("Starting Pi-hole Suite...")
# Verify API key
api_key = os.environ.get("SUITE_API_KEY")
if not api_key:
logger.error("SUITE_API_KEY environment variable must be set")
logger.info("Generate one with: openssl rand -hex 16")
sys.exit(1)
# Initialize database
try:
conn = init_db()
logger.info("Database initialized successfully")
except Exception as e:
logger.error(f"Failed to initialize database: {e}")
sys.exit(1)
# Start core DNS monitoring
try:
dns_thread = threading.Thread(
target=dns_start,
args=(conn,),
daemon=True,
name="DNSMonitor",
)
dns_thread.start()
logger.info("DNS monitor started")
except Exception as e:
logger.error(f"Failed to start DNS monitor: {e}")
# Start optional demo allocator if enabled
if ENABLE_PYALLOC_DEMO:
try:
alloc_thread = threading.Thread(
target=alloc_start,
args=(conn,),
daemon=True,
name="AllocDemo",
)
alloc_thread.start()
logger.info("PyAlloc demo component started")
except Exception as e:
logger.warning(f"Failed to start PyAlloc demo: {e}")
logger.info(f"API Key: {api_key[:8]}...")
logger.info("Starting API server...")
try:
asyncio.run(run_api())
except KeyboardInterrupt:
logger.info("Shutting down...")
except Exception as e:
logger.error(f"Application error: {e}")
sys.exit(1)
</code_context>
<issue_to_address>
**issue (code-quality):** Extract duplicate code into function ([`extract-duplicate-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-duplicate-method/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # Enable and start the service | ||
|
|
||
| cat > .env <<ENV |
There was a problem hiding this comment.
suggestion (bug_risk): Die Umgebungsdatei wird bedingungslos überschrieben, was Benutzeränderungen löschen könnte.
Bitte überprüft, ob .env existiert, bevor ihr sie überschreibt, und erwägt, den Benutzer aufzufordern oder die Datei zu sichern, um den Verlust benutzerdefinierter Einstellungen zu verhindern.
Original comment in English
suggestion (bug_risk): Environment file is overwritten unconditionally, which may erase user changes.
Please check if .env exists before overwriting, and consider prompting the user or backing up the file to prevent loss of custom settings.
| return os.getenv("SUITE_API_KEY", "") | ||
|
|
||
|
|
||
| def get_db() -> Generator[sqlite3.Connection, None, None]: |
There was a problem hiding this comment.
suggestion (bug_risk): Die Datenbankverbindung wird nach Gebrauch nicht geschlossen, was zu Ressourcenlecks führen kann.
Bitte aktualisiert get_db, um sicherzustellen, dass die Datenbankverbindung nach Gebrauch ordnungsgemäß geschlossen wird, z.B. durch die Verwendung eines Kontextmanagers.
Original comment in English
suggestion (bug_risk): Database connection is not closed after use, which may lead to resource leaks.
Please update get_db to ensure the database connection is properly closed after use, such as by using a context manager.
| All endpoints require the `X-API-Key` header: | ||
| ```bash | ||
| curl -H "X-API-Key: your-api-key" http://127.0.0.1:8090/endpoint |
There was a problem hiding this comment.
security (curl-auth-header): Ein potenzielles Autorisierungstoken wurde in einem Curl-Befehlsheader entdeckt, was die über Curl aufgerufene Ressource kompromittieren könnte.
Quelle: gitleaks
Original comment in English
security (curl-auth-header): Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
Source: gitleaks
| if not (len(v) == 17 and v.count(':') == 5): | ||
| raise ValueError('Invalid MAC address format') | ||
| # Simple MAC address validation (XX:XX:XX:XX:XX:XX) | ||
| if not (len(v) == 17 and v.count(":") == 5): |
There was a problem hiding this comment.
suggestion (code-quality): Vereinfacht den logischen Ausdruck unter Verwendung der De Morgan'schen Gesetze (de-morgan)
Suggested change
| if not (len(v) == 17 and v.count(":") == 5): | |
| if len(v) != 17 or v.count(":") != 5: |
Original comment in English
suggestion (code-quality): Simplify logical expression using De Morgan identities (de-morgan)
Suggested change
| if not (len(v) == 17 and v.count(":") == 5): | |
| if len(v) != 17 or v.count(":") != 5: |
|
|
||
| # Start core DNS monitoring | ||
| try: | ||
| dns_thread = threading.Thread( |
There was a problem hiding this comment.
issue (code-quality): Duplizierten Code in eine Funktion extrahieren (extract-duplicate-method)
Original comment in English
issue (code-quality): Extract duplicate code into function (extract-duplicate-method)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Entferne übrig gebliebene Konfliktmarker und führe eine umfassende Bereinigung im gesamten Repository durch: überarbeite das Installer-Skript, optimiere die FastAPI-App und Pydantic-Schemas, räume die Test-Suite und freigegebenen Module auf und überarbeite die README-Dokumentation.
Enhancements:
Documentation:
Tests:
Chores:
Original summary in English
Summary by Sourcery
Remove leftover conflict markers and perform a broad cleanup across the repository: refactor the installer script, streamline FastAPI app and pydantic schemas, tidy test suite and shared modules, and overhaul the README documentation.
Enhancements:
Documentation:
Tests:
Chores: