The new gh pr view call will require pull-requests: read permission on the job token. Right now the workflow-level permissions are {} and this job only grants contents: read, so gh pr view is likely to 403 on workflow_dispatch runs. Add pull-requests: read (or move it to workflow-level if preferred) so PR metadata resolution works reliably.

The metadata resolution step doesn’t fail fast if the PR number is invalid or if gh pr view returns an empty HEAD_SHA. In that case actions/checkout will fall back to the default ref and you may deploy/comment against the wrong commit. Consider adding set -euo pipefail and explicit validation (non-empty PR_NUMBER and HEAD_SHA, with a clear error) before writing outputs.

deploy-preview now runs unconditionally for workflow_dispatch, bypassing the existing same-repo guard. That makes it possible to deploy previews (using the Cloudflare token) for fork-based PRs if someone manually dispatches the workflow with that PR number. Consider enforcing the same-repo check for dispatch too (e.g., resolve headRepository.fullName in the metadata step and fail/skip when it doesn’t match github.repository, or add an explicit input to allow forks).

⚠️ Potential issue | 🟠 Major

Keep workflow_dispatch deploys limited to open, same-repo PRs.

Line 170 short-circuits the old eligibility checks for every manual run. That means a closed PR can get a fresh preview/comment even though cleanup-preview never runs for dispatch, and cross-repo PRs are no longer filtered out. Resolve the PR state/repo in the metadata step and gate this job on those outputs as well.

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/pages-preview.yml around lines 170 - 172, The current job
conditional short-circuits eligibility for manual runs; add a metadata step that
resolves the pull_request state and whether the head repo matches the target
repo (set outputs like pr_state and same_repo) and then change the job if to
require those outputs for workflow_dispatch as well (e.g. only proceed when
needs.metadata.outputs.pr_state == 'open' and needs.metadata.outputs.same_repo
== 'true'); update the conditional that currently uses
workflow_dispatch/pull_request head checks so both manual dispatches and PR
events are gated by the metadata outputs.

🧹 Nitpick | 🔵 Trivial

Well-implemented PR metadata resolution.

This addresses the security concerns from earlier reviews:

1. Template injection mitigated: User-controlled inputs are routed through environment variables (PR_INPUT, EVENT_NAME), preventing shell injection.
2. Leading zeros prevented: Regex ^[1-9][0-9]*$ ensures canonical PR numbers (rejects 00123).
3. Closed PRs rejected: Dispatch runs fail fast if PR is not OPEN.
4. Cross-repo PRs filtered: same_repo output enables downstream gating.

One minor hardening suggestion: jq returns the literal string "null" for missing/null JSON fields, which would pass the empty check at line 81. Consider adding an explicit check:

+          if [ -z "$HEAD_SHA" ] || [ "$HEAD_SHA" = "null" ]; then
-          if [ -z "$HEAD_SHA" ]; then
             echo "::error::Failed to resolve HEAD SHA for PR #$PR_NUMBER"
             exit 1
           fi

,

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/pages-preview.yml around lines 43 - 88, The jq -r call
extracting headRefOid can yield the literal string "null" which bypasses the
current empty check for HEAD_SHA; update the logic around PR_JSON and the
HEAD_SHA extraction (the jq -r '.headRefOid' invocation and the subsequent
HEAD_SHA variable) to treat both empty string and the literal "null" as missing
(e.g., after setting HEAD_SHA, test if HEAD_SHA is empty or equals "null" and
fail with the existing error path), and apply the same "null" check pattern for
any other jq-extracted variables you rely on (like IS_CROSS_REPO or PR_STATE) so
null JSON fields are treated as absent.

In workflow_dispatch runs, cross-repo PRs are not rejected—SAME_REPO is set to false, but the workflow still proceeds with the build job. This conflicts with the PR description/CLAUDE.md text that says cross-repo PRs are rejected. Either fail fast when isCrossRepository is true (exit 1) or update the documentation to say deploy is skipped for cross-repo PRs.

This bullet says dispatch runs "reject closed/cross-repo PRs", but the workflow currently only rejects non-OPEN PRs; cross-repo PRs still build and only skip deploy. Please align this documentation with the actual workflow behavior (or adjust the workflow to fail on cross-repo dispatch runs if that’s intended).

Unreachable empty-JSON guard with set -euo pipefail

The if [ -z "$PR_JSON" ] guard is effectively unreachable in any failure path. With set -euo pipefail active, if gh pr view exits non-zero (e.g., PR not found, auth error, network failure), bash exits immediately during the command substitution — PR_JSON is never assigned, and execution never reaches the empty check. The custom ::error:: annotation will therefore never fire; the workflow will just exit with gh's own error output.

If you want the custom error message to appear, you need to suppress the exit or capture the exit code explicitly:

Or alternatively, simply remove the dead guard and rely on gh's own error output (which is already clear) plus the set -e exit.

This is a comment left during a code review.
Path: .github/workflows/pages-preview.yml
Line: 64-67

Comment:
**Unreachable empty-JSON guard with `set -euo pipefail`**

The `if [ -z "$PR_JSON" ]` guard is effectively unreachable in any failure path. With `set -euo pipefail` active, if `gh pr view` exits non-zero (e.g., PR not found, auth error, network failure), bash exits immediately during the command substitution — `PR_JSON` is never assigned, and execution never reaches the empty check. The custom `::error::` annotation will therefore never fire; the workflow will just exit with gh's own error output.

If you want the custom error message to appear, you need to suppress the exit or capture the exit code explicitly:

```suggestion
            CURL_EXIT=0
            PR_JSON=$(gh pr view "$PR_NUMBER" --repo "$GH_REPOSITORY" \
              --json headRefOid,state,isCrossRepository 2>&1) || CURL_EXIT=$?
            if [ "$CURL_EXIT" -ne 0 ] || [ -z "$PR_JSON" ]; then
              echo "::error::Failed to fetch metadata for PR #$PR_NUMBER (exit $CURL_EXIT)"
              exit 1
            fi
```

Or alternatively, simply remove the dead guard and rely on gh's own error output (which is already clear) plus the `set -e` exit.

How can I resolve this? If you propose a fix, please make it concise.

Inline ${{ }} expression in env: value for URL construction

${{ needs.build.outputs.pr_number }} is used inline within the YAML string value of the PREVIEW_URL env var. While the value is validated upstream as ^[1-9][0-9]*$ (digits only), this is still an inline template expansion. Since pr_number can only be digits at this point, there is no actual injection risk here — but for consistency with the safe pattern used in the build step's PR_INPUT env var, consider assembling the URL in the script itself:

Then construct the URL in the script: block: const url = \https://pr-${prNumber}.synthorg-pr-preview.pages.dev\`;`. This is a minor consistency suggestion since pr_number is already digit-validated.

This is a comment left during a code review.
Path: .github/workflows/pages-preview.yml
Line: 237

Comment:
**Inline `${{ }}` expression in `env:` value for URL construction**

`${{ needs.build.outputs.pr_number }}` is used inline within the YAML string value of the `PREVIEW_URL` env var. While the value is validated upstream as `^[1-9][0-9]*$` (digits only), this is still an inline template expansion. Since `pr_number` can only be digits at this point, there is no actual injection risk here — but for consistency with the safe pattern used in the build step's `PR_INPUT` env var, consider assembling the URL in the script itself:

```suggestion
          PREVIEW_URL: ""
          PR_NUMBER: ${{ needs.build.outputs.pr_number }}
          HEAD_SHA: ${{ needs.build.outputs.head_sha }}
```

Then construct the URL in the `script:` block: `const url = \`https://pr-${prNumber}.synthorg-pr-preview.pages.dev\`;`. This is a minor consistency suggestion since `pr_number` is already digit-validated.

How can I resolve this? If you propose a fix, please make it concise.

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}