qa-review: tune the review job, and record that it is on hold (with why and how to resume)

.github/workflows/qa-review.yml

@@ -151,7 +151,8 @@ jobs:
       # to the PR itself (post_comment). Only an explicit "false" counts as public.
       - name: Show result in the run summary (public target repos only)
         if: steps.review.outcome == 'success' && steps.review.outputs.repo_private == 'false'
-        run: cat "$QA_BODY_OUT" >> "$GITHUB_STEP_SUMMARY"
+        # tee: also in the job log, which (unlike the summary) can be read via the API/CLI.
+        run: tee -a "$GITHUB_STEP_SUMMARY" < "$QA_BODY_OUT"
 
       - name: Note that the result is withheld from the run summary
         if: steps.review.outcome == 'success' && steps.review.outputs.repo_private != 'false'

CHANGELOG.md

@@ -2,25 +2,15 @@
 
 Short, dated summary of notable fixes and changes. For the full "why," see `PHASE2-SETUP.md` (Crisp triage design) or the linked PRs.
 
-## 2026-10-09 — QA review: advisory review job (step 2), scoped to complement existing checks
+## 2026-10-09 — QA review bot: built, run by hand only, ON HOLD
 
-Found while inspecting the pilot repo (`user-registration-pro`): it already has PHPCS-on-PR (`pr-code-sniff.yml`), an AI security scan (`security-review.yml`), and `ThemeGrill/claudegrill`'s deterministic E2E suite with `.themegrill-qa/` test cases and knowledge. claudegrill's README says AI was deliberately removed from the PR path. So the original plan (own static tools + droplet WordPress sandbox) would have duplicated all of that, and was dropped for now. This bot is opt-in only (someone must request `tg-autopilot` as reviewer) and is scoped to what those checks don't do.
+A bot that reviews a PR on request: it reads the diff and the repo's `.themegrill-qa/` notes and writes one advisory comment (summary, risks tied to changed lines, existing test cases to run by hand). Files: `qa-review.yml`, `scripts/qa-review-*.mjs`, `prompts/qa-review.md`, `config/qa-review-repos.json`.
 
-- New `review` job in `qa-review.yml`, after the gate. It never checks out or runs PR code: it reads the diff and the base branch's `.themegrill-qa/` (`suite.json` area map, `testcase-index.json` titles, `knowledge.md`) through the API, plus the other checks' results, and makes ONE model call (`prompts/qa-review.md`, default `gpt-5.4-mini`, override with the `QA_REVIEW_MODEL` repo variable).
-- Output is one sticky advisory comment: summary, areas touched (deterministic, from `suite.json`), risks, existing test cases worth running by hand, and scenarios with no test case.
-- Findings are verified, not trusted (see the CHANGELOG 2026-10-05 note on uncalibrated confidence): a risk is shown only if its `file:line` is an ADDED line in the diff; a suggested test only if its title exists verbatim in the index. Everything else is dropped and counted in the comment footer. No confidence number is shown. Model output is stripped of URLs, `@mentions`, HTML and backticks. An unparseable or empty model answer fails the job instead of posting "no risks found".
-- Not verified yet against a real model call: output quality, and cost (`pricing.mjs` has no row for the default model, so cost shows only if you add one). Uses the shared `OPENAI_API_KEY`; a dedicated spend-capped key is advisable before n8n makes this fire automatically.
-- Still manual dispatch only; no n8n flow or org webhook yet.
-- **Privacy:** `themegrill/.github` is PUBLIC, so run summaries/logs are public. The first dry run wrote a review of a private-repo PR into a public run summary (one-line change; the run was deleted). The review body now goes to the run summary only when the target repo is explicitly public; for private repos it is posted to the PR only (`post_comment=true`). Never log or summarize review content, PR text or diffs from private repos in this repo's runs.
-- The bot's fine-grained PAT cannot read check runs (403, needs "Checks: read"); that section is best-effort and says "not available" until the PAT is updated.
-
-## 2026-10-09 — QA review: gate only (step 1 of a staged build, not a review yet)
-
-First piece of a PR QA-review agent (request `tg-autopilot` as reviewer -> deep review). This change adds only the front half: `qa-review.yml` (`repository_dispatch: qa-review` or manual `workflow_dispatch`), `scripts/qa-review-gate.mjs` and `config/qa-review-repos.json` (allowlist; pilot is `themegrill/user-registration-pro` only).
-
-- The gate re-verifies everything against the live API (the dispatch payload is untrusted): allowlisted repo, PR open and not a draft, same-repo (forks denied), `head_sha` current, requester is not the bot, requester has `write` or higher (triage is not enough, each review will spend LLM money). A wrong token identity (not `tg-autopilot`) fails the job rather than going silently green.
-- `scripts/qa-review-comment.mjs` keeps ONE sticky PR comment, matched by bot login plus marker. Manual runs post nothing unless `post_comment` is true. The "accepted" comment is a placeholder and says no findings will follow.
-- No n8n flow, org webhook, static checks, LLM review or droplet sandbox yet; the only way in is a manual dispatch. Not yet verified: that the real `BOT_TOKEN_THEMEGRILL` authenticates as `tg-autopilot` and can read the pilot repo.
+- **Run it:** Actions -> "QA review" -> Run workflow (`repo`, `pr`, optional `post_comment`). Only allowlisted repos, and the requester needs write access. Nothing triggers it automatically: there is no n8n flow or webhook.
+- **Built to not mislead:** a risk is shown only if its cited line is an added line in the diff, and a suggested test only if it exists in the repo's test index; anything else is dropped and counted. No confidence number. Review text for a private repo is never written to this public repo's run summary or logs.
+- **Why on hold:** it only reads code and never runs the plugin, and it overlaps Copilot, `security-review.yml` and PHPCS. `ThemeGrill/claudegrill` already runs the real Playwright e2e, and its AI agent tier needs an Anthropic key and is switched off on purpose. Don't build a second e2e system next to it.
+- **If resumed:** (1) spike locally first: an OpenAI agent (`opencode`) following a PR's "How to test" steps against a site from claudegrill's `boot-wp.mjs`, e.g. on `user-registration-pro#1610`; (2) only if it works, run it in a private repo, because this repo is public; (3) then add the n8n trigger and a dedicated, spend-capped OpenAI key.
+- **Gotchas:** use `themegrill/user-registration`, not the `wpeverest/` name (that org's token gets 403). The pro repo's token can't read check runs, so that line says "not available". The same PR can get somewhat different reviews run to run. About 9-13k input tokens per review; cost not measured.
 
 ## 2026-10-05 — wp.org triage files issues in the pro repo first
 

config/qa-review-repos.json

@@ -1,6 +1,7 @@
 {
   "wpeverest": [],
   "themegrill": [
+    "user-registration",
     "user-registration-pro"
   ]
 }

prompts/qa-review.md

@@ -14,11 +14,13 @@ PHPCS/coding-standards, formatting, and the end-to-end suite run separately. Do
 - Security-relevant changes: missing nonce/capability checks, unescaped output, unprepared SQL, in code the PR adds or changes.
 - Behaviour a user can see that has no existing test case.
 
-# Evidence rules (strict, enforced by a script after you answer)
+# Evidence rules (strict, partly enforced by a script after you answer)
 
 - A "risk" MUST cite a `file` from the diff and a `line` number that is an ADDED line, i.e. one marked `L<number>+` in the diff. Use that exact number. Findings that cite anything else are discarded.
-- A "manual_test" MUST be copied VERBATIM from `<existing_test_cases>`. Titles not in that list are discarded. Only choose cases genuinely affected by the change (maximum 8).
-- Say what you can see in the diff. Do not claim a bug unless the changed lines show it. If it depends on code you cannot see, say "depends on code outside this diff" in `evidence`, or leave it out. Prefer fewer, solid findings over many weak ones. Empty lists are a good answer for a clean change.
+- A risk must describe a CONCRETE failure that the changed lines themselves show: what input or situation breaks, and why. Do NOT write "if X relies on Y" or "could potentially" about code you cannot see. Do NOT raise anything the PR description already explains or answers. If you would have to guess about code outside the diff, leave it out. An empty `risks` list is the correct answer for most small, clean fixes.
+- A "manual_test" MUST be copied VERBATIM from `<existing_test_cases>`, and must test the SAME feature the PR changes. A test for a different feature that merely shares a page, a shortcode or a screen is wrong: leave it out. No test is better than a loosely related one.
+- A "new_scenario" is for user-visible behaviour with NO existing test case. Do NOT repeat anything already listed in the PR description's own testing steps; the author has covered those. Do not pad: if you have nothing the PR author has not already covered, return none.
+- Match the size of your answer to the size of the change. A one-to-ten line change normally deserves a short summary and few or no findings. More findings is not better.
 - Never state a confidence number.
 
 # Output
@@ -38,4 +40,4 @@ Return ONLY a JSON object, no markdown fences, exactly this shape:
   ]
 }
 
-Limits: at most 8 risks, 8 manual_tests, 5 new_scenarios. Keep every string under 300 characters.
+Hard maximums are 8 risks, 8 manual_tests, 5 new_scenarios, and a script trims further for small changes. Keep every string under 300 characters.

scripts/openai-client.mjs

@@ -8,7 +8,10 @@ export async function chatJSON(systemPrompt, userContent, fallback) {
 }
 
 // Same call, plus the token usage OpenAI reports -- feeds the event log.
-export async function chatJSONWithUsage(systemPrompt, userContent, fallback) {
+// responseFormat defaults to loose JSON mode (existing callers unchanged). Pass
+// {type:"json_schema", json_schema:{name, strict:true, schema}} to make the API
+// enforce the shape -- loose mode lets the model rename keys (seen in QA review).
+export async function chatJSONWithUsage(systemPrompt, userContent, fallback, responseFormat = { type: "json_object" }) {
   if (!OPENAI_API_KEY || !CLASSIFY_MODEL) {
     throw new Error("Missing required env var: OPENAI_API_KEY or CLASSIFY_MODEL");
   }
@@ -20,7 +23,7 @@ export async function chatJSONWithUsage(systemPrompt, userContent, fallback) {
     },
     body: JSON.stringify({
       model: CLASSIFY_MODEL,
-      response_format: { type: "json_object" },
+      response_format: responseFormat,
       messages: [
         { role: "system", content: systemPrompt },
         { role: "user", content: userContent },

scripts/qa-review-render.mjs

@@ -27,21 +27,38 @@ export function sanitize(value, max = 300) {
 
 const asArray = (v) => (Array.isArray(v) ? v : []);
 
-// ctx: { addedByFile: {file: Set<line>}, titles: [{title,file}] }
+// How many findings a change of this size can plausibly justify. Enforced here
+// in code because a prompt-only limit was ignored: a ONE-line PR got 1 risk,
+// 1 test and 3 scenarios (user-registration-pro#1610 dry run).
+export function limitsFor(changedLines) {
+  if (changedLines <= 10) return { risks: 1, manual: 2, scenarios: 1 };
+  if (changedLines <= 60) return { risks: 3, manual: 4, scenarios: 2 };
+  if (changedLines <= 300) return { risks: 5, manual: 6, scenarios: 3 };
+  return { risks: 8, manual: 8, scenarios: 5 };
+}
+
+// ctx: { addedByFile: {file: Set<line>}, titles: [{title,file}], limits?: {risks,manual,scenarios} }
 export function validateReview(raw, ctx) {
   const data = raw && typeof raw === "object" ? raw : {};
+  const limits = ctx.limits ?? { risks: 8, manual: 8, scenarios: 5 };
   const known = new Map(ctx.titles.map((t) => [t.title.trim(), t]));
   const dropped = { risks: 0, manual_tests: 0, new_scenarios: 0 };
+  // Why each item was discarded, for debugging. Contains model/PR-derived text,
+  // so the caller must only ever print it for explicitly public repos.
+  const detail = [];
 
   const risks = [];
   for (const r of asArray(data.risks)) {
     const line = Number(r?.line);
     const lines = ctx.addedByFile[r?.file];
-    if (!lines || !Number.isInteger(line) || !lines.has(line) || !sanitize(r.claim)) {
+    const why = !lines ? "file-not-in-shown-diff" : !Number.isInteger(line) ? "line-not-an-integer" : !lines.has(line) ? "line-not-an-added-line" : !sanitize(r.claim) ? "empty-claim" : null;
+    if (why) {
       dropped.risks++;
+      const added = lines ? [...lines].sort((a, b) => a - b) : [];
+      detail.push({ kind: "risk", why, file: String(r?.file).slice(0, 120), line: r?.line, addedRange: added.length ? [added[0], added[added.length - 1], added.length] : null });
       continue;
     }
-    if (risks.length < 8) risks.push({ file: r.file, line, claim: sanitize(r.claim), evidence: sanitize(r.evidence) });
+    if (risks.length < limits.risks) risks.push({ file: r.file, line, claim: sanitize(r.claim), evidence: sanitize(r.evidence) });
   }
 
   const manual = [];
@@ -50,22 +67,24 @@ export function validateReview(raw, ctx) {
     const key = typeof t?.title === "string" ? t.title.trim() : "";
     if (!known.has(key) || seen.has(key)) {
       dropped.manual_tests++;
+      detail.push({ kind: "manual_test", why: seen.has(key) ? "duplicate-title" : "title-not-in-index", title: key.slice(0, 120) });
       continue;
     }
     seen.add(key);
-    if (manual.length < 8) manual.push({ title: key, file: known.get(key).file, why: sanitize(t.why) });
+    if (manual.length < limits.manual) manual.push({ title: key, file: known.get(key).file, why: sanitize(t.why) });
   }
 
   const scenarios = [];
   for (const s of asArray(data.new_scenarios)) {
     if (!sanitize(s?.scenario)) {
       dropped.new_scenarios++;
+      detail.push({ kind: "scenario", why: "empty-scenario" });
       continue;
     }
-    if (scenarios.length < 5) scenarios.push({ scenario: sanitize(s.scenario), why: sanitize(s.why) });
+    if (scenarios.length < limits.scenarios) scenarios.push({ scenario: sanitize(s.scenario), why: sanitize(s.why) });
   }
 
-  return { summary: sanitize(data.summary, 700), risks, manual, scenarios, dropped };
+  return { summary: sanitize(data.summary, 700), risks, manual, scenarios, dropped, detail };
 }
 
 const STATE_ICON = { success: "✅", failure: "❌", cancelled: "⚪", skipped: "⚪", neutral: "⚪", timed_out: "❌", action_required: "⚠️" };
@@ -106,6 +125,9 @@ export function renderComment({ review, facts }) {
   L.push("### Possible risks in the changed lines");
   if (review.risks.length) {
     for (const r of review.risks) L.push(`- \`${sanitize(r.file, 200)}:${r.line}\`: ${r.claim}${r.evidence ? ` _(${r.evidence})_` : ""}`);
+  } else if (review.dropped.risks > 0) {
+    // Not "none found": the model raised something and it failed verification.
+    L.push(`_The model raised ${review.dropped.risks} possible issue(s) that could not be tied to a changed line in the diff, so none are shown. This is not a clean result; review the change by hand._`);
   } else {
     L.push("_None found that could be tied to a specific changed line._");
   }

scripts/qa-review-run.mjs

@@ -6,7 +6,18 @@ import { fileURLToPath } from "node:url";
 import { ghTokenForRepo } from "./github-client.mjs";
 import { chatJSONWithUsage } from "./openai-client.mjs";
 import { buildDiff, isTestFile, loadQaData, makeReader, mapAreas } from "./qa-review-context.mjs";
-import { renderComment, validateReview } from "./qa-review-render.mjs";
+import { limitsFor, renderComment, validateReview } from "./qa-review-render.mjs";
+
+const str = { type: "string" };
+const obj = (properties) => ({ type: "object", properties, required: Object.keys(properties), additionalProperties: false });
+// Strict structured output. Keep in sync with the "Output" section of prompts/qa-review.md.
+export const REVIEW_SCHEMA = obj({
+  summary: str,
+  risks: { type: "array", items: obj({ file: str, line: { type: "integer" }, claim: str, evidence: str }) },
+  manual_tests: { type: "array", items: obj({ title: str, why: str }) },
+  new_scenarios: { type: "array", items: obj({ scenario: str, why: str }) },
+});
+export const REVIEW_FORMAT = { type: "json_schema", json_schema: { name: "qa_review", strict: true, schema: REVIEW_SCHEMA } };
 
 const TAGS = "pr_title|pr_body|diff|knowledge|existing_test_cases|other_checks|areas_touched";
 
@@ -56,9 +67,11 @@ export async function runReview({ repo, prNumber, expectedSha, reader, chat, sys
     throw new Error("No reviewable diff: every changed file was skipped, binary, or over the size limit.");
   }
 
-  const { data, usage } = await chat(systemPrompt, buildUserMessage({ pr, areas, checks, qa, diff }), null);
+  const { data, usage } = await chat(systemPrompt, buildUserMessage({ pr, areas, checks, qa, diff }), null, REVIEW_FORMAT);
   if (!data || typeof data !== "object") throw new Error("Model returned unparseable output; nothing to post.");
-  const review = validateReview(data, { addedByFile: diff.addedByFile, titles: qa.titles });
+  // Size = what the model was actually shown, so a huge PR with a tiny shown part isn't over-credited.
+  const changedLines = files.filter((f) => diff.shown.includes(f.filename)).reduce((n, f) => n + (f.additions ?? 0) + (f.deletions ?? 0), 0);
+  const review = validateReview(data, { addedByFile: diff.addedByFile, titles: qa.titles, limits: limitsFor(changedLines) });
   // An empty summary means the model refused or derailed. Posting "no risks found"
   // from that would read as a clean bill of health, which is the wrong failure mode.
   if (!review.summary) throw new Error("Model returned no usable summary; refusing to post an empty review.");
@@ -83,7 +96,9 @@ export async function runReview({ repo, prNumber, expectedSha, reader, chat, sys
   // Fail safe: only an explicit `false` counts as public. The review is analysis
   // of the repo's code, and this workflow's own run summary/logs are public.
   const isPrivate = pr.base?.repo?.private !== false;
-  return { body, review, usage, isPrivate };
+  // Discard reasons quote model/PR-derived text: public repos only, never private.
+  if (!isPrivate && review.detail.length) console.log(`Discarded suggestions: ${JSON.stringify(review.detail)}`);
+  return { body, review, usage, isPrivate, changedLines };
 }
 
 async function main() {