Fix CI: deploy stages skipped on push to main#16
Conversation
…ncestors GitHub Actions skips downstream jobs when any transitive dependency was skipped, even if the direct dependency succeeded. Since ai-review and security-review are skipped on push (PR-only), deploy-staging and all subsequent jobs were skipped too. Fix: add 'if: always() && needs.<job>.result == success' to deploy-staging, e2e-tests-staging, and deploy-production so they run when their direct dependency succeeds regardless of skipped ancestors.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM. This PR correctly identifies and fixes a common GitHub Actions pipeline issue where skipped statuses from conditional jobs (like ai-review and security-review which are PR-only) can transitively cause downstream jobs to also skip, even if they're meant to run on push to main.
The fix, applying if: always() && needs.<job>.result == 'success' to deploy-staging, e2e-tests-staging, and deploy-production, is the standard and recommended way to ensure jobs evaluate the result of their dependencies rather than just their status.
Furthermore, the build job's if: always() combined with its internal logic to conditionally check the success of ai-review and security-review only for pull_request events, correctly gates the pipeline without blocking push to main scenarios.
This is a well-understood problem in GitHub Actions, and the proposed solution is robust and correctly implemented. No security concerns or architectural misalignments were found.
There was a problem hiding this comment.
🔒 Security Review
Security Score: A | 1 INFO
Security Review: CI/CD Pipeline Fix
Status: APPROVED ✓
This PR fixes a critical CI/CD issue where deploy stages were skipped after PR merge due to GitHub Actions transitive job dependencies. The fix is sound and does not introduce security vulnerabilities.
Key Findings
✓ No security risks introduced. The changes are purely workflow orchestration logic:
- Added
if: always() && needs.<job>.result == 'success'conditions to deploy-staging, e2e-tests-staging, and deploy-production - This correctly bypasses the skipped reviewer jobs on push-to-main while ensuring build success is required
- All sensitive credentials (GCP_SA_KEY, SLACK tokens, NEO4J_PASSWORD, ANTHROPIC_API_KEY) remain properly scoped to their respective jobs
- No credentials are hardcoded, logged, or exposed in error messages
OWASP Top 10 Analysis
- A01 Access Control: PASS - Job permissions and secrets scoping remain intact
- A02 Cryptographic Failures: PASS - Credential handling unchanged
- A07 Authentication Failures: PASS - SLACK_SIGNING_SECRET verification in bot code unaffected by workflow changes
- A09 Logging & Monitoring: PASS - No new logging that could expose PII or credentials
- All others: N/A - Workflow is orchestration only, no application code changes
Design Verification
The fix correctly implements conditional gate logic:
- On PR: All jobs run (ai-review, security-review, builds, deploy-staging, e2e, deploy-production)
- On push to main: Reviewers are skipped, but deploy-staging correctly waits for
needs.build.result == 'success' - Downstream dependencies: e2e-tests-staging depends on deploy-staging success, deploy-production depends on e2e success AND main branch check
This matches the PR's stated test plan and is consistent with secure CI/CD patterns.
Credential Scope Review
All secrets remain properly isolated:
GCP_SA_KEY: Used only in build/deploy jobsSLACK_STAGING_*: E2E tests onlyNEO4J_STAGING_PASSWORD: E2E tests onlyANTHROPIC_API_KEY: E2E tests only (separate from ai-reviewer's GEMINI_API_KEY)
No regression in the repository's documented security patterns.
Recommendation
APPROVE - This PR successfully fixes the deploy-on-push issue without introducing security vulnerabilities or violating established security patterns.
Findings
🔵 [INFO] The build gate job uses if: always() which allows it to proceed even if ai-review or security-review are skipped (not failed). This is intentional design for push-to-main scenarios, but relies on downstream job conditions to enforce the gate correctly.
File: .github/workflows/ci.yml:line 86
Category: A05_misconfiguration
Impact: On a push to main, the build gate will succeed even though reviewer jobs are skipped. The actual enforcement happens in downstream deploy-staging/e2e-tests-staging conditions which correctly check needs.build.result == 'success'. This is safe by design but could be clearer.
Recommendation: This is acceptable as designed. The downstream jobs properly enforce needs.build.result == 'success' which prevents broken builds from deploying. Consider adding a comment in the build job explaining the intentional if: always() pattern for clarity.
OWASP Top 10 Checklist
| Category | Status |
|---|---|
| A01 Access Control | ✅ PASS |
| A02 Crypto Failures | ✅ PASS |
| A03 Injection | ✅ PASS |
| A04 Insecure Design | ✅ PASS |
| A05 Misconfiguration | ✅ PASS |
| A06 Vulnerable Components | ✅ PASS |
| A07 Auth Failures | ✅ PASS |
| A08 Integrity Failures | ✅ PASS |
| A09 Logging Monitoring | ✅ PASS |
| A10 Ssrf | ✅ PASS |
🤖 Security review powered by Claude
1 participant
Summary
ai-reviewandsecurity-revieware skipped on push (PR-only), all downstream jobs afterbuildgot skipped even thoughbuildsucceededif: always() && needs.<job>.result == 'success'to deploy-staging, e2e-tests-staging, and deploy-productionTest plan