Merge pull request #374 from quarto-dev/bugfix/bd-22rtwdur-windows-smoke-all-suite

Fix Windows-only theme CSS fallback and metadata href separators

Author: cscheid

Cargo.lock

@@ -0,0 +1,166 @@
+# Embedded-resource virtual-path contract
+
+**Status:** Design (2026-07-03). Fix tracked by strand `bd-nxxgg0pp`
+(discovered from `bd-3fgnmlco`, blocks the `bd-22rtwdur` smoke-all umbrella).
+
+Quarto compiles Bootstrap/theme SCSS from resources embedded at build time
+via `include_dir!`. The `@import`/`@use` statements *inside* that embedded
+SCSS (e.g. Bootstrap's own `@import "vendor/rfs"`) are resolved at compile
+time against a **virtual** load path rooted at `RESOURCE_PATH_PREFIX =
+"/__quarto_resources__"`. This document defines the contract that virtual
+namespace must obey so resolution behaves identically on every OS and on
+both compile backends (native grass, WASM dart-sass).
+
+## Two path domains
+
+Two distinct kinds of path flow through the SASS layer, both currently
+typed as `std::path::PathBuf`/`&Path`:
+
+- **(a) Real OS paths** — user project documents, custom theme directories
+  (`ThemeContext` load paths). `Path::join` and native separators are
+  *correct* here. This domain is not changed by this work.
+- **(b) Virtual embedded-resource namespace** — `/__quarto_resources__/…`.
+  Must be **forward-slash on every platform**. This is a URL-like opaque
+  string domain, not a filesystem path.
+
+The bug (below) is that domain (b) is forced through `std::path` machinery
+that is correct only for domain (a).
+
+## Root cause (Windows) — source-anchored
+
+On a Windows build, every embedded `@import` fails to resolve, theme/
+bootstrap/highlight compilation errors, and the render silently falls back
+to base CSS (`compile_theme_css.rs:547`). Chain:
+
+1. `default_load_paths()` (`quarto-sass/src/resources.rs:305-310`) returns
+   virtual paths built with `format!` — forward-slash, e.g.
+   `/__quarto_resources__/bootstrap/scss`.
+2. grass resolves `@import "vendor/rfs"` in `Visitor::find_import`
+   (`grass_compiler 0.13.4`, `crates/compiler/src/evaluate/visitor.rs:858-859`):
+   `let path_buf = load_path.join(path);`. `PathBuf::join` is target-OS
+   aware and inserts `\` on Windows → `/__quarto_resources__/bootstrap/scss\vendor/rfs`.
+   grass's own `options.rs` documents this limitation ("still using
+   std::path… constrains to the target platform").
+3. grass calls `self.options.fs.is_file(&path_buf)` on that raw `\`-joined
+   path (via the `try_path!` macro, visitor.rs:819/823). `Fs::canonicalize`
+   is **not** a pre-lookup hook — `import_like_node` (visitor.rs:891) calls
+   it only *after* `find_import` already matched, for the `import_cache`/
+   `files_seen` key and `fs.read`. So the `Fs` impl must normalize itself.
+4. Our `RuntimeFs` (`quarto-system-runtime/src/sass_native.rs:84-119`)
+   checks embedded first → `EmbeddedResources::is_file` →
+   `strip_prefix` (`quarto-sass/src/resources.rs:167-184`), which is pure
+   `&str` surgery that only ever trims `/`. It yields relative key
+   `\vendor/rfs.scss`.
+5. Embedded file keys are forward-slash — `include_dir_macros 0.7.4`
+   normalizes `\`→`/` at compile time (`src/lib.rs:134-136`). So
+   `\vendor/rfs.scss` never matches `vendor/rfs.scss` → miss.
+
+Empirically verified: `PathBuf::from("/__quarto_resources__/bootstrap/scss")
+.join("vendor/_rfs.scss").to_string_lossy()` = `…/scss\vendor/_rfs.scss` on
+Windows; `str::lines`-style stripping leaves the leading `\`. Real OS load
+paths (custom themes, reveal SCSS) are immune — `RuntimeFs`'s `std::fs`
+fallback is separator-tolerant on Windows, and reveal SCSS is fully inlined
+(no `find_import`).
+
+## Native vs WASM contract
+
+| Aspect | Native (grass) | WASM (dart-sass via JS) |
+|---|---|---|
+| Prefix | `RESOURCE_PATH_PREFIX` | same constant, re-used |
+| Separator | forward-slash by construction; **breaks** when grass's `Path::join` injects `\` | pure forward-slash JS strings; `vfs:` URL importer never touches `path` |
+| Lookup | `EmbeddedResources::strip_prefix` accepts 3 path shapes, normalizes to a relative key | JS importer builds full keys itself (`loadPath + "/" + url`), flat exact-match VFS |
+| Correctness on Windows | **broken** (this bug) | correct (no `\` ever introduced) |
+
+WASM already matches the dart-sass best-practice importer contract
+(custom `vfs:` scheme, `canonicalize`→`load`, pure string) in
+`ts-packages/wasm-js-bridge/src/sass.js`. The native side is the one that
+diverges from the shared forward-slash contract.
+
+**Known WASM divergences (deferred — not this bug):**
+- WASM `populate_vfs_with_embedded_resources`
+  (`wasm-quarto-hub-client/src/lib.rs:64-76`) materializes **only the
+  Bootstrap pool** into the VFS; native exposes all five pools via
+  `CombinedResources`. A future cross-pool `@import`-by-path would resolve
+  native but fail WASM. Currently masked (no such import shipped).
+- The `sass-utils` load path is a dead no-op on WASM (nothing stored there).
+
+Tracked separately (see Deferred work).
+
+## Fix
+
+**Reuse the existing shared helper — do not introduce a new function.**
+`quarto-util` already owns the canonicalization boundary:
+
+```rust
+// crates/quarto-util/src/path.rs:23
+pub fn to_forward_slashes(path: &Path) -> String {
+    path.to_string_lossy().replace('\\', "/")
+}
+```
+
+This is byte-identical to what an earlier draft proposed as a new
+`to_virtual_key`, and it is already unit-tested (unix no-op + Windows
+conversion). It is leading-slash-preserving and idempotent — exactly what
+`strip_prefix` needs, since the very next step, `.strip_prefix(RESOURCE_PATH_PREFIX)`,
+depends on the leading `/`. (An earlier draft's extra
+`split('/').filter(!empty).join("/")` would have *dropped* that leading
+slash and broken the prefix strip; grass's `load_path.join(path)` never
+produces `//` anyway, so only the `\`→`/` replacement is needed.)
+
+There are already two private copies of this logic
+(`quarto-core/src/stage/stages/document_profile.rs:132` `to_forward_slash`,
+`quarto-core/src/project/discovery.rs:181` `to_forward_slashes`), which is
+exactly the proliferation to avoid — consolidate on the `quarto-util` one.
+
+- **Add `quarto-util` as a dependency of `quarto-sass`** (it is not one
+  today). `quarto-util::path` is pure `std` and already reasons about
+  `wasm32` behavior (`is_rooted`), so it is safe in `quarto-sass`'s WASM
+  build. Verify the WASM build during implementation.
+- **Placement — `EmbeddedResources` only.** Call `to_forward_slashes` at
+  the top of `strip_prefix` (which every `is_file`/`is_dir`/`read`/`read_str`
+  funnels through) and in `collect_files`/`collect_directories` for key
+  symmetry. This is the embedded-namespace boundary — the same "normalize
+  once at the boundary" idiom `include_dir` and `rust-embed` use.
+- **`RuntimeFs` stays untouched.** Its `read`/`is_file` fall back to real
+  `std::fs` for user files; normalizing `\`→`/` there would mangle genuine
+  Windows paths (`C:\Users\…`). Normalization is strictly embedded-only.
+  (This corrects a research suggestion to also normalize in `RuntimeFs`.)
+- **Shared helper, not a newtype (for now).** grass's `Fs` and
+  `SystemRuntime` are hard-typed to `std::path`; a `VirtualPath` newtype
+  would ripple through both for a currently-single namespace. Promote to a
+  newtype (rust-analyzer `VfsPath` style) only if the virtual surface grows.
+- **Document the two domains** at the top of `resources.rs` so future
+  embedded providers route through `to_forward_slashes` instead of growing
+  their own string-trimming.
+
+## Test plan (TDD)
+
+1. **RED (in-source unit test, runs on Linux CI):** feed a backslash-joined
+   virtual path (as grass produces on Windows) to `EmbeddedResources::is_file`
+   / `read` and assert it resolves. Build the `\` path inline so the test
+   exercises the corruption on any host. Confirm it fails before the fix.
+2. **GREEN:** add `quarto-util` as a `quarto-sass` dependency and call
+   `quarto_util::path::to_forward_slashes` at the top of `strip_prefix` and
+   in `collect_files`/`collect_directories`. Do **not** add a new
+   normalization function.
+3. **Regression:** full `cargo nextest run -p quarto-sass` — the 30
+   `@import "vendor/rfs"` failures (uncovered once the parse_layer CRLF bug
+   `bd-3fgnmlco` was fixed) must go to zero.
+
+## Deferred work (own strands)
+
+- **Unify embedded pools into the WASM VFS** / drop the dead `sass-utils`
+  load path, so native and WASM resolve the same cross-pool imports.
+- **`windows-latest` CI job** running `compile_scss_with_embedded` against
+  Bootstrap — CI is Unix-only today, so this bug cannot regress-guard on CI.
+  The in-source backslash test covers the logic; real-Windows E2E coverage
+  needs the job.
+
+## Decisions (2026-07-03, with Chris)
+
+- Abstraction: reuse the existing `quarto_util::to_forward_slashes` (add
+  `quarto-util` dep to `quarto-sass`); no new function; newtype deferred.
+  Consolidate the two private dupes onto it opportunistically.
+- Placement: `EmbeddedResources` boundary only; `RuntimeFs` untouched.
+- WASM pool divergence: documented + deferred to a strand.
+- Windows CI job: deferred to a strand.

claude-notes/designs/embedded-resource-virtual-path-contract.md

@@ -230,14 +230,20 @@ fn adjust_paths_recursive(value: &mut ConfigValue, metadata_dir: &Path, document
     match &mut value.value {
         ConfigValueKind::Path(path_str) => {
             let path = PathBuf::from(&*path_str);
-            // Only adjust relative paths (not absolute, not URLs)
-            if path.is_relative()
+            // Only adjust relative paths (not absolute, not URLs). Use
+            // `is_rooted` (has_root), not `Path::is_relative`: on Windows a
+            // POSIX-absolute path like `/usr/share/base.css` is not
+            // `is_absolute` (no drive prefix) and would be wrongly rebased.
+            if !quarto_util::is_rooted(&path)
                 && !path_str.starts_with("http://")
                 && !path_str.starts_with("https://")
             {
                 let abs_path = metadata_dir.join(&path);
                 if let Some(adjusted) = pathdiff::diff_paths(&abs_path, document_dir) {
-                    *path_str = adjusted.to_string_lossy().into_owned();
+                    // The adjusted value is used verbatim in HTML hrefs (e.g. a
+                    // `css: !path` <link>), so it must use forward slashes on
+                    // every platform; pathdiff yields native separators.
+                    *path_str = quarto_util::to_forward_slashes(&adjusted);
                 }
             }
         }

crates/quarto-core/src/project/mod.rs

@@ -17,6 +17,8 @@ once_cell.workspace = true
 include_dir.workspace = true
 # Runtime abstraction for cross-platform file access (WASM + native)
 quarto-system-runtime.workspace = true
+# Shared path helpers (to_forward_slashes for virtual-resource key normalization)
+quarto-util.workspace = true
 # ConfigValue for theme config extraction
 quarto-pandoc-types.workspace = true
 # SourceInfo for source-mapped diagnostics

crates/quarto-sass/Cargo.toml

@@ -102,8 +102,17 @@ enum LayerType {
 /// assert!(layer.rules.contains(".container"));
 /// ```
 pub fn parse_layer(content: &str, hint: Option<&str>) -> Result<SassLayer, SassError> {
+    // Layer parsing is line-ending agnostic. On a Windows checkout the SCSS
+    // sources baked in via include_dir! carry CRLF (git autocrlf), and the
+    // $-anchored boundary regex matches before the `\n` but leaves the `\r`,
+    // so CRLF-terminated markers would be rejected. SCSS -> CSS keeps no
+    // byte-offset map, so normalizing to LF at this single chokepoint is
+    // correct (contrast the document-content preserve policy). Normalizing
+    // here also keeps a clean LF stream flowing into grass.
+    let content = content.replace("\r\n", "\n");
+
     // Verify that at least one boundary marker exists
-    if !LAYER_BOUNDARY_TEST.is_match(content) {
+    if !LAYER_BOUNDARY_TEST.is_match(&content) {
         return Err(SassError::NoBoundaryMarkers {
             hint: hint.map(String::from),
         });
@@ -395,6 +404,28 @@ $second: 2;
         assert!(layer.rules.contains(".rule2"));
     }
 
+    #[test]
+    fn test_parse_crlf_terminated_markers() {
+        // On a Windows checkout, resources/scss/**/*.scss are baked into the
+        // binary with CRLF line endings (git autocrlf) via include_dir!. The
+        // $-anchored boundary regex must still recognize CRLF-terminated
+        // markers, and no residual `\r` may leak into the content handed to
+        // grass. SCSS->CSS keeps no byte-offset map, so normalizing here is
+        // correct (unlike the document-content preserve policy).
+        let lf = "/*-- scss:defaults --*/\n$primary: blue !default;\n/*-- scss:rules --*/\n.container { color: $primary; }\n";
+        let crlf = lf.replace('\n', "\r\n");
+
+        let layer =
+            parse_layer(&crlf, Some("theme.scss")).expect("CRLF-terminated markers must parse");
+
+        assert!(layer.defaults.contains("$primary: blue !default;"));
+        assert!(layer.rules.contains(".container"));
+        assert!(
+            !layer.defaults.contains('\r') && !layer.rules.contains('\r'),
+            "residual CR leaked into layer content fed to grass"
+        );
+    }
+
     #[test]
     fn test_merge_layers_defaults_reversed() {
         let framework = SassLayer {

crates/quarto-sass/src/layer.rs

@@ -166,7 +166,12 @@ impl EmbeddedResources {
 
     /// Strip the resource prefix from a path to get the relative path.
     fn strip_prefix(&self, path: &Path) -> String {
-        let path_str = path.to_string_lossy();
+        // Normalize to forward slashes first. Embedded keys are forward-slash
+        // (include_dir normalizes at build time), but grass resolves @import
+        // via PathBuf::join, which injects a native `\` on Windows into our
+        // virtual load paths. Canonicalize here — the single embedded-lookup
+        // boundary — so the string surgery below matches the keys on every OS.
+        let path_str = quarto_util::path::to_forward_slashes(path);
 
         // Strip absolute resource prefix if present
         let without_abs_prefix = path_str
@@ -229,7 +234,7 @@ impl quarto_system_runtime::EmbeddedResourceProvider for EmbeddedResources {
 /// include_dir, so we don't need to track or add prefixes.
 fn collect_files(dir: &Dir<'static>, files: &mut HashSet<String>) {
     for file in dir.files() {
-        files.insert(file.path().to_string_lossy().to_string());
+        files.insert(quarto_util::path::to_forward_slashes(file.path()));
     }
 
     for subdir in dir.dirs() {
@@ -243,7 +248,7 @@ fn collect_files(dir: &Dir<'static>, files: &mut HashSet<String>) {
 /// include_dir, so we don't need to track or add prefixes.
 fn collect_directories(dir: &Dir<'static>, dirs: &mut HashSet<String>) {
     for subdir in dir.dirs() {
-        dirs.insert(subdir.path().to_string_lossy().to_string());
+        dirs.insert(quarto_util::path::to_forward_slashes(subdir.path()));
         collect_directories(subdir, dirs);
     }
 }
@@ -440,6 +445,26 @@ mod tests {
         )));
     }
 
+    #[test]
+    fn test_is_file_tolerates_backslash_separators() {
+        // On Windows, grass resolves @import by PathBuf::join-ing a virtual
+        // load path with the import, which inserts a backslash at the join
+        // boundary (e.g. Bootstrap's _mixins.scss does `@import "vendor/rfs"`).
+        // The embedded lookup keys are forward-slash (include_dir normalizes
+        // at build time), so the lookup must tolerate the backslash shape.
+        // Built inline so it reproduces on Linux CI, where `\` is an ordinary
+        // filename byte rather than a separator.
+        let windows_shape = Path::new("/__quarto_resources__/bootstrap/scss\\vendor/_rfs.scss");
+        assert!(
+            BOOTSTRAP_RESOURCES.is_file(windows_shape),
+            "backslash-separated virtual path must resolve to the embedded file"
+        );
+        assert!(
+            BOOTSTRAP_RESOURCES.read(windows_shape).is_some(),
+            "backslash-separated virtual path must be readable"
+        );
+    }
+
     #[test]
     fn test_full_prefix() {
         assert_eq!(