Skip to content

Commit d318511

Browse files
authored
refactor: align reviewer board attention parity (#566)
* refactor: align reviewer board attention parity * fix: narrow reviewer board repair attention * fix: retain fail-closed reviewer evidence * fix: scope reviewer board repair evidence * fix: support legacy deferred comment evidence * fix: harden deferred payload contracts * fix: require review comment head evidence * fix: harden deferred review comment diagnostics * fix: gate deferred parse diagnostics * fix: centralize deferred identity recovery * fix: validate deferred source object keys
1 parent 0934126 commit d318511

24 files changed

Lines changed: 2235 additions & 81 deletions

.github/workflows/reviewer-bot-pr-review-comment-observer.yml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ jobs:
2626
COMMENT_AUTHOR: ${{ github.event.comment.user.login }}
2727
COMMENT_AUTHOR_ID: ${{ github.event.comment.user.id }}
2828
COMMENT_USER_TYPE: ${{ github.event.comment.user.type }}
29+
COMMENT_COMMIT_ID: ${{ github.event.comment.commit_id }}
2930
COMMENT_SENDER_TYPE: ${{ github.event.sender.type }}
3031
COMMENT_INSTALLATION_ID: ${{ github.event.installation.id }}
3132
COMMENT_PERFORMED_VIA_GITHUB_APP: ${{ github.event.comment.performed_via_github_app.id > 0 && 'true' || 'false' }}
@@ -42,6 +43,7 @@ jobs:
4243
'source_workflow_file': '.github/workflows/reviewer-bot-pr-review-comment-observer.yml',
4344
'source_run_id': int(os.environ['GITHUB_RUN_ID']),
4445
'source_run_attempt': int(os.environ['GITHUB_RUN_ATTEMPT']),
46+
'source_artifact_name': f"reviewer-bot-review-comment-context-{os.environ['GITHUB_RUN_ID']}-attempt-{os.environ['GITHUB_RUN_ATTEMPT']}",
4547
'source_event_name': 'pull_request_review_comment',
4648
'source_event_action': 'created',
4749
'source_event_key': f"pull_request_review_comment:{os.environ['COMMENT_ID']}",
@@ -52,6 +54,7 @@ jobs:
5254
'comment_author': os.environ['COMMENT_AUTHOR'],
5355
'comment_author_id': int(os.environ['COMMENT_AUTHOR_ID']),
5456
'comment_user_type': os.environ['COMMENT_USER_TYPE'],
57+
'source_commit_id': os.environ.get('COMMENT_COMMIT_ID', '').strip() or None,
5558
'comment_sender_type': os.environ['COMMENT_SENDER_TYPE'],
5659
'comment_installation_id': os.environ.get('COMMENT_INSTALLATION_ID', '').strip() or None,
5760
'comment_performed_via_github_app': os.environ['COMMENT_PERFORMED_VIA_GITHUB_APP'] == 'true',

.github/workflows/reviewer-bot-sweeper-repair.yml

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,6 @@ jobs:
7070
WORKFLOW_RUN_ID: ${{ github.run_id }}
7171
WORKFLOW_NAME: ${{ github.workflow }}
7272
WORKFLOW_JOB_NAME: ${{ github.job }}
73-
REVIEWER_BOARD_ENABLED: ${{ github.event_name == 'workflow_dispatch' && github.event.inputs.action == 'preview-reviewer-board' && 'true' || 'false' }}
74-
REVIEWER_BOARD_TOKEN: ${{ github.event_name == 'workflow_dispatch' && github.event.inputs.action == 'preview-reviewer-board' && secrets.REVIEWER_BOARD_TOKEN || '' }}
7573
run: uv run --project "$BOT_SRC_ROOT" reviewer-bot
7674
- name: Workflow summary
7775
run: |

scripts/reviewer_bot_core/reviewer_response_policy.py

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,25 @@
2020

2121
_UNSET = object()
2222
_ASSIGNMENT_GUIDANCE_AUTHORS = {"github-actions", "github-actions[bot]", "guidelines-bot"}
23+
_FAIL_CLOSED_REVIEWER_ACTIVITY_GAP_REASONS = frozenset(
24+
{
25+
"observer_failed",
26+
"observer_cancelled",
27+
"observer_run_missing",
28+
"observer_state_unknown",
29+
"artifact_missing",
30+
"artifact_invalid",
31+
"artifact_expired",
32+
"reconcile_failed_closed",
33+
}
34+
)
35+
_REVIEWER_ACTIVITY_GAP_KINDS = frozenset(
36+
{
37+
"issue_comment:created",
38+
"pull_request_review:submitted",
39+
"pull_request_review_comment:created",
40+
}
41+
)
2342

2443

2544
def _record_timestamp(record: dict | None, *, parse_timestamp) -> datetime | None:
@@ -139,6 +158,102 @@ def _build_current_scope_key(
139158
)
140159

141160

161+
def _gap_source_kind(gap: dict) -> str | None:
162+
source_event_kind = gap.get("source_event_kind")
163+
if isinstance(source_event_kind, str) and source_event_kind.strip():
164+
return source_event_kind
165+
source_event_key = gap.get("source_event_key")
166+
if not isinstance(source_event_key, str):
167+
return None
168+
if source_event_key.startswith("issue_comment:"):
169+
return "issue_comment:created"
170+
if source_event_key.startswith("pull_request_review_comment:"):
171+
return "pull_request_review_comment:created"
172+
if source_event_key.startswith("pull_request_review:"):
173+
return "pull_request_review:submitted"
174+
return None
175+
176+
177+
def _gap_event_timestamp(gap: dict):
178+
return live_review_support.parse_github_timestamp(gap.get("source_event_created_at"))
179+
180+
181+
def _visible_review_commit_id(gap: dict) -> str | None:
182+
diagnostic = gap.get("visible_review_diagnostic")
183+
payload = diagnostic.get("payload") if isinstance(diagnostic, dict) else None
184+
commit_id = payload.get("commit_id") if isinstance(payload, dict) else None
185+
if isinstance(commit_id, str) and commit_id.strip():
186+
return commit_id
187+
source_commit_id = gap.get("source_commit_id")
188+
if isinstance(source_commit_id, str) and source_commit_id.strip():
189+
return source_commit_id
190+
comment = gap.get("comment")
191+
comment_commit_id = None
192+
if isinstance(comment, dict):
193+
comment_commit_id = comment.get("commit_id") or comment.get("original_commit_id")
194+
return comment_commit_id if isinstance(comment_commit_id, str) and comment_commit_id.strip() else None
195+
196+
197+
def _visible_activity_author(gap: dict) -> str | None:
198+
diagnostic = gap.get("visible_review_diagnostic")
199+
payload = diagnostic.get("payload") if isinstance(diagnostic, dict) else None
200+
author = payload.get("author") if isinstance(payload, dict) else None
201+
if isinstance(author, str) and author.strip():
202+
return author
203+
source_actor = gap.get("source_actor_login")
204+
if isinstance(source_actor, str) and source_actor.strip():
205+
return source_actor
206+
actor = gap.get("actor")
207+
if isinstance(actor, str) and actor.strip():
208+
return actor
209+
comment = gap.get("comment")
210+
user = comment.get("user") if isinstance(comment, dict) else None
211+
login = user.get("login") if isinstance(user, dict) else None
212+
return login if isinstance(login, str) and login.strip() else None
213+
214+
215+
def has_fail_closed_reviewer_activity_for_current_scope(
216+
review_data: dict,
217+
fail_closed_gaps,
218+
*,
219+
anchor_timestamp: str | None = None,
220+
current_head_sha: str | None = None,
221+
) -> bool:
222+
current_reviewer = review_data.get("current_reviewer")
223+
if not isinstance(current_reviewer, str) or not current_reviewer.strip():
224+
return False
225+
floor = live_review_support.parse_github_timestamp(anchor_timestamp)
226+
if floor is None:
227+
_, cycle_boundary = _initial_cycle_boundary(review_data)
228+
floor = live_review_support.parse_github_timestamp(cycle_boundary)
229+
for gap in fail_closed_gaps:
230+
if not isinstance(gap, dict):
231+
continue
232+
if gap.get("operator_action_required") is not True:
233+
continue
234+
if gap.get("reason") not in _FAIL_CLOSED_REVIEWER_ACTIVITY_GAP_REASONS:
235+
continue
236+
source_kind = _gap_source_kind(gap)
237+
if source_kind not in _REVIEWER_ACTIVITY_GAP_KINDS:
238+
continue
239+
event_timestamp = _gap_event_timestamp(gap)
240+
if event_timestamp is None:
241+
continue
242+
if floor is not None and event_timestamp < floor:
243+
continue
244+
author = _visible_activity_author(gap)
245+
if not isinstance(author, str) or author.lower() != current_reviewer.lower():
246+
continue
247+
if source_kind in {"pull_request_review:submitted", "pull_request_review_comment:created"}:
248+
if not isinstance(current_head_sha, str) or not current_head_sha.strip():
249+
continue
250+
commit_id = _visible_review_commit_id(gap)
251+
if commit_id != current_head_sha:
252+
continue
253+
return True
254+
return False
255+
256+
142257
def _current_scope_fields(
143258
review_data: dict,
144259
current_reviewer: str | None,
@@ -478,6 +593,7 @@ def derive_reviewer_response_state(
478593
reason="no_reviewer_activity",
479594
scope_fields=_current_scope_fields(review_data, current_reviewer, current_head, None),
480595
anchor_timestamp=_initial_reviewer_anchor(review_data),
596+
current_head_sha=current_head,
481597
reviewer_comment=reviewer_comment,
482598
reviewer_review=reviewer_review,
483599
current_cycle_reviewer_handoff=reviewer_handoff,

scripts/reviewer_bot_lib/deferred_gap_bookkeeping.py

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,11 +235,49 @@ def _payload_or_existing(payload: dict, existing: dict, key: str):
235235
return existing.get(key) if value is None else value
236236

237237

238+
def _first_present_payload_value(payload: dict, keys: tuple[str, ...]):
239+
for key in keys:
240+
if key not in payload:
241+
continue
242+
value = payload.get(key)
243+
if isinstance(value, str):
244+
value = value.strip()
245+
if value is not None and value != "":
246+
return value
247+
return None
248+
249+
250+
def _copy_source_evidence(fields: dict, payload: dict, existing: dict) -> None:
251+
evidence_fields = {
252+
"source_actor_login": ("source_actor_login", "comment_author", "actor_login"),
253+
"source_actor_id": ("source_actor_id", "comment_author_id", "actor_id"),
254+
"source_actor_user_type": ("source_actor_user_type", "comment_user_type"),
255+
"source_actor_sender_type": ("source_actor_sender_type", "comment_sender_type"),
256+
"source_actor_installation_id": ("source_actor_installation_id", "comment_installation_id"),
257+
"source_actor_performed_via_github_app": (
258+
"source_actor_performed_via_github_app",
259+
"comment_performed_via_github_app",
260+
),
261+
"source_comment_id": ("source_comment_id", "comment_id"),
262+
"source_review_id": ("source_review_id", "review_id", "pull_request_review_id"),
263+
"source_commit_id": ("source_commit_id",),
264+
"source_review_state": ("source_review_state",),
265+
}
266+
for target_key, source_keys in evidence_fields.items():
267+
value = _first_present_payload_value(payload, source_keys)
268+
if value is None:
269+
value = existing.get(target_key)
270+
if value is not None:
271+
fields[target_key] = value
272+
273+
238274
def _source_event_created_at(payload: dict, existing: dict):
239275
return (
240276
payload.get("source_created_at")
277+
or payload.get("comment_created_at")
241278
or payload.get("source_submitted_at")
242279
or payload.get("source_dismissed_at")
280+
or payload.get("source_event_created_at")
243281
or existing.get("source_event_created_at")
244282
)
245283

@@ -280,6 +318,7 @@ def record_deferred_gap_diagnostic(
280318
source_dismissed_at = _payload_or_existing(payload, existing, "source_dismissed_at")
281319
if source_dismissed_at is not None:
282320
fields["source_dismissed_at"] = source_dismissed_at
321+
_copy_source_evidence(fields, payload, existing)
283322
existing.update(fields)
284323
changed = previous != existing
285324
deferred_gaps[source_event_key] = existing

scripts/reviewer_bot_lib/event_inputs.py

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -446,9 +446,15 @@ def build_pull_request_sync_request(bot: EventInputsContext) -> PullRequestSyncR
446446
)
447447

448448

449-
def build_replay_comment_event_request(payload, *, live_comment=None, comment_body: str | None = None) -> CommentEventRequest:
450-
if getattr(getattr(payload, "identity", None), "schema_version", None) != 3:
451-
raise InvalidEventInput("build_replay_comment_event_request", ("schema-v3 comment-like payload required",))
449+
def build_replay_comment_event_request(
450+
payload,
451+
*,
452+
live_comment=None,
453+
live_pr=None,
454+
comment_body: str | None = None,
455+
) -> CommentEventRequest:
456+
if not hasattr(payload, "identity") or not hasattr(payload, "comment_id"):
457+
raise InvalidEventInput("build_replay_comment_event_request", ("typed deferred comment payload required",))
452458
if not hasattr(payload, "issue_state") or not hasattr(payload, "issue_author") or not hasattr(payload, "issue_labels"):
453459
raise InvalidEventInput("build_replay_comment_event_request", ("typed deferred comment payload required",))
454460
resolved_body = payload.comment_body if comment_body is None else comment_body
@@ -467,12 +473,18 @@ def build_replay_comment_event_request(payload, *, live_comment=None, comment_bo
467473
if live_comment is not None and live_comment.comment_performed_via_github_app_available
468474
else payload.comment_performed_via_github_app
469475
)
476+
issue_author = payload.issue_author
477+
if live_pr is not None and not issue_author.strip():
478+
issue_author = live_pr.issue_author
479+
issue_labels = payload.issue_labels
480+
if live_pr is not None and not issue_labels:
481+
issue_labels = live_pr.issue_labels
470482
return CommentEventRequest(
471483
issue_number=payload.identity.pr_number,
472484
is_pull_request=True,
473485
issue_state=payload.issue_state,
474-
issue_author=payload.issue_author,
475-
issue_labels=payload.issue_labels,
486+
issue_author=issue_author,
487+
issue_labels=issue_labels,
476488
comment_id=payload.comment_id,
477489
comment_author=(live_comment.comment_author if live_comment is not None else payload.comment_author),
478490
comment_author_id=payload.comment_author_id,

scripts/reviewer_bot_lib/project_board.py

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@
66
from dataclasses import asdict, dataclass
77
from typing import Any
88

9+
from scripts.reviewer_bot_core import live_review_support, reviewer_response_policy
10+
11+
from . import deferred_gap_bookkeeping as gap_bookkeeping
912
from .config import (
1013
REVIEWER_BOARD_ENABLED_ENV,
1114
REVIEWER_BOARD_FIELD_ASSIGNED_AT,
@@ -92,6 +95,9 @@ class ReviewStateDerivation:
9295
state: str
9396
anchor_timestamp: str | None
9497
reason: str | None
98+
current_scope_key: str | None
99+
current_scope_basis: str | None
100+
current_head_sha: str | None
95101

96102

97103
@dataclass(frozen=True)
@@ -262,6 +268,21 @@ def _format_date(value: Any) -> str | None:
262268
return value[:10]
263269

264270

271+
def _deferred_gap_records(review_data: dict[str, Any]) -> tuple[dict, ...]:
272+
return tuple(
273+
gap_bookkeeping.get_deferred_gap(review_data, source_event_key)
274+
for source_event_key in gap_bookkeeping.list_deferred_gap_keys(review_data)
275+
)
276+
277+
278+
def _timestamp_at_or_after_anchor(value: Any, anchor: str | None) -> bool:
279+
timestamp = live_review_support.parse_github_timestamp(value)
280+
anchor_timestamp = live_review_support.parse_github_timestamp(anchor)
281+
if timestamp is None or anchor_timestamp is None:
282+
return False
283+
return timestamp >= anchor_timestamp
284+
285+
265286
def _derive_review_state(
266287
bot: ProjectBoardProjectionContext,
267288
issue_number: int,
@@ -279,6 +300,9 @@ def _derive_review_state(
279300
state=state,
280301
anchor_timestamp=derived.get("anchor_timestamp") if isinstance(derived.get("anchor_timestamp"), str) else None,
281302
reason=derived.get("reason") if isinstance(derived.get("reason"), str) else None,
303+
current_scope_key=derived.get("current_scope_key") if isinstance(derived.get("current_scope_key"), str) else None,
304+
current_scope_basis=derived.get("current_scope_basis") if isinstance(derived.get("current_scope_basis"), str) else None,
305+
current_head_sha=derived.get("current_head_sha") if isinstance(derived.get("current_head_sha"), str) else None,
282306
)
283307

284308

@@ -355,13 +379,21 @@ def derive_board_projection(input: BoardProjectionInput) -> BoardProjectionValue
355379

356380
needs_attention = REVIEWER_BOARD_OPTION_ATTENTION_NO
357381
repair_needed = load_repair_marker(review_data, "status_label_projection")
358-
if isinstance(repair_needed, dict) and repair_needed.get("kind") == "projection_failure":
382+
fail_closed_reviewer_activity = reviewer_response_policy.has_fail_closed_reviewer_activity_for_current_scope(
383+
review_data,
384+
_deferred_gap_records(review_data),
385+
anchor_timestamp=derivation.anchor_timestamp,
386+
current_head_sha=derivation.current_head_sha,
387+
)
388+
if (isinstance(repair_needed, dict) and repair_needed.get("kind") == "projection_failure") or fail_closed_reviewer_activity:
359389
needs_attention = REVIEWER_BOARD_OPTION_ATTENTION_PROJECTION_REPAIR_REQUIRED
360390
elif review_data.get("mandatory_approver_required"):
361391
needs_attention = REVIEWER_BOARD_OPTION_ATTENTION_TRIAGE_APPROVAL_REQUIRED
362-
elif review_data.get("transition_notice_sent_at"):
392+
elif derivation.state != "awaiting_reviewer_response":
393+
needs_attention = REVIEWER_BOARD_OPTION_ATTENTION_NO
394+
elif _timestamp_at_or_after_anchor(review_data.get("transition_notice_sent_at"), derivation.anchor_timestamp):
363395
needs_attention = REVIEWER_BOARD_OPTION_ATTENTION_TRANSITION_NOTICE_SENT
364-
elif review_data.get("transition_warning_sent"):
396+
elif _timestamp_at_or_after_anchor(review_data.get("transition_warning_sent"), derivation.anchor_timestamp):
365397
needs_attention = REVIEWER_BOARD_OPTION_ATTENTION_WARNING_SENT
366398

367399
return BoardProjectionValues(

0 commit comments

Comments
 (0)