Skip to content

[Fix] Review notifications omit resolve actions for open feedback #1165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
roomote-roomote wants to merge 2 commits into from
Closed

[Fix] Review notifications omit resolve actions for open feedback #1165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
73c4f78
fix: preserve PR review resolve actions
daniel-lxs Aug 7, 2026
53fe8f3
fix: include all actionable PR state
daniel-lxs Aug 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
259 changes: 258 additions & 1 deletion packages/sdk/src/server/lib/task-runs/__tests__/pr-review-notification-delivery.test.ts
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

76 changes: 69 additions & 7 deletions packages/sdk/src/server/lib/task-runs/pr-review-notification-delivery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -750,11 +750,44 @@ export async function triagePrReviewActivity({
prompt,
});

if (!object.worthNotifying && !containsSelfReviewResult) {
const unresolvedThreadCount = context?.unresolvedThreadCount ?? 0;
const failedCheckNames =
context?.ciStatus?.checks
.filter((check) => check.status === 'failure' || check.status === 'error')
.map((check) => check.name) ?? [];
const hasMergeConflicts = context?.mergeable === false;
const actionableProblemCount =
unresolvedThreadCount +
failedCheckNames.length +
(hasMergeConflicts ? 1 : 0);
const hasLiveActionableState = actionableProblemCount > 0;

if (
!object.worthNotifying &&
!containsSelfReviewResult &&
!hasLiveActionableState
) {
return { post: false, reason: 'not_worth_notifying' };
}

const summary = object.summary.trim();
const deterministicSummary = [
...(unresolvedThreadCount > 0
? [
`${unresolvedThreadCount} unresolved review ${unresolvedThreadCount === 1 ? 'thread remains' : 'threads remain'}`,
]
: []),
...(failedCheckNames.length > 0
? [
`${failedCheckNames.length} CI ${failedCheckNames.length === 1 ? 'check is' : 'checks are'} failing`,
]
: []),
...(hasMergeConflicts ? ['the pull request has merge conflicts'] : []),
].join(', ');
const summary =
object.summary.trim() ||
(hasLiveActionableState
? `[${repository}#${prNumber}](${prUrl}) needs attention: ${deterministicSummary}.`
: '');

if (!summary) {
throw new Error(
Expand All @@ -764,15 +797,44 @@ export async function triagePrReviewActivity({

const followUpQuestion = object.followUpQuestion.trim();
const followUpPrompt = object.followUpPrompt.trim();
// The offer is only actionable when both halves exist; a question without
// an injectable instruction (or vice versa) degrades to a plain summary.
const hasFollowUp = followUpQuestion.length > 0 && followUpPrompt.length > 0;
const hasGeneratedFollowUp =
followUpQuestion.length > 0 && followUpPrompt.length > 0;
const fallbackInstructions = [
...(unresolvedThreadCount > 0
? [
'Revalidate each unresolved review comment against the current code and address valid issues.',
]
: []),
...(failedCheckNames.length > 0
? [
`Investigate and fix the failing CI checks: ${failedCheckNames.join(', ')}.`,

@roomote-community roomote-community Bot Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CI check names are untrusted PR-controlled data (a pull_request workflow can assign a job an instruction-looking name), but this fallback interpolates them verbatim into followUpPrompt. That prompt is auto-dispatched directly to the agent when auto-handle is enabled, so a malicious check name can become an agent instruction without user interaction. Preserve the label as quoted data or sanitize it before building the executable follow-up prompt.

]
: []),
...(hasMergeConflicts
? [
"Resolve the pull request's merge conflicts without discarding either side's intended changes.",
]
: []),
];
const fallbackFollowUp = hasLiveActionableState
? {
question:
actionableProblemCount === 1
? 'Would you like me to resolve this issue?'
: 'Would you like me to resolve these issues?',
prompt: `Review and resolve the actionable problems on [${repository}#${prNumber}](${prUrl}). ${fallbackInstructions.join(' ')} Update the pull request with the validated fixes.`,
}
: null;

return {
post: true,
summary,
followUpQuestion: hasFollowUp ? followUpQuestion : null,
followUpPrompt: hasFollowUp ? followUpPrompt : null,
followUpQuestion:
fallbackFollowUp?.question ??
(hasGeneratedFollowUp ? followUpQuestion : null),
followUpPrompt:
fallbackFollowUp?.prompt ??
(hasGeneratedFollowUp ? followUpPrompt : null),
};
}

Expand Down
Loading