fix: auto-complete approved review comments (#5839)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Issue lifecycle and review handoff rely on a comment-driven "approve" gesture from the active reviewer to transition `in_review` → `done` > - The original auto-completion path matched approval markers loosely and split the comment insert from the status transition, which let `NOT APPROVED` close issues and let a 422-on-status-change leave an orphan comment behind > - That broke the safety expectation that a rejection comment can never auto-complete an issue, and that observable state (comment+status) cannot diverge from intended state > - This pull request tightens the approval regex against negated phrasings and wraps the comment insert + status transition + execution decision in one transaction so a failed transition rolls the comment back > - The benefit is that reviewers can post negated phrasings safely, and any failure in the auto-approval transition leaves the thread unchanged instead of in a half-applied state ## Linked Issues or Issue Description ### What happened? The comment-driven auto-approval path in `routes/issues.ts` had two latent safety bugs surfaced during review: 1. The approval-detection regex matched negated phrasings such as `NOT APPROVED`, `NOT APPROVED.`, `Do not approve`, `Not approving this`, so a reviewer comment intended as a rejection could auto-complete the issue. 2. The auto-approval insert + status transition + execution decision were not atomic. If the post-comment status update returned 422 (`unprocessable`), the persisted approval comment was left behind without the corresponding state change, leaving the thread half-applied. ### Expected behavior - Negated approval phrasings (`NOT APPROVED`, `not approved.`, `I do not approve`, `not approving this`, etc.) must never trigger auto-completion. Positive controls (`Approved`, `LGTM, approved`) must continue to trigger it. - A failed status transition must roll back the corresponding approval comment so observable state and intended state never diverge. ### Steps to reproduce 1. Open an `in_review` issue assigned to a reviewer. 2. As the reviewer, post `NOT APPROVED` as a comment. 3. Prior to this fix: the issue auto-transitions to `done`. After this fix: the issue stays `in_review`, the comment lands, and no transition fires. 4. Separately, induce a 422 on the post-approval status update (e.g. concurrent delete). Prior to this fix: the approval comment is persisted but the issue stays `in_review`. After this fix: the comment is rolled back along with the failed transition. ### Paperclip version or commit Branch tip `ca60f00276` at the time of this submission. Targets `master`. ### Deployment mode Affects both hosted and self-hosted deployments. Behavior is server-side only. ## What Changed - Tightened the review-marker approval regex in `server/src/routes/issues.ts` so it rejects negated phrasings while preserving positive controls. - Required structured `kind: review` / `decision: approved` metadata adjacent to the markdown approval marker (rejects blank-separated structured approval and mismatched actor kinds). - Wrapped the auto-approval comment insert, status transition, and execution decision in a single drizzle transaction in `server/src/routes/issues.ts`, threading the transaction handle through `addComment` in `server/src/services/issues.ts` so a concurrent delete or 422 transition rolls back the comment. - Added a dedicated activity log entry for the post-approval status transition and skipped stale `issue_commented` wakes that arrive after the auto-approval transition. - Added 61 regression tests in `server/src/__tests__/issue-comment-reopen-routes.test.ts` covering negated phrasings, positive controls, structured-metadata adjacency, actor-kind matching, atomic rollback on transition failure, and stale-wake suppression. ## Verification - `cd server && npx vitest run src/__tests__/issue-comment-reopen-routes.test.ts` → 61/61 passing locally. - Typecheck on changed files passes locally. - All required CI checks expected to be green on this branch tip. ## Risks - Low risk. Changes are localized to the comment-driven auto-approval path; existing `addComment` callers are unaffected (the new transaction handle parameter is optional and defaults to the top-level `db`). - The transaction wrapper changes observable timing very slightly (single tx vs. two-step), but the only externally visible effect is atomicity — failures now leave no orphan state. - Regex tightening is opt-out safe (positive controls still match) but if a reviewer in the wild used a creative phrasing not covered by the regression set, they may need to repost as `Approved`. ## Model Used - Provider: Anthropic - Model: Claude Opus 4.7 (`claude-opus-4-7`) - Capabilities: extended thinking, tool use, code execution ## Checklist - [x] I searched for similar open/closed PRs and confirmed this is not a duplicate - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have either linked existing issues or described the issue in-PR following the relevant issue template - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have considered and documented any risks above --------- Co-authored-by: Tommy <tommy@Mac-mini-Anton.local> Co-authored-by: Paperclip <noreply@paperclip.ing> Co-authored-by: Devin Foley <devin@paperclip.ing>
This commit is contained in:
ymmot
+268
-17
@@ -558,6 +558,44 @@ function executionPrincipalsEqual(
|
||||
return left.type === "agent" ? left.agentId === right.agentId : left.userId === right.userId;
|
||||
}
|
||||
|
||||
function actorMatchesExecutionParticipant(
|
||||
actor: { actorType: "user" | "agent"; actorId: string },
|
||||
participant: ParsedExecutionState["currentParticipant"] | null,
|
||||
) {
|
||||
if (!participant) return false;
|
||||
// Require the actor kind to match the participant kind before comparing ids. Without this
|
||||
// an agent and a user that happen to share an id value would falsely satisfy participant
|
||||
// gating on the auto-approval path.
|
||||
if (participant.type !== actor.actorType) return false;
|
||||
return participant.type === "agent" ? participant.agentId === actor.actorId : participant.userId === actor.actorId;
|
||||
}
|
||||
|
||||
// Negation/rejection markers that invalidate an otherwise approval-looking heading.
|
||||
// Match common phrasings ("NOT APPROVED", "Do not approve", "Not approving", "Changes requested",
|
||||
// "Rejected", "Denied", "Blocked") so a reviewer comment intending to reject cannot auto-complete
|
||||
// the issue. We rely on the heading being a single line, so testing the heading text alone is safe.
|
||||
const APPROVAL_NEGATION_REGEX =
|
||||
/\b(?:NOT|REJECT(?:ED|ING|S)?|DENY|DENIED|DENYING|BLOCK(?:ED|ING|S)?|CHANGES?\s+REQUESTED)\b/i;
|
||||
|
||||
function isApprovalReviewComment(body: string) {
|
||||
const normalized = body.replace(/\r\n?/g, "\n");
|
||||
const headingMatch = normalized.match(/(?:^|\n)##\s*Review:\s*([^\n]*)/i);
|
||||
if (headingMatch) {
|
||||
const headingText = headingMatch[1];
|
||||
if (/\bAPPROVED\b/i.test(headingText) && !APPROVAL_NEGATION_REGEX.test(headingText)) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
// Require the `kind: review` and `decision: approved` lines to appear on truly consecutive
|
||||
// lines (no blank-line separation) so prose like "the previous sprint decision: approved"
|
||||
// can't combine with an unrelated `kind: review` line elsewhere in the body to trigger
|
||||
// auto-approval. Use `[ \t]*` between the lines so `\s*` does not silently swallow a newline.
|
||||
return (
|
||||
/^[ \t]*kind[ \t]*:[ \t]*review[ \t]*\n[ \t]*decision[ \t]*:[ \t]*approved[ \t]*$/im.test(normalized)
|
||||
|| /^[ \t]*decision[ \t]*:[ \t]*approved[ \t]*\n[ \t]*kind[ \t]*:[ \t]*review[ \t]*$/im.test(normalized)
|
||||
);
|
||||
}
|
||||
|
||||
function buildExecutionStageWakeContext(input: {
|
||||
state: ParsedExecutionState;
|
||||
wakeRole: ExecutionStageWakeContext["wakeRole"];
|
||||
@@ -919,6 +957,13 @@ function buildExecutionStageWakeup(input: {
|
||||
return null;
|
||||
}
|
||||
|
||||
class AutoApprovalIssueMissingError extends Error {
|
||||
constructor() {
|
||||
super("Issue not found during auto-approval transaction");
|
||||
this.name = "AutoApprovalIssueMissingError";
|
||||
}
|
||||
}
|
||||
|
||||
export function issueRoutes(
|
||||
db: Db,
|
||||
storage: StorageService,
|
||||
@@ -6567,6 +6612,8 @@ export function issueRoutes(
|
||||
let reopenFromStatus: string | null = null;
|
||||
let interruptedRunId: string | null = null;
|
||||
let currentIssue = issue;
|
||||
let issueBeforeCommentDecision = issue;
|
||||
let commentDecisionStageWakeup: ReturnType<typeof buildExecutionStageWakeup> | null = null;
|
||||
const commentReferenceSummaryBefore = await issueReferencesSvc.listIssueReferenceSummary(issue.id);
|
||||
|
||||
let scheduledRetrySupersededByComment = false;
|
||||
@@ -6654,16 +6701,143 @@ export function issueRoutes(
|
||||
}
|
||||
}
|
||||
|
||||
const comment = await svc.addComment(id, req.body.body, {
|
||||
agentId: actor.agentId ?? undefined,
|
||||
userId: actor.actorType === "user" ? actor.actorId : undefined,
|
||||
runId: actor.runId,
|
||||
}, {
|
||||
authorType: req.body.authorType ?? (actor.actorType === "agent" ? "agent" : "user"),
|
||||
presentation: req.body.presentation ?? null,
|
||||
metadata: req.body.metadata ?? null,
|
||||
sourceTrust: await sourceTrustForActorWrite(currentIssue, actor),
|
||||
});
|
||||
const currentExecutionState = parseIssueExecutionState(currentIssue.executionState);
|
||||
const currentExecutionPolicy = normalizeIssueExecutionPolicy(currentIssue.executionPolicy ?? null);
|
||||
const shouldAutoApproveReviewComment =
|
||||
currentIssue.status === "in_review" &&
|
||||
currentExecutionState?.status === "pending" &&
|
||||
actorMatchesExecutionParticipant(actor, currentExecutionState.currentParticipant ?? null) &&
|
||||
isApprovalReviewComment(req.body.body);
|
||||
|
||||
// Persist the comment and the auto-approval state transition atomically when both apply.
|
||||
// Without a single transaction, a 422 (or any error) thrown by the status update after the
|
||||
// comment is inserted would leave an orphan comment without the corresponding state change.
|
||||
let comment: Awaited<ReturnType<typeof svc.addComment>>;
|
||||
if (shouldAutoApproveReviewComment) {
|
||||
const transition = applyIssueExecutionPolicyTransition({
|
||||
issue: currentIssue,
|
||||
policy: currentExecutionPolicy,
|
||||
requestedStatus: "done",
|
||||
requestedAssigneePatch: {},
|
||||
actor: {
|
||||
agentId: actor.agentId ?? null,
|
||||
userId: actor.actorType === "user" ? actor.actorId : null,
|
||||
},
|
||||
commentBody: req.body.body,
|
||||
});
|
||||
const decisionId = transition.decision ? randomUUID() : null;
|
||||
if (decisionId) {
|
||||
const nextExecutionState = transition.patch.executionState;
|
||||
if (!nextExecutionState || typeof nextExecutionState !== "object") {
|
||||
throw new Error("Execution policy decision patch is missing executionState");
|
||||
}
|
||||
transition.patch.executionState = {
|
||||
...nextExecutionState,
|
||||
lastDecisionId: decisionId,
|
||||
};
|
||||
}
|
||||
|
||||
issueBeforeCommentDecision = currentIssue;
|
||||
const updatePatch = {
|
||||
...transition.patch,
|
||||
status: typeof transition.patch.status === "string" ? transition.patch.status : "done",
|
||||
actorAgentId: actor.agentId ?? null,
|
||||
actorUserId: actor.actorType === "user" ? actor.actorId : null,
|
||||
};
|
||||
|
||||
const sourceTrust = await sourceTrustForActorWrite(currentIssue, actor);
|
||||
const commentOptions = {
|
||||
authorType: req.body.authorType ?? (actor.actorType === "agent" ? "agent" : "user"),
|
||||
presentation: req.body.presentation ?? null,
|
||||
metadata: req.body.metadata ?? null,
|
||||
sourceTrust,
|
||||
};
|
||||
let txResult: { comment: Awaited<ReturnType<typeof svc.addComment>>; issue: NonNullable<Awaited<ReturnType<typeof svc.update>>> };
|
||||
try {
|
||||
txResult = await db.transaction(async (tx) => {
|
||||
const insertedComment = await svc.addComment(
|
||||
id,
|
||||
req.body.body,
|
||||
{
|
||||
agentId: actor.agentId ?? undefined,
|
||||
userId: actor.actorType === "user" ? actor.actorId : undefined,
|
||||
runId: actor.runId,
|
||||
},
|
||||
commentOptions,
|
||||
tx,
|
||||
);
|
||||
const updated = await svc.update(id, updatePatch, tx);
|
||||
// Throw (not return null) so drizzle rolls back the inserted comment when the issue
|
||||
// has been concurrently deleted between the initial fetch and the in-transaction update.
|
||||
if (!updated) throw new AutoApprovalIssueMissingError();
|
||||
|
||||
if (transition.decision && decisionId) {
|
||||
await tx.insert(issueExecutionDecisions).values({
|
||||
id: decisionId,
|
||||
companyId: updated.companyId,
|
||||
issueId: updated.id,
|
||||
stageId: transition.decision.stageId,
|
||||
stageType: transition.decision.stageType,
|
||||
actorAgentId: actor.agentId ?? null,
|
||||
actorUserId: actor.actorType === "user" ? actor.actorId : null,
|
||||
outcome: transition.decision.outcome,
|
||||
body: transition.decision.body,
|
||||
createdByRunId: actor.runId ?? null,
|
||||
});
|
||||
}
|
||||
|
||||
return { comment: insertedComment, issue: updated };
|
||||
});
|
||||
} catch (err) {
|
||||
if (err instanceof AutoApprovalIssueMissingError) {
|
||||
res.status(404).json({ error: "Issue not found" });
|
||||
return;
|
||||
}
|
||||
throw err;
|
||||
}
|
||||
comment = txResult.comment;
|
||||
currentIssue = txResult.issue;
|
||||
// Mirror the normal status-change audit trail: every other in_review -> done path
|
||||
// emits an `issue.updated` activity, so emit one here too for the auto-approval path.
|
||||
if (issueBeforeCommentDecision.status !== currentIssue.status) {
|
||||
await logActivity(db, {
|
||||
companyId: currentIssue.companyId,
|
||||
actorType: actor.actorType,
|
||||
actorId: actor.actorId,
|
||||
agentId: actor.agentId,
|
||||
runId: actor.runId,
|
||||
action: "issue.updated",
|
||||
entityType: "issue",
|
||||
entityId: currentIssue.id,
|
||||
details: {
|
||||
status: currentIssue.status,
|
||||
identifier: currentIssue.identifier,
|
||||
source: "auto_approval_comment",
|
||||
_previous: { status: issueBeforeCommentDecision.status },
|
||||
},
|
||||
});
|
||||
}
|
||||
commentDecisionStageWakeup = buildExecutionStageWakeup({
|
||||
issueId: currentIssue.id,
|
||||
previousState: currentExecutionState,
|
||||
nextState: parseIssueExecutionState(currentIssue.executionState),
|
||||
interruptedRunId,
|
||||
requestedByActorType: actor.actorType,
|
||||
requestedByActorId: actor.actorId,
|
||||
});
|
||||
} else {
|
||||
comment = await svc.addComment(id, req.body.body, {
|
||||
agentId: actor.agentId ?? undefined,
|
||||
userId: actor.actorType === "user" ? actor.actorId : undefined,
|
||||
runId: actor.runId,
|
||||
}, {
|
||||
authorType: req.body.authorType ?? (actor.actorType === "agent" ? "agent" : "user"),
|
||||
presentation: req.body.presentation ?? null,
|
||||
metadata: req.body.metadata ?? null,
|
||||
sourceTrust: await sourceTrustForActorWrite(currentIssue, actor),
|
||||
});
|
||||
}
|
||||
|
||||
await issueReferencesSvc.syncComment(comment.id);
|
||||
const commentReferenceSummaryAfter = await issueReferencesSvc.listIssueReferenceSummary(currentIssue.id);
|
||||
const commentReferenceDiff = issueReferencesSvc.diffIssueReferenceSummary(
|
||||
@@ -6735,14 +6909,32 @@ export function issueRoutes(
|
||||
|
||||
// Merge all wakeups from this comment into one enqueue per agent to avoid duplicate runs.
|
||||
void (async () => {
|
||||
const wakeups = new Map<string, Parameters<typeof heartbeat.wakeup>[1]>();
|
||||
type WakeupRequest = NonNullable<Parameters<typeof heartbeat.wakeup>[1]>;
|
||||
const wakeups = new Map<string, { agentId: string; wakeup: WakeupRequest }>();
|
||||
const addWakeup = (agentId: string, wakeup: WakeupRequest) => {
|
||||
const wakeIssueId =
|
||||
wakeup.payload && typeof wakeup.payload === "object" && typeof wakeup.payload.issueId === "string"
|
||||
? wakeup.payload.issueId
|
||||
: currentIssue.id;
|
||||
const key = `${agentId}:${wakeIssueId}`;
|
||||
if (wakeups.has(key)) return;
|
||||
wakeups.set(key, { agentId, wakeup });
|
||||
};
|
||||
|
||||
if (commentDecisionStageWakeup) {
|
||||
addWakeup(commentDecisionStageWakeup.agentId, commentDecisionStageWakeup.wakeup);
|
||||
}
|
||||
|
||||
const assigneeId = currentIssue.assigneeAgentId;
|
||||
const actorIsAgent = actor.actorType === "agent";
|
||||
const selfComment = actorIsAgent && actor.actorId === assigneeId;
|
||||
const skipWake = selfComment || isClosed;
|
||||
// Re-derive closed-ness from the post-mutation issue so the auto-approval
|
||||
// transition (in_review -> done) suppresses a stale `issue_commented` wake
|
||||
// to the returnAssignee for an already-completed issue.
|
||||
const skipWake = selfComment || isClosedIssueStatus(currentIssue.status);
|
||||
if (assigneeId && (reopened || !skipWake)) {
|
||||
if (reopened) {
|
||||
wakeups.set(assigneeId, {
|
||||
addWakeup(assigneeId, {
|
||||
source: "automation",
|
||||
triggerDetail: "system",
|
||||
reason: "issue_reopened_via_comment",
|
||||
@@ -6769,7 +6961,7 @@ export function issueRoutes(
|
||||
},
|
||||
});
|
||||
} else {
|
||||
wakeups.set(assigneeId, {
|
||||
addWakeup(assigneeId, {
|
||||
source: "automation",
|
||||
triggerDetail: "system",
|
||||
reason: "issue_commented",
|
||||
@@ -6804,9 +6996,8 @@ export function issueRoutes(
|
||||
}
|
||||
|
||||
for (const mentionedId of mentionedIds) {
|
||||
if (wakeups.has(mentionedId)) continue;
|
||||
if (actorIsAgent && actor.actorId === mentionedId) continue;
|
||||
wakeups.set(mentionedId, {
|
||||
addWakeup(mentionedId, {
|
||||
source: "automation",
|
||||
triggerDetail: "system",
|
||||
reason: "issue_comment_mentioned",
|
||||
@@ -6824,7 +7015,67 @@ export function issueRoutes(
|
||||
});
|
||||
}
|
||||
|
||||
for (const [agentId, wakeup] of wakeups.entries()) {
|
||||
const becameDone = issueBeforeCommentDecision.status !== "done" && currentIssue.status === "done";
|
||||
if (becameDone) {
|
||||
const dependents = await svc.listWakeableBlockedDependents(currentIssue.id);
|
||||
for (const dependent of dependents) {
|
||||
addWakeup(dependent.assigneeAgentId, {
|
||||
source: "automation",
|
||||
triggerDetail: "system",
|
||||
reason: "issue_blockers_resolved",
|
||||
payload: {
|
||||
issueId: dependent.id,
|
||||
resolvedBlockerIssueId: currentIssue.id,
|
||||
blockerIssueIds: dependent.blockerIssueIds,
|
||||
},
|
||||
requestedByActorType: actor.actorType,
|
||||
requestedByActorId: actor.actorId,
|
||||
contextSnapshot: {
|
||||
issueId: dependent.id,
|
||||
taskId: dependent.id,
|
||||
wakeReason: "issue_blockers_resolved",
|
||||
source: "issue.blockers_resolved",
|
||||
resolvedBlockerIssueId: currentIssue.id,
|
||||
blockerIssueIds: dependent.blockerIssueIds,
|
||||
},
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
const becameTerminal =
|
||||
!["done", "cancelled"].includes(issueBeforeCommentDecision.status) &&
|
||||
["done", "cancelled"].includes(currentIssue.status);
|
||||
if (becameTerminal && currentIssue.parentId) {
|
||||
const parent = await svc.getWakeableParentAfterChildCompletion(currentIssue.parentId);
|
||||
if (parent) {
|
||||
addWakeup(parent.assigneeAgentId, {
|
||||
source: "automation",
|
||||
triggerDetail: "system",
|
||||
reason: "issue_children_completed",
|
||||
payload: {
|
||||
issueId: parent.id,
|
||||
completedChildIssueId: currentIssue.id,
|
||||
childIssueIds: parent.childIssueIds,
|
||||
childIssueSummaries: parent.childIssueSummaries,
|
||||
childIssueSummaryTruncated: parent.childIssueSummaryTruncated,
|
||||
},
|
||||
requestedByActorType: actor.actorType,
|
||||
requestedByActorId: actor.actorId,
|
||||
contextSnapshot: {
|
||||
issueId: parent.id,
|
||||
taskId: parent.id,
|
||||
wakeReason: "issue_children_completed",
|
||||
source: "issue.children_completed",
|
||||
completedChildIssueId: currentIssue.id,
|
||||
childIssueIds: parent.childIssueIds,
|
||||
childIssueSummaries: parent.childIssueSummaries,
|
||||
childIssueSummaryTruncated: parent.childIssueSummaryTruncated,
|
||||
},
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
for (const { agentId, wakeup } of wakeups.values()) {
|
||||
heartbeat
|
||||
.wakeup(agentId, wakeup)
|
||||
.catch((err) => logger.warn({ err, issueId: currentIssue.id, agentId }, "failed to wake agent on issue comment"));
|
||||
|
||||
Reference in New Issue
Block a user