Repository navigation
ci: workflow efficiency and external-contributor automation - #1897
Merged
Merged
Conversation
LucasSantana-Dev
enabled auto-merge (squash)
July 27, 2026 18:20
📝 WalkthroughWalkthroughThe PR consolidates CI workflows and gates, adds external pull-request automation, extracts deployment logic into reusable scripts, updates workflow scheduling and monitoring behavior, and documents the revised CI/CD structure. ChangesCI and pull-request automation
Deployment script extraction
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant DeployWorkflow
participant TriggerWebhook
participant Homelab
participant GitHubStatus
participant OAuthSmoke
DeployWorkflow->>TriggerWebhook: trigger deployment
TriggerWebhook->>Homelab: POST webhook request
DeployWorkflow->>GitHubStatus: poll homelab-deploy status
DeployWorkflow->>OAuthSmoke: run OAuth smoke checks
OAuthSmoke->>Homelab: validate auth endpoints
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
scripts/deploy/trigger-deploy-webhook.sh (2)
25-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
try_webhook_urlre-splits a payload the retry helper already parsed.
call_webhook_with_retrycomputeshttp/body, discards them, echoes the raw response, andtry_webhook_urlsplits it again — then re-joins in reverse order for the caller to split a third time. Returning the code via a status/global (or having the retry helper emitcode\nbodydirectly) would remove two of the three parse passes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/deploy/trigger-deploy-webhook.sh` around lines 25 - 59, Refactor the response handoff between call_webhook_with_retry and try_webhook_url so the parsed HTTP code and body are emitted in the caller’s expected order directly, avoiding repeated tail/sed splitting and rejoining. Preserve retry decisions and the existing final code/body output contract.
8-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFail with an explicit message when required env is missing.
With
set -u, an unsetWEBHOOK_SECRET/DEPLOY_SHAaborts insidecall_webhookwith a bare "unbound variable", after the script already printed "Triggering deploy webhook". The other extracted scripts use${VAR:?...}; match that here.♻️ Proposed refactor
+: "${WEBHOOK_URL:?WEBHOOK_URL is required}" +: "${WEBHOOK_SECRET:?WEBHOOK_SECRET is required}" +: "${DEPLOY_SHA:?DEPLOY_SHA is required}" + echo "==> Triggering deploy webhook..."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/deploy/trigger-deploy-webhook.sh` around lines 8 - 12, Update the deploy webhook flow around call_webhook and the DEPLOY_SHA usage to validate required WEBHOOK_SECRET and DEPLOY_SHA environment variables explicitly with parameter-expansion checks, producing clear variable-specific errors before printing “Triggering deploy webhook” or invoking curl. Match the existing ${VAR:?} validation style used by the other extracted scripts.scripts/deploy/derive-webhook-origin.sh (1)
13-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWrap
new URL()in try/catch for a clean failure message.Invalid
DEPLOY_WEBHOOK_URLcurrently surfaces as a raw Node stack trace in the deploy log.trigger-deploy-webhook.shalready handles this case gracefully; mirroring it here keeps diagnostics consistent.♻️ Proposed refactor
const raw = (process.env.ORIGIN_INPUT_URL ?? "").trim(); if (!raw) { console.error("derive-webhook-origin.sh: empty URL"); process.exit(1); } - const u = new URL(raw); + let u; + try { + u = new URL(raw); + } catch (error) { + console.error(`derive-webhook-origin.sh: invalid URL: ${error.message}`); + process.exit(1); + } console.log(`${u.protocol}//${u.host}`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/deploy/derive-webhook-origin.sh` around lines 13 - 21, Wrap the URL construction in the inline Node script used by derive-webhook-origin.sh with try/catch, preserving the existing empty-input validation and origin output for valid URLs. On invalid input, catch the parsing error and emit a concise contextual error message before exiting with failure, matching trigger-deploy-webhook.sh’s graceful handling.scripts/deploy/oauth-smoke.sh (1)
160-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse one temp file plus a
trapinstead of per-attemptmktemp/rm.Three cleanup sites for the same file; an early
set -eabort still leaks it.Also applies to: 210-216
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/deploy/oauth-smoke.sh` around lines 160 - 166, Update the OAuth redirect contract checks around the repeated curl attempts to create one temporary headers file before the loop and register a trap that removes it on exit, including set -e failures. Reuse that file for each attempt, remove the per-attempt mktemp and rm calls, and apply the same cleanup approach to the later check around the referenced second block..github/workflows/ci.yml (2)
145-165: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPass discord-player versions via
env:instead of inline${{ }}interpolation.The extracted
pinned/installedoutputs are already constrained to\d+\.\d+\.\d+by thegrep -oPextraction, so the static-analysis template-injection flags (lines 157, 158, 161, 162, 165) aren't currently exploitable. Still, routing these values throughenv:rather than inline${{ }}in therun:block is the standard hardening pattern and removes the injection shape entirely, independent of any future change to the extraction regex.🔒 Proposed fix
- name: Detect discord-player major bump beyond pinned + env: + PINNED: ${{ steps.discord_player.outputs.pinned }} + INSTALLED: ${{ steps.discord_player.outputs.installed }} run: | - PINNED_MAJOR=$(echo "${{ steps.discord_player.outputs.pinned }}" | cut -d. -f1) - INSTALLED_MAJOR=$(echo "${{ steps.discord_player.outputs.installed }}" | cut -d. -f1) + PINNED_MAJOR=$(echo "$PINNED" | cut -d. -f1) + INSTALLED_MAJOR=$(echo "$INSTALLED" | cut -d. -f1) if [ "$INSTALLED_MAJOR" -gt "$PINNED_MAJOR" ]; then echo "ERROR: discord-player MAJOR bump detected" - echo "Pinned: ${{ steps.discord_player.outputs.pinned }} (major: $PINNED_MAJOR)" - echo "Installed: ${{ steps.discord_player.outputs.installed }} (major: $INSTALLED_MAJOR)" + echo "Pinned: $PINNED (major: $PINNED_MAJOR)" + echo "Installed: $INSTALLED (major: $INSTALLED_MAJOR)" exit 1 fi - echo "discord-player version compatible (pinned: ${{ steps.discord_player.outputs.pinned }}, installed: ${{ steps.discord_player.outputs.installed }})" + echo "discord-player version compatible (pinned: $PINNED, installed: $INSTALLED)"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 145 - 165, Update the “Detect discord-player major bump beyond pinned” step to pass the discord-player outputs through an env: mapping, then reference those environment variables inside the shell script instead of inline ${{ steps.discord_player.outputs.* }} interpolation. Use the environment values consistently for major extraction, error messages, and the compatibility message while preserving the existing comparison and failure behavior.Source: Linters/SAST tools
236-294: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin
madgeto an exact version used by this hard-gate job.
npm install --no-save madge@^7 --ignore-scriptsinstallsmadgeoutside the lockfile with an unpinned major range, so an unreviewedmadge/transitive update can change the circular-dependency check output or behavior. Use an exact patch version instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 236 - 294, Update the “Install madge” step in the madge job to install madge using an exact reviewed patch version instead of the ^7 range. Keep the existing --no-save and --ignore-scripts flags unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/deploy.yml:
- Around line 46-47: Disable credential persistence in the Checkout steps by
setting persist-credentials to false in .github/workflows/deploy.yml lines 46-47
and .github/workflows/deploy-staging.yml lines 38-39. In the staging workflow,
also consider pinning the checkout ref to the PR base SHA as requested.
In @.github/workflows/external-pr-notify.yml:
- Around line 51-56: Update both Discord POST fetch calls in the workflow to use
an AbortController with a short bounded timeout, passing its signal to fetch and
clearing the timeout after completion. Preserve the existing POST payload and
response error handling while ensuring stalled webhooks are aborted before the
workflow-level timeout.
- Around line 39-42: Bound the fully constructed Discord embed title in the
webhook payload before assigning it to embeds[0].title. Update the title
expression in the payload creation block to truncate the combined action, PR
number, and pr.title value to Discord’s allowed embed-title length, preserving
the existing title content when it is within the limit.
In @.github/workflows/pr-agent.yml:
- Around line 17-19: Update the concurrency group expression in the workflow to
use the pull request number for pull_request_target events and the issue number
for issue_comment events before falling back to github.ref. Preserve the
existing workflow prefix so comment-triggered runs are isolated per PR.
In `@docs/CI_CD.md`:
- Around line 51-61: The CI/CD Overview in docs/CI_CD.md is outdated relative to
the current workflow. Update its branch triggers and job descriptions to match
ci.yml, including the per-package checks and Quality Gates, and remove the
obsolete E2E claim unless you clearly identify its current location or fallback.
In `@scripts/deploy/oauth-smoke.sh`:
- Around line 117-145: Update the inline Node.js validation in the
expected_values assignment so invalid JSON and missing auth fields write their
diagnostic messages to stderr instead of stdout. Preserve the nonzero exits and
allow the existing auth_config_client_id/expected_redirect_uri guard to report
the failure.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 145-165: Update the “Detect discord-player major bump beyond
pinned” step to pass the discord-player outputs through an env: mapping, then
reference those environment variables inside the shell script instead of inline
${{ steps.discord_player.outputs.* }} interpolation. Use the environment values
consistently for major extraction, error messages, and the compatibility message
while preserving the existing comparison and failure behavior.
- Around line 236-294: Update the “Install madge” step in the madge job to
install madge using an exact reviewed patch version instead of the ^7 range.
Keep the existing --no-save and --ignore-scripts flags unchanged.
In `@scripts/deploy/derive-webhook-origin.sh`:
- Around line 13-21: Wrap the URL construction in the inline Node script used by
derive-webhook-origin.sh with try/catch, preserving the existing empty-input
validation and origin output for valid URLs. On invalid input, catch the parsing
error and emit a concise contextual error message before exiting with failure,
matching trigger-deploy-webhook.sh’s graceful handling.
In `@scripts/deploy/oauth-smoke.sh`:
- Around line 160-166: Update the OAuth redirect contract checks around the
repeated curl attempts to create one temporary headers file before the loop and
register a trap that removes it on exit, including set -e failures. Reuse that
file for each attempt, remove the per-attempt mktemp and rm calls, and apply the
same cleanup approach to the later check around the referenced second block.
In `@scripts/deploy/trigger-deploy-webhook.sh`:
- Around line 25-59: Refactor the response handoff between
call_webhook_with_retry and try_webhook_url so the parsed HTTP code and body are
emitted in the caller’s expected order directly, avoiding repeated tail/sed
splitting and rejoining. Preserve retry decisions and the existing final
code/body output contract.
- Around line 8-12: Update the deploy webhook flow around call_webhook and the
DEPLOY_SHA usage to validate required WEBHOOK_SECRET and DEPLOY_SHA environment
variables explicitly with parameter-expansion checks, producing clear
variable-specific errors before printing “Triggering deploy webhook” or invoking
curl. Match the existing ${VAR:?} validation style used by the other extracted
scripts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a0b7126a-646d-4249-8de4-129551ce7045
📒 Files selected for processing (24)
.github/actions/docker-build-service/action.yml.github/workflows/auto-update-pr-branches.yml.github/workflows/bundle-size.yml.github/workflows/ci.yml.github/workflows/deploy-staging.yml.github/workflows/deploy.yml.github/workflows/external-pr-notify.yml.github/workflows/fork-ci-autoapprove.yml.github/workflows/labeler.yml.github/workflows/madge.yml.github/workflows/mutation.yml.github/workflows/path-portability.yml.github/workflows/pr-agent.yml.github/workflows/pr-labels.yml.github/workflows/quality.yml.github/workflows/queueresolver-canary.yml.github/workflows/renovate-health.yml.github/workflows/vercel-deploy-watch.ymldecisions/2026-07-27-ci-workflow-efficiency.mddocs/CI_CD.mdscripts/deploy/derive-webhook-origin.shscripts/deploy/oauth-smoke.shscripts/deploy/trigger-deploy-webhook.shscripts/deploy/wait-homelab-status.sh
💤 Files with no reviewable changes (5)
- .github/workflows/madge.yml
- .github/workflows/vercel-deploy-watch.yml
- .github/workflows/labeler.yml
- .github/workflows/path-portability.yml
- .github/workflows/queueresolver-canary.yml
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 24 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked by 14 unresolved issues from previous reviews.
Re-trigger cubic
|
LucasSantana-Dev
added a commit
that referenced
this pull request
Jul 30, 2026
## Summary Follow-up to #1890. The v2.37.0 deploy failed, auto-rolled back to v2.36.1, and nobody noticed for four days because a rolled-back deploy read like a generic red check. This PR makes the rollback state explicit: - **Rollback-aware failure notification**: restores a `Notify failure` step (`if: failure()`) in `deploy.yml`, backed by new `scripts/deploy/notify-deploy-failure.sh`. On any failed deploy it queries `/api/health/version` and annotates the run with the SHA production actually serves: "auto-rollback restored service on `<sha>`" vs. a generic failure, or "failure is in a post-deploy check" when production does run the expected SHA. - **Loud failure when production ends up older than the release**: the script compares the running SHA against the deploy target via the GitHub compare API; when the running SHA is strictly older (behind_by=0, ahead_by>0) it emits `PRODUCTION IS BEHIND THE RELEASE: <sha> is N commit(s) older than <tag>`. The `Validate deployed version` step's final error now also names the last observed production SHA instead of a bare mismatch. - **Pollable deployed SHA**: `/api/health` now includes `commitSha` (from the `COMMIT_SHA` env var already baked into the image via the Dockerfile ARG/ENV, same source as `/api/health/version`), so a scheduled check can alert when the running SHA drifts from the latest tag. Note: the `Notify failure` step referenced in the issue had been removed in #1897; this restores it in rollback-aware form. ## Verification - `npx jest tests/unit/routes/health.test.ts tests/integration/routes/health.test.ts` in packages/backend: 2 suites, 20 tests passed (includes 2 new `/api/health` commitSha tests). - `npm run type:check` (backend): clean; full monorepo type:check also ran via pre-commit hook and passed. - `eslint src/routes/health.ts`: clean (test file is ignored by the repo eslint config, as before). - `bash -n scripts/deploy/notify-deploy-failure.sh`: OK; deploy.yml parses as valid YAML. - Script branch behavior exercised locally with stubbed curl/gh: rollback case, post-deploy-check case, and unreachable-health case all emit the intended annotations and exit 0. Closes #1892 <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Make rolled-back deploys obvious in CI and surface the live commit SHA so silent rollbacks don’t read like generic failures. Failed deploys now say when production auto-rolled back, which SHA is serving, and when the failure happened before rollout. - **Bug Fixes** - Restore a rollback-aware "Notify failure" step in deploy.yml using scripts/deploy/notify-deploy-failure.sh. It reads /api/health/version to annotate the run with the live commitSha, distinguishes auto-rollback vs. post-deploy checks, adds a clear pre-deploy failure message when no SHA is resolved, treats HTTP errors as unreachable, and warns when production is behind the release. - Expose commitSha on /api/health and /api/health/version (from COMMIT_SHA) and add tests. The "Validate deployed version" step now prints the expected and last-seen production SHA with a clear hint when they differ. <sup>Written for commit 07ab6ae. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/1922?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Health checks now include a deployed commit identifier when available. * Deployment failure notifications now distinguish pre-deployment failures from post-deployment verification failures. * **Bug Fixes** * Improved “validate deployed version” timeout reporting by showing the last served commit identifier (when detected) and clarifying rollback vs. flaky checks. * Deployment failure diagnostics better differentiate between unreachable health checks and missing commit info. * **Tests** * Added unit test coverage for `/api/health` with `COMMIT_SHA` set and unset. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a){kind=link}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CI workflow efficiency refactor plus external-contributor automation. 24 workflows audited; dead weight removed, micro-workflows folded, duplication consolidated, deploy logic extracted to testable scripts.
External contributors
external-pr-notify.yml: Discord alert on external PR open/reopen/ready + daily digest of open external PRsfork-ci-autoapprove.yml: auto-approves queued CI runs for returning contributors andCI_AUTO_APPROVE_AUTHORSallowlist; first-timers get a comment + Discord pingpr-agent.yml: switched topull_request_targetso fork PRs actually get AI review (forkpull_requestruns get no secrets); fixed concurrency groupEfficiency refactor (ADR:
decisions/2026-07-27-ci-workflow-efficiency.md)vercel-deploy-watch.yml(dead; watched non-repo projects on a 15-min cron), Socket.dev no-op step, advisory TruffleHog step (secretlint + GitGuardian remain the blocking layer)ci.yml: path-portability, madge (job named exactlymadge / packages/bot, preserving the required check context), discord-player major-pin guard; deleted the 3 source workflowspr-labels.yml(fork-safe); mutation.yml 3 jobs into 1 matrix job; bundle-size dropped informational compressed-size job (kept size-limit gate); docker build retry duplication into composite actiondeploy.yml763 to 360 lines: webhook trigger, origin derivation, status polling, OAuth smokes extracted toscripts/deploy/*.sh(shellcheck-clean, mock-server tested); deploy.yml + deploy-staging.yml gained the checkout step they now needSafety
pull_request(GitHub-hosted runners, no secrets, read-only token);pull_request_targetworkflows never check out PR codeactionlint+ YAML parse + shellcheck sweep cleanTest plan
Summary by cubic
Streamlined CI by consolidating workflows, removing dead jobs, and adding automation so external contributors’ fork PRs are visible and unblocked. Deploy logic is extracted into testable scripts; required checks and safety guarantees remain intact.
New Features
external-pr-notify.yml: Discord alert on external PR open/reopen/ready, plus daily digest.fork-ci-autoapprove.yml: auto-approves queued CI for trusted external authors; first-timers get a comment.pr-agent.yml: switched topull_request_targetso fork PRs receive reviews; fixed concurrency grouping to key by PR number.pr-labels.yml: unified path-based and size labeling in one fork-safe workflow.Refactors
path-portability.yml,madge.yml, and thediscord-playerpin guard intoci.yml(installsripgrep/jq); removedqueueresolver-canary.yml.vercel-deploy-watch.yml, Socket.dev no-op, advisory TruffleHog step;renovate-health.ymlnow exits 0 when its tracking issue exists; auto-update cron moved to hourly;quality.ymlno longer double-lints.bundle-size.ymlby dropping the informational compressed-size job (kept size-limit gate).Docker build servicecomposite action (.github/actions/docker-build-service) to dedupe Docker Buildx setup and retries.deploy.ymlfrom ~763 to ~360 lines; extracted webhook trigger, origin derivation, status polling, and OAuth smokes toscripts/deploy/*.sh; added checkout todeploy.ymlanddeploy-staging.yml.pull_requestwith no secrets;pull_request_targetjobs never check out PR code; actionlint/YAML/shellcheck are clean.Written for commit 071ba3b. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Documentation