Skip to content

Commit 24c8fc9

Browse files
committed
fix: harden reviewer bot guardrails
1 parent bd49c6f commit 24c8fc9

11 files changed

Lines changed: 481 additions & 6 deletions

File tree

.github/actions/reviewer-bot-source/action.yml

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,32 @@ runs:
1010
- id: source-root
1111
shell: bash
1212
run: |
13+
workspace_root="$(git -C "$GITHUB_WORKSPACE" rev-parse --show-toplevel)"
14+
if [ "$workspace_root" != "$GITHUB_WORKSPACE" ]; then
15+
echo "reviewer-bot source checkout root does not match GITHUB_WORKSPACE" >&2
16+
exit 1
17+
fi
18+
checked_out_sha="$(git -C "$GITHUB_WORKSPACE" rev-parse HEAD)"
19+
if [ -n "${GITHUB_SHA:-}" ] && [ "$checked_out_sha" != "$GITHUB_SHA" ]; then
20+
echo "reviewer-bot source checkout does not match GITHUB_SHA" >&2
21+
exit 1
22+
fi
23+
remote_url="$(git -C "$GITHUB_WORKSPACE" config --get remote.origin.url || true)"
24+
case "$remote_url" in
25+
*"$GITHUB_REPOSITORY"*) ;;
26+
*)
27+
echo "reviewer-bot source checkout remote does not match GITHUB_REPOSITORY" >&2
28+
exit 1
29+
;;
30+
esac
1331
if [ ! -f "$GITHUB_WORKSPACE/scripts/reviewer_bot.py" ]; then
1432
echo "reviewer-bot source checkout is missing scripts/reviewer_bot.py" >&2
1533
exit 1
1634
fi
35+
bot_entry="$(realpath "$GITHUB_WORKSPACE/scripts/reviewer_bot.py")"
36+
if [ "$bot_entry" != "$GITHUB_WORKSPACE/scripts/reviewer_bot.py" ]; then
37+
echo "reviewer-bot entrypoint must be a regular file inside the trusted checkout" >&2
38+
exit 1
39+
fi
1740
printf 'bot-src-root=%s\n' "$GITHUB_WORKSPACE" >> "$GITHUB_OUTPUT"
1841
printf 'BOT_SRC_ROOT=%s\n' "$GITHUB_WORKSPACE" >> "$GITHUB_ENV"

scripts/reviewer_bot_lib/app.py

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -461,6 +461,11 @@ def execute_run(bot: AppExecutionRuntime, context: EventContext) -> ExecutionRes
461461
if manual_projection_policy is not None
462462
else True
463463
)
464+
allow_status_label_sync = (
465+
manual_projection_policy.allow_status_label_sync
466+
if manual_projection_policy is not None
467+
else True
468+
)
464469
if (
465470
lock_required
466471
and event_name in {"schedule", "workflow_dispatch"}
@@ -560,7 +565,16 @@ def execute_run(bot: AppExecutionRuntime, context: EventContext) -> ExecutionRes
560565

561566
maintenance.emit_pending_issue314_state_health_repair_summary(bot)
562567

563-
if touched_items:
568+
if touched_items and not allow_status_label_sync:
569+
_log(
570+
bot,
571+
"info",
572+
"Skipping status-label sync because manual dispatch policy is read-only.",
573+
manual_action=context.manual_action or "",
574+
touched_items=touched_items,
575+
)
576+
577+
if touched_items and allow_status_label_sync:
564578
if not lock_acquired:
565579
raise RuntimeError(
566580
"Status-label projection reached apply path without lease lock. "

scripts/reviewer_bot_lib/maintenance_schedule.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
)
1212
from .overdue import (
1313
backfill_transition_notice_if_present,
14+
backfill_transition_warning_if_present,
1415
check_overdue_reviews,
1516
handle_overdue_review_warning,
1617
)
@@ -139,6 +140,16 @@ def _run_tracked_pr_maintenance(bot, state: dict) -> tuple[bool, list[int]]:
139140

140141
def _run_overdue_pass(bot, state: dict) -> bool:
141142
changed = False
143+
active_reviews = state.get("active_reviews")
144+
if isinstance(active_reviews, dict):
145+
for issue_key, review_data in active_reviews.items():
146+
if not isinstance(review_data, dict):
147+
continue
148+
try:
149+
issue_number = int(issue_key)
150+
except ValueError:
151+
continue
152+
changed = backfill_transition_warning_if_present(bot, state, issue_number) or changed
142153
overdue_reviews = check_overdue_reviews(bot, state)
143154
for review in overdue_reviews:
144155
issue_number = review["issue_number"]

scripts/reviewer_bot_lib/overdue.py

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -729,6 +729,70 @@ def _effective_response_with_cadence(bot, issue_number: int, review_data: dict,
729729
return effective_response, cadence, reminder_scan
730730

731731

732+
def backfill_transition_warning_if_present(bot, state: dict, issue_number: int) -> bool:
733+
active_reviews = state.get("active_reviews") if isinstance(state, dict) else None
734+
review_data = active_reviews.get(str(issue_number)) if isinstance(active_reviews, dict) else None
735+
if not isinstance(review_data, dict):
736+
return False
737+
if not isinstance(review_data.get("current_reviewer"), str) or not review_data["current_reviewer"].strip():
738+
return False
739+
existing_warning = review_data.get("transition_warning_sent")
740+
if isinstance(existing_warning, str) and existing_warning.strip():
741+
return False
742+
try:
743+
issue_snapshot_result = bot.github.get_issue_or_pr_snapshot_result(issue_number)
744+
except (AssertionError, AttributeError, RuntimeError):
745+
return False
746+
issue_snapshot = issue_snapshot_result.payload if issue_snapshot_result.ok else None
747+
if not isinstance(issue_snapshot, dict):
748+
return False
749+
if str(issue_snapshot.get("state", "")).lower() == "closed":
750+
return False
751+
752+
response_state = bot.adapters.review_state.compute_reviewer_response_state(
753+
issue_number,
754+
review_data,
755+
issue_snapshot=issue_snapshot,
756+
)
757+
_effective_response, cadence, _reminder_scan = _effective_response_with_cadence(
758+
bot,
759+
issue_number,
760+
review_data,
761+
response_state,
762+
)
763+
receipt = cadence.warning_receipt
764+
if receipt is None or receipt.receipt_kind != "warning":
765+
return False
766+
if receipt.source not in {"comment_scan", "live_comment_scan"}:
767+
return False
768+
if not isinstance(receipt.created_at, str) or not receipt.created_at.strip():
769+
return False
770+
771+
changed = False
772+
if review_data.get("transition_warning_sent") != receipt.created_at:
773+
review_data["transition_warning_sent"] = receipt.created_at
774+
changed = True
775+
delivery_result = build_reminder_delivery_persistence_result(
776+
issue_number=issue_number,
777+
reviewer=receipt.reviewer,
778+
head_sha=receipt.head_sha,
779+
cycle_key=receipt.cycle_key,
780+
scope_key=receipt.scope_key,
781+
receipt_kind="warning",
782+
comment_posted=False,
783+
comment_id=receipt.comment_id,
784+
comment_created_at=receipt.created_at,
785+
state_save_attempted=False,
786+
state_save_succeeded=False,
787+
recovered_receipt=receipt,
788+
diagnostic_reason="live_warning_receipt_backfill",
789+
)
790+
changed = _store_reminder_delivery_result(review_data, delivery_result) or changed
791+
if changed:
792+
bot.collect_touched_item(issue_number)
793+
return changed
794+
795+
732796
def _find_existing_warning_comment(
733797
bot,
734798
issue_number: int,

scripts/reviewer_bot_lib/reconcile.py

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,28 @@ def build_command_replay_receipt(
252252
clear_gap_allowed: bool,
253253
diagnostic_reason: str | None = None,
254254
) -> CommandReplayReceipt:
255-
if not replay_attempted:
255+
normalized_side_effects = tuple(
256+
sorted(
257+
{
258+
str(item).strip()
259+
for item in command_side_effects_attempted
260+
if str(item).strip()
261+
}
262+
)
263+
)
264+
normalized_command_name = command_name.strip() if isinstance(command_name, str) and command_name.strip() else None
265+
consistency_error = None
266+
if replay_attempted and normalized_command_name is None:
267+
consistency_error = "missing_command_name_for_replay"
268+
elif not replay_attempted and normalized_side_effects:
269+
consistency_error = "side_effects_without_replay"
270+
elif not replay_attempted and state_save_required:
271+
consistency_error = "state_save_required_without_replay"
272+
273+
if consistency_error is not None:
274+
result = "blocked_inconsistent_replay_receipt"
275+
diagnostic_reason = diagnostic_reason or consistency_error
276+
elif not replay_attempted:
256277
result = "pass_diagnostic_only"
257278
elif not mark_reconciled_allowed or not clear_gap_allowed:
258279
result = "blocked_authority_missing"
@@ -263,9 +284,9 @@ def build_command_replay_receipt(
263284
return CommandReplayReceipt(
264285
source_event_key=source_event_key,
265286
issue_number=issue_number,
266-
command_name=command_name,
287+
command_name=normalized_command_name,
267288
replay_attempted=replay_attempted,
268-
command_side_effects_attempted=command_side_effects_attempted,
289+
command_side_effects_attempted=normalized_side_effects,
269290
state_save_required=state_save_required,
270291
state_save_succeeded=state_save_succeeded,
271292
mark_reconciled_allowed=mark_reconciled_allowed,

tests/contract/reviewer_bot/test_workflow_files.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -423,6 +423,18 @@ def test_reviewer_bot_workflows_use_shared_source_action_without_raw_extraction(
423423
assert "uses: ./.github/actions/reviewer-bot-source" in text
424424
assert "BOT_SRC_ROOT: ${{ steps.bot-source.outputs.bot-src-root }}" in text
425425

426+
427+
def test_reviewer_bot_source_action_validates_checkout_provenance():
428+
action = Path(".github/actions/reviewer-bot-source/action.yml").read_text(encoding="utf-8")
429+
430+
assert 'git -C "$GITHUB_WORKSPACE" rev-parse --show-toplevel' in action
431+
assert 'git -C "$GITHUB_WORKSPACE" rev-parse HEAD' in action
432+
assert '"$checked_out_sha" != "$GITHUB_SHA"' in action
433+
assert 'git -C "$GITHUB_WORKSPACE" config --get remote.origin.url' in action
434+
assert '*"$GITHUB_REPOSITORY"*' in action
435+
assert 'realpath "$GITHUB_WORKSPACE/scripts/reviewer_bot.py"' in action
436+
assert "printf 'BOT_SRC_ROOT=%s\\n'" in action
437+
426438
def test_workflow_summaries_and_runbook_references_exist():
427439
runbook = Path("docs/reviewer-bot-review-freshness-operator-runbook.md")
428440
assert runbook.exists()

tests/integration/reviewer_bot/test_app_execution.py

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -445,6 +445,30 @@ def fake_save_state(state):
445445
assert saved_states[-1]["active_reviews"]["42"]["sidecars"]["repair_markers"]["status_label_projection"]["kind"] == "projection_failure"
446446

447447

448+
def test_execute_run_manual_read_only_policy_blocks_touched_status_label_sync(monkeypatch):
449+
harness = AppHarness(monkeypatch)
450+
harness.set_event(
451+
EVENT_NAME="workflow_dispatch",
452+
EVENT_ACTION="",
453+
MANUAL_ACTION="preview-status-label-projection",
454+
ISSUE_NUMBER=42,
455+
VALIDATION_NONCE="nonce",
456+
)
457+
harness.stub_load_state(lambda *, fail_on_unavailable=False: make_state())
458+
459+
def fake_manual_dispatch(state):
460+
harness.runtime.collect_touched_item(42)
461+
return False
462+
463+
harness.stub_handler("handle_manual_dispatch", fake_manual_dispatch)
464+
harness.stub_sync_status_labels(lambda current, issue_numbers: pytest.fail("read-only manual policy must not sync labels"))
465+
466+
result = harness.run_execute()
467+
468+
assert result.exit_code == 0
469+
assert result.state_changed is False
470+
471+
448472
def test_execute_run_returns_failure_when_lock_release_fails(monkeypatch):
449473
harness = AppHarness(monkeypatch)
450474
harness.set_event(EVENT_NAME="issue_comment", EVENT_ACTION="created")

0 commit comments

Comments
 (0)