Skip to content

fix(guardrail): detect direct pushes using merged PR records - #54

Open
greenc-FNAL wants to merge 2 commits into
mainfrom
fix/guardrail-merged-pr-detection
Open

greenc-FNAL wants to merge 2 commits into
mainfrom
fix/guardrail-merged-pr-detection

Conversation

@greenc-FNAL

@greenc-FNAL greenc-FNAL commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Identify PR merges into this repository's main using GitHub's merged-PR records, including rebase merges, rather than trusting committer names or exempting bot pushes.
  • Correlate matching PRs with the captured push payload's repository.pushed_at, accepting epoch or UTC ISO timestamps within a documented 60-second tolerance. This avoids runner-queue and rerun timing errors and alerts on stale merge restorations.
  • Always alert for forced updates; if event timing is missing or invalid, warn and send an alert rather than suppressing it.
  • Validate the Slack webhook before the successful-merge early exit and require HTTP 200, rejecting redirects and delivery failures.
  • Test the actual shell script with isolated GitHub and Slack stubs in a dedicated CI job that also runs for test-only changes.

Scope and relationships

Changes are limited to .github/workflows/guardrail-audit-alert.yaml, .github/workflows/ci.yaml, and tests/test_guardrail_audit_alert.py. Action interfaces, releases, dependencies, and existing Python tooling are unchanged. The guardrail workflow and test file are identical across all fifteen action repositories; other CI content and pins are preserved.

This addresses the concern reviewed in action-detect-act-env#53, without using the unsupported github.run_started_at context from Copilot's corrective commit. The guardrail correction can merge independently of the actionlint-pinning PR.

Validation

  • Pinned actionlint 1.7.12, standalone ShellCheck, Bash syntax, whitespace, and repository pre-commit hooks passed.
  • All ten guardrail test methods passed in every repository, covering 35 checks per repository, including timestamp/window boundaries, delayed runs/reruns, pagination, PR identity, forced and bot pushes, missing configuration, lookup failures, payload escaping, and HTTP redirects/delivery failures.
  • Workflow/test byte equality and absence of unrelated CI changes were checked across all fifteen repositories.
  • No live Slack webhook delivery was exercised.

Known limitation

This is best-effort temporal correlation, not complete event-provenance tracking. A rewind and restoration within 60 seconds of the original PR merge may still suppress the restoration alert. That bounded limitation is intentional; exact detection of every replay is outside this change's scope.

  • Guardrail workflow: Detects qualifying merges into main through GitHub’s merged-PR records, including rebase merges. It suppresses an alert only when the PR merge commit, repository, target branch, and repository.pushed_at match within 60 seconds.
  • Alert handling: Forced pushes bypass the PR lookup and send an alert. Missing or invalid push timestamps trigger a warning and send an alert. The workflow validates the Slack webhook before checking for a matching merge and fails unless Slack returns HTTP 200.
  • Tests: Adds tests that run the workflow’s shell script with stubbed GitHub and Slack commands. Coverage includes merge matching, time boundaries, pagination, forced pushes, invalid timestamps, lookup failures, webhook responses, payload escaping, and event context.
  • CI: Adds a guardrail-test job that runs the test module after successful setup. It is not gated on changed files.
  • Known limitation: A rewind and restore within 60 seconds of the original PR merge may still match the merge record and suppress the restoration alert.
  • Validation status: The supplied evidence describes the test coverage, but does not establish that tests or other checks were run for this revision.

@coderabbitai

coderabbitai Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Walkthrough

The guardrail alert workflow now checks push events against merged pull requests before sending Slack alerts. It validates webhook delivery, and a new unittest suite covers the workflow behavior. CI runs that suite after setup succeeds.

Changes: Guardrail audit alerts

Layer / File(s) Summary
Push correlation and alert delivery
.github/workflows/guardrail-audit-alert.yaml
The workflow checks non-forced pushes for a merged pull request matching the pushed SHA, targeting main in the same repository, and merged within 60 seconds of the push event time. A match suppresses the alert. Forced pushes bypass the check, and invalid or missing timestamps produce a warning and continue to alert. Slack delivery fails unless the request succeeds and returns HTTP 200.
Workflow test coverage and CI execution
tests/test_guardrail_audit_alert.py, .github/workflows/ci.yaml
The test suite exercises merge-time boundaries, PR matching, forced pushes, error cases, HTTP status handling, and payload escaping. CI runs the suite after setup succeeds.

Priority: Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant GitHubActions
  participant GitHubPRAPI
  participant SlackWebhook
  GitHubActions->>GitHubPRAPI: Query paginated pull requests for the pushed SHA
  GitHubPRAPI-->>GitHubActions: Return merged pull request records
  alt Matching merge within 60 seconds
    GitHubActions->>GitHubActions: Exit without sending an alert
  else No qualifying merge or forced push
    GitHubActions->>SlackWebhook: Post alert payload
    SlackWebhook-->>GitHubActions: Return HTTP status
  end
Loading
Merge Risk: Minimal · up to 214ea

The long line may produce a warning but does not block CI. No actionable merge-blocking issue remains.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: detecting direct pushes by checking merged PR records in the guardrail workflow.
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 8 functions across 1 files. (2 skipped: 2 unsupported.)

Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues remain for the PR-based direct-push guardrail update.

No open issues 🎉

What changed in this PR

Updates the direct-push guardrail to distinguish merges into main from direct branch updates using GitHub’s merged pull request records rather than committer names.

Changes:

  • Matches pushed SHAs against merged PRs, supporting rebase merges while still alerting on forced updates.
  • Removes the Dependabot exemption and grants only pull request read permission.
  • Preserves safe Slack JSON construction and makes API, missing-webhook, and HTTP delivery errors fail the workflow.
File Description
.github/​workflows/​guardrail-audit-alert.yaml Adds PR-based push classification and strengthens Slack alert error handling.

Review effort: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants