Skip to content

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

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: Detects merge pushes by matching GitHub’s merged-PR records against the pushed SHA, repository, main base branch, and push event timestamp. Accepts epoch or UTC ISO timestamps within a 60-second window.
  • Alert behavior: Forced pushes bypass merge lookup and alert. Missing or invalid timestamps warn and alert. The Slack webhook must be configured, and delivery must return HTTP 200.
  • CI and tests: Adds a CI job that runs the guardrail test suite from the checked-out repository. Tests cover merge matching, timestamp handling, forced pushes, lookup failures, webhook responses, payload escaping, and event context.
  • Limitation: A rewind and restoration within 60 seconds of the original merge can still match the merge record and suppress an alert. 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 for recent merged pull requests before alerting on non-forced pushes. It validates Slack delivery, and CI runs tests for the workflow’s matching, error handling, and payload behavior.

Changes: Guardrail audit alerts

Layer / File(s) Summary
Push classification and merge lookup
.github/workflows/guardrail-audit-alert.yaml, tests/test_guardrail_audit_alert.py
The workflow checks non-forced pushes for a matching merge into main within 60 seconds of the repository push timestamp. Tests cover matching criteria, pagination, timestamp formats, forced pushes, and lookup errors.
Slack delivery and CI tests
.github/workflows/guardrail-audit-alert.yaml, tests/test_guardrail_audit_alert.py, .github/workflows/ci.yaml
The workflow builds a JSON payload and requires a successful Slack request with HTTP status 200. Tests check payload escaping and delivery outcomes. CI runs the guardrail test suite.

Priority: Normal

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

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Push as GitHub push event
  participant Workflow as Guardrail workflow
  participant API as GitHub Pull Requests API
  participant Slack
  Push->>Workflow: Provide push SHA, forced flag, and repository timestamp
  alt Non-forced push with valid timestamp
    Workflow->>API: Search pull requests for a matching merge
    API-->>Workflow: Return pull-request records
    alt Matching merge is within 60 seconds
      Note over Workflow: Exit without sending an alert
    else No matching recent merge
      Workflow->>Slack: Send JSON alert payload
      Slack-->>Workflow: Return HTTP status
    end
  else Forced push or invalid timestamp
    Workflow->>Slack: Send JSON alert payload
    Slack-->>Workflow: Return HTTP status
  end
Loading
Merge Risk: Minimal · up to a8aa7

The guardrail now detects merges through GitHub's merged-PR records and checks that Slack delivery returns HTTP 200. Tests run the real script against stubs. No concrete merge-blocking defect was established; live Slack delivery has 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 now detects direct pushes by matching merged PR records instead of committer names. The changeset matches this. The guardrail workflow now checks merged …
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 now detects direct pushes by matching merged PR records instead of committer names. The changeset matches this. The guardrail workflow now checks merged PRs against push timestamps, and a new test suite covers the behavior. The title is short, uses a conventional-commit prefix, and is specific enough for a teammate to scan history.

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: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
📝 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 were identified for the updated direct-push alert workflow.

No open issues 🎉

What changed in this PR

Updates the direct-push alert workflow to recognize merges into main through GitHub’s merged-PR records, including rebase merges, rather than committer names.

Changes:

  • Matches pushed commits against merged PRs while always 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 Replaces committer-based detection with PR-record checks and strengthens Slack delivery error handling.

Review effort: Balanced

@greenc-FNAL

Copy link
Copy Markdown
Contributor Author

The guardrail update is published and its actionlint, YAML, and dedicated guardrail-test jobs pass. The remaining CodeQL failure predates this update: init is pinned to 4.39.1 while analyze remains at 4.38.2, producing "Loaded a configuration file for version 4.39.1, but running version 4.38.2". The original PR-head CI already failed (https://github.com/Framework-R-D/action-detect-relevant-changes/actions/runs/40935298766). The matching analyze dependency update is already open as #51. Those existing pins were intentionally left unchanged in this guardrail-only correction.

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