Skip to content

ci: add build check to verify public/ is up to date#21

Merged
Troublor merged 8 commits into
mainfrom
william/ci/add-build-check
Mar 24, 2026
Merged

ci: add build check to verify public/ is up to date#21
Troublor merged 8 commits into
mainfrom
william/ci/add-build-check

Conversation

@Troublor

Copy link
Copy Markdown
Collaborator

Summary

Add a CI check that runs make and verifies the committed public/ directory matches the build output. Fails with a clear error message if a contributor forgot to run make before committing.

This replaces the manual "always run make before committing" requirement with an automated gate on PRs.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Mar 24, 2026

Copy link
Copy Markdown

Deploying megaeth-documentation with  Cloudflare Pages  Cloudflare Pages

Latest commit: bc38e5f
Status: ✅  Deploy successful!
Preview URL: https://c4a7f597.megaeth-documentation.pages.dev
Branch Preview URL: https://william-ci-add-build-check.megaeth-documentation.pages.dev

View logs

Comment thread .github/workflows/build-check.yml
@claude

claude Bot commented Mar 24, 2026

Copy link
Copy Markdown
Contributor

Good addition — automating the make check removes a whole class of stale-output PRs.

One bug: the staleness check uses git diff, which only catches modifications to already-tracked files. A new page (new public/*.html) would be untracked and silently pass. Switch to git status --porcelain public/ manifest/ to catch both modified and new files. Suggestion left inline.

Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Comment on lines +20 to +27

- name: Check public/ is up to date
if [ -n "$(git status --porcelain public/ manifest/)" ]; then
if [ -n "$(git diff --name-only public/)" ]; then
echo "::error::public/ is out of date. Run 'make' locally and commit the output."
git diff --stat public/
exit 1
fi

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.

The step is missing run: | — as written this is invalid YAML and the workflow will fail to parse. There are also two conflicting if conditions: the first (using git status --porcelain) is correct but unreachable; the second (using git diff) is what actually runs but misses untracked files (e.g. a newly added page).

Suggested change
- name: Check public/ is up to date
if [ -n "$(git status --porcelain public/ manifest/)" ]; then
if [ -n "$(git diff --name-only public/)" ]; then
echo "::error::public/ is out of date. Run 'make' locally and commit the output."
git diff --stat public/
exit 1
fi
- name: Check public/ is up to date
run: |
if [ -n "$(git status --porcelain public/ manifest/)" ]; then
echo "::error::public/ is out of date. Run 'make' locally and commit the output."
git status --short public/ manifest/
exit 1
fi

@claude

claude Bot commented Mar 24, 2026

Copy link
Copy Markdown
Contributor

The Check public/ is up to date step has a critical YAML syntax error: it's missing run: |, so the workflow will fail to parse entirely. The step also has two stacked if conditions — the correct one (git status --porcelain) is dead code, and the one that would actually run (git diff) misses untracked files. Fix left inline.

Comment thread .github/workflows/build-check.yml Outdated
types: [opened, synchronize, reopened]

jobs:
build-check:

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.

The workflow is missing an explicit permissions block. Without it, the job inherits the repo's default permissions, which is broader than needed. Add least-privilege permissions:

Suggested change
build-check:
jobs:
build-check:
runs-on: ubuntu-24.04
permissions:
contents: read
timeout-minutes: 5

@claude

claude Bot commented Mar 24, 2026

Copy link
Copy Markdown
Contributor

Good addition — automating the make check removes a whole class of stale-public/ PRs.

One issue: the job is missing an explicit permissions block (see inline comment). Since this workflow only reads the repo and doesn't post to PRs or write anything, contents: read is sufficient and follows least-privilege best practice, consistent with how pr-review in claude.yml is scoped.

Comment thread .github/workflows/build-check.yml Outdated
fetch-depth: 1

- name: Install Pandoc
run: sudo apt-get update && sudo apt-get install -y pandoc

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.

Pandoc version mismatch will cause spurious failures.

apt-get install pandoc on Ubuntu 24.04 installs whatever version ships in the distro repos (currently 3.1.3), which likely differs from what contributors have locally. Different Pandoc versions can produce different HTML output even from identical Markdown, so the public/ diff check will fail for any contributor not using the exact same version.

Pin to a specific version, ideally the one contributors are expected to use:

Suggested change
run: sudo apt-get update && sudo apt-get install -y pandoc
- name: Install Pandoc
run: |
PANDOC_VERSION=3.6.4
wget -q "https://github.com/jgm/pandoc/releases/download/${PANDOC_VERSION}/pandoc-${PANDOC_VERSION}-1-amd64.deb" -O /tmp/pandoc.deb
sudo dpkg -i /tmp/pandoc.deb

(Adjust PANDOC_VERSION to match what the team uses locally — worth documenting in CLAUDE.md too.)

run: make

- name: Check public/ is up to date
run: |

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.

manifest/ is also committed (it's auto-generated and tracked per CLAUDE.md). Consider checking it too so stale manifest files don't slip through:

Suggested change
run: |
if [ -n "$(git status --porcelain public/ manifest/)" ]; then

@claude

claude Bot commented Mar 24, 2026

Copy link
Copy Markdown
Contributor

The approach is solid — run make, check for a dirty working tree. Two issues:

  1. Pandoc version (blocking): apt-get install pandoc on Ubuntu 24.04 installs ~3.1.3, which almost certainly differs from contributors' local installs. Different Pandoc versions produce different HTML, so this check will spuriously fail. Pin to a specific version (see inline comment).

  2. manifest/ not covered (minor): manifest/ is committed and auto-generated — the check should cover it too alongside public/.

@Troublor Troublor merged commit c944697 into main Mar 24, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant