ci: pick the docs-preview toolchain from the PR checkout - #3081
Conversation
Sorry, something went wrong.
docs-preview.yml runs on pull_request_target, so the workflow file always comes from the base branch while the code being built comes from the PR head. Hardcoding the build recipe here breaks previews for any head whose toolchain differs from the base — including the pending MkDocs-to-Zensical migration, whose preview can currently only go green after it merges. Detect the recipe from the checkout instead: heads that ship scripts/docs/build.sh (a complete recipe, dependency sync included) build with it; anything else keeps the current MkDocs path, dependency sync and all. Fail the artifact upload when the build arm writes nothing to site/ rather than surfacing as a download error in the deploy job.
There was a problem hiding this comment.
I didn't find any bugs — the fallback arm is byte-equivalent to the current step and the new arm only activates for heads shipping scripts/docs/build.sh — but this changes a pull_request_target workflow's execution path, so it warrants a human maintainer's sign-off.
Extended reasoning...Overview
This PR modifies a single file, .github/workflows/docs-preview.yml. It replaces the hardcoded MkDocs build step with a two-arm recipe: if the checked-out PR head ships scripts/docs/build.sh, run it; otherwise run the existing uv sync --frozen --group docs + mkdocs build commands, byte-equivalent to the step it replaces. It also adds scripts/docs/** to the trigger paths, sets if-no-files-found: error on the artifact upload, and expands the security header comment. On main today the change is a no-op since scripts/docs/build.sh does not exist (verified in the checkout).
Security risks
This is a pull_request_target workflow, which is inherently security-sensitive. The new arm executes a head-supplied shell script on the build runner. The PR's argument that this adds no new capability appears sound: the build job already executes arbitrary head-controlled Python (mkdocstrings imports src/mcp, !!python/name: config directives), runs with permissions: contents: read, persist-credentials: false, cache saving disabled, and no secrets — the Cloudflare token lives only in the separate deploy job, which consumes the static artifact. The authorize gate (admin sender / admin-maintainer commenter) is untouched. I could not identify a concrete new attack vector, but changes to the execution path of a dangerous-trigger workflow deserve human verification of that reasoning.
Level of scrutiny
High, despite the small diff. CI workflow files — especially pull_request_target ones — are production trust-boundary code where a subtle mistake (e.g., a future arm inheriting secrets, or the head-script arm being reachable in an unintended job) has outsized consequences. The bug hunting system found no bugs, and I found none either, but the correct bar here is a maintainer who owns the CI security posture confirming that head-supplied build scripts are an acceptable design.
Other factors
The build.sh arm is untestable until #3073 lands, so its end-to-end verification is deferred to that PR's preview run — a reasonable but human-judgment-dependent tradeoff. There is also a merge-ordering dependency with #3073 and a cross-referenced duplicate of the detection logic in scripts/build-docs.sh on that branch, which is coordination a human should be aware of. No prior reviews or comments exist on the timeline.
Sorry, something went wrong.
Unblocks docs previews for #3073 (and any future toolchain change) by resolving the pull_request_target chicken-and-egg: this workflow file always runs from the base branch, so a hardcoded build recipe can only preview heads that share the base's toolchain — #3073's preview check is currently guaranteed-red until after it merges.
Motivation and Context
The build job checks out the PR head but runs base-branch workflow steps. Instead of hardcoding the MkDocs commands, detect the recipe from the checkout: heads that ship scripts/docs/build.sh (a complete recipe, dependency sync included — arrives with #3073) build with it; all other heads — including v1.x heads previewed via /preview-docs — keep today's MkDocs path unchanged. Same pattern as build_site() in scripts/build-docs.sh on #3073; the two copies cross-reference each other.
On main today this is a no-op: scripts/docs/build.sh doesn't exist, so every preview takes the fallback arm, which is byte-equivalent to the current step.
Also sets if-no-files-found: error on the artifact upload, so a build arm that violates the site/ output contract fails loudly in build instead of as a confusing artifact-download error in deploy.
Security posture is unchanged: the build job already executes head-controlled Python (mkdocstrings imports src/mcp, !!python/name: config directives), is admin-gated by authorize, and never shares a runner with the Cloudflare token. The header comment now spells out the head-supplied-script vector explicitly.
How Has This Been Tested?
The fallback arm is the existing command relocated (verified by inspection; it's what every preview on main runs today). The build.sh arm is exercised by #3073's preview run once this merges — that run is the end-to-end verification, and #3073's own CI builds the same script from its branch.
Breaking Changes
None.
Types of changes
Checklist
Additional context
Merge order: this first, then #3073 — its preview goes green on the next push to that branch afterwards.
AI Disclaimer