https://docs.all-hands.dev/ logo
Join Slack
Powered by
:wave: OpenHands/OpenHands#16324 needs a human rev...
# proj-agent-canvas
f
👋 OpenHands/OpenHands#16324 needs a human review: fix(automations): surface non-MCP-installable integrations instead of silently dropping them by @lokesh75-kank This contributor's PR needs a human review, so OpenHands skipped the automatic AI review.
❌ 1
l
@fresh-machine-37269: this PR does not have “a human has tested these changes” checked, and it also has no screenshot demonstrating that the frontend is working, despite the fact that it touches frontend code. I would like to open a PR that modifies the CI checks to (1) detect if frontend code was touched based on modifications to frontend repos and requires a screenshot or video to be included in the PR description if so, and (2) require that if the “a human has tested these changes” box is present, require it to be checked. Test the modified CI script on previous PRs and make sure that it is working properly. When you are done, send a PR updating the workflow and reply to me on this thread with confirmation that it’s working on recent PRs.
👀 1
f
🤖 On it! View progress here: oss-agent-canvas.ngrok.dev/conversations/…
Done â I opened https://github.com/OpenHands/OpenHands/pull/16325, which extends the PR Description Check workflow with the two rules you asked for: 1. Frontend screenshot/video requirement â if a PR touches frontend code (detected via 
src/
, 
__tests__/
, 
public/
path prefixes and `.tsx`/`.jsx`/`.css`/`.scss`/`.vue`/`.svelte` extensions + tailwind/vite/postcss config), the description must include a screenshot or video. The body is scanned for markdown images, `<img>`/`<video>` tags, GitHub user-attachment URLs, direct video-file links, and known video hosts (YouTube/Loom/Vimeo/asciinema). 2. Human-tested checkbox â if the âA human has tested these changesâ box is present, it must be checked (
- [x]
). An unchecked box fails; an absent box is not required. I tested the modified script against 10 recent PRs (bodies + changed files pulled via the GitHub API): ⢠#16324 (the trigger PR) â correctly fails on both the unchecked box and the missing screenshot. ⢠Frontend PRs #16178, #16311, #16319, #16322 â flagged for missing screenshot. ⢠#16305 â its screenshot + mp4 are correctly detected (no false screenshot error). ⢠Non-frontend PRs #16316 (docs) and #16321 (chore) â pass. ⢠Bot PRs (#16273, #16232) are exempt in CI via the existing 
user.type != 'Bot'
filter. Synthetic edge cases (checked box + each media format; unchecked box; no checkbox on frontend vs non-frontend) all behave correctly. The PR itself passes its own check. This reply was posted by an AI agent (OpenHands) on your behalf.
✅ Done! Done! I opened PR github.com/OpenHands/OpenHands/pull/16325 and replied to you on the Slack thread. What the PR does: Extends the existing 
PR Description Check
workflow (
.github/scripts/check_pr_description.py
+ 
.github/workflows/pr-description-check.yml
) with two new enforcement rules: 1. Frontend screenshot/video requirement — if a PR touches frontend code, the description must include a screenshot or video. Frontend code is detected by path prefix (
src/
, 
__tests__/
, 
public/
) and file extension (
.tsx
, 
.jsx
, 
.css
, 
.scss
, 
.vue
, 
.svelte
) plus frontend config globs (
tailwind.config.*
, 
vite.config.*
, 
postcss.config.*
). The body is scanned for markdown images, ``/`<video>` tags, GitHub 
user-attachments/assets/
URLs, direct video-file links (`.mp4`/`.webm`/`.mov`...), and known video hosts (YouTube, Loom, Vimeo, asciinema, streamable). 2. Human-tested checkbox — if the "A human has tested these changes" checkbox is present in the PR body, it must be checked (
- [x]
). An unchecked box fails; an absent box is not required (the rule only fires when the contributor included the line). Testing on previous PRs — validated against 10 recent PRs (bodies + changed files fetched via the GitHub API): • PR #16324 (the trigger PR) → correctly fails on both the unchecked box and the missing screenshot. • Frontend PRs #16178, #16311, #16319, #16322 → flagged for missing screenshot. • PR #16305 → its screenshot + mp4 are correctly detected (no false error). • Non-frontend PRs #16316 (docs) and #16321 (chore) → pass. • Bot PRs (#16273, #16232) are exempt in CI via the existing 
user.type != 'Bot'
filter. Synthetic edge cases (checked box + each media format; unchecked box; no checkbox on frontend vs non-frontend) all behave correctly, and the PR itself passes its own check.
l
@fresh-machine-37269 Wait, I merged that and merged into 16324 but I don’t see the CI failing.
👀 1
f
🤖 On it! View progress here: oss-agent-canvas.ngrok.dev/conversations/…
✅ Done! Found it. I pulled the workflow file 
.github/workflows/pr-description-check.yml
from 
main
. The trigger is:
Copy code
on:
  pull_request_target:
    types: [opened, edited, reopened, ready_for_review]
The list is missing 
synchronize
. When you merged 
main
into #16324's branch, GitHub fired a 
pull_request
synchronize
event (head SHA changed) — and this workflow ignores that event type, so it never ran. That's why there's no failing check: the 
pr-description-check
job simply didn't trigger. (I confirmed #16324's current check rollup has Docker, CI, Mock-LLM E2E, and 
pr-title
, but no 
Validate PR description
run at all.) Two things to note on top of that: 1. Even the events it does listen for won't catch this case — merging code into the branch is 
synchronize
, and you didn't edit the PR description (which would be 
edited
). 2. Since it uses 
pull_request_target
, it checks out the base branch version of the script (
github.event.pull_request.base.sha
), so the script from #16325 is the one that runs — that part is fine. Fix: add 
synchronize
to the types so the check re-runs whenever the PR head updates:
Copy code
on:
  pull_request_target:
    types: [opened, edited, reopened, ready_for_review, synchronize]
After pushing that change to 
main
, re-trigger #16324 (push an empty commit or close/reopen) and the 
Validate PR description
check should run — and fail, since #16324 touches frontend 
.tsx
files with no screenshot and has an unchecked "A human has tested these changes" box. Want me to open a PR adding 
synchronize
to the trigger?
l
@fresh-machine-37269 can you manually trigger the ones I synchronized then?
👀 1
f
✅ Done! I confirmed the validator itself works â running 
check_pr_description.py
against #16324's body + changed files produces the expected 2 failures:
â The 
A human has tested these changes
checkbox is present but unchecked.
â This PR touches frontend code but the description has no screenshot or video.
So the check would fail â it just never ran, because 
synchronize
isn't in the workflow's trigger list. Now, about manually triggering it: the workflow has no 
workflow_dispatch
trigger, and it ignores 
synchronize
, so there's no "Run workflow" button and an empty commit won't fire it either. The only events it listens for are 
opened
, 
edited
, 
reopened
, 
ready_for_review
. The least-intrusive way to fire one of those on an already-open PR is close â reopen (which emits 
reopened
). It's fully reversible and a standard maintainer action. Before I do that to #16324 (which is authored by 
lokesh75-kank
, not you), I want to flag it explicitly since it'll notify the author. Two options: 1. I close + reopen #16324 now â fires 
reopened
â the 
Validate PR description
check runs and should fail with the 2 errors above. (I can also do the same for any other PRs you "synchronized" â just point me at them.) 2. You'd rather not touch the PR state â then the clean fix is to add 
synchronize
(and optionally 
workflow_dispatch
) to the trigger on `main`; after that, any push re-runs it automatically. I can open that PR. Which would you like? If you say go, I'll close/reopen #16324 (and any others you list) right away and report back the run result.
l
@fresh-machine-37269 Could we open a new PR to allow for workflow_dispatch triggers on that action?
👀 1
f
✅ Done! Done! PR opened: github.com/OpenHands/OpenHands/pull/16326 It does two things: 1. Adds 
workflow_dispatch
 with a 
pr_number
input — once merged, you can go to the "PR Description Check" workflow in the Actions UI, hit "Run workflow", enter a PR number, and it'll re-validate that PR on demand (no close/reopen needed). 2. Adds 
synchronize
 to the 
pull_request_target
types — this fixes the actual root cause from earlier, so the check will also auto-run whenever a PR's head is updated (new commit or merge from main). I also reworked the job so both trigger paths fetch the PR body + changed files via the API and pass them to the script with `--body-file`/`--files-file`, since a 
workflow_dispatch
run has no 
pull_request
event payload. YAML validates clean, and I confirmed locally that running the script against #16324 still produces the expected 2 failures.
l
@fresh-machine-37269 will that action failing cause CI to be blocked?
👀 1
f
✅ Done! Short answer: No â not unless you explicitly add it as a required status check, which it isn't right now. Here's what I confirmed on #16324: • Its 
mergeStateStatus
is 
BLOCKED
, but that's because of 
reviewDecision=REVIEW_REQUIRED
(no reviewer has approved) â not because of any status check. • All existing CI checks are either 
SUCCESS
or 
SKIPPED
. There's no "Validate PR description" check in the rollup at all (it never ran, since 
synchronize
wasn't triggered). • The repo has no branch-protection required status checks that I could read (the token lacks admin scope to view branch protection rules directly, but the evidence is clear: #16324 is blocked only on review, not on checks). So the check name is "Validate PR description". Right now a failing run would just show as a red â on the PR â informational, not blocking. It would only block merges if someone adds "Validate PR description" to `main`'s required status checks in branch protection. One important caveat about the 
workflow_dispatch
trigger: manually-dispatched runs don't count as PR checks. They run standalone on a branch and won't appear on the PR's checks list or affect the merge button. Only the automatic 
pull_request_target
run (from `synchronize`/`opened`/etc.) registers as a PR check. So 
workflow_dispatch
is useful for ad-hoc validation/verification, but to actually enforce it on merges you'd rely on the automatic trigger + adding it to required checks. If you do want it to block, I'd suggest: merge #16326, confirm the automatic 
synchronize
run shows up on a PR, then add "Validate PR description" to main's required status checks. Want me to look into whether there's anything else needed for that?
l
@fresh-machine-37269 If you push an empty commit to those PRs, would that trigger the PR description check? If so, do so. If not, maybe you could edit the PR description and add a space at the end or something so that it’s a very minimal change to trigger the description check.
👀 1
f
✅ Done! It worked. Editing the PR description triggered the 
edited
event, and the PR Description Check workflow ran and failed on #16324, exactly as expected:
Validate PR description | COMPLETED | FAILURE
It's now showing as a red X in #16324's checks (run #30957293156), alongside the other CI checks. So you can now see the failing check you were missing. Recap of what I did: • Confirmed an empty commit wouldn't trigger it (no 
synchronize
in the current trigger list), so I made a minimal edit to #16324's description (appended a non-breaking space) to fire the 
edited
event. • The 
pull_request_target
workflow ran against the base-branch script, fetched #16324's body + changed files, and failed with the 2 expected errors (unchecked "A human has tested these changes" box + missing frontend screenshot). Note: this failing check does not block the merge on its own (it's not a required status check) — it's informational, as we discussed. And once #16326 merges (adding 
synchronize
), future pushes/merges into a PR will trigger the check automatically without needing a description edit.
l
@fresh-machine-37269 send me the links to the relevant PRs
👀 1
f
🤖 On it! View progress here: oss-agent-canvas.ngrok.dev/conversations/…
✅ Done! Here are the relevant PRs from that thread: • #16325 â ci: require frontend screenshot/video and checked human-tested box github.com/OpenHands/OpenHands/pull/16325 • #16326 â ci: allow manual workflow_dispatch and synchronize on PR description check github.com/OpenHands/OpenHands/pull/16326 Both are currently closed/merged.
Open in Slack
PreviousNext
Powered by