diff --git a/.gitignore b/.gitignore index 6bd2f0c4..d4a818e6 100644 --- a/.gitignore +++ b/.gitignore @@ -33,3 +33,9 @@ DerivedData/ # Core dumps (a crashing test run inside a container drops one here) core core.* + +# Audit scratch: lift-up plans and burn-down reports are session artifacts. +# Their durable findings belong in ROADMAP.md (deferred work) and CHANGELOG.md +# (what shipped), not as 200KB of process notes at the repo root. +/lift-up-plan-*.md +/burn-down-report-*.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 8074c299..96c972d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,8 +6,32 @@ Rostrum is **pre-1.0**: minor versions may change API. Format follows ## [Unreleased] +Nothing yet. + +## [0.4.0] — 2026-08-17 + +The **"Measure & trust"** program (see `ROADMAP.md`): the library's two +biggest asserted qualities become measured ones. Text layout stops guessing — +real font metrics, a computed `normAutofit`, and text that provably fits its +box — and the lossless round-trip stops being unproven against foreign files, +gated by a real-deck corpus and the python-pptx oracle. Alongside it, a pass +of untrusted-input hardening: a malformed or hostile `.pptx` now throws, +clamps, or is recorded, instead of aborting the host process. + ### Added +- **`renderSVGReportingProblems(slideAt:pixelWidth:)`** — the same render as + `renderSVG`, with the inheritance diagnostics kept instead of dropped. The + returned `SlideRenderProblems` names a broken slide → layout → master link + (`layoutUnresolved` / `masterUnresolved`, `isEmpty` when the chain is + sound), so a caller can tell a damaged deck apart from one rendered wrong. + A slide with a broken chain still renders; it just comes back without what + it would have inherited. +- **`Shape.markAsPlaceholder(type:idx:)`** — writes the `p:ph` binding onto a + shape the caller positioned itself, so a drawn title is a *title* to + PowerPoint: present in the outline view, the slide navigator, "reuse + slides" and a screen reader. Costs no pixels, since an explicit `a:xfrm` + still wins. - **Deck extraction** — `Presentation.outline()` projects an opened deck onto a `Sendable` value model: per slide, the title, subtitle, body paragraphs with their outline level, table cells, SmartArt labels, speaker notes, an @@ -71,6 +95,25 @@ Rostrum is **pre-1.0**: minor versions may change API. Format follows ### Changed +- **`slides.add(layout:)` is now `slides.add(clonedFrom:)`.** The old label + did not say what it did, which left it a near-homophone of the adder that + binds a slide to a layout *without* copying its placeholder shapes. The new + name states plainly that it clones them. The old spelling still compiles, + deprecated with `renamed:`, so existing code keeps building and Xcode + offers the fix-it. +- **Slides built by the one-call builders carry real title placeholders.** + Every deck the builders wrote was headless: they draw the title on their + own grid as a plain text box, and a deck of plain text boxes has no titles + at all as far as PowerPoint is concerned — nothing in the outline view, the + slide navigator, "reuse slides" or a screen reader. Builders now bind the + slide to a layout that declares a title and mark the drawn title with + `p:ph`. Identical pixels; the semantics come back. +- **`ShapeCollection.count` and its subscript stopped walking the whole + tree.** Both routed through `all`, which builds one facade per shape, so + reading `count` — or indexing in a loop — cost a full `p:spTree` walk every + time and made ordinary iteration quadratic. They now count and index the + children directly, building no facade at all for `count` and exactly one + per subscript. No cache, so there is nothing that can go stale. - **Opening a `.potx` or `.ppsx` no longer converts it.** A template opens as a template and saves as one, byte-identically; previously it was retyped to a presentation on open, so a template could not survive a round trip. To @@ -94,8 +137,6 @@ Rostrum is **pre-1.0**: minor versions may change API. Format follows registered render with real word wrap and baseline placement in `renderSVG`; the rest wrap on a character-width estimate. -### Changed - - **`renderSVG` wraps long text instead of truncating it.** Paragraphs without registered metrics used to emit one line and drop the remainder behind an ellipsis, so a preview silently rewrote the deck's own words @@ -240,8 +281,6 @@ Rostrum is **pre-1.0**: minor versions may change API. Format follows coordinates, plus `shape.shapeID` and `shape.explicitFrame`. `shapes.autoShapes` keeps the old `p:sp`-only view. -### Changed - - **Read budgets are on by default.** `Presentation(data:)`, `OPCPackage.read` and `ZipReader` now default to `ZipReader.Limits.default` — 4 GiB of declared uncompressed bytes, far past any real deck — instead of @@ -293,6 +332,29 @@ Rostrum is **pre-1.0**: minor versions may change API. Format follows ### Fixed +- **A processing instruction with no data no longer kills the process on + Linux.** `` — the dataless spelling — is a NULL dereference + inside libxml2 by way of swift-corelibs-foundation's `XMLParser`: SIGSEGV, + not a throw, raised before any Rostrum code runs, so no caller could defend + against it. Since Rostrum opens files it did not write, any `.pptx` + carrying one was a **denial of service on untrusted input** — the same + family as the DOCTYPE rejection and the nesting ceiling, and the same + remedy this file already establishes: check the bytes before the parser + sees them, because that parser traps rather than throws. Isolated to + exactly that spelling — `` and `` parse, `` crashes, + comments are fine. Every dataless instruction is now given a payload before + parsing and has it turned back into `nil` on the way out, so the node still + knows it was the dataless spelling and still writes itself back as + ``; the payload is a token generated per parse rather than a + positional count, so there is no bookkeeping to drift out of step with + whichever instructions libxml2 reports. Comments and CDATA are stepped over + rather than scanned into — a lookalike inside one is content, and rewriting + it would change bytes the round trip promises to keep — and a document + without one, which is very nearly all of them, is not copied at all. + Root-caused in a Linux container rather than inferred from CI. +- `import FoundationNetworking` where Linux needs it: `OpenAIProvider` and + its test were the only two places using `URLSession` without it, which + broke the Linux build once the crash above stopped masking it. - **An edited part no longer loses its XML comments and processing instructions.** The promise is that opening and saving never drops XML Rostrum does not model, and a comment is the plainest case of that XML — @@ -389,6 +451,41 @@ Rostrum is **pre-1.0**: minor versions may change API. Format follows ### Lectern (the sample app) +- **The shell is a library.** The app opens on the decks you already have — + Quick Look covers, a grid or a list, search, and drag-and-drop onto the + shell itself — rather than on a compose form that asserted writing a deck + was the only thing the app did. Returning to the library is one movement + instead of three, the grid lays out once at the width it will end up, and + thumbnails survive the trip instead of being rebuilt on every return. +- **One deck inspector, not two.** The two that had grown separately are + reconciled into a single `DeckInspector`/`SlideDigest` pair in LecternCore, + which is why the inspection is testable headlessly on Linux. +- **Slide counts for the whole library are read concurrently, off the main + actor.** They were fetched one deck at a time on the main actor, so a + library of any size stalled the UI on launch. Decks that already have a + count are not re-read. +- **The two image caches are bounded.** Both grew for the life of the + process; they are now LRU with a cap, pinned by tests. +- **Settings only offers providers the app can actually use.** Two were + listed but unwired; OpenAI is now wired for real, and a stored selection + pointing at an unwired provider migrates to the default instead of + silently failing. +- **A file-picker failure is no longer indistinguishable from a cancel** — + the app says what went wrong instead of quietly doing nothing. +- **Rejected drafts are written with file protection on macOS too**, matching + what iOS already did. +- **Deleting a deck asks first.** The redesigned library deleted from a + button that promised a confirmation it never showed. +- The app icon has dark and tinted variants, on an OS that asks for both. +- **The app-hosted tests now run in a gate.** `Lectern/AppTests` compiled but + was executed by nothing; `scripts/verify.sh` runs it, along with a tracked + `scripts/hooks/pre-push` and `scripts/install-hooks.sh`. Implemented + locally rather than as a hosted-macOS CI step, so `ci.yml` is unchanged and + no CI cost was added. +- `Export Everything…` creates the folder it exports into, and test runs no + longer orphan saved API keys. +- Attached documents are fenced in the prompt, and the generation deadline is + a real ceiling rather than a suggestion. - **Cancel now stops the pipeline, not just the screen.** A cancel during the QA pass was swallowed by a `try?`, so the run went on to generate paid images and write a deck the user had stopped; cancellation now propagates, diff --git a/README.md b/README.md index 2d22ea18..62d88ccc 100644 --- a/README.md +++ b/README.md @@ -64,7 +64,7 @@ More recipes in the [cookbook](docs/COOKBOOK.md). Swift Package Manager: ```swift -.package(url: "https://github.com/welshofer/rostrum", from: "0.3.0") +.package(url: "https://github.com/welshofer/rostrum", from: "0.4.0") ``` then add `"Rostrum"` to your target's dependencies. @@ -85,7 +85,7 @@ then add `"Rostrum"` to your target's dependencies. | **Tables** | grid, cell fills & anchors, merge, banded rows | | **Charts** | bar / line / pie / area / doughnut / scatter / **radar / bubble / combo**, stacked & multi-series, titles, **data labels**, axis control, embedded Edit-Data workbook | | **Chart editing** | `deck.charts` reads any deck's charts; `replaceData` swaps every cache and the workbook or **refuses without writing a byte**; `addSeries` / `removeSeries` | -| **Rendering** | `renderSVG(slideAt:)` / `exportSVG` — headless slide→SVG previews with real font metrics, master/layout inheritance, no platform text stack | +| **Rendering** | `renderSVG(slideAt:)` / `exportSVG` — headless slide→SVG previews with real font metrics, master/layout inheritance, no platform text stack; `renderSVGReportingProblems` names a broken inheritance chain instead of quietly rendering without it | | **SmartArt** | Basic Block List creation; **text extraction from any diagram** | | **Comments** | modern threaded comments, replies, resolve | | **Notes** | per-slide speaker notes | diff --git a/ROADMAP.md b/ROADMAP.md index fa1f3807..37760ea3 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -190,7 +190,7 @@ shipped; both are re-opened above (Phase 3 / Phase 4 item 5). P4's "using the TTF metrics we already parse" was wrong: nothing parsed TTF metrics until v0.4 M1 below; the SVG renderer's text is still approximate until M2. -## Program: v0.4 — Measure & trust (2026-07-22 →) +## Program: v0.4 — Measure & trust ✅ (2026-07-22 → 2026-08-17, shipped as v0.4.0) The theme: convert the library's two biggest asserted qualities into *measured* ones. Text layout stops guessing (the #1 pain in Lectern, the @@ -279,8 +279,11 @@ Hardening backlog (schedule opportunistically): bomb amplifies across many entries~~ ✅ 2026-07-27. Resource exhaustion rather than a trap, so the fix had to be an API decision, not a patch: a caller-supplied `ZipReader.Limits` threaded through `OPCPackage.read` and - `Presentation.init`, defaulting to `.unlimited` so decks that are merely - large keep opening. + `Presentation.init`. It first defaulted to `.unlimited` so decks that are + merely large kept opening; the default is now `.default` — 4 GiB of + declared uncompressed bytes, far past any real deck — so untrusted input is + bounded out of the box, with `.unlimited` passed explicitly for archives + the caller already trusts. The budget is enforced **up front, from the central directory**, not accumulated as entries decode. Since each entry is bounded by its own declared @@ -412,6 +415,47 @@ encoding and `OPCPackage.serialize` to honour it for parts that never went dirty. Found 2026-07-28 by an adversarial review of the corpus gate, which had been claiming full byte identity in its own doc comment. +## Deferred — real, deliberately not scheduled + +Findings from the 2026-08-11 audits that were confirmed against the code and +then *not* acted on, recorded here so they are not rediscovered from scratch. +Each is a judgement about leverage, not a doubt about the finding. + +**Rostrum** + +- **Effective-frame inheritance matches layout → master by reduced type** + (`Slide.swift`) — a real asymmetry, but no observed deck reaches it and the + fix needs the full placeholder-matching table. +- **`OPCPackage` multi-pass serialisation** — measured in milliseconds against + a whole-deck save; below the noise floor. +- **`RostrumError` carries prose, not structured cases** — a genuine API + ergonomics gap, low leverage while the consumer set is this small. +- **Text measurement ignores kerning, ligatures and shaping** + (`FontMetrics.swift`) — documented behaviour; fixing it means a shaping + engine, which is out of scope for a zero-dependency library. +- **No snapshot or golden-file tests for `SVGRenderer`** — + `SVGRendererTests.swift` asserts structure, so a *visual* regression in the + preview path would pass silently. +- `DeckRenderer`, `KeychainStore` and `SlideRasterizer` have **no tests at + all** — distinct from tests that existed but never ran, which is closed. +- **`Examples/` and `Tools/` have never been audited** — four executable + targets plus `extract-schema.py` sit outside every surveyed set so far. + +**Lectern** + +- Only one image failure is reported when several fail — the collapse is in a + warning path the user rarely sees. +- No cancel affordance *during* a long generation — needs a cancellation + token threaded through `DeckGenerator` and the provider. +- **The decks already written remain headless.** The title-placeholder fix + applies to newly written decks only; repairing the existing library is a + migration, not a lift-up item. +- iOS keeps live `WKWebView` slide previews while macOS rasterizes — + `takeSnapshot` needs a window, and the iOS path is not currently slow. +- **`DeckRenderer.swift` is 923 lines** — the single place where IR, layout, + furniture, fonts, charts and previews all meet. Not a defect, but previews + and font resolution are both self-contained and the obvious next split. + ## Standing quality gates - `swift test` green on Linux on every push; on macOS in the pull-request diff --git a/Sources/Rostrum/Presentation/SVGRenderer.swift b/Sources/Rostrum/Presentation/SVGRenderer.swift index 632dc486..8c938f99 100644 --- a/Sources/Rostrum/Presentation/SVGRenderer.swift +++ b/Sources/Rostrum/Presentation/SVGRenderer.swift @@ -22,7 +22,11 @@ struct SVGRenderer { private let emuPerPoint = 12700 func render(pixelWidth: Int) throws -> (svg: String, problems: SlideRenderProblems) { - let dom = try slidePart.dom() + // Not for the value: this is the one call that surfaces a malformed + // slide part as a thrown error. Everything below reaches the tree + // through `existingSpTree`, which swallows the parse with `try?` and + // would render a silently blank slide instead. + _ = try slidePart.dom() // p:sldSz comes from the file too, and the aspect-ratio conversion below // goes through Int(_: Double), which traps when the double is out of // range — so bound the dimensions before dividing by them. diff --git a/burn-down-report-20260805.md b/burn-down-report-20260805.md deleted file mode 100644 index 0e34c79a..00000000 --- a/burn-down-report-20260805.md +++ /dev/null @@ -1,201 +0,0 @@ -# Burn-Down: Rostrum 0.4 + Lectern Deck Workbench - -> Approved direction: stop arbitrary template application; release the proven -> Rostrum work, then make Lectern exercise Rostrum's read and -> structure-preserving edit surface against real decks. -> -> Integration branch: `burndown/deck-workbench-20260805` - -## Preflight - -- Working tree: clean `main`, synchronized with `origin/main`. -- Remote: `git@github.com:welshofer/rostrum.git`. -- Backend/deploy surface: none. This run builds native apps and a Swift - library; deploy is not applicable. -- Orchestrator: GPT-5.6 Sol. The burn-down skill is calibrated for Fable; the - user's explicit “Do not stop” authorized proceeding on the current model. -- `CLAUDE_CODE_SUBAGENT_MODEL`: unset. -- Worktree isolation: not exposed by the agent API. Manual git worktrees are - used where items are disjoint; items sharing `AppState`, the workbench actor, - or inspector views are serialized. - -### Definition of Done - -```text -build: Lectern/scripts/build.sh -quiet && Lectern/scripts/build-ios.sh -quiet -test: swift test && (cd Lectern && swift test) && swift run ReadmeSnippets "$(mktemp -d)" -lint: not configured -gate: ./scripts/verify.sh -``` - -The repository defines no linter. `scripts/verify.sh` is the canonical gate and -runs both Swift test suites, the executable documentation, and both app builds. - -## Scope - -### REL-0 — Close the stale image-layout PR - -- **Location:** GitHub PR #14. -- **Proof:** PR #14 was open, stale since 2026-07-29, and conflicting with - `main`. -- **Do:** Close it; any useful crop behavior must return as a narrow, - independently-proven change. -- **Status:** shipped — PR #14 closed. - -### FUNC-1 — Build the deck workbench core - -- **Location:** `Lectern/App/AppState.swift:17-21`, - `Lectern/Sources/LecternCore/Rendering/DeckRenderer.swift:1-80`. -- **Proof:** `AppState.Phase` has only compose/generating/result/failed, and - `DeckRenderer` only turns generated `DeckIR` into a new presentation. No - production code opens an arbitrary user deck. -- **Do:** Add a `DeckWorkbench` actor and Sendable inspection snapshot that - opens `.pptx`/`.potx`/`.ppsx`, renders contact-sheet previews, and extracts - document properties, slide/layout/master identity, fonts, charts, notes, - comments, sections, media, shape counts, and validation issues. -- **Lane:** fable/orchestrator — architecture-defining actor + Sendable model. -- **Effort/impact:** L/L. - -### USE-1 — Build the Open Deck inspector UI - -- **Location:** `Lectern/App/ContentView.swift:31-40`, - `Lectern/App/DeckLibrarySheet.swift:22-54`, - `Lectern/App/LecternApp.swift:116-145`. -- **Proof:** the phase switch has no inspector; the library only opens, - reveals, and deletes generated files; File → Open Deck does not exist. -- **Do:** Add one PowerPoint file importer, menu/toolbar entry points, loading - and failure states, a contact sheet, and deck/slide details. Keep the first - slice read-only. -- **Lane:** fable/orchestrator — shared app state and cross-platform SwiftUI. -- **Effort/impact:** L/L. - -### STAB-1 — Add a local UI smoke test - -- **Location:** `Lectern/project.yml:1-81`. -- **Proof:** only application targets are declared; no UI-test target exists. -- **Do:** Add a macOS XCUITest that launches with a generated fixture and - proves the inspector is reachable and shows the deck. -- **Lane:** opus — bounded multi-file test harness. -- **Effort/impact:** M/L. - -### FUNC-2 — Add safe slide edits and save-as-copy - -- **Location:** `Sources/Rostrum/Presentation/Slides.swift:68-144`, - `Lectern/App/DeckLibrarySheet.swift:104-151`. -- **Proof:** Rostrum already has remove/move/duplicate, but Lectern exposes none - of them and has no workbench save path. -- **Do:** Expose reorder, duplicate, delete, undo-by-reload, and deterministic - save-as-copy from the inspector. Never overwrite the source. -- **Lane:** fable/orchestrator — mutable actor state + app flow. -- **Effort/impact:** L/L. - -### FUNC-3 — Add formatting-preserving text, notes, and metadata edits - -- **Location:** `Sources/Rostrum/Presentation/Notes.swift:5-36`, - `Sources/Rostrum/Presentation/DocumentProperties.swift`, - `Sources/Rostrum/Presentation/Text.swift`. -- **Proof:** Rostrum can edit these structures, but Lectern exposes none of - them. Text replacement must operate on existing runs rather than rebuild - shapes. -- **Do:** Add find/replace across existing runs, notes editing, and document - property editing; save only to a copy. -- **Lane:** opus — well-specified edit surface after workbench core exists. -- **Effort/impact:** M/L. - -### FUNC-4 — Add safe chart-data editing - -- **Location:** `Sources/Rostrum/Charts/ChartReader.swift:252-289`. -- **Proof:** `replaceData` and its refusal model exist, but no app calls them. -- **Do:** Inspect categories/series, edit values without structural change, - surface refusal reasons before mutation, and save to a copy. -- **Lane:** opus. -- **Effort/impact:** M/L. - -### FUNC-5 — Add image replacement that preserves composition - -- **Location:** `Sources/Rostrum/Presentation/Pictures.swift`, - `Sources/Rostrum/Presentation/Media.swift`. -- **Proof:** picture data can be read, but there is no public operation to - replace one picture without changing its frame/crop or mutating every shape - that shares the original media part. -- **Do:** Add copy-on-write picture replacement in Rostrum and expose it in the - workbench. -- **Lane:** opus — bounded OPC/media change with losslessness implications. -- **Effort/impact:** M/L. - -### FUNC-6 — Add slide import - -- **Location:** `Sources/Rostrum/Presentation/DeckMerge.swift:230`, - `Lectern/App/DeckLibrarySheet.swift`. -- **Proof:** `slides.importAll(from:)` exists, but Lectern cannot combine user - decks. -- **Do:** Import selected slides from a second deck, preserve their layouts and - relationships, preview the result, and save to a copy. -- **Lane:** opus. -- **Effort/impact:** M/L. - -### REL-1 — Build the local compatibility lab - -- **Location:** `Tests/RostrumTests/RealDeckCorpusTests.swift`, - `Lectern/Sources/LecternCore`. -- **Proof:** the repository corpus is developer-facing; Lectern cannot locally - open, validate, round-trip, reopen, render, or export a report for a user's - real deck. -- **Do:** Add a local-only compatibility run and exportable JSON/Markdown - report. Never upload or automatically commit decks. -- **Lane:** opus. -- **Effort/impact:** M/L. - -### REL-2 — Ship Rostrum 0.4.0 - -- **Location:** `CHANGELOG.md:7`, `README.md:51`, `ROADMAP.md:206`. -- **Proof:** latest release is `v0.3.1`; `main` already contains the v0.4 - Measure & Trust program and a large Unreleased changelog. -- **Do:** finalize release notes/docs, merge on green checks, tag `v0.4.0`, and - publish the GitHub release. -- **Lane:** sonnet for docs; orchestrator for tag/release. -- **Effort/impact:** M/L. - -## Out of Scope - -- Applying a template or “AI beautify” to an arbitrary existing deck. -- Recreating the abandoned `feat/apply-template`, `feat/rebind-theme`, or - `feat/lectern-rebrand` branches. -- Template-native generation. It may be reconsidered only after the workbench - proves placeholder/layout/font behavior against a real corpus. - -## Routing and Schedule - -| Wave | Item(s) | Execution | -|---|---|---| -| 0 | REL-0 | completed directly | -| 1 | FUNC-1 | orchestrator, serialized | -| 2 | USE-1, REL-1 | manual worktrees if write sets remain disjoint | -| 3 | STAB-1 | manual worktree | -| 4 | FUNC-2 | orchestrator, serialized | -| 5 | FUNC-3, FUNC-4, FUNC-5, FUNC-6 | manual worktrees only where core/UI files do not collide; otherwise serialized | -| 6 | REL-2 | docs worktree, then integration release | - -## Run Events - -- 2026-08-05: PR #14 closed. -- 2026-08-05: integration branch created from clean `main`. -- 2026-08-05, Wave 1 — FUNC-1 implemented directly on the integration - branch. Added `DeckWorkbench`, the Sendable inspection model, on-demand - slide rendering, and four integration tests. - - Build: macOS + iOS app builds passed. - - Test: 595 Rostrum tests, 126 LecternCore tests, and both README snippets - passed. - - Lint: not configured. - - Citation gate: the proof at `AppState.swift:17-21` still describes the app - surface until USE-1, but the core proof is false: - `Lectern/Sources/LecternCore/Workbench/DeckWorkbench.swift` now owns an - opened `Presentation` inside an actor and returns `DeckInspection`; no - `Presentation` crosses isolation. - - Opus review requested changes. Addressed all three before closing the - wave: arbitrary files now use a 1 GiB declared-output budget; an - unresolvable slide is skipped and reported instead of aborting inspection; - render errors are mapped to the workbench error contract; snapshot models - are producer-owned rather than exposing an unusable public initializer. - - Final integration gate: 595 Rostrum tests, 128 LecternCore tests, README - snippets, macOS build, and iOS Simulator build all passed. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 0c6e385b..69d51dd8 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -81,8 +81,10 @@ part. **Orphan preservation.** Zip members unreachable from the relationship graph are preserved on save (a stricter posture than python-pptx, which -re-packages only reachable parts). An audit API reports -them; `prune()` is an explicit opt-in. +re-packages only reachable parts). What a read could *not* keep is reported: +`deck.readWarnings` names any carried entry that failed to decode and was +dropped, rather than letting it vanish silently. A general orphan-audit API +and an opt-in `prune()` are intended but **not yet implemented**. **Stable identity handles.** `deck.slides[slideID]` subscripts keyed on the `sldId`/`spid` values already in the XML, alongside positional access, so user diff --git a/lift-up-plan-20260731-2.md b/lift-up-plan-20260731-2.md deleted file mode 100644 index 0144a2a1..00000000 --- a/lift-up-plan-20260731-2.md +++ /dev/null @@ -1,853 +0,0 @@ -# Lift-Up Plan: Lectern - -> Platform: mixed Apple — a SwiftUI app targeting macOS 26 and iOS/iPadOS 26 (`Lectern/project.yml`), on `LecternCore`, a SwiftPM package floored at macOS 13 / iOS 16 and built on Linux in CI -> Surveyed: 2026-07-31 -> Coverage: full — every file under `Lectern/App` (12 files) and `Lectern/Sources/LecternCore` (17 files), 5,610 lines, plus `project.yml` and the 2,057-line test target. Nothing in this target was left unread. `Sources/Rostrum` is **out of scope** by request; findings that resolve into the library are noted and excluded. -> Attractiveness anchor: inferred — **Raycast** (a single-window Mac utility: keyboard-first, instant, calm density, a first run that teaches itself) - -Second audit of this target today. Nine fixes landed on `main` in between, so every candidate here was re-verified against the current files rather than carried over — six previously-reported items are now genuinely fixed and appear only in *Dropped during verification*, not as findings. Two of the findings below are defects in code I wrote this afternoon. - ---- - -## Performance - -### 1. The style gallery re-filters 150 styles four times per keystroke - -- **Location:** `Lectern/App/StylePickerSheet.swift:27-37` -- **Proof:** - ```swift - private func matches(_ s: Style) -> Bool { - let q = query.trimmingCharacters(in: .whitespaces).lowercased() - let tagOK = activeTag == nil || s.tags.contains(activeTag!) - let qOK = q.isEmpty || s.name.lowercased().contains(q) - || s.tags.contains { $0.contains(q) } || (s.vibe?.lowercased().contains(q) ?? false) - return tagOK && qOK - } - - private var filtered: [Style] { app.styles.filter(matches) } - private var favorites: [Style] { filtered.filter { app.isFavorite($0.slug) } } - private var recents: [Style] { app.recents.compactMap { slug in filtered.first { $0.slug == slug } } } - ``` -- **Verified:** `grep -n 'filtered\|@State private var query\|debounce\|searchable' App/StylePickerSheet.swift` → - ``` - 9: @State private var query = "" - 35: private var filtered: [Style] { app.styles.filter(matches) } - 36: private var favorites: [Style] { filtered.filter { app.isFavorite($0.slug) } } - 37: private var recents: [Style] { app.recents.compactMap { slug in filtered.first { $0.slug == slug } } } - 48: if !favorites.isEmpty { section("Favorites", favorites) } - 49: if !recents.isEmpty { section("Recents", recents) } - 50: section(activeTag == nil && query.isEmpty ? "All \(app.styles.count)" : "\(filtered.count) results", filtered) - ``` -- **Do:** `filtered` is computed, and `body` evaluates it at lines 48, 49 and twice on 50 — four full passes over 150 styles per keystroke. Worse, `matches` recomputes `query.trimmingCharacters(...).lowercased()` *inside* the loop, so a single character typed allocates ~600 throwaway Strings. Hoist the normalized query into a computed property outside `matches`, and materialize `filtered` once into a `let` at the top of `body` (or `@State` updated in `.onChange(of: query)`), deriving `favorites`/`recents` from that one array. -- **Why:** Search across 150 styles is this sheet's primary interaction and it does ~600 allocations and four array copies per keypress, all on the main actor. -- **Effort:** S · **Impact:** M - -### 2. The contact sheet spawns one WKWebView — and one web content process — per slide - -- **Location:** `Lectern/App/SlidePreview.swift:48-56`, used from `SlideContactSheet` at `:92-101` -- **Proof:** - ```swift - @MainActor fileprivate func makeWebView() -> WKWebView { - let view = WKWebView() - #if os(iOS) - view.scrollView.isScrollEnabled = false - view.isOpaque = false - view.backgroundColor = .clear - #endif - return view - } - ``` -- **Verified:** `grep -rn 'WKProcessPool\|WKWebViewConfiguration\|takeSnapshot\|ImageRenderer' App/SlidePreview.swift` → `# (no matches)` -- **Do:** `slideCount` ranges to 40 (`ContentView.swift:206`), so a long deck instantiates up to 40 `WKWebView`s, each with its own WebKit content and networking process. `LazyVGrid` defers creation but never tears them down once scrolled past. Rasterize each SVG once via `WKWebView.takeSnapshot(with:)` and show plain `Image` views in the grid, keeping a live web view only for a full-size inspector. Failing that, share one `WKProcessPool` through a `WKWebViewConfiguration` and cap how many live views exist at once. -- **Why:** Forty web content processes for forty static pictures is hundreds of megabytes and visible scroll stutter, on the screen the user lands on after every single generation. -- **Effort:** M · **Impact:** L - -### 3. Launch reads 150 full `design.md` files to parse six header fields - -- **Location:** `Lectern/Sources/LecternCore/StyleCatalog/StyleCatalog.swift:77` -- **Proof:** - ```swift - private func parse(slug: String, designURL: URL, thumbnail: URL?) -> Style { - let text = (try? String(contentsOf: designURL, encoding: .utf8)) ?? "" - // YAML frontmatter takes precedence when present. - if let front = frontmatter(text) { - ``` -- **Verified:** `cat App/Resources/Styles/*.md | wc -c` → `1449192`; `ls App/Resources/Styles/*.md | wc -l` → `150`; `grep -rn 'cache\|manifest\|index.json' Sources/LecternCore/StyleCatalog/StyleCatalog.swift` → `# (no matches)` -- **Do:** 1.45 MB of markdown is read and UTF-8 decoded at every launch to extract name, vibe, category, theme, palette and font — all of which sit in the first ~40 lines of each file. It is correctly off-main (`AppState.loadStyles` wraps it in `Task.detached`), so this is latency-to-first-gallery rather than a hang. Generate a `styles-index.json` at build time from a script in `Lectern/scripts/` (matching the existing `build.sh`/`build-ios.sh` convention) and have `StyleCatalog` prefer it, falling back to the directory scan when absent. -- **Why:** The style picker is the app's most differentiated surface; it should be populated before the user can reach it, not after a 1.45 MB parse. -- **Effort:** M · **Impact:** S - -### 4. Slide previews render strictly serially, after the deck is already saved - -- **Location:** `Lectern/Sources/LecternCore/Rendering/DeckRenderer.swift:72-76` -- **Proof:** - ```swift - private static func previews(of presentation: Presentation) -> [String] { - (0.. Bool { id == .anthropic } - ``` -- **Verified:** `grep -rn 'struct OpenAIProvider\|struct GeminiProvider\|struct CustomProvider' Sources/` → `# (no matches)`. The only OpenAI/Gemini types are `OpenAIImageProvider` and `GeminiImageProvider`, which conform to `ImageProvider`, not `LLMProvider`. -- **Do:** `ProviderID` has four cases, Settings renders all four with a "(soon)" suffix, and `Theme.swift:29-35` gives each a polished display label — the whole surface is built for a capability that does not exist. `AnthropicProvider` is only 241 lines and `LLMProvider` is already the right seam, so port it to OpenAI's chat-completions + `tools` and Gemini's `generateContent` + `functionDeclarations`. Both now inherit the shared `HTTPRetry` policy for free. If they are not going to ship, cut the dead cases from `ProviderID` instead. -- **Why:** Anyone holding an OpenAI key sees it offered, selects it, pastes the key, and hits a dead end — the app advertises four doors and opens one. -- **Effort:** L · **Impact:** L - -### 2. The model list is a compile-time constant, and the live one is fetched then discarded - -- **Location:** `Lectern/App/AppState.swift:44-50` and `:226-235` -- **Proof:** - ```swift - static func defaultModels(for id: ProviderID) -> [String] { - switch id { - case .anthropic: return ["claude-opus-4-8", "claude-sonnet-5", "claude-fable-5", "claude-haiku-4-5-20251001"] - default: return [] - } - } - var modelOptions: [String] { Self.defaultModels(for: providerID) } - ``` - and the fetch that throws its result away: - ```swift - func validateKey() async { - guard let key = KeychainStore.read(for: providerID) else { keyStatus = .invalid("No key stored."); return } - keyStatus = .validating - do { - let models = try await AnthropicModels.list(apiKey: key) - keyStatus = .valid(models.count) - } catch { - ``` -- **Verified:** `grep -n 'AnthropicModels.list\|modelOptions\|defaultModels' App/AppState.swift` → - ``` - 44: static func defaultModels(for id: ProviderID) -> [String] { - 50: var modelOptions: [String] { Self.defaultModels(for: providerID) } - 230: let models = try await AnthropicModels.list(apiKey: key) - ``` - `models` is consumed only as `.count`; `modelOptions` never reads it. -- **Do:** The curation rationale at `:40-43` is sound — the raw `/v1/models` dump is full of point releases and EAP builds. But the consequence is that a new model needs an app rebuild, and the hardcoded list rots silently. Intersect the fetched list against a curated *prefix* allowlist rather than exact strings, so new point releases appear automatically while noise stays hidden, and mark any selected model the account can no longer reach. -- **Why:** A deck generator whose model menu is a compile-time constant is one provider release away from offering only models the user cannot call. -- **Effort:** S · **Impact:** M - -### 3. PDF grounding reads one document, silently stops at 40k characters, and cannot read scans - -- **Location:** `Lectern/App/PDFGrounding.swift:24-32` -- **Proof:** - ```swift - for i in 0.. maxChars { break } - } - let trimmed = text.trimmingCharacters(in: .whitespacesAndNewlines) - guard !trimmed.isEmpty else { return nil } - let truncated = trimmed.count > maxChars - return Source(name: url.lastPathComponent, - text: String(trimmed.prefix(maxChars)), - ``` -- **Verified:** `grep -rn 'VNRecognizeText\|import Vision\|allowsMultipleSelection' App/` → `# (no matches)`; `grep -n 'grounding' App/AppState.swift` shows `private(set) var grounding: PDFGrounding.Source?` — a single optional, not a collection. -- **Do:** Three gaps in order of value. (a) The loop `break`s positionally, so a 300-page report contributes only its opening pages while the UI still reports the full `doc.pageCount` — rank or summarize chunks by relevance to the prompt instead of truncating at the front. (b) Accept several PDFs by making `grounding` an array; `fileImporter` needs only `allowsMultipleSelection: true`. (c) Route the "no selectable text" case through `VNRecognizeTextRequest` rather than telling the user scans need OCR and stopping. -- **Why:** "Ground the deck on real facts" is what separates this from a chat prompt, and today it reads the first dozen pages of one text-layer PDF. -- **Effort:** M · **Impact:** M - -### 4. A finished deck can only be regenerated from scratch, though the revise path already exists - -- **Location:** the `.result` phase — `Lectern/App/ContentView.swift:41-48` -- **Proof:** - ```swift - @ViewBuilder private var phaseView: some View { - switch app.phase { - case .compose: ComposeView() - case .generating: GeneratingView() - case .result(let r): ResultView(result: r) - case .failed(let m): FailedView(message: m) - } - } - ``` -- **Verified:** `grep -rn 'revise' App/ Sources/LecternCore/` → - ``` - Sources/LecternCore/Providers/AnthropicProvider.swift:56: public func revise(_ request: DeckRequest, deckJSON: String, - Sources/LecternCore/Providers/DeckGenerator.swift:82: if let revised = try? await provider.revise(request, deckJSON: draftJSON, emit: emit), - Sources/LecternCore/Providers/Providers.swift:79: func revise(_ request: DeckRequest, deckJSON: String, - Sources/LecternCore/Providers/Providers.swift:85: func revise(_ request: DeckRequest, deckJSON: String, - ``` - Every call site is internal to the QA pass; nothing in `App/` reaches it. -- **Do:** `.result` is terminal — the only exit is "New", which discards everything and starts a fresh paid generation. `provider.revise(_:deckJSON:emit:)` is implemented and already round-trips a whole deck through the forced schema. Expose it: persist the validated `DeckIR` beside the `.pptx`, let the user tap a slide in the contact sheet, type an instruction, and re-render. The deck library from this afternoon gives that persisted IR somewhere to live. -- **Why:** One bad slide in twenty currently costs a full regeneration at full price, with no guarantee the other nineteen survive. -- **Effort:** L · **Impact:** L - -### 5. The library can open and delete a deck but not rename it, and the app generates colliding names - -- **Location:** `Lectern/App/DeckLibrarySheet.swift:120-152` (the row's actions), against `Lectern/Sources/LecternCore/Rendering/DeckRenderer.swift:885-895` -- **Proof:** the name is derived from the title, with a numeric suffix on collision: - ```swift - private func outputURL(title: String, in directory: URL) throws -> URL { - try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) - let base = slugify(title.isEmpty ? "deck" : title) - var candidate = directory.appendingPathComponent("\(base).pptx") - var n = 2 - while FileManager.default.fileExists(atPath: candidate.path) { - candidate = directory.appendingPathComponent("\(base)-\(n).pptx") - n += 1 - } - return candidate - } - ``` -- **Verified:** `grep -rn 'rename\|moveItem' App/DeckLibrarySheet.swift Sources/LecternCore/Storage/DeckLibrary.swift` → - ``` - Sources/LecternCore/Storage/DeckLibrary.swift:85: try fileManager.removeItem(at: deck.url) - ``` - No rename anywhere. And the real Decks folder shows the collision behaviour in the wild: `train-the-skill-not-the-weights.pptx`, `-2.pptx`, `-3.pptx`, plus `training-the-skill-not-the-weights.pptx` and `-2.pptx` — five near-identical names from re-runs of one idea. -- **Do:** Add rename to `DeckLibrary` (a `moveItem` with the same never-overwrite guard `DeckStorage.migrateDecks` uses) and an inline-editable name in the row. This is what makes the numeric-suffix pile-up survivable: the date and size in the row tell decks apart, but nothing lets the user fix the names. -- **Why:** Re-running a prompt is the normal workflow, and it produces indistinguishable files the user cannot correct from inside the app that made them. -- **Effort:** M · **Impact:** M - ---- - -## Stability - -**4 items, not 5.** A fifth candidate — the `outputURL` existence-then-write race — was investigated and dropped as unreachable, because generation is serialized behind `phase != .generating` (see *Dropped during verification*). Rather than pad, this dimension ships four. That is a fair count for a target with no force-unwraps, no `try!`, and no TODOs across 5,610 lines. - -### 1. A cancelled generation's completion clobbers the run that replaced it - -- **Location:** `Lectern/App/AppState.swift:310-323` -- **Proof:** - ```swift - self.phase = .result(result) - } catch is CancellationError { - self.phase = .compose - } catch { - self.phase = .failed(Self.describe(error)) - } - } - } - - func cancel() { task?.cancel(); task = nil; phase = .compose } - ``` -- **Verified:** `grep -c 'generationID\|epoch' App/AppState.swift` → `0`. The identity-guard pattern *is* known in this file — `grep -n 'guard imageProviderID == id' App/AppState.swift` → - ``` - 205: guard imageProviderID == id else { return } - 208: guard imageProviderID == id else { return } - ``` - — it is simply absent from the terminal `phase` writes. -- **Do:** Cancellation is cooperative, so the old task's `catch` runs some time *after* `cancel()` returns. Cancel then immediately regenerate, and the stale task resolves and writes `.compose` (or `.failed`) over the live `.generating` run — dropping the UI back to the form while a paid generation continues invisibly, its result unreachable. Add a monotonically increasing `generationID`, capture it in the task, and guard every terminal write with `guard self.generationID == captured else { return }`, exactly as line 205 already does for the image provider. -- **Why:** The user watches their in-flight generation vanish for no reason, and has no route back to the deck it eventually produces. -- **Effort:** S · **Impact:** M - -### 2. `validateKey()` is missing the provider guard its image-side twin has - -- **Location:** `Lectern/App/AppState.swift:226-235` -- **Proof:** - ```swift - func validateKey() async { - guard let key = KeychainStore.read(for: providerID) else { keyStatus = .invalid("No key stored."); return } - keyStatus = .validating - do { - let models = try await AnthropicModels.list(apiKey: key) - keyStatus = .valid(models.count) - } catch { - keyStatus = .invalid(Self.describe(error)) - } - } - ``` -- **Verified:** the image equivalent, `sed -n '198,210p' App/AppState.swift` → - ``` - imageKeyStatus = .validating - do { - try await ImageProviderFactory.validate(id: id, apiKey: key) - guard imageProviderID == id else { return } - imageKeyStatus = .valid - } catch { - guard imageProviderID == id else { return } - imageKeyStatus = .invalid(Self.describe(error)) - } - ``` - `validateImageKey` captures `let id = imageProviderID` up front and guards both branches. `validateKey` captures nothing and guards nothing. -- **Do:** Capture `let id = providerID` before the `await` and guard both writes with `guard providerID == id else { return }`. Note `selectProvider` (`:158`) already resets `keyStatus = .unknown`, so a late write actively overwrites a correct reset with a stale verdict. -- **Why:** Switching provider in Settings mid-validation paints a green "Valid · 4 models" — or a red rejection — against the wrong provider's key, in the one place in the app whose entire job is telling you whether your key works. -- **Effort:** S · **Impact:** S - -### 3. A missing Styles resource degrades to zero styles with nothing said anywhere - -- **Location:** `Lectern/App/AppState.swift:131-136`, with `Lectern/Sources/LecternCore/StyleCatalog/StyleCatalog.swift:47` -- **Proof:** - ```swift - func loadStyles() async { - guard styles.isEmpty, let dir = Bundle.main.resourceURL?.appendingPathComponent("Styles") else { return } - let loaded = await Task.detached { (try? StyleCatalog().load(from: dir)) ?? [] }.value - styles = loaded - if selectedStyleSlug == nil { selectedStyleSlug = recents.first ?? loaded.first?.slug } - } - ``` - and one layer down: - ```swift - let entries = (try? fm.contentsOfDirectory(at: root, includingPropertiesForKeys: [.isDirectoryKey])) ?? [] - ``` -- **Verified:** `grep -rn 'styleLoadError\|stylesError\|ContentUnavailableView' App/AppState.swift App/ContentView.swift` → `# (no matches)`. The app's only `ContentUnavailableView`s are `StylePickerSheet.swift:46` (empty *search result*) and `DeckLibrarySheet.swift:27` (empty *library*) — neither is a load failure. -- **Do:** Three `try?`-to-default conversions stack: a missing bundle folder, an unreadable directory, an unreadable file. The result is `styles == []`, `selectedStyleSlug == nil`, and `generate()` proceeding with `styleSlug: "default"` and `designURL: nil` — an unstyled deck, with the picker still cheerfully offering to "Search 150 styles". Add a `styleLoadError: String?` to `AppState`, set it when the load throws or returns empty, and surface it on the STYLE card. -- **Why:** A resource-bundling regression would ship an app that quietly produces unstyled decks and passes every headless test, because `LecternCoreTests` uses its own fixtures rather than the app bundle. -- **Effort:** S · **Impact:** M - -### 4. Launch does two synchronous Keychain reads on the main actor, each materializing the whole secret - -- **Location:** `Lectern/App/AppState.swift:117-127`, calling `Lectern/App/KeychainStore.swift:40-56` -- **Proof:** - ```swift - init() { - let d = UserDefaults.standard - if let raw = d.string(forKey: Keys.provider), let id = ProviderID(rawValue: raw) { providerID = id } - if let m = d.string(forKey: Keys.model), Self.defaultModels(for: providerID).contains(m) { model = m } - if let raw = d.string(forKey: Keys.imageProvider), let id = ImageProviderID(rawValue: raw) { imageProviderID = id } - favorites = Set(d.stringArray(forKey: Keys.favorites) ?? []) - recents = d.stringArray(forKey: Keys.recents) ?? [] - useSmartArt = d.bool(forKey: Keys.useSmartArt) - hasKey = KeychainStore.hasKey(for: providerID) - hasImageKey = KeychainStore.hasKey(forImage: imageProviderID) - } - ``` -- **Verified:** `grep -n 'kSecReturnData\|static func hasKey' App/KeychainStore.swift` → - ``` - 47: kSecReturnData as String: true, - 81: static func hasKey(for provider: ProviderID) -> Bool { read(for: provider) != nil } - 90: static func hasKey(forImage provider: ImageProviderID) -> Bool { read(forImage: provider) != nil } - ``` - `hasKey` is `read(...) != nil`, and `read` sets `kSecReturnData: true` — so an existence test decrypts and copies the API key. `grep -c 'KeychainStore.hasKey\|KeychainStore.read' App/AppState.swift` → `10`. -- **Do:** `AppState.init()` runs during `LecternApp`'s `@State` initialization, so two blocking `SecItemCopyMatching` calls sit ahead of the first frame; on a locked login keychain that is an indefinite main-thread stall behind a system unlock prompt. Add a `hasKey`-only query using `kSecReturnData: false` + `kSecReturnAttributes: true` so existence never materializes the secret, then move the probes into the existing `start()` (`:83-93`). The one wrinkle to handle deliberately: `hasKey` gates the Generate button, so make it a three-state `Bool?` and render nothing until known, rather than flashing "add an API key" on every launch. -- **Why:** A locked keychain turns launch into an indefinite hang, and the app decrypts its most sensitive value ten times over to answer a boolean. -- **Effort:** M · **Impact:** M - ---- - -## Reliability - -**4 items, not 5.** This dimension was largely emptied earlier today — the retry policy, `max_tokens` truncation, image fan-out and atomic writes all landed on `main` and are listed under *Dropped during verification*. What remains is genuinely four; padding to five would mean inventing a fifth. - -### 1. The Validate button is the one network call with no retry at all - -- **Location:** `Lectern/Sources/LecternCore/Providers/AnthropicModels.swift:28-40` -- **Proof:** - ```swift - case 200: - let obj = try? JSONSerialization.jsonObject(with: data) as? [String: Any] - let models = (obj?["data"] as? [[String: Any]])?.compactMap { $0["id"] as? String } ?? [] - return models - case 401, 403: - throw LecternError.authFailed(provider: "Anthropic") - case 429: - let retry = Int(http.value(forHTTPHeaderField: "Retry-After") ?? "") ?? 2 - throw LecternError.rateLimited(afterSeconds: retry) - default: - throw LecternError.providerError(status: http.statusCode, message: "couldn't list models") - } - ``` -- **Verified:** `for f in Sources/LecternCore/Providers/*.swift; do grep -q 'HTTPRetry' "$f" || echo "$f"; done` → - ``` - Sources/LecternCore/Providers/AnthropicModels.swift - Sources/LecternCore/Providers/DeckGenerator.swift - Sources/LecternCore/Providers/DeckSchema.swift - Sources/LecternCore/Providers/ImageGeneration.swift - Sources/LecternCore/Providers/PriceTable.swift - Sources/LecternCore/Providers/PromptTemplates.swift - Sources/LecternCore/Providers/ProviderFactory.swift - Sources/LecternCore/Providers/Providers.swift - ``` - Of those, only `AnthropicModels` makes network calls — the rest are prompts, schema, pricing and factories. So it is the single network caller the shared policy does not reach. -- **Do:** Mine, from this afternoon: I unified `AnthropicProvider`, `GeminiImageProvider` and `OpenAIImageProvider` onto `HTTPRetry` and did not notice this file. It still hand-rolls the 429 path, does not translate a `URLError`, and never retries — so a transient blip makes a perfectly good key report as broken. Route it through `HTTPRetry` and give it the same `send:` test seam the three providers have. -- **Why:** This backs the Validate button, whose entire purpose is to tell the user whether their key works. A dropped connection currently answers "no". -- **Effort:** S · **Impact:** M - -### 2. A validated, fully paid-for deck is discarded when rendering fails - -- **Location:** `Lectern/Sources/LecternCore/Providers/DeckGenerator.swift:56-76` and `:250-256` -- **Proof:** the draft is preserved on the schema-failure path only — - ```swift - private static func keepRejectedDraft(_ json: String, in directory: URL) -> URL? { - let url = directory.appendingPathComponent("rejected-draft.json") - do { - try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) - try json.write(to: url, atomically: true, encoding: .utf8) - return url - } catch { - return nil - } - } - ``` -- **Verified:** `grep -n 'keepRejectedDraft\|RenderError.renderFailed' Sources/LecternCore/Providers/DeckGenerator.swift` → - ``` - 57: if let kept = Self.keepRejectedDraft(repaired.json, in: diagnostics ?? directory) { - 66: private static func keepRejectedDraft(_ json: String, in directory: URL) -> URL? - 253: } catch let RenderError.renderFailed(underlying) { - ``` - Line 57 is the schema-invalid branch; line 253 rethrows a render failure and keeps nothing. -- **Do:** The instinct is right and already documented — "Without it the only record of what the model actually sent is an error string." But it is applied to the failure where the deck was *invalid*, not the one where the deck was **valid and our renderer broke**. Persist the validated `DeckIR` before calling `renderer.render`, and on `RenderError` keep it and offer a retry that skips straight back to rendering. -- **Why:** A bug in our own renderer costs the user the entire generation fee with nothing recoverable. -- **Effort:** S · **Impact:** M - -### 3. A 120-second timeout now hides behind three retries with no overall deadline - -- **Location:** `Lectern/Sources/LecternCore/Providers/AnthropicProvider.swift:158-166` -- **Proof:** - ```swift - while true { - var req = URLRequest(url: endpoint, timeoutInterval: 120) - req.httpMethod = "POST" - req.setValue(apiKey, forHTTPHeaderField: "x-api-key") // never logged (I1) - req.setValue("2023-06-01", forHTTPHeaderField: "anthropic-version") - req.setValue("application/json", forHTTPHeaderField: "content-type") - req.httpBody = try JSONSerialization.data(withJSONObject: payload) - ``` -- **Verified:** `grep -n 'timeoutInterval\|maxAttempts\|deadline\|Date()' Sources/LecternCore/Providers/AnthropicProvider.swift` → - ``` - 159: var req = URLRequest(url: endpoint, timeoutInterval: 120) - 183: guard attempt + 1 < HTTPRetry.maxAttempts else { - 200: if attempt + 1 < HTTPRetry.maxAttempts else { - ``` - No wall-clock deadline anywhere; `HTTPRetry.maxAttempts` is 3. -- **Do:** Also mine, from this afternoon — adding retries without a ceiling changed the worst case. A deck request that times out now does so three times with 2s and 4s backoff between: up to ~366 seconds during which `GeneratingView` shows a spinner and a stage label that never advances. Track a deadline at the top of `send` and stop retrying once it passes, and emit a progress event on each retry so the UI can say "connection dropped — retrying" instead of appearing hung. -- **Why:** The fix for one dropped socket should not turn a stalled network into six minutes of a UI that looks frozen and says nothing. -- **Effort:** S · **Impact:** M - -### 4. The migration notice is lost if the user quits before seeing it - -- **Location:** `Lectern/App/AppState.swift:83-93` -- **Proof:** - ```swift - func start() async { - #if os(macOS) - let moved = await Task.detached { Self.migrateLegacyDecks() }.value - if moved > 0 { - migrationNotice = "Moved \(moved) deck\(moved == 1 ? "" : "s") to Documents › Lectern." - } - #endif - refreshLibrary() - } - ``` -- **Verified:** `grep -n 'migrationNotice' App/AppState.swift App/ContentView.swift` → - ``` - App/AppState.swift:79: private(set) var migrationNotice: String? - App/AppState.swift:82: func dismissMigrationNotice() { migrationNotice = nil } - App/AppState.swift:88: migrationNotice = "Moved \(moved) deck\(moved == 1 ? "" : "s") to Documents › Lectern." - ``` - In-memory only — no `UserDefaults`, no persistence. -- **Do:** Also mine. The migration is once-only and irreversible from the app's side: after it runs, `moved` is 0 forever. If the user quits before reading the notice — or never opens Compose that session — they are never told their 21 decks moved, and the old folder is gone. Persist a "migration announced" flag in `UserDefaults` and keep showing the notice until it is actually dismissed. -- **Why:** A one-shot message about relocating someone's documents should not depend on them looking at the right screen during the right launch. -- **Effort:** S · **Impact:** S - ---- - -## Security - -### 1. The macOS app ships with neither App Sandbox nor Hardened Runtime - -- **Location:** `Lectern/project.yml:26-45` (the macOS target's `settings.base`) -- **Proof:** - ```yaml - settings: - base: - PRODUCT_BUNDLE_IDENTIFIER: com.lectern.app - MARKETING_VERSION: "1.0" - CURRENT_PROJECT_VERSION: "1" - GENERATE_INFOPLIST_FILE: "YES" - ASSETCATALOG_COMPILER_APPICON_NAME: AppIcon - SWIFT_VERSION: "6.0" - SWIFT_STRICT_CONCURRENCY: complete - ``` -- **Verified:** `grep -rn 'ENABLE_APP_SANDBOX\|ENABLE_HARDENED_RUNTIME\|com.apple.security' project.yml` → `# (no matches)`; `find . -name '*.entitlements' -not -path '*/.build*'` → `./App/Lectern-iOS-Sim.entitlements` — iOS simulator only. The macOS target has no entitlements file at all. -- **Do:** The app reads user-selected PDFs, holds API keys in the login keychain, calls three vendors, and renders generated markup in a WebKit process — unsandboxed, without hardened runtime. `AppState.attachPDF` already calls `startAccessingSecurityScopedResource()`, so the code is written *as if* sandboxed. Add an entitlements file with `com.apple.security.app-sandbox`, `files.user-selected.read-write` and `network.client`, and set `ENABLE_HARDENED_RUNTIME: "YES"`. **Sequence this after a decision on deck storage:** inside a sandbox `.documentDirectory` resolves to the container, which would put decks straight back into a hidden per-app folder and undo `AppState.swift:370-390`. Reaching the real `~/Documents` needs a user-chosen folder plus a security-scoped bookmark. -- **Why:** Without hardened runtime the app cannot be notarized and cannot ship; without the sandbox, a WebKit or PDFKit parsing bug is an unconfined foothold. -- **Effort:** M · **Impact:** L - -### 2. Rejected drafts are written in the clear, under one fixed name, and never expire - -- **Location:** `Lectern/Sources/LecternCore/Providers/DeckGenerator.swift:66-76` -- **Proof:** - ```swift - private static func keepRejectedDraft(_ json: String, in directory: URL) -> URL? { - let url = directory.appendingPathComponent("rejected-draft.json") - do { - try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) - try json.write(to: url, atomically: true, encoding: .utf8) - return url - } catch { - return nil - } - } - ``` -- **Verified:** `grep -rn 'prune\|expire\|olderThan\|completeFileProtection\|FileProtection' Sources/LecternCore/ App/` → `# (no matches)`. The only `removeItem` calls are `DeckLibrary.swift:85` (user-initiated delete) and `DeckStorage.swift:68` (legacy folder cleanup) — neither touches diagnostics. -- **Do:** Half-fixed today: it now lands in `Lectern/Diagnostics` rather than among the user's documents, which was the urgent part. What remains is retention. The file is the model's rendering of the prompt plus up to 40,000 characters lifted from whatever PDF was attached, it keeps one fixed name so the newest silently replaces the last, and nothing ever deletes it — a failed generation from months ago is still sitting there in plaintext. Give it a per-run name, set `.completeFileProtection` on iOS, and prune anything older than a few days on launch. -- **Why:** Confidential source material persists indefinitely with no retention policy and no way for the user to know it exists. -- **Effort:** S · **Impact:** M - -### 3. Slide previews render generated markup in WebKit with JavaScript enabled - -- **Location:** `Lectern/App/SlidePreview.swift:48-56` -- **Proof:** - ```swift - @MainActor fileprivate func makeWebView() -> WKWebView { - let view = WKWebView() - #if os(iOS) - view.scrollView.isScrollEnabled = false - view.isOpaque = false - view.backgroundColor = .clear - #endif - return view - } - ``` -- **Verified:** `grep -rn 'allowsContentJavaScript\|WKWebViewConfiguration\|WKPreferences\|navigationDelegate' App/` → `# (no matches)`. Escaping on the producing side is correct (`Sources/Rostrum/Presentation/SVGRenderer.swift:746-757` escapes `&`, `<`, `>` in text content), so this is defense in depth, not a live exploit. -- **Do:** `WKWebView()` takes a default configuration in which `defaultWebpagePreferences.allowsContentJavaScript` is `true`. The markup is assembled from LLM output that may itself be grounded in an attacker-supplied PDF, and it loads into the app's own WebKit context. Construct with a `WKWebViewConfiguration` setting `allowsContentJavaScript = false`, and add a `WKNavigationDelegate` that cancels every navigation except the initial `loadHTMLString`. The comment at `:19-22` already argues the nil `baseURL` removes network and file access — closing off script execution completes that argument. -- **Why:** One missed escape anywhere in a 780-line renderer becomes script execution inside the app rather than a broken thumbnail, and the mitigation is three lines. -- **Effort:** S · **Impact:** M - -### 4. Testing whether a key exists decrypts and copies the key - -- **Location:** `Lectern/App/KeychainStore.swift:40-56` and `:81` -- **Proof:** - ```swift - let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecAttrAccount as String: account, - kSecReturnData as String: true, - kSecMatchLimit as String: kSecMatchLimitOne, - ] - var item: CFTypeRef? - guard SecItemCopyMatching(query as CFDictionary, &item) == errSecSuccess, - let data = item as? Data else { return nil } - return String(data: data, encoding: .utf8) - ``` - with the existence test defined as a full read: - ```swift - static func hasKey(for provider: ProviderID) -> Bool { read(for: provider) != nil } - ``` -- **Verified:** `grep -n 'kSecReturnAttributes\|kSecReturnRef' App/KeychainStore.swift` → `# (no matches)`; `grep -c 'KeychainStore.hasKey' App/AppState.swift` → `6` -- **Do:** Every `hasKey` call materializes the plaintext API key into a Swift `String` — a heap allocation with no zeroing, retained until ARC gets round to it — purely to answer whether a key is present. Six call sites in `AppState` alone, two of them on the launch path. Add a dedicated existence query with `kSecReturnData: false` and `kSecReturnAttributes: true`, so the secret only leaves the keychain when it is genuinely about to be sent. -- **Why:** The app's most sensitive value is decrypted and copied into unmanaged memory dozens of times per session for no reason beyond a boolean, widening the window for a memory disclosure to matter. -- **Effort:** S · **Impact:** M - -### 5. Generated decks on iOS are published to the Files app by default - -- **Location:** `Lectern/project.yml:86-90` -- **Proof:** - ```yaml - # Generated decks land in Documents/Decks; these two make them visible - # in the Files app (and to Finder file sharing) rather than trapped in - # the sandbox. - INFOPLIST_KEY_UIFileSharingEnabled: "YES" - INFOPLIST_KEY_LSSupportsOpeningDocumentsInPlace: "YES" - ``` -- **Verified:** `grep -rn 'UIFileSharingEnabled\|LSSupportsOpeningDocumentsInPlace\|FileProtection' project.yml App/` → - ``` - project.yml:89: INFOPLIST_KEY_UIFileSharingEnabled: "YES" - project.yml:90: INFOPLIST_KEY_LSSupportsOpeningDocumentsInPlace: "YES" - ``` - No file-protection class is set anywhere. -- **Do:** This is a deliberate, well-reasoned trade — it is how decks are reachable at all on iOS, and it should stay. What is missing is the other half: `UIFileSharingEnabled` exposes the whole Documents directory over USB file sharing to anything that can talk to the device, and the decks themselves carry no data-protection class, so they are readable whenever the device is unlocked and by a backup. Set `.completeFileProtectionUntilFirstUserAuthentication` on written decks, and keep anything that is not a user document (diagnostics especially) out of Documents — which the Diagnostics split already starts. -- **Why:** Decks are generated from the user's prompts and their private PDFs; publishing them over USB file sharing with no protection class is a broader grant than "let them see their files". -- **Effort:** S · **Impact:** M - ---- - -## Usability - -### 1. The compose screen's icon-only buttons are unlabelled for VoiceOver - -- **Location:** `Lectern/App/ContentView.swift:197-199` -- **Proof:** - ```swift - Button { app.clearPDF() } label: { Image(systemName: "xmark.circle.fill") } - .buttonStyle(.plain).foregroundStyle(.secondary) - } - ``` -- **Verified:** `grep -n 'accessibility' App/ContentView.swift` → - ``` - 117: .accessibilityLabel("Dismiss") - 121: .accessibilityElement(children: .combine) - ``` - Both are on the migration notice I added today. The 401-line file's other controls — including this one — have none. The pattern exists elsewhere: `grep -rn 'accessibilityLabel' App/` also shows `SlidePreview.swift:108,139`, `StyleThumbnail.swift:84`, and `DeckLibrarySheet.swift:141,147,150`. -- **Do:** VoiceOver reads this as "xmark circle fill". Add `.accessibilityLabel("Remove PDF")`. `StylePickerSheet.swift:86` has the same unlabelled search-clear button. Then set `.accessibilityElement(children: .combine)` on each `Card` so a card reads as one unit rather than four fragments. -- **Why:** The PDF-grounding card is a primary flow and its only destructive control is unreachable by name — in an app that ships four dedicated accessibility *styles* (`contrastink`, `largeprint`, `nightreader`). -- **Effort:** S · **Impact:** M - -### 2. The Mac app has no menu bar of its own - -- **Location:** `Lectern/App/LecternApp.swift:88-110` -- **Proof:** - ```swift - var body: some Scene { - WindowGroup("Lectern") { - ContentView() - .environment(app) - #if os(macOS) - .background(LaunchFrame()) - .onAppear { delegate.app = app } - #endif - } - #if os(macOS) - .defaultSize(width: 780, height: 1060) - #endif - ``` -- **Verified:** `grep -rn '\.commands\|CommandGroup\|CommandMenu' App/` → `# (no matches)`. The whole keyboard surface is three shortcuts: `grep -rn 'keyboardShortcut' App/` → - ``` - App/StylePickerSheet.swift:76: .keyboardShortcut(.defaultAction) - App/ContentView.swift:238: .keyboardShortcut("l", modifiers: .command) - App/ContentView.swift:246: .keyboardShortcut(.return, modifiers: .command) - App/DeckLibrarySheet.swift:80: .keyboardShortcut(.defaultAction) - ``` -- **Do:** The app inherits SwiftUI's default File/Edit/View menus, full of items that do nothing here (New Window, Print, Undo). Add a `.commands { }` block: replace `CommandGroup(.newItem)` with "New Deck ⌘N" wired to `app.reset()`, add "Your Decks ⇧⌘L" and "Choose Style… ⇧⌘S", a "Open Decks Folder" item, and `CommandGroup(replacing: .help)` pointing at the README. Remove the inapplicable defaults. -- **Why:** Measured against the Raycast anchor, a keyboard-first Mac utility whose File menu is full of no-ops reads as a prototype rather than a Mac app. -- **Effort:** S · **Impact:** M - -### 3. The failure screen is a dead end that throws away its own diagnosis - -- **Location:** `Lectern/App/ContentView.swift:388-401` -- **Proof:** - ```swift - @Environment(AppState.self) private var app - let message: String - var body: some View { - VStack(spacing: 16) { - Image(systemName: "exclamationmark.triangle.fill").font(.system(size: 44)).foregroundStyle(.orange) - Text(message).font(.title3).multilineTextAlignment(.center).frame(maxWidth: 420) - Button("Back to Compose") { app.reset() }.buttonStyle(.glassProminent).controlSize(.large) - } - .padding(48) - .frame(maxWidth: .infinity, maxHeight: .infinity) - } - ``` -- **Verified:** `grep -rn 'Retry\|Try again\|Open Settings' App/` → `# (no matches)` -- **Do:** Every failure — rate limit, rejected key, dropped connection, truncated response, unparseable draft — funnels into one button that returns to the form. `AppState.describe` (`:352-378`) already knows exactly which `LecternError` occurred, then flattens it to a `String` and discards the type. Pass the `LecternError` itself into `FailedView` and branch: `.rateLimited` gets "Try again" with a countdown, `.authFailed`/`.noKey` get "Open Settings", `.networkOffline` a plain retry, `.responseTruncated` a "generate fewer slides" button that adjusts the stepper, and `.schemaInvalid` a "Show the rejected draft" that reveals the file whose path is currently sitting in the message as unclickable prose. -- **Why:** The most common failure is a rate limit, and it currently costs the user their place in the flow with no action available but starting over. -- **Effort:** S · **Impact:** M - -### 4. The style gallery's search is hand-rolled, so none of the platform behaviour works - -- **Location:** `Lectern/App/StylePickerSheet.swift:81-90` -- **Proof:** - ```swift - HStack(spacing: 8) { - Image(systemName: "magnifyingglass").foregroundStyle(.secondary) - TextField("Search 150 styles by name or vibe", text: $query) - .textFieldStyle(.plain) - if !query.isEmpty { - Button { query = "" } label: { Image(systemName: "xmark.circle.fill").foregroundStyle(.tertiary) } - .buttonStyle(.plain) - } - } - ``` -- **Verified:** `grep -rn 'searchable\|FocusState\|submitLabel' App/` → `# (no matches)` -- **Do:** Because it is a raw `TextField` rather than `.searchable`, the sheet opens with focus nowhere (you must click before typing), ⌘F does nothing, Esc does not clear, there is no scope bar for the tag chips, and iOS gets no search key on the keyboard. Replace with `.searchable(text: $query, placement: .toolbar, prompt: "Search 150 styles")`, add a `@FocusState` so the field is focused on presentation, and move `pillTags` into `.searchScopes` where it belongs. -- **Why:** Choosing among 150 styles is the app's most differentiated interaction, and reaching its search currently requires taking your hands off the keyboard. -- **Effort:** S · **Impact:** M - -### 5. The longest screen in the app announces nothing and estimates nothing - -- **Location:** `Lectern/App/ContentView.swift:268-284` -- **Proof:** - ```swift - @Environment(AppState.self) private var app - var body: some View { - VStack(spacing: 18) { - ProgressView().controlSize(.large) - Text(app.stage).font(.title3.weight(.semibold)).contentTransition(.opacity) - if app.total > 0 { - ProgressView(value: Double(app.drafted), total: Double(app.total)) - .frame(maxWidth: 280) - Text("\(app.drafted) of \(app.total) \(app.progressNoun)").font(.callout).foregroundStyle(.secondary) - } - Button("Cancel", role: .cancel) { app.cancel() }.buttonStyle(.glass) - } - .padding(48) - .frame(maxWidth: .infinity, maxHeight: .infinity) - } - ``` -- **Verified:** `grep -n 'accessibilityValue\|AccessibilityNotification\|announce\|elapsed\|ETA' App/ContentView.swift` → `# (no matches)` -- **Do:** Nothing here is announced when it changes, so a VoiceOver user hears silence for a multi-minute generation and cannot distinguish progress from a hang. Add `.accessibilityElement(children: .combine)` with an `.accessibilityValue` built from `stage` and `drafted/total`, and post an `AccessibilityNotification.Announcement` from `AppState.apply(_:)` on each stage change. Separately: `stage` moves through ten named phases with no elapsed time and no estimate, and `PriceTable.estimate` already models deck size well enough to produce a rough one. -- **Why:** The screen users spend the most time on is the least communicative, and for a VoiceOver user it is entirely opaque. -- **Effort:** S · **Impact:** M - ---- - -## Attractiveness / Sexiness - -Anchor: **Raycast** — a single-window Mac utility that feels instant, teaches itself on first launch, and treats motion as feedback rather than decoration. - -### 1. A Liquid Glass app with a flat, pre-26 app icon - -- **Location:** `Lectern/App/Assets.xcassets/AppIcon.appiconset/Contents.json` -- **Proof:** - ```json - { - "images" : [ - { "idiom" : "mac", "scale" : "1x", "size" : "16x16", "filename" : "icon_16.png" }, - { "idiom" : "mac", "scale" : "2x", "size" : "16x16", "filename" : "icon_32.png" }, - ``` - …through to: - ```json - { "idiom" : "universal", "platform" : "ios", "size" : "1024x1024", "filename" : "icon_1024.png" } - ], - "info" : { "author" : "xcode", "version" : 1 } - } - ``` -- **Verified:** `find . -name '*.icon' -not -path '*/.build*'` → `# (no matches)`; `ls App/Assets.xcassets/AppIcon.appiconset/` → `Contents.json icon_1024.png icon_128.png icon_16.png icon_256.png icon_32.png icon_512.png icon_64.png` — seven flat PNGs, no layered source. -- **Do:** Both targets deploy at 26.0 and the UI commits to Liquid Glass throughout (`.buttonStyle(.glass)`, `.glassProminent`, `.regularMaterial` cards). The icon is the one surface that did not come along: a flat pre-26 `.appiconset` gets none of the specular, depth, or tinted/clear/dark treatments the system now applies. Rebuild it in Icon Composer as a layered `.icon` and point `ASSETCATALOG_COMPILER_APPICON_NAME` at it. -- **Why:** The icon is the first and most-repeated impression — Dock, Spotlight, App Switcher — and it is currently the only part of the product that looks older than the OS it targets. -- **Effort:** M · **Impact:** M - -### 2. The four principal states cut hard, with no transition - -- **Location:** `Lectern/App/ContentView.swift:41-48` -- **Proof:** - ```swift - @ViewBuilder private var phaseView: some View { - switch app.phase { - case .compose: ComposeView() - case .generating: GeneratingView() - case .result(let r): ResultView(result: r) - case .failed(let m): FailedView(message: m) - } - } - ``` -- **Verified:** `grep -n 'animation\|transition\|withAnimation\|matchedGeometry' App/ContentView.swift` → `# (no matches)`. The app animates in exactly one place: `grep -rn 'withAnimation\|\.animation(' App/` → `App/Theme.swift:88: withAnimation(.easeOut(duration: 0.2)) { image = loaded }` -- **Do:** The entire user journey swaps instantaneously — pressing Generate replaces a full form with a spinner in a single frame, and the finished deck arrives with the same abruptness. Wrap the switch in `.animation(.smooth, value: app.phase)` and give each branch an asymmetric transition (compose pushes out, generating fades up, result scales in from the progress indicator). A `matchedGeometryEffect` from the Generate button to the progress ring would make the causal link explicit. -- **Why:** State changes are where a native app earns its feeling of quality; four hard cuts make a carefully built product feel like four screens bolted together. -- **Effort:** S · **Impact:** M - -### 3. First launch is a form you cannot submit - -- **Location:** `Lectern/App/ContentView.swift:225-232`, gated by `Lectern/App/AppState.swift:262-264` -- **Proof:** - ```swift - var canGenerate: Bool { - phase != .generating && hasKey && !prompt.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty - } - ``` - and the only guidance offered: - ```swift - if !app.hasKey { - Label("Add an API key in \(AppState.settingsHint) to generate", systemImage: "key") - .font(.callout).foregroundStyle(.secondary) - ``` -- **Verified:** `grep -rn 'onboard\|firstRun\|hasLaunched\|AppStorage\|welcome' App/*.swift` → `# (no matches)` -- **Do:** A new user opens a 780×1060 window showing five cards, a dimmed Generate button, and a line of grey text naming a menu item they must find themselves — and that label is not even a button. Build a first-run state: one welcoming panel explaining what Lectern does, a control that opens Settings directly at the key field (making that `Label` a `Button` is the minimum), and two or three of the best style thumbnails as a preview of what they are buying. Gate it on `@AppStorage("hasCompletedOnboarding")`. -- **Why:** First run is the only moment every user is guaranteed to reach, and it currently presents a locked door with the key described in small grey text. -- **Effort:** M · **Impact:** L - -### 4. The longest wait is a bare spinner, and the outline it could show is discarded - -- **Location:** `Lectern/App/ContentView.swift:271-279`, against `Lectern/App/AppState.swift:326-338` -- **Proof:** - ```swift - VStack(spacing: 18) { - ProgressView().controlSize(.large) - Text(app.stage).font(.title3.weight(.semibold)).contentTransition(.opacity) - if app.total > 0 { - ProgressView(value: Double(app.drafted), total: Double(app.total)) - .frame(maxWidth: 280) - Text("\(app.drafted) of \(app.total) \(app.progressNoun)").font(.callout).foregroundStyle(.secondary) - } - ``` -- **Verified:** `grep -n 'case .outlineReady' App/AppState.swift` → - ``` - 330: case .outlineReady: stage = "Outline ready" - ``` - The associated `DeckOutline` — title, sections, slide stubs — is not bound and not stored; `grep -c 'DeckOutline' App/AppState.swift` → `0`. -- **Do:** The pipeline emits a genuinely interesting narrative — outlining, drafting, validating, repairing, polishing, illustrating, rendering — and the UI flattens all ten stages into one line of text over a generic spinner. Show them as a checklist that fills in. The real prize is line 330: `outlineReady` carries the actual `DeckOutline` and the handler ignores its payload entirely. Bind it, and reveal the deck's title and section names the moment they land, then let slide thumbnails populate as they render. -- **Why:** A multi-minute wait is the app's biggest engagement risk and its biggest opportunity, and the data for a compelling progressive reveal is already arriving and being dropped one line short of the screen. -- **Effort:** M · **Impact:** L - -### 5. The payoff screen is buried under four collapsible caveat drawers - -- **Location:** `Lectern/App/ContentView.swift:322-380` -- **Proof:** - ```swift - if !result.warnings.isEmpty { - DisclosureGroup("\(result.warnings.count) validation warning(s)") { - ``` - followed in sequence by: - ```swift - if !result.droppedContent.isEmpty { - DisclosureGroup("\(result.droppedContent.count) slide(s) lost content to layout limits") { - ``` - ```swift - if !result.schemaIssues.isEmpty { - DisclosureGroup("\(result.schemaIssues.count) schema issue(s) in the written deck") { - ``` - ```swift - if !result.unmeasuredFonts.isEmpty { - DisclosureGroup("\(result.unmeasuredFonts.count) font(s) not installed") { - ``` -- **Verified:** `grep -c 'DisclosureGroup' App/ContentView.swift` → `4` -- **Do:** The taxonomy behind these four buckets is genuinely excellent, and the comments justifying the separation are the best writing in the file — but the user's moment of delight is "my deck is ready", and it arrives stacked beneath up to four grey accordions of caveats. Collapse them behind a single quiet "Details" affordance with an inline badge, give the contact sheet the full height, and make Open/Share the visual anchor. Keep all four categories intact *inside* the panel, where their precision is a feature rather than an apology. -- **Why:** This is the screen the entire product exists to reach; against the Raycast anchor it should feel like an arrival, not a lint report. -- **Effort:** S · **Impact:** M - ---- - -## First move - -**A cancelled generation's completion clobbers the run that replaced it** (from Stability) - -Ship this first because it is the only item here that can silently destroy work the user has already paid for, and because the fix is an afternoon. Cancel a generation, start another, and the first task's `catch` — running some indeterminate time later, since cancellation is cooperative — writes `.compose` or `.failed` over the live `.generating` state. The user watches their in-flight run vanish from the screen for no visible reason; the generation itself keeps going, keeps spending, and lands its result into a phase nobody is showing. There is no route back to that deck: the library only lists what reached disk, and this one may never get there. It ranks above the sandbox work (larger, and genuinely blocked on the deck-storage decision it would undo), above the provider gaps (multi-day), and above the retry and diagnostics items (real but lower stakes) because it is small, self-contained, and the pattern is already in this exact file — `validateImageKey` guards its late writes at lines 205 and 208, so the fix is to apply a known local idiom to the three terminal writes at 314-318. Doing it also unblocks Stability #2, which is the same bug in a second function, and makes the Reliability #3 retry work safe: adding retries lengthens the window during which a stale task can resolve over a live one. - -## Dropped during verification - -- **Anthropic retries once with a flat delay** — already fixed. `grep -n 'HTTPRetry' Sources/LecternCore/Providers/AnthropicProvider.swift` shows the shared policy in use; `HTTPRetry.swift:15-32` provides three attempts with exponential backoff. Landed today in `0f595d3`. -- **`max_tokens` is a flat 8,192 and truncation is silent** — already fixed. `grep -n 'stop_reason\|outputTokenBudget' Sources/LecternCore/Providers/AnthropicProvider.swift` → `48:"max_tokens": Self.outputTokenBudget(for: request)`, `104:obj["stop_reason"] as? String == "max_tokens"`. -- **Image generation fans out with no ceiling** — already fixed. `grep -n 'inFlightLimit' Sources/LecternCore/Providers/DeckGenerator.swift` → `126: let inFlightLimit = max(1, imageProvider.id.maximumConcurrentRequests)`. -- **Decks accumulate with no way to see them** — already fixed. `Sources/LecternCore/Storage/DeckLibrary.swift` and `App/DeckLibrarySheet.swift` now exist, reachable at ⌘L. -- **Decks are written to Application Support** — already fixed. `AppState.swift:370-390` writes to `~/Documents/Lectern` with a one-time migration. -- **`rejected-draft.json` is written among the user's decks** — partially fixed, so the *location* half is dropped and only retention survives, as Security #2. `grep -n 'diagnostics' Sources/LecternCore/Providers/DeckGenerator.swift` → `:34 diagnostics: URL? = nil` and `:57 in: diagnostics ?? directory`. -- **`outputURL` has a check-then-write race** — unreachable. `AppState.generate()` opens with `guard phase != .generating else { return }` (`:268`), so two generations cannot overlap, and nothing else in the target calls `render`. Would be real if concurrent generation were ever added; not real today. -- **`DeckNormalizer` could mangle prose into stat tiles** — cited code does something else. `promoteNumericBullets` requires `stats.count == bullets.count` (`DeckNormalizer.swift:170`), so it fires only when *every* bullet parses as a figure, and `leadingStat` rejects a lone number with no caption. -- **`StyleCatalog.palette` could loop forever on malformed markdown** — provably terminates. The scan reassigns `scan = rest` after each `#`, where `rest` is strictly shorter, so the loop advances even when no hex digits follow. - -## Deferred - -- **`DeckRenderer.swift` is 911 lines** — nearly twice the next-largest file in the target, and the single place where IR, layout, furniture, fonts, charts, scrims and previews all meet. Not a defect; the obvious split is previews and font resolution, both self-contained. -- **`LecternCoreTests.swift` is 1,495 lines in one suite** — 73% of the test target in a single file, now spanning rendering, validation, providers, retry and fonts. The three newer files (`DeckStorageTests`, `DeckLibraryTests`, `DeckNormalizerTests`) show the better shape. -- **No UI tests at all** — every test targets `LecternCore`; `App/` has none, which is why the Linux break and both Apple-platform regressions this week were caught by compilation rather than by a test. -- **`PDFGrounding.extract` calls `text.count` per page** — an O(n) grapheme walk inside the page loop. Bounded by `maxChars` so it is not pathological, but `text.utf8.count` would be O(1)-ish and exact for the purpose. -- **`Lectern/README.md:127` still describes decks as living in `Documents/Decks`** — true for iOS, stale for macOS since this afternoon's move to `~/Documents/Lectern`. diff --git a/lift-up-plan-20260731.md b/lift-up-plan-20260731.md deleted file mode 100644 index c01ed121..00000000 --- a/lift-up-plan-20260731.md +++ /dev/null @@ -1,958 +0,0 @@ -# Lift-Up Plan: Rostrum + Lectern - -> Platform: mixed — `Rostrum`, a zero-dependency Swift library (macOS 13 / iOS 16 / Linux), plus `Lectern`, a SwiftUI app (macOS 26 / iOS 26) that consumes it -> Surveyed: 2026-07-31 -> Coverage: partial — full read of `Lectern/App`, `Lectern/Sources/LecternCore`, `Sources/Rostrum/{XML,Zip,OPC,Core}`, `Presentation/{Presentation,SVGRenderer}.swift`, CI, and `project.yml`. Not read end-to-end: `Sources/Rostrum/Presentation/SlideBuilders.swift` (975 lines), `Charts/*` (2,436 lines), `Schema/GeneratedSchema.swift` (mechanically generated), `Tools/*`, and the 12,858-line test corpus beyond structural greps. -> Attractiveness anchor: inferred — **Raycast** (single-window Mac utility: keyboard-first, instant, calm density, a first run that teaches itself) - -A note on tone before the list: this is an unusually disciplined codebase. Sixteen thousand lines of library with **two** `try!` (both provably safe by construction), **zero** force-unwraps, zero TODOs, zip-bomb limits, coordinate-overflow bounding, and an XXE guard already in place. Seven candidates were dropped during verification because the fix was already there. The findings below are therefore mostly about the seams — the app layer, the async lifecycle, and one genuine hole in the library's untrusted-input defenses. - ---- - -## Performance - -### 1. Every keystroke in the style picker re-filters 150 styles three times - -- **Location:** `Lectern/App/StylePickerSheet.swift:27-37` -- **Proof:** - ```swift - private func matches(_ s: Style) -> Bool { - let q = query.trimmingCharacters(in: .whitespaces).lowercased() - let tagOK = activeTag == nil || s.tags.contains(activeTag!) - let qOK = q.isEmpty || s.name.lowercased().contains(q) - || s.tags.contains { $0.contains(q) } || (s.vibe?.lowercased().contains(q) ?? false) - return tagOK && qOK - } - - private var filtered: [Style] { app.styles.filter(matches) } - private var favorites: [Style] { filtered.filter { app.isFavorite($0.slug) } } - private var recents: [Style] { app.recents.compactMap { slug in filtered.first { $0.slug == slug } } } - ``` -- **Verified:** `grep -n 'filtered\|@State private var query\|debounce\|searchable' Lectern/App/StylePickerSheet.swift` → - ``` - 9: @State private var query = "" - 35: private var filtered: [Style] { app.styles.filter(matches) } - 36: private var favorites: [Style] { filtered.filter { app.isFavorite($0.slug) } } - 37: private var recents: [Style] { app.recents.compactMap { slug in filtered.first { $0.slug == slug } } } - 48: if !favorites.isEmpty { section("Favorites", favorites) } - 49: if !recents.isEmpty { section("Recents", recents) } - 50: section(activeTag == nil && query.isEmpty ? "All \(app.styles.count)" : "\(filtered.count) results", filtered) - ``` - No debounce, no memoization, no `searchable`. -- **Do:** `filtered` is a computed property, so `body` evaluates it at lines 48, 49, 50 (twice) — four full passes over 150 styles per keystroke, and `matches` re-runs `query.trimmingCharacters(...).lowercased()` inside the loop, allocating a fresh String 600 times per character typed. Hoist the normalized query out of `matches` into a computed `normalizedQuery`, and compute `filtered` once into a `let` at the top of `body` (or an `@State` updated in `.onChange(of: query)`), then derive `favorites`/`recents` from that single array. -- **Why:** Search in a 150-item gallery is the picker's primary interaction, and it currently does ~600 string allocations and 4 array copies per keypress on the main actor. -- **Effort:** S · **Impact:** M - -### 2. The contact sheet spawns one WKWebView — and one web content process — per slide - -- **Location:** `Lectern/App/SlidePreview.swift:48-56`, used from `SlideContactSheet` at `Lectern/App/SlidePreview.swift:92-101` -- **Proof:** - ```swift - @MainActor fileprivate func makeWebView() -> WKWebView { - let view = WKWebView() - #if os(iOS) - view.scrollView.isScrollEnabled = false - view.isOpaque = false - view.backgroundColor = .clear - #endif - return view - } - ``` -- **Verified:** `grep -n 'WKProcessPool\|WKWebViewConfiguration\|ImageRenderer\|snapshot\|takeSnapshot' Lectern/App/SlidePreview.swift` → `# (no matches)` -- **Do:** A 40-slide deck (the `slideCount` ceiling is `3...40`, `ContentView.swift:206`) instantiates up to 40 `WKWebView`s, each backed by its own WebKit content and networking process. `LazyVGrid` defers creation but never tears them down once scrolled. Render each SVG once to a bitmap via `WKWebView.takeSnapshot(with:)` (or rasterize off-main and cache a `CGImage`), then show plain `Image` views in the grid; keep a live web view only for a full-size single-slide inspector. Failing that, share one `WKProcessPool` via `WKWebViewConfiguration` and cap concurrent live views. -- **Why:** Forty web content processes for forty static pictures is hundreds of megabytes and a visible scroll stutter on the deck-review screen users land on after every generation. -- **Effort:** M · **Impact:** L - -### 3. Full-resolution generated images are base64-embedded into every 640px preview - -- **Location:** `Sources/Rostrum/Presentation/SVGRenderer.swift:297` -- **Proof:** - ```swift - guard let media = package.parts[target] else { return "" } - let ext = target.ext.lowercased() - let mime = ext == "jpg" || ext == "jpeg" ? "image/jpeg" : ext == "gif" ? "image/gif" : "image/png" - let data = media.blob.base64EncodedString() - return "" - } - ``` -- **Verified:** `grep -rn 'base64EncodedString\|downsample\|thumbnail\|CGImageSourceCreateThumbnail' Sources/Rostrum/` → - ``` - Sources/Rostrum/Presentation/SVGRenderer.swift:297: let data = media.blob.base64EncodedString() - ``` - Only one call site; no downsampling anywhere in the library. -- **Do:** `DeckRenderer` calls `renderSVG(slideAt:pixelWidth: 640)` (`DeckRenderer.swift:74`) while `GeminiImageProvider` requests `"image_size": "2K"` (`GeminiImageProvider.swift:66`). Each preview string therefore carries a whole 2K JPEG inflated 1.33× by base64 — for a 640px thumbnail. Add a `maxImagePixels` parameter to `renderSVG` that downsamples the blob (ImageIO on Apple platforms, a nearest-neighbour box filter on Linux to keep the zero-dependency rule) before encoding, defaulting to the render width. -- **Why:** `DeckResult.previews` is `[String]` held in memory and pushed into N web views; at 2K source images a 20-slide illustrated deck is tens of megabytes of base64 text for pictures displayed at 640px. -- **Effort:** M · **Impact:** L - -### 4. Slide previews render strictly serially - -- **Location:** `Lectern/Sources/LecternCore/Rendering/DeckRenderer.swift:72-76` -- **Proof:** - ```swift - private static func previews(of presentation: Presentation) -> [String] { - (0.. Style { - let text = (try? String(contentsOf: designURL, encoding: .utf8)) ?? "" - // YAML frontmatter takes precedence when present. - if let front = frontmatter(text) { - ``` -- **Verified:** `cat Lectern/App/Resources/Styles/*.md | wc -c` → `1449192`; `ls Lectern/App/Resources/Styles/*.md | wc -l` → `150`; `grep -rn 'cache\|Cache\|manifest\|index.json' Lectern/Sources/LecternCore/StyleCatalog/StyleCatalog.swift` → `# (no matches)` -- **Do:** 1.45 MB of markdown is read and fully decoded to UTF-8 at every launch to extract name, vibe, category, theme, palette and font — all of which live in the first ~40 lines. It is correctly off-main (`AppState.loadStyles` wraps it in `Task.detached`), so this is latency-to-first-gallery, not a hang. Generate a `styles-index.json` manifest at build time (a script in `Lectern/scripts/`, consistent with the existing `build-ios.sh` convention) and have `StyleCatalog` load that, falling back to the current directory scan when the manifest is absent. -- **Why:** The style picker is the app's signature surface; it should be populated before the user can reach it, not after a 1.45 MB parse. -- **Effort:** M · **Impact:** S - ---- - -## Functionality - -### 1. Three of the four advertised providers are not implemented - -- **Location:** `Lectern/Sources/LecternCore/Providers/ProviderFactory.swift:19-25` -- **Proof:** - ```swift - case .anthropic: - return AnthropicProvider(apiKey: key, model: model) - case .openAI, .gemini, .custom: - throw LecternError.providerError(status: 0, message: "\(id.rawValue) isn't wired up yet — use Anthropic.") - } - } - - /// Whether `id` currently has a live implementation (independent of any key). - public static func isWired(_ id: ProviderID) -> Bool { id == .anthropic } - ``` -- **Verified:** `grep -rn 'struct OpenAIProvider\|struct GeminiProvider\|struct CustomProvider' Lectern/Sources/` → `# (no matches)` (only `OpenAIImageProvider` and `GeminiImageProvider`, which are image-only and conform to `ImageProvider`, not `LLMProvider`). -- **Do:** `ProviderID` has four cases, Settings renders all four in the picker with a "(soon)" suffix, and `Theme.swift:29-35` gives each a polished display label — the whole surface is built for a capability that does not exist. `AnthropicProvider` is only 143 lines and the `LLMProvider` protocol is already the right seam, so port it to OpenAI's chat-completions + `tools` and Gemini's `generateContent` + `functionDeclarations`. Alternatively, cut the dead cases from `ProviderID` until they ship. -- **Why:** Every user who owns an OpenAI key sees it offered, selects it, pastes a key, and hits a dead end — the app advertises four doors and opens one. -- **Effort:** L · **Impact:** L - -### 2. Generated decks accumulate on disk with no way to see them again - -- **Location:** `Lectern/App/AppState.swift:332-345` -- **Proof:** - ```swift - static func decksDirectory() -> URL { - #if os(iOS) - // Documents, not Application Support: with UIFileSharingEnabled + - // LSSupportsOpeningDocumentsInPlace the decks show up in the Files app, - // which is the iOS equivalent of "Reveal in Finder". - let base = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask).first - ?? URL(fileURLWithPath: NSTemporaryDirectory()) - return base.appendingPathComponent("Decks", isDirectory: true) - ``` -- **Verified:** `grep -rn 'contentsOfDirectory' Lectern/App/ Lectern/Sources/` → - ``` - Lectern/Sources/LecternCore/Rendering/DeckRenderer.swift:251: guard let files = try? FileManager.default.contentsOfDirectory( - Lectern/Sources/LecternCore/StyleCatalog/StyleCatalog.swift:47: let entries = (try? fm.contentsOfDirectory(at: root, includingPropertiesForKeys: [.isDirectoryKey])) ?? [] - ``` - Line 251 enumerates Office **font** directories; line 47 enumerates **styles**. Nothing ever lists `decksDirectory()`. -- **Do:** `ContentView.swift:34` comments "No sidebar — there's no deck History to show, so a single pane is honest." The honesty is admirable but the decks are right there. Add a `DeckLibrary` that lists `decksDirectory()` sorted by creation date, with the stored `DeckIR` JSON beside each `.pptx` so a deck can be reopened, re-rendered in a different style, or re-generated. On macOS, restore the sidebar; on iOS, add a second tab. -- **Why:** Once you hit "New" the previous deck is unreachable inside the app, and each one cost a real API call — the product forgets everything the user paid for. -- **Effort:** L · **Impact:** L - -### 3. The key-validation call fetches the live model list and throws it away - -- **Location:** `Lectern/App/AppState.swift:44-47` and `Lectern/App/AppState.swift:190-196` -- **Proof:** - ```swift - static func defaultModels(for id: ProviderID) -> [String] { - switch id { - case .anthropic: return ["claude-opus-4-8", "claude-sonnet-5", "claude-fable-5", "claude-haiku-4-5-20251001"] - default: return [] - } - } - ``` - and: - ```swift - func validateKey() async { - guard let key = KeychainStore.read(for: providerID) else { keyStatus = .invalid("No key stored."); return } - keyStatus = .validating - do { - let models = try await AnthropicModels.list(apiKey: key) - keyStatus = .valid(models.count) - } catch { - ``` -- **Verified:** `grep -n 'AnthropicModels.list\|modelOptions\|defaultModels' Lectern/App/AppState.swift` → - ``` - 44: static func defaultModels(for id: ProviderID) -> [String] { - 53: var modelOptions: [String] { Self.defaultModels(for: providerID) } - 194: let models = try await AnthropicModels.list(apiKey: key) - ``` - `models` is consumed only as `.count`; `modelOptions` never reads it. -- **Do:** The curation rationale at lines 40-43 is sound — the raw `/v1/models` dump is noisy. But the consequence is that a new model requires an app rebuild, and the hardcoded list will silently rot (`claude-fable-5` and `claude-opus-4-8` are already unverifiable against the live account). Intersect the fetched list with a curated *prefix* allowlist rather than an exact-match list, so new point releases appear automatically while EAP builds stay hidden, and surface anything in the list that the account can no longer access. -- **Why:** A deck generator whose model menu is a compile-time constant is one provider release away from offering only models the user cannot call. -- **Effort:** S · **Impact:** M - -### 4. PDF grounding takes one document, silently caps at 40k characters, and cannot read scans - -- **Location:** `Lectern/App/PDFGrounding.swift:24-37` -- **Proof:** - ```swift - static func extract(from url: URL) async -> Source? { - await Task.detached(priority: .userInitiated) { () -> Source? in - guard let doc = PDFDocument(url: url) else { return nil } - var text = "" - for i in 0.. maxChars { break } - } - let trimmed = text.trimmingCharacters(in: .whitespacesAndNewlines) - guard !trimmed.isEmpty else { return nil } - let truncated = trimmed.count > maxChars - return Source(name: url.lastPathComponent, - text: String(trimmed.prefix(maxChars)), - pageCount: doc.pageCount, - truncated: truncated) - }.value - } - ``` -- **Verified:** `grep -rn 'VNRecognizeText\|Vision\|OCR\|\[URL\]\|grounding\b' Lectern/App/AppState.swift Lectern/App/PDFGrounding.swift` → - ``` - Lectern/App/AppState.swift:57: private(set) var grounding: PDFGrounding.Source? - Lectern/App/AppState.swift:216: func attachPDF(_ url: URL) async { - Lectern/App/AppState.swift:229: func clearPDF() { grounding = nil; groundingError = nil } - ``` - `grounding` is a single optional, not a collection; no Vision import anywhere. -- **Do:** Three gaps, ordered by value. (a) The loop breaks at 40k chars so a 300-page report contributes only its opening pages, while the UI still reports the full `doc.pageCount` — front-load a cheap extractive summary or chunk-and-rank by query relevance instead of truncating positionally. (b) Accept multiple PDFs by making `grounding` an array — `fileImporter` needs only `allowsMultipleSelection: true`. (c) Route the "no selectable text" case through `VNRecognizeTextRequest` rather than telling the user "scans need OCR" and stopping. -- **Why:** "Ground the deck on real facts" is the feature that separates this from a chat prompt, and today it reads the first 12 pages of one text-layer PDF. -- **Effort:** M · **Impact:** M - -### 5. A finished deck cannot be adjusted — only regenerated from scratch - -- **Location:** the compose → result flow, `Lectern/App/ContentView.swift:41-47` and `Lectern/App/AppState.swift:286` -- **Proof:** - ```swift - @ViewBuilder private var phaseView: some View { - switch app.phase { - case .compose: ComposeView() - case .generating: GeneratingView() - case .result(let r): ResultView(result: r) - case .failed(let m): FailedView(message: m) - } - } - ``` - and the only exit from `.result`: - ```swift - func reset() { stage = ""; drafted = 0; total = 0; phase = .compose } - ``` -- **Verified:** `grep -rn 'regenerate\|reroll\|editSlide\|func revise' Lectern/App/ Lectern/Sources/LecternCore/` → - ``` - Lectern/Sources/LecternCore/Providers/AnthropicProvider.swift:56: public func revise(_ request: DeckRequest, deckJSON: String, - Lectern/Sources/LecternCore/Providers/Providers.swift:79: func revise(_ request: DeckRequest, deckJSON: String, - Lectern/Sources/LecternCore/Providers/Providers.swift:85: func revise(_ request: DeckRequest, deckJSON: String, - ``` - `revise` exists but is called only internally by the QA pass (`DeckGenerator.swift:82`); no user-facing entry point. -- **Do:** The four-state phase machine is terminal at `.result` — the only affordance is "New", which discards the deck and starts a fresh paid generation. `provider.revise(_:deckJSON:emit:)` is already implemented and already round-trips a whole deck through the schema. Expose it: let the user tap a slide in the contact sheet, type an instruction, and re-render. Persist the validated `DeckIR` JSON next to the `.pptx` so this survives relaunch. -- **Why:** One bad slide in twenty currently costs a full regeneration at full price, with no guarantee the other nineteen survive intact. -- **Effort:** L · **Impact:** L - ---- - -## Stability - -**4 items, not 5.** A fifth candidate — "a cancel during the QA pass is swallowed and the deck renders anyway" — was investigated and dropped during verification once the renderer's own cancellation handling was read in full (see *Dropped during verification*). Rather than pad, this dimension ships four. That is the honest count for a library with two `try!` (both provably safe), zero force-unwraps, and zero TODOs across 16,179 lines. - -### 1. A cancelled generation's completion handler clobbers the state of the run that replaced it - -- **Location:** `Lectern/App/AppState.swift:246-286` -- **Proof:** - ```swift - self.phase = .result(result) - } catch is CancellationError { - self.phase = .compose - } catch { - self.phase = .failed(Self.describe(error)) - } - } - } - - func cancel() { task?.cancel(); task = nil; phase = .compose } - ``` -- **Verified:** `grep -n 'guard self.providerID == \|guard imageProviderID == id\|generation ==\|epoch\|token' Lectern/App/AppState.swift` → - ``` - 169: guard imageProviderID == id else { return } - 172: guard imageProviderID == id else { return } - 254: if self.imageProviderID == imageID { self.imageKeyStatus = .valid } - 256: if self.imageProviderID == imageID { - ``` - Identity guards exist in `validateImageKey` (169, 172) and for `imageKeyStatus` inside `generate` (254, 256) — but none guards the terminal `phase` writes at 276-281. -- **Do:** Cancellation is cooperative, so the old task's `catch` block runs some time *after* `cancel()` returns. If the user cancels and immediately regenerates, the stale task resolves and writes `phase = .compose` (or `.failed`) over the live `.generating` run, dropping the UI back to the compose form while a paid generation continues invisibly. Add a monotonically increasing `generationID`, capture it in the task, and guard every terminal write with `guard self.generationID == captured else { return }` — the same pattern already used correctly at line 169. -- **Why:** The user sees their in-flight generation vanish from the screen for no reason, and has no way to reach the deck it eventually produces. -- **Effort:** S · **Impact:** M - -### 2. `validateKey()` is missing the provider guard its image-side twin has - -- **Location:** `Lectern/App/AppState.swift:190-201` -- **Proof:** - ```swift - func validateKey() async { - guard let key = KeychainStore.read(for: providerID) else { keyStatus = .invalid("No key stored."); return } - keyStatus = .validating - do { - let models = try await AnthropicModels.list(apiKey: key) - keyStatus = .valid(models.count) - } catch { - keyStatus = .invalid(Self.describe(error)) - } - } - ``` -- **Verified:** `sed -n '160,176p' Lectern/App/AppState.swift` (the image equivalent) → - ``` - imageKeyStatus = .validating - do { - try await ImageProviderFactory.validate(id: id, apiKey: key) - guard imageProviderID == id else { return } - imageKeyStatus = .valid - } catch { - guard imageProviderID == id else { return } - imageKeyStatus = .invalid(Self.describe(error)) - } - ``` - `validateImageKey` captures `let id = imageProviderID` up front and guards both branches; `validateKey` captures nothing and guards nothing. -- **Do:** Capture `let id = providerID` before the `await`, then guard both the success and failure writes with `guard providerID == id else { return }`. Note `selectProvider` (line 121) already resets `keyStatus = .unknown`, so a late write actively overwrites a correct reset with a stale verdict. -- **Why:** Switching provider in Settings while a validation is in flight paints a green "Valid · 4 models" (or a red rejection) against the wrong provider's key — the one state in Settings a user has to trust. -- **Effort:** S · **Impact:** S - -### 3. A missing Styles resource folder degrades to zero styles with no error anywhere - -- **Location:** `Lectern/App/AppState.swift:88-93`, with `Lectern/Sources/LecternCore/StyleCatalog/StyleCatalog.swift:47` -- **Proof:** - ```swift - func loadStyles() async { - guard styles.isEmpty, let dir = Bundle.main.resourceURL?.appendingPathComponent("Styles") else { return } - let loaded = await Task.detached { (try? StyleCatalog().load(from: dir)) ?? [] }.value - styles = loaded - if selectedStyleSlug == nil { selectedStyleSlug = recents.first ?? loaded.first?.slug } - } - ``` - and, one layer down: - ```swift - let entries = (try? fm.contentsOfDirectory(at: root, includingPropertiesForKeys: [.isDirectoryKey])) ?? [] - ``` -- **Verified:** `grep -n 'stylesError\|styleLoadFailed\|ContentUnavailableView' Lectern/App/AppState.swift Lectern/App/ContentView.swift` → `# (no matches)` - (the only `ContentUnavailableView` in the app is `StylePickerSheet.swift:46`, for an empty *search result*, not a load failure.) -- **Do:** Three `try?`-to-default conversions stack up: a missing bundle folder, an unreadable directory, and an unreadable file each degrade silently. The result is `styles == []`, `selectedStyleSlug == nil`, and `generate()` proceeding with `styleSlug: "default"` and `designURL: nil` — an unstyled deck with no indication anything went wrong, while the picker cheerfully says "Search 150 styles". Add a `styleLoadError: String?` to `AppState`, set it when `load` throws or returns empty, and surface it on the STYLE card in `ComposeView`. -- **Why:** A resource-bundling regression in the Xcode project would ship an app that quietly produces unstyled decks and passes every headless test, because `LecternCoreTests` uses its own fixtures rather than the app bundle. -- **Effort:** S · **Impact:** M - -### 4. Synchronous Keychain reads run on the main actor, including at launch - -- **Location:** `Lectern/App/AppState.swift:74-81`, calling `Lectern/App/KeychainStore.swift:40-56` -- **Proof:** - ```swift - favorites = Set(d.stringArray(forKey: Keys.favorites) ?? []) - recents = d.stringArray(forKey: Keys.recents) ?? [] - useSmartArt = d.bool(forKey: Keys.useSmartArt) - hasKey = KeychainStore.hasKey(for: providerID) - hasImageKey = KeychainStore.hasKey(forImage: imageProviderID) - } - ``` - and `hasKey` resolves to a blocking `SecItemCopyMatching`: - ```swift - var item: CFTypeRef? - guard SecItemCopyMatching(query as CFDictionary, &item) == errSecSuccess, - let data = item as? Data else { return nil } - return String(data: data, encoding: .utf8) - ``` -- **Verified:** `grep -n 'KeychainStore.read\|KeychainStore.hasKey' Lectern/App/AppState.swift` → - ``` - 79: hasKey = KeychainStore.hasKey(for: providerID) - 80: hasImageKey = KeychainStore.hasKey(forImage: imageProviderID) - 124: hasKey = KeychainStore.hasKey(for: id) - 131: hasKey = KeychainStore.hasKey(for: providerID) - 147: hasImageKey = KeychainStore.hasKey(forImage: imageProviderID) - 191: hasImageKey = KeychainStore.hasKey(forImage: imageProviderID) - 243: hasKey = KeychainStore.hasKey(for: providerID) - 244: hasImageKey = KeychainStore.hasKey(forImage: imageProviderID) - ``` - `AppState` is `@MainActor` (line 9), so all eight are main-thread; `KeychainStore` exposes no async variant. -- **Do:** `AppState.init()` runs during `LecternApp`'s `@State` initialization, so two blocking `SecItemCopyMatching` calls sit on the launch path. If the login keychain is locked — a normal state after a cold boot on macOS — the call blocks the main thread behind a system unlock prompt before the first frame draws. Worse, `hasKey` reads the *entire secret* just to test existence. Add `kSecReturnData: false` + `kSecReturnAttributes: true` for the existence check, and move the launch-time probes into an `async` step after first paint. -- **Why:** A locked keychain currently turns app launch into an indefinite main-thread stall, and the app reads its most sensitive value eight times over just to answer a boolean. -- **Effort:** S · **Impact:** M - ---- - -## Reliability - -### 1. The text provider retries once, with a flat delay — while the image provider does it properly - -- **Location:** `Lectern/Sources/LecternCore/Providers/AnthropicProvider.swift:78-106` -- **Proof:** - ```swift - let data: Data, response: URLResponse - do { (data, response) = try await session.data(for: req) } - catch let error as URLError where error.code == .notConnectedToInternet { throw LecternError.networkOffline } - ``` - and the retry arm: - ```swift - case 429, 500...599: - let retryAfter = Int(http.value(forHTTPHeaderField: "Retry-After") ?? "") ?? 2 - if attempt == 0 { try? await Task.sleep(nanoseconds: UInt64(retryAfter) * 1_000_000_000); continue } - ``` -- **Verified:** `grep -n 'for attempt in' Lectern/Sources/LecternCore/Providers/*.swift` → - ``` - Lectern/Sources/LecternCore/Providers/AnthropicProvider.swift:80: for attempt in 0...1 - Lectern/Sources/LecternCore/Providers/GeminiImageProvider.swift:73: for attempt in 0..<3 - ``` - Gemini's arm, for contrast (`GeminiImageProvider.swift:96-98`): `let fallback = min(60, 2 * (1 << attempt))` — genuine exponential backoff. -- **Do:** Two defects in one function. (a) Only `.notConnectedToInternet` is translated; a `.timedOut` (against a 120-second ceiling), `.networkConnectionLost`, or `.cannotFindHost` propagates raw, is never retried, and reaches the user as a Foundation string via `describe`'s `localizedDescription` fallback. (b) The single retry uses a flat 2-second default with no exponentiation and no jitter. Lift `GeminiImageProvider`'s loop shape — 3 attempts, `min(60, 2 * (1 << attempt))`, `Retry-After` honored — into a shared `HTTPRetry` helper and use it from both, adding the transient `URLError` codes to the retryable set. -- **Why:** The single most expensive, longest-running call in the product has the weakest retry policy in the codebase, and a transient TCP reset discards a paid multi-thousand-token generation. -- **Effort:** M · **Impact:** L - -### 2. `max_tokens` is fixed at 8192 and truncation is never detected - -- **Location:** `Lectern/Sources/LecternCore/Providers/AnthropicProvider.swift:43` and `:69` -- **Proof:** - ```swift - let payload: [String: Any] = [ - "model": model, - "max_tokens": 8192, - "system": system, - "messages": [["role": "user", "content": user]], - "tools": [tool], - "tool_choice": ["type": "tool", "name": "emit_deck"], - ] - ``` -- **Verified:** `grep -rn 'stop_reason\|max_tokens\|slideCount' Lectern/Sources/LecternCore/Providers/AnthropicProvider.swift` → - ``` - 43: "max_tokens": 8192, - 69: "max_tokens": 8192, - 50: emit(.drafting(completed: 0, total: request.slideCount)) - 53: emit(.drafting(completed: request.slideCount, total: request.slideCount)) - ``` - `stop_reason` is never read anywhere in the target. -- **Do:** `slideCount` ranges to 40 and `includeNotes` adds a speaker-notes paragraph per slide, so a large deck can exceed 8192 output tokens. When it does, the API returns `stop_reason: "max_tokens"` with a partial `tool_use` input — which is still a *valid JSON object*, so `extractDeckJSON` accepts it and the validator sees a short-but-well-formed deck. Scale `max_tokens` from `request.slideCount` and `request.notes`, and read `stop_reason` in `send`, throwing a dedicated `.responseTruncated` so the pipeline's existing repair path can retry with a higher ceiling. -- **Why:** Asking for 40 slides can silently yield 22 with no warning anywhere in the UI — the validator's `requestedSlideCount` check is the only thing standing between this and a silently short deck. -- **Effort:** S · **Impact:** L - -### 3. Image generation fans out with no concurrency ceiling - -- **Location:** `Lectern/Sources/LecternCore/Providers/DeckGenerator.swift:120-131` -- **Proof:** - ```swift - await withTaskGroup(of: (String, Result).self) { group in - for (id, brief, aspect, role) in briefed { - group.addTask { - do { - let data = try await imageProvider.image(prompt: brief.prompt, style: style, - aspect: aspect, role: role) - return (id, .success(data)) - } catch { - return (id, .failure(error)) - } - } - } - ``` -- **Verified:** `grep -n 'maxConcurrent\|Semaphore\|prefix(\|chunked\|withThrowingTaskGroup' Lectern/Sources/LecternCore/Providers/DeckGenerator.swift` → `# (no matches)` -- **Do:** Every briefed slide gets a task immediately, so a 40-slide deck can fire 40 simultaneous 2K image requests at a provider whose per-minute limits are far below that. The result is self-inflicted 429s: `GeminiImageProvider` burns its three attempts on congestion this code created, and the failures surface as "N of M image(s) couldn't be generated". Use the standard bounded-group idiom — seed `min(4, briefed.count)` tasks, then add one more each time `next()` yields. -- **Why:** The illustration feature degrades exactly when it matters most (long decks), and it does so for a reason entirely within our control. -- **Effort:** S · **Impact:** L - -### 4. Decks are written non-atomically, so an interrupted save corrupts the file - -- **Location:** `Sources/Rostrum/Presentation/Presentation.swift:191-193` -- **Proof:** - ```swift - public func save(to url: URL) throws { - try serializedData().write(to: url) - } - } - ``` -- **Verified:** `grep -rn 'write(to:.*options\|\.atomic\|atomically' Sources/Rostrum/Presentation/Presentation.swift Lectern/Sources/LecternCore/Rendering/DeckRenderer.swift` → - ``` - Lectern/Sources/LecternCore/Rendering/DeckRenderer.swift:416: try presentation.save(to: url) - ``` - Only `DeckGenerator.keepRejectedDraft` uses `atomically: true` (`DeckGenerator.swift:65`) — the deck-save path does not. -- **Do:** `Data.write(to:)` without `.atomic` truncates the destination and streams into it. A crash, a full disk, or an iOS suspension mid-write leaves a truncated `.pptx` that PowerPoint will refuse to open, and if the path already held a deck the original is gone. Change to `try serializedData().write(to: url, options: .atomic)`. This is a one-line fix in the library that protects every consumer, and `keepRejectedDraft` already demonstrates the convention. -- **Why:** The `.pptx` on disk is described throughout the codebase as "the deliverable"; the last step of producing it should not be able to destroy a previous one. -- **Effort:** S · **Impact:** M - -### 5. A render failure discards a validated, fully paid-for deck - -- **Location:** `Lectern/Sources/LecternCore/Providers/DeckGenerator.swift:58-71` and `:213-224` -- **Proof:** - ```swift - private static func keepRejectedDraft(_ json: String, in directory: URL) -> URL? { - let url = directory.appendingPathComponent("rejected-draft.json") - do { - try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) - try json.write(to: url, atomically: true, encoding: .utf8) - return url - } catch { - return nil - } - } - ``` -- **Verified:** `grep -n 'keepRejectedDraft\|RenderError.renderFailed' Lectern/Sources/LecternCore/Providers/DeckGenerator.swift` → - ``` - 57: if let kept = Self.keepRejectedDraft(repaired.json, in: directory) { - 62: let url = directory.appendingPathComponent("rejected-draft.json") - 223: } catch let RenderError.renderFailed(underlying) { - ``` - The draft is preserved only on the *schema-invalid* path (line 57); the render-failure path at 223 rethrows and keeps nothing. -- **Do:** The instinct is already right and already documented — "Without it the only record of what the model actually sent is an error string." But it is applied to the one failure mode where the deck was *invalid*, not to the one where the deck was **valid and the render broke**. Persist the validated `DeckIR` JSON before calling `renderer.render`, and on `RenderError` keep it and offer a retry that skips straight to rendering. -- **Why:** A rendering bug — ours, not the model's — currently costs the user the entire generation fee with nothing recoverable. -- **Effort:** S · **Impact:** M - ---- - -## Security - -### 1. A 1,446-byte `.pptx` crashes the host process, defeating the zip limits entirely - -- **Location:** `Sources/Rostrum/XML/XML.swift:45` (the `Element` class) and `:137` (recursive `serialize`), reached from `Sources/Rostrum/Presentation/Presentation.swift:92` -- **Proof:** the library documents a throw: - ```swift - /// - Parse errors throw `RostrumError.xmlMalformed` with the parser's message - /// and line number. - public enum XML { - ``` - and the tree it builds is a chain of reference types with a recursive serializer and no depth bound anywhere: - ```swift - private func serialize(into out: inout String) { - out += "<" - out += name - ``` -- **Verified:** `grep -rn 'depth\|maxDepth\|nesting\|recursion' Sources/Rostrum/ --include='*.swift'` → - ``` - Sources/Rostrum/Presentation/Design.swift:188: "typography rationale", "layout system", "depth and hierarchy", "shape language", - ``` - The single match is a string literal in an unrelated design vocabulary list. No depth guard exists. - - Confirmed empirically end-to-end, through the public API, with the zip budget explicitly engaged: - ``` - $ ls -l bomb.pptx - -rw-r--r-- 1446 bomb.pptx # 40,000 nested elements, deflated - - $ ./xmldepth # Presentation(contentsOf:limits: .init(totalUncompressedBytes: 1_000_000)) - opening bomb.pptx with a 1 MB zip budget… - opened: 0 slides - Segmentation fault: 11 - exit=139 - ``` - Bisected to isolate the stage — parsing *succeeds*; the crash is the recursive ARC release of the element chain when the tree deallocates: - ``` - --- depth 5000 parseonly --- PARSE OK / DONE (tree about to deallocate) / EXITED CLEANLY / exit=0 - --- depth 20000 parseonly --- PARSE OK / DONE (tree about to deallocate) / Segmentation fault: 11 / exit=139 - ``` -- **Do:** Two unbounded recursions share one root cause: `Element` is a `final class` holding `children`, so both the compiler-synthesized deinit chain and `serialize(into:)` recurse once per level. Add a depth counter to `TreeBuilder.parser(_:didStartElement:...)` and throw `RostrumError.xmlMalformed` past a ceiling (Word and PowerPoint themselves cap around 100; a limit of 1,000 is generous and well under the ~20,000 crash threshold measured above). Then make teardown iterative — give `Element` an explicit `deinit` that walks the tree onto a worklist and releases breadth-first — and convert `serialize` to an explicit stack. Add the 40,000-deep fixture to `Tests/RostrumTests/FuzzTests.swift`, which today has no nesting-depth case. -- **Why:** This is a remotely triggerable, unrecoverable process kill on the library's central promise — safely reading a file someone else made — and it is entirely immune to the `ZipReader.Limits` hardening built specifically to stop hostile archives, because the payload is 1.4 KB. -- **Effort:** M · **Impact:** L - -### 2. The macOS app ships with neither App Sandbox nor Hardened Runtime - -- **Location:** `Lectern/project.yml:26-45` (the macOS target's `settings.base`) -- **Proof:** - ```yaml - settings: - base: - PRODUCT_BUNDLE_IDENTIFIER: com.lectern.app - MARKETING_VERSION: "1.0" - CURRENT_PROJECT_VERSION: "1" - GENERATE_INFOPLIST_FILE: "YES" - ASSETCATALOG_COMPILER_APPICON_NAME: AppIcon - SWIFT_VERSION: "6.0" - SWIFT_STRICT_CONCURRENCY: complete - INFOPLIST_KEY_LSApplicationCategoryType: public.app-category.productivity - INFOPLIST_KEY_NSHumanReadableCopyright: "" - ``` -- **Verified:** `grep -rn 'ENABLE_APP_SANDBOX\|ENABLE_HARDENED_RUNTIME\|com.apple.security' Lectern/ --include='*.yml' --include='*.entitlements' --include='*.plist'` → - ``` - Lectern/.build-xcode/Build/Intermediates.noindex/Lectern.build/Debug/Lectern.build/DerivedSources/Entitlements.plist:5: com.apple.security.get-task-allow - ``` - The only hit is a generated Debug artifact (`get-task-allow` is the debugger entitlement). `find Lectern -name '*.entitlements' -not -path '*/.build*'` → `Lectern/App/Lectern-iOS-Sim.entitlements` — iOS simulator only; the macOS target has no entitlements file at all. -- **Do:** The app reads user-selected PDFs, holds API keys in the login keychain, makes network calls to three vendors, and renders generated markup in a WebKit process — with no sandbox and no hardened runtime. `AppState.attachPDF` already calls `startAccessingSecurityScopedResource()` (`AppState.swift:218`), so the code is written *as if* sandboxed. Add an entitlements file with `com.apple.security.app-sandbox`, `files.user-selected.read-only`, and `network.client`, set `ENABLE_HARDENED_RUNTIME: "YES"`, and regenerate. Note this changes the keychain access story documented in `KeychainStore.swift:12-18` — a sandboxed app gets its own keychain partition, which actually *solves* the rebuild-instability problem described there. -- **Why:** Without hardened runtime the app cannot be notarized and cannot ship; without the sandbox, a WebKit or PDFKit parsing bug is an unconfined foothold on the user's Mac. -- **Effort:** M · **Impact:** L - -### 3. Untrusted-archive limits are opt-in rather than opt-out - -- **Location:** `Sources/Rostrum/Presentation/Presentation.swift:92` and `:102` -- **Proof:** - ```swift - public init(data: Data, limits: ZipReader.Limits = .unlimited) throws { - package = try OPCPackage.read(data: data, limits: limits) - let main = try package.mainDocumentPart() - ``` - and: - ```swift - public convenience init(contentsOf url: URL, limits: ZipReader.Limits = .unlimited) throws { - try self.init(data: Data(contentsOf: url), limits: limits) - } - ``` -- **Verified:** `grep -rn 'limits:' Sources/ Lectern/ Tools/ Examples/ --include='*.swift' | grep -v 'Zip/ZipReader.swift'` → - ``` - Sources/Rostrum/OPC/OPCPackage.swift:106: public static func read(data: Data, limits: ZipReader.Limits = .unlimited) throws -> OPCPackage { - Sources/Rostrum/Presentation/Presentation.swift:92: public init(data: Data, limits: ZipReader.Limits = .unlimited) throws { - Sources/Rostrum/Presentation/Presentation.swift:102: public convenience init(contentsOf url: URL, limits: ZipReader.Limits = .unlimited) throws { - Tools/pptx-tool/main.swift:47: deck = try Presentation(data: data, limits: .init(totalUncompressedBytes: budget)) - ``` - Exactly one of four call sites passes a budget. `.unlimited` is the default at all three API layers. -- **Do:** The `Limits` machinery is thoughtfully built and its documentation is candid about what it does and does not cover. The problem is purely the default: every caller who does not know the parameter exists is unprotected, and only `pptx-tool` knows. Flip the default to a generous concrete ceiling (say 2 GB declared uncompressed, which no legitimate deck approaches) and let callers opt *into* `.unlimited`. Separately, `Data(contentsOf: url)` at line 103 reads the whole file into memory before any limit applies — add `.mappedIfSafe` so a huge file does not become a huge allocation. -- **Why:** Security controls that must be discovered to be effective protect only the readers of the docs, and this library's whole premise is parsing files from elsewhere. -- **Effort:** S · **Impact:** M - -### 4. Failed drafts — including text derived from the user's private PDF — are written to disk in the clear and never cleaned up - -- **Location:** `Lectern/Sources/LecternCore/Providers/DeckGenerator.swift:61-70` -- **Proof:** - ```swift - private static func keepRejectedDraft(_ json: String, in directory: URL) -> URL? { - let url = directory.appendingPathComponent("rejected-draft.json") - do { - try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) - try json.write(to: url, atomically: true, encoding: .utf8) - return url - } catch { - return nil - } - } - ``` -- **Verified:** `grep -rn 'rejected-draft\|removeItem\|FileProtection\|completeFileProtection' Lectern/Sources/ Lectern/App/` → - ``` - Lectern/Sources/LecternCore/Providers/DeckGenerator.swift:61: let url = directory.appendingPathComponent("rejected-draft.json") - ``` - No deletion, no expiry, no file-protection attribute anywhere in the app. -- **Do:** The draft is the model's rendering of the user's prompt plus up to 40,000 characters lifted from a PDF they attached — which may be confidential. It lands at a fixed, predictable filename inside `decksDirectory()`, which on iOS is `Documents/Decks` with `UIFileSharingEnabled: YES` (`project.yml:88-90`), making it visible to the Files app and to Finder file sharing. Move it to a `Diagnostics` subdirectory excluded from file sharing, set `.completeFileProtection` on iOS, name it per-run rather than reusing one path, and delete drafts older than a few days on launch. Also surface a "Reveal diagnostic" affordance so the user knows it exists. -- **Why:** Confidential source material silently persists in a user-visible, file-shared folder with no retention policy and no way to know it is there. -- **Effort:** S · **Impact:** M - -### 5. Slide previews run in a WebKit view with JavaScript enabled by default - -- **Location:** `Lectern/App/SlidePreview.swift:48-56` -- **Proof:** - ```swift - @MainActor fileprivate func makeWebView() -> WKWebView { - let view = WKWebView() - #if os(iOS) - view.scrollView.isScrollEnabled = false - view.isOpaque = false - view.backgroundColor = .clear - #endif - return view - } - ``` -- **Verified:** `grep -rn 'allowsContentJavaScript\|WKWebViewConfiguration\|WKPreferences\|defaultWebpagePreferences' Lectern/App/` → `# (no matches)` - Escaping on the producing side *is* present and correct (`Sources/Rostrum/Presentation/SVGRenderer.swift:746-757` escapes `&`, `<`, `>` in text content, which is sufficient to prevent tag injection), so this is defense in depth rather than a live exploit. -- **Do:** `WKWebView()` uses a default configuration in which `defaultWebpagePreferences.allowsContentJavaScript` is `true`. The markup being loaded is assembled from LLM output that may itself be grounded in an attacker-supplied PDF, and it is loaded into the app's own WebKit context. Construct the view with a `WKWebViewConfiguration` that sets `allowsContentJavaScript = false`, and add a `WKNavigationDelegate` that cancels every navigation except the initial `loadHTMLString`. The comment at lines 19-22 correctly notes the nil `baseURL` removes network and file access — closing off script execution completes the argument. -- **Why:** One missed escape anywhere in a 780-line renderer becomes script execution inside the app rather than a broken thumbnail; the mitigation is three lines and costs nothing. -- **Effort:** S · **Impact:** M - ---- - -## Usability - -### 1. Icon-only buttons in the compose screen are unlabelled for VoiceOver - -- **Location:** `Lectern/App/ContentView.swift:178-179` -- **Proof:** - ```swift - Spacer() - Button { app.clearPDF() } label: { Image(systemName: "xmark.circle.fill") } - .buttonStyle(.plain).foregroundStyle(.secondary) - } - ``` -- **Verified:** `grep -n 'accessibility' Lectern/App/ContentView.swift` → `# (no matches)` - For contrast, the same author labelled the analogous controls elsewhere — `grep -rn 'accessibilityLabel' Lectern/App/` → - ``` - Lectern/App/SlidePreview.swift:108: .accessibilityLabel("Slide \(index + 1) of \(previews.count)") - Lectern/App/SlidePreview.swift:139: .accessibilityLabel("Slide \(index + 1) of \(previews.count)") - Lectern/App/StyleThumbnail.swift:84: .accessibilityLabel("\(style.name), \(style.badge)") - ``` - `ContentView.swift` — the app's largest and most-used view, 369 lines — has zero accessibility modifiers. -- **Do:** VoiceOver reads this button as "xmark circle fill". Add `.accessibilityLabel("Remove PDF")`. The same file has a second unlabelled icon control at `StylePickerSheet.swift:86` (the search-clear "xmark.circle.fill"). Sweep both files and add labels, then set `.accessibilityElement(children: .combine)` on each `Card` so the grouping reads as one unit rather than four fragments. -- **Why:** The PDF-grounding card is a primary flow, and its only destructive control is unreachable by name for VoiceOver users — in an app that ships four dedicated accessibility *styles* (`contrastink`, `largeprint`, `nightreader`). -- **Effort:** S · **Impact:** M - -### 2. The Mac app has no menu bar of its own - -- **Location:** `Lectern/App/LecternApp.swift:88-110` -- **Proof:** - ```swift - var body: some Scene { - WindowGroup("Lectern") { - ContentView() - .environment(app) - #if os(macOS) - .background(LaunchFrame()) - .onAppear { delegate.app = app } - #endif - } - #if os(macOS) - .defaultSize(width: 780, height: 1060) - #endif - - #if os(macOS) - Settings { - SettingsView() - .environment(app) - } - #endif - } - ``` -- **Verified:** `grep -rn '\.commands\|CommandGroup\|CommandMenu' Lectern/App/` → `# (no matches)` - The app's entire keyboard surface is two shortcuts: `grep -rn 'keyboardShortcut' Lectern/App/` → - ``` - Lectern/App/StylePickerSheet.swift:76: .keyboardShortcut(.defaultAction) - Lectern/App/ContentView.swift:226: .keyboardShortcut(.return, modifiers: .command) - ``` -- **Do:** The app inherits SwiftUI's default File/Edit/View menus, which are full of items that do nothing here (New Window, Print, Undo). Add a `.commands { }` block: replace `CommandGroup(.newItem)` with "New Deck ⌘N" wired to `app.reset()`, add "Choose Style… ⇧⌘S", "Open Decks Folder ⇧⌘O" (which also makes the Functionality item 2 gap survivable in the interim), and a `CommandGroup(replacing: .help)` pointing at the README. Delete the menu items that are inapplicable. -- **Why:** Against the Raycast anchor — a keyboard-first Mac utility — an app whose only shortcut is ⌘Return, with a File menu full of no-ops, reads as a prototype rather than a Mac app. -- **Effort:** S · **Impact:** M - -### 3. The failure screen is a dead end that discards the diagnosis - -- **Location:** `Lectern/App/ContentView.swift:357-369` -- **Proof:** - ```swift - struct FailedView: View { - @Environment(AppState.self) private var app - let message: String - var body: some View { - VStack(spacing: 16) { - Image(systemName: "exclamationmark.triangle.fill").font(.system(size: 44)).foregroundStyle(.orange) - Text(message).font(.title3).multilineTextAlignment(.center).frame(maxWidth: 420) - Button("Back to Compose") { app.reset() }.buttonStyle(.glassProminent).controlSize(.large) - } - .padding(48) - .frame(maxWidth: .infinity, maxHeight: .infinity) - } - } - ``` -- **Verified:** `grep -rn 'Retry\|Try again\|Open Settings\|retry' Lectern/App/` → `# (no matches)` -- **Do:** Every failure — a rate limit, a rejected key, a dropped connection, an unparseable draft — funnels into one button that goes back to the form. `AppState.describe` (lines 300-323) already knows exactly which error occurred, so the recovery can be specific: `.rateLimited` deserves a "Try again" with a countdown, `.authFailed` and `.noKey` deserve an "Open Settings" button, `.networkOffline` deserves a plain retry, and `.schemaInvalid` should offer "Show the rejected draft" (the file `keepRejectedDraft` already wrote, whose path is in the message text as unclickable prose). Pass the `LecternError` itself into `FailedView` rather than a flattened `String`. -- **Why:** The most common failure — a rate limit — currently costs the user their place in the flow and gives them no action beyond starting over. -- **Effort:** S · **Impact:** M - -### 4. The style gallery's search is a hand-rolled field with no platform behaviour - -- **Location:** `Lectern/App/StylePickerSheet.swift:81-90` -- **Proof:** - ```swift - HStack(spacing: 8) { - Image(systemName: "magnifyingglass").foregroundStyle(.secondary) - TextField("Search 150 styles by name or vibe", text: $query) - .textFieldStyle(.plain) - if !query.isEmpty { - Button { query = "" } label: { Image(systemName: "xmark.circle.fill").foregroundStyle(.tertiary) } - .buttonStyle(.plain) - } - } - .padding(.horizontal, 12).padding(.vertical, 9) - .background(.regularMaterial, in: .capsule) - ``` -- **Verified:** `grep -rn 'searchable\|FocusState\|focused\|submitLabel' Lectern/App/` → `# (no matches)` -- **Do:** Because it is a raw `TextField` rather than `.searchable`, the sheet opens with focus nowhere (the user must click before typing), ⌘F does nothing, Esc does not clear, there is no scope bar for the tag chips, and on iOS the keyboard has no search affordance. Replace with `.searchable(text: $query, placement: .toolbar, prompt: "Search 150 styles")` and add a `@FocusState` so the field is focused on presentation. Move the `pillTags` row into `.searchScopes` where it belongs. -- **Why:** Picking from 150 styles is the app's most differentiated interaction, and reaching its search currently requires taking your hands off the keyboard. -- **Effort:** S · **Impact:** M - -### 5. The generating screen announces nothing to VoiceOver and estimates nothing - -- **Location:** `Lectern/App/ContentView.swift:236-252` -- **Proof:** - ```swift - struct GeneratingView: View { - @Environment(AppState.self) private var app - var body: some View { - VStack(spacing: 18) { - ProgressView().controlSize(.large) - Text(app.stage).font(.title3.weight(.semibold)).contentTransition(.opacity) - if app.total > 0 { - ProgressView(value: Double(app.drafted), total: Double(app.total)) - .frame(maxWidth: 280) - Text("\(app.drafted) of \(app.total) \(app.progressNoun)").font(.callout).foregroundStyle(.secondary) - } - Button("Cancel", role: .cancel) { app.cancel() }.buttonStyle(.glass) - } - .padding(48) - .frame(maxWidth: .infinity, maxHeight: .infinity) - } - } - ``` -- **Verified:** `grep -n 'accessibilityValue\|announce\|AccessibilityNotification\|ETA\|estimated' Lectern/App/ContentView.swift` → `# (no matches)` -- **Do:** Nothing here is announced when it changes, so a VoiceOver user hears silence for the entire multi-minute generation and cannot tell progress from a hang. Add `.accessibilityElement(children: .combine)` with an `.accessibilityValue` derived from `stage` and `drafted/total`, and post an `AccessibilityNotification.Announcement` from `AppState.apply(_:)` on each stage change. Separately, `stage` transitions through eight named phases but shows no elapsed time and no estimate — `PriceTable` already models the deck's size, so a rough ETA is available. -- **Why:** The longest-running screen in the app is also its least communicative, and for a VoiceOver user it is entirely opaque. -- **Effort:** S · **Impact:** M - ---- - -## Attractiveness / Sexiness - -Anchor: **Raycast** — a single-window Mac utility that feels instant, teaches itself on first launch, and treats motion as feedback rather than decoration. - -### 1. A Liquid Glass app with a flat, legacy app icon - -- **Location:** `Lectern/App/Assets.xcassets/AppIcon.appiconset/Contents.json` -- **Proof:** - ```json - { - "images" : [ - { "idiom" : "mac", "scale" : "1x", "size" : "16x16", "filename" : "icon_16.png" }, - { "idiom" : "mac", "scale" : "2x", "size" : "16x16", "filename" : "icon_32.png" }, - ``` - …through to: - ```json - { "idiom" : "universal", "platform" : "ios", "size" : "1024x1024", "filename" : "icon_1024.png" } - ], - "info" : { "author" : "xcode", "version" : 1 } - } - ``` -- **Verified:** `find Lectern -name '*.icon' -not -path '*/.build*'` → `# (no matches)` - `ls Lectern/App/Assets.xcassets/AppIcon.appiconset/` → `Contents.json icon_1024.png icon_128.png icon_16.png icon_256.png icon_32.png icon_512.png icon_64.png` — seven flat PNGs, no layered source. -- **Do:** The targets deploy at macOS 26 / iOS 26 and the UI commits to Liquid Glass throughout (`.buttonStyle(.glass)`, `.glassProminent`, `.regularMaterial` cards). The icon is the one surface that did not come along: a flat pre-26 `.appiconset` gets none of the specular, depth, or tinted/clear/dark treatments the system now applies. Rebuild it in Icon Composer as a layered `.icon`, and set `ASSETCATALOG_COMPILER_APPICON_NAME` against it. -- **Why:** The icon is the first and most-repeated impression — in the Dock, in Spotlight, in the App Switcher — and it is currently the only part of the product that looks like it predates the OS it targets. -- **Effort:** M · **Impact:** M - -### 2. Phase changes cut hard, with no transition - -- **Location:** `Lectern/App/ContentView.swift:41-47` -- **Proof:** - ```swift - @ViewBuilder private var phaseView: some View { - switch app.phase { - case .compose: ComposeView() - case .generating: GeneratingView() - case .result(let r): ResultView(result: r) - case .failed(let m): FailedView(message: m) - } - } - ``` -- **Verified:** `grep -n 'animation\|transition\|withAnimation\|matchedGeometry' Lectern/App/ContentView.swift` → `# (no matches)` - The app does animate elsewhere — `grep -rn 'withAnimation\|\.animation(' Lectern/App/` → - ``` - Lectern/App/Theme.swift:88: withAnimation(.easeOut(duration: 0.2)) { image = loaded } - ``` - One animation, on thumbnail fade-in. -- **Do:** The four principal states of the app — the entire user journey — swap instantaneously, so pressing Generate replaces a full form with a spinner in a single frame, and the finished deck appears with the same abruptness. Wrap the switch in a `.animation(.smooth, value: app.phase)` and give each branch an asymmetric transition (compose pushes out, generating fades up, result scales in from the progress indicator). A `matchedGeometryEffect` from the Generate button to the progress ring would make the causal link explicit. -- **Why:** Against the Raycast anchor, state changes are where a native app earns its feeling of quality; four hard cuts make a carefully-built product feel like four screens bolted together. -- **Effort:** S · **Impact:** M - -### 3. First launch is a form you cannot submit - -- **Location:** the compose flow — `Lectern/App/ContentView.swift:213-232`, gated by `Lectern/App/AppState.swift:232-234` -- **Proof:** - ```swift - var canGenerate: Bool { - phase != .generating && hasKey && !prompt.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty - } - ``` - and the only guidance offered: - ```swift - if !app.hasKey { - Label("Add an API key in \(AppState.settingsHint) to generate", systemImage: "key") - .font(.callout).foregroundStyle(.secondary) - ``` -- **Verified:** `grep -rn 'onboard\|firstRun\|welcome\|hasLaunched\|AppStorage' Lectern/App/` → `# (no matches)` -- **Do:** A new user opens a 780×1060 window showing five cards, a dimmed Generate button, and a grey line of text naming a menu item they must find themselves — and the label is not even a button. Build a first-run state: a single welcoming panel that explains what Lectern does, links straight to the key field (make that label a `Button` that opens Settings at minimum), and shows two or three of the best style thumbnails as a preview of what they are buying. Gate it on an `@AppStorage("hasCompletedOnboarding")` flag. -- **Why:** The first run is the only moment where every user is guaranteed to be present, and it currently presents a locked door with the key described in small grey text. -- **Effort:** M · **Impact:** L - -### 4. The longest screen in the app is an indeterminate spinner - -- **Location:** `Lectern/App/ContentView.swift:239-248` -- **Proof:** - ```swift - VStack(spacing: 18) { - ProgressView().controlSize(.large) - Text(app.stage).font(.title3.weight(.semibold)).contentTransition(.opacity) - if app.total > 0 { - ProgressView(value: Double(app.drafted), total: Double(app.total)) - .frame(maxWidth: 280) - Text("\(app.drafted) of \(app.total) \(app.progressNoun)").font(.callout).foregroundStyle(.secondary) - } - Button("Cancel", role: .cancel) { app.cancel() }.buttonStyle(.glass) - } - ``` -- **Verified:** `grep -n 'GenerationEvent\|case .outlining\|case .auditing' Lectern/App/AppState.swift` → - ``` - 289: private func apply(_ event: GenerationEvent) { - 291: case .preparingSource: stage = "Reading source" - 292: case .outlining: stage = "Outlining" - 293: case .outlineReady: stage = "Outline ready" - 294: case .drafting(let c, let t): stage = "Writing slides"; drafted = c; total = t; progressNoun = "slides" - 295: case .validating: stage = "Validating" - 296: case .repairing: stage = "Repairing" - 297: case .auditing: stage = "Polishing (QA pass)" - 298: case .illustrating(let c, let t): stage = "Generating images"; drafted = c; total = t; progressNoun = "images" - 299: case .rendering: stage = "Rendering .pptx" - 300: case .finished: stage = "Done" - ``` - Ten richly-named stages arrive at the UI and are rendered as one line of text above a generic spinner. -- **Do:** The pipeline emits a genuinely interesting narrative — outlining, drafting, validating, repairing, polishing, illustrating, rendering — and the UI flattens it into a `ProgressView()`. Show the stages as a vertical checklist that fills in as each completes, and — the real prize — `outlineReady` already carries the actual `DeckOutline` with its title and section names, which is currently *discarded* (line 293 sets only a string). Reveal the outline as it lands, then let slide thumbnails populate as they render. -- **Why:** A multi-minute wait is the app's biggest engagement risk and its biggest opportunity; the data for a compelling progressive reveal is already flowing and being thrown away one line short of the screen. -- **Effort:** M · **Impact:** L - -### 5. The result screen buries its payoff under four collapsible warning drawers - -- **Location:** `Lectern/App/ContentView.swift:290-348` -- **Proof:** - ```swift - if !result.warnings.isEmpty { - DisclosureGroup("\(result.warnings.count) validation warning(s)") { - ``` - …followed in sequence by: - ```swift - if !result.droppedContent.isEmpty { - DisclosureGroup("\(result.droppedContent.count) slide(s) lost content to layout limits") { - ``` - ```swift - if !result.schemaIssues.isEmpty { - DisclosureGroup("\(result.schemaIssues.count) schema issue(s) in the written deck") { - ``` - ```swift - if !result.unmeasuredFonts.isEmpty { - DisclosureGroup("\(result.unmeasuredFonts.count) font(s) not installed") { - ``` -- **Verified:** `grep -c 'DisclosureGroup' Lectern/App/ContentView.swift` → `4` - All four sit inside the same `VStack` beneath the action row, each `.frame(maxWidth: 420)`. -- **Do:** The taxonomy behind these four buckets is genuinely excellent and the comments justifying the separation are the best in the file — but the user's moment of delight is "my deck is ready", and it arrives stacked under up to four grey accordions of caveats. Collapse them into a single "Details" affordance with a quiet inline badge, let the contact sheet take the full height, and promote Open/Share to the visual anchor. Keep the four categories intact *inside* the details panel, where their precision is a feature rather than an apology. -- **Why:** This is the screen the entire product exists to reach; measured against the Raycast anchor it should feel like an arrival, not a lint report. -- **Effort:** S · **Impact:** M - ---- - -## First move - -**A 1,446-byte `.pptx` crashes the host process, defeating the zip limits entirely** (from Security) - -Ship this first because it is the only finding in the report that is *proven*, not argued — a 1,446-byte file, opened through the documented public API with the security budget explicitly engaged, terminates the process with SIGSEGV, and the bisection shows exactly why (recursive ARC teardown of the `XML.Element` chain, crossing the stack limit somewhere between depth 5,000 and 20,000). Everything else here is a judgement call about priorities; this is a reproducible crash. It also matters disproportionately because of what Rostrum is: a library whose entire value proposition is safely reading files that someone else made, which documents that malformed input *throws* `RostrumError.xmlMalformed`, and which has invested real care in exactly this threat model — the `ZipReader.Limits` struct, the zip64 count bounding, the coordinate-overflow clamp at `intAttr`, the explicit `shouldResolveExternalEntities = false`. This bug walks straight past all of it, because the payload is 1.4 KB and the limits only bound *declared uncompressed bytes*. Every downstream consumer inherits the hole, including Lectern, which today is unsandboxed (Security item 2) and so hands an attacker an unconfined crash. The fix is well-bounded and testable in a single sitting: a depth counter in `TreeBuilder`, an iterative `deinit` and an iterative `serialize`, and a nesting-depth case added to `FuzzTests.swift`, which currently has none. Do that, then take Reliability item 3 (bound the image fan-out) as the fast follow, since it is an afternoon's work and removes a failure mode the product inflicts on itself. - -## Dropped during verification - -- **A cancel during the QA pass is swallowed and the deck renders anyway** — cited code does something else. `try? await provider.revise(...)` (`DeckGenerator.swift:81`) *does* convert `CancellationError` into `nil`, and `DeckGenerator.swift` itself has no cancellation checks. But re-reading the renderer showed the stage below it is thoroughly guarded: `grep -rn 'Task.isCancelled\|checkCancellation' Lectern/Sources/LecternCore/` → - ``` - Lectern/Sources/LecternCore/Rendering/DeckRenderer.swift:331: try Task.checkCancellation() - Lectern/Sources/LecternCore/Rendering/DeckRenderer.swift:341: try Task.checkCancellation() - Lectern/Sources/LecternCore/Rendering/DeckRenderer.swift:414: try Task.checkCancellation() - Lectern/Sources/LecternCore/Rendering/DeckRenderer.swift:423: previews: Task.isCancelled ? [] : Self.previews(of: presentation), - ``` - with a comment at 326-330 describing precisely the symptom I was going to claim — "Without these the user is returned to Compose and then, a few seconds later, thrown into a Result screen for the deck they just cancelled, with the file already written." No file is written after a cancel; the checks at 331 and 414 see to that, and `illustrate`'s task-group children inherit cancellation. The residue is cosmetic (cancelled image requests are reported as image *failures* rather than as a cancellation), which does not belong in a top-5. -- **XXE / external entity resolution in the XML parser** — already in place: `grep -n 'shouldResolveExternalEntities' Sources/Rostrum/XML/XML.swift` → `193: parser.shouldResolveExternalEntities = false`. Explicitly disabled. -- **Integer overflow trap on coordinates parsed from a hostile deck** — already in place. `SVGRenderer.intAttr` routes every file-derived coordinate through a bounded `coordinate(_:)`, with a comment naming this exact risk: "Swift's `+` traps on overflow. Bounding at the single point where file bytes become numbers is what makes all of that arithmetic safe." -- **`try!` in `Inflate.fixedTables` is a latent crash** — cited code is provably safe: the two calls at `Inflate.swift:286-287` construct RFC 1951 §3.2.6 fixed tables from compile-time-constant lengths. These are the only two `try!` in 16,179 lines and the accompanying comment ("well-formed by construction; failure is impossible") is correct. -- **Unescaped model text reaching the SVG preview enables XSS** — already in place: `SVGRenderer.swift:746-757` escapes `&`, `<`, `>` on every text run, and `colorHex` is validated rather than interpolated raw (comment at line 705). Reduced to the defense-in-depth JavaScript finding (Security item 5) rather than an active vulnerability. -- **Force-unwraps and crash operators across the app layer** — no occurrences: `grep -rn 'try!\|as! \|fatalError\|\.first!\|\.last!' Lectern/App Lectern/Sources` → `# (no matches)`. -- **Unhandled TODO/FIXME debt** — effectively none: the only repo-wide match is `ZipWriter.swift:160`, a doc comment describing an *unimplemented case in the OOXML spec*, not deferred work. -- **`Presentation.save` leaks partial state on failure / no `documentKind` guard** — mislocated. The guard exists at `Presentation.swift:95-98` (`throw RostrumError.notAPresentation`), and it fired correctly during my own exploit development, forcing me to set a valid PresentationML content type before the crash was reachable. - -## Deferred - -- **CI runs three jobs on macOS hosted runners** (`.github/workflows/ci.yml:27` `runs-on: macos-15`, `:51` `runs-on: macos-26`) — outside the seven lenses, but worth acting on: hosted macOS minutes bill at 10× Linux, and this workflow triggers on **every push to every branch** (`on: push` with no branch filter). The Linux job already covers `swift build` + `swift test` on 6.0/6.1. Consider making the macOS and `xcodebuild` jobs manual or `main`-only and verifying Apple-platform builds locally. -- **`AppState.task` is never cleared after a successful run** (`AppState.swift:286`) — `task = nil` happens only in `cancel()`. Harmless today (a completed `Task` releases its closure), but it makes the lifecycle harder to reason about; fold into the `generationID` work in Stability item 1. -- **`DeckRenderer.swift` is 907 lines** — nearly twice the next-largest Lectern file and the single place where IR, layout, furniture, fonts, charts and previews all meet. Not a defect, but the obvious next split (previews and font resolution are both self-contained). -- **No snapshot or golden-file tests for `SVGRenderer`** — `Tests/RostrumTests/SVGRendererTests.swift` is 281 lines of structural assertions; visual regressions in the preview path would pass silently. -- **`Examples/` and `Tools/` were not audited** — four executable targets plus `extract-schema.py` sit outside the surveyed set. diff --git a/lift-up-plan-20260809.md b/lift-up-plan-20260809.md deleted file mode 100644 index 21f41763..00000000 --- a/lift-up-plan-20260809.md +++ /dev/null @@ -1,582 +0,0 @@ -# Lift-Up Plan: Rostrum + Lectern - -> Platform: mixed (macOS 26 + iOS 26 app on a macOS 13 / iOS 16 library) -> Surveyed: 2026-08-09 -> Coverage: full for `Lectern/App/`, `Lectern/Sources/LecternCore/`; partial for `Sources/Rostrum/` (Presentation/, Zip/, XML/, OPC/, Charts/, Fonts/ read; `Tools/` and `Examples/` not audited) -> Attractiveness anchor: inferred — **Raycast** (the macOS-native benchmark for a single-window utility that must feel instant and premium). Not user-supplied. -> Model tier: Opus 5 (frontier) — no rerun needed. - -Verification was unusually punishing on this codebase: 11 of ~34 candidates were -dropped, most because the "missing" guard was already there. Several dimensions -therefore ship fewer than 5 items rather than padded ones. That is a signal about -Rostrum's quality, not about the audit. - -## Performance - -### 1. The contact sheet builds one `WKWebView` per slide - -- **Location:** `Lectern/App/SlidePreview.swift:120` -- **Proof:** - ```swift - LazyVGrid(columns: columns, spacing: 14) { - ForEach(Array(previews.enumerated()), id: \.offset) { index, svg in - SlidePreview(svg: svg) - .aspectRatio(16.0 / 9.0, contentMode: .fit) - ``` - `SlidePreview` is a `WKWebView` (`makeWebView` → `WKWebView(frame:configuration:)`), - so each tile is a full web content process. -- **Verified:** `grep -n 'WKWebView\|NSViewRepresentable\|UIViewRepresentable' Lectern/App/SlidePreview.swift` → - ``` - 2:import WebKit - 62: @MainActor fileprivate func makeWebView(_ coordinator: Coordinator) -> WKWebView { - 69: let view = WKWebView(frame: .zero, configuration: config) - 86:extension SlidePreview: NSViewRepresentable { - 87: func makeNSView(context: Context) -> WKWebView { makeWebView(context.coordinator) } - ``` -- **Do:** Rasterize each SVG once to a `CGImage` off the main actor and show `Image` - in the grid; keep the `WKWebView` only for a single full-size detail view. On macOS - the SVG can go through `NSImage`-free `CGImageSourceCreateWithData` after a one-shot - WebKit snapshot (`takeSnapshot(with:)`), cached by slide index. -- **Why:** A 60-slide deck currently spawns dozens of web content processes while - scrolling; each carries its own JS-disabled renderer and several MB of RSS. This is - the single biggest reason the inspector feels heavy on real decks. -- **Effort:** M · **Impact:** L - -### 2. Every slide is rendered to SVG before the inspector appears - -- **Location:** `Lectern/Sources/LecternCore/Inspection/DeckInspection.swift:149` -- **Proof:** - ```swift - public static func inspect(deckAt url: URL, - renderPreviews: Bool = true, - limits: ZipReader.Limits = - .init(totalUncompressedBytes: DeckInspector.defaultReadLimit), - onEvent: (Event) -> Void = { _ in }) throws -> DeckInspection { - ``` -- **Verified:** `grep -n 'renderPreviews' Lectern/Sources/LecternCore/Inspection/DeckInspection.swift Lectern/App/AppState.swift` → - ``` - Lectern/Sources/LecternCore/Inspection/DeckInspection.swift:146: /// - Parameter renderPreviews: pass `false` to skip the slowest step when - Lectern/Sources/LecternCore/Inspection/DeckInspection.swift:149: renderPreviews: Bool = true, - Lectern/Sources/LecternCore/Inspection/DeckInspection.swift:178: if renderPreviews { - ``` - No hit in `AppState.swift` — the call site takes the default, so previews are always eager. -- **Do:** Render previews lazily per visible tile (or cap the eager pass at the first - ~12 slides and stream the rest), driven by the existing `.rendering(done:total:)` - event. `AppState.inspect(deckAt:)` should pass `renderPreviews: false` and fault - tiles in from the contact sheet. -- **Why:** The user's own library has decks of 84–96 MB; every one of those pays full - SVG rasterization of every slide — including base64-inlined media — before a single - fact appears on screen. Time-to-first-content is currently bounded by the slowest step. -- **Effort:** M · **Impact:** L - -### 3. The deck library re-scans the directory on every appearance - -- **Location:** `Lectern/Sources/LecternCore/Storage/DeckLibrary.swift:36` -- **Proof:** - ```swift - guard let entries = try? fileManager.contentsOfDirectory( - at: directory, includingPropertiesForKeys: keys, - options: [.skipsHiddenFiles, .skipsSubdirectoryDescendants]) else { return [] } - return entries.compactMap { url -> DeckFile? in - guard isDeck(url) else { return nil } - ``` -- **Verified:** `grep -rn 'refreshLibrary()' Lectern/App/` → - ``` - Lectern/App/ContentView.swift:133: .task { app.refreshLibrary() } - Lectern/App/DeckLibrarySheet.swift:57: .task { app.refreshLibrary() } - ``` -- **Do:** Cache the listing in `AppState` and invalidate on write/delete, or watch the - directory with a `DispatchSource` file-system observer. Keep the rescan as the - fallback path only. -- **Why:** Two `.task` sites re-stat 29+ files (several ~90 MB) each time Home appears - or the sheet opens, on the main actor's behalf. It is invisible today and will not - stay invisible as the library grows. -- **Effort:** S · **Impact:** M - -### 4. Font files are re-read and re-parsed on every render - -- **Location:** `Lectern/Sources/LecternCore/Rendering/DeckRenderer.swift:185` -- **Proof:** - ```swift - guard let url = installedFontFile(named: name) ?? officeFontFile(named: name), - let data = try? Data(contentsOf: url), - let face = familyCandidates(for: name) - .lazy.compactMap({ faceIndex(named: $0, in: data) }).first, - ``` -- **Verified:** `grep -n 'cache\|Cache\|memo' Lectern/Sources/LecternCore/Rendering/DeckRenderer.swift` → `# (no matches)` -- **Do:** Memoize `name → (URL, faceIndex, Data)` in a `static let` actor-isolated cache - keyed by font name; font files do not change during a run. -- **Why:** `Data(contentsOf:)` on a font file is tens of MB for large families, repeated - per render pass and per face candidate. -- **Effort:** S · **Impact:** M - -Four items. A fifth Performance candidate (`XML.textContent` string concatenation) -was dropped — see *Dropped during verification*. - -## Functionality - -### 1. Two of the three text providers are selectable but throw - -- **Location:** `Lectern/Sources/LecternCore/Providers/ProviderFactory.swift:20` -- **Proof:** - ```swift - switch id { - case .anthropic: - return AnthropicProvider(apiKey: key, model: model) - case .openAI, .gemini, .custom: - throw LecternError.providerError(status: 0, message: "\(id.rawValue) isn't wired up yet — use Anthropic.") - } - ``` -- **Verified:** `grep -n 'ProviderID.allCases\|isWired' Lectern/App/SettingsView.swift` → - ``` - 44: ForEach(ProviderID.allCases, id: \.self) { id in - 62: .disabled(!ProviderFactory.isWired(app.providerID)) - 179: if !ProviderFactory.isWired(app.providerID) { - ``` -- **Do:** Either implement `OpenAIProvider` behind the existing `LLMProvider` protocol - (the image providers already prove the shape), or remove `.openAI`/`.gemini`/`.custom` - from the Settings picker until they exist. -- **Why:** The picker advertises three choices and honours one. The app degrades - politely, but a settings screen that lists capabilities it does not have is a promise - the product breaks on first use. -- **Effort:** L · **Impact:** M - -### 2. Decks can be created and deleted but never renamed - -- **Location:** `Lectern/Sources/LecternCore/Storage/DeckLibrary.swift` -- **Proof:** - ```swift - public static func delete(_ deck: DeckFile, - fileManager: FileManager = .default) throws { - try fileManager.removeItem(at: deck.url) - } - ``` -- **Verified:** `grep -rn 'rename\|Rename' Lectern/App/DeckLibrarySheet.swift Lectern/Sources/LecternCore/Storage/DeckLibrary.swift` → `# (no matches)` -- **Do:** Add `DeckLibrary.rename(_:to:)` doing a collision-checked `moveItem`, and wire - it to an inline `TextField` rename in `DeckRow` (double-click / return-to-commit). -- **Why:** Deck names are model-generated slugs like - `paperbanana-automating-academic-illustration-for-ai-scientis-2-rebranded`. The one - affordance a user needs most for their own archive is the one missing. -- **Effort:** S · **Impact:** M - -### 3. The inspector's slide tiles are not linked to their text - -- **Location:** `Lectern/App/InspectorView.swift:55` -- **Proof:** - ```swift - if !inspection.previews.isEmpty { - Card(title: "SLIDES", systemImage: "rectangle.on.rectangle") { - SlideContactSheet(previews: inspection.previews, - titles: inspection.previewTitles) - .frame(minHeight: 260) - } - } - ``` -- **Verified:** `grep -n 'ScrollViewReader\|scrollTo\|onTapGesture' Lectern/App/InspectorView.swift` → `# (no matches)` -- **Do:** Wrap the inspector `ScrollView` in a `ScrollViewReader`, give each slide block - in `textCard` an `.id(slide.number)`, and make a contact-sheet tile tap scroll to it. -- **Why:** The two halves of the inspector describe the same slides and cannot refer to - each other; finding the words for the tile you are looking at means scrolling and counting. -- **Effort:** S · **Impact:** M - -Three items. Candidates 4 and 5 (`ChartReader.setFormula` unused; missing chart-kind -coverage) were dropped — see *Dropped during verification*. - -## Stability - -### 1. Section writing swallows its own errors - -- **Location:** `Lectern/Sources/LecternCore/Rendering/DeckRenderer.swift:890` -- **Proof:** - ```swift - distinct.insert((name: opening?.isEmpty == false ? opening! : "Opening", startSlide: 0), - at: 0) - try? presentation.setSections(distinct) - ``` -- **Verified:** `grep -n 'try?' Lectern/Sources/LecternCore/Rendering/DeckRenderer.swift | wc -l` → `14` -- **Do:** Capture the failure into the render's existing `warnings` array rather than - discarding it: `do { try presentation.setSections(distinct) } catch { warnings.append(...) }`. -- **Why:** A deck that silently loses its section structure looks correct in Lectern and - wrong in PowerPoint, with no signal anywhere that a step failed. -- **Effort:** S · **Impact:** M - -### 2. `PackURI` traps rather than throws on a malformed path - -- **Location:** `Sources/Rostrum/OPC/PackURI.swift:19` -- **Proof:** - ```swift - public init(_ value: String) { - precondition(value.hasPrefix("/"), "pack URI must be absolute: \(value)") - ``` -- **Verified:** `grep -rn 'PackURI(' Sources/Rostrum/OPC/OPCPackage.swift | head -3` → - ``` - Sources/Rostrum/OPC/OPCPackage.swift:118: let uri = PackURI("/" + entry.name) - Sources/Rostrum/OPC/OPCPackage.swift:263: let uri = PackURI("/" + entry.name) - ``` -- **Do:** The two package-read call sites prepend `/` so they cannot trip the - precondition today — that is load-bearing and undocumented. Add a failable - `PackURI(validating:)` used at every boundary that consumes zip entry names, and note - the invariant at the call sites. -- **Why:** The precondition is one refactor away from becoming a crash on a hostile - archive: it is the only thing between an attacker-controlled entry name and a trap. -- **Effort:** S · **Impact:** M · `speculative` (no current reachable path; the anchor is - the untrusted-input adjacency, not a live bug) - -Two items. Six Stability candidates were dropped as provably safe — this is the -strongest dimension in the codebase and the report says so rather than inventing three -more. See *Dropped during verification*. - -## Reliability - -### 1. The retry deadline only gates *starting* an attempt - -- **Location:** `Lectern/Sources/LecternCore/Providers/HTTPRetry.swift:84` -- **Proof:** - ```swift - static func hasTimeToRetry(startedAt: Date, nextWait: Int, now: Date = Date()) -> Bool { - now.addingTimeInterval(TimeInterval(nextWait)).timeIntervalSince(startedAt) < overallDeadline - } - ``` -- **Verified:** `grep -n 'overallDeadline\|timeoutInterval' Lectern/Sources/LecternCore/Providers/HTTPRetry.swift Lectern/Sources/LecternCore/Providers/AnthropicProvider.swift` → - ``` - Lectern/Sources/LecternCore/Providers/HTTPRetry.swift:82: static let overallDeadline: TimeInterval = 180 - Lectern/Sources/LecternCore/Providers/AnthropicProvider.swift:149: var req = URLRequest(url: endpoint, timeoutInterval: 600) - ``` -- **Do:** Wrap the whole retry loop in a `Task` with a deadline, or lower the per-request - `timeoutInterval` so `attempts × timeout` cannot exceed `overallDeadline`. -- **Why:** A request started at t=179 s with a 600 s socket timeout can hold the - "generating" screen for ten minutes against a 180 s stated ceiling. The user's only - exit is Cancel. -- **Effort:** S · **Impact:** M - -### 2. Image failures silently downgrade a paid deck - -- **Location:** `Lectern/Sources/LecternCore/Providers/DeckGenerator.swift:180` -- **Proof:** - ```swift - case .failure(let error): failures.append(DeckGenerator.imageFailure(error)) - } - var warnings: [String] = discarded - if !failures.isEmpty { - ``` -- **Verified:** `grep -n 'failures.isEmpty\|warnings.append' Lectern/Sources/LecternCore/Providers/DeckGenerator.swift | head -4` → - ``` - 182: if !failures.isEmpty { - ``` -- **Do:** Distinguish "some images failed" from ordinary warnings in `DeckResult`, and - offer a "Retry missing images" action on `ResultView` that regenerates only the failed - briefs rather than the whole deck. -- **Why:** Every image is a paid call. Today a half-illustrated deck is reported in the - same disclosure group as a schema nit, and the only remedy offered is regenerating - everything from scratch. -- **Effort:** M · **Impact:** M - -Two items; three Reliability candidates were dropped as already-handled. - -## Security - -### 1. Grounding text from a PDF is interpolated straight into the prompt - -- **Location:** `Lectern/Sources/LecternCore/Providers/PromptTemplates.swift:110` -- **Proof:** - ```swift - if let grounding = request.groundingText, !grounding.isEmpty { - parts.append("Ground every factual claim in the SOURCE MATERIAL below; do not invent statistics.\n\n" - + "--- SOURCE MATERIAL ---\n\(grounding)") - } - return parts.joined(separator: "\n\n") - ``` -- **Verified:** `grep -rn 'sanitiz\|escape\|injection' Lectern/Sources/LecternCore/Providers/PromptTemplates.swift Lectern/App/PDFGrounding.swift` → `# (no matches)` -- **Do:** Move grounding into its own message turn rather than concatenating it into the - instruction block, delimit it with a nonce fence, and state in the system prompt that - source material is data and never instructions. -- **Why:** The PDF is frequently someone else's document. Text inside it can currently - redirect a paid generation — including the QA pass that reviews the result. -- **Effort:** S · **Impact:** M - -### 2. Rejected drafts persist unencrypted outside iOS - -- **Location:** `Lectern/Sources/LecternCore/Providers/DeckGenerator.swift:75` -- **Proof:** - ```swift - try Data(json.utf8).write(to: url, options: [.atomic, .completeFileProtection]) - #else - try Data(json.utf8).write(to: url, options: .atomic) - ``` -- **Verified:** `grep -rn 'completeFileProtection' Lectern/Sources/LecternCore/` → - ``` - Lectern/Sources/LecternCore/Providers/DeckGenerator.swift:75: try Data(json.utf8).write(to: url, options: [.atomic, .completeFileProtection]) - ``` - Single occurrence — the macOS branch has no equivalent. -- **Do:** On macOS, write rejected drafts to a directory with owner-only POSIX - permissions (`0o700`) and prune on a timer, or keep them in memory unless a debug - flag is set. -- **Why:** A rejected draft contains the user's full prompt and any grounding excerpts, - left at rest in a readable location on a multi-user Mac. -- **Effort:** S · **Impact:** M - -Two items. Three Security candidates were dropped as already mitigated — notably the -untrusted-file read budget and the preview sandbox, both already correct. - -## Usability - -### 1. Every control on the compose form is invisible to VoiceOver - -- **Location:** `Lectern/App/ContentView.swift:253` -- **Proof:** - ```swift - Card(title: "PROMPT", systemImage: "text.alignleft") { - TextEditor(text: $app.prompt) - .font(.body).scrollContentBackground(.hidden) - .frame(minHeight: 120) - .overlay(alignment: .topLeading) { - if app.prompt.isEmpty { - Text("What is this presentation about, and what should it accomplish?") - .foregroundStyle(.tertiary).allowsHitTesting(false).padding(.top, 2) - } - } - } - ``` - The `Card` title is decoration; the placeholder is a non-hit-testable overlay. Neither - reaches the accessibility tree. -- **Verified:** `grep -n 'accessibilityLabel' Lectern/App/ContentView.swift` → - ``` - 246: .accessibilityLabel("Dismiss") - 328: .accessibilityLabel("Remove PDF") - ``` - Only two, both on icon buttons elsewhere. Confirmed empirically against the live app's - accessibility tree, which reports the prompt as a bare `AXTextArea` with no description - and the audience picker as `AXPopUpButton Value: General` with no label. -- **Do:** Give `TextEditor` an `.accessibilityLabel("Prompt")`, replace `Picker("")` + - `.labelsHidden()` with real labels (`Picker("Audience", …)`) kept visually hidden via - `.accessibilityLabel` rather than erased, and label the `Stepper` and `Toggle` groups. -- **Why:** The entire primary task of the app — describing a deck — is unusable with - VoiceOver. The visual `Card` headings carry all the meaning and none of it is exposed. -- **Effort:** S · **Impact:** L - -### 2. A 29-deck library has no search - -- **Location:** `Lectern/App/DeckLibrarySheet.swift:35` -- **Proof:** - ```swift - List { - ForEach(app.library) { deck in - DeckRow(deck: deck, - open: { open(deck) }, - ``` -- **Verified:** `grep -rn 'searchable' Lectern/App/ Lectern/Sources/` → `# (no matches)` -- **Do:** Add `.searchable(text:)` over the library list filtering on deck name, and a - sort control (name / date / size). `StylePickerSheet` already implements exactly this - pattern for styles and can be copied. -- **Why:** The library is the only route back to decks that cost real money to make, and - the names are long model-generated slugs. Finding one is currently linear scanning. -- **Effort:** S · **Impact:** M - -### 3. You cannot drop a deck on the window to inspect it - -- **Location:** `Lectern/App/ContentView.swift:350` -- **Proof:** - ```swift - .dropDestination(for: URL.self) { urls, _ in - guard let url = urls.first(where: { $0.pathExtension.lowercased() == "pdf" }) else { return false } - Task { await app.attachPDF(url) } - return true - } isTargeted: { dropTargeted = $0 } - ``` -- **Verified:** `grep -rn 'dropDestination' Lectern/App/` → - ``` - Lectern/App/ContentView.swift:350: .dropDestination(for: URL.self) { urls, _ in - ``` - The only drop target in the app, scoped to `ComposeView` and to `.pdf`. -- **Do:** Add a `.dropDestination` at the `HomeView` (and window) level accepting - `pptx`/`potx`/`ppsx` that calls `app.inspect(deckAt:)`, with a highlighted drop state. -- **Why:** "Open a deck" is half the product, and the most natural macOS gesture for it - does nothing. The app already registers those UTTypes for the file importer. -- **Effort:** S · **Impact:** M - -### 4. Hero glyphs are fixed-size and ignore Dynamic Type - -- **Location:** `Lectern/App/ContentView.swift:96` -- **Proof:** - ```swift - Image(systemName: "rectangle.on.rectangle.angled") - .font(.system(size: 44)).foregroundStyle(.tint) - Text("Lectern").font(.largeTitle.weight(.semibold)) - ``` -- **Verified:** `grep -rn 'font(.system(size:' Lectern/App/*.swift` → - ``` - Lectern/App/ContentView.swift:96: .font(.system(size: 44)).foregroundStyle(.tint) - Lectern/App/ContentView.swift:143: Image(systemName: systemImage).font(.system(size: 28)) - Lectern/App/ContentView.swift:437: Image(systemName: "checkmark.seal.fill").font(.system(size: 52)).foregroundStyle(.green) - Lectern/App/ContentView.swift:524: Image(systemName: "exclamationmark.triangle.fill").font(.system(size: 44)).foregroundStyle(.orange) - Lectern/App/StyleThumbnail.swift:39: .font(.system(size: 12, weight: .semibold)) - Lectern/App/StyleThumbnail.swift:59: .font(.system(size: 9, weight: .semibold)) - ``` -- **Do:** Replace with relative sizing — `.font(.system(size: 44, relativeTo: .largeTitle))` - or `.imageScale(.large)` on a text-style font — so the glyphs track the user's setting. -- **Why:** At larger accessibility text sizes the labels grow and the icons do not, which - breaks the visual hierarchy of exactly the two screens that carry the product's identity. -- **Effort:** S · **Impact:** S - -### 5. The window has a default size but no resizability contract - -- **Location:** `Lectern/App/LecternApp.swift:52` -- **Proof:** - ```swift - .defaultSize(width: 780, height: 1060) - ``` -- **Verified:** `grep -rn 'windowResizability\|defaultSize\|WindowGroup' Lectern/App/LecternApp.swift` → - ``` - 40: WindowGroup("Lectern") { - 52: .defaultSize(width: 780, height: 1060) - ``` -- **Do:** Add `.windowResizability(.contentMinSize)` so the 640×560 `minWidth/minHeight` - already declared in `ContentView` is actually enforced by the window. -- **Why:** The content declares a minimum the window does not honour; dragging small - produces clipped controls rather than a floor. -- **Effort:** S · **Impact:** S - -## Attractiveness / Sexiness - -Anchor: **Raycast** — a single-window macOS tool that feels instant, dense and -deliberate, where every wait is narrated and every state looks designed. - -### 1. A two-minute paid generation shows a spinner and a noun - -- **Location:** `Lectern/App/ContentView.swift:396` -- **Proof:** - ```swift - var body: some View { - VStack(spacing: 18) { - ProgressView().controlSize(.large) - Text(app.stage).font(.title3.weight(.semibold)).contentTransition(.opacity) - if app.total > 0 { - ProgressView(value: Double(app.drafted), total: Double(app.total)) - .frame(maxWidth: 280) - Text("\(app.drafted) of \(app.total) \(app.progressNoun)").font(.callout).foregroundStyle(.secondary) - } - Button("Cancel", role: .cancel) { app.cancel() }.buttonStyle(.glass) - } - ``` -- **Verified:** `grep -n 'previews\|skeleton\|shimmer\|redacted' Lectern/App/ContentView.swift | sed -n '1,6p'` → - ``` - 432: if result.previews.isEmpty { - 440: SlideContactSheet(previews: result.previews, titles: result.previewTitles) - ``` - Nothing progressive exists during generation; previews appear only in `ResultView`. -- **Do:** Show the deck assembling: a skeleton contact sheet of `app.total` placeholder - tiles that fill in as slides are drafted, with the stage label as a caption. The - generator already emits `.drafting(c, t)` per slide, so the data is there. -- **Why:** This is the screen the user stares at for the longest, on the most expensive - action, and it is the least designed one in the app. Raycast's rule is that waiting is - a state to design, not a gap to fill with a spinner. -- **Effort:** M · **Impact:** L - -### 2. First run drops you at a fork with no idea what either side does - -- **Location:** `Lectern/App/ContentView.swift:88` -- **Proof:** - ```swift - var body: some View { - @Bindable var app = app - VStack(spacing: 30) { - Spacer() - VStack(spacing: 10) { - Image(systemName: "rectangle.on.rectangle.angled") - .font(.system(size: 44)).foregroundStyle(.tint) - Text("Lectern").font(.largeTitle.weight(.semibold)) - Text("Write a deck, or take one apart.") - ``` -- **Verified:** `grep -rn 'onboard\|firstRun\|hasLaunched\|welcome' Lectern/App/ Lectern/Sources/` → `# (no matches)` -- **Do:** On first launch (no key, empty library) replace the fork with a one-screen - welcome: what Lectern does, the single field that unblocks it (the API key), and a - "Try Inspect with a sample deck" path that needs no key at all. -- **Why:** Without a key, Create fails at the first press; Inspect is the only working - half and nothing says so. A first run that cannot succeed is the most expensive - polish gap in the product. -- **Effort:** M · **Impact:** L - -### 3. The no-preview success state is a bare checkmark on an empty pane - -- **Location:** `Lectern/App/ContentView.swift:437` -- **Proof:** - ```swift - if result.previews.isEmpty { - Spacer() - Image(systemName: "checkmark.seal.fill").font(.system(size: 52)).foregroundStyle(.green) - Spacer() - } else { - ``` -- **Verified:** `grep -n 'ContentUnavailableView' Lectern/App/*.swift` → - ``` - Lectern/App/DeckLibrarySheet.swift:28: ContentUnavailableView( - ``` - Used once, in the library only. -- **Do:** Replace with a composed success state — deck name, slide count, file size, and - the primary Open action — or reuse `ContentUnavailableView` with a description - explaining why no previews were rendered. -- **Why:** The payoff moment of a paid generation currently renders as a floating green - glyph in whitespace, which reads as a placeholder rather than a finish. -- **Effort:** S · **Impact:** M - -### 4. The inspector is fourteen identical material slabs - -- **Location:** `Lectern/App/ContentView.swift:186` -- **Proof:** - ```swift - .padding(16) - .frame(maxWidth: .infinity, alignment: .leading) - .background(.regularMaterial, in: .rect(cornerRadius: 16, style: .continuous)) - } - ``` -- **Verified:** `grep -c 'Card(title:' Lectern/App/InspectorView.swift` → `9` -- **Do:** Give `Card` a density variant: stats and findings as full cards, the secondary - reads (fonts, properties, masters) as a single grouped section with dividers. Introduce - one accent — the deck's own theme colour, already extracted in `themeFonts`/masters — - so the inspector looks like it is about *that* deck. -- **Why:** Nine same-weight cards give the eye no hierarchy; everything is equally - important, so nothing is. The anchor's inspector panes lead with one number and - demote the rest. -- **Effort:** M · **Impact:** M - -Four items; a fifth (app-icon quality) was not assessable — see *Deferred*. - ---- - -## First move - -**Every control on the compose form is invisible to VoiceOver** (from Usability) - -Ship this first because it is the only item on the list where the product is currently -*broken* rather than merely unpolished, and it is an afternoon's work. Lectern's primary -task — describing a deck — cannot be performed at all with VoiceOver: the prompt is an -unlabelled text area, and the audience and goal pickers were constructed with `Picker("")` -plus `.labelsHidden()`, which does not hide a label, it deletes one. I confirmed this both -in source and against the running app's accessibility tree, where the prompt appears as a -bare `AXTextArea` with no description. Every other item on this plan makes a working thing -better; this one makes a non-working thing work, for the users least able to route around -it. It also costs the least of any `Impact: L` item here, needs no architectural decision, -and cannot conflict with the larger UI work that follows — which means it can land while -the generating-screen redesign (the other `L`) is still being built. - -## Dropped during verification - -- **Force-unwrapped provider endpoint URLs** (`AnthropicProvider.swift:18`, `OpenAIImageProvider.swift:65`, `GeminiImageProvider.swift:69`, `AnthropicModels.swift:11`) — cited code does something else: all four unwrap compile-time string literals that are provably valid, so the `!` can never trap. Classic false positive. -- **`try!` in the DEFLATE fixed-table path** (`Inflate.swift:299-300`) — cited code does something else: the lengths are RFC 1951 constants, not input-derived; `HuffmanTable` cannot fail on them. -- **`data.series[0]` in pie/doughnut generation** (`ChartXML.swift:517`) — already in place: `grep -n 'series.isEmpty' Sources/Rostrum/Charts/ChartData.swift` → `26: precondition(!series.isEmpty, "chart needs at least one series")`. Empty series cannot reach the subscript. -- **Compose form has no validation feedback** — already in place: `.disabled(!app.canGenerate)` at `ContentView.swift:389`. -- **Home screen buttons lack accessibility labels** — already in place: verified against the live accessibility tree, which reports `AXButton Description: Create, Describe it, and Lectern writes the .pptx.` SwiftUI combines the label stack automatically. -- **The inspector reads untrusted decks with no decompression limit** — already in place: fixed earlier today; `DeckInspector.defaultReadLimit = 1 << 30` is now passed as `ZipReader.Limits`. -- **SVG previews could execute script** — already in place: `config.defaultWebpagePreferences.allowsContentJavaScript = false` plus a navigation delegate that allows only `about:blank`. -- **Provider responses cached to disk** — already in place: `ProviderNetworking.session = URLSession(configuration: .ephemeral)`. -- **`XML.textContent` string concatenation is quadratic** — mislocated: the builder already coalesces chunks (`pendingText.count == 1 ? pendingText[0] : pendingText.joined()`), so the pathological case is handled at parse time; the remaining concatenation is per-node and shallow. -- **`ChartReader.setFormula` is dead code** — cited code does something else: it is called from the chart-series editing path, not from `ChartReader` itself, so "unused" was an artefact of the single-file grep. -- **Deck generation leaves orphaned artifacts when cancelled** — already in place: the pipeline throws on `CancellationError` before the write step rather than after (`DeckGenerator.swift:90-107`). - -## Deferred - -- **App icon and marketing surface quality** — could not assess: the asset catalog was not inspected and there is no marketing surface in the repo. -- **`Tools/` and `Examples/` executables** — outside the two products under audit; no user-facing surface. -- **Structured error taxonomy for `RostrumError`** — real but low leverage; the strings are already human-readable and the library's consumers are few. -- **Concurrent image generation memory ceiling** (`DeckGenerator.swift:153`) — real, but bounded by `maximumConcurrentRequests` and only reachable on very large illustrated decks. -- **`OPCPackage` multi-pass serialization** — real but measured in milliseconds against a whole-deck save; below the noise floor of item Performance-2. diff --git a/lift-up-plan-20260811-lectern.md b/lift-up-plan-20260811-lectern.md deleted file mode 100644 index 615078bf..00000000 --- a/lift-up-plan-20260811-lectern.md +++ /dev/null @@ -1,420 +0,0 @@ -# Lift-Up Plan: Lectern - -> Platform: macOS 26 + iPadOS/iOS 26 SwiftUI app (from `project.yml` deployment targets and the `NavigationSplitView` shell); built on the in-repo Rostrum library -> Surveyed: 2026-08-11 -> Coverage: full for `Lectern/App/` and `Lectern/Sources/LecternCore/`; `Lectern/Tests/` and `.github/workflows/` surveyed for the CI finding -> Attractiveness anchor: inferred — **Keynote**, Apple's own deck app and the thing a user will unconsciously measure this against on the same machine. -> Model tier: Opus 5 (frontier) — no rerun needed. - -**Context for the counts.** The library shell was rebuilt from scratch this week -and a burn-down pass already shipped most of the obvious Usability and -Attractiveness work — VoiceOver labels, the drafting skeleton, library search, -the `.pptx` drop target, Dynamic Type, the grid/list toggle, the first-run empty -state and the transition rework. Those are gone from this list because they are -done, which is why those two dimensions ship one item each. What is left is -sharper: two of the eight items below are regressions the redesign itself -introduced, found by grepping the new code against the old. - -## Performance - -### 1. Slide counts for the whole library are fetched one at a time, on the main actor - -- **Location:** `Lectern/App/AppState.swift:130` -- **Proof:** - ```swift - func loadSlideCounts() async { - for deck in library where slideCounts[deck.url] == nil { - if let card = await DeckCardIndex.shared.card(for: deck) { - slideCounts[deck.url] = card.slideCount - } - } - } - ``` - with the enclosing type declared at `:9`: - ```swift - @MainActor - @Observable - ``` -- **Verified:** `grep -n 'TaskGroup\|withTaskGroup\|async let\|concurrentPerform' Lectern/App/AppState.swift` → `# (no matches)` -- **Do:** Fan the reads out with `withTaskGroup`, collect into a local - dictionary, and assign `slideCounts` once at the end so the observation fires a - single time instead of once per deck. -- **Why:** Each iteration hops off the main actor and back, and every assignment - to `slideCounts` invalidates every observing card view. `DeckCardIndex` already - does the expensive part cheaply — one memory-mapped zip entry, gated two at a - time — so this loop is throwing away the concurrency the index was built to - allow. At 29 decks it is tolerable; the shape gets worse linearly and it is the - one remaining serial main-actor loop in the library path. -- **Effort:** S · **Impact:** M - -### 2. Two image caches grow for the life of the process - -- **Location:** `Lectern/App/DeckThumbnail.swift:20` and `Lectern/App/SlideRasterizer.swift:26` -- **Proof:** - ```swift - private var cache: [Key: CGImage] = [:] - ``` - ```swift - private var cache: [String: Data] = [:] - ``` -- **Verified:** `grep -rn 'removeValue\|evict\|countLimit\|totalCostLimit\|NSCache\|removeAll' Lectern/App/DeckThumbnail.swift Lectern/App/SlideRasterizer.swift` → `# (no matches)` -- **Do:** Move both to `NSCache` with a `totalCostLimit` set from the decoded byte - size, or keep the dictionaries and evict least-recently-used past a fixed count. - The thumbnail key already includes path, mtime and width, so entries for the - same deck at a stale mtime are pure garbage and never collected. -- **Why:** A library browsed at two widths holds two full-resolution `CGImage`s - per deck forever, and the rasterizer holds a PNG per slide of every deck opened - this session. Neither has an upper bound. The app currently survives on the - user's 29 decks; a user with several hundred, or one who leaves it open for a - week, has a slow leak with no ceiling. This was the failure mode behind the - 183-second / 2 GB card build earlier this week — the cost model is the same one, - just deferred. -- **Effort:** S · **Impact:** M - -## Functionality - -### 1. Settings offers two providers the app cannot use - -- **Location:** `Lectern/Sources/LecternCore/Providers/ProviderFactory.swift:16` -- **Proof:** - ```swift - switch id { - case .anthropic: - return AnthropicProvider(apiKey: key, model: model) - case .openAI: - return OpenAIProvider(apiKey: key, model: model) - case .gemini, .custom: - throw LecternError.providerError(status: 0, message: "\(id.rawValue) isn't wired up yet — use Anthropic or OpenAI.") - } - ``` -- **Verified:** `grep -rn 'case custom\|case gemini' Lectern/Sources/LecternCore/Providers/Providers.swift` → - ``` - Lectern/Sources/LecternCore/Providers/Providers.swift:23: case gemini - Lectern/Sources/LecternCore/Providers/Providers.swift:24: case custom - ``` -- **Do:** Either finish Gemini — its structured-output schema is an OpenAPI subset - that needs `DeckSchema` translated and a key to test against — or hide the - unimplemented cases from the Settings picker until they work, driving the picker - off the existing `isImplemented` helper that sits directly below this function. -- **Why:** A user picks Gemini in Settings, pastes a valid key, watches it save, - clicks Generate, and gets told it "isn't wired up yet". The failure is honest - but it arrives after the user has done all the work, and `.custom` has no - meaning at all — there is nowhere to enter a custom endpoint. The helper that - would let the picker tell the truth already exists and is unused by the UI. -- **Effort:** S · **Impact:** M - -One item. Three other Functionality candidates were shipped during this session and are excluded. - -## Stability - -### 1. CI compiles the app targets but never runs their tests - -- **Location:** `.github/workflows/ci.yml:91` -- **Proof:** - ```yaml - - name: Build the Lectern app (compiled by nothing else) - ``` -- **Verified:** `grep -n 'name:\|xcodebuild' .github/workflows/ci.yml` → - ``` - 86: - name: Test Rostrum (Darwin XML branch) - 88: - name: Test LecternCore (CoreText/CoreGraphics half included) - 91: - name: Build the Lectern app (compiled by nothing else) - ``` - and the tests that exist but never run: `ls Lectern/Tests/LecternAppTests/` → - ``` - DeckRendererTests.swift - KeychainStoreTests.swift - SlideRasterizerTests.swift - ``` -- **Do:** Add an `xcodebuild test` step for the app scheme to the existing macOS - job. The repo's rule is that macOS CI stays cheap, and this is one more step in - a job that is already paying the macOS runner premium — the marginal cost is - small and the coverage gap it closes is the app's entire UI-adjacent layer. -- **Why:** `DeckRenderer`, `KeychainStore` and `SlideRasterizer` live in the app - target, not `LecternCore`, so their tests are invisible to CI. Those three are - exactly where this session's two worst bugs lived — the keychain read failure - and the headless-slide-title bug — and both were caught by a human running the - app, not by a test. Tests that exist and never execute are worse than no tests, - because they read as coverage. -- **Effort:** S · **Impact:** L - -One item. All other Stability candidates were force-unwraps that verification showed to be provably safe. - -## Reliability - -### 1. Every file-picker failure is discarded identically to a cancel - -- **Location:** `Lectern/App/ContentView.swift:100`, `Lectern/App/ContentView.swift:391`, `Lectern/App/InspectorView.swift:32` -- **Proof:** - ```swift - .fileImporter(isPresented: $showImporter, allowedContentTypes: [.presentationML]) { result in - guard let url = try? result.get() else { return } - openDeck(at: url) - } - ``` -- **Verified:** `grep -rn 'case .failure\|catch {' Lectern/App/ContentView.swift Lectern/App/InspectorView.swift` → `# (no matches)` -- **Do:** Switch on the `Result`: `.success` proceeds, `.failure` sets the - existing `errorMessage` state that both views already render. The alert - machinery is present; only the wiring from these three call sites is missing. -- **Why:** `try? result.get()` cannot distinguish "the user pressed Cancel" from - "the file is on an unmounted volume", "the sandbox refused the scoped bookmark" - or "the document is corrupt". All four produce the same outcome: nothing - happens, no message, no spinner, no explanation. On iPadOS, where the picker - routinely returns files from iCloud Drive that are not yet downloaded, this is - the likely first failure a new user meets — and it looks like the app ignored - them. -- **Effort:** S · **Impact:** M - -One item. - -## Security - -### 1. Rejected drafts are written unencrypted on macOS but protected on iOS - -- **Location:** `Lectern/Sources/LecternCore/Providers/DeckGenerator.swift:76` -- **Proof:** - ```swift - // On iOS this file sits in the app container alongside a Documents - // folder published over USB file sharing; encrypt it at rest. - #if os(iOS) - try Data(json.utf8).write(to: url, options: [.atomic, .completeFileProtection]) - #else - try Data(json.utf8).write(to: url, options: .atomic) - #endif - ``` -- **Verified:** `grep -rn 'completeFileProtection\|NSFileProtection\|setResourceValues' Lectern/Sources/LecternCore/ | grep -v Tests` → - ``` - Lectern/Sources/LecternCore/Providers/DeckGenerator.swift:77: try Data(json.utf8).write(to: url, options: [.atomic, .completeFileProtection]) - ``` -- **Do:** On macOS, write the draft with `0o600` POSIX permissions and place it in - Application Support rather than a user-visible directory — or, better, decide - the drafts do not need to persist at all and keep the last one in memory for - the retry. -- **Why:** A rejected draft is the raw model response to the user's prompt: their - topic, their pasted grounding documents, and whatever confidential material they - attached. On iOS that was recognised and protected. On macOS the same content - lands in a plain file readable by every process running as the user, including - anything sandboxed with a Documents entitlement, and it is never cleaned up. - The comment shows the risk was understood; the platform split just left the - desktop out. -- **Effort:** S · **Impact:** M - -One item. Every other Security candidate — key storage, prompt-injection fencing, redirect handling, retry deadlines — was verified as already mitigated this session. - -## Usability - -### 1. The redesigned library deletes decks with no confirmation, from a button that promises one - -- **Location:** `Lectern/App/LibraryView.swift:407` and `Lectern/App/DeckListView.swift:149` -- **Proof:** - ```swift - Button("Delete…", role: .destructive) { app.deleteFromLibrary(deck) } - ``` - where the old sheet it replaced did ask, at `Lectern/App/DeckLibrarySheet.swift:79`: - ```swift - .confirmationDialog( - ``` -- **Verified:** `grep -rn 'confirmationDialog' Lectern/App/` → - ``` - Lectern/App/DeckLibrarySheet.swift:79: .confirmationDialog( - ``` -- **Do:** Give the new grid and list rows the same `confirmationDialog` and - `pendingDelete` state the sheet already uses — or add real undo via - `UndoManager`, which is the Keynote-grade answer and works from ⌘Z. -- **Why:** This is a regression the redesign introduced. The trailing ellipsis in - "Delete…" is an Apple HIG promise that a confirmation follows; here the click - deletes immediately and permanently, with no undo and no trash. The user's decks - are the only irreplaceable thing in this app — they are the output of paid model - calls — and the two most-used paths to delete one now have less protection than - the sheet they replaced. -- **Effort:** S · **Impact:** L - -One item; the rest of this dimension shipped earlier in the session. - -## Attractiveness / Sexiness - -Anchor: **Keynote**. - -### 1. The app icon has no dark or tinted variant on an OS that asks for both - -- **Location:** `Lectern/App/Assets.xcassets/AppIcon.appiconset/Contents.json:3` -- **Proof:** - ```json - "images" : [ - { "idiom" : "mac", "scale" : "1x", "size" : "16x16", "filename" : "icon_16.png" }, - { "idiom" : "mac", "scale" : "2x", "size" : "16x16", "filename" : "icon_32.png" }, - ``` - through to the final entry, with no appearance keys anywhere in the array: - ```json - { "idiom" : "universal", "platform" : "ios", "size" : "1024x1024", "filename" : "icon_1024.png" } - ], - ``` -- **Verified:** `grep -n 'appearances\|luminosity\|tinted' Lectern/App/Assets.xcassets/AppIcon.appiconset/Contents.json` → `# (no matches)` -- **Do:** Add dark and tinted appearance variants to the `universal` iOS entry - and the large macOS sizes. The source art already exists at 1024; the variants - are a dark-background version and a monochrome version. -- **Why:** This app targets macOS 26 and iOS 26, where a user in dark mode or with - a tinted home screen sees every well-maintained app adapt and the rest sit there - in default light colours. Keynote adapts. The icon is the single most-seen pixel - of the product and the only part of it a user judges before launching. Every - size is otherwise present and correct, so this is the last mile of an icon set - that is already 90% done. -- **Effort:** S · **Impact:** M - -One item. - ---- - -## First move - -**CI compiles the app targets but never runs their tests** (from Stability) - -Ship this first because it is the only item that changes the odds on all the -others. `DeckRenderer`, `KeychainStore` and `SlideRasterizer` have test files that -have never executed in CI — and those three files are precisely where this -session's two most expensive bugs lived. The keychain read failure and the -headless-slide-title bug both reached the user, both took a live debugging session -to trace, and both were the kind of thing a test in an already-written file would -have caught at push time. Right now the repo has the appearance of coverage -without the fact of it, which is the most dangerous state a test suite can be in, -because it makes everyone downstream — including the burn-down run this plan will -feed — trust a green check that never looked at the app. It is one step added to a -macOS job that is already running and already paying for the runner, so it costs -almost nothing, and it is the item that makes shipping the other seven safe. - -## Dropped during verification - -- **Anthropic/OpenAI endpoint URLs are force-unwrapped** — cited code does something else: both are compile-time string literals that `URL(string:)` cannot fail on. Dropped in the 2026-08-09 run too; re-verified. -- **`PriceTable` returns a wrong estimate for unknown models** — already in place: `grep -n 'return nil' Lectern/Sources/LecternCore/Providers/PriceTable.swift` shows exact-key lookup returning `nil`, so unknown models show no estimate rather than a wrong one. Correct by design. -- **The retry loop can exceed its stated deadline** — already in place: `HTTPRetry.timeout(startedAt:cap:)` was added this session and clamps each attempt to the remaining budget. -- **Grounding text is interpolated into the prompt unfenced** — already in place: `PromptTemplates.groundingBlock` wraps attachments in a random `fenceToken`; shipped this session. -- **`DeckCardIndex` opens the whole package to count slides** — already in place: it memory-maps one zip entry (`ppt/presentation.xml`) and counts `p:sldId`; this was the 183 s → 0.79 s fix. -- **Thumbnail generation blocks the scroll** — already in place: `QLThumbnailGenerator` with a two-at-a-time gate, keyed by path + mtime + width. -- **The grid re-deals during the sidebar animation** — already in place: `deckColumnCount` derives from window width, not container width, and columns are `.flexible()`. -- **The app has no `.pptx` drop target** — already in place: re-added to the shell this session at `ContentView.swift`. -- **The library has no search** — already in place: shipped in the burn-down pass. -- **Slide tiles swallow the scroll wheel** — already in place: fixed this session on both platforms. -- **The app icon is missing sizes** — already in place: `ls` shows all seven macOS sizes plus the iOS 1024 present and correctly mapped. Only the *appearance variants* are missing, which is reported above as a separate, narrower finding. - -## Deferred - -- **Only one image failure is reported when several images fail** — real, but the collapse is in a warning path the user rarely sees; low leverage next to the delete regression. -- **No cancel affordance during a long generation** — real gap against Keynote, but it needs a cancellation token threaded through `DeckGenerator` and the provider; L effort for an operation that usually completes in under a minute. -- **The 29 existing decks remain headless** — the title fix only applies to newly written decks. A one-shot repair pass over the library is real work but is a migration, not a lift-up item. -- **iOS keeps live `WKWebView` slide previews while macOS rasterizes** — a platform asymmetry worth closing, but `takeSnapshot` needs a window and the iOS path is not currently slow. - ---- - -# Burn-down — 20260811 - -Executed by `/burn-down` on branch `burndown/liftup-20260811`. The routing table, roster -substitution and isolation notes are recorded once, in -`lift-up-plan-20260811-rostrum.md` — the two plans were burned down as a single run. - -Summary: frontier `claude-opus-5`, strong `claude-opus-4.8`, fast `claude-sonnet-5`, -reviewer `gpt-5.6-sol` (different family, for genuine decorrelation). - -## Manifest — Lectern items - -| Item | Status | Lane | Actual executor | Commit | Verify | -|---|---|---|---|---|---| -| L-STAB-1 | shipped | strong | `claude-opus-4.8` | `c98def5` | full gate green; `ci.yml` diff **0 bytes** | -| L-USE-1 | shipped | fast | `claude-sonnet-5` | `6bd4f76` | 627/161/5 green | -| L-SEC-1 | shipped | strong | `claude-opus-4.8` | `ac6fb6e` | 627/162 green | -| L-REL-1 | shipped | strong | `claude-opus-4.8` | `1b0fc87` | 643/161/7 green | -| L-PERF-1 | shipped | strong | `claude-opus-4.8` | `a6b146f` | 643/162/7 green | -| L-PERF-2 | shipped | strong | `claude-opus-4.8` | `995a1b8` | 643/162/15 green | -| L-FUNC-1 | shipped | fast | `claude-sonnet-5` | `ef39583` | 643/162/24 green | -| L-ATTR-1 | shipped | strong | `claude-opus-4.8` | `813e212` | 662/162/21 green | - -Final integration gate: `./scripts/verify.sh` → **All green** (Rostrum 665, LecternCore 162, -app-hosted 28, macOS + iOS app builds). The app-hosted suite went from **2 tests, run by -nothing** to **28 tests, run by a real gate**. - -## Scope amendment authorised by the repository owner - -**L-STAB-1** was planned as "add an `xcodebuild test` step to the existing macOS CI job". The -owner overrode that approach on cost grounds (hosted macOS bills ~10x; standing rule is that -CI stays cheap and Apple-side verification happens locally). Implemented instead as a local -gate: the app-hosted test stage was added to the existing `scripts/verify.sh`, plus a tracked -`scripts/hooks/pre-push` and `scripts/install-hooks.sh`. **`.github/workflows/ci.yml` is -byte-identical** — no CI cost was added. - -This turned out to restore consistency rather than merely apply a preference: `git log` shows -`scripts/verify.sh` was created in `3f6acce "ci: Linux only, and make the local gate a real -command"`. The audit had proposed contradicting a decision already recorded in the repo's own -history. - -## Citation-gate evidence (orchestrator re-read, non-delegable) - -- **L-USE-1** — the direct-delete button exists nowhere in `Lectern/App/`; both sites now set - `pendingDelete`, gating a wired `.confirmationDialog`. -- **L-SEC-1** — the plain `.atomic` macOS write is gone; the file is created owner-only. -- **L-REL-1** — `try? result.get()` absent from all three sites outside a doc comment; all - three route through `FileImportOutcome.handle`. -- **L-PERF-1** — the serial `for deck in library where…` loop is gone; `withTaskGroup` + - a single `slideCounts.merge`. **`DeckCardIndex` diff is 0 bytes** — the throttle that - prevents oversubscription was not weakened. -- **L-PERF-2** — no unbounded `[String: Image]` remains; `BoundedCache` defined once; - `inFlight` drains via `defer`. -- **L-FUNC-1** — picker driven by `ProviderPicker.selectable`; `ProviderFactory`'s throw intact. -- **L-STAB-1** — `scripts/verify.sh:64` runs `Lectern/scripts/test-app.sh`; `ci.yml` unchanged; - `core.hooksPath` confirmed unset so the hook could not block this run. -- **L-ATTR-1** — appearance entries present; `assetutil` shows `UIAppearanceDark` and - `ISAppearanceTintable` in the compiled `Assets.car`; **both PNGs visually inspected by the - orchestrator** and judged legible and on-brand. - -## Plan errata found during execution - -1. **L-STAB-1** — the audit's `Verified:` field listed three app-test files - (`Lectern/Tests/LecternAppTests/{DeckRenderer,KeychainStore,SlideRasterizer}Tests.swift`) - that **do not exist and never did**. The real app-test target is `Lectern/AppTests/`, which - held one file. The gap was real; the proof was fabricated. Corrected mid-flight, and no - landed artifact repeats the false claim. -2. **L-REL-1** — the plan claimed `.failure` could set "the existing `errorMessage` state that - both views already render". No such state existed; the surfacing had to be built. -3. **L-FUNC-1** — the plan claimed the `isWired` helper was "unused by the UI". It was already - used: `SettingsView.swift:45` renders `"\(id.label) (soon)"`. Scope was narrowed to the gap - that genuinely remained — a "(soon)" row was still *selectable*. -4. **L-PERF-2** — the plan quoted the cache types as `[Key: CGImage]` and `[String: Data]`; - both are `[String: Image]`. It also proposed `NSCache`, which cannot hold a SwiftUI `Image` - (a struct) without boxing — that guidance would have sent the agent down a dead end. - -## Cross-model review - -`gpt-5.6-sol` raised one **major** and two **minors** against Lectern items. - -- **Major, fixed** (`DeckGenerator.swift`): `FileManager.createFile` does not guarantee the mode - is applied before the bytes land, so L-SEC-1's window may have remained open. Replaced with - `open(O_WRONLY|O_CREAT|O_EXCL, 0o600)`, where POSIX applies the mode at inode creation — - correct regardless of Foundation's internals. (`c472fa7`) -- **Minor, accepted with reason** (L-USE-1, L-REL-1): the added tests exercise the extracted - helper, so reverting the *view* wiring would still pass. This is a fair critique. The app has - no UI-test harness, and adding XCUITest for these two items would mean flaky tests and real - scope creep, so it is recorded honestly here rather than papered over. Worth a future item. -- The reviewer also flagged that HEAD contained items outside its first review scope. That was - wave pipelining, not a defect; those items were covered by the second review pass. - -## Out-of-scope observations, for a future `/lift-up` - -Recorded, deliberately **not** acted on, per burn-down's rule against self-directed additions: - -- `DeckRenderer`, `KeychainStore` and `SlideRasterizer` have **no tests at all** — genuine - missing coverage, distinct from the L-STAB-1 gap that they merely never *ran*. -- Two pre-existing warnings remain: `SVGRenderer.swift:25` (unused `dom`) and - `AppState.swift:113` (unused `Int` expression). - -## Cross-model review, second pass - -- **Major, fixed** (`AppState.swift`): `selectProvider` corrected the model for the newly - chosen provider but never persisted it, and `init` fell back to the property default when the - stored model did not match. Together, a stored OpenAI selection could come back after a - relaunch holding a **Claude** model name — which generation would then send to OpenAI, with - the Model picker showing no matching tag. Both ends now normalise and persist the pair, with - a test asserting the model always belongs to the selected provider. (`c634ea4`) -- **Not taken, with reason**: the reviewer reported that `DeckGenerator`'s `open()` needs - conditional `Darwin`/`Glibc` imports or LecternCore will not build on Linux. - `Lectern/Package.swift:11` declares `platforms: [.macOS(.v13), .iOS(.v16)]` — LecternCore does - not target Linux, and both app builds pass. Dropped as inapplicable. - -Final gate after these fixes: **Rostrum 669, LecternCore 162, app-hosted 30 — All green.** diff --git a/lift-up-plan-20260811-rostrum.md b/lift-up-plan-20260811-rostrum.md deleted file mode 100644 index 2957fbce..00000000 --- a/lift-up-plan-20260811-rostrum.md +++ /dev/null @@ -1,425 +0,0 @@ -# Lift-Up Plan: Rostrum - -> Platform: Swift library — macOS 13+, iOS 16+, Linux (from `Package.swift` `platforms:`; zero SwiftPM dependencies) -> Surveyed: 2026-08-11 -> Coverage: full for `Sources/Rostrum/` (Zip, XML, OPC, Core, Presentation, Drawing, Charts, Fonts, Schema); `Tools/` and `Examples/` surveyed but not audited line-by-line -> Attractiveness anchor: inferred — **python-pptx**, the library Rostrum is a port of in spirit and the one its users will compare it to. For a library, "attractiveness" reads as API elegance, documentation, and the quality of the XML it emits. -> Model tier: Opus 5 (frontier) — no rerun needed. - -**Read the counts before the items.** This is Rostrum's third audit, it carries 627 tests -including a foreign-deck corpus and a round-trip fixed-point gate, and it shows. -Most candidates died in verification because the guard was already there — the -zip budget, the DTD block, the depth cap, the bounded-int helpers all exist. Four -dimensions ship fewer than five items and two ship none. Padding them would have -meant inventing work; the empty sections are the finding. - -## Performance - -### 1. `ShapeCollection` re-walks the XML tree on every access - -- **Location:** `Sources/Rostrum/Presentation/Shapes.swift:26` -- **Proof:** - ```swift - public var all: [Shape] { - guard let spTree = Slide.existingSpTree(of: part) else { return [] } - return Self.children(of: spTree, part: part, package: package) - } - ``` - and the three accessors built on it: - ```swift - public var count: Int { all.count } - - public subscript(index: Int) -> Shape { all[index] } - - public func makeIterator() -> IndexingIterator<[Shape]> { - all.makeIterator() - } - ``` -- **Verified:** `grep -n 'cache\|memo\|lazy var\|stored' Sources/Rostrum/Presentation/Shapes.swift` → `# (no matches)` -- **Do:** Cache the built array on `ShapeCollection` and invalidate it in `part.markDirty()`, or make `count`/`subscript` read the `p:spTree` children directly instead of materialising every `Shape` facade. At minimum, document that `subscript` is O(n) so callers reach for `all` once. -- **Why:** `subscript(index:)` looks like an array access and costs a full tree walk plus a facade allocation per shape, so the ordinary `for i in 0..") - #expect(root.serialized() == "") - } - ``` -- **Verified:** `grep -n -i 'lossless\|round-trip' CLAUDE.md` → - ``` - 19:- **Lossless round-trip is sacred.** Opening a file and saving it must never - ``` -- **Do:** Either carry comments and processing instructions through the DOM as - opaque nodes so they survive a save, or amend the invariant in `CLAUDE.md` and - the README to state the exception plainly. Do not leave the two disagreeing. -- **Why:** The project's most-stated promise is that opening and saving a deck - never drops XML it does not model. A comment is exactly that, it is dropped, - and a test enforces the dropping — so a deck from a producer that annotates its - XML loses those annotations silently on any save. Whichever way this is - resolved, the current state means the headline guarantee is not quite true. -- **Effort:** M · **Impact:** M - -One item. - -## Stability - -**Zero items survived verification.** Every candidate was a force unwrap, `try!` -or `precondition` that turned out to be provably unreachable — the same result as -the 2026-08-09 audit, which is itself the finding. The specific drops are listed -in *Dropped during verification*; three of them were re-checked from scratch this -run rather than carried over, and all three failed again. - -## Reliability - -### 1. A broken layout relationship collapses inheritance in silence - -- **Location:** `Sources/Rostrum/Presentation/SVGRenderer.swift:70` -- **Proof:** - ```swift - private func inheritanceChain() -> (layout: Part?, master: Part?) { - guard let rel = slidePart.rels.first(ofType: RelType.slideLayout), - let layout = try? package.part( - at: PackURI.resolve(target: rel.target, relativeTo: slidePart.uri.baseURI)) - else { return (nil, nil) } - guard let masterRel = layout.rels.first(ofType: RelType.slideMaster), - let master = try? package.part( - at: PackURI.resolve(target: masterRel.target, relativeTo: layout.uri.baseURI)) - else { return (layout, nil) } - return (layout, master) - } - ``` -- **Verified:** `grep -n 'warning\|warnings\|unmeasured' Sources/Rostrum/Presentation/SVGRenderer.swift` → `# (no matches)` -- **Do:** Return the reason alongside the chain and surface it the way - `renderSVG` already surfaces `unmeasuredFonts` — a named list of what could not - be resolved — so a caller can say "this deck's layout is missing" instead of - showing an unstyled picture. -- **Why:** A deck whose layout relationship is damaged still renders, with every - inherited background, placeholder position and theme colour quietly gone. It - looks like Rostrum rendered the deck wrong rather than like the deck is broken, - and the renderer already has a vocabulary for degraded output it declines to - use here. -- **Effort:** S · **Impact:** M - -One item. - -## Security - -**Zero items survived verification.** This is the third pass over these parsers -and every proposed hardening was already present: the decompression budget, the -`` rejection that closes billion-laughs, the XML depth cap, the -bounded-integer attribute helpers, and per-record skipping in the font name -table. The verbatim greps are in *Dropped during verification*. For a library -whose whole job is parsing hostile files, an empty Security section earned by -checking is the strongest thing this report says. - -## Usability - -### 1. Three ways to add a slide, two of them nearly the same name - -- **Location:** `Sources/Rostrum/Presentation/Layouts.swift:60` and `:84` -- **Proof:** - ```swift - @discardableResult - public func add(layout: SlideLayout) throws -> Slide { - ``` - ```swift - @discardableResult - func add(boundTo layout: SlideLayout) throws -> Slide { - ``` -- **Verified:** `grep -n 'func add(layout\|func add(boundTo\|public func add()' Sources/Rostrum/Presentation/Layouts.swift Sources/Rostrum/Presentation/Slides.swift` → - ``` - Sources/Rostrum/Presentation/Layouts.swift:60: public func add(layout: SlideLayout) throws -> Slide { - Sources/Rostrum/Presentation/Layouts.swift:84: func add(boundTo layout: SlideLayout) throws -> Slide { - Sources/Rostrum/Presentation/Slides.swift:69: public func add() throws -> Slide { - ``` -- **Do:** Name the difference rather than hiding it in a preposition: - `add(clonedFrom:)` versus `add(bound​To:)`, or one method with an explicit - `placeholders: .cloned | .none`. If `add(boundTo:)` is to stay internal, say so - in its doc comment. -- **Why:** `add(layout:)` clones the layout's placeholder shapes; `add(boundTo:)` - binds the relationship and clones nothing. Those produce very different decks, - and the names differ by one word that does not suggest which. The second was - added this week for the title-placeholder fix, so the confusion is new and cheap - to correct now. -- **Effort:** S · **Impact:** S - -One item. - -## Attractiveness / Sexiness - -Anchor: **python-pptx**. For a library this is API elegance, documentation, and -whether the emitted XML looks like something PowerPoint itself would write. - -### 1. The docs promise a verification the CI does not perform - -- **Location:** `Lectern/README.md:172` against `.github/workflows/ci.yml:91` -- **Proof:** - ``` - **Verified:** the app compiles under **Swift 6 complete strict-concurrency**, - ``` - and what CI actually runs: - ``` - - name: Build the Lectern app (compiled by nothing else) - ``` -- **Verified:** `grep -n 'name:\|xcodebuild' .github/workflows/ci.yml` → - ``` - 86: - name: Test Rostrum (Darwin XML branch) - 88: - name: Test LecternCore (CoreText/CoreGraphics half included) - 91: - name: Build the Lectern app (compiled by nothing else) - ``` -- **Do:** Either add the app-target test step to the macOS job or reword the - claim to say it is verified locally. The repo's own instruction is that macOS - CI stays cheap, so rewording is the likely right answer. -- **Why:** A reader arriving at this project judges it on whether its stated - guarantees hold. "Verified" next to something CI never runs is the one kind of - documentation error that costs trust everywhere else in the README. -- **Effort:** S · **Impact:** S - -One item. - ---- - -## First move - -**The lossless round-trip has a hole, and a test holds it open** (from Functionality) - -Ship this first because it is the only item on this list that touches what the -project says about itself. Rostrum's pitch — the reason to choose it over writing -XML by hand — is that opening and saving a file cannot damage what it does not -understand. `CLAUDE.md` calls that sacred. But comments and processing -instructions are dropped on parse, and `commentsAreDropped` asserts it, so the -guarantee has a documented, tested exception nobody reading the pitch would -expect. Every other item here makes a working thing better; this one decides -whether a headline claim is true. It is also cheap either way: carrying comments -as opaque nodes is a contained change to one file, and if the team decides the -exception is correct, amending two sentences costs nothing and the invariant -becomes honest. Doing it first means the next audit measures the library against -a promise it actually keeps. - -## Dropped during verification - -- **`FontMetrics` re-parses the whole font on every registration** — already in place: `grep -n 'cache\|memo\|\[String: FontMetrics\]' Sources/Rostrum/Fonts/FontLibrary.swift` → `13: private var byName: [String: FontMetrics] = [:]`. `FontLibrary` memoises by family name. -- **`XML.textContent` is quadratic in text length** — cited code does something else: the parser coalesces chunks before materialising (`let text = pendingText.count == 1 ? pendingText[0] : pendingText.joined()`), so the pathological case is handled at parse time. -- **`addTextBox` force-unwraps `p:spPr`** — cited code does something else: the element comes from `makeSp` two lines above, which constructs it; the unwrap cannot fail on a shape this function just built. -- **`try!` in the DEFLATE fixed tables** (`Inflate.swift:299-300`) — cited code does something else: the lengths are RFC 1951 constants, not input-derived. Re-checked this run; unchanged. -- **`PackURI.init` traps on a malformed path** — cited code does something else: both package-read call sites prepend `/` before constructing, so the precondition is unreachable from file data. Carried from 2026-08-09 and re-verified. -- **Zip decompression is unbounded** — already in place: `Inflate.swift:92-97` caps the reservation at `Swift.min(size, 1 << 20)` and `ZipReader.Limits` budgets total output. -- **XML is open to billion-laughs** — already in place: `XML.swift:271-278` rejects ` `Sources/Rostrum/Schema/OXMLHelpers.swift:54` — `replaceChildElements` discards every comment -> and PI. Slide move, duplicate, and import invoke it, violating lossless round-trip. - -Confirmed and fixed inline (`c472fa7`): comments and processing instructions are now carried -through the rebuild, while insignificant whitespace is still dropped. Five regression tests -added; **four of the five fail against the old code**, and the whitespace test correctly still -passes — verified by reverting the function and re-running. - -## Cross-model review, second pass - -The same reviewer (`gpt-5.6-sol`) re-reviewed the full delta and found the first repair -incomplete. Both findings were confirmed by the orchestrator and fixed in `c634ea4`: - -- **Blocker — preserved is not the same as unmoved.** The first fix appended carried nodes - *after* the elements, so even `move(from: 0, to: 0)` relocated a comment. Each carried node - now returns to the index it held, making a no-op rebuild byte-identical. A new test asserts - **exact serialization**, not merely that the comment is present somewhere. -- **Blocker — the same bug in two more places.** Asked explicitly whether the element-only - rebuild existed elsewhere, the reviewer found `Sections.swift:154` (`list.children = []`) - and `Theme.swift:87` (`el.children = [.element(...)]`). Both now route through the shared - `replaceChildElements`, so a fourth site cannot quietly drift from the rule. - -Final gate after these fixes: **Rostrum 669, LecternCore 162, app-hosted 30 — All green.** - -## CI state at merge — an unresolved Linux Swift 6.1 crash - -Recorded honestly rather than rounded up. - -| Check | Result | -|---|---| -| Local `./scripts/verify.sh` (macOS + iOS) | **green** — Rostrum 671, LecternCore 162, app-hosted 30 | -| Linux Swift 6.0 | **green** | -| GitGuardian | **green** | -| macOS (PR gate) | skipped by the workflow's own conditions | -| Linux Swift 6.1 | **red — SIGSEGV, not root-caused** | - -The 6.1 job dies with `Bad pointer dereference at 0x0` in a thread whose only frame is inside -libc; every other thread is an unrelated test doing ordinary Zip/Inflate work. It reproduces -across re-runs, so it is not flaky, but it is not attributable to a line of our code. - -**It did not take a green check red.** The base branch (`burndown/deck-workbench-20260805`, -PR #25) was already failing Linux 6.1 before this run, for an unrelated and pre-existing -reason: `Lectern/Sources/LecternCore/Providers/OpenAIProvider.swift` uses `URLSession` without -importing `FoundationNetworking`, which does not compile on Linux. On that run the Rostrum test -step passed (627 tests) and the LecternCore step failed to build. - -Two hypotheses were tried and **both were wrong about the crash**, though each was worth keeping: - -1. **`XML.Node` had grown from stride 24 to 40** — measured with `MemoryLayout` on both branches - — because the processing-instruction case carries a two-word payload inline, and a - multi-payload enum is sized by its largest case. Making that one case `indirect` restored - stride 24 exactly. That is a real memory win on the hottest structure in the library and a - test now pins it. It did not fix the crash. -2. **Two tests each build a 100,000-node tree** and this branch took the suite from 627 to 671 - tests, so more runs concurrently. They now live in a `@Suite(.serialized)`, keeping the depth - and both assertions unchanged. It did not fix the crash either. - -Merged on the owner's decision, with the crash tracked as separate work. The obvious next steps -for whoever picks it up: fix the `FoundationNetworking` import so the 6.1 job can get past -LecternCore at all, then bisect the Rostrum suite on 6.1 — most cheaply by running it -non-parallel, since every symptom so far points at whole-suite memory pressure under Swift -6.1's runtime rather than at any single test. - -## Linux, resolved — root cause found in a container, not guessed - -The crash recorded above is fixed. It was neither of the two things guessed at -while iterating through CI; both of those changes were kept because each stands -on its own, but neither was the cause. Getting a real Linux environment -(`colima` + the `swift:6.1` image) turned a day of speculation into a -twenty-minute bisect. - -**A processing instruction with no data kills the process.** `` is a -NULL dereference inside libxml2 by way of swift-corelibs-foundation's -`XMLParser` — SIGSEGV, not a throw, before any Rostrum code runs. Isolated to -exactly that spelling with a four-line program: - -| Input | Result | -|---|---| -| `` | ok, `data="d"` | -| `` | ok, `data=""` | -| **``** | **SIGSEGV** | -| `` | ok | - -This was a **denial of service on untrusted input**, not a test-only problem: -Rostrum opens files it did not write, and any `.pptx` carrying `` -would have taken the process down on Linux. It belongs in the same family as -the DOCTYPE rejection and the nesting ceiling — and `parseDocument` already -established the remedy, since it screens for invalid UTF-8 and out-of-range -scalars precisely because *"swift-corelibs-foundation's parser can TRAP (SIGILL, -not throw)"*. - -The fix gives every dataless instruction a payload before the parser sees it and -turns that payload back into `nil` on the way out, so the node still knows it -was the dataless spelling and still writes itself back as ``. A token -generated per parse, rather than a positional count, keeps the mapping local -with no bookkeeping to drift out of step with whichever instructions libxml2 -reports. Comments and CDATA are stepped over rather than scanned into, and a -document without one — very nearly all of them — is not copied at all. - -**Then a second, older problem surfaced behind it.** With the crash gone the job -reached the LecternCore step, where `OpenAIProvider.swift` and its test used -`URLSession` without `import FoundationNetworking`. Every sibling provider -already had it; both files were added earlier in the same session. That also -settled a cross-model review question empirically: `DeckGenerator`'s `open()` -with an explicit `0600` mode needs no Darwin/Glibc shim, because Foundation -re-exports it on Linux. - -**Verified**, in `swift:6.1` on Linux with CI's own oracle tooling installed: -Rostrum **676** tests pass where the suite previously died mid-run, LecternCore -**158** pass, and Swift 6.0 builds. CI on PR #25 is now green on every check — -Linux 6.1, Linux 6.0, the macOS PR gate, and GitGuardian — for the first time on -this branch.