ci: add .pr/ artifact convention to extensions - #545
Conversation
Add the PR Artifacts workflow and document the .pr/ convention in AGENTS.md, matching the convention already present in OpenHands, software-agent-sdk, and automation. Also remove five orphaned custom-automation screenshots that were committed directly to main instead of living in a PR's temporary .pr/ directory. Co-authored-by: openhands <openhands@all-hands.dev>
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
|
@OpenHands understand why CI PR description fails and fix it please. |
|
I'm on it! enyst can track my progress at all-hands.dev |
|
Fixed. The current PR Description Check was failing for two concrete reasons:
I created and linked #546, confirmed it received The refreshed PR Description Check now passes, and all 8 PR checks are green. No source change or additional commit was needed because the failure was entirely in PR metadata. This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
Resolved on PR #545.
No code changes, commit, or push were needed because the failure was entirely in PR metadata. |
enyst
left a comment
There was a problem hiding this comment.
🔴 Needs improvement
[CRITICAL ISSUES]
- The PAT-backed approval cleanup lacks an authorization check for the reviewer. See the inline finding.
[IMPROVEMENT OPPORTUNITIES]
- The advertised manual trigger cannot execute any job. See the inline finding.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This adds a security-sensitive workflow with repository write access and a bot PAT. Fork code is correctly kept out of privileged execution, but the approval-to-write authorization boundary must be explicit in the workflow rather than assumed from repository settings.
VERDICT:
❌ Needs rework: Do not enable the privileged cleanup path until it verifies that the approving reviewer has repository write permission.
KEY INSIGHT:
Copying an established workflow also copies its trust assumptions; privileged automation must enforce those assumptions itself.
This review was created by an AI agent (OpenHands) on behalf of the user.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it is merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
| if: >- | ||
| github.event_name == 'pull_request_review' && | ||
| github.event.review.state == 'approved' && | ||
| github.event.pull_request.head.repo.full_name == github.repository |
There was a problem hiding this comment.
🔴 Critical - authorization: This lets any submitted approved review reach the PAT-backed push. GitHub documents that, by default, any user can submit an approving review in a public repository; whether that approval counts for branch protection is separate. The same-repository head check does not validate the reviewer. Before exposing OPENHANDS_BOT_GITHUB_PAT_PUBLIC, query the reviewers permission and require write/maintain/admin access (or enforce an equivalent trusted association in the job), then bind checkout/push to the validated PR head/ref. Otherwise an outsider can make the bot delete .pr/, create a commit, and trigger CI under its identity.
| name: PR Artifacts | ||
|
|
||
| on: | ||
| workflow_dispatch: # Manual trigger for testing |
There was a problem hiding this comment.
🟡 Suggestion - dead trigger: This says the dispatch is for manual testing, but every job-level if requires either pull_request_review or pull_request_target, so a manual dispatch runs zero jobs. Either remove this trigger/comment or add a safe dispatch-specific validation job/inputs that actually exercise the intended path.
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
… main Co-authored-by: openhands <openhands@all-hands.dev>
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
HUMAN:
This PR proposes adding the same temporary
.pr/workflow used by the other OpenHands repositories. No manual approval or merge test was run because that would require disposable same-repository and fork PRs; the existing CI suite passed, and reviewer verification steps are below.Why
The
.pr/PR-artifacts convention is established acrossOpenHands,software-agent-sdk, andautomation, but is missing fromextensions. This adds the workflow and documentation so the convention is consistent across all four repositories.Summary
.github/workflows/pr-artifacts.ymlwith parity tosoftware-agent-sdk: comment when.pr/exists, remove it from same-repository PR branches after approval, and remove it frommainafter merging fork PRs..pr/convention inAGENTS.md.custom-automation-*.pngscreenshots that were committed to.pr/onmain.Issue Number
Fixes #546
How to Test
.github/workflows/pr-artifacts.ymland verifypull_request_targetonly reads fork contents through the GitHub API; it never checks out or executes fork code with a write token..pr/notes.md, verify the workflow posts one notice and removes.pr/after approval..pr/notes.md, verify the workflow posts one notice and removes.pr/from the base branch after merge.Video/Screenshots
Not applicable - this is a CI workflow and repository documentation change.
Notes
The
.pr/notice is informational and does not block merging.This PR description was updated by an AI agent (OpenHands) on behalf of the user.