Improve incremental review output and fix todo tracker race (#529)

* improve incremental review output and fix todo tracker race

- reviewed changes section: summarize at logical-change level with
  past-tense verbs, not per-file enumerations
- add TodoTracker.completeAll() to mark all non-cancelled items as
  completed before snapshotting the collapsible in review/progress posts

Made-with: Cursor

* completeAll -> completeInProgress: only mark in-progress items as completed

Pending items that were genuinely skipped stay as-is in the collapsible,
so the task list honestly reflects what the agent actually did.

Made-with: Cursor

entry

@@ -108715,6 +108715,7 @@ function ReportProgressTool(ctx) {
       if (!params.target_plan_comment && ctx.toolState.todoTracker) {
         ctx.toolState.todoTracker.cancel();
         await ctx.toolState.todoTracker.settled();
+        ctx.toolState.todoTracker.completeInProgress();
         const collapsible = ctx.toolState.todoTracker.renderCollapsible();
         if (collapsible) {
           body = `${body}
@@ -147452,6 +147453,7 @@ function CreatePullRequestReviewTool(ctx) {
       if (body && ctx.toolState.selectedMode === "Review" && ctx.toolState.todoTracker) {
         ctx.toolState.todoTracker.cancel();
         await ctx.toolState.todoTracker.settled();
+        ctx.toolState.todoTracker.completeInProgress();
         const collapsible = ctx.toolState.todoTracker.renderCollapsible();
         if (collapsible) {
           body = `${body}
@@ -148893,7 +148895,7 @@ ${learningsStep(t, 6)}`
 5. Self-critique: drop any comments that are praise, style preferences, speculative, about pre-existing code, or not actionable.
 
 6. **Summarize**: build two distinct sections for the review body:
-   a. **Reviewed changes**: one bullet per logical change in the new commits (e.g. \`- dispatch/route.ts: switched from parse() to safeParse with structured 400 responses\`). describe what each change does \u2014 these are the novel changes you are acknowledging as reviewed.
+   a. **Reviewed changes**: summarize at the logical-change level, not per-file. each bullet starts with a past-tense verb (e.g. \`- Extracted shared CLI runtime into a single module\`, \`- Renamed package to pullfrog\`). avoid file paths unless they add clarity. if the changes can be described in one sentence, use one sentence \u2014 no bullets needed.
    b. **Prior review feedback**: list only the prior review comments that WERE addressed by the new commits (\`- [x] safeParse instead of parse \u2014 addressed\`). omit unaddressed comments. a change can appear in both sections \u2014 described as a reviewed change AND acknowledged as addressed feedback.
    - no headings, no tables, no prose paragraphs in either section \u2014 just bullets
    - in some cases you may receive a complete diff for the whole pull request instead of an incremental one. when this happens, you will need to determine what changes have happened since Pullfrog's most recent review.
@@ -151194,6 +151196,11 @@ function createTodoTracker(onUpdate) {
     async settled() {
       await inflightPromise;
     },
+    completeInProgress() {
+      for (const item of state.values()) {
+        if (item.status === "in_progress") item.status = "completed";
+      }
+    },
     renderCollapsible() {
       if (state.size === 0) return "";
       const todos = Array.from(state.values());

mcp/comment.ts

@@ -386,6 +386,7 @@ export function ReportProgressTool(ctx: ToolContext) {
       if (!params.target_plan_comment && ctx.toolState.todoTracker) {
         ctx.toolState.todoTracker.cancel();
         await ctx.toolState.todoTracker.settled();
+        ctx.toolState.todoTracker.completeInProgress();
         const collapsible = ctx.toolState.todoTracker.renderCollapsible();
         if (collapsible) {
           body = `${body}\n\n${collapsible}`;

mcp/review.ts

@@ -87,6 +87,7 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) {
       if (body && ctx.toolState.selectedMode === "Review" && ctx.toolState.todoTracker) {
         ctx.toolState.todoTracker.cancel();
         await ctx.toolState.todoTracker.settled();
+        ctx.toolState.todoTracker.completeInProgress();
         const collapsible = ctx.toolState.todoTracker.renderCollapsible();
         if (collapsible) {
           body = `${body}\n\n${collapsible}`;

modes.ts

@@ -134,7 +134,7 @@ ${learningsStep(t, 6)}`,
 5. Self-critique: drop any comments that are praise, style preferences, speculative, about pre-existing code, or not actionable.
 
 6. **Summarize**: build two distinct sections for the review body:
-   a. **Reviewed changes**: one bullet per logical change in the new commits (e.g. \`- dispatch/route.ts: switched from parse() to safeParse with structured 400 responses\`). describe what each change does — these are the novel changes you are acknowledging as reviewed.
+   a. **Reviewed changes**: summarize at the logical-change level, not per-file. each bullet starts with a past-tense verb (e.g. \`- Extracted shared CLI runtime into a single module\`, \`- Renamed package to pullfrog\`). avoid file paths unless they add clarity. if the changes can be described in one sentence, use one sentence — no bullets needed.
    b. **Prior review feedback**: list only the prior review comments that WERE addressed by the new commits (\`- [x] safeParse instead of parse — addressed\`). omit unaddressed comments. a change can appear in both sections — described as a reviewed change AND acknowledged as addressed feedback.
    - no headings, no tables, no prose paragraphs in either section — just bullets
    - in some cases you may receive a complete diff for the whole pull request instead of an incremental one. when this happens, you will need to determine what changes have happened since Pullfrog's most recent review.

utils/todoTracking.ts

@@ -56,6 +56,8 @@ export type TodoTracker = {
   cancel: () => void;
   /** resolves when any in-flight onUpdate call completes */
   settled: () => Promise<void>;
+  /** mark in-progress items as completed (for final snapshot before review/progress post) */
+  completeInProgress: () => void;
   renderCollapsible: () => string;
   readonly enabled: boolean;
   /** true after the tracker has successfully called onUpdate at least once */
@@ -135,6 +137,12 @@ export function createTodoTracker(onUpdate: (body: string) => Promise<void>): To
       await inflightPromise;
     },
 
+    completeInProgress() {
+      for (const item of state.values()) {
+        if (item.status === "in_progress") item.status = "completed";
+      }
+    },
+
     renderCollapsible(): string {
       if (state.size === 0) return "";
       const todos = Array.from(state.values());