Skip to content

fix(review): enforce required live UI evidence before approval #650

Description

@neubig

Actual Behavior

The reviewer approved OpenHands/OpenHands#17586 at head 9a0044b0ddb75bad7198298d59c9d125c20d0fd4 even though:

  • the PR explicitly says the after-captures are CSS-contract captures and that the full Agent Canvas stack was not booted;
  • the review itself says the test is a CSS-token contract rather than a rendered-geometry check;
  • the OpenHands repository review guide requires screenshot or video evidence from the real app for user-visible UI behavior.

The approval was review 5281107136 and was dismissed. The automatic maintainer request was removed because it depended on the false-positive approval.

Reproduce the state with:

gh api repos/OpenHands/OpenHands/pulls/17586/reviews/5281107136

Desired Behavior

When repository guidance requires live UI evidence, the reviewer must not approve a user-visible UI PR unless the evidence is from a real running application and exercises the production-facing path. Unit tests, CSS-token assertions, generated mockups, or reconstructed captures may support the review but must not substitute for the required live evidence.

The review should state exactly what live evidence is missing and use COMMENT with changes requested. It must not trigger maintainer handoff until a later review verifies compliant evidence.

Context

This is a reviewer instruction-following failure rather than an ambiguity in the OpenHands repository rule. A nearby review on OpenHands/OpenHands#17588 correctly enforced the same rule, so the durable fix should make the reviewer apply it consistently.


OpenHands AI triage

The following comments and acceptance criteria were added by the OpenHands AI agent.

Triage

This is a bug report against the reusable GitHub PR reviewer automation in this repository, not the Actions-based plugins/pr-review. The failing review (all-hands-bot, review 5281107136, dismissed) was published by skills/github-pr-reviewer/scripts/worker.py -> main._build_review_prompt(), whose workflow already requires the reviewer to read the target repository's own guidance; the target repository's AGENTS.md and .agents/skills/custom-codereview-guide.md both require a screenshot or video from the real app for user-visible UI changes and state that tests are "regression proof, not a substitute."

Bounded scope: make the reviewer's shared review prompt enforce that requirement so a user-visible UI change cannot be approved on non-live evidence, and confirm the deterministic handoff does not fire for such a review. The prompt is shared by the scheduled label scan and the reviewer-request event path in skills/github-pr-reviewer/scripts/main.py, so one change covers both. The handoff in skills/github-pr-reviewer/scripts/worker.py is already keyed off the review body's trailing verdict (✅ APPROVED or 🛑 MAINTAINER DECISION REQUIRED), so withholding approval means no maintainer request; that coupling must be preserved and covered by a test rather than reimplemented. If shipped bundle files change, automations/catalog/github-pr-reviewer/manifest.json setup.bundle.version and the entry version (and the matching template.version in tests/fixtures/automations/github-pr-reviewer.json) must be bumped with scripts/sync_extensions.py and npm run build:automations regenerated.

Non-goals: no change to the Actions-based plugins/pr-review; no new deterministic evidence checker, screenshot analyzer, or CI gate; no change to the native review format, the event: COMMENT / REQUEST_CHANGES policy already in the prompt, the scope gate, the maintainer selection in scripts/maintainer_handoff.py, or profile/secret handling; no change to trigger selection, stale-head suppression, or checkout lifecycle; no runtime, agent-server, SDK, or automation-service changes.

Acceptance Criteria

  • The reviewer's shared review prompt (used by both the scheduled label scan and the review_requested event path) requires, before approving a change that alters user-visible UI behavior, live evidence from a real running application that exercises the production-facing path, when the target repository's guidance requires that evidence.
  • When that evidence is required and the only available evidence is unit tests, CSS-token or contract assertions, generated mockups, or reconstructed captures, the review is not approved: it publishes one native review with event: COMMENT that names exactly which live evidence is missing.
  • A review that withholds approval for missing live evidence does not end with the ✅ APPROVED verdict, so the deterministic handoff in worker.py requests no maintainer; a later review that verifies compliant live evidence and ends with ✅ APPROVED performs the existing handoff unchanged.
  • A UI change that does supply compliant live evidence from the real app can still be approved through the existing flow when no other material finding remains.
  • Non-UI changes (backend, API, CLI, script) keep the existing evidence rule (the real command and observed output, with tests insufficient as sole proof).
  • Automated tests pin the requirement: a prompt-level assertion in skills/github-pr-reviewer/tests/test_main.py fails without the requirement and the existing tests in skills/github-pr-reviewer/tests/test_main.py and tests/test_github_reviewer_delivery.py still pass; uv run pytest -q passes.
  • skills/github-pr-reviewer/SKILL.md and skills/github-pr-reviewer/README.md document that live UI evidence is required before approval and that missing evidence yields a COMMENT review with no maintainer handoff.
  • The reusable extension and the generated automation bundle remain synchronized: python scripts/sync_extensions.py --check and npm run build:automations report no drift, and any changed bundle file carries a bumped automations/catalog/github-pr-reviewer/manifest.json setup.bundle.version with the matching template.version fixture.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingpriority:highHigh priority for triage against backlogready-for-devScoped for contribution; managed by repository readiness checks.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions