Fix quarto preview crash on knitr document in a project subdirectory#14699
Open
cderv wants to merge 9 commits into
Open
Fix quarto preview crash on knitr document in a project subdirectory#14699cderv wants to merge 9 commits into
cderv wants to merge 9 commits into
Conversation
fileExecutionEngineAndTarget accepted both relative and absolute paths, so the cached ExecutionTarget could hold either form depending on the caller. Its fileInformationCache keys are normalized to absolute, so when preview seeds the shared context via format resolution with a cwd-relative path and renderProject later looks the file up by its absolute path, the two collide on the same key and the render inherits the stale relative source/input. The knitr engine runs its R subprocess with cwd set to the project dir, where a subdirectory-relative filename does not resolve, so rmarkdown:::abs_path(input) throws. Normalizing the path once at this single convergence point makes target.source and target.input absolute for every caller and engine, consistent with the cache key and with project renders (which already pass absolute paths). This establishes the always-absolute contract discussed in #14151. Fixes #14683 (knitr preview of a document in a project subdirectory, run from that subdirectory with a bare filename, as RStudio's Render button invokes it). Fixes #12401 (QUARTO_DOCUMENT_PATH was relative for single-file renders but absolute for project renders).
…14683) An automated `quarto preview` test is not practical (preview starts a blocking server), so document the real-CLI reproduction as a manual test-matrix entry: previewing a knitr document from its own subdirectory with a bare filename (the RStudio Render-button shape) must render without the rmarkdown:::abs_path failure, and QUARTO_DOCUMENT_PATH must resolve to the doc's subdirectory.
The T33 matrix entry described its setup inline but shipped no fixture, so the test was not self-contained. Add knitr-subdir-14683/ (a website project with a knitr doc at labs/doc.qmd that echoes QUARTO_DOCUMENT_PATH) and point T33 at it, so a tester can run the real-CLI reproduction directly.
…e false-pass The knitr e2e fixture was created at module scope (unconditional) but only cleaned up in teardown, which the harness skips along with cwd/setup whenever the Rscript prereq is false — leaking a temp dir on any run without R. Move creation into the prereq closure so it only happens when the test will actually run. Both the #14683 cache-invariant guard and the #12401 test derived their "relative" input via relative(Deno.cwd(), absFile). path.relative() falls back to returning the absolute path unchanged when its arguments are on different Windows drives, which would let these tests pass even without the fix on a machine/CI job where the OS temp drive differs from the checkout drive (see test-smokes.yml's existing "Fix temp dir to use runner one (windows)" step, added for exactly this drive-mismatch reason). Switch both to TestContext.cwd with a bare filename, removing the dependency on Deno.cwd()'s relationship to the temp directory's drive entirely.
Collaborator
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
The missing-file assertion fired before the result.error check, so a CI-only failure gave no diagnostic beyond "no HTML produced". Include the error message/stack in the existence assertion so the next CI run shows the actual cause.
Probes whether Rscript can load rmarkdown/knitr from callR's actual spawn cwd (the project dir) before the real render runs, so a CI-only failure shows package/environment state instead of just "no HTML". Uses a script file rather than inline -e code: an -e argument with embedded quotes gets mis-tokenized by cmd.exe when Rscript resolves to a .bat shim (e.g. rig-managed R installs) — the same reason callR always passes a script file, never inline -e.
The diagnostic probe (previous commit) confirmed the CI failure: R's .Rprofile lookup is cwd-exact with no parent-directory search, and the regression's own repro shape requires the process cwd to be a scratch directory outside the repo (the doc's subdirectory). That cwd never finds tests/.Rprofile, so renv never activates, and on CI — where rmarkdown/knitr live only in tests/renv's project library — R fails with "there is no package called 'rmarkdown'" before ever reaching the #14683 code path. It never showed up locally because a typical dev R install has these packages in a library that's on .libPaths() regardless of renv activation. setup() now writes a .Rprofile into the fixture project directory that sets RENV_PROJECT and sources the real tests/renv/activate.R, so renv activates against the actual project/library regardless of the fixture's own cwd. Removes the now-unneeded diagnostic probe. Adds tests/unit/CLAUDE.md documenting the mechanism for future debugging, since the failure mode (fast, silent, error message swallowed by renderProject's error wrapping) is not obvious from the test output alone.
tests/unit/CLAUDE.md would have loaded for every file touched under tests/unit/, but the renv-activation workaround only matters for preview-subdir-knitr-cwd.test.ts. Move it to .claude/rules/testing/ with paths frontmatter scoped to that single file, matching how the other testing rules (smoke-all-tests.md, playwright-tests.md) are scoped.
…omment Per repo convention, .claude/rules/testing/ files are terse pointers and llm-docs/testing-patterns.md holds the actual explanation — llm-docs explicitly excludes issue-specific notes. Move the detailed mechanism there as a generalized "R Tests That Change Working Directory" entry (no issue number, survives independently of #14683), trim the rule file back to a short pointer, and trim the test's own setup() comment to a one-line pointer plus what's specific to this fixture.
Member
Author
|
I think I still need to run all the manual preview testing before merging this... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a regression from #13804 (merged 2026-01-22), live in the current stable release (1.9.38) as well as main.
When previewing a knitr document that lives in a project subdirectory, running
quarto previewfrom that subdirectory with a bare filename — the shape RStudio's Render button uses — fails with:Root Cause
#13804 moved preview to share one
ProjectContext(and itsfileInformationCache) between format-resolution and the render, instead of rebuilding a fresh context for each, and normalized the cache's keys to absolute paths so they wouldn't miss on Windows path-separator differences. But the cachedExecutionTarget.input/source(set by each engine'starget()from the raw caller-supplied path) was never normalized to match. Preview's format-resolution step seeds the cache with the cwd-relative bare filename; the laterrenderProjectcall looks the same file up by its absolute path, collides on the same normalized cache key, and inherits the stale relativetarget.input. The knitr subprocess runs with cwd set to the project directory, where a subdirectory-relative filename doesn't resolve, soabs_path()throws.Fix
Normalize the path to absolute once, at the single convergence point (top of
fileExecutionEngineAndTargetinsrc/execute/engine.ts), sotarget.source/target.inputare always absolute regardless of how the caller spelled the path — consistent with project renders, which were already absolute. This also fixes #12401:QUARTO_DOCUMENT_PATHwas relative for single-file renders but absolute for project renders, since both derive fromtarget.source.A separate, narrower-scoped PR will follow to backport this to the v1.9 branch.
Test Plan
quarto previewcan't be exercised by the automated suite (it starts a blocking server) — a manual regression check is added attests/docs/manual/preview/README.md(T33): preview a knitr document from its own subdirectory with a bare filename; must render without theabs_pathfailure, andQUARTO_DOCUMENT_PATHmust resolve to the doc's subdirectory.Fixes #14683, fixes #12401, fixes #14151