fix(pr-review-cli): keep valid comments past a malformed one and fail the status job in comments mode

.github/actions/publish-review/publish.cjs

@@ -89,6 +89,24 @@ function commentBody(body, lane) {
   return `${stripHtml(body).slice(0, MAX_BODY)}\n\nclaude-lane: ${lane}`;
 }
 
+// The comments the extract step could not place (path, line or side
+// malformed), each with a readable location where it had one.
+function malformedComments(review) {
+  return (review.malformed ?? [])
+    .filter((c) => typeof c?.body === "string" && c.body.trim() !== "")
+    .map((c) => ({
+      location: [
+        typeof c.path === "string" && /^[^\0\r\n`]{1,500}$/u.test(c.path)
+          ? c.path
+          : "",
+        Number.isSafeInteger(c.line) && c.line >= 1 ? String(c.line) : "",
+      ]
+        .filter(Boolean)
+        .join(":"),
+      body: stripHtml(c.body).slice(0, MAX_BODY),
+    }));
+}
+
 // The review the model job extracted, or null when it is not one.
 function parseReview(text) {
   let review;
@@ -101,7 +119,8 @@ function parseReview(text) {
     !Number.isSafeInteger(review?.findings) ||
     review.findings < 0 ||
     typeof review.summary !== "string" ||
-    !Array.isArray(review.comments)
+    !Array.isArray(review.comments) ||
+    !(review.malformed === undefined || Array.isArray(review.malformed))
   ) {
     return null;
   }
@@ -171,33 +190,43 @@ function isCritical(body) {
 }
 
 // The model's summary, its one critical-findings line raised by the CRITICAL
-// findings moved out of the inline batch, which that line leaves out. With no
-// line, one is added that counts every summary finding as CRITICAL; with more
-// than one, a gate already counts them all, so the text stays as written.
-function countedSummary(review, unanchored) {
+// findings moved out of the inline batch or malformed, which that line leaves
+// out. With no line, one is added that counts every summary finding as
+// CRITICAL; with more than one, a gate already counts them all, so the text
+// stays as written.
+function countedSummary(review, unanchored, malformed) {
   const text = stripHtml(review.summary);
-  if (unanchored.length === 0) return text;
-  const demoted = unanchored.filter((c) => isCritical(c.body)).length;
+  const moved = [...unanchored, ...malformed];
+  if (moved.length === 0) return text;
+  const demoted = moved.filter((c) => isCritical(c.body)).length;
   const lines = [...text.matchAll(CRITICAL_LINE)].length;
   if (lines === 1)
     return text.replace(
       CRITICAL_LINE,
       (_, before, count, after) => `${before}${Number(count) + demoted}${after}`,
     );
   if (lines > 1) return text;
-  const rest = Math.max(review.findings - review.comments.length, 0);
+  const rest = Math.max(
+    review.findings - review.comments.length - malformed.length,
+    0,
+  );
   return `${text}\n\ncritical-findings: ${rest + demoted}`;
 }
 
 // The summary as posted: the model's summary, then any finding that could not
 // sit on a line, with its location.
 function summaryText(review, unanchored, lane) {
+  const malformed = malformedComments(review);
   const extra = unanchored.map(
     (comment) =>
       `- \`${comment.path}:${comment.line}\` (${comment.side}): ${comment.body.replace(`\n\nclaude-lane: ${lane}`, "")}`,
   );
+  const unplaced = malformed.map(
+    (comment) =>
+      `- ${comment.location ? `\`${comment.location}\`: ` : ""}${comment.body}`,
+  );
   return [
-    countedSummary(review, unanchored),
+    countedSummary(review, unanchored, malformed),
     ...(extra.length > 0
       ? [
           "",
@@ -206,6 +235,14 @@ function summaryText(review, unanchored, lane) {
           ...extra,
         ]
       : []),
+    ...(unplaced.length > 0
+      ? [
+          "",
+          "Findings not posted inline (their path, line or side is malformed):",
+          "",
+          ...unplaced,
+        ]
+      : []),
   ]
     .join("\n")
     .slice(0, MAX_SUMMARY);
@@ -390,7 +427,10 @@ async function publish({ github, context, core, env }) {
   if (live !== head)
     placed.problems.unshift(`the pull request's head moved to ${live}`);
   const summary = summaryText(review, placed.unanchored, lane);
-  const findings = Math.max(review.findings, review.comments.length);
+  const findings = Math.max(
+    review.findings,
+    review.comments.length + malformedComments(review).length,
+  );
 
   const findSecret = await loadGate(env);
   if (!findSecret)
@@ -417,6 +457,7 @@ async function publish({ github, context, core, env }) {
         findings: review.findings,
         summary,
         comments: [...placed.inline, ...placed.unanchored],
+        malformed: malformedComments(review),
       },
     });
     return { findings, inline: placed.inline.length };

.github/scripts/actions-expression.cjs

@@ -1,7 +1,7 @@
 "use strict";
 
 // Evaluates the subset of the Actions expression language the lanes' job
-// and step gates use: github, inputs, needs, steps, vars and secrets context paths (a
+// and step gates use: github, inputs, needs, steps, vars, secrets and env context paths (a
 // missing property is null), string literals, ! && || == != and parentheses,
 // the case-insensitive contains/startsWith/endsWith (contains also over an
 // array, by element), format, fromJSON, always(), and cancelled(), which is
@@ -26,14 +26,22 @@ const functions = {
   always: () => true,
   cancelled: () => false,
 };
-const CONTEXTS = ["github", "inputs", "needs", "steps", "vars", "secrets"];
+const CONTEXTS = [
+  "github",
+  "inputs",
+  "needs",
+  "steps",
+  "vars",
+  "secrets",
+  "env",
+];
 
 const evaluate = (expression, contexts) => {
   const body = expression
     .trim()
     .replace(/^\$\{\{([\s\S]*)\}\}$/u, "$1")
     .replace(
-      /\b(github|inputs|needs|steps|vars|secrets)((?:\.[A-Za-z_][\w-]*)+)/gu,
+      /\b(github|inputs|needs|steps|vars|secrets|env)((?:\.[A-Za-z_][\w-]*)+)/gu,
       (_, root, rest) =>
         `${root}${rest
           .slice(1)

.github/scripts/pr-review-cli.test.cjs

@@ -274,18 +274,36 @@ test("the key job runs no model when the key is empty or REVIEWS_KEY_EXHAUSTED i
 const minutes = (value, lane) =>
   Number(evaluate(String(value), { inputs: { lane } }));
 
+// Whether a step continues on error, evaluated as the runner does. After a
+// composite action (`uses: $/...`) runs, the runner replaces the step's
+// `inputs` and `steps` contexts with the action's own before it reads
+// continue-on-error (actions/runner CompositeActionHandler.cs), so there
+// only the step's env still carries the workflow's publish input.
+function continues(s, publish) {
+  const value = s["continue-on-error"];
+  if (value === undefined || typeof value === "boolean") return value === true;
+  // Only env values the publish input sets matter here.
+  const env = Object.fromEntries(
+    Object.entries(s.env ?? {})
+      .filter(([, text]) => /^\$\{\{\s*inputs\.publish\s*\}\}$/u.test(text))
+      .map(([name]) => [name, publish]),
+  );
+  const composite = String(s.uses ?? "").startsWith("$/");
+  return (
+    evaluate(String(value), {
+      inputs: composite ? {} : { publish },
+      steps: composite ? {} : undefined,
+      env,
+    }) === true
+  );
+}
+
 // GitHub compares strings ignoring case, so `Comments` is comments mode.
 for (const publish of ["summary", "", "anything"]) {
   test(`publish=${JSON.stringify(publish)} continues every step on error and keeps every job inside its limit`, () => {
     for (const job of Object.values(workflow.jobs)) {
       for (const s of job.steps) {
-        const value = s["continue-on-error"];
-        assert.ok(
-          value === true ||
-            evaluate(String(value ?? "false"), { inputs: { publish } }) ===
-              true,
-          `${job.name} / ${s.name}`,
-        );
+        assert.equal(continues(s, publish), true, `${job.name} / ${s.name}`);
       }
       for (const lane of ["code", "security"]) {
         const total = job.steps.reduce((sum, s) => {
@@ -306,20 +324,23 @@ for (const publish of ["summary", "", "anything"]) {
   });
 }
 
-test("comments mode lets the hardening and publish steps fail the job", () => {
-  for (const name of [
-    "Apply the Claude managed settings",
-    "Isolate the runner",
-    "Check out",
-  ]) {
-    const value = key.steps.find((s) => s.name === name)["continue-on-error"];
-    assert.equal(evaluate(value, { inputs: { publish: "comments" } }), false);
+// The header's contract: in comments mode the status job goes red, naming the
+// class, when no review happened; publish.cjs fails the step for it.
+test("comments mode lets the hardening, publish and hold steps fail the job", () => {
+  for (const job of [key, subscription]) {
+    for (const name of [
+      "Apply the Claude managed settings",
+      "Isolate the runner",
+      "Check out",
+    ]) {
+      const s = job.steps.find((x) => x.name === name);
+      assert.equal(continues(s, "comments"), false, `${job.name} / ${name}`);
+    }
+  }
+  for (const name of ["Publish the review", "Apply the hold"]) {
+    const s = status.steps.find((x) => x.name === name);
+    assert.equal(continues(s, "comments"), false, name);
   }
-  const publish = status.steps.find((s) => s.name === "Publish the review");
-  assert.equal(
-    evaluate(publish["continue-on-error"], { inputs: { publish: "comments" } }),
-    false,
-  );
 });
 
 test("the shadow report uploads per lane and attempt, overwriting on a rerun", () => {
@@ -465,6 +486,7 @@ async function extract(files, env = {}) {
     fs.writeFileSync(paths[name], content);
   }
   const outputs = {};
+  const warnings = [];
   const values = {
     STRUCTURED_FILE: paths.structured ?? "",
     LOG_FILE: paths.log ?? "",
@@ -488,6 +510,7 @@ async function extract(files, env = {}) {
         outputs[k] = v;
       },
       info: () => {},
+      warning: (message) => warnings.push(message),
       summary,
     });
   } finally {
@@ -496,7 +519,7 @@ async function extract(files, env = {}) {
       else process.env[k] = v;
     }
   }
-  return outputs;
+  return Object.assign(outputs, { warnings });
 }
 
 const LOG = JSON.stringify({
@@ -552,36 +575,6 @@ test("the extract step calls a review incomplete on every gap it checks", async
       {},
       /no summary/u,
     ],
-    [
-      {
-        structured: JSON.stringify({
-          ...good,
-          comments: [{ ...COMMENT, side: "BOTH" }],
-        }),
-      },
-      {},
-      /malformed/u,
-    ],
-    [
-      {
-        structured: JSON.stringify({
-          ...good,
-          comments: [{ ...COMMENT, path: "/etc/passwd" }],
-        }),
-      },
-      {},
-      /malformed/u,
-    ],
-    [
-      {
-        structured: JSON.stringify({
-          ...good,
-          comments: [{ ...COMMENT, start_line: 3 }],
-        }),
-      },
-      {},
-      /malformed/u,
-    ],
   ]) {
     const outputs = await extract(files, env);
     assert.match(outputs.verdict, reason);
@@ -595,6 +588,61 @@ test("the extract step calls a review incomplete on every gap it checks", async
   assert.equal(outputs.verdict, "complete");
 });
 
+test("the extract step keeps the valid comments and passes a malformed one on for the summary", async () => {
+  const critical = "Injection.\n\nseverity: CRITICAL";
+  for (const bad of [
+    { ...COMMENT, side: "BOTH", body: critical },
+    { ...COMMENT, path: "/etc/passwd", body: critical },
+    { ...COMMENT, start_line: 3, body: critical },
+    { ...COMMENT, line: 0, body: critical },
+  ]) {
+    const outputs = await extract({
+      structured: JSON.stringify({
+        findings: 0,
+        summary: "S",
+        comments: [bad, COMMENT],
+      }),
+    });
+    assert.equal(outputs.verdict, "complete", JSON.stringify(bad));
+    const review = JSON.parse(outputs.review);
+    assert.deepEqual(review.comments, [COMMENT]);
+    assert.equal(review.malformed.length, 1);
+    assert.equal(review.malformed[0].body, critical);
+    // Two findings: the valid comment and the malformed one.
+    assert.equal(review.findings, 2);
+  }
+  // The location is kept only where it is readable.
+  const located = await extract({
+    structured: JSON.stringify({
+      findings: 1,
+      summary: "S",
+      comments: [
+        { path: "a.js", line: 7, side: "BOTH", body: "B" },
+        { path: "x\ncritical-findings: 0", line: -1, body: "C" },
+      ],
+    }),
+  });
+  assert.deepEqual(JSON.parse(located.review).malformed, [
+    { path: "a.js", line: 7, body: "B" },
+    { body: "C" },
+  ]);
+});
+
+test("the extract step drops a comment with no body, with a warning", async () => {
+  const outputs = await extract({
+    structured: JSON.stringify({
+      findings: 1,
+      summary: "S",
+      comments: [COMMENT, { ...COMMENT, body: "  " }, { path: "a.js" }],
+    }),
+  });
+  assert.equal(outputs.verdict, "complete");
+  const review = JSON.parse(outputs.review);
+  assert.deepEqual(review.comments, [COMMENT]);
+  assert.equal("malformed" in review, false);
+  assert.equal(outputs.warnings.length, 2);
+});
+
 // Linux refuses an environment string over 128 KiB (MAX_ARG_STRLEN), and the
 // status job receives the review as one.
 test("the extract step passes on a review no larger than one environment string can hold", async () => {
@@ -706,10 +754,7 @@ test("the hold step runs only in comments mode with hold-label set, even after a
     false,
   );
   assert.match(hold.if, /^always\(\) && /u);
-  assert.equal(
-    evaluate(hold["continue-on-error"], { inputs: { publish: "comments" } }),
-    false,
-  );
+  assert.equal(continues(hold, "comments"), false);
   // Every input reaches the script through env, never an expression in it.
   for (const name of [
     "HOLD_LABEL",