From 8fa3858e97d1ceb2518aef08c63936e3b472ada7 Mon Sep 17 00:00:00 2001 From: Luke Date: Thu, 9 Jul 2026 13:27:10 -0700 Subject: [PATCH 1/4] Post an explanatory comment when a PR has only skippable files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a PR contained only files DiffSentry skips (a minified bundle, lockfile, or generated output), the review path hit the empty-files early return: it set a green commit status and returned without posting anything, leaving the "…reviewing… hang tight" status comment on the PR forever. getPRContext now captures the filenames dropped by the operator ignore list into a new PRContext.ignoredFiles field instead of discarding them. The empty-files branch uses that (plus path-filter and trivial/empty patches) to distinguish "only skippable files" from a genuinely empty diff, and in the former case upserts a status comment naming the skipped files grouped by reason. Because it upserts on STATUS_MARKER, this also resolves the stale in-review comment. Genuinely empty diffs and incremental no-ops (filesSkippedSimilar) stay silent — status only. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/github.ts | 10 +++++++++- src/reviewer.ts | 39 +++++++++++++++++++++++++++++++++++++++ src/types.ts | 7 +++++++ 3 files changed, 55 insertions(+), 1 deletion(-) diff --git a/src/github.ts b/src/github.ts index 0f1cc14..dcefd63 100644 --- a/src/github.ts +++ b/src/github.ts @@ -422,8 +422,15 @@ export class GitHubClient { ]); const fileCap = maxFiles != null && maxFiles > 0 ? maxFiles : this.config.maxFilesPerReview; + const ignoredFiles: string[] = []; const files: FileChange[] = filesResponse.data - .filter((f) => !this.isIgnored(f.filename)) + .filter((f) => { + if (this.isIgnored(f.filename)) { + ignoredFiles.push(f.filename); + return false; + } + return true; + }) .slice(0, fileCap) .map((f) => ({ filename: f.filename, @@ -445,6 +452,7 @@ export class GitHubClient { headSha: pr.data.head.sha, defaultBranch: pr.data.base.repo.default_branch, files, + ignoredFiles, isDraft: pr.data.draft, labels: pr.data.labels.map((l) => l.name ?? ""), author: pr.data.user?.login, diff --git a/src/reviewer.ts b/src/reviewer.ts index f3ffa6e..ea1878d 100644 --- a/src/reviewer.ts +++ b/src/reviewer.ts @@ -583,6 +583,45 @@ export class Reviewer { if (context.files.length === 0) { log.info("No reviewable files in PR, skipping"); + + // Distinguish "PR only contains files we deliberately skip (a minified + // bundle, lockfile, generated output)" from a genuinely empty diff. In + // the former case, post an info comment naming the skipped files so the + // PR doesn't just show a green check with no explanation — and so the + // "…reviewing… hang tight" status comment gets resolved rather than + // left hanging forever. `filesSkippedSimilar` is excluded on purpose: + // in incremental mode it means nothing new changed (a true no-op). + const skippedGroups: Array<{ label: string; files: string[] }> = [ + { label: "Ignored (minified, generated, or lockfile)", files: context.ignoredFiles ?? [] }, + { label: "Excluded by path filters", files: filesIgnoredByPathFilter.map((f) => f.path) }, + { label: "No reviewable changes", files: filesSkippedTrivial }, + ].filter((g) => g.files.length > 0); + + if (skippedGroups.length > 0) { + const totalSkipped = skippedGroups.reduce((n, g) => n + g.files.length, 0); + const body = + STATUS_MARKER + "\n" + + `> :next_track_button: **DiffSentry** — no reviewable changes. ` + + `This ${mode === "full" ? "pull request" : "update"} only contains ` + + `${totalSkipped} file${totalSkipped === 1 ? "" : "s"} DiffSentry skips, ` + + `so there's nothing to review.\n\n` + + skippedGroups + .map((g) => + `
${g.label} (${g.files.length})\n\n` + + g.files.map((f) => `- \`${f}\``).join("\n") + + `\n
`, + ) + .join("\n\n"); + try { + await this.github.upsertComment( + installationId, owner, repo, pullNumber, body, STATUS_MARKER, signal + ); + log.info({ totalSkipped }, "Posted no-reviewable-files status comment"); + } catch (err) { + log.warn({ err }, "Failed to post no-reviewable-files status comment"); + } + } + if (repoConfig.reviews?.commit_status !== false) { await this.github.setCommitStatus( installationId, owner, repo, context.headSha, diff --git a/src/types.ts b/src/types.ts index 09173cb..9faf7a2 100644 --- a/src/types.ts +++ b/src/types.ts @@ -385,6 +385,13 @@ export interface PRContext { /** Repo default branch (e.g. "main"). Authoritative source for .diffsentry.yaml. */ defaultBranch?: string; files: FileChange[]; + /** + * Filenames dropped by the operator-level ignore list (`ignoredPatterns` — + * e.g. *.min.js, *.map, dist/**, lockfiles) before `files` was built. Kept so + * the reviewer can tell "PR only contains ignored files (minified bundle)" + * apart from a genuinely empty diff and surface which files were skipped. + */ + ignoredFiles?: string[]; isDraft?: boolean; labels?: string[]; author?: string; From 8073c7882e6695fe16ab272d275ae254054b0127 Mon Sep 17 00:00:00 2001 From: Luke Date: Thu, 9 Jul 2026 13:38:17 -0700 Subject: [PATCH 2/4] Label upsertComment logs by comment type; cap skipped-file lists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes from reviewing PR #77's own review logs: - upsertComment hardcoded "Created/Updated ... walkthrough comment" even though it backs the status, walkthrough, sticky and issue-summary comments alike. Distinct comments therefore looked like duplicate walkthroughs in the logs. Derive a label from the marker and log the created comment id too. - Cap each skipped-file group in the no-reviewable-files status comment at 50 entries with an "…and N more" overflow line, so a monorepo PR touching hundreds of lockfiles/dist artifacts can't exceed GitHub's ~65 KB comment limit and make the upsert throw — which would leave the stale "reviewing…" comment this branch exists to resolve. (Addresses DiffSentry finding on #77.) Co-Authored-By: Claude Opus 4.8 (1M context) --- src/github.ts | 13 ++++++++++--- src/reviewer.ts | 21 ++++++++++++++++----- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/src/github.ts b/src/github.ts index dcefd63..fe923d5 100644 --- a/src/github.ts +++ b/src/github.ts @@ -779,6 +779,13 @@ export class GitHubClient { (c) => c.user?.type === "Bot" && c.body?.includes(marker) ); + // upsertComment is generic — it backs the status, walkthrough, sticky, + // issue-summary and pre-merge comments alike. Derive a label from the + // marker so the log names the actual comment type instead of always saying + // "walkthrough comment", which made distinct comments look like duplicate + // walkthroughs in the logs. + const commentLabel = marker.replace(//, "").trim() || marker; + if (existing) { await octokit.issues.updateComment({ owner, @@ -786,15 +793,15 @@ export class GitHubClient { comment_id: existing.id, body, }); - log.info({ commentId: existing.id }, "Updated existing walkthrough comment"); + log.info({ commentId: existing.id, comment: commentLabel }, "Updated existing comment"); } else { - await octokit.issues.createComment({ + const created = await octokit.issues.createComment({ owner, repo, issue_number: pullNumber, body, }); - log.info("Created new walkthrough comment"); + log.info({ commentId: created.data.id, comment: commentLabel }, "Created new comment"); } } diff --git a/src/reviewer.ts b/src/reviewer.ts index ea1878d..8096791 100644 --- a/src/reviewer.ts +++ b/src/reviewer.ts @@ -599,6 +599,12 @@ export class Reviewer { if (skippedGroups.length > 0) { const totalSkipped = skippedGroups.reduce((n, g) => n + g.files.length, 0); + // Cap each group's rendered list: a monorepo PR touching hundreds of + // lockfiles / dist artifacts could otherwise blow past GitHub's ~65 KB + // comment-body limit, making the upsert throw — which would leave the + // stale "reviewing…" comment in place, the very thing this branch exists + // to resolve. Overflow is summarised as "…and N more". + const MAX_FILES_PER_GROUP = 50; const body = STATUS_MARKER + "\n" + `> :next_track_button: **DiffSentry** — no reviewable changes. ` + @@ -606,11 +612,16 @@ export class Reviewer { `${totalSkipped} file${totalSkipped === 1 ? "" : "s"} DiffSentry skips, ` + `so there's nothing to review.\n\n` + skippedGroups - .map((g) => - `
${g.label} (${g.files.length})\n\n` + - g.files.map((f) => `- \`${f}\``).join("\n") + - `\n
`, - ) + .map((g) => { + const shown = g.files.slice(0, MAX_FILES_PER_GROUP); + const overflow = g.files.length - shown.length; + return ( + `
${g.label} (${g.files.length})\n\n` + + shown.map((f) => `- \`${f}\``).join("\n") + + (overflow > 0 ? `\n- …and ${overflow} more` : "") + + `\n
` + ); + }) .join("\n\n"); try { await this.github.upsertComment( From 312bfc212ce91ada395e136442ede6953a45c707 Mon Sep 17 00:00:00 2001 From: Luke Date: Thu, 9 Jul 2026 13:46:52 -0700 Subject: [PATCH 3/4] =?UTF-8?q?Use=20literal=20=E2=8F=AD=EF=B8=8F=20glyph?= =?UTF-8?q?=20and=20make=20create-comment=20log=20throw-safe?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address two DiffSentry findings on #77: - Replace the :next_track_button: emoji shortcode in the no-reviewable-files status comment with the literal ⏭️ glyph, which is guaranteed to render on GitHub and matches the PR-description example. - Guard the new "Created new comment" log with optional chaining (created?.data?.id) so an unexpected createComment response shape can't turn a successful write into a thrown TypeError from a log line. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/github.ts | 2 +- src/reviewer.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/github.ts b/src/github.ts index fe923d5..cb3ef85 100644 --- a/src/github.ts +++ b/src/github.ts @@ -801,7 +801,7 @@ export class GitHubClient { issue_number: pullNumber, body, }); - log.info({ commentId: created.data.id, comment: commentLabel }, "Created new comment"); + log.info({ commentId: created?.data?.id, comment: commentLabel }, "Created new comment"); } } diff --git a/src/reviewer.ts b/src/reviewer.ts index 8096791..9c99201 100644 --- a/src/reviewer.ts +++ b/src/reviewer.ts @@ -607,7 +607,7 @@ export class Reviewer { const MAX_FILES_PER_GROUP = 50; const body = STATUS_MARKER + "\n" + - `> :next_track_button: **DiffSentry** — no reviewable changes. ` + + `> ⏭️ **DiffSentry** — no reviewable changes. ` + `This ${mode === "full" ? "pull request" : "update"} only contains ` + `${totalSkipped} file${totalSkipped === 1 ? "" : "s"} DiffSentry skips, ` + `so there's nothing to review.\n\n` + From cc66f4bf5f9664b8fc39fca4c137bacf24f93cda Mon Sep 17 00:00:00 2001 From: Luke Date: Thu, 9 Jul 2026 14:05:08 -0700 Subject: [PATCH 4/4] fix: surface files dropped by the max-files-per-review cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address DiffSentry findings on #77: files removed by the maxFilesPerReview cap (the post-ignore overflow beyond .slice(0, fileCap)) were recorded nowhere, so a >cap PR whose remaining reviewable files were all trivial/ similar could still hit the silent "no reviewable files" green-status path — the exact failure mode this PR exists to fix, via a different drop reason. getPRContext now captures that overflow into PRContext.cappedFiles, and the empty-files branch adds a "Beyond the max-files-per-review cap" skipped group so the explanatory status comment fires (and clears the hang-tight comment) whenever the PR had files and every one was deliberately dropped. Report-only: capped files remain unreviewed (the cap is intentional), they are just no longer silently discarded. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/github.ts | 22 ++++++++++++++-------- src/reviewer.ts | 1 + src/types.ts | 7 +++++++ 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/github.ts b/src/github.ts index cb3ef85..0be4189 100644 --- a/src/github.ts +++ b/src/github.ts @@ -423,14 +423,19 @@ export class GitHubClient { const fileCap = maxFiles != null && maxFiles > 0 ? maxFiles : this.config.maxFilesPerReview; const ignoredFiles: string[] = []; - const files: FileChange[] = filesResponse.data - .filter((f) => { - if (this.isIgnored(f.filename)) { - ignoredFiles.push(f.filename); - return false; - } - return true; - }) + const afterIgnore = filesResponse.data.filter((f) => { + if (this.isIgnored(f.filename)) { + ignoredFiles.push(f.filename); + return false; + } + return true; + }); + // Files beyond the max-files cap are dropped from review. Record them (like + // ignoredFiles) so the reviewer can surface "N files beyond the review cap" + // instead of silently returning a green status when the cap is the only + // reason nothing was reviewed. + const cappedFiles: string[] = afterIgnore.slice(fileCap).map((f) => f.filename); + const files: FileChange[] = afterIgnore .slice(0, fileCap) .map((f) => ({ filename: f.filename, @@ -453,6 +458,7 @@ export class GitHubClient { defaultBranch: pr.data.base.repo.default_branch, files, ignoredFiles, + cappedFiles, isDraft: pr.data.draft, labels: pr.data.labels.map((l) => l.name ?? ""), author: pr.data.user?.login, diff --git a/src/reviewer.ts b/src/reviewer.ts index 9c99201..c3e8095 100644 --- a/src/reviewer.ts +++ b/src/reviewer.ts @@ -594,6 +594,7 @@ export class Reviewer { const skippedGroups: Array<{ label: string; files: string[] }> = [ { label: "Ignored (minified, generated, or lockfile)", files: context.ignoredFiles ?? [] }, { label: "Excluded by path filters", files: filesIgnoredByPathFilter.map((f) => f.path) }, + { label: "Beyond the max-files-per-review cap", files: context.cappedFiles ?? [] }, { label: "No reviewable changes", files: filesSkippedTrivial }, ].filter((g) => g.files.length > 0); diff --git a/src/types.ts b/src/types.ts index 9faf7a2..b7658f8 100644 --- a/src/types.ts +++ b/src/types.ts @@ -392,6 +392,13 @@ export interface PRContext { * apart from a genuinely empty diff and surface which files were skipped. */ ignoredFiles?: string[]; + /** + * Filenames dropped by the `maxFilesPerReview` cap — the post-`ignoredFiles` + * overflow beyond `.slice(0, fileCap)`. Recorded so the empty-files status + * comment can explain "N files beyond the review cap" rather than silently + * returning when the cap is the only reason nothing was reviewed. + */ + cappedFiles?: string[]; isDraft?: boolean; labels?: string[]; author?: string;