Skip to content

fix: Align /hold permission with /verified and /wip status-label gate #1164

Closed

Description

@myakove

Summary

Align /hold so PR authors can set/clear hold without being OWNERS approvers, while keeping the gate narrower than /verified//wip because hold is merge-blocking: author OR OWNERS pull-request approvers only.

Intentional permission model

/hold is merge-blocking, so its gate is narrower than /verified and /wip:

  1. PR author (parent_committer), OR
  2. OWNERS pull-request approvers

Rationale: external authors must be able to hold their own PR (the original pain vs approvers-only), but collaborators/contributors/reviewers who are not approvers must NOT be able to block merges via /hold.

/verified and /wip keep _can_manage_pr_status_label (author OR is_user_valid_to_run_commands).
/hold uses dedicated _can_manage_hold_label (author OR approvers).

Problem / Motivation

/hold is currently approvers-only in webhook_server/libs/handlers/issue_comment_handler.py (checks all_pull_request_approvers and posts "only approvers can mark pull request with hold").

/verified and /wip already use _can_manage_pr_status_label which allows:

  1. PR author (parent_committer)
  2. OR users who pass is_user_valid_to_run_commands

External PR authors cannot /hold their own PR without being an OWNERS approver. We need author access for /hold (as in #1155 / #1156 for verified/wip), but with a dedicated narrower gate because hold blocks merges.

Root cause

Hold branch (around lines 464-479 in issue_comment_handler.py) uses a separate approver-only check instead of _can_manage_hold_label (author OR approvers).

Requirements

  1. Replace the approver check + custom comment with _can_manage_hold_label (author or approvers)
  2. Update the helper docstring for _can_manage_hold_label / related hold helpers
  3. Update tests: author can hold; unrelated blocked; non-approver collaborators/contributors denied; approver still works

Deliverables

  • Code changes in issue_comment_handler.py
  • Unit tests covering author allow, deny, and approver paths
  • Documentation / docstring updates as needed

Out of scope

  • Merge-blocker semantics of the hold label
  • Draft-PR allowlist
  • Other commands

Done

  • /hold and /hold cancel use _can_manage_hold_label (author or approvers; not the shared _can_manage_pr_status_label gate)
  • PR author can set/clear hold without being an OWNERS approver (covered by unit tests; manual verify still open)
  • Unrelated commenters still denied (unit tests)
  • OWNERS pull-request approvers still can set/clear hold (unit tests)
  • Unit tests cover author allow, deny, and approver paths
  • Full verify passes (ruff, mypy, pytest) — ruff/format pass; pytest all green; coverage 89.68% is pre-existing on main

Activity

  1. myakove commented on Jul 21, 2026

    @myakove
    CollaboratorAuthor

    Progress update

    Implementation is in #1165.

    Done (checked on issue body)

    • /hold and /hold cancel use _can_manage_pr_status_label
    • PR author can set/clear hold without being OWNERS approver (unit tests; manual verify still open)
    • Unrelated commenters still denied (unit tests)
    • Approvers / allowed command users still can (unit tests)
    • Unit tests cover author allow, deny, and approver paths
    • Full verify — ruff/format pass; pytest all green; coverage 89.68% is pre-existing on main

    Issue left open — manual test plan items remain on the PR.

  2. myakove commented on Jul 21, 2026

    @myakove
    CollaboratorAuthor

    Clarifies the intentional permission model for /hold in response to the Qodo security finding on PR #1165: expanded hold access (author + is_user_valid_to_run_commands) is an accepted product decision, not a regression to open access for random commenters.

  3. myakove commented on Jul 21, 2026

    @myakove
    CollaboratorAuthor

    Refined the intentional permission model based on the Qodo security review on PR #1165: /hold stays narrower than /verified//wip (author OR OWNERS approvers via _can_manage_hold_label), so non-approver collaborators/contributors/reviewers cannot block merges.

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

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions