feat(pr-pipeline): let a trusted fix request start address-feedback without an escalation

.changes/pr-pipeline/fix-address-feedback-trusted-fix-request-57421cca.md

@@ -0,0 +1,15 @@
+---
+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)).
+  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=<n> -f activity=address-feedback`.
+  `version` stays 2.

.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
@@ -17,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";
@@ -121,6 +124,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 +157,62 @@ 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 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
+// 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 asked = comments.findIndex((comment) => isHuman(comment) && asks(fixers, comment));
+    const authors = [];
+    for (const [i, comment] of comments.entries()) {
+      if (comment.authorId === reviewerId) {
+        authors.push({ author: "reviewer" });
+      } else if (isHuman(comment)) {
+        authors.push(i >= asked ? { author: "trusted-human", direction: true } : { author: "trusted-human" });
+      } 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 +232,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 +253,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 +294,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 +303,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 +340,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 }),
   };
 }
 

.github/scripts/pr-unblock/address-feedback.test.mjs

@@ -248,6 +248,65 @@ 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, 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");
+  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 }));

.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

.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 }));

.github/workflows/pr-run-activity-write.yml

@@ -567,8 +567,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