From 323841328a2eacb9c4834c0b8943afcd879586c8 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 19 Oct 2026 09:25:04 -0400 Subject: [PATCH 1/4] feat(pr-pipeline): work a reviewer thread a trusted writer asked to fix address-feedback's threadAuthors now also accepts a thread the lanes App never commented on when a trusted person with write access replies to the reviewer bot's finding with a fix request. Every other comment must still be the reviewer bot's or a trusted person's, under the same edit rules. The select step, the reply job's re-read and the dispatcher compute the write-access fix requesters on every PR, not only human-gated ones. The dispatcher trigger is unchanged: a fix request still waits for the next completed review run. Refs: #8031 Co-Authored-By: Claude Opus 5.5 --- ...s-feedback-trusted-fix-request-57421cca.md | 13 ++++ .../scripts/pr-unblock/address-feedback.mjs | 76 +++++++++++++------ .../pr-unblock/address-feedback.test.mjs | 51 +++++++++++++ .../scripts/pr-unblock/feedback-dispatch.mjs | 28 +++---- .../pr-unblock/feedback-dispatch.test.mjs | 37 +++++++++ docs/conventions/pr-pipeline/README.md | 18 +++-- .../pr-pipeline/pr-run-activity.md | 25 +++--- 7 files changed, 194 insertions(+), 54 deletions(-) create mode 100644 .changes/pr-pipeline/fix-address-feedback-trusted-fix-request-57421cca.md diff --git a/.changes/pr-pipeline/fix-address-feedback-trusted-fix-request-57421cca.md b/.changes/pr-pipeline/fix-address-feedback-trusted-fix-request-57421cca.md new file mode 100644 index 0000000000..001fe99f32 --- /dev/null +++ b/.changes/pr-pipeline/fix-address-feedback-trusted-fix-request-57421cca.md @@ -0,0 +1,13 @@ +--- +bump: minor +--- + +### Changed + +- `address-feedback` now also works a review thread the lanes App never commented on when a + trusted person with write or admin permission replies to the reviewer bot's finding with a fix + request (`PR-fix lane: please fix this finding`); every other comment must still be the reviewer + bot's or a trusted person's. The select step, the reply job's re-read and + `pr-feedback-dispatch.yml` judge threads the same way + ([README.md](https://github.com/melodic-software/claude-code-plugins/blob/main/docs/conventions/pr-pipeline/README.md)). + The dispatcher still starts only when a review run completes. `version` stays 2. diff --git a/.github/scripts/pr-unblock/address-feedback.mjs b/.github/scripts/pr-unblock/address-feedback.mjs index 875ab0f146..16a02fe4ff 100644 --- a/.github/scripts/pr-unblock/address-feedback.mjs +++ b/.github/scripts/pr-unblock/address-feedback.mjs @@ -6,7 +6,8 @@ // `prepare: review-threads` skill, before the head checkout and any // model turn, with the job's read-only token: keeps the unresolved // review threads the reviewer bot opened in a review on a head a -// review lane's marker confirms, when every comment is the bot's or +// review lane's marker confirms, when every comment is the bot's, +// a trusted person with write access asked the lane to fix it, or // a trusted person answered the lane's escalation, then decides // skip (no such thread), escalate (the PR changes .github/**, or // the configured number of rounds already ran) or proceed, writing the thread file @@ -121,6 +122,14 @@ export async function fixRequesters({ comments, trustedHumans, permission }) { return writers; } +// fixRequesters over a PR's review comments (the REST rows). +export const reviewFixers = ({ reviewComments, trustedHumans = new Set(), permission }) => + fixRequesters({ + comments: reviewComments.map((row) => ({ authorId: row.user?.id, login: row.user?.login, body: row.body })), + trustedHumans, + permission, + }); + // The base's human-gated role label: config.role_labels["human-gated"] of // .work-item-tracker.json, needs-human when the file or entry is absent. export function humanGatedLabelFrom(path) { @@ -146,35 +155,60 @@ export function humanGatedLabelFrom(path) { // Who wrote each comment of a thread, or null when it is not one to work. // `comments` are oldest first, each { authorId, createdAt, body, lastEditedAt, -// editorId }. The first is the reviewer bot's, and either every comment is, -// or the lanes App's last comment escalated the thread and every comment after -// it is a trusted person's reply. Those replies are direction. A comment by -// anyone else, a trusted person's comment last edited by anyone but a trusted -// person, or a reply not after the escalation leaves the thread alone. The -// select step and the reply job (keepThreads) and the dispatcher +// editorId }. The first is the reviewer bot's, and one of these holds: +// - every comment is the reviewer bot's; +// - the lanes App never commented, every later comment is the reviewer bot's +// or a trusted person's, and one is a fix request (isFixRequest) by one of +// `fixers` (the trusted people with write access who wrote a fix request, +// fixRequesters); the trusted people's comments are direction; +// - the lanes App's last comment escalated the thread and every comment after +// it is a trusted person's reply, which is direction. +// A comment by anyone else, a trusted person's comment last edited by anyone +// but a trusted person, or a reply not after the escalation leaves the thread +// alone. The select step and the reply job (keepThreads) and the dispatcher // (feedback-dispatch.mjs) all judge a thread with this. With `requesters` (a // set of ids, given on a PR carrying the human-gated label), a thread is -// worked only when a reply after the escalation is a fix request -// (isFixRequest) by one of them. -export function threadAuthors(comments, { reviewerId, appId, trustedHumans = new Set(), requesters = null }) { +// worked only when a reply is a fix request by one of them, after the +// escalation when there is one. +export function threadAuthors(comments, { reviewerId, appId, trustedHumans = new Set(), requesters = null, fixers = requesters ?? new Set() }) { if (comments.length === 0 || comments[0].authorId !== reviewerId) { return null; } if (comments.every((comment) => comment.authorId === reviewerId)) { return requesters ? null : comments.map(() => ({ author: "reviewer" })); } + const isHuman = (comment) => + trustedHumans.has(comment.authorId) && (comment.lastEditedAt == null || trustedHumans.has(comment.editorId)); + const asks = (ids, comment) => ids.has(comment.authorId) && isFixRequest(comment.body); const last = comments.findLastIndex((comment) => comment.authorId === appId); + if (last === -1) { + const humanAsks = (ids) => comments.some((comment) => isHuman(comment) && asks(ids, comment)); + if (!humanAsks(fixers) || (requesters && !humanAsks(requesters))) { + return null; + } + const authors = []; + for (const comment of comments) { + if (comment.authorId === reviewerId) { + authors.push({ author: "reviewer" }); + } else if (isHuman(comment)) { + authors.push({ author: "trusted-human", direction: true }); + } else { + return null; + } + } + return authors; + } if (last < 1 || !ESCALATION_REPLY.test(firstLine(comments[last].body)) || last === comments.length - 1) { return null; } - if (requesters && !comments.slice(last + 1).some((comment) => requesters.has(comment.authorId) && isFixRequest(comment.body))) { + if (requesters && !comments.slice(last + 1).some((comment) => asks(requesters, comment))) { return null; } const escalatedAt = Date.parse(comments[last].createdAt ?? ""); const authors = []; for (const [i, comment] of comments.entries()) { const id = comment.authorId; - const human = trustedHumans.has(id) && (comment.lastEditedAt == null || trustedHumans.has(comment.editorId)); + const human = isHuman(comment); if (i > last) { // NaN compares false, so an unparseable time leaves the thread alone. if (!human || !(Date.parse(comment.createdAt ?? "") > escalatedAt)) { @@ -194,7 +228,7 @@ export function threadAuthors(comments, { reviewerId, appId, trustedHumans = new // review the reviewer bot submitted on a marker-confirmed head, and // threadAuthors accepts its comments. A thread with more comments than one // page reads is left alone. -export function keepThreads({ threads, reviewComments, reviews, heads, reviewerId, appId, trustedHumans = new Set(), requesters = null }) { +export function keepThreads({ threads, reviewComments, reviews, heads, reviewerId, appId, trustedHumans = new Set(), requesters = null, fixers = requesters ?? new Set() }) { const comments = new Map(reviewComments.map((comment) => [comment.id, comment])); const reviewsById = new Map(reviews.map((review) => [review.id, review])); const kept = []; @@ -215,7 +249,7 @@ export function keepThreads({ threads, reviewComments, reviews, heads, reviewerI lastEditedAt: nodes[i].lastEditedAt, editorId: nodes[i].editor?.databaseId, })), - { reviewerId, appId, trustedHumans, requesters }, + { reviewerId, appId, trustedHumans, requesters, fixers }, ); const review = reviewsById.get(rows[0].pull_request_review_id); if (authors === null || review?.user?.id !== reviewerId || !heads.has(review.commit_id)) { @@ -256,13 +290,7 @@ export async function select({ // The human-gated label holds the PR except for threads a trusted person // with write access asked the lane to fix. const gated = (pull.labels ?? []).some((label) => label.name === humanGatedLabel); - const requesters = gated - ? await fixRequesters({ - comments: reviewComments.map((row) => ({ authorId: row.user?.id, login: row.user?.login, body: row.body })), - trustedHumans, - permission: api.getPermission, - }) - : null; + const fixers = await reviewFixers({ reviewComments, trustedHumans, permission: api.getPermission }); const kept = keepThreads({ threads, reviewComments, @@ -271,7 +299,8 @@ export async function select({ reviewerId, appId, trustedHumans, - requesters, + requesters: gated ? fixers : null, + fixers, }); if (kept.length === 0) { return { decision: "skip", skipReason: gated ? "awaiting-human" : "not-applicable", threads: kept }; @@ -307,9 +336,10 @@ export async function currentThreads({ api, pr, reviewerId, appId, trustedHumans api.listReviewComments(pr), api.listThreads(pr), ]); + const fixers = await reviewFixers({ reviewComments, trustedHumans, permission: api.getPermission }); return { issueComments, - kept: keepThreads({ threads, reviewComments, reviews, heads: reviewedHeads(issueComments), reviewerId, appId, trustedHumans }), + kept: keepThreads({ threads, reviewComments, reviews, heads: reviewedHeads(issueComments), reviewerId, appId, trustedHumans, fixers }), }; } diff --git a/.github/scripts/pr-unblock/address-feedback.test.mjs b/.github/scripts/pr-unblock/address-feedback.test.mjs index 3185b764b8..0fafe13e49 100644 --- a/.github/scripts/pr-unblock/address-feedback.test.mjs +++ b/.github/scripts/pr-unblock/address-feedback.test.mjs @@ -248,6 +248,57 @@ test("without needs-human nothing changes and no permission is read", async () = assert.deepEqual(api.permissionReads, []); }); +// The thread on #8031: the reviewer bot's finding, then the owner's fix +// request, with no reply from the lanes App. +function asked(replies, edits = {}) { + const rows = [[100, REVIEWER, "finding 100"], ...replies]; + return { + reviewComments: rows.map(([id, user, body], i) => comment(id, user, { body, created_at: at(i) })), + threads: [ + { + id: "PRRT_1", + isResolved: false, + comments: { + totalCount: rows.length, + nodes: rows.map(([id]) => ({ databaseId: id, lastEditedAt: edits[id] ? at(30) : null, editor: edits[id] ? { databaseId: edits[id] } : null })), + }, + }, + ], + }; +} + +test("with no escalation, a write-access trusted person's fix request is kept as direction", async () => { + const api = fakeApi(asked([[101, HUMAN, REQUEST]])); + const result = await run(api); + assert.equal(result.decision, "proceed"); + assert.equal(result.fixRequested, false); + assert.deepEqual(result.file.threads[0].comments, [ + { id: 100, author: "reviewer", body: "finding 100" }, + { id: 101, author: "trusted-human", direction: true, body: REQUEST }, + ]); + assert.deepEqual(api.permissionReads, ["kyle-sexton"]); +}); + +test("with no escalation, on a needs-human PR, a write-access trusted person's fix request is kept", async () => { + const result = await run(fakeApi({ ...asked([[101, HUMAN, REQUEST]]), labels: ["needs-human"] })); + assert.equal(result.decision, "proceed"); + assert.equal(result.fixRequested, true); +}); + +for (const [label, overrides] of [ + ["an untrusted person's fix request", { ...asked([[101, STRANGER, REQUEST]]), permissions: { stranger: "admin" } }], + ["a trusted person's fix request without write access", { ...asked([[101, HUMAN, REQUEST]]), permissions: { "kyle-sexton": "read" } }], + ["a trusted person's reply that asks no fix", asked([[101, HUMAN, DECISION]])], + ["a fix request last edited by a stranger", asked([[101, HUMAN, REQUEST]], { 101: STRANGER })], + ["a stranger's comment beside a trusted fix request", asked([[101, HUMAN, REQUEST], [102, STRANGER, INJECTION]])], +]) { + test(`with no escalation, ${label} is not kept`, async () => { + const result = await run(fakeApi(overrides)); + assert.equal(result.decision, "skip"); + assert.equal(result.skipReason, "not-applicable"); + }); +} + test("trustedHumansFrom reads only the people on the list", (t) => { const dir = mkdtempSync(join(tmpdir(), "address-feedback-")); t.after(() => rmSync(dir, { recursive: true, force: true })); diff --git a/.github/scripts/pr-unblock/feedback-dispatch.mjs b/.github/scripts/pr-unblock/feedback-dispatch.mjs index f4e297f923..6320105d60 100644 --- a/.github/scripts/pr-unblock/feedback-dispatch.mjs +++ b/.github/scripts/pr-unblock/feedback-dispatch.mjs @@ -19,7 +19,8 @@ // - both review lanes' newest reviewed-head markers, written by // github-actions[bot], name that head; // - an unresolved review thread exists that address-feedback would work: -// every comment is the reviewer bot's, or a trusted person answered the +// every comment is the reviewer bot's, a trusted person with write access +// asked the lane to fix it (isFixRequest), or a trusted person answered the // lane's escalation (address-feedback.mjs threadAuthors); on a PR carrying // the human-gated label, only a thread where a trusted person with write // access asked the lane to fix it (isFixRequest); @@ -85,7 +86,7 @@ export function markedHead(comments, lane, markerAuthorId = DEFAULTS.markerAutho } // An unresolved thread, read in full, that threadAuthors accepts. -export function hasWorkableThread(threads, { reviewerId, appId, trustedHumans, requesters = null }) { +export function hasWorkableThread(threads, { reviewerId, appId, trustedHumans, requesters = null, fixers = requesters ?? new Set() }) { return threads.some( (thread) => thread.isResolved === false && @@ -98,7 +99,7 @@ export function hasWorkableThread(threads, { reviewerId, appId, trustedHumans, r lastEditedAt: node.lastEditedAt, editorId: node.editor?.databaseId, })), - { reviewerId, appId, trustedHumans, requesters }, + { reviewerId, appId, trustedHumans, requesters, fixers }, ) !== null, ); } @@ -194,20 +195,19 @@ export async function dispatchFeedback({ // A human-gated PR dispatches only for a thread a trusted person with // write access asked the lane to fix. const gated = (pull.labels ?? []).some((label) => label.name === humanGatedLabel); - const requesters = gated - ? await fixRequesters({ - comments: threads.flatMap((thread) => - thread.comments.nodes.map((node) => ({ authorId: node.author?.databaseId, login: node.author?.login, body: node.body })), - ), - trustedHumans, - permission: api.getPermission, - }) - : null; - if (!hasWorkableThread(threads, { reviewerId: settings.reviewerId, appId: appSenderId, trustedHumans, requesters })) { + const fixers = await fixRequesters({ + comments: threads.flatMap((thread) => + thread.comments.nodes.map((node) => ({ authorId: node.author?.databaseId, login: node.author?.login, body: node.body })), + ), + trustedHumans, + permission: api.getPermission, + }); + const requesters = gated ? fixers : null; + if (!hasWorkableThread(threads, { reviewerId: settings.reviewerId, appId: appSenderId, trustedHumans, requesters, fixers })) { return skip( gated ? `#${number} is labeled ${humanGatedLabel} and has no thread a trusted person with write access asked the lane to fix` - : `#${number} has no unresolved thread left by the reviewer bot alone or answered by a trusted person`, + : `#${number} has no unresolved thread left by the reviewer bot alone, asked to be fixed, or answered by a trusted person`, ); } // Both review lanes finish on one head, and their dispatchers queue in one diff --git a/.github/scripts/pr-unblock/feedback-dispatch.test.mjs b/.github/scripts/pr-unblock/feedback-dispatch.test.mjs index 0f210500f7..8396ab162d 100644 --- a/.github/scripts/pr-unblock/feedback-dispatch.test.mjs +++ b/.github/scripts/pr-unblock/feedback-dispatch.test.mjs @@ -525,6 +525,43 @@ test("without needs-human a trusted reply dispatches with no permission read", a assert.deepEqual(api.calls.permission, []); }); +// The #8031 shape: the reviewer bot's finding, then `reply`, with no reply +// from the lanes App. +const askedThread = (reply) => ({ + isResolved: false, + comments: { + nodes: [{ author: CLAUDE, body: "finding" }, reply].map((node, i) => ({ + createdAt: `2026-10-17T20:0${i}:00Z`, + lastEditedAt: null, + editor: null, + ...node, + })), + pageInfo: { hasNextPage: false }, + }, +}); + +test("a write-access trusted person's fix request on a thread the lane never answered dispatches", async () => { + const api = fakeApi({ threads: [askedThread({ author: KYLE_LOGIN, body: REQUEST })] }); + assert.equal((await decide(api)).result.number, 42); + assert.equal(api.calls.dispatch.length, 1); + assert.deepEqual(api.calls.permission, ["kyle-sexton"]); +}); + +for (const [label, options] of [ + ["an untrusted person's fix request", { threads: [askedThread({ author: STRANGER_LOGIN, body: REQUEST })], permissions: { stranger: "admin" } }], + ["a trusted person's fix request without write access", { + threads: [askedThread({ author: KYLE_LOGIN, body: REQUEST })], + permissions: { "kyle-sexton": "read" }, + }], + ["a trusted reply that asks no fix", { threads: [askedThread({ author: KYLE_LOGIN, body: "Owner decision: drop it." })] }], +]) { + test(`a thread the lane never answered dispatches nothing for ${label}`, async () => { + const api = fakeApi(options); + assert.equal((await decide(api)).result, null); + assert.deepEqual(api.calls.dispatch, []); + }); +} + test("main reads the trusted people from the base's trusted-actor list", async (t) => { const dir = mkdtempSync(join(tmpdir(), "feedback-dispatch-")); t.after(() => rmSync(dir, { recursive: true, force: true })); diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index c4797eb118..6de6f1bf72 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -134,8 +134,9 @@ the upstream run id from the event and reads the rest back. It dispatches `pr-un this repository, exactly one open non-draft PR into `main` has its head, the lanes App opened that PR, it carries no `do-not-merge`, both lanes' newest reviewed-head markers name the head, an unresolved thread exists that `address-feedback` would keep (left only by the -reviewer bot, or answered by a trusted human after the lane escalated it; on a PR carrying the -human-gated label, only one whose answer is a fix request, below), the head has no +reviewer bot, one the lane never answered where a fix request, below, follows the reviewer bot's +finding, or answered by a trusted human after the lane escalated it; on a PR carrying the +human-gated label, only one whose answer is a fix request), the head has no `pr-unblock / address-feedback` check run, and no `pr-unblock.yml` run titled `pr-unblock address-feedback #` is still queued or going (both lanes finishing at once). Then it runs the base's resolve-config for that PR and head with `address-feedback` selected, so only that @@ -146,11 +147,16 @@ In `pr-unblock.yml`, `address-feedback` (the model job, `mutate-branch`) is foll `publish-feedback` (a script job with no model, `publish-artifact`), which posts the replies the model job recorded ([`pr-run-activity.md`](pr-run-activity.md#prepare-review-threads)). +A fix request is a reply by a person on the trusted-actor list who holds write or admin +permission on the repository, edited by no one else, with a line that starts with +`PR-fix lane: please fix this finding`, in any letter case, outside a fenced code block (a quoted +`>` line never matches). On a thread the lanes App never commented on, one fix request after the +reviewer bot's finding makes the thread one `address-feedback` works, as long as every other +comment is the reviewer bot's or a trusted person's; those people's comments are direction. + On a PR carrying the human-gated role label (`needs-human` unless the base's -`.work-item-tracker.json` names another), `address-feedback` works a thread only when a reply -after the lane's escalation, by a person on the trusted-actor list who holds write or admin -permission on the repository, has a line that starts with `PR-fix lane: please fix this finding`, -in any letter case, outside a fenced code block (a quoted `>` line never matches). Without such a +`.work-item-tracker.json` names another), `address-feedback` works a thread only when it carries +a fix request, after the lane's escalation when there is one. Without such a thread the dispatcher starts nothing and the select step skips with `awaiting-human`, so the PR stays held; with one, the write runner lifts only the human-gated hold for that run, and `do-not-merge` still stops it. `pr-merge` refuses the PR either way. diff --git a/docs/conventions/pr-pipeline/pr-run-activity.md b/docs/conventions/pr-pipeline/pr-run-activity.md index 5e61870358..1da48fc281 100644 --- a/docs/conventions/pr-pipeline/pr-run-activity.md +++ b/docs/conventions/pr-pipeline/pr-run-activity.md @@ -645,17 +645,20 @@ the base's `.github/scripts/pr-unblock/address-feedback.mjs` and `publish-feedba 1. After `resolve-config` and before the mint, the model job runs `address-feedback.mjs select` with the job's `GITHUB_TOKEN`. It keeps each unresolved thread whose first comment's review the reviewer bot (`claude[bot]` in the base's trusted-actors list, by id) submitted on a head - that a `claude-lane-reviewed-head` marker from `github-actions[bot]` names, when either every - comment is the reviewer bot's, or the lanes App's last comment is a reply the reply job left - open (`INCORRECT`, `UNCERTAIN` or `VALID-defer`) and every comment after it is by a `human` on - that list, created after it, and never edited or last edited by such a human. Every other - comment must be the reviewer bot's, the lanes App's or such a human's, or the thread is left - alone. The thread file gives each comment an `author` (`reviewer`, `lane`, `trusted-human`) and - marks the replies after the escalation `direction: true`, which the skill follows within the - same path rules. On a PR carrying the base's human-gated role label it keeps a thread only when - one of those replies is a fix request (a line starting `PR-fix lane: please fix this finding`, - any case, outside a fenced block) by a listed human whose permission on the repository reads - `write` or `admin` (`GET /repos/{owner}/{repo}/collaborators/{username}/permission`), and + that a `claude-lane-reviewed-head` marker from `github-actions[bot]` names, when every + comment is the reviewer bot's; or the lanes App never commented and one comment is a fix + request (a line starting `PR-fix lane: please fix this finding`, any case, outside a fenced + block) by a `human` on that list whose permission on the repository reads `write` or `admin` + (`GET /repos/{owner}/{repo}/collaborators/{username}/permission`); or the lanes App's last + comment is a reply the reply job left open (`INCORRECT`, `UNCERTAIN` or `VALID-defer`) and + every comment after it is by such a `human` (any permission), created after it. A human's + comment counts only when never edited or last edited by such a human. Every other comment must + be the reviewer bot's, the lanes App's or such a human's, or the thread is left alone. The + thread file gives each comment an `author` (`reviewer`, `lane`, `trusted-human`) and marks + `direction: true` the replies after the escalation, or with no escalation every human comment, + which the skill follows within the same path rules. The reply job's re-read judges threads the + same way. On a PR carrying the base's human-gated role label it keeps a thread only when one of + those replies is such a fix request by a write-access human, and reports `fix-requested=true`, which lifts only the human-gated hold for the run. No kept thread skips `not-applicable`, or `awaiting-human` on such a PR. A PR diff touching `.github/**` escalates `feedback-protected-path`, and two earlier rounds escalate From 68fba69e4798e954e89e1dbc7774af72612bf80d Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 19 Oct 2026 09:27:28 -0400 Subject: [PATCH 2/4] docs(pr-pipeline): state the fix-request edit rule and select-step path Co-Authored-By: Claude Opus 5.5 --- .github/workflows/pr-run-activity-write.yml | 5 +++-- docs/conventions/pr-pipeline/README.md | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/workflows/pr-run-activity-write.yml b/.github/workflows/pr-run-activity-write.yml index 8bc6da1d90..48e56cf3d7 100644 --- a/.github/workflows/pr-run-activity-write.yml +++ b/.github/workflows/pr-run-activity-write.yml @@ -555,8 +555,9 @@ jobs: # review-threads, before the mint and any model turn, with the job's # read-only token: keeps the unresolved threads the reviewer bot opened - # on a head a review lane's marker confirms, left by it alone or answered - # by a trusted person after the lane escalated them, then + # on a head a review lane's marker confirms, left by it alone, asked to be + # fixed by a trusted person with write access, or answered by a trusted + # person after the lane escalated them, then # skips (none), escalates (a .github/** diff, or the round cap) or # writes the thread file the skill's review-thread tool reads. It also # copies out the base's thread-scope.mjs with its SHA-256, which the diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 9e2b2749d8..a658137450 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -148,7 +148,7 @@ In `pr-unblock.yml`, `address-feedback` (the model job, `mutate-branch`) is foll model job recorded ([`pr-run-activity.md`](pr-run-activity.md#prepare-review-threads)). A fix request is a reply by a person on the trusted-actor list who holds write or admin -permission on the repository, edited by no one else, with a line that starts with +permission on the repository, never edited or last edited by a person on that list, with a line that starts with `PR-fix lane: please fix this finding`, in any letter case, outside a fenced code block (a quoted `>` line never matches). On a thread the lanes App never commented on, one fix request after the reviewer bot's finding makes the thread one `address-feedback` works, as long as every other From 9494afa115f7a420c9e63385d5464bb340a2b4a4 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 19 Oct 2026 09:50:35 -0400 Subject: [PATCH 3/4] docs(pr-pipeline): state the manual dispatch that starts a fix-request round Co-Authored-By: Claude Opus 5.5 --- ...ix-address-feedback-trusted-fix-request-57421cca.md | 4 +++- docs/conventions/pr-pipeline/README.md | 10 ++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/.changes/pr-pipeline/fix-address-feedback-trusted-fix-request-57421cca.md b/.changes/pr-pipeline/fix-address-feedback-trusted-fix-request-57421cca.md index 001fe99f32..78292e025f 100644 --- a/.changes/pr-pipeline/fix-address-feedback-trusted-fix-request-57421cca.md +++ b/.changes/pr-pipeline/fix-address-feedback-trusted-fix-request-57421cca.md @@ -10,4 +10,6 @@ bump: minor bot's or a trusted person's. The select step, the reply job's re-read and `pr-feedback-dispatch.yml` judge threads the same way ([README.md](https://github.com/melodic-software/claude-code-plugins/blob/main/docs/conventions/pr-pipeline/README.md)). - The dispatcher still starts only when a review run completes. `version` stays 2. + A fix request posted after the head's review starts the round when a trusted person runs + `gh workflow run pr-unblock.yml -R melodic-software/claude-code-plugins -f pr-number= -f activity=address-feedback`. + `version` stays 2. diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index a658137450..f05dbd8f72 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -153,6 +153,16 @@ permission on the repository, never edited or last edited by a person on that li `>` line never matches). On a thread the lanes App never commented on, one fix request after the reviewer bot's finding makes the thread one `address-feedback` works, as long as every other comment is the reviewer bot's or a trusted person's; those people's comments are direction. +`pr-feedback-dispatch.yml` runs only when a review run completes, so a fix request posted after +the head's review starts nothing on its own. To start the round, a person on the trusted-actor +list dispatches the lane directly: + +```bash +gh workflow run pr-unblock.yml -R melodic-software/claude-code-plugins -f pr-number= -f activity=address-feedback +``` + +That run names no dispatcher run, so the gate checks the person as the actor, and the select step +judges the threads as above. On a PR carrying the human-gated role label (`needs-human` unless the base's `.work-item-tracker.json` names another), `address-feedback` works a thread only when it carries From b2349275c9ca062c85df437a8ecaee73f86fec33 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 19 Oct 2026 09:53:51 -0400 Subject: [PATCH 4/4] fix(pr-pipeline): mark direction only from the first fix request on A trusted comment before the write-access fix request on a thread the lane never answered stays context, so text relayed earlier does not become direction once the request lands. Co-Authored-By: Claude Opus 5.5 --- ...-address-feedback-trusted-fix-request-57421cca.md | 2 +- .github/scripts/pr-unblock/address-feedback.mjs | 12 ++++++++---- .github/scripts/pr-unblock/address-feedback.test.mjs | 8 ++++++++ docs/conventions/pr-pipeline/README.md | 11 ++++++----- docs/conventions/pr-pipeline/pr-run-activity.md | 3 ++- 5 files changed, 25 insertions(+), 11 deletions(-) diff --git a/.changes/pr-pipeline/fix-address-feedback-trusted-fix-request-57421cca.md b/.changes/pr-pipeline/fix-address-feedback-trusted-fix-request-57421cca.md index 78292e025f..a77caede46 100644 --- a/.changes/pr-pipeline/fix-address-feedback-trusted-fix-request-57421cca.md +++ b/.changes/pr-pipeline/fix-address-feedback-trusted-fix-request-57421cca.md @@ -10,6 +10,6 @@ bump: minor bot's or a trusted person's. The select step, the reply job's re-read and `pr-feedback-dispatch.yml` judge threads the same way ([README.md](https://github.com/melodic-software/claude-code-plugins/blob/main/docs/conventions/pr-pipeline/README.md)). - A fix request posted after the head's review starts the round when a trusted person runs + A fix request posted after the head's review starts the round when an account on the trusted-actor list runs `gh workflow run pr-unblock.yml -R melodic-software/claude-code-plugins -f pr-number= -f activity=address-feedback`. `version` stays 2. diff --git a/.github/scripts/pr-unblock/address-feedback.mjs b/.github/scripts/pr-unblock/address-feedback.mjs index 16a02fe4ff..a9ac2d6fe6 100644 --- a/.github/scripts/pr-unblock/address-feedback.mjs +++ b/.github/scripts/pr-unblock/address-feedback.mjs @@ -18,7 +18,9 @@ // // Every comment body is copied into the thread file and never logged or // interpolated into a command. Only a trusted person's reply to the lane's -// escalation is marked as direction; every other body is data. +// escalation, or with no escalation a trusted person's comment from the first +// write-access fix request on, is marked as direction; every other body is +// data. import { appendFileSync, readFileSync, writeFileSync } from "node:fs"; import { pathToFileURL } from "node:url"; @@ -160,7 +162,8 @@ export function humanGatedLabelFrom(path) { // - the lanes App never commented, every later comment is the reviewer bot's // or a trusted person's, and one is a fix request (isFixRequest) by one of // `fixers` (the trusted people with write access who wrote a fix request, -// fixRequesters); the trusted people's comments are direction; +// fixRequesters); the trusted people's comments from the first such +// request on are direction; // - the lanes App's last comment escalated the thread and every comment after // it is a trusted person's reply, which is direction. // A comment by anyone else, a trusted person's comment last edited by anyone @@ -186,12 +189,13 @@ export function threadAuthors(comments, { reviewerId, appId, trustedHumans = new if (!humanAsks(fixers) || (requesters && !humanAsks(requesters))) { return null; } + const asked = comments.findIndex((comment) => isHuman(comment) && asks(fixers, comment)); const authors = []; - for (const comment of comments) { + for (const [i, comment] of comments.entries()) { if (comment.authorId === reviewerId) { authors.push({ author: "reviewer" }); } else if (isHuman(comment)) { - authors.push({ author: "trusted-human", direction: true }); + authors.push(i >= asked ? { author: "trusted-human", direction: true } : { author: "trusted-human" }); } else { return null; } diff --git a/.github/scripts/pr-unblock/address-feedback.test.mjs b/.github/scripts/pr-unblock/address-feedback.test.mjs index 0fafe13e49..2a637d243d 100644 --- a/.github/scripts/pr-unblock/address-feedback.test.mjs +++ b/.github/scripts/pr-unblock/address-feedback.test.mjs @@ -279,6 +279,14 @@ test("with no escalation, a write-access trusted person's fix request is kept as assert.deepEqual(api.permissionReads, ["kyle-sexton"]); }); +test("with no escalation, a trusted comment before the fix request is context, not direction", async () => { + const result = await run(fakeApi(asked([[101, HUMAN, DECISION], [102, REVIEWER, "finding 102"], [103, HUMAN, REQUEST]]))); + assert.deepEqual( + result.file.threads[0].comments.map((row) => [row.id, row.author, row.direction ?? false]), + [[100, "reviewer", false], [101, "trusted-human", false], [102, "reviewer", false], [103, "trusted-human", true]], + ); +}); + test("with no escalation, on a needs-human PR, a write-access trusted person's fix request is kept", async () => { const result = await run(fakeApi({ ...asked([[101, HUMAN, REQUEST]]), labels: ["needs-human"] })); assert.equal(result.decision, "proceed"); diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index e0da90a259..7eb3158c35 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -148,21 +148,22 @@ In `pr-unblock.yml`, `address-feedback` (the model job, `mutate-branch`) is foll model job recorded ([`pr-run-activity.md`](pr-run-activity.md#prepare-review-threads)). A fix request is a reply by a person on the trusted-actor list who holds write or admin -permission on the repository, never edited or last edited by a person on that list, with a line that starts with +permission on the repository, never edited, or last edited by a person on that list, with a line that starts with `PR-fix lane: please fix this finding`, in any letter case, outside a fenced code block (a quoted `>` line never matches). On a thread the lanes App never commented on, one fix request after the reviewer bot's finding makes the thread one `address-feedback` works, as long as every other -comment is the reviewer bot's or a trusted person's; those people's comments are direction. +comment is the reviewer bot's or a trusted person's; their comments from the first fix request on +are direction. `pr-feedback-dispatch.yml` runs only when a review run completes, so a fix request posted after -the head's review starts nothing on its own. To start the round, a person on the trusted-actor +the head's review starts nothing on its own. To start the round, an account on the trusted-actor list dispatches the lane directly: ```bash gh workflow run pr-unblock.yml -R melodic-software/claude-code-plugins -f pr-number= -f activity=address-feedback ``` -That run names no dispatcher run, so the gate checks the person as the actor, and the select step -judges the threads as above. +That run names no dispatcher run, so the gate checks that account as the actor, and the select +step judges the threads as above. On a PR carrying the human-gated role label (`needs-human` unless the base's `.work-item-tracker.json` names another), `address-feedback` works a thread only when it carries diff --git a/docs/conventions/pr-pipeline/pr-run-activity.md b/docs/conventions/pr-pipeline/pr-run-activity.md index 044906d967..201706496d 100644 --- a/docs/conventions/pr-pipeline/pr-run-activity.md +++ b/docs/conventions/pr-pipeline/pr-run-activity.md @@ -656,7 +656,8 @@ the base's `.github/scripts/pr-unblock/address-feedback.mjs` and `publish-feedba comment counts only when never edited or last edited by such a human. Every other comment must be the reviewer bot's, the lanes App's or such a human's, or the thread is left alone. The thread file gives each comment an `author` (`reviewer`, `lane`, `trusted-human`) and marks - `direction: true` the replies after the escalation, or with no escalation every human comment, + `direction: true` the replies after the escalation, or with no escalation every human comment + from the first such fix request on, which the skill follows within the same path rules. The reply job's re-read judges threads the same way. On a PR carrying the base's human-gated role label it keeps a thread only when one of those replies is such a fix request by a write-access human, and