diff --git a/.planning/phases/06-wire-icon-intunewin/06-RESEARCH.md b/.planning/phases/06-wire-icon-intunewin/06-RESEARCH.md new file mode 100644 index 0000000..ca73ba8 --- /dev/null +++ b/.planning/phases/06-wire-icon-intunewin/06-RESEARCH.md @@ -0,0 +1,356 @@ +# Phase 6: Wire Icon into .intunewin Export — Research + +**Researched:** 2026-04-10 +**Domain:** Python file I/O, Peewee ORM query, .intunewin staging pipeline +**Confidence:** HIGH + +--- + + +## Phase Requirements + +| ID | Description | Research Support | +|----|-------------|-----------------| +| PKG-04 | User can upload a custom PNG icon for Intune app display (256x256, max 750KB) | Icon upload (icons.py + Icon model) is already complete. This phase wires the stored icon into the .intunewin build by copying it into the tmpdir before `build_intunewin()` is called. | + + +--- + +## Summary + +Phase 5 implemented icon upload and storage (`icons.py`, `Icon` model, SHA256-addressed +file under `DATA_DIR/icons/`). The `get_intunewin_package()` endpoint in `packages.py` +already builds a `.intunewin` file from a `TemporaryDirectory` staging area. The gap is +that the endpoint never queries the `Icon` model, so the PNG never lands in the tmpdir and +is never encrypted into the inner ZIP. + +The work is minimal and mechanical: one `Icon.get_or_none()` lookup, one `shutil.copy2()` +(or equivalent `open/write`) into `tmpdir`, and an integration test that verifies the icon +is present inside the exported package. No schema changes, no new dependencies. + +**Primary recommendation:** Query `Icon.get_or_none(Icon.printer == printer_id)` in +`get_intunewin_package()`, and if an icon exists copy it into `tmpdir` as `icon.png` +before calling `build_intunewin()`. Add one integration test that uploads an icon and +verifies `icon.png` appears inside the `.intunewin` inner ZIP. + +--- + +## Standard Stack + +### Core (already in place — no new installs) + +| Library | Version | Purpose | Why Standard | +|---------|---------|---------|--------------| +| peewee | 3.x | ORM query for Icon record | Already used throughout; `Icon.get_or_none()` follows established pattern | +| Python shutil / pathlib | stdlib | Copy icon file from ICONS_DIR into tmpdir | Zero-dep, already used in codebase | +| Pillow | current | Not needed here (already validated on upload) | Upload already guarantees PNG 256x256 | +| zipfile | stdlib | Read inner ZIP of .intunewin to assert icon presence in tests | Already used in test_packages.py | + +**Installation:** None required. All dependencies exist. + +--- + +## Architecture Patterns + +### Existing tmpdir staging pattern (packages.py lines 132-149) + +```python +with tempfile.TemporaryDirectory(prefix="imptune_") as tmpdir: + # Write scripts + with open(os.path.join(tmpdir, "install.ps1"), "w", encoding="utf-8") as f: + f.write(install_script) + # ... uninstall.ps1, detect.ps1 ... + + # Extract driver ZIP into tmpdir/drivers/ + drivers_subdir = os.path.join(tmpdir, "drivers") + os.makedirs(drivers_subdir, exist_ok=True) + with zipfile.ZipFile(driver_zip_path, "r") as driver_zf: + driver_zf.extractall(drivers_subdir) + + # Build .intunewin + build_intunewin(tmpdir, "install.ps1", output_path) +``` + +The icon copy slots in immediately after driver extraction, before `build_intunewin()`. + +### Pattern: icon lookup + conditional copy + +```python +import shutil +from imptune.db.models import Icon + +# Inside get_intunewin_package(), after driver validation: +icon_record = Icon.get_or_none(Icon.printer == printer_id) +if icon_record is not None: + icon_src = os.path.join(cfg.ICONS_DIR, icon_record.sha256) + icon_dst = os.path.join(tmpdir, "icon.png") + shutil.copy2(icon_src, icon_dst) +``` + +**Key decision:** Copy is conditional — printers without an uploaded icon still export +successfully. `shutil.copy2` preserves metadata and is the idiomatic stdlib copy call. + +### Icon filename in tmpdir + +Use `"icon.png"` as the fixed destination name regardless of `original_filename`. This is +predictable for the test assertion and matches Intune's expectation for app icons (a +well-known filename in the package root). + +### Established ORM access pattern (from project decisions) + +- Use `Model.get_or_none()` (not `Model.get()`) — avoids `DoesNotExist` exception +- Test assertions use `list(Model.select().where(...))` not `Model.get()` — avoids Peewee + cursor-caching issue with re-init'd DBs across tests (Phase 03 decision) + +### Anti-Patterns to Avoid + +- **Raising 422 when icon is absent:** Icon is optional. The export must succeed without one. +- **Storing icon in a subdirectory:** `build_intunewin` walks `source_dir` recursively; any + placement works, but root-level `icon.png` is simplest and most predictable. +- **Re-validating the PNG in packages.py:** Validation already happened at upload time. Do + not import Pillow into packages.py; just copy the bytes. + +--- + +## Don't Hand-Roll + +| Problem | Don't Build | Use Instead | Why | +|---------|-------------|-------------|-----| +| File copy into tmpdir | Custom open/read/write loop | `shutil.copy2()` | Handles edge cases, one line | +| Icon existence check | Filesystem path probe | `Icon.get_or_none()` | Source of truth is the DB record, not filesystem | +| Verify icon in .intunewin | Manual ZIP decryption | Read inner ZIP via `zipfile` after decoding (see test pattern below) | The test can open the outer ZIP and then the inner ZIP without decryption using stored bytes | + +--- + +## Common Pitfalls + +### Pitfall 1: ICONS_DIR not `cfg.ICONS_DIR` + +**What goes wrong:** Hardcoding `DATA_DIR + "/icons"` instead of reading `cfg.ICONS_DIR`. +**Why it happens:** `cfg.ICONS_DIR` was added in Phase 5 (config.py line 13); easy to miss. +**How to avoid:** Always use `cfg.ICONS_DIR` — monkeypatched by `tmp_data_dir` fixture in tests. +**Warning signs:** Integration test writes icon to `cfg.ICONS_DIR` but packages.py reads +from a hardcoded path — icon silently missing in package. + +### Pitfall 2: Icon copy placed after `build_intunewin()` call + +**What goes wrong:** Icon is written to tmpdir but after `build_intunewin()` already ran; +not included in the inner ZIP. +**How to avoid:** Icon copy must come BEFORE `build_intunewin(tmpdir, ...)` call. + +### Pitfall 3: Test verifying icon in outer ZIP instead of inner ZIP + +**What goes wrong:** The `.intunewin` outer ZIP contains only `IntuneWinPackage/Contents/ +IntunePackage.intunewin` (encrypted blob) and `Detection.xml`. `icon.png` is NOT a member +of the outer ZIP; it is packed into the inner ZIP (the plaintext content ZIP). A test that +opens the outer ZIP and checks `namelist()` for `icon.png` will always fail. +**How to avoid:** The integration test must decrypt (or use a pre-decryption approach — see +Code Examples below) to verify icon presence. The simplest approach: use a test fixture +that calls `build_intunewin` directly against a tmpdir so the inner ZIP can be inspected +before encryption, OR read the Detection.xml and re-derive key material from a +test-seeded build. Given current architecture uses random keys, the cleanest test strategy +is: + a) Make a real HTTP request through the test client to get the `.intunewin` bytes + b) Use `build_intunewin`'s known structure: the inner ZIP is the AES-encrypted payload + — cannot open it directly without the key. + c) **Best approach:** Test that the icon file is present in `tmpdir` staging BEFORE + `build_intunewin()` is called by exposing a helper, OR introduce a + `build_intunewin` that accepts a pre-built inner ZIP for testability. + d) **Pragmatic approach that avoids refactoring:** In the integration test, directly + call the packages.py function with a monkeypatched `build_intunewin` that records + what was in `tmpdir` instead of encrypting. This is the pattern to use. + +**Recommended test strategy:** Monkeypatch `build_intunewin` to capture the staging +directory contents, then assert `icon.png` is present among the staged files. This is +simpler than decrypting the output and avoids coupling the test to the encryption +implementation. + +### Pitfall 4: `printer_id` vs ORM instance in Icon query + +**What goes wrong:** `Icon.get_or_none(Icon.printer == printer)` (ORM instance) vs +`Icon.get_or_none(Icon.printer == printer_id)` (integer). Both work in Peewee, but the +integer form is more explicit and consistent with how icons.py deletes records +(`Icon.delete().where(Icon.printer == printer_id)`). +**How to avoid:** Use integer `printer_id` (the local variable from `_get_printer_and_driver` +returns the printer ORM object; extract `.id` from it or use it directly — Peewee resolves +FK equality either way, but be consistent with existing codebase style). + +--- + +## Code Examples + +### Icon lookup and conditional copy (insert inside `get_intunewin_package`) + +```python +# Source: derived from icons.py pattern + packages.py TemporaryDirectory pattern +import shutil +from imptune.db.models import Icon + +# After extracting driver ZIP, before build_intunewin(): +icon_record = Icon.get_or_none(Icon.printer == printer.id) +if icon_record is not None: + icon_src = os.path.join(cfg.ICONS_DIR, icon_record.sha256) + if os.path.isfile(icon_src): + shutil.copy2(icon_src, os.path.join(tmpdir, "icon.png")) +``` + +### Integration test — icon presence in staged files (monkeypatch approach) + +```python +# Source: pattern established by test_packages.py + test_icon_upload.py + +def test_intunewin_includes_icon(client, setup_printer_with_driver, tmp_data_dir, monkeypatch): + """Icon PNG is included in .intunewin staging directory.""" + import io + import os + from PIL import Image + from imptune.db.models import Icon + import imptune.config as cfg + import imptune.api.packages as pkg_module + + printer, _ = setup_printer_with_driver + + # Create and upload icon + img = Image.new("RGBA", (256, 256), color="red") + buf = io.BytesIO() + img.save(buf, format="PNG") + png_data = buf.getvalue() + + resp = client.post( + f"/printers/{printer.id}/icon", + files={"file": ("icon.png", io.BytesIO(png_data), "image/png")}, + ) + assert resp.status_code == 200 + + # Capture staging contents via monkeypatched build_intunewin + staged_files = [] + + def fake_build(source_dir, setup_file, output_path): + staged_files.extend(os.listdir(source_dir)) + # Write a minimal valid file so endpoint can read it + with open(output_path, "wb") as f: + f.write(b"FAKE") + + monkeypatch.setattr(pkg_module, "build_intunewin", fake_build) + + resp = client.get(f"/printers/{printer.id}/packages/intunewin") + assert resp.status_code == 200 + assert "icon.png" in staged_files +``` + +### Test: export succeeds with no icon (icon-free baseline) + +```python +def test_intunewin_without_icon_succeeds(client, setup_printer_with_driver, monkeypatch): + """Export succeeds even when no icon has been uploaded.""" + import imptune.api.packages as pkg_module + import os + + printer, _ = setup_printer_with_driver + + def fake_build(source_dir, setup_file, output_path): + with open(output_path, "wb") as f: + f.write(b"FAKE") + + monkeypatch.setattr(pkg_module, "build_intunewin", fake_build) + + resp = client.get(f"/printers/{printer.id}/packages/intunewin") + assert resp.status_code == 200 +``` + +--- + +## State of the Art + +| Old Approach | Current Approach | When Changed | Impact | +|--------------|------------------|--------------|--------| +| icon stored as column in Printer | Separate `Icon` model with FK, SHA256-addressed on disk | Phase 5 | Clean separation; icon is optional; no schema migration needed here | +| build_intunewin reads arbitrary files | build_intunewin walks entire tmpdir | Phase 5 | Any file placed in tmpdir automatically included in inner ZIP | + +--- + +## Open Questions + +1. **Should missing icon file on disk (orphaned DB record) be silently skipped or 422?** + - What we know: `icon_record` can exist in DB but file might be absent (disk corruption, + manual cleanup). Current `icons.py` does not handle this case either. + - Recommendation: Silent skip (don't copy if `os.path.isfile(icon_src)` is False) — + consistent with the optional nature of icons and avoids breaking exports for stale records. + +2. **Is there any Intune-specific requirement for the icon filename or location inside .intunewin?** + - What we know: The `.intunewin` package is a deployment container; Intune does NOT + read the icon from within the .intunewin file. The icon is set separately in the + Intune portal during app creation. The PKG-04 requirement says "included in the + .intunewin package" as a convenience/archive — not as an Intune-interpreted artifact. + - Confidence: MEDIUM — based on reverse-engineering of .intunewin format. The + requirement text "Intune displays it as the app icon" describes the end-state in + Intune, not a technical requirement for the .intunewin binary. + - Recommendation: Include the icon in the package root as `icon.png` for user + convenience (they can extract it when uploading manually to Intune portal). + +--- + +## Validation Architecture + +### Test Framework + +| Property | Value | +|----------|-------| +| Framework | pytest (detected: pytest.ini or pyproject.toml implicit) | +| Config file | none detected — runs via `python -m pytest` | +| Quick run command | `python -m pytest tests/test_packages.py -x -q` | +| Full suite command | `python -m pytest tests/ -q` | + +### Phase Requirements -> Test Map + +| Req ID | Behavior | Test Type | Automated Command | File Exists? | +|--------|----------|-----------|-------------------|-------------| +| PKG-04 | Icon PNG is copied into .intunewin staging dir when icon exists | integration | `python -m pytest tests/test_packages.py::TestIntunewinDownload -x -q` | Partial — test_packages.py exists but no icon-in-package test yet | +| PKG-04 | Export succeeds when no icon uploaded | integration | `python -m pytest tests/test_packages.py::TestIntunewinDownload -x -q` | Partial — baseline export tests exist | + +### Sampling Rate + +- **Per task commit:** `python -m pytest tests/test_packages.py -x -q` +- **Per wave merge:** `python -m pytest tests/ -q` +- **Phase gate:** Full suite green before `/gsd:verify-work` + +### Wave 0 Gaps + +- [ ] `tests/test_packages.py` — add `test_intunewin_includes_icon` test (monkeypatch `build_intunewin`) +- [ ] `tests/test_packages.py` — add `test_intunewin_without_icon_succeeds` test + +*(Existing test infrastructure covers the rest — no new files or fixtures needed)* + +--- + +## Sources + +### Primary (HIGH confidence) + +- Direct code reading: `imptune/api/packages.py` — full `get_intunewin_package()` implementation +- Direct code reading: `imptune/api/icons.py` — Icon upload, SHA256 storage path, `cfg.ICONS_DIR` +- Direct code reading: `imptune/db/models.py` — `Icon` model, FK to `Printer`, `sha256` field +- Direct code reading: `imptune/config.py` — `ICONS_DIR = str(Path(DATA_DIR) / "icons")` +- Direct code reading: `imptune/generators/intunewin_builder.py` — `build_intunewin` walks entire `source_dir` +- Direct code reading: `tests/conftest.py` — `tmp_data_dir` monkeypatches `cfg.ICONS_DIR` +- Direct code reading: `tests/test_packages.py` — existing test patterns (fixtures, monkeypatch style) +- Direct code reading: `.planning/STATE.md` — Phase 5 decisions on ICONS_DIR, Icon model, shutil pattern + +### Secondary (MEDIUM confidence) + +- Intune .intunewin format: icon is NOT read from within the package by Intune — icons are + set in the Intune portal. Including it in the package is for user convenience only. + (Based on reverse-engineering notes in `intunewin_builder.py` comments + svrooij.io reference) + +--- + +## Metadata + +**Confidence breakdown:** +- Standard stack: HIGH — all libraries already in codebase, no new dependencies +- Architecture: HIGH — single code location change (packages.py), clear insertion point +- Pitfalls: HIGH — inner-ZIP test trap verified by reading intunewin_builder.py structure +- Test strategy: HIGH — monkeypatch pattern already established in existing test suite + +**Research date:** 2026-04-10 +**Valid until:** Stable — no moving parts (stdlib + existing codebase only)