|
| 1 | +# Resource-copy failure → structured diagnostic |
| 2 | + |
| 3 | +**Strand:** bd-bxrkxblx |
| 4 | +**Date:** 2026-06-19 |
| 5 | +**Status:** assessment / awaiting go-ahead (DO NOT execute yet) |
| 6 | + |
| 7 | +## Overview |
| 8 | + |
| 9 | +`cargo run --bin q2 -- render` (against `docs/`) currently emits: |
| 10 | + |
| 11 | +``` |
| 12 | +error: /…/docs/guides/authoring/figures.qmd: failed to copy /…/docs/guides/authoring/images/html-figure.png -> /…/docs/_site/guides/authoring/images/html-figure.png: No such file or directory (os error 2) |
| 13 | +``` |
| 14 | + |
| 15 | +This is a **plain, unstructured error string**. It has no error code, no |
| 16 | +explanation of what the user did wrong, no actionable hint, and — most |
| 17 | +importantly — no source span pointing at the broken reference. It is exactly |
| 18 | +the class of message `quarto-error-reporting` exists to replace. |
| 19 | + |
| 20 | +The goal of this work: turn this (and its sibling I/O failures) into a |
| 21 | +tidyverse-shaped `DiagnosticMessage` with a `Q-5-x` error code, a catalog |
| 22 | +entry, an explanation, and ideally a span underlining the offending |
| 23 | +`` reference in `figures.qmd`. |
| 24 | + |
| 25 | +## Root cause / mechanism |
| 26 | + |
| 27 | +The error has two distinct layers, and we need to fix the **reporting** layer |
| 28 | +(the underlying trigger — a missing image — is legitimately an error; we just |
| 29 | +report it badly). |
| 30 | + |
| 31 | +### Underlying trigger |
| 32 | +`docs/guides/authoring/figures.qmd:19` references `images/html-figure.png`, but |
| 33 | +`docs/guides/authoring/images/` **does not exist**. The resource collector |
| 34 | +enqueues a copy of the (non-existent) source; the copy fails at flush with |
| 35 | +`ENOENT`. |
| 36 | + |
| 37 | +### Reporting path (the actual bug to fix) |
| 38 | +1. `ResourceCollectorTransform::collect_resource` |
| 39 | + (`crates/quarto-core/src/transforms/resource_collector.rs:398-424`) builds |
| 40 | + `src = input_dir.join(url)` / `dest = output_dir.join(url)` and pushes the |
| 41 | + pair into `self.copies`. **No source span is carried** — the copy intent is |
| 42 | + a bare `(PathBuf, PathBuf)` (`ctx.resource_copies: Vec<(PathBuf, PathBuf)>`, |
| 43 | + `render.rs:303`). The `Inline::Image` it came from *does* have a |
| 44 | + `SourceInfo` in scope at the call site, but it is discarded. |
| 45 | +2. The intents are drained and flushed: |
| 46 | + - native single-doc: `render_to_file.rs:393-403` |
| 47 | + - WASM/preview + project pass-2: `pass2_renderer.rs:667-683` |
| 48 | + (`flush_resource_copies`) |
| 49 | +3. `OutputSink::flush` runs the copy and, on I/O failure, returns |
| 50 | + `OutputSinkError::Copy { src, dest, source }` |
| 51 | + (`crates/quarto-core/src/output_sink.rs:116-121`). Its `Display` |
| 52 | + (`output_sink.rs:157-164`) is the `failed to copy … -> …: …` string. |
| 53 | +4. `From<OutputSinkError> for QuartoError` |
| 54 | + (`output_sink.rs:170-174`) flattens it to |
| 55 | + `QuartoError::other(value.to_string())` — **all structure is lost here**. |
| 56 | +5. The render command attaches this opaque error to the failing document as a |
| 57 | + `pass2_failure` whose `diagnostics` vec is **empty**, so it falls into the |
| 58 | + **legacy** branch in `crates/quarto/src/commands/render.rs:910-912`: |
| 59 | + `eprintln!("error: {}: {}", failure.input.display(), failure.error)`. |
| 60 | + Failures *with* structured `diagnostics` already go through the pretty |
| 61 | + coalescer (`render.rs:907-909`); ours never gets there. |
| 62 | + |
| 63 | +### Why this is the right framing |
| 64 | +There is already a **resource subsystem** in the catalog (`Q-5-1..5`, |
| 65 | +`subsystem: "project"`) and a working precedent for exactly this conversion: |
| 66 | +`resource_error_to_parse_error` in |
| 67 | +`crates/quarto-core/src/project_resources.rs:645-705` turns a `ResourceError` |
| 68 | +into a `DiagnosticMessage` with `.with_code("Q-5-x")`, `.with_location(span)`, |
| 69 | +`.problem(...)`, `.add_info(...)`, degrading gracefully to a span-less message |
| 70 | +when no FileId resolves. We follow that pattern. |
| 71 | + |
| 72 | +`Q-5-1..5` are taken; the next free project-subsystem code is **`Q-5-6`** (and |
| 73 | +`Q-5-7` if we split error classes — see Decision 2). |
| 74 | + |
| 75 | +## Decisions — SETTLED (2026-06-19) |
| 76 | + |
| 77 | +The user reviewed the three decisions. Resolution below; the original options |
| 78 | +are kept in git history. |
| 79 | + |
| 80 | +### Settled design: split the two failure modes, two codes, span-aware |
| 81 | + |
| 82 | +**Two distinct failure classes, reported differently:** |
| 83 | + |
| 84 | +1. **Referenced resource missing** — `Q-5-6`, **warning**, span-aware. The |
| 85 | + common, user-fixable case (a `` whose source doesn't |
| 86 | + exist). Detected **before** any copy is attempted; the build **continues** |
| 87 | + (matches Q1's tolerant behavior). Diagnostic underlines the offending |
| 88 | + reference in the source `.qmd`. |
| 89 | +2. **Resource copy/write failed for an environment reason** — `Q-5-7`, hard |
| 90 | + **error**. Permission-denied, disk-full (`ENOSPC`), etc., surfaced at |
| 91 | + `OutputSink::flush`. The render fails. The specific OS condition is carried |
| 92 | + as an **advisory detail** inside the same diagnostic — we pattern-match |
| 93 | + `io::ErrorKind` (`PermissionDenied`, `StorageFull`, …) to tailor the hint |
| 94 | + where the OS gives a usable kind, falling back to the raw message |
| 95 | + otherwise. |
| 96 | + |
| 97 | +### Where the existence check lives — drain site, not the transform |
| 98 | + |
| 99 | +The user's intent was "detect at collection time." In our architecture the |
| 100 | +`ResourceCollectorTransform` is a **pure AST→AST transform with no `SystemRuntime`** |
| 101 | +(`RenderContext` carries no runtime handle, `render.rs:182`). Putting the |
| 102 | +`is_file` stat *inside* the transform would force either plumbing a runtime |
| 103 | +into a currently-I/O-free transform or reaching for bare `std::fs` (smell in |
| 104 | +the `?Send` pipeline shared with WASM). |
| 105 | + |
| 106 | +Instead the existence check lives at the **drain/flush sites** |
| 107 | +(`render_to_file.rs:393`, `pass2_renderer::flush_resource_copies:667`), which |
| 108 | +already hold a `runtime: &dyn SystemRuntime` exposing |
| 109 | +`is_file` / `path_exists` (trait method, native + WASM). This is **functionally |
| 110 | +identical** to "collection time" from the user's perspective — the check |
| 111 | +happens *before any copy is attempted* — but sites the I/O where a runtime |
| 112 | +already exists. It is therefore the originally-described "option C" semantics at |
| 113 | +**~the same cost as option B** (the only real cost is threading the span, which |
| 114 | +B needs anyway; the stat itself is one `runtime.is_file(src)` call). My earlier |
| 115 | +"C is the most work" assessment wrongly assumed the stat had to live inside the |
| 116 | +transform. |
| 117 | + |
| 118 | +WASM gets correct behavior for free: in `vfs_root` mode the collector emits |
| 119 | +**zero** copy intents (`resource_collector.rs:99`), so the drain list is empty |
| 120 | +and both the existence check and the flush no-op. |
| 121 | + |
| 122 | +### Drain-site algorithm |
| 123 | + |
| 124 | +For each `(src, dest, origin_span)` intent (span threaded per below): |
| 125 | + |
| 126 | +```text |
| 127 | +if !runtime.is_file(src): |
| 128 | + emit Q-5-6 WARNING with origin_span; skip this copy; continue |
| 129 | +else: |
| 130 | + sink.copy(src, dest); enqueue |
| 131 | +# after the loop: |
| 132 | +sink.flush(runtime): |
| 133 | + on OutputSinkError::Copy/Write/... -> emit Q-5-7 ERROR, |
| 134 | + advisory tailored from io::ErrorKind |
| 135 | +``` |
| 136 | + |
| 137 | +### Where the conversion lives |
| 138 | +A dedicated converter near the drain sites (mirroring |
| 139 | +`resource_error_to_parse_error`), **not** the lossy `From<OutputSinkError> for |
| 140 | +QuartoError`. It takes the failure (+ the intent's `origin` span) and produces |
| 141 | +`DiagnosticMessage` + `SourceContext`. Both drain sites call it. |
| 142 | + |
| 143 | +## Test plan (TDD — write first) |
| 144 | + |
| 145 | +1. **Catalog presence test** (in `catalog.rs` tests, mirroring the |
| 146 | + `error_catalog_has_q_13_navigation_codes` style): assert `Q-5-6` (and |
| 147 | + `Q-5-7` if we split) exist, are in the `project` subsystem, mention |
| 148 | + "resource"/"copy", and have a `docs_url` ending in the code. |
| 149 | +2. **Conversion unit test** (quarto-core): given an `OutputSinkError::Copy` |
| 150 | + with an ENOENT source + an origin `SourceInfo`, the converter yields a |
| 151 | + `DiagnosticMessage` with the right code, a problem mentioning the missing |
| 152 | + path, and a resolved location. A second test: span-less degradation when no |
| 153 | + origin is available. |
| 154 | +3. **End-to-end render test** (route through the real render path, per the |
| 155 | + end-to-end-verification rule): render a fixture doc referencing a missing |
| 156 | + image and assert the structured diagnostic (code + pretty form) appears |
| 157 | + instead of the legacy `error: …: failed to copy …` line. Use a fixture under |
| 158 | + the crate's `tests/` tree, not `docs/`. |
| 159 | +4. **Manual E2E**: `cargo run --bin q2 -- render <fixture>` and paste the |
| 160 | + before/after terminal output into this plan. |
| 161 | + |
| 162 | +## Work items |
| 163 | + |
| 164 | +- [x] Settle Decisions 1–3 with the user. (settled 2026-06-19, above) |
| 165 | +- [x] Add catalog entries `Q-5-6` (referenced resource not found, warning) and |
| 166 | + `Q-5-7` (resource copy/write failed, error) to |
| 167 | + `crates/quarto-error-reporting/error_catalog.json`; write the catalog |
| 168 | + presence test (fails first). *(2026-06-19: test |
| 169 | + `error_catalog_has_q_5_6_and_q_5_7` in `catalog.rs` — verified red then |
| 170 | + green. Both entries in `project` subsystem.)* |
| 171 | +- [x] Enrich the resource-copy intent to carry the originating `SourceInfo`: |
| 172 | + new `ResourceCopyIntent { src, dest, origin: SourceInfo }` in `render.rs` |
| 173 | + replaces the `(PathBuf, PathBuf)` tuple. `origin` populated in |
| 174 | + `resource_collector.rs` from `img.target_source.url` (falling back to |
| 175 | + `img.source_info`). Threaded through `render.rs` ctx, |
| 176 | + `stage/context.rs`, `render_to_file.rs`, and |
| 177 | + `pass2_renderer::flush_resource_copies`. *(2026-06-19, compiles.)* |
| 178 | +- [x] Add the dedicated converter |
| 179 | + (`crates/quarto-core/src/resource_copy_diagnostics.rs`), modeled on |
| 180 | + `resource_error_to_parse_error`. Three unit tests (all pass): (a) missing |
| 181 | + source + span → `Q-5-6` warning, located; (b) `OutputSinkError::Copy` |
| 182 | + with `PermissionDenied` → `Q-5-7` error with the permission advisory; (c) |
| 183 | + disk-full (`Other` kind) surfaces the raw OS message. Plus shared helpers |
| 184 | + `enqueue_resource_copies` (existence-check + enqueue) and |
| 185 | + `copy_failure_error` (wrap as `QuartoError::Parse`). |
| 186 | +- [x] Implement the drain-site algorithm at **both** sites |
| 187 | + (`render_to_file.rs`, `pass2_renderer::flush_resource_copies`): |
| 188 | + `runtime.is_file(src)` pre-check → `Q-5-6` warning merged into |
| 189 | + `render_output.diagnostics`; on flush, only `OutputSinkError::Copy` |
| 190 | + routes to `Q-5-7` (other variants — HTML write, dir create — keep the |
| 191 | + existing path). `Q-5-7` reaches the pretty coalescer via |
| 192 | + `file_failure_from_error` lifting `QuartoError::Parse` diagnostics. |
| 193 | +- [x] Confirm warning semantics: `Q-5-6` is pushed as a *warning* into the |
| 194 | + render output's diagnostics and the copy is skipped — the render |
| 195 | + proceeds. Only the `Q-5-7` `Copy` flush fault returns `Err` and aborts. |
| 196 | + *(Still to verify end-to-end that exit code is 0 with only Q-5-6.)* |
| 197 | +- [x] End-to-end render test + manual verification. *(2026-06-19)* |
| 198 | + - Regression tests added to |
| 199 | + `tests/integration/render_preserves_source_files.rs` (single-doc + |
| 200 | + website config), both green. |
| 201 | + - **Manual single-doc:** `q2 render <fixture>/doc.qmd` with a missing |
| 202 | + `images/nope.png` → printed `Warning: [Q-5-6] Referenced resource not |
| 203 | + found`, a pretty ariadne diagnostic underlining exactly `images/nope.png` |
| 204 | + in the reference, the problem prose, the hint, **exit code 0**, and the |
| 205 | + HTML was still produced. Output inspected. |
| 206 | + - **Manual project (the original repro):** `q2 render docs/` → the former |
| 207 | + `error: …/figures.qmd: failed to copy …/images/html-figure.png … (os |
| 208 | + error 2)` is now a `Q-5-6` warning; `Rendered 166 of 166 files … — 1 |
| 209 | + error, 27 warnings`. The remaining "1 error" is a *pre-existing, |
| 210 | + unrelated* `duplicate crossref identifier fig-charts`, not our copy |
| 211 | + failure. No `Q-5-7` fired (no permission/disk faults), as expected. |
| 212 | + - **Q-5-7** (permission/disk) is covered by the converter unit tests; |
| 213 | + not separately driven through the binary (reliably forcing a |
| 214 | + permission/ENOSPC copy fault is platform-specific — see the |
| 215 | + cross-platform rule). The native render-to-file routing |
| 216 | + (`OutputSinkError::Copy → copy_failure_error`) is simple and shared |
| 217 | + with the unit-tested helper. *Honest status: Q-5-7's end-to-end binary |
| 218 | + path was not exercised; its diagnostic shape and the routing helper |
| 219 | + are unit-tested.* |
| 220 | + - The WASM/preview drain (`pass2_renderer::flush_resource_copies`) |
| 221 | + mirrors the native drain and shares `enqueue_resource_copies`; not |
| 222 | + separately E2E'd on native (it's the WASM-only `WasmPassTwoOutput` |
| 223 | + path). |
| 224 | +- [x] `cargo nextest run --workspace` (10,284 passed) and full `cargo xtask |
| 225 | + verify` (Rust + WASM + hub-client builds and tests) — both green |
| 226 | + (2026-06-19). One clippy `io_other_error` lint in a test was fixed |
| 227 | + (`std::io::Error::other`). |
| 228 | +- [x] Authored matching docs pages `docs/errors/project/Q-5-6.qmd` and |
| 229 | + `Q-5-7.qmd`, modeled on the sibling Q-5 pages (front-matter schema, |
| 230 | + What/Why/How + Related sections, `status: stub`). Both render cleanly via |
| 231 | + `q2 render docs/` (auto-discovered by the `errors/index.qmd` listing; no |
| 232 | + manual index edit needed). No `cargo xtask error-docs` tool exists yet |
| 233 | + and there is no automated catalog↔docs parity test to satisfy. |
| 234 | + |
| 235 | +## Key references |
| 236 | +- `crates/quarto-core/src/output_sink.rs:116-174` — `OutputSinkError` + the |
| 237 | + lossy `From … for QuartoError`. |
| 238 | +- `crates/quarto-core/src/transforms/resource_collector.rs:398-424` — where the |
| 239 | + copy intent (and the discarded span) originate. |
| 240 | +- `crates/quarto-core/src/render.rs:303` — `resource_copies` ctx field type. |
| 241 | +- `crates/quarto-core/src/render_to_file.rs:393-403`, |
| 242 | + `crates/quarto-core/src/project/pass2_renderer.rs:667-683` — flush sites. |
| 243 | +- `crates/quarto-core/src/project_resources.rs:645-705` — |
| 244 | + `resource_error_to_parse_error`, the precedent to mirror. |
| 245 | +- `crates/quarto/src/commands/render.rs:887-916` — legacy vs. structured |
| 246 | + reporting split (the branch we want to land in). |
| 247 | +- `crates/quarto-error-reporting/src/builder.rs` — `DiagnosticMessageBuilder` |
| 248 | + (`with_code`, `with_location`, `problem`, `add_info`, `add_hint`). |
0 commit comments