test(deploy): e2e smoke for weekly-digest --mode cloud#99
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a scheduled/manual GitHub Actions workflow and a Node.js E2E smoke test that builds and deploys a weekly-digest agent to staging, triggers a cron tick (or checks active status), polls GitHub Issues for the digest, closes the issue, and ensures teardown; workflow posts Slack alerts on failure. ChangesWeekly Digest E2E Smoke Test
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-e2e.yml:
- Around line 48-70: The smoke test step with id "smoke" currently uses
continue-on-error: true which prevents the job from failing; add a final step
(e.g., name "Fail job on smoke failure") that runs only when steps.smoke.outcome
== 'failure' and executes an explicit exit 1 to propagate the failure to the job
result, keeping the existing "Report smoke result" and "Notify
`#workforce-alerts`" steps as-is so logs and notifications still run before the
job is marked failed.
In `@packages/deploy/test/e2e/weekly-digest.smoke.test.ts`:
- Around line 37-38: The smoke test currently backdates startedAt and selects
the first matching open GitHub issue updated after that timestamp, which can
collide with concurrent runs; generate a unique run marker (e.g. const runMarker
= process.env.GITHUB_RUN_ID || <generated id>) and include that marker in any
created/updated issue content (title or body) when emitting the weekly digest,
then change the lookup/verification logic (the code paths that use startedAt and
the issue-searching logic referenced around the lookup and the later
verification blocks) to require the found issue’s body/title contains this
runMarker before treating it as this run’s result; also update any assertions
that currently rely only on updated_at to check the marker so overlapping runs
don’t interfere.
- Around line 299-317: The destroyDeployment function currently returns early
inside the loop on the first successful/acceptable DELETE, skipping the second
cleanup; change it so both candidate URLs in the candidates array are always
requested: for each url call fetch(...) with method 'DELETE' and
authHeaders(stagingToken), treat res.ok or status 404/405/501 as a success but
continue to the next candidate, set lastError when a request returns an
unexpected status (using new Error(`${res.status} ${await res.text()}`)), and
after the loop throw lastError if present; adjust any early return inside the
for..of so both agent and deployment deletes are attempted even when the first
succeeds.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f71a634b-d805-4eec-a488-71d9d2920409
📒 Files selected for processing (2)
.github/workflows/deploy-e2e.ymlpackages/deploy/test/e2e/weekly-digest.smoke.test.ts
Comment on lines
+37
to
+38
| const startedAt = new Date(Date.now() - 120_000); | ||
|
|
Contributor
There was a problem hiding this comment.
Correlate the observed GitHub issue to this run.
This lookup accepts the first open issue whose title matches Weekly digest — ... and whose updated_at is newer than a backdated timestamp. Two overlapping smoke runs against the shared fixture repo can therefore pass on, or close, each other's issue. Add a run-specific marker (for example deploymentId or GITHUB_RUN_ID) to the emitted issue content and verify that marker here before treating the issue as this deployment's result.
Also applies to: 94-100, 320-355
🤖 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 `@packages/deploy/test/e2e/weekly-digest.smoke.test.ts` around lines 37 - 38,
The smoke test currently backdates startedAt and selects the first matching open
GitHub issue updated after that timestamp, which can collide with concurrent
runs; generate a unique run marker (e.g. const runMarker =
process.env.GITHUB_RUN_ID || <generated id>) and include that marker in any
created/updated issue content (title or body) when emitting the weekly digest,
then change the lookup/verification logic (the code paths that use startedAt and
the issue-searching logic referenced around the lookup and the later
verification blocks) to require the found issue’s body/title contains this
runMarker before treating it as this run’s result; also update any assertions
that currently rely only on updated_at to check the marker so overlapping runs
don’t interfere.
Track K: Track K — End-to-end smoke test See workforce/docs/plans/deploy-v1-schema-cascade-spec.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/deploy-e2e.yml (1)
8-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReduce secret exposure scope and remove unused permission.
Secrets at lines 22, 25, and 27 are exposed to all steps via job-level
env, includingpnpm installand build steps that don't use them. Move secrets to the specific steps that need them:WORKFORCE_E2E_STAGING_TOKENandWORKFORCE_E2E_GITHUB_TOKENto the "Run weekly-digest smoke" step, andSLACK_WEBHOOK_URLto the "Notify#workforce-alerts" step. Additionally, remove theissues: writepermission at line 10—no step in the workflow creates or updates issues, so this violates least-privilege principle. Thecurlcommand at line 69 should also include--failto catch HTTP errors.Suggested hardening diff
permissions: contents: read - issues: write jobs: weekly-digest: @@ env: WORKFORCE_E2E_STAGING_URL: ${{ vars.WORKFORCE_E2E_STAGING_URL }} WORKFORCE_E2E_STAGING_WORKSPACE_ID: ${{ vars.WORKFORCE_E2E_STAGING_WORKSPACE_ID }} WORKFORCE_E2E_FIXTURE_REPO: AgentWorkforce/deploy-e2e-fixtures - WORKFORCE_E2E_STAGING_TOKEN: ${{ secrets.WORKFORCE_E2E_STAGING_TOKEN }} - WORKFORCE_E2E_GITHUB_TOKEN: ${{ secrets.WORKFORCE_E2E_GITHUB_TOKEN || github.token }} - SLACK_WEBHOOK_URL: ${{ secrets.WORKFORCE_ALERTS_SLACK_WEBHOOK_URL }} @@ - name: Run weekly-digest smoke id: smoke + env: + WORKFORCE_E2E_STAGING_TOKEN: ${{ secrets.WORKFORCE_E2E_STAGING_TOKEN }} + WORKFORCE_E2E_GITHUB_TOKEN: ${{ secrets.WORKFORCE_E2E_GITHUB_TOKEN }} run: node --test packages/deploy/test/e2e/weekly-digest.smoke.test.ts @@ - name: Notify `#workforce-alerts` if: failure() && steps.smoke.outcome == 'failure' + env: + SLACK_WEBHOOK_URL: ${{ secrets.WORKFORCE_ALERTS_SLACK_WEBHOOK_URL }} run: | if [ -z "${SLACK_WEBHOOK_URL}" ]; then echo "No Slack webhook configured; skipping `#workforce-alerts` notification." exit 0 fi payload=$(node -e "console.log(JSON.stringify({text: 'SMOKE_TEST: FAIL — weekly-digest deploy E2E failed. See ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}'}))") - curl -sS -X POST -H 'content-type: application/json' --data "$payload" "$SLACK_WEBHOOK_URL" + curl -sS --fail -X POST -H 'content-type: application/json' --data "$payload" "$SLACK_WEBHOOK_URL"🤖 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/deploy-e2e.yml around lines 8 - 10, The workflow currently exposes secrets via job-level env and grants an unnecessary permission; remove the job-level environment entries for WORKFORCE_E2E_STAGING_TOKEN, WORKFORCE_E2E_GITHUB_TOKEN, and SLACK_WEBHOOK_URL and instead add WORKFORCE_E2E_STAGING_TOKEN and WORKFORCE_E2E_GITHUB_TOKEN as env only on the "Run weekly-digest smoke" step and add SLACK_WEBHOOK_URL as env only on the "Notify `#workforce-alerts`" step; also remove the top-level permission issues: write from the permissions block and update the curl invocation (the curl command in the notify step) to include --fail so HTTP errors are detected.
🤖 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-e2e.yml:
- Line 69: The curl invocation that posts to Slack currently uses "curl -sS -X
POST -H 'content-type: application/json' --data \"$payload\"
\"$SLACK_WEBHOOK_URL\"" which will still exit zero on HTTP 4xx/5xx; update the
command to fail on non-2xx (e.g., add --fail or --fail-with-body) and check the
exit status so the workflow fails when Slack returns an error; modify the POST
command (the curl line) to use --fail-with-body --show-error (or --fail) and
capture/check its exit code (or let the step fail) so non-2xx responses cause
the job to fail.
---
Duplicate comments:
In @.github/workflows/deploy-e2e.yml:
- Around line 8-10: The workflow currently exposes secrets via job-level env and
grants an unnecessary permission; remove the job-level environment entries for
WORKFORCE_E2E_STAGING_TOKEN, WORKFORCE_E2E_GITHUB_TOKEN, and SLACK_WEBHOOK_URL
and instead add WORKFORCE_E2E_STAGING_TOKEN and WORKFORCE_E2E_GITHUB_TOKEN as
env only on the "Run weekly-digest smoke" step and add SLACK_WEBHOOK_URL as env
only on the "Notify `#workforce-alerts`" step; also remove the top-level
permission issues: write from the permissions block and update the curl
invocation (the curl command in the notify step) to include --fail so HTTP
errors are detected.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 470f213a-0063-48bd-83b5-8679d2eed2b8
📒 Files selected for processing (2)
.github/workflows/deploy-e2e.ymlpackages/deploy/test/e2e/weekly-digest.smoke.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/deploy/test/e2e/weekly-digest.smoke.test.ts
- destroyDeployment: continue to next candidate on acceptable DELETE response so both deployment and agent cleanups are always attempted (was returning early, leaking agent resources) - deploy-e2e workflow: use curl -fsS --retry for Slack webhook so non-2xx responses fail the alert step instead of being silently swallowed Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Spec reference
Source spec: workforce/docs/plans/deploy-v1-schema-cascade-spec.md
Track section: Track K — End-to-end smoke test
Final signoff
Final gate (typecheck + tests)
Self-reflection report
Known gaps after this PR
Co-Authored-By: Ricky deploy-v1 schema cascade noreply@agentworkforce.com