feat(ci): add GitHub Actions workflow for sequential CMS and Web deployment#8
feat(ci): add GitHub Actions workflow for sequential CMS and Web deployment#8
Conversation
…cations to Vercel
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on January 9
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
web/src/layout/Layout.astro
Outdated
| const isPreviewDeployment = VERCEL_ENV === 'preview' | ||
|
|
||
| console.log('[Preview Test] CMS_URL:', CMS_URL) | ||
| console.log('[Preview Test] VERCEL_ENV:', VERCEL_ENV) |
There was a problem hiding this comment.
Debug console.log statements will run in production
The console.log statements labeled [Preview Test] are unconditional and will execute on every page render in all environments, including production. Unlike the isPreviewDeployment banner which is conditional, these logs always run. This appears to be temporary debugging code that was included for testing purposes but will pollute server logs with CMS_URL and VERCEL_ENV values on every page load.
Pull Request ReviewSummaryThis PR adds a preview deployment banner to display the CMS URL and updates the deployment workflow to pass Issues Found🔴 Critical: Security Concern - Exposing Internal URLsLocation: The banner exposes the internal CMS preview URL to all users visiting the preview deployment. This could be a security risk: <div class="bg-amber-500 text-black text-center py-2 text-sm font-medium">
Preview Deployment - CMS: {CMS_URL}
</div>Recommendation: If this banner is for debugging purposes, consider:
🟡 Medium: Client-Side Environment Variable MisconfigurationLocation:
Issues:
Recommendation:
🟡 Medium: Deployment Workflow - Redundant Environment VariableLocation: The workflow now passes --build-env CMS_URL=${{ needs.deploy-cms.outputs.deployment_url }} -e CMS_URL=${{ needs.deploy-cms.outputs.deployment_url }}Issue:
Clarification needed: Is the runtime 🟢 Minor: Debug Code Left in Production PathLocation: Console logs are present in the component: console.log('[Preview Test] CMS_URL:', CMS_URL)
console.log('[Preview Test] VERCEL_ENV:', VERCEL_ENV)Recommendation: Remove these before merging or wrap them in a development-only check. Performance Considerations
Positive Aspects
Recommendations
Review conducted per repository's CLAUDE.md guidelines |
Preview Deployment
The Web preview uses the CMS preview URL for content fetching. |
Code Review for PR #8SummaryThis PR adds preview deployment diagnostics by displaying the CMS URL in a banner on preview deployments and enhancing the CI workflow to capture and expose Vercel inspect URLs. Potential Bugs & Issues1. Fragile Shell Parsing in GitHub ActionsLocation: The workflow uses Recommendation: Add validation after extraction to ensure URLs were successfully extracted, or use Vercel's JSON output if available. 2. Missing Error Handling for Empty Inspect URLsLocation: The PR comment table includes inspect URLs without checking if they exist. If Recommendation: Use conditional formatting or fallback text. 3. CMS_URL Exposed to ClientLocation:
Recommendation: Verify if preview CMS URLs should be publicly visible. If not, consider using server-side rendering only for this banner. Performance Considerations1. Runtime Environment Variable AccessLocation: The layout logs Recommendation: Remove debug logs before merging or gate them behind a debug flag. 2. Banner HTML Injected on Every PageLocation: Impact is minimal - the banner is small and conditionally rendered. No significant performance concern. Security Concerns1. Vercel Token Exposure RiskLocation: The workflow captures full Vercel CLI output including stderr. If Vercel CLI ever logs the token or sensitive info to stderr, it could be captured. Recommendation: Be cautious with captured output. 2. Preview Deployment AuthenticationLocation: The workflow requires disabling authentication on preview deployments. Anyone with the URL can access the CMS. Combined with exposing Recommendation:
3. Command Injection RiskLocation: The workflow passes deployment URL directly to shell without proper quoting. While unlikely with Vercel URLs, it's not properly quoted. Recommendation: Add quotes around the variable. Additional Notes
Recommendations SummaryHigh Priority:
Medium Priority:
Low Priority:
|
Pull Request ReviewOverviewThis PR refactors the deployment workflow and adds preview deployment visibility features. The changes simplify the GitHub Actions workflow and add a banner to preview deployments showing the CMS URL being used. Potential Bugs and Issues1. Critical: Bash boolean expression syntax errorLocation: .github/workflows/deploy.yml:68 The GitHub expression syntax is invalid. The && operator in GitHub expressions has higher precedence than ||, and this expression doesn't properly evaluate to a bash string. Fix: Use proper boolean evaluation with extra parentheses. 2. Fragile URL parsing with grep/awkLocation: .github/workflows/deploy.yml:71-72, 100-101 Issues:
Recommendation: Add validation to check if URLs were successfully extracted before proceeding. 3. Console.log in production codeLocation: web/src/layout/Layout.astro:30-31 Debug logs left in production code. These will run on every page load in all environments. Fix: Remove before merging or guard with development check using import.meta.env.DEV 4. Potential environment variable undefined issueLocation: web/src/layout/Layout.astro:70 The CMS_URL is accessed in the client-side template but might be undefined if the env variable isn't set during build. Recommendation: Add safety check with fallback value. Performance Considerations✅ Workflow simplification improves CI speed - Removing Node.js setup and Vercel CLI installation steps is a good optimization. ✅ Banner adds minimal overhead - The preview banner only renders when VERCEL_ENV === 'preview'. Security Concerns🔴 Authentication disabled for CMS previews - Preview CMS deployments are publicly accessible without authentication. Implement VERCEL_AUTOMATION_BYPASS_SECRET as noted in the TODO. SummaryMust fix before merge:
Should fix soon:
Overall: The workflow simplification is well-designed, but there are critical syntax and logging issues that need addressing before merge. |
Code ReviewCritical IssueShell Expression Syntax Error (.github/workflows/deploy.yml:68) - The PROD_FLAG assignment has incorrect bash syntax and will cause deployment failures. It should use proper conditional logic like the deploy-web job does (lines 93-100). Other Issues
Security Concerns
Performance
Positive Changes
|
Code Review - PR #8: Preview Deployment WorkflowSummaryThis PR streamlines the deployment workflow by simplifying the GitHub Actions configuration and adding a preview banner to show the CMS URL being used. Overall the changes are well-structured, but there are several issues that need attention. 🐛 Critical Issues1. Incorrect Bash Syntax in Workflow (
|
PR Review: Preview Deployment WorkflowI've reviewed the changes in this PR. Here's my analysis: Potential Bugs & IssuesCritical: Shell Operator Precedence Bug (Line 128)The PROD_FLAG assignment has incorrect operator precedence that will cause unexpected behavior. Issue: The && operator has higher precedence than ||, so the expression evaluates incorrectly. Fix needed: Add parentheses to group the OR conditions before the AND. Medium: Shell Pattern Matching Logic (Lines 102-103)Similar operator precedence issue in the change detection logic. Fix needed: Use if-then-else blocks instead of chained operators for clarity and correctness. Medium: Missing CMS URL Handling in Web DeployLine 161-162 passes CMS_URL to Vercel, but when CMS deployment is skipped, needs.deploy-cms.outputs.deployment_url will be empty. This could cause the Web build to use an undefined/empty CMS_URL. Recommendation: Add validation or use a fallback URL when CMS is skipped. Minor: Console Logs Left in Production CodeLines 30-31 in Layout.astro contain debug console logs that will execute on every page render. Recommendation: Remove before merging or wrap in a debug flag check. Security ConcernsMedium: Exposure of Internal CMS URLThe preview banner (lines 68-72 in Layout.astro) displays the CMS URL publicly. While this is a preview environment, it could expose internal infrastructure details and potential attack surface for the CMS. Recommendation: Consider whether this information should be visible to all preview visitors, or restrict to authenticated users only. Info: TODO AcknowledgedLine 21 correctly identifies the need to use VERCEL_AUTOMATION_BYPASS_SECRET instead of disabling authentication entirely. This is a good security practice to track. Performance ConsiderationsPositive: Conditional DeploymentThe new detect-changes job prevents unnecessary deployments, which is excellent for reducing CI/CD costs, faster feedback loops, and lower Vercel bandwidth usage. Positive: Parallel Job StructureThe workflow correctly uses job dependencies instead of sequential steps, allowing GitHub Actions to optimize scheduling. Additional Observations
SummaryMust Fix Before Merge:
Should Fix:
Nice to Have:
Overall, this is a well-structured improvement to the deployment workflow with smart optimizations. The conditional deployment logic is solid, but needs the operator precedence fixes to work correctly. |
PR Review: Preview Deployment WorkflowI've reviewed the changes and here's my feedback: Potential Bugs/Issues
Performance Considerations
Security Concerns
Additional Observations✅ Good practices:
📋 Recommendations:
Testing ChecklistBefore merging, verify:
Priority Fixes: Item #1 (operator precedence) is critical and will cause incorrect change detection. |
| HEAD_SHA=${{ github.sha }} | ||
| fi | ||
|
|
||
| CHANGED_FILES=$(git diff --name-only $BASE_SHA $HEAD_SHA 2>/dev/null || echo "") |
There was a problem hiding this comment.
Shallow clone breaks PR change detection silently
For pull requests, fetch-depth: 2 is insufficient to ensure github.event.pull_request.base.sha and head.sha exist in the shallow clone. When the base branch has diverged or the PR has multiple commits, the git diff command at line 99 fails silently (error suppressed with 2>/dev/null || echo ""), resulting in empty CHANGED_FILES. This causes both cms_changed and web_changed to be 'false', and no preview deployments will occur for the PR.
PR Review - Preview Deployment WorkflowThis PR implements a smart deployment workflow with change detection, skip tags, and preview deployments. The implementation is solid but has critical issues that must be addressed before merging. Critical Bugs1. Bash Operator Precedence Issue (.github/workflows/deploy.yml:102-103) 2. Test Code Left in Production
Security Concerns1. VERCEL_TOKEN Exposure - Consider using env vars instead of command args Performance1. Redundant Checkout - Consider using dorny/paths-filter for change detection RecommendationsBefore merging:
Future improvements:
Overall: Solid architecture with good separation of concerns. Critical issues are the operator precedence bug and test code cleanup. |
cms/src/endpoints/sitemap.ts
Outdated
| pages.push({ | ||
| path: '/dummy-test-entry', | ||
| updatedAt: new Date().toISOString(), | ||
| }) |
There was a problem hiding this comment.
Test dummy entry added to production sitemap
A dummy test entry with path /dummy-test-entry is being added to the sitemap endpoint. The TODO comment explicitly states "Remove this dummy entry - testing CMS-only deployment", indicating this is temporary test code. This will cause a fake non-existent page to appear in the production sitemap, potentially affecting SEO and confusing search engines.
PR Review: Preview Deployment WorkflowSummaryThis PR enhances the deployment workflow with intelligent change detection, skip tags, and preview visibility. Overall good improvements, but there are some issues that need attention before merging. Critical Issues1. Potential Security Risk: CMS_URL ExposureLocation: The PR adds console logging of console.log('[Preview Test] CMS_URL:', CMS_URL)
console.log('[Preview Test] VERCEL_ENV:', VERCEL_ENV)Issue: These console logs will be visible in browser dev tools on preview deployments, potentially exposing internal URLs to anyone with access to the preview. Recommendation: Remove these debug logs before merging, or wrap them in a more restrictive condition if they're needed for troubleshooting. 2. Test Code Left in Production PathLocation: // TODO: Remove this dummy entry - testing CMS-only deployment
pages.push({
path: '/dummy-test-entry',
updatedAt: new Date().toISOString(),
})Issue: This dummy sitemap entry will be included in production builds and could affect SEO or cause confusion. Recommendation: Remove this test code before merging. If needed for testing, use environment checks to only include in preview/development. Bugs & Issues3. Workflow Change Detection LogicLocation: CMS_CHANGED=$([[ "$CHANGED_FILES" == *"cms/"* ]] || [[ "$CHANGED_FILES" == *".github/workflows/deploy.yml"* ]] && echo 'true' || echo 'false')
WEB_CHANGED=$([[ "$CHANGED_FILES" == *"web/"* ]] || [[ "$CHANGED_FILES" == *".github/workflows/deploy.yml"* ]] && echo 'true' || echo 'false')Issue: Bash operator precedence may cause unexpected behavior. The Current behavior: Recommendation: Add explicit parentheses or restructure: CMS_CHANGED=$(if [[ "$CHANGED_FILES" == *"cms/"* ]] || [[ "$CHANGED_FILES" == *".github/workflows/deploy.yml"* ]]; then echo 'true'; else echo 'false'; fi)
WEB_CHANGED=$(if [[ "$CHANGED_FILES" == *"web/"* ]] || [[ "$CHANGED_FILES" == *".github/workflows/deploy.yml"* ]]; then echo 'true'; else echo 'false'; fi)4. Missing Error Handling for Git DiffLocation: CHANGED_FILES=$(git diff --name-only $BASE_SHA $HEAD_SHA 2>/dev/null || echo "")Issue: If Recommendation: Add explicit error handling or fail the job if git commands fail: CHANGED_FILES=$(git diff --name-only $BASE_SHA $HEAD_SHA) || {
echo "Error: Failed to detect changes"
exit 1
}Performance Considerations5. Unnecessary Workflow ComplexityLocation: Observation: The workflow now has three jobs (detect-changes → deploy-cms → deploy-web) with conditional logic spread across multiple places. Minor Concern: While functional, this adds complexity. Consider if the change detection could be simplified or if skip tags alone would suffice for most use cases. Benefit: The change detection does prevent unnecessary deployments, saving build minutes and costs. Security Concerns6. Vercel Authentication Disabled in DocumentationLocation: The README recommends disabling Vercel Authentication:
Concern: This creates a security gap where preview CMS deployments are publicly accessible without authentication. Note: The TODO comment in the workflow acknowledges this: # TODO: Migrate to using VERCEL_AUTOMATION_BYPASS_SECRET instead of disabling authentication.Recommendation: Prioritize implementing Code Quality Issues7. Inconsistent Environment Variable AccessLocation: import { CMS_URL } from 'astro:env/client'
import { VERCEL_ENV } from 'astro:env/server'Observation: Mixing client and server environment imports in the same component. Question: Is 8. Typo in Layout.astroLocation: <slot /></output>Issue: There's a stray Impact: This will cause HTML validation errors and may break the page layout. Fix Required: Change to Positive Aspects
Recommendations SummaryBefore Merging:
Post-Merge:
Optional:
Let me know if you'd like help addressing any of these issues! |
| echo "CMS preview deployed to: $DEPLOYMENT_URL" | ||
| env: | ||
| VERCEL_PROJECT_ID: ${{ secrets.VERCEL_CMS_PROJECT_ID }} | ||
| COMMIT_MSG="${{ github.event.head_commit.message || github.event.pull_request.title || '' }}" |
There was a problem hiding this comment.
Skip tags won't work in PR commit messages
The skip tag detection uses github.event.head_commit.message which is null for pull request events, falling back to github.event.pull_request.title. This means for PRs, [skip-cms] or [skip-web] tags must be added to the PR title, not the commit message as documented in the README. Users following the documentation ("Add these tags to your commit message...") will find the feature doesn't work on pull requests.
Code Review - PR #8I've reviewed this PR and found several issues that should be addressed: 🐛 Potential Bugs
🔒 Security Concerns
⚡ Performance Considerations
🧹 Code Quality
✅ Positive Observations
📋 SummaryMust fix before merge:
Recommended improvements:
Let me know if you need clarification on any of these points! |
jhb-dev
force-pushed
the
test/preview-deployment
branch
from
2f88cf4 to
3ff8c22
Compare
PR Review: Preview Deployment WorkflowI've reviewed this PR and found several issues that need attention before merging. Potential Bugs1. Bash Logic Error in Workflow (Critical)Location: .github/workflows/deploy.yml:98-99 The bash operator precedence in the change detection logic is incorrect. The && binds tighter than ||, which means when neither cms/ nor workflow file changes, it still outputs true instead of false. Fix: Use proper if-else structure instead of chained operators. 2. Test Code Left in ProductionLocation: cms/src/endpoints/sitemap.ts:62-66 Dummy sitemap entry with TODO comment should be removed before merge. Performance Considerations1. Git Fetch Depth Limitationfetch-depth: 2 may cause git diff failures for PRs with many commits. Consider using fetch-depth: 0 or add error handling. 2. Client-Side CMS_URL ExposureCMS_URL is exposed in browser bundle. Consider if preview URLs should be visible to all users. Security Concerns1. Bypass Secret Exposure (High Severity)Location: .github/workflows/deploy.yml:166-167 The bypass secret is passed as runtime environment variable (--env) in addition to build-time. This exposes the secret in the deployed environment where it could be extracted. Fix: Remove --env flags for the bypass secret, only use --build-env. 2. Sensitive Data in Console LogsLocation: web/src/layout/Layout.astro:30-31 Debug console.log statements expose deployment info. Remove before production. 3. Preview Banner Information DisclosureBanner displays CMS URL publicly on all preview deployments. Evaluate if this is acceptable. Summary
The core approach is sound, but these issues should be addressed before merging to production. |
Code ReviewI have reviewed PR #8 and identified several areas for feedback: Potential Bugs/Issues
Performance Considerations
Security Concerns
Recommendations PriorityHigh Priority:
Medium Priority:
Low Priority:
Overall, this is a well-structured PR that adds important functionality for preview deployments. The main concern is the bash logic bug that could cause incorrect change detection. |
jhb-dev
changed the title
| echo "| Project | URL |" >> $GITHUB_STEP_SUMMARY | ||
| echo "|---------|-----|" >> $GITHUB_STEP_SUMMARY | ||
| echo "| CMS | ${{ needs.deploy-cms.outputs.deployment_url || 'Skipped' }} |" >> $GITHUB_STEP_SUMMARY | ||
| echo "| Web | ${{ steps.deploy.outputs.deployment_url }} |" >> $GITHUB_STEP_SUMMARY |
There was a problem hiding this comment.
PR comment job skipped when CMS deployment is skipped
The comment-preview job has needs: [deploy-cms, deploy-web] but its if condition lacks always(). In GitHub Actions, when any job in needs is skipped, dependent jobs are also skipped by default. With the new change detection logic, deploy-cms can now be skipped for PRs with only web changes, causing comment-preview to be skipped even when deploy-web succeeds. The deploy-web job correctly uses always() at line 143 to handle this, but comment-preview doesn't follow the same pattern.
| - name: Check for skip tags | ||
| id: skip | ||
| run: | | ||
| COMMIT_MSG="${{ github.event.head_commit.message || github.event.pull_request.title || '' }}" |
There was a problem hiding this comment.
Script injection via untrusted commit message or PR title
The commit message and PR title are directly interpolated into a bash script using ${{ github.event.head_commit.message || github.event.pull_request.title }}. This is a well-documented GitHub Actions script injection vulnerability. An attacker can craft a PR title like Fix $(malicious_command) or use backticks, and the shell will execute the embedded command on the runner. For PRs from forks, anyone can control the PR title. The safe pattern is to pass untrusted values via the env: context instead of direct interpolation.
1 participant
Testing the preview deployment workflow. This adds a banner showing the CMS URL used for the build.
Note
Conditionally deploy CMS/Web with skip tags and CMS-first ordering, add CMS preview auth bypass support to Web fetches, and document the deployment process.
detect-changesto deploy CMS/Web only when their folders or workflow change; supports[skip-cms]/[skip-web]; adds manual inputs; ensures CMS deploy runs before Web; passesCMS_URLand optionalCMS_VERCEL_AUTOMATION_BYPASS_SECRETto Web preview builds; updates summary output and PR comment to handle skipped CMS.CMS_VERCEL_AUTOMATION_BYPASS_SECRETto env schema; newsdk/bypassHeaderand integrates it ingetRedirectsandsdk/cachedFetchso preview builds can access protected CMS via Vercel automation bypass header.Written by Cursor Bugbot for commit 2d1465e. This will update automatically on new commits. Configure here.