refactor progress comments into a bundled type + helper module (#567)
* refactor progress comments into a single bundled type + helper module
introduce ProgressComment ({ id, type: "issue" | "review" }) as the canonical handle for
the GitHub comment a run uses to report progress, and route every read/update/delete/create
through a single helper module (action/utils/progressComment.ts). previously every site that
touched the progress comment hardcoded octokit.rest.issues.*Comment, which made adding a
second comment type (review-thread replies) require duplicating the same branch in 6+ places
— the same shape that bit pullfrog/app#445.
new capability: when the address-reviews trigger fires for a one-off review comment, the
"Leaping into action" comment is now posted as a reply in that review thread instead of as
a top-level PR timeline comment. the helper handles failure (e.g. parent comment deleted)
by silently falling back to a top-level issue comment, so the run never loses its progress
surface.
changes:
- action/utils/progressComment.ts (new) — ProgressComment type + getProgressComment,
updateProgressComment, deleteProgressCommentApi, createLeapingProgressComment. uses a
structural Octokit interface to bridge the @octokit/rest version mismatch between the
action package (v22) and the root project (v21).
- action/internal/index.ts — re-export the new types and helpers for cross-boundary use.
- action/external.ts, action/utils/payload.ts — replace progressCommentId: string with
progressComment: { id: string, type: "issue" | "review" } in WriteablePayload + JsonPayload.
wire-format breaking, no legacy fallback (in-flight runs across the deploy lose their
progress comment, fine).
- action/mcp/server.ts — ToolState.progressCommentId becomes
progressComment: ProgressComment | null | undefined (same tristate semantics).
- action/main.ts, action/mcp/comment.ts, action/utils/errorReport.ts,
action/utils/postCleanup.ts — every issues.*Comment call against the progress comment
routes through the helper module. zero hardcoded API branching outside the helper.
- utils/github/triggerWorkflow.ts — drop createLeapingComment + updateCommentToLeaping;
dispatchAndTrackWorkflow gains a resolution chain (existingComment → replyToReviewComment
→ triggeringIssue → none) and an existingComment: ProgressComment param plus
replyToReviewComment: { pullNumber, commentId }.
- utils/webhooks/handleWebhook.ts — dispatch closure threads replyToReviewComment through;
the one-off review comment branch passes it and skips the now-redundant eyes reaction
on the comment we're about to reply to.
- app/trigger/[owner]/[repo]/[number]/page.tsx, utils/github/runActionLocal.ts,
app/api/cli/dispatch/route.ts, app/api/dispatch-workflow/route.ts — call sites updated to
new shape.
no schema or DB column changes. the existing WorkflowRun.progressCommentId column is still
written by id only; type lives only on the in-flight payload, which is sufficient for
runtime since it's the only thing that needs to know which API to call.
* anneal pass 1: fallback visibility + stale doc/comment updates
- progressComment.ts: when reviewReply→issue fallback fires, prepend a [!NOTE] callout
with a permalink back to the original review comment. without this, the parent comment
showed no eyes reaction (deliberately skipped) and no reply, leaving the user with no
signal that anything happened.
- wiki/post-cleanup.md: update progressCommentId references to progressComment, document
the new helper-based dispatch by type.
- wiki/main.md: update initToolState({ progressCommentId }) → ({ progressComment })
in the resolver-chain diagram.
- action/main.ts, action/mcp/review.ts: update two stale comments that referenced the
old field name.
* anneal pass 2: post-cleanup detection through fallback notice + log cleanup
- isLeapingIntoActionCommentBody: strip a leading GFM blockquote/alert before
testing the leaping prefix. without this, the [!NOTE] callout that the
reviewReply→issue fallback prepends would prevent post-cleanup from
recognizing the stuck "Leaping into action..." comment, leaving it permanently
on the PR timeline if the workflow died before any progress update.
- progressComment helper: switch from log.warning (action-flavored, emits a
::warning:: GitHub Actions annotation) to console.warn so the helper doesn't
pollute Vercel logs when invoked from the webhook context.
- triggerWorkflow.ts: drop the duplicate caller-side log on review-reply
failure — the helper already speaks loudly. Reword the catch-branch log to
reflect that it now only fires when both the reply AND the helper's internal
fallback failed.
- progressComment.ts: document that the [!NOTE] fallback notice is overwritten
on the first report_progress call, and explain the trade-off vs persisting
it through the action payload + ToolState.
* debloat: drop the [!NOTE] fallback callout
Reverting two pieces from the prior anneal pass:
- progressComment.ts: drop the [!NOTE] callout that the reviewReply→issue fallback
prepended to the leaping body. It disappeared on the agent's first report_progress
call, which made it half-committed to visibility — worse than either properly
persisting it (real engineering) or leaving the fallback silent (current choice).
The console.warn diagnostic and the workflow-run footer link in the leaping
comment itself give us enough signal for the rare case where both API endpoints
fail at once.
- isLeapingIntoActionCommentBody: revert the leading-blockquote stripping; only
needed to compensate for the [!NOTE] callout.
Keeping: the console.warn-vs-log.warning fix (real cross-runtime concern), the
duplicate-log drop in triggerWorkflow.ts, the wiki updates, and the two stale
source-comment fixes.
* fix: prevent stranded task list overwriting post-cleanup message
When a run is cancelled, the action's todoTracker may have an HTTP write in
flight to GitHub when SIGTERM lands. The action process dies, but the request
data has already left the socket — GitHub processes it and updates the comment
body to the (stale) task list. Meanwhile post-cleanup, running in a separate
process, writes the "This run was cancelled 🛑" message. If the tracker's
in-flight write happens to land *after* post-cleanup's write, the user never
sees the cancellation message.
Two-layer fix:
- Action side: cancel the tracker in the SIGTERM signal handler so no new
debounced writes get scheduled. This shrinks the race window but can't
un-send a request already on the wire.
- Post-cleanup side: after writing, verify the body landed and re-issue if
another write clobbered ours. Loops up to 3× with a 3s settle delay so
delayed in-flight writes from the dying action have time to arrive before
our read-back check decides whether to retry.
* lint: import createLeapingProgressComment from pullfrog/internal in test script
* address bot review findings: reply-target root, version bump, GET error handling
Three real findings from the bot reviews on #567 plus a small DRY pass:
1. handleWebhook reply-target: `newComments[0]` may be a reply, not a
top-level review comment. `getReviewCommentsWithReplies` returns root +
replies for any thread the review touched, and `pull_request_review_id`
filtering only narrows by *which review submitted*, not *root vs reply*.
When a user submits a single reply as their entire review (e.g. replying
to someone else's comment to ping @pullfrog), the reply ID flowed through
to `createReplyForReviewComment`, which 422s on replies-to-replies and
degraded to a top-level issue comment — exactly the polluted-PR-timeline
behavior this PR was built to remove. Walk up `in_reply_to` from the
already-fetched thread data to find the root and reply there instead.
2. action/package.json: bumped 0.0.202 → 0.0.204. main is at 0.0.203 and
our wire format changed; without a bump validateCompatibility can't
surface the mismatch on the deploy boundary, and the merge would have
gone backwards.
3. postCleanup writeAndVerify: distinguish a thrown verify-GET from a
"body got overwritten" mismatch. Treating a transient 5xx/rate-limit GET
the same as a clobber wasted PUT attempts and printed a misleading
"in-flight writes kept clobbering us" warning. We trust our PUT (which
returned 200) and exit instead of amplifying writes against a flaky API.
4. Small DRY: extracted parseProgressComment for the
`{ id: string; type } -> ProgressComment` parse that had drifted across
server.ts and postCleanup.ts.
This commit is contained in:
Colin McDonnell
committed by
pullfrog[bot]
pullfrog[bot]
parent
1e17a76863
commit
560e27bda5
@@ -0,0 +1,261 @@
|
||||
/**
|
||||
* Single source of truth for reading, updating, deleting, and creating "progress comments" —
|
||||
* the GitHub comments Pullfrog uses to surface a run's status.
|
||||
*
|
||||
* A progress comment can be one of two distinct GitHub entities with non-overlapping IDs and
|
||||
* distinct REST endpoints:
|
||||
* - "issue": a top-level issue/PR timeline comment (octokit.rest.issues.*Comment)
|
||||
* - "review": an inline PR review-thread comment (octokit.rest.pulls.*ReviewComment)
|
||||
*
|
||||
* Callers carry a `ProgressComment` (id + type) value end-to-end so the right endpoint is always
|
||||
* picked. Adding a third comment type later means one new branch in this file, not six.
|
||||
*/
|
||||
|
||||
export type ProgressCommentType = "issue" | "review";
|
||||
|
||||
export type ProgressComment = {
|
||||
id: number;
|
||||
type: ProgressCommentType;
|
||||
};
|
||||
|
||||
/**
|
||||
* Parse the on-the-wire `{ id: string; type }` shape (the form carried in `JsonPayload`)
|
||||
* into the in-memory `ProgressComment` shape. Returns undefined when the id isn't a
|
||||
* positive integer so callers can short-circuit cleanly. Callers handle logging.
|
||||
*/
|
||||
export function parseProgressComment(
|
||||
raw: { id: string; type: ProgressCommentType } | null | undefined
|
||||
): ProgressComment | undefined {
|
||||
if (!raw?.id) return undefined;
|
||||
const id = parseInt(raw.id, 10);
|
||||
if (Number.isNaN(id) || id <= 0) return undefined;
|
||||
return { id, type: raw.type };
|
||||
}
|
||||
|
||||
// minimal Octokit shape needed by the progress-comment helpers. structural so the helper
|
||||
// can be called from both the action package (@octokit/rest v22) and the root project
|
||||
// (@octokit/rest v21) without a nominal type clash. only the methods used here are listed.
|
||||
interface CommentResponse {
|
||||
data: { id: number; body?: string | null | undefined; html_url: string; node_id?: string };
|
||||
}
|
||||
export interface ProgressCommentOctokit {
|
||||
rest: {
|
||||
issues: {
|
||||
createComment: (params: {
|
||||
owner: string;
|
||||
repo: string;
|
||||
issue_number: number;
|
||||
body: string;
|
||||
}) => Promise<CommentResponse>;
|
||||
getComment: (params: {
|
||||
owner: string;
|
||||
repo: string;
|
||||
comment_id: number;
|
||||
}) => Promise<CommentResponse>;
|
||||
updateComment: (params: {
|
||||
owner: string;
|
||||
repo: string;
|
||||
comment_id: number;
|
||||
body: string;
|
||||
}) => Promise<CommentResponse>;
|
||||
deleteComment: (params: {
|
||||
owner: string;
|
||||
repo: string;
|
||||
comment_id: number;
|
||||
}) => Promise<unknown>;
|
||||
};
|
||||
pulls: {
|
||||
createReplyForReviewComment: (params: {
|
||||
owner: string;
|
||||
repo: string;
|
||||
pull_number: number;
|
||||
comment_id: number;
|
||||
body: string;
|
||||
}) => Promise<CommentResponse>;
|
||||
getReviewComment: (params: {
|
||||
owner: string;
|
||||
repo: string;
|
||||
comment_id: number;
|
||||
}) => Promise<CommentResponse>;
|
||||
updateReviewComment: (params: {
|
||||
owner: string;
|
||||
repo: string;
|
||||
comment_id: number;
|
||||
body: string;
|
||||
}) => Promise<CommentResponse>;
|
||||
deleteReviewComment: (params: {
|
||||
owner: string;
|
||||
repo: string;
|
||||
comment_id: number;
|
||||
}) => Promise<unknown>;
|
||||
};
|
||||
};
|
||||
}
|
||||
|
||||
interface ApiCtx {
|
||||
octokit: ProgressCommentOctokit;
|
||||
owner: string;
|
||||
repo: string;
|
||||
}
|
||||
|
||||
/**
|
||||
* Fetch a progress comment via the appropriate REST endpoint for its type.
|
||||
* Returns the common subset of fields callers actually use.
|
||||
*/
|
||||
export async function getProgressComment(
|
||||
ctx: ApiCtx,
|
||||
comment: ProgressComment
|
||||
): Promise<{ id: number; body: string | undefined; html_url: string }> {
|
||||
const result = await (comment.type === "review"
|
||||
? ctx.octokit.rest.pulls.getReviewComment({
|
||||
owner: ctx.owner,
|
||||
repo: ctx.repo,
|
||||
comment_id: comment.id,
|
||||
})
|
||||
: ctx.octokit.rest.issues.getComment({
|
||||
owner: ctx.owner,
|
||||
repo: ctx.repo,
|
||||
comment_id: comment.id,
|
||||
}));
|
||||
return {
|
||||
id: result.data.id,
|
||||
body: result.data.body ?? undefined,
|
||||
html_url: result.data.html_url,
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Update a progress comment in place via the appropriate REST endpoint.
|
||||
* Returns the common subset of fields callers actually use.
|
||||
*/
|
||||
export async function updateProgressComment(
|
||||
ctx: ApiCtx,
|
||||
comment: ProgressComment,
|
||||
body: string
|
||||
): Promise<{
|
||||
id: number;
|
||||
body: string | undefined;
|
||||
html_url: string;
|
||||
node_id: string | undefined;
|
||||
}> {
|
||||
const result = await (comment.type === "review"
|
||||
? ctx.octokit.rest.pulls.updateReviewComment({
|
||||
owner: ctx.owner,
|
||||
repo: ctx.repo,
|
||||
comment_id: comment.id,
|
||||
body,
|
||||
})
|
||||
: ctx.octokit.rest.issues.updateComment({
|
||||
owner: ctx.owner,
|
||||
repo: ctx.repo,
|
||||
comment_id: comment.id,
|
||||
body,
|
||||
}));
|
||||
return {
|
||||
id: result.data.id,
|
||||
body: result.data.body ?? undefined,
|
||||
html_url: result.data.html_url,
|
||||
node_id: result.data.node_id,
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Delete a progress comment via the appropriate REST endpoint.
|
||||
* Lower-level than `deleteProgressComment` in mcp/comment.ts — that one also clears
|
||||
* tool state. Callers that don't have a ToolContext (post cleanup, error handlers)
|
||||
* should use this directly; the higher-level wrapper delegates here.
|
||||
*/
|
||||
export async function deleteProgressCommentApi(
|
||||
ctx: ApiCtx,
|
||||
comment: ProgressComment
|
||||
): Promise<void> {
|
||||
if (comment.type === "review") {
|
||||
await ctx.octokit.rest.pulls.deleteReviewComment({
|
||||
owner: ctx.owner,
|
||||
repo: ctx.repo,
|
||||
comment_id: comment.id,
|
||||
});
|
||||
return;
|
||||
}
|
||||
await ctx.octokit.rest.issues.deleteComment({
|
||||
owner: ctx.owner,
|
||||
repo: ctx.repo,
|
||||
comment_id: comment.id,
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Discriminated target for `createLeapingProgressComment`. The two variants map to the two
|
||||
* distinct GitHub create endpoints; review-reply additionally needs the parent comment ID.
|
||||
*/
|
||||
export type CreateProgressCommentTarget =
|
||||
| { kind: "issue"; issueNumber: number }
|
||||
| { kind: "reviewReply"; pullNumber: number; replyToCommentId: number };
|
||||
|
||||
export interface CreatedProgressComment {
|
||||
comment: ProgressComment;
|
||||
body: string | undefined;
|
||||
html_url: string;
|
||||
}
|
||||
|
||||
/**
|
||||
* Create the initial "Leaping into action..." progress comment.
|
||||
*
|
||||
* Reliability: when `kind: "reviewReply"` fails (e.g. the parent comment was deleted or the
|
||||
* thread is otherwise unreachable), falls back to a top-level issue comment on the same PR
|
||||
* rather than leaving the run with no progress surface. The fallback is logged.
|
||||
*
|
||||
* (PR # === issue # in GitHub's number space, so `pullNumber` doubles as the fallback target.)
|
||||
*/
|
||||
export async function createLeapingProgressComment(
|
||||
ctx: ApiCtx,
|
||||
target: CreateProgressCommentTarget,
|
||||
body: string
|
||||
): Promise<CreatedProgressComment> {
|
||||
if (target.kind === "reviewReply") {
|
||||
try {
|
||||
const result = await ctx.octokit.rest.pulls.createReplyForReviewComment({
|
||||
owner: ctx.owner,
|
||||
repo: ctx.repo,
|
||||
pull_number: target.pullNumber,
|
||||
comment_id: target.replyToCommentId,
|
||||
body,
|
||||
});
|
||||
return {
|
||||
comment: { id: result.data.id, type: "review" },
|
||||
body: result.data.body ?? undefined,
|
||||
html_url: result.data.html_url,
|
||||
};
|
||||
} catch (error) {
|
||||
// console.warn (not the action-flavored log.warning) because this helper runs in
|
||||
// both the action runtime and the Next.js webhook context, and we don't want a
|
||||
// ::warning:: GitHub Actions annotation leaking into Vercel logs.
|
||||
console.warn(
|
||||
`[progressComment] review reply failed (parent ${target.replyToCommentId} on PR #${target.pullNumber}), falling back to issue comment:`,
|
||||
error
|
||||
);
|
||||
const fallback = await ctx.octokit.rest.issues.createComment({
|
||||
owner: ctx.owner,
|
||||
repo: ctx.repo,
|
||||
issue_number: target.pullNumber,
|
||||
body,
|
||||
});
|
||||
return {
|
||||
comment: { id: fallback.data.id, type: "issue" },
|
||||
body: fallback.data.body ?? undefined,
|
||||
html_url: fallback.data.html_url,
|
||||
};
|
||||
}
|
||||
}
|
||||
const result = await ctx.octokit.rest.issues.createComment({
|
||||
owner: ctx.owner,
|
||||
repo: ctx.repo,
|
||||
issue_number: target.issueNumber,
|
||||
body,
|
||||
});
|
||||
return {
|
||||
comment: { id: result.data.id, type: "issue" },
|
||||
body: result.data.body ?? undefined,
|
||||
html_url: result.data.html_url,
|
||||
};
|
||||
}
|
||||
Reference in New Issue
Block a user