fix: split preview workflow so fork PRs can deploy to gh-pages

[d-morrison]

Summary

• The old preview.yml ran entirely under pull_request, giving fork PRs a read-only GITHUB_TOKEN that can't push to gh-pages (403 error seen in PR #846).
• Splits into two workflows using the standard workflow_run pattern:
  • preview.yml (name: Quarto Preview Build) — triggers on pull_request, contents: read only. Builds the site and uploads a pr-preview-site artifact with the rendered site + PR metadata (meta/pr-number.txt, meta/action.txt).
  • preview-deploy.yml (name: Quarto Preview Deploy) — triggers via workflow_run on build completion. Runs in the base-repo context with contents: write, downloads the artifact, pushes to gh-pages, and posts PR comments.
• PR number is embedded in the artifact because github.event.workflow_run.pull_requests is empty for fork PRs (GitHub strips it for security).

Test plan

• Open a PR from a fork and confirm Quarto Preview Build passes and Quarto Preview Deploy successfully pushes the preview and posts the preview URL comment
• Close a fork PR and confirm the deploy workflow removes the preview and updates the comment
• Open a PR from a non-fork branch and confirm both workflows behave identically to before

https://claude.ai/code/session_01CjeVhx6eX7p8W5kLdmMNht

---

Generated by Claude Code

[claude]

Claude finished @d-morrison's task in 3m 20s —— View job

---

Code Review: PR #906 — Split preview workflow for fork PR gh-pages access

• Gather context and read changed files
• Analyze for bugs, security issues, and best practices

---

Overall verdict

The approach is correct — the workflow_run split is the right solution for the fork-PR 403 problem. One critical bug will silently break the "PR closed → remove preview" path. Two minor issues noted below.

---

Critical bug — artifact path mismatch for remove case

preview.yml lines 170–176 vs. preview-deploy.yml lines 31–35

For the closed event, the build workflow uploads only the meta/ directory:

# preview.yml:175 (remove case)
path: _preview_upload/meta/

actions/upload-artifact@v4 stores paths relative to the given root, so the artifact contains:

pr-number.txt      ← root of artifact
action.txt

But the deploy workflow always reads:

# preview-deploy.yml:34
echo "pr-number=$(cat _preview_download/meta/pr-number.txt)" >> "$GITHUB_OUTPUT"

After download to _preview_download/, those files land at _preview_download/pr-number.txt — not _preview_download/meta/pr-number.txt. The Read PR metadata step will produce empty outputs, pr-number will be blank, and rossjrw/pr-preview-action will either no-op or error.

Fix: Change preview.yml line 175 to use the same root as the deploy case:

-          path: _preview_upload/meta/
+          path: _preview_upload/

Because _preview_upload/site/ doesn't exist for the closed path, only meta/ gets uploaded — but with the correct meta/ subdirectory preserved in the artifact. Fix this →

---

Minor — _preview_upload/site/ is never created

preview.yml lines 46–48 and 158–160

The Save PR metadata step runs mkdir -p _preview_upload/meta (creates only the meta subdirectory). The later Stage site for upload step then does:

cp -r _site/. _preview_upload/site/

GNU cp on Ubuntu will usually create a missing destination directory when the source ends with /., but this is an implementation detail rather than guaranteed POSIX behavior. An explicit mkdir -p _preview_upload/site/ before the cp is safer. Fix this →

---

Minor — redundant explicit delete before recreate: true

preview-deploy.yml lines 56–78

The "Delete old preview comment" step (lines 56–63) uses delete: true to remove the existing comment, then the very next "Comment with preview link" step (lines 65–78) uses recreate: true, which already performs its own delete-then-create cycle. The explicit delete step is redundant and adds an extra API call with a continue-on-error: true that masks any failure. Remove the "Delete old preview comment" step and rely solely on recreate: true. Fix this →

---

Design note — security posture is sound

The Save PR metadata step runs before actions/checkout, so no fork code can tamper with pr-number.txt. The value ${{ github.event.pull_request.number }} is GitHub-provided metadata evaluated from the base repo's workflow file — the fork can't inject a different PR number. The overall design correctly follows the GitHub-recommended workflow_run pattern for privileged operations on fork PRs.

[claude]

Claude finished @d-morrison's task in 3m 56s —— View job

---

Code Review: PR #906 — Split preview workflow for fork PR gh-pages access

• Gather context and read changed files
• Verify previous review findings are addressed
• Analyze current code for new issues

---

Status of prior findings (commit 5a70f03)

All three issues from the previous review are confirmed fixed:

• ✅ Critical: Upload preview artifact (remove) now uses path: _preview_upload/ (not _preview_upload/meta/), so the meta/ subdirectory is preserved in the artifact and correctly matches _preview_download/meta/pr-number.txt in the deploy workflow.
• ✅ Minor: mkdir -p _preview_upload/site/ is now explicit before the cp (line 161).
• ✅ Minor: The redundant Delete old preview comment step with delete: true is gone; recreate: true handles the cycle.

---

New findings

Medium — No concurrency group in preview-deploy.yml

preview-deploy.yml — jobs: block (line 9)

preview.yml has concurrency: group: preview-${{ github.ref }}, cancel-in-progress: true, which prevents two build runs from racing. But preview-deploy.yml has no concurrency key at all. If two build runs complete close together (e.g., a fast push followed by a label event), two deploy jobs can run simultaneously and both attempt to push to gh-pages — the second push will fail with a non-fast-forward error, and the preview may end up in a partially-written state.

Suggested fix:

concurrency:
  group: preview-deploy-${{ github.event.workflow_run.head_branch }}
  cancel-in-progress: false   # don't cancel mid-push; let the latest queued run proceed

cancel-in-progress: false is safer here because cancelling a deploy mid-git push could corrupt the gh-pages branch. Fix this →

---

Minor — "Preview removed" comment fires even when there was no preview to remove

preview-deploy.yml lines 71–83

The Remove preview comment step fires unconditionally when steps.meta.outputs.action == 'remove'. If the PR never had a successful preview deployed (e.g., the first build failed), the comment will say "Preview removed" even though nothing was ever shown. Compare the deploy comment, which correctly guards on steps.preview.outputs.deployment-action == 'deploy'. The remove comment should similarly guard on steps.preview-remove.outputs.deployment-action == 'remove':

-        if: steps.meta.outputs.action == 'remove'
+        if: steps.meta.outputs.action == 'remove' && steps.preview-remove.outputs.deployment-action == 'remove'

Fix this →

---

Observation — preview-deploy.yml in paths: trigger causes unnecessary builds

preview.yml line 18

- '.github/workflows/preview-deploy.yml'

Changes to the deploy workflow trigger a full Quarto build that uploads an artifact and immediately triggers a deploy that does nothing useful (since no content changed). This is harmless but wasteful. It was probably copied from the old pattern where publish.yml was listed, where it made sense because that workflow was doing the render. Now that the deploy is a separate receiver, removing this path filter would be cleaner.

---

Overall verdict

The design is correct and all prior findings are resolved. The medium concurrency issue is the most actionable remaining item — concurrent gh-pages pushes are a real failure mode. The two minor items are low-risk quality improvements.

[claude]

Claude finished @d-morrison's task in 2m 40s —— View job

---

Code Review: PR #906 — Split preview workflow for fork PR gh-pages access

• Gather context
• Read current workflow files
• Verify second-round findings are addressed
• Analyze for new issues
• Post review

---

Status of prior findings (commit e48e3e2)

All three issues from the second round are confirmed fixed:

• ✅ Medium: Concurrency group added to preview-deploy.yml — using preview-deploy-${{ github.event.workflow_run.head_repository.id }}-${{ github.event.workflow_run.head_branch }} with cancel-in-progress: false. The head_repository.id component is a good addition beyond the original suggestion — it correctly differentiates between two forks that happen to share a branch name.
• ✅ Minor: "Remove preview comment" step (line 80) now guards on both steps.meta.outputs.action == 'remove' and steps.preview-remove.outputs.deployment-action == 'remove'.
• ✅ Observation: preview-deploy.yml was not added to the paths: filter. Additionally, publish.yml was also cleaned out of the paths list — a sensible housekeeping improvement.

---

New findings

Minor — echo with \n in debug step won't render as newline

preview.yml lines 145–148

echo "contents of _site:\n"

The \n is printed literally (as \n) rather than as a newline. The step is debug-only so there's no functional impact, but the intent is clearly to print a blank line between sections. Fix with printf or add -e:

-          echo "contents of _site:\n"
+          echo "contents of _site:"
           ls _site/
-          echo "contents of .:\n"
+          echo "contents of .:"

Fix this →

---

Informational — new actions in deploy workflow use floating version tags

preview-deploy.yml lines 46 and 63

rossjrw/pr-preview-action@v1 and marocchino/sticky-pull-request-comment@v3 are pinned to mutable version tags. A supply-chain compromise or unintended update to either action would silently change behavior in the privileged deploy workflow (which has contents: write). Pinning to commit SHAs is the hardened approach:

rossjrw/pr-preview-action@<commit-sha>  # v1.x.y
marocchino/sticky-pull-request-comment@<commit-sha>  # v3.x.y

This is consistent with the existing style in preview.yml, so it's a repo-wide pattern rather than a new problem introduced here. Noting it since the deploy workflow carries elevated permissions.

---

Overall verdict

The design is correct, all prior findings are resolved, and no new critical or medium issues are present. The PR is ready to merge. The two items above are both low-priority quality notes, not blockers.

[claude]

Claude finished @d-morrison's task in 3m 3s —— View job

---

Code Review: PR #906 — Split preview workflow for fork PR gh-pages access

• Gather context and read prior review history
• Read current workflow files
• Verify all prior findings are resolved
• Analyze for new issues
• Post review

---

Status of all prior findings

All findings from rounds 1–3 are confirmed resolved in the current code:

• ✅ Critical (round 1): Artifact path mismatch — Upload preview artifact (remove) now uses path: _preview_upload/, preserving meta/ layout.
• ✅ Minor (round 1): Explicit mkdir -p _preview_upload/site/ added before cp (line 160).
• ✅ Minor (round 1): Redundant Delete old preview comment step removed; recreate: true handles the cycle.
• ✅ Medium (round 2): Concurrency group added to deploy workflow with head_repository.id + head_branch and cancel-in-progress: false.
• ✅ Minor (round 2): "Remove preview comment" step now guards on steps.preview-remove.outputs.deployment-action == 'remove' (line 80).
• ✅ Observation (round 2): preview-deploy.yml not in paths: filter.
• ✅ Minor (round 3): Literal \n removed from echo debug lines (commit 50a7402).

---

New findings

Minor — Deploy workflow fires on skipped-build runs, artifact download fails

preview-deploy.yml line 24 / preview.yml line 36

When a PR with matching paths: files receives an unrecognized label (e.g. "wontfix"), the build workflow triggers (path filter matches the PR's changed files) but the build job is skipped due to line 36's if guard. Depending on GitHub's behavior, the workflow may report conclusion == 'success' even with all jobs skipped — causing the deploy workflow to fire. The Download preview artifact step then fails (no artifact was uploaded), producing a noisy failed deploy run.

The fix is to add an early-exit job or use the workflow_run filter branches: — but the simplest guard is to add a check for the artifact's existence before download. Alternatively, using the (undocumented but real) behavior: GitHub reports a workflow conclusion of success only when at least one job actually ran and succeeded; all-skipped workflows often report skipped. If that's the case in practice, the deploy guard at line 24 is already sufficient. Worth verifying with a test run using an unrecognized label.

Investigate this →

---

Observation — paths: filter could leave stale preview deployed on edge-case close

preview.yml lines 11–22

The paths: filter applies to all pull_request event types, including closed. If a PR's .qmd changes are squashed/rebased out before the PR is closed (so the final HEAD no longer touches any path in the filter), the closed event won't trigger the build workflow, and the deployed preview is never cleaned up. This is a pre-existing design issue not introduced by this PR, but is worth a follow-up issue. A common mitigation is a separate cleanup workflow that runs on pull_request: closed with no paths: filter, uploads only the remove metadata.

---

Informational — Floating version tags in privileged deploy workflow (unchanged from round 3)

preview-deploy.yml lines 46 and 64

rossjrw/pr-preview-action@v1 and marocchino/sticky-pull-request-comment@v3 remain pinned to mutable version tags. Since the deploy workflow carries contents: write and pull-requests: write, a supply-chain compromise to either action would be highly privileged. Pinning to commit SHAs is the hardened approach. Not a blocker, but worth a follow-up.

---

Overall verdict

The design is correct and secure. All prior findings are resolved. The PR is ready to merge. The new items above are all low-priority: one minor noise/UX issue (skipped-build deploy failure), one pre-existing design gap (stale preview on edge-case close), and one carry-over supply-chain hygiene note. None are blockers.

[claude]

Claude finished @d-morrison's task in 6m 2s —— View job

---

Code Review: PR #906 — Split preview workflow for fork PR gh-pages access

• Gather context and read prior review history
• Read current workflow files
• Verify all prior findings are resolved
• Analyze for new issues
• Post review

---

Status of all prior findings (rounds 1–4)

All findings from rounds 1–4 are confirmed resolved in the current code:

• ✅ Critical (round 1): Artifact path mismatch — both upload steps now use path: _preview_upload/, preserving the meta/ layout.
• ✅ Minor (round 1): Explicit mkdir -p _preview_upload/site/ before cp (preview.yml line 160).
• ✅ Minor (round 1): Redundant "Delete old preview comment" step removed; recreate: true handles the cycle.
• ✅ Medium (round 2): Concurrency group added to deploy workflow with head_repository.id + head_branch and cancel-in-progress: false (preview-deploy.yml lines 11–13).
• ✅ Minor (round 2): "Remove preview comment" step now guards on steps.preview-remove.outputs.deployment-action == 'remove' (preview-deploy.yml line 103).
• ✅ Observation (round 2): preview-deploy.yml not added to paths: filter; publish.yml also cleaned up.
• ✅ Minor (round 3): Literal \n removed from debug echo lines (preview.yml lines 145–148).
• ✅ Minor (round 4): Skipped-build deploy noise — fully addressed by the new gate job (preview-deploy.yml lines 20–36), which probes the GitHub API to confirm the artifact exists before allowing the deploy job to proceed.

---

New findings

Minor — deployment-action == 'deploy' skips comment updates on subsequent pushes

preview-deploy.yml line 88

if: steps.meta.outputs.action == 'deploy' && steps.preview.outputs.deployment-action == 'deploy'

rossjrw/pr-preview-action outputs deployment-action: deploy when it creates a new preview, and deployment-action: update when it refreshes an existing one. Because the condition requires == 'deploy', the "Comment with preview link" step is silently skipped for every push after the first — the "Built at" timestamp in the sticky comment becomes stale even though the preview itself is current.

The fix depends on intent:

• If the goal is to refresh the comment on every push (recommended — keeps the timestamp accurate):

  -        if: steps.meta.outputs.action == 'deploy' && steps.preview.outputs.deployment-action == 'deploy'
  +        if: steps.meta.outputs.action == 'deploy' && steps.preview.outputs.deployment-action != 'none'

• If the goal is "post only on first deploy, don't spam on updates," the current code is correct — please clarify in a comment.

Fix this →

---

Informational — Gate job gh api call is not paginated

preview-deploy.yml line 33

count=$(gh api \
  "repos/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }}/artifacts" \
  --jq '[.artifacts[] | select(.name == "pr-preview-site")] | length')

The GitHub REST API returns up to 30 items per page by default. With only one expected artifact per build run this is fine in practice, but --paginate would make the check robust against edge-case accumulation (e.g. re-runs that somehow produce extra artifacts):

-  count=$(gh api \
-    "repos/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }}/artifacts" \
-    --jq '[.artifacts[] | select(.name == "pr-preview-site")] | length')
+  count=$(gh api --paginate \
+    "repos/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }}/artifacts" \
+    --jq '[.artifacts[] | select(.name == "pr-preview-site")] | length' | \
+    paste -sd+ | bc)

Not a blocker — the single-artifact invariant holds in every normal run.

---

Informational — Floating version tags in privileged deploy workflow (carry-over from round 3)

preview-deploy.yml lines 69 and 87

rossjrw/pr-preview-action@v1 and marocchino/sticky-pull-request-comment@v3 remain pinned to mutable version tags in the workflow that carries contents: write and pull-requests: write. Pinning to commit SHAs is the hardened approach for supply-chain hygiene. Not a blocker given these are well-known, widely-used actions.

---

Observation — Stale preview on edge-case close (carry-over from round 4, pre-existing)

preview.yml lines 11–22

The paths: filter applies to closed events too. If a PR's files are squashed/rebased away before close, the closed event won't match the filter, the build workflow won't fire, and the deployed preview is never removed. This is a pre-existing design limitation not introduced by this PR — worth a follow-up issue rather than a blocker here.

---

Note on inline comments

I attempted to post findings as inline review comments per the --comment flag, but no inline PR review comment tool is available in this session. Findings are anchored to specific lines above.

---

Overall verdict

The design is correct, the security posture is sound, and all critical/medium findings from prior rounds are resolved. The PR is ready to merge. The one actionable new item (deployment-action == 'deploy' condition on line 88) is a minor UX issue — worth a quick clarification or fix before merge, but not a blocker if the "post only on first deploy" behavior is intentional.

[claude]

Claude finished @d-morrison's task in 2m 15s —— View job

---

Code Review: PR #906 — Split preview workflow for fork PR gh-pages access

• Gather context and read prior review history
• Read current workflow files
• Verify round 5 findings are addressed
• Analyze for new issues
• Post review

---

Status of all prior findings (rounds 1–5)

All findings through round 5 are confirmed resolved in the current code:

• ✅ Critical (round 1): Artifact path mismatch — both upload steps use path: _preview_upload/, preserving meta/ layout.
• ✅ Minor (round 1): Explicit mkdir -p _preview_upload/site/ before cp (preview.yml line 160).
• ✅ Minor (round 1): Redundant "Delete old preview comment" step removed; recreate: true handles the cycle.
• ✅ Medium (round 2): Concurrency group in deploy workflow with head_repository.id + head_branch and cancel-in-progress: false (preview-deploy.yml lines 11–13).
• ✅ Minor (round 2): "Remove preview comment" step guards on steps.preview-remove.outputs.deployment-action == 'remove' (line 103).
• ✅ Observation (round 2): preview-deploy.yml not in paths: filter.
• ✅ Minor (round 3): Literal \n removed from debug echo lines.
• ✅ Minor (round 4): Skipped-build deploy noise handled by the gate job (preview-deploy.yml lines 20–36).
• ✅ Minor (round 5): deployment-action == 'deploy' → now != 'none' (preview-deploy.yml line 88). The sticky comment timestamp will now update on every push (deploy or update), not just the first deploy.

---

New findings

No new critical, medium, or minor issues found.

---

Carry-over informational items (unchanged, not blockers)

Informational — Gate job gh api call is not paginated (preview-deploy.yml line 33)

The GitHub API returns up to 30 items per page by default. In practice a single build run will never accumulate more than 1 artifact with this name, so this is a non-issue in any realistic scenario. Fix this →

Informational — Floating version tags in privileged deploy workflow (preview-deploy.yml lines 69 and 102)

rossjrw/pr-preview-action@v1 and marocchino/sticky-pull-request-comment@v3 are mutable tags in the workflow that holds contents: write and pull-requests: write. Pinning to commit SHAs is the supply-chain-hardened approach. Fix this →

Observation — Stale preview on edge-case close (preview.yml lines 11–22, pre-existing)

The paths: filter applies to closed events too. If a PR's files are squashed away before close, the closed event won't match the filter and the deployed preview is never removed. Pre-existing design limitation, worth a follow-up issue.

---

Overall verdict

The design is correct, the security posture is sound, and all critical/medium/minor findings from all prior rounds are resolved. The deployment-action != 'none' fix ensures the sticky comment timestamp stays current on every push. This PR is ready to merge.

[claude]

Claude finished @d-morrison's task in 2m 21s —— View job

---

Code Review: PR #906 — Split preview workflow for fork PR gh-pages access

• Gather context and read prior review history
• Read current workflow files
• Analyze for issues
• Post review findings

---

Status of all prior findings (rounds 1–5)

All findings from the six prior review rounds are confirmed resolved in the merged code:

• ✅ Critical (R1): Artifact path mismatch — both upload steps use path: _preview_upload/, preserving meta/ layout.
• ✅ Minor (R1): Explicit mkdir -p _preview_upload/site/ before cp (preview.yml:160).
• ✅ Minor (R1): Redundant "Delete old preview comment" step removed; recreate: true handles the cycle.
• ✅ Medium (R2): Concurrency group in deploy workflow with head_repository.id + head_branch and cancel-in-progress: false (preview-deploy.yml:11–13).
• ✅ Minor (R2): "Remove preview comment" step guards on steps.preview-remove.outputs.deployment-action == 'remove' (preview-deploy.yml:103).
• ✅ Observation (R2): preview-deploy.yml not in paths: filter; publish.yml also cleaned out.
• ✅ Minor (R3): Literal \n removed from debug echo lines (preview.yml:145–148).
• ✅ Minor (R4): Skipped-build deploy noise handled by the gate job with artifact existence check (preview-deploy.yml:20–36).
• ✅ Minor (R5): deployment-action == 'deploy' → now != 'none' so sticky comment updates on every push (preview-deploy.yml:88).

---

New findings

No new critical or medium issues found. The security posture is sound: PR metadata is written before actions/checkout (preview.yml:45–49), so fork code cannot tamper with the PR number or action type embedded in the artifact.

Informational — Floating version tags in privileged deploy workflow (carry-over from R3)

preview-deploy.yml:69, preview-deploy.yml:87

rossjrw/pr-preview-action@v1 and marocchino/sticky-pull-request-comment@v3 remain on mutable version tags in the workflow that holds contents: write and pull-requests: write. A supply-chain compromise to either action would be highly privileged. Pinning to commit SHAs is the hardened approach. Not a blocker — these are well-known, widely-used actions. Fix this →

Informational — gh api call in gate job is not paginated (carry-over from R5)

preview-deploy.yml:33–35

The gh api call listing artifacts defaults to 30 results per page. In practice a single build run will produce exactly one pr-preview-site artifact, so this is never an issue. Noted for completeness.

Observation — Stale preview on edge-case close (carry-over from R4, pre-existing)

preview.yml:11–22

The paths: filter applies to closed events. If a PR's files are squashed away before the PR is closed (so HEAD no longer touches any listed path), the closed event won't trigger the build workflow and the deployed preview is never cleaned up. This is a pre-existing design limitation not introduced by this PR — worth a follow-up issue tracking a dedicated cleanup workflow on closed with no paths: filter.

---

Overall verdict

Design is correct, security posture is sound, and all critical/medium/minor findings from all prior rounds are resolved. The three remaining items above are all carry-over informational notes, not blockers. The merged code is in good shape.

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.

_{2026-06-16 17:06 UTC}