fix: estc-pr-buildkite-detective bot allowlist, noop safe-output, and PR discovery#634
fix: estc-pr-buildkite-detective bot allowlist, noop safe-output, and PR discovery#634
Conversation
…afe-output - Add buildkite-limited-access[bot] to the bots allowlist - Extract PR numbers from check_run events, branches and target URL from status events - Use .commit.sha instead of .sha for status events - Update Step 1 to follow the PR Actions Detective approach for PR discovery - Add noop safe-output (prompt already referenced it) - Add Target URL, PR Numbers, and Branches to the context section Co-authored-by: strawgate <6384545+strawgate@users.noreply.github.com>
Per code review feedback: .sha and .commit.sha should be equivalent in GitHub status events per the docs. Keep .sha for safety. Co-authored-by: strawgate <6384545+strawgate@users.noreply.github.com>
| @@ -99,10 +101,16 @@ steps: | |||
| echo "BK_EVENT_ID=$(jq -r '.id' "$GITHUB_EVENT_PATH")" >> "$GITHUB_ENV" | |||
| echo "BK_FAILURE_STATE=$(jq -r '.state' "$GITHUB_EVENT_PATH")" >> "$GITHUB_ENV" | |||
| echo "BK_COMMIT_SHA=$(jq -r '.sha' "$GITHUB_EVENT_PATH")" >> "$GITHUB_ENV" | |||
There was a problem hiding this comment.
This is not correct, the .sha points to the workflow reference
It should point to the branch from the PR.
For instance, see #632 (comment)
There was a problem hiding this comment.
https://docs.github.com/en/actions/reference/workflows-and-actions/contexts
github.sha | string | The commit SHA that triggered the workflow. The value of this commit SHA depends on the event that triggered the workflow. For more information, see Events that trigger workflows. For example, ffac537e6cbbf934b08745a378932722df287a53.
-- | -- | --
github.sha string The commit SHA that triggered the workflow. The value of this commit SHA depends on the event that triggered the workflow. For more information, see [Events that trigger workflows](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows). For example, ffac537e6cbbf934b08745a378932722df287a53.
There was a problem hiding this comment.
Fixed in 24933f3. For status events, BK_COMMIT_SHA now extracts the PR branch commit from .branches[] by filtering out the default branch (read from .repository.default_branch), falling back to .sha only when no non-default branch is present.
|
@copilot please address feedback |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds Buildkite context variables (BK_BRANCHES, BK_PR_NUMBERS, BK_TARGET_URL) and propagates them through prompt rendering and placeholder substitution; prefers explicit PR numbers when present, otherwise resolves PRs via GitHub commit- and branch-based queries and noops with a standard message if none found. Appends buildkite-limited-access[bot] to allowed-bots, adds a Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Requesting changes: PR association in Step 1 still needs candidate validation to avoid selecting unrelated historical PRs from commits/{sha}/pulls.
What is this? | From workflow: Trigger PR Review
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| 3. For each matching PR, keep author, branches, and fork status for downstream analysis. | ||
| 2. Find the associated pull request(s): | ||
| - If **PR Numbers** in the Context section above is non-empty (e.g., from `check_run` events), use those PR numbers directly with `pull_request_read` method `get`. | ||
| - Otherwise, use `bash` + `gh api repos/${{ github.repository }}/commits/{commit_sha}/pulls` to find PRs containing the commit SHA. If no results, also try searching open PRs whose head branch matches one of the **Branches** listed in the Context section. |
There was a problem hiding this comment.
gh api repos/.../commits/{commit_sha}/pulls can return closed/historical PR associations, so treating any result as the target PR may mis-route analysis to the wrong PR for status events.
In the exact failure mode this PR is fixing (status SHA not equal to current PR head), this step can still stop on an unrelated merged PR and never reach branch-based open-PR matching. Please require candidate validation (e.g., open state and/or head.ref match against BK_BRANCHES) before accepting commit-associated PRs, and fall back when none validate.
…r status events Co-authored-by: strawgate <6384545+strawgate@users.noreply.github.com>
|
@copilot please address feedback |
Co-authored-by: strawgate <6384545+strawgate@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/gh-aw-estc-pr-buildkite-detective.md:
- Around line 103-106: The jq expressions that set BK_BRANCHES and the later PR
extraction read arrays that may be null; update the echo lines that build
BK_BRANCHES and BK_PR_NUMBERS to defensively extract arrays using the optional
iterator and a default empty array (e.g., use .branches[]? and/or wrap with //
[]), and do the same for any `.pull_requests` or similar array extractions
reading from GITHUB_EVENT_PATH so iterations over BK_BRANCHES and BK_PR_NUMBERS
never break on null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: faeef151-0896-4f83-9e4c-a9cc53b3a5da
📒 Files selected for processing (2)
.github/workflows/gh-aw-estc-pr-buildkite-detective.lock.yml.github/workflows/gh-aw-estc-pr-buildkite-detective.md
|
/ai please address feedback |
|
@copilot please address feedback |
Co-authored-by: strawgate <6384545+strawgate@users.noreply.github.com>
Addressed all review feedback in 9deb89a:
|
| 3. For each matching PR, keep author, branches, and fork status for downstream analysis. | ||
| 2. Find the associated pull request(s): | ||
| - If **PR Numbers** in the Context section above is non-empty (e.g., from `check_run` events), use those PR numbers directly with `pull_request_read` method `get`. | ||
| - Otherwise, use `bash` + `gh api repos/${{ github.repository }}/commits/{commit_sha}/pulls` to find PRs containing the commit SHA. Filter the results to keep only PRs whose `state` is `"open"` and, when **Branches** is available, whose `head.ref` matches one of the listed branches. If no candidates remain, also try searching open PRs whose head branch matches one of the **Branches** listed in the Context section. |
There was a problem hiding this comment.
The branch-only fallback can mis-associate unrelated PRs when multiple open PRs share the same head.ref (common with forks or generic branch names like main/fix-ci).
In this path, candidates are matched only by branch name from BK_BRANCHES, which doesn’t include repo owner/context. That can route analysis/comments to the wrong PR.
Please add a disambiguation requirement in the fallback (for example: match head.repo.full_name and/or verify candidate head.sha/status target_url context before accepting).
This PR fixes three issues in the
estc-pr-buildkite-detectiveworkflow where PR association and safe output handling could fail.What changed
buildkite-limited-access[bot]alongside the configurableallowed-bot-usersinput.noopsafe-output: Declarednoopundersafe-outputsso the agent can emit it when no PR can be resolved.statusevents, switched to.commit.sharather than.sha.check_run:BK_PR_NUMBERSfrom.check_run.pull_requests[].number,BK_TARGET_URLfrom.check_run.details_urlstatus:BK_BRANCHESfrom.branches[].name,BK_TARGET_URLfrom.target_url// []fallback for.branchesand.check_run.pull_requestsarrays to prevent errors when these fields are null.[]when no pull requests are present, so downstream "non-empty" checks are unambiguous.check_run) and otherwise fall back togh api repos/{repo}/commits/{sha}/pullsplus branch matching (status), instead of listing all open PRs and matching onlyhead.sha. Results fromcommits/{sha}/pullsare now filtered to keep only PRs withstate == "open"and matchinghead.ref, preventing misrouting to closed or historical PRs.Also includes the corresponding compiled lockfile updates.
The body of this PR is automatically managed by the Trigger Update PR Body workflow.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.