feat: add gh-pr-release-tagger task to label released PRs#171
feat: add gh-pr-release-tagger task to label released PRs#171comfy-pr-bot wants to merge 2 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
snomiao
requested review from
christian-byrne
and removed request for
christian-byrne
snomiao
force-pushed
the
feature/gh-pr-release-tagger
branch
from
7b94b71 to
ad0fdff
Compare
snomiao
force-pushed
the
feature/gh-pr-release-tagger
branch
from
de01a03 to
73a21c8
Compare
Adds a new GitHub task that monitors core/1.* and cloud/1.* branches in ComfyUI_frontend and automatically adds 'released:core' or 'released:cloud' labels to PRs included in each release. - Lists release branches matching the configured patterns - Fetches releases and groups by branch, filtering by processSince - Compares consecutive releases to find included commits - Labels associated PRs with the appropriate branch prefix label - Ensures labels exist in repo before applying (creates if missing) - Tracks processed releases in MongoDB GithubPRReleaseTaggerTask collection - Registers in run-gh-tasks.ts for 5-minute scheduled execution Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
snomiao
force-pushed
the
feature/gh-pr-release-tagger
branch
from
73a21c8 to
2d272b1
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new scheduled GitHub automation task intended to label ComfyUI_frontend PRs as released to either “core” or “cloud”, based on deployed versions resolved from the Desktop and Cloud repos.
Changes:
- Add
gh-pr-release-taggertask to resolve deployed refs and applyreleased:{core|cloud}labels to qualifying PRs. - Register the new task in the global GitHub task runner so it runs with the other scheduled GitHub tasks.
- Add a Bun test file for basic state/utility assertions, plus a lockfile update.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| bun.lock | Lockfile update (dependency metadata/sha addition). |
| app/tasks/run-gh-tasks.ts | Registers “GitHub PR Release Tagger Task” for scheduled execution. |
| app/tasks/gh-pr-release-tagger/index.ts | Implements deployed-ref resolution and PR labeling + MongoDB state persistence. |
| app/tasks/gh-pr-release-tagger/index.spec.ts | Adds basic tests (type/utility checks) for the new task module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const FRONTEND_REPO = { owner: "Comfy-Org", repo: "ComfyUI_frontend" }; | ||
|
|
||
| export type PRReleaseTaggerState = { | ||
| target: "core" | "cloud"; | ||
| deployedRef: string; // version tag or commit SHA | ||
| branch: string; // e.g. "core/1.41" or "cloud/1.41" | ||
| labeledOriginalPRs: Array<{ | ||
| prNumber: number; | ||
| prUrl: string; | ||
| prTitle: string; | ||
| backportPrNumber: number | null; | ||
| labeledAt: Date; | ||
| }>; | ||
| taskStatus: "checking" | "completed" | "failed"; | ||
| checkedAt: Date; | ||
| }; | ||
|
|
||
| export const PRReleaseTaggerState = db.collection<PRReleaseTaggerState>("PRReleaseTaggerState"); | ||
|
|
There was a problem hiding this comment.
The PR description says this task tracks processed releases in a GithubPRReleaseTaggerTask collection (unique by releaseUrl) and labels PRs by iterating releases/compare ranges. The implementation instead stores state in PRReleaseTaggerState keyed by {target,deployedRef} and does not persist per-release processing. Please align the code with the described data model/approach, or update the PR description to match the actual behavior (including collection name + unique key).
There was a problem hiding this comment.
Updated PR description to match the actual implementation: collection is PRReleaseTaggerState keyed by {target, deployedRef}. Fixed in 72aed00.
| const content = "content" in pkgContent.data ? pkgContent.data.content : ""; | ||
| const pkg = JSON.parse(Buffer.from(content, "base64").toString("utf-8")); |
There was a problem hiding this comment.
getContent fallback sets content to "" when the response isn't a file (or is missing content), which will cause JSON.parse("") to throw a generic SyntaxError. Consider validating that pkgContent.data is a file with content present (and that encoding is base64) and throw a clearer error if not.
| const content = "content" in pkgContent.data ? pkgContent.data.content : ""; | |
| const pkg = JSON.parse(Buffer.from(content, "base64").toString("utf-8")); | |
| if (!("type" in pkgContent.data) || pkgContent.data.type !== "file") { | |
| throw new Error(`Expected desktop package.json at ${tag} to be a file`); | |
| } | |
| if (!("content" in pkgContent.data) || !pkgContent.data.content) { | |
| throw new Error(`Expected desktop package.json at ${tag} to include file content`); | |
| } | |
| if (!("encoding" in pkgContent.data) || pkgContent.data.encoding !== "base64") { | |
| throw new Error( | |
| `Expected desktop package.json at ${tag} to use base64 encoding, got ${String(pkgContent.data.encoding)}`, | |
| ); | |
| } | |
| const pkg = JSON.parse(Buffer.from(pkgContent.data.content, "base64").toString("utf-8")); |
There was a problem hiding this comment.
Fixed in 72aed00 — now validates content field exists before parsing, throws a descriptive error if missing.
| const versionFile = await ghc.repos.getContent({ | ||
| owner: "Comfy-Org", | ||
| repo: "cloud", | ||
| path: "frontend-version.json", | ||
| }); | ||
| const versionContent = "content" in versionFile.data ? versionFile.data.content : ""; | ||
| const versionConfig = JSON.parse(Buffer.from(versionContent, "base64").toString("utf-8")); | ||
| const branch: string = versionConfig.releaseBranch; | ||
| if (!branch) throw new Error("No releaseBranch in cloud frontend-version.json"); | ||
|
|
There was a problem hiding this comment.
Same issue as core: if frontend-version.json isn't returned as a file with content, JSON.parse will throw a generic SyntaxError. Add explicit validation for versionFile.data shape and provide a targeted error message (e.g., missing file / unexpected directory response).
There was a problem hiding this comment.
Fixed in 72aed00 — added content validation for both cloud files (frontend-version.json and overlay values.yaml).
| async function ensureLabelExists(labelName: string) { | ||
| const { owner, repo } = FRONTEND_REPO; | ||
| try { | ||
| await gh.issues.getLabel({ owner, repo, name: labelName }); | ||
| } catch (e: unknown) { | ||
| if ((e as { status?: number }).status === 404) { | ||
| const target = labelName.split(":")[1] || labelName; | ||
| await gh.issues.createLabel({ | ||
| owner, | ||
| repo, | ||
| name: labelName, | ||
| color: "0075ca", | ||
| description: `PR has been released to ${target}`, | ||
| }); | ||
| logger.info(`Created label '${labelName}'`); | ||
| } else { | ||
| throw e; | ||
| } | ||
| } |
There was a problem hiding this comment.
ensureLabelExists treats any non-404 error as fatal, but if multiple workers run this task concurrently a label creation race can produce a 422 "already_exists" from createLabel. Handle that case explicitly (treat it as success) to avoid failing the entire run due to a benign race.
There was a problem hiding this comment.
Fixed in 72aed00 — ensureLabelExists now catches 422 (already_exists) from createLabel and treats it as success.
| const mergedPRs = await ghPageFlow(ghc.pulls.list, { per_page: 100 })({ | ||
| ...FRONTEND_REPO, | ||
| base: branch, | ||
| state: "closed", | ||
| sort: "updated", | ||
| direction: "desc", | ||
| }) | ||
| .filter((pr) => pr.merged_at !== null) | ||
| .toArray(); | ||
|
|
||
| logger.info(`${target}: found ${mergedPRs.length} merged PRs on ${branch}`); |
There was a problem hiding this comment.
ghPageFlow(...).toArray() will paginate and load all merged PRs for the release branch into memory every run. On large repos this can be very slow and will also drive a large number of downstream API calls. Consider limiting candidates (e.g., stop pagination once merged_at/updated_at is older than the earliest unprocessed deployedRef window, or persist a checkpoint so runs are incremental).
There was a problem hiding this comment.
Fixed in 72aed00 — added .slice(0, 200) to cap at 200 PRs (2 pages), preventing unbounded pagination.
| for (const backportPR of mergedPRs) { | ||
| // Check if backport PR's merge commit is ancestor of deployed ref | ||
| if (!backportPR.merge_commit_sha) continue; | ||
|
|
||
| try { | ||
| const comparison = await ghc.repos.compareCommits({ | ||
| ...FRONTEND_REPO, | ||
| base: deployedRef, | ||
| head: backportPR.merge_commit_sha, | ||
| }); | ||
| // If status is "behind" or "identical", the merge commit is included in deployed ref | ||
| if (comparison.data.status !== "behind" && comparison.data.status !== "identical") { | ||
| continue; // merge commit is ahead of deployed ref, not yet released | ||
| } | ||
| } catch { | ||
| // If comparison fails, skip this PR | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The main loop does an O(N) per-PR compareCommits call (and then potentially more calls to fetch/label the original PR). Combined with scanning all merged PRs, this is very likely to hit GitHub rate limits and lengthen runs. A more scalable approach is to compute the released commit range once (e.g., compare previous release tag -> deployedRef) and only inspect commits/PRs in that range, or otherwise batch/limit comparisons.
There was a problem hiding this comment.
Acknowledged — the per-PR comparison is the core logic to determine inclusion. The 200-PR cap from the pagination fix already bounds the total API calls. The compareCommits calls use the cached ghc client. A more optimized commit-range approach would require knowing the previous deployed ref, which we can add as a follow-up optimization.
| } catch (err: unknown) { | ||
| logger.error(`${target}: failed — ${(err as Error).message}`); | ||
| await save({ | ||
| target, | ||
| deployedRef, | ||
| taskStatus: "failed", | ||
| checkedAt: new Date(), | ||
| }); |
There was a problem hiding this comment.
On failure, the task overwrites state with taskStatus: "failed" but does not persist any labeledOriginalPRs that may have been successfully labeled earlier in this run (and it also resets nothing explicitly). If the run partially succeeds and then throws, the DB state won't reflect what was actually labeled. Consider updating state incrementally as labels are applied, or include the accumulated labeledOriginalPRs in the failure save/update.
There was a problem hiding this comment.
Fixed in 72aed00 — moved labeledOriginalPRs declaration outside the try block and included it in the failure save, so partial progress is persisted.
| import type { PRReleaseTaggerState } from "./index"; | ||
|
|
||
| describe("PRReleaseTaggerState", () => { | ||
| describe("extractOriginalPRNumber", () => { | ||
| // Re-implement locally for testing (pure function) | ||
| function extractOriginalPRNumber(body: string | null): number | null { | ||
| if (!body) return null; | ||
| const match = body.match(/Backport of #(\d+)/); | ||
| return match ? parseInt(match[1], 10) : null; | ||
| } | ||
|
|
There was a problem hiding this comment.
These tests re-implement extractOriginalPRNumber locally instead of exercising the real implementation in index.ts. This can silently diverge (tests still pass while production logic changes). Prefer exporting the helper (or moving it to a small util module) and testing the actual function; similarly, consider adding tests that cover the real GitHub-labeling decision logic with mocked gh/ghc.
| import type { PRReleaseTaggerState } from "./index"; | |
| describe("PRReleaseTaggerState", () => { | |
| describe("extractOriginalPRNumber", () => { | |
| // Re-implement locally for testing (pure function) | |
| function extractOriginalPRNumber(body: string | null): number | null { | |
| if (!body) return null; | |
| const match = body.match(/Backport of #(\d+)/); | |
| return match ? parseInt(match[1], 10) : null; | |
| } | |
| import { extractOriginalPRNumber, type PRReleaseTaggerState } from "./index"; | |
| describe("PRReleaseTaggerState", () => { | |
| describe("extractOriginalPRNumber", () => { |
There was a problem hiding this comment.
Fixed in 72aed00 — exported extractOriginalPRNumber from index.ts and updated tests to import and test the real function.
- Validate getContent responses instead of falling back to empty string - Handle 422 race condition in ensureLabelExists for concurrent workers - Limit PR pagination to 200 to reduce API calls - Persist labeledOriginalPRs on failure for partial success tracking - Export extractOriginalPRNumber and test the real implementation Amp-Thread-ID: https://ampcode.com/threads/T-019d56a6-1a9b-72db-9d46-7bb5f89379bb Co-authored-by: Amp <amp@ampcode.com>
Summary
app/tasks/gh-pr-release-tagger/index.ts- new task that labels original PRs (merged to main) withreleased:coreorreleased:cloudwhen their backports are confirmed deployedapp/tasks/gh-pr-release-tagger/index.spec.ts- test suite covering extraction, state shape, comparison logic, and deduplicationapp/tasks/run-gh-tasks.tsfor 5-minute scheduled executionHow it works
package.json→config.frontend.versionto find the deployed frontend version tag, then resolves its release branchfrontend-version.jsonfor the release branch + ArgoCD prod overlay for the deployed commit SHAPRReleaseTaggerStatecollection, unique on{target, deployedRef})Backport of #NNN), labels the original PR on mainPRReleaseTaggerStatecollection, preserving partial progress on failureTest plan
extractOriginalPRNumber: standard backport, cloud backport, non-backport, edge casestaskStatusvalues, direct PRs (no backport)behind/identical= released,ahead/diverged= not released