Skip to content

docs(plans): add First-Print implementation plan#58

Merged
strausmann merged 13 commits into
mainfrom
feat/first-print-plan
May 15, 2026
Merged

docs(plans): add First-Print implementation plan#58
strausmann merged 13 commits into
mainfrom
feat/first-print-plan

Conversation

@strausmann
Copy link
Copy Markdown
Owner

Summary

Adds the implementation plan for the First-Print pipeline. Docs-only PR, no production code yet.

Plan file: docs/plans/2026-05-15-first-print.md (~4540 lines, 17 phases, ~60 atomic tasks, TDD-strict).

This PR is a follow-up to the design PR #57. The design PR's HEAD is merged into this branch so reviewers see the latest design in context.

What this plan delivers when executed

  • PrinterBackend Protocol (print_image + query_status — no send_bytes)
  • PTouchBackend wrapping the ptouch library + MockPrinterBackend (both shipping under app/printer_backends/)
  • PrinterError hierarchy with HTTP mapping
  • ESC i S status query over raw asyncio socket (ptouch has no public status API)
  • SNMP helpers for what TCP/9100 cannot serve:
    • query_model_pjl — Brother private OID for model discovery (ADR 0004)
    • query_live_status — Host-Resources Printer MIB for during-print status
  • PTP750WDriver with a make_queue_printer(...) factory so subclassed drivers inherit the bridge to PrintQueue automatically
  • ModelRegistry and a new BackendRegistry, both populated via setuptools entry_points — third-party drivers/backends ship as separate pip packages with zero core changes
  • PrintService orchestrator (lookup or raw data → render → enqueue)
  • REST: POST /print (202 with job_id) + GET /jobs/{job_id} (incl. live SNMP block while job is PRINTING)
  • Lifespan: SNMP-first model discovery, printer_model fallback, fail-fast on inconsistent config
  • Hardware smoke script for a real PT-P750W (pytest --hardware opt-in)

Review focus

I'd appreciate feedback especially on:

  1. TDD granularity — are the failing-test/implement/run/commit steps the right size?
  2. Spec coverage — anything in docs/designs/2026-05-15-first-print.md not covered by a task?
  3. build_print_job resolution — Phase 8.1 returns empty bytes from PTP750WDriver.build_print_job for Protocol conformance; the design's open question (delegate to ptouch encoder vs RasterizablePrinterModel sub-protocol) is deferred to follow-up.
  4. SNMP failure-mode coverage — Phase 13.1 has three lifespan tests (resolve from PJL, fallback to setting, fail when no fallback). Are the boundaries right?

Execution

Subagent-Driven Development per task — implementer never pushes; orchestrator pushes after human code review.

Privacy

The plan describes a private LAN/Tailscale deployment as an example for "where SNMPv2c is sufficient". No specific IPs, hostnames, or tokens are referenced.

Refs #22

Design for the first end-to-end print pipeline targeting the
Brother PT-P750W. Introduces the PrinterBackend Protocol, the
PTouchBackend adapter wrapping the ptouch library, the PTP750W
driver/bridge combo, a PrintService orchestrator, and the
POST /print + GET /jobs/{id} REST surface.

Includes error hierarchy, HTTP status mapping, retry policy,
testing strategy (unit + integration via mock backend +
manual hardware smoke), and explicit acceptance criteria.

Marked as Draft pending review.

Refs #22
- Move PTP750WPrinter bridge to app/printer_models/pt.py per ADR 0004
- Rename `printer_id` field to `id` to match `_PrinterLike` Protocol in
  print_queue.py
- Use the real TapeRegistry.lookup_pt(width_mm, media_type) signature;
  introduce per-print `media_type` option with a configurable default
- PrintQueue is instantiated with `printers=[...]` (no max_concurrency
  argument); remove `printer_max_concurrency` from settings — the
  queue gives each printer its own dedicated worker by design
- Move MockPrinterBackend from tests/_fakes/ to app/printer_backends/
  so production lifespan never imports from tests; mock stays usable
  for local dev without hardware
- Clarify `send_bytes` uses asyncio.open_connection (non-blocking)

Refs #22
Make printer extension a day-one property of First-Print rather than
a phase-2 retrofit. Concretely:

- Driver discovery is plugin-based: ModelRegistry walks the
  `label_hub.printer_models` entry-points group on app start, the
  built-in PT-Series driver self-registers, third-party drivers
  ship as separate pip packages.
- Backend discovery mirrors the same pattern with a new
  BackendRegistry on the `label_hub.printer_backends` group; the
  built-in `ptouch` and `mock` backends self-register.
- Backends expose `from_settings(settings) → PrinterBackend` so the
  lifespan stays trivial and series-agnostic (no per-backend
  if-tree).
- The bridge to PrintQueue's `_PrinterLike` becomes
  `Driver.make_queue_printer(...)` — a factory method on the driver,
  not a separate exported class. Subclassed drivers inherit it for
  free, the PT-specific `_PTPQueuePrinter` adapter stays private to
  `pt.py`.
- Settings: `printer_model` (series-agnostic string, resolved
  against ModelRegistry) replaces the PT-only `printer_pt_model`
  literal. Both `printer_backend` and `printer_model` are plain
  strings now so third-party plugins can be selected without code
  change.

New "Extensibility" section walks through five concrete extension
paths from smallest to largest intervention: subclass-in-series,
decorator backend (for vendor-lib quirks), subclass driver
(model-specific status/encoding), new series with new backend, and
third-party pip package via entry_points.

Acceptance criteria + test plan extended for plugin-discovery and
unknown-plugin failure modes.

Lifecycle hooks (before_print/after_print) are deliberately out of
scope for First-Print (YAGNI) — paths 2 and 3 cover every realistic
case today.

Refs #22
Verified against the existing code; fixes for all eight findings:

- PrintQueue API: replace `enqueue(...)` with the real `submit(printer_id,
  image, tape_mm, **options) → job_id` signature throughout the data flow
  and acceptance criteria.
- Job state names align with the real `JobState` enum
  (queued / paused / printing / completed / failed / cancelled). Replaces
  the made-up `running` / `done` states in the data-flow, REST schema,
  integration tests, and acceptance criteria.
- TTL claim corrected: `_jobs` has no eviction today (queue source has a
  TODO about unbounded growth). Documented as planned for Phase 5 with
  the practical reasoning why First-Print can ship without it.
- `PrintOptions` shared-default-instance anti-pattern fixed:
  `Field(default_factory=PrintOptions)` plus `frozen=True` on the model.
- Raw-data payload schema is now explicit: a dedicated `RawLabelData`
  Pydantic model mirroring `LabelData`'s shape (title, primary_id,
  qr_payload, secondary as list-coerced-to-tuple), validated by the
  framework. `source_app` is set to "manual" by PrintService rather
  than accepted from the client.
- PTP750WDriver Protocol conformance: `build_print_job` is now described
  as delegating to the ptouch library's internal raster builder
  (exact entry point to confirm in plan Phase 1, fallback to a raw
  encoder per Brother Raster Command Reference). `query_status` raises
  on a non-matching host argument rather than silently ignoring it.
- `send_bytes` flow specifies the full lifecycle:
  write → `drain()` → `close()` → `wait_closed()` to avoid truncated
  raster streams under backpressure.
- Settings: keep the existing `pt750w_host`/`pt750w_port` naming.
  Add only `printer_backend`, `printer_model`, `printer_queue_timeout_s`.
  Backend `from_settings` knows which existing host field to read.

Refs #22
Four findings on the extensibility commit:

A — Removed `send_bytes` from `PrinterBackend` Protocol. No concrete
    caller exists in First-Print, the second TCP path would compete
    with the ptouch library for the printer's single TCP session
    (Brother PT hardware limit), and Customization Path 2 (decorator
    backend) does not need it. The hook can be added back additively
    if a real use case appears. Decorator-backend example in the
    Extensibility section trimmed to two methods.

B — Resource Busy on parallel TCP/9100 connections: resolved by
    removing `send_bytes`. A note about the single-session limit is
    now part of the YAGNI rationale.

C — `query_status(host=...)` Protocol mismatch: the current bound-
    backend driver still raises on a non-matching host (no behavioural
    change). The cleaner alternative — refactoring the Protocol so
    `query_status` takes no `host` — is documented in Open Questions
    as a follow-up PR that touches every PrinterModel call site, out
    of scope for First-Print.

D — `build_print_job` Protocol relaxation: kept the delegation-to-
    ptouch-encoder approach. The alternative — splitting rasterization
    into a `RasterizablePrinterModel` sub-protocol — is documented
    alongside C as a candidate follow-up. Either resolution will work,
    the decision depends on whether ptouch exposes a public encoder.

Refs #22
The print path uses TCP/9100 single-session (ptouch holds it during
a print). SNMP on UDP/161 gives us a parallel channel for the things
TCP cannot serve:

* Model auto-discovery (ADR 0004) — read Brother private OID
  1.3.6.1.4.1.2435.2.3.9.1.1.7.0 → PJL string → ModelRegistry.find_by_pjl.
* Live status during a running print — hrPrinterStatus and
  hrPrinterDetectedErrorState are reachable while ptouch holds the
  print socket.

ESC i S is retained for pre-print validation (it returns tape_mm and
media_type directly as integers/enums; SNMP would need string parsing
for the equivalent info).

Spec additions:

* New SNMP helper component in the component map and architecture
  diagram (sits beside the backend, not behind it).
* Two new async helpers: query_model_pjl() and query_live_status().
* New settings: printer_discover_via_snmp (default True),
  printer_snmp_community (default 'public'). printer_model becomes
  the fallback when SNMP fails or is disabled.
* Lifespan flow documented with the three valid configurations
  (auto+fallback, auto-only, manual).
* GET /jobs/{job_id} returns a `live` sub-object while the job is
  printing; null otherwise. SNMP failure at request time is
  non-fatal — the live block is just omitted.
* SnmpDiscoveryError and SnmpQueryError added to the exception
  hierarchy; SnmpDiscoveryError is the only one that can prevent
  app start.
* Four new acceptance criteria (12-14 + renumbered tail).

Refs #22
Comprehensive TDD-strict implementation plan tracking the
First-Print pipeline design (docs/designs/2026-05-15-first-print.md).

17 phases, ~60 atomic tasks, each with failing test first → minimal
implementation → green test → commit. Conventional Commits with the
new printer-backends scope (added in Phase 0). Every commit footer
ends with Refs #22.

Covers:

* Phase 0 — commitlint scope extension
* Phase 1 — ptouch entry-points doc, Brother ESC i S wire format,
  Brother SNMP OIDs (Brother private PJL OID + Host-Resources MIB)
* Phase 2 — PrinterError hierarchy (incl. SnmpDiscoveryError,
  SnmpQueryError)
* Phase 3 — PrinterBackend Protocol (print_image + query_status only,
  send_bytes deliberately omitted)
* Phase 4 — MockPrinterBackend in app/printer_backends/
* Phase 5 — BackendRegistry + entry_points discovery
* Phase 6 — ESC i S status_query helper, PTouchBackend, and the
  SNMP helper module (query_model_pjl + query_live_status + LiveStatus)
* Phase 7 — ModelRegistry.find_by_model_id + entry_points discovery
* Phase 8 — PTP750WDriver + make_queue_printer + _PTPQueuePrinter
* Phase 9 — Pydantic schemas (PrintRequest, RawLabelData, PrintOptions,
  PrintJobResponse, PrintJobStatusResponse incl. optional `live` block)
* Phase 10 — PrintService orchestrator
* Phase 11 — REST routes incl. live SNMP block while job is PRINTING
* Phase 12 — Settings (printer_backend, printer_model,
  printer_discover_via_snmp, printer_snmp_community,
  printer_queue_timeout_s)
* Phase 13 — Lifespan with SNMP-first model discovery and
  printer_model fallback
* Phase 14 — Integration tests (happy path, tape mismatch, offline,
  SNMP discovery)
* Phase 15 — Hardware smoke script + gated hardware test
* Phase 16 — Final verification + handoff (no auto-push)

Implementer subagents do NOT push — orchestrator pushes after
human code review.

Refs #22
@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 the implementation plan for the First-Print pipeline, which enables end-to-end label printing on Brother PT-P750W hardware. The documentation covers the proposed architecture, data flow, REST API schemas, and a TDD-focused execution strategy, laying the groundwork for future development without modifying production code.

Highlights

  • Documentation: Added a comprehensive implementation plan for the First-Print pipeline, detailing 17 phases and 60 atomic tasks with a strict TDD approach.
  • Architecture: Outlined the new printer backend protocol, SNMP helpers for model discovery and live status, and a plugin-based registry architecture for future extensibility.
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.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

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 the design for the First-Print Pipeline, establishing an end-to-end workflow for printing on Brother PT-P750W hardware using a plugin-based architecture and SNMP for discovery and live status. Feedback focuses on ensuring strict protocol conformance for method signatures, preventing silent failures by raising explicit errors for unimplemented methods, and mitigating memory risks in the in-memory job store. Additionally, recommendations were made to optimize API responsiveness by caching live status and to resolve inconsistencies between the retry policy and acceptance criteria.

self._backend = backend

# --- PrinterModel methods ---
async def query_status(self, host: str = "", port: int = 9100, timeout_s: float = 5.0):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The query_status implementation in PTP750WDriver makes the host argument optional (host: str = ""), whereas the PrinterModel Protocol (defined in app/printer_backends/base.py) requires it. While the design notes this as a "dead" argument to be refactored later, the signature mismatch should be avoided to maintain strict Protocol conformance and avoid potential issues with static analysis or positional calls.

Suggested change
async def query_status(self, host: str = "", port: int = 9100, timeout_s: float = 5.0):
async def query_status(self, host: str, port: int = 9100, timeout_s: float = 5.0):

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in 2d2ca27. The signature now matches the Protocol: async def query_status(self, host: str, port: int = 9100, timeout_s: float = 5.0). The empty-string default is gone. Empty host is still accepted as a 'use bound backend's host' shorthand (no raise); a non-matching non-empty host still raises ValueError. The unit test that called it without args now passes host="" explicitly.

Comment on lines +169 to +189
def build_print_job(self, image, tape_spec, auto_cut=True, high_resolution=False) -> bytes:
"""Encode an image into the Brother raster byte stream for PT-Series.

First-Print happy path goes through `backend.print_image()` and does
**not** call this method. It is kept here for callers that need raw
bytes (export-to-file, debugging) and for Protocol conformance.

Implementation: delegates to the ptouch library's internal raster
builder (e.g. `ptouch.label.ImageLabel(image, ...).encode()`). The
exact entry point will be confirmed in implementation Phase 1 — if
ptouch does not expose a public encoder, the fallback is a raw
encoder built from the Brother Raster Command Reference.

Alternative considered: refactoring `PrinterModel` so rasterization
is in a separate sub-protocol (`RasterizablePrinterModel`). That is
a cleaner shape if a backend genuinely cannot expose an encoder
and is flagged as a possible follow-up. For First-Print we keep the
Protocol unchanged and assume the ptouch encoder is reachable.
"""
# impl details deferred to plan
...
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Implementing build_print_job as a no-op (returning empty bytes) violates the PrinterModel Protocol's contract, which expects a valid raster byte stream. Even if the primary path uses backend.print_image, other consumers (such as the mentioned debugging or export-to-file features) will receive invalid data. If the implementation is deferred, it is safer to raise NotImplementedError than to return empty bytes to avoid silent failures.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Agreed and fixed in 2d2ca27build_print_job now raises NotImplementedError with a message explaining the deferral. Added a unit test asserting the raise. The pyproject coverage config already excludes raise NotImplementedError so coverage isn't affected.

Comment on lines +257 to +260
In-memory store, no eviction (the current `PrintQueue._jobs` dict keeps every job for the process lifetime — there is a TODO in the queue source about unbounded growth). A bounded job store with TTL or LRU eviction is **planned for Phase 5** alongside SQLite persistence; not part of First-Print.

For First-Print the practical impact is acceptable: jobs are tiny (a few KB each — PNG payload + metadata), the maintainer's deployment prints maybe dozens of labels per day, and a periodic process restart (e.g. via Watchtower image updates) acts as a coarse eviction in practice. If memory becomes an issue before Phase 5, the simplest stop-gap is a startup-time cap on the dict size with FIFO eviction — kept out of scope here to avoid scope creep.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Relying on process restarts for job eviction is a risk for long-running services. Since PrintQueue._jobs stores image payloads, memory usage will grow linearly with print volume. A simple TTL or a maximum size for the in-memory store should be considered for the MVP to prevent potential OOM issues before the Phase 5 persistence work is completed.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Valid concern. Memory growth is acknowledged in the Persistence section of the design — the current FIFO/restart-as-eviction is acceptable for First-Print (small payloads + low daily volume + Watchtower restarts), but you're right that a bounded job store is the proper fix. It stays a Phase 5 deliverable alongside SQLite persistence. If pre-Phase-5 OOM ever becomes real, a 10-line collections.deque(maxlen=N) drop-in is the planned mitigation. I'll open a follow-up issue once #58 lands so it doesn't get lost.

Comment thread docs/designs/2026-05-15-first-print.md Outdated
BackendRegistry.ensure_discovered()

backend = _build_backend(settings)
driver_cls = ModelRegistry.find_by_model_id(settings.printer_model)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The lifespan snippet directly resolves the driver class using settings.printer_model, which bypasses the SNMP discovery logic described in Phase 13. It should instead call _resolve_printer_model (defined at line 521) to correctly handle the printer_discover_via_snmp setting and the PJL-based resolution.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Good catch — that was a leftover snippet in the spec (PR #57), not the plan. The plan's Phase 13 lifespan does route through _resolve_model_id correctly. Fixed in the spec at commit 160aea5 — the snippet now calls _resolve_printer_model before find_by_model_id and wires app.state.printer_host + app.state.printer_snmp_community for the route handler's live-SNMP path.


### Live-status path

`GET /jobs/{job_id}` keeps reading the in-memory FSM (Job.state, error_code, ...) as today. **Additionally**: when `job.state == JobState.PRINTING`, the route handler calls `query_live_status(host)` and attaches the returned `LiveStatus` to the response as a sub-object. Failure of the SNMP query is non-fatal — the live block is just omitted.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Performing an SNMP query (with a 3s timeout) during a GET /jobs/{id} request may significantly degrade API responsiveness, especially if the printer is offline. Consider caching the live status in the Job object within the worker loop or using a much shorter timeout for this polling endpoint to ensure the UI remains snappy.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in 2d2ca27 — the route handler now passes timeout_s=1.0 (down from the helper's default 3.0s) so the request path stays snappy when SNMP is slow or unreachable. Failure remains non-fatal: SnmpQueryErrorlive = None and the response is still 200. A proper cache (worker writes, route reads) would be even better and is the right thing to do once the worker FSM gains a hook for periodic SNMP polling; that is the same place where bounded job eviction (your finding 3) will land. I'll cover both in the Phase-5 persistence work.


### Retry policy

- `query_status` retries 3 times with back-off (0s, 1s, 2s) on `socket.timeout` / `OSError`.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

There is a discrepancy between the retry policy (3 retries = 4 attempts) and Acceptance Criterion 5 (exactly 3 attempts). The retry logic or the AC should be aligned to avoid confusion during implementation and testing.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Aligned in 2d2ca27. The implementation does exactly 3 attempts (initial + 2 retries; back-off sleeps of 0s, 1s, 2s between them — see _RETRY_BACKOFFS = (0.0, 1.0, 2.0)). The wording 'retries 3 times' was the confusing part; the plan + acceptance criteria now say 'exactly 3 attempts'.

The CI privacy-scan correctly flagged the placeholder host
"10.0.0.5" used throughout the plan's test code as an RFC 1918
private address. Replaced with 192.0.2.10 (RFC 5737 TEST-NET-1),
which is reserved for documentation and not flagged by the scan.

No semantic change — purely a placeholder substitution in 17 spots
of example test code.

Refs #22
Six findings, all valid or sensible clarifications:

* query_status(host: str, ...) drops the empty-string default to match
  the PrinterModel Protocol signature exactly. Empty string is still
  accepted (means "use the bound backend's host"); a non-matching
  non-empty host still raises ValueError. The unit test that called
  query_status() without arguments now passes host="" explicitly.
* build_print_job raises NotImplementedError instead of returning
  empty bytes — unintended callers now fail loudly. Added a unit
  test asserting the raise. Coverage config already excludes
  `raise NotImplementedError`.
* Live-status SNMP call from GET /jobs/{id} now passes timeout_s=1.0
  (was the default 3.0s) — keeps the request path snappy even when
  SNMP is slow or unreachable; failure stays non-fatal and the live
  block is omitted.
* Retry-policy wording aligned with Acceptance Criterion 5: "exactly
  3 attempts" (was "3 retries", which Gemini read as 4 attempts).
  The implementation does 3 attempts with back-off sleeps 0s, 1s, 2s
  between them — semantic unchanged.

Memory growth of PrintQueue._jobs is acknowledged in the Persistence
section already; bounded eviction stays a Phase-5 deliverable for
First-Print. The other Gemini concern (lifespan bypassing
_resolve_printer_model in the Spec) lives in #57 and is fixed there.

Refs #22
Earlier draft of the lifespan snippet still did
ModelRegistry.find_by_model_id(settings.printer_model) directly,
bypassing the SNMP-discovery flow introduced in the SNMP-hybrid
section. Fixed: the snippet now picks model_id from
_resolve_printer_model (SNMP first, fall back to setting) before
the registry lookup. Also wires app.state.printer_host and
app.state.printer_snmp_community so the route handler can do live
SNMP queries.

Refs #22
…ter_id

Spec-vs-Plan drift fix: PrintService takes lookup_service +
printer_id (Plan Phase 10), not integration_registry. The lifespan
snippet in the spec is the authoritative example for the constructor
call.

Refs #22
Five Critical + several Important findings from a fresh-eyes review.
The plan was producing tests against StatusBlock / Job / TemplateLoader
APIs that don't exist in the real codebase. Add a Phase 1.5 — Domain
Model Extensions — before any backend code is written:

* Task 1.5.1 — StatusBlock @Property tape_empty / cover_open /
  loaded_tape_mm (derived from the real errors IntFlag and
  media_width_mm fields). Plus tests/_helpers/status.make_status_block
  so subsequent tests don't spell out all 18 dataclass fields.
* Task 1.5.2 — TemplateNotFoundError in template_loader (subclasses
  KeyError; the route maps it to HTTP 404 per Acceptance #6).
* Task 1.5.3 — LookupFailedError umbrella + UnknownAppError as a
  subclass; AppLookupService.lookup wraps any plugin runtime
  exception. Route maps to HTTP 502 per Acceptance #7.
* Task 1.5.4 — Job carries error_code / error_message / error_detail
  (Acceptance #4 needs structured detail like {expected_mm, loaded_mm}).
* Task 1.5.5 — PrintQueue worker catches PrinterError subclasses,
  populates the three new Job fields, then transitions to FAILED.
  TapeMismatchError gets a typed detail dict.
* Task 1.5.6 — @runtime_checkable on _PrinterLike Protocol so the
  Phase 8 isinstance() check works.

Additional fixes:

* Phase 6.1 parse_status_reply delegates to the existing
  StatusBlockParser.parse instead of inventing a partial parser. The
  legacy _MEDIA_TYPE_LOOKUP table is removed.
* Phase 8 commit body no longer claims "returns empty bytes" —
  build_print_job raises NotImplementedError, which the code already
  does. Stale doc text removed.
* PTouchBackend._ptouch_print is now model-aware via _PTOUCH_PRINTER_
  CLASSES[model_id] lookup. Previously hardcoded PTP750W defeated
  Extensibility Path 1 (PT-P900, PT-E550W).
* PrintService no longer forwards copies to queue.submit. Multi-copy
  delivery is a Phase-5 follow-up; clients can post N times today.
  Test assertion updated to "copies not in kwargs"; commit body of
  the PrintService task corrected.
* app/main.py snippet has `import logging` (was missing while
  _log = logging.getLogger(__name__) was already present).
* app/api/routes/print.py snippet now has one consolidated import
  block — the previous "Required imports for the route module"
  trailing block is folded in to prevent paste mistakes.
* Convention note added at the top of Phase 2 spelling out that
  every later test uses make_status_block(...) instead of
  StatusBlock(...). The implementer rewrites the six existing
  StatusBlock(...) call sites to make_status_block(...) accordingly.

Refs #22
strausmann added a commit that referenced this pull request May 15, 2026
Comprehensive design for the First-Print pipeline:
- PrinterBackend Protocol (print_image + query_status; no send_bytes)
- PTouchBackend + MockPrinterBackend in app/printer_backends/
- PTP750WDriver with make_queue_printer(...) factory in printer_models/pt.py
- ModelRegistry + new BackendRegistry both driven by setuptools entry_points
- PrintService orchestrator, POST /print + GET /jobs/{id} REST surface
- SNMP hybrid: Brother private OID for model discovery + Host-Resources MIB for live status during print
- App lifespan: SNMP-first discovery with printer_model fallback
- Extensibility paths: same-series add-on, decorator backend, subclass driver, new series + backend, third-party pip package
- Error hierarchy with HTTP status mapping
- Testing strategy: pure unit + mock-backend integration + manual hardware smoke

Went through 4 review iterations (Gemini + Copilot + superpowers code review):
- 5 + 8 + 4 + 6 + 5 inline findings, all addressed or explicitly deferred
- Spec-vs-Plan drift fixes (PrintService constructor)
- YAGNI cut on send_bytes
- Plan-Branch (#58) builds on this and stays in lockstep

Refs #22
@strausmann strausmann marked this pull request as ready for review May 15, 2026 18:21
Copilot AI review requested due to automatic review settings May 15, 2026 18:21
@strausmann strausmann merged commit e7c93de into main May 15, 2026
12 checks passed
@strausmann strausmann deleted the feat/first-print-plan branch May 15, 2026 22:02
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.

1 participant