Skip to content

feat: emit PR review completion events#90

Closed
Pigbibi wants to merge 2 commits into
mainfrom
codex/review-event-bridge-p0-20260717
Closed

feat: emit PR review completion events#90
Pigbibi wants to merge 2 commits into
mainfrom
codex/review-event-bridge-p0-20260717

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Summary

  • emit one sanitized PR-review completion event from the shared Codex review workflow through GitHub OIDC
  • validate repository/run/workflow provenance, persist bounded idempotency state, and send Telegram notification without affecting automation failure history
  • allow the authenticated event route through the existing Cloudflare proxy and configure a persistent optional Telegram environment file

Safety boundaries

  • observation/notification only; no closure, merge, rerun, deployment, or live execution action
  • no review body, token, secret, account ID, or repository-specific configuration in the event
  • workflow notification failure is continue-on-error and cannot override the Codex Review result
  • activation still requires separately approved Cloudflare Worker/VPS deployment and root-only /etc/codex-audit-bridge/telegram.env provisioning

Verification

  • python3 -m pytest -q — 662 passed, 1 skipped, 64 subtests passed
  • node --test cloudflare/codex-audit-proxy/tests/index.test.mjs — 14 passed
  • python3 -m compileall -q scripts service tests
  • actionlint .github/workflows/codex_pr_review.yml .github/workflows/vps_codex_service_ops.yml
  • bash -n scripts/deploy_codex_audit_service.sh
  • shellcheck --severity=error scripts/deploy_codex_audit_service.sh
  • YAML parse and git diff --check

Known bounded behavior

Notification delivery is at-least-once. A process crash after Telegram accepts a message but before the sent state is persisted can produce a duplicate; the existing heartbeat remains the reconciliation fallback.

Co-Authored-By: Codex <noreply@openai.com>
@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown

🤖 Codex PR Review

🚫 Merge blocked: 2 serious issue(s) found in high-risk files

🚫 Blocking Issues

These issues must be fixed before this PR can be merged:

1. 🟠 [HIGH] Logic in scripts/emit_pr_review_event.py

The deduplication key only includes repository, pr_number, head_sha, and GITHUB_RUN_ID. GitHub reruns keep the same run_id and only increment GITHUB_RUN_ATTEMPT, so a rerun with a different final outcome will be treated as an already-sent duplicate and its notification will be dropped. That can leave operators with a stale failure/success alert that no longer matches the latest workflow attempt. (line 101)

Suggestion: Include GITHUB_RUN_ATTEMPT in the emitted metadata and in run_id, and validate/persist it on the service side so each rerun attempt is tracked independently.

2. 🟠 [HIGH] Logic in .github/workflows/codex_pr_review.yml

The new event step runs under if: always(), and the emitter accepts skipped as a valid review outcome. If an earlier guard/setup step fails before Run Codex PR Review executes at all (for example fork rejection, bridge checkout failure, or Python setup failure), the workflow can still emit a pr_review_completed event with review_outcome=skipped, producing a false completion notification for a review that never happened. (line 158)

Suggestion: Only emit the event when the review step actually ran, or reject skipped for this event type. A simple fix is to gate the step on a non-empty/non-skipped steps.review.outcome or require the review decision artifact to exist before emitting.

ℹ️ Other Findings

1. 🟡 [MEDIUM] Reliability in scripts/emit_pr_review_event.py

The emitter treats any notification status other than sent or deduplicated as a hard delivery failure, but the service intentionally returns skipped when telegram.env is not configured. Because the PR explicitly makes that environment file optional, every review run against an unprovisioned service will retry three times and end with a warning even though the route handled the event as designed. (line 175)

Suggestion: Treat notification.status == "skipped" as a successful no-op when Telegram is intentionally unconfigured, or suppress emission earlier when notification delivery is disabled.

2. 🟡 [MEDIUM] Reliability in service/review_event_notification.py

dispatch_review_event_notification() records only a single boolean result from send_telegram_alert(), but that helper sends to multiple chat IDs. If one chat succeeds and another fails, the whole event is recorded as failed and will be retried, causing duplicate messages to the chats that already received it. That breaks the intended one-event notification semantics whenever multiple chat IDs are configured. (line 140)

Suggestion: Return per-chat delivery results instead of a single boolean, and persist which recipients have already been delivered so retries only target the remaining failures.


Review by Codex PR Review bot • PR

Co-Authored-By: Codex <noreply@openai.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4a7e7b64d1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +142 to +147
sent = send_telegram_alert(
text=format_review_event_notification(event),
token=token,
chat_ids=chat_ids,
)
return {"status": "sent" if sent else "failed"}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid retrying partially delivered Telegram alerts

When multiple Telegram chat IDs are configured and one of them fails, send_telegram_alert has already attempted delivery to every chat but returns a single false aggregate; this maps the whole review event to failed, so the emitter treats it as retryable and the store will not deduplicate it. In that partial-failure scenario, the chats that did receive the message get the same PR review notification again on each retry instead of only retrying the failed recipient or recording a partial status.

Useful? React with 👍 / 👎.

@Pigbibi

Pigbibi commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

PERMANENT_FREEZE/CLOSE_RESLICE: final closure is exhausted. Current-head review found two reachable blockers: rerun attempts share the same notification identity, and a skipped review step can still emit a completion event. Evidence, branch, commits, tests, and review history are preserved. No second closure, rerun, merge, deploy, or automatic successor.

@Pigbibi Pigbibi closed this Jul 16, 2026
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.

1 participant