Skip to content

Clear open items on a reviewer-routing override instead of masking the route - #205

Open
trask wants to merge 11 commits into
open-telemetry:mainfrom
trask:pr-dashboard-override-clears-open-items
Open

Clear open items on a reviewer-routing override instead of masking the route#205
trask wants to merge 11 commits into
open-telemetry:mainfrom
trask:pr-dashboard-override-clears-open-items

Conversation

@trask

@trask trask commented Jul 30, 2026

Copy link
Copy Markdown
Member

Note

Twelve open pull requests across the org still carry the dashboard:route-overridden label. Nothing removes it once this ships, so it has to be deleted by hand after merging.

When an author comments /dashboard route:reviewers, the dashboard moved the pull request to Waiting on reviewers but left the review items that had routed it to the author untouched. The override was only lifted once routing would reach reviewers on its own, which those items prevented, so it stayed in force indefinitely and hid later feedback that genuinely needed the author. Whenever it was lifted, the same unresolved items sent the pull request straight back to the author and the author had to repeat the command.

The command now clears the review items and required check failures that were already open when it was posted, and routing continues on its own from there. Anything that arrives after the command still routes the pull request back to the author, including a reviewer saying on one of the cleared items that it was not actually handled, and the cleared items no longer appear in the status comment's remaining-items list.

The dashboard:route-overridden label is gone. The command comment is the durable record of the override, so the label no longer decides anything, and routing can no longer be flipped by editing a label by hand. Hand-applying the label to force Waiting on reviewers is replaced by the command itself, which approver-team members can already use.

Reported on open-telemetry/semantic-conventions#3860.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Updates the pull request dashboard’s /dashboard route:reviewers behavior from a route “mask” to a “watermark” that clears pre-existing author-blocking items so routing can proceed naturally and the override can self-release.

Changes:

  • Document that /dashboard route:reviewers clears pre-existing review items and required-check failures rather than only masking routing.
  • Add logic to record an override watermark timestamp and clear overridden author actions / CI failures prior to that watermark.
  • Extend unit tests to cover watermark survival, clearing behavior, and CI-routing impact.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pull-request-dashboard/README.md Updates user-facing documentation to describe clearing/watermark semantics.
.github/scripts/pull-request-dashboard/test_dashboard_override.py Adds tests for watermark timestamp, clearing behavior, and updated command replies.
.github/scripts/pull-request-dashboard/test_dashboard.py Adds routing test ensuring cleared CI failures don’t route to author.
.github/scripts/pull-request-dashboard/dashboard_override.py Implements watermark timestamp, clearing of overridden items, and revised override application logic.
.github/scripts/pull-request-dashboard/dashboard.py Wires clearing into PR result building and adjusts CI routing to respect override-cleared failures.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/scripts/pull-request-dashboard/dashboard_override.py Outdated
Comment thread .github/scripts/pull-request-dashboard/dashboard_override.py
@trask
trask requested a review from Copilot July 30, 2026 20:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

.github/scripts/pull-request-dashboard/dashboard.py:1120

  • facts.get("dashboard_override_cleared_ci") returns None when the key is absent, and not None is False, which will incorrectly treat CI failures as “cleared” if route_pr is ever called with facts that don’t include this key. Use an explicit default (e.g., facts.get("dashboard_override_cleared_ci", False)) or coerce to bool so missing data does not suppress CI failure routing.
    ci_failing = facts.get("ci_failing_count", 0) > 0 and not facts.get(
        "dashboard_override_cleared_ci"
    )
    if ci_failing and not is_maintenance_bot:

.github/scripts/pull-request-dashboard/dashboard_override.py:363

  • The comparisons use <= override_since, but the stated behavior is “already open when it was posted.” If an item/check failure shares the exact same timestamp as the command (GitHub timestamps are typically second-granularity), <= can clear things that occurred concurrently/after the command. Consider using strict < for “predate,” or introduce a deterministic tie-break (e.g., include the command comment id as part of the watermark and compare (timestamp, id)).
        if entry.get("action") == "author" and since and since <= override_since:

.github/scripts/pull-request-dashboard/dashboard_override.py:372

  • The comparisons use <= override_since, but the stated behavior is “already open when it was posted.” If an item/check failure shares the exact same timestamp as the command (GitHub timestamps are typically second-granularity), <= can clear things that occurred concurrently/after the command. Consider using strict < for “predate,” or introduce a deterministic tie-break (e.g., include the command comment id as part of the watermark and compare (timestamp, id)).
    facts["dashboard_override_cleared_ci"] = bool(
        facts.get("ci_failing_count")
        and ci_failing_since
        and ci_failing_since <= override_since

.github/scripts/pull-request-dashboard/test_dashboard.py:40

  • _author_route_facts (per its name) reads like it should describe the default “CI failing routes to author” case, but it sets dashboard_override_cleared_ci to True, which represents the exception described by the comment. To avoid confusion in future tests, either rename this helper (e.g., to indicate “override-cleared” CI) or default dashboard_override_cleared_ci to False and override it only in the tests that need the cleared behavior.
        # A failing required check routes a human-authored PR to the author
        # unless an override already cleared it.
        facts: dict[str, object] = {
            "ci_failing_count": 1,
            "dashboard_override_cleared_ci": True,
        }

@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Jul 30, 2026

Copy link
Copy Markdown

Pull request dashboard status

Waiting on reviewers · refreshed 2026-07-30 22:44 UTC

Review the latest changes.

Status above doesn't look right?
  • Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
  • Anything look wrong? Report it with what you expected; it helps us improve the dashboard.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

.github/scripts/pull-request-dashboard/dashboard_override.py:125

  • latest_authorized_command_at compares timestamps as raw strings (created_at > latest). This only behaves correctly if every timestamp is guaranteed to be the same sortable format (e.g., RFC3339/ISO8601 UTC with the same precision). To make the watermark robust and semantically correct, parse timestamps to datetimes (e.g., via the existing parse_ts) and compare those, then store the normalized string (or store seconds since epoch).
def latest_authorized_command_at(
    raw: dict[str, Any],
    author: str,
    reviewers: set[str] | None,
) -> str:
    """Timestamp of the newest authorized override command, acknowledged or not.

    Unlike `latest_authorized_command`, acknowledged commands still count: the
    watermark has to outlive the acknowledgement, or the discussions a command
    cleared would come back on the next refresh.
    """
    latest = ""
    for comment in raw.get("issue_comments") or []:
        if parse_dashboard_command(comment) != DASHBOARD_OVERRIDE_SUBCOMMAND:
            continue
        if not is_authorized_commander(
            actor_login(comment.get("user") or {}), author, reviewers
        ):
            continue
        created_at = comment.get("created_at") or ""
        if created_at > latest:
            latest = created_at
    return latest

.github/scripts/pull-request-dashboard/dashboard_override.py:374

  • clear_overridden_actions relies on string timestamp comparisons and uses <= override_since. Two concrete issues: (1) string ordering is fragile if timestamp formats/precision differ; prefer parsing via parse_ts (or comparing normalized epoch seconds). (2) <= will clear items with the exact same timestamp as the command; if the intent is strictly “already open when it was posted”, consider using < (or a more precise tie-breaker such as comment id) so same-second events created after the command aren't accidentally cleared.
def clear_overridden_actions(
    facts: dict[str, Any],
    pending_actions: dict[str, dict[str, Any]],
) -> dict[str, dict[str, Any]]:
    """Drop the author actions an override command already answered.

    `/dashboard route:reviewers` is the author saying everything open at that
    moment is handled. Clearing those actions keeps them from routing the pull
    request back to the author on every later refresh, which would make the
    author repeat the command each time review comes back around. Anything that
    happens after the command keeps its author action, so new feedback still
    reaches the author, including a reviewer reopening something the command
    cleared.
    """
    override_since = facts.get("dashboard_override_since") or ""
    facts["dashboard_override_cleared_count"] = 0
    facts["dashboard_override_cleared_ci"] = False
    if not override_since:
        return pending_actions
    remaining: dict[str, dict[str, Any]] = {}
    cleared = 0
    for discussion_id, entry in pending_actions.items():
        since = entry.get("since") or ""
        if entry.get("action") == "author" and since and since <= override_since:
            cleared += 1
            continue
        remaining[discussion_id] = entry
    ci_failing_since = facts.get("ci_failing_since") or ""
    facts["dashboard_override_cleared_count"] = cleared
    facts["dashboard_override_cleared_ci"] = bool(
        facts.get("ci_failing_count")
        and ci_failing_since
        and ci_failing_since <= override_since
    )
    return remaining

.github/scripts/pull-request-dashboard/dashboard_override.py:363

  • The boundary behavior around since == override_since is now part of the override semantics (whether same-second feedback is considered ‘already open’ or not). Add a focused unit test that covers equality (and ideally mixed precision, e.g., timestamps with/without fractional seconds) to lock in the intended behavior and prevent regressions.
        if entry.get("action") == "author" and since and since <= override_since:

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

.github/scripts/pull-request-dashboard/dashboard_override.py:374

  • ci_failing_since is the earliest timestamp across all current failures, so this flag also masks failures that arrived after the command. For example, if check A failed before the command and remains failing while check B fails afterward, the minimum timestamp still belongs to A, dashboard_override_cleared_ci stays true, and route_pr does not send the new failure to the author. Track the failing checks individually (or compute the count whose completion time is after the watermark) and route based on that remaining count.
    facts["dashboard_override_cleared_ci"] = bool(
        facts.get("ci_failing_count")
        and ci_failing_since
        and ci_failing_since <= override_since

.github/scripts/pull-request-dashboard/dashboard.py:1119

  • Masking the failure only in routing leaves ci_failing_count unchanged. render_status_comment() still reads that raw count and, after this command routes the PR to reviewers, renders the cleared failure under “Also blocked by,” so cleared required-check items remain in the status comment contrary to the PR’s stated behavior. Expose an effective uncleared failure count and use it consistently for both routing and status rendering.
    ci_failing = facts.get("ci_failing_count", 0) > 0 and not facts.get(
        "dashboard_override_cleared_ci"
    )

.github/scripts/pull-request-dashboard/dashboard_override.py:120

  • This re-evaluates historical commands against the current approver-team membership on every refresh. If an approver posts an accepted command and is later removed from the team, the command is no longer considered authorized, dashboard_override_since disappears, and all previously cleared actions return even though the acknowledged command comment remains. Treat the app’s acknowledgement marker as durable proof that a historical command was authorized, while continuing to validate unacknowledged commands against current membership.
        if not is_authorized_commander(
            actor_login(comment.get("user") or {}), author, reviewers
        ):

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

.github/scripts/pull-request-dashboard/dashboard_override.py:290

  • A command can clear the last author action when approval_count already meets the threshold, causing route_pr() to return maintainer. This branch then tells the user the PR was routed to reviewers even though it went directly to maintainers. Handle the maintainer route explicitly so the acknowledgement reports the actual destination.
        elif route == "copilot":
            message = (
                "accepted the reviewer-routing override; the reviewer handoff "
                "is waiting on Copilot."
            )
        else:
            message = "routed this pull request to reviewers."

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

.github/scripts/pull-request-dashboard/dashboard.py:1388

  • This changes both the persisted dashboard-result shape/meaning and how delivery interprets it, but DASHBOARD_STATE_VERSION is still 6. The compatibility contract in state.py:20-26 requires that version to increase for exactly this kind of change; otherwise overlapping old workflow runs can accept these results as compatible and apply the previous override behavior. Please bump the dashboard state version with this change.
        pending_actions = clear_overridden_actions(facts, pending_actions)

.github/scripts/pull-request-dashboard/pr_status_comment.py:224

  • The status-comment rendering now suppresses failures cleared by the command, but STATUS_COMMENT_REVISION remains unchanged. Per state.py:37-39, renderer behavior changes need a revision bump so existing comments are queued for rollout; without it, comments not otherwise queued can retain the old blocked-by text. Please increment the status-comment revision.
    failing_count = uncleared_ci_failing_count(facts)

@trask
trask requested a review from Copilot July 30, 2026 22:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

.github/scripts/pull-request-dashboard/dashboard_override.py:400

  • This treats required-check failures as an all-or-nothing group. If check A failed before the command and check B fails afterward, dashboard_override_cleared_ci becomes false and uncleared_ci_failing_count() returns both failures; the status comment reports two blockers and fallback_wait_ts() uses A's pre-command timestamp even though A was supposed to stay cleared. Track/count individual failures relative to the watermark (and retain the earliest uncleared timestamp) so only B contributes to routing, status, and wait age.
    # Keyed off the newest failure so a check that fails after the command still
    # routes to the author, even while an older cleared failure persists.
    facts["dashboard_override_cleared_ci"] = bool(
        facts.get("ci_failing_count")
        and ci_failing_latest
        and ci_failing_latest <= override_since
    )

.github/scripts/pull-request-dashboard/delivery.py:15

  • Removing the override delivery path leaves existing workflow-owned labels stranded. The reported semantic-conventions PR #3860 still carries dashboard:route-overridden, and there is now no reference that removes that label from PRs or deletes it from repositories, so rollout will leave misleading labels indefinitely despite the PR saying the label is gone. Keep a one-time/idempotent cleanup delivery (or provide an explicit repository-label migration) before retiring this stage.
from dashboard_override import deliver_dashboard_command_replies

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

pull-request-dashboard/README.md:190

  • This description does not match the implementation: clear_overridden_actions() clears only author-owned review actions, and after clearing, route_pr() may send an already-approved PR directly to maintainers (as the new maintainer acknowledgement test expects). Describe this as leaving the author route and continuing through natural routing rather than guaranteeing Waiting on reviewers.
can comment `/dashboard route:reviewers`. The command clears every review item
and required check failure that was already open when it was posted, so the
pull request routes to *Waiting on reviewers* and stays there instead of falling
back to the author on the next refresh. Members of the repository's
`approver_teams` can use the same

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

.github/scripts/pull-request-dashboard/dashboard_override.py:388

  • This inclusive comparison can discard feedback posted after the command when both GitHub events share the same timestamp (the API timestamps and existing fixtures are second-granularity). That violates the core guarantee that anything arriving after the command still routes to the author. Treat equal timestamps as uncleared; this conservatively leaves an ambiguous same-second pre-command item open instead of masking potentially new feedback.
        if entry.get("action") == "author" and since and since <= override_since:

.github/scripts/pull-request-dashboard/dashboard.py:582

  • A required check can finish after the command within the same timestamp tick. Because equality is currently considered cleared, that new failure is hidden from routing and the status comment even though the PR promises all post-command failures remain actionable. Keep equal timestamps uncleared to avoid masking an ambiguously ordered failure.
            if override_since is None or ts > override_since

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

@trask
trask marked this pull request as ready for review July 30, 2026 22:43
@trask
trask requested a review from a team as a code owner July 30, 2026 22:43
@trask
trask requested a review from adrielp July 30, 2026 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants