|
| 1 | +# Fix Windows path-separator assumptions in pampa quarto_api path tests (bd-picv) |
| 2 | + |
| 3 | +**Date:** 2026-06-24 |
| 4 | +**Braid:** bd-picv |
| 5 | +**Worktree:** `.worktrees/bd-picv-fix-windows-path-separator` (branch `braid/bd-picv-fix-windows-path-separator`, based on `main`) |
| 6 | +**Status:** Implemented 2026-06-24 (production normalize + `is_rooted`). pampa lib + targeted tests green on Windows. Full `cargo xtask verify` pending as pre-push gate. |
| 7 | + |
| 8 | +## Triage verdict |
| 9 | + |
| 10 | +**Ready to design → design now settled.** Root cause confirmed and reproduced; fix |
| 11 | +direction chosen by user (option a: production-side forward-slash normalization + |
| 12 | +`is_absolute()`→`is_rooted()` swap). Ready to implement TDD-first. |
| 13 | + |
| 14 | +## Issue context |
| 15 | + |
| 16 | +Strand filed 2026-04-28 (bug, P2, labels lua/pampa/windows). Original framing: ~10 |
| 17 | +tests in `crates/pampa/src/lua/quarto_api.rs` fail on Windows because expected paths |
| 18 | +hardcode forward slashes while the Windows impl joins with backslashes via `Path::join`. |
| 19 | +Strand offered a binary choice: (a) normalize output to forward slashes, or (b) |
| 20 | +platform-correct test expectations. |
| 21 | + |
| 22 | +**Fresh investigation reframed the strand:** |
| 23 | + |
| 24 | +- **Native Windows is functionally fine — not a live bug.** `resolve_path` output flows |
| 25 | + only to `io.open`, `fs::read` (via `mediabag.fetch`), and `dofile`. All accept |
| 26 | + backslash paths on Windows. `q2 render` works on Windows; the backslashes are cosmetic. |
| 27 | + The only breakage is the test assertions. |
| 28 | +- **There is a deeper, currently-masked bug.** `resolve_path` gates on `p.is_absolute()` |
| 29 | + (`quarto_api.rs:360`). The workspace's own `quarto-util::is_rooted` doc comment says use |
| 30 | + `is_rooted()` instead, because `is_absolute()` returns `false` on `wasm32` for |
| 31 | + `/project/...` VFS paths. bd-vxl8 already swept `io_wasm.rs`/`dofile_wasm.rs` to |
| 32 | + `is_rooted`; `resolve_path` was missed. No current caller passes an absolute path here |
| 33 | + (extension Lua uses relative names), so it's latent — but a real convention divergence. |
| 34 | +- **Fix belongs in production.** `quarto_util::to_forward_slashes` already exists and |
| 35 | + bd-vxl8 set the precedent of normalizing separators at the boundary. Option (a) makes |
| 36 | + output deterministic cross-platform, matches quarto-cli `pathWithForwardSlashes` and this |
| 37 | + tree's convention, and lets tests assert forward slashes on all platforms with **no |
| 38 | + `#[cfg(windows)]` duplication**. Option (b) leaves production inconsistent and forces |
| 39 | + cfg-gated test expectations. |
| 40 | + |
| 41 | +## Dependency graph |
| 42 | + |
| 43 | +- **related → bd-3pe8** (closed). "Audit pampa production Lua code for Windows path |
| 44 | + escaping." Concluded production Lua *string interpolation* was safe (uses Lua C API, not |
| 45 | + `format!` into source); the test-side fix was correct there. bd-picv is the adjacent |
| 46 | + surface bd-3pe8 did **not** cover: the *return value* of a runtime function, not |
| 47 | + interpolation into source. |
| 48 | +- The actual convention-setting strand is **bd-vxl8** (commit `b406cadc`): swapped |
| 49 | + `is_absolute`/`starts_with('/')` → `quarto_util::is_rooted` in `io_wasm.rs` / |
| 50 | + `dofile_wasm.rs`, and used `to_forward_slashes` when embedding native paths into Lua |
| 51 | + source strings in tests. This is the precedent bd-picv follows. |
| 52 | + |
| 53 | +## What the code looks like today |
| 54 | + |
| 55 | +Reproduced at HEAD on Windows (2026-06-24): |
| 56 | + |
| 57 | +``` |
| 58 | +test_normalize_path_simple: left: "\\a\\b\\c" right: "/a/b/c" |
| 59 | +test_quarto_utils_resolve_path_relative_with_subdir: left: "\\ext\\dir\\sub\\data.json" right: "/ext/dir/sub/data.json" |
| 60 | +``` |
| 61 | + |
| 62 | +Key code: |
| 63 | + |
| 64 | +- `quarto_api.rs:358-369` — `resolve_path`: `if p.is_absolute() { return Ok(path); }` |
| 65 | + then `let resolved = PathBuf::from(&script_dir).join(&path); Ok(normalize_path(&resolved))`. |
| 66 | +- `quarto_api.rs:476-496` — `normalize_path`: collapses `.`/`..`, then |
| 67 | + `result.to_string_lossy().to_string()` — emits native separators verbatim. |
| 68 | +- Production `push_script_dir` callers feed native paths with backslashes: |
| 69 | + - `filter.rs:177-182` — `filter_path.parent().to_string_lossy()` |
| 70 | + - `shortcode.rs:136-142` — `script_path.parent().to_string_lossy()` |
| 71 | +- Helper: `quarto-util/src/path.rs:23` `to_forward_slashes`, `:14` `is_rooted` |
| 72 | + (re-exported `quarto-util/src/lib.rs:8`). |
| 73 | + |
| 74 | +The ~10 failing tests run twice (lib + bin/pampa integration), so ~18-20 visible failures |
| 75 | +from one root cause. |
| 76 | + |
| 77 | +## Proposed phases |
| 78 | + |
| 79 | +### Phase 0 — Test plan (TDD) |
| 80 | + |
| 81 | +The failing tests already encode the desired forward-slash behavior — they ARE the |
| 82 | +failing-first tests. Confirm they fail at branch HEAD on Windows (done). Add coverage the |
| 83 | +current tests lack: |
| 84 | + |
| 85 | +- [ ] A test that `resolve_path` returns forward slashes when the **pushed script_dir |
| 86 | + itself contains backslashes** (simulates the real `filter.rs`/`shortcode.rs` input on |
| 87 | + Windows). Current tests push forward-slash dirs (`/some/extension/dir`), so they don't |
| 88 | + exercise the backslash-input path. Use a backslash literal dir and assert |
| 89 | + forward-slash output. → `test_quarto_utils_resolve_path_backslash_script_dir`. |
| 90 | +- [x] A test that a rooted input is returned forward-slash-normalized and NEVER joined onto |
| 91 | + the script dir. → `test_quarto_utils_resolve_path_rooted_ignores_script_dir`. Pushes a |
| 92 | + `C:\some\dir` script dir, resolves `/abs/x.json`; pre-fix the `is_absolute` gate let it |
| 93 | + fall through and join (RED: `left: "C:\\abs\\x.json"`), pinning the `is_rooted` swap. |
| 94 | + |
| 95 | +### Phase 1 — Production fix in `quarto_api.rs` |
| 96 | + |
| 97 | +- [x] `resolve_path`: replaced `p.is_absolute()` with `quarto_util::is_rooted(p)`; rooted |
| 98 | + branch returns `quarto_util::to_forward_slashes(p)`. |
| 99 | +- [x] `normalize_path`: trailing `result.to_string_lossy().to_string()` replaced with |
| 100 | + `quarto_util::to_forward_slashes(&result)`. |
| 101 | +- [x] No-script-dir relative branch: **normalized** (residual Q1 resolved — see below). The |
| 102 | + branch now returns `to_forward_slashes(p)` so every return path is forward-slash. |
| 103 | + |
| 104 | +### Phase 2 — Verify tests pass with NO cfg-gating |
| 105 | + |
| 106 | +- [x] All 10 original assertions + 2 new pass on Windows with **no `#[cfg(windows)]`**. |
| 107 | +- [x] Updated 3 stale assertions in `shortcode.rs` (`test_shortcode_resolve_path`, |
| 108 | + `..._multi_extension`) that encoded the old backslash output — now use |
| 109 | + `quarto_util::to_forward_slashes` on the expected temp path. These exercise the real |
| 110 | + production path (real temp-dir script dir → forward-slash output end-to-end). |
| 111 | + |
| 112 | +### Phase 3 — Workspace verification |
| 113 | + |
| 114 | +- [x] `cargo nextest run -p pampa` lib tests green; full-crate run green **except** 8 |
| 115 | + pre-existing Windows failures (`test_json_writer`, `test_html_writer`, |
| 116 | + `unit_test_corpus_matches_*`, `unit_test_snapshots_{json,native}`, |
| 117 | + `test_qmd_roundtrip_consistency`, `test_metadata_source_tracking_002_qmd`). Confirmed |
| 118 | + via `git stash` they fail identically on clean branch HEAD — CRLF/snapshot failures from |
| 119 | + the broader Windows test campaign, unrelated to this change and out of scope. |
| 120 | +- [x] `cargo build --workspace` clean (0 errors; 4 pre-existing unrelated warnings). |
| 121 | +- [x] `quarto-util` is already a normal dependency of pampa (`Cargo.toml:56 |
| 122 | + quarto-util.workspace = true`) — residual Q2 resolved, no promotion needed. |
| 123 | +- [ ] `cargo xtask verify` — full leg (incl. WASM hub-build), since the `is_rooted` change |
| 124 | + is WASM-relevant. **Pending — run as the pre-push gate.** |
| 125 | + |
| 126 | +## Open design questions for the user — RESOLVED |
| 127 | + |
| 128 | +1. **No-script-dir relative branch.** Resolved: **normalized.** Single invariant — |
| 129 | + "resolve_path always returns forward slashes." Implemented. |
| 130 | +2. **`quarto-util` dependency tier.** Resolved: already a regular (`.workspace = true`) |
| 131 | + dependency of pampa; no change needed. |
| 132 | + |
| 133 | +## Risks / tradeoffs |
| 134 | + |
| 135 | +- **Behavior change, low risk.** `resolve_path` output changes from backslash to |
| 136 | + forward-slash on Windows. All known consumers (`io.open`, `fs::read`, `dofile`) accept |
| 137 | + forward slashes on Windows, so no functional regression. WASM already produces |
| 138 | + forward-slash, unaffected. |
| 139 | +- **`is_rooted` swap is behavior-neutral today** (no caller passes an absolute path), but |
| 140 | + forward-compatible: a future caller passing a `/project/...` VFS path in WASM would now be |
| 141 | + handled correctly instead of being wrongly joined to the script dir. |
| 142 | +- **No DOM/document-output exposure found** — `resolve_path` output does not flow into |
| 143 | + hrefs/srcs in rendered output (only to file-read APIs), so this is not a rendered-artifact |
| 144 | + correctness fix, just a convention + latent-WASM-bug fix. |
0 commit comments