Clear open items on a reviewer-routing override instead of masking the route - #205
Clear open items on a reviewer-routing override instead of masking the route#205trask wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
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:reviewersclears 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.
There was a problem hiding this comment.
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")returnsNonewhen the key is absent, andnot NoneisFalse, which will incorrectly treat CI failures as “cleared” ifroute_pris 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 setsdashboard_override_cleared_citoTrue, 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 defaultdashboard_override_cleared_citoFalseand 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,
}
Pull request dashboard statusWaiting on reviewers · refreshed 2026-07-30 22:44 UTC Review the latest changes. Status above doesn't look right?
|
There was a problem hiding this comment.
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_atcompares 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 existingparse_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_actionsrelies on string timestamp comparisons and uses<= override_since. Two concrete issues: (1) string ordering is fragile if timestamp formats/precision differ; prefer parsing viaparse_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_sinceis 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:
There was a problem hiding this comment.
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_sinceis 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_cistays true, androute_prdoes 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_countunchanged.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_sincedisappears, 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
):
…ailure and share the uncleared count
There was a problem hiding this comment.
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_countalready meets the threshold, causingroute_pr()to returnmaintainer. This branch then tells the user the PR was routed to reviewers even though it went directly to maintainers. Handle themaintainerroute 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."
… in the command reply
There was a problem hiding this comment.
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_VERSIONis still6. The compatibility contract instate.py:20-26requires 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_REVISIONremains unchanged. Perstate.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)
…n and status comment revision
There was a problem hiding this comment.
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_cibecomes false anduncleared_ci_failing_count()returns both failures; the status comment reports two blockers andfallback_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
… that predate the override command
There was a problem hiding this comment.
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
…ming routing, not pinning to reviewers
There was a problem hiding this comment.
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
Note
Twelve open pull requests across the org still carry the
dashboard:route-overriddenlabel. 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-overriddenlabel 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.