Audit: 41 unaddressed review findings across PRs #50-#65

[strausmann]

Was

Audit aller 13 zuletzt gemergeden PRs (PRs #50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 63, 64, 65) am 2026-05-16 ergab 77 Review-Findings, davon 41 unaddressed. Dieses Issue listet die unadressierten Findings priorisiert. Audit-Report (read-only): /tmp/pr-review-audit-label-printer-hub.md lokal.

Neue Governance-Rule review-feedback-policy.md (im homelab-management Repo) verhindert dass das wieder passiert: alle Reviews müssen vor Merge adressiert sein, Bot-Reviews bekommen ≥15 min Wartezeit nach letztem Push.

Adressiert (zu verifizieren)

• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 → Fix-PR fix(api): Phase 6b review-findings — 5 KRITISCH + 3 WICHTIG #66 (8 Findings): qr.py UUID, events.py raw HTML, Settings-Wiring, status_probe _last+online, queue_depth, callback coverage, subscriber counting

Offene Findings — KRITISCH

• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — print_service.py:88-102 — submit-then-pause race condition (tape-mismatch queue path). Worker kann Job dequeuen+printen BEVOR PAUSED-Transition durchläuft.
• PR feat(api): Phase 6a — REST API (spec + plan) #64 docs — Phase 6a spec/plan — mark_cancelled docs erlauben PRINTING-Cancel. Code ist OK (queued-only restricted in Phase 6a impl), aber spec/plan dokumentieren das alte Verhalten. (Niedrige Prio: Code richtig.)

Offene Findings — WICHTIG (Code-Bugs)

• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — print.py:124 — resume_printer dokumentiert 409 für "already active" aber returned das NIE. Broken contract.
• PR feat(printer-models): PrintQueue worker with pause/resume/cancel/retry #50 — print_queue.py:stop() — setzt nicht _done_event für in-flight jobs. wait_for_job() Callers hängen bis Timeout.
• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — print.py:154 — resume_job dokumentiert error_code=invalid_state in 409, implementiert aber nur plain HTTPException.

Offene Findings — MEDIUM (Performance + Cleanup)

• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — events.py:152 — kein initial state snapshot auf SSE connect. Clients sehen leere Widgets bis nächstes Event.
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — event_bus.py:98 — unsubscribe() clears nicht _dropped entries. UUIDs leaken über Reconnects.
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — config.py — SSE Settings haben keine Field(gt=0) Validation. sse_queue_size=0 → unbounded queue.
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — pyproject.toml + ci.yml — slow marker als "excluded from CI" dokumentiert aber CI excluded nicht. Wall-clock-Tests laufen in jedem CI-Run.
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — .env.example — fünf neue PRINTER_HUB_SSE_* env vars fehlen.
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — tape_change_producer.py:46 — hardcoded PT-series Tape-Lookup in einem generischen Service. Verletzt Plugin-Konvention (app/printer_models/<series>.py). Wird Problem bei QL-series.
• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — snmp_helper.py:110 — neue SnmpEngine pro SNMP-Query erstellt. Init lädt MIBs, perf-impact.
• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — main.py:182 — TemplateLoader.load_dir + ModelRegistry.ensure_discovered sync I/O in async lifespan. Blockiert event-loop bei Startup.
• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — main.py:374 — _LifespanManager in main.py (490 lines, >400 limit). Sollte nach app/utils/asgi.py.
• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — print_service.py:117 — Label-data resolution duplikat zwischen on_tape_mismatch=queue path und happy path.
• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — main.py:112,119 — _build_backend() und factory als Any typed. Verletzt mypy strict policy.
• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — README.md:5 — Codecov badge URL embed Token ?token=JRG4PDU2QX in public repo.
• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — backend/docs/ptouch-integration.md:10 — Doc sagt ptouch.LaminatedTape*mm, Code uses ptouch.Tape*mm. Doc ≠ code.
• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — tests/unit/test_config_printer.py:23 — Settings() loadet .env default. Non-hermetic für devs mit lokaler .env. Sollte Settings(_env_file=None).
• PR feat(label-renderer): Template schema + Pillow/qrcode renderer for 1-bit label bitmaps #54 — label_renderer.py — _load_font() ruft ImageFont.truetype() pro text element. Kein lru_cache.
• PR feat(label-renderer): Template schema + Pillow/qrcode renderer for 1-bit label bitmaps #54 — template.py — validator error message sagt "got None or 0" auch wenn negative values rejected. Misleading.
• PR feat(integrations): LabelData schema + Snipe-IT lookup client #51 — snipeit/plugin.py:55 — httpx.AsyncClient per lookup() call. Connection-Pool disabled. TODO existing, not fixed.
• PR feat(integrations): LabelData schema + Snipe-IT lookup client #51 — snipeit/plugin.py:67 — Response payload dict[str, Any]. Violates Any policy.
• PR feat(integrations): LabelData schema + Snipe-IT lookup client #51 — snipeit/plugin.py:52 — Kein Test asserts Authorization: Bearer ... Header. Future refactor könnte auth silently droppen.
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — events.py:208 — sse_events_published_total increment in per-subscriber loop. Eine publish() mit N Clients = N counts. Metric name "published" misleading.
• PR feat(integrations): AppLookupService aggregator — Phase 3 complete #53 — lookup_service.py — _clients typed str statt _AppName = Literal[...]. Static analysis verliert type safety.

Offene Findings — MEDIUM (DOCS — Spec/Plan-Bugs)

Diese betreffen die spec/plan-Markdowns; das Implementations-Code ist richtig — die Dokumentation aber irreführend für nächste Implementer.

• PR feat(api): Phase 6a — REST API (spec + plan) #64 spec/plan — LABEL_HUB_WEBHOOK_API_KEY (falsch) statt PRINTER_HUB_WEBHOOK_API_KEY
• PR feat(api): Phase 6a — REST API (spec + plan) #64 spec — Endpunkt-Zahlen inkonsistent (6 vs 7 printers; 20 vs 21 total)
• PR feat(api): Phase 6a — REST API (spec + plan) #64 spec — /api/print/... baseline falsch (existierende Routes auf /print, /jobs)
• PR feat(api): Phase 6a — REST API (spec + plan) #64 spec/plan — ProblemDetail modulpfad inkonsistent (errors.py vs schemas/problem.py)
• PR feat(api): Phase 6a — REST API (spec + plan) #64 spec — InvalidJobStateError referenced; nicht im code, Phase 5 raises ValueError
• PR feat(api): Phase 6a — REST API (spec + plan) #64 spec — /healthz tag inkonsistent (system vs meta)
• PR feat(api): Phase 6a — REST API (spec + plan) #64 spec — /jobs baseline inkorrekt
• PR docs(specs): Phase 5 — SQLModel persistence layer (spec + plan) #63 plan — referenced .gitlab-ci.yml (project uses GitHub Actions)
• PR docs(specs): Phase 5 — SQLModel persistence layer (spec + plan) #63 plan — poetry/uv (project uses hatchling/pip)
• PR docs(specs): Phase 5 — SQLModel persistence layer (spec + plan) #63 plan — LABEL_HUB_DATABASE_URL (correct: PRINTER_HUB_DATABASE_URL)
• PR docs(specs): Phase 5 — SQLModel persistence layer (spec + plan) #63 plan — module-level engine instantiation (override impossible)
• PR docs(specs): Phase 5 — SQLModel persistence layer (spec + plan) #63 spec — npm run typecheck (project: mypy app)
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 spec — task-count inconsistency (13 vs 15 vs 12)
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 spec — Traefik middleware sse-flush@docker suffix fehlt in der spec (plan ok)
• PR feat(integrations): plugin architecture for lookup clients (Phase 3.5) #55 — integrations/README.md:87 — guidance für 3rd-party plugins falsch (Settings.extra="ignore")
• PR feat(integrations): plugin architecture for lookup clients (Phase 3.5) #55 — integrations/README.md:25 — Beispiel settings.myapp_url ohne Hinweis dass bundled plugins die Field in config.py adden müssen
• PR feat(integrations): AppLookupService aggregator — Phase 3 complete #53 — lookup_service.py — error message computes sorted(self._clients) statt self.available_apps (already sorted)

Plan

1. Diese Woche: KRITISCH + WICHTIG Code-Bugs in 1-2 Fix-PRs angehen (PR feat(printer-models): PrintQueue worker with pause/resume/cancel/retry #50/feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 race + 409 + stop)
2. Diese Woche: MEDIUM Perf-Bugs (httpx pooling, font caching, SnmpEngine singleton)
3. Diese Woche: spec/plan DOCS-Korrekturen in einem PR (LABEL_HUB → PRINTER_HUB, npm → mypy, gitlab-ci → github-actions, poetry → hatchling)
4. Niedrige Prio: typ-cleanup (Any → Literal), test-hermetic, README badge token

Lessons Learned (process)

Neue Rule .claude/rules/review-feedback-policy.md:

• Pre-Merge-Checkliste mit ≥15 min Wartezeit nach Push für Bot-Reviews
• Klassifikation aller Bot-Reviewer
• Audit-Pflicht alle 4-6 Wochen
• Post-Merge-Reviews: Fix-PR Pflicht statt ignorieren

Refs #14, #18, #19, #22

[strausmann]

Update 2026-05-16 22:00 UTC — PR #66 gemerged (2026-05-16T21:58:32Z, squash-commit 76f57e2).

Adressiert durch PR #66 (15 Findings)

Aus initialer Audit-Liste (8 Findings, alle KRITISCH+WICHTIG)

• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — qr.py UUID
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — events.py raw HTML
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — events.py Settings wiring
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — status_probe _last + online flag
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — queue_depth
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — callback coverage (submit/pause/resume/cancel/clear)
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — subscriber counting (3× overcount)
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — events.py:208 events_published metric placement

Aus PR #66 Round-2 Bot-Reviews (7 Findings)

• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — submit() fake QUEUED→QUEUED state-change suppressed
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — empty SSE fragment skip
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — status_probe docstring/impl alignment
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — distinct_subscriber_count(channels=) public API
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — route uses public method (no more private _subscribers reach)
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — test_sse_flush cleanup deduplicated
• (already in audit) — covered

PR #68 (offen) wird adressieren

• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — submit-then-pause race (✅ fixed in fcc35f4)
• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — resume_printer 409 (✅ fixed in 225ddb9)
• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — resume_job 409 error_code (✅ fixed in 225ddb9)
• PR feat(printer-models): PrintQueue worker with pause/resume/cancel/retry #50 — stop() _done_event (✅ fixed in 9d6cb0d)
• PR fix(api): print-queue review-followups — race, 409 contracts, stop() cleanup #68 Gemini-review: Any return types in print.py (in progress, round-2)
• PR fix(api): print-queue review-followups — race, 409 contracts, stop() cleanup #68 Gemini+Copilot-review: error_msg sync in stop() (in progress)
• PR fix(api): print-queue review-followups — race, 409 contracts, stop() cleanup #68 Copilot-review: duplicate test (in progress)

Verbleibend nach PR #68 Merge

~18 Findings (MEDIUM Code-Quality + Spec/Plan-Docs). Nächste Fix-PR-Empfehlung: MEDIUM Perf-Bugs zusammen (httpx pooling PR #51, font caching PR #54, SnmpEngine singleton PR #59).

[strausmann]

Update 2026-05-16 22:21 UTC — PR #68 gemerged (squash-commit 650f823).

Total Findings adressed: 22 von 41

PR #66 (15 Findings)

Siehe vorherigen Kommentar.

PR #68 (7 Findings)

• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — print_service.py submit-then-pause race
• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — print.py resume_printer 409 already_active
• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — print.py resume_job 409 invalid_state error_code
• PR feat(printer-models): PrintQueue worker with pause/resume/cancel/retry #50 — print_queue.py stop() _done_event
• PR fix(api): print-queue review-followups — race, 409 contracts, stop() cleanup #68 bot-review — Any-types narrowed (Gemini)
• PR fix(api): print-queue review-followups — race, 409 contracts, stop() cleanup #68 bot-review — error_msg sync in stop() (Gemini + Copilot dup)
• PR fix(api): print-queue review-followups — race, 409 contracts, stop() cleanup #68 bot-review — duplicate test removed (Copilot)

~19 verbleibende Findings

Code-Bugs (MEDIUM, Perf)

• PR feat(integrations): LabelData schema + Snipe-IT lookup client #51 — httpx.AsyncClient per-call (no pooling)
• PR feat(label-renderer): Template schema + Pillow/qrcode renderer for 1-bit label bitmaps #54 — ImageFont.truetype() no caching
• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — SnmpEngine per-query (no MIB caching)
• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — TemplateLoader/ModelRegistry sync I/O in async lifespan
• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — _LifespanManager in main.py (490 lines, >400)
• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — Label-data resolution duplikat
• PR feat(label-renderer): Template schema + Pillow/qrcode renderer for 1-bit label bitmaps #54 — validator error message wording
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — events.py:152 initial snapshot
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — event_bus.py _dropped leak
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — config.py Field(gt=0) validation
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — pyproject.toml slow marker CI exclude
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — .env.example SSE vars
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 — tape_change_producer hardcoded PT-series
• PR feat(integrations): LabelData schema + Snipe-IT lookup client #51 — dict[str, Any] policy violation
• PR feat(integrations): LabelData schema + Snipe-IT lookup client #51 — kein Authorization-Header Test

Code-Quality (MEDIUM)

• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — README codecov badge token
• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — ptouch-integration.md Tape doc mismatch
• PR feat(api): First-Print pipeline — POST /print to PT-P750W (E2E verified) #59 — Settings(_env_file=None) in tests
• PR feat(integrations): AppLookupService aggregator — Phase 3 complete #53 — lookup_service Literal typing

DOCS (MEDIUM — Phase 5/6a specs)

• PR feat(api): Phase 6a — REST API (spec + plan) #64 spec — LABEL_HUB_WEBHOOK_API_KEY → PRINTER_HUB_
• PR feat(api): Phase 6a — REST API (spec + plan) #64 spec — endpoint count inconsistency
• PR feat(api): Phase 6a — REST API (spec + plan) #64 spec — /api/print baseline
• PR feat(api): Phase 6a — REST API (spec + plan) #64 spec — ProblemDetail module path
• PR feat(api): Phase 6a — REST API (spec + plan) #64 spec — InvalidJobStateError
• PR feat(api): Phase 6a — REST API (spec + plan) #64 spec — /healthz tag
• PR docs(specs): Phase 5 — SQLModel persistence layer (spec + plan) #63 plan — gitlab-ci → github actions
• PR docs(specs): Phase 5 — SQLModel persistence layer (spec + plan) #63 plan — poetry → hatchling
• PR docs(specs): Phase 5 — SQLModel persistence layer (spec + plan) #63 plan — LABEL_HUB_DATABASE_URL
• PR docs(specs): Phase 5 — SQLModel persistence layer (spec + plan) #63 plan — module-level engine
• PR docs(specs): Phase 5 — SQLModel persistence layer (spec + plan) #63 spec — npm run typecheck
• PR feat(api): Phase 6b — SSE EventBus (spec + plan) #65 spec — task count + Traefik suffix
• PR feat(integrations): plugin architecture for lookup clients (Phase 3.5) #55 — integrations/README guidance

Nächste PRs

1. fix/perf-pooling-caching — httpx pooling + font caching + SnmpEngine singleton (3 Commits)
2. fix/audit-cleanup-medium — Code-Quality + DOCS Sammel-PR
3. Phase 7a — Frontend-Basis (Spec liegt, Plan-Writer braucht Retry)