Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions apps/desktop/src/main/services/ipc/registerIpc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,10 @@ import type {
LandResult,
LandStackEnhancedArgs,
LandQueueNextArgs,
CleanupPrBranchArgs,
CleanupPrBranchResult,
PrCheck,
PrCommit,
PrComment,
PrReviewThread,
PrHealth,
Expand Down Expand Up @@ -5878,6 +5881,7 @@ export function registerIpc({

ipcMain.handle(IPC.prsGetDetail, (_e, args: { prId: string }) => getCtx().prService.getDetail(args.prId));
ipcMain.handle(IPC.prsGetFiles, (_e, args: { prId: string }) => getCtx().prService.getFiles(args.prId));
ipcMain.handle(IPC.prsGetCommits, (_e, args: { prId: string }): Promise<PrCommit[]> => getCtx().prService.getCommits(args.prId));
ipcMain.handle(IPC.prsGetActionRuns, (_e, args: { prId: string }) => getCtx().prService.getActionRuns(args.prId));
ipcMain.handle(IPC.prsGetActivity, (_e, args: { prId: string }) => getCtx().prService.getActivity(args.prId));
ipcMain.handle(IPC.prsAddComment, (_e, args) => getCtx().prService.addComment(args));
Expand Down Expand Up @@ -5930,6 +5934,8 @@ export function registerIpc({
);
},
);
ipcMain.handle(IPC.prsCleanupBranch, (_e, args: CleanupPrBranchArgs): Promise<CleanupPrBranchResult> =>
getCtx().prService.cleanupBranch(args));

// Issue Inventory (PR convergence loop)
ipcMain.handle(IPC.prsIssueInventorySync, async (_e, args: { prId: string }): Promise<IssueInventorySnapshot> => {
Expand Down
102 changes: 102 additions & 0 deletions apps/desktop/src/main/services/prs/issueInventoryService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,108 @@ describe("issueInventoryService", () => {
expect(insertCalls.length).toBe(0);
});

it("treats unresolved threads with a latest resolution acknowledgement as fixed", () => {
const db = makeMockDb();
db.get.mockReturnValue(makeFakeRow({
external_id: "thread:thread-ack",
state: "new",
thread_comment_count: 1,
thread_latest_comment_id: "comment-1",
}));
db.all.mockReturnValue([makeFakeRow({
external_id: "thread:thread-ack",
state: "new",
thread_comment_count: 1,
thread_latest_comment_id: "comment-1",
})]);

const service = createIssueInventoryService({ db });
service.syncFromPrData(
PR_ID,
[],
[makeReviewThread({
id: "thread-ack",
comments: [
{
id: "comment-1",
author: "reviewer",
authorAvatarUrl: null,
body: "This needs a null check.",
url: null,
createdAt: "2026-03-23T12:00:00.000Z",
updatedAt: "2026-03-23T12:00:00.000Z",
},
{
id: "comment-2",
author: "arul28",
authorAvatarUrl: null,
body: "Fixed in the latest commit.",
url: null,
createdAt: "2026-03-23T12:10:00.000Z",
updatedAt: "2026-03-23T12:10:00.000Z",
},
],
})],
[],
);

const updateCalls = db.run.mock.calls.filter(
(call: unknown[]) => typeof call[0] === "string" && (call[0] as string).includes("update pr_issue_inventory"),
);
expect(updateCalls.some((call: unknown[]) => (call[1] as unknown[])[8] === "fixed")).toBe(true);
});

it("does not treat negative resolution wording as fixed", () => {
const db = makeMockDb();
db.get.mockReturnValue(makeFakeRow({
external_id: "thread:thread-not-fixed",
state: "new",
thread_comment_count: 1,
thread_latest_comment_id: "comment-1",
}));
db.all.mockReturnValue([makeFakeRow({
external_id: "thread:thread-not-fixed",
state: "new",
thread_comment_count: 1,
thread_latest_comment_id: "comment-1",
})]);

const service = createIssueInventoryService({ db });
service.syncFromPrData(
PR_ID,
[],
[makeReviewThread({
id: "thread-not-fixed",
comments: [
{
id: "comment-1",
author: "reviewer",
authorAvatarUrl: null,
body: "Please tighten this logic.",
url: null,
createdAt: "2026-03-23T12:00:00.000Z",
updatedAt: "2026-03-23T12:00:00.000Z",
},
{
id: "comment-2",
author: "reviewer",
authorAvatarUrl: null,
body: "This is not fixed yet.",
url: null,
createdAt: "2026-03-23T12:10:00.000Z",
updatedAt: "2026-03-23T12:10:00.000Z",
},
],
})],
[],
);

const updateCalls = db.run.mock.calls.filter(
(call: unknown[]) => typeof call[0] === "string" && (call[0] as string).includes("update pr_issue_inventory"),
);
expect(updateCalls.some((call: unknown[]) => (call[1] as unknown[])[8] === "fixed")).toBe(false);
});

it("skips outdated review threads", () => {
const db = makeMockDb();
db.get.mockReturnValue(null);
Expand Down
13 changes: 11 additions & 2 deletions apps/desktop/src/main/services/prs/issueInventoryService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import type {
PrReviewThread,
} from "../../../shared/types";
import { DEFAULT_CONVERGENCE_RUNTIME_STATE, DEFAULT_PIPELINE_SETTINGS } from "../../../shared/types";
import { isNoisyIssueComment } from "./resolverUtils";
import { isNoisyIssueComment, looksLikeResolutionAck } from "./resolverUtils";
import { nowIso } from "../shared/utils";

// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -740,7 +740,16 @@ export function createIssueInventoryService(deps: { db: AdeDb }) {
threadLatestCommentSource: source,
};

if (thread.isResolved || thread.isOutdated) {
// Only treat as a resolution ACK when the latest reply is from a real
// human (or an unclassified author). Review bots — CodeRabbit (normalized
// from `coderabbitai`), Copilot, Codex, Greptile, Seer, ADE, etc. — often
// mention "fixed/resolved/done" inside long markdown bodies without
// actually acknowledging a fix, so passing their comments through the
// heuristic silently flips real, open threads to `"fixed"`.
const latestReplyLooksResolved = (source === "human" || source === "unknown")
&& looksLikeResolutionAck(body);

Comment thread
greptile-apps[bot] marked this conversation as resolved.
if (thread.isResolved || thread.isOutdated || latestReplyLooksResolved) {
if (!existing) continue;
upsertItem(prId, externalId, threadData, {
state: "fixed",
Expand Down
2 changes: 2 additions & 0 deletions apps/desktop/src/main/services/prs/prIssueResolver.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,8 @@ describe("buildPrIssueResolutionPrompt", () => {
expect(prompt).toContain("Watch carefully for regressions caused by your fixes.");
expect(prompt).toContain("update the test");
expect(prompt).toContain("rerun the complete failing test files or suites locally");
expect(prompt).toContain("one bounded Path to Merge resolution round");
expect(prompt).toContain("ADE will poll GitHub");
expect(prompt).toContain("Commit the changes and push the PR branch before you stop.");
expect(prompt).toContain("If you cannot safely commit or push the necessary changes");
expect(prompt).toContain("prRefreshIssueInventory");
Expand Down
4 changes: 3 additions & 1 deletion apps/desktop/src/main/services/prs/prIssueResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,9 @@ export function buildPrIssueResolutionPrompt(args: IssueResolutionPromptArgs): s
"",
"Requirements",
"- Fix all valid issues in the selected scope, not just the first one.",
"- Treat this chat as one bounded Path to Merge resolution round. Make one coherent set of fixes for the current inventory, then hand control back to ADE.",
"- If you make local code or git changes that should affect the PR, do not finish with local-only state. Commit the changes and push the PR branch before you stop.",
"- After you push, do not wait indefinitely for CI or advisory review bots. ADE will poll GitHub, observe custom post-push comments, and launch the next round if new actionable work appears.",
"- If you only resolve stale review threads or other PR metadata with ADE tools and no local git changes are needed, say that clearly in your final note.",
"- If you cannot safely commit or push the necessary changes, stop with a concrete blocker instead of exiting as if the round succeeded.",
);
Expand Down Expand Up @@ -559,7 +561,7 @@ export function buildPrIssueResolutionPrompt(args: IssueResolutionPromptArgs): s
"- Before you push, rerun the complete failing test files or suites locally, not just the specific failing test names. Test runners and sharded CI can hide additional failures behind the first error in a file.",
"- Treat newly added or heavily modified test files as likely regression hotspots, even if CI only surfaced a different failure first.",
"- Watch carefully for regressions caused by your fixes. If a change breaks an existing test because the expected behavior legitimately changed, update the test. Do not change tests just to mask a bug.",
"- Continue iterating until the selected issue set is cleared and CI is green, or stop only with a concrete blocker and explain it clearly.",
"- Stop with a concise final note that lists what changed, what you validated, whether you pushed, and any concrete blocker ADE should surface to the operator.",
);
if (isIncremental) {
promptSections.push(
Expand Down
88 changes: 88 additions & 0 deletions apps/desktop/src/main/services/prs/prService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import type {
CommitIntegrationArgs,
CleanupIntegrationWorkflowArgs,
CleanupIntegrationWorkflowResult,
CleanupPrBranchArgs,
CleanupPrBranchResult,
DeleteIntegrationProposalArgs,
DeleteIntegrationProposalResult,
DismissIntegrationCleanupArgs,
Expand Down Expand Up @@ -2615,6 +2617,10 @@ export function createPrService({
const createdAt = nowIso();
const headBranch = asString(pr?.head?.ref) || branchNameFromRef(lane.branchRef);
const baseBranch = asString(pr?.base?.ref) || branchNameFromRef(lane.baseRef);
const laneBranch = branchNameFromRef(lane.branchRef);
if (normalizeBranchName(laneBranch) !== normalizeBranchName(headBranch)) {
throw new Error(`Cannot link PR #${locator.number} to lane "${lane.name}" because the PR head branch is "${headBranch}" but the lane branch is "${laneBranch}".`);
}

// Backfill creation_strategy for imported PRs that don't have one stored yet.
// The wizard defaults to "pr_target" when users create via the UI; mirror
Expand Down Expand Up @@ -2653,6 +2659,78 @@ export function createPrService({
return await refreshOne(prId);
};

const cleanupBranch = async (args: CleanupPrBranchArgs): Promise<CleanupPrBranchResult> => {
const row = getRow(args.prId);
if (!row) throw new Error(`PR not found: ${args.prId}`);
if (row.state !== "merged" && row.state !== "closed") {
throw new Error("Branch cleanup is only available after a PR is merged or closed.");
}

const branchName = branchNameFromRef(row.head_branch);
if (!branchName || branchName === "HEAD") {
throw new Error("PR head branch is missing.");
}

const lanes = await laneService.list({ includeArchived: true, includeStatus: false });
const primaryBranches = new Set(
lanes
.filter((lane) => lane.laneType === "primary")
.map((lane) => normalizeBranchName(branchNameFromRef(lane.branchRef)))
.filter(Boolean),
);
if (primaryBranches.has(normalizeBranchName(branchName))) {
throw new Error(`Refusing to clean up protected branch "${branchName}".`);
}

const result: CleanupPrBranchResult = {
prId: args.prId,
branchName,
localDeleted: false,
remoteDeleted: false,
localError: null,
remoteError: null,
};

if (args.deleteLocalBranch !== false) {
const refCheck = await runGit(["show-ref", "--verify", "--quiet", `refs/heads/${branchName}`], {
cwd: projectRoot,
timeoutMs: 8_000,
});
if (refCheck.exitCode === 0) {
Comment thread
greptile-apps[bot] marked this conversation as resolved.
// Only force-delete when the PR is merged. For closed (non-merged) PRs
// the branch may still hold unintegrated work, so use the safe `-d`
// variant which refuses to drop unmerged commits.
const deleteFlag = row.state === "merged" ? "-D" : "-d";
const deleted = await runGit(["branch", deleteFlag, branchName], { cwd: projectRoot, timeoutMs: 30_000 });
if (deleted.exitCode === 0) result.localDeleted = true;
else result.localError = deleted.stderr || deleted.stdout || `Failed to delete local branch ${branchName}`;
}
}

if (args.deleteRemoteBranch) {
const remote = args.remoteName?.trim() || "origin";
const remoteCheck = await runGit(["remote", "get-url", remote], { cwd: projectRoot, timeoutMs: 8_000 });
if (remoteCheck.exitCode !== 0) {
result.remoteError = `Remote '${remote}' is not configured for this repository`;
} else {
const remoteRefCheck = await runGit(["ls-remote", "--heads", remote, branchName], {
cwd: projectRoot,
timeoutMs: 12_000,
});
if (remoteRefCheck.exitCode === 0 && remoteRefCheck.stdout.trim().length > 0) {
const deleted = await runGit(["push", remote, "--delete", branchName], { cwd: projectRoot, timeoutMs: 45_000 });
if (deleted.exitCode === 0) result.remoteDeleted = true;
else result.remoteError = deleted.stderr || deleted.stdout || `Failed to delete remote branch ${branchName}`;
}
Comment on lines +2665 to +2724
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Avoid force-deleting branches for closed-but-unmerged PRs.

This path treats "closed" the same as "merged" and then uses git branch -D plus optional remote deletion. For a PR that was closed without merging, that can erase the only copy of unmerged work. At minimum, gate cleanup to merged PRs until this API has an explicit force/unsafe mode for abandoned branches.

Suggested safe default
-    if (row.state !== "merged" && row.state !== "closed") {
-      throw new Error("Branch cleanup is only available after a PR is merged or closed.");
+    if (row.state !== "merged") {
+      throw new Error("Branch cleanup is only available after a PR is merged.");
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (row.state !== "merged" && row.state !== "closed") {
throw new Error("Branch cleanup is only available after a PR is merged or closed.");
}
const branchName = branchNameFromRef(row.head_branch);
if (!branchName || branchName === "HEAD") {
throw new Error("PR head branch is missing.");
}
const lanes = await laneService.list({ includeArchived: true, includeStatus: false });
const primaryBranches = new Set(
lanes
.filter((lane) => lane.laneType === "primary")
.map((lane) => normalizeBranchName(branchNameFromRef(lane.branchRef)))
.filter(Boolean),
);
if (primaryBranches.has(normalizeBranchName(branchName))) {
throw new Error(`Refusing to clean up protected branch "${branchName}".`);
}
const result: CleanupPrBranchResult = {
prId: args.prId,
branchName,
localDeleted: false,
remoteDeleted: false,
localError: null,
remoteError: null,
};
if (args.deleteLocalBranch !== false) {
const refCheck = await runGit(["show-ref", "--verify", "--quiet", `refs/heads/${branchName}`], {
cwd: projectRoot,
timeoutMs: 8_000,
});
if (refCheck.exitCode === 0) {
const deleted = await runGit(["branch", "-D", branchName], { cwd: projectRoot, timeoutMs: 30_000 });
if (deleted.exitCode === 0) result.localDeleted = true;
else result.localError = deleted.stderr || deleted.stdout || `Failed to delete local branch ${branchName}`;
}
}
if (args.deleteRemoteBranch) {
const remote = args.remoteName?.trim() || "origin";
const remoteCheck = await runGit(["remote", "get-url", remote], { cwd: projectRoot, timeoutMs: 8_000 });
if (remoteCheck.exitCode !== 0) {
result.remoteError = `Remote '${remote}' is not configured for this repository`;
} else {
const remoteRefCheck = await runGit(["ls-remote", "--heads", remote, branchName], {
cwd: projectRoot,
timeoutMs: 12_000,
});
if (remoteRefCheck.exitCode === 0 && remoteRefCheck.stdout.trim().length > 0) {
const deleted = await runGit(["push", remote, "--delete", branchName], { cwd: projectRoot, timeoutMs: 45_000 });
if (deleted.exitCode === 0) result.remoteDeleted = true;
else result.remoteError = deleted.stderr || deleted.stdout || `Failed to delete remote branch ${branchName}`;
}
if (row.state !== "merged") {
throw new Error("Branch cleanup is only available after a PR is merged.");
}
const branchName = branchNameFromRef(row.head_branch);
if (!branchName || branchName === "HEAD") {
throw new Error("PR head branch is missing.");
}
const lanes = await laneService.list({ includeArchived: true, includeStatus: false });
const primaryBranches = new Set(
lanes
.filter((lane) => lane.laneType === "primary")
.map((lane) => normalizeBranchName(branchNameFromRef(lane.branchRef)))
.filter(Boolean),
);
if (primaryBranches.has(normalizeBranchName(branchName))) {
throw new Error(`Refusing to clean up protected branch "${branchName}".`);
}
const result: CleanupPrBranchResult = {
prId: args.prId,
branchName,
localDeleted: false,
remoteDeleted: false,
localError: null,
remoteError: null,
};
if (args.deleteLocalBranch !== false) {
const refCheck = await runGit(["show-ref", "--verify", "--quiet", `refs/heads/${branchName}`], {
cwd: projectRoot,
timeoutMs: 8_000,
});
if (refCheck.exitCode === 0) {
const deleted = await runGit(["branch", "-D", branchName], { cwd: projectRoot, timeoutMs: 30_000 });
if (deleted.exitCode === 0) result.localDeleted = true;
else result.localError = deleted.stderr || deleted.stdout || `Failed to delete local branch ${branchName}`;
}
}
if (args.deleteRemoteBranch) {
const remote = args.remoteName?.trim() || "origin";
const remoteCheck = await runGit(["remote", "get-url", remote], { cwd: projectRoot, timeoutMs: 8_000 });
if (remoteCheck.exitCode !== 0) {
result.remoteError = `Remote '${remote}' is not configured for this repository`;
} else {
const remoteRefCheck = await runGit(["ls-remote", "--heads", remote, branchName], {
cwd: projectRoot,
timeoutMs: 12_000,
});
if (remoteRefCheck.exitCode === 0 && remoteRefCheck.stdout.trim().length > 0) {
const deleted = await runGit(["push", remote, "--delete", branchName], { cwd: projectRoot, timeoutMs: 45_000 });
if (deleted.exitCode === 0) result.remoteDeleted = true;
else result.remoteError = deleted.stderr || deleted.stdout || `Failed to delete remote branch ${branchName}`;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/desktop/src/main/services/prs/prService.ts` around lines 2522 - 2577,
The current check treats row.state === "closed" the same as "merged" and allows
destructive deletes; change the gate so cleanup only proceeds for merged PRs
(i.e., require row.state === "merged" and throw for any other state), preventing
local (branch deletion via runGit(["branch","-D", ...])) and remote deletions
for closed-but-unmerged PRs until an explicit force/unsafe flag is added to the
API; update the initial state check around row.state and ensure the code paths
using branchName, args.deleteLocalBranch and args.deleteRemoteBranch remain
reachable only when the PR is merged.

}
}

if (result.localError || result.remoteError) {
logger.warn("prs.branch_cleanup_partial_failure", result);
}
return result;
};

const land = async (args: LandPrArgs): Promise<LandResult> => {
const row = getRow(args.prId);
if (!row) throw new Error(`PR not found: ${args.prId}`);
Expand Down Expand Up @@ -5034,6 +5112,10 @@ export function createPrService({
return await linkToLane(args);
},

async cleanupBranch(args: CleanupPrBranchArgs): Promise<CleanupPrBranchResult> {
return await cleanupBranch(args);
},

getForLane(laneId: string): PrSummary | null {
const row = getRowForLane(laneId);
return row ? rowToSummary(row) : null;
Expand Down Expand Up @@ -5306,6 +5388,12 @@ export function createPrService({
return files;
},

async getCommits(prId: string): Promise<PrCommit[]> {
const commits = await getCommitsSnapshot(prId);
upsertSnapshotRow({ prId, commits });
return commits;
},

async getActionRuns(prId: string): Promise<PrActionRun[]> {
const row = requireRow(prId);
const repo = repoFromRow(row);
Expand Down
39 changes: 39 additions & 0 deletions apps/desktop/src/main/services/prs/resolverUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ const NOISY_BODY_PATTERNS = [
/<!-- internal state/i,
/walkthrough/i,
/@codex review/i,
/^@(copilot|coderabbitai|greptile|codex)\s+review\b/i,
/\bship-lane handoff\b/i,
/\bclear-to-merge\b/i,
/\bci green\b/i,
];

export function isNoisyIssueComment(comment: PrComment): boolean {
Expand All @@ -26,6 +30,41 @@ export function isNoisyIssueComment(comment: PrComment): boolean {
return NOISY_BODY_PATTERNS.some((pattern) => pattern.test(body));
}

const RESOLUTION_NEGATION_PATTERNS = [
/\bnot\s+(fixed|addressed|resolved|done|handled)\b/i,
/\b(still|isn'?t|is not|not yet)\s+(an issue|fixed|addressed|resolved|done|handled|working)\b/i,
/\b(no|doesn'?t|does not|won'?t|can'?t|cannot|hasn'?t|haven'?t)\s+(fix|address|resolve|handle|be\s+(fixed|addressed|resolved|done|handled))\b/i,
/\b(will|to\s+be)\s+(fixed|addressed|resolved|done|handled)\s+(in|by)\b/i,
/\bstill\s+(broken|failing|an\s+issue)\b/i,
];

Comment thread
greptile-apps[bot] marked this conversation as resolved.
// ACK patterns are intentionally narrow: they must look like an actual
// acknowledgement rather than any mention of the verbs "fixed/resolved/done".
// `looksLikeResolutionAck` auto-flips issue inventory items to `"fixed"`, so
// false positives silently drop live review threads from the convergence loop.
const RESOLUTION_ACK_PATTERNS = [
// Terse standalone acks: "fixed.", "done!", "resolved"
/^\s*(fixed|addressed|resolved|done|handled)[.! ]*$/i,
// First-person / subject acks: "I fixed", "we addressed", "this is fixed"
/\b(i|we|i've|we've|this\s+(is|was)|that\s+(is|was)|it\s+(is|was))\s+(now\s+)?(fixed|addressed|resolved|done|handled)\b/i,
// Commit/PR-reference acks: "Fixed in commit abc", "addressed in #123",
// "resolved in the latest push". Scoped to concrete references to avoid
// matching deferrals like "addressed in a follow-up PR".
/^\s*(fixed|addressed|resolved|handled)\s+(in|by)\s+(the\s+latest|commit\b|#\d+|[0-9a-f]{7,40}\b|pr\s*#?\d+|my\s+(latest|most\s+recent))/i,
/\bshould be (good|fixed|resolved)\b/i,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[🟡 Medium] [🔵 Bug]

The first RESOLUTION_ACK_PATTERNS entry /\b(fixed|addressed|resolved|done|handled)\b/i matches extremely common words in isolation. A thread reply like "Once we're done with the refactor, this can be revisited" or "I handled the error differently in the old version" would match done/handled and cause looksLikeResolutionAck to return true, auto-marking the issue as fixed in the convergence pipeline.

// apps/desktop/src/main/services/prs/resolverUtils.ts
const RESOLUTION_ACK_PATTERNS = [
  /\b(fixed|addressed|resolved|done|handled)\b/i,  // "done" is extremely broad

The negation patterns only catch explicit negations ("not fixed", "still an issue") but not conversational uses of "done" or "handled". Since false positives silently close actionable review threads, this could cause real issues to be skipped by the PR convergence pipeline. Consider anchoring these words more tightly — e.g., requiring them near the start of short comments (/^\s*(fixed|done|handled|addressed|resolved)\.?\s*$/i) or requiring a preceding verb/pronoun ("I fixed", "this is handled").

/\bno longer (an issue|applies|reproduces)\b/i,
/\bthanks[,! ]+(fixed|addressed|resolved)\b/i,
/\bclear-to-merge\b/i,
/\bci green\b/i,
];

export function looksLikeResolutionAck(body: string | null | undefined): boolean {
const text = (body ?? "").trim();
if (!text) return false;
if (RESOLUTION_NEGATION_PATTERNS.some((pattern) => pattern.test(text))) return false;
return RESOLUTION_ACK_PATTERNS.some((pattern) => pattern.test(text));
}
Comment on lines +45 to +66
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

looksLikeResolutionAck is too permissive and can auto-mark live threads as fixed.

RESOLUTION_ACK_PATTERNS matches a bare word fixed | addressed | resolved | done | handled anywhere in the body, and RESOLUTION_NEGATION_PATTERNS only catches a narrow set of adjacency forms (not <verb>, still/isn't/is not/not yet <verb>, no/doesn't/does not <verb>). That leaves several everyday human replies that will incorrectly satisfy the heuristic, and — via issueInventoryService.syncFromPrData — flip an inventory item to state: "fixed" without any real acknowledgement:

  • "hasn't been fixed" / "won't be resolved" — no negation pattern matches, ACK matches fixed / resolved.
  • "Can you confirm this is resolved?" — ACK matches resolved; it's a question, not an ack.
  • "This was fixed in PR #123 but reintroduced after the merge." — ACK matches; no adjacent "not".
  • "Still broken, haven't fixed it yet." — negation requires still <verb>, not still broken; ACK matches fixed.
  • "Are we done discussing this?" / "poorly done." — ACK matches done.

This is especially risky because pr_pipeline_settings.auto_merge can drive merges off the convergence status computed from the inventory. A stray human comment containing one of these words on an unresolved thread could silently be counted toward convergence.

Suggestions (any of these would materially reduce false positives):

  1. Require the ack to be short / mostly ack-like (e.g., body length under ~120 chars after trimming, or the ack phrase within the first line) — genuine resolution acks are usually terse.
  2. Drop done and handled from the ack patterns; they carry the most ambient false positives.
  3. Expand negation coverage: add haven'?t, hasn'?t, won'?t, can'?t, cannot, and (need|needs|needed) to be before the ack verbs, and allow a small word gap (\b(not|never|hasn'?t|haven'?t)\b[\w\s]{0,15}\b(fixed|addressed|resolved|done|handled)\b).
  4. Require a positive anchor (start-of-body or directly after common lead-ins like "this is", "should be", "now") rather than matching anywhere in the body.
🛡️ Illustrative tightening
-const RESOLUTION_NEGATION_PATTERNS = [
-  /\bnot\s+(fixed|addressed|resolved|done|handled)\b/i,
-  /\b(still|isn'?t|is not|not yet)\s+(an issue|fixed|addressed|resolved|done|handled|working)\b/i,
-  /\b(no|doesn'?t|does not)\s+(fix|address|resolve|handle)\b/i,
-];
-
-const RESOLUTION_ACK_PATTERNS = [
-  /\b(fixed|addressed|resolved|done|handled)\b/i,
-  /\bshould be (good|fixed|resolved)\b/i,
-  /\bno longer (an issue|applies|reproduces)\b/i,
-  /\bthanks[,! ]+(fixed|addressed|resolved)\b/i,
-  /\bclear-to-merge\b/i,
-  /\bci green\b/i,
-];
+const RESOLUTION_NEGATION_PATTERNS = [
+  /\b(not|never|hasn'?t|haven'?t|won'?t|can'?t|cannot|isn'?t|is not|not yet|still)\b[^\n]{0,40}\b(fixed|addressed|resolved|done|handled|working)\b/i,
+  /\b(no|doesn'?t|does not|didn'?t|did not)\s+(fix|address|resolve|handle)\b/i,
+  /\?\s*$/, // questions are not acks
+];
+
+// Anchor the ack near the start of the body so casual mentions deeper in
+// long replies don't flip inventory state.
+const RESOLUTION_ACK_PATTERNS = [
+  /^\s*(this (is|was|should be)\s+)?(now\s+)?(fixed|addressed|resolved)\b/i,
+  /^\s*should be (good|fixed|resolved)\b/i,
+  /^\s*no longer (an issue|applies|reproduces)\b/i,
+  /^\s*thanks[,! ]+(fixed|addressed|resolved)\b/i,
+  /\bclear-to-merge\b/i,
+  /\bci green\b/i,
+];
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/desktop/src/main/services/prs/resolverUtils.ts` around lines 39 - 53,
looksLikeResolutionAck is too permissive and auto-marks threads fixed; update
the logic in looksLikeResolutionAck and the pattern constants
(RESOLUTION_ACK_PATTERNS, RESOLUTION_NEGATION_PATTERNS) to reduce false
positives: remove the ambiguous tokens "done" and "handled" from
RESOLUTION_ACK_PATTERNS, add expanded negation tokens (haven'?t, hasn'?t,
won'?t, can'?t, cannot, need(s|ed) to be) and a small-word-gap negation form
like \b(not|never|hasn'?t|haven'?t|won'?t|can'?t|cannot|need(?:s|ed)? to
be)\b[\w\s]{0,15}\b(fixed|addressed|resolved)\b in RESOLUTION_NEGATION_PATTERNS,
and change looksLikeResolutionAck to require the body be short/terse (e.g.,
trimmed length < 120) or the ack phrase appear on the first line or immediately
after positive anchors like "this is", "should be", "now" before returning true;
ensure the function still trims and checks negations first and update any
callers (e.g., issueInventoryService.syncFromPrData usage) to rely on the
tightened heuristic.


/**
* Map ADE's permission mode to the agent chat permission mode.
* Shared by both the issue resolver and rebase resolver.
Expand Down
7 changes: 7 additions & 0 deletions apps/desktop/src/preload/global.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,10 @@ import type {
PrActionRun,
PrActivityEvent,
PrCheck,
PrCommit,
PrComment,
CleanupPrBranchArgs,
CleanupPrBranchResult,
PrConflictAnalysis,
PrDetail,
PrEventPayload,
Expand Down Expand Up @@ -1473,6 +1476,7 @@ declare global {
onEvent: (cb: (ev: PrEventPayload) => void) => () => void;
getDetail: (prId: string) => Promise<PrDetail>;
getFiles: (prId: string) => Promise<PrFile[]>;
getCommits: (prId: string) => Promise<PrCommit[]>;
getActionRuns: (prId: string) => Promise<PrActionRun[]>;
getActivity: (prId: string) => Promise<PrActivityEvent[]>;
addComment: (args: AddPrCommentArgs) => Promise<PrComment>;
Expand Down Expand Up @@ -1542,6 +1546,9 @@ declare global {
launchIssueResolutionFromThread: (
args: LaunchPrIssueResolutionFromThreadArgs,
) => Promise<LaunchPrIssueResolutionFromThreadResult>;
cleanupBranch: (
args: CleanupPrBranchArgs,
) => Promise<CleanupPrBranchResult>;
};
rebase: {
scanNeeds: () => Promise<RebaseNeed[]>;
Expand Down
Loading
Loading