feat(ci): add Slack notifications for build success/failure

[justin-layerv]

Summary

qurl-python had no Slack notifications. Adds a notify job matching the standardized format across all LayerV repos.

Notification format

:white_check_mark: *QURL Python SDK Build* successful

Branch:   main (abc1234)     Trigger:  Push by username
Tests:    :white_check_mark:           Python:   3.10, 3.12, 3.13

Commit: `feat: add response validation`

abc1234  |  View workflow

Checklist

• Verify SLACK_WEBHOOK_URL secret exists in qurl-python repo settings
• Merge and push to main to trigger a real notification

🤖 Generated with Claude Code

[claude]

PR Review: feat(ci): add Slack notifications for build success/failure

Overall

Straightforward addition of a Slack notification job to the CI workflow. The approach is reasonable — single inline shell step, graceful skip when the secret is missing, continue-on-error: true so notifications never block the pipeline.

Issues

1. Script injection via expressions in run block (Security — Medium)

COMMIT_MESSAGE is set via env: (good), but github.ref_name, github.actor, github.server_url, github.repository, github.run_id, and github.sha are interpolated directly via expression syntax inside the run: block. A crafted branch name containing backticks or command substitution could inject shell commands in the workflow context.

Recommendation: Move all expression interpolations into env: variables (like you already do for COMMIT_MESSAGE), then reference them as shell variables in the script. This is the standard mitigation for GitHub Actions script injection.

2. Python version list is hardcoded (Maintainability — Low)

The notification hardcodes Python: 3.10, 3.12, 3.13 but the actual matrix is defined separately in the test job. If the matrix changes, this will silently drift. Worth a comment to keep in sync with the test matrix at minimum.

3. JSON construction via string concatenation (Robustness — Low)

Building JSON by concatenating shell strings is fragile. The sed escaping on the commit message handles backslashes and double quotes but misses newlines, tabs, and control characters. Consider using jq (pre-installed on ubuntu-latest) to build the payload, which handles all escaping correctly.

Looks Good

• if: always() && github.event_name != 'pull_request' — correct gate.
• continue-on-error: true and timeout-minutes: 5 — good defensive defaults.
• Graceful exit when SLACK_WEBHOOK_URL is not set.
• curl -sfS flags — appropriate for CI.

Summary

The main item to address before merging is issue 1 (script injection via expression interpolation in run: blocks). This is a well-documented GitHub Actions security pitfall. The rest are minor improvements.

[justin-layerv]

CR feedback addressed

Fixed

• Script injection via expression interpolation — Moved all github.* context expressions from run: block into env: variables. A crafted branch name like `curl attacker.com` could have executed arbitrary commands when interpolated directly in the shell script.

Deferred

• Hardcoded Python versions — Low priority, comment-level concern. The matrix is 3 lines above the notification.
• jq for JSON construction — Consistent with all other repos' notification patterns.

[claude]

PR Review - feat(ci): add Slack notifications. Overall well-structured. Security is good - github context in env vars prevents injection. Potential issues: (1) hardcoded Python versions will go stale if matrix changes, (2) JSON via bash string concat is fragile but acceptable with continue-on-error, (3) HEADER on line 92 missing Build. Looks good to merge once SLACK_WEBHOOK_URL secret is confirmed.

[claude]

Detailed Review

Security: GitHub context in env vars prevents shell injection via crafted branch names or actor names. Webhook URL handling gracefully exits when secret is missing. Commit message sanitization (line 128) escapes backslashes and quotes, but newlines/control characters could break JSON - consider jq -Rs.

Issue 1 - Hardcoded Python versions (line 121): The version string is hardcoded but the matrix is in the test job. If the matrix changes, notifications will be stale. Add a sync comment.

Issue 2 - JSON string concatenation: Fragile but acceptable with continue-on-error. Consider jq if expanding.

Issue 3 - Matrix result semantics: needs.test.result aggregates all matrix jobs correctly (success = all pass).

Nit: HEADER on line 92 says QURL Python SDK but other branches say QURL Python SDK Build - missing word Build for incomplete case.

Nit: curl write-out with http_code would help debug notification failures.

Good: timeout-minutes + continue-on-error, skips PR builds, clean job separation.

Verdict: Looks good to merge. Security is well-handled, continue-on-error prevents CI blocking.