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.

  • Alert workflow: Detects merges into this repository’s main by matching the push SHA against merged-PR records and correlating merged_at with the repository push timestamp within 60 seconds. Forced pushes do not receive the merge exemption. Missing or invalid timestamps produce a warning and send an alert.
  • Delivery and permissions: Grants pull-requests: read. Checks webhook configuration before merge detection, safely builds the Slack JSON payload with jq, and fails on API errors, curl errors, or any Slack status other than HTTP 200.
  • CI and tests: Adds a guardrail-test job that runs the workflow’s shell logic tests after setup succeeds, without a pull-request change filter. The tests cover timestamp boundaries, PR identity and pagination, force pushes, failure paths, payload escaping, and event context. Test execution results were not provided.

@coderabbitai

coderabbitai Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Walkthrough

The guardrail workflow now checks merged pull request records before suppressing push alerts. It sends Slack alerts for unmatched pushes and fails when delivery does not return HTTP 200. The CI workflow runs tests for the guardrail script.

Changes: Guardrail audit alerts

Layer / File(s) Summary
Push context and merged PR matching
.github/workflows/guardrail-audit-alert.yaml, tests/test_guardrail_audit_alert.py
The workflow checks non-forced pushes for a merged PR matching the push SHA, main branch, repository, and 60-second timestamp window. Tests cover matching, edge cases, and lookup failures.
Slack delivery and CI validation
.github/workflows/guardrail-audit-alert.yaml, tests/test_guardrail_audit_alert.py, .github/workflows/ci.yaml
The script requires HTTP 200 from Slack. Tests check delivery and payload handling, and CI runs the guardrail test suite.

Priority: Normal

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

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant PushEvent
  participant GuardrailWorkflow
  participant GitHubCLI
  participant Slack
  PushEvent->>GuardrailWorkflow: Provide push SHA, force flag, and repository timestamp
  GuardrailWorkflow->>GitHubCLI: Look up merged PR records for non-forced pushes
  GitHubCLI-->>GuardrailWorkflow: Return PR records
  GuardrailWorkflow->>Slack: Send alert when no qualifying PR matches
Loading
Merge Risk: Minimal · up to c56ca

No confirmed issue blocks merging after normal checks. Live GitHub lookup and Slack delivery have not been exercised.

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 names the main change: the guardrail alert now detects direct pushes by matching pushed SHAs against merged PR records. The raw summary and objectives confirm this is the core change in guar…
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: Title check

Explanation

The title names the main change: the guardrail alert now detects direct pushes by matching pushed SHAs against merged PR records. The raw summary and objectives confirm this is the core change in guardrail-audit-alert.yaml, with the supporting CI job and tests. The conventional-commit prefix 'fix(guardrail):' is specific and not vague. Length is 60 characters, which is acceptable.

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

Choose a reason for hiding this comment

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

🟢 Approval recommended

The main-branch guardrail update has no active review issues.

No open issues 🎉

What changed in this PR

Updates the repository’s main-branch guardrail to distinguish PR merges from direct pushes using GitHub’s merged-PR records rather than committer names.

Changes:

  • Recognizes PR merges, including rebase merges, by matching the pushed SHA.
  • Treats forced updates as alert-worthy and removes the Dependabot exemption.
  • Adds PR-read permission, preserves safe Slack JSON encoding, and fails on API, missing-webhook, or delivery errors.
File Description
.github/​workflows/​guardrail-audit-alert.yaml Replaces committer-based detection with PR-record checks 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