Skip to content

docs(api): printers.yaml weg, Drucker in DB + /admin/printers Admin-UI (#124) [DRAFT]#125

Merged
strausmann merged 37 commits into
mainfrom
spec/printers-yaml-to-db
Jun 20, 2026
Merged

docs(api): printers.yaml weg, Drucker in DB + /admin/printers Admin-UI (#124) [DRAFT]#125
strausmann merged 37 commits into
mainfrom
spec/printers-yaml-to-db

Conversation

@strausmann

Copy link
Copy Markdown
Owner

Draft-Spec für Issue #124

Dieser PR enthält nur den Spec-Entwurf für die Migration von printers.yaml in die DB plus eine neue Admin-UI /admin/printers/. Keine Code-Änderungen — Review-Grundlage für die 4 Fachteams.

Issue: #124
Brainstorming: Session vom 2026-06-14 mit @strausmann

Kern-Entscheidungen (User-bestätigt)

  1. KEIN env bootstrap. HUB_PRINTERS_JSON wurde explizit verworfen — Operator legt Drucker nur über die Admin-UI an.
  2. ID-Pattern erweitert: derive_printer_id(model, host, port, created_at). Bestandsdrucker behalten ihre alten UUIDs (created_at war damals nicht im Salt), neue Drucker bekommen kollisionssicheren UUID auch bei IP/Port-Wiederverwendung.
  3. UI-Edit nur für variable Felder: name, connection.{host,port,snmp}, queue.timeout_s, cut_defaults.half_cut, enabled. slug/model/backend/id bleiben immutable nach Create.
  4. Plugin-Architektur unangetastet: Druckermodelle bleiben Compile-Time-Plugins (ptouch, brother_ql). Admin-UI Model-Dropdown wird aus Plugin-Registry gefüllt.
  5. Audit-Tabelle printers_audit analog Hangar layouts_audit.
  6. Hangar bleibt unangetastet — konsumiert weiter GET /api/printers.

Was sich konkret ändert

Komponente Aktion
app/services/printer_config_loader.py gelöscht
app/db/lifespan.py::upsert_runtime_printers() gelöscht
app/schemas/printer_config.py gelöscht
/etc/printer-hub/printers.yaml Volume-Mount gelöscht
printers.yaml aus /docker/stacks/hangar-print-hub/config/ gelöscht
app/services/printer_admin_service.py neu
app/api/routes/admin_printers.py neu (JSON-API)
app/web/routes/admin_printers.py neu (HTML-UI)
app/templates/admin_printers/ neu (Jinja2)
Alembic-Migration printers_audit neu

Review-Fokus für Fachteams

  • ops: Compose-Migration sauber? Stack-Restart-Pfad ok? Backup/Rollback für Bestandsdrucker?
  • network: Pangolin SSO + Header-Auth Bypass für API-Tools (/api/v1/admin/printers) wie in pangolin-resource-standard.md?
  • storage: DB-Migration kollidiert nicht mit bestehender Hub-DB-Backup-Strategie? Audit-Tabelle Größe vertretbar?
  • ansible/code-quality: Service-Design clean? Test-Strategie ausreichend? Plugin-Registry-Ansatz tragfähig?

Closes-Spec-of #124

@gemini-code-assist

Copy link
Copy Markdown

Summary of Changes

Hello, 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 introduces a design specification for transitioning the printer management system from a file-based configuration to a database-driven model. By removing the dependency on 'printers.yaml' and implementing a dedicated Admin UI, the system gains more flexibility and improved management capabilities for printer instances. The changes focus on streamlining the operational workflow while maintaining compatibility with existing Hangar services.

Highlights

  • Migration to Database: The printer configuration is being migrated from a static 'printers.yaml' file to the database, which will become the sole source of truth.
  • New Admin UI: A new administrative interface at '/admin/printers/' is being introduced to allow operators to create, edit, and delete printer configurations directly.
  • Audit Logging: A new 'printers_audit' table is being added to track all changes made to printer configurations, ensuring accountability and traceability.
  • Removal of Legacy Code: The 'printer_config_loader.py' service and 'upsert_runtime_printers' logic are being removed as they are no longer needed with the database-driven approach.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize the 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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. 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.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces a design specification for migrating printer configurations from printers.yaml to a database-backed system with a dedicated Admin UI. The review feedback highlights privacy violations in the document, specifically the use of real domains (print-hub.strausmann.cloud and hangar.strausmann.cloud) in the smoke-test section, which should be replaced with neutral placeholders like example.com to comply with the repository's privacy guidelines.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.


1. PR merge → CI green
2. Dockhand: `down_stack(hangar-print-hub)` → Volume-Mount `printers.yaml` entfernen via `update_stack_compose` → `start_stack`
3. Browser: `https://print-hub.strausmann.cloud/admin/printers` → Liste der 2 Bestandsdrucker

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Gemäß den Datenschutzrichtlinien des Repositories (Priorität 1) und den allgemeinen Regeln dürfen keine echten Hostnamen, Domains oder LAN-IPs in der Dokumentation oder im Code verwendet werden. Bitte ersetzen Sie print-hub.strausmann.cloud durch eine neutrale Dokumentations-Domain wie print-hub.example.com.

Vorschlag:
3. Browser: https://print-hub.example.com/admin/printers` → Liste der 2 Bestandsdrucker`

References
  1. Datenschutzverletzungen. Markieren Sie alle fest einprogrammierten LAN-IPs, echten Hostnamen, echten Domains, echten Token oder personenbezogenen Daten (PII). Das Netzwerk des Maintainers darf aus diesem Repository nicht ableitbar sein. (link)
  2. Use RFC 5737 documentation IPs and 'example.com' placeholders instead of real LAN IPs, hostnames, or domains in documentation and code to maintain privacy.

3. Browser: `https://print-hub.strausmann.cloud/admin/printers` → Liste der 2 Bestandsdrucker
4. Edit `brother-p750w` → ändere Hostname testweise auf `192.0.2.1` → Save → Reload → Wert übernommen
5. Edit zurück auf echten Hostnamen
6. Hangar `/admin/layouts/` → unverändert, `https://hangar.strausmann.cloud/` Print-Buttons funktionieren

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Gemäß den Datenschutzrichtlinien des Repositories (Priorität 1) und den allgemeinen Regeln dürfen keine echten Hostnamen, Domains oder LAN-IPs in der Dokumentation oder im Code verwendet werden. Bitte ersetzen Sie hangar.strausmann.cloud durch eine neutrale Dokumentations-Domain wie hangar.example.com.

Vorschlag:
6. Hangar /admin/layouts/→ unverändert,https://hangar.example.com/` Print-Buttons funktionieren`

References
  1. Datenschutzverletzungen. Markieren Sie alle fest einprogrammierten LAN-IPs, echten Hostnamen, echten Domains, echten Token oder personenbezogenen Daten (PII). Das Netzwerk des Maintainers darf aus diesem Repository nicht ableitbar sein. (link)
  2. Use RFC 5737 documentation IPs and 'example.com' placeholders instead of real LAN IPs, hostnames, or domains in documentation and code to maintain privacy.

@strausmann

Copy link
Copy Markdown
Owner Author

Spec-Review: network-Team

Status: NEEDS_FIXES

Findings

CRITICAL

C1 — Pangolin Remote-User Header: exakter Name fehlt im Spec

Spec sagt updated_by wird aus "Pangolin Remote-User Header" befüllt. Pangolin setzt Header-Namen je nach Konfiguration unterschiedlich. Im HomeLab ist der korrekte Header-Name zu verifizieren — wenn er falsch ist, bleibt updated_by dauerhaft leer und der Audit-Trail ist wertlos.

Aktion: Pangolin-Dashboard → Org → Einstellungen → Header-Name notieren und im Spec exakt benennen. Wahrscheinlich X-Pangolin-User, aber muss gegen Live-System verifiziert werden (nie raten).

C2 — JSON-API hinter Pangolin SSO oder nicht?

Spec listet JSON-API (GET/POST/PUT/DELETE /api/v1/admin/printers) mit "API-Key (admin scope)"-Auth — aber sagt nicht ob diese Routes ebenfalls hinter der Pangolin-Resource (SSO) laufen oder direkt mit Bearer-Token erreichbar sein sollen.

Wenn die JSON-API hinter derselben Pangolin-Resource wie die HTML-UI läuft: Tooling und Ansible-Aufrufe kommen nicht durch ohne den Header-Auth-Bypass (claude-automation Basic-Auth). Der Spec-Abschnitt "Akzeptanzkriterien" erwähnt "Pangolin-Resource-Standard eingehalten (SSO + Header-Auth Bypass für API-Tools)" — aber die Implementierungs-Sektion beschreibt keinen Schritt dafür.

Explizit klären: Laufen HTML-UI und JSON-API auf derselben Pangolin-Resource? Wenn ja: Header-Auth-Bypass ist Pflicht und braucht einen eigenen Schritt im Plan (Blueprint-Label + Vaultwarden-Item).

HIGH

H1 — CSRF-Schutz für HTML-Form-POSTs fehlt im Spec

Spec definiert POST /admin/printers, POST /admin/printers/{slug} und POST /admin/printers/{slug}/delete als Browser-Forms ohne CSRF-Token. Pangolin-SSO authentifiziert die Session, schützt aber nicht vor Cross-Site-Request-Forgery: Ein Operator der gleichzeitig eine fremde Seite öffnet, kann durch präparierten Link unbemerkt Drucker-Daten verändern oder löschen.

Aktion: CSRF-Token-Mechanismus (z.B. itsdangerous, FastAPI-Security-Middleware oder ein Session-Token in Hidden-Field) muss in der Spec als Pflicht-Abschnitt ergänzt werden — insbesondere für Delete-Confirm-Form.

H2 — Pangolin-Resource-Standard: Bestandsresource braucht Header-Auth-Bypass-Update

Die Resource print-hub.strausmann.cloud existiert bereits. Laut pangolin-resource-standard.md müssen ALLE Resources auth.basic-auth.user=claude-automation + Passwort gesetzt haben. Falls die bestehende Resource das noch nicht hat, ist ein Blueprint-Label-Update nötig — nicht eine neue Resource anlegen.

Aktion: Prüfen ob die Bestandsresource bereits auth.basic-auth hat (via mcp__pangolin-api__). Falls nicht: Schritt im Plan ergänzen. Wichtig: Änderung NUR in Blueprint-Labels (Compose), nie direkt per API — Newt würde API-Änderungen beim nächsten Sync überschreiben.

MEDIUM

M1 — LAN-Routing Hub → Drucker: Spec macht keine Aussage

Drucker liegen im LAN (172.16.x.x). Der Hub-Container läuft im traefik-public-Netz auf hhdocker02/03. Spec geht davon aus dass connection.host (eine LAN-IP) vom Hub-Container direkt erreichbar ist — ohne zu prüfen ob das VLAN-Routing das erlaubt.

Aktion: Bestätigen dass hhdocker02/03 → Drucker-VLAN geroutet ist (UniFi-Firewall-Regel). Falls nicht, braucht der Hub ein zweites Netzwerk oder eine Host-Route. Kein neues VLAN nötig, aber die Annahme muss explizit im Spec stehen.

M2 — Hangar → Hub-Kommunikation: Domain oder intern?

Spec sagt Hangar PrinterSync ruft GET /api/printers alle 5min auf — aber nicht über welche URL. Wenn Hangar den Aufruf über https://print-hub.strausmann.cloud macht (Pangolin-Route), braucht auch Hangar den Header-Auth-Bypass. Wenn intern (Container-Netzwerk oder Tailscale-direkt), ist das nicht nötig.

Aktion: URL für Hangar→Hub-Aufruf im Spec explizit benennen.

LOW

L1 — Healthcheck-Path für bestehende Pangolin-Resource prüfen

Spec entfernt den Volume-Mount printers.yaml und den PRINTER_CONFIG_PATH-Pfad aus dem Compose. Falls der Pangolin-Healthcheck auf einen Endpoint zeigt der printers.yaml indirekt nutzt (z.B. /api/printers auf frischer DB), könnte HC nach Deploy kurz unhealthy sein. Kein Blocker, aber im Smoke-Test explizit prüfen.

@strausmann

Copy link
Copy Markdown
Owner Author

Spec-Review: ops-Team

Status: NEEDS_FIXES

Findings

HIGH (sollte fixed werden)

1. Env-Var-Entfernung: Merge-Pflicht nicht adressiert

Abschnitt "Migration für Bestand, Phase 2" sagt: «printers.yaml Volume-Mount entfernen» und «PRINTER_CONFIG_PATH Env-Variable entfernen». Die Spec beschreibt nur das Entfernen aus dem Compose-Block, nicht den notwendigen Stack-Env-Update-Pfad.

Problem: update_stack_env hat PUT-Replace-Semantik (nicht PATCH). Wer nur PRINTER_CONFIG_PATH löscht, muss trotzdem alle anderen Variablen mitschicken — sonst werden sie gelöscht (bekannter Vorfall 2026-06-01, hangar-print-hub, 7 Variablen verloren).

Korrekter Migrations-Pfad muss explizit in der Spec stehen:

1. get_stack_env → alle vorhandenen Variablen lesen
2. PRINTER_CONFIG_PATH aus der Liste entfernen
3. update_stack_env mit MERGED-Liste (ohne PRINTER_CONFIG_PATH)
4. update_stack_compose (Volume-Mount entfernen), restart=False
5. down_stack + start_stack (NICHT restart — wendet keine Compose-Änderungen an)

Hinweis: restart_stack reicht hier nicht. Die Compose ändert sich (Volume-Mount weg) — das erfordert ein recreate (down + start).

2. Pre-Deploy-DB-Snapshot fehlt

Abschnitt «Migration für Bestand» sagt «Snapshot DB-Inhalt sichern» ohne konkreten Befehl. Bei einem Fresh-Deploy auf leerer DB (z.B. nach versehentlichem DELETE FROM printers) gibt es keinen Recovery-Pfad zurück zu den YAML-konfigurierten Druckern — die Datei soll ja gelöscht werden.

Empfehlung: Konkrete Pre-Deploy-Anweisung ergänzen, z.B.:

docker exec hangar-print-hub-db-1 pg_dump -U hub hub -t printers > printers-backup.sql

Alternativ: Explizit festhalten, dass das PBS-Tagesbackup als Rollback ausreicht (wenn das stimmt).

MEDIUM (Discussion)

3. Watchtower-Timing während Migration

Der Smoke-Test-Pfad (Abschnitt «Smoke-Test Production», Schritt 2) lässt Watchtower aktiv. Watchtower auf hhdocker03 läuft mit Scope hangar-print-hub. Wenn Watchtower zwischen down_stack und start_stack ein Image-Update triggert, lädt er die neue Compose ohne Volume-Mount — das ist unkritisch wenn der Timing-Window klein ist, sollte aber explizit dokumentiert sein.

Optionen: Watchtower kurz pausieren während Migration, oder klarstellen warum das Fenster unkritisch ist.

4. Health-Endpoints nicht adressiert

Die Spec führt Admin-UI-Routen mit DB-Operationen ein, nennt aber keinen Health-Endpoint für Uptime-Kuma/Prometheus-Alerting bei 500-Fehlern oder DB-Verbindungsabbruch im laufenden Betrieb. Nicht blockierend für #124, aber als Follow-up-Issue empfohlen.

5. Audit-Tabelle: Retention-Strategie fehlt

Abschnitt «Audit-Tabelle printers_audit» hat created_at-Index, aber keine Aussage zu Cleanup/Archivierung. Bei seltenen Drucker-Änderungen kein akutes Problem — trotzdem sollte die Spec oder ein Follow-up-Issue klären ob Audit-Rows unbegrenzt wachsen.

LOW / Suggestions

6. Compose-Validation-Reihenfolge

Dockhand validiert beim update_stack_compose gegen die aktuelle Stack-Env. Wenn PRINTER_CONFIG_PATH noch als required-Variable im Compose-environment-Block steht, schlägt Validation fehl. Spec sollte festhalten: Compose-Block-Entfernung vor Env-Var-Entfernung.

Was gut ist

  • Abschnitt «Startup-Flow (neu)» ist klar: leere Tabelle = kein Fehler, kein Sync — richtiger Ansatz
  • UUID-Stabilität für Bestandsdrucker explizit zugesagt (Formel ohne created_at) — schützt Hangar-Referenzen
  • Risiko-Tabelle adressiert R1 (Hangar-Ref auf gelöschte Drucker) und R2 (API-Key-Auth) als offene Punkte — transparent
  • Test-Strategie (Unit + Integration + E2E + Smoke) ist vollständig, fresh_install_e2e ist genau der richtige Smoke-Path

@strausmann

Copy link
Copy Markdown
Owner Author

Spec-Review: storage-Team

Status: NEEDS_FIXES

Findings

CRITICAL

[C1] JSONB in SQLite existiert nicht

Sowohl printers (bestehend) als auch printers_audit (neu) verwenden JSONB als Spaltentyp. SQLite kennt kein JSONB — es wird stillschweigend als TEXT gespeichert, d.h. keine JSON-Validierung, kein Index auf JSON-Pfade. Das ist für dieses Setup (single-replica SQLite) funktional akzeptabel, aber die Spec sollte das explizit benennen: JSONBJSON TEXT (SQLite-kompatibel). Analog Hangar Phase 1k.1b wo beide Dialekte dokumentiert wurden. Falls Postgres jemals in Scope kommt, braucht es eine Dialekt-aware Migration.

Aktion: Spec-Kommentar in Schema-Abschnitt ergänzen: "SQLite speichert JSONB als TEXT, keine JSON-Constraints". Alembic-Migration entsprechend mit sa.Text() schreiben, nicht JSONB.

HIGH

[H1] Credentials in before_json / after_json — Datenleck-Risiko

Der Delete-Flow speichert before_json = row_json in printers_audit. Die connection-Spalte enthält snmp.community (typischerweise "public", aber konfigurierbar). Bei Backup-Diebstahl (PBS-Snapshot oder SQLite-Dump) sind diese Credentials im Klartext im Audit-Trail sichtbar — für alle jemals gelöschten Drucker dauerhaft.

Aktion: Spec muss entscheiden: (a) connection.snmp.community aus before_json/after_json redaktieren ("***"), oder (b) explizit dokumentieren dass SNMP-Community als unkritisch eingestuft wird (HomeLab-Kontext). Option (b) wäre für dieses Setup vertretbar, muss aber bewusste Entscheidung sein.

[H2] SELECT … FOR UPDATE existiert in SQLite nicht

Update- und Delete-Flow zeigen SELECT … WHERE slug=? FOR UPDATE. SQLite unterstützt FOR UPDATE nicht — der ORM wirft entweder einen Fehler oder ignoriert das Hint stillschweigend. SQLite-Locking läuft über BEGIN IMMEDIATE / BEGIN EXCLUSIVE auf Datenbankebene, nicht auf Zeilenebene.

Aktion: Spec muss beschreiben wie Concurrent-Write-Protection tatsächlich implementiert wird: BEGIN IMMEDIATE via SQLAlchemy with_for_update()-Fallback oder explizite Session-Isolation. Bei single-process FastAPI mit AsyncSession ist das Risiko gering, aber die Spec sollte nicht implizieren dass Row-Level-Locking vorhanden ist.

MEDIUM

[M1] Pre-Deploy-Snapshot nicht spezifiziert

Spec sagt "Snapshot DB-Inhalt sichern" (Phase 1) ohne konkreten Mechanismus. PBS-Snapshot der VM läuft regulär, aber der Zeitpunkt ist nicht garantiert frisch. Empfehlung: Expliziten sqlite3 /data/printer-hub.db ".backup /tmp/printer-hub-pre-deploy-20260614180912.db" Step in die Deployment-Smoke-Test-Checkliste aufnehmen (Schritt 2 vor down_stack).

[M2] Transaktions-Atomicity nicht explizit dokumentiert

Spec zeigt Create/Update/Delete-Flows mit implicit COMMIT am Ende, aber erwähnt nicht explizit dass INSERT INTO printers und INSERT INTO printers_audit in einer gemeinsamen SQLAlchemy-Transaktion liegen. Wenn der Audit-INSERT fehlschlägt, muss der Drucker-INSERT rolliert werden — und umgekehrt. Analog Hangar Phase 1k.1b sollte die Spec schreiben: "Beide INSERTs in einer BEGIN/COMMIT-Transaktion — Audit-Fehler rollt Drucker-Änderung zurück".

LOW

[L1] Indexe bei <1000 Rows redundant aber harmlos

Zwei Indexe auf printers_audit bei realistisch <50 KB Daten (10 Drucker × 20 Edits × 10 Jahre). Kein Performance-Problem, aber Spec könnte einen Satz ergänzen: "Indexe für zukünftige Skalierung, aktuell nicht erforderlich".


storage-team review — 2026-06-14

@codecov

codecov Bot commented Jun 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.20370% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.71%. Comparing base (2ff51d2) to head (51bc230).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
backend/app/api/routes/admin_printers_api.py 73.49% 22 Missing ⚠️
backend/app/services/printer_model_registry.py 84.00% 4 Missing and 4 partials ⚠️
backend/app/services/audit_redaction.py 82.60% 2 Missing and 2 partials ⚠️
backend/app/main.py 93.75% 2 Missing ⚠️
backend/app/services/printer_admin_service.py 98.56% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #125      +/-   ##
==========================================
- Coverage   89.30%   87.71%   -1.60%     
==========================================
  Files          91       94       +3     
  Lines        4263     4574     +311     
  Branches      368      400      +32     
==========================================
+ Hits         3807     4012     +205     
- Misses        358      459     +101     
- Partials       98      103       +5     
Components Coverage Δ
Printer Backends (transport) 85.71% <100.00%> (-1.16%) ⬇️
Printer Models (drivers) 88.20% <ø> (ø)
Services 91.30% <94.50%> (+0.09%) ⬆️
REST API 83.31% <74.11%> (-1.67%) ⬇️
Pydantic Schemas 100.00% <100.00%> (ø)
Integration Plugins 100.00% <ø> (ø)
Files with missing lines Coverage Δ
backend/app/api/routes/print.py 98.13% <100.00%> (+0.03%) ⬆️
backend/app/db/engine.py 76.00% <ø> (ø)
backend/app/db/lifespan.py 63.33% <ø> (-24.31%) ⬇️
backend/app/models/printer.py 100.00% <100.00%> (ø)
backend/app/printer_backends/exceptions.py 100.00% <100.00%> (ø)
backend/app/repositories/printers.py 97.05% <100.00%> (+0.28%) ⬆️
backend/app/schemas/printer_admin.py 100.00% <100.00%> (ø)
backend/app/services/backend_router.py 98.38% <100.00%> (+1.32%) ⬆️
backend/app/services/print_service.py 90.27% <100.00%> (+0.57%) ⬆️
backend/app/services/printer_identity.py 100.00% <100.00%> (ø)
... and 5 more

... and 5 files with indirect coverage changes

Flag Coverage Δ
backend 87.71% <91.20%> (-1.60%) ⬇️

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


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30adf7d...51bc230. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@strausmann

Copy link
Copy Markdown
Owner Author

Spec-Review: code-quality / Architektur

Status: NEEDS_FIXES


Findings

CRITICAL

C1 — derive_printer_id Signatur-Bruch ohne Backwards-Compat-Pfad

Die Spec erweitert die Signatur auf derive_printer_id(model, host, port, created_at) (Abschnitt "Komponenten → ID-Generierung"), aber der existierende Code printer_identity.py:21 hat def derive_printer_id(model: str, host: str, port: int) -> UUID. Es gibt 4 Aufruf-Stellen (lifespan.py:199, 3 Test-Files), die alle die 3-arg-Variante nutzen.

Die Spec behandelt das als "Bestandsdrucker behalten ihre alte UUID — Migration berechnet sie NICHT neu" — macht aber keine Aussage darüber, wann/wo die 4-arg-Variante statt der 3-arg-Variante aufgerufen wird. Wird upsert_runtime_printers() gleichzeitig entfernt (ja, laut Spec), dann ist der Aufruf in lifespan.py:199 weg. Bleiben die 3 Test-Files: die müssen ebenfalls angepasst oder entfernt werden.

Pflicht: Spec muss explizit benennen: "Die bestehende 3-arg-Funktion wird durch eine überlastete/neue 4-arg-Variante ersetzt; alle Test-Aufruf-Stellen in test_lifespan_seeds_and_upserts.py und test_lifespan_printer_upsert.py werden entfernt, da upsert_runtime_printers() selbst entfernt wird."

C2 — FK-Kaskade: DELETE blockiert oder zerstört Job-History

job.printer_id und print_batch.printer_id sind foreign_key="printers.id" ohne ON DELETE CASCADE oder SET NULL. Ein DELETE FROM printers WHERE id=? schlägt damit bei SQLite mit einem FK-Constraint-Fehler fehl (oder lässt verwaiste Rows bei deaktivierten FK-Checks).

Die Spec adressiert das im Delete-Flow nicht — sie schreibt nur "Operator-Verantwortung" für Hangar-Layouts (Abschnitt "Implikation für Hangar"). Auch printer_state und printer_status_cache haben FK auf printers.id.

Pflicht: Spec muss entscheiden: (a) Soft-Delete (enabled=false) statt hartem DELETE, oder (b) Service prüft vor DELETE ob jobs/print_batch/preset Rows existieren und wirft 409, oder (c) Alembic-Migration ergänzt ON DELETE SET NULL / ON DELETE CASCADE pro Tabelle. Ohne diese Entscheidung wird delete_printer() in Produktion inkonsistent fehlschlagen.


HIGH

H1 — Pydantic-Schemas undefiniert

Spec nennt PrinterCreatePayload und PrinterUpdatePayload im Service-Interface, definiert aber keine Felder. Aus dem Form-Table (Abschnitt "Web-Routes") lassen sich die Felder ableiten, aber die Spec fehlt für:

  • slug-Regex-Validator (^[a-z0-9-]+$) — muss im Schema als @field_validator stehen, nicht nur in der Form-Beschreibung
  • Port-Range-Validator (1–65535)
  • connection.snmp.community als required wenn discover=true (cross-field)
  • Welche Felder PrinterUpdatePayload enthält vs. welche bei Update ignoriert werden (immutable: slug, model, backend, id)

Pflicht: Spec ergänzen mit zumindest den Validator-Regeln und dem Update-Subset. Implementer soll keine Schema-Entscheidungen treffen müssen.

H2 — Immutable Fields: Service-seitige Durchsetzung fehlt

Abschnitt "Update-Flow" Step 5c: UPDATE printers SET name=?, connection=?, enabled=?, updated_at=now(). Das impliziert, dass slug/model/backend/id beim Update nicht gesetzt werden. Aber die Spec sagt nicht explizit, dass update_printer() diese Felder aus dem Patch-Payload aktiv herausfiltert (statt nur nicht setzt).

Bei der JSON-API (PUT /api/v1/admin/printers/{slug}) kann ein Aufrufer {"model": "QL-820NWB"} schicken — wenn der Service das stillschweigend ignoriert, ist das OK, muss aber so spezifiziert sein. Wenn der Service mit 422 antwortet, muss das im Error-Handling stehen.

Pflicht: Ein Satz in "Komponenten → PrinterAdminService": "Felder slug, model, backend, id im PrinterUpdatePayload werden vom Service ignoriert (kein 422); PrinterUpdatePayload enthält diese Felder nicht."


MEDIUM

M1 — Test-Coverage-Schwellen fehlen (test-coverage-pflicht.md)

Spec listet Test-Kategorien, aber keine Coverage-Schwellen. Laut test-coverage-pflicht.md brauchen Mutation-Funktionen (create/update/delete) mindestens 85% und müssen Happy + Error-Path + DB-Error abdecken.

Aktuell fehlt in der Test-Sektion: (a) DB-Error-Pfad (z.B. Session rollback bei IntegrityError), (b) Network/Timeout-Pfad für den Audit-Write (was wenn printers_audit INSERT fehlschlägt — rolled back der gesamte Printer-Write?). Die Spec muss diese Szenarien explizit benennen.

M2 — created_at Timezone-Spezifikation fehlt

Spec Abschnitt Create-Flow Step 5a: created_at = now() — unklar ob datetime.now() (naiv, lokale TZ) oder datetime.now(timezone.utc) (aware). Da created_at.isoformat() in den UUIDv5-Salt eingeht, ist das Format deterministisch relevant: 2026-06-14T10:00:00 vs. 2026-06-14T10:00:00+00:00 erzeugen verschiedene UUIDs für denselben Zeitpunkt.

Pflicht: Spec muss schreiben: created_at = datetime.now(timezone.utc) — explizit aware, UTC.

M3 — Plugin-Registry-Kopplung nicht diskutiert

Spec Abschnitt 4: "Quelle: ptouch.PRINTERS (ptouch-py) und brother_ql.MODELS" — das ist direkte Kopplung an die interne API dritter Pakete. Spec markiert R3 als "Verifikation im Plan-Schritt", trifft aber schon eine Implementierungsentscheidung ohne zu klären was passiert wenn ptouch.PRINTERS ein privates Attribut ist oder sich umbenennt.

Alternative (robuster): Jedes Backend-Plugin exportiert selbst eine SUPPORTED_MODELS: list[str]-Konstante. Spec sollte die Kopplung explizit als "akzeptiertes Risiko für #124, Plugin-abstraktion out-of-scope" markieren — aktuell steht das nicht da.


LOW

L1 — Audit-Tabelle: kein FK auf printers.id

printers_audit.printer_id ist UUID NOT NULL aber kein FOREIGN KEY. Damit können Audit-Rows für gelöschte Drucker bestehen bleiben — das ist für ein Audit-Trail das gewünschte Verhalten. Spec sollte das explizit begründen: "kein FK absichtlich, damit Audit-History nach Drucker-Löschung erhalten bleibt."

L2 — i18n-Politik

Spec Abschnitt "Out of Scope": "Mehrsprachigkeit der Admin-UI (deutsch only)" — gut. Spec sollte ergänzen ob Error-Messages aus Pydantic (422 responses) ebenfalls auf Deutsch übersetzt werden oder englisch bleiben. Aktuell unklar für Implementer.


Was gut ist

  • Migrations-Ansatz ist sauber: Kein Daten-Migration-Risiko, weil DB bereits gefüllt ist. Phase 1/2/3 klar getrennt.
  • Startup-Flow-Vereinfachung: lifespan.py verliert upsert_runtime_printers() ohne Ersatz — das ist genau richtig. Der Kommentar-Docblock in lifespan.py (Call-Order 1–7) muss im gleichen PR aktualisiert werden.
  • Audit-Pattern analog Hangar: Bewusste Entscheidung als "akzeptabel für separate Repos ohne Shared Library" explizit zu markieren (R5-ähnlich) wäre noch wünschenswert, aber das Pattern selbst ist konsistent.
  • JSON-API parallel zu HTML-UI: Klug — Ansible-Automation kann sofort Drucker anlegen ohne Browser.
  • Deutsche Umlaute: Spec verwendet ä/ö/ü/ß durchgängig korrekt.

@strausmann

Copy link
Copy Markdown
Owner Author

Round-2 Spec-Review: ops-Team

Status: APPROVE (mit einem LOW-Hinweis)


Round-1 Findings — Verifikation

  • H1 env-merge-Pflicht: adressiert. Das Python-Snippet zeigt korrekt get_stack_env → filter(key != PRINTER_CONFIG_PATH) → update_stack_env. Eine explizite Vor-/Nach-Verifikation (Key-Anzahl loggen) fehlt im Snippet, aber die Rule dockhand-stack-env-merge.md schreibt das vor — der implementierende Agent muss sie gelesen haben. Für die Spec reicht das Merge-Pattern aus.
  • H2 down/start: adressiert. Reihenfolge down_stack → update_stack_compose → start_stack ist korrekt. Rollback-Pfad (Punkt 4: down_stack + start_stack) ebenfalls vorhanden.
  • M1 sqlite3 .backup: adressiert. Befehl korrekt: docker exec ... sqlite3 /data/printer-hub.db '.backup /data/printer-hub.db.bak-pre-124'. WAL-Modus: SQLite .backup-Kommando ist WAL-safe (es benutzt die Backup-API intern, nicht file copy). Pfad /data/printer-hub.db.bak-pre-124 liegt im gleichen Volume — ist OK, PBS sichert das Verzeichnis ohnehin, und der explizite docker cp danach zieht eine Kopie auf den Host.

Neue Round-2 Findings

LOW — down_stack-Fehlerbehandlung nicht spezifiziert

Spec sagt down_stack → update_stack_compose → start_stack, aber: wenn down_stack fehlschlägt (Container hängt, Timeout), ist der Stack halb-tot. Der Rollback-Pfad im Spec setzt voraus dass der Stack schon unten ist. Empfehlung für den implementierenden Agent: vor down_stack prüfen ob alle Container healthy/running sind, und bei Fehler manuell docker kill via SSH als Fallback dokumentieren — nicht im Spec selbst ändern nötig, reicht als Deploy-Hinweis.

LOW — CSRF und /healthz-Konflikt unwahrscheinlich aber prüfenswert

Starlette-CSRF-Middleware prüft nur Requests mit Content-Type application/x-www-form-urlencoded oder multipart/form-data. Der Healthcheck GET /healthz ist ein GET — kein CSRF-Token nötig, kein Konflikt. Trotzdem: beim ersten Deploy curl -fsS https://print-hub.strausmann.cloud/healthz unmittelbar nach start_stack ausführen bevor Smoke-Tests. Der Spec listet das als Schritt 3 der Produktion-Smoke-Tests — passt.

ACCEPTED — Soft-Delete ohne Cleanup-Job (R6)

Audit-Tabelle wächst nie über ~30 KB bei realistischem Betrieb. Kein Cleanup-Job nötig. Akzeptiert.


Was gut ist

  • SNMP-Redaction via redact_secrets() ist explizit und erweiterbar — gut für künftige Secret-Felder.
  • Watchtower-Pause korrekt via set_container_auto_update(policy='never') vor Migration, 'any' nach Verifikation — der Zeitraum (~5 Min für Migration) ist kurz genug.
  • Alembic-Migration läuft im Startup-Flow vor allem anderen — Container wird erst HEALTHY wenn Migration durch ist. Healthcheck-Timeout ist Implementierungsdetail, aber der Ansatz ist korrekt.
  • Hangar→Hub intern via Container-Netz klar dokumentiert — kein Pangolin-Pfad, kein Header-Auth nötig für Hangar-Sync.

ops-Team: APPROVE

@strausmann

Copy link
Copy Markdown
Owner Author

Round-2 Spec-Review: network-Team

Status: NEEDS_FIXES (1 HIGH, 1 MED, 2 LOW — aber alle klar lösbar, kein Architektur-Problem)


Round-1 Findings — Verifikation

Finding Status Notiz
C1 Pangolin Header-Name adressiert Remote-User + X-Pangolin-Token korrekt aus app/auth/dependencies.py. sso_trust_token-Check gegen Vault-Item-Name ist Laufzeit-Config, nicht Spec-Pflicht. OK.
C2 JSON-API Auth-Pfad adressiert Drei-Pfad-Entscheidung dokumentiert. Header-Auth via Blueprint Labels (nicht API). Feedback pangolin_labels_source_of_truth korrekt angewendet.
H3 CSRF-Schutz adressiert Starlette CSRF Middleware mit SameSite=Strict. Differenzierung SSO-Session (CSRF geprüft) vs. Basic-Auth/API-Key (CSRF-frei, weil kein Browser-Origin) — korrekt.
H4 Bestandsresource Header-Auth teilweise Spec nennt R2 als Risiko + Phase-0-Live-Check via mcp__pangolin-api__resource_by_resourceId. Konkrete Response-Felder fehlen (siehe H4-Nachschärfung unten).

Neue Round-2 Findings

[HIGH] N1: Healthcheck-Labels fehlen im Spec-Blueprint-Beispiel

Spec zeigt Blueprint-Labels in Sektion "Authentifizierung" ohne den Healthcheck-Block. pangolin-resource-standard.md macht healthcheck.{enabled,hostname,path,port,interval} seit Newt v1.18.4 verbindlich. Das Standardbeispiel label-printer-hub auf hhdocker02 hat diese Labels. Ohne sie zeigt Pangolin den Service als "unhealthy" — was HC-basierte Alerts und Pangolin-interne Entscheidungen betrifft.

Fix: Blueprint-Beispiel in Sektion "Authentifizierung" um den Healthcheck-Block erweitern (Container-Name print-hub, Port 8000, Pfad /healthz).

[MED] N2: Hangar→Hub Container-DNS-Name nicht verifiziert

Spec sagt http://print-hub:8000/api/printers. Der Dockhand-Stack heißt hangar-print-hub, der Service im Compose print-hub — Docker-Compose setzt den Container-DNS-Namen aus Compose-Service-Name, nicht Stack-Name. Das wäre korrekt wenn der Service in compose.yaml tatsächlich print-hub: heißt. Vor Implementation: docker inspect hangar-print-hub-print-hub-1 --format '{{.NetworkSettings.Networks}}' oder docker exec hangar-print-hub-hangar-1 nslookup print-hub verifizieren. Spec sollte diesen Verifikationsschritt in Phase-3-Smoke aufnehmen.

[LOW] N3: claude-automation Username — Standardisierung nicht referenziert

Spec definiert claude-automation als User, referenziert aber nicht explizit den HomeLab-Standard (label-printer-hub auf hhdocker02 als kanonisches Beispiel, Vault-Item-Namenskonvention Pangolin Header Auth - Print Hub). Kein Blocker — aber ein expliziter Verweis auf pangolin-resource-standard.md im Spec würde dem Implementierer ersparen, die Konvention neu erfinden zu müssen.

[LOW] N4: Bekannter Pangolin-Bug #3099 nicht erwähnt

Mit auth.sso-enabled=true + auth.basic-auth.* zeigt Pangolin (Issue fosrl/pangolin#3099, aktuell open) Browser-Usern einen Basic-Auth-Dialog statt SSO-Redirect. Cancel im Dialog bringt die SSO-Page. Für Browser-UX relevant — sollte im Spec als bekannter Zustand dokumentiert sein, damit der Implementierer keinen Bug-Report öffnet.


Was gut ist

  • H1 env-merge + H2 down_stack statt restart exakt nach dockhand-stack-env-merge.md — vorbildlich.
  • feedback_pangolin_labels_source_of_truth korrekt angewendet: Labels als Source of Truth, kein API-Set.
  • SSO-Label-Syntax (auth.sso-enabled=true, nicht auth.sso=true) korrekt.
  • CSRF-Differenzierung ist wasserdicht: Basic-Auth/API-Key können nicht aus Browser-Origin missbräuchlich verwendet werden.
  • Phase-0-Live-Check für Bestandsresource explizit genannt.

Pflicht vor Merge: N1 (Healthcheck-Labels) in Blueprint-Beispiel ergänzen. N2 als Verifikationsschritt in Phase-3-Smoke aufnehmen. N3 + N4 als Kommentar oder Risiko-Tabelleneintrag — kein Blocking.

@strausmann

Copy link
Copy Markdown
Owner Author

Round-2 Spec-Review: storage-Team

Status: NEEDS_FIXES (1 HIGH neu, 1 MED neu, Rest APPROVE)


Round-1 Findings — Verifikation

  • C3 SQLite/JSONB: ✅ adressiert — sa.JSON() in Audit-Migration korrekt, konsistent mit printers.connection.
  • H4 SNMP-Redaction: ⚠️ teilweise — siehe neues Finding H4-B unten.
  • H5 SELECT FOR UPDATE: ✅ adressiert — BEGIN IMMEDIATE-Ansatz ist korrekt für aiosqlite.

Neue Round-2 Findings

HIGH — H4-B: SNMP-Schema-Inkompatibilität (flach vs. verschachtelt)

Problem: Die neue PrinterConnection in der Spec ist flach (snmp_discover, snmp_community als Top-Level-Felder). Das bestehende YAML-Schema (printer_config.py) ist verschachtelt (snmp.discover, snmp.community). Die laufende upsert_runtime_printers() schreibt aktuell nur {host: ..., port: ...} in die DB — SNMP-Felder fehlen komplett in Bestandsrows.

Das führt zu zwei Problemen:

  1. redact_secrets(payload) sucht nach connection.snmp_community (flach), aber Bestandsdrucker aus YAML haben entweder kein SNMP-Feld oder eine andere Struktur in connection. → Redaction greift bei Bestand-Updates ggf. ins Leere.
  2. Spec zeigt connection.snmp_community im Audit-Kommentar aber snmp_discover/snmp_community im Pydantic-Schema. Kein explizites Statement was in connection JSON gespeichert wird vs. was im Pydantic-Payload ist.

Fix: Spec muss explizit definieren: (a) was genau in printers.connection gespeichert wird (alle Felder der PrinterConnection-Klasse, serialisiert via model_dump()?), und (b) ob redact_secrets auf das gespeicherte dict oder den Pydantic-dump operiert. Empfehlung: connection speichert exakt PrinterConnection.model_dump() — dann ist das Schema konsistent und Redaction robust.

MED — M7: BEGIN IMMEDIATE + SQLAlchemy session.begin() Zusammenspiel

Spec sagt async with session.begin() plus BEGIN IMMEDIATE für Update-Flow, aber beschreibt nicht wo BEGIN IMMEDIATE exakt ausgeführt wird. Bei aiosqlite/SQLAlchemy async öffnet session.begin() eine reguläre Transaktion. Ein explizites await session.execute(text('BEGIN IMMEDIATE')) innerhalb von session.begin() schlägt fehl (sqlite3.OperationalError: cannot start a transaction within a transaction).

Korrekte Umsetzung: Entweder Connection-Level-Event vor dem session.begin() (event.listen(engine.sync_engine, 'begin', lambda conn: conn.execute(text('BEGIN IMMEDIATE')))) oder isolation_level='IMMEDIATE' am Engine setzen. Spec sollte die gewählte Variante explizit nennen — beide sind valide, aber der Implementer muss es wissen.


Was gut ist

  • Soft-Delete / kein FK auf printers_audit.printer_id: sauber und richtig begründet.
  • Transaktion Rollback (M6): async with session.begin() rollt bei Exception automatisch zurück — Drucker-INSERT + Audit-INSERT sind atomar. Korrekt.
  • .backup-Befehl (M1): SQLite .backup synchronisiert bei aktivem WAL-Mode automatisch den Checkpoint — kein manuelles PRAGMA wal_checkpoint nötig. Ist korrekt so.
  • Slug-Recycling / Soft-Delete: Richtig erkannt dass ein disabled Drucker seinen Slug blockiert. Das Out-of-Scope-Statement in Sektion Out of Scope reicht.
  • Audit-Retention: <50KB/10 Jahre ist realistisch, kein Cleanup-Code nötig.

@strausmann

Copy link
Copy Markdown
Owner Author

Round-2 Spec-Review: code-quality

Status: NEEDS_FIXES (2 Medium, 1 High — kein neues CRITICAL)


Round-1 Findings — Verifikation

  • C4 derive_printer_id Backwards-Compat: ⚠️ teilweise adressiert — Spec benennt die 3 richtigen Test-Files. Im Live-Code existieren aber noch 2 weitere 3-arg-Aufrufer die die Spec übersieht:

    • tests/integration/test_lifespan_seeds_and_upserts.py:88derive_printer_id("PT-P750W", "192.0.2.50", 9100) (3-arg)
    • tests/integration/db/test_lifespan_printer_upsert.py:51,101,102,161 — mehrfach 3-arg
      Diese Tests testen upsert_runtime_printers (wird entfernt), sind also mit dem Löschen von upsert_runtime_printers hinfällig. Spec sollte sie explizit als "auch zu löschen" listen — sonst bricht CI beim Merge, weil die Tests gegen eine nicht mehr existente Funktion importieren.
  • C5 DELETE-Strategie (Soft-Delete): ⚠️ teilweise adressiert — Spec dokumentiert disable_printer korrekt. Aber: der bestehende PrintService prüft enabled nicht beim Submit eines Print-Jobs via UUID. app/services/print_service.py erhält eine printer_id UUID direkt und prüft nie ob enabled=true. Der Spec-Text sagt "PrintRequest mit UUID eines disabled Druckers → 409 printer disabled" — aber kein Implementierungsschritt nennt wo diese Prüfung eingebaut wird (Service? Repository? Route?). Das ist ein implementierender Subagent wird das übersehen.

  • H6 Pydantic-Payload-Felder:adressiertPrinterConnection ist flat (snmp_discover, snmp_community). Aber: bestehende Bestandsdrucker-Rows haben connection = {"host": ..., "port": ...} ohne SNMP-Keys (gesetzt von upsert_runtime_printers). Neuer PrinterConnection-Parser erwartet host+port als Pflichtfelder — das passt. SNMP-Felder sind optional (default False/None). Kein Breaking Change für Bestandsdaten.

  • H2 Immutable-Fields silent ignore:adressiert — Sektion "Komponenten" benennt das Verhalten. Fehlender Test-Case (Update mit anderem slug → 200 + DB unverändert) ist jetzt explizit in Testing-Sektion gelistet: "Versuch slug/model/backend zu ändern → wird ignoriert (kein 422)". ✅


Neue Round-2 Findings

HIGH

H-NEW-1: test_lifespan_seeds_and_upserts.py + test_lifespan_printer_upsert.py fehlen in der Lösch-Liste (C4-Nachzügler)

Beide Dateien importieren upsert_runtime_printers und derive_printer_id mit 3-arg-Signatur. Spec nennt nur 3 Files zum Löschen/Migrieren. Diese 2 Integration-Test-Files werden beim Merge gegen eine entfernte Funktion importieren → CI bricht. Spec muss tests/integration/test_lifespan_seeds_and_upserts.py und tests/integration/db/test_lifespan_printer_upsert.py in die Liste aufnehmen.

MEDIUM

M-NEW-1: C5 — PrintService.submit_print_job prüft enabled nicht

Der Spec-Satz "PrintRequest mit UUID eines disabled Druckers schlägt mit 409 printer disabled fehl" ist korrekt als Ziel. Aber im bestehenden Code (app/services/print_service.py) gibt es keine enabled-Prüfung — nur UUID-Lookup. Ohne einen expliziten Implementierungsschritt ("In PrintService.submit_print_job oder im Printer-Repository: vor Job-Eintrag prüfen ob printer.enabled=true, sonst raise 409") wird der Implementer das übersehen. Akzeptanzkriterien nennen es nicht. Ein Test-Case fehlt ebenfalls.

M-NEW-2: redact_secrets — kein Modul-Pfad, keine Test-Strategie

Spec sagt "Helper redact_secrets(payload: dict) -> dict im Service". Der Testing-Abschnitt listet redact_secrets-Unit-Tests explizit — gut. Aber der Modul-Pfad fehlt: lebt er in printer_admin_service.py (private Funktion) oder als eigenes app/services/audit_redaction.py? Ohne Festlegung wird der Implementer entweder eine private Methode (schlecht testbar ohne Import) oder ein eigenes Modul bauen. Coverage-Threshold ist nur für printer_admin_service.py definiert — wenn redact_secrets in ein separates Modul wandert, fehlt der Threshold-Eintrag. Empfehlung: app/services/audit_redaction.py mit eigenem 85%-Threshold (Mutation-Logic — ersetzt Secret-Werte).

LOW

L-NEW-1: CSRF-Test-Strategie zu vage

middleware/csrf.py hat 80% Coverage-Threshold aber kein konkretes Test-Pattern. Starlette-CSRF-Middleware ist ein Third-Party-Package — testen heißt: Token fehlt → 403, Token falsch → 403, Token korrekt → 200, JSON-API-Pfad bypass (Basic-Auth) → kein CSRF-Check. Spec sollte mindestens diese 3 Fälle in der Integration-Test-Liste nennen (3 davon fehlen, nur der POST /admin/printers ohne CSRF → 403 ist gelistet).

L-NEW-2: Trailing-Slash-Konvention für JSON-API nicht festgelegt

6 neue /api/v1/admin/printers/...-Endpunkte. FastAPI-Standard ist kein Trailing Slash, aber Spec zeigt inkonsistente Notation (mal …/disable, mal /api/v1/admin/printers). Einmal festlegen, damit OpenAPI-Schema sauber wird.


Was gut ist

  • SNMP-Schema-Konsistenz: Flaches PrinterConnection-Schema ist konsistent mit dem was upsert_runtime_printers in connection-JSON schreibt ({"host": ..., "port": ...}) — kein Migration-Risk für Bestandsdaten.
  • BEGIN IMMEDIATE für SQLite statt SELECT FOR UPDATE — korrekter Dialect-Workaround, gut begründet.
  • redact_secrets-Test explizit in Unit-Tests — das Richtige, nur der Modul-Pfad fehlt.
  • Migration-Schritte (Snapshot, down_stack, env-merge) sind vollständig und reproduzierbar.

Blockierend vor Umsetzung: H-NEW-1 (CI bricht sonst beim Merge) und M-NEW-1 (fehlendes Akzeptanzkriterium für disabled-Printer-Check).

@strausmann

Copy link
Copy Markdown
Owner Author

Round-3 Spec-Review: ops-Team

Status: APPROVE

Round-2 (APPROVE) bestätigt?

Ja. Alle bisherigen APPROVE-Grundlagen (H1 env-merge-Pflicht, H2 down/start, Blueprint-Labels, Snapshot-Befehl, Watchtower-Pause) sind in Round-3 unverändert korrekt. Keine der neuen Bausteine zieht das Round-2-OK in Zweifel.

Neue Round-3 Findings

LOW: WAL-Pragma + Backup-Artefakte (.wal/.shm)

Der Connect-Listener in engine.py setzt journal_mode=WAL. Falls die Production-DB bisher im Rollback-Journal-Modus läuft, switcht das erste Open den Modus und erzeugt .wal + .shm Dateien neben der .db. Der Spec-Backup-Befehl (sqlite3 .backup) ist korrekt: SQLite's .backup-Befehl handhabt WAL-Frames automatisch und erzeugt eine konsistente Kopie. Kein Anpassungsbedarf.

Einzige Lücke: Phase 1a sichert explizit printer-hub.db.bak-pre-124 — nach dem ersten Start existieren auch printer-hub.db-wal und printer-hub.db-shm. Der PBS-Job sichert /data/ sowieso vollständig. Kein Blocking-Issue, aber der Rollback-Pfad erwähnt nur die .db-Datei. Empfehlung (non-blocking): Rollback-Sektion um einen Hinweis erweitern: rm -f /data/printer-hub.db-wal /data/printer-hub.db-shm vor dem Restore, um WAL-Artefakte zu beseitigen.

LOW: Alembic-down für Phase-1b fehlt im Rollback-Pfad

Der Rollback-Pfad beschreibt SQLite-Restore + Compose-Revert. Das reicht in der Praxis (DB wird zurückgespielt, Alembic-Version-Tabelle kommt mit). Formal fehlt aber ein alembic downgrade -1 für den Fall dass jemand einen Rollback ohne vollständigen DB-Restore versucht. Da der Spec explizit sagt "SQLite-Restore aus Backup" als primären Rollback, ist das kein Blocking-Issue. Empfehlung (non-blocking): Im Rollback-Pfad explizit dokumentieren: "Alembic-down wird NICHT genutzt — ausschließlich DB-Restore-Pfad.

LOW: Container-DNS-Smoke-Schritt ist fragil bei abweichendem Compose-Service-Name

getent hosts print-hub funktioniert nur wenn der Compose-Service-Name tatsächlich print-hub heißt. Der Spec-Hinweis "falls Service-Name abweicht: Compose-Service-Definition prüfen" ist ausreichend. In modernen Compose-Versionen (Docker Compose v2) wird der Bindestrich-Container-Name verwendet (hangar-print-hub-print-hub-1), der DNS-Alias im Container-Netz ist aber der Service-Name (print-hub). Das ist korrekt. Non-blocking.

LOW: PrinterDisabledError → 409 (API-Contract-Änderung)

POST /api/v1/print gibt neu 409 statt nur 404 für disabled Drucker. Der Hangar-Client ruft diesen Endpoint aktuell aus einem enabled=true-gefilterten Cache auf — ein disabled Drucker sollte Hangar daher gar nicht erreichen. 409 ist trotzdem eine Contract-Änderung für Drittaufrufer. Spec stuft das korrekt als Out-of-Scope #124 ein und R1 dokumentiert das Risiko. Non-blocking; Hangar-Issue zum Hinweis auf 409-Handling wäre sauber.

Was gut ist

  • M7 (IMMEDIATE via Engine-Default): Die Strategie, isolation_level="SERIALIZABLE" auf Engine-Ebene zu setzen statt manuell BEGIN IMMEDIATE in Sessions zu injizieren, ist die korrekte SQLAlchemy+aiosqlite Lösung. Kein doppeltes BEGIN-Problem.
  • H8b Backfill-Idempotenz: if "snmp" not in conn_json verhindert Doppel-Writes bei Migrations-Wiederholung sauber.
  • Rollback-Vollständigkeit: Snapshot vor Deploy, lokale Kopie, Stack-Env-Merge-Revert — alle drei Schritte dokumentiert.
  • Phase-3-Verifikations-Checkliste: Die SQL-Verifikation der Backfill-Ergebnisse ist konkret und copy-paste-ready.

ops-Team APPROVE — alle Findings sind LOW und non-blocking.

@strausmann

Copy link
Copy Markdown
Owner Author

Round-3 Spec-Review: network-Team

Status: APPROVE

Round-2 Findings — Verifikation

  • H7 Healthcheck-Labels: vollständig adressiert. Der Blueprint-Snippet enthält alle Pflichtfelder per pangolin-resource-standard.md: name, full-domain, protocol=http, ssl=true, targets[0].method=http, targets[0].port=8000, targets[0].path-match=prefix, alle fünf healthcheck.*-Labels (enabled, hostname=print-hub, path=/healthz, port=8000, interval=30), auth.sso-enabled=true, auth.basic-auth.user + password. Port 8000 und Compose-Service-Name print-hub stimmen mit dem Architektur-Diagramm überein.

  • L3 Pangolin Bug #3099: adressiert. Risiko R8 benennt das Phänomen (Basic-Auth-Dialog statt SSO-Redirect), verlinkt den Upstream-Issue und erklärt den Cancel-Fallback. Kein weiterer Handlungsbedarf.

  • L4 CSRF-Strategie: adressiert. Vier explizite Test-Fälle in tests/middleware/test_csrf.py sind dokumentiert (gültiger Match → 303, Cookie ohne Hidden-Field → 403, falsches Hidden-Field → 403, Authorization-Header → CSRF skipped → 200). JSON-API-Pfad korrekt als CSRF-frei ausgewiesen wenn Basic-Auth oder API-Key genutzt wird.

Neue Round-3 Findings

Keine. Die Backfill- und PrintService-Änderungen (H8b, M8) haben keine neuen Pangolin- oder Netzwerk-Implikationen. Hangar→Hub-Routing bleibt intern via Container-Netz — Pangolin-Resource nicht betroffen.

Was gut ist

  • Vault-Item-Konvention (Pangolin Header Auth - Print Hub, Username claude-automation, Collection Automation/Claude-Team) ist exakt nach Standard.
  • Phase-0-Live-Check mit mcp__pangolin-api__resource_by_resourceId + Erwartung headerAuth != None und healthCheck.enabled=true ist präzise genug für die Implementierung.
  • Der Hinweis, Header-Auth ausschließlich via Blueprint Labels zu setzen (nie per API), ist explizit dokumentiert und referenziert das richtige Memory (feedback_pangolin_labels_source_of_truth).
  • auth.sso-enabled=true (korrekte Syntax, nicht auth.sso=true) ist im Snippet richtig.

@strausmann

Copy link
Copy Markdown
Owner Author

Round-3 Spec-Review: storage-Team

Status: APPROVE (mit einem dokumentierten LOW-Finding)


Round-2 Findings — Verifikation

  • H8a SNMP-Schema verschachtelt: vollständig adressiert. SNMPConfig als Sub-Class mit korrekter DB-JSON-Form, Pydantic und DB konsistent.
  • H8b Backfill: vollständig adressiert. Alembic-Migration ergänzt snmp-Block idempotent (if "snmp" not in conn_json), neue Spalten queue_timeout_s + cut_defaults_half_cut mit NOT NULL + server_default. SQLite 3.35+ unterstützt das. Verifikations-SQL ist explizit in Phase 3 gelistet — gut.
  • M7 Transaktions-Strategie: vollständig adressiert. isolation_level="SERIALIZABLE" auf Engine-Level (aiosqlite mappt auf IMMEDIATE), Connect-Listener für journal_mode=WAL + foreign_keys=ON. Service-Layer nutzt nur async with session.begin(): — kein doppelter BEGIN-Konflikt möglich.

Neue Round-3 Findings

LOW — Backfill Edge-Case: connection IS NULL

Der Backfill-Code schreibt bei connection IS NULL ein {"snmp": ...} ohne host/port:

conn_json = row.connection or {}
if "snmp" not in conn_json:
    conn_json["snmp"] = {"discover": False, "community": "public"}
    conn.execute(... .values(connection=conn_json))

NULL-connection ist im Schema erlaubt (nullable=True). Bei Bestand tritt das nicht auf (upsert_runtime_printers schrieb immer host+port). Für zukünftige Migrations-Sicherheit empfehle ich eine defensive Schutzklausel:

if not conn_json or "host" not in conn_json:
    # skip + log — connection ohne host macht keinen Sinn
    continue

Alternativ reicht ein Kommentar: "Invariante: connection enthält immer host+port (upsert_runtime_printers-Garantie) — skip wenn NULL".

LOW — Kein Downgrade-Pfad dokumentiert

Spec enthält keinen def downgrade() für die kombinierte Migration. Bei fehlgeschlagenem Upgrade (z.B. Backfill-Fehler mitten in der Schleife) bleibt die Alembic-Versions-Row unkonsistent. Da der Rollback-Pfad über SQLite-Restore läuft (Phase Rollback), ist das akzeptabel — aber die Spec sollte explizit dokumentieren: "Downgrade-Migration ist leer / nicht vorgesehen — Rollback erfolgt via SQLite-Restore (Phase Rollback-Pfad)".


Was gut ist

  • isolation_level="SERIALIZABLE" auf Engine-Ebene ist die richtige Lösung für den BEGIN IMMEDIATE/session.begin()-Konflikt — sauber und zukunftssicher.
  • Atomicity-Garantie INSERT printers + INSERT printers_audit in einer Transaktion ist explizit und korrekt.
  • Idempotenz-Schutz im Backfill (if "snmp" not in conn_json) verhindert Doppel-Schreibungen bei versehentlichem Migrations-Replay.
  • Verifikations-SQL in Phase 3 ist copy-paste-ready — genau richtig für Produktiv-Deploy.
  • Kein FK auf printers_audit.printer_id ist die korrekte Entscheidung bei Soft-Delete.

Die beiden LOW-Findings blockieren nicht — APPROVE.

storage-agent, Round-3

@strausmann

Copy link
Copy Markdown
Owner Author

Round-3 Spec-Review: code-quality

Status: NEEDS_FIXES (2 neue Medium-Findings)


Round-2 Findings — Verifikation

  • H9 Test-Files (5 statt 3): ✅ adressiert — Sektion "ID-Generierung" listet alle 5 Files explizit, inkl. test_lifespan_seeds_and_upserts.py und test_lifespan_printer_upsert.py. grep-Verifikationsschritt ist im Akzeptanzkriterium verankert. Reicht.
  • M8 enabled-Check: ✅ adressiert — Code-Snippet, 2 Test-Cases, Sektion "Implikationen für Hangar+PrintService" klar.
  • M9 redact_secrets Modul: ✅ adressiert — app/services/audit_redaction.py, Skeleton, 4 Edge-Cases, 80%-Schwelle.
  • L CSRF 4 Fälle / Trailing-Slash: ✅ beide adressiert.

Neue Round-3 Findings

[MEDIUM] M11 — LabelHubException existiert nicht

Spec definiert:

class PrinterDisabledError(LabelHubException):
    http_status = 409

Live-Check: backend/app/printer_backends/exceptions.py kennt nur PrinterError als Basisklasse. Eine Datei app/exceptions.py existiert noch nicht (laut Spec wird sie neu angelegt). LabelHubException ist nirgendwo definiert.

Der Implementer steht vor der Wahl: LabelHubException neu einführen (was die Basis für alle künftigen HTTP-Exceptions wäre) oder PrinterDisabledError von Exception oder PrinterError ableiten. Die Spec muss das entscheiden — sonst entstehen zwei divergierende Implementierungen wenn Backend-Team und Hub-Team unabhängig arbeiten.

Fix: Spec ergänzt app/exceptions.py mit class LabelHubException(Exception): http_status: int = 500 als Basis ODER tauscht die Basis auf PrinterError aus (dann ist http_status-Convention aber nicht standardisiert).


[MEDIUM] M12 — model_dump() → flache Spalten: Mapping unspezifiziert

Create-Flow Step 5c:

row_dict = payload.model_dump() + {"id": printer_id, "created_at": ..., "updated_at": ...}
# INSERT INTO printers (...)

payload.model_dump() gibt {"queue": {"timeout_s": 30}, "cut_defaults": {"half_cut": false}, "connection": {...}, ...}. Die Spalten queue_timeout_s und cut_defaults_half_cut sind aber flach (Alembic-Migration Phase 1b). row_dict kann so nicht direkt als INSERT-Payload genutzt werden — printers.queue existiert als Spalte nicht.

Der Implementer muss manuell flatten:

row_dict["queue_timeout_s"] = payload.queue.timeout_s
row_dict["cut_defaults_half_cut"] = payload.cut_defaults.half_cut
row_dict.pop("queue")
row_dict.pop("cut_defaults")

Das ist nicht trivial und fehleranfällig (besonders beim update_printer-Pfad mit optionalem Patch). Spec sollte ein explizites _to_db_row(payload) Helper-Snippet zeigen oder zumindest die Flattening-Logik im Data-Flow Step benennen.


[LOW] Engine-Snippet Reihenfolge (Lesbarkeit)

M7-Snippet zeigt @event.listens_for(engine.sync_engine, "connect") als Dekorator vor create_async_engine — das wäre zur Laufzeit ein NameError. Live engine.py macht es korrekt (create first, listen second). Das Snippet ist misleading für einen neuen Implementer. Empfehlung: Snippet in chronologische Reihenfolge bringen oder als Pseudo-Code kennzeichnen.

Außerdem: Live engine.py hat bereits _apply_pragmas mit WAL + foreign_keys + busy_timeout. Die Spec soll nur isolation_level="SERIALIZABLE" ergänzen — das steht nicht explizit als Diff. Bitte klarstellen was neu hinzukommt vs. was bereits vorhanden ist.


Was gut ist

  • Alle 5 H9-Test-Files sauber kategorisiert (löschen vs. migrieren).
  • redact_secrets mit frozenset[tuple]-Pfad-Konvention ist erweiterbar und klar.
  • SNMPConfig-Pfad ("connection", "snmp", "community") ist konsistent mit Pydantic-Schema.
  • M7-Entscheidung (Engine-Level IMMEDIATE statt manuelles BEGIN) ist architektonisch sauber — kein session.begin()-Konflikt.
  • CSRF-Strategie (Authorization-Header skipped) deckt den JSON-API-Pfad korrekt ab.
  • Akzeptanzkriterien vollständig und messbar.

@strausmann

Copy link
Copy Markdown
Owner Author

Round-4 Spec-Review: code-quality

Status: APPROVE

Round-3 Findings — Verifikation

  • M11 PrinterDisabledError-Basisklasse: ✅ vollständig adressiert. exceptions.py bestätigt: PrinterError ist die echte Root-Basisklasse (nicht LabelHubException). PrinterDisabledError(PrinterError) mit __init__(self, printer_id: UUID, slug: str) ist korrekt platziert in app/printer_backends/exceptions.py. Die Ablage ist pragmatisch akzeptabel — alle Hardware-/Status-Exceptions leben dort, und HTTP-Mapping erfolgt sauber getrennt in api/routes/print.py analog TapeMismatchError. Keine semantische Spannung: die Klasse beschreibt einen Drucker-Zustand, nicht einen HTTP-Fehler.

  • M12 Flattening-Helper: ✅ vollständig adressiert. Drei explizite Funktionen mit klarer Verantwortung:

    • _payload_to_row mappt verschachtelt → flach, connection bleibt als JSON-Blob
    • _apply_update_patch returnt nur geänderte Spalten (effizientes partial UPDATE)
    • _row_to_audit_view rekonstruiert verschachtelte Form für redact_secrets

    Test-Cases für alle drei sind explizit im Testing-Abschnitt benannt. ✅

  • LOW Engine-Snippet: ✅ adressiert. Korrekte Reihenfolge (create_async_engine vor @event.listens_for), als Pseudo-Code markiert, Delta-Hinweis ("was sich konkret ändert") ergänzt.

Neues Finding — R4a: _apply_update_patch partial-connection-Update nicht möglich

Die Spec sollte explizit dokumentieren (empfohlen, kein Blocker):

_apply_update_patch behandelt patch.connection atomar — wenn connection nicht None ist, wird die gesamte connection-JSON-Spalte ersetzt. Ein Operator, der nur connection.host ändern will, muss die gesamte PrinterConnection mitschicken (inkl. snmp, port). Das ist für den API-Pfad nicht offensichtlich.

Empfehlung: einen Satz in der _apply_update_patch-Beschreibung ergänzen: "Partial-Update von Sub-Feldern in connection ist nicht unterstützt — der Client muss das vollständige connection-Objekt senden." Alternativ als Akzeptanzkriterium oder API-Doku. Kein Code-Änderungsbedarf — das Verhalten ist bewusst und konsistent mit dem Update-Schema.

Was gut ist

  • _row_to_audit_view mit "id": str(row["id"]) ist korrekt: UUID → String-Konvertierung nur hier nötig (andere Felder sind primitive Typen, die SQLite/JSON direkt serialisiert). Kein Konsistenzproblem.
  • Atomicity-Garantie (printers + printers_audit in einer Transaktion) ist klar dokumentiert mit Rollback-Semantik.
  • silent ignore von slug/model/backend/id in _apply_update_patch ist jetzt explizit im Docstring und im Kommentar im Update-Flow — keine Überraschungen mehr für Implementierer.
  • Exception-Datei verifiziert: TapeMismatchError als Pattern-Referenz für den print.py-Handler ist eine solide Vorlage.

Freigabe zur Implementierung.

@strausmann

Copy link
Copy Markdown
Owner Author

Plan-Review Round-1: storage-Team

Status: NEEDS_FIXES (2 Medium, 2 Low)


Findings

MEDIUM — Task 1.1: isolation_level="SERIALIZABLE" mappt nicht zuverlässig auf aiosqlite

Das Plan-Snippet setzt create_async_engine(..., isolation_level="SERIALIZABLE"). In SQLAlchemy 2 + aiosqlite wird SERIALIZABLE auf BEGIN IMMEDIATE gemappt — aber das ist ein undokumentiertes Implementierungsdetail des aiosqlite-Dialekts. Der Test prüft engine.dialect.isolation_level == "SERIALIZABLE" über eine OR-Bedingung, die so breit ist dass sie fast immer True ergibt unabhängig vom tatsächlichen Verhalten. Besser: in test_connect_listener_sets_wal_and_foreign_keys das BEGIN-Verhalten direkt verifizieren via PRAGMA journal_mode (bereits vorhanden) statt die Dialect-Introspection zu testen. Der Test für journal_mode=wal und foreign_keys=1 ist ausreichend und korrekt — die Isolation-Level-Assertion ergibt keinen Mehrwert und kann entfernt werden um False-Confidence zu vermeiden. Handlungsempfehlung: test_engine_isolation_level_is_serializable ersatzlos streichen, nur den Pragma-Test behalten.

MEDIUM — Task 1.3: downgrade() raises NotImplementedError statt pass/no-op

alembic downgrade -1 bricht hart mit NotImplementedError. In CI-Umgebungen die Migration-Roundtrip testen (down + up) würde das die Pipeline lahmlegen. Für einen SQLite-only-Service ist das Rollback-Modell (Restore aus Backup) korrekt — aber die Ablehnung gehört als Kommentar/Logging, nicht als Exception. Handlungsempfehlung: downgrade() durch reinen No-op ersetzen (pass mit erklärendem Docstring), damit alembic downgrade -1 in CI durchläuft. Der Rollback-Hinweis im Docstring bleibt. Kein Code-Pfad sollte NotImplementedError auf alembic-Aufrufe werfen.

LOW — Task 1.3 Test-Wording: "Defaults durch die Migration" ist irreführend

Der Test test_backfill_sets_snmp_defaults_for_existing_printers kommentiert "Backfill darf in Test-Setup nicht erneut laufen (Migration ist schon angewandt) — also prüfen wir nur dass Defaults greifbar sind" und testet dann queue_timeout_s == 30. Dieser Wert kommt aus dem server_default der DDL, nicht aus dem Backfill-Pfad der Migration. Das ist im Kommentar bereits halb erklärt, aber der Test-Name suggeriert er testet den Backfill. Der eigentliche Backfill-Pfad (SNMP-Block ergänzen für Rows ohne ihn) wird im Test-Setup nicht getriggert weil der Row NACH der Migration inserted wird. Handlungsempfehlung: Test umbenennen zu test_new_rows_get_server_defaults_after_migration. Einen separaten Test ergänzen der die Backfill-Logik direkt in einer In-Memory-DB testet ohne alembic (Unit-Test der upgrade()-Funktion mit eigenem Connection-Mock).

LOW — Task 2.5 / Phase 8.4: before_json/after_json als String via _json_or_none

Das _record_audit-Insert übergibt before/after als json.dumps()-String an den aiosqlite-Bind-Param. Die Spalte printers_audit.before_json ist sa.JSON() — SQLAlchemy würde bei ORM-Nutzung automatisch serialisieren. Mit raw text()-Insert muss der String explizit sein, was korrekt ist. Aber: bei Phase 8.4 Backfill-Verifikation gibt json_extract(connection, '$.snmp.discover') für SQLite einen Integer (0/1) zurück, kein Boolean. Das Smoke-Output-Matching in der Verifikations-Anleitung sollte snmp_discover=0 (nicht False) als expected Wert dokumentieren. Handlungsempfehlung: Kommentar in Task 8.4 Step 3 ergänzen: json_extract gibt 0/1 (Integer) zurück, nicht true/false.


Was gut ist

  • Backfill-Schutzklauseln (NULL-connection skip, missing-host skip, Idempotenz via "snmp" in conn_json: continue) sind vollständig und korrekt.
  • WAL-Backup-Sicherheit: sqlite3 .backup ist WAL-aware und konsistent — richtige Wahl.
  • Env-Merge (Phase 8.3) folgt sauber dem Merge-Pattern aus dockhand-stack-env-merge.md.
  • server_default für queue_timeout_s und cut_defaults_half_cut mit sa.false() ist idiomatisch korrekt für SQLAlchemy+SQLite.
  • No-FK auf printers_audit.printer_id ist die richtige Entscheidung — Soft-Delete behält den Parent-Row sowieso.

@strausmann

Copy link
Copy Markdown
Owner Author

Plan-Review Round-1: ops-Team

Status: NEEDS_FIXES
Reviewer: ops-agent | Datum: 2026-06-14
Plan: 2026-06-14-printers-yaml-to-db-plan.md (Commit 44d543a)


Findings

HIGH

H1 — Parameter-Name-Bug Task 8.2: auto_update= existiert nicht
set_container_auto_update(auto_update="never") — der MCP-Tool-Parameter heißt policy=, nicht auto_update=. Der Call würde mit einem TypeError scheitern und Watchtower läuft weiter während des Deploys.

Fix: set_container_auto_update(containerName="...", environmentId=10, policy="never")

H2 — Rollback-Pfad fehlt komplett
Phase 8 hat keinen expliziten Sub-Task für den Fall dass der Health-Check nach start_stack nicht grün wird. Was ist dann zu tun?

Minimaler Rollback-Step nach 8.3:

8.3.R Rollback (falls Health-Check rot):

  1. down_stack("hangar-print-hub")
  2. docker exec hangar-print-hub-print-hub-1 sqlite3 /data/printer-hub.db ".restore /data/printer-hub.db.bak-pre-124"
  3. update_stack_compose(restart=False) mit dem alten Compose (ohne neue Labels)
  4. start_stack("hangar-print-hub")
  5. set_container_auto_update(policy="any") (Watchtower re-aktivieren)

Ohne diesen Pfad ist kein sicheres Abbrechen möglich.


MEDIUM

M1 — Phase 0: Volume-Mount-Check fehlt
Step 0 prüft DB-Schema und Pangolin-Resource, aber nicht was tatsächlich bei /data im laufenden Container gemountet ist. Empfehlung: Step 0.X ergänzen:

mcp__dockhand__exec_container(command=["sh", "-c", "mount | grep /data"])

Schützt vor dem Fall dass das Backup auf den falschen Pfad läuft.

M2 — Pangolin Newt-Sync-Delay nicht berücksichtigt (Smoke 8.4 Step 4)
Nach start_stack kann es bis zu 5 Minuten dauern bis neue Blueprint-Labels via Newt in Pangolin aktiv sind. Ein Browser-Test als Step 4 in 8.4 kann spurious failen.

Fix: Vor dem Browser-Test explizit warten — sleep 60 oder Retry-Loop mit Pangolin-Health-API-Check (mcp__pangolin-api__get_resource bis Status "healthy").


LOW

L1 — Task 3.3 Granularität
4 HTML-Templates in einem einzigen TDD-Step ist für Subagent-Driven-Development großzügig. Bei Auftreten von Subagent-Context-Erschöpfung ggf. aufteilen: 3.3a (list + detail) / 3.3b (create + edit).


Was gut ist

  • Phase 1.3 Alembic-Backfill korrekt: NULL-Guard + fehlender-Host-Guard + idempotentes Check auf "snmp"-Key — für Prod-DB ausreichend defensiv.
  • Task 8.3 Env-Merge sauber: get_stack_env → Filter PRINTER_CONFIG_PATH → merged list → update_stack_env — kein PUT-Replace-Fehler.
  • 8.3 Compose-Update-Pipeline korrekt: update_stack_compose(restart=False) + down_stack + start_stack (nicht restart_stack).
  • Phase 5 vor Phase 7 — Dependency-Reihenfolge ist korrekt.
  • SQLAlchemy WAL + SERIALIZABLE Connect-Listener, printers_audit SNMP-Redaction, Soft-Delete mit FK-Safety — alles solide.

@strausmann

Copy link
Copy Markdown
Owner Author

Plan-Review Round-1: code-quality

Status: NEEDS_FIXES (2 HIGH, 3 MEDIUM, 2 LOW — kein Blocker für Implementierungsstart wenn HIGH-Punkte vor Task 3.2 bzw. 4.1 adressiert werden)


Findings

HIGH

H1 — GET /api/printers filtert enabled NICHT (bestätigtes Gap)

backend/app/repositories/printers.py list_all() macht select(Printer).order_by(...) ohne WHERE enabled = true. Die bestehende Route GET /api/printers in printers.py ruft printers_repo.list_all(session) auf — deaktivierte Drucker werden also zurückgegeben. Der Plan-Self-Review markiert diesen Spec-Punkt als ✅ (Task 7.1 Step 3 test_fresh_install_disable_filters_from_public_list), aber kein Task in Phase 1–5 repariert list_all oder die bestehende Route.

Task 7.1 testet das Verhalten erst als E2E — wenn list_all nie angepasst wurde, schlägt der Test fehl und der Implementer muss zurückgehen. Klarer wäre: in Phase 4 oder 5 einen eigenen Task „printers_repo.list_all filtert enabled=true by default" einfügen, mit isoliertem Failing-Test auf Repository-Ebene. Alternativ: expliziter Kommentar im Spec-Coverage-Check dass list_all in Task 5.2 angepasst wird.

H2 — Task 3.2 ...-Placeholder verletzt test-coverage-pflicht.md-Pflicht explizit

test_create_printer_duplicate_slug_409, test_get_printer_by_slug, test_update_printer_silently_ignores_slug, test_disable_printer_204, test_disable_already_disabled_409, test_enable_after_disable_200 haben {...} bzw. ... als Body. Das sind 6 von 9 Tests in der Datei. Die Rule sagt: keine "Similar to Task N" Verweise, Mutation-Pfade brauchen Happy-Path + Error-Path + Network-Error. „Implementer ergänzt nach Pattern" delegiert Entscheidungen die in den Plan gehören. Mindestens die Payloads und die assert-Statements müssen ausgeschrieben sein.


MEDIUM

M1 — Task 3.4 „Pattern wie Task 3.3" für 5 Routes reicht nicht

Task 3.3 schreibt 5 Tests (list, new-form, create-post, edit-prefilled-stub, confirm-disable-stub). Task 3.4 hat kein einziges Test-Snippet für die 5 fehlenden Routes (edit-GET, edit-POST, disable-GET, disable-POST, enable-POST). Für Soft-Routes wie enable/disable-POST sind Redirect-Ziel und 409-Konflikt-Fall die Spec-relevanten Assertions. Ein Implementer der nur Task 3.4 liest, schreibt unter Zeitdruck Minimal-Tests ohne die Fehler-Pfade.

M2 — Phase 6 ohne Verifikations-Task für Pangolin Header-Auth

Phase 6 endet mit Labels im Compose. Der erste echte Smoke-Test ist Task 8.4 Step 5 (PrintService 409). Es fehlt ein direkter curl-Test claude-automation → 200, der vor dem Stack-Neustart in Phase 8 ausgeführt wird und der Pangolin-Resource-Standard-Checklist (curl -u claude-automation:<pw>) entspricht. Ohne diesen Schritt ist der Header-Auth-Bypass erst nach dem Deploy verifiziert — Rollback-Fenster ist dann enger.

M3 — hangar_meta-Tabelle im Hub nachweislich nicht vorhanden, „weglassen"-Kommentar reicht nicht

Plan sagt: „Falls hangar_meta-Tabelle im Hub nicht existiert: weglassen." Das erzeugt einen OperationalError zur Laufzeit wenn Task 5.2 blindlings deployed wird. Der Implementer braucht entweder: (a) den Marker komplett streichen (kein Wert für HomeLab-Betrieb), oder (b) einen expliziten CREATE TABLE IF NOT EXISTS-Step davor. Die aktuelle Formulierung lässt den Implementer raten.


LOW

L1 — Task 4.1 Fixture-Setup nicht ausgeschrieben

test_submit_print_job_raises_disabled_for_disabled_printer hat # ... (existing fixture-Setup) ... als Placeholder. tests/services/test_print_service.py nutzt vermutlich einen @pytest.fixture disabled_printer der erst durch die neuen PrinterAdminService-Methoden befüllt werden kann. Der konkrete Fixture-Aufbau (Session, PrinterAdminService.create_printer, disable_printer) sollte ausgeschrieben sein — das ist ein Mutation-Pfad.

L2 — Coverage-Schwelle für admin_printers_web.py (70%) ist unter Projekt-Baseline für Business-Routes

Der Plan selbst setzt 70% für die Web-Routes, während die JSON-API (80%) und der Service (85%) höher liegen. Die Web-Routes enthalten Form-Parsing, Redirect-Logik und CSRF-Flow — das sind Mutations-Pfade. 70% ist nach test-coverage-pflicht.md für „HTMX-Event-Wiring / Form-Handler" Minimum 80%. Empfehlung: auf 80% anheben oder in die Coverage-Whitelist mit Begründung aufnehmen.


Was gut ist

  • TDD-Strict-Pattern in Phase 1 und Phase 2 (Tasks 1.1–2.5) ist vorbildlich: jeder Task hat Step 1 (Failing-Test), Step 2 (Run → FAIL), Step 3 (Implementation), Step 4 (Run → PASS), dann Commit. Kein einziger Task in diesen Phasen überspringt das Red-Step.
  • Dockhand Stack-Env Merge-Pattern in Task 8.3 korrekt umgesetzt (get → filter → put + Verifikation via assert).
  • redact_secrets Deep-Copy-Verhalten (Mutation-Schutz) durch test_redact_does_not_mutate_input explizit getestet — gut.
  • Phase 5.3 Grep-Verifikation (H9-Check) ist sauber: zwei separate Greps bestätigen dass keine upsert_runtime_printers- und derive_printer_id-Altaufrufer verbleiben.
  • Deutsche Umlaute durchgehend korrekt (ä/ö/ü/ß) — keine Verstöße gefunden.
  • Coverage-Schwellen-Tabelle im Self-Review ist vollständig und enthält Task-Referenzen für Verifikation.

@strausmann

Copy link
Copy Markdown
Owner Author

Plan-Review Round-1: network-Team

Status: APPROVE mit 3 Anmerkungen (keine CRITICAL/HIGH)


Findings

MEDIUM — Phase 0 Step 3: MCP-Tool-Name stimmt nicht (wird fehlschlagen)

Plan schreibt:

mcp__pangolin-api__org_by_orgId_resources
mcp__pangolin-api__resource_by_resourceId

Das MCP-Tool heisst in dieser Umgebung so nicht — Pangolin MCP nutzt andere Tool-Namen (ermittelbar via Tool-Discovery). Subagent wird auf No such tool available laufen und abbrechen.

Fix: Step 3 umformulieren auf SSH-Fallback:

# Vault-Key: homelab-pangolin-integration-api (Feld: password)
PANGOLIN_URL="https://backend-api.strausmann.cloud"
curl -sf -H "Authorization: Bearer ${PANGOLIN_TOKEN}" \
  "${PANGOLIN_URL}/v1/org/strausmann/resources" \
  | python3 -c "import json,sys; [print(r['resourceId'], r.get('fullDomain')) for r in json.load(sys.stdin)['data']['resources'] if 'print-hub' in r.get('fullDomain','')]"

Alternativ: MCP-Tool-Namen via mcp__pangolin-api__* Discovery vor dem Call verifizieren. Der Pangolin-API-Key ist in Vaultwarden unter homelab-pangolin-integration-api (Feld password) — steht im SKILL.md, aber nicht im vault-map, also kein vault_load_service-Shortcut.

LOW — Phase 3.1 CSRF: zwei Implementierungs-Alternativen ohne Entscheidung

Plan zeigt setup_csrf_middleware() via starlette_csrf.CSRFMiddleware direkt UND darunter CSRFWithAuthSkip(BaseHTTPMiddleware) als "Alternative wenn starlette-csrf keinen Auth-Skip kann". Der Implementer muss wählen.

Empfehlung: starlette-csrf zuerst evaluieren (pip show starlette-csrf + Doku prüfen ob exempt_headers / exclude_patterns unterstützt werden). Falls Auth-Header-Skip eingebaut ist: direktes CSRFMiddleware. Falls nicht: Wrapper-Variante. Plan sollte das als sequentielle Entscheidung formulieren, nicht als zwei parallele Optionen — sonst landen beide im Code.

LOW — Phase 3.2 require_admin_user: Trust-Scope ist HomeLab-adäquat, mit einer Ergänzung

if request.headers.get("Authorization", "").lower().startswith("basic "):
    return "claude-automation"

Der Plan vertraut hier blindlings dem Authorization-Header, ohne den Credential-Wert zu prüfen. Pangolin reicht bei korrekt konfiguriertem Header-Auth-Bypass den Remote-User Header mit dem echten SSO-User durch — Authorization: Basic erscheint nur wenn der claude-automation Bypass greift. Das ist akzeptable Pragmatik für HomeLab.

Aber: Fehlt im Plan der Verweis auf den Pangolin Bug #3099. Im Browser-Smoke-Test (Phase 8.4) erscheint ein Basic-Auth-Dialog statt SSO-Login-Page — das ist kein Bug im Hub-Code, sondern bekannte Pangolin-Regression. Der Implementer muss wissen: Cancel im Dialog → SSO-Login erscheint. Ohne diesen Hinweis wird der Smoke-Test als fehlgeschlagen gewertet.

Fix: Phase 8.4 ergänzen:

Hinweis: Pangolin Bug #3099 — Browser zeigt evtl. Basic-Auth-Dialog statt SSO-Login. Cancel → SSO-Page erscheint. Kein Hub-Bug.


Was gut ist

  • Phase 6.1 Vault-Item: Collection Automation/Claude-Team + Naming Pangolin Header Auth - Print Hub exakt nach pangolin-resource-standard.md. Keine Abweichung.
  • Phase 6.2 Blueprint-Labels: Alle 14 Pflichtfelder vorhanden (name, full-domain, protocol, ssl, target, healthcheck mit hostname=print-hub, auth.sso-enabled, auth.basic-auth). hostname=print-hub als Service-Name ist der korrekte Container-DNS-Name im Compose-Setup.
  • Phase 8 down+start statt restart: Korrekt für Volume-Mount-Änderung. Dockhand-Merge-Pattern aus dockhand-stack-env-merge.md explizit im Spec referenziert.
  • Hangar→Hub intern: Kein Pangolin-Pfad, kein Header-Auth nötig. Richtig erkannt.
  • SSO-Label-Syntax: auth.sso-enabled=true korrekt (nicht sso=true, welches still ignoriert würde).

@strausmann

Copy link
Copy Markdown
Owner Author

Plan-Review Round-2: ops-Team

Status: APPROVE (mit einem LOW-Finding zur expliziten Dokumentation)


Round-1 Findings — Verifikation

H1 Rollback (Task 8.5): Adressiert. 8 Steps vollständig. Reihenfolge ist korrekt: Stack down → DB-Restore → WAL/SHM-Removal → Compose-Revert → Env-Re-Merge → printers.yaml-Restore → Stack-Start → Verifikation. WAL/SHM-Removal in Step 2 direkt nach dem DB-Restore und vor Stack-Start ist die richtige Position — SQLite würde andernfalls beim Mount den WAL-Zustand wiederherstellen und den clean Restore überschreiben.

C1 policy= Fix (Tasks 8.2 + 8.4): Adressiert. Beide Calls nutzen jetzt korrekt policy="never" (8.2) und policy="any" (8.4). Das war das einzige ops-CRITICAL aus Round-1.

M6 Smoke-Schritte: Adressiert. Task 8.4 enthält jetzt json_extract Integer-Output-Hinweis (0/1 statt false/true) und Pangolin-Bug-#3099-Vermerk. Reicht.


Neue Round-2 Findings

[LOW] Task 8.5 Step 3: PRE_DEPLOY_COMPOSE_CONTENT implizit — Die Variable PRE_DEPLOY_COMPOSE_CONTENT in Rollback Step 3 wird als # aus Phase 0 Live-Check festgehalten kommentiert. Phase 0 Step 4 liest das Schema, speichert aber keinen Compose-Snapshot. Task 6.2 Step 1 holt mit get_stack_compose() den Live-Compose und zeigt ihn via print(existing["content"]) — aber der Plan schreibt nirgends explizit vor, diesen Wert in phase0-live-check-results.md zu persistieren. Empfehlung: in Task 6.2 Step 3 (Hinweis im Plan) einen Pflichtschritt ergänzen: PRE_DEPLOY_COMPOSE_CONTENT = existing["content"] aus Step 1 in die Live-Check-Doku schreiben — als Copy-Paste-Block, damit Rollback ohne Gedächtnis funktioniert. Nicht blocker, aber bei unerwarteten Fehlern ist der Compose-Stand die einzige Restore-Basis.

[KEINE ISSUE] Task 6.3 Reihenfolge: Die curl-Schritte in 6.3 sind als Pre-Deploy-Verifikation markiert — das macht sie zu einem "diese Labels funktionieren bereits"-Check. Der Text sagt klar "Nach Stack-Update in Phase 8.3 muss der Header-Auth-Bypass funktionieren. Dieser Task wird VOR dem Smoke-Test (8.4) explizit ausgeführt". Das bedeutet: 6.3 läuft nach 8.3, nicht als Phase-6-Check. Die Positionierung im Plan unter Phase 6 ist irreführend, aber der Text-Kommentar in Task 6.3 macht die tatsächliche Ausführungsreihenfolge klar. Kein Bug.

[KEINE ISSUE] Task 2.6/2.7 und Hangar PrinterSync: Plan sagt, Hangar filtert disabled Drucker bereits im Cache-Layer heraus. Smoke-Test 8.4 Step 2 verifiziert DNS-Erreichbarkeit zwischen den Containern. Ein dedizierter /api/printers-Filtertest läuft in Task 7.1 (test_fresh_install_disable_filters_from_public_list). Das reicht — kein weiterer Smoke-Step nötig.


Ops-Team gibt APPROVE. Der LOW-Hinweis zu PRE_DEPLOY_COMPOSE_CONTENT kann inlined beim Ausführen von Task 6.2 Step 3 adressiert werden — kein neuer Plan-Round erforderlich.

@strausmann

Copy link
Copy Markdown
Owner Author

Plan-Review Round-2: network-Team

Status: APPROVE

Round-1 Findings — Verifikation

C3 Falsch-Positiv akzeptiert: Ja
Die Notiz in der Findings-Tabelle ist klar und vollständig. Live-Verifikation per ToolSearch hat beide Tools mit den exakten Namen aus dem Plan bestätigt. Kein curl-Fallback nötig — Phase 0 Step 3 bleibt wie geschrieben.

L1 Pangolin Bug #3099: adressiert
Task 8.4 Step 4 erklärt den Workaround korrekt: Cancel im Basic-Auth-Dialog → HTML-Body-Fallback → SSO-Login-Page. Der Link auf den upstream Issue ist vorhanden. Reicht für einen Smoke-Test-Hinweis.

L2 CSRF-Variante: adressiert
Entscheidung für Wrapper ist klar dokumentiert (Task 3.1 Step 3 Kommentar: "L2-Round-1 Entscheidung gegen die zwei Alternativen"). Der Wrapper skipped CSRF wenn -Header gesetzt ist — das ist die korrekte Implementierung für den Header-Auth-Bypass.

Neue Round-2 Findings

R2-N1 (LOW): Task 6.3 Reihenfolge — leicht irreführend
Task 6.3 trägt die Nummer "6.3" und liegt in Phase 6, aber sein Beschreibungstext sagt "Nach Stack-Update in Phase 8.3". Die Aufgabe wird also nach 8.3 ausgeführt, ist aber als Phase-6-Task nummeriert. Das ist nicht falsch, aber verwirrend für einen Implementer der sequenziell arbeitet.

Empfehlung: In Task 6.3 Intro-Zeile klarstellen: "Dieser Task ist in Phase 6 vorbereitet (Credentials, Vault), wird aber nach 8.3 ausgeführt — erst dann sind die Labels live." Die aktuelle Formulierung sagt das schon halb, aber ein expliziter Satz "Ausführungs-Zeitpunkt: nach Task 8.3, vor Task 8.4" beseitigt jeden Zweifel.

R2-N2 (LOW): Pangolin-Resource Bestand-Detection fehlt
Phase 0 Step 3 prüft ob bereits gesetzt ist. Phase 6.2 überschreibt die Labels immer — auch wenn bereits ein anderer Header-Auth-User/Password aktiv ist. Wenn die Resource bereits mit anderem Secret hat (z.B. aus manuellem Dashboard-Set), überschreiben die Blueprint-Labels das beim nächsten Sync.

Das ist per ("Labels sind Source of Truth, API-Änderungen werden überschrieben") korrekt und gewollt. Kein Plan-Defekt, nur eine Klarstellung: wenn Phase 0 Step 3 zeigt dass , muss das Secret aus dem Vault-Item in 6.1 mit dem bestehenden übereinstimmen oder bewusst überschrieben werden. Dieser Hinweis fehlt in Task 6.1/6.2.

Empfehlung: In Task 6.2 Step 2 eine Bemerkung: "Falls Phase 0 Step 3 zeigt dass headerAuth bereits gesetzt ist: das neue Secret aus Task 6.1 ersetzt es. Vault-Item entsprechend anlegen/aktualisieren."


Beide Round-2-Findings sind LOW und blockieren die Umsetzung nicht. Der Plan ist in allen network-relevanten Punkten korrekt ausgearbeitet.

@strausmann

Copy link
Copy Markdown
Owner Author

Plan-Review Round-2: storage-Team

Status: NEEDS_FIXES (1 MED neu)

Round-1 Findings — Verifikation

  • M1 Isolation-Test gestrichen: adressiert. Nur PRAGMA-Test bleibt — zuverlässig und aussagekräftig. OK.
  • M2 downgrade → pass: adressiert. Docstring erklärt warum kein SQLite-DDL-Rollback möglich ist. CI-konform. OK.
  • L5 json_extract Output: adressiert. Task 8.4 Step 3 nennt explizit 0/1 statt false/true und zeigt erwarteten Smoke-Output. OK.
  • L6 Test-Wording + Backfill-Function-Test: teilweise adressiert — der Backfill-Test ist da, aber enthält einen Sync/Async-Mismatch (siehe unten).

Neue Round-2 Findings

[MED] R2-M1 — Sync/Async-Mismatch in test_backfill_function_idempotent_and_safe

Der Test ruft _backfill_snmp(conn) mit einem AsyncConnection auf (async with engine.begin() as conn), aber _backfill_snmp verwendet synchrone SQLAlchemy-Aufrufe ohne await (bind.execute(sa.text(...))).

Das ist auf der Produktions-Seite korrekt: env.py nutzt run_sync(do_run_migrations) — der bind den op.get_bind() in upgrade() liefert, ist eine echte sync Connection. Die Funktion selbst ist richtig.

Das Problem liegt im Test: AsyncConnection.execute() ohne await gibt ein Coroutine-Objekt zurück das nie ausgeführt wird. Die Rows werden nicht verändert, der Test läuft aber ohne Exception durch — er prüft damit nichts.

Fix: Test muss engine.sync_engine.begin() als sync-Context nutzen, oder den Helper über connection.run_sync(_backfill_snmp) aufrufen:

# Option A — sync engine direkt
with engine.sync_engine.begin() as conn:
    mig._backfill_snmp(conn)

# Option B — run_sync Pattern (spiegelt Alembic-Produktionsverhalten)
async with engine.begin() as conn:
    await conn.run_sync(mig._backfill_snmp)

Option B ist vorzuziehen weil sie 1:1 das Alembic-run_sync-Pattern aus env.py spiegelt.


[LOW] R2-L1 — Rollback Task 8.5 Step 2: Reihenfolge ist korrekt

Die Frage war ob WAL/SHM vor oder nach dem Restore gelöscht werden müssen. Die Plan-Reihenfolge (cp backup → db, dann rm WAL/SHM) ist korrekt: Stack ist bereits in Step 1 gestoppt, kein SQLite-Prozess kann neue WAL-Dateien anlegen. Das WAL/SHM-Löschen nach dem Restore bereinigt stale Dateien die zur alten (fehlerhaften) DB gehören. Kein Fix nötig.


[LOW] R2-L2 — C2 list_all bestehende Aufrufer

Task 2.6 Step 5 sagt bereits: pytest -x --ff -q läuft und bei brechenden Aufrufern auf include_disabled=True umstellen. Das ist ausreichend — ein expliziter grep-Schritt wäre nice-to-have, aber kein Blocker.


Zusammenfassung

Ein echter Bug: R2-M1 muss gefixt werden bevor der Test aussagekräftig ist — aktuell testet test_backfill_function_idempotent_and_safe die Backfill-Logik nicht wirklich. Alle Round-1-Findings bis auf L6 sind vollständig adressiert.

@strausmann

Copy link
Copy Markdown
Owner Author

Plan-Review Round-2: code-quality

Status: NEEDS_FIXES (2 neue Findings, 1 davon MEDIUM)


Round-1 Findings — Verifikation

  • H1 GET /api/printers (C2): adressiert — Tasks 2.6 + 2.7 vollständig, 3 + 2 Tests konkret ausgeschrieben, list_all Signatur korrekt mit include_disabled: bool = False, Route nutzt Default.
  • H2 Task 3.2 Placeholder: adressiert — alle 9 Tests vollständig ausgeschrieben, kein ... oder pass.
  • M3 Task 3.4 Tests: adressiert — 5 Tests mit Setup, CSRF-Cookie-Roundtrip und konkreten Assertions.
  • M4 Task 6.3 curl-Verifikation: adressiert — 4 Steps mit konkreten curl-Kommandos, Secret-Abruf aus Vault, erwartetes HTTP-Status je Step, Ergebnis-Dokumentation in phase0-Doku.
  • M5 hangar_meta entfernt: adressiert — Task 5.2 enthält expliziten Kommentar "M5-Round-1: Der Marker-Code ist weggefallen — Tot-Code", Funktion wird vollständig gelöscht ohne Ersatz.
  • L3 Task 4.1 Fixture-Setup: adressiert — disabled_printer-Fixture vollständig mit allen Feldern inkl. queue_timeout_s, cut_defaults_half_cut, created_at, updated_at. Regression-Test für enabled_printer ergänzt.
  • L4 Web-Coverage 80%: adressiert — Coverage-Tabelle enthält app/api/routes/admin_printers_web.py mit 80% und kommentiertem Hinweis "L4-Round-1: 70%→80%".

Neue Round-2 Findings

R2-M1 (MEDIUM) — Task 8.5 Step 4: Potential-Duplikat bei Rollback-Env-Merge

# Task 8.5 Step 4:
existing = mcp__dockhand__get_stack_env(...)
merged = list(existing["variables"]) + [
    {"key": "PRINTER_CONFIG_PATH", "value": "/etc/printer-hub/printers.yaml", ...},
]

Wenn PRINTER_CONFIG_PATH zu diesem Zeitpunkt noch in existing ist (z.B. weil Task 8.3 Step 1 partiell fehlgeschlagen ist und der Key nicht entfernt wurde), enthält merged den Key zweimal. Dockhand-Verhalten bei Duplikaten ist undokumentiert — könnte zu unerwartetem Compose-State führen.

Fix: vor dem Concat explizit filtern:

merged = [v for v in existing["variables"] if v["key"] != "PRINTER_CONFIG_PATH"] + [
    {"key": "PRINTER_CONFIG_PATH", "value": "/etc/printer-hub/printers.yaml", "isSecret": False},
]

Das ist symmetrisch zu Task 8.3 Step 1 und macht den Rollback idempotent.


R2-L1 (LOW) — Task 8.5 Step 3: PRE_DEPLOY_COMPOSE_CONTENT nicht explizit gesichert

Task 8.5 Step 3 referenziert PRE_DEPLOY_COMPOSE_CONTENT als Variable "aus Phase 0 Live-Check festgehalten". Phase 0 Step 4 sichert aber nur das DB-Schema (sqlite3 .schema printers), nicht den Compose-Inhalt.

Der Rollback-Pfad setzt voraus dass der Agent den originalen Compose-Content irgendwo hat — das ist nur der Fall wenn Task 6.2 Step 1 (get_stack_compose) ausgeführt wurde und das Ergebnis in der phase0-Doku notiert ist. Task 6.2 Step 3 sagt "in phase0-live-check-results.md festhalten", aber nicht explizit dass der volle Compose-Content gespeichert werden muss (nur Vault-Item-UUID + Label-Notizen).

Fix: In Task 0.1 (Phase 0) einen zusätzlichen Step ergänzen:

Step 4b: Compose-Inhalt für Rollback sichern

compose = mcp__dockhand__get_stack_compose(environmentId=10, name="hangar-print-hub")
# Vollständigen content in phase0-live-check-results.md als Code-Block speichern

Expected: YAML-Content in phase0-Doku, damit Task 8.5 Step 3 darauf zugreifen kann.

Alternativ in Task 8.5 Step 3 den Abruf direkt vor dem Revert machen (aber dann ist fraglich ob der Container schon crasht und get_stack_compose noch funktioniert). Phase-0-Sicherung ist robuster.


R2-INFO — Task 2.7 Test-Konsistenz: kein Query-Param mehr, Test OK

test_get_api_printers_include_disabled_query_param_admin_only prüft dass der ?include_disabled=true Query-Param auf dem Public-Endpoint ignoriert wird (disabled bleibt raus). Das ist konsistent mit dem Route-Code in Step 3 der keinen include_disabled-Parameter kennt — printers_repo.list_all(session) ohne Argument filtert immer. Kein Bug, kein Fix nötig.


Zusammenfassung

R2-M1 ist ein echter Rollback-Korrektheitsfehler — im Fehlerfall (den dieser Task absichern soll) kann der Env-State inkonsistent werden. Bitte vor Round-3 adressieren. R2-L1 ist ein Dokumentationslücke die den Rollback-Pfad fragil macht.

@strausmann

Copy link
Copy Markdown
Owner Author

Plan-Review Round-3: ops-Team

Status: APPROVE

Round-2 Findings — Verifikation

  • R2-L1 Compose-Content (Phase 0 Step 4b): Adressiert. get_stack_compose()["content"] wird gesichert und als Code-Block unter ## Pre-Deploy Compose-Snapshot in phase0-live-check-results.md eingetragen. Task 8.5 Step 3 referenziert PRE_DEPLOY_COMPOSE_CONTENT konsistent. Reicht vollständig.

  • R2-M2 Task 8.5 Step 4 Filter (cross-team mit code-q): Adressiert. Filter [v for v in existing["variables"] if v["key"] != "PRINTER_CONFIG_PATH"] vor dem Append ist analog zu 8.3 Step 1 — verhindert Duplikat auch bei partiellem 8.3-Durchlauf. Korrekt.

Round-3 Spezifisch

Task 8.3.5 Reihenfolge (R2-L2): Verschiebung nach Phase 8 ist korrekt. sleep 60 als Newt-Sync-Puffer ist ein bewährter Ansatz (Newt-Sync-Zeit aus HomeLab-Memory bestätigt). Reihenfolge 8.3 (Stack-Restart) → 8.3.5 (curl mit sleep 60) → 8.4 (Smoke-Test) ist logisch zwingend — Pangolin sieht Labels erst nach Stack-Restart. Sinnvoll.

Phase 0 Step 3 Bestand-Detection (R2-L3): 4-Fälle-Entscheidungsbaum deckt alle Zustände ab: null/claude-automation/fremder-User/healthCheck-disabled. Der STOP-Fall bei fremdem User ist wichtig und korrekt als manuelle Klärung markiert. Vollständig.

Keine neuen Round-3-Findings aus ops-Perspektive.


ops-Team Review, Sonnet 4.6

@strausmann

Copy link
Copy Markdown
Owner Author

Plan-Review Round-3: network-Team

Status: APPROVE

Round-2 Findings — Verifikation

R2-L2 Task 8.3.5 Verschiebung: adressiert. Task 6.3 ist jetzt ein leerer Platzhalter mit explizitem Hinweis auf Task 8.3.5. Die Positionierung zwischen 8.3 (start_stack) und 8.4 (Smoke-Test) ist korrekt.

R2-L3 Bestand-Detection: adressiert. Phase 0 Step 3 enthält einen vollständigen 4-Fall-Entscheidungsbaum (null / claude-automation / anderer User / healthCheck.disabled).

Round-3 Findings

Task 8.3.5 sleep 60 — akzeptabel mit Vorbehalt (LOW):
Step 1 enthält sleep 60 als Puffer. 60 Sekunden sind ein Minimum; Newt-Sync kann bis zu 5 Minuten dauern. Step 5 hat einen expliziten Retry-Pfad ("weitere 2 Min warten und wiederholen"), der das abfängt. Ein until-Loop mit Timeout wäre präziser, aber der Retry-Pfad ist klar genug beschrieben. Kein NEEDS_FIXES.

Bestand-Detection — ssoEnabled == false nicht im Baum (LOW):
Der Entscheidungsbaum prüft headerAuth und healthCheck, aber nicht explizit auth.ssoEnabled == false. In der Praxis ist SSO für print-hub aktiv. Falls es abgeschaltet wäre, würde Phase 6.2 auth.sso-enabled=true trotzdem setzen — was korrekt ist. Der fehlende Fall ist kein Blocking-Issue. Kein NEEDS_FIXES.

Task 6.2 Compose-Snippet: Passwort-Klartext korrekt auf pangolin-resource-standard.md-Workaround referenziert. LGTM.

@strausmann

Copy link
Copy Markdown
Owner Author

Plan-Review Round-3: storage-Team

Status: APPROVE (mit einem LOW-Hinweis)


Round-2 Findings — Verifikation

R2-M1 run_sync: vollständig adressiert.

Task 1.3, Zeilen 488-493: der Test ruft jetzt await conn.run_sync(mig._backfill_snmp) auf. _backfill_snmp(bind) nimmt eine sync Connection als erstes Argument — genau das liefert run_sync. Die Coroutine wird jetzt tatsächlich ausgeführt. Analogie zu alembic env.py ist korrekt gewählt. Idempotenz-Prüfung (2x-Aufruf) ist damit funktional. ✅


Neue Round-3 Findings

R3-L1 (LOW): Task 8.5 Step 2 — WAL/SHM-Reihenfolge semantisch falsch

Aktuell: cp backup → db dann rm -f db-wal db-shm

SQLite WAL-Semantik: Das alte .db-wal enthält Transaktionen nach dem Backup-Zeitpunkt. Wenn das restaurierte .db zusammen mit dem alten .db-wal im Filesystem liegt — auch nur für Millisekunden — kann SQLite beim nächsten Open einen WAL-Recovery versuchen der nicht zum Checkpoint-Stand des Backups passt. Stack ist zwar gestoppt (Step 1), aber die sicherere Reihenfolge ist:

Risikobewertung: LOW — Stack ist in Step 1 gestoppt, kein Prozess öffnet die DB zwischen cp und rm. In der Praxis kein Crash-Risiko, aber semantisch ist rm-vor-cp die korrekte Reihenfolge bei SQLite-WAL-Restore.


Alle anderen Aspekte (R2-M2 Filter in Step 4, R2-L1 Phase-0-Sicherung, Rollback-Gesamtstruktur) sind korrekt. Plan ist umsetzungsbereit.

@strausmann

Copy link
Copy Markdown
Owner Author

Plan-Review Round-3: code-quality

Status: APPROVE


Round-2 Findings — Verifikation

  • R2-M2 Task 8.5 Step 4 Filter: vollständig adressiert. Das Filter-Pattern ist korrekt und konsistent mit 8.3 Step 1:

    merged = [v for v in existing["variables"] if v["key"] != "PRINTER_CONFIG_PATH"]
    merged.append({"key": "PRINTER_CONFIG_PATH", "value": "...", "isSecret": False})

    isSecret: False ist korrekt für einen YAML-Pfad — kein Secret-Wert. Kommentar "Filter analog 8.3" macht die Intention explizit. ✅

  • R2-L1 PRE_DEPLOY_COMPOSE_CONTENT: Phase 0 Step 4b ist klar formuliert. Der Aufruf, die Sicherungsanweisung als ## Pre-Deploy Compose-Snapshot Code-Block und die Referenz content=PRE_DEPLOY_COMPOSE_CONTENT in Task 8.5 Step 3 bilden eine geschlossene Kette. Implementer kann die Variable nicht übersehen. ✅


Neue Round-3 Findings

R3-L1 (LOW): Task 6.3 Verweis-Stub — Navigation leicht riskant

Der 3-Zeilen-Stub in Phase 6 lautet sinngemäß: "Dieser Schritt wurde nach Task 8.3.5 verschoben." Das ist korrekt, aber ein Implementer der sequenziell arbeitet könnte in Phase 6 stoppen und denken, die curl-Verifikation entfällt. Der Stub enthält keinen direkten Anker-Link auf Task 8.3.5, nur "Siehe Task 8.3.5 unten". In einem 4000-Zeilen-Plan ist das ausreichend, wenn der Implementer von oben nach unten arbeitet — aber bei Subagent-Driven-Development, wo Agents direkt auf benannte Tasks dispatcht werden, könnte Phase 6 ohne 8.3.5 als vollständig erscheinen. Empfehlung: Stub mit explizitem Hinweis "8.3.5 ist Pflicht-Schritt, KEIN optional" ergänzen. Kein Blocker.

R3-N1 (NOTE): Phase 0 Step 3 STOP-Fall — Fallback vorhanden, ausreichend

Der Entscheidungsbaum hat einen headerAuth.user != "claude-automation" STOP-Fall. Dieser Fall fordert "manuelle Klärung mit User: Entscheidung ersetzen oder zweiten Bypass-Account anlegen?" — das ist korrekt und realistisch für einen HomeLab-Betrieb. Ein Issue-Kommentar-Template fehlt, aber der STOP-Fall blockiert den Subagent sauber ohne dass er rät. Akzeptabel.

R3-N2 (NOTE): Coverage-Tabelle vs. Task 8.3.5

Task 8.3.5 ist ein reiner curl-Verifikations-Task ohne neuen Code — keine Coverage-Implikation. Die Coverage-Tabelle referenziert ihn korrekt nicht. ✅


Gesamtbewertung

Alle Round-2 Findings korrekt und vollständig adressiert. R3-L1 ist ein Low-Risiko Navigations-Hinweis der bei sequenzieller Ausführung nicht relevant ist. Plan ist umsetzungsreif.

@strausmann

Copy link
Copy Markdown
Owner Author

Spec-Reset Round-1: ops-Team

Status: NEEDS_FIXES (2 HIGH, 1 MEDIUM — kein Blocker für Implementierung, aber vor Deploy-Phase A schließen)


Findings

HIGH

H1 — Rollback zu vage für Implementer

Spec sagt nur: DB-Restore + Compose-Revert + Watchtower wieder auf any. Details analog alter Spec Phase 8.5.

Die alte Spec (2026-06-14) ist als obsolet markiert. Ein Implementer der die alte Spec nicht kennt kann den Rollback nicht ausführen. Konkret fehlt:

# Was ein Implementer braucht:
docker cp label-printer-hub-backend:/data/printer-hub.db.bak-pre-124 /tmp/
docker exec label-printer-hub-backend sh -c   "sqlite3 /data/printer-hub.db '.restore /data/printer-hub.db.bak-pre-124'"
mcp__dockhand__update_stack_compose(env=10, name='label-printer-hub',
  content='<Compose-Snapshot aus Phase A.3>', restart=True)
mcp__dockhand__set_container_auto_update(env=10,
  containerName='label-printer-hub-backend', policy='any')
mcp__dockhand__set_container_auto_update(env=10,
  containerName='label-printer-hub-frontend', policy='any')

Fix: Rollback-Abschnitt in der Spec ausschreiben. Referenz auf veraltete Spec entfernen.

H2 — Vault-Item für headerAuthId 8 nicht verifiziert

Spec benennt R5 korrekt, erklärt aber nicht was Phase 0 konkret tun soll. headerAuthId 8 erscheint in der Pangolin-API-Antwort im -Feld, nicht im -Feld — der User-Name ist aus dem Live-Check nicht ablesbar.

Wenn beim Smoke-Test in Phase C ein curl -u claude-automation:<pw> scheitert, ist unklar welches Vault-Item der Implementer öffnen soll.

Fix: Phase 0 muss explizit: mcp__pangolin-api__get_header_auth(headerAuthId=8) aufrufen (oder API-Äquivalent) und das Vault-Item mit Name + UUID dokumentieren. Ergebnis in Phase-0-Doku festhalten, NICHT im PR.


MEDIUM

M1 — Migration schlägt fehl wenn Backend-Container-Start abbricht

Spec beschreibt: Alembic-Migration läuft beim Container-Start. Es gibt keinen expliziten Hinweis was passiert wenn Alembic mit einem Fehler abbricht (z.B. SQLite locked wegen WAL-Reader, Schema-Konflikt).

Healthcheck curl http://localhost:8000/healthz prüft ob die App antwortet — aber wenn Alembic die Migration abbricht, startet FastAPI nicht. Container geht in unhealthy → autoheal könnte Restart triggern → Migration läuft erneut → Loop.

Mitigation: Alembic-Migrationen sind idempotent (Standard-Verhalten). Das löst den Loop. Aber der Implementer sollte explizit wissen: docker logs label-printer-hub-backend | grep alembic ist der erste Diagnoseschritt wenn Healthcheck rot bleibt.

Fix: Einen Satz in Phase B ergänzen: Bei Healthcheck-Rot sofort Backend-Logs auf Alembic-Fehler prüfen (docker logs label-printer-hub-backend). Alembic-Migration ist idempotent — manueller Restart löst temporäre Locks.


LOW

L1 — HUB_VERSION Migration nicht addressiert

Spec erwähnt korrekt dass Production auf feat-first-print läuft. Der Zeitpunkt des Branch-Merge nach main ist offen. Wenn Watchtower dann main-Tag zieht ohne dass Issue #124 deployed ist → falsches Image. Das ist Scope-Out, aber der Implementer sollte einen Hinweis bekommen: Working-Branch MUSS von origin/feat/first-print abzweigen (steht in Akzeptanzkriterien ✓) und das PR-Merge-Target ist ebenfalls feat/first-print, nicht main.


Was gut ist

  • Live-State-Tabelle vollständig und verifiziert — kein Rate gegen Doku-Stand
  • Watchtower-Pause für BEIDE Container explizit (R6 + Phase A.2) — wichtig, oft vergessen
  • Bootstrap-Marker hub_meta.printers_bootstrap_done verhindert ENV-Override nach Operator-Pflege — sauber
  • Compose-Snapshot in Phase A.3 als Rollback-Basis explizit erwähnt
  • derive_printer_id 4-arg mit timezone-aware Pflicht ist ein solider Deterministik-Anker
  • Coverage-Schwellen konkret benannt (85% / 80% / 85%)
  • Pangolin-Resource wird korrekt als keine Aktion nötig klassifiziert — kein unnecessary Re-Setup

ops-Team Review — Spec-Reset Round-1 — 2026-06-19

@strausmann

Copy link
Copy Markdown
Owner Author

Spec-Reset Round-1: network-Team

Status: NEEDS_FIXES (2 Punkte, 1 davon CRITICAL)


Findings

CRITICAL

F1 — healthcheck.hostname fehlt im Live-State

Die Spec zeigt unter Live-State nur hcEnabled: true, healthy — ohne healthcheck.hostname. Laut pangolin-resource-standard.md ist healthcheck.hostname seit Newt v1.18.4 ein Pflichtfeld (führt zu Blueprint-Validierungsfehler der alle anderen Resources auf dem Node blockiert).

Der Live-State-Dump (mcp__pangolin-api__resource_by_resourceId(123)) zeigt hcEnabled in der Targets-Liste — aber das hcHostname-Feld ist nicht sichtbar. Unklar ob es gesetzt ist und im Dump nur nicht angezeigt wird, oder ob es fehlt.

Aktion für Implementer (Phase 0): Zielwert muss label-printer-hub-frontend (Container-Name) sein, da das Target auf Docker-Containernamen läuft. Falls hcHostname fehlt oder leer ist: via Pangolin API setzen. Da Resource via Blueprint-Labels verwaltet wird, muss die Änderung in den Labels erfolgen, nicht via API-PUT.


MEDIUM

F2 — Remote-User → updated_by: Propagation-Mechanismus nicht spezifiziert

Spec sagt (Abschnitt "Frontend-Änderungen / Auth"): "Frontend liest Remote-User aus Request-Header, nutzt als updated_by beim Backend-Call. Backend prüft Authorization-Header (API-Key) für den eigentlichen Call."

Offen: Über welchen HTTP-Header übergibt das Frontend den Remote-User-Wert ans Backend? Pangolin reicht Remote-User nur bis zum Frontend durch, nicht weiter. Ohne explizite Spezifikation entscheidet der Implementer willkürlich (z.B. X-Remote-User, X-Forwarded-User, oder gar nicht).

Empfehlung: Spec explizit machen: Frontend setzt X-Remote-User: <pangolin-remote-user> Header im Backend-Call. Backend liest updated_by aus diesem Header (Fallback: "system"). Das ist ein klar definierbares Muster, kein Sicherheitsproblem — das Backend ist intern und nicht direkt aus dem Internet erreichbar.


LOW / INFO

F3 — Service-Account-API-Key Quelle nicht spezifiziert

Spec erwähnt "Service-Account-API-Key" für Frontend→Backend-Kommunikation ohne Vault-Item-Referenz. Kein Blocker für den Implementer (der legt den Key an), aber Audit-Trail wird mit updated_by-Wert aus dem Header befüllt — der muss konsistent sein. Empfehlung: Vault-Item-Name im Akzeptanzkriterium ergänzen oder zumindest "Frontend-Service-Account-Key in Vaultwarden anlegen" als Schritt in Phase A.

F4 — R5 (headerAuthId 8): Aktion bei Nicht-Konformität fehlt

Spec sagt: "Phase 0 muss klären welcher User/Pass aktiv ist." Kein konkreter Schritt falls User NICHT claude-automation ist. Empfehlung: Einen Satz ergänzen: "Falls headerAuthId=8 einen anderen User als claude-automation verwendet: bestehende Header-Auth via Pangolin API aktualisieren (oder neues Item anlegen + headerAuthId austauschen). Blueprint-Label muss dann auth.basic-auth.user=claude-automation widerspiegeln."

F5 — CSRF-Entscheidung: delegieren ist OK

"Implementer prüft existing Pattern" ist akzeptabel — admin_api_keys.html ist das Vorbild und das Muster ist bereits etabliert. Kein Spec-Änderungsbedarf. Nur sicherstellen dass das Akzeptanzkriterium einen CSRF-Check enthält (aktuell fehlt er in der Checkliste).


Network-Perspektive: Kein Pangolin-Resource-Änderungsbedarf

Bestätigt: Issue #124 erfordert keine Änderung an Resource 123, SSO-Konfiguration, Routing oder DNS. Die Two-Container-Architektur (Frontend :8080, Backend intern :8000) ist korrekt im Target abgebildet.

Offene Validierung vor Merge: F1 muss verifiziert sein (hcHostname gesetzt). Alles andere kann der Implementer direkt lösen.

strausmann and others added 24 commits June 20, 2026 17:05
Neuer Plan basierend auf Spec Round-6 Working Draft. Strategie-Wechsel:
nicht weiter Spec-Annahmen-Fehler korrigieren, sondern Plan robust
gegen sie machen.

Plan-Prinzip:
- Phase 0 sammelt ALLE Live-Werte (Mount-Pfade, API-Routes,
  ENV-VARS, DB-Stand) in docs/superpowers/plans/2026-06-19-
  phase0-live-state.md.
- Jede weitere Phase startet mit Pre-Check-Step der relevante
  Werte zieht.
- Bei Konflikt Spec ↔ Live-Container: NIEMALS Spec-Wert nutzen,
  IMMER Live-Wert + PR-Kommentar mit Befund.

Struktur:
- 22 Tasks in 9 Phasen
- Backend (Python, 13 Tasks): Foundation + Service-Layer + JSON-API
  + PrintService + Removal
- Frontend (Go, 4 Tasks): gorilla/csrf nachgeruestet auf existing
  Admin-Routes + admin_printers.go Handler + 3 Templates +
  Router-Wireup mit oapi-codegen Update
- Phase 6.0 Bootstrap: SSH-Direktaufruf in Backend-Container fuer
  Service-Account-Key (kein Pangolin-Bootstrap)
- Production-Deploy mit LIVE-Pfaden:
  /docker/stacks/hangar-print-hub/data/hub/printer-hub.db
  /docker/stacks/hangar-print-hub/config/printers.yaml

CSRF-Hardening eingerollt: existing Admin-API-Keys-Routes werden
in derselben Migration mit csrfMW gesichert.

Refs #124

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round-5 Reviews: ops/network/storage APPROVE, code-quality NEEDS_FIXES.

HIGH
- H1 (code-q) Bootstrap-Code referenzierte nicht-existente APIKeyService.
  Echte Codebase nutzt Repository-Pattern: api_keys_repo.create(session,
  ApiKey). Korrigiert mit bcrypt.hashpw + secrets.token_urlsafe + echter
  ApiKey-Konstruktion. Plus Implementer-Verifikation per inspect.signature
  vor Ausfuehrung.

MEDIUM
- M1 (code-q) Scope 'admin:printers' → 'admin'. Existing Codebase nutzt
  3-stufige Hierarchie read/print/admin (admin ⊇ print ⊇ read), KEIN
  fine-grained admin:* Scope. Bootstrap-Key mit ['admin'].
- M2 (ops) compose-passthrough-Pflicht fuer Secrets: BACKEND_SERVICE_
  ACCOUNT_KEY + CSRF_KEY muessen in compose.yaml unter environment: als
  ${VAR} deklariert sein, sonst Silent-Failure. Neue Phase 6.0 Step 4b
  mit Verifikations-Befehl docker exec env grep.

LOW
- ops: get_stack_env Baseline als Pre-Check + Image-Digest beider
  Container vor Deploy festhalten fuer Phase 8.5 Rollback ohne Raten.
- network: headerAuthId 8 ist NICHT in /v1/resource API sichtbar, statt-
  dessen Vault-Item Smoke-Test (curl -u claude-automation). Plus
  Pangolin Bug #3099 Hinweis in Phase 8.4 Smoke (Browser zeigt
  Basic-Auth-Dialog statt SSO-Redirect, Cancel → SSO).

Refs #124

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Privacy / secret scan CI-Fail behoben:

- Verworfene Reset-Spec geloescht (2026-06-19-printers-env-to-db-design.md)
- Obsoleter Round-4-Plan geloescht (2026-06-14-printers-yaml-to-db-plan.md)
- Aktive Spec + Plan: alle privaten Artefakte ersetzt:
  - Domains *.strausmann.cloud/de/net → *.example.test
  - Hostnames hhdocker0X → docker-prod-node / docker-secondary-node
  - Private IPs (172.16.x.x) → 192.0.2.x (RFC 5737)

Plan Round-7 Fixes (code-quality Round-6 NEEDS_FIXES):

- H1: Bootstrap-Code nutzt jetzt generate_api_key() Helper aus
  app/auth/key_generator.py statt manueller Key-Erzeugung. Liefert
  korrektes Format lh_pat_<43-char> + 16-char-Prefix. Sonst stille
  401-Fehler.
- H2: Vault-Notes Scope-Klaerung 'admin (3-stufig: admin ⊇ print ⊇
  read)' statt verwirrendem 'admin:printers'.
- M2: oapi-codegen Workflow konkretisiert — make gen-client ruft
  laufendes Backend per curl ab, NICHT lokale Datei. Phase 7.2 mit
  zwei Optionen (lokal uvicorn vs. Remote via Pangolin Bypass).

Refs #124

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Gemini-Re-Review nach Privacy-Push hat 3 MED-Findings aufgezeigt:

- CSRF Spec-Snippet (Round-4 obsolete-marker Block) zeigte
  len(csrfKey) != 32 statt korrekt != 64 (64 hex chars = 32 raw
  bytes). Inline-Fix mit Kommentar.
- Privacy-Pedantik: docker-prod-node und id_ed25519_homelab_nodes
  als zu spezifisch. Auf prod-node.example.test +
  id_ed25519_placeholder umgestellt.
- Plan Task 1.1: existing _apply_pragmas-Listener in
  backend/app/db/engine.py setzt schon WAL + foreign_keys +
  synchronous + busy_timeout. Plan-Anweisung NUR
  isolation_level=SERIALIZABLE ergaenzen, KEINEN zweiten Listener
  einfuehren.

Refs #124

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Round-1-4-Backup-Block in Spec explizit als OBSOLET markiert
  (verwendete /docker/stacks/label/... statt live-verified
  /docker/stacks/hangar-print-hub/data/hub/).
- L3-Anhang-Note korrigiert: Live-Pfad ist NICHT der per
  STACKS_BASE_HOMEDIR abgeleitete /docker/stacks/label/-Pfad,
  sondern /docker/stacks/hangar-print-hub/data/hub/.
- Plan Rollback Step 2 jetzt mit ABSOLUTEN Pfaden statt relativ.

example.com vs example.test: bleibt bei .test (RFC 6761 reserved
TLD fuer testing, auch privacy-scan-compliant).

Refs #124

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Live-verifiziert via docker inspect + docker exec + git show:
- DB-Pfad: /docker/stacks/hangar-print-hub/data/hub/printer-hub.db
- Image-SHAs beider Container fuer Rollback
- 2 Bestandsdrucker (brother-p750w + brother-ql820nwb)
- 8 Tables existing, printers_audit fehlt noch
- Frontend HAT KEINE CSRF-Library — gorilla/csrf in Phase 7.1 neu
- engine.py _apply_pragmas existiert bereits (Plan Task 1.1: nur
  isolation_level=SERIALIZABLE ergaenzen, KEIN zweiter Listener)

Refs #124
…verwendet

isolation_level="SERIALIZABLE" explizit an create_async_engine() übergeben.
Kein zweiter Connect-Listener nötig — der bestehende _apply_pragmas-Hook
setzt bereits journal_mode=WAL, synchronous=NORMAL, foreign_keys=ON und
busy_timeout=5000 korrekt.

Tests in test_engine_pragmas.py prüfen:
- dialect._on_connect_isolation_level == "SERIALIZABLE" (explizit gesetzt)
- WAL + foreign_keys und SERIALIZABLE koexistieren ohne Konflikte
Neue Exception-Klasse PrinterDisabledError als Subclass von PrinterError.
Konstruktor nimmt printer_id (UUID) und slug positional entgegen und
mappt semantisch auf HTTP 409 (Drucker existiert, ist nur deaktiviert).

TDD: 3 neue Tests (Hierarchie, Attribut-Speicherung, str-Inhalt) erst
auf FAIL geprüft, dann Implementierung ergänzt — alle 940 Tests grün.
…ackfill

- Neue Spalten queue_timeout_s (Integer, NOT NULL, default 30) und
  cut_defaults_half_cut (Boolean, NOT NULL, default false) auf printers-Tabelle
- Neue printers_audit-Tabelle mit 8 Pflichtfeldern und 2 Indizes
  (idx_printers_audit_printer_id, idx_printers_audit_created_at_desc)
- _backfill_snmp als top-level Funktion (testbar via run_sync):
  befüllt connection.snmp für Bestandsdrucker mit host-Feld,
  idempotent (überspringt rows mit vorhandenem snmp)
- PrinterAudit SQLModel-Klasse in printer.py + __init__.py Export
  damit alembic check keinen Drift erkennt
- 3 TDD-Tests (T1: Spalten, T2: Audit-Tabelle+Indizes, T3: Backfill)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ben (DESC)

- `text("created_at DESC")` in `__table_args__` von PrinterAudit konsistent
  mit der Migration `42fbd015698d` (die bereits `sa.text("created_at DESC")`
  verwendet)
- `text`-Import in `models/printer.py` ergänzt
- `alembic/env.py`: `_include_object`-Hook hinzugefügt der Text-Expression-
  Indexes vom Autogenerate-Vergleich ausschließt — SQLite kann solche Indexes
  nicht reflektieren, was sonst einen False-Positive-Drift in `alembic check`
  erzeugt; die Migration selbst erstellt den Index weiterhin korrekt

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Erweitert derive_printer_id(model, host, port) auf 4-arg-Signatur mit
pflichtmäßigem created_at_utc (timezone-aware datetime). Naive datetime
löst ValueError aus (TZ-Sensitivität explizit gewollt).

upsert_runtime_printers nutzt Slug-Lookup für bestehende Drucker (liest
created_at aus der DB → stabile UUID über Neustarts), und datetime.now(UTC)
für neue Drucker. Tests die auf 3-arg-UUID-Stabilität angewiesen sind
temporär mit @pytest.mark.skip markiert (Issue #124 Phase 5).

Coverage printer_identity.py: 100% (10 Unit-Tests).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Task 2.1: Erstellt `backend/app/schemas/printer_admin.py` mit
SNMPConfig, PrinterConnection, PrinterCutDefaults, PrinterQueueSettings,
PrinterCreatePayload und PrinterUpdatePayload als Pydantic v2-Schemas.

- SNMPConfig: discover=False-Default, community-Pflicht bei discover=True
- SLUG_PATTERN r"^[a-z0-9][a-z0-9-]{1,62}[a-z0-9]$" erzwungen
- Backend-Literal lehnt unbekannte Werte im Schema ab
- PrinterUpdatePayload: alle Felder optional (PATCH-Semantik)
- 38 Tests (100% Coverage), mypy strict clean, ruff clean

Refs #124
Fügt `app/services/audit_redaction.py` hinzu: Deep-Copy-basierter
Redaction-Helper der bekannte Secret-Pfade (z.B. connection.snmp.community)
durch '***REDACTED***' ersetzt bevor before_json/after_json in printers_audit
geschrieben werden.

- SECRET_PATHS als frozenset[tuple[str,...]] erweiterbar
- None-Werte werden NICHT redacted (fehlende Werte nicht verschleiern)
- Fehlende Zwischenpfade werden stillschweigend übersprungen
- 5 TDD-Tests, 89% Coverage (≥80% Gate bestanden)
- mypy strict + ruff clean

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Neue Registry-Schicht für verfügbare (backend, model)-Kombinationen.
Liest aus ptouch.PRINTERS (Spec-Pfad) bzw. ptouch.printers-Klassen
sowie brother_ql.models.MODELS (Spec-Pfad) bzw. ALL_MODELS (0.9.4 API).
Fällt auf HARDCODED_FALLBACK_MODELS zurück wenn beide Plugins leer sind.

- 12 Tests (TDD), 88% Coverage, mypy strict clean, ruff clean
- PrinterModel frozen dataclass, beide Plugin-APIs zweistufig abgesichert
…rding

Implementiert Tasks 2.4 + 2.5 aus Issue #124 (Printers YAML-to-DB Migration).

Flattening-Helper (top-level Funktionen):
- _payload_to_row: Pydantic-Payload → flache DB-row (connection als JSON, queue/cut_defaults flat)
- _apply_update_patch: PATCH-Semantik, gibt nur geänderte Spalten zurück
- _row_to_audit_view: Rekonstruiert verschachtelte Audit-JSON-Darstellung

PrinterAdminService (AsyncSession + audit_user):
- create_printer: UUIDv5 via derive_printer_id, flush+commit, DuplicateSlug/NameError
- update_printer: PATCH-Semantik, setattr per change, PrinterNotFoundBySlugError
- disable_printer / enable_printer: Soft-Delete mit PrinterAlreadyDisabled/EnabledError
- list_printers: include_disabled=False (default) filtert deaktivierte aus
- _record_audit: INSERT printers_audit mit redact_secrets(before/after)

Tests: 42 Tests (TDD), Coverage 98.80% (≥85%-Pflicht erfüllt)
mypy --strict: clean, ruff: clean

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…disabled

Task 2.6: list_all erhält include_disabled=False als Keyword-Only-Default.
Standard-Aufrufe schließen deaktivierte Drucker aus (Soft-Delete-Filter).
Admin-UI kann include_disabled=True übergeben für die vollständige Liste.

Task 2.7: GET /api/printers ruft list_all ohne include_disabled — Public-API
filtert deaktivierte Drucker immer heraus. Kein neuer Query-Parameter.

Tests: 4 Repo-Unit-Tests (test_printers_repo.py) + 2 Route-Tests ergänzt.
Volle Suite: 1042 passed, 13 skipped.

Refs #124
Implementiert die JSON-Admin-API für Drucker-Verwaltung (Task 3.1):

- GET    /api/v1/admin/printers                — Liste (include_disabled-Filter)
- POST   /api/v1/admin/printers                — Anlegen (201, 409 bei Dup-Slug/Name)
- GET    /api/v1/admin/printers/{slug}         — Einzelabruf (200, 404)
- PUT    /api/v1/admin/printers/{slug}         — Aktualisieren via PATCH-Semantik (200, 404)
- POST   /api/v1/admin/printers/{slug}/disable — Soft-Delete (200, 404, 409 already_disabled)
- POST   /api/v1/admin/printers/{slug}/enable  — Reaktivierung (200, 404, 409 already_enabled)

Auth: require_admin (analog admin_api_keys.py), kein CSRF, nur JSON.
Service-Layer: PrinterAdminService mit Audit-Trail-Unterstützung.
Tests: 19 Unit-Tests (TDD, 100% Coverage bei korrektem async-Concurrency-Tracking).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ledError

- PrintService.__init__ erhält printer_slug + printer_enabled (default True)
- submit_print_job prüft am Anfang ob Drucker aktiv ist; wirft PrinterDisabledError
  bevor preflight_check oder queue.submit aufgerufen werden
- POST /print Route: PrinterDisabledError → 409 mit Body {"error": "printer_disabled", "slug": "..."}
  (spec-konformes Format, abweichend von error_code-Muster der anderen Fehler)
- 5 neue Tests: 3 service-level + 1 http-level + 1 enabled-happy-path

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tfernt

Phase 5: YAML→DB-Sync-Pfad vollständig entfernt. Lifespan lädt Drucker
jetzt ausschließlich aus der DB (SELECT Printer WHERE enabled=true).

Entfernt:
- app/schemas/printer_config.py (PrinterYAMLConfig, SNMPConfig, etc.)
- app/services/printer_config_loader.py (PrinterConfigLoader)
- tests/db/test_lifespan.py
- tests/unit/test_lifespan.py
- tests/services/test_printer_config_loader.py
- tests/integration/test_lifespan_seeds_and_upserts.py
- tests/integration/test_lifespan_multi_printer.py
- tests/integration/db/test_lifespan_printer_upsert.py

Geändert:
- app/db/lifespan.py: upsert_runtime_printers() + Imports entfernt
- app/main.py: PrinterConfigLoader → _build_configs_from_db() (DB-Loading),
  Empty-Printer-Guard für Fresh-Install (app.state.print_service=None)
- app/services/backend_router.py: PrinterYAMLConfig + verwandte Klassen
  von printer_config.py hierher verschoben (primärer Konsument)

Tests: 1035 passed, 19 skipped (printer-abhängige Tests auf Task C2
auto-seed-Drucker warten), 0 failed. mypy strict + ruff clean.
- gorilla/csrf v1.7.3 als direkte Dependency eingeführt
- buildCSRFMiddleware() liest CSRF_KEY (64 Hex-Zeichen = 32 Bytes) aus Env
- newRouter() nimmt csrfMW-Parameter; Admin-Subrouter (/admin/*) mit Middleware
- TemplateData.CSRFField (template.HTML) + baseData()-Helper für alle Admin-Handler
- admin_api_keys_create.html + admin_api_keys_detail.html: {{.CSRFField}} in POST-Forms
- csrf_test.go: 6 Tests (GET-OK, NoToken-403, WrongToken-403, ValidToken-200, TODO)
- main_test.go: newRouter()-Aufrufe auf 4-Parameter-Signatur aktualisiert

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ask 7.2)

Aktualisiert den OpenAPI-Snapshot und regeneriert den Go-Client um die 6 neuen
Admin-Printers-Endpoints aus Task 3.1 abzubilden:
  GET/POST /api/v1/admin/printers
  GET/PUT  /api/v1/admin/printers/{slug}
  POST     /api/v1/admin/printers/{slug}/disable
  POST     /api/v1/admin/printers/{slug}/enable

Änderungen:
- openapi.snapshot.json: Vollständig erneuert via Backend-Introspection ohne
  laufenden Server (mgr._app.openapi()); anyOf-null-Muster (OpenAPI 3.1) in
  nullable:true (3.0-kompatibel) normalisiert für oapi-codegen-Kompatibilität
- client.gen.go: Regeneriert mit oapi-codegen v2.7.1; enthält neue Admin-
  Printer-Typen (AppApiRoutesAdminPrintersApiPrinterRead, etc.)
- client.go: PrinterRead-Alias auf AppSchemasPrinterPrinterRead (Typ-Umbenennung
  durch oapi-codegen nach Hinzufügen zweier gleichnamiger Schemas);
  ListPrinters-Signatur um nil-Params ergänzt; ListTemplates als Stub der
  ErrNotImplemented zurückgibt (GET /api/templates in Phase 1k.1a entfernt)
- jobs.go: CreatedAt-Feld ist jetzt string statt time.Time (Schema-Änderung)
- Tests: template_test.go + templates_test.go + main_test.go auf 503-Erwartung
  aktualisiert; client_test.go: TestListTemplatesFiltersByApp durch
  TestListTemplatesReturnsErrNotImplemented ersetzt

CI-Check "oapi-codegen output is up to date": go test -race ./... grün.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r-Verwaltung (Tasks 7.3+7.4)

- admin_printers.go: 9 Handler (ListPrintersPage, NewPrinterPage, CreatePrinter,
  PrinterDetailPage, EditPrinterPage, UpdatePrinter, DisablePrinterConfirmPage,
  DisablePrinter, EnablePrinter) + WithSlug-Varianten für Tests
- admin_printers_test.go: 20 Tests, Coverage 70.8% (Happy-Path + Error-Path + Backend-Fehler)
- base.go: 4 neue page names + Stub-Templates für Tests
- 4 HTML-Templates: Liste, Formular (Create+Edit), Detail, Bestätigung-Deaktivierung
- main.go: 9 neue Routes unter /admin/printers (CSRF-geschützt)

go test -race ./... grün, go vet ./... sauber

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- EditPrinterPageWithSlug befüllt SNMP-Felder aus printer.Connection
  (verhindert silent data loss bei Edit-Submit ohne SNMP-Eingabe)
- admin_printers_form.html placeholder: 192.168.1.100 → 192.0.2.10
- Neuer Test TestEditPrinterPageWithSlug_PrefillsSnmpFields verifiziert
  Prefill von discover=true (checked) + community="public" (value)
- Coverage bleibt 71.2% (chi-Wrapper sind 2-Zeiler delegieren an WithSlug,
  die WithSlug-Varianten haben 76-100%)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@strausmann strausmann force-pushed the spec/printers-yaml-to-db branch from 5258a3e to 342243b Compare June 20, 2026 18:36
)

- app/main.py:304 Imports nach Stdlib/3rd-Party/App-Block sortiert (ruff I001)
- test_audit_redaction.py: 10.0.0.5 → 192.0.2.5 (RFC 5737 statt RFC 1918)

Privacy-Scan und Ruff-Lint waren in CI fehlgeschlagen, beide jetzt grün.
@strausmann

Copy link
Copy Markdown
Owner Author

Status Update — Phase 7 fertig

Phasen 0–7 alle commited + gepusht. Branch ist auf origin/main rebased.

Code-Commits (10 in Reihe nach Live-State):

  • ade07c0 Phase 1.1 Engine SERIALIZABLE
  • 461a4da Phase 1.2 PrinterDisabledError
  • 8968cfc Phase 1.3 Migration
  • 6862495 Phase 1.4 derive_printer_id
  • 2b6d7bc Phase 2.1 Schemas
  • b832d51 Phase 2.2 Audit-Redaction
  • 59eb54c Phase 2.3 Model-Registry
  • 3dc499b+f2a93a1 Phase 2.4–2.7 AdminService + Filter
  • b98cee3 Phase 3.1 JSON-API
  • 9c7adb0 Phase 4.1 PrintService enabled-Check
  • bbf2b59 Phase 5.1 PrinterConfigLoader-Removal
  • 9083868 Phase 7.1 gorilla/csrf
  • a3f30e8 Phase 7.2 oapi-codegen
  • 11f1f97+342243b Phase 7.3+7.4 Frontend Admin-UI
  • 51bc230 CI-Fix ruff + privacy

CI Status: 16/18 SUCCESS, 1 codecov/project FAILURE (-1.59% wegen skipped Phase-5 Tests pending Phase-6 Bootstrap), 1 in progress.

Phase 6 + Phase 8 ausstehend — brauchen User-Aktion:

  • Phase 6: Bootstrap Service-Account-Key + CSRF-Key + Pangolin Header-Auth Verifikation (operativ, nicht autonom)
  • Phase 8: Production-Deploy (PR mergen + DB-Migration + Stack-Update)

Bitte bei Wiederkunft: codecov-Threshold reviewen (akzeptabel weil pending bootstrap?) oder die skipped Tests entsperren?

@strausmann strausmann merged commit 41bef28 into main Jun 20, 2026
18 of 19 checks passed
@strausmann strausmann deleted the spec/printers-yaml-to-db branch June 20, 2026 19:21
github-actions Bot pushed a commit that referenced this pull request Jun 24, 2026
## 0.11.0 (2026-06-24)

* feat(auth): Pangolin-SSO + Bypass für alle Scopes trusted (ADR 0014) (#133) ([4fe1a91](4fe1a91)), closes [#133](#133) [#78](#78) [130/#132](#132)
* feat(nav): "Drucker" Link für Admin-Drucker-Verwaltung + getrennte ActiveNav-Werte (#135) ([1e982b0](1e982b0)), closes [#135](#135) [#104](#104) [#104](#104) [#104](#104)
* fix(frontend): forward Remote-User zum Backend (Pangolin SSO-Standard-Header) (#132) ([38c0cc3](38c0cc3)), closes [#132](#132) [#130](#130) [#130](#130) [#131](#131) [#130](#130)
* fix(frontend): forward X-Pangolin-Token zum Backend (Browser-User 503-Fix) (#130) ([5fb2038](5fb2038)), closes [#130](#130)
* fix(frontend): forwardAuth ergänzt X-Pangolin-Token + Remote-User (Admin-Routes) (#134) ([af9ee28](af9ee28)), closes [#134](#134) [130/#132](#132) [#130](#130) [#132](#132) [#133](#133)
* chore(deps): bump the go-minor-and-patch group across 1 directory with 2 updates (#128) ([a72dd90](a72dd90)), closes [#128](#128)
* ci(deps): bump lewagon/wait-on-check-action in the actions-all group (#127) ([e4139ab](e4139ab)), closes [#127](#127)
* docs(api): printers.yaml weg, Drucker in DB + /admin/printers Admin-UI (#124) [DRAFT] (#125) ([41bef28](41bef28)), closes [#124](#124) [#125](#125) [#124](#124) [#124](#124) [#124](#124) [#124](#124) [#124](#124) [#3099](https://github.com/strausmann/label-printer-hub/issues/3099) [#124](#124) [#124](#124) [#124](#124) [#124](#124) [#124](#124) [#124](#124) [#3099](https://github.com/strausmann/label-printer-hub/issues/3099) [#124](#124) [#124](#124) [#124](#124) [#124](#124) [#124](#124) [#124](#124) [#124](#124) [#124](#124) [#124](#124) [#124](#124) [#124](#124) [#124](#124) [#124](#124) [#124](#124) [#124](#124) [#124](#124) [compose-passthrou#Pflicht](https://github.com/compose-passthrou/issues/Pflicht)

[skip ci]
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.

2 participants