From 90f1986fa02a65cfb73a5b8f4e5dd28e0ab8359b Mon Sep 17 00:00:00 2001 From: Matt Rubens <2600+mrubens@users.noreply.github.com> Date: Sun, 12 Jul 2026 13:01:12 +0000 Subject: [PATCH] Improve PR review notifications to skip green-CI padding When review findings are already actionable, omit "CI is passing" mentions and keep focus on offers to fix comments, failed checks, or conflicts. --- CHANGELOG.md | 4 ++-- .../pr-review-notification-delivery.test.ts | 12 +++++++++++- .../pr-review-notification-delivery.ts | 17 ++++++++++++++--- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c1103289..f9f57ceb7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ This file tracks product releases for Roomote (single monorepo version). Automat - Signed public artifact raw URLs (allowlisted images and videos used for visual proofs and PR embeds) expire 30 days after they are signed, and cache headers stay within the remaining TTL, so a leaked screenshot link cannot be fetched indefinitely. - Blaxel sandbox lifecycle is more resilient: deterministic external IDs with idempotent create, bounded retries on readiness-sensitive calls, reuse of preview resources across standby/resume instead of delete-and-recreate, and immediate failure on non-retryable 4xx errors. - Local Docker development rebuilds worker images that lack current networking tools before launching tasks, routes sandbox HTTP/WebSocket traffic through the public app edge so tunneled clients get a usable live session, and marks preview auth cookies Secure when using SameSite=None and Partitioned so iframe previews authenticate reliably. -- PR review notification updates treat failing CI checks and live merge conflicts as high-signal blockers: triage copy names the problem and offers a fix or conflict resolution instead of burying it after a soft "looked good" wrap-up. +- PR review notification updates treat failing CI checks and live merge conflicts as high-signal blockers: triage copy names the problem and offers a fix or conflict resolution instead of burying it after a soft "looked good" wrap-up. When findings or other open feedback are already actionable, the notification no longer pads with “CI is passing”; green checks are only mentioned when there is nothing else to act on. - Main GitHub PR review summary comments now show a compact status footer with the review phase and short commit SHA (`Reviewing abc1234` / `Reviewed abc1234`), using a linked SHA when a commit URL can be built. - Docker and Blaxel standby environments can resume even when they were suspended before the first agent harness session, so early-sleep retains come back to Idle without forcing a new session create path. - The worker common env file (`~/.roomote/env.sh`, which holds deployment secrets such as cloud tokens) is written owner-only (`0o600`) with `~/.roomote` locked to `0o700`, so other sandbox users cannot read those secrets. @@ -60,7 +60,7 @@ This file tracks product releases for Roomote (single monorepo version). Automat ### Patch changes -- Ship the post-0.0.3 develop backlog toward production, including Daytona environment/task snapshot resume with legacy env aliases, default-deny API authorization, R_* public env canonicalization, CI status in PR review feedback replies, sandbox provider UX fixes, and other already-merged fixes. +- Ship the post-0.0.3 develop backlog toward production, including Daytona environment/task snapshot resume with legacy env aliases, default-deny API authorization, R\_\* public env canonicalization, CI status in PR review feedback replies, sandbox provider UX fixes, and other already-merged fixes. ## 0.0.3 (2026-07-11) diff --git a/packages/sdk/src/server/lib/task-runs/__tests__/pr-review-notification-delivery.test.ts b/packages/sdk/src/server/lib/task-runs/__tests__/pr-review-notification-delivery.test.ts index 4182d5b64..a4bd43096 100644 --- a/packages/sdk/src/server/lib/task-runs/__tests__/pr-review-notification-delivery.test.ts +++ b/packages/sdk/src/server/lib/task-runs/__tests__/pr-review-notification-delivery.test.ts @@ -370,11 +370,21 @@ describe('triagePrReviewActivity', () => { 'do not omit the platform name on these self-review messages', ); expect(system).toContain( - 'when "Current pull request state" includes CI check lines', + 'focus the message on offers to address open feedback', ); expect(system).toContain( 'when any CI check is listed as failure or error, treat it as high-signal', ); + expect(system).toContain( + 'when open feedback is already actionable (findings, requested changes, or', + ); + expect(system).toContain('do not mention that CI is passing or green'); + expect(system).toContain( + 'only mention successful or all-green CI when there is nothing actionable', + ); + expect(system).toContain( + 'when "Current pull request state" includes CI check lines', + ); expect(system).toContain( 'when "Current pull request state" includes "- Merge conflicts: yes"', ); diff --git a/packages/sdk/src/server/lib/task-runs/pr-review-notification-delivery.ts b/packages/sdk/src/server/lib/task-runs/pr-review-notification-delivery.ts index 7811f046b..a981094ff 100644 --- a/packages/sdk/src/server/lib/task-runs/pr-review-notification-delivery.ts +++ b/packages/sdk/src/server/lib/task-runs/pr-review-notification-delivery.ts @@ -423,15 +423,26 @@ any feedback. Rules: conflicts), do not add a question or call to action - never claim that any changes were made in response to the feedback, and do not promise follow-up actions -- when "Current pull request state" includes CI check lines (for example - "- Lint: success"), use those live per-check statuses when mentioning CI +- focus the message on offers to address open feedback (comments, findings, + requested changes), failed CI, or merge conflicts. Prefer one clear call to + action for the actionable problem - when any CI check is listed as failure or error, treat it as high-signal and actionable: call the failure out clearly instead of burying it after a soft "looked good" review wrap-up. Prefer a shape like "I reviewed [owner/repo#42](pull request URL) on GitHub and the code looked good overall, but a test is failing in CI. Do you want me to fix it?" Name the failed check when one is listed. Pending checks may get a brief mention but - must not overshadow a hard failure. + must not overshadow a hard failure +- when open feedback is already actionable (findings, requested changes, or + unhandled review comments), do not mention that CI is passing or green — + skip "All listed CI checks are passing" style padding and go straight from + the findings to the offer to help +- only mention successful or all-green CI when there is nothing actionable + (no open feedback, no failed CI, no merge conflicts), and only as brief + context — for example after a clean self-review with no issues +- when "Current pull request state" includes CI check lines (for example + "- Lint: success"), use those live per-check statuses only if you do mention + CI under the rules above - when "Current pull request state" includes "- Merge conflicts: yes", treat conflicts as high-signal and actionable with the same weight as failed CI. Call them out clearly and offer to resolve them, for example: "and the PR