chore: add slack notification with execution link

[assafgi]

No description provided.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[assafgi]

• chore: tolerate transient empty status in mq test poll loops instead of failing #2591
• chore: add slack notification with execution link #2587 👈 (View in Graphite)
• fix: add admission control to block finalizers removal not made by the operator service account #2575
• main

---

How to use the Graphite Merge Queue

Add the label main-merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has required the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[claude]

Claude finished @assafgi's task in 48s —— View job

---

Code Review: Slack notification with execution link

Reviewed the change against origin/05-31-fix_add_admission_control.... This adds a Slack notification posted after the upgrade-extended test execution is created. The change is small and the overall approach is reasonable (non-blocking curl, skipped when the webhook secret is empty). A few things worth addressing:

---

🔴 Script injection via direct ${{ }} interpolation — dagger_ci_on_merge_queue_plan.yaml:229

${{ github.head_ref || github.ref_name }} is interpolated directly into the shell script body. Branch/ref names are attacker-influenceable and can contain shell metacharacters (backticks, $(...), ";), so this is a classic GitHub Actions script-injection vector — the value is expanded by the runner before the shell sees it.

The same concern applies to ${{ secrets.SLACK_WEBHOOK_URL }} on lines 243–244: interpolating a secret into the script text (rather than passing it through env:) is discouraged, since a value containing shell-significant characters could break or alter the command, and it's harder for log masking to reason about.

Recommended: pass these through the step's env: and reference them as shell variables, which are not re-parsed by the runner:

        env:
          SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }}
          HEAD_BRANCH: ${{ github.head_ref || github.ref_name }}
          RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}

          SLACK_PAYLOAD=$(jq -n \
            --arg url "$EXECUTION_URL" \
            --arg branch "$HEAD_BRANCH" \
            --arg run_url "$RUN_URL" \
            '{ ... }')
          if [ -n "$SLACK_WEBHOOK_URL" ]; then
            curl -sf -X POST "$SLACK_WEBHOOK_URL" \
              -H "Content-Type: application/json" \
              -d "$SLACK_PAYLOAD" || echo "::warning::Failed to send Slack notification"
          fi

🟡 "test started" notification fires even if it later fails to start meaningfully — :225-247

The notification is sent immediately after execution_id is obtained but before any polling. That's fine for a "started" message, but note there is no corresponding "succeeded/failed" notification — the Slack channel only ever sees starts, never outcomes. If the intent is observability, consider also notifying on the terminal status in the polling loop (lines 251–266). Optional, depending on intent.

🟡 Hardcoded test name — :232,238

"upgrade-extended test started" is duplicated as a literal in both text and the block. Minor, but if this snippet gets copied to other test jobs it'll be easy to forget to update. A shell variable for the test label would make it reusable.

🟢 Nits

• Trailing-whitespace cleanup on line 267 is a nice touch and unrelated-but-harmless.
• curl -sf correctly fails the request silently and the || echo "::warning::" keeps the notification best-effort/non-blocking — good.

---

Summary

Functionally sound and non-blocking. The main thing I'd fix before merge is moving github.head_ref/secrets.SLACK_WEBHOOK_URL out of inline ${{ }} interpolation and into env: to close the script-injection vector (finding 🔴). The rest are optional improvements.

Note: I can't push a fix here myself — GitHub App permissions don't allow modifying files under .github/workflows/. The author will need to apply the env: change manually.
· 06-11-chore_add_slack_notification_with_execution_link

[graphite-app]

Merge activity

• Jun 11, 7:27 AM UTC: assafgi added this pull request to the Graphite merge queue.
• Jun 11, 7:28 AM UTC: CI is running for this pull request on a draft pull request (#2588) due to your merge queue CI optimization settings.
• Jun 11, 7:32 AM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #2575.
• Jun 11, 7:32 AM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #2575.
• Jun 11, 7:39 AM UTC: assafgi added this pull request to the Graphite merge queue.
• Jun 11, 7:39 AM UTC: CI is running for this pull request on a draft pull request (#2589) due to your merge queue CI optimization settings.
• Jun 11, 7:42 AM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #2575.
• Jun 11, 7:42 AM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #2575.
• Jun 11, 8:39 AM UTC: assafgi added this pull request to the Graphite merge queue.
• Jun 11, 8:40 AM UTC: CI is running for this pull request on a draft pull request (#2590) due to your merge queue CI optimization settings.
• Jun 11, 9:13 AM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #2575.
• Jun 11, 9:13 AM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #2575.
• Jun 11, 10:08 AM UTC: kristina-solovyova added this pull request to the Graphite merge queue.
• Jun 11, 10:09 AM UTC: CI is running for this pull request on a draft pull request (#2592) due to your merge queue CI optimization settings.
• Jun 11, 11:01 AM UTC: Merged by the Graphite merge queue via draft PR: #2592.

[graphite-app]

Graphite Automations

"Add anton/matt/sergey/kristina as reviwers on operator PRs" took an action on this PR • (06/11/26)

2 reviewers were added to this PR based on Anton Bykov's automation.

[claude]

Claude finished @assafgi's task in 35s —— View job

---

Re-review: no code changes since last review

This synchronize event came from a downstack rebase, not a content change — the diff for .github/workflows/dagger_ci_on_merge_queue_plan.yaml is byte-for-byte identical to the version I reviewed at 07:26 UTC (file SHA 1f37c7b). Per the repo's "exit early if nothing changed" guidance, I won't repeat the full review.

The one open item from the prior review still stands:

🔴 Script injection via inline ${{ }} interpolation — dagger_ci_on_merge_queue_plan.yaml:229,243-244. ${{ github.head_ref || github.ref_name }} and ${{ secrets.SLACK_WEBHOOK_URL }} are still expanded into the shell script body by the runner before the shell sees them. Branch/ref names are attacker-influenceable, so this remains a classic Actions script-injection vector. Move them into the step's env: and reference them as $HEAD_BRANCH / $SLACK_WEBHOOK_URL shell variables (full snippet in my earlier review comment).

Note: I can't apply this myself — GitHub App permissions don't allow modifying files under .github/workflows/. It needs to be applied manually. Fix this →

The 🟡/🟢 items (no terminal-status notification, duplicated test-name literal, nits) are unchanged and remain optional.
· 06-11-chore_add_slack_notification_with_execution_link

[claude]

Claude finished @assafgi's task in 1m 39s —— View job

---

Re-review: new HTTP error-handling added since last review

This synchronize event did change the file (SHA 1f37c7b → 578a2b6e). Beyond the Slack snippet, the POST to /api/executions now has explicit HTTP-status handling in both jobs. Reviewing the new logic below.

• Fetch latest diff vs base
• Review new HTTP error-handling block
• Re-check open findings from prior review

---

✅ HTTP error handling — good improvement (:214-227, :361-374)

Switching curl -sf → -sS -w "\n%{http_code}" and checking the code explicitly is a real upgrade: you now surface the response body on failure (::error::… Response: $RESPONSE) instead of curl exiting silently. With GitHub Actions' default bash --noprofile --norc -eo pipefail, the edge cases hold up:

• -sS (no -f) returns exit 0 on 4xx/5xx, so the manual HTTP_CODE check is what catches them — correct.
• A transport failure (after --retry 3 --retry-connrefused) exits non-zero and set -e fails the step.
• curl's "no response" sentinel 000 evaluates as 0 -lt 200 → triggers the error branch. Fine.

No bugs in this block.

🔴 Script injection via inline ${{ }} — still open (:236, :250-251)

Unchanged from my earlier review. ${{ github.head_ref || github.ref_name }} (line 236) and ${{ secrets.SLACK_WEBHOOK_URL }} (lines 250–251) are still expanded into the script body by the runner before bash sees them. Branch/ref names are attacker-influenceable → classic Actions script-injection vector. Move both into the step's env: and reference as $HEAD_BRANCH / $SLACK_WEBHOOK_URL:

        env:
          SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }}
          HEAD_BRANCH: ${{ github.head_ref || github.ref_name }}

          SLACK_PAYLOAD=$(jq -n --arg url "$EXECUTION_URL" --arg branch "$HEAD_BRANCH" ... )
          if [ -n "$SLACK_WEBHOOK_URL" ]; then
            curl -sf -X POST "$SLACK_WEBHOOK_URL" -H "Content-Type: application/json" -d "$SLACK_PAYLOAD" \
              || echo "::warning::Failed to send Slack notification"
          fi

Fix this →

🟡 Slack block only in upgrade_extended_test, not clients_only_test

The new HTTP-handling change was applied to both jobs, but the Slack "test started" notification was added only to the upgrade-extended job (:232-254). If the intent is observability across the merge-queue plan, clients_only_test silently has no notification. Intentional? Worth a one-line confirmation.

🟡 Duplicated HTTP-handling block

The ~9-line capture/check block is copy-pasted in both jobs (:214-227 and :361-374). Fine for now, but a composite action / reusable step would prevent drift if a third test job is added.

---

Summary

The new HTTP error-handling is a genuine, bug-free improvement. The blocking item remains the 🔴 script-injection vector — moving github.head_ref and secrets.SLACK_WEBHOOK_URL into env:. The 🟡 items (Slack only on one job, duplicated block) are optional.

Note: I can't apply the fix — GitHub App permissions don't allow modifying files under .github/workflows/. It needs to be applied manually.
· 06-11-chore_add_slack_notification_with_execution_link

[claude]

Claude finished @assafgi's task in 1m 6s —— View job

---

Re-review: PR-title injection now fixed via env:

The file changed since my last review (SHA 578a2b6e → 7c7acd7e). The Slack payload was reworked — it now reports PR title / number / URL / target branch instead of head_ref, and github.event.pull_request.title was moved into the step's env: block. Reviewing the delta below.

• Fetch latest diff vs base
• Re-assess the open 🔴 script-injection finding
• Check the reworked Slack payload

---

✅ Script-injection vector — substantially addressed (:182-184)

This is the right fix. The PR title is the genuinely attacker-controllable value (a PR author can put $(...), backticks, "; etc. in it), and it's now passed through env: PR_TITLE and consumed only via jq --arg pr_title "$PR_TITLE". The runner no longer expands it into the script body, and jq --arg safely JSON-escapes it — so both the shell-injection and the JSON-breakage paths are closed. Good.

🟡 secrets.SLACK_WEBHOOK_URL still inline (:259-261)

if [ -n "${{ secrets.SLACK_WEBHOOK_URL }}" ]; then
  curl -sf -X POST "${{ secrets.SLACK_WEBHOOK_URL }}" ...

Lower risk than the title — a webhook secret is admin-set, not attacker-supplied — so I'm not blocking on it. But it's still best practice to move it to env: (a value with shell-significant chars could break the command, and inline secrets are harder for log-masking to reason about):

        env:
          PR_TITLE: ${{ github.event.pull_request.title }}
          SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }}

          if [ -n "$SLACK_WEBHOOK_URL" ]; then
            curl -sf -X POST "$SLACK_WEBHOOK_URL" -H "Content-Type: application/json" -d "$SLACK_PAYLOAD" \
              || echo "::warning::Failed to send Slack notification"
          fi

Fix this →

The remaining ${{ }} values fed to jq --arg (pr_number, html_url, base_ref, server_url, repository, run_id) are either GitHub-controlled or numeric; base_ref is the lowest-trust of them but is a target-branch name and low-risk. Optional to move for consistency.

🟡 Slack notification only in upgrade_extended_test, not clients_only_test

Unchanged from last review. The HTTP-status handling was added to both jobs (:217-227 and :367-377), but the Slack "test started" block lives only in the upgrade-extended job (:235-261). If observability across the whole merge-queue plan is the goal, clients_only_test is silent. Intentional?

---

Summary

The blocking 🔴 from my prior reviews is now resolved — the user-controllable PR title is correctly routed through env: + jq --arg. What remains is optional: moving the (trusted) SLACK_WEBHOOK_URL secret to env: for hygiene, and deciding whether clients_only_test should also notify. Nothing here blocks merge from my side.

Note: I can't apply these myself — GitHub App permissions don't allow modifying files under .github/workflows/.
· 06-11-chore_add_slack_notification_with_execution_link