feat: submit a published drawing version to the printer (D08) - #863

Review of PR 863 (D08: print a published drawing version)

Verdict: changes requested — one validation-order defect in the exact place the PR claims safety; everything else checks out.

What checked out

• The submission is safe by construction: direct argv, no shell; the PDF travels on stdin so no temporary file exists; stdin is dropped before the wait (no pipe deadlock); a missing or rejecting lp is BACKEND_UNAVAILABLE, never a panic; the non-unix backend refuses honestly; the Windows follow-up is documented.
• The pipeline is honest: 1:1 centred layout that refuses instead of rescaling (a rescaled print would misstate every drawn size), deterministic page bytes bound to the version, translation-invariant placement pinned by a test, margin and unresolved-projection refusals.
• The CLI mirrors export-drawing exactly: one exact published version reopened from its archived source, the live model never read; the sheet document refuses a contradictory --sheet; --dry-run reports the exact invocation and byte count and the invocation shape is validated before either path, so a dry run and a submission refuse identically.
• Tests pin the invocation shape, the name bounds and the stripping, the rejecting-queue refusal, dry-run exactness and replay, and the honest refusals; the receipt is exact (commit 7c06e2c, tree 6e2518fb verified against the commit object, base 7c21d02 the current master tip and parent, 2711/2711).
• The delivery bookkeeping is right: #472 sits open and closes here with its printer half actually delivered (the PDF half shipped in #729), and the epic #464 closes with all eight drawing cards referenced. This also resolves the #472 tension the #862 review recorded.

The finding

The dash check in lp_arguments runs on the raw queue name before bounded_text strips ASCII control characters. A printer argument like \u{1}-x passes the starts_with check and the stripped -x reaches the lp argv as the -d value — the validated shape and the submitted string differ, contradicting the documented invariant that a queue name never starts with a dash. There is no injection (argv form, getopt consumes the value), but the fix is one line: strip and bound first, then check the stripped name for the leading dash (and keep the empty-refusal), so the checked string is exactly the submitted one.

Review, round 2 — approved

The round-1 finding is fixed exactly as requested on head 950c1e4: lp_arguments now strips control characters and bounds the queue name first, then checks the stripped name for the leading dash, and pushes that same validated string as the -d value — the validated name and the submitted name are one string again, the doc comment states the new order, and the refusal test list gained the control-prefixed regression case.

The receipt is exact for the new head: commit 950c1e4 (the head per the pull request), tree 49f1c48b (verified against the commit object), base 7c21d02 (the current master tip, still the parent), full scope, 2711/2711, source_unchanged, all six stages green.

Verdict: approved. No inline findings. Everything else stands from round 1: the argv-only submission with stdin bytes and no temporary file, the honest 1:1 layout refusals, the CLI mirroring export-drawing with the validated dry-run shape, the exact replay, the honest refusals, and the bookkeeping — #472 closes here with its printer half delivered and the epic #464 closes with all eight drawing cards referenced.

Review of PR 863 (D08: submit a published drawing version to the printer)

Verdict: changes requested. The code is sound; the blockers are the delivery bookkeeping and the CI state of the current head.

What checks out

• lp_arguments strips control characters and bounds the queue name first, then checks the stripped name for a leading -, so the validated string is the submitted one. The invocation is argv-only, with no shell.
• The PDF travels on stdin, so no temporary file exists. A missing or rejecting lp is BACKEND_UNAVAILABLE, never a panic.
• print-drawing mirrors export-drawing: one exact published version reopened from its archived source, the live model never read, and a dry run and a real submission refuse on the same shape.

Blocking

1. CI has no result for the current head. The latest CI run on 950c1e42 (run 42671384272) ended as a failure with no jobs ("likely failed because of a workflow file issue"), and mergeStateStatus is BLOCKED. The earlier green run belongs to a previous head. Please re-run CI and make sure the required checks pass on 950c1e42.
2. Closes #472 is not backed by the issue's own acceptance. The delivery note on #472 (post-#729 audit) says the remaining work is "native printer submission/spooling and its dialog/integration acceptance". This PR delivers a CUPS path only (macOS and Linux). On Windows, submit returns BACKEND_UNAVAILABLE, and there is no print dialog. BeamCraft targets Windows, Linux and macOS desktops. Either:
   • change Closes #472 to Refs #472, post the delivered and remaining split on the issue, and open a follow-up for the Windows backend and the dialog; or
   • get an explicit owner decision that #472 closes with these limits, and record it in the PR and in docs/plan/decisions.md.
3. Closes #464 (the M4 epic) closes it while #333 stays open. The description says #333 tracks the remaining native drawing UI. Closing the epic here contradicts that. Use Refs #464, or have the owner confirm the closure explicitly.

Non-blocking

• The print-drawing branch in drawing_cmd.rs repeats most of the export-drawing resolution (version lookup, projection::resolve, per-view, section and detail resolution, sheet check). A shared helper would keep the two commands from drifting apart.
• In print::submit, if lp exits before reading stdin, write_all fails and the function returns without waiting for the child. Please wait() (or kill and wait) on that path so no zombie remains.
• The tests cover the dry run and the refusals, but no test covers a real submission on the success path (for example with a stub lp on PATH). Without one, the request-line handling is unverified.

Review, round 3

The new head commit adds real content beyond the approved round: the handover failure now kills and waits the print service (no zombie), export-drawing and print-drawing share one resolve_published helper (same refusals, same JSON shape), and a stub print service drives a real submission end to end, asserting the plain-argv delivery and the exact page bytes. The refactor is verified equivalent: the shared helper keeps the NotFound refusal, the sheet gating and every output field identical, and the 2026-11-10 decisions entry records the owner boundary honestly. All good.

Verdict: changes requested — two items, both about the merge consequences of this head:

1. The body still carries Closes #464, and its epic paragraph still says the epic closes with this change — but the head commit itself records the 2026-11-10 owner decision that the epic #464 stays open while the broader-drawing follow-up #333 is tracked outside the M4 milestone. On merge, GitHub will auto-close #464 through the body reference, directly contradicting the decision recorded in the same pull request. Remove the Closes #464 line (keep Closes #472) and update the epic paragraph to the delivered boundary.

2. The body receipt is stale: it records commit 950c1e4 / tree 49f1c48b / base 7c21d02, while this head is 7e8daf0 with tree 69bdfd04 (verified against the commit object) on the new master tip a95d6b7. Re-record the gate on this head, publish the report, and restore the receipt block naming the new commit, tree and base.

The code content itself is ready: with those two body fixes, the D08 delivery (with the CUPS-only boundary and the documented Windows and native-dialog follow-ups) is complete.

Review, round 4 — approved

Both round-3 findings are fixed on the same head 7e8daf0, exactly as requested:

• The body now says Refs #464, not Closes, and its paragraph states the boundary outright: the epic stays open while #333 tracks the remaining native drawing UI outside the milestone — so the merge no longer auto-closes the epic against the 2026-11-10 owner decision recorded in the same push. The body also gained the delivery-limits section naming that decision and the documented Windows and native-dialog follow-ups.
• The receipt is refreshed and exact: commit 7e8daf0, tree 69bdfd04 (verified against the commit object), base a95d6b7 (the current master tip), full scope, 2714/2714 (the increment is the stub-service submission test), source_unchanged, all six stages green.

Verdict: approved. No inline findings. The delivery stands as verified in round 3: the CUPS submission with the strip-first validated names, the zombie-free handover, the shared published-version resolution, the stub-service end-to-end test, and the honest limits — with #472 closing through its reference and the epic honestly kept open.