Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 74 additions & 11 deletions .github/workflows/pages-preview.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,26 +13,85 @@ on:
- "src/ai_company/**"
- "scripts/**"
- ".github/workflows/pages-preview.yml"
workflow_dispatch:
inputs:
pr_number:
description: "PR number to build and deploy a preview for (use for Dependabot PRs)"
required: true
type: string

permissions: {}

concurrency:
group: "pages-preview-${{ github.event.pull_request.number }}"
group: "pages-preview-${{ github.event.pull_request.number || github.event.inputs.pr_number }}"
cancel-in-progress: true

jobs:
build:
name: Build Preview
if: github.event.action != 'closed'
if: github.event_name == 'workflow_dispatch' || github.event.action != 'closed'
runs-on: ubuntu-latest
timeout-minutes: 10
permissions:
contents: read
Comment thread
greptile-apps[bot] marked this conversation as resolved.
pull-requests: read
outputs:
pr_number: ${{ steps.pr.outputs.pr_number }}
head_sha: ${{ steps.pr.outputs.head_sha }}
same_repo: ${{ steps.pr.outputs.same_repo }}
steps:
- name: Resolve PR metadata
id: pr
env:
GH_TOKEN: ${{ github.token }}
EVENT_NAME: ${{ github.event_name }}
PR_INPUT: ${{ github.event.inputs.pr_number }}
PR_EVENT_NUMBER: ${{ github.event.pull_request.number }}
PR_EVENT_HEAD_SHA: ${{ github.event.pull_request.head.sha }}
PR_EVENT_HEAD_REPO: ${{ github.event.pull_request.head.repo.full_name }}
GH_REPOSITORY: ${{ github.repository }}
run: |
set -euo pipefail
PR_NUMBER="${PR_EVENT_NUMBER:-$PR_INPUT}"
if ! [[ "$PR_NUMBER" =~ ^[1-9][0-9]*$ ]]; then
echo "::error::PR number must be a positive integer, got: '$PR_NUMBER'"
exit 1
fi

if [ "$EVENT_NAME" = "workflow_dispatch" ]; then
PR_JSON=$(gh pr view "$PR_NUMBER" --repo "$GH_REPOSITORY" \
--json headRefOid,state,isCrossRepository)
if [ -z "$PR_JSON" ]; then
echo "::error::Failed to fetch metadata for PR #$PR_NUMBER"
exit 1
fi
Comment on lines +64 to +67
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
if [ -z "$PR_JSON" ]; then
echo "::error::Failed to fetch metadata for PR #$PR_NUMBER"
exit 1
fi
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.

Prompt To Fix With AI
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.

HEAD_SHA=$(echo "$PR_JSON" | jq -r '.headRefOid')
PR_STATE=$(echo "$PR_JSON" | jq -r '.state')
IS_CROSS_REPO=$(echo "$PR_JSON" | jq -r '.isCrossRepository')

if [ "$PR_STATE" != "OPEN" ]; then
echo "::error::PR #$PR_NUMBER is $PR_STATE, not OPEN — cannot deploy preview"
exit 1
fi
SAME_REPO=$( [ "$IS_CROSS_REPO" = "false" ] && echo "true" || echo "false" )
else
Comment on lines 35 to +77
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +61 to +77
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
HEAD_SHA="$PR_EVENT_HEAD_SHA"
SAME_REPO=$( [ "$PR_EVENT_HEAD_REPO" = "$GH_REPOSITORY" ] && echo "true" || echo "false" )
fi

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

echo "pr_number=$PR_NUMBER" >> "$GITHUB_OUTPUT"
echo "head_sha=$HEAD_SHA" >> "$GITHUB_OUTPUT"
Comment thread
coderabbitai[bot] marked this conversation as resolved.
echo "same_repo=$SAME_REPO" >> "$GITHUB_OUTPUT"
Comment on lines +43 to +89
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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:

🛡️ Optional hardening
+          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

,

🤖 Prompt for AI Agents
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.


- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6
with:
persist-credentials: false
ref: ${{ github.event.pull_request.head.sha }}
ref: ${{ steps.pr.outputs.head_sha }}
Comment on lines +53 to +94
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

# --- MkDocs (documentation at /docs) ---
- name: Set up Python
Expand Down Expand Up @@ -82,8 +141,8 @@ jobs:
# --- Inject preview banner ---
- name: Inject preview banner
env:
PR_NUMBER: ${{ github.event.pull_request.number }}
HEAD_SHA: ${{ github.event.pull_request.head.sha }}
PR_NUMBER: ${{ steps.pr.outputs.pr_number }}
HEAD_SHA: ${{ steps.pr.outputs.head_sha }}
run: |
python3 << 'PYEOF'
import os, pathlib, re
Expand Down Expand Up @@ -144,7 +203,7 @@ jobs:
deploy-preview:
name: Deploy Preview
needs: build
if: github.event.pull_request.head.repo.full_name == github.repository
if: needs.build.outputs.same_repo == 'true'
runs-on: ubuntu-latest
environment: cloudflare-preview
timeout-minutes: 5
Expand All @@ -167,19 +226,22 @@ jobs:
env:
CLOUDFLARE_API_TOKEN: ${{ secrets.CLOUDFLARE_API_TOKEN }}
CLOUDFLARE_ACCOUNT_ID: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }}
PR_NUMBER: ${{ github.event.pull_request.number }}
PR_NUMBER: ${{ needs.build.outputs.pr_number }}
run: |
npm i --no-save wrangler@3.114.17
npx wrangler pages deploy _site --project-name=synthorg-pr-preview --branch="pr-${PR_NUMBER}"

- name: Comment preview URL
uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0
env:
PREVIEW_URL: "https://pr-${{ github.event.pull_request.number }}.synthorg-pr-preview.pages.dev"
PREVIEW_URL: "https://pr-${{ needs.build.outputs.pr_number }}.synthorg-pr-preview.pages.dev"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
PREVIEW_URL: "https://pr-${{ needs.build.outputs.pr_number }}.synthorg-pr-preview.pages.dev"
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.

Prompt To Fix With AI
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!

PR_NUMBER: ${{ needs.build.outputs.pr_number }}
HEAD_SHA: ${{ needs.build.outputs.head_sha }}
with:
script: |
const url = process.env.PREVIEW_URL;
const headSha = context.payload?.pull_request?.head?.sha || context.sha;
const headSha = process.env.HEAD_SHA;
const prNumber = parseInt(process.env.PR_NUMBER, 10);
const marker = '<!-- synthorg-pr-preview -->';
const body = [
marker,
Expand All @@ -195,7 +257,7 @@ jobs:
{
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: context.issue.number,
issue_number: prNumber,
per_page: 100,
}
);
Expand All @@ -213,14 +275,15 @@ jobs:
await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: context.issue.number,
issue_number: prNumber,
body,
});
}

cleanup-preview:
name: Cleanup Preview
if: >-
github.event_name == 'pull_request' &&
github.event.action == 'closed' &&
github.event.pull_request.head.repo.full_name == github.repository
runs-on: ubuntu-latest
Expand Down
9 changes: 6 additions & 3 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -182,12 +182,15 @@ src/ai_company/
- **Jobs**: lint (ruff) + type-check (mypy src/ tests/) + test (pytest + coverage) run in parallel → ci-pass (gate)
- **Pages**: `.github/workflows/pages.yml` — exports OpenAPI schema (`scripts/export_openapi.py`), builds Astro landing + MkDocs docs, merges, deploys to GitHub Pages on push to main. Triggers on `docs/**`, `site/**`, `mkdocs.yml`, `pyproject.toml`, `uv.lock`, `src/ai_company/**`, `scripts/**`, workflow file changes, and `workflow_dispatch`.
- **PR Preview**: `.github/workflows/pages-preview.yml`
- Builds site on PRs (same path triggers as Pages), injects "Development Preview" banner, deploys to Cloudflare Pages (`synthorg-pr-preview` project) via wrangler CLI
- Builds site on PRs (same path triggers as Pages) and on `workflow_dispatch` (with `pr_number` input, for Dependabot PRs that can't trigger `pull_request` with secrets)
- Dispatch runs resolve PR metadata (head SHA, state, same-repo check) via `gh pr view`; PR-triggered runs use event context directly
- Validates `pr_number` is a positive integer, rejects closed/cross-repo PRs on dispatch, and fails fast if HEAD SHA resolution fails
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
- Validates `pr_number` is a positive integer, rejects closed/cross-repo PRs on dispatch, and fails fast if HEAD SHA resolution fails
- Validates `pr_number` is a positive integer, rejects non-OPEN PRs on dispatch, and fails fast if HEAD SHA resolution fails

Copilot uses AI. Check for mistakes.
- Injects "Development Preview" banner, deploys to Cloudflare Pages (`synthorg-pr-preview` project) via wrangler CLI
- Each PR gets a unique preview URL at `pr-<number>.synthorg-pr-preview.pages.dev`
- Requires `CLOUDFLARE_API_TOKEN` + `CLOUDFLARE_ACCOUNT_ID` secrets
- Checks out PR head SHA (not merge commit) so build matches reported commit
- Build job runs regardless (catches build failures); deploy job skips on fork PRs (no secrets access)
- Cleanup job deletes preview comment and Cloudflare deployments on PR close
- Build job runs regardless (catches build failures); deploy job skips on fork PRs (same-repo check via job output)
- Cleanup job deletes preview comment and Cloudflare deployments on PR close (pull_request events only)
- Concurrency group cancels stale builds on rapid pushes
- **Docker**: `.github/workflows/docker.yml` — builds backend + web images, pushes to GHCR, signs with cosign. Scans: Trivy (CRITICAL = hard fail, HIGH = warn-only) + Grype (critical cutoff). CVE triage via `.github/.trivyignore.yaml` and `.github/.grype.yaml`. Images only pushed after scans pass. Triggers on push to main and version tags (`v*`).
- **Matrix**: Python 3.14
Expand Down
Loading