Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
No review was started: this request came from a bot account. Manual reviews can only be requested by someone with write access to this repository. Ask a maintainer to comment @claude review, or have your automation post the comment from a user account with write access.
Tip: disable this comment in your organization's Code Review settings.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 720038f17b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - pre-job | ||
| if: needs.pre-job.outputs.should_skip != 'true' | ||
| - build-and-check-links | ||
| if: ${{ !cancelled() && needs.pre-job.outputs.should_skip != 'true' }} |
There was a problem hiding this comment.
When build-and-check-links is cancelled, !cancelled() prevents this compatibility job from running, so its conclusion is skipped; GitHub treats skipped required jobs as successful for branch protection. Repositories still requiring the preserved check-sitemap-links check can therefore merge even though neither link suite completed, contrary to the requirement that all three CI checks run on every PR. Run the proxy with always() and let its existing result comparison fail for cancelled.
AGENTS.md reference: AGENTS.md:L61-L63
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Looks good, straightforward CI simplification.
What was reviewed: the merged build-and-check-links job (single build/server now runs both pnpm link-check and pnpm sitemap-check), the new passthrough check-sitemap-links job that preserves the required-check name, the updated test:link-check script wiring, and the summary job's added cancelled handling. Checked the timeout/cancellation edge case for the passthrough job — it uses !cancelled() (workflow-level) rather than the default success(), so it still runs and correctly fails via test "$RESULT" = success even if build-and-check-links times out or fails, rather than being skipped. The pnpm-lock.yaml change is just a zod/katex transitive-resolution bump within the existing ^4.3.6 range in package.json.
Extended reasoning...
Overview
This PR touches only .github/workflows/ci.yml and pnpm-lock.yaml. It consolidates two previously separate build+server+check jobs (link-check and sitemap-check) into one job to avoid duplicating the ~10-minute build/server startup, while keeping a thin passthrough check-sitemap-links job so the required branch-protection check name is preserved. It also broadens the unit-test step to run both checker test files via a new test:link-check script, raises build-and-check-links timeout from 30 to 40 minutes, and makes the final CI summary job treat cancelled results as failures in addition to failure. pnpm-lock.yaml only bumps the transitive zod resolution (4.3.6 -> 4.5.4) and katex (0.16.46 -> 0.16.47) within the existing semver ranges declared in package.json.
Security risks
None. This is CI tooling and a lockfile resolution bump within existing semver constraints; no application code, auth, or data-handling paths are touched.
Level of scrutiny
Low-to-moderate is appropriate: it's a CI-only config change with no user-facing or security-sensitive code, but I still traced the conditional logic carefully since a mistake here could silently make a required check pass when it shouldn't. I verified package.json already declares pnpm run test:link-check as node --test scripts/check-links.test.js scripts/check-sitemap-links.test.js, matching the new step, and confirmed link-check/sitemap-check scripts still exist unchanged.
Other factors
I specifically walked through the scenario the bug hunter's ruled-out note raised: if build-and-check-links times out, GitHub reports that job's own result as cancelled (not failure), but the passthrough job's job-level cancelled() check evaluates the overall workflow-run cancellation state (not the needs-job's individual conclusion), so !cancelled() remains true and the passthrough job still executes its test "$RESULT" = success step, which fails correctly when RESULT is cancelled or failure. This confirms the design intentionally avoids the default success() job condition (which would have caused the passthrough job to be skipped, potentially satisfying the required check incorrectly) and instead correctly propagates non-success outcomes. Given the mechanical nature of the change, existing test coverage for the checker scripts, and this verified control-flow correctness, I'm comfortable approving without requiring further human review.
This branch was successfully deployed
1 participant
Note
Low Risk
CI workflow and lockfile-only changes; no application runtime logic. Lockfile zod bump is the main unrelated variable if merged as-is.
Overview
Consolidates link and sitemap CI so one job builds the Next.js app once, runs checker unit tests via
pnpm run test:link-check, then validates markdown/source links and sitemap URLs against the same production server. Sitemap checking still runs when the link step fails, as long as the server started successfully.The standalone
check-sitemap-linksjob is kept only as a compatibility shim (short timeout, assertsbuild-and-check-linkssucceeded) so existing required check names in branch protection stay valid.all-checks-passnow fails on cancelled jobs as well as failures.Also refreshes
pnpm-lock.yaml(notablyzod4.3.6→4.5.4 and minor transitive bumps), which is unrelated to the workflow refactor.Reviewed by Cursor Bugbot for commit 8b62639. Bugbot is set up for automated code reviews on this repo. Configure here.
The workflow consolidation appears safe to merge, but the unrelated production lockfile refresh should be removed or reviewed separately.
Summary
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD P[pre-job] --> B[build-and-check-links] B --> U[Checker unit tests] U --> N[Build Next.js app] N --> S[Start production server] S --> L[Check markdown and source links] S --> M[Check sitemap URLs] B --> C[check-sitemap-links compatibility check] P --> C B --> A[all-checks-pass] C --> AReviews (1) · Last reviewed commit: "chore(ci): simplify link checking"